permissions: type ApprovalContext.toolOrigin as OwnerKind#32339
Merged
Conversation
Follow-up to PR #32294 review. The `toolOrigin` field on `ApprovalContext` was carrying a lossy projection ("skill" | "builtin") of the registry's `OwnerKind` ("skill" | "plugin" | "mcp"): - skill + plugin → projected to "skill" - mcp + core → projected to "builtin" The projection was implemented as an IIFE on the construction site in `checker.ts`, which obscured the underlying source of truth and forced every downstream consumer to reason about a custom union that doesn't match what the rest of the codebase uses. This PR matches the consumer's type to the source of truth: - `ApprovalContext.toolOrigin?: OwnerKind` — same union the registry exposes via `getToolOwner(name)?.kind`. - `checker.ts` drops the IIFE — `toolOrigin: getToolOwner(toolName)?.kind` is a one-liner that exposes the full ownership signal. - `approval-policy.ts` section 6 ("third-party skill tool") now spells out the disjunction the projection was hiding: `toolOrigin === "skill" || toolOrigin === "plugin"` Behavior preserved: skill and plugin tools are both treated as extension-class for approval; MCP and core tools fall through to the risk-based path. Section 8 ("bundled skill") stays as `toolOrigin === "skill"` — gated by `isSkillBundled` which is skill-only by construction. ## Test changes - `approval-policy.test.ts`: the one `toolOrigin: "builtin"` literal — testing "non-extension owner with manifest override falls through" — reframed to `toolOrigin: "mcp"`, which preserves the intent ("the third-party clause doesn't fire when toolOrigin is set to a non-extension kind") under the richer type. - Added a `toolOrigin: "plugin"` case that locks in the plugin → extension-owned routing the disjunction now expresses explicitly. Without it, a future refactor could silently drop the `|| "plugin"` arm and the existing skill-only tests wouldn't catch it. ## Verification - `bunx tsc --noEmit` clean across assistant. - `bunx eslint` clean on all three touched files. - `bun test src/permissions/approval-policy.test.ts src/__tests__/checker.test.ts` → 212 / 212 pass.
dvargasfuertes
approved these changes
May 27, 2026
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
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.
Follow-up to PR #32294 review. The
toolOriginfield onApprovalContextwas carrying a lossy projection ("skill" | "builtin")of the registry's
OwnerKind("skill" | "plugin" | "mcp"):The projection was implemented as an IIFE on the construction site in
checker.ts, which obscured the underlying source of truth and forcedevery downstream consumer to reason about a custom union that doesn't
match what the rest of the codebase uses.
This PR matches the consumer's type to the source of truth:
ApprovalContext.toolOrigin?: OwnerKind— same union the registryexposes via
getToolOwner(name)?.kind.checker.tsdrops the IIFE —toolOrigin: getToolOwner(toolName)?.kindis a one-liner that exposes the full ownership signal.
approval-policy.tssection 6 ("third-party skill tool") now spellsout the disjunction the projection was hiding:
toolOrigin === "skill" || toolOrigin === "plugin"Behavior preserved: skill and plugin tools are both treated as
extension-class for approval; MCP and core tools fall through to the
risk-based path. Section 8 ("bundled skill") stays as
toolOrigin === "skill"— gated byisSkillBundledwhich isskill-only by construction.
Test changes
approval-policy.test.ts: the onetoolOrigin: "builtin"literal —testing "non-extension owner with manifest override falls through" —
reframed to
toolOrigin: "mcp", which preserves the intent ("thethird-party clause doesn't fire when toolOrigin is set to a
non-extension kind") under the richer type.
toolOrigin: "plugin"case that locks in the plugin →extension-owned routing the disjunction now expresses explicitly.
Without it, a future refactor could silently drop the
|| "plugin"arm and the existing skill-only tests wouldn't catch it.
Verification
bunx tsc --noEmitclean across assistant.bunx eslintclean on all three touched files.bun test src/permissions/approval-policy.test.ts src/__tests__/checker.test.ts→ 212 / 212 pass.