From 74d14f68bb87ee39ba346f6335556a056791d773 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:11:49 +0800 Subject: [PATCH] fix(acp): accept baseline resource links and release a failed prompt slot ACP v1 requires every agent to accept text AND resource_link prompt content; the automation rewrite dropped the resource_link half of that baseline. Restore the old bracketed-reference flattening in the codec, reject only beyond-baseline blocks, and update the package contract and Agent Note. Also release the per-session prompt slot when agent.send() throws synchronously (an agent disposed outside the bridge would otherwise wedge the session into permanent 'already in flight' rejections), drop the tautological version-negotiation branch, prove the scenario env layer reaches the snapshot subprocess, pin bridge-side fail-closed permission errors, and correct two overpromising test names. --- ...-23-acp-automation-only-protocol.i18n.yaml | 4 +-- ...2026-07-23-acp-automation-only-protocol.md | 2 +- ...6-07-23-acp-automation-only-protocol.zh.md | 2 +- packages/acp/acp/README.md | 8 +++--- packages/acp/acp/src/codec.ts | 26 +++++++++++++----- packages/acp/acp/src/index.ts | 27 ++++++++++++++----- packages/acp/acp/tests/approval.spec.ts | 7 +++++ packages/acp/acp/tests/bridge.spec.ts | 23 +++++++++++++--- packages/acp/acp/tests/codec.spec.ts | 17 +++++++++--- packages/acp/acp/tests/edges.spec.ts | 6 ++++- packages/acp/acp/tests/harness.ts | 5 +++- packages/acp/acp/tests/turns.spec.ts | 13 +++++++++ .../tests/fixtures/fake-acp-agent.ts | 2 ++ .../support/acp-snapshot/tests/suite.spec.ts | 2 ++ 14 files changed, 114 insertions(+), 30 deletions(-) diff --git a/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.i18n.yaml b/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.i18n.yaml index 6d477ec53a..2901a3b813 100644 --- a/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.i18n.yaml +++ b/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write -2026-07-23-acp-automation-only-protocol.md: 5faeae49d8730c12995c80057437350af68cf962 -2026-07-23-acp-automation-only-protocol.zh.md: e464ca023a56eb5cb5b997c12a14cafb1f93ac23 +2026-07-23-acp-automation-only-protocol.md: 2a92f306065b348764f35e1e63f0d7750a636372 +2026-07-23-acp-automation-only-protocol.zh.md: 5889d668310e3bd63f3934a39d4e5250f83f063d diff --git a/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.md b/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.md index 5faeae49d8..2a92f30606 100644 --- a/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.md +++ b/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.md @@ -14,7 +14,7 @@ The snapshot suite complicates removal. Most ACP scenarios exercise the assemble ## Decision -`@deepseek-ai/dsh-acp` is an automation transport under [`packages/acp/acp`](../../../../packages/acp/acp/README.md), outside the `ui` package group. Its public protocol is intentionally small: version negotiation, fresh text sessions with one in-flight prompt each, committed assistant text updates, per-session cancellation, concurrent sessions, and connection-owned teardown. The bridge rejects additional directories, MCP servers, non-text prompts, empty prompts, unknown sessions, and overlapping prompts. +`@deepseek-ai/dsh-acp` is an automation transport under [`packages/acp/acp`](../../../../packages/acp/acp/README.md), outside the `ui` package group. Its public protocol is intentionally small: version negotiation, fresh text sessions with one in-flight prompt each, committed assistant text updates, per-session cancellation, concurrent sessions, and connection-owned teardown. Prompts carry the spec-required baseline only — text plus resource links flattened to bracketed textual references; the bridge rejects additional directories, MCP servers, beyond-baseline prompt content (image, audio, embedded resources), empty prompts, unknown sessions, and overlapping prompts. The bridge emits only committed `assistant/message` text. Reasoning, raw chunks, tool activity, todos, plans, titles, retry markers, terminal metadata, diffs, locations, and resource links remain in the durable session log or in UI-specific transports. It does not provide session load/list/delete, commands, modes, configuration selectors, model switching, plan review, or human elicitation. diff --git a/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.zh.md b/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.zh.md index e464ca023a..5889d66831 100644 --- a/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.zh.md +++ b/.agents/notes/implemented/simplification/2026-07-23-acp-automation-only-protocol.zh.md @@ -14,7 +14,7 @@ ACP 仍有一个有用的职责：另一个 agent（智能体）或自动化控 ## 决策 -`@deepseek-ai/dsh-acp` 是位于 [`packages/acp/acp`](../../../../packages/acp/acp/README.md) 下、独立于 `ui` 包组的自动化传输层。其公开协议特意保持精简：版本协商、全新文本会话（每个会话最多允许一个进行中的提示词）、已提交的助手文本更新、按会话取消、并发会话，以及由连接负责的资源清理。桥接层会拒绝附加目录、MCP 服务器、非文本提示词、空提示词、未知会话和重叠提示词。 +`@deepseek-ai/dsh-acp` 是位于 [`packages/acp/acp`](../../../../packages/acp/acp/README.md) 下、独立于 `ui` 包组的自动化传输层。其公开协议特意保持精简：版本协商、全新文本会话（每个会话最多允许一个进行中的提示词）、已提交的助手文本更新、按会话取消、并发会话，以及由连接负责的资源清理。提示词只承载规范要求的基线内容——文本，加上被展平为方括号文本引用的资源链接；桥接层会拒绝附加目录、MCP 服务器、超出基线的提示词内容（图片、音频、内嵌资源）、空提示词、未知会话和重叠提示词。 桥接层只发出已提交的 `assistant/message` 文本。推理、原始分片、工具活动、待办事项、计划、标题、重试标记、终端元数据、diff、位置和资源链接仍保留在持久会话日志或 UI 专用传输层中。它不提供会话加载、列出与删除、命令、模式、配置选择器、模型切换、plan 评审或面向人类的询问。 diff --git a/packages/acp/acp/README.md b/packages/acp/acp/README.md index 6a2db4e6ba..20b1ecbefe 100644 --- a/packages/acp/acp/README.md +++ b/packages/acp/acp/README.md @@ -19,10 +19,10 @@ Both fields are optional so another agent/request listener may supply the target | Method | Behavior | |---|---| -| `initialize` | Negotiates the supported version and advertises text-only prompts. No session, editor, terminal, filesystem, or MCP capability is advertised. | +| `initialize` | Negotiates the supported version and advertises baseline-only prompts (no image, audio, or embedded-context capability). No session, editor, terminal, filesystem, or MCP capability is advertised. | | `authenticate` | No-op because the server advertises no authentication methods. | | `session/new` | Creates a fresh agent with an absolute primary `cwd`; empty `additionalDirectories` and `mcpServers` are accepted, non-empty values reject. | -| `session/prompt` | Concatenates text blocks, rejects empty or non-text input, permits one in-flight request per session, and settles from that request's owning durable `turn/end`. | +| `session/prompt` | Concatenates text blocks, renders baseline resource links as bracketed textual references, rejects empty or beyond-baseline input, permits one in-flight request per session, and settles from that request's owning durable `turn/end`. | | `session/cancel` | Cancels only the addressed agent and settles its pending prompt as `cancelled`; unknown ids are no-ops. | | `session/update` | Emits one `agent_message_chunk` per non-empty text block in a committed `assistant/message`. Raw deltas and non-message events are omitted. | | `session/request_permission` | Offers one-shot allow/reject choices for bridge-owned approval requests carrying a tool call id. Clients may answer automatically. | @@ -45,7 +45,7 @@ Client disconnect and Cordis disposal share one memoized teardown. The bridge fi #### What the model sees -`session/prompt` text blocks are concatenated verbatim into one user message. Protocol metadata, client capabilities, permission choices, and session ids never enter the model request. +`session/prompt` text blocks are concatenated verbatim into one user message; a baseline resource link appears in that message as a bracketed `[resource_link name=… uri=…]` reference the model may open with its own tools. Protocol metadata, client capabilities, permission choices, and session ids never enter the model request. #### Token effect @@ -72,6 +72,6 @@ Append-only through the owning tool result. ## Known Limitations and Deferred Work - **Fresh sessions only** — load, list, resume, delete, and fork are unsupported. -- **Text and one workspace only** — resource links, images, audio, embedded resources, non-empty additional directories, and MCP servers reject. +- **Baseline prompts and one workspace only** — images, audio, embedded resources, non-empty additional directories, and MCP servers reject; resource links flatten to textual references rather than fetched content. - **Committed answers only** — live progress, reasoning, tool activity, plans, titles, and usage stay off the wire. - **Connection-owned lifetime** — one connection releases all of its sessions; per-session close is not implemented. diff --git a/packages/acp/acp/src/codec.ts b/packages/acp/acp/src/codec.ts index 53ba437425..2a88af1184 100644 --- a/packages/acp/acp/src/codec.ts +++ b/packages/acp/acp/src/codec.ts @@ -31,19 +31,33 @@ export function turnEndToStopReason(reason: TurnEndReason): StopReason { } /** - * Concatenate an ACP prompt's text blocks. + * Flatten an ACP prompt's baseline blocks to text. Text blocks concatenate + * verbatim; resource links become explicit textual references so a baseline + * client can point at files without the bridge silently dropping that context. * @param prompt - supported ACP prompt blocks. - * @returns text in wire order. + * @returns text in wire order, with resource links rendered as bracketed references. */ export function acpPromptToText(prompt: readonly AcpContentBlock[]): string { - return prompt.flatMap(block => block.type === 'text' ? [block.text] : []).join('') + return prompt.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 a prompt asks the automation bridge to carry non-text content. + * Whether a prompt carries content beyond the ACP baseline. The spec requires + * every agent to accept `text` and `resource_link`; richer inline payloads + * (image, audio, embedded resource) are optional capabilities this bridge does + * not advertise, so they are rejected rather than silently dropped. * @param prompt - ACP prompt blocks to inspect. - * @returns `true` when any block is not text. + * @returns `true` when any block is neither `text` nor `resource_link`. */ 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/acp/src/index.ts b/packages/acp/acp/src/index.ts index 59ed499b00..c4973a9e1d 100644 --- a/packages/acp/acp/src/index.ts +++ b/packages/acp/acp/src/index.ts @@ -192,12 +192,11 @@ export function apply(ctx: Context, config: AcpConfig): void { const makeAgent = (connection: AgentSideConnection): AcpAgent => { conn = connection return { - initialize(params: InitializeRequest): Promise { - const protocolVersion = params.protocolVersion === PROTOCOL_VERSION - ? params.protocolVersion - : PROTOCOL_VERSION + initialize(_params: InitializeRequest): Promise { + // Single-version agent: per spec, answer the latest version this + // server supports regardless of the client's requested version. return Promise.resolve({ - protocolVersion, + protocolVersion: PROTOCOL_VERSION, agentInfo: { name: 'deepseek-harness-acp', version: '0.0.1' }, agentCapabilities: { promptCapabilities: { image: false, audio: false, embeddedContext: false }, @@ -239,14 +238,28 @@ 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') + throw invalidParams('only text and resource_link prompt content is supported') } const text = acpPromptToText(params.prompt) if (text.trim().length === 0) throw invalidParams('empty prompt') const stopReason = await new Promise((resolve, reject) => { + // Arm the slot before send() so a listener-driven synchronous turn + // cannot slip past correlation; a synchronous send() failure (an + // agent disposed outside the bridge, e.g. an agent-loop-only reload) + // must free the slot again or the session would reject every later + // prompt as already in flight. record.inflight = { resolve, reject, turn: undefined } - record.agent.send([{ type: 'text', text }]) + try { + record.agent.send([{ type: 'text', text }]) + } catch (error: unknown) { + record.inflight = undefined + // send() throws only Errors (disposed agent / invalid input); the + // String arm is a defensive fallback for a non-Error throw. + /* v8 ignore next */ + const detail = error instanceof Error ? error.message : String(error) + throw internalError(`prompt was not queued: ${detail}`) + } }) return { stopReason } }, diff --git a/packages/acp/acp/tests/approval.spec.ts b/packages/acp/acp/tests/approval.spec.ts index 648f597010..01bcd83249 100644 --- a/packages/acp/acp/tests/approval.spec.ts +++ b/packages/acp/acp/tests/approval.spec.ts @@ -50,6 +50,13 @@ describe('ACP machine permission policy', () => { await expect(harness.ctx.approval.request(request)).resolves.toBe('rejected') }) + it('fails closed when the client errors the permission request', async () => { + harness = await makeBridgeHarness() + const request = await ownedRequest() + harness.onPermission = () => { throw new Error('client gone') } + await expect(harness.ctx.approval.request(request)).resolves.toBe('unavailable') + }) + it('delegates a same-id foreign agent', async () => { harness = await makeBridgeHarness() const request = await ownedRequest() diff --git a/packages/acp/acp/tests/bridge.spec.ts b/packages/acp/acp/tests/bridge.spec.ts index a462506038..619a628ea1 100644 --- a/packages/acp/acp/tests/bridge.spec.ts +++ b/packages/acp/acp/tests/bridge.spec.ts @@ -107,7 +107,7 @@ describe('automation-only ACP bridge', () => { })).resolves.toHaveProperty('sessionId') }) - it('rejects empty and non-text prompts before a turn starts', async () => { + it('rejects empty and beyond-baseline prompts before a turn starts', async () => { harness = await makeBridgeHarness() await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) @@ -116,11 +116,28 @@ describe('automation-only ACP bridge', () => { .rejects.toThrow(/empty prompt/) await expect(harness.client.prompt({ sessionId, - prompt: [{ type: 'resource_link', name: 'file', uri: 'file:///tmp/a' }], - })).rejects.toThrow(/only text/) + prompt: [{ type: 'image', data: '', mimeType: 'image/png' }], + })).rejects.toThrow(/only text and resource_link/) expect(harness.ctx.agents.get(SessionId(sessionId))?.session.events.some(event => event.type === 'turn/start')).toBe(false) }) + it('renders baseline resource links as textual references in the user message', async () => { + harness = await makeBridgeHarness({ script: [textResponse('done')] }) + await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) + await harness.client.prompt({ + sessionId, + prompt: [ + { type: 'text', text: 'summarize' }, + { type: 'resource_link', name: 'notes.txt', uri: 'file:///tmp/notes.txt' }, + ], + }) + expect(harness.adapter.requests[0]?.messages.at(-1)?.content).toEqual([{ + type: 'text', + text: 'summarize\n[resource_link name="notes.txt" uri="file:///tmp/notes.txt"]\n', + }]) + }) + it('rejects prompts for unknown sessions and ignores unknown cancellation', async () => { harness = await makeBridgeHarness() await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) diff --git a/packages/acp/acp/tests/codec.spec.ts b/packages/acp/acp/tests/codec.spec.ts index 4f8eaecd56..2fdf544500 100644 --- a/packages/acp/acp/tests/codec.spec.ts +++ b/packages/acp/acp/tests/codec.spec.ts @@ -20,10 +20,19 @@ describe('ACP automation codec', () => { expect(turnEndToStopReason({ kind: 'future' } as unknown as TurnEndReason)).toBe('end_turn') }) - it('concatenates text and rejects every non-text block', () => { + it('flattens baseline blocks and rejects everything richer', () => { expect(acpPromptToText([{ type: 'text', text: 'a' }, { type: 'text', text: 'b' }])).toBe('ab') - expect(acpPromptToText([{ type: 'resource_link', name: 'x', uri: 'file:///x' }])).toBe('') - expect(promptHasUnsupportedContent([{ type: 'text', text: 'ok' }])).toBe(false) - expect(promptHasUnsupportedContent([{ type: 'resource_link', name: 'x', uri: 'file:///x' }])).toBe(true) + expect(acpPromptToText([ + { type: 'text', text: 'see' }, + { type: 'resource_link', name: 'x', uri: 'file:///x' }, + ])).toBe('see\n[resource_link name="x" uri="file:///x"]\n') + expect(acpPromptToText([{ type: 'image', data: '', mimeType: 'image/png' }])).toBe('') + expect(promptHasUnsupportedContent([ + { type: 'text', text: 'ok' }, + { type: 'resource_link', name: 'x', uri: 'file:///x' }, + ])).toBe(false) + expect(promptHasUnsupportedContent([ + { type: 'image', data: '', mimeType: 'image/png' }, + ])).toBe(true) }) }) diff --git a/packages/acp/acp/tests/edges.spec.ts b/packages/acp/acp/tests/edges.spec.ts index ab70859939..68e78fb256 100644 --- a/packages/acp/acp/tests/edges.spec.ts +++ b/packages/acp/acp/tests/edges.spec.ts @@ -54,7 +54,11 @@ describe('ACP automation output boundary', () => { expect(harness.updates).toHaveLength(0) }) - it('contains client update failures without changing prompt settlement', async () => { + // `session/update` is a JSON-RPC notification, so a client-side handler + // failure never reaches the bridge; this pins that the prompt still settles + // normally with such a client. The bridge's own write-failure guard is + // transport-level and documented untestable at `notify`. + it('settles the prompt normally when the client rejects update notifications', async () => { harness = await makeBridgeHarness({ script: [textResponse('answer')] }) await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] }) diff --git a/packages/acp/acp/tests/harness.ts b/packages/acp/acp/tests/harness.ts index 35d3c79947..aa3564ea8a 100644 --- a/packages/acp/acp/tests/harness.ts +++ b/packages/acp/acp/tests/harness.ts @@ -101,6 +101,8 @@ export interface BridgeHarness { closeClientTransport: () => Promise abortClientTransport: () => Promise acpFiber: Awaited> + /** The AgentLoop fiber, so a test can reload the loop out from under the bridge. */ + loopFiber: Awaited> dispose: () => Promise } @@ -115,7 +117,7 @@ export async function makeBridgeHarness(options: { const adapter = new MockAdapter(options.script ?? []) const ctx = new Context() await mountAgentLoopTestDependencies(ctx, { systemPrompt: { persona: options.persona ?? '' } }) - await ctx.plugin(AgentLoop, { agents: [] }) + const loopFiber = await ctx.plugin(AgentLoop, { agents: [] }) ctx.llm.registerAdapter(['mock'], adapter) const agentToClient = new TransformStream() @@ -140,6 +142,7 @@ export async function makeBridgeHarness(options: { onSessionUpdateError: undefined, client: undefined as unknown as ClientSideConnection, acpFiber: undefined as unknown as BridgeHarness['acpFiber'], + loopFiber, closeClientTransport: async () => { await clientToAgentWriter.close() }, abortClientTransport: async () => { await clientToAgentWriter.abort(new Error('client transport failed')) }, dispose: async () => { await ctx.fiber.dispose() }, diff --git a/packages/acp/acp/tests/turns.spec.ts b/packages/acp/acp/tests/turns.spec.ts index e4123efe60..8a6ab09404 100644 --- a/packages/acp/acp/tests/turns.spec.ts +++ b/packages/acp/acp/tests/turns.spec.ts @@ -109,6 +109,19 @@ describe('ACP prompt lifecycle', () => { await expect(prompt).resolves.toEqual({ stopReason: 'cancelled' }) }) + it('frees the prompt slot when the agent rejects the send synchronously', async () => { + harness = await makeBridgeHarness({ script: [] }) + const sessionId = await newSession(harness) + // Reload the loop out from under the bridge: its agents dispose while the + // bridge record survives, so the next send() throws synchronously. + await harness.loopFiber.dispose() + await expect(harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'one' }] })) + .rejects.toThrow(/prompt was not queued/) + // The failed prompt must not wedge the session's single prompt slot. + await expect(harness.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'two' }] })) + .rejects.toThrow(/prompt was not queued/) + }) + it('permits only one in-flight prompt per session', async () => { harness = await makeBridgeHarness({ script: ['hang'] }) const sessionId = await newSession(harness) diff --git a/packages/support/acp-snapshot/tests/fixtures/fake-acp-agent.ts b/packages/support/acp-snapshot/tests/fixtures/fake-acp-agent.ts index 00e7c8756a..9be693e7ad 100644 --- a/packages/support/acp-snapshot/tests/fixtures/fake-acp-agent.ts +++ b/packages/support/acp-snapshot/tests/fixtures/fake-acp-agent.ts @@ -149,6 +149,8 @@ async function handlePrompt(id: number | string): Promise { override: process.env.DSH_SNAPSHOT_OVERRIDE ?? null, childFiles: process.env.DSH_SNAPSHOT_CHILD_FILES ?? null, spillRoot: process.env.DSH_SNAPSHOT_SPILL_ROOT ?? null, + // Scenario-supplied deployment env (the `Scenario.env` layering seam). + permissionMode: process.env.DSH_PERMISSION_MODE ?? null, })}`) } if (behavior.echoWorkspace === true) { diff --git a/packages/support/acp-snapshot/tests/suite.spec.ts b/packages/support/acp-snapshot/tests/suite.spec.ts index 15838480c7..e021bc31d9 100644 --- a/packages/support/acp-snapshot/tests/suite.spec.ts +++ b/packages/support/acp-snapshot/tests/suite.spec.ts @@ -131,6 +131,8 @@ describe('defineAcpSnapshotSuite: refresh write-back', () => { expect(stdout).not.toContain('stale stdout') expect(stdout).toContain('env:{\\"mode\\":\\"replay\\"') expect(stdout).not.toContain('\\"mode\\":\\"refresh\\"') + // The scenario's own env layer reached the subprocess. + expect(stdout).toContain('\\"permissionMode\\":\\"never\\"') const blocked = readFileSync(join(refreshDir, 'blocked-log', 'session.jsonl'), 'utf8') expect(blocked).toContain('"decision":"block"')