Add PR monitoring: server-driven babysit mode for agent threads#4428
Add PR monitoring: server-driven babysit mode for agent threads#4428t3dotgg wants to merge 4 commits into
Conversation
Threads can now formally monitor a PR after creating it. The agent calls a new monitor_start MCP tool; a dedicated PullRequestMonitor service polls GitHub every 30s, diffs review comments / reviews / check runs against a per-thread cursor, and wakes the thread with a self-contained prompt when there is something to address. The server — never the agent — declares the PR ready from head-SHA-anchored GitHub state. - GitHub monitor snapshot fetch (reviews, threads, check runs, mergeability) with pure cursor-diff and readiness modules - thread.monitor.start/update/end domain events through decider/projector, monitor state on the thread shell, migration 034 (monitor_json) - Wake dispatch protocol: claim → fresh terminal revalidation at the send boundary → dispatch → cursor ack only after accepted start; user turns always win and rewind in-flight wake cursors so events re-diff; 10-wake circuit breaker ends as needs-attention - Monitoring holds the thread unsettled; user settle ends the monitor; session teardown ends it as session-ended - UI: steady cyan Monitoring pill (never masks approval/input/failed), blockers sub-line, header MonitorStrip with Stop, emerald Ready-to-merge state with confirm dialog; clients can only end a monitor as "stopped" Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // the payoff moment — treat it as unread-prominent — while an actively | ||
| // monitoring thread suppresses the success-shaped unread from its quiet | ||
| // wake-turn completions (never a blocked state — that surfaces via status). | ||
| const isUnread = |
There was a problem hiding this comment.
🟡 Medium components/SidebarV2.tsx:433
When a thread is actively monitoring and a user-initiated turn completes, the row stays receded with a Monitoring pill instead of showing the unread "Done" signal. The !isMonitoring guard on isUnread suppresses hasUnseenCompletion for all turn completions on monitoring threads, not just quiet monitor wake turns. So a user who starts a normal turn, navigates away, and returns never sees that their turn finished — the row remains dimmed as though nothing happened. The UI has no way to distinguish monitor-generated wake turns from user-initiated turns here, so the blanket suppression hides legitimate completion notifications.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/SidebarV2.tsx around line 433:
When a thread is actively monitoring and a user-initiated turn completes, the row stays receded with a `Monitoring` pill instead of showing the unread "Done" signal. The `!isMonitoring` guard on `isUnread` suppresses `hasUnseenCompletion` for *all* turn completions on monitoring threads, not just quiet monitor wake turns. So a user who starts a normal turn, navigates away, and returns never sees that their turn finished — the row remains dimmed as though nothing happened. The UI has no way to distinguish monitor-generated wake turns from user-initiated turns here, so the blanket suppression hides legitimate completion notifications.
There was a problem hiding this comment.
This is the intended v1 behavior (design decision D6: monitoring suppresses success-shaped noise for the whole mode — the pill stays steady and quiet through wake turns). You're right that it also swallows user-initiated turn completions mid-monitor; distinguishing wake turns from user turns in the shell needs a turn-origin marker, which is queued as a v2 refinement rather than added to this PR. Blocked states (approval/input/failed) are never suppressed.
| return { | ||
| ...metadata, | ||
| behindBaseBy: null, | ||
| requiredChecksKnown: false, |
There was a problem hiding this comment.
🟡 Medium sourceControl/gitHubPullRequestMonitor.ts:422
getPullRequestMonitorSnapshot hard-codes requiredChecksKnown: false, so the required-check set is never actually determined. When the current head has at least one successful check run and no other blockers exist, downstream computeReadiness returns ready: true with an empty blockers list, even though required-check coverage was never established. PullRequestMonitor.pollRegistration then ends monitoring with reason ready, producing a false-green verdict from incomplete check evidence. The snapshot fetcher should derive requiredChecksKnown from the repository's required-checks configuration, or readiness must not terminate as ready while requiredChecksKnown is false.
Also found in 1 other location(s)
apps/server/src/monitor/readiness.ts:58
requiredChecksKnownaffects only the display label and never adds a blocker. When it isfalsebut at least one current-head check exists and succeeds,blockersis empty andreadybecomes true;PullRequestMonitor.pollRegistrationconsequently ends monitoring with reasonreadyeven though the required-check set is explicitly unknown. This creates a false-green verdict from incomplete check evidence.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/sourceControl/gitHubPullRequestMonitor.ts around line 422:
`getPullRequestMonitorSnapshot` hard-codes `requiredChecksKnown: false`, so the required-check set is never actually determined. When the current head has at least one successful check run and no other blockers exist, downstream `computeReadiness` returns `ready: true` with an empty `blockers` list, even though required-check coverage was never established. `PullRequestMonitor.pollRegistration` then ends monitoring with reason `ready`, producing a false-green verdict from incomplete check evidence. The snapshot fetcher should derive `requiredChecksKnown` from the repository's required-checks configuration, or readiness must not terminate as `ready` while `requiredChecksKnown` is `false`.
Also found in 1 other location(s):
- apps/server/src/monitor/readiness.ts:58 -- `requiredChecksKnown` affects only the display label and never adds a blocker. When it is `false` but at least one current-head check exists and succeeds, `blockers` is empty and `ready` becomes true; `PullRequestMonitor.pollRegistration` consequently ends monitoring with reason `ready` even though the required-check set is explicitly unknown. This creates a false-green verdict from incomplete check evidence.
There was a problem hiding this comment.
Deliberate v1 scope: requiredChecksKnown stays false until branch-protection/required-checks querying lands, and readiness deliberately cannot label itself "Ready to merge" in that state — computeReadiness returns the honest "no-known-blockers" label instead (see evidenceSupportsReadyLabel). The monitor does end with reason ready on that evidence, but the UI copy distinguishes the two. Required-checks querying is the planned v2 upgrade (design doc §P0).
| event.payload.activity.kind === "provider.turn.start.failed" | ||
| ) { | ||
| // engine.dispatch succeeded but the provider reactor failed the turn | ||
| // asynchronously — the acked events never reached an agent (I5). |
There was a problem hiding this comment.
🟡 Medium monitor/PullRequestMonitor.ts:344
onEvent releases any provider.turn.start.failed activity on the thread via releaseClaim(threadId, () => true), without checking whether the failed turn corresponds to the claim's commandId. A delayed failure from an older or user-initiated turn will delete a newer monitor wake claim and, if that newer wake is already in-flight, rewind its cursor to previousCursor — silently canceling a legitimate wake and causing the next poll to re-diff the same events. Consider correlating the activity's commandId (or turn id) against claim.commandId before releasing.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/monitor/PullRequestMonitor.ts around line 344:
`onEvent` releases *any* `provider.turn.start.failed` activity on the thread via `releaseClaim(threadId, () => true)`, without checking whether the failed turn corresponds to the claim's `commandId`. A delayed failure from an older or user-initiated turn will delete a newer monitor wake claim and, if that newer wake is already in-flight, rewind its cursor to `previousCursor` — silently canceling a legitimate wake and causing the next poll to re-diff the same events. Consider correlating the activity's `commandId` (or turn id) against `claim.commandId` before releasing.
|
|
||
| return { | ||
| ...metadata, | ||
| behindBaseBy: null, |
There was a problem hiding this comment.
🟡 Medium sourceControl/gitHubPullRequestMonitor.ts:421
behindBaseBy is hard-coded to null in every snapshot returned by getPullRequestMonitorSnapshot, so the monitor can never detect that a PR has fallen behind its base branch. Downstream cursorFromSnapshot/diffPullRequestMonitorSnapshot only emit a behind-base event when this value is positive, meaning the live GitHub fetcher will never wake the agent to update the branch. The field should be populated from GitHub comparison data instead of always returning null.
Also found in 1 other location(s)
apps/server/src/monitor/readiness.ts:28
computeReadinessnever considerssnapshot.behindBaseBy. A PR whose base branch advanced can still havemergeability: "mergeable"and successful checks attached to its unchanged head SHA, causingready: true. Because polling evaluates readiness beforemonitorDiff(which explicitly modelsbehind-baseas actionable), the monitor ends as ready instead of waking the agent to update the branch.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/sourceControl/gitHubPullRequestMonitor.ts around line 421:
`behindBaseBy` is hard-coded to `null` in every snapshot returned by `getPullRequestMonitorSnapshot`, so the monitor can never detect that a PR has fallen behind its base branch. Downstream `cursorFromSnapshot`/`diffPullRequestMonitorSnapshot` only emit a `behind-base` event when this value is positive, meaning the live GitHub fetcher will never wake the agent to update the branch. The field should be populated from GitHub comparison data instead of always returning `null`.
Also found in 1 other location(s):
- apps/server/src/monitor/readiness.ts:28 -- `computeReadiness` never considers `snapshot.behindBaseBy`. A PR whose base branch advanced can still have `mergeability: "mergeable"` and successful checks attached to its unchanged head SHA, causing `ready: true`. Because polling evaluates readiness before `monitorDiff` (which explicitly models `behind-base` as actionable), the monitor ends as ready instead of waking the agent to update the branch.
There was a problem hiding this comment.
Deliberate v1 gap: behindBaseBy population (and behind-base wakes) is deferred — the diff/wake plumbing for it is in place so the fetcher can light it up without protocol changes. Tracked in the implementation plan as a known v1 cut.
ApprovabilityVerdict: Needs human review 5 blocking correctness issues found. This PR introduces a major new feature (PR monitoring/babysit mode) with substantial server infrastructure, new orchestration events, database migration, and UI components. Additionally, there are 6 unresolved medium-severity review comments identifying potential bugs including false-green verdicts from incomplete check evidence and self-triggered wake cycles. You can customize Macroscope's approvability policy. Learn more. |
- Stop prepareMcpSession ending the active monitor on session start (session recycles no longer kill their own babysit) - Fail closed at the wake send boundary: a failed fresh PR fetch discards the wake instead of falling back to the stale poll state - Correlate provider.turn.start.failed releases by dispatch time and rewind wakeCount alongside the cursor - Monotonic generation allocator; generation-conditional updateCursor/incrementWake/remove so stale polls can't mutate a newer monitor - Dispatch thread.monitor.end before removing the registration in session teardown; end monitors on thread.archived - Preserve wakeCount on idempotent monitor.start replay (breaker can't be reset) - Key check-run cursors by head SHA + name + run id; detect resolved→unresolved thread reopens - Human changes-requested reviews now block readiness - Thread monitoringStartedAt from the registration so pre-existing unresolved threads don't block readiness forever - Use the PR's base ref in the wake prompt instead of hard-coded main - Boot reconcile: threads projecting "monitoring" with no registration are ended as session-ended Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| "stale", | ||
| ]); | ||
|
|
||
| export const getPullRequestMonitorSnapshot = Effect.fn("getPullRequestMonitorSnapshot")( |
There was a problem hiding this comment.
🟡 Medium sourceControl/gitHubPullRequestMonitor.ts:227
When the agent replies to an unresolved review thread, its own reply becomes the latest comment and updates updatedAt. The next poll treats this as a new reviewer comment and re-wakes the same thread, creating a self-triggered wake cycle until the wake limit. The GraphQL query fetches comments(last: 1) without filtering by author, so the agent's own reply is indistinguishable from new reviewer feedback. Consider excluding comments authored by the monitored bot from the thread snapshot, or tracking the last known reviewer comment so agent replies don't appear as new activity.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/sourceControl/gitHubPullRequestMonitor.ts around line 227:
When the agent replies to an unresolved review thread, its own reply becomes the latest comment and updates `updatedAt`. The next poll treats this as a new reviewer comment and re-wakes the same thread, creating a self-triggered wake cycle until the wake limit. The GraphQL query fetches `comments(last: 1)` without filtering by author, so the agent's own reply is indistinguishable from new reviewer feedback. Consider excluding comments authored by the monitored bot from the thread snapshot, or tracking the last known reviewer comment so agent replies don't appear as new activity.
- Ignore review-thread activity whose latest comment is the viewer's own reply, breaking the self-triggered wake cycle - Retry boot reconciliation on the first successful poll instead of swallowing a failed shell snapshot at startup - Remove the registration when archive teardown dispatch fails so archived threads are never polled or woken again - Stamp wake claims with the turn command's createdAt so provider turn-start failures (which echo that timestamp) match the release condition; a fresh post-dispatch timestamp could never match Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6d65afe. Configure here.
| ), | ||
| { discard: true }, | ||
| ); | ||
| yield* Ref.set(reconciled, true); |
There was a problem hiding this comment.
Reconcile aborts remaining orphans
Medium Severity
Boot reconcile walks shell threads with Effect.forEach and aborts on the first thread.monitor.end failure, so later orphan monitors in the same snapshot are skipped until the next poll. The previous start() path logged per-thread errors and kept going, so one bad orphan could block cleanup for the rest in each reconcile attempt.
Reviewed by Cursor Bugbot for commit 6d65afe. Configure here.


Threads can now formally monitor a PR after creating it. The agent calls a new
monitor_startMCP tool; a dedicatedPullRequestMonitorservice polls GitHub every 30s, diffs review comments / reviews / check runs against a per-thread cursor, and wakes the thread with a self-contained prompt when there is something to address. The server — never the agent — declares the PR ready from head-SHA-anchored GitHub state.What's included
gitHubPullRequestMonitor.ts): reviews, review threads + resolution, check runs, head SHA, mergeability; pure cursor-diff and readiness modules with false-green guards (stale approvals don't count, zero checks ≠ green, changes-requested blocks until re-review)thread.monitor-started/-snapshot-updated/-endedthrough decider/projector; monitor state on the thread shell; migration 035 (monitor_json)needs-attentionsession-ended; merged/closed PRs end it asterminalwith the whole poll cycle discarded (no post-merge resurrection)"stopped"— the other reasons are server verdictsDesign docs: UX plan https://lmrxu2kpvpm0.postplan.dev · implementation plan https://nr9llb5mku4z.postplan.dev
Testing
🤖 Generated with Claude Code
Note
High Risk
Introduces a boot-time poll loop and automatic
thread.turn.startdispatches from server verdicts on GitHub state; incorrect diff/readiness or wake logic could spam turns or miss feedback. Touches orchestration lifecycle, MCP auth capabilities, and persistence migration.Overview
Adds PR monitoring (“babysit”): agents start watching an open PR via a new MCP
monitor_starttool; the server polls GitHub, diffs review/CI state, and wakes the thread with prompts when something needs action—without the agent deciding “ready” itself.Server:
PullRequestMonitor(30s poll + domain-event reactions), in-memoryMonitorRegistry(generation-safe register/rollback), GitHub snapshot + cursor diff/readiness/wake-prompt helpers. New orchestration commands/events (thread.monitor.start|update|end) wired through decider, projector, and SQL projection (monitor_json, migration 035). MCP sessions gain amonitorcapability; provider session teardown ends active monitors. Wake protocol handles busy threads, failed starts, user-turn wins, 10-wake breaker, and terminal PR states.Client:
endMonitorcommand (user stop only). Threads withstatus === "monitoring"stay unsettled (I2). UI:MonitorStripin chat, cyan “Monitoring” pill + blockers sub-line in sidebar v2, emerald ready state with confirm-to-open-PR merge flow.Reviewed by Cursor Bugbot for commit 6d65afe. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add server-driven PR monitoring with agent babysit mode for threads
MonitorRegistrytracks active monitor sessions with generation-guarded mutations; aPullRequestMonitorservice polls GitHub for PR state changes and manages wake/claim lifecycle.monitorDiff.tsto detect actionable PR changes (new review comments, check failures, behind-base, changes-requested) andreadiness.tsto compute merge-readiness with a structured blockers list.thread.monitor.start/update/end) and events (thread.monitor-started/snapshot-updated/ended); the decider and projector handle all transitions including auto-emittingthread.monitor-endedon thread settle.monitor_jsoncolumn onprojection_threads(migration 035) and surfaces it through all snapshot query paths and theProjectionThreadschema.monitor_startMCP tool and anendThreadMonitorclient command; clears active monitors when an MCP session is revoked.MonitorStripshows monitoring/ready/stopped states with blockers, merge confirmation, and a stop action; sidebar rows display a steady cyan 'Monitoring' pill and suppress the elapsed timer while monitoring.Macroscope summarized 6d65afe.