fix(tool-projection): self-review cleanup for host_file_* exposure#29621
Merged
credence-the-bot[bot] merged 1 commit intoMay 5, 2026
Conversation
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: Claude Opus 4.7 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
credence-the-bot
Bot
merged commit a0261f9
into
credence-the-bot/host-file-cross-client-exposure-plan
13 checks passed
credence-the-bot Bot
added a commit
that referenced
this pull request
May 5, 2026
…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>
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
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
Phase 4 round-1 cleanup addressing minor integration-review gaps from the self-review pass on PR #29613:
assistant-event-hub.jsin transfer.test.ts to match the read/write/edit test isolation patternPart of plan: host-file-cross-client-exposure-plan.md (fix round 1)