fix(core): resolve a parameter expansion in command position to its command root - #7143
Merged
Conversation
…ommand root
`getCommandRoot` had no answer for a command that begins with a parameter
expansion, and the shell tool treats no-answer as a hard refusal — "Could not
identify command root to obtain permission from user" — issued from
validateToolParamValues, before any approval mode is consulted, YOLO included.
That form is not exotic: the bundled /review skill invokes every one of its
~27 commands as `"${QWEN_CODE_CLI:-qwen}" review …`, so every review run
opened with a refused command until the model hand-resolved the variable and
retried with the explicit path. Observed on every dogfooded run since the
variable landed.
The resolver mirrors what the spawned shell will do, against the environment
it will inherit: `:-` substitutes the default when the variable is unset OR
empty, `-` only when unset, and a bare `$VAR` that resolves to nothing still
yields no root — such a command has nothing to name and stays refusable.
Leading env assignments are skipped first, exactly as the plain-token path
skips them. With the variable set, the root becomes the real entry's basename
(`dev.js`, `qwen`), so permission prompts and allowlists name the binary that
actually runs.
Verified end-to-end: the exact skill form now executes through the real shell
tool in Auto mode, where it was previously refused in YOLO. Six tests pin the
POSIX semantics both ways; disabling the resolver turns all six red; the
shell.ts and monitor.ts caller suites pass unchanged.
Collaborator
|
Thanks for the PR! Template: Uses custom headings (What / Why / Semantics / Verification) instead of the PR template sections. Content is thorough — clear reproduction, POSIX semantics explanation, and test evidence — so passing this with a note. Please use the PR template headings in future PRs for consistency. Problem: Observed bug with strong evidence. The bundled Direction: Aligned. The shell tool needs to identify the command root for permission checking — a parameter expansion in command position is a legitimate pattern that should be resolvable, not a reason to refuse the command. This is a narrowly scoped fix to Size: Core path ( Approach: Scope feels right — one focused function ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板： 使用了自定义标题（What / Why / Semantics / Verification），而非 PR 模板的标准章节。但内容充分——有清晰的复现、POSIX 语义说明和测试证据——因此通过并附注。建议后续 PR 使用 PR 模板 的标准标题以保持一致性。 问题： 已观测到的 bug，有强证据。内置的 方向： 对齐。shell 工具需要识别命令根以进行权限检查——命令位置的参数展开是一种合理的模式，应该被解析而非作为拒绝命令的理由。这是对 规模： 核心路径（ 方案： 范围合理——一个专注的函数（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewThe implementation is clean and well-reasoned. Here's my assessment:
POSIX semantics — the
Integration point — the resolver is called in Reuse — the new function leverages three existing helpers ( Tests — six well-targeted test cases covering both operators, bare variables, empty-but-set, unset, and leading env assignments. The No critical issues found. The code is focused, the regex is narrow, and the integration is minimal. Test ResultsUnit TestsAll tests pass in the PR branch: Real-Scenario Verification (tmux)Ran BEFORE (main branch — no fix): AFTER (PR branch — with fix): Every scenario that previously returned 中文说明代码审查实现简洁且设计合理。
POSIX 语义 — 集成点 — 解析器在 复用 — 新函数复用了三个已有辅助函数，而非重新实现 token 解析或引号剥离。 测试 — 六个针对性测试用例覆盖两种操作符、裸变量、空但已设置、未设置和前置环境变量赋值。 未发现关键问题。 代码专注、正则窄、集成最小。 测试结果单元测试PR 分支所有测试通过：shell-utils 145 个（含 6 个新增）、shell 278 个、monitor 80 个，共 503 个，0 失败。 真实场景验证（tmux）对比 main（修复前）和 PR 分支（修复后），对内置 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 This is a textbook-quality fix: real problem, clear reproduction, minimal change, correct POSIX semantics, comprehensive tests. Reflection on the three stages:
Verdict: Approving. This fixes a real daily-use breakage with a focused, well-tested change. Ship it. ✅ 中文说明信心度: 5/5 这是一个教科书级别的修复：真实问题、清晰复现、最小变更、正确的 POSIX 语义、全面的测试。 三个阶段回顾：
结论： 批准。这修复了一个真实影响日常使用的中断问题，变更专注且测试充分。可以合并。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — real bug, clear fix, well-tested. Ship it. ✅
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. |
doudouOUC
reviewed
Jul 18, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
The targeted quoted ${VAR:-default} path works and the package test passes. I found one narrower mismatch in the newly claimed unquoted $VAR support, noted inline.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Review caught the claimed-but-unmodeled half: an UNQUOTED expansion field-splits before command lookup. With the variable unset, the shell removes the empty expansion and runs the next word — this helper returned no root and the valid command stayed hard-refused; with a multi-word value, the shell's command is the first field — this helper reported the whole value as the root. Both verified against real bash. Quoting now decides, as in the shell: a quoted expansion is one word (empty = an empty command name, refusable); an unquoted one splits, an empty result falls through to the next token, and the first field names the root. The regex backreference also stops accepting asymmetric quoting. Both branches are mutation-checked: dropping the empty-removal or the split turns the new test red.
qwen-code-ci-bot
approved these changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
yiliang114
reviewed
Jul 18, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Clean fix for a real dogfooded problem — "${QWEN_CODE_CLI:-qwen}" review … commands had no identifiable root and were hard-refused by the shell tool.
What's good
- POSIX parameter expansion semantics are correctly implemented:
:-substitutes on unset-or-empty,-only on unset — verified against the test cases for both operators with set, empty, and unset variables. - Quoted vs unquoted distinction is faithful to shell behavior: quoted = single word (no splitting), unquoted = field-split on whitespace. The test for
$VAR OKwithVAR='/usr/bin/env printf'returning'env'(not'env printf') pins this correctly. - Empty unquoted expansion falls through to the next token — matching how bash removes the empty expansion and shifts the command position. Test pins this with
$VAR printf OK→'printf'. - Integration with
getCommandRootsis tested end-to-end ("${QWEN_CODE_CLI:-qwen}" review fetch-pr 7 --out x.json && echo done→['qwen', 'echo']), which is the actual use case. - Leading env assignments (
FOO=1 "${VAR:-default}" …) are correctly skipped before expansion resolution.
Suggestions
-
Regex readability (
PARAMETER_EXPANSION_COMMAND): The regex is dense — consider a brief inline comment labeling each capture group (e.g.,// m[1]=quote, m[2]=braced name, m[3]=operator, m[4]=default, m[5]=bare name). Not blocking, but the next reader will thank you. -
Single-quoted expansions:
'${VAR:-default}'won't match the regex (only"is recognized), which is correct — single quotes suppress expansion in shell. The fallback path through the tokenizer would return the literal string as the root, which is fine. But a one-line comment on the regex noting "double-quoted only — single quotes suppress expansion" would make the intent explicit.
Verdict
No blockers. Well-scoped fix with thorough test coverage. LGTM.
— Qwen Code review
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.
What
getCommandRootnow resolves a parameter expansion standing in command position —"$VAR",${VAR},"${VAR:-default}",${VAR-default}— the way the spawned shell will resolve it, against the environment it will inherit.Why
A command with no identifiable root is hard-refused by the shell tool (
validateToolParamValues→ "Could not identify command root to obtain permission from user") before any approval mode is consulted — YOLO included. And the form is not exotic: the bundled/reviewskill invokes every one of its ~27 commands as"${QWEN_CODE_CLI:-qwen}" review …so every review run opened with a refused command until the model noticed, hand-resolved the variable (
echo $QWEN_CODE_CLI), and retried with the explicit path. Observed on every dogfooded run since #7033 landed the variable; reproduced directly:Semantics
POSIX, deliberately narrow:
:-substitutes the default when the variable is unset or empty;-only when unset$VARthat resolves to nothing still yields no root — such a command has nothing to name, and stays refusable exactly like an empty commandFOO=1 "${BAR:-baz}" …) are skipped first, as the plain-token path already doesdev.js,qwen) — permission prompts and allowlists name the binary that actually runs, not a${…}literalVerification
shell.ts/monitor.tscaller suites pass unchanged (423 tests)