feat(cli): show description and level in /skills ACP output - #6117
Merged
Conversation
The /skills command in daemon/ACP mode only showed skill names as a plain list. Now it displays description and level label alongside each name, matching the information density of the interactive mode dialog. Extract levelLabel() from SkillsManagerDialog into a shared utility to avoid duplication. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
|
Thanks for the PR! (Re-run — updating prior comment.) Template looks good ✓ — all required sections present with before/after evidence and bilingual description. On direction: Closing the information gap between interactive and ACP/daemon modes for On approach: The scope is minimal and well-targeted — extend Moving on to code review. 🔍 中文说明感谢贡献！（重新运行 — 更新之前的评论。） 模板完整 ✓ — 所有必需部分均有，包含前后对比证据和双语描述。 方向：缩小 方案：范围最小化且目标明确——扩展 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Contributor
There was a problem hiding this comment.
Pull request overview
Improves /skills output in daemon/ACP (non-interactive) mode by including each skill’s description and source level label, bringing it closer to the interactive dialog’s information density. It also deduplicates level-label rendering by extracting a shared levelLabel() utility used by both the dialog and ACP listing.
Changes:
- Extend the UI
SkillDefinitionmodel to optionally includedescriptionandlevel. - Render description + level label in the non-interactive
SKILLS_LISThistory view. - Extract
levelLabel()into a shared utility and reuse it from both/skillsandSkillsManagerDialog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/ui/utils/skill-level-label.ts | New shared levelLabel() helper for translating skill level to a label. |
| packages/cli/src/ui/types.ts | Extends SkillDefinition for richer /skills list rendering; minor type formatting cleanup. |
| packages/cli/src/ui/components/views/SkillsList.tsx | Displays skill description and level label in the skills list view (ACP/non-interactive output). |
| packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx | Switches dialog to use shared levelLabel() utility. |
| packages/cli/src/ui/commands/skillsCommand.ts | Populates SKILLS_LIST entries with description + translated level label in ACP/non-interactive mode. |
| packages/cli/src/ui/commands/skillsCommand.test.ts | Updates tests to assert description + level label are included in ACP/non-interactive listing output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Code Review(Re-run — updating prior comment.) Independent proposal (before reading diff): To solve this, I'd extend Comparison with PR: The implementation matches my independent proposal exactly. The type extension is clean ( The Reuse check: All previously-reported blocking issues resolved:
No new blocking findings. TestingUnit tests: 10/10 pass ✓ — covers sort order, description/level rendering, bare skills, non-user-invocable filtering, disabled filtering, empty-state messages, and interactive dialog return. Build: Succeeds ✓ Tmux testing: Ink renders to the alternate screen buffer, making Prior reviewer verification (from PR comments):
(Expected — 中文说明代码审查（重新运行 — 更新之前的评论。） 独立方案（读 diff 之前）： 扩展 与 PR 的对比： 实现与我的独立方案完全一致。类型扩展干净， 所有之前报告的阻塞性问题已修复： TS2206 导入语法、ACP addItem+void 被吞掉、原始 skill.level、缺少穷举检查。 无新的阻塞性发现。 测试单元测试： 10/10 通过 ✓ 之前的审查者验证： @wenshao 构建了真实二进制并在所有模式下 E2E 测试，推荐合并 ✅；@DragonnZhang 在 Ubuntu 上 E2E tmux 测试，已批准 ✅；Ubuntu CI：通过 ✅ — Qwen Code · qwen3.7-max |
Collaborator
|
(Re-run — updating prior comment.) Clean, well-scoped PR. The motivation is real and the implementation matches what I'd have done independently: extend the type, map the fields, extract the shared utility, return text instead of void for ACP. The critical bug that @wenshao caught in round 2 (ACP Unit tests are thorough — 10 tests covering the new behavior paths including bare skills, sort order, and edge cases. Two human reviewers verified with real builds. Ubuntu CI is green. The remaining CI skips (macOS, Windows, integration) are from the stale branch, not a code defect. Approving. ✅ 中文说明干净、范围合理的 PR。动机真实，实现与我的独立方案一致：扩展类型、映射字段、提取共享工具函数、为 ACP 返回文本而非 void。 @wenshao 在第 2 轮发现的关键 bug（ACP 单元测试全面——10 个测试覆盖新行为路径。两位人类审查者用真实构建验证过。Ubuntu CI 绿色。其余 CI 跳过（macOS、Windows、集成）是因为分支过时，而非代码缺陷。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously 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. ✅
Address Copilot review: re-add the note that levelLabel() must be called at render/command time (not module-load) so language switches take effect. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
Review fix summary
CI: Reran |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM ✅ — clean, well-scoped PR. No review findings. Downgraded from Approve to Comment: CI failing (Post Coverage Comment, Test) — these failures appear unrelated to this PR's changes.
— 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 review findings. Clean, well-scoped PR with good test coverage. Downgraded from Approve to Comment: CI failing (Post Coverage Comment (ubuntu-latest, 22.x), Test (ubuntu-latest, Node 22.x)) — these failures appear unrelated to this PR's changes.
— qwen3.7-max via Qwen Code /review
Collaborator
Local verification report (real build + tmux + daemon E2E)I built the PR head as a real binary and exercised Environment
What passes ✅
The What does not work ❌ — the daemon/ACP feature is a no-opReal That Root cause
Net: the enriched Suggested fixMirror // ACP branch of skillsCommand, instead of addItem(...) + void:
const lines = sortedSkills.map(
(s) => ` - ${s.name}${s.description ? ` ${s.description}` : ''}` +
`${s.level ? ` (${levelLabel(s.level)})` : ''}`,
);
return {
type: 'message',
messageType: 'info',
content: `${t('Available skills:')}\n\n${lines.join('\n')}`,
};Keep the Verdict
(Minor: the 中文版（完整对应）本地真实验证报告（真实构建 + tmux + daemon 端到端）我把 PR head 编译成真实二进制，并在它所声称的每一种模式下运行了 环境
通过项 ✅
把 不生效项 ❌ —— daemon/ACP 功能是个 no-op真实
根因
结论： 建议修法照抄 // skillsCommand 的 ACP 分支，把 addItem(...) + void 换成：
const lines = sortedSkills.map(
(s) => ` - ${s.name}${s.description ? ` ${s.description}` : ''}` +
`${s.level ? ` (${levelLabel(s.level)})` : ''}`,
);
return {
type: 'message',
messageType: 'info',
content: `${t('Available skills:')}\n\n${lines.join('\n')}`,
};
结论
（小事： |
The ACP/daemon pipeline does not render skills_list history items — context.ui.addItem() + void return produces "Command executed successfully." Follow the aboutCommand pattern: build the text and return a MessageActionReturn so the content reaches the client. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
|
Thanks — great catch. The ACP branch was using the interactive rendering pattern (addItem + void return) which gets swallowed by handleSlashCommand as "Command executed successfully." Fixed in 33a30d1: switched to the Also removed the unused |
Collaborator
Author
Review fix summary (round 2)
All 9 unit tests pass. The ACP branch now returns the skill list as text directly. |
DragonnZhang
reviewed
Jul 1, 2026
Use the shared levelLabel() utility instead of raw skill.level to ensure consistent translated labels across SkillsList, the dialog, and ACP text. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
Review fix summary (round 3)
CI: |
doudouOUC
enabled auto-merge
July 1, 2026 11:25
DragonnZhang
reviewed
Jul 1, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Code Review (automated)
No high-confidence findings. This is a clean, well-scoped PR.
Summary of review:
- Data flow verified:
SkillConfig(fromlistSkills()) carriesdescription: stringandlevel: SkillLevelas required fields, so the ACP listing always has the data it needs. - The switch from
addItem()+ void return toMessageActionReturncorrectly addresses the ACP no-op issue found in prior review rounds. - The
levelLabel()extraction is a pure refactor — the render-time evaluation comment is preserved, and both call sites (skillsCommand.ts,SkillsManagerDialog.tsx) import from the shared module. SkillsList.tsxgracefully degrades whendescription/levelare absent (conditional rendering), so old serializedHistoryItemSkillsListentries still render correctly.- Test coverage is thorough: priority ordering, description/level display, disabled filtering, non-user-invocable filtering, and edge-case messages are all covered.
- The
as SkillLevelcast inSkillsList.tsxis justified — the data originates fromSkillConfig.level: SkillLevel.
LGTM.
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.
[Suggestion] The !skillManager early-return path (around line 38) still uses context.ui.addItem() + void return, while every other branch was migrated to MessageActionReturn. In ACP/non-interactive mode, a void return is mapped to "Command executed successfully." by the handler — silently discarding the error. Consider migrating it to the same pattern:
return {
type: 'message' as const,
messageType: 'error' as const,
content: t('Could not retrieve skill manager.'),
};[Suggestion] No test file exists for the SkillsList React component, which gained substantial new rendering logic (truncate, conditional description, conditional level). Consider adding a SkillsList.test.tsx to cover the new branches.
…l test - Fix TS2206: remove redundant inner `type` modifier in import statement - Migrate !skillManager early return to MessageActionReturn in ACP mode - Sanitize descriptions (collapse newlines, truncate to 80 chars) in ACP text - Add test case for skills without description or level 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
Review fix summary (round 4 — ci-bot CHANGES_REQUESTED)
|
wenshao
reviewed
Jul 1, 2026
Compile-time error if a new SkillLevel is added without a translation, preventing untranslated lowercase labels from leaking to all call sites. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
Review fix summary (round 5 — wenshao)
|
DragonnZhang
approved these changes
Jul 1, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Looks good -- clean extraction of levelLabel into a shared module, correct dual-path handling (interactive returns dialog, ACP returns message with description+level), and thorough test updates.
Minor observation (non-blocking): SkillDefinition.level is typed as string rather than SkillLevel, requiring an as SkillLevel cast in SkillsList.tsx. Since SkillLevel is already imported there, tightening the type on SkillDefinition would remove the cast.
Collaborator
E2E Tmux Test Report — PR #6117Test Environment
Test ResultsTest 1: Interactive mode —
|
qwen-code-ci-bot
requested changes
Jul 1, 2026
Collaborator
✅ Local verification with a real built binary (tmux + ACP e2e) — PASSI built the PR head into a real Environment
1) Unit tests + typecheck
2) Headline feature — real ACP e2e (PASS)Drove the real agent over ACP JSON‑RPC stdio ( Verified in one real run:
3) A/B proving the change is load‑bearing (before → after)Reverting only Before this PR the actual list was pushed into a TUI‑only 4) Interactive TUI via tmux (PASS)Launched the real binary in tmux and ran Observations (non‑blocking — for consideration, not merge blockers)
VerdictThe PR does what it claims, is well‑tested, typechecks cleanly, and closes a genuine ACP UX gap. LGTM / recommend merge — optionally clean up the now‑dead 🇨🇳 中文版（点击展开）✅ 使用真实构建的二进制在本地验证（tmux + ACP 端到端）— 通过我把 PR head 构建成了真实的 环境
1）单元测试 + 类型检查
2）核心功能 —— 真实 ACP 端到端（通过）用真实 agent 走 ACP JSON‑RPC stdio（ 一次真实运行即验证：
3）A/B 证明改动承重（改前 → 改后）仅把 改前，真实列表被塞进一个只有 TUI 才能渲染的 4）tmux 下的交互式 TUI（通过）在 tmux 里启动真实二进制并执行 观察项（非阻塞 —— 仅供参考，不是合并阻断项）
结论该 PR 名副其实，测试充分，类型检查干净，且弥补了一个真实的 ACP 使用体验缺口。LGTM / 建议合并 —— 可在后续 PR 里顺手清掉现已死亡的 Verified locally on a real built binary (worktree @ |
Collaborator
CI failure diagnosis — branch is stale, not a code bugTL;DR: The red check is not caused by the code in this PR. Your branch is behind What's actually failingOnly Why
Knock-on effects (both are symptoms, not separate problems)
FixBring the latest
No source change is required for the CI failure itself. Minor, non-blocking FYI (does not affect CI — noted only so your diff stays clean): 🇨🇳 中文版CI 失败诊断 —— 分支陈旧,并非代码 bug结论先行: 这个红叉不是本 PR 代码引起的。你的分支落后于 到底哪里失败了只有 原因
连带影响(这两条都是症状,不是独立问题)
修复方法把最新的
针对 CI 失败本身,无需改动任何源码。 次要、不阻塞的提示(不影响 CI,仅为让你的 diff 更干净): |
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. |
Collaborator
|
@qwen-code /triage |
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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No critical findings. Deterministic analysis (tsc, eslint) clean, all 10 tests pass.
Needs human review:
sanitize()inskillsCommand.tscollapses newlines and truncates but does not strip ANSI escape sequences. Both ACP and interactive paths share this gap. Low priority for a local CLI tool (codebase already depends onstrip-ansi).- The
extensionSkillLevelbranch inlevelLabel()is not covered by any test. The TypeScript exhaustivenevercheck provides compile-time safety.
— qwen3.7-max via Qwen Code /review
| : `${oneLine.slice(0, Math.max(0, max - 1))}…`; | ||
| }; | ||
| const lines = sortedSkills.map( | ||
| (s) => |
Collaborator
There was a problem hiding this comment.
[Suggestion] The ACP plain-text listing does not pad skill names to a fixed column width before appending descriptions. Both SkillsManagerDialog.tsx (line 261) and SkillsList.tsx (line 38) use .padEnd(NAME_COLUMN) for alignment, but this path uses a fixed two-space separator regardless of name length. The PR description's "After" example shows aligned output that the code cannot produce for skills of differing name lengths.
Suggested change
| (s) => | |
| const NAME_COLUMN = 24; | |
| const lines = sortedSkills.map( | |
| (s) => | |
| ` - ${s.name.padEnd(NAME_COLUMN)}${s.description ? `${sanitize(s.description, 80)}` : ''}` + | |
| `${s.level ? ` (${levelLabel(s.level)})` : ''}`, | |
| ); |
— qwen3.7-max via 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 this PR does
In daemon/ACP (non-interactive) mode, the
/skillscommand now displays description and level label alongside each skill name, instead of showing only bare names. This brings the information density closer to the interactive mode's dialog.Additionally, extracts the
levelLabel()utility fromSkillsManagerDialoginto a shared module (skill-level-label.ts) to avoid duplication between the dialog and the command.Why it's needed
Users in daemon mode had no way to see what each skill does or where it comes from — the
/skillsoutput was a plain list of names with no context. The interactive mode already shows descriptions and level labels (Project/User/Extension/Bundled), but this information was lost in ACP mode due to theSkillDefinitiontype only carrying{ name: string }.Reviewer Test Plan
How to verify
/skills— each skill should now showname + description + (Level)instead of justname/skills— the dialog should work exactly as before (thelevelLabelextraction is a pure refactor)Evidence (Before & After)
Before (ACP mode):
After (ACP mode):
Tested on
Risk & Scope
SkillDefinitionare optional, no runtime validation exists on this type, and no other packages consume it.GET /workspace/skills) still does not reflect user-controlledskills.disabledstate — that is a separate gap.{ name }only will render gracefully (description/level simply omitted).中文说明
这个 PR 做了什么
在 daemon/ACP（非交互式）模式下，
/skills命令现在会在每个 skill 名字旁边显示描述和来源级别标签，而不是只显示光秃秃的名字。这使得信息密度接近交互模式的对话框。同时将
levelLabel()工具函数从SkillsManagerDialog提取到共享模块（skill-level-label.ts），避免重复代码。为什么需要
daemon 模式下用户无法看到每个 skill 的用途和来源——
/skills输出只是一个纯名字列表。交互模式已经显示了描述和级别标签（Project/User/Extension/Bundled），但这些信息在 ACP 模式下丢失了，因为SkillDefinition类型只携带{ name: string }。风险与范围
SkillDefinition上的新字段都是可选的，该类型没有运行时校验，也没有其他包消费它。GET /workspace/skills）仍然不反映用户控制的skills.disabled状态——这是一个单独的 gap。{ name }的序列化历史条目会优雅降级渲染。🤖 Generated with Qwen Code