diff --git a/.changeset/dedup-register-rejected-calls.md b/.changeset/dedup-register-rejected-calls.md new file mode 100644 index 0000000000..525d2df3a9 --- /dev/null +++ b/.changeset/dedup-register-rejected-calls.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/agent-core": patch +--- + +Count validation-rejected tool calls toward the repeat breaker so reminders fire at 3/5/8 and the turn force-stops at 12. diff --git a/packages/agent-core/src/agent/turn/index.ts b/packages/agent-core/src/agent/turn/index.ts index 5b31756cbc..ccdeed9399 100644 --- a/packages/agent-core/src/agent/turn/index.ts +++ b/packages/agent-core/src/agent/turn/index.ts @@ -978,6 +978,16 @@ export class TurnFlow { return this.agent.permission.beforeToolCall(ctx); }, finalizeToolResult: async (ctx) => { + // Calls rejected in preflight (e.g. invalid args) never reach + // prepareToolExecution, so register them here — otherwise the + // repeat breaker cannot count them and the model can re-issue + // the same invalid call indefinitely. + deduper.registerSkipped( + ctx.toolCall.id, + ctx.toolCall.name, + ctx.args, + ctx.toolCall.arguments, + ); // Resolve dedup BEFORE firing the PostToolUse hook so same-step // dups (whose ctx.result is the dedup placeholder) report the // original's real outcome, not an empty success. diff --git a/packages/agent-core/src/agent/turn/tool-dedup.ts b/packages/agent-core/src/agent/turn/tool-dedup.ts index f57679c8f7..229daa079c 100644 --- a/packages/agent-core/src/agent/turn/tool-dedup.ts +++ b/packages/agent-core/src/agent/turn/tool-dedup.ts @@ -2,6 +2,7 @@ import type { ContentPart } from '@moonshot-ai/kosong'; import type { TelemetryClient } from '../../telemetry'; import type { LLMRequestTrace } from '../../loop/llm'; +import { parseToolCallArguments } from '../../loop/tool-args-parse'; import type { ExecutableToolResult } from '../../loop/types'; import { canonicalTelemetryArgs } from './canonical-args'; @@ -190,6 +191,35 @@ export class ToolCallDeduplicator { return null; } + /** + * Register a call that bypassed `prepareToolExecution` — e.g. args + * validation rejected it in preflight, so the prepare hook never ran. Must + * be called before `finalizeResult` for such calls, otherwise the repeat + * circuit breaker never counts rejected calls and the model can re-issue + * the same invalid call without ever tripping the streak. No-op when the + * call was already registered through the normal prepare path. + * + * `rawArguments` is the provider's raw arguments string. Args that failed + * JSON parsing were normalized to `{}` by the loop, which would key every + * malformed-but-different attempt identically; those are keyed on the raw + * text so only true re-issues count as repeats. + */ + registerSkipped( + toolCallId: string, + toolName: string, + args: unknown, + rawArguments?: string | null, + ): void { + if (this.callKeyByCallId.has(toolCallId)) return; + const keyArgs = + rawArguments !== undefined && + rawArguments !== null && + parseToolCallArguments(rawArguments).parseFailed + ? rawArguments + : args; + this.checkSameStep(toolCallId, toolName, keyArgs); + } + /** * Called from `finalizeToolResult`, in provider order. For first-occurrence * calls, projects the consecutive streak ending at this call and, if the diff --git a/packages/agent-core/test/agent/turn.test.ts b/packages/agent-core/test/agent/turn.test.ts index 6e4c46de0a..255b9da54f 100644 --- a/packages/agent-core/test/agent/turn.test.ts +++ b/packages/agent-core/test/agent/turn.test.ts @@ -793,6 +793,61 @@ describe('Agent turn flow', () => { }); }); + it('force-stops a turn that keeps re-issuing the same validation-rejected call', async () => { + const records: TelemetryRecord[] = []; + const ctx = testAgent({ + kaos: createCommandKaos('bad'), + telemetry: recordingTelemetry(records), + }); + ctx.configure({ tools: ['Bash'] }); + await ctx.rpc.setPermission({ mode: 'yolo' }); + records.length = 0; + + // 12 identical calls missing the required "command": each is rejected in + // preflight. If the breaker did not count them, the turn would keep going + // and consume the 13th scripted response. + for (let i = 0; i < 12; i += 1) { + ctx.mockNextResponse(invalidBashCallWithId(`call_bad_${String(i)}`)); + } + ctx.mockNextResponse({ type: 'text', text: 'must never be generated' }); + + await ctx.rpc.prompt({ input: [{ type: 'text', text: 'Repeat the bad call' }] }); + await ctx.untilTurnEnd(); + + expect(ctx.llmCalls).toHaveLength(12); + const actions = records + .filter((entry) => entry.event === 'tool_call_repeat') + .map((entry) => entry.properties?.['action']); + expect(actions).toEqual([ + 'none', 'r1', 'r1', 'r2', 'r2', 'r2', 'r3', 'r3', 'r3', 'r3', 'stop', + ]); + }); + + it('does not force-stop when the malformed argument text keeps changing', async () => { + const records: TelemetryRecord[] = []; + const ctx = testAgent({ + kaos: createCommandKaos('bad'), + telemetry: recordingTelemetry(records), + }); + ctx.configure({ tools: ['Bash'] }); + await ctx.rpc.setPermission({ mode: 'yolo' }); + records.length = 0; + + // 12 rejected calls, each with DIFFERENT malformed raw JSON: all normalize + // to {} on parse failure, but they are not repeats of the same call, so + // the turn must not be force-stopped. + for (let i = 0; i < 12; i += 1) { + ctx.mockNextResponse(malformedBashCallWithId(`call_mal_${String(i)}`, i)); + } + ctx.mockNextResponse({ type: 'text', text: 'recovered' }); + + await ctx.rpc.prompt({ input: [{ type: 'text', text: 'Repeat the bad call' }] }); + await ctx.untilTurnEnd(); + + expect(ctx.llmCalls).toHaveLength(13); + expect(records.filter((entry) => entry.event === 'tool_call_repeat')).toHaveLength(0); + }); + it('fires PostToolUse for same-step dups with the original real output, not the dedup placeholder', async () => { // Hook command asserts the dup's PostToolUse payload carries the real // stdout ('dup'), not the placeholder (''). @@ -2799,6 +2854,25 @@ function bashCallWithId(id: string, command: string): ToolCall { }; } +function invalidBashCallWithId(id: string): ToolCall { + return { + type: 'function', + id, + name: 'Bash', + arguments: JSON.stringify({ timeout: 60 }), + }; +} + +function malformedBashCallWithId(id: string, variant: number): ToolCall { + // Invalid JSON (unquoted key), unique per variant. + return { + type: 'function', + id, + name: 'Bash', + arguments: `{"command_${String(variant)}: "ls"`, + }; +} + function agentSwarmCall(): ToolCall { return { type: 'function', diff --git a/packages/agent-core/test/agent/turn/tool-dedup.test.ts b/packages/agent-core/test/agent/turn/tool-dedup.test.ts index f2aa09f9aa..e6919b5ad5 100644 --- a/packages/agent-core/test/agent/turn/tool-dedup.test.ts +++ b/packages/agent-core/test/agent/turn/tool-dedup.test.ts @@ -573,4 +573,78 @@ describe('ToolCallDeduplicator', () => { expect(true).toBe(true); }); }); + + describe('skipped registration (calls rejected before prepareToolExecution)', () => { + // Calls rejected in preflight (e.g. invalid args) never reach + // prepareToolExecution/checkSameStep; the loop registers them late via + // registerSkipped at finalize time so the breaker still counts them. + async function runRejected( + dedup: ToolCallDeduplicator, + callId: string, + result: ExecutableToolResult, + ): Promise { + dedup.registerSkipped(callId, 'Bash', { timeout: 60 }); + return dedup.finalizeResult(callId, 'Bash', { timeout: 60 }, result); + } + + it('counts skipped calls toward the streak and force-stops at 12, keeping the error flag', async () => { + const dedup = new ToolCallDeduplicator(); + let last: ExecutableToolResult | undefined; + for (let i = 0; i < 12; i += 1) { + dedup.beginStep(); + last = await runRejected(dedup, `c${String(i)}`, errResult('Invalid args')); + dedup.endStep(); + } + expect(last!.isError).toBe(true); + expect(last!.stopTurn).toBe(true); + expect(last!.output as string).toContain(REMINDER_TEXT_3.trim()); + }); + + it('does not double-register a call that already went through checkSameStep', async () => { + const { client, events } = makeRecordingTelemetry(); + const dedup = new ToolCallDeduplicator({ telemetry: client }); + for (let i = 0; i < 2; i += 1) { + dedup.beginStep(); + const callId = `c${String(i)}`; + expect(dedup.checkSameStep(callId, 'Read', { p: 1 })).toBeNull(); + dedup.registerSkipped(callId, 'Read', { p: 1 }); + await dedup.finalizeResult(callId, 'Read', { p: 1 }, okResult('R')); + dedup.endStep(); + } + // Exactly one repeat at count 2 — a double registration would inflate + // the streak and fire the reminder one occurrence early. + const repeats = events.filter((e) => e.event === 'tool_call_repeat'); + expect(repeats.map((e) => e.properties?.['repeat_count'])).toEqual([2]); + }); + + it('counts identical malformed argument texts as repeats', async () => { + const { client, events } = makeRecordingTelemetry(); + const dedup = new ToolCallDeduplicator({ telemetry: client }); + for (let i = 0; i < 2; i += 1) { + dedup.beginStep(); + const callId = `c${String(i)}`; + dedup.registerSkipped(callId, 'Bash', {}, '{"command":'); + await dedup.finalizeResult(callId, 'Bash', {}, errResult('Invalid args')); + dedup.endStep(); + } + const repeats = events.filter((e) => e.event === 'tool_call_repeat'); + expect(repeats.map((e) => e.properties?.['repeat_count'])).toEqual([2]); + }); + + it('does not treat different malformed argument texts as the same call', async () => { + const { client, events } = makeRecordingTelemetry(); + const dedup = new ToolCallDeduplicator({ telemetry: client }); + const raws = ['{"command":', '{"comand":', '{"command": "ls"']; + for (let i = 0; i < 3; i += 1) { + dedup.beginStep(); + const callId = `c${String(i)}`; + dedup.registerSkipped(callId, 'Bash', {}, raws[i]); + await dedup.finalizeResult(callId, 'Bash', {}, errResult('Invalid args')); + dedup.endStep(); + } + // All three normalize to {} on parse failure, but the raw texts differ, + // so no repeat streak may form. + expect(events.filter((e) => e.event === 'tool_call_repeat')).toHaveLength(0); + }); + }); });