fix(clients): reconcile tool activity lifecycles - #7617
Conversation
|
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:
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
maria-rcks
changed the title
Thread transfer impact
This comment will update automatically after the next completed run. |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces significant new runtime behavior for tool activity lifecycle reconciliation with complex merging logic. Two Medium severity findings identify bugs in the lifecycle merging code (duplicate completions from approval.requested rows, parallel tools merging incorrectly). The scope and complexity of the changes warrant careful human review. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
c5101c8 to
29acbf9
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| (candidate) => | ||
| candidate.activityKind !== "tool.completed" && | ||
| candidate.turnId === previous.turnId && | ||
| candidate.itemType === previous.itemType && |
There was a problem hiding this comment.
🟡 Medium src/session-logic.ts:1146
An earlier approval.requested row can make an id-less tool.completed appear ambiguous, so the keyed tool.started remains visible as running and a duplicate completion row is emitted. The candidate check must consider only tool.started and tool.updated rows as competing lifecycle targets.
- candidate.activityKind !== "tool.completed" &&
+ (candidate.activityKind === "tool.started" ||
+ candidate.activityKind === "tool.updated") &&🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/session-logic.ts around line 1146:
An earlier `approval.requested` row can make an id-less `tool.completed` appear ambiguous, so the keyed `tool.started` remains visible as running and a duplicate completion row is emitted. The candidate check must consider only `tool.started` and `tool.updated` rows as competing lifecycle targets.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
29acbf9 to
befa511
Compare
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 befa511. Configure here.
| return true; | ||
| } | ||
| return ( | ||
| (previous.activityKind !== "tool.started" || next.activityKind === "tool.completed") && |
There was a problem hiding this comment.
Id-less updates merge parallel tools
Medium Severity
The new id-less collapse rule attaches an unkeyed tool.updated to the previous keyed row whenever label and itemType match. The competing-target check only runs for tool.completed, so a parallel same-label update without a call id can fuse into the adjacent call and hide a distinct in-flight tool.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit befa511. Configure here.
There was a problem hiding this comment.
Reviewed the changed web UI files for shared-primitive usage, Tailwind/CSS ownership, and accessibility. The new live-activity row, tool-group summary row, and the live-activity-focus mask/animation utilities look consistent with the existing timeline row idiom (the mask + keyframes are exactly the kind of behaviorally complex CSS that belongs in index.css), and the AgentSpawnCtaRow rework correctly routes its action through the shared Button/Tooltip primitives.
Two findings, both in MessagesTimeline.test.tsx, where the tests no longer match the component contract this PR introduces.
Posted via Macroscope — UI Consistency
| expect(markup).toContain("Thinking"); | ||
| expect(markup).not.toContain("Thinking ·"); | ||
| expect(markup).not.toContain('aria-hidden="true" class="size-6 shrink-0"'); | ||
| expect(markup).toContain("gap-1.5 px-0.5 py-0.5"); |
There was a problem hiding this comment.
This assertion doesn't match what the iconless Thinking row emits. LiveActivityContent builds cn("flex min-h-6 min-w-0 items-center gap-1.5 py-0.5", resolvedIconName ? "px-0.5" : "px-1", …), and with no icon (ThinkingActivityRow) the rendered order is gap-1.5 py-0.5 px-1 — px-0.5 is only emitted when an icon is present. cn (twMerge) does not reorder classes, so this test fails as written; the later "aligns the iconless Thinking row" test asserts the correct string.
| expect(markup).toContain("gap-1.5 px-0.5 py-0.5"); | |
| expect(markup).toContain("gap-1.5 py-0.5 px-1"); |
Posted via Macroscope — UI Consistency
| isWorking | ||
| activeTurnInProgress | ||
| activeTurnStartedAt={MESSAGE_CREATED_AT} |
There was a problem hiding this comment.
activeTurnInProgress was removed from MessagesTimelineProps in this PR, so this explicit JSX attribute no longer type-checks (Property 'activeTurnInProgress' does not exist on type 'IntrinsicAttributes & MessagesTimelineProps'). Consider dropping it — the row now derives its state from showThinking.
| isWorking | |
| activeTurnInProgress | |
| activeTurnStartedAt={MESSAGE_CREATED_AT} | |
| isWorking | |
| activeTurnStartedAt={MESSAGE_CREATED_AT} |
Posted via Macroscope — UI Consistency
1 participant
What changed
Moves the deeper behavior out of #7152 into a follow-up layer:
Why
The one-line tool UI can be reviewed and merged independently. This PR contains the turn, lifecycle, and mobile behavior needed for complete provider histories.
Validation
Built with GPT-5.6-sol in the Codex harness.
Note
Reconcile tool activity lifecycles by
toolCallIdand overhaulMessagesTimelinerowstool.started,tool.updated,tool.completed) now collapse by stabletoolCallIdscoped to a turn, preventing cross-turn merges and ambiguous id-less completions. Applies to both session-logic.ts and threadActivity.ts.deriveWorkLogEntriesnow surfacestool.startedrows (previously blanket-skipped), filters Codex terminal interactions and unkeyed statusless starts, and infersinProgress/completedstatuses when missing.work-livefor active expandable tool batches, enrichedwork-togglewith summaries and failure icons, andworkingrows with ashowThinkingflag. Turn folds anchor near the assistant response when collapsed and above the first hidden entry when expanded.tokenizeShellCommand,commandProgramName) extracts program names for live command labels;workEntryIconNamedelegates totoolGroupActionfor icon selection.deriveActivePlanStateremoved;activeComposerTaskStepsnow sourced fromturnPlansfor the active turn.MessagesTimelineno longer acceptsworkingStepLabeloractiveTurnInProgressprops.MessagesTimelineprop interface removesworkingStepLabelandactiveTurnInProgress; any external callers passing these props will break.deriveActiveWorkStartedAtsignature gainslatestUserMessageAt(defaults tonull), andderiveActivePlanStateis deleted from session-logic.ts.Macroscope summarized befa511.
Note
Medium Risk
Changes are concentrated in client-side timeline and work-log derivation with broad test coverage, but incorrect collapse or fold anchoring could misrepresent tool history or hide activity in edge cases (parallel id-less tools, steers).
Overview
Tool lifecycle reconciliation on web and mobile collapses
tool.started/tool.updated/tool.completedbyturnId+toolCallId, including interleaved and late events, while refusing to merge ambiguous id-less completions or reuse IDs across turns. Terminal outcomes (completed, failed, etc.) win over stale in-progress updates.Work log and timeline presentation now surfaces in-flight
tool.startedrows, filters duplicate plan updates, unkeyed statusless starts, Codex PTY terminal-input noise, andExitPlanModemarkers. Settled turn folds anchor before the next visible assistant reply when collapsed (fixes steer-superseded turns) and before the first hidden entry when expanded. Live tool batches stay expandable as entries append; lifecycle marker filtering andsummarizeToolGrouplabeling improve settled overflow rows.Web chat drops
deriveActivePlanState/workingStepLabel; composer task steps come fromturnPlansfor the active turn. The working row no longer shows plan step text. Subagent spawn UI shows coordinator failure states and uses a compact card with an Eye action instead of a full-width button.Failure display splits metadata vs rendered output via
workEntryDisplayIndicatesToolFailureso command text likerg 'command not found'does not show a false failure while still running.Reviewed by Cursor Bugbot for commit befa511. Bugbot is set up for automated code reviews on this repo. Configure here.