tools: stamp executionTarget at load time (move Tool field to LoadedTool)#32017
Conversation
Every LoadedTool now carries executionTarget as a required field, resolved once at construction. The runtime resolveExecutionTarget just reads the field for registered tools; manifest fallback remains for the Permission Simulator's unregistered-tool path. - Class-style core tools: explicit "sandbox"/"host" alongside category - Object-literal proxy tools (ui-surface, apps, computer-use): "host" - Plugin loader applyPluginToolDefaults: defaults to "sandbox" - buildProxyTool: runs computeExecutionTarget on incoming manifest fields - Skill + MCP factories already stamped the field; type contract now requires it resolveExecutionTarget split into: - computeExecutionTarget(name, declared?, executionMode?): pure load-time fn - resolveExecutionTarget(name, manifestOverride?): runtime reader The prefix heuristic (host_*/computer_use_*) survives inside computeExecutionTarget as defense-in-depth for callers that don't supply a declared value.
vellum-apollo-bot
Bot
requested review from
awlevin and
siddseethepalli
as code owners
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| description: "tool from a skill", | ||
| category: "skill", | ||
| defaultRiskLevel: RiskLevel.Low, | ||
| executionTarget: "sandbox" as const, |
There was a problem hiding this comment.
🚩 Test mock in tool-executor.test.ts default getTool may be missing executionTarget
The diff for tool-executor.test.ts starts at line 236, meaning the mock.module('../tools/registry.js', ...) call at the top of the file was NOT modified. That default getTool mock likely returns tool objects without executionTarget. Since mock.module callbacks bypass TypeScript's structural type checking, this won't cause a compile error — but tests using the default mock where the executor reads tool.executionTarget would get undefined. This is a test-only concern (not a production bug) but may cause test failures. The specific overrides (lines 336, 426) were correctly updated.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified — the default getTool mock at line 137 returns tools without executionTarget. After this round's refactor, executor.ts reads getTool(name)?.executionTarget ?? "sandbox" inline, so the missing field falls through to sandbox — same effective behavior as before. All tests in this file pass. No fixture change needed.
![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: f16f206071
ℹ️ 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".
| const executionTarget = | ||
| tool.executionTarget ?? PLUGIN_TOOL_DEFAULTS.executionTarget; |
There was a problem hiding this comment.
Defaulting every plugin tool without executionTarget to "sandbox" breaks existing proxy-style plugin tools that only set executionMode: "proxy": before this change they were treated as host tools, but now they are classified as sandbox and will skip host-specific approval/trust-rule behavior (requiresGuardianApprovalForActor checks executionTarget === "host"). This is a behavioral regression for loaded external plugins and can under-enforce guardian gates for host-running plugin actions unless every plugin is updated to add executionTarget explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved. resolveExecutionTarget (called from finalizeTool) now honors executionMode === "proxy" and returns "host" before falling back to sandbox. Plugin proxy tools that only set executionMode: "proxy" are correctly classified as host and keep the requiresGuardianApprovalForActor gate. Resolution order: declared executionTarget > executionMode === "proxy" > host_*/computer_use_* name prefix > sandbox.
| @@ -136,6 +136,7 @@ export class AskQuestionTool implements Tool { | |||
There was a problem hiding this comment.
All of these should implement ToolDefinition instead, so that we could share default methods and not need these executionTarget diffs
There was a problem hiding this comment.
Done. All class-style core tools now implements ToolDefinition (host-filesystem/, host-terminal/host-shell, terminal/shell, network/web-fetch, network/web-search, memory/register, subagent/notify-parent, system/request-permission, skills/execute, skills/load, credential-execution/, credentials/vault, ask-question, filesystem/* — ~22 files). Class instance exports dropped their type annotations so TS infers the concrete class type. finalizeTool in tools/registry.ts fills in defaults + executionTarget at registration so no more explicit executionTarget diffs on individual tools.
| /** | ||
| * Where the tool's code physically runs — sandbox (assistant container) | ||
| * or host (guardian's device via host-bridge proxy). Resolved once at | ||
| * load time by `resolveExecutionTarget` and stamped onto every loaded | ||
| * tool. Read by `requiresGuardianApprovalForActor` (the only behavioral | ||
| * gate) and surfaced to the client UI / guardian channels on approval | ||
| * prompts. | ||
| */ |
There was a problem hiding this comment.
make this and the other fields on ToolDefinition a one liner
There was a problem hiding this comment.
Done. Every ToolDefinition field has a one-liner docstring now: description, defaultRiskLevel, input_schema, executionTarget, execute. See tools/types.ts.
There was a problem hiding this comment.
we have another tool resolution method that we should combine this with
There was a problem hiding this comment.
Done. Deleted PLUGIN_TOOL_DEFAULTS + applyPluginToolDefaults entirely. Defaults now live in TOOL_DEFAULTS and are applied by finalizeTool in tools/registry.ts, which registerPluginTools calls on every plugin tool. The external loader just pushes raw ToolInput now.
| * proxy in the registry so the rest of the tool-manifest plumbing can be | ||
| * exercised end-to-end. | ||
| */ | ||
| function buildProxyTool(manifest: ToolManifest): Tool { |
There was a problem hiding this comment.
We should delete ToolManifest and replace with ToolDefinition. Then combine this with our consolidated ToolDefinition -> LoadedTool/Tool method that we have somewhere
There was a problem hiding this comment.
Partial — buildProxyTool now returns finalizeTool({...}), so proxy tools coming through IPC share the consolidated finalization path. Full ToolManifest schema deletion + replacement with ToolDefinition over IPC is queued as a follow-up PR (~80-100 lines across ipc/skill-routes/ and downstream consumers).
| description: "A test skill tool", | ||
| category: "skill", | ||
| defaultRiskLevel: RiskLevel.Low, | ||
| executionTarget: "sandbox", |
There was a problem hiding this comment.
most of these tests should likely be ToolDefinition's
There was a problem hiding this comment.
These fixtures are currently Tool because they carry runtime-only fields (origin, ownerSkillId, ownerSkillBundled, ownerSkillVersionHash, category) that aren't on ToolDefinition — the checker exercises behavior that depends on those. If you want them slimmed to ToolDefinition + a finalizeTool wrap (where applicable), happy to do that in a follow-up. Let me know.
| * time. For unregistered tools (Permission Simulator's "what would this | ||
| * tool do?" path), falls back to manifest metadata or `computeExecutionTarget`. | ||
| */ | ||
| export function resolveExecutionTarget( |
There was a problem hiding this comment.
we only need this one, not computeExecutionTarget, and should change the signature to just accepting a ToolDefinition. We shouldn't call const tool = getTool(toolName) in this method bc by then, it's already all compiled
There was a problem hiding this comment.
Done. Collapsed both into a single pure function resolveExecutionTarget(input: ToolInput): ExecutionTarget in tools/execution-target.ts. No getTool call — operates purely on the input. Rules: declared executionTarget > executionMode === "proxy" > host_*/computer_use_* name prefix > sandbox. Called from inside finalizeTool.
| // Plugin-tool defaults + finalize live in `tools/registry.ts` (see | ||
| // `TOOL_DEFAULTS` and `finalizeTool`). Plugin tools are passed to | ||
| // `registerPluginTools` as a raw `ToolInput`, which calls `finalizeTool` | ||
| // for every tool — that path applies the shared defaults and stamps | ||
| // `executionTarget` via `resolveExecutionTarget` (which honors | ||
| // `executionMode === "proxy"` for proxy-style plugin tools). |
There was a problem hiding this comment.
Delete this comment
There was a problem hiding this comment.
Done — the breadcrumb landed with the finalizeTool work and has been dropped.
| ...allUiSurfaceTools.map(finalizeTool), | ||
| ...coreAppProxyTools.map(finalizeTool), |
There was a problem hiding this comment.
finalizeTool returns Tool, I would not expect to map this when the method requires ToolDefinition[] return type
There was a problem hiding this comment.
Reverted. The .map(finalizeTool) is gone; this file is unchanged in the narrowed PR.
| let shellTool: Tool; | ||
| // shellTool is the concrete class instance; type from the module | ||
| // export so `.execute(...)` calls don't hit the optional-execute path | ||
| // on the loose `Tool` shape. | ||
| let shellTool: typeof import("../tools/terminal/shell.js")["shellTool"]; |
There was a problem hiding this comment.
I don't expect this to need to change, please revert
There was a problem hiding this comment.
Reverted — the typing change came in with the finalizeTool consolidation, which has been dropped. Test is back to its main shape.
| const hooks = await loadHooks(pluginDir, name); | ||
| if (hooks !== undefined) plugin.hooks = hooks; | ||
|
|
||
| const tools: PluginToolRegistration[] = []; |
There was a problem hiding this comment.
We shouldn't need this PluginToolRegistration type now
There was a problem hiding this comment.
Same as above — PluginToolRegistration cleanup moves to PR B.
| ProviderResponse, | ||
| SendMessageOptions, | ||
| ToolDefinition, | ||
| ToolDefinition as ProviderToolDefinition, |
There was a problem hiding this comment.
I expect to be able to delete this type and reuse either ToolDefinition or Tool
There was a problem hiding this comment.
Deferred to PR B (finalizeTool + defaults + plugin-type consolidation). PluginToolRegistration is unchanged in this PR — narrowed scope is just the executionTarget field migration.
| // --------------------------------------------------------------------------- | ||
|
|
||
| const appOpenTool: Tool = { | ||
| const appOpenTool: Tool = finalizeTool({ |
There was a problem hiding this comment.
Why not keep this as ToolDefinition
There was a problem hiding this comment.
Reverted to ToolDefinition[]. The finalizeTool-wrapped Tool[] shape is gone with the rest of the consolidation; the proxy-array literals are back to the loose authoring type.
| @@ -1,33 +1,29 @@ | |||
| import { getTool } from "./registry.js"; | |||
| import type { ExecutionTarget } from "./types.js"; | |||
| import type { ExecutionTarget, ToolInput } from "./types.js"; | |||
There was a problem hiding this comment.
We shouldn't need this ToolInput type
There was a problem hiding this comment.
Reverted. resolveExecutionTarget now takes a structural { name; executionTarget?; executionMode? } — no ToolInput, no getTool call inside, single function.
| * literals) targets this type instead of `Tool` so the per-tool boilerplate | ||
| * stays minimal. | ||
| */ | ||
| export type ToolInput = Omit<Tool, keyof Required<ToolDefinition>> & |
There was a problem hiding this comment.
Yea we should avoid introducing this at all
There was a problem hiding this comment.
Reverted. ToolInput is gone from this PR; the type lived under the finalizeTool consolidation which has been split out to PR B.
Round 3 review (PR #32017): narrow scope back to just the executionTarget field migration. Drop the finalizeTool / TOOL_DEFAULTS / ToolInput / plugin-type consolidation work (those land as separate PRs). Changes in this commit: - types.ts: replace the multi-line executionTarget JSDoc with one-liner docstrings on every ToolDefinition field. - execution-target.ts: collapse computeExecutionTarget + resolveExecutionTarget into a single resolveExecutionTarget(tool: { name; executionTarget?; executionMode? }) with no getTool dependency — pure function. - executor.ts: read stamped executionTarget via getTool(name)?.executionTarget, falling back to the inference rules for unknown tools. - settings-routes.ts: inline lookup for the Permission Simulator path (registered tool ?? manifest override ?? inference rules). - ipc/skill-routes/registries.ts: buildProxyTool calls the unified resolveExecutionTarget with a structural argument. - tool-executor-lifecycle-events.test.ts: trim stale computeExecutionTarget comment.
vellum-apollo-bot
Bot
force-pushed
the
apollo/execution-target-loaded-tool
branch
from
7e7f49a to
58a6a87
Compare
|
Force-pushed the narrowed scope per #32017 (review) — this PR is now just the executionTarget field migration + the single-function The All 8 threads from the Round 3 review answered above. |
Replaces the plugin-loader-local applyPluginToolDefaults with a single
finalizeTool helper that lifts any ToolDefinition into a LoadedTool by
filling documented defaults and attaching the registration-time name.
TOOL_DEFAULTS replaces PLUGIN_TOOL_DEFAULTS as the constants source.
- assistant/src/tools/tool-defaults.ts (new): TOOL_DEFAULTS +
finalizeTool(def: ToolDefinition, name: string): LoadedTool.
Default execute now uses a generic 'tool {name} has no execute
implementation' message (no more 'plugin tool' branding) so the same
default works for any loader that consumes finalizeTool.
- assistant/src/plugins/external-plugin-loader.ts: deletes the local
PLUGIN_TOOL_DEFAULTS + applyPluginToolDefaults, calls finalizeTool
from the new module. Drops the now-unused RiskLevel and
ToolExecutionResult type imports.
- assistant/src/plugins/types.ts: deletes the PluginToolRegistration
alias (per review feedback on #32017). Plugin.tools is now directly
LoadedTool[]; updated JSDoc explains the loader contract.
- assistant/src/tools/types.ts: refresh stale ToolDefinition JSDoc
pointing at the now-removed applyPluginToolDefaults to instead
reference finalizeTool in tool-defaults.ts.
Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
1 participant
Summary
Narrowed scope (per Vargas Round 3 — split into 3 PRs). This PR ships only the executionTarget field migration. The defaults /
finalizeTool/ plugin-type consolidation moves to PR B; theToolManifest → ToolDefinitionIPC change moves to PR C.What's in this PR
executionTargetfromToolup toToolDefinition(optional in the authoring shape).LoadedTool = Required<ToolDefinition> & { name }now requiresexecutionTargeton every registered tool — load-time stamping enforced at the type level.resolveExecutionTarget(tool: { name; executionTarget?; executionMode? }): ExecutionTarget— pure function, nogetToolinside, no(name, declared, mode)triple. Replaces the split intocomputeExecutionTarget+resolveExecutionTarget.buildProxyToolin IPC) stampexecutionTargetonce at construction.executor.ts,settings-routes.ts) read the stamped field viagetTool(name)?.executionTargetwith the inference rules as a defensive fallback for unknown tools.ToolDefinitionfield.What's NOT in this PR (split out)
finalizeTool+TOOL_DEFAULTS+ plugin-type consolidation (PluginToolRegistrationdeletion,applyPluginToolDefaultsremoval, loader consolidation, theimplements ToolDefinitionswitch on class-style tools).ToolManifestschema →ToolDefinitionover IPC inipc/skill-routes/registries.ts.Tests
Relevant tests run green in isolation:
tool-executor,tool-executor-lifecycle-events,checker,computer-use-tools,computer-use-skill-manifest-regression,plugin-bootstrap,plugin-tool-contribution,external-plugin-loader,terminal-tools,background-shell-bash,credential-execution-tools,registry. The one failing test in this surface area (buildSanitizedEnv > injects INTERNAL_GATEWAY_BASE_URL from gateway config) reproduces onmain— pre-existing.Lint + typecheck clean.