WIP: "review unsettled threads" view that summarizes in-progress work - #4417
WIP: "review unsettled threads" view that summarizes in-progress work#4417t3dotgg wants to merge 23 commits into
Conversation
Adds a sidebar-v2 button that reviews every unsettled thread across all connected environments: an LLM summarizes each thread, suggests a corrected title when the current one is misleading, and recommends settling threads whose work has concluded. Results stream into a new /review-sweep page with per-item Apply buttons and Apply All — nothing applies without a click. - contracts: ReviewThreadSummaryInput/Result, review.summarizeThread WS RPC, reviewSweep capability flag (old servers are skipped) - server: generateThreadReview TextGeneration op across all five providers, conservative prompt with transcript capping, and ReviewService.summarizeThread reading the projection - client-runtime: summarizeThread RPC command atom - web: reviewSweepStore runner (concurrency 3, runId-guarded, survives navigation), /review-sweep view, SidebarV2 entry button Settle recommendations are blocked for active threads at four layers: prompt rules, normalizeThreadReview, a server-side projection re-check (never trusts the client's canSettleNow), and a client canSettle re-check at apply time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 8 blocking correctness issues found. This WIP PR introduces a significant new 'review unsettled threads' feature with new server RPCs, client state management, PR merge queues, and UI components. There are 25+ unresolved review comments identifying substantive bugs in merge handling, settle logic, and state management that warrant human attention. You can customize Macroscope's approvability policy. Learn more. |
t3dotgg
changed the title
- macroscope: guard apply-title/apply-settle completions with the runId captured before the RPC so a Re-run mid-flight never stamps applied state onto the new run's cards; applyAll re-reads items from the store each step so mid-apply dismissals are respected, and bails out entirely when the run changes. - cursor: the server's activity re-check now also treats a queued turn start (user message no session has adopted yet) as active, mirroring the decider's thread.settle guard; sweep candidate ordering uses NaN-safe toSortableTimestamp instead of raw Date.parse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed all four bot findings in 8aad08d:
🤖 Generated with Claude Code |
The sidebar button now only navigates to /review-sweep; the sweep no longer auto-starts. The idle screen shows exactly what a run will do before any model call happens: how many threads will be reviewed (and how many the cap would skip), which text-generation model each environment's server will use, and a cost/latency heads-up when the run is large (15+ threads). Starting requires an explicit click. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The environment/model breakdown previously aggregated every sweep candidate, so with more than SWEEP_MAX_THREADS unsettled threads it could list environments whose threads would all be skipped and row totals exceeding the actual call count. The summary now sorts and caps candidates with the same helpers startReviewSweep uses, so the rows always describe exactly the calls a run would make. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Environment labels can collide (e.g. identical hostnames); keying the rows by label would make React mis-reconcile them. Key by the unique environment id instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Latest bugbot pass (which reviewed dc66ff1):
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/server/src/review/ReviewService.ts (1)
163-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMislabeled
operationon non-generation errors.
mapProjectionErrorand the server-settings load failure both tagTextGenerationError.operationas"generateThreadReview", even though the actual failing step is a projection read or settings load, not the generation call. This will misdirect error triage toward the text-generation provider when the real fault is elsewhere.Also applies to: 211-220
🤖 Prompt for 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. In `@apps/server/src/review/ReviewService.ts` around lines 163 - 168, Update the TextGenerationError operation labels in mapProjectionError and the server-settings load failure handling to identify their actual failing steps rather than "generateThreadReview". Use distinct operation values for the thread projection read and server-settings load, while preserving the existing error details and cause propagation.
🤖 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 `@apps/server/src/review/ReviewService.ts`:
- Around line 178-186: Update the cwd resolution in the review summary flow
around resolveThreadWorkspaceCwd so an unresolved thread workspace does not fall
back to process.cwd(). Instead, propagate a typed retryable error when the
project shell is unavailable or no workspace path can be resolved, preserving
the resolved workspace path for valid threads.
---
Nitpick comments:
In `@apps/server/src/review/ReviewService.ts`:
- Around line 163-168: Update the TextGenerationError operation labels in
mapProjectionError and the server-settings load failure handling to identify
their actual failing steps rather than "generateThreadReview". Use distinct
operation values for the thread projection read and server-settings load, while
preserving the existing error details and cause propagation.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2fc3dff3-55aa-451c-833a-e81b598de55f
📒 Files selected for processing (26)
apps/server/src/environment/ServerEnvironment.tsapps/server/src/git/GitManager.test.tsapps/server/src/review/ReviewService.test.tsapps/server/src/review/ReviewService.tsapps/server/src/server.test.tsapps/server/src/server.tsapps/server/src/textGeneration/ClaudeTextGeneration.tsapps/server/src/textGeneration/CodexTextGeneration.tsapps/server/src/textGeneration/CursorTextGeneration.tsapps/server/src/textGeneration/GrokTextGeneration.tsapps/server/src/textGeneration/OpenCodeTextGeneration.tsapps/server/src/textGeneration/TextGeneration.test.tsapps/server/src/textGeneration/TextGeneration.tsapps/server/src/textGeneration/TextGenerationPrompts.test.tsapps/server/src/textGeneration/TextGenerationPrompts.tsapps/server/src/textGeneration/TextGenerationUtils.tsapps/server/src/ws.tsapps/web/src/components/SidebarV2.tsxapps/web/src/components/reviewSweep/ReviewSweepView.tsxapps/web/src/reviewSweepStore.tsapps/web/src/routeTree.gen.tsapps/web/src/routes/review-sweep.tsxpackages/client-runtime/src/state/review.tspackages/contracts/src/environment.tspackages/contracts/src/review.tspackages/contracts/src/rpc.ts
| const projectOption = yield* projectionSnapshotQuery | ||
| .getProjectShellById(thread.projectId) | ||
| .pipe(Effect.mapError(mapProjectionError)); | ||
| const cwd = | ||
| resolveThreadWorkspaceCwd({ | ||
| thread, | ||
| projects: Option.isSome(projectOption) ? [projectOption.value] : [], | ||
| }) ?? process.cwd(); | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't fall back to process.cwd() when the thread's workspace can't be resolved.
If getProjectShellById returns None (deleted/renamed project, projection lag, etc.) and the thread has no worktreePath, cwd silently becomes the server process's own working directory rather than the thread's project. That directory is then handed straight to the provider CLI spawn (see ClaudeTextGeneration.generateThreadReview / CodexTextGeneration.generateThreadReview), which can read local context files (e.g. AGENTS.md/CLAUDE.md) from that cwd and mix unrelated content into the prompt sent to an external LLM. Since review-sweep runs unattended over up to 50 threads with concurrency 3, this silent fallback could review "the server's own directory" instead of failing loudly.
Prefer failing the summary for that thread (surfacing a typed error the client can show as a retry-able failure) over guessing a cwd.
🔒 Proposed fix: fail instead of silently falling back
- const cwd =
- resolveThreadWorkspaceCwd({
- thread,
- projects: Option.isSome(projectOption) ? [projectOption.value] : [],
- }) ?? process.cwd();
+ const cwd = resolveThreadWorkspaceCwd({
+ thread,
+ projects: Option.isSome(projectOption) ? [projectOption.value] : [],
+ });
+ if (cwd === undefined) {
+ return yield* new TextGenerationError({
+ operation: "generateThreadReview",
+ detail: `Unable to resolve a workspace cwd for thread '${input.threadId}'.`,
+ });
+ }🤖 Prompt for 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.
In `@apps/server/src/review/ReviewService.ts` around lines 178 - 186, Update the
cwd resolution in the review summary flow around resolveThreadWorkspaceCwd so an
unresolved thread workspace does not fall back to process.cwd(). Instead,
propagate a typed retryable error when the project shell is unavailable or no
workspace path can be resolved, preserving the resolved workspace path for valid
threads.
The provider CLI spawn reads local context files (AGENTS.md, CLAUDE.md) from its cwd, so silently falling back to process.cwd() when a thread has no worktree and its project shell is missing would mix the server's own directory contents into an external LLM prompt. Return a typed TextGenerationError instead — the sweep UI already renders it as a retry-able error card. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…verview # Conflicts: # apps/server/src/environment/ServerEnvironment.ts # packages/contracts/src/environment.ts
|
Two more updates:
🤖 Generated with Claude Code |
- Group sweep cards and resolve project titles by scoped (environmentId, projectId) key — bare project ids can collide across connected environments. - Withdraw a settle recommendation from the card when the apply-time canSettle guard refuses it, instead of leaving a CTA the guard will keep rejecting. - Document why summarizeThread's two projection reads are ordered transcript-then-shell: reading the activity view last catches sessions that start mid-read, and the reverse skew only affects summary staleness — settles are re-validated at apply time and by the decider, so no consistency window can settle live work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Cursor's post-merge review pass, addressed in c68e9fa:
🤖 Generated with Claude Code |
Real-data testing surfaced the gap the plan had flagged: the sweep passed changeRequestState: null, so threads the sidebar auto-settles (merged/closed PR + idle) counted as unsettled — 18 candidates vs ~7 sidebar cards. SidebarV2 now mirrors its per-row PR-state map into the sweep store, isSweepCandidate consults it, and the pre-run summary subscribes to a version counter so its count tracks PR states as they stream in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Real-data feedback: uniform gray cards grouped by project buried the actionable items. The results view is now organized by what the user should do, ordered easiest-to-clear first: 1. Ready to settle (one-click, emerald accent) 2. Title fixes (sky) 3. Needs your attention (amber — awaiting review/input) 4. In flight / review-failed / still-reviewing tails Cards are denser, carry a per-bucket accent stripe and a single primary action, and gain a metadata row: project favicon + name, branch, thread age, and diff size. Diff stats (+adds/−dels/files) come from the thread's latest ready checkpoint, returned by the review RPC as a new optional diffStats field — no extra client requests. Within each bucket, smallest diff sorts first to build clearing momentum. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| additions: latestCheckpoint.files.reduce((sum, file) => sum + file.additions, 0), | ||
| deletions: latestCheckpoint.files.reduce((sum, file) => sum + file.deletions, 0), | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
Diff stats use last turn only
Medium Severity
The diffStats calculation uses only the latest checkpoint's diff. Checkpoints store per-turn changes, not cumulative thread diffs. This understates the total diff for multi-turn threads, impacting features like sorting that assume it reflects the whole thread's effort.
Reviewed by Cursor Bugbot for commit f508f42. Configure here.
| const blocked = shell?.hasPendingApprovals === true || shell?.hasPendingUserInput === true; | ||
| if (blocked) return "attention"; | ||
| if (working) return "inFlight"; | ||
| return "attention"; |
There was a problem hiding this comment.
Queued work mislabeled as attention
Low Severity
Live classification treats only session running/starting as in flight. A just-sent user message that has not been adopted yet (hasQueuedTurnStart) falls through to attention with “Waiting on you,” even though the next step is the agent, not the user.
Reviewed by Cursor Bugbot for commit f508f42. Configure here.
Real-data feedback round two:
- Results render as a responsive grid (1/2/3 columns) instead of a
full-width list.
- While the sweep runs, the page shows only a progress count
("Reviewing your work — 7 of 18"); results reveal all at once when
every thread is done, so triage happens in one pass over a stable
layout instead of chasing cards as they pop in.
- The model now returns a one-sentence imperative nextStep ("Review
and merge PR #4415.") which becomes the card body — a direct answer
to "what should I do here". Summaries are capped at one sentence and
demoted to an info-icon tooltip.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The screenshot from real-data testing showed nextStep rambling into
status recaps ("The requested comparison and hosted write-up are
complete, and..."). The prompt now demands a verb-first command of at
most 10 words with explicit GOOD/BAD examples, and normalizeThreadReview
backstops drift by truncating to the first sentence, hard-capped at 80
chars. Attention-bucket cards fall back to the summary when an older
server omits nextStep, so they always say what's being waited on.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four features from real-data testing feedback: - The sidebar button now opens a launch modal (scope + model + cost summary) instead of navigating. Starting closes the modal and shows a "running in background" toast; completion raises a toast with an actionable-count summary and a "View results" link. The sweep was already navigation-proof; now the UX matches. - Reviews investigate deeper: when a thread's branch has a PR, the server fetches state, review decision, CI rollup, mergeability, and the last five comments via `gh pr view`, feeds them into the review prompt, and returns a prStatus with a computed mergeReady flag (open + clean merge + CI not failing + not blocked on review). Lookups degrade to "no PR context" on any failure. - New "Merge ready" bucket at the top of the results grid with per-card "Merge & settle" and a section-level "Merge all (N)". - Merging runs through a SEQUENTIAL queue: each PR is re-validated against live GitHub state before merging (an earlier queue entry may have just changed the base), merged PRs settle their thread immediately, and a conflict marks the card and hands the rebase to the thread's own agent via a turn message — the queue continues past it. review.mergePullRequest returns conflict/already-closed/not-ready as outcomes rather than errors so the queue can branch on them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| const recent = input.recentMessages.slice(-THREAD_REVIEW_RECENT_MESSAGE_COUNT); | ||
| const transcript = recent | ||
| .map( | ||
| (message) => | ||
| `[${message.role}] ${limitSection(message.text, THREAD_REVIEW_MESSAGE_CHAR_LIMIT)}`, | ||
| ) | ||
| .join("\n\n"); |
There was a problem hiding this comment.
🟡 Medium textGeneration/TextGenerationPrompts.ts:270
limitSection(transcript, 24_000) truncates from the front, so when the selected messages exceed 24,000 characters the newest messages are dropped and only older ones remain. This can cause the model to miss the thread's final outcome and produce an incorrect summary or settle recommendation. Consider applying the aggregate cap from the tail so the newest messages are preserved.
const recent = input.recentMessages.slice(-THREAD_REVIEW_RECENT_MESSAGE_COUNT);
+ let transcriptChars = 0;
const transcript = recent
.map(
(message) =>
`[${message.role}] ${limitSection(message.text, THREAD_REVIEW_MESSAGE_CHAR_LIMIT)}`,
)
+ .reverse()
+ .filter((line) => (transcriptChars += line.length + 2) <= 24_000)
+ .reverse()
.join("\n\n");🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/textGeneration/TextGenerationPrompts.ts around lines 270-276:
`limitSection(transcript, 24_000)` truncates from the front, so when the selected messages exceed 24,000 characters the newest messages are dropped and only older ones remain. This can cause the model to miss the thread's final outcome and produce an incorrect summary or settle recommendation. Consider applying the aggregate cap from the tail so the newest messages are preserved.
| } | ||
|
|
||
| const outcome = result.value.outcome; | ||
| if (outcome === "merged" || outcome === "already-closed") { |
There was a problem hiding this comment.
🟠 High src/reviewSweepStore.ts:486
mergeOne treats the server outcome "already-closed" as "merged", so a PR that was merely closed (not merged) gets its card marked merged and its thread automatically settled. A merge-ready PR can be closed without merging between the sweep review and this action, and this branch incorrectly concludes the work as merged and settles the thread. Consider only matching the "merged" outcome here and handling "already-closed" as a non-merged terminal state.
| if (outcome === "merged" || outcome === "already-closed") { | |
| if (outcome === "merged") { |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/reviewSweepStore.ts around line 486:
`mergeOne` treats the server outcome `"already-closed"` as `"merged"`, so a PR that was merely closed (not merged) gets its card marked `merged` and its thread automatically settled. A merge-ready PR can be closed without merging between the sweep review and this action, and this branch incorrectly concludes the work as merged and settles the thread. Consider only matching the `"merged"` outcome here and handling `"already-closed"` as a non-merged terminal state.
| after.patchItem(key, { mergeStatus: "conflicted", mergeDetail: result.value.detail }); | ||
| // Hand the conflict to the thread's own agent: it has the branch | ||
| // checked out and the full context to rebase and resolve. | ||
| await runAtomCommand( |
There was a problem hiding this comment.
🟡 Medium src/reviewSweepStore.ts:509
When threadEnvironment.startTurn fails in the conflict branch, mergeOne still returns "conflict" and leaves the card marked conflicted, so the UI shows "agent rebasing" and the queue toast reports the handoff succeeded — but no agent was actually asked to resolve it. The PR can remain stuck with no signal for the user to intervene. Consider checking the startTurn result and, on failure, patching the card to failed with the error detail and returning "failed" instead of "conflict".
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/reviewSweepStore.ts around line 509:
When `threadEnvironment.startTurn` fails in the conflict branch, `mergeOne` still returns `"conflict"` and leaves the card marked `conflicted`, so the UI shows "agent rebasing" and the queue toast reports the handoff succeeded — but no agent was actually asked to resolve it. The PR can remain stuck with no signal for the user to intervene. Consider checking the `startTurn` result and, on failure, patching the card to `failed` with the error detail and returning `"failed"` instead of `"conflict"`.
| if (item.result.prStatus?.mergeReady && !item.settleApplied) return "mergeReady"; | ||
| if (item.result.recommendSettle && !item.settleApplied) return "settle"; | ||
| if (item.result.suggestedTitle && !item.titleApplied) return "title"; | ||
| // Live shell wins over review-time knowledge: a thread blocked on the user | ||
| // right now belongs in "attention" even if the review predates the block. | ||
| const shell = readThreadShell(item.ref); | ||
| const working = shell?.session?.status === "running" || shell?.session?.status === "starting"; | ||
| const blocked = shell?.hasPendingApprovals === true || shell?.hasPendingUserInput === true; | ||
| if (blocked) return "attention"; | ||
| if (working) return "inFlight"; | ||
| return "attention"; |
There was a problem hiding this comment.
🟡 Medium reviewSweep/ReviewSweepView.tsx:135
A completed item with recommendSettle: true is classified into the settle bucket even when its live shell now has pending approvals, pending user input, or a running session. This contradicts the intended live-state precedence: the comment says "Live shell wins over review-time knowledge," but the recommendSettle check runs before the blocked/working checks, so a thread that became blocked after the review still shows a stale Settle action (rejected only after the user clicks). Reorder so the live-shell checks take priority over the recommendation checks.
- if (item.result.prStatus?.mergeReady && !item.settleApplied) return "mergeReady";
- if (item.result.recommendSettle && !item.settleApplied) return "settle";
- if (item.result.suggestedTitle && !item.titleApplied) return "title";
// Live shell wins over review-time knowledge: a thread blocked on the user
// right now belongs in "attention" even if the review predates the block.
const shell = readThreadShell(item.ref);
const working = shell?.session?.status === "running" || shell?.session?.status === "starting";
const blocked = shell?.hasPendingApprovals === true || shell?.hasPendingUserInput === true;
if (blocked) return "attention";
if (working) return "inFlight";
+ if (item.result.prStatus?.mergeReady && !item.settleApplied) return "mergeReady";
+ if (item.result.recommendSettle && !item.settleApplied) return "settle";
+ if (item.result.suggestedTitle && !item.titleApplied) return "title";🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/reviewSweep/ReviewSweepView.tsx around lines 135-145:
A completed item with `recommendSettle: true` is classified into the `settle` bucket even when its live shell now has pending approvals, pending user input, or a running session. This contradicts the intended live-state precedence: the comment says "Live shell wins over review-time knowledge," but the `recommendSettle` check runs before the `blocked`/`working` checks, so a thread that became blocked after the review still shows a stale Settle action (rejected only after the user clicks). Reorder so the live-shell checks take priority over the recommendation checks.
| }, | ||
| { reportFailure: false }, | ||
| ); | ||
| if (settleResult._tag === "Success") { |
There was a problem hiding this comment.
🟡 Medium src/reviewSweepStore.ts:499
When a PR merge succeeds but the subsequent threadEnvironment.settle call fails, mergeOne ignores the failure and returns "merged" with mergeStatus set to "merged". This leaves the thread unsettled with no error surfaced and no retry path — the card stops offering any action once mergeStatus becomes "merged". Consider surfacing the settle failure (e.g., via a toast or mergeDetail) so the user knows the thread still needs to be settled.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/reviewSweepStore.ts around line 499:
When a PR merge succeeds but the subsequent `threadEnvironment.settle` call fails, `mergeOne` ignores the failure and returns `"merged"` with `mergeStatus` set to `"merged"`. This leaves the thread unsettled with no error surfaced and no retry path — the card stops offering any action once `mergeStatus` becomes `"merged"`. Consider surfacing the settle failure (e.g., via a toast or `mergeDetail`) so the user knows the thread still needs to be settled.
There was a problem hiding this comment.
One error-handling convention issue found in the new review service code. See inline comment.
Posted via Macroscope — Effect Service Conventions
Merged origin/main (9 commits) and fixed review findings: - Macroscope (CI failure): ReviewMergeError dropped the underlying failure by stringifying it into `detail`. It now carries an optional `cause` and uses a static detail, matching the sibling mapper in summarizeThread. - Cursor (medium, real bug): diffStats used only the latest checkpoint, but CheckpointReactor diffs previous→current, so each checkpoint's files are that turn's delta. Now sums every ready checkpoint, deduping the file count by path. Regression test added. - Cursor (medium): SidebarV2 remounting published an empty PR-state map and wiped the mirrored cache, making merged-PR threads look unsettled. publishSweepChangeRequestStates now merges instead of replacing. - Macroscope (medium): the buckets memo read live shells via readThreadShell but only depended on items, so cards could stay in a stale bucket. It now depends on thread shells too. - Cursor (low): sweep candidate selection used a raw wall clock while SidebarV2 quantizes to the minute, disagreeing near the merged-PR idle boundary. Both now use the same minute-quantized clock. - Cursor (low): threads with a queued turn start are classified as in-flight rather than needing attention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Synced with Fixed
Already fixed in earlier commits (re-posted against the new diff): "Project groups ignore environment scope" (c68e9fa — grouping keys on scoped project refs), "Stale settle CTA after activity block" (c68e9fa — the recommendation is withdrawn when the apply-time guard refuses), and "Inconsistent projection reads for settle" (documented at the read site: the transcript→shell order deliberately catches sessions that start mid-read, and settles are re-validated at apply time and by the decider). Typecheck, lint, and fmt clean; 14 ReviewService tests and 76 tests across the touched server areas pass. 🤖 Generated with Claude Code |
| }, | ||
| { reportFailure: false }, | ||
| ); | ||
| return "conflict"; |
There was a problem hiding this comment.
Conflict handoff failure goes unseen
High Severity
The merge queue prematurely updates an item's status to 'conflicted' and 'agent rebasing' when a merge conflict occurs. The subsequent startTurn command, intended to hand off the rebase to the agent, ignores its own failures. This can lead to silently unresolved conflicts, as the UI falsely indicates the agent is working without user notification or a retry path.
Reviewed by Cursor Bugbot for commit c1b3390. Configure here.
| if (settleResult._tag === "Success") { | ||
| useReviewSweepStore.getState().patchItem(key, { settleApplied: true }); | ||
| } | ||
| return "merged"; |
There was a problem hiding this comment.
Stale settle flag after re-run
Medium Severity
After a successful merge, settleApplied is written with no runId check. A Re-run during that settle RPC can stamp the new run’s card as already settled, hiding merge/settle CTAs for work that was never applied in the new sweep.
Reviewed by Cursor Bugbot for commit c1b3390. Configure here.
| // Bump the store version so the pre-run summary recomputes its candidate | ||
| // count as PR states stream in from the sidebar rows. | ||
| useReviewSweepStore.getState().bumpCandidateVersion(); | ||
| } |
There was a problem hiding this comment.
PR state cache never clears
Medium Severity
publishSweepChangeRequestStates only merges keys and never deletes. When SidebarV2 clears a thread’s PR state to null, the sweep cache keeps a stale merged/closed value, so idle threads can be treated as settled and omitted from the candidate set while the sidebar still shows them unsettled.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c1b3390. Configure here.
| if (settleResult._tag === "Success") { | ||
| useReviewSweepStore.getState().patchItem(key, { settleApplied: true }); | ||
| } | ||
| return "merged"; |
There was a problem hiding this comment.
Merge settle skips live guard
High Severity
mergeOne settles after merged or already-closed without re-checking canSettle on the live shell. That breaks the settle-safety invariant other apply paths enforce, so a thread that became active between review and merge can still be settled if the decider race window is missed, hiding in-flight work from the sidebar.
Reviewed by Cursor Bugbot for commit bcd4dfa. Configure here.
| const blocked = shell?.hasPendingApprovals === true || shell?.hasPendingUserInput === true; | ||
| if (blocked) return "attention"; | ||
| if (working) return "inFlight"; | ||
| return "attention"; |
There was a problem hiding this comment.
Completed cards mis-bucketed
Medium Severity
classifySweepItem exits the merge-ready and settle buckets once settleApplied is true, then falls through to attention. Successfully merged or settled cards therefore reappear under Needs your attention, and the Settled badge path in the settle bucket is unreachable.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bcd4dfa. Configure here.
| autoSettleAfterDays: options.autoSettleAfterDays, | ||
| changeRequestState, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Snoozed threads enter the sweep
Medium Severity
isSweepCandidate mirrors sidebar settlement via effectiveSettled but never checks effectiveSnoozed. SidebarV2 keeps snoozed threads on a separate shelf out of the active unsettled list, so the sweep still reviews, recommends, and can settle/merge work the user explicitly hid.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 900a40e. Configure here.
| shell.session?.status === "running" || | ||
| hasQueuedTurnStart(shell, DateTime.toEpochMillis(now)); | ||
| const canSettleNow = input.canSettleNow && !serverSideActive; | ||
| const isActive = !canSettleNow; |
There was a problem hiding this comment.
Client flag marks idle threads active
Medium Severity
isActive is derived as !canSettleNow, so a client canSettleNow: false forces the prompt’s ACTIVE rules even when the server’s own projection shows the thread idle. That suppresses settle recommendations and can produce misleading next-step guidance for settleable threads.
Reviewed by Cursor Bugbot for commit 900a40e. Configure here.
| mergeable === true && | ||
| checksPassing !== false && | ||
| reviewDecision !== "CHANGES_REQUESTED" && | ||
| reviewDecision !== "REVIEW_REQUIRED"; |
There was a problem hiding this comment.
Unknown CI still marks merge-ready
Medium Severity
toPrStatus sets mergeReady when checksPassing !== false, so a null/unknown CI rollup still counts as merge-ready. The contract describes mergeReady as requiring CI green, and the sweep will offer Merge & settle / Merge all for those PRs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 900a40e. Configure here.
The server's textGenerationModelSelection is tuned for commit messages and PR bodies — the wrong default for reasoning over a thread. The sweep now sends an explicit modelSelection, defaulting to the composer picker's sticky (last-used) model, and the launch modal embeds the same ProviderModelPicker so it can be changed per run. The server falls back to its text-gen setting only when a client sends no selection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| }), | ||
| ), | ||
| ); | ||
| if (!mergeResult.merged) { |
There was a problem hiding this comment.
🟠 High review/ReviewService.ts:527
mergePullRequest returns outcome: "merged" whenever gh pr merge --squash exits successfully, but with a GitHub merge queue that command can enqueue the PR and exit while the PR is still open. The downstream caller then settles the thread even though the merge may later fail or conflict. Consider re-querying the PR with gh pr view after the merge command and only returning "merged" when the PR state is actually MERGED (or surfacing a separate "queued" outcome).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/review/ReviewService.ts around line 527:
`mergePullRequest` returns `outcome: "merged"` whenever `gh pr merge --squash` exits successfully, but with a GitHub merge queue that command can enqueue the PR and exit while the PR is still open. The downstream caller then settles the thread even though the merge may later fail or conflict. Consider re-querying the PR with `gh pr view` after the merge command and only returning `"merged"` when the PR state is actually `MERGED` (or surfacing a separate `"queued"` outcome).
| return [...candidates].sort((a, b) => activityMs(b) - activityMs(a)); | ||
| } | ||
|
|
||
| async function reviewOne(runId: number, item: SweepItem): Promise<void> { |
There was a problem hiding this comment.
🟡 Medium src/reviewSweepStore.ts:224
reviewOne sends the same readSweepModelSelection() value to every target environment, but that selection originates from the primary environment's composer picker. Provider instance IDs are server-local, so when a secondary environment doesn't have that instance the review fails instead of falling back to that server's own default model. The same path also fails when the sticky instance was deleted from the primary server after being selected. Validate the selection against the target environment's available models, or omit modelSelection so each server uses its own default when the instance is unavailable.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/reviewSweepStore.ts around line 224:
`reviewOne` sends the same `readSweepModelSelection()` value to every target environment, but that selection originates from the primary environment's composer picker. Provider instance IDs are server-local, so when a secondary environment doesn't have that instance the review fails instead of falling back to that server's own default model. The same path also fails when the sticky instance was deleted from the primary server after being selected. Validate the selection against the target environment's available models, or omit `modelSelection` so each server uses its own default when the instance is unavailable.
| threadId: item.ref.threadId, | ||
| canSettleNow, | ||
| ...(modelSelection !== null ? { modelSelection } : {}), | ||
| }, |
There was a problem hiding this comment.
Cross-env model selection breaks reviews
Medium Severity
The sweep now applies a single client-derived modelSelection to all environments. Since ProviderInstanceId is server-specific, environments lacking that instance fail summarizeThread with a TextGenerationError instead of using their own default text-gen settings.
Reviewed by Cursor Bugbot for commit a8c3047. Configure here.
The modal exposed only the model, so reasoning effort, thinking, and fast mode weren't reachable — and worse, picking a model called createModelSelection without options, silently clearing whatever traits the sticky selection carried. The modal now renders the composer's TraitsPicker beside the model picker, and a model switch preserves the current options when the instance is unchanged (a different instance's options aren't transferable, so those start clean). No server change needed: the provider CLIs already derive effort/thinking/fastMode/ultracode from modelSelection.options. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| model, | ||
| instanceId === activeInstanceId ? activeOptions : undefined, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Unsanitized traits survive model switches
Medium Severity
Same-instance model changes reuse raw activeOptions in createModelSelection without the sanitization resolveAppModelSelectionState / getComposerProviderState apply in settings text-gen. Invalid effort or other trait values can remain in the stored sweep selection and be sent on summarizeThread, while the Traits UI may already show the resolved default.
Reviewed by Cursor Bugbot for commit 8c9dbfc. Configure here.
The in-progress view was a spinner and a counter, which wasted the most interesting moment of a multi-minute sweep. It now shows a progress bar plus a live list of every thread being reviewed — running ones first — with project favicon, a per-thread elapsed clock that ticks while the review runs, and the tokens and cost each review actually consumed. The header aggregates run totals. Usage is real, not estimated: the Claude CLI's `--output-format json` envelope already reports `usage` and `total_cost_usd`, so runClaudeJson now surfaces them through an optional onUsage sink, generateThreadReview attaches them to its result, and the review RPC returns them as an optional `usage` field. Input counts fold in cache-creation and cache-read tokens so the number matches billing. Providers that report nothing simply omit the field. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| : {}), | ||
| durationMs: Math.round(envelope.duration_api_ms ?? 0), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Zero usage when absent
Low Severity
The onUsage callback in runClaudeJson defaults missing token/duration telemetry to zero. This causes generateThreadReview to always attach a usage object, even when the Claude CLI reports no telemetry, leading to "0 tok" in the UI where usage should be absent.
Reviewed by Cursor Bugbot for commit f70a4e5. Configure here.
| cache_read_input_tokens: Schema.optionalKey(Schema.NullOr(Schema.Number)), | ||
| }), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Strict Claude usage schema
Medium Severity
ClaudeOutputEnvelope now schema-validates optional usage, total_cost_usd, and duration_api_ms. That decoder is shared by every Claude JSON text-gen op, so a CLI envelope whose telemetry fields are present but differently typed or shaped fails the whole decode as “unexpected output format,” including commit/PR/title generation that previously ignored those extras. Elsewhere Claude usage is soft-parsed for the same reason.
Reviewed by Cursor Bugbot for commit f70a4e5. Configure here.
Results were ephemeral module state, so a reload or accidental navigation threw away a multi-minute (and real-money) run. The store now persists to localStorage. In-flight RPCs can't survive a reload, so pending/running items rehydrate as retry-able errors and an interrupted run reports as complete — the header offers Re-run rather than a progress bar that would never advance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The results view used ad-hoc chrome: colored left-border card stripes, uppercase micro-headers, and raw-div lists. Restyled to the same patterns as the settings pages: section headers with tinted icon chips and a title/hint stack, standard Card surfaces with the app's title tracking and 13px muted body copy, hover-revealed card actions, the settings scroll-fade container, and the progress list inside a Card with row dividers. Cards drop the accent stripe — the section a card sits in already says what kind it is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| <Button | ||
| size="xs" | ||
| variant="ghost" | ||
| disabled={running} |
There was a problem hiding this comment.
🟡 Medium reviewSweep/ReviewSweepView.tsx:814
The Re-run button stays enabled during a merge, so clicking it mid-merge leaves successfully merged PRs unsettled. mergeOne sends the merge request, then checks the stale-run guard after startReviewSweep bumps runId; the guard aborts before the follow-up settle runs, so the merged thread never settles and its merge status is lost from the UI. Disable Re-run while any merge or apply operation is active — not just while phase is "running".
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/reviewSweep/ReviewSweepView.tsx around line 814:
The **Re-run** button stays enabled during a merge, so clicking it mid-merge leaves successfully merged PRs unsettled. `mergeOne` sends the merge request, then checks the stale-run guard after `startReviewSweep` bumps `runId`; the guard aborts before the follow-up settle runs, so the merged thread never settles and its merge status is lost from the UI. Disable Re-run while any merge or apply operation is active — not just while `phase` is `"running"`.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 15 total unresolved issues (including 14 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ec34a6f. Configure here.
| ...restored, | ||
| items, | ||
| phase: restored.phase === "running" ? "complete" : restored.phase, | ||
| }; |
There was a problem hiding this comment.
Stuck merge cards after reload
Medium Severity
The new persist merge recovers interrupted review RPCs (pending/running → error) but leaves mergeStatus of merging (and related in-flight merge lifecycle) untouched. After a reload, mergeQueueActive is gone, per-card Merge only appears for idle/failed, and Merge all skips merging, so those cards can stay on a permanent Merging spinner with no retry path short of Re-run.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ec34a6f. Configure here.
|
Closing in favor of #2829 (orchestration V2). #2829 deletes the V1 orchestration layer this PR builds on — This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them. Once #2829 merges, please rebase onto |
2 participants
Summary
Adds a one-click "Review unsettled work" sweep to sidebar v2. Clicking the new checklist button in the sidebar header opens
/review-sweepand kicks off an agentic pass over every unsettled, unarchived thread across all connected environments:Results stream in progressively (client-side pool, concurrency 3, 50-thread cap favoring most-recently-active). Each card has Apply title / Settle / Dismiss / Retry; the header has Apply All. Nothing applies without a click. Results are ephemeral per run — no new persisted thread state.
Mock/design doc: https://b8x4s2sr11ko.postplan.dev
Current state:
Implementation
ReviewThreadSummaryInput/Result+ReviewThreadNotFoundError, newreview.summarizeThreadWS RPC, newreviewSweepcapability flag so clients skip pre-feature servers under version skew.generateThreadReviewop on theTextGenerationservice, implemented in all five provider backends (Claude/Codex/Cursor/Grok/OpenCode) sharingbuildThreadReviewPrompt(transcript capped to last 20 non-streaming messages, ~2k chars each) and anormalizeThreadReviewsanitizer.ReviewService.summarizeThreadreads the transcript viaProjectionSnapshotQueryand uses the server'stextGenerationModelSelection.summarizeThreadRPC command atom in the review atom group.reviewSweepStore.ts(non-persisted Zustand store + module-level runner withrunIdstaleness guards; sweeps survive navigation),/review-sweeproute +ReviewSweepView, SidebarV2 header button. Applying reuses the existingthread.meta.updateandthread.settlecommands.Settle-safety invariant
A running / awaiting-approval / awaiting-input thread can never be settled through this feature, enforced at four independent layers:
recommendSettle must be false)normalizeThreadReviewforces it off for active threadscanSettleNowcanSettleagainst the live shell at apply timeTesting
ReviewService.summarizeThread(not-found, transcript extraction/streaming exclusion, stale-canSettleNowserver override),buildThreadReviewPrompt(caps, active-thread rule),normalizeThreadReview(sanitization, placeholder-title drop)canSettleNow; cap sorting bycreatedAtinstead of last activity) fixed with regression coverage🤖 Generated with Claude Code
Note
Medium Risk
Touches thread transcripts sent to external LLMs from resolved worktrees, GitHub merge via
gh, and settle recommendations—mitigated by server-side activity guards and click-to-apply, but merge/settle mistakes or prompt leakage remain possible if guards drift.Overview
Introduces an end-to-end work review sweep for unsettled threads: the web app collects candidates (aligned with sidebar settlement rules and mirrored PR state), runs up to 50 reviews concurrently in the background, and triages results on
/review-sweepinto buckets (merge-ready, settle, title fix, attention, etc.) with opt-in apply actions.Server & contracts: Advertises a
reviewSweepcapability and wiresreview.summarizeThread/review.mergePullRequestover WS.ReviewService.summarizeThreadloads the thread from projections, resolves a strict workspacecwd(noprocess.cwd()fallback), re-derives activity (pending approvals/input, session state, queued turn start) so settle recommendations cannot trust the client alone, optionally enriches viagh pr view, calls newgenerateThreadReviewacross text-generation backends (shared prompt +normalizeThreadReview), and returns summary,nextStep, title/settle hints, checkpoint diff stats, PR status, and optional usage.mergePullRequestre-validates live GitHub state thengh pr merge --squash, returning structured outcomes (merged, conflict, not-ready, already-closed).Web:
reviewSweepStore(persisted run state, model picker defaulting to composer sticky selection), launch dialog + sidebar checklist control, progress/toasts, apply-all / per-card settle & rename, and a sequential merge queue that settles on success andstartTurnhandoff on conflicts.Reviewed by Cursor Bugbot for commit ec34a6f. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add 'review unsettled threads' sweep view with AI summarization and PR merge support
/review-sweeproute andReviewSweepViewUI that groups unsettled threads into actionable buckets (merge-ready, settle, title, attention, failed, etc.) accessible via a new sidebar toolbar button.summarizeThreadandmergePullRequestRPCs toReviewService, backed by a newgenerateThreadReviewtext-generation operation implemented across all AI providers (Claude, Codex, Cursor, Grok, OpenCode).summarizeThreadbuilds a structured prompt from the thread transcript, optional PR context fetched viagh pr view, and diff stats; returns a summary, next step, suggested title, and settle recommendation.mergePullRequestvalidates live PR state before invokinggh pr merge --squash, returning structured outcomes:merged,conflict,already-closed, ornot-ready; on conflict, a rebase instruction is posted to the thread.canSettleNowcan be overridden; stale runId guards prevent cross-run state corruption but require careful sequencing.Macroscope summarized ec34a6f.
Summary by CodeRabbit
New Features
Bug Fixes