feat(opentui): Add the backend composition root (Batch 5) - #10696
Merged
Conversation
…and host Batch 5 (slice 1/3): the OpenTUI backend has all its pieces (dispatcher, dialogs, live turn, folding model) but no composition root that owns state and wires them. Add the three self-contained leaves first: - OpenTuiErrorBoundary: React boundary mirroring the ink shared ErrorBoundary (recordForExitEcho + consumeLastRenderError) for the @opentui/react tree. - OpenTuiRuntime: process-level lifecycle (runtime sidecar, dual-output bridge, remote-input watcher, memory-pressure monitor, ordered shutdown). - OpenTuiAppHost: concrete OpenTuiCommandHost + SessionSwitchHost — owns command history with faithful useHistory parity, pending/btw/session state, shell allowlist, idle/processing, and delegates the live transcript and modal confirmations to shell-supplied seams. Host/runtime/boundary are additive and unwired (behavior unchanged); dialog mounting and the app shell follow.
Complete the Batch 5 backend composition root: the exhaustive dialog mount routes every OpenTuiDialogRequest to its component, and the app shell assembles the command bridge (host + dispatcher + gateway), dialog mount, and error boundary, wiring the composer through the gateway so dispatch outcomes open dialogs, reach the live-turn seam, or quit.
Adds a fixed banner slot below the transcript, hidden while a dialog is open, to mirror the ink DefaultAppLayout so populating the parity gap (G-3) later does not re-open the shell layout.
…ead dispatch chain executeSlashCommand and its resolution/mapping helpers were superseded by OpenTuiSlashDispatcher and the slash gateway; no production path reached them. slash-dispatch now exports only the loader that commands-dispatch and the composer still consume.
github-actions
Bot
added
the
review/self-reported
13 tasks
Collaborator
Collaborator
|
Thanks for the PR! Batch 5 of the ink→OpenTUI migration — the gate checks all pass. Template: complete ✓ Problem: this is planned migration work, not a speculative change — Batch 5 is named in the migration design doc ( Direction: aligned. Additive-only per the design's rollout plan, default renderer stays ink, and the shell is intentionally not mounted yet (that is the renderer-activation batch). No auth/sandbox/telemetry surface touched. Size: no core-module paths are touched (everything is Approach: the thin-composition-root shape matches the design (seams for transcript/model-turn instead of forking Risk: no elevated risk signals — none of the changed paths match the repo's revert-correlated path list. Moving on to code review. 🔍 中文说明感谢贡献！这是 ink→OpenTUI 迁移的 Batch 5，各项门槛检查均通过。 **模板：**完整 ✓ **问题：**这是计划内的迁移工作，不是投机性改动——Batch 5 在迁移设计文档（ **方向：**对齐。按设计文档的推进计划纯增量，默认渲染器仍是 ink，该 shell 有意暂不被入口挂载（那是渲染器激活批次的工作）。不涉及 auth/sandbox/telemetry 面。 **规模：**未触及核心模块路径（全部在 **方案：**薄组合根的形态与设计一致（transcript/模型轮次作为 seam，而不是 fork **风险：**无升级风险信号——改动路径均未命中仓库的易回滚路径清单。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-dev-bot
previously approved these changes
Sep 1, 2026
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at head 71686431.
- Verified the "nothing mounts it yet" claim against the tree:
opentui-app-shellhas no non-test importers, so the ink renderer path is untouched; the five new modules compose only among themselves. - The
slash-dispatch.tstrim is a clean dead-code removal: the sole surviving exportloadInteractiveCommandskeeps its live consumers (commands-dispatch.ts,input-prompt.tsx) intact and test-pinned. - The failure posture is fail-closed where it matters: the confirmation bridge auto-denies instead of hanging a command, a failed dispatcher init rejects later submissions with the recorded reason instead of misrouting them to the model, and subtree render errors land in the error boundary.
- No open prior reviews or threads; no new Critical issues found. CI on this head has no failures (Test/Integration still running); per the channel convention the call is on the review itself.
yiliang114
reviewed
Sep 1, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Review findings (verified on 7168643; local tests for all six new/changed modules pass 50/50):
P2 — opentui-app-shell.tsx confirmations.presentShell (~line 127): a non-empty confirmation list parks the promise forever. The auto-deny branch only fires for an empty list; for any commandsToConfirm.length > 0 the resolve is stashed in confirmationRef and nothing in this batch ever calls it (grep confirms the only references are inside presentShell itself). OpenTuiSlashDispatcher.run() awaits host.presentShellConfirmation on a confirm_shell_commands result (commands-dispatch.ts:831), so once Batch 6 mounts this shell, the first such command hangs run() indefinitely — the gateway then keeps busy = true and rejects every later slash submission until restart, and setIsProcessing never clears. The inline comment ("without a renderer the shell must not hang a command, so it auto-denies") and the test claim ("auto-denies the confirmation bridge so no command can hang") only hold for the empty case. Suggest auto-denying (resolve Cancel) whenever no confirmation renderer exists yet, so the seam fails safe until Batch 6 swaps in the real bridge.
P3 (note for Batch 6) — the host memo and dispatcher effect depend on raw callback-prop identities (getSessionStats, onToggleVim, transcript/reset); inline callbacks at the mount site would recreate the host and re-run the full interactive command loader per render. Stabilize via refs when wiring Batch 6.
Otherwise clean: slash-dispatch trim leaves only the still-consumed loadInteractiveCommands (no dangling imports of removed symbols); runtime pressure monitor is idempotent with proper shutdown; dialog-mount async helpers all catch internally; error boundary swallows nothing. No production impact today (shell not mounted, ink still default), but the P2 should be fixed before activation.
chiga0
commented
Sep 1, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
Author
There was a problem hiding this comment.
Independent review @ head `71686431` — Batch 5 (backend composition root)
Scope: All 12 changed files. Dead-code removal (slash-dispatch.ts) and five new modules (opentui-app-shell, opentui-dialog-mount, opentui-error-boundary, opentui-host, opentui-runtime), each with a test counterpart.
Excluded: macOS/Windows platform behaviour (no host); Test (ubuntu-latest) and Integration Tests (no-AK) were still in-progress at review time — see CI section below.
F1 — Major: presentShell non-empty list hangs; test only covers the empty path
File: packages/cli/src/ui/opentui/opentui-app-shell.tsx lines 127–133
presentShell(commandsToConfirm: readonly string[]) {
return new Promise<ShellConfirmationResolution>((resolve) => {
confirmationRef.current = { resolve }; // stored but never read
if (commandsToConfirm.length === 0) {
confirmationRef.current = null;
resolve({ outcome: ToolConfirmationOutcome.Cancel }); // ← only path that resolves
}
});
},When commandsToConfirm.length > 0 the promise constructor exits without resolving. confirmationRef.current receives the resolver, but no other path in the component ever reads that ref and calls .resolve(). The promise hangs indefinitely.
PR description claims: "The confirmation bridge auto-denies (Cancel / false) so no command can hang waiting for a renderer this shell does not own."
Test (opentui-app-shell.test.tsx — "auto-denies the confirmation bridge") only calls presentShellConfirmation([]) (line 73 of the test). A call with ['dangerous-cmd'] would time out rather than resolve. The stated guarantee is unverified.
Blast radius now: zero — the shell is unimported and unreachable in production. Before Batch 6 ships: any command that passes a non-empty allowlist (e.g. /quit, /clear in a managed session) to presentShellConfirmation will hang the dispatcher.
Fix: either auto-deny for all lists in this stub shell (resolve(...) outside the if), or note explicitly in the docblock that the non-empty path is deferred to Batch 6 and add a timeout/auto-deny safeguard. Either way the test should cover both paths.
Cross-check against existing review (qwen-code-dev-bot, APPROVED @ 71686431)
| Their claim | Verdict |
|---|---|
| "nothing mounts it yet… five new modules compose only among themselves" | ✅ Confirmed — gh search code found zero non-test importers |
| "slash-dispatch.ts trim is a clean dead-code removal" | ✅ Confirmed — all four removed exports (isSlashCommandInput, resolveSlashCommand, SlashEffect, SlashDispatchEnv) appear only in slash-dispatch.ts and its test on the main branch |
| "the confirmation bridge auto-denies instead of hanging a command" | |
| "a failed dispatcher init rejects later submissions" | ✅ Confirmed — gateway.failInit(error) wires this; covered in the test |
| "subtree render errors land in the error boundary" | ✅ Confirmed — test catches a subtree render error inside the error boundary exercises this |
CI at head 71686431
| Suite | Result |
|---|---|
| Desktop Shell (ubuntu-22.04) | ✅ success |
| Desktop Shell (windows-2022) | ✅ success |
| Secret scan, CVE audit | ✅ success |
| Test (ubuntu-latest, Node 22.x) | ⏳ in_progress at review time |
| Integration Tests (no-AK, No Sandbox) | ⏳ in_progress at review time |
| Test (macos-latest, Node 22.x) | |
| Test (windows-latest, Node 22.x) | |
| Integration Tests (CLI, No Sandbox) |
macOS/Windows test suites are skipped. The new modules have no platform-branch code visible in the diff; the skip is a pre-existing CI topology choice, not introduced by this PR. Disclosed per §D.
Summary
The dead-code deletion is clean and the five new modules wire correctly. One design-contract gap (F1) contradicts both the PR description and the test's stated guarantee, and will become a live hang in Batch 6 if not addressed. No other blockers found.
Reviewed with AI assistance.
Collaborator
Code reviewI proposed my own shape for this batch before reading the diff — one app-shell component composing error boundary + dialog mount + composer over a stable class host, a runtime sidecar extracted from the ink entry, plus per-piece tests — and the PR's structure matches that almost exactly. The host faithfully mirrors 1. The confirmation bridge auto-denies only the empty list — a non-empty shell confirmation hangs the dispatch loop forever. In 2. Suggestion — image paths fold into the prompt as literal sequenceDiagram
participant P1 as Composer (OpenTuiInputPrompt)
participant P2 as Gateway (OpenTuiSlashGateway)
participant P3 as Dispatcher (OpenTuiSlashDispatcher)
participant P4 as App shell (OpenTuiApp)
participant P5 as Dialog mount (OpenTuiDialogMount)
participant P6 as Entry seams (onSubmitPrompt, onQuit)
P1->>P2: submit text
P2->>P3: dispatch once attached (failInit records load errors)
P3-->>P2: outcome, or false for plain prompts
P2-->>P4: settlement (rejected submissions notify, not misroute)
P4->>P4: applyOutcome
alt open_dialog
P4->>P5: composer swapped for the dialog
P5-->>P4: onClose restores the composer
else submit_prompt or plain prompt
P4->>P6: onSubmitPrompt(content)
else quit
P4->>P6: onQuit(messages)
end
Files changed (12)
TestingThis is an unattended CI run, so no local test execution — the evidence below is the PR's own CI at fetch time. The author's claim of "61 files / 967 tests pass" locally is noted as the author's claim, not independently re-run. No failures at fetch time; the ubuntu unit leg was still running (not polled). The macOS/Windows unit legs reported Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the one open behavioral claim: 中文说明代码审查：读 diff 前我先独立给出了自己的方案——一个 app-shell 组件，把 error boundary + dialog mount + composer 组合在一个稳定的 class host 之上，外加从 ink 入口提取的 runtime sidecar 和逐件测试——PR 的结构与之几乎完全一致。host 忠实复刻了 1. 确认桥只对空列表自动拒绝——非空的 shell 确认会让分发循环永久挂起。 2. 建议——图片路径以字面 测试：无人值守 CI 运行，不做本地执行——上方证据为抓取时刻 PR 自身 CI 的结果。作者"61 文件 / 967 测试通过"的本地声明仅作作者声明记录，未独立复跑。抓取时刻无失败；ubuntu 单测腿仍在运行（未轮询）。macOS/Windows 单测腿报告 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 2/5 — everything here is clean, well-tested, and faithful to the migration design, but the confirmation seam ships the exact hang it promises cannot happen, and that needs a fix before merge. Stepping back: the approach matches what I'd have proposed for this batch — a thin shell over a stable class host, seams where Batch 6 will plug in, tests pinning each wiring claim. The dead-chain trim is verified dead, the runtime sidecar is a faithful extraction, and nothing reachable from today's product changes. I'm not approving this because it "ran out of reasons to say no" — the direction and structure genuinely earn landing. The one thing I can't look past is finding 1 above: So: request changes on that single point — the Stage 2 comment has the details and the proposed fix. The 中文说明置信度：2/5 —— 整体干净、测试充分、忠实于迁移设计，但确认桥恰恰交付了它自己承诺不会发生的那种挂起，合并前需要修掉。 退一步看：这个方案与我对本批次的独立设想一致——薄 shell 架在稳定的 class host 之上，为 Batch 6 留出 seam，测试逐条钉住接线声明。死链删除已验证确为死代码，runtime sidecar 是忠实提取，当下产品的可达路径零改动。我并非因为"找不到反对理由"才不批准——方向和结构确实配得上合入。 唯一过不去的是上面的发现 1： 因此：仅就这一点请求修改——细节和建议修复见 Stage 2 评论。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
previously requested changes
Sep 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes on one specific point — the rest of the PR looks solid.
The shell confirmation bridge auto-denies only the empty command list: for a non-empty confirm_shell_commands resolution, presentShell stashes the resolver in confirmationRef and nothing ever resolves it (no confirmation dialog variant exists, and the await runs after the abort race, so ESC won't unblock it either). That is the exact hang the seam's comment and the PR description promise cannot happen — details and a one-line fix (always resolve Cancel, matching presentAction) are in my review comment above, finding 1. Please also add a test driving a non-empty list through the seam.
中文说明
仅就一个具体点请求修改——PR 其余部分都没问题。
shell 确认桥只对空命令列表自动拒绝：对非空的 confirm_shell_commands 结果，presentShell 把 resolver 存进 confirmationRef 后再无任何东西 resolve 它（不存在确认对话框变体，且该 await 发生在 abort race 之后，ESC 也无法解锁）。这正是该 seam 注释和 PR 描述承诺不会发生的那种挂起——细节与一行修复（无条件 resolve Cancel，与 presentAction 一致）见上方审查评论的发现 1。请同时补一个驱动非空列表穿过该 seam 的测试。
— Qwen Code · qwen3.8-max
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
The confirmation bridge auto-denied only the empty command list, so the non-empty list the dispatcher actually awaits left a promise nothing would ever resolve — hanging the dispatch loop and the gateway busy flag for the rest of the session once the shell is mounted. It now denies every request, matching the action-confirmation branch, and the pending-resolver slot it never used is gone. Composer image paths no longer flatten into an invented `[image] <path>` text part that nothing parses; they travel to the live-turn seam as a structured argument so the entry layer can build real image parts.
Collaborator
Author
|
Fixed in 63aa227 — replying to the blocker and the two notes. 1 / P2 / F1 — the confirmation bridge hung on non-empty lists. Confirmed against presentShell: () =>
Promise.resolve<ShellConfirmationResolution>({
outcome: ToolConfirmationOutcome.Cancel,
}),
presentAction: () => Promise.resolve(false),The pending-resolver slot had no reader anywhere in the tree, so Verified by mutation: restoring the length-0-only branch makes the strengthened test fail — the await never settles and vitest times the test out at 15 s — and the committed version passes. Evidence at the new head: 2 — onSubmitPrompt?: (content: PartListUnion, imagePaths?: readonly string[]) => void;A composer prompt passes its text plus 3 / P3 — host memo identity. Acknowledged, and deliberately not changed in this commit: there is no mount site yet, so a ref-stabilizing wrapper here would be unverifiable machinery that no test can exercise. Recorded as a Batch-6 activation requirement — the mount site must pass stable identities, otherwise every render recreates the host and re-runs the full interactive command loader. |
chiga0
requested review from
qwen-code-ci-bot,
qwen-code-dev-bot and
yiliang114
September 1, 2026 11:54
chiga0
commented
Sep 1, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
Author
There was a problem hiding this comment.
Re-review @ head 63aa2272 (1 new commit)
Prior finding status
| Finding | Status |
|---|---|
F1 — presentShell non-empty list hangs |
✅ Fixed |
F1 fix (fix(opentui): Make the composition-root seams fail safe):
confirmationRef removed entirely. Both presentShell and presentAction now resolve unconditionally with Cancel/false via Promise.resolve(...). The test adds the non-empty-list case (['rm -rf build', 'npm publish']) alongside the empty-list one. Fix is correct and the test is no longer vacuous.
Bonus fix in same commit: onSubmitPrompt signature changed from (content: PartListUnion) to (content: PartListUnion, imagePaths?: readonly string[]) — image paths are passed through as a structured argument instead of being folded into the prompt text. The companion test was updated ('passes pasted image paths through structured, not folded into the text'). This is a correct seam boundary: encoding image paths into parts belongs to the entry layer, not the shell.
CI at 63aa2272
| Suite | Result |
|---|---|
| Desktop Shell (ubuntu-22.04 / windows-2022) | ✅ success |
| Integration Tests (no-AK, No Sandbox) | ✅ success (was in_progress at round 1) |
| Secret scan, CVE audit | ✅ success |
| Test (ubuntu-latest, Node 22.x) | ⏳ in_progress |
| Test (macos-latest / windows-latest) | |
| Integration Tests (CLI, No Sandbox) |
No new blockers at this head. The two outstanding CI points are pre-existing topology choices, not introduced by this PR.
Reviewed with AI assistance.
chiga0
commented
Sep 1, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
Author
There was a problem hiding this comment.
R1 review @ 63aa2272 — 1 blocker + 3 majors (one finding withdrawn as already fixed by your last commit)
Round note: I reviewed 71686431, then 63aa2272 landed mid-review. Nothing here is posted against a SHA I did not read — anchors and every claim below were re-derived at 63aa2272.
Withdrawn: my R1-1 from the first pass, on presentShell. 63aa2272 replaced the never-read confirmationRef with an unconditional Promise.resolve({outcome: Cancel}), and its new comment names the mechanism ("a pending promise here would hang the dispatcher's run() (and the gateway's busy flag) for the rest of the session"). That fix is correct — commands-dispatch.ts:836-841 honours the Cancel and the gateway's finally becomes reachable. I had probed it as a wedge (second dispatch() rejected with A slash command is already running., first still pending 200 ms later) and I'm glad it's gone. But the same commit left its twin in place, which is R1-1 below.
Ran (linux / node v24.20.0 / vitest 3.2.7): npx vitest run src/ui/opentui/ → 61 files / 967 tests green at 71686431, matching the PR's stated evidence exactly; npx eslint --max-warnings 0 on the six new/changed modules → clean, zero output; probes 3/3 (gateway wedge) and 3/3 (host throw propagation).
Claims checked that hold, so nobody re-spends time on them:
- "removal of dead, test-only exports that had no production callers anywhere in the repo" — verified. All seven removed symbols (
isSlashCommandInput,SlashResolution,resolveSlashCommand,SlashEffect,SlashEffectWithNotice,SlashDispatchEnv,executeSlashCommand) have zero referencing files outsideslash-dispatch.ts/slash-dispatch.test.ts, counted at both merge base56f92c84a8and head, non-test included. - Nothing outside
packages/cli/src/ui/opentui/imports the shell, so the batch is genuinely unmounted.
Two description corrections:
- "OpenTUI remains behind
QWEN_TUI_RENDERER" — that env var exists only indocs/design/2026-08-28-opentui-migration-design.md(3 mentions, docs-only at merge base too); no code reads it anywhere. The batch is inert because nothing imports it, not because a flag gates it. Fine for Batch 5, but the Risk section shouldn't lean on a kill switch that does not exist yet. - "image paths fold into the submitted content" — as of
63aa2272they no longer fold:onSubmitPrompt(text, imagePaths)passes them structurally and the flattening was deleted. The new behaviour is the better one (it matches ink's attachment handling), so it is the test-plan line that is stale, not the code.
Not verified / not covered (stated, not implied):
npx tsc --noEmitinpackages/cliOOM'd in my environment (node heap limit), so "package-level tsc clean" is something I could not check. My harness also resolves@qwen-code/*through a checkout older than this PR's base, so a resolution error would have been meaningless — flagging that so my typecheck silence is not read as a pass.- No macOS/Windows run.
Test (ubuntu-latest, Node 22.x),Integration Tests (no-AK)andtriagewere allin_progresswhile I worked, so CI has no verdict on this head yet either. opentui-runtime.tssidecar lifecycle: not audited. Nothing outside it and its test readsdualOutputBridge/remoteInputWatcherat head — recorded, not filed.opentui-error-boundary.tsxis mounted atopentui-app-shell.tsx:277with no props, while the ink call site (ui/startInteractiveUI.tsx:262-274) passes both; I did not resolve whether that is a defect, so it is not filed either — whoever reviews Batch 6's mounting should look.- A lens claim that a failed dispatcher init permanently refuses plain prompts did not survive my probe: the gateway recovers cleanly on
attach()afterfailInit(). Not posted.
Reviewed with AI assistance.
yiliang114
previously approved these changes
Sep 1, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
The P2 from my earlier review is fixed in 63aa227: presentShell now denies every request outright (matching presentAction), the dead pending-resolver slot is gone, and no promise can park the dispatcher's run() / the gateway busy flag once Batch 6 mounts the shell. The bonus fix is also right: composer image paths travel to the live-turn seam as a structured imagePaths argument instead of an invented [image] <path> text part.
Verified locally on 63aa227: opentui-app-shell tests 12/12. Earlier verification on the prior head still applies: all six modules 50/50, slash-dispatch trim leaves no dangling consumers. The P3 memo-deps note stands as Batch-6 wiring guidance, not a blocker.
qwen-code-dev-bot
previously approved these changes
Sep 1, 2026
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed at head 63aa2272 (my approval on 71686431 was auto-dismissed by the fix push).
- The round's one blocking finding is fixed exactly as requested: the confirmation bridge now denies unconditionally (
presentShellresolvesCancelwithout storing a resolver,presentActionresolvesfalse), so a non-emptyconfirm_shell_commandslist can no longer park the dispatcher'srun()and the gateway busy flag; a regression test drives a non-empty list through the seam. The image-path handling change in the same commit (structuredimagePathssecond argument instead of flattening into prompt text) is a cleaner seam split and test-pinned. - Re-checked the rest of the delta (one commit, two files) — the previously reviewed guarantees still hold: nothing mounts the shell yet, the
slash-dispatchtrim keeps its live consumers, and the init-failure path still rejects instead of misrouting. - CI on this head has no failures (three jobs still running); per the channel convention the call is on the review itself. The bot's CHANGES_REQUESTED was cast on the superseded head and its finding is resolved here.
Collaborator
|
Verification note — head The blocking finding from the earlier review round is fixed at head, in the form that was asked for:
Sweep of the rest of the batch (12 files, +2725/−979) found no new Criticals: app-shell init effect is Local at head (tarball + tmux e2e N/A by design: CI at head: Test (ubuntu) and review-pr still pending at time of writing (everything else green). Approving is therefore left to a later pass once the remaining lanes land; nothing here blocks the two existing approvals. 中文摘要：在 head |
Extension consent stored a request that no renderer in this batch could answer, so the caller awaited a promise that never settles; route it through the bridge the shell already auto-denies. The host's caller-owned steps (subscribers, onChange, transcript reset) run inside the /resume and /branch commit window, where one throw rolls core back and deletes the branch being displayed, so isolate them. Make the session re-key an explicit shell seam that reports when nothing owns it, wire the permissions dialog's directory edits to real settings persistence, and say so when a settings row opens a sub-dialog the mount does not route.
chiga0
dismissed stale reviews from qwen-code-dev-bot and yiliang114
via
September 1, 2026 13:10
d8c17a4
Enter in the arena model picker is the only way to start a session, and it works by writing the command into the composer the entry layer owns. With no composer owner wired, the dialog closed and the selection vanished with it, so surface that the command was lost instead.
Collaborator
Author
|
Thanks — recorded. Status after your snapshot at The four inline threads from the same round are now fixed, answered and resolved, on head
One deferral is recorded rather than fixed: routing settings rows into their sub-dialogs needs a dialog-change channel the mount does not have, plus a composer owner — filed as U-9 in the Batch 6 table on #8662. Local at |
qwen-code-dev-bot
approved these changes
Sep 1, 2026
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed at head 0289df07 (my approval on 63aa2272 was auto-dismissed by two new fix commits).
- Read both deltas in full. They close review-found gaps in the composition root by giving silent no-op seams real behavior or an explicit fail-safe: the permissions dialog now actually adds/removes a workspace directory (parity with the ink dialog's settings-scope write), the model picker's arena hand-off and the
/resume//branchre-key report a notice instead of vanishing when their owner seam is unwired, and extension-update consent routes through the always-denying bridge so a pending request can't wedge the slash gateway. OpenTuiAppHost.notify/resetTranscriptnow isolate each UI step (runIsolated), so a throwing subscriber can no longer leave a session swap half-committed — a real correctness fix for the re-key path.- All of it stays inside modules nothing mounts yet; the default (ink) renderer path and
slash-dispatch's live loader are unchanged, and each new branch is test-pinned. - The prior bot CHANGES_REQUESTED was on
71686431(the auto-deny hang fix already landed at63aa2272); 0 threads remain unresolved. CI on this head has no failures (three jobs still running); per the channel convention the call is on the review itself.
chiga0
commented
Sep 1, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
Author
There was a problem hiding this comment.
Re-review @ head 0289df07 (round 3 — 2 new commits)\n\n\nTwo fix commits since round 2 (63aa2272): d8c17a4d + 0289df07.\n\nAll R1 findings resolved — no blocking finding remains.\n\n- R1-1 (confirmUpdateExtensionRequest slot) → ✅ Routes through presentAction auto-deny bridge; .catch(()=>false) handles rejected bridge. Tested.\n- R1-2 (startNewSession no-op) → ✅ New onStartNewSession prop; reports when absent. useMemo deps updated. runIsolated on resetTranscript/listeners aligns with session-switch commit-point contract (prevents spurious core rollback past the commit point). Tested.\n- R1-3 (three no-op callbacks) → ✅ onAddDirectory/onRemoveDirectory wired; onSelectSetting and arena onFillInput both report when absent. Tested.\n- R1-4 (name-only stubs, wiring untested) → ✅ mocks.stub() records props; dialogProp() fails fast on missing prop. Wiring tests call through recorded callbacks and assert helpers.\n\nCross-check: qwen-code-ci-bot CHANGES_REQUESTED (bridge hang) and yiliang114 P2 — both fixed at 63aa2272, confirmed at current head. No outstanding finding from any reviewer.\n\nScope: 6 files, 313+/52−. Static: session-switch commit-point logic, ConfirmationRequest chain, settings dedup — all traced. Rung 2 not run (no environmental behaviour changed).\n\nNo blocking findings. No approvalBlockers. Ready to merge — approval requires a non-author reviewer.\n\n_Reviewed with AI assistance._
yiliang114
approved these changes
Sep 1, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Re-review after the new push (d8c17a4 + 0289df0): the follow-up hardening is correct.
- Extension consent no longer parks a request no renderer can answer:
addConfirmUpdateExtensionRequestroutes through the auto-deny bridge and always settlesonConfirm, so the slash gateway cannot wedge. runIsolatedshields the caller-owned steps (subscribers, onChange, transcript reset) inside the /resume//branch commit window — a broken callback can no longer roll core back or delete the fork just shown.- Session re-key is now an explicit
onStartNewSessionseam that reports when unwired instead of silently keying the new transcript to the old session; permissions-dialog directory edits persist to settings with correct scope handling; the settings sub-dialog and arena-composer gaps notify instead of vanishing.
Verified locally on 0289df0 (after a clean npm ci): app-shell + host + dialog-mount tests 42/42. Earlier verification (50/50 across all six modules, P2 auto-deny fix) still applies.
qqqys
approved these changes
Sep 1, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Approve @ head 0289df07 — R1 阻塞项独立复核通过 / Independent delta verification
Gate / 门禁: qwen-code-dev-bot APPROVED at this head (13:59Z); CI product lanes all green at 0289df07 (only the review-pr meta lane still running); chiga0 round-3 re-review at this head records all R1 findings resolved; mergeable=true.
Delta verified / 增量核实 (63aa2272…0289df07, commits d8c17a4d + 0289df07, 7 files):
- R1-1 (extension-consent 死槽 → 桥接,我上一轮也独立确认过此阻塞) —
opentui-host.ts的confirmUpdateExtensionRequest存储字段已整体删除;addConfirmUpdateExtensionRequest改为走deps.confirmations.presentAction(prompt),.catch(() => false)兜底后必然回调onConfirm,等待方不可能再挂住(此前 app-shell 无人读该槽位 = 潜在 wedge)。两个新测试钉住:桥批准 →onConfirm(true);桥 reject →onConfirm(false)。 - R1-2 (
startNewSession空操作) — app-shell 新增onStartNewSessionprop 并接入 entry seam;无 owner 时给出「Session state was not re-keyed」提示而非静默。notify()与resetTranscript经runIsolated隔离回调异常,与 session-switch 的 commit-point 契约一致(单个订阅者抛错不再触发 core 回滚/误删 branch fork)。测试钉住。 - R1-3 (三个 no-op 回调) —
onAddDirectory/onRemoveDirectory经新addWorkspaceDirectory/removeWorkspaceDirectory助手真实落盘并 reload;onSelectSetting与 arenaonFillInput在无 owner 时明示而非吞掉。两个目录助手与 inkPermissionsDialog的 add/remove commit 路径逐行对比一致(addDirectory→Workspace 去重持久化;remove 按 User→Workspace 顺序命中即 break)。 - R1-4 (桩只断言名字) —
mocks.stub()记录 props,接线测试改为调用录制回调并断言助手被调。
No Critical at this head. / 未发现阻塞合并的 Critical。
chiga0
enabled auto-merge
September 1, 2026 14:36
qwen-code-dev-bot
pushed a commit
to qqqys/qwen-code
that referenced
this pull request
Sep 1, 2026
…wenLM#10724) * docs(opentui): Record the migration status through the composition root The design doc still described the state of 2026-08-28, with only the infra batch landed. Record the five batches now on main, name the seams the composition root leaves to renderer activation, and list the two items deferred to that batch. * docs(opentui): Record the composition-root contracts and correct stale activation scope The design doc described QWEN_TUI_RENDERER as an existing opt-in and the activation batch as carrying runtime fixes that already shipped in QwenLM#10128. Both drifted from the code while the batches landed, which is the kind of claim a reviewer had to catch on QwenLM#10696. State the contracts the composition-root review settled so the activation batch inherits them instead of rediscovering them. * docs(opentui): Record measured runtime status and what the batch reviews kept finding Three claims in the design doc described instruments and gates that are not in the tree: the session-replay harness (issue QwenLM#10005 is still open, nothing measures flicker today) and plain-Node loadability, which 0.5.8 fails on Node 24 — verified locally, not just reported in review. The recurring finding classes are recorded so the activation batch does not re-earn them.
Collaborator
|
Released in v0.23.0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds the OpenTUI backend composition root — the single place that assembles the command bridge, dialog mount, error boundary, and runtime sidecar into one app shell and wires the composer to the slash dispatcher. It also trims
slash-dispatch.tsto the one function production still consumes (interactive command loading) and drops the superseded dispatch chain that no caller reached.Why it's needed
This is Batch 5 of the ink→OpenTUI migration tracked in #8662. Batches 1–4 shipped the framework-neutral model, foundation modules, live-session/input, and the dialog + command layers as independent pieces — nothing yet assembles them into an app. This batch introduces that assembly point without touching the running product: the default renderer stays ink, OpenTUI remains behind
QWEN_TUI_RENDERER, and this shell is intentionally not mounted by the entry (that is Batch 6, renderer activation). Keeping it a thin composition root — assembling the existing parts and exposing the transcript view and model turn as explicit seams — avoids forkingAppContainer's state, which the migration design deliberately rejects (the OpenTUI-native state machinery already landed with the Live-session batch).Reviewer Test Plan
How to verify
This is additive backend glue that is not user-reachable yet (nothing mounts the shell), so there is no interactive Before/After. Confirm the wiring by behavior, exercised in unit tests:
open_dialogswaps the composer for the dialog mount (and closes back),submit_promptand a non-slash prompt reach the model-turn seam,quitreaches the entry, and pasted image paths reach the seam structured so the entry layer builds the real image parts.false) so no command can hang waiting for a renderer this shell does not own.onChange, transcript reset) cannot unwind a/resumeor/branchswap, and the session re-key reaches the entry seam or reports that nothing owns it.slash-dispatch.tsleaves the live loader and all its consumers (the dispatcher and composer completion) green.Run, from
packages/cli:npx vitest run src/ui/opentui/(61 files / 977 tests pass).Evidence (Before & After)
N/A — non-user-visible: not yet mounted, and the default (ink) renderer path is unchanged.
Tested on
Environment
Vitest unit tests (jsdom with a mocked
@opentuiruntime), plus package-leveltsc --noEmitandeslint --max-warnings 0. No shipped-CLI build/runtime change; CI exercises the full macOS/Windows/Linux matrix.Risk & Scope
AppContainercopy — the transcript render and model turn are seams to be filled in Batch 6, so this PR alone is deliberately not a runnable UI.Linked Issues
Batch 5 of #8662 (tracking issue) — part of the migration; does not close #8662.
中文说明
本 PR 做了什么
新增 OpenTUI 的后端组合根：把 command bridge、dialog mount、error boundary 和 runtime sidecar 装配成一个 app shell，并把 composer 接到 slash dispatcher。同时把
slash-dispatch.ts收敛到生产仍在用的那一个函数（交互式命令加载），删掉已无人调度的旧分发链。为什么需要
这是 #8662 跟踪的 ink→OpenTUI 迁移的 Batch 5。前四个 batch 已分别交付了框架中立的模型、foundation 模块、live-session/input、以及 dialog+command 各层，但还没有任何东西把它们装配成一个 app。本 batch 引入这个装配点，同时不改动在跑的产品：默认渲染器仍是 ink，OpenTUI 仍藏在
QWEN_TUI_RENDERER之后，且这个 shell 刻意不被入口挂载（那是 Batch 6 渲染器激活）。保持为薄组合根——装配既有部件、把 transcript 渲染与模型轮次作为显式 seam 暴露——避免了 forkAppContainer的状态，这正是迁移设计明确反对的（OpenTUI 自己的状态机器已随 Live-session batch 落地）。如何验证
属于尚不可达的增量后端胶水（没有东西挂载该 shell），因此没有交互式的 Before/After。用单元测试按行为核对：composer 输入经 gateway 分发并逐种 outcome 落地；确认桥自动拒绝以免命令挂起；dispatcher 初始化失败时后续提交带原因被拒而非误发到模型；子树渲染异常被 error boundary 捕获；通知栏预留槽在 transcript 下方渲染、开对话框时隐藏；删除
slash-dispatch.ts死链后，活的加载函数及其消费方全绿。packages/cli下运行npx vitest run src/ui/opentui/（61 文件 / 967 测试通过）。风险与范围
薄组合根不是 1:1 复制 AppContainer，transcript/模型轮次是留给 Batch 6 的 seam，故本 PR 单独看并非可运行 UI（有意如此）。端到端分发与真实会话行为、以及填充通知栏（G-3）、settings 行到子对话的路由与把 composer 交给 arena 选择器（U-9）不在本批范围。无破坏性改动：纯增量，外加删除全仓无生产调用方的、仅测试可达的死导出。