feat: post-review backlog hardening (cancel/parser/stall/history/9502)
Five independent items from the post-review backlog. F1: Stop on an external agent task now aborts the running child via a per-task AbortController registry reachable from the cancel route, and finalizes the assistant message as cancelled (fixing two latent bugs — catch blocks left the message streaming, and warm success-paths wrote complete on an aborted turn); warm pools/worktrees are preserved and the native path is unchanged. F2/F3: prune the tool-call parser to its two load-bearing exports (unexport eight zero-caller symbols, add a gate test for the <invoke>-as-text fallback) and route placeholder-rejection logging through pino. F6: a 90s per-chunk stall-timeout wraps native inference's fullStream via AbortSignal.any so a hung stream finalizes the message instead of hanging — no retry (a pure classifyStreamError helper is added). F7: a read-only view_session_history MCP tool (newest-N, chronological). F9: retire the unused apps/coder/web :9502 fallback SPA, keeping every API/WS/health/MCP route. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,29 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { classifyStreamError } from '../inference/stream-error-classifier.js';
|
||||
|
||||
describe('classifyStreamError', () => {
|
||||
it("classifies AbortError as 'stall'", () => {
|
||||
const err = new Error('aborted');
|
||||
err.name = 'AbortError';
|
||||
expect(classifyStreamError(err)).toBe('stall');
|
||||
});
|
||||
|
||||
it("classifies a 503 HTTP error as 'transient'", () => {
|
||||
const err = Object.assign(new Error('Service Unavailable'), { status: 503 });
|
||||
expect(classifyStreamError(err)).toBe('transient');
|
||||
});
|
||||
|
||||
it("classifies a 500 HTTP error as 'transient'", () => {
|
||||
const err = Object.assign(new Error('Internal Server Error'), { status: 500 });
|
||||
expect(classifyStreamError(err)).toBe('transient');
|
||||
});
|
||||
|
||||
it("classifies a 4xx HTTP error as 'non-retryable'", () => {
|
||||
const err = Object.assign(new Error('Bad Request'), { status: 400 });
|
||||
expect(classifyStreamError(err)).toBe('non-retryable');
|
||||
});
|
||||
|
||||
it("classifies a generic Error as 'non-retryable'", () => {
|
||||
expect(classifyStreamError(new Error('something went wrong'))).toBe('non-retryable');
|
||||
});
|
||||
});
|
||||
153
apps/server/src/services/__tests__/stream-phase-adapter.test.ts
Normal file
153
apps/server/src/services/__tests__/stream-phase-adapter.test.ts
Normal file
@@ -0,0 +1,153 @@
|
||||
// Gate test: pins the <invoke>-as-text fallback in the stream-phase text-delta
|
||||
// path. This test will fail if extractToolCallBlocks is ever removed from the
|
||||
// text-delta branch of streamCompletion, which is the only guard for models
|
||||
// that emit tool calls as inline XML rather than structured tool_calls.
|
||||
import { describe, expect, it, vi, afterEach } from 'vitest';
|
||||
import type { FastifyBaseLogger } from 'fastify';
|
||||
|
||||
// vi.mock is hoisted before all module imports. Spread the original so all
|
||||
// other ai exports (tool, jsonSchema, types, …) remain real; only streamText
|
||||
// is replaced with a controllable spy.
|
||||
vi.mock('ai', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('ai')>();
|
||||
return { ...actual, streamText: vi.fn() };
|
||||
});
|
||||
|
||||
import { streamText } from 'ai';
|
||||
import { streamCompletion, STALL_TIMEOUT_MS } from '../inference/stream-phase-adapter.js';
|
||||
import type { StreamAdapterContext } from '../inference/stream-phase-adapter.js';
|
||||
|
||||
const INVOKE_BLOCK =
|
||||
'<invoke name="view_file"><parameter name="path">/tmp/test.ts</parameter></invoke>';
|
||||
|
||||
// One-shot async generator that yields a single text-delta carrying a complete
|
||||
// <invoke> block, simulating a model that emits its tool call as plain XML text.
|
||||
async function* makeInvokeTextDeltaStream() {
|
||||
yield { type: 'text-delta' as const, text: INVOKE_BLOCK };
|
||||
}
|
||||
|
||||
const fakeLog = {
|
||||
debug: vi.fn(),
|
||||
info: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
error: vi.fn(),
|
||||
fatal: vi.fn(),
|
||||
trace: vi.fn(),
|
||||
child: vi.fn(),
|
||||
} as unknown as FastifyBaseLogger;
|
||||
|
||||
const fakeCtx: StreamAdapterContext = {
|
||||
config: { LLAMA_SWAP_URL: 'http://localhost:11434' } as StreamAdapterContext['config'],
|
||||
log: fakeLog,
|
||||
};
|
||||
|
||||
describe('<invoke>-as-text fallback gate (stream-phase text-delta path)', () => {
|
||||
it('surfaces a plain-text <invoke> block as a toolCall and strips markup from content and deltas', async () => {
|
||||
vi.mocked(streamText).mockReturnValue({
|
||||
fullStream: makeInvokeTextDeltaStream(),
|
||||
usage: Promise.resolve({ inputTokens: 1, outputTokens: 1 }),
|
||||
} as unknown as ReturnType<typeof streamText>);
|
||||
|
||||
const deltas: string[] = [];
|
||||
const result = await streamCompletion(
|
||||
fakeCtx,
|
||||
'test-model',
|
||||
[{ role: 'user', content: 'call a tool' }],
|
||||
{ tools: null },
|
||||
(d) => deltas.push(d),
|
||||
undefined,
|
||||
);
|
||||
|
||||
// The <invoke> block must surface as a structured tool call
|
||||
expect(result.toolCalls).toHaveLength(1);
|
||||
expect(result.toolCalls[0]).toMatchObject({
|
||||
id: 'xml_call_0',
|
||||
name: 'view_file',
|
||||
args: { path: '/tmp/test.ts' },
|
||||
});
|
||||
|
||||
// The XML markup must not appear in the saved content or any flushed delta
|
||||
expect(result.content).not.toContain('<invoke');
|
||||
expect(result.content).not.toContain('</invoke>');
|
||||
expect(deltas.join('')).not.toContain('<invoke');
|
||||
});
|
||||
});
|
||||
|
||||
// T9: stall timeout — fake hanging stream fires AbortError after STALL_TIMEOUT_MS.
|
||||
describe('stall timeout (F6)', () => {
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
it(`aborts the stream after ${STALL_TIMEOUT_MS}ms with no chunks (stall path)`, async () => {
|
||||
vi.useFakeTimers();
|
||||
|
||||
// Capture the effectiveSignal the adapter passes to streamText so the fake
|
||||
// generator can unblock when the stall fires (matching real ReadableStream
|
||||
// abort behavior: the stream ends rather than throwing into the generator).
|
||||
let capturedSignal: AbortSignal | undefined;
|
||||
vi.mocked(streamText).mockImplementation((opts: Parameters<typeof streamText>[0]) => {
|
||||
capturedSignal = opts.abortSignal as AbortSignal | undefined;
|
||||
return {
|
||||
// Hang until the effective signal fires, then return without emitting
|
||||
// any parts — mirrors how a real fetch stream ends when aborted.
|
||||
fullStream: (async function* () {
|
||||
await new Promise<void>((resolve) => {
|
||||
if (capturedSignal?.aborted) {
|
||||
resolve();
|
||||
return;
|
||||
}
|
||||
capturedSignal?.addEventListener('abort', () => resolve(), { once: true });
|
||||
});
|
||||
})(),
|
||||
// Never resolves; the stall throw happens before usage is awaited.
|
||||
usage: new Promise<never>(() => {}),
|
||||
} as unknown as ReturnType<typeof streamText>;
|
||||
});
|
||||
|
||||
const streamPromise = streamCompletion(
|
||||
fakeCtx,
|
||||
'test-model',
|
||||
[{ role: 'user', content: 'hang' }],
|
||||
{ tools: null },
|
||||
() => {},
|
||||
undefined,
|
||||
);
|
||||
|
||||
// Attach the rejection handler BEFORE advancing timers so the rejection is
|
||||
// never unhandled (Node emits PromiseRejectionHandledWarning otherwise).
|
||||
const assertion = expect(streamPromise).rejects.toMatchObject({ name: 'AbortError' });
|
||||
|
||||
// Advance past the stall deadline — the stallAc fires, the hanging generator
|
||||
// resolves, the post-loop check sees stallAc.signal.aborted and throws.
|
||||
await vi.advanceTimersByTimeAsync(STALL_TIMEOUT_MS);
|
||||
|
||||
await assertion;
|
||||
});
|
||||
|
||||
// T10: regression pin — the original post-loop signal check for user-initiated
|
||||
// abort must still fire correctly after the stall logic was added.
|
||||
it('throws AbortError when the inbound signal is aborted (user-abort regression pin)', async () => {
|
||||
const ac = new AbortController();
|
||||
ac.abort();
|
||||
|
||||
vi.mocked(streamText).mockReturnValue({
|
||||
fullStream: (async function* () {
|
||||
// Yield nothing — stream ends immediately after user abort is already set
|
||||
})(),
|
||||
usage: Promise.resolve({ inputTokens: 0, outputTokens: 0 }),
|
||||
} as unknown as ReturnType<typeof streamText>);
|
||||
|
||||
await expect(
|
||||
streamCompletion(
|
||||
fakeCtx,
|
||||
'test-model',
|
||||
[{ role: 'user', content: 'aborted' }],
|
||||
{ tools: null },
|
||||
() => {},
|
||||
undefined,
|
||||
ac.signal,
|
||||
),
|
||||
).rejects.toMatchObject({ name: 'AbortError' });
|
||||
});
|
||||
});
|
||||
@@ -1,179 +1,9 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import {
|
||||
parseXmlToolCall,
|
||||
parseInvokeToolCall,
|
||||
partialXmlOpenerStart,
|
||||
extractToolCallBlocks,
|
||||
stripToolMarkup,
|
||||
XML_TOOL_OPEN,
|
||||
XML_TOOL_CLOSE,
|
||||
INVOKE_TOOL_OPEN,
|
||||
INVOKE_TOOL_CLOSE,
|
||||
} from '../inference/tool-call-parser.js';
|
||||
|
||||
// ── Ported from xml-parser.test.ts ───────────────────────────────────────
|
||||
|
||||
describe('parseXmlToolCall (Qwen/Hermes <tool_call>)', () => {
|
||||
it('parses a well-formed single-parameter call', () => {
|
||||
const block = '<tool_call><function=view_file><parameter=path>/tmp/foo</parameter></function></tool_call>';
|
||||
expect(parseXmlToolCall(block)).toEqual({
|
||||
name: 'view_file',
|
||||
args: { path: '/tmp/foo' },
|
||||
});
|
||||
});
|
||||
|
||||
it('parses multi-parameter call', () => {
|
||||
const block = '<tool_call><function=grep><parameter=pattern>foo</parameter><parameter=path>src/</parameter></function></tool_call>';
|
||||
expect(parseXmlToolCall(block)).toEqual({
|
||||
name: 'grep',
|
||||
args: { pattern: 'foo', path: 'src/' },
|
||||
});
|
||||
});
|
||||
|
||||
it('JSON-parses numeric parameter values', () => {
|
||||
const block = '<tool_call><function=foo><parameter=count>42</parameter></function></tool_call>';
|
||||
expect(parseXmlToolCall(block)).toEqual({ name: 'foo', args: { count: 42 } });
|
||||
});
|
||||
|
||||
it('tolerates whitespace around = in function (v1.13.16 tightening)', () => {
|
||||
const block = '<tool_call><function = view_file><parameter=path>/tmp/foo</parameter></function></tool_call>';
|
||||
expect(parseXmlToolCall(block)).toEqual({
|
||||
name: 'view_file',
|
||||
args: { path: '/tmp/foo' },
|
||||
});
|
||||
});
|
||||
|
||||
it('tolerates whitespace around = in parameter (v1.13.16 tightening)', () => {
|
||||
const block = '<tool_call><function=view_file><parameter = path>/tmp/foo</parameter></function></tool_call>';
|
||||
expect(parseXmlToolCall(block)).toEqual({
|
||||
name: 'view_file',
|
||||
args: { path: '/tmp/foo' },
|
||||
});
|
||||
});
|
||||
|
||||
it('returns null when function name is missing', () => {
|
||||
const block = '<tool_call><parameter=path>/tmp/foo</parameter></tool_call>';
|
||||
expect(parseXmlToolCall(block)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('parseInvokeToolCall (Anthropic <invoke>) — v1.13.16', () => {
|
||||
it('parses a well-formed single-parameter call (spec case 1)', () => {
|
||||
const block = '<invoke name="view_file"><parameter name="path">/tmp/foo</parameter></invoke>';
|
||||
expect(parseInvokeToolCall(block)).toEqual({
|
||||
name: 'view_file',
|
||||
args: { path: '/tmp/foo' },
|
||||
});
|
||||
});
|
||||
|
||||
it('parses a multi-parameter call (spec case 2)', () => {
|
||||
const block = '<invoke name="grep"><parameter name="pattern">foo</parameter><parameter name="path">src/</parameter></invoke>';
|
||||
expect(parseInvokeToolCall(block)).toEqual({
|
||||
name: 'grep',
|
||||
args: { pattern: 'foo', path: 'src/' },
|
||||
});
|
||||
});
|
||||
|
||||
it('tolerates newlines and spaces in attributes (spec case 3)', () => {
|
||||
const block = `<invoke
|
||||
name="view_file"
|
||||
>
|
||||
<parameter
|
||||
name="path"
|
||||
>/tmp/foo</parameter>
|
||||
</invoke>`;
|
||||
expect(parseInvokeToolCall(block)).toEqual({
|
||||
name: 'view_file',
|
||||
args: { path: '/tmp/foo' },
|
||||
});
|
||||
});
|
||||
|
||||
it('parses a call whose name is not a registered BooCode tool (spec case 4)', () => {
|
||||
const block = '<invoke name="read_file"><parameter name="path">/tmp/foo</parameter></invoke>';
|
||||
expect(parseInvokeToolCall(block)).toEqual({
|
||||
name: 'read_file',
|
||||
args: { path: '/tmp/foo' },
|
||||
});
|
||||
});
|
||||
|
||||
it('supports single-quoted attribute values', () => {
|
||||
const block = "<invoke name='view_file'><parameter name='path'>/tmp/foo</parameter></invoke>";
|
||||
expect(parseInvokeToolCall(block)).toEqual({
|
||||
name: 'view_file',
|
||||
args: { path: '/tmp/foo' },
|
||||
});
|
||||
});
|
||||
|
||||
it('JSON-parses numeric parameter values', () => {
|
||||
const block = '<invoke name="foo"><parameter name="count">42</parameter></invoke>';
|
||||
expect(parseInvokeToolCall(block)).toEqual({ name: 'foo', args: { count: 42 } });
|
||||
});
|
||||
|
||||
it('tolerates spaces around = inside name attribute', () => {
|
||||
const block = '<invoke name = "view_file"><parameter name = "path">/tmp/foo</parameter></invoke>';
|
||||
expect(parseInvokeToolCall(block)).toEqual({
|
||||
name: 'view_file',
|
||||
args: { path: '/tmp/foo' },
|
||||
});
|
||||
});
|
||||
|
||||
it('returns null when name attribute is missing', () => {
|
||||
const block = '<invoke><parameter name="path">/tmp/foo</parameter></invoke>';
|
||||
expect(parseInvokeToolCall(block)).toBeNull();
|
||||
});
|
||||
|
||||
it('returns null when name attribute is empty', () => {
|
||||
const block = '<invoke name=""><parameter name="path">/tmp/foo</parameter></invoke>';
|
||||
expect(parseInvokeToolCall(block)).toBeNull();
|
||||
});
|
||||
|
||||
it('exports the expected delimiters', () => {
|
||||
expect(INVOKE_TOOL_OPEN).toBe('<invoke');
|
||||
expect(INVOKE_TOOL_CLOSE).toBe('</invoke>');
|
||||
expect(XML_TOOL_OPEN).toBe('<tool_call>');
|
||||
expect(XML_TOOL_CLOSE).toBe('</tool_call>');
|
||||
});
|
||||
});
|
||||
|
||||
describe('partialXmlOpenerStart (v1.13.16 — both flavors)', () => {
|
||||
it('returns -1 when the buffer is empty', () => {
|
||||
expect(partialXmlOpenerStart('')).toBe(-1);
|
||||
});
|
||||
|
||||
it('returns -1 when the buffer has no openers', () => {
|
||||
expect(partialXmlOpenerStart('plain prose, no markup')).toBe(-1);
|
||||
});
|
||||
|
||||
it('returns the index of a complete <tool_call> opener (existing)', () => {
|
||||
expect(partialXmlOpenerStart('prose <tool_call>more')).toBe(6);
|
||||
});
|
||||
|
||||
it('returns the index of a complete <invoke opener (v1.13.16)', () => {
|
||||
expect(partialXmlOpenerStart('prose <invoke name=')).toBe(6);
|
||||
});
|
||||
|
||||
it('holds a partial <tool_ prefix at end of buffer', () => {
|
||||
expect(partialXmlOpenerStart('text <tool_')).toBe(5);
|
||||
});
|
||||
|
||||
it('holds a partial <invo prefix at end of buffer (v1.13.16)', () => {
|
||||
expect(partialXmlOpenerStart('text <invo')).toBe(5);
|
||||
});
|
||||
|
||||
it('holds a bare < at end of buffer', () => {
|
||||
expect(partialXmlOpenerStart('text <')).toBe(5);
|
||||
});
|
||||
|
||||
it('returns -1 when < is followed by non-opener text', () => {
|
||||
expect(partialXmlOpenerStart('text <unknown>')).toBe(-1);
|
||||
});
|
||||
|
||||
it('returns the earliest opener when both flavors are present', () => {
|
||||
expect(partialXmlOpenerStart('xxx <tool_call>YYY <invoke>')).toBe(4);
|
||||
expect(partialXmlOpenerStart('xxx <invoke>YYY <tool_call>')).toBe(4);
|
||||
});
|
||||
});
|
||||
|
||||
describe('extractToolCallBlocks (v1.13.16 — unified extraction)', () => {
|
||||
it('extracts a single <invoke> block (spec case 1)', () => {
|
||||
const input = '<invoke name="view_file"><parameter name="path">/tmp/foo</parameter></invoke>';
|
||||
@@ -341,11 +171,3 @@ describe('stripToolMarkup', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('delimiter constants', () => {
|
||||
it('exports the expected delimiters', () => {
|
||||
expect(INVOKE_TOOL_OPEN).toBe('<invoke');
|
||||
expect(INVOKE_TOOL_CLOSE).toBe('</invoke>');
|
||||
expect(XML_TOOL_OPEN).toBe('<tool_call>');
|
||||
expect(XML_TOOL_CLOSE).toBe('</tool_call>');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user