Gate-B fingerprint covers the index, not just the worktree (0.5.0)#8
Merged
Conversation
Spec: Gate A clean at pass 15 (~108 findings; the command-retargeting guard was split to its own story at pass 13). Plan: Gate A clean at pass 3.
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.
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.
A prefix assignment on a SHELL FUNCTION call (rev/commitpre) persists in the shell after the call returns, unlike the same prefix on an external command. GIT_INDEX_FILE leaked out of section 27 and corrupted section 28s three-way .context divergence fixture, silently defeating its guard: mutating the hooks git rm -rfq --cached to -rq left the suite fully green. Fixes: wrap the three leaking calls in a subshell so the assignment cannot escape; add a guard assertion before section 28 asserting GIT_INDEX_FILE is unset, so a future regression fails loudly; fix a related snapshot-ordering flakiness in section 28s real index bytes unchanged assertion (index snapshot was taken before two read-only git commands that can trigger a racy-clean stat-cache rewrite). Updated the plan (Task 2 Step 2, section 27) to match and to name the trap.
The shell-side eff_index test/cp resolved a relative GIT_INDEX_FILE against the hook cwd, while every git call used -C repo_root; git itself resolves it against the top-level. From a subdirectory this silently took the absent-index carve-out and hashed a constant empty tree, a false Gate B satisfied. Normalize eff_index against repo_root. Also contains five PATH=... rev leaks in codex-gate.test.sh section 24 (prefix assignment on a shell function persists after return in POSIX sh, same class as the GIT_INDEX_FILE leak fixed earlier for section 27) and adds a guard asserting PATH is restored after the section.
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.
…ality and systemMessage clauses in Gate B tests Finding 1: the empty-state branch carried two overlapping comments (old + new); merged into one that attributes the state to an absent fingerprint. Finding 2: the satisfied-message test asserted only the bare word fingerprint; now pins the actual equality clause plus its other required clauses. Finding 3: the stale-message test never pinned the systemMessage; added an exact-match assertion and an absence check for the old causal label. Plan doc corrected to match.
…ison Per-clause greps only catch a regression someone thought to enumerate; a meaning-reversing edit (inserting "do not ", swapping "Usually" for "Always", appending an unasserted causal claim) stayed green. Compare the complete additionalContext and systemMessage against a fixture instead, with $passes/$floor/$fresh pinned deterministically per branch. Also qualifies the empty-state comment: a failed REPLACEMENT write preserves the older fingerprint and lands in the stale branch, not this one - only a first write with nothing to fall back to reaches empty-state. Plan doc updated to match the implemented test code.
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 4's own comment block still said "BOTH components" and enumerated only the tracked diff and worktree tree, though the fingerprint now has three (Gate B finding, spec+quality passes both flagged it).
Gate B (pass 2, both spec and quality) flagged "what a commit would actually carry" as an overclaim: spec sec:2 already narrows this to "the index as it stands when the hook is invoked" and calls out the compound-command mutation as a separate, parked defect. Apply that same scoping everywhere the unqualified claim was made.
Gate B found the same unverified-enforcement-claim that Task 3 removed from the satisfied message: "any change by any tool invalidates" is false (.context/ and ignored paths are excluded, post-invocation mutations are unseen), and "what is being committed is what was reviewed" claims the hook cannot support - it compares a fingerprint of disk while the reviewer reads a git range. Also reflows an AGENTS.md line break that split an inline code span.
That phrasing was my own replacement for an earlier overclaim, and it is one too. The fingerprint covers the effective index PLUS untracked and unstaged worktree content; a plain git commit carries only the index. It is a deliberate superset so the gate errs toward firing, and the prose now says that at all three sites (README, the hook header, architecture.md). Also: only UNTRACKED ignored paths are excluded - a tracked file counts even if it matches .gitignore - and getting-started no longer promises that any file change invalidates.
Gate B found the same overclaim in two spellings my greps had missed: "everything a commit could carry" (AGENTS.md invariant 3) and "everything that could end up in a commit" (the hash comment). Both now use the canonical wording. Fourth round of this defect on one task, and the cause each time was searching for a phrase rather than the claim: I grepped "what a commit would carry" and missed two synonyms. Found this round by sweeping for totality words near commit/fingerprint/hash instead.
getting-started.md showed "3 on current code"; the hook emits "3 on current fingerprint" since Task 3 renamed it. A doc quoting output the tool does not produce is worse than vague prose - a reader compares it against their terminal. Also retires the last comments equating fingerprint coverage with commit payload. Not changed: the shipped satisfied message. Its "(unchanged since that review)" modifies the FINGERPRINT, which is exactly what equality proves, and the text is pinned in the approved spec and its golden-fixture test.
The fresh count tracks passes carrying the CURRENT fingerprint; it does not establish that Codex read those bytes. Phrase now gone repo-wide.
…im rule Gate B on Task 6 found both. The narrowed tree-unavailable row and the ledger row listed only git add/write-tree as untested seams; mktemp -d and the throwaway-index git rm --cached are untested too. Both now list all four. The recurrence row used rung=pending with a ref pointing at no todos.md item, breaking the ledger contract. The rung is now real: a Dont in AGENTS.md, sibling to the existing manifest-claims rule, pairing a stated rule with a by-meaning grep recipe and saying plainly that nothing runs it in CI. Also records a checker false positive found by real use: check-invariants.sh greps the working tree, so gitignored scratch quoting a violating pattern fails it. Writing that row literally reproduced the bug, which is noted in the row.
Final whole-branch review, which mutation-tested the hook and found two mutants surviving. Finding 1 (Important): the approved spec pinned a tree_hash() snippet that omits the relative-GIT_INDEX_FILE normalization added later at Gate B (92a23f0). That is CLAUDE.md 5s "a fix that changes specified behaviour updates the spec in the same commit" - the docs-drift class this repo records against itself - so the snippet and its rationale are now in the spec. The untested-seam list is rewritten by DERIVING it from the code: four seams have stubs, seven do not, and the two write-tree calls are distinct sites. Three earlier versions enumerated it from memory and understated it every time; the row now says so. The ledger also records that only the -f flag of the .context removal is mutation-covered, not the removal itself. Nits: architecture.md no longer cites a hardcoded hook line number (it had already drifted), and getting-started no longer stacks two parentheticals.
…ontext exclusion, the inert unavailable guard on a second unhashable pass, and section 24 state leaks (stub_dir, REAL_GIT)
Final Gate B. The state-leak fix guarded PATH and REAL_GIT but not the temp directory itself - dropping `rm -rf "$stub_dir"` left every assertion green, the same partial fix one more time. Now mutation-verified. The ledger row still said the index-component .context exclusion was untested; a85395f added that test, so the record was stale within its own task. The seam inventory said FOUR tested shapes while naming five. Not changed: invariant 12. Gate B reported it violated, but it diffed against an intermediate commit; CI runs check-version-bump on pull_request with the PRs base_ref, and against the real merge-base with main the check passes.
…tion Same class, second occurrence: closing this story the battery again had to be run with .superpowers/sdd/ moved aside by hand. Not a one-off confusing red but a remembered manual step on a command AGENTS.md presents as what CI runs. Recorded as a candidate note toward the rows eventual escalation trigger, not acted on - the row stays parked.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughGate-B now fingerprints the effective index and included worktree content, fails closed when hashing is unavailable, detects staged/worktree divergence, updates messages and tests, and aligns specifications, documentation, plugin metadata, and backlog records. ChangesGate-B index-tree fingerprinting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Hook as codex-gate.sh
participant Git
participant Index as Throwaway index
participant State as Gate-B state
Hook->>Git: Resolve effective index and repository data
Hook->>Index: Build index and worktree trees
Index->>Git: Write tree identifiers
Hook->>State: Compare fingerprint and update freshness
State-->>Hook: Emit satisfied or unconfirmed message
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/architecture.md (1)
76-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix hard line wrap.
The sentence is prematurely wrapped, leaving a very short line for
dangerous direction,. Reflowing the paragraph would improve readability.♻️ Proposed fix
-(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 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🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` around lines 76 - 78, Reflow the paragraph in docs/architecture.md around “A false ✓ is the dangerous direction” so the sentence is wrapped at a more natural width, avoiding the short standalone line while preserving the wording and meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 206: Update the reverse-order regex alternation in the documented grep
command to include “any change” alongside “everything,” “anything,” and “all
content,” preserving the existing forward-order patterns and exclusions.
In `@docs/superpowers/plans/2026-07-19-gate-b-index-tree.md`:
- Around line 517-518: Update the documented test checks around the
codex-gate.test.sh invocations, including the checks at the referenced later
sections, to capture the runner’s exit status separately from the grep failure
count. Assert that the script exits successfully and that the count of lines
matching ^FAIL is zero, rather than relying on the grep pipeline’s status.
---
Nitpick comments:
In `@docs/architecture.md`:
- Around line 76-78: Reflow the paragraph in docs/architecture.md around “A
false ✓ is the dangerous direction” so the sentence is wrapped at a more natural
width, avoiding the short standalone line while preserving the wording and
meaning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b83a610-ef91-430c-9199-4fc011f30f17
📒 Files selected for processing (15)
AGENTS.mdREADME.mddocs/architecture.mddocs/getting-started.mddocs/hardening-log.mddocs/superpowers/plans/2026-07-19-gate-b-index-tree.mddocs/superpowers/specs/2026-07-19-gate-b-index-tree-design.mddocs/superpowers/stories/2026-07-18-gate-b-hash-staged-worktree-divergence-story.mddocs/superpowers/stories/2026-07-19-command-retargeting-guard-story.mdplugins/dev-workflow/.claude-plugin/plugin.jsonplugins/dev-workflow/CHANGELOG.mdplugins/dev-workflow/commands/workflow-init.mdplugins/dev-workflow/hooks/codex-gate.shplugins/dev-workflow/hooks/codex-gate.test.shtodos.md
CI caught what macOS hid: `cmp` on .git/index failed on Linux. Byte-identity is not a property the hook guarantees - git rewrites the index stat cache during ordinary read-only operations, and the hook runs `git diff HEAD`. The earlier comment blamed placement, but moving the snapshot could not fix it either. `git ls-files --stage` compares mode, object id, stage and path for every entry, which is the property actually claimed - the hook must not change what the users index MEANS - and it is immune to benign stat-cache rewrites. Strictly stronger than the byte check on entry coverage. Mutation-verified: dropping GIT_INDEX_FILE from the hooks `git rm --cached` so it hits the real index turns it red.
Both verified by direct probe, by me rather than a triage subagent: this PR touches AGENTS.md, plugins/ and commands/, so process-pr-reviews instruction-path precheck skips delegation. 1. The overclaim grep recipe was asymmetric - its reverse-order arm omitted `any change`, so "the fingerprint guarantees any change" escaped. Probed before and after; now caught. This strengthens the rung recorded in the 2026-07-19 unverified-enforcement-claim row rather than needing a new one. 2. `... | grep -cE "^FAIL"` reports the greps status, not the test runners, so a suite that died part-way would read as 0 failures and be logged green - in the steps used to prove this branch guards were load-bearing. All three sites now capture exit= separately and assert both. New ledger row.
Greptile SummaryThis PR closes the Gate-B false-positive where
Confidence Score: 4/5The hook implementation is correct and safe to merge; the only concern is a stale grep pattern in the test suite that fails on non-GNU-grep systems. The core implementation is sound: three-component buffered stream, unavailable fail-closed contract, relative GIT_INDEX_FILE normalisation, and -f flag for three-way divergence are all correctly written and mutation-verified. The single finding is that a grep pattern in test 18 had its first alternative rendered stale by this PR's message rewording, leaving the test dependent on a GNU grep BRE extension on non-Linux systems. The hook runtime itself is unaffected. plugins/dev-workflow/hooks/codex-gate.test.sh line 415 — grep pattern for the Finding 9 fresh-count assertion needs its first alternative updated to the new message wording. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant R as PostToolUse review
participant T as tree_hash
participant S as gateB state file
participant P as PreToolUse commit
R->>T: compute fingerprint
T->>T: git diff HEAD tracked diff
T->>T: cp eff_index to tmp_index
T->>T: git rm --cached -f .context
T->>T: git write-tree INDEX tree SHA
T->>T: git add -A excluding .context
T->>T: git write-tree WORKTREE tree SHA
T->>T: checksum buffered stream
T-->>R: h or unavailable
R->>S: store h update freshCount
P->>S: read reviewed fingerprint
P->>T: compute current fingerprint
T-->>P: current or unavailable
alt current or reviewed is unavailable
P-->>P: STOP cannot confirm
else reviewed not equal current
P-->>P: STOP cannot confirm
else passes below floor
P-->>P: WARN below floor
else all checks pass
P-->>P: Gate B satisfied
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant R as PostToolUse review
participant T as tree_hash
participant S as gateB state file
participant P as PreToolUse commit
R->>T: compute fingerprint
T->>T: git diff HEAD tracked diff
T->>T: cp eff_index to tmp_index
T->>T: git rm --cached -f .context
T->>T: git write-tree INDEX tree SHA
T->>T: git add -A excluding .context
T->>T: git write-tree WORKTREE tree SHA
T->>T: checksum buffered stream
T-->>R: h or unavailable
R->>S: store h update freshCount
P->>S: read reviewed fingerprint
P->>T: compute current fingerprint
T-->>P: current or unavailable
alt current or reviewed is unavailable
P-->>P: STOP cannot confirm
else reviewed not equal current
P-->>P: STOP cannot confirm
else passes below floor
P-->>P: WARN below floor
else all checks pass
P-->>P: Gate B satisfied
end
|
All three valid, all mine. 1. The grep recipe false-positived on its own repo: `commit` matches `committed`, so an unrelated adoption comment hit. Word boundaries added, with a note saying why they are load-bearing. 2. The three verification sites shared a fixed /tmp/suite.out, so concurrent runs could read each others results. Per-run mktemp now, and the command RETURNS success only when the runner status and the FAIL count are both zero rather than printing them to eyeball. Verified negatively too: a runner that exits 3 without printing FAIL now reports failure, where the old idiom said green. 3. The ledger row claimed those commands assert both values when they only printed them - an unverified-enforcement-claim inside the row recording that very class. The commands now do assert, so the claim is true rather than softened, and the row records the miss.
Greptile, PR #8: the Finding-9 assertion at test.sh:415 had a stale first alternative - this PRs message rewrite renamed "cover the CURRENT tree" to "CURRENT content fingerprint", so it matched nothing and the assertion rested entirely on `\|`, which is alternation only under GNU-style BRE and literal under POSIX. It would have failed on a stock BSD grep (invariant 4: machines we do not control). Now a -F match on the real wording. Notable: this passed locally only because this machines `grep` is ugrep 7.5.0, not BSD grep - so earlier BSD-grep conclusions in this session were made on the wrong implementation. CI (GNU) hid it too. Three pre-existing BRE alternations in the same file converted to -E. Small, local to a file this PR already touches. Also Gate B MINOR: the plans Expected: lines still described count-only output after the commands began asserting both values.
Owner
Author
|
Greptile: accepted and fixed — good catch, and it was hiding behind two layers.
Two reasons it stayed invisible:
Fixed by pinning the real wording with Validated by me directly rather than by a triage subagent: this PR touches Also in the same commit, from Gate B: the plan's Suite: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the Gate-B false-positive recorded in
todos.md:git commitcommits the index, but every fingerprint component described the worktree. Staging a change and then reverting the file on disk left the fingerprint unchanged, so the gate reported "satisfied" for content nobody reviewed.Verified before the fix, in a throwaway repo:
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 updating reports "cannot confirm". A single review pass clears it.
This is expected, not a bug — please don't read it as the hook having broken. (A
WIP:commit or a docs-only commit bypasses the comparison and is unaffected.) Also inCHANGELOG.mdunder 0.5.0.Staging now invalidates a review.
git addof already-reviewed content changes the index tree, so the fingerprint changes. The committed bytes are unchanged, making this a false invalidation — accepted under invariant 2 ("loose in the firing direction"), and the reminder says staging alone can cause it. One clean pass clears it. This was a real decision, recorded in the spec: the previous behaviour was asserted by a test as though it were a principle, but it had only ever fallen out of an implementation choice.What changed
GIT_INDEX_FILEwhen set, else the git-dir index) beforegit add -Abrings the throwaway index up to the worktree. A relativeGIT_INDEX_FILEis normalized against$repo_root, because git resolves it against the repo top-level while the hook'scpresolves against its own cwd.date+PID nonce that could collide under PID reuse and report satisfied.0.5.0; two hardening-ledger rows.Testing
180 assertions (was 145). New coverage: the divergence repro, ambient alternate-index shapes, three-way-divergent tracked
.context, and fault injection for checksum, seed-copy, git-dir andgit difffailures — each fault held across both the stored and the recomputed fingerprint, since a fault applied only at commit time proves nothing.Load-bearing guards are mutation-verified: dropping the index
write-tree, the-fongit rm --cached, the.contextexclusion, the relative-index normalization, or theunavailableguard each turns a specific test red.Known gaps, parked rather than hidden
tree_hash's failure seams have no targeted test (mktemp -d, the non-symbolic unresolvable-HEAD branch, the throwaway-indexgit rm --cached, eachwrite-treeseparately,git add -A, the buffered-stream redirect). Enumerated by derivation from the code — three earlier attempts understated it from memory.GIT_INDEX_FILE=,-C,--git-dir, …) can still commit content the hook never fingerprinted. Split to its own story with default-deny as the settled approach, after five successive blocklist bypasses showed that enumerating spellings loses.mcp__codex__reviewreads a git range, so staged-but-never-committed content can be fingerprinted as reviewed. Pre-existing; closing it would mandate a WIP commit per review.Review record
Gate A: 15 passes on the spec, 3 on the plan. Gate B: every code commit, 14 rounds. Plus a whole-branch review that mutation-tested the hook (7 experiments, 2 survivors, both fixed). Final pass clean on both tracks.
Spec:
docs/superpowers/specs/2026-07-19-gate-b-index-tree-design.md· Plan:docs/superpowers/plans/2026-07-19-gate-b-index-tree.mdSummary by CodeRabbit
New Features
Documentation
Chores