fix(cli): wire ACP model-invocable commands - #5504
Conversation
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @tt-a1i, thanks for the fix — the bug itself (ACP snapshots missing model-invocable command wiring, issue #5503) is real and worth addressing.
However, the PR body doesn't follow our pull request template. The current headings are ## Summary, ## Testing, and ## AI Assistance Disclosure, but the template requires:
## What this PR does— describe the change in prose## Why it's needed— motivation and user-facing impact## Reviewer Test Plan— with How to verify, Evidence (Before & After), and Tested on (OS table)## Risk & Scope— main risk, what's out of scope, breaking changes## Linked Issues—Fixes #5503
Could you update the PR description to match the template? It helps reviewers (and future-us) understand the context without reading every diff line. Happy to re-run triage once that's done.
中文说明
你好 @tt-a1i,感谢提交修复——#5503 描述的 bug(ACP 快照缺少 model-invocable command 注册)确实存在且值得修复。
不过 PR 正文没有使用我们的PR 模板。当前的标题是 ## Summary、## Testing 和 ## AI Assistance Disclosure,但模板要求的是:
## What this PR does— 用文字描述改动## Why it's needed— 动机和用户影响## Reviewer Test Plan— 包含 How to verify(验证步骤)、Evidence(前后对比)、Tested on(操作系统表格)## Risk & Scope— 主要风险、不在范围内的内容、破坏性变更## Linked Issues—Fixes #5503
能否按照模板更新 PR 描述?这样 reviewer(和未来的我们)不需要逐行看 diff 就能理解上下文。更新后可以重新触发 triage。
— Qwen Code · qwen3.7-max
|
updated the PR description to match the template.\n\n@qwen-code /triage |
|
updated the PR description to match the template. @qwen-code /triage |
|
@qwen-code /triage |
✅ Local end-to-end verification (real command pipeline, tmux)Verified by driving the real exported
A/B result — each probe maps to a PR bullet
In ACP mode the command is loaded on both builds, but BASE never registers the provider/executor, so Unit test + revert-proof + static
Note
Verdict: LGTM — ACP now wires model-invocable commands so the model can actually invoke them via SkillTool, with disabled-command exclusion, a fresh hook signal, and no partial registration. 🇨🇳 中文版本(点击展开)✅ 本地端到端验证(真实命令管线,tmux)通过驱动真实导出的
A/B 结果 —— 每个探针对应一个 PR 改动点
在 ACP 模式下命令在两个版本都能被加载,但 BASE 从不注册 provider/executor,因此 单元测试 + 反向验证 + 静态检查
补充说明
结论:LGTM —— ACP 现在正确接线了 model-invocable 命令,使模型能真正通过 SkillTool 调用它们,并具备禁用命令排除、全新 hook 信号、且不做部分注册。 |
|
Thanks for the PR! Template looks good ✓ — all required headings present, bilingual summary included, test plan is specific with commands. On direction: this is a straightforward bug fix — ACP's On approach: the scope is tight and well-targeted. Two focused changes:
The two-CommandService approach in Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ — 所有必需标题齐全,双语说明包含,测试计划具体且包含可执行命令。 方向:这是一个直接的 bug 修复 — ACP 的 方案:范围紧凑且目标明确。两个聚焦改动:
进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): Comparison with the diff: No critical blockers or AGENTS.md violations found. The code is focused, idiomatic, and follows project conventions. Test ResultsUnit Tests (worktree)New tests cover all four facets of the fix:
Static ChecksReal-Scenario Test (tmux, dev build)CLI starts cleanly and responds to prompts with the PR changes applied. Note: the ACP-specific bug (model-invocable commands not registered in ACP snapshot path) requires an ACP server for full E2E reproduction — not available in this environment. However, the unit tests comprehensively cover the registration, exclusion, signal, and no-settings paths, and the prior maintainer A/B verification (@wenshao's comment) confirms the fix against the real pipeline. 中文说明代码审查独立方案(读 diff 前): 与 diff 对比: 无关键阻碍或 AGENTS.md 违规。 代码聚焦、符合惯例、遵循项目约定。 测试结果单元测试(worktree)新测试覆盖了修复的所有四个方面:
静态检查真实场景测试(tmux,dev 构建)CLI 启动正常,带 PR 改动能正确响应 prompt。注意:ACP 特定 bug(ACP 快照路径未注册 model-invocable 命令)需要 ACP 服务器做完整 E2E 复现——当前环境不可用。但单元测试全面覆盖了注册、排除、信号和无 settings 路径,且之前维护者的 A/B 验证(@wenshao 的评论)已在真实管线上确认了修复效果。 — Qwen Code · qwen3.7-max |
|
This PR is clean and ready to merge. Stepping back: it fixes a real gap (#5503) where ACP mode advertises commands to the model via the snapshot but never wires the provider/executor on Config, so SkillTool can't actually invoke them. The fix is minimal — thread settings through the existing call chain, extract the registration into a shared function, and fix two latent bugs (disabled-command leak, expired signal reuse) along the way. The implementation matches what I'd propose independently. The two-CommandService pattern in 194 unit tests pass, lint and format are clean, the dev CLI starts and responds normally. The new regression tests are well-constructed — running them against the base source confirms they'd catch the bug. Combined with @wenshao's prior A/B verification against the real pipeline, the evidence is solid. Approving. ✅ 中文说明这个 PR 干净且可以合并。 退一步看:它修复了真实的缺口(#5503)——ACP 模式通过快照向模型 advertised 命令,但从未在 Config 上接线 provider/executor,导致 SkillTool 无法实际调用它们。修复最小化——将 settings 透传到现有调用链中,提取注册逻辑到共享函数,并顺带修复了两个潜在 bug(禁用命令泄漏、过期信号复用)。 实现与我独立提出的方案一致。 194 个单元测试通过,lint 和格式检查干净,dev CLI 正常启动和响应。新增的回归测试构造良好——在 base 源码上运行确认它们能捕获 bug。加上 @wenshao 之前在真实管线上的 A/B 验证,证据充分。 批准合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
Why it's needed
ACP
available_commands_updatebuilds slash commands throughgetAvailableCommands(), but that path only returned the command list. It did not wire the model-invocable command provider/executor ontoConfig, soSkillToolcould miss MCP prompts or file commands that the ACP command snapshot already knew about.There was also a cancellation edge where a snapshot timeout signal could be captured by a long-lived executor. Later
UserPromptExpansionhooks could then see an already-aborted signal.Reviewer Test Plan
How to verify
npm test --workspace=packages/cli -- --coverage.enabled=false src/nonInteractiveCliCommands.test.ts.npm test --workspace=packages/cli -- --coverage.enabled=false src/acp-integration/session/Session.test.ts.npx eslint packages/cli/src/nonInteractiveCliCommands.ts packages/cli/src/nonInteractiveCliCommands.test.ts packages/cli/src/acp-integration/session/Session.ts packages/cli/src/acp-integration/session/Session.test.ts.npx prettier --check packages/cli/src/nonInteractiveCliCommands.ts packages/cli/src/nonInteractiveCliCommands.test.ts packages/cli/src/acp-integration/session/Session.ts packages/cli/src/acp-integration/session/Session.test.ts.git diff --check.Evidence (Before & After)
Before: ACP snapshots could list commands without wiring the matching model-invocable provider/executor on
Config, so later model-invocable command execution could miss commands the snapshot had advertised. After: the snapshot path registers the same model-invocable command surface when settings are available, and disabled commands remain excluded.Tested on
Environment (optional)
Local npm workspace.
npm run typecheck --workspace=packages/clistill fails on existingBaseTextInput.tsxink/domandink/components/CursorContexttype resolution.Risk & Scope
Configwhen settings are available.Linked Issues
Fixes #5503
中文说明
What this PR does
Why it's needed
ACP
available_commands_update通过getAvailableCommands()构建 slash commands,但旧路径只返回 command list,没有把 model-invocable command provider/executor 挂到Config上。因此SkillTool可能拿不到 ACP command snapshot 已经知道的 MCP prompts 或 file commands。此外还有一个取消信号边界:snapshot timeout signal 如果被长期 executor 捕获,后续
UserPromptExpansionhooks 可能会看到一个已经 aborted 的 signal。Reviewer Test Plan
How to verify
git diff --check命令。Evidence (Before & After)
修复前:ACP snapshot 可能列出 commands,但没有把对应 model-invocable provider/executor 注册到
Config,导致后续 model-invocable command execution 漏掉 snapshot 已经 advertised 的 commands。修复后:settings 可用时,snapshot 路径会注册同一套 model-invocable command surface,并继续排除 disabled commands。Tested on
Environment (optional)
本地 npm workspace。
npm run typecheck --workspace=packages/cli仍在既有BaseTextInput.tsx的ink/dom和ink/components/CursorContext类型解析处失败。Risk & Scope
Config上的 model-invocable command registration。Linked Issues
Fixes #5503
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.