fix(cli): complete repeated skill slash commands#7720
Conversation
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks @Sparkle6979 — the fix itself looks well targeted (reordering the mid-input check ahead of the start-of-line block, and dropping the now-redundant startsWith('/') guard, is exactly the right shape for #7717), and the regression tests are appreciated.
The one blocker is the PR body: it doesn't follow the PR template. It currently has ## Summary and ## Validation, but the template's required sections are missing:
## What this PR does## Why it's needed## Reviewer Test Plan(with### How to verify,### Evidence (Before & After),### Tested on)## Risk & Scope## Linked Issues(yourFixes #7717line is great — just move it under this heading)
Could you restructure the body to match the template? Since this is a user-visible completion change, the Evidence (Before & After) section is the part reviewers most want to see — a short before/after of typing /skill1 /sk<TAB> and /skill1⏎/sk<TAB> would make this easy to approve. The code is in good shape; this is purely a formatting ask.
中文说明
感谢 @Sparkle6979 —— 修复本身很到位（把 mid-input 检测调整到行首检测之前，并移除现在已经多余的 startsWith('/') 守卫，正是 #7717 需要的形态），回归测试也很好。
唯一的阻塞项是 PR 正文：它没有遵循 PR 模板。目前使用的是 ## Summary 和 ## Validation，但模板要求的章节缺失：
## What this PR does## Why it's needed## Reviewer Test Plan（包含### How to verify、### Evidence (Before & After)、### Tested on）## Risk & Scope## Linked Issues（你的Fixes #7717这行很好——把它移到这个标题下即可）
能否按模板调整正文结构？由于这是一个用户可见的补全改动，Evidence (Before & After) 是 reviewer 最想看的部分——一段输入 /skill1 /sk<TAB> 和 /skill1⏎/sk<TAB> 的 before/after 演示会让这个 PR 很容易通过。代码本身没有问题，这纯粹是格式上的请求。
— Qwen Code · qwen3.8-max-preview
f92e16e to
d4c608f
Compare
|
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）为单个提交。 |
Review + Linux E2E verification report (real build, tmux)Verdict: the fix is correct, minimal, and verified working end-to-end on Linux. Code review reasoning
Verification evidence (Linux, commit d4c608f)
This fills the PR's "Linux: not tested" gap. No regressions observed in line-start completion. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
| function isStackedSkillCompletableCommand(cmd: SlashCommand): boolean { | ||
| return ( | ||
| cmd.kind === CommandKind.SKILL && cmd.userInvocable !== false && !cmd.hidden | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Duplicated predicate — the three-condition check here (kind === SKILL && userInvocable !== false && !hidden) is duplicated inline inside isValidStackedSkillPrefix at commands.ts:46-50. Both answer the same question but live in different modules.
Failure scenario: if a maintainer adds a fourth condition (e.g. cmd.deprecated) to one site without discovering the other, the dropdown would offer commands the prefix validator rejects — silent no-op completions.
Concrete fix: export this predicate from commandUtils.ts (alongside the existing isMidInputCompletableCommand), then import it in both useCommandCompletion.tsx and commands.ts.
| function isStackedSkillCompletableCommand(cmd: SlashCommand): boolean { | |
| return ( | |
| cmd.kind === CommandKind.SKILL && cmd.userInvocable !== false && !cmd.hidden | |
| ); | |
| } | |
| function isStackedSkillCompletableCommand(cmd: SlashCommand): boolean { | |
| return ( | |
| cmd.kind === CommandKind.SKILL && | |
| cmd.userInvocable !== false && | |
| !cmd.hidden | |
| ); | |
| } |
中文说明
[Suggestion] 重复的判断条件 —— 此处的三条件检查在 commands.ts:46-50 的 isValidStackedSkillPrefix 中被内联重复。两者回答同一个问题，却分处不同模块。
失败场景：如果维护者向其中一个位置添加了第四个条件（例如 cmd.deprecated），而未发现另一个，则下拉菜单会提供前缀验证器会拒绝的命令 —— 产生静默的无效补全。
建议修复：将此谓函数导出到 commandUtils.ts（与现有的 isMidInputCompletableCommand 并列），然后在 useCommandCompletion.tsx 和 commands.ts 中导入它。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in e4855cf. I kept the shared predicate in src/utils/commands.ts so the lower-level command utility does not depend on ui/utils. Both call sites now use it.
| it('rejects non-skill, unknown, and prompt-text prefixes', () => { | ||
| expect(isValidStackedSkillPrefix('/help ', mockCommandsWithSkills)).toBe( | ||
| false, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Missing test coverage for userInvocable: false and hidden: true branches — isValidStackedSkillPrefix checks both conditions, but no test creates a skill command with userInvocable: false or hidden: true. The existing rejection tests use /help (kind BUILT_IN), so rejection fires on kind !== SKILL and never reaches those checks.
Failure scenario: if either the userInvocable or hidden check were accidentally removed or inverted, every existing test would still pass — the regression would ship undetected.
Concrete fix: add test cases with { kind: SKILL, userInvocable: false } and { kind: SKILL, hidden: true } skill commands and assert isValidStackedSkillPrefix returns false.
中文说明
[Suggestion] userInvocable: false 和 hidden: true 分支缺少测试覆盖 —— isValidStackedSkillPrefix 检查了这两个条件，但没有测试用例创建一个 userInvocable: false 或 hidden: true 的 skill 命令。现有的拒绝测试使用 /help（kind 为 BUILT_IN），拒绝在 kind !== SKILL 处触发，从未到达这些检查。
失败场景：如果 userInvocable 或 hidden 检查被意外移除或反转，所有现有测试仍会通过 —— 回归将无法被检测到。
建议修复：添加 { kind: SKILL, userInvocable: false } 和 { kind: SKILL, hidden: true } 的测试用例，断言 isValidStackedSkillPrefix 返回 false。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added both cases in e4855cf. The prefix validator now covers userInvocable: false and hidden: true skills.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Review — verified locally at head
|
| Check | Result |
|---|---|
vitest run on the 5 focused files |
167/167 pass (matches PR body) |
Wider sweep: src/ui/hooks src/ui/utils src/utils |
3338 pass; the 5 failures are env artifacts of my worktree (@qwen-code/channel-github unresolved, ink width in MarkdownDisplay/CodeColorizer) — unrelated to this diff |
tsc --noEmit (packages/cli) |
clean (only the pre-existing baseUrl deprecation notice) |
prettier --check on all 6 changed files |
clean |
Behaviour A/B, base 596abd966 vs head, 20 inputs, driving useCommandCompletion with the real useSlashCompletion + real TextBuffer (not the mock the unit tests use) |
see below |
| Mutation matrix, 9 mutations | 8 caught, 1 survivor (finding 6) |
The A/B accepts suggestion 0 for each input and feeds the resulting buffer back through parseStackedSlashCommands, so it checks the completion candidates against what execution actually accepts — not just what the hook was called with:
input base -> head (suggestions) exec after accept
/review /sto [] -> [store-locally, front-end-store-rules] skills=[review,store-locally]
/review\n/sto [] -> [store-locally, front-end-store-rules] skills=[review,store-locally]
/review /sto [] -> [store-locally, front-end-store-rules] skills=[review,store-locally]
/review\n\n/sto [] -> [store-locally, front-end-store-rules] skills=[review,store-locally]
/rv /sto (altName) [] -> [store-locally, front-end-store-rules] skills=[review,store-locally]
/review /sto trailing text [] -> [...] -> "/review /store-locally trailing text" rest="trailing text"
/review / [] -> [s1,s2,s3,s4,review,store-locally,front-end-store-rules]
/s1 /s2 /s3 /s4 /rev [] -> [review] (5th slot allowed)
/s1 /s2 /s3 /s4 /review /rev [] -> [] (6th blocked — MAX_STACKED_SKILLS boundary correct)
/review /store-not (FILE) [] -> [] (non-SKILL excluded)
/review /hidden [] -> []
/review /model-only [] -> []
/stats /sto [] -> [] (no fall-through to mid-input)
/unknown /sto [] -> []
/review arg /sto [] -> []
please /sto [store-notes, ...] -> unchanged (mid-input intact)
/sto unchanged (empty dropdown + ghost "re-notes")
The MAX_STACKED_SKILLS boundary is exactly right (tokens.length >= 5 on the prefix ⇒ a 4-skill prefix still offers a 5th, a 5-skill prefix offers nothing), and the candidate set matches parseStackedSlashCommands' acceptance rule (kind === SKILL) precisely — including the deliberate inclusion of modelInvocable: false skills, which is the part a reviewer is most likely to second-guess.
Mutations that correctly turned the suite red: max-stack >=→>; dropping !hidden; dropping userInvocable !== false; dropping kind === SKILL; dropping the whitespace conjunct of isInitialCommandOnFirstLine; dropping !isSlashLedInput from isRegularMidInput; removing the ghost-text prefix guard; and restoring if (input.startsWith('/')) return null; in findMidInputSlashCommand. That last one is the real oracle — the new tests genuinely pin the bug fix.
Also confirmed: findMidInputSlashCommand has no callers outside useCommandCompletion.tsx, so the contract change is contained.
1. Same-line stacked skills with disable-model-invocation are offered but render unhighlighted
findSlashCommandTokens (→ parseInputForHighlighting, commandUtils.ts:389-397) marks any non-line-start token valid only when modelInvocable === true. The new stacked path only requires kind === SKILL. Those two rules now disagree, and the PR moves the disagreement from "you'd have to type it by hand" to "it's the top suggestion":
/review /store-locally -> ["command:/review", "default: ", "default:/store-locally"] <-- 2nd token not highlighted
/review\n/store-locally -> ["command:/review", "default:\n", "command:/store-locally"] <-- fine (line-start)
(where store-locally is kind: SKILL, userInvocable: true, modelInvocable: false, i.e. a skill with disable-model-invocation: true in its frontmatter — a supported user-facing field). Both forms execute identically as skills=[review, store-locally].
So after accepting the very suggestion this PR adds, the same-line form shows the token as plain text while the multi-line form shows it as a command. Pre-existing inconsistency, newly reachable by default.
Concrete fix: in findSlashCommandTokens, treat a mid-input token as valid when the text before it is a valid stacked-skill prefix — isValidStackedSkillPrefix is now exported and does exactly that check. Happy for this to be a follow-up issue instead, but it's worth an explicit decision rather than shipping silently.
2. Two extra full-buffer code-point scans per render
With a counting wrapper around toCodePoints, one render over a 100 KB single-line buffer:
| buffer | base | head |
|---|---|---|
slash-led (/review …x… /sto) |
3 calls / 300 K chars | 6 calls / 600 K chars |
plain text (please …x… /sto) |
4 calls / 400 K chars | 5 calls / 500 K chars |
toCodePoints(buffer.text).slice(0, midCmd.startPos).join('') is built twice (routing memo + ghost-text memo), on top of the two findMidInputSlashCommand calls that each already build the same array internally. On a slash-led buffer base short-circuited to ~0 of this work, so it's a 2× increase on a path that runs per keystroke in the render body. Absolute cost is a few ms at 100 KB — modest, but the fix is nearly free:
findMidInputSlashCommand already holds codePoints; return the prefix string (or the array) alongside token/startPos/partialCommand, and both call sites drop their re-derivation.
3. isSlashLedInput should use isSlashCommand(), not raw startsWith('/')
prefix.trimStart().startsWith('/') also catches inputs that this repo deliberately classifies as not slash commands. Verified dead zone:
/tmp/foo.txt please /sto -> mode IDLE, no suggestions, no ghost text
hasSlashCommandPathSeparator makes this a plain prompt (handleSlashCommand returns false), so mid-input completion for /sto is exactly what should fire. Same story for //note … /sto. This matches base behaviour so it isn't a regression — but since the predicate is being rewritten anyway, isSlashCommand(prefix.trimStart()) (already imported in this file, and it excludes //, /* and path-like tokens) is both more precise and self-documenting.
4. Undocumented behaviour change: leading blank line
A/B difference not covered by the PR body or its tests:
"\n/review please /sto" base: [store-notes, front-end-store-rules] head: IDLE, nothing
I think head is the better behaviour (that buffer submits as /review with args please /sto, so the old ghost text was suggesting a completion that would never execute), but it is a behaviour change and belongs in Risk & Scope.
5. Nit — the stack's own members are offered again
/review /review is suggested and accepted; parseStackedSlashCommands then returns skills=[review, review] and slashCommandProcessor runs the action twice, appending its content twice. Filtering prefix tokens out of the candidate list is a two-line change.
6. Nit — dead clause in isValidStackedSkillPrefix
token.length === 1 never changes the outcome: findCommandByName('') already returns undefined. Mutation M9 (deleting it) left all 133 tests green — the only survivor in the matrix. Drop it, or keep it with a comment saying it's a fast path.
7. Nit — two conditions that can't fire in the TUI
CommandService.getCommandsForMode('interactive') (CommandService.ts:140) already filters !cmd.hidden && cmd.userInvocable !== false, so isStackedSkillCompletableCommand's hidden/userInvocable checks are unreachable through the interactive path. Fine as defensive coding on an exported helper — just flagging that the tests added for them (correctly, at the bot's request) cover a state the product doesn't produce.
8. Test note — ' /sto' pins routing, not outcome
With the real useSlashCompletion, an indented first command yields zero dropdown suggestions: useCommandParser does query.substring(1), so ' /sto' → partial = '/sto', which fzf can't match against any command name. Mid-input ghost text still fires (store-notes). Identical on base, and the isInitialCommandOnFirstLine guard correctly preserves it — but the test asserts only the arguments useSlashCompletion was called with, so it would keep passing even if that dropdown never produced anything. A one-line comment saying "asserts routing, not suggestions" would prevent a future reader from over-reading it.
Nice work — the three-way split of SlashCompletionContext reads much better than the old boolean, the isValidStackedSkillPrefix extraction lines the completion filter up with the executor's own rule, and the max-stack boundary is one of the easier things to get off by one and it's correct.
中文说明
评审结论 — 已在 head e4855cf8 本地验证
结论：无阻断问题。 修复正确，路由重写在我能探测到的所有场景下都保持了原有行为，测试也确实具备承载力（load-bearing）。发现 1 建议在合并前明确决策，其余为小问题。
注：@gwinthis 的 Linux E2E 是在 d4c608fd2（第一个 commit）上跑的，之后又落了三个 commit（含 isStackedSkillCompletableCommand 重构）。下面所有结论均在 head 上重跑。
我实际运行的检查
- 5 个重点测试文件：167/167 通过（与 PR 正文一致）
- 更大范围
src/ui/hooks src/ui/utils src/utils：3338 通过；5 个失败是我 worktree 的环境问题（@qwen-code/channel-github无法解析、MarkdownDisplay/CodeColorizer的 ink 宽度），与本 diff 无关 tsc --noEmit（packages/cli）：干净prettier --check6 个改动文件：干净- 行为 A/B：base
596abd966对比 head，20 个输入，使用真实的useSlashCompletion+ 真实TextBuffer驱动useCommandCompletion（而非单测里的 mock） - 变异测试矩阵：9 个变异，8 个被测试捕获，1 个存活（见发现 6）
A/B 会对每个输入接受第 0 个候选，再把结果 buffer 喂回 parseStackedSlashCommands，因此校验的是"补全候选 vs 执行端实际接受的集合"，而不只是 hook 的调用参数。MAX_STACKED_SKILLS 边界完全正确（前缀 4 个技能仍会给出第 5 个，前缀 5 个则不再给出），候选集也精确匹配 parseStackedSlashCommands 的接受规则（kind === SKILL），包括有意纳入 modelInvocable: false 的技能——这正是 reviewer 最可能质疑的一点。
被正确捕获的变异包括：max-stack >=→>、去掉 !hidden、去掉 userInvocable !== false、去掉 kind === SKILL、去掉 isInitialCommandOnFirstLine 的空白前缀判定、去掉 isRegularMidInput 的 !isSlashLedInput、移除 ghost-text 前缀守卫，以及恢复 findMidInputSlashCommand 中的 if (input.startsWith('/')) return null;。最后一项是真正的 oracle——说明新测试确实钉住了这个 bug 修复。
另外确认：findMidInputSlashCommand 在 useCommandCompletion.tsx 之外没有调用方，契约变更影响面是收敛的。
1. 同一行的 disable-model-invocation 技能会被推荐，但不会高亮
findSlashCommandTokens（→ parseInputForHighlighting）对非行首 token 仅在 modelInvocable === true 时判定为 valid，而新的 stacked 路径只要求 kind === SKILL。两条规则现在不一致，且本 PR 把这个不一致从"必须手打才会遇到"变成了"它就是首选候选"：
/review /store-locally -> ["command:/review", "default: ", "default:/store-locally"] <-- 第二个 token 未高亮
/review\n/store-locally -> ["command:/review", "default:\n", "command:/store-locally"] <-- 正常（行首）
（store-locally 为 kind: SKILL, userInvocable: true, modelInvocable: false，即 frontmatter 里写了 disable-model-invocation: true 的技能——这是对用户开放的字段。）两种写法执行结果都是 skills=[review, store-locally]。
建议修复：在 findSlashCommandTokens 中，当 mid-input token 之前的文本构成合法 stacked skill 前缀时也判定为 valid——isValidStackedSkillPrefix 现在已导出，正好做这件事。放到 follow-up issue 也可以，但希望是一个明确决策而非默默带过。
2. 每次渲染多出两次全 buffer 的 code-point 扫描
给 toCodePoints 加计数包装，100 KB 单行 buffer 单次渲染：
| buffer | base | head |
|---|---|---|
| 以斜杠开头 | 3 次 / 30 万字符 | 6 次 / 60 万字符 |
| 普通文本 | 4 次 / 40 万字符 | 5 次 / 50 万字符 |
toCodePoints(buffer.text).slice(0, midCmd.startPos).join('') 被构造了两次（路由 memo + ghost-text memo），此外两次 findMidInputSlashCommand 内部又各自构建了同一个数组。以斜杠开头的 buffer 在 base 上会短路掉这部分工作，因此这是渲染路径上按键级别的 2× 增长。100 KB 下绝对成本只有几毫秒，但修法几乎免费：findMidInputSlashCommand 手上已经有 codePoints，把 prefix 一并返回即可，两个调用点都不必重算。
3. isSlashLedInput 建议改用 isSlashCommand() 而非裸的 startsWith('/')
prefix.trimStart().startsWith('/') 会误伤本仓库明确判定为"非 slash command"的输入。已验证的盲区：
/tmp/foo.txt please /sto -> mode IDLE，无候选，无 ghost text
hasSlashCommandPathSeparator 会让它作为普通 prompt 提交（handleSlashCommand 返回 false），所以 /sto 的 mid-input 补全本该触发。//note … /sto 同理。这与 base 行为一致，不算回归——但既然这个判定本来就在重写，用 isSlashCommand(prefix.trimStart())（本文件已引入，且已排除 //、/* 和路径形态）会更精确也更自解释。
4. 未在正文说明的行为变更：开头空行
"\n/review please /sto" base: [store-notes, front-end-store-rules] head: IDLE，无任何提示
我认为 head 的行为更好（该 buffer 会作为 /review + args please /sto 提交，旧的 ghost text 提示的是一个永远不会执行的补全），但这是行为变更，应写入 Risk & Scope。
5. 小问题：已在栈内的技能仍会被再次推荐
/review /review 会被推荐并可接受，parseStackedSlashCommands 随后返回 skills=[review, review]，slashCommandProcessor 会执行两次 action 并追加两遍内容。把前缀中已有的 token 从候选里过滤掉是两行的改动。
6. 小问题：isValidStackedSkillPrefix 中的死代码
token.length === 1 不会改变任何结果：findCommandByName('') 本来就返回 undefined。变异 M9（删除该子句）后 133 个测试全绿——是矩阵中唯一存活的变异。建议删除，或保留并注释说明这是快速路径。
7. 小问题：两个在 TUI 中不可能触发的条件
CommandService.getCommandsForMode('interactive') 已经过滤了 !cmd.hidden && cmd.userInvocable !== false，因此 isStackedSkillCompletableCommand 里的 hidden/userInvocable 判断在交互路径上不可达。作为导出 helper 的防御性代码没问题——只是说明（按 bot 要求补充的）相应测试覆盖的是产品不会产生的状态。
8. 测试说明：' /sto' 钉住的是路由而非结果
使用真实 useSlashCompletion 时，缩进的行首命令得到的候选数为 0：useCommandParser 执行 query.substring(1)，' /sto' → partial = '/sto'，fzf 无法匹配任何命令名。此时 mid-input ghost text 仍会触发（store-notes）。base 上完全相同，isInitialCommandOnFirstLine 守卫正确地保持了这一行为——但该测试只断言了 useSlashCompletion 的调用参数，即使那个下拉框永远为空它也会通过。加一行"断言的是路由而非候选"的注释可以避免后来者误读。
总体很好——SlashCompletionContext 的三态拆分比原来的布尔值可读性强很多，isValidStackedSkillPrefix 的抽取让补全过滤与执行端规则对齐，而 max-stack 边界是最容易差一位的地方，这里是正确的。
|
Thanks for the careful pass. I agreed with the main consistency issue, so I kept the follow-up small and pushed What changed:
I also updated the PR body:
I intentionally left two points out of this patch:
Checks run after syncing with the current PR head: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| export const isStackedSkillCompletableCommand = ( | ||
| command: SlashCommand, | ||
| ): boolean => | ||
| command.kind === CommandKind.SKILL && | ||
| command.userInvocable !== false && | ||
| !command.hidden; |
There was a problem hiding this comment.
[Suggestion] Missing negative test coverage in completion dropdown — the stacked-skill dropdown test only exercises the kind === SKILL filter. The userInvocable !== false and !hidden conditions are never verified as exclusion criteria at the dropdown level (they are tested in isValidStackedSkillPrefix unit tests, but that's a different consumer).
Failure scenario: if !command.hidden were dropped from this function, hidden skills would appear in the stacked-skill dropdown with no test catching it.
Consider adding a test in useCommandCompletion.test.ts that includes a hidden: true skill and a userInvocable: false skill in the command list and asserts neither appears in the slashCommands array passed to useSlashCompletion.
— qwen3.7-max via Qwen Code /review
| return tokens.every((token) => { | ||
| if (!token.startsWith('/')) return false; | ||
| const command = findCommandByName(token.slice(1), commands); | ||
| return command !== undefined && isStackedSkillCompletableCommand(command); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Case-sensitivity inconsistency — findCommandByName uses exact string match (cmd.name === name), but findSlashCommandTokens resolves tokens case-insensitively via commandMap.get(commandName.toLowerCase()).
Failure scenario: user types /Review /bugfix (mixed case). The highlighting path marks /Review as valid (green chip, case-insensitive lookup), but isValidStackedSkillPrefix fails to resolve Review (case-sensitive), so /bugfix is marked invalid (red chip). The dropdown also skips the STACKED_SKILL context, showing all commands instead of skills-only.
Consider making findCommandByName case-insensitive (lowercase both sides) to match the convention used by findSlashCommandTokens and getBestSlashCommandMatch.
— qwen3.7-max via Qwen Code /review
Re-verification (R2) — head
|
| # | R1 finding | Status | Evidence |
|---|---|---|---|
| 1 | Same-line stacked disable-model-invocation skill offered but unhighlighted |
Fixed (same-line); residual for indented continuation → R2-3 | highlight A/B below |
| 2 | Two extra full-buffer code-point scans per render | Stands, unchanged | re-measured: 6 calls / 600 K chars (slash-led), 5 / 500 K (plain) — identical to R1 |
| 3 | isSlashLedInput should use isSlashCommand() |
Fixed | 3 inputs move from IDLE/no suggestions → working mid-input completion |
| 4 | Leading-blank-line change undocumented | Fixed | now in Risk & Scope |
| 5 | Stack's own members offered again | Stands | re-confirmed /review /review → EXEC=STACKED skills=[review,review]; deferring as a product decision is reasonable |
| 6 | Dead token.length === 1 clause |
Fixed | dropped |
| 7 | hidden/userInvocable unreachable in TUI |
Stands (informational) | — |
| 8 | ' /sto' test pins routing, not outcome |
Fixed | comment added |
Deferring 2 and 5 is the right call — 2 changes the findMidInputSlashCommand return contract, 5 changes what /review /review means.
What I ran
| Check | Result |
|---|---|
| 5 focused test files | 171/171 pass (matches the PR body) |
Wider sweep src/ui/hooks src/ui/utils src/utils |
3347 tests pass, 0 test failures; 2 files fail to collect (@qwen-code/web-templates entry unbuilt) — worktree artifact, not this diff |
tsc --noEmit (packages/cli), A/B vs merge-base 3d395606a |
97 errors at head, 97 at merge-base, set difference empty; none in this PR's files |
prettier --check on the 6 changed files |
clean |
eslint on the 6 changed files |
clean — and I planted 3 errors to prove eslint was actually live, not silently no-op'ing |
Highlight A/B (real parseInputForHighlighting, per logical line as InputPrompt calls it) vs real execution oracle (submit gate copied from slashCommandProcessor.ts:829-899), 24 inputs |
see below |
Hook A/B (real useSlashCompletion + real TextBuffer), 13 inputs |
see below |
Mutation matrix, 6 mutations targeting only 06a376db |
5 killed, 1 survivor (R2-2) |
Perf ladder on findSlashCommandTokens, base vs head |
R2-1 |
Finding 1 is fixed, and the A/B against 283f4a9fb (the commit before the highlight fix) is narrow — exactly 9 rows change, every one of them adding a highlight for a disable-model-invocation skill token:
input before 06a376db at head EXEC (real submit path)
/review /store-locally [/review] -> [/review, /store-locally] STACKED skills=[review,store-locally]
/review /store-locally [/review] -> [/review, /store-locally] STACKED skills=[review,store-locally]
/review\t/store-locally [/review] -> [/review, /store-locally] STACKED skills=[review,store-locally]
/review /store-locally tail text [/review] -> [/review, /store-locally] STACKED ... rest="tail text"
\n/review /store-locally [/review] -> [/review, /store-locally] STACKED skills=[review,store-locally]
\n\n/review /store-locally [/review] -> [/review, /store-locally] STACKED skills=[review,store-locally]
/review /store-locally /front-end-... [/review, /front-...] -> [/review, /store-locally, /front-...] STACKED skills=[review,store-locally,front-end-store-rules]
context\n/review /store-locally [/review] -> [/review, /store-locally] PROMPT (not a slash command) <-- R2-4
hello world\n/review /store-locally [/review] -> [/review, /store-locally] PROMPT (not a slash command) <-- R2-4
Seven of the nine are strictly correct. The MAX_STACKED_SKILLS boundary lines up with execution too: for /s1 /s2 /s3 /s4 /s5 /s6, a 5-token prefix makes token 6 invalid, which matches parseStackedSlashCommands treating it as remainingText.
The hook A/B isolates finding 3's fix cleanly — three inputs and nothing else:
/tmp/foo.txt please /sto IDLE, no suggestions -> SLASH, sugg=[store-notes, front-end-store-rules]
// note /sto IDLE, no suggestions -> SLASH, sugg=[store-notes, front-end-store-rules]
/* block /sto IDLE, no suggestions -> SLASH, sugg=[store-notes, front-end-store-rules]
Everything else held: /review /sto and /review\n/sto → [store-locally, front-end-store-rules]; /stats /sto, /unknown /sto, /review please /sto → []; please /sto → [store-notes, front-end-store-rules]; /sto → empty dropdown + ghost re-notes.
Mutations correctly killed: dropping isStackedSkillCompletableCommand(cmd); dropping the modelInvocable disjunct; reverting finding 3's fix back to startsWith('/'); forcing isValidStackedSkillPrefix false; max-stack >=→>. Each died on a test whose name matches what it broke.
R2-1. Quadratic blow-up in findSlashCommandTokens for disable-model-invocation skills
commandUtils.ts:405 computes the prefix per token:
const prefix = text.slice(0, start);
valid = cmd.modelInvocable === true ||
(isStackedSkillCompletableCommand(cmd) && isValidStackedSkillPrefix(prefix, commands));The slice itself is free — V8 makes it a SlicedString. The cost lands when isValidStackedSkillPrefix calls .trim() and .split(/\s+/), which flatten it. So every token that is a stackable skill and not model-invocable pays an O(line) copy plus a full re-split. Measured on one logical line of prose + repeated /store-locally, ms per findSlashCommandTokens call:
| tokens | line length | before 06a376db |
at head |
|---|---|---|---|
| 100 | 9.6 KB | 0.04 | 2.64 |
| 200 | 19 KB | 0.09 | 10.55 |
| 400 | 38 KB | 0.17 | 41.99 |
| 800 | 77 KB | 0.36 | 167.36 |
Doubling the token count roughly quadruples the time. The control pins the trigger exactly — same lines with /review (model-invocable, so the rule short-circuits before the flatten) are identical on both sides (0.04 / 0.15 / 0.30 ms at 100 / 400 / 800). So this is specific to skills with disable-model-invocation: true — which is precisely the skill kind this PR's feature exists to serve.
It matters because InputPrompt calls parseInputForHighlighting once per rendered visual row, re-parsing the whole logical line each time, so the per-call number is multiplied by the visible rows of that line on every keystroke.
Reaching it needs a user-authored disable-model-invocation skill whose name repeats ~100+ times in a single logical line, so I would not call it likely. I am raising it because the fix is cheap and I verified it: the predicate is monotone — once a prefix token isn't a stackable skill, no longer prefix can be valid — so it collapses to one left-to-right pass:
// before the loop
let runOk = true, runCount = 0, lastEnd = 0;
// inside the loop, replacing the mid-input branch
if (runOk && text.slice(lastEnd, start).trim().length > 0) runOk = false;
valid = cmd.modelInvocable === true ||
(isStackedSkillCompletableCommand(cmd) &&
runOk && runCount > 0 && runCount < MAX_STACKED_SKILLS);
if (runOk && isStackedSkillCompletableCommand(cmd)) runCount++; else runOk = false;
lastEnd = start + fullMatch.length;I differential-tested this against the shipped version over 8496 inputs — exhaustive 2- and 3-token combinations across 17 command names (skills, non-skills, hidden, non-user-invocable, aliases, unknown, /123, empty) × 8 separators (spaces, tabs, newlines, empty, , , x), plus 4000 seeded pseudo-random multi-token inputs — and got 0 mismatches on the per-token validity vector. Perf after: 158.69 ms → 0.358 ms at 800 tokens, same results at every rung.
(runOk must also be cleared when cmd is undefined, which the else branch above handles.)
R2-2. The new rule's central conjunct is untested — mutation survivor
Deleting isValidStackedSkillPrefix(prefix, commands) from the highlight rule, so any user-invocable non-hidden skill becomes valid mid-input regardless of what precedes it, leaves 171/171 tests green. It is the one survivor in the R2 matrix.
That is exactly the claim 06a376db's message makes ("...when they follow a valid stacked-skill prefix"), and the two new tests don't pin it: /review-skill /store-locally has a valid prefix, and /review-skill /clear is killed by the isStackedSkillCompletableCommand conjunct instead (/clear is BUILT_IN). Neither exercises a stackable skill after an invalid prefix.
The mutant is not vacuous — 9 inputs diverge observably. On 6 of them the mutant is plainly wrong, highlighting a token whose prefix is not a stacked-skill run:
please /store-locally head: [] mutant: [/store-locally]
/stats /store-locally head: [/stats] mutant: [/stats, /store-locally]
/unknown /store-locally head: [] mutant: [/store-locally]
/review arg /store-locally head: [/review] mutant: [/review, /store-locally]
please /review /store-locally head: [/review] mutant: [/review, /store-locally]
/tmp/foo.txt /store-locally head: [] mutant: [/store-locally]
On the remaining 3 — /review\n /store-locally, /review\n\t/store-locally, /review\n /store-locally — the mutant coincidentally lands on the right answer, because those really do execute as stacked skills. That's R2-3 below, and it's why an over-broad rule can't be distinguished from the correct one by the current suite.
One test closes it — a disable-model-invocation skill after a non-skill prefix must stay invalid:
const tokens = findSlashCommandTokens('/clear /store-locally', commands); // or 'please /store-locally'
expect(tokens[1]).toMatchObject({ commandName: 'store-locally', valid: false });R2-3. Residual of finding 1: indented continuation lines still diverge
Not changed by this PR (identical on both sides), but it is the leftover half of finding 1 and worth stating as a known limitation rather than leaving implied:
/review\n /store-locally HL=[/review] (2nd token plain) EXEC=STACKED skills=[review,store-locally]
/review\n\t/store-locally HL=[/review] EXEC=STACKED skills=[review,store-locally]
And the hook probe shows completion does serve that form — /review\n /sto → [store-locally, front-end-store-rules], with store-locally top — so accepting the top suggestion again lands on a token that executes but renders as plain text.
The cause is structural: InputPrompt.tsx:1877 calls parseInputForHighlighting(logicalLine, …), so findSlashCommandTokens only ever sees one logical line and can never observe a cross-line prefix. isLineStart (start === 0 || precedingChar === '\n') covers the unindented continuation, which is why /review\n/store-locally already worked. Fixing the indented case needs the previous line's state threaded in, so it's fair to leave out — just say so.
R2-4. Pre-existing: highlighting ignores the logical line index (amplified here)
context\n/review /store-locally HL=[/review, /store-locally] EXEC=PROMPT (not a slash command)
Both tokens render as commands, but the buffer submits as a plain prompt. parseInputForHighlighting takes an index (logical line number) and its no-metadata fallback correctly restricts to index === 0:
} else if (match[1] !== undefined) {
// Group 1: line-start slash command — only highlight on logical line 0
type = index === 0 ? 'command' : 'default';…but the slashCommands branch — the one InputPrompt always takes — ignores index entirely, and findSlashCommandTokens isn't given it. So a /cmd at the start of any line reads as a command.
Base already mis-highlighted /review here, so the bug is not this PR's; head extends it from one token to every stacked token on that line. Arguably head is now self-consistent (whole line highlighted) where base was half-and-half. Worth a follow-up issue against parseInputForHighlighting rather than anything in this diff.
R2-5. Nit — two spellings of the same predicate
useCommandCompletion.tsx:170 now reads isSlashCommand(prefix.trimStart()) (finding 3's fix), but the ghost-text guard at line 381 still reads prefix.trimStart().startsWith('/'). I checked whether that matters: switching line 381 to isSlashCommand too produced byte-identical hook-probe output across all 13 inputs, so there is no behavioural difference to fix — the routing memo already returns before the ghost memo can see a divergent case. Purely a readability point: one concept, two spellings, in one function.
Good follow-up. The highlight alignment is the right call and the A/B shows it is narrowly scoped — six of the eight changed rows are strictly correct, and the MAX_STACKED_SKILLS boundary agrees with parseStackedSlashCommands on both the completion and the highlight side, which is the part that was easiest to get wrong. R2-1 and R2-2 are both small and I've verified the fix for each; R2-3 and R2-4 are fine as follow-ups.
中文说明
复验（R2）— head 4543bf0b
接续我的 R1 评审（head e4855cf8）。此后的增量为 06a376db 加两次 main 合并；PR 自身改动为 5 个文件，生产代码仅 3 行（commandUtils.ts 高亮规则、useCommandCompletion.tsx 判定、commands.ts 删除死代码）。
结论：仍无阻断问题。 发现 1、3、4、6、8 已确实修复——每一项我都重跑验证，而非采信回复。在探测高亮改动时新增两项：一个平方级性能回归（R2-1，已给出经等价性验证的 3 行修法），以及新规则的核心判定条件缺少测试覆盖（R2-2，变异测试证实）。
既有发现 → 在 4543bf0b 的状态
| # | R1 发现 | 状态 | 证据 |
|---|---|---|---|
| 1 | 同行 stacked disable-model-invocation skill 被推荐但不高亮 |
已修复（同行）；缩进续行仍有残留 → R2-3 | 见下方高亮 A/B |
| 2 | 每次渲染多两次全 buffer code-point 扫描 | 仍存在，未变化 | 重新测量：6 次 / 60 万字符（斜杠开头）、5 次 / 50 万字符（普通）——与 R1 完全一致 |
| 3 | isSlashLedInput 应使用 isSlashCommand() |
已修复 | 3 个输入从 IDLE／无候选变为正常 mid-input 补全 |
| 4 | 开头空行的行为变更未说明 | 已修复 | 已写入 Risk & Scope |
| 5 | 栈内已有 skill 会被再次推荐 | 仍存在 | 重新确认 /review /review → EXEC=STACKED skills=[review,review]；作为产品决策另行处理是合理的 |
| 6 | token.length === 1 死代码 |
已修复 | 已删除 |
| 7 | hidden/userInvocable 在 TUI 中不可达 |
仍存在（仅说明性） | — |
| 8 | ' /sto' 测试钉住的是路由而非结果 |
已修复 | 已加注释 |
将 2 和 5 留作后续是正确的：2 会改变 findMidInputSlashCommand 的返回契约，5 会改变 /review /review 的语义。
我实际运行的检查
- 5 个重点测试文件：171/171 通过（与 PR 正文一致）
- 更大范围
src/ui/hooks src/ui/utils src/utils：3347 个测试通过，0 个测试失败；2 个文件收集失败（@qwen-code/web-templates入口未构建）——worktree 环境问题，与本 diff 无关 tsc --noEmit（packages/cli），与 merge-base3d395606a做 A/B：head 97 个错误，merge-base 97 个，集合差为空；本 PR 涉及的文件无任何错误- 6 个改动文件的
prettier --check：干净 - 6 个改动文件的
eslint：干净——并植入 3 个错误作为正向对照，证明 eslint 确实在工作而非静默空跑 - 高亮 A/B（真实
parseInputForHighlighting，按逻辑行调用，与InputPrompt一致）对比真实执行 oracle（提交门禁抄自slashCommandProcessor.ts:829-899），30 个输入 - Hook A/B（真实
useSlashCompletion+ 真实TextBuffer），13 个输入 - 变异矩阵，6 个变异，仅针对
06a376db：5 个被捕获，1 个存活（R2-2） - 性能梯度测量
findSlashCommandTokens，base 对比 head：见 R2-1
发现 1 已修复，且与 283f4a9fb（高亮修复前的 commit）的 A/B 范围很窄——恰好 9 行发生变化，每一行都是为 disable-model-invocation skill token 新增高亮。其中 7 行完全正确，另 2 行即 R2-4。MAX_STACKED_SKILLS 边界与执行端也一致：对 /s1 … /s6，5 token 的前缀会使第 6 个 token 判为 invalid，这与 parseStackedSlashCommands 把它当作 remainingText 的行为吻合。
Hook A/B 干净地隔离出发现 3 的修复效果——只有三个输入发生变化：/tmp/foo.txt please /sto、// note /sto、/* block /sto 均从 IDLE／无候选变为 SLASH + [store-notes, front-end-store-rules]。其余全部保持：/review /sto 与 /review\n/sto → [store-locally, front-end-store-rules]；/stats /sto、/unknown /sto、/review please /sto → []；please /sto → [store-notes, front-end-store-rules]； /sto → 空下拉 + ghost re-notes。
被正确捕获的变异：去掉 isStackedSkillCompletableCommand(cmd)；去掉 modelInvocable 分支；把发现 3 的修复回退为 startsWith('/')；强制 isValidStackedSkillPrefix 返回 false；max-stack >=→>。每个变异致死的测试名称都与其破坏的行为对应。
R2-1. findSlashCommandTokens 对 disable-model-invocation skill 出现平方级性能劣化
commandUtils.ts:405 每个 token 都重新计算前缀。slice 本身是免费的（V8 生成 SlicedString），代价出现在 isValidStackedSkillPrefix 调用 .trim() 和 .split(/\s+/) 将其展平时。因此每个「是可 stack 的 skill 且非 model-invocable」的 token 都要付一次 O(行长) 拷贝加一次完整重新切分。在「散文 + 重复 /store-locally」的单逻辑行上测得每次 findSlashCommandTokens 调用耗时：
| token 数 | 行长 | 06a376db 之前 |
head |
|---|---|---|---|
| 100 | 9.6 KB | 0.04 | 2.64 |
| 200 | 19 KB | 0.09 | 10.55 |
| 400 | 38 KB | 0.17 | 41.99 |
| 800 | 77 KB | 0.36 | 167.36 |
token 数翻倍，耗时约变四倍。对照组精确锁定了触发条件——同样的行改用 /review（model-invocable，规则在展平前短路）时两侧完全相同（100/400/800 分别为 0.04/0.15/0.30 ms）。所以这只影响带 disable-model-invocation: true 的 skill，而这恰恰是本 PR 功能所服务的那类 skill。
之所以值得关注：InputPrompt 每渲染一个可视行就调用一次 parseInputForHighlighting，每次都重新解析整条逻辑行，因此上表数值会乘以该行的可见行数，且发生在每次按键上。
要触发它，需要用户自建的 disable-model-invocation skill 名称在单条逻辑行中重复约 100 次以上，因此我不认为这很常见。我提出它是因为修法很便宜且我已验证：该判定是单调的——一旦前缀中出现非可 stack skill 的 token，任何更长的前缀都不可能合法——因此可以收敛为一次从左到右的遍历（维护 runOk / runCount / lastEnd 三个变量，见英文版代码）。
我将该写法与线上版本做了差分测试，覆盖 8496 个输入——17 个命令名（skill、非 skill、hidden、非 user-invocable、别名、未知、/123、空）与 8 种分隔符（空格、制表符、换行、空串、, 、x）的 2-token 与 3-token 穷举组合，外加 4000 个固定种子的伪随机多 token 输入——逐 token 的 validity 向量零不一致。修复后性能：800 token 从 158.69 ms 降至 0.358 ms，各档结果一致。
R2-2. 新规则的核心判定条件无测试覆盖——变异存活
从高亮规则中删除 isValidStackedSkillPrefix(prefix, commands)，即让任何 user-invocable 且非 hidden 的 skill 在 mid-input 位置一律判为 valid（无论其前文是什么），171/171 测试仍全绿。这是 R2 矩阵中唯一存活的变异。
而这恰恰是 06a376db commit message 所声称的内容（"当它们跟在合法 stacked-skill 前缀之后"）。两个新测试并未钉住它：/review-skill /store-locally 的前缀是合法的，而 /review-skill /clear 是被 isStackedSkillCompletableCommand 这一条件杀掉的（/clear 是 BUILT_IN）。两者都没有覆盖「可 stack 的 skill 跟在非法前缀之后」。
该变异并非空变异——有 9 个输入可观测地发生分歧。其中 6 个变异明显错误（高亮了前缀并非 stacked-skill 序列的 token）：please /store-locally、/stats /store-locally、/unknown /store-locally、/review arg /store-locally、please /review /store-locally、/tmp/foo.txt /store-locally。剩下 3 个（/review\n /store-locally 及其制表符／多空格变体）变异恰好给出了正确答案，因为它们确实会作为 stacked skills 执行——这正是下面的 R2-3，也说明现有测试集无法把「过宽的规则」和「正确的规则」区分开。
补一个测试即可闭合：findSlashCommandTokens('/clear /store-locally', commands)（或 'please /store-locally'）的第二个 token 应为 valid: false。
R2-3. 发现 1 的残留：缩进续行仍不一致
本 PR 未改变该行为（两侧一致），但它是发现 1 未覆盖的另一半，值得明确写为已知限制：
/review\n /store-locally HL=[/review]（第二个 token 为普通文本） EXEC=STACKED skills=[review,store-locally]
/review\n\t/store-locally HL=[/review] EXEC=STACKED skills=[review,store-locally]
而 hook 探测显示补全确实服务这种写法——/review\n /sto → [store-locally, front-end-store-rules]，且 store-locally 排首位——所以接受首选候选后，又会落到一个「能执行但显示为普通文本」的 token 上。
根因是结构性的：InputPrompt.tsx:1877 调用 parseInputForHighlighting(logicalLine, …)，因此 findSlashCommandTokens 只能看到单条逻辑行，永远无法观察跨行前缀。isLineStart（start === 0 || precedingChar === '\n'）覆盖了未缩进的续行，这也是 /review\n/store-locally 早已正常的原因。修复缩进情形需要把上一行的状态传入，因此不纳入本 PR 是合理的——只要说明即可。
R2-4. 既有问题：高亮忽略逻辑行号（本 PR 放大了它）
context\n/review /store-locally HL=[/review, /store-locally] EXEC=PROMPT（不是 slash command）
两个 token 都渲染为命令，但该 buffer 会作为普通 prompt 提交。parseInputForHighlighting 接收 index（逻辑行号），其「无命令元数据」的回退分支正确地限制了 index === 0；但 InputPrompt 始终走的 slashCommands 分支完全忽略 index，而 findSlashCommandTokens 也拿不到它。于是任意行首的 /cmd 都会被当作命令。
base 上 /review 本就被错误高亮，所以这不是本 PR 引入的；head 只是把它从一个 token 扩展到该行所有 stacked token。某种意义上 head 反而自洽了（整行统一高亮），而 base 是半高亮。建议针对 parseInputForHighlighting 另开 issue，而非在本 diff 内处理。
R2-5. 小问题——同一概念的两种写法
useCommandCompletion.tsx:170 现在写作 isSlashCommand(prefix.trimStart())（发现 3 的修复），但第 381 行的 ghost-text 守卫仍是 prefix.trimStart().startsWith('/')。我验证过这是否有影响：把第 381 行也改为 isSlashCommand 后，13 个输入的 hook 探测输出逐字节相同，因此不存在需要修的行为差异——路由 memo 会在 ghost memo 能观察到分歧之前就返回。这纯粹是可读性问题：同一个概念，在同一个函数里有两种写法。
这个 follow-up 做得不错。高亮对齐方向正确，A/B 也显示改动范围收敛——8 个变化行中 6 个完全正确，且 MAX_STACKED_SKILLS 边界在补全侧和高亮侧都与 parseStackedSlashCommands 一致，而这是最容易出错的部分。R2-1 和 R2-2 都很小，且我已分别验证过修法；R2-3 与 R2-4 留作后续即可。
|
Thanks, pushed a small follow-up in It adds negative coverage for the stacked-skill dropdown filter ( I left the case-sensitivity question, the longer-line perf optimization, and the indented continuation highlighting behavior out of this PR to keep the fix scoped to #7717. Happy to follow up separately if maintainers want to standardize those. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Independent local verification report (head
|
| Scenario | Observed |
|---|---|
/review /str (same line) |
Suggestion dropdown appears (fuzzy skill match in this environment's skill set) |
| Tab on the suggestion | Input becomes /review /extension-creator — only the second token replaced, the stacked prefix kept |
/stats /ext (prefix is a built-in, not a skill) |
No stacked suggestions — invalid prefix correctly refused |
/review ⏎ /ext (multi-line stack) |
Suggestion dropdown appears — the cross-line form works |
please /ext |
No dropdown — correct here, since mid-input offers only model-invocable commands and this environment's matching skill isn't one; the mid-input regression surface is covered by the unit suites below |
2. Code review notes
- The dangerous-looking removal of
if (input.startsWith('/')) return nullfromfindMidInputSlashCommandis compensated at both consumers: the hook classifies slash-led buffers intoLINE_START/STACKED_SKILLand refuses ghost text for any other slash-led prefix (isSlashCommand(prefix.trimStart()) → return null), andfindSlashCommandTokensonly marks a mid-token valid when the prefix passesisValidStackedSkillPrefix. Invalid slash-led input can reach neither stacked completion nor ghost text — the PR's stated invariant holds at both read sites. isValidStackedSkillPrefixenforces every prior token to be a user-invocable, non-hidden skill and respectsMAX_STACKED_SKILLS— the completion path reuses the execution path's own vocabulary (isStackedSkillCompletableCommand), so what completes is what will run.- Highlighting and completion share the same predicate pair, keeping the "highlighted as valid ⇔ would execute" alignment the PR claims.
3. Tests
Focused suites on this branch: 173/173 passed (commands, commandUtils, useCommandCompletion, useSlashCompletion ×2 — includes the stacked-guard additions from 26e74c3).
Conclusion
Thesis: the old detector treated a slash-led buffer as one line-start query, so the second token of a stack was invisible to completion; this PR fixes it by classifying the context first and only then choosing the candidate set. Evidence: before/after in the real TUI including the Tab-replacement semantics and the non-skill-prefix refusal (§1), both compensating guards located at the exact read sites (§2), and green focused suites (§3). LGTM.
中文摘要
Linux 真机 TUI 验证（构建本分支 bundle）：基线 0.20.1 对 /review /str 无任何建议；补丁版同行与跨行栈式补全均出下拉，Tab 只替换第二个 token 且保留前缀，/stats /ext（前缀非技能）正确拒绝。代码层面：删掉 startsWith('/') 早退的风险在两个消费点均有补偿守卫（hook 的三上下文分类 + ghost text 对 slash 前缀的拒绝；token 高亮复用同一谓词），"无效 slash 输入既不进栈式补全也不进 ghost text"的不变式在两个读点都成立。聚焦套件 173/173。LGTM。
— independent review loop, real-TUI verification on Linux
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Re-verification (R3) — real local build, before/after TUI framesFollow-up to R1 (head I ran everything at Verdict: merge-ready. Both R2 findings are resolved as far as this PR is concerned: the mutation survivor is now killed by exactly the test CI on the current head was still running when I posted this, so the verdict is about the code, not the check marks. Delta since R2 is one commit, What I ran
The full-suite failures are local environment only (this worktree shares a sibling worktree's Before / after, from the real completion stackNothing in the completion path is mocked: real (Caveat, stated plainly: I render the production dropdown and highlighter, but I do not mount The fix — same line:
The fix — across logical lines:
Highlight now agrees with execution. This is R1 finding 1, and it is the part I most wanted to see rendered rather than asserted —
The cap is respected — six complete skill tokens, tokens 1–5 highlight, the 6th stays plain:
Nothing else moved — these four frames are byte-identical on both arms:
The highlight/execution oracle, driving The sixth-token row is the one worth noting: highlight marks exactly five tokens valid and the sixth plain, and execution stacks exactly five and pushes R2 finding 1 (mutation survivor) — closedR2's central complaint was that deleting the Full matrix (baseline reports 0 failures; the positive control kills 14, so the harness is live):
M11 is not a coverage gap. New in R3 — two coverage gaps, both minor1. The R3 delta's only production line is untested.
The window is narrow but reachable: it needs the first token to be slash-led yet not a slash command (path or comment), an exact model-invocable token mid-text, and an 2.
Head is right; only the guard is untested. One case in the existing 3. Note, not a finding. The new The deferred perf item, quantified at headYou explicitly left this out of the PR and I agree with the call, but the numbers should be on the record for whoever merges. Same process, same machine; the only delta is the highlight rule:
It matters more than the per-call numbers suggest because Reproduction# focused suite
npm run test --workspace=packages/cli -- \
src/utils/commands.test.ts src/ui/utils/commandUtils.test.ts \
src/ui/hooks/useCommandCompletion.test.ts src/ui/hooks/useSlashCompletion.test.ts \
src/ui/hooks/useSlashCompletion.integration.test.ts --coverage=false
# -> 173 passed (173)Everything else was run in two detached worktrees at 中文复核（R3）—— 本地真实构建 + 前后对比 TUI 截图接续 R1（head 我在 结论：可以合并。 R2 的两项发现就本 PR 而言都已了结：变异存活者现在正好被 发帖时当前 head 的 CI 仍在运行，因此这个结论针对代码本身，而非 CI 勾选状态。 R2 之后的增量只有一个提交 我跑了什么
全量套件的失败纯属本地环境（该 worktree 共用了同级 worktree 的 前后对比：来自真实补全栈补全链路上没有任何 mock：真实 （如实说明局限：我渲染的是生产的下拉组件与高亮器，但没有挂载 上面 5 张图依次是：同行栈式补全、跨逻辑行栈式补全、高亮与执行对齐（R1 发现 1， 第六个 token 那一行值得一提：高亮恰好标 5 个 token 有效、第 6 个为普通文本；执行也恰好栈 5 个并把 R2 发现 1（变异存活者）—— 已了结R2 的核心意见是：从高亮规则里删掉 完整矩阵见上表（基线 0 失败；正对照杀掉 14 条，证明这套装置是活的）。 M11 不是覆盖缺口。 R3 新增 —— 两个较小的覆盖缺口1. R3 增量里唯一那行生产代码没有测试锚定。
触发窗口很窄但可达：需要首 token 以斜杠开头却不是斜杠命令（路径或注释），文中有一个精确匹配的 model-invocable token，且该命令带 2.
head 的行为是对的，只是守卫没测试。在现有 3. 说明，不算发现。 新增的 延后的性能项：在 head 上量化你明确把这项排除在本 PR 之外，我同意这个判断，但数据应当留档以供合并者参考。同一进程、同一机器，唯一变量是高亮规则（数据见上图）：
它的影响比单次调用数字更大，因为 复现npm run test --workspace=packages/cli -- \
src/utils/commands.test.ts src/ui/utils/commandUtils.test.ts \
src/ui/hooks/useCommandCompletion.test.ts src/ui/hooks/useSlashCompletion.test.ts \
src/ui/hooks/useSlashCompletion.integration.test.ts --coverage=false
# -> 173 passed (173)其余检查都在两个 detached worktree 中进行： |
5 participants
What this PR does
Restores skill completion for later tokens in a stacked invocation, both on one line and across lines:
The completion hook now distinguishes three cases that previously shared the same mid-input path:
Only valid skill prefixes enter stacked completion, and only skills are offered there. Invalid slash-led input does not fall through to mid-input ghost text.
This also keeps input highlighting aligned with the execution path: same-line stacked skill tokens are highlighted as valid commands when they follow a valid stacked-skill prefix, including skills with
disable-model-invocation.Why it's needed
PR #5898 added mid-input completion for prompts such as
please /review /sto. PR #6361 later added stacked leading skills, but the completion detector still treated the whole first line as one slash query and stopped scanning whenever the buffer started with/.Reviewer Test Plan
How to verify
With
reviewandfront-end-store-rulesavailable as skills:/review /sto, then accept the suggestion with Tab./review, add a new line with/sto, then accept the suggestion with Tab./stats /stoand/unknown /stodo not offerstoas a stacked skill.please /stostill works.Focused checks run locally:
Prettier, ESLint, and
git diff --checkalso pass for the changed files.Evidence (Before & After)
Before:
After accepting the matching suggestion:
The hook tests exercise the actual replacement for both forms.
Tested on
Environment (optional)
Node.js 24.18.0, npm 11.16.0.
Risk & Scope
isSlashCommand()classifies as non-commands, such as path-like text or comments, still allow normal mid-input completion after them.Linked Issues
Fixes #7717
中文说明
本 PR 做了什么
恢复 stacked skill 后续 token 的自动补全，同时覆盖同一行和跨行两种形式：
补全逻辑现在会区分三种场景：行首 slash command、普通 prompt 中可由模型调用的命令，以及已有 skill 后面继续输入的 user-invocable skill。只有合法的 skill 前缀会进入 stacked completion，候选中也只包含 skill；无效的 slash 前缀不会再旁路到 mid-input ghost text。
同时，输入高亮现在也和执行路径保持一致：同一行 stacked skill token 只要位于合法 stacked-skill 前缀之后，就会按有效命令高亮，包括带
disable-model-invocation的 skill。为什么需要
PR #5898 为
please /review /sto这类输入增加了 mid-input completion。PR #6361 后来支持 stacked skills，但补全检测仍会把整个首行当成一个 slash query，并且只要 buffer 以/开头就停止扫描后续 token。Reviewer 测试计划
如何验证
准备
review和front-end-store-rules两个 skill：/review /sto，按 Tab 接受候选。/review，换行后输入/sto，再按 Tab 接受候选。/stats /sto和/unknown /sto不会把sto当成 stacked skill 补全。please /sto这样的普通 mid-input completion 仍然正常。本地检查结果：5 个相关测试文件、171 个测试全部通过；CLI typecheck、Prettier、ESLint 和
git diff --check也全部通过。风险与范围
isSlashCommand()判定为非命令的 slash-led 前缀，例如路径文本或注释，后续仍可触发普通 mid-input completion。关联 Issue
Fixes #7717