tools: collapse owner{Skill,Plugin,Mcp}Id into OwnerInfo on Tool#32224
Conversation
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: 7e67779e5e
ℹ️ 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".
| owner: z.object({ | ||
| kind: z.literal("skill"), | ||
| id: z.string().min(1), | ||
| }), |
There was a problem hiding this comment.
When an existing workspace skill (or a stale skill child process during upgrade) is still using the previous @vellumai/skill-host-contracts client, its host.registries.register_tools frame contains ownerSkillId rather than the new owner object. This schema now rejects that frame before buildProxyTool can install any tools, so those skills silently lose their registered tools after the assistant updates. Since this is a daemon↔skill wire contract, keep a legacy ownerSkillId fallback and translate it to { kind: "skill", id } during the transition.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…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).
| // would leak into the global registry on socket close. Only skill-owned | ||
| // tools flow over IPC — plugin and MCP tools live in-process on the | ||
| // assistant side, so the schema is strict to kind: "skill". | ||
| owner: z.object({ | ||
| kind: z.literal("skill"), | ||
| id: z.string().min(1), | ||
| }), |
There was a problem hiding this comment.
We should be able to remove this from the manifest and look up the owner from the registry by tool name
There was a problem hiding this comment.
Done in f558a78. The per-tool owner field is gone from the IPC schema — ToolManifestSchema no longer accepts it, and registration now flows ownership in through a single top-level skillId: z.string().min(1) field on RegisterToolsParams:
const RegisterToolsParams = z.object({
skillId: z.string().min(1),
tools: z.array(ToolManifestSchema).min(1),
});One frame = one skill's batch, which matches how registerSkillTools(skillId, tools) works in-process and avoids letting a skill claim a different skill's name per-tool. Inside the handler, ownership is recorded by registerSkillTools(skillId, proxies) into the new registry-side ownersByName map; the per-connection ref-count mirroring collapses from a Set<string> loop into a single conn.addSkillToolsOwner(skillId). Lookups go through getToolOwner(name). Added a rejects missing skillId test on the IPC handler so the zod contract is exercised.
| /** Identifies the owning extension when origin is 'skill' / 'plugin' / 'mcp'. Absent for core tools. */ | ||
| owner?: OwnerInfo; |
There was a problem hiding this comment.
Same here. We should be able to remove this from the tool too and rely on the registry tracking tool ownership
There was a problem hiding this comment.
Done in f558a78. The owner field is removed from Tool entirely. Ownership now lives exclusively on the registry, keyed by tool name:
// In registry.ts
const ownersByName = new Map<string, OwnerInfo>();
export function getToolOwner(name: string): OwnerInfo | undefined {
return ownersByName.get(name);
}The map is populated only by the register*Tools functions (registerSkillTools(skillId, tools), registerPluginTools(pluginName, tools), registerMcpTools(serverId, tools), registerExternalTools(owner, toolsOrProvider), plus the bundled registerAppTools / registerSystemTools / registerUiSurfaceTools). The bare registerTool intentionally does not record ownership — core tools have no entry and getToolOwner returns undefined for them. This means a manifest can't claim ownership by forging a field on the literal; the only way to claim a tool is to go through a register* function, which stamps the owner from its arguments. (Updated the OwnerInfo doc-comment to point at getToolOwner.)
Fallout, all migrated to getToolOwner(name):
permissions/checker.ts→isToolOwnerSkillBundledtools/tool-approval-handler.ts→ load-hint resolutiondaemon/conversation-skill-tools.ts→ post-registration filter that rejects tools that lost ownership across re-registration
The createSkillTool / createSkillToolsFromManifest factories also dropped their skillId parameter — ownership is recorded by registerSkillTools(skillId, tools), not stamped on the Tool object the factory returns. Side benefit: tests can no longer spoof ownership through a tool literal, which closed a small attack surface in plugin-tool-contribution.test.ts (the spoofed owner field on the cast object is now genuinely inert; added a comment).
The ownerSkillId log-field key on the existing tool-registry log lines is preserved so log consumers don't break.
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.
vellum-apollo-bot
Bot
force-pushed
the
apollo/tool-owner-info
branch
from
7e67779 to
f558a78
Compare
Round 1 follow-up: ownership off the
|
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.
|
CI caught a real one. Pushed Failure: Cause: the file-local Fix: wire
|
…32294) Continuing the Tool field-by-field triage. With ownership now living in the registry's `ownersByName` map (PR #32224), `Tool.origin` is a strict subset of `getToolOwner(name)?.kind`: "skill" / "plugin" / "mcp" map 1:1 to owner kinds, and the absence of an owner is the canonical signal for core tools. Keeping the field added a second source of truth ownership consumers had to keep in sync. ## Architecture - Drop `origin?: "core" | "skill" | "mcp" | "plugin"` from both Tool interfaces (`assistant/src/tools/types.ts` and `packages/skill-host-contracts/src/tool-types.ts`). - All four production write sites no longer stamp the field: - `tools/mcp/mcp-tool-factory.ts` - `tools/skills/skill-tool-factory.ts` - `ipc/skill-routes/registries.ts` (IPC handler for skill projection) - `tools/registry.ts` `registerPluginTools` plugin stamp - `daemon/meet-manifest-loader.ts` (daemon-side projection) - Production read sites switch to `getToolOwner(t.name)?.kind`: - `tools/policy-context.ts` — skill/plugin branch - `permissions/checker.ts` — `toolOrigin` field plumbed to the gateway-side risk assessor (still "skill" / "builtin" externally) - `tasks/tool-sanitizer.ts` — filter out skill tools - Registry-internal reads use the `ownersByName` map directly (same module, no extra indirection): - "is core" check collapses from `existing.origin === "core" || !existing.origin` to `!ownersByName.has(tool.name)` across all 3 register*Tools paths - `getMcpToolDefinitions` / `getSkillToolNames` / `getAllToolDefinitions` / `coreToolsSnapshot` filters all switch to owner-kind lookups ## Test changes - Dropped `origin: "skill"` from 11 test fixtures across 11 files; the Tool literals now match the post-refactor interface. - Replaced 8 `tool.origin` / `retrieved?.origin` assertions with ownership-or-existence checks; in every case there's already a `getToolOwner(name)` assertion right next to it, so the read-side semantics are still covered. - Deleted two `skill-tool-factory.test.ts` tests that only verified the factory stamps origin — that responsibility is now entirely the registry's, covered by `registry.test.ts`. - Updated the spoofed-ownership case in `plugin-tool-contribution.test.ts` to reflect the new model: the Tool type no longer carries ownership at all, so forged `origin`/`owner` fields on the literal are inert extras the registry never reads. The bootstrap is the single source of truth. - Added `getToolOwner` to the `tool-executor-lifecycle-events.test.ts` registry mock, matching the pattern already in `tool-executor.test.ts`, so production reads through `policy-context.ts` keep resolving. - The `daemon/__tests__/meet-manifest-loader.test.ts` `origin` assertion is gone — the daemon-side projection no longer stamps the kind. The authoritative ownership stamp happens on the assistant side via `registerSkillTools(skillId, tools)` at the IPC boundary, covered separately by `ipc/skill-routes/__tests__/registries.test.ts`. ## Verification - `bunx tsc --noEmit` clean across assistant + packages/skill-host-contracts. - `bunx eslint` clean across all 23 touched files. - All touched test files pass individually: - registry.test.ts → 27/27 - skill-tool-factory.test.ts → 15/15 - plugin-tool-contribution.test.ts → 12/12 - checker.test.ts → 132/132 - tool-executor.test.ts → 80/80 - tool-executor-lifecycle-events.test.ts → 17/17 - conversation-skill-tools.test.ts → 62/62 - conversation-app-control-instantiation.test.ts → 6/6 - skill-projection-feature-flag.test.ts → 4/4 - computer-use-skill-manifest-regression.test.ts → 10/10 - daemon/meet-manifest-loader.test.ts + conversation-tool-setup-exclude + ipc/skill-routes/registries.test.ts → 34/34 - Full-folder `bun test src/__tests__/` hits the same pre-existing cross-test mock contamination on bare main (`getWorkspaceDirOverride` ESM link error) — not caused by this PR. Confirmed identical baseline by stash + re-run. ## Field landscape after this PR ``` ToolDefinition: description?, defaultRiskLevel?, input_schema?, executionTarget?, execute? LoadedTool = Required<ToolDefinition> & { name } Tool extends LoadedTool: category, executionMode? ``` Co-authored-by: vellum-apollo-bot[bot] <234526108+vellum-apollo-bot[bot]@users.noreply.github.com>
1 participant
Summary
Collapse
ownerSkillId/ownerPluginId/ownerMcpServerIdonToolinto a singleowner?: OwnerInfowhere:This is Field 5 in the Tool field-by-field triage that started with #32083 (
ownerSkillBundled) and continued through #32132 (ownerSkillVersionHash).Why
All three previous fields encoded the same shape — kind + id — and consumer code branched on
originto decide which one to read. Concrete examples:permissions/checker.tsandtool-approval-handler.tsonly consume the skill-owned variant — they readtool.ownerSkillIdafter gating onorigin === "skill".tools/registry.tsconflict-detection had three parallelexisting.ownerXId !== <thisId>branches.ipc/skill-routes/registries.tsdefinedownerSkillIdbecause plugin and MCP tools never cross the IPC boundary.A single discriminated field eliminates the duplication and makes future owner kinds (e.g. dynamic local tools) additive instead of widening the
Toolshape.What changed
Tool shape (
assistant/src/tools/types.ts, mirrored inpackages/skill-host-contracts/src/tool-types.ts):OwnerKindandOwnerInfotypes.ownerSkillId?/ownerPluginId?/ownerMcpServerId?withowner?: OwnerInfo.Stampers (
tools/skills/skill-tool-factory.ts,tools/mcp/mcp-tool-factory.ts,daemon/meet-manifest-loader.ts,packages/skill-host-contracts/src/client.ts):owner: { kind: <X>, id: <id> }where<X>is"skill","mcp", or"plugin".Consumers (
tools/registry.ts,permissions/checker.ts,tools/tool-approval-handler.ts,daemon/conversation-skill-tools.ts):tool.owner?.kind/ readtool.owner?.idinstead oftool.ownerXId.IPC contract (
ipc/skill-routes/registries.ts):ToolManifestSchema.ownerisz.object({ kind: z.literal("skill"), id: z.string().min(1) }). Strict tokind: "skill"since only skill-owned tools flow over IPC (plugin and MCP tools live in-process on the daemon side).Plugin spoof-defense (
tools/registry.ts:registerPluginTools):ownervia spread + explicit assignment. The test for this case (plugin-tool-contribution.test.ts) now constructs a spoofedowner: { kind: "skill", id: "some-other-skill" }and asserts the registry rewrites it to{ kind: "plugin", id: <plugin> }.What deliberately did not change
Tool-registered log lines in
registry.tscontinue to emit the old field names as structured log keys:The log key shape is decoupled from the Tool-type field shape so downstream log consumers that grep on
ownerSkillIdetc. keep working. We can rename these in a follow-up if/when downstream consumers move to the new shape.The local variable
ownerSkillIdintool-approval-handler.ts(line 371) was kept — it reads fromtool.owner?.kind === "skill" ? tool.owner.id : undefinedand remains semantically accurate.Test surface migrated
47 references across 11 test files migrated to the new shape:
__tests__/plugin-tool-contribution.test.ts(10)__tests__/conversation-skill-tools.test.ts(6)__tests__/checker.test.ts(6, already migrated in earlier in-progress edits)ipc/skill-routes/__tests__/registries.test.ts(4)__tests__/registry.test.ts(12)__tests__/skill-tool-factory.test.ts(3)__tests__/tool-executor.test.ts(3)__tests__/tool-executor-lifecycle-events.test.ts(3)__tests__/conversation-app-control-instantiation.test.ts(2)__tests__/skill-projection.benchmark.test.ts(2)__tests__/skill-projection-feature-flag.test.ts(2)__tests__/computer-use-skill-manifest-regression.test.ts(1)daemon/__tests__/meet-manifest-loader.test.ts(1)Verification
bunx tsc --noEmitclean (assistant + contracts)eslintclean across all touched filestool-approval-handler.test.ts,permissions/checker.test.ts,plugin-bootstrap.test.ts,skill-load-tool.test.ts,mcp-abort-signal.test.ts,daemon-skill-host.test.tsownerSkillIdetc. keys at runtime