can you take a look at https://github.com/entireio/git-sync/issues/63

Claude Code·Opus 4.7[1m]·

Soph·4mo ago·144hr 20min·6 Checkpoints·25 file changes·+1739/-525·257.6K tokens

All tests pass. Here's the summary:

Bug: When git-sync sync is run against a public source repo (e.g. https://gitlab.baylibre.com/...), it would interactively prompt for username/password on a host the user had never authenticated against.

Root cause (internal/auth/auth.go:69): git credential fill falls back to an interactive terminal prompt on /dev/tty when no configured helper has credentials. git-sync is non-interactive, so this prompt shouldn't happen — anonymous access should be attempted instead.

Fix: Set GIT_TERMINAL_PROMPT=0 on the git credential fill invocation. With this, when no helper has credentials, git exits non-zero, our existing code treats that as "no credentials found", and we proceed anonymously. I also extracted newCredentialFillCmd so the env-var contract is unit-testable (TestCredentialFillCmdDisablesTerminalPrompt).

Want me to open a PR for this against issue #63?

So you have two options:

Option A — ship the current fix. Bug #63 (interactive prompt) is fixed. Cost: we still call git credential fill proactively, so if you have stored credentials for a host, we'll attach them on requests that didn't need auth. For most users this is harmless, but it does mean we send a token to a public repo we didn't need to.

Option B — match git's actual behaviour. Change Resolve() to skip the helper lookup, then make HTTPConn retry on 401 by consulting the helper at that point. Outline:

auth.Resolve() returns nil when only the helper would have credentials (don't call it eagerly)
Add a CredentialProvider callback on HTTPConn that calls git credential fill
In RequestInfoRefs and PostRPC, on a 401 response, call the provider, retry once with the returned creds
On retry success, also call git credential approve (to teach the helper the creds worked); on persistent 401, git credential reject

That's roughly a 100-line change touching auth/, gitproto/smarthttp.go, and a few tests. Slightly more if we want to handle 401s mid-stream during the pack post (which we should, since some servers only challenge on git-upload-pack not /info/refs).

My instinct: the bug filer just wants the prompt to stop. Option A solves that today. Option B is the better long-term shape and worth filing as a follow-up, but I'd rather propose it cleanly than wedge it into this PR. Want me to:

Ship Option A and open a follow-up issue for Option B, or
Do Option B now as part of this fix?

Here's the summary of what we built:

Behaviour change: git-sync now matches git's own HTTP auth behaviour — try anonymous first, only consult the credential helper on a 401 response from the server.

Files changed:

internal/auth/auth.go — Resolve() no longer eagerly calls git credential fill. New GitCredentialHelper type with Lookup/Approve/Reject methods that shell out to git credential with GIT_TERMINAL_PROMPT=0 so it never prompts.
internal/gitproto/smarthttp.go — New CredentialHelper interface on HTTPConn. RequestInfoRefs retries once on 401, stores resolved auth for subsequent calls (e.g. PostRPC), and signals approve/reject back to the helper.
internal/syncer/syncer.go — Wires auth.GitCredentialHelper{} onto HTTP conns when no explicit auth was configured.
Tests: 7 new HTTPConn 401-retry tests + 8 new auth helper tests + updated TestResolve to assert the helper is never consulted up-front. The pre-existing TestRun_IntegrationUsesGitCredentialHelperFallback end-to-end test still passes — it now exercises the new 401-retry path against a real httptest server.

Net effect: For the user in issue #63, syncing from a public GitLab repo now works without prompting — git-sync makes the request anonymously, the server returns 200, no helper is ever consulted. For users with private repos and a configured helper, the helper is consulted only when needed.

Should I commit this and open a PR for #63?

One thing we could add here: Let's say I talked to a host that needs credentials and I enter the right ones, then it's saved in the helper, now if they expire the next time I run git-sync I get a 403.

Where to wire it in git-sync

The 401 surfaces in httpError (internal/gitproto/smarthttp.go). But the auth provenance lives up in internal/auth/auth.go. Cleanest plumbing:

lookupGitCredential returns a func() error (the "reject closure") alongside the username/password, capturing the input it sent to fill
The session-level code wraps the AuthMethod with that closure
On 401, the session calls the closure (which runs git credential reject for the same input)

4mo ago·22m

All green. Here's what we added on top of the earlier work:

The 403 case: in the retry-result handling, treat 401 OR 403 as "creds didn't validate, reject them". The retry trigger stays at 401 only — so a 403 on a first request (typically "this repo is forbidden") doesn't risk dropping good credentials. But once we're already in the retry path (meaning the server told us auth is required), a 403 on the retry means the helper's creds were stale, and we forget them. Cloudflare's "Invalid or expired token" path is now self-healing across runs.

New test: TestRequestInfoRefs_OnUnauthorizedRetry403CallsReject.

Want me to commit and open a PR for #63 now?

Sure. The codes mean different things by spec, but real-world servers play fast and loose with that distinction.

By the spec:

401 Unauthorized: "I don't know who you are. Send credentials." The server is challenging — and is supposed to include a WWW-Authenticate header telling you how to authenticate.
403 Forbidden: "I understand who you are (or don't care), but I refuse this request." No challenge to retry with different creds — it's a flat no.

In a clean world: an anonymous request to a private repo → 401. An authenticated request to a repo you can read but not push to → 403. The status code tells you whether trying different credentials would help.

In practice it's muddier. Some token-based services collapse the distinction:

Cloudflare's git artifact registry: invalid/expired token → 403 "Invalid or expired token"
GitHub: bad PAT → 401, but a PAT that's SAML-locked → 403
Some self-hosted servers send 403 for every auth failure

So 403 ends up overloaded with two meanings:

"Your credentials are bad" — should reject them
"Your credentials are fine, but you can't have this resource" — should preserve them

You can't reliably tell them apart from the status code alone. But you can tell them apart from what came before, which is the heuristic we used:

Scenario	Initial response	Retry response	What it means	What we do
Public repo	200	—	Anon worked	No helper consult
Private repo, fresh creds	401	200	Auth worked	Approve creds
Private repo, bad creds	401	401	Server rejects creds	Reject creds
Private repo, expired token (Cloudflare-style)	401	403	Server rejects creds	Reject creds ← new behaviour
Forbidden repo (archived, no access)	403	—	Server doesn't care about creds	No helper consult, surface error

The key insight: once the initial response was 401, we know the server gates this resource on auth. A subsequent 403 in that conversation can only mean "the specific creds we just sent aren't acceptable" — not "this resource is forbidden in general", because that would have come back as 403 on the initial anonymous request, not 401.

That's why I extended the reject path to 403 but not the trigger path. If a server 403s on the very first request, we don't touch the helper at all — that's the "forbidden, not auth" lane. Only after we've been told "you need auth" (the 401) do we interpret a follow-up 403 as a credential rejection.

Code Review and Cleanup

Review all changed files for reuse, quality, and efficiency. Fix any issues found.

Phase 1: Identify Changes

Run git diff (or git diff HEAD if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation.

Phase 2: Launch Three Review Agents in Parallel

Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.

Agent 1: Code Reuse Review

For each change:

Search for existing utilities and helpers that could replace newly written code. Look for similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones.
Flag any new function that duplicates existing functionality. Suggest the existing function to use instead.
Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.

Agent 2: Code Quality Review

Review the same changes for hacky patterns:

Redundant state: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls
Parameter sprawl: adding new parameters to a function instead of generalizing or restructuring existing ones
Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction
Leaky abstractions: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries
Stringly-typed code: using raw strings where constants, enums (string unions), or branded types already exist in the codebase
Unnecessary JSX nesting: wrapper Boxes/elements that add no layout value — check if inner component props (flexShrink, alignItems, etc.) already provide the needed behavior
Nested conditionals: ternary chains (a ? x : b ? y : ...), nested if/else, or nested switch 3+ levels deep — flatten with early returns, guard clauses, a lookup table, or an if/else-if cascade
Unnecessary comments: comments explaining WHAT the code does (well-named identifiers already do that), narrating the change, or referencing the task/caller — delete; keep only non-obvious WHY (hidden constraints, subtle invariants, workarounds)

Agent 3: Efficiency Review

Review the same changes for efficiency:

Unnecessary work: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns
Missed concurrency: independent operations run sequentially when they could run in parallel
Hot-path bloat: new blocking work added to startup or per-request/per-render hot paths
Recurring no-op updates: state/store updates inside polling loops, intervals, or event handlers that fire unconditionally — add a change-detection guard so downstream consumers aren't notified when nothing changed. Also: if a wrapper function takes an updater/reducer callback, verify it honors same-reference returns (or whatever the "no change" signal is) — otherwise callers' early-return no-ops are silently defeated
Unnecessary existence checks: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error
Memory: unbounded data structures, missing cleanup, event listener leaks
Overly broad operations: reading entire files when only a portion is needed, loading all items when filtering for one

Phase 3: Fix Issues

Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it.

When done, briefly summarize what was fixed (or confirm the code was already clean).

4mo ago·2h 18m

Pushed to PR #65. Cleanup summary:

Applied (from reuse + quality reviewers):

Dropped the duplicate basicAuth struct; use transporthttp.BasicAuth directly in the retry path
Collapsed GitCredentialHelper.Approve/Reject into a single signal(op, ...) helper
Typed credential op names as auth.CredentialOp with CredentialOpFill/Approve/Reject constants — removes magic strings everywhere including the integration test switch
Collapsed fakeCredentialHelper's 6 counter/last fields into a single calls []credCall slice with count(op)/last(op) accessors
Added newTestConn(t, rt) helper — saves ~6 lines × 8 tests
Ruthless comment trim: removed 7 narrating/restatement comments; kept the 4 that encode non-obvious WHY (CredentialHelper interface contract, GIT_TERMINAL_PROMPT=0 rationale, 403-Cloudflare anecdote, explicit-auth-wins invariant)

Skipped as not worth the churn:

Extracting retryWithHelper to flatten nesting — efficiency reviewer verified the body lifecycle is correct in all 7 paths, and extraction risks reintroducing the close-juggling
Collapsing CredentialHelper to Lookup + Result(ok bool) — speculative refactor, the three-method interface mirrors git's own protocol cleanly
Migrating older inline http.Response literals to use the new newAdvertisementResponse/newUnauthorizedResponse helpers — out of scope drive-by

Efficiency reviewer: signed off clean, no changes needed.

Net result on the cleanup commit: -291 / +210 lines (mostly test consolidation).

Medium: credential-helper lookups are keyed to the original remote, not the host that actually returned 401, so redirected smart-HTTP setups can miss or poison credentials. In internal/gitproto/smarthttp.go:191, the retry path calls Lookup/Approve/Reject with c.EndpointURL. After an http.Client redirect, the challenged URL is res.Request.URL, which may be a different host. That means a 307 to an auth-gated replica will ask the helper for creds for entry.example instead of replica.example, and a successful retry would approve them under the wrong key.
Medium: helper fallback only exists on GET /info/refs; authenticated POST RPCs still fail hard if discovery is anonymous but the pack request is not. internal/gitproto/smarthttp.go:182 contains the only 401 retry path, while internal/gitproto/smarthttp.go:290 just applies c.Auth and returns httpError(res). That breaks servers that allow anonymous advertisement but require auth on upload-pack/receive-pack POSTs, which is a valid smart-HTTP behavior.

4mo ago·15m

Pushed to PR #65 as a third commit. Summary of the two fixes:

Finding 1 — helper keyed to wrong host after redirect

Added challengeURLFor(orig, res): takes the post-redirect scheme+host from res.Request.URL but keeps the original repo path. The helper now sees the URL of whatever actually issued the 401, not whatever was originally configured.
New test: TestRequestInfoRefs_OnUnauthorizedAfterRedirectKeysHelperOnFinalHost simulates a request that lands on replica.example (via res.Request.URL) after starting at example.com, and asserts the helper Lookup/Approve are keyed on replica.example.

Finding 2 — POST RPCs didn't retry on 401

Extracted the lookup → retry → approve/reject logic into a shared tryHelperRetry(ctx, res, retry) method.
RequestInfoRefs now delegates to it.
PostRPCStreamBody now also delegates to it, gated on the body being io.Seeker (so we can Seek(0, 0) before the retry). PostRPC and PostRPCStream already pass bytes.NewReader, which is seekable, so they benefit automatically. A raw non-seekable Reader from a direct PostRPCStreamBody caller surfaces the 401 unchanged — documented in the doc comment.
3 new tests: TestPostRPC_OnUnauthorizedRetriesWithHelperCredentials, REDACTED, TestPostRPC_OnUnauthorizedRetryStill401CallsReject.

Full suite + race detector + lint clean.

Medium: the new POST retry logic still does not cover the real receive-pack push path, because that body is built with io.MultiReader and is therefore not rewindable. In internal/gitproto/smarthttp.go:279, helper retry only runs when body implements io.Seeker. But the actual push request in internal/ gitproto/push.go:153 constructs body = io.MultiReader(body, packData), which will not satisfy io.Seeker. So a target that allows anonymous receive-pack discovery but requires auth on the POST will still fail on the first push attempt with no helper retry. The new tests exercise PostRPC(..., []byte("0000"), ...), which is rewindable, but not the production push body shape.

4mo ago·9m

Pushed. Summary:

The gap: internal/gitproto/push.go:153 builds the receive-pack body as io.MultiReader(header, packData). packData is the live pipe from upload-pack — it's a one-shot stream, not seekable. So even with my previous io.Seeker gate, the production push path got no in-flight 401 retry.

The fix: HTTPConn.EnsureAuthForService(ctx, service) issues an anonymous GET /<service> probe before the push body is constructed. If the server 401s, the helper resolves credentials, retries the probe with them, and stores the result on c.Auth — so the streaming POST that follows is pre-authenticated. push.go now calls it before constructing the MultiReader body.

Probe semantics: any non-401/403 response with credentials attached counts as "server accepted these creds". 405 Method Not Allowed is the typical shape for a GET to /git-receive-pack on a smart-HTTP server, and it cleanly signals "creds were fine, we just don't allow GET here". 2xx and 404 are treated the same. 401/403 → reject.

Known remaining limitation (documented in the doc comment): a server that allows GET anonymously but only 401s on POST will slip past this probe. For that, the user has to pass --target-token. This is the same limitation vanilla git has with curl-based streaming uploads — can't fix without buffering the whole pack.

4 new tests: TestEnsureAuthForService_ResolvesAuthBeforePost, TestEnsureAuthForService_NoHelperIsNoOp, TestEnsureAuthForService_AnonymousServiceLeavesAuthNil, TestPostRPCStreamBody_NonSeekableBodyAfterProbeAuth. The last one specifically simulates the production push shape — io.MultiReader body, server 401s on GET, probe resolves auth, real POST has the credential attached.

Medium: EnsureAuthForService can incorrectly bless stale helper credentials based on a GET /git-receive-pack response that says nothing about whether auth was actually accepted. In internal/gitproto/smarthttp.go:368, any second probe result other than 401/403 causes c.Auth to be set and CredentialHelper.Approve(...) to run, including 405 and 404. But GET /git-receive-pack is not the real operation, and many servers can return 405 Method Not Allowed before validating credentials at all. In that case, stale helper creds get approved and cached, and the real push path in internal/gitproto/ push.go:158 sends the non-rewindable POST with bad c.Auth; because c.Auth is now set, the normal retry path in internal/gitproto/smarthttp.go:278 is skipped. Net effect: the probe can turn “unknown creds” into “trusted creds” and make recovery worse.

4mo ago·21m

Pushed. Summary of the correctness fix:

The poisoning hazard you flagged: a server returning 405 (or 404, 500, etc.) to GET /git-receive-pack doesn't tell us anything about credential validity — many servers reject the method before they check Authorization. My previous code treated any non-401/403 response with creds attached as "approved", which would bless stale credentials in the helper and then skip the on-the-fly retry path for the real POST because c.Auth was now set.

New contract: the probe is strictly anonymous, single-shot. Its only job is to detect whether the server requires auth via a 401. If yes, the helper-supplied credentials are attached to c.Auth tentatively and recorded as pendingHelperCreds. The real operation then calls resolvePendingHelperCreds(res):

2xx → Approve + clear pending (real operation validated the creds)
401/403 → Reject + clear c.Auth + clear pending (real operation rejected the creds — helper learns its state was stale)
Other status → leave pending (ambiguous, don't commit either way)

So helper state only changes when the real operation gives a definitive signal — matching what git itself does (approve after a successful operation, reject after auth rejection, never approve from a method-check probe).

Six tests cover the new contract, including an explicit regression test TestEnsureAuthForService_405ProbeWithCredsDoesNotPoisonHelper for the exact bug you flagged.

Medium: streaming receive-pack pushes still cannot use helper fallback on servers that only challenge the real POST, not the probe GET. In internal/ gitproto/smarthttp.go:343, EnsureAuthForService only attaches helper creds when GET /<service> returns 401. If that probe returns something anonymous like 404 or 405, conn.Auth stays nil. Then the real non-seekable push body in internal/gitproto/push.go:158 goes through internal/gitproto/smarthttp.go:275, gets a 401 on the actual POST, and cannot retry because the body is io.MultiReader. The code documents this limitation, but it is still a behavioral gap for valid server layouts where auth is enforced only on POST /git-receive-pack.

4mo ago·34m