Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 146 additions & 0 deletions packages/core/src/core/coreToolScheduler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4110,4 +4110,150 @@ describe('CoreToolScheduler validation retry loop detection', () => {
expect(msg).toBeDefined();
expect(msg).not.toContain(RETRY_LOOP_STOP_DIRECTIVE);
});

it('should isolate retry counters per-tool across batches', async () => {
// Regression: the batch-level continues-loop check used to keep *all*
// retry state whenever any current request matched a previously failing
// tool. That let stale counts for an unrelated tool survive long enough
// to fire RETRY LOOP DETECTED prematurely the next time that tool was
// called. The correct behaviour prunes counters per-tool: keep only
// counters whose tool name actually appears in the current batch.
class StrictToolAlt extends BaseDeclarativeTool<
{ other: string },
ToolResult
> {
static readonly Name = 'strictStringToolAlt';
constructor() {
super(
StrictToolAlt.Name,
'StrictStringToolAlt',
'Alt tool requiring string other param.',
Kind.Other,
{
type: 'object',
properties: { other: { type: 'string' } },
required: ['other'],
},
);
}
protected createInvocation(params: {
other: string;
}): ToolInvocation<{ other: string }, ToolResult> {
return new (class extends BaseToolInvocation<
{ other: string },
ToolResult
> {
constructor(p: { other: string }) {
super(p);
}
getDescription() {
return 'strictStringToolAlt invocation';
}
async execute(): Promise<ToolResult> {
return { llmContent: 'ok', returnDisplay: 'ok' };
}
})(params);
}
}

const toolA = new StrictStringTool();
const toolB = new StrictToolAlt();
const mockToolRegistry = {
ensureTool: async (name: string) =>
name === StrictStringTool.Name
? toolA
: name === StrictToolAlt.Name
? toolB
: undefined,
getTool: (name: string) =>
name === StrictStringTool.Name
? toolA
: name === StrictToolAlt.Name
? toolB
: undefined,
getFunctionDeclarations: () => [],
tools: new Map(),
discovery: {},
registerTool: () => {},
getToolByName: (name: string) =>
name === StrictStringTool.Name
? toolA
: name === StrictToolAlt.Name
? toolB
: undefined,
getToolByDisplayName: () => undefined,
getTools: () => [],
discoverTools: async () => {},
getAllTools: () => [],
getAllToolNames: () => [StrictStringTool.Name, StrictToolAlt.Name],
getToolsByServer: () => [],
} as unknown as ToolRegistry;

const mockConfig = {
getSessionId: () => 'test-session-id',
getUsageStatisticsEnabled: () => true,
getDebugMode: () => false,
getApprovalMode: () => ApprovalMode.YOLO,
getPermissionsAllow: () => [],
getContentGeneratorConfig: () => ({
model: 'test-model',
authType: 'gemini',
}),
getShellExecutionConfig: () => ({
terminalWidth: 90,
terminalHeight: 30,
}),
storage: { getProjectTempDir: () => '/tmp' },
getTruncateToolOutputThreshold: () => 100,
getTruncateToolOutputLines: () => 10,
getToolRegistry: () => mockToolRegistry,
getUseModelRouter: () => false,
getGeminiClient: () => null,
isInteractive: () => true,
getIdeMode: () => false,
getExperimentalZedIntegration: () => false,
getChatRecordingService: () => undefined,
getMessageBus: vi.fn().mockReturnValue(undefined),
getDisableAllHooks: vi.fn().mockReturnValue(true),
setApprovalMode: vi.fn(),
} as unknown as Config;

const onToolCallsUpdate = vi.fn();
const scheduler = new CoreToolScheduler({
config: mockConfig,
onAllToolCallsComplete: vi.fn(),
onToolCallsUpdate,
getPreferredEditor: () => 'vscode',
onEditorClose: vi.fn(),
});

// Tool A fails twice, accumulating a retry count of 2.
await scheduler.schedule(
[makeRequest('a1', StrictStringTool.Name, { value: 123 })],
new AbortController().signal,
);
await scheduler.schedule(
[makeRequest('a2', StrictStringTool.Name, { value: 123 })],
new AbortController().signal,
);

// Now a batch for tool B only — tool A's counter must be pruned because
// A is not present in this batch.
await scheduler.schedule(
[makeRequest('b1', StrictToolAlt.Name, { other: 456 })],
new AbortController().signal,
);

// Tool A fails once more. Under the old wholesale-keep behaviour this
// would be the third consecutive A failure and would trip the directive.
// Under per-tool pruning the counter starts fresh at 1 and no directive
// should be emitted.
await scheduler.schedule(
[makeRequest('a3', StrictStringTool.Name, { value: 123 })],
new AbortController().signal,
);
const msg = getLastErrorMessage(onToolCallsUpdate);
expect(msg).toBeDefined();
expect(msg).not.toContain(RETRY_LOOP_STOP_DIRECTIVE);
});
});
24 changes: 12 additions & 12 deletions packages/core/src/core/coreToolScheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -794,20 +794,20 @@ export class CoreToolScheduler {
}
const requestsToProcess = Array.isArray(request) ? request : [request];

// Check if this batch continues a validation retry loop.
// Keys are "<toolName>:<errorMessage>"; if no request reuses a tool name
// that previously failed validation, reset the tracker.
// Prune validation retry state per-tool, not wholesale. Keys are
// "<toolName>:<errorMessage>"; retain counters only for tools actually
// present in the current batch. Keeping every tracked tool's counters
// whenever any current request matched caused stale counts for
// unrelated tools to survive and fire RETRY LOOP DETECTED prematurely
// the next time those tools were used.
if (this.validationRetryCounts.size > 0) {
Comment thread
euxarist marked this conversation as resolved.
Resolved
const prevTools = new Set<string>();
for (const key of this.validationRetryCounts.keys()) {
const currentToolNames = new Set(requestsToProcess.map((r) => r.name));
for (const key of [...this.validationRetryCounts.keys()]) {
const sep = key.indexOf(':');
prevTools.add(sep === -1 ? key : key.slice(0, sep));
}
const hasPrevFailingTool = requestsToProcess.some((r) =>
prevTools.has(r.name),
);
if (!hasPrevFailingTool) {
this.validationRetryCounts.clear();
const toolName = sep === -1 ? key : key.slice(0, sep);
if (!currentToolNames.has(toolName)) {
this.validationRetryCounts.delete(key);
}
}
}

Expand Down
Loading
Loading