mirror of
https://github.com/MoonshotAI/kimi-code.git
synced 2026-07-10 01:39:25 +00:00
fix: close interrupted tool calls on resume (#768)
This commit is contained in:
parent
4516f62f6a
commit
c6a996756c
7 changed files with 280 additions and 10 deletions
6
.changeset/close-interrupted-tool-results.md
Normal file
6
.changeset/close-interrupted-tool-results.md
Normal file
|
|
@ -0,0 +1,6 @@
|
|||
---
|
||||
"@moonshot-ai/agent-core": patch
|
||||
"@moonshot-ai/kimi-code": patch
|
||||
---
|
||||
|
||||
Recover resumed sessions when an interrupted tool call result was not recorded.
|
||||
|
|
@ -158,7 +158,9 @@ export class DefaultCompactionStrategy implements CompactionStrategy {
|
|||
* results for one exchange land consecutively before the next non-tool
|
||||
* message. So if the suffix starts with a tool result, its `asst_w_tc`
|
||||
* must be in the compacted prefix, which would orphan that result
|
||||
* (e.g. splitting between tool_a and tool_b of a parallel call).
|
||||
* (e.g. splitting between tool_a and tool_b of a parallel call), AND
|
||||
* - the compacted prefix itself does not end with an unresolved tool
|
||||
* exchange, because pending tool results must remain in the retained tail.
|
||||
*/
|
||||
function canSplitAfter(messages: readonly Message[], index: number): boolean {
|
||||
const m = messages[index];
|
||||
|
|
@ -166,5 +168,22 @@ function canSplitAfter(messages: readonly Message[], index: number): boolean {
|
|||
if (m.role === 'user') return false;
|
||||
if (m.role === 'assistant' && m.toolCalls.length > 0) return false;
|
||||
if (messages[index + 1]?.role === 'tool') return false;
|
||||
if (prefixEndsWithOpenToolExchange(messages, index)) return false;
|
||||
return true;
|
||||
}
|
||||
|
||||
function prefixEndsWithOpenToolExchange(messages: readonly Message[], index: number): boolean {
|
||||
if (messages[index]?.role !== 'tool') return false;
|
||||
|
||||
let toolResultCount = 0;
|
||||
for (let i = index; i >= 0; i--) {
|
||||
const message = messages[i];
|
||||
if (message === undefined) return false;
|
||||
if (message.role === 'tool') {
|
||||
toolResultCount++;
|
||||
continue;
|
||||
}
|
||||
return message.role === 'assistant' && message.toolCalls.length > toolResultCount;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,7 +20,13 @@ const TOOL_EMPTY_STATUS = '<system>Tool output is empty.</system>';
|
|||
const TOOL_EMPTY_ERROR_STATUS =
|
||||
'<system>ERROR: Tool execution failed. Tool output is empty.</system>';
|
||||
const TOOL_OUTPUT_EMPTY_TEXT = 'Tool output is empty.';
|
||||
const TOOL_INTERRUPTED_ON_RESUME_OUTPUT =
|
||||
'Tool execution was interrupted before its result was recorded. Do not assume the tool completed successfully.';
|
||||
|
||||
// Invariant: _history must not contain an unresolved tool call exchange except
|
||||
// at the tail. When the tail is unresolved, pendingToolResultIds is exactly the
|
||||
// set of missing tool result ids for that tail exchange; appendMessage keeps
|
||||
// later messages in deferredMessages until those ids are resolved.
|
||||
export class ContextMemory {
|
||||
private _history: ContextMessage[] = [];
|
||||
private _tokenCount = 0;
|
||||
|
|
@ -210,6 +216,24 @@ export class ContextMemory {
|
|||
this.pushHistory(...trimTrailingOpenToolExchange(source.project(source.history)));
|
||||
}
|
||||
|
||||
finishResume(): void {
|
||||
const interruptedToolCallIds = [...this.pendingToolResultIds];
|
||||
this.openSteps.clear();
|
||||
if (interruptedToolCallIds.length === 0) return;
|
||||
|
||||
for (const toolCallId of interruptedToolCallIds) {
|
||||
this.appendLoopEvent({
|
||||
type: 'tool.result',
|
||||
parentUuid: toolCallId,
|
||||
toolCallId,
|
||||
result: {
|
||||
output: TOOL_INTERRUPTED_ON_RESUME_OUTPUT,
|
||||
isError: true,
|
||||
},
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
appendLoopEvent(event: LoopRecordedEvent): void {
|
||||
this.agent.records.logRecord({
|
||||
type: 'context.append_loop_event',
|
||||
|
|
|
|||
|
|
@ -296,11 +296,17 @@ export class Agent {
|
|||
|
||||
async resume(): Promise<{ warning?: string }> {
|
||||
const result = await this.records.replay();
|
||||
this.goal.normalizeAfterReplay();
|
||||
await this.background.loadFromDisk();
|
||||
await this.background.reconcile();
|
||||
await this.cron?.loadFromDisk();
|
||||
this.turn.finishResume();
|
||||
try {
|
||||
this.replayBuilder.postRestoring = true;
|
||||
this.goal.normalizeAfterReplay();
|
||||
await this.background.loadFromDisk();
|
||||
await this.background.reconcile();
|
||||
await this.cron?.loadFromDisk();
|
||||
this.context.finishResume();
|
||||
this.turn.finishResume();
|
||||
} finally {
|
||||
this.replayBuilder.postRestoring = false;
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -3,13 +3,14 @@ import type { AgentReplayRecord, AgentReplayRecordPayload } from '../..';
|
|||
import type { ContextMessage } from '../context';
|
||||
|
||||
export class ReplayBuilder {
|
||||
postRestoring = false;
|
||||
captureLiveRecords = false;
|
||||
protected readonly records: AgentReplayRecord[] = [];
|
||||
|
||||
constructor(public readonly agent: Agent) {}
|
||||
|
||||
push(record: AgentReplayRecordPayload): void {
|
||||
if (this.captureLiveRecords || this.agent.records.restoring) {
|
||||
if (this.captureLiveRecords || this.agent.records.restoring || this.postRestoring) {
|
||||
this.records.push({
|
||||
...record,
|
||||
time: this.agent.records.restoring?.time ?? Date.now(),
|
||||
|
|
|
|||
|
|
@ -879,13 +879,29 @@ describe('FullCompaction', () => {
|
|||
messages:
|
||||
user: text "old user one"
|
||||
assistant: text "old assistant one"
|
||||
user: text "run both tools"
|
||||
assistant: [] calls call_open_one:LookupOne { "query": "one" }, call_open_two:LookupTwo { "query": "two" }
|
||||
tool[call_open_one]: text "one result"
|
||||
user: text <compaction-instruction>
|
||||
`);
|
||||
expect(ctx.agent.context.history.map((message) => message.role)).toEqual([
|
||||
'assistant',
|
||||
'user',
|
||||
'assistant',
|
||||
'tool',
|
||||
]);
|
||||
ctx.dispatch({
|
||||
type: 'context.append_loop_event',
|
||||
event: {
|
||||
type: 'tool.result',
|
||||
parentUuid: 'call_open_two',
|
||||
toolCallId: 'call_open_two',
|
||||
result: { output: 'two result' },
|
||||
},
|
||||
});
|
||||
expect(ctx.agent.context.history.map((message) => message.role)).toEqual([
|
||||
'assistant',
|
||||
'user',
|
||||
'assistant',
|
||||
'tool',
|
||||
'tool',
|
||||
]);
|
||||
await ctx.expectResumeMatches();
|
||||
});
|
||||
|
|
@ -1152,6 +1168,9 @@ describe('FullCompaction', () => {
|
|||
|
||||
expect(ctx.agent.context.history.map((m) => m.role)).toEqual([
|
||||
'assistant',
|
||||
'user',
|
||||
'assistant',
|
||||
'tool',
|
||||
]);
|
||||
|
||||
ctx.dispatch({
|
||||
|
|
@ -1166,6 +1185,9 @@ describe('FullCompaction', () => {
|
|||
|
||||
expect(ctx.agent.context.history.map((m) => m.role)).toEqual([
|
||||
'assistant',
|
||||
'user',
|
||||
'assistant',
|
||||
'tool',
|
||||
'tool',
|
||||
'user',
|
||||
]);
|
||||
|
|
|
|||
|
|
@ -408,6 +408,186 @@ describe('Agent resume', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('closes interrupted trailing tool calls with synthetic error results after resume', async () => {
|
||||
const persistence = new RecordingAgentPersistence([
|
||||
{
|
||||
type: 'config.update',
|
||||
cwd: process.cwd(),
|
||||
modelAlias: MOCK_PROVIDER.model,
|
||||
systemPrompt: DEFAULT_TEST_SYSTEM_PROMPT,
|
||||
thinkingLevel: 'off',
|
||||
},
|
||||
{
|
||||
type: 'context.append_message',
|
||||
message: {
|
||||
role: 'user',
|
||||
content: [{ type: 'text', text: 'Run both lookups' }],
|
||||
toolCalls: [],
|
||||
origin: { kind: 'user' },
|
||||
},
|
||||
},
|
||||
{
|
||||
type: 'context.append_loop_event',
|
||||
event: {
|
||||
type: 'step.begin',
|
||||
uuid: 'interrupted-step',
|
||||
turnId: '0',
|
||||
step: 1,
|
||||
},
|
||||
},
|
||||
{
|
||||
type: 'context.append_loop_event',
|
||||
event: {
|
||||
type: 'tool.call',
|
||||
uuid: 'call-one',
|
||||
turnId: '0',
|
||||
step: 1,
|
||||
stepUuid: 'interrupted-step',
|
||||
toolCallId: 'call_interrupted_one',
|
||||
name: 'LookupOne',
|
||||
args: { query: 'one' },
|
||||
},
|
||||
},
|
||||
{
|
||||
type: 'context.append_loop_event',
|
||||
event: {
|
||||
type: 'tool.call',
|
||||
uuid: 'call-two',
|
||||
turnId: '0',
|
||||
step: 1,
|
||||
stepUuid: 'interrupted-step',
|
||||
toolCallId: 'call_interrupted_two',
|
||||
name: 'LookupTwo',
|
||||
args: { query: 'two' },
|
||||
},
|
||||
},
|
||||
{
|
||||
type: 'context.append_loop_event',
|
||||
event: {
|
||||
type: 'tool.result',
|
||||
parentUuid: 'call-one',
|
||||
toolCallId: 'call_interrupted_one',
|
||||
result: { output: 'one result' },
|
||||
},
|
||||
},
|
||||
]);
|
||||
const ctx = testAgent({ persistence });
|
||||
|
||||
await ctx.agent.resume();
|
||||
|
||||
expect(ctx.agent.context.history.map((message) => message.role)).toEqual([
|
||||
'user',
|
||||
'assistant',
|
||||
'tool',
|
||||
'tool',
|
||||
]);
|
||||
const syntheticResult = ctx.agent.context.history.at(-1);
|
||||
expect(syntheticResult).toMatchObject({
|
||||
role: 'tool',
|
||||
toolCallId: 'call_interrupted_two',
|
||||
isError: true,
|
||||
});
|
||||
expect(textContent(syntheticResult)).toContain(
|
||||
'Tool execution was interrupted before its result was recorded',
|
||||
);
|
||||
const replayMessages = ctx.agent.replayBuilder
|
||||
.buildResult()
|
||||
.flatMap((record) => (record.type === 'message' ? [record.message] : []));
|
||||
expect(replayMessages.map((message) => message.role)).toEqual([
|
||||
'user',
|
||||
'assistant',
|
||||
'tool',
|
||||
'tool',
|
||||
]);
|
||||
expect(replayMessages.at(-1)).toMatchObject({
|
||||
role: 'tool',
|
||||
toolCallId: 'call_interrupted_two',
|
||||
isError: true,
|
||||
});
|
||||
expect(textContent(replayMessages.at(-1))).toContain(
|
||||
'Tool execution was interrupted before its result was recorded',
|
||||
);
|
||||
expect(
|
||||
persistence.appended.filter(
|
||||
(record) =>
|
||||
record.type === 'context.append_loop_event' &&
|
||||
record.event.type === 'tool.result' &&
|
||||
record.event.toolCallId === 'call_interrupted_two',
|
||||
),
|
||||
).toEqual([
|
||||
expect.objectContaining({
|
||||
type: 'context.append_loop_event',
|
||||
event: expect.objectContaining({
|
||||
type: 'tool.result',
|
||||
parentUuid: 'call_interrupted_two',
|
||||
toolCallId: 'call_interrupted_two',
|
||||
result: {
|
||||
output:
|
||||
'Tool execution was interrupted before its result was recorded. Do not assume the tool completed successfully.',
|
||||
isError: true,
|
||||
},
|
||||
}),
|
||||
}),
|
||||
]);
|
||||
|
||||
ctx.mockNextResponse({ type: 'text', text: 'Recovered after resume.' });
|
||||
await ctx.rpc.prompt({ input: [{ type: 'text', text: 'continue after resume' }] });
|
||||
await ctx.untilTurnEnd();
|
||||
|
||||
const syntheticRecordIndex = persistence.records.findIndex(
|
||||
(record) =>
|
||||
record.type === 'context.append_loop_event' &&
|
||||
record.event.type === 'tool.result' &&
|
||||
record.event.toolCallId === 'call_interrupted_two',
|
||||
);
|
||||
const freshUserRecordIndex = persistence.records.findIndex(
|
||||
(record) =>
|
||||
record.type === 'context.append_message' &&
|
||||
record.message.role === 'user' &&
|
||||
textContent(record.message) === 'continue after resume',
|
||||
);
|
||||
expect(syntheticRecordIndex).toBeGreaterThan(-1);
|
||||
expect(freshUserRecordIndex).toBeGreaterThan(-1);
|
||||
expect(syntheticRecordIndex).toBeLessThan(freshUserRecordIndex);
|
||||
|
||||
const llmHistory = ctx.llmCalls[0]?.history ?? [];
|
||||
expect(llmHistory.map((message) => message.role)).toEqual([
|
||||
'user',
|
||||
'assistant',
|
||||
'tool',
|
||||
'tool',
|
||||
'user',
|
||||
]);
|
||||
expect(textContent(llmHistory[3])).toContain(
|
||||
'<system>ERROR: Tool execution failed.</system>',
|
||||
);
|
||||
expect(textContent(llmHistory[3])).toContain(
|
||||
'Tool execution was interrupted before its result was recorded',
|
||||
);
|
||||
expect(textContent(llmHistory[4])).toBe('continue after resume');
|
||||
expect(
|
||||
ctx.agent.context.history.some(
|
||||
(message) => message.role === 'user' && textContent(message) === 'continue after resume',
|
||||
),
|
||||
).toBe(true);
|
||||
|
||||
const resumedAgain = testAgent({ persistence });
|
||||
await resumedAgain.agent.resume();
|
||||
|
||||
expect(resumedAgain.agent.context.history.map((message) => message.role)).toEqual([
|
||||
'user',
|
||||
'assistant',
|
||||
'tool',
|
||||
'tool',
|
||||
'user',
|
||||
'assistant',
|
||||
]);
|
||||
expect(textContent(resumedAgain.agent.context.history[3])).toContain(
|
||||
'Tool execution was interrupted before its result was recorded',
|
||||
);
|
||||
expect(textContent(resumedAgain.agent.context.history[4])).toBe('continue after resume');
|
||||
});
|
||||
|
||||
it('rebuilds goal completion replay cards without adding model-visible context', async () => {
|
||||
const persistence = new RecordingAgentPersistence([
|
||||
{
|
||||
|
|
@ -583,6 +763,18 @@ function withMetadata(events: readonly AgentRecord[]): readonly AgentRecord[] {
|
|||
];
|
||||
}
|
||||
|
||||
function textContent(
|
||||
message:
|
||||
| { readonly content: readonly { readonly type: string; readonly text?: string }[] }
|
||||
| undefined,
|
||||
): string {
|
||||
return (
|
||||
message?.content
|
||||
.map((part) => (part.type === 'text' && typeof part.text === 'string' ? part.text : ''))
|
||||
.join('') ?? ''
|
||||
);
|
||||
}
|
||||
|
||||
function resumeHistory(): AgentRecord[] {
|
||||
return [
|
||||
{
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue