refactor(tools): move name to ToolDefinition, delete ToolManifest, retype core tools#32631
Conversation
vellum-apollo-bot
Bot
requested review from
awlevin and
siddseethepalli
as code owners
![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: b99899588d
ℹ️ 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".
| } | ||
| let tool = finalizedByDefinition.get(definition); | ||
| if (!tool) { | ||
| tool = finalizeTool(definition, name); |
There was a problem hiding this comment.
When a class-based core tool is registered, this now stores the shallow object returned by finalizeTool() rather than the original instance. That changes the receiver for prototype execute methods: in local/no-proxy host file transfer, HostFileTransferTool.execute() reaches this.executeLocal(...), but the finalized plain object does not inherit that private prototype method, so host_file_transfer throws a TypeError instead of copying the file. Binding execute to the original definition or preserving the prototype avoids breaking class tools that call helper methods through this.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
What would it take to make these all objects instead of class + instances? What do we lose? Was making these class + instances an intentional decision?
There was a problem hiding this comment.
Resolved by converting all 22 class-based tools to plain ToolDefinition literals in 867810963c. There are no instances or prototype chains anymore, so finalizeTool can't lose a method receiver — it just spreads the literal and fills defaults.
Two tools needed light surgery to lose this:
host-filesystem/transfer.ts:executeLocalis now a module-level function.ask-question/ask-question-tool.ts: the constructor-injectedprompterFactoryis now acreateAskQuestionTool(prompterFactory?)factory;askQuestionToolis the default-wired export.
The finalizedByDefinition WeakMap stays — its purpose now is idempotent re-registration (silent no-op for test resets / module re-imports), not prototype preservation.
There was a problem hiding this comment.
Done in 867810963c. All 22 class-based tools are now plain ToolDefinition literals. What we lose: nothing material. What changed:
- No
this. 20 of 22 tools never usedthis.to begin with — pure mechanical conversion. The two that did:host-filesystem/transfer:private async executeLocal(...)lifted to a module-level function.ask-question: constructor-injectedprompterFactorybecame acreateAskQuestionTool(prompterFactory?)factory;askQuestionToolis the default-wired export.
- No factories required for the rest. Every other tool is a single
export const fooTool: ToolDefinition = { name, description, input_schema, defaultRiskLevel, category, executionTarget, async execute(...) { ... } }. finalizeToolis now structural-only. It spreads the literal, fills defaults, and returns a new object — no prototype concerns left. ThefinalizedByDefinitionWeakMap stays only for idempotent re-registration (silent no-op for test resets / module re-imports).- Schema consolidation (your other comment).
ToolDefinitionSchemaintools/types.tsis the source of truth;WireToolDefinitionSchemais.required({...})over it.ToolDefinitionisz.infer<...> & { execute? }.
Why classes felt right earlier: TypeScript's autocomplete on class bodies is nicer than on object literals, and the abstract base hinted at "must implement execute". But neither outweighs the runtime simplification — and ToolDefinition typed on the literal still gives field-level completion. Lint catches missing execute at the call site instead of at type-check time.
…type core tools Closes the ToolDefinition refactor cycle started by #32362 (executionMode) and #32472 (category) — every tool author writes a `ToolDefinition` literal, the registry finalizes it into a `Tool`. ## name → ToolDefinition (PR #32472 follow-up review comment) `name?: string` is now an optional field on `ToolDefinition` with a JSDoc note that loaders default it to the source file basename (`tools/read.ts` → `read`). `Tool` collapses from `Required<ToolDefinition> & { name: string }` to bare `Required<ToolDefinition>` since name is now part of the base shape. `finalizeTool(tool, defaultName)` now prefers `tool.name` over the file-derived default, so a literal that overrides the file name still wins. ## Delete ToolManifest The Zod-derived `ToolManifest` type in `ipc/skill-routes/registries.ts` was a near-duplicate of `ToolDefinition` minus `execute` (functions don't cross IPC). Replaced with `ToolDefinition` directly; renamed the schema to `WireToolDefinitionSchema` to reflect that it's the wire form of the canonical type. `buildProxyTool(definition: ToolDefinition)` uses `!` assertions on fields the Zod schema guarantees are set. ## Core tools as ToolDefinition Every core tool literal — 22 `class XxxTool implements ToolDefinition` sites plus 14 `: ToolDefinition = { ... }` object literals across `apps/`, `ui-surface/`, `computer-use/` definitions — now uses the relaxed authoring contract. Aggregate lists (`explicitTools`, `cesTools`, `coreAppProxyTools`, `allUiSurfaceTools`) are typed `ToolDefinition[]`. `registerTool` widens to accept `ToolDefinition` and finalizes internally via a `WeakMap<ToolDefinition, Tool>` so idempotent re-registration (test reset helpers, ESM module re-imports) stays the same silent no-op the old `existing === tool` check provided. Throws if `tool.name` is missing — the file-basename default is a loader concern, not a registry concern. ## Verified locally - `eslint src/tools src/ipc/skill-routes/registries.ts` — clean. - `bun test plugin-bootstrap plugin-tool-contribution plugin-types registry skill-tool-manifest tool-executor tool-approval-handler conversation-skill-tools conversation-app-control-instantiation skill-projection-feature-flag` — 263/263 pass. - Broader `bun test src/tools/` reproduces a 410/489 pass baseline identical to `origin/main` HEAD — no new regressions.
PR #32472 widened `ToolDefinition` (made name/description/etc. optional). This commit fixes the cascade of typecheck failures in core-tool tests + two production-side mismatches. ## Registry returns Tool[], not ToolDefinition[] `getAllToolDefinitions()` and `getMcpToolDefinitions()` previously returned `ToolDefinition[]`. At runtime they always return finalized `Tool` objects (full Required<ToolDefinition>); the loose typing was lying. Tightening to `Tool[]` fixes: - `conversation.ts` and `conversation-tool-setup.ts` `.name` reads that became `string | undefined` under the widened type. - Cross-package assignability to `@vellumai/skill-host-contracts`'s stricter `ToolDefinition` (which requires name/description/input_schema). - `btw-routes.ts` passing the result to `runBtwSidechain` whose `tools` parameter is the contracts-package shape. ## buildProxyTool takes Zod-inferred wire shape `buildProxyTool(definition: ToolDefinition)` was being passed elements from `z.array(WireToolDefinitionSchema)`. The Zod-inferred type has strict field types (e.g. `defaultRiskLevel: "low"|"medium"|"high"`) that don't match assistant's enum-typed `ToolDefinition` even though they're structurally equivalent. Switching the parameter to `z.infer<typeof WireToolDefinitionSchema>` aligns the types and drops the `!` assertions / `as` casts that the prior version needed. ## Tests: ! on .execute() calls and tool.description 13 test files invoke `.execute()` directly on imported tool exports typed as `ToolDefinition`. Added non-null assertions matching the runtime invariant — these tools are class instances or fully-specified literals where every field is implemented. Also: `computer-use-tools.test.ts` `schema()` helper now accepts `input_schema?: object` instead of requiring it; `background-shell-bash` and `terminal-tools` local `shellTool: Tool` declarations changed to `shellTool: ToolDefinition` matching the module export type; `credential-execution-tools` drops an explicit `(t: Tool)` callback annotation that tripped the variance check.
PR #32595 ('Observability for subprocess orphans') merged into main after this branch opened. It introduced shell-observability.test.ts using `let shellTool: Tool` + `shellTool.execute(...)` — the same pattern fad576a fixed in the other shell tests. CI's typecheck on the merge commit catches this; rebasing surfaces the file so the fix goes in-branch. Same mechanical fix: `Tool` → `ToolDefinition` for the variable type, `execute!` on the four call sites.
vellum-apollo-bot
Bot
force-pushed
the
apollo/name-to-tool-definition
branch
from
fad576a to
ee1d51b
Compare
| * over IPC. | ||
| * Wire form of a {@link ToolDefinition} sent over IPC by a skill process. | ||
| * Identical structurally to {@link ToolDefinition} except `execute` is | ||
| * dropped (a closure cannot cross the socket) — the daemon synthesizes |
There was a problem hiding this comment.
We should be able to move this schema to tools/types.ts, and then ToolDefinition is just an infer of the schema so that these two are consolidated
There was a problem hiding this comment.
Done in 867810963c. WireToolDefinitionSchema now lives in tools/types.ts as ToolDefinitionSchema.required({ name, description, input_schema, defaultRiskLevel, category }) (with executionTarget left optional so the daemon's resolveExecutionTarget still gets to fill it).
ToolDefinition is z.infer<typeof ToolDefinitionSchema> & { execute?: ... } — the schema is the source of truth for both the in-process author shape and the IPC wire shape, with execute added back as a TS-only overlay since closures can't cross the socket.
Bonus: defaultRiskLevel is now z.enum(RiskLevel) (the native enum from skill-host-contracts), so the as RiskLevel cast in buildProxyTool is gone — the inferred type already IS RiskLevel.
| } | ||
| let tool = finalizedByDefinition.get(definition); | ||
| if (!tool) { | ||
| tool = finalizeTool(definition, name); |
There was a problem hiding this comment.
What would it take to make these all objects instead of class + instances? What do we lose? Was making these class + instances an intentional decision?
… + consolidate schemas
Replaces all 22 class-based tools with plain object literals typed as
`ToolDefinition`, addressing PR review feedback. Class+instance
indirection is gone; `finalizeTool` no longer has anything to do with
prototypes.
Schema consolidation:
- `ToolDefinitionSchema` (tools/types.ts) is the single source of truth.
`WireToolDefinitionSchema = ToolDefinitionSchema.required({...})` with
`executionTarget` optional.
- `ipc/skill-routes/registries.ts` imports the canonical schema; the
local duplicate and `RiskLevel` cast are removed
(`z.enum(RiskLevel)` infers as the native enum).
Refactors:
- ask-question: constructor-injected `prompterFactory` becomes a
`createAskQuestionTool()` factory; default export `askQuestionTool`.
Test file updated to import the literal + factory.
- host-filesystem/transfer: `executeLocal` lifted from a private method
to a module-level function (registry stores finalized literal
references, so `this`-based dispatch is unavailable).
- 20 other tools converted to literals (zero `this.` usage).
Tests:
- ask-question 23/23, host-filesystem 29/29, memory 14/14, network
web-search 36/36 pass in isolation. Broad-batch failures are
pre-existing Bun mock-leakage: baseline at merge-base shows
167 fail / 537 tests; this branch shows 79 fail / 489 tests under
the same conditions.
… to object CI typecheck caught three regressions from the schema-driven type: 1. `ExecutionTarget` type import was removed by accident while restructuring the import block. Restored. 2. `ToolDefinition.execute` was inferred as optional (because the schema doesn't include execute), so every test calling `tool.execute(...)` failed with TS18048. Changed the type overlay to require execute — every in-process tool literal has one, and the wire schema still parses without it (closures can't cross IPC; `buildProxyTool` synthesizes execute on arrival). 3. `input_schema` was inferred as `Record<string, unknown>` from `z.record(z.string(), z.unknown())`, which is stricter than the prior `object` interface field. Factories and tests assigning typed JSON-schema objects (`as object`) failed. Widened the overlay to `input_schema?: object` so authors can keep using JSON schema literals; wire form still parses to `Record<string, unknown>` via `WireToolDefinitionSchema`. Also updated `managed-skill-lifecycle.test.ts` to use the literal `skillLoadTool` import instead of the (removed) `SkillLoadTool` class.
…ites Reverting the previous `execute: required` change — there are schema-only ToolDefinitions in the codebase (`graphRememberDefinition`, `storeStyleAnalysisTool`, `SWEEP_TOOL`) that are handed to providers as function-calling schemas without ever being registered for execution. Requiring execute on the type would break those legitimate use cases, plus the plugin-api contract test that asserts an empty literal satisfies `ToolDefinition`. Instead: keep `execute` optional, and add the `!` bang to test call sites (mirroring the existing pattern in `host-filesystem/*.test.ts`). Sed-replaced 111 call sites across 8 test files. Affected tools: credentialStoreTool, fileListTool, makeAuthenticatedRequestTool, recallTool, rememberTool, runAuthenticatedCommandTool, webSearchTool. Also added a comment in types.ts documenting why execute is optional and the schema-only ToolDefinition pattern, with pointers to the three existing instances.
|
|
||
| describe("bash tool background mode", () => { | ||
| let shellTool: Tool; | ||
| let shellTool: ToolDefinition; |
There was a problem hiding this comment.
For all of these files, we can avoid the execute!(...) by just importing the shell tool above instead of the inline import in the before each, then remove this let and type definition
There was a problem hiding this comment.
Done — refactored 3 Pattern A test files (background-shell-bash, shell-observability, terminal-tools) and dropped .execute!( bangs across all 27 affected tests.
The root cause of the bangs was : ToolDefinition = { ... } widening execute back to optional at the export site. Switched the 28 tool source files to ... } satisfies ToolDefinition so the inferred per-export type preserves execute as required at static-import call sites. ToolDefinition itself keeps execute optional because schema-only tools like graphRememberDefinition, storeStyleAnalysisTool, and SWEEP_TOOL legitimately omit it.
Two test files intentionally kept the explicit annotation:
openai-responses-provider.test.ts— passes the shape asToolDefinitionto the provider, doesn't call.execute.plugin-api-tool-definition.test.ts— contract test that assertsconst tool: ToolDefinition = {}compiles (the empty-literal contract).
One file (shell-observability.test.ts) had to keep the dynamic import in beforeEach — it mocks ../util/logger.js and shell.ts captures the logger at module-eval time, so a static import would hoist past mock.module() and the test would see the real pino logger. Used let shellTool: typeof import("../tools/terminal/shell.js")["shellTool"] so the satisfies-narrowed shape is preserved (no bang needed) with a comment explaining the ordering requirement. Confirmed the ordering behavior with a probe in /tmp/bunmock-chain.
| // `WireToolDefinitionSchema` is the single source of truth for the wire | ||
| // form of a tool. It lives in `tools/types.ts` alongside the loose | ||
| // `ToolDefinitionSchema` that `ToolDefinition` is inferred from, so both | ||
| // the in-process author shape and the IPC wire shape derive from one | ||
| // schema declaration. |
There was a problem hiding this comment.
Delete this comment
There was a problem hiding this comment.
Done — deleted the comment block. Updated ToolDefinitionSchema's own doc to reflect that author and wire share one schema and the daemon re-finalizes locally.
| const RegisterToolsParams = z.object({ | ||
| skillId: z.string().min(1), | ||
| tools: z.array(ToolManifestSchema).min(1), | ||
| tools: z.array(WireToolDefinitionSchema).min(1), |
There was a problem hiding this comment.
This should be ToolDefinitionSchema
There was a problem hiding this comment.
Done — RegisterToolsParams.tools is now z.array(ToolDefinitionSchema).min(1).
| function buildProxyTool(manifest: ToolManifest): Tool { | ||
| // RiskLevel is a string enum whose values are "low" | "medium" | "high", | ||
| // matching the schema above exactly — the cast is a no-op at runtime. | ||
| type WireToolDefinition = z.infer<typeof WireToolDefinitionSchema>; |
There was a problem hiding this comment.
We don't need this type WireToolDefinition is redundant with ToolDefinition
There was a problem hiding this comment.
Done — deleted along with buildProxyTool. The handler now takes ToolDefinitionSchema-parsed entries and runs them through finalizeTool.
| * 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 shouldn't need this helper at all, we have a finalizeTool to use instead
There was a problem hiding this comment.
Done — deleted buildProxyTool and routed through finalizeTool(tool, tool.name ?? "") instead. finalizeTool already synthesizes a no-op error closure when execute is absent (closures can't cross IPC), so the no-supervisor path surfaces a clear "not wired up" isError: true result instead of throwing. Updated the registries test "proxy execute throws" → "proxy execute surfaces an error result" to match.
| * the socket, so {@link buildProxyTool} synthesizes one that forwards | ||
| * invocations back over IPC. | ||
| */ | ||
| export const WireToolDefinitionSchema = ToolDefinitionSchema.required({ |
There was a problem hiding this comment.
Delete this redundant schema, we only want ToolDefinitionSchema, ToolDefinition, and Tool
There was a problem hiding this comment.
Done — deleted WireToolDefinitionSchema and its doc block in cdb986a. ToolDefinitionSchema is now the single source of truth; the IPC layer parses incoming skill tools against it directly and re-finalizes locally via finalizeTool.
…dary Round 2 feedback on PR #32631: kill the dual-schema design. types.ts: - Delete WireToolDefinitionSchema and its doc block. - ToolDefinitionSchema is the single source of truth — all fields optional, both author and wire paths share it. - Update ToolDefinition's doc to reflect the new contract and note the `satisfies ToolDefinition` pattern. registries.ts (skill-routes): - Drop the Wire-level-schemas comment block. - Use ToolDefinitionSchema in z.array(). - Delete buildProxyTool + WireToolDefinition type. - Replace tools.map(buildProxyTool) with tools.map((t) => finalizeTool(t, t.name ?? "")) — finalizeTool already synthesizes a no-op error closure for the no-supervisor path. Tool sources (28 files: terminal/shell, host-terminal/host-shell, host-filesystem/*, filesystem/*, credentials/vault, credential-execution/*, memory/register, memory/graph/tools, memory/v2/sweep-job, messaging/style-analyzer, ui-surface, computer-use/definitions, network/{web-fetch,web-search}, system/request-permission, subagent/notify-parent, skills/execute, apps/definitions): - Switch from `: ToolDefinition = { ... }` to `... } satisfies ToolDefinition` so the inferred per-export type preserves `execute` as required at call sites that statically import the literal. The schema-only definitions (graphRememberDefinition, storeStyleAnalysisTool, SWEEP_TOOL) intentionally keep `execute` absent — ToolDefinition itself still has it optional. Test files (27 files): - Drop the `.execute!(` bang now that satisfies preserves the required type at the call site. - Pattern A refactor: replace inline `await import("../tools/...")` in beforeEach with module-scope static imports for background-shell-bash and terminal-tools. shell-observability keeps a dynamic import because the test mocks `../util/logger.js` and shell.ts captures the logger at module-eval time — static import there would hoist past mock.module() and the test would see the real logger. Type-narrow via `typeof import(...)["shellTool"]` so no bang is needed. Intentional reverts: - openai-responses-provider.test.ts and plugin-api-tool-definition.test.ts keep `: ToolDefinition = { ... }` — they assert type shape (including the empty-literal contract test) rather than calling .execute, so the widened type is the right tool there. registries.test.ts: - "rejects missing required fields" → "fills defaults for partial tool entries": partial entries no longer reject (one all-optional schema + finalizeTool fills defaults). - "proxy execute throws when no supervisor is attached" → "proxy execute surfaces an error result when no supervisor is attached": finalizeTool synthesizes a no-op error closure now, not a throw.
…ze before supervisor short-circuit Round 2 CI follow-up — three type-check failures from PR #32631 round 2: 1. `skillLoadTool` (skills/load.ts) and `askQuestionTool` (ask-question/ask-question-tool.ts) source files were missed in the first satisfies pass: - `skills/load.ts` contains a regex literal (`/!`[^`]*`/g`) inside the execute body. The AST converter at scratch/satisfies-converter.ts doesn't track regex literals, so its brace matcher walked through the backticks inside the character class, lost depth tracking, and silently emitted "no spots found". Converted manually. - `ask-question/ask-question-tool.ts` wraps a factory call (`createAskQuestionTool()` returns ToolDefinition). The cleanest fix is to drop the factory's explicit return annotation and apply `satisfies ToolDefinition` inside the returned literal so the inferred return type carries the narrow shape through the export. 2. `ipc/skill-routes/registries.ts` supervisor short-circuit log + return read `tools.map((t) => t.name)` where `t.name` is `string | undefined` per the all-optional schema. Pulled `finalizeTool` ahead of the supervisor branch so both paths see a `Tool[]` (name guaranteed). 3. `ipc/skill-routes/__tests__/registries.test.ts` "fills defaults" assertion compared `installed.defaultRiskLevel` (typed `RiskLevel`) against the string literal `"medium"` — strict enum compare needs `RiskLevel.Medium`.
…estion test The helper had `tool: ToolDefinition` in its explicit return type, which collapses the satisfies-narrowed shape returned by `createAskQuestionTool()` back to the public author-facing shape (execute optional). Remove the explicit return type so callers see the inferred narrow shape and can invoke `tool.execute(...)` without a `!` bang. Also drop the now-unused `ToolDefinition` type import.
| // error. The execute closure arrives as a no-op error closure from | ||
| // `finalizeTool`; the production (supervisor) path replaces it with | ||
| // the dispatching closure installed by the manifest loader at boot. | ||
| const proxies = tools.map((tool) => finalizeTool(tool, tool.name ?? "")); |
There was a problem hiding this comment.
finalizeTool's second arg can be optional so it can house this ?? logic there and not force consumers to pass tool twice
| // Finalize before branching so both the supervisor short-circuit and | ||
| // the in-memory registration path see a `Tool[]` with guaranteed | ||
| // `name`. Skills run `finalizeTool` locally before sending, so the | ||
| // `?? ""` is a defensive empty-string default — `registerSkillTools` | ||
| // will reject an empty name on the non-supervisor path with a clear | ||
| // error. The execute closure arrives as a no-op error closure from | ||
| // `finalizeTool`; the production (supervisor) path replaces it with | ||
| // the dispatching closure installed by the manifest loader at boot. |
| export function createAskQuestionTool( | ||
| prompterFactory: () => Pick<QuestionPrompter, "prompt"> = () => | ||
| new QuestionPrompter({ broadcastMessage }), | ||
| ) { |
There was a problem hiding this comment.
we don't need this double factory. We should be able to use the same satisfies ToolDefinition pattern that we have elsewhere
There was a problem hiding this comment.
Killed in #32683 (2809e31). askQuestionTool is now a plain } satisfies ToolDefinition literal matching every other tool. The mock seam moved to the module boundary — the test uses mock.module("../../permissions/question-prompter.js", …) + a dynamic await import("./ask-question-tool.js"), which replaces both layers (the factory and the prompter-factory parameter) with a single hook.
| export type ToolDefinition = Omit< | ||
| z.infer<typeof ToolDefinitionSchema>, | ||
| "input_schema" | ||
| > & { |
There was a problem hiding this comment.
Figure out a way to make this just export type ToolDefinition = z.infer<typeof ToolDefinitionSchema>, meaning input_schema and execute are both typed correctly in ToolDefinitionSchema
There was a problem hiding this comment.
Done in #32683 (2809e31). ToolDefinitionSchema now uses z.custom<object>(predicate) for input_schema (rejects null/arrays/primitives at runtime, accepts plain objects) and z.custom<(input, ctx) => Promise<ToolExecutionResult>>() for execute (runtime no-op — the wire path drops closures anyway, and finalizeTool synthesizes a no-op error closure on arrival). ToolDefinition = z.infer<typeof ToolDefinitionSchema> — no overlay, both fields typed correctly from the schema.
…32683) Address all 4 review comments from PR #32631 (post-merge): 1. types.ts — ToolDefinitionSchema now uses z.custom<object>(predicate) for input_schema and z.custom<(input, ctx) => Promise<...>>() for execute. ToolDefinition collapses to a direct z.infer<typeof ToolDefinitionSchema> — single declaration, no Omit overlay, both fields typed correctly. z.custom on execute is a runtime no-op (the wire path drops closures anyway, finalizeTool synthesizes a no-op error closure on arrival). 2. tool-defaults.ts — finalizeTool's defaultName arg is now optional with an empty-string default. The empty-string fallback is fail-loud because registerSkillTools rejects an empty name with a clear error. 3. registries.ts — call site is now a one-liner: tools.map((t) => finalizeTool(t)). The defensive '?? ""' moved inside finalizeTool itself, so it doesn't repeat at every IPC handler. Comment trimmed to match. 4. ask-question-tool.ts — killed the createAskQuestionTool factory and the prompterFactory parameter. askQuestionTool is now a plain literal matching every other tool: '} satisfies ToolDefinition'. Test rewrite uses mock.module() on ../../permissions/question- prompter.js + a dynamic import of the tool, replacing makeToolWithStub's prompter-factory seam. Tests: 55/55 across ask-question, registries, plugin-api-tool- definition, plugin-tool-contribution. Lint clean. Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
Workspace tools are user-authored files dropped under
`<workspaceDir>/tools/<name>.{ts,js,json}` that authoritatively replace
same-named core tools at registry-load time. The original core tool is
stashed and restored on workspace-tool unregister, so workspace overrides
are non-destructive to the core baseline.
Highlights:
- New `registerWorkspaceTools` / `unregisterWorkspaceTool` /
`removeCoreToolViaWorkspace` / `restoreStrippedCoreTool` registry
entry points. Ownership is tracked in the existing `ownersByName` map
(`kind: "workspace"`, `id: <workspacePath>`) — no extra fields land
on the `Tool` object, keeping the post-#32631 `Required<ToolDefinition>`
shape pristine.
- Workspace ownership wins over plugin / skill / MCP: registrations of
the same name from those surfaces warn-skip (or throw, for skills) so
the workspace override stays canonical for its lifetime.
- `<name>.removed` sentinel strips a core tool from the live registry
while keeping it in the stash. Deleting the sentinel restores it.
Workspace-owned names can't be sentinel-stripped (the user must
remove the workspace tool file first).
- File watcher (`workspace-tools-watcher.ts`) drives hot reload:
re-imports on change, unregister on delete, re-strip / re-restore on
sentinel toggle. Uses `getToolOwner` to detect workspace ownership
(no `origin` field needed).
- Loader (`workspace-tools/loader.ts`) delegates default-filling to the
shared `finalizeTool` from `tool-defaults.ts`, layering only two
workspace-specific overrides on top: `defaultRiskLevel: "high"` (user
code from disk runs with full privilege) and a workspace-flavored
default `execute` error message.
Followup items from PR #31949 round-1 review (now incorporated):
- Generalized the `workspace-tools.md` doc example to use
`ToolDefinition` instead of `PluginTool` (comment 3295343340).
- Default-risk-level table entry shows `"high"` (comment 3295345328).
Followup from PR #32683 review (also incorporated):
- `QuestionPrompter` dropped its `{ broadcastMessage }` constructor arg
and imports `broadcastMessage` directly from `runtime/assistant-event-hub.js`.
Updated `ask-question-tool.ts` caller + `question-prompter.test.ts`
setup (the test now mocks the module export rather than injecting via
constructor; `mock.module` calls in that file also spread the real
module exports so other tests in the same `bun test` run still see
e.g. `assistantEventHub` and `setNestedValue`).
Tests: 88/88 passing across the 4 critical files (registry.test.ts,
workspace-tool-loader.test.ts, question-prompter.test.ts,
ask-question-tool.test.ts).
Workspace tools are user-authored files dropped under
`<workspaceDir>/tools/<name>.{ts,js,json}` that authoritatively replace
same-named core tools at registry-load time. The original core tool is
stashed and restored on workspace-tool unregister, so workspace overrides
are non-destructive to the core baseline.
Highlights:
- New `registerWorkspaceTools` / `unregisterWorkspaceTool` /
`removeCoreToolViaWorkspace` / `restoreStrippedCoreTool` registry
entry points. Ownership is tracked in the existing `ownersByName` map
(`kind: "workspace"`, `id: <workspacePath>`) — no extra fields land
on the `Tool` object, keeping the post-#32631 `Required<ToolDefinition>`
shape pristine.
- Workspace ownership wins over plugin / skill / MCP: registrations of
the same name from those surfaces warn-skip (or throw, for skills) so
the workspace override stays canonical for its lifetime.
- `<name>.removed` sentinel strips a core tool from the live registry
while keeping it in the stash. Deleting the sentinel restores it.
Workspace-owned names can't be sentinel-stripped (the user must
remove the workspace tool file first).
- File watcher (`workspace-tools-watcher.ts`) drives hot reload:
re-imports on change, unregister on delete, re-strip / re-restore on
sentinel toggle. Uses `getToolOwner` to detect workspace ownership
(no `origin` field needed).
- Loader (`workspace-tools/loader.ts`) delegates default-filling to the
shared `finalizeTool` from `tool-defaults.ts`, layering only two
workspace-specific overrides on top: `defaultRiskLevel: "high"` (user
code from disk runs with full privilege) and a workspace-flavored
default `execute` error message.
Followup items from PR #31949 round-1 review (now incorporated):
- Generalized the `workspace-tools.md` doc example to use
`ToolDefinition` instead of `PluginTool` (comment 3295343340).
- Default-risk-level table entry shows `"high"` (comment 3295345328).
Followup from PR #32683 review (also incorporated):
- `QuestionPrompter` dropped its `{ broadcastMessage }` constructor arg
and imports `broadcastMessage` directly from `runtime/assistant-event-hub.js`.
Updated `ask-question-tool.ts` caller + `question-prompter.test.ts`
setup (the test now mocks the module export rather than injecting via
constructor; `mock.module` calls in that file also spread the real
module exports so other tests in the same `bun test` run still see
e.g. `assistantEventHub` and `setNestedValue`).
Tests: 88/88 passing across the 4 critical files (registry.test.ts,
workspace-tool-loader.test.ts, question-prompter.test.ts,
ask-question-tool.test.ts).
Workspace tools are user-authored files dropped under
`<workspaceDir>/tools/<name>.{ts,js,json}` that authoritatively replace
same-named core tools at registry-load time. The original core tool is
stashed and restored on workspace-tool unregister, so workspace overrides
are non-destructive to the core baseline.
Highlights:
- New `registerWorkspaceTools` / `unregisterWorkspaceTool` /
`removeCoreToolViaWorkspace` / `restoreStrippedCoreTool` registry
entry points. Ownership is tracked in the existing `ownersByName` map
(`kind: "workspace"`, `id: <workspacePath>`) — no extra fields land
on the `Tool` object, keeping the post-#32631 `Required<ToolDefinition>`
shape pristine.
- Workspace ownership wins over plugin / skill / MCP: registrations of
the same name from those surfaces warn-skip (or throw, for skills) so
the workspace override stays canonical for its lifetime.
- `<name>.removed` sentinel strips a core tool from the live registry
while keeping it in the stash. Deleting the sentinel restores it.
Workspace-owned names can't be sentinel-stripped (the user must
remove the workspace tool file first).
- File watcher (`workspace-tools-watcher.ts`) drives hot reload:
re-imports on change, unregister on delete, re-strip / re-restore on
sentinel toggle. Uses `getToolOwner` to detect workspace ownership
(no `origin` field needed).
- Loader (`workspace-tools/loader.ts`) delegates default-filling to the
shared `finalizeTool` from `tool-defaults.ts`, layering only two
workspace-specific overrides on top: `defaultRiskLevel: "high"` (user
code from disk runs with full privilege) and a workspace-flavored
default `execute` error message.
Followup items from PR #31949 round-1 review (now incorporated):
- Generalized the `workspace-tools.md` doc example to use
`ToolDefinition` instead of `PluginTool` (comment 3295343340).
- Default-risk-level table entry shows `"high"` (comment 3295345328).
Followup from PR #32683 review (also incorporated):
- `QuestionPrompter` dropped its `{ broadcastMessage }` constructor arg
and imports `broadcastMessage` directly from `runtime/assistant-event-hub.js`.
Updated `ask-question-tool.ts` caller + `question-prompter.test.ts`
setup (the test now mocks the module export rather than injecting via
constructor; `mock.module` calls in that file also spread the real
module exports so other tests in the same `bun test` run still see
e.g. `assistantEventHub` and `setNestedValue`).
Tests: 88/88 passing across the 4 critical files (registry.test.ts,
workspace-tool-loader.test.ts, question-prompter.test.ts,
ask-question-tool.test.ts).
1 participant
What
Closes the
ToolDefinitionrefactor cycle started by #32362 (executionMode) and #32472 (category): every tool author writes aToolDefinitionliteral, the registry finalizes it into aTool.1.
namemoves ontoToolDefinitionPer Vargas's review comment on #32472:
name?: stringis now an optional field onToolDefinitionwith a JSDoc note that loaders default it to the source file basename (tools/read.ts→read).Toolcollapses fromRequired<ToolDefinition> & { name: string }to bareRequired<ToolDefinition>— name is now part of the base shape.finalizeTool(tool, defaultName)preferstool.nameover the file-derived default, so a literal that overrides the file name still wins.2. Delete
ToolManifestThe Zod-derived
ToolManifesttype inipc/skill-routes/registries.tswas a near-duplicate ofToolDefinitionminusexecute(functions don't cross IPC). Replaced withToolDefinitiondirectly; renamed the schema toWireToolDefinitionSchemato reflect that it's the wire form of the canonical type.buildProxyTool(definition: ToolDefinition)uses!assertions on fields the Zod schema guarantees are set.3. Core tools are
ToolDefinition, notToolEvery core tool literal — 22
class XxxTool implements ToolDefinitionsites plus 14: ToolDefinition = { ... }object literals acrossapps/,ui-surface/,computer-use/definitions — now uses the relaxed authoring contract. Aggregate lists (explicitTools,cesTools,coreAppProxyTools,allUiSurfaceTools) are typedToolDefinition[].registerToolwidens to acceptToolDefinitionand finalizes internally via aWeakMap<ToolDefinition, Tool>so idempotent re-registration (test reset helpers, ESM module re-imports) stays the same silent no-op the oldexisting === toolcheck provided. Throws iftool.nameis missing — file-basename defaulting is a loader concern, not a registry concern.Why
The "Workspace Tools" PR review (#31949) asked for the
ToolDefinitionshape to be the canonical author-facing contract for every tool source — workspace, plugin, skill, MCP, and core. PRs #32362 and #32472 liftedexecutionModeandcategoryfromTooltoToolDefinition; this PR finishes the cycle withnameand applies the same authoring contract to core-tool literals.After this lands, the order of operations to load any tool is uniform regardless of source:
ToolDefinitionliteral (omitting any field with a documented default).finalizeTool(def, fileBasename)or hands it toregisterTool, which finalizes internally.Tool.Verification
eslint src/tools src/ipc/skill-routes/registries.ts— clean.bun test plugin-bootstrap plugin-tool-contribution plugin-types registry skill-tool-manifest tool-executor tool-approval-handler conversation-skill-tools conversation-app-control-instantiation skill-projection-feature-flag— 263/263 pass.bun test src/tools/reproduces a 410/489 pass baseline identical toorigin/mainHEAD — no new regressions.Stack / follow-ups
apollo/workspace-tool-overrides) — the "generalize this toToolDefinition" directive in that review is fully discharged by the tools: drop executionMode, unify host-dispatch via execute() #32362 + refactor(tools): move category to ToolDefinition, collapse LoadedTool into Tool #32472 + this PR stack.defaultRiskLevel: "high"for workspace tools, docs cleanups,.removedsentinel, single-file API, file watcher in place of restart) stay queued for the rebase pass.cc @dvargasfuertes