feat(macos): decode targetClientId from HostBashRequest and accept targeted non-local requests#29315
Conversation
…rgeted non-local requests
![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: 617c409bfa
ℹ️ 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".
| guard isLocalConversation || isTargeted else { | ||
| break |
There was a problem hiding this comment.
This acceptance guard cannot make cross-client host bash work by itself, because EventStreamClient.shouldIgnoreHostToolRequest still drops every host_bash_request whose conversationId is not locally owned before this switch ever runs (clients/shared/Network/EventStreamClient.swift, case .hostBashRequest). In the targeted-proxy path, the conversation is intentionally non-local, so the message is filtered out and isTargeted is never evaluated, leaving targeted host bash requests non-functional.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Accept if this conversation is locally owned, OR if the request | ||
| // is explicitly targeted at this client (cross-client proxy routing). | ||
| let localClientId = DeviceIdStore.getOrCreate() | ||
| let isLocalConversation = self.mainWindow?.conversationManager | ||
| .conversations.contains(where: { $0.conversationId == msg.conversationId }) ?? false | ||
| let isTargeted = msg.targetClientId == localClientId | ||
| guard isLocalConversation || isTargeted else { | ||
| break | ||
| } |
There was a problem hiding this comment.
🔴 Upstream SSE filter drops cross-client proxy bash requests before targetClientId guard can evaluate
The new targetClientId guard in AppDelegate+ConnectionSetup.swift:374 is unreachable for its intended cross-client proxy use case. EventStreamClient.shouldIgnoreHostToolRequest (clients/shared/Network/EventStreamClient.swift:607-632) runs first and drops any hostBashRequest whose conversationId is not in locallyOwnedConversationIds (line 610-612). For cross-client proxy routing, the conversation is owned by a different client, so locallyOwnedConversationIds.contains(msg.conversationId) is false, and the message is silently discarded with a warning log before handleParsedMessage ever broadcasts it to subscribers. The new guard at line 374 that checks msg.targetClientId == localClientId never gets a chance to execute, completely breaking the cross-client proxy feature.
Upstream filter that blocks the message
EventStreamClient.swift:609-612:
case .hostBashRequest(let msg):
if locallyOwnedConversationIds.contains(msg.conversationId) { return false }
log.warning("Ignoring host_bash_request for non-local conversation ...")
return trueThis needs to also pass through messages where msg.targetClientId matches the local client ID.
Prompt for agents
The cross-client proxy feature for hostBashRequest is broken because EventStreamClient.shouldIgnoreHostToolRequest (clients/shared/Network/EventStreamClient.swift, lines 607-632) drops all host_bash_request messages whose conversationId is not in locallyOwnedConversationIds — before the message reaches the new targetClientId guard in AppDelegate+ConnectionSetup.swift.
For cross-client proxy routing, the conversation is owned by a different client, so the conversation ID won't be in locallyOwnedConversationIds. The fix needs to be in shouldIgnoreHostToolRequest: the hostBashRequest case (line 609-612) should also allow through messages where msg.targetClientId matches the local client ID (DeviceIdStore.getOrCreate()). Something like:
case .hostBashRequest(let msg):
if locallyOwnedConversationIds.contains(msg.conversationId) { return false }
if msg.targetClientId == DeviceIdStore.getOrCreate() { return false }
log.warning(...)
return true
Note: The guard in AppDelegate+ConnectionSetup.swift (lines 368-376) may then be redundant with this upstream fix, or can serve as defense-in-depth. Consider whether the duplicate check in AppDelegate is still needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| case .hostFileRequest(let msg): | ||
| HostToolExecutor.executeHostFileRequest(msg) |
There was a problem hiding this comment.
🚩 Inconsistent cross-client routing guard across host tool types
The new targetClientId-based routing guard is only added for .hostBashRequest but not for .hostFileRequest, .hostCuRequest, .hostBrowserRequest, or .hostTransferRequest (AppDelegate+ConnectionSetup.swift:378-412). This may be intentional if cross-client proxy routing is only planned for bash requests, but it's worth confirming the intent — if the feature is expected to extend to other host tool types, the same pattern (and the same upstream shouldIgnoreHostToolRequest fix) would be needed for those cases as well.
(Refers to lines 378-412)
Was this helpful? React with 👍 or 👎 to provide feedback.
credence-the-bot
Bot
merged commit 17d89fe
into
Credence/targeted-host-proxy
credence-the-bot
Bot
deleted the
targeted-host-proxy/pr-8-macos-accept-targeted
branch
Summary
Note: Swift changes require host build to verify.
Part of plan: targeted-host-proxy.md (PR 8 of 8)