From a4d75aa2b9a15b03b845aa8d457061c6e7131fa0 Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" <277301654+credence-the-bot[bot]@users.noreply.github.com> Date: Tue, 5 May 2026 07:36:13 +0000 Subject: [PATCH 1/4] fix(tool-projection): expose host_file_* on cross-client transports when capable client connected (#29613) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(tool-projection): add CROSS_CLIENT_EXPOSED_CAPABILITIES set Generalizes the cross-client tool-exposure carve-out in isToolActiveForContext from a hardcoded host_bash check to a Set containing host_bash and host_file. Phase 2 (PR #29398) and Phase 3 (PR #29440) shipped the routing infrastructure for cross-client host_file_* execution but never extended this exposure gate, so web/iphone turns silently lacked all four host_file_* tools even when a macOS client was connected. Also fixes a latent miscount: the hub query now uses the actual capability under inspection instead of the hardcoded "host_bash" string. Co-Authored-By: Claude Opus 4.7 * test(tool-projection): cover host_file_* cross-client exposure paths Generalizes the per-capability client-count mock from a single host_bash counter to a Map keyed by capability, then adds 16 new tests (4 host_file_* tools × {exposed-with-client, blocked-no-client, blocked-on-chrome-extension, blocked-when-hasNoClient}) plus a regression guard verifying listClientsByCapability is queried with the actual capability under inspection (D5 latent-bug fix). Co-Authored-By: Claude Opus 4.7 * docs(tool-projection): update file docstring for cross-client carve-out Updates the test file's top-of-file docstring to describe the generalized CROSS_CLIENT_EXPOSED_CAPABILITIES carve-out (host_bash + host_file) instead of the Phase-1-only host_bash framing. Co-Authored-By: Claude Opus 4.7 * fix(tool-projection): add host_file_* executor guards for cross-client transports Adds the 'no-client' and 'stale-targetClientId' runtime guards to host_file_read / write / edit (and the missing no-client guard to host_file_transfer), mirroring the existing host_bash guards in host-shell.ts. Without these guards, exposing host_file_* on non-host-proxy transports (web, iphone) via PR #29613's cross-client carve-out could silently fall through to the daemon container's filesystem if the macOS client disconnects between tool projection and tool execution — reading or modifying files inside the cloud daemon instead of the user's host machine. Addresses Codex P1 on PR #29613. Co-Authored-By: Claude Opus 4.7 * fix(tool-projection): scope host_file_* disconnected-target guard to non-host-proxy turns Codex P2 on PR #29613: the disconnected-target guard added in commit 4ec0e1aebf rejects any host_file_{read,write,edit} call with target_client_id when HostFileProxy is unavailable, even on macos turns where local-fs fallback IS the intended offline behavior. If a stale or auto-filled target_client_id leaks in from a prior cross-client turn, a macos turn now errors instead of writing locally — a regression vs the pre-PR behavior. Scope the guard to !supportsHostProxy(transport) so it only fires on web/iphone, where local fs would be the daemon container's filesystem and falling through is genuinely unsafe. macos turns silently ignore a stale target_client_id and fall through to FileSystemOps as before. Adds one regression test per executor (read/write/edit) covering macos + stale target_client_id + unavailable proxy → falls through to local fs. Co-Authored-By: Claude Opus 4.7 * fix(tool-projection): scope host_file_transfer disconnected-target guard to non-host-proxy turns Devin review on PR #29613: transfer.ts had the same scope drift the prior commit fixed in read/write/edit. The pre-existing post-proxy guard `if (targetClientId != null) { ... no host client is available }` fired on macOS too, so a stale/auto-filled target_client_id from a prior cross-client turn caused host_file_transfer to error on macOS even though host_file_{read,write,edit} silently fell through to local fs (the desired offline-mode behavior). Replaces the unscoped guard with a scoped guard #3 placed at the top of execute() (matching the read/write/edit pattern), keyed on !supportsHostProxy(transport). The non-host-proxy + stale-target case now produces the same "target client ... is no longer connected" message as the other tools; on macOS the stale target_client_id is silently ignored and the call falls through to executeLocal as before PR #29613. Adds two tests in transfer.test.ts: web + stale target → error, and macos + stale target → falls through to local copy. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: credence-the-bot[bot] Co-authored-by: Claude Opus 4.7 --- .../__tests__/conversation-tool-setup.test.ts | 111 +++++++++++---- .../src/daemon/conversation-tool-setup.ts | 43 +++++- .../src/tools/host-filesystem/edit.test.ts | 130 ++++++++++++++++++ assistant/src/tools/host-filesystem/edit.ts | 36 +++++ .../src/tools/host-filesystem/read.test.ts | 112 +++++++++++++++ assistant/src/tools/host-filesystem/read.ts | 36 +++++ .../tools/host-filesystem/transfer.test.ts | 91 +++++++++++- .../src/tools/host-filesystem/transfer.ts | 49 +++++-- .../src/tools/host-filesystem/write.test.ts | 116 ++++++++++++++++ assistant/src/tools/host-filesystem/write.ts | 36 +++++ 10 files changed, 718 insertions(+), 42 deletions(-) create mode 100644 assistant/src/tools/host-filesystem/edit.test.ts create mode 100644 assistant/src/tools/host-filesystem/read.test.ts create mode 100644 assistant/src/tools/host-filesystem/write.test.ts diff --git a/assistant/src/daemon/__tests__/conversation-tool-setup.test.ts b/assistant/src/daemon/__tests__/conversation-tool-setup.test.ts index 36f7409d26f..80fde45e68a 100644 --- a/assistant/src/daemon/__tests__/conversation-tool-setup.test.ts +++ b/assistant/src/daemon/__tests__/conversation-tool-setup.test.ts @@ -19,29 +19,28 @@ * host_file_* are filtered out for chrome-extension regardless of the * hasNoClient flag. * - * Cross-client exception (Phase 1): host_bash is allowed for non-host-proxy - * interfaces (e.g. "web") when at least one host_bash-capable client is - * connected via the event hub. host_file_* and host_browser remain filtered - * regardless (Phase 2). + * Cross-client exception: tools whose capabilities are in + * CROSS_CLIENT_EXPOSED_CAPABILITIES (host_bash, host_file) are allowed for + * non-host-proxy interfaces (e.g. "web") when at least one capable client + * is connected via the event hub. host_browser is excluded (chrome-extension + * is its own executor; web turns have no CDP target model). */ import { beforeEach, describe, expect, mock, test } from "bun:test"; // ── Module-level mocks ───────────────────────────────────────────── -// Control how many host_bash-capable clients the hub reports. -let mockHostBashClientCount = 0; +// Control how many capable clients the hub reports per capability. +const mockClientCountByCapability = new Map(); mock.module("../../runtime/assistant-event-hub.js", () => ({ assistantEventHub: { listClientsByCapability: (cap: string) => { - if (cap === "host_bash") { - return Array.from({ length: mockHostBashClientCount }, (_, i) => ({ - clientId: `mock-client-${i}`, - capabilities: ["host_bash"], - })); - } - return []; + const count = mockClientCountByCapability.get(cap) ?? 0; + return Array.from({ length: count }, (_, i) => ({ + clientId: `mock-${cap}-client-${i}`, + capabilities: [cap], + })); }, }, broadcastMessage: () => {}, @@ -72,7 +71,7 @@ function makeCtx( } beforeEach(() => { - mockHostBashClientCount = 0; + mockClientCountByCapability.clear(); }); describe("isToolActiveForContext — host tool capability gating", () => { @@ -213,7 +212,7 @@ describe("isToolActiveForContext — cross-client exception (Phase 1: host_bash) test("host_bash is active for web transport when a host_bash-capable client is connected", () => { // Cross-client path: a web turn should see host_bash when a macOS client // with host_bash capability is connected via the event hub. - mockHostBashClientCount = 1; + mockClientCountByCapability.set("host_bash", 1); expect( isToolActiveForContext( "host_bash", @@ -224,7 +223,7 @@ describe("isToolActiveForContext — cross-client exception (Phase 1: host_bash) test("host_bash is NOT active for web transport when no capable client is connected", () => { // No cross-client fallback: hub has no host_bash-capable subscribers. - mockHostBashClientCount = 0; + mockClientCountByCapability.set("host_bash", 0); expect( isToolActiveForContext( "host_bash", @@ -233,11 +232,11 @@ describe("isToolActiveForContext — cross-client exception (Phase 1: host_bash) ).toBe(false); }); - test("host_file_read is NOT active for web transport even when a capable client is connected (Phase 2 gate)", () => { - // The cross-client exception is scoped to host_bash only. - // host_file_* remain filtered for non-host-proxy interfaces regardless - // of connected clients until Phase 2 lands. - mockHostBashClientCount = 1; + test("host_file_read is NOT active for web transport when only a host_bash client is connected", () => { + // The cross-client exception is per-capability: a host_bash-capable + // client in the hub does not satisfy host_file's exposure check, since + // listClientsByCapability is queried with the tool's actual capability. + mockClientCountByCapability.set("host_bash", 1); expect( isToolActiveForContext( "host_file_read", @@ -249,7 +248,7 @@ describe("isToolActiveForContext — cross-client exception (Phase 1: host_bash) test("host_bash for macos transport is unaffected by the cross-client exception", () => { // macos natively supports host_bash via host proxy — the supportsHostProxy // check passes, so the cross-client branch is never reached. - mockHostBashClientCount = 0; + mockClientCountByCapability.set("host_bash", 0); expect( isToolActiveForContext( "host_bash", @@ -262,7 +261,7 @@ describe("isToolActiveForContext — cross-client exception (Phase 1: host_bash) // Even with a capable client in the hub, the macos SSE path takes // precedence — it passes the supportsHostProxy check, bypasses the // cross-client branch, and reaches the hasNoClient gate. - mockHostBashClientCount = 1; + mockClientCountByCapability.set("host_bash", 1); expect( isToolActiveForContext( "host_bash", @@ -275,7 +274,7 @@ describe("isToolActiveForContext — cross-client exception (Phase 1: host_bash) // Security boundary: chrome-extension only gets host_browser. The // cross-client exception explicitly excludes chrome-extension transport // regardless of how many host_bash-capable clients are in the hub. - mockHostBashClientCount = 1; + mockClientCountByCapability.set("host_bash", 1); expect( isToolActiveForContext( "host_bash", @@ -287,7 +286,7 @@ describe("isToolActiveForContext — cross-client exception (Phase 1: host_bash) test("host_bash is NOT active for web transport when hasNoClient is true (no approval UI)", () => { // hasNoClient gate: no interactive approval UI available for this turn. // Cross-client exception must not bypass this gate. - mockHostBashClientCount = 1; + mockClientCountByCapability.set("host_bash", 1); expect( isToolActiveForContext( "host_bash", @@ -297,6 +296,68 @@ describe("isToolActiveForContext — cross-client exception (Phase 1: host_bash) }); }); +describe("isToolActiveForContext — cross-client exposure for host_file_*", () => { + const HOST_FILE_TOOLS = [ + "host_file_read", + "host_file_write", + "host_file_edit", + "host_file_transfer", + ] as const; + + for (const tool of HOST_FILE_TOOLS) { + test(`${tool} is exposed for web transport when a host_file client is connected`, () => { + mockClientCountByCapability.set("host_file", 1); + expect( + isToolActiveForContext( + tool, + makeCtx({ hasNoClient: false, transportInterface: "web" }), + ), + ).toBe(true); + }); + + test(`${tool} is NOT exposed for web when no host_file client is connected`, () => { + mockClientCountByCapability.set("host_file", 0); + expect( + isToolActiveForContext( + tool, + makeCtx({ hasNoClient: false, transportInterface: "web" }), + ), + ).toBe(false); + }); + + test(`${tool} is NOT exposed for chrome-extension (security boundary)`, () => { + mockClientCountByCapability.set("host_file", 1); + expect( + isToolActiveForContext( + tool, + makeCtx({ hasNoClient: true, transportInterface: "chrome-extension" }), + ), + ).toBe(false); + }); + + test(`${tool} is NOT exposed when hasNoClient is true (no approval UI)`, () => { + mockClientCountByCapability.set("host_file", 1); + expect( + isToolActiveForContext( + tool, + makeCtx({ hasNoClient: true, transportInterface: "web" }), + ), + ).toBe(false); + }); + } + + test("listClientsByCapability is queried with the actual capability, not host_bash (regression guard for D5 latent bug)", () => { + mockClientCountByCapability.set("host_bash", 0); + mockClientCountByCapability.set("host_file", 1); + expect( + isToolActiveForContext( + "host_file_transfer", + makeCtx({ hasNoClient: false, transportInterface: "web" }), + ), + ).toBe(true); + }); +}); + describe("HOST_TOOL_NAMES derivation", () => { test("HOST_TOOL_NAMES is derived from HOST_TOOL_TO_CAPABILITY", () => { // Sanity check: every tool in the names set has a capability mapping. diff --git a/assistant/src/daemon/conversation-tool-setup.ts b/assistant/src/daemon/conversation-tool-setup.ts index df44abb735e..df13437aec5 100644 --- a/assistant/src/daemon/conversation-tool-setup.ts +++ b/assistant/src/daemon/conversation-tool-setup.ts @@ -350,6 +350,29 @@ export const HOST_TOOL_TO_CAPABILITY = new Map([ // Derived from HOST_TOOL_TO_CAPABILITY so the invariant "every host tool has // a capability mapping" is a structural fact — no runtime assertion needed. export const HOST_TOOL_NAMES = new Set(HOST_TOOL_TO_CAPABILITY.keys()); +/** + * Capabilities eligible for cross-client exposure on non-host-proxy + * transports (e.g. web, iphone routing to a connected macOS client). + * Adding a capability here exposes ALL tools that map to it (per + * HOST_TOOL_TO_CAPABILITY) on non-host-proxy transports — the daemon then + * routes the actual invocation to the connected capable client via the + * proxy's targetClientId path. + * + * Inclusions: + * - host_bash (Phase 1, PR #29322) + * - host_file (Phases 2 & 3, PRs #29398 + #29440) + * + * Exclusions: + * - host_browser: chrome-extension is its own executor; web turns don't + * have a CDP target model. Re-evaluate when host browser via macOS + * host proxy ships (PR #27489). + * - host_app_control, host_cu: not in HOST_TOOL_TO_CAPABILITY + * (skill-routed). + */ +const CROSS_CLIENT_EXPOSED_CAPABILITIES = new Set([ + "host_bash", + "host_file", +]); const CLIENT_CAPABILITY_TOOL_NAMES = new Set(["app_open"]); const PLATFORM_TOOL_NAMES = new Set(["request_system_permission"]); @@ -384,16 +407,22 @@ export function isToolActiveForContext( // Per-capability check is authoritative for structural support: if the // transport cannot service this capability, the tool is filtered out. if (transport && capability && !supportsHostProxy(transport, capability)) { - // Cross-client exception: allow host_bash for non-host-proxy interfaces when - // at least one capable client is connected via the event hub. - // Only applies to host_bash (not host_file, host_cu, host_browser — Phase 2). - // Excludes chrome-extension (security boundary: extension only gets host_browser) - // and hasNoClient turns (no interactive approval UI available). + // Cross-client exception: allow host tools whose capabilities have + // cross-client routing infrastructure (Phases 1–3) to be exposed for + // non-host-proxy transports (e.g. "web", "iphone") when at least one + // capable client is connected via the event hub. Members of + // CROSS_CLIENT_EXPOSED_CAPABILITIES (host_bash, host_file) qualify; + // host_browser is intentionally excluded (chrome-extension is its + // own executor and web turns don't have a CDP target model). + // chrome-extension transport is excluded as a security boundary + // (extension only gets host_browser); hasNoClient turns are excluded + // (no interactive approval UI available). if ( - capability === "host_bash" && + capability && + CROSS_CLIENT_EXPOSED_CAPABILITIES.has(capability) && transport !== "chrome-extension" && !ctx.hasNoClient && - assistantEventHub.listClientsByCapability("host_bash").length > 0 + assistantEventHub.listClientsByCapability(capability).length > 0 ) { return true; } diff --git a/assistant/src/tools/host-filesystem/edit.test.ts b/assistant/src/tools/host-filesystem/edit.test.ts new file mode 100644 index 00000000000..6dbb1895384 --- /dev/null +++ b/assistant/src/tools/host-filesystem/edit.test.ts @@ -0,0 +1,130 @@ +import { mkdtempSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, mock, test } from "bun:test"; + +import type { ToolContext } from "../types.js"; + +// --------------------------------------------------------------------------- +// Singleton mocks — must precede the tool import so bun's module mock applies. +// --------------------------------------------------------------------------- + +let mockProxyAvailable = false; + +mock.module("../../daemon/host-file-proxy.js", () => ({ + HostFileProxy: { + get instance() { + return { + isAvailable: () => mockProxyAvailable, + request: () => Promise.resolve({ content: "ok", isError: false }), + }; + }, + }, +})); + +mock.module("../../runtime/assistant-event-hub.js", () => ({ + assistantEventHub: { + listClientsByCapability: () => [], + }, +})); + +const { hostFileEditTool } = await import("./edit.js"); + +const testDirs: string[] = []; + +afterEach(() => { + mockProxyAvailable = false; + for (const dir of testDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +function makeTempDir(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "host-edit-test-"))); + testDirs.push(dir); + return dir; +} + +function makeContext( + workingDir: string, + transportInterface: ToolContext["transportInterface"], +): ToolContext { + return { + workingDir, + conversationId: "test-conv", + trustClass: "guardian", + transportInterface, + }; +} + +describe("host_file_edit cross-client guards", () => { + test("returns 'no client' error on web transport when proxy unavailable and no targetClientId", async () => { + const workingDir = makeTempDir(); + const result = await hostFileEditTool.execute( + { + path: "/some/host/path.txt", + old_string: "foo", + new_string: "bar", + }, + makeContext(workingDir, "web"), + ); + expect(result.isError).toBe(true); + expect(result.content).toContain( + "no client with host_file capability is connected", + ); + }); + + test("returns 'specified client disconnected' error when targetClientId set but proxy unavailable on web", async () => { + const workingDir = makeTempDir(); + const result = await hostFileEditTool.execute( + { + path: "/some/host/path.txt", + old_string: "foo", + new_string: "bar", + target_client_id: "abc-123", + }, + makeContext(workingDir, "web"), + ); + expect(result.isError).toBe(true); + expect(result.content).toContain( + 'target client "abc-123" is no longer connected', + ); + }); + + test("falls through to local fs on macos transport when proxy unavailable", async () => { + const workingDir = makeTempDir(); + const result = await hostFileEditTool.execute( + { + path: "/nonexistent/x.txt", + old_string: "foo", + new_string: "bar", + }, + makeContext(workingDir, "macos"), + ); + // Proves the guard did NOT fire on macOS — instead we got the + // local FileSystemOps error path (file not found / IO error). + expect(result.isError).toBe(true); + expect(result.content.toLowerCase()).toMatch(/not found|enoent|editing/); + }); + + test("does NOT reject on macos transport with a stale target_client_id when proxy unavailable (regression: P2 fix)", async () => { + const workingDir = makeTempDir(); + const result = await hostFileEditTool.execute( + { + path: "/nonexistent/x.txt", + old_string: "foo", + new_string: "bar", + target_client_id: "stale-mac", + }, + makeContext(workingDir, "macos"), + ); + // The disconnected-target guard is scoped to non-host-proxy transports + // (!supportsHostProxy). On macos, a stale target_client_id auto-filled + // from a prior cross-client turn must be silently ignored and the call + // must fall through to local FileSystemOps, NOT reject with "target + // client ... is no longer connected". + expect(result.isError).toBe(true); + expect(result.content).not.toContain("is no longer connected"); + expect(result.content.toLowerCase()).toMatch(/not found|enoent|editing/); + }); +}); diff --git a/assistant/src/tools/host-filesystem/edit.ts b/assistant/src/tools/host-filesystem/edit.ts index 4c606d4eb3a..fc3f7240a40 100644 --- a/assistant/src/tools/host-filesystem/edit.ts +++ b/assistant/src/tools/host-filesystem/edit.ts @@ -109,6 +109,42 @@ class HostFileEditTool implements Tool { }; } + // Guard: non-host-proxy interfaces with no capable clients connected. + // Without this guard, the request would fall through to local + // FileSystemOps below and read the daemon container's filesystem + // instead of the user's host machine. + if ( + targetClientId == null && + transportInterface != null && + !supportsHostProxy(transportInterface) && + !HostFileProxy.instance.isAvailable() + ) { + return { + content: + "Error: no client with host_file capability is connected. Connect a macOS client to use host_file from a non-desktop interface.", + isError: true, + }; + } + + // Guard: explicit targetClientId provided on a non-host-proxy transport + // but proxy is unavailable (client disconnected between tool-definition + // and tool-execution). Scoped to !supportsHostProxy so macos turns — + // where local-fs fallback IS the intended offline behavior — still fall + // through if the LLM auto-fills a stale target_client_id from a prior + // cross-client turn. On web/iphone, the call must fail loudly rather + // than silently target the daemon container's filesystem. + if ( + targetClientId != null && + transportInterface != null && + !supportsHostProxy(transportInterface) && + !HostFileProxy.instance.isAvailable() + ) { + return { + content: `Error: target client "${targetClientId}" is no longer connected. The specified client may have disconnected since the tool was called. Run \`assistant clients list --capability host_file\` to see currently connected clients.`, + isError: true, + }; + } + // Proxy to connected client for execution on the user's machine // when a capable client is available (managed/cloud-hosted mode). if (HostFileProxy.instance.isAvailable()) { diff --git a/assistant/src/tools/host-filesystem/read.test.ts b/assistant/src/tools/host-filesystem/read.test.ts new file mode 100644 index 00000000000..c29fe90c1fe --- /dev/null +++ b/assistant/src/tools/host-filesystem/read.test.ts @@ -0,0 +1,112 @@ +import { mkdtempSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, mock, test } from "bun:test"; + +import type { ToolContext } from "../types.js"; + +// --------------------------------------------------------------------------- +// Singleton mocks — must precede the tool import so bun's module mock applies. +// --------------------------------------------------------------------------- + +let mockProxyAvailable = false; + +mock.module("../../daemon/host-file-proxy.js", () => ({ + HostFileProxy: { + get instance() { + return { + isAvailable: () => mockProxyAvailable, + request: () => Promise.resolve({ content: "ok", isError: false }), + }; + }, + }, +})); + +mock.module("../../runtime/assistant-event-hub.js", () => ({ + assistantEventHub: { + listClientsByCapability: () => [], + }, +})); + +const { hostFileReadTool } = await import("./read.js"); + +const testDirs: string[] = []; + +afterEach(() => { + mockProxyAvailable = false; + for (const dir of testDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +function makeTempDir(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "host-read-test-"))); + testDirs.push(dir); + return dir; +} + +function makeContext( + workingDir: string, + transportInterface: ToolContext["transportInterface"], +): ToolContext { + return { + workingDir, + conversationId: "test-conv", + trustClass: "guardian", + transportInterface, + }; +} + +describe("host_file_read cross-client guards", () => { + test("returns 'no client' error on web transport when proxy unavailable and no targetClientId", async () => { + const workingDir = makeTempDir(); + const result = await hostFileReadTool.execute( + { path: "/some/host/path.txt" }, + makeContext(workingDir, "web"), + ); + expect(result.isError).toBe(true); + expect(result.content).toContain( + "no client with host_file capability is connected", + ); + }); + + test("returns 'specified client disconnected' error when targetClientId set but proxy unavailable on web", async () => { + const workingDir = makeTempDir(); + const result = await hostFileReadTool.execute( + { path: "/some/host/path.txt", target_client_id: "abc-123" }, + makeContext(workingDir, "web"), + ); + expect(result.isError).toBe(true); + expect(result.content).toContain( + 'target client "abc-123" is no longer connected', + ); + }); + + test("falls through to local fs on macos transport when proxy unavailable and path is non-image", async () => { + const workingDir = makeTempDir(); + const result = await hostFileReadTool.execute( + { path: "/nonexistent/x.txt" }, + makeContext(workingDir, "macos"), + ); + // Proves the guard did NOT fire on macOS — instead we got the + // local FileSystemOps NOT_FOUND error. + expect(result.isError).toBe(true); + expect(result.content).toContain("File not found"); + }); + + test("does NOT reject on macos transport with a stale target_client_id when proxy unavailable (regression: P2 fix)", async () => { + const workingDir = makeTempDir(); + const result = await hostFileReadTool.execute( + { path: "/nonexistent/x.txt", target_client_id: "stale-mac" }, + makeContext(workingDir, "macos"), + ); + // The disconnected-target guard is scoped to non-host-proxy transports + // (!supportsHostProxy). On macos, a stale target_client_id auto-filled + // from a prior cross-client turn must be silently ignored and the call + // must fall through to local FileSystemOps (NOT_FOUND for a fake path), + // NOT reject with "target client ... is no longer connected". + expect(result.isError).toBe(true); + expect(result.content).toContain("File not found"); + expect(result.content).not.toContain("is no longer connected"); + }); +}); diff --git a/assistant/src/tools/host-filesystem/read.ts b/assistant/src/tools/host-filesystem/read.ts index bfe219701d0..ef7e96f3542 100644 --- a/assistant/src/tools/host-filesystem/read.ts +++ b/assistant/src/tools/host-filesystem/read.ts @@ -80,6 +80,42 @@ class HostFileReadTool implements Tool { }; } + // Guard: non-host-proxy interfaces with no capable clients connected. + // Without this guard, the request would fall through to local + // FileSystemOps below and read the daemon container's filesystem + // instead of the user's host machine. + if ( + targetClientId == null && + transportInterface != null && + !supportsHostProxy(transportInterface) && + !HostFileProxy.instance.isAvailable() + ) { + return { + content: + "Error: no client with host_file capability is connected. Connect a macOS client to use host_file from a non-desktop interface.", + isError: true, + }; + } + + // Guard: explicit targetClientId provided on a non-host-proxy transport + // but proxy is unavailable (client disconnected between tool-definition + // and tool-execution). Scoped to !supportsHostProxy so macos turns — + // where local-fs fallback IS the intended offline behavior — still fall + // through if the LLM auto-fills a stale target_client_id from a prior + // cross-client turn. On web/iphone, the call must fail loudly rather + // than silently target the daemon container's filesystem. + if ( + targetClientId != null && + transportInterface != null && + !supportsHostProxy(transportInterface) && + !HostFileProxy.instance.isAvailable() + ) { + return { + content: `Error: target client "${targetClientId}" is no longer connected. The specified client may have disconnected since the tool was called. Run \`assistant clients list --capability host_file\` to see currently connected clients.`, + isError: true, + }; + } + // Proxy to connected client for execution on the user's machine // when a capable client is available (managed/cloud-hosted mode), // including image reads that need the host filesystem view. diff --git a/assistant/src/tools/host-filesystem/transfer.test.ts b/assistant/src/tools/host-filesystem/transfer.test.ts index 117ddab86ae..a5e83140fdf 100644 --- a/assistant/src/tools/host-filesystem/transfer.test.ts +++ b/assistant/src/tools/host-filesystem/transfer.test.ts @@ -50,8 +50,16 @@ function makeTempDir(): string { return dir; } -function makeContext(workingDir: string): ToolContext { - return { workingDir, conversationId: "test-conv", trustClass: "guardian" }; +function makeContext( + workingDir: string, + transportInterface?: ToolContext["transportInterface"], +): ToolContext { + return { + workingDir, + conversationId: "test-conv", + trustClass: "guardian", + transportInterface, + }; } // --------------------------------------------------------------------------- @@ -269,3 +277,82 @@ describe("host_file_transfer managed mode", () => { expect(toSandboxCalls.length).toBe(0); }); }); + +// --------------------------------------------------------------------------- +// Cross-client guard tests +// --------------------------------------------------------------------------- + +describe("host_file_transfer cross-client guards", () => { + test("returns 'no client' error on web transport when proxy unavailable and no targetClientId", async () => { + // mockProxyAvailable defaults to false. + const workingDir = makeTempDir(); + const srcDir = makeTempDir(); + const srcFile = join(srcDir, "source.txt"); + writeFileSync(srcFile, "content"); + + const result = await hostFileTransferTool.execute( + { + source_path: srcFile, + dest_path: "out.txt", + direction: "to_sandbox", + }, + makeContext(workingDir, "web"), + ); + + expect(result.isError).toBe(true); + expect(result.content).toContain( + "no client with host_file capability is connected", + ); + expect(toSandboxCalls.length).toBe(0); + }); + + test("returns 'specified client disconnected' error when targetClientId set but proxy unavailable on web", async () => { + const workingDir = makeTempDir(); + const srcDir = makeTempDir(); + const srcFile = join(srcDir, "source.txt"); + writeFileSync(srcFile, "content"); + + const result = await hostFileTransferTool.execute( + { + source_path: srcFile, + dest_path: "out.txt", + direction: "to_sandbox", + target_client_id: "abc-123", + }, + makeContext(workingDir, "web"), + ); + + expect(result.isError).toBe(true); + expect(result.content).toContain( + 'target client "abc-123" is no longer connected', + ); + expect(toSandboxCalls.length).toBe(0); + }); + + test("does NOT reject on macos transport with a stale target_client_id when proxy unavailable (regression: Devin-flagged scope drift fix)", async () => { + const workingDir = makeTempDir(); + const srcDir = makeTempDir(); + const srcFile = join(srcDir, "source.txt"); + writeFileSync(srcFile, "content"); + const destFile = join(workingDir, "stale-target.txt"); + + const result = await hostFileTransferTool.execute( + { + source_path: srcFile, + dest_path: destFile, + direction: "to_sandbox", + target_client_id: "stale-mac", + }, + makeContext(workingDir, "macos"), + ); + + // The disconnected-target guard is scoped to non-host-proxy transports + // (!supportsHostProxy). On macos, a stale target_client_id auto-filled + // from a prior cross-client turn must be silently ignored and the local + // copy must succeed, NOT reject with "target client ... is no longer + // connected" or the older "target_client_id was specified but no host + // client is available" message. + expect(result.isError).toBe(false); + expect(existsSync(destFile)).toBe(true); + }); +}); diff --git a/assistant/src/tools/host-filesystem/transfer.ts b/assistant/src/tools/host-filesystem/transfer.ts index 2a5f0a5e343..b263a1221ab 100644 --- a/assistant/src/tools/host-filesystem/transfer.ts +++ b/assistant/src/tools/host-filesystem/transfer.ts @@ -106,6 +106,43 @@ class HostFileTransferTool implements Tool { return { content: `Error: multiple clients support host_file. Specify which client to use with \`target_client_id\`. Run \`assistant clients list --capability host_file\` to see client IDs and labels.`, isError: true }; } + // Guard: non-host-proxy interfaces with no capable clients connected. + // Without this guard, a web/ios turn whose host_file client has + // disconnected since projection would fall through to executeLocal + // below and act on the daemon container's filesystem instead of + // the user's host machine. + if ( + targetClientId == null && + context.transportInterface != null && + !supportsHostProxy(context.transportInterface) && + !HostTransferProxy.instance.isAvailable() + ) { + return { + content: + "Error: no client with host_file capability is connected. Connect a macOS client to use host_file_transfer from a non-desktop interface.", + isError: true, + }; + } + + // Guard: explicit targetClientId provided on a non-host-proxy transport + // but proxy is unavailable (client disconnected between tool-definition + // and tool-execution). Scoped to !supportsHostProxy so macos turns — + // where local-fs fallback IS the intended offline behavior — silently + // ignore a stale target_client_id auto-filled from a prior cross-client + // turn. On web/iphone, the call must fail loudly rather than silently + // target the daemon container's filesystem. + if ( + targetClientId != null && + context.transportInterface != null && + !supportsHostProxy(context.transportInterface) && + !HostTransferProxy.instance.isAvailable() + ) { + return { + content: `Error: target client "${targetClientId}" is no longer connected. The specified client may have disconnected since the tool was called. Run \`assistant clients list --capability host_file\` to see currently connected clients.`, + isError: true, + }; + } + // Validate that host-side paths are absolute. if (direction === "to_host" && !isAbsolute(destPath)) { return { @@ -172,14 +209,10 @@ class HostFileTransferTool implements Tool { ); } - if (targetClientId != null) { - return { - content: `Error: target_client_id '${targetClientId}' was specified but no host client is available. Ensure the client is connected.`, - isError: true, - }; - } - - // Local mode: direct filesystem copy. + // Local mode: direct filesystem copy. The non-host-proxy + stale + // target_client_id case is caught by the scoped guard at the top of + // execute(); on macos a stale target_client_id is silently ignored + // here, matching the read/write/edit pattern. return this.executeLocal(resolvedSourcePath, resolvedDestPath, overwrite); } diff --git a/assistant/src/tools/host-filesystem/write.test.ts b/assistant/src/tools/host-filesystem/write.test.ts new file mode 100644 index 00000000000..82eaa4278ab --- /dev/null +++ b/assistant/src/tools/host-filesystem/write.test.ts @@ -0,0 +1,116 @@ +import { existsSync, mkdtempSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, mock, test } from "bun:test"; + +import type { ToolContext } from "../types.js"; + +// --------------------------------------------------------------------------- +// Singleton mocks — must precede the tool import so bun's module mock applies. +// --------------------------------------------------------------------------- + +let mockProxyAvailable = false; + +mock.module("../../daemon/host-file-proxy.js", () => ({ + HostFileProxy: { + get instance() { + return { + isAvailable: () => mockProxyAvailable, + request: () => Promise.resolve({ content: "ok", isError: false }), + }; + }, + }, +})); + +mock.module("../../runtime/assistant-event-hub.js", () => ({ + assistantEventHub: { + listClientsByCapability: () => [], + }, +})); + +const { hostFileWriteTool } = await import("./write.js"); + +const testDirs: string[] = []; + +afterEach(() => { + mockProxyAvailable = false; + for (const dir of testDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +function makeTempDir(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "host-write-test-"))); + testDirs.push(dir); + return dir; +} + +function makeContext( + workingDir: string, + transportInterface: ToolContext["transportInterface"], +): ToolContext { + return { + workingDir, + conversationId: "test-conv", + trustClass: "guardian", + transportInterface, + }; +} + +describe("host_file_write cross-client guards", () => { + test("returns 'no client' error on web transport when proxy unavailable and no targetClientId", async () => { + const workingDir = makeTempDir(); + const result = await hostFileWriteTool.execute( + { path: "/some/host/path.txt", content: "hello" }, + makeContext(workingDir, "web"), + ); + expect(result.isError).toBe(true); + expect(result.content).toContain( + "no client with host_file capability is connected", + ); + }); + + test("returns 'specified client disconnected' error when targetClientId set but proxy unavailable on web", async () => { + const workingDir = makeTempDir(); + const result = await hostFileWriteTool.execute( + { + path: "/some/host/path.txt", + content: "hello", + target_client_id: "abc-123", + }, + makeContext(workingDir, "web"), + ); + expect(result.isError).toBe(true); + expect(result.content).toContain( + 'target client "abc-123" is no longer connected', + ); + }); + + test("falls through to local fs on macos transport when proxy unavailable", async () => { + const workingDir = makeTempDir(); + const destFile = join(workingDir, "out.txt"); + const result = await hostFileWriteTool.execute( + { path: destFile, content: "hello" }, + makeContext(workingDir, "macos"), + ); + // Proves the guard did NOT fire on macOS — local write succeeded. + expect(result.isError).toBe(false); + expect(existsSync(destFile)).toBe(true); + }); + + test("does NOT reject on macos transport with a stale target_client_id when proxy unavailable (regression: P2 fix)", async () => { + const workingDir = makeTempDir(); + const destFile = join(workingDir, "stale-target.txt"); + const result = await hostFileWriteTool.execute( + { path: destFile, content: "hello", target_client_id: "stale-mac" }, + makeContext(workingDir, "macos"), + ); + // The disconnected-target guard is scoped to non-host-proxy transports + // (!supportsHostProxy). On macos, a stale target_client_id auto-filled + // from a prior cross-client turn must be silently ignored and the local + // write must succeed, NOT reject with "target client ... is no longer + // connected". + expect(result.isError).toBe(false); + expect(existsSync(destFile)).toBe(true); + }); +}); diff --git a/assistant/src/tools/host-filesystem/write.ts b/assistant/src/tools/host-filesystem/write.ts index be23354bd7c..defbb969e71 100644 --- a/assistant/src/tools/host-filesystem/write.ts +++ b/assistant/src/tools/host-filesystem/write.ts @@ -79,6 +79,42 @@ class HostFileWriteTool implements Tool { }; } + // Guard: non-host-proxy interfaces with no capable clients connected. + // Without this guard, the request would fall through to local + // FileSystemOps below and read the daemon container's filesystem + // instead of the user's host machine. + if ( + targetClientId == null && + transportInterface != null && + !supportsHostProxy(transportInterface) && + !HostFileProxy.instance.isAvailable() + ) { + return { + content: + "Error: no client with host_file capability is connected. Connect a macOS client to use host_file from a non-desktop interface.", + isError: true, + }; + } + + // Guard: explicit targetClientId provided on a non-host-proxy transport + // but proxy is unavailable (client disconnected between tool-definition + // and tool-execution). Scoped to !supportsHostProxy so macos turns — + // where local-fs fallback IS the intended offline behavior — still fall + // through if the LLM auto-fills a stale target_client_id from a prior + // cross-client turn. On web/iphone, the call must fail loudly rather + // than silently target the daemon container's filesystem. + if ( + targetClientId != null && + transportInterface != null && + !supportsHostProxy(transportInterface) && + !HostFileProxy.instance.isAvailable() + ) { + return { + content: `Error: target client "${targetClientId}" is no longer connected. The specified client may have disconnected since the tool was called. Run \`assistant clients list --capability host_file\` to see currently connected clients.`, + isError: true, + }; + } + // Proxy to connected client for execution on the user's machine // when a capable client is available (managed/cloud-hosted mode). if (HostFileProxy.instance.isAvailable()) { From a0261f9c1cc70cbe70295688af9a9f110df0de50 Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" <277301654+credence-the-bot[bot]@users.noreply.github.com> Date: Tue, 5 May 2026 07:54:37 +0000 Subject: [PATCH 2/4] fix(tool-projection): self-review cleanup for host_file_* exposure (#29621) Phase 4 round 1: addresses minor integration-review gaps from the self-review pass on PR #29613. - Replace stale "iphone" references with the actual InterfaceId "ios" in 6 comment locations (conversation-tool-setup.ts, the 4 host_file_* executors). - Add an explanatory note in each host_file_* executor's disconnected-target guard pointing out the deliberate divergence from host_bash (host-shell.ts:239-247) and linking to the PR #29613 review for rationale, so future readers understand the asymmetry is intentional. - Align transfer.ts's "no client connected" guard message with the read/write/edit pattern by referring to "host_file" (the capability) rather than "host_file_transfer" (the tool name). - Add a stub mock for assistant-event-hub.js in transfer.test.ts so the multi-client guard test runs against an isolated stub rather than the live process-wide singleton, matching the read/write/edit test pattern. Co-authored-by: credence-the-bot[bot] Co-authored-by: Claude Opus 4.7 --- assistant/src/daemon/conversation-tool-setup.ts | 4 ++-- assistant/src/tools/host-filesystem/edit.ts | 5 ++++- assistant/src/tools/host-filesystem/read.ts | 5 ++++- assistant/src/tools/host-filesystem/transfer.test.ts | 9 +++++++++ assistant/src/tools/host-filesystem/transfer.ts | 7 +++++-- assistant/src/tools/host-filesystem/write.ts | 5 ++++- 6 files changed, 28 insertions(+), 7 deletions(-) diff --git a/assistant/src/daemon/conversation-tool-setup.ts b/assistant/src/daemon/conversation-tool-setup.ts index df13437aec5..208e6d3e041 100644 --- a/assistant/src/daemon/conversation-tool-setup.ts +++ b/assistant/src/daemon/conversation-tool-setup.ts @@ -352,7 +352,7 @@ export const HOST_TOOL_TO_CAPABILITY = new Map([ export const HOST_TOOL_NAMES = new Set(HOST_TOOL_TO_CAPABILITY.keys()); /** * Capabilities eligible for cross-client exposure on non-host-proxy - * transports (e.g. web, iphone routing to a connected macOS client). + * transports (e.g. web, ios routing to a connected macOS client). * Adding a capability here exposes ALL tools that map to it (per * HOST_TOOL_TO_CAPABILITY) on non-host-proxy transports — the daemon then * routes the actual invocation to the connected capable client via the @@ -409,7 +409,7 @@ export function isToolActiveForContext( if (transport && capability && !supportsHostProxy(transport, capability)) { // Cross-client exception: allow host tools whose capabilities have // cross-client routing infrastructure (Phases 1–3) to be exposed for - // non-host-proxy transports (e.g. "web", "iphone") when at least one + // non-host-proxy transports (e.g. "web", "ios") when at least one // capable client is connected via the event hub. Members of // CROSS_CLIENT_EXPOSED_CAPABILITIES (host_bash, host_file) qualify; // host_browser is intentionally excluded (chrome-extension is its diff --git a/assistant/src/tools/host-filesystem/edit.ts b/assistant/src/tools/host-filesystem/edit.ts index fc3f7240a40..e6093c6b4b1 100644 --- a/assistant/src/tools/host-filesystem/edit.ts +++ b/assistant/src/tools/host-filesystem/edit.ts @@ -131,8 +131,11 @@ class HostFileEditTool implements Tool { // and tool-execution). Scoped to !supportsHostProxy so macos turns — // where local-fs fallback IS the intended offline behavior — still fall // through if the LLM auto-fills a stale target_client_id from a prior - // cross-client turn. On web/iphone, the call must fail loudly rather + // cross-client turn. On web/ios, the call must fail loudly rather // than silently target the daemon container's filesystem. + // Note: this scoping deliberately differs from host_bash + // (host-shell.ts:239-247), which rejects unconditionally; see PR #29613 + // review discussion for rationale. if ( targetClientId != null && transportInterface != null && diff --git a/assistant/src/tools/host-filesystem/read.ts b/assistant/src/tools/host-filesystem/read.ts index ef7e96f3542..bc6c783c89e 100644 --- a/assistant/src/tools/host-filesystem/read.ts +++ b/assistant/src/tools/host-filesystem/read.ts @@ -102,8 +102,11 @@ class HostFileReadTool implements Tool { // and tool-execution). Scoped to !supportsHostProxy so macos turns — // where local-fs fallback IS the intended offline behavior — still fall // through if the LLM auto-fills a stale target_client_id from a prior - // cross-client turn. On web/iphone, the call must fail loudly rather + // cross-client turn. On web/ios, the call must fail loudly rather // than silently target the daemon container's filesystem. + // Note: this scoping deliberately differs from host_bash + // (host-shell.ts:239-247), which rejects unconditionally; see PR #29613 + // review discussion for rationale. if ( targetClientId != null && transportInterface != null && diff --git a/assistant/src/tools/host-filesystem/transfer.test.ts b/assistant/src/tools/host-filesystem/transfer.test.ts index a5e83140fdf..a2a6cf8e958 100644 --- a/assistant/src/tools/host-filesystem/transfer.test.ts +++ b/assistant/src/tools/host-filesystem/transfer.test.ts @@ -31,6 +31,15 @@ mock.module("../../daemon/host-transfer-proxy.js", () => ({ }, })); +// Mirror read/write/edit test files: stub the event hub so the multi-client +// guard at line ~100 of transfer.ts is exercised against an isolated stub +// rather than the live process-wide singleton. +mock.module("../../runtime/assistant-event-hub.js", () => ({ + assistantEventHub: { + listClientsByCapability: () => [], + }, +})); + const { hostFileTransferTool } = await import("./transfer.js"); const testDirs: string[] = []; diff --git a/assistant/src/tools/host-filesystem/transfer.ts b/assistant/src/tools/host-filesystem/transfer.ts index b263a1221ab..09c9f41cb60 100644 --- a/assistant/src/tools/host-filesystem/transfer.ts +++ b/assistant/src/tools/host-filesystem/transfer.ts @@ -119,7 +119,7 @@ class HostFileTransferTool implements Tool { ) { return { content: - "Error: no client with host_file capability is connected. Connect a macOS client to use host_file_transfer from a non-desktop interface.", + "Error: no client with host_file capability is connected. Connect a macOS client to use host_file from a non-desktop interface.", isError: true, }; } @@ -129,8 +129,11 @@ class HostFileTransferTool implements Tool { // and tool-execution). Scoped to !supportsHostProxy so macos turns — // where local-fs fallback IS the intended offline behavior — silently // ignore a stale target_client_id auto-filled from a prior cross-client - // turn. On web/iphone, the call must fail loudly rather than silently + // turn. On web/ios, the call must fail loudly rather than silently // target the daemon container's filesystem. + // Note: this scoping deliberately differs from host_bash + // (host-shell.ts:239-247), which rejects unconditionally; see PR #29613 + // review discussion for rationale. if ( targetClientId != null && context.transportInterface != null && diff --git a/assistant/src/tools/host-filesystem/write.ts b/assistant/src/tools/host-filesystem/write.ts index defbb969e71..bdb205d19c5 100644 --- a/assistant/src/tools/host-filesystem/write.ts +++ b/assistant/src/tools/host-filesystem/write.ts @@ -101,8 +101,11 @@ class HostFileWriteTool implements Tool { // and tool-execution). Scoped to !supportsHostProxy so macos turns — // where local-fs fallback IS the intended offline behavior — still fall // through if the LLM auto-fills a stale target_client_id from a prior - // cross-client turn. On web/iphone, the call must fail loudly rather + // cross-client turn. On web/ios, the call must fail loudly rather // than silently target the daemon container's filesystem. + // Note: this scoping deliberately differs from host_bash + // (host-shell.ts:239-247), which rejects unconditionally; see PR #29613 + // review discussion for rationale. if ( targetClientId != null && transportInterface != null && From c5ecdc2660d5c1e6a6d5c4343c8bccf256546140 Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" <277301654+credence-the-bot[bot]@users.noreply.github.com> Date: Tue, 5 May 2026 08:06:48 +0000 Subject: [PATCH 3/4] fix(tool-projection): replace PR cross-reference in divergence comment with inline rationale (#29628) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 4 round 2: Devin BUG findings on PR #29621 flagged the "see PR #29613 review discussion for rationale" tail of the new divergence comments as a violation of assistant/AGENTS.md "Code comments" rule (don't narrate history; describe the current state). Replaces the PR cross-reference in each of the 4 host_file_* executors with a short inline statement of the asymmetry — that host_bash rejects unconditionally for any stale target_client_id regardless of transport — keeping the divergence call-out without relying on PR-discussion archaeology. Co-authored-by: credence-the-bot[bot] Co-authored-by: Claude Opus 4.7 --- assistant/src/tools/host-filesystem/edit.ts | 4 ++-- assistant/src/tools/host-filesystem/read.ts | 4 ++-- assistant/src/tools/host-filesystem/transfer.ts | 4 ++-- assistant/src/tools/host-filesystem/write.ts | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/assistant/src/tools/host-filesystem/edit.ts b/assistant/src/tools/host-filesystem/edit.ts index e6093c6b4b1..c4be612cbbb 100644 --- a/assistant/src/tools/host-filesystem/edit.ts +++ b/assistant/src/tools/host-filesystem/edit.ts @@ -134,8 +134,8 @@ class HostFileEditTool implements Tool { // cross-client turn. On web/ios, the call must fail loudly rather // than silently target the daemon container's filesystem. // Note: this scoping deliberately differs from host_bash - // (host-shell.ts:239-247), which rejects unconditionally; see PR #29613 - // review discussion for rationale. + // (host-shell.ts:239-247), which rejects unconditionally for any + // stale target_client_id regardless of transport. if ( targetClientId != null && transportInterface != null && diff --git a/assistant/src/tools/host-filesystem/read.ts b/assistant/src/tools/host-filesystem/read.ts index bc6c783c89e..ba8a5193c1c 100644 --- a/assistant/src/tools/host-filesystem/read.ts +++ b/assistant/src/tools/host-filesystem/read.ts @@ -105,8 +105,8 @@ class HostFileReadTool implements Tool { // cross-client turn. On web/ios, the call must fail loudly rather // than silently target the daemon container's filesystem. // Note: this scoping deliberately differs from host_bash - // (host-shell.ts:239-247), which rejects unconditionally; see PR #29613 - // review discussion for rationale. + // (host-shell.ts:239-247), which rejects unconditionally for any + // stale target_client_id regardless of transport. if ( targetClientId != null && transportInterface != null && diff --git a/assistant/src/tools/host-filesystem/transfer.ts b/assistant/src/tools/host-filesystem/transfer.ts index 09c9f41cb60..1b20f153525 100644 --- a/assistant/src/tools/host-filesystem/transfer.ts +++ b/assistant/src/tools/host-filesystem/transfer.ts @@ -132,8 +132,8 @@ class HostFileTransferTool implements Tool { // turn. On web/ios, the call must fail loudly rather than silently // target the daemon container's filesystem. // Note: this scoping deliberately differs from host_bash - // (host-shell.ts:239-247), which rejects unconditionally; see PR #29613 - // review discussion for rationale. + // (host-shell.ts:239-247), which rejects unconditionally for any + // stale target_client_id regardless of transport. if ( targetClientId != null && context.transportInterface != null && diff --git a/assistant/src/tools/host-filesystem/write.ts b/assistant/src/tools/host-filesystem/write.ts index bdb205d19c5..1df11782be8 100644 --- a/assistant/src/tools/host-filesystem/write.ts +++ b/assistant/src/tools/host-filesystem/write.ts @@ -104,8 +104,8 @@ class HostFileWriteTool implements Tool { // cross-client turn. On web/ios, the call must fail loudly rather // than silently target the daemon container's filesystem. // Note: this scoping deliberately differs from host_bash - // (host-shell.ts:239-247), which rejects unconditionally; see PR #29613 - // review discussion for rationale. + // (host-shell.ts:239-247), which rejects unconditionally for any + // stale target_client_id regardless of transport. if ( targetClientId != null && transportInterface != null && From 789bea2efc577daead5c4464c797f1f8d124a650 Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" Date: Tue, 5 May 2026 08:24:10 +0000 Subject: [PATCH 4/4] fix(tool-projection): close backwards-compat hole in host_file_* disconnected-target guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 on the feature-branch PR (#29632): the disconnected-target guard added in PR #29613 / cycle 2 only fires when \`context.transportInterface != null\`. On the documented legacy/ backwards-compat path where transport metadata is missing, a call with target_client_id and no connected host client skips this error path and falls through to local FileSystemOps / executeLocal — silently targeting the daemon container's filesystem instead of the intended host client. This was a real regression introduced by cycle 3's removal of transfer.ts's pre-existing unscoped guard. Restructure the condition so the guard fires for both non-host-proxy transports AND undefined transport, skipping only when transport is explicitly host-proxy-capable (macos, where local-fs fallback IS the intended offline behavior). Keeps the cycle-2 behavior for macos fall-through while restoring the safety guarantee for legacy callers. Adds one regression test per executor (read/write/edit/transfer) for the undefined-transport + target_client_id + no-proxy → reject path. Co-Authored-By: Claude Opus 4.7 --- .../src/tools/host-filesystem/edit.test.ts | 21 ++++++++++++++ assistant/src/tools/host-filesystem/edit.ts | 20 ++++++------- .../src/tools/host-filesystem/read.test.ts | 17 +++++++++++ assistant/src/tools/host-filesystem/read.ts | 20 ++++++------- .../tools/host-filesystem/transfer.test.ts | 29 +++++++++++++++++++ .../src/tools/host-filesystem/transfer.ts | 21 +++++++------- .../src/tools/host-filesystem/write.test.ts | 18 ++++++++++++ assistant/src/tools/host-filesystem/write.ts | 20 ++++++------- 8 files changed, 126 insertions(+), 40 deletions(-) diff --git a/assistant/src/tools/host-filesystem/edit.test.ts b/assistant/src/tools/host-filesystem/edit.test.ts index 6dbb1895384..b4c81a975ed 100644 --- a/assistant/src/tools/host-filesystem/edit.test.ts +++ b/assistant/src/tools/host-filesystem/edit.test.ts @@ -127,4 +127,25 @@ describe("host_file_edit cross-client guards", () => { expect(result.content).not.toContain("is no longer connected"); expect(result.content.toLowerCase()).toMatch(/not found|enoent|editing/); }); + + test("rejects when target_client_id is set but transport metadata is missing (legacy/backwards-compat path)", async () => { + const workingDir = makeTempDir(); + const result = await hostFileEditTool.execute( + { + path: "/some/host/path.txt", + old_string: "foo", + new_string: "bar", + target_client_id: "abc-123", + }, + // transportInterface intentionally undefined (legacy callers). + makeContext(workingDir, undefined), + ); + // Without transport metadata, falling through to local fs would + // silently target the daemon container. The guard fires for undefined + // transport AND non-host-proxy transports — only macos turns skip it. + expect(result.isError).toBe(true); + expect(result.content).toContain( + 'target client "abc-123" is no longer connected', + ); + }); }); diff --git a/assistant/src/tools/host-filesystem/edit.ts b/assistant/src/tools/host-filesystem/edit.ts index c4be612cbbb..ddd5685ede2 100644 --- a/assistant/src/tools/host-filesystem/edit.ts +++ b/assistant/src/tools/host-filesystem/edit.ts @@ -126,21 +126,21 @@ class HostFileEditTool implements Tool { }; } - // Guard: explicit targetClientId provided on a non-host-proxy transport - // but proxy is unavailable (client disconnected between tool-definition - // and tool-execution). Scoped to !supportsHostProxy so macos turns — - // where local-fs fallback IS the intended offline behavior — still fall - // through if the LLM auto-fills a stale target_client_id from a prior - // cross-client turn. On web/ios, the call must fail loudly rather - // than silently target the daemon container's filesystem. + // Guard: explicit targetClientId provided but proxy is unavailable. + // Fires on non-host-proxy transports (web, ios) AND on legacy callers + // without transport metadata, where falling through to local fs would + // silently target the daemon container's filesystem instead of the + // intended host client. Skips only when transport is explicitly + // host-proxy-capable (macos), where local-fs fallback IS the intended + // offline behavior — a stale target_client_id auto-filled from a prior + // cross-client turn is silently ignored on those turns. // Note: this scoping deliberately differs from host_bash // (host-shell.ts:239-247), which rejects unconditionally for any // stale target_client_id regardless of transport. if ( targetClientId != null && - transportInterface != null && - !supportsHostProxy(transportInterface) && - !HostFileProxy.instance.isAvailable() + !HostFileProxy.instance.isAvailable() && + (transportInterface == null || !supportsHostProxy(transportInterface)) ) { return { content: `Error: target client "${targetClientId}" is no longer connected. The specified client may have disconnected since the tool was called. Run \`assistant clients list --capability host_file\` to see currently connected clients.`, diff --git a/assistant/src/tools/host-filesystem/read.test.ts b/assistant/src/tools/host-filesystem/read.test.ts index c29fe90c1fe..02462669e9a 100644 --- a/assistant/src/tools/host-filesystem/read.test.ts +++ b/assistant/src/tools/host-filesystem/read.test.ts @@ -109,4 +109,21 @@ describe("host_file_read cross-client guards", () => { expect(result.content).toContain("File not found"); expect(result.content).not.toContain("is no longer connected"); }); + + test("rejects when target_client_id is set but transport metadata is missing (legacy/backwards-compat path)", async () => { + const workingDir = makeTempDir(); + const result = await hostFileReadTool.execute( + { path: "/some/host/path.txt", target_client_id: "abc-123" }, + // transportInterface intentionally undefined (legacy callers). + makeContext(workingDir, undefined), + ); + // When transport metadata is missing we cannot rule out a non-host-proxy + // turn, so falling through to local fs would silently target the daemon + // container. The guard fires for both undefined transport AND + // non-host-proxy transports — only macos turns skip it. + expect(result.isError).toBe(true); + expect(result.content).toContain( + 'target client "abc-123" is no longer connected', + ); + }); }); diff --git a/assistant/src/tools/host-filesystem/read.ts b/assistant/src/tools/host-filesystem/read.ts index ba8a5193c1c..842bb5ca4a7 100644 --- a/assistant/src/tools/host-filesystem/read.ts +++ b/assistant/src/tools/host-filesystem/read.ts @@ -97,21 +97,21 @@ class HostFileReadTool implements Tool { }; } - // Guard: explicit targetClientId provided on a non-host-proxy transport - // but proxy is unavailable (client disconnected between tool-definition - // and tool-execution). Scoped to !supportsHostProxy so macos turns — - // where local-fs fallback IS the intended offline behavior — still fall - // through if the LLM auto-fills a stale target_client_id from a prior - // cross-client turn. On web/ios, the call must fail loudly rather - // than silently target the daemon container's filesystem. + // Guard: explicit targetClientId provided but proxy is unavailable. + // Fires on non-host-proxy transports (web, ios) AND on legacy callers + // without transport metadata, where falling through to local fs would + // silently target the daemon container's filesystem instead of the + // intended host client. Skips only when transport is explicitly + // host-proxy-capable (macos), where local-fs fallback IS the intended + // offline behavior — a stale target_client_id auto-filled from a prior + // cross-client turn is silently ignored on those turns. // Note: this scoping deliberately differs from host_bash // (host-shell.ts:239-247), which rejects unconditionally for any // stale target_client_id regardless of transport. if ( targetClientId != null && - transportInterface != null && - !supportsHostProxy(transportInterface) && - !HostFileProxy.instance.isAvailable() + !HostFileProxy.instance.isAvailable() && + (transportInterface == null || !supportsHostProxy(transportInterface)) ) { return { content: `Error: target client "${targetClientId}" is no longer connected. The specified client may have disconnected since the tool was called. Run \`assistant clients list --capability host_file\` to see currently connected clients.`, diff --git a/assistant/src/tools/host-filesystem/transfer.test.ts b/assistant/src/tools/host-filesystem/transfer.test.ts index a2a6cf8e958..093eac27213 100644 --- a/assistant/src/tools/host-filesystem/transfer.test.ts +++ b/assistant/src/tools/host-filesystem/transfer.test.ts @@ -338,6 +338,35 @@ describe("host_file_transfer cross-client guards", () => { expect(toSandboxCalls.length).toBe(0); }); + test("rejects when target_client_id is set but transport metadata is missing (legacy/backwards-compat path)", async () => { + const workingDir = makeTempDir(); + const srcDir = makeTempDir(); + const srcFile = join(srcDir, "source.txt"); + writeFileSync(srcFile, "content"); + const destFile = join(workingDir, "should-not-exist.txt"); + + const result = await hostFileTransferTool.execute( + { + source_path: srcFile, + dest_path: destFile, + direction: "to_sandbox", + target_client_id: "abc-123", + }, + // transportInterface intentionally omitted (legacy callers). + makeContext(workingDir), + ); + + // Without transport metadata, falling through to executeLocal would + // silently target the daemon container. The guard fires for undefined + // transport AND non-host-proxy transports — only macos turns skip it. + expect(result.isError).toBe(true); + expect(result.content).toContain( + 'target client "abc-123" is no longer connected', + ); + expect(existsSync(destFile)).toBe(false); + expect(toSandboxCalls.length).toBe(0); + }); + test("does NOT reject on macos transport with a stale target_client_id when proxy unavailable (regression: Devin-flagged scope drift fix)", async () => { const workingDir = makeTempDir(); const srcDir = makeTempDir(); diff --git a/assistant/src/tools/host-filesystem/transfer.ts b/assistant/src/tools/host-filesystem/transfer.ts index 1b20f153525..294eabfb276 100644 --- a/assistant/src/tools/host-filesystem/transfer.ts +++ b/assistant/src/tools/host-filesystem/transfer.ts @@ -124,21 +124,22 @@ class HostFileTransferTool implements Tool { }; } - // Guard: explicit targetClientId provided on a non-host-proxy transport - // but proxy is unavailable (client disconnected between tool-definition - // and tool-execution). Scoped to !supportsHostProxy so macos turns — - // where local-fs fallback IS the intended offline behavior — silently - // ignore a stale target_client_id auto-filled from a prior cross-client - // turn. On web/ios, the call must fail loudly rather than silently - // target the daemon container's filesystem. + // Guard: explicit targetClientId provided but proxy is unavailable. + // Fires on non-host-proxy transports (web, ios) AND on legacy callers + // without transport metadata, where falling through to executeLocal + // would silently target the daemon container's filesystem instead of + // the intended host client. Skips only when transport is explicitly + // host-proxy-capable (macos), where local-fs fallback IS the intended + // offline behavior — a stale target_client_id auto-filled from a prior + // cross-client turn is silently ignored on those turns. // Note: this scoping deliberately differs from host_bash // (host-shell.ts:239-247), which rejects unconditionally for any // stale target_client_id regardless of transport. if ( targetClientId != null && - context.transportInterface != null && - !supportsHostProxy(context.transportInterface) && - !HostTransferProxy.instance.isAvailable() + !HostTransferProxy.instance.isAvailable() && + (context.transportInterface == null || + !supportsHostProxy(context.transportInterface)) ) { return { content: `Error: target client "${targetClientId}" is no longer connected. The specified client may have disconnected since the tool was called. Run \`assistant clients list --capability host_file\` to see currently connected clients.`, diff --git a/assistant/src/tools/host-filesystem/write.test.ts b/assistant/src/tools/host-filesystem/write.test.ts index 82eaa4278ab..23a6d51152b 100644 --- a/assistant/src/tools/host-filesystem/write.test.ts +++ b/assistant/src/tools/host-filesystem/write.test.ts @@ -113,4 +113,22 @@ describe("host_file_write cross-client guards", () => { expect(result.isError).toBe(false); expect(existsSync(destFile)).toBe(true); }); + + test("rejects when target_client_id is set but transport metadata is missing (legacy/backwards-compat path)", async () => { + const workingDir = makeTempDir(); + const destFile = join(workingDir, "should-not-exist.txt"); + const result = await hostFileWriteTool.execute( + { path: destFile, content: "hello", target_client_id: "abc-123" }, + // transportInterface intentionally undefined (legacy callers). + makeContext(workingDir, undefined), + ); + // Without transport metadata, falling through to local fs would + // silently target the daemon container. The guard fires for undefined + // transport AND non-host-proxy transports — only macos turns skip it. + expect(result.isError).toBe(true); + expect(result.content).toContain( + 'target client "abc-123" is no longer connected', + ); + expect(existsSync(destFile)).toBe(false); + }); }); diff --git a/assistant/src/tools/host-filesystem/write.ts b/assistant/src/tools/host-filesystem/write.ts index 1df11782be8..f07325c2355 100644 --- a/assistant/src/tools/host-filesystem/write.ts +++ b/assistant/src/tools/host-filesystem/write.ts @@ -96,21 +96,21 @@ class HostFileWriteTool implements Tool { }; } - // Guard: explicit targetClientId provided on a non-host-proxy transport - // but proxy is unavailable (client disconnected between tool-definition - // and tool-execution). Scoped to !supportsHostProxy so macos turns — - // where local-fs fallback IS the intended offline behavior — still fall - // through if the LLM auto-fills a stale target_client_id from a prior - // cross-client turn. On web/ios, the call must fail loudly rather - // than silently target the daemon container's filesystem. + // Guard: explicit targetClientId provided but proxy is unavailable. + // Fires on non-host-proxy transports (web, ios) AND on legacy callers + // without transport metadata, where falling through to local fs would + // silently target the daemon container's filesystem instead of the + // intended host client. Skips only when transport is explicitly + // host-proxy-capable (macos), where local-fs fallback IS the intended + // offline behavior — a stale target_client_id auto-filled from a prior + // cross-client turn is silently ignored on those turns. // Note: this scoping deliberately differs from host_bash // (host-shell.ts:239-247), which rejects unconditionally for any // stale target_client_id regardless of transport. if ( targetClientId != null && - transportInterface != null && - !supportsHostProxy(transportInterface) && - !HostFileProxy.instance.isAvailable() + !HostFileProxy.instance.isAvailable() && + (transportInterface == null || !supportsHostProxy(transportInterface)) ) { return { content: `Error: target client "${targetClientId}" is no longer connected. The specified client may have disconnected since the tool was called. Run \`assistant clients list --capability host_file\` to see currently connected clients.`,