v2.7.0 fix(eng): pr-watcher v4 senses via a deterministic script, not a subagent - #58
Merged
Merged
Conversation
sensor-poll.sh implements the whole /eng:pr-watcher sensing protocol (init pass, status-primary 15s loop, comment-stream fallback, settle conditions, slice/total budgets) and prints exactly one JSON object per invocation. Pure decision logic lives in sensor-poll-lib.sh with bats coverage in tests/sensor-poll.bats. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rewrite the SKILL.md sensing contract: the dispatcher now runs scripts/sensor-poll.sh in foreground Bash slices (continue outcome + sensor-state.json span the 30-minute budget) instead of spawning a general-purpose sensor subagent with a prompt template. The subagent contract was structurally unsatisfiable (foreground sleep blocked; background tasks and Monitor end the turn), which parked the watcher after CodeRabbit had already finished on email-hero PR 79 (2026-07-20). Adds continue/error outcomes to the dispatcher branch table and updates the failure table, state layout, and closing rules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review batch (5 toolkit lenses + codex cross-model pass): - Blocker: do not mark a terminal transition consumed until the comment streams are fetched; a single failed fetch was permanently losing the settled round to idle_timeout, the exact incident class this PR fixes. - Unify failure accounting in a fail_tick helper; stream-only failures now count toward the error threshold (reset only after a fully clean tick). - Init pass retries 3x before outcome error instead of failing on the first blip; failure-table docs now match. - Capture gh stderr into error_message (makes the 401 dispatcher branch implementable); atomic save_state + validated load_state and baseline reads so corrupt files re-init instead of breaking the one-JSON contract; bootstrap and arg errors emit full-schema JSON. - PATH append instead of prepend: still fixes the stripped-PATH case but lets a test stub win, enabling a stubbed-gh integration bats file covering every init-pass branch (10 new tests; 28 total green). - sp_all_quiet: timestamp-less or unparseable items block quietness instead of counting as vacuously quiet. - Refresh stale sensor-subagent comments in the ship-watch-nudge scripts; correct the CHANGELOG claim about the broadened fallback marker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR watcher is migrated from a sensor subagent to a deterministic foreground polling script. Shared Bash decision logic, persisted slice state, expanded outcomes, dispatcher documentation, hooks, release metadata, and Bats tests are added or updated. ChangesPR watcher sensor migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant SensorPoll
participant SensorState
participant GitHub
Dispatcher->>SensorPoll: Run foreground sensor slice
SensorPoll->>SensorState: Load or save slice progress
SensorPoll->>GitHub: Fetch PR, statuses, and review streams
GitHub-->>SensorPoll: Return current review data
SensorPoll-->>Dispatcher: Emit one JSON outcome
Dispatcher->>SensorPoll: Rerun when outcome is continue
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.gitignore:
- Line 28: Update the .gitignore entry for .gstack/ so the directory remains
ignored while explicitly re-including .gstack/qa-quincey/recipe.yml for version
control. Ensure Git’s ignore rules allow both the required directory path and
recipe file to be tracked.
In `@eng/skills/pr-watcher/scripts/sensor-poll.sh`:
- Around line 48-60: Update the argument parsing loop around the case branches
for --owner, --repo, --pr, --state-dir, --slice-seconds, and --total-seconds to
validate that a value argument exists before reading "$2". Route missing values
through arg_error so the script preserves its single-JSON stdout contract
instead of triggering a set -u unbound-variable failure; keep the existing
required-option validation unchanged.
- Around line 140-148: Update fetch_streams to use gh api pagination for the
issue_comments, reviews, and review_comments requests, preserving the existing
error handling and filtering flow. Add since parameters to the issue and review
comment endpoints using the appropriate baseline timestamp or established state,
while retaining per_page=100, so all new feedback remains available to
sp_filter_new.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fdccbca-a647-4b5b-a481-1786446cc1d9
📒 Files selected for processing (10)
.gitignoreeng/.claude-plugin/plugin.jsoneng/hooks/scripts/ship-watch-nudge-lib.sheng/hooks/scripts/ship-watch-nudge.sheng/skills/pr-watcher/CHANGELOG.mdeng/skills/pr-watcher/SKILL.mdeng/skills/pr-watcher/scripts/sensor-poll-lib.sheng/skills/pr-watcher/scripts/sensor-poll.sheng/skills/pr-watcher/tests/sensor-poll-integration.batseng/skills/pr-watcher/tests/sensor-poll.bats
…ack/ Comment: #58 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rvives the machine-global ignore Comment: #58 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…etches Both sensor-poll.sh findings from the same round (shared file, one commit): a missing flag value now routes through arg_error instead of crashing past the one-JSON contract, and fetch_streams follows all pages via gh api --paginate (page 1 is oldest-first, so 100+ item streams went blind to new feedback) with jq -s add flattening the per-page arrays. Comment: #58 (comment) Comment: #58 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@eng/skills/pr-watcher/tests/sensor-poll-integration.bats`:
- Around line 123-127: Update the test “flag with no value emits one error JSON,
never a set -u crash” to parse all output values with jq -s, assert the
resulting array length is exactly 1, then validate the sole error payload’s
outcome and error_message fields.
- Around line 16-26: The gh stub currently ignores whether callers pass
--paginate, so the pagination test cannot detect its removal. Update the stub’s
endpoint matching in the heredoc and the pagination test setup around the
relevant polling test to require --paginate for the paginated request, while
preserving existing fixture responses and failure behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 590fd0fe-97b8-48ac-a6a7-7b523af18755
📒 Files selected for processing (3)
.gitignoreeng/skills/pr-watcher/scripts/sensor-poll.sheng/skills/pr-watcher/tests/sensor-poll-integration.bats
…tub; assert single JSON value) Same round, same file, one commit: the gh stub now refuses stream fetches that lack --paginate (dropping the flag fails the suite), and both arg-error tests parse stdout with jq -s asserting exactly one JSON value. Comment: #58 (comment) Comment: #58 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
4 tasks
mujtaba3B
added a commit
that referenced
this pull request
Jul 20, 2026
…audited operator bypass (#59) * feat(eng): detect CodeRabbit's rate-limited-as-FAILURE status shape mc_cr_failure_rate_limited answers one question: is a CodeRabbit commit status of "failure" on HEAD really a rate limit rather than a genuine CR objection? Two independent proofs, either sufficient: the status description says rate limit (case-insensitive substring, so "Review rate limited" and "Rate limit exceeded" both hit), or CR posted its existing "rate limited by coderabbit.ai" marker comment. This is the third rate-limit shape. The two the lib already had key on the marker comment and cover a MISSING status (CR never started) and a stuck PENDING one (CR started, then hit the limit mid-flight). Neither covers what CR does when an INCREMENTAL pass burns the limit: it resolves its per-commit status to failure with a rate-limit description and often posts no marker comment at all. Fails closed in every degraded direction (non-failure state, empty description, unparseable comments), so a genuine CR failure is never mistaken for a rate limit. * v2.8.0 fix(eng): clear a rate-limited CR failure, and add an audited operator bypass The gate reached its rate-limit helpers only for a missing or pending CodeRabbit status, so a failure status blocked unconditionally. On #58 CR had fully reviewed the PR and acked all five findings; its final incremental pass over a test-only commit tripped the limit and posted status=failure "Review rate limited". Every dimension rendered checked, the verdict was NOT CLEAR on that one line, and landing took the documented human workaround: move .merge-clearance.json aside, gh pr merge --admin, restore. That workaround is what this retires. Two paths clear a failure now, and BOTH require a current /eng:cr review on the head, so "never both reviewers down" still holds: - rate-limited failure: machine-detectable via mc_cr_failure_rate_limited (lazy status-description fetch, only on the failure branch, so the happy path makes no extra API call). Auto-satisfies with the backstop, no flag, exactly like the missing and stuck-pending shapes. - genuine failure: the new --override-cr-failure flag. Human judgment, so it is never inferred. Recorded in the checklist line, the JSON verdict, the stamp evidence and the posted status description. Also fixes the checklist mark: cr_mark ignored both the failure status and the reviewed-head blocker, so a PR blocked solely by either rendered CodeRabbit as a green tick while the verdict said NOT CLEAR. The one dimension actually blocking was the one shown as passing. pr-watcher's Step 4b/4h now name all three shapes in a table and tell the dispatcher to read the status description itself (the sensor does not carry it), so watcher and gate reach the same conclusion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(eng): close the stale-marker bypass, and make the CR-failure decision a tested unit Review (Codex plus the silent-failure and comment lenses, all three independently) found the shape-3 marker proof reused the LOOSE mc_cr_rate_limited, which matches the marker anywhere in a PR's history. So: CR is rate-limited on an early commit and posts its notice, later recovers, reviews, and fails HEAD for real. The gate read the stale marker, auto-cleared the genuine failure with no operator flag, and labelled the audit trail "rate-limited". That is the exact trap mc_cr_rate_limited_latest already exists to close for the stuck-pending shape; shape 3 now gets the same discipline. Reproduced before the fix, and pinned by a regression test. Also from the review: - the description classifier matched a bare "rate limit" substring, so "not a rate limit issue" qualified while the hyphenated "Rate-limited" did not. Now a positive rate[ -]?limit match with a negation guard. The residual (a trailing negation) is documented, not silently accepted. - the whole CR-failure decision moves into a pure mc_cr_failure_disposition in the lib, with its truth table as bats cases. It was the one line standing between --override-cr-failure and a bare merge bypass, and it was verifiable only by hand. It also fixes attribution: the rate-limit check runs first, so passing the flag defensively on a rate-limited failure records "rate-limited", not a human override that never happened, and a later grep for real overrides stays clean. - the checklist mark now reads a CR_BLOCKED flag set beside each blocker instead of re-deriving the same conditions in a second dialect. That mirroring is what produced #58; reading the blockers makes "green iff nothing is blocking" true by construction rather than by review. - STATUS_DESC accumulates ordered notes and truncates at GitHub's ~140 chars instead of last-wins overwrites. A docs-only PR cleared through a rate-limited CR failure posted only the bookkeeping note, dropping the security-relevant reason from the most durable audit surface while the local stamp still recorded it. - the marker proof is now genuinely lazy (description first, comments only if inconclusive), the flag warns instead of silently no-opping on a non-failure status, and several comments that the new behavior had made false are corrected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
QA_STATUS: prod_verified |
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
Fixes the /eng:pr-watcher sensor design so it can never sit parked after CodeRabbit has already finished (observed live on email-hero PR 79, 2026-07-20: the sensor subagent backgrounded its polling, then parked on Monitors whose conditions fired within a minute of CodeRabbit's terminal status, with no agent left to consume them).
Root cause: the sensor-subagent contract ("block 30 minutes inside one agent turn, end with exactly one JSON") is structurally unsatisfiable. Foreground sleep is blocked for agents, and both background tasks and Monitor end the agent's turn, which the dispatcher reads as the final answer.
The fix (pr-watcher v4):
scripts/sensor-poll.sh, a deterministic bash script the dispatcher runs in FOREGROUND Bash slices. One command in, one JSON out, by construction; acontinueoutcome plussensor-state.jsonspans the 30-minute cycle budget across slices (a foreground call caps at 10 minutes).already_settled/cr_failure/ backlog-drain branches, settle conditions) carry over.scripts/sensor-poll-lib.shwith bats coverage: 18 unit tests plus 10 stubbed-gh integration tests over every init-pass branch.outcome: errorthreshold, init-pass retry tolerance, captured gh stderr inerror_message, atomic/validated state and baseline reads, full-schema JSON on bootstrap/arg errors, and robust gh/jq resolution under Claude Code's stripped PATH (the incident's poll script died on a hardcoded/opt/homebrew/bin/jq).QA
📄 Plan view: https://claude.ai/code/artifact/d4780d67-94c1-46a8-9615-8c99dce6db0c
QA driver: Claude, the building agent (this session) - every Dev row is scriptable from this machine right now
Standard (all green): unit tests · lint/types · CI ·
/eng:cr🖥️ Development
Before merge we prove the new watch script itself: unit-test its decision rules, then point it at real finished pull requests and watch it come back with one clean JSON answer in seconds instead of parking forever.
bats eng/skills/pr-watcher/tests/sensor-poll.bats(single file)scripts/sensor-poll.shagainst merged PR mujtaba3B/email-hero#79pr_closed, returns in under 30salready_settledimmediately, no polling loop enteredcontinue; slice 2 resumes cumulative ticks/elapsed fromsensor-state.json; exhausted budget emitsidle_timeout--slice-seconds/--total-secondsagainst an open PR with no fresh CR activityenv -i PATH=/usr/bin:/bin/opt/homebrew/bin/jqfailure observed in the incidentsubagent_type, no sensor prompt template); dispatcher loop calls the script in foreground🚀 Production
After merge we reinstall the plugin so Claude actually runs the new copy, then watch it handle a real CodeRabbit review round without ever getting stuck.
bin/install, then diff the plugin-cache copy against merged maineng/2.7.0carriesskills/pr-watcher/scripts/sensor-poll.shbyte-identical to the repobin/installre-runs, so this row is the refresh AND its verification/eng:pr-watcherrun on a live PR, observe one full CodeRabbit roundupdated_atProduction artifacts: the plugin cache copy
~/.claude/plugins/cache/gstack-extensions/eng/2.7.0/skills/pr-watcher/(SKILL.md +scripts/sensor-poll.sh) on Mujtaba's laptop, exercised by invoking/eng:pr-watcher; refreshed only bybin/install(prod row 1 performs and verifies that refresh). The eng plugin version is bumped to 2.7.0 in this PR so the cache path is machine-matchable.Definition of Done:
/eng:cr) + CodeRabbit addressedwhere-things-run.jsonbumped if the deploy changed hostsQA posture: Pre-merge, state
QA_STATUS: dev_verifiedplusEVIDENCE:once every Dev QA box and every Definition-of-Done bullet is checked. Post-deploy, stateQA_STATUS: prod_verifiedplusEVIDENCE:once the Prod QA rows are verified live.Summary by CodeRabbit
New Features
Documentation
Tests
Chores