feat(plugin-api): export ToolDefinition for tool-file authoring#31956
Conversation
The author-facing tool spec that plugin tool files and workspace tool files default-export. Lives at `@vellumai/plugin-api`'s public surface, type-only, structurally identical to the existing internal `PluginTool` shape so plugin authors can migrate by changing only the import name. This is the type the upcoming workspace-tool authoring guide (PR #31949) will reference instead of the daemon-internal `PluginTool` name, which was never publicly exported. Workspace tools and plugin tools share this surface but differ in their default risk floor: plugin tools default to \"medium\" (in-tree-vetted code), workspace tools default to \"high\" (operator-authored on-disk code). The default switch lives in each loader; this PR just adds the shared author-facing type. The name `ToolDefinition` collides with an internal type of the same name in `@vellumai/skill-host-contracts` that represents the JSON-schema bundle sent to providers. The doc comment on the new interface flags this — the imports are disjoint (different packages), so plugin authors always land on the right one. Tests cover full / empty / each risk-level literal and the narrow ToolContext exposure.
Per Vargas's review on #31956: the previous `PluginTool` type in tools/types.ts was `Omit<Tool, 5 fields> & { 4 optional fields }`, which retained ALL of Tool's internal stamps (origin, ownerPluginId, executionMode, executionTarget, owner* fields) on the author surface. Authors were told via docstring to leave them blank, but TypeScript didn't enforce it. The new `ToolDefinition` is the strict 4-field author surface — this commit unifies the two. Source-of-truth structure (mirrors the PluginToolContext/ToolContext pattern already used in this file): - `tools/types.ts` defines `PluginToolSpec` (strict author shape). All 4 fields optional. Defaults filled by the loader at registration. - `plugin-api/types.ts` re-exports as `ToolDefinition` for external authors. Docstring travels with the symbol via LSP. `LoadedPluginTool` is the post-defaults, ready-to-register shape. Overrides two fields from `Required<PluginToolSpec>`: - `defaultRiskLevel`: `RiskLevel` enum (vs the public string union), because `Tool.defaultRiskLevel` is the enum. - `execute`: rich `ToolContext`/`ToolExecutionResult` (vs the narrow public types), because `Tool.execute` is rich. The narrow→rich function cast happens once in `applyPluginToolDefaults`, the documented loader boundary. `applyPluginToolDefaults` also tightened: dropped the broad `...tool` spread so any extra runtime fields a JS-author or transpiled artifact might ship — origin, executionMode, owner stamps — get filtered out at the load boundary. TS authors are caught by the new narrow shape; this is the runtime backstop. Spoof test updated to cast through `unknown` — the narrow `LoadedPluginTool` no longer surfaces those fields, so we simulate a hostile/transpiled artifact arriving with spoofed fields baked in. The bootstrap-side defense is the second layer that must hold even when the type-level defense is bypassed. Typecheck, lint, all 21 affected tests: pass.
|
Pushed What changed:
Why two names (PluginToolSpec internal, ToolDefinition public): Matches the existing convention in Verification:
The naming-collision question with |
| * (`RiskLevel.Low === "low"`); the loader casts to the enum at the | ||
| * registration boundary in `applyPluginToolDefaults`. | ||
| */ | ||
| defaultRiskLevel?: "low" | "medium" | "high"; |
There was a problem hiding this comment.
Make this RiskLevel
There was a problem hiding this comment.
Done. defaultRiskLevel?: RiskLevel now. RiskLevel is re-exported from @vellumai/plugin-api so authors can write defaultRiskLevel: RiskLevel.Low without reaching into internal imports.
| context: ToolContext, | ||
| ) => Promise<ToolExecutionResult>; | ||
| }; | ||
| context: PluginToolContext, | ||
| ) => Promise<PluginToolExecutionResult>; |
There was a problem hiding this comment.
Reverted toward the simple intersection. Settled on Omit<ToolDefinition, "execute"> & {required fields + rich execute} — the Omit strips the optional narrow execute from ToolDefinition so the rich override is the sole signature on the loaded shape. Without the Omit, TS produces an intersection of the two execute types and requires the function to satisfy both, which breaks because the rich ToolContext adds trustClass that the narrow one lacks.
If you'd rather have a fully simple intersection without the Omit, the alternative is making ToolDefinition.execute itself rich-typed — that gets back to ToolDefinition & {required} clean. Happy to flip if you prefer that shape; current code keeps the public surface narrow on ToolDefinition and does the widening at the loader boundary.
There was a problem hiding this comment.
Idk what this means, but in general, we don't want a separate PluginToolContext and a separate PluginToolResult, we want consolidation as much as possible. We should delete the plugin variants if we can.
There was a problem hiding this comment.
Done in 8236f27. Deleted both PluginToolContext and PluginToolExecutionResult — their fields are inlined into the unified ToolContext / ToolExecutionResult interfaces. ToolDefinition.execute now references the rich types directly, and @vellumai/plugin-api re-exports them under their canonical names (no alias dance). Plugin authors see the same shape the daemon uses internally.
| Tool, | ||
| "category" | "getDefinition" | "name" | "description" | "defaultRiskLevel" | ||
| > & { | ||
| export interface PluginToolSpec { |
There was a problem hiding this comment.
Just call this ToolDefinition instead of PluginTool and we can export this guy out of plugin-api. also let's reduce all of the comments that were added back to how they were or one liners at most
There was a problem hiding this comment.
Done in 6dca9ac. Renamed PluginToolSpec → ToolDefinition and the @vellumai/plugin-api package now re-exports it directly (no alias dance). The skill-host-contracts ToolDefinition is imported locally as ProviderToolSchema so the public name is free.
Dropped the unused bulk re-export from tools/types.ts. Two stragglers that were importing ToolDefinition (the JSON-schema bundle) from tools/types — daemon/meet-manifest-loader.ts and ipc/skill-routes/registries.ts — now import from providers/types.js like the rest of the tool files.
Comments trimmed back to one-liners + an 8-line interface docstring (was a ~35-line block).
…nTool
- Rename PluginToolSpec -> ToolDefinition (exported from plugin-api).
- defaultRiskLevel uses the RiskLevel enum (was string union).
- LoadedPluginTool reverted to a thin intersection: Omit<ToolDefinition,
"execute"> & {required fields + rich execute}. The Omit drops the
optional narrow execute so the rich override is the sole execute
signature on the loaded shape.
- Skill-host-contracts ToolDefinition aliased locally as
ProviderToolSchema; the bulk re-export from tools/types is gone since
no consumer used it (all go through providers/types). Two stragglers
(daemon/meet-manifest-loader.ts, ipc/skill-routes/registries.ts) now
import from providers/types for consistency.
- plugin-api/types: drops the alias dance, re-exports ToolDefinition
directly and exposes RiskLevel for authors.
- Comments trimmed back toward one-liners; loader narrow-execute cast
dropped (assignability holds via contravariance).
| ExecutionTarget, | ||
| Tool, | ||
| ToolContext, | ||
| ToolDefinition, |
There was a problem hiding this comment.
Remove from providers/types.ts and keep it exported from here
There was a problem hiding this comment.
Read this two ways and want to confirm the scope before I make the change:
(A) Narrow: flip just this file (and the sibling ipc/skill-routes/registries.ts) back to importing ToolDefinition from ../tools/types.js. Adapt the local definition: ToolDefinition annotation to ToolDefinition & { name: string } (since the new author-facing ToolDefinition has no name). Leave providers/types.ts as the canonical re-export of the JSON-schema-bundle shape that ~40 other files consume.
(B) Broad: drop export type { ToolDefinition } from providers/types.ts entirely. Migrate all ~40 consumers (tool files using getDefinition(): ToolDefinition, provider files with tools?: ToolDefinition[]). The new ToolDefinition (author-facing, optional fields, no name) replaces the JSON-schema-bundle one everywhere — providers handle description/input_schema as optional with runtime fallbacks. Possibly rename the JSON-schema bundle to something like ProviderToolSchema locally.
This round only does the import-alias change (8236f27 removes the direct skill-host-contracts import in tools/types.ts). I held off on (A) or (B) until you confirm — happy to do either. Which?
There was a problem hiding this comment.
Done in fff69c4 (narrow scope). Both meet-manifest-loader.ts and ipc/skill-routes/registries.ts now import the author-facing ToolDefinition from tools/types.js. The local literal uses satisfies ToolDefinition & { name: string } instead of : annotation so the literal-narrowed shape (with required description/input_schema) is preserved for the getDefinition() closure return — TS otherwise widens to the optional-fields shape and fails the ProviderToolSchema return type check on Tool.getDefinition(). The broader providers/types.ts migration (40+ consumers) is queued for a follow-up.
| * contravariance and the runtime ctx already carries the rich fields. | ||
| */ | ||
| export type LoadedPluginTool = PluginTool & { | ||
| export type LoadedPluginTool = Omit<ToolDefinition, "execute"> & { |
There was a problem hiding this comment.
This should be just Required<ToolDefinition> & { name: string }
There was a problem hiding this comment.
Done in 8236f27. LoadedPluginTool is now Required<ToolDefinition> & { name: string } — works cleanly since execute is uniformly the rich type everywhere now.
| context: ToolContext, | ||
| ) => Promise<ToolExecutionResult>; | ||
| }; | ||
| context: PluginToolContext, | ||
| ) => Promise<PluginToolExecutionResult>; |
There was a problem hiding this comment.
Idk what this means, but in general, we don't want a separate PluginToolContext and a separate PluginToolResult, we want consolidation as much as possible. We should delete the plugin variants if we can.
| ToolDefinition, | ||
| // The JSON-schema-bundle type sent to LLM providers; aliased locally so | ||
| // `ToolDefinition` is free for the author-facing tool spec defined below. | ||
| ToolDefinition as ProviderToolSchema, |
There was a problem hiding this comment.
Would delete this import, we'll deal with this package in the future
There was a problem hiding this comment.
Dropped the direct @vellumai/skill-host-contracts ToolDefinition import in 8236f27. To avoid rippling through the ~30 tool files + ~10 provider files that still consume the JSON-schema-bundle shape as ToolDefinition, I aliased it locally as ProviderToolSchema via ../providers/types.js (one indirection away from the package). Net effect: tools/types.ts no longer references the package directly, but the broader migration (renaming ToolDefinition → ProviderToolSchema in providers/types.ts and updating ~40 consumers) is deferred. Want me to take that on in this PR or queue it as a follow-up?
- Delete PluginToolContext (inline its fields into ToolContext) and
PluginToolExecutionResult (inline into ToolExecutionResult). The narrow
public/rich-internal split is gone; plugin authors see the same
ToolContext/ToolExecutionResult shape the daemon uses internally.
- ToolDefinition.execute now references the unified types directly.
- LoadedPluginTool collapses to Required<ToolDefinition> & { name: string }.
- @vellumai/plugin-api re-exports ToolContext/ToolExecutionResult directly
(no alias dance) — same names, same source.
- Drop the direct skill-host-contracts ToolDefinition import in
tools/types.ts; ProviderToolSchema is aliased via providers/types.ts
for now (single-source-of-truth migration deferred to a follow-up).
- Trim docstrings to one-liners.
30/30 plugin tests pass; typecheck/lint clean.
Switch the two proxy-tool builders to import the author-facing
ToolDefinition (from tools/types.ts) instead of the JSON-schema-bundle
re-export in providers/types.ts. The local definition literal uses
`satisfies ToolDefinition & { name: string }` so the literal narrowing
is preserved for the getDefinition() closure return.
Narrow scope per review feedback — the broader providers/types.ts
migration is deferred to a follow-up.
| * to accurately label lifecycle events for skill-provided tools. */ | ||
| executionTarget?: ExecutionTarget; | ||
| getDefinition(): ToolDefinition; | ||
| getDefinition(): ProviderToolSchema; |
There was a problem hiding this comment.
In the next PR, let's start working towards replacing/consolidating Tool with LoadedTool and the first step towards doing so will be deleting this getDefinition method
There was a problem hiding this comment.
In fact, in that PR, define Tool as an extends LoadedTool
There was a problem hiding this comment.
Followed up in #32004 — Tool now extends LoadedTool, getDefinition() deleted across all tools, registry now passes Tool[] where ToolDefinition[] was expected (structurally compatible).
There was a problem hiding this comment.
Done in #32004. LoadedPluginTool renamed to LoadedTool; Tool extends LoadedTool adds only category + ownership metadata.
Per Vargas's followup review comments on PR #31956: - Delete Tool.getDefinition() - Define Tool as extends LoadedTool (renamed from LoadedPluginTool) The new type hierarchy: - ToolDefinition: author-facing plugin spec (all fields optional) - LoadedTool = Required<ToolDefinition> & { name: string } - Tool extends LoadedTool with category + ownership metadata Migration touched every core/skill/plugin/MCP tool: - Class-style tools: getDefinition() method replaced with top-level input_schema field (hoisted from the closure return value). - Object-literal tools (ui-surface, apps, computer-use): same hoist. - registry.ts: getMcpToolDefinitions / getAllToolDefinitions no longer .map(t => t.getDefinition()) — Tool[] is structurally ToolDefinition[]. - Plugin stamping in registerPluginTools: spreads loaded tool directly instead of destructuring input_schema and rebuilding via getDefinition. - Provider-safe wrapper: spreads the tool, overrides name; no longer needs to override getDefinition. Special cases: - ask-question: hoisted optionItemsSchema to module-level OPTION_ITEMS_SCHEMA. - memory/register Remember+Recall: delegate to graphRemember/graphRecall definitions' input_schema directly. Test fixtures (27 sites): arrow-closure getDefinition shapes converted to top-level input_schema fields. Production code is now structurally fully clean of getDefinition(). Co-authored-by: ApolloBot <apollobot@vellum.ai>
1 participant
Prereq for #31949 (per review feedback). Adds
ToolDefinitionas the public author-facing tool spec on@vellumai/plugin-api.Why
Workspace tool files and plugin tool files both default-export the same shape:
{ description?, defaultRiskLevel?, input_schema?, execute? }. Today the daemon-internal type is calledPluginTooland isn't exported from the public package — the workspace-tools doc example on #31949 was broken on that detail. Switching toToolDefinitiongives both audiences a single neutral name to import.Scope
Pure addition. No runtime behavior, no rename of existing names.
assistant/src/plugin-api/types.ts— newToolDefinitioninterfaceassistant/src/plugin-api/index.ts— adds the type-only re-export + docstring entryassistant/src/__tests__/plugin-api-tool-definition.test.ts—satisfies-style shape tests (full literal / empty literal / each risk level / narrowToolContextexposure)The new type's
executeparameter is the public-narrowToolContext(PluginToolContext), not the daemon-richToolContext, so authors can't accidentally rely on daemon-internal fields.Naming collision (intentional)
ToolDefinitionalready exists in@vellumai/skill-host-contractsas the JSON-schema bundle sent to providers ({ name, description, input_schema }). That's the runtime/provider-side shape; this is the author-time shape. Different packages, disjoint imports, plugin authors always land on the right one via@vellumai/plugin-api. Doc comment on the new interface flags this explicitly.Happy to rename the schema-bundle one to
ProviderToolSchemainstead and have a single canonicalToolDefinition— say the word and I'll do the sweep.Default-risk-level: workspace = "high"
The other half of the same comment — bumping
WORKSPACE_TOOL_DEFAULTS.defaultRiskLevelfrom"medium"→"high"— lives on #31949 alongside the workspace-tool loader (which doesn't exist onmainyet). The interface doc here describes the per-origin default contract so both PRs reference the same surface.Verification
bunx tsc --noEmitcleanplugin-api-shim,plugin-types) tests still pass