Skip to content

fix(cli): prevent dialog clipping in short terminals - #9040

Open
shenyankm wants to merge 17 commits into
QwenLM:mainfrom
shenyankm:fix/dialog-height-overflow
Open

fix(cli): prevent dialog clipping in short terminals#9040
shenyankm wants to merge 17 commits into
QwenLM:mainfrom
shenyankm:fix/dialog-height-overflow

Conversation

@shenyankm

@shenyankm shenyankm commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Prevents the /statusline and /skills configuration dialogs from rendering beyond a constrained terminal-height budget. /statusline uses a selectable compact layout below 16 rows, while /skills limits locked-skill rows only when a height budget is present and preserves every locked skill otherwise.

Why it's needed

The dialogs reserved a fixed amount of chrome while always rendering at least one list row. On short terminals, the total height exceeded the parent budget, clipping content and making options unreachable. This restores keyboard access to the complete selectable list without changing the normal-height layout.

Reviewer Test Plan

How to verify

  1. Start Qwen Code with a short terminal height and run /statusline.
  2. Confirm that at 16 rows or more the existing full dialog is shown, and below 16 rows the dialog renders only its selectable list without vertical clipping.
  3. Use arrow keys or j to move through the compact /statusline list; every preset remains reachable and Enter still saves the selection.
  4. Run /skills with several skills disabled at a higher scope. Confirm that a constrained terminal never exceeds its available rows, locked skills appear only as the (+N locked) count and are excluded from search, and an unconstrained terminal shows every locked row.

Evidence (Before & After)

Before: the clipped /statusline dialog is documented in #9037 with the reporter's screenshots.

After: targeted Ink rendering tests assert the one-row /statusline layout, its keyboard navigation, the 18-row /skills budget, and unconstrained locked-skill visibility.

Tested on

OS Status
🍏 macOS ⚠️
🪟 Windows ⚠️
🐧 Linux

Environment (optional)

Linux; Node.js 24.18.0; targeted CLI Vitest rendering tests.

Risk & Scope

  • Main risk or tradeoff: very short /statusline layouts omit nonessential title, search, preview, and help text to reserve all rows for the selectable list.
  • Not validated / out of scope: package-wide TypeScript checking remains blocked by pre-existing ACP, channel, and doctor declaration mismatches outside this change.
  • Breaking changes / migration notes: none.

Linked Issues

Closes #9037

中文说明

此 PR 的内容

防止 /statusline/skills 配置对话框超出受限终端高度预算。/statusline 在低于 16 行时使用可选择的紧凑布局;/skills 只在存在高度预算时限制锁定技能行数,并在没有高度约束时保留全部锁定技能。

为什么需要它

这些对话框会预留固定的界面行,同时始终渲染至少一行列表。在短终端中,总高度会超过父级预算,导致内容被裁剪且选项无法访问。此修复在不改变正常高度布局的前提下恢复了通过键盘访问完整可选列表的能力。

审阅者测试计划

如何验证

  1. 在高度较短的终端中启动 Qwen Code 并运行 /statusline
  2. 确认高度为 16 行及以上时显示现有完整对话框;低于 16 行时只显示可选择列表,且没有垂直裁剪。
  3. 使用方向键或 j 在紧凑 /statusline 列表中移动;每个预设均可访问,Enter 仍会保存选择。
  4. 在较高作用域禁用多个技能后运行 /skills。确认受限终端不会超过可用行数,锁定技能仅以 (+N locked) 计数显示且不参与搜索;未受限终端会显示全部锁定行。

证据(修复前与修复后)

修复前:#9037 包含报告者提供的被裁剪 /statusline 对话框截图。

修复后:针对 Ink 渲染的测试断言了一行 /statusline 布局及其键盘导航、18 行 /skills 预算,以及无约束时锁定技能的可见性。

测试环境

操作系统 状态
🍏 macOS ⚠️
🪟 Windows ⚠️
🐧 Linux

环境(可选)

Linux;Node.js 24.18.0;目标 CLI Vitest 渲染测试。

风险与范围

  • 主要风险或权衡:极短的 /statusline 布局会省略非必要的标题、搜索、预览和帮助文本,以便将所有行留给可选择列表。
  • 未验证 / 不在范围内:包级 TypeScript 检查仍被本次变更范围外既有的 ACP、channel 和 doctor 声明不匹配阻断。
  • 破坏性变更 / 迁移说明:无。

关联问题

Closes #9037

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Aug 13, 2026
@shenyankm
shenyankm marked this pull request as ready for review August 13, 2026 04:52
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR — this is a re-run on the current head (68ea6ff2), which is a long way from the commit my last pass reviewed. Everything below is re-derived against that head, not carried over.

Template looks good ✓ — all required sections present, with a real reviewer test plan and a full Chinese translation.

Problem: observed, not theoretical. #9037 is still open and carries the reporter's screenshots of the clipped /statusline dialog. It is self-reported by you (hence the review/self-reported label), but the reproduction does not rest on that: @wenshao built this PR and a merge-base-reverted control into two bundles whose only difference is your diff, and A/B'd them in tmux PTYs at 8/10/12/14/16/20/24/30/40 rows and 46 columns. The control shows zero reachable selectable rows and overpaint artifacts at 12 rows; the PR build shows a clean list. So the defect is real and independently witnessed.

Direction: aligned. A dialog that paints rows over each other and leaves options unreachable by keyboard is a straightforward defect — there is no product-direction question here, and nothing about it pulls focus. Upstream CHANGELOG: no entry for dialog height budgets or short-terminal fitting; the status-line area is actively worked upstream, but nothing there is a signal either way on this change.

Size: Stage 0 does not apply. Nothing under packages/core/src/** or the other core path globs is touched, and the change stays inside one package plus docs, so it is not cross-package either. The title is fix(cli):, not a refactor, so no tier-1 question arises. Breakdown for reference: 556 production lines (SkillsManagerDialog.tsx 322, StatusLineDialog.tsx 179, the nine locale files 27, MultiSelect.tsx 22, statusLinePresets.ts 6), 712 test lines, 19 docs lines. That is under the 1000-line advisory, so no split request on size grounds — and the test-to-production ratio here is genuinely good.

Approach: the /statusline half is the fix the issue actually reports, and its shape is right — count the fixed chrome, hand the list budget − chrome with a floor of one row, and shed chrome in tiers when that floor is all that is left. The /skills half came from a "Related behavior" paragraph at the bottom of the same issue rather than from the reported bug, and it is where nearly all the complexity and the one open behavioural question live. Sixteen review rounds later it is fair to ask whether that half wanted to be its own PR; not something to change now, but worth naming for next time. The specific thing I would still push on: the locked-skills section collapses based on whether a height budget exists rather than whether the budget is too small to hold it. That is both the simpler condition and the one that matches your own stated intent ("locked skills appear only as the (+N locked) count" when constrained, "every locked row" otherwise). Detail and line references are in the stage-2 comment.

Risk: no elevated risk signals — none of the changed files match the revert-correlated path list, so no extra review depth is prescribed on that basis. One thing to keep in view anyway: the macOS and Windows unit legs are skipped on fork PRs, so a change whose entire subject is terminal geometry is only proven on Linux by CI.

Moving on to code review. 🔍

中文说明

感谢贡献!这是在当前 head(68ea6ff2)上的重新审查——距离我上一次审查的 commit 已经隔了很多改动,下面所有结论都是针对该 head 重新得出的,不是沿用旧结果。

模板完整 ✓ —— 所有必需章节齐全,包含真实的 reviewer test plan 和完整中文翻译。

问题: 是已观测到的 bug,不是理论性加固。#9037 仍处于 open 状态,并附有报告者提供的 /statusline 对话框被裁剪的截图。该 issue 由你本人提交(因此有 review/self-reported 标签),但复现并不依赖这一点:@wenshao 把本 PR 与一个「除你的 diff 外完全相同、四个生产文件回退到 merge base」的对照版本分别打包,在 tmux PTY 中以 8/10/12/14/16/20/24/30/40 行以及 46 列做了 A/B。对照版本在 12 行时可见可选行数为 0 并出现覆写残影;PR 版本渲染出干净的列表。所以这个缺陷是真实存在的,且有第三方见证。

方向: 对齐。对话框自我覆写、选项无法用键盘访问,是一个明确的缺陷——这里不存在产品方向争议,也不会分散重点。上游 CHANGELOG 中没有关于对话框高度预算或短终端适配的条目;status line 区域上游一直在活跃改动,但对本变更不构成任何方向的信号。

规模: Stage 0 不适用。没有触及 packages/core/src/** 或其他核心路径通配,改动也只落在单个 package 加 docs,因此也不属于跨包改动。标题是 fix(cli): 而非 refactor,所以不涉及 tier-1。数据供参考:生产代码 556 行SkillsManagerDialog.tsx 322、StatusLineDialog.tsx 179、九个语言文件 27、MultiSelect.tsx 22、statusLinePresets.ts 6)、测试 712 行文档 19 行。低于 1000 行的大 PR 建议阈值,因此不会以规模为由要求拆分——而且这里的测试/生产比例相当好。

方案: /statusline 这一半正是 issue 报告的问题,其思路是对的——统计固定界面行数,把 预算 − 界面行数 交给列表并保留一行下限,当只剩这一行时再分档削减界面。/skills 这一半来自同一 issue 底部的「相关行为」段落而非报告的 bug,几乎所有复杂度和唯一未决的行为问题都集中在这里。经过 16 轮审查,值得问一句:这一半是否本该是独立的 PR?现在不必改,但对下次有参考价值。我仍会坚持的一点是:锁定技能区块的折叠依据是是否存在高度预算,而不是预算是否小到装不下它。后者既是更简单的条件,也更符合你自己陈述的意图(受限时锁定技能「仅以 (+N locked) 计数显示」,否则「显示全部锁定行」)。细节和行号见 stage-2 评论。

风险: 无升级风险信号——改动文件均未命中与 revert 相关的路径清单,因此不需要基于该信号加深审查。但有一点值得留意:fork PR 的 macOS 与 Windows 单元测试任务会被跳过,所以一个主题完全是终端几何的改动,CI 只在 Linux 上得到了验证。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 68ea6ff2b2cc0d73e515285f45a55cf62c06f4a6 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Code review

My independent proposal first — written from the title and the "Why it's needed" section, before reading the diff: count the dialog's fixed chrome rows, hand the interactive list budget − chrome with a floor of one row, shed chrome in tiers once that floor is all that remains, and collapse secondary read-only sections only when the residual genuinely cannot hold them — never merely because a budget happens to exist. The PR does the first three well and diverges on the fourth, and that divergence is the one thing I would want resolved before merge.

One regression-class finding, still open at this head

@wenshao's runtime report of 2026-08-29 (run at d1590f1a) named this as the one to fix before merging. I did not take that on trust — I re-verified it line by line against 68ea6ff2, including the constrainHeight chain on current main, and it stands unchanged.

SkillsManagerDialog.tsx:229 sets const constrained = availableTerminalHeight !== undefined, and :245 returns [] from filteredLocked whenever constrained is true. availableTerminalHeight is undefined only when uiState.constrainHeight is false (DialogManager.tsx:84, passed to the mount at :504). But constrainHeight initialises to true (AppContainer.tsx:3774), is set back to true by any keypress that finds it false (AppContainer.tsx:46634666), and is cleared only by Ctrl+S (:4689). Opening /skills costs more keystrokes than that, so in the shipped UI the budget always exists → constrained is always true → the entire filteredLocked.length > 0 block at :624641 (the Locked by higher-scope settings header, the per-skill rows, the [locked: <scope>] labels) never renders. At any height, including a 40-row terminal where residual could hold those rows with room to spare.

Same root cause, one hundred lines down. :543 computes matchedCount = filteredUnlocked.length + filteredLocked.length against totalCount = allSkills.length, which includes the locked skills. Because filteredLocked is unconditionally empty on the constrained path, a query that matches only locked skills renders 0 / 23 skills · (+4 locked) immediately followed by No skills match the search. — the badge and the count contradict each other inside the same subtitle row (:565569).

What this costs is informational rather than data loss: after this PR there is no in-product way to learn which settings scope is holding a skill down. Relatedly, the All available skills are locked at a higher scope (see below). string this PR removes from all nine locales used to point at a section that, on this code path, no longer renders at all.

The fix is small, and the value it needs is already computed: residual at :499503. Decide the collapse from whether residual can hold the locked rows, and fall back to the (+N locked) badge only when it cannot. The badge is a real improvement — it just should not be the only outcome. Fixing this also resolves the count, since matchedCount becomes correct once filteredLocked is populated on the paths where the rows are actually shown.

Two things I checked that turned out fine

truncateLabels was added to the shared MultiSelect, which an earlier review round flagged as silently changing ArenaStartDialog — the one consumer this PR does not modify. It is opt-in and defaults to false (MultiSelect.tsx:37, :67), and only SkillsManagerDialog.tsx:610 and StatusLineDialog.tsx:287 pass it, so no third consumer's rendering changed. That concern is closed.

The activeValue state plus its reconciling useEffect were removed in favour of reading the highlighted row from onConfirm's new second argument. That deletes a stale-highlight bug class rather than adding one — the old effect had to guess the highlighted row after a filter dropped it, and the new path is told. @wenshao confirmed it behaviourally clean both on first render and after a search filter, which matches my read of the diff.

Not repeating the review lane

The /review skill is at round 16 on this PR and posted zero new findings this round, with sixteen Suggestion-level items recorded as deferred — missing test witnesses for the tier gates and narrow-width truncation guards, the MultiSelect scroll window not re-expanding after a shrink-then-grow, the non-interactive separator spending a scarce compact-tier row, docs wording. Under this repo's own "don't let review rounds balloon" rule those are follow-up material, not merge conditions, and that includes the duplicate (+N locked) badge when every skill is locked and constrained, which is cosmetic. I am not adding to that pile. The finding above is the one I treat differently, because it is a behaviour regression rather than a missing witness.

Not verified: that the row arithmetic still holds at this head. @wenshao's A/B ran at d1590f1a; two main merges have landed since (30908bbf, 68ea6ff2), and budget constants interact with chrome that main may have moved underneath them. The gate is static — I did not build or run any PR-derived code, and I did not re-run the author's tests. The author's "Tested on" column (Linux ✅, macOS ⚠️, Windows ⚠️) is the author's claim, not evidence I reproduced.

Sandboxed verification would settle exactly that: @qwen-code /verify — the claim that the height budget still fits at 68ea6ff2 after two main merges is the same A/B claim @wenshao's report made at d1590f1a and nothing has re-made since, and a run would also settle directly observable behaviour the diff cannot: whether the locked-skills section renders at a 40-row budget. You do not have write access, so this needs a maintainer's @qwen-code /verify comment to approve the head it runs against. That sponsored run carries a pre-execution risk screen and a full workspace wipe, but the report it produces is still shaped by adversarial input and should be read with the same skepticism as the fork's own CI logs.

Files changed (19 of 19 shown)
File What changed
packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx The bulk of the PR: compact/bare height tiers, residual-based list sizing, bare-mode key suppression, one-row label normalisation, the locked section gated on constrained, and the onConfirm active-value refactor
packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx New suite pinning the tiers, bare-mode key handling, and the constrained locked-count badge
packages/cli/src/ui/components/StatusLineDialog.tsx Options hoisted to module scope, a 16-row full/compact layout switch, compact-mode query bypass, and truncate wrapping on every counted row
packages/cli/src/ui/components/StatusLineDialog.test.tsx Reworked for the compact layout plus keyboard navigation through it
packages/cli/src/ui/components/shared/MultiSelect.tsx New opt-in truncateLabels prop; onConfirm now also receives the active value
packages/cli/src/ui/components/shared/MultiSelect.test.tsx New tests for those two MultiSelect changes
packages/cli/src/ui/statusLinePresets.ts Collapses line breaks in the assembled status line so one preset stays one row
packages/cli/src/ui/statusLinePresets.test.ts Test for the line-break collapse
docs/design/short-skills-dialog.md New design doc stating the /skills budget invariants
docs/users/features/status-line.md Documents the 16-row compact threshold for users
packages/cli/src/i18n/locales/ca.js · de.js · en.js · fr.js · ja.js · pt.js · ru.js · zh.js · zh-TW.js Nine mechanical one-line edits: drop the (see below) locked string, add the (+{{count}} locked) badge key

CI evidence

All 43 check-runs on 68ea6ff2 are settled — nothing pending, nothing red, so there is no failing job log to quote. The skips are the fork-PR legs that require secrets, not failures. My previous stage-2 comment deferred on a red Test (ubuntu-latest, Node 22.x); that failure is gone on this head.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
Test (macos-latest, Node 22.x) ⏭️ skipped
Test (windows-latest, Node 22.x) ⏭️ skipped
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Integration Tests (CLI, No Sandbox) ⏭️ skipped
TUI parity snapshots (ink vs opentui) ✅ success
OpenTUI no-flicker gate ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Classify PR ✅ success

Two of those greens matter for this diff specifically: TUI parity snapshots (ink vs opentui) and OpenTUI no-flicker gate both exercise the rendering path the four production files change, and both pass. The macOS and Windows unit legs are skipped because this is a fork PR, so the dialog arithmetic is proven on Linux only — worth holding in mind for a change whose whole subject is terminal geometry, and one more reason the sandboxed lane above is worth a maintainer's comment.

中文说明

代码审查

先说我自己的独立方案(只看标题和「为什么需要它」,未看 diff 时写下的):统计对话框固定界面行数,把 预算 − 界面行数 交给可交互列表并保留一行下限;当只剩这一行时再分档削减界面;而次要的只读区块,只在剩余空间确实装不下时才折叠——绝不能仅仅因为「存在预算」就折叠。本 PR 前三点做得很好,第四点出现了分歧,而这个分歧正是我认为合并前应当解决的一点。

一条回归级发现,在当前 head 上仍然存在

@wenshao 在 2026-08-29 的运行时报告(跑在 d1590f1a)把这一条列为合并前应修的问题。我没有直接采信,而是针对 68ea6ff2 逐行重新验证,包括在当前 main 上复核 constrainHeight 链路——结论不变。

SkillsManagerDialog.tsx:229 写的是 constrained = availableTerminalHeight !== undefined:245constrained 为真时直接让 filteredLocked 返回 []。而 availableTerminalHeight 只有在 uiState.constrainHeight 为 false 时才是 undefinedDialogManager.tsx:84,在 :504 传给挂载点)。但 constrainHeight 初值为 trueAppContainer.tsx:3774),任何发现它为 false 的按键都会把它重新置为 trueAppContainer.tsx:46634666),只有 Ctrl+S 会清除它(:4689)。打开 /skills 所需的按键数远超于此,所以在实际发布的 UI 中预算总是存在 → constrained 恒为真 → :624641 的整个 filteredLocked.length > 0 区块(Locked by higher-scope settings 标题、逐条技能行、[locked: <scope>] 标签)永远不会渲染。在任何高度下都如此,包括 40 行终端——那里 residual 完全放得下这些行。

同一根因,往下约一百行。:543matchedCount = filteredUnlocked.length + filteredLocked.length 去对比 totalCount = allSkills.length,而后者包含锁定技能。由于受限路径下 filteredLocked 恒为空,一个只匹配锁定技能的查询会渲染出 0 / 23 skills · (+4 locked),紧接着是 No skills match the search.——徽章与计数在同一行副标题里自相矛盾(:565569)。

代价是信息层面的,不是数据丢失:本 PR 之后,产品内再也没有办法得知是哪个 settings scope 锁住了某个技能。相关地,本 PR 从九个语言文件中删除的 All available skills are locked at a higher scope (see below). 原本指向一个在此代码路径下已完全不渲染的区块。

修复很小,而且所需的值已经算好了::499503residual。用「residual 能否放下锁定行」来决定折叠,只在放不下时才退回 (+N locked) 徽章。徽章本身是真正的改进——只是不该成为唯一结果。修好这一点也顺带修好计数,因为在真正显示锁定行的路径上 filteredLocked 会有内容,matchedCount 自然正确。

我核查过、确认没问题的两点

共享 MultiSelect 新增的 truncateLabels,早前某轮审查曾指出它会悄悄改变本 PR 未修改的唯一使用者 ArenaStartDialog 的渲染。它是可选参数且默认 falseMultiSelect.tsx:37:67),只有 SkillsManagerDialog.tsx:610StatusLineDialog.tsx:287 传了它,所以第三个使用者的渲染并未改变。这个顾虑已经关闭。

activeValue 状态及其协调用的 useEffect 被移除,改为从 onConfirm 新增的第二个参数读取高亮行。这删除了一类「高亮行过期」的 bug,而不是新增:旧的 effect 在过滤掉高亮行之后必须去猜,而新路径是被直接告知的。@wenshao 确认首次渲染和搜索过滤之后行为都干净,这与我对 diff 的判读一致。

不重复审查通道已有的内容

/review 技能在本 PR 上已到第 16 轮,本轮未发布任何新发现,另有 16 条 Suggestion 级条目被记录为延后——分档门控与窄宽度截断保护缺少测试见证、MultiSelect 滚动窗口在「先缩后扩」后不会重新展开、非交互分隔行占用了一个稀缺的紧凑档行、文档措辞。按本仓库自己的「不要让审查轮次膨胀」规则,这些属于后续跟进材料而非合并条件,其中包括「全部技能被锁定且受限时 (+N locked) 徽章渲染两次」这一条——那是外观问题。我不再往这堆上加东西。上面那条发现我区别对待,因为它是行为回归,而不是缺失的测试见证。

未验证: 行数算术在当前 head 上是否仍然成立。@wenshao 的 A/B 跑在 d1590f1a;此后已合入两次 main30908bbf68ea6ff2),而预算常量所依赖的界面行数可能被 main 在下面改动过。本审查是静态的——我没有构建或运行任何来自 PR 的代码,也没有重跑作者的测试。作者的「测试环境」列(Linux ✅、macOS ⚠️、Windows ⚠️)是作者的自述,不是我复现出的证据。

沙箱验证正好能解决这一点:@qwen-code /verify —— 「高度预算在两次合入 main 之后于 68ea6ff2 上仍然适配」这个主张,正是 @wenshao 报告在 d1590f1a 上做过、此后无人重做的同一个 A/B 主张;一次运行还能直接判定 diff 本身看不出来的行为:40 行预算下锁定技能区块是否渲染。你没有写权限,因此需要 maintainer 用 @qwen-code /verify 评论来批准所运行的 head。这类受资助运行带有执行前风险筛查和完整工作区擦除,但其产出的报告仍由对抗性输入塑造,应当以对待 fork 自身 CI 日志的同等怀疑去阅读。

CI 证据

68ea6ff2 上全部 43 个 check-run 均已结束——无 pending、无 red,因此没有失败日志可引用。被跳过的项是需要 secrets 的 fork PR 任务,不是失败。我上一条 stage-2 评论曾以 Test (ubuntu-latest, Node 22.x) 变红为由延后;该失败在此 head 上已消失。上方表格中真正与本 diff 相关的是两项:TUI parity snapshots (ink vs opentui)OpenTUI no-flicker gate,它们都覆盖四个生产文件改动的渲染路径,且均通过。macOS 与 Windows 单元测试因为是 fork PR 而被跳过,所以对话框算术只在 Linux 上得到证明——对一个主题完全是终端几何的改动,这一点值得记住,也是上面那条沙箱通道值得 maintainer 留一句评论的又一个理由。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 68ea6ff2b2cc0d73e515285f45a55cf62c06f4a6 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the fix is real and was independently proven end-to-end, but a behaviour regression that a maintainer called a pre-merge condition is still open at this head, and whether to land it anyway is his call rather than mine.

Stepping back, this PR does what it says it does. @wenshao's A/B harness — one worktree, one node_modules, the only variable being your diff — is about as clean a proof as a rendering change can get, and the height sweep is the convincing part: at 24 rows, a height plenty of people actually use, the pre-fix /statusline still loses its title, paints the query row over Type to search, and paints one preset row over another. After the PR it does not. The reviewer test plan reproduces step for step, every preset stays reachable in the compact layout, Enter still saves, and the live-resize round trip behaves on all four paths. If I had to maintain this in six months I would not curse the author for the /statusline half — the tiered chrome-shedding is legible and the constants are commented with what they count.

My independent proposal and the PR agree on nearly everything, which is normally a good sign. The one place they diverge is the locked-skills collapse, and it diverges in the direction that loses information: gating on "a budget exists" instead of "the budget is too small" makes the read-only locked section dead code in the shipped UI at every height, including a 40-row terminal under no pressure at all, and leaves matchedCount reporting against a total that includes the very skills it just hid. That is a one-condition fix using a value the file already computes, and it is the difference between "the dialog fits" and "the dialog fits and still tells you what it dropped".

So I am not approving, and I am not requesting changes either. Not approving, because a regression the maintainer explicitly conditioned merge on is unresolved at the commit I reviewed, and approving would attest to a state that does not exist. Not requesting changes, because the PR already carries a standing CHANGES_REQUESTED from the review lane — a second gating review saying the same thing would add noise, not signal — and because the live question is not whether the finding is real. I verified it at 68ea6ff2 line by line; it is real. The live question is whether losing the locked-scope listing is an acceptable trade for a clipping fix, or something to land now and restore in a follow-up. That is a product call and it belongs to the person who found it.

One thing I want to flag plainly, because it is exactly what gets lost at round 16: the review lane deferred this same finding under its convergence posture — "recorded, not requested in this round". Convergence pressure is the right instinct for missing test witnesses and doc wording, and this repo's own rule says to stop landing Suggestions after roughly five rounds. But that rule's carve-out is "correctness, security, data loss, regressions", and this is a regression. Deferring it because the round count got high is the failure mode the rule exists to prevent. If the answer is "yes, we accept the trade, ship it and restore the listing later", that is a perfectly legitimate answer — it just should be reached deliberately rather than by attrition.

Worth saying out loud as well: sixteen rounds on a dialog-height fix is a lot, and the second half of this PR — the /skills budget work, which came from a "Related behavior" note in the issue rather than from the reported bug — is where nearly every one of those rounds was spent. Nothing actionable now, but the /statusline fix on its own would very likely have landed a month ago, and the reported bug would already be closed.

Deferring to the maintainer; the reason and the specific ask are in the comment below.

中文说明

Confidence: 3/5 —— 修复是真实的,并且已被独立地端到端证明有效;但一条被 maintainer 列为合并前条件的行为回归在当前 head 上仍未解决,而「是否照样合并」应由他决定,不该由我决定。

退一步看,这个 PR 确实做到了它声称的事。@wenshao 的 A/B 装置——同一个 worktree、同一份 node_modules,唯一变量就是你的 diff——对一个渲染类改动能给出的证明已属最干净的一档,而高度扫描是最有说服力的部分:在 24 行(很多人日常真在用的高度)下,修复前的 /statusline 依然会丢掉标题、把查询行覆写在 Type to search 上、把一个预设行覆写在另一个上;PR 之后不再如此。Reviewer test plan 逐步可复现,紧凑布局下每个预设都可访问,Enter 仍能保存,实时改尺寸的四条往返路径行为都正确。如果六个月后由我来维护,/statusline 这一半不会让我抱怨作者——分档削减界面的逻辑清晰,常量也注明了各自统计的是什么。

我的独立方案与本 PR 在几乎所有点上一致,这通常是好迹象。唯一的分歧是锁定技能区块的折叠,而分歧方向是丢信息的一侧:以「存在预算」而非「预算太小」作为门控,使得只读的锁定技能区块在实际发布的 UI 中于任何高度都成为死代码,包括毫无压力的 40 行终端;同时 matchedCount 仍然对着一个「包含刚刚被隐藏的那些技能」的总数上报。这是一个只需改一个条件的修复,而且用的值这个文件已经算好了;它决定了结果是「对话框放得下」还是「对话框放得下,并且仍然告诉你它丢掉了什么」。

所以我不批准,也不请求变更。不批准,是因为在我审查的这个 commit 上,一条 maintainer 明确作为合并条件的回归尚未解决,批准等于为一个并不存在的状态背书。不请求变更,是因为本 PR 已经带有审查通道留下的、仍然生效的 CHANGES_REQUESTED——再提交一份说同样话的门控审查只会增加噪音而非信号;而且当下真正的问题并不是这条发现是否成立。我已针对 68ea6ff2 逐行验证,它成立。当下真正的问题是:为了修好裁剪而失去锁定 scope 的列表,是否是可以接受的取舍,还是应当先合并、后续再恢复。这是产品判断,属于发现问题的那个人。

有一点我想直说,因为这正是在第 16 轮最容易被丢掉的东西:审查通道在其收敛姿态下把同一条发现延后了——「已记录,本轮不要求修改」。对缺失的测试见证和文档措辞来说,收敛压力是正确的直觉,本仓库自己的规则也说大约五轮之后就不该再落地 Suggestion。但那条规则的例外项是「正确性、安全、数据丢失、回归」,而这一条正是回归。因为轮次变高就把它延后,恰恰是这条规则要防止的失效模式。如果答案是「是的,我们接受这个取舍,先合并,之后再恢复该列表」,那完全站得住脚——只是这个决定应当是被明确做出的,而不是被消耗战磨出来的。

也值得说出口的是:一个对话框高度修复走到 16 轮已经很多了,而本 PR 的后半部分——/skills 预算工作,它来自 issue 里的「相关行为」附注而非报告的 bug——几乎耗掉了其中每一轮。现在已无可操作之处,但单独的 /statusline 修复很可能一个月前就合并了,报告的 bug 也早就关闭了。

已转交 maintainer;具体原因与请求见下方评论。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 68ea6ff2b2cc0d73e515285f45a55cf62c06f4a6 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

⏸️ Deferring to @wenshao — needs a human call on this one.

This supersedes my earlier defer note on this PR, which was about a red Test (ubuntu-latest, Node 22.x) check. That is settled: CI on 68ea6ff2 is fully green with nothing pending, and the fix itself is proven by your own A/B runtime report, which I found convincing and did not need to redo.

What I cannot resolve from the diff, the tests, and the PR description is your Finding 1, and I re-verified it against the current head rather than trusting the August report. SkillsManagerDialog.tsx:229 gates on the existence of a height budget rather than its size, so filteredLocked at :245 is unconditionally empty in the shipped UI — constrainHeight starts true and every keypress resets it, so availableTerminalHeight is always defined by the time /skills mounts. The read-only locked section at :624641 therefore never renders at any terminal height, including 40 rows where residual has room, and matchedCount at :543 under-reports against a total that still includes the hidden skills. You called this a fix-before-merge on 2026-08-29. The round-16 review deferred it as non-blocking under its convergence posture.

Both positions cannot be right, and you are the one who set the condition — so the call is yours: hold the PR for the one-condition fix (decide the collapse from residual, which :499503 already computes, and keep the (+N locked) badge as the fallback only when the rows genuinely do not fit), or land it as-is and restore the locked-scope listing in a follow-up issue. Either is defensible; what I would not want is for it to be settled by review-round attrition, since AGENTS.md's convergence carve-out explicitly keeps regressions in the "land only Critical fixes" set.

I have not approved and have not submitted a second CHANGES_REQUESTED — the review lane's existing one still stands on this PR.

中文说明

⏸️ 转交 @wenshao —— 这一条需要人工判断。

本条取代我早前在本 PR 上的延后说明,那一条讲的是 Test (ubuntu-latest, Node 22.x) 变红。该问题已解决:68ea6ff2 上的 CI 全绿且无 pending,而修复本身已由你自己的 A/B 运行时报告证明——我认为那份报告是有说服力的,无需重做。

我无法从 diff、测试和 PR 描述中解决的,是你的发现 1;我没有采信 8 月那份报告,而是针对当前 head 重新做了验证。SkillsManagerDialog.tsx:229 的门控依据是高度预算是否存在而非其大小,因此 :245filteredLocked 在实际发布的 UI 中恒为空——constrainHeight 初值为 true 且每次按键都会重置,所以 /skills 挂载时 availableTerminalHeight 总是有值的。于是 :624641 的只读锁定技能区块在任何终端高度下都不会渲染,包括 residual 尚有余量的 40 行;而 :543matchedCount 仍对着一个包含被隐藏技能的总数上报,导致计数偏低。你在 2026-08-29 把这一条列为合并前应修;第 16 轮审查则在其收敛姿态下把它延后为非阻断项。

这两个立场不可能同时成立,而条件是你设的——所以决定权在你:要么让 PR 等这一个条件的修复(用 :499503 已经算出的 residual 来决定是否折叠,只在确实放不下时才退回 (+N locked) 徽章),要么按现状合并、并在后续 issue 中恢复锁定 scope 列表。两者都站得住脚;我不希望看到的是它被审查轮次的消耗战决定下来,因为 AGENTS.md 的收敛例外条款明确把「回归」保留在「只落地 Critical 修复」的集合里。

我没有批准,也没有提交第二份 CHANGES_REQUESTED——审查通道原有的那一份仍然生效。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 68ea6ff2b2cc0d73e515285f45a55cf62c06f4a6 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): "This PR fixes issue #9037: it prevents the /statusline and…": did not read each of the six pre-existing StatusLineDialog tests that keep availableTerminalHeight={18} (new formula gives them a 3-row window vs the old 10) …; "This PR fixes issue #9037: it prevents the /statusline and…": no executed Ink render to confirm the subtitle-wrap clip in finding 1 (geometry verified from source only).; "This PR fixes issue #9037: it prevents the /statusline and…": staticExtraHeight 's exact runtime composition not traced; used only to argue small-budget reachability.; "This PR fixes issue #9037: it prevents the /statusline and…": none — all checks I started completed within budget.; "This PR fixes issue #9037: it prevents the /statusline and…": full tsc typecheck of packages/cli not run (vitest compiled the changed files; the diff introduces no interface/prop changes, so type-level risk is minimal)..

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):"This PR fixes issue #9037: it prevents the /statusline and…"did not read each of the six pre-existing StatusLineDialog tests that keep availableTerminalHeight={18} (new formula gives them a 3-row window vs the old 10) …"This PR fixes issue #9037: it prevents the /statusline and…"no executed Ink render to confirm the subtitle-wrap clip in finding 1 (geometry verified from source only)."This PR fixes issue #9037: it prevents the /statusline and…"staticExtraHeight 's exact runtime composition not traced; used only to argue small-budget reachability."This PR fixes issue #9037: it prevents the /statusline and…"none — all checks I started completed within budget."This PR fixes issue #9037: it prevents the /statusline and…"full tsc typecheck of packages/cli not run (vitest compiled the changed files; the diff introduces no interface/prop changes, so type-level risk is minimal).

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Comment thread packages/cli/src/ui/components/StatusLineDialog.test.tsx
@shenyankm
shenyankm force-pushed the fix/dialog-height-overflow branch from f5484fc to 1ba8a96 Compare August 13, 2026 08:30
@github-actions

Copy link
Copy Markdown
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)为单个提交。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — no check was left unfinished at the ceiling..

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):"You are review agent reverse-audit — Reverse audit agent…"none — no check was left unfinished at the ceiling.

— qwen3.8-max via Qwen Code /review (v0.21.11)

Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not explored to full depth (tool budget reached): "PR #9040 fixes issue #9037 by preventing the /statusline…": nothing meaningful left over. Used about 20 tool calls.; "PR #9040 fixes issue #9037 by preventing the /statusline…": none — all planned checks completed within budget..

中文说明

未探索到全部深度(达到工具调用预算):"PR #9040 fixes issue #9037 by preventing the /statusline…"nothing meaningful left over. Used about 20 tool calls."PR #9040 fixes issue #9037 by preventing the /statusline…"none — all planned checks completed within budget.

— qwen3.8-max via Qwen Code /review (v0.21.11)

Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Cap MultiSelect labels to the render width and truncate counted single-line texts so the height budget holds at any terminal width; give the interactive unlocked list first claim on flexible rows and let the locked block collapse to (or drop) its hint instead of overflowing.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — finished all planned checks.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; chunk 2: none — labelCap's 24-cell floor ( Math.max(DESCRIPTION_COLUMN, …) ) means option rows themselves wrap below ~38 cols, which also breaks the budget, but that com…; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget..

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

[Critical] R4-2: The search row's query text (<Text>{query || ...}</Text> in the Search: row of packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx, ~line 683) has no wrap="truncate", but SKILLS_DIALOG_FIXED_ROWS counts that row as exactly one content row — so a query longer than the content width wraps the counted row to two rows and the dialog overflows its budget, violating the allocation comment's own invariant "FIXED_ROWS + list + locked never exceeds the budget". The subtitle and footer received wrap="truncate" in this revision; the search row was missed (round-3 blocker R3-3's suggested fix explicitly named "subtitle, search row, footer"). Failure scenario: executed Ink render at this commit: /skills with 1 unlocked + 1 locked skill at availableTerminalHeight={13} renders exactly 13 rows (fits); typing a 95-character query matching both skills wraps the search row and renders 14 rows vs a 13-row budget; adding wrap="truncate" flips 14 → 13. Threshold scales with width (query ≥ ~65 cells at an 80-column terminal); at any filled budget the wrapped row clips the footer/locked hint under overflow="hidden". (This finding could not be anchored inline: the query Text sits in unchanged code outside every diff hunk.) Suggested fix: {query || (…)} — the same treatment StatusLineDialog's "> ${query}" row already received in this PR.

中文说明

仅完成部分审查,审查缺口已披露。

未探索到全部深度(达到工具调用预算):"You are review agent reverse-audit — Reverse audit agent…"none — finished all planned checks."You are review agent reverse-audit — Reverse audit agent…"none — all checks above completed within budget.;chunk 2:none — labelCap's 24-cell floor ( Math.max(DESCRIPTION_COLUMN, …) ) means option rows themselves wrap below ~38 cols, which also breaks the budget, but that com…"You are review agent reverse-audit — Reverse audit agent…"none — all planned checks completed within budget.

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

[Critical] R4-2: The search row's query text (<Text>{query || ...}</Text> in the Search: row of packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx, ~line 683) has no wrap="truncate", but SKILLS_DIALOG_FIXED_ROWS counts that row as exactly one content row — so a query longer than the content width wraps the counted row to two rows and the dialog overflows its budget, violating the allocation comment's own invariant "FIXED_ROWS + list + locked never exceeds the budget". The subtitle and footer received wrap="truncate" in this revision; the search row was missed (round-3 blocker R3-3's suggested fix explicitly named "subtitle, search row, footer"). Failure scenario: executed Ink render at this commit: /skills with 1 unlocked + 1 locked skill at availableTerminalHeight={13} renders exactly 13 rows (fits); typing a 95-character query matching both skills wraps the search row and renders 14 rows vs a 13-row budget; adding wrap="truncate" flips 14 → 13. Threshold scales with width (query ≥ ~65 cells at an 80-column terminal); at any filled budget the wrapped row clips the footer/locked hint under overflow="hidden". (This finding could not be anchored inline: the query Text sits in unchanged code outside every diff hunk.) Suggested fix: {query || (…)} — the same treatment StatusLineDialog's "> ${query}" row already received in this PR.

— qwen3.8-max via Qwen Code /review (v0.21.11)

Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/components/DialogManager.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "PR #9040 fixes issue #9037: it prevents the /statusline…": none (finished within ~7 of ~42 tool calls).; "PR #9040 fixes issue #9037: it prevents the /statusline…": none — all planned checks completed within budget.; "PR #9040 fixes issue #9037: it prevents the /statusline…": none — all checks I started were completed within budget.; "PR #9040 fixes issue #9037: it prevents the /statusline…": none — finished within budget; "PR #9040 fixes issue #9037: it prevents the /statusline…": none — all checks I started completed within budget., and 6 more.

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

中文说明

仅完成部分审查,审查缺口已披露。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):"PR #9040 fixes issue #9037: it prevents the /statusline…"none (finished within ~7 of ~42 tool calls)."PR #9040 fixes issue #9037: it prevents the /statusline…"none — all planned checks completed within budget."PR #9040 fixes issue #9037: it prevents the /statusline…"none — all checks I started were completed within budget."PR #9040 fixes issue #9037: it prevents the /statusline…"none — finished within budget"PR #9040 fixes issue #9037: it prevents the /statusline…"none — all checks I started completed within budget.,另有 6 条。

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

— qwen3.8-max via Qwen Code /review (v0.21.11)

Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx Outdated
Comment thread packages/cli/src/ui/statusLinePresets.ts Outdated
Comment thread packages/cli/src/ui/components/DialogManager.test.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Mirrors the /statusline compact path in /skills: budgets at or below the
fixed frame drop border, paddingY, and footer so the always-rendered
list row fits. Truncates every remaining counted text (translated
titles, Type to search, Preview) at any width, folds \v/\f in status
line fields, and notes hidden locked skills in the subtitle when no
locked row fits (round-5 review).
Covers the compact-frame budgets (11/10/8), the locked-only query at a
frame-floor budget, the ca-locale 26-column title, the subtitle locked
count at residual === 1, \v/\f folding, and diverges the two width
sources in the DialogManager wiring test (round-5 review).
@shenyankm

Copy link
Copy Markdown
Contributor Author

Round-5 review: all seven findings addressed in ba35a4c + 42303a5. No deferrals.

R5-1 (Critical) — irreducible 12-row frame at budgets ≤ 11. The /skills dialog now mirrors the /statusline compact path: when the budget is at or below the fixed frame it drops the border, vertical padding, and footer (6 rows), so the always-rendered list row fits with room to spare down to a 6-row budget. This also repairs the secondary symptom — at those budgets a locked-only search previously left the list area blank, and the freed rows now surface the locked matches and the hidden-count hint again. Tests pin budgets 11/10/8 and the locked-only search at budget 11.

R5-2 (Critical) — translated title wraps. The title renders with truncation, so the 23-cell ca translation (and any other locale) stays on one row at any width. A render test drives the ca locale at 26 columns and asserts the 12-row budget holds; no blank residual row survived in the executed renders.

R5-3 — /statusline budget below ~25 columns. The title and "Type to search" prompt now truncate like every other counted row; the constant's width caveat is gone since the count is valid at any width.

R5-4 — \v/\f not folded. The status-line collapse now folds all whitespace (matching the skills-side one-line helper), so vertical-tab and form-feed characters in POSIX paths can no longer split the status line into multiple terminal rows. Covered by a new preset test.

R5-5 — wiring test blind to the width source. The DialogManager test now diverges the two candidate width sources (prop 120 vs main area 60), so a mis-wiring to the raw terminal width fails the row-count assertion.

R5-6 — locked skills vanish at residual === 1. When the mixed layout has no row left for the locked block, the subtitle now carries the count as "(+N locked)" instead of leaving the locked skills untraceable. Pinned at budget 12.

R5-7 — Preview label. The block label truncates like its siblings.

中文说明

第 5 轮评审:七条 findings 已在 ba35a4c + 42303a5 全部处理,无延后项。

R5-1(Critical)—— 预算 ≤ 11 时不可再简的 12 行框架。 /skills 对话框现镜像 /statusline 的紧凑路径:预算不高于固定框架时去掉边框、纵向内边距与页脚(共 6 行),强制列表行得以容纳,下限至 6 行预算。次要症状同步修复——此前该预算下仅匹配锁定技能的搜索会留下空白列表区,释放的行数现在能重新显示锁定匹配与隐藏计数提示。测试固定了 11/10/8 三档预算及预算 11 下的锁定搜索。

R5-2(Critical)—— 译文标题折行。 标题改为截断渲染,23 格的加泰罗尼亚语译文(及任何语言)在任意宽度下保持单行。新增渲染测试以 ca 语言、26 列驱动并断言 12 行预算成立;执行渲染中未出现残留空行。

R5-3 —— /statusline 预算在约 25 列以下失效。 标题与 "Type to search" 提示现与其他被计数行一样截断;常量注释中的宽度限制说明已删除,行数在任意宽度下有效。

R5-4 —— \v/\f 未折叠。 状态行折叠现覆盖全部空白字符(与 skills 侧单行助手一致),POSIX 路径中的垂直制表符/换页符不能再把状态行拆成多个终端行。新增预设测试覆盖。

R5-5 —— 接线测试无法区分宽度来源。 DialogManager 测试现使两个候选宽度来源取不同值(prop 120 vs 主区域 60),误接到原始终端宽度会使行数断言失败。

R5-6 —— residual === 1 时锁定技能无痕消失。 混合布局中锁定区块分不到行时,副标题以 "(+N locked)" 携带计数,不再无迹可寻。已在预算 12 下固定。

R5-7 —— Preview 标签。 该块标签与同级文本一样截断。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

Not explored to full depth (tool budget reached): "PR #9040 修复 #9037:防止 /statusline 与 /skills…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed (~20 tool calls).; "You are review agent reverse-audit — Reverse audit agent…": did not exercise the compact/escape paths interactively in a real TTY (tmux) — covered only by the unit tests above.; "You are review agent reverse-audit — Reverse audit agent…": did not audit buildStatusLinePresetParts internals field-by-field beyond the join-and-collapse (the collapse runs post-join, which bounds all fields regardles….

中文说明

已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。

未探索到全部深度(达到工具调用预算):"PR #9040 修复 #9037:防止 /statusline 与 /skills…"none — all planned checks completed within budget."You are review agent reverse-audit — Reverse audit agent…"none — all planned checks completed (~20 tool calls)."You are review agent reverse-audit — Reverse audit agent…"did not exercise the compact/escape paths interactively in a real TTY (tmux) — covered only by the unit tests above."You are review agent reverse-audit — Reverse audit agent…"did not audit buildStatusLinePresetParts internals field-by-field beyond the join-and-collapse (the collapse runs post-join, which bounds all fields regardles…

— qwen3.8-max via Qwen Code /review (v0.21.11)

Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Not explored to full depth (tool budget reached): "PR #9040 fixes #9037 by preventing the /statusline and…": none — all checks above completed within budget. The remainder of SkillsManagerDialog.test.tsx (past diff line 1026) is outside my assigned range and belongs …; "PR #9040 fixes #9037 by preventing the /statusline and…": none — all checks above completed within budget.; "PR #9040 fixes #9037 by preventing the /statusline and…": none — all checks I started were completed within budget.; chunk 6: running packages/cli vitest suite for statusLinePresets.test.ts — worktree has no node_modules installed; string-semantics verification was done directly in…; "PR #9040 fixes #9037 by preventing the /statusline and…": none — all checks I started were completed within budget (~17 of ~38 calls)., and 2 more.

中文说明

仅完成部分审查,审查缺口已披露。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未审查:reverse audit — did not converge within the reverse-audit round cap of 5。

未探索到全部深度(达到工具调用预算):"PR #9040 fixes #9037 by preventing the /statusline and…"none — all checks above completed within budget. The remainder of SkillsManagerDialog.test.tsx (past diff line 1026) is outside my assigned range and belongs …"PR #9040 fixes #9037 by preventing the /statusline and…"none — all checks above completed within budget."PR #9040 fixes #9037 by preventing the /statusline and…"none — all checks I started were completed within budget.;chunk 6:running packages/cli vitest suite for statusLinePresets.test.ts — worktree has no node_modules installed; string-semantics verification was done directly in…"PR #9040 fixes #9037 by preventing the /statusline and…"none — all checks I started were completed within budget (~17 of ~38 calls).,另有 2 条。

— qwen3.8-max via Qwen Code /review (v0.21.11)

Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/statusLinePresets.ts Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/statusLinePresets.ts Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Round-7 R7-1: the /skills compact frame still floored at 6 rows
(title + subtitle + search + list margin + the forced list row), so
height budgets <= 5 overflowed and overflow="hidden" clipped the
interactive list itself. Add a bare tier that sheds the remaining
chrome at budgets <= 5 so only the list area renders, mirroring the
/statusline compact path. The hidden search row also bypasses and
suppresses query mutation so a retained query cannot filter invisibly.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": none — I finished every check I started within the tool budget..

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

中文说明

仅完成部分审查,审查缺口已披露。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 4)"none — I finished every check I started within the tool budget.

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

— qwen3.8-max via Qwen Code /review (v0.21.12)

Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/components/StatusLineDialog.test.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx
Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

— qwen3.8-max via Qwen Code /review (v0.21.12)

Comment thread packages/cli/src/ui/components/StatusLineDialog.tsx Outdated
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx
Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx
Comment thread packages/cli/src/ui/components/shared/MultiSelect.tsx Outdated
@shenyankm

Copy link
Copy Markdown
Contributor Author

@yiliang114 这个 CI bot 似乎出现问题了;审查请求已经三天仍然没有回复。麻烦帮忙看一下,谢谢!

@wenshao

wenshao commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /review

@github-actions

Copy link
Copy Markdown
Contributor

Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R12 compact-mode search-row key gates unwitnessed (StatusLineDialog.tsx:200) — already reported as R4-12 (comment 3777951848) at the same location on the current head; same territory also covered by R7-9 (comment 3782616721)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/ui/components/shared/MultiSelect.test.tsx:37 — [test] New MultiSelect test is inert against the PR's own change (passes with the whole change reverted)
  • packages/cli/src/ui/components/StatusLineDialog.tsx:141 — [test] The ?? 18 height fallback is untested (mutant survives the whole suite)
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:279 — [probe] persistChanges refresh ordering unpinned — a Promise.all mutant stays green (ORDER MATTERS comment)
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:330 — [probe] Esc-during-loading guard unwitnessed — guard-removed mutant shows a spurious saved toast
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:267 — [probe] handlePick disabled-row Enter branch untested — mutant fills the buffer with the disabled skill
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:319 — [probe] Loading/error truncate guard unwitnessed — 34-char fixture message is too short to wrap
  • packages/cli/src/ui/components/StatusLineDialog.tsx:291 — [probe] No-match message wrap="truncate" unwitnessed — mutant renders 20 rows vs a 16-row budget
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:207 — [probe] disableVimNav wiring unwitnessed — j/k in a query double-handled under the mutant, Enter picks the wrong skill
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:252 — [probe] persistChanges failure branches untested — deleting both try/catch blocks keeps the suite green
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:66 — [probe] Untrusted-workspace branch untested — isTrusted hardcoded true, guard deletion survives the suite
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

收敛姿态下延后(第 12 轮,非阻断)——已记录,本轮不要求修改:共 10 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.3)

@wenshao

wenshao commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Runtime verification report — PR #9040

I built the PR into a real CLI bundle and drove the actual TUI in tmux PTYs at fixed terminal sizes, A/B against a bundle whose only difference is that the four production files plus src/i18n/locales/* are reverted to the merge base (52cfb18). Everything else — same worktree, same node_modules, same assets — is identical, so the only variable is this PR's diff.

Verdict: the fix is real and holds up under every size and width I threw at it. One behavioural regression is worth fixing before merge (Finding 1), plus one minor counter bug and one test-coverage gap.


1. The reported bug reproduces, and the PR fixes it

/statusline at 100×12 — same terminal, same keystrokes:

statusline at 12 rows

The 24-row case is the more interesting one, because it is a height plenty of people actually use and the pre-fix dialog still overflows: the title is gone, the query row is painted over Type to search (>ype to search), one preset row is painted over another (model-only … without reasoning level + en available), and the Preview label has vanished.

statusline at 24 rows

Height sweep at 100 columns (selectable rows visible / overpaint artifacts detected):

terminal rows /statusline BEFORE /statusline AFTER /skills BEFORE /skills AFTER
8 0 rows 2 rows 0 rows 3 rows
10 0 rows, 1 artifact 4 rows 0 rows 5 rows
12 0 rows, 3 artifacts 6 rows 0 rows, 2 artifacts 2 rows
14 1 row, 1 artifact 8 rows 1 row 4 rows
16 1 row, 1 artifact 9 rows 2 rows 6 rows
20 4 rows, 1 artifact 9 rows 4 rows 4 rows
24 7 rows, 1 artifact 3 rows 8 rows 8 rows
30 9 rows 9 rows 13 rows 14 rows
40 9 rows 9 rows 15 rows 15 rows

Note the /skills dialog title: on the pre-fix build the Manage Skills heading is clipped away at every height I tested including 40 rows, because wrapped description rows push the top of the dialog out of the overflow="hidden" box. After the PR it is present from 12 rows up.

/skills at 100×10 — the pre-fix dialog is pure chrome with no list at all:

skills at 10 rows

2. The reviewer test plan checks out

  • Every preset stays reachable in the compact layout. At 100×12, 16 × Down walks from Use theme colors to the last preset session-id; the window scrolls, nothing is skipped.
  • Enter still saves. Down×6 → SpaceEnter at 100×12 wrote ui.statusLine.items including total-input-tokens into user settings and the live status line picked it up in the same frame.
  • /skills bare tier is fully functional. At 100×10: typing zzz does not filter the hidden query away (list unchanged); DownSpaceEsc persisted {"skills":{"disabled":["proj-skill-2"]}} to workspace settings; Enter picks the highlighted skill both on the first render and after a search filter (the activeValueonConfirm(_, active) refactor is behaviourally clean); Enter on a row the user just toggled off persists the change without filling the input.
  • Live resize round-trip. Typed git in the full layout at 30 rows → resized to 12: the list renders unfiltered (hidden query bypassed); typed zzz in compact: the query is not mutated; resized back to 30: the search row still reads exactly > git; Esc in compact closes the dialog instead of silently clearing the hidden query. The production code is correct on all four — see Finding 3 for the test-coverage side of this.
  • Width-awareness holds. At 46×12 the pre-fix dialog wraps rows and paints the footer into its own border; the PR truncates to the render width and the budget survives.

narrow terminal

  • oneLine() earns its place. A skill whose YAML description is a multi-line block scalar silently consumes two list rows before the fix:

multi-line description

3. Finding 1 — the locked-skills section is removed at every terminal size, not only constrained ones

This is the one I would fix before merging.

SkillsManagerDialog.tsx:236 gates on the existence of a budget, not on its size:

const constrained = availableTerminalHeight !== undefined;
// …
const filteredLocked = useMemo(() => {
  if (constrained) return [];   // :252

and availableTerminalHeight is undefined for this dialog only when uiState.constrainHeight === false:

  • DialogManager.tsx:83const constrainedDialogHeight = constrainHeight ? dialogMaxHeight : undefined;
  • DialogManager.tsx:492SkillsManagerDialog is mounted with availableTerminalHeight={constrainedDialogHeight}
  • AppContainer.tsx:3285constrainHeight initialises to true
  • AppContainer.tsx:4157-4161every keypress sets it back to true before dispatching, and Ctrl+S (SHOW_MORE_LINES) is the only thing that clears it. Opening /skills costs seven more keystrokes after that, so it is true again by the time the dialog mounts.

Confirmed at runtime on a 40-row terminal (dialog budget 35 rows — no height pressure at all): Ctrl+S at an empty prompt then /skills, and Ctrl+S with the dialog already open, both still render only (+4 locked). So the whole filteredLocked.length > 0 block — the Locked by higher-scope settings (cannot toggle here): header, the per-skill rows, and the [locked: <scope>] labels — is dead code in the shipped UI, and the All available skills are locked at a higher scope (see below). string this PR removes from all nine locales is simply gone rather than relocated.

locked-skills regression

The user-visible cost: with four skills disabled at User scope, searching locked on a 40-row terminal used to answer 4 / 23 skills and name the scope that locked each one. After the PR it answers 0 / 23 skills + No skills match the search. while the very same subtitle says (+4 locked). There is no longer any in-product way to learn which settings scope is holding a skill down.

Suggested shape of the fix: decide the collapse from the budget rather than from "a budget exists" — keep the locked rows while residual can actually hold them, and fall back to the (+N locked) badge (which is a genuine improvement) only when it can't.

4. Finding 2 — matchedCount under-reports whenever locked rows are hidden

SkillsManagerDialog.tsx:557 still computes filteredUnlocked.length + filteredLocked.length, but filteredLocked is unconditionally [] on the constrained path, so the {{matched}} / {{total}} header counts against a total that includes the locked skills. That is the 0 / 23 above. Same root cause as Finding 1; it should fall out of the same fix.

5. Finding 3 — two surviving mutants (test coverage, not a bug)

All 70 tests in the four touched files pass locally, as do the 30 tests in arena/, DialogManager.test.tsx, layouts/ and layoutUtils.test.ts. tsc --noEmit on packages/cli is clean apart from the pre-existing TS5101 baseUrl deprecation, so the PR description's "package-wide TypeScript checking remains blocked" caveat does not seem to apply to packages/cli.

I ran 14 mutants against the PR's own suite; 12 were killed, which is a genuinely strong result for a rendering change:

# mutation result
M1 constrainedfalse killed (11)
M2 bare threshold 54 killed (4)
M3 compact threshold <= 11< 11 killed (1)
M4 /statusline full-layout threshold 1615 killed (2)
M5 compact maxItemsToShow +1 killed (2)
M6 MultiSelect ignores truncateLabels killed (3)
M7 oneLine() → identity killed (1)
M8 bare no longer bypasses a retained query killed (2)
M9 backspace not suppressed in bare mode killed (1)
M10 /statusline Esc: hasFullLayout && queryquery survived
M11 /statusline printable chars: drop the hasFullLayout && guard survived
M12 separator no longer excluded from search killed (1)
M13 vim nav disabled by a hidden query in bare mode killed (1)
M14 full-layout maxItemsToShow +1 killed (3)

M10 and M11 are the /statusline counterparts of guards the /skills suite does pin (keeps bare-mode keys active without changing a retained query). The shipped code is correct on both — I verified it with the live resize round-trip in section 2 — so this is purely a missing test, reachable in practice by resizing the terminal while the dialog is open. Worth a couple of lines in StatusLineDialog.test.tsx.


Summary

Fixes #9037 ✅ verified end-to-end at 8/10/12/14/16/18/20/24 rows and 46 columns
Reviewer test plan ✅ all four steps reproduce
Regressions found ⚠️ locked-skills section removed at all sizes (Finding 1), matchedCount under-reports (Finding 2)
Test strength 70/70 pass; 12 of 14 mutants killed; two /statusline compact-mode guards untested
Recommendation Fix Finding 1 (a one-condition change) before merge; Findings 2–3 fold into it
Harness details
  • Both arms built from one worktree at d1590f1a with npm run bundle. dist-pr = PR sources; dist-base = StatusLineDialog.tsx, MultiSelect.tsx, SkillsManagerDialog.tsx, statusLinePresets.ts and src/i18n/locales/ reverted to 52cfb18 (merge base), everything else byte-identical.
  • Isolated HOME, so no real user settings were touched. Fixture: 9 project skills in <ws>/.qwen/skills/ (one with a multi-line YAML block-scalar description) + the bundled skills, and 4 user-level skills listed in skills.disabled at User scope to produce the locked set.
  • Driven with tmux new-session -x <cols> -y <rows> and send-keys, observed with capture-pane only (never tee, which would flip the render mode). Screenshots are capture-pane -e ANSI rendered to PNG, full pane height preserved.
  • Live resize done with tmux resize-window while the dialog was open.
  • One harness note for anyone reproducing this: if you build the worktree with a node_modules link farm, ink-testing-library must be a real copy rather than a symlink, otherwise it resolves a second ink instance and every keyboard-driven test fails with an empty query. That artifact — not the PR — is what makes 15 tests look red on a naive setup.
中文版报告

PR #9040 运行时验证报告

我把这个 PR 构建成真实的 CLI bundle,在固定尺寸的 tmux PTY 中驱动真实 TUI,并与另一个 bundle 做 A/B —— 后者唯一的差别是把四个生产文件加上 src/i18n/locales/* 回退到 merge base(52cfb18)。其余(同一 worktree、同一 node_modules、同一资源)完全相同,因此唯一的变量就是本 PR 的 diff。

结论:修复是真实有效的,在我测试的所有高度与宽度下都成立。有一个行为回归建议在合并前修掉(发现 1),另有一个次要计数错误和一处测试覆盖缺口。

1. 问题可复现,PR 确实修好了

/statusline 在 100×12 下:修复前标题、搜索标签和整个可选列表全部被破坏;修复后是干净的紧凑列表(见上文第一张图)。

更值得注意的是 24 行的情况——这是很多人日常使用的高度,而修复前的对话框依然溢出:标题消失、查询行覆盖了 Type to search(变成 >ype to search)、一个预设行被另一行覆写、Preview 标签消失(见上文第二张图)。

高度扫描表见上文英文部分(列为「可见可选行数 / 检测到的覆写残影数」)。

请注意 /skills 的标题:在修复前的构建中,Manage Skills 标题在我测试的所有高度(包括 40 行)都被裁掉了,因为换行的描述行把对话框顶部挤出了 overflow="hidden" 盒子。PR 之后,从 12 行起标题都在。

2. 审阅者测试计划全部通过

  • 紧凑布局下每个预设都可达。 100×12 下按 16 次 Down 可以从 Use theme colors 走到最后一个预设 session-id,窗口正常滚动,没有跳过任何项。
  • Enter 仍会保存。 100×12 下 Down×6 → SpaceEnter 把含 total-input-tokensui.statusLine.items 写入了用户设置,实时状态栏同帧生效。
  • /skills bare 层功能完整。 100×10 下:输入 zzz 不会用隐藏的查询去过滤(列表不变);DownSpaceEsc{"skills":{"disabled":["proj-skill-2"]}} 持久化到工作区设置;Enter 在首次渲染时和搜索过滤后都能正确选中高亮的技能(activeValueonConfirm(_, active) 的重构在行为上是干净的);在刚被切换为关闭的行上按 Enter 会持久化改动且不填充输入框。
  • 实时缩放往返。 在 30 行的完整布局下输入 git → 缩放到 12 行:列表未被过滤(隐藏查询被正确旁路);在紧凑模式下输入 zzz:查询没有被污染;缩放回 30 行:搜索行仍然恰好是 > git;紧凑模式下按 Esc 会关闭对话框,而不是静默清掉隐藏的查询。生产代码这四点都是对的——测试覆盖方面见发现 3。
  • 宽度感知成立。 46×12 下,修复前的对话框会换行并把页脚画进自己的边框;PR 按渲染宽度截断,预算得以保持。
  • oneLine() 是必要的。 一个 YAML description 为多行块标量的技能,在修复前会静默占用两行列表。

3. 发现 1 —— 锁定技能区块在任何终端尺寸下都被移除,而不只是受限时

这是我建议在合并前修掉的一点。

SkillsManagerDialog.tsx:236 判断的是「预算是否存在」,而不是「预算够不够」:

const constrained = availableTerminalHeight !== undefined;
// …
const filteredLocked = useMemo(() => {
  if (constrained) return [];   // :252

而对这个对话框来说,availableTerminalHeight 只有在 uiState.constrainHeight === false 时才是 undefined

  • DialogManager.tsx:83 —— const constrainedDialogHeight = constrainHeight ? dialogMaxHeight : undefined;
  • DialogManager.tsx:492 —— SkillsManagerDialogavailableTerminalHeight={constrainedDialogHeight} 挂载
  • AppContainer.tsx:3285 —— constrainHeight 初始为 true
  • AppContainer.tsx:4157-4161 —— 任何按键在分发前都会把它设回 true,而 Ctrl+SSHOW_MORE_LINES)是唯一能清除它的操作。之后还需要再敲七次键才能打开 /skills,所以对话框挂载时它必然又是 true

已在 40 行终端(对话框预算 35 行,完全没有高度压力)上运行时确认:在空提示符下按 Ctrl+S 再执行 /skills,以及在对话框已打开时按 Ctrl+S,两种情况都仍然只渲染 (+4 locked)。也就是说整个 filteredLocked.length > 0 区块——Locked by higher-scope settings (cannot toggle here): 标题、逐条技能行、[locked: <scope>] 标签——在实际发布的 UI 中都是死代码;而本 PR 从九个语言包中删掉的 All available skills are locked at a higher scope (see below). 文案是被彻底移除,而不是被搬到别处。

用户可见的代价:在有四个技能于 User 作用域被禁用的情况下,40 行终端上搜索 locked,以前会回答 4 / 23 skills 并指出是哪个作用域锁住了它们;PR 之后回答 0 / 23 skills + No skills match the search.,而同一行副标题却写着 (+4 locked)。用户在产品内再也无法得知究竟是哪个设置作用域压住了某个技能。

建议的修法:让折叠由预算大小决定,而不是由「存在预算」决定——只要 residual 还放得下就保留锁定行,放不下时再退回到 (+N locked) 徽标(这个徽标本身是一个真正的改进)。

4. 发现 2 —— 隐藏锁定行时 matchedCount 少算

SkillsManagerDialog.tsx:557 仍然计算 filteredUnlocked.length + filteredLocked.length,但在受限路径上 filteredLocked 恒为 [],于是 {{matched}} / {{total}} 的分母包含了锁定技能而分子不包含,就出现了上面的 0 / 23。与发现 1 同源,应当随同一处修改一起解决。

5. 发现 3 —— 两个存活变异体(测试覆盖问题,不是缺陷)

四个被改动文件中的 70 个测试在本地全部通过,arena/DialogManager.test.tsxlayouts/layoutUtils.test.ts 的 30 个测试也全部通过。packages/clitsc --noEmit 除了既有的 TS5101 baseUrl 弃用告警外是干净的,因此 PR 描述中「包级 TypeScript 检查仍被阻断」的说明至少对 packages/cli 不成立。

我对 PR 自带的测试套件跑了 14 个变异体,杀死 12 个——对一个渲染类改动来说这是相当强的结果。完整表格见上文英文部分。

M10 与 M11 是 /skills 套件已经钉住的守卫(keeps bare-mode keys active without changing a retained query)在 /statusline 侧的对应物。发布代码在这两点上都是正确的——我用第 2 节的实时缩放往返验证过——所以这纯粹是缺测试,而且在实践中可以通过「对话框打开时缩放终端」触达。建议在 StatusLineDialog.test.tsx 里补两行。

总结

是否修复 #9037 ✅ 在 8/10/12/14/16/18/20/24 行及 46 列下端到端验证通过
审阅者测试计划 ✅ 四个步骤全部可复现
发现的回归 ⚠️ 锁定技能区块在所有尺寸下被移除(发现 1);matchedCount 少算(发现 2)
测试强度 70/70 通过;14 个变异体杀死 12 个;/statusline 紧凑模式的两个守卫未被测试覆盖
建议 合并前修掉发现 1(只需改一个条件);发现 2–3 可随之一并解决

@wenshao

wenshao commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code attempted to resolve merge conflicts but the run did not complete successfully.

Check the workflow run for full logs.

@wenshao

wenshao commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Root cause

main's #10600 (569756ee50, workspace-scoped extension skill state) rewrote how the /skills dialog derives its initial checked state, colliding with this PR's rewrite of SkillsManagerDialog.tsx. The PR's other files auto-merged or were untouched upstream.

Textual or semantic

Semantic, but orthogonal. main removed initialResolved = resolveSkillSettings(settings); initial selection now comes from config.isSkillEnabled(), which includes the new workspace skill state, and computeWorkspaceSkillListUpdates lost its lockedNames/defaultDisabled args. These regions auto-merged cleanly (the PR never touched them). This PR removed the activeValue state and SkillItemValue interface, moving Enter-to-invoke from onHighlight tracking to MultiSelect.onConfirm(selected, activeValue) — a contract the PR added in MultiSelect.tsx, which main never touched, so it survives intact.

The only textual conflict was the state block. Resolution keeps the PR's activeValue deletion (keeping it would reference the deleted SkillItemValue type) and takes main's comment wording, since the block no longer captures workspace lists:

const [query, setQuery] = useState('');
// Capture the higher-scope disabled lists once at mount.
const higher = useMemo(() => buildHigherDisabled(settings), [settings]);

Verified: merged file vs main = PR's diff + exactly the #10600 adoptions; vs PR head = #10600's diff. Only 11 files differ from both parents; all locales load; the deleted locale key has no remaining references.

What is load-bearing

  • Enter/pick requires MultiSelect.onConfirm to pass the active row's value as its second argument and items to use value: s.name (string). Reverting either side breaks Enter-to-invoke.
  • initialSelectedKeys now depends on config.isSkillEnabled; do not reintroduce resolveSkillSettings in this dialog — feat: manage workspace-scoped extension skill state #10600 superseded it.

What I could not verify

No build/tests run. Known fallout in a NON-conflicted file: SkillsManagerDialog.test.tsx is PR-only and its createConfig mock has no isSkillEnabled method; the merged initialSelectedKeys calls config?.isSkillEnabled(skill), so every test loading unlocked skills throws TypeError: config.isSkillEnabled is not a function. Fix = add isSkillEnabled to the test mocks — out of scope (only the conflicted file may be edited); PR CI will surface it.

中文说明

根因:main 的 #10600569756ee50)重写 /skills 对话框初始勾选状态来源,与本 PR 对 SkillsManagerDialog.tsx 的重写相撞。其余文件自动合并或上游未改。

冲突性质:语义冲突但改动正交。main 删除 initialResolved,初始选择改由 config.isSkillEnabled() 得出;computeWorkspaceSkillListUpdates 去掉 lockedNames/defaultDisabled 参数,这些区域因 PR 未触碰而自动合并。本 PR 删除 activeValue 状态与 SkillItemValue 接口,Enter 调用改用 MultiSelect.onConfirm(selected, activeValue)——该契约由本 PR 在 MultiSelect.tsx 新增,main 未动过该文件,完整保留。

解决:保留 PR 对 activeValue 的删除(否则引用已删类型),采用 main 新注释(该块不再捕获工作区列表)。已核对:相对 main 恰为 PR diff 加 #10600 采纳,相对 PR 头部恰为 #10600 diff;仅 11 个文件与双亲都不同,语言文件均可加载。

关键依赖:Enter 选中依赖 onConfirm 第二参数传出高亮行值且 itemsvalue: s.nameinitialSelectedKeys 已依赖 config.isSkillEnabled,勿恢复 resolveSkillSettings 路径。

未能验证:本流程不跑构建测试。已知非冲突文件受影响:SkillsManagerDialog.test.tsx 为本 PR 独有,其 mock 无 isSkillEnabled,合并后调用会抛 TypeError。需在测试 mock 中补该方法,但本次只允许改冲突文件,留待 PR CI 暴露后处理。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • D13-4 persistChanges refresh-ordering unpinned — already reported in the round-12 deferred list (review 5058926694), same location
  • D13-5 compact-mode escape with a retained hidden query unpinned — already reported as R4-12 (comment 3777951848)
  • D13-6 j/k append-to-query under an active query unpinned — already reported in the round-12 deferred list (review 5058926694, disableVimNav wiring)
  • D13-7 Enter on a just-toggled-off row unpinned — already reported in the round-12 deferred list (review 5058926694, handlePick disabled-row Enter branch)
  • D13-8 compact-mode printable-input gate unpinned — already reported as R7-9 (comment 3782616721)
  • D13-9 Esc-during-loading guard unpinned — already reported in the round-12 deferred list (review 5058926694)
  • D13-10 untrusted-workspace persist guard untested — already reported in the round-12 deferred list (review 5058926694)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:172 — [probe] no test presses Enter while a search filter is active in full/compact layout (the deleted activeValue guard's scenario)
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:276 — [probe] re-enable persistence direction never exercised (workspace skills.enabled / value-undefined branches unpinned)
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:212 — [probe] Escape-with-active-query test never asserts the dialog stays open (the branch's return is unpinned)
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 7 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

收敛姿态下延后(第 13 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.3)

Comment thread packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R14-3 persistChanges refresh-ordering unpinned (SkillsManagerDialog.test.tsx:282) — already reported in the round-12 deferred list (review 5058926694), same location
  • R14-4 untrusted-workspace branch untested (SkillsManagerDialog.test.tsx:69) — already reported in the round-12 deferred list (review 5058926694), same location
  • R14-6 Esc-during-loading guard unpinned (SkillsManagerDialog.test.tsx:333) — already reported in the round-12 deferred list (review 5058926694), same location
  • R14-7 handlePick toggled-off Enter branch untested (SkillsManagerDialog.test.tsx:173) — already reported in the round-12 deferred list (review 5058926694), same location
  • R14-8 persistChanges failure branches untested (SkillsManagerDialog.test.tsx:284) — already reported in the round-12 deferred list (review 5058926694), same location

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 10.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent 2": none — no check was cut short..

12 Suggestion(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement).

Deferred under the convergence posture (round 14, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:25 — [review] Every fixture skill uses level: 'user' , so the LEVEL_ORDER term of sortSkills (project < user < extension < bundled) is pinned by none of the 26 tests — …
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:27 — [review] No fixture sets userInvocable , so the dialog's list.filter((skill) => skill.userInvocable !== false) (SkillsManagerDialog.tsx:169) is pinned by none of the…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:35 — [review] No test renders MultiSelect with more than 15 unlocked skills, so the 15-row list cap ( maxItemsToShow = Math.min(15, Math.max(1, residual)) , SkillsManagerDia…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:48 — [review] No test resolves listSkills to an empty array, so the allSkills.length === 0 → 'No skills are currently available.' branch is pinned by no test (the load…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:54 — [review] The restored createConfig mock hardcodes isSkillEnabled to true , so the dialog's config-disabled-skill path has no test: neither the initial unchecked re…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:58 — [review] The Array.isArray guard on user-editable skills.disabled (SkillsManagerDialog.tsx:94-95 in namesFromScope ) is pinned by no test — every settings fixture …
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:59 — [review] Every fixture uses lowercase names that exactly match skill names, so the dialog's case-insensitive lock matching ( lower() / normalizeNames in buildHigherDi…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:60 — [review] Every createSettings fixture seeds only the User scope; the Workspace scope (4th LoadedSettings argument) is empty in all 26 tests, so persistChanges 's w…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:66 — [review] Every fixture disables skills at User scope only, so scopeOf 's System > User > SystemDefaults precedence and the rendered [locked: {{scope}}] label (Skills…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:245 — [review] The + filteredLocked.length term of matchedCount (SkillsManagerDialog.tsx:543) is pinned by no test — the only count assertions ('1 / 10 skills', '0 / 10 …
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:256 — [review] The no-change early return in persistChanges ( if (!disabledChanged && !enabledChanged) return 'ok'; , SkillsManagerDialog.tsx:308) is pinned by no test — …
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:282 — [review] The persist test asserts every side effect of a successful save except the user-facing confirmation toast — addItem is only ever supplied as an uninspected …
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 5 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:reverse audit — did not converge within the reverse-audit round cap of 10。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):"agent 2"none — no check was cut short.

12 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论;CLI 已将其移入下方延后清单(下限强制执行)。

收敛姿态下延后(第 14 轮,非阻断)——已记录,本轮不要求修改:共 12 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.3)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • DUP-1 unwitnessed compact-mode key-suppression guards (StatusLineDialog.tsx:200) — already reported as R4-12 (comment 3777951848) and R7-9 (comment 3782616721)
  • DUP-2 createConfig hardcodes isSkillEnabled true, config-disabled path untested (SkillsManagerDialog.test.tsx:54) — already reported in the round-14 deferred list (review 5085698077)
  • DUP-3 handlePick toggled-off early-return branch untested (SkillsManagerDialog.test.tsx:173) — already reported in the round-12 deferred list (review 5058926694) and re-confirmed as R14-7
  • DUP-4 disableVimNav query-active side unwitnessed (SkillsManagerDialog.test.tsx:211) — already reported in the round-12 deferred list (review 5058926694)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent invariant-b (packages/cli/src/ui/components/skills/Sk…": none — no check was cut short..

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:171 — [probe] onConfirm pick contract only exercised at list index 0 — no test navigates before pressing Enter; mutant picking items[0]?.value survives all 44 tests
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:613 — [probe] (+N locked) renders twice when constrained and all skills locked (subtitle slot + list-area row), spending one budgeted row on repeated information
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 4 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent invariant-b (packages/cli/src/ui/components/skills/Sk…"none — no check was cut short.

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

收敛姿态下延后(第 15 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • SkillsManagerDialog.test.tsx:171 Enter/pick contract only exercised at list index 0 - already reported (round-13 and round-15 deferral entries)
  • shared/MultiSelect.tsx:84 onConfirm active-value argument indistinguishable from items[0] - already reported (round-15 deferral entry)
  • SkillsManagerDialog.test.tsx:291 ca-locale title assertion resolves on the loading frame - already reported (comment 3782616715)
  • SkillsManagerDialog.tsx:613 locked-count badge rendered twice when all skills are locked and constrained - already reported (round-15 deferral entry)
  • SkillsManagerDialog.test.tsx:220 matched/total readout asymmetry under a height budget - already reported (comments 3785816314 and 3789720271, plus a round-14 deferral entry)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": I did not run packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx to confirm the error/loading matrix is green at the reviewed commit; the row …; "agent reverse-audit (round 2)": bisecting which preceding test / which post-commit pass adds the 21st row (candidate list narrowed to emitLayoutListeners in node_modules/ink/build/reconcile…; "agent reverse-audit (round 3)": npm run typecheck was not run — the changed onConfirm arity and the two wrap literals were checked against styles.d.ts / useSelectionList.ts types by re…; "agent reverse-audit (round 3)": I did not determine *why* items[0].value equals the picked row in SkillsManagerDialog.test.tsx:163-177 (fixture ordering vs. scrollOffset vs. INITIALIZE ….

Not reviewed: reverse audit — stopped before round 5 by the review time budget.

Deferred under the convergence posture (round 16, not a blocker) — recorded, not requested in this round:

  • docs/users/features/status-line.md:42 — [review] Documented 16-row threshold is the budget, not terminal rows
  • packages/cli/src/ui/components/StatusLineDialog.test.tsx:369 — [review] Compact layout's only save path has no Enter test
  • packages/cli/src/ui/components/StatusLineDialog.tsx:147 — [review] Non-interactive separator consumes a scarce compact-tier row
  • packages/cli/src/ui/components/StatusLineDialog.tsx:150 — [review] The search text's only gained token has no test witness
  • packages/cli/src/ui/components/StatusLineDialog.tsx:227 (+2 locations) — [review] Tier-keyed query-input gates are unwitnessed in both dialogs
  • packages/cli/src/ui/components/StatusLineDialog.tsx:291 (+3 locations) — [review] Four narrow-width truncation guards have no witness
  • packages/cli/src/ui/components/shared/MultiSelect.tsx:122 — [review] Scroll window never re-expands after a shrink-then-grow…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:116 — [review] The j/k guard's !query half has no witness
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx:344 — [review] oneLine()'s six non-newline break characters are unpinned
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:135 (+1 locations) — [review] TAB exempt from both new one-row normalizations
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:223 (+1 locations) — [review] Compact/bare tier contents unpinned by upper-bound-only…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:245 — [review] Locked-skill rows vanish at every height, not only scarce…
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:263 — [review] Truncated labels eat the skill's scope tag
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:530 — [review] Error-state dismissal hint suppressed where it would fit
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:569 — [review] Locked-skill count renders nowhere in the bare tier
  • packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:579 (+1 locations) — [review] Search row truncation hides the live edit point
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 5 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 2)"I did not run packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx to confirm the error/loading matrix is green at the reviewed commit; the row …"agent reverse-audit (round 2)"bisecting which preceding test / which post-commit pass adds the 21st row (candidate list narrowed to emitLayoutListeners in node_modules/ink/build/reconcile…"agent reverse-audit (round 3)"npm run typecheck was not run — the changed onConfirm arity and the two wrap literals were checked against styles.d.ts / useSelectionList.ts types by re…"agent reverse-audit (round 3)"I did not determine *why* items[0].value equals the picked row in SkillsManagerDialog.test.tsx:163-177 (fixture ordering vs. scrollOffset vs. INITIALIZE …

未审查:反向审计——评审时间预算不足,未能开始第 5 轮。

收敛姿态下延后(第 16 轮,非阻断)——已记录,本轮不要求修改:共 16 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

@wenshao

wenshao commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 412 passed · 0 failed · 412 total

Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:412 通过 · 0 失败 · 412 总计

抖动门:✅ 4 changed test file(s) x 5 identical rounds, no divergence

Verification report

PR 9040 deep verification — fix(cli): prevent dialog clipping in short terminals

Verdict: findings — 412 scripted assertions executed, 412 pass / 0 fail. The central
claim is proven load-bearing by A/B. No blocking finding. Three items worth a reviewer's
attention (one behavioural gap against the design doc this PR adds, one unpinned-guard coverage
gap, one pre-existing observation), plus one correction to the PR description.

  • Verified head: 68ea6ff2b2cc0d73e515285f45a55cf62c06f4a6 (git rev-parse HEAD^2)
  • Control (base side of the A/B): 7567824d4cd0bd23b0bf7a9cf18aab9424cc2dd7 (HEAD^1)
  • The snapshot's baseRefOid (1feb3804cf8d9feaa9781e818a0a6276a8894a6f) is not the merge
    parent — main moved between snapshot and checkout. The A/B uses HEAD^1, per the merge-ref
    contract.
中文摘要

结论:findings(412 条脚本断言全部通过,0 失败;无阻断性问题)。

  • A/B 结论:同一套渲染 harness 分别跑在 PR 构建与 base 构建上,覆盖 2 个对话框 × 14 个高度预算
    (1..24) × 4 个终端宽度 (20/26/40/100) = 每侧 112 个格子。head 侧 0/112 格子超预算,base 侧
    111/112 超预算(唯一不超的是 /skills 宽度 100、预算 24)。核心主张成立且是本次改动带来的。
    详见下方 "Central claim + A/B" 表与截图 01-ab-height-budget-head-vs-base.png
  • findings
    1. 极矮预算(≤5 行,bare 档)下,若同时存在可切换技能,被高作用域锁定的技能完全没有任何表示
      (既无行也无 (+N locked) 计数),与本 PR 新增的 docs/design/short-skills-dialog.md 所述
      "锁定技能以计数表示" 相矛盾。全部技能都被锁定时不受影响(计数本身成为唯一列表行)。
    2. 两处 Math.max(1, ...) 下限没有被任何测试钉住(变异矩阵中的 2 个存活体);实测在越界预算
      0 / -5 下确实生效,属"防御有效但无测试覆盖",非死代码,不是合并条件。
    3. 技能描述中的终端转义(OSC-8 超链接)会原样进入输出;base 与 head 表现一致,属既有问题
      非本 PR 引入。
  • 对 PR 描述的更正:描述称"包级 TypeScript 检查仍被既有 ACP/channel/doctor 声明不匹配阻断"。
    在本合并头上实测 npx tsc --noEmit -p packages/cli/tsconfig.json 0 错误,该说明已过期。
  • 未覆盖范围:逐 commit 归因(浅克隆只可见 17 个 commit 中的 1 个)、真实 TTY/tmux 交互、
    非 en 语言的翻译长度、生产环境 staticExtraHeight 实测值、其他对话框的同类问题、仓库级全量测试、
    与最新 main 的试合并。详见 "Not covered"。

Central claim + A/B

Central claim. When a height budget is supplied, /statusline and /skills render no more
rows than that budget, at any terminal width, while keeping the selectable list reachable.

Secondary claims. (a) With no budget supplied the layout is unchanged from base.
(b) The budget still holds when list content tries to break rows (multi-line descriptions,
narrow widths).

Oracle for every cell: the row count of the real component's rendered Ink frame
(lastFrame().split('\n').length) versus the budget passed as availableTerminalHeight, plus the
widest rendered line versus the column count. No stubs: real StatusLineDialog /
SkillsManagerDialog / MultiSelect / LoadedSettings, rendered through ink-testing-library.

Witness: 01-ab-height-budget-head-vs-base.png.

dialog width cells HEAD rows > budget BASE rows > budget
statusline 20 14 0 / 14 14 / 14
statusline 26 14 0 / 14 14 / 14
statusline 40 14 0 / 14 14 / 14
statusline 100 14 0 / 14 14 / 14
skills 20 14 0 / 14 14 / 14
skills 26 14 0 / 14 14 / 14
skills 40 14 0 / 14 14 / 14
skills 100 14 0 / 14 13 / 14
total 112 0 / 112 111 / 112

The single base cell that fits is /skills at width 100, budget 24, where base's fixed 23 rows
happen to land under 24. Every expectation above is pinned per arm in the harness, so the base
arm's overflow is an assertion that passes when base overflows as predicted — fail: 0 on
both arms is the A/B working, not the A/B being skipped.

Worked cell, /skills, width 100, budget 11:

HEAD RESULT|head|skills|w=100|budget=11|rows=10|over=-1|widest=99|title+search+badge|selectable=5|lockedRows=0
BASE RESULT|base|skills|w=100|budget=11|rows=23|over=12|widest=100|title+search+footer+lockedrows|selectable=5|lockedRows=5

The clipping consequence is real, not hypothetical. DefaultAppLayout.tsx:94-95 wraps
DialogManager in <Box height={dialogHeight} overflow={uiState.constrainHeight ? 'hidden' : undefined}>,
and constrainedDialogHeight / listDialogHeight are both getDialogMaxHeight(...) — the same
number as the clip box height. So base's 23 rows inside an 11-row overflow='hidden' box loses 12
rows, including interactive ones. Head renders 10.

Secondary claim (a) — an A/A parity control. With availableTerminalHeight omitted, head and
base render identically: UNCONSTRAINED|head|rows=23|lockedRows=5 and
UNCONSTRAINED|base|rows=23|lockedRows=5. The unconstrained locked section is preserved exactly,
as the description claims.

Secondary claim (b) — content that tries to break rows. Witness: the sweep below, at
/skills budget 11, one hostile character injected into every unlocked description
(SWEEP|<arm>|<payload>|... in logs/*-behavior-assert.txt):

payload HEAD rows BASE rows
benign space 10 23
LF 10 33
CRLF 10 33
CR / VT / FF / NEL / U+2028 / U+2029 / TAB 10 23
SGR colour escape 10 23
OSC-8 hyperlink escape 10 23

Head renders 10 rows for every payload — identical to benign, so oneLine() costs nothing on
clean input (zero collateral). Base inflates by exactly 10 rows on LF/CRLF (5 skills × 2 breaks),
which is the sanitizer being load-bearing. Scaling ladder (descriptions of 2 k / 5 k / 20 k
characters containing line breaks): head 10 / 10 / 10 rows in 28 / 28 / 30 ms — flat, no
superlinear behaviour; base 116 rows at every size.

Test Plan step 3 (navigation) measured, not assumed. At /statusline budget 8, driving real
keypresses through the real KeypressProvider:
NAV|head|budget=8|distinctActiveRows=17|maxRowsDuringNav=8|jMoved=true|onClose=1|onSaved=1|savedKeys=["type","useThemeColors","items"].
All 17 selectable rows (theme-colours + 16 presets; navigation skips the disabled separator)
became active, j moved the marker, height never exceeded 8, and Enter saved and closed. Base
reaches the same 17 rows but at maxRowsDuringNav=20 — over its budget of 8.

Corrections

The PR description's "Not validated / out of scope" line is stale. It states that
"package-wide TypeScript checking remains blocked by pre-existing ACP, channel, and doctor
declaration mismatches outside this change". At this merge head:

cd packages/cli && npx tsc --noEmit -p tsconfig.json    # -> 0 errors

Zero errors across the whole packages/cli project, including all four touched test files. The
branch merged main on 2026-09-06, which is the likely reason. This is a correction to the
description, not a request to change code — but a reviewer reading it would skip a gate that
now passes, and the same 0-error run is what proves the widened MultiSelect.onConfirm signature
((selectedValues, activeValue)) is consistent at every call site, not only at the one this PR
changed.

Findings

F1 — Suggestion: at budgets ≤ 5, higher-scope-locked skills leave no trace at all

docs/design/short-skills-dialog.md, added by this PR, states that when /skills receives a
height constraint "Higher-scope-locked skills are represented by a count". Measured against the
real component with 5 locked + 5 unlocked skills, the count is present from budget 6 upward and
absent below it, because the bare tier removes the subtitle that carries the badge:

LOCKED-probe (mixed fixture, width 100)     badge present?
  budget 1  -> rows=1  title/search/footer none    NO
  budget 5  -> rows=5  title/search/footer none    NO
  budget 6  -> rows=6  title+search+badge         yes
  budget 11 -> rows=10 title+search+badge         yes

Reproduce — the scratch test file is removed from the tree after each run, so copy the canonical
harness back in first (logs/head-height-budget-assert.txt holds the cells already measured):

cp tmp/pr9040-verify-20260907-000421/harness/height-budget.test.tsx \
   packages/cli/src/ui/components/zz-verify-height-budget.test.tsx
cd packages/cli && VERIFY_ARM=head VERIFY_MODE=report \
  VERIFY_OUT=/tmp/head.txt npx vitest run \
  src/ui/components/zz-verify-height-budget.test.tsx --silent=false
grep '^RESULT|head|skills|w=100|budget=[1235]|' /tmp/head.txt   # flags=none, no badge
rm packages/cli/src/ui/components/zz-verify-height-budget.test.tsx

Bounded, and this is the part that keeps it a Suggestion rather than a blocker:

  • The all-locked case is unaffected — there the badge becomes the single list row, so it
    renders at every budget including 1 (LOCKED|head|budget=1|rows=1|badge=true). The loss needs
    at least one workspace-toggleable skill to exist.
  • The budget invariant itself still holds; nothing is clipped and no row is unreachable. What is
    lost is one piece of information, silently.
  • Commit ba35a4c added the badge precisely to note "hidden locked skills … when no locked row
    fits (round-5 review)"; the round-7 bare tier then removed the surface it renders on. So this
    reads as an interaction between two review rounds, not an oversight in either.
  • The doc sentence and the behaviour disagree, and the doc is the artifact a future editor will
    trust. Either narrow the doc to "when the compact tier applies" or surface the count in bare
    mode; both are one-line changes.

Not measured: the real terminal height at which budget ≤ 5 occurs. getDialogMaxHeight is
Math.max(1, terminalHeight - staticExtraHeight - 2), and I did not measure
staticExtraHeight in a running app, so I cannot state how rare this is — only that it needs a
very short terminal.

F2 — Nice to have: two Math.max(1, …) floors are live but unpinned

The mutation matrix (witness 02-mutation-matrix-9-of-11-killed.png) reverted one guard per row
and re-ran all four affected test files. Baseline unmutated control: Tests 70 passed (70).

mutation (guard reverted) suite result verdict
STATUS_LINE_DIALOG_FIXED_ROWS 15 → 8 5 failed | 65 passed KILLED
statusline hasFullLayout → always true 4 failed | 66 passed KILLED
statusline Math.max(1, …)Math.max(0, …) 70 passed SURVIVED
skills compact → always false 12 failed | 58 passed KILLED
skills bare → always false 8 failed | 62 passed KILLED
skills if (constrained) return []if (false) 11 failed | 59 passed KILLED
skills Math.max(1, residual)residual 70 passed SURVIVED
skills oneLine() → passthrough 1 failed | 69 passed KILLED
skills bare-mode backspace gate removed 1 failed | 69 passed KILLED
MultiSelect truncateLabels ignored 3 failed | 67 passed KILLED
statusLinePresets line-break collapse removed 7 failed | 63 passed KILLED

9/11 killed. Each survivor has a positive control in the same file: reverting
hasFullLayout turns 4 tests red in StatusLineDialog.test.tsx, and reverting compact turns 12
red in SkillsManagerDialog.test.tsx — so the runner demonstrably collects tests that exercise
both mutated files, and the two survivals are real.

Classification, by measurement rather than by reading the code. Both survivors are the same shape
(a floor of 1 on the item count). I drove budgets the production caller cannot produce:

FLOOR|statusline|budget=0|rows=1     FLOOR|skills|budget=0|rows=1
FLOOR|statusline|budget=-5|rows=1    FLOOR|skills|budget=-5|rows=1

The floors bind: without them these budgets would render a 0-row list, i.e. an open dialog
with nothing in it. So they are live defence against out-of-contract props, not dead code.
They survive because DialogManager is the only caller and it passes getDialogMaxHeight(...),
which already clamps to >= 1 (layoutUtils.ts) — the guard is correct exactly as it stands and
is unreachable through the shipped path.

This is completeness reporting, not a merge condition. The fixture that would pin it is a render
at availableTerminalHeight={0} asserting one row.

F3 — Informational, pre-existing: terminal escapes in skill descriptions are forwarded verbatim

An OSC-8 hyperlink payload placed in a skill description reaches the rendered frame unchanged:
SWEEP|head|OSC8_LINK|…|oscPassthrough=true — and identically on the control,
SWEEP|base|OSC8_LINK|…|oscPassthrough=true. oneLine() collapses line breaks only; it does not
strip escapes, and does not claim to.

Reported for completeness because the PR touches this exact expression, and explicitly not
attributed to it: base and head agree, so nothing regressed. A project-level SKILL.md in a
cloned repository is untrusted content, and this is a path from that content to the user's
terminal. No exploit was demonstrated and none is claimed — the observable is that the bytes
survive. strip-ansi is already a packages/cli dependency and packages/cli/src/ui/utils/textUtils.ts
already uses it, so if a maintainer wants this closed it is cheap; it is out of scope for a
height-budget fix and should not hold this PR.

Not covered

  • Per-commit attribution. The checkout is depth 2: git rev-list HEAD^1..HEAD^2 returns 1
    commit while the snapshot's commits array lists 17. git rev-parse --is-shallow-repository
    is true. Only the aggregate HEAD^1..HEAD diff was verified; no per-commit table is claimed.
  • No real TTY. This job has no terminal, so all rendering evidence is ink-testing-library
    frames of the real components — not a live tmux session and not the reporter's screenshot from
    bug(cli): /statusline dialog is clipped in short terminals #9037. I reproduced the shape (row counts against the same budget the clip box uses), not
    the reporter's end-to-end visual. Test Plan step 1 ("start Qwen Code with a short terminal
    height") was therefore performed at the component seam, and step 4's "excluded from search" was
    verified through the code path (filteredLocked returns [] when constrained) plus the badge
    measurement, not by typing a query that matched only a locked skill.
  • Non-English locales. The nine locale files each add (+{{count}} locked). I rendered only
    the default locale, so a long translation wrapping the subtitle at narrow widths is unmeasured —
    the subtitle does carry wrap="truncate", which is why I expect it to hold, but that is
    reading, not measurement.
  • Production staticExtraHeight. Not measured, so the real-world reachability of budgets ≤ 5
    (F1) is unquantified.
  • Sibling dialogs. ThemeDialog, ModelDialog, SettingsDialog, ConsentPrompt and
    ShellConfirmationDialog also consume availableTerminalHeight and were not swept for the same
    bug class. SettingsDialog already uses clampDialogHeight; the others were not examined.
  • Gates run vs skipped. Run: the four affected test files (70 passed / 70), tsc --noEmit for
    packages/cli (0 errors), eslint on the eight touched files (0 problems, with a planted
    unused-variable violation confirmed reported and then reverted, so the gate is proven live).
    Skipped: the repo-wide unit suite, npm run lint/format/preflight, integration tests, and a
    trial merge into current main (no network or token in this job; the head already merged main
    on 2026-09-06, and HEAD^1 is the checkout's base tip).
  • screen.diff in the context directory lists the same 19 files as git diff HEAD^1..HEAD but
    is not byte-identical to it. I treated the live git diff as authoritative and did not investigate
    the discrepancy.

Methodology

Container job on the merge-ref checkout (HEAD = merge commit, HEAD^1 = base tip,
HEAD^2 = PR head), Node v22.23.2, with npm ci and npm run build already completed at head.
The base arm is a scratch worktree at HEAD^1 under tmp/base-tree. Control cleanliness was
established before trusting any cell: git diff --name-only HEAD^1..HEAD over packages/core,
package.json, package-lock.json and every packages/*/package.json is empty, and
git diff --stat HEAD^1..HEAD -- packages/core is empty, so the dependency tree and core are
byte-identical between arms and reusing the root node_modules is a pure code A/B. The base
worktree needed two mechanical fixes that change nothing it loads: npm run generate for
src/generated/git-commit.ts, and symlinks for untouched packages' dist/ and package-local
node_modules/ (the base packages/cli vitest config aliases @qwen-code/qwen-code-core to the
base tree's own ../core/src, and root ajv is 6.15.0 while core's local ajv is 8.20.0, which
is what the missing link actually broke). Both harnesses are byte-identical on the two arms
(diff verified) and use only props whose interfaces were compared across arms first; canonical
copies are in harness/, raw per-cell output in logs/, and the mutation driver is
harness/mutation-matrix.sh (it re-runs the four test files per mutation, records the suite
counts, verifies by sha256 that each mutation actually applied, and restores via git checkout --).
Every number above comes from a scripted check that ran: 412 assertions across four arm/harness
runs (head 232 + 30, base 120 + 30), tallied by the harnesses themselves into the TALLY lines in
logs/.

Flakiness gate log

rounds=5 files=4 skipped=0
file packages/cli/src/ui/components/StatusLineDialog.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/StatusLineDialog.test.tsx
file packages/cli/src/ui/components/shared/MultiSelect.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/shared/MultiSelect.test.tsx
file packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/skills/SkillsManagerDialog.test.tsx
file packages/cli/src/ui/statusLinePresets.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/statusLinePresets.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/cli/src/ui/components/StatusLineDialog.test.tsx: PPPPP
  packages/cli/src/ui/components/shared/MultiSelect.test.tsx: PPPPP
  packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx: PPPPP
  packages/cli/src/ui/statusLinePresets.test.ts: PPPPP

verdict: pass
summary: 4 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/ui/components/StatusLineDialog.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/components/shared/MultiSelect.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/statusLinePresets.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/components/StatusLineDialog.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/components/shared/MultiSelect.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/statusLinePresets.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/components/StatusLineDialog.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/components/shared/MultiSelect.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/statusLinePresets.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/components/StatusLineDialog.test.tsx: P (exit 0)
round 4 · packages/cli/src/ui/components/shared/MultiSelect.test.tsx: P (exit 0)
round 4 · packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx: P (exit 0)
round 4 · packages/cli/src/ui/statusLinePresets.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/components/StatusLineDialog.test.tsx: P (exit 0)
round 5 · packages/cli/src/ui/components/shared/MultiSelect.test.tsx: P (exit 0)
round 5 · packages/cli/src/ui/components/skills/SkillsManagerDialog.test.tsx: P (exit 0)
round 5 · packages/cli/src/ui/statusLinePresets.test.ts: P (exit 0)

Evidence images

01-ab-height-budget-head-vs-base

02-mutation-matrix-9-of-11-killed

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

⚠️ The bot's only review on 68ea6ff2b2cc0d73e515285f45a55cf62c06f4a6 is a COMMENTED one, which carries no vote — so it has no verdict of its own on this commit, and main needs two approving reviews: an approval left by another account is a separate vote and does not count as the bot's own. Two different things look like this, and the stage-3 comment above says which: the triage skill deferring on purpose at 3/5 — a fork refactor hitting the approval guardrail, or a core change escalated for maintainer awareness, both normal outcomes — or an earlier approval that a push dismissed, leaving only the comment behind, which needs a fresh review.

⚠️ 机器人在 68ea6ff2b2cc0d73e515285f45a55cf62c06f4a6 上唯一的评审是 COMMENTED不带票 —— 因此它在该 commit 上没有自己的裁决,而 main 需要两个批准(其他账号的批准是另一张票)。有两种情况长这样,上方的 stage-3 评论会说明是哪一种:triage skill 在 3/5 时有意 defer(fork refactor 命中审批护栏,或核心改动被升级交由维护者把关,两者都是正常结果);或者更早的批准被一次推送作废、只剩下这条评论,此时需要重新评审。

The stage comments above were updated with the latest result. View workflow run.

上方各阶段评论已更新为最新结果。查看工作流运行

@wenshao

wenshao commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Runtime verification — round 2, at head 68ea6ff2b2

My round-1 report was run at d1590f1a. Since then the branch took three merges from main plus one test fix, including the /resolve reconciliation against #10600's workspace-scoped skill state — so I rebuilt the whole harness from scratch at the current head and re-ran everything.

Same method as before: both arms bundled from one worktree with one node_modules; the BEFORE arm reverts only StatusLineDialog.tsx, MultiSelect.tsx, SkillsManagerDialog.tsx, statusLinePresets.ts and src/i18n/locales/*.js to origin/main (1feb3804cf, which is also the merge base for every file this PR touches). Everything else is byte-identical, so the only variable is this diff. Driven in tmux PTYs at fixed sizes against an isolated HOME.

Verdict: the fix still holds at this head, and the merge reconciliation is behaviourally correct. Finding 1 from round 1 is still live and is still the one thing I would fix before merging. One new finding (a tier-boundary cliff) and two new surviving mutants this round.


1. The bug still reproduces, and the PR still fixes it

/statusline at 100×12:

statusline at 12 rows

/skills at 100×18 — the pre-fix build loses its title and overpaints rows onto each other ((Project)t), (Project)oject), [locked: User]]); the PR renders a clean bordered dialog:

skills at 18 rows

Height sweep at 100 columns, fixture = 8 project skills (one with a multi-line YAML block-scalar description) + the bundled skills + 5 user skills, 4 of them in skills.disabled at User scope. Cell = selectable rows visible / overpaint artifacts detected:

terminal rows /skills BEFORE /skills AFTER /statusline BEFORE /statusline AFTER
8 0 3 0 2
10 0 5 0 4
12 0 2 0 6
14 1, 1 artifact 4 1 8
16 2, 2 artifacts 6 1 9
18 2, 2 artifacts 2 2 9
20 3, 1 artifact 4 4 9
24 5, 2 artifacts 8 7 3
30 11, 1 artifact 14 9 9
40 14, 1 artifact 15 9 9

Zero overpaint artifacts on the PR arm at every size; the BEFORE arm produces them from 14 rows up. The Manage Skills heading is clipped away on the BEFORE arm at every height tested including 40 (wrapped description rows push the top of the dialog out of the overflow="hidden" box); the PR shows it from 12 rows up, which is where the bare tier ends.

The /statusline 24-row cell is worth reading correctly: 7 on BEFORE is seven rows painted, several of them corrupt; 3 on AFTER is three rows that are actually right. At a 24-row terminal the dialog budget is 19 and the full layout's chrome is exactly 15, so 4 items is the honest number.

Width-awareness at 46 columns still holds — the pre-fix dialog paints its footer into its own border and drops the list entirely:

narrow terminal

Reviewer test plan, re-run at this head: all four steps reproduce. Every preset stays reachable in the compact layout (16 × Down walks from Use theme colors to session-id); Enter still saves (Down×6 → SpaceEnter at 100×12 wrote ui.statusLine.items including total-input-tokens and the live status line picked it up in the same frame); the /skills bare tier at 100×10 does not filter on the hidden query, and DownSpaceEsc persisted {"skills":{"disabled":["proj-beta"]}} to workspace settings.

2. The #10600 merge reconciliation is behaviourally sound

The /resolve merge replaced initialResolved = resolveSkillSettings(settings) with config.isSkillEnabled(skill) and dropped lockedNames / defaultDisabled from the computeWorkspaceSkillListUpdates call, matching main's new signature. I checked the observable consequence rather than the diff: with skills.disabled: ["proj-beta","proj-gamma"] pre-seeded at workspace scope, the PR build opens /skills with both rows unchecked. Correct. (The BEFORE arm can't even be read at that size — the rows are overpainted.)

I also compared the two arms with zero higher-scope locks at 40 rows: the only difference is that the PR collapses multi-line descriptions to one truncated row instead of wrapping them to two. For users with no skills.disabled at User/System/SystemDefaults scope, this PR is a strict improvement with no information loss.

3. Finding 1 (carried over, still live) — the locked-skills section is dropped at every terminal size

SkillsManagerDialog.tsx:229 still gates on the existence of a budget rather than its size:

const constrained = availableTerminalHeight !== undefined;   // :229
// …
const filteredLocked = useMemo(() => {
  if (constrained) return [];                                // :245

This round I have a sharper demonstration than last time. At 100×40 — 40 rows, dialog budget 35, no height pressure whatsoever — searching for a locked skill:

locked-skills regression

main answers 4 / 27 skills and names each locked skill with the scope that locked it. This PR answers 0 / 27 skills + No skills match the search. while the same subtitle says (+4 locked). There is no longer any in-product way to learn which settings scope is holding a skill down.

I also found the one escape hatch, and it does not help. availableTerminalHeight is undefined for this dialog only when uiState.constrainHeight === false (DialogManager.tsx:84/504), and Ctrl+S with the dialog already open does clear it — the locked rows appear. But AppContainer.tsx:4663-4665 sets constrainHeight back to true on the next keypress, so the first character you type to search for a locked skill takes them away again:

Ctrl+S transient

So the accurate statement is: you can glimpse the locked list, but you cannot search it, and the [locked: <scope>] labels are only ever visible in a state that any keystroke destroys. The All available skills are locked at a higher scope (see below). string this PR removes from all nine locales is gone rather than relocated.

Blast radius is bounded — lockedSkills is non-empty only when something appears in skills.disabled at SystemDefaults/User/System scope (buildHigherDisabled, :98-118), which the dialog itself never writes (it only writes Workspace scope). So this hits hand-edited user settings and admin/system policy, not the default install. But for exactly those users it is a regression relative to main, at every terminal height.

The suggested shape is unchanged and small: decide the collapse from the budget rather than from "a budget exists" — keep the locked rows while residual can hold them, and fall back to the (+N locked) badge (which is a genuine improvement) only when it cannot.

4. Finding 2 (same root cause) — the count contract is inconsistent

SkillsManagerDialog.tsx:542-543: totalCount = allSkills.length includes locked skills, while matchedCount = filteredUnlocked.length + filteredLocked.length cannot, because filteredLocked is unconditionally [] on the constrained path. That is the 0 / 27 above. Separately, (+4 locked) is rendered as an addendum to a total that already contains those 4, so it reads as 31. Both fall out of the Finding 1 fix.

5. Finding 3 (new this round) — a tier-boundary cliff, PR-only

Growing the terminal by one row can shrink the list, because crossing the tier boundary reinstates a frame that costs more than the row you gained. Measured on the PR arm, unfiltered:

dialog terminal rows selectable rows, PR selectable rows, main
/statusline 19 9
/statusline 20 9 4
/statusline 21 1 5
/statusline 22 1 6
/skills 16 6 2
/skills 17 1
/skills 18 2 2

threshold cliff

Nothing is clipped — the 21-row frame fits its budget exactly — so this is not the bug the PR set out to fix, and main is monotonic here only because it overflows instead. But at 21 rows the dialog spends 15 rows of chrome to show a single option, where its own compact layout one row earlier shows nine, and 21–26 rows is squarely inside the "short terminal" range this PR targets. A floor on the switch (stay compact until the full layout can show at least N items) would remove it. I'd call this a follow-up rather than a blocker.

6. Test strength — 18 mutants, 12 killed

70/70 tests in the four touched files pass, as do 461 tests across DialogManager.test.tsx, shared/, layouts/, layoutUtils.test.ts and config/skill-settings.test.ts. tsc --noEmit on packages/cli is clean and so is eslint on all eight touched files — the PR description's "package-wide TypeScript checking remains blocked" caveat no longer applies to packages/cli at this head.

# mutation result
M1 constrainedfalse killed (11)
M2 bare threshold -6-7 killed (4)
M3 compact <=< killed (1)
M4 /statusline full-layout threshold +1+0 killed (2)
M5 /skills maxItemsToShow +1 killed (4)
M6 MultiSelect ignores truncateLabels killed (1)
M7 oneLine() → identity killed (1)
M8 bare no longer bypasses a retained query killed (2)
M9 backspace not suppressed in bare mode killed (1)
M10 /statusline Esc drops the hasFullLayout guard survived
M11 /statusline printable chars drop the hasFullLayout guard survived
M12 separator no longer excluded from search killed (1)
M13 j/k early return drops bare survived — equivalent
M14 /statusline full-layout maxItemsToShow +1 killed (3)
M15 buildStatusLinePresetLines newline scrub removed killed (7)
M16 initial checked state ignores config.isSkillEnabled survived
M17 compact tier's frameRows reverts to the full frame survived
M18 /statusline maxItemsToShow cap 1020 survived — benign

Three of these are worth acting on:

  • M16SkillsManagerDialog.test.tsx:52 mocks isSkillEnabled: vi.fn().mockReturnValue(true), so the production dependency the feat: manage workspace-scoped extension skill state #10600 merge introduced is inert in the suite: replacing the whole predicate with () => true leaves all 26 tests green. I verified the real behaviour by hand (§2), but nothing in CI would catch it regressing. A single case with a workspace-disabled skill would close it.

  • M17 — the budget tests are all toBeLessThanOrEqual, never a lower bound, so nothing pins that the compact tier actually reclaims the 6 rows it sheds. Reverting frameRows for the compact tier turns /skills at 100×16 from 6 selectable rows into 1, wastes 5 blank rows, and the entire suite stays green:

    surviving mutant M17

    The /statusline suite already has the counterpart (uses every available row in an intermediate compact layout); the /skills suite is missing it.

  • M10 / M11 — carried over from round 1 and unchanged. closes on escape in the compact layout runs with an empty query, and bypasses a hidden query in compact mode and restores it never types while compact, so neither guard is witnessed. The shipped code is correct on both (I re-checked with a live resize round trip); it is purely a missing test, reachable in practice by resizing the terminal while the dialog is open.

M13 is an equivalent mutant and I would not ask for a test: dropping bare from the j/k early return lets those keys fall through to the printable-character branch, which is itself !bare-gated, and MultiSelect receives the keypress from its own subscription regardless — so the behaviour is identical. The guard is redundant, not load-bearing. M18 is benign: the 10 cap is a product choice, and raising it overflows nothing at any budget.


Summary

Fixes #9037 at this head ✅ verified end-to-end at 8/10/12/14/16/18/20/24/30/40 rows and 46 columns
Reviewer test plan ✅ all four steps reproduce
#10600 merge reconciliation ✅ workspace-scoped disables render correctly
No-locked-skills case ✅ strict improvement over main, no information loss
Regressions vs main ⚠️ locked-skills section unsearchable at all sizes (F1); matchedCount / (+N locked) inconsistent (F2)
New this round ⚠️ tier-boundary cliff: 20→21 rows costs 8 preset rows (F3, follow-up)
Tests 70/70 + 461 neighbours pass; tsc and eslint clean; 12 of 18 mutants killed
Recommendation Fix F1 (one condition, using a value the file already computes) before merge; F2 falls out of it; F3, M16 and M17 are good follow-ups
Harness details
  • Worktree at 68ea6ff2b2, npm install reconciled against the branch's own lockfile. Both arms via npm run build && npm run bundle; dist-pr = PR sources, dist-base = the five paths above reverted to origin/main. Arm identity checked by probing the bundles for STATUS_LINE_DIALOG_FIXED_ROWS, (+{{count}} locked) and All available skills are locked at a higher scope — 1/19/0 on the PR arm, 0/0/19 on the base arm.
  • origin/main has no commits touching any file this PR changes since the merge base, and git merge-tree reports a clean merge.
  • Isolated HOME per run, so no real user settings were touched. Fixture regenerated per session so persistence probes start from a known state.
  • Driven with tmux new-session -x <cols> -y <rows> + send-keys, observed with capture-pane only (never tee, which flips the render mode). Screenshots are capture-pane -e ANSI rendered through xterm.js under Playwright.
  • Mutants applied to the TypeScript sources and run against the PR's own four test files; M17's blast radius measured by re-bundling the mutant and driving the real CLI.
  • Artifact-count metric = a rendered list line with text after its trailing (Project)/(User)/(Bundled) scope tag, box borders stripped first.
中文说明

PR #9040 运行时验证报告(第 2 轮,head 68ea6ff2b2

我的第 1 轮报告是在 d1590f1a 上做的。此后分支合入了三次 main 加一次测试修复,其中包括针对 #10600(工作区作用域技能状态)的 /resolve 冲突解决——因此我在当前 head 上从零重建了整套装置并重跑了全部验证。

方法与上轮相同:两臂都从同一个 worktree、同一份 node_modules 构建;BEFORE 臂只把 StatusLineDialog.tsxMultiSelect.tsxSkillsManagerDialog.tsxstatusLinePresets.tssrc/i18n/locales/*.js 回退到 origin/main1feb3804cf,也是本 PR 所有改动文件的 merge base)。其余完全一致,因此唯一变量就是这份 diff。在隔离 HOME 下用固定尺寸的 tmux PTY 驱动真实 TUI。

结论:在当前 head 上修复依然成立,合并冲突的解决在行为上也是正确的。第 1 轮的发现 1 仍然存在,仍然是我建议合并前修掉的唯一一点。 本轮新增一个发现(层级边界断崖)和两个新的存活变异体。

1. 问题依然可复现,PR 依然修好了

/statusline 100×12、/skills 100×18 的对比见上文英文部分的前两张图:修复前标题丢失、行与行互相覆写((Project)t)(Project)oject)[locked: User]]),修复后是干净的对话框。

高度扫描表见上文(单元格 = 可见可选行数 / 检测到的覆写残影数)。PR 臂在所有尺寸下零残影;BEFORE 臂从 14 行起就开始出现。Manage Skills 标题在 BEFORE 臂上于我测试的所有高度(含 40 行)都被裁掉——换行的描述行把对话框顶部挤出了 overflow="hidden" 盒子;PR 之后从 12 行起标题都在(12 行以下是设计上的 bare 层)。

/statusline 24 行那一格需要正确解读:BEFORE 的 7 是「画了七行、其中几行是坏的」,AFTER 的 3 是三行真正正确的内容。24 行终端的对话框预算是 19,完整布局的固定开销恰好是 15,所以 4 项是诚实的数字。

46 列的宽度感知同样成立——修复前的对话框把页脚画进自己的边框并且整个列表消失(见上文第三张图)。

审阅者测试计划在本 head 上四步全部复现:紧凑布局下 16 次 Down 可从 Use theme colors 走到 session-id;100×12 下 Down×6 → SpaceEnter 把含 total-input-tokensui.statusLine.items 写入用户设置且状态栏同帧生效;100×10 的 /skills bare 层不会用隐藏查询过滤,DownSpaceEsc{"skills":{"disabled":["proj-beta"]}} 持久化到工作区设置。

2. 与 #10600 的合并解决在行为上是正确的

/resolveinitialResolved = resolveSkillSettings(settings) 换成了 config.isSkillEnabled(skill),并从 computeWorkspaceSkillListUpdates 调用里去掉了 lockedNames / defaultDisabled,与 main 的新签名一致。我没有只看 diff,而是看可观察后果:在工作区作用域预置 skills.disabled: ["proj-beta","proj-gamma"] 后打开 /skills,PR 构建把这两行渲染为未勾选。正确。(BEFORE 臂在该尺寸下根本读不出来——行被覆写了。)

我还在 40 行、没有任何高作用域锁定的情况下对比了两臂:唯一差别是 PR 把多行描述折叠成一行并截断,而不是换行成两行。对于 ~/.qwen/settings.json 等处没有 skills.disabled 的用户,本 PR 是纯粹的改进,没有信息损失。

3. 发现 1(沿用上轮,仍然存在)——锁定技能区块在任何终端尺寸下都被丢弃

SkillsManagerDialog.tsx:229 判断的仍然是「预算是否存在」而不是「预算够不够」(const constrained = availableTerminalHeight !== undefined;,随后 :245if (constrained) return [];)。

本轮我拿到了比上轮更锋利的证据。在 100×40(40 行终端、对话框预算 35、完全没有高度压力)下搜索一个被锁定的技能:main 回答 4 / 27 skills 并逐条指出是哪个作用域锁住了它;本 PR 回答 0 / 27 skills + No skills match the search.,而同一行副标题却写着 (+4 locked)。用户在产品内再也无法得知究竟是哪个设置作用域压住了某个技能。(见上文第四张图。)

我也找到了唯一的逃生口,但它没有用。对这个对话框来说 availableTerminalHeight 只有在 uiState.constrainHeight === false 时才是 undefinedDialogManager.tsx:84/504),而在对话框已打开时按 Ctrl+S 确实会清除它——锁定行会出现。但 AppContainer.tsx:4663-4665 会在下一次按键时把它设回 true,所以你为了搜索锁定技能而敲下的第一个字符就会让它们再次消失(见上文第五张图)。

准确的说法是:你可以瞥见锁定列表,但无法搜索它,而 [locked: <scope>] 标签只在一个任何按键都会摧毁的状态里可见。本 PR 从九个语言包中删掉的 All available skills are locked at a higher scope (see below). 是被彻底移除而非搬到别处。

影响面是有界的——只有当 SystemDefaults/User/System 作用域的 skills.disabled 里有内容时 lockedSkills 才非空(buildHigherDisabled:98-118),而对话框自己只写 Workspace 作用域。所以受影响的是手工编辑过用户设置的人和受管理员/系统策略约束的人,不是默认安装。但对这部分用户来说,这在所有终端高度上都是相对 main 的回归。

建议的修法不变且很小:让折叠由预算大小决定,而不是由「存在预算」决定——只要 residual 还放得下就保留锁定行,放不下时再退回 (+N locked) 徽标(这个徽标本身是真正的改进)。

4. 发现 2(同源)——计数契约不自洽

SkillsManagerDialog.tsx:542-543totalCount = allSkills.length 包含锁定技能,而 matchedCount = filteredUnlocked.length + filteredLocked.length 不可能包含,因为受限路径上 filteredLocked 恒为 [],于是就有了上面的 0 / 27。另外 (+4 locked) 是作为「额外」附加在一个已经包含这 4 个的总数后面,读起来像 31。两者都会随发现 1 的修复一并解决。

5. 发现 3(本轮新增)——层级边界断崖,仅 PR 存在

把终端高度增加一行反而可能让列表变短,因为跨过层级边界会重新装上一个比你新得到的那一行更贵的外框。PR 臂实测(未过滤):/statusline 20 行 → 9 项,21 行 → 1 项,22 行 → 1 项;main 对应是 4 / 5 / 6,单调。/skills 16 行 → 6 项,17 行 → 1 项,18 行 → 2 项。(见上文第六张图。)

没有任何内容被裁剪——21 行的外框恰好落在预算内——所以这不是本 PR 要修的那个 bug,main 在这里单调也只是因为它直接溢出了。但 21 行时对话框花 15 行外框只为显示一个选项,而它自己的紧凑布局在低一行时能显示九个;而且 21–26 行正落在本 PR 所针对的「短终端」区间里。给层级切换加一个下限(在完整布局至少能显示 N 项之前保持紧凑)就能消除它。我认为这属于后续项而非阻塞项。

6. 测试强度——18 个变异体,杀死 12 个

四个被改动文件的 70 个测试全部通过,DialogManager.test.tsxshared/layouts/layoutUtils.test.tsconfig/skill-settings.test.ts 的 461 个测试也全部通过。packages/clitsc --noEmit 干净,八个改动文件的 eslint 也干净——PR 描述里「包级 TypeScript 检查仍被阻断」的说明在本 head 上对 packages/cli 已不再成立。完整变异体表格见上文英文部分。

其中三项值得处理:

  • M16 —— SkillsManagerDialog.test.tsx:52isSkillEnabled mock 成 vi.fn().mockReturnValue(true),于是 feat: manage workspace-scoped extension skill state #10600 合并引入的这个生产依赖在测试里是惰性的:把整个判定换成 () => true,26 个测试全绿。我已手工验证了真实行为(见第 2 节),但 CI 抓不到它退化。补一个「工作区禁用了某技能」的用例即可闭合。
  • M17 —— 预算类测试全部是 toBeLessThanOrEqual,从来没有下界,所以没有任何测试钉住紧凑层真的把它省下的 6 行用起来了。把紧凑层的 frameRows 改回完整外框,/skills 在 100×16 下从 6 行可选变成 1 行、浪费 5 行空白,而整套测试依然全绿(见上文第七张图)。/statusline 套件已经有对应用例(uses every available row in an intermediate compact layout),/skills 套件缺这一条。
  • M10 / M11 —— 沿用上轮且未变化。closes on escape in the compact layout 在空查询下运行,bypasses a hidden query in compact mode and restores it 从未在紧凑状态下真的输入过,因此这两个守卫都没有被见证。发布代码这两点都是对的(我用实时缩放往返复核过),纯属缺测试,而且实践中「对话框打开时缩放终端」就能触达。

M13 是等价变异体,我不会为它要求补测试:把 barej/k 的提前返回里去掉后,这两个键会落到可打印字符分支,而该分支本身就被 !bare 挡住;同时 MultiSelect 通过自己的订阅照样收到这次按键。行为完全相同,这个守卫是冗余而非承重。M18 是良性的10 这个上限是产品取舍,调高它在任何预算下都不会溢出。

总结

在本 head 上是否修复 #9037 ✅ 在 8/10/12/14/16/18/20/24/30/40 行及 46 列下端到端验证通过
审阅者测试计划 ✅ 四步全部复现
#10600 合并冲突解决 ✅ 工作区作用域禁用渲染正确
无锁定技能的场景 ✅ 相对 main 是纯粹改进,无信息损失
相对 main 的回归 ⚠️ 锁定技能区块在所有尺寸下不可搜索(F1);matchedCount / (+N locked) 不自洽(F2)
本轮新增 ⚠️ 层级边界断崖:20→21 行少 8 个预设行(F3,后续项)
测试 70/70 + 461 邻近用例通过;tsceslint 干净;18 个变异体杀死 12 个
建议 合并前修掉 F1(一个条件,用的还是文件里已经算好的值);F2 随之解决;F3、M16、M17 作为后续项

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(cli): /statusline dialog is clipped in short terminals

4 participants