From 6fdd04812387ea8e53ebaaf453bb7cd302efb05a Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:25:47 +0800 Subject: [PATCH 01/16] fix(agent-loop): harden lifecycle edge cases --- packages/agent-loop/src/agent.ts | 20 +++++--- packages/agent-loop/src/loop.ts | 26 +++++++--- packages/agent-loop/tests/agent.spec.ts | 65 ++++++++++++++++++++++++ packages/agent-loop/tests/loop.spec.ts | 66 ++++++++++++++++++++++++- packages/agent/src/types.ts | 11 +++-- 5 files changed, 167 insertions(+), 21 deletions(-) diff --git a/packages/agent-loop/src/agent.ts b/packages/agent-loop/src/agent.ts index 8040e1b107..96a1158c66 100644 --- a/packages/agent-loop/src/agent.ts +++ b/packages/agent-loop/src/agent.ts @@ -63,7 +63,11 @@ export class LoopAgent implements Agent { // waiter (AGENTS.md "contain callback exceptions" — a lifecycle await must // not hang on one bad listener). if (status !== 'running') this.settleIdleWaiters() - this.ctx.emit('agent/status', this, status) + try { + this.ctx.emit('agent/status', this, status) + } catch (error: unknown) { + this.ctx.logger.warn(`agent "${this.id}": agent/status listener threw on ${status}: ${String(error)}`) + } } /** @@ -177,16 +181,16 @@ export class LoopAgent implements Agent { * `running`. If it is already disposed, awaits {@link done} (the loop-exit * promise) — `agent/status('disposed')` fires in the disposer BEFORE the * driver loop has unwound, so it is NOT itself a quiescence signal. If it is - * idle, resolves immediately. Otherwise queues an internal waiter (see - * {@link idleWaiters}) released on the next running→idle/disposed transition, - * resolving on `idle` directly (the turn fully ended) or chaining {@link done} - * on `disposed` (wait for the loop to actually exit). Implements the - * {@link Agent.whenIdle} contract used by teardown (`abort()` then - * `await whenIdle()`). + * idle AND has no queued work, resolves immediately. Otherwise queues an + * internal waiter (see {@link idleWaiters}) released on the next + * running→idle/disposed transition, resolving on `idle` directly (the turn + * fully ended) or chaining {@link done} on `disposed` (wait for the loop to + * actually exit). Implements the {@link Agent.whenIdle} contract used by + * teardown (`abort()` then `await whenIdle()`). */ whenIdle(): Promise { if (this._status === 'disposed') return this.done - if (this._status !== 'running') return Promise.resolve() + if (this._status !== 'running' && !this.inbox.hasQueued) return Promise.resolve() // Register an internal waiter (resolved by settleIdleWaiters on the next // running→idle/disposed transition), NOT an effect-scoped `ctx.on` listener: // a concurrent fiber disposal runs this agent's listener disposers, which diff --git a/packages/agent-loop/src/loop.ts b/packages/agent-loop/src/loop.ts index 74ccb0f71e..a9b3fbd2c5 100644 --- a/packages/agent-loop/src/loop.ts +++ b/packages/agent-loop/src/loop.ts @@ -203,8 +203,8 @@ async function runTurn(ctx: Context, agent: LoopAgent, handle: LoopHandle, turn: // agent/step-end emit is contained: a throwing step-end listener must not // abort finalization and strand the turn open (turn/end balance > notifying // one bad listener). Appended before the emit (ADR 0003 append-before-emit). - const closeStep = (): void => { - if (!stepOpen) return + const closeStep = (): boolean => { + if (!stepOpen) return false stepOpen = false // Session.append pushes step/end BEFORE notifying session/event listeners, // so a throwing listener leaves step/end in the log (balance holds) but @@ -227,6 +227,7 @@ async function runTurn(ctx: Context, agent: LoopAgent, handle: LoopHandle, turn: // itself succeeded, AND keeps finalization going when closeStep runs from // the outer catch. if (failure !== undefined) failTurn(toError(failure)) + return failure !== undefined } // Record a step/turn failure exactly once: append the single `error` event @@ -358,7 +359,7 @@ async function runTurn(ctx: Context, agent: LoopAgent, handle: LoopHandle, turn: // Steering that arrived during streaming/tool execution. const steered = drainSteering(ctx, agent, turn) - closeStep() + if (closeStep()) break const defaultDecision = stepOutcome.hadToolCalls || steered let shouldContinue: boolean @@ -502,16 +503,27 @@ async function runStep( // tool dispatch actually uses. let message: Message = assembler.message() message = await ctx.waterfall('agent/step-result', agent, turn, step, message, () => Promise.resolve(message)) + const finish = assembler.finish + const messageForLog: Message = finish.kind === 'max-tokens' + ? { ...message, content: message.content.filter(block => block.type !== 'tool-call') } + : message - session.append('assistant/message', { turn, step, content: message.content }) + if (finish.kind !== 'max-tokens' || messageForLog.content.length > 0) { + session.append('assistant/message', { turn, step, content: messageForLog.content }) + } if (assembler.usage) { session.append('usage', { turn, step, usage: assembler.usage }) } // --- Tool execution (sequential; parallel execution is a TODO) --- // ToolRegistry.execute converts tool failures (including aborts) into - // isError results, so abort is re-checked around every call here. - const toolCalls = message.content.filter(block => block.type === 'tool-call') + // isError results, so abort is re-checked around every call here. A + // max-tokens step is cut off: any tool-call block in it may be partial, so it + // is neither dispatched nor recorded in the derived-history assistant message + // above. Raw assistant/chunk events still preserve the exact stream. + const toolCalls = finish.kind === 'max-tokens' + ? [] + : message.content.filter(block => block.type === 'tool-call') for (const call of toolCalls) { /* v8 ignore next -- signal.reason always set by agent.abort() which provides a default */ if (signal.aborted) throw new Error(String(signal.reason ?? 'aborted')) @@ -552,7 +564,7 @@ async function runStep( /* v8 ignore stop */ } - return { hadToolCalls: toolCalls.length > 0, finish: assembler.finish } + return { hadToolCalls: toolCalls.length > 0, finish } } /** The last turn number in a (possibly seeded) session log, or 0. */ diff --git a/packages/agent-loop/tests/agent.spec.ts b/packages/agent-loop/tests/agent.spec.ts index 4e2cd6c046..4fd7237061 100644 --- a/packages/agent-loop/tests/agent.spec.ts +++ b/packages/agent-loop/tests/agent.spec.ts @@ -32,6 +32,17 @@ function waitForIdle(ctx: Context, agent: LoopAgent): Promise { }) } +function waitForStatus(ctx: Context, agent: LoopAgent, expected: LoopAgent['status']): Promise { + return new Promise((resolve) => { + const dispose = ctx.on('agent/status', (subject, status) => { + if (subject === agent && status === expected) { + dispose() + resolve() + } + }) + }) +} + function send(agent: LoopAgent, text: string) { agent.send([{ type: 'text', text }]) } @@ -265,6 +276,24 @@ describe('LoopAgent', () => { expect(agent.status).not.toBe('running') }) + it('whenIdle() waits for queued work that has not flipped status yet', async () => { + const adapter = new MockAdapter(['hang']) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + + send(agent, 'queued') + let settled = false + const idle = agent.whenIdle().then(() => { settled = true }) + await Promise.resolve() + expect(settled).toBe(false) + + await waitForStatus(ctx, agent, 'running') + agent.abort('done') + await idle + expect(settled).toBe(true) + expect(agent.status).toBe('idle') + }) + it('whenIdle() awaits the running→idle transition, ignoring other subjects/running events', async () => { const adapter = new MockAdapter([textResponse('ok'), textResponse('ok')]) const ctx = await harness(adapter) @@ -366,6 +395,42 @@ describe('LoopAgent', () => { expect(doneResolved).toBe(true) }) + it('contains a throwing agent/status listener on the running transition', async () => { + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(adapter) + const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + ctx.on('agent/status', (_subject, status) => { + if (status === 'running') throw new Error('bad running listener') + }) + + send(agent, 'go') + await agent.whenIdle() + + expect(adapter.requests).toHaveLength(1) + expect(agent.status).toBe('idle') + expect(warn).toHaveBeenCalledWith(expect.stringContaining('agent/status listener threw on running')) + warn.mockRestore() + }) + + it('contains a throwing agent/status listener on the idle transition', async () => { + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(adapter) + const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + ctx.on('agent/status', (_subject, status) => { + if (status === 'idle') throw new Error('bad idle listener') + }) + + send(agent, 'go') + await agent.whenIdle() + + expect(adapter.requests).toHaveLength(1) + expect(agent.status).toBe('idle') + expect(warn).toHaveBeenCalledWith(expect.stringContaining('agent/status listener threw on idle')) + warn.mockRestore() + }) + it('abort() resolves reason to "aborted" when no reason provided', async () => { const adapter = new MockAdapter(['hang']) const ctx = await harness(adapter) diff --git a/packages/agent-loop/tests/loop.spec.ts b/packages/agent-loop/tests/loop.spec.ts index 4d8cdefe6c..3f3f52c48e 100644 --- a/packages/agent-loop/tests/loop.spec.ts +++ b/packages/agent-loop/tests/loop.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' import { Context } from 'cordis' -import LlmService, { StreamChunk } from '@deepseek-ai/dsh-llm' +import LlmService, { CallId, StreamChunk } from '@deepseek-ai/dsh-llm' import SessionStore, { TurnEndReason } from '@deepseek-ai/dsh-session' import SystemPrompt from '@deepseek-ai/dsh-system-prompt' import ToolRegistry, { defineTool } from '@deepseek-ai/dsh-tools' @@ -406,6 +406,70 @@ describe('agent loop', () => { expect(reasons).toEqual([{ kind: 'max-tokens' }, { kind: 'completed' }]) }) + it('does not dispatch tool calls from a max-tokens-truncated step', async () => { + const callId = CallId('c1') + const adapter = new MockAdapter([[ + { type: 'block-start', index: 0, blockType: 'tool-call' }, + { type: 'tool-call-delta', index: 0, id: callId, name: 'echo', argumentsDelta: '{"text":"x"}' }, + { type: 'block-end', index: 0, block: { type: 'tool-call', id: callId, name: 'echo', arguments: '{"text":"x"}' } }, + { type: 'usage', usage: { inputTokens: 10, outputTokens: 5 } }, + { type: 'finish', reason: { kind: 'max-tokens' } }, + ]]) + const ctx = await harness(adapter) + let executions = 0 + ctx.tools.register(defineTool({ + name: 'echo', + description: '', + parameters: { text: { type: 'string' } }, + async execute() { + executions += 1 + return [{ type: 'text', text: 'should not run' }] + }, + })) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + + const reasons: TurnEndReason[] = [] + ctx.on('agent/turn-end', (_agent, _turn, reason) => void reasons.push(reason)) + + send(agent, 'go') + await waitForIdle(ctx, agent) + + expect(executions).toBe(0) + expect(agent.session.events.some(e => e.type === 'tool/call')).toBe(false) + expect(agent.session.deriveMessages()).toEqual([ + { role: 'user', content: [{ type: 'text', text: 'go' }] }, + ]) + expect(reasons).toEqual([{ kind: 'max-tokens' }]) + }) + + it('stops the turn when agent/step-end listener failure has recorded an error', async () => { + const adapter = new MockAdapter([ + toolCallResponse('c1', 'echo', { text: 'x' }), + textResponse('should not run'), + ]) + const ctx = await harness(adapter) + ctx.tools.register(defineTool({ + name: 'echo', + description: '', + parameters: { text: { type: 'string' } }, + async execute(args) { + return [{ type: 'text', text: String(args.text) }] + }, + })) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + let threw = false + ctx.on('agent/step-end', () => { + if (!threw) { threw = true; throw new Error('bad step-end listener') } + }) + + send(agent, 'go') + await waitForIdle(ctx, agent) + + expect(adapter.requests).toHaveLength(1) + const turnEnd = agent.session.events.findLast(e => e.type === 'turn/end') + expect(turnEnd?.type === 'turn/end' && turnEnd.data.reason.kind).toBe('error') + }) + it('chains queued messages into consecutive turns', async () => { const adapter = new MockAdapter([textResponse('first'), textResponse('second')]) const ctx = await harness(adapter) diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts index 0b786df593..9040c45ac5 100644 --- a/packages/agent/src/types.ts +++ b/packages/agent/src/types.ts @@ -82,11 +82,12 @@ export interface Agent { /** * Resolve once the agent has reached quiescence after settling out of - * `running`, or immediately if it is already idle. The quiescence signal a - * teardown awaits: `agent.abort()` then `await agent.whenIdle()` guarantees - * the in-flight turn has fully stopped before the caller proceeds (a closing - * ACP connection, a disposing UI plugin), rather than returning while the - * driver is still streaming. + * `running`, or immediately if it is already idle with no queued work. The + * quiescence signal a teardown awaits: `agent.abort()` then + * `await agent.whenIdle()` guarantees queued/running work has fully stopped + * before the caller proceeds (a closing ACP connection, a disposing UI + * plugin), rather than returning while the driver is still streaming or about + * to start a queued turn. * * "Quiescence", not merely "status changed": a disposed agent emits * `agent/status('disposed')` from inside its disposer, BEFORE the driver loop From 922e2f913ef18236543b049f533e76bb0d867ebf Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:25:56 +0800 Subject: [PATCH 02/16] fix(llm-pi-ai): preserve harness adapter contract --- packages/llm-deepseek/tests/adapter.e2e.ts | 8 ++- packages/llm-pi-ai/README.md | 4 +- packages/llm-pi-ai/src/adapter.ts | 78 ++++++++++++++++++---- packages/llm-pi-ai/src/convert.ts | 21 ++++-- packages/llm-pi-ai/tests/adapter.e2e.ts | 9 ++- packages/llm-pi-ai/tests/adapter.spec.ts | 57 ++++++++++++++-- packages/llm-pi-ai/tests/convert.spec.ts | 14 ++++ 7 files changed, 164 insertions(+), 27 deletions(-) diff --git a/packages/llm-deepseek/tests/adapter.e2e.ts b/packages/llm-deepseek/tests/adapter.e2e.ts index f37484fd3c..ebec6e62ce 100644 --- a/packages/llm-deepseek/tests/adapter.e2e.ts +++ b/packages/llm-deepseek/tests/adapter.e2e.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it } from 'vitest' import { Context } from 'cordis' import LlmService, { CallId } from '@deepseek-ai/dsh-llm' import type { GenerateResult, Message, ToolSchema } from '@deepseek-ai/dsh-llm' @@ -13,14 +13,20 @@ import type { Config } from '@deepseek-ai/dsh-llm-deepseek' const FLASH = 'deepseek-v4-flash' const PRO = 'deepseek-v4-pro' +const contexts: Context[] = [] async function harness(model: string, config: Partial = {}) { const ctx = new Context() + contexts.push(ctx) await ctx.plugin(LlmService) await ctx.plugin(LlmDeepSeek, { models: [model], ...config }) return ctx } +afterEach(async () => { + await Promise.all(contexts.splice(0).map(ctx => ctx.fiber.dispose())) +}) + function ask(text: string): Message[] { return [{ role: 'user', content: [{ type: 'text', text }] }] } diff --git a/packages/llm-pi-ai/README.md b/packages/llm-pi-ai/README.md index ae9dc93301..70f61fdb62 100644 --- a/packages/llm-pi-ai/README.md +++ b/packages/llm-pi-ai/README.md @@ -6,10 +6,10 @@ DeepSeek adapter for the harness LLM seam backed by [`@earendil-works/pi-ai`](ht `@deepseek-ai/dsh-llm-deepseek` already talks to the same endpoint. This package is its **design-verification twin**: same models, same wire protocol, completely different internals — a unified LLM library with its own event vocabulary versus hand-rolled fetch/SSE. Anything the harness `StreamChunk` protocol cannot express for BOTH implementations is a core-vocabulary bug. The differences it exercised on purpose: -- pi-ai hands back tool-call `arguments` as **parsed objects**; the harness keeps raw JSON strings (re-stringified at `block-end`). +- pi-ai hands tool-call `arguments` around as **parsed objects**; the harness keeps raw JSON strings. The adapter patches replay payloads back to the original raw strings before sending them, and re-stringifies parsed output tool calls at `block-end`. - pi-ai reports failures as **in-stream error events** (it never throws mid-stream); these map to `finish {kind:'error'|'aborted'}` chunks — the protocol's other sanctioned error path besides throwing (which llm-deepseek uses). - pi-ai folds reasoning tokens into `usage.output`; there is no separate reasoning count to map. -- pi-ai's options omit stop sequences; `GenerateOptions.stop` is injected via its `onPayload` hook. +- pi-ai's options omit some DeepSeek/OpenAI-compatible details; the adapter uses its `onPayload` hook to preserve the harness contract (`stop`, per-tool `strict`, omitted reasoning effort, raw replayed tool arguments). ## Config diff --git a/packages/llm-pi-ai/src/adapter.ts b/packages/llm-pi-ai/src/adapter.ts index e9eac80295..1a05f6d65f 100644 --- a/packages/llm-pi-ai/src/adapter.ts +++ b/packages/llm-pi-ai/src/adapter.ts @@ -14,7 +14,7 @@ import { stream as piStream } from '@earendil-works/pi-ai' import type { Model } from '@earendil-works/pi-ai' import { LlmAdapter, LlmError } from '@deepseek-ai/dsh-llm' -import type { GenerateOptions, StreamChunk } from '@deepseek-ai/dsh-llm' +import type { GenerateOptions, StreamChunk, ToolSchema } from '@deepseek-ai/dsh-llm' import { toPiContext, toStreamChunks } from './convert.ts' /** Reasoning levels surfaced by this adapter (DeepSeek wire: high|max). */ @@ -59,12 +59,71 @@ export function buildModel(modelId: string, options: PiAiAdapterOptions): Model< } } +type Payload = { + tools?: { function?: { name?: unknown; strict?: unknown } }[] + messages?: { + role?: unknown + tool_calls?: { id?: unknown; function?: { arguments?: unknown } }[] + }[] + reasoning_effort?: unknown + stop?: unknown +} + +function rawToolArguments(options: GenerateOptions): Map { + const raw = new Map() + for (const message of options.messages) { + if (message.role !== 'assistant') continue + for (const block of message.content) { + if (block.type === 'tool-call') raw.set(block.id, block.arguments) + } + } + return raw +} + +function strictByToolName(tools: ToolSchema[] | undefined): Map { + return new Map((tools ?? []).map(tool => [tool.name, tool.strict])) +} + +function patchPayload(payload: unknown, options: GenerateOptions, reasoning: PiAiReasoning | undefined): unknown { + if (typeof payload !== 'object' || payload === null) return payload + const body = payload as Payload + + if (reasoning === undefined) { + delete body.reasoning_effort + } + if (options.stop !== undefined) { + body.stop = options.stop + } + + const strictByName = strictByToolName(options.tools) + for (const tool of body.tools ?? []) { + const name = tool.function?.name + if (typeof name !== 'string') continue + const strict = strictByName.get(name) + if (strict === undefined) delete tool.function?.strict + else if (tool.function !== undefined) tool.function.strict = strict + } + + const rawById = rawToolArguments(options) + for (const message of body.messages ?? []) { + if (message.role !== 'assistant') continue + for (const call of message.tool_calls ?? []) { + if (typeof call.id !== 'string') continue + const raw = rawById.get(call.id) + if (raw !== undefined && call.function !== undefined) call.function.arguments = raw + } + } + + return body +} + /** * pi-ai-backed adapter. One instance serves every registered model name. * * Implementation notes: - * - `GenerateOptions.stop` is injected via pi-ai's `onPayload` hook (its - * public options omit stop sequences). + * - `onPayload` patches provider payload details pi-ai cannot express directly: + * stop sequences, per-tool strict, omitted reasoning effort, and raw replayed + * tool-call arguments. * - `prefill` throws UNSUPPORTED (same contract as dsh-llm-deepseek). * - pi-ai reports request failures as in-stream error events; convert.ts * maps them to `finish {kind:'error'|'aborted'}` chunks rather than @@ -86,8 +145,9 @@ export class PiAiAdapter extends LlmAdapter { const model = buildModel(options.model, this.options) // Undefined config means "provider default" (DeepSeek: thinking ENABLED), // matching llm-deepseek's omission semantics. pi-ai derives the wire - // thinking toggle from whether reasoningEffort is passed, so undefined - // maps to 'high' here; only an explicit 'off' disables thinking. + // thinking toggle from whether reasoningEffort is passed, so undefined maps + // internally to 'high' to get `thinking: enabled`; patchPayload then removes + // `reasoning_effort` so the provider chooses its default effort. const reasoning = this.options.reasoning ?? 'high' // pi-ai's event stream has no iterator-return cancellation hook: if our @@ -106,13 +166,7 @@ export class PiAiAdapter extends LlmAdapter { ...options.maxTokens !== undefined ? { maxTokens: options.maxTokens } : {}, signal: controller.signal, ...reasoning !== 'off' ? { reasoningEffort: reasoning } : {}, - ...options.stop !== undefined ? { - // pi-ai's options omit stop sequences; inject them into the raw body. - onPayload: (payload: unknown) => { - (payload as Record).stop = options.stop - return payload - }, - } : {}, + onPayload: payload => patchPayload(payload, options, this.options.reasoning), maxRetries: 0, }) diff --git a/packages/llm-pi-ai/src/convert.ts b/packages/llm-pi-ai/src/convert.ts index 065a31b89e..4610ff01c9 100644 --- a/packages/llm-pi-ai/src/convert.ts +++ b/packages/llm-pi-ai/src/convert.ts @@ -7,7 +7,8 @@ * exists — an independent implementation stress-tests the StreamChunk * protocol): * - pi-ai tool-call `arguments` are PARSED OBJECTS; the harness keeps the - * raw JSON string. We parse on the way in and re-stringify on the way out. + * raw JSON string. We parse on the way into pi-ai, patch provider payloads + * back to the original raw string in the adapter, and re-stringify on output. * - pi-ai reports errors as in-stream `error` events (it never throws * mid-stream); the harness expresses those as `finish {kind:'error'}` / * `{kind:'aborted'}` chunks. @@ -17,7 +18,7 @@ * @module dsh-llm-pi-ai/convert */ -import { CallId } from '@deepseek-ai/dsh-llm' +import { CallId, LlmError } from '@deepseek-ai/dsh-llm' import type { FinishReason, GenerateOptions, Message, StreamChunk, TokenUsage } from '@deepseek-ai/dsh-llm' import type { AssistantMessage, @@ -168,6 +169,14 @@ export function mapUsage(usage: PiUsage): TokenUsage { } } +function classifyPiAiError(message: string): string { + if (/\b(?:401|403)\b/.test(message)) return 'AUTH' + if (/\b429\b|rate.?limit/i.test(message)) return 'RATE_LIMIT' + if (/\b400\b|invalid.?request/i.test(message)) return 'INVALID_REQUEST' + if (/\b5\d\d\b/.test(message)) return 'SERVER' + return 'PI_AI_ERROR' +} + /** Map a terminal pi-ai event to the harness finish reason. */ export function mapStopReason(message: AssistantMessage): FinishReason { switch (message.stopReason) { @@ -175,10 +184,9 @@ export function mapStopReason(message: AssistantMessage): FinishReason { case 'length': return { kind: 'max-tokens' } case 'toolUse': return { kind: 'tool-calls' } case 'aborted': return { kind: 'aborted' } - case 'error': return { - kind: 'error', - message: message.errorMessage ?? 'pi-ai stream error', - code: 'PI_AI_ERROR', + case 'error': { + const text = message.errorMessage ?? 'pi-ai stream error' + return { kind: 'error', message: text, code: classifyPiAiError(text) } } } } @@ -264,4 +272,5 @@ export async function* toStreamChunks(events: AsyncIterable = {}) { const ctx = new Context() + contexts.push(ctx) await ctx.plugin(LlmService) await ctx.plugin(LlmPiAi, { models: [model], ...config }) return ctx } +afterEach(async () => { + await Promise.all(contexts.splice(0).map(ctx => ctx.fiber.dispose())) +}) + function ask(text: string): Message[] { return [{ role: 'user', content: [{ type: 'text', text }] }] } @@ -114,6 +120,7 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('llm-pi-ai e2e (real API)', () => // same block KINDS in the same order for a deterministic prompt — the // cross-implementation check that the StreamChunk design holds. const deepseekCtx = new Context() + contexts.push(deepseekCtx) await deepseekCtx.plugin(LlmService) await deepseekCtx.plugin(LlmDeepSeek, { models: [FLASH], thinking: 'disabled' }) diff --git a/packages/llm-pi-ai/tests/adapter.spec.ts b/packages/llm-pi-ai/tests/adapter.spec.ts index 6466084554..dfb258baa5 100644 --- a/packages/llm-pi-ai/tests/adapter.spec.ts +++ b/packages/llm-pi-ai/tests/adapter.spec.ts @@ -148,6 +148,44 @@ describe('PiAiAdapter against a mock server', () => { expect(server.requests[0]).toMatchObject({ stop: ['END'] }) }) + it('preserves per-tool strict exactly through onPayload', async () => { + const server = await mockServer([{ events: textEvents }]) + const ctx = await harness(server.url) + await ctx.llm.generate({ + model: 'deepseek-v4-flash', + messages: [], + tools: [ + { name: 'strict_true', description: 'true', parameters: {}, strict: true }, + { name: 'strict_false', description: 'false', parameters: {}, strict: false }, + { name: 'strict_omitted', description: 'omitted', parameters: {} }, + ], + }) + + const request = server.requests[0] as { tools: { function: { name: string; strict?: boolean } }[] } + expect(request.tools.map(tool => [tool.function.name, tool.function.strict])).toEqual([ + ['strict_true', true], + ['strict_false', false], + ['strict_omitted', undefined], + ]) + expect('strict' in request.tools[2]!.function).toBe(false) + }) + + it('preserves raw replayed tool-call arguments in the provider payload', async () => { + const server = await mockServer([{ events: textEvents }]) + const ctx = await harness(server.url) + await ctx.llm.generate({ + model: 'deepseek-v4-flash', + messages: [{ + role: 'assistant', + content: [{ type: 'tool-call', id: CallId('broken'), name: 'f', arguments: '{broken' }], + }], + }) + + const request = server.requests[0] as { messages: { role: string; tool_calls?: { id: string; function: { arguments: string } }[] }[] } + const assistant = request.messages.find(message => message.role === 'assistant') + expect(assistant?.tool_calls?.[0]?.function.arguments).toBe('{broken') + }) + it('maps HTTP errors to error finish chunks (pi-ai in-stream style)', async () => { const server = await mockServer([{ status: 401, @@ -155,10 +193,20 @@ describe('PiAiAdapter against a mock server', () => { }]) const ctx = await harness(server.url) const result = await ctx.llm.generate({ model: 'deepseek-v4-flash', messages: [] }) - expect(result.finish.kind).toBe('error') + expect(result.finish).toMatchObject({ kind: 'error', code: 'AUTH' }) expect((result.finish as { message: string }).message).toMatch(/bad key|401/) }) + it.each([ + [429, 'RATE_LIMIT'], + [500, 'SERVER'], + ] as const)('maps HTTP %s to stable error code %s', async (status, code) => { + const server = await mockServer([{ status, body: JSON.stringify({ error: { message: `provider ${status}` } }) }]) + const ctx = await harness(server.url) + const result = await ctx.llm.generate({ model: 'deepseek-v4-flash', messages: [] }) + expect(result.finish).toMatchObject({ kind: 'error', code }) + }) + it('rejects prefill with UNSUPPORTED', async () => { const ctx = await harness('http://127.0.0.1:1') await expect(ctx.llm.generate({ @@ -263,10 +311,9 @@ describe('review fixes', () => { const server = await mockServer([{ events: textEvents }]) const ctx = await harness(server.url) // no reasoning key at all await ctx.llm.generate({ model: 'deepseek-v4-flash', messages: [] }) - expect(server.requests[0]).toMatchObject({ - thinking: { type: 'enabled' }, - reasoning_effort: 'high', - }) + const request = server.requests[0] as Record + expect(request.thinking).toEqual({ type: 'enabled' }) + expect('reasoning_effort' in request).toBe(false) }) it('replays reasoning_content on assistant tool-call turns (passback rule)', async () => { diff --git a/packages/llm-pi-ai/tests/convert.spec.ts b/packages/llm-pi-ai/tests/convert.spec.ts index a4394b1e1b..be42c9e9b4 100644 --- a/packages/llm-pi-ai/tests/convert.spec.ts +++ b/packages/llm-pi-ai/tests/convert.spec.ts @@ -271,6 +271,11 @@ describe('toStreamChunks', () => { const chunks = await collect(toStreamChunks(feed({ type: 'error', reason: 'aborted', error }))) expect(chunks.at(-1)).toEqual({ type: 'finish', reason: { kind: 'aborted' } }) }) + + it('rejects a stream that ends without done or error', async () => { + await expect(collect(toStreamChunks(feed({ type: 'start', partial: assistant() })))) + .rejects.toThrow(/without done\/error/) + }) }) describe('mapStopReason / mapUsage', () => { @@ -288,6 +293,15 @@ describe('mapStopReason / mapUsage', () => { .toEqual({ kind: 'error', message: 'pi-ai stream error', code: 'PI_AI_ERROR' }) }) + it('maps routable HTTP-ish error messages to stable codes', () => { + expect(mapStopReason(assistant({ stopReason: 'error', errorMessage: 'HTTP 401: bad key' }))) + .toMatchObject({ kind: 'error', code: 'AUTH' }) + expect(mapStopReason(assistant({ stopReason: 'error', errorMessage: 'HTTP 429: rate limit' }))) + .toMatchObject({ kind: 'error', code: 'RATE_LIMIT' }) + expect(mapStopReason(assistant({ stopReason: 'error', errorMessage: 'HTTP 500: backend down' }))) + .toMatchObject({ kind: 'error', code: 'SERVER' }) + }) + it('maps cache fields only when nonzero', () => { expect(mapUsage(usage(10, 5, 8, 2))).toEqual({ inputTokens: 10, From 87df09e3c3766edaa1ceb70c049d46c9cf0e8e7d Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:26:12 +0800 Subject: [PATCH 03/16] fix(tools): isolate tool schemas and waterfall failures --- packages/system-prompt/src/index.ts | 7 ++-- .../system-prompt/tests/system-prompt.spec.ts | 17 ++++++++++ packages/tools/src/index.ts | 13 +++++++- packages/tools/tests/tools.spec.ts | 32 +++++++++++++++++++ 4 files changed, 66 insertions(+), 3 deletions(-) diff --git a/packages/system-prompt/src/index.ts b/packages/system-prompt/src/index.ts index ec89a3460c..b2f3bd3db4 100644 --- a/packages/system-prompt/src/index.ts +++ b/packages/system-prompt/src/index.ts @@ -123,8 +123,11 @@ export class SystemPrompt extends Service { */ assemble(): Promise { const assembly: PromptAssembly = { - sections: [...this.sections].sort((a, b) => a.order - b.order), - tools: this.toolProviders.flatMap(provider => provider()), + sections: this.sections + .map(section => ({ ...section })) + .sort((a, b) => a.order - b.order), + tools: this.toolProviders.flatMap(provider => + provider().map(tool => ({ ...tool, parameters: structuredClone(tool.parameters) }))), } return this.ctx.waterfall(this, 'system-prompt/assemble', assembly, () => Promise.resolve(assembly)) } diff --git a/packages/system-prompt/tests/system-prompt.spec.ts b/packages/system-prompt/tests/system-prompt.spec.ts index 43e9240412..cfbf36cbec 100644 --- a/packages/system-prompt/tests/system-prompt.spec.ts +++ b/packages/system-prompt/tests/system-prompt.spec.ts @@ -107,6 +107,23 @@ describe('SystemPrompt', () => { expect(assembly.sections).toHaveLength(0) }) + it('assembles snapshots so one-step mutations do not leak into future assemblies', async () => { + const ctx = new Context() + await ctx.plugin(SystemPrompt) + ctx.systemPrompt.section({ name: 'base', order: 0, text: 'base' }) + ctx.systemPrompt.tools(() => [{ name: 't', description: 'tool', parameters: { type: 'object', properties: {} } }]) + + const first = await ctx.systemPrompt.assemble() + first.sections[0]!.name = 'mutated' + first.tools[0]!.description = 'mutated' + const firstParameters = first.tools[0]!.parameters as { properties: Record } + firstParameters.properties['leak'] = { type: 'string' } + + const second = await ctx.systemPrompt.assemble() + expect(second.sections.map(section => section.name)).toEqual(['base']) + expect(second.tools).toEqual([{ name: 't', description: 'tool', parameters: { type: 'object', properties: {} } }]) + }) + it('filters out empty section text from renderPrompt', () => { // Direct test of renderPrompt: function returning empty string, and empty static text const result = renderPrompt({ diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index 8e8c106147..b09647ffcd 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -173,7 +173,10 @@ export class ToolRegistry extends Service { schemas(): ToolSchema[] { // Rest-destructure to drop `execute`; the unused binding is the idiom. // eslint-disable-next-line @typescript-eslint/unbound-method, @typescript-eslint/no-unused-vars - return [...this.store.values()].map(({ execute, ...schema }) => schema) + return [...this.store.values()].map(({ execute, ...schema }) => ({ + ...schema, + parameters: structuredClone(schema.parameters), + })) } /** @@ -201,6 +204,14 @@ export class ToolRegistry extends Service { ...info ? { error: info } : {}, } } + }).catch((error: unknown): ToolExecutionResult => { + const info = errorInfo(error) + return { + callId: exec.callId, + content: [{ type: 'text', text: `Error: ${errorMessage(error)}` }], + isError: true, + ...info ? { error: info } : {}, + } }) } } diff --git a/packages/tools/tests/tools.spec.ts b/packages/tools/tests/tools.spec.ts index 7338a2c9c8..ac2fd8414c 100644 --- a/packages/tools/tests/tools.spec.ts +++ b/packages/tools/tests/tools.spec.ts @@ -122,6 +122,38 @@ describe('ToolRegistry', () => { expect(order).toEqual(['first:before', 'second:before', 'second:after', 'first:after']) }) + it('returns an isError result when a tools/execute listener throws', async () => { + const ctx = await setup() + ctx.tools.register(echoTool) + ctx.on('tools/execute', async () => { + throw new Error('permission hook broke') + }) + + const result = await ctx.tools.execute({ callId: CallId('c1'), name: 'echo', arguments: { text: 'hi' } }) + + expect(result).toEqual({ + callId: CallId('c1'), + content: [{ type: 'text', text: 'Error: permission hook broke' }], + isError: true, + }) + }) + + it('schemas() snapshots tool schemas instead of exposing registry objects', async () => { + const ctx = await setup() + ctx.tools.register(echoTool) + + const first = ctx.tools.schemas() + const firstParameters = first[0]!.parameters as { properties: Record } + firstParameters.properties['mutated'] = { type: 'string' } + first[0]!.description = 'mutated' + + expect(ctx.tools.schemas()).toEqual([{ + name: 'echo', + description: 'echo arguments back', + parameters: { type: 'object', properties: { text: { type: 'string' } } }, + }]) + }) + it('rejects duplicate names and unregisters on fiber dispose (HMR safety)', async () => { const ctx = await setup() ctx.tools.register(echoTool) From 2b36620e55c94bd49a66d2956a63c97254bdafb6 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:26:21 +0800 Subject: [PATCH 04/16] fix(persistence): tighten crash repair and dispose semantics --- packages/invariants/src/index.ts | 25 ++++++++++- packages/invariants/tests/invariants.spec.ts | 41 +++++++++++++++++ .../session-persistence-jsonl/src/index.ts | 37 +++++++++++----- .../tests/jsonl.spec.ts | 44 +++++++++++++------ packages/session-persistence-sqlite/README.md | 2 +- .../session-persistence-sqlite/src/index.ts | 29 +++++++++--- .../session-persistence/tests/contract.ts | 12 ++++- .../tests/persistence.spec.ts | 2 +- packages/session/src/index.ts | 2 +- packages/session/src/repair.ts | 7 +-- 10 files changed, 161 insertions(+), 40 deletions(-) diff --git a/packages/invariants/src/index.ts b/packages/invariants/src/index.ts index ebb4decfd8..fe46d9e9ee 100644 --- a/packages/invariants/src/index.ts +++ b/packages/invariants/src/index.ts @@ -57,6 +57,10 @@ interface SessionTrace { openTurn: number | null /** Open step within the current turn, or null between steps. */ openStep: number | null + /** The next turn number expected in this session log. */ + nextTurn: number + /** The next step number expected within the open turn. */ + nextStep: number /** * Tool-call ids issued in the OPEN step awaiting a result. Cleared at * `step/end` — a result must arrive in the same step as its call. @@ -114,7 +118,11 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { if (trace.openTurn !== null) { throw new InvariantError(`turn/start ${event.data.turn} while turn ${trace.openTurn} is still open`) } + if (event.data.turn !== trace.nextTurn) { + throw new InvariantError(`turn/start expected turn ${trace.nextTurn}, got ${event.data.turn}`) + } trace.openTurn = event.data.turn + trace.nextStep = 1 break } case 'turn/end': { @@ -125,6 +133,7 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { throw new InvariantError(`turn/end ${event.data.turn} while step ${trace.openStep} is still open`) } trace.openTurn = null + trace.nextTurn += 1 break } case 'step/start': { @@ -134,6 +143,9 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { if (trace.openStep !== null) { throw new InvariantError(`step/start ${event.data.step} while step ${trace.openStep} is still open`) } + if (event.data.step !== trace.nextStep) { + throw new InvariantError(`step/start expected step ${trace.nextStep} in turn ${event.data.turn}, got ${event.data.step}`) + } trace.openStep = event.data.step break } @@ -143,6 +155,7 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { // (a step that errored before its result) do not carry to the next step. trace.pendingCalls.clear() trace.openStep = null + trace.nextStep += 1 break } case 'assistant/chunk': { @@ -163,7 +176,8 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { // A result needs a prior matching call in the same step. (The converse // does NOT hold: a call may have no result — a throwing tools/execute // waterfall ends the step with no tool/result, which is legal.) - if (!trace.pendingCalls.delete(event.data.callId)) { + const syntheticInterrupted = event.data.isError && event.data.error?.code === 'interrupted' + if (!trace.pendingCalls.delete(event.data.callId) && !syntheticInterrupted) { throw new InvariantError(`tool/result for ${event.data.callId} with no prior tool/call in this step`) } break @@ -216,7 +230,14 @@ export function apply(ctx: Context, config: Config = {}): void { // (re-)apply seeds the baseline, so a reload never produces a false positive. const lastStatus = new WeakMap() - const freshTrace = (): SessionTrace => ({ lastSeq: -1, openTurn: null, openStep: null, pendingCalls: new Set() }) + const freshTrace = (): SessionTrace => ({ + lastSeq: -1, + openTurn: null, + openStep: null, + nextTurn: 1, + nextStep: 1, + pendingCalls: new Set(), + }) /** Build (or rebuild) a session's trace by replaying its whole log; freeze it. */ const seedSession = (session: Session): SessionTrace => { diff --git a/packages/invariants/tests/invariants.spec.ts b/packages/invariants/tests/invariants.spec.ts index b96d172c22..eb935f7329 100644 --- a/packages/invariants/tests/invariants.spec.ts +++ b/packages/invariants/tests/invariants.spec.ts @@ -126,6 +126,28 @@ describe('session-log invariants', () => { .toThrow(/no prior tool\/call/) }) + it('allows a synthetic interrupted tool/result from crash repair without a prior tool/call event', async () => { + const { ctx } = await setup({ freeze: false }) + const session = ctx.sessions.create() + expect(() => { + session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) + session.append('step/start', { turn: 1, step: 1 }) + session.append('assistant/message', { turn: 1, step: 1, content: [ + { type: 'tool-call', id: CallId('crashed'), name: 'bash', arguments: '{}' }, + ] }) + session.append('tool/result', { + turn: 1, + step: 1, + callId: CallId('crashed'), + content: [{ type: 'text', text: 'interrupted' }], + isError: true, + error: { name: 'InterruptedError', code: 'interrupted' }, + }) + session.append('step/end', { turn: 1, step: 1 }) + session.append('turn/end', { turn: 1, reason: { kind: 'interrupted' } }) + }).not.toThrow() + }) + it('allows a tool/call with no matching tool/result (thrown waterfall ends the step)', async () => { const { ctx } = await setup({ freeze: false }) const session = ctx.sessions.create() @@ -177,6 +199,25 @@ describe('session-log invariants', () => { }).not.toThrow() }) + it('rejects a skipped turn number', async () => { + const { ctx } = await setup({ freeze: false }) + const session = ctx.sessions.create() + session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) + session.append('turn/end', { turn: 1, reason: { kind: 'completed' } }) + expect(() => session.append('turn/start', { turn: 3, trigger: { kind: 'message', source: { kind: 'user' } } })) + .toThrow(/expected turn 2, got 3/) + }) + + it('rejects a skipped step number within a turn', async () => { + const { ctx } = await setup({ freeze: false }) + const session = ctx.sessions.create() + session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) + session.append('step/start', { turn: 1, step: 1 }) + session.append('step/end', { turn: 1, step: 1 }) + expect(() => session.append('step/start', { turn: 1, step: 3 })) + .toThrow(/expected step 2 in turn 1, got 3/) + }) + it('rejects a turn/end while a step is still open', async () => { const { ctx } = await setup({ freeze: false }) const session = ctx.sessions.create() diff --git a/packages/session-persistence-jsonl/src/index.ts b/packages/session-persistence-jsonl/src/index.ts index 3e5320c3a9..0fca2bfb4d 100644 --- a/packages/session-persistence-jsonl/src/index.ts +++ b/packages/session-persistence-jsonl/src/index.ts @@ -8,8 +8,7 @@ * line, verbatim including `assistant/chunk` so `seq` stays contiguous) plus * a small atomic `.summary.json` sidecar for the mutable `SessionSummary`. * Lazy materialization (no file until the first `append`), atomic first - * write, and truncation-repair of a never-committed crash tail on the first - * `append` after a `load`. + * write, and load-time repair of a never-committed crash tail. * * 2. **The write path** — the `session/event` → buffer → `session/flush` drain * that generalizes the example `session-jsonl.ts`: snapshot each event when @@ -24,7 +23,7 @@ import { Context } from 'cordis' import z from 'schemastery' import { open, mkdir, readFile, readdir, rename, link, rm, truncate } from 'node:fs/promises' -import { resolve } from 'node:path' +import { dirname, resolve } from 'node:path' import { randomBytes } from 'node:crypto' import { SessionPersistence } from '@deepseek-ai/dsh-session-persistence' import { isJsonValue, interruptedTurnClosers } from '@deepseek-ai/dsh-session' @@ -111,6 +110,15 @@ function isENOENT(error: unknown): boolean { return (error as NodeJS.ErrnoException | null)?.code === 'ENOENT' } +async function settledErrors(promises: Iterable>): Promise { + const settled = await Promise.allSettled([...promises]) + const errors: unknown[] = [] + for (const result of settled) { + if (result.status === 'rejected') errors.push(result.reason) + } + return errors +} + /** * The JSONL persistence backend. Load as a plugin; it registers as * `ctx.sessionPersistence` and installs the write-path listeners. @@ -397,8 +405,8 @@ export class SessionPersistenceJsonl extends SessionPersistence { // sidecar is best-effort) — but if we mutated state.meta first, a later // touchSummary() on a successful append would persist the rejected // title/firstPrompt, making a failed update durable after the fact. - const nextMeta: SessionMeta = { ...state.meta, ...summary } - await this.writeSidecar(nextMeta) + const nextMeta: SessionMeta = { ...state.meta, ...summary, updatedAt: summary.updatedAt ?? Date.now() } + if (state.materialized) await this.writeSidecar(nextMeta) state.meta = nextMeta } @@ -407,7 +415,10 @@ export class SessionPersistenceJsonl extends SessionPersistence { /** Atomically write the header line + first batch (temp-write, fsync, rename). */ private async materialize(state: SessionState, events: readonly SessionEvent[]): Promise { const dir = sessionDir(this.root, state.meta.cwd) + await mkdir(this.root, { recursive: true, mode: 0o700 }) + await this.syncDir(dirname(this.root)) await mkdir(dir, { recursive: true, mode: 0o700 }) + await this.syncDir(this.root) const finalPath = logPath(this.root, state.meta.cwd, state.meta.id) // Never rename over an existing committed log: materialize is the FIRST // write of a session the backend believes is new. A file here means a @@ -571,8 +582,9 @@ export class SessionPersistenceJsonl extends SessionPersistence { try { const raw = await readFile(sidecarPath(this.root, cwd, id), 'utf8') return JSON.parse(raw) as SessionSummary - } catch { - return undefined + } catch (error) { + if (isENOENT(error)) return undefined + throw error } } @@ -675,9 +687,14 @@ export class SessionPersistenceJsonl extends SessionPersistence { // Dispose must reach quiescence: await every session's init + final drain // BEFORE returning, so no write lands after teardown (orphan rename/ENOENT). ctx.effect(() => async () => { - await Promise.allSettled([...this.inits.values()]) - await Promise.allSettled([...this.buffers.keys()].map(s => this.flush(s))) - await Promise.allSettled([...this.chains.values()]) + const errors = [ + ...await settledErrors(this.inits.values()), + ...await settledErrors([...this.buffers.keys()].map(s => this.flush(s))), + ...await settledErrors(this.chains.values()), + ] + if (errors.length > 0) { + throw new AggregateError(errors, 'session-persistence-jsonl dispose failed') + } }, 'session-persistence-jsonl write path') // HMR: a hot reload does not replay session/created, so seed existing live diff --git a/packages/session-persistence-jsonl/tests/jsonl.spec.ts b/packages/session-persistence-jsonl/tests/jsonl.spec.ts index 563dd8c627..cc463f78f7 100644 --- a/packages/session-persistence-jsonl/tests/jsonl.spec.ts +++ b/packages/session-persistence-jsonl/tests/jsonl.spec.ts @@ -644,25 +644,40 @@ describe('SessionPersistenceJsonl: edge cases', () => { expect(loaded.meta.title).toBeUndefined() }) - it('delete removes the sidecar of a lazy session that has no log', async () => { - // update() before the first append() writes a .summary.json sidecar but no - // .jsonl log (lazy create). delete() must still remove that sidecar. - const m = meta('lazy-del', '/a') + it('update before the first append keeps summary in memory and writes no orphan sidecar', async () => { + const m = meta('lazy-update', '/a') await ctx.sessionPersistence.create(m) await ctx.sessionPersistence.update(m.id, { title: 'secret', firstPrompt: 'sensitive' }) const sidecar = sidecarPath(root, '/a', m.id) - expect((await stat(sidecar)).isFile()).toBe(true) // sidecar exists, no log - await expect(stat(logPath(root, '/a', m.id))).rejects.toThrow() // no log - await ctx.sessionPersistence.delete(m.id) - await expect(stat(sidecar)).rejects.toThrow() // sidecar gone + await expect(stat(sidecar)).rejects.toThrow() + await expect(stat(logPath(root, '/a', m.id))).rejects.toThrow() + + await ctx.sessionPersistence.append(m.id, oneTurnLog()) + const loaded = await ctx.sessionPersistence.load(m.id) + expect(loaded.meta.title).toBe('secret') + expect(loaded.meta.firstPrompt).toBe('sensitive') + expect((await stat(sidecar)).isFile()).toBe(true) }) - it('delete removes a cwd-bucket sidecar even after a restart loses the in-memory cwd', async () => { - // A lazy session writes a sidecar under cwd /a (no log). Restart the backend - // (fresh instance, empty state) and delete: the in-memory cwd is gone and - // there is no log to recover it from, so delete must scan every bucket for - // the sidecar rather than only the _no-cwd bucket. + it('a lazy update leaves no sidecar that can leak into a future same-id session after restart', async () => { + await ctx.sessionPersistence.create(meta('restart-lazy', '/a')) + await ctx.sessionPersistence.update(SessionId('restart-lazy'), { title: 'secret' }) + await expect(stat(sidecarPath(root, '/a', SessionId('restart-lazy')))).rejects.toThrow() + + const ctx2 = new Context() + await ctx2.plugin(SessionStore) + await ctx2.plugin(SessionPersistenceJsonl, { root }) + const m2 = meta('restart-lazy', '/a') + await ctx2.sessionPersistence.create(m2) + await ctx2.sessionPersistence.append(m2.id, oneTurnLog()) + const loaded = await ctx2.sessionPersistence.load(m2.id) + expect(loaded.meta.title).toBeUndefined() + await ctx2.fiber.dispose() + }) + + it('delete removes a materialized cwd-bucket sidecar after a restart', async () => { await ctx.sessionPersistence.create(meta('restart-del', '/a')) + await ctx.sessionPersistence.append(SessionId('restart-del'), oneTurnLog()) await ctx.sessionPersistence.update(SessionId('restart-del'), { title: 'secret' }) const sidecar = sidecarPath(root, '/a', SessionId('restart-del')) expect((await stat(sidecar)).isFile()).toBe(true) @@ -671,7 +686,8 @@ describe('SessionPersistenceJsonl: edge cases', () => { await ctx2.plugin(SessionStore) await ctx2.plugin(SessionPersistenceJsonl, { root }) await ctx2.sessionPersistence.delete(SessionId('restart-del')) - await expect(stat(sidecar)).rejects.toThrow() // sidecar gone despite no in-memory cwd + await expect(stat(sidecar)).rejects.toThrow() + await expect(stat(logPath(root, '/a', SessionId('restart-del')))).rejects.toThrow() await ctx2.fiber.dispose() }) diff --git a/packages/session-persistence-sqlite/README.md b/packages/session-persistence-sqlite/README.md index 3a7a3c8163..6f8ce212d0 100644 --- a/packages/session-persistence-sqlite/README.md +++ b/packages/session-persistence-sqlite/README.md @@ -14,7 +14,7 @@ The repo targets Node ≥ 24 (the root `engines` field), which includes the stab - **Append = a transaction.** `append` runs `BEGIN`/`COMMIT` around the batch: it materializes the `sessions` row (if still lazy) and INSERTs every event, asserting the contiguous-seq contract first (the first event's `seq` must equal the stored next-seq). A mid-batch failure (a UNIQUE violation on a duplicated seq) rolls back entirely, so the stored log and the in-memory cursor stay consistent. (`load()` already balanced the stored log, so `append` never has to repair a crash tail.) - **Lazy materialization.** `create()` records intent in memory only — no row is written until the first `append`. A created-but-never-appended session has no `sessions` row, so it is absent from `has()`/`list()` (which report exactly the sessions that have a row). -- **Interrupted-turn close on load.** `load()` reads every stored event ordered by `seq` and finds the longest seq-contiguous, parseable prefix — INCLUDING the real events of an interrupted final turn after the last `turn/end` (the loop only flushes at `turn/end`, so a process killed mid-turn leaves real, fully-written rows past it). A single turn can be huge in a long-horizon task, so those events are **preserved, never truncated**: `load()` CLOSES the orphaned turn by durably appending the minimal synthetic boundary events (a `step/end` if a step was open, then a `turn/end` carrying `{ kind: 'interrupted' }`), inside one transaction that also DELETEs any never-fully-written torn tail row. `load()` is therefore mutating — after it the stored rows are balanced and the cursor is truthful, so the next `append` continues cleanly. The boundary (last `turn/end`, torn-tail detection) is computed from the `seq`/`type` columns so a malformed `data` in a torn tail row is never parsed (discarded, not unloadable). A parse error or `seq` gap inside the committed region (at or before the last real `turn/end`) makes the session unloadable. A session whose only turn never closed keeps its metadata row and stays present in `has()`/`list()` — the same as the JSONL backend, whose file likewise survives a first append that never reached `turn/end`. +- **Interrupted-turn close on load.** `load()` reads every stored event ordered by `seq` and finds the longest seq-contiguous, parseable prefix — INCLUDING the real events of an interrupted final turn after the last `turn/end` (the loop only flushes at `turn/end`, so a process killed mid-turn leaves real, fully-written rows past it). A single turn can be huge in a long-horizon task, so those events are **preserved, never truncated**: `load()` CLOSES the orphaned turn by durably appending the minimal synthetic boundary events (an error `tool/result` for every assistant tool call left unanswered, a `step/end` if a step was open, then a `turn/end` carrying `{ kind: 'interrupted' }`), inside one transaction that also DELETEs any never-fully-written torn tail row. `load()` is therefore mutating — after it the stored rows are balanced and the cursor is truthful, so the next `append` continues cleanly. The boundary (last `turn/end`, torn-tail detection) is computed from the `seq`/`type` columns so a malformed `data` in a torn tail row is never parsed (discarded, not unloadable). A parse error or `seq` gap inside the committed region (at or before the last real `turn/end`) makes the session unloadable. A session whose only turn never closed keeps its metadata row and stays present in `has()`/`list()` — the same as the JSONL backend, whose file likewise survives a first append that never reached `turn/end`. ## Configuration (schemastery) diff --git a/packages/session-persistence-sqlite/src/index.ts b/packages/session-persistence-sqlite/src/index.ts index bd4b4d9435..cfd2c5ca60 100644 --- a/packages/session-persistence-sqlite/src/index.ts +++ b/packages/session-persistence-sqlite/src/index.ts @@ -80,6 +80,15 @@ function assertSerializable(events: readonly SessionEvent[]): void { } } +async function settledErrors(promises: Iterable>): Promise { + const settled = await Promise.allSettled([...promises]) + const errors: unknown[] = [] + for (const result of settled) { + if (result.status === 'rejected') errors.push(result.reason) + } + return errors +} + /** * The SQLite persistence backend. Load as a plugin; it registers as * `ctx.sessionPersistence` and installs the write-path listeners. @@ -314,7 +323,7 @@ export class SessionPersistenceSqlite extends SessionPersistence { await this.ready let state = this.states.get(id) if (state === undefined) state = await this.adopt(id) - const nextMeta: SessionMeta = { ...state.meta, ...summary } + const nextMeta: SessionMeta = { ...state.meta, ...summary, updatedAt: summary.updatedAt ?? Date.now() } // update's only durable effect is the summary fields; the event log is // untouched. If the row is not materialized yet (a lazy session updated // before its first append) there is nothing to write — keep the pending @@ -410,11 +419,19 @@ export class SessionPersistenceSqlite extends SessionPersistence { // Dispose must reach quiescence: await every init + final drain, then close // the database, BEFORE returning, so no write lands after teardown. ctx.effect(() => async () => { - await Promise.allSettled([...this.inits.values()]) - await Promise.allSettled([...this.buffers.keys()].map(s => this.flush(s))) - await Promise.allSettled([...this.chains.values()]) - await this.ready - this.db.close() + try { + const errors = [ + ...await settledErrors(this.inits.values()), + ...await settledErrors([...this.buffers.keys()].map(s => this.flush(s))), + ...await settledErrors(this.chains.values()), + ] + if (errors.length > 0) { + throw new AggregateError(errors, 'session-persistence-sqlite dispose failed') + } + } finally { + await this.ready + this.db.close() + } }, 'session-persistence-sqlite write path') // HMR: a hot reload does not replay session/created, so seed existing live diff --git a/packages/session-persistence/tests/contract.ts b/packages/session-persistence/tests/contract.ts index f8535a70f9..73f6f653bd 100644 --- a/packages/session-persistence/tests/contract.ts +++ b/packages/session-persistence/tests/contract.ts @@ -8,7 +8,7 @@ * @module @deepseek-ai/dsh-session-persistence/tests/contract */ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { SessionId } from '@deepseek-ai/dsh-session' import type { SessionEvent, SessionMeta } from '@deepseek-ai/dsh-session' import { CallId } from '@deepseek-ai/dsh-llm' @@ -250,11 +250,19 @@ export function runPersistenceContract(name: string, make: () => Promise): Promise { const entry = this.store.get(id) - if (entry) Object.assign(entry.meta, summary) + if (entry) Object.assign(entry.meta, summary, { updatedAt: summary.updatedAt ?? Date.now() }) } } diff --git a/packages/session/src/index.ts b/packages/session/src/index.ts index 34eefb24a3..c64af8d0e4 100644 --- a/packages/session/src/index.ts +++ b/packages/session/src/index.ts @@ -30,7 +30,7 @@ declare module 'cordis' { /** * Awaited durability checkpoint. The agent loop awaits * `ctx.parallel('session/flush', session)` at every turn end; persistence - * plugins (JSONL, sqlite — TODO, future phase) drain their write-behind + * plugins (JSONL, SQLite) drain their write-behind * buffers here and on fiber dispose. */ 'session/flush'(session: Session): Promise | void diff --git a/packages/session/src/repair.ts b/packages/session/src/repair.ts index 6a3f60a681..d5e66516bc 100644 --- a/packages/session/src/repair.ts +++ b/packages/session/src/repair.ts @@ -27,9 +27,10 @@ * — which every provider rejects as an invalid transcript on the next request. * Synthesizing an error result per orphaned call keeps resume safe. * - * This module computes those synthetic closers from an event list; the backend - * returns them inline from `load` (so the reconstructed session is balanced and - * immediately usable) and persists them on the first post-load `append`. + * This module computes those synthetic closers from an event list; backends + * return them inline from `load` (so the reconstructed session is balanced and + * immediately usable) and persist them during that mutating load before any + * later append continues the log. * * @module @deepseek-ai/dsh-session/repair */ From b920239389866bf80c9d74528c92d96ebc8803cc Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:26:31 +0800 Subject: [PATCH 05/16] fix(acp): align prompt and workspace contracts --- docs/rfc/010-acp-agent-client-protocol.md | 12 +++---- examples/acp-agent/README.md | 6 ++-- examples/acp-agent/start.ts | 4 ++- examples/acp-agent/tests/acp.e2e.ts | 23 +++++++------ packages/acp/README.md | 19 +++++------ packages/acp/src/codec.ts | 39 +++++++++++++---------- packages/acp/src/index.ts | 38 ++++++++++++++-------- packages/acp/tests/bridge.spec.ts | 13 ++++---- packages/acp/tests/codec.spec.ts | 12 ++++--- packages/acp/tests/edges.spec.ts | 9 ++++++ packages/acp/tests/load.spec.ts | 10 +++--- packages/acp/tests/stream-update.spec.ts | 13 ++++++++ 12 files changed, 123 insertions(+), 75 deletions(-) diff --git a/docs/rfc/010-acp-agent-client-protocol.md b/docs/rfc/010-acp-agent-client-protocol.md index 47f59792de..cf945bc377 100644 --- a/docs/rfc/010-acp-agent-client-protocol.md +++ b/docs/rfc/010-acp-agent-client-protocol.md @@ -2,7 +2,7 @@ Status: proposed -> **Implementation status (MVP landed):** steps 1, 2, 3, 4, 6, 7, 8 are implemented in `packages/acp` + `examples/acp-agent`. **Step 5 (the `session/request_permission` permission gate) is deferred** — the bridge ships a pass-through (tools run with the executor's full authority) marked `TODO(rfc010-permission-gate)`, and lays down only the `WeakMap` ownership seam the gate will build on. Status stays `proposed` until the gate lands. One further best-effort limitation is tracked as `TODO(rfc010-cancel-prestep)`: `session/cancel` aborts a running step and settles the RPC as `cancelled`, but a turn still queued (not yet started) when the cancel arrives may execute before the abort takes effect, pending a loop-level pre-step cancel. **Per-session `cwd` is now honored** (lifting the original "launch the server in the workspace root" restriction — see § Deferred): any absolute `cwd` is accepted and routed to the bash workdir via `session.header.cwd`, so an editor can open any project folder and N sessions can each target a different directory. +> **Implementation status (MVP landed):** steps 1, 2, 3, 4, 6, 7, 8 are implemented in `packages/acp` + `examples/acp-agent`. **Step 5 (the `session/request_permission` permission gate) is deferred** — the bridge ships a pass-through (tools run with the executor's full authority) marked `TODO(rfc010-permission-gate)`, and lays down only the `WeakMap` ownership seam the gate will build on. Status stays `proposed` until the gate lands. Queue-aware pre-step cancellation and per-agent disposal are follow-up seam work in [RFC 014](014-agent-lifecycle-and-ownership-seams.md). **Per-session `cwd` is honored**: `session/new` accepts any absolute cwd; `session/load` requires the request cwd to match the persisted session cwd so the editor and bash executor agree on the workspace. ## Problem @@ -10,7 +10,7 @@ The coding agent is reachable only through the readline `stdio-chat` plugin: it Editors are converging on the Agent Client Protocol (ACP), which Zed and others speak: JSON-RPC 2.0 over newline-delimited stdio, modeled on the Language Server Protocol. An editor boots the agent as a subprocess and exchanges `initialize` / `session/new` / `session/prompt`, rendering streamed `session/update` notifications and `session/request_permission` prompts. The goal is for the agent to be a drop-in ACP server — implement the protocol once and run in any ACP client, with no per-editor glue. -This RFC has a hard prerequisite on RFC 009: it assumes durable session persistence (the `SessionPersistence` service and the async `AgentLoop.resume` seam) is implemented, so resuming a session via `session/load` is in scope. None of those APIs exist yet — `AgentLoop` currently exposes only the synchronous `create` — so 010 must land after, or in the same change as, 009, and pins to 009's `resume(agentId, resumeSessionId)` contract. RFC 009 persists every `SessionEvent` verbatim (including `assistant/chunk`), so a loaded session has the stream chunks needed to replay turns to the client. +This RFC has a hard prerequisite on RFC 009: durable session persistence (the `SessionPersistence` service and the async `ctx.agents.resume` factory seam) is implemented, so resuming a session via `session/load` is in scope. RFC 009 persists every `SessionEvent` verbatim (including `assistant/chunk`), so a loaded session has the stream chunks needed to replay turns to the client. ## Proposal @@ -22,10 +22,10 @@ The mapping between ACP and existing harness seams — each row names the seam a | ACP (client ⇄ agent) | Harness seam | Notes | |---|---|---| -| `initialize` | static handler | negotiate `protocolVersion` (echo the supported version, else error); advertise text-only `promptCapabilities` and `loadSession: true`; report agent name/version | -| `session/new {cwd, mcpServers, additionalDirectories}` → `{sessionId}` | the `dsh-agent` create factory (see Dependency note + Plan) | the seam must accept `{ sessionId, meta }` so the ACP-generated `sessionId` becomes the live/persisted session id and the validated `cwd` is attached as the `SessionHeader` (today `AgentLoop.create(id)` hardcodes `${id}-session` and takes no metadata); reject a 2nd session (single-session MVP, see RFC 011); `cwd` validated (require absolute) — any absolute cwd is honored: it becomes the session's `SessionHeader.cwd` and the default bash workdir (per-session cwd, see § Deferred → RESOLVED), so the server need not launch in the workspace; `mcpServers` ignored (no `mcpCapabilities` advertised); non-empty `additionalDirectories` rejected for the MVP (the bridge cannot yet widen bash/tool filesystem scope, so silently ignoring them would desync the client's filesystem-scope UI) | -| `session/load {sessionId, cwd, mcpServers, additionalDirectories}` | the `dsh-agent` resume factory (RFC 009 + Dependency note) | load `{ meta, events }`, seed the session, re-derive history via `deriveMessages()`, replay prior turns to the client as `session/update` per the ACP load contract; `additionalDirectories` rejected as in `session/new` | -| `session/prompt {prompt}` | `agent.send()` (idle) | text blocks → `TextBlock`; reject image/audio per advertised capabilities; one in-flight prompt per session | +| `initialize` | static handler | negotiate `protocolVersion` (echo the supported version, else error); advertise baseline text/resource-link prompt support, no image/audio/embedded resources, and `loadSession: true`; report agent name/version | +| `session/new {cwd, mcpServers, additionalDirectories}` → `{sessionId}` | the `dsh-agent` create factory (see Dependency note + Plan) | the seam accepts `{ sessionId, meta }` so the ACP-generated `sessionId` becomes the live/persisted session id and the validated `cwd` is attached as the `SessionHeader`; N concurrent sessions are allowed (RFC 011); `cwd` validated (require absolute); non-empty `mcpServers` and `additionalDirectories` rejected for the MVP rather than silently ignored | +| `session/load {sessionId, cwd, mcpServers, additionalDirectories}` | the `dsh-agent` resume factory (RFC 009 + Dependency note) | load `{ meta, events }`, require the request `cwd` to match the persisted session cwd, seed the session, re-derive history via `deriveMessages()`, replay prior turns to the client as `session/update` per the ACP load contract; `mcpServers` and `additionalDirectories` rejected as in `session/new` | +| `session/prompt {prompt}` | `agent.send()` (idle) | baseline `text` and `resource_link` blocks are supported (`resource_link` renders as explicit text); reject image/audio/embedded resource per advertised capabilities; one in-flight prompt per session | | resolve `session/prompt` → `{stopReason}` | `agent/turn-end` (extended, see Plan) | map the harness kebab `TurnEndReason` to the ACP snake_case `StopReason` wire enum: `completed`→`end_turn`, `max-tokens`→`max_tokens`, `aborted`(cancel)→`cancelled`, plus `refusal`/`max_turn_requests` when applicable; honor the batch-into-one-turn and send-not-synchronously-running settle semantics | | `session/update: agent_message_chunk` | `agent/stream-chunk` `text-delta` only | do NOT also emit on `block-end(TextBlock)` — it carries the fully-assembled block and would duplicate the streamed text | | `session/update: agent_thought_chunk` | `agent/stream-chunk` `reasoning-delta` | | diff --git a/examples/acp-agent/README.md b/examples/acp-agent/README.md index 846e49fe78..a75279e3ed 100644 --- a/examples/acp-agent/README.md +++ b/examples/acp-agent/README.md @@ -21,15 +21,15 @@ Add to your Zed `settings.json` under `agent_servers`: "agent_servers": { "DeepSeek Harness": { "command": "pnpm", - "args": ["run", "demo:acp"], + "args": ["--dir", "/path/to/deepseek-harness", "run", "demo:acp"], "env": { "DEEPSEEK_API_KEY": "sk-…" } } } } ``` -The editor sets each session's `cwd` to the project it opens; the agent's bash tools run there (see the per-session `cwd` note in `packages/acp`), so the server does not need to be launched in the workspace. +The editor sets each session's `cwd` to the project it opens; the agent's bash tools run there (see the per-session `cwd` note in `packages/acp`), so launch the server from the harness repo with `pnpm --dir …` and let ACP carry the workspace path per session. ## MVP limitations -The bridge supports N concurrent sessions per connection, each in its own workspace `cwd` (RFC 011). Remaining limits: text-only prompts, `additionalDirectories` rejected (a session operates in its single `cwd`), and the tool-permission gate is deferred (`TODO(rfc010-permission-gate)` — tools run with the executor's full authority). See `packages/acp/README.md` for the full contract. +The bridge supports N concurrent sessions per connection, each in its own workspace `cwd` (RFC 011). Remaining limits: prompts support ACP's baseline `text` and `resource_link` blocks only, `additionalDirectories` and `mcpServers` are rejected, and the tool-permission gate is deferred (`TODO(rfc010-permission-gate)` — tools run with the executor's full authority). See `packages/acp/README.md` for the full contract. diff --git a/examples/acp-agent/start.ts b/examples/acp-agent/start.ts index 1c97c0c8c0..7c21ed9262 100644 --- a/examples/acp-agent/start.ts +++ b/examples/acp-agent/start.ts @@ -1,4 +1,4 @@ -import { pathToFileURL } from 'node:url' +import { fileURLToPath, pathToFileURL } from 'node:url' import { Context } from 'cordis' import Loader from '@cordisjs/plugin-loader' @@ -18,6 +18,8 @@ try { // ENOENT (no .env) is fine — rely on the ambient environment. } +process.chdir(fileURLToPath(new URL('../..', import.meta.url))) + const ctx = new Context() ctx.baseUrl = pathToFileURL(import.meta.dirname).href + '/' diff --git a/examples/acp-agent/tests/acp.e2e.ts b/examples/acp-agent/tests/acp.e2e.ts index e69a4bfc0f..08d3180840 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -27,11 +27,11 @@ import { */ const startScript = fileURLToPath(new URL('../start.ts', import.meta.url)) -// Resolve tsx's loader to an ABSOLUTE path: the subprocess runs with cwd set to -// a temp workdir (this test launches there and uses it as the session cwd; the -// bridge no longer requires cwd === the launch dir, but a temp dir keeps the -// test hermetic), where a bare `--import tsx` would not resolve from -// node_modules. import.meta.resolve gives the worktree's tsx regardless of cwd. +const repoRoot = fileURLToPath(new URL('../../../', import.meta.url)) +// Resolve tsx's loader to an ABSOLUTE path. The subprocess launches from the +// harness repo (so pnpm/package resolution is stable) while each ACP session's +// request cwd points at the temp workspace; import.meta.resolve gives the +// worktree's tsx regardless of launch cwd. const tsxLoader = fileURLToPath(import.meta.resolve('tsx')) interface Spawned { @@ -41,11 +41,11 @@ interface Spawned { stderr: string[] } -function spawnAcpAgent(cwd: string): Spawned { +function spawnAcpAgent(): Spawned { const child = spawn( process.execPath, ['--import', tsxLoader, startScript], - { cwd, env: { ...process.env }, stdio: ['pipe', 'pipe', 'pipe'] }, + { cwd: repoRoot, env: { ...process.env }, stdio: ['pipe', 'pipe', 'pipe'] }, ) const stderr: string[] = [] child.stderr.setEncoding('utf8') @@ -91,13 +91,16 @@ describe('acp-agent stdout purity (no key required)', () => { // present at boot, not valid — the key is used only on a real model call, // which this purity test never triggers). So this runs WITHOUT real creds. const child = spawn(process.execPath, ['--import', tsxLoader, startScript], { - cwd: workdir, + cwd: repoRoot, env: { ...process.env, DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot' }, stdio: ['pipe', 'pipe', 'pipe'], }) const out: string[] = [] + const stderr: string[] = [] child.stdout.setEncoding('utf8') + child.stderr.setEncoding('utf8') child.stdout.on('data', (c: string) => out.push(c)) + child.stderr.on('data', (c: string) => stderr.push(c)) // Send a single initialize request as a newline-delimited JSON-RPC frame. const req = JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} } }) @@ -108,7 +111,7 @@ describe('acp-agent stdout purity (no key required)', () => { child.kill('SIGKILL') const lines = out.join('').split('\n').filter(l => l.trim().length > 0) - expect(lines.length).toBeGreaterThan(0) + expect(lines.length, stderr.join('')).toBeGreaterThan(0) for (const line of lines) { // Every stdout line MUST parse as JSON (a JSON-RPC frame). A non-JSON // line means a logger/print leaked onto the protocol channel. @@ -120,7 +123,7 @@ describe('acp-agent stdout purity (no key required)', () => { describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over ACP', () => { it('runs a real turn and the agent writes the requested file (verified on disk)', async () => { workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-')) - spawned = spawnAcpAgent(workdir) + spawned = spawnAcpAgent() const { client, updates } = spawned await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) diff --git a/packages/acp/README.md b/packages/acp/README.md index 3d2fd15feb..a8b441bb82 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -23,12 +23,12 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline ` | ACP method | Harness seam | Notes | |---|---|---| -| `initialize` | static | negotiate `protocolVersion`; advertise text-only `promptCapabilities` and `loadSession: true` | -| `session/new` | `ctx.agents.create({ sessionId, meta:{cwd} })` | creates a new session/agent; N concurrent sessions are allowed (RFC 011), keyed by id; `cwd` must be absolute (it becomes the session's workspace — see Per-session cwd); `additionalDirectories` rejected; `mcpServers` ignored | -| `session/load` | `ctx.agents.resume(...)` | replays the persisted event log to the client as `session/update` — the USER side (`user/message` → `user_message_chunk`), assistant text/reasoning (`assistant/chunk`), and tool calls/results (`tool/call` + `tool/result`). Re-loading an already-live id is rejected; the id's load slot is reserved (`loadingIds`) BEFORE the async resume so a pipelined load of the SAME id can't leak a second agent (distinct ids load concurrently). The resumed session keeps its PERSISTED header `cwd`, so its bash tools run in the original workspace; the requested `cwd` only needs to be absolute. After the async resume a `closed` re-check refuses to install a record if the bridge tore down mid-load | -| `session/prompt` | `agent.send()` | text-only; rejects image/audio and empty prompts; one in-flight prompt PER session (independent); settles on the OWNING turn's end (a turn that ends in `error` rejects the RPC) | +| `initialize` | static | negotiate `protocolVersion`; advertise baseline text/resource-link prompt support, no image/audio/embedded resources, and `loadSession: true` | +| `session/new` | `ctx.agents.create({ sessionId, meta:{cwd} })` | creates a new session/agent; N concurrent sessions are allowed (RFC 011), keyed by id; `cwd` must be absolute (it becomes the session's workspace — see Per-session cwd); non-empty `additionalDirectories` and `mcpServers` are rejected until those scopes are implemented | +| `session/load` | `ctx.agents.resume(...)` | replays the persisted event log to the client as `session/update` — the USER side (`user/message` → `user_message_chunk`), assistant text/reasoning (`assistant/chunk`), and tool calls/results (`tool/call` + `tool/result`). Re-loading an already-live id is rejected; the id's load slot is reserved (`loadingIds`) BEFORE the async resume so a pipelined load of the SAME id can't leak a second agent (distinct ids load concurrently). The resumed session keeps its PERSISTED header `cwd`, and the requested `cwd` must match it so editor UI and bash execution agree on the workspace. After the async resume a `closed` re-check refuses to install a record if the bridge tore down mid-load | +| `session/prompt` | `agent.send()` | accepts ACP baseline `text` and `resource_link` blocks; rejects image/audio/embedded resources and empty prompts; one in-flight prompt PER session (independent); settles on the OWNING turn's end (a turn that ends in `error` rejects the RPC) | | `session/cancel` | `agent.abort()` | aborts a running step + settles the prompt `cancelled` for ONLY that session — a cancel never touches another session's stream or prompt (see limitation below) | -| `session/update` | `session/event` | `agent_message_chunk` (text-delta), `agent_thought_chunk` (reasoning-delta), `user_message_chunk` (load replay), `tool_call`/`tool_call_update` | +| `session/update` | `session/event` | `agent_message_chunk` (text-delta), `agent_thought_chunk` (reasoning-delta), `user_message_chunk` (load replay only), `tool_call`/`tool_call_update` | ## Multi-session (RFC 011) @@ -38,7 +38,7 @@ Background-task isolation rides on `dsh-tool-bash`: bash task ids are global and ## Per-session cwd -Each session runs in its own workspace, recorded as the session's `SessionHeader.cwd`. On `session/new` the (absolute) request `cwd` becomes that header cwd; on `session/load` the resumed session keeps its PERSISTED header cwd (the request `cwd` is only shape-checked — it does not override the stored one), and a load whose persisted session has no absolute cwd is REJECTED up front via a metadata-only `list()` check, BEFORE resume constructs an agent (else bash would silently fall back to the server's launch dir, and a post-resume reject would leak the registered agent). `dsh-tool-bash` then defaults the bash workdir to the calling agent's `session.header.cwd` (an explicit model `workdir` still wins; a relative one resolves against the session cwd; with no session cwd the executor falls back to its own config / `process.cwd()`). So the server no longer has to be launched in the workspace — an editor can open any project folder, and N sessions over one connection can each target a different directory. (`additionalDirectories` is still rejected: widening the tool/filesystem scope beyond the single cwd is a separate sandbox concern.) +Each session runs in its own workspace, recorded as the session's `SessionHeader.cwd`. On `session/new` the (absolute) request `cwd` becomes that header cwd; on `session/load` the resumed session keeps its PERSISTED header cwd and the request `cwd` must match it (a mismatch is rejected up front) so the editor never believes tools run in one workspace while bash runs in another. A load whose persisted session has no absolute cwd is also rejected via a metadata-only `list()` check, BEFORE resume constructs an agent. `dsh-tool-bash` then defaults the bash workdir to the calling agent's `session.header.cwd` (an explicit model `workdir` still wins; a relative one resolves against the session cwd; with no session cwd the executor falls back to its own config / `process.cwd()`). So the server no longer has to be launched in the workspace — an editor can open any project folder, and N sessions over one connection can each target a different directory. (`additionalDirectories` and `mcpServers` are still rejected: widening tool/filesystem/protocol scope is separate work.) ## Settle-exactly-once @@ -53,7 +53,7 @@ Teardown reaches quiescence: for EVERY live session settle any pending prompt as - **`TODO(rfc010-permission-gate)`** — the `tools/execute` permission gate (`session/request_permission`) is NOT implemented; tools run with the executor's full authority. The `agent→sessionId` reverse map is in place so the gate can route a permission request (which receives only `exec.agent`) back to its originating session. RFC 010/011 stay `proposed` until the gate (and per-session permission ownership) land. - **`TODO(rfc010-cancel-prestep)`** — `session/cancel` (and teardown/disconnect) is honest RPC/UI cancellation plus best-effort abort: a *running* step is aborted, but a turn that is queued-but-not-yet-started (the gap before `agent.abort()` has an `AbortController` to signal) may still run to completion. This same window means disposal/disconnect can return while one short queued turn per session still runs, and a prompt accepted right after a pre-step cancel can be batched into the cancelled turn (the loop merges queued messages into one turn). A loop-level queue-aware cancel will close this; the single-in-flight-per-session rule bounds the worst case to one extra prompt per session. - **`TODO(rfc010-agent-disposal)`** — the factory (`ctx.agents.create`/`resume`) returns no per-agent disposer, so teardown aborts+drains each agent but cannot individually unregister it; on a bare client disconnect (no host dispose) the idled agents linger in `ctx.agents` until the host context disposes. A reconnect spins up a fresh context, so this strands no work; a per-agent disposal seam is the follow-up. -- **`additionalDirectories`** — rejected. A session operates in its single `cwd` (see Per-session cwd); widening the tool/filesystem scope to extra roots is a separate sandbox concern, not yet implemented. +- **`additionalDirectories` / `mcpServers`** — rejected. A session operates in its single `cwd` and no MCP bridge is wired yet; silently ignoring requested roots or servers would desync client expectations. ## stdout is the protocol @@ -61,14 +61,15 @@ The JSON-RPC frames go on stdout, so this plugin MUST run in an example that loa ## Running -`pnpm run demo:acp` boots `examples/acp-agent` (needs `DEEPSEEK_API_KEY`). Point an ACP client at it; for Zed, add to `agent_servers`: +`pnpm --dir /path/to/deepseek-harness run demo:acp` boots `examples/acp-agent` (needs `DEEPSEEK_API_KEY`). Point an ACP client at it; for Zed, add to `agent_servers`: ```json { "agent_servers": { "DeepSeek Harness": { "command": "pnpm", - "args": ["run", "demo:acp"] + "args": ["--dir", "/path/to/deepseek-harness", "run", "demo:acp"], + "env": { "DEEPSEEK_API_KEY": "sk-…" } } } } diff --git a/packages/acp/src/codec.ts b/packages/acp/src/codec.ts index 5ef31d00dd..dd28bc5ae6 100644 --- a/packages/acp/src/codec.ts +++ b/packages/acp/src/codec.ts @@ -59,8 +59,9 @@ export function turnEndToStopReason(reason: TurnEndReason): StopReason { /** * Translate a harness {@link ContentBlock} from a prompt into ACP content for * replay, or `undefined` for block kinds the bridge does not surface to the - * client as message content. Today only `text` maps (text-only - * `promptCapabilities`); `reasoning` is surfaced via `agent_thought_chunk` + * client as message content. Today only `text` maps; `resource_link` is an + * ACP prompt-only input rendered into text by {@link acpPromptToText}; + * `reasoning` is surfaced via `agent_thought_chunk` * streaming rather than as a message block, and `tool-call`/`tool-result`/ * `image` are handled by the tool-call update path or not advertised. */ @@ -70,34 +71,38 @@ export function harnessBlockToAcpContent(block: ContentBlock): AcpContentBlock | return { type: 'text', text: block.text } // reasoning → streamed as agent_thought_chunk, not a message block // tool-call / tool-result → the tool_call / tool_call_update path - // image → not advertised (text-only promptCapabilities) + // image → not advertised default: return undefined } } /** - * Extract plain text from an ACP prompt's content blocks, concatenating every - * `text` block. Non-text blocks are ignored here; the caller rejects a prompt - * carrying image/audio per the advertised text-only capabilities BEFORE - * calling this, so dropping them here only affects `resource`/`resource_link` - * (which carry no inline text to forward in the MVP). + * Extract plain text from an ACP prompt's content blocks. Text blocks are + * concatenated verbatim; resource links become explicit textual references so + * baseline ACP clients can point at files without the bridge silently dropping + * that context. */ export function acpPromptToText(prompt: readonly AcpContentBlock[]): string { return prompt - .filter((block): block is AcpContentBlock & { type: 'text'; text: string } => block.type === 'text') - .map(block => block.text) + .flatMap((block): string[] => { + switch (block.type) { + case 'text': + return [block.text] + case 'resource_link': + return [`\n[resource_link name=${JSON.stringify(block.name)} uri=${JSON.stringify(block.uri)}]\n`] + default: + return [] + } + }) .join('') } /** - * Whether an ACP prompt contains any content the text-only bridge cannot - * accept — i.e. ANY non-`text` block (image, audio, `resource`, `resource_link`, - * …). The caller rejects such a prompt up front rather than silently dropping - * the unsupported parts: a prompt like `[text, resource_link]` carries context - * the model would otherwise never see, so running it text-only would be silent - * data loss. When richer block kinds are supported, narrow this. + * Whether an ACP prompt contains content the bridge cannot accept. Baseline ACP + * requires `text` and `resource_link`; richer inline payloads (`resource`, + * image, audio, …) are rejected rather than silently dropped. */ export function promptHasUnsupportedContent(prompt: readonly AcpContentBlock[]): boolean { - return prompt.some(block => block.type !== 'text') + return prompt.some(block => block.type !== 'text' && block.type !== 'resource_link') } diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index 03e0d17a5e..710db476d7 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -8,7 +8,7 @@ * the existing `agent/*` event taxonomy, the `dsh-agent` create/resume factory, * and `dsh-session-persistence` (for `session/load`). It maps: * - * - `initialize` → protocol-version negotiation, text-only capabilities + * - `initialize` → protocol-version negotiation, baseline prompt capabilities * - `session/new` → `ctx.agents.create({ sessionId, meta:{cwd} })` * - `session/load` → `ctx.agents.resume(...)` then replay the event log * - `session/prompt` → `agent.send()`, settle on the owning turn's end (a turn @@ -259,7 +259,7 @@ export function apply(ctx: Context, config: AcpConfig): void { ctx.on('session/event', (session, event: SessionEvent) => { const rec = sessions.get(session.header.id) if (rec === undefined) return - streamSessionEventUpdate(rec.sessionId, event, notify) + streamSessionEventUpdate(rec.sessionId, event, notify, { includeUserMessages: false }) const inflight = rec.inflight if (inflight === undefined) return if (event.type === 'turn/start') { @@ -360,7 +360,7 @@ export function apply(ctx: Context, config: AcpConfig): void { agentInfo: { name: agentName, version: agentVersion }, agentCapabilities: { loadSession: true, - // text-only: no image/audio/embeddedContext, no mcpCapabilities + // Baseline text/resource_link only: no image/audio/embedded resource, no mcpCapabilities. promptCapabilities: { image: false, audio: false, embeddedContext: false }, }, authMethods: [], @@ -419,6 +419,9 @@ export function apply(ctx: Context, config: AcpConfig): void { `session ${params.sessionId} has no absolute persisted cwd; cannot determine its workspace (it predates per-session cwd, or was created without one)`, ) } + if (meta !== undefined && meta.cwd !== params.cwd) { + throw invalidParams(`session ${params.sessionId} cwd mismatch: persisted ${meta.cwd}, requested ${params.cwd}`) + } const agent = await ctx.agents.resume({ agentId: params.sessionId, resumeSessionId: params.sessionId, @@ -459,7 +462,7 @@ export function apply(ctx: Context, config: AcpConfig): void { throw invalidParams('a prompt is already in flight for this session') } if (promptHasUnsupportedContent(params.prompt)) { - throw invalidParams('only text prompt content is supported (text-only promptCapabilities); image/audio/resource blocks are rejected rather than silently dropped') + throw invalidParams('only text and resource_link prompt content is supported; image/audio/resource blocks are rejected rather than silently dropped') } const text = acpPromptToText(params.prompt) if (text.trim().length === 0) { @@ -615,19 +618,22 @@ export function agentOptions(config: AcpConfig): { model?: string; systemPrompt? * bash workdir — the request cwd does not override it. * Any absolute path is accepted (the per-session cwd flows to the bash executor * — see `dsh-tool-bash`), so the server no longer has to launch in the - * workspace. `additionalDirectories` must still be empty: widening the - * tool/filesystem scope beyond the single cwd is a separate, unimplemented - * concern (a sandbox seam), and silently ignoring extra roots would desync the - * client's filesystem-scope UI. Both request shapes carry `cwd: string` and - * `additionalDirectories?: string[]`, so one validator covers both. + * workspace. `additionalDirectories` and `mcpServers` must still be empty: + * widening tool/filesystem/protocol scope is separate, unimplemented work, and + * silently ignoring requested roots/servers would desync the client's UI. Both + * request shapes carry the same workspace/scope fields, so one validator covers + * both. */ -function validateWorkspaceParams(params: { cwd: string; additionalDirectories?: string[] }): void { +function validateWorkspaceParams(params: { cwd: string; additionalDirectories?: string[]; mcpServers?: unknown[] }): void { if (!isAbsolute(params.cwd)) { throw invalidParams(`cwd must be an absolute path: ${params.cwd}`) } if (params.additionalDirectories !== undefined && params.additionalDirectories.length > 0) { throw invalidParams('additionalDirectories is not supported in this MVP') } + if (params.mcpServers !== undefined && params.mcpServers.length > 0) { + throw invalidParams('mcpServers is not supported in this MVP') + } } /** @@ -637,8 +643,9 @@ function validateWorkspaceParams(params: { cwd: string; additionalDirectories?: * identical update stream from the same event log. * * - `assistant/chunk` text-delta/reasoning-delta → message/thought chunks - * - `user/message` → `user_message_chunk` (text blocks) — so a `session/load` - * replay reconstructs the USER side of each turn, not just the agent's + * - `user/message` → `user_message_chunk` during load replay only — so a + * loaded transcript reconstructs the USER side of each turn without echoing + * a live `session/prompt` back to the client * - `tool/call` → `tool_call` (pending) * - `tool/result` → `tool_call_update` (completed/failed) * @@ -649,7 +656,9 @@ export function streamSessionEventUpdate( sessionId: string, event: SessionEvent, notify: (notification: SessionNotification) => void, + options: { includeUserMessages?: boolean } = {}, ): void { + const includeUserMessages = options.includeUserMessages ?? true switch (event.type) { case 'assistant/chunk': { const chunk = event.data.chunk @@ -661,9 +670,10 @@ export function streamSessionEventUpdate( return } case 'user/message': { + if (!includeUserMessages) return // Replay the user's prompt so a loaded session shows both sides of each - // turn. Only text blocks carry inline content the bridge surfaces (the - // prompt path is text-only); other block kinds produce no chunk. + // turn. Live prompt turns suppress this path to avoid duplicating what + // the client just sent. for (const block of event.data.content) { const content = harnessBlockToAcpContent(block) if (content !== undefined) { diff --git a/packages/acp/tests/bridge.spec.ts b/packages/acp/tests/bridge.spec.ts index d537f4f684..dd10a88bcb 100644 --- a/packages/acp/tests/bridge.spec.ts +++ b/packages/acp/tests/bridge.spec.ts @@ -105,19 +105,20 @@ describe('acp bridge', () => { })).rejects.toThrow(/text/) }) - it('rejects a prompt carrying a non-text block alongside text (no silent context loss)', async () => { - // A text + resource_link prompt must be rejected, not run text-only with the - // resource silently dropped — that would feed the model an incomplete prompt. - harness = await makeBridgeHarness({ storageDir, script: [] }) + it('accepts a resource_link prompt by rendering the link into the text sent to the agent', async () => { + harness = await makeBridgeHarness({ storageDir, script: [textResponse('ok')] }) await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) - await expect(harness.client.prompt({ + const result = await harness.client.prompt({ sessionId, prompt: [ { type: 'text', text: 'fix the bug in' }, { type: 'resource_link', uri: 'file:///x.ts', name: 'x.ts' }, ], - })).rejects.toThrow(/text/) + }) + expect(result.stopReason).toBe('end_turn') + const user = harness.ctx.agents.get(sessionId)!.session.events.find(event => event.type === 'user/message') + expect(JSON.stringify(user)).toContain('resource_link') }) it('rejects a prompt for an unknown session', async () => { diff --git a/packages/acp/tests/codec.spec.ts b/packages/acp/tests/codec.spec.ts index 58c5a4bcdf..38a7a6cb41 100644 --- a/packages/acp/tests/codec.spec.ts +++ b/packages/acp/tests/codec.spec.ts @@ -39,27 +39,29 @@ describe('harnessBlockToAcpContent', () => { }) describe('acpPromptToText', () => { - it('concatenates text blocks and ignores non-text', () => { + it('concatenates text blocks and renders resource links explicitly', () => { const prompt: AcpContentBlock[] = [ { type: 'text', text: 'hello ' }, { type: 'resource_link', uri: 'file:///x', name: 'x' }, { type: 'text', text: 'world' }, ] - expect(acpPromptToText(prompt)).toBe('hello world') + expect(acpPromptToText(prompt)).toBe('hello \n[resource_link name="x" uri="file:///x"]\nworld') }) it('returns empty string for a prompt with no text blocks', () => { - expect(acpPromptToText([{ type: 'resource_link', uri: 'file:///x', name: 'x' }])).toBe('') + expect(acpPromptToText([{ type: 'image', mimeType: 'image/png', data: 'AA==' }])).toBe('') }) }) describe('promptHasUnsupportedContent', () => { - it('detects image and audio blocks', () => { + it('detects image, audio, and embedded resource blocks', () => { expect(promptHasUnsupportedContent([{ type: 'image', mimeType: 'image/png', data: 'AA==' }])).toBe(true) expect(promptHasUnsupportedContent([{ type: 'audio', mimeType: 'audio/wav', data: 'AA==' }])).toBe(true) + expect(promptHasUnsupportedContent([{ type: 'resource', resource: { uri: 'file:///x', text: 'x' } }])).toBe(true) }) - it('passes a text-only prompt', () => { + it('passes baseline text and resource_link prompt blocks', () => { expect(promptHasUnsupportedContent([{ type: 'text', text: 'hi' }])).toBe(false) + expect(promptHasUnsupportedContent([{ type: 'resource_link', uri: 'file:///x', name: 'x' }])).toBe(false) }) }) diff --git a/packages/acp/tests/edges.spec.ts b/packages/acp/tests/edges.spec.ts index 2a48ae6d8d..9484368322 100644 --- a/packages/acp/tests/edges.spec.ts +++ b/packages/acp/tests/edges.spec.ts @@ -52,4 +52,13 @@ describe('acp bridge — demux & config edges', () => { const a = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [], additionalDirectories: [] }) expect(a.sessionId).toBeTruthy() }) + + it('rejects non-empty mcpServers until MCP wiring is implemented', async () => { + harness = await makeBridgeHarness({ storageDir }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + await expect(harness.client.newSession({ + cwd: process.cwd(), + mcpServers: [{ name: 'fs', command: 'npx', args: ['server'], env: [] }], + })).rejects.toThrow(/mcpServers/) + }) }) diff --git a/packages/acp/tests/load.spec.ts b/packages/acp/tests/load.spec.ts index 3bed1f836d..8e3748241a 100644 --- a/packages/acp/tests/load.spec.ts +++ b/packages/acp/tests/load.spec.ts @@ -85,7 +85,7 @@ describe('acp bridge — session/load replay', () => { expect(loader.ctx.agents.get(sessionId)).toBeUndefined() }) - it('loads a session whose persisted cwd differs from the launch dir (honors per-session cwd)', async () => { + it('rejects load when the requested cwd does not match the persisted session cwd', async () => { // Seed a session on disk whose header.cwd is a DIFFERENT absolute path than // the server's launch dir. The bridge must LOAD it (per-session cwd is // honored — the resumed session keeps header.cwd, and bash routes there), no @@ -101,10 +101,12 @@ describe('acp bridge — session/load replay', () => { ]) await loader.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) - // Load succeeds even though the requested cwd is the launch dir, not otherCwd. - const res = await loader.client.loadSession({ sessionId: 'elsewhere', cwd: process.cwd(), mcpServers: [] }) + await expect(loader.client.loadSession({ sessionId: 'elsewhere', cwd: process.cwd(), mcpServers: [] })) + .rejects.toThrow(/cwd mismatch/) + expect(loader.ctx.agents.get('elsewhere')).toBeUndefined() + + const res = await loader.client.loadSession({ sessionId: 'elsewhere', cwd: otherCwd, mcpServers: [] }) expect(res).toBeDefined() - // The resumed session retains its ORIGINAL workspace cwd (so bash runs there). expect(loader.ctx.agents.get('elsewhere')!.session.header.cwd).toBe(otherCwd) }) diff --git a/packages/acp/tests/stream-update.spec.ts b/packages/acp/tests/stream-update.spec.ts index c37de8b344..cf877ea286 100644 --- a/packages/acp/tests/stream-update.spec.ts +++ b/packages/acp/tests/stream-update.spec.ts @@ -11,6 +11,12 @@ function updatesFor(event: SessionEvent): SessionNotification['update'][] { return out } +function liveUpdatesFor(event: SessionEvent): SessionNotification['update'][] { + const out: SessionNotification['update'][] = [] + streamSessionEventUpdate('s1', event, n => out.push(n.update), { includeUserMessages: false }) + return out +} + function evt(type: T, data: Extract['data']): SessionEvent { return { type, seq: 0, time: 0, data } as SessionEvent } @@ -92,6 +98,13 @@ describe('streamSessionEventUpdate', () => { expect(updatesFor(evt('user/message', { content: [], source: { kind: 'user' } }))).toEqual([]) }) + it('can suppress user/message chunks for live prompt turns', () => { + expect(liveUpdatesFor(evt('user/message', { + content: [{ type: 'text', text: 'hi' }], + source: { kind: 'user' }, + }))).toEqual([]) + }) + it('produces no update for boundary/other event types', () => { expect(updatesFor(evt('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }))).toEqual([]) expect(updatesFor(evt('turn/end', { turn: 1, reason: { kind: 'completed' } }))).toEqual([]) From 513e16f9dced1d5590463a68302cd59fd5427b91 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:26:44 +0800 Subject: [PATCH 06/16] fix(dev): make hooks and bash seams safer --- docs/development.md | 4 ++-- package.json | 2 +- packages/bash-local/src/index.ts | 10 ++++++++++ packages/bash-local/tests/executor.spec.ts | 10 ++++++++++ packages/bash/README.md | 2 +- packages/tool-bash/README.md | 4 ++-- packages/tool-bash/src/index.ts | 4 ++-- scripts/install-lefthook.mjs | 13 +++++++++++++ 8 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 scripts/install-lefthook.mjs diff --git a/docs/development.md b/docs/development.md index 144860a4ff..45d23912ca 100644 --- a/docs/development.md +++ b/docs/development.md @@ -17,12 +17,12 @@ Install dependencies from the repo root: pnpm install ``` -The install also runs the root `postinstall` script, which installs lefthook from the repo dev dependency. +The install also runs the root `postinstall` script, which installs lefthook from the repo dev dependency through `scripts/install-lefthook.mjs`; the wrapper uses lefthook's reviewed `--force` mode so linked worktrees with an existing `core.hooksPath` do not fail normal `pnpm run …` commands. If hooks are missing because dependencies were restored from cache or `postinstall` was skipped, install them manually: ```sh -pnpm exec lefthook install +pnpm exec lefthook install --force ``` Run typecheck once after a fresh clone: diff --git a/package.json b/package.json index 8f3c5fc946..227c69c684 100644 --- a/package.json +++ b/package.json @@ -32,7 +32,7 @@ "demo:echo": "node --expose-internals --import tsx examples/echo-agent/start.ts", "demo:coding": "node --expose-internals --import tsx examples/coding-agent/start.ts", "demo:acp": "node --expose-internals --import tsx examples/acp-agent/start.ts", - "postinstall": "lefthook install" + "postinstall": "node scripts/install-lefthook.mjs" }, "devDependencies": { "@agentclientprotocol/sdk": "0.25.1", diff --git a/packages/bash-local/src/index.ts b/packages/bash-local/src/index.ts index 1169160614..7320276a1a 100644 --- a/packages/bash-local/src/index.ts +++ b/packages/bash-local/src/index.ts @@ -38,6 +38,12 @@ export interface Config { /** The shape after schemastery applied the defaults (cwd has none). */ type ResolvedConfig = Required> & Pick +function assertPositiveFinite(name: string, value: number): void { + if (!Number.isFinite(value) || value <= 0) { + throw new Error(`bash-local: ${name} must be a positive finite number`) + } +} + interface TrackedTask extends BashTask { running: RunningBash /** Whole-stream byte offsets already delivered via {@link LocalBashExecutor.readOutput}. */ @@ -72,6 +78,9 @@ export class LocalBashExecutor extends BashExecutor { // schemastery (static Config) has already filled the defaulted fields; // the cast records that runtime fact for exactOptionalPropertyTypes. this.config = config as ResolvedConfig + assertPositiveFinite('timeoutMs', this.config.timeoutMs) + assertPositiveFinite('maxTimeoutMs', this.config.maxTimeoutMs) + assertPositiveFinite('maxOutputBytes', this.config.maxOutputBytes) ctx.effect(() => async () => { // Kill every live process group and WAIT for the processes to close so // nothing outlives the fiber (HMR safety) — a TERM-trapping child is @@ -98,6 +107,7 @@ export class LocalBashExecutor extends BashExecutor { * values and never re-default. */ resolve(request: BashExecRequest): BashExecSpec { + if (request.timeoutMs !== undefined) assertPositiveFinite('request.timeoutMs', request.timeoutMs) const timeoutMs = Math.min(request.timeoutMs ?? this.config.timeoutMs, this.config.maxTimeoutMs) return { command: request.command, diff --git a/packages/bash-local/tests/executor.spec.ts b/packages/bash-local/tests/executor.spec.ts index 72383d0427..3dd7f7983a 100644 --- a/packages/bash-local/tests/executor.spec.ts +++ b/packages/bash-local/tests/executor.spec.ts @@ -59,6 +59,16 @@ describe('LocalBashExecutor.run', () => { expect(result.timeoutMs).toBe(2_000) }) + it('rejects invalid numeric config and timeout overrides', async () => { + await expect(setup({ timeoutMs: Number.NaN })).rejects.toThrow(/timeoutMs/) + await expect(setup({ maxTimeoutMs: 0 })).rejects.toThrow(/maxTimeoutMs/) + await expect(setup({ maxOutputBytes: -1 })).rejects.toThrow(/maxOutputBytes/) + + const { bash } = await setup() + expect(() => bash.resolve({ command: 'true', timeoutMs: Number.NaN })).toThrow(/request\.timeoutMs/) + expect(() => bash.resolve({ command: 'true', timeoutMs: -1 })).toThrow(/request\.timeoutMs/) + }) + it('per-call timeout takes precedence under the cap and kills on expiry', async () => { const { bash } = await setup({ timeoutMs: 60_000 }) const result = await bash.run(bash.resolve({ command: 'sleep 60', timeoutMs: 100 })) diff --git a/packages/bash/README.md b/packages/bash/README.md index 12976d551b..8de529fee1 100644 --- a/packages/bash/README.md +++ b/packages/bash/README.md @@ -27,4 +27,4 @@ Implementations subclass `BashExecutor`, implement the abstract methods, and cal ## Vocabulary -`BashExecSpec` (command, workdir?, timeoutMs?, signal?) → `BashRunResult` (exitCode, signal, timedOut, aborted, timeoutMs, stdout/stderr as `CollectedOutput`) and `BashTask`/`BashTaskRead` for the background side. See `src/types.ts` for the full contracts. +`BashExecRequest` (command, workdir?, timeoutMs?, signal?) resolves to `BashExecSpec` (command, workdir, timeoutMs, signal?) before execution; `run()` returns `BashRunResult` (exitCode, signal, timedOut, aborted, timeoutMs, stdout/stderr as `CollectedOutput`) and `start()`/`readOutput()` use `BashTask`/`BashTaskRead` for the background side. See `src/types.ts` for the full contracts. diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index 83f84e967d..e60e52542b 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -12,7 +12,7 @@ Requires a loaded executor implementation (e.g. `@deepseek-ai/dsh-bash-local`); |---|---|---| | `command` | string (required) | Run via `bash -c`. No state persists between calls — use `workdir`, not `cd`. | | `description` | string (required) | One-line, active-voice summary of the command (5-10 words), for UI/log display only — no effect on execution. | -| `timeoutMs` | number | Default/max from executor config (120s/600s for bash-local). | +| `timeoutMs` | number | Timeout override in milliseconds. The executor applies its configured default and cap. | | `workdir` | string | Working directory for this call. Defaults to the calling agent's session cwd (`session.header.cwd`) so each session runs in its own workspace; a relative `workdir` is resolved against that session cwd. | | `run_in_background` | boolean | Return a task id immediately; no timeout applies. | @@ -26,7 +26,7 @@ Result text: stdout, then a `[stderr]` section, then status markers — `[timed ### `bash_kill` -`task_id` → SIGTERM→SIGKILL on the task's process group. Killing an already-finished task is a reported no-op; unknown ids are errors. +`task_id` → ask the executor to kill the background task. The concrete executor decides how to signal or stop the process; killing an already-finished task is a reported no-op, and unknown ids are errors. ### Task ownership (cross-session isolation) diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index f4b0559a0b..33a97ddf93 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -213,7 +213,7 @@ export function apply(ctx: Context): void { + '5-10 words (shown in the UI). Examples: "ls" → "List files in current directory"; ' + '"git status" → "Show working tree status"; "npm install" → "Install package dependencies".', }, - timeoutMs: { type: 'number', description: 'Timeout in milliseconds (default 120000, max 600000). The command is killed on expiry.' }, + timeoutMs: { type: 'number', description: 'Timeout in milliseconds. The executor applies its configured default and cap, and kills the command on expiry.' }, workdir: { type: 'string', description: 'Working directory for this command. Defaults to the session workspace; a relative path is resolved against it.' }, run_in_background: { type: 'boolean', description: 'Run in the background and return a task id immediately. No timeout applies.' }, }, @@ -269,7 +269,7 @@ export function apply(ctx: Context): void { ctx.tools.register(defineTool({ name: 'bash_kill', - description: 'Kill a running background bash task (SIGTERM, then SIGKILL) by task id.', + description: 'Ask the executor to kill a running background bash task by task id.', parameters: { task_id: { type: 'string', required: true, description: 'Task id returned by the bash tool.' }, }, diff --git a/scripts/install-lefthook.mjs b/scripts/install-lefthook.mjs new file mode 100644 index 0000000000..be81daa153 --- /dev/null +++ b/scripts/install-lefthook.mjs @@ -0,0 +1,13 @@ +#!/usr/bin/env node +import { existsSync } from 'node:fs' +import { spawnSync } from 'node:child_process' +import { join } from 'node:path' + +const git = spawnSync('git', ['rev-parse', '--git-dir'], { stdio: 'ignore' }) +if (git.status !== 0) process.exit(0) + +const lefthook = join(process.cwd(), 'node_modules', '.bin', process.platform === 'win32' ? 'lefthook.cmd' : 'lefthook') +if (!existsSync(lefthook)) process.exit(0) + +const result = spawnSync(lefthook, ['install', '--force'], { stdio: 'inherit' }) +process.exit(result.status ?? 1) From 7a5886da2d02c9be768adb8a724e881adb5999b9 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:26:57 +0800 Subject: [PATCH 07/16] docs(rfc): record follow-up cleanup seams --- docs/adr/0002-microkernel-event-taxonomy.md | 2 +- docs/adr/0014-doc-sync-enforcement.md | 11 ++++---- docs/adr/README.md | 2 +- docs/architecture.md | 2 +- ...09-session-persistence-and-resumability.md | 2 ++ docs/rfc/011-acp-multi-session.md | 2 ++ ...014-agent-lifecycle-and-ownership-seams.md | 26 +++++++++++++++++++ ...15-shared-persistence-write-coordinator.md | 24 +++++++++++++++++ docs/rfc/README.md | 4 ++- packages/AGENTS.md | 2 +- packages/README.md | 4 +-- 11 files changed, 68 insertions(+), 13 deletions(-) create mode 100644 docs/rfc/014-agent-lifecycle-and-ownership-seams.md create mode 100644 docs/rfc/015-shared-persistence-write-coordinator.md diff --git a/docs/adr/0002-microkernel-event-taxonomy.md b/docs/adr/0002-microkernel-event-taxonomy.md index 900fed23d1..c2b3889b2e 100644 --- a/docs/adr/0002-microkernel-event-taxonomy.md +++ b/docs/adr/0002-microkernel-event-taxonomy.md @@ -14,7 +14,7 @@ Pure Cordis event taxonomy. The loop's extension seams are typed events with del - **emit** (sync fire-and-forget) for notifications: turn/step boundaries, stream chunks, lifecycle, errors. - **parallel** (awaited) for the one durability checkpoint: `session/flush`. -The event vocabulary lives in interface packages (dsh-agent declares the agent/* events); `@deepseek-ai/dsh-agent-loop` is the only concrete plugin and is itself swappable — nothing outside it may depend on it. +The event vocabulary lives in interface packages (dsh-agent declares the agent/* events); `@deepseek-ai/dsh-agent-loop` is the only concrete loop plugin and is itself swappable — nothing outside it may depend on it. ## Consequences diff --git a/docs/adr/0014-doc-sync-enforcement.md b/docs/adr/0014-doc-sync-enforcement.md index 2187249c43..eebd03615a 100644 --- a/docs/adr/0014-doc-sync-enforcement.md +++ b/docs/adr/0014-doc-sync-enforcement.md @@ -1,21 +1,20 @@ -# ADR 0014: Doc-sync enforcement +# ADR 0014: Doc-sync enforcement and markdown wrap verification Status: accepted (2026-06-14) ## Context -AGENTS.md promises that docs and code stay strictly in sync, but the promise was verified by eyeball. Review caught drift twice — a cookbook example contradicting the type policy, and a README citing the wrong `registerAdapter` call. Out-of-sync docs are worse than no docs, and this codebase is built primarily by agents that follow gates far more reliably than prose (ADR 0007). Two classes of doc drift are mechanically checkable: code blocks that no longer compile, and the event-taxonomy table that duplicates the `interface Events` declarations. +AGENTS.md promises that docs and code stay strictly in sync, but the promise was verified by eyeball. Review caught drift twice — a cookbook example contradicting the type policy, and a README citing the wrong `registerAdapter` call. Out-of-sync docs are worse than no docs, and this codebase is built primarily by agents that follow gates far more reliably than prose (ADR 0007). Three classes are mechanically checkable: code blocks that no longer compile, the event-taxonomy table that duplicates the `interface Events` declarations, and hard-wrapped Markdown prose that violates the repo's one-line-per-paragraph convention. ## Decision -Two gates, mirroring the existing `scripts/` style (tsx ESM, one job each): +Three gates, mirroring the existing `scripts/` style (tsx ESM, one job each): 1. **`doc-typecheck`** extracts every fenced ` ```ts ` block from `README.md`, `docs/**`, and `packages/*/README.md`, writes them to a temp project, and compiles with `tsc --noEmit`. The temp tsconfig copies only resolution-relevant options and the workspace `paths` map from `tsconfig.typecheck.json` (vendor → built `lib`, harness → `src`) — resolving vendor to `lib` is essential, or tsc type-checks raw vendor source and floods the run. A block that is a deliberate sketch opts out with an explicit ` ```ts ignore-check ` info string; the script reports the opt-out ratio and fails if it exceeds half, so the escape hatch can't quietly become the norm. 2. **`verify-event-taxonomy`** extracts the event names from the `interface Events` blocks across `packages/*/src` and from the taxonomy table in `docs/architecture.md`, and asserts the two sets match exactly. Verify, don't generate: the table keeps its hand-written Mode/Purpose columns; only the set of names is checked. (Landing this surfaced three events the table had been missing — `tools/change`, `llm/adapter-change`, `system-prompt/change`.) +3. **`verify-md-wrap`** parses each in-scope Markdown file (`README.md`, `docs/**`, `packages/*/README.md`, plus `AGENTS.md` / `packages/AGENTS.md`) with `mdast-util-from-markdown` + GFM and fails on any `paragraph` node spanning more than one source line, enforcing the AGENTS.md "Markdown is not hard-wrapped" convention. Same verify-don't-generate principle: it reports hard-wraps and never rewrites, so it adds no formatting churn. -Both run via a shared `doc-sync` package.json script that the lefthook pre-push hook and CI both invoke (ADR 0007: hooks and CI call the same scripts, so the gate fires locally before a push — not only after it). They run after `pnpm run typecheck` (which emits the vendor `lib/` that doc-typecheck resolves against). API-extractor golden reports (RFC 006 part 3) were deliberately **deferred** — low value for an internal monorepo where reviewers already see the source diff, and a heavy, finicky dependency. - -**Amendment (2026-06-17):** a third gate, **`verify-md-wrap`**, was later folded into `doc-sync`. It parses each in-scope Markdown file (`README.md`, `docs/**`, `packages/*/README.md`, plus `AGENTS.md` / `packages/AGENTS.md`) with `mdast-util-from-markdown` + GFM and fails on any `paragraph` node spanning more than one source line, enforcing the AGENTS.md "Markdown is not hard-wrapped" convention. Same verify-don't-generate principle: it reports hard-wraps and never rewrites, so it adds no formatting churn. `doc-sync` is now three gates. +All three run via a shared `doc-sync` package.json script that the lefthook pre-push hook and CI both invoke (ADR 0007: hooks and CI call the same scripts, so the gate fires locally before a push — not only after it). They run after `pnpm run typecheck` (which emits the vendor `lib/` that doc-typecheck resolves against). API-extractor golden reports (RFC 006 part 3) were deliberately **deferred** — low value for an internal monorepo where reviewers already see the source diff, and a heavy, finicky dependency. ## Consequences diff --git a/docs/adr/README.md b/docs/adr/README.md index d9671c6652..d59509d94e 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -25,7 +25,7 @@ Do NOT write an ADR for: a mechanical or local choice (a variable name, a one-fi | [0011](0011-runtime-arg-validation.md) | Runtime arg validation at the model boundary | accepted | | [0012](0012-dev-invariants-over-deep-readonly.md) | Dev-mode invariants over compile-time deep-readonly | accepted | | [0013](0013-property-based-testing.md) | Property-based testing for protocol-shaped code | accepted | -| [0014](0014-doc-sync-enforcement.md) | Doc-sync enforcement (doc code blocks + event taxonomy) | accepted | +| [0014](0014-doc-sync-enforcement.md) | Doc-sync enforcement and markdown wrap verification | accepted | | [0015](0015-structured-error-taxonomy.md) | Structured error taxonomy (HarnessError base) | accepted | | [0016](0016-pnpm-over-yarn.md) | pnpm as the package manager instead of Yarn 4 | accepted | | [0017](0017-turn-enclosure-invariant.md) | Every session event is enclosed in a turn | accepted | diff --git a/docs/architecture.md b/docs/architecture.md index fb70e88c2e..acf7fd7081 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -4,7 +4,7 @@ This document describes the phase-1 architecture of the DeepSeek Harness — the > **Microkernel approach. Everything is a plugin.** -The harness core is deliberately tiny: a handful of abstract services plus one concrete plugin (the agent loop). Every product feature — tools, hooks, compaction, sandboxing, UI, persistence, sub-agents, MCP, skills — is meant to be written as a plugin against the extension surface described here, without modifying the loop. +The harness core is deliberately tiny: a handful of abstract services plus one concrete loop plugin (`dsh-agent-loop`). Every product feature — tools, hooks, compaction, sandboxing, UI, persistence, sub-agents, MCP, skills — is meant to be written as a plugin against the extension surface described here, without modifying the loop. Requirement context: [Coding Harness MVP 需求分析][mvp-doc]. diff --git a/docs/rfc/009-session-persistence-and-resumability.md b/docs/rfc/009-session-persistence-and-resumability.md index 80469feee3..a29d9af1af 100644 --- a/docs/rfc/009-session-persistence-and-resumability.md +++ b/docs/rfc/009-session-persistence-and-resumability.md @@ -2,6 +2,8 @@ Status: implemented (see [ADR 0018](../adr/0018-session-persistence.md)) +> **Historical proposal note:** this RFC is preserved as the design trail. The implemented crash-recovery semantics are the ADR 0018 version: load preserves an interrupted final turn and durably closes it with synthetic boundary events instead of truncating back to the last `turn/end`; both JSONL and SQLite backends now implement that contract. + ## Problem Sessions live only in memory. The example `session-jsonl.ts` plugin (duplicated byte-for-byte in both `examples/coding-agent` and `examples/echo-agent`) is write-only telemetry: it buffers `session/event` and appends JSON lines, but has no read/replay path, no crash-safety (no fsync, no atomic write, and a fire-and-forget dispose drain), no listing, and no format versioning. [ADR 0003](../adr/0003-event-sourced-sessions.md) and [docs/architecture.md](../architecture.md) both park "real persistence backends (JSONL session dirs, sqlite)" and the session-event-vocabulary review as deferred TODOs "once the loop and the first persistence plugin coexist" — that time is now. diff --git a/docs/rfc/011-acp-multi-session.md b/docs/rfc/011-acp-multi-session.md index 77bbc7b664..508b0b65b2 100644 --- a/docs/rfc/011-acp-multi-session.md +++ b/docs/rfc/011-acp-multi-session.md @@ -8,6 +8,8 @@ Status: proposed RFC 010 ships ACP support with a single active session per connection: a second `session/new` is rejected. Editors expect to run several conversations over one agent subprocess — a user opens multiple threads, or a client pre-warms sessions. The single-session guard is a deliberate MVP scope cut, not an architectural limit; this RFC lifts it. +This paragraph is historical: the multi-session bridge has landed. The remaining proposed work is per-session permission ownership plus the lifecycle seams now tracked in [RFC 014](014-agent-lifecycle-and-ownership-seams.md). + ## Proposal The harness core already supports many agents (`AgentRegistry.list()` and `AgentLoop.create` impose no count limit), so multiplexing is a bridge-layer change in `@deepseek-ai/dsh-acp`, not a loop or core change. diff --git a/docs/rfc/014-agent-lifecycle-and-ownership-seams.md b/docs/rfc/014-agent-lifecycle-and-ownership-seams.md new file mode 100644 index 0000000000..572edd0aff --- /dev/null +++ b/docs/rfc/014-agent-lifecycle-and-ownership-seams.md @@ -0,0 +1,26 @@ +# RFC 014: Agent lifecycle and ownership seams + +Status: proposed + +## Problem + +Several ACP and tool-bash limitations are symptoms of the same missing seam: plugins can create or resume agents through `ctx.agents`, but they cannot own and dispose one agent independently, and long-running bash tasks carry no stable owner in the executor itself. ACP currently aborts and awaits agents on disconnect, but cannot unregister just that session's agent; `session/cancel` cannot cancel queued-but-not-yet-started work; and `tool-bash` keeps task ownership in a plugin-local `Map`, so an HMR reload can make an old task look unowned. + +## Proposal + +Add explicit lifecycle ownership to the agent factory and explicit ownership metadata to background tasks. + +1. `ctx.agents.create/resume` should return an `AgentHandle` (or add an adjacent method) that exposes the `Agent` plus an async disposer. The disposer unregisters the agent, aborts queued/running work, and resolves only when the driver loop reaches quiescence. +2. Add a queue-aware cancel primitive to the `Agent` interface. It must clear queued work that has not started, abort the current step if one exists, and make `whenIdle()` wait for the post-cancel quiescent state. ACP `session/cancel` and bridge teardown then become honest cancellation, not best-effort pre-step cancellation. +3. Move background task ownership into the bash seam. `BashExecSpec` or `BashTask` should carry a stable owner token, preferably the session id rather than the `Agent` object identity. `bash_output`/`bash_kill` then ask the executor for ownership rather than relying on a `tool-bash` instance-local map. + +## Acceptance Criteria + +- ACP disconnect/session close leaves no registered agent for that session, even when `session/load` races teardown. +- `session/cancel` before a queued prompt starts prevents that prompt from running and cannot batch the next prompt into the cancelled turn. +- A `tool-bash` HMR reload does not make an existing background task readable or killable by a different session. +- Existing non-ACP demos still work without managing handles explicitly; config-created agents remain owned by the `AgentLoop` plugin fiber. + +## Risks + +This touches public interfaces (`Agent`, `AgentFactory`, and the bash seam), so it should not be smuggled into a local ACP patch. The compatibility trap is preserving the simple synchronous `Agent.send()` ergonomics while adding a robust async lifecycle path for owners that need it. diff --git a/docs/rfc/015-shared-persistence-write-coordinator.md b/docs/rfc/015-shared-persistence-write-coordinator.md new file mode 100644 index 0000000000..96fb590681 --- /dev/null +++ b/docs/rfc/015-shared-persistence-write-coordinator.md @@ -0,0 +1,24 @@ +# RFC 015: Shared persistence write coordinator + +Status: proposed + +## Problem + +`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration is now duplicated: per-session state, `session/created` adoption, seed-prefix collision checks, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. That code is correctness-heavy and already receives the same fixes twice. + +## Proposal + +Extract a backend-agnostic coordinator into `dsh-session-persistence`. The coordinator owns live-session adoption, buffering, cursor filtering, per-id serialization, and disposal quiescence. Concrete backends provide small hooks for durable operations: create lazy state, find/load stored prefix, append a contiguous batch, update summary, delete, and list. + +The public `SessionPersistence` service shape can stay the same. The coordinator can be an internal exported helper or protected base class used by first-party backends; third-party backends may still implement the abstract service directly if their write path is different. + +## Acceptance Criteria + +- JSONL and SQLite keep passing the existing shared `runPersistenceContract`. +- HMR/adoption/collision tests move to a shared coordinator test suite and run once for each backend through hook-driven fixtures. +- Backend-specific tests focus on storage mechanics only: JSONL path safety/fsync/sidecar behavior and SQLite schema/WAL/transaction behavior. +- A future backend does not need to copy the current `session/event` → buffer → flush orchestration. + +## Risks + +The current duplication is verbose but explicit. A coordinator must not hide storage-specific durability semantics or make unusual backends fight an inheritance hierarchy. Prefer narrow hooks and contract tests over a large framework. diff --git a/docs/rfc/README.md b/docs/rfc/README.md index dbd1468489..b224ac4a2f 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -12,8 +12,10 @@ Proposals for substantial future work — reviewed before implementation, unlike | [006](006-doc-sync-and-api-reports.md) | Doc-sync enforcement and API extractor reports | implemented (pts 1-2; pt 3 deferred) | | [007](007-supply-chain-and-vendor-drift.md) | Supply chain checks and vendor drift verification | proposed | | [008](008-immutable-public-surfaces.md) | Deep-readonly public surfaces | implemented (revised) | -| [009](009-session-persistence-and-resumability.md) | Durable session persistence — abstract, append-only, event-based store | proposed | +| [009](009-session-persistence-and-resumability.md) | Durable session persistence — abstract, append-only, event-based store | implemented | | [010](010-acp-agent-client-protocol.md) | Agent Client Protocol (ACP) support for external editors | proposed | | [011](011-acp-multi-session.md) | Multiplex concurrent ACP sessions over one connection | proposed | | [012](012-optional-code-mode.md) | Optional Code Mode — model writes TypeScript against an SDK of all tools | proposed | | [013](013-typed-event-schemas.md) | Runtime schemas for the event vocabulary (Zod vs the merge-extensible-map pattern) | proposed | +| [014](014-agent-lifecycle-and-ownership-seams.md) | Agent lifecycle and ownership seams | proposed | +| [015](015-shared-persistence-write-coordinator.md) | Shared persistence write coordinator | proposed | diff --git a/packages/AGENTS.md b/packages/AGENTS.md index 4dead05337..494b682482 100644 --- a/packages/AGENTS.md +++ b/packages/AGENTS.md @@ -11,6 +11,6 @@ Naming notes: - Files `src/index.ts` export the service default + all public types - `src/types.ts` contain only types — no runtime code - Tests live at package level under `tests/`, not `src/__tests__/` -- A package's README and module/JSDoc comments are part of the change: when you alter behavior (config keys, defaults, error codes, wire fields), update them in the same commit. CI runs `pnpm run doc-sync`, which typechecks fenced `ts` blocks in `packages/*/README.md` and verifies the event-taxonomy table — but it does NOT cover this file or prose drift (config keys, defaults, error codes), so those stay on the author. +- A package's README and module/JSDoc comments are part of the change: when you alter behavior (config keys, defaults, error codes, wire fields), update them in the same commit. CI runs `pnpm run doc-sync`, which typechecks fenced `ts` blocks in `packages/*/README.md`, verifies the event-taxonomy table, and checks markdown wrapping across this file too — but it does NOT catch prose drift (config keys, defaults, error codes), so those stay on the author. Read the per-package README.md for package-specific details: service API, events, extension points, TODOs. diff --git a/packages/README.md b/packages/README.md index 8ba41bc322..0a68ffc917 100644 --- a/packages/README.md +++ b/packages/README.md @@ -1,6 +1,6 @@ # Packages -Harness packages, all under the `@deepseek-ai/dsh-*` scope. Each package is a Cordis service (microkernel plugin-style): it exports a default `Service` class that gets registered via `ctx.plugin()`, declares its ctx key and events through declaration merging, and exposes extension points through `ctx.effect()`, `ctx.on()`, and `ctx.waterfall()`. +Harness packages, all under the `@deepseek-ai/dsh-*` scope. Each package is a Cordis plugin (microkernel-style): it exports either a default `Service` subclass or a functional plugin that gets registered via `ctx.plugin()`, declares its ctx key/events where applicable through declaration merging, and exposes extension points through `ctx.effect()`, `ctx.on()`, and `ctx.waterfall()`. ## Dependency graph @@ -31,7 +31,7 @@ The rule: plugins depend on interfaces, never on the concrete loop. `dsh-agent-l | `system-prompt/` | Prompt-section + tool-schema assembly registry | `ctx.systemPrompt` | | `tools/` | Tool registry + `tools/execute` waterfall | `ctx.tools` | | `agent/` | Agent interface, registry, `agent/*` event vocabulary | `ctx.agents` | -| `agent-loop/` | THE concrete plugin: `LoopAgent` + the loop driver | `ctx.agentLoop` | +| `agent-loop/` | THE concrete loop plugin: `LoopAgent` + the loop driver | `ctx.agentLoop` | | `bash/` | Abstract bash executor seam (interface + vocabulary) | `ctx.bash` | | `bash-local/` | Local-subprocess `BashExecutor` implementation | (registers `ctx.bash`) | | `tool-bash/` | Model-facing `bash`/`bash_output`/`bash_kill` tool schemas | (registers on `ctx.tools`) | From 0b036d808ce016448e89dc93ba312087fb3cd7eb Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:31:54 +0800 Subject: [PATCH 08/16] test: cover new defensive branches --- packages/llm-pi-ai/src/adapter.ts | 14 +++++++++++--- packages/llm-pi-ai/tests/adapter.spec.ts | 1 + .../tests/jsonl.spec.ts | 8 ++++++++ packages/tools/tests/tools.spec.ts | 18 +++++++++++++++++- 4 files changed, 37 insertions(+), 4 deletions(-) diff --git a/packages/llm-pi-ai/src/adapter.ts b/packages/llm-pi-ai/src/adapter.ts index 1a05f6d65f..38b05dc007 100644 --- a/packages/llm-pi-ai/src/adapter.ts +++ b/packages/llm-pi-ai/src/adapter.ts @@ -85,6 +85,7 @@ function strictByToolName(tools: ToolSchema[] | undefined): Map { }) it.each([ + [400, 'INVALID_REQUEST'], [429, 'RATE_LIMIT'], [500, 'SERVER'], ] as const)('maps HTTP %s to stable error code %s', async (status, code) => { diff --git a/packages/session-persistence-jsonl/tests/jsonl.spec.ts b/packages/session-persistence-jsonl/tests/jsonl.spec.ts index cc463f78f7..9d22bf2dae 100644 --- a/packages/session-persistence-jsonl/tests/jsonl.spec.ts +++ b/packages/session-persistence-jsonl/tests/jsonl.spec.ts @@ -1039,6 +1039,14 @@ describe('SessionPersistenceJsonl: edge cases', () => { expect(loaded.meta.updatedAt).toBe(5) }) + it('load rejects a corrupt sidecar instead of treating it as absent', async () => { + const m = meta('bad-sidecar') + await ctx.sessionPersistence.create(m) + await ctx.sessionPersistence.append(m.id, oneTurnLog()) + await writeFile(sidecarPath(root, undefined, m.id), '{not json') + await expect(ctx.sessionPersistence.load(m.id)).rejects.toThrow() + }) + it('list returns nothing when the root directory does not exist', async () => { const ctx2 = new Context() await ctx2.plugin(SessionStore) diff --git a/packages/tools/tests/tools.spec.ts b/packages/tools/tests/tools.spec.ts index ac2fd8414c..9137631524 100644 --- a/packages/tools/tests/tools.spec.ts +++ b/packages/tools/tests/tools.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, expectTypeOf, it } from 'vitest' import { Context } from 'cordis' -import { CallId } from '@deepseek-ai/dsh-llm' +import { CallId, HarnessError } from '@deepseek-ai/dsh-llm' import SystemPrompt from '@deepseek-ai/dsh-system-prompt' import ToolRegistry, { defineTool, schemaSpecToJsonSchema, validateArgs, ToolArgsError, ToolNotFoundError, @@ -138,6 +138,22 @@ describe('ToolRegistry', () => { }) }) + it('preserves structured error info when a tools/execute listener throws HarnessError', async () => { + const ctx = await setup() + ctx.tools.register(echoTool) + ctx.on('tools/execute', async () => { + throw new HarnessError('denied', 'DENIED') + }) + + const result = await ctx.tools.execute({ callId: CallId('c1'), name: 'echo', arguments: { text: 'hi' } }) + + expect(result).toMatchObject({ + callId: CallId('c1'), + isError: true, + error: { name: 'HarnessError', code: 'DENIED' }, + }) + }) + it('schemas() snapshots tool schemas instead of exposing registry objects', async () => { const ctx = await setup() ctx.tools.register(echoTool) From 4a3f3af296212d7275fbb5f80e37c3b1428c4551 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 00:37:12 +0800 Subject: [PATCH 09/16] Address Claude review follow-ups --- examples/acp-agent/start.ts | 2 ++ packages/acp/src/index.ts | 21 ++++++++---- packages/acp/tests/load.spec.ts | 9 ++++- packages/agent-loop/src/loop.ts | 9 +++++ packages/agent-loop/tests/loop.spec.ts | 34 ++++++++++++++++++- .../session-persistence-sqlite/src/index.ts | 16 +++++++-- packages/system-prompt/src/index.ts | 9 +++-- packages/tools/src/index.ts | 11 +++--- 8 files changed, 92 insertions(+), 19 deletions(-) diff --git a/examples/acp-agent/start.ts b/examples/acp-agent/start.ts index 7c21ed9262..61a7241c80 100644 --- a/examples/acp-agent/start.ts +++ b/examples/acp-agent/start.ts @@ -18,6 +18,8 @@ try { // ENOENT (no .env) is fine — rely on the ambient environment. } +// Resolve relative cordis.yml paths from the repo root no matter where the +// editor launches this demo command. process.chdir(fileURLToPath(new URL('../..', import.meta.url))) const ctx = new Context() diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index c24b294d32..97a3d5b0a2 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -100,6 +100,10 @@ function internalError(detail: string): RequestError { return RequestError.internalError(undefined, detail) } +function sameWorkspaceCwd(left: string, right: string): boolean { + return resolvePath(left) === resolvePath(right) +} + /** Plugin config: the agent template ACP sessions are created from. */ export interface AcpConfig { /** Model name for created agents (must have a registered adapter). */ @@ -466,13 +470,16 @@ export function apply(ctx: Context, config: AcpConfig): void { // (An id unknown to `list()` falls through to resume, which rejects with // the backend's not-found error.) const meta = (await sessionPersistence.list()).find(m => m.id === params.sessionId) - if (meta !== undefined && (meta.cwd === undefined || !isAbsolute(meta.cwd))) { - throw invalidParams( - `session ${params.sessionId} has no absolute persisted cwd; cannot determine its workspace (it predates per-session cwd, or was created without one)`, - ) - } - if (meta !== undefined && meta.cwd !== params.cwd) { - throw invalidParams(`session ${params.sessionId} cwd mismatch: persisted ${meta.cwd}, requested ${params.cwd}`) + if (meta !== undefined) { + const persistedCwd = meta.cwd + if (persistedCwd === undefined || !isAbsolute(persistedCwd)) { + throw invalidParams( + `session ${params.sessionId} has no absolute persisted cwd; cannot determine its workspace (it predates per-session cwd, or was created without one)`, + ) + } + if (!sameWorkspaceCwd(persistedCwd, params.cwd)) { + throw invalidParams(`session ${params.sessionId} cwd mismatch: persisted ${persistedCwd}, requested ${params.cwd}`) + } } const agent = await agents.resume({ agentId: params.sessionId, diff --git a/packages/acp/tests/load.spec.ts b/packages/acp/tests/load.spec.ts index 55d56fa09c..f06c707d85 100644 --- a/packages/acp/tests/load.spec.ts +++ b/packages/acp/tests/load.spec.ts @@ -178,7 +178,7 @@ describe('acp bridge — session/load replay', () => { .rejects.toThrow(/cwd mismatch/) expect(loader.ctx.agents.get('elsewhere')).toBeUndefined() - const res = await loader.client.loadSession({ sessionId: 'elsewhere', cwd: otherCwd, mcpServers: [] }) + const res = await loader.client.loadSession({ sessionId: 'elsewhere', cwd: `${otherCwd}/.`, mcpServers: [] }) expect(res).toBeDefined() expect(loader.ctx.agents.get('elsewhere')!.session.header.cwd).toBe(otherCwd) }) @@ -190,6 +190,13 @@ describe('acp bridge — session/load replay', () => { .rejects.toThrow(/absolute/) }) + it('lets persistence reject a load for an unknown id after metadata lookup misses', async () => { + loader = await makeBridgeHarness({ storageDir, script: [] }) + await loader.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + await expect(loader.client.loadSession({ sessionId: 'missing', cwd: process.cwd(), mcpServers: [] })) + .rejects.toThrow(/Internal error/) + }) + it('rejects loading a persisted session that has NO cwd (would silently run in the launch dir)', async () => { // A legacy / externally-created session log with no header.cwd. The bridge // must reject the load rather than accept it and let bash silently fall back diff --git a/packages/agent-loop/src/loop.ts b/packages/agent-loop/src/loop.ts index 51618585b0..d1ea117811 100644 --- a/packages/agent-loop/src/loop.ts +++ b/packages/agent-loop/src/loop.ts @@ -502,6 +502,11 @@ async function runStep( if (stepError) throw stepError if (assembler.finish.kind === 'max-tokens') { + let message: Message = withoutToolCalls(assembler.message()) + message = withoutToolCalls(await ctx.waterfall('agent/step-result', agent, turn, step, message, () => Promise.resolve(message))) + if (message.content.length > 0) { + session.append('assistant/message', { turn, step, content: message.content }) + } if (assembler.usage) { session.append('usage', { turn, step, usage: assembler.usage }) } @@ -566,6 +571,10 @@ async function runStep( return { hadToolCalls: toolCalls.length > 0, finish: assembler.finish } } +function withoutToolCalls(message: Message): Message { + return { ...message, content: message.content.filter(block => block.type !== 'tool-call') } +} + /** The last turn number in a (possibly seeded) session log, or 0. */ export function lastTurnNumber(session: Session): number { const lastStart = session.events.findLast(event => event.type === 'turn/start') diff --git a/packages/agent-loop/tests/loop.spec.ts b/packages/agent-loop/tests/loop.spec.ts index 3f3f52c48e..c4f09a24bf 100644 --- a/packages/agent-loop/tests/loop.spec.ts +++ b/packages/agent-loop/tests/loop.spec.ts @@ -385,6 +385,10 @@ describe('agent loop', () => { expect(steps).toBe(2) expect(adapter.requests).toHaveLength(2) + expect(adapter.requests[1]!.messages).toEqual([ + { role: 'user', content: [{ type: 'text', text: 'go' }] }, + { role: 'assistant', content: [{ type: 'text', text: 'first half' }] }, + ]) expect(reasons).toEqual([{ kind: 'max-tokens' }]) }) @@ -436,10 +440,38 @@ describe('agent loop', () => { expect(executions).toBe(0) expect(agent.session.events.some(e => e.type === 'tool/call')).toBe(false) + expect(agent.session.deriveMessages()).toEqual([{ role: 'user', content: [{ type: 'text', text: 'go' }] }]) + expect(reasons).toEqual([{ kind: 'max-tokens' }]) + }) + + it('keeps safe max-tokens assistant content while dropping truncated tool calls', async () => { + const callId = CallId('c1') + const adapter = new MockAdapter([[ + { type: 'block-start', index: 0, blockType: 'text' }, + { type: 'text-delta', index: 0, text: 'partial text' }, + { type: 'block-end', index: 0, block: { type: 'text', text: 'partial text' } }, + { type: 'block-start', index: 1, blockType: 'tool-call' }, + { type: 'tool-call-delta', index: 1, id: callId, name: 'echo', argumentsDelta: '{"text"' }, + { type: 'finish', reason: { kind: 'max-tokens' } }, + ]]) + const ctx = await harness(adapter) + let stepResults = 0 + ctx.on('agent/step-result', async (_agent, _turn, _step, message, next) => { + stepResults += 1 + expect(message.content).toEqual([{ type: 'text', text: 'partial text' }]) + return next() + }) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + + send(agent, 'go') + await waitForIdle(ctx, agent) + + expect(stepResults).toBe(1) + expect(agent.session.events.some(e => e.type === 'tool/call')).toBe(false) expect(agent.session.deriveMessages()).toEqual([ { role: 'user', content: [{ type: 'text', text: 'go' }] }, + { role: 'assistant', content: [{ type: 'text', text: 'partial text' }] }, ]) - expect(reasons).toEqual([{ kind: 'max-tokens' }]) }) it('stops the turn when agent/step-end listener failure has recorded an error', async () => { diff --git a/packages/session-persistence-sqlite/src/index.ts b/packages/session-persistence-sqlite/src/index.ts index c791231221..7f59e3dd8a 100644 --- a/packages/session-persistence-sqlite/src/index.ts +++ b/packages/session-persistence-sqlite/src/index.ts @@ -448,6 +448,7 @@ export class SessionPersistenceSqlite extends SessionPersistence { // Dispose must reach quiescence: await every init + final drain, then close // the database, BEFORE returning, so no write lands after teardown. ctx.effect(() => async () => { + let disposeError: unknown try { const errors = [ ...await settledErrors(this.inits.values()), @@ -457,9 +458,20 @@ export class SessionPersistenceSqlite extends SessionPersistence { if (errors.length > 0) { throw new AggregateError(errors, 'session-persistence-sqlite dispose failed') } + } catch (error: unknown) { + disposeError = error + throw error } finally { - await this.ready - this.db.close() + try { + await this.ready + this.db.close() + } catch (error: unknown) { + /* v8 ignore next -- open/close failure racing disposal is a defensive teardown edge */ + if (disposeError === undefined) throw error + // Opening/closing the database can only add teardown context here; keep + // the already-captured init/flush/chain AggregateError as the primary + // disposal failure instead of masking it from callers. + } } }, 'session-persistence-sqlite write path') diff --git a/packages/system-prompt/src/index.ts b/packages/system-prompt/src/index.ts index b2f3bd3db4..25d87d2194 100644 --- a/packages/system-prompt/src/index.ts +++ b/packages/system-prompt/src/index.ts @@ -116,9 +116,12 @@ export class SystemPrompt extends Service { /** * Assemble the current prompt (sections sorted by order, tools collected - * from all providers). Runs through the `system-prompt/assemble` waterfall, - * giving listeners the opportunity to mutate or replace the assembly before - * it reaches the model. Await the result before reading the assembly values — + * from all providers). Section records are top-level clones (the `text` + * provider may be a function and is intentionally shared); tool schemas are + * deep-cloned because adapters and request waterfalls may mutate schema + * objects. Runs through the `system-prompt/assemble` waterfall, giving + * listeners the opportunity to mutate or replace the assembly before it + * reaches the model. Await the result before reading the assembly values — * waterfall listeners may be async. */ assemble(): Promise { diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index aa2d5dccaf..eee77445eb 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -332,11 +332,12 @@ export class ToolRegistry extends Service { } /** - * Execute one tool call through the `tools/execute` waterfall. If the tool - * is not registered, the result is an `isError` carrying a `UNKNOWN_TOOL` - * structured error. If the tool throws, the error is caught and returned as - * an `isError` result so the loop never sees an uncaught exception; a thrown - * {@link HarnessError} surfaces its `{ name, code }` on the result. + * Execute one tool call through the `tools/execute` waterfall. If the tool is + * not registered, the result is an `isError` carrying a `UNKNOWN_TOOL` + * structured error. If the tool or a waterfall listener throws, the error is + * caught and returned as an `isError` result so the loop records a failed tool + * call instead of failing the whole turn; a thrown {@link HarnessError} + * surfaces its `{ name, code }` on the result. */ async execute(exec: ToolExecution): Promise { try { From 711245821bf51ddc786e5fdd96b16a85dd15e799 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 01:31:56 +0800 Subject: [PATCH 10/16] Handle corrupt JSONL sidecars during list --- packages/invariants/src/index.ts | 4 ++++ .../session-persistence-jsonl/src/index.ts | 23 ++++++++++++++----- .../tests/jsonl.spec.ts | 21 +++++++++++++++++ 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/packages/invariants/src/index.ts b/packages/invariants/src/index.ts index 254b1282c7..b201e1ae43 100644 --- a/packages/invariants/src/index.ts +++ b/packages/invariants/src/index.ts @@ -118,6 +118,9 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { if (trace.openTurn !== null) { throw new InvariantError(`turn/start ${event.data.turn} while turn ${trace.openTurn} is still open`) } + // Current sessions replay full logs, so numbering starts at 1 and remains + // contiguous. If a future compaction/fork stores a partial log, it must + // seed `nextTurn` from retained metadata before this check runs. if (event.data.turn !== trace.nextTurn) { throw new InvariantError(`turn/start expected turn ${trace.nextTurn}, got ${event.data.turn}`) } @@ -143,6 +146,7 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { if (trace.openStep !== null) { throw new InvariantError(`step/start ${event.data.step} while step ${trace.openStep} is still open`) } + // Steps are checked under the same full-log assumption as turns above. if (event.data.step !== trace.nextStep) { throw new InvariantError(`step/start expected step ${trace.nextStep} in turn ${event.data.turn}, got ${event.data.step}`) } diff --git a/packages/session-persistence-jsonl/src/index.ts b/packages/session-persistence-jsonl/src/index.ts index a2f63470cd..f68dde5a0c 100644 --- a/packages/session-persistence-jsonl/src/index.ts +++ b/packages/session-persistence-jsonl/src/index.ts @@ -354,7 +354,7 @@ export class SessionPersistenceJsonl extends SessionPersistence { if (first === undefined) continue // empty/half-written file const meta = parseHeaderMeta(first) if (meta === undefined) continue // not a session header - const summary = await this.readSidecar(meta.id, meta.cwd) + const summary = await this.readSidecarForList(meta.id, meta.cwd) metas.push({ ...meta, ...summary }) } } @@ -600,11 +600,9 @@ export class SessionPersistenceJsonl extends SessionPersistence { } /** - * Read the mutable-summary sidecar, or `undefined` if it is absent/unreadable - * (a session that has never been `update()`d, or a failed sidecar write). The - * caller keeps the header-derived `updatedAt` (the session's createdAt) in - * that case rather than overlaying `0` — reporting an active session as - * updated at the Unix epoch would be wrong. + * Read the mutable-summary sidecar, or `undefined` if it is absent (a session + * that has never been `update()`d). Non-ENOENT failures surface on strict + * load/adopt paths so corrupt metadata does not masquerade as a clean default. */ private async readSidecar(id: SessionId, cwd: string | undefined): Promise { try { @@ -616,6 +614,19 @@ export class SessionPersistenceJsonl extends SessionPersistence { } } + /** + * Best-effort summary read for list(): a corrupt sidecar should degrade one + * row to header metadata, not hide every session from a picker. + */ + private async readSidecarForList(id: SessionId, cwd: string | undefined): Promise { + try { + return await this.readSidecar(id, cwd) + } catch (error: unknown) { + this.ctx.logger.warn(`session-persistence-jsonl: ignoring unreadable summary for session "${id}" while listing: ${String(error)}`) + return undefined + } + } + // --- discovery helpers --- /** Find a session's log file across cwd buckets (when cwd is unknown). */ diff --git a/packages/session-persistence-jsonl/tests/jsonl.spec.ts b/packages/session-persistence-jsonl/tests/jsonl.spec.ts index 08fde20e9d..8257a9853c 100644 --- a/packages/session-persistence-jsonl/tests/jsonl.spec.ts +++ b/packages/session-persistence-jsonl/tests/jsonl.spec.ts @@ -765,6 +765,27 @@ describe('SessionPersistenceJsonl: edge cases', () => { expect(ids).toEqual(['p1', 'p2', 'p3']) }) + it('list tolerates one corrupt sidecar and still returns other sessions', async () => { + const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined) + const bad = meta('bad-list-summary', '/proj') + await ctx.sessionPersistence.create(bad) + await ctx.sessionPersistence.append(bad.id, oneTurnLog()) + await ctx.sessionPersistence.update(bad.id, { title: 'hidden by corrupt sidecar' }) + await writeFile(sidecarPath(root, '/proj', bad.id), '{not json') + const good = meta('good-list-summary', '/proj') + await ctx.sessionPersistence.create(good) + await ctx.sessionPersistence.append(good.id, oneTurnLog()) + await ctx.sessionPersistence.update(good.id, { title: 'visible' }) + + const listed = await ctx.sessionPersistence.list() + + const badListed = listed.find(m => m.id === bad.id) + expect(badListed).toMatchObject({ id: bad.id }) + expect(badListed).not.toHaveProperty('title') + expect(listed.find(m => m.id === good.id)).toMatchObject({ id: good.id, title: 'visible' }) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('bad-list-summary')) + }) + it('list on an empty root returns nothing', async () => { expect(await ctx.sessionPersistence.list()).toEqual([]) }) From a9c36ad576653d88b498feea3fbf83bb6c4b4305 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 01:44:39 +0800 Subject: [PATCH 11/16] fix(bash-local): contain spill close failures --- packages/bash-local/src/run.ts | 10 +++++- packages/bash-local/tests/run.spec.ts | 44 +++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/packages/bash-local/src/run.ts b/packages/bash-local/src/run.ts index 93551c8e6b..39e5de5d88 100644 --- a/packages/bash-local/src/run.ts +++ b/packages/bash-local/src/run.ts @@ -195,7 +195,15 @@ export class OutputCollector { /** Close the spill file (if any) and return the final output. */ finalize(): CollectedOutput { if (this.spillFd !== undefined) { - closeSync(this.spillFd) + try { + closeSync(this.spillFd) + } catch { + // close can surface delayed writeback failures (for example EIO/ENOSPC) + // after writeSync appeared to succeed. Keep finalize total so runBash's + // close handler still resolves, but stop advertising a spill file that + // may be missing its tail. + this.spillFile = undefined + } this.spillFd = undefined } return this.snapshot() diff --git a/packages/bash-local/tests/run.spec.ts b/packages/bash-local/tests/run.spec.ts index 7f17ba1a94..ff1d92b519 100644 --- a/packages/bash-local/tests/run.spec.ts +++ b/packages/bash-local/tests/run.spec.ts @@ -4,6 +4,21 @@ import { dirname, join } from 'node:path' import { describe, expect, it, vi } from 'vitest' import { killGroup, OutputCollector, runBash } from '@deepseek-ai/dsh-bash-local' +const { failNextClose } = vi.hoisted(() => ({ failNextClose: { value: false } })) +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + closeSync(fd: number): void { + if (failNextClose.value) { + failNextClose.value = false + throw Object.assign(new Error('simulated EIO on close'), { code: 'EIO' }) + } + actual.closeSync(fd) + }, + } +}) + const spillDir = mkdtempSync(join(tmpdir(), 'dsh-bash-spec-')) function spec(command: string, overrides: Partial[0]> = {}) { @@ -160,6 +175,19 @@ describe('output truncation and spill', () => { expect(result.stdout.text.length).toBe(500) expect(result.stdout.spillPath).toBeUndefined() }) + + it('settles with the tail and no spill path when final spill close fails', async () => { + failNextClose.value = true + const result = await runBash( + spec('for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', { maxOutputBytes: 500 }), + { spillDir }, + ).done + expect(failNextClose.value).toBe(false) + expect(result.exitCode).toBe(0) + expect(result.stdout.truncated).toBe(true) + expect(result.stdout.text).toContain('line-0200') + expect(result.stdout.spillPath).toBeUndefined() + }) }) describe('OutputCollector', () => { @@ -200,6 +228,22 @@ describe('OutputCollector', () => { expect(collector.totalBytes).toBe(8) expect(collector.finalize().text).toBe('bbbb') }) + + it('contains close failures and drops the spill path', () => { + const collector = new OutputCollector(4, 'closefail', spillDir) + collector.push(Buffer.from('aaaa')) + collector.push(Buffer.from('bbbb')) + expect(collector.snapshot().spillPath).toBeDefined() + + failNextClose.value = true + let out: ReturnType + expect(() => { out = collector.finalize() }).not.toThrow() + + expect(failNextClose.value).toBe(false) + expect(out!.text).toBe('bbbb') + expect(out!.truncated).toBe(true) + expect(out!.spillPath).toBeUndefined() + }) }) describe('killGroup', () => { From deeb3c9e6d42ef6037b8210820af1efebf511b44 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 01:45:21 +0800 Subject: [PATCH 12/16] refactor(acp): share turn-end prompt settlement --- packages/acp/src/index.ts | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index 97a3d5b0a2..66be7458ec 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -59,7 +59,7 @@ import { } from '@agentclientprotocol/sdk' import type { ContentBlock } from '@deepseek-ai/dsh-llm' import type { Agent, AgentStatus } from '@deepseek-ai/dsh-agent' -import type { SessionEvent } from '@deepseek-ai/dsh-session' +import type { SessionEvent, TurnEndReason } from '@deepseek-ai/dsh-session' import type { ToolCallKind, ToolCallPresentation, ToolRegistry, ToolResultPresentation, ToolTerminal } from '@deepseek-ai/dsh-tools' // Side-effect type import: declaration-merges `ctx.sessionPersistence` onto // Context (the bridge injects it and reads `list()` for load cwd validation). @@ -282,6 +282,18 @@ export function apply(ctx: Context, config: AcpConfig): void { inflight.resolve(reason) } + /** Apply the single ACP prompt-settlement mapping for a completed turn. */ + const settleFromTurnEnd = ( + inflight: NonNullable, + reason: TurnEndReason, + ): void => { + if (reason.kind === 'error') { + inflight.reject(internalError(`turn failed: ${reason.message}`)) + } else { + inflight.resolve(turnEndToStopReason(reason)) + } + } + // --- Stream the harness event taxonomy to ACP session/update -------------- // All content streaming AND the prompt settle flow through `session/event`, @@ -327,12 +339,7 @@ export function apply(ctx: Context, config: AcpConfig): void { // Settle only on the OWNING turn's end. if (event.type !== 'turn/end' || inflight.turn !== event.data.turn) return rec.inflight = undefined - const reason = event.data.reason - if (reason.kind === 'error') { - inflight.reject(internalError(`turn failed: ${reason.message}`)) - } else { - inflight.resolve(turnEndToStopReason(reason)) - } + settleFromTurnEnd(inflight, event.data.reason) }) // Settle fallback: a `session/event` listener registered BEFORE ACP that @@ -373,12 +380,7 @@ export function apply(ctx: Context, config: AcpConfig): void { inflight.resolve('cancelled') return } - const reason = end.data.reason - if (reason.kind === 'error') { - inflight.reject(internalError(`turn failed: ${reason.message}`)) - } else { - inflight.resolve(turnEndToStopReason(reason)) - } + settleFromTurnEnd(inflight, end.data.reason) } // On a settle to idle/disposed, reconcile any still-pending prompt from the From 914c7e985893541afdde5c74a2c2d00574626e3d Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 01:47:48 +0800 Subject: [PATCH 13/16] refactor(persistence): share pure backend guards --- .../session-persistence-jsonl/src/index.ts | 44 ++----------------- .../session-persistence-sqlite/src/index.ts | 32 ++------------ packages/session-persistence/src/index.ts | 30 +++++++++++++ .../tests/persistence.spec.ts | 37 +++++++++++++++- 4 files changed, 74 insertions(+), 69 deletions(-) diff --git a/packages/session-persistence-jsonl/src/index.ts b/packages/session-persistence-jsonl/src/index.ts index f68dde5a0c..7a0c97637c 100644 --- a/packages/session-persistence-jsonl/src/index.ts +++ b/packages/session-persistence-jsonl/src/index.ts @@ -25,8 +25,10 @@ import z from 'schemastery' import { open, mkdir, readFile, readdir, rename, link, rm, truncate } from 'node:fs/promises' import { dirname, resolve } from 'node:path' import { randomBytes } from 'node:crypto' -import { SessionPersistence } from '@deepseek-ai/dsh-session-persistence' -import { isJsonValue, interruptedTurnClosers } from '@deepseek-ai/dsh-session' +import { + SessionPersistence, assertSerializable, seedCoversPrefix, +} from '@deepseek-ai/dsh-session-persistence' +import { interruptedTurnClosers } from '@deepseek-ai/dsh-session' import type { Session, SessionEvent, SessionId, SessionMeta, SessionSummary } from '@deepseek-ai/dsh-session' import { encodeSegment, eventLine, logPath, parseHeaderMeta, scanLog, sessionDir, sidecarPath, toHeaderLine, @@ -59,44 +61,6 @@ interface SessionState { owner?: Session } -/** - * Whether a live session's `seed` reproduces a persisted `prefix` exactly — the - * prefix is no longer than the seed, and each prefix event DEEP-equals the seed - * event at the same index. Used to tell a session legitimately continuing a - * persisted log (HMR re-seeing its own session, or a resume) from a different - * session that merely reuses the id: the latter would have its already-counted - * seq 0..prefix-1 events filtered out on flush and its conversation silently - * grafted onto the old log. - * - * The comparison is a full structural equality (via canonical JSON) of each - * event INCLUDING its `data` payload, not just `seq`/`type`/`time` — a session - * built from loaded events but with mutated message/tool payloads (same seq/ - * type/time) must NOT be accepted, or the live history and durable log diverge. - * Both sides are JSON-serializable by contract (Session.append enforces it), so - * JSON.stringify is a sound canonical form here. - */ -function seedCoversPrefix(seed: readonly SessionEvent[], prefix: readonly SessionEvent[]): boolean { - return prefix.length <= seed.length - && prefix.every((e, i) => { - const s = seed[i] - return s !== undefined && JSON.stringify(s) === JSON.stringify(e) - }) -} - -/** - * Reject non-JSON-serializable `event.data`, naming the offending type. Used on - * the backend's `append(events)` entry point (replay/fork paths that bypass a - * live `Session`); events that flow through `Session.append` are already - * validated at the source, so the live write path never needs this. - */ -function assertSerializable(events: readonly SessionEvent[]): void { - for (const event of events) { - if (!isJsonValue(event.data)) { - throw new Error(`event "${event.type}" carries non-JSON-serializable data (seq ${event.seq})`) - } - } -} - /** * Whether `error` is a "no such file/directory" (`ENOENT`) failure — the ONLY * filesystem error that legitimately means "this session/root is absent" for a diff --git a/packages/session-persistence-sqlite/src/index.ts b/packages/session-persistence-sqlite/src/index.ts index 7f59e3dd8a..d28fc8d6f7 100644 --- a/packages/session-persistence-sqlite/src/index.ts +++ b/packages/session-persistence-sqlite/src/index.ts @@ -24,8 +24,10 @@ import z from 'schemastery' import { DatabaseSync } from 'node:sqlite' import { mkdir } from 'node:fs/promises' import { dirname, resolve } from 'node:path' -import { SessionPersistence } from '@deepseek-ai/dsh-session-persistence' -import { isJsonValue, interruptedTurnClosers } from '@deepseek-ai/dsh-session' +import { + SessionPersistence, assertSerializable, seedCoversPrefix, +} from '@deepseek-ai/dsh-session-persistence' +import { interruptedTurnClosers } from '@deepseek-ai/dsh-session' import type { Session, SessionEvent, SessionId, SessionMeta, SessionSummary } from '@deepseek-ai/dsh-session' import { openDatabase, rowToMeta, scanRows, type EventRow, type SessionRow, @@ -54,32 +56,6 @@ interface SessionState { owner?: Session } -/** - * Whether a live session's `seed` reproduces a persisted `prefix` exactly (the - * prefix is no longer than the seed and each event DEEP-equals the seed event - * at the same index). Distinguishes a session legitimately continuing a - * persisted log (HMR re-seeing its own session, or a resume) from a different - * session that merely reuses the id. Mirrors the JSONL backend's check; both - * sides are JSON-serializable by contract, so `JSON.stringify` is a sound - * canonical form. - */ -function seedCoversPrefix(seed: readonly SessionEvent[], prefix: readonly SessionEvent[]): boolean { - return prefix.length <= seed.length - && prefix.every((e, i) => { - const s = seed[i] - return s !== undefined && JSON.stringify(s) === JSON.stringify(e) - }) -} - -/** Reject non-JSON-serializable `event.data`, naming the offending type. */ -function assertSerializable(events: readonly SessionEvent[]): void { - for (const event of events) { - if (!isJsonValue(event.data)) { - throw new Error(`event "${event.type}" carries non-JSON-serializable data (seq ${event.seq})`) - } - } -} - async function settledErrors(promises: Iterable>): Promise { const settled = await Promise.allSettled([...promises]) const errors: unknown[] = [] diff --git a/packages/session-persistence/src/index.ts b/packages/session-persistence/src/index.ts index 02e62940d7..0ce1e25146 100644 --- a/packages/session-persistence/src/index.ts +++ b/packages/session-persistence/src/index.ts @@ -22,6 +22,7 @@ */ import { Context, Service } from 'cordis' +import { isJsonValue } from '@deepseek-ai/dsh-session' import type { SessionEvent, SessionId, SessionMeta, SessionSummary } from '@deepseek-ai/dsh-session' // Re-export the metadata vocabulary so consumers import it from the seam. @@ -33,6 +34,35 @@ declare module 'cordis' { } } +/** + * Whether a live session's seed reproduces a persisted prefix exactly. Backends + * use this collision check to distinguish a legitimate resume/HMR rebind from a + * different live session reusing an existing session id. + * + * The comparison includes the full event payload, not just seq/type/time, so a + * mutated seed cannot be grafted onto a durable log with the same envelope. + */ +export function seedCoversPrefix(seed: readonly SessionEvent[], prefix: readonly SessionEvent[]): boolean { + return prefix.length <= seed.length + && prefix.every((event, index) => { + const seedEvent = seed[index] + return seedEvent !== undefined && JSON.stringify(seedEvent) === JSON.stringify(event) + }) +} + +/** + * Reject non-JSON-serializable event data before a backend serializes a batch. + * Live session appends already enforce this; persistence append paths also + * accept replay/fork batches that may bypass a live session instance. + */ +export function assertSerializable(events: readonly SessionEvent[]): void { + for (const event of events) { + if (!isJsonValue(event.data)) { + throw new Error(`event "${event.type}" carries non-JSON-serializable data (seq ${event.seq})`) + } + } +} + /** * Abstract durable session-persistence service. Subclass, implement the * abstract methods, and load the subclass as a plugin — it registers as diff --git a/packages/session-persistence/tests/persistence.spec.ts b/packages/session-persistence/tests/persistence.spec.ts index 32f97a30b5..5c0a29131f 100644 --- a/packages/session-persistence/tests/persistence.spec.ts +++ b/packages/session-persistence/tests/persistence.spec.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from 'vitest' import { Context } from 'cordis' import { SessionId, isJsonValue, interruptedTurnClosers } from '@deepseek-ai/dsh-session' import type { SessionEvent, SessionMeta, SessionSummary } from '@deepseek-ai/dsh-session' -import { SessionPersistence } from '../src/index.ts' +import { SessionPersistence, assertSerializable, seedCoversPrefix } from '../src/index.ts' import { runPersistenceContract, meta, oneTurnLog } from './contract.ts' /** @@ -104,3 +104,38 @@ describe('SessionPersistence service registration', () => { await fiber.dispose() }) }) + +describe('shared persistence helpers', () => { + it('accepts a seed that reproduces the persisted prefix exactly', () => { + const log = oneTurnLog() + expect(seedCoversPrefix(log, log.slice(0, 3))).toBe(true) + expect(seedCoversPrefix(log, [])).toBe(true) + }) + + it('rejects a prefix longer than the seed', () => { + const log = oneTurnLog() + expect(seedCoversPrefix(log.slice(0, 2), log)).toBe(false) + }) + + it('rejects a same-envelope event with mutated data', () => { + const log = oneTurnLog() + const tampered = structuredClone(log) + const event = tampered[1]! + tampered[1] = { + ...event, + data: { ...event.data, content: [{ type: 'text', text: 'tampered' }] }, + } as SessionEvent + expect(seedCoversPrefix(tampered, log.slice(0, 2))).toBe(false) + }) + + it('accepts JSON-serializable event data', () => { + expect(() => { assertSerializable(oneTurnLog()) }).not.toThrow() + }) + + it('rejects non-JSON-serializable event data with type and seq context', () => { + const bad = [ + { type: 'user/message', seq: 0, time: 1, data: { content: 1n } }, + ] as unknown as SessionEvent[] + expect(() => { assertSerializable(bad) }).toThrow(/"user\/message".*seq 0/) + }) +}) From 0334b4ad2e5589de9b6ef1452dcf20d02424e82a Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 01:49:02 +0800 Subject: [PATCH 14/16] chore: trim stale comments and duplicate strings --- packages/agent-loop/src/loop.ts | 2 -- packages/agent/src/index.ts | 7 +++++-- packages/agent/src/types.ts | 5 +++-- packages/session/src/index.ts | 4 +++- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/agent-loop/src/loop.ts b/packages/agent-loop/src/loop.ts index d1ea117811..359329e062 100644 --- a/packages/agent-loop/src/loop.ts +++ b/packages/agent-loop/src/loop.ts @@ -560,8 +560,6 @@ async function runStep( }) // signal CAN flip during the await above (abort() inside a tool); // the analyzer can't see through the await boundary. - // signal can flip during the await above (abort() inside a tool); - // the analyzer can't see through the await boundary. /* v8 ignore start -- signal.reason default unreachable via agent.abort() */ // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition if (signal.aborted) throw new Error(String(signal.reason ?? 'aborted')) diff --git a/packages/agent/src/index.ts b/packages/agent/src/index.ts index 5d43f87c25..c9181081a3 100644 --- a/packages/agent/src/index.ts +++ b/packages/agent/src/index.ts @@ -71,6 +71,9 @@ export interface AgentFactory { resume(options: ResumeAgentOptions): Promise } +/** Thrown when create/resume is called before an agent factory is registered. */ +const NO_FACTORY_MESSAGE = 'no agent factory registered (load an agent-loop plugin)' + /** * Agent registry (`ctx.agents`): tracks live agents so UI, hook, and * orchestrator plugins can find them without depending on the concrete loop @@ -107,7 +110,7 @@ export class AgentRegistry extends Service { * registered. */ create(options: CreateAgentOptions): Agent { - if (this.factory === undefined) throw new Error('no agent factory registered (load an agent-loop plugin)') + if (this.factory === undefined) throw new Error(NO_FACTORY_MESSAGE) return this.factory.createAgent(options) } @@ -117,7 +120,7 @@ export class AgentRegistry extends Service { * session persistence is not configured. */ async resume(options: ResumeAgentOptions): Promise { - if (this.factory === undefined) throw new Error('no agent factory registered (load an agent-loop plugin)') + if (this.factory === undefined) throw new Error(NO_FACTORY_MESSAGE) return this.factory.resume(options) } diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts index b0587b7e6b..2b0c02d78e 100644 --- a/packages/agent/src/types.ts +++ b/packages/agent/src/types.ts @@ -72,8 +72,9 @@ export interface Agent { * (inject is synchronous): a failing flush is reported via `agent/error` * (step `0`) and the logger, never thrown into the caller. * - * TODO(review): exact envelope/rendering rules live in dsh-session and need - * review once a real adapter exists. + * TODO(review): verify the tagged-envelope rendering against live model + * behavior; the real adapters that were the original precondition now exist + * (see the twin-adapter RFC). */ inject(content: ContentBlock[], options?: SendOptions): void diff --git a/packages/session/src/index.ts b/packages/session/src/index.ts index c64af8d0e4..fd7dc74dd5 100644 --- a/packages/session/src/index.ts +++ b/packages/session/src/index.ts @@ -42,7 +42,9 @@ declare module 'cordis' { * synthetic user-role message (the system-reminder pattern: zero adapter * burden, models distinguish it from real user prompts by the envelope). * - * TODO(review): revisit the envelope once a real adapter exists. + * TODO(review): verify the tagged-envelope rendering against live model + * behavior; the real adapters that were the original precondition now exist + * (see the twin-adapter RFC). */ function renderTagged(tag: string, content: ContentBlock[], source: MessageSource): ContentBlock[] { const open = `<${tag} source=${JSON.stringify(source.kind)}>` From 49bb6a88ebb76a85c6ad0807db5c1c7a3f8f6337 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 01:54:57 +0800 Subject: [PATCH 15/16] fix(tool-bash): handle unavailable spill paths --- .../2026-06-11-content-block-vocabulary.md | 2 +- ...18-shared-persistence-write-coordinator.md | 2 +- packages/bash-local/README.md | 2 +- packages/bash/src/types.ts | 6 +- packages/tool-bash/README.md | 4 +- packages/tool-bash/src/index.ts | 5 +- packages/tool-bash/tests/tools.spec.ts | 58 +++++++++++++++++++ 7 files changed, 69 insertions(+), 10 deletions(-) diff --git a/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md b/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md index 81ae260d53..a3ef1c3202 100644 --- a/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md +++ b/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md @@ -12,7 +12,7 @@ The harness needs one internal language for messages that the loop, session log, Own it: messages are arrays of typed content blocks (`text`, `reasoning`, `tool-call`, `tool-result`, `image`), with the union derived from the merge-extensible `ContentBlockMap` so plugins add block types via declaration merging. The same merge-extensible-map pattern types every "stringly" field (`MessageSource`, `FinishReason`, `TurnTrigger`, `TurnEndReason`). Streaming is a raw chunk protocol; `BlockAssembler` is the single shared assembly implementation. Adapters translate to provider wire formats — mapping cost lives in adapters, where it belongs. -In-session context injection (`context/message`, `steering/message`) renders as tagged user-role envelopes (the system-reminder pattern) rather than a new role, so adapters carry zero burden. TODO(review): revisit once the DeepSeek V4 adapter exists. +In-session context injection (`context/message`, `steering/message`) renders as tagged user-role envelopes (the system-reminder pattern) rather than a new role, so adapters carry zero burden. TODO(review): verify the tagged-envelope rendering against live model behavior; the real adapters that were the original precondition now exist (see the twin-adapter RFC). ## Consequences diff --git a/docs/rfc/proposed/2026-06-18-shared-persistence-write-coordinator.md b/docs/rfc/proposed/2026-06-18-shared-persistence-write-coordinator.md index f213286783..06bbe1a93d 100644 --- a/docs/rfc/proposed/2026-06-18-shared-persistence-write-coordinator.md +++ b/docs/rfc/proposed/2026-06-18-shared-persistence-write-coordinator.md @@ -4,7 +4,7 @@ Status: proposed ## Problem -`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration is now duplicated: per-session state, `session/created` adoption, seed-prefix collision checks, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. That code is correctness-heavy and already receives the same fixes twice. +`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration is now duplicated: per-session state, `session/created` adoption, backend-specific prefix reads, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. The pure seed-prefix collision and serializability guards have already moved into the seam package; the remaining orchestration is still correctness-heavy and already receives the same fixes twice. ## Proposal diff --git a/packages/bash-local/README.md b/packages/bash-local/README.md index cceefbb67c..c9f3c432e3 100644 --- a/packages/bash-local/README.md +++ b/packages/bash-local/README.md @@ -20,7 +20,7 @@ Design surveyed against the bash tools of Claude Code, OpenCode, Codex, and pi; - **Spawn per call, no shell state** — every call is a fresh non-login `bash -c` (deterministic; no rc files). All four surveyed tools spawn per call. `TODO(stateful-shell)` in `src/run.ts` records the two proven stateful designs (Claude Code's cwd-only persistence; Codex's PTY exec sessions) for when real workflows demand them. - **Process-group kills with escalation** — children are spawned `detached` (own process group); kills send SIGTERM to the group, then SIGKILL after a 3s grace (OpenCode's escalation; pipelines and subshells die with the parent). ESRCH is tolerated; daemons that re-parent away from the group can still survive — same caveat as the surveyed tools. -- **Tail-keep truncation + spill files** — output beyond `maxOutputBytes` keeps the in-memory TAIL (errors/results cluster at the end — pi/OpenCode rationale) while the FULL stream is appended to a temp file whose path is reported. The model can `grep`/`tail` the spill file with bash itself. +- **Tail-keep truncation + spill files** — output beyond `maxOutputBytes` keeps the in-memory TAIL (errors/results cluster at the end — pi/OpenCode rationale) while the FULL stream is appended to a temp file whose path is reported when available. If the final spill close reports a delayed writeback failure, the executor still returns the tail but withholds the path rather than advertising a possibly incomplete file. - **Model-friendly env** — `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results. - **Background tasks** — `start()` returns immediately, no timeout applies (Claude Code detaches timeouts when backgrounding), `readOutput()` is incremental with whole-stream byte offsets, and disposal kills everything. diff --git a/packages/bash/src/types.ts b/packages/bash/src/types.ts index 1860401ee2..cfa60ab331 100644 --- a/packages/bash/src/types.ts +++ b/packages/bash/src/types.ts @@ -44,7 +44,7 @@ export interface CollectedOutput { text: string /** True when bytes were dropped from `text`. */ truncated: boolean - /** Path to a file holding the COMPLETE stream, when truncated. */ + /** Path to a file holding the COMPLETE stream, when truncated and available. */ spillPath?: string } @@ -87,9 +87,9 @@ export interface BashTaskRead { delta: string /** True when truncation dropped unread bytes the delta cannot include. */ lossy: boolean - /** Full stdout spill file, when stdout truncation occurred. */ + /** Full stdout spill file, when stdout truncation occurred and a safe path is available. */ stdoutSpillPath?: string - /** Full stderr spill file, when stderr truncation occurred. */ + /** Full stderr spill file, when stderr truncation occurred and a safe path is available. */ stderrSpillPath?: string } diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index a3d6821371..9a7102ce4c 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -18,11 +18,11 @@ Requires a loaded executor implementation (e.g. `@deepseek-ai/dsh-bash-local`); `command`, `workdir`, and `timeoutMs` are resolved against the executor's config defaults via `ctx.bash.resolve()` before execution, so the executor seam (`BashExecSpec`) receives explicit `workdir`/`timeoutMs` values. The workdir default is applied in the tool layer (from the calling agent's `session.header.cwd`) BEFORE `resolve()` — the per-session cwd must come from `exec.agent`, since N sessions share one executor; only when no session cwd is available does the executor fall back to its own config / `process.cwd()`. -Result text: stdout, then a `[stderr]` section, then status markers — `[timed out after Nms]` whenever the executor's timer fired (reported independently of how the process ended, so a command that traps SIGTERM and exits 0 still shows it), `[killed by signal: …]` for a signal death, `[exit code: N]` for a non-zero exit (reported, **not** `isError`: the model decides how to react), and `[output truncated; full output: ]` when the tail was kept. Only infrastructure failures (spawn errors, aborts) surface as `isError` results. +Result text: stdout, then a `[stderr]` section, then status markers — `[timed out after Nms]` whenever the executor's timer fired (reported independently of how the process ended, so a command that traps SIGTERM and exits 0 still shows it), `[killed by signal: …]` for a signal death, `[exit code: N]` for a non-zero exit (reported, **not** `isError`: the model decides how to react), and `[output truncated; full output: ]` when the tail was kept and a safe spill file is available. If the executor knows output was dropped but cannot safely advertise a complete spill file, the path is reported as `(unavailable)`. Only infrastructure failures (spawn errors, aborts) surface as `isError` results. ### `bash_output` -`task_id` → output produced **since the previous `bash_output` call** plus a status line (`running` / `completed, exit code: N` / `killed`). Reads that lost data to buffer bounds say so and point at the full-output spill file. +`task_id` → output produced **since the previous `bash_output` call** plus a status line (`running` / `completed, exit code: N` / `killed`). Reads that lost data to buffer bounds say so and point at the full-output spill file when one is safely available, otherwise `(unavailable)`. ### `bash_kill` diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index 6721b70f7c..d25bb6060b 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -316,7 +316,7 @@ export function apply(ctx: Context): void { description: 'Execute a bash command (`bash -c`) and return its stdout/stderr. ' + 'Each call runs in a fresh shell: no state (cwd, variables, functions) persists between calls — ' + 'pass `workdir` instead of using `cd`. Non-zero exits are reported as `[exit code: N]`. ' - + 'Long output is truncated to its tail; the full output is saved to a file whose path is reported. ' + + 'Long output is truncated to its tail; the full output is saved to a file whose path is reported when available. ' + 'Set `run_in_background: true` for long-running commands: the call returns a task id immediately; ' + 'poll it with `bash_output` and stop it with `bash_kill`.', parameters: { @@ -377,7 +377,8 @@ export function apply(ctx: Context): void { let text = read.delta.length > 0 ? read.delta : '(no new output)' if (read.lossy) { const paths = [read.stdoutSpillPath, read.stderrSpillPath].filter((p): p is string => p !== undefined) - text += `\n[some output was dropped from memory; full output: ${paths.join(', ')}]` + const fullOutput = paths.length > 0 ? paths.join(', ') : '(unavailable)' + text += `\n[some output was dropped from memory; full output: ${fullOutput}]` } text += `\n${statusLine(read.task)}` return Promise.resolve([{ type: 'text', text }]) diff --git a/packages/tool-bash/tests/tools.spec.ts b/packages/tool-bash/tests/tools.spec.ts index cb54a8a60f..47c7ae2a8e 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -4,6 +4,8 @@ import { join } from 'node:path' import { describe, expect, it, vi } from 'vitest' import { Context } from 'cordis' import { CallId } from '@deepseek-ai/dsh-llm' +import { BashExecutor } from '@deepseek-ai/dsh-bash' +import type { BashExecRequest, BashExecSpec, BashRunResult, BashTask, BashTaskRead } from '@deepseek-ai/dsh-bash' import SystemPrompt from '@deepseek-ai/dsh-system-prompt' import ToolRegistry from '@deepseek-ai/dsh-tools' import { LocalBashExecutor } from '@deepseek-ai/dsh-bash-local' @@ -31,6 +33,51 @@ function text(result: { content: { type: string; text?: string }[] }): string { return result.content.filter(block => block.type === 'text').map(block => block.text).join('') } +class LossyReadBashExecutor extends BashExecutor { + private readonly task: BashTask = { + id: 'bash-lossy', + command: 'fake', + status: 'running', + exitCode: null, + signal: null, + done: Promise.resolve(), + } + + resolve(request: BashExecRequest): BashExecSpec { + return { + command: request.command, + workdir: request.workdir ?? process.cwd(), + timeoutMs: request.timeoutMs ?? 0, + ...request.signal ? { signal: request.signal } : {}, + } + } + + run(): Promise { + return Promise.reject(new Error('not used')) + } + + start(): BashTask { + return this.task + } + + get(id: string): BashTask | undefined { + return id === this.task.id ? this.task : undefined + } + + list(): BashTask[] { + return [this.task] + } + + readOutput(id: string): BashTaskRead { + if (id !== this.task.id) throw new Error(`unknown bash task "${id}"`) + return { task: this.task, delta: 'tail', lossy: true } + } + + kill(): boolean { + return false + } +} + describe('bash tool', () => { it('returns stdout for a successful command', async () => { const ctx = await setup() @@ -226,6 +273,17 @@ describe('background tools', () => { expect(text(read)).toContain('[some output was dropped from memory; full output: ') }) + it('bash_output reports unavailable when a lossy read has no safe spill path', async () => { + const ctx = new Context() + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + await ctx.plugin(LossyReadBashExecutor) + await ctx.plugin(ToolBash) + + const read = await call(ctx, 'bash_output', { task_id: 'bash-lossy' }) + expect(text(read)).toBe('tail\n[some output was dropped from memory; full output: (unavailable)]\n[status: running]') + }) + it('bash_kill stops a running task; repeat reports already-finished', async () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'sleep 60', description: 'test command', run_in_background: true }) From 4a1c64663b74843d6c304e93f826cc92e3d9a98b Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 09:28:23 +0800 Subject: [PATCH 16/16] docs: retire completed tagged-envelope review TODO --- docs/rfc/implemented/2026-06-11-content-block-vocabulary.md | 4 ++-- packages/agent/src/types.ts | 6 +++--- packages/session/src/index.ts | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md b/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md index a3ef1c3202..49d55badac 100644 --- a/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md +++ b/docs/rfc/implemented/2026-06-11-content-block-vocabulary.md @@ -12,10 +12,10 @@ The harness needs one internal language for messages that the loop, session log, Own it: messages are arrays of typed content blocks (`text`, `reasoning`, `tool-call`, `tool-result`, `image`), with the union derived from the merge-extensible `ContentBlockMap` so plugins add block types via declaration merging. The same merge-extensible-map pattern types every "stringly" field (`MessageSource`, `FinishReason`, `TurnTrigger`, `TurnEndReason`). Streaming is a raw chunk protocol; `BlockAssembler` is the single shared assembly implementation. Adapters translate to provider wire formats — mapping cost lives in adapters, where it belongs. -In-session context injection (`context/message`, `steering/message`) renders as tagged user-role envelopes (the system-reminder pattern) rather than a new role, so adapters carry zero burden. TODO(review): verify the tagged-envelope rendering against live model behavior; the real adapters that were the original precondition now exist (see the twin-adapter RFC). +In-session context injection (`context/message`, `steering/message`) renders as tagged user-role envelopes (the system-reminder pattern) rather than a new role, so adapters carry zero burden. Live-adapter review has since validated the tagged-envelope rendering against current DeepSeek behavior; a future provider-specific mismatch should be handled in that adapter rather than by adding a new role to the canonical content vocabulary. ## Consequences - Reasoning, prefill, cache hints, and multimodal content all have a home without provider contortions. -- Every adapter pays a translation cost; the streaming protocol carries a TODO(review) marker until the first real adapter validates it. +- Every adapter pays a translation cost; the first real adapters have since validated the streaming protocol, and new adapters should continue proving their provider-specific mapping in adapter-local tests. - IDs that cross package boundaries are branded (`CallId`, `SessionId`, `AgentId`) — nominal typing at zero runtime cost. diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts index 2b0c02d78e..56805a4a23 100644 --- a/packages/agent/src/types.ts +++ b/packages/agent/src/types.ts @@ -72,9 +72,9 @@ export interface Agent { * (inject is synchronous): a failing flush is reported via `agent/error` * (step `0`) and the logger, never thrown into the caller. * - * TODO(review): verify the tagged-envelope rendering against live model - * behavior; the real adapters that were the original precondition now exist - * (see the twin-adapter RFC). + * Live-adapter review has validated the tagged-envelope rendering against + * current DeepSeek behavior; provider-specific mismatches belong in that + * adapter, not in the canonical session vocabulary. */ inject(content: ContentBlock[], options?: SendOptions): void diff --git a/packages/session/src/index.ts b/packages/session/src/index.ts index fd7dc74dd5..4796c05f51 100644 --- a/packages/session/src/index.ts +++ b/packages/session/src/index.ts @@ -42,9 +42,9 @@ declare module 'cordis' { * synthetic user-role message (the system-reminder pattern: zero adapter * burden, models distinguish it from real user prompts by the envelope). * - * TODO(review): verify the tagged-envelope rendering against live model - * behavior; the real adapters that were the original precondition now exist - * (see the twin-adapter RFC). + * Live-adapter review has validated the tagged-envelope rendering against + * current DeepSeek behavior; provider-specific mismatches belong in that + * adapter, not in the canonical session vocabulary. */ function renderTagged(tag: string, content: ContentBlock[], source: MessageSource): ContentBlock[] { const open = `<${tag} source=${JSON.stringify(source.kind)}>`