You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Deferred review finding from PR #12531: attribute an MCP permission rule to its server from producer identity #13412
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
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.
packages/core/src/permissions/rule-parser.ts: Structural ask the review has repeated since round 19.hasAmbiguousMcpGrant'srawGrantbranch attributes a permission rule to an MCP server by re-splitting the flattened rule string — it derives aprefix/rawBoundary/otherBoundaryfrom themcp__<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 head7d5192bad16e(the original clause plus the gated mirror disjunct, pinned by theit.each([false, true])rows inpackages/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 —McpToolIdentityis threaded throughmatchesRule/matchesMcpPattern, andToolRegistry.getMcpToolIdentities()is whatPermissionManager.isMcpAllowAmbiguousalready 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. — commentTwo 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.packages/core/src/permissions/rule-parser.ts: R23-1. The exact (non-wildcard) arm ofhasAmbiguousMcpGrantdecides withspellings(other).includes(pattern)plus a bare-whole-server disjunct and never asks the server-head question, so a legacy exactallowwritten 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 baseranand headran: same asmain, 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 therc:4177751345row 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:36and its.zh-CN.mdtwin both state that "a legacy exact entry cannot grant tofoo:barwhilefoo_baris also registered", which the code does not do at head0a5e943bf1. 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. — commentpackages/core/src/permissions/permission-manager.ts: R26-1.isMcpAllowAmbiguoussources its ambiguity pool fromthis.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-frontmattermcpServersthe check is blind to agent-local competitors (P3:foo.bar/evilstays auto-approved whenfoo_barexists only in the agent's frontmatter) and refuses an agent-local owner (P4:foo_bar/evilloses its grant and is asked). Round 4 measured both in a real environment: P3 same asmain; 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