Repository navigation
fix(gates): a gate was truncating the shared clone's history on every sweep - #365
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Agent-written comment (posted through Antony's @claude review this |
|
Claude finished @astubbs's task in 8m 16s —— View job Review complete
Note on this sandbox: I also could not execute
|
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
|
|
Agent-written comment (posted through Antony's Both review observations addressed in The regression, and it was mine. Observation 1 - Observation 2 - "already shallow → shallowing is a no-op" is not universal. Correct, and a fair catch: Also acted on the review's note about the sandbox reviewing the default branch's copy of the hook: nothing to change in the PR, but it is worth recording that the hook fired for real mid-review and denied a range query in a shallow sandbox clone - a live demonstration of the class, from the reviewer's own transcript. One thing the review flagged that is not being changed: the solutions doc's frontmatter follows its sibling Still needed: a human LGTM. |
|
@codex review this |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37e3c64b01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
365 is the fix for something this branch hit twice today: a gate was truncating the shared clone's history on every sweep, so `git merge-base` answered exit 1 where it had answered a commit an hour earlier, and half the gates would have judged a truncated graft while reporting green. Taking it here stops that recurring mid-merge. 366 brings the hosted-chaos trial and, with it, #351 - which diagnosed `OffsetEncodingBackPressureTest` (the 4/45 flake, the most frequent tracked one) as a test that froze the offset it then asserted. FOUR CONFLICTS, and the two that needed a decision: - docs/quarantined-tests.md - 366 removes the back-pressure entry because #351 released it. Taken. But the PCMetricsTest entry that stays cited it as "the entry above" for the shared shape of their failures, so that cross-reference is repaired rather than left pointing at nothing: the rhyme is now stated in the past tense and names #351 as the first place to look for whether the two are one phenomenon or two. Registry re-verified, 2 entries. - docs/inflight/test-untracked-ci-flakes.md - 366 retires the whole "NOT diagnosed - quarantined anyway" section. Taken whole rather than re-adding a section its author deliberately removed, after confirming the durable part - why quarantine rule 1 changed - survives in the solutions write-up that replaced it. - docs/ci.md and maven.yml - both sides edited the same paragraph and the same matrix. Kept both edits: the proto-breaking gate this branch gained from master stays listed beside 366's hosted-chaos job, and `continue-on-error` now honours both `advisory` (the execution-mode axis) and `optional` (366's experiment). Two names for one meaning, deliberately not unified - renaming either would edit the other branch's vocabulary and re-conflict when it lands. Also fixes two SpotBugs findings the merge surfaced in SessionEndTest: an ignored `CountDownLatch.await(timeout)` return in each of two user functions. A discarded await cannot be told from a call whose result nobody needs, and false there means the release never came inside the budget. Named per the repo convention rather than thrown from, since throwing on that thread would fail the record for a reason the test is not measuring. Verified on the merged tree: full reactor test-compile green, bin/check-all.sh 18/18 with only check-shell-lint CANNOT (shellcheck is not installed on this box). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019HwDcZdfv1Ne4u3g6ThrG9
|
@claude review this |
|
Claude finished @astubbs's task in 5m 9s —— View job Review complete
Sandbox note, and a live repeat of the exact defect class this PR fixes: this session's working copy of The four Codex bypasses (
|
…weep `bin/check-quarantine-owners.sh` reached the owning PR's base and merge preview with `git fetch --quiet --depth=1`. A depth-limited fetch writes the `shallow` file, and that file lives in the shared `--git-common-dir` - so one run truncated history for EVERY worktree of the clone at once, not just the caller's. `bin/check-all.sh` globs `bin/check-*.sh`, which made the mandated pre-push sweep an instruction to corrupt the clone. WHAT IT COST. Nothing goes red; the damage lands on other commands. Observed three times in one session: `git merge-base` returned empty, ahead/behind counts read 295 and 835 against true values in the tens, and commits that had demonstrably landed reported "NOT an ancestor of master". Read naively that says master was rewritten and a day's work is gone, and two sessions nearly acted on it. One repair failed with "shallow file has changed since we read it" because a sibling worktree was fetching concurrently. THE FIX. Ref previews are fetched into a throwaway git dir and read back through `FETCH_HEAD` there, so the working clone is never a fetch target and its depth is neither read nor written. Measured cost: ~1.4s and ~2.3MB for the first fetch, the dir reused for the rest of the run; a live gate run takes 3.5s and produces the same verdicts. REJECTED: choosing the depth from `git rev-parse --is-shallow-repository` and only passing `--depth=1` when the clone is already shallow. It is the obvious repair and it still SAMPLES shared state a sibling worktree can change between the sample and the fetch. Fetching elsewhere needs no sample, so there is nothing to race on; it also cannot deepen a clone that arrived shallow on purpose, and an interrupted run leaves the clone exactly as it was. The scratch dir removes a second shared-state fault in passing - the old code clobbered FETCH_HEAD for every other worktree - and replaces gh_query's per-call `mktemp`, which leaked a file on every interrupted run. `GIT_DIR=<dir> git <subcommand>` rather than `git --git-dir=<dir> <subcommand>`, though they mean the same thing: the global-option spelling pushes the subcommand out of `$1`, and `CheckQuarantineOwnersScriptTest` stubs `git` on PATH with a `case "$1" in fetch|show|cat-file)` dispatcher, so three of its twelve assertions failed while the script behaved perfectly. A wrapper looking at `$1` is a real pattern, so the script states the reason rather than leaving the spelling to look arbitrary. TWO BASH FINDINGS came out of proving the interrupt case, both fixed here. `exit` from inside a signal handler is documented to run the EXIT trap and measurably does not always - instrumented, the TERM handler ran in the main shell and the EXIT trap did not follow, on roughly one run in five - so the handlers call the cleanup themselves. And a second signal arriving during teardown re-enters the handler and abandons a half-finished `rm -rf`, so cleanup disarms INT/TERM as its first act. Both were found because the arm was flaky at three passes in five; the fix is in the gate, not a poll in the test. THE INVARIANT IS ASSERTED, NOT DESCRIBED. `bin/test-check-quarantine-owners.sh` is new and hermetic - a fake `gh` on PATH and a local file:// origin carrying real refs/pull/N/merge refs - covering: a full clone stays full, a deliberately shallow clone is neither deepened nor re-shallowed, an interrupted run leaves both the clone and the temp dir clean, and the gate still reaches its OK and its "does NOT yet remove the quarantine" verdicts, so the side effect cannot be removed by removing the check. 13 arms; 6 were verified red against the previous code, including all three depth arms, while both behaviour arms stayed green - which is what shows this preserves what the gate does.
Fixing the one script removes today's instance; the class survives it. Three
entrances, none of which can see the others, so each gets its own gate.
A SCRIPT doing it is caught by a `shared-git-state` row in
`bin/check-shell-hazards.sh` - the second category in a table built for
GNU-vs-BSD divergence, because both have the same signature: no error, a wrong
answer somewhere else, unreachable by a linter. `git clone --depth` is
deliberately not the hazard, since a clone owns its own depth; `git pull --depth`
is, and was missed by the first pattern.
A HAND-TYPED `--depth=1` is denied by `.claude/hooks/check-shallow-history.sh`,
which already denied the QUERIES that answer wrongly from a truncated graft and
now also denies the fetch that does the truncating, naming the throwaway-git-dir
alternative. The two directions are gated in opposite senses on purpose: a query
is wrong only when the clone is ALREADY shallow, a shallowing fetch only worth
stopping while it is NOT.
FOUR BYPASSES, all in code this PR added, all found by review and all closed
here. Each is a way the guard could be walked past while still reporting a
verdict:
- The hook reported one verdict and exited, so `git merge-base ...;
git fetch --depth=1 ...` read as a QUERY and the shallowing fetch was
allowed. It now emits one record per git call and walks them in command
order against a single state read, so a shallowing fetch arms the queries
after it.
- `--git-dir=.git` was trusted from its spelling, which made the escape hatch
itself a bypass. The target is now resolved with `rev-parse
--git-common-dir` and compared against this clone's, so a sibling worktree's
`.git` correctly denies - the actual incident this PR exists for.
- `command git` and `timeout 60 git` skipped the hook entirely, including the
pre-existing query arm. It now walks assignment prefixes and eleven
wrappers, plus a subshell's leading `(`.
- The hazard gate matched physical lines, so `git fetch \` followed by
`--depth=1` read clean - a hole in EVERY row, not just the new one.
Continuations are now joined into logical commands before any regex runs.
TWO MORE OF THE SAME CLASS surfaced while validating those, both fixed. The
hazard row had stopped seeing this repository's own fetch: wrapping the call as
`preview_git fetch ...` put the `_` before `git` outside the row's prefix class,
so coverage of `check-quarantine-owners.sh` silently went to zero and its
`hazard-ok:` marker excused nothing. Proven both ways - marker off, it fires;
marker on, clean. And the new joiner matched rows against a record still
carrying its own `NNN:` line-number prefix; harmless today, but a future row
anchored `^` would have matched nothing, silently, forever.
TWO DELIBERATE IMPRECISIONS are recorded in the hook's header rather than left
implicit, both erring towards denial. A redirect target only the shell can
expand is judged on its text and errs toward ALLOWING, because resolving it
would mean running it and failing closed would deny `--git-dir="$(mktemp -d)"` -
the very idiom the deny message recommends. And `-C <dir>` is not read as a
redirect, since `-C .` names this repository and treating it as one would be a
bypass.
Arms: 38 to 67 in `check-shallow-history`, 34 to 44 in `check-shell-hazards`.
Every new one was verified red against the previous code first - 5 hook arms
with no false positives among the 33 existing, and 2 hazard arms against the
pattern's looser first version.
The transferable half, which a fix commit and an inflight note are both the wrong home for: a gate that mutates shared repository state as a side effect of checking, where nothing goes red and the damage lands on unrelated commands later. Carries the defect-class sweep with its reasons, so "none found" says where it looked: `pr-checklist.yml`'s own depth-1 fetch on a CI runner that owns its workspace, the workflows' `fetch-depth` checkouts, `ci-mutation-test.sh`'s undepthed fetch, the `git clone --depth=1` fixtures in two self-tests, and every other git mutation in `bin/`, each of which targets a scratch repository rather than the working clone.
1d7759b to
2ce14db
Compare
…is fixed on master #365 landed the fix (`3a98b8d6d`), so this note now describes a defect that no longer exists. `docs/inflight/AGENTS.md` is explicit that the directory tracks only what is currently OPEN and that a closed item is deleted rather than rewritten into a FIXED narrative - and leaving it would have been worse than untidy, because a note asserting the repo's own gate sweep corrupts the clone is exactly the kind a future session acts on. Nothing is lost. The note's own "what a fix has to preserve" section proposed fetching into a scratch git dir, which is what #365 implemented, and the transferable half - the class, the sweep's reasons, and the "none found" list with where it looked - is now docs/solutions/workflow-issues/a-gate-truncated-the-shared-clone-2026-08-26.md, which arrived on this branch with the master merge below. Verified nothing in the tree cited the deleted path.
Nine commits, no conflicts. Read for inherited obligations, and this records what that returned, including the nothings. The `branch_accounting` register added by #365 does not want an entry for this branch: its 44 records are mirrored upstream refs, deliberate deletions, and two of ours, and its own header says a tip is recorded for branches that are gone rather than demanded for ones that are not. The new `bin/check-upstream-map.sh` validates it and passes. Two obligations land at merge prep rather than here: the checklist now asks whether this PR should close an issue, and `docs/inflight/issue-index.md` is what makes that check cheap. Neither is actionable while the PR is open. Verified after the merge: core compiles, and the branch's own tests pass - OffsetEncoderWidenedRangeRaceTest (3) and RemovedPartitionStateTest (2). `bin/check-all.sh` reports 14 passed, 0 failed; check-shell-lint could not run because shellcheck is absent from this machine, which is not a pass - this branch changes no shell files, so nothing of its own is unmeasured.
1 participant
It serves no single issue, so it cites none.
Description
bin/check-quarantine-owners.shreached the owning PR's base and merge preview withgit fetch --quiet --depth=1. A depth-limited fetch writes theshallowfile, and that filelives in the shared
--git-common-dir- so one run truncated history for every worktree of theclone at once, not just the caller's.
bin/check-all.shglobsbin/check-*.sh, so the mandatedpre-push sweep was also an instruction to corrupt the clone.
Nothing goes red. The damage lands on other commands, and it fired three times in one session:
git merge-basereturned empty, ahead/behind counts read 295 and 835 against true values in thetens, and commits that had demonstrably landed reported "NOT an ancestor of master". Read naively
that says master was rewritten and a day's work is gone - two sessions nearly acted on it. One
repair even failed with "shallow file has changed since we read it", because a sibling worktree
was fetching concurrently.
The fix
Ref previews are fetched into a throwaway git dir and read back through
FETCH_HEADthere:Rejected: a conditional depth - passing
--depth=1only whengit rev-parse --is-shallow-repositorysays the clone is already shallow. It is the obvious repairand it still samples shared state a sibling worktree can change between the sample and the fetch.
Fetching elsewhere needs no sample.
How each requirement is met:
so its
shallowfile is never written. Asserted bybin/test-check-quarantine-owners.shfrom both directions, and confirmed live: a real run againstGitHub takes 3.5s, produces the same three verdicts, and leaves
is-shallow=falsewith noshallowfile in.git/.the
OK: ... its merge result removes the quarantineverdict and thedoes NOT yet remove the quarantineverdict, againstrefs/pull/7/mergeandrefs/pull/8/mergefixtures - so the side effect cannot be removed by removing the check. Both of those arms are
green against the unfixed script, which is what shows this preserves behaviour.
written, so a CI checkout at depth 1 is untouched. Arm: a deliberately shallow clone stays
shallow plus is not deepened (
rev-list --count HEADstays 1).shallowfile, and not
FETCH_HEAD, which the old code also clobbered for every other worktree. Aninterrupted run leaves the clone exactly as it was, because it was never the fetch target. Arm:
the gate is killed mid-run and the clone is asserted unshallowed, with its scratch dir gone.
Two bash findings came out of proving requirement 4, both fixed here:
exitfrom inside a signal handler is documented to run theEXITtrap and measurably does notalways. Instrumented, the
TERMhandler ran in the main shell ($$==$BASHPID) and theEXITtrap did not follow, on roughly one run in five. The handlers now call the cleanupthemselves.
rm -rf.Cleanup disarms
INT/TERMas its first act.The scratch dir also replaces
gh_query's per-callmktemp, which leaked a file on everyinterrupted run.
Gating the class, not the instance
Three entrances, none of which can see the others:
bin/or.claude/hooks/fetches with--depthbin/check-shell-hazards.sh, newshared-git-staterow - fails the build.claude/hooks/check-shallow-history.sh- denies it, and names the throwaway-git-dir alternative.claude/hooks/check-shallow-history.sh- already existed; deniesmerge-base,rev-list, rangesgit clone --depthis deliberately not the hazard (a clone owns its own depth);git pull --depthis, and is matched. The hook's two directions are gated in opposite senses: a query iswrong only when the clone is already shallow, a shallowing fetch only worth stopping while it is
not - denying it in an intentionally shallow CI clone would be noise, and noise is what gets a
hook switched off.
Red controls
Every new assertion was run against the previous code first.
bin/test-check-quarantine-owners.sh(new, 13 arms)bin/test-check-shallow-history.sh(67 arms after both rounds)bin/test-check-shell-hazards.sh(44 arms after both rounds)Each original was restored byte-identically afterwards and verified with
cmpplusgit hash-object.The
shared-git-statepattern's first version readcore.pager=catas the subcommand and missedgit -c k=v fetch --depth=1entirely - the same walk-past the hook's own header documents forgit -C DIR rev-list. It was found by an arm, and both forms are covered now.Defect-class sweep - checked and dismissed
.github/workflows/pr-checklist.ymlrunsgit fetch --no-tags --depth=1 origin <base>from aJS step: a CI runner's own workspace, deliberately shallow, with no worktrees sharing it. Not an
instance, and outside the hazard gate's corpus (
bin/+.claude/hooks/).actions/checkoutwithfetch-depth: 1inclaude.yml,claude-code-review.yml: a freshper-job clone, not a fetch into a shared one.
bin/ci-mutation-test.shfetches its base ref with no--depth- a plain fetch isadditive and writes no
shallowfile.git clone --depth=1inbin/test-check-shallow-history.shand in this PR's own self-test:building a shallow fixture in a scratch directory that owns its depth.
bin/-update-ref,reset,checkoutintest-ci-mutation-test.sh,test-check-file-refs.sh,test-check-branch-self-reference.sh,test-check-agent-hooks.sh- targets amktemp -dfixture repo, verified individually, not theworking clone.
bin/rename-packages.sh'sgit fetch origin/git checkout origin/master -- ...are insideits
--helptext, printed as instructions, never executed.Result: one instance, now fixed; nothing else in the class.
A regression this PR caused and fixed
CheckQuarantineOwnersScriptTest- a Java behavioural test of this shell script, which the prior-artsearch missed - stubs
giton PATH with acase "$1" in fetch|show|cat-file)dispatcher. The firstversion of the fix wrote
git --git-dir=X show ..., putting a global option in$1, so the stubmatched nothing, printed nothing, and 3 of its 12 assertions failed while the script behaved
perfectly.
Fixed in the code, not the test:
GIT_DIR=<dir> git <subcommand>means exactly the same thingand leaves the subcommand where every wrapper looks for it. All 12 pass locally. That is the third
instance of this walk-past in a week -
git -C DIR rev-listpast the hook,git -c k=v fetchpastthe new hazard row, and now a global option past a test stub - so the script states the reason rather
than leaving the next editor to rediscover it. The hook learned the same idiom (
GIT_DIR=prefixcounts as a redirect alongside
--git-dir), or it would deny the very alternative its own denymessage recommends.
Two deliberate imprecisions in the hook, raised in review
Both now recorded in the hook's header, and both err towards denial:
-C <dir>is not read as a redirect, sogit -C /other/repo fetch --depth=1is denied thoughharmless. Treating it as one would be a bypass, since
-C .names this repository.(
--depth=5over a depth-1 clone) still rewrites the shared graft. Bounded rather than missed: inan already-shallow clone the query arm is armed, so no truncated answer reaches anyone silently -
and denying depth changes in an intentionally shallow CI clone is the noise that gets hooks
switched off.
Codex review round (
1d7759b9b)Codex found four real bypasses, all in code this PR added. All four are fixed, with every new
assertion verified red against the pre-change file first.
git merge-base …; git fetch --depth=1 …classified as a QUERY and the fetch was then allowed in a full clone--git-dir=.git/GIT_DIR=.gitwere exempted from their spelling, making the escape hatch a bypassrev-parse --git-common-dirand compared with this clone's — which correctly denies a sibling worktree's.gitcommand git/env git/timeout 60 gitleftgitout of command position, skipping the hook entirely — including the pre-existing query arm(bin/check-shell-hazards.shmatched physical lines, sogit fetch \+--depth=1 …read clean — a hole in every rowTwo more of the same class surfaced while validating, both fixed here:
preview_git fetch --depth=1 …, and the_beforegitfalls outside the row's prefix class —so coverage of
bin/check-quarantine-owners.shsilently dropped to zero and itshazard-ok:marker was excusing nothing. The row now matches a command word ending in
git. Proven both ways:marker removed → the gate fires on that line; marker present → the corpus is clean.
NNN:prefix.Harmless today, but rows are data contributors add, and one anchored with a bare
^would havematched nothing, silently, forever. The regex now sees command text only.
Arms: 38 → 67 (
check-shallow-history), 34 → 44 (check-shell-hazards).One deliberate imprecision, stated in the hook's header rather than left implicit: a redirect
target only the shell can expand is judged on its text and errs towards allowing, because
resolving it would mean running it, and failing closed would deny
--git-dir="$(mktemp -d)"— theidiom the deny message itself recommends.
Verification
bin/check-all.sh --with-tests: 39 ran, 37 passed, 0 failed. The twoCANNOT RUNare theshellcheck-dependent lanes - shellcheck is Ansible-managed and not installed - so the lane's exact
invocation was run through the cached
koalaman/shellcheck:stableimage instead: clean at--severity=errorover the whole corpus, and clean at--severity=warningover every file thisPR touches.
bin/check-all.shbefore and after:is-shallow=falseboth times. That is the whole point - thesweep no longer truncates the clone it is checking.
Note for #338
docs/inflight/ci-check-quarantine-owners-reshallows-the-clone.mdrecords this bug and lives onthat PR's branch, not on master, so it cannot be deleted from here. It should be dropped from
#338 before that merges, or master gains a note describing a fixed bug.
Its durable replacement is
docs/solutions/workflow-issues/a-gate-truncated-the-shared-clone-2026-08-26.md, added here.docs/inflight/ci-networked-checker-in-reviewer-grant.mdquoted the two removed lines; itsquotation is refreshed and its argument - the script still reaches the network inside the reviewer's
grant - is unchanged, so the note correctly stays open.
Checklist
docs/solutions/workflow-issues/a-gate-truncated-the-shared-clone-2026-08-26.mdadded;
docs/inflight/ci-networked-checker-in-reviewer-grant.mdcitation refresheddocs/features/-N/A - repo tooling, no user-facing behaviour changesbin/test-check-quarantine-owners.sh(new, 13 arms), plus 13 arms onbin/test-check-shallow-history.shand 5 onbin/test-check-shell-hazards.sh; every newassertion shown red against the old code first