fix(tool-projection): expose host_file_* on cross-client transports when capable client connected#29632
Merged
noanflaherty merged 4 commits intoMay 5, 2026
Conversation
…hen capable client connected (#29613) * 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<HostProxyCapability> 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 4ec0e1a 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…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] <credence-the-bot[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…t with inline rationale (#29628) 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] <credence-the-bot[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…onnected-target guard 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 <noreply@anthropic.com>
Contributor
Author
|
@chatgpt-codex-connector review P1 backwards-compat hole addressed in commit 789bea2 — guard now fires when transport metadata is missing too. Macos fall-through preserved. |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
noanflaherty
deleted the
credence-the-bot/host-file-cross-client-exposure-plan
branch
noanflaherty
pushed a commit
that referenced
this pull request
May 7, 2026
#29829) * feat(daemon): expose host_browser to LLM cross-client on web/iOS turns Adds "host_browser" to CROSS_CLIENT_EXPOSED_CAPABILITIES so the LLM- exposure carve-out in isToolActiveForContext lets web and iOS turns see the tool when at least one host_browser-capable client (macOS or chrome-extension) is connected via the assistant event hub. Routing for host_browser shipped in PR #27489 (host-browser-via-macos- host-proxy), but the LLM-exposure gate in conversation-tool-setup.ts was still hardcoded to deny non-host-proxy transports. With this change the user can drive their host browser from any interactive interface, matching the parity already extended to host_bash + host_file in PR #29632. Tests: 8 new cases mirror the host_file_* block (web exposed, ios exposed, web denied without client, ios denied without client, hasNoClient gate honored, macos and chrome-extension transports unaffected, per- capability invariant guard against hardcoded-capability regressions). Total: 44 pass / 0 fail. Defense-in-depth follow-up (deferred): host_browser does not yet adopt the same-actor guard that host_bash / host_file / host_cu / host_transfer apply on their result routes. The chrome-extension client today does not send `x-vellum-client-id` on its result POST; enforcing the guard requires a coordinated extension change. The current `requireGuardian` gate on `/v1/host-browser-result` is the active same-binding boundary. * fix(host-browser): bind host_browser proxy + result route to caller actor The original cross-client exposure change opened host_browser to web/iOS turns whenever any host-browser-capable client is connected, but did not extend the same-user actor binding that already protects host_bash, host_file, and host_cu. Codex flagged this on the parent commit as a multi-actor cross-user host-action boundary violation; Devin tracked the same gap as a deferral. This commit closes the gap by mirroring the host-cu pattern at every layer: Daemon proxy (host-browser-proxy.ts): - resolveTargetClient(sourceActorPrincipalId) filters host_browser candidate clients by actorPrincipalId before applying the chrome-extension-first interface preference. When no actor is supplied (legacy/internal flows), falls back to the prior getPreferredClientByCapability path. - request() accepts optional sourceActorPrincipalId, refuses to dispatch to a different-actor client via enforceSameActorOrErrorResult, persists targetClientId/targetActorPrincipalId on the pending interaction, and broadcasts with targetClientId so the client-id binding rides on the SSE envelope. Result route (host-browser-routes.ts): - resolveHostBrowserResultByRequestId now reads x-vellum-client-id and x-vellum-actor-principal-id headers, validates the submitter matches peeked.targetClientId (400 missing / 403 mismatch), and runs enforceSameActorOrThrow against the persisted targetActorPrincipalId. - On a 403 the pending interaction is preserved so the legitimate client can still submit. Untargeted requests pass through unchanged. - OpenAPI gains additionalResponses for 400/403/404/409. CDP client plumbing (extension-cdp-client.ts, factory.ts): - ExtensionCdpClient takes optional sourceActorPrincipalId and forwards it on every proxy.request call. - buildCandidateList and buildPinnedCandidateList read sourceActorPrincipalId from ToolContext (already populated from conversation.trustContext.guardianPrincipalId) and thread it through createExtensionCdpClient. Client-side header emission: - clients/shared/Network/HostProxyClient.swift (macOS): postBrowserResult now sends X-Vellum-Client-Id, mirroring postCuResult / postTransferResult. - clients/chrome-extension/background/worker.ts: both the SSE-path POST and the self-hosted fallback POST add X-Vellum-Client-Id from getClientId(). The Chrome-extension WS host_browser_result code path is untouched and remains dead code on the server side. SameActorOp union gains "host_browser" so the structured-warn audit log entry uses the same shape as the other host-proxy capabilities. Test fixtures: - assistant/src/__tests__/fixtures/mock-chrome-extension.ts now sends X-Vellum-Client-Id on its result POST so e2e tests do not 400. Tests added: - assistant/src/__tests__/host-browser-proxy.test.ts: 6 new tests in a "same-actor binding" describe — persists target binding when actor matches, rejects when only different-actor clients are connected, prefers chrome-extension over macos among same-actor clients, falls back to macOS bridge for same actor, legacy path still works, rejects when the only candidates are different-actor. - assistant/src/__tests__/host-browser-routes.test.ts: new "handleHostBrowserResult — same-actor guard" describe block covering targeted-correct-headers, whitespace trim, missing/wrong client-id, actor-principal mismatch, and untargeted regressions. - assistant/src/tools/browser/cdp-client/__tests__/factory.test.ts: asserts the factory threads sourceActorPrincipalId from ToolContext into createExtensionCdpClient on the extension code path. - factory.test.ts also adopts the spread-real-module mock pattern so its createExtensionCdpClient stub does not clobber ExtensionCdpClient for downstream test files (defensive, since CI test.sh already runs each file in its own bun process). Backwards compatibility: - Chrome-extension WS host_browser_result code path is dead on the server side; only the HTTP POST path is exercised by both clients. - Untargeted requests with no targetClientId continue to work, so pre-update Chrome extensions (which now will start sending the header) do not regress. - Legacy callers without sourceActorPrincipalId hit the getPreferredClientByCapability fallback unchanged. Refs: Codex P1 / Devin deferral on PR #29829. * chore(openapi): regenerate openapi.yaml for host-browser-result actor-binding responses Adds 400/403/404/409 entries for /v1/host-browser-result. No spec authoring changes — `bun run generate:openapi` output for the additionalResponses introduced in the previous commit. --------- Co-authored-by: credence-the-bot <196986132+credence-the-bot@users.noreply.github.com> Co-authored-by: credence-the-bot <credence-the-bot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Generalizes the cross-client tool-exposure carve-out in
isToolActiveForContextfrom a hardcodedhost_bashcheck to aSet<HostProxyCapability>containinghost_bashandhost_file. Phase 2 (PR #29398) and Phase 3 (PR #29440) shipped the routing infrastructure for cross-clienthost_file_*execution but never extended this exposure gate. As a result, web/iphone turns silently lacked all fourhost_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 (D5).During the per-PR review-gate cycle, Codex (P1) and Devin (BUG) flagged that exposing
host_file_*cross-client without tightening the executor-side guards would silently fall through to the daemon container's filesystem if the macOS client disconnected between projection and execution. Three follow-up fix cycles were applied:4ec0e1aebf): added "no client" + "stale targetClientId" runtime guards tohost-filesystem/{read,write,edit}.ts+ scopedtransfer.tsno-client guard.da11f13140): scoped the disconnected-target guards to non-host-proxy transports so macOS local-fs offline-mode behavior is preserved.54a2e39468): applied the same scoped pattern totransfer.ts(Devin-flagged) and removed the pre-existing unscoped post-proxy guard.Self-review rounds 1 and 2 then landed PR #29621 (iphone→ios comment renames; divergence-from-host_bash inline note; transfer message wording alignment; transfer.test.ts event-hub mock isolation) and PR #29628 (replaced "see PR #29613" cross-reference with inline rationale).
Design Doc
.private/plans/host-file-cross-client-exposure-design.mdBehavior change
host_file_read/write/edit/transfernow appear in the LLM's tool list and route to the macOS client via existing Phase 2/3 proxies.host_fileclient connected →host_file_*tools are filtered (or, if reached at execute time due to disconnect, the executor returns "no client connected" / "target client is no longer connected").target_client_idfrom a prior cross-client turn is silently ignored and the call falls through to local fs (the intended offline-mode behavior).Tests
host_file_*exposure tests inconversation-tool-setup.test.ts(4 tools × 4 scenarios) + 1 D5 regression guard.host-filesystem/{read,write,edit}.test.ts(3 new test files) and an extension totransfer.test.ts.target_client_idinhost-filesystem/{read,write,edit}.test.ts(one per executor) plus one intransfer.test.ts.mockHostBashClientCount→mockClientCountByCapability: Map<string, number>).Risk
Low — additive change. Re-uses the same chrome-extension and hasNoClient gates as the existing host_bash carve-out. Executor-side guards add fail-loudly behavior on web/ios without altering macOS behavior. No new security surface.
Plan
.private/plans/host-file-cross-client-exposure-plan.mdPRs merged into this feature branch