From 642780220e77e297c4333b282dbec605c28efe3b Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 15 Jun 2026 11:40:06 -0700 Subject: [PATCH] chore(acp): apply subagent-review follow-ups (changeset, KTD10 tests, docs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two-reviewer pass (security + architecture) on KTD10 + the full Route A increment: no code defects, no P0, merge-ready as a dormant increment. Applying the P1 follow-ups: - Add the feature changeset (@runfusion/fusion minor) — the one convention gap. - KTD10 tests: fail-closed (bridge not resolved -> env stays unset -> -p) and idempotency (second onLoad keeps the first published path). - Document the two intentional, parallel MCP-forwarding paths (U10 engine-adapter vs U11 provider-driver) so nobody double-forwards, and the known ACP-path-token-usage=0 residual so U12 doesn't treat it as a bug. Reviewers confirmed: dormancy invariant holds end-to-end (nothing sets FUSION_CLAUDE_ACP=1; both flag+path required; -p is the default); OAuth pi path untouched. 206/206 plugin tests, 333/333 pi-claude-cli tests, typecheck clean. Co-Authored-By: Claude Opus 4.8 --- .changeset/acp-route-a-claude-cli-bridge.md | 11 ++++++ docs/acp-contract.md | 5 +++ .../src/__tests__/index.test.ts | 8 +++++ .../src/__tests__/ktd10-fail-closed.test.ts | 35 +++++++++++++++++++ 4 files changed, 59 insertions(+) create mode 100644 .changeset/acp-route-a-claude-cli-bridge.md create mode 100644 plugins/fusion-plugin-acp-runtime/src/__tests__/ktd10-fail-closed.test.ts diff --git a/.changeset/acp-route-a-claude-cli-bridge.md b/.changeset/acp-route-a-claude-cli-bridge.md new file mode 100644 index 0000000000..27a3f3efbe --- /dev/null +++ b/.changeset/acp-route-a-claude-cli-bridge.md @@ -0,0 +1,11 @@ +--- +"@runfusion/fusion": minor +--- + +Route Fusion's Claude CLI path through the ACP bridge (`claude-code-cli-acp`) instead of `claude -p` (Route A, dormant behind an OFF-by-default kill-switch). + +- **U10** — forward `mcpServers` on ACP `session/new` through the runtime contract (`AgentRuntimeOptions.mcpServers` + the plugin's `newAcpSession`); defaults to `[]` so existing read-only ACP "ask" turns are unchanged. +- **U11** — `streamViaAcp`: the `pi-claude-cli` provider can drive Claude through the bundled ACP bridge, returning the same `AssistantMessageEventStream` as the `-p` path. Dispatched only when `FUSION_CLAUDE_ACP=1` and a bridge path are present, so the live `-p` path is byte-for-byte untouched by default. Full-history prompting, schema-only MCP forwarding with break-early on pi-known tools, control-char/size sanitization, env allow-list, process-registry registration, and inactivity timeout. +- **KTD10** — the ACP runtime plugin publishes its identity-pinned bundled bridge path on load so the kill-switch needs no manual path; it does not enable the transport. + +The Claude-via-pi OAuth path is unchanged. Live verification confirmed the bridge gates tool execution behind `session/request_permission` (forwarded MCP tools and native tools do not execute when cancelled). Remaining for a follow-up: picker/auth/status surface (U12), workflow `model`-node verification (U13), and production rollout. diff --git a/docs/acp-contract.md b/docs/acp-contract.md index 394e39c1d5..70df18f4e9 100644 --- a/docs/acp-contract.md +++ b/docs/acp-contract.md @@ -229,3 +229,8 @@ pi-known tools is SAFE. Also validated: the bridged `claude` authenticates only with the richer env allow-list (HOME/PATH + USER/SHELL/LANG/XDG_*) that `streamViaAcp` forwards — a thin {HOME,PATH} env fails with "Not logged in". The Route A enablement gate is CLEARED. Harness: /tmp/acp-toolflow/verify2.mjs. + +### Route A architecture notes (2026-06-15, from review) + +- **Two parallel MCP-forwarding paths, by design.** U10 wires `mcpServers` through the engine `AgentRuntimeOptions` → ACP plugin adapter `newAcpSession` (for the engine-driven `acp` runtime). U11's `pi-claude-cli` provider does NOT consume that field — `streamViaAcp` drives its OWN inline ACP client and builds `mcpServers` locally via `buildAcpMcpServers` (KTD10: the provider speaks ACP directly via the published bridge path, never through the plugin adapter). The two intersect only at the shared schema-only MCP server shape. Do not "wire U10 into U11" — that would double-forward. +- **Known residual: ACP-path token usage/cost reads zero.** `streamViaAcp` synthesizes pi events via `createEventBridge` from ACP `session/update`s, which carry no token-usage frames, so `output.usage` stays zero on the ACP path. Cost telemetry undercounts when the kill-switch is enabled. The U12 status surface should not treat zero-usage as a bug; wiring usage (if the bridge ever exposes it) is deferred. diff --git a/plugins/fusion-plugin-acp-runtime/src/__tests__/index.test.ts b/plugins/fusion-plugin-acp-runtime/src/__tests__/index.test.ts index 843fb68de5..de1196997f 100644 --- a/plugins/fusion-plugin-acp-runtime/src/__tests__/index.test.ts +++ b/plugins/fusion-plugin-acp-runtime/src/__tests__/index.test.ts @@ -165,4 +165,12 @@ describe("KTD10 — onLoad publishes the bundled bridge path (Route A)", () => { plugin.hooks?.onLoad?.(fakeCtx() as never); expect(process.env.FUSION_CLAUDE_ACP_BRIDGE).toBe("/custom/bridge/path"); }); + + it("is idempotent — a second onLoad keeps the first published path and does not throw", () => { + delete process.env.FUSION_CLAUDE_ACP_BRIDGE; + plugin.hooks?.onLoad?.(fakeCtx() as never); + const first = process.env.FUSION_CLAUDE_ACP_BRIDGE; + expect(() => plugin.hooks?.onLoad?.(fakeCtx() as never)).not.toThrow(); + expect(process.env.FUSION_CLAUDE_ACP_BRIDGE).toBe(first); + }); }); diff --git a/plugins/fusion-plugin-acp-runtime/src/__tests__/ktd10-fail-closed.test.ts b/plugins/fusion-plugin-acp-runtime/src/__tests__/ktd10-fail-closed.test.ts new file mode 100644 index 0000000000..dbbb91191d --- /dev/null +++ b/plugins/fusion-plugin-acp-runtime/src/__tests__/ktd10-fail-closed.test.ts @@ -0,0 +1,35 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; + +// Force the bundled-bridge resolver to "not_resolved" so we can assert onLoad's +// fail-closed branch: when the bridge isn't installed, nothing is published and +// Route A stays unavailable (the kill-switch falls back to `-p`). +vi.mock("../cli-spawn.js", async (importActual) => { + const actual = await importActual(); + return { + ...actual, + resolveBundledClaudeBridgeBinary: () => ({ + kind: "not_resolved", + requested: "claude-code-cli-acp", + path: "/missing/claude-code-cli-acp", + reason: "bundled bridge not installed (test)", + }), + }; +}); + +import plugin from "../index.js"; + +const fakeCtx = () => ({ settings: {}, logger: { info: () => undefined, warn: () => undefined } }); + +describe("KTD10 fail-closed — bundled bridge not resolved", () => { + const saved = process.env.FUSION_CLAUDE_ACP_BRIDGE; + afterEach(() => { + if (saved === undefined) delete process.env.FUSION_CLAUDE_ACP_BRIDGE; + else process.env.FUSION_CLAUDE_ACP_BRIDGE = saved; + }); + + it("does NOT publish FUSION_CLAUDE_ACP_BRIDGE when the bridge is not resolved", () => { + delete process.env.FUSION_CLAUDE_ACP_BRIDGE; + plugin.hooks?.onLoad?.(fakeCtx() as never); + expect(process.env.FUSION_CLAUDE_ACP_BRIDGE).toBeUndefined(); + }); +});