tools: drop ownerSkillVersionHash denormalization#32132
Merged
Conversation
vellum-apollo-bot
Bot
requested review from
awlevin and
siddseethepalli
as code owners
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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.
vellum-apollo-bot
Bot
force-pushed
the
apollo/drop-owner-skill-version-hash
branch
from
b70b7c6 to
6bedf67
Compare
dvargasfuertes
approved these changes
May 26, 2026
vellum-apollo-bot Bot
added a commit
that referenced
this pull request
May 27, 2026
…ic OwnerInfo
Collapses three parallel ownership fields on Tool into a single
`owner?: OwnerInfo` where OwnerInfo is `{ kind: 'skill' | 'plugin' | 'mcp', id: string }`.
Why:
- Three fields encoded the same shape (kind + id) and the consumer code
branched on origin to pick which to read. One field with discriminated
kind makes that branching unnecessary.
- New owner kinds can be added without growing the Tool type.
- Same shape used end-to-end (registry, IPC manifest, contracts).
Behavior preserved:
- Plugin spoof-defense in registerPluginTools still overwrites owner via
spread + explicit assignment. Test updated to spoof the new shape.
- Skill/plugin/MCP conflict-detection paths now read `existing.owner?.kind`
and `existing.owner?.id` instead of `existing.ownerSkillId` etc.
- IPC ToolManifestSchema in skill-routes/registries.ts now takes
`owner: { kind: 'skill', id: string }` (strict to kind: 'skill' since
only skill-owned tools flow over IPC; plugin and MCP tools live in-process).
Log fields kept as-is:
- Tool-registered log lines in registry.ts still emit `ownerSkillId`,
`ownerPluginId`, `ownerMcpServerId` keys so downstream log consumers
that grep on those names continue to work. The structured log key is
intentionally decoupled from the Tool-type field shape.
Test surface migrated: 11 unit + IPC test files (47 references).
All targeted tests green, typecheck clean, lint clean.
Field 5 in the Tool field-by-field triage (following PR #32083, #32132).
vellum-apollo-bot Bot
added a commit
that referenced
this pull request
May 27, 2026
…ic OwnerInfo
Collapses three parallel ownership fields on Tool into a single
`owner?: OwnerInfo` where OwnerInfo is `{ kind: 'skill' | 'plugin' | 'mcp', id: string }`.
Why:
- Three fields encoded the same shape (kind + id) and the consumer code
branched on origin to pick which to read. One field with discriminated
kind makes that branching unnecessary.
- New owner kinds can be added without growing the Tool type.
- Same shape used end-to-end (registry, IPC manifest, contracts).
Behavior preserved:
- Plugin spoof-defense in registerPluginTools still overwrites owner via
spread + explicit assignment. Test updated to spoof the new shape.
- Skill/plugin/MCP conflict-detection paths now read `existing.owner?.kind`
and `existing.owner?.id` instead of `existing.ownerSkillId` etc.
- IPC ToolManifestSchema in skill-routes/registries.ts now takes
`owner: { kind: 'skill', id: string }` (strict to kind: 'skill' since
only skill-owned tools flow over IPC; plugin and MCP tools live in-process).
Log fields kept as-is:
- Tool-registered log lines in registry.ts still emit `ownerSkillId`,
`ownerPluginId`, `ownerMcpServerId` keys so downstream log consumers
that grep on those names continue to work. The structured log key is
intentionally decoupled from the Tool-type field shape.
Test surface migrated: 11 unit + IPC test files (47 references).
All targeted tests green, typecheck clean, lint clean.
Field 5 in the Tool field-by-field triage (following PR #32083, #32132).
dvargasfuertes
pushed a commit
that referenced
this pull request
May 27, 2026
) * tools: replace ownerSkillId/ownerPluginId/ownerMcpServerId with generic OwnerInfo Collapses three parallel ownership fields on Tool into a single `owner?: OwnerInfo` where OwnerInfo is `{ kind: 'skill' | 'plugin' | 'mcp', id: string }`. Why: - Three fields encoded the same shape (kind + id) and the consumer code branched on origin to pick which to read. One field with discriminated kind makes that branching unnecessary. - New owner kinds can be added without growing the Tool type. - Same shape used end-to-end (registry, IPC manifest, contracts). Behavior preserved: - Plugin spoof-defense in registerPluginTools still overwrites owner via spread + explicit assignment. Test updated to spoof the new shape. - Skill/plugin/MCP conflict-detection paths now read `existing.owner?.kind` and `existing.owner?.id` instead of `existing.ownerSkillId` etc. - IPC ToolManifestSchema in skill-routes/registries.ts now takes `owner: { kind: 'skill', id: string }` (strict to kind: 'skill' since only skill-owned tools flow over IPC; plugin and MCP tools live in-process). Log fields kept as-is: - Tool-registered log lines in registry.ts still emit `ownerSkillId`, `ownerPluginId`, `ownerMcpServerId` keys so downstream log consumers that grep on those names continue to work. The structured log key is intentionally decoupled from the Tool-type field shape. Test surface migrated: 11 unit + IPC test files (47 references). All targeted tests green, typecheck clean, lint clean. Field 5 in the Tool field-by-field triage (following PR #32083, #32132). * tools: move ownership from Tool to registry-side ownersByName map Addresses the round-1 review on PR #32224. The Tool object no longer carries an owner field — ownership lives exclusively on the registry, keyed by tool name, and is read through a new getToolOwner(name) accessor. Callers that need to know who owns a tool now go through the registry instead of trusting a manifest-supplied field. ## Architecture - New ownersByName: Map<string, OwnerInfo> in registry.ts populated exclusively by the register*Tools functions. The only way to claim a tool is to go through a register* function, so callers cannot spoof ownership by forging a field on the Tool literal. - New exported getToolOwner(name): OwnerInfo | undefined as the single read accessor. Core tools have no entry — returns undefined. - registerTool (bare install) intentionally does NOT record ownership. Only registerSkillTools / registerPluginTools / registerMcpTools / registerExternalTools / registerAppTools / registerSystemTools / registerUiSurfaceTools populate ownersByName, each stamping owner from their argument list. ## Production-side changes - tools/types.ts + packages/skill-host-contracts/src/tool-types.ts: dropped owner?: OwnerInfo from the Tool type; OwnerInfo docstring points at getToolOwner. - registry.ts: register*/unregister*/externalToolProviders all updated. registerExternalTools(owner, toolsOrProvider) — owner is now a required first argument. externalToolProviders entries are {owner, provider} tuples; initializeTools propagates owner into ownersByName at provider-resolution time. - IPC: register_tools params now carry skillId at the top level alongside tools (one frame = one skill's batch). The per-tool manifest schema no longer carries an owner field. The daemon-side handler calls registerSkillTools(skillId, proxies). Connection ref-count mirroring simplifies to a single conn.addSkillToolsOwner(skillId). - skill-host-contracts client: sends {skillId, tools: manifests}. - daemon-skill-host adapter wraps the single-arg facet method, deriving owner from the skillId closure. - skill-tool-factory: dropped the skillId parameter from createSkillTool / createSkillToolsFromManifest — ownership is now recorded by registerSkillTools, not stamped on the Tool object. Updated all call sites (conversation-skill-tools, meet-manifest-loader). - Non-registry consumers now read ownership via getToolOwner(name): permissions/checker (isToolOwnerSkillBundled), tools/tool-approval-handler (load-hint logic), daemon/conversation-skill-tools (post-registration filter). - providers-setup, mcp-reload-service: registerMcpTools(serverId, tools). ## Logging Preserved the ownerSkillId log field key on the existing log calls so log consumers don't break. The field still resolves to the same string the registry now stores in ownersByName. ## Tests - registry.test.ts: imported getToolOwner; simplified makeSkillTool signature (no owner stamp); rewrote 'origin metadata' / 'dynamic skill tool registry' / 'skill tool reference counting' blocks. - Test mocks for skill-tool-factory and registry updated to match the new signatures across conversation-skill-tools.test.ts, conversation-app-control-instantiation.test.ts, skill-projection-feature-flag.test.ts. Each mock derives ownership from its own mockRegisteredTools keying (since the real registry derives it from the register*Tools argument). - plugin-tool-contribution.test.ts: ownership assertions go through getToolOwner(name) instead of reading retrieved?.owner. - registries.test.ts (IPC): handler params now include skillId at the top level; added a 'rejects missing skillId' test that asserts the new zod schema enforces it. - meet-manifest-loader.test.ts: mock signature mirrors registerExternalTools(owner, provider); one test now asserts the captured owner is {kind: 'skill', id: 'meet-join'}. - checker.test.ts: mock skill tools registered via registerSkillTools(skillId, [tool]) so isToolOwnerSkillBundled resolves through the registry. ## Verification - bunx tsc --noEmit: clean across assistant + skill-host-contracts. - All touched test files pass when run individually (Bun's mock.module is global so cross-file runs hit pre-existing contamination, identical 29 fails on main vs this branch). - Linted all touched files; clean. * tools: wire getToolOwner through tool-executor.test.ts registry mock CI surfaced a single failure on PR #32224 — `ToolExecutor allowedToolNames gating > inactive skill tool names the owning skill in the load hint`. Root cause: the file-local `mock.module("../tools/registry.js", ...)` only stubbed `getTool` and `getAllTools`, so production's new `getToolOwner(name)` import resolved to `undefined`, dropping the load-hint owner branch and producing the generic "Load the skill that provides this tool first." message. Wire the mock's `getToolOwner` to surface the optional `owner` field from the override-produced tool. The three tests that already encode `owner: { kind: "skill", id: ... }` inline keep working unchanged; the failing test now resolves the owner the same way production does. Verified: bun test src/__tests__/tool-executor.test.ts → 80/80 pass. bunx tsc --noEmit clean. eslint clean. --------- Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com> Co-authored-by: vellum-apollo-bot[bot] <209693332+vellum-apollo-bot[bot]@users.noreply.github.com> Co-authored-by: vellum-apollo-bot[bot] <234526108+vellum-apollo-bot[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Continuing the
Toolfield-by-field triage. DropsownerSkillVersionHashfromTool— including the assistant-side type, contracts-side type, the IPC schema + relay, the IPC client payload builder,skill-tool-factory,meet-manifest-loader, and the registry's plugin-clear.Why
The field had zero production consumers. The tamper-detection security mechanism that matters — blocking execution when a skill's source changed since approval — is the
expectedSkillVersionHashoption captured into each skill tool'sexecute()closure, checked insiderunSkillToolScript/runSkillToolScriptSandbox. Nothing reads the field off theToolobject at runtime:executor.tsdoes not reference it.buildProxyToolcarries the hash onto the daemon-sideTool, but the supervisor short-circuit replaces that proxy's execute closure with the manifest's, and nothing reads the field on the daemon side either.So the field is pure write-only denormalization on top of an already-correct closure-based mechanism.
Side cleanup
In
meet-manifest-loader.ts, the field's removal leftbuildProxyTool'smanifestHashparam dead. Dropped the param and updated the single callsite.Test changes
"version hash plumbing to projected tools"suite inconversation-skill-tools.test.ts. The suite's comment block claimedexecutor.tsuses the field to build policy context — stale;executor.tshas no reference to the field. Re-aiming the assertions would only verify 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 inskill-script-runner-host.test.ts(5+ cases) andskill-script-runner.test.tsassertingexpectedSkillVersionHashdirectly.skill-tool-factory.test.tstests that asserted only the field ("sets ownerSkillVersionHash from versionHash","passes versionHash through to all created tools")."execute() works correctly when versionHash is provided"since it exercises the real closure → runner integrity check; trimmed the stale field assertion at the tail.plugin-tool-contribution.test.ts.skills/meet-join/__tests__/entrypoint.test.ts'sfakeToolhelper also still carriedownerSkillBundled: true— a leftover from PR tools: drop ownerSkillBundled denormalization #32083 that the priorassistant packagesgrep scope missed. Cleaned both fields here.Verification
bun run typecheckclean (assistant + contracts package).bun run lintclean.skill-script-runner.test.tsandskill-script-runner-host.test.tsas a smoke check for the real tamper path.mock.modulepollution causes batch failures (5 specific test files combined →API_KEY_PROVIDERSSyntaxError, plus a drizzle-orm hoist artifact). Confirmed identical batch result on cleanmainviagit stashbaseline — not caused by this PR.Field landscape after this PR
Three fields down (
executionTarget,ownerSkillBundled,ownerSkillVersionHash), with the broader triage now turning to the remaining owner fields andorigin/executionMode/category.