[bench] review fixes: fail-open prose, viewer stats parity, partial-commit guard - #26
Merged
Merged
Conversation
… behavior Three comments still described the pre-migration fail-open posture: - hooks/pre-tool-use.py said "failing open" on pipeline import failure, but main() issues a fail-closed VETO when the pipeline is unavailable. The utils.diff import comment now names the inline fallback instead. - utils/api.py claimed a spurious PARSE_FAILURE would make the runner "fail open into a PASS"; the runner fails closed on stage errors, so the real hazard is a spurious denial of a legitimate change. - utils/diff.py cited "C-007 fail-open" for the error-dict path; it now describes the actual contract (visible, adjudicable failure, C-001). Comment-only change; no behavior differences. Includes the governance receipts for each edit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqaVre4yc5BBn44XmX3Nak
The viewer banner computed pass/veto percentages over all ledger entries while cmd_stats computed them over adjudicated entries (excluding chain-retirement anchors), so the two surfaces could disagree on any chain holding an anchor. The banner now uses the adjudicated count for both the rates and the renamed "Governed changes" tile, matching cmd_stats exactly. Adds ViewerStatsParityTests locking the two surfaces together: banner percentages must equal cmd_stats output on a fixture containing an ANCHOR entry, and an anchor-only chain must render 0.0% rates rather than divide by zero. Includes the governance receipts for each edit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqaVre4yc5BBn44XmX3Nak
The prompt now states that "position" must be exactly REBUT, CONCEDE, or MITIGATE, and that agreeing with a finding (including one the Challenger marked context-only) means CONCEDE. _VALID_POSITIONS already rejects anything else, and an out-of-schema position invalidates the whole response into a PIPELINE_ERROR the runner fails closed on, so a Defender inventing CONFIRM or AGREE could get a legitimate change spuriously vetoed. The prompt now tells the model what the validator enforces. Prompt-only change; validation logic untouched. Includes the governance receipts for the edit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqaVre4yc5BBn44XmX3Nak
Ignores graphify-out/ (generated knowledge-graph output that embeds absolute local paths; regenerate with /graphify) and commits the six receipts from the 2026-08-04 graphify session: two PASS entries for the graphify-out chunk write, two fail-closed VETO receipts for scratchpad writes (pipeline error mid-run; diff bodies redacted as out-of-project), and two PASS entries for the .gitignore edit itself. These entries are parents of receipts already committed on this branch, so without them a fresh clone would fail `python -m cli verify` with MISSING_PARENT. Recorded content was scanned: no absolute local paths appear in the committed receipts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqaVre4yc5BBn44XmX3Nak
Two redacted PASS receipts for an edit to the session memory file outside the repo (diff bodies omitted as out-of-project; path and verdict retained). Committed immediately so no untracked entries remain: under DAG storage the next receipt names every current tip as parent, and an uncommitted parent breaks verification on a fresh clone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqaVre4yc5BBn44XmX3Nak
Under DAG storage append_entry names every current tip as a parent, including tips that exist only as untracked files, so committing a partial set of ledger/entries/ files publishes children whose parents are not in git and a fresh clone fails `python -m cli verify` with MISSING_PARENT. This happened in practice on this branch: receipts committed on 2026-08-06 named an uncommitted 2026-08-04 receipt as parent. Two layers close it. scripts/githooks/pre-commit refuses a commit that stages entry files while others remain untracked (opt-in per clone via `git config core.hooksPath scripts/githooks`, documented in Quick Start; verified here by a deliberately partial commit that was refused). CommittedChainVerificationTests in tests/test_ledger_hygiene is the enforced CI backstop: it runs verify_chain on exactly the state git holds, where a partial commit surfaces as MISSING_PARENT on the clean checkout. Includes the governance receipts for these edits, among them one fail-closed VETO receipt from a transient Challenger error on the first attempt to write the hook script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqaVre4yc5BBn44XmX3Nak
sh does not tolerate CRLF, so on a Windows clone with autocrlf the pre-commit guard would check out with CRLF endings and fail on every branch. The .gitattributes pin keeps scripts/githooks/* stored and checked out with LF everywhere. Includes the governance receipts for the edit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqaVre4yc5BBn44XmX3Nak
![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: 7cc4053c0b
ℹ️ 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".
Git ignores a hook that is not executable, so the mode-100644 file shipped in 7cc4053 meant the partial-ledger-commit guard silently never ran for anyone on a Unix clone; the failure it prevents would only surface later in CI. Windows masked this locally because it has no executable bit. Mode change only (100644 to 100755), no content change. Flagged by Codex review on PR #26. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqaVre4yc5BBn44XmX3Nak
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.
Summary
A full codebase review found no structural problems but surfaced small drift items; this branch fixes them, lands stranded work from an earlier session, and adds a guard against the ledger hazard the review uncovered.
hooks/pre-tool-use.py,utils/api.py, andutils/diff.py: prose still described the pre-migration fail-open posture while the code fails closed. Comment-only, no behavior change.ViewerStatsParityTestslocks the two surfaces together, including the anchor-only zero-rate edge.positionenum (REBUT / CONCEDE / MITIGATE) so an invented CONFIRM cannot invalidate a whole response into a fail-closed VETO, plus thegraphify-out/ignore and that session's receipts.ledger/entries/files breakspython -m cli verifyon a fresh clone (this bit us on this very branch).scripts/githooks/pre-commitrefuses such commits (opt-in:git config core.hooksPath scripts/githooks), andCommittedChainVerificationTestsis the enforced CI backstop..gitattributespins LF so the sh hook survives Windows checkouts.Every edit was adjudicated PASS by the governance pipeline; all receipts are included, one of them an honest fail-closed VETO from a transient Challenger error. Chain verifies at 254 entries in exactly the state git holds.
Test plan
python -m unittest tests.test_viewer tests.test_hook tests.test_diff tests.test_api tests.test_defender tests.test_ledger_hygieneall pass locallypython -m cli verifypasses on the committed state (clean tree)