feat(core): add Tool(param:value) permission syntax for parameter-level access control#6106
Merged
Merged
Conversation
…el access control Introduces key:value parameter matching in permission rules, allowing users to grant or deny tool access based on specific input parameters. - Parse key:value pairs from specifiers for literal-kind rules - Support wildcard patterns (*), multiple params, and mixed syntax - Thread toolParams through PermissionCheckContext and matchesRule - Add 11 unit tests covering parsing, matching, wildcards, and edge cases - Maintain backward compatibility with existing specifier kinds Example rules: Agent(model:opus) # deny agents using Opus model Agent(coder,model:*) # deny coder-type agents with any model Bash(git:*) # still works (legacy :* → git *) Closes #6100
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: this is a feature (parity with Claude Code's Direction: solid alignment. Claude Code's CHANGELOG explicitly lists Approach: the scope grew since last review (now 6 files, +912/-52), but the bulk of additions are tests (~520 lines in Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个功能特性（与 Claude Code 的 方向：与项目方向高度对齐。Claude Code 的 CHANGELOG 明确列出了 方案：范围较上次审查有所增长（现在 6 个文件，+912/-52），但新增的大部分是测试（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal before reading the diff: extend Findings (updated for current diff):
No correctness bugs, security issues, or AGENTS.md violations found. Test ResultsUnit tests: 280/280 passed (including 22 new tests covering param parsing, wildcard matching, mixed specifier+params, legacy useStatusLine tests: 68/68 passed. TypeScript: clean compile, no errors. CI: all checks green — Integration check — direct param matching against the live source via 中文说明代码审查阅读 diff 前的独立方案：在 发现（针对当前 diff 更新）：
未发现正确性 bug、安全问题或违反 AGENTS.md 规范的情况。 测试结果单元测试： 280/280 通过（包括 22 个新测试）。 — Qwen Code · qwen3.7-max |
Collaborator
ReflectionRe-reviewing after the PR grew from +340/-48 to +912/-52. The growth is almost entirely tests — 22 new tests now (up from 11), covering edge cases that the earlier review and community reviewers flagged: ReDoS safety, type guards (boolean/null/undefined/object rejection), number coercion, My independent proposal before reading the diff was the same design. The PR's implementation matches or exceeds it. The only surprise in the original review — The motivation remains genuine: Claude Code ships this syntax, the linked issue (#6100) exists, and the use cases (cost control, security policies) are real. The scope is tight — 6 files, all related to the permission system, no drive-by refactors. The All 280 permission tests pass, all 68 useStatusLine tests pass, typecheck is clean, CI is green, and the integration check confirms the feature works end-to-end. The earlier CI failure ( If I had to maintain this in six months, I'd thank the author for: the clean separation between specifier matching and param matching in Verdict: Approve. Ships the feature cleanly, tests prove it works, CI is green. ✅ 中文说明反思在 PR 从 +340/-48 增长到 +912/-52 后重新审查。增长几乎全部来自测试——现在有 22 个新测试（从 11 个增加），覆盖了早期审查和社区审查者提出的边缘场景：ReDoS 安全性、类型守卫（boolean/null/undefined/object 拒绝）、数字强制转换、 阅读 diff 前我的独立方案相同。PR 的实现与我的方案一致甚至更好。原始审查中唯一的意外——复用 动机仍然是真实的：Claude Code 已有此语法，关联的 issue（#6100）存在，用例（成本控制、安全策略）是真实的。范围紧凑——6 个文件，全部与权限系统相关，无顺手重构。 所有 280 个权限测试通过，68 个 useStatusLine 测试通过，类型检查干净，CI 全绿，集成检查确认功能端到端工作。早期的 CI 失败（ 如果我六个月后要维护这段代码，我会感谢作者： 结论：批准。 功能干净地交付，测试证明它有效，CI 全绿。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
added
category/core
qwen-code-ci-bot
requested changes
Jul 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] buildPermissionRules does not propagate toolParamMatchers for the "Always Allow" flow (rule-parser.ts ~line 489). When a user selects "Always Allow" for a tool matched by a param-level rule (e.g., Agent(model:opus)), the literal case uses only ctx.specifier (which is undefined after param extraction) and generates a bare Agent rule — matching ALL agent invocations regardless of model. This is a security escalation.
case 'literal':
default: {
const parts: string[] = [];
if (ctx.specifier) parts.push(ctx.specifier);
if (ctx.toolParams) {
for (const [k, v] of Object.entries(ctx.toolParams)) {
if (typeof v === 'string') parts.push(`${k}:${v}`);
}
}
if (parts.length > 0) return [`${displayName}(${parts.join(',')})`];
return [displayName];
}
— qwen3.7-max via Qwen Code /review
[Critical] MCP tool rules with param matchers silently ignore the matchers. matchesRule returns early via matchesMcpPattern for MCP tools before reaching the param-matching block (rule-parser.ts ~line 1049). The parser accepts key:value syntax for MCP tools (they're literal-kind), but the matcher discards param constraints. A rule like mcp__chrome__navigate(server_name:chrome) matches every invocation regardless of params.
[Critical] All 11 new tests exercise only parseRule and matchesRule directly (permission-manager.test.ts). Zero integration tests verify that toolParams flows through the 4 modified PermissionManager methods: evaluateSingle, findMatchingDenyRule, hasRelevantRules, and hasMatchingAskRule. Each builds a 7-element positional matchArgs tuple — a misordering would silently produce wrong permission decisions with no test to catch it.
[Suggestion] Several edge cases in the new parsing/matching code lack test coverage (permission-manager.test.ts): null param values, non-string coercion, colons in URL-like values, partial wildcards, and MCP tool rules with param matchers.
…mission syntax - buildPermissionRules now propagates toolParamMatchers for 'Always Allow' flow - MCP tool rules now check param matchers after name matching - Added Object.hasOwn check to prevent prototype chain lookup vulnerability - Replaced matchesCommandPattern with matchesParamValuePattern for param value matching - Added diagnostic logging for param matching failures - Added warnings for empty valuePattern, invalid keys, and non-literal key:value syntax - Added type checking for non-primitive param values
DragonnZhang
reviewed
Jul 1, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Independent Review (second pass)
All critical findings from the prior review round have been resolved by commit 8e59413:
| Prior Comment | Status | Evidence |
|---|---|---|
matchesCommandPattern reused for param values (line 1241) |
Fixed | Dedicated matchesParamValuePattern() at line 801 with proper regex escaping |
Prototype chain lookup without Object.hasOwn (line 1223) |
Fixed | Object.hasOwn(toolParams, matcher.key) at line 1218 |
| No diagnostic logging (line 1209) | Fixed | debugLogger.debug() at lines 1219, 1225, 1231, 1238 |
Empty valuePattern edge case (line 326) |
Fixed | debugLogger.warn() at lines 314-317 |
No warning for non-literal key:value (line 305) |
Fixed | debugLogger.warn() at lines 340-343 |
String(actualValue) coercion (line 1236) |
Fixed | Type guard for string/number at line 1229 before coercion |
Remaining suggestions (non-blocking)
- 8 positional parameters in
matchesRule(line 1088): Consider refactoring to an options object. Not blocking but would improve readability and reduce risk of argument-order bugs in callers.
Security assessment
The deny-first evaluation order in evaluateSingle ensures that param-level deny rules cannot be bypassed by "Always Allow" (which creates allow rules via buildPermissionRules). The matchesParamValuePattern regex construction properly escapes special characters. The legacy :* → * conversion is correctly scoped to specifierKind === 'command' only, preventing interference with the new key:value syntax.
No high-confidence correctness or security issues found in the permission-specific changes.
qwen-code-ci-bot
requested changes
Jul 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] Missing test coverage for two important code paths:
-
buildPermissionRuleswithtoolParams— The new literal/default case that serializesctx.toolParamsinto rule strings has no tests. This is the code path that generates "Always Allow" rules. Without tests, the unfiltered serialization bug (see inline comment on line 508) shipped undetected. -
MCP param matching branch — All 11 new param matcher tests use
Agent(a standard literal tool). The ~45 lines of MCP-specific param matching logic (lines 1108-1144 inmatchesRule) are entirely untested. MCP tools are a primary use case for theTool(param:value)syntax.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
requested changes
Jul 1, 2026
|
|
||
| // ── Standard tool name matching (with meta-category support) ───────── | ||
| if (!toolMatchesRuleToolName(rule.toolName, canonicalCtxToolName)) { | ||
| return false; |
Collaborator
There was a problem hiding this comment.
[Critical] MCP branch ignores rule.specifier when param matchers are present. After matchesMcpPattern succeeds and param matchers pass, the code returns true without ever checking rule.specifier. A rule like mcp__server__tool(safe_mode,level:basic) matches ANY invocation of mcp__server__tool where level=basic, completely ignoring the safe_mode specifier — a permission escalation via silently weakened allow rules.
The standard (non-MCP) branch correctly checks specifier before param matchers (line 1153+). The MCP branch should do the same:
Suggested change
| return false; | |
| } | |
| // Also check specifier if present (same as standard branch) | |
| if (rule.specifier && specifier !== rule.specifier) { | |
| return false; | |
| } | |
| return true; | |
| } |
— qwen3.7-max via Qwen Code /review
wenshao
requested changes
Jul 1, 2026
| return false; | ||
| } | ||
| // MCP tools matched by name; now check param matchers if present | ||
| if (rule.toolParamMatchers?.length) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The param-matching logic here (~35 lines: Object.hasOwn, null/undefined guard, type guard for string | number, matchesParamValuePattern call, debug logging) is duplicated verbatim in the standard tool path at line ~1209. Any future fix (e.g., adding boolean coercion, changing the type guard) must be applied in both places or they will diverge silently.
Consider extracting a shared helper:
function matchesToolParamMatchers(
rule: PermissionRule,
toolParams: Record<string, unknown> | undefined,
): boolean { ... }Additionally, this entire MCP param-matcher code path has no test coverage — all 11 new tests use Agent (literal specifier kind) and never enter this branch.
— qwen3.7-max via Qwen Code /review
| ); | ||
| return false; | ||
| } | ||
| if (typeof actualValue !== 'string' && typeof actualValue !== 'number') { |
Collaborator
There was a problem hiding this comment.
[Suggestion] When a tool param value is not string or number (e.g., boolean true, array, object), the matcher returns false with only a debug-level log. For a deny rule like Agent(restricted:true), if the tool passes restricted: true (a boolean, not the string "true"), the deny rule silently fails to match and the tool proceeds unblocked.
Consider coercing booleans to string (String(actualValue) produces "true"/"false"), or at minimum document this type limitation prominently in the toolParamMatchers type docs so rule authors know boolean/object/array params cannot be matched.
— qwen3.7-max via Qwen Code /review
| } | ||
| matchers.push({ key, valuePattern }); | ||
| continue; | ||
| } else if (key && !/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(key)) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] Two minor issues here:
-
The key validation regex
/^[a-zA-Z_][a-zA-Z0-9_]*$/rejects hyphens. Common MCP tool param names likeserver-nameormax-retrieswill silently fail to parse as key:value matchers. Consider allowing hyphens:/^[a-zA-Z_][a-zA-Z0-9_-]*$/. -
The
else ifre-tests the negated regex, which is redundant — in theelsebranch, ifkeyis truthy, the regex must have failed. Simplify toelse if (key).
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
requested changes
Jul 1, 2026
…ission syntax - Add 's' flag to RegExp in matchesParamValuePattern for multiline support - Filter buildPermissionRules to stable param keys only (model, subagent_type, skill, server_name) to prevent sensitive data leakage - Reject MCP rules with unsupported specifiers instead of silently ignoring - Fix :* wildcard conversion to use global replace for backward compatibility - Extract shared evaluateParamMatchers helper to deduplicate MCP and standard branches
wenshao
requested changes
Jul 2, 2026
| // already encodes server + tool identity. If a specifier was somehow | ||
| // parsed (e.g. user wrote `mcp__srv__tool(XXX)`), reject the match | ||
| // rather than silently ignoring the constraint. | ||
| if (rule.specifier) { |
Collaborator
There was a problem hiding this comment.
[Critical] MCP "Always Allow" regression: buildPermissionRules literal/default case (line ~510) generates MCP rules WITH a specifier (from server_name), e.g. mcp__chrome__navigate(chrome). But this new guard rejects any MCP rule carrying a specifier. The result: every "Always Allow" click on an MCP tool produces a rule that immediately becomes a dead entry in settings.json — parseRule sets specifier = "chrome", then matchesRule sees it truthy and returns false.
Pre-existing MCP "Always Allow" rules in users' settings.json also break silently on upgrade (they worked before because the old MCP branch ignored specifiers).
Fix: In buildPermissionRules, emit bare [displayName] for MCP tools (no specifier, no stable params) since the tool name already encodes server identity:
Suggested change
| if (rule.specifier) { | |
| // MCP tool names already encode server + tool identity. | |
| // Don't add specifiers — matchesRule rejects MCP rules with specifiers. | |
| if (canonicalName.startsWith('mcp__')) { | |
| return [displayName]; | |
| } |
For backward compat with existing persisted rules, consider softening this guard to ignore (not reject) specifiers that match the server_name extracted from the tool name.
— qwen3.7-max via Qwen Code /review
| expect(matchesRule(rule, 'mcp__chrome__navigate')).toBe(false); | ||
| expect(matchesRule(rule, 'mcp__other__use_browser')).toBe(false); | ||
| }); | ||
|
|
Collaborator
There was a problem hiding this comment.
[Suggestion] Missing test coverage for the MCP + toolParamMatchers combination in matchesRule. The MCP branch (rule-parser.ts ~lines 1155-1183) was rewritten with three new sub-paths: specifier rejection, param matcher evaluation, and their combination — but no test exercises any of them through matchesRule.
Recommended additions:
parseRule('mcp__server__tool(model:opus)')→matchesRulewith{model: 'opus'}should betrue- Same rule with
{model: 'sonnet'}should befalse - Same rule with no
toolParamsshould befalse parseRule('mcp__server__tool(feature_x)')(bare specifier) →matchesRuleshould befalse(specifier rejection)
— qwen3.7-max via Qwen Code /review
Collaborator
Review Summary — PR #6106Verdict: COMMENT (CI failing — not approving until green) What this PR doesIntroduces CI StatusBuild fails with Fix RequiredRemove the Design AssessmentThe
Test CoverageThe new tests cover parseRule extraction, multi-param matching, wildcard patterns, mixed specifier+params, and legacy compatibility. Existing prior review comments already flag some edge cases (MCP backward compat, ReDoS risk) that remain worth addressing. — qwen3-coder via Qwen Code /review |
DragonnZhang
reviewed
Jul 2, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
CI still failing — the unused @ts-expect-error at gitWorktreeService.ts:1561 from the previous review cycle remains unresolved in commit b026ffa.
The directive suppresses a type error for allowUnsafeHooksPath that does not exist in the CI environment (simple-git's type definitions already include it). Remove the two-line comment to unblock the build:
const worktreeGit = simpleGit(worktreePath, {
- // @ts-expect-error — allowUnsafeHooksPath is supported at runtime but
- // not yet declared in simple-git@3.28.0 type definitions.
unsafe: { allowUnsafeHooksPath: true },
});The core Tool(param:value) permission syntax implementation looks solid — parsing, matching, and test coverage are well-structured. Once the build is green this should be ready to merge.
— qwen3-coder via Qwen Code /review
…lue) syntax - Remove unused @ts-expect-error in gitWorktreeService.ts (CI blocker) - Fix ReDoS in matchesParamValuePattern: replace regex with linear-time glob matcher using indexOf, avoiding catastrophic backtracking on multi-wildcard patterns like *a*a*a*a*b - Fix MCP backward compatibility: exclude MCP tools from key:value parsing in parseRule and buildPermissionRules to preserve existing MCP deny rule semantics - Add tests for MCP + param matcher, partial wildcards, ReDoS prevention, number coercion, and buildPermissionRules with toolParams (stable params, volatile params, sensitive data, round-trip)
DennisYu07
force-pushed
the
feat/tool-param-permission-syntax
branch
from
0adb27c to
b8f8cfb
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
DennisYu07
force-pushed
the
feat/tool-param-permission-syntax
branch
from
b8f8cfb to
0adb27c
Compare
wenshao
reviewed
Jul 5, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
permission-manager.test.ts |
Missing test for MCP specifier rejection: no test calls matchesRule with an MCP rule carrying a specifier to verify the new if (rule.specifier) return false branch |
Add matchesRule(parseRule('mcp__server__tool(server_name:myserver)'), 'mcp__server__tool') → expect false; and a counterpart verifying bare MCP rules still match |
scripts/tests/qwen-autofix-workflow.test.js |
Missing test for address-review zero-output failure: the anyOutput path where qwen exits 0 but writes neither address-summary.md nor no-action.md is untested |
Add a test with a stub that writes nothing and verify status !== 0 and stderr contains address-summary.md |
permission-manager.test.ts |
Missing test for invalid-key fallback: parseRule('Agent(my-key:value)') with a hyphenated key falls through to plain specifier, but no test verifies this |
Add test verifying toolParamMatchers is undefined and specifier is 'my-key:value' |
.github/workflows/qwen-autofix.yml (review-address report step ~line 1370) |
Agent-written files dumped to GITHUB_STEP_SUMMARY without code-fence wrapping, unlike the issue-autofix report step which uses code fences. Agent markdown renders as formatted text |
Wrap with echo '\``'/cat/echo '```'` consistent with issue-autofix step |
.github/workflows/qwen-autofix.yml (Publish PR ~line 779, Push and report ~line 1328) |
--force-with-lease removed from git push, making autofix branches fragile against external pushes to the predictable autofix/issue-NNNN branch name |
Restore git push --force-with-lease origin "${BRANCH}" or add a comment explaining why plain push is preferred |
.github/workflows/qwen-autofix.yml (Decide phases ~line 191) |
Summary line prints raw ISSUE_NUMBER from event payload but routing uses sanitized ROUTE_ISSUE. When sanitize_number rejects a value, the summary is misleading |
Print sanitized values: issue='#${ROUTE_ISSUE:-n/a}' instead of issue='#${ISSUE_NUMBER:-n/a}' |
.github/workflows/qwen-autofix.yml (issue-autofix report step ~line 803) |
Agent-written files containing triple-backtick code blocks break the outer code fence rendering in step summary | Use <details> blocks instead of code fences, or use a 4-backtick fence |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
requested changes
Jul 6, 2026
| if (actualValue === undefined || actualValue === null) { | ||
| debugLogger.debug( | ||
| `Param matcher failed: rule="${ruleRaw}" key=${matcher.key} expected="${matcher.valuePattern}" actual=null/undefined`, | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] evaluateParamMatchers has explicit type-guard branches for null/undefined (line 893) and non-string/non-number (line 900), but neither branch is tested. These are security-relevant guards — if a future refactor loosens the type check, { model: null } could silently match model:*.
Suggested change
| ); | |
| it('matchesRule rejects boolean, null, and object param values', async () => { | |
| const rule = parseRule('Agent(model:*)'); | |
| expect(matchesRule(rule, 'agent', undefined, undefined, undefined, undefined, undefined, { model: true })).toBe(false); | |
| expect(matchesRule(rule, 'agent', undefined, undefined, undefined, undefined, undefined, { model: null })).toBe(false); | |
| expect(matchesRule(rule, 'agent', undefined, undefined, undefined, undefined, undefined, { model: undefined })).toBe(false); | |
| expect(matchesRule(rule, 'agent', undefined, undefined, undefined, undefined, undefined, { model: { nested: 'opus' } })).toBe(false); | |
| }); |
— qwen3.7-max via Qwen Code /review
…ine test timeout - Make matchesParamValuePattern case-insensitive to match matchesDomainPattern convention - Remove duplicate JSDoc block before matchesParamValuePattern - Remove dead server_name from stableParamKeys in buildPermissionRules - Add PermissionManager integration tests with toolParams (evaluate, findMatchingDenyRule, hasRelevantRules, hasMatchingAskRule) - Add type guard tests for evaluateParamMatchers (null, undefined, boolean, object) - Fix useStatusLine.test.ts timeout by stubbing cron-task exports in core mock
wenshao
reviewed
Jul 6, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Downgraded from Approve to Comment: CI failing: review-pr. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No new review findings beyond the existing 34 inline comments. Downgraded from Approve to Comment: CI still running.
The implementation is well-structured — the linear-time glob matching in matchesParamValuePattern correctly avoids ReDoS, Object.hasOwn guards against prototype chain lookups, and the stableParamKeys whitelist prevents sensitive data leakage into persisted rules. All 280 tests pass.
Two minor observations for future consideration (not blockers):
- The comma-separated syntax doesn't support OR semantics for a single key (e.g.,
Agent(model:gpt-4,claude-opus)requires two separate rules) - The
{ key: string; valuePattern: string }shape appears as an anonymous inline type in 4 locations — a namedToolParamMatcherinterface would ease future maintenance
— qwen3.7-max via Qwen Code /review
Collaborator
|
@qwen-code /triage |
wenshao
previously approved these changes
Jul 6, 2026
qwen-code-ci-bot
approved these changes
Jul 6, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
Verification report — real-flow testing of
|
mcp__github__create_issue(repo:secret-repo) on {repo:"secret-repo"} |
|
|---|---|
base edc0555ed |
deny ✅ |
PR head ed40d657c |
default → allowed ❌ |
Bare mcp__github__create_issue still denies; adding the (repo:…) filter silently disables the entire deny rule. For a security control that is exactly the wrong direction, and it's why the earlier "MCP specifier rejection = correct hardening" note is misleading — for a deny rule, "reject the match" means "stop denying."
Suggested fix: remove the !canonicalName.startsWith('mcp__') guard in parseRule. That single change makes the already-present matchesRule MCP block live and correct (specifier becomes empty → matchers populated → checked), while stray non-key:value MCP specifiers are still rejected as before.
Reproduce
terminal output (text)
══════════════════════════════════════════════════════════════════════════
PR #6106 Tool(param:value) permission syntax — real-flow verification
worktree ed40d657c · core built from PR head · decisions from shipped code
══════════════════════════════════════════════════════════════════════════
[0] Ground truth: the REAL AgentTool parameter schema
properties = [ description, prompt, subagent_type, run_in_background, isolation ] additionalProperties = false
✓ WORKS Agent tool has NO "model" parameter (closed schema) → subagent model comes from subagentConfig.model, never a tool arg
[1] The generic mechanism works for params that really exist
✓ WORKS Agent(subagent_type:coder) denies a coder subagent
✓ WORKS Agent(subagent_type:*) wildcard denies any subagent_type
✓ WORKS Agent(isolation:worktree) denies worktree launches (genuinely new rule)
✓ WORKS Skill(args:danger) denies on the real "args" param
[2] DEFECT #1 Agent(model:opus) — the PR headline & issue #6100's goal
realistic subagent launch = {"description":"review code","prompt":"do it","subagent_type":"coder"}
ctx.toolParams.model = undefined → matcher has nothing to match
✗ DEFECT Agent(model:opus) does NOT deny a real subagent launch observed "default", PR body promises "deny"
(only fires if a phantom {model:"opus"} is injected — which the schema forbids
and which the tool ignores; the unit tests only exercise that phantom object)
[3] DEFECT #2 mcp__server__tool(param:value) fails open — a regression
bare mcp__github__create_issue → deny (denies, correct)
param mcp__github__create_issue(repo:secret) → default (A/B: base=deny, PR head=default)
✗ DEFECT adding (repo:...) SILENTLY DISABLES the MCP deny rule parseRule skips mcp__ → key:value stays in specifier → matchesRule "if(specifier)return false"
[4] Backward compatibility (unchanged)
✓ WORKS Bash(git:*) → "git *" still denies "git status"
✓ WORKS WebFetch(domain:example.com) still domain-matches (no param matcher)
✓ WORKS Agent(coder) specifier still denies coder subagent
══════════════════════════════════════════════════════════════════════════
all observations matched · 7 behaviors work · 2 confirmed defects
══════════════════════════════════════════════════════════════════════════
harness (drives the compiled core; no mocks)
/**
* PR #6106 verification harness — drives the REAL compiled permission flow.
*
* Imports the same functions the CoreToolScheduler / ACP Session call at
* permission-check time:
* - buildPermissionCheckContext (permission-helpers.js) ← threads toolParams
* - PermissionManager.evaluate ← rule engine
* - parseRule (rule-parser.js) ← key:value parser
* - AgentTool (agent.js) ← real subagent tool schema
*
* Every decision below is produced by the shipped code, not a mock.
*/
import { PermissionManager } from './packages/core/dist/src/permissions/permission-manager.js';
import { buildPermissionCheckContext } from './packages/core/dist/src/core/permission-helpers.js';
import { parseRule } from './packages/core/dist/src/permissions/rule-parser.js';
import { AgentTool } from './packages/core/dist/src/tools/agent/agent.js';
const TARGET = '/tmp/proj';
let ok = 0, bad = 0;
const G = (s) => `\x1b[32m${s}\x1b[0m`, R = (s) => `\x1b[31m${s}\x1b[0m`;
const Y = (s) => `\x1b[33m${s}\x1b[0m`, B = (s) => `\x1b[1m${s}\x1b[0m`, D = (s) => `\x1b[2m${s}\x1b[0m`;
function pmWithDeny(deny) {
const pm = new PermissionManager({
getPermissionsAllow: () => [], getPermissionsAsk: () => [], getPermissionsDeny: () => deny,
getProjectRoot: () => TARGET, getCwd: () => TARGET, getApprovalMode: () => 'default',
});
pm.initialize(); // constructor does NOT load rules; must init once
return pm;
}
async function decide(deny, tool, params) {
const pm = pmWithDeny(deny);
const ctx = buildPermissionCheckContext(tool, params, TARGET);
return { decision: await pm.evaluate(ctx), ctx };
}
/** expected==true → green check; expected==false → this is a CONFIRMED DEFECT (red). */
function check(desc, cond, isGoodBehavior, note = '') {
const good = isGoodBehavior ? cond : cond; // cond already encodes "observed == expected"
if (cond) { ok++; console.log(` ${isGoodBehavior ? G('✓ WORKS ') : R('✗ DEFECT')} ${desc}${note ? ' ' + Y(note) : ''}`); }
else { bad++; console.log(` ${R('! HARNESS-MISS')} ${desc}${note ? ' ' + Y(note) : ''}`); }
}
console.log(B('\n══════════════════════════════════════════════════════════════════════════'));
console.log(B(' PR #6106 Tool(param:value) permission syntax — real-flow verification'));
console.log(D(' worktree ed40d657c · core built from PR head · decisions from shipped code'));
console.log(B('══════════════════════════════════════════════════════════════════════════'));
// [0] The real Agent tool schema — does a subagent launch even carry a `model`?
console.log(B('\n[0] Ground truth: the REAL AgentTool parameter schema'));
const agentTool = new AgentTool({
getSubagentManager: () => ({ addChangeListener: () => () => {}, listSubagents: async () => [] }),
isAgentTeamEnabled: () => false, getModel: () => 'qwen3-coder-plus',
});
const props = Object.keys(agentTool.schema.parametersJsonSchema.properties ?? {});
console.log(D(` properties = [ ${props.join(', ')} ] additionalProperties = ${agentTool.schema.parametersJsonSchema.additionalProperties}`));
check('Agent tool has NO "model" parameter (closed schema)', !props.includes('model'), true,
'→ subagent model comes from subagentConfig.model, never a tool arg');
// [1] The generic mechanism — works for params the tool actually has
console.log(B('\n[1] The generic mechanism works for params that really exist'));
check('Agent(subagent_type:coder) denies a coder subagent',
(await decide(['Agent(subagent_type:coder)'], 'agent', { description: 'x', prompt: 'y', subagent_type: 'coder' })).decision === 'deny', true);
check('Agent(subagent_type:*) wildcard denies any subagent_type',
(await decide(['Agent(subagent_type:*)'], 'agent', { description: 'x', prompt: 'y', subagent_type: 'anything' })).decision === 'deny', true);
check('Agent(isolation:worktree) denies worktree launches (genuinely new rule)',
(await decide(['Agent(isolation:worktree)'], 'agent', { description: 'x', prompt: 'y', subagent_type: 'c', isolation: 'worktree' })).decision === 'deny', true);
check('Skill(args:danger) denies on the real "args" param',
(await decide(['Skill(args:danger)'], 'skill', { skill: 'shell', args: 'danger' })).decision === 'deny', true);
// [2] DEFECT #1 — the flagship example is inert
console.log(B('\n[2] ') + R('DEFECT #1') + B(' Agent(model:opus) — the PR headline & issue #6100\'s goal'));
const realistic = { description: 'review code', prompt: 'do it', subagent_type: 'coder' };
const d2 = await decide(['Agent(model:opus)'], 'agent', realistic);
console.log(D(` realistic subagent launch = ${JSON.stringify(realistic)}`));
console.log(D(` ctx.toolParams.model = ${d2.ctx.toolParams?.model} → matcher has nothing to match`));
check('Agent(model:opus) does NOT deny a real subagent launch',
d2.decision !== 'deny', false, `observed "${d2.decision}", PR body promises "deny"`);
console.log(D(' (only fires if a phantom {model:"opus"} is injected — which the schema forbids'));
console.log(D(' and which the tool ignores; the unit tests only exercise that phantom object)'));
// [3] DEFECT #2 — MCP param rule fails OPEN (A/B-proven regression)
console.log(B('\n[3] ') + R('DEFECT #2') + B(' mcp__server__tool(param:value) fails open — a regression'));
const d3 = await decide(['mcp__github__create_issue(repo:secret-repo)'], 'mcp__github__create_issue', { repo: 'secret-repo' });
const bare = await decide(['mcp__github__create_issue'], 'mcp__github__create_issue', { repo: 'secret-repo' });
console.log(D(` bare mcp__github__create_issue → ${bare.decision} (denies, correct)`));
console.log(D(` param mcp__github__create_issue(repo:secret) → ${d3.decision} ${R('(A/B: base=deny, PR head=default)')}`));
check('adding (repo:...) SILENTLY DISABLES the MCP deny rule',
d3.decision !== 'deny' && bare.decision === 'deny', false,
'parseRule skips mcp__ → key:value stays in specifier → matchesRule "if(specifier)return false"');
// [4] Backward compatibility — untouched
console.log(B('\n[4] Backward compatibility (unchanged)'));
check('Bash(git:*) → "git *" still denies "git status"',
parseRule('Bash(git:*)').specifier === 'git *' &&
(await decide(['Bash(git:*)'], 'run_shell_command', { command: 'git status' })).decision === 'deny', true);
check('WebFetch(domain:example.com) still domain-matches (no param matcher)',
parseRule('WebFetch(domain:example.com)').specifierKind === 'domain' &&
(await decide(['WebFetch(domain:example.com)'], 'web_fetch', { url: 'https://example.com/a' })).decision === 'deny', true);
check('Agent(coder) specifier still denies coder subagent',
(await decide(['Agent(coder)'], 'agent', { description: 'x', prompt: 'y', subagent_type: 'coder' })).decision === 'deny', true);
console.log(B('\n══════════════════════════════════════════════════════════════════════════'));
const verdict = bad === 0
? `${G('all observations matched')} · ${G('7 behaviors work')} · ${R('2 confirmed defects')}`
: `${R(bad + ' harness misses')} — investigate`;
console.log(B(' ' + verdict));
console.log(B('══════════════════════════════════════════════════════════════════════════\n'));
process.exit(bad > 0 ? 1 : 0);Scope note: I did not re-litigate CI or the generic parser/matcher quality (both look good). These two findings are about end-to-end behavior that the unit tests, by construction, can't catch.
中文版（点击展开）
验证报告 — 对 Tool(param:value) 的真实链路测试
方法。 从本 PR head（ed40d657c）构建 packages/core，驱动真实的、发布态权限链路 —— buildPermissionCheckContext → PermissionManager.evaluate → matchesRule，即 CoreToolScheduler 与 ACP Session 在审批时实际调用的函数。无 mock，下面每个判定都由编译后的代码产生。Finding 2 还通过替换编译后的 rule-parser.js，与 merge-base（edc0555ed）做了 A/B。单测：permission-manager.test.ts 280/280 通过。
结论。 通用机制实现干净、完全向后兼容。但有两点在端到端层面不成立，其中一点正是本 PR 的头号用例。
✅ 确实可用（真实链路已确认）
- 对工具真实拥有的参数做匹配：
Agent(subagent_type:coder)、Agent(subagent_type:*)、Agent(isolation:worktree)、Skill(args:danger)均能正确 deny/allow。 - 多匹配器的 AND 逻辑、
*通配值。 - 向后兼容完好：
Bash(git:*)→git *、WebFetch(domain:…)域名匹配、既有的Agent(coder)specifier 都未变。
🔴 Finding 1（High）—— 头号示例 Agent(model:opus) 是空转的；issue #6100 的真实目标未达成
#6100 的存在正是为了*"阻止请求 Opus 模型的子代理"*（堵住子代理 model override 绕过限制的口子）。在 qwen-code 中，本机制无法堵这个口子：
- 真实的
AgentToolschema 没有model参数 ——properties = [description, prompt, subagent_type, run_in_background, isolation]，additionalProperties: false。（Claude Code 的 Agent 工具有model参数；qwen-code 没有。） - 子代理的模型是后续从
subagentConfig.model解析的（子代理定义文件，agent.ts:1628），从不来自工具调用参数。 - 因此在权限检查时
ctx.toolParams.model恒为undefined，对真实调用{description, prompt, subagent_type}，Agent(model:opus)求值为default（不拦截）。
针对 model 的 11 个新测试，全都是把手工构造的 { model: 'opus' } 对象直接喂给 matchesRule —— 而真实 Agent 工具在运行时从不产生这个对象。匹配器是对的，但运行时根本没有这个输入。于是以 "Closes #6100" 合入，等于交付了一个"文档头号示例静默失效、且关联 issue 的核心诉求未实现"的功能。
修复建议： 要么在权限上下文中解析出子代理的实际模型（在检查前用 subagentConfig.model 合成一个 model），真正关闭 #6100；要么从 PR 描述、types.ts 文档注释和测试中去掉 model:* 示例，并说明 Agent 参数匹配只覆盖真实调用参数（subagent_type、isolation、name）。
🔴 Finding 2（High，回归）—— mcp__server__tool(param:value) 静默 fail-open
Patch 2（8e5941323）在 matchesRule 中加入了 MCP 参数检查；Patch 3（89715c74c）又在 parseRule 里加了 !canonicalName.startsWith('mcp__')，使 MCP 规则永远不会产生 toolParamMatchers。两个后果：
-
matchesRule里的 MCP 参数检查块成了死代码（MCP 的 matchers 永远为空）。 -
回归 —— 由于
key:value文本仍留在rule.specifier中，MCP 分支的if (rule.specifier) return false触发，规则什么都不匹配。同规则同输入 A/B：mcp__github__create_issue(repo:secret-repo)对{repo:"secret-repo"}base edc0555eddeny ✅ PR head ed40d657cdefault → 放行 ❌ 裸
mcp__github__create_issue仍会拦截；一旦加上(repo:…)过滤，整条 deny 规则被静默禁用。对安全控制而言这是完全反向的；这也是为什么"MCP specifier rejection = 正确加固"的说法有误 —— 对 deny 规则来说，"拒绝匹配"等于"停止拦截"。
修复建议： 删掉 parseRule 中的 !canonicalName.startsWith('mcp__') 判断。仅此一处改动就能让 matchesRule 中已有的 MCP 块生效且正确（specifier 变空 → matchers 被填充 → 被检查），同时非 key:value 的杂散 MCP specifier 仍按原样被拒。
范围说明：我没有重复审查 CI 或通用 parser/matcher 的质量（两者都不错）。这两个发现针对的是单测按其构造方式无法覆盖的端到端行为。
yiliang114
approved these changes
Jul 7, 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
5 participants
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 this PR does
Adds a
Tool(param:value)syntax to permission rules that lets users grant or deny tool access based on specific input parameter values. For example,Agent(model:opus)can now deny subagent launches that request the Opus model, whileAgent(coder,model:*)can deny all coder-type subagents regardless of which model they use.The implementation extends the rule parser to extract
key:valuepairs from specifiers of literal-kind tools (likeAgentandSkill), threads the full tool input parameters throughPermissionCheckContext, and evaluates each matcher at permission check time using glob-aware value matching. Existing specifier kinds (command, path, domain) and legacy:*syntax for shell commands remain unaffected.Why it's needed
Current permission rules can only match tools by name, shell command pattern, file path, or domain. There is no way to restrict tools based on their input parameters — for example, a user might want to allow subagents in general but block those using expensive models, or allow MCP tools but deny specific operations. This forces users into an all-or-nothing choice per tool, which is too coarse for real-world permission policies.
With parameter-level matching, users can express fine-grained policies like:
Agent(model:opus)— deny agents using Opus model (cost control)Agent(coder,model:*)— deny coder-type agents with any model specifiedSkill(skills:dangerous-skill)— deny a specific skill by nameThis aligns with Claude Code's permission syntax and addresses a frequently requested feature for enterprise and team policy enforcement.
Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/permissions/permission-manager.test.ts— all 258 tests should pass, including 11 new tests covering:parseRuleextractingkey:valuepairs from specifiersmatchesRuleevaluating param matchers againsttoolParamsmodel:*) matching any valueAgent(coder,model:opus))Bash(git:*)still converts toBash(git *),WebFetch(domain:x.com)unaffectedcd packages/core && npx tsc --noEmitpermissions.deny = ["Agent(model:opus)"]in settings, then attempt to spawn a subagent withmodel: "opus"— it should be blockedEvidence (Before & After)
before：
after：
Tested on
Environment (optional)
Unit tests only —
npx vitest runfrompackages/core.Risk & Scope
key:valueparsing could theoretically conflict with future specifier kinds that use colons. Mitigated by only parsing for literal-kind specifiers (Agent, Skill) and preserving existing behavior for command/path/domain kinds.buildPermissionCheckContext, which already includes them).Bash(git:*)legacy syntax still converts toBash(git *)as before.Linked Issues
Closes #6100
中文说明
此 PR 做了什么
为权限规则添加了
Tool(param:value)语法，允许用户根据工具的特定输入参数值来授权或拒绝工具访问。例如，Agent(model:opus)现在可以阻止请求使用 Opus 模型的子代理启动，而Agent(coder,model:*)可以阻止所有 coder 类型的子代理，无论它们使用哪个模型。实现扩展了规则解析器，从 literal-kind 工具（如
Agent和Skill）的 specifier 中提取key:value对，将完整的工具输入参数通过PermissionCheckContext传递，并在权限检查时使用支持通配符的值匹配来评估每个匹配器。现有的 specifier 类型（command、path、domain）和 shell 命令的遗留:*语法不受影响。为什么需要
当前的权限规则只能按工具名称、shell 命令模式、文件路径或域名进行匹配。无法根据输入参数来限制工具——例如，用户可能希望允许子代理但阻止使用昂贵模型的子代理，或允许 MCP 工具但拒绝特定操作。这迫使用户对每个工具做出全有或全无的选择，对于真实的权限策略来说过于粗糙。
通过参数级匹配，用户可以表达细粒度策略，如：
Agent(model:opus)— 拒绝使用 Opus 模型的代理（成本控制）Agent(coder,model:*)— 拒绝所有指定模型的 coder 类型代理Skill(skills:dangerous-skill)— 按名称拒绝特定技能这与 Claude Code 的权限语法一致，解决了企业和团队策略执行中频繁请求的功能需求。