From db944811d18704972db6df209af0763c887ae364 Mon Sep 17 00:00:00 2001 From: 7Sageer <7sageer@djwcb.cn> Date: Tue, 28 Jul 2026 18:50:31 +0800 Subject: [PATCH 1/2] fix(agent-core-v2): count validation-rejected tool calls toward the repeat breaker --- .../v2-dedup-register-rejected-calls.md | 5 + .../agent-core-v2/docs/state-manifest.d.ts | 2 +- .../src/agent/toolDedupe/toolDedupeService.ts | 48 ++++- .../agent/toolExecutor/toolExecutorService.ts | 28 +-- .../src/agent/toolExecutor/toolHooks.ts | 4 +- .../agent-core-v2/src/tool/tool-args-parse.ts | 30 ++++ .../test/agent/toolDedupe/toolDedupe.test.ts | 168 +++++++++++++++++- .../agent/toolExecutor/toolExecutor.test.ts | 3 +- .../toolSelect/toolSelectService.test.ts | 2 + 9 files changed, 262 insertions(+), 28 deletions(-) create mode 100644 .changeset/v2-dedup-register-rejected-calls.md create mode 100644 packages/agent-core-v2/src/tool/tool-args-parse.ts diff --git a/.changeset/v2-dedup-register-rejected-calls.md b/.changeset/v2-dedup-register-rejected-calls.md new file mode 100644 index 0000000000..0908b88264 --- /dev/null +++ b/.changeset/v2-dedup-register-rejected-calls.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/agent-core-v2": 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-v2/docs/state-manifest.d.ts b/packages/agent-core-v2/docs/state-manifest.d.ts index 90050ef3ba..4f6bded88c 100644 --- a/packages/agent-core-v2/docs/state-manifest.d.ts +++ b/packages/agent-core-v2/docs/state-manifest.d.ts @@ -1010,7 +1010,7 @@ export interface AgentStateSnapshot { 'llmRequester.lastConfigLogSignature': string | undefined; 'llmRequester.mediaDegradedTurns': Set; 'llmRequester.mediaStrippedTurns': Map; 'llmRequester.turnConfigs': Map { + // Calls rejected in preflight (e.g. invalid args) never reach + // onBeforeExecuteTool, so register them here — otherwise the repeat + // breaker cannot count them and the model can re-issue the same + // invalid call indefinitely. + this.registerSkipped( + ctx.toolCall.id, + ctx.toolCall.name, + ctx.args, + ctx.toolCall.arguments, + ctx.trace, + ); ctx.result = await this.finalizeResult( ctx.toolCall.id, ctx.toolCall.name, @@ -305,6 +320,37 @@ export class AgentToolDedupeService extends Disposable implements IAgentToolDedu return { syntheticResult: null }; } + /** + * Register a call that bypassed `onBeforeExecuteTool` — e.g. args + * validation rejected it in preflight, so the veto event never fired. + * 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 + * before-execute path. + * + * `rawArguments` is the provider's raw arguments string. Args that failed + * JSON parsing were normalized to `{}` by the executor, which would key + * every malformed-but-different attempt identically; those are keyed on + * the raw text so only true re-issues count as repeats. + */ + private registerSkipped( + toolCallId: string, + toolName: string, + args: unknown, + rawArguments: unknown, + trace: LLMRequestTrace | undefined, + ): void { + if (this.callKeyByCallId.has(toolCallId)) return; + const keyArgs = + rawArguments !== undefined && + rawArguments !== null && + parseToolCallArguments(rawArguments).parseFailed + ? rawArguments + : args; + this.checkToolCall(toolCallId, toolName, keyArgs, trace); + } + private recordDupType( toolCallId: string, toolName: string, diff --git a/packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts b/packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts index 076e646eb3..035bff170b 100644 --- a/packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts +++ b/packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts @@ -27,6 +27,7 @@ import { type JsonType, type ToolArgsValidator, } from '#/tool/args-validator'; +import { parseToolCallArguments } from '#/tool/tool-args-parse'; import { PathSecurityError } from '#/tool/path-access'; import { isAbortError, isUserCancellation } from '#/_base/utils/abort'; import { IEventBus } from '#/app/event/eventBus'; @@ -595,17 +596,16 @@ export class AgentToolExecutorService implements IAgentToolExecutorService { result: ToolResult, options: ToolExecutorExecuteOptions, ): Promise { - if (call.kind === 'rejected') { - return result; - } - + // Preflight-rejected calls run the hook too: they bypassed + // `onBeforeExecuteTool`, and this is the only interception point every + // call still passes through (the repeat breaker registers them here). const didCtx: ToolDidExecuteContext = { turnId: options.turnId, signal: options.signal, trace: options.trace, toolCall: call.toolCall, toolCalls: [call.toolCall], - tool: call.tool, + tool: call.kind === 'runnable' ? call.tool : undefined, args: call.args, result: result as ExecutableToolResult, }; @@ -756,24 +756,6 @@ function preflightToolCall( return { kind: 'runnable', toolCall, toolName, tool, args: parsedArgs.data }; } -export function parseToolCallArguments(raw: unknown): { - readonly data: unknown; - readonly parseFailed: boolean; - readonly error?: string; -} { - if (raw === null || raw === undefined || (typeof raw === 'string' && raw.length === 0)) { - return { data: {}, parseFailed: false }; - } - if (typeof raw !== 'string') { - return { data: raw, parseFailed: false }; - } - try { - return { data: JSON.parse(raw) as unknown, parseFailed: false }; - } catch (error) { - return { data: {}, parseFailed: true, error: errorMessage(error) }; - } -} - function validateExecutableToolArgs(tool: ExecutableTool, args: unknown): string | null { let validator = validators.get(tool); if (validator === undefined) { diff --git a/packages/agent-core-v2/src/agent/toolExecutor/toolHooks.ts b/packages/agent-core-v2/src/agent/toolExecutor/toolHooks.ts index e4fa4e6221..9cc7d84bd2 100644 --- a/packages/agent-core-v2/src/agent/toolExecutor/toolHooks.ts +++ b/packages/agent-core-v2/src/agent/toolExecutor/toolHooks.ts @@ -18,7 +18,9 @@ * `waitUntil(promise)`; the executor awaits all of them before dispatching * an allowed call (e.g. MCP initial load). * - `hooks.onDidExecuteTool` (ordered hook slot, `ToolDidExecuteContext`): - * post-execution result finalization, kept as an `OrderedHookSlot`. + * post-execution result finalization, kept as an `OrderedHookSlot`. Every + * call reaches it — including preflight-rejected ones (missing/unavailable + * tool, guard denial, invalid args), which arrive without `tool` set. * * Participants such as `permissionGate`, `toolDedupe`, `externalHooks`, * `goal`, `plan`, `swarm`, `btw`, and `mcp` register through these surfaces. diff --git a/packages/agent-core-v2/src/tool/tool-args-parse.ts b/packages/agent-core-v2/src/tool/tool-args-parse.ts new file mode 100644 index 0000000000..9e9145f901 --- /dev/null +++ b/packages/agent-core-v2/src/tool/tool-args-parse.ts @@ -0,0 +1,30 @@ +/** + * `tool` domain (L3) — tool-call arguments parsing. + * + * Decodes the provider's raw `arguments` payload into a plain value. A + * payload that fails JSON parsing is normalized to `{}` and flagged with + * `parseFailed`, so callers can tell "the model sent an empty object" apart + * from "the model sent malformed text". Pure helper; no scoped service. + */ + +export function parseToolCallArguments(raw: unknown): { + readonly data: unknown; + readonly parseFailed: boolean; + readonly error?: string; +} { + if (raw === null || raw === undefined || (typeof raw === 'string' && raw.length === 0)) { + return { data: {}, parseFailed: false }; + } + if (typeof raw !== 'string') { + return { data: raw, parseFailed: false }; + } + try { + return { data: JSON.parse(raw) as unknown, parseFailed: false }; + } catch (error) { + return { + data: {}, + parseFailed: true, + error: error instanceof Error ? error.message : String(error), + }; + } +} diff --git a/packages/agent-core-v2/test/agent/toolDedupe/toolDedupe.test.ts b/packages/agent-core-v2/test/agent/toolDedupe/toolDedupe.test.ts index 85f464d717..2035a0e461 100644 --- a/packages/agent-core-v2/test/agent/toolDedupe/toolDedupe.test.ts +++ b/packages/agent-core-v2/test/agent/toolDedupe/toolDedupe.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { DisposableStore } from '#/_base/di/lifecycle'; import { createServices, type TestInstantiationService } from '#/_base/di/test'; @@ -8,8 +8,10 @@ import { type ToolCall } from '#/kosong/contract/message'; import { emptyUsage } from '#/kosong/contract/usage'; import { ITelemetryService } from '#/app/telemetry/telemetry'; import { ISessionContext } from '#/session/sessionContext/sessionContext'; +import type { ISessionProcessRunner } from '#/session/process/processRunner'; import { IAgentScopeContext } from '#/agent/scopeContext/scopeContext'; import { IAgentLoopService } from '#/agent/loop/loop'; +import { IAgentProfileService } from '#/agent/profile/profile'; import { IAgentStateService } from '#/agent/state/agentState'; import { AgentStateService } from '#/agent/state/agentStateService'; import type { ExecutableTool, ExecutableToolContext, ExecutableToolResult, ToolExecution, ToolResult } from '#/tool/toolContract'; @@ -26,6 +28,8 @@ import { stubLoopWithHooks } from '../loop/stubs'; import { stubToolExecutorEvents } from '../toolExecutor/stubs'; import { registerToolResultTruncationServices } from '../toolResultTruncation/stubs'; import { registerTestAgentWireServices } from '../../wire/stubs'; +import { createTestAgent, execEnvServices, telemetryServices } from '../../harness'; +import { createFakeProcessRunner } from '../../tools/fixtures/fake-exec'; const { REMINDER_TEXT_1, REMINDER_TEXT_3, makeReminderText2 } = toolDedupeTesting; const ZERO_USAGE = emptyUsage(); @@ -832,4 +836,166 @@ describe('AgentToolDedupeService', () => { expect(telemetryEvents.filter((e) => e.event === 'tool_call_repeat')).toHaveLength(0); }); }); + + describe('preflight-rejected calls (bypass onBeforeExecuteTool)', () => { + // Calls rejected by args validation in preflight never fire + // onBeforeExecuteTool; the dedupe hook registers them late at + // onDidExecuteTool time so the repeat breaker still counts them. + class StrictTool implements ExecutableTool> { + readonly name = 'Strict'; + readonly description = 'Requires a command string.'; + readonly parameters = { + type: 'object', + properties: { command: { type: 'string' } }, + required: ['command'], + additionalProperties: true, + }; + readonly calls: Array> = []; + + resolveExecution(args: Record): ToolExecution { + return { + approvalRule: this.name, + execute: async () => { + this.calls.push(args); + return { output: 'ran' }; + }, + }; + } + } + + function invalidCall(id: string): ToolCall { + // Missing the required "command". + return { type: 'function', id, name: 'Strict', arguments: JSON.stringify({ timeout: 60 }) }; + } + + function malformedCall(id: string, rawArguments: string): ToolCall { + return { type: 'function', id, name: 'Strict', arguments: rawArguments }; + } + + it('counts rejected calls toward the streak and force-stops at 12, keeping the error flag', async () => { + const h = createHarness(); + const tool = new StrictTool(); + h.registry.register(tool); + let last: ToolResult | undefined; + for (let i = 0; i < 12; i += 1) { + const [result] = await runStep(h, 1, i + 1, [invalidCall(`c${String(i)}`)]); + last = result!.result; + } + expect(tool.calls).toHaveLength(0); + expect(last!.isError).toBe(true); + expect(last!.stopTurn).toBe(true); + expect(last!.output as string).toContain(REMINDER_TEXT_3.trim()); + const actions = telemetryEvents + .filter((e) => e.event === 'tool_call_repeat') + .map((e) => e.properties?.['action']); + expect(actions).toEqual(['none', 'r1', 'r1', 'r2', 'r2', 'r2', 'r3', 'r3', 'r3', 'r3', 'stop']); + }); + + it('does not double-register a call that already went through onBeforeExecuteTool', async () => { + const h = createHarness(undefined, { executorEvents: true }); + for (let i = 0; i < 2; i += 1) { + await beforeStep(h, 1, i + 1); + const callId = `c${String(i)}`; + expect(await h.fireBefore(willCtx(callId, 'Read', { p: 1 }))).toBeUndefined(); + const d = didCtx(callId, 'Read', { p: 1 }, okResult('R')); + await h.executor.hooks.onDidExecuteTool.run(d); + await afterStep(h, 1, i + 1); + } + // Exactly one repeat at count 2 — a double registration would inflate + // the streak and fire the reminder one occurrence early. + const repeats = telemetryEvents.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 h = createHarness(); + h.registry.register(new StrictTool()); + for (let i = 0; i < 2; i += 1) { + await runStep(h, 1, i + 1, [malformedCall(`c${String(i)}`, '{"command":')]); + } + const repeats = telemetryEvents.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 h = createHarness(); + h.registry.register(new StrictTool()); + const raws = ['{"command":', '{"comand":', '{"command": "ls"']; + for (let i = 0; i < 3; i += 1) { + await runStep(h, 1, i + 1, [malformedCall(`c${String(i)}`, raws[i]!)]); + } + // All three normalize to {} on parse failure, but the raw texts + // differ, so no repeat streak may form. + expect(telemetryEvents.filter((e) => e.event === 'tool_call_repeat')).toHaveLength(0); + }); + }); + + describe('turn-level repeat breaker for rejected calls', () => { + function invalidBashCallWithId(id: string): ToolCall { + // Missing the required "command". + 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 rejectedBashAgent(records: TelemetryRecord[]): { + readonly ctx: ReturnType; + readonly exec: ReturnType; + } { + const exec = vi.fn().mockRejectedValue(new Error('Bash should not execute')); + const ctx = createTestAgent( + telemetryServices(recordingTelemetry(records)), + execEnvServices({ processRunner: createFakeProcessRunner({ exec: exec as unknown as ISessionProcessRunner['exec'] }) }), + ); + ctx.get(IAgentProfileService).update({ activeToolNames: ['Bash'] }); + records.length = 0; + return { ctx, exec }; + } + + it('force-stops a turn that keeps re-issuing the same validation-rejected call', async () => { + const records: TelemetryRecord[] = []; + const { ctx, exec } = rejectedBashAgent(records); + + // 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(exec).not.toHaveBeenCalled(); + 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, exec } = rejectedBashAgent(records); + + // 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(exec).not.toHaveBeenCalled(); + expect(ctx.llmCalls).toHaveLength(13); + expect(records.filter((entry) => entry.event === 'tool_call_repeat')).toHaveLength(0); + }); + }); }); diff --git a/packages/agent-core-v2/test/agent/toolExecutor/toolExecutor.test.ts b/packages/agent-core-v2/test/agent/toolExecutor/toolExecutor.test.ts index e115737f7b..f0842a968c 100644 --- a/packages/agent-core-v2/test/agent/toolExecutor/toolExecutor.test.ts +++ b/packages/agent-core-v2/test/agent/toolExecutor/toolExecutor.test.ts @@ -17,7 +17,8 @@ import { } from '#/tool/toolContract'; import { IAgentToolExecutorService } from '#/agent/toolExecutor/toolExecutor'; import type { BeforeToolExecuteEvent } from '#/agent/toolExecutor/toolHooks'; -import { AgentToolExecutorService, parseToolCallArguments } from '#/agent/toolExecutor/toolExecutorService'; +import { AgentToolExecutorService } from '#/agent/toolExecutor/toolExecutorService'; +import { parseToolCallArguments } from '#/tool/tool-args-parse'; import { IAgentToolResultTruncationService } from '#/agent/toolResultTruncation/toolResultTruncation'; import { makeAgentScopeContext, IAgentScopeContext } from '#/agent/scopeContext/scopeContext'; import { IAgentToolRegistryService } from '#/agent/toolRegistry/toolRegistry'; diff --git a/packages/agent-core-v2/test/agent/toolSelect/toolSelectService.test.ts b/packages/agent-core-v2/test/agent/toolSelect/toolSelectService.test.ts index 0258f7b2ef..d3009bfb0d 100644 --- a/packages/agent-core-v2/test/agent/toolSelect/toolSelectService.test.ts +++ b/packages/agent-core-v2/test/agent/toolSelect/toolSelectService.test.ts @@ -857,6 +857,7 @@ describe('AgentToolSelectService executor interception', () => { `Tool "${MCP_ALPHA}" is available but not loaded. ` + `Call select_tools with ["${MCP_ALPHA}"] first, then call the tool.`, isError: true, + stopTurn: false, }); expect(alpha.calls).toBe(0); }); @@ -887,6 +888,7 @@ describe('AgentToolSelectService executor interception', () => { output: `Tool "${MCP_ALPHA}" was loaded but is no longer active. Ask the user to enable it before calling it again.`, isError: true, + stopTurn: false, }); expect(alpha.calls).toBe(0); }); From 9d99e64d8f0f24d49563a41ff4df7b00167afa01 Mon Sep 17 00:00:00 2001 From: 7Sageer <7sageer@djwcb.cn> Date: Tue, 28 Jul 2026 19:29:16 +0800 Subject: [PATCH 2/2] style(agent-core-v2): remove inline implementation comments --- .../src/agent/toolDedupe/toolDedupeService.ts | 23 +------------------ .../agent/toolExecutor/toolExecutorService.ts | 3 --- 2 files changed, 1 insertion(+), 25 deletions(-) diff --git a/packages/agent-core-v2/src/agent/toolDedupe/toolDedupeService.ts b/packages/agent-core-v2/src/agent/toolDedupe/toolDedupeService.ts index c78e23e46b..e28e897129 100644 --- a/packages/agent-core-v2/src/agent/toolDedupe/toolDedupeService.ts +++ b/packages/agent-core-v2/src/agent/toolDedupe/toolDedupeService.ts @@ -5,10 +5,7 @@ * hooks, an `onBeforeExecuteTool` veto listener (same-step duplicates are * vetoed with a placeholder synthetic result), and an `onDidExecuteTool` * hook to drive same-step suppression and cross-step repeat reminders, and - * reports repeat telemetry through `telemetry`. Calls rejected in preflight - * never reach `onBeforeExecuteTool`; the `onDidExecuteTool` hook registers - * them late (`registerSkipped`) so the repeat breaker still counts them. - * The mutable dedupe state + * reports repeat telemetry through `telemetry`. The mutable dedupe state * (`stepCalls`, `originalCallIndex`, `syntheticCallIds`, `callKeyByCallId`, * `consecutiveKey`, `consecutiveCount`, `activeTurnId`, `activeStep`) is * registered into `agentState` (`IAgentStateService`) and read/written @@ -183,10 +180,6 @@ export class AgentToolDedupeService extends Disposable implements IAgentToolDedu } }); toolExecutor.hooks.onDidExecuteTool.register('toolDedupe', async (ctx, next) => { - // Calls rejected in preflight (e.g. invalid args) never reach - // onBeforeExecuteTool, so register them here — otherwise the repeat - // breaker cannot count them and the model can re-issue the same - // invalid call indefinitely. this.registerSkipped( ctx.toolCall.id, ctx.toolCall.name, @@ -320,20 +313,6 @@ export class AgentToolDedupeService extends Disposable implements IAgentToolDedu return { syntheticResult: null }; } - /** - * Register a call that bypassed `onBeforeExecuteTool` — e.g. args - * validation rejected it in preflight, so the veto event never fired. - * 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 - * before-execute path. - * - * `rawArguments` is the provider's raw arguments string. Args that failed - * JSON parsing were normalized to `{}` by the executor, which would key - * every malformed-but-different attempt identically; those are keyed on - * the raw text so only true re-issues count as repeats. - */ private registerSkipped( toolCallId: string, toolName: string, diff --git a/packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts b/packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts index 035bff170b..911d584933 100644 --- a/packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts +++ b/packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts @@ -596,9 +596,6 @@ export class AgentToolExecutorService implements IAgentToolExecutorService { result: ToolResult, options: ToolExecutorExecuteOptions, ): Promise { - // Preflight-rejected calls run the hook too: they bypassed - // `onBeforeExecuteTool`, and this is the only interception point every - // call still passes through (the repeat breaker registers them here). const didCtx: ToolDidExecuteContext = { turnId: options.turnId, signal: options.signal,