Review PR 124 and Fix Token Refusal

Claude Code·Opus 5.5·nodo·8h ago·36min·2 Checkpoints·11 file changes·+515/-212·66.4K tokens

PR #124 is ready to merge once two things are fixed: a refused-token retry that reports the wrong reason, and some unrelated toolchain changes that should be split out. I found no blocking bugs. Tests for internal/strategy/bootstrap and internal/gitproto pass locally on Go 1.27.2.

What the PR does: private GitHub repos larger than 1.5 GiB always bootstrapped as one pack, because the repo-size lookup sent no credentials and GitHub answers 404 to anonymous callers for private repos. The PR sends the source credentials to api.github.com and nowhere else. If GitHub refuses the token with 401 or 403, it retries once anonymously. Every lookup outcome is now logged.

Credential handling looks right:

  • EndpointAuth won't return helper credentials that were looked up for a host a redirect led to.
  • GitHubOwnerRepo only accepts github.com on its default port.
  • guardRedirects anchors on the API URL, so a redirect away from api.github.com strips Authorization.
  • Credentials written into the URL are copied over correctly. They would otherwise be lost, because the API request is built from a different URL.

Should fix

  1. The retry hides the real error. When the token is refused with 401 or 403, lookup is reset and only the status survives as refusedAuthStatus. A 403 from an exhausted rate limit or SAML/SSO enforcement on a private repo then shows up as github api returned 404: Not Found. The log has refused_auth_status=403, but the CLI notice doesn't. The user is pointed at "missing credentials" when the real cause is SSO or the rate limit. Keep the first attempt's error and include it in the notice and the log.
  2. Unrelated changes in the same PR. The Go 1.27.2 toolchain bump (go.mod, mise.toml), golangci-lint 2.15.0 and the new goconst exclusions have nothing to do with the fix. The toolchain changelog entry also has no PR link. Move them to their own PR so this one is easy to revert, or at least add the link.

Nits

  • An empty repo, or one GitHub hasn't sized yet, reports 0. That now logs a WARN plus a "single pack" notice, which is noisy when one pack is correct. INFO would do for that case.
  • A token written as the username with no password (https://TOKEN@github.com/...) is skipped by sourceCredentials, so the lookup goes anonymous. This is a small gap that's easy to cover.
  • Helper credentials stored for github.com now also go to api.github.com. That's same-site and documented, but it loosens the "helper creds are bound to their host" note in samesite.go. Update that comment.

(2) undo this I will fix it separately

(1) fix it fix the nits too

8h ago·3m

I fixed (1) and all three nits, and removed the toolchain commit. It's all committed locally on claude/practical-gauss-nj0xyd but not pushed yet. go test ./... and golangci-lint run (v2.11.4) both pass.

Toolchain change removed: I dropped commit 58e81acb (Go 1.27.2, golangci-lint 2.15.0, the goconst settings and their changelog line) from the branch. Because it was already pushed, updating the PR needs a force push.

(1) The refused-token error is no longer lost: when GitHub refuses the token and the anonymous retry also fails, the original refusal is kept. The log gets a new refused_auth_error field. The CLI notice now reads source credentials refused: github api returned 403: …SAML…; anonymous retry: github api returned 404: Not Found, so SSO and rate-limit problems show up as what they are.

Nits

  • Size 0: a repo GitHub reports as size 0 (empty, or not measured yet) now logs at INFO with size_kb=0 and mode=one_shot, and prints no notice. A missing size or a negative one still logs a WARN.
  • Token as the URL username: https://TOKEN@github.com/... is now sent with an empty password, the same way Go's HTTP client sends it on the git requests. If the API rejects it, the anonymous retry still runs.
  • samesite.go comment: it now says EndpointAuth passes helper credentials stored for the endpoint's site to the GitHub API on that same site.

There are new tests for each fix, and the changelog entry mentions the reported refusal. The PR description still says "A reported size of 0 is treated as unknown", so that line needs updating.

Should I force-push with git push --force-with-lease?

yes

8h ago·14.2s

I force-pushed the branch, so PR #124 now ends at 6e753384 and no longer has the toolchain commit. The push was guarded on 58e81acb, so it couldn't have overwritten anything else pushed in the meantime.

The PR description still says "A reported size of 0 is treated as unknown", which is no longer true. Do you want me to update it?

update it

8h ago·59.9s

I updated the PR description: https://github.com/entireio/git-sync/pull/124

  • Size 0: the old "treated as unknown" line now says a size of 0 selects a single pack, logged at INFO with no warning or notice. A missing or negative size still counts as a failed lookup.
  • Token refusal: the description says the refusal is reported when the anonymous retry also fails, lists the new refused_auth_error log field, and notes that the CLI notice names it.
  • Earlier commits: it now covers two changes that came in after the description was written: tokens written into the source URL are sent, and EndpointAuth never hands over credentials looked up for a host a redirect led to.
  • Tests: the list includes the new tests. I re-ran go test -race on the two affected packages and both pass, so the description's test claims still hold.

I didn't touch the Cursor Bugbot summary; the bot rewrites that section itself.

can you ensure that this change can be used in /Users/nodo/work/entire/mirror-pipeline in production?

8h ago·4m

Yes, mirror-pipeline can use this change in production without any code changes. The only step is to bump its git-sync dependency once #124 is merged.

What I checked

  • It builds and its tests pass against the PR branch. I pointed a temporary workspace file at my local checkout of the PR branch (nothing in mirror-pipeline was modified). go build ./... and go vet pass, and every package's tests pass except one. That one is TestGitSyncUserAgentIdentity, and it fails only because a local checkout reports its version as git-sync/dev. It won't happen with a real version, and the same suite passes on mirror-pipeline's current pin. The installation-token leak guard (TestSyncDoesNotLeakInstallationToken) passes.
  • The worker actually runs the lookup. The library never sets TargetMaxPack, sources are https://github.com/<owner>/<repo>, and requests to api.github.com go through the public transport, not the cluster mTLS one. entiredb-sourced syncs are not GitHub, so they skip the lookup.
  • The worker's token reaches the API. The App token is passed as Username: "x-access-token", Token: token, which becomes explicit Basic auth on the source connection, and that is what the lookup sends.
  • GitHub's REST API accepts that form. I checked live against the private entirehq/mirror-pipeline repo: anonymous gets 404, and Basic auth with x-access-token:<token> gets 200. I used my gh OAuth token, not an App installation token; the auth scheme is the same. Every GitHub App has repository metadata read, which is the only permission this call needs.
  • No regression for jobs without an installation ID. They still look up anonymously, as before.

What changes in production

  • Private GitHub repos of 1.5 GiB or more will now bootstrap in 512 MiB batches instead of one pack. This is the point of the fix, but it is new behaviour for those repos, so watch the first few large private bootstraps.
  • Every bootstrap now writes a Bootstrap GitHub size preflight record through BootstrapLogger, carrying the worker's target_ulid and owner/repo fields. Failures log at WARN, so check whether any log monitor fires on WARN from mirror-worker.
  • A failed lookup also prints one plain-text line to stderr. That's how git-sync's other notices already reach the library, but this one mentions the CLI flag --target-max-pack-bytes, which means nothing in the worker. The WARN record is the useful signal.
  • Each bootstrap attempt makes one extra API call against the installation's rate limit, with at most 30 seconds of added wait.

To ship: after #124 merges, run go get entire.io/entire/git-sync@<merge-sha> in mirror-pipeline. The PR is based on the commit mirror-pipeline already pins (013012ac), so nothing else comes with it.

can you re-run and adverarial review on the git-sync pr?

8h ago·5.4s

The adversarial review of #124 is running in the background at high effort. I'll report its findings when it finishes.

7h ago·23.3s

The review found no crash bugs, but it did find three real problems with where credentials can end up. None of them affect mirror-pipeline, which passes an explicit App token, keeps TLS verification on and uses no credential helper. The review ran at xhigh, not high: it reuses the last level and didn't pick up the high I passed. I checked #2 against the code myself.

Credential handling (should fix)

  1. EndpointAuth checks the wrong host pair (smarthttp.go:495). It asks whether the host the helper credentials were looked up for is on the endpoint's site. It never asks whether the host they're sent to may receive them. Example: a redirect to replica.github.com makes the helper return that host's credentials, and the lookup then sends them to api.github.com, which is a sibling of that host, not a subdomain. This needs redirect-following plus a helper entry for the replica host, so it's an edge case, but it breaks the rule the samesite.go comment claims.
  2. Credentials go out with TLS verification off (bootstrap.go:1936). HTTPConn already refuses to send helper credentials to another host when TLS verification is skipped (skipsTLSVerify, lines 790 and 891), but the lookup bypasses that check. With verification off, anyone who can intercept api.github.com gets the github.com token. Explicit tokens go out the same way.
  3. The rate-limit claim is overstated. The credential helper is only asked after a 401, and a public repo never returns one, so its lookup stays anonymous. The changelog and usage.md promise rate-limit relief that only applies to --source-token or tokens written into the URL.

My earlier nit fix was wrong 5. A bare username is now sent as a credential (bootstrap.go:1939). https://alice@github.com/... or git@ goes to the API as Basic alice:. GitHub answers 401, which costs a wasted request, may count as a failed login, and logs a misleading refusal. The code can't tell a token written as the username from an account name. I'd revert to requiring a password.

Logging and accuracy (minor) 4. The failure notice says "bootstrapping as a single pack", but the run can still switch to batches if the target rejects the pack. It also names a CLI flag that library callers don't have, and with -v the failure prints twice. 6. authenticated: true is set when credentials are attached, even if a redirect stripped them before they arrived. 7. The comment says control characters are stripped from GitHub's message, but sanitize.Text only filters single bytes. Two-byte control characters such as U+009B and bidi overrides still reach the terminal. 8. TestGitHubPreflightRedirectDropsCredentials also passes if the redirect target is never contacted. It should assert that the target received exactly one request.

Design 9. The policy for which credentials may go to a sibling service is split between gitproto.EndpointAuth and bootstrap.sourceCredentials, and neither checks the real destination. One method such as HTTPConn.CredentialsFor(dest), applying sameSite(origin, dest) and the TLS check to every credential source, would fix #1, #2 and #5 together. 10. githubSizeLookup stores state it could derive. The two log messages also use different names (bootstrap github preflight vs Bootstrap GitHub size preflight), so a query for one misses the other.

I'd fix #9 (which also covers #1, #2 and #5), correct the docs for #3, and do #4, #6, #7 and #8 while I'm there. Should I go ahead?

ok fix everything

7h ago·6m

Same as before (only the local-checkout git-sync/dev version artifact). Committing.

3f5458dfix: measure size-lookup credentials against their real destination Adversarial review of the GitHub size preflight found the credential policy split between gitproto.EndpointAuth and bootstrap's sourceCredentials, neither of which looked at where the request goes. HTTPConn.CredentialsFor(dest) replaces both. Each credential is measured from the URL it is bound to (EndpointURL for explicit and URL userinfo, the lookup URL for helper credentials), dest must be same-site with it, and with TLS verification off nothing crosses to another host. So: - helper credentials looked up for a redirect host no longer reach a sibling of that host; - credentials are not sent to the API with TLS verification off; - a URL username without a password is not sent (it may be an account name, which the API would only refuse); - the configured API base no longer receives credentials unless it is on the source's site. Also: - log "authenticated" from the request that produced the answer, so a redirect that stripped the header reads false; - strip C1 controls and bidi formatting from GitHub's message; - notice says the bootstrap "starts with" a single pack, and a failed lookup is no longer printed twice under -v; - one record name for the verbose log and PushLogger; - githubSizeLookup keeps the refused attempt as a nested answer; - the redirect test asserts the redirect target was reached; - docs: a public repo reached only through a credential helper is still looked up anonymously. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019JjbQK1QFpSHS4yYkrWS5Y Entire-Checkpoint: 01M5AHETQ72WQ2NWA9M65233AD+399/-196