can you fix the conflict and rebase onto main?

Done. The conflict is fixed and the branch is rebased onto main.

What happened

The rebase of soph/http-stale-pool-fix (8 commits) onto origin/main conflicted in go.mod and go.sum — both on the go-git version line:

• main had bumped to the tagged release v6.0.0-alpha.4 (via the dependabot PR #60 merge).
• my branch pinned the pseudo-version v6.0.0-alpha.4.0.20260521151600-590487407c38.

I kept the pseudo-version, because the pseudo-version's alpha.4.0.<ts>-<commit> form means its commit (go-git PR #2142, which adds WithObjectSelector) lands after the alpha.4 tag. The streaming-push code in internal/gitproto/push.go calls WithObjectSelector, so the tagged alpha.4 wouldn't compile — confirmed by the commit message ("Will switch to a tagged release once one is cut").

Verification

• No conflict markers remain
• go build ./... → OK
• go test ./internal/gitproto/ → ok

Notes

• Your uncommitted .entire/.gitignore change was stashed during the rebase and restored afterward — it's back in the working tree, unstaged, exactly as before. .clawpatch/ is still untracked.
• The branch now sits on top of main at 53a216b8. It's diverged from origin/soph/http-stale-pool-fix (the 8 commits got new SHAs), so updating the remote will need a force-push — I didn't push since you didn't ask. Want me to git push --force-with-lease?

Pushed. soph/http-stale-pool-fix on the remote now points at the rebased 2dc121df, replacing the old 7b7ff50b. The --force-with-lease succeeded, confirming no one else had pushed to the branch in the meantime.

I've reviewed both unresolved threads against the current branch tip (91a1224a, 5 commits past the 15b80ac5 the bots reviewed — both threads are marked "outdated" because the code moved). Here's my assessment.

Both comments are still relevant — one strongly, one arguably

1. "Cancel masked as HTTP 401" (auth.go) — ✅ still valid

The interface was extended since the review: Lookup now returns an err, and tryHelperRetry does propagate it (smarthttp.go:451-455 returns the error instead of falling through). So the plumbing to fix this now exists — but the implementation still doesn't use it:

GitCredentialCommand runs via exec.CommandContext(ctx, …), so a cancelled/timed-out context kills the subprocess and returns an error — which line 111 blanket-swallows into ok=false, err=nil. The //nolint:unparam // err is always nil today on line 100 confirms it's never populated. Net effect is exactly what the bot described: a cancel/timeout during git credential fill surfaces as the original HTTP 401 rather than context.Canceled/DeadlineExceeded.

Fix is now small and localized to Lookup: when helperErr != nil && ctx.Err() != nil, return ctx.Err() instead of swallowing.

2. "Premature credential helper approve" (smarthttp.go) — ⚠️ structurally still present, but low severity

The logic moved into tryHelperRetry (smarthttp.go:471-473) and the new probe path resolvePendingHelperCreds (:399-407). Both still call Approve + set c.Auth on any 2xx — and in RequestInfoRefs that runs at lines 199-205, before httpError (208), the content-type check (219-221), and the size-limit check (236-238). So a 2xx with the wrong content-type/oversized body still persists credentials while RequestInfoRefs returns an error.

My take, for nodo's "I think this is valid right?": it's real but low severity, and I'd push back slightly on the "leak" framing:

• A 2xx means the server actually authenticated the credentials — they're valid for that host, so storing them is correct and matches git (git approves at the HTTP-transport layer before parsing the protocol body).
• The credentials being approved are the same ones the helper just returned, so there's no new secret being persisted.
• The content-type/size failures are protocol-level, not auth-level.

If you want strict "approve only on a fully-validated success," the fix is to move resolvePendingHelperCreds + the tryHelperRetry approval to after the content-type and size checks. Defensible hardening, but not a security bug.

Recommendation: Fix #1 (it's a genuine, now-easy diagnostics bug). For #2, either resolve the thread with a note that 2xx == auth-success-by-design, or do the small reorder if you prefer strictness.

Want me to make either change? Note this is the soph/issue-63-deferred-credential-helper branch (PR #65), not the one currently checked out (soph/http-stale-pool-fix), so I'd need to switch branches or use a worktree first.

Both threads resolved. Summary of what landed on PR #65:

Commits pushed to soph/issue-63-deferred-credential-helper:

• 724c2df8 — auth: surface context cancellation from credential Lookup
• 6da1abe6 — gitproto: defer credential approval until response passes validation

Cursor Bugbot threads:

• Premature credential helper approve (smarthttp.go) — replied (#discussion_r3316986192), resolved.
• Cancel masked as HTTP 401 (auth.go) — replied (#discussion_r3316986538), resolved.

Verification before push: go build ./..., go vet ./..., golangci-lint run (changed packages), full go test ./... — all green. Two new regression tests pin the contracts (TestRequestInfoRefs_OnUnauthorizedRetry2xxBadContentTypeDoesNotApprove, TestGitCredentialHelper_Lookup_ContextCanceledSurfacesError).

Worktree note: I left the worktree at /Users/soph/Work/entire/devenv/git-sync-pr65 in case you want to poke at it. Once you're done, git worktree remove /Users/soph/Work/entire/devenv/git-sync-pr65 cleans it up; the branch itself stays since you pushed it.

Lint clean, stash restored.

Summary

The "conflict" was from main advancing past PR #64 (you said #63 but #63 is the issue this PR closes; #64 is the http-stale-pool-fix PR that merged). Branch was 8 commits ahead of an older merge-base; main had reworked the same gitproto files (HTTP_TRACE, withHTTPTrace, the go-git pseudo-version bump).

Conflicts resolved (in 2 of the 8 commits):

• 15b80ac5 (auth: defer credential helper until 401) — smarthttp.go RequestInfoRefs body. Main added withHTTPTrace inline; the branch extracted into doInfoRefsRequest. Resolution: took the branch's refactor (call the helper) and moved withHTTPTrace into doInfoRefsRequest, so both the initial request and the helper-retry get traced — symmetric with how doPostRPCRequest does it.

• 6247df31 (key helper on the challenged host; retry POSTs too) — smarthttp.go doPostRPCRequest. Combined the branch's ApplyAuth(req, auth) (parameterized auth so retry can use different creds) with main's httpTraceEnabled() → dumpOutgoingRequest(...) block right after.

Commits 4–8 (including the two from earlier today: auth: surface context cancellation… and gitproto: defer credential approval…) replayed without conflict.

Verification: go build ./..., go vet ./..., full go test ./... across 14 packages, golangci-lint run on the three changed packages — all green.

State: branch is now at b0ad3186, merge-base with origin/main is 957831eb (main's tip). Remote shows "ahead 22, behind 8" because all 8 branch commits got new SHAs. Working tree has your .entire/.gitignore change restored (unstaged) and .clawpatch/ still untracked.

Want me to git push --force-with-lease to update PR #65?

Branch is ready: 9 commits ahead of the old remote (8 rebased + 1 new fix), 8 behind because of the SHA changes from the rebase. Want me to git push --force-with-lease?

Summary of what's in the new commit (5ec540df)

Root cause: Go's http.Client.shouldCopyHeaderOnRedirect strips Authorization on cross-host redirects, so when an origin redirected to a challenger, our retry-against-c.EndpointURL replayed through the same redirect and arrived at the challenger naked — got 401 — and we Reject'd the user's valid creds.

Two code paths fixed:

1. tryHelperRetry — captures res.Request.URL (the actually-challenged URL after redirects) and passes it as an override target to the retry callback. The retry hits the challenger directly, no redirect, auth intact.

2. EnsureAuthForService — probe-then-Lookup-against-challengeURL (previously Lookup-then-probe-then-key-on-something-else, which led to Approve/Reject hitting a key the helper has no entry for). Drops the pre-probe Lookup optimization — necessary trade-off since the user may have creds for the challenger and not for the origin.

3. adoptChallengeHost — new helper. On either path, once auth resolves on a different host, we rewrite c.EndpointURL.Scheme/Host to the challenger. Without this, follow-up ops on the same conn would redirect again, lose auth, and the same Reject-valid-creds bug would just shift to resolvePendingHelperCreds.

Signature changes: doInfoRefsRequest and doPostRPCRequest gained an optional target *url.URL; nil = old behaviour.

Tests added:

• TestRequestInfoRefs_OnUnauthorizedAfterCrossHostRedirectRetriesAgainstChallenger — drives the full production flow with a real 307→401 chain, asserts the retry actually arrives at the challenger with Basic auth, Approve fires keyed correctly, no Reject, and c.EndpointURL adopts the challenger.
• TestEnsureAuthForService_CrossHostProbeLooksUpAndAdoptsChallenger — same idea on the probe path.

Test update: TestRun_Replicate_SubcommandExecutesAgainstEmptyTarget now expects 2 receive-pack POSTs (probe + real) since the probe is unconditional; TestMain comment in cmd/git-sync updated.

Lint clean, full suite (go test ./...) green.

Honest answer: no, not 100% safe under all threat models — but the residual risks are either pre-existing (not introduced by this fix), inherited from Go's standard policy, or require user opt-outs (TLS-off). Let me walk through what we do and don't defend against.

What the fix does guarantee

For each retry, the host the credentials are sent to is the host Lookup was queried for. The keying is consistent across Lookup → retry → Approve/Reject (smarthttp.go:639-660 for tryHelperRetry, :539-559 for EnsureAuthForService):

So if the helper has nothing stored for the actual challenge host, we bail (ok=false). The most dangerous case in the original bug — sending origin credentials to a host the user never authorized — can't happen now, because we don't lookup by origin and forward the result to the redirect target. The lookup key is the redirect target.

Threats the fix neutralizes

1. Open-redirect to an unrelated host. Origin redirects to evil.example. challengeURL = evil.example. Lookup against evil.example returns nothing (user has no creds stored there). Bail. ✓
2. The original "valid creds for replica get Reject'd" bug. Fixed.
3. Multi-hop legitimate redirect chains. Go follows them under TLS; we retry against the final URL, which is the only host that ever saw a 401. ✓

Residual risks (in increasing severity)

1. Path-scoping mismatch on the helper key — minor, same host

challengeURLFor builds the helper key as {scheme,host} from final, path from orig. So if the user stored creds for https://replica.example/path-A and the actual redirect target is https://replica.example/path-B, we'd Lookup by path-A but the retry hits path-B (since retryTarget = res.Request.URL has path-B). Same host, but creds reach a path they weren't scoped to.

In practice irrelevant for the helpers most people use (osxkeychain, wincred, libsecret, manager-core — host-scoped, not path-scoped). For path-scoped helpers it's a minor scope leak within a host the user trusted enough to store creds against. Pre-existing behavior — challengeURLFor already worked this way before this fix.

2. Go's subdomain rule for subsequent redirects within the retry — inherited from Go

tryHelperRetry's retry request goes to retryTarget with auth attached, and then Go's http.Client follows any further redirects. If replica.example redirects again to evil.replica.example, Go's shouldCopyHeaderOnRedirect allows the header on subdomain matches and the Authorization travels with it.

This is Go's stdlib policy — same behavior pre-fix and post-fix. Subdomain takeover at the challenger is the attack surface. Mitigated by: TLS cert validation on the subdomain, and by the assumption that if a user trusts replica.example with creds they trust its subdomains. If you want to be strict you'd have to set a custom CheckRedirect that bans all redirects on the retry — the user's option (b) — which is more work and may break legitimate setups (e.g., load-balancer subdomain hops).

3. SkipTLSVerify — user-opt-in footgun

If the user opts into SkipTLSVerify, an active MITM can become the redirect chain's terminus, present any cert, and we'll happily lookup-and-send creds to it (because the helper sees a host it does have creds for — replica.example — the attacker is impersonating that host). This is the standard TLS-off risk and not specific to credential helpers. The new code makes it slightly worse in one narrow sense: we now bypass Go's cross-host header-strip for the first hop. So pre-fix, MITM could intercept the initial 401 but the retry's auth got stripped before reaching them; post-fix, MITM gets the creds on the retry. Worth documenting if SkipTLSVerify is allowed in production.

4. Compromised but TLS-valid challenger — out of scope

If replica.example itself is compromised, creds the user stored for it leak. This is by design — the user has the creds stored there, the helper hands them out for that host. Pre-fix vs post-fix makes no difference here.

5. Helper returning unrelated creds — out of scope

A misbehaving credential helper that returns creds for hosts other than the query is a helper bug. We trust the helper API contract. Same risk pre-fix and post-fix.

What we don't guarantee anymore (deliberate change)

Pre-fix, Go's cross-host header strip was an additional defense even against a misconfigured helper key. The previous code accidentally relied on it: even when our keying was wrong (in the EnsureAuthForService inconsistency I called out earlier), Go's strip prevented the creds from actually leaving. The fix removes that "safety net" for the origin→challenger hop, because we have to in order to make auth work. Defense now rests entirely on the keying being correct + TLS being on.

Practical recommendation

The fix is safe provided:

• TLS verification is on (i.e., SkipTLSVerify is not enabled, or is limited to dev/test).
• The credential helper behaves per spec (returns creds for the queried host, not others).

Both of those are baseline assumptions the rest of the system already makes. So in practice, yes — this is safe for the intended deployment shape.

If you want to tighten the SkipTLSVerify interaction, one option is to refuse adoptChallengeHost (and refuse the cross-host retry) when SkipTLSVerify is true. That preserves the old "Go strips auth, but at least it doesn't leak to a MITM" behavior at the cost of leaving the user with a 401 they have to resolve manually. Let me know if you want me to add that gate.

No response requested.

Working as designed — and the design is the issue #63 fix this very branch shipped.

auth.newGitCredentialCmd runs git credential fill with GIT_TERMINAL_PROMPT=0:

So when the 401 from …artifacts.cloudflare.net triggers tryHelperRetry, the helper subprocess runs but git is forbidden from falling back to a terminal prompt. If no configured helper (osxkeychain, libsecret, manager-core, …) has an entry for that host, the subprocess exits without creds → Lookup returns ok=false → we surface the 401 rather than block on stdin. That's the whole "match git's behaviour without becoming interactive" point of PR #65.

Vanilla git push to the same URL would prompt, because git in normal use is interactive. git-sync deliberately isn't.

To unblock yourself

Pre-seed the helper with creds for that exact host once, then re-run:

After that, your configured helper (Keychain on macOS) will return those creds to git-sync's Lookup and the retry will go through.

Or if git-sync has a flag for explicit auth (--token / --bearer-token / whatever the cobra command exposes on the target side — I haven't checked which is wired up for the sync subcommand), pass the Cloudflare token directly and skip the helper entirely. Want me to look up which flag applies?

The full picture from #63: it was a public GitLab repo prompting for auth interactively because auth.Resolve was proactively invoking git credential fill. The user had no creds, pressed Enter twice, and the public clone went through — but the prompt itself was the bug. Fix: don't ask the helper unless the server actually returns 401, and even then don't allow a tty fallback.

So the design choice traces to that issue. Two assumptions baked in:

1. git-sync is non-interactive by contract. It needs to work from cron, CI, daemons, the syncer's background loop — anywhere there's no controlling terminal. A tty prompt would hang forever there.
2. Credentials come from somewhere ambient (an OS keychain helper, an Entire DB token, or an explicit --*-token CLI flag). They are never meant to be solicited at runtime.

Your current case (a private Cloudflare artifacts URL the helper doesn't know about yet) is the UX edge of that contract: there's no "first-time seed" path inside git-sync. You're expected to either:

• Pre-seed the helper out-of-band (e.g. git credential approve once, or a vanilla git ls-remote to the URL which does prompt and then stores).
• Pass auth via a CLI flag — if the sync command exposes a --target-token / --target-bearer-token for the target side. (The cobra wiring is in cmd/git-sync/ — I haven't traced exactly which flags are wired up there. Want me to check?)

If this UX cliff is biting you in practice it might be worth revisiting — e.g. an opt-in --interactive flag that flips GIT_TERMINAL_PROMPT=1 for one-shot terminal runs while preserving the non-interactive default for automation. But that'd be new work, not a bug in the current behaviour.

Committed (566de827). To answer your original question one more time concretely:

You should now get a prompt when you re-run

from a terminal where GIT_TERMINAL_PROMPT isn't set to 0. The 401 from the Cloudflare artifacts URL → tryHelperRetry → Lookup → git credential fill will fall through to a tty prompt (since your keychain has no entry for 8b04…artifacts.cloudflare.net yet), you enter the creds once, and the keychain remembers them. Same shape as the first time you push to any new host with vanilla git.

CI and the syncer background loop are unaffected: they already pass GIT_TERMINAL_PROMPT=0 (or should, same as plain git), and we now pass that through rather than overriding it.

Let me unpack nodo's three points one at a time, since the first one is already addressed but they didn't see the fix:

1. The Lookup(c.EndpointURL) → keyed-on-challengeURL mismatch

The link they sent (6da1abe6) is from before my cross-host fix landed. That exact bug was the second concern I called out earlier; commit 5ec540df already rewrote EnsureAuthForService to probe first, then Lookup(challengeURL):

So Lookup key, pendingHelperCreds.url, and the subsequent Approve/Reject target all match. Worth pointing nodo at 5ec540df.

2. "challengeURLFor may amend the URL even though it does not check FollowInfoRefsRedirect. … target URL should be brought along, instead of updated on demand here."

This one I think nodo is right about — and we should change it. What I currently do is mutate c.EndpointURL.Scheme/Host via adoptChallengeHost whenever auth resolves against a cross-host challenger. Two problems with that:

• Semantic blur. c.EndpointURL started as "what the user typed." After adoptChallengeHost runs it's "the redirect target we discovered." Display/logging/telemetry that reads c.EndpointURL for the user-facing identity now sees something the user didn't type. FollowInfoRefsRedirect already does this same blur (consistency was my rationale), but at least there it's gated by an explicit opt-in flag. My path mutates regardless.
• Implicit redirect-following. A user who hasn't set FollowInfoRefsRedirect is saying "talk to the host I typed." We're effectively overriding that whenever auth happens to resolve via a redirect. nodo's right that this conflates two distinct user intents.

Two ways to fix, in increasing scope:

(a) Gate adoptChallengeHost on FollowInfoRefsRedirect. Minimal change. Without the flag, the retry still goes to the challenger (so the immediate op succeeds), but c.EndpointURL isn't mutated — and the very next op redirects, gets stripped, surfaces 401. Honest to the user's stated intent: "you said don't follow redirects, so we don't carry you across them." User can opt in by setting the flag.

(b) Stop mutating c.EndpointURL entirely; introduce c.resolvedTarget *url.URL (or similar) that URL-building reads when set. FollowInfoRefsRedirect would write to that field instead of mutating EndpointURL too, for consistency. Cleaner separation, but more files touched and more carrying-along through doInfoRefsRequest/doPostRPCRequest.

My lean: (a) for now. It's a one-line gate inside adoptChallengeHost (and one new test), respects nodo's intent, and keeps the existing FollowInfoRefsRedirect semantics intact. (b) is a worthwhile cleanup but a separate piece of work.

3. "FollowInfoRefsRedirect follows all redirects. Upstream by default only follows the initial request."

This is a sharper observation about default behavior. Vanilla git's HTTP transport sets http.followRedirects=initial by default — it follows the /info/refs redirect but not subsequent POST redirects (the user has to set http.followRedirects=true for that). Our Go http.Client follows all redirects unconditionally.

That's a divergence from git that's broader than just the auth case — it affects every POST on a redirecting endpoint, not just helper-auth flows. Fixing it means installing a custom CheckRedirect on the conn's http.Client that mirrors git's policy. Out of scope for this PR but worth filing as a follow-up if you want behavioral parity with git.

Want me to implement (a) (gate adoptChallengeHost on FollowInfoRefsRedirect) now, and draft a reply to nodo that walks through all three points?

Let me show both as concrete diffs against the current code.

Option (a) — gate adoptChallengeHost on FollowInfoRefsRedirect

The change is one new if-check at the top of adoptChallengeHost:

Walking through a redirecting setup (github.com → replica.example, user has creds for replica):

Pros: one-line change, semantically honest. "You said don't follow redirects → we don't carry you across them."

Cons: the false-flag user sees a confusing partial success — info/refs worked, push 401'd. They have to read the docs or error message to know they need to set the flag. We can mitigate that with a clear error message when we detect this pattern, but it's still a footgun.

Option (b) — stop mutating c.EndpointURL, introduce a separate field

The reviewer's preferred shape, but bigger in scope. The idea: c.EndpointURL becomes immutable user input; a new field tracks the resolved working endpoint discovered at runtime.

The field and accessor:

Every URL-building call site changes from c.EndpointURL.String() to c.requestURL().String():

Three such call sites (doInfoRefsRequest, doPostRPCRequest, doServiceProbe).

adoptChallengeHost and the FollowInfoRefsRedirect block both set the field instead of mutating:

Crucially — even in (b), adoptChallengeHost still needs the FollowInfoRefsRedirect gate. The separate field is purely about where the discovered endpoint lives; the decision whether to follow the cross-host challenge at all is still the user's opt-in. (b) doesn't replace (a), it builds on it.

What (b) buys you over (a):

• c.EndpointURL is now a stable identity — anything that logs "syncing from <EndpointURL>" tells the same story before and after auth resolves.
• Tests can assert "where did we end up?" via c.resolvedEndpoint distinctly from "what did the user ask for?" via c.EndpointURL.
• Future code paths that need the user-typed identity (e.g., metrics tagging, error messages, the credential helper key) can't accidentally pick up the post-redirect value.
• Symmetric treatment of FollowInfoRefsRedirect's existing mutation, which has the same semantic-blur problem.

What it costs:

• New field, new accessor, a few call-site changes.
• The existing TestRequestInfoRefs_FollowInfoRefsRedirect_SubsequentPOSTHitsRedirectedHost test and our new TestRequestInfoRefs_OnUnauthorizedAfterCrossHostRedirectRetriesAgainstChallenger both currently assert conn.EndpointURL.Host == replica.example — those flip to asserting conn.resolvedEndpoint.Host == replica.example (or use an accessor). Small churn, but in tests that nodo cares about.
• External readers of conn.EndpointURL (the syncer's hostnameFromURL for display, anything that calls conn.Endpoint()): now correctly return the user-typed URL, but if any of them wanted the resolved one for some reason, they'd need to switch. I'd want to grep for those before committing.

So, succinctly

• (a) is the gate. "Don't mutate endpoint unless the user opted into redirect-following." Minimal, addresses the implicit-redirect concern, leaves the mutation pattern in place.
• (b) is the gate plus a refactor. Same gate, but now the discovered URL lives in a separate field instead of overwriting user input. Better state hygiene, more files touched, also fixes the existing FollowInfoRefsRedirect's analogous semantic blur.

If we do nothing else, (a) alone fully answers nodo's "doesn't check FollowInfoRefsRedirect" point — the mutation now does check it. (b) is the more principled fix and arguably what the codebase wants long-term, but it's a "while we're in here" cleanup, not strictly required to address the feedback.

Does that clarify which lever does what? Happy to drill into either one further.

Right — let me implement (b). I'll do it in clean stages so I can verify after each.

Stage 1: add the resolvedEndpoint field and the requestURL() accessor:

Committed (c01a2da7). Branch is ahead 1 of origin.

Summary of what changed

c.resolvedEndpoint *url.URL is the new field. When set, it supersedes c.EndpointURL.Scheme/Host for outgoing requests (via the new c.requestURL() accessor). c.EndpointURL itself is never mutated anymore — anything that wanted "what the user typed" (the SSH-scheme check, the github.com check, display/logging) keeps getting the original.

adoptChallengeHost is now gated on FollowInfoRefsRedirect, per nodo's point that the auth path shouldn't implicitly follow redirects without the user's opt-in. With the flag off, the immediate retry still hits the challenger directly (so the production bug — Reject'ing valid creds — stays fixed), but c.resolvedEndpoint doesn't get populated and follow-up ops stay pointed at the user's URL.

The existing FollowInfoRefsRedirect block in RequestInfoRefs also stops mutating EndpointURL and writes to resolvedEndpoint instead — same semantic blur nodo flagged on the auth path applied here too, so it gets the same treatment.

The two TLS-off gates now compare challengeURL.Host against c.requestURL().Host rather than c.EndpointURL.Host. Same effective behavior on a fresh conn; correctly handles a conn that has already adopted a previous redirect.

Tests: the two adoption tests now set the flag and assert resolvedEndpoint; the DoesNotFollowByDefault and both TLS-off tests added a resolvedEndpoint == nil assertion; a new TestRequestInfoRefs_CrossHostRetryWithoutFollowFlagSucceedsButDoesNotAdopt covers the gated half (flag off, retry succeeds, no adoption).

Want me to push, and draft the reply to nodo summarizing what c01a2da7 does?

Pulling the still-open list from this PR review thread:

Open

1. Path-aware credential helper doc note on challengeURLFor — from our earlier conversation. We agreed on option A: no behavior change, just a one-line note in the challengeURLFor doc comment explaining the path-keying choice (key on orig.Path so user-stored creds remain findable, at the cost of helper-state imprecision for path-aware helpers when the redirect rewrites paths). I never landed that note. Small, ~5 minutes.

2. nodo's third point — FollowInfoRefsRedirect follows all redirects — Go's http.Client follows all redirects by default; git's HTTP transport only follows the initial /info/refs (the rest requires http.followRedirects=true). Our behavior is more permissive than git. Fixing it means installing a custom CheckRedirect on the conn's HTTP client to mirror git's policy. Larger and broader than this PR — affects every POST on a redirecting endpoint, not just the auth flow. My take: file as a follow-up issue rather than expand this PR.

3. Push c01a2da7 — local-only right now. The remote PR still doesn't show the option-(b) refactor.

4. Reply to nodo on the PR — they were reading commit 6da1abe6 when they wrote the feedback, so they haven't seen 5ec540df (the Lookup-keying fix) or c01a2da7 (the no-more-mutation + gate refactor). Worth a thread reply that walks through both commits and addresses each of their three points directly.

Closed earlier in this thread

• ✅ Lookup-against-EndpointURL-but-key-on-challengeURL mismatch → fixed in 5ec540df (probe first, then Lookup(challengeURL)).
• ✅ "Target URL should be brought along, instead of updated on demand" → fixed in c01a2da7 (separate resolvedEndpoint field).
• ✅ Implicit-redirect-following on the auth path → fixed in c01a2da7 (gated on FollowInfoRefsRedirect).

My suggestion: knock out (1) as a tiny doc-only commit, push (3), then draft the nodo reply (4) — and file (2) as a separate issue so it's tracked but doesn't expand this PR's scope. Want me to do that sequence?

All three done:

1. 27e145fd — gitproto: document the orig-path keying choice on challengeURLFor (doc-only).
2. Pushed 566de827..27e145fd, so the remote now has both c01a2da7 (the resolvedEndpoint refactor from earlier) and the new doc note.
3. Issue #67 filed — "gitproto: align HTTP redirect-following with git's default (only /info/refs, not POSTs)" — captures nodo's third point with background, why-it-matters, proposed shape, scope, and links back to the relevant PR #65 commits for context.

PR #65 is now caught up with all the in-scope review feedback; #67 carries the out-of-scope piece forward.