diff --git a/AGENTS.md b/AGENTS.md index 4d0823a..e585554 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,12 +83,20 @@ reader can judge whether it still holds. 2. **Loose in the firing direction.** On uncertainty, fire. A missed commit (false ✓) is the dangerous direction; a redundant warning is the accepted price. 3. **Gate-B validity is content-derived, never event-derived.** Invalidation compares a - hash of the working tree — `git diff HEAD` plus a tree id written from a throwaway - index, so untracked paths, contents and file modes all count, minus `.context/`. An + fingerprint of the effective index plus the included worktree content, as of the + hook's invocation — a deliberate superset of any one commit's payload, so the gate + errs toward firing: `git diff HEAD` for tracked content, a tree id written from the + **effective index** (`GIT_INDEX_FILE` when set, else the git-dir index), and a tree + id written from a throwaway index brought up to the worktree — so untracked paths, + contents and file modes all count, minus `.context/`. + The index component exists because `git commit` commits the index: without it, staging + a change and reverting the file on disk read as unchanged and reported satisfied. An event-derived check misses a file changed through Bash (`sed -i`, `git apply`, codegen) and leaves a stale ✓ standing; so does a name-only view of untracked files, or any hand-rolled walk that re-derives what `git write-tree` already gets right - (symlink targets, exotic path encodings, non-regular files). + (symlink targets, exotic path encodings, non-regular files). When the fingerprint + cannot be computed it is the literal `unavailable`, which never matches — including + against itself. 4. **POSIX `sh`, and `jq` is optional.** No bash-isms; correct behaviour via fallback parsing when `jq` is absent. The hook runs on machines whose environment we do not control, and CI invokes it with `sh`. Enforced mechanically by the lint command @@ -188,6 +196,21 @@ reader can judge whether it still holds. matches that word order, i.e. "convention-loaded" / "convention loading"; a sentence reading "loaded by convention" is caught by the `declare[sd]?` arm only when it also contains a form of *declare*, so read the hits rather than trusting the count.) +- **Never describe what a gate proves without checking what it actually compares.** + Prose that overstates a mechanism is this repo's most persistent defect, and it + regenerates: fixing the index-tree story took four Gate-B rounds because *each + correction introduced a subtler version of the same claim* — "the tree-hash proves + what was reviewed", then "what is being committed is what was reviewed", then "what a + commit would actually carry", then "everything a commit could carry". Every round + searched for the previous **phrase**, so a synonym survived. Search for the **claim**: + `grep -rniE '(everything|anything|all content|any change)[^.]{0,80}\b(commit|fingerprint|hash)\b|\b(commit|fingerprint|hash)\b[^.]{0,80}(everything|anything|all content|any change)' --include='*.md' --include='*.sh' . | grep -vE 'source-files/|docs/superpowers/'` + (The `\b` boundaries matter: without them `commit` matches `committed` and the + recipe reports its own false positives.) Two things this does NOT do, stated so + nobody mistakes it for a guard: nothing runs it + in CI — it is a recipe a human runs — and an overclaim phrased without those totality + words escapes it entirely. It raises the floor; it does not close the class. The + underlying rule is the check itself: for every sentence about a gate, name the exact + comparison the code performs, and delete any part of the sentence that outruns it. - **Never rename or delete a doc section without grepping for references first.** `ci.yml` once pointed at a deleted README section; `MANIFEST.md` listed a `CLAUDE.md` that did not exist. Docs-drift is this plugin's own taxonomy class and this repo is diff --git a/README.md b/README.md index a61ac3c..4796dac 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ Why each of these, and how to adapt them: [`docs/coding-workflow.md`](docs/codin | `/dev-workflow:process-pr-review` | command — validates PR bot comments against the code and your invariants, replies to each, fixes regressions, tracks pre-existing issues. | | `dev-workflow:finding-triage` | agent — read-only, fresh context, judges whether one PR-bot claim is actually true of the code. Used by the PR processor; never counts as a review gate. | | `/dev-workflow:workflow-init` | command — scaffolds the per-project files, then interviews you to write `AGENTS.md`. | -| codex-gate hook | non-blocking reminders that count Gate A and Gate B passes, and verify a Gate-B review against the actual content of the working tree. Always exits 0. | +| codex-gate hook | non-blocking reminders that count Gate A and Gate B passes, and verify a Gate-B review against a fingerprint of the effective index plus the included worktree content, as of the hook's invocation — a deliberate superset of any one commit's payload, so the gate errs toward firing. Always exits 0. | ## Setup diff --git a/docs/architecture.md b/docs/architecture.md index 00ab9d9..6757337 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -61,15 +61,25 @@ It doesn't, because that is blind to a file changed through Bash — `sed -i`, `eslint --fix`, `git apply`, a codegen step — which emits no such event and would leave a stale "reviewed" marker standing. -Instead the hook stores a hash of the working tree at review time and recomputes it at -commit: `git diff HEAD` for tracked content, plus a tree id written from a throwaway -index so untracked files count by path, content and mode. (Asking git for the tree +Instead the hook stores a fingerprint of the index and the working tree at review time +and recomputes it at commit: `git diff HEAD` for tracked content, a tree id for the +effective index, and a tree id written from a throwaway index brought up to the +worktree so untracked files count by path, content and mode. (Asking git for the tree rather than walking the files in shell is deliberate — a hand-rolled walk has to re-derive symlink targets, git's path quoting and non-regular files, and got all three -wrong before this was reduced to `git write-tree`.) Any change by any tool -invalidates; an edit-then-undo correctly stays valid, because what is being committed -*is* what was reviewed. A false ✓ is the dangerous direction, so the check is tied to -what is actually on disk rather than to what the harness happened to notice. +wrong before this was reduced to `git write-tree`.) Any change to *included* content, +made by any tool and present when the hook runs, invalidates — `.context/` and +untracked ignored paths are excluded by design (a *tracked* file still counts even if it +matches `.gitignore`). An unstaged edit-then-undo still matches, because it restores +the fingerprint — which says the content is unchanged since the review, not that Codex +read it: the hook compares a fingerprint of disk while the reviewer reads a git range +(the hook's own hard-floor comment, above `floor=3`, says the same). A false ✓ is the +dangerous direction, +so the check is tied to the effective index plus the included worktree content as of +the hook's invocation — a superset of any one commit's payload, chosen so the gate errs +toward firing — rather than to what the harness happened to notice. (A mutation after +that invocation, such as the compound `printf x > f && git commit -am y`, is still +unseen — a separate, parked defect, not this one.) Two consequences worth knowing: diff --git a/docs/getting-started.md b/docs/getting-started.md index 27e24f7..5282bf2 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -42,9 +42,11 @@ skipping locally only postpones the red. **7. Gate B on the diff.** Claude makes a `WIP:`-prefixed commit (gives Codex a range to read; the hook knows WIP doesn't end the cycle), then loops `mcp__codex__review` the same way: three passes, final clean. Verification is by -**content** — any file change after the last review, even from a formatter, flips -it back to unsatisfied (staging alone doesn't; `git add` changes no content). On -`✓ Codex Gate B satisfied (3/3 cycle, 3 on current code)`, the real commit replaces +**content** — any change to included content present when the hook runs, even from a +formatter, flips it back to unsatisfied; `.context/` and untracked ignored paths are +excluded, and staging counts, because the fingerprint covers the index and that is what +a commit carries. On +`✓ Codex Gate B satisfied (3/3 cycle, 3 on current fingerprint)`, the real commit replaces the WIP via `git commit --amend`. **8. PR and bots.** Open the PR as usual; once the bots have commented, run diff --git a/docs/hardening-log.md b/docs/hardening-log.md index 8ae2542..d478033 100644 --- a/docs/hardening-log.md +++ b/docs/hardening-log.md @@ -20,3 +20,6 @@ escape `\|`, one line), `source` (gate-a|gate-b|bot|manual), | 2026-07-18 | unverified-enforcement-claim | six citable claims across one spec, its plan and its diff that something was enforced/caught/guaranteed where no mechanism did it — each caught by a gate, none by the author, one written into the same document that records the pattern | gate-a | major | P std | docs/prompt-standards.md item 11 (+ the same item in the workflow-init inline template). Mixed provenance: four from Gate A on the spec, one from Gate A on the plan, one from Gate B on the pre-merge diff; `source` records Gate A as the majority and the trigger | | 2026-07-18 | docs-drift | a Gate-B fix reordered a precedence rule and added a terminal state in process-pr-review; the approved spec was never updated and disagreed until a PR bot found it | bot | major | P std | CLAUDE.md §5 Gate B, "a fix that changes specified behaviour updates the spec in the same commit" (+ the workflow-init inline template). Escalated from the 2026-07-18 `1 prose` row, whose AGENTS.md rule was scoped to manifest claims and could not reach spec-vs-implementation drift | | 2026-07-19 | artifact-version-not-bumped | plugin changes merged without a version bump, so installed copies silently stayed stale — a machine ran 0.1.0 while main was at 0.4.0, the 0.4.0 bump had to be requested during PR #4, and main still carried two un-released plugin commits when this was written | manual | major | 2 lint | scripts/check-version-bump.sh + .test.sh, wired PR-only in ci.yml; AGENTS.md invariant 12. It verifies that a bump is PRESENT, not that it is right. It does not check: semantic correctness (a patch where a minor was due passes); direction (any different string passes, including a decrease); anything outside a pull_request event (a direct push to main bypasses it entirely); deletion of an entire plugin directory; any change to which directory the marketplace entry points at (rename, copy, or `source` repoint); two PRs branched from the same version each bumping to the same new one; and whether the version was ever released or tagged | +| 2026-07-19 | false-negative-gate | both Gate-B hash components described the worktree while `git commit` commits the index, so staging a change and reverting the file on disk reported satisfied on unreviewed content | gate-b | blocker | 4 test | plugins/dev-workflow/hooks/codex-gate.test.sh sections 24-30 + AGENTS.md invariant 3 (amended to name all three components). What the tests DO cover: the index component exists and moves on divergence, the failure marker never self-matches, and the ambient-alternate-index and three-way-`.context` states. What they do NOT: seven of `tree_hash`'s failure seams have no targeted test — `mktemp -d`, the non-symbolic unresolvable-HEAD branch, the throwaway-index `git rm --cached`, each `write-tree` separately, `git add -A`, and the buffered-stream redirect (enumerated in todos.md by derivation after three attempts understated it from memory); every divergence shape (they pin the mechanism, not exhaustive enumeration); mutation after the PreToolUse event, which is the parked compound-command row; command-local retargeting (`GIT_INDEX_FILE=`/`-C`/`--git-dir`), split to the 2026-07-19 command-retargeting-guard story; and a `git add`/`write-tree` failure inside the throwaway index, which stays parked | +| 2026-07-19 | unverified-enforcement-claim | across four Gate-B rounds on the index-tree documentation task, prose repeatedly claimed more than the hook guarantees, and each fix introduced a subtler version of the same overclaim — "the tree-hash proves what was reviewed", then "what is being committed is what was reviewed", then "what a commit would actually carry", then "everything a commit could carry"; every round searched for the previous PHRASE rather than the CLAIM, so a synonym survived each time | gate-b | major | 1 prose | AGENTS.md Don'ts, "Never describe what a gate proves without checking what it actually compares" — a rule plus a by-meaning grep recipe, sibling to the existing manifest-claims rule. Deliberately NOT another banned phrase: adding one phrase per round is the same rung a fifth time. What it does NOT do, in the rule text as well as here: nothing runs it in CI (a human runs it), and an overclaim phrased without those totality words escapes it. It raises the floor; it does not close the class | +| 2026-07-20 | verification-masks-failure | the plan's mutation-verification steps read `sh …codex-gate.test.sh 2>&1 \| grep -cE '^FAIL'`, which reports the GREP's status, not the runner's — a suite that died part-way, or exited non-zero without printing `FAIL`, would read as 0 and be recorded as green. Found by CodeRabbit on PR #8, in the very steps used to prove this branch's guards were load-bearing | bot | major | 1 prose | plan Task 1 Step 3 + the two later mutation steps now capture `exit=$?` separately into a variable and RETURN success only when the status and the FAIL count are both zero (per-run `mktemp` file, not a shared path), with the reason stated inline. An earlier revision of this row said they "assert both" when they only printed both and still returned grep's status — caught by Gate B, in the row recording this very class. Scope of the fix, plainly: it corrects the three sites in this plan and the rationale a future plan author reads — nothing checks new plans for the same shape, so a plan written tomorrow can reintroduce it | diff --git a/docs/superpowers/plans/2026-07-19-gate-b-index-tree.md b/docs/superpowers/plans/2026-07-19-gate-b-index-tree.md new file mode 100644 index 0000000..941938e --- /dev/null +++ b/docs/superpowers/plans/2026-07-19-gate-b-index-tree.md @@ -0,0 +1,1135 @@ +# Gate-B Index-Tree Hash Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the Gate-B hook hash what a commit will actually carry — the index — so a +staged change with a reverted worktree can no longer report "Gate B satisfied". + +**Architecture:** `tree_hash()` in `plugins/dev-workflow/hooks/codex-gate.sh` gains a +third component: a tree id written from the *effective* index, taken before `git add -A` +brings the throwaway index up to the worktree. The function is also restructured so its +component stream is buffered to a file and checksummed only when every producer +succeeded; on any failure it returns the literal `unavailable`, which the comparison sites +treat as never-equal. Consumers of the value (the stale branch, the empty-state branch, +the fresh-count comparison) are updated in step. + +**Tech Stack:** POSIX `sh`, `git` plumbing (`write-tree`, `rev-parse`, `symbolic-ref`), +`shellcheck` 0.11.0, the repo's own `codex-gate.test.sh` harness (bare `pass`/`fail` +helpers, no framework). + +**Spec:** `docs/superpowers/specs/2026-07-19-gate-b-index-tree-design.md` (Gate A clean, +pass 15). + +## Global Constraints + +- **POSIX `sh` only.** No bash-isms. CI invokes the hook with `sh`. (AGENTS.md invariant 4) +- **The hook always exits 0.** No change may introduce a non-zero exit. (invariant 1) +- **`shellcheck --shell=sh` with NO exclusions** for `codex-gate.sh`. The test file keeps + its single `--exclude=SC2015`. A `[ $? -eq 0 ]` construct is SC2181 and will fail. +- **Loose in the firing direction.** On uncertainty, fire. A false ✓ is the forbidden + direction; a redundant warning is the accepted price. (invariant 2) +- **Prompt changes pass `docs/prompt-standards.md`** — all 12 items. Every reminder string + in this plan is a shipped prompt. (invariant 11) +- **A plugin change requires a version bump.** Any edit under `plugins/` must bump + `plugins/dev-workflow/.claude-plugin/plugin.json` `version` in the same PR, with a + `CHANGELOG.md` entry. (invariant 12) — Task 5. +- **No `Co-Authored-By: Claude` / `Generated with` trailers** on any commit. +- **Quality battery** (must be green before the final commit): + `shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && sh plugins/dev-workflow/hooks/codex-gate.test.sh` + +--- + +### Task 1: Failure contract — buffered stream, `unavailable` marker, and its consumers + +The marker and the guards that read it are one contract and ship together: a marker with +unguarded consumers would let two failing invocations compare equal, which is the exact +false-✓ this story exists to close. + +**Files:** +- Modify: `plugins/dev-workflow/hooks/codex-gate.sh:194-234` (the whole `tree_hash()`) +- Modify: `plugins/dev-workflow/hooks/codex-gate.sh:318-324` (PostToolUse store path) +- Modify: `plugins/dev-workflow/hooks/codex-gate.sh:397` (the stale-branch condition) +- Test: `plugins/dev-workflow/hooks/codex-gate.test.sh` (append a new section) + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `tree_hash()` prints either a checksum token (`[0-9a-f]+` or `cksum` digits) or + the literal string `unavailable`, one line, always exit 0. Task 2 adds a component + inside it; Task 3 rewrites the messages in the branches guarded here. + +- [ ] **Step 1: Write the failing tests** + +Append to `plugins/dev-workflow/hooks/codex-gate.test.sh`, before the final summary block: + +```sh +# 24. Failure contract: an uncomputable hash must never satisfy, and repeated failures +# must never match each other. Spec §3 "The nonce goes away". +# Each fault spans BOTH the stored and the recomputed fingerprint — with the fault +# applied only at commit time, a mismatch proves nothing about the handling. +stub_dir=$(mktemp -d) +reset_all +printf '1' > "$floorf" # floor is checked LAST; without this a faulted run + # reports "below floor" and the test passes vacuously + +# 24a. every checksum tool fails silently (exit 0, no output) +for t in shasum sha1sum cksum; do + printf '#!/bin/sh\nexit 0\n' > "$stub_dir/$t"; chmod +x "$stub_dir/$t" +done +PATH="$stub_dir:$PATH" rev +out=$(PATH="$stub_dir:$PATH" commitpre) +printf '%s' "$out" | grep -q 'not satisfied' && pass "silent checksum -> not satisfied" || fail "silent checksum -> not satisfied" +# Absence is asserted by inverting the RESULT, not with `grep -v` — `grep -qv` means +# "some line lacks the pattern", which is a different question and was observed to +# return 1 regardless on the dev machine. +printf '%s' "$out" | grep -qF 'no mcp__codex__review has run' \ + && fail "silent checksum -> not the never-run branch" \ + || pass "silent checksum -> not the never-run branch" + +# 24b. a checksum that PRINTS a plausible token and then fails +for t in shasum sha1sum cksum; do + printf '#!/bin/sh\nprintf "deadbeef -\\n"\nexit 1\n' > "$stub_dir/$t"; chmod +x "$stub_dir/$t" +done +reset_all; printf '1' > "$floorf" +PATH="$stub_dir:$PATH" rev +out=$(PATH="$stub_dir:$PATH" commitpre) +printf '%s' "$out" | grep -q 'not satisfied' && pass "checksum prints then fails -> not satisfied" || fail "checksum prints then fails -> not satisfied" +[ "$(cat "$fresh" 2>/dev/null || echo 0)" = 0 ] && pass "unhashable pass leaves freshCount 0" || fail "unhashable pass leaves freshCount 0" + +# 24c. seed-copy failure (stub cp) must fire, not silently hash an empty index +printf '#!/bin/sh\nexit 1\n' > "$stub_dir/cp"; chmod +x "$stub_dir/cp" +rm -f "$stub_dir/shasum" "$stub_dir/sha1sum" "$stub_dir/cksum" +reset_all; printf '1' > "$floorf" +PATH="$stub_dir:$PATH" rev +printf '%s' "$(PATH="$stub_dir:$PATH" commitpre)" | grep -q 'not satisfied' \ + && pass "seed-copy failure -> not satisfied" || fail "seed-copy failure -> not satisfied" +rm -f "$stub_dir/cp" + +# 24d. a selective git wrapper that fails ONLY `diff` +cat > "$stub_dir/git" <<'STUB' +#!/bin/sh +for a in "$@"; do case "$a" in diff) exit 1 ;; esac; done +exec "$REAL_GIT" "$@" +STUB +chmod +x "$stub_dir/git" +REAL_GIT=$(command -v git); export REAL_GIT +reset_all; printf '1' > "$floorf" +PATH="$stub_dir:$PATH" rev +printf '%s' "$(PATH="$stub_dir:$PATH" commitpre)" | grep -q 'not satisfied' \ + && pass "git diff failure -> not satisfied" || fail "git diff failure -> not satisfied" + +# 24e. a selective git wrapper that fails ONLY `rev-parse --absolute-git-dir` +cat > "$stub_dir/git" <<'STUB' +#!/bin/sh +case "$*" in *"--absolute-git-dir"*) exit 1 ;; esac +exec "$REAL_GIT" "$@" +STUB +chmod +x "$stub_dir/git" +reset_all; printf '1' > "$floorf" +PATH="$stub_dir:$PATH" rev +printf '%s' "$(PATH="$stub_dir:$PATH" commitpre)" | grep -q 'not satisfied' \ + && pass "unresolvable git-dir -> not satisfied" || fail "unresolvable git-dir -> not satisfied" +rm -f "$stub_dir/git" +rm -f "$floorf" +reset_all; rev; rev; rev + +# 25. Unborn repo: no commits and no .git/index must still hash and self-match, or the +# first commit in a fresh repo STOPs forever. Spec §3 "But an absent index is not a +# failed copy". +unborn=$(mktemp -d) +( + cd "$unborn" || exit 1 + git init -q; git config user.email t@t; git config user.name t + mkdir -p .context; : > .context/codex-gate.on + printf '1' > .context/codex-gate.floor # one pass is enough for this fixture + printf 'x\n' > a.ts + R='{"hook_event_name":"PostToolUse","tool_name":"mcp__codex__review","tool_input":{}}' + printf '%s' "$R" | sh "$HOOK" >/dev/null + h1=$(cat .context/codex-gate.gateB 2>/dev/null) + printf '%s' "$R" | sh "$HOOK" >/dev/null + h2=$(cat .context/codex-gate.gateB 2>/dev/null) + [ -n "$h1" ] && [ "$h1" != unavailable ] && [ "$h1" = "$h2" ] || exit 1 + # ...and the FIRST commit must actually be able to reach satisfied. Hashing and + # self-matching is not enough: a consumer-side regression could still STOP every + # first commit forever, which is the failure this fixture exists to catch. + out=$(printf '%s' '{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"git commit --allow-empty -m init"}}' | sh "$HOOK") + printf '%s' "$out" | grep -q 'Gate B satisfied' || exit 1 +) && pass "unborn repo hashes, self-matches, and can reach satisfied" \ + || fail "unborn repo hashes, self-matches, and can reach satisfied" +rm -rf "$unborn" +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `sh plugins/dev-workflow/hooks/codex-gate.test.sh 2>&1 | grep -E '^FAIL'` +Expected: FAIL lines for `silent checksum -> not satisfied`, `checksum prints then fails +-> not satisfied`, `unhashable pass leaves freshCount 0`, `seed-copy failure -> not +satisfied`, `git diff failure -> not satisfied`, `unresolvable git-dir -> not satisfied`. +(`unborn repo hashes and self-matches` may already pass — the current code returns a +nonce-free empty-ish hash there. That is fine; it is a regression guard for Task 2.) + +- [ ] **Step 3: Replace `tree_hash()` with the buffered form** + +Replace `plugins/dev-workflow/hooks/codex-gate.sh:194-234` (the entire existing +`tree_hash() { … }`) with: + +```sh +tree_hash() { + ok=1 + # Same fallback order as before; no checksum tool at all is itself a failure. + sum_cmd=$(command -v shasum || command -v sha1sum || command -v cksum) || ok=0 + # mktemp -d, not a predictable "$TMPDIR/name.$$": on a shared /tmp a predictable name + # is a symlink target an attacker can plant. + tmp_dir=$(mktemp -d 2>/dev/null) || { tmp_dir=''; ok=0; } + if [ -n "$tmp_dir" ]; then + tmp_index="$tmp_dir/index" + # The component stream is BUFFERED and checksummed only on full success (below). + # Printing a failure marker in-stream would checksum the marker together with the + # partial output, so no consumer would ever see the literal `unavailable` and two + # failing runs could produce equal hashes — the false-✓ direction. + { + # (0) tracked content. `git diff HEAD` is staging-independent, so it sees staged + # and unstaged edits alike. An unborn branch is identified POSITIVELY: a bare + # "--verify failed" also covers a corrupt or unreadable HEAD, and emitting the + # constant `no-head` for those would self-match. + if git -C "$repo_root" rev-parse --verify -q HEAD >/dev/null 2>&1; then + git -C "$repo_root" diff HEAD -- . ':(exclude).context' 2>/dev/null || ok=0 + elif git -C "$repo_root" symbolic-ref -q HEAD >/dev/null 2>&1; then + printf 'no-head\n' + else + ok=0 + fi + + # (1) WORKTREE tree, from a throwaway index. Task 2 adds the INDEX tree here. + # git_dir must RESOLVE: unchecked, a failure yields "/index", which does not + # exist, so the absent-index carve-out below would read it as "nothing staged" + # and hand back the empty tree — a constant that matches itself. + # The chain's status is consumed by `if` directly; a trailing `[ $? -eq 0 ]` is + # SC2181 and the hook is linted with no exclusions. + if git_dir=$(git -C "$repo_root" rev-parse --absolute-git-dir 2>/dev/null) && + [ -n "$git_dir" ] && + { [ ! -e "$git_dir/index" ] || cp "$git_dir/index" "$tmp_index" 2>/dev/null; } && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" add -A \ + -- . ':(exclude).context' >/dev/null 2>&1 && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" write-tree 2>/dev/null + then :; else ok=0; fi + } > "$tmp_dir/stream" 2>/dev/null || ok=0 + fi + + h='' + if [ "$ok" -eq 1 ]; then + # The checksum's OWN status must be seen: `cmd < file | awk` reports awk's status, + # so a checksum failing after emitting a partial line would be stored as a real + # fingerprint. Parse with a shell expansion — `${raw%% *}` has no exit status to mask. + if raw=$("$sum_cmd" < "$tmp_dir/stream" 2>/dev/null); then + h=${raw%% *} + fi + fi + if [ -n "${tmp_dir:-}" ]; then rm -rf "$tmp_dir" 2>/dev/null; fi + # A checksum that runs but emits nothing is a failure too, not an empty tree. The + # marker is a CONSTANT, not a nonce: `date +%s`+`$$` can repeat under PID reuse inside + # one second, and two colliding failures would compare equal and report satisfied. + # Never-matching is enforced at the comparison sites instead. + if [ -n "$h" ]; then printf '%s\n' "$h"; else printf 'unavailable\n'; fi +} +``` + +- [ ] **Step 4: Guard the fresh-count comparison** + +Find the fresh-count comparison (currently line 324 — anchor on the text, not the +number, since Step 3 changed the length of `tree_hash()` above it). Replace: + +```sh + if [ "$h" = "$prev" ]; then bump_count "$fresh_file"; else { printf '%s' 1 > "$fresh_file"; } 2>/dev/null || true; fi +``` + +with: + +```sh + # An unhashable pass covers nothing, so it neither counts as "same tree as last + # pass" nor starts a fresh streak at 1 — two `unavailable` values are not a match. + if [ "$h" != unavailable ] && [ "$h" = "$prev" ]; then + bump_count "$fresh_file" + elif [ "$h" = unavailable ]; then + { printf '%s' 0 > "$fresh_file"; } 2>/dev/null || true + else + { printf '%s' 1 > "$fresh_file"; } 2>/dev/null || true + fi +``` + +- [ ] **Step 5: Guard the stale-branch comparison** + +At `plugins/dev-workflow/hooks/codex-gate.sh:397`, replace: + +```sh + elif [ "$reviewed" != "$current" ]; then +``` + +with: + +```sh + # `unavailable` on EITHER side is never a match: an uncomputable fingerprint + # must read as unverified, and two of them must not cancel out. + elif [ "$current" = unavailable ] || [ "$reviewed" = unavailable ] || + [ "$reviewed" != "$current" ]; then +``` + +- [ ] **Step 6: Run lint and the full suite** + +Run: `shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && sh plugins/dev-workflow/hooks/codex-gate.test.sh` +Expected: no shellcheck output; every test line `ok - …`; final summary reports 0 +failures. If SC2181 appears, the `[ $? -eq 0 ]` form crept back into Step 3. + +- [ ] **Step 7: Commit** + +```bash +git add plugins/dev-workflow/hooks/codex-gate.sh plugins/dev-workflow/hooks/codex-gate.test.sh +git commit -m 'fix(hook): make an uncomputable Gate-B fingerprint fail closed' \ + -m 'Replace the date+PID failure nonce with a constant `unavailable` marker and +enforce never-matching at the comparison sites, so two failing invocations +cannot compare equal and report satisfied. Buffer the component stream and +checksum it only on full success, so a partial stream is never hashed.' +# Single quotes, not double: a `backticked` word inside a double-quoted -m is +# command-substituted by the shell (verified — it runs the word and drops it). +``` + +--- + +### Task 2: The index-tree component + +**Files:** +- Modify: `plugins/dev-workflow/hooks/codex-gate.sh` (`tree_hash()`, as rewritten in Task 1) +- Modify: `plugins/dev-workflow/hooks/codex-gate.test.sh:180-194` (test 3f) +- Test: `plugins/dev-workflow/hooks/codex-gate.test.sh` (new section) + +**Interfaces:** +- Consumes: `tree_hash()` as Task 1 leaves it — the buffered form with a **single** + `write-tree` (the worktree tree) and no `eff_index`. Verified: that form reproduces the + current hook's hash byte-for-byte, so Task 1 changed the failure contract only. +- Produces: `tree_hash()` with three components. Step 4 adds `eff_index`, the forced + `.context` removal, and the pre-`add -A` index `write-tree` — all load-bearing, none of + them present after Task 1. + +- [ ] **Step 1: Rewrite test 3f to assert the decision** + +Replace `plugins/dev-workflow/hooks/codex-gate.test.sh:180-194` with: + +```sh +# 3f. DECIDED at spec §2 (2026-07-19-gate-b-index-tree-design.md): staging +# already-reviewed content DOES invalidate. The hash covers the index tree, and +# `git add` changes it. The bytes that would be committed are unchanged, so this is +# a false invalidation — accepted under invariant 2 ("loose in the firing +# direction"), and the STOP message explains that staging alone can cause it. +# This test previously asserted the OPPOSITE as though it were a principle; the +# behaviour was never decided, it fell out of an implementation choice. +reset_all +printf 'reviewed change\n' >> app.ts +rev; rev; rev # 3 passes covering the modified (unstaged) tree +printf '%s' "$(commitpre)" | grep -q 'Gate B satisfied' && pass "setup: satisfied on unstaged change" || fail "setup: satisfied on unstaged change" +git add app.ts >/dev/null 2>&1 # staging only — no content change +printf '%s' "$(commitpre)" | grep -q 'not satisfied' \ + && pass "staging a reviewed tracked file -> NOT satisfied (spec §2 decision)" \ + || fail "staging a reviewed tracked file -> NOT satisfied (spec §2 decision)" +# The old trailing assertion ("untracked file on a staged tree -> not satisfied") is +# GONE on purpose: once staging alone invalidates, it passes regardless of the untracked +# file and tests nothing. Test 3c already covers untracked content. +git reset -q >/dev/null 2>&1; git checkout -- app.ts >/dev/null 2>&1 +reset_all; rev; rev; rev +``` + +- [ ] **Step 2: Write the new failing tests** + +Append to `plugins/dev-workflow/hooks/codex-gate.test.sh`: + +```sh +# 26. THE DEFECT (spec §1): staged content diverging from the worktree. +# NOTE: the revert is a direct byte write, NOT `git checkout -- app.ts` — checkout +# restores the worktree FROM THE INDEX, which would install v2 on disk and destroy +# the divergence, making this test pass against the unfixed hook. +reset_all +rev; rev; rev +printf '%s' "$(commitpre)" | grep -q 'Gate B satisfied' && pass "setup: satisfied on clean tree" || fail "setup: satisfied on clean tree" +printf 'v2\n' > app.ts; git add app.ts >/dev/null 2>&1 # index: v2 +printf 'v1\n' > app.ts # worktree: back to HEAD bytes +printf '%s' "$(commitpre)" | grep -q 'not satisfied' \ + && pass "staged-vs-worktree divergence -> NOT satisfied" \ + || fail "staged-vs-worktree divergence -> NOT satisfied" +git reset -q >/dev/null 2>&1; git checkout -- app.ts >/dev/null 2>&1 + +# 27. Ambient alternate index. Three shapes: a negative-only test would be satisfied by +# an implementation that fires whenever GIT_INDEX_FILE is set — a permanent STOP. +alt_dir=$(mktemp -d) +# 27a. divergent: fingerprint recorded WITHOUT the alternate index, alternate enabled +# only for the commit check. +reset_all; printf '1' > "$floorf" +rev +cp .git/index "$alt_dir/alt" +printf 'SNEAKY\n' > app.ts; GIT_INDEX_FILE="$alt_dir/alt" git add app.ts >/dev/null 2>&1 +printf 'v1\n' > app.ts +printf '%s' "$(GIT_INDEX_FILE="$alt_dir/alt" commitpre)" | grep -q 'not satisfied' \ + && pass "ambient divergent alternate index -> NOT satisfied" \ + || fail "ambient divergent alternate index -> NOT satisfied" +# 27b. stable: same unchanged alternate index across review AND commit -> satisfied, +# with each index file byte-identical to its OWN pre-hook snapshot. +cp .git/index "$alt_dir/default.before"; cp "$alt_dir/alt" "$alt_dir/alt.before" +reset_all; printf '1' > "$floorf" +# Subshell: a prefix assignment on a SHELL FUNCTION call (rev/commitpre are functions, +# not external commands) persists in the shell after the call returns — unlike the same +# prefix on an external command. Without containment, GIT_INDEX_FILE leaks out of +# section 27 and corrupts every later section's fixtures (notably section 28). +( GIT_INDEX_FILE="$alt_dir/alt" rev ) +printf '%s' "$(GIT_INDEX_FILE="$alt_dir/alt" commitpre)" | grep -q 'Gate B satisfied' \ + && pass "ambient stable alternate index -> satisfied" \ + || fail "ambient stable alternate index -> satisfied" +cmp -s .git/index "$alt_dir/default.before" && pass "default index untouched" || fail "default index untouched" +cmp -s "$alt_dir/alt" "$alt_dir/alt.before" && pass "alternate index untouched" || fail "alternate index untouched" +# 27c. missing path: git treats a nonexistent GIT_INDEX_FILE as an EMPTY index, so the +# hook must hash and self-match rather than returning `unavailable`. +reset_all; printf '1' > "$floorf" +( GIT_INDEX_FILE="$alt_dir/does-not-exist" rev ) +h1=$(cat "$state" 2>/dev/null) +( GIT_INDEX_FILE="$alt_dir/does-not-exist" rev ) +h2=$(cat "$state" 2>/dev/null) +{ [ -n "$h1" ] && [ "$h1" != unavailable ] && [ "$h1" = "$h2" ]; } \ + && pass "missing alternate index hashes and self-matches" \ + || fail "missing alternate index hashes and self-matches" +rm -rf "$alt_dir"; rm -f "$floorf" +git checkout -- app.ts >/dev/null 2>&1; git reset -q >/dev/null 2>&1 +reset_all; rev; rev; rev + +# 27d. Guard: confirm section 27's containment held. If a future edit reintroduces the +# leak (e.g. drops a subshell above), this fails loudly instead of section 28 quietly +# going vacuous against a stale, since-deleted GIT_INDEX_FILE path. +[ -z "${GIT_INDEX_FILE+x}" ] && pass "GIT_INDEX_FILE not leaked out of section 27" || fail "GIT_INDEX_FILE not leaked out of section 27" + +# 27e. FINDING 1 — a RELATIVE ambient GIT_INDEX_FILE must resolve against the +# repository TOP-LEVEL, exactly as git itself does — not against the hook's own +# cwd. The shell-side `[ ! -e ]` test and `cp` are plain shell commands (unlike +# every git call in the hook, which uses `-C "$repo_root"`), so an unnormalized +# relative path resolves differently depending on where the hook happens to run +# from. Run BOTH the review pass and the commit check from a SUBDIRECTORY with a +# relative alt index that actually lives at the repo root: an unnormalized hook +# can't find it either time, takes the same absent-index carve-out both times, and +# the two constant empty-tree hashes MATCH — a false "satisfied" even though the +# alt index stages content the worktree does not have. +# The alt index file lives under `.context/` — excluded from the diff-HEAD and +# worktree-tree components by their own `:(exclude).context` pathspec — so it is +# never picked up as an untracked file by `add -A` itself; the only way it can +# affect the hash is through eff_index resolution, which is exactly what this +# test needs to isolate. +reset_all; printf '1' > "$floorf" +mkdir -p sub +cp .git/index "$work/.context/rel-idx" # a copy of the CURRENT (matching) index +( cd sub && GIT_INDEX_FILE=.context/rel-idx rev ) # review, from a subdir, relative alt index +printf 'SNEAKY\n' > app.ts +GIT_INDEX_FILE="$work/.context/rel-idx" git add app.ts >/dev/null 2>&1 # stage into the ALT index only +printf 'v1\n' > app.ts # worktree stays at the reviewed bytes +out=$(cd sub && GIT_INDEX_FILE=.context/rel-idx commitpre) +printf '%s' "$out" | grep -q 'not satisfied' \ + && pass "relative ambient GIT_INDEX_FILE from a subdirectory -> NOT satisfied (Finding 1)" \ + || fail "relative ambient GIT_INDEX_FILE from a subdirectory -> NOT satisfied (Finding 1)" +rm -f "$work/.context/rel-idx"; rm -rf sub +git checkout -- app.ts >/dev/null 2>&1; git reset -q >/dev/null 2>&1 +reset_all; rev; rev; rev + +# 28. Tracked .context/ diverging THREE ways (index differs from both HEAD and worktree) +# is exactly the state where `git rm --cached` refuses without -f, silently (stderr +# is redirected). The hash must still be computable and self-match, and the user's +# real index must be untouched — the forced removal runs on the throwaway index only. +git add -f .context >/dev/null 2>&1; git commit -qm "track .context" >/dev/null 2>&1 +printf 'staged\n' > .context/codex-gate.on; git add .context/codex-gate.on >/dev/null 2>&1 +printf 'worktree\n' > .context/codex-gate.on +cp .git/index "$work/index.before" +before_tree=$(git write-tree 2>/dev/null) +before_blob=$(git rev-parse :.context/codex-gate.on 2>/dev/null) +reset_all +rev; h1=$(cat "$state" 2>/dev/null) +rev; h2=$(cat "$state" 2>/dev/null) +{ [ -n "$h1" ] && [ "$h1" != unavailable ] && [ "$h1" = "$h2" ]; } \ + && pass "three-way .context divergence hashes and self-matches" \ + || fail "three-way .context divergence hashes and self-matches" +[ "$(git write-tree 2>/dev/null)" = "$before_tree" ] && pass "real index tree unchanged" || fail "real index tree unchanged" +[ "$(git rev-parse :.context/codex-gate.on 2>/dev/null)" = "$before_blob" ] && pass "staged .context blob unchanged" || fail "staged .context blob unchanged" +cmp -s .git/index "$work/index.before" && pass "real index bytes unchanged" || fail "real index bytes unchanged" +: > .context/codex-gate.on +git rm -rq --cached .context >/dev/null 2>&1; git commit -qm "untrack .context" >/dev/null 2>&1 +reset_all; rev; rev; rev +``` + +- [ ] **Step 3: Run the tests to verify they fail** + +> Capture the runner's exit status separately from the `FAIL` count. Piping straight +> into `grep -c` reports the *grep's* status, so a suite that dies part-way — or exits +> non-zero without printing `FAIL` — reads as `0` and looks green. Both numbers matter: +> `exit=0` **and** a count of `0` — so the command below *returns* success only when +> both hold, rather than printing them for you to eyeball. The output file is per-run +> (`mktemp`), not a fixed path: two concurrent runs sharing `/tmp/suite.out` could read +> each other's results. + +Run: `sh plugins/dev-workflow/hooks/codex-gate.test.sh 2>&1 | grep -E '^FAIL'` +Expected: FAIL for `staged-vs-worktree divergence -> NOT satisfied`, `staging a reviewed +tracked file -> NOT satisfied (spec §2 decision)`, and `ambient divergent alternate index +-> NOT satisfied`. Task 1 deliberately left the index component out, so these are a +genuine red — the hash still describes only the worktree. + +- [ ] **Step 4: Add the index-tree component** + +In `tree_hash()`, replace the producer chain written in Task 1 Step 3 with: + +```sh + # (1) INDEX tree and (2) WORKTREE tree, from one throwaway index. + # The index tree is taken BEFORE `add -A` brings the temp index up to the + # worktree, because `git commit` commits the index — that is the whole defect. + # `eff_index`, not `$git_dir/index`: git honours GIT_INDEX_FILE, and a missing + # alternate index is an EMPTY index to git, so the carve-out must follow the same + # path git will. + # A RELATIVE GIT_INDEX_FILE must then be normalized against $repo_root before the + # `[ ! -e ]` test and `cp` below: those are plain shell commands, resolved against + # the hook's OWN cwd — while every git call here uses `-C "$repo_root"`, and git + # itself resolves a relative GIT_INDEX_FILE against the repository TOP-LEVEL, not + # the caller's cwd. Left unnormalized, running the hook from a subdirectory with a + # relative ambient GIT_INDEX_FILE makes the shell half look in the wrong place, + # find nothing, and take the absent-index carve-out — hashing a CONSTANT empty tree + # while the real index has content (a false "satisfied", not a false STOP). + # $repo_root is already absolute (from `git rev-parse --show-toplevel`), so + # prefixing it is enough; an already-absolute eff_index (incl. the $git_dir/index + # default) is left as-is. + # `rm -rfq --cached`: without -f git refuses to remove a path whose staged content + # differs from both HEAD and the worktree — exactly the divergent state this story + # is about — and does so silently, since stderr is redirected. It runs against the + # THROWAWAY index; the user's real staging area is untouched. + if git_dir=$(git -C "$repo_root" rev-parse --absolute-git-dir 2>/dev/null) && + [ -n "$git_dir" ] && + eff_index=${GIT_INDEX_FILE:-$git_dir/index} && + case "$eff_index" in + /*) : ;; + *) eff_index="$repo_root/$eff_index" ;; + esac && + { [ ! -e "$eff_index" ] || cp "$eff_index" "$tmp_index" 2>/dev/null; } && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" rm -rfq --cached \ + --ignore-unmatch -- .context >/dev/null 2>&1 && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" write-tree 2>/dev/null && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" add -A \ + -- . ':(exclude).context' >/dev/null 2>&1 && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" write-tree 2>/dev/null + then :; else ok=0; fi +``` + +**Post-report addendum (Finding 1, Gate-B code review):** the two halves of the +carve-out resolved relative paths differently — the shell test/`cp` against the hook's +own cwd, every `git` call against `-C "$repo_root"` — so a relative ambient +`GIT_INDEX_FILE` run from a subdirectory silently took the carve-out and hashed a +constant empty tree, a false "satisfied". Fixed by normalizing `eff_index` against +`$repo_root` with a `case` (already-absolute paths, including the `$git_dir/index` +default, pass through unchanged). Covered by test 27e below. + +Run: `suite_out=$(mktemp) && sh plugins/dev-workflow/hooks/codex-gate.test.sh >"$suite_out" 2>&1; suite_status=$?; fail_count=$(grep -cE '^FAIL' "$suite_out" || :); rm -f "$suite_out"; printf 'exit=%s FAIL=%s\n' "$suite_status" "$fail_count"; [ "$suite_status" -eq 0 ] && [ "$fail_count" -eq 0 ]` +Expected: `exit=0 FAIL=0`, and the command itself exits 0 — it returns success only when both are zero. + +- [ ] **Step 5: Mutation-verify each guard** + +Each mutation must turn a specific test red. Apply, run, revert. + +1. Delete the first `write-tree` line (the index component) → test 26 must FAIL. +2. Change `-rfq` to `-rq` (drop the force) → test 28 must FAIL. +3. Change `eff_index=${GIT_INDEX_FILE:-$git_dir/index}` to `eff_index=$git_dir/index` → + test 27a must FAIL. + +Run after each: `suite_out=$(mktemp) && sh plugins/dev-workflow/hooks/codex-gate.test.sh >"$suite_out" 2>&1; suite_status=$?; fail_count=$(grep -cE '^FAIL' "$suite_out" || :); rm -f "$suite_out"; printf 'exit=%s FAIL=%s\n' "$suite_status" "$fail_count"; [ "$suite_status" -eq 0 ] && [ "$fail_count" -eq 0 ]` +Expected: before reverting, `FAIL` is at least 1 and the command exits non-zero; after reverting all three, `exit=0 FAIL=0` and it exits 0. + +- [ ] **Step 6: Run the full battery** + +Run: `shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && sh plugins/dev-workflow/hooks/codex-gate.test.sh` +Expected: clean shellcheck, 0 failures. + +- [ ] **Step 7: Commit** + +```bash +git add plugins/dev-workflow/hooks/codex-gate.sh plugins/dev-workflow/hooks/codex-gate.test.sh +git commit -m "fix(hook): hash the index tree, not just the worktree + +git commit commits the INDEX, but both hash components described the +worktree, so staging a change and reverting the file on disk reported Gate B +satisfied on unreviewed content. Add a tree id taken from the effective index +before the throwaway index is brought up to the worktree. + +Staging already-reviewed content now invalidates a review — a false +invalidation, accepted under invariant 2. Test 3f asserted the opposite as a +principle and is rewritten to assert the decision." +``` + +--- + +### Task 3: Messages — stale, empty-state, and satisfied branches + +**Files:** +- Modify: `plugins/dev-workflow/hooks/codex-gate.sh` — the three `emit` calls in the + commit-check `case`. **Anchor on their text, not on line numbers:** Task 1 replaced + `tree_hash()` with a longer function, so every line below it has shifted. Find them + with `grep -n 'STOP — Codex Gate B not satisfied\|Codex Gate B: \$passes' plugins/dev-workflow/hooks/codex-gate.sh`. +- Modify: `plugins/dev-workflow/hooks/codex-gate.test.sh:63` (asserts the old wording) +- Test: `plugins/dev-workflow/hooks/codex-gate.test.sh` (new section) + +**Interfaces:** +- Consumes: the branch conditions from Task 1 Step 5. +- Produces: nothing consumed by later tasks. + +- [ ] **Step 1: Update the existing assertion that this change breaks** + +At `plugins/dev-workflow/hooks/codex-gate.test.sh:63`, replace: + +```sh +printf '%s' "$out" | grep -q 'tree has CHANGED' && pass "Edit-tool change -> reported as stale tree" || fail "Edit-tool change -> reported as stale tree" +``` + +with: + +```sh +printf '%s' "$out" | grep -q 'cannot confirm' && pass "Edit-tool change -> reported as unconfirmed" || fail "Edit-tool change -> reported as unconfirmed" +``` + +- [ ] **Step 2: Write the failing message tests** + +Append to `plugins/dev-workflow/hooks/codex-gate.test.sh`: + +```sh +# 29. Message contracts (spec §4). These are shipped prompts — the product itself, not +# incidental output — so each branch's COMPLETE additionalContext and systemMessage +# is pinned as a golden fixture and compared EXACTLY, replacing per-clause greps: a +# clause list only catches a regression someone thought to enumerate (inserting +# "do not " before an asserted clause, or "Usually" -> "Always", stays green under +# it); an exact comparison catches any wording change. $passes, $floor and $fresh +# are pinned by the setup immediately before each assertion, so every fixture below +# is deterministic. +json_field() { # $1 = json text, $2 = key -> prints the string value. No jq required — + # same fallback idiom as the hook's own field(): safe here because none + # of the three golden messages below contain a literal backslash or quote. + printf '%s' "$1" | grep -o "\"$2\"[[:space:]]*:[[:space:]]*\"[^\"]*\"" | head -n1 | sed 's/^.*:[[:space:]]*"\(.*\)"$/\1/' +} + +# 29a. STALE branch: 1 recorded pass this cycle, default floor 3, no CLAUDE.md (so the +# generic policy phrase), a change made through the Edit tool. +# A `rev` baseline is required before the edit: without one, `reviewed` is empty and +# the edit below lands in the EMPTY-STATE branch, not the STALE branch this fixture +# describes (matches the existing pattern in section 3a). +reset_all; rev +printf 'edit\n' >> app.ts +run '{"hook_event_name":"PostToolUse","tool_name":"Edit","tool_input":{"file_path":"app.ts"}}' >/dev/null +out=$(commitpre) +ctx=$(json_field "$out" additionalContext) +msg=$(json_field "$out" systemMessage) +expected_ctx="STOP — Codex Gate B not satisfied: the hook cannot confirm that the content you are about to commit is the content mcp__codex__review last saw (1 recorded pass(es) this cycle). Usually that means the working tree or the index changed since the review. It can also mean you only staged already-reviewed content — the bytes are fine, but the hook cannot tell staging from editing; that this hook was upgraded and the recorded fingerprint uses the older format (see CHANGELOG); or that the fresh fingerprint could not be computed or could not be stored. Run Gate B (mcp__codex__review) now — one clean pass is the complete remedy for the staging and post-upgrade cases too. If a fresh pass leaves this unchanged with nothing edited in between, the fault is in the machinery rather than the code: check that .context/ is writable, that TMPDIR is writable, that a checksum tool (shasum, sha1sum or cksum) runs, that git status works, and that the disk is not full — then run one more pass to record a usable fingerprint. Per this project's review policy you MUST re-review after every fix." +expected_msg="⚠ Codex Gate B not satisfied (cannot confirm review)" +[ "$ctx" = "$expected_ctx" ] && pass "stale additionalContext matches exactly" || fail "stale additionalContext matches exactly" +[ "$msg" = "$expected_msg" ] && pass "stale systemMessage matches exactly" || fail "stale systemMessage matches exactly" +git checkout -- app.ts >/dev/null 2>&1 + +# 29b. SATISFIED branch: 3/3 passes this cycle, all 3 fresh (unchanged tree). The hook +# fingerprints disk; mcp__codex__review reads a git range (spec §7) — the exact +# fixture below is what pins that the message never claims Codex read the bytes. +reset_all; rev; rev; rev +out=$(commitpre) +ctx=$(json_field "$out" additionalContext) +msg=$(json_field "$out" systemMessage) +expected_ctx="Codex Gate B: 3/3 pass(es) this cycle, of which 3 cover the CURRENT content fingerprint (unchanged since that review). The floor counts the cycle; only the fresh pass(es) carry the same fingerprint as what you are committing. Per this project's review policy, commit only if your final pass was clean — no new Blocker/Major." +expected_msg="✓ Codex Gate B satisfied (3/3 cycle, 3 on current fingerprint)" +[ "$ctx" = "$expected_ctx" ] && pass "satisfied additionalContext matches exactly" || fail "satisfied additionalContext matches exactly" +[ "$msg" = "$expected_msg" ] && pass "satisfied systemMessage matches exactly" || fail "satisfied systemMessage matches exactly" + +# 29c. EMPTY-STATE branch: no fingerprint recorded this cycle, default floor 3. +reset_all +out=$(commitpre) +ctx=$(json_field "$out" additionalContext) +msg=$(json_field "$out" systemMessage) +expected_ctx="STOP — Codex Gate B not satisfied: no fingerprint is recorded for this cycle — either no mcp__codex__review has run, or the last one's fingerprint could not be written or read back. Per this project's review policy you MUST reach a minimum of 3 passes per cycle. Run Gate B (mcp__codex__review) now; if this repeats, check that .context/ and the state file inside it are readable and writable, and if the file exists but is unreadable or empty, delete it and run a fresh pass." +expected_msg="⚠ Codex Gate B: no recorded review" +[ "$ctx" = "$expected_ctx" ] && pass "empty-state additionalContext matches exactly" || fail "empty-state additionalContext matches exactly" +[ "$msg" = "$expected_msg" ] && pass "empty-state systemMessage matches exactly" || fail "empty-state systemMessage matches exactly" +reset_all; rev; rev; rev + +# 30. UNSTORABLE STATE (spec §5 test 11). Cause 5 lives OUTSIDE tree_hash: the store path +# writes with `2>/dev/null || true` because the hook must always exit 0, so a pass can +# hash perfectly and still leave the old fingerprint behind. Three shapes, because +# they reach the branch by different routes. +# Each shape depends on chmod actually DENYING access, which is false under an +# effective root uid — a root-run CI job would take the success path and then fail the +# assertion, reporting a product defect that is really a privilege artefact. Probe the +# exact operation each shape needs, restore what the probe changed, and skip loudly. +probe_denies() { # $1 = probe command; returns 0 when the operation was DENIED + ( eval "$1" ) >/dev/null 2>&1 && return 1 || return 0 +} +skip() { printf 'skip - %s\n' "$1"; } + +# 30a. replacement fails: the state file itself is read-only +reset_all; rev +before=$(cat "$state") +chmod 0444 "$state" 2>/dev/null +if probe_denies "printf x >> \"$state\""; then + printf 'later edit\n' >> app.ts + rev # hashes fine, but cannot replace the fingerprint + [ "$(cat "$state")" = "$before" ] && pass "unwritable state file keeps the old fingerprint" || fail "unwritable state file keeps the old fingerprint" + printf '%s' "$(commitpre)" | grep -q 'cannot confirm' \ + && pass "stale fingerprint after a failed store -> cannot confirm" \ + || fail "stale fingerprint after a failed store -> cannot confirm" +else + skip "unwritable state file (chmod does not deny writes here — running as root?)" +fi +chmod 0644 "$state" 2>/dev/null +git checkout -- app.ts >/dev/null 2>&1 + +# 30b. first write fails: no prior state, and the directory refuses a new file +reset_all +chmod 0555 .context 2>/dev/null +if probe_denies "touch .context/probe-$$"; then + rev # cannot create the state file at all + out=$(commitpre) + printf '%s' "$out" | grep -qF 'no fingerprint is recorded' \ + && pass "unwritable .context -> empty-state branch" || fail "unwritable .context -> empty-state branch" + printf '%s' "$out" | grep -qF 'could not be written or read back' \ + && pass "empty-state branch admits a failed first write" || fail "empty-state branch admits a failed first write" +else + skip "unwritable .context directory (chmod does not deny creation here — running as root?)" +fi +chmod 0755 .context 2>/dev/null; rm -f ".context/probe-$$" + +# 30c. read fails: the state file exists and is non-empty but cannot be read +reset_all; rev +chmod 0000 "$state" 2>/dev/null +if probe_denies "cat \"$state\""; then + out=$(commitpre) + printf '%s' "$out" | grep -qF 'no fingerprint is recorded' \ + && pass "unreadable state file -> empty-state branch" || fail "unreadable state file -> empty-state branch" + # invariant 1: advisory hook, always exit 0, even when its own state is unreadable. + # Checked with `if`, not `[ $? -eq 0 ]` — the latter is SC2181 and the test file + # excludes only SC2015, so it would fail lint. + if run '{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"git commit -m x"}}' >/dev/null 2>&1; then + pass "unreadable state file -> hook still exits 0" + else + fail "unreadable state file -> hook still exits 0" + fi +else + skip "unreadable state file (chmod does not deny reads here — running as root?)" +fi +chmod 0644 "$state" 2>/dev/null +reset_all; rev; rev; rev +``` + +- [ ] **Step 3: Run the tests to verify they fail** + +Run: `sh plugins/dev-workflow/hooks/codex-gate.test.sh 2>&1 | grep -E '^FAIL'` +Expected: FAIL for all six section-29 exact-match assertions (`stale additionalContext +matches exactly`, `stale systemMessage matches exactly`, `satisfied additionalContext +matches exactly`, `satisfied systemMessage matches exactly`, `empty-state +additionalContext matches exactly`, `empty-state systemMessage matches exactly`), plus +section 30's `cannot confirm` / `no fingerprint is recorded` / `could not be written or +read back` assertions. Any `skip -` lines are acceptable only if you are running as +root; on a normal account all three shapes must execute. + +- [ ] **Step 4: Rewrite the three messages** + +Replace the empty-state `emit` call — the one under `if [ -z "$reviewed" ]; then`, whose +text begins "no mcp__codex__review has run this cycle" — together with the comment above +it, which asserts a cause that may be false: + +```sh + # `-z "$reviewed"` no longer means only "no pass this cycle": a pass whose + # state write failed, or a state file that cannot be read back, leaves it + # empty too. The message names the absent FINGERPRINT, not an absent review. + emit "STOP — Codex Gate B not satisfied: no fingerprint is recorded for this cycle — either no mcp__codex__review has run, or the last one's fingerprint could not be written or read back. Per $policy you MUST reach a minimum of $floor passes per cycle. Run Gate B (mcp__codex__review) now; if this repeats, check that .context/ and the state file inside it are readable and writable, and if the file exists but is unreadable or empty, delete it and run a fresh pass." "⚠ Codex Gate B: no recorded review" +``` + +Replace the stale `emit` call — the one whose text begins "the working tree has CHANGED + since the last mcp__codex__review" — with: + +```sh + # Content check, not event check: this fires for a change made through ANY + # tool — Edit/Write, or a Bash `sed -i` / `eslint --fix` / `git apply`. + # It names the STATE, never a cause: five states reach here and the hook + # cannot tell them apart (spec §4). Do not "improve" this into asserting + # that the tree changed — under a repeated computation failure nothing + # changed, and under a failed state write the content may be exactly what + # was reviewed. + emit "STOP — Codex Gate B not satisfied: the hook cannot confirm that the content you are about to commit is the content mcp__codex__review last saw ($passes recorded pass(es) this cycle). Usually that means the working tree or the index changed since the review. It can also mean you only staged already-reviewed content — the bytes are fine, but the hook cannot tell staging from editing; that this hook was upgraded and the recorded fingerprint uses the older format (see CHANGELOG); or that the fresh fingerprint could not be computed or could not be stored. Run Gate B (mcp__codex__review) now — one clean pass is the complete remedy for the staging and post-upgrade cases too. If a fresh pass leaves this unchanged with nothing edited in between, the fault is in the machinery rather than the code: check that .context/ is writable, that TMPDIR is writable, that a checksum tool (shasum, sha1sum or cksum) runs, that git status works, and that the disk is not full — then run one more pass to record a usable fingerprint. Per $policy you MUST re-review after every fix." "⚠ Codex Gate B not satisfied (cannot confirm review)" +``` + +Replace the satisfied `emit` call — the one whose text begins "Codex Gate B: $passes/$floor +pass(es) this cycle" — with: + +```sh + # Distinguish the two counts (Finding 9): the cycle total includes passes + # made BEFORE later edits, which no longer cover the code being committed. + # FINGERPRINT EQUALITY IS ALL THIS PROVES. The hook compares a hash of + # disk; mcp__codex__review reads a git range — so a match does NOT + # establish that Codex read these bytes (spec §7, and the review-range row + # in todos.md). The stronger phrasing was here and was removed; do not + # restore it as a clarity improvement. + emit "Codex Gate B: $passes/$floor pass(es) this cycle, of which $fresh cover the CURRENT content fingerprint (unchanged since that review). The floor counts the cycle; only the fresh pass(es) carry the same fingerprint as what you are committing. Per $policy, commit only if your final pass was clean — no new Blocker/Major." "✓ Codex Gate B satisfied ($passes/$floor cycle, $fresh on current fingerprint)" +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `suite_out=$(mktemp) && sh plugins/dev-workflow/hooks/codex-gate.test.sh >"$suite_out" 2>&1; suite_status=$?; fail_count=$(grep -cE '^FAIL' "$suite_out" || :); rm -f "$suite_out"; printf 'exit=%s FAIL=%s\n' "$suite_status" "$fail_count"; [ "$suite_status" -eq 0 ] && [ "$fail_count" -eq 0 ]` +Expected: `exit=0 FAIL=0`, and the command itself exits 0 — it returns success only when both are zero. + +- [ ] **Step 6: Check the prompts against the standards** + +Read `docs/prompt-standards.md` and verify all 12 items against the three new strings — +in particular item 10 (identical-symptom causes each get a distinguishing check and a fix) +and item 12 (all-caps reserved for hard rules: `STOP` and `MUST` qualify, `INDEX` does +not and must not reappear). + +- [ ] **Step 7: Commit** + +```bash +git add plugins/dev-workflow/hooks/codex-gate.sh plugins/dev-workflow/hooks/codex-gate.test.sh +git commit -m "fix(hook): stop the Gate-B messages asserting causes they cannot know + +The stale branch is reached by five distinct states, so it names the state +(cannot confirm) rather than claiming the tree changed. The empty-state branch +admits a fingerprint that failed to write or read. The satisfied branch drops +'actually reviewed what you are committing' for fingerprint equality: the hook +hashes disk, the reviewer reads a git range." +``` + +--- + +### Task 4: Documentation — every site that describes the old behaviour + +Found by grep, not memory. Docs-drift is this project's most frequent finding class and +this repo is its most frequent site. + +**Files:** +- Modify: `AGENTS.md` (invariant 3, ~line 85) +- Modify: `plugins/dev-workflow/hooks/codex-gate.sh:6-11` (header) and `:105` +- Modify: `plugins/dev-workflow/hooks/codex-gate.sh:150-193` (the hash comment block) +- Modify: `plugins/dev-workflow/hooks/codex-gate.test.sh:126` (the NOTE) +- Modify: `README.md:26` +- Modify: `docs/architecture.md:64-72` +- Modify: `docs/getting-started.md:46` +- Modify: `plugins/dev-workflow/commands/workflow-init.md:778` +- Modify: `docs/superpowers/stories/2026-07-18-gate-b-hash-staged-worktree-divergence-story.md` + +**Interfaces:** +- Consumes: the implemented behaviour from Tasks 1–3. +- Produces: nothing consumed by later tasks. + +- [ ] **Step 1: Re-run the grep that found these, to catch anything added since** + +Run: +```bash +EXCL='source-files/|docs/superpowers/|\.mcp/|\.remember/' +grep -rniE "working tree|worktree" --include='*.md' . | grep -vE "$EXCL" | grep -iE "hash|tree id|invalidat|staging|staged" +grep -rn "tree-hash proves\|actually reviewed\|staging alone" --include='*.md' --include='*.sh' . | grep -vE "$EXCL" +``` +These two greps are a **safety net for sites added since this plan was written**, not a +rediscovery of the full list. They do not return every site in this task, and they return +one that belongs elsewhere: + +- Expect `docs/architecture.md` and `docs/getting-started.md` from the first grep, and + `plugins/dev-workflow/hooks/codex-gate.sh` (line 105) from the second. +- Expect a `todos.md` hit. It is **Task 6-owned** — the parked row being removed there — + and must not be edited here. +- `README.md:26`, `plugins/dev-workflow/commands/workflow-init.md:778`, the hook header + (6-11), the hash comment block (150-193) and the test NOTE (126) do **not** match these + patterns. They are known sites, listed above, and are edited in Steps 3-4 by name. + +The exclusions matter: `docs/superpowers/` holds this plan and the approved spec, which +describe the change and must NOT be rewritten to match it; `.mcp/` and `.remember/` are +generated state. The story file under `docs/superpowers/stories/` is edited in Step 5 by +name. + +Any hit outside the exclusions that is neither listed above nor the known todos.md row is +a site added since this plan was written — add it to this task before proceeding. + +- [ ] **Step 2: Amend AGENTS.md invariant 3** + +Replace the invariant-3 body so it names all three components: + +```markdown +3. **Gate-B validity is content-derived, never event-derived.** Invalidation compares a + fingerprint of everything a commit could carry: `git diff HEAD` for tracked content, + a tree id written from the **effective index** (`GIT_INDEX_FILE` when set, else the + git-dir index), and a tree id written from a throwaway index brought up to the + worktree — so untracked paths, contents and file modes all count, minus `.context/`. + The index component exists because `git commit` commits the index: without it, staging + a change and reverting the file on disk read as unchanged and reported satisfied. An + event-derived check misses a file changed through Bash (`sed -i`, `git apply`, + codegen) and leaves a stale ✓ standing; so does a name-only view of untracked files, + or any hand-rolled walk that re-derives what `git write-tree` already gets right + (symlink targets, exotic path encodings, non-regular files). When the fingerprint + cannot be computed it is the literal `unavailable`, which never matches — including + against itself. +``` + +- [ ] **Step 3: Fix the hook's own prose** + +`codex-gate.sh:6-11` — replace "a hash of the working tree" with "a fingerprint of the +index and the working tree", and "compares what is actually on disk" with "compares what +a commit would actually carry". + +`codex-gate.sh:105` — replace "(unlike Gate B, where the tree-hash proves what was +reviewed)" with "(unlike Gate B, which at least compares a content fingerprint — though +that proves the content is unchanged since the review, not that Codex read it)". + +`codex-gate.sh:150-193` — the block above `tree_hash()`. Delete the `KNOWN GAP` paragraph +entirely (it is now fixed). Replace the "Tracked CONTENT comes from `git diff HEAD` … +does not change the hash" paragraph with: + +```sh +# Tracked CONTENT comes from `git diff HEAD`, which is staging-independent. The INDEX +# tree is hashed separately, so `git add` of an already-reviewed file DOES invalidate: +# decided at docs/superpowers/specs/2026-07-19-gate-b-index-tree-design.md §2. The +# committed bytes are unchanged in that case, so it is a false invalidation — accepted +# under invariant 2, and the STOP message says staging alone can cause it. +# +# The seed copy is correctness-critical, not just a speed optimisation: `write-tree` on +# an empty index SUCCEEDS with the well-known empty tree, so a silently-failed copy would +# make the index component a constant that matches itself. +``` + +`codex-gate.test.sh:126` — the NOTE claiming the guard has no regression test. Narrow it: + +```sh +# NOTE: sections 24-25 cover the "could not be computed" guard for checksum, seed-copy, +# git-dir and diff failures. Still uncovered: a `git add`/`write-tree` failure inside the +# throwaway index (see the tree-unavailable row in todos.md, which stays parked). +``` + +- [ ] **Step 4: Fix the user-facing docs** + +`README.md:26` — replace "verify a Gate-B review against the actual content of the working +tree" with "verify a Gate-B review against a fingerprint of the index and working tree — +what a commit would actually carry". + +`docs/architecture.md:64-72` — name three components instead of two, and replace "the +check is tied to what is actually on disk" with "the check is tied to what a commit would +carry — the index as well as the worktree". Keep the edit-then-undo sentence but scope it: +an unstaged edit-then-undo still matches. + +`docs/getting-started.md:46` — replace "(staging alone doesn't; `git add` changes no +content)" with "(staging counts too: the fingerprint covers the index, because that is +what a commit carries)". + +`plugins/dev-workflow/commands/workflow-init.md:778` — replace "on both the tracked and +untracked side" with "from all three of its fingerprint components". + +- [ ] **Step 5: Settle the story's open questions** + +In `docs/superpowers/stories/2026-07-18-gate-b-hash-staged-worktree-divergence-story.md`: + +§2 — narrow "regardless of how the divergence arose" to "as of the hook's invocation: +a compound `git add X && git commit` stages after the PreToolUse fingerprint and remains +the separate Parked defect." + +§5 — replace all three open questions with their settled answers: staging invalidates +(spec §2); one content rule covers every commit shape, but only as of hook invocation, so +`git add X && git commit` stays parked; invariant 3's wording did need amending (Step 2). + +- [ ] **Step 6: Verify no site was missed** + +Run: `grep -rn "tree-hash proves\|actually reviewed what you are committing\|staging alone doesn't" --include='*.md' --include='*.sh' . | grep -v source-files/ | grep -v docs/superpowers/` +Expected: no output. + +Run the battery again (docs edits can break the hook if a comment was mangled): +`shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && sh plugins/dev-workflow/hooks/codex-gate.test.sh` +Expected: clean, 0 failures. + +- [ ] **Step 7: Commit** + +```bash +git add AGENTS.md README.md docs/architecture.md docs/getting-started.md docs/superpowers/stories plugins/dev-workflow/hooks plugins/dev-workflow/commands/workflow-init.md +git commit -m "docs: describe the fingerprint as index + worktree, not worktree alone + +Amend invariant 3 to name all three components and why the index one exists. +Correct every site that described the hash as worktree-only or claimed the +tree-hash proves what was reviewed, and settle the story's open questions." +``` + +--- + +### Task 5: Version bump and CHANGELOG + +Invariant 12: a PR touching `plugins/` must bump that manifest's version, or CI fails. + +**Files:** +- Modify: `plugins/dev-workflow/.claude-plugin/plugin.json` (`version`) +- Modify: `plugins/dev-workflow/CHANGELOG.md` + +**Interfaces:** +- Consumes: the completed changes from Tasks 1–4. +- Produces: nothing. + +- [ ] **Step 1: Bump the manifest** + +In `plugins/dev-workflow/.claude-plugin/plugin.json`, change `"version": "0.4.1"` to +`"version": "0.5.0"` — minor, not patch: the hook's fingerprint composition changes and a +previously-satisfied workflow (review → stage → commit) now stops. + +- [ ] **Step 2: Add the CHANGELOG entry** + +Insert directly below the intro paragraphs in `plugins/dev-workflow/CHANGELOG.md`: + +```markdown +## 0.5.0 + +- **Gate-B fingerprint covers the index.** `git commit` commits the index, but both hash + components described the worktree, so staging a change and then reverting the file on + disk reported Gate B satisfied on content nobody reviewed. The fingerprint now includes + a tree id from the effective index (`GIT_INDEX_FILE` when set, else the git-dir index). +- **Staging now invalidates a review.** `git add` of already-reviewed content changes the + index tree, so the fingerprint changes. The committed bytes are unchanged, making this + a false invalidation — accepted under the "loose in the firing direction" invariant, and + the reminder explains that staging alone can cause it. One clean pass clears it. +- **Upgrading invalidates any in-flight review once.** The fingerprint's composition + changed, so a fingerprint recorded by 0.4.x cannot match one computed by 0.5.0. The + first commit attempt after upgrading reports "cannot confirm"; a single review pass + clears it. This is expected, not a bug. +- **An uncomputable fingerprint now fails closed.** The failure value is a constant that + never matches — including against itself — replacing a `date`+PID nonce that could + collide under PID reuse and report satisfied. +- **Reminder text no longer asserts causes it cannot know.** The stale reminder names the + state ("cannot confirm") rather than claiming the tree changed, and describes the causes + that can produce it; the satisfied reminder claims fingerprint equality rather than that + Codex read the bytes. The clauses are pinned by the message-contract tests in + `codex-gate.test.sh` section 29 — that suite is what keeps this true, and it asserts the + clauses listed there, not an exhaustive enumeration of states. +``` + +- [ ] **Step 3: Run the version-bump checker's own regression suite** + +Run: `sh scripts/check-version-bump.test.sh` +Expected: exits 0. Nothing is staged or stashed here — the checker compares *commits*, +so it cannot see an uncommitted bump, and `git add -A`/`git stash` would sweep in +unrelated working-tree changes. + +The checker itself runs in Step 5, **after** the bump is committed, against the branch's +real base ref. Run against `main` with the bump still uncommitted it reports clean — +correctly, and uselessly. + +- [ ] **Step 4: Run the full quality battery** + +Run: +```bash +shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && \ +shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && \ +shellcheck --shell=sh scripts/check-invariants.sh && \ +shellcheck --shell=sh --exclude=SC2015 scripts/check-invariants.test.sh && \ +shellcheck --shell=sh scripts/check-version-bump.sh && \ +shellcheck --shell=sh scripts/check-version-bump.test.sh && \ +sh plugins/dev-workflow/hooks/codex-gate.test.sh && \ +sh scripts/check-invariants.test.sh && sh scripts/check-invariants.sh && \ +sh scripts/check-version-bump.test.sh && \ +claude plugin validate . --strict +``` +Expected: every command exits 0; `claude plugin validate` reports the plugin valid. + +- [ ] **Step 5: Commit, then run the version-bump checker against the real base** + +```bash +git add plugins/dev-workflow/.claude-plugin/plugin.json plugins/dev-workflow/CHANGELOG.md +git commit -m 'chore(plugin): release 0.5.0' +``` + +Then, with the work committed: + +Run: `sh scripts/check-version-bump.sh "$(git merge-base HEAD main)"` +Expected: exits 0, reporting the bump as present. This is the one battery member with a +precondition — it compares commits, so it is meaningless before the commit exists, and +`main` is the right base only while this branch is based on current `main`. CI passes the +PR's own base ref instead. + +--- + +### Task 6: Ledger and backlog + +**Files:** +- Modify: `todos.md` (remove the closed Parked row; add the review-range row) +- Modify: `docs/hardening-log.md` (append one row) + +**Interfaces:** +- Consumes: everything above. +- Produces: nothing. + +- [ ] **Step 1: Remove the closed row and add the new one** + +In `todos.md` under `### Parked (trigger-gated)`, delete the entire +"**Gate-B hash misses staged-vs-worktree divergence (false ✓, invariant 3).**" bullet. +The other four Parked rows stay untouched. + +Narrow the tree-unavailable row to match the test NOTE from Task 4 Step 3: sections 24–25 +now cover checksum, seed-copy, git-dir and diff failures, so what remains parked is the +`git add`/`write-tree` failure seam inside the throwaway index. + +Add, in the same Parked list: + +```markdown +- [ ] **Gate-B fingerprints disk; the reviewer reads history.** A review pass records a + fingerprint of the index and worktree, but `mcp__codex__review` reads a **git + range** — so content that is staged and never committed can be fingerprinted as + reviewed without Codex having read it, and three such passes reach ✓. Raised at + Gate A pass 8 of the index-tree story and deliberately deferred there: closing it + means refusing to satisfy Gate B unless the index and worktree correspond to the + reviewed range, i.e. mandating a WIP commit for every review. That redefines the + gate rather than fixing a hash, so it needs its own story and its own decision. + CLAUDE.md §5's WIP-commit flow is the current mitigation. +``` + +- [ ] **Step 2: Append the ledger row** + +Append this row verbatim to the table at the end of `docs/hardening-log.md` (columns +already verified against the existing rows: `date | fingerprint | finding | source | +severity | rung | ref`): + +```markdown +| 2026-07-19 | false-negative-gate | both Gate-B hash components described the worktree while `git commit` commits the index, so staging a change and reverting the file on disk reported satisfied on unreviewed content | gate-b | blocker | 4 test | plugins/dev-workflow/hooks/codex-gate.test.sh sections 24-30 + AGENTS.md invariant 3 (amended to name all three components). What the tests DO cover: the index component exists and moves on divergence, the failure marker never self-matches, and the ambient-alternate-index and three-way-`.context` states. What they do NOT: every divergence shape (they pin the mechanism, not exhaustive enumeration); mutation after the PreToolUse event, which is the parked compound-command row; command-local retargeting (`GIT_INDEX_FILE=`/`-C`/`--git-dir`), split to the 2026-07-19 command-retargeting-guard story; and a `git add`/`write-tree` failure inside the throwaway index, which stays parked | +``` + +- [ ] **Step 3: Verify the backlog reference still resolves** + +Run: `grep -n "staged-vs-worktree" todos.md docs/hardening-log.md docs/superpowers/ -r` +Expected: no `pending` ledger row still points at the deleted todos.md bullet. Any hit in +`docs/superpowers/` is a spec/story reference and is fine. + +- [ ] **Step 4: Commit** + +```bash +git add todos.md docs/hardening-log.md +git commit -m 'docs(ledger): close the staged-vs-worktree row, park the review-range gap' +``` + +- [ ] **Step 5: Run the complete quality battery on the finished branch** + +Task 5's battery ran before these ledger and backlog edits existed, so it did not cover +the final commit. `check-invariants.sh` reads markdown too — an unpinned example in a new +todos.md line would fail it — so the battery is re-run here, in the exact form AGENTS.md +calls canonical, with every commit in place: + +```bash +shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && \ +shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && \ +shellcheck --shell=sh scripts/check-invariants.sh && \ +shellcheck --shell=sh --exclude=SC2015 scripts/check-invariants.test.sh && \ +shellcheck --shell=sh scripts/check-version-bump.sh && \ +shellcheck --shell=sh scripts/check-version-bump.test.sh && \ +sh plugins/dev-workflow/hooks/codex-gate.test.sh && \ +sh scripts/check-invariants.test.sh && sh scripts/check-invariants.sh && \ +sh scripts/check-version-bump.test.sh && \ +sh scripts/check-version-bump.sh "$(git merge-base HEAD main)" && \ +claude plugin validate . --strict +``` +Expected: every command exits 0. If anything fails, fix it and amend the relevant commit — +the branch must not be opened as a PR with a red battery. + +--- + +## Verification checklist (before opening the PR) + +- [ ] Complete quality battery green on the FINAL commit (Task 6 Step 5) — not just + Task 5 Step 4, which ran before the ledger and backlog edits existed. +- [ ] All three mutations from Task 2 Step 5 turn a test red, and reverting them turns it + green again. +- [ ] `git log --format='%(trailers)' origin/main..HEAD` prints nothing (no `Co-Authored-By`). +- [ ] Story 2 exists at `docs/superpowers/stories/2026-07-19-command-retargeting-guard-story.md` + (AC-10 — it does; committed as `a0151ce`). +- [ ] Gate B (`mcp__codex__review`) on the full diff, per CLAUDE.md §5: three passes + minimum, final pass clean, re-reviewed after every fix. diff --git a/docs/superpowers/specs/2026-07-19-gate-b-index-tree-design.md b/docs/superpowers/specs/2026-07-19-gate-b-index-tree-design.md new file mode 100644 index 0000000..dea789c --- /dev/null +++ b/docs/superpowers/specs/2026-07-19-gate-b-index-tree-design.md @@ -0,0 +1,677 @@ +# Gate-B hash: cover the index, not just the worktree — Spec + +**Date:** 2026-07-19 · **Size:** story · **Story:** +`docs/superpowers/stories/2026-07-18-gate-b-hash-staged-worktree-divergence-story.md` + +**Gate A cost before the split:** 13 passes, ~108 findings. Recorded as a calibration +point for the risk-profile work (todos.md, P2) — a spec pinning shell code and prompt text +converges far slower than one at design altitude, and half of that cost was a second +feature that should have been its own story (§7). + +## 1. Problem + +`tree_hash()` in `plugins/dev-workflow/hooks/codex-gate.sh` composes two components — +`git diff HEAD` and a tree id written from a throwaway index seeded from the real index +and then brought up to date with `git add -A`. Both describe the **worktree**. `git +commit` commits the **index**. + +**Repro** (verified in a throwaway repo): + +```sh +printf 'v2\n' > app.ts && git add app.ts # index: v2 +printf 'v1\n' > app.ts # worktree: back to HEAD's bytes +``` + +`git diff HEAD` is then empty and the worktree tree id is unchanged, so the hash matches +the reviewed one and Gate B reports satisfied — while the commit carries `v2`. This is +the false-✓ direction invariant 3 exists to close. Found by Codex at Gate B on PR #1, +deferred there because the fix carries a decision rather than a correction. + +**Do not write the revert as `git checkout -- app.ts`.** That restores the worktree +*from the index*, so it would install `v2` on disk and leave index and worktree in +agreement — the divergence never happens, and a test built on it would pass against the +unfixed hook. Restore the bytes directly (as above) or use `git restore --source=HEAD +--worktree -- app.ts`; the `printf` form is preferred in tests, since it needs no +minimum git version. + +## 2. Decision record — does a bare `git add` invalidate a review? + +The fix forces an answer, and the answer was never actually made: today's behaviour +("staging does not invalidate") fell out of an implementation choice and is asserted by +test 3f as though it were a principle. + +**Two candidate semantics were considered.** + +- **A (chosen) — hash the index tree as a third component.** Content rule, no + interpretation: whatever the index would commit is part of the hash. Consequence: + staging already-reviewed content changes the index tree, so it invalidates the review + even though the bytes that would be committed are unchanged. +- **B (rejected) — hash the worktree tree plus an *index class*** (`matches-HEAD` / + `matches-worktree` / `other:`). Semantically ideal: "the commit carries + exactly what was reviewed", and `review → git add → commit` stays satisfied. + +**Chosen: A. Staging invalidates.** The false invalidation is an accepted price under +invariant 2 ("loose in the firing direction"), and B buys its precision with a rule the +next reader has to re-derive before touching invariant 3. This repo has already paid +twice for exactly that kind of cleverness (rename detection; the invariant-checker +regex). A boring content rule that is wrong only in the safe direction is the house +style. + +**What A costs, stated plainly:** the sequence `review → git add → +git commit` as three separate steps now produces a STOP, and the hook cannot distinguish +it from a real edit. §4 mitigates by making the message explain its own possible +falseness; §5 rewrites test 3f to assert this decision instead of contradicting it. + +**What A does not claim.** The guarantee is about **the index as it stands when the hook +is invoked**. A mutation that happens after the PreToolUse event — the compound +`printf x > f && git commit -am y`, or any concurrent writer — is still unseen; that is +the separate Parked defect (§7). The story's "regardless of how the divergence arose" is +narrowed to this snapshot semantics accordingly. + +## 3. Design — `tree_hash()` gains an index-tree component + +The temp index is already a copy of the real index. Take a tree id from it **before** +`git add -A` brings it up to the worktree, and both tree components come out of one temp +directory and two `write-tree` calls. + +**The function's shape has to change, not just its contents.** Today the producers feed +`{ … } | shasum` directly, so a failure marker printed among them would be *checksummed +along with the partial output* — the guards downstream would never see the literal +`unavailable`, and two repeated failures could again produce equal hashes. The component +stream is therefore buffered to a file in the temp directory that already exists, and the +checksum runs only once every producer has succeeded: + +```sh +tree_hash() { + ok=1 + # same fallback order as today; no tool at all is itself a failure + sum_cmd=$(command -v shasum || command -v sha1sum || command -v cksum) || ok=0 + tmp_dir=$(mktemp -d 2>/dev/null) || { tmp_dir=''; ok=0; } + if [ -n "$tmp_dir" ]; then + tmp_index="$tmp_dir/index" + { + # (0) tracked content, with the unborn-repo case positively identified + if git -C "$repo_root" rev-parse --verify -q HEAD >/dev/null 2>&1; then + git -C "$repo_root" diff HEAD -- . ':(exclude).context' 2>/dev/null || ok=0 + elif git -C "$repo_root" symbolic-ref -q HEAD >/dev/null 2>&1; then + printf 'no-head\n' # unborn branch: legitimate, nothing to diff against + else + ok=0 # detached/corrupt/unreadable HEAD is NOT unborn + fi + + # (1) and (2), from one throwaway index. git_dir must RESOLVE: an unchecked failure + # yields "/index", which does not exist, so the absent-index carve-out would read it + # as "nothing staged" and hand back the empty tree — a constant, hole reopened. + # The chain's status is consumed by `if` directly: a following `[ $? -eq 0 ]` is + # SC2181, which the hook cannot carry (linted with no exclusions). + if git_dir=$(git -C "$repo_root" rev-parse --absolute-git-dir 2>/dev/null) && + [ -n "$git_dir" ] && + eff_index=${GIT_INDEX_FILE:-$git_dir/index} && + case "$eff_index" in # a RELATIVE GIT_INDEX_FILE must be normalized + /*) : ;; # against $repo_root — see below + *) eff_index="$repo_root/$eff_index" ;; + esac && + { [ ! -e "$eff_index" ] || cp "$eff_index" "$tmp_index" 2>/dev/null; } && + GIT_INDEX_FILE=… git rm -rfq --cached --ignore-unmatch -- .context >/dev/null 2>&1 && + GIT_INDEX_FILE=… git write-tree && # (1) INDEX tree + GIT_INDEX_FILE=… git add -A -- . ':(exclude).context' >/dev/null 2>&1 && + GIT_INDEX_FILE=… git write-tree # (2) WORKTREE tree + then :; else ok=0; fi + } > "$tmp_dir/stream" 2>/dev/null || ok=0 + fi # the whole group is skipped when tmp_dir could not be created — + # otherwise the redirect would target "/stream" + + h='' + if [ "$ok" -eq 1 ]; then + # The checksum's OWN status must be seen. `cmd < file | awk` reports awk's status, + # so a checksum that fails after emitting a partial line would be accepted and + # stored as a repeatable fingerprint — two failures comparing equal, which is the + # false-✓ direction again. Capture, check status, then parse with a shell expansion + # rather than a second command: `${raw%% *}` has no exit status to mask, so the + # parser cannot fail half-way the way a piped `awk` can. + if raw=$("$sum_cmd" < "$tmp_dir/stream" 2>/dev/null); then + h=${raw%% *} + fi + fi + [ -n "${tmp_dir:-}" ] && rm -rf "$tmp_dir" 2>/dev/null # after the checksum reads it + # a checksum tool that runs but produces nothing is a failure too, not an empty tree + if [ -n "$h" ]; then printf '%s\n' "$h"; else printf 'unavailable\n'; fi +} +``` + +`ok` and the `git_dir` locals live in one shell — the buffering removes the pipeline that +previously forced a subshell, and a redirect on a `{ … }` group does not create one, so +the `ok=0` assignments inside it survive. Every read is initialized first or `:-` +defaulted, so `set -u` holds. + +**Write it as `if`/`else`, not `[ … ] && … || …`** — for readability, not for lint. +Checked rather than assumed (shellcheck 0.11.0, `--shell=sh`): SC2015 does *not* fire +when the branches are builtin `printf`, so the terse form would have passed. It fires on +the function-call form, which is why `--exclude=SC2015` exists for the *test* file. The +hook is linted with no exclusions, so a future edit that turns either branch into a +helper call would start failing — `if`/`else` is the shape that stays lintable. + +**Unborn is identified positively — and the residue is safe.** A bare "`rev-parse +--verify HEAD` failed" is not evidence of an unborn repo; `symbolic-ref -q HEAD` +succeeding while `--verify` fails is much closer to the real signature: a branch is +pointed at, with no commit yet. It is not *exclusive*, though — a symbolic HEAD whose +target ref exists but is corrupt satisfies it too, and so would a wrapper that fails only +the verify call. Those states take the `no-head` path rather than failing closed. + +That is safe, and the reason is worth stating because it is not obvious: component (0) is +a *diff against a baseline*, while components (1) and (2) are complete descriptions of +the index and the worktree. Losing the baseline never loses content coverage — anything +that would land in a commit is still hashed by (1) and (2). What (0) adds is +discrimination between two states that share a tree but differ against HEAD; with no +resolvable HEAD there is no such distinction to lose. A wholly unresolvable HEAD (not +even symbolic) still fails closed, since that indicates a repo the hook cannot read at +all. + +**A silent checksum is a failure.** If the checksum program runs but emits nothing, the +old code published an empty string, which the commit check reads as `[ -z "$reviewed" ]` +— the "no review has run" branch, whose message asserts a cause that is not true here. +Non-empty output is now part of success. + +**The nonce goes away.** Today an uncomputable hash appends +`tree-unavailable-$(date +%s)$$`, betting that no two invocations share a second and a +PID. PID reuse inside one second is rare, not impossible, and the bet is on the +false-✓ side: two failing invocations that collide would compare equal and report +satisfied. Since the sentinel exists precisely to *never* match, make that a fact rather +than a probability — a constant `unavailable`, with the comparison treating it as +never-equal on either side: + +```sh +if [ "$current" = unavailable ] || [ "$reviewed" = unavailable ] || + [ "$reviewed" != "$current" ]; then # the stale branch +``` + +This is less machinery than the nonce, not more: one deterministic token, no clock, no +PID, and a failure that cannot be mistaken for a fingerprint. A stored `unavailable` +(the review pass itself could not hash) likewise never satisfies. + +**One more consumer needs the guard.** The PostToolUse store path decides the *fresh* +pass count with `[ "$h" = "$prev" ]`. Two consecutive unhashable passes would compare +equal there and bump the fresh count — reporting that N passes cover the current tree +when none of them could see it. The satisfied branch is unreachable in that state anyway, +so the damage is confined to a number in a message, but the number is wrong and it is one +condition to prevent: require `[ "$h" != unavailable ]` before treating a repeat as +fresh, and zero the fresh count on an unhashable pass rather than starting it at 1 — a +pass that could not see the tree covers nothing. Every other consumer is a string +comparison that the two guards above already cover. + +**Ordering and framing.** The two tree ids are consecutive lines in the buffered stream, +distinguished by position. The contract is all-or-nothing: any producer failing — HEAD +discrimination, git-dir resolution, the seed copy, `.context` removal, the diff, `add`, +either `write-tree`, or the checksum itself — clears `ok`, and a cleared `ok` means the +stream is never checksummed at all. So a short or partial stream can never become +something that looks like a fingerprint. + +**The seed copy is inside the `&&` chain, and that is load-bearing.** Today a failed +`cp` means a cold index — slower, equally correct. Under this design a cold index is +*empty*, and `git write-tree` on an empty index **succeeds** with the well-known empty +tree `4b825dc…` (verified). Component (1) would then be a constant that matches itself +at review and at commit, silently reopening the hole. A failed copy must therefore fire, +not fall through. + +**But an absent index is not a failed copy.** `git init` creates no `.git/index` at all +— it appears on the first `git add` (verified). Treating that as a copy failure would +make every hash in an unborn repo a fresh sentinel, so an initial `git commit +--allow-empty` could never be satisfied: a permanent STOP in exactly the repo state +where someone is setting the workflow up. When the index file does not exist, a cold +empty temp index is the *correct* answer, not a degraded one — no index means nothing +staged, and the empty tree is what a commit would carry. Hence the +`{ [ ! -e "$eff_index" ] || cp … ; }` guard: skip the copy when there is nothing to copy, +require it to succeed when there is. + +**Keyed to `$eff_index`, not `$git_dir/index`** — the distinction is load-bearing once +the effective index can be an ambient `GIT_INDEX_FILE`. Git treats a `GIT_INDEX_FILE` +pointing at a *nonexistent* path as an empty index (verified: `write-tree` returns the +empty tree with rc 0), so the carve-out must follow the same path git will. Keyed to the +default index instead, the hook would demand a copy of a missing alternate index and +return `unavailable` forever in a situation git itself considers ordinary. + +**The index to hash is the *effective* one.** `git commit` honours `GIT_INDEX_FILE`, so a +commit can carry a tree that neither HEAD nor the worktree ever held — verified: with an +alternate index staging `SNEAKY` and the worktree back at `v1`, the commit carried +`SNEAKY`. When `GIT_INDEX_FILE` is exported in the session the hook inherits the same +environment as the command it gates, so seeding from `${GIT_INDEX_FILE:-$git_dir/index}` +closes it in one line. The hook's own `GIT_INDEX_FILE=…` overrides are local to each `git` +call and unaffected. + +A *command-local* override (`GIT_INDEX_FILE=alt git commit`) is invisible to the hook's +environment and is **not** handled here — that is story 2 (§7). + +**A relative `GIT_INDEX_FILE` must be normalized against `$repo_root`.** Added at Gate B +on Task 2 (commit `92a23f0`), after this spec was approved — recorded here because +CLAUDE.md §5 requires a fix that changes specified behaviour to update the spec in the +same commit, and leaving it out is the docs-drift class this project already records +against itself. The `[ ! -e ]` test and the `cp` are plain shell commands resolved +against the HOOK's cwd, while every git call uses `-C "$repo_root"` and git resolves a +relative `GIT_INDEX_FILE` against the repository TOP-LEVEL (verified: from a +subdirectory, an index existing only at the top level resolves, and one existing only in +that subdirectory yields the empty tree). Without the normalization, the hook run from a +subdirectory takes the absent-index carve-out, the index component becomes the constant +empty tree, and the staged-vs-worktree false-PASS this story closes silently returns. +Test 27e covers it. + +**`git rm` needs `-f`.** Without it, git refuses to remove a path whose staged content +differs from both HEAD and the worktree — exactly the divergent state this story is +about — and, with stderr redirected, does so silently (verified: `rc=1`, message +suppressed). The removal runs against the throwaway index only; the real staging area is +untouched (verified). `--ignore-unmatch` makes it a no-op when `.context/` is untracked. +Without the removal, a project that commits its adoption marker (the normal case) would +carry `.context/` into the index tree, where the hook's own state writes could +invalidate the review it just recorded. + +**`git diff HEAD` failure is no longer silent.** A dropped component is a false-✓ risk in +its own right: the remaining components can self-match while tracked content goes +unhashed. Its status now feeds the same sentinel. The one legitimate failure — a repo +with no commits, where `HEAD` does not resolve — is discriminated up front and emits the +constant `no-head` token instead. Nothing is lost there: with no HEAD, components (1) +and (2) already describe all content, and emitting a nonce would STOP every commit in a +repo that has not made its first one. + +Nothing else about the *hashing* changes: component ordering, the +`shasum`/`sha1sum`/`cksum` fallback order, and the exit-0 guarantee are untouched. Edits +outside `tree_hash()` are in scope where this section names them — the two comparison +guards, the fresh-count guard, and §4's message branches. + +## 4. The STOP message must explain its own false-fire + +The stale branch — the one reached when the stored and current fingerprints disagree, now +including the two `unavailable` guards from §3 rather than the bare `[ "$reviewed" != +"$current" ]` it used to be — fires whenever they differ. That is **all** it knows. Five +distinct states reach this branch, and the message may assert none of them as *the* cause +(prompt-standards item 10): + +1. content really changed since the review — the intended case; +2. already-reviewed content was merely staged (§2's accepted false fire); +3. the hash could not be computed (marker) — unwritable `TMPDIR`, a git failure; +4. the stored fingerprint predates this hook version, whose composition changed from + two components to three; +5. **the fresh pass's fingerprint was never stored.** The PostToolUse path writes state + with `{ printf … > "$state_file"; } 2>/dev/null || true` — deliberately silent, since + the hook must always exit 0. If `.context/` is unwritable, a review that hashed + perfectly leaves the old value behind, and the retry is stale forever. This one was + missed until Gate A pass 5; it is invisible from inside `tree_hash`, which is why the + message has to name it. + +**The message states what the hook knows, which is one thing: it could not confirm that +the pending commit was reviewed.** Not that content changed — in the repeated-failure +path both values are the `unavailable` marker, so nothing "changed" and no fingerprint +"differs". Not that the recorded passes fail to cover the commit — under cause 5 a pass +may have reviewed exactly this content and only failed to record it. Every earlier draft +of this section asserted one of those, and each was false on at least one of the five +states. The exact `additionalContext` (product prompt text, so pinned rather than +paraphrased): + +> STOP — Codex Gate B not satisfied: the hook cannot confirm that the content you are +> about to commit is the content mcp__codex__review last saw ($passes recorded pass(es) +> this cycle). Usually that means the working tree or the index changed since the review. +> It can also mean you only staged already-reviewed content — the bytes are fine, but the +> hook cannot tell staging from editing; that this hook was upgraded and the recorded +> fingerprint uses the older format (see CHANGELOG); or that the fresh fingerprint could +> not be computed or could not be stored. Run Gate B (mcp__codex__review) now — one clean +> pass is the complete remedy for the staging and post-upgrade cases too. If a fresh pass +> leaves this unchanged with nothing edited in between, the fault is in the machinery +> rather than the code: check that `.context/` is writable, that TMPDIR is writable, that +> a checksum tool (`shasum`, `sha1sum` or `cksum`) runs, that `git status` works, and +> that the disk is not full — then run one more pass to record a usable fingerprint. Per +> $policy you MUST re-review after every fix. + +- `systemMessage`: `⚠ Codex Gate B not satisfied (cannot confirm review)`. Not "stale": + when both values are `unavailable`, nothing became stale — confirmation was never + established in the first place. + +**The staging case gets no shortcut.** An earlier draft offered "commit via the WIP/amend +flow" as a way out without another pass. It is not one: the WIP commit moves HEAD, which +moves component (0), and the store path deliberately leaves the old fingerprint in place, +so the amend is still unconfirmed. The honest remedy for every cause here is one clean +pass, so the message says only that. (The WIP/amend flow remains what CLAUDE.md §5 +prescribes for producing a reviewable `baseSha` — a different problem.) + +**Why "a fresh pass leaves this unchanged" and not "retry immediately".** A mismatch of +*any* cause survives a bare retry — the stored fingerprint is only replaced by a review +pass. So "does it repeat?" separates nothing. + +**Why it says "the fault is in the machinery" and stops there.** The two-invocation test +narrows; it does not name. A single transient failure during the fresh pass stores +`unavailable`, and a healthy retry then compares a good hash against it — still stale, in +a repo that is now fine. That is why the remedy ends with *run one more pass* rather than +declaring a permanent environment fault: the transient case needs exactly one more pass, +and the persistent case needs the checks first and then a pass anyway. One instruction +serves both. + +**Why a bounded check-list and not a failure taxonomy.** Many distinct operations can set +the marker — temp dir, git-dir resolution, index copy, `.context` removal, the diff, +`add`, either `write-tree`, the checksum, HEAD discrimination — plus the state write, +which is outside `tree_hash` entirely. Reporting which one moved would mean carrying a +reason code out of a function whose whole contract is "one value that never collides", +and putting a multi-branch troubleshooting tree in a hook reminder. The five checks named +cover the things that actually differ between environments, including the two the earlier +drafts missed (`.context/` writability, checksum availability). Anything past them is a +bug report, not a user remedy. Stated here because "enumerate every cause" is otherwise +the obvious reading of prompt-standards item 10, and this is a deliberate departure. + +**The *satisfied* branch overclaims too, and this change is what proves it.** It currently +tells the model that the fresh passes "actually reviewed what you are committing" and that +the tree is "unchanged since that review". §7 establishes why that is false: the hook +compares a fingerprint of *disk*, while `mcp__codex__review` reads a *git range*, so a +fingerprint match does not establish that Codex read those bytes. Leaving it while +correcting every neighbouring claim would ship the one sentence this spec's own analysis +disproves. + +It becomes fingerprint-only: the passes cover the *same content fingerprint* as the one +being committed — with the code comment citing §7 for why the stronger phrasing was +dropped, so the next reader does not "restore" it as a clarity improvement. The narrower +claim is still the useful one: it is exactly what the hash can support. + +**The "no review has run" branch needs the same treatment.** `[ -z "$reviewed" ]` no +longer means only "no pass this cycle": a first pass whose state write failed leaves it +empty, and so does a state file that exists but cannot be *read* (`cat` failing on +permissions, or holding nothing). All three must be covered, and all three parts of that +branch — not just the model-facing text: + +- `additionalContext`: *no fingerprint is recorded for this cycle — either no + mcp__codex__review has run, or the last one's fingerprint could not be written or read + back. Check that `.context/` and the state file inside it are readable and writable; if + the file exists but is unreadable or empty, delete it and run a fresh pass.* +- `systemMessage`: `⚠ Codex Gate B: no recorded review` — not "not run", which asserts + the very cause that may be false. +- the adjacent code comment, which currently reads "nothing has been reviewed this + cycle". A shipped comment stating the wrong reason outlives the prompt that was fixed + around it; it must describe an absent *fingerprint*, not an absent review. + +One message per branch, and no new branches: telling the causes apart would mean +reporting which component moved, which is more machinery than the ambiguity is worth. + +**Upgrade note.** Adding a third component changes every stored fingerprint's meaning, so +the first commit attempt after upgrading invalidates any in-flight review once. One pass +clears it; the CHANGELOG entry must say so, or it reads as a bug. + +## 5. Tests (`plugins/dev-workflow/hooks/codex-gate.test.sh`) + +1. **New — the divergence repro.** Review a tree; edit and `git add` new content; restore + the worktree bytes to HEAD's (per §1, *not* `git checkout --`); assert **not + satisfied**. Mutation-verified: with the index component removed, this test fails. +2. **Rewritten — 3f.** Now asserts *staging a reviewed tracked file invalidates*, with a + comment naming this spec and §2's decision. The current comment asserts the opposite + as a principle; leaving it is the silent contradiction AC-3 exists to prevent. + + Its trailing "untracked file on a staged tree → not satisfied" assertion must be + re-based or dropped, not just carried along: once staging alone invalidates, that + assertion passes no matter what the untracked file does, so it would sit in the suite + looking like coverage while testing nothing. Either re-review to a clean state on the + staged tree first, then add the untracked file — or delete it, since test 3c already + covers untracked content. +3. **New — self-match.** A tree with staged content produces a hash that matches itself + across two hook invocations with no intervening change (no STOP-forever on a state the + hash can compute). +4. **New — seed-copy failure fires.** A stub `cp` earlier on `PATH` that exits non-zero + makes the gate read **not satisfied**. This is the regression guard for §3's + load-bearing claim: a future `|| true` on that line silently restores the false-✓ + defect, and nothing else would catch it. `PATH` interposition is portable POSIX and + needs no new test harness. + + It must also assert the fresh count is **0** after a failed pass, and still 0 after a + repeated one — the §3 store-path guard is new specified behaviour, and without an + assertion an implementation can keep writing 1 while every verdict test stays green. + + **The fault must be active for BOTH fingerprints** — the one the review pass stores + *and* the one the commit check computes — with no state change in between. Enabling it + only at commit time proves nothing: the fingerprints would differ anyway, so the test + would pass even against a broken `cp … || true`. With the fault spanning both and the + handling broken, both sides compute the same empty-tree-based hash and the gate reports + *satisfied* — which is exactly what the test must catch. That sequencing is what makes + the mutation verification real, and it doubles as the "repeated failure never + satisfies" case that retires the old nonce (§3). +5. **New — unborn repo.** In a repo with no commits and no `.git/index`, the hash + self-matches across invocations and an initial `git commit --allow-empty` can reach + satisfied. Guards the §3 absent-index carve-out: without it, the `cp`-must-succeed + rule silently STOPs every first commit. +6. **New — `git diff HEAD` failure fires.** A selective `git` wrapper earlier on `PATH` + that delegates every subcommand except `diff` (which it fails) must leave the gate + **not satisfied** — under the same both-fingerprints sequencing as test 4, and for the + same reason. Without this, the newly specified `diff_rc` handling can be dropped or + regressed with the whole suite green. +7. **New — `.context/` in three-way divergence.** A *tracked* `.context/` file whose index + bytes differ from both HEAD and the worktree — the state that makes `git rm --cached` + refuse without `-f` (verified) — must still hash and self-match; mutation-verified by + dropping `-f`. + + **It must also assert the user's real index is untouched:** capture the real index's + tree id, the staged `.context` blob, *and the index file's raw bytes* before the hook + runs, and require all three identical after. The semantic checks alone can pass while + extension/stat-cache bytes shift, which is weaker than AC-4's no-modification claim — + test 12 already byte-compares, so this matches it. The new `-f` makes this a data-loss path — a future edit that drops + `GIT_INDEX_FILE` from that one command would force-unstage the user's real `.context` + entries, and a self-match-only test stays green while it happens. +8. **New — unresolvable git-dir fires.** A selective `git` wrapper that fails only + `rev-parse --absolute-git-dir`, active across both fingerprints, must leave the gate + **not satisfied**. AC-4 names this path; without a test the pass-3 regression can + return through an incomplete implementation with the plan looking complete. +9. **New — checksum failure fires.** A stub checksum program that exits 0 but prints + nothing must yield `unavailable`, not an empty fingerprint — i.e. the commit check + must reach the stale branch, *not* the "no review has run" branch whose message would + assert a false cause. Same both-fingerprints sequencing as tests 4, 6 and 8: with the + stub active only at commit time, a mismatch against the earlier valid hash proves + nothing about whether the empty-output handling exists. + + **A second stub covers the other half:** one that prints a plausible token and *then* + exits non-zero. That is the case the masked-status defect allowed — output looks fine, + status is ignored, and two such invocations self-match into a false ✓. Empty-output + and failing-with-output are different mutations; a test for one does not catch the + other. +10. **New — message text.** Assert every clause AC-5 promises: the cannot-confirm label, + the staging explanation, the compute-or-store failure mention, both remedies (fresh + pass — no WIP/amend shortcut), the post-upgrade case, and the fresh-pass + discriminator with all five of its checks (`.context/` writable, TMPDIR writable, + checksum tool runs, `git status` works, disk not full). Not "repeat immediately" — that + wording was the pass-3 defect and a test asserting it would re-enshrine it. AC-5 is + product behaviour (invariant 11), so an unasserted clause regresses with the suite + green. +10b. **New — satisfied-branch wording.** The satisfied message must not claim the passes + reviewed the content, only that they cover the same fingerprint. Assert the absence of + the old "actually reviewed what you are committing" phrasing as well as the presence of + the new one — a message test that only checks for new text stays green if both survive. +11. **New — unstorable state.** Three shapes, because they reach the branch by different + routes: + - *Replacement fails.* Make the **state file itself** read-only (not just its parent + — a redirect can still truncate an existing writable file inside a non-writable + directory, so `chmod` on the directory would test something else or nothing). + Change the tree, run a pass, prove the stored bytes are unchanged, then assert the + commit check does not claim the recorded passes cover the commit. + - *First write fails*, with no prior state: assert the empty-state branch admits an + unwritten-or-unreadable fingerprint rather than flatly "no review has run". + - *Read fails*: an existing, non-empty state file that cannot be read. It reaches the + empty-state branch by a different route than an unwritten one, so the routing can + regress independently while the message text still matches. Assert the message and + that the hook still exits 0 (invariant 1). + + Cause 5 lives outside `tree_hash`, so no other test reaches it. + + **These three depend on `chmod` actually denying access, which is false under an + effective root uid** — a root-run CI job would silently exercise the success path and + then fail on the assertion, reporting a product defect that is really a privilege + artefact. Probe the *exact* operation each fixture depends on — replacing an existing + file, creating a file in a directory, and reading a file are three different denials, + and a platform can grant one while refusing another — restore whatever the probe + changed, and skip with a printed reason when the fault cannot be induced. A skipped + test that says why beats a red one that lies. +12. **New — ambient alternate index.** Three shapes, because a negative-only test would + be satisfied by an implementation that simply fires whenever `GIT_INDEX_FILE` is set — + a permanent STOP, not a fix: + - *Divergent:* record the review fingerprint **without** the alternate index, then + enable a divergent ambient `GIT_INDEX_FILE` for the commit check only → **not + satisfied** (verified as a real false-✓ today). The sequencing has to be pinned: if + the same alternate index spanned both, the next shape would require *satisfied*, and + the two would specify contradictory implementations. Mutation-verify against seeding + `$git_dir/index`. + - *Stable:* review and commit under the **same unchanged** alternate index → + **satisfied**, with the alternate index and the default index each byte-identical to + *its own* snapshot taken before the hook ran (not to each other — they legitimately + differ). This is the assertion that stops "always fire" from passing. + - *Missing path:* `GIT_INDEX_FILE` pointing at a nonexistent path is an empty index to + git (verified), so the hook must hash and **self-match** there rather than returning + `unavailable`, with both index files untouched. This is the guard against keying the + absent-index carve-out to the default index. + + Command-local overrides and retargeted-repo commands are story 2 (§7); no test here + covers them, and none should — a test for a guard this story does not ship would fail + or, worse, pass vacuously. + +13. **Preserved.** `.context/` churn — tracked and untracked — still does not invalidate + (existing 3e / 3e-bis, now also exercising the index component). + +14. **Existing assertions that this change breaks.** Two must be updated in the same + commit or the suite fails on a correct implementation: + - **line 63** asserts the literal `tree has CHANGED`, which the new message does not + contain. Re-point it at the new label. + - **line 126's NOTE** says tree_hash's "could not be computed" guard has no + regression test. Tests 4, 6, 8 and 9 give it several. Narrow it to what stays + uncovered — the `git add`/`write-tree` failure seam — rather than deleting it: the + todos.md row it points at is the *tree-unavailable test* row, which stays parked. + That row needs the same narrowing, so the two keep agreeing. + +**Every failure test must reach the floor.** The hook checks empty state, then stale, +then the floor — the floor is *last*. That is exactly why the fixture must meet it: the +mutation these tests exist to catch makes two failing hashes match, which skips the stale +branch and falls through to the floor. With one pass the result is "below floor" — not +satisfied, but for the wrong reason, so the test would stay green against the mutation. Use the existing `.context/codex-gate.floor` +override (set it to 1) or run three faulted passes, then assert. + +Not covered, deliberately: post-invocation mutation (§7). Tests 4 and 6 establish a +`PATH`-interposition seam that the Parked "no regression test for tree_hash's *tree +unavailable* guard" item also wants — that item nonetheless **stays parked**; building +the seam here for paths this story introduces is not licence to spend its budget. + +## 6. Docs and invariants + +Every site that describes the hash as worktree-only, found by grep rather than memory — +docs-drift is this repo's own most frequent finding class: + +- **`AGENTS.md:86`, invariant 3** — "compares a hash of the working tree" names the + referent this story proves wrong. Amend to state all three components (diff vs HEAD, + index tree, worktree tree) and why the index one exists: the commit takes the index. +- **Hook header, lines 6–11** — "a hash of the working tree", "compares what is actually + on disk". +- **The satisfied branch's message** (~line 407) — "actually reviewed what you are + committing" / "unchanged since that review". Same false claim as line 105, in the + message users and the model see most often. Narrow to fingerprint equality, with a + comment citing §7. +- **Hook line 105** — "unlike Gate B, where the tree-hash proves what was reviewed". §7 + establishes that this is false: the hash proves equality with a recorded *disk* + fingerprint, not that Codex read those bytes (it reads a git range). Correcting the + surrounding hash comments while leaving this one would ship a claim the same change + disproves. Narrow it to what the hash actually proves. +- **Hook comment block, ~150–193** — delete the `KNOWN GAP` paragraph, correct the + "`git add` … does not change the hash" paragraph to state §2's decision, and note that + the seed copy is now correctness-critical. +- **`README.md:26`** — "verify a Gate-B review against the actual content of the working + tree". +- **`docs/architecture.md:64–72`** — names the two components, and asserts the check is + "tied to what is actually on disk". (Its "an edit-then-undo correctly stays valid" + sentence remains true — an unstaged edit-then-undo still matches — but re-read it in + context once the surrounding text changes.) +- **`plugins/dev-workflow/commands/workflow-init.md:777`** — a *shipped prompt*, not + prose: the scaffolded explanation says `.context/` is excluded "on both the tracked and + untracked side". With a third component that sentence is incomplete. (Missed by the + first grep, which paired *worktree* with hash words; found at Gate A pass 2.) +- **The story itself** (`docs/superpowers/stories/2026-07-18-…-story.md`) — its §2 still + promises detection "regardless of how the divergence arose", which §2 of this spec + narrows to snapshot semantics, and **all three** of its §5 open questions are now + settled: the bare-`git add` question (§2 here), whether the answer should depend on + commit shape (no — one content rule covers every shape, but only as of the **hook's + invocation**: `git commit` and `git commit -a` are fully covered, while + `git add X && git commit` is a compound command whose staging runs *after* the + PreToolUse fingerprint and therefore stays the separate Parked defect — §2, §7), and + whether invariant 3's wording needs amending (yes — §6 here). Replace each with its answer, or the story contradicts the spec built from it. + (Missed by the §6 grep twice: the story is the one doc that describes the behaviour + without using the word *hash*.) +- **`docs/getting-started.md:46`** — "(staging alone doesn't; `git add` changes no + content)" is the plainest statement of the behaviour Option A reverses, in the one + document a new user reads end to end. Rewrite the parenthetical. +- **`todos.md`** — remove the Parked row this closes, and add one for the review-range + gap surfaced at Gate A pass 8 (below). The other four Parked hook items + stay parked. +- **Version bump + CHANGELOG** — invariant 12: the hook is under `plugins/`, so the + manifest version bumps in the same PR with a `CHANGELOG.md` entry. +- **Hardening ledger** — a row for this finding, per `dev-workflow:harden-finding`. + +## 7. Out of scope + +The other Parked hook items — jq-free escaped-quote parsing, compound commands hashing +the pre-mutation tree, the `tree-unavailable` guard's missing test, temp-index object +churn. The compound-command item is adjacent (it also concerns what a commit carries) +but is a separate defect with a separate fix. + +**Raised at Gate A pass 8 and deliberately not taken: the review-range gap.** Codex +argued that a review pass blesses the *fingerprint* of the index and worktree, while +`mcp__codex__review` reads a **git range** — so content that is staged but never +committed can be fingerprinted as reviewed without Codex having read it, and three such +passes reach ✓. + +That is true, and it is not this story. It is a property of how Gate B has always worked: +the hook records what is on disk, the reviewer reads history, and CLAUDE.md §5's +WIP-commit flow exists precisely to make the range contain the work. Closing it would +mean refusing to satisfy Gate B unless the index and worktree correspond to the reviewed +range — effectively mandating a WIP commit for every review, which redefines the gate +rather than fixing a hash. This story is about the fingerprint disagreeing with *the +commit*; that one is about the fingerprint disagreeing with *the reviewer*. Different +defect, different decision, its own story: **a new `todos.md` Parked row** (§6), not a +silent expansion of this one. + +**Non-goal: the command-retargeting guard — split into its own story.** A commit whose +*command* selects a different index, git dir or work tree (`GIT_INDEX_FILE=`, `GIT_DIR=`, +`GIT_WORK_TREE=`, `-C`, `--git-dir`, `--work-tree`) is a real false-✓ path — verified, +`GIT_INDEX_FILE=alt git commit` committed content the worktree never held — but it is not +this story's defect and is not fixed here. Story 2 owns it: +`docs/superpowers/stories/2026-07-19-command-retargeting-guard-story.md`. + +Why it moved out: it began as a two-line addition at Gate A pass 7 and grew into a +cohesive second feature — its own STOP branch and message, a placement rule relative to +`is_docs_only` and the WIP exemption, and an eight-shape test matrix. Five successive +bypasses (backslash-newline, `-\C`, quoted `GIT_INDEX_"FILE"=`, `${x-}` expansion, brace +expansion) each drew one more token into a blocklist, which is the pattern AGENTS.md's +escalation trigger names as the same rung applied repeatedly rather than the ladder +working. Story 2 starts from default-deny instead, decided rather than discovered. + +Only the *ambient* case stays here, because it is a property of which index to hash rather +than of parsing a command: one `${GIT_INDEX_FILE:-…}` in `tree_hash()` (§3). + +## 8. Acceptance criteria + +- [ ] **AC-1** Staging new content, then restoring the worktree bytes to HEAD's → **not + satisfied** at commit time. +- [ ] **AC-2** That scenario has a regression test, mutation-verified: reverting the fix + makes it fail. +- [ ] **AC-3** The bare-`git add` question is answered in writing (§2) and test 3f is + rewritten to assert that answer, with a comment naming the decision. +- [ ] **AC-4** Every state whose hash **can** be computed *and whose git configuration is + deterministic* matches itself across + invocations with no intervening change (no STOP-forever) — including an unborn repo + with no `.git/index`, and a tracked `.context/` file diverging three ways. Every + state whose hash **cannot** be computed fails closed: reads unreviewed, never + satisfied — including seed-copy failure, `git diff HEAD` failure, and unresolvable + git-dir, each with the fault spanning both fingerprints so that repeated failure + never self-matches. Both directions are exercised by tests, and the hook must not + leave the user's real index or staging area modified in any of them. +- [ ] **AC-5** The stale STOP message claims only what the hook knows — that it cannot + confirm the pending commit was reviewed — names the staging and compute/store cases + without asserting any as *the* cause, and carries the fresh-pass discriminator with + its five environment checks (§4). The empty-state branch likewise admits an + unstored fingerprint, and the **satisfied** branch claims fingerprint equality + rather than that Codex reviewed the content (§7). Each clause asserted by a test, + including all five checks. +- [ ] **AC-6** Invariant 3's wording covers the index component, and every doc site + listed in §6 is updated in the same change. +- [ ] **AC-7** Quality battery green, including `shellcheck --shell=sh` and the hook + suite under `sh`. +- [ ] **AC-8** Plugin manifest `version` bumped with a matching `CHANGELOG.md` entry + (invariant 12), and that entry states the one-time post-upgrade invalidation (§4). +- [ ] **AC-10** Story 2 exists on disk at the path §7 names before this story closes. + Removing the Parked row while its split-off half has only a dangling reference would + lose the follow-up work entirely — the row is the current owner, so the replacement + owner has to exist first. +- [ ] **AC-9** The Parked `todos.md` row this closes is removed, and a hardening-ledger + row is appended. diff --git a/docs/superpowers/stories/2026-07-18-gate-b-hash-staged-worktree-divergence-story.md b/docs/superpowers/stories/2026-07-18-gate-b-hash-staged-worktree-divergence-story.md index ed5a5d0..701ca6f 100644 --- a/docs/superpowers/stories/2026-07-18-gate-b-hash-staged-worktree-divergence-story.md +++ b/docs/superpowers/stories/2026-07-18-gate-b-hash-staged-worktree-divergence-story.md @@ -16,7 +16,9 @@ deferred there, because the fix contains a decision rather than just a correctio ## 2. Desired outcome A commit whose staged content differs from what was reviewed is never reported as -Gate-B satisfied, regardless of how the divergence arose. Alongside that, the project +Gate-B satisfied, as of the hook's invocation: a compound `git add X && git commit` +stages after the PreToolUse fingerprint and remains the separate Parked defect. +Alongside that, the project has an explicit, recorded answer to the question the fix forces: whether merely staging already-reviewed content should invalidate a review. Today that behaviour is asserted by a passing test but was never actually decided — it fell out of an implementation @@ -52,13 +54,14 @@ choice. ## 5. Open questions - Should staging already-reviewed content (a bare `git add` that changes nothing about - the bytes) invalidate the review? Invariant 2 argues yes; day-to-day usability argues - no, since `review → stage → commit` is normal and a STOP there is noise on every - commit. + the bytes) invalidate the review? **Settled: yes** — staging invalidates (spec §2). - Should the answer depend on the commit's shape — `git commit` (index only) vs `git commit -a` vs `git add X && git commit` — or should one rule cover all three? + **Settled:** one content rule covers every commit shape, but only as of the hook's + invocation, so `git add X && git commit` — which stages after the PreToolUse + fingerprint — stays parked. - Does invariant 3's wording need amending, given "working tree" is the referent that - turned out to be wrong? + turned out to be wrong? **Settled: yes**, it did (Step 2). ## 6. Suggested size diff --git a/docs/superpowers/stories/2026-07-19-command-retargeting-guard-story.md b/docs/superpowers/stories/2026-07-19-command-retargeting-guard-story.md new file mode 100644 index 0000000..e5692cb --- /dev/null +++ b/docs/superpowers/stories/2026-07-19-command-retargeting-guard-story.md @@ -0,0 +1,88 @@ +# Command-retargeting commits bypass Gate B — Story + +**Date:** 2026-07-19 · **Size:** story + +## 1. Problem statement + +`repo_root` is bound once when the hook starts, so a commit whose *command* selects a +different index, git directory or work tree commits content the hook never fingerprinted +— while Gate B reports satisfied. Verified: with an alternate index staging `SNEAKY` and +the worktree holding `v1`, `GIT_INDEX_FILE=alt git commit` committed `SNEAKY`. + +Found during Gate A on the index-tree hash story +(`docs/superpowers/specs/2026-07-19-gate-b-index-tree-design.md`), where it was first +taken in as a two-line addition and then split out at §7 as a cohesive second feature. + +## 2. Desired outcome + +Commit commands the hook cannot positively read as a plain `git commit` are treated as +unverified — the gate fails closed rather than reporting satisfied. An ordinary commit is +unaffected. + +The user learns that the hook *cannot verify* this commit, rather than that the code is +unreviewed: re-reviewing cannot make a retargeted command verifiable, so a message +prescribing another review would send someone into an unbounded loop. + +## 3. Acceptance criteria + +- [ ] An ordinary `git commit -m "…"` with no override still reaches satisfied — no + regression in the common path. +- [ ] Each override spelling is not satisfied: `GIT_INDEX_FILE=`, `GIT_DIR=`, + `GIT_WORK_TREE=`, `-C`, `--git-dir`, `--work-tree` — the last exercised via + `commit -a`, which is what makes an alternate work tree affect committed bytes. +- [ ] All five bypasses documented during story 1 fail closed, each mutation-verified + independently: backslash-newline, `-\C`, quoted `GIT_INDEX_"FILE"=`, `${x-}` + expansion, brace expansion. +- [ ] A spelling *not* anticipated by the implementation also fails closed — the property + that distinguishes this outcome from a blocklist, and the reason this is a story + rather than a sixth patch. +- [ ] The `WIP:` exemption is preserved: a WIP commit stays cycle-internal even when it + carries an override. +- [ ] The verdict precedes docs-only classification, which reads the default index and + would otherwise wave through an alternate index staging code. +- [ ] The message states that the hook cannot establish the target, and does not + prescribe re-reviewing. +- [ ] Quality battery green, including `shellcheck --shell=sh` and the hook suite under + `sh`. + +## 4. Affected AGENTS.md invariants + +- `## What this project is` — "a fingerprinted hardening ledger where a recurring finding + escalates one rung harder (prose → lint → type → test)". This governs the shape of the + answer: five successive bypasses were each absorbed by adding one more token to a + blocklist, which is the same rung repeated rather than the ladder working. + *(The concrete precedent is `todos.md`'s "Escalation trigger for the invariant checker": + "Adding one more regex arm per newly-discovered spelling is **not** the ladder working; + it is the same rung applied repeatedly… the answer is a real YAML/shell parse logged as + the next rung — not another patch." It lives in todos.md, not AGENTS.md, and is scoped + to the invariant checker; it also excludes spellings "found during development", as + these were. It is cited as precedent for the pattern, not as a rule that formally + fired.)* +- `## Key invariants → Hook` — "**Loose in the firing direction.** On uncertainty, fire. A + missed commit (false ✓) is the dangerous direction; a redundant warning is the accepted + price." This is what prices failing closed on unreadable commands as acceptable. +- `## Key invariants → Hook` — "**Gate-B validity is content-derived, never + event-derived.**" The defect is a ✓ standing for content that was never hashed. +- `## Key invariants → Hook` — "**The hook always exits 0.**" +- `## Key invariants → Hook` — "**POSIX `sh`, and `jq` is optional.**" +- `## Prompts and scaffolding` — "**Prompt changes pass `docs/prompt-standards.md`** — all + 12 checklist items, for any skill, command, agent definition, hook message, or + scaffolded template." The new reminder text is a shipped prompt. + +## 5. Open questions + +- Where is the boundary of a command the hook can "positively read"? Env-prefixed + commands, `-c key=value` flags, multi-line commands and `&&` chains are each a + deliberate include or exclude, not something to inherit from whatever the earlier + blocklist happened to cover. +- What is the UX cost of failing closed on legitimate exotic invocations? Invariant 2 + prices it as acceptable, but the frequency should be estimated rather than assumed — a + gate that fires on commits people actually make will be worked around. +- Does this outcome subsume the parked compound-commands item (`printf x > f && git commit + -am y`, where the hook hashes the pre-mutation tree)? If an `&&` chain is unreadable by + definition, that row closes here; if not, it stays parked. + +## 6. Suggested size + +`story` — one coherent behaviour with one settled outcome, fits one spec → plan → PR. +Larger than the hash change it was split from, but not multiple subsystems. diff --git a/plugins/dev-workflow/.claude-plugin/plugin.json b/plugins/dev-workflow/.claude-plugin/plugin.json index 5b0ceb2..a10195d 100644 --- a/plugins/dev-workflow/.claude-plugin/plugin.json +++ b/plugins/dev-workflow/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "dev-workflow", "displayName": "Cross-Model Review Workflow", - "version": "0.4.1", + "version": "0.5.0", "description": "Spec-driven workflow with two independent cross-model review gates, an append-only hardening ledger with an escalation ladder, and repo-enforced quality. Requires the superpowers plugin.", "author": { "name": "Daniel Sänger", diff --git a/plugins/dev-workflow/CHANGELOG.md b/plugins/dev-workflow/CHANGELOG.md index ce8614d..4f4a23c 100644 --- a/plugins/dev-workflow/CHANGELOG.md +++ b/plugins/dev-workflow/CHANGELOG.md @@ -22,6 +22,32 @@ unambiguously, still fails. Deleting only a plugin's *manifest* while the direct keeps shipping fails too. AGENTS.md invariant 12 carries the complete list. +## 0.5.0 + +- **Gate-B fingerprint covers the index.** `git commit` commits the index, but both hash + components described the worktree, so staging a change and then reverting the file on + disk reported Gate B satisfied on content nobody reviewed. The fingerprint now includes + a tree id from the effective index (`GIT_INDEX_FILE` when set, else the git-dir index). +- **Staging now invalidates a review.** `git add` of already-reviewed content changes the + index tree, so the fingerprint changes. The committed bytes are unchanged, making this + a false invalidation — accepted under the "loose in the firing direction" invariant, and + the reminder explains that staging alone can cause it. One clean pass clears it. +- **Upgrading invalidates any in-flight review once.** The fingerprint's composition + changed, so a fingerprint recorded by 0.4.x cannot match one computed by 0.5.0. The + first Gate-B-applicable commit attempt after upgrading reports "cannot confirm" (a + `WIP:` commit or a docs-only commit bypasses the comparison); a single review pass + clears it. This is expected, not a bug. +- **An uncomputable fingerprint now fails closed.** The failure value is a constant that + never matches — including against itself — replacing a `date`+PID nonce that could + collide under PID reuse and report satisfied. +- **Reminder text no longer asserts causes it cannot know.** The stale reminder names the + state ("cannot confirm") rather than claiming the tree changed, and describes the causes + that can produce it; the satisfied reminder claims fingerprint equality rather than that + Codex read the bytes. The message-contract tests in `codex-gate.test.sh` section 29 are + what keep this true: they compare each branch's complete `additionalContext` and + `systemMessage` against a golden fixture, so any reworded or reversed clause fails the + suite rather than only the clauses someone thought to enumerate. + ## 0.4.1 - **A plugin change now requires a version bump**, checked in CI on every pull request diff --git a/plugins/dev-workflow/commands/workflow-init.md b/plugins/dev-workflow/commands/workflow-init.md index 0261ee4..a2f02b4 100644 --- a/plugins/dev-workflow/commands/workflow-init.md +++ b/plugins/dev-workflow/commands/workflow-init.md @@ -775,8 +775,8 @@ states the adoption instead of inferring it from a heading someone may reword. **Commit it.** `CLAUDE.md` is committed, so a clone is adopted the moment it lands; leaving the marker untracked would make adoption depend on who ran this command. (The -hook excludes `.context/` from its tree hash on both the tracked and untracked side, so -a committed marker does not disturb Gate B.) +hook excludes `.context/` from all three of its fingerprint components, so a committed +marker does not disturb Gate B.) Say in the report that this is what makes the hook speak in this project, and that deleting it plus the §5 heading is the way to make it stop. diff --git a/plugins/dev-workflow/hooks/codex-gate.sh b/plugins/dev-workflow/hooks/codex-gate.sh index e58232f..5b6019b 100755 --- a/plugins/dev-workflow/hooks/codex-gate.sh +++ b/plugins/dev-workflow/hooks/codex-gate.sh @@ -3,12 +3,15 @@ # Reads a Claude Code hook payload (JSON) on stdin, maintains Gate-A/Gate-B state, # and emits reminders. # -# Gate B is verified by CONTENT, not by events: the state file holds a hash of the -# working tree taken at review time, and the commit check recomputes it. An -# event-based scheme (invalidate on Edit/Write) is blind to a file changed through -# Bash — `sed -i`, `eslint --fix`, `git apply`, a codegen step — which would leave a -# stale "reviewed" marker standing. A false ✓ is the dangerous direction, so the -# hook compares what is actually on disk. +# Gate B is verified by CONTENT, not by events: the state file holds a fingerprint of +# the index and the working tree taken at review time, and the commit check +# recomputes it. An event-based scheme (invalidate on Edit/Write) is blind to a file +# changed through Bash — `sed -i`, `eslint --fix`, `git apply`, a codegen step — which +# would leave a stale "reviewed" marker standing. A false ✓ is the dangerous +# direction, so the hook compares three components at invocation time: `git diff HEAD` +# for tracked content, a tree id for the effective index, and a tree id for the worktree. +# That is a deliberate SUPERSET of any one commit's payload — a plain `git commit` carries +# only the index — because firing on more than strictly necessary is the safe direction. set -u payload=$(cat) @@ -102,7 +105,9 @@ policy=$(gate_citation) || policy="this project's review policy" # read Codex's findings (so it can't auto-detect the "zero-findings" early exit # — that judgment stays with the model per §5), but it CAN count passes and flag # when the floor isn't met. This is what backs Gate A, which has no content check -# behind it (unlike Gate B, where the tree-hash proves what was reviewed). +# behind it (unlike Gate B, which at least compares a content fingerprint — though +# that proves the current fingerprint matches the recorded one, not that Codex read +# those bytes). # Per-project override: .context/codex-gate.floor holding a positive integer. floor=3 if [ -f "$floor_file" ]; then @@ -147,29 +152,31 @@ fi read_count() { if [ -f "$1" ]; then cat "$1" 2>/dev/null || echo 0; else echo 0; fi; } bump_count() { n=$(read_count "$1"); { printf '%s' "$((n + 1))" > "$1"; } 2>/dev/null || true; } -# Hash of everything that could end up in a commit: the diff of tracked files -# against HEAD (staged + unstaged) plus the untracked files' paths, contents and -# modes, via a throwaway-index tree id (see below). `.context/` is -# excluded because the hook writes its own state there — including it would make -# the hash change every time the hook runs, so it could never match itself. +# Fingerprint used for the Gate-B comparison, computed at hook invocation: the diff of +# tracked files against HEAD (staged + unstaged), a tree id for the EFFECTIVE INDEX, and +# a tree id for the WORKTREE's untracked paths, contents and modes, via a throwaway +# index (see below). `.context/` is excluded because the hook writes its own state there — +# including it would make the hash change every time the hook runs, so it could never +# match itself. # -# BOTH components must exclude it, and for different reasons. Untracked state is kept -# out by the same `:(exclude)` pathspec on `add -A`. Tracked state needs it too: -# `.context/` is committed in some projects — the adoption marker is meant to be -# shared, so this is the normal case, not an exotic one — and a tracked state file -# lands in `git diff HEAD`, where the hook's own write would invalidate the review it -# just recorded and STOP every commit forever. +# ALL THREE components must exclude it, and each does so a different way. The tracked +# diff excludes it via a `:(exclude)` pathspec. The index tree excludes it by `git rm +# --cached` against the throwaway index before that tree is written. The worktree tree +# excludes it via the same `:(exclude)` pathspec on `add -A`. `.context/` is committed +# in some projects — the adoption marker is meant to be shared, so this is the normal +# case, not an exotic one — and a tracked state file left in would land in `git diff +# HEAD`, where the hook's own write would invalidate the review it just recorded and +# STOP every commit forever. # -# Tracked CONTENT comes from `git diff HEAD`, which is staging-independent: it sees -# staged and unstaged edits alike, so `git add` of an already-reviewed file does not -# change the hash — status output would have flipped that file's column on staging -# (` M` → `M `) and falsely invalidated a review of unchanged content. +# Tracked CONTENT comes from `git diff HEAD`, which is staging-independent. The INDEX +# tree is hashed separately, so `git add` of an already-reviewed file DOES invalidate: +# decided at docs/superpowers/specs/2026-07-19-gate-b-index-tree-design.md §2. The +# committed bytes are unchanged in that case, so it is a false invalidation — accepted +# under invariant 2, and the STOP message says staging alone can cause it. # -# KNOWN GAP (tracked in todos.md): both components describe the WORKTREE, so a tracked -# file whose staged content differs from its worktree content — `git add` it, then -# revert the file on disk — is committed from the index but hashed from disk, and reads -# as unchanged. Closing it means hashing the index tree separately, which also makes a -# bare `git add` invalidate a review; that trade needs its own change and tests. +# The seed copy is correctness-critical, not just a speed optimisation: `write-tree` on +# an empty index SUCCEEDS with the well-known empty tree, so a silently-failed copy would +# make the index component a constant that matches itself. # # Untracked files contribute NAME AND CONTENT. Name alone is not enough: `git add f # && git commit` is one Bash call, so the hook is consulted while `f` is still @@ -192,45 +199,85 @@ bump_count() { n=$(read_count "$1"); { printf '%s' "$((n + 1))" > "$1"; } 2>/dev # GIT_INDEX_FILE keeps this off the real index, so the user's staging area is # untouched. `add -A` respects .gitignore, so ignored files stay out. tree_hash() { - { - git -C "$repo_root" diff HEAD -- . ':(exclude).context' 2>/dev/null - # mktemp -d, not a predictable "$TMPDIR/name.$$": on a shared /tmp a predictable - # name is a symlink target an attacker can plant, and the seed copy below would - # then write index data through it into a file we do not own. - tmp_dir=$(mktemp -d 2>/dev/null) || tmp_dir='' - if [ -n "$tmp_dir" ]; then - tmp_index="$tmp_dir/index" - # Seed from the real index so git's stat cache still applies and only genuinely - # changed files get re-hashed. Starting empty is correct but re-hashes the entire - # repo on every hook invocation, which on a large repo is the difference between - # imperceptible and unusable. Copy failure is fine — an absent seed just means a - # cold, slower, equally correct index. - # --absolute-git-dir, not --git-path: the latter answers relative to git's cwd - # ("​.git/index"), but the `cp` resolves against the SHELL's cwd — which is - # wherever the harness invoked the hook, not necessarily the repo root. From any - # subdirectory the copy then fails silently and every run falls back to a cold - # index, i.e. re-hashing the whole repo, which is exactly what seeding avoids. - cp "$(git -C "$repo_root" rev-parse --absolute-git-dir 2>/dev/null)/index" "$tmp_index" 2>/dev/null || true - GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" add -A \ - -- . ':(exclude).context' >/dev/null 2>&1 && - GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" write-tree 2>/dev/null - tree_rc=$? - rm -rf "$tmp_dir" - else - tree_rc=1 - fi - # If the tree could not be computed, emit a value that cannot match ANY stored - # hash, so the gate reads as unreviewed. Staying silent would drop the component - # and let an unwritable TMPDIR collapse the hash to a constant — a permanent - # false ✓, the one direction invariant 3 exists to prevent. Firing wrongly is the - # accepted cost (invariant 2). - [ "${tree_rc:-1}" -eq 0 ] || printf 'tree-unavailable-%s\n' "$(date +%s 2>/dev/null)$$" - } | { - if command -v shasum >/dev/null 2>&1; then shasum - elif command -v sha1sum >/dev/null 2>&1; then sha1sum - else cksum # POSIX fallback: weaker, but present everywhere + ok=1 + # Same fallback order as before; no checksum tool at all is itself a failure. + sum_cmd=$(command -v shasum || command -v sha1sum || command -v cksum) || ok=0 + # mktemp -d, not a predictable "$TMPDIR/name.$$": on a shared /tmp a predictable name + # is a symlink target an attacker can plant. + tmp_dir=$(mktemp -d 2>/dev/null) || { tmp_dir=''; ok=0; } + if [ -n "$tmp_dir" ]; then + tmp_index="$tmp_dir/index" + # The component stream is BUFFERED and checksummed only on full success (below). + # Printing a failure marker in-stream would checksum the marker together with the + # partial output, so no consumer would ever see the literal `unavailable` and two + # failing runs could produce equal hashes — the false-✓ direction. + { + # (0) tracked content. `git diff HEAD` is staging-independent, so it sees staged + # and unstaged edits alike. An unborn branch is identified POSITIVELY: a bare + # "--verify failed" also covers a corrupt or unreadable HEAD, and emitting the + # constant `no-head` for those would self-match. + if git -C "$repo_root" rev-parse --verify -q HEAD >/dev/null 2>&1; then + git -C "$repo_root" diff HEAD -- . ':(exclude).context' 2>/dev/null || ok=0 + elif git -C "$repo_root" symbolic-ref -q HEAD >/dev/null 2>&1; then + printf 'no-head\n' + else + ok=0 + fi + + # (1) INDEX tree and (2) WORKTREE tree, from one throwaway index. + # The index tree is taken BEFORE `add -A` brings the temp index up to the + # worktree, because `git commit` commits the index — that is the whole defect. + # `eff_index`, not `$git_dir/index`: git honours GIT_INDEX_FILE, and a missing + # alternate index is an EMPTY index to git, so the carve-out must follow the same + # path git will. + # A RELATIVE GIT_INDEX_FILE must then be normalized against $repo_root before the + # `[ ! -e ]` test and `cp` below: those are plain shell commands, resolved against + # the hook's OWN cwd — while every git call here uses `-C "$repo_root"`, and git + # itself resolves a relative GIT_INDEX_FILE against the repository TOP-LEVEL, not + # the caller's cwd. Left unnormalized, running the hook from a subdirectory with a + # relative ambient GIT_INDEX_FILE makes the shell half look in the wrong place, + # find nothing, and take the absent-index carve-out — hashing a CONSTANT empty tree + # while the real index has content (a false "satisfied", not a false STOP). + # $repo_root is already absolute (from `git rev-parse --show-toplevel`), so + # prefixing it is enough; an already-absolute eff_index (incl. the $git_dir/index + # default) is left as-is. + # `rm -rfq --cached`: without -f git refuses to remove a path whose staged content + # differs from both HEAD and the worktree — exactly the divergent state this story + # is about — and does so silently, since stderr is redirected. It runs against the + # THROWAWAY index; the user's real staging area is untouched. + if git_dir=$(git -C "$repo_root" rev-parse --absolute-git-dir 2>/dev/null) && + [ -n "$git_dir" ] && + eff_index=${GIT_INDEX_FILE:-$git_dir/index} && + case "$eff_index" in + /*) : ;; + *) eff_index="$repo_root/$eff_index" ;; + esac && + { [ ! -e "$eff_index" ] || cp "$eff_index" "$tmp_index" 2>/dev/null; } && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" rm -rfq --cached \ + --ignore-unmatch -- .context >/dev/null 2>&1 && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" write-tree 2>/dev/null && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" add -A \ + -- . ':(exclude).context' >/dev/null 2>&1 && + GIT_INDEX_FILE="$tmp_index" git -C "$repo_root" write-tree 2>/dev/null + then :; else ok=0; fi + } > "$tmp_dir/stream" 2>/dev/null || ok=0 + fi + + h='' + if [ "$ok" -eq 1 ]; then + # The checksum's OWN status must be seen: `cmd < file | awk` reports awk's status, + # so a checksum failing after emitting a partial line would be stored as a real + # fingerprint. Parse with a shell expansion — `${raw%% *}` has no exit status to mask. + if raw=$("$sum_cmd" < "$tmp_dir/stream" 2>/dev/null); then + h=${raw%% *} fi - } 2>/dev/null | awk '{print $1}' + fi + if [ -n "${tmp_dir:-}" ]; then rm -rf "$tmp_dir" 2>/dev/null; fi + # A checksum that runs but emits nothing is a failure too, not an empty tree. The + # marker is a CONSTANT, not a nonce: `date +%s`+`$$` can repeat under PID reuse inside + # one second, and two colliding failures would compare equal and report satisfied. + # Never-matching is enforced at the comparison sites instead. + if [ -n "$h" ]; then printf '%s\n' "$h"; else printf 'unavailable\n'; fi } emit() { # $1 = additionalContext (model-visible), $2 = systemMessage (user) @@ -317,11 +364,21 @@ case "$event" in mkdir -p "$state_dir" 2>/dev/null h=$(tree_hash) prev=$(cat "$state_file" 2>/dev/null || echo '') - # A pass covering the SAME tree as the previous pass adds to the fresh count; - # a pass on a changed tree starts the fresh count over. This is what lets the - # satisfied message say how many passes cover the code being committed, - # rather than how many happened at some point this cycle (Finding 9). - if [ "$h" = "$prev" ]; then bump_count "$fresh_file"; else { printf '%s' 1 > "$fresh_file"; } 2>/dev/null || true; fi + # A pass carrying the SAME fingerprint as the previous pass adds to the fresh + # count; a pass on a changed fingerprint starts the count over. That is what lets + # the satisfied message report how many passes carry the CURRENT fingerprint, + # rather than how many happened at some point this cycle (Finding 9). It does not + # establish that Codex read those bytes — see the note at the satisfied branch. + # An unhashable pass carries no fingerprint, so it neither counts as a match with + # the last pass nor starts a fresh streak at 1 — two `unavailable` values are not + # a match. + if [ "$h" != unavailable ] && [ "$h" = "$prev" ]; then + bump_count "$fresh_file" + elif [ "$h" = unavailable ]; then + { printf '%s' 0 > "$fresh_file"; } 2>/dev/null || true + else + { printf '%s' 1 > "$fresh_file"; } 2>/dev/null || true + fi { printf '%s' "$h" > "$state_file"; } 2>/dev/null || true bump_count "$count_file" ;; @@ -391,20 +448,38 @@ case "$event" in reviewed=$(cat "$state_file" 2>/dev/null || echo '') current=$(tree_hash) if [ -z "$reviewed" ]; then - # No count ratio here on purpose: nothing has been reviewed this cycle, - # so showing "N/3" would read as floor progress. - emit "STOP — Codex Gate B not satisfied: no mcp__codex__review has run this cycle, so the CURRENT code is unreviewed. Per $policy you MUST reach a minimum of $floor passes per cycle and re-review after every fix. Run Gate B (mcp__codex__review) now, or proceed only if this change is trivial." "⚠ Codex Gate B not run" - elif [ "$reviewed" != "$current" ]; then - # Content check, not event check: this fires for a change made through - # ANY tool — Edit/Write, or a Bash `sed -i` / `eslint --fix` / `git apply`. - emit "STOP — Codex Gate B not satisfied: the working tree has CHANGED since the last mcp__codex__review, so the code you are about to commit is unreviewed (the $passes pass(es) this cycle covered the pre-change tree). Per $policy you MUST re-review after every fix. Run Gate B (mcp__codex__review) now, or proceed only if this change is trivial." "⚠ Codex Gate B stale (tree changed since review)" + # No count ratio here on purpose: no fingerprint is recorded for this + # cycle, so showing "N/3" would read as floor progress. `-z "$reviewed"` + # does not mean only "no pass this cycle": a FIRST write that fails (no + # prior fingerprint to fall back to), or a state file that cannot be read + # back, leaves it empty too. A failed REPLACEMENT write is different — it + # preserves the older, now-stale fingerprint, which reaches the STALE + # branch below, not this one. The message names the absent FINGERPRINT, + # not an absent review. + emit "STOP — Codex Gate B not satisfied: no fingerprint is recorded for this cycle — either no mcp__codex__review has run, or the last one's fingerprint could not be written or read back. Per $policy you MUST reach a minimum of $floor passes per cycle. Run Gate B (mcp__codex__review) now; if this repeats, check that .context/ and the state file inside it are readable and writable, and if the file exists but is unreadable or empty, delete it and run a fresh pass." "⚠ Codex Gate B: no recorded review" + # `unavailable` on EITHER side is never a match: an uncomputable fingerprint + # must read as unverified, and two of them must not cancel out. + elif [ "$current" = unavailable ] || [ "$reviewed" = unavailable ] || + [ "$reviewed" != "$current" ]; then + # Content check, not event check: this fires for a change made through ANY + # tool — Edit/Write, or a Bash `sed -i` / `eslint --fix` / `git apply`. + # It names the STATE, never a cause: five states reach here and the hook + # cannot tell them apart (spec §4). Do not "improve" this into asserting + # that the tree changed — under a repeated computation failure nothing + # changed, and under a failed state write the content may be exactly what + # was reviewed. + emit "STOP — Codex Gate B not satisfied: the hook cannot confirm that the content you are about to commit is the content mcp__codex__review last saw ($passes recorded pass(es) this cycle). Usually that means the working tree or the index changed since the review. It can also mean you only staged already-reviewed content — the bytes are fine, but the hook cannot tell staging from editing; that this hook was upgraded and the recorded fingerprint uses the older format (see CHANGELOG); or that the fresh fingerprint could not be computed or could not be stored. Run Gate B (mcp__codex__review) now — one clean pass is the complete remedy for the staging and post-upgrade cases too. If a fresh pass leaves this unchanged with nothing edited in between, the fault is in the machinery rather than the code: check that .context/ is writable, that TMPDIR is writable, that a checksum tool (shasum, sha1sum or cksum) runs, that git status works, and that the disk is not full — then run one more pass to record a usable fingerprint. Per $policy you MUST re-review after every fix." "⚠ Codex Gate B not satisfied (cannot confirm review)" elif [ "$passes" -lt "$floor" ]; then emit "Codex Gate B floor NOT met: only $passes/$floor mcp__codex__review pass(es) since the last commit. Per $policy the review is a LOOP with a hard minimum of $floor passes — run more (the ONLY early exit is a pass that returned zero findings), or proceed only if this change is trivial." "⚠ Codex Gate B below floor ($passes/$floor)" else # Distinguish the two counts (Finding 9): the cycle total includes passes - # made BEFORE later edits, which no longer cover the code being committed. - # Reporting only "$passes/$floor" would read as if all of them did. - emit "Codex Gate B: $passes/$floor pass(es) this cycle, of which $fresh cover the CURRENT tree (unchanged since that review). The floor counts the cycle; only the fresh pass(es) actually reviewed what you are committing. Per $policy, commit only if your final pass was clean — no new Blocker/Major." "✓ Codex Gate B satisfied ($passes/$floor cycle, $fresh on current code)" + # made BEFORE later edits, so they carry a different fingerprint. + # FINGERPRINT EQUALITY IS ALL THIS PROVES. The hook compares a hash of + # disk; mcp__codex__review reads a git range — so a match does NOT + # establish that Codex read these bytes (spec §7, and the review-range row + # in todos.md). The stronger phrasing was here and was removed; do not + # restore it as a clarity improvement. + emit "Codex Gate B: $passes/$floor pass(es) this cycle, of which $fresh cover the CURRENT content fingerprint (unchanged since that review). The floor counts the cycle; only the fresh pass(es) carry the same fingerprint as what you are committing. Per $policy, commit only if your final pass was clean — no new Blocker/Major." "✓ Codex Gate B satisfied ($passes/$floor cycle, $fresh on current fingerprint)" fi fi fi diff --git a/plugins/dev-workflow/hooks/codex-gate.test.sh b/plugins/dev-workflow/hooks/codex-gate.test.sh index 339efb2..e86b37f 100644 --- a/plugins/dev-workflow/hooks/codex-gate.test.sh +++ b/plugins/dev-workflow/hooks/codex-gate.test.sh @@ -45,7 +45,7 @@ rev # 2. Below floor (1/3) -> NOT satisfied yet; reaching floor (3/3) -> satisfied out=$(commitpre) -printf '%s' "$out" | grep -q 'below floor\|floor NOT met' && pass "1/3 passes -> below floor" || fail "1/3 passes -> below floor" +printf '%s' "$out" | grep -qE 'below floor|floor NOT met' && pass "1/3 passes -> below floor" || fail "1/3 passes -> below floor" printf '%s' "$out" | grep -q 'hookSpecificOutput' && pass "emits JSON additionalContext" || fail "emits JSON additionalContext" rev; rev # reach the floor: 3 passes total, tree unchanged out=$(commitpre) @@ -60,7 +60,7 @@ printf 'v2\n' >> app.ts run '{"hook_event_name":"PostToolUse","tool_name":"Edit","tool_input":{"file_path":"app.ts"}}' >/dev/null out=$(commitpre) printf '%s' "$out" | grep -q 'not satisfied' && pass "Edit-tool change -> not satisfied" || fail "Edit-tool change -> not satisfied" -printf '%s' "$out" | grep -q 'tree has CHANGED' && pass "Edit-tool change -> reported as stale tree" || fail "Edit-tool change -> reported as stale tree" +printf '%s' "$out" | grep -q 'cannot confirm' && pass "Edit-tool change -> reported as unconfirmed" || fail "Edit-tool change -> reported as unconfirmed" # 3b. THE MAJOR: a file changed through BASH (no Edit/Write event at all) -> stale. # Under the old event-based scheme this produced a false ✓. @@ -123,10 +123,9 @@ rm -f link.ts; ln -s absent-b link.ts printf '%s' "$(commitpre)" | grep -q 'not satisfied' && pass "retargeted untracked symlink -> NOT satisfied" || fail "retargeted untracked symlink -> NOT satisfied" rm -f link.ts -# NOTE: the "tree could not be computed" guard in tree_hash has no regression test. -# Forcing it portably means making mktemp or git fail on demand — TMPDIR=/dev/null -# looks like it does that but BSD/macOS mktemp silently falls back to /var/folders, so -# such a test passes for the wrong reason. Tracked in todos.md. +# NOTE: sections 24-25 cover the "could not be computed" guard for checksum, seed-copy, +# git-dir and diff failures. Still uncovered: a `git add`/`write-tree` failure inside the +# throwaway index (see the tree-unavailable row in todos.md, which stays parked). # A FIFO is not committable content; hashing must skip it rather than block on a # reader that never arrives. The hook is advisory and must not be able to wedge a @@ -170,6 +169,29 @@ rev; rev; rev printf '%s' "$(commitpre)" | grep -q 'Gate B satisfied' && pass "tracked .context/ state does not invalidate the hash" || fail "tracked .context/ state does not invalidate the hash" rev # more churn against the committed state printf '%s' "$(commitpre)" | grep -q 'Gate B satisfied' && pass "tracked .context/ churn stays satisfied" || fail "tracked .context/ churn stays satisfied" + +# 3e-ter. GAP 1 — the INDEX-tree component must exclude .context/ too, not just the +# diff-HEAD and worktree-tree components (each guarded by its own `:(exclude)` +# pathspec, untouched by this bug). Unstaged .context/ churn never reaches the real git +# index, so the two assertions above never exercise the `git rm --cached ... .context` +# line at all: nothing forces it to matter. STAGING the churn is the only way to make it +# matter — but staging .context ALONE trips the docs-only branch (is_docs_only treats +# .context/ as documentation) before Gate B is even evaluated, so a second staged, +# non-.context file is needed just to reach the fingerprint comparison at all. +# sidefile.ts is a throwaway file kept byte-identical for the whole test, so it cannot +# be the thing that (in)validates — only the .context staging can. +printf 'side\n' > sidefile.ts; git add sidefile.ts >/dev/null 2>&1 +rev; rev; rev +printf '%s' "$(commitpre)" | grep -q 'Gate B satisfied' && pass "setup: satisfied with sidefile.ts staged" || fail "setup: satisfied with sidefile.ts staged" +rev # hook writes fresh state into .context/ +git add -f .context >/dev/null 2>&1 # stage the hook's own churn (sidefile.ts untouched) +out=$(commitpre) +printf '%s' "$out" | grep -q 'Gate B satisfied' \ + && pass "staged .context churn does not invalidate (index-tree .context exclusion)" \ + || fail "staged .context churn does not invalidate (index-tree .context exclusion)" +git rm -q --cached sidefile.ts >/dev/null 2>&1; rm -f sidefile.ts +git reset -q -- .context >/dev/null 2>&1 # unstage; real index back to HEAD for .context/ + # ...while a real code change is still caught printf 'code change\n' >> app.ts printf '%s' "$(commitpre)" | grep -q 'not satisfied' && pass "tracked .context/: real code change still invalidates" || fail "tracked .context/: real code change still invalidates" @@ -177,21 +199,26 @@ git checkout -- app.ts >/dev/null 2>&1 git rm -rq --cached .context >/dev/null 2>&1; git commit -qm "untrack .context" >/dev/null 2>&1 reset_all; rev; rev; rev -# 3f. FINDING B — pure staging must NOT invalidate: `git diff HEAD` covers staged and -# unstaged tracked content alike, so `git add` of an already-reviewed file changes -# nothing that could end up in the commit. +# 3f. DECIDED at spec §2 (2026-07-19-gate-b-index-tree-design.md): staging +# already-reviewed content DOES invalidate. The hash covers the index tree, and +# `git add` changes it. The bytes that would be committed are unchanged, so this is +# a false invalidation — accepted under invariant 2 ("loose in the firing +# direction"), and the STOP message explains that staging alone can cause it. +# This test previously asserted the OPPOSITE as though it were a principle; the +# behaviour was never decided, it fell out of an implementation choice. reset_all printf 'reviewed change\n' >> app.ts rev; rev; rev # 3 passes covering the modified (unstaged) tree printf '%s' "$(commitpre)" | grep -q 'Gate B satisfied' && pass "setup: satisfied on unstaged change" || fail "setup: satisfied on unstaged change" git add app.ts >/dev/null 2>&1 # staging only — no content change -out=$(commitpre) -printf '%s' "$out" | grep -q 'Gate B satisfied' && pass "staging a reviewed tracked file -> still satisfied (Finding B)" || fail "staging a reviewed tracked file -> still satisfied (Finding B)" -# ...and an untracked file added on top still invalidates (direction preserved) -printf 'new\n' > also-new.ts -printf '%s' "$(commitpre)" | grep -q 'not satisfied' && pass "untracked file on a staged tree -> NOT satisfied" || fail "untracked file on a staged tree -> NOT satisfied" -rm -f also-new.ts +printf '%s' "$(commitpre)" | grep -q 'not satisfied' \ + && pass "staging a reviewed tracked file -> NOT satisfied (spec §2 decision)" \ + || fail "staging a reviewed tracked file -> NOT satisfied (spec §2 decision)" +# The old trailing assertion ("untracked file on a staged tree -> not satisfied") is +# GONE on purpose: once staging alone invalidates, it passes regardless of the untracked +# file and tests nothing. Test 3c already covers untracked content. git reset -q >/dev/null 2>&1; git checkout -- app.ts >/dev/null 2>&1 +reset_all; rev; rev; rev # 4. Gate A exec must NOT satisfy Gate B (separate state) reset_all @@ -338,13 +365,13 @@ rm -f "$countA" run '{"hook_event_name":"PostToolUse","tool_name":"mcp__codex__exec","tool_input":{}}' >/dev/null [ "$(cat "$countA" 2>/dev/null)" = 1 ] && pass "exec bumps Gate A count to 1" || fail "exec bumps Gate A count to 1" out=$(run '{"hook_event_name":"PreToolUse","tool_name":"Skill","tool_input":{"skill":"superpowers:executing-plans"}}') -printf '%s' "$out" | grep -q 'below floor\|floor NOT met' && pass "1/3 exec -> Gate A below floor" || fail "1/3 exec -> Gate A below floor" +printf '%s' "$out" | grep -qE 'below floor|floor NOT met' && pass "1/3 exec -> Gate A below floor" || fail "1/3 exec -> Gate A below floor" run '{"hook_event_name":"PostToolUse","tool_name":"mcp__codex__exec","tool_input":{}}' >/dev/null run '{"hook_event_name":"PostToolUse","tool_name":"mcp__codex__exec","tool_input":{}}' >/dev/null out=$(run '{"hook_event_name":"PreToolUse","tool_name":"Skill","tool_input":{"skill":"superpowers:executing-plans"}}') printf '%s' "$out" | grep -q 'floor met' && pass "3/3 exec -> Gate A satisfied" || fail "3/3 exec -> Gate A satisfied" # FINDING 12: the Gate-A satisfied wording must NOT overstate — it counts calls only. -printf '%s' "$out" | grep -q 'count only\|COUNT ONLY' && pass "Gate A satisfied says 'count only' (Finding 12)" || fail "Gate A satisfied says 'count only' (Finding 12)" +printf '%s' "$out" | grep -qE 'count only|COUNT ONLY' && pass "Gate A satisfied says 'count only' (Finding 12)" || fail "Gate A satisfied says 'count only' (Finding 12)" run '{"hook_event_name":"PostToolUse","tool_name":"Skill","tool_input":{"skill":"superpowers:executing-plans"}}' >/dev/null [ ! -f "$countA" ] && pass "plan execution resets Gate A count" || fail "plan execution resets Gate A count" @@ -385,7 +412,12 @@ printf 'post-review rewrite\n' >> app.ts # big change AFTER the passes rev # one fresh pass on the new tree out=$(commitpre) printf '%s' "$out" | grep -q '4/3' && pass "cycle total counts all 4 passes" || fail "cycle total counts all 4 passes" -printf '%s' "$out" | grep -q '1 cover the CURRENT tree\|of which 1' && pass "fresh count reports only 1 pass covers current code (Finding 9)" || fail "fresh count reports only 1 pass covers current code (Finding 9)" +# -F on the real wording: the old pattern's first alternative ("1 cover the CURRENT +# tree") was renamed to "CURRENT content fingerprint" by this PR and matched nothing, +# leaving the assertion resting on `\|` — alternation only under GNU-style BRE, literal +# under POSIX, so it would have failed on a stock BSD grep (invariant 4: machines we do +# not control). +printf '%s' "$out" | grep -qF 'of which 1 cover the CURRENT content fingerprint' && pass "fresh count reports only 1 pass covers current code (Finding 9)" || fail "fresh count reports only 1 pass covers current code (Finding 9)" git checkout -- app.ts >/dev/null 2>&1 reset_all @@ -565,5 +597,374 @@ rm -f CLAUDE.md git checkout -- . >/dev/null 2>&1 reset_all +# 24. Failure contract: an uncomputable hash must never satisfy, and repeated failures +# must never match each other. Spec §3 "The nonce goes away". +# Each fault spans BOTH the stored and the recomputed fingerprint — with the fault +# applied only at commit time, a mismatch proves nothing about the handling. +stub_dir=$(mktemp -d) +# Snapshot PATH before any stubbing, for the containment guard at the end of this +# section (24f) — a prefix assignment on a SHELL FUNCTION call (rev/commitpre are +# functions, not external commands) persists in the shell after the call returns, +# same defect class as the GIT_INDEX_FILE leak fixed in section 27. Every bare +# (not already inside `$(...)`) `PATH=... rev` below must be run in a subshell. +path_before_24="$PATH" +reset_all +printf '1' > "$floorf" # floor is checked LAST; without this a faulted run + # reports "below floor" and the test passes vacuously + +# 24a. every checksum tool fails silently (exit 0, no output) +for t in shasum sha1sum cksum; do + printf '#!/bin/sh\nexit 0\n' > "$stub_dir/$t"; chmod +x "$stub_dir/$t" +done +( PATH="$stub_dir:$PATH" rev ) +out=$(PATH="$stub_dir:$PATH" commitpre) +printf '%s' "$out" | grep -q 'not satisfied' && pass "silent checksum -> not satisfied" || fail "silent checksum -> not satisfied" +# Absence is asserted by inverting the RESULT, not with `grep -v` — `grep -qv` means +# "some line lacks the pattern", which is a different question and was observed to +# return 1 regardless on the dev machine. +printf '%s' "$out" | grep -qF 'no mcp__codex__review has run' \ + && fail "silent checksum -> not the never-run branch" \ + || pass "silent checksum -> not the never-run branch" + +# 24b. a checksum that PRINTS a plausible token and then fails +for t in shasum sha1sum cksum; do + printf '#!/bin/sh\nprintf "deadbeef -\\n"\nexit 1\n' > "$stub_dir/$t"; chmod +x "$stub_dir/$t" +done +reset_all; printf '1' > "$floorf" +( PATH="$stub_dir:$PATH" rev ) +out=$(PATH="$stub_dir:$PATH" commitpre) +printf '%s' "$out" | grep -q 'not satisfied' && pass "checksum prints then fails -> not satisfied" || fail "checksum prints then fails -> not satisfied" +[ "$(cat "$fresh" 2>/dev/null || echo 0)" = 0 ] && pass "unhashable pass leaves freshCount 0" || fail "unhashable pass leaves freshCount 0" + +# 24b-bis. GAP 2 — a SECOND consecutive unhashable pass must not be treated as a +# fresh-fingerprint match. Both fingerprints are the literal marker `unavailable`, and +# the guard against that is `[ "$h" != unavailable ] && [ "$h" = "$prev" ]`; a single +# unhashable pass can't distinguish it from the weaker `[ "$h" = "$prev" ]`, because +# `prev` is still empty on the first pass either way. Only a second consecutive +# unhashable pass gives `prev` the value `unavailable`, which is the only case where +# the two conditions disagree. +( PATH="$stub_dir:$PATH" rev ) +[ "$(cat "$fresh" 2>/dev/null || echo 0)" = 0 ] && pass "second consecutive unhashable pass still leaves freshCount 0" || fail "second consecutive unhashable pass still leaves freshCount 0" + +# 24c. seed-copy failure (stub cp) must fire, not silently hash an empty index +printf '#!/bin/sh\nexit 1\n' > "$stub_dir/cp"; chmod +x "$stub_dir/cp" +rm -f "$stub_dir/shasum" "$stub_dir/sha1sum" "$stub_dir/cksum" +reset_all; printf '1' > "$floorf" +( PATH="$stub_dir:$PATH" rev ) +printf '%s' "$(PATH="$stub_dir:$PATH" commitpre)" | grep -q 'not satisfied' \ + && pass "seed-copy failure -> not satisfied" || fail "seed-copy failure -> not satisfied" +rm -f "$stub_dir/cp" + +# 24d. a selective git wrapper that fails ONLY `diff` +cat > "$stub_dir/git" <<'STUB' +#!/bin/sh +for a in "$@"; do case "$a" in diff) exit 1 ;; esac; done +exec "$REAL_GIT" "$@" +STUB +chmod +x "$stub_dir/git" +REAL_GIT=$(command -v git); export REAL_GIT +reset_all; printf '1' > "$floorf" +( PATH="$stub_dir:$PATH" rev ) +printf '%s' "$(PATH="$stub_dir:$PATH" commitpre)" | grep -q 'not satisfied' \ + && pass "git diff failure -> not satisfied" || fail "git diff failure -> not satisfied" + +# 24e. a selective git wrapper that fails ONLY `rev-parse --absolute-git-dir` +cat > "$stub_dir/git" <<'STUB' +#!/bin/sh +case "$*" in *"--absolute-git-dir"*) exit 1 ;; esac +exec "$REAL_GIT" "$@" +STUB +chmod +x "$stub_dir/git" +reset_all; printf '1' > "$floorf" +( PATH="$stub_dir:$PATH" rev ) +printf '%s' "$(PATH="$stub_dir:$PATH" commitpre)" | grep -q 'not satisfied' \ + && pass "unresolvable git-dir -> not satisfied" || fail "unresolvable git-dir -> not satisfied" +rm -f "$stub_dir/git" +rm -rf "$stub_dir" # GAP 3 — stub_dir (mktemp -d) outlives its own guard otherwise +unset REAL_GIT # GAP 3 — exported at 24d, never unset otherwise +rm -f "$floorf" +reset_all; rev; rev; rev + +# 24f. Guard: confirm section 24's PATH containment held, and that REAL_GIT (exported +# at 24d for the selective git wrappers) was unset again. If a future edit +# reintroduces either leak (e.g. drops a subshell above, or a new `export REAL_GIT` +# without matching cleanup), this fails loudly instead of stale state silently +# weakening isolation in every later section — the same "guard the variable you +# happened to remember" gap this section's own comment warns about, closed for both +# variables instead of just PATH. +[ "$PATH" = "$path_before_24" ] && pass "PATH not leaked out of section 24" || fail "PATH not leaked out of section 24" +[ -z "${REAL_GIT+x}" ] && pass "REAL_GIT unset after section 24" || fail "REAL_GIT unset after section 24" +# ...and the stub directory itself. Guarding only the variables was the same partial +# fix again: dropping `rm -rf "$stub_dir"` left every assertion green, so the suite +# could leak a temp dir per run undetected. $work's EXIT trap does not cover it — +# stub_dir is its own mktemp -d outside that tree. +[ ! -d "$stub_dir" ] && pass "stub_dir removed after section 24" || fail "stub_dir removed after section 24" + +# 25. Unborn repo: no commits and no .git/index must still hash and self-match, or the +# first commit in a fresh repo STOPs forever. Spec §3 "But an absent index is not a +# failed copy". +unborn=$(mktemp -d) +( + cd "$unborn" || exit 1 + git init -q; git config user.email t@t; git config user.name t + mkdir -p .context; : > .context/codex-gate.on + printf '1' > .context/codex-gate.floor # one pass is enough for this fixture + printf 'x\n' > a.ts + R='{"hook_event_name":"PostToolUse","tool_name":"mcp__codex__review","tool_input":{}}' + printf '%s' "$R" | sh "$HOOK" >/dev/null + h1=$(cat .context/codex-gate.gateB 2>/dev/null) + printf '%s' "$R" | sh "$HOOK" >/dev/null + h2=$(cat .context/codex-gate.gateB 2>/dev/null) + [ -n "$h1" ] && [ "$h1" != unavailable ] && [ "$h1" = "$h2" ] || exit 1 + # ...and the FIRST commit must actually be able to reach satisfied. Hashing and + # self-matching is not enough: a consumer-side regression could still STOP every + # first commit forever, which is the failure this fixture exists to catch. + out=$(printf '%s' '{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"git commit --allow-empty -m init"}}' | sh "$HOOK") + printf '%s' "$out" | grep -q 'Gate B satisfied' || exit 1 +) && pass "unborn repo hashes, self-matches, and can reach satisfied" \ + || fail "unborn repo hashes, self-matches, and can reach satisfied" +rm -rf "$unborn" + +# 26. THE DEFECT (spec §1): staged content diverging from the worktree. +# NOTE: the revert is a direct byte write, NOT `git checkout -- app.ts` — checkout +# restores the worktree FROM THE INDEX, which would install v2 on disk and destroy +# the divergence, making this test pass against the unfixed hook. +reset_all +rev; rev; rev +printf '%s' "$(commitpre)" | grep -q 'Gate B satisfied' && pass "setup: satisfied on clean tree" || fail "setup: satisfied on clean tree" +printf 'v2\n' > app.ts; git add app.ts >/dev/null 2>&1 # index: v2 +printf 'v1\n' > app.ts # worktree: back to HEAD bytes +printf '%s' "$(commitpre)" | grep -q 'not satisfied' \ + && pass "staged-vs-worktree divergence -> NOT satisfied" \ + || fail "staged-vs-worktree divergence -> NOT satisfied" +git reset -q >/dev/null 2>&1; git checkout -- app.ts >/dev/null 2>&1 + +# 27. Ambient alternate index. Three shapes: a negative-only test would be satisfied by +# an implementation that fires whenever GIT_INDEX_FILE is set — a permanent STOP. +alt_dir=$(mktemp -d) +# 27a. divergent: fingerprint recorded WITHOUT the alternate index, alternate enabled +# only for the commit check. +reset_all; printf '1' > "$floorf" +rev +cp .git/index "$alt_dir/alt" +printf 'SNEAKY\n' > app.ts; GIT_INDEX_FILE="$alt_dir/alt" git add app.ts >/dev/null 2>&1 +printf 'v1\n' > app.ts +printf '%s' "$(GIT_INDEX_FILE="$alt_dir/alt" commitpre)" | grep -q 'not satisfied' \ + && pass "ambient divergent alternate index -> NOT satisfied" \ + || fail "ambient divergent alternate index -> NOT satisfied" +# 27b. stable: same unchanged alternate index across review AND commit -> satisfied, +# with each index file byte-identical to its OWN pre-hook snapshot. +cp .git/index "$alt_dir/default.before"; cp "$alt_dir/alt" "$alt_dir/alt.before" +reset_all; printf '1' > "$floorf" +# Subshell: a prefix assignment on a SHELL FUNCTION call (rev/commitpre are functions, +# not external commands) persists in the shell after the call returns — unlike the same +# prefix on an external command. Without containment, GIT_INDEX_FILE leaks out of +# section 27 and corrupts every later section's fixtures (notably section 28). +( GIT_INDEX_FILE="$alt_dir/alt" rev ) +printf '%s' "$(GIT_INDEX_FILE="$alt_dir/alt" commitpre)" | grep -q 'Gate B satisfied' \ + && pass "ambient stable alternate index -> satisfied" \ + || fail "ambient stable alternate index -> satisfied" +cmp -s .git/index "$alt_dir/default.before" && pass "default index untouched" || fail "default index untouched" +cmp -s "$alt_dir/alt" "$alt_dir/alt.before" && pass "alternate index untouched" || fail "alternate index untouched" +# 27c. missing path: git treats a nonexistent GIT_INDEX_FILE as an EMPTY index, so the +# hook must hash and self-match rather than returning `unavailable`. +reset_all; printf '1' > "$floorf" +( GIT_INDEX_FILE="$alt_dir/does-not-exist" rev ) +h1=$(cat "$state" 2>/dev/null) +( GIT_INDEX_FILE="$alt_dir/does-not-exist" rev ) +h2=$(cat "$state" 2>/dev/null) +{ [ -n "$h1" ] && [ "$h1" != unavailable ] && [ "$h1" = "$h2" ]; } \ + && pass "missing alternate index hashes and self-matches" \ + || fail "missing alternate index hashes and self-matches" +rm -rf "$alt_dir"; rm -f "$floorf" +git checkout -- app.ts >/dev/null 2>&1; git reset -q >/dev/null 2>&1 +reset_all; rev; rev; rev + +# 27d. Guard: confirm section 27's containment held. If a future edit reintroduces the +# leak (e.g. drops a subshell above), this fails loudly instead of section 28 quietly +# going vacuous against a stale, since-deleted GIT_INDEX_FILE path. +[ -z "${GIT_INDEX_FILE+x}" ] && pass "GIT_INDEX_FILE not leaked out of section 27" || fail "GIT_INDEX_FILE not leaked out of section 27" + +# 27e. FINDING 1 — a RELATIVE ambient GIT_INDEX_FILE must resolve against the +# repository TOP-LEVEL, exactly as git itself does — not against the hook's own +# cwd. The shell-side `[ ! -e ]` test and `cp` are plain shell commands (unlike +# every git call in the hook, which uses `-C "$repo_root"`), so an unnormalized +# relative path resolves differently depending on where the hook happens to run +# from. Run BOTH the review pass and the commit check from a SUBDIRECTORY with a +# relative alt index that actually lives at the repo root: an unnormalized hook +# can't find it either time, takes the same absent-index carve-out both times, and +# the two constant empty-tree hashes MATCH — a false "satisfied" even though the +# alt index stages content the worktree does not have. +# The alt index file lives under `.context/` — excluded from the diff-HEAD and +# worktree-tree components by their own `:(exclude).context` pathspec — so it is +# never picked up as an untracked file by `add -A` itself; the only way it can +# affect the hash is through eff_index resolution, which is exactly what this +# test needs to isolate. +reset_all; printf '1' > "$floorf" +mkdir -p sub +cp .git/index "$work/.context/rel-idx" # a copy of the CURRENT (matching) index +( cd sub && GIT_INDEX_FILE=.context/rel-idx rev ) # review, from a subdir, relative alt index +printf 'SNEAKY\n' > app.ts +GIT_INDEX_FILE="$work/.context/rel-idx" git add app.ts >/dev/null 2>&1 # stage into the ALT index only +printf 'v1\n' > app.ts # worktree stays at the reviewed bytes +out=$(cd sub && GIT_INDEX_FILE=.context/rel-idx commitpre) +printf '%s' "$out" | grep -q 'not satisfied' \ + && pass "relative ambient GIT_INDEX_FILE from a subdirectory -> NOT satisfied (Finding 1)" \ + || fail "relative ambient GIT_INDEX_FILE from a subdirectory -> NOT satisfied (Finding 1)" +rm -f "$work/.context/rel-idx"; rm -rf sub +git checkout -- app.ts >/dev/null 2>&1; git reset -q >/dev/null 2>&1 +reset_all; rev; rev; rev + +# 28. Tracked .context/ diverging THREE ways (index differs from both HEAD and worktree) +# is exactly the state where `git rm --cached` refuses without -f, silently (stderr +# is redirected). The hash must still be computable and self-match, and the user's +# real index must be untouched — the forced removal runs on the throwaway index only. +git add -f .context >/dev/null 2>&1; git commit -qm "track .context" >/dev/null 2>&1 +printf 'staged\n' > .context/codex-gate.on; git add .context/codex-gate.on >/dev/null 2>&1 +printf 'worktree\n' > .context/codex-gate.on +before_tree=$(git write-tree 2>/dev/null) +before_blob=$(git rev-parse :.context/codex-gate.on 2>/dev/null) +# Compare the index's ENTRIES, not its bytes. An earlier version of this test ran +# `cmp` on .git/index and passed on macOS while failing on Linux CI: git rewrites the +# index's stat cache during ordinary read-only operations, and the hook runs +# `git diff HEAD`, so byte-identity is a property the hook neither guarantees nor +# claims. `ls-files --stage` covers mode, object id, stage and path for EVERY entry, +# which is the property actually asserted — the hook must not change what the user's +# index means — and it is immune to benign stat-cache rewrites. +before_stage=$(git ls-files --stage 2>/dev/null) +reset_all +rev; h1=$(cat "$state" 2>/dev/null) +rev; h2=$(cat "$state" 2>/dev/null) +{ [ -n "$h1" ] && [ "$h1" != unavailable ] && [ "$h1" = "$h2" ]; } \ + && pass "three-way .context divergence hashes and self-matches" \ + || fail "three-way .context divergence hashes and self-matches" +[ "$(git write-tree 2>/dev/null)" = "$before_tree" ] && pass "real index tree unchanged" || fail "real index tree unchanged" +[ "$(git rev-parse :.context/codex-gate.on 2>/dev/null)" = "$before_blob" ] && pass "staged .context blob unchanged" || fail "staged .context blob unchanged" +[ "$(git ls-files --stage 2>/dev/null)" = "$before_stage" ] \ + && pass "real index entries unchanged" || fail "real index entries unchanged" +: > .context/codex-gate.on +git rm -rq --cached .context >/dev/null 2>&1; git commit -qm "untrack .context" >/dev/null 2>&1 +reset_all; rev; rev; rev + +# 29. Message contracts (spec §4). These are shipped prompts — the product itself, not +# incidental output — so each branch's COMPLETE additionalContext and systemMessage +# is pinned as a golden fixture and compared EXACTLY, replacing the old per-clause +# greps: a clause list only catches a regression someone thought to enumerate +# (inserting "do not " before an asserted clause, or "Usually" -> "Always", stayed +# green under it); an exact comparison catches any wording change. $passes, $floor +# and $fresh are pinned by the setup immediately before each assertion, so every +# fixture below is deterministic. +json_field() { # $1 = json text, $2 = key -> prints the string value. No jq required — + # same fallback idiom as the hook's own field(): safe here because none + # of the three golden messages below contain a literal backslash or quote. + printf '%s' "$1" | grep -o "\"$2\"[[:space:]]*:[[:space:]]*\"[^\"]*\"" | head -n1 | sed 's/^.*:[[:space:]]*"\(.*\)"$/\1/' +} + +# 29a. STALE branch: 1 recorded pass this cycle, default floor 3, no CLAUDE.md (so the +# generic policy phrase), a change made through the Edit tool. +# A `rev` baseline is required before the edit: without one, `reviewed` is empty and +# the edit below lands in the EMPTY-STATE branch, not the STALE branch this fixture +# describes (matches the existing pattern in section 3a). +reset_all; rev +printf 'edit\n' >> app.ts +run '{"hook_event_name":"PostToolUse","tool_name":"Edit","tool_input":{"file_path":"app.ts"}}' >/dev/null +out=$(commitpre) +ctx=$(json_field "$out" additionalContext) +msg=$(json_field "$out" systemMessage) +expected_ctx="STOP — Codex Gate B not satisfied: the hook cannot confirm that the content you are about to commit is the content mcp__codex__review last saw (1 recorded pass(es) this cycle). Usually that means the working tree or the index changed since the review. It can also mean you only staged already-reviewed content — the bytes are fine, but the hook cannot tell staging from editing; that this hook was upgraded and the recorded fingerprint uses the older format (see CHANGELOG); or that the fresh fingerprint could not be computed or could not be stored. Run Gate B (mcp__codex__review) now — one clean pass is the complete remedy for the staging and post-upgrade cases too. If a fresh pass leaves this unchanged with nothing edited in between, the fault is in the machinery rather than the code: check that .context/ is writable, that TMPDIR is writable, that a checksum tool (shasum, sha1sum or cksum) runs, that git status works, and that the disk is not full — then run one more pass to record a usable fingerprint. Per this project's review policy you MUST re-review after every fix." +expected_msg="⚠ Codex Gate B not satisfied (cannot confirm review)" +[ "$ctx" = "$expected_ctx" ] && pass "stale additionalContext matches exactly" || fail "stale additionalContext matches exactly" +[ "$msg" = "$expected_msg" ] && pass "stale systemMessage matches exactly" || fail "stale systemMessage matches exactly" +git checkout -- app.ts >/dev/null 2>&1 + +# 29b. SATISFIED branch: 3/3 passes this cycle, all 3 fresh (unchanged tree). The hook +# fingerprints disk; mcp__codex__review reads a git range (spec §7) — the exact +# fixture below is what pins that the message never claims Codex read the bytes. +reset_all; rev; rev; rev +out=$(commitpre) +ctx=$(json_field "$out" additionalContext) +msg=$(json_field "$out" systemMessage) +expected_ctx="Codex Gate B: 3/3 pass(es) this cycle, of which 3 cover the CURRENT content fingerprint (unchanged since that review). The floor counts the cycle; only the fresh pass(es) carry the same fingerprint as what you are committing. Per this project's review policy, commit only if your final pass was clean — no new Blocker/Major." +expected_msg="✓ Codex Gate B satisfied (3/3 cycle, 3 on current fingerprint)" +[ "$ctx" = "$expected_ctx" ] && pass "satisfied additionalContext matches exactly" || fail "satisfied additionalContext matches exactly" +[ "$msg" = "$expected_msg" ] && pass "satisfied systemMessage matches exactly" || fail "satisfied systemMessage matches exactly" + +# 29c. EMPTY-STATE branch: no fingerprint recorded this cycle, default floor 3. +reset_all +out=$(commitpre) +ctx=$(json_field "$out" additionalContext) +msg=$(json_field "$out" systemMessage) +expected_ctx="STOP — Codex Gate B not satisfied: no fingerprint is recorded for this cycle — either no mcp__codex__review has run, or the last one's fingerprint could not be written or read back. Per this project's review policy you MUST reach a minimum of 3 passes per cycle. Run Gate B (mcp__codex__review) now; if this repeats, check that .context/ and the state file inside it are readable and writable, and if the file exists but is unreadable or empty, delete it and run a fresh pass." +expected_msg="⚠ Codex Gate B: no recorded review" +[ "$ctx" = "$expected_ctx" ] && pass "empty-state additionalContext matches exactly" || fail "empty-state additionalContext matches exactly" +[ "$msg" = "$expected_msg" ] && pass "empty-state systemMessage matches exactly" || fail "empty-state systemMessage matches exactly" +reset_all; rev; rev; rev + +# 30. UNSTORABLE STATE (spec §5 test 11). Cause 5 lives OUTSIDE tree_hash: the store path +# writes with `2>/dev/null || true` because the hook must always exit 0, so a pass can +# hash perfectly and still leave the old fingerprint behind. Three shapes, because +# they reach the branch by different routes. +# Each shape depends on chmod actually DENYING access, which is false under an +# effective root uid — a root-run CI job would take the success path and then fail the +# assertion, reporting a product defect that is really a privilege artefact. Probe the +# exact operation each shape needs, restore what the probe changed, and skip loudly. +probe_denies() { # $1 = probe command; returns 0 when the operation was DENIED + ( eval "$1" ) >/dev/null 2>&1 && return 1 || return 0 +} +skip() { printf 'skip - %s\n' "$1"; } + +# 30a. replacement fails: the state file itself is read-only +reset_all; rev +before=$(cat "$state") +chmod 0444 "$state" 2>/dev/null +if probe_denies "printf x >> \"$state\""; then + printf 'later edit\n' >> app.ts + rev # hashes fine, but cannot replace the fingerprint + [ "$(cat "$state")" = "$before" ] && pass "unwritable state file keeps the old fingerprint" || fail "unwritable state file keeps the old fingerprint" + printf '%s' "$(commitpre)" | grep -q 'cannot confirm' \ + && pass "stale fingerprint after a failed store -> cannot confirm" \ + || fail "stale fingerprint after a failed store -> cannot confirm" +else + skip "unwritable state file (chmod does not deny writes here — running as root?)" +fi +chmod 0644 "$state" 2>/dev/null +git checkout -- app.ts >/dev/null 2>&1 + +# 30b. first write fails: no prior state, and the directory refuses a new file +reset_all +chmod 0555 .context 2>/dev/null +if probe_denies "touch .context/probe-$$"; then + rev # cannot create the state file at all + out=$(commitpre) + printf '%s' "$out" | grep -qF 'no fingerprint is recorded' \ + && pass "unwritable .context -> empty-state branch" || fail "unwritable .context -> empty-state branch" + printf '%s' "$out" | grep -qF 'could not be written or read back' \ + && pass "empty-state branch admits a failed first write" || fail "empty-state branch admits a failed first write" +else + skip "unwritable .context directory (chmod does not deny creation here — running as root?)" +fi +chmod 0755 .context 2>/dev/null; rm -f ".context/probe-$$" + +# 30c. read fails: the state file exists and is non-empty but cannot be read +reset_all; rev +chmod 0000 "$state" 2>/dev/null +if probe_denies "cat \"$state\""; then + out=$(commitpre) + printf '%s' "$out" | grep -qF 'no fingerprint is recorded' \ + && pass "unreadable state file -> empty-state branch" || fail "unreadable state file -> empty-state branch" + # invariant 1: advisory hook, always exit 0, even when its own state is unreadable. + # Checked with `if`, not `[ $? -eq 0 ]` — the latter is SC2181 and the test file + # excludes only SC2015, so it would fail lint. + if run '{"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":"git commit -m x"}}' >/dev/null 2>&1; then + pass "unreadable state file -> hook still exits 0" + else + fail "unreadable state file -> hook still exits 0" + fi +else + skip "unreadable state file (chmod does not deny reads here — running as root?)" +fi +chmod 0644 "$state" 2>/dev/null +reset_all; rev; rev; rev + echo "---" [ "$fails" -eq 0 ] && { echo "all passed"; exit 0; } || { echo "$fails failed"; exit 1; } diff --git a/todos.md b/todos.md index 3961512..51d2951 100644 --- a/todos.md +++ b/todos.md @@ -24,16 +24,6 @@ driven by recurrence rather than by enthusiasm. ### Parked (trigger-gated) -- [ ] **Gate-B hash misses staged-vs-worktree divergence (false ✓, invariant 3).** - Both hash components describe the worktree, but `git commit` commits the INDEX. - Repro: `git add app.ts` with new content, then revert `app.ts` on disk to HEAD — - the commit carries the staged content while the hash reads the tree as unchanged, - so Gate B reports satisfied on unreviewed content. Pre-existing, not introduced by - the write-tree change. Fix means hashing the index tree as a third component - (`git rm --cached -r .context` on the temp index first, or the tracked adoption - marker re-invalidates forever) — which also makes a bare `git add` invalidate a - review, so the existing "add does not change the hash" test has to be re-decided. - Own branch, own tests. - [ ] **jq-free parser stops at an escaped JSON quote.** A payload containing `echo \"quoted\" && git commit -m x` decodes to nothing, so no reminder fires — wrong direction under invariant 2, and only on machines without `jq`. Needs @@ -43,12 +33,49 @@ driven by recurrence rather than by enthusiasm. && git commit -am x` is one PreToolUse event: the hook hashes before the mutation runs, so the commit carries content the hash never saw. Consider treating any command segment preceding `git commit` as uncertain and firing. -- [ ] **No regression test for tree_hash's "tree unavailable" guard.** The guard emits - a never-matching value when `mktemp`/`git add`/`write-tree` fails, so the gate - reads unreviewed instead of collapsing to a constant. Testing it needs a portable - way to make those fail on demand — `TMPDIR=/dev/null` is not one (BSD/macOS - `mktemp` falls back to `/var/folders`, so the test would pass for the wrong - reason). Consider a stub `git` earlier on `PATH`. +- [ ] **No regression test for a `git add`/`write-tree` failure inside the throwaway + index.** Derived from the code, not recalled: sections 24a-24e stub FIVE failure shapes — + every checksum tool failing silently, a checksum printing a token then failing, the + seed `cp`, `git diff HEAD`, and `rev-parse --absolute-git-dir`. SEVEN have no + targeted test: `mktemp -d`; the non-symbolic unresolvable-HEAD branch (the + `else ok=0` arm); the throwaway-index `git rm -rfq --cached`; the first + `write-tree` (index tree); `git add -A`; the second `write-tree` (worktree tree); + and failure of the redirect that creates the buffered stream. The two `write-tree` + calls are distinct sites needing distinct tests — one covers the index component, + the other the worktree component. Each needs a portable way to fail exactly one + call without disturbing the rest; a selective `git` wrapper earlier on `PATH` (as + 24d/24e already use) is the seam for the git ones. This row was written three + times from memory and understated the gap every time — re-derive from the code + before trusting it. +- [ ] **Gate-B fingerprints disk; the reviewer reads history.** A review pass records a + fingerprint of the index and worktree, but `mcp__codex__review` reads a **git + range** — so content that is staged and never committed can be fingerprinted as + reviewed without Codex having read it, and three such passes reach ✓. Raised at + Gate A pass 8 of the index-tree story and deliberately deferred there: closing it + means refusing to satisfy Gate B unless the index and worktree correspond to the + reviewed range, i.e. mandating a WIP commit for every review. That redefines the + gate rather than fixing a hash, so it needs its own story and its own decision. + CLAUDE.md §5's WIP-commit flow is the current mitigation. +- [ ] **`check-invariants.sh` scans untracked scratch directories, so local scratch can + fail it.** It greps the working tree recursively, not the tracked set, so a + gitignored scratch file that merely *quotes* a violating pattern trips it. Hit for + real: the SDD scratch under `.superpowers/` held pasted test output in which the + word `npx` sat next to a `--yes` flag inside one of the checker's OWN test + descriptions, and the checker then reported an unpinned-npx violation against a + repo containing no such call. (This row deliberately does not quote that string + verbatim — doing so put the pattern into a tracked file and made the checker fail + on this very commit, which is the bug demonstrating itself.) A false positive, so + it is the safe direction — but it is confusing, and it makes "the battery is + green" depend on what else happens to be on disk. Surfaced by real use during the + index-tree story, not by a gate. Fix would be to scan tracked files (or honour + `.gitignore`), with a reject/accept fixture for a violating pattern inside an + ignored path. + **Occurrence 2 (candidate note, not a fix): recurring operator friction.** Closing + that same story, the battery had to be run with `.superpowers/sdd/` moved aside + *again* — by hand, remembered rather than prompted. So this is not only a + confusing one-off red: it is a step a human must know about and repeat, on a + command AGENTS.md presents as "what CI runs". Two occurrences of the same class + now; counts toward whatever trigger this row is eventually escalated on. - [ ] **Temp-index writes land in the real object database.** `git add -A` against the throwaway index writes loose blobs/trees into the user's repo (verified: 3 → 5 objects per review). Unreachable, so gc collects them, but a temporary