diff --git a/assistant/docs/plugins.md b/assistant/docs/plugins.md index 1e923712f28..20979972c1d 100644 --- a/assistant/docs/plugins.md +++ b/assistant/docs/plugins.md @@ -548,9 +548,10 @@ middleware. Each is optional. An array of `Tool` objects. The bootstrap registers them with the global tool registry after `init()` succeeds, stamping `origin: "plugin"` and -`ownerPluginId: ` so they live in a ref-count namespace -disjoint from real skills (a plugin whose `manifest.name` happens to -match a skill id cannot collide with that skill's registrations). +`owner: { kind: "plugin", id: }` so they live in a ref-count +namespace disjoint from real skills (a plugin whose `manifest.name` +happens to match a skill id cannot collide with that skill's +registrations). ```typescript const myPlugin: Plugin = { diff --git a/assistant/src/__tests__/checker.test.ts b/assistant/src/__tests__/checker.test.ts index 046a9298125..19847bd96df 100644 --- a/assistant/src/__tests__/checker.test.ts +++ b/assistant/src/__tests__/checker.test.ts @@ -109,7 +109,7 @@ import { } from "../permissions/checker.js"; import { _clearGlobalCacheForTesting } from "../permissions/gateway-threshold-reader.js"; import { RiskLevel } from "../permissions/types.js"; -import { registerTool } from "../tools/registry.js"; +import { registerSkillTools, registerTool } from "../tools/registry.js"; import type { Tool } from "../tools/types.js"; import * as platformModule from "../util/platform.js"; @@ -128,6 +128,9 @@ const STRICT_GATEWAY_THRESHOLDS = { } as const; // Register a mock skill-origin tool for testing default-ask policy. +// Ownership is recorded by `registerSkillTools(skillId, tools)`, not by +// stamping a field on the `Tool` object — `isToolOwnerSkillBundled()` reads +// from the registry via `getToolOwner(name)` to find the owning skill id. const mockSkillTool: Tool = { name: "skill_test_tool", description: "A test skill tool", @@ -135,16 +138,16 @@ const mockSkillTool: Tool = { defaultRiskLevel: RiskLevel.Low, executionTarget: "sandbox", origin: "skill", - ownerSkillId: "test-skill", input_schema: { type: "object" as const, properties: {} }, execute: async () => ({ content: "ok", isError: false }), }; -registerTool(mockSkillTool); +registerSkillTools("test-skill", [mockSkillTool]); // Register a mock bundled skill-origin tool for testing strict mode + bundled // policy. `app-builder` is a real entry under `bundled-skills/`, so // `loadSkillCatalog()` reports it as `bundled: true` and the permission -// checker derives `isSkillBundled = true` without any per-tool stamp. +// checker derives `isSkillBundled = true` from the registry's ownersByName +// map (populated by `registerSkillTools`) without any per-tool stamp. const mockBundledSkillTool: Tool = { name: "skill_bundled_test_tool", description: "A test bundled skill tool", @@ -152,11 +155,10 @@ const mockBundledSkillTool: Tool = { defaultRiskLevel: RiskLevel.Low, executionTarget: "sandbox", origin: "skill", - ownerSkillId: "app-builder", input_schema: { type: "object" as const, properties: {} }, execute: async () => ({ content: "ok", isError: false }), }; -registerTool(mockBundledSkillTool); +registerSkillTools("app-builder", [mockBundledSkillTool]); // Register CU tools so check() can look them up in the tool registry // instead of falling through to Medium (unknown tool). @@ -389,11 +391,10 @@ describe("Permission Checker", () => { defaultRiskLevel: RiskLevel.Medium, executionTarget: "sandbox", origin: "skill", - ownerSkillId: "test-skill", input_schema: { type: "object" as const, properties: {} }, execute: async () => ({ content: "ok", isError: false }), }; - registerTool(mediumSkillTool); + registerSkillTools("test-skill", [mediumSkillTool]); const result = await check("skill_medium_tool", {}, "/tmp"); expect(result.decision).toBe("prompt"); expect(result.reason).toContain("Skill tool"); diff --git a/assistant/src/__tests__/computer-use-skill-manifest-regression.test.ts b/assistant/src/__tests__/computer-use-skill-manifest-regression.test.ts index 4bd491f2d51..f4b15c650c6 100644 --- a/assistant/src/__tests__/computer-use-skill-manifest-regression.test.ts +++ b/assistant/src/__tests__/computer-use-skill-manifest-regression.test.ts @@ -123,12 +123,14 @@ describe("computer-use skill manifest regression", () => { category: "computer-use", defaultRiskLevel: RiskLevel.Low, origin: "skill" as const, - ownerSkillId: "computer-use", execute: async () => ({ content: "stub", isError: false }), }), ); - expect(() => registerSkillTools(skillTools)).not.toThrow(); + // Owner flows in through `registerSkillTools(skillId, tools)` and lands + // in the registry's `ownersByName` map — the tools themselves carry no + // per-tool owner field. + expect(() => registerSkillTools("computer-use", skillTools)).not.toThrow(); // Clean up unregisterSkillTools("computer-use"); diff --git a/assistant/src/__tests__/conversation-app-control-instantiation.test.ts b/assistant/src/__tests__/conversation-app-control-instantiation.test.ts index f9deba12e43..6e41667bb7f 100644 --- a/assistant/src/__tests__/conversation-app-control-instantiation.test.ts +++ b/assistant/src/__tests__/conversation-app-control-instantiation.test.ts @@ -98,9 +98,10 @@ mock.module("../skills/tool-manifest.js", () => ({ })); mock.module("../tools/skills/skill-tool-factory.js", () => ({ + // Mirrors the real factory: no skillId in/out — ownership is recorded by + // the registry at `registerSkillTools(skillId, tools)` time. createSkillToolsFromManifest: ( entries: SkillToolManifest["tools"], - skillId: string, _skillDir: string, _versionHash: string, _bundled?: boolean, @@ -112,25 +113,20 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ defaultRiskLevel: RiskLevel.Medium, executionTarget: "sandbox" as const, origin: "skill" as const, - ownerSkillId: skillId, input_schema: entry.input_schema as object, execute: async () => ({ content: "", isError: false }), })), })); mock.module("../tools/registry.js", () => ({ - registerSkillTools: (tools: Tool[]) => { - const skillIds = new Set(); - for (const tool of tools) { - const skillId = tool.ownerSkillId!; - skillIds.add(skillId); - const existing = mockRegisteredTools.get(skillId) ?? []; - existing.push(tool); - mockRegisteredTools.set(skillId, existing); - } - for (const id of skillIds) { - mockSkillRefCount.set(id, (mockSkillRefCount.get(id) ?? 0) + 1); - } + // Matches the new signature: `registerSkillTools(skillId, tools)`. The + // skillId comes from the caller (conversation-skill-tools) and is the + // sole source of truth for ownership. + registerSkillTools: (skillId: string, tools: Tool[]) => { + const existing = mockRegisteredTools.get(skillId) ?? []; + existing.push(...tools); + mockRegisteredTools.set(skillId, existing); + mockSkillRefCount.set(skillId, (mockSkillRefCount.get(skillId) ?? 0) + 1); return tools; }, unregisterSkillTools: (skillId: string) => { @@ -151,6 +147,23 @@ mock.module("../tools/registry.js", () => ({ } return found; }, + // Mirrors the registry's `ownersByName` accessor: derives the owning + // skillId from `mockRegisteredTools` keying so the production + // `getToolOwner(name)` call in `conversation-skill-tools.ts` resolves to + // the same shape the real registry would return. + getToolOwner: ( + name: string, + ): { kind: "skill" | "plugin" | "mcp"; id: string } | undefined => { + let ownerSkillId: string | undefined; + for (const [skillId, tools] of mockRegisteredTools.entries()) { + for (const tool of tools) { + if (tool.name === name) ownerSkillId = skillId; + } + } + return ownerSkillId === undefined + ? undefined + : { kind: "skill", id: ownerSkillId }; + }, getSkillToolNames: () => { const names: string[] = []; for (const tools of mockRegisteredTools.values()) { diff --git a/assistant/src/__tests__/conversation-skill-tools.test.ts b/assistant/src/__tests__/conversation-skill-tools.test.ts index 8a26c4e86db..14e6b05a1b5 100644 --- a/assistant/src/__tests__/conversation-skill-tools.test.ts +++ b/assistant/src/__tests__/conversation-skill-tools.test.ts @@ -92,9 +92,10 @@ mock.module("../skills/tool-manifest.js", () => ({ })); mock.module("../tools/skills/skill-tool-factory.js", () => ({ + // Mirrors the real factory: no skillId in/out — ownership is recorded by + // the registry at `registerSkillTools(skillId, tools)` time. createSkillToolsFromManifest: ( entries: SkillToolManifest["tools"], - skillId: string, _skillDir: string, _versionHash: string, _bundled?: boolean, @@ -106,7 +107,6 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ defaultRiskLevel: RiskLevel.Medium, executionTarget: "sandbox" as const, origin: "skill" as const, - ownerSkillId: skillId, input_schema: entry.input_schema as object, execute: async () => ({ content: "", isError: false }), })); @@ -114,18 +114,14 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ })); mock.module("../tools/registry.js", () => ({ - registerSkillTools: (tools: Tool[]) => { - const skillIds = new Set(); - for (const tool of tools) { - const skillId = tool.ownerSkillId!; - skillIds.add(skillId); - const existing = mockRegisteredTools.get(skillId) ?? []; - existing.push(tool); - mockRegisteredTools.set(skillId, existing); - } - for (const id of skillIds) { - mockSkillRefCount.set(id, (mockSkillRefCount.get(id) ?? 0) + 1); - } + // Matches the new signature: `registerSkillTools(skillId, tools)`. The + // skillId comes from the caller (conversation-skill-tools) and is the + // sole source of truth for ownership. + registerSkillTools: (skillId: string, tools: Tool[]) => { + const existing = mockRegisteredTools.get(skillId) ?? []; + existing.push(...tools); + mockRegisteredTools.set(skillId, existing); + mockSkillRefCount.set(skillId, (mockSkillRefCount.get(skillId) ?? 0) + 1); return tools; }, unregisterSkillTools: (skillId: string) => { @@ -149,6 +145,22 @@ mock.module("../tools/registry.js", () => ({ } return found; }, + // Mirrors the registry's `ownersByName` accessor: the mock derives the + // owning skillId from `mockRegisteredTools` (which is keyed by skillId) + // and reports it back as the `OwnerInfo` shape production callers expect. + getToolOwner: ( + name: string, + ): { kind: "skill" | "plugin" | "mcp"; id: string } | undefined => { + let ownerSkillId: string | undefined; + for (const [skillId, tools] of mockRegisteredTools.entries()) { + for (const tool of tools) { + if (tool.name === name) ownerSkillId = skillId; + } + } + return ownerSkillId === undefined + ? undefined + : { kind: "skill", id: ownerSkillId }; + }, getSkillToolNames: () => { const names: string[] = []; for (const tools of mockRegisteredTools.values()) { @@ -1728,10 +1740,13 @@ describe("bundled skill: app-builder", () => { expect(tools).toBeDefined(); expect(tools!.length).toBe(4); - // All tools should have skill origin metadata + // All tools should have skill origin metadata. Ownership is asserted by + // the fact that `mockRegisteredTools.get("app-builder")` returned the + // tools — i.e. the mock `registerSkillTools(skillId, tools)` was called + // with skillId "app-builder", which is what the registry would record + // in production. for (const tool of tools!) { expect(tool.origin).toBe("skill"); - expect(tool.ownerSkillId).toBe("app-builder"); } }); }); @@ -2227,7 +2242,7 @@ describe("hash change re-prompt regressions (PR 35)", () => { expect(sessionState.get("oncall")).toBe("v2:oncall-edited"); }); - test("registered tools carry updated ownerSkillId after hash change re-registration", () => { + test("registered tools carry updated owner after hash change re-registration", () => { mockCatalog = [makeSkill("deploy")]; mockManifests = { deploy: makeManifest(["deploy_run"]) }; mockVersionHashes = { deploy: "v1:pre-edit" }; @@ -2236,12 +2251,14 @@ describe("hash change re-prompt regressions (PR 35)", () => { ...skillLoadMessages(''), ]; - // Turn 1 + // Turn 1: ownership is asserted by the mock registry's keying — the + // tools landing under skillId "deploy" in `mockRegisteredTools` means + // `registerSkillTools("deploy", tools)` was called, which is the only + // path that records ownership in the real registry. projectSkillTools(history, { previouslyActiveSkillIds: sessionState }); const toolsV1 = mockRegisteredTools.get("deploy"); expect(toolsV1).toBeDefined(); expect(toolsV1!.length).toBe(1); - expect(toolsV1![0].ownerSkillId).toBe("deploy"); // Edit mockVersionHashes = { deploy: "v2:post-edit" }; @@ -2251,7 +2268,6 @@ describe("hash change re-prompt regressions (PR 35)", () => { const toolsV2 = mockRegisteredTools.get("deploy"); expect(toolsV2).toBeDefined(); expect(toolsV2!.length).toBeGreaterThanOrEqual(1); - expect(toolsV2![0].ownerSkillId).toBe("deploy"); }); }); diff --git a/assistant/src/__tests__/plugin-tool-contribution.test.ts b/assistant/src/__tests__/plugin-tool-contribution.test.ts index 672b7e451f7..9eb9b2f8804 100644 --- a/assistant/src/__tests__/plugin-tool-contribution.test.ts +++ b/assistant/src/__tests__/plugin-tool-contribution.test.ts @@ -6,7 +6,7 @@ * * - Registering a plugin with `tools: Tool[]`, running `bootstrapPlugins`, * and observing the contributed tool via `getAllTools()` / `getTool()`. - * - Tool ownership metadata (`origin: "plugin"`, `ownerPluginId: `) + * - Tool ownership metadata (`origin: "plugin"`, `owner: { kind: "plugin", id: }`) * stamped authoritatively by `registerPluginTools` regardless of what the * plugin author set on the incoming object. * - Shutdown hook unregistering the contributed tools so the registry is @@ -55,6 +55,7 @@ import { getAllTools, getPluginRefCount, getTool, + getToolOwner, registerPluginTools, unregisterPluginTools, } from "../tools/registry.js"; @@ -164,13 +165,18 @@ describe("plugin tool contributions", () => { const retrieved = getTool("plugin-contrib-tool"); expect(retrieved).toBeDefined(); - // Ownership metadata must be stamped authoritatively by the bootstrap — - // the registry uses it to drive ref-counting and conflict detection when - // the plugin shuts down or is hot-reloaded. Plugin tools live in their - // own `origin: "plugin"` namespace, disjoint from real skills, so a - // plugin name that happens to match a skill id cannot collide. + // Ownership is recorded authoritatively by the bootstrap into the + // registry's `ownersByName` map (keyed by tool name, accessed via + // `getToolOwner(name)`) — the registry uses it to drive ref-counting + // and conflict detection when the plugin shuts down or is hot-reloaded. + // Plugin tools live in their own `origin: "plugin"` namespace, disjoint + // from real skills, so a plugin name that happens to match a skill id + // cannot collide. Ownership is not stamped on the `Tool` object itself. expect(retrieved?.origin).toBe("plugin"); - expect(retrieved?.ownerPluginId).toBe("alpha-contributor"); + expect(getToolOwner("plugin-contrib-tool")).toEqual({ + kind: "plugin", + id: "alpha-contributor", + }); // The tool surfaces in the global `getAllTools()` snapshot, which is // what downstream consumers (tool-manifest, session projection) read. @@ -248,22 +254,26 @@ describe("registerPluginTools / unregisterPluginTools helpers", () => { __resetRegistryForTesting(); }); - test("registerPluginTools stamps category, origin, and ownerPluginId from the plugin name", () => { - // Even if the plugin author hands in a tool with no category or ownership - // metadata, the helper fills it in so the tool can be registered and - // unregistered consistently. + test("registerPluginTools stamps category and origin from the plugin, records ownership in the registry", () => { + // Even if the plugin author hands in a tool with no category or + // ownership metadata, the helper fills in category/origin on the tool + // and records ownership in the registry's `ownersByName` map — the + // tool itself never carries an `owner` field, so plugin authors can't + // spoof ownership by forging one. const accepted = registerPluginTools("my-plugin", [ makeFakeTool("pt_stamped"), ]); expect(accepted).toHaveLength(1); expect(accepted[0]?.category).toBe("plugin"); expect(accepted[0]?.origin).toBe("plugin"); - expect(accepted[0]?.ownerPluginId).toBe("my-plugin"); + expect(getToolOwner("pt_stamped")).toEqual({ + kind: "plugin", + id: "my-plugin", + }); const retrieved = getTool("pt_stamped"); expect(retrieved?.category).toBe("plugin"); expect(retrieved?.origin).toBe("plugin"); - expect(retrieved?.ownerPluginId).toBe("my-plugin"); }); test("registerPluginTools exposes provider-safe aliases for unsafe plugin tool names", async () => { @@ -316,25 +326,31 @@ describe("registerPluginTools / unregisterPluginTools helpers", () => { test("registerPluginTools overwrites any pre-existing ownership metadata", () => { // A plugin author could (maliciously or mistakenly) hand in a tool // pre-tagged with another skill's or plugin's ID. The helper must - // overwrite it so the bootstrap is always the source of truth for - // ownership — and it must clear cross-origin fields (ownerSkillId / - // ownerMcpServerId) so the stamped tool cannot leak across namespaces - // or spoof skill-origin auto-allow. + // overwrite both `origin` and `owner` so the bootstrap is always the + // source of truth for ownership and the stamped tool cannot leak across + // namespaces or spoof skill-origin auto-allow. // - // The narrow `ToolDefinition` shape doesn't expose these ownership - // fields, so the cast through `unknown` simulates a hostile or - // transpiled artifact that arrives with spoofed fields baked in — - // the bootstrap-side defense is the second layer that must hold. + // The narrow `ToolDefinition` shape doesn't expose ownership fields, so + // the cast through `unknown` simulates a hostile or transpiled artifact + // that arrives with spoofed fields baked in — the bootstrap-side + // defense is the second layer that must hold. const spoofed = { ...makeFakeTool("pt_spoof"), origin: "skill", - ownerSkillId: "some-other-skill", + // Forging an `owner` field on the tool literal has no effect — the + // `Tool` type doesn't carry ownership at all, and the registry only + // populates `ownersByName` from the first argument to the + // `register*Tools` function. Cast through `unknown` to simulate a + // hostile or transpiled artifact arriving with extra fields. + owner: { kind: "skill", id: "some-other-skill" }, } as unknown as LoadedTool; registerPluginTools("my-plugin", [spoofed]); const retrieved = getTool("pt_spoof"); expect(retrieved?.origin).toBe("plugin"); - expect(retrieved?.ownerPluginId).toBe("my-plugin"); - expect(retrieved?.ownerSkillId).toBeUndefined(); + expect(getToolOwner("pt_spoof")).toEqual({ + kind: "plugin", + id: "my-plugin", + }); }); test("unregisterPluginTools removes the plugin's tools", () => { diff --git a/assistant/src/__tests__/registry.test.ts b/assistant/src/__tests__/registry.test.ts index 6129bd2f609..6d398092cf7 100644 --- a/assistant/src/__tests__/registry.test.ts +++ b/assistant/src/__tests__/registry.test.ts @@ -9,6 +9,7 @@ import { getSkillRefCount, getSkillToolNames, getTool, + getToolOwner, initializeTools, registerSkillTools, registerTool, @@ -40,11 +41,10 @@ function makeFakeTool(name: string): Tool { }; } -function makeSkillTool(name: string, ownerSkillId: string): Tool { +function makeSkillTool(name: string): Tool { return { ...makeFakeTool(name), origin: "skill" as const, - ownerSkillId, }; } @@ -222,11 +222,10 @@ describe("tool origin metadata", () => { __resetRegistryForTesting(); }); - test("registers a skill-origin tool and preserves metadata via getTool()", () => { + test("registers a skill-origin tool and preserves origin via getTool()", () => { const skillTool: Tool = { ...makeFakeTool("test-skill-origin-tool"), origin: "skill", - ownerSkillId: "test-skill", }; registerTool(skillTool); @@ -234,16 +233,20 @@ describe("tool origin metadata", () => { const retrieved = getTool("test-skill-origin-tool"); expect(retrieved).toBeDefined(); expect(retrieved?.origin).toBe("skill"); - expect(retrieved?.ownerSkillId).toBe("test-skill"); + // `registerTool` is the bare-install path used by tests + core + // bootstraps; it does not record ownership. Tools that need an owner + // must go through `registerSkillTools(skillId, ...)` or its sibling + // entry points so the registry populates `ownersByName`. + expect(getToolOwner("test-skill-origin-tool")).toBeUndefined(); }); - test("core tools default to no origin metadata (undefined)", async () => { + test("core tools have no origin metadata and no owner", async () => { await initializeTools(); const coreTool = getTool("host_file_read"); expect(coreTool).toBeDefined(); expect(coreTool?.origin).toBeUndefined(); - expect(coreTool?.ownerSkillId).toBeUndefined(); + expect(getToolOwner("host_file_read")).toBeUndefined(); }); }); @@ -252,66 +255,64 @@ describe("dynamic skill tool registry", () => { __resetRegistryForTesting(); }); - test("registers skill tools and retrieves them", () => { - const tools = [ - makeSkillTool("sk_tool_a", "my-skill"), - makeSkillTool("sk_tool_b", "my-skill"), - ]; - registerSkillTools(tools); + test("registers skill tools and retrieves them with owner via getToolOwner", () => { + registerSkillTools("my-skill", [ + makeSkillTool("sk_tool_a"), + makeSkillTool("sk_tool_b"), + ]); expect(getTool("sk_tool_a")).toBeDefined(); expect(getTool("sk_tool_a")?.origin).toBe("skill"); - expect(getTool("sk_tool_a")?.ownerSkillId).toBe("my-skill"); + expect(getToolOwner("sk_tool_a")).toEqual({ kind: "skill", id: "my-skill" }); expect(getTool("sk_tool_b")).toBeDefined(); expect(getTool("sk_tool_b")?.origin).toBe("skill"); + expect(getToolOwner("sk_tool_b")).toEqual({ kind: "skill", id: "my-skill" }); }); test("skips skill tool that collides with a core tool without throwing", async () => { await initializeTools(); // host_file_read is a core tool registered during init - const colliding = makeSkillTool("host_file_read", "rogue-skill"); - const accepted = registerSkillTools([colliding]); + const accepted = registerSkillTools("rogue-skill", [ + makeSkillTool("host_file_read"), + ]); // The colliding tool should be silently skipped expect(accepted).toHaveLength(0); // The core tool should still be in place (not overwritten) const retrieved = getTool("host_file_read"); expect(retrieved?.origin).toBeUndefined(); // core tools have no origin + expect(getToolOwner("host_file_read")).toBeUndefined(); }); test("allows replacement within the same owning skill", () => { - const original = makeSkillTool("sk_replaceable", "owner-skill"); - registerSkillTools([original]); + registerSkillTools("owner-skill", [makeSkillTool("sk_replaceable")]); const replacement: Tool = { - ...makeSkillTool("sk_replaceable", "owner-skill"), + ...makeSkillTool("sk_replaceable"), description: "Updated description", }; // Should not throw - registerSkillTools([replacement]); + registerSkillTools("owner-skill", [replacement]); const retrieved = getTool("sk_replaceable"); expect(retrieved?.description).toBe("Updated description"); }); test("rejects replacement from a different owning skill", () => { - const original = makeSkillTool("sk_owned", "skill-alpha"); - registerSkillTools([original]); + registerSkillTools("skill-alpha", [makeSkillTool("sk_owned")]); - const intruder = makeSkillTool("sk_owned", "skill-beta"); - expect(() => registerSkillTools([intruder])).toThrow( - 'already registered by skill "skill-alpha"', - ); + expect(() => + registerSkillTools("skill-beta", [makeSkillTool("sk_owned")]), + ).toThrow('already registered by skill "skill-alpha"'); }); test("unregisterSkillTools removes all tools for a skill", () => { - const tools = [ - makeSkillTool("sk_rm_1", "removable-skill"), - makeSkillTool("sk_rm_2", "removable-skill"), - ]; - registerSkillTools(tools); + registerSkillTools("removable-skill", [ + makeSkillTool("sk_rm_1"), + makeSkillTool("sk_rm_2"), + ]); expect(getTool("sk_rm_1")).toBeDefined(); expect(getTool("sk_rm_2")).toBeDefined(); @@ -319,24 +320,29 @@ describe("dynamic skill tool registry", () => { expect(getTool("sk_rm_1")).toBeUndefined(); expect(getTool("sk_rm_2")).toBeUndefined(); + // Ownership map is cleared in lockstep with the tools map. + expect(getToolOwner("sk_rm_1")).toBeUndefined(); + expect(getToolOwner("sk_rm_2")).toBeUndefined(); }); test("unregisterSkillTools does not affect tools from other skills", () => { - registerSkillTools([makeSkillTool("sk_keep", "keep-skill")]); - registerSkillTools([makeSkillTool("sk_remove", "nuke-skill")]); + registerSkillTools("keep-skill", [makeSkillTool("sk_keep")]); + registerSkillTools("nuke-skill", [makeSkillTool("sk_remove")]); unregisterSkillTools("nuke-skill"); expect(getTool("sk_keep")).toBeDefined(); expect(getTool("sk_remove")).toBeUndefined(); + expect(getToolOwner("sk_keep")).toEqual({ kind: "skill", id: "keep-skill" }); + expect(getToolOwner("sk_remove")).toBeUndefined(); }); test("getSkillToolNames returns only skill tool names", async () => { await initializeTools(); - registerSkillTools([ - makeSkillTool("sk_names_a", "names-skill"), - makeSkillTool("sk_names_b", "names-skill"), + registerSkillTools("names-skill", [ + makeSkillTool("sk_names_a"), + makeSkillTool("sk_names_b"), ]); const skillNames = getSkillToolNames(); @@ -350,19 +356,22 @@ describe("dynamic skill tool registry", () => { test("registerSkillTools skips core-colliding tools but registers the rest", async () => { await initializeTools(); - const tools = [ - makeSkillTool("sk_atomic_ok", "atomic-skill"), - makeSkillTool("host_file_read", "atomic-skill"), // collides with core - ]; - - const accepted = registerSkillTools(tools); + const accepted = registerSkillTools("atomic-skill", [ + makeSkillTool("sk_atomic_ok"), + makeSkillTool("host_file_read"), // collides with core + ]); // Only the non-colliding tool should be accepted expect(accepted).toHaveLength(1); expect(accepted[0].name).toBe("sk_atomic_ok"); - // The non-colliding tool should be registered + // The non-colliding tool should be registered with the correct owner expect(getTool("sk_atomic_ok")).toBeDefined(); + expect(getToolOwner("sk_atomic_ok")).toEqual({ + kind: "skill", + id: "atomic-skill", + }); // The core tool should be untouched expect(getTool("host_file_read")?.origin).toBeUndefined(); + expect(getToolOwner("host_file_read")).toBeUndefined(); }); }); @@ -372,17 +381,17 @@ describe("skill tool reference counting", () => { }); test("ref count increments on each registerSkillTools call", () => { - registerSkillTools([makeSkillTool("rc_a", "rc-skill")]); + registerSkillTools("rc-skill", [makeSkillTool("rc_a")]); expect(getSkillRefCount("rc-skill")).toBe(1); - // Second session registers the same skill (same ownerSkillId allows replacement) - registerSkillTools([makeSkillTool("rc_a", "rc-skill")]); + // Second session registers the same skill (same owner id allows replacement) + registerSkillTools("rc-skill", [makeSkillTool("rc_a")]); expect(getSkillRefCount("rc-skill")).toBe(2); }); test("unregister decrements ref count but keeps tools when count > 0", () => { - registerSkillTools([makeSkillTool("rc_keep", "rc-multi")]); - registerSkillTools([makeSkillTool("rc_keep", "rc-multi")]); + registerSkillTools("rc-multi", [makeSkillTool("rc_keep")]); + registerSkillTools("rc-multi", [makeSkillTool("rc_keep")]); expect(getSkillRefCount("rc-multi")).toBe(2); unregisterSkillTools("rc-multi"); @@ -392,8 +401,8 @@ describe("skill tool reference counting", () => { }); test("tools are removed only when last reference is unregistered", () => { - registerSkillTools([makeSkillTool("rc_last", "rc-final")]); - registerSkillTools([makeSkillTool("rc_last", "rc-final")]); + registerSkillTools("rc-final", [makeSkillTool("rc_last")]); + registerSkillTools("rc-final", [makeSkillTool("rc_last")]); unregisterSkillTools("rc-final"); expect(getTool("rc_last")).toBeDefined(); diff --git a/assistant/src/__tests__/skill-projection-feature-flag.test.ts b/assistant/src/__tests__/skill-projection-feature-flag.test.ts index 6357830465a..eb522f7fe9b 100644 --- a/assistant/src/__tests__/skill-projection-feature-flag.test.ts +++ b/assistant/src/__tests__/skill-projection-feature-flag.test.ts @@ -115,9 +115,10 @@ mock.module("../skills/tool-manifest.js", () => ({ })); mock.module("../tools/skills/skill-tool-factory.js", () => ({ + // Mirrors the real factory: no skillId in/out — ownership is recorded by + // the registry at `registerSkillTools(skillId, tools)` time. createSkillToolsFromManifest: ( entries: SkillToolManifest["tools"], - skillId: string, _skillDir: string, _versionHash: string, _bundled?: boolean, @@ -129,7 +130,6 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ defaultRiskLevel: RiskLevel.Medium, executionTarget: "sandbox" as const, origin: "skill" as const, - ownerSkillId: skillId, input_schema: entry.input_schema as object, execute: async () => ({ content: "", isError: false }), })); @@ -137,18 +137,14 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ })); mock.module("../tools/registry.js", () => ({ - registerSkillTools: (tools: Tool[]) => { - const skillIds = new Set(); - for (const tool of tools) { - const skillId = tool.ownerSkillId!; - skillIds.add(skillId); - const existing = mockRegisteredTools.get(skillId) ?? []; - existing.push(tool); - mockRegisteredTools.set(skillId, existing); - } - for (const id of skillIds) { - mockSkillRefCount.set(id, (mockSkillRefCount.get(id) ?? 0) + 1); - } + // Matches the new signature: `registerSkillTools(skillId, tools)`. The + // skillId comes from the caller (conversation-skill-tools) and is the + // sole source of truth for ownership. + registerSkillTools: (skillId: string, tools: Tool[]) => { + const existing = mockRegisteredTools.get(skillId) ?? []; + existing.push(...tools); + mockRegisteredTools.set(skillId, existing); + mockSkillRefCount.set(skillId, (mockSkillRefCount.get(skillId) ?? 0) + 1); return tools; }, unregisterSkillTools: (skillId: string) => { @@ -170,6 +166,23 @@ mock.module("../tools/registry.js", () => ({ } return found; }, + // Mirrors the registry's `ownersByName` accessor: derives the owning + // skillId from `mockRegisteredTools` keying so the production + // `getToolOwner(name)` call in `conversation-skill-tools.ts` resolves to + // the same shape the real registry would return. + getToolOwner: ( + name: string, + ): { kind: "skill" | "plugin" | "mcp"; id: string } | undefined => { + let ownerSkillId: string | undefined; + for (const [skillId, tools] of mockRegisteredTools.entries()) { + for (const tool of tools) { + if (tool.name === name) ownerSkillId = skillId; + } + } + return ownerSkillId === undefined + ? undefined + : { kind: "skill", id: ownerSkillId }; + }, getSkillToolNames: () => { const names: string[] = []; for (const tools of mockRegisteredTools.values()) { diff --git a/assistant/src/__tests__/skill-projection.benchmark.test.ts b/assistant/src/__tests__/skill-projection.benchmark.test.ts index 45170dcc089..9eff4860967 100644 --- a/assistant/src/__tests__/skill-projection.benchmark.test.ts +++ b/assistant/src/__tests__/skill-projection.benchmark.test.ts @@ -111,7 +111,7 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ category: "skill", defaultRiskLevel: "low", origin: "skill" as const, - ownerSkillId: skillId, + owner: { kind: "skill" as const, id: skillId }, input_schema: e.input_schema, execute: async () => ({ content: "", isError: false }), })), @@ -164,7 +164,8 @@ mock.module("../tools/registry.js", () => ({ }, unregisterSkillTools: (skillId: string) => { for (const [name, t] of benchmarkRegistry) { - if ((t as { ownerSkillId?: string }).ownerSkillId === skillId) + const owner = (t as { owner?: { kind: string; id: string } }).owner; + if (owner?.kind === "skill" && owner.id === skillId) benchmarkRegistry.delete(name); } }, diff --git a/assistant/src/__tests__/skill-tool-factory.test.ts b/assistant/src/__tests__/skill-tool-factory.test.ts index 7b374d71cab..9ad8b2af7f6 100644 --- a/assistant/src/__tests__/skill-tool-factory.test.ts +++ b/assistant/src/__tests__/skill-tool-factory.test.ts @@ -75,7 +75,6 @@ describe("createSkillTool", () => { test("produces a tool with correct name, description, and category", () => { const tool = createSkillTool( makeEntry(), - "my-skill", "/skills/my-skill", "v1:test", ); @@ -85,16 +84,14 @@ describe("createSkillTool", () => { expect(tool.category).toBe("testing"); }); - test("sets origin to skill and ownerSkillId", () => { + test("sets origin to skill (ownership recorded by registry, not factory)", () => { const tool = createSkillTool( makeEntry(), - "weather-skill", "/skills/weather", "v1:test", ); expect(tool.origin).toBe("skill"); - expect(tool.ownerSkillId).toBe("weather-skill"); }); test.each([ @@ -104,7 +101,6 @@ describe("createSkillTool", () => { ] as const)('maps risk "%s" to RiskLevel.%s', (risk, expected) => { const tool = createSkillTool( makeEntry({ risk }), - "sk", "/skills/sk", "v1:test", ); @@ -128,7 +124,6 @@ describe("createSkillTool", () => { description: "Scrape a URL", input_schema: schema, }), - "scraper", "/skills/scraper", "v1:test", ); @@ -148,7 +143,6 @@ describe("createSkillTool", () => { const hash = computeSkillVersionHash(tempDir); const tool = createSkillTool( makeEntry({ executor: "echo.ts" }), - "my-skill", tempDir, hash, ); @@ -167,7 +161,6 @@ describe("createSkillTool", () => { const hash = computeSkillVersionHash(tempDir); const tool = createSkillTool( makeEntry({ executor: "nonexistent.ts" }), - "my-skill", tempDir, hash, ); @@ -193,7 +186,6 @@ describe("createSkillToolsFromManifest", () => { const tools = createSkillToolsFromManifest( entries, - "multi-skill", "/skills/multi", "v1:test", ); @@ -207,7 +199,7 @@ describe("createSkillToolsFromManifest", () => { ]); }); - test("all created tools share the same skillId and origin", () => { + test("all created tools share the same origin (ownership is recorded by the registry, not the factory)", () => { const entries: SkillToolEntry[] = [ makeEntry({ name: "alpha" }), makeEntry({ name: "beta" }), @@ -215,21 +207,18 @@ describe("createSkillToolsFromManifest", () => { const tools = createSkillToolsFromManifest( entries, - "shared-skill", "/skills/shared", "v1:test", ); for (const tool of tools) { expect(tool.origin).toBe("skill"); - expect(tool.ownerSkillId).toBe("shared-skill"); } }); test("returns an empty array when given no entries", () => { const tools = createSkillToolsFromManifest( [], - "empty-skill", "/skills/empty", "v1:test", ); @@ -248,7 +237,6 @@ describe("createSkillTool — unknown parameter validation", () => { const hash = computeSkillVersionHash(tempDir); const tool = createSkillTool( makeEntry({ executor: "echo.ts" }), - "my-skill", tempDir, hash, ); @@ -268,7 +256,6 @@ describe("createSkillTool — unknown parameter validation", () => { const hash = computeSkillVersionHash(tempDir); const tool = createSkillTool( makeEntry({ executor: "echo.ts" }), - "my-skill", tempDir, hash, ); @@ -288,7 +275,6 @@ describe("createSkillTool — unknown parameter validation", () => { const hash = computeSkillVersionHash(tempDir); const tool = createSkillTool( makeEntry({ executor: "echo.ts" }), - "my-skill", tempDir, hash, ); @@ -308,7 +294,6 @@ describe("createSkillTool — unknown parameter validation", () => { properties: { query: { type: "string" } }, }, }), - "my-skill", tempDir, hash, ); @@ -325,7 +310,6 @@ describe("createSkillTool — unknown parameter validation", () => { executor: "echo.ts", input_schema: { type: "object" }, }), - "my-skill", tempDir, hash, ); @@ -346,7 +330,6 @@ describe("createSkillTool — version hash plumbing to runner", () => { const hash = computeSkillVersionHash(tempDir); const tool = createSkillTool( makeEntry({ executor: "echo.ts" }), - "my-skill", tempDir, hash, ); diff --git a/assistant/src/__tests__/tool-executor-lifecycle-events.test.ts b/assistant/src/__tests__/tool-executor-lifecycle-events.test.ts index fca0c9213c3..7d495f7812d 100644 --- a/assistant/src/__tests__/tool-executor-lifecycle-events.test.ts +++ b/assistant/src/__tests__/tool-executor-lifecycle-events.test.ts @@ -93,7 +93,7 @@ mock.module("../tools/registry.js", () => ({ category: "skill", defaultRiskLevel: "low", origin: "skill" as const, - ownerSkillId: "test-skill", + owner: { kind: "skill", id: "test-skill" }, executionTarget: "host" as const, input_schema: {}, execute: async () => { @@ -109,7 +109,7 @@ mock.module("../tools/registry.js", () => ({ category: "skill", defaultRiskLevel: "low", origin: "skill" as const, - ownerSkillId: "test-skill", + owner: { kind: "skill", id: "test-skill" }, executionTarget: "sandbox" as const, input_schema: {}, execute: async () => { @@ -127,7 +127,7 @@ mock.module("../tools/registry.js", () => ({ category: "skill", defaultRiskLevel: "low", origin: "skill" as const, - ownerSkillId: "test-skill", + owner: { kind: "skill", id: "test-skill" }, executionTarget: "sandbox" as const, input_schema: {}, execute: async () => { diff --git a/assistant/src/__tests__/tool-executor.test.ts b/assistant/src/__tests__/tool-executor.test.ts index 9c0e256553b..1276c0717da 100644 --- a/assistant/src/__tests__/tool-executor.test.ts +++ b/assistant/src/__tests__/tool-executor.test.ts @@ -148,6 +148,16 @@ mock.module("../tools/registry.js", () => ({ }; }, getAllTools: () => (getAllToolsOverride ? getAllToolsOverride() : []), + // Ownership lives on the registry post-refactor; production reads it via + // getToolOwner(name) rather than a field on the Tool object. Mirror that by + // surfacing the optional `owner`-shaped field from the override-produced + // tool so existing tests can encode owner inline. + getToolOwner: (name: string) => { + const t = getToolOverride?.(name) as + | { owner?: { kind: "skill" | "mcp" | "plugin"; id: string } } + | undefined; + return t?.owner; + }, })); mock.module("../tools/shared/filesystem/path-policy.js", () => ({ @@ -335,7 +345,7 @@ describe("ToolExecutor allowedToolNames gating", () => { defaultRiskLevel: RiskLevel.Low, executionTarget: "sandbox" as const, origin: "skill" as const, - ownerSkillId: "my-skill", + owner: { kind: "skill", id: "my-skill" }, input_schema: { type: "object" as const, properties: {} }, execute: async () => fakeToolResult, }; @@ -372,7 +382,7 @@ describe("ToolExecutor policy context plumbing", () => { category: "skill", defaultRiskLevel: RiskLevel.Low, origin: "skill" as const, - ownerSkillId: "my-skill-123", + owner: { kind: "skill", id: "my-skill-123" }, executionTarget: "sandbox" as const, input_schema: { type: "object" as const, properties: {} }, execute: async () => fakeToolResult, @@ -453,7 +463,7 @@ describe("ToolExecutor policy context plumbing", () => { category: "skill", defaultRiskLevel: RiskLevel.Low, origin: "skill" as const, - ownerSkillId: "host-skill", + owner: { kind: "skill", id: "host-skill" }, executionTarget: "host" as const, input_schema: { type: "object" as const, properties: {} }, execute: async () => fakeToolResult, diff --git a/assistant/src/daemon/__tests__/conversation-tool-setup-exclude.test.ts b/assistant/src/daemon/__tests__/conversation-tool-setup-exclude.test.ts index 70aff778929..d5e4a2738e5 100644 --- a/assistant/src/daemon/__tests__/conversation-tool-setup-exclude.test.ts +++ b/assistant/src/daemon/__tests__/conversation-tool-setup-exclude.test.ts @@ -89,7 +89,7 @@ describe("createResolveToolsCallback — config.tools.exclude", () => { }); test("excluded MCP tool is omitted from the resolved tool list", () => { - registerMcpTools([ + registerMcpTools("test-server", [ mcpTool("mcp__server__navigate"), mcpTool("mcp__server__click"), ]); diff --git a/assistant/src/daemon/__tests__/meet-manifest-loader.test.ts b/assistant/src/daemon/__tests__/meet-manifest-loader.test.ts index 8ab0c6b672c..e875bbb113e 100644 --- a/assistant/src/daemon/__tests__/meet-manifest-loader.test.ts +++ b/assistant/src/daemon/__tests__/meet-manifest-loader.test.ts @@ -163,14 +163,23 @@ describe("loadMeetManifestProxies", () => { const capturedRoutes: SkillRoute[] = []; const capturedHooks: string[] = []; + // Capture both the owner (first arg) and the provider (second arg) so + // we can assert the meet-join skill id flows through to the registry + // call. Ownership lives on the registry, not on the `Tool` object, + // so the only place to check it is on this argument. + const capturedOwners: Array<{ kind: string; id: string }> = []; await loadMeetManifestProxies(supervisor, { manifestPath, - registerTools: (p) => capturedProviders.push(p), + registerTools: (owner, p) => { + capturedOwners.push(owner); + capturedProviders.push(p); + }, registerRoute: (r) => capturedRoutes.push(r), registerShutdown: (name) => capturedHooks.push(name), }); expect(capturedProviders).toHaveLength(1); + expect(capturedOwners).toEqual([{ kind: "skill", id: "meet-join" }]); const tools = capturedProviders[0]!(); expect(tools).toHaveLength(1); const t = tools[0]!; @@ -180,7 +189,6 @@ describe("loadMeetManifestProxies", () => { expect(t.defaultRiskLevel).toBe(RiskLevel.Medium); expect(t.executionMode).toBe("proxy"); expect(t.origin).toBe("skill"); - expect(t.ownerSkillId).toBe("meet-join"); expect(t.input_schema).toEqual( FIXTURE_MANIFEST.tools[0]!.input_schema, ); @@ -194,7 +202,11 @@ describe("loadMeetManifestProxies", () => { await loadMeetManifestProxies(stub.supervisor, { manifestPath, - registerTools: (p) => captured.push(p), + // Mock signature mirrors `registerExternalTools(owner, provider)` — + // ownership is recorded by the registry-side argument, not by the + // `Tool` object. This test doesn't care which owner flows through; + // the round-trip dispatch behavior is what's under test. + registerTools: (_owner, p) => captured.push(p), registerRoute: () => undefined, registerShutdown: () => undefined, }); @@ -236,7 +248,11 @@ describe("loadMeetManifestProxies", () => { await loadMeetManifestProxies(stub.supervisor, { manifestPath, - registerTools: (p) => captured.push(p), + // Mock signature mirrors `registerExternalTools(owner, provider)` — + // ownership is recorded by the registry-side argument, not by the + // `Tool` object. This test doesn't care which owner flows through; + // the round-trip dispatch behavior is what's under test. + registerTools: (_owner, p) => captured.push(p), registerRoute: () => undefined, registerShutdown: () => undefined, }); @@ -413,7 +429,7 @@ describe("loadMeetManifestProxies", () => { await expect( loadMeetManifestProxies(stub.supervisor, { manifestPath, - registerTools: (p) => { + registerTools: (_owner, p) => { providerInvoked = true; p(); }, diff --git a/assistant/src/daemon/conversation-skill-tools.ts b/assistant/src/daemon/conversation-skill-tools.ts index e460a97f05b..fddb639767e 100644 --- a/assistant/src/daemon/conversation-skill-tools.ts +++ b/assistant/src/daemon/conversation-skill-tools.ts @@ -23,6 +23,7 @@ import { parseToolManifestFile } from "../skills/tool-manifest.js"; import { computeSkillVersionHash } from "../skills/version-hash.js"; import { getTool, + getToolOwner, registerSkillTools, unregisterSkillTools, } from "../tools/registry.js"; @@ -313,7 +314,6 @@ export function projectSkillTools( // Create runtime Tool objects const tools = createSkillToolsFromManifest( manifest.tools, - skillId, skill.directoryPath, currentHash, skill.bundled, @@ -324,7 +324,7 @@ export function projectSkillTools( const prevHash = prevActive.get(skillId); if (prevHash === undefined) { // Newly active skill — register for the first time - accepted = registerSkillTools(tools); + accepted = registerSkillTools(skillId, tools); } else if (prevHash !== currentHash) { // Hash changed — unregister stale tools, then re-register with new definitions log.info( @@ -334,7 +334,7 @@ export function projectSkillTools( unregisterSkillTools(skillId); alreadyUnregistered.add(skillId); try { - accepted = registerSkillTools(tools); + accepted = registerSkillTools(skillId, tools); } catch (err) { log.error( { err, skillId }, @@ -351,12 +351,9 @@ export function projectSkillTools( // because the permission checker derives bundled state from the // live catalog instead of a stamped tool field. accepted = tools.filter((t) => { - const reg = getTool(t.name); - return ( - reg !== undefined && - reg.origin === "skill" && - reg.ownerSkillId === skillId - ); + if (getTool(t.name) === undefined) return false; + const owner = getToolOwner(t.name); + return owner?.kind === "skill" && owner.id === skillId; }); } diff --git a/assistant/src/daemon/daemon-skill-host.ts b/assistant/src/daemon/daemon-skill-host.ts index 320251158c8..818da872f2f 100644 --- a/assistant/src/daemon/daemon-skill-host.ts +++ b/assistant/src/daemon/daemon-skill-host.ts @@ -219,8 +219,15 @@ function buildRegistriesFacet(skillId: string): RegistriesFacet { // overlay (`assistant/src/tools/types.ts`); the assistant-side // registry accepts the daemon flavor. Skills construct tools via // helpers that already produce the daemon shape, so a cast at this - // boundary is safe. - registerTools: (provider) => registerExternalTools(provider as never), + // boundary is safe. The contract's `registerTools(provider)` stays + // single-arg — skill code never needs to know its own id — and this + // adapter pairs the provider with the owner derived from the + // surrounding {@link buildRegistriesFacet} closure. + registerTools: (provider) => + registerExternalTools( + { kind: "skill", id: skillId }, + provider as never, + ), registerSkillRoute: (route: SkillRoute): SkillRouteHandle => registerSkillRoute(route) as unknown as SkillRouteHandle, // Namespace hook names by skillId so two skills using the same label diff --git a/assistant/src/daemon/mcp-reload-service.ts b/assistant/src/daemon/mcp-reload-service.ts index 213a0f8dc90..6b1c06ff99b 100644 --- a/assistant/src/daemon/mcp-reload-service.ts +++ b/assistant/src/daemon/mcp-reload-service.ts @@ -82,7 +82,7 @@ async function doReload(): Promise { serverConfig, manager, ); - const accepted = registerMcpTools(mcpTools); + const accepted = registerMcpTools(serverId, mcpTools); const acceptedNames = accepted.map((t) => t.name); toolCount += accepted.length; servers.push({ diff --git a/assistant/src/daemon/meet-manifest-loader.ts b/assistant/src/daemon/meet-manifest-loader.ts index d429094be4f..0eb6d8e8bf5 100644 --- a/assistant/src/daemon/meet-manifest-loader.ts +++ b/assistant/src/daemon/meet-manifest-loader.ts @@ -140,7 +140,6 @@ function buildProxyTool( executionMode: "proxy", executionTarget: "host" as ExecutionTarget, origin: "skill", - ownerSkillId: MEET_SKILL_ID, execute: async (input, context) => { // `dispatchTool` ensures the meet-host child is up + connected // before sending the frame, so callers don't need a separate @@ -315,7 +314,10 @@ export interface MeetManifestLoaderDeps { /** Override for the manifest path resolver. */ manifestPath?: string; /** Override for {@link registerExternalTools}. */ - registerTools?: (provider: () => Tool[]) => void; + registerTools?: ( + owner: { kind: "skill" | "plugin" | "mcp"; id: string }, + provider: () => Tool[], + ) => void; /** Override for {@link registerSkillRoute}. */ registerRoute?: (route: SkillRoute) => unknown; /** Override for {@link registerShutdownHook}. */ @@ -358,8 +360,10 @@ export async function loadMeetManifestProxies( // Tool provider resolves the full proxy list lazily so the tool manifest // reflects the manifest file at `initializeTools()` time — same timing - // contract as the in-process skill's provider closure. - registerTools(() => + // contract as the in-process skill's provider closure. Owner is recorded + // up-front so `getToolOwner(name)` returns the meet-join skill id without + // the tool object having to carry it. + registerTools({ kind: "skill", id: MEET_SKILL_ID }, () => manifest.tools.map((entry) => buildProxyTool(entry, supervisor), ), diff --git a/assistant/src/daemon/providers-setup.ts b/assistant/src/daemon/providers-setup.ts index 775fb5a656e..5cbaf744dd5 100644 --- a/assistant/src/daemon/providers-setup.ts +++ b/assistant/src/daemon/providers-setup.ts @@ -133,7 +133,7 @@ export async function initializeProvidersAndTools( serverConfig, manager, ); - registerMcpTools(mcpTools); + registerMcpTools(serverId, mcpTools); } } catch (err) { log.error( diff --git a/assistant/src/ipc/skill-routes/__tests__/registries.test.ts b/assistant/src/ipc/skill-routes/__tests__/registries.test.ts index a72573264d8..f056a108c9b 100644 --- a/assistant/src/ipc/skill-routes/__tests__/registries.test.ts +++ b/assistant/src/ipc/skill-routes/__tests__/registries.test.ts @@ -19,6 +19,7 @@ import { __clearExternalToolProvidersForTesting, __clearRegistryForTesting, getTool, + getToolOwner, } from "../../../tools/registry.js"; import { __getActiveSessionCountForTesting, @@ -55,7 +56,12 @@ afterEach(() => { describe("host.registries.register_tools", () => { test("installs proxy tools into the daemon's external tool registry", async () => { + // `skillId` lives at the top of the params object (one frame = one + // skill's batch). The per-tool manifest schema no longer carries an + // `owner` field — ownership flows in through this top-level key and + // is recorded in the registry's `ownersByName` map. const result = (await registerToolsRoute.handler({ + skillId: "demo-skill", tools: [ { name: "skill_demo_tool", @@ -64,7 +70,6 @@ describe("host.registries.register_tools", () => { defaultRiskLevel: "low", category: "skill", executionTarget: "sandbox", - ownerSkillId: "demo-skill", }, ], })) as { registered: string[] }; @@ -73,12 +78,16 @@ describe("host.registries.register_tools", () => { const installed = getTool("skill_demo_tool"); expect(installed).toBeDefined(); expect(installed!.origin).toBe("skill"); - expect(installed!.ownerSkillId).toBe("demo-skill"); + expect(getToolOwner("skill_demo_tool")).toEqual({ + kind: "skill", + id: "demo-skill", + }); expect(installed!.executionMode).toBe("proxy"); }); test("proxy execute throws when no supervisor is attached", async () => { await registerToolsRoute.handler({ + skillId: "stub-skill", tools: [ { name: "skill_stub_tool", @@ -86,7 +95,6 @@ describe("host.registries.register_tools", () => { input_schema: { type: "object" }, defaultRiskLevel: "medium", category: "skill", - ownerSkillId: "stub-skill", }, ], }); @@ -106,16 +114,37 @@ describe("host.registries.register_tools", () => { }); test("rejects empty tool list", async () => { - await expect(registerToolsRoute.handler({ tools: [] })).rejects.toThrow(); + await expect( + registerToolsRoute.handler({ skillId: "any-skill", tools: [] }), + ).rejects.toThrow(); }); test("rejects missing required fields", async () => { await expect( registerToolsRoute.handler({ + skillId: "any-skill", tools: [{ name: "missing_rest" }], }), ).rejects.toThrow(); }); + + test("rejects missing skillId", async () => { + // skillId is the only place ownership flows in over IPC — without it + // the registry can't claim the tools, so the handler must reject. + await expect( + registerToolsRoute.handler({ + tools: [ + { + name: "skill_orphan_tool", + description: "no owner", + input_schema: { type: "object" }, + defaultRiskLevel: "low", + category: "skill", + }, + ], + }), + ).rejects.toThrow(); + }); }); // --------------------------------------------------------------------------- @@ -302,6 +331,7 @@ describe("lazy-external short-circuit", () => { const result = (await registerToolsRoute.handler( { + skillId: "demo-skill", tools: [ { name: "skill_demo_tool", @@ -309,7 +339,6 @@ describe("lazy-external short-circuit", () => { input_schema: {}, defaultRiskLevel: "low", category: "skill", - ownerSkillId: "demo-skill", }, ], }, diff --git a/assistant/src/ipc/skill-routes/registries.ts b/assistant/src/ipc/skill-routes/registries.ts index 9e489ca8ed1..57440c2f892 100644 --- a/assistant/src/ipc/skill-routes/registries.ts +++ b/assistant/src/ipc/skill-routes/registries.ts @@ -49,15 +49,19 @@ const ToolManifestSchema = z.object({ category: z.string().min(1), executionTarget: z.enum(["sandbox", "host"]).optional(), executionMode: z.enum(["local", "proxy"]).optional(), - // Required so disconnect can decrement the tool-registry refcount: a - // tool registered without an owner has no ref-counted entry to drop and - // would leak into the global registry on socket close. - ownerSkillId: z.string().min(1), }); export type ToolManifest = z.infer; +// `skillId` lives at the params level rather than per-tool: a single +// `register_tools` IPC frame is always one skill's batch, ownership flows +// through `registerSkillTools(skillId, tools)` into the registry's +// `ownersByName` map, and the tools themselves stay free of owner +// metadata so callers cannot spoof ownership by forging a field on the +// manifest. Only skill-owned tools cross IPC — plugin and MCP tools live +// in-process on the assistant side. const RegisterToolsParams = z.object({ + skillId: z.string().min(1), tools: z.array(ToolManifestSchema).min(1), }); @@ -192,7 +196,6 @@ function buildProxyTool(manifest: ToolManifest): Tool { executionMode: manifest.executionMode ?? "proxy", }), origin: "skill", - ownerSkillId: manifest.ownerSkillId, execute: async () => { // Only reached when no supervisor is attached (tests/boot race); // the supervisor short-circuit above replaces this with the @@ -210,7 +213,7 @@ async function handleRegisterTools( params: Record | undefined, connection?: unknown, ): Promise<{ registered: string[] }> { - const { tools } = RegisterToolsParams.parse(params); + const { skillId, tools } = RegisterToolsParams.parse(params); const conn = connection as SkillIpcConnection | undefined; // Supervisor short-circuit: when a supervisor is registered, the @@ -222,7 +225,7 @@ async function handleRegisterTools( if (sessionSupervisor) { if (conn) sessionSupervisor.setActiveConnection(conn); log.info( - { count: tools.length, names: tools.map((t) => t.name) }, + { count: tools.length, names: tools.map((t) => t.name), ownerSkillId: skillId }, "Supervisor active: skipping in-memory tool re-registration; manifest proxies serve dispatches", ); return { registered: tools.map((t) => t.name) }; @@ -232,23 +235,19 @@ async function handleRegisterTools( // `registerExternalTools` is only consumed inside `initializeTools()` at // daemon boot; IPC children connect after boot, so route through // `registerSkillTools` into the live registry the agent-loop reads from. - const accepted = registerSkillTools(proxies); - - // `registerSkillTools` increments the registry refcount once per unique - // ownerSkillId in the batch; mirror that on the connection so disconnect - // issues exactly the matching number of decrements. - if (conn) { - const ownerIds = new Set(); - for (const tool of accepted) { - if (tool.ownerSkillId) ownerIds.add(tool.ownerSkillId); - } - for (const skillId of ownerIds) { - conn.addSkillToolsOwner(skillId); - } + // Owner is stamped registry-side from `skillId`; the proxy `Tool` objects + // carry no owner field. + const accepted = registerSkillTools(skillId, proxies); + + // `registerSkillTools` increments the registry refcount once per call; + // mirror that on the connection so disconnect issues exactly one matching + // decrement. + if (conn && accepted.length > 0) { + conn.addSkillToolsOwner(skillId); } log.info( - { count: accepted.length, names: accepted.map((t) => t.name) }, + { count: accepted.length, names: accepted.map((t) => t.name), ownerSkillId: skillId }, "Registered skill proxy tools via IPC", ); return { registered: accepted.map((t) => t.name) }; diff --git a/assistant/src/permissions/checker.ts b/assistant/src/permissions/checker.ts index 35e7ca63c01..ebb322c74e6 100644 --- a/assistant/src/permissions/checker.ts +++ b/assistant/src/permissions/checker.ts @@ -15,7 +15,7 @@ import { looksLikeHostPortShorthand, looksLikePathOnlyInput, } from "../tools/network/url-safety.js"; -import { getTool } from "../tools/registry.js"; +import { getTool, getToolOwner } from "../tools/registry.js"; import type { Tool } from "../tools/types.js"; import { getDeprecatedDir, @@ -156,11 +156,15 @@ function resolveSkillIdAndHash( * Returns false when the tool has no owning skill or the skill is not in * the catalog. Derived from `loadSkillCatalog()` at check time so the * answer reflects current catalog truth (managed overrides flip the bit - * without needing to re-register tools). + * without needing to re-register tools). Owner is looked up from the tool + * registry (`getToolOwner(name)`) rather than read from the `Tool` object, + * since ownership lives on the registry, not on the tool itself. */ function isToolOwnerSkillBundled(tool: Tool | undefined): boolean { - if (!tool?.ownerSkillId) return false; - const skill = loadSkillCatalog().find((s) => s.id === tool.ownerSkillId); + if (!tool) return false; + const owner = getToolOwner(tool.name); + if (owner?.kind !== "skill") return false; + const skill = loadSkillCatalog().find((s) => s.id === owner.id); return skill?.bundled ?? false; } diff --git a/assistant/src/tools/mcp/mcp-tool-factory.ts b/assistant/src/tools/mcp/mcp-tool-factory.ts index 9d16c0b9264..6f7df81abf4 100644 --- a/assistant/src/tools/mcp/mcp-tool-factory.ts +++ b/assistant/src/tools/mcp/mcp-tool-factory.ts @@ -49,7 +49,6 @@ export function createMcpTool( category: "mcp", defaultRiskLevel: riskLevel, origin: "mcp", - ownerMcpServerId: serverId, executionTarget: "host", input_schema: metadata.inputSchema as object, diff --git a/assistant/src/tools/registry.ts b/assistant/src/tools/registry.ts index cbff71f8d53..0907c5dd6e2 100644 --- a/assistant/src/tools/registry.ts +++ b/assistant/src/tools/registry.ts @@ -10,7 +10,7 @@ import { hostFileWriteTool } from "./host-filesystem/write.js"; import { hostShellTool } from "./host-terminal/host-shell.js"; import { toProviderSafeToolName } from "./provider-tool-name.js"; import { registerSystemTools } from "./system/register.js"; -import type { LoadedTool, Tool } from "./types.js"; +import type { LoadedTool, OwnerInfo, Tool } from "./types.js"; import { allUiSurfaceTools } from "./ui-surface/definitions.js"; import { registerUiSurfaceTools } from "./ui-surface/registry.js"; @@ -18,6 +18,15 @@ const log = getLogger("tool-registry"); const tools = new Map(); +// Authoritative map of tool ownership, keyed by tool name. Populated by the +// `register*` functions and read by `getToolOwner()`. Lives on the registry +// (not on the `Tool` object) so callers cannot spoof ownership by writing a +// field on the manifest — the only way to claim a tool is to go through a +// `register*` function, which stamps the owner from its arguments. Core +// tools intentionally have no entry here; `getToolOwner` returns `undefined` +// for them. +const ownersByName = new Map(); + // ── External tool registry ─────────────────────────────────────────── // Skills register their tools here at initialization time so the tool // manifest can include them without importing from `../skills/`. @@ -28,7 +37,10 @@ const tools = new Map(); // feature-flag check until after the daemon has run // `mergeDefaultWorkspaceConfig()`, so skills see the merged config // instead of forcing an early `loadConfig()` against unmerged defaults. -const externalToolProviders: Array<() => Tool[]> = []; +const externalToolProviders: Array<{ + owner: OwnerInfo; + provider: () => Tool[]; +}> = []; /** * Register tools provided by an external skill. Called during skill @@ -44,20 +56,30 @@ const externalToolProviders: Array<() => Tool[]> = []; * dependency: skills/load.ts → … → meet-join/register.ts → tool-manifest.ts * → skills/load.ts. Keeping it here lets external skill bootstraps import * from registry.ts, which is already a leaf in the dependency graph. + * + * `owner` records which extension produced these tools — typed + * {@link OwnerInfo} so ownership flows through `ownersByName` at + * `initializeTools()` time, the same way `register*` registers it for + * IPC-loaded tools. Eager (boot-time) skill bootstraps go through this + * path rather than `registerSkillTools`, so this is where their owner + * lookup gets established. */ export function registerExternalTools( + owner: OwnerInfo, toolsOrProvider: Tool[] | (() => Tool[]), ): void { const provider = typeof toolsOrProvider === "function" ? toolsOrProvider : () => toolsOrProvider; - externalToolProviders.push(provider); + externalToolProviders.push({ owner, provider }); } -/** Return all externally registered tools. */ -function getExternalTools(): Tool[] { - return externalToolProviders.flatMap((provider) => provider()); +/** Return all externally registered tools paired with their owners. */ +function getExternalTools(): Array<{ owner: OwnerInfo; tool: Tool }> { + return externalToolProviders.flatMap(({ owner, provider }) => + provider().map((tool) => ({ owner, tool })), + ); } // Snapshot of core tools captured after initializeTools() completes. @@ -76,6 +98,27 @@ const skillRefCount = new Map(); // separate and covers the case of two extensions choosing the same tool name. const pluginRefCount = new Map(); +/** + * Format an owner for log messages and error strings. Returns a stable + * human-readable description (e.g. `skill "deploy"`, `plugin "weather"`, + * `MCP server "github"`). When an owner is missing (core tool) or has an + * unrecognized kind, returns a fallback string so log/error sites never + * produce `undefined` interpolations. + */ +function describeOwner(owner: OwnerInfo | undefined): string { + if (!owner) return "core tool"; + switch (owner.kind) { + case "skill": + return `skill "${owner.id}"`; + case "plugin": + return `plugin "${owner.id}"`; + case "mcp": + return `MCP server "${owner.id}"`; + default: + return `${(owner as OwnerInfo).kind}-origin tool`; + } +} + function withProviderSafeToolName(tool: Tool): Tool { const safeName = toProviderSafeToolName(tool.name); if (safeName === tool.name) { @@ -107,14 +150,32 @@ export function getAllTools(): Tool[] { } /** - * Register multiple skill-origin tools at once. + * Return the recorded owner for a tool, or `undefined` if the tool is + * core-origin (no owner) or unknown. Consumers that need to gate behavior on + * which extension contributed a tool (permissions checker, approval-handler + * load hints, conversation-skill-tools projection) call this rather than + * reading owner off the `Tool` object — the registry is the single source of + * truth for ownership. + */ +export function getToolOwner(name: string): OwnerInfo | undefined { + return ownersByName.get(name); +} + +/** + * Register multiple skill-origin tools owned by `skillId`. + * * Skips any tool whose name collides with a core tool (logs a warning instead * of throwing so the remaining tools in the batch still get registered). * Throws if a tool name collides with a skill tool owned by a different skill. - * Allows replacement when the incoming tool has the same ownerSkillId as the existing one, - * which supports hot-reloading a skill without tearing down first. + * Allows replacement when the incoming tool has the same skill owner id as + * the existing one, which supports hot-reloading a skill without tearing + * down first. + * + * Ownership is recorded in {@link ownersByName} keyed by tool name; the + * `Tool` object itself carries no owner metadata, so callers cannot spoof + * ownership by writing fields on the manifest. */ -export function registerSkillTools(newTools: Tool[]): Tool[] { +export function registerSkillTools(skillId: string, newTools: Tool[]): Tool[] { // Filter out tools that collide with core tools, and validate the rest. const accepted: Tool[] = []; for (const tool of newTools) { @@ -123,67 +184,55 @@ export function registerSkillTools(newTools: Tool[]): Tool[] { const existingIsCore = existing.origin === "core" || !existing.origin; if (existingIsCore) { log.warn( - { toolName: tool.name, skillId: tool.ownerSkillId }, - `Skill "${tool.ownerSkillId}" tried to register tool "${tool.name}" which conflicts with a core tool. Skipping.`, + { toolName: tool.name, ownerSkillId: skillId }, + `Skill "${skillId}" tried to register tool "${tool.name}" which conflicts with a core tool. Skipping.`, ); continue; } // Existing is from a different origin (plugin/mcp) or a different // skill — skill tools can only replace themselves (hot-reload). - if ( - existing.origin !== "skill" || - existing.ownerSkillId !== tool.ownerSkillId - ) { - const owner = - existing.origin === "skill" - ? `skill "${existing.ownerSkillId}"` - : existing.origin === "plugin" - ? `plugin "${existing.ownerPluginId}"` - : existing.origin === "mcp" - ? `MCP server "${existing.ownerMcpServerId}"` - : `${existing.origin ?? "unknown"}-origin tool`; + const existingOwner = ownersByName.get(tool.name); + const existingSkillId = + existingOwner?.kind === "skill" ? existingOwner.id : undefined; + if (existing.origin !== "skill" || existingSkillId !== skillId) { throw new Error( - `Skill tool "${tool.name}" is already registered by ${owner}`, + `Skill tool "${tool.name}" is already registered by ${describeOwner(existingOwner)}`, ); } } accepted.push(tool); } - // Collect unique skill IDs from the batch to bump ref counts - const skillIds = new Set(); for (const tool of accepted) { tools.set(tool.name, tool); - if (tool.ownerSkillId) skillIds.add(tool.ownerSkillId); + ownersByName.set(tool.name, { kind: "skill", id: skillId }); log.info( - { name: tool.name, ownerSkillId: tool.ownerSkillId }, + { name: tool.name, ownerSkillId: skillId }, "Skill tool registered", ); } - for (const id of skillIds) { - skillRefCount.set(id, (skillRefCount.get(id) ?? 0) + 1); + if (accepted.length > 0) { + skillRefCount.set(skillId, (skillRefCount.get(skillId) ?? 0) + 1); } return accepted; } /** - * Register tools contributed by a plugin. Stamps `origin: "plugin"` and - * `ownerPluginId: pluginName` on every incoming tool so plugin ownership is - * tracked in a namespace disjoint from skill tools — if a plugin's - * `manifest.name` happens to match a skill id, the two do not share refcount - * state or conflict-detection paths. + * Register tools contributed by the plugin named `pluginName`. Stamps + * `origin: "plugin"` on every incoming tool and records the plugin owner in + * {@link ownersByName} keyed by tool name — ownership lives on the registry, + * never on the `Tool` object itself, so the bootstrap cannot be spoofed into + * claiming tools on behalf of an unrelated extension by forging fields on + * the manifest. Plugin ownership is tracked in a namespace disjoint from + * skill tools: if a plugin's `manifest.name` happens to match a skill id, + * the two do not share refcount state or conflict-detection paths. * * Conflict handling mirrors {@link registerSkillTools}: collisions with core * tools log a warning and skip; collisions with tools owned by a different * plugin, skill, or MCP server throw; re-registering the same plugin's own * tool (hot reload) is allowed. - * - * The stamp is authoritative: any pre-existing `origin` / `ownerPluginId` / - * `ownerSkillId` / `ownerMcpServerId` fields on the incoming tools are - * overwritten so the bootstrap cannot be spoofed into claiming tools on - * behalf of an unrelated extension. */ export function registerPluginTools( pluginName: string, @@ -194,9 +243,6 @@ export function registerPluginTools( ...pluginTool, category: "plugin", origin: "plugin" as const, - ownerPluginId: pluginName, - ownerSkillId: undefined, - ownerMcpServerId: undefined, }; return withProviderSafeToolName(tool); }); @@ -208,22 +254,23 @@ export function registerPluginTools( const existingIsCore = existing.origin === "core" || !existing.origin; if (existingIsCore) { log.warn( - { toolName: tool.name, pluginName }, + { toolName: tool.name, ownerPluginId: pluginName }, `Plugin "${pluginName}" tried to register tool "${tool.name}" which conflicts with a core tool. Skipping.`, ); continue; } - if (existing.origin === "plugin") { - if (existing.ownerPluginId !== pluginName) { + const existingOwner = ownersByName.get(tool.name); + if (existingOwner?.kind === "plugin") { + if (existingOwner.id !== pluginName) { throw new Error( - `Plugin tool "${tool.name}" is already registered by plugin "${existing.ownerPluginId}"`, + `Plugin tool "${tool.name}" is already registered by plugin "${existingOwner.id}"`, ); } // Same plugin re-registering its own tool (hot reload) — allow. } else { // Conflict with a skill or MCP-owned tool. throw new Error( - `Plugin "${pluginName}" tried to register tool "${tool.name}" which conflicts with a ${existing.origin}-origin tool`, + `Plugin "${pluginName}" tried to register tool "${tool.name}" which conflicts with ${describeOwner(existingOwner)}`, ); } } @@ -232,8 +279,9 @@ export function registerPluginTools( for (const tool of accepted) { tools.set(tool.name, tool); + ownersByName.set(tool.name, { kind: "plugin", id: pluginName }); log.info( - { name: tool.name, ownerPluginId: tool.ownerPluginId }, + { name: tool.name, ownerPluginId: pluginName }, "Plugin tool registered", ); } @@ -262,10 +310,11 @@ export function unregisterPluginTools(pluginName: string): void { } pluginRefCount.delete(pluginName); - for (const [name, tool] of tools) { - if (tool.origin === "plugin" && tool.ownerPluginId === pluginName) { + for (const [name, owner] of ownersByName) { + if (owner.kind === "plugin" && owner.id === pluginName) { tools.delete(name); - log.info({ name, pluginName }, "Plugin tool unregistered"); + ownersByName.delete(name); + log.info({ name, ownerPluginId: pluginName }, "Plugin tool unregistered"); } } } @@ -294,20 +343,25 @@ export function unregisterSkillTools(skillId: string): void { // Last reference - actually remove the tools skillRefCount.delete(skillId); - for (const [name, tool] of tools) { - if (tool.origin === "skill" && tool.ownerSkillId === skillId) { + for (const [name, owner] of ownersByName) { + if (owner.kind === "skill" && owner.id === skillId) { tools.delete(name); - log.info({ name, skillId }, "Skill tool unregistered"); + ownersByName.delete(name); + log.info({ name, ownerSkillId: skillId }, "Skill tool unregistered"); } } } /** - * Register multiple MCP-origin tools at once. + * Register multiple MCP-origin tools owned by the MCP server `serverId`. + * * Skips any tool whose name collides with a core tool (logs a warning). * Throws if a tool name collides with a tool owned by a different MCP server. + * + * Ownership is recorded in {@link ownersByName} keyed by tool name; the + * `Tool` object itself carries no owner metadata. */ -export function registerMcpTools(newTools: Tool[]): Tool[] { +export function registerMcpTools(serverId: string, newTools: Tool[]): Tool[] { const accepted: Tool[] = []; for (const tool of newTools) { const existing = tools.get(tool.name); @@ -315,39 +369,29 @@ export function registerMcpTools(newTools: Tool[]): Tool[] { const existingIsCore = existing.origin === "core" || !existing.origin; if (existingIsCore) { log.warn( - { toolName: tool.name, serverId: tool.ownerMcpServerId }, - `MCP server "${tool.ownerMcpServerId}" tried to register tool "${tool.name}" which conflicts with a core tool. Skipping.`, + { toolName: tool.name, ownerMcpServerId: serverId }, + `MCP server "${serverId}" tried to register tool "${tool.name}" which conflicts with a core tool. Skipping.`, ); continue; } - if (existing.origin === "skill") { - log.warn( - { - toolName: tool.name, - serverId: tool.ownerMcpServerId, - skillId: existing.ownerSkillId, - }, - `MCP server "${tool.ownerMcpServerId}" tried to register tool "${tool.name}" which conflicts with skill tool from "${existing.ownerSkillId}". Skipping.`, - ); - continue; - } - if (existing.origin === "plugin") { + const existingOwner = ownersByName.get(tool.name); + if ( + existingOwner?.kind === "skill" || + existingOwner?.kind === "plugin" + ) { log.warn( { toolName: tool.name, - serverId: tool.ownerMcpServerId, - pluginName: existing.ownerPluginId, + ownerMcpServerId: serverId, + existingOwner, }, - `MCP server "${tool.ownerMcpServerId}" tried to register tool "${tool.name}" which conflicts with plugin tool from "${existing.ownerPluginId}". Skipping.`, + `MCP server "${serverId}" tried to register tool "${tool.name}" which conflicts with ${describeOwner(existingOwner)}. Skipping.`, ); continue; } - if ( - existing.origin === "mcp" && - existing.ownerMcpServerId !== tool.ownerMcpServerId - ) { + if (existingOwner?.kind === "mcp" && existingOwner.id !== serverId) { throw new Error( - `MCP tool "${tool.name}" is already registered by MCP server "${existing.ownerMcpServerId}"`, + `MCP tool "${tool.name}" is already registered by MCP server "${existingOwner.id}"`, ); } } @@ -356,8 +400,9 @@ export function registerMcpTools(newTools: Tool[]): Tool[] { for (const tool of accepted) { tools.set(tool.name, tool); + ownersByName.set(tool.name, { kind: "mcp", id: serverId }); log.info( - { name: tool.name, ownerMcpServerId: tool.ownerMcpServerId }, + { name: tool.name, ownerMcpServerId: serverId }, "MCP tool registered", ); } @@ -369,9 +414,10 @@ export function registerMcpTools(newTools: Tool[]): Tool[] { * Unregister all MCP-origin tools from the registry. */ export function unregisterAllMcpTools(): void { - for (const [name, tool] of tools) { - if (tool.origin === "mcp") { + for (const [name, owner] of ownersByName) { + if (owner.kind === "mcp") { tools.delete(name); + ownersByName.delete(name); log.info({ name }, "MCP tool unregistered (reload)"); } } @@ -440,10 +486,13 @@ export async function initializeTools(): Promise { // External skill tools — registered by skill bootstrap modules via // `registerExternalTools()`. Called at init time (not spread into // `explicitTools`) so registrations that happen between module-load - // and `initializeTools()` are picked up. - const extTools = getExternalTools(); - for (const tool of extTools) { + // and `initializeTools()` are picked up. Each provider pairs its tools + // with an OwnerInfo so the registry can record ownership in + // {@link ownersByName} alongside the bare `registerTool()` install. + const extEntries = getExternalTools(); + for (const { owner, tool } of extEntries) { registerTool(tool); + ownersByName.set(tool.name, owner); } // Host tools are registered explicitly so host access stays opt-in until @@ -481,7 +530,7 @@ export async function initializeTools(): Promise { const manifestToolNames = new Set([ ...eagerModuleToolNames, ...explicitTools.map((t: Tool) => t.name), - ...extTools.map((t: Tool) => t.name), + ...extEntries.map(({ tool }) => tool.name), ...hostTools.map((t: Tool) => t.name), ...cesTools.map((t: Tool) => t.name), ...allComputerUseTools.map((t: Tool) => t.name), @@ -514,6 +563,7 @@ export async function initializeTools(): Promise { */ export function __resetRegistryForTesting(): void { tools.clear(); + ownersByName.clear(); skillRefCount.clear(); pluginRefCount.clear(); @@ -531,6 +581,7 @@ export function __resetRegistryForTesting(): void { */ export function __clearRegistryForTesting(): void { tools.clear(); + ownersByName.clear(); skillRefCount.clear(); pluginRefCount.clear(); } diff --git a/assistant/src/tools/skills/skill-tool-factory.ts b/assistant/src/tools/skills/skill-tool-factory.ts index 83db5c99458..d114d6db9e2 100644 --- a/assistant/src/tools/skills/skill-tool-factory.ts +++ b/assistant/src/tools/skills/skill-tool-factory.ts @@ -42,11 +42,13 @@ function validateNoUnknownParams( /** * Create a runtime Tool object from a manifest entry. * Maps SkillToolEntry metadata to the Tool interface and routes execution - * through the skill script runner. + * through the skill script runner. Ownership (the originating skill id) is + * recorded by the tool registry at `registerSkillTools(skillId, tools)` + * time, not stamped on the `Tool` object — see + * {@link ../../tools/registry.getToolOwner}. */ export function createSkillTool( entry: SkillToolEntry, - skillId: string, skillDir: string, versionHash: string, bundled?: boolean, @@ -57,7 +59,6 @@ export function createSkillTool( category: entry.category, defaultRiskLevel: riskMap[entry.risk], origin: "skill", - ownerSkillId: skillId, executionTarget: entry.execution_target as ExecutionTarget, input_schema: entry.input_schema as object, @@ -84,15 +85,17 @@ export function createSkillTool( /** * Create runtime Tool objects from all entries in a manifest. + * The caller is responsible for passing the resulting array to + * `registerSkillTools(skillId, tools)`, which is where ownership is + * recorded. */ export function createSkillToolsFromManifest( entries: SkillToolEntry[], - skillId: string, skillDir: string, versionHash: string, bundled?: boolean, ): Tool[] { return entries.map((entry) => - createSkillTool(entry, skillId, skillDir, versionHash, bundled), + createSkillTool(entry, skillDir, versionHash, bundled), ); } diff --git a/assistant/src/tools/tool-approval-handler.ts b/assistant/src/tools/tool-approval-handler.ts index c9923501843..cad499025f0 100644 --- a/assistant/src/tools/tool-approval-handler.ts +++ b/assistant/src/tools/tool-approval-handler.ts @@ -10,7 +10,7 @@ import { createOrReuseToolGrantRequest } from "../runtime/tool-grant-request-hel import { redactSecrets } from "../security/secret-scanner.js"; import { computeToolApprovalDigest } from "../security/tool-approval-digest.js"; import { getLogger } from "../util/logger.js"; -import { getAllTools, getTool } from "./registry.js"; +import { getAllTools, getTool, getToolOwner } from "./registry.js"; import { isSideEffectTool } from "./side-effects.js"; import { summarizeToolInput } from "./tool-input-summary.js"; import { @@ -368,8 +368,11 @@ export class ToolApprovalHandler { // Gate tools not active for the current turn if (context.allowedToolNames && !context.allowedToolNames.has(name)) { - const loadHint = tool.ownerSkillId - ? `Load the "${tool.ownerSkillId}" skill that provides this tool first.` + const owner = getToolOwner(name); + const ownerSkillId = + owner?.kind === "skill" ? owner.id : undefined; + const loadHint = ownerSkillId + ? `Load the "${ownerSkillId}" skill that provides this tool first.` : `Load the skill that provides this tool first.`; const msg = `Tool "${name}" is not currently active. ${loadHint}`; const durationMs = Date.now() - startTime; diff --git a/assistant/src/tools/types.ts b/assistant/src/tools/types.ts index 0896b64d94e..613ae225486 100644 --- a/assistant/src/tools/types.ts +++ b/assistant/src/tools/types.ts @@ -340,16 +340,24 @@ export interface ToolDefinition { /** Tool after the loader has derived its name and filled defaults. */ export type LoadedTool = Required & { name: string }; +/** The kind of extension that owns a tool. Core tools have no owner. */ +export type OwnerKind = "skill" | "mcp" | "plugin"; + +/** + * Identifies which extension owns a tool (skill / plugin / MCP server). + * Tracked by the tool registry keyed by tool name, not stored on the `Tool` + * object itself — query via {@link ../tools/registry.getToolOwner}. + */ +export interface OwnerInfo { + kind: OwnerKind; + /** ID of the owning extension (skill id / plugin name / MCP server id). */ + id: string; +} + export interface Tool extends LoadedTool { category: string; /** When set to 'proxy', the tool is forwarded to a connected client rather than executed locally. */ executionMode?: "local" | "proxy"; /** Whether this tool is a core built-in, provided by a skill, contributed by a plugin, or from an MCP server. */ origin?: "core" | "skill" | "mcp" | "plugin"; - /** If origin is 'skill', the ID of the owning skill. */ - ownerSkillId?: string; - /** If origin is 'mcp', the ID of the owning MCP server. */ - ownerMcpServerId?: string; - /** If origin is 'plugin', the name of the owning plugin. */ - ownerPluginId?: string; } diff --git a/packages/skill-host-contracts/src/client.ts b/packages/skill-host-contracts/src/client.ts index 95ca444f12e..aa8efd30e3b 100644 --- a/packages/skill-host-contracts/src/client.ts +++ b/packages/skill-host-contracts/src/client.ts @@ -1082,7 +1082,6 @@ export class SkillHostClient implements SkillHost { category: t.category, executionTarget: t.executionTarget, executionMode: t.executionMode ?? "proxy", - ownerSkillId: t.ownerSkillId ?? this.options.skillId, }; }); // Cache the provider so `skill.dispatch_tool` can resolve a tool @@ -1092,10 +1091,17 @@ export class SkillHostClient implements SkillHost { "skill.dispatch_tool", this.dispatchTool.bind(this), ); + // Skill ownership travels at the params level (not per-tool) — the + // daemon-side registry stamps the owner from `skillId` into its + // own `ownersByName` map keyed by tool name. The `Tool` objects on + // the wire stay free of owner metadata so callers cannot spoof + // ownership by forging a field on the manifest. + // // Fire-and-forget; registration failures surface in the daemon log. - this.call("host.registries.register_tools", { tools: manifests }).catch( - swallow, - ); + this.call("host.registries.register_tools", { + skillId: this.options.skillId, + tools: manifests, + }).catch(swallow); }, registerSkillRoute: (route: SkillRoute): SkillRouteHandle => { // Cache the route by its regex source — `skill.dispatch_route` uses diff --git a/packages/skill-host-contracts/src/tool-types.ts b/packages/skill-host-contracts/src/tool-types.ts index 88878e776b7..b828d05af80 100644 --- a/packages/skill-host-contracts/src/tool-types.ts +++ b/packages/skill-host-contracts/src/tool-types.ts @@ -28,6 +28,22 @@ export type ExecutionTarget = "sandbox" | "host"; +/** The kind of extension that owns a tool. Core tools have no owner. */ +export type OwnerKind = "skill" | "mcp" | "plugin"; + +/** + * Identifies which extension owns a tool (skill / plugin / MCP server). + * Tracked by the daemon's tool registry keyed by tool name, not stored on + * the `Tool` object itself. The registry function `getToolOwner(name)` + * returns this shape; ownership is established at `register_tools` time + * via the `host.registries.register_tools` IPC method. + */ +export interface OwnerInfo { + kind: OwnerKind; + /** ID of the owning extension (skill id / plugin name / MCP server id). */ + id: string; +} + export enum RiskLevel { Low = "low", Medium = "medium", @@ -423,12 +439,6 @@ export interface Tool { executionMode?: "local" | "proxy"; /** Whether this tool is a core built-in, provided by a skill, contributed by a plugin, or from an MCP server. */ origin?: "core" | "skill" | "mcp" | "plugin"; - /** If origin is 'skill', the ID of the owning skill. */ - ownerSkillId?: string; - /** If origin is 'mcp', the ID of the owning MCP server. */ - ownerMcpServerId?: string; - /** If origin is 'plugin', the name of the owning plugin. */ - ownerPluginId?: string; /** Declared execution target from the skill manifest. Used by resolveExecutionTarget * to accurately label lifecycle events for skill-provided tools. */ executionTarget?: ExecutionTarget;