fix(reviewer): apply a docs-only rubric so doc PRs aren't over-flagged - #337
Merged
Conversation
Root fix for the over-flagging that looped #334 (the escalation-budget fix #335 only bounded the symptom). When a PR's diff is documentation-only (every changed file is .md/.markdown/.rst/.txt or under docs/), the self-review prompt now carries a docs rubric: review for internal consistency, accuracy against the repo, broken references, and clarity — and explicitly DO NOT raise concerns that a statement is "unverifiable in the diff", "lacks CI/test evidence", or "references work outside this diff". A document legitimately summarizes and points to work it cannot contain (CI runs, secrets, sibling/other-repo PRs); demanding in-diff proof of those is not a valid concern for a docs PR. Mixed (doc+code) and config-only diffs (e.g. .console/reconcile.yaml) still get the full review rubric — only all-documentation diffs are right-sized. Helpers: _is_doc_path / _files_from_diff / _diff_is_docs_only; rubric _DOC_ONLY_REVIEW_RUBRIC. Tests: path classification, docs-only detection, and that _phase1 injects the rubric for docs-only diffs and omits it for code. Reviewer suite 123 pass; audit clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
Critical bug in _phase1(): _pr_files is assigned only in the else branch (line with _files_from_diff) but used unconditionally after the if/else block at docs_only = _diff_is_docs_only(_pr_files). Large diffs will trigger the if branch and cause NameError when _pr_files is undefined. Fix: move _pr_files = _files_from_diff(diff) before the if/else block so it's defined for all code paths. |
…paths The _pr_files variable is used unconditionally at line 2112 to determine if a diff is documentation-only. Previously it was assigned conditionally: - In the if branch (large diffs): via gh_client.list_pr_files() - In the else branch (small diffs): via _files_from_diff() This fragile approach could break if there was any code path where _pr_files wasn't assigned. Move the baseline assignment (_files_from_diff) before the if/else block so _pr_files is always defined, then optionally override it with the GitHub API call for large diffs. Resolves self-review concern: ensures _pr_files is defined for all code paths. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
ProtocolWarden
added a commit
that referenced
this pull request
Jun 19, 2026
#335 stopped the fleet's own fix-push from resetting the fix_attempts budget by matching the recorded last_fix_push_sha. But that SHA is recorded only AFTER the fix pass returns — so if the watcher is interrupted between the push and the record (a long fix pass killing the process, as seen on #337), the SHA is lost and the next poll mistakes our own push for an external one, resetting the budget and re-opening the #334 infinite-loop risk. Add a restart-safe fallback using state that survives the pre-fix save: when there is an active fix cycle (fix_attempts > 0) but the pass outcome was never recorded (last_fix_pass_pushed absent — popped at dispatch start, re-set only on completion), a head move is our interrupted fix's push, not an external one — so do not reset. A poll never observes this mid-dispatch (the dispatch is synchronous within one poll), so the fallback only triggers after a restart. External pushes after a COMPLETED pass still reset for a fresh review. Tests: restart-mid-fix preserves the budget (→2); the self/external-push tests now set last_fix_pass_pushed=True to model a completed prior pass. Reviewer suite 124 pass; audit clean. Co-authored-by: ProtocolWarden <ProtocolWarden@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
ProtocolWarden
added a commit
that referenced
this pull request
Jun 21, 2026
) * feat(eval): Phase 4 scaffolding — self-healing agent-quality guard Stands up the EVAL machinery (HARNESS_TRUST_HARDENING §4) — everything buildable ahead of the one irreducibly-human step (the operator answer-key signature): - corpus.py: append-only, hash-chained case ledger. Editing/deleting any past entry breaks the chain → tamper-evident (verified by a required CI check). - signing.py: Ed25519 operator signatures. A case is *graded* only when its signature verifies against the constitution public key; asymmetric so no compute inside the trust boundary can forge a label. Private key stays offline. - replay.py: deterministic BLOCKING gate — replays input.checks through the pure code-computed verdict (pr_review_watcher.verdict.compute_verdict) and exact- matches the committed answer. No model → zero flakiness; catches a #313-style verdict-bypass regression. Only operator-signed cases count toward the gate. - critic.py: non-blocking, different-model-family N-of-M drift monitor (the model extractor is an injected seam; production wires a different-family adapter). - constitution.py: monotonic baseline floor + report-only→blocking graduation (D-EVAL-3). The gate can never block before the answer key is seeded (§0.1). - verify.py + eval-corpus-integrity.yml: the required check tying chain + signatures + floor together. CODEOWNERS pins corpus + constitution + workflow to the operator (D-EVAL-2). Seeds 7 unsigned candidate cases (#313/#337 classes); all pass replay and the gate is correctly report-only (0/15 signed). The exam/answer-key split: the fleet may append candidate cases, but only an offline operator signature makes one count toward the gate. 33 unit tests (hash-chain tamper-evidence, signature non-transfer, replay gating, graduation, drift majority-vote, end-to-end verify). ruff/ty clean. Deferred (irreducibly human): operator generates the Ed25519 key offline, commits the pubkey, signs >=15 seed cases → gate graduates to blocking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(eval): declare cryptography dep; wire monotonic floor into verify.py CI surfaced two gaps in the Phase 4 scaffolding: - cryptography was used by eval.signing but not declared, so the test/ty jobs failed to import it (it was only transitively present locally). Added it to [project] dependencies. - the monotonic baseline-floor check lived only in the workflow's inline Python heredoc, so constitution.is_monotonic_successor_of was "tested but never called in production" (D12). Moved the enforcement into verify.py (--base-floor) and simplified the workflow to call it — de-stringifies the check and clears D12. 35 unit tests (added monotonic raise/lower cases); ruff/ty/D12 clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: ProtocolWarden <ProtocolWarden@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
The root fix for the over-flagging that looped #334 — the escalation-budget
fix (#335) only bounded the symptom; this stops the reviewer raising the
false-positive in the first place.
When a PR's diff is documentation-only (every changed file is
.md/.markdown/.rst/.txtor underdocs/), the self-review prompt now carries adocs rubric: review for internal consistency, accuracy against the repo, broken
references, and clarity — and do NOT raise concerns that a statement is
"unverifiable in the diff", "lacks CI/test evidence", or "references work outside
this diff". A document legitimately points to work it can't contain (CI runs,
secrets, sibling/other-repo PRs); demanding in-diff proof of those isn't a valid
concern for a docs PR.
Scoped tightly: mixed (doc+code) and config-only diffs (e.g.
.console/reconcile.yaml) still get the full review rubric — only all-docs diffsare right-sized.
Helpers
_is_doc_path/_files_from_diff/_diff_is_docs_only; rubric_DOC_ONLY_REVIEW_RUBRIC. Tests cover classification, detection, and that_phase1injects the rubric for docs-only and omits it for code. Reviewer suite123 pass; audit clean.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
🤖 Generated with Claude Code