From 6bedf67dd29a2a41008d4790eb5476f98bcbb514 Mon Sep 17 00:00:00 2001 From: "vellum-apollo-bot[bot]" <221541397+vellum-apollo-bot[bot]@users.noreply.github.com> Date: Tue, 26 May 2026 18:33:11 +0000 Subject: [PATCH] tools: drop ownerSkillVersionHash denormalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ownerSkillVersionHash was a denormalization on Tool that had ZERO production consumers. The tamper-detection security mechanism that matters — blocking execution when a skill's source changed since approval — is the `expectedSkillVersionHash` option captured into each skill tool's `execute()` closure, checked inside runSkillToolScript / runSkillToolScriptSandbox. Nothing reads the field off the Tool object at runtime. Same shape as PR #32083: removed from Tool (assistant + contracts), IPC schema + buildProxyTool relay, the IPC client payload builder, skill-tool-factory, meet-manifest-loader, and the registry's plugin-clear. In meet-manifest-loader the field's removal left buildProxyTool's `manifestHash` param dead — dropped the param and updated the single callsite. Test changes: - Deleted the dedicated 'version hash plumbing to projected tools' suite in conversation-skill-tools.test.ts (110 lines). The suite's comment claimed downstream executor.ts uses the field to build policy context — stale; executor.ts has no reference to the field. Re-aiming the assertions would only test that the test's own mock factory received the right arg, which tests the mock, not the production code. Real tamper-detection coverage already lives in skill-script-runner-host.test.ts (5+ cases) and skill-script-runner.test.ts asserting `expectedSkillVersionHash` directly. - Deleted two skill-tool-factory tests that asserted only the field ('sets ownerSkillVersionHash from versionHash', 'passes versionHash through to all created tools'). Kept 'execute() works correctly when versionHash is provided' since it exercises the real closure → runner integrity check; trimmed the stale field assertion at the tail. - Updated mock factories in 5 sibling tests and the spoofed-ownership case in plugin-tool-contribution.test.ts. - skills/meet-join/__tests__/entrypoint.test.ts's fakeTool helper also still carried `ownerSkillBundled: true` — a leftover from PR #32083 that the prior `assistant packages` grep scope missed. Cleaned both fields here. Typecheck and lint clean. All 10 touched test files pass individually (plus skill-script-runner.test.ts and skill-script-runner-host.test.ts as smoke check for the real tamper path). A pre-existing cross-file mock.module pollution causes batch failures on clean main — confirmed identical 0/9 batch result with `git stash` baseline. --- ...ersation-app-control-instantiation.test.ts | 3 +- .../conversation-skill-tools.test.ts | 117 +----------------- .../plugin-tool-contribution.test.ts | 6 +- .../skill-projection-feature-flag.test.ts | 3 +- .../skill-projection.benchmark.test.ts | 3 +- .../src/__tests__/skill-tool-factory.test.ts | 32 ----- assistant/src/__tests__/tool-executor.test.ts | 2 - .../__tests__/meet-manifest-loader.test.ts | 1 - assistant/src/daemon/meet-manifest-loader.ts | 4 +- assistant/src/ipc/skill-routes/registries.ts | 2 - assistant/src/tools/registry.ts | 1 - .../src/tools/skills/skill-tool-factory.ts | 1 - assistant/src/tools/types.ts | 2 - packages/skill-host-contracts/src/client.ts | 1 - .../skill-host-contracts/src/tool-types.ts | 2 - skills/catalog.json | 4 +- skills/meet-join/__tests__/entrypoint.test.ts | 2 - 17 files changed, 9 insertions(+), 177 deletions(-) diff --git a/assistant/src/__tests__/conversation-app-control-instantiation.test.ts b/assistant/src/__tests__/conversation-app-control-instantiation.test.ts index 5fde0c13081..f9deba12e43 100644 --- a/assistant/src/__tests__/conversation-app-control-instantiation.test.ts +++ b/assistant/src/__tests__/conversation-app-control-instantiation.test.ts @@ -102,7 +102,7 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ entries: SkillToolManifest["tools"], skillId: string, _skillDir: string, - versionHash: string, + _versionHash: string, _bundled?: boolean, ): Tool[] => entries.map((entry) => ({ @@ -113,7 +113,6 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ executionTarget: "sandbox" as const, origin: "skill" as const, ownerSkillId: skillId, - ownerSkillVersionHash: versionHash, input_schema: entry.input_schema as object, execute: async () => ({ content: "", isError: false }), })), diff --git a/assistant/src/__tests__/conversation-skill-tools.test.ts b/assistant/src/__tests__/conversation-skill-tools.test.ts index 6c010d3ac89..8a26c4e86db 100644 --- a/assistant/src/__tests__/conversation-skill-tools.test.ts +++ b/assistant/src/__tests__/conversation-skill-tools.test.ts @@ -96,7 +96,7 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ entries: SkillToolManifest["tools"], skillId: string, _skillDir: string, - versionHash: string, + _versionHash: string, _bundled?: boolean, ): Tool[] => { return entries.map((entry) => ({ @@ -107,7 +107,6 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ executionTarget: "sandbox" as const, origin: "skill" as const, ownerSkillId: skillId, - ownerSkillVersionHash: versionHash, input_schema: entry.input_schema as object, execute: async () => ({ content: "", isError: false }), })); @@ -2256,120 +2255,6 @@ describe("hash change re-prompt regressions (PR 35)", () => { }); }); -// --------------------------------------------------------------------------- -// Version hash plumbing regression tests -// Verify that createSkillToolsFromManifest receives the computed hash and -// that projected tools carry ownerSkillVersionHash, which downstream -// components (executor.ts) use to build policy context. -// --------------------------------------------------------------------------- - -describe("version hash plumbing to projected tools", () => { - let sessionState: Map; - - beforeEach(() => { - mockCatalog = []; - mockManifests = {}; - mockRegisteredTools = new Map(); - mockUnregisteredSkillIds = []; - mockSkillRefCount = new Map(); - mockVersionHashes = {}; - mockVersionHashErrors = new Set(); - sessionState = new Map(); - }); - - test("projected tools carry ownerSkillVersionHash matching the computed hash", () => { - mockCatalog = [makeSkill("deploy")]; - mockManifests = { deploy: makeManifest(["deploy_run", "deploy_status"]) }; - mockVersionHashes = { deploy: "v1:secure-hash-abc" }; - - const history: Message[] = [ - ...skillLoadMessages(''), - ]; - - projectSkillTools(history, { previouslyActiveSkillIds: sessionState }); - - const tools = mockRegisteredTools.get("deploy"); - expect(tools).toBeDefined(); - expect(tools!.length).toBe(2); - - // Every tool created for this skill must carry the version hash - for (const tool of tools!) { - expect(tool.ownerSkillVersionHash).toBe("v1:secure-hash-abc"); - } - }); - - test("after hash change re-registration, new tools carry the updated hash", () => { - mockCatalog = [makeSkill("deploy")]; - mockManifests = { deploy: makeManifest(["deploy_run"]) }; - mockVersionHashes = { deploy: "v1:hash-before" }; - - const history: Message[] = [ - ...skillLoadMessages(''), - ]; - - // Turn 1: register with original hash - projectSkillTools(history, { previouslyActiveSkillIds: sessionState }); - const toolsV1 = mockRegisteredTools.get("deploy"); - expect(toolsV1).toBeDefined(); - expect(toolsV1![0].ownerSkillVersionHash).toBe("v1:hash-before"); - - // Simulate file edit — hash changes - mockVersionHashes = { deploy: "v2:hash-after" }; - - // Turn 2: re-registration with new hash - projectSkillTools(history, { previouslyActiveSkillIds: sessionState }); - const toolsV2 = mockRegisteredTools.get("deploy"); - expect(toolsV2).toBeDefined(); - - // The most recently registered tool should carry the new hash - const lastTool = toolsV2![toolsV2!.length - 1]; - expect(lastTool.ownerSkillVersionHash).toBe("v2:hash-after"); - }); - - test("tools for multiple co-active skills each carry their own version hash", () => { - mockCatalog = [makeSkill("deploy"), makeSkill("oncall")]; - mockManifests = { - deploy: makeManifest(["deploy_run"]), - oncall: makeManifest(["oncall_page"]), - }; - mockVersionHashes = { - deploy: "v1:deploy-hash-123", - oncall: "v1:oncall-hash-456", - }; - - const history: Message[] = [ - ...skillLoadMessages(''), - ...skillLoadMessages(''), - ]; - - projectSkillTools(history, { previouslyActiveSkillIds: sessionState }); - - const deployTools = mockRegisteredTools.get("deploy"); - expect(deployTools).toBeDefined(); - expect(deployTools![0].ownerSkillVersionHash).toBe("v1:deploy-hash-123"); - - const oncallTools = mockRegisteredTools.get("oncall"); - expect(oncallTools).toBeDefined(); - expect(oncallTools![0].ownerSkillVersionHash).toBe("v1:oncall-hash-456"); - }); - - test("default hash is used and plumbed when no explicit hash override is set", () => { - mockCatalog = [makeSkill("deploy")]; - mockManifests = { deploy: makeManifest(["deploy_run"]) }; - // No mockVersionHashes override — mock returns 'v1:default-hash-deploy' - - const history: Message[] = [ - ...skillLoadMessages(''), - ]; - - projectSkillTools(history, { previouslyActiveSkillIds: sessionState }); - - const tools = mockRegisteredTools.get("deploy"); - expect(tools).toBeDefined(); - expect(tools![0].ownerSkillVersionHash).toBe("v1:default-hash-deploy"); - }); -}); - // --------------------------------------------------------------------------- // Child skill includes: no auto-activation // --------------------------------------------------------------------------- diff --git a/assistant/src/__tests__/plugin-tool-contribution.test.ts b/assistant/src/__tests__/plugin-tool-contribution.test.ts index 700ad8a26eb..672b7e451f7 100644 --- a/assistant/src/__tests__/plugin-tool-contribution.test.ts +++ b/assistant/src/__tests__/plugin-tool-contribution.test.ts @@ -318,8 +318,8 @@ describe("registerPluginTools / unregisterPluginTools helpers", () => { // 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 / ownerSkillVersionHash) so the stamped tool cannot - // leak across namespaces or spoof skill-origin auto-allow. + // ownerMcpServerId) so 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 @@ -329,14 +329,12 @@ describe("registerPluginTools / unregisterPluginTools helpers", () => { ...makeFakeTool("pt_spoof"), origin: "skill", ownerSkillId: "some-other-skill", - ownerSkillVersionHash: "deadbeef", } 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(retrieved?.ownerSkillVersionHash).toBeUndefined(); }); test("unregisterPluginTools removes the plugin's tools", () => { diff --git a/assistant/src/__tests__/skill-projection-feature-flag.test.ts b/assistant/src/__tests__/skill-projection-feature-flag.test.ts index d26c3129a30..8000e7d1e4a 100644 --- a/assistant/src/__tests__/skill-projection-feature-flag.test.ts +++ b/assistant/src/__tests__/skill-projection-feature-flag.test.ts @@ -113,7 +113,7 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ entries: SkillToolManifest["tools"], skillId: string, _skillDir: string, - versionHash: string, + _versionHash: string, _bundled?: boolean, ): Tool[] => { return entries.map((entry) => ({ @@ -124,7 +124,6 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ executionTarget: "sandbox" as const, origin: "skill" as const, ownerSkillId: skillId, - ownerSkillVersionHash: versionHash, input_schema: entry.input_schema as object, execute: async () => ({ content: "", isError: false }), })); diff --git a/assistant/src/__tests__/skill-projection.benchmark.test.ts b/assistant/src/__tests__/skill-projection.benchmark.test.ts index 247695c45a9..45170dcc089 100644 --- a/assistant/src/__tests__/skill-projection.benchmark.test.ts +++ b/assistant/src/__tests__/skill-projection.benchmark.test.ts @@ -102,7 +102,7 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ entries: Array<{ name: string; description: string; input_schema: object }>, skillId: string, _skillDir: string, - versionHash: string, + _versionHash: string, _bundled?: boolean, ) => entries.map((e) => ({ @@ -112,7 +112,6 @@ mock.module("../tools/skills/skill-tool-factory.js", () => ({ defaultRiskLevel: "low", origin: "skill" as const, ownerSkillId: skillId, - ownerSkillVersionHash: versionHash, input_schema: e.input_schema, execute: async () => ({ content: "", isError: false }), })), diff --git a/assistant/src/__tests__/skill-tool-factory.test.ts b/assistant/src/__tests__/skill-tool-factory.test.ts index f0f75eabcc1..7b374d71cab 100644 --- a/assistant/src/__tests__/skill-tool-factory.test.ts +++ b/assistant/src/__tests__/skill-tool-factory.test.ts @@ -97,18 +97,6 @@ describe("createSkillTool", () => { expect(tool.ownerSkillId).toBe("weather-skill"); }); - test("sets ownerSkillVersionHash from versionHash", () => { - const hash = "v1:abc123def456"; - const tool = createSkillTool( - makeEntry(), - "my-skill", - "/skills/my-skill", - hash, - ); - - expect(tool.ownerSkillVersionHash).toBe(hash); - }); - test.each([ ["low", RiskLevel.Low], ["medium", RiskLevel.Medium], @@ -249,24 +237,6 @@ describe("createSkillToolsFromManifest", () => { expect(tools).toEqual([]); }); - test("passes versionHash through to all created tools", () => { - const hash = "v1:deadbeef"; - const entries: SkillToolEntry[] = [ - makeEntry({ name: "alpha" }), - makeEntry({ name: "beta" }), - ]; - - const tools = createSkillToolsFromManifest( - entries, - "versioned-skill", - "/skills/versioned", - hash, - ); - - for (const tool of tools) { - expect(tool.ownerSkillVersionHash).toBe(hash); - } - }); }); // --------------------------------------------------------------------------- @@ -389,7 +359,5 @@ describe("createSkillTool — version hash plumbing to runner", () => { const parsed = JSON.parse(result.content); expect(parsed.input).toEqual({ query: "test" }); expect(parsed.workingDir).toBe("/my/project"); - // Confirm the tool still has the hash stored - expect(tool.ownerSkillVersionHash).toBe(hash); }); }); diff --git a/assistant/src/__tests__/tool-executor.test.ts b/assistant/src/__tests__/tool-executor.test.ts index 024d0ef2baf..9c0e256553b 100644 --- a/assistant/src/__tests__/tool-executor.test.ts +++ b/assistant/src/__tests__/tool-executor.test.ts @@ -373,7 +373,6 @@ describe("ToolExecutor policy context plumbing", () => { defaultRiskLevel: RiskLevel.Low, origin: "skill" as const, ownerSkillId: "my-skill-123", - ownerSkillVersionHash: "abc123hash", executionTarget: "sandbox" as const, input_schema: { type: "object" as const, properties: {} }, execute: async () => fakeToolResult, @@ -455,7 +454,6 @@ describe("ToolExecutor policy context plumbing", () => { defaultRiskLevel: RiskLevel.Low, origin: "skill" as const, ownerSkillId: "host-skill", - ownerSkillVersionHash: "host-hash", executionTarget: "host" as const, input_schema: { type: "object" as const, properties: {} }, execute: async () => fakeToolResult, diff --git a/assistant/src/daemon/__tests__/meet-manifest-loader.test.ts b/assistant/src/daemon/__tests__/meet-manifest-loader.test.ts index cb344138e25..8ab0c6b672c 100644 --- a/assistant/src/daemon/__tests__/meet-manifest-loader.test.ts +++ b/assistant/src/daemon/__tests__/meet-manifest-loader.test.ts @@ -181,7 +181,6 @@ describe("loadMeetManifestProxies", () => { expect(t.executionMode).toBe("proxy"); expect(t.origin).toBe("skill"); expect(t.ownerSkillId).toBe("meet-join"); - expect(t.ownerSkillVersionHash).toBe(FIXTURE_MANIFEST.sourceHash); expect(t.input_schema).toEqual( FIXTURE_MANIFEST.tools[0]!.input_schema, ); diff --git a/assistant/src/daemon/meet-manifest-loader.ts b/assistant/src/daemon/meet-manifest-loader.ts index c22ac13bb79..d429094be4f 100644 --- a/assistant/src/daemon/meet-manifest-loader.ts +++ b/assistant/src/daemon/meet-manifest-loader.ts @@ -129,7 +129,6 @@ function serializeToolContext(context: ToolContext): Record { function buildProxyTool( entry: ManifestToolEntry, supervisor: MeetHostSupervisor, - manifestHash: string, ): Tool { const risk = coerceRiskLevel(entry.risk, entry.name); return { @@ -142,7 +141,6 @@ function buildProxyTool( executionTarget: "host" as ExecutionTarget, origin: "skill", ownerSkillId: MEET_SKILL_ID, - ownerSkillVersionHash: manifestHash, execute: async (input, context) => { // `dispatchTool` ensures the meet-host child is up + connected // before sending the frame, so callers don't need a separate @@ -363,7 +361,7 @@ export async function loadMeetManifestProxies( // contract as the in-process skill's provider closure. registerTools(() => manifest.tools.map((entry) => - buildProxyTool(entry, supervisor, manifest.sourceHash), + buildProxyTool(entry, supervisor), ), ); diff --git a/assistant/src/ipc/skill-routes/registries.ts b/assistant/src/ipc/skill-routes/registries.ts index 146870b795a..9e489ca8ed1 100644 --- a/assistant/src/ipc/skill-routes/registries.ts +++ b/assistant/src/ipc/skill-routes/registries.ts @@ -53,7 +53,6 @@ const ToolManifestSchema = z.object({ // 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), - ownerSkillVersionHash: z.string().optional(), }); export type ToolManifest = z.infer; @@ -194,7 +193,6 @@ function buildProxyTool(manifest: ToolManifest): Tool { }), origin: "skill", ownerSkillId: manifest.ownerSkillId, - ownerSkillVersionHash: manifest.ownerSkillVersionHash, execute: async () => { // Only reached when no supervisor is attached (tests/boot race); // the supervisor short-circuit above replaces this with the diff --git a/assistant/src/tools/registry.ts b/assistant/src/tools/registry.ts index f9e78eafd9d..cbff71f8d53 100644 --- a/assistant/src/tools/registry.ts +++ b/assistant/src/tools/registry.ts @@ -197,7 +197,6 @@ export function registerPluginTools( ownerPluginId: pluginName, ownerSkillId: undefined, ownerMcpServerId: undefined, - ownerSkillVersionHash: undefined, }; return withProviderSafeToolName(tool); }); diff --git a/assistant/src/tools/skills/skill-tool-factory.ts b/assistant/src/tools/skills/skill-tool-factory.ts index e8c26231c80..83db5c99458 100644 --- a/assistant/src/tools/skills/skill-tool-factory.ts +++ b/assistant/src/tools/skills/skill-tool-factory.ts @@ -59,7 +59,6 @@ export function createSkillTool( origin: "skill", ownerSkillId: skillId, executionTarget: entry.execution_target as ExecutionTarget, - ownerSkillVersionHash: versionHash, input_schema: entry.input_schema as object, diff --git a/assistant/src/tools/types.ts b/assistant/src/tools/types.ts index 497e9afa402..0896b64d94e 100644 --- a/assistant/src/tools/types.ts +++ b/assistant/src/tools/types.ts @@ -352,6 +352,4 @@ export interface Tool extends LoadedTool { ownerMcpServerId?: string; /** If origin is 'plugin', the name of the owning plugin. */ ownerPluginId?: string; - /** Content-hash of the owning skill's source at registration time. */ - ownerSkillVersionHash?: string; } diff --git a/packages/skill-host-contracts/src/client.ts b/packages/skill-host-contracts/src/client.ts index 19a37ad92f2..95ca444f12e 100644 --- a/packages/skill-host-contracts/src/client.ts +++ b/packages/skill-host-contracts/src/client.ts @@ -1083,7 +1083,6 @@ export class SkillHostClient implements SkillHost { executionTarget: t.executionTarget, executionMode: t.executionMode ?? "proxy", ownerSkillId: t.ownerSkillId ?? this.options.skillId, - ownerSkillVersionHash: t.ownerSkillVersionHash, }; }); // Cache the provider so `skill.dispatch_tool` can resolve a tool diff --git a/packages/skill-host-contracts/src/tool-types.ts b/packages/skill-host-contracts/src/tool-types.ts index cef8349792d..88878e776b7 100644 --- a/packages/skill-host-contracts/src/tool-types.ts +++ b/packages/skill-host-contracts/src/tool-types.ts @@ -429,8 +429,6 @@ export interface Tool { ownerMcpServerId?: string; /** If origin is 'plugin', the name of the owning plugin. */ ownerPluginId?: string; - /** Content-hash of the owning skill's source at registration time. */ - ownerSkillVersionHash?: string; /** Declared execution target from the skill manifest. Used by resolveExecutionTarget * to accurately label lifecycle events for skill-provided tools. */ executionTarget?: ExecutionTarget; diff --git a/skills/catalog.json b/skills/catalog.json index 8512d780978..887ba509c9c 100644 --- a/skills/catalog.json +++ b/skills/catalog.json @@ -367,7 +367,7 @@ "display-name": "LLM Cost Optimizer" } }, - "updatedAt": "2026-05-26T13:03:52-05:00" + "updatedAt": "2026-05-26T14:11:54-04:00" }, { "id": "macos-automation", @@ -426,7 +426,7 @@ "feature-flag": "meet" } }, - "updatedAt": "2026-05-24T10:49:52-04:00" + "updatedAt": "2026-05-26T18:33:11Z" }, { "id": "meme-generator", diff --git a/skills/meet-join/__tests__/entrypoint.test.ts b/skills/meet-join/__tests__/entrypoint.test.ts index 3e638500a45..3cb710c8bff 100644 --- a/skills/meet-join/__tests__/entrypoint.test.ts +++ b/skills/meet-join/__tests__/entrypoint.test.ts @@ -75,8 +75,6 @@ const fakeTool = (name: string) => ({ category: "fake", defaultRiskLevel: "low", ownerSkillId: "meet-join", - ownerSkillBundled: true, - ownerSkillVersionHash: "test", executionTarget: "host" as const, executionMode: "proxy" as const, getDefinition: () => ({ name, description: "test fake", input_schema: {} }),