Skip to content

Commit 4750cdf

Browse files
oratisclaude
andauthored
fix(core): agent-loop & provider correctness (compaction, parallel tools, truncation) (#80)
* chore(tooling): enforce lint in pre-commit + ignore Rust build output eslint was scanning apps/desktop/src-tauri/target/ (generated Cargo JS), producing 49 spurious errors locally — CI only escaped because it lints before cargo build. Add target/ + release-artifacts/ to the ignore list, then wire `pnpm lint` into the pre-commit hook so lint regressions are caught before they land instead of only in CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(core): agent-loop & provider correctness Three independent fixes in the core engine: 1. Auto-compact fired every turn once over the threshold. `shouldCompact` read the cumulative `totalUsage` (summed across all turns, never reset), so it both triggered far too early (each turn re-sends the whole history, inflating the sum) and, once over the line, re-compacted the already- compacted history on every subsequent turn. Use the latest turn's `result.usage.inputTokens` — the true current-context size — instead. 2. Read-only tools now execute concurrently. The per-turn tool loop awaited each call sequentially; Read/Grep/Glob/WebFetch/WebSearch are side-effect- free and independent, so they run via Promise.all while mutating tools (Edit/Write/Bash/…) stay sequential to preserve snapshot ordering and one-at-a-time approval prompts. Results are reassembled in the model's original order via tool_use_id. 3. A max_tokens-truncated tool call is no longer executed as garbage. When the completion is cut off mid-arguments the partial JSON won't parse and `input` became {} — e.g. a Write with no file_path. The DeepSeek provider now drops such calls and reports stopReason 'max_tokens' so the loop ends cleanly instead of executing a malformed call. Adds regression tests for all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3e806ef commit 4750cdf

4 files changed

Lines changed: 311 additions & 32 deletions

File tree

packages/core/src/agent.test.ts

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,157 @@ describe('runAgent', () => {
278278
}
279279
});
280280

281+
it('runs multiple read-only tool calls concurrently and preserves result order', async () => {
282+
const events2: string[] = [];
283+
const delay = (ms: number) => new Promise((r) => setTimeout(r, ms));
284+
const slowReadOnly = (name: string) => ({
285+
name,
286+
definition: { name, description: name, inputSchema: { type: 'object', properties: {} } },
287+
async execute() {
288+
events2.push(`start:${name}`);
289+
await delay(20);
290+
events2.push(`end:${name}`);
291+
return { content: `${name} done` };
292+
},
293+
});
294+
// Custom registry with two read-only-named tools (Grep + Glob ∈ READ_ONLY_TOOLS).
295+
const tools = new ToolRegistry([
296+
slowReadOnly('Grep'),
297+
slowReadOnly('Glob'),
298+
] as unknown as Parameters<typeof ToolRegistry.prototype.register>[0][]);
299+
300+
const provider = new MockProvider([
301+
{
302+
content: [
303+
{ type: 'text', text: 'searching' },
304+
{ type: 'tool_use', id: 'g1', name: 'Grep', input: {} },
305+
{ type: 'tool_use', id: 'g2', name: 'Glob', input: {} },
306+
],
307+
stopReason: 'tool_use',
308+
usage: { inputTokens: 1, outputTokens: 1, reasoningTokens: 0, cacheReadTokens: 0 },
309+
},
310+
endTurn('done'),
311+
]);
312+
313+
const result = await runAgent({
314+
provider,
315+
tools,
316+
systemPrompt: '',
317+
userMessage: 'find things',
318+
model: 'deepseek-chat',
319+
cwd,
320+
});
321+
322+
// Concurrency: both tools start before either finishes.
323+
expect(events2.slice(0, 2).every((e) => e.startsWith('start:'))).toBe(true);
324+
expect(events2.slice(2).every((e) => e.startsWith('end:'))).toBe(true);
325+
326+
// Result order matches the model's call order (Grep then Glob) regardless of
327+
// which promise settled first.
328+
const toolResultMsg = result.history[2]!; // user msg with tool_result blocks
329+
expect(toolResultMsg.role).toBe('user');
330+
const ids = toolResultMsg.content
331+
.filter((b): b is Extract<ContentBlock, { type: 'tool_result' }> => b.type === 'tool_result')
332+
.map((b) => b.tool_use_id);
333+
expect(ids).toEqual(['g1', 'g2']);
334+
});
335+
336+
it('does not auto-compact on cumulative usage when each turn is below threshold', async () => {
337+
// Regression: shouldCompact must use the *current* turn's input tokens, not
338+
// the cumulative sum across turns. contextWindow 100, threshold 0.8 → trigger
339+
// at 80. Each turn reports inputTokens 30 (below 80), so the per-turn proxy
340+
// never crosses — but the cumulative sum (30+30+30=90) would, under the old
341+
// buggy logic, fire compaction on turn 3. Assert it never fires.
342+
await fs.writeFile(join(cwd, 'x.txt'), 'data');
343+
344+
// A provider that counts how many times the compaction summarizer runs
345+
// (identified by the compaction system prompt + empty tool list).
346+
let summarizerCalls = 0;
347+
const turn = (): ProviderResult => ({
348+
content: withToolCall('working', {
349+
type: 'tool_use',
350+
id: `c${Math.random()}`,
351+
name: 'Read',
352+
input: { file_path: 'x.txt' },
353+
}),
354+
stopReason: 'tool_use',
355+
usage: { inputTokens: 30, outputTokens: 0, reasoningTokens: 0, cacheReadTokens: 0 },
356+
});
357+
const scripted: ProviderResult[] = [turn(), turn(), endTurn('done')];
358+
const countingProvider: Provider = {
359+
name: 'counting',
360+
async runTurn(opts: ProviderRunOpts): Promise<ProviderResult> {
361+
if (opts.systemPrompt.startsWith('You compress long agent conversations')) {
362+
summarizerCalls++;
363+
return endTurn('summary');
364+
}
365+
const next = scripted.shift();
366+
if (!next) throw new Error('no scripted response');
367+
return next;
368+
},
369+
};
370+
371+
const result = await runAgent({
372+
provider: countingProvider,
373+
tools: new ToolRegistry(),
374+
systemPrompt: 'agent',
375+
userMessage: 'go',
376+
model: 'deepseek-chat',
377+
cwd,
378+
autoCompact: { contextWindow: 100, threshold: 0.8 },
379+
});
380+
381+
expect(result.stopReason).toBe('end_turn');
382+
expect(summarizerCalls).toBe(0);
383+
});
384+
385+
it('auto-compacts once when a single turn crosses the threshold', async () => {
386+
// Inverse of the above: when the *current* turn's input alone exceeds the
387+
// threshold (90 > 80), compaction should fire. History after one tool turn
388+
// is short, so compact() keeps it verbatim, but the summarizer is still
389+
// invoked — proving the trigger path is live.
390+
await fs.writeFile(join(cwd, 'x.txt'), 'data');
391+
let summarizerCalls = 0;
392+
const scripted: ProviderResult[] = [
393+
{
394+
content: withToolCall('working', {
395+
type: 'tool_use',
396+
id: 'big',
397+
name: 'Read',
398+
input: { file_path: 'x.txt' },
399+
}),
400+
stopReason: 'tool_use',
401+
usage: { inputTokens: 90, outputTokens: 0, reasoningTokens: 0, cacheReadTokens: 0 },
402+
},
403+
endTurn('done'),
404+
];
405+
const provider: Provider = {
406+
name: 'counting',
407+
async runTurn(opts: ProviderRunOpts): Promise<ProviderResult> {
408+
if (opts.systemPrompt.startsWith('You compress long agent conversations')) {
409+
summarizerCalls++;
410+
return endTurn('summary');
411+
}
412+
const next = scripted.shift();
413+
if (!next) throw new Error('no scripted response');
414+
return next;
415+
},
416+
};
417+
418+
await runAgent({
419+
provider,
420+
tools: new ToolRegistry(),
421+
systemPrompt: 'agent',
422+
userMessage: 'go',
423+
model: 'deepseek-chat',
424+
cwd,
425+
// Tiny keep window so compact() doesn't short-circuit on the short history.
426+
autoCompact: { contextWindow: 100, threshold: 0.8, keepFirstPairs: 0, keepLastMessages: 1 },
427+
});
428+
429+
expect(summarizerCalls).toBe(1);
430+
});
431+
281432
it('honors systemReminders: false to skip injection entirely', async () => {
282433
const provider = new MockProvider([endTurn('hi')]);
283434
const tools = new ToolRegistry();

packages/core/src/agent.ts

Lines changed: 67 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,14 @@ export interface RunAgentResult {
101101

102102
const DEFAULT_MAX_TURNS = 16;
103103

104+
/**
105+
* Tools with no side effects whose results don't depend on each other — safe to
106+
* execute concurrently within a single turn. Everything else (Edit/Write/Bash/
107+
* TodoWrite/AskUserQuestion/ExitPlanMode) runs sequentially to preserve snapshot
108+
* ordering, mutation order, and one-at-a-time interactive prompts.
109+
*/
110+
const READ_ONLY_TOOLS = new Set(['Read', 'Grep', 'Glob', 'WebFetch', 'WebSearch']);
111+
104112
/**
105113
* Runs the agent loop until the model produces an end_turn (no tool calls),
106114
* or `maxTurns` is reached, or the abort signal fires.
@@ -233,14 +241,27 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
233241
return { history, turnsUsed, usage: totalUsage, stopReason: 'end_turn', modeSignal };
234242
}
235243

236-
// Execute tool calls and append a single user-role message with tool_result blocks
237-
const toolResults: ToolResultBlock[] = [];
238-
for (const block of result.content) {
239-
if (block.type !== 'tool_use') continue;
240-
const toolUse = block as ToolUseBlock;
244+
// Execute tool calls and append a single user-role message with tool_result
245+
// blocks. Two phases:
246+
// 1. (sequential) resolve handler + permission for each call. Approval
247+
// prompts must never overlap, so gating stays strictly ordered.
248+
// 2. (mixed) execute. Side-effect-free reads run concurrently via
249+
// Promise.all (the common "model emits 3 Reads at once" case); tools
250+
// that mutate state / snapshot run sequentially to preserve ordering.
251+
// tool_result blocks carry their tool_use_id, so the final array is
252+
// re-assembled in the model's original order regardless of finish order.
253+
const toolBlocks = result.content.filter(
254+
(b): b is ToolUseBlock => b.type === 'tool_use',
255+
);
256+
const resultsById = new Map<string, ToolResultBlock>();
257+
type Ready = { toolUse: ToolUseBlock; handler: NonNullable<ReturnType<typeof opts.tools.get>> };
258+
const ready: Ready[] = [];
259+
260+
// Phase 1 — sequential gate + approval.
261+
for (const toolUse of toolBlocks) {
241262
const handler = opts.tools.get(toolUse.name);
242263
if (!handler) {
243-
toolResults.push({
264+
resultsById.set(toolUse.id, {
244265
type: 'tool_result',
245266
tool_use_id: toolUse.id,
246267
content: `Error: tool not found: ${toolUse.name}`,
@@ -268,7 +289,7 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
268289
allowed = decision === true || decision === 'always';
269290
}
270291
if (!allowed) {
271-
toolResults.push({
292+
resultsById.set(toolUse.id, {
272293
type: 'tool_result',
273294
tool_use_id: toolUse.id,
274295
content: `Tool call blocked: ${verdict.reason}`,
@@ -287,12 +308,16 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
287308
}
288309
}
289310

290-
// Pre-execution snapshot (Edit/Write only)
291-
if (
292-
opts.enableSnapshots !== false &&
293-
opts.session &&
294-
(toolUse.name === 'Edit' || toolUse.name === 'Write')
295-
) {
311+
ready.push({ toolUse, handler });
312+
}
313+
314+
// Runs one approved tool end-to-end: pre-snapshot, execute, PostToolUse
315+
// hook, post-snapshot, event + result. Side-effect-free tools call this
316+
// concurrently; mutating tools call it one at a time (see partition below).
317+
const execOne = async ({ toolUse, handler }: Ready): Promise<void> => {
318+
const isFileMutation = toolUse.name === 'Edit' || toolUse.name === 'Write';
319+
320+
if (opts.enableSnapshots !== false && opts.session && isFileMutation) {
296321
const filePath = (toolUse.input as { file_path?: string }).file_path;
297322
if (filePath) {
298323
await opts.session.manager.snapshot({
@@ -327,13 +352,7 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
327352
});
328353
}
329354

330-
// Post-execution snapshot
331-
if (
332-
opts.enableSnapshots !== false &&
333-
opts.session &&
334-
(toolUse.name === 'Edit' || toolUse.name === 'Write') &&
335-
!tr.isError
336-
) {
355+
if (opts.enableSnapshots !== false && opts.session && isFileMutation && !tr.isError) {
337356
const filePath = (toolUse.input as { file_path?: string }).file_path;
338357
if (filePath) {
339358
await opts.session.manager.snapshot({
@@ -347,13 +366,26 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
347366
}
348367

349368
opts.onEvent?.({ type: 'tool_result', id: toolUse.id, result: tr });
350-
toolResults.push({
369+
resultsById.set(toolUse.id, {
351370
type: 'tool_result',
352371
tool_use_id: toolUse.id,
353372
content: tr.content,
354373
is_error: tr.isError,
355374
});
356-
}
375+
};
376+
377+
// Phase 2 — execute. Read-only tools have no side effects and don't touch
378+
// snapshotSeq, so they're safe to run concurrently; everything else stays
379+
// sequential to keep snapshot ordering deterministic.
380+
const parallel = ready.filter((r) => READ_ONLY_TOOLS.has(r.toolUse.name));
381+
const serial = ready.filter((r) => !READ_ONLY_TOOLS.has(r.toolUse.name));
382+
await Promise.all(parallel.map(execOne));
383+
for (const r of serial) await execOne(r);
384+
385+
// Re-assemble in the model's original tool-call order.
386+
const toolResults: ToolResultBlock[] = toolBlocks
387+
.map((b) => resultsById.get(b.id))
388+
.filter((r): r is ToolResultBlock => r !== undefined);
357389

358390
const resultMsg: StoredMessage = {
359391
role: 'user',
@@ -363,12 +395,22 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
363395
history.push(resultMsg);
364396
if (opts.session) await opts.session.manager.append(opts.session.id, resultMsg);
365397

366-
// M3c: auto-compact if usage crossed threshold
398+
// M3c: auto-compact if the *current* context crossed the threshold.
399+
//
400+
// Use this turn's usage (result.usage), NOT the cumulative totalUsage.
401+
// `result.usage.inputTokens` is exactly the size of the history we just
402+
// sent to the model, so it is the true current-context proxy. Cumulative
403+
// usage is wrong on two counts: it sums every turn's input (each turn
404+
// re-sends the whole history, so it inflates far past the real window and
405+
// crosses the threshold too early), and it never shrinks after a compaction
406+
// — meaning once over the line it would re-compact the already-compacted
407+
// history on every subsequent turn. The next turn's inputTokens naturally
408+
// reflects the freshly-compacted (smaller) context, so this self-corrects.
367409
if (
368410
opts.autoCompact &&
369411
shouldCompact({
370-
inputTokens: totalUsage.inputTokens,
371-
outputTokens: totalUsage.outputTokens,
412+
inputTokens: result.usage.inputTokens,
413+
outputTokens: result.usage.outputTokens,
372414
contextWindow: opts.autoCompact.contextWindow,
373415
threshold: opts.autoCompact.threshold,
374416
})

packages/core/src/providers/deepseek.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,72 @@ describe('DeepSeekProvider', () => {
139139
expect(result.content[0].input).toEqual({ file_path: 'src/a.ts' });
140140
}
141141
});
142+
143+
it('drops a tool call truncated by max_tokens instead of executing garbage', async () => {
144+
// Model started a Write but the completion was cut off mid-arguments
145+
// (finish_reason 'length'). The partial JSON won't parse.
146+
const chunks = [
147+
{ choices: [{ delta: { content: 'Creating the file' } }] },
148+
{
149+
choices: [
150+
{
151+
delta: {
152+
tool_calls: [
153+
{ index: 0, id: 'call_w', function: { name: 'Write', arguments: '{"file_path":"a.js","content":"cons' } },
154+
],
155+
},
156+
},
157+
],
158+
},
159+
{
160+
choices: [{ delta: {}, finish_reason: 'length' }],
161+
usage: { prompt_tokens: 10, completion_tokens: 3000 },
162+
},
163+
];
164+
const p = new DeepSeekProvider({ apiKey: 'sk-test', fetch: mockFetch(chunks) });
165+
const result = await p.runTurn({
166+
model: 'deepseek-chat',
167+
systemPrompt: '',
168+
tools: [{ name: 'Write', description: '', inputSchema: { type: 'object', properties: {} } }],
169+
messages: [{ role: 'user', content: [{ type: 'text', text: 'write it' }] }],
170+
});
171+
// The malformed Write must NOT appear as an executable tool_use.
172+
expect(result.content.some((b) => b.type === 'tool_use')).toBe(false);
173+
// The partial text is preserved.
174+
expect(result.content.some((b) => b.type === 'text')).toBe(true);
175+
// The loop should stop, not try to execute the truncated call.
176+
expect(result.stopReason).toBe('max_tokens');
177+
});
178+
179+
it('keeps a valid call but drops a truncated sibling in the same turn', async () => {
180+
const chunks = [
181+
{
182+
choices: [
183+
{
184+
delta: {
185+
tool_calls: [
186+
{ index: 0, id: 'ok1', function: { name: 'Read', arguments: '{"file_path":"a.ts"}' } },
187+
{ index: 1, id: 'bad', function: { name: 'Write', arguments: '{"file_path":"b.ts","content":"x' } },
188+
],
189+
},
190+
},
191+
],
192+
},
193+
{ choices: [{ delta: {}, finish_reason: 'length' }], usage: { prompt_tokens: 5, completion_tokens: 8 } },
194+
];
195+
const p = new DeepSeekProvider({ apiKey: 'sk-test', fetch: mockFetch(chunks) });
196+
const result = await p.runTurn({
197+
model: 'deepseek-chat',
198+
systemPrompt: '',
199+
tools: [],
200+
messages: [{ role: 'user', content: [{ type: 'text', text: 'go' }] }],
201+
});
202+
const toolUses = result.content.filter((b) => b.type === 'tool_use');
203+
expect(toolUses).toHaveLength(1);
204+
expect(toolUses[0]?.type === 'tool_use' && toolUses[0].name).toBe('Read');
205+
// At least one valid call survived → still a tool_use turn.
206+
expect(result.stopReason).toBe('tool_use');
207+
});
142208
});
143209

144210
describe('DeepSeekProvider message conversion', () => {

0 commit comments

Comments
 (0)