feat(host-proxy): Phase 3 — targetClientId routing for host_file_transfer#29440
Merged
Conversation
… message envelopes (#29431) * feat(host-proxy): add targetClientId support to HostTransferProxy and message envelopes * fix(tests): add listClientsByCapability and getClientById to host-transfer-proxy mock --------- Co-authored-by: credence-the-bot <credence@vellum.ai>
…outes; add target_client_id to tool (#29434) * feat(routes): enforce x-vellum-client-id ownership on host-transfer routes; add target_client_id to tool * fix(tests): await .rejects assertions; guard executeLocal fallthrough on targetClientId --------- Co-authored-by: credence-the-bot <credence@vellum.ai>
…eaders for host_transfer (#29435) * feat(macos): targetClientId acceptance guard and x-vellum-client-id headers for host_transfer * fix(swift): use setValue (not addValue) for extraHeaders in GatewayHTTPClient.get --------- Co-authored-by: credence-the-bot <credence@vellum.ai>
Contributor
Author
|
@codexbot review |
Contributor
Author
|
@devin-ai-integration[bot] please review |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🚩 Pre-existing: resolveTransferContentGetHeaders consumes transfer content after handler already did
The resolveTransferContentGetHeaders function at assistant/src/runtime/routes/host-transfer-routes.ts:65-84 calls match.proxy.getTransferContent(transferId) which is single-use (deletes the entry from the transfers map). But the HTTP adapter in assistant/src/runtime/routes/http-adapter.ts:107-125 runs the handler first, then calls resolveResponseHeaders. So by the time resolveTransferContentGetHeaders runs, the handler at line 52 has already consumed the entry via getTransferContent. The function always falls through to the default { "Content-Type": "application/octet-stream" } return, meaning Content-Length and X-Transfer-SHA256 response headers are never set on GET transfer content responses. This is pre-existing (not introduced by this PR) and doesn't cause functional breakage since the Swift client's pullTransferContent doesn't inspect those headers, but it means the documented response headers are never sent.
(Refers to lines 65-84)
Was this helpful? React with 👍 or 👎 to provide feedback.
noanflaherty
pushed a commit
that referenced
this pull request
May 3, 2026
…content (#29449) The HTTP adapter calls 'r.handler' BEFORE 'resolveResponseHeaders' (http-adapter.ts:107-125), so when 'handleTransferContentGet' consumes the entry via 'getTransferContent', the entry is gone by the time 'resolveTransferContentGetHeaders' runs. The resolver silently fell through to the default '{ Content-Type: application/octet-stream }' — the documented 'Content-Length' and 'X-Transfer-SHA256' response headers were never sent. Fix: HostTransferProxy stashes size/sha256 in a 'justConsumedMetadata' Map synchronously inside 'getTransferContent' (before the entry is deleted), and exposes 'takeJustConsumedTransferMetadata' which the header resolver reads + clears on response. A 30s 'unref()'d timer clears stale entries if the resolver never runs (e.g., handler error after consume, abort). This is pre-existing — Devin flagged it on PR #29440 as r3178602466. The Swift client's 'pullTransferContent' doesn't currently inspect either header, so there's no observable functional change today; the fix restores the documented response contract. Tests: 4 new cases in 'takeJustConsumedTransferMetadata' covering post-consume read, single-use semantics, never-consumed transferIds, and unknown ids in the presence of a different consumed transfer. Co-authored-by: credence-the-bot <credence@vellum.ai>
credence-the-bot Bot
added a commit
that referenced
this pull request
May 5, 2026
…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>
noanflaherty
pushed a commit
that referenced
this pull request
May 5, 2026
…hen capable client connected (#29632) * fix(tool-projection): expose host_file_* on cross-client transports when 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> * 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] <credence-the-bot[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> * fix(tool-projection): replace PR cross-reference in divergence comment 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> * fix(tool-projection): close backwards-compat hole in host_file_* disconnected-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> --------- Co-authored-by: credence-the-bot[bot] <277301654+credence-the-bot[bot]@users.noreply.github.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>
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
host_file_transfer, the final host-proxy tool familytargetClientIdsupport toHostTransferProxyand all 3 message envelopes, with upfront client validation and ambiguity guard in the toolx-vellum-client-idownership on the 3 transfer HTTP routes (GET/PUT content, POST result), closing the gap where any connected client could claim a targeted transferX-Vellum-Client-Idheader on all transfer HTTP callsWhat changed
PR 1 — Core proxy + envelopes (#29431)
message-types/host-transfer.ts:targetClientId?: stringon all 3 envelopeshost-transfer-proxy.ts: resolution block (validate/auto-resolve) beforenew Promise; per-transfertargetClientIdstored inTransferEntry; all 4 cancel sites (2 abort handlers,cancel(),dispose()) threaded;getTargetClientIdForTransfer()peek helperhost-transfer-proxy-targeted.test.ts(583 lines)PR 2 — Routes + tool schema (#29434)
host-transfer-routes.ts: BadRequestError (missing header) / ForbiddenError (mismatch) on GET/PUT content and POST resulttransfer.ts:target_client_idinput param; multi-client ambiguity guard; targeted-but-no-proxy error guardhost-transfer-routes-targeted.test.ts(447 lines, 4 cases × 3 routes)openapi.yaml: 400/403 additionalResponses on all 3 transfer routesPR 3 — Swift/macOS client (#29435)
MessageTypes.swift:targetClientIdonHostTransferRequestEventStreamClient.swift: targeted pass-through inshouldIgnoreHostToolRequestAppDelegate+ConnectionSetup.swift:isTargeted || isUntargetedLocalguard forhostTransferRequestGatewayHTTPClient.swift:extraHeadersongetoverload (setValue, consistent with post/put)HostProxyClient.swift:X-Vellum-Client-Idheader onpostTransferResult,pullTransferContent,pushTransferContentTest plan
host-transfer-proxy-targeted.test.ts: requestToHost/requestToSandbox with valid target, auto-resolve (1 client), unknown client, incapable client, abort handler cancel, cancel() and dispose() cancel paths, timeout, getTargetClientIdForTransfer, regressionhost-transfer-routes-targeted.test.ts: targeted+correct/missing/wrong × 3 routes; pending entry NOT consumed on 400/403host-transfer-proxy.test.ts: existing single-client flows regression (mock updated for new hub methods)🤖 Generated with Claude Code