refactor(tools): move category to ToolDefinition, collapse LoadedTool into Tool by vellum-apollo-bot[bot] · Pull Request #32472 · vellum-ai/vellum-assistant

vellum-apollo-bot · 2026-05-28T21:02:25Z

Updated per follow-up:

Move category into the tool definition. category?: string now lives on ToolDefinition alongside description / defaultRiskLevel / etc., not on a downstream wrapper.

Keep LoadedTool = Required<ToolDefinition>. Per directive, the loaded shape stays Required<ToolDefinition> & { name }, with finalizeTool filling a category: "" default (deny-by-default under Slack allowedToolCategories).

Pick one of Tool / LoadedTool. Counted refs in assistant/:

Tool: 485 (production 279)
LoadedTool: 21 (production 15)

Kept Tool, deleted LoadedTool. All 21 ref sites swept.

Final diff: 8 files, +32/-25.

assistant/src/tools/types.ts — category? onto ToolDefinition, Tool = Required<ToolDefinition> & { name }, wrapper deleted
assistant/src/tools/tool-defaults.ts — category: "" default + finalizeTool stamps it
assistant/src/tools/registry.ts, assistant/src/plugins/{types,external-plugin-loader}.ts, assistant/src/tools/execution-target.ts — LoadedTool → Tool
assistant/src/__tests__/plugin-{tool-contribution,types}.test.ts — same rename + category: "" on literals

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c4f7e277a5

ℹ️ 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".

chatgpt-codex-connector · 2026-05-28T21:05:50Z

+      return provider().map((tool) => ({
+        owner,
+        tool,
+        category: categories?.get(tool.name),


Preserve categories for external skill tools

When an external provider is registered through the existing SkillHost adapter, no categoriesProvider is supplied (daemon-skill-host still forwards only the owner and provider), but the tools it returns can still carry their manifest category (for example the meet-join factories return category: "meet"). This line records only categories?.get(...), so those boot-time external skill tools are installed with an undefined category; in Slack channels using allowedToolCategories, isToolAllowedInChannel then rejects them even when the configured category should allow them. Fall back to the tool's legacy category field when no sibling category map is provided, or update the adapter to supply one.

Useful? React with 👍 / 👎.

devin-ai-integration

Devin Review found 2 potential issues.

View 4 additional findings in Devin Review.

devin-ai-integration · 2026-05-28T21:06:21Z

+): { tools: Tool[]; categories: Map<string, string> } {
+  const tools = entries.map((entry) =>
    createSkillTool(entry, skillDir, versionHash, bundled),
  );
+  const categories = new Map<string, string>();
+  for (const entry of entries) {
+    categories.set(entry.name, entry.category);
+  }
+  return { tools, categories };


🔴 Incomplete migration: test mocks of createSkillToolsFromManifest still return Tool[] instead of { tools, categories }

The PR changed createSkillToolsFromManifest to return { tools: Tool[]; categories: Map<string, string> } (at assistant/src/tools/skills/skill-tool-factory.ts:96), and conversation-skill-tools.ts:315 now destructures const { tools, categories } = createSkillToolsFromManifest(...). However, four test files still mock this function to return a plain Tool[] array:

assistant/src/__tests__/conversation-skill-tools.test.ts:102

assistant/src/__tests__/skill-projection-feature-flag.test.ts:125

assistant/src/__tests__/conversation-app-control-instantiation.test.ts:108

assistant/src/__tests__/skill-projection.benchmark.test.ts:107

When the production code destructures { tools, categories } from a plain array, both tools and categories are undefined. The subsequent if (tools.length > 0) at conversation-skill-tools.ts:322 throws TypeError: Cannot read properties of undefined (reading 'length'), crashing every test that exercises projectSkillTools.

Prompt for agents

The return type of createSkillToolsFromManifest changed from Tool[] to { tools: Tool[]; categories: Map<string, string> } but four test files still mock it with the old signature. Each mock needs to be updated to return { tools: [...], categories: new Map([...]) } instead of a bare array. Affected files: 1. assistant/src/__tests__/conversation-skill-tools.test.ts — the mock.module for ../tools/skills/skill-tool-factory.js around line 94-113 2. assistant/src/__tests__/skill-projection-feature-flag.test.ts — same mock pattern around line 117-136 3. assistant/src/__tests__/conversation-app-control-instantiation.test.ts — same mock pattern around line 100-118 4. assistant/src/__tests__/skill-projection.benchmark.test.ts — same mock pattern around line 100-117 In each case, the mock's createSkillToolsFromManifest should wrap its return in { tools: entries.map(...), categories: new Map(entries.map(e => [e.name, e.category])) } to match the new production signature.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-05-28T21:06:22Z

+export function setToolCategory(name: string, category: string): void {
+  categoryByName.set(name, category);
 }


🚩 setToolCategory exported but unused in this PR

setToolCategory at assistant/src/tools/registry.ts:189-191 is exported for use by IPC / MCP / skill synthesis sites that set categories outside the registerTool call. It's currently not called anywhere in the diff — the IPC route in registries.ts:234-243 passes categories via registerSkillTools(skillId, proxies, { categories }) instead. This function may be intended for future use or external callers not visible in this diff. Not a bug, but worth confirming it has a consumer or removing it.

Was this helpful? React with 👍 or 👎 to provide feedback.

…y shape CI Test job failed on PR #32472 with `TypeError: undefined is not an object (evaluating 'tools.length')` in projectSkillTools. Four test files mocked `createSkillToolsFromManifest` returning the old bare `Tool[]` shape, but the PR's production change makes it return `{ tools, categories: Map<string, string> }` so categories travel as a sibling map (`Tool` no longer carries category at the runtime level). Updated mocks: - assistant/src/__tests__/conversation-app-control-instantiation.test.ts - assistant/src/__tests__/conversation-skill-tools.test.ts - assistant/src/__tests__/skill-projection-feature-flag.test.ts - assistant/src/__tests__/skill-projection.benchmark.test.ts Each mock now returns `{ tools, categories }`, drops the `category` field from individual tool literals (the source of the duplication this PR removes), and populates a per-entry category Map mirroring the real factory body. The benchmark mock additionally dropped a spurious `skillId` positional argument and `owner` field — neither is on the real factory signature; both were leftovers from an earlier refactor. Local: `bun test` on the three failing test files passes 72/72; `bunx tsc --noEmit` clean.

dvargasfuertes · 2026-05-29T15:09:23Z

+export interface ExplicitToolEntry {
+  tool: Tool;
+  category: string;
+}


We do not want to introduce a new type that just moves category somewhere else. it seems like category might be worth optionally keeping? how does it get used after explicitTools?

Traced. Category flows: declaration → registry → exactly one read site, by name.

explicitTools / hostTools / skill-manifest entries / IPC / MCP / plugin → registerTool / registerSkillTools / registerExternalTools / ... → categoryByName: Map<string, string> → getToolCategory(name) — ONE call site — → tool-approval-handler.ts:408 → isToolAllowedInChannel → Slack channel allowedToolCategories

Nothing reads tool.category from the struct after registration. The field on Tool was purely the declaration mechanism — the wire from author-of-the-tool to the registry.

Given that, you're right that ExplicitToolEntry is just relocation. Two paths:

Option A (this PR): drop the field, plumb a sibling map through every batch-registration boundary. 50 files.

Option B (your hint): category?: string optional on Tool. Literals declare it inline (as they already do). registerTool reads tool.category ?? options?.category for the registry. No wrapper type, no { tools, categories } factory return, no categoriesProvider plumbing. ~5 files.

Going Option B. Bonus: it closes the Codex bot's external-tools concern (meet/IPC tools that carry category: "meet" inline on their synthesized literal are preserved automatically — no categoriesProvider needed) and Devin's setToolCategory is vestigial under Option B so I'll drop it.

Keeping the proxy-execute.ts centralization and the stale comment removal — those are unrelated to the category question. Pushing the reshape shortly.

Pushed Option B. PR is now a single-file change: Tool.category becomes category?: string.

assistant/src/tools/types.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

Force-pushed (the Option A commits c4f72e77a5 + 953d355d35 are dropped from the branch but preserved in the PR thread history). Title updated to match. PR description still describes Option A — I'll update that next.

Done. Three changes in this push (cef3be5227):

category?: string moves onto ToolDefinition (line 339, sibling to description/defaultRiskLevel/etc.) — first-class author-facing field, no longer relocated to a wrapper.

LoadedTool kept as Required<ToolDefinition> semantics, but renamed to Tool. Counted refs in assistant/ — Tool 485 vs LoadedTool 21 — so kept Tool, deleted LoadedTool. All 21 ref sites swept.

finalizeTool stamps category: "" as the new default (deny-by-default under Slack allowedToolCategories).

Result: Tool is the single loaded type, Required<ToolDefinition> & { name: string }. The wrapper indirection (interface Tool extends LoadedTool { category }) is gone. 8 files, +32/-25.

Trace of category usage across the assistant: declaration sites (explicitTools, hostTools, skill manifest, IPC, MCP, plugin synthesis) -> registerTool reads tool.category to stamp categoryByName -> getToolCategory(name) - one production read site - -> tool-approval-handler -> isToolAllowedInChannel -> Slack channel allowedToolCategories The field on Tool is the declaration wire between tool authors and the registry. It is consumed at registration time, then queried by name. Tools whose category is fixed at synthesis time (e.g. plugin -> "plugin", MCP -> "mcp") set it via the registry directly and never needed the field. Make category optional so synthesized tools can omit it on the literal when the registry already knows the value. Existing tool literals are unchanged.

Two follow-ups to cef3be5 ('relax Tool.category to optional'): 1. **Lint** — `src/tools/tool-defaults.ts` import order tripped `simple-import-sort/imports`. `eslint --fix` resolves to alphabetical: `RiskLevel, Tool, ToolDefinition, ToolExecutionResult`. 2. **Type Check** — `src/__tests__/plugin-bootstrap.test.ts:372` constructed a `Tool` literal omitting `category`. With `Tool = Required<ToolDefinition> & { name }`, marking `ToolDefinition.category?` optional doesn't relax `Tool` itself — `Required<>` strips the question mark, so `Tool` still requires `category`. The test was constructing a finalized `Tool`, not an author-facing `ToolDefinition`, so it needs the field. Added `category: 'test'` matching the surrounding pattern in this file. Verified locally: `bun test src/__tests__/plugin-bootstrap.test.ts` 14/14 pass; `eslint src/tools/tool-defaults.ts` clean.

dvargasfuertes · 2026-05-29T19:43:03Z

 /** Tool after the loader has derived its name and filled defaults. */
-export type LoadedTool = Required<ToolDefinition> & {
+export type Tool = Required<ToolDefinition> & {
  name: string;


In the next PR, let's move this field into ToolDefinition too as optional, noting that we default to using the file name if not specified

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.

…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.

…type core tools (#32631) * refactor(tools): move name to ToolDefinition, delete ToolManifest, retype 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. * fix(tools): typecheck after ToolDefinition widening 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. * fix(tools): update shell-observability test to widened ToolDefinition 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. * refactor(tools): convert class-based tools to ToolDefinition literals + 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. * fix(tools): make ToolDefinition.execute required + widen input_schema 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. * fix(tools): keep execute optional on ToolDefinition, bang test call sites 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. * fix(tools): bang skillLoadTool.execute in managed-skill-lifecycle test * refactor(tools): consolidate to one schema + finalizeTool at IPC boundary 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. * fix(tools): convert skillLoadTool + ask-question to satisfies, finalize 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`. * fix(tests): drop widening return type from makeToolWithStub in ask-question 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. --------- Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>

vellum-apollo-bot Bot requested review from awlevin, noanflaherty and siddseethepalli as code owners May 28, 2026 21:02

chatgpt-codex-connector Bot reviewed May 28, 2026

View reviewed changes

devin-ai-integration Bot reviewed May 28, 2026

View reviewed changes

dvargasfuertes reviewed May 29, 2026

View reviewed changes

vellum-apollo-bot Bot force-pushed the apollo/drop-tool-category branch from 953d355 to f05a0a2 Compare May 29, 2026 18:29

vellum-apollo-bot Bot changed the title ~~refactor(tools): drop redundant category field from Tool~~ refactor(tools): relax Tool.category to optional May 29, 2026

vellum-apollo-bot Bot force-pushed the apollo/drop-tool-category branch 2 times, most recently from fa05d29 to 320f764 Compare May 29, 2026 18:59

vellum-apollo-bot Bot force-pushed the apollo/drop-tool-category branch from 320f764 to cef3be5 Compare May 29, 2026 19:10

vellum-apollo-bot Bot changed the title ~~refactor(tools): relax Tool.category to optional~~ refactor(tools): move category to ToolDefinition, collapse LoadedTool into Tool May 29, 2026

dvargasfuertes approved these changes May 29, 2026

View reviewed changes

dvargasfuertes merged commit 7e77ebd into main May 29, 2026
13 checks passed

dvargasfuertes deleted the apollo/drop-tool-category branch May 29, 2026 19:43

vellum-apollo-bot Bot mentioned this pull request May 29, 2026

refactor(tools): move name to ToolDefinition, delete ToolManifest, retype core tools #32631

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tools): move category to ToolDefinition, collapse LoadedTool into Tool#32472

refactor(tools): move category to ToolDefinition, collapse LoadedTool into Tool#32472
dvargasfuertes merged 2 commits into
mainfrom
apollo/drop-tool-category

vellum-apollo-bot Bot commented May 28, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 28, 2026

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

devin-ai-integration Bot May 28, 2026

Uh oh!

devin-ai-integration Bot May 28, 2026

Uh oh!

dvargasfuertes May 29, 2026

Uh oh!

vellum-apollo-bot Bot May 29, 2026

Uh oh!

vellum-apollo-bot Bot May 29, 2026

Uh oh!

vellum-apollo-bot Bot May 29, 2026

Uh oh!

dvargasfuertes May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

vellum-apollo-bot Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot May 28, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 29, 2026

Choose a reason for hiding this comment

Uh oh!

vellum-apollo-bot Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

vellum-apollo-bot Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

vellum-apollo-bot Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

dvargasfuertes May 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vellum-apollo-bot Bot commented May 28, 2026 •

edited

Loading