feat(chat): make Steer messages IM-style - #2070
Conversation
📝 WalkthroughWalkthroughThis change implements durable IM-style Steer messages. Steers persist as transcript messages, support batching and recovery, reuse ACP projections, publish renderer updates, expose read receipts, and keep pending-input UI focused on Queue items. ChangesDurable Steer message lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/session/chatService.ts (1)
185-191: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace the non-null assertion on
result.userMessagewith an explicit check.
result.userMessageis optional on the turn result. The assertion at line 187 silences that, somessagecan beundefinedat runtime. The route contract insrc/shared/contracts/routes/chat.routes.tsrequiresmessage: ChatMessageRecordSchemaforaccepted: true. An absentuserMessagetherefore fails Zod validation at the IPC boundary and reports a schema error instead of the real cause.The current reject branch only covers
attachmentPreparation?.status === 'needs_user_action'. Any other outcome that produces no persisted user message reaches this path.Fail fast with a clear error, or map the missing message to the rejected variant.
🐛 Proposed fix
if (result.attachmentPreparation?.status === 'needs_user_action') { return { accepted: false, message: null, attachmentPreparation: result.attachmentPreparation } } + if (!result.userMessage) { + throw new Error(`Steer accepted without a persisted user message: ${sessionId}`) + } + return { accepted: true, - message: result.userMessage!, + message: result.userMessage, ...(result.attachmentPreparation ? { attachmentPreparation: result.attachmentPreparation } : {}) }🤖 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 `@src/main/session/chatService.ts` around lines 185 - 191, Replace the non-null assertion in the accepted result branch of chatService with an explicit result.userMessage check. When the message is absent, fail clearly or return the rejected variant so accepted responses always contain a valid ChatMessageRecord; preserve the existing attachmentPreparation handling for valid messages.test/main/session/session.integration.test.ts (1)
2388-2405: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winConfirm the mocked
steerPendingInputresponses match the production flow.
convertPendingInputToSteerdelegates directly tosteerPendingInput, and both routes callsteerPendingInputonce. If the real route path does not invoke this twice for one user action, this test should use the same resolved value for both calls.🤖 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 `@test/main/session/session.integration.test.ts` around lines 2388 - 2405, The test’s mocked steerPendingInput responses must reflect the production call flow. Review convertPendingInputToSteer and steerPendingInput, then update the mock setup and assertions so each user action expects the actual number of calls and uses the same resolved value for both calls when production invokes them equivalently.
🧹 Nitpick comments (3)
src/main/session/data/transcript.ts (1)
285-325: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse the record you already read instead of a second read pass.
Both methods read each message twice. The loop already produces
updatedfor every id, then line 311 and line 324 read every message again throughrequireMessage. Collect the records in the loop and return them.♻️ Proposed change
markSteerMessagesRead(messageIds: string[], readAt: number): ChatMessageRecord[] { + const updatedRecords: ChatMessageRecord[] = [] for (const messageId of messageIds) { @@ const updated = this.getMessage(messageId) if (!updated) { throw new Error(`Failed to mark steer message read: ${messageId}`) } this.tapeFacts.appendMessageReplacement(updated, 'steer_message_read') + updatedRecords.push(updated) } - return messageIds.map((messageId) => this.requireMessage(messageId)) + return updatedRecords } settleSteerMessages(messageIds: string[]): ChatMessageRecord[] { + const updatedRecords: ChatMessageRecord[] = [] for (const messageId of messageIds) { @@ const updated = this.requireMessage(messageId) this.tapeFacts.appendMessageReplacement(updated, 'steer_message_settled') + updatedRecords.push(updated) } - return messageIds.map((messageId) => this.requireMessage(messageId)) + return updatedRecords }🤖 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 `@src/main/session/data/transcript.ts` around lines 285 - 325, Update markSteerMessagesRead and settleSteerMessages to collect each successfully retrieved updated record in an array during the existing loop, then return that array directly instead of mapping messageIds through requireMessage for a second read.src/main/session/data/pendingInputStore.ts (1)
536-545: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog invalid
message_ids_jsonbefore falling back.
decodePayloadanddecodeBlockinglog corrupt JSON.decodeMessageIdsreturns[]silently. A silent fallback drops the transcript link. The steer item then fails the claim check with "has no linked messages", and restart recovery creates a second user message for the same input. A log line makes that state diagnosable.♻️ Proposed logging
private decodeMessageIds(row: DeepChatPendingInputRow): string[] { try { const parsed = JSON.parse(row.message_ids_json) as unknown return Array.isArray(parsed) ? parsed.filter((value): value is string => typeof value === 'string' && value.length > 0) : [] - } catch { + } catch (error) { + console.error( + `[DeepChatPendingInputStore] Invalid pending input message IDs JSON: ${row.id}`, + error + ) return [] } }🤖 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 `@src/main/session/data/pendingInputStore.ts` around lines 536 - 545, Update decodeMessageIds in the JSON.parse error path to log that row.message_ids_json is invalid before returning the existing empty array fallback, matching the diagnostics used by decodePayload and decodeBlocking. Keep valid-array filtering and non-array behavior unchanged.src/main/session/data/pendingInputs.ts (1)
91-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the steer user-message creation into one helper.
The same
createUserMessagecall withstatus: 'pending'and theinputReceiptmetadata appears three times in this file: lines 91-104, 134-147, and 342-355. One helper keeps the receipt contract in a single place, so a future change to the receipt shape cannot diverge between accept, promote, and recovery.♻️ Proposed helper
+ private createPendingSteerMessage(sessionId: string, input: SendMessageInput): string { + return this.transcript.createUserMessage( + sessionId, + this.transcript.getNextOrderSeq(sessionId), + toUserMessageContent(input), + { + status: 'pending', + metadata: { + inputReceipt: { + mode: 'steer', + readAt: null + } + } + } + ) + }Then call
this.createPendingSteerMessage(sessionId, input)inacceptSteerMessage,promoteQueuedInputToSteerMessage, andrecoverClaimedInputsAfterRestart.🤖 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 `@src/main/session/data/pendingInputs.ts` around lines 91 - 104, Extract the duplicated pending steer createUserMessage logic into a createPendingSteerMessage helper on the containing class, preserving the existing sessionId, input conversion, pending status, and steer inputReceipt metadata. Replace the inline implementations in acceptSteerMessage, promoteQueuedInputToSteerMessage, and recoverClaimedInputsAfterRestart with calls to this helper.
🤖 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 `@src/main/agent/acp/instance/acpAgentInstance.ts`:
- Around line 347-350: The cancel method currently overwrites an existing
cancellation cause during repeated cancellation. Update AcpAgentInstance.cancel
so active.cancelCause is assigned only when no cause has been recorded,
preserving the initial pending_input cause; add a regression test that calls
cancel('pending_input') followed by cancel() before the aborted prompt settles
and verifies the original cause is retained.
In `@src/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.ts`:
- Around line 333-337: Update the active-generation branch in the pending input
admission coordinator to retain the promoted item’s Steer batch: merge it into
an existing pending Steer batch when available, otherwise call
setActiveSteerPendingInputId() with the promoted record. Ensure
acceptVisibleSteerInput() can use the resulting batch as mergeItemId for the
next direct Steer, and add a regression test covering Queue promotion followed
by direct Steer.
In `@src/main/agent/manager/directAcpAgentBackend.ts`:
- Around line 138-143: Update the async list() method to re-read pending inputs
from runtime.listPendingInputs(sessionId) after resumePendingInputs completes,
returning that post-resume snapshot instead of the initial inputs. Add a
recovery regression test covering pending.list() after a Steer is claimed,
verifying it returns the updated receipt state.
In `@src/main/session/data/pendingInputs.ts`:
- Around line 323-327: Update the blocked-input recovery branch in the
pending-inputs flow to settle or retract all messages linked to the blocked
steer, including its reserved assistant message, before calling
convertSteerInputToQueue. Reuse the existing message-settlement/retraction and
assistant-reservation cleanup mechanisms, then requeue the payload while
preserving sessionIds tracking and loop control.
In `@src/renderer/src/components/message/MessageItemUser.vue`:
- Around line 183-189: Update the effectiveReadOnly computed property in
MessageItemUser so accepted Steer messages with inputReceipt metadata remain
read-only, in addition to the existing isReadOnly and pending-status conditions.
Preserve edit, retry, and delete availability for other messages.
In `@test/renderer/features/chat-page/composables/useComposerSubmit.test.ts`:
- Line 740: Remove the duplicate const steering declarations in each affected
test scope, retaining exactly one createDeferred declaration at
test/renderer/features/chat-page/composables/useComposerSubmit.test.ts lines
740-740 and 789-789.
---
Outside diff comments:
In `@src/main/session/chatService.ts`:
- Around line 185-191: Replace the non-null assertion in the accepted result
branch of chatService with an explicit result.userMessage check. When the
message is absent, fail clearly or return the rejected variant so accepted
responses always contain a valid ChatMessageRecord; preserve the existing
attachmentPreparation handling for valid messages.
In `@test/main/session/session.integration.test.ts`:
- Around line 2388-2405: The test’s mocked steerPendingInput responses must
reflect the production call flow. Review convertPendingInputToSteer and
steerPendingInput, then update the mock setup and assertions so each user action
expects the actual number of calls and uses the same resolved value for both
calls when production invokes them equivalently.
---
Nitpick comments:
In `@src/main/session/data/pendingInputs.ts`:
- Around line 91-104: Extract the duplicated pending steer createUserMessage
logic into a createPendingSteerMessage helper on the containing class,
preserving the existing sessionId, input conversion, pending status, and steer
inputReceipt metadata. Replace the inline implementations in acceptSteerMessage,
promoteQueuedInputToSteerMessage, and recoverClaimedInputsAfterRestart with
calls to this helper.
In `@src/main/session/data/pendingInputStore.ts`:
- Around line 536-545: Update decodeMessageIds in the JSON.parse error path to
log that row.message_ids_json is invalid before returning the existing empty
array fallback, matching the diagnostics used by decodePayload and
decodeBlocking. Keep valid-array filtering and non-array behavior unchanged.
In `@src/main/session/data/transcript.ts`:
- Around line 285-325: Update markSteerMessagesRead and settleSteerMessages to
collect each successfully retrieved updated record in an array during the
existing loop, then return that array directly instead of mapping messageIds
through requireMessage for a second read.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86ea8d60-ae26-43c5-8dd7-1909ff771c4f
📒 Files selected for processing (88)
docs/architecture/session-management.mddocs/features/im-style-steer-messages/plan.mddocs/features/im-style-steer-messages/spec.mddocs/features/im-style-steer-messages/tasks.mdsrc/main/agent/acp/compatibility/adapters.tssrc/main/agent/acp/instance/acpAgentInstance.tssrc/main/agent/acp/instance/acpAgentRuntime.tssrc/main/agent/acp/instance/ports.tssrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/harness/deepChatAgentHarness.tssrc/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.tssrc/main/agent/deepchat/runtime/pendingInputContracts.tssrc/main/agent/deepchat/runtime/pendingInputPump.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/manager/deepChatAgentBackend.tssrc/main/agent/manager/directAcpAgentBackend.tssrc/main/agent/manager/sessionHandles.tssrc/main/app/composition.tssrc/main/data/schemaCatalog.tssrc/main/session/chatService.tssrc/main/session/data/contracts.tssrc/main/session/data/index.tssrc/main/session/data/pendingInputStore.tssrc/main/session/data/pendingInputs.tssrc/main/session/data/tables/deepchatPendingInputs.tssrc/main/session/data/transcript.tssrc/main/session/turn.tssrc/renderer/api/SessionClient.tssrc/renderer/src/components/chat/ChatInputToolbar.vuesrc/renderer/src/components/chat/PendingInputLane.vuesrc/renderer/src/components/message/MessageInfo.vuesrc/renderer/src/components/message/MessageItemUser.vuesrc/renderer/src/features/chat-page/ChatPage.vuesrc/renderer/src/features/chat-page/composables/useComposerSubmit.tssrc/renderer/src/features/chat-page/composables/useDisplayMessages.tssrc/renderer/src/features/chat-page/model/displayMessage.tssrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/de-DE/chat.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/es-ES/chat.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/id-ID/chat.jsonsrc/renderer/src/i18n/it-IT/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ms-MY/chat.jsonsrc/renderer/src/i18n/pl-PL/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/tr-TR/chat.jsonsrc/renderer/src/i18n/vi-VN/chat.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/messageIpc.tssrc/renderer/src/stores/ui/pendingInput.tssrc/shared/contracts/events.tssrc/shared/contracts/events/sessions.events.tssrc/shared/contracts/routes/chat.routes.tssrc/shared/contracts/routes/sessions.routes.tssrc/shared/types/agent-interface.d.tstest/main/agent/acp/instance/acpAgentRuntime.test.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.test.tstest/main/agent/deepchat/runtime/pendingInputPump.test.tstest/main/agent/manager/agentManager.test.tstest/main/agent/manager/deepChatAgentBackend.test.tstest/main/agent/manager/directAcpAgentBackend.test.tstest/main/routes/dispatcher.test.tstest/main/session/assignment.test.tstest/main/session/chatService.test.tstest/main/session/data/pendingInputStore.test.tstest/main/session/data/pendingInputs.test.tstest/main/session/data/tables/deepchatPendingInputsTable.test.tstest/main/session/runtimeIntegration.test.tstest/main/session/session.integration.test.tstest/main/session/turn.test.tstest/renderer/components/ChatInputToolbar.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/PendingInputLane.test.tstest/renderer/components/message/MessageItemUser.test.tstest/renderer/features/chat-page/composables/useComposerSubmit.test.tstest/renderer/stores/messageStore.reactivity.test.tstest/renderer/stores/messageStore.test.tstest/renderer/stores/pendingInputStore.test.ts
💤 Files with no reviewable changes (7)
- src/main/agent/manager/sessionHandles.ts
- src/renderer/src/stores/ui/pendingInput.ts
- test/main/agent/manager/agentManager.test.ts
- src/main/agent/manager/deepChatAgentBackend.ts
- src/main/agent/deepchat/harness/deepChatAgentHarness.ts
- test/main/session/assignment.test.ts
- test/main/agent/manager/deepChatAgentBackend.test.ts
| if (input.state === 'blocked') { | ||
| this.store.convertSteerInputToQueue(input.id) | ||
| sessionIds.add(input.sessionId) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Blocked steer recovery orphans the linked transcript messages.
A steer input can only reach state blocked through blockClaimedInput, which requires state claimed. claimSteerInput (line 217) rejects a claim when messageIds is empty, and it also reserves an assistant message. So every blocked steer row carries at least one linked user message and an assistantMessageId.
Recovery converts that row back to a queue draft. The linked user messages stay pending with inputReceipt.readAt set, and SessionTranscript.shouldKeepPending keeps them pending forever. The reserved assistant message also stays pending. The user then sees a permanently unsettled steer message in the transcript plus the same text again as an editable Queue draft.
Settle or retract the linked messages before you requeue the payload, and clear the assistant reservation.
🐛 Proposed direction
if (input.state === 'blocked') {
+ if (input.messageIds.length > 0 || input.assistantMessageId) {
+ // Settle the sent conversation facts, then requeue only the editable draft.
+ this.transcript.settleSteerMessages(input.messageIds)
+ }
this.store.convertSteerInputToQueue(input.id)
sessionIds.add(input.sessionId)
continue
}🤖 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 `@src/main/session/data/pendingInputs.ts` around lines 323 - 327, Update the
blocked-input recovery branch in the pending-inputs flow to settle or retract
all messages linked to the blocked steer, including its reserved assistant
message, before calling convertSteerInputToQueue. Reuse the existing
message-settlement/retraction and assistant-reservation cleanup mechanisms, then
requeue the payload while preserving sessionIds tracking and loop control.
| const receipt = ref<'unread' | 'read' | null>(null) | ||
| let receiptTimer: ReturnType<typeof setTimeout> | null = null | ||
|
|
||
| const effectiveReadOnly = computed(() => props.isReadOnly || props.message.status === 'pending') | ||
| const receiptLabel = computed(() => | ||
| receipt.value ? t(`chat.messageReceipt.${receipt.value}`) : undefined | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep accepted Steer messages immutable.
Line 186 only marks pending messages as read-only. An accepted Steer is a persisted sent user message with inputReceipt, so edit, retry, and delete become available after acceptance. Include Steer receipt metadata in effectiveReadOnly.
Proposed fix
-const effectiveReadOnly = computed(() => props.isReadOnly || props.message.status === 'pending')
+const effectiveReadOnly = computed(
+ () =>
+ props.isReadOnly ||
+ props.message.status === 'pending' ||
+ props.message.inputReceipt?.mode === 'steer'
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const receipt = ref<'unread' | 'read' | null>(null) | |
| let receiptTimer: ReturnType<typeof setTimeout> | null = null | |
| const effectiveReadOnly = computed(() => props.isReadOnly || props.message.status === 'pending') | |
| const receiptLabel = computed(() => | |
| receipt.value ? t(`chat.messageReceipt.${receipt.value}`) : undefined | |
| ) | |
| const receipt = ref<'unread' | 'read' | null>(null) | |
| let receiptTimer: ReturnType<typeof setTimeout> | null = null | |
| const effectiveReadOnly = computed( | |
| () => | |
| props.isReadOnly || | |
| props.message.status === 'pending' || | |
| props.message.inputReceipt?.mode === 'steer' | |
| ) | |
| const receiptLabel = computed(() => | |
| receipt.value ? t(`chat.messageReceipt.${receipt.value}`) : undefined | |
| ) |
🤖 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 `@src/renderer/src/components/message/MessageItemUser.vue` around lines 183 -
189, Update the effectiveReadOnly computed property in MessageItemUser so
accepted Steer messages with inputReceipt metadata remain read-only, in addition
to the existing isReadOnly and pending-status conditions. Preserve edit, retry,
and delete availability for other messages.
|
|
||
| it('blocks duplicates and clears the draft only after acceptance', async () => { | ||
| const steering = createDeferred<{ accepted: boolean }>() | ||
| const steering = createDeferred<{ accepted: true; message: ChatMessageRecord }>() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate steering declarations.
Each test scope declares const steering twice. TypeScript rejects duplicate block-scoped declarations, so this test file does not compile.
test/renderer/features/chat-page/composables/useComposerSubmit.test.ts#L740-L740: Keep onecreateDeferreddeclaration.test/renderer/features/chat-page/composables/useComposerSubmit.test.ts#L789-L789: Keep onecreateDeferreddeclaration.
📍 Affects 1 file
test/renderer/features/chat-page/composables/useComposerSubmit.test.ts#L740-L740(this comment)test/renderer/features/chat-page/composables/useComposerSubmit.test.ts#L789-L789
🤖 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 `@test/renderer/features/chat-page/composables/useComposerSubmit.test.ts` at
line 740, Remove the duplicate const steering declarations in each affected test
scope, retaining exactly one createDeferred declaration at
test/renderer/features/chat-page/composables/useComposerSubmit.test.ts lines
740-740 and 789-789.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/main/agent/acp/instance/acpAgentInstance.test.ts`:
- Around line 313-326: Update the test “keeps first-turn readiness and the first
cancellation cause stable” to assert connection.cancel was called once after the
cancellation operations complete, while retaining the earlier connection.prompt
assertion for readiness and the existing cancelCauses expectation.
In `@test/main/agent/manager/directAcpAgentBackend.test.ts`:
- Around line 341-348: Strengthen the test “exposes direct transfer, pending,
subagent, generation, and close facets” by recording lifecycle events in the
resume, runtime-close, and cleanup-session mocks. After awaiting handle.close(),
assert that resume completion precedes both runtime.close and cleanupSession,
while preserving the existing pending-input assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dcf16993-4bec-4c81-b1e5-b6d6cbfcd465
📒 Files selected for processing (5)
src/main/agent/acp/instance/acpAgentInstance.tssrc/main/agent/manager/directAcpAgentBackend.tssrc/main/session/chatService.tstest/main/agent/acp/instance/acpAgentInstance.test.tstest/main/agent/manager/directAcpAgentBackend.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/session/chatService.ts
- src/main/agent/manager/directAcpAgentBackend.ts
- src/main/agent/acp/instance/acpAgentInstance.ts
| it('keeps first-turn readiness and the first cancellation cause stable', async () => { | ||
| const harness = createHarness({ promptNeverSettles: true }) | ||
| const sending = harness.instance.send('hello') | ||
|
|
||
| await expect(harness.instance.waitForFirstTurnReady({ timeoutMs: 100 })).resolves.toBe(true) | ||
| expect(harness.connection.prompt).toHaveBeenCalledTimes(1) | ||
|
|
||
| await harness.instance.cancel() | ||
| const pendingInputCancellation = harness.instance.cancel('pending_input') | ||
| const repeatedCancellation = harness.instance.cancel() | ||
| await Promise.all([pendingInputCancellation, repeatedCancellation]) | ||
| await sending | ||
|
|
||
| expect(harness.connection.prompt).toHaveBeenCalledTimes(1) | ||
| expect(harness.cancelCauses).toEqual(['pending_input']) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the cancellation operation.
Line 325 checks connection.prompt, which only proves that the prompt started. It does not prove that cancellation happened once. Assert connection.cancel instead. Keep the earlier prompt assertion for readiness.
Suggested assertion
- expect(harness.connection.prompt).toHaveBeenCalledTimes(1)
+ expect(harness.connection.cancel).toHaveBeenCalledTimes(1)As per coding guidelines: Add the smallest regression test for user-visible behavior or a documented contract.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('keeps first-turn readiness and the first cancellation cause stable', async () => { | |
| const harness = createHarness({ promptNeverSettles: true }) | |
| const sending = harness.instance.send('hello') | |
| await expect(harness.instance.waitForFirstTurnReady({ timeoutMs: 100 })).resolves.toBe(true) | |
| expect(harness.connection.prompt).toHaveBeenCalledTimes(1) | |
| await harness.instance.cancel() | |
| const pendingInputCancellation = harness.instance.cancel('pending_input') | |
| const repeatedCancellation = harness.instance.cancel() | |
| await Promise.all([pendingInputCancellation, repeatedCancellation]) | |
| await sending | |
| expect(harness.connection.prompt).toHaveBeenCalledTimes(1) | |
| expect(harness.cancelCauses).toEqual(['pending_input']) | |
| it('keeps first-turn readiness and the first cancellation cause stable', async () => { | |
| const harness = createHarness({ promptNeverSettles: true }) | |
| const sending = harness.instance.send('hello') | |
| await expect(harness.instance.waitForFirstTurnReady({ timeoutMs: 100 })).resolves.toBe(true) | |
| expect(harness.connection.prompt).toHaveBeenCalledTimes(1) | |
| const pendingInputCancellation = harness.instance.cancel('pending_input') | |
| const repeatedCancellation = harness.instance.cancel() | |
| await Promise.all([pendingInputCancellation, repeatedCancellation]) | |
| await sending | |
| expect(harness.connection.cancel).toHaveBeenCalledTimes(1) | |
| expect(harness.cancelCauses).toEqual(['pending_input']) |
🤖 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 `@test/main/agent/acp/instance/acpAgentInstance.test.ts` around lines 313 -
326, Update the test “keeps first-turn readiness and the first cancellation
cause stable” to assert connection.cancel was called once after the cancellation
operations complete, while retaining the earlier connection.prompt assertion for
readiness and the existing cancelCauses expectation.
Source: Coding guidelines
| it('exposes direct transfer, pending, subagent, generation, and close facets', async () => { | ||
| const harness = createHarness() | ||
| const handle = harness.backend.open(sessionId, descriptor) | ||
| const pendingBeforeResume = { id: 'steer', mode: 'steer', state: 'pending' } | ||
| const pendingAfterResume = { ...pendingBeforeResume, state: 'claimed' } | ||
| harness.runtime.listPendingInputs | ||
| .mockReturnValueOnce([pendingBeforeResume]) | ||
| .mockReturnValueOnce([pendingAfterResume]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Verify resume completion before cleanup.
The assertion at Line 379 proves only that resumePendingInputs was called. The preconfigured results at Lines 344-348 and the assertion at Line 362 do not prove that resume completed before handle.close() finished or before runtime.close and cleanupSession ran.
Record lifecycle events in the mocks. Assert that resume completion occurs before cleanup. The runtime method hydrates the session and drains pending inputs, so cleanup-first ordering can lose recovery work.
As per coding guidelines: Add the smallest regression test for user-visible behavior or a documented contract.
Also applies to: 362-362, 379-381
🤖 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 `@test/main/agent/manager/directAcpAgentBackend.test.ts` around lines 341 -
348, Strengthen the test “exposes direct transfer, pending, subagent,
generation, and close facets” by recording lifecycle events in the resume,
runtime-close, and cleanup-session mocks. After awaiting handle.close(), assert
that resume completion precedes both runtime.close and cleanupSession, while
preserving the existing pending-input assertions.
Source: Coding guidelines
Summary
Unreaduntil runtime claim, thenRead, then no receiptpending_inputboundary and start the response in a new assistant rowWhy
Steer previously behaved like a bottom command: before the first assistant stream update it was disabled, and after acceptance it used loading/pending UI instead of appearing in the conversation. That made admission timing and response ownership unclear.
The interaction now follows an IM model. A Steer appears in the message list while the other side is preparing or typing, becomes
Readwhen the loop claims it, and receives a distinct assistant response below it.UI: before / after
Before: pre-stream Steer was blocked
After: Steer is sent immediately
After the safe read boundary
Readstays visible untilreadAt + 1.5s, fades for 150 ms, and disappears. Reduced-motion mode skips the fade. The receipt stays in the existing fixed-height message-info line, so row height and virtual-scroll geometry do not change.Behavior and architecture
orderSeq.pending_input, removes an empty assistant reservation if one exists, settles the source claim, and lets the pending-input pump claim Steer.pending_input.currentStreamMessageIdavailability.docs/features/im-style-steer-messages/contains the complete UI states, ordering invariants, failure behavior, implementation map, and release QA.Test scope
The regression suite stays lean: existing cases were repurposed for the new contract, and the obsolete disabled-tooltip case was removed rather than adding a parallel test matrix.
Validation
pnpm run formatpnpm run i18n— 20 locales, no missing or invalid keyspnpm run lintpnpm run typecheck— main and rendererManual release QA
Summary by CodeRabbit
New Features
Documentation