feat: targeted host proxy routing for cross-client host_bash execution#29322
Conversation
…tion (#29309) Co-authored-by: Credence <credence@anthropic.com>
…tant clients list (#29310) Co-authored-by: Credence <credence@anthropic.com>
…pending interaction plumbing (#29312) Co-authored-by: Credence <credence@anthropic.com>
…tClientId through broadcast (#29313) * feat(host-bash-proxy): resolve and validate target client; pass targetClientId through broadcast * fix: remove multi-client error from proxy; move ambiguity check to tool layer --------- Co-authored-by: Credence <credence@anthropic.com>
…ecutor (#29314) * feat(host-bash): add target_client_id parameter to tool schema and executor * fix: sort imports --------- Co-authored-by: Credence <credence@anthropic.com>
…rgeted non-local requests (#29315) Co-authored-by: Credence <credence@anthropic.com>
…lients (403) (#29317) * feat(host-bash-result): reject result submissions from non-targeted clients (403) * fix: regenerate openapi.yaml --------- Co-authored-by: Credence <credence@anthropic.com>
… a capable client is connected (#29316) * feat(tool-projection): include host_bash for non-host interfaces when a capable client is connected * fix: sort imports * fix: include broadcastMessage in event hub mock * fix: use plain function instead of mock.fn in event hub mock --------- Co-authored-by: Credence <credence@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![credence-the-bot[bot]](https://avatars.githubusercontent.com/in/3422848?s=60&v=4){kind=small}
There was a problem hiding this comment.
credence-the-bot review — targeted host proxy routing
Overall: solid. The core routing logic is correct end-to-end. A few things worth verifying before merge:
🟡 Swift acceptance guard assumes conversationManager.conversations is complete
AppDelegate+ConnectionSetup.swift now requires isLocalConversation || isTargeted. For untargeted (legacy macOS-origin) requests, this relies on self.mainWindow?.conversationManager.conversations containing the active conversation at the moment the host_bash_request arrives.
Before this PR, there was no guard — the client executed any request. Now if conversationManager.conversations is a lazy/partial list (e.g. only recently opened conversations), existing macOS-origin host bash could silently break.
This was flagged as Ambiguity #3 in the plan. Worth a quick check that conversationManager.conversations is always populated when a bash request can arrive, or adding a comment explaining the invariant.
🟡 Multi-client ambiguity enforcement is split across two layers
The proxy (host-bash-proxy.ts) lets multi-client + no-target fall through to untargeted broadcast with a comment that the tool executor layer handles it. That's correct — host-shell.ts returns an error before calling the proxy in that case.
But this creates a footgun if HostBashProxy.request() is ever called directly (not via the tool). Low risk now, but worth a comment in the proxy making the layered contract explicit, or adding the ambiguity check directly in the proxy.
✅ Everything else looks correct
- Hub conversation-filter bypass for targeted delivery: correct — web-origin conversationId differs from macOS subscription
- Auto-resolve for single capable client: correct
- 403/400 on result route: correct, ForbiddenError confirmed present
X-Vellum-Client-Idon result POST: correct addition to HostProxyClientextraHeadersonGatewayHTTPClient.post(): clean, backwards-compatible- Tool projection scoped to
host_bashonly (Phase 2 gate): correct - LABEL column in CLI: correct
- All Phase 2 tools (host_file, host_cu, host_browser) correctly excluded
- macOS CodingKeys camelCase passthrough for
targetClientId: correct - OpenAPI additionalResponses for 400/403: correct
Neither concern above is a blocker — the 🟡 items are verification questions, not bugs. CI is green.
|
@codexbot review |
|
@devin review |
…ross-client host_bash exception Devin cycle 1 findings: - Cross-client exception in isToolActiveForContext() was firing for chrome-extension transport, exposing host_bash to extension users when any macOS client was connected — violating the established security boundary. - Exception also bypassed the hasNoClient gate, meaning API-only turns with no interactive approval UI could see host_bash in their tool definitions. Both fixed by adding explicit guards to the cross-client condition: && transport !== 'chrome-extension' && !ctx.hasNoClient Two new tests cover the previously-missing scenarios.
| } else if (capableClients.length === 1) { | ||
| // Auto-resolve when exactly one capable client is connected. | ||
| resolvedTargetClientId = capableClients[0].clientId; |
There was a problem hiding this comment.
🚩 Auto-resolve to single client creates implicit targeting — verify intent for existing macos-only flows
In host-bash-proxy.ts:96-98, when exactly one host_bash-capable client is connected and no explicit targetClientId is provided, the proxy auto-resolves to that client. This changes the existing behavior: previously all host_bash_request messages were untargeted broadcasts; now with a single client they become targeted. The targeted path in the hub (assistant-event-hub.ts:281-289) bypasses conversation-id filtering, which is the desired behavior for cross-client routing. For the existing macos-only flow (single macOS client, conversation originated on macOS), the macOS client would have received the message anyway via conversation-id or capability matching, so the behavioral change is transparent. However, the conversation-id bypass means the event will be delivered even if the client has a mismatched conversation filter — which was always the case for cross-client but is now also the case for single-client auto-resolution. This is intentional per the test at host-bash-proxy.test.ts:560-587 but worth noting as a semantic change.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@codexbot review |
|
@devin review |
…sconnected
Devin BUG: the two guards gating non-host-proxy cross-client execution
both checked `targetClientId == null` first. When the LLM resolved an
explicit target_client_id (from a prior `assistant clients list` call),
both guards were bypassed entirely. If the target client then disconnected,
`HostBashProxy.instance.isAvailable()` returned false and execution fell
through to the local daemon spawn — silently running inside Docker instead
of on the user's machine.
Fix: add an explicit guard before the proxy block:
if (targetClientId != null && !HostBashProxy.instance.isAvailable())
→ return error with the stale client ID and recovery hint
Test: adds a unit test verifying the guard fires and no spawn occurs.
| } else if (capableClients.length === 1) { | ||
| // Auto-resolve when exactly one capable client is connected. | ||
| resolvedTargetClientId = capableClients[0].clientId; | ||
| } | ||
| // capableClients.length === 0 or > 1 without explicit target: resolvedTargetClientId | ||
| // stays undefined and falls through to untargeted broadcast — the existing timeout/error | ||
| // path handles the zero-client case, and multi-client ambiguity is enforced at the tool | ||
| // executor layer (not here) once target_client_id is exposed in the tool schema. |
There was a problem hiding this comment.
🚩 Multi-macOS-client untargeted execution remains a known limitation
When two macOS clients are connected and a macOS-originated turn invokes host_bash without an explicit target_client_id, the proxy's auto-resolve only kicks in for capableClients.length === 1 (assistant/src/daemon/host-bash-proxy.ts:96-98). With 2+ clients, resolvedTargetClientId stays undefined and the request is broadcast untargeted. The new Swift guard (isLocalConversation || isTargeted) doesn't prevent duplicate execution in this case because both macOS clients sync the same conversation list, so isLocalConversation is true for both. This matches pre-PR behavior (no guard existed before). The PR's comment at host-bash-proxy.ts:100-103 explicitly acknowledges this: "multi-client ambiguity is enforced at the tool executor layer (not here) once target_client_id is exposed in the tool schema." The multi-client guard at host-shell.ts:208-218 only applies to non-host-proxy transports (web, iOS, etc.).
Was this helpful? React with 👍 or 👎 to provide feedback.
…rgetClientId Devin found that the defense-in-depth guard used isLocalConversation || isTargeted, which allows a non-target macOS client to accept a request if the conversation is in its local list (all clients sync the same conversation list from the daemon). Fix: split into isTargeted (explicit match) and isUntargetedLocal (no targetClientId AND isLocalConversation). Guard now rejects when targetClientId points to a different client, even if isLocalConversation is true.
|
@codexbot review |
|
@devin review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdee79f73b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| let isTargeted = msg.targetClientId == localClientId | ||
| let isUntargetedLocal = msg.targetClientId == nil && isLocalConversation | ||
| guard isTargeted || isUntargetedLocal else { |
There was a problem hiding this comment.
The new targetClientId handling here is unreachable for cross-client web→macOS requests because EventStreamClient.shouldIgnoreHostToolRequest still drops every host_bash_request whose conversationId is not locally owned (see clients/shared/Network/EventStreamClient.swift, host-bash branch). In the new flow, targeted requests intentionally carry a non-local conversation ID, so they are filtered out before this switch runs and host_bash never executes on the targeted Mac. Please update the prefilter to allow targeted requests addressed to this client.
Useful? React with 👍 / 👎.
| let localClientId = DeviceIdStore.getOrCreate() | ||
| let isLocalConversation = self.mainWindow?.conversationManager | ||
| .conversations.contains(where: { $0.conversationId == msg.conversationId }) ?? false | ||
| let isTargeted = msg.targetClientId == localClientId | ||
| let isUntargetedLocal = msg.targetClientId == nil && isLocalConversation | ||
| guard isTargeted || isUntargetedLocal else { | ||
| break | ||
| } |
There was a problem hiding this comment.
🚩 macOS client mainWindow nil check could silently drop untargeted requests during startup
The untargeted path isUntargetedLocal = msg.targetClientId == nil && isLocalConversation depends on self.mainWindow?.conversationManager.conversations, which evaluates to false via ?? false when mainWindow is nil. If the SSE connection is established before the main window is created, untargeted host_bash_request events would be silently dropped.
In practice this is unlikely to cause issues because: (1) the auto-targeting in host-bash-proxy.ts targets the single client for the common case, making isTargeted true regardless of mainWindow state; (2) for macos-origin turns, the user must be interacting with the app (main window exists). However, it's a subtle behavioral change from the pre-PR code where ALL host bash requests were unconditionally processed.
Was this helpful? React with 👍 or 👎 to provide feedback.
…entStreamClient prefilter Targeted cross-client requests (web→macOS) carry the web conversation ID, which is not in locallyOwnedConversationIds on the macOS client. Without this fix, shouldIgnoreHostToolRequest() drops them before AppDelegate can perform the targetClientId check, making the entire feature non-functional. Fix: if targetClientId is set, skip the local-ownership check and let AppDelegate+ConnectionSetup.swift handle routing via isTargeted guard.
|
@chatgpt-codex-connector @devin-ai-integration cycle 4 — pushed Fix: This was the Codex P1 on fdee79f. Devin's 🚩 ANALYSIS (mainWindow nil drop during startup) is a known edge-case: the Please review |
|
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". |
| const targetClientId = | ||
| typeof input.target_client_id === "string" | ||
| ? input.target_client_id | ||
| : undefined; |
There was a problem hiding this comment.
🟡 Empty-string target_client_id bypasses multi-client ambiguity guard due to null vs truthiness mismatch
The guards in host-shell.ts use targetClientId == null (lines 209, 222) while the proxy in host-bash-proxy.ts uses if (input.targetClientId) (line 87, truthiness check). An empty string "" passes != null but fails the truthiness check. When the LLM sends target_client_id: "" on a non-host-proxy interface (e.g. web) with 2+ capable clients:
- Guard 1 at
assistant/src/tools/host-terminal/host-shell.ts:208-218is skipped ("" == null→ false) - The proxy at
assistant/src/daemon/host-bash-proxy.ts:87treats""as falsy → untargeted broadcast - Since the conversation belongs to the web turn, no macOS client's
locallyOwnedConversationIdscontains it → all clients ignore → silent timeout instead of a clear "specify which client" error.
Fix approach
Normalize the empty/whitespace-only string to undefined at extraction:
const targetClientId =
typeof input.target_client_id === "string" && input.target_client_id.trim()
? input.target_client_id.trim()
: undefined;This aligns with the trimming done for header values in host-bash-routes.ts:40 and events-routes.ts:81.
| const targetClientId = | |
| typeof input.target_client_id === "string" | |
| ? input.target_client_id | |
| : undefined; | |
| const targetClientId = | |
| typeof input.target_client_id === "string" && input.target_client_id.trim() | |
| ? input.target_client_id.trim() | |
| : undefined; |
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…arget_client_id - HostBashCancelRequest: add optional targetClientId field to type - host-bash-proxy: pass targetClientId in both abort and dispose cancel broadcasts so host_bash_cancel reaches only the client that owns the request - host-shell: normalize empty-string target_client_id to undefined so it is consistent with the proxy's truthiness guard (resolves 🟡 BUG) Addresses Devin findings on PR #29322: BUG: empty-string target_client_id bypasses multi-client ambiguity guard ANALYSIS: cancel messages for targeted requests not themselves targeted
|
Cycle 4 fixes pushed (commit
All 27 host-bash-proxy tests pass. 3 cycles required before merge — this is cycle 4. |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
|
Merged latest Root cause: commits #29374 + #29378 on main removed the CI now running on |
| if ( | ||
| capability === "host_bash" && | ||
| transport !== "chrome-extension" && | ||
| !ctx.hasNoClient && | ||
| assistantEventHub.listClientsByCapability("host_bash").length > 0 | ||
| ) { | ||
| return true; |
There was a problem hiding this comment.
🚩 Cross-client exception allows host_bash for ALL non-chrome-extension, non-macos transports (iOS, CLI, Slack channels etc.)
The cross-client exception at assistant/src/daemon/conversation-tool-setup.ts:393-399 enables host_bash for any transport that isn't chrome-extension and isn't already a host-proxy interface (macos). This includes ios, cli, web, and channel interfaces like slack, telegram, etc. The !ctx.hasNoClient guard prevents channel-originated turns (which typically have hasNoClient: true since they lack SSE clients) from activating, but ios and cli turns with connected clients would gain host_bash visibility when a macOS client is connected. Tests only cover web transport — the iOS/CLI cross-client path is implicitly enabled but untested.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
✅ By design. The cross-client exception is intentionally transport-agnostic (excluding only chrome-extension, which has its own security boundary). The purpose of this PR is precisely to enable non-desktop interfaces (web, iOS, CLI) to route host_bash to a connected macOS client.
The !ctx.hasNoClient guard already blocks all channel-originated turns (Slack, Telegram, etc.) that lack an interactive SSE client. iOS and CLI turns with a connected interactive session gaining access to a macOS client's shell is correct behavior — restricting this to web-only would defeat the feature.
Test coverage for iOS/CLI cross-client path would be a nice follow-up, but this is a coverage gap, not a security issue.
|
@noanflaherty — PR #29322 is ready to merge 🎉 CI green on
Summary of what this PR ships: |
|
Codex P1 (line 378) — fixed in The P1 flagged that That fix was applied in commit case .hostBashRequest(let msg):
// Targeted cross-client requests carry a non-local conversationId by design.
// Pass them through so AppDelegate+ConnectionSetup can perform the targetClientId check.
if msg.targetClientId != nil { return false }
if locallyOwnedConversationIds.contains(msg.conversationId) { return false }
return trueTargeted requests are explicitly passed through. @codexbot please re-review the current HEAD |
|
@codexbot please review the current HEAD |
|
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". |
Adds target_client_id to all 8 app_control_* tool input schemas (start, observe, key, key_combo, sequence, type, click, drag) and threads it through the surfaceProxyResolver dispatch branch in conversation-surfaces.ts. Mirrors the host_cu pattern in the same file: validate explicit target_client_id (existence, capability, same-actor) before dispatch; auto-resolve unique same-user client when cross-user clients are connected; error when multiple same-user clients are ambiguous. Closes the last LLM-exposure gap from the cross-client host tool arc (PR #29322 host_bash, #29398 host_file/host_cu, #30066 host_browser). Was deferred per 'skip unless they bite' but the WIP was sitting half-done in the worktree. Tests: - conversation-surfaces-app-control.test.ts: new 317-line file covering unavailability, end-to-end dispatch, local stop short- circuit, target_client_id existence/capability/same-actor checks, multi-client error path, and cross-user rejection. - app-control-flow.test.ts: extended with target_client_id checks. Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
1 participant
Summary
Enables web-origin turns to route
host_bashexecution to a connected macOS client by client ID. Before this change,host_bashwas only available on the interface that originated the turn — a web conversation could not invoke it even with a desktop client connected.Changes
X-Vellum-Machine-Nameon SSE subscription; surfaced inassistant clients listas aLABELcolumn for human-readable targetingtargetClientId?: stringadded toHostBashRequestandPendingInteractionpublish()acceptstargetClientIdto bypass conversation-scoped filtering and route directly to a specific client;broadcastMessage()threads the value into pending interaction registrationtarget_client_idadded as optional field tohost_bash; multi-client ambiguity and no-client errors surface at the tool executor layer for non-host-proxy interfaceshost_bashis now included forweb/non-host-proxy interfaces when at least one capable client is connected (host_file, host_cu, host_browser remain Phase 2)POST /v1/host-bash-resultvalidatesx-vellum-client-idheader against the pending interaction'stargetClientId; mismatches return 403 and keep the request pendingtargetClientIdfromHostBashRequest; accepts host bash requests for non-local conversations when the request is explicitly targeted at this clientSecurity
webis NOT declared a host-proxy interface — this is routed delegation, not local capabilityBackwards Compatibility
Desktop-origin
host_bashbehavior is fully preserved. Clients not sendingX-Vellum-Client-Idon result posts will get 400 only for requests that have atargetClientIdset (i.e. cross-client requests). Existing macOS-origin flows always havetargetClientId == nilso they are unaffected.Test Plan
🤖 Generated with Claude Code