Fix async mirror-create review findings from #2207

[Soph]

https://entire.io/gh/entireio/cli/trails/1221

Why

Review of #2207 (async mirror-request routes) surfaced 15 findings after it merged. This fixes them.

Rebased onto main since it was opened, which changed the stakes twice. #2246 made the async route the default, so none of these are behind an opt-in flag any more — the suspended-placement exit-0, the cross-jurisdiction polling failure, and the unbounded synchronous timeout are all live. And 02abf214b moved the cross-jurisdiction transport into the shared auth-go/crossjuris package, so the Location-canonicalization this PR removes now lives in cross_juris_client.go rather than the deleted cross_juris_transport.go.

What

The Location header is no longer used at all. awaitMirrorPlacement drives the poll off the 202 body's required requestId, which removes three findings together. Location is optional in the spec, so requiring it failed a create whose body already reported a succeeded placement with a mirror id and URL. Its host must never become a poll target either: mirrorRequestPollTarget accepted http://<any host> (unlike isSafeOrigin, which allows plaintext only for loopback precisely because these paths re-target the login JWT). And the WithServerURL override it fed was actively harmful: short-circuiting the poll at the home core skips the 421, and the follower treats a bare 401 as an exchange trigger only on a hop it reached by following one — so a placement that stayed pending past the token cache died with a 401 while the placement was fine. Strictly worse than not optimising, since awaitMirrorReady was immune only because it never got the override. Polls now stay on the client's base URL and 421 on every tick.

mirrorLocationCanonicalizer goes with it, and this is the one part worth a second opinion. It survived the auth-go refactor and was rebuilt there as a wrapper RoundTripper that rewrites onto resp.Request so it works after the replay — real work, deliberately preserved. But its only consumer was the poll target this PR removes, so it is now maintaining a header nothing reads, derived from a value the server supplies. Reviving it means giving a server-named host a say in where the control-plane bearer goes, which is what the follower's federation check exists to gate. Happy to keep it if it has a use I have not found — @pfleidi, this is the hunk to look at.

Route parity for suspended placements. MirrorRequestResult has no suspended field, while CreatedMirror does and every downstream branch reads it — so create --no-wait against an admin-suspended placement printed "Initial clone may still be in progress" and exited 0, where the sync route warns and exits non-zero. A script chaining create --no-wait && git clone would proceed and fail at the clone. applyAsyncSuspension does one status read after placement so both routes reach the same outcome shape. The durable fix is adding suspended to MirrorRequestResult server-side, after which the helper should go; its doc comment says so.

--wait-timeout now means one thing. It bounded only the clone poll on the sync route, so a hanging CreateMirror ignored the flag entirely. Main's help text documented that split; this makes the deadline cover submission, placement and clone on both routes instead, and a sync create that expires reports timed out registering the mirror rather than a raw transport error. TestRepoMirrorCreate_WaitTimeoutHelp is updated to pin the symmetric promise.

An accepted request id survives a wait with no verdict — a timeout or an exhausted poll budget previously returned mirrorCreateOutcome{}, discarding the only handle on work that may still be progressing, and there is no mirror request get subcommand to look one up. A terminal failure deliberately does not print that notice: the server is done, and pairing repo_inaccessible with "re-run it to pick it up" sends the user in circles. That distinction is Bugbot's finding on the first revision, now carried by a mirrorPlacementFailedError type.

Smaller behavioural fixes. The wizard classifies a placement timeout as timed out instead of error, so the same condition is not rendered two ways depending on which half of the timeout window it landed in. One relabelled spinner (startUpdatableSpinner, which exists for this) replaces three per-phase spinners that each stamped their own check mark — three success claims for one operation, including in CI logs, and a tick on a phase merely superseded by the next. And ENTIRE_ASYNC_MIRROR_REQUESTS overrides the route in both directions: repo mirror create names a repo the caller usually has not cloned, so a switch readable only from the cwd's .entire/settings.json has no effect exactly where the command is most used. The settings-load error is reported instead of swallowed, but does not change the route — #2246's "keep the default on settings errors" is preserved, because a read that failed is not evidence the repo opted out.

Comments. Restores the maxConsecutivePollErrors rationale (the measured minimize_latency + follower-read window, the 15x2s arithmetic, and the CRDB-leaseholder fix that would retire it — now that a second phase shares the constant) and the createAndAwaitMirror doc comment stating the error contract both callers still branch on. Documents the four functions #2207 added, and records why TestSessionStore_ProbingManyDirectoriesRetainsNoDescriptors is deliberately not parallel — it counts this process's open descriptors, so restoring t.Parallel() for convention's sake makes it flaky.

Deliberately not fixed

The unknown-status branch stays unreachable. MirrorRequestStatus is a generated closed enum, so a new server status fails in UnmarshalText and surfaces as a decode error swallowed by the retry budget, not as the intended message. Honouring the operation's own "unknown values are generic terminal failures" contract needs the enum generated open, which is a spec change. The branch is kept and the comment says why it cannot fire.

The follower's bare-401 gate is unchanged. With the WithServerURL override gone there is no caller that reaches it, so the bug is fixed without touching it — and it now lives in auth-go rather than this repo. Widening recovery to any origin we hold a cached token for would prevent the class recurring, but it changes when a login JWT is re-exchanged on a bare 401: an auth-policy change for a shared library, not a ride-along here.

The setting stays in the project layer. The env var is the escape hatch; moving async_mirror_requests out of version-controlled settings entirely is a product call.

Verification

mise run lint — 0 issues. mise run test:ci — passed on the rebased tree (unit, integration, vogon canary, roger-roger canary).

New regression tests, one per finding: TestCreateAndAwaitMirror_AsyncIgnoresLocation (seven Location shapes including http://evil.example and a path-prefixed core, asserting no request is re-targeted at the named host), AsyncMissingRequestID, AsyncSuspendedPlacement (one-shot and wizard), AsyncTimeoutKeepsRequestID, SyncTimeoutCoversCreate, TestCreateOneMirror_AsyncPlacementTimeoutIsTimedOut, TestReportOneShotMirror_TerminalFailureIsNotCalledInFlight, and TestResolveAsyncMirrorRequests (now covering the default-on tri-state and the settings-error case). TestCreateAndAwaitMirror_AsyncCrossJurisdiction asserts every call — submission, placement poll, status read — arrives at the configured core and is redirected, which is the guard against the override coming back.

Two tests are deleted because they pinned behaviour this PR removes on purpose: TestCreateAndAwaitMirror_AsyncLocationValidation (Location parsing) and TestRoundTripper_421CanonicalizesMirrorRequestLocation (the canonicalizer).

Not exercised against a live control plane. Worth a real repo mirror create on both routes before this lands, particularly now that async is the default.

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

It changes mirror-create auth routing and polling semantics in a user-critical area (cross-jurisdiction + timeouts), which warrants final human review despite strong regression test coverage.

Pull request overview

This PR addresses post-merge review findings from #2207 by hardening and clarifying the opt-in async repo mirror create flow, with a focus on safe polling (requestId-based), consistent timeout semantics, suspended-placement parity, and cleaner CLI UX.

Changes:

• Remove all reliance on the Location header for async mirror-request polling; drive placement polling off the 202 body’s requestId and keep polls on the client’s configured core URL (421 redirect handling remains in the transport).
• Make --wait-timeout apply consistently across both async and sync routes (submission/registration + placement + clone wait), and preserve/print an accepted async requestId when the wait fails.
• Improve CLI output and tests: single relabelled spinner for one-shot create, wizard labels placement timeouts as “timed out”, and add regression tests covering the reviewed findings.

File summaries

File	Description
internal/coreapi/cross_juris_transport.go	Removes mirror-request Location canonicalization from the cross-jurisdiction transport path.
internal/coreapi/cross_juris_transport_test.go	Deletes the test that asserted the removed Location canonicalization behavior.
cmd/entire/cli/repo_mirror.go	Adds env-first async-route selection, unifies timeout semantics, introduces requestID retention for async failures, and switches one-shot create to a single updatable spinner.
cmd/entire/cli/repo_mirror_probe.go	Reworks async placement polling to use the 202 body’s requestId (not Location), removes Location parsing/poll targeting, and expands rationale/comments.
cmd/entire/cli/repo_mirror_request_test.go	Adds/updates regression tests for async route findings (ignore Location host/path, missing requestId, suspension parity, timeout behavior, env precedence, cross-jurisdiction routing).
cmd/entire/cli/repo_mirror_create_wizard.go	Classifies placement-timeout-without-placement as “timed out” (not generic error) for consistent wizard status reporting.
cmd/entire/cli/agent/session_store_test.go	Documents why one FD-counting test is intentionally non-parallel to avoid flakiness.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 1327e34. Configure here.}