Review PR 124 and Fix Token Refusal

can you review https://github.com/entireio/git-sync/pull/124
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:
EndpointAuthwon't return helper credentials that were looked up for a host a redirect led to.GitHubOwnerRepoonly acceptsgithub.comon its default port.guardRedirectsanchors on the API URL, so a redirect away fromapi.github.comstripsAuthorization.- 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
- The retry hides the real error. When the token is refused with 401 or 403,
lookupis reset and only the status survives asrefusedAuthStatus. A 403 from an exhausted rate limit or SAML/SSO enforcement on a private repo then shows up asgithub api returned 404: Not Found. The log hasrefused_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. - Unrelated changes in the same PR. The Go 1.27.2 toolchain bump (
go.mod,mise.toml), golangci-lint 2.15.0 and the newgoconstexclusions 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 bysourceCredentials, so the lookup goes anonymous. This is a small gap that's easy to cover. - Helper credentials stored for
github.comnow also go toapi.github.com. That's same-site and documented, but it loosens the "helper creds are bound to their host" note insamesite.go. Update that comment.
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=0andmode=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.gocomment: it now saysEndpointAuthpasses 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?
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?
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_errorlog 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
EndpointAuthnever hands over credentials looked up for a host a redirect led to. - Tests: the list includes the new tests. I re-ran
go test -raceon 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?
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 ./...andgo vetpass, and every package's tests pass except one. That one isTestGitSyncUserAgentIdentity, and it fails only because a local checkout reports its version asgit-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 arehttps://github.com/<owner>/<repo>, and requests toapi.github.comgo 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-pipelinerepo: anonymous gets 404, and Basic auth withx-access-token:<token>gets 200. I used myghOAuth 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 preflightrecord throughBootstrapLogger, carrying the worker'starget_ulidand 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.
The adversarial review of #124 is running in the background at high effort. I'll report its findings when it finishes.
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)
EndpointAuthchecks 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 toreplica.github.commakes the helper return that host's credentials, and the lookup then sends them toapi.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 thesamesite.gocomment claims.- Credentials go out with TLS verification off (
bootstrap.go:1936).HTTPConnalready 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 interceptapi.github.comgets the github.com token. Explicit tokens go out the same way. - 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.mdpromise rate-limit relief that only applies to--source-tokenor 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?