fix(reviewer): don't reset the escalation budget on the fleet's own fix-push - #335
Merged
Merged
Conversation
…ix-push A CONCERNS PR whose concerns are unsatisfiable in-diff (e.g. a doc summarizing out-of-diff facts) looped forever: each auto-fix pushed a new head, which _phase1's "head changed after concerns → reset fix state" treated as new work and zeroed fix_attempts — so the budget never reached max_fix_attempts and the PR never terminated (observed on #334: 7 self-pushes, fix_attempts stuck at 1, accumulating evidence-file cruft). Fix: record the head each fix pass produces (last_fix_push_sha) and reset the budget ONLY on an EXTERNAL push (current head ≠ our last fix-push). Self-pushes now accumulate toward max_fix_attempts, so a non-converging PR terminates (close+requeue) instead of churning. External (human) pushes still reset for a fresh review, unchanged. Surfaced now because Part B (#333) made reviewer-verdict a required check, so the loop became a hard merge blocker rather than advisory churn. Tests: self-pushed fix preserves the budget (→2); external push resets (→1). Reviewer suite 118 pass; audit clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 18, 2026
ProtocolWarden
added a commit
that referenced
this pull request
Jun 19, 2026
#337) * fix(reviewer): apply a docs-only rubric so doc PRs aren't over-flagged 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> * fix(reviewer): move _pr_files assignment before if/else for all code 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> --------- Co-authored-by: ProtocolWarden <ProtocolWarden@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Operations Center Bot <operations-center-bot@example.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>
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 bug (surfaced by #334)
A CONCERNS PR whose concerns are unsatisfiable in-diff — e.g. a docs PR
summarizing facts that live outside the diff (CI runs, a secret refresh, sibling
PRs) — looped forever: 7 self-pushes,
fix_attemptsstuck at 1, accumulatingroot-level evidence cruft (
VERIFICATION_*.md,RESOLUTION_SUMMARY.md), and itnever escalated.
Root cause:
_phase1's "head changed after concerns → reset fix state" fired onevery head move — including the fleet's own fix-push. So the escalation
budget reset each pass and never reached
max_fix_attempts.The fix
Record the head each fix pass produces (
last_fix_push_sha) and reset the budgetonly on an external push (
current head ≠ our last fix-push). Self-pushes nowaccumulate → a non-converging PR terminates (close + requeue) instead of
churning. External (human) pushes still reset for a fresh review.
This matters more now that Part B (#333) made
reviewer-verdicta requiredcheck: the loop became a hard merge blocker, not just advisory churn.
Tests
test_phase1_self_pushed_fix_does_not_reset_budget(→ fix_attempts 2,last_fix_push_sha recorded) and
test_phase1_external_push_resets_budget(→ 1).Reviewer suite 118 pass; audit clean.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
🤖 Generated with Claude Code