Fix ADE Claude Agent SDK chats to behave like Claude Code#789
Conversation
Root-caused against real chat transcripts and covered by tests.
Service (chat/agentChatService.ts):
- Stop phantom subagent rows: emit subagent_result only when its
subagent_started was emitted — fixes the ~99-subagent over-count and the
wall of "stopped — interrupted" cards on interrupt; clear taskId+agentId
aliases on terminal.
- Query lifecycle: single-flight query start + a queryGeneration token that
aborts+reaps a start superseded by reset/interrupt (no twin claude
--resume subprocess, no resurrected turn); reap on reset AND interrupt.
- Background tasks survive turn boundaries (only interrupt/reset/dispose
stop them); sticky titles; run_in_background Bash classified as background.
- Fix assistant-text doubling via a durable per-(messageId,index) record.
- Fix ghost todo row: ordinal TaskUpdate id remaps onto the Nth created
task; never fabricate a row from a bare id.
- Steer contract returns { steerId, queued, reason }: queue-full no longer
silently drops the message; reasoning effort applied at delivery;
interrupt-replace acks only after Claude accepts.
- Share isNonAgentTaskRun so idle and foreground classify identically.
Renderer: split mid-turn Send button (Send now delivers into the running
turn on the exact returned steerId; caret = Queue / Interrupt & replace)
with always-on tooltips; interrupt-stopped subagent cards fold into one
group card; Chat Info "Completed" bucket + Clear-on-expand; wider 2-line
subagent cards.
Parity: iOS (grouping, Completed drawer, steer queue-full, local_bash) and
the ADE Code TUI kept in lockstep; internal docs updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe PR updates Claude steering result propagation and queue-full handling, refines Claude task lifecycle and transcript emission, groups interrupted subagents in desktop and iOS timelines, and changes roster controls from “Earlier” to “Completed”. ChangesClaude steering and runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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 |
arul28
changed the title
|
@copilot review but do not make fixes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift (1)
2685-2687: 📐 Maintainability & Code Quality | 🔵 TrivialUnused
earlier/hiddenparameters insectionHint.The simplified hint no longer references
earlierorhidden. Harmless but dead signature surface.🤖 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/ios/ADE/Views/Work/WorkChatRichCardViews.swift` around lines 2685 - 2687, Remove the unused earlier and hidden parameters from sectionHint, then update every call site to pass only active, running, and failed while preserving the existing hint output.apps/desktop/src/renderer/components/chat/ChatSubagentsPanel.test.tsx (1)
549-549: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename stale variable
earlierToggleto match the "Completed" bucket.The variable retrieves the "Completed (1)" button but retains the old
earlierTogglename from before the "Earlier" → "Completed" rename, which could confuse future readers.♻️ Proposed rename
- const earlierToggle = screen.getByRole("button", { name: "Completed (1)" }); + const completedToggle = screen.getByRole("button", { name: "Completed (1)" });🤖 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/desktop/src/renderer/components/chat/ChatSubagentsPanel.test.tsx` at line 549, Rename the local variable earlierToggle to a name matching the Completed bucket, such as completedToggle, and update all references to it within the relevant test.
🤖 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/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 9028-9047: Update armAndSubmitSteerDispatch to handle rejection
from window.ade.agentChat.dispatchSteer instead of discarding it with void.
Surface the failure to the user through the existing error-reporting or
notification mechanism, while preserving the current queued-result guard and
dispatch parameters for successful submissions.
- Around line 8726-8730: Update sendMessageOrSteerIfBusy() to capture and return
the AgentChatSteerResult produced by steerMessage() when send() throws an
isTurnAlreadyActiveError, so submit() receives the fallback result and
dispatchSteer executes the queue_full restore/notice path. Add a test covering
send() rejecting with turn-already-active while turnActiveBySession[sessionId]
remains false.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/ChatSubagentsPanel.test.tsx`:
- Line 549: Rename the local variable earlierToggle to a name matching the
Completed bucket, such as completedToggle, and update all references to it
within the relevant test.
In `@apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift`:
- Around line 2685-2687: Remove the unused earlier and hidden parameters from
sectionHint, then update every call site to pass only active, running, and
failed while preserving the existing hint output.
🪄 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: CHILL
Plan: Pro
Run ID: 34029e50-f14e-4a5f-b7c5-e484e3e124c5
⛔ Files ignored due to path filters (5)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**
📒 Files selected for processing (36)
apps/ade-cli/src/headlessLinearServices.tsapps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsxapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/claudeAssistantTextDedup.test.tsapps/desktop/src/main/services/chat/claudeQueryLifecycle.test.tsapps/desktop/src/main/services/chat/claudeSubagentResultGate.test.tsapps/desktop/src/main/services/chat/claudeTaskTodos.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatSubagentsPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatSubagentsPanel.tsxapps/desktop/src/renderer/components/chat/SubagentActivityCards.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/renderer/webclient/adapter/agentChat.tsapps/desktop/src/shared/chatSubagents.test.tsapps/desktop/src/shared/chatSubagents.tsapps/desktop/src/shared/types/chat.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Timeline.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
- sendMessageOrSteerIfBusy: capture steerMessage()'s result in the turn-already-active fallback so a stale-turn-active race still flows through the queue_full restore/notice path (was dropped, clearing the draft as if the message had sent). - dispatchSteer/editSteer are fire-and-forget IPC — surface a rejection via a composer notice (shared dispatchSteerSafely helper) instead of silently swallowing it, across every Send-now / Interrupt & replace / staged-row / edit site. - Regression test for the stale-turn-active + queue_full fallback path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cf088a062
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await call<AgentChatSteerResult>("chat.steer", args, { | ||
| steerId: globalThis.crypto.randomUUID(), | ||
| queued: true, | ||
| }, false); |
There was a problem hiding this comment.
When the web-client adapter cannot execute chat.steer (missing descriptor, disconnected host, host_unavailable, etc.), this fallback fabricates a new steerId and returns queued: true even though no command was sent and there is no offline queue for this path. In the live Vite/web renderer, submitting during an active turn will then clear the draft and, for “Send now”, try to dispatch a non-existent steer id, silently losing the user's message instead of preserving it or surfacing the connection error.
Useful? React with 👍 / 👎.
| if (managed.session.provider === "claude") { | ||
| managed.session.reasoningEffort = normalizeReasoningEffort(managed.session.reasoningEffort); | ||
| managed.session.interactionMode = nextSteer.interactionMode ?? managed.session.interactionMode ?? "default"; |
There was a problem hiding this comment.
This delivery path persists nextSteer.reasoningEffort when the steer is queued, but at turn delivery it normalizes the session's current value instead of applying the captured value. If a client queues an active-turn steer with a specific reasoning effort (for example via chat.send routed through routeActiveToSteer) and the session effort changes before the current turn finishes, the queued turn runs with the later session setting rather than the per-message setting the queue stored.
Useful? React with 👍 / 👎.
… effort - The web-client steer adapter no longer fabricates queued:true when chat.steer can't execute (host unavailable / missing descriptor). It throws instead, so the composer restores the draft and surfaces the connection error rather than silently losing the message and dispatching a steer id that never existed. - deliverNextQueuedSteer applies the queued steer's captured reasoning effort (mirroring the execution/interaction modes right below) instead of the session's current value, so a queued steer honors the effort chosen for it even if the session effort changes before delivery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 22688-22694: Update deliverNextQueuedSteer where it applies
nextSteer.reasoningEffort to detect an effort change and invalidate/reset the
existing Claude query before runClaudeTurn executes. Mirror the reasoning-effort
invalidation behavior used by the normal send path, while preserving query reuse
when the normalized effort remains unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 7e060a94-3238-4a30-ad97-43308b128af0
📒 Files selected for processing (4)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/webclient/adapter/agentChat.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/components/chat/AgentChatPane.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
| // Apply the reasoning effort captured when the steer was queued (the same | ||
| // way the execution/interaction modes below are), so a queued steer runs | ||
| // at the effort the user chose for it, not a later session change. | ||
| managed.session.reasoningEffort = normalizeReasoningEffort(nextSteer.reasoningEffort ?? managed.session.reasoningEffort); | ||
| managed.session.interactionMode = nextSteer.interactionMode ?? managed.session.interactionMode ?? "default"; | ||
| managed.session.permissionMode = syncLegacyPermissionMode(managed.session) ?? managed.session.permissionMode; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='apps/desktop/src/main/services/chat/agentChatService.ts'
# Show the relevant region around the queued steer handling
sed -n '22620,22740p' "$FILE"
printf '\n---\n'
# Find the Claude query lifecycle helpers and effort handling
rg -n "buildClaudeQueryOptions|ensureClaudeQuery|resetClaudeQuerySession|pendingSessionReset|reasoningEffort|setPermissionMode|warmQuery|query =" "$FILE"Repository: arul28/ADE
Length of output: 23373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='apps/desktop/src/main/services/chat/agentChatService.ts'
printf '%s\n' '--- around 28600 ---'
sed -n '28590,28655p' "$FILE"
printf '\n%s\n' '--- around 32090 ---'
sed -n '32090,32175p' "$FILE"
printf '\n%s\n' '--- around 22680 ---'
sed -n '22680,22705p' "$FILE"Repository: arul28/ADE
Length of output: 8858
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='apps/desktop/src/main/services/chat/agentChatService.ts'
printf '%s\n' '--- buildClaudeQueryOptions ---'
sed -n '22100,22330p' "$FILE"
printf '\n%s\n' '--- ensureClaudeQuery ---'
sed -n '22520,22620p' "$FILE"
printf '\n%s\n' '--- runClaudeTurn entry ---'
sed -n '14590,14670p' "$FILE"Repository: arul28/ADE
Length of output: 20784
Reset the Claude query when a queued steer changes reasoning effort.
deliverNextQueuedSteer updates managed.session.reasoningEffort, but runClaudeTurn can reuse an existing live/warm Claude query whose effort was baked in at creation. Mirror the effort-change invalidation used in the normal send path so the queued steer actually runs with the captured effort.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/desktop/src/main/services/chat/agentChatService.ts` around lines 22688 -
22694, Update deliverNextQueuedSteer where it applies nextSteer.reasoningEffort
to detect an effort change and invalidate/reset the existing Claude query before
runClaudeTurn executes. Mirror the reasoning-effort invalidation behavior used
by the normal send path, while preserving query reuse when the normalized effort
remains unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 497f59c52c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function isInterruptStoppedResultCard( | ||
| event: ChatTranscriptGroupedEnvelope["event"], | ||
| ): event is SubagentResultCardRenderEvent { | ||
| return event.type === "subagent_result_card" && event.status === "stopped"; |
There was a problem hiding this comment.
This treats every status === "stopped" result as an interrupt casualty, but the runtime also emits stopped results when a workflow ends with agents still running (for example, it emits status: "stopped" with the summary “Workflow ended before this agent finished.”). When two of those workflow/script-stopped agents are consecutive, their individual reason gets collapsed into the “agents stopped when you interrupted” card (the iOS mirror uses the same predicate), which misattributes the failure and hides the actionable summary. Please distinguish actual interrupt-generated stops before folding them.
Useful? React with 👍 / 👎.
… rebuild Revert the per-queued-steer reasoning-effort change. Honoring a captured per-steer effort requires tearing down the live Claude query mid-delivery, which disrupts the delivery flow (broke 3 steer tests) for a rare edge (queue a steer, then change the session effort before the turn ends). The delivered steer runs at the session's current effort by design; the reason is now documented inline instead of a bare self-normalize. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the review findings:
Two findings deliberately deferred (low-value edges, documented inline):
|
1 participant
Makes ADE's Claude chat surface behave like Claude Code, backed by the Agent SDK. Every fix was root-caused against real chat transcripts and is covered by tests (1037 desktop chat tests, 1699 CLI, 859 TUI, iOS build green).
Why
ADE Claude chats were bugging out in ways that made the SDK look broken when the harness was actually at fault: a roster claiming ~99 subagents, a wall of ~50 "stopped — interrupted" cards on a single interrupt, doubled assistant text, a phantom
1todo, background jobs falsely stopped at every turn boundary, and a mid-turn send that queued instead of steering the live turn.Service (
chat/agentChatService.ts)subagent_resultonly when itssubagent_startedwas emitted (emittedSubagentStartIds). Fixes the ~99-count and the interrupt card-wall; clears bothtaskIdandagentIdaliases on terminal.queryStartPromise) + aqueryGenerationtoken that aborts+reaps a start superseded by reset/interrupt (no twinclaude --resumesubprocess, no resurrected turn). Reap on reset and interrupt; orphansystem_noticewhen a reset stops open background tasks.run_in_backgroundBash classified as background.(messageId, contentIndex)emitted-text record.TaskUpdateid remaps onto the Nth created task; never fabricate a row from a bare id.{ steerId, queued, reason }— queue-full no longer silently drops the message; reasoning effort applied at delivery; interrupt-replace acks only after Claude accepts.isNonAgentTaskRunso idle and foreground paths classify identically.Renderer
steerId(Claude Code parity); caret menu = Queue / Interrupt & replace; hover tooltips always on regardless of the tooltip setting.Parity
iOS (grouping, Completed drawer, steer queue-full
.dropped,local_bash) and the ADE Code TUI (Completed drawer, steer queue-full) kept in lockstep. Internal docs updated.Verification
/quality(0 blockers; steer-race, queue-full-drop, query-race, and idle/foreground divergence fixed and re-reviewed) →/test(regression tests for each contract; docs/mobile/CLI/TUI parity). Deferred low-probability edges are noted in the review.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR makes Claude Agent SDK chats behave more like Claude Code. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
No blocking issues found in the changed code.
No files need attention.
What T-Rex did
Important Files Changed
Reviews (4): Last reviewed commit: "Steer delivery: keep session-effort beha..." | Re-trigger Greptile