gate: fix all three review findings on the two board gates - #1170
Merged
Conversation
The first PROTECTED tuple swapped AGENT_LOG.md in for IDEAS.md and kept the count at eight — which is exactly why it read as correct. The canonical eight are the ones .claude/BOOT.md's immutability table names and .claude/settings.json denies Edit/Write/MultiEdit on, and IDEAS.md is among them: a real 1204-line append-only ledger. The .claude/board/** path filter started the workflow on any PR that truncated it while check() never looked at it. Now nine: the canonical eight plus AGENT_LOG.md, which CLAUDE.md's one-writer rule also calls append-only. Adding it was never the error; substituting it was. Fire-tested: IDEAS.md 1204 -> 1150 lines gives exit 1 naming the file; restored, 9/9 ok. Found by review, not by the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv
The added-lines scoping was structurally blind to the gate's own motivating case. A prepend to EPIPHANIES.md moves targets under citations that live in other, UNCHANGED files on UNCHANGED lines, so filtering findings to the lines a PR added reported success on exactly the change the gate exists to catch. Confirmed, not theoretical. Now: scan the corpus at merge-base(BASE, HEAD) and at HEAD, key each citation by its own content plus an occurrence index (never by line number -- the citing line shifts too), and fail only where a citation is decayed now and was not decayed at the base. That catches both a target moving under an old citation and a newly added wrong one, while the 148-decay backlog stays reported and non-failing. The base revision is read through a detached worktree at the merge-base sha, removed in a finally; the checked-out tree is never touched. Any git failure fails closed. Also kills the workflow's "|| true" on the file-list diff, which turned an unresolvable merge base into an empty list and a green run -- bypassing the script's own fail-closed handling. There is no file list any more. Two defects found while verifying rather than trusting: the self-test printed the harness's own pass/fail under a label reading "expect 1", so a PASSING run printed 0 beside the word "expect 1" and asserted nothing about the real exit path. It now asserts the verdict the GATE would return, and that assertion fires under the second disable, so it is load-bearing rather than decoration. Disable-verified twice, red-then-green, by the orchestrator and not only by the worker: ignoring base verdicts turns the backlog case into a false new decay (MUST-STAY-SILENT red); never marking anything new kills the CAN-FIRE half. Real-repo checks: --since HEAD~1 gives 0 new / 148 pre-existing / 0 fixed, no leftover worktree, and an unresolvable base exits 1 with the fetch-depth hint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6ef76953-3e12-47fd-baf0-926f8fbb71a9) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10cb920d53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+316
to
+318
| base_key = (rel, f.cited, f.start, f.end) | ||
| occ[base_key] = occ.get(base_key, -1) + 1 | ||
| out[base_key + (occ[base_key],)] = f |
There was a problem hiding this comment.
When one file contains multiple citations to the same path and span but with different anchors, pairing them solely by occurrence order is unstable under the board's normal prepend workflow. For example, if the base contains an existing decayed B.md:3 citation and HEAD prepends a new valid B.md:3 citation, the valid citation is paired with the old decay and reported as fixed, while the unchanged old citation receives a new occurrence index and is reported as a new decay, incorrectly failing CI. This pattern already occurs in several corpus files, so the comparison key needs to incorporate the recovered anchor (or otherwise match citation content independently of position).
Useful? React with 👍 / 👎.
AdaWorldAPI
added a commit
that referenced
this pull request
Sep 4, 2026
Records the three review findings on the two board gates, and the uncomfortable half: #1167, #1168 and #1170 each shipped without their board hygiene in-commit -- three times running, by the session whose entire subject was making board discipline mechanical rather than remembered. #1169 recorded the first two; this records the third and its own lateness. The two defects are the same shape as the failure each gate exists to prevent: a substitution that preserves the count (IDEAS.md swapped out of an eight-tuple that stayed eight long), and a scope that excludes the case it was built for (added-lines filtering, blind to an EPIPHANIES.md prepend by construction). Three sub-findings kept: a count is not a set; a self-test can assert nothing while printing a number; a green CI tick is not evidence the gate ran -- the job log was pulled instead, precisely because the finding was a gate reporting success on the case it exists to catch. The argument for mechanical checks does not rest on the author being careless. Over one session the same author, holding the rule in mind, writing the tooling for the rule, broke it three times and caught it zero times. Both gates run green on this commit; the supersession index regenerates byte-identical (the entry cites no D-ids). Claude-Session: https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv Co-authored-by: Claude <noreply@anthropic.com>
AdaWorldAPI
pushed a commit
that referenced
this pull request
Sep 4, 2026
…he lint name Merging origin/main brought in the new `citation-decay` gate (#1168/#1170), which fired on exactly one new citation — mine: TECH_DEBT.md -> crates/sigker/src/signature.rs:133 anchor=symbol:"needs_range_loop" (anchor absent within +-3 lines) The gate is right and the finding is a real defect, not a false positive. `needs_range_loop` is clippy's LINT NAME; it appears nowhere in the cited source. The gate extracts the backticked token nearest the citation as the anchor, so the lint name won that race and then could not be found at the line it supposedly addressed. A citation whose anchor is absent from its target is unverifiable by construction — which is the whole point of the gate. Re-anchored on `for flat in 0..len`, which is the actual text at signature.rs:133, and the lint name is now named in prose instead. The entry also records why, so the next reader does not "fix" it back. Local gate runs after the merge, all green: - citation_decay.py --self-test: passed (both halves) - citation_decay.py --since <base>: 0 new decays (148 pre-existing backlog, unchanged and not failing) - append_only_gate.py --self-test: passed, 7 cases - append_only_gate.py origin/main: no protected file shrank, 9 checked (TECH_DEBT 4161 -> 4212, +51) - supersession_index.py: regenerated after the merge, byte-identical - plan_dids.py: no added plans, nothing to check - cargo test --manifest-path crates/sigker/Cargo.toml: 62 passed, 0 failed - cargo fmt --check: clean Also merges origin/main (f30e300..ac9148f) to clear the merge conflict that made this PR un-mergeable. The merge itself was clean; EPIPHANIES.md auto-merged, both sides having prepended. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016WkNBjHc2e3zuyz9i8qJEv
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
2 participants
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.
Fixes all three review findings on #1167 and #1168. Every one was verified against the tree before acting; all three are real.
Codex P1 on #1167 —
IDEAS.mdwas substituted out, not omittedPROTECTEDswappedAGENT_LOG.mdin forIDEAS.mdand kept the count at eight, which is exactly why it read as correct..claude/BOOT.md's immutability table and.claude/settings.json's deny list both nameIDEAS.md; it is a real 1204-line append-only ledger, and the.claude/board/**path filter started the workflow on any PR that truncated it whilecheck()never looked at it. Now nine — the canonical eight plusAGENT_LOG.md, which CLAUDE.md's one-writer rule also calls append-only. Adding that one was never the error; substituting it was.Fire-tested:
IDEAS.md1204 → 1150 gives exit 1 naming the file; restored, 9/9 ok.Codex P1 on #1168 — the gate was blind to its own motivating case
Prepending to
EPIPHANIES.mdmoves targets under citations that live in other, unchanged files on unchanged lines.--added-lines-onlytherefore reported success on precisely the change the gate exists to catch.Replaced with a regression comparison: scan the corpus at
merge-base(BASE, HEAD)and at HEAD, key each citation by its own content plus an occurrence index (never by line number — the citing line shifts too), and fail only where a citation is decayed now and was not at the base. That catches both a target moving under an old citation and a newly added wrong one, while the backlog stays reported and non-failing. The base revision is read through a detached worktree at the merge-base sha, removed in afinally; the checked-out tree is never touched, and any git failure fails closed.Two rejected alternatives, both measured: scanning unscoped fails every PR on the backlog; scanning changed files fails nearly every PR because
EPIPHANIES.mdalone carries pre-existing decays and the board-hygiene rule means almost every PR touches it.Codex P2 on #1168 —
|| trueswallowed the fail-closed pathAn unresolvable merge base became an empty
FILESand a green exit, bypassing the script's own handling. The new CLI takes no file list, so that shell branch is gone.Verification I ran myself, not only the worker's
--since HEAD~1→ 0 new / 148 pre-existing / 0 fixed, no leftover worktree;--since origin/main~500→ exit 1 with thefetch-depth: 0hint.One defect I found by reading rather than trusting the report: the new self-test printed the harness's own pass/fail under a label reading
expect 1, so a passing run printed0next to the word "expect 1" and asserted nothing about the real exit path. It now asserts the verdict the gate would actually return — and that assertion is what fires under Disable 2, so it is load-bearing rather than decoration.Number correction: the backlog is 148, not the 124 measured yesterday — the corpus grew. The earlier figure is not wrong, it is stale, and the workflow comment now carries both with dates.
Neither gate caught its own defect, and both defects are the same shape as what they gate against: a substitution that preserves the count, and a scope that excludes the case it was built for.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv