tools: drop ownerSkillBundled denormalization#32083
Merged
Merged
Conversation
Removes `ownerSkillBundled` from `Tool` everywhere. The field was a denormalized copy of `SkillSummary.bundled` carried for the permission checker's bundled-trust gating. Two consequences fell out of that: - A drift-check in `conversation-skill-tools.ts` had to re-register skill tools whenever a managed override flipped bundled state, even when the source hash was unchanged. - The meet-host manifest loader hardcoded `ownerSkillBundled: true` even though meet-join is not GA and is not actually bundled (the bundled-skills dir doesn't contain it). That was a latent bug granting bundled-skill auto-allow to a non-GA integration. The permission checker now derives bundled state at check time via `loadSkillCatalog().find(s => s.id === tool.ownerSkillId)?.bundled`, so the catalog is the single source of truth. The drift-check branch collapses, the meet-host hardcode is removed, and the field is dropped from IPC schema, IPC payload builder, the registry's plugin-clear path, and `packages/skill-host-contracts`. Tests: `checker.test.ts`'s bundled fixture now uses `app-builder` (a real bundled skill id) so catalog derivation returns true without any stamped field. The plugin-tool-contribution spoof case drops the `ownerSkillBundled` assertion. Five other fixtures drop the field from their mock factories. No behavioral change for in-tree skills (catalog derivation matches the previous stamped value). One intentional behavioral change: the meet-join skill no longer auto-allows as bundled — it now goes through the same permission gating as any non-bundled skill, which is correct given its pre-GA status.
vellum-apollo-bot
Bot
requested review from
awlevin,
noanflaherty and
siddseethepalli
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33767e0756
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+347
to
+351
| // Hash unchanged — filter to only tools that are actually registered | ||
| // for this skill. Some tools may have been skipped during initial | ||
| // registration due to core-name collisions — don't let them leak | ||
| // back in. Bundled-status drift no longer requires re-registration | ||
| // because the permission checker derives bundled state from the |
There was a problem hiding this comment.
This branch now treats prevHash === currentHash as a no-op and only filters already-registered tools, but a skill can switch from bundled to managed/plugin (or to a different directory) with identical file hash. In that case the registry keeps stale tool instances whose execute closures were built with the old skillDir/bundled values, so the override may keep running bundled-host behavior instead of the new source until content changes; this regresses source-of-truth and execution-target semantics for same-hash overrides.
Useful? React with 👍 / 👎.
dvargasfuertes
approved these changes
May 26, 2026
Comment on lines
+347
to
+351
| // Hash unchanged — filter to only tools that are actually registered | ||
| // for this skill. Some tools may have been skipped during initial | ||
| // registration due to core-name collisions — don't let them leak | ||
| // back in. Bundled-status drift no longer requires re-registration | ||
| // because the permission checker derives bundled state from the |
vellum-apollo-bot Bot
pushed a commit
that referenced
this pull request
May 26, 2026
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
pushed a commit
that referenced
this pull request
May 26, 2026
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.
dvargasfuertes
pushed a commit
that referenced
this pull request
May 26, 2026
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. Co-authored-by: vellum-apollo-bot[bot] <221541397+vellum-apollo-bot[bot]@users.noreply.github.com>
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
Removes
ownerSkillBundledfromTooleverywhere. The permission checker now derives bundled state fromloadSkillCatalog()at check time instead of consulting a stamped field on the tool.Why
The field was a denormalized copy of
SkillSummary.bundledcarried only so the permission checker could gate bundled-trust auto-allow without a lookup. Two consequences fell out of that:conversation-skill-tools.tshad to re-register skill tools whenever a managed override flipped bundled state, even when the source hash was unchanged. The check existed only to keep the denormalized field in sync.ownerSkillBundled: trueeven thoughmeet-joinis not GA and not actually bundled (the bundled-skills dir doesn't contain it). That granted bundled-skill auto-allow to a pre-GA integration — a latent bug, not a constraint to preserve.With the field deleted, the catalog is the single source of truth, the drift-check evaporates, and the meet-host bug goes away.
How
permissions/checker.ts— newisToolOwnerSkillBundled(tool)helper derives vialoadSkillCatalog().find(s => s.id === tool.ownerSkillId)?.bundled. Hot-path cost is comparable to the existing catalog lookups already done per permission check (hasInlineExpansions,computeTransitiveHashSafe).daemon/conversation-skill-tools.ts— drift-checkelsebranch collapses into a simple "filter to actually-registered tools" filter. Bundled-status drift no longer requires re-registration because the checker reads catalog truth directly.daemon/meet-manifest-loader.ts— incorrectownerSkillBundled: truehardcode removed.tools/types.ts(Tool),packages/skill-host-contracts/src/tool-types.ts, IPC schema +buildProxyToolinipc/skill-routes/registries.ts, IPC payload builder inpackages/skill-host-contracts/src/client.ts,registerPluginToolsclear logic intools/registry.ts, and skill-tool-factory's stamping (thebundledarg still flows to the script-runner closure for the bundled-import behavior — that's a separate concern).Tests
checker.test.ts— bundled-fixture switched fromgmail(not in bundled-skills/) toapp-builder(a real bundled-skills/ entry). With catalog derivation, the checker correctly reportsisSkillBundled = truewithout any stamped field. All 132 tests pass.plugin-tool-contribution.test.ts— spoofed-ownership case dropsownerSkillBundledfrom both the spoof and the assertion. 12/12 pass.meet-manifest-loader.test.ts— drops theownerSkillBundled === trueexpectation. 12/12 pass.createSkillToolsFromManifestmock returns:conversation-app-control-instantiation,skill-projection-feature-flag,conversation-skill-tools,skill-projection.benchmark.Behavioral change (intentional)
meet-joinno longer auto-allows as a bundled skill. It now goes through the same permission gating as any non-bundled skill, which is correct given its pre-GA status. In-tree skills underassistant/src/config/bundled-skills/continue to be treated as bundled (catalog derivation matches the previous stamped value).Series context
Part of the post-PR-A/PR-B field triage on
Tool. Three end-of-series cleanups still queued:LoadedToolorTool(keep one).ToolManifestin favor ofToolDefinition.ToolDefinitions instead ofTools.Next field after this one:
ownerSkillVersionHash.