Skip to content

Commit 8b81ab6

Browse files
os-zhuangclaude
andauthored
fix(hooks): guard-governed-enqueue's sibling-checkout lookup is injectable, so its self-test is hermetic on a box that carries the sibling (#18365)
Fixes #18321 ## The defect `.claude/hooks/guard-governed-enqueue.selftest.sh`'s case "an exception-row path in a repo this container cannot resolve" rested on a premise about the **box**, not about the hook: that no `objectstack-ai/cloud` checkout sits beside this one. The hook resolves a sibling checkout by comparing origin slugs under the parent directory of its own repo root, so on a container that *does* carry a sibling `cloud` checkout the guard resolved it, recomputed the register predicate on that tree with `--root`, got the governed answer and **blocked**. The matrix read `54 passed, 1 failed` there, and was green in CI only because the runner mounts no sibling. `lint.yml`'s step comment calls these matrices hermetic; this case was not. ## What changed **The hook** now reads `OS_GOVERNED_ENQUEUE_SIBLING_ROOT` for the directory a sibling is resolved under. It moves **where** the search looks and nothing else — the origin-slug comparison remains the entire admission rule, so a sibling that resolves is audited exactly as before, and a root holding no matching checkout resolves nothing. No fail-open path was added or widened. | value | meaning | |:---|:---| | unset | the parent of this checkout — today's behaviour, to the byte | | empty | identical to unset; an empty value is an accident, and the safe reading of an accident is "no override", never "look nowhere" | | a directory carrying no matching checkout (one that does not exist included) | nothing resolves; the run proceeds exactly as on a box without the sibling | **The matrix** now owns its own premise: the "cannot resolve" case points the lookup at a directory it creates and knows is empty, and one new case pins the other half — a sibling that *does* resolve is audited on its own tree. The throwaway sibling is built in the self-test (`git init` plus an `origin` naming the target repo is the whole admission requirement) and removed by the existing `trap` on the matrix's own temp root, so no new cleanup path was needed. Both temp directories live under that root by construction. The new case is written as **agreement with the register**, not as a copied verdict — the shape this file's own header says it learned the hard way, where a hard-coded `expect allow` went red over an upstream register change the hook had nothing to do with. ## Reverse verification BEFORE, on `origin/main` `1411cf2c`, this container, `/home/user/cloud` present: ``` 54 passed, 1 failed FAIL want=allow got=block an exception-row path in a repo this container cannot resolve ``` AFTER, at `5391e5c0`, same container, `/home/user/cloud` still present — four injection states, all `exit 0`: ``` UNSET exit=0 56 passed, 0 failed EMPTYDIR exit=0 56 passed, 0 failed NONEXISTENT exit=0 56 passed, 0 failed EMPTYSTRING exit=0 56 passed, 0 failed ``` **"Unset changes nothing" is proved directly**, not inferred: the *untouched* matrix was run against the *changed* hook, and its output was byte-identical to the baseline log (`diff` empty — still `54 passed, 1 failed`, still the same one case). The variable's semantics were also measured against the hook directly, with the real sibling present, on a payload targeting `objectstack-ai/cloud`: ``` unset -> block (resolves the real sibling) empty string -> block (identical to unset) =/home/user -> block (explicit, same directory as the default) empty dir -> allow (nothing resolves) non-existent -> allow (nothing resolves) ``` **Firing control.** Two were run, each mutating the committed file, proving the mutation landed on disk by hash, and restoring with `git checkout HEAD --` under a `trap` (`git diff HEAD` empty afterwards, blob hash back to the HEAD blob): - *the control this card prescribed* — give the new case a **not-governed** fixture: **stays green**, `56 passed, 0 failed`. It cannot fire, and that is a property of the assertion rather than a gap: an agreement assertion flips the register leg and the hook together, so they still agree. Reported rather than papered over. - *a control that targets the property under test* — the throwaway sibling's `origin` names a different repo, so it is no longer admitted: **red**, `55 passed, 1 failed`, `FAIL want=block got=allow a sibling checkout that resolves is audited, never waved through`. This is the mutation that corresponds to "the hook stopped consulting the sibling tree", which is what the case exists to catch. Diff confined to the two files in the declared surface: ``` .claude/hooks/guard-governed-enqueue.selftest.sh | 92 +++++++++++++++++++--- .claude/hooks/guard-governed-enqueue.sh | 41 +++++++++- 2 files changed ``` ## Gates `dispatch-gates.mjs` derives 13 families from the real change set (2 paths, three-dot vs merge base `1411cf2c6`); all 13 ran and all recorded `exit 0`. Reconciliation: ``` Run reconciliation — 13 derived, 13 run, 0 NOT-MEASURED, 0 UNRUN. ``` `check-doc-formula-expressions` first returned **exit 3 (PREREQUISITE NOT MET — nothing measured)** because two workspace packages were unbuilt; it was re-run to `exit 0` after a targeted build, and only that second reading is recorded. The path face `check-governed-merges.mjs --test` answers **GOVERNED** (`.claude/**`), as expected. `shellcheck` is **not measured**: the tree wires no shellcheck step (`lint.yml` runs the hook matrices, it does not lint them) and the binary is absent from this container. `bash -n` parses both files and `check:bash32-floor` passes. ## Acceptance notes Two things measured on the way, both **out of scope for this PR** and neither fixed here: 1. **The case never reached the fail-open it claimed to pin.** The comment that used to sit on it described the "no checkout of the target repo is available" branch. It does not reach that branch, in any environment including CI: with nothing resolved the register is asked *without* `--root`, answers about *this* tree, finds the path byte-exact against its own generator and lifts it, so the hook leaves at the cleared-predicate `exit 0` with **empty stderr**. That branch is reachable — a path hitting the exception row but absent from the generator's declared output set returns governed with a non-empty `exceptions` list — but no case in the matrix reaches it today. The stale comment is corrected here because it sits on the case being re-keyed; pinning the branch is separate work. 2. **The slug reader keeps a `.git` suffix.** Its path character class owns the dot and is greedy, so an origin of `https://github.com/objectstack-ai/cloud.git` yields `objectstack-ai/cloud.git` and matches nothing. A sibling cloned with the conventional URL therefore does not resolve. The same expression also derives the slug for a bare `gh pr merge NUMBER`, where a non-matching slug makes the API read 404 and the guard fail open. Measured here; reported for its own card. The new case uses the bare URL form on purpose and says so in place, so nobody "tidies" a `.git` onto it. Also noted, not filed: `lint.yml`'s step is named for the two matrices that existed when it was written, while discovery now picks up more. The step comment is explicit that discovery is the contract and that a hard-coded list would be the defect, so the name is illustrative rather than a ledger — nothing to reconcile. Whoever adds the next hook matrix is the one who reads it. `lint.yml` is outside this PR's declared surface and was read, not edited; its hermetic claim becomes true rather than edited. Serial context: PR #18317 touched `.claude/settings.json` and landed before this branch was cut; it is unrelated to either hook file and nothing here depends on it. ## 维护者速读(草稿) **改了什么** — 给这个 PreToolUse 守卫加了一个环境变量,用来指定「到哪个目录下去找兄弟仓 checkout」。默认不设时行为与今天逐字节一致。自测脚本据此改写了一个用例,并新增一个用例。 **为什么改** — 这个自测用例原本依赖「这台机器上没有 cloud 仓的 checkout」这个环境事实,而不 是依赖守卫本身的行为。凡是挂了 `/home/user/cloud` 的机器上它就红,CI 绿只是因为 runner 上 恰好没有。测试的结论必须由被测代码决定,不能由机器上还挂了什么决定。 **风险与代价(含回滚)** — 风险低。变量只改变「去哪里找」,不改变「找到了算不算数」—— 判定仍然是比对 origin slug,所以误设一个值只会让它找不到(退回到本来就存在的放行分支), 不会让它放过本该拦截的东西。已逐项实测 unset / 空串 / 空目录 / 不存在目录四种取值。回滚就是 还原这两个文件,无数据迁移、无发布物、无下游依赖。 **席位意见** — (待席位填写) **你要做的** — `.claude/**` 属受管面,按 Prime Directive #14 需要一次授权的 APPROVED 审核; 本 PR 保持 draft,未请求任何 reviewer,未触碰 auto-merge。除批准外无需其他动作。 --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01HZfg2AwVX191qCizp88gQr --- _Generated by [Claude Code](https://claude.ai/code/session_01HZfg2AwVX191qCizp88gQr)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 05f1cdd commit 8b81ab6

2 files changed

Lines changed: 127 additions & 6 deletions

File tree

.claude/hooks/guard-governed-enqueue.selftest.sh

Lines changed: 88 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -342,12 +342,96 @@ expect allow 'a PR reporting no changed files is not a governed answer' \
342342
"$(mcp $AUTO 13794)" "OS_GOVERNED_ENQUEUE_FIXTURE=$F_EMPTY"
343343

344344
echo "== a generated-exception row on a repo with no checkout to recompute against =="
345-
# objectstack-ai/cloud has no sibling checkout here, so the register cannot
346-
# recompute the row's provenance on the RIGHT tree. Judging one repo's paths
347-
# against another's files would be worse than not answering: fail open, say so.
345+
# With no checkout of the target repo the register cannot recompute the row's
346+
# provenance on the RIGHT tree, and judging one repo's paths against another's
347+
# files would be worse than not answering: fail open, say so.
348+
#
349+
# ⚠️ THIS CASE'S PREMISE IS INJECTED, AND THE INJECTION IS THE REPAIR. It used to
350+
# rest on a fact about the BOX — "objectstack-ai/cloud has no sibling checkout
351+
# here" — which is a property of the container, not of the hook, and not true
352+
# everywhere: on a box that does carry a sibling `cloud` checkout the guard
353+
# resolved it, recomputed the predicate on it and BLOCKED, so this matrix read
354+
# `54 passed, 1 failed` there and was green in CI only because the runner
355+
# mounts no sibling. A case whose verdict depends on what else happens to sit
356+
# next to the checkout is not hermetic, whatever the step comment says. It now
357+
# points the lookup at a directory it created itself and therefore knows is
358+
# empty, so the "cannot resolve" premise is one this file OWNS on every box.
359+
# ONE spelling of the path, read by the fixture the hook is handed AND by the
360+
# register leg below. Two spellings drift: change the fixture alone and the
361+
# agreement leg goes on asking about the old path, which is agreement with a
362+
# question nobody asked.
363+
CROSS_REPO_PATH=skills/objectstack-data/references/_index.md
364+
F_CROSS_REGEN="$(fixture cross-repo-regen "$(files_of "$CROSS_REPO_PATH")" "$NO_REVIEWS")"
365+
NO_SIBLING_ROOT="$root/no-sibling-here" # under $root: the existing trap removes it
366+
mkdir -p "$NO_SIBLING_ROOT"
367+
#
368+
# ⚠️ AND IT DOES NOT REACH THE "no checkout … is available" FAIL-OPEN — measured,
369+
# because the comment that used to sit here said it did. With nothing resolved
370+
# the register is asked WITHOUT `--root`, so it answers about THIS tree, where
371+
# this path is byte-exact against its own generator and therefore LIFTED: the
372+
# hook leaves at the cleared-predicate `exit 0` with EMPTY stderr, several
373+
# branches above that fail-open. Pinning a warning here would pin a sentence
374+
# nothing prints. What this case does hold is the property the card is about —
375+
# the verdict must not depend on what else is mounted beside the checkout — and
376+
# the resolved-sibling case below is its other half: same fixture, same payload,
377+
# only the injected root differs.
348378
expect allow 'an exception-row path in a repo this container cannot resolve' \
349379
"$(mcp $AUTO 999 objectstack-ai cloud)" \
350-
"OS_GOVERNED_ENQUEUE_FIXTURE=$(fixture cross-repo-regen "$(files_of skills/objectstack-data/references/_index.md)" "$NO_REVIEWS")"
380+
"OS_GOVERNED_ENQUEUE_FIXTURE=$F_CROSS_REGEN" "OS_GOVERNED_ENQUEUE_SIBLING_ROOT=$NO_SIBLING_ROOT"
381+
382+
echo "== ...and a sibling checkout that DOES resolve is audited on its own tree =="
383+
# The other half of that same branch, and the reason the case above needs an
384+
# injection rather than a rename: when a checkout of the target repo IS
385+
# reachable, resolving it and recomputing the predicate there is the DESIGNED
386+
# behaviour — the answer then reflects the DIFF instead of the environment,
387+
# which is the whole point of passing `--root`. Pin only the fail-open and the
388+
# guard stays green after it stops looking for siblings at all.
389+
#
390+
# The sibling is BUILT here rather than borrowed from the box: `git init` plus an
391+
# `origin` naming objectstack-ai/cloud is the entire admission requirement, since
392+
# the hook compares origin slugs and reads nothing else. Measured on a container
393+
# carrying the real read-only /home/user/cloud checkout: this throwaway and that
394+
# checkout hand the register the SAME verdict with the SAME reason — governed,
395+
# `pureRegeneration: false`, "the generator declared no output set … fail closed"
396+
# — because the `gen:skill-refs` toolchain cannot run on either tree. The
397+
# throwaway reproduces the real sibling, so no further tree is needed.
398+
#
399+
# ⛔ The origin URL is the BARE form on purpose — do not "tidy" a `.git` suffix
400+
# onto it. The hook's slug reader keeps that suffix (its path character class
401+
# owns the dot and swallows it, leaving `cloud.git`), so the `.git` spelling
402+
# resolves NOTHING and this case would silently become a second copy of the one
403+
# above. Measured here; filed separately as its own defect, since the same
404+
# reader also derives the slug for a bare `gh pr merge <n>`.
405+
#
406+
# ⭐ Asserted as AGREEMENT with the register, for the reason the pure-regeneration
407+
# case above learned the hard way: `skills/**` leaving the governed fence, or this
408+
# exception row being retired, would flip the verdict for a reason the hook had
409+
# nothing to do with, and a verdict copied from the register makes this matrix a
410+
# second register. Here and in CI today that branch is `block`.
411+
SIBLING_ROOT="$root/sibling-parent" # under $root: the existing trap removes it
412+
mkdir -p "$SIBLING_ROOT/cloud"
413+
git -C "$SIBLING_ROOT/cloud" init -q >/dev/null 2>&1
414+
git -C "$SIBLING_ROOT/cloud" remote add origin https://github.com/objectstack-ai/cloud >/dev/null 2>&1
415+
node "$repo_root/scripts/pm/check-governed-merges.mjs" --test --root "$SIBLING_ROOT/cloud" \
416+
"$CROSS_REPO_PATH" >/dev/null 2>&1
417+
sibling_rc=$?
418+
if [ "$sibling_rc" -eq 0 ]; then
419+
sibling_want=allow
420+
sibling_branch='LIFTED on the sibling tree — the hook must answer the same way'
421+
else
422+
sibling_want=block
423+
sibling_branch="GOVERNED on the sibling tree (exit $sibling_rc, fail-closed: the generator cannot run there) — the refusal must stand"
424+
fi
425+
printf ' .. register verdict on the RESOLVED sibling: %s\n' "$sibling_branch"
426+
expect "$sibling_want" 'a sibling checkout that resolves is audited, never waved through' \
427+
"$(mcp $AUTO 999 objectstack-ai cloud)" \
428+
"OS_GOVERNED_ENQUEUE_FIXTURE=$F_CROSS_REGEN" "OS_GOVERNED_ENQUEUE_SIBLING_ROOT=$SIBLING_ROOT"
429+
# ⛔ No companion assertion that the fail-open text is ABSENT here — it was
430+
# written, and measured to be a phantom: when the sibling does NOT resolve the
431+
# hook does not print that warning either (it leaves at the cleared-predicate
432+
# exit with empty stderr, per the case above), so the assertion passed in both
433+
# worlds and discriminated nothing. The verdict row above is the discriminator,
434+
# and it is the one that goes red when the sibling stops being resolved.
351435

352436
echo "== the deliberate exception switch =="
353437
expect allow 'OS_ALLOW_GOVERNED_ENQUEUE=1 on the blocking case' \

.claude/hooks/guard-governed-enqueue.sh

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,34 @@
148148
# change where DATA comes from and nothing else; the open escape hatch above is
149149
# the way to actually skip the guard.
150150
#
151+
# OS_GOVERNED_ENQUEUE_SIBLING_ROOT=<dir> is the third, and it moves WHERE a
152+
# sibling checkout is looked for — never WHAT is accepted as one. The admission
153+
# rule stays the origin-slug comparison further down: a directory becomes the
154+
# tree this guard audits only when its own `origin` declares the target
155+
# `owner/repo`, so a value pointed anywhere wrong resolves NOTHING. It cannot
156+
# widen the audit, and it cannot soften one either — a sibling that does resolve
157+
# is judged exactly as it is today.
158+
#
159+
# unset → `$(dirname "$repo_root")`, the parent of this checkout. Today's
160+
# behaviour, to the byte.
161+
# empty → the same as unset. An empty value is an accident (`export VAR=`,
162+
# or `VAR="$SOMETHING_UNSET"`), and the safe reading of an accident
163+
# is "no override" — never "look nowhere", which would silently
164+
# drop a real audit.
165+
# a directory carrying no matching checkout, one that does not exist included
166+
# → nothing resolves, and the run takes the existing "no checkout of
167+
# the target repo is available" fail-open below, with its existing
168+
# warning. That is the branch a box WITHOUT the sibling has always
169+
# taken; this variable opens no new way out.
170+
#
171+
# Why it exists: the self-test's cross-repo case asserts that fail-open, and its
172+
# premise used to be a fact about the BOX ("objectstack-ai/cloud has no sibling
173+
# checkout here") rather than about the hook. On a container that does carry a
174+
# sibling `cloud` checkout the guard resolved it, recomputed the predicate on it
175+
# and blocked — 54 passed / 1 failed, green in CI only because CI carries no
176+
# sibling. The matrix now sets this variable and owns its own premise, and the
177+
# resolved-sibling BLOCK is pinned beside it as the deliberate behaviour it is.
178+
#
151179
# Self-test (no network, no build): .claude/hooks/guard-governed-enqueue.selftest.sh
152180

153181
set -uo pipefail
@@ -447,11 +475,20 @@ done < "$work/files.txt"
447475

448476
slug_of() { git -C "$1" remote get-url origin 2>/dev/null | sed -n 's#.*github\.com[:/]\([A-Za-z0-9._-]*/[A-Za-z0-9._-]*\)\(\.git\)\{0,1\}/*$#\1#p'; }
449477

478+
# WHERE a sibling is looked for is injectable (OS_GOVERNED_ENQUEUE_SIBLING_ROOT,
479+
# header); WHAT is accepted as one is not. The slug comparison below is the whole
480+
# admission rule and is untouched by it, so the variable can only move the
481+
# search — a sibling that resolves is audited exactly as before, and a root
482+
# holding no matching checkout resolves nothing and falls through to the
483+
# "no checkout available" fail-open, the branch a box without the sibling
484+
# already takes. Unset or empty ⇒ the parent of this checkout, as always.
485+
sibling_root="${OS_GOVERNED_ENQUEUE_SIBLING_ROOT:-$(dirname "$repo_root")}"
486+
450487
target_root=""
451488
if [ "$(slug_of "$repo_root")" = "$owner/$repo" ]; then
452489
target_root="$repo_root"
453-
elif [ "$(slug_of "$(dirname "$repo_root")/$repo")" = "$owner/$repo" ]; then
454-
target_root="$(dirname "$repo_root")/$repo"
490+
elif [ "$(slug_of "$sibling_root/$repo")" = "$owner/$repo" ]; then
491+
target_root="$sibling_root/$repo"
455492
fi
456493

457494
test_args=(--test --json)

0 commit comments

Comments
 (0)