feat: Refresh reused checkouts from remote mirrors

[pda]

Description

Fetch the immutable build commit from the remote mirror into a reused checkout before falling back to canonical. This is PR 3 based on #4161.

A mirror miss never deletes the checkout. Unsafe promisor-cleanup failure quarantines the checkout's Git state, recreates an empty workdir for teardown hooks, and lets the next mirror-ineligible attempt clone canonically.

Context

The explicit mirror URL makes the bounded source deterministic despite customer insteadOf rules. That requires filter inheritance and one-shot promisor cleanup without mutating user-owned promisor configuration.

Changes

• Attempt only immutable commit fetches and skip canonical fetch only after a local hit.
• Preserve effective fetch flags, including Git-accepted --fi* and --no-fi* abbreviations.
• Inherit origin filters only without an explicit override.
• Establish canonical promisor ownership before removing mirror ownership on every outcome.
• Write a credential-free digest marker before mirror fetches and repair only the matching agent-created promisor after interruption, URL removal, or rotation.
• Leave unmarked and unrelated user promisor configuration untouched; clear marker-only crash windows safely.
• Preserve pre-existing unmarked config for the exact mirror URL by skipping the mirror and using canonical.
• Fall back to canonical when marker setup or ownership inspection fails.
• Route cleanup failure through clean canonical retry, prioritising quarantine over cancellation handling.
• Keep gitfile-backed reused checkouts canonical because external Git directories cannot be safely quarantined with the worktree.
• Recreate an empty checkout directory after quarantine so teardown hooks retain a valid working directory.
• Run mirror-keyed config commands silently with checkout PATH resolution, preventing URL-bearing argv/stderr leaks.
• Preserve cancellation without starting canonical fallback.

Filtered Git fetches can transiently write URL-named promisor keys. Completed fetches remove the keys and marker; the next fetch repairs only matching marked state left by process interruption.

F2 mirror-routed lazy materialisation remains outside this stack; lazy reads use canonical origin.

Testing

Real Git-over-HTTP tests cover hit, lag, failure, timeout, cancellation, automatic/inherited/abbreviated filters, marked repair after URL removal, preservation of unmarked user promisors including an exact-URL section with extra keys, marker-only crash recovery, marker privacy and setup failure, canonical materialisation, credential-safe config failures, checkout PATH, state preservation, and debug suppression. Regression tests additionally force cancellation plus config-lock cleanup failure, verify unsafe Git state is removed while the teardown workdir remains, and keep real --separate-git-dir checkouts mirror-ineligible.

Disclosures / Credits

Implemented with Cursor under human direction. Independent review drove all-outcome and restart recovery, scoped ownership marking, exact-URL collision handling, option parity, safe retry, gitfile/PATH correctness, teardown invariants, and logging hardening.

  

[buildsworth-bk-app]

I didn't find a concrete issue to flag inline. This adds a credentialed mirror fetch and persistent promisor reconfiguration to reused checkouts, so it still needs a human sanity-check.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10780, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

About buildsworth

Model: gpt-5.6-sol with xhigh thinking.

How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.

Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):

• L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
• L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
• L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.

[cursor]

Propagated the implementation-derived plan correction in merge 5822f5e6. The updated plan now exactly matches this PR's cancellation-safe all-outcome promisor cleanup, inherited-filter, and --no-filter behavior; the PR 3 code diff is unchanged against its updated base.

[buildsworth-bk-app]

I didn't find a concrete issue to flag on the updated head. The change still adds a credentialed network path and rewrites persistent promisor state in reused checkouts, so it remains one for human sanity-checking.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10797, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

Merged the reviewed foundation and on-host fixes through 026f60ea. The reused-checkout layer now inherits debug-proof hidden prompts and the explicit Git 2.45+ local-probe contract.

[cursor]

Added the red-step regression in 8eaa9a0d proving that debug-mode promisor inspection and cleanup must not print mirror-URL-bearing config argv.

[cursor]

Fixed the remaining reused-checkout logging gap in d392cc2f: all promisor config reads, writes, and section removal now use the force-hidden prompt option, so mirror URLs cannot appear in debug argv. The direct mirror fetch now also carries the plan’s explicit caveat for user-supplied ref-mutating flags.

[cursor]

Propagated the recursive-submodule eligibility foundation through 49b96dc9; reused checkouts remain eligible because no clone-time submodule initialization occurs.

[buildsworth-bk-app]

I didn't find a concrete issue to flag on the current head. This changes the credentialed fetch path and persistent promisor ownership in reused checkouts, so it still needs a human sanity-check.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10829, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

Propagated the final plan alignment through 2dc033e7; the reused-checkout implementation diff remains scoped to its layer.

[buildsworth-bk-app]

I found two blocking edge paths in the new promisor and filter handling; details are inline.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10849, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

Added the re-review red step in aba002d1 for Git-accepted --fil=… and --no-fil fetch option abbreviations.

[buildsworth-bk-app]

On it! Follow along with my review here: https://buildkite.com/buildkite/buildsworth-review-pull-request/builds/10853

[cursor]

Implemented reused-checkout re-review fixes in 58b00ebd: Git-accepted --fil*/--no-fil* flags suppress inherited filters, and promisor-cleanup failures now use an explicit preserve-workdir error path so the outer checkout retry cannot delete a usable checkout.

[buildsworth-bk-app]

Two earlier concerns are still open, and I found one additional cleanup-failure issue inline.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10855, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[buildsworth-bk-app]

The three earlier concerns remain open on the current head; I didn't find a separate new issue this pass.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10864, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[buildsworth-bk-app]

On it! Follow along with my review here: https://buildkite.com/buildkite/buildsworth-review-pull-request/builds/10870

[cursor]

Final reused-checkout fix 9f96250f recognizes Git’s shortest --fi*/--no-fi* abbreviations and routes failed promisor cleanup through clean canonical retry because preserving an unsafe mirror promisor is not acceptable.

[buildsworth-bk-app]

One earlier concern is fixed. The credential-bearing config failure path and the cancellation-plus-cleanup-failure path remain open in their existing threads; I didn't find a separate new issue this pass.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10878, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[buildsworth-bk-app]

The credential-redaction path is fixed. The cancellation-plus-cleanup-failure case remains open in its existing thread, and I found one new PATH-resolution issue inline.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10893, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

@buildsworth-bk review

@buildsworth-bk approve L2

[buildsworth-bk-app]

The two earlier concerns remain open in their threads. I also found that the new config fast path rejects checkouts where .git is a gitfile; details inline.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10898, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[buildsworth-bk-app]

The Git path and gitfile concerns are fixed. The cancellation-plus-cleanup-failure case remains open in its existing thread; I didn't find a separate new issue this pass.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10900, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[buildsworth-bk-app]

The cancellation-plus-cleanup-failure concern is still open on the unchanged head; I didn't find a separate new issue this pass.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 10906, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

Propagated the resolved stack by merging the updated #4161 branch into this branch. The merge was clean and carries the latest #4160/main changes without altering #4162’s reused-checkout-specific diff. Pushed as 53c023b6; focused verification will follow.

[buildsworth-bk-app]

The cancellation-plus-cleanup-failure concern is still open in its existing thread; I didn't find a separate new issue on the current head.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 11125, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

Rebased and force-pushed the unchanged #4162 feature commit onto #4161’s Windows test-resource cleanup.

[buildsworth-bk-app]

The existing gitfile cleanup concern is still open in its thread; I also left one documentation correction inline.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 11291, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

Addressed the follow-up gitfile edge in 3cbb0c27: reused checkouts whose .git is a gitfile are now mirror-ineligible. Their external Git directories outlive worktree cleanup, so keeping them canonical avoids creating one-shot mirror promisor state that cannot be safely quarantined. Added real --separate-git-dir eligibility coverage; the existing interrupted-promisor repair coverage remains.

[buildsworth-bk-app]

The gitfile exclusion fixes the earlier unsafe-cleanup case. The documentation note remains open in its thread, and I found a separate teardown regression in the cleanup branch.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 11293, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

Addressed both latest findings and force-pushed them as one clean follow-up commit (68407df9): the cleanup branch now recreates an empty checkout directory after removing unsafe Git state so teardown hooks retain a valid working directory, and the regression asserts that directory/Git-state split. The docs now say direct filtered fetches may transiently write promisor keys and guarantee only that the mirror URL is not retained after a completed fetch.

[cursor]

Folded the Windows CI follow-up into the cleanup commit (d29842a3): the reused-checkout test helper now closes any checkoutRoot opened by checkout(), so TempDir cleanup is not blocked on Windows. This is test-only resource cleanup; the production teardown fix is unchanged.

Previous concerns have been addressed.

[buildsworth-bk-app]

The two open concerns are fixed, and I didn't find a new issue on this head. This remains a credentialed fetch path that rewrites persistent promisor state, so it still needs human review.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 11298, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

Previous concerns have been addressed.

[buildsworth-bk-app]

The previously open documentation and teardown concerns remain fixed, and I didn't find a new issue on this head. The reused-checkout feature files are unchanged from the last reviewed head; this still changes a credentialed fetch path and persistent promisor ownership, so it needs human review.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 11421, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[cursor]

Review: regression risk for jobs with no remote mirror configured

TL;DR: Overall risk to existing no-mirror behaviour is low. The new mirror fetch/cleanup machinery is unreachable without a mirror URL, but three pieces of changed code do run on every job regardless of mirror configuration. Each has a narrow, mostly benign impact, detailed below. CI is green (the full pipeline exercises the extensive pre-existing no-mirror checkout integration tests), and the fetch/filter/mirror tests in internal/job pass locally on this branch.

Why the main path is safe

resolveRemoteMirrorAttempt in internal/job/checkout_remote_mirror.go returns skip(remoteMirrorSkipNoURL) immediately when GitRemoteMirrorURL is empty, and the new fetch branch in fetchSource is gated on isExistingCheckoutRemoteMirrorAttempt, which requires a non-skipped attempt. So the mirror fetch, promisor config writes, and the detached 5-second cleanup never execute for no-mirror jobs. The new checkoutUsesGitFile stat also only runs after the no-URL gate.

Three changes that do affect the no-mirror path

1. repairInterruptedRemoteMirrorPromisors runs unconditionally at the top of every fetchSource (internal/job/checkout_fetch.go).

• For an ordinary checkout it is one os.Stat plus one read of .git/config, and it bails out unless the config contains both [remote "http and promisor — a no-op with no subprocess for virtually every real repo. For gitfile-backed checkouts (--separate-git-dir/worktrees) it instead runs one git config --local --get-regexp subprocess per checkout; exit code 1 is handled as "nothing to do". Cost is negligible either way.
• The one genuine behaviour change: if a remote.<http(s)-url>.promisor key exists that the agent did not create — Git itself persists these when a user's hook or script does a direct-URL filtered fetch inside the checkout — the agent will now silently set remote.origin.promisor=true, copy the filter onto remote.origin.partialclonefilter, and delete the whole remote.<url> section. For that (niche) user, lazy object fetches retarget to origin, which may not contain the objects if the URL was a different repo. This is the only place the PR actively mutates state on jobs that never opted into remote mirrors.
• New failure route: any error here (unreadable/corrupt .git/config, git config exiting with something other than 0/1) becomes gitErrorFetchRetryClean, which wipes the checkout and re-clones. On main such broken states would generally have failed the subsequent fetch and hit the same wipe, so this is close to neutral.

2. hasPartialFilterFlags broadened to match --fi* abbreviations (internal/job/git.go). This function is shared with the no-mirror sparse-checkout logic (userSuppliedCloneFilter and addBloblessFilter in internal/job/checkout.go). For users spelling out --filter, behaviour is identical. For users passing a Git-accepted abbreviation like --fil=blob:none, it is a fix (previously the agent could append a second filter). The only false positives are invalid flags like --filtered in a non-final position — Git rejects those flags anyway, so no working configuration changes outcome. Note the existing "filter prefix lookalike" unit test in internal/job/git_test.go only still passes because the lookalike is the last token; the protection is positional now, which is a test-coverage nit rather than a regression.

3. gitErrorFetchRetryClean handling reordered in the checkout retry loop (internal/job/checkout.go). This error type already existed on main with a no-mirror producer: gitFetch returns it on exit code 128 (Git fatal errors). For non-cancelled jobs the new dedicated case is behaviourally identical to the old shared case (warn, remove checkout dir, recreate, retry). The delta is that the new case now sits before the context.Canceled checks: if a fetch fails with exit 128 in the same instant the job is cancelled, main printed "Checkout was cancelled" and preserved the checkout, while this PR wipes and recreates the checkout dir, so teardown hooks see an empty workdir and the next build re-clones. This reordering is deliberate (mirror cleanup failures must quarantine even under cancellation) but it also applies to the pre-existing exit-128 producer. The race is narrow — signal-killed git reports exit −1, not 128, and is caught by an earlier case — so exposure is a legitimate fatal fetch failure racing with cancellation. Consequence is a slower next build, not incorrect state. Also, the WasRetried branch in the new case is dead code, since no gitErrorFetchRetryClean producer sets WasRetried.

Verification

• All PR checks pass, including the full Buildkite pipeline (build #13706).
• Locally on this branch: go test ./internal/job/ -run 'RemoteMirror|PartialFilter|FetchSource|Fetch' -count=1 → ok ... 31.058s. The new test file includes a gitfile-checkout repair no-op test and separate-git-dir mirror-ineligibility regression tests.

If any of the three findings are worth hardening before merge, the cheapest wins would be gating repairInterruptedRemoteMirrorPromisors on the mirror feature ever having been eligible for the checkout (or at least logging when it rewrites config it didn't create), and adding a non-final-position lookalike case to TestHasPartialFilterFlags.

[cursor]

Implemented the promisor-repair ownership gate in 3783e043:

• writes a credential-free SHA-256 marker before a remote-mirror fetch
• repairs only the URL-named promisor whose digest matches that marker
• clears marker-only crash windows safely
• leaves unmarked and unrelated user promisor configuration untouched
• falls back to canonical if the marker cannot be written

Focused reused-checkout mirror tests pass locally, including the new ownership, crash-window, privacy, and fail-open cases.

@buildsworth-bk review

Previous concerns have been addressed.

[buildsworth-bk-app]

The earlier cleanup concerns remain fixed. I found one narrow ownership edge inline. This still changes a credentialed fetch path and persistent promisor state, so it remains one for human review; the current agent CI run is also failing.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 11449, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

Previous concerns have been addressed.

[buildsworth-bk-app]

The same-URL ownership edge is fixed: the mirror path now checks for existing local remote config before marking, and the regression preserves the full user-owned section. I didn't find another concrete issue. This still changes a credentialed fetch path and persistent promisor ownership, so it needs human review; agent CI is still running.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 11453, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

[catkins]

Is this a plan reference?

[pda]

yep… 

agent/docs/remote-git-mirrors.md

Lines 1445 to 1452 in 01c20f6

	**C21 — User-supplied ref-mutating fetch flags apply to PR 3's mirror fetch.**
	The existing-checkout path forwards the effective canonical fetch flags to
	preserve operator intent. Flags such as `--tags` or `--prune-tags` can therefore
	import or remove tags according to the mirror's lagging view even when the
	immutable commit fetch misses. Origin tracking branch refs are not targeted, and
	R8 still chooses the build by full object ID. Avoiding this would require
	silently changing user fetch semantics for one source. *Comment: at the direct
	mirror fetch in `checkout_fetch.go`.*

maybe shouldn't be cited from a code comment, although that plan does exist on main for now.