Skip to content

Deferred review finding from PR #12531: attribute an MCP permission rule to its server from producer identity #13412

Description

@yiliang114

Verified review finding from PR #12531 whose fix lies outside that PR's footprint, deferred by the PR closeout loop for follow-up. Each rc: item links back to its original review comment. A maintainer or the PR author can turn any item into its own issue/PR and apply the ready-for-agent flow to that issue — nothing here is scheduled automatically.

  • rc:4177751345 packages/core/src/permissions/rule-parser.ts: Structural ask the review has repeated since round 19. hasAmbiguousMcpGrant's rawGrant branch attributes a permission rule to an MCP server by re-splitting the flattened rule string — it derives a prefix/rawBoundary/otherBoundary from the mcp__<server>__<tool> spelling and then compares server names and pure-underscore remainders. That enumerative guard is now correct for both orientations of the underscore flattening at head 7d5192bad16e (the original clause plus the gated mirror disjunct, pinned by the it.each([false, true]) rows in packages/core/src/permissions/mcp-grant-ambiguity.test.ts), so no live defect remains. What is deferred is the shape: attribute the rule to its server from producer identity instead of re-parsing the flattened name. The plumbing already exists — McpToolIdentity is threaded through matchesRule / matchesMcpPattern, and ToolRegistry.getMcpToolIdentities() is what PermissionManager.isMcpAllowAmbiguous already passes in — so the rewrite replaces string re-splitting with an identity lookup rather than adding a channel. Deferred because it is an architectural change to a shared permission path and PR fix(core): stop MCP server rules from authorizing a colliding server #12531 is at +2811/-107, 1.87x the closeout loop's 1500-addition ceiling after 25 substantive rounds. — comment

Two rows were added on 2026-10-06 after @wenshao's round-4 maintainer verification ruled R23-1 and R26-1 real, non-blocking against main, and reasonable to track here.

  • rc:4186114278 packages/core/src/permissions/rule-parser.ts: R23-1. The exact (non-wildcard) arm of hasAmbiguousMcpGrant decides with spellings(other).includes(pattern) plus a bare-whole-server disjunct and never asks the server-head question, so a legacy exact allow written in one server's spelling still grants a tool of a differently-registered server whose spelling collides with it. @wenshao's round-4 real-environment differential (row E1) records base ran and head ran: same as main, not a regression, so it does not block PR fix(core): stop MCP server rules from authorizing a colliding server #12531. Deferred are both halves of the fix: the producer-identity rewrite in the rc:4177751345 row above, which is the only formulation that also closes this arm, and the still-open doc/code mismatch — docs/design/mcp-tool-name-provider-compatibility.md:36 and its .zh-CN.md twin both state that "a legacy exact entry cannot grant to foo:bar while foo_bar is also registered", which the code does not do at head 0a5e943bf1. Round 4 offered "narrow the doc sentence, or track it in Deferred review finding from PR #12531: attribute an MCP permission rule to its server from producer identity #13412"; this row is the second option. — comment
  • rc:4189563018 packages/core/src/permissions/permission-manager.ts: R26-1. isMcpAllowAmbiguous sources its ambiguity pool from this.config.getToolRegistry() — the PermissionManager's own, i.e. session, registry — while the identity being decided can come from a different registry accessor, so for a subagent with agent-frontmatter mcpServers the check is blind to agent-local competitors (P3: foo.bar/evil stays auto-approved when foo_bar exists only in the agent's frontmatter) and refuses an agent-local owner (P4: foo_bar/evil loses its grant and is asked). Round 4 measured both in a real environment: P3 same as main; P4 new at head but fail-closed — it costs a prompt and never admits a foreign tool. Neither blocks PR fix(core): stop MCP server rules from authorizing a colliding server #12531. Deferred because the fix is the shape decision round 4 assigns to this issue: which registry owns the ambiguity check. — comment

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    category/coreCore engine and logicpriority/P3Low - Minor, cosmetic, nice-to-fix issuesscope/mcpModel Context Protocolstatus/blockedBlocked by external dependencytype/enhancementNon-bug improvement or optimization

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions