tools: drop executionMode from base-list dedup; owner is sufficient#32365
Merged
Conversation
The filter at registry.ts:getAllToolDefinitions and tasks/tool-sanitizer.ts:getRegisteredToolNames was t.executionMode !== "proxy" && getToolOwner(t.name)?.kind !== "skill" Two clauses, one job: keep proxy/projection tools out of the base tool list so they don't double-up when a skill projects them. The first clause is redundant — every tool the LLM should NOT see by default is already excluded by the owner clause: - computer_use_*: not core-registered at all (#16598 removed registerComputerUseActionTools). They enter the registry only via registerSkillTools("computer-use", …) during skill projection, where their owner becomes {kind: "skill", id: "computer-use"} — owner-clause excludes them. - meet_*: registered via registerExternalTools with skill owner — owner-clause excludes them. - ui_show / ui_update / ui_dismiss: registered as core (no owner) → SHOULD appear in the base list. The old filter excluded them via executionMode, then conversation-tool-setup.buildToolDefinitions spliced them back in unconditionally. Net result identical. - app_open: same shape as ui_*. After this PR: - registry.ts:getAllToolDefinitions and tool-sanitizer.ts both filter on owner alone. - conversation-tool-setup.buildToolDefinitions stops splicing allUiSurfaceTools + coreAppProxyTools — they flow through getAllToolDefinitions naturally, identical final tool set. - registry.ts snapshot drops the vestigial allComputerUseTools.map entry from manifestToolNames (no-op since #16598). - registry.test.ts "core registry includes app_open proxy tool" flips its assertion to expect app_open in getAllToolDefinitions, which is the new (correct) behavior. executionMode stays on Tool definitions: it still drives dispatch routing via resolveExecutionTarget (host vs sandbox) and the tool-approval-handler proxyToolResolver gate. That role survives. This PR removes the field's secondary role as the base-list dedup flag — which is what PR #32362 had to rename to "projectedBySkill" to express. With this PR landed, that rename is no longer needed: the field can simply be deleted in the follow-up. ### AGENTS.md compliance - assistant/src/tools/AGENTS.md — no new tools added; refactor of existing registry/dispatch surface only.
dvargasfuertes
previously approved these changes
May 28, 2026
| * `initializeTools()`, so they flow through `getAllToolDefinitions()` | ||
| * with no need to splice them in here. | ||
| */ | ||
| export function buildToolDefinitions(): ToolDefinition[] { |
Contributor
There was a problem hiding this comment.
Looks like this method is redundant now and can be deleted
Contributor
Author
There was a problem hiding this comment.
Done in 780b237 — buildToolDefinitions() removed; the 4 callers (conversation.ts x2, btw-routes.ts, always-loaded-tools-guard.test.ts) now call getAllToolDefinitions() directly. The btw-routes.test.ts mock was repointed to tools/registry.js.
Comment on lines
+525
to
+529
| // `allComputerUseTools` is intentionally NOT listed: the computer-use | ||
| // tools are no longer core-registered (removed in #16598) — they live | ||
| // on the bundled `computer-use` skill and enter the registry via | ||
| // `registerSkillTools()` during skill projection. Listing them here | ||
| // would be a no-op since they're never in `tools` at init time. |
Contributor
There was a problem hiding this comment.
Delete this comment
review feedback: - delete redundant buildToolDefinitions() wrapper; replace 4 callers with direct getAllToolDefinitions() calls (conversation.ts, btw-routes.ts, + 2 test files). - delete historical comment about allComputerUseTools in registry.ts. ci fixes (both pre-existing on main from #32361): - add home/feed/query route-policy entry (settings.read), matching the POST endpoint introduced in #32361 but never registered. - regenerate openapi.yaml to include the new /v1/home/feed/query path (445 paths, 507 operations).
dvargasfuertes
approved these changes
May 28, 2026
vellum-apollo-bot Bot
pushed a commit
that referenced
this pull request
May 28, 2026
Eliminates the executor's mode branch in favor of self-contained execute() bodies for every host-proxied tool, then deletes the now-redundant executionMode field entirely. Why --- PR #32342 moved executionMode from `Tool` to `ToolDefinition` as a pure type-level move. This PR (alternative) goes further: it asks whether the field is needed at all once dispatch is unified. After the unification, executionMode had two genuinely different concerns conflated under one name: 1. **Dispatch** — folded into each tool's `execute()`. The executor no longer special-cases mode='proxy'; every tool's execute() handles its own forwarding (to context.proxyToolResolver for client tools, to supervisor.dispatchTool for meet tools, etc.). 2. **Skill projection dedup** — already removed in #32365, which proved the owner-only filter on `getAllToolDefinitions` and `getRegisteredToolNames` is sufficient. No replacement field needed. So executionMode just gets deleted outright. No rename. Changes ------- - Each proxy tool's `execute()` now forwards via context.proxyToolResolver with name-bound closure. No-resolver case returns a structured error result (isError: true) instead of throwing. - executor.ts drops both mode branches (initial dispatch + CES retry). - `resolveExecutionTarget` drops rule #2 (mode=proxy ⇒ host). Every declaration site that used to rely on it already stamps target='host' directly. - `tool-approval-handler.ts` drops the resolver-aware available-tools filter — execute() handles no-resolver gracefully now. - IPC schema in skill-routes/registries.ts drops executionMode from the wire payload; contracts-side Tool + client.ts manifest projection drop it too. Field count: -1 executionMode, +0. The win is dispatch unification plus the simpler tool surface that #32365 enabled.
vellum-apollo-bot Bot
pushed a commit
that referenced
this pull request
May 28, 2026
Eliminates the executor's mode branch in favor of self-contained execute() bodies for every host-proxied tool, then deletes the now-redundant executionMode field entirely. Why --- PR #32342 moved executionMode from `Tool` to `ToolDefinition` as a pure type-level move. This PR (alternative) goes further: it asks whether the field is needed at all once dispatch is unified. After the unification, executionMode had two genuinely different concerns conflated under one name: 1. **Dispatch** — folded into each tool's `execute()`. The executor no longer special-cases mode='proxy'; every tool's execute() handles its own forwarding (to context.proxyToolResolver for client tools, to supervisor.dispatchTool for meet tools, etc.). 2. **Skill projection dedup** — already removed in #32365, which proved the owner-only filter on `getAllToolDefinitions` and `getRegisteredToolNames` is sufficient. No replacement field needed. So executionMode just gets deleted outright. No rename. Changes ------- - Each proxy tool's `execute()` now forwards via context.proxyToolResolver with name-bound closure. No-resolver case returns a structured error result (isError: true) instead of throwing. - executor.ts drops both mode branches (initial dispatch + CES retry). - `resolveExecutionTarget` drops rule #2 (mode=proxy ⇒ host). Every declaration site that used to rely on it already stamps target='host' directly. - `tool-approval-handler.ts` drops the resolver-aware available-tools filter — execute() handles no-resolver gracefully now. - IPC schema in skill-routes/registries.ts drops executionMode from the wire payload; contracts-side Tool + client.ts manifest projection drop it too. Field count: -1 executionMode, +0. The win is dispatch unification plus the simpler tool surface that #32365 enabled.
dvargasfuertes
pushed a commit
that referenced
this pull request
May 28, 2026
Eliminates the executor's mode branch in favor of self-contained execute() bodies for every host-proxied tool, then deletes the now-redundant executionMode field entirely. Why --- PR #32342 moved executionMode from `Tool` to `ToolDefinition` as a pure type-level move. This PR (alternative) goes further: it asks whether the field is needed at all once dispatch is unified. After the unification, executionMode had two genuinely different concerns conflated under one name: 1. **Dispatch** — folded into each tool's `execute()`. The executor no longer special-cases mode='proxy'; every tool's execute() handles its own forwarding (to context.proxyToolResolver for client tools, to supervisor.dispatchTool for meet tools, etc.). 2. **Skill projection dedup** — already removed in #32365, which proved the owner-only filter on `getAllToolDefinitions` and `getRegisteredToolNames` is sufficient. No replacement field needed. So executionMode just gets deleted outright. No rename. Changes ------- - Each proxy tool's `execute()` now forwards via context.proxyToolResolver with name-bound closure. No-resolver case returns a structured error result (isError: true) instead of throwing. - executor.ts drops both mode branches (initial dispatch + CES retry). - `resolveExecutionTarget` drops rule #2 (mode=proxy ⇒ host). Every declaration site that used to rely on it already stamps target='host' directly. - `tool-approval-handler.ts` drops the resolver-aware available-tools filter — execute() handles no-resolver gracefully now. - IPC schema in skill-routes/registries.ts drops executionMode from the wire payload; contracts-side Tool + client.ts manifest projection drop it too. Field count: -1 executionMode, +0. The win is dispatch unification plus the simpler tool surface that #32365 enabled. Co-authored-by: vellum-apollo-bot[bot] <220839023+vellum-apollo-bot[bot]@users.noreply.github.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.
What
Drops the redundant
executionMode !== "proxy"clause from the base-list dedup filter at two sites. Owner-based exclusion alone covers every case.Before:
After:
Why
Two clauses were doing one job — keep proxy/projection tools out of the base tool list so they don't double-up when a skill projects them in. Tracing each registration path, the
executionModehalf is redundant:computer_use_*registerComputerUseActionTools); enters registry only viaregisterSkillTools("computer-use", …)during skill projection.{kind: "skill"}.meet_*registerExternalToolswith skill owner{kind: "skill"}.ui_show,ui_update,ui_dismissregisterUiSurfaceTools()buildToolDefinitionsspliced them back in.app_openregisterAppTools()ui_*.For
ui_*andapp_open, the prior logic was an exclude-then-re-add gymnastic. Net result identical to just letting them flow through.What this PR changes
assistant/src/tools/registry.ts—getAllToolDefinitionsfilter collapses to owner-only.assistant/src/tasks/tool-sanitizer.ts—getRegisteredToolNamesfilter collapses to owner-only.assistant/src/daemon/conversation-tool-setup.ts—buildToolDefinitionsstops splicingallUiSurfaceTools + coreAppProxyTools. They now flow throughgetAllToolDefinitions()directly. Identical final tool set.coreAppProxyTools,allUiSurfaceToolsinconversation-tool-setup.ts;allComputerUseToolsinregistry.ts).allComputerUseTools.map(t.name)entry frommanifestToolNamesininitializeTools()snapshot — no-op since chore: remove registerComputerUseActionTools() #16598.assistant/src/__tests__/registry.test.ts— "core registry includesapp_openproxy tool" flips its assertion to expectapp_openINgetAllToolDefinitions, matching the new (correct) behavior.What this PR explicitly does NOT change
executionModestays on every Tool definition. It still drives:resolveExecutionTarget(host vs sandbox).tool-approval-handlerproxyToolResolvergate attool-approval-handler.ts:344.ipc/skill-routes/registries.ts).The field's secondary role as a base-list dedup flag is what this PR removes. With this landed, PR #32362 no longer needs to rename it to
projectedBySkill— the field can simply be deleted in that follow-up (it'll be unused everywhere except in the literal definitions, which #32362 was already deleting along with the dispatch role).Verification
bunx tsc --noEmit— clean.bunx eslinton touched files — clean.bunx prettier --checkon touched files — clean.registry.test.ts— 27/27 passalways-loaded-tools-guard.test.ts— 1/1 pass (still 11 tools whenhasNoClient: true;isToolActiveForContextgatesui_*/app_openper turn)browser-skill-baseline-tool-payload.test.ts— 3/3 pass (count range 15–50 absorbs +4)browser-skill-endstate.test.ts— 8/8 passconversation-init.benchmark.test.ts— 8/8 passcomputer-use-tools.test.ts— 26/26 passcu-unified-flow.test.ts— 30/30 passconversation-runtime-assembly.test.ts— 212/212 passtool-executor.test.ts— 80/80 passconversation-tool-setup-app-refresh.test.ts+-tools-disabled.test.ts— 21/21 passpermissions/checker.test.ts— 31/31 pass__tests__/checker.test.ts— 132/132 passipc/skill-routes/__tests__/registries.test.ts— 16/16 passresolve-required-tools.test.ts— 5/5 passSequencing with #32362
#32362 (host-dispatch unification) is ON HOLD pending this PR's merge. Once this lands:
executionMode→projectedBySkillrename in tools: drop executionMode, unify host-dispatch via execute() #32362 will be dropped — the dedup-role of the field is already gone.tool-types.tsrename).AGENTS.md compliance
assistant/src/tools/AGENTS.md— no new tools added; refactor of existing registry/dispatch surface only.