-
Notifications
You must be signed in to change notification settings - Fork 907
fix(agent-core): realign mid-history interrupted tool calls on resume #1027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5fb8cf6
ce92945
9c52224
f8518dc
5055262
8ed41c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@moonshot-ai/kimi-code": patch | ||
| --- | ||
|
|
||
| Fix resume not realigning a tool call that was interrupted mid-history. The synthetic interrupted result is now closed in place at the next step boundary, so later turns and deferred messages keep their recorded order instead of only the trailing exchange being repaired. The `/messages` wire transcript reducer mirrors the same closure so its folded length stays aligned with live history, preventing the later turn from being duplicated/reordered. Replay also drops a tool result whose call is no longer awaiting one, so a stale interrupted result left at the log tail by an older resume of a damaged session is not re-applied as a duplicate. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,10 +227,20 @@ export class ContextMemory { | |
| } | ||
|
|
||
| finishResume(): void { | ||
| const interruptedToolCallIds = [...this.pendingToolResultIds]; | ||
| this.openSteps.clear(); | ||
| if (interruptedToolCallIds.length === 0) return; | ||
| this.closePendingToolResults(); | ||
| } | ||
|
|
||
| // Synthesize interrupted tool results for any still-open tool calls, closing | ||
| // the exchange in place. Called at every replayed step boundary (see the | ||
| // `step.begin` case) so a tool call left unresolved mid-history is closed | ||
| // exactly where it occurred — otherwise it would keep `hasOpenToolExchange` | ||
| // true and strand every later message in `deferredMessages`, so only the | ||
| // trailing exchange ends up aligned. `finishResume` runs the same routine once | ||
| // more to close a genuine trailing interruption at end of resume. | ||
| private closePendingToolResults(): void { | ||
| if (this.pendingToolResultIds.size === 0) return; | ||
| const interruptedToolCallIds = [...this.pendingToolResultIds]; | ||
| for (const toolCallId of interruptedToolCallIds) { | ||
| this.appendLoopEvent({ | ||
| type: 'tool.result', | ||
|
|
@@ -251,6 +261,11 @@ export class ContextMemory { | |
| }); | ||
| switch (event.type) { | ||
| case 'step.begin': { | ||
| // A new assistant step means any tool calls still pending from an | ||
| // earlier step were interrupted (the invariant guarantees this never | ||
| // happens live, so this is a no-op outside replay). Close them in place | ||
| // before opening the new step so mid-history gaps stay aligned. | ||
| this.closePendingToolResults(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a damaged session has already been resumed by the previous tail-only logic, Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When this runs during Useful? React with 👍 / 👎. |
||
| const message: ContextMessage = { | ||
| role: 'assistant', | ||
| content: [], | ||
|
|
@@ -316,6 +331,10 @@ export class ContextMemory { | |
| return; | ||
| } | ||
| case 'tool.result': { | ||
| // Drop a result for an id that is not awaiting one: it was already | ||
| // closed in place at a step boundary (a stale duplicate from an older | ||
| // tail-only finishResume), or its call is gone. | ||
| if (!this.pendingToolResultIds.has(event.toolCallId)) return; | ||
| const message = createToolMessage(event.toolCallId, toolResultOutputForModel(event.result)); | ||
| this.pushHistory({ | ||
| ...message, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds the new replay-time behavior to close a pending tool call before the next
step.begin, but the daemon message reducer inpackages/agent-core/src/services/message/transcript.tsstill treatsstep.beginas a plain assistant append and never clearspendingToolResultIds. BecauseMessageService._getTranscriptEntriesresumes first and then mergescontext.history.slice(transcript.foldedLength), a session matching the new mid-history interruption case will make/messagesinclude the later assistant from the wire, then append the live tail from the wrong folded index, duplicating/reordering the later turn and omitting the synthetic tool error. Please mirror thisclosePendingToolResults()behavior inreduceWireRecordswhen reducing a newstep.begin.Useful? React with 👍 / 👎.