feat(cli): OpenTUI migration infra batch — locked deps, dependency-direction arch check, framework-neutral streaming model - #10134
Conversation
Infra batch of the OpenTUI migration tracked in QwenLM#8662. Versions are pinned (not ranged) so renderer behavior stays reproducible across the landing batches.
…streaming model Infra batch of the OpenTUI migration tracked in QwenLM#8662. The arch check keeps packages/core and the new ui/model layer free of renderer-framework imports; the streaming model folds agent-loop stream events into render-ready history items so both renderers share one source of truth. Both modules gain consumers in later batches.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Stopping at the direction gate, before code review — this one needs a maintainer decision. @chiga0
The mechanical gates are clean: template complete ✓, no core-module paths touched (~385 production lines under packages/cli/src/ui/model/ plus a 260-line arch-check script, ~496 test lines, the rest lockfile/dependency entries), no high-risk paths matched. The concern is not the shape of the code — it is that this PR is the first landing step of a migration whose direction has no recorded maintainer decision.
The record on #8662:
- The issue triage of 2026-08-07 escalated the migration proposal to the maintainers: "maintainer decision required before any implementation; design docs need to be public first", labeling it
need-discussion/status/waiting-for-feedback. Both labels are still applied, and no maintainer has touched the thread since. - There is no maintainer comment on #8662, #8677, or #10128, and no human review on #8677 — the only reviews there are the bot's.
- @chiga0's own tracker comment (2026-08-24) still lists "Direction sign-off here … Maintainers' call" as remaining before merge.
So the PR body's "the plan approved in #8662" overstates the record: the plan is posted there; the approval is not observable anywhere in the thread.
Why this batch can't ride in as harmless additive infra: every piece of it only pays off if the migration proceeds — locked @opentui dependencies (with per-platform native packages) enter the lockfile, an architecture rule starts constraining imports in the business core, and a streaming model lands with zero consumers. The predecessor #10128 was different: those two fixes stand on their own as startup robustness for the ink path. Landing this batch is de facto ratification of step 1 of the plan before the decision is made — it pre-empts exactly the question that is waiting on the maintainers.
What would unblock this:
- A maintainer decision recorded on #8662 — go / no-go / go-with-conditions — or an explicit statement that additive batches may land ahead of that decision.
- The design doc committed under
docs/design/— the original escalation asked for it to be public, and the repo convention (AGENTS.md) expects one for work of this scale; today the plan lives only in the issue body.
No judgment is being passed on the code itself — the workflow stops at the direction gate, so there is no code review and no CI verdict in this pass. Once a decision is recorded, re-run with @qwen-code /triage and it will pick up from here.
Escalating to @wenshao @pomelo-nwu — a renderer migration (and the dependency bet that comes with it) is a product/platform call the triage gate cannot make for the project.
中文说明
在代码审查之前止步于方向门禁——这一项需要 maintainer 决策。@chiga0
机械门禁全部干净：模板完整 ✓，未触及核心模块路径（约 385 生产行位于 packages/cli/src/ui/model/，另有 260 行架构检查脚本、约 496 行测试，其余为 lockfile/依赖条目），未命中任何高风险路径。问题不在代码形态——而在于这个 PR 是一次迁移的第一个落地步骤，而该迁移的方向没有已记录的 maintainer 决策。
#8662 上的记录：
- 2026-08-07 的 issue 分诊已将迁移提案转交 maintainer："任何实施前需 maintainer 决策；设计文档需先公开"，并标记
need-discussion/status/waiting-for-feedback。两个标签至今仍在，且此后没有任何 maintainer 在该线程中出现。 - #8662、#8677、#10128 上均无 maintainer 评论；#8677 上唯一的 review 来自机器人。
- @chiga0 自己的追踪评论（2026-08-24）仍将"此处的方向签核 …… Maintainers' call"列为合入前的待办事项。
因此 PR 正文中"在 #8662 已批准的计划"夸大了记录：计划确实发布在那里，但批准在线程中任何地方都观察不到。
为什么这一批不能作为无害的纯增量基础设施直接放行：其中每一部分只有在迁移推进的前提下才有价值——锁版的 @opentui 依赖（含各平台原生包）进入 lockfile，一条架构规则开始约束业务核心的 import，一个没有消费者的 streaming model 落地。前一个批次 #10128 不同：那两个修复本身即成立，是 ink 路径的启动健壮性改进。落地这一批事实上是在决策作出之前批准了计划的第一步——恰恰抢先了正在等待 maintainer 回答的那个问题。
解除阻塞的条件：
- 在 #8662 上记录一个 maintainer 决策——推进 / 不推进 / 有条件推进——或者明确表示允许增量批次在决策之前先行落地。
- 将设计文档提交到
docs/design/——最初的升级处理就要求文档公开，仓库规范（AGENTS.md）也要求这一规模的工作有设计文档；目前计划只存在于 issue 正文中。
本次不对代码本身下判断——工作流在方向门禁处停止，因此没有代码审查，也没有 CI 结论。决策记录在案后，用 @qwen-code /triage 重跑即可从这里继续。
转交 @wenshao @pomelo-nwu —— 渲染器迁移（及其伴随的依赖押注）是产品/平台级决策，分诊门禁不能替项目作出。
— Qwen Code · qwen3.8-max
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 4: vitest execution of streaming-model.test.ts — worktree has no node_modules; all 14 tests hand-traced against streaming-model.ts at HEAD instead.
Test Plan (not a blocker): 24 tests pass — this review observed 24784 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 4：vitest execution of streaming-model.test.ts — worktree has no node_modules; all 14 tests hand-traced against streaming-model.ts at HEAD instead。
Test Plan（非阻断）：24 tests pass — this review observed 24784 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…o CI, streaming-model hardening, dev-only renderer deps (R1-1..R1-23)
R1 review batch — all 20 comments addressed (commit 3d37d90)Each thread carries the detailed per-comment reply; this is the roll-up. Critical (2) — fixed
Suggestions (18) — all addressed
Verification
|
|
Two status items on this head ( Direction gate (Stage 1c) — unblock condition 1 is now recorded on #8662: the maintainer direction call was posted there on 2026-08-26 (yiliang114): "Phase 1 (landing the code in batches) — go ahead.", with the ground rules that every batch still lands on its own review merits and that "#10128 and #10134 will get human review next (the current CHANGES_REQUESTED on #10134 is the automated bot pass)". The R1 code-review batch was addressed in the same commit (roll-up above, per-thread replies on all 20 comments); unblock condition 2 (design notes in the repo/thread) remains tracked in #8662. CI — |
|
CI failure on Root cause: Fix: report violations relative to the rule root instead — equally precise in production ( Everything else in that run was green (1774/1775 passing before this fix). |
|
CI status on this head (
Treating it as an infrastructure flake (same class as the earlier runner cancellation) and re-running the failed job once more. If it repeats on the next attempt, will investigate the worker-RPC timeout itself. 中文说明本 head（ |
|
@qwen-code-ci-bot /review |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
The model and aggregation paths look coherent on the current head, and the required CI checks are green. I found two remaining blockers in the dependency-direction gate; details are inline.
…symlink traversal in dep-direction gate
- findImports now handles ImportTypeNode (type X = import("...").Y) and accepts interpolation-free template literals as specifiers for import()/require()/vi.mock(), so every statically knowable module specifier is classified; interpolated templates stay skipped.
- listSourceFiles resolves each symlink target against the physical rule root and reports escaped links as gate failures, so traversal cannot leave the root and relative imports cannot be masked by lexical symlink paths.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
The import-specifier fix is complete, and escaped symlinks now fail closed on normal POSIX paths. One physical-path issue remains in the in-root symlink path; details are inline.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Infra Batch 1 — OpenTUI migration Phase 0
What I read: streaming-model.ts, streaming-model.test.ts, stream-aggregation.ts, check-tui-dep-direction.mjs, packages/cli/package.json diff, package-lock.json additions.
What I ran: CI log for the failing Test ubuntu-latest job.
CI red
Test ubuntu-latest exits 1. All 66 test files and 1779 tests passed. The failure comes from [vitest-worker]: Timeout calling "onTaskUpdate" printed after the suite finishes — a pre-existing vitest worker-process infrastructure flake, not a test failure introduced by this PR.
Independent findings
streaming-model.ts — reducer is correct. The pure fold over 13 StreamEvent variants carries an exhaustiveness guard at the default branch. streaming: event.type !== 'done' / done: event.type === 'done' reset correctly on every non-done event for multi-turn folding; the snapshot immutability tests cover this.
stream-aggregation.ts — 60 ms coalescing window is cleanly extracted and framework-neutral.
packages/cli/package.json — @opentui/core and @opentui/react are in devDependencies, not dependencies; not shipped to users.
package-lock.json — native platform packages (@opentui/core-darwin-arm64, -linux-x64, -win32-x64, etc.) are all "optional": true; a non-matching platform skips them cleanly.
No independent blockers.
Cross-check against existing reviews
qwen-code-ci-bot R1-1 ("hand-rolls a lexer + regex instead of the TypeScript compiler API"): REFUTED. The current head uses ts.createSourceFile and walks ImportDeclaration / ExportDeclaration / isImportTypeNode / isCallExpression AST nodes.
qwen-code-ci-bot R1-20 ("entry-point guard compares non-realpathed process.argv[1]"): REFUTED. Lines 393-394 read realpathSync(process.argv[1]) === fileURLToPath(import.meta.url) — realpathSync is present.
yiliang114 [P1] "Resolve relative imports from the physical symlink target": confirmed still open at HEAD — see inline.
A symlink's bytes live wherever the link points, but the gate resolves a file's relative imports from the path it was reached at. Following links therefore desyncs lexical resolution from physical location and can report an escape that physically stays in the rule root, or pass an import that physically escapes — the latter reproduced end-to-end on the prior head via a link into a skipped dist directory. Neither direction is auditable link-by-link, and the tree commits no symlinks, so the walker now reports any symlink and fails the gate instead of following it. This also removes the relative()-based containment predicate, which accepted cross-volume absolute results on Windows.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-review: b53b58d3 — P1 fully resolved
Original P1 (dirname(file)): resolved by not following any symlink at all. Any symlink in the scanned tree lands in symlinks[] and fails the gate immediately — files[] is now exclusively real file paths, so the dirname(file) site at the relative-import resolver is always correct. No partial fix needed.
The code is also simpler after the change: visitedDirs, realpathSync, statSync, relative, and escapesRoot are all removed.
Third new test ('fails closed on a link whose lexical path masks a physical escape') is an executable regression witness for the exact false-negative I described — a deep in-root symlink whose physical target's .. steps reach packages/cli but whose lexical location's do not. ✓
CI still red on Test ubuntu-latest — same pre-existing vitest worker timeout; all 1779 tests pass.
No remaining open findings. GitHub blocks self-approval, so flagging here: the fix is complete and ready for @yiliang114 to dismiss and re-review.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
The child-symlink path is now correctly fail-closed, and the focused checks pass. One root-boundary bypass remains; details are inline.
readdirSync follows a symlink transparently, so a rule root that is itself a link — or whose ancestor components are — let a commit substitute the protected scan with a clean tree elsewhere while the configured path read unchanged and no in-tree symlink diagnostic fired (both forms reproduced: swapping packages/cli/src/ui/model for a link exited 0, as did swapping the ui ancestor). Every path component below the repo root is now lstat-checked before enumeration and any symlink fails the gate.
…ion gate
Self-audit found three statically knowable framework-reference forms the
AST walk skipped: import-equals (`import x = require("ink")`, with or
without the export modifier), `require.resolve("ink")`, and
`import.meta.resolve("ink")` — the latter two name a framework package
even though they never load it. All three are now classified with probe
tests; the namespace form of import-equals (no module specifier) stays
ignored. The dynamic-import operator aliasing limitation (`const i =
import; i("...")`) is documented in the detector contract rather than
silently left open.
|
Self-audit of the dependency-direction gate (5310a86), run as an adversarial probe suite against the detector and walker rather than waiting for review to surface gaps: Fixed (were silently skipped by the AST walk):
All three have probe tests; suite is 45/45; the gate still passes on the real tree (1276 core files scanned). Probed and confirmed already caught: symlink chains (link → link → banned file), directory-symlink smuggling of a banned tree, dangling symlinks, Disclosed limitations (documented in the detector contract, not silently left):
The probe suite covers 13 attack scenarios; 12 catch, the 1 remaining is the disclosed aliasing limitation. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
The previous PASS bypass is fixed, and the focused gate suite passes 45/45. One ordering blocker remains: root-path validation still runs after enumeration, so the checker can traverse outside the repository before rejecting the link; details are inline.
Mutation audit found the main() wiring unguarded: removing the root-path symlink check, the in-tree symlink failure loop, the violation aggregation, or the populated-root guard left every unit test green while a manual probe showed the bypass re-opening. Six subprocess tests now run the real script against a fixture checkout (clean tree, symlinked rule root, symlinked ancestor, banned import, in-tree symlink, empty rule root) and assert exit codes and diagnostics; each of those four deletions now fails the suite.
…on gate The root-path symlink check ran after requirePopulatedRoot() had already walked both roots, so a symlinked rule root pointed at an external directory got its files read and parsed before the link was diagnosed, and an empty external target exited through the populated-root guard without ever reaching the symlink diagnostic. Reject any link in the rule-root path components before enumeration and exit immediately. The end-to-end fixture tests now assert the redirected target is never scanned (a banned import in the substitute stays out of the output) and that an empty substitute reports the symlink error, not the empty-root error.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Code review complete on head 8727c47. All R1 findings and the two P1 blockers are resolved: the dep-direction gate is now AST-based, symlinks fail closed (rule-root paths validated before enumeration), import-equals/resolution probes are classified, the model layer is hardened (StreamingModelState rename, exhaustiveness guard, user-event + immutability tests), and renderer deps sit in devDependencies. No code blockers remain.
Note: this does not clear the direction gate — the triage CHANGES_REQUESTED on #8662 (migration go/no-go, escalated to @wenshao @pomelo-nwu) is a maintainer decision, not a code issue.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Maintainer review — approved
Re-reviewed at head 8727c479. All five P1 findings I raised earlier are now fixed on this head, verified against the current source (not just the author's roll-up):
- P1 specifier coverage →
findImportsnow walksImportTypeNode(type X = import("…")) and accepts interpolation-free template literals viastaticSpecifier, alongsideimport-equals,require.resolve, andimport.meta.resolve. - P1 symlink escape / relative-import resolution → the walker now fails closed on any symlink (
listSourceFilesrecords and never follows), which removes the lexical-vs-physical resolution class entirely rather than papering over it. - P1 symlinked rule root →
symlinkedPathComponentslstat-checks every component below the repo root, andmain()rejects a linked root/ancestor before eitherrequirePopulatedRoot()call — so no enumeration or parsing happens against a redirected root.
Gate wiring is in place and correct: npm run check:tui-dep-direction (root package.json L94) + the ci.yml step, and @opentui/* sits in devDependencies until the first runtime import lands. The streaming model is a clean pure reducer (no input mutation, never exhaustiveness guard, StreamingModelState named apart from the ui/types.ts enum).
CI: all critical jobs green on this head (Real daemon E2E / Java 11, the full Java matrix, Desktop Shell, TruffleHog, CVE audit). Test (ubuntu-latest, Node 22.x) is still re-running — the prior two red marks were an external cancellation and a vitest worker-RPC timeout with 0 assertion failures, which the author diagnosed and I'm treating as infra flake. Nothing in this diff touches test infra, so I'm not blocking approval on that job.
Scope note: this is a dependency-ordered infra batch (#8662); the model modules gain consumers in later batches and remain covered by tests + the arch gate. That sequencing is the approved plan.
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. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": empirical mutation run (remove cancelTimer() from flushNow, npx vitest run src/ui/model/stream-aggregation.test.ts in packages/cli) — worktree has no node_m….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/model/streaming-model.ts:238 — [probe] Reducer finalizes only the trailing assistant item (open thinking/tool/task items and mid-list assistants strand live flags after done)packages/cli/src/ui/model/stream-aggregation.ts:79 — [probe] flushNow's cancelTimer() is unwitnessed — mutant silently stalls window-driven deliverypackages/cli/src/ui/model/stream-aggregation.ts:105 — [probe] discard()'s cancelTimer() is unwitnessed — mutant flushes a retry's events prematurelypackages/cli/src/ui/model/streaming-model.test.ts:277 — [probe] Multi-turn fold test never pins item-id continuation across done (nextSeq reset mutant survives)packages/cli/src/ui/model/streaming-model.test.ts:291 — [probe] No test pins that task-start closes a trailing streaming assistant item
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：empirical mutation run (remove cancelTimer() from flushNow, npx vitest run src/ui/model/stream-aggregation.test.ts in packages/cli) — worktree has no node_m…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
💬 Review comments (head 8727c47)
在既有两轮机器审查与人工审查之后独立复核了当前 head，以下发现未见此前讨论。纯静态审查，未运行测试。
Independent re-review at the current head, after the two bot rounds and the human review; the findings below have not been raised before. Static-only, no tests executed.
🎉 值得肯定 / Highlights
R1 的全部修复与五个 P1 已逐点核对确认落地：AST 化扫描、符号链接在枚举前拒绝、@opentui/* 移入 devDependencies 并锁版、StreamingModelState 改名、never 穷尽守卫、测试补齐。287 行 reducer 逐分支推演无边界错误、不可变契约成立；聚合器定时器生命周期闭合；门禁对符号链接/空根/不可列举目录全链路 fail-closed 且顺序正确。
All R1 fixes and the five P1s were verified as landed, point by point: AST-based scanning, symlinks rejected before enumeration, @opentui/* moved to devDependencies with pinned versions, the StreamingModelState rename, the never exhaustiveness guard, and the test expansions. The 287-line reducer holds up under per-branch scrutiny with an intact immutability contract; the aggregator's timer lifecycle is closed; the gate fails closed across symlinks, empty roots, and unlistable directories in the correct order.
🟡 Important — 规则 1 可被裸包名穿透 / Rule 1 bypassed by bare package imports
scripts/check-tui-dep-direction.mjs:303-313
问题 / Problem: noRelativeIntoCli 只在 imp.spec.startsWith('.') 分支内判定，只拦相对路径；而 cli 包名是 @qwen-code/qwen-code（packages/cli/package.json），npm workspaces 会在根 node_modules 建符号链接使其可解析。裸导入既非封禁家族、也非相对路径。
The noRelativeIntoCli check only runs inside the imp.spec.startsWith('.') branch, so it only catches relative specifiers. But the cli package is named @qwen-code/qwen-code (see packages/cli/package.json) and resolves through the workspace symlink in the root node_modules. A bare import is neither a banned family nor dot-prefixed.
失败场景 / Failure scenario: 在 packages/core/src 中写 import { ... } from '@qwen-code/qwen-code' 门禁直接 PASS——恰是规则 1 要阻断的 core→cli 方向；eslint 侧无 import/no-unresolved 兜底。这与第二轮已列的两条（规则 2 的 selfContained、noRelativeIntoCli 的测试覆盖）是不同问题。
import { ... } from '@qwen-code/qwen-code' inside packages/core/src passes the gate — exactly the core→cli direction Rule 1 exists to block; eslint has no import/no-unresolved backstop. This is distinct from the two round-2 items (Rule 2 selfContained, and test coverage of the noRelativeIntoCli branch).
建议修复 / Suggested fix:
const CLI_PACKAGE_NAME = JSON.parse(
readFileSync(join(CLI_PACKAGE, 'package.json'), 'utf8'),
).name;
// in checkRule, beside the relative-specifier branch:
if (
rules.noRelativeIntoCli &&
(imp.spec === CLI_PACKAGE_NAME ||
imp.spec.startsWith(CLI_PACKAGE_NAME + '/'))
) {
violations.push(
` ${rel} ${imp.kind} '${imp.spec}'` +
` (bare import reaches into packages/cli)`,
);
}🟢 Nits
-
重复 start 事件产生幽灵条目 / Duplicate start events create ghost items —
packages/cli/src/ui/model/streaming-model.ts:167-178, 200-211
tool-start/task-start无条件 push，而 output/end/progress 只按 id 命中第一项。触发：重试/重连重放同一个tool-start{id}→ 两个同 id 项，后续输出只喂第一个，第二个永久done: false空转。建议幂等替换（已存在同 id 项则重置），或至少在StreamEvent类型上声明 start id 唯一。
tool-start/task-startpush unconditionally while output/end/progress only match the first item by id. A replayed start (retry/reconnect) yields two same-id items; later output feeds only the first and the second staysdone: falseforever. Suggest idempotent replacement (reset when the id exists), or at least documenting start-id uniqueness onStreamEvent. -
SKIP_DIRS 静默缩扫描 / SKIP_DIRS silently shrinks the scan —
scripts/check-tui-dep-direction.mjs:55, 89-90
node_modules/dist/.git命中即无条件跳过，与 docblock “must not shrink silently” 的承诺不一致——其余异常（不可列举、符号链接、空根）都已 fail-closed。触发需要同 PR 改 .gitignore 配合（评审可见），故定 Nit：建议与符号链接同等待遇，收集后在 main() 判失败。
Matched directories are skipped unconditionally, inconsistent with the docblock's "must not shrink silently" pledge — every other anomaly (unlistable dirs, symlinks, empty roots) already fails closed. Exploitation requires co-modifying .gitignore in the same PR (visible in review), hence Nit: collect and fail in main() like symlinks. -
exit(1)紧随stdout.write，CI 管道上失败原因可能被截断 /exit(1)right afterstdout.writemay truncate the failure reason on CI pipes — 脚本从node:process引入exit/stdout，4 处exit(1)均紧随 write。Node 对管道的 stdout 写入是异步的，process.exit不等待 flush；门禁红时最后一条报错恰是最需要的那条。建议改process.exitCode = 1; return;让进程自然退出冲刷缓冲。
The script bindsexit/stdoutfromnode:process; all fourexit(1)sites immediately follow a write. stdout writes to pipes are async andprocess.exitdoes not flush — the last diagnostic is exactly the one lost when the gate fails. Suggestprocess.exitCode = 1; return;so the process exits naturally and flushes. -
chmod-000 测试在 root 身份的 runner 上必红 / The chmod-000 test fails on root-identity runners —
scripts/tests/check-tui-dep-direction.test.js:372-389
chmodSync(blocked, 0o000)对 root 无效（DAC 豁免），readdirSync仍成功，断言unreadableDirs.length > 0失败；而testjob 的runs-on可由分类器换到自建池，ci.yml 中存在[ "$(id -u)" = '0' ]的 root lane。建议it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)。
chmodSync(blocked, 0o000)is a no-op for root (DAC exemption), soreaddirSyncstill succeeds and theunreadableDirs.length > 0assertion fails; thetestjob'sruns-oncan be swapped to a classifier-provided self-hosted pool, and ci.yml contains a[ "$(id -u)" = '0' ]root lane. Suggestit.skipIf(process.platform === 'win32' || process.getuid?.() === 0).
💡 信息项 / Note
该门禁目前不是 required status check（main ruleset 整体如此，属仓库既有策略），强制力依赖人工评审；门禁稳定后可考虑入 ruleset。
The gate is not currently a required status check (the main ruleset has none today; repo-wide policy), so its enforcement relies on human review attention. Consider adding it to the ruleset once stable.
结论 / Verdict
无 Critical、1 个 Important（约 10 行可修）→ 评论级，不阻塞批准之外的流程判断。另注意：当前合入的真实阻塞是方向门禁——迁移 go/no-go 的维护者决定与 docs/design/ 设计文档，而非代码本身。
No Critical, one Important (~10-line fix) → comment-level. Also note: the real merge blocker today is the direction gate — the maintainer go/no-go decision on the migration and the docs/design/ design doc — not the code itself.
- Rule 1 now blocks bare imports of the cli package's own name (read from its manifest so a rename cannot decouple the check): the workspace symlink makes `import ... from '@qwen-code/qwen-code'` resolve from core, which is exactly the core->cli reach the rule forbids. - Classify the remaining statically knowable forms: the vi module-loading family (mock/doMock/importActual/importMock), module.require, require.main.require, and ambient module declarations. - Fail closed on skipped-directory names (node_modules/dist/.git) inside a protected root instead of silently shrinking the scan. - Replace process.exit(1) with process.exitCode + natural unwind so buffered diagnostics flush to CI pipes before termination. - Add the missing witnesses: noRelativeIntoCli relative and bare arms, the ui/model rule arm (both messages), the core-root symlink arm, the empty ui/model arm, a .js extension fixture, an unlistable-directory e2e case, the skippable-directory case, @solid/ and @react-* table rows, and the entry-guard import-suppression side.
A retry/reconnect can re-emit a tool-start or task-start whose id already exists; the unconditional push created a duplicate that later events (which match the first id hit) left stranded at done:false forever. Reset the existing item instead. Also extend the fold immutability matrix with the two rewrite branches it did not exercise (closeTrailingAssistant via text->tool-start and text->done, and the thinking-end rewrite), so an in-place mutation in either branch now turns the suite red.
|
@ytahdn Thanks for the careful independent review — every item is addressed in Important — Rule 1 bypassed by bare package imports: fixed. The gate reads the cli package's name from its own manifest (so a rename cannot decouple the check) and now blocks any bare import of it under Rule 1, alongside the relative-path arm. Witnessed at both levels: a unit case on the rule and an end-to-end fixture case ( Nit 1 — ghost items from duplicate starts: fixed by taking the idempotent-replacement option: Nit 2 — SKIP_DIRS silently shrinks the scan: fixed — Nit 3 — Nit 4 — chmod-000 test under root: both chmod-based tests now carry On the note (gate not in the required ruleset): agreed it is worth considering once the gate settles; leaving that call to the maintainers. Verification: gate suite 67/67 + e2e witnesses for every fail-closed arm; streaming model suite 20/20; mutation spot-checks confirm each new branch turns its witnesses red (bare-import arm, vi-family arm, ambient arm, closeTrailingAssistant in-place mutant, replay-reset revert); the gate passes on the real tree (1276 core files). |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
✅ APPROVE
在 head f35267c 上复核：上一轮审查提出的五条问题（一条 Important、四条 Nit）已全部确认修复，新增代码未发现新问题。
Re-verified at head f35267c: all five findings from my previous review (one Important, four Nits) are confirmed fixed, and the new commits introduce no new issues.
修复核对 / Fix verification
-
🟡 规则 1 裸包名穿透 — 已修复：
CLI_PACKAGE_NAME改为直接从 cli 包的 manifest 读取（包改名不会使检查脱钩），判定独立于相对路径分支，同时覆盖spec === name与name + '/'子路径两种形态，报 "bare import reaches into packages/cli"；单测与端到端见证均已补齐。
(Rule 1 bare-package bypass — fixed:CLI_PACKAGE_NAMEis now read from the cli package's manifest (so a rename cannot decouple the check), the check sits outside the relative-specifier branch and covers bothspec === nameand thename + '/'subpath form, reporting "bare import reaches into packages/cli"; unit and end-to-end witnesses added.) -
🟢 重放 start 幽灵条目 — 已修复：
tool-start/task-start改为幂等重置（已存在同 id 项则替换而非 push），注释写明重试/重连重放场景；新增两个专项测试，不可变矩阵补齐两个重写分支。
(Replayed-start ghost items — fixed:tool-start/task-startnow reset an existing same-id item instead of pushing a duplicate; the retry/reconnect replay scenario is documented; two dedicated tests added and the immutability matrix extended with the two rewrite branches.) -
🟢 SKIP_DIRS 静默缩扫描 — 已修复：命中 node_modules / dist / .git 的目录现被收集并在 main() 逐条报错判失败，与符号链接同等待遇，fail-closed 语义自此完整。
(SKIP_DIRS silent scan shrinkage — fixed: matched directories are now collected and each reported as an error in main(), failing the gate; treated like symlinks, completing the fail-closed posture.) -
🟢 exit(1) 截断管道诊断 — 已修复：所有失败路径改为
process.exitCode = 1; return;自然退出以冲刷缓冲（含requirePopulatedRoot返回 null 的改造），原因注明在注释中。
(stdout truncation on exit(1) — fixed: every failure path now setsprocess.exitCode = 1; return;so buffered diagnostics flush to CI pipes before termination (includingrequirePopulatedRootreturning null); rationale documented in comments.) -
🟢 chmod-000 测试在 root 身份的 runner 上必红 — 已修复：两处相关测试的 skipIf 均扩展为
process.platform === 'win32' || process.getuid?.() === 0。
(chmod-000 test failing on root runners — fixed: skipIf at both sites extended withprocess.getuid?.() === 0.)
附带观察 / Also observed
这两个提交同时落地了第二轮审查的多条项目：vi 模块族（mock/doMock/importActual/importMock）、module.require、require.main.require 与 ambient 声明的分类，门禁测试新增约 183 行补齐此前缺失的见证。四类异常输入（符号链接、空根、不可列举目录、可跳过目录）全部拒绝放行，规则根路径预检仍先于枚举。
The same commits also landed several round-2 items: classification of the vi module-loading family (mock/doMock/importActual/importMock), module.require, require.main.require, and ambient module declarations, plus ~183 lines of new gate tests covering previously missing witnesses. All four anomaly classes (symlinks, empty roots, unlistable directories, skippable directories) fail closed, and rule-root path validation still runs before enumeration.
结论 / Verdict
代码侧无阻塞项，批准。其余合入前提为非代码项（迁移方向决定与设计文档），由维护者流程在对应追踪 issue 中记录即可。
No code blockers remain; approving. The remaining merge preconditions are non-code (the migration direction decision and the design doc) and only need to be recorded in the tracking issue through the maintainer process.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Reviewed at f35267c against base fdd7248 and verified locally on this head:
npm run check:tui-dep-direction: PASS (1300 core files / 9541 specifiers, model layer clean)vitest run scripts/tests/check-tui-dep-direction.test.js: 65 passed, 2 skippedvitest run src/ui/model/(packages/cli): 31 passedtsc --noEmitin packages/cli: clean
No blockers. One non-blocking observation for a later hardening pass: findImports has no branch for triple-slash /// <reference types="react" /> directives, so type-only coupling via a reference directive can slip past the gate. Types-only and requires deliberate use, so fine to fold into a subsequent gate iteration rather than this batch.
|
Released in v0.22.3. |
4 participants
What this PR does
First infra batch of the OpenTUI migration tracked in #8662. It adds three things, all additive: the locked OpenTUI renderer dependencies, a dependency-direction architecture check, and a framework-neutral streaming model layer. No existing code path changes behavior; the default ink renderer is untouched.
Why it's needed
The migration lands in dependency-ordered batches, and this batch provides the foundation the later batches build on. The renderer dependencies are pinned (not ranged) so renderer behavior stays reproducible across every landing batch. The architecture check statically enforces that the business core and the new streaming-model layer stay free of renderer-framework imports (react / solid / ink / opentui), which is what keeps the migration reversible and lets both renderers share one streaming source of truth. The streaming model itself folds agent-loop stream events into render-ready history items; it was extracted from the migration branch's renderer backend and carries its full test suite. The modules gain consumers in later batches (the ink-side wiring refactor is intentionally deferred because the target file has since diverged on main and deserves its own focused batch).
Reviewer Test Plan
How to verify
node scripts/check-tui-dep-direction.mjs— scans packages/core (1277 files) and the new model layer, exits 0 with "PASS — dependency direction holds".cd packages/cli && npx vitest run src/ui/model/— 24 tests pass (2 files).npm installproduces only the opentui entries and their per-platform native packages in the lockfile;npm run buildand typecheck are clean.Evidence (Before & After)
N/A (infrastructure; no user-visible change)
Tested on
Environment (optional)
Node 22.23.1;
npm run buildclean afternpm install.Risk & Scope
Linked Issues
Part of #8662 (Infra batch). Follows #10128, which pulled the renderer-independent startup fixes forward.
中文说明
本 PR 做了什么
#8662 跟踪的 OpenTUI 迁移的第一个 Infra 批次。纯增量地加入三样东西：锁版的 OpenTUI 渲染器依赖、一个依赖方向架构检查脚本、一个框架无关的 streaming model 层。不改变任何既有代码路径的行为；默认 ink 渲染器不受影响。
为什么需要
迁移按依赖序分批落地，本批提供后续批次的地基。渲染器依赖锁版（不用范围号），保证跨批次的渲染器行为可复现。架构检查静态强制业务核心与新的 streaming-model 层不引入任何渲染器框架 import（react / solid / ink / opentui），这是迁移可回退的保证，也让两个渲染器共享同一份 streaming 事实源。streaming model 把 agent-loop 流事件折叠成可直接渲染的历史条目，从迁移分支的渲染器后端中提取而来，自带完整测试。这些模块在后续批次获得消费者（ink 侧接线重构刻意推迟——目标文件在 main 上已有大幅演进，值得单独一批专注处理）。
评审测试计划
如何验证
node scripts/check-tui-dep-direction.mjs——扫描 packages/core（1277 文件）与新 model 层，exit 0 输出 "PASS — dependency direction holds"。cd packages/cli && npx vitest run src/ui/model/——24 个测试通过（2 个文件）。npm install只在 lockfile 中新增 opentui 条目及其平台原生包；npm run build与 typecheck 干净。前后对比证据
N/A（基础设施；无用户可见变化）
测试环境
环境（可选）
Node 22.23.1；
npm install后npm run build干净。风险与范围
关联 Issue
属于 #8662（Infra 批次）。跟在 #10128 之后——后者把渲染器无关的启动修复前置了。