feat(cli): OpenTUI migration foundation batch — theme, a11y, clipboard, keys, dialogs scaffolding - #10146
feat(cli): OpenTUI migration foundation batch — theme, a11y, clipboard, keys, dialogs scaffolding#10146chiga0 wants to merge 13 commits into
Conversation
|
Thanks for the PR! Re-run on the retargeted diff — gate re-checked against the current head. Template looks good ✓ Problem: unchanged from the prior pass — this is the Foundation-modules batch of the OpenTUI migration tracked in #8662. The motivation is observed, not theoretical (flicker/mouse/renderer-maintenance reports against the patched ink build: #6137, #8580, #8659), and the maintainer explicitly approved the Phase 1 batched landing in #8662. Direction: aligned and endorsed. Since the last pass the stack resolved: #10134 merged and this PR retargeted to main via a clean merge (zero conflicts), so this review judges the final diff against main rather than a stacked base. Size: 54 files, +11,165 / −4. Breakdown: 5,785 production lines (5,781 additions — 26 all-new modules plus four one-line Approach: the shape is right — additive leaf modules, colocated tests, no reachable consumer yet, so the default ink path stays untouched by construction. Both Stage 1 observations from the prior pass are resolved: the tracking table in #8662 now reflects the pulled-forward dialog scaffolding, and the #10134 base landed. The four touches on existing code are each a one-line Risk: no elevated risk signals (no high-risk path matches). Moving on to code review. 🔍 中文说明感谢贡献！本次为改向（retarget）后 diff 的复跑——针对当前 head 重新过门禁。 模板完整 ✓ 问题：与上一轮一致——这是 #8662 跟踪的 OpenTUI 迁移中 Foundation modules 批次。动机是已观测到的问题而非理论假设（打补丁的 ink 构建上的闪烁/鼠标/渲染器维护报告：#6137、#8580、#8659），且维护者已在 #8662 明确批准 Phase 1 分批落地。 方向：对齐且已获背书。上轮之后叠层已解开：#10134 合入，本 PR 经干净合并（零冲突）改回 main，本次评审直接对 main 上的最终 diff 作判断。 规模：54 个文件，+11,165 / −4。拆分：5,785 行生产代码（5,781 行新增——26 个全新模块加四个既有文件上各一行的 方案：形态正确——纯增量叶子模块、就近测试、尚无任何可达消费者，默认 ink 路径在构造上不受影响。上轮的两点观察均已解决：#8662 跟踪表已反映提前落地的 dialog 脚手架；#10134 base 已合入。对既有代码的四处改动均为各一行的 风险：无升级风险信号（未命中高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review — re-run at
|
| Check | Conclusion |
|---|---|
Classify PR |
✅ success |
Dependency CVE audit |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Secret scan (TruffleHog) |
✅ success |
Test (ubuntu-latest, Node 22.x) |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
Sandboxed verification lanes: nothing to exercise yet — the batch has no reachable consumer, so /tmux has no TUI surface to drive, and the suite/build claims are now settled by the PR's own CI above rather than author self-report. When the backend batch wires these modules, /verify becomes the lane that matters.
中文说明
代码审查（复跑于 e0c4f14d）：上一轮的两个阻塞项均已修复并在源码中核验——slash-command-parse.ts（及其 465 行测试）已删除，slash-dispatch.ts 改为从原模块 ../commands/commands.js 导入 parseSlashCommand/parseStackedSlashCommands，注释与事实一致；dialog-scope-utils.ts 已删除，dialogs-theme.tsx 改为从 ../../config/dialogScopeUtils.js 导入（与 ink ThemeDialog 同源）。此后按维护者四条意见的加固（d09231cb11）也已就位且自洽：kitty 应答正则要求 flags 参数（回显的裸查询无法伪造 kitty 支持）、探针只保留 256 字节有界尾部缓冲、no-op 的迟到应答 drain 已移除、读屏 writer 在 appendStatic/updateDynamic 内部强制纯文本契约（剥离 ANSI + 去除 C0）。
本人对全部生产模块的复查未发现新的 Critical。亮点：安全敏感面完整保留而非近似 ink 的约束（OSC 8 scheme 白名单与标签欺骗后缀直接复用 utils/osc8.js、链接命中拒绝不安全 scheme、OSC 52 有 TTY 门控与 75KB 上限、代理凭据脱敏经新增 redactProxy 导出复用），复用方向一致正确——keyMatchers、commandMetadata、input-mouse（re-export 而非 fork）、退出清理链、sanitizeSensitiveText、成本/统计工具均取自原模块。测试扎实（496 个用例、无 skip、断言可观察行为——含 R1/R2 修复锚点如非 BMP 字符的 UTF-16 单元映射、读屏 writer 的 OSC 52 注入测试）。批次在构造上仍然不可达：opentui/ 之外无任何代码 import 它，四处既有文件改动均为纯 export 追加。
非阻塞、留给接线批次的记录：
- quit 载荷契约（独立静态复审同样提出）：
quit效果丢弃了QuitActionReturn.messages（ink 渲染的/quit回显与会话时长摘要）。接线前要么让效果携带投影文本（可复用现有notice模式），要么明确由 backend 重新合成。 - 若干 parity 缺口届时一并合入：
error/finished分支未发retry-countdown-clear、/context detail与/mcp schema投影丢失 ink 会渲染的明细/schema 段、SkillsList 名字截断差一列、model_fallback的模型名未过sanitizeDisplayText、help 布局的singleColumn是死代码（≥72 钳制后恒为 false）。 - 仅卫生问题：
dialogs-shared.tsx/dialogs-theme.tsx是packages/cli/src/ui下首批 kebab-case.tsx（AGENTS.md 约定组件文件为PascalCase.tsx）。.tsx并无强制规则、目录内部自洽，不构成阻塞——仅记录。
测试证据：CI 已在该 head 上运行。PR 的主张均被证实，除一项与本 PR 无关的红检查：Test (ubuntu-latest) 恰好失败于一个测试——web-shell 的 MessageList.dom.test.tsx（时序敏感的 DOM 断言）。该包与本 PR 完全无关：diff 中零 web-shell 文件、web-shell 不依赖 packages/cli、该测试文件相对 base 未改动。判定为既有 flake 而非 PR 缺陷。同 job 其余全部通过——所有 linter（含架构规则的 ESLint、Prettier、actionlint、shellcheck、yamllint、敏感词）、lockfile/依赖方向检查、构建，以及完整套件：core 25,913 通过、cli 22,168 通过（含本批 26 个 opentui 文件）。Windows/macOS 测试通道按 ci.yml 设计跳过（PR 触发器因既有 Windows 问题关闭）；集成测试不在 PR 通道。评审时 web-shell E2E Smoke 仍在运行；下表为快照，CI 落定后 finalize 任务会刷新。维护者重跑失败任务即可排除该 flake。
沙盒验证通道：目前无用户可见面——批次尚无可达消费者，/tmux 无可驱动的 TUI 面，测试/构建主张已由上述 CI 证实（不再是作者自述）。待 backend 批次接线后，/verify 才是关键通道。
— Qwen Code · qwen3.8-max
Reviewed at e0c4f14d628ee74a38f342ae9fe89b8d6526a6cf · re-run with @qwen-code /triage
|
Confidence: 4/5 — clean across every stage; the only reservations are non-blocking follow-ups for the wiring batch and one pre-existing CI flake that is not this PR's. Stepping back: the prior pass scored this 2/5 over two verbatim forks of modules the PR could import, plus zero CI evidence on that head. Both are resolved. The forks are gone — the batch now dispatches through the original What I still carry as reservations, none blocking a foundation batch with no reachable consumer:
Verdict: approve. One (One note for the thread: a bot 中文说明置信度：4/5 —— 各阶段均干净；仅有的保留是留给接线批次的非阻塞跟进项，以及一项与本 PR 无关的既有 CI flake。 整体看：上一轮因两处"本可直接 import 却逐字 fork"以及该 head 上零 CI 证据而给 2/5，两者都已解决。fork 已删净——批次现在走原 仍然保留的顾虑（均不阻塞一个尚无可达消费者的 foundation 批次）：
结论：批准。该 head 上还有一个 （线程说明：作者请求的一次 bot — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Two of the new modules are verbatim copies of existing, importable code — slash-command-parse.ts copies ui/commands/commands.ts and dialog-scope-utils.ts copies config/dialogScopeUtils.ts (~736 lines including the duplicated tests). For a parity migration the forked parser is a silent-drift hazard; both originals are already reachable from opentui (this PR's own imports prove it). Please delete the copies and import the originals. Also note: no CI run exists on this head yet, so the green-suite claim is unverified, and the base (#10134) still carries CHANGES_REQUESTED. Details in my stage-2/stage-3 comments. 🙏
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
|
Both Stage-1 observations addressed:
|
|
Both duplication findings fixed in a8e3a50 — as the Stage-3 ask summarizes:
Net: −738 lines (736 duplicated + 2 import rewires), one source of truth for both. The originals keep their existing coverage, so no parity test was lost — the duplicated suites went with the copies. The two non-blocking notes (dialog re-cut, CommandContext stub fold-in) are recorded: the re-cut is now reflected in #8662, and the stub fold-in stays planned for the backend batch. Local verification at a8e3a50: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): chunk 26: none — parity of mouse-caret.ts against the original utils/input-mouse.ts#visualClickToOffset it claims to port was not diffed line-by-line, but that source….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): 349 tests pass — this review observed 25097 passed; 99 tests pass — this review observed 25097 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）：chunk 26：none — parity of mouse-caret.ts against the original utils/input-mouse.ts#visualClickToOffset it claims to port was not diffed line-by-line, but that source…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：349 tests pass — this review observed 25097 passed; 99 tests pass — this review observed 25097 passed。
— qwen3.8-max via Qwen Code /review (v0.22.2)
R1 review round: fixes pushed (54edee0)All 19 Critical findings and the low-cost Suggestions are addressed in Highlights
Verification: Deferred (threads left open, replies explain scope)
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): 349 tests pass — this review observed 25127 passed; 99 tests pass — this review observed 25127 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/opentui/a11y-screen-reader.test.ts:244 — [review] The clearDynamic test never re-writes the pre-clear…packages/cli/src/ui/opentui/clipboard.ts:28 — [review] Hand-rolled tmux/screen DCS wrapping duplicates core's…packages/cli/src/ui/opentui/commands-output.ts:31 — [review] Added optional parameter now on messageToHistoryItem is…packages/cli/src/ui/opentui/dialogs-core.ts:166 — [review] applyNumberSelectKey's exact-boundary comparator…packages/cli/src/ui/opentui/dialogs-shared.test.tsx:60 — [review] No test moves the highlight between digit and flush, so…packages/cli/src/ui/opentui/dialogs-shared.test.tsx:61 — [review] The flush test never types after the timeout commit, so…packages/cli/src/ui/opentui/dialogs-shared.tsx:187 — [review] The resync path resets activeIndex/scrollOffset but leaves…packages/cli/src/ui/opentui/dialogs-shared.tsx:227 — [review] selectIndex fires onSelect but never onHighlight,…packages/cli/src/ui/opentui/dialogs-shared.tsx:243 — [review] The focused:false gate — the only protection against two…packages/cli/src/ui/opentui/dialogs-shared.tsx:369 — [review] DialogSelect gates visual selection on focused but not…packages/cli/src/ui/opentui/dialogs-theme.tsx:135 — [review] The Math.max(1,…) pane clamps plus the theme-mode header's…packages/cli/src/ui/opentui/early-input.test.ts:102 — [review] The successful-injection test never asserts polling stops…packages/cli/src/ui/opentui/early-input.test.ts:124 — [review] The 'stops retrying after maxAttempts' test asserts…packages/cli/src/ui/opentui/early-input.ts:48 — [review] The truncated-tail guard covers only CSI (ESC [); a…packages/cli/src/ui/opentui/event-adapter.test.ts:49 — [review] The tool-args and confirm channels have zero assertions —…packages/cli/src/ui/opentui/event-adapter.ts:70 — [review] The confirm event carries only {id, tool, title};…packages/cli/src/ui/opentui/event-adapter.ts:74 — [review] The compaction union member is producer-less and…packages/cli/src/ui/opentui/event-adapter.ts:110 — [review] The goal-legacy union member has no producer and no…packages/cli/src/ui/opentui/event-adapter.ts:330 — [review] The image channel skips ink's guards (non-image MIME, size…packages/cli/src/ui/opentui/event-adapter.ts:435 — [review] The 'Press Ctrl+Y to retry' hint is emitted…- …and 37 more (see the run report)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：349 tests pass — this review observed 25127 passed; 99 tests pass — this review observed 25127 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 57 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
… dialogs scaffolding Foundation batch of the OpenTUI migration tracked in QwenLM#8662. Adds the renderer-neutral foundation modules under ui/opentui: theme family, a11y (plain-text, screen-reader), clipboard, key-map, mouse hit/caret, link-click + osc8 parity, early-input, exit guard/lifecycle, kitty negotiation, event-adapter, item-projection, slash dispatch (+ command parsing), commands context/output, help content, input history, and the dialog scaffolding primitives (core/shared) with the theme dialog. Two helpers land inside ui/opentui rather than utils/ to respect the utils leaf-layer rule (QwenLM#9737). Stacked on the infra batch: consumes ui/model streaming model and @OpenTui deps. No reachable ink code changes beyond a one-line export addition in the shared osc8 module.
Upstream renamed setGeminiMdFileCount to setMemoryFileCount in the command UI contract; the rebase onto main surfaced the mismatch at build. Rename the host interface member, the bridge wiring, the dispatch stub, and the test mock to match.
54edee0 to
8e9a6c7
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Rebased onto main now that the infra batch has merged (#10134 → Verified on the rebased head All 38 round-1 findings remain addressed at this head; the review threads were anchored to pre-rebase commits and will read as outdated. |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
@qwen-code-ci-bot /review |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Independent pass at 8e9a6c7, scoped to new ground — not repeating the 28 open threads. Local verification on this head (tarball checkout, deps from the unchanged lockfile): full src/ui/opentui/ suite 339/339 across 25 files, tsc --noEmit clean in packages/cli. Four new findings inline, concentrated in kitty-negotiation.ts (reply regex matches its own query; the late-reply drain is a no-op listener; unbounded buffer re-scan) and the screen-reader writer (plain-text contract not enforced — stripAnsi has zero callers in this batch). The two edits to existing files are export-only and safe; clipboard OSC 52 payloads are base64-wrapped and the OSC 8/link paths reuse the existing scheme allowlist, so those surfaces look fine.
Round-2 review fixes (17 Critical + 10 Suggestion resolved in code):
- dialogs-shared: move number-select flush out of the setState updater
(StrictMode double-fires onSelect); split setActiveIndex (ink
SET_ACTIVE_INDEX, lands on any in-range row) from highlightIndex
(arrow keys skip disabled rows) so wheel navigation never sticks
- event-adapter: chat_compressed notice mirrors ink formatCount ('~'
prefix for estimated counts); vision_bridge_notice renders
summary\nnotice; explicit projections for task_execution /
findings_list / terminal_image keep multi-MB payloads off the
transcript; retry-countdown-clear forwards isContinuation
- slash-dispatch: isSlashCommandInput drops the '?' branch (ink gate
routes ? input to the model); executeSlashCommand races the action
against the abort signal; dialog effects carry the
OpenDialogActionReturn payload; projected added-item text surfaces
alongside non-handled effects (notice); message-shaped items project
to their text; ui.history comes from env; absent sessionStats stamp
now, not epoch; telemetry parity (recordSkillInvocation /
recordAutoSkillCommandUsage / makeSlashCommandEvent)
- item-projection: model stats render per-(model,source) sections with
N/A for unpriced entries; Tool Calls line uses ASCII x like ink;
redactProxy deduplicated via systemInfoFields export
- theme: palette/syntax colors resolve through color-utils toHex before
parseColor (ink CSS names / *bright names no longer degrade to
magenta); unresolvable values stay unset
- key-map: kitty 'kpenter' normalizes to 'return'; resolveCommands
exposes ink's key fan-out (Ctrl+C fires QUIT + CLEAR_INPUT)
- a11y: hardWrap delegates to wrap-ansi (word-boundary parity with
ink's screen-reader path); markdown reducer tracks fence length,
keeps fence-like lines literal inside fences and inner backticks in
multi-backtick spans; stripAnsi delegates to strip-ansi plus a
private-parameter CSI pass (SGR mouse, DEC save/restore)
- clipboard: OSC 52 write gated on a TTY (stderr preferred), tests spy
the stream instead of writing real sequences to the runner's terminal
- exit-guard: independent per-key arm windows like ink
- dialogs-theme: diff preview pane receives syntaxStyle/filetype
…review - kitty-negotiation: KITTY_REPLY_RE requires at least one flag digit (\d+), so an echoed bare query \x1b[?u in PTY/CI environments no longer resolves true and locks the renderer into kitty mode on a terminal that never answers queries; the accumulation buffer keeps only a 256-byte tail (bounded memory, bounded rescan under byte floods); the settle-window drain is removed — an EventEmitter data listener cannot consume chunks from other listeners, so late replies flow to the renderer's input parser like any other terminal noise - a11y-screen-reader: ScreenReaderOutputWriter sanitizes written content (stripAnsi + drop bare C0/C1 controls, keep newlines) so the plain-text-only contract is enforced at the writer instead of trusting every future caller — smuggled OSC 52 clipboard writes or title/cursor sequences cannot execute on the main screen
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM on re-review at e0c4f14. All four of my earlier findings are fixed in d09231c and verified in source:
- Kitty reply regex now requires at least one flag digit, so an echoed bare query can no longer fake kitty support (echo + split-chunk tests added).
- The no-op late-reply drain and settle window are removed; the comment now states the actual contract (late replies reach the renderer's input parser).
- The probe buffer keeps a 256-byte tail — bounded memory and bounded rescans, with a byte-flood test; worst case fails safe toward legacy keys.
- The screen-reader writer enforces its own plain-text contract via sanitize() (stripAnsi + bare C0/DEL drop) in both appendStatic and updateDynamic, with a smuggled-OSC-52 test.
Also spot-checked the R2 batch (theme colors resolved through toHex before parseColor; kpenter aliased to return) — both match their findings. Local verification on this head: full opentui suite 375/375 across 25 files (up from 339 — the fixes carry their tests), tsc --noEmit clean in packages/cli.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Independent static review @ head e0c4f14 / 独立静态复审 @ head e0c4f14
Pure static review (no tests executed). Four module batches (a11y + theme / input & terminal-negotiation security / stream-event adapter & history projections / slash dispatch & dialogs) walked all 55 files. The exclusion context was extracted up front from the two bot review rounds (35 findings), the 57 deferred convergence-posture items, and the maintainer's four findings from earlier today — none are re-reported here. The head is a clean merge of main (zero combined hunks), and every prior-round fix was re-verified in the head tree.
纯静态审查（未运行测试），4 个模块批次（a11y+theme / 输入与终端协商安全批 / 流事件适配与历史投影 / slash 分发与 dialogs）走查全部 55 个文件。排除集事先提取自两轮 bot 审查（35 条）、57 条延后项以及今天早些时候维护者的 4 条发现——本文不重复这些条目。head 是干净的 main 合并（零 combined hunks），历轮修复均在 head tree 上逐条复验。
Prior fixes re-verified / 既有修复核验
The forked copies are genuinely gone: slash-dispatch.ts:45 now imports parseSlashCommand from ../commands/commands.js and dialogs-theme.tsx imports from ../../config/dialogScopeUtils.js, with no residue tree-wide. The maintainer's four findings (kitty reply-regex self-match, no-op drain listener, unbounded probe buffer, screen-reader writer sanitization) are all fixed and coherent at this head. The security surfaces were exercised adversarially and hold: clipboard platform fallbacks pipe text via stdin.write with fixed argv (no shell interpolation), OSC 52 payloads are base64-wrapped with the 75KB cap and TTY gate, and the link-click scheme gate rejects javascript:/file:/case-folding/full-width disguises. All 20 stream-event members and all 14 ToolResultDisplay members were checked against core's actual shapes with no silent drops.
fork 副本确实删净：slash-dispatch.ts:45 现从 ../commands/commands.js 导入 parseSlashCommand，dialogs-theme.tsx 从 ../../config/dialogScopeUtils.js 导入，全树无残留。维护者的 4 条（kitty 应答正则自匹配、no-op drain、无界探针缓冲、读屏 writer 净化）均已修复且自洽。安全面经对抗性推演成立：剪贴板平台回退全部经 stdin.write 管道、固定参数数组（无 shell 拼接），OSC 52 载荷 base64 包裹、75KB 上限、TTY 门控，link-click 的 scheme 门禁拒绝 javascript:/file:/大小写伪装/全角伪装。20 个流事件成员与 14 个 ToolResultDisplay 成员均与 core 实际形状核对，无静默丢弃。
New findings (no Criticals) / 新发现（无 Critical）
🟡 Important
-
slash-dispatch.ts:296-297— thequitarm returns bare{ kind: 'quit' }and dropsQuitActionReturn.messages, where quitCommand computes the/quitecho item and the session-duration summary that ink renders viasetQuittingMessages. TheSlashEffectquit variant carries no payload and exit-lifecycle only runs cleanup, so this output is permanently lost under the new renderer unless the backend is contractually expected to re-synthesize it — nothing in the contract says so. Suggest carrying the projected messages on the effect (the existingnoticefield pattern fits) or documenting backend responsibility explicitly. Confidence: medium (may be intentionally deferred to the backend batch).quit 分支返回裸
{ kind: 'quit' }，丢弃QuitActionReturn.messages——quitCommand 在其中计算/quit回显条目与会话时长摘要，ink 经setQuittingMessages渲染。SlashEffect的 quit 变体无载荷、exit-lifecycle 只做清理，除非契约约定后端重新合成（当前无任何承载），该输出在新渲染器永久丢失。建议随 effect 携带投影后的文本（可复用notice字段模式）或显式注明后端职责。置信度：中（可能是有意留给 backend 批次）。
🟢 Nits
-
event-adapter.ts— theerror(~468-485) andfinished(~665-678) branches never emitretry-countdown-clear, while the other four sites do; ink clears unconditionally inhandleErrorEvent(use-llm-stream.ts:2110-2111) and inhandleFinishedEvent(:2228). A terminal event inside the live countdown window leaves the stale↻row. Fix: emit the clear in both branches.error与finished分支不发retry-countdown-clear，其余四处都发；ink 在handleErrorEvent（use-llm-stream.ts:2110-2111）无条件清、handleFinishedEvent（:2228）也清。倒计时窗口内到达终态会残留过期↻行。两个分支补发 clear 即可。 -
item-projection.ts:414/462—projectContextUsageusesshowDetailsonly to toggle the hint line; ink's ContextUsage (views/ContextUsage.tsx:472) renders the per-item detail sections (built-in/MCP tools, memory files, skills) plus the compaction-thresholds block./context detailtranscripts end up with strictly less information. Fix: project the detail arrays andbreakdown.thresholds.projectContextUsage只用showDetails切换提示语；ink ContextUsage 在该标志下渲染逐条明细（内置/MCP 工具、记忆文件、技能）与压缩阈值块。/context detail的转录反而信息更少。建议投影明细数组与breakdown.thresholds。 -
item-projection.ts(projectMcpStatus) — ignores theshowSchema/showTipsfields carried byHistoryItemMcpStatus(ui/types.ts:439); ink's McpStatus.tsx:191 prints parameter schemas and tips./mcp schematranscripts are indistinguishable from/mcp.projectMcpStatus忽略HistoryItemMcpStatus（ui/types.ts:439）携带的showSchema/showTips；ink McpStatus.tsx:191 会打印参数 schema 与提示。/mcp schema转录与/mcp无差别。 -
slash-dispatch.ts:428—settings: env.settings ?? nullcontradicts the realCommandContext.services.settingstype (non-nullLoadedSettings, commands/types.ts:63) and is bridged withas unknown as. If the backend omits settings, commands reading.mergedthrow a TypeError surfaced as a generic command failure. Suggest makingenv.settingsrequired.settings: env.settings ?? null与真实类型矛盾（CommandContext.services.settings为非空LoadedSettings，commands/types.ts:63），靠as unknown as绕过。后端漏传时读.merged的命令抛 TypeError，表现为泛化失败。建议env.settings设为必填。 -
help-content.ts:185—singleColumnis dead:safeWidthis clamped to ≥72, sodescWidth < 4is always false (the test itself assertscomputeHelpWidthLayout(1).singleColumn === false), and ink's original has no single-column mode. Drop the field or evaluate it before the clamp.singleColumn永假：safeWidth钳到 ≥72 后descWidth < 4恒为 false（测试自身断言computeHelpWidthLayout(1).singleColumn === false），且 ink 原版无单列模式。删除该字段或在钳制前判断。 -
early-input.ts:45— truncated SS3 leaks a bareO: the SS3 strip requires the final byte and the truncated-tail fallback covers only CSI (ESC [). If startup capture stops at the second byte of a 3-byte F1-F4, oneOlands in the composer. Add a/\u001BO$/fallback symmetric with line 48.截尾 SS3 泄漏单个
O：SS3 剥离要求 final 字节，截尾兜底只覆盖 CSI（ESC [）。启动捕获恰在 F1-F4（3 字节）第二字节停止时，composer 会多出一个O。补一条与 48 行对称的/\u001BO$/。 -
link-click.ts:135/174—readBufferRowtrims trailing whitespace fromtextbut not fromcellColumns, so a URL ending at end-of-row with a full-width final character misses clicks on the right half of that cell (fallback bound islastColumn + 1, covering only the left half). UX-only; keep the two arrays length-aligned or record per-character next-column.readBufferRow裁掉text尾部空白但cellColumns不同步，行尾以全角字符结尾的 URL 点击其右半格不命中（回退边界lastColumn + 1只覆盖左半）。纯 UX；保持两数组等长或记录每字符的下一列起点。 -
kitty-negotiation.ts:134-136(Confidence: low) — the two probestdout.writecalls are unguarded; if one throws synchronously (e.g.ERR_STREAM_DESTROYEDwhileisTTYstill reports true),finish()never runs: raw mode is not restored and the data listener leaks. Wrap the writes in try/catch callingfinish(false).探针的两次
stdout.write无保护；若同步抛错（如isTTY仍为 true 时ERR_STREAM_DESTROYED），finish()永不执行：raw 模式不恢复、监听器泄漏。用 try/catch 包裹并走finish(false)。
💡 Suggestions
-
a11y-screen-reader.ts:137-150— the hand-rollederaseLinesis byte-identical toansi-escapes'eraseLines, which the repo already imports elsewhere (ui/utils/terminal-resize-reflow.ts:299). Reuse it.手写
eraseLines与ansi-escapes同名助手字节级等价，仓库已在 ui/utils/terminal-resize-reflow.ts:299 引入该库，可直接复用。 -
help-content.ts:138—truncateTextis a zero-value passthrough wrapper of the exportedtruncateHelpText; call the original directly.truncateText是对已导出truncateHelpText的零价值透传包装，调用点直接用原函数。 -
item-projection.ts:179— the SkillsList truncate is off by one (projection total length n+1 vs ink's n viaslice(0, max-1) + '…'), shifting the description column by one cell whenever a name truncates.SkillsList 截断差一字符（投影总长 n+1，ink 为
slice(0, max-1) + '…'总长 n），名字截断时描述列整体右移一格。 -
event-adapter.ts:601—model_fallbackmodel names skip ink'ssanitizeDisplayText(use-llm-stream.ts:2800-2801); low injection probability, but it is a sanitization gap vs the claimed parity.model_fallback的模型名未过 ink 的sanitizeDisplayText；注入概率低，但与声明的 parity 相比是消毒缺口。 -
dialogs-shared.tsx:242—selectIndex(mouse click) firesonSelectwithoutonHighlight; ink dispatches SET_ACTIVE_INDEX + SELECT_CURRENT so the highlight callback precedes select. Harmless for the current close-on-select consumer; a future stay-open dialog driven by highlight would desync on mouse input.selectIndex（鼠标点击）只触发onSelect不触发onHighlight；ink 依次 dispatch SET_ACTIVE_INDEX + SELECT_CURRENT，highlight 回调先于 select。当前"选中即关"消费者无影响，后续依赖 highlight 驱动预览且保持打开的对话框会在鼠标输入下失同步。 -
Two low-confidence edges:
event-adapter.tsmcp_appfalls through toJSON.stringifyiffallbackTextis ever absent (currently unreachable; contradicts the "HTML never enters output" comment), anditem-projection.tsprojectAboutshows the Base URL line whenselectedAuthTypeis the empty string where ink hides it.两条低置信边界：
mcp_app在fallbackText缺省时落入JSON.stringify（当前不可达，但与"HTML 绝不进输出"注释矛盾）；projectAbout在selectedAuthType为空串时多打一行 Base URL（ink 隐藏）。
Positives / 值得肯定
The fix round is genuinely high quality: numeric quick-select now runs onSelect from a ref outside setState updaters with a StrictMode single-fire test, setActiveIndex/highlightIndex are correctly split, the abort race cleans up without dangling rejections, and telemetry ordering on throw matches ink's catch path. Every module documents the ink behavior it reproduces, and the tests assert observable behavior rather than mirroring implementations.
修复轮质量很高：数字快选经 ref 在 setState updater 之外执行 onSelect 并配 StrictMode 单次触发测试，setActiveIndex/highlightIndex 拆分正确，abort race 清理无悬挂 rejection，抛错路径的遥测顺序与 ink catch 一致。每个模块都注明其复刻的 ink 行为，测试断言可观察行为而非镜像实现。
CI
Test (ubuntu) is still in progress at this head; everything else completed is green or skipped.
head 上 Test (ubuntu) 仍在运行，其余已完成项为绿或跳过。
Verdict / 结论
💬 Comment — zero new Criticals from this pass. The one Important (quit payload contract) and the parity gaps are worth pinning down before the backend batch wires these effects, but they do not block a foundation batch that has no reachable consumer yet.
💬 评论——本轮零新增 Critical。1 条 Important（quit 载荷契约）与若干 parity 缺口值得在 backend 批次接线前钉死，但不阻塞这个尚无可达消费者的 foundation 批次。
…ation All 15 findings from the independent static review verified in source and fixed (no false positives; none deferred): - quit effect carries QuitActionReturn.messages projected to text on a notice field — ink renders them via QuittingDisplay and the payload was permanently lost (Important #1) - error and finished branches emit retry-countdown-clear like ink's handleErrorEvent/handleFinishedEvent, so a terminal event inside the countdown window no longer leaves a stale retry row (#2) - projectContextUsage renders the compaction-threshold ladder and the per-item detail sections (tools/memory/skills, ink's sort order) when showDetails is on — /context detail transcripts no longer show strictly less than the compact view (#3) - projectMcpStatus honors showSchema (parameter JSON under each tool) and showTips, so /mcp schema is distinguishable from /mcp (QwenLM#4) - SlashDispatchEnv.settings is required: the real CommandContext.services.settings is non-null and a null surfaced as a generic command failure on first .merged read (QwenLM#5) - dead singleColumn flag removed from the help width layout (clamp makes it always false; ink has no single-column mode) (QwenLM#6) - truncated SS3 tail (bare ESC O) is stripped like the truncated CSI tail, so a captured half F1-F4 no longer leaks 'O' into the composer (QwenLM#7) - readBufferRow trims cellColumns alongside text, so URLs ending at end-of-row on a wide character hit-test on both halves of the cell (QwenLM#8) - kitty probe writes guarded: a synchronous stream throw settles the probe (restores raw mode, removes the listener) instead of leaking (QwenLM#9) - eraseLines reuses the ansi-escapes helper (already a repo dependency) instead of a byte-identical hand-rolled copy (QwenLM#10) - truncateText passthrough wrapper dropped; callers use the exported truncateHelpText directly (QwenLM#11) - SkillsList truncate keeps total length n like ink, so the description column no longer shifts by one cell when a name truncates (QwenLM#12) - model_fallback names pass through sanitizeDisplayText like ink (QwenLM#13) - selectIndex fires onHighlight before onSelect (ink dispatches SET_ACTIVE_INDEX then SELECT_CURRENT), keeping highlight-driven stay-open dialogs synced on mouse input (QwenLM#14) - mcp_app without fallbackText renders empty instead of JSON-dumping the embedded HTML; projectAbout hides Base URL when selectedAuthType is empty, matching ink's formatBaseUrl (QwenLM#15)
…at/tui-foundation-batch
|
Thank you @yiliang114 for the approval and the verified re-review, and @ytahdn for the independent static pass — all 15 findings were checked against the source (every one held; none deferred) and fixed in ff19f4f, pushed on top of the head you reviewed:
378/378 opentui tests, typecheck, and lint are green on the pushed head; regression tests added for the quit notice, both countdown clears, the context detail sections, and the mcp schema/tips paths. |
|
Heads-up @yiliang114: pushing the review-response fixes (ff19f4f, merged into ac1278f) auto-dismissed your approval, so the PR is back to review-required. The head diff since your approval is exactly the 15 ytahdn-finding fixes described in my comment above — no other changes. Full CI is green on ac1278f (Test/Desktop Shell/web-shell E2E/CVE/secret scan). Could you re-approve when convenient? |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-approved at ac1278f after the ytahdn-review delta (ff19f4f). Reviewed every changed module in the delta: the eraseLines swap to the shared ansi-escapes helper, the mouse-select highlight ordering fix, the ESC-O partial-tail strip in early-input (also closes the residual I noted in my first pass), the event-adapter parity fixes (mcp_app empty-fallback no longer dumps raw HTML, retry-countdown clears on error/finished, sanitized fallback model names), the help-content cleanup, the item-projection fixes plus the compaction-ladder/detail/schema projections with Number() coercion, the kitty probe's fail-settled write guard, the wide-char cellColumns trim in link-click, and the slash-dispatch settings-required + quit-notice projection changes. Local verification on this head: full opentui suite 378/378 across 25 files (up from 375), tsc --noEmit clean in packages/cli. Prior findings remain fixed; no new issues in the delta.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: the executable-script lint — the report is stale or its diff could not be verified; re-run qwen review script-lint.
Not reviewed: 35 of the diff's 36 sections, the linked-issue fidelity pass, the whole-diff test-coverage check, the removed-behavior audit, the cross-file consistency pass, the build-and-test check — its prompt was built, but no agent on record was launched with it.
Not reviewed: 1 of the diff's 36 sections — no record shows its brief reaching an agent, so this dimension was reviewed, if at all, from a prompt the run wrote for itself.
Not reviewed: verification and reverse audit — both prompts were built, but no agent was launched with either — the posted findings cannot be counted as verified, and the pass that hunts what the rest of the review missed cannot be certified.
Convergence: round 3 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 28 (24 new). Findings keep coming back to the same files: packages/cli/src/ui/opentui/slash-dispatch.ts (findings in rounds 1, 2; 2 more now); packages/cli/src/ui/opentui/a11y-plain-text.ts (findings in round 2; 1 more now); packages/cli/src/ui/opentui/a11y-screen-reader.ts (findings in round 2; 1 more now), and 1 more file(s). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
未审查：the executable-script lint — the report is stale or its diff could not be verified; re-run qwen review script-lint。
未审查：diff 36 个片段中的 35 个、关联 issue 一致性检查、全 diff 测试覆盖检查、删除行为审计、跨文件一致性检查、构建与测试验证——它的 prompt 已构建，但没有任何 agent 有记录用它启动过。
未审查：diff 36 个片段中的 1 个——没有记录表明它的 brief 到达过任何 agent，这个维度即便被审查过，也是基于这次 run 自行编写的 prompt。
未审查：验证与反向审计——两份 prompt 都已构建，但都没有 agent 用它们启动——发布的发现不能算作已验证，搜寻评审遗漏问题的工序也无法作证。
收敛情况：第 3 轮发布了 5 条行内评论，其中 5 条是首次提出；上一轮发布了 28 条（其中 24 条首次提出）。发现反复回到同一批文件：packages/cli/src/ui/opentui/slash-dispatch.ts（第 1、2 轮已出过发现，本轮又有 2 条）；packages/cli/src/ui/opentui/a11y-plain-text.ts（第 2 轮已出过发现，本轮又有 1 条）；packages/cli/src/ui/opentui/a11y-screen-reader.ts（第 2 轮已出过发现，本轮又有 1 条），另有 1 个文件。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
- executeSlashCommand catch checks the abort signal first: an ESC- cancelled command (action rejects AbortError) returns handled with no failure telemetry or error message, mirroring ink's processor — the race promise never resolves when the signal is already aborted at addEventListener time - submit effect carries the full SubmitPromptActionReturn contract (modelOverride, onComplete, refreshContextFilesOnWrite) so the backend can honor /model <id> <prompt>, /dream's manual-run record, and /remember's context refresh like ink instead of silently degrading them - closing fences cannot carry info text (CommonMark): a ```js line inside an open block is literal body, not an early close that drops the block and inverts parse state for the rest of the document - info items append their linkUrl/linkText footer (ink's InfoMessage renders it; headless/SSH users need the printed URL, e.g. /bug) - the screen-reader sanitize keeps TAB: it separates words in tool/model output and deleting it fused adjacent tokens
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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): chunk 20: executing packages/cli/src/ui/opentui/item-projection.test.ts (no installed dependencies in the shared review worktree; installing/building the monorepo excee…; "agent reverse-audit (round 2)": did not locate core's AgentEvent→ServerGeminiStreamEvent bridge to verify whether live per-tool progress updates (subagent task_execution 'running' displays, …; chunk 9: verifying e.scroll?.direction , MouseButton.LEFT , and the onMouseScroll / onMouseOver / onMouseUp prop typings against the installed @opentui/core@0.5.8 …; chunk 8: could not execute dialogs-shared.test.tsx (no node_modules in the review worktree); the numeric-flush finding was verified by reading the code paths at HEAD…; chunk 31: execute slash-dispatch.test.ts under vitest to confirm green (blocked: worktree lacks node_modules and built dist; install+build exceeds tool budget), and 1 more.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): 349 tests pass — this review observed 25927 passed; 99 tests pass — this review observed 25927 passed.
Convergence: round 4 posted 71 inline comment(s), 71 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/cli/src/ui/opentui/item-projection.ts (findings in round 3; 13 more now); packages/cli/src/ui/opentui/slash-dispatch.ts (findings in round 3; 10 more now); packages/cli/src/ui/opentui/a11y-plain-text.ts (findings in round 3; 4 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 20：executing packages/cli/src/ui/opentui/item-projection.test.ts (no installed dependencies in the shared review worktree; installing/building the monorepo excee…；"agent reverse-audit (round 2)"：did not locate core's AgentEvent→ServerGeminiStreamEvent bridge to verify whether live per-tool progress updates (subagent task_execution 'running' displays, …；chunk 9：verifying e.scroll?.direction , MouseButton.LEFT , and the onMouseScroll / onMouseOver / onMouseUp prop typings against the installed @opentui/core@0.5.8 …；chunk 8：could not execute dialogs-shared.test.tsx (no node_modules in the review worktree); the numeric-flush finding was verified by reading the code paths at HEAD…；chunk 31：execute slash-dispatch.test.ts under vitest to confirm green (blocked: worktree lacks node_modules and built dist; install+build exceeds tool budget)，另有 1 条。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：349 tests pass — this review observed 25927 passed; 99 tests pass — this review observed 25927 passed。
收敛情况：第 4 轮发布了 71 条行内评论，其中 71 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/cli/src/ui/opentui/item-projection.ts（第 3 轮已出过发现，本轮又有 13 条）；packages/cli/src/ui/opentui/slash-dispatch.ts（第 3 轮已出过发现，本轮又有 10 条）；packages/cli/src/ui/opentui/a11y-plain-text.ts（第 3 轮已出过发现，本轮又有 4 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| loadHistory: () => {}, | ||
| refreshStatic: () => {}, |
There was a problem hiding this comment.
[Suggestion] Commands that rebuild history imperatively via ui.loadHistory are silently swallowed by the no-op stub with no deferral notice — unlike the equivalent load_history result type, which gets an explicit 'not yet available' message.
User with collapsed resume history runs /history expand-now: expandNowCommand detects suppressed items, computes the expanded list, calls context.ui.loadHistory(updated) — the stub drops it — and returns void, so executeSlashCommand yields {kind:'handled'}. In ink the transcript expands; through this dispatcher nothing happens and nothing tells the user the feature is unported, breaking the module's own deferral-message convention (the same stub is why /restore's transcript restore is lost).
Suggested fix: Route loadHistory/refreshStatic through SlashDispatchEnv callbacks the backend can wire, or have the stub record the call and surface the same explicit parity-deferral message the load_history branch produces.
Fix witness: A command whose action calls ctx.ui.loadHistory([...]) must produce the deferral message (or the env callback must be invoked); deleting the notice/guard turns it red.
中文说明
经 ui.loadHistory 命令式重建历史的命令被空操作桩静默吞掉且无延迟提示——等价的 load_history 结果类型却有显式 'not yet available' 消息。带折叠恢复历史的用户运行 /history expand-now：expandNowCommand 检测被抑制条目、计算展开列表、调用 context.ui.loadHistory(updated)——桩丢弃它——并返回 void，于是 executeSlashCommand 得到 {kind:'handled'}。ink 中转录展开；经本分发器什么都不发生也没人告知用户该功能未移植，违反模块自身的延迟消息约定（同一个桩也是 /restore 转录恢复丢失的原因）。修复：把 loadHistory/refreshStatic 路由到后端可接线的 SlashDispatchEnv 回调；或让桩记录调用并上呈与 load_history 分支相同的显式延迟消息。修复见证：动作调用 ctx.ui.loadHistory([...]) 的命令必须产生延迟消息（或 env 回调被调用）；删除提示/守卫会变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const notice = projectAddedItems(addedItems, env); | ||
| if (effect.kind === 'handled') { | ||
| return { kind: 'message', messageType: 'info', content: notice }; |
There was a problem hiding this comment.
[Suggestion] History items added via ui.addItem with type error/warning are always projected onto a messageType:'info' effect, dropping the item's severity that ink renders.
The suite's own R2-5 case is the trigger: a command adds { type:'error', text:'Unknown extensions source: bogus.' } and returns no result (/extensions explore bogus). Ink renders the item with error styling; the OpenTUI effect is { kind:'message', messageType:'info' } — a user-visible error surfaces as a neutral info message. The effect type already carries 'warning'|'error' and the projection knows the item type; only the mapping discards it.
Observed:
probe: replaying the real extensionsCommand trigger through dispatch → {kind:'message', messageType:'info', content:'Unknown extensions source: bogus.'}.
Suggested fix: Derive the effect's messageType from the projected item type (single message-type item → its type; otherwise the first message-type item's type) instead of hardcoding 'info'.
Fix witness: Extend the R2-5 test to assert effect.messageType === 'error' for the error item (and 'warning' for a warning item); the hardcoded 'info' fails it.
中文说明
经 ui.addItem 添加的 error/warning 类型历史条目总是被投影为 messageType:'info' 的 effect，丢掉了 ink 会渲染的条目严重级。套件自身的 R2-5 用例就是触发器：命令添加 { type:'error', text:'Unknown extensions source: bogus.' } 且不返回结果（/extensions explore bogus）。ink 以错误样式渲染该条目；OpenTUI 的 effect 是 { kind:'message', messageType:'info' }——用户可见的错误以中性 info 呈现。effect 类型本已携带 'warning'|'error'，投影也知道条目类型；只有映射丢弃了它。探针：经分发重放真实 extensionsCommand 触发 → messageType 为 'info'。修复：从投影条目类型推导 effect 的 messageType（单个消息型条目 → 其类型；否则首个消息型条目的类型），而不是硬编码 'info'。修复见证：扩展 R2-5 测试，对 error 条目断言 effect.messageType === 'error'（warning 同理）；硬编码 'info' 会失败。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| * The currently active theme mapped for OpenTUI — includes the ink parity | ||
| * rule that `NO_COLOR` forces the NoColor theme. | ||
| */ | ||
| export function getActiveOpenTuiTheme(): OpenTuiThemeDefinition { |
There was a problem hiding this comment.
[Suggestion] The NO_COLOR parity the docstring promises is mapped but then discarded by the apply path: NoColorTheme maps to an all-empty palette, and applyOpenTuiTheme drops every empty value and falls back to the full-color built-in dark surface — under NO_COLOR the OpenTUI renderer shows full color where ink renders colorless.
With NO_COLOR=1, themeManager.getActiveTheme() returns NoColorTheme and getActiveOpenTuiTheme() yields its all-empty palette (pinned by theme-parity.test.ts). The documented apply path then executes its value === '' ? undefined : toHex(value) filter, drops every palette key, and Object.assign(C, DARK, {}) leaves the full-color dark palette active — theme.test.ts explicitly asserts C.text stays '#CDD6F4'. Trigger: any user who sets NO_COLOR once the renderer is wired; the override the module header claims to carry has no visual effect.
Suggested fix: Either honor NoColor on the apply path where opentui tolerates it, or correct the theme-parity docstring to state that NO_COLOR cannot be honored on the OpenTUI renderer and let the renderer-selection layer act on that.
Fix witness: theme.test.ts's NoColor case currently pins the divergent fallback; the chosen fix must change/extend that test to pin the honored behavior (or the documented fallback).
中文说明
docstring 承诺的 NO_COLOR 对等被映射后又被应用路径丢弃：NoColorTheme 映射为全空调色板，而 applyOpenTuiTheme 丢弃每个空值并回退到全彩内置深色表面——NO_COLOR 下 OpenTUI 渲染器显示全彩，而 ink 渲染无色。NO_COLOR=1 时 themeManager.getActiveTheme() 返回 NoColorTheme，getActiveOpenTuiTheme() 产出全空调色板（theme-parity.test.ts 钉住）。随后文档化的应用路径执行 value === '' ? undefined : toHex(value) 过滤，丢弃每个调色板键，Object.assign(C, DARK, {}) 留下全彩深色调色板——theme.test.ts 显式断言 C.text 保持 '#CDD6F4'。触发：任何设置 NO_COLOR 的用户，一旦渲染器接线；模块头声称携带的覆盖没有视觉效果。修复：或在 opentui 容忍处兑现 NoColor，或修正 theme-parity 的 docstring 声明 OpenTUI 渲染器无法兑现 NO_COLOR、由渲染器选择层据此处理。修复见证：theme.test.ts 的 NoColor 用例当前钉住相异的回退；所选修复必须改动/扩展该测试以钉住被兑现的行为（或被文档化的回退）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| export function createSyntaxStyle( | ||
| definition: OpenTuiThemeDefinition, | ||
| ): SyntaxStyle { | ||
| return SyntaxStyle.fromStyles(definition.syntaxStyles); | ||
| } |
There was a problem hiding this comment.
[Suggestion] createSyntaxStyle hands raw ink color values to SyntaxStyle.fromStyles without the toHex normalization its sibling path applyOpenTuiTheme performs, so for ink-name colors the two construction paths produce different opentui colors for the same theme definition; unrecognized names degrade to magenta.
Verified against @opentui/core 0.5.8: parseColor accepts only hex plus a 28-entry CSS table; anything else defaults to magenta with a console.warn. ANSI theme: syntaxStylesFromInkTheme emits {fg:'green'} for number/comment; via createSyntaxStyle opentui parses CSS green = #8000 while applyOpenTuiTheme resolves ink green to #00ff00 — the same theme renders different greens depending on which API wiring uses. Custom themes: 'bluebright' passes through unresolved (opentui knows 'brightblue', reversed) → keywords render magenta, while applyOpenTuiTheme resolves it to #8080ff. theme.ts's own comment documents this hazard.
Observed:
probe (real parseColor + repo toHex): green → #008000 vs #00ff00; bluebright → #ff00ff (warn 'Invalid hex color: bluebright, defaulting to magenta') vs #8080ff; coral → #ff00ff (warn) vs #ff7f50.
Suggested fix: Normalize in createSyntaxStyle the same way applyOpenTuiTheme does — resolve each style's fg/bg through toHex, delete unresolvable ones, then call fromStyles — ideally via a shared helper.
Fix witness: Assert createSyntaxStyle(getOpenTuiTheme('ANSI')!).styles['number'].fg === '#00ff00' (reds today — it is 'green'), and/or that every fg matches /^#[0-9a-f]{6}$/ for all built-ins.
中文说明
createSyntaxStyle 把原始 ink 颜色值直接交给 SyntaxStyle.fromStyles，没有做兄弟路径 applyOpenTuiTheme 所执行的 toHex 规范化，因此对 ink 名称颜色，两条构造路径对同一主题定义产出不同的 opentui 颜色；无法识别的名称退化为品红。对照 @opentui/core 0.5.8 验证：parseColor 只接受 hex 加 28 项 CSS 表；其余默认品红并 console.warn。ANSI 主题：syntaxStylesFromInkTheme 对 number/comment 发出 {fg:'green'}；经 createSyntaxStyle，opentui 把 CSS green 解析为 #8000，而 applyOpenTuiTheme 把 ink green 解析为 #00ff00——同一主题因接线使用哪个 API 而渲染不同绿色。自定义主题：'bluebright' 原样通过（opentui 只认 'brightblue'，方向相反）→ 关键字渲染品红，而 applyOpenTuiTheme 解析为 #8080ff。theme.ts 的注释本就记录了这个危害。探针（真实 parseColor + 仓库 toHex）：green → #8000 对 #00ff00；bluebright → #ff00ff（warn 'Invalid hex color: bluebright, defaulting to magenta'）对 #8080ff；coral → #ff00ff（warn）对 #ff7f50。修复：在 createSyntaxStyle 中做与 applyOpenTuiTheme 相同的规范化——每个样式的 fg/bg 经 toHex 解析、删除不可解析者，再调 fromStyles——理想做法是抽出共享助手。修复见证：断言 createSyntaxStyle(getOpenTuiTheme('ANSI')!).styles['number'].fg === '#00ff00'（当前为 'green'，红），并/或所有内置主题的每个 fg 匹配 /^#[0-9a-f]{6}$/。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| 'markup.raw': { fg: inlineCode }, | ||
| 'markup.italic': { italic: true }, |
There was a problem hiding this comment.
[Suggestion] markdownMarkupTokens defines markup.raw (the markdown_inline code_span capture) but omits markup.raw.block, which is the capture @opentui/core's markdown highlights.scm actually uses for fenced and indented code blocks — and the style fallback cannot bridge the gap.
Verified against lockfile-pinned @opentui/core 0.5.8: assets/markdown/highlights.scm captures (fenced_code_block) @markup.raw.block and (indented_code_block) @markup.raw.block; markup.raw only exists in markdown_inline. Style resolution for an unmatched group falls back to the FIRST dot-segment only, so markup.raw.block falls back to markup — which theme.ts also does not define — never to markup.raw. The bundled grammars are only javascript/typescript/zig/markdown, so any fenced block with another language (python, bash, json — the common case in model output) is styled by markup.raw.block alone: fenced-code bodies render in the default foreground instead of the code color once tree-sitter markdown is enabled, while inline code keeps it.
Suggested fix: Add the block capture alongside the inline one: 'markup.raw.block': { fg: inlineCode },
Fix witness: theme.test.ts's markdownMarkupTokens capture-name key list: adding 'markup.raw.block' to it reds without the token.
中文说明
markdownMarkupTokens 定义了 markup.raw（markdown_inline 的 code_span 捕获）但遗漏了 markup.raw.block——@opentui/core 的 markdown highlights.scm 对围栏与缩进代码块实际使用的捕获——且样式回退无法弥合。对照锁定的 @opentui/core 0.5.8 验证：assets/markdown/highlights.scm 捕获 (fenced_code_block) @markup.raw.block 与 (indented_code_block) @markup.raw.block；markup.raw 只在 markdown_inline 中。未匹配组的样式解析只回退到首个点分段，因此 markup.raw.block 回退到 markup——theme.ts 同样未定义——永远不会到 markup.raw。捆绑语法只有 javascript/typescript/zig/markdown，因此其他语言（python、bash、json——模型输出的常见情形）的围栏块只由 markup.raw.block 定样式：一旦启用 tree-sitter markdown，围栏代码主体以默认前景色渲染而非代码色，行内代码却保留。修复：在行内捕获旁加上块捕获：'markup.raw.block': { fg: inlineCode }。修复见证：theme.test.ts 的 markdownMarkupTokens 捕获名键列表：未加该键时，把 'markup.raw.block' 加入列表会变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
- a11y-plain-text: split on all CommonMark line endings so CRLF
markdown opens/closes fences correctly; private-param CSI regex
covers ECMA-48 intermediate bytes; DCS/SOS/PM/APC and unterminated
OSC sequences consumed before strip-ansi; code-span pattern mirrors
ink's INLINE_CODE_SPAN_PATTERN (non-empty content, closing-run
lookbehind)
- dialogs-shared: clearNumberBuffer called from setActiveIndex,
selectIndex, and resyncKey block so wheel/hover/click/resync can't
commit a stale numeric-flush selection the user never made
- event-adapter: tool_call_response carries visionBridgeNotice on the
tool-result event (ink ToolMessage renders the egress disclosure)
- item-projection: projectContextUsage reads memoryFiles as { path,
tokens } (ContextMemoryDetail), not { name, tokens }
- key-map: 10 kp* keypad-navigation aliases (kpleft→left, …) and
super flag folded into meta (ink Cmd+Enter = newline, not submit)
- link-click: cellColumns no longer truncated to trimmed text length
(preserves the wide-glyph right-half boundary); findUrlAtRow end
boundary is width-aware (stringWidth of the last glyph)
- slash-dispatch: submit effect carries PartListUnion content + a
textContent string for text-only consumers (image parts survive);
toggleVimEnabled and startNewSession seams wired from env; abort
race resolves immediately for an already-aborted signal
- clipboard: OSC 52 self-write removed — copyToClipboard's existing
fallback (writeOsc52 / wrapForMultiplexer) is the single source
- a11y-screen-reader: appendStatic skips clean === '\n' (ink's
hasStaticOutput guard)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-32 single-byte escape sequences (ESC + one final byte) leak into the composer — already reported (comment 3884061149)
- R4-33 decodeCapturedInput strips editing keys without applying their effect — already reported (comment 3884061154)
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 — stopped before round 2 by the review time budget.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/opentui/dialogs-core.ts:16 — [review] Module header points at dialogs-core.tsx, which does not exipackages/cli/src/ui/opentui/dialogs-shared.test.tsx:8 — [review] The suite never presses Enter, so useDialogSelect's two selepackages/cli/src/ui/opentui/dialogs-theme.test.ts:8 — [review] OpenTuiThemeDialog — the sole consumer wiring preview heightpackages/cli/src/ui/opentui/early-input.test.ts:73 — [review] drainCapturedInputAsText is tested only for the empty-bufferpackages/cli/src/ui/opentui/event-adapter.ts:274 — [review] The task_execution flattening drops taskDescription, thoughpackages/cli/src/ui/opentui/event-adapter.ts:286 — [review] The findings_list projection collapses ink's FindingsDisplaypackages/cli/src/ui/opentui/help-content.ts:252 — [review] formatHelpText never truncates shortcut descriptions, but inpackages/cli/src/ui/opentui/help-content.ts:284 — [review] formatHelpText pushes 'Browse built-in commands:' unconditiopackages/cli/src/ui/opentui/help-content.ts:290 — [review] When there are no custom commands, formatHelpText drops thepackages/cli/src/ui/opentui/item-projection.ts:164 — [review] List projections indent sub-rows with a 1-space base where ipackages/cli/src/ui/opentui/item-projection.ts:277 — [review] projectModelStats gates the Cached/Thoughts rows per entry,packages/cli/src/ui/opentui/item-projection.ts:353 — [review] projectSkillStats sorts by count only; ink breaks count tiespackages/cli/src/ui/opentui/item-projection.ts:447 — [review] projectContextUsage emits the 'Usage by category' header uncpackages/cli/src/ui/opentui/item-projection.ts:560 — [review] The skills-detail block emits the body-tokens suffix for evepackages/cli/src/ui/opentui/kitty-negotiation.ts:139 — [review] The write-throw guard (stdout.write throw -> finish(false))packages/cli/src/ui/opentui/link-click.ts:157 — [review] mailto: is in the reused osc8 scheme allowlist but unreachabpackages/cli/src/ui/opentui/mouse-hit.ts:45 — [review] mouse-hit.ts re-implements frameAnchor/terminalRowToLayoutRopackages/cli/src/ui/opentui/mouse-hit.ts:147 — [review] The container.width > 0 bypass in resolveListIndex is exercipackages/cli/src/ui/opentui/osc8-parity.ts:80 — [review] The legacy fallback's byte-identity invariant (label and URLpackages/cli/src/ui/opentui/osc8-parity.ts:110 — [review] renderBareUrl's defense-in-depth scheme-allowlist check is p- …and 6 more (see the run report)
Convergence: round 5 posted 27 inline comment(s), 13 of them reported for the first time. Findings keep coming back to the same files: packages/cli/src/ui/opentui/a11y-plain-text.ts (findings in round 4; 3 more now); packages/cli/src/ui/opentui/key-map.ts (findings in round 4; 2 more now); packages/cli/src/ui/opentui/link-click.ts (findings in round 4; 2 more now), and 5 more file(s). (Evidence: the previous round's work list was truncated to fit the marker, so the rounds named above may be an undercount, and a new finding written under an earlier round's id cannot be told from a re-post over a partial list, so the new-finding count may be understated.) A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 26 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 27 条行内评论，其中 13 条是首次提出。发现反复回到同一批文件：packages/cli/src/ui/opentui/a11y-plain-text.ts（第 4 轮已出过发现，本轮又有 3 条）；packages/cli/src/ui/opentui/key-map.ts（第 4 轮已出过发现，本轮又有 2 条）；packages/cli/src/ui/opentui/link-click.ts（第 4 轮已出过发现，本轮又有 2 条），另有 5 个文件。（证据说明：上一轮的工作清单为放进标记而被截断，上述轮次可能少计；在不完整的清单上，冠以早先轮次 id 的新发现无法与重发区分，首次提出的条数可能少计。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // CommonMark line endings: \r\n, \n, and lone \r all terminate a line; | ||
| // splitting on \n alone leaves \r on the line, which `.` excludes and | ||
| // `$` cannot see past, deadening fence detection for CRLF markdown. | ||
| for (const rawLine of markdown.split(/\r\n|\n|\r/)) { |
There was a problem hiding this comment.
[Critical] R5-1: [certifies-falsely] [new-surface] markdownToPlainText hand-rolls a regex mirror of ink's inline/block markdown semantics over unbounded model output instead of deriving text from the renderer's tokenizer or a real CommonMark parser; the corner space does not converge — rounds 1, 3, 4 and 5 have each found new corners. Round-5 demonstrated corners: (a) italic pass diverges from ink's INLINE_MARKDOWN_REGEX ('foo * bar' -> 'foo * bar' vs ink's 'foo bar*'); (b) NUL-digit-NUL placeholder restore collides with literal text ('real \u00000\u0000 end' gets the code-span content substituted); (c) the fence opener accepts a backtick fence whose info string contains a backtick, which CommonMark/ink reject, inverting parse state for the rest of the document; (d) spaced thematic breaks ('* * *') slip past the hr regex while ink's hrRegex matches them. Any model output containing one of these corners is announced to screen-reader users differently from what ink renders; each round patches corners without closing the class.
Witness:
probe: markdownToPlainText('*foo * bar*')='foo * bar' vs ink tokenizer squashed='foo bar*'; 'real `code` here \u00000\u0000 end' -> 'real code here code end'; '```a`b\n# Not a heading' -> '# Not a heading...' while ink CODE_FENCE_RE.exec('```a`b')=null; '* * *' -> '* * *' while ink hrRegex.test('* * *')=true
Suggested fix: Derive the reduction from the renderer's authoritative tokenization (getPlainTextLength in InlineMarkdownRenderer.tsx already walks INLINE_MARKDOWN_REGEX and strips markers — extract and share it), or from a real markdown parser's AST, instead of parallel regexes.
Fix witness: Pin the divergence cases above in a11y-plain-text.test.ts; they are red today and only go green once the reduction is derived from ink's tokenizer. Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
markdownToPlainText 用手工正则镜像 ink 的内联/块级 markdown 语义，而不是从渲染器的 tokenizer 或真正的 CommonMark 解析器推导纯文本；corner 空间无法收敛——第 1、3、4、5 轮各发现新 corner。本轮实证：(a) 斜体处理与 ink 的 INLINE_MARKDOWN_REGEX 分歧（'foo * bar' -> 'foo * bar'，ink 为 'foo bar*'）；(b) NUL-数字-NUL 占位符还原与字面文本冲突，用户文本被代码段内容替换；(c) 围栏开启正则接受 info 串含反引号的反引号围栏（CommonMark/ink 均拒绝），反转文档其余部分的解析状态；(d) 带空格的分割线 '* * *' 逃过 hr 正则。建议改为从渲染器的权威 token 流推导（InlineMarkdownRenderer.tsx 的 getPlainTextLength 已遍历 INLINE_MARKDOWN_REGEX 并去标记，可抽出共享），或从真正的 markdown 解析器 AST 推导。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const OTHER_ESCAPE_SEQUENCE = | ||
| /\x1b[PX^_][\s\S]*?(?:\x1b\\|\x07|$)|\x1b\][^\x07\x1b]*(?:\x07|\x1b\\|$)/g; |
There was a problem hiding this comment.
[Critical] R4-14: [certifies-falsely] [new-surface] (fix-induced) The round-4 fix of R4-14 added OTHER_ESCAPE_SEQUENCE, but an OSC sequence followed by another escape without BEL/ST is not consumed, so its body leaks into screen-reader text despite the regex's stated purpose. A truncated stream or tool output whose title/hyperlink OSC lost its BEL, followed by colored output, leaks the OSC body into the announced text via ScreenReaderOutputWriter.sanitize.
Witness:
probe: stripAnsi('\x1b]0;secret\x1b[31mred\x1b[0m') = 'ecretred' (expected 'red'); leak reaches the writer: appendStatic emits 'ecretred\n'
Suggested fix: Make the OSC alternative stop (without consuming) at any bare ESC that does not start ST: \x1b][^\x07\x1b]*(?:\x07|\x1b\|(?=\x1b)|$) — an ESC inside an OSC body is invalid, so truncating there leaves the following sequence intact for strip-ansi.
Fix witness: Add expect(stripAnsi('\x1b]0;secret\x1b[31mred\x1b[0m')).toBe('red') to a11y-plain-text.test.ts; removing the (?=\x1b) alternative makes it red again. Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
（fix-induced）第 4 轮对 R4-14 的修复新增了 OTHER_ESCAPE_SEQUENCE 预处理，但未带 BEL/ST 终止、后随另一个转义序列的 OSC 不会被消费，其正文仍会泄漏进读屏文本（实测泄漏到 ScreenReaderOutputWriter 的播报输出）。建议 OSC 分支在遇到不构成 ST 的裸 ESC 时停止且不消费：改为 \x1b][^\x07\x1b]*(?:\x07|\x1b\|(?=\x1b)|$)。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const lastChar = row.text[hit.end - 1] ?? ''; | ||
| const lastColumn = row.cellColumns[row.cellColumns.length - 1] ?? 0; | ||
| endCellExclusive = lastColumn + (stringWidth(lastChar) || 1); |
There was a problem hiding this comment.
[Critical] R4-8: [certifies-falsely] [new-surface] (fix-induced) The round-4 fix of R4-8 made the end-boundary fallback width-aware via stringWidth(lastChar), but lastChar is the last UTF-16 UNIT, not the last code point: for a URL ending in a non-BMP character (emoji), it is the lone low surrogate, stringWidth of it is 0, and the ||1 fallback yields width 1 instead of 2 — the right-half cell of the final emoji misses the hit. A rendered row 'https://x.co/🎉' with the URL at the row's content edge: clicking the right half of the final emoji does not open the link.
Witness:
probe: left-half of final emoji -> HIT 'https://x.co/🎉'; right-half -> null; BMP-wide control 'https://x.co/文' hits both halves; stringWidth(lone low surrogate)=0, stringWidth('🎉')=2
Suggested fix: Take the last code point instead of the last UTF-16 unit: const lastChar = Array.from(row.text).at(-1) ?? ''.
Fix witness: Add a link-click.test.ts row 'https://a.dev/🎉' asserting findUrlAtRow hits on both the last and last+1 cells and null at last+2; reverting to row.text[hit.end - 1] makes the right-cell assertion fail. Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
（fix-induced）第 4 轮对 R4-8 的修复让行末边界回退感知宽度（stringWidth(lastChar)），但 lastChar 取的是最后一个 UTF-16 单元而非最后一个码点：URL 以 emoji（非 BMP）结尾时，它是孤立低代理位，stringWidth 为 0，||1 回退得到宽度 1 而非 2——末尾 emoji 右半格点击无法命中链接（已执行验证：左半命中、右半 miss，BMP 宽字符对照两边均命中）。建议用 Array.from(row.text).at(-1) 取最后一个码点。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| return ( | ||
| <DialogFrame> | ||
| {mode === 'theme' ? ( |
There was a problem hiding this comment.
[Critical] R4-30: (still stands from round 4, covers R4-29, severity escalated) The tight-height (includePadding=false) branch of the height budget is only half-applied: DialogFrame hardcodes padding={1} with no vertical-padding prop while ink's ThemeDialog sets paddingTop/Bottom={includePadding ? 1 : 0}, and theme mode adds a marginBottom={1} header margin and a marginTop={1} preview-pane margin that ink renders neither. 24-row terminal with 13+ themes: budget 19 (getDialogMaxHeight(24,3)), internal 15 < 16 -> includePadding=false; ink renders 19 rows (fits exactly), the port renders 22 rows — exceeding the computed budget by 3, clipping the bottom border/footer, in the exact scenario the budget function exists for.
Witness:
probe: computeThemePreviewLayout(19, 13) = {includePadding: false, codeBlockHeight: 4, diffHeight: 2}; row count from quoted JSX: ink 19 vs port 22 (16 content + 2 unconditional frame padding + 2 borders + 2 added margins)
Suggested fix: Give DialogFrame a paddingY prop and pass layout.includePadding ? 1 : 0 from OpenTuiThemeDialog; drop the theme-mode marginBottom/marginTop rows to match ink's snapshot geometry.
Fix witness: A render test mounting OpenTuiThemeDialog with availableTerminalHeight=19 and 13 themes asserting total height <= 19 and zero frame vertical padding when includePadding=false; removing the propagation turns it red. Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
（第 4 轮 R4-30 仍成立，合并 R4-29，严重度升级）紧凑高度（includePadding=false）分支只做了一半：DialogFrame 硬编码 padding={1} 且没有垂直 padding 参数，而 ink 的 ThemeDialog 用 paddingTop/Bottom={includePadding ? 1 : 0}；theme 模式还额外加了 ink 并不渲染的 marginBottom={1}（标题下）与 marginTop={1}（预览区上）。24 行终端、13 个主题：预算 19（getDialogMaxHeight(24,3)），内部 15 < 16 -> includePadding=false；ink 渲染 19 行恰好放下，本实现渲染 22 行——在预算函数存在的场景里超出预算 3 行并裁掉底边框。建议给 DialogFrame 增加 paddingY 参数并由对话框传入 layout.includePadding ? 1 : 0，同时去掉两个多余 margin。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| for (let x = 0; x < grid.width; x++) { | ||
| const codePoint = chars[base + x]; | ||
| cells[x] = isContinuationCell(codePoint) ? '' : (lineChars[ci++] ?? ' '); |
There was a problem hiding this comment.
[Critical] R5-2: [certifies-falsely] [new-surface] The native-grid decode consumes exactly one code point per non-continuation cell; if a cell resolves to a multi-code-point grapheme cluster (combining mark, ZWJ/skin-tone emoji), every subsequent cell's character and cellColumns entry shifts, corrupting hit-test mapping and even truncating the row text. The shipped libopentui.so exposes CHAR_FLAG_GRAPHEME/drawGrapheme/GraphemeTracker, but link-click.ts handles only CHAR_FLAG_CONTINUATION. 'Café https://a.dev' with é as e+U+0301 occupying one terminal cell: cellColumns shift right by the surplus code points, trailing code points of the row are dropped (URL 'a.dev' truncated to 'a.de' observed), clicking the visible URL start misses, and a click one cell off opens the truncated URL.
Witness:
probe (native-path input modeling a grapheme cell): decoded cells leak the combining mark into the next cell; row.text='Café https://a.de' (final 'v' dropped); findUrlAtRow(x=5)=null while findUrlAtRow(x=6) hits — off by one
Suggested fix: Consume one grapheme cluster per non-continuation cell (Intl.Segmenter granularity:'grapheme'), or verify and document @opentui/core's per-cell invariant at the seam.
Fix witness: A native-grid test whose getRealCharBytes returns 'e\u0301x' for two non-continuation cells asserting cellColumns 0,0,1 — red today (returns 0,1,2). Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
原生网格解码对每个非续接单元恰好消费一个码点；若单元解析为多码点字位簇（组合标记、ZWJ/肤色 emoji），其后所有单元的字符与 cellColumns 条目整体右移，命中映射被破坏，行尾码点还会被截断（实测 'a.dev' 被截成 'a.de'，可见 URL 起点点击 miss、偏一格反而打开截断 URL）。libopentui.so 暴露 CHAR_FLAG_GRAPHEME/drawGrapheme/GraphemeTracker，但 link-click.ts 只处理 CHAR_FLAG_CONTINUATION。建议按字位簇消费（Intl.Segmenter, granularity:'grapheme'），或在接缝处验证并记录库的每单元不变量。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| default: | ||
| break; |
There was a problem hiding this comment.
[Suggestion] R4-40: (still stands from round 4) The mapper's top-level switch over ServerGeminiStreamEvent ends in a bare default:break with no exhaustiveness guard, so a new core event type silently produces nothing. Core adds an event type; the OpenTUI mapper drops it without a compile error or a log, and the divergence is discovered only by user reports.
Witness:
round-5 re-check at HEAD: unchanged from round 4
Suggested fix: Add a satisfy/exhaustiveness check or a debug log on unhandled event types.
Fix witness: Adding a new member to the event union must fail compilation or produce the logged fallback. Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
（第 4 轮仍成立）映射器顶层 switch 以裸 default:break 结尾、无穷尽性守卫；核心新增事件类型会被静默丢弃且无编译错误、无日志。建议加 satisfies/穷尽性检查，或对未处理事件类型输出调试日志。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| ): Promise<void> { | ||
| const map = createEventMapper(); |
There was a problem hiding this comment.
[Suggestion] R4-41: (still stands from round 4; re-verified this round) pumpServerStream — the documented drain for real agent streams and the only non-test consumer of createEventMapper — constructs the mapper with no EventMapperContext and accepts no parameter to forward one, making all four context knobs (showCitations, formatError, getModelName, getMaxSessionTurns) dead switches on the sole production path. Once wired: ui.showCitations:false still shows citations; errors bypass parseAndFormatApiError (no 429/auth hints, empty-message errors silently dropped); compaction and max-turns notices print 'the model' / 'the configured limit' instead of real values.
Witness:
round-5 probe at HEAD: pumpServerStream.length === 2; draining max_session_turns emits 'the configured limit'; a citation event passes ungated
Suggested fix: Add a third parameter and forward it: pumpServerStream(stream, sink, context?: EventMapperContext) -> createEventMapper(context).
Fix witness: A test pumping a citation event with { showCitations: () => false } asserting no info event reaches the sink goes red if the parameter is dropped. Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
（第 4 轮仍成立，本轮复核）pumpServerStream——文档声明用于真实代理流的唯一非测试消费者——构造 createEventMapper() 时不接受也不转发 EventMapperContext，四个上下文旋钮（showCitations、formatError、getModelName、getMaxSessionTurns）在唯一生产路径上全是死开关。探针验证：max_session_turns 通知打印 'the configured limit'，citation 不被门控。建议增加第三参数并转发：createEventMapper(context)。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| */ | ||
| onWindowExpired?: (key: ExitGuardKey) => void; | ||
| /** Injectable timer for tests. */ | ||
| setTimeoutFn?: (fn: () => void, ms: number) => unknown; |
There was a problem hiding this comment.
[Suggestion] R4-42: (still stands from round 4) The injectable timer seam (setTimeoutFn/clearTimeoutFn) is a dead switch: declared and read but never set by any caller in this commit. An unexercised injection seam can rot silently; the guard's timing behavior is only ever tested through real timers.
Witness:
round-5 re-check at HEAD: unchanged from round 4; tests use vi.useFakeTimers() instead of the seam
Suggested fix: Either exercise the seam in a test or drop it until a consumer needs it.
中文说明
（第 4 轮仍成立）可注入定时器接缝（setTimeoutFn/clearTimeoutFn）被声明、被读取，但无任何调用方设置——死开关（测试用的是 vi.useFakeTimers()）。建议用测试行使该接缝，或在有消费者之前删除。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| } | ||
|
|
||
| /** Footer hint text for an armed exit (ink Footer.tsx / ExitWarning parity). */ | ||
| export function exitGuardHint(key: ExitGuardKey): string { |
There was a problem hiding this comment.
[Suggestion] R4-43: (still stands from round 4) exitGuardHint hardcodes English hint strings, but the ink footer renders both hints through t() and all nine locale files translate both keys. zh locale, Ctrl+C once: ink shows 再次按 Ctrl+C 退出; the OpenTUI footer shows English — breaking the parity the module docstring and the 'matches the ink footer wording' test claim (the test pins the English strings, hiding the divergence).
Witness:
round-5 probe at HEAD: exitGuardHint returns the English literals under any locale; zh.js:1809 carries the translation
Suggested fix: Return t(key === 'ctrl-d' ? 'Press Ctrl+D again to exit.' : 'Press Ctrl+C again to exit.') — both keys already exist in every locale file.
Fix witness: A test switching the active language to zh and asserting the translated hint goes red if the t() wrapper is removed. Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
（第 4 轮仍成立）exitGuardHint 硬编码英文提示，而 ink 的 footer 两处提示都经 t() 渲染，且九个语言文件均已翻译（zh.js:1809 '再次按 Ctrl+C 退出'）。zh 语言下按一次 Ctrl+C：ink 显示中文，OpenTUI 显示英文——现有测试固定英文字符串，反而掩盖了分歧。建议包 t()（两个键在所有语言中已存在，无需新增翻译）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| out.push({ | ||
| type: 'error', | ||
| text: message, | ||
| hint: 'Press Ctrl+Y to retry', |
There was a problem hiding this comment.
[Suggestion] R4-44: (still stands from round 4; scope corrected this round) The hardcoded-English divergence holds for the error retry hint: ink wraps it in t() and all nine locales translate '按 Ctrl+Y 重试。', while the adapter hardcodes it. The round-4 sibling examples (compression/max-turns/cancel notices) are retracted — ink hardcodes those too, and the adapter is faithful parity there. Non-English locale: every error notice from the adapter ships the English retry hint while ink localizes it.
Witness:
round-5 probe at HEAD: error events carry the hardcoded hint; locale grep shows nine translations against ink's t(...) call sites; compression/max-turns texts verified byte-identical to ink's hardcoded originals
Suggested fix: Wrap the retry hint in t() (the key already exists in every locale).
Fix witness: A locale-switched test asserting the translated hint goes red if t() is removed. Please add this test and confirm the mutation — remove the fix/guard and verify the test goes red.
中文说明
（第 4 轮仍成立，本轮修正范围）错误重试提示硬编码英文：ink 用 t() 包裹 'Press Ctrl+Y to retry'，九个语言均已翻译（中文 '按 Ctrl+Y 重试。'）。第 4 轮列举的姊妹例子（压缩/最大轮次/取消通知）予以撤回——ink 同样硬编码这些字符串，适配器在那些位置是忠实对等。建议只给重试提示包 t()。
— qwen3.8-max via Qwen Code /review (v0.22.2)
- event-adapter: finished branch emits retry-countdown-clear BEFORE the info notice so the countdown row is actually cleared (the fold only pops when the last item is the retry row) - item-projection: /mcp tips now include all 5 lines ink renders (added OAuth auth tip and Ctrl+T toggle tip) - link-click: wide-glyph end boundary uses the last code point (not UTF-16 code unit) so non-BMP emoji are measured correctly by stringWidth - a11y-plain-text: CSI_SEQUENCE replaces PRIVATE_PARAM_CSI — drops the marker requirement so any CSI (with or without private parameter marker, with or without intermediate bytes) is fully consumed
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): "agent 1b": none — no line needed.; chunk 12: could not execute npx vitest run src/ui/opentui/event-adapter.test.ts — the review worktree has no installed node_modules and no built workspace dist; static …; chunk 19: none — wait, per instructions: no checks were cut short, so no Budget gap line..
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
Not reviewed: "agent reverse-audit (round 1)" — the agent made no tool call: it read nothing.
Not reviewed: "agent verify", "agent verify (round 1)" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/opentui/a11y-plain-text.test.ts:22 — [probe] The end-of-input ($) branch of OTHER_ESCAPE_SEQUENCE is…packages/cli/src/ui/opentui/dialogs-shared.test.tsx:135 — [probe] The resync branch's disarm of an armed numeric flush has…packages/cli/src/ui/opentui/dialogs-shared.tsx:108 — [probe] DialogTabBar hardcodes the active-tab foreground '#000000'…packages/cli/src/ui/opentui/dialogs-shared.tsx:370 — [probe] DialogSelect's row-render logic (indicator/number color…packages/cli/src/ui/opentui/early-input.test.ts:123 — [probe] The 'stops retrying after maxAttempts' test contains zero…packages/cli/src/ui/opentui/event-adapter.test.ts:57 — [probe] visionBridgeNotice propagation has zero coverage even on…packages/cli/src/ui/opentui/event-adapter.test.ts:77 — [probe] Every tool_call_response mapper test asserts display:''…packages/cli/src/ui/opentui/event-adapter.ts:481 — [probe] The failed/cancelled branch of the tool-end mapping has no…packages/cli/src/ui/opentui/exit-lifecycle.ts:46 — [probe] exitSession/isExitInProgress/EXIT_CODE_* have zero…packages/cli/src/ui/opentui/item-projection.test.ts:46 — [review] The vi.mock override hardcodes getMCPServerStatus to…packages/cli/src/ui/opentui/item-projection.ts:73 — [probe] projectAbout emits a 'Qwen Code' line when cliVersion is…packages/cli/src/ui/opentui/item-projection.ts:261 — [review] ~30 user-facing strings are hardcoded English where ink…packages/cli/src/ui/opentui/item-projection.ts:276 — [probe] The Cached/Thoughts rows in projectModelStats are gated…packages/cli/src/ui/opentui/item-projection.ts:353 — [probe] projectSkillStats sorts only by call count, omitting ink's…packages/cli/src/ui/opentui/item-projection.ts:408 — [probe] projectInsightProgress dropped ink's negative-side clamp…packages/cli/src/ui/opentui/item-projection.ts:441 — [probe] projectContextUsage never emits the over-limit warning ink…packages/cli/src/ui/opentui/item-projection.ts:447 — [probe] 'Usage by category' is pushed unconditionally; ink renders…packages/cli/src/ui/opentui/item-projection.ts:559 — [probe] The skill body-cost suffix prints for every loaded skill;…packages/cli/src/ui/opentui/item-projection.ts:562 — [probe] Skill names are truncated at 30 chars but padded to only…packages/cli/src/ui/opentui/item-projection.ts:731 — [probe] projectMcpStatus drops MCP prompt descriptions that ink…- …and 23 more (see the run report)
Convergence: round 6 posted 7 inline comment(s), 3 of them reported for the first time; the previous round posted 27 (13 new). Findings keep coming back to the same files: packages/cli/src/ui/opentui/event-adapter.ts (findings in round 4; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："agent 1b"：none — no line needed.；chunk 12：could not execute npx vitest run src/ui/opentui/event-adapter.test.ts — the review worktree has no installed node_modules and no built workspace dist; static …；chunk 19：none — wait, per instructions: no checks were cut short, so no Budget gap line.。
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
未审查："agent reverse-audit (round 1)"——该 agent 未发起任何工具调用：它什么都没读。
未审查："agent verify"、"agent verify (round 1)"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 43 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 7 条行内评论，其中 3 条是首次提出；上一轮发布了 27 条（其中 13 条首次提出）。发现反复回到同一批文件：packages/cli/src/ui/opentui/event-adapter.ts（第 4 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // passthroughs never reach the screen reader as announced garbage. | ||
| /* eslint-disable no-control-regex */ | ||
| const OTHER_ESCAPE_SEQUENCE = | ||
| /\x1b[PX^_][\s\S]*?(?:\x1b\\|\x07|$)|\x1b\][^\x07\x1b]*(?:\x07|\x1b\\|$)/g; |
There was a problem hiding this comment.
[Critical] R4-14: [certifies-falsely] [new-surface] An OSC sequence followed by another escape without BEL/ST still violates the strip contract — the visible text after it is swallowed from the screen-reader announcement entirely.
The round-4 fix added OTHER_ESCAPE_SEQUENCE and this round's broadened CSI_SEQUENCE changed the symptom without closing it: for '\x1b]0;secret\x1b[31mred\x1b[0m' (a title OSC that lost its BEL, followed by colored output), the CSI pass strips '\x1b[31m'/'\x1b[0m' first, leaving '\x1b]0;secretred'; the OSC alternative then takes its end-of-input branch and consumes everything, so stripAnsi returns '' and ScreenReaderOutputWriter.appendStatic writes nothing — the legitimate text 'red' never reaches the announcement. Any truncated stream or tool output whose OSC lost its BEL, followed by real content, goes silent.
Witness:
probe at HEAD: stripAnsi('\x1b]0;secret\x1b[31mred\x1b[0m') = "" (expected "red")
writer.appendStatic(same) -> writes: [] (expected ["red\n"])
mutant ($ branch removed): "ecretred" — round-5 leak shape, suite still 19/19 green
Make the OSC alternative stop (without consuming) at any bare ESC that does not start ST, so the following sequence survives for the CSI pass and the text after it is kept — add a (?=\x1b) alternative to the OSC half. The header comment's "consume through ST/BEL/end-of-input" rationale answers dropping the garbage body itself; it does not answer swallowing the legitimate trailing text.
| /\x1b[PX^_][\s\S]*?(?:\x1b\\|\x07|$)|\x1b\][^\x07\x1b]*(?:\x07|\x1b\\|$)/g; | |
| /\x1b[PX^_][\s\S]*?(?:\x1b\\|\x07|$)|\x1b\][^\x07\x1b]*(?:\x07|\x1b\\|(?=\x1b)|$)/g; |
Please add expect(stripAnsi('\x1b]0;secret\x1b[31mred\x1b[0m')).toBe('red') to a11y-plain-text.test.ts, and confirm the mutation — remove the lookahead alternative and verify the new test goes red.
中文说明
R4-14（仍成立）：未带 BEL/ST 终止、后随另一个转义序列的 OSC 仍然违反剥离契约——其后的可见文本会被整体吞掉，无法进入读屏播报。第 4 轮新增的 OTHER_ESCAPE_SEQUENCE 与本轮放宽的 CSI_SEQUENCE 改变了症状但未关闭问题：对 '\x1b]0;secret\x1b[31mred\x1b[0m'（丢了 BEL 的标题 OSC + 彩色输出），CSI 通道先剥掉 '\x1b[31m'/'\x1b[0m'，OSC 分支随后走到"直到输入结束"分支，把剩余文本全部消费——stripAnsi 返回 ''，读屏写入器什么都不写，合法文本 'red' 丢失。建议把 OSC 分支改为在遇到不构成 ST 的裸 ESC 时停止（前瞻 (?=\x1b)），让后续序列留给 CSI 通道、其后的文本保留。请补充上述断言并做变异验证（移除前瞻分支后新测试应变红）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * and horizontal rules are dropped. Bullet markers stay — they are readable | ||
| * content in the ink parity path too. | ||
| */ | ||
| export function markdownToPlainText(markdown: string): string { |
There was a problem hiding this comment.
[Critical] R5-1: [certifies-falsely] [new-surface] markdownToPlainText hand-rolls a regex mirror of ink's inline/block markdown semantics over unbounded model output instead of deriving text from the renderer's tokenizer or a real CommonMark parser; the corner space does not converge — rounds 1, 3, 4, 5 and now 6 have each found new corners.
Round 6 re-executed the round-5 corners at this commit (all still reproduce: italic divergence on 'foo * bar', the NUL-placeholder collision, a backtick fence whose info string contains a backtick inverting parse state, spaced thematic breaks '* * *' slipping past the hr regex) and found two more: the ... underline construct is missing entirely — markdownToPlainText('hi') announces the literal tags where ink renders 'hi' — and the unescapeMarkdownDollars prose pass is missing, so 'costs $5' reaches the reader with the backslash where ink says '$'. Any model output containing one of these corners is announced to screen-reader users differently from what ink renders.
Witness:
probe at HEAD vs ink authority:
'*foo * bar*' -> 'foo * bar' (ink tokenizes 'foo bar*')
'> ```\n> code\n> ```' -> '```\ncode\n```' (fence detection runs before de-quote)
'``` `weird\nstill?\n```' -> 'still?' (ink CODE_FENCE_RE rejects the opener; two lines dropped)
'* * *' unmatched, ink hrRegex.test('* * *') = true
inline-pass diff vs InlineMarkdownRenderer.tsx: no <u> branch, no unescapeMarkdownDollars
Derive the reduction from the renderer's authoritative tokenization (getPlainTextLength in InlineMarkdownRenderer.tsx already walks INLINE_MARKDOWN_REGEX and strips markers — extract and share it) or from a real CommonMark parser AST, instead of parallel regexes; if the hand-rolled reducer is deliberately kept for this foundation PR, at minimum add the pass and the unescapeMarkdownDollars parity pass before the placeholder restore. Note the module's own pinned invariant: ink guards underscore emphasis at word boundaries so identifiers like init survive (test 'leaves dunders and snake_case identifiers untouched') — any replacement must preserve that.
Please pin the divergence cases above in a11y-plain-text.test.ts, and confirm the mutation — they are red today and only go green once the reduction is derived from ink's tokenizer or the missing passes are added.
中文说明
R5-1（仍成立）：markdownToPlainText 用手工正则镜像 ink 的内联/块级 markdown 语义，作用于无界的模型输出，而不是从渲染器的 tokenizer 或真正的 CommonMark 解析器推导纯文本；corner 空间无法收敛——第 1、3、4、5、6 轮各发现新 corner。本轮在该提交上复测第 5 轮 corner 全部仍可复现（'foo * bar' 斜体分歧、NUL 占位符冲突、info 串含反引号的围栏反转解析状态、带空格的 '* * *' 逃过 hr 正则），并新发现两处：... 下划线结构完全缺失（'hi' 会把字面标签读出来，ink 渲染为 'hi'）；缺少 unescapeMarkdownDollars 通道（'costs $5' 会带着反斜杠播报）。建议改为从渲染器权威 token 流（InlineMarkdownRenderer.tsx 的 getPlainTextLength）或真正的 markdown 解析器 AST 推导；若本基础 PR 刻意保留手工归约器，至少补上 通道与 $ 反转义通道。注意模块自身已固定的约束：ink 在词边界保护下划线强调，init 这类标识符必须原样保留。请补充上述分歧用例并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| borderColor={C.dim} | ||
| paddingX={1} | ||
| paddingY={layout.includePadding ? 1 : 0} | ||
| marginTop={1} |
There was a problem hiding this comment.
[Critical] R4-30: [certifies-falsely] [new-surface] The tight-height (includePadding=false) branch of the theme dialog's height budget is still only half-applied — the port renders 22 rows into a 19-row budget and clips the bottom border/footer, in the exact scenario the budget function exists for.
DialogFrame hardcodes padding={1} with no vertical-padding prop, while ink's ThemeDialog sets paddingTop/Bottom={includePadding ? 1 : 0}; theme mode additionally renders a marginBottom={1} header margin (line 227) and a marginTop={1} preview-pane margin (line 268) that ink renders neither of. With a 24-row terminal and 16+ themes the budget is 19 (getDialogMaxHeight(24,3)) and computeThemePreviewLayout(19, 17) picks includePadding=false — ink renders exactly 19 rows, the port renders 22 (16 content + 2 unconditional frame padding + 2 borders + 2 added margins), 3 rows over.
Witness:
probe at HEAD: computeThemePreviewLayout(19, 17) = {includePadding:false, codeBlockHeight:6, diffHeight:3}
row sums from quoted JSX constants: ink = 1+0+15+2+0+0+1 = 19 (fits) | port = 1+1+16+2+1+1 = 22
DialogFrame (dialogs-shared.tsx:70) hardcodes padding={1}; margins quoted at dialogs-theme.tsx:227 and :268
Give DialogFrame a paddingY prop and pass layout.includePadding ? 1 : 0 from OpenTuiThemeDialog, and drop the two theme-mode margin rows to match ink's snapshot geometry:
// dialogs-shared.tsx — DialogFrame gains a vertical-padding prop
padding={1} paddingY={props.paddingY ?? 1}
// dialogs-theme.tsx — remove the margins ink does not render
<box flexDirection="row"> {/* was marginBottom={1} */}
<box> {/* was marginTop={1} */}The fix must keep PREVIEW_PANE_FIXED_VERTICAL_SPACE = 8 consistent with the rendered markup (its comment enumerates what the 8 rows count). Please add a render test mounting OpenTuiThemeDialog with availableTerminalHeight=19 and 13+ themes asserting total height <= 19 and zero frame vertical padding when includePadding=false — removing the propagation must turn it red.
中文说明
R4-30（仍成立，覆盖 R4-29）：主题对话框紧凑高度（includePadding=false）分支仍然只做了一半——本实现在 19 行预算里渲染 22 行，裁掉底边框/页脚，正是预算函数要解决的场景。DialogFrame 硬编码 padding={1} 且没有垂直 padding 参数（ink 的 ThemeDialog 用 paddingTop/Bottom={includePadding ? 1 : 0}）；theme 模式还多渲染了 ink 没有的 marginBottom={1}（第 227 行）与 marginTop={1}（第 268 行）。24 行终端、16+ 主题时预算为 19，ink 恰好渲染 19 行，本实现渲染 22 行，超出 3 行。建议给 DialogFrame 增加 paddingY 参数并由对话框传入 layout.includePadding ? 1 : 0，同时去掉两个多余 margin；保持 PREVIEW_PANE_FIXED_VERTICAL_SPACE = 8 与渲染结构一致。请补充渲染测试（19 行预算、13+ 主题，断言总高 <= 19 且紧凑分支下框架垂直 padding 为 0），并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // Full ECMA-48 CSI production (parameter bytes incl. ':' for kitty | ||
| // CSI-u, intermediate bytes, any final @-~): arrows, editing keys | ||
| // like Delete/Home/PgDn ('~' final), function keys, modifier forms. | ||
| .replace(/\u001B\[[0-9:;<=>?]*[ -/]*[@-~]/g, '') |
There was a problem hiding this comment.
[Critical] R6-54: [certifies-falsely] [new-surface] Double-bracket F-key sequences (ESC [ [ A..E, the TERM=linux F1-F5 encoding) survive the capture filter, and the generic CSI strip eats only ESC [ [ — the second '[' is a valid final byte in [@-~] — leaking the trailing letter into the composer.
A user on a Linux virtual console (TERM=linux; terminfo kf1=\E[[A .. kf5=\E[[E) who presses F1-F5 while the CLI boots gets stray 'A'-'E' text injected into their first prompt: classifyEscapeSequence keeps the sequence (the third byte '[' is none of ?/>/<, so it is user input), and decodeCapturedInput's CSI regex then matches '\x1b[[' alone and leaves the payload letter. That is exactly the corruption this module exists to prevent, on the drain path that is its sole documented purpose.
Witness:
probe at HEAD (full capture pipeline):
decode('\x1b[[A') => 'A'
decode('a\x1b[[Ab\x1b[[Ec') => 'aAbEc'
F1..F5 interleaved => 'aAbBcCdDeEf'
flip: adding .replace(/\u001B\[\[[A-E]/g, '') before the CSI pass -> '', 'abc', 'abcdef'
Strip the double-bracket form whole, before the generic CSI rule (order matters — the generic regex matches '\x1b[[' by itself, so it must run second):
// Linux-console double-bracket F1-F5 (ESC [ [ A..E): not valid ECMA-48;
// strip whole before the generic CSI rule, which would match only ESC [ [
// and leak the trailing letter.
.replace(/\u001B\[\[[A-E]/g, '')| .replace(/\u001B\[[0-9:;<=>?]*[ -/]*[@-~]/g, '') | |
| // Linux-console double-bracket F1-F5 (ESC [ [ A..E): not valid ECMA-48; | |
| // strip whole before the generic CSI rule, which matches only ESC [ [ | |
| .replace(/\u001B\[\[[A-E]/g, '') | |
| .replace(/\u001B\[[0-9:;<=>?]*[ -/]*[@-~]/g, '') |
Please add expect(decodeCapturedInput(Buffer.from('a\u001B[[Ab\u001B[[Ec'))).toBe('abc') to early-input.test.ts — removing the new replace yields 'aAbEc', so the test goes red.
中文说明
R6-54：双括号功能键序列（ESC [ [ A..E，TERM=linux 的 F1-F5 编码）能通过捕获过滤，而通用 CSI 剥离只吃掉 ESC [ [——第二个 '[' 是 [@-~] 中的合法终止字节——导致尾部字母泄漏进输入框。Linux 虚拟控制台用户在启动期间按 F1-F5，会把 'A'-'E' 注入第一条提示词：classifyEscapeSequence 保留该序列（第三字节 '[' 不是 ?/>/<，判为用户输入），decodeCapturedInput 的 CSI 正则只匹配 '\x1b[['。这正是本模块要防止的损坏，且发生在它唯一职责所在的排空路径上。建议在通用 CSI 规则之前整体剥离双括号形式（顺序重要）。请补充上述测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const display = renderResultDisplay(v.resultDisplay); | ||
| if (display) |
There was a problem hiding this comment.
[Critical] R6-1: [certifies-falsely] [new-surface] The vision-bridge egress disclosure (visionBridgeNotice) is silently dropped whenever the tool response has no displayable result content — exactly the case core produces for cancelled-after-completion responses.
Core's createCancelledResponse hardcodes resultDisplay: undefined while deliberately preserving visionBridgeNotice ("bridge notices survive cancellation", coreToolScheduler.ts:959-984), and cancelAfterPostProcessing forwards it. In the mapper, renderResultDisplay(undefined) returns '', so the if (display) gate emits no tool-result — the notice rides only inside that event — and only tool-end {summary:'cancelled'} reaches the UI: the user is never told their image left the machine via the vision model. Ink renders the notice regardless of display content (ToolMessage.tsx:954-960), and the diff's own ink-parity comment states the notice should ride the card.
Witness:
probe at HEAD: mapper on {type:'tool_call_response', value:{callId:'c1',
executionStatus:'cancelled', visionBridgeNotice:'Converted 1 image via qwen3-vl-plus.'}}
-> [{"type":"tool-end","id":"c1","success":false,"summary":"cancelled"}]
r6_1_any_event_carries_notice: false
flip: `if (display || visionBridgeNotice)` -> tool-result carries the notice
| const display = renderResultDisplay(v.resultDisplay); | |
| if (display) | |
| const display = renderResultDisplay(v.resultDisplay); | |
| if (display || visionBridgeNotice) |
Emit the result event when there is either display content or a notice — the display:'' shape is already used by the diff/todos/ansi branches. Please add a mapper test for a cancelled response carrying visionBridgeNotice and assert the emitted event carries it — removing the || visionBridgeNotice guard must make it red.
中文说明
R6-1：只要工具响应没有可显示的结果内容，视觉桥接出口披露（visionBridgeNotice）就会被静默丢弃——而这正是 core 在"完成后取消"场景产生的情况。core 的 createCancelledResponse 硬编码 resultDisplay: undefined 但刻意保留 visionBridgeNotice（coreToolScheduler.ts:959-984 注释：桥接通知在取消后保留）；映射器中 renderResultDisplay(undefined) 返回 ''，if (display) 门控不再发出 tool-result，而通知只挂在该事件上——UI 只收到 tool-end {summary:'cancelled'}，用户完全不知道自己的图片已经发送给视觉模型。ink 无论有无显示内容都会渲染该通知（ToolMessage.tsx），且本 diff 自己的 ink 对等注释也写明通知应随卡片显示。建议改为 if (display || visionBridgeNotice)。请补充取消响应携带 visionBridgeNotice 的映射测试，并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| }`, | ||
| ); | ||
| } else { | ||
| lines.push(` - ${skill.name}`); |
There was a problem hiding this comment.
[Critical] R6-2: [certifies-falsely] [new-surface] projectSkillsList drops the skill level suffix when a skill has no description, breaking parity with ink's SkillsList — the transcript silently loses the skill's scope (Bundled/Project/User/Extension) exactly for description-less skills.
Ink renders the level independently of the description: SkillsList.tsx applies {skill.level && ...levelLabel...} outside the description conditional, and skillsCommand.ts:101-102 appends the level regardless of description. Here the else branch pushes only - ${skill.name}, so projectSkillsList([{name:'x', level:'bundled'}]) projects ' - x' where ink renders '- x (Bundled)'. description is optional in SkillDefinition, and no projectSkillsList test exists to catch this.
Witness:
probe at HEAD:
projectSkillsList([{name:'x', level:'bundled'}]) -> "Available skills:\n\n - x"
control (with description) -> "... - x d (Bundled)"
scratch-tree fix -> "Available skills:\n\n - x (Bundled)"
Append the same suffix in the else branch. Ink renders the bare name unmodified in the no-description case (no truncate/padEnd — SkillsList.tsx:40-44), so the fix must not pad or truncate there.
| lines.push(` - ${skill.name}`); | |
| lines.push( | |
| ` - ${skill.name}${skill.level ? ` (${levelLabel(skill.level)})` : ''}`, | |
| ); |
Please add a projectSkillsList case asserting the output for [{name:'x', level:'bundled'}] contains '(Bundled)' — removing the suffix from the else branch must make it red.
中文说明
R6-2：projectSkillsList 在技能没有描述时丢弃技能级别后缀，与 ink 的 SkillsList 不一致——恰好对无描述技能，转录文本静默丢失技能作用域（Bundled/Project/User/Extension）。ink 的级别渲染独立于描述条件（SkillsList.tsx 在描述条件之外渲染 {skill.level && ...}，skillsCommand.ts:101-102 同样无条件追加级别）；此处 else 分支只 push - ${skill.name}，于是 projectSkillsList([{name:'x', level:'bundled'}]) 输出 ' - x'，而 ink 渲染 '- x (Bundled)'。SkillDefinition 中 description 为可选，且不存在 projectSkillsList 测试。建议在 else 分支追加相同后缀；注意 ink 在无描述时原样输出名称（不截断/补齐）。请补充测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| let ci = 0; | ||
| for (let x = 0; x < grid.width; x++) { | ||
| const codePoint = chars[base + x]; | ||
| cells[x] = isContinuationCell(codePoint) ? '' : (lineChars[ci++] ?? ' '); |
There was a problem hiding this comment.
[Critical] R5-2: [certifies-falsely] [new-surface] The native-grid decode consumes exactly one code point per non-continuation cell; a cell that resolves to a multi-code-point grapheme cluster (combining mark, ZWJ/skin-tone emoji) shifts every subsequent cell's character and cellColumns entry — corrupting hit-test mapping and truncating row text.
Verified against the real library this round: @opentui/core 0.5.8 stores 'Café' (é as e+U+0301) with the cluster in ONE cell (char flag 800200fe) while getRealCharBytes resolves it as TWO code points (bytes 65 cc 81). Feeding that real grid through this decoder shifts cellColumns right by the surplus code points and drops trailing code points — the URL truncates and every click maps one cell off.
Witness:
probe against real OptimizedBuffer via bun:ffi (all three widthMethods):
drawText('Cafe\u0301 https://a.dev') -> é in one cell, getRealCharBytes = 2 code points
PR-DECODE row0: findUrlAtRow(5) = null findUrlAtRow(6) = "https://a.dev"
(column 5 is the true display column of 'h')
full-width variant: row.text = "Café https://a.de" (final 'v' lost)
Consume one grapheme cluster per non-continuation cell instead of one code point — e.g. segment each realLine with new Intl.Segmenter(undefined, {granularity: 'grapheme'}) and advance the cell cursor by cluster, or verify and document @opentui/core's per-cell invariant at this seam if the library guarantees one cluster per cell in another shape. The shipped libopentui.so exposes CHAR_FLAG_GRAPHEME/drawGrapheme/GraphemeTracker but link-click.ts handles only CHAR_FLAG_CONTINUATION.
Please add a native-grid test whose getRealCharBytes returns 'e\u0301x' for two non-continuation cells asserting cellColumns 0,0,1 — it is red today (returns 0,1,2) — and confirm the mutation.
中文说明
R5-2（仍成立）：原生网格解码对每个非续接单元恰好消费一个码点；若单元解析为多码点字位簇（组合标记、ZWJ/肤色 emoji），其后所有单元的字符与 cellColumns 条目整体右移——命中映射被破坏、行尾码点被截断。本轮用真实库验证：@opentui/core 0.5.8 把 'Café'（é = e+U+0301）存为一个单元（char flag 800200fe），但 getRealCharBytes 解析出两个码点。把真实网格喂给该解码器，cellColumns 右移、行尾码点丢失（'a.dev' 截成 'a.de'），点击整体偏一格。建议按字位簇消费（Intl.Segmenter granularity:'grapheme'），或在该接缝验证并记录 @opentui/core 的每单元不变量（libopentui.so 暴露 CHAR_FLAG_GRAPHEME/drawGrapheme/GraphemeTracker，但 link-click.ts 只处理 CHAR_FLAG_CONTINUATION）。请补充原生网格测试（getRealCharBytes 返回 'e\u0301x' 两个非续接单元，断言 cellColumns 0,0,1——当前为红），并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
- abort race: already-aborted signal now skips command.action entirely (result = undefined) instead of eagerly evaluating it as a Promise.race argument — the action's side effects (clear, persist, addItem) must not run on a cancelled submission - parent command telemetry: logEvent (slash_command SUCCESS) is now called before the early return for parent commands with subCommands (help listing) and bare handled — matching ink's finally-block logging
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
4 participants
What this PR does
Foundation batch of the OpenTUI migration tracked in #8662. Adds the renderer foundation modules under a new opentui UI directory: the theme family, accessibility layers (plain-text and screen-reader), clipboard, key mapping, mouse hit-testing and caret placement, link clicking with OSC 8 parity, early input handling, exit guard and lifecycle, kitty graphics negotiation, the stream event adapter, history item projection, slash dispatch with command parsing, command context/output plumbing, help content, input history, and the dialog scaffolding primitives plus the theme dialog. All files are new; the single touch on existing code is a one-line export addition in the shared OSC 8 module.
Stacked PR: base is the infra batch branch (see #10134), because this batch consumes the framework-neutral streaming model and the locked OpenTUI dependencies. Once #10134 merges, this PR gets retargeted to main and only its own commits remain.
Why it's needed
The migration lands in dependency-ordered batches. These modules are the shared foundation every later batch (live session, dialogs, backend composition) builds on, and landing them first keeps each follow-up batch small and reviewable. Two command/dialog helpers land inside the opentui directory instead of the leaf utils layer because main's new utils dependency-direction rule (#9737) forbids utils importing upward — the helpers are OpenTUI-specific anyway.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/ui/opentui/— 349 tests pass across 26 files.cd packages/cli && npx vitest run src/ui/utils/osc8.test.ts— 99 tests pass (the one-line export change breaks nothing).npm run buildclean; ESLint clean including thearchitecture/no-utils-upward-importrule;node scripts/check-tui-dep-direction.mjspasses.Evidence (Before & After)
N/A (no user-visible change; modules gain consumers in later batches)
Tested on
Environment (optional)
Node 22.23.1; stacked on feat/tui-infra-batch (#10134).
Risk & Scope
Linked Issues
Part of #8662 (Foundation modules batch). Stacked on #10134.
中文说明
本 PR 做了什么
#8662 跟踪的 OpenTUI 迁移的 Foundation 批次。在新的 opentui UI 目录下加入渲染器基础模块：theme 家族、无障碍层（纯文本与读屏）、剪贴板、键位映射、鼠标命中与光标定位、OSC 8 对等的链接点击、早期输入处理、退出守卫与生命周期、kitty 图形协商、流事件适配器、历史条目投影、斜杠分发与命令解析、命令上下文/输出管道、帮助内容、输入历史，以及对话框脚手架基元和主题对话框。全部是新文件；对既有代码的唯一改动是共享 OSC 8 模块加一行 export。
叠层 PR：base 是 Infra 批分支（见 #10134），因为本批消费框架无关的 streaming model 与锁版 OpenTUI 依赖。#10134 合入后本 PR 改回 main，只剩自身提交。
为什么需要
迁移按依赖序分批落地。这些模块是后续所有批次（live session、dialogs、backend 组合根）共享的地基，先落地让后续每批保持小而可审。两个命令/对话框 helper 落在 opentui 目录而非 utils 叶子层，因为 main 新的 utils 依赖方向规则（#9737）禁止 utils 向上 import——这两个 helper 本来就是 OpenTUI 专用的。
评审测试计划
如何验证
cd packages/cli && npx vitest run src/ui/opentui/——26 个文件 349 个测试通过。cd packages/cli && npx vitest run src/ui/utils/osc8.test.ts——99 个测试通过（一行 export 改动无破坏）。npm run build干净；ESLint 干净（含architecture/no-utils-upward-import规则）；node scripts/check-tui-dep-direction.mjs通过。前后对比证据
N/A（无用户可见变化；模块在后续批次获得消费者）
测试环境
环境（可选）
Node 22.23.1；叠在 feat/tui-infra-batch（#10134）之上。
风险与范围
关联 Issue
属于 #8662（Foundation modules 批次）。叠在 #10134 之上。