fix(workspace): make a managed base checkout refuse inbound pushes (BLO-31555) - #1640
Conversation
…BLO-31555) BLO-31359 stopped the ephemeral run clone from carrying the project base checkout as its configured `origin` (#1616). That closed the path runs actually took, but not the capability: the run pod and the base share one filesystem, so pushing to the base by explicit path still landed, and so did re-adding the remote by hand. Only the receiving side can refuse. Installs a `pre-receive` hook on the base at `ensureManagedProjectWorkspace`'s single exit point, alongside `ensureCheckoutGitIdentity` (BLO-23894) and `ensureManagedCheckoutCanServeClones` (BLO-31351). The exit point is the whole point: every base checkout a run could push into already exists, so a guard installed after the `git clone` would protect only the one checkout nobody has had a chance to push to yet. No config-only mechanism covers this. `receive.denyDeletes`, `denyNonFastForwards` and `denyCurrentBranch` all refuse destructive updates but none refuses creating a *new* ref, which is the observed shape (12+ `blo-*` branches in one base). They are configured anyway, so existing refs stay protected even if the hook file is removed -- no anchored ref may be lost. Hook placement resolves `core.hooksPath` first, because `.git/hooks/pre-receive` is silently ignored when it is populated and this repo's own fixtures show it is in real environments. When the effective hooks dir belongs to the repo the hook goes there and no config is written; only when it is outside the repo does the guard take `core.hooksPath` over, recording what it displaced. A `pre-receive` dropped into a global hooks dir would apply to every repo on the host. `pre-receive` runs only under `receive-pack`, so `git clone --shared` from the base, fetches into it, `git config` writes and `git worktree add` are all unaffected. Failure is non-fatal throughout: this is hardening on a provisioning path, and taking a run down because a hook could not be written would trade a latent hazard for a certain outage.
1 similar comment
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0588546
Critical Issues (0)
None. The guard is non-fatal by construction, creates and deletes no refs, and pre-receive runs only under receive-pack — I re-verified the blast-radius claim against execution-workspaces.ts, git-worktree-ownership.ts and workspace-realization.ts and found no code path that pushes into a managed checkout or worktree.
Important Issues (3)
-
[pr-review-toolkit: error-handling / gstack: structural]
server/src/services/managed-checkout-push-guard.ts:300— a foreignpre-receivein an in-repo hooks dir is overwritten with no backup, no record and no warning. The verbatim comparisonexisting === PUSH_GUARD_HOOKcannot distinguish a stale or hand-defanged Paperclip hook (which test 8 requires be rewritten) from an operator's own hook, so both are clobbered. This contradicts the module's own promise at line 46 — "Every pre-existing hook keeps working and the operator's intent is preserved" — which holds for every hook name except the one this module writes. Note the machinery to tell them apart already exists and is never read back:PUSH_GUARD_HOOK_MARKER(line 95) is embedded in the hook body but is not exported and not used for detection.- Rewrite when the file is absent or contains
PUSH_GUARD_HOOK_MARKER; when a non-markerpre-receiveis present, preserve it (rename topre-receive.paperclip-displaced, or chain-exec it) and warn — mirroring the care the out-of-repo branch already takes withdisplacedHooksPath. Defaultgit init/clonetemplates shippre-receive.sample, notpre-receive, so this is operator-installed-only and low-likelihood — but it is silent and irreversible, which is why it is worth the four lines.
- Rewrite when the file is absent or contains
-
[gstack: conditional side effects]
server/src/services/managed-checkout-push-guard.ts:316— thecore.hooksPathtakeover is written with--local, which git stores in$GIT_COMMON_DIR/configand therefore shares with every linked worktree of the base. This project provisions execution workspaces as worktrees (providerType === "git_worktree",execution-workspaces.ts:529; the PR's own test 6 exercisesgit worktree addoff the base), so in the displaced case hook resolution is silently repointed for every run workspace derived from that base — not just the base. The warning at line 339 says "hooks that lived in it no longer run for this checkout", which understates the scope in exactly the environment this branch exists to serve. The module docstring already notes that linked worktrees share one hooks dir, so the mechanism is understood; only the operator-facing text and the verification claim lag behind it.- Widen the warning to name derived worktrees, and confirm no run-workspace flow depends on an inherited
post-checkout/pre-commit/pre-pushfrom the displaced directory. Per-worktree scoping would needextensions.worktreeConfig+--worktree, which carries its own consequences — worth being a deliberate decision rather than an unstated default. (The corollary that run worktrees also inherit thepre-receiveis benign given the grep above, and arguably desirable.)
- Widen the warning to name derived worktrees, and confirm no run-workspace flow depends on an inherited
-
[pr-review-toolkit: code]
server/src/services/managed-checkout-push-guard.ts:324— the receive-config idempotency read usesgit config --get, which resolves across all scopes, so when an inherited global or system config already carries the desired value the--localkey is never written. That defeats the stated purpose ofPUSH_GUARD_RECEIVE_CONFIG(lines 137–146: "keeps protecting existing refs even with the hook file removed", and AC6 durability), because the protection then lives in config the checkout does not own and that a different pod or image may not carry. The distinction is real and per-key:core.hooksPathat line 306 genuinely wants the effective value; these three want the local one.- Read with
["config", "--local", "--get", key]forPUSH_GUARD_RECEIVE_CONFIGso the keys are always materialized in the checkout's own config. One flag, and it restores the property the constant's docstring claims.
- Read with
Suggestions (4)
- [pr-review-toolkit: error-handling]
server/src/services/managed-checkout-push-guard.ts:361— the catch resetshookPathtonulland asserts the checkout "may still acceptgit push <path>", but the hook file is written before both config writes. If the hook landed and only a latergit configthrew, the guard is in force (or, in the displaced case, in force with no recorded displacement) and the warning tells an operator the opposite. Consider reporting what was actually achieved. - [native-codex: efficiency]
server/src/services/managed-checkout-push-guard.ts:323— the steady state still spends ~5gitsubprocesses per provisioning call (1rev-parse, 1core.hooksPathread, 3 receive-key reads), per project, per heartbeat, even whenhookCurrentis true and nothing needs writing. Recording a local guard-version key and short-circuiting the receive loop would keep the hot path at two. - [native-codex: observability]
server/src/services/managed-checkout-push-guard.ts:339— after a displacement, the second provision reads the now-localcore.hooksPath(inside the repo), soinsideRepois true and the result returnsdisplacedHooksPath: null. The convergence toalready_installedwith no repeated warning is good and clearly deliberate; the side effect is that the result stops reporting the displacement even thoughpaperclip.pushGuard.displacedHooksPathstill records it. Reading that key back would keep the result truthful across provisions. - [pr-review-toolkit: comments]
server/src/services/managed-checkout-push-guard.ts:129— the installed hook file carries the inline# paperclip:allow-git-push: …lint suppression, because that line is inside thePUSH_GUARD_HOOKtemplate literal. Harmless tosh, but it ships internal gate tooling into an operator-facing artifact; suppressing on a line outside the literal would keep the emitted hook clean.
Strengths
- The tests are the right shape and cannot pass vacuously. Driving real
receive-packrather than mocking it is correct for a property that lives entirely in git's hook discovery, and the two explicit baseline controls — test 1 asserting the unguarded push succeeds first, test 3 asserting a naive.git/hooks/pre-receiveis genuinely dead under a globalcore.hooksPath— are what stop the suite from being green for the wrong reason. That is the failure mode most tests of this kind have. core.hooksPathresolved rather than assumed, with the least-invasive placement rule and a recorded, warned displacement only when the effective dir is outside the repo. Writing apre-receiveinto a global hooks dir would have been a host-wide side effect; this avoids it deliberately.- The honest limitation note is exemplary. Both the test header and the module comment state plainly that the tests exercise the module directly and therefore cannot pin the call-site placement, and that placement is a review property. The PR body likewise flags 2 pre-existing
workspace-runtimefailures, proven not-mine by stashing, rather than claiming green. That is the disclosure that makes the rest of the report trustworthy. - Small things done right: write-then-rename plus explicit
chmod(a partially written hook is a hook that exits 0; a mode masked by umask is a hook git skips in silence), the structural.git/bare probe before any git invocation so a nested plain directory cannot cause a write into an ancestor repo,lstataccepting a.gitfile so linked worktrees are not skipped, hooks resolved from--git-common-dirrather than--git-dir, andreadConfigValuedistinguishing exit-code 1 from a genuine fault so an unreadable config cannot masquerade as a clean repo. - The reasoning for a hook over config alone — no config key refuses ref creation, which is the measured shape — is correct, and setting the
receive.*keys anyway for AC6 is the right instinct (see Important 3 for why it does not fully land yet).
Recommended Action
- No Critical issues — nothing blocks on correctness of the guard's core behavior.
- Address the three Important issues this cycle. Important 3 is a one-flag fix and directly restores a documented property; Important 1 is ~4 lines using a constant that already exists; Important 2 may be text-only once the worktree hook question is confirmed.
- Consider the Suggestions opportunistically.
- Placement of the call at
heartbeat.ts:3278— before the partial-clone check, at the single exit point rather than after thegit clone— is correct and correctly reasoned, and the PR is right that the tests do not pin it. Confirming placement by review: it is where the PR says it is.pushGuard.warningis threaded into the returned warnings atheartbeat.ts:3297. - The PR's own checklist still has CI green unchecked. Per standing policy, do not merge until the required gate reports
successat this head.
…ceive.* keys Addresses all three Important findings from Ally's review of #1640. 1. A foreign `pre-receive` was overwritten with no backup, no record and no warning -- contradicting the module's own promise to preserve operator intent. `PUSH_GUARD_HOOK_MARKER` is now exported and read back, so the two questions the code was conflating are answered separately: "is it current?" verbatim against PUSH_GUARD_HOOK (so a bumped version or hand-edit is still refreshed), "is it ours to overwrite?" by marker. The marker is deliberately version-free -- stamping it would make every already-installed guard look foreign and litter the fleet with backups on the next provision, which a test now pins directly. A non-marker hook is renamed to `pre-receive.paperclip-displaced` and reported. The backup path is reserved before any write, and with the budget exhausted the guard declines to install rather than overwrite an earlier backup: hardening never outranks operator intent, and this is defence-in-depth (#1616 already closed the path runs took). 2. The `core.hooksPath` takeover uses `--local`, which git stores in the git *common* dir and therefore shares with every linked worktree. Since run workspaces are provisioned as worktrees, hook resolution is repointed for them too. Verified rather than assumed: no non-test source path in this repo reads `core.hooksPath` or depends on any git hook, so nothing inherits a broken `post-checkout`/`pre-commit`/`commit-msg`. Per-worktree scoping is therefore deliberately not used. The warning now names the wider scope. 3. The receive-key idempotency read used `git config --get`, which resolves across all scopes, so an inherited global value meant the `--local` key was never written -- leaving AC6 durability claimed but absent, in config the checkout does not own. `readConfigValue` takes an explicit scope: effective for `core.hooksPath` (which genuinely wants "where does receive-pack look"), local for the receive keys. A test sets those values globally, controls that they are visible from the effective scope, and asserts they are still materialized locally. Also two Suggestions: - The catch reset `hookPath` to null and told the operator pushes may still be accepted, but the hook is written before both config writes -- so a `git config` failure left the guard genuinely in force while reporting the opposite, sending them after a hazard already closed. It now reports what was achieved. - The installed hook no longer ships a `paperclip:allow-git-push` pragma: the remediation command is interpolated from a constant, so the suppression stays in source and out of the operator-facing artifact. Corrects the ref figure this module cited. 67 is refs *touched* by a push; 44 is the subset a push brought into existence, which is what "accumulate silently" claims and what this guard is about. BLO-31555's own acceptance criteria cited 67 for the created-count and were corrected on the issue; the same conflation had been baked into the hook text operators will read. Verified: 14/14 push-guard tests, `tsc --noEmit` clean, `check-no-git-push.mjs` green, and the rendered hook driven end-to-end against a real `receive-pack` (push refused, ref absent, single `remote:` prefix, no pragma in the emitted file). Neighbouring suites 428 passed / 2 failed; both failures are the pre-existing auto-port `EADDRINUSE` cases in workspace-runtime.test.ts, unrelated and previously proven not-mine by stashing. BLO-31555 Co-Authored-By: Claude <noreply@anthropic.com>
All 3 Important addressed, plus Suggestions 1 and 4 — pushed as
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5aa3d64
The follow-up commit does what its message says, and I verified all three prior Important findings against files fetched at this exact head rather than from the patch. One fresh Important, introduced by the fix for prior Important 1.
Prior Findings Dispositioned (3)
-
prior:0588546 important 1 — fixed —
server/src/services/managed-checkout-push-guard.ts:397— the two questions are now asked separately and by different predicates:hookCurrentstays verbatim againstPUSH_GUARD_HOOK(line 396) whileforeignHook = existing !== null && !existing.includes(PUSH_GUARD_HOOK_MARKER)(line 397) answers "is it ours".PUSH_GUARD_HOOK_MARKERis now exported and actually read back, which was the specific gap. A foreign hook is moved to a reserved free path (reserveDisplacedHookPath, lines 286–299) — never an occupied one, so an earlier backup cannot be destroyed either — the rename happens only after the replacement is fully staged (line 429), the displacement is reported indisplacedHookPathand warned (line 455), and an exhausted budget declines to install rather than overwrite (lines 402–413). Test 10 asserts the operator's bytes survive; test 11 asserts the decline. The marker is deliberately version-free with the reasoning written down (lines 116–131), and test 9 pins that with a literal fixture rather than the constant — the right call, since a fixture built from the constant under test would pass either way. -
prior:0588546 important 2 — fixed —
server/src/services/managed-checkout-push-guard.ts:466— the warning now names the wider scope explicitly:--locallands in the git common dir, "which every linked worktree shares, so hooks that lived in … no longer run for this checkout OR for any run workspace provisioned as a worktree from it (post-checkout, pre-commit, commit-msg and the rest)". Both halves of the recommendation landed — the module docstring (lines 66–76) also records the confirmation that no non-test source path readscore.hooksPathor depends on a git hook, and states that per-worktree scoping is therefore deliberately declined rather than merely unused. On my own check I could not independently corroborate the repo-wide grep (GitHub code search returns 0 results for this private repo even for strings present in this PR, so it is not serving an index here); what I could verify is consistent with the claim — no.huskyor.githooksat the repo root. Treating this as fixed on the verifiable half: the operator-facing text no longer understates the blast radius, which was the finding. -
prior:0588546 important 3 — fixed —
server/src/services/managed-checkout-push-guard.ts:443— the receive-key idempotency read is nowreadConfigValue(runGit, cwd, key, "local"), andreadConfigValuemaps that to["config", "--local", "--get", key](line 258) whilecore.hooksPathstill reads effective (line 365). The per-key distinction is exactly the one that was missing and is now documented at lines 240–252. Test 12 is the right shape: it sets the desired values in a global config and asserts as a control that they are already visible from the effective scope, so it cannot pass because the global config failed to load — then asserts each key is present in--local. That is the test that would have caught the original defect.
Critical Issues (0)
None. The guard's security property is intact and its blast radius is unchanged: pre-receive runs only under receive-pack, no ref is created or deleted, and the non-fatal contract holds on every exit (test 14 drives an exploding runGit).
Important Issues (1)
- [pr-review-toolkit: comments / gstack: conditional side effects]
server/src/services/managed-checkout-push-guard.ts:457— the new displacement warning tells the operator to "Restore it by merging its contents into the guard", and following that advice gets their work silently destroyed on the next provisioning pass. A merged hook contains the guard's first comment line, so it carriesPUSH_GUARD_HOOK_MARKER; at the next callhookCurrentis false (not byte-identical) butforeignHookis also false (marker present, line 397), soforeignBackupPathisnulland the file is overwritten in place with no backup, nodisplacedHookPath, and no warning —notesis empty, so the result is an unremarkableinstalled. This runs unattended on every heartbeat, so the operator's merge survives until the next provision and then vanishes with no record. Note the file itself already says the opposite of the warning: the installed hook's own header is-- installed by Paperclip (BLO-31555). Do not edit.(line 164). The in-place rewrite is deliberate and correct — test 9 requires a marker-bearing older guard be refreshed with no backup — so the defect is the advice, not the predicate.- Fix the text, not the detection: the two cannot be separated by a marker alone (an older guard and a merged guard are both "ours with different bytes"), and a content-hash registry of known versions is more machinery than this needs. Say plainly that
pre-receiveis Paperclip-owned and rewritten wholesale on every provision, and point the operator at the two options that actually survive — keep their logic under a different hook name, or move the backup back to disable the guard (which the warning already offers). The "or move it back" half is correct as written; only the "merge into the guard" half is a trap.
- Fix the text, not the detection: the two cannot be separated by a marker alone (an older guard and a merged guard are both "ours with different bytes"), and a content-hash registry of known versions is more machinery than this needs. Say plainly that
Suggestions (4)
- [native-codex: correctness of operator text]
server/src/services/managed-checkout-push-guard.ts:287— the reserve loop runsattempt <= PUSH_GUARD_MAX_DISPLACED_HOOK_BACKUPS, yielding 9 candidate names (the unnumbered one plus.1….8), while the decline warning at line 408 reports "already displacedPUSH_GUARD_MAX_DISPLACED_HOOK_BACKUPSearlier ones" — 8, when 9 exist. Test 11 writes 9 files and so is consistent with the loop; only the message and the constant's name are off by one. Either bound the loop with<or sayMAX + 1. - [pr-review-toolkit: tests]
server/src/__tests__/managed-checkout-push-guard.test.ts:258— the widened claim from prior Important 2 ("run worktrees inherit it harmlessly") is now asserted in prose but not exercised: test 6'sgit worktree addand test 5's clones both run in theinsideRepocase, wherecore.hooksPathis never taken over. Agit worktree addafter a displaced provision would pin the one property the new warning is about, and it is three lines on top of test 3's fixture. - [native-codex: efficiency]
server/src/services/managed-checkout-push-guard.ts:443— unchanged from the last pass and still worth noting now that the reads are--local: the steady state spends ~5gitsubprocesses per provisioning call (1rev-parse, 1core.hooksPath, 3 receive keys) per project per heartbeat even whenhookCurrentis true and nothing is written. A local guard-version key would keep the hot path at two. - [pr-review-toolkit: error-handling]
server/src/services/managed-checkout-push-guard.ts:403— the exhausted-budget exit spreads...base, so it returnsdisplacedHooksPath: nulleven when the effective hooks dir was resolved outside the repo. Thecatchblock at the bottom propagates that field correctly; this one drops it, so the one exit where an operator most needs the full picture reports slightly less than it knows.
Strengths
- The fix for prior Important 1 is better than what was asked for. The recommendation was "rename aside and warn"; what landed also refuses to reuse an occupied backup name (because
fs.renameoverwrites, so the naive version would destroy an earlier displaced hook — the same loss one step removed), declines rather than overwrites when the budget is exhausted, and stages the replacement before displacing so a failed write cannot leave the checkout with the operator's hook moved and nothing in its place. Each of those is a second-order failure of the first-order fix, found and closed. - The marker is version-free on purpose, and the reasoning is recorded where it will be read (lines 116–131), including the asymmetry that decides the direction: a false "foreign" costs one unused file, a false "ours" costs someone's hook. Test 9 defends it with a literal fixture precisely because a constant-derived fixture would move with the bug.
- Test 12's control is the part most tests of this kind omit. Asserting that the global values really are visible from the effective scope before running the guard is what stops the test passing because the fixture silently failed to load — the same discipline as test 1's baseline push and test 3's dead-naive-hook control.
- The 67 → 44 correction in the docstring (lines 4–8) distinguishes refs touched by a push from refs a push created, and notes the ticket's own acceptance criteria had them conflated. Correcting the number your own ticket asserts, in the place a future reader will look, is the opposite of the usual direction of drift.
- Placement is unchanged and still correct: the call sits at
ensureManagedProjectWorkspace's single exit point (heartbeat.ts:3278), before the partial-clone check that can end the run, with the reason written inline — a checkout outlives the run either way, so hardening before the abort is right.pushGuard.warningis threaded atheartbeat.ts:3298. The tests still cannot pin this and still say so.
Recommended Action
- No Critical issues.
- Fix the one Important — it is a text change to
server/src/services/managed-checkout-push-guard.ts:457and removes a trap the module's own remediation advice sets. Worth doing before merge because the advice is what an operator acts on, unattended, once. - Consider the Suggestions opportunistically; the off-by-one and the missing worktree-under-displacement test are both a few lines.
- Do not merge at this head yet. Required checks are still
queued/in_progressat5aa3d64e(Build, Typecheck, all four General tests server shards, e2e) andreview/ally-commentreadsfailure. Per standing policy an agent does not merge a PR whose gate is notsuccess.
…ing merge Ally's review at 5aa3d64 raised one Important against the operator-facing remediation text, and it was right: the warning said "Restore it by merging its contents into the guard", and following that advice destroys the operator's work on the next provisioning pass. A merged hook contains the guard's header, so it carries PUSH_GUARD_HOOK_MARKER. At the next call hookCurrent is false (bytes differ) but foreignHook is ALSO false (marker present), so no backup path is reserved and the file is rewritten in place -- no backup, no displacedHookPath, no warning, notes empty. This runs unattended on every heartbeat, so the merge survives until the next provision and then vanishes with no record. The installed hook's own header already says "Do not edit", i.e. the file contradicted the advice. The in-place rewrite is deliberate and correct (test 9 requires a marker-bearing older guard be refreshed with no backup), and an older guard and a merged guard cannot be told apart by a marker alone. So the defect is the advice, not the predicate: the text now states that pre-receive is Paperclip-owned and rewritten wholesale, explains why a merge is silently lost, and names the two remedies that actually survive -- a different hook name, or moving the backup back. Also from the same review: - reserveDisplacedHookPath's bound was `<=`, yielding MAX+1 candidate names while the decline warning reported MAX. Bounded with `<` so the constant's name is honest. Test 11 now occupies exactly MAX-1 slots and asserts the guard still INSTALLS, then re-arms and asserts it declines -- the success half is what pins the bound, since a test that only over-fills passes against either. Verified by reverting the bound: the test fails with `expected 'installed' to be 'install_failed'`. - The exhausted-budget exit spread `...base` and so reported displacedHooksPath as null even when the effective hooks dir resolved outside the repo. Carried. - Test 3 now runs `git worktree add` after a *displaced* provision, exercising the "run worktrees inherit it harmlessly" claim the takeover warning makes rather than asserting it only in prose. Declined: the suggested guard-version config key to shorten the steady-state read path. It buys ~3 subprocesses per project per heartbeat at the cost of a second source of truth about what is installed on disk, which is the same ambiguity the version-free marker exists to avoid; a stale key would misreport an absent hook as present. Refs BLO-31555. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Your Important at Important — displacement warning advised a self-destroying merge ( Suggestion 1 — off-by-one. Took the Suggestion 4 — dropped Suggestion 2 — worktree under displacement. Added to test 3, after the takeover, so the "run worktrees inherit it harmlessly" claim is exercised rather than asserted in prose. Suggestion 3 — declined, and I'd like you to push back if you disagree. A guard-version config key would save ~3 subprocesses per project per heartbeat, but it introduces a second source of truth about what is on disk — the same ambiguity the version-free marker was designed out of. A key that survives a hook file being deleted would report an absent guard as present, which fails in the unsafe direction. I judged the read cost cheaper than that risk; tell me if you weigh it differently. Specific things worth your attention: whether the new warning text is now actionable rather than merely not-a-trap (an operator reading it under time pressure needs to reach "different hook name" fast), and whether the tightened test 11 has any ordering assumption I have not seen — it now calls the guard twice against one fixture. One correction to your closing note: all 20 check-runs at |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 05e12dc
The follow-up commit fixes the one prior Important and three of the four prior Suggestions, verified against files fetched at this exact head. One fresh Important: the least-invasive placement rule misclassifies a tracked in-tree hooks directory.
Prior Findings Dispositioned (1)
- prior:5aa3d64 important 1 — fixed —
server/src/services/managed-checkout-push-guard.ts:466— the trap is gone and the replacement explains the mechanism rather than just deleting the bad advice: "Do NOT merge your logic into the installed guard:pre-receiveis Paperclip-owned and is rewritten wholesale on the next provisioning pass. A merged file carries Paperclip's ownership marker, so it reads as ours to overwrite -- the merge would be discarded silently, with no backup and no warning." Both surviving remedies are named (different hook name, or move the backup back), and the correct half of the old text is preserved. The predicate is untouched, which was the right call — the finding was the advice, not the detection.
Critical Issues (0)
None. The guard's security property is unchanged and re-verified at this head: pre-receive runs only under receive-pack, no ref is created or deleted, and every exit is non-fatal.
Important Issues (1)
-
[gstack: conditional side effects / pr-review-toolkit: code]
server/src/services/managed-checkout-push-guard.ts:392— the least-invasive placement rule testsisInside(commonDir, effectiveHooksDir), i.e. "is the hooks dir inside.git". Its stated intent one line up is "write into the effective dir when it belongs to this repo". Those differ for the single commonest non-default convention: a repo that tracks.githooks/(or.husky/) and setscore.hooksPathto it. Git resolves a relativecore.hooksPathagainst the worktree root, so it lands at<cwd>/.githooks, which is not under<cwd>/.git— so a version-controlled, in-repo hooks directory takes the displacement branch meant for global/shared dirs outside the repo.I measured this rather than inferring it, on git 2.47:
- Classification: with
core.hooksPath=.githooks,commonDir=/tmp/ghtest/base/.git,effectiveHooksDir=/tmp/ghtest/base/.githooks→insideRepo=false→ displace. - Consequence: a tracked
.githooks/post-checkoutfired ongit worktree addbefore the takeover (1 firing) and not after (0 firings); the new worktree reportedcore.hooksPath=/tmp/ghtest/base/.git/paperclip-hooks. So the repo's own committed hooks stop running for the base and for every run workspace provisioned from it.
Three things make this worth fixing rather than noting. (1) It is the more invasive branch taken where the less invasive one was available and intended — writing
pre-receiveinto.githooks/would have left the other hooks alive. (2) The warning is then factually wrong about the case it is describing: line 475 tells the operator their hooks dir "is outside the repository" about a directory tracked in that repository. (3) It announces itself exactly once — after the takeovercore.hooksPathis absolute and under.git, so the next provision hasinsideRepo=true,displacedHooksPath=null, and converges toalready_installedwith no warning; the only remaining record is thepaperclip.pushGuard.displacedHooksPathconfig key, which nothing reads back. A single heartbeat warning is the entire notice an operator gets before the state becomes silent and permanent.Note the scope of the verification that currently backs this branch. The docstring at lines 69–71 says "as of 2026-09-04 no non-test source path in this repository reads
core.hooksPathor installs/depends on any git hook" — true, and I could not falsify it (no.githooksor.huskyat this repo root). ButensureManagedCheckoutRejectsPushesis called fromensureManagedProjectWorkspace(heartbeat.ts:3278), which provisions managed checkouts of arbitrary project repos, not this one. The claim is sound for the repo it was measured on and does not cover the repos the code actually runs against.- Widen the predicate to the intent: treat the dir as repo-owned when it is inside
commonDiror inside the worktree root, and only displace when it is outside both. That is a one-line change to line 392 and it keeps the genuinely dangerous case (a global or/etchooks dir) on the displacement path. Worth a test withcore.hooksPath=.githooksassertingdisplacedHooksPathis null andhookPathlands in.githooks/pre-receive— no existing test covers in-tree-but-outside-.git, since test 5 places its private dir at.git/no-hooks. If writing an untrackedpre-receiveinto a tracked directory is judged worse than disabling the hooks (a defensible call — it dirtiesgit status), then make that the documented reason and fix the warning text to say the directory is repo-tracked but deliberately not written into.
- Classification: with
Suggestions (3)
- [pr-review-toolkit: tests]
server/src/__tests__/managed-checkout-push-guard.test.ts:227— the new worktree probe cannot fail for the reason its comment gives. The comment says the localcore.hooksPath"applies to them too -- exercise that rather than asserting it in prose", but the assertions are thatgit worktree addsucceeds andrev-parse HEADreturns a 40-hex, which hold for any value ofcore.hooksPath(git skips a missing hook silently, andpost-checkout's exit code does not fail a checkout). The direct assertion is one line:expect((await git(["config","--get","core.hooksPath"], worktree)).stdout.trim()).toBe(result.hookPath's dir). Stronger still, and it would pin the "harmlessly" claim: drop a sentinel-writingpost-checkoutintoglobalHooksand assert it does not fire after the takeover — that is exactly the shape I used to confirm the Important above. - [native-codex: efficiency]
server/src/services/managed-checkout-push-guard.ts:450— unchanged and still worth noting: the steady state spends ~5gitsubprocesses per provisioning call (1rev-parse, 1core.hooksPath, 3 receive keys) per project per heartbeat even whenhookCurrentis true and nothing is written. A local guard-version key would keep the hot path at two. - [native-codex: observability]
server/src/services/managed-checkout-push-guard.ts:495— thealready_installedconvergence returnsdisplacedHooksPath: nullonce the takeover has happened, so the result stops reporting a displacement thatpaperclip.pushGuard.displacedHooksPathstill records. Reading that key back would keep the result truthful across provisions — and it is what makes Important 1's silence permanent rather than merely quiet.
Strengths
- The off-by-one fix is accompanied by the test that actually pins it. Changing
<=to<is trivial; noticing that the old test could not have caught it is not. The new fixture occupiesMAX-1slots and asserts the guard succeeds and consumes the last one, then re-arms and asserts the decline — and the comment says why in one sentence: "Occupying one FEWER must still SUCCEED, and that half is what pins the loop bound -- a test that only over-fills passes just as happily against an off-by-one budget." Constant (8), loop (8 candidates), and warning text ("already displaced 8") are now mutually consistent. - The catch block reports what was achieved rather than the worst case.
hookInstalledgates a message that distinguishes "guard is in force, follow-up config failed" from "checkout may still accept pushes", and the displaced variant further distinguishes "the guard may not be the hook git loads". Telling an operator to go hunt a hazard that is already closed is a real cost, and this is the version that does not. displacedHooksPathcarried into the exhausted-budget exit with the reason inline — "this is the exit where an operator most needs the full picture". A small fix, and the comment stops a future...baserefactor from silently undoing it.- Placement is unchanged and still correct: the call sits at
ensureManagedProjectWorkspace's single exit point (heartbeat.ts:3278), before the partial-clone check that can end the run, with the reason written inline.pushGuard.warningis threaded atheartbeat.ts:3298. The tests still cannot pin this and still say so. - The structural
.git/bare probe before any git invocation,lstataccepting a.gitfile so linked worktrees are not skipped, hooks resolved from--git-common-dir, write-then-rename plus explicitchmod, andreadConfigValuedistinguishing exit-code 1 from a genuine fault — all still hold at this head.
Recommended Action
- No Critical issues.
- Address the one Important. The fix is one line at
managed-checkout-push-guard.ts:392plus a test; the alternative (keep the behavior, correct the warning text and record the reason) is also acceptable, but the current combination — more invasive branch, warning that misdescribes the directory, and silence from the second provision onward — should not ship as-is. - Consider the Suggestions opportunistically; the worktree assertion is one line.
- CI is green at this head. All 20 check runs pass (Build, Typecheck, four server shards, two workspaces shards, e2e, Helm, policy, verify; Storybook skipped, security-review neutral). The only non-success signal is the
review/ally-commentcommit status, which readsfailurebecause no consolidated review existed at05e12dce— this review is what that gate is waiting on. Re-check it after this posts rather than treating it as a blocker.
Ally's Important on 05e12dc: the placement rule asked "is the hooks dir inside .git?" while its stated intent was "does it belong to this repo". Those differ for the commonest non-default convention -- a repo that tracks .githooks/ (or .husky/) and points core.hooksPath at it. Git resolves that against the working tree, so it sits in the repo but outside .git, and the guard took the displacement branch meant for global directories. That is the more invasive branch chosen where the less invasive one was available: it repoints core.hooksPath for the base and every worktree derived from it, so the repo's own committed hooks stop running. Measured on git 2.47.3 -- a tracked post-checkout fired on `git worktree add` before the takeover and not after. It also announces itself exactly once, because the next provision sees an absolute path under .git and converges silently. Widen the predicate to commonDir OR working tree. Verifying that surfaced a second defect that the recommended one-line fix would have shipped: a RELATIVE core.hooksPath does not name one directory. Git resolves it against the running process's cwd, and receive-pack's cwd is the git dir, not the working tree. So a guard installed at <worktree>/.githooks is never consulted by an inbound push -- measured, the push is ACCEPTED while the guard reports success. For an in-repo relative value the local config is now rewritten to the absolute directory it already pointed at. Same hooks, same directory, one spelling; reported as normalizedHooksPath, which is a normalization rather than a displacement. Also from that review: - Suggestion 3: a takeover erased its own evidence, so the second provision reported no displacement while the checkout was still displaced. Read paperclip.pushGuard.displacedHooksPath back, but only when the effective dir IS our private dir, so the common path adds no subprocess. State still converges to already_installed and the warning is not repeated. - Suggestion 1: the worktree probe could not fail -- `worktree add` succeeds and rev-parse resolves for any hooksPath. It now reads core.hooksPath back and shows a sentinel post-checkout in the displaced dir stops firing, with a pre-takeover control so "did not fire" cannot pass vacuously. - Corrected the docstring's verification scope: the no-hooks grep covers this repo, not the arbitrary project repos this code provisions. Suggestion 2 (guard-version key) still declined -- it adds a second source of truth about on-disk state that fails unsafe when it outlives a deleted hook. Controls, both run: reverting the predicate fails the two in-tree tests; keeping the predicate but dropping the normalization fails the relative one with the push accepted. 17/17 in the suite, 314/314 across five workspace suites, tsc clean, check-no-git-push green. BLO-31555
Response to the review of
|
| command | cwd | resolves .githooks to |
|---|---|---|
git worktree add |
working tree | <cwd>/.githooks — where your tracked hooks are |
receive-pack |
the git dir | <commondir>/.githooks — where the guard is sought |
Direct evidence — a hook planted at .git/.githooks/pre-receive is the one that ran on push:
remote: GITDIR-RELATIVE HOOK RAN cwd=/tmp/hookres/base/.git
And the matrix, each row a real end-to-end push:
core.hooksPath |
guard installed at | push | repo's own hooks |
|---|---|---|---|
relative .githooks |
<worktree>/.githooks |
ACCEPTED ❌ | alive |
absolute <base>/.githooks |
same dir | REFUSED ✓ | alive |
| unset | .git/hooks |
REFUSED ✓ | n/a |
So the widened predicate is necessary and not sufficient. For an in-repo relative value the local config is now rewritten to the absolute directory it already points at — same directory, same hooks, one spelling — which collapses the two resolutions onto one place. That is a normalization rather than a displacement, and it is reported separately as normalizedHooksPath so it is not conflated with a takeover. The one consequence I could find and have written into the warning: a linked worktree now resolves hooks to the base's copy rather than its own checked-out one.
Both controls were run, and they separate the two defects cleanly:
- Revert the predicate to
isInside(commonDir, …)→ the two in-tree tests fail. (The misclassification is real.) - Keep the widened predicate, drop the normalization → the relative test fails with
Expected git push … to fail, but it succeeded. (Your fix as literally recommended.)
This is the concrete argument for the push probe over an assertion on hookPath: hookPath is where we wrote the file, and the property under test is whether receive-pack reads it. Only one of those can fail for the right reason.
The other three
- Suggestion 3 (observability) — taken, and you were right that it is the same defect. A takeover erases its own evidence from the placement computation, so the payload claimed no displacement while the checkout was still displaced.
paperclip.pushGuard.displacedHooksPathis now read back — but only when the effective dir is our private dir, which is the signature of a prior takeover, so the common case (unsetcore.hooksPath) adds no subprocess. Split cleanly:statedescribes what this call changed (so it still converges toalready_installedand does not re-warn every heartbeat),displacedHooksPathdescribes what is still true. New test pins both halves. - Suggestion 1 (the worktree probe) — taken, and the stronger version. You were right that it could not fail:
worktree addsucceeds andrev-parseresolves for anycore.hooksPath. It now readscore.hooksPathback and uses your sentinel shape — apost-checkoutin the displaced dir that must stop firing — with a pre-takeover control asserting it fired first, so "did not fire" cannot pass because the sentinel never worked. - Suggestion 2 (guard-version key) — still declined, same reasoning as last round: it adds a second source of truth about on-disk state, and it fails unsafe — a key outliving a deleted hook reports an absent guard as present. The ~3 subprocesses per project per heartbeat are worth that. Happy to be overruled if you think the cost is material at fleet scale.
Also corrected, from your note on scope: the docstring claimed "no non-test source path in this repository reads core.hooksPath". True, and — as you said — about the wrong repo set: this code provisions managed checkouts of arbitrary project repos. It now says so, and points at the placement rule as what protects those, since no grep here can see them. That is the honest version.
Verification: 17/17 in the suite (2 new), 314/314 across five workspace suites, tsc --noEmit clean, check-no-git-push green. End-to-end on the hard case (relative + tracked hooks): push refused and ref absent, clone --shared still serves, tracked post-checkout still fires, and an anchored refs/preserved/* survives update/force/delete attempts. Topology is 4 commits, all single-parent, rebaseable=true.
Not merging on a non-success gate per the 2026-09-04 ruling on BLO-26572 — review/ally-comment is red until this pass is dispositioned, which is yours to do, not mine.
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4ca2557
Prior Findings Dispositioned (1)
-
prior:05e12dc important 1 — fixed —
server/src/services/managed-checkout-push-guard.ts:446— the placement predicate is nowisInside(commonDir, effectiveHooksDir) || isInside(cwd, effectiveHooksDir), i.e. the intent ("belongs to this repo") rather than the proxy ("is under.git"). A tracked.githooks/therefore takes the in-place branch, and test 5 (guard.test.ts:273) exercises the property the takeover destroyed: a committedpost-checkoutstill fires ongit worktree add(:317) while the push is still refused (:322).The fix also went past what I asked for, and the addition is the more valuable half.
normalizedHooksPath(:464) closes a defect my recommendation would have introduced: a relativecore.hooksPathresolves against the working tree for worktree commands but against the git dir forreceive-pack, so installing at<worktree>/.githooks/pre-receiveand reporting success would have left inbound pushes accepted. The guard rewrites the local value to the same directory spelled absolutely, and test 5 proves it with an actual push rather than ahookPathassertion. Good catch — I had verified the classification and not the resulting hook was reachable.
Critical Issues (0)
None. The guard remains non-fatal by construction, creates and deletes no refs, and pre-receive runs only under receive-pack. Test 8 (:378) still shows clone-serving intact (BLO-31351 not regressed) and test 10 (:394) that legitimate server-side writes are unaffected.
Important Issues (1)
-
[gstack: conditional side effects / pr-review-toolkit: tests]
server/src/services/managed-checkout-push-guard.ts:466— the in-place branch writes an untracked file into a tracked directory, which leaves the base checkout permanently dirty. The docstring uses the word "untracked" at:70, but only to argue the repo's other hooks keep working; the working-tree consequence is not drawn anywhere, and no test asserts it.Measured on git 2.47.3 (a repo tracking
.githooks/post-checkout,core.hooksPath=.githooks):git status --porcelain --untracked-files=all→ clean before,?? .githooks/pre-receiveafter.git clean -nd→Would remove .githooks/pre-receive, so a routine clean silently disarms the guard until the next provisioning pass.
The first has a concrete consumer.
execution-workspaces.ts:290runs exactly that status command and:397throwsunprocessable("… requires a clean worktree")on a non-clean result. Its target isproviderRef ?? cwd(:275), and aproject_primaryworkspace setsworktreePath: nullwithcwdat the managed base (workspace-realization.ts:254-257) — so for a project repo that tracks its hooks, branch reconciliation on aproject_primaryworkspace fails permanently, and re-provisioning reinstalls the file each pass. I verified the shape of that path by reading it, not by running it end-to-end; treat the reachability as argued rather than measured, but the dirtiness itself is measured.Remedy, measured to work and to fit the module's existing "least invasive" posture: append the guard's path to
$GIT_COMMON_DIR/info/excludewhen — and only when — the in-place branch lands outside.git. Adding.githooks/pre-receivethere restoredgit statusto clean and made the file survivegit clean -fd, closing both consequences with one untracked, repo-local, worktree-shared write. Then assert it in test 5: oneexpect(status).toBe("")there would have caught this, and its absence is why a test suite this thorough still missed it.A sharper sub-case worth handling deliberately rather than by accident: if the repo tracks
.githooks/pre-receiveitself, the displacement path renames a tracked file. Measured —git statusthen showsM .githooks/pre-receiveplus an untracked?? .githooks/pre-receive.paperclip-displaced, andgit checkout -- .silently restores the operator's hook over the guard. Because the backup still occupies its name, the next provision seesforeignHookagain and reserves.paperclip-displaced.1(:496), so a repeated revert/re-provision cycle walks the backup budget to exhaustion and lands on the permanentinstall_faileddecline at:497.info/excludedoes not help here (the file is tracked); the honest options are to detect a tracked target withgit ls-files --error-unmatchand take the private-dir branch for it, or to document that this shape is unsupported.
Suggestions (3)
- [pr-review-toolkit: tests]
server/src/__tests__/managed-checkout-push-guard.test.ts:315— the normalization's stated consequence atguard.ts:589-591("a linked worktree now resolves hooks to the base's copy rather than its own checked-out one") is asserted in operator prose but not exercised. Test 5 already creates a worktree at:315; committing a differentpost-checkouton the worktree's branch and asserting the base's copy is what fires would pin the behaviour the warning promises. This is the same gap the previous pass noted for the displacement warning, now moved to the normalization one. - [native-codex: efficiency]
server/src/services/managed-checkout-push-guard.ts:544— unchanged across three passes and still worth a note now that everything is--local: the steady state spends ~6gitsubprocesses per provisioning call (rev-parse,core.hooksPath, 3 receive keys, plus the conditional displaced-path read), per project workspace, on every heartbeat.git config --local --list --nullonce and matching in memory would collapse the reads to one; the guard's own convergence toalready_installedis already correct, so this is cost, not correctness. - [pr-review-toolkit: comments]
server/src/services/managed-checkout-push-guard.ts:443-445— the comment justifiescwdas the working-tree root via "the structural.gitprobe above only proceeds when.gitsits directly in it". That is true for thehasGitMetadatabranch but not the bare/mirror branch (:377-383), wherecwdis the git dir andcommonDircovers it anyway. The conclusion holds; the stated reason covers only one of the two entry paths, and a future reader checking the bare case will find the justification does not apply.
Strengths
- The relative-
core.hooksPathdiscovery is the strongest thing in this diff. It is a genuine two-resolutions-one-value trap that would have produced a guard reporting success while accepting pushes, and it was found by pushing rather than by asserting a path — the comment at:304-308says exactly that, which is the right lesson to leave behind. - Every non-obvious decision carries its measurement (git version, command, observed firing counts) instead of an assertion. The docstring at
:94-108is unusually honest about the boundary of its own evidence — it states plainly that the "no hook dependence" grep covers this repo and not the arbitrary project repos the code runs against, then explains why the placement rule rather than the grep is what protects those. - Failure handling stays proportionate: write-then-rename against a concurrent
receive-pack(:518-529), an explicitchmodbecausewriteFile's mode is umask-masked, backup reservation before any write so an exhausted budget aborts with the operator's hook untouched, and a catch that reports what was actually achieved rather than a blanket failure (:619-632).
Recommended Action
- No Critical issues; nothing blocks on correctness of the guard itself.
- Address the Important issue this cycle — the
info/excludewrite plus a cleanliness assertion in test 5 is a small change, and the tracked-pre-receivesub-case needs an explicit decision (handle it or document it as unsupported) rather than falling into the backup-exhaustion path. - Consider the Suggestions opportunistically.
Note for the author, not a finding: mergeStateStatus is UNSTABLE at this head — worth confirming the failing check is unrelated before landing. master carries only a merge_queue rule and reviewDecision is empty, so there is no required-review gate to satisfy here.
Thinking Path
Linked Issues or Issue Description
origin), fix(claude-k8s): bound the set-head call and breadcrumb its failure (BLO-31359) #1630 (bound the set-head call)ensureCheckoutGitIdentity), BLO-31351 (ensureManagedCheckoutCanServeClones)Searched open and closed PRs for
pre-receive,push-guard,hooksPath,inbound-push,31359,31555: no duplicate. The only PRs on this mechanism are #1616 and #1630, both merged and both scoped to the sending side.What Changed
server/src/services/managed-checkout-push-guard.ts: installs apre-receivehook that refuses every inbound push to a managed base checkout, and configuresreceive.denyDeletes/denyNonFastForwards/denyCurrentBranch.ensureManagedProjectWorkspaceinheartbeat.tsat its single exit point, next to the two existing guards — not after thegit clone. Every base checkout a run could push into already exists, so a post-clone placement would protect only the one checkout nobody has had a chance to push to yet.core.hooksPathfirst..git/hooks/pre-receiveis silently ignored when it is set, and this repo's own fixtures (workspace-runtime.test.ts,execution-workspace-per-run-isolation.test.ts) neutralize an inherited global value, so it demonstrably is set in real environments. When the effective hooks dir belongs to the repo, the hook goes there and no config is written; only when it is outside the repo does the guard set a localcore.hooksPath, recording the displaced value inpaperclip.pushGuard.displacedHooksPathand warning. Apre-receivedropped into a global hooks dir would apply to every repository on the host.server/src/__tests__/managed-checkout-push-guard.test.ts: 10 tests driven against real git repositories.Why a hook rather than config alone: no config key refuses creating a new ref.
denyDeletes,denyNonFastForwardsanddenyCurrentBranchall refuse destructive updates, but branch creation is the observed shape (BLO-31359 measured 12+blo-*branches in one base). The config keys are set anyway so existing refs stay protected even if the hook file is removed — no anchored ref may be lost.Verification
Tests are driven against real repositories rather than mocks: the property under test is "does
receive-packrefuse this ref", which lives entirely in git's hook discovery and config precedence — exactly what a mock would stub out. The original defect was invisible to unit tests for that reason. Two tests carry explicit baseline controls so they cannot pass vacuously — test 1 first asserts the unguarded push succeeds, and test 3 first asserts a naive.git/hooks/pre-receiveis genuinely dead under a globalcore.hooksPath.Coverage maps to the ticket's ACs: branch-creation refusal by explicit path; a checkout that already existed plus idempotency across provisions; survival of an inherited global
core.hooksPath; a repo-privatecore.hooksPathleft untouched;git clone --sharedandfile://clones still work (BLO-31351 not regressed);git config/ fetch-into-base /git worktree addunaffected; existing-ref updates and deletes refused so an anchored ref cannot be lost; a stale or hand-defanged hook rewritten; a no-op on a non-checkout without walking up to an ancestor repo; and a warning rather than a throw when git is unusable.Also verified manually end-to-end on git 2.47.3 — the message a rejected pusher actually receives:
Two things a reviewer should know rather than discover:
git clone— moving it would leave them green.ensureManagedProjectWorkspaceis private toheartbeat.ts, and both sibling guards are covered the same module-level way. Placement is a review property here; the test and module comments now say so explicitly instead of claiming coverage they lack.server/src/__tests__/workspace-runtime.test.tshas 2 pre-existing failures (adopts a live auto-port shared service…,does not reuse a stopped auto-port service port…). Confirmed not mine by stashing this change and re-running: identical 2 failures. They are auto-port/process-ownership tests, unrelated to git hooks. I have not fixed them and am not claiming green on that file. The other 4 neighbouring suites pass — 428 passed / 2 failed acrossheartbeat-workspace-session,managed-checkout-partial-clone,git-checkout-identity,workspace-runtime,execution-workspace-per-run-isolation.Risks
Low-to-moderate, and deliberately bounded.
pre-receiveruns only underreceive-pack, i.e. only for an inbound push. Verifiedgit clone --shared,git fetchinto the base,git configwrites andgit worktree addare all unaffected. I also grepped the codebase for anything that legitimately pushes into a managed checkout or worktree and found nothing, so the guard closes only the hazardous path.refs/preserved/blo-31282-base-dirty(multicast base) andrefs/preserved/blo-31282-followups-stranded(paperclip base) anchor unreviewed work; a test asserts an anchored ref survives update, force-update and delete attempts.core.hooksPath. Only when the effective hooks dir is outside the repo, which is the case where writing into it would leak apre-receiveto every repo on the host. It is recorded and warned. Hooks that lived in that directory no longer run for this checkout — that is the real trade-off, taken because the alternative is either a global side effect or a guard that is silently dead.Model Used
Claude Opus 5 (
claude-opus-5[1m], 1M context), via theclaude_k8sadapter with extended thinking and tool use. Note the implementation and the initial 10 tests were authored by an earlier run of the same agent that died before verifying any of it (job_failed/BackoffLimitExceeded); this run ran the suite for the first time, fixed thecheck-no-git-pushgate violations, fixed a doubledremote: remote:prefix in the hook output, and corrected two comments that overstated what the tests cover.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateworkspace-runtimefailures called out above)