tools: drop Tool.origin in favor of registry-side ownership lookup

[vellum-apollo-bot]

Continuing the Tool field-by-field triage. With ownership now living in
the registry's ownersByName map (PR #32224), Tool.origin is a
strict subset of getToolOwner(name)?.kind: "skill" / "plugin" / "mcp"
map 1:1 to owner kinds, and the absence of an owner is the canonical
signal for core tools. Keeping the field added a second source of truth
ownership consumers had to keep in sync.

Architecture

• Drop origin?: "core" | "skill" | "mcp" | "plugin" from both Tool
  interfaces (assistant/src/tools/types.ts and
  packages/skill-host-contracts/src/tool-types.ts).
• All four production write sites no longer stamp the field:
  • tools/mcp/mcp-tool-factory.ts
  • tools/skills/skill-tool-factory.ts
  • ipc/skill-routes/registries.ts (IPC handler for skill projection)
  • tools/registry.ts registerPluginTools plugin stamp
  • daemon/meet-manifest-loader.ts (daemon-side projection)
• Production read sites switch to getToolOwner(t.name)?.kind:
  • tools/policy-context.ts — skill/plugin branch
  • permissions/checker.ts — toolOrigin field plumbed to the
    gateway-side risk assessor (still "skill" / "builtin" externally)
  • tasks/tool-sanitizer.ts — filter out skill tools
• Registry-internal reads use the ownersByName map directly (same
  module, no extra indirection):
  • "is core" check collapses from existing.origin === "core" || !existing.origin
    to !ownersByName.has(tool.name) across all 3 register*Tools paths
  • getMcpToolDefinitions / getSkillToolNames / getAllToolDefinitions
    / coreToolsSnapshot filters all switch to owner-kind lookups

Test changes

• Dropped origin: "skill" from 11 test fixtures across 11 files; the
  Tool literals now match the post-refactor interface.
• Replaced 8 tool.origin / retrieved?.origin assertions with
  ownership-or-existence checks; in every case there's already a
  getToolOwner(name) assertion right next to it, so the read-side
  semantics are still covered.
• Deleted two skill-tool-factory.test.ts tests that only verified the
  factory stamps origin — that responsibility is now entirely the
  registry's, covered by registry.test.ts.
• Updated the spoofed-ownership case in plugin-tool-contribution.test.ts
  to reflect the new model: the Tool type no longer carries ownership at
  all, so forged origin/owner fields on the literal are inert extras
  the registry never reads. The bootstrap is the single source of truth.
• Added getToolOwner to the tool-executor-lifecycle-events.test.ts
  registry mock, matching the pattern already in tool-executor.test.ts,
  so production reads through policy-context.ts keep resolving.
• The daemon/__tests__/meet-manifest-loader.test.ts origin assertion
  is gone — the daemon-side projection no longer stamps the kind. The
  authoritative ownership stamp happens on the assistant side via
  registerSkillTools(skillId, tools) at the IPC boundary, covered
  separately by ipc/skill-routes/__tests__/registries.test.ts.

Verification

• bunx tsc --noEmit clean across assistant + packages/skill-host-contracts.
• bunx eslint clean across all 23 touched files.
• All touched test files pass individually:
  • registry.test.ts → 27/27
  • skill-tool-factory.test.ts → 15/15
  • plugin-tool-contribution.test.ts → 12/12
  • checker.test.ts → 132/132
  • tool-executor.test.ts → 80/80
  • tool-executor-lifecycle-events.test.ts → 17/17
  • conversation-skill-tools.test.ts → 62/62
  • conversation-app-control-instantiation.test.ts → 6/6
  • skill-projection-feature-flag.test.ts → 4/4
  • computer-use-skill-manifest-regression.test.ts → 10/10
  • daemon/meet-manifest-loader.test.ts + conversation-tool-setup-exclude
    • ipc/skill-routes/registries.test.ts → 34/34
• Full-folder bun test src/__tests__/ hits the same pre-existing
  cross-test mock contamination on bare main (getWorkspaceDirOverride
  ESM link error) — not caused by this PR. Confirmed identical baseline
  by stash + re-run.

Field landscape after this PR

ToolDefinition: description?, defaultRiskLevel?, input_schema?, executionTarget?, execute?
LoadedTool = Required<ToolDefinition> & { name }
Tool extends LoadedTool: category, executionMode?

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 52ecbe7bef

ℹ️ 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]

Clear stale ownership when core tools overwrite skills

When a boot-time external skill tool collides with a later core registration during initializeTools() (external providers are installed before host/UI/system tools), registerTool() overwrites the tools entry but leaves the old ownersByName record behind. Since this new filter now treats ownersByName as authoritative, the final core tool is still classified as skill-owned and gets dropped from the base tool list (and other owner-based checks will treat it as a third-party skill). Before this refactor, replacing the Tool object also replaced its origin, so the stale ownership could not survive; clear ownership in registerTool() when installing core tools or route these collisions through the owner-aware registration path.

Useful? React with 👍 / 👎.

[devin-ai-integration]

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

[devin-ai-integration]

🚩 Benchmark test has pre-existing mock signature mismatch for registerSkillTools

The mock in skill-projection.benchmark.test.ts:159-162 defines registerSkillTools with a single tools parameter, but production registerSkillTools takes (skillId: string, newTools: Tool[]). This means the mock receives skillId as the tools argument and tries to iterate over a string. This is pre-existing (not introduced by this PR) — the PR only removed origin: "skill" from the factory mock at line 113. The benchmark likely still runs because the string iteration produces character objects whose .name is undefined, resulting in benign benchmarkRegistry.set(undefined, ...) entries that don't affect timing measurements.

(Refers to lines 159-163)

---

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

[dvargasfuertes]

this toolOrigin field should just be the same type as ownerKind

[vellum-apollo-bot]

Picked up as a follow-up PR — keeping #32294 as the field-drop and a separate PR for the type fix so the diff stays scoped.

Follow-up: PR #32339 — #32339

• ApprovalContext.toolOrigin?: OwnerKind (matches the registry's source-of-truth union)
• IIFE in checker.ts collapses to toolOrigin: getToolOwner(toolName)?.kind
• Section 6 in approval-policy.ts spells out the previously-hidden "skill" || "plugin" disjunction
• Added a toolOrigin: "plugin" test to lock in the extension-owned routing