Extract Phase 5 PR Monitor polling logic into bin/pr-monitor#205
Merged
Conversation
Per #195 Option A, moves the polling loop, event classification, and dedup state out of the natural-language rule and into a single shellcheck-clean bash script. The rule keeps the reaction protocol (steps 2-7) and the polling commands used at re-fetch time, but the per-arm regeneration of ~30-50 lines of bash is gone. Closes #195. Co-authored-by: Claude Opus 4.6 <[email protected]>
Without the seed, an already-APPROVED PR or an existing FAILURE run emits a spurious event the moment the monitor arms. The first poll now populates prev_state / prev_decision / seen_runs / seen_comments without printing; only transitions during the monitor lifetime wake the operator. Also restores the "top-level PR comments come from the comments field at re-fetch" caveat to Phase 5 — the prior extraction dropped it. Co-authored-by: Claude Opus 4.6 <[email protected]>
declare -A requires bash 4+, but the macOS default /bin/bash is 3.2. The brewed bash on the user PATH is bash 5+; using /usr/bin/env bash picks it up. CLAUDE.md prescribes /bin/bash only for bootstrap scripts, not bin/. Co-authored-by: Claude Opus 4.6 <[email protected]>
Owner
Author
|
テストコメント |
The earlier design only emitted review-thread comments, on the assumption that any other event would force a re-fetch that surfaced top-level comments contextually. A top-level comment arriving alone — common when a reviewer leaves a single "please fix X" without state or thread change — kept the monitor silent and the request invisible. Fixed by adding a NEW_TOP_COMMENT: <author> event class with the same first-poll seeding and seen-id dedup as the inline path. Co-authored-by: Claude Opus 4.6 <[email protected]>
…_TOP_COMMENT reaction Merges main into the branch (10 commits, including #214 git-essentials extraction and #216 READY_FOR_REVIEW support) and closes the resulting gaps: - bin/pr-monitor: add isDraft to gh pr view fetch, track prev_isdraft, emit READY_FOR_REVIEW when isDraft transitions true→false (first-poll seeding suppresses emit at arm time, matching existing pattern) - git-workflow.md Phase 5 step 4: add NEW_TOP_COMMENT reaction entry (reply via gh pr comment or act on fix requests) After this commit the three-way consistency holds: script emits == event-list in rule == step 4 reaction protocol Co-authored-by: Claude Sonnet 4.6 <[email protected]>
emit_new_comments and emit_new_top_comments shared the same comm -23 dedup logic, differing only in the source jq filter, the seen-IDs file, and the output label. Replace both with a single emit_new that takes the pre-extracted tab-separated lines, the seen file, and the label; the optional third field (path:line) is appended only when present. The two call sites now do just the jq extraction. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The per-event reaction list re-derived the same fix/reply/no-op decision for each event class. Replace it with one decision tree keyed on comment content plus a short event-specific notes block. Two reductions are folded in here: the "skip isResolved/isOutdated threads" instruction is dropped because bin/pr-monitor already filters those at emit time (the emit->react race guard is kept), and the redundant per-event branches collapse to fix-request / question / informational. Also remove the duplicated NEW_TOP_COMMENT body-fetch note from the event list, leaving it only in the reaction notes. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Three review-surfaced defects in the emit path: - emit_new passed the newline-separated new-ID list through `awk -v`, which BWK awk (the macOS default) rejects with "newline in string", aborting the monitor under set -e. This fires exactly when a reviewer leaves 2+ comments in one poll interval. Pass the list via the environment and read ENVIRON[] instead. - The scalar `jq -r` extractions of pr_json were unguarded, so a non-empty-but-truncated response would exit nonzero and kill the monitor silently. Gate the loop body on a jq parse check, treating malformed input like the empty-fetch case (sleep + continue). - Top-level and review-thread comments authored by the monitor account itself are now filtered out, so replies it posts via `gh pr comment` do not re-emit as fresh NEW_TOP_COMMENT / NEW_COMMENT events. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The step 4 intro claimed the monitor only emits comment events for unresolved, non-outdated threads, but that holds for NEW_COMMENT only; NEW_TOP_COMMENT is emitted regardless of resolution state. Narrow the sentence and scope the re-fetch race-guard to NEW_COMMENT. Also add a one-line note that STATE is terminal and handled in step 6, so a reader scanning step 4 does not think it was dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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.
Implements #195 — Option A (full extraction to
bin/pr-monitor).Why
Re-generating ~50 lines of bash from the natural-language Phase 5 description on every Monitor arming costs tokens and drifts. The #194 review surfaced exactly that kind of drift (dropped DISMISSED filter, missing multi-reviewer dedup). Owning a single canonical script trades a small amount of code maintenance for testability and consistency.
Option B (compose external tools) was rejected: the OSS landscape surveyed in #195 does not implement the PENDING/draft filter, and stitching
gh-wait+gh-watch-citogether has more moving parts than a single script.What
bin/pr-monitorpolls a PR and emits one line per actionable change across six event classes (STATE / REVIEW / READY_FOR_REVIEW / NEW_COMMENT / NEW_TOP_COMMENT / CI_FAILURE), seeding prior state on the first poll so it never wakes the operator with events that already held at arm time. Comments authored by the monitor's own account are excluded, so itsgh pr commentreplies don't re-emit.claude/rules/git-workflow.mdPhase 5: the polling-mechanics block is replaced bybin/pr-monitor <PR number>. The re-fetch commands and reaction protocol stay in the rule (they are how the assistant handles each event line, not how the monitor produces them). main's READY_FOR_REVIEW (Keep PR as draft until the user marks it ready #216) and git-essentials.md (Extract always-on git rules into git-essentials.md #214) changes are merged in and synced.NEW_COMMENT; the redundant per-event branches collapse independently.Notes
/usr/bin/env bash(not/bin/bash) becausedeclare -Aneeds bash 4+ and macOS ships 3.2.gh run list/ GraphQL) fails on the very first poll whilegh pr viewsucceeds, that signal is not seeded and the next poll emits its pre-existing items once as if new — self-correcting after one poll. A proper fix needs per-signal seeding flags; gating the global first-poll flip on all fetches would instead risk a deaf monitor if GraphQL keeps failing.databaseId, so it won't re-emitCI_FAILURE(the seen-runs reset is keyed on head-SHA change). Covers the push-fix workflow; a same-SHA re-run would need manual inspection.Verification
emit_new) run directly with 2 IDs through theENVIRON[]awk: both lines print inLABEL: author path:lineform; single-ID still works. The quiet-PR smoke test does not reach this path, which is why the multi-comment crash it fixes went unnoticed.me-authored comment and keeps others; emptymefilters nothing.jq -rexit 5 and abort the script underset -eu, confirming the newjq -e .parse-gate routes such a poll tosleep + continueinstead.NEW_COMMENT.