tools: collapse owner{Skill,Plugin,Mcp}Id into OwnerInfo on Tool

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: <plugin.name>` 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: <plugin.name> }` 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 = {

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,35 +128,37 @@ 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",
   category: "skill",
   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",
   category: "skill",
   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");

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");

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<string>();
-    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()) {

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,26 +107,21 @@ 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<string>();
-    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('<loaded_skill id="deploy" />'),
     ];
 
-    // 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");
   });
 });