refactor(host-browser): retire HOST_BROWSER_INTERFACE_PREFERENCE#30201
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ 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". |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 Stale JSDoc on request() still references removed "interface-preference resolution"
The request() method's JSDoc at assistant/src/daemon/host-browser-proxy.ts:138-140 still says "falls back to interface-preference resolution without an actor filter" but the underlying resolveTargetClient now simply returns candidates[0] (most-recently-active). This comment was not updated alongside the implementation change and now describes behavior that no longer exists. The assistant/AGENTS.md rule states: "When writing or updating comments, do not reference code that has been removed." Two other files also have stale references: factory.ts:102 and extension-cdp-client.ts:56 both mention "interface-preference order" in their targetClientId JSDoc comments.
(Refers to lines 138-140)
Was this helpful? React with 👍 or 👎 to provide feedback.
Drops the chrome-extension-first interface preference from HostBrowserProxy and removes the now-unused `getPreferredClientByCapability` method on the event hub. Auto-resolution now picks the most-recently-active host_browser client (lastActiveAt-desc), matching the host_bash / host_file_* / host_cu / host_app_control family. When the LLM needs a specific transport (e.g. Chrome Extension's `chrome.debugger` over the macOS CDP bridge), it can pass `target_client_id` explicitly via the LLM-facing param shipped in #30066. Behavior change: when a user has both Chrome Extension and macOS clients connected, host_browser previously routed to the Chrome Extension by hardcoded preference. It now routes to whichever client was active most recently. The LLM-facing `target_client_id` override (#30066) is the supported way to pin a transport. Closes the last item from the cross-client host tool tech-debt list. Was deferred per 'skip unless they bite' alongside the app_control `target_client_id` LLM-param work; lands together with PR #30200.
Devin caught three JSDoc blocks still referencing 'interface-
preference order/resolution' after the constant was deleted:
- HostBrowserProxy.request() — described the no-actor fallback
as 'interface-preference resolution'; now matches the resolver
('most-recently-active host_browser client').
- extension-cdp-client.ts — targetClientId JSDoc.
- factory.ts — targetClientId JSDoc.
Also dropped a history-style comment in the test ('chrome-
extension preference was retired now that LLMs can pick…') —
code-comments rule is current-state only.
Tests still 27/27. Lint clean.
credence-the-bot
Bot
force-pushed
the
credence/host-browser-pref-retire
branch
from
312e13c to
8846582
Compare
|
Thanks @devin-ai-integration — fixed in 8846582. Three JSDoc blocks updated (host-browser-proxy.ts:138-140 request(), extension-cdp-client.ts:56, factory.ts:102) plus a history-style comment cleaned up in the test file. Tests still 27/27, lint clean. @devin review |
1 participant
Drops the chrome-extension-first interface preference from
HostBrowserProxyand removes the now-unusedgetPreferredClientByCapabilitymethod on the event hub. Auto-resolution now picks the most-recently-activehost_browserclient (lastActiveAt-desc), matching the rest of the host-tool family.Why
Pre-#30066,
HOST_BROWSER_INTERFACE_PREFERENCE = ["chrome-extension", "macos"]was the only way for the proxy to pick a transport when a user had both connected. After #30066, the LLM can pin a transport viatarget_client_id. The hardcoded preference is now arbitrary tech debt — every other host-tool capability (host_bash,host_file_*,host_cu,host_app_control) auto-resolves bylastActiveAt-desc and letstarget_client_iddo the override work.This was the second of the two "skip unless they bite" items from the cross-client host tool arc. Lands alongside #30200 (
target_client_idforapp_control_*) which is the first.Behavior change
When a user has both Chrome Extension and macOS host_browser clients connected:
The LLM-facing
target_client_idoverride (#30066) is the supported way to pin a specific transport — that's what it's for. Single-client setups (the common case) are unaffected.What changed
assistant/src/daemon/host-browser-proxy.ts— DeleteHOST_BROWSER_INTERFACE_PREFERENCEconstant.resolveTargetClientsimplifies tolistClientsByCapability(...)[0]for the no-actor branch andlistClientsByCapability(...).find(c => c.actorPrincipalId === sourceActorPrincipalId)for the same-actor branch.isAvailable()switches togetMostRecentClientByCapability("host_browser").assistant/src/runtime/assistant-event-hub.ts— Delete the unusedgetPreferredClientByCapabilitymethod (the only caller wasHostBrowserProxy).assistant/src/__tests__/host-browser-proxy.test.ts— Drop thegetPreferredClientByCapabilitymock andmockHasConnectionflag; consolidate onmockClients. Rewrite the "prefers chrome-extension" and "regression guard" tests to assert the newlastActiveAt-desc semantics. 27/27 tests pass.Verification
bun test src/__tests__/host-browser-proxy.test.ts→ 27/27 passbun run linton changed files → cleanbun run typecheck→ no new errors (pre-existing failures inpackages/ces-clientandpackages/service-contractsare unrelated to this PR)