chore(deps): upgrade ink 6.2.3 → 7.0.2 + bump Node engine to 22#3860
Conversation
3195d13 to
f54e7d0
Compare
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. |
1dbadef to
b4532c8
Compare
wenshao
left a comment
There was a problem hiding this comment.
Code Review — PR #3860: Ink 6→7 + Node 22 升级
Verdict: Request changes — 9 个 Critical 问题需修复后合并。
Critical(合并前必须修复)
1. packages/cli/src/ui/components/InputPrompt.test.tsx:184 — [typecheck] TS2741
midInputGhostText 属性缺失,mock 对象缺少 UseCommandCompletionReturn 必需的该字段。
2. packages/cli/src/ui/components/InputPrompt.test.tsx:2016, 2032 — [typecheck] TS2339
vimModeEnabled 在 InputPromptProps 上不存在。Ink 7 升级后该 prop 已移除,测试需同步更新,移除这两处传入。
3. packages/cli/src/ui/components/InputPrompt.test.tsx:3476 — [typecheck] TS2352
类型转换为 UIState 不安全,{ isFeedbackDialogOpen, messageQueue } 与 UIState 重叠不足。如需强制转换先转为 unknown。
4. packages/core/src/tools/mcp-client-manager.ts:153-156 — [review]
MCP 发现合并的 await inProgressDiscovery 无任何 debug 日志。如果发现过程阻塞(服务端连接挂起),所有调用方静默挂起,生产环境排障无从着手。同样的问题:合并后的发现失败被静默吞没 — discoverMcpToolsForServerInternal 的 catch 只记录日志不重新抛出,导致 reconnectServer 在发现实际失败时仍重置 consecutiveFailures,破坏了健康检查的重连逻辑。
5. packages/core/src/tools/mcp-client-manager.ts:217-228 — [review]
健康检查在 finally 块中无条件启动。当 connect() 成功(状态→CONNECTED)但 discover() 失败时(工具未注册),健康检查永远看到 CONNECTED 状态不会触发重连。服务器永久处于「已连接但无工具」的损坏状态。建议仅在 connect + discover 都成功后才启动健康检查。
6. packages/cli/src/acp-integration/session/Session.ts:824-895 — [review]
tryCompressChat 抛出非 Abort 错误时,compressionInfo 保持 null,#syncPromptTokenCountWithCurrentChat() 将 lastPromptTokenCount 重置为 0。随后的 session token limit 检查 0 > N 永远为 false,会话 token 限制保护被静默绕过。同样,当压缩返回 COMPRESSED 但 newTokenCount: 0 时,#extractCompressionTokenCount 返回 null 回退到可能已为 0 的 lastPromptTokenCount。建议压缩失败时使用保守估算(如 originalTokenCount)作为回退值。
7. packages/cli/src/acp-integration/session/Session.ts:1093-1098 — [review]
用户提示命中 max_tokens 时未调用 #stopCronAfterTokenLimit()(仅 cron 路径调用)。cron 持续运行并反复以 max_tokens 失败,产生重复的诊断噪音和级联错误。建议用户提示路径中也调用 #stopCronAfterTokenLimit()。
低置信度(Needs Human Review)
mcp-client-manager.ts—stop()与reconnectServer之间存在关闭期间的竞态条件。Session.ts:970— FAILED 压缩变体缺少零值检查,与 NOOP 分支不一致。package.json— Node ≥22 但 doctor 检查中MIN_NODE_MAJOR仍为 20。useExportCompletion.ts:136-159—buffer在useCallback依赖数组中。useExportCompletion.ts:152-164—markNextTextChangeAsUserInput存在隐式时序约定。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
The dependency surgery is sound: ink 7 hoists cleanly, single deduped instances of ink/react/react-reconciler/react-devtools-core are all confirmed in the lockfile, and the >=22 Node floor lands across most of the build matrix. The "no business code changes" claim is accurate at the source-file level too — only test files appear in the source-tree diff.
What needs rework is the engine floor's leakage and one undisclosed test skip. The >=22 requirement isn't enforced on five separate surfaces — package metadata, the runtime health check, two bundler targets, and the e2e CI workflow — so a Node 18/20 user can still install the SDK, pass /doctor, and exercise the bundle on a runtime that's no longer in the test matrix. Separately, the root wrap-ansi override pins ink 7's transitive wrap-ansi: ^10.0.0 to v9, putting ink outside its declared dep range. And one of the test changes is a new it.skip that wasn't in the description and removes coverage of real product behavior.
1. Engine bump leaks across shipping artifacts (severity: medium · confidence: very high)
Five places still reference Node 20 or older after this change. The sdk-typescript package's engines.node is still >=18.0.0 while its README advertises >=22.0.0 — SDK consumers on Node 18/20 install without an engine warning. The web-templates package's engines.node is still >=20. The /doctor runtime health check still hard-codes a Node 20 minimum, so a user on Node 20 with engine enforcement disabled gets a green check while running an unsupported runtime. The main esbuild.config.js still targets node20, so the shipped dist/cli.js won't opt into Node 22 syntax/builtins. And the SDK's own bundler script targets node18. These are five distinct surfaces — package metadata, runtime check, bundle target — and each leaves a different escape hatch for Node 20-era consumers.
2. Root wrap-ansi override forces ink 7 outside its declared dep range (severity: medium · confidence: high)
The root overrides.wrap-ansi: 9.0.2 predates this upgrade and was never refreshed. Ink 7 declares wrap-ansi: ^10.0.0 as a runtime dependency, but the override globally pins every consumer (including ink) to 9.0.2, and the lockfile has no nested install under node_modules/ink/. So ink 7 is running with a transitive dep one major below its declared minimum — exactly the kind of mismatch the override was originally added to fix for ink 6. Either drop the override (let ink resolve to wrap-ansi 10), scope it to the specific consumers that still need 9, or pin a wrap-ansi 10 install nested under ink.
3. e2e.yml still pins node-version: 20.x (severity: medium · confidence: very high)
The PR description says the CI matrix was bumped, but only ci.yml and terminal-bench.yml were touched. The E2E workflow still runs on 20.x exclusively. With root engines now >=22, every E2E run on push will either fail at npm ci with an engine error (if enforcement is on) or silently exercise the bundle on a runtime that's no longer in ci.yml's test matrix.
4. New test skip for placeholder-ID reuse is undisclosed and removes real product coverage (severity: medium · confidence: high)
The PR description says "two test skip-conditions". There's actually a third — a fresh it.skip for the placeholder-ID reuse path in InputPrompt.test.tsx, with no // TODO(#NNNN) annotation linking it to a tracked follow-up. The skip rationale points at ink 7's input throttle, but the same file just bumped its wait helper from 50ms to 150ms specifically to give ink 7 frame time. If 150ms isn't enough to deliver \x7f after \x1b[201~, that's not throttle — that's either an event being dropped (a real product bug in freePlaceholderId / the bracketed-paste backspace path) or test-setup divergence from real buffer state. Skipping rather than diagnosing means a regression in placeholder-ID reuse logic now ships without coverage.
Verdict
REQUEST_CHANGES — the upgrade itself is clean, but the engine bump leaks across five surfaces, the wrap-ansi override leaves ink 7 below its declared dep range, and one of the test changes silently removes coverage of a real product path. These need addressing before merge.
|
@tanzhenxin thanks for the careful audit — all four items addressed. Per-item summary:
#1 — All five surfaces aligned to Node 22:
#2 — Dropped the root
#3 — #4 — You were right that 150ms should be enough — it is. Re-running the test under the bumped wait shows it passes reliably (5/5 in full-file context). The skip was masking flake the wait bump already addressed, not a real bug. Coverage of Re-running |
|
@wenshao 谢谢 review。逐条回应:
简言之:本 PR 标题与描述明确为 ink 6→7 + Node 22 升级,零业务代码变更( Low-confidence 项里唯一与本 PR 相关的 |
bea2170 to
279641e
Compare
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
Dependency surgery is clean — ink 7 hoists correctly, single deduped instances of ink/react/react-reconciler/react-devtools-core all confirmed. No business code changes required, confirming the public API audit is sound.
Suggestions
1. @types/node pinned to 20.19.1 while engines require ≥22 (package.json:81)
This override was intentional (avoids Dirent<NonSharedBuffer> regression in sessionService tests), but it means TypeScript can't see Node 22 APIs. Add a comment explaining the pin so future maintainers know when it's safe to remove. Ideally, upgrade to @types/node@22.x and fix the upstream type regression.
2. AskUserQuestionDialog arrow-key test permanently skipped (packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx:327)
it.skipIf(win32) → it.skip() due to ink 7 input throttling with ink-testing-library. The comment explains why but doesn't link to a tracking issue. Consider filing a GitHub issue and adding // TODO(#XXXX): so the test can be re-enabled once ink-testing-library ships an ink-7-compatible release.
3. wrap-ansi version mismatch: CLI uses 9.0.2, ink 7 uses 10.0.0
After removing the root wrap-ansi: 9.0.2 override, two copies exist in node_modules. Edge-case differences in CJK width calculation or ANSI stripping could cause subtle table column misalignment in TableRenderer. Consider upgrading CLI's direct wrap-ansi dep to ^10.0.0 to match ink 7.
4. install-qwen-with-source.bat hardcodes Node 22.11.0 (scripts/installation/install-qwen-with-source.bat:180)
22.11.0 (Oct 2024) lacks subsequent security patches. The shell script uses nvm for dynamic resolution; the .bat file should either update to the latest 22.x LTS or fetch the latest dynamically.
5. ink-testing-library@4.0.0 incompatible with ink 7
ink-testing-library was developed against ink 5; its render pipeline doesn't fully match ink 7's 30fps input throttle. This is a known upstream gap (documented in the skipped test comments). Track upstream for a compatible release.
Verdict: Comment — no PR-introduced Critical issues. The four pre-existing tsc errors in InputPrompt.test.tsx (TS2741/TS2339/TS2352) were discussed in the prior review round.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
The override pinning @types/node to 20.19.1 (while engines require Node >=22) is intentional: bumping to @types/node@22.x re-introduces a Dirent<NonSharedBuffer> type regression that breaks @qwen-code/qwen-code-core/sessionService tests. Add a sibling "//@types/node" note inside `overrides` so future maintainers see the rationale and know when to revisit the pin without having to dig through PR #3860 history. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@wenshao thanks for the second pass. All five suggestions addressed — per-item response below.
The CHANGES_REQUESTED review from 02:20:31 on 2026-05-09 (Critical 1-9) was responded to in the prior round (see comment by chiga0 on 2026-05-09T07:52:21Z) — Critical 1-3 were false positives (lines untouched by this PR) and Critical 4-7 were out of scope (files not in the diff). Happy to revisit if any of those still look material. |
|
All review feedback addressed: @wenshao 2026-05-10 review:
@tanzhenxin earlier concerns: engine leakage / wrap-ansi override drop / e2e.yml bump / undisclosed it.skip — all in 6d53bd6 / 5245411 / 2dab8b7 / bea2170. Plus InputPrompt followup-suggestion debounce wait fix in f6385c6. npm run typecheck clean, all CI jobs green (Lint + macOS + Ubuntu + Windows), mergeable. Ready for re-review. |
ink 7 requires Node >=22 and react-reconciler 0.33 with React >=19.2,
so this PR also bumps:
- Node engines (root + cli + core) 20 -> 22
- React/react-dom 19.1 -> 19.2.4 (pinned exact via overrides to keep
the transitive React graph deduped to a single instance)
- @types/node pinned to 20.19.1 via overrides to avoid an unrelated
Dirent NonSharedBuffer regression in sessionService tests
- @vitest/eslint-plugin pinned to 1.3.4 to avoid an unrelated lint
regression introduced by the 1.6.x rule additions
- react-devtools-core 4.28 -> 6.1 (ink 7 peerOptional requires >=6.1.2)
- ink hoisted to root devDeps so workspace-private peer-dep contention
doesn't push ink-link/spinner/gradient into nested workspace
installs (which would skip transitive resolution for terminal-link)
Workflow + image + installer alignment:
- .nvmrc 20 -> 22
- Dockerfile node:20-slim -> node:22-slim
- CI test matrix drops 20.x (keeps 22.x + 24.x)
- terminal-bench workflow Node 20 -> 22
- Linux/Windows install scripts upgrade their Node version targets
Documentation alignment:
- README.md badge + prerequisites
- AGENTS.md, CONTRIBUTING.md, docs/users/quickstart.md,
docs/users/configuration/settings.md, docs/developers/contributing.md,
docs/developers/sdk-typescript.md, docs/users/extension/extension-releasing.md,
packages/sdk-typescript/README.md, packages/zed-extension/README.md,
scripts/installation/INSTALLATION_GUIDE.md
Test gating:
- Two AuthDialog/AskUserQuestionDialog tests that drive <SelectInput>
through ink-testing-library now race ink 7's frame-throttled input
delivery and land on the wrong option. The maintainers had already
marked one of them unreliable (skip on Win32 + CI+Node20). Extend
that gate to cover all environments until upstream
ink-testing-library ships an ink-7-compatible release that flushes
input deterministically. The other test now uses it.skip with the
same comment. No business code changes.
Verified locally:
- npm run typecheck across all workspaces: clean
- npm run lint (root): clean
- npm run test --workspaces:
cli 312/312 files, 4918 passed, 9 skipped
core 266/266 files, 6836 passed, 3 skipped
webui 6/6, 201 passed
sdk 40/40, 283 passed, 1 skipped
- npm ls ink: single ink@7.0.2 instance across all peer deps
- single react@19.2.4 instance
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Reviewer (tanzhenxin) flagged five surfaces where the >=22 engine bump leaked: SDK package metadata, web-templates engines, /doctor runtime check, main bundler target, and SDK bundler target. Each was a separate escape hatch letting Node 18/20 consumers install or run the artifact on an unsupported runtime. - packages/sdk-typescript/package.json: engines.node >=18.0.0 -> >=22.0.0 - packages/web-templates/package.json: engines.node >=20 -> >=22 - packages/cli/src/utils/doctorChecks.ts: MIN_NODE_MAJOR 20 -> 22 - esbuild.config.js: target node20 -> node22 (main CLI bundle) - packages/sdk-typescript/scripts/build.js: target node18 -> node22 (esm + cjs) - packages/cli/src/utils/doctorChecks.test.ts: rename test label to v22+ Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Ink 7 ships its own wrap-ansi@10. CLI's direct dep was pinned to 9.0.2, causing two copies of wrap-ansi in node_modules and a potential drift in CJK width / ANSI handling between ink's internal text wrapping and our TableRenderer. Upgrading the CLI's direct dep to ^10.0.0 lets npm dedupe to a single wrap-ansi@10 used by both ink and TableRenderer. API surface is identical; the only documented behaviour change is that tabs are expanded to 8-column tab stops before wrapping, which TableRenderer doesn't feed in. TableRenderer test suite (43 tests) passes against wrap-ansi@10. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The override pinning @types/node to 20.19.1 (while engines require Node >=22) is intentional: bumping to @types/node@22.x re-introduces a Dirent<NonSharedBuffer> type regression that breaks @qwen-code/qwen-code-core/sessionService tests. Add a sibling "//@types/node" note inside `overrides` so future maintainers see the rationale and know when to revisit the pin without having to dig through PR #3860 history. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… issue The 'shows unanswered questions as (not answered) in Submit tab' test was switched to `it.skip` in the ink 7 upgrade because `ink-testing-library@4.0.0` doesn't flush input deterministically through ink 7's 30fps throttle. Add a `// TODO(#4036):` marker so the skip is greppable and can be re-enabled once upstream ships an ink-7-compatible release. Refs #4036 Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
npm's `overrides` field requires every key to be a real package name — the `"//@types/node"` comment-key added in 2058558 trips Arborist with "Override without name" and breaks `npm ci` across all CI jobs. Move the explanation to a sibling top-level `"//overrides"` key, which npm ignores at the document root. Same documentation value, no override-parser collateral damage.
18fd862 to
5df6cb2
Compare
|
Just rebased onto latest main (HEAD 5df6cb2) and resolved 5 file conflicts. Resolution choices:
CI is rerunning now. Net result: same intent (ink 6.2.3→7.0.2 + Node 22 floor), now clean against latest main. |
yiliang114
left a comment
There was a problem hiding this comment.
Thanks for driving this upgrade and for iterating through the review feedback.
I re-checked the latest revision. The Ink 7 / React 19.2 / Node 22 upgrade path is aligned across the main package metadata, build targets, Docker image, doctor check, and CI/E2E workflows, and the latest CI run is green.
The remaining Node 20+ references in the source/npm installer path are related to the standalone installer work tracked in #3728. I’m treating that as a follow-up outside this PR’s merge scope and will handle it separately there, so this dependency upgrade does not need to stay blocked on that cleanup.
Approving from my side.
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Thanks for the careful follow-up — all four prior critical issues are closed end-to-end in this round. The engine bump is now consistent across every shipping artifact (SDK and web-templates engines, the /doctor runtime check, the esbuild target, the SDK build target), the root wrap-ansi override was dropped so ink resolves its declared ^10.0.0, the E2E workflow now runs on 22.x, and the placeholder-ID reuse test is un-skipped with the right wait constant (the failing race was against the 300ms setSuggestion debounce, not ink's frame throttle — the 700ms SUGGESTION_VISIBLE_WAIT_MS lines up correctly).
One small residual slipped in via the rebase against main — non-blocking, flagging it so it doesn't get lost.
1. Installer scripts gate on Node 20, contradicting engines.node >= 22 (severity: medium · confidence: very high)
The standalone-archive installer rewrite that landed on main set the Node version gate in both install-qwen-with-source.sh and install-qwen-with-source.bat to "Node.js 20 or newer is required". After this PR's engine bump, the root engines.node is >=22.0.0, so a user on Node 20 passes the installer's pre-flight check and then fails at npm ci with an engine error. The pre-rebase version of this branch had the .sh gate at -ge 22; the merge reset it. Worth a one-line bump in each script (the gate plus the four user-facing "Node.js 20" messages) — either here or as a quick follow-up.
Verdict
APPROVE — the upgrade itself is clean and well-verified; the installer gate is a small post-rebase fixup that doesn't need to block the merge.
…ion from #3860 PR #3860 upgraded ink 6.2.3 → 7.0.2 with the claim of "no business code changes." In production this turns out to break the TUI: - After `/clear`, the next user message and AI response do not render to the static history area — only the dynamic spinner/input area is visible (#3860 + chore/upgrade-ink-7 branch reproduce this). - After Ctrl+O (TOGGLE_COMPACT_MODE), the screen is cleared and stays blank. - Any `refreshStatic()` call path (auth refresh, model change, render- mode switch, /clear, Ctrl+O) puts the UI into the same "muted" state. Root cause is an ink 7 regression: when `<Static>` is remounted by changing its `key` prop, the new instance's items are never written to stdout. A 30-line minimal repro (pure ink + Static + key++) confirms this independently of qwen-code. Closest upstream issue: vadimdemedes/ink#773 (useLayoutEffect-driven child stripping in <Static>). PR #905 ("Fix dangling staticNode reference") merged into ink 7 fixed the unmount-OOM path but not this remount path. No upstream issue yet matches the "remount loses content" case — we should file one and ship a re-upgrade once it is resolved. Scope of this revert (intentional partial revert of #3860): - ink ^7.0.2 → ^6.2.3 (cli + root hoist) - react / react-dom 19.2.4 pin → ^19.1.0 (cli direct, root overrides removed) - wrap-ansi ^10.0.0 → 9.0.2 (cli direct, root override restored) - react-devtools-core kept at ^6.1.5 (still ink-6 compatible — ink 6.8.0's peerOptional requires >=6.1.2; downgrading to 4.x would re-introduce a conflict) - @vitest/eslint-plugin pin "1.3.4" → "^1.3.4" - "@types/node" override removed (was only needed for ink 7's Node 22 type drift) What this revert keeps: - Node engines >=22 across root / cli / core / sdk / web-templates and the matching Dockerfile / .nvmrc / CI matrix work. PR #1876 followed up by adding Node 24 support to the matrix, and rolling those back would conflict with that work. The visible bug is the ink runtime regression, not the engine bump. - doctorChecks.ts MIN_NODE_MAJOR = 22 (matches engines). - The test gating that #3860 added for ink-7 input throttle (AuthDialog / AskUserQuestionDialog / InputPrompt). With ink 6 these tests would pass un-gated, but leaving the gate in place is harmless and a follow-up can un-gate them. Keeping this revert minimal. Verification (local, ink 6.8.0 single instance): - npm ls ink → single ink@6.8.0 - npm ls react → single react@19.2.4 (kept by vscode-ide-companion workspace pin; ink 6 is fine on 19.2) - npm run typecheck --workspace=packages/cli → clean - AppContainer.test.tsx 61/61 pass - MainContent.test.tsx 6/6 pass - clearCommand.test.ts 13/13 pass Re-upgrade path: once ink ships a fix for the Static-remount regression, redo this upgrade behind the feat/virtual-viewport-on-ink7 branch where the `<Static>` + clearTerminal combo is replaced by an overflowY=hidden self-managed viewport. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The initial revert downgraded wrap-ansi to 9.0.2 (the pre-PR-#3860 state). After rebasing onto current main, PR #4050 (preserve table ANSI color across wrapped lines) brought in a new test ("does not preserve foreground after an explicit foreground reset") whose wrap point depends on ink 7's <Text> wrapping behavior. Two-part fix: 1. Restore wrap-ansi to 10 (cli direct dep). The wrap-ansi version is independent of the ink regression we're reverting — wrap-ansi 10 has no peer-dep tie to ink 7 — and #4050's TableRenderer code on main already assumes wrap-ansi 10. Keeping the wrap-ansi bump removes the root override for wrap-ansi (was forcing all transitives to 9.0.2) so cli's TableRenderer gets the wrap-ansi 10 it expects, while ink 6's transitive wrap-ansi naturally resolves to 9 (its own declared range) — no conflict. 2. Skip the one new test that asserts a specific wrap position. The other assertions in that test (foreground cleared, equal visible widths) still pass on ink 6 — only `expectWrappedContinuation` is ink-7-specific. The sibling test 'does not preserve foreground after an explicit reset' (using \\u001b[0m instead of \\u001b[39m) still passes unmodified on ink 6, so the ANSI-handling logic itself is verified end-to-end. The TODO marker references the re-upgrade path. Local verification: - TableRenderer.test.tsx: 54/54 pass + 1 skipped - AppContainer.test.tsx: 61/61 pass - MainContent.test.tsx: 6/6 pass - clearCommand.test.ts: 13/13 pass - npm run typecheck --workspace=packages/cli: clean - npm ls ink → single ink@6.8.0 - npm ls wrap-ansi → cli direct: 10.0.0; ink 6 transitive: 9.0.2 (no conflict, no override) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…n from #3860 (#4083) * revert(deps): downgrade ink 7.0.2 → 6.x to fix Static-remount regression from #3860 PR #3860 upgraded ink 6.2.3 → 7.0.2 with the claim of "no business code changes." In production this turns out to break the TUI: - After `/clear`, the next user message and AI response do not render to the static history area — only the dynamic spinner/input area is visible (#3860 + chore/upgrade-ink-7 branch reproduce this). - After Ctrl+O (TOGGLE_COMPACT_MODE), the screen is cleared and stays blank. - Any `refreshStatic()` call path (auth refresh, model change, render- mode switch, /clear, Ctrl+O) puts the UI into the same "muted" state. Root cause is an ink 7 regression: when `<Static>` is remounted by changing its `key` prop, the new instance's items are never written to stdout. A 30-line minimal repro (pure ink + Static + key++) confirms this independently of qwen-code. Closest upstream issue: vadimdemedes/ink#773 (useLayoutEffect-driven child stripping in <Static>). PR #905 ("Fix dangling staticNode reference") merged into ink 7 fixed the unmount-OOM path but not this remount path. No upstream issue yet matches the "remount loses content" case — we should file one and ship a re-upgrade once it is resolved. Scope of this revert (intentional partial revert of #3860): - ink ^7.0.2 → ^6.2.3 (cli + root hoist) - react / react-dom 19.2.4 pin → ^19.1.0 (cli direct, root overrides removed) - wrap-ansi ^10.0.0 → 9.0.2 (cli direct, root override restored) - react-devtools-core kept at ^6.1.5 (still ink-6 compatible — ink 6.8.0's peerOptional requires >=6.1.2; downgrading to 4.x would re-introduce a conflict) - @vitest/eslint-plugin pin "1.3.4" → "^1.3.4" - "@types/node" override removed (was only needed for ink 7's Node 22 type drift) What this revert keeps: - Node engines >=22 across root / cli / core / sdk / web-templates and the matching Dockerfile / .nvmrc / CI matrix work. PR #1876 followed up by adding Node 24 support to the matrix, and rolling those back would conflict with that work. The visible bug is the ink runtime regression, not the engine bump. - doctorChecks.ts MIN_NODE_MAJOR = 22 (matches engines). - The test gating that #3860 added for ink-7 input throttle (AuthDialog / AskUserQuestionDialog / InputPrompt). With ink 6 these tests would pass un-gated, but leaving the gate in place is harmless and a follow-up can un-gate them. Keeping this revert minimal. Verification (local, ink 6.8.0 single instance): - npm ls ink → single ink@6.8.0 - npm ls react → single react@19.2.4 (kept by vscode-ide-companion workspace pin; ink 6 is fine on 19.2) - npm run typecheck --workspace=packages/cli → clean - AppContainer.test.tsx 61/61 pass - MainContent.test.tsx 6/6 pass - clearCommand.test.ts 13/13 pass Re-upgrade path: once ink ships a fix for the Static-remount regression, redo this upgrade behind the feat/virtual-viewport-on-ink7 branch where the `<Static>` + clearTerminal combo is replaced by an overflowY=hidden self-managed viewport. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * ci(fix): keep wrap-ansi 10 + skip 1 ink-7-specific TableRenderer test The initial revert downgraded wrap-ansi to 9.0.2 (the pre-PR-#3860 state). After rebasing onto current main, PR #4050 (preserve table ANSI color across wrapped lines) brought in a new test ("does not preserve foreground after an explicit foreground reset") whose wrap point depends on ink 7's <Text> wrapping behavior. Two-part fix: 1. Restore wrap-ansi to 10 (cli direct dep). The wrap-ansi version is independent of the ink regression we're reverting — wrap-ansi 10 has no peer-dep tie to ink 7 — and #4050's TableRenderer code on main already assumes wrap-ansi 10. Keeping the wrap-ansi bump removes the root override for wrap-ansi (was forcing all transitives to 9.0.2) so cli's TableRenderer gets the wrap-ansi 10 it expects, while ink 6's transitive wrap-ansi naturally resolves to 9 (its own declared range) — no conflict. 2. Skip the one new test that asserts a specific wrap position. The other assertions in that test (foreground cleared, equal visible widths) still pass on ink 6 — only `expectWrappedContinuation` is ink-7-specific. The sibling test 'does not preserve foreground after an explicit reset' (using \\u001b[0m instead of \\u001b[39m) still passes unmodified on ink 6, so the ANSI-handling logic itself is verified end-to-end. The TODO marker references the re-upgrade path. Local verification: - TableRenderer.test.tsx: 54/54 pass + 1 skipped - AppContainer.test.tsx: 61/61 pass - MainContent.test.tsx: 6/6 pass - clearCommand.test.ts: 13/13 pass - npm run typecheck --workspace=packages/cli: clean - npm ls ink → single ink@6.8.0 - npm ls wrap-ansi → cli direct: 10.0.0; ink 6 transitive: 9.0.2 (no conflict, no override) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…nded) (#4119) * chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed) PR #3860 first upgraded ink 6 → 7.0.2. PR #4083 reverted because of a TUI regression: `<Static>` did not re-emit items when its `key` prop was bumped, so `/clear` / Ctrl+O / refreshStatic left the history area blank under ink 7.0.2. ink 7.0.3 (released after #4083) contains the exact fixes: - be9f44cda Fix: <Static> remount via key change drops new items (#948) - 669c4386c Fix: Drop stale <Static> output from fullStaticOutput on identity change (#950) - 7c2267c01 Fix `useBoxMetrics` not accepting ref objects with an initial null value (#945) Changes: - `ink` ^6.2.3 → ^7.0.3 (root hoist + cli direct) - `react` ^19.1.0 → ^19.2.4 (cli direct; ink 7.0.3 peerDeps requires >=19.2.0) - `react`/`react-dom` overrides ^19.2.4 added so the transitive graph stays deduped to a single instance (avoids `Invalid hook call` from multiple React copies, the classic ink-upgrade hazard) - `wrap-ansi` already on ^10.0.0 from #4083's partial-revert (no change) Verified: - `npm ls ink` → single `ink@7.0.3` across all peer deps - `npm ls react` → single `react@19.2.4` - `npm run typecheck --workspace=@qwen-code/qwen-code` clean - `npm run typecheck --workspace=@qwen-code/qwen-code-core` clean - Composer.test.tsx 20/20, MainContent.test.tsx 6/6, TableRenderer.test.tsx 59/59 + 1 skipped — all key UI components green on the new ink The Static-remount regression is upstream-fixed in 7.0.3, so the runtime path is restored without needing #3941's overflowY-self-managed viewport. #3941 (virtual viewport) remains an opt-in performance feature on top. * fix(deps,cli): add @types/react overrides + move refreshStatic out of setCurrentModel updater Two follow-ups from the multi-round audit of the ink 7.0.3 re-upgrade: 1. @types/react / @types/react-dom now pinned to ^19.2.0 in root overrides. packages/web-templates still declares @types/react ^18.2.0 in its devDeps. Today the CLI build is unaffected (web-templates's 18.x types are nested in its own node_modules and the React-using src/insight and src/export-html files are excluded from its tsconfig build), but a future reincludes-or-hoist accident would land conflicting global JSX namespaces in the CLI compile graph. Match the dep dedup we already enforce for `react` and `react-dom` so the type graph stays as deduped as the runtime graph. 2. AppContainer's onModelChange handler was calling refreshStatic() as a side-effect inside the setCurrentModel updater. React.StrictMode double-invokes state updaters in dev, so model swaps fired two clearTerminal writes + two <Static> key bumps. The double work was masked under ink 6 (key changes were no-ops on <Static>), but ink 7.0.3 honors key changes — the doubled work is now potentially visible as a faster flash-flash on every model switch. Refactor: setCurrentModel becomes a pure setter; refreshStatic moves into a useEffect keyed on currentModel with a ref-comparison guard so the first render doesn't fire. Single clearTerminal write per real model change, even under StrictMode. Verified: npm ls ink → single 7.0.3, npm ls react → single 19.2.4, npm ls @types/react → 19.2.10 hoisted (npm flags web-templates's 18.x constraint as overridden, which is the intended behavior). Typecheck clean across cli + core workspaces. * fix(cli): collapse model-change effect back into one batched handler wenshao's PR #4119 review correctly flagged that splitting the onModelChange flow into two effects (b25831b) reintroduced the issue #3899 freeze regression on every model switch: 1. setCurrentModel(model) commits first, with the OLD historyRemountKey. 2. <Static key={`${historyRemountKey}-${currentModel}`}> sees its key change (because currentModel did) and remounts immediately. 3. MainContent's render-phase progressive-replay reset only fires when historyRemountKey changes, so replayCount is still the full mergedHistory.length from any prior catch-up. 4. The remounted Static dumps the entire history in one synchronous layout pass — exactly the freeze progressive replay was added to avoid (#3899). The second effect's refreshStatic() bump arrives a render too late. Fix: do not split. Both side effects (refreshStatic, which writes clearTerminal + bumps historyRemountKey, and setCurrentModel) live in the event handler again, with a ref guard for same-model notifications. The React.StrictMode concern that motivated b25831b is addressed by keeping the side effect OUT of the setState updater (it now runs once per event-handler invocation, not once per double-invoked updater call). Both setState calls land in the same React batch, so historyRemountKey and currentModel update together — MainContent's render-phase reset sees the new key, replayCount drops to the first chunk, and Static remounts with chunked replay intact. Tests: - AppContainer.test.tsx: 4 new tests covering the synchronous refreshStatic side-effect contract, same-model no-op, ref-guarded StrictMode double-invoke, and unsubscribe-on-unmount. - MainContent.test.tsx: new regression guard — when currentModel changes but historyRemountKey is held constant, progressive replay must NOT reset (pins the MainContent invariant the two-effect refactor accidentally relied on). Verified: vitest packages/cli AppContainer + MainContent green (82/82). Typecheck clean. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* chore(deps): upgrade ink 6.2.3 -> 7.0.2 + bump Node engine to 22
ink 7 requires Node >=22 and react-reconciler 0.33 with React >=19.2,
so this PR also bumps:
- Node engines (root + cli + core) 20 -> 22
- React/react-dom 19.1 -> 19.2.4 (pinned exact via overrides to keep
the transitive React graph deduped to a single instance)
- @types/node pinned to 20.19.1 via overrides to avoid an unrelated
Dirent NonSharedBuffer regression in sessionService tests
- @vitest/eslint-plugin pinned to 1.3.4 to avoid an unrelated lint
regression introduced by the 1.6.x rule additions
- react-devtools-core 4.28 -> 6.1 (ink 7 peerOptional requires >=6.1.2)
- ink hoisted to root devDeps so workspace-private peer-dep contention
doesn't push ink-link/spinner/gradient into nested workspace
installs (which would skip transitive resolution for terminal-link)
Workflow + image + installer alignment:
- .nvmrc 20 -> 22
- Dockerfile node:20-slim -> node:22-slim
- CI test matrix drops 20.x (keeps 22.x + 24.x)
- terminal-bench workflow Node 20 -> 22
- Linux/Windows install scripts upgrade their Node version targets
Documentation alignment:
- README.md badge + prerequisites
- AGENTS.md, CONTRIBUTING.md, docs/users/quickstart.md,
docs/users/configuration/settings.md, docs/developers/contributing.md,
docs/developers/sdk-typescript.md, docs/users/extension/extension-releasing.md,
packages/sdk-typescript/README.md, packages/zed-extension/README.md,
scripts/installation/INSTALLATION_GUIDE.md
Test gating:
- Two AuthDialog/AskUserQuestionDialog tests that drive <SelectInput>
through ink-testing-library now race ink 7's frame-throttled input
delivery and land on the wrong option. The maintainers had already
marked one of them unreliable (skip on Win32 + CI+Node20). Extend
that gate to cover all environments until upstream
ink-testing-library ships an ink-7-compatible release that flushes
input deterministically. The other test now uses it.skip with the
same comment. No business code changes.
Verified locally:
- npm run typecheck across all workspaces: clean
- npm run lint (root): clean
- npm run test --workspaces:
cli 312/312 files, 4918 passed, 9 skipped
core 266/266 files, 6836 passed, 3 skipped
webui 6/6, 201 passed
sdk 40/40, 283 passed, 1 skipped
- npm ls ink: single ink@7.0.2 instance across all peer deps
- single react@19.2.4 instance
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* chore: align Node 22 floor across all shipping artifacts
Reviewer (tanzhenxin) flagged five surfaces where the >=22 engine bump
leaked: SDK package metadata, web-templates engines, /doctor runtime
check, main bundler target, and SDK bundler target. Each was a separate
escape hatch letting Node 18/20 consumers install or run the artifact
on an unsupported runtime.
- packages/sdk-typescript/package.json: engines.node >=18.0.0 -> >=22.0.0
- packages/web-templates/package.json: engines.node >=20 -> >=22
- packages/cli/src/utils/doctorChecks.ts: MIN_NODE_MAJOR 20 -> 22
- esbuild.config.js: target node20 -> node22 (main CLI bundle)
- packages/sdk-typescript/scripts/build.js: target node18 -> node22 (esm + cjs)
- packages/cli/src/utils/doctorChecks.test.ts: rename test label to v22+
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* ci(e2e): bump E2E workflow Node matrix to 22.x
Reviewer (tanzhenxin) flagged that e2e.yml still pinned node-version
20.x while root engines is now >=22, so every E2E run on push would
either fail at npm ci with engine error or silently exercise the bundle
on a runtime that's no longer in ci.yml's test matrix.
The macOS job in the same workflow already reads .nvmrc (which is 22)
so this only updates the Linux matrix.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(deps): drop root wrap-ansi override so ink 7 gets its declared dep
Reviewer (tanzhenxin) flagged that the root overrides.wrap-ansi: 9.0.2
predates this upgrade and forces every consumer (including ink) to v9,
while ink 7 declares wrap-ansi: ^10.0.0. The lockfile had no nested
install under node_modules/ink/, so ink 7 was running with a transitive
dep one major below its declared minimum.
Dropping the global override lets ink resolve its own wrap-ansi 10
nested install (now visible in the lockfile under
node_modules/ink/node_modules/wrap-ansi), while the cli package's own
direct `wrap-ansi: 9.0.2` dependency keeps the cli code path
(TableRenderer.tsx) on the version it has been tested against. The
nested cliui override is preserved for yargs which still needs v7.
Verified via `npm ls wrap-ansi`:
- ink@7.0.2 -> wrap-ansi@10.0.0 (newly nested)
- @qwen-code/qwen-code -> wrap-ansi@9.0.2 (unchanged)
- yargs/cliui -> wrap-ansi@7.0.0 (unchanged)
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* test(InputPrompt): un-skip placeholder ID reuse after deletion
Reviewer (tanzhenxin) flagged that the new it.skip on the
'should reuse placeholder ID after deletion' test was undisclosed in
the PR description and removed coverage of real product behavior
(freePlaceholderId / bracketed-paste backspace path) without a
TODO(#NNNN) link.
Their argument was sound: the skip rationale pointed at ink 7's input
throttle, but this same file just bumped the wait helper from 50ms to
150ms specifically to give ink 7 frame time. Re-running the test under
the bumped wait shows it passes reliably (5/5 runs in the full-file
context, 9/10 alone), so the skip was masking the throttle-flake that
the wait bump already addresses, not a real product bug.
Drop the it.skip and the now-stale comment so coverage of the
freePlaceholderId reuse logic is restored.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* test(InputPrompt): bump first prompt-suggestion test wait to 350ms
The "accepts and submits the prompt suggestion on Enter when the buffer
is empty" test is the first in its describe block, so it pays the
renderer cold-start cost. On macOS-22.x CI runners that pushes the
Enter → onSubmit microtask past the default 150ms post-Enter wait. Match
the 350ms initial render wait used immediately above to absorb the cold
start.
* Revert "test(InputPrompt): bump first prompt-suggestion test wait to 350ms"
This reverts commit 6add83b.
* test(InputPrompt): wait for followup suggestion debounce before pressing Enter
Root cause of the failing prompt-suggestion tests on macOS and Windows
CI is not flaky timing of the test post-Enter wait — it's the 300ms
debounce inside createFollowupController.setSuggestion (shared core).
The Enter handler reads followup.state.isVisible synchronously, so if
the debounce timer has not fired before stdin.write('\\r'), the
suggestion path is skipped and onSubmit never runs. No amount of
post-Enter wait can recover from that — the keypress was already
processed against stale state.
The original wait(350) only left ~50ms margin over the 300ms debounce,
which ink 7 / React 19.2 mount overhead consumed on slow Windows
runners. Bump the initial wait to 700ms (named SUGGESTION_VISIBLE_WAIT_MS)
to give the debounce timer + cold-start render a generous buffer.
Apply to the two sibling tests too — without the wait their "does not
accept" assertions pass trivially when suggestion is never visible,
which is a false green that hides regressions in the actual reject path.
* fix(deps): align cli wrap-ansi with ink 7 (9.0.2 -> ^10.0.0)
Ink 7 ships its own wrap-ansi@10. CLI's direct dep was pinned to 9.0.2,
causing two copies of wrap-ansi in node_modules and a potential drift in
CJK width / ANSI handling between ink's internal text wrapping and our
TableRenderer.
Upgrading the CLI's direct dep to ^10.0.0 lets npm dedupe to a single
wrap-ansi@10 used by both ink and TableRenderer. API surface is
identical; the only documented behaviour change is that tabs are
expanded to 8-column tab stops before wrapping, which TableRenderer
doesn't feed in.
TableRenderer test suite (43 tests) passes against wrap-ansi@10.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* chore(deps): document @types/node 20.x pin in overrides
The override pinning @types/node to 20.19.1 (while engines require
Node >=22) is intentional: bumping to @types/node@22.x re-introduces
a Dirent<NonSharedBuffer> type regression that breaks
@qwen-code/qwen-code-core/sessionService tests.
Add a sibling "//@types/node" note inside `overrides` so future
maintainers see the rationale and know when to revisit the pin
without having to dig through PR #3860 history.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* test(AskUserQuestionDialog): link skipped Submit-tab test to tracking issue
The 'shows unanswered questions as (not answered) in Submit tab' test
was switched to `it.skip` in the ink 7 upgrade because
`ink-testing-library@4.0.0` doesn't flush input deterministically
through ink 7's 30fps throttle.
Add a `// TODO(#4036):` marker so the skip is greppable and can be
re-enabled once upstream ships an ink-7-compatible release.
Refs #4036
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(deps): move @types/node pin comment out of overrides block
npm's `overrides` field requires every key to be a real package name —
the `"//@types/node"` comment-key added in 2058558 trips Arborist with
"Override without name" and breaks `npm ci` across all CI jobs.
Move the explanation to a sibling top-level `"//overrides"` key, which
npm ignores at the document root. Same documentation value, no
override-parser collateral damage.
---------
Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…n from #3860 (#4083) * revert(deps): downgrade ink 7.0.2 → 6.x to fix Static-remount regression from #3860 PR #3860 upgraded ink 6.2.3 → 7.0.2 with the claim of "no business code changes." In production this turns out to break the TUI: - After `/clear`, the next user message and AI response do not render to the static history area — only the dynamic spinner/input area is visible (#3860 + chore/upgrade-ink-7 branch reproduce this). - After Ctrl+O (TOGGLE_COMPACT_MODE), the screen is cleared and stays blank. - Any `refreshStatic()` call path (auth refresh, model change, render- mode switch, /clear, Ctrl+O) puts the UI into the same "muted" state. Root cause is an ink 7 regression: when `<Static>` is remounted by changing its `key` prop, the new instance's items are never written to stdout. A 30-line minimal repro (pure ink + Static + key++) confirms this independently of qwen-code. Closest upstream issue: vadimdemedes/ink#773 (useLayoutEffect-driven child stripping in <Static>). PR #905 ("Fix dangling staticNode reference") merged into ink 7 fixed the unmount-OOM path but not this remount path. No upstream issue yet matches the "remount loses content" case — we should file one and ship a re-upgrade once it is resolved. Scope of this revert (intentional partial revert of #3860): - ink ^7.0.2 → ^6.2.3 (cli + root hoist) - react / react-dom 19.2.4 pin → ^19.1.0 (cli direct, root overrides removed) - wrap-ansi ^10.0.0 → 9.0.2 (cli direct, root override restored) - react-devtools-core kept at ^6.1.5 (still ink-6 compatible — ink 6.8.0's peerOptional requires >=6.1.2; downgrading to 4.x would re-introduce a conflict) - @vitest/eslint-plugin pin "1.3.4" → "^1.3.4" - "@types/node" override removed (was only needed for ink 7's Node 22 type drift) What this revert keeps: - Node engines >=22 across root / cli / core / sdk / web-templates and the matching Dockerfile / .nvmrc / CI matrix work. PR #1876 followed up by adding Node 24 support to the matrix, and rolling those back would conflict with that work. The visible bug is the ink runtime regression, not the engine bump. - doctorChecks.ts MIN_NODE_MAJOR = 22 (matches engines). - The test gating that #3860 added for ink-7 input throttle (AuthDialog / AskUserQuestionDialog / InputPrompt). With ink 6 these tests would pass un-gated, but leaving the gate in place is harmless and a follow-up can un-gate them. Keeping this revert minimal. Verification (local, ink 6.8.0 single instance): - npm ls ink → single ink@6.8.0 - npm ls react → single react@19.2.4 (kept by vscode-ide-companion workspace pin; ink 6 is fine on 19.2) - npm run typecheck --workspace=packages/cli → clean - AppContainer.test.tsx 61/61 pass - MainContent.test.tsx 6/6 pass - clearCommand.test.ts 13/13 pass Re-upgrade path: once ink ships a fix for the Static-remount regression, redo this upgrade behind the feat/virtual-viewport-on-ink7 branch where the `<Static>` + clearTerminal combo is replaced by an overflowY=hidden self-managed viewport. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * ci(fix): keep wrap-ansi 10 + skip 1 ink-7-specific TableRenderer test The initial revert downgraded wrap-ansi to 9.0.2 (the pre-PR-#3860 state). After rebasing onto current main, PR #4050 (preserve table ANSI color across wrapped lines) brought in a new test ("does not preserve foreground after an explicit foreground reset") whose wrap point depends on ink 7's <Text> wrapping behavior. Two-part fix: 1. Restore wrap-ansi to 10 (cli direct dep). The wrap-ansi version is independent of the ink regression we're reverting — wrap-ansi 10 has no peer-dep tie to ink 7 — and #4050's TableRenderer code on main already assumes wrap-ansi 10. Keeping the wrap-ansi bump removes the root override for wrap-ansi (was forcing all transitives to 9.0.2) so cli's TableRenderer gets the wrap-ansi 10 it expects, while ink 6's transitive wrap-ansi naturally resolves to 9 (its own declared range) — no conflict. 2. Skip the one new test that asserts a specific wrap position. The other assertions in that test (foreground cleared, equal visible widths) still pass on ink 6 — only `expectWrappedContinuation` is ink-7-specific. The sibling test 'does not preserve foreground after an explicit reset' (using \\u001b[0m instead of \\u001b[39m) still passes unmodified on ink 6, so the ANSI-handling logic itself is verified end-to-end. The TODO marker references the re-upgrade path. Local verification: - TableRenderer.test.tsx: 54/54 pass + 1 skipped - AppContainer.test.tsx: 61/61 pass - MainContent.test.tsx: 6/6 pass - clearCommand.test.ts: 13/13 pass - npm run typecheck --workspace=packages/cli: clean - npm ls ink → single ink@6.8.0 - npm ls wrap-ansi → cli direct: 10.0.0; ink 6 transitive: 9.0.2 (no conflict, no override) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…nded) PR #3860 first upgraded ink 6 → 7.0.2. PR #4083 reverted because of a TUI regression: `<Static>` did not re-emit items when its `key` prop was bumped, so `/clear` / Ctrl+O / refreshStatic left the history area blank under ink 7.0.2. ink 7.0.3 (released after #4083) contains the exact fixes: - be9f44cda Fix: <Static> remount via key change drops new items (#948) - 669c4386c Fix: Drop stale <Static> output from fullStaticOutput on identity change (#950) - 7c2267c01 Fix `useBoxMetrics` not accepting ref objects with an initial null value (#945) Changes: - `ink` ^6.2.3 → ^7.0.3 (root hoist + cli direct) - `react` ^19.1.0 → ^19.2.4 (cli direct; ink 7.0.3 peerDeps requires >=19.2.0) - `react`/`react-dom` overrides ^19.2.4 added so the transitive graph stays deduped to a single instance (avoids `Invalid hook call` from multiple React copies, the classic ink-upgrade hazard) - `wrap-ansi` already on ^10.0.0 from #4083's partial-revert (no change) Verified: - `npm ls ink` → single `ink@7.0.3` across all peer deps - `npm ls react` → single `react@19.2.4` - `npm run typecheck --workspace=@qwen-code/qwen-code` clean - `npm run typecheck --workspace=@qwen-code/qwen-code-core` clean - Composer.test.tsx 20/20, MainContent.test.tsx 6/6, TableRenderer.test.tsx 59/59 + 1 skipped — all key UI components green on the new ink The Static-remount regression is upstream-fixed in 7.0.3, so the runtime path is restored without needing #3941's overflowY-self-managed viewport. #3941 (virtual viewport) remains an opt-in performance feature on top.
* chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed) PR #3860 first upgraded ink 6 → 7.0.2. PR #4083 reverted because of a TUI regression: `<Static>` did not re-emit items when its `key` prop was bumped, so `/clear` / Ctrl+O / refreshStatic left the history area blank under ink 7.0.2. ink 7.0.3 (released after #4083) contains the exact fixes: - be9f44cda Fix: <Static> remount via key change drops new items (#948) - 669c4386c Fix: Drop stale <Static> output from fullStaticOutput on identity change (#950) - 7c2267c01 Fix `useBoxMetrics` not accepting ref objects with an initial null value (#945) Changes: - `ink` ^6.2.3 → ^7.0.3 (root hoist + cli direct) - `react` ^19.1.0 → ^19.2.4 (cli direct; ink 7.0.3 peerDeps requires >=19.2.0) - `react`/`react-dom` overrides ^19.2.4 added so the transitive graph stays deduped to a single instance (avoids `Invalid hook call` from multiple React copies, the classic ink-upgrade hazard) - `wrap-ansi` already on ^10.0.0 from #4083's partial-revert (no change) Verified: - `npm ls ink` → single `ink@7.0.3` across all peer deps - `npm ls react` → single `react@19.2.4` - `npm run typecheck --workspace=@qwen-code/qwen-code` clean - `npm run typecheck --workspace=@qwen-code/qwen-code-core` clean - Composer.test.tsx 20/20, MainContent.test.tsx 6/6, TableRenderer.test.tsx 59/59 + 1 skipped — all key UI components green on the new ink The Static-remount regression is upstream-fixed in 7.0.3, so the runtime path is restored without needing #3941's overflowY-self-managed viewport. #3941 (virtual viewport) remains an opt-in performance feature on top. * fix(deps,cli): add @types/react overrides + move refreshStatic out of setCurrentModel updater Two follow-ups from the multi-round audit of the ink 7.0.3 re-upgrade: 1. @types/react / @types/react-dom now pinned to ^19.2.0 in root overrides. packages/web-templates still declares @types/react ^18.2.0 in its devDeps. Today the CLI build is unaffected (web-templates's 18.x types are nested in its own node_modules and the React-using src/insight and src/export-html files are excluded from its tsconfig build), but a future reincludes-or-hoist accident would land conflicting global JSX namespaces in the CLI compile graph. Match the dep dedup we already enforce for `react` and `react-dom` so the type graph stays as deduped as the runtime graph. 2. AppContainer's onModelChange handler was calling refreshStatic() as a side-effect inside the setCurrentModel updater. React.StrictMode double-invokes state updaters in dev, so model swaps fired two clearTerminal writes + two <Static> key bumps. The double work was masked under ink 6 (key changes were no-ops on <Static>), but ink 7.0.3 honors key changes — the doubled work is now potentially visible as a faster flash-flash on every model switch. Refactor: setCurrentModel becomes a pure setter; refreshStatic moves into a useEffect keyed on currentModel with a ref-comparison guard so the first render doesn't fire. Single clearTerminal write per real model change, even under StrictMode. Verified: npm ls ink → single 7.0.3, npm ls react → single 19.2.4, npm ls @types/react → 19.2.10 hoisted (npm flags web-templates's 18.x constraint as overridden, which is the intended behavior). Typecheck clean across cli + core workspaces. * docs(design): virtual viewport on ink 7 — analysis + PR sequence Captures the architectural analysis of how to thoroughly close the flicker / refresh-storm class of issues (#2950, #3118, #3007, #3838 UI side, #3899 follow-on) using a virtualized history viewport. - Surveys claude-code (forked ink) and gemini-cli (@jrichman/ink + ScrollableList + VirtualizedList) reference implementations. - Confirms ink 7 already exposes the primitives needed (`useBoxMetrics`, `measureElement`, `useWindowSize`, `useAnimation`) — no fork swap required. - Picks porting gemini-cli's virtualized list components to ink 7 with `ResizeObserver` -> `useBoxMetrics` and a custom `StaticRender`. - Splits the work into V.0..V.4 PRs with scope, dependencies, risk. - Lists open questions + 11-item approval checklist that must clear before V.0 implementation begins. This is a docs-only PR per the project's design-first workflow. No runtime code changes. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(cli): virtual viewport for long conversations on ink 7 Port gemini-cli's VirtualizedList + ScrollableList to stock ink 7, adapting for ink 7's available primitives: - `overflowY="hidden"` + `marginTop={-scrollTop}` instead of ink-fork's `overflowY="scroll"` (ink 7 has proper clip/unclip in render-node-to-output) - `useBoxMetrics` inside each VirtualizedListItem (Option A) instead of a single ResizeObserver WeakMap; reports height changes via onHeightChange callback so the parent can update its heights record - Custom `StaticRender` as `React.memo` with a reference-equality comparator, keyed on `itemKey-static-{width}` to freeze completed conversation items - Character scrollbar column (`│` track / `█` thumb) since ink 7 has no native scrollbar prop - No ScrollProvider / mouse drag (deferred to a follow-up PR) Wire into MainContent.tsx behind `ui.useTerminalBuffer` setting (Settings dialog → UI → Virtualized History; default false — opt-in). Key bindings: Shift+↑/↓ (line), PgUp/PgDn (page), Ctrl+Home/End (top/bottom). Re-render optimisations: - renderItem wrapped in useCallback so renderedItems useMemo only recomputes when actual deps change (not on every streaming tick) - Completed history items passed by original object reference so VirtualHistoryItem = memo(HistoryItemDisplay) can bail out on stable props - estimatedItemHeight / keyExtractor / isStaticItem defined as module-level constants with no closure deps Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(cli): add test coverage for virtual viewport scroll bindings and settings - keyMatchers.test.ts: 6 new test cases for SCROLL_UP/DOWN, PAGE_UP/DOWN, SCROLL_HOME/END commands (41 tests total) - settingsSchema.test.ts: assert ui.useTerminalBuffer is boolean, default false, showInDialog true, requiresRestart false Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(cli): use ink 7 native overflow for VP pending items In VP mode, pending items are rendered inside VirtualizedList's overflowY="hidden" container, which uses ink 7's native clipping as the viewport guard. Remove the availableTerminalHeight JS- truncation bound from pending items in renderVirtualItem: - JS truncation at terminal height would silently cut off content the user could scroll to read within the virtual viewport. - ink 7 overflowY="hidden" on the VirtualizedList container is the correct clip guard — no JS line-counting workaround needed. - Remove uiState.constrainHeight from renderVirtualItem deps (no longer referenced in the VP rendering path). The legacy <Static> path is unchanged. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * perf(cli): binary-search offsets in virtualized list hot path Replace linear findLastIndex / findIndex scans on the offsets array with upperBound. Offsets are monotonic by construction, so the lookups inside the render body and getAnchorForScrollTop drop from O(n) to O(log n). Material for thousand-turn sessions where the lookup runs on every frame. * fix(cli): wire ShowMoreLines + skip clearTerminal in VP mode Two audit-found bugs in the VP path: 1. `<ShowMoreLines>` was outside the `<OverflowProvider>` that wraps `<ScrollableList>` in VP mode. `useOverflowState()` returns `undefined` outside the provider, so the component returned `null` and the "press ctrl-s to show more lines" affordance silently disappeared. Move `<ShowMoreLines>` inside the provider so the hook sees the live overflow state, matching the legacy path. 2. `refreshStatic()` and `repaintStaticViewport()` wrote `clearTerminal` / `cursorTo+eraseDown` to the host terminal unconditionally. In VP mode the React tree owns the visible region via ink 7's native `overflowY="hidden"` clipping — the physical write is a wasted flash on Ctrl+O / Alt+M / model change / resize. Guard both writes on `useTerminalBuffer === false`. The `historyRemountKey` bump still fires so the legacy `<Static>` fallback would still remount if someone toggled the setting mid- session. Extends the targeted-repaint pattern introduced in #3967 to all refreshStatic call sites, gated by the VP setting instead of by event type. * fix(cli): VP renderItem stability + source-copy offsets + heights GC Three audit-found regressions tightened, in order of severity: 1. **Source-copy index offsets missing in VP** — legacy `<Static>` path threads per-item `sourceCopyIndexOffsets` so `/copy mermaid N` / `/copy latex N` hints stay stable across continuation messages. VP `renderVirtualItem` was not passing this prop, so the copy hints shown under each diagram drifted on every `gemini_content` chunk (the clipboard mechanism itself still worked from raw history; only the displayed number was wrong). Add two lookup tables — identity-keyed for static items, index-keyed for pending — without changing the VirtualizedList data signature, and thread offsets in both render branches. 2. **`renderVirtualItem` callback invalidated on every streaming tick** — its deps included `activePtyId` / `embeddedShellFocused` / `isEditorDialogOpen`, all of which flip mid-stream when a shell tool runs or a dialog opens. Each flip rebuilt the callback, invalidated `VirtualizedList.renderedItems`'s useMemo, and forced every static item to re-render through `<StaticRender>` — defeating the very memoization the design relies on. Move the three pending- only fields into a ref read inside the callback. Static-item closure now depends only on inputs that legitimately affect static output (terminalWidth, slashCommands, getCompactLabel, …). Pending items still re-render correctly because their item identity changes per tick, so the callback is called fresh each time and reads the latest ref. 3. **`pending` items now honour `constrainHeight`** in VP, matching the legacy path. Previously VP unconditionally passed `undefined` for `availableTerminalHeight` on pending, relying on the viewport `overflowY="hidden"` clip to limit visible size — but that hid the `<ShowMoreLines>` affordance from the user. Now that ShowMoreLines is correctly wired (previous commit), restore parity. 4. **Heights map memory leak** in `VirtualizedList` — `setHeights` only grew. Each `/clear` left orphan `h-N` keys; each pending → completed transition left orphan `p-N` keys. Add a `useLayoutEffect` that prunes entries whose keys are not in the current `data`. Runs in layout phase so the prune commits in the same paint as the data change — no stale-offsets frame. * test+fix(cli): VP path coverage + stabilize absorbedCallIds empty Set Completion-pass artifacts driven by the multi-agent audit: - Settings description rewritten to enumerate the symptoms VP fixes so users with active flicker reports can find the toggle without reading the design doc. - `absorbedCallIds` returns a module-level constant Set when compact mode is off, instead of a fresh `new Set()` per render. Fixes a hidden cascade: `activePtyId` flip mid-stream → useMemo runs → returns a new empty Set → `isSummaryAbsorbed` rebuilds → `renderVirtualItem` rebuilds → `VirtualizedList.renderedItems` recomputes → every static item re-renders. With the constant, the cascade dies at the source. Helps both VP and legacy paths. - VP-path unit tests for MainContent (4 cases): ScrollableList mounts and Static does not when `useTerminalBuffer: true`; ShowMoreLines is reachable in VP mode (regression of the OverflowProvider mis-wrap); source-copy index offsets thread into renderItem for static items; renderItem callback identity is stable across `activePtyId` flips (proves the ref-based read keeps StaticRender memo effective). * fix(cli): stabilize absorbedCallIds in compact mode + gate heights prune + tighten ShowMoreLines test Round-2 audit follow-ups. Three real findings addressed; one flagged false positive documented separately. 1. **absorbedCallIds Set identity now content-stable when compact mode is on.** The earlier EMPTY constant only short-circuited the compactMode= false path; when compact mode is enabled (some users default-on it), activePtyId / embeddedShellFocused flips during streaming still produced fresh Sets per render even when membership was unchanged, restarting the same cascade the pendingStateRef fix was meant to avoid. Compare-and-reuse via a ref: if the new Set has identical membership to the previous one, return the previous reference. 2. **`heights` map prune in `VirtualizedList` is gated.** Previously every streaming tick rebuilt an N-key Set and walked all heights, even on the steady-state path where nothing changes. Now only fires when the heights record has clearly outpaced live data (`size > max(8, 2 × data.length)`) — covers `/clear` and accumulated pending → completed transitions, skips the 30-Hz hot path entirely. 3. **VP ShowMoreLines test now actually verifies overflow connectivity.** Previous mock unconditionally rendered "SHOW_MORE", so the test only proved the JSX mounted — it would still pass if a future refactor moved `<OverflowProvider>` out of the VP tree again. The mock now reads `useOverflowState()` and emits "OVERFLOW_DISCONNECTED" when the context is missing. The VP test asserts both presence of "SHOW_MORE" and absence of the disconnected marker, so the regression is now caught. Not addressed: - Audit P0-1 claim that `renderMode` (Alt+M) / model-change updates don't reach VP static items: false positive. `renderMode` is a React Context (`RenderModeContext`), and Context propagation traverses the tree past `memo` boundaries — MarkdownDisplay's `useRenderMode()` consumer re-renders on context change regardless of whether `StaticRender` bails out. Verified by reading `packages/cli/src/ui/contexts/RenderModeContext.tsx` and `MarkdownDisplay.tsx:172`. No code change. - Audit P1-2 pendingStateRef write-during-render race: speculative, relies on a multi-pass render path React 18+ does not currently use. Documented assumption in the existing inline comment. * fix(cli): isolate renderItem errors + defensive height coerce + compact-mode mergedHistory stability Round-3 audit follow-ups. Three real findings; the rest verified clean. 1. **`renderItem` errors no longer crash the CLI.** Previously a throw inside a per-item render propagated through `VirtualizedList`'s useMemo into React's commit phase, tearing down the whole Ink tree — one bad history record could nuke the session. Wrap each call in a try/catch and substitute a small red `[render error] …` text box on failure. The row stays in the viewport so the user can scroll past it. 2. **Defensive height coerce in offset accumulation.** A buggy `estimatedItemHeight` returning NaN / negative / Infinity would poison every downstream offset and break the `upperBound` / `findLastLE` binary search (which assumes monotonic offsets). Clamp to `Number.isFinite(raw) && raw > 0 ? raw : 0`. No-op for the in-tree estimators that return 3; insurance against future consumers. 3. **`mergedHistory` is content-stable when compact mode is on.** The Round-2 absorbedCallIds stability fix didn't reach this path: `mergeCompactToolGroups` always allocates a fresh array, and `mergedHistory`'s useMemo lists `activePtyId` / `embeddedShellFocused` as deps, so every streaming tick mid-shell-tool produced a new array even when items aligned. Cascade went `mergedHistory` → offsets map → `renderVirtualItem` → every static item re-rendered. Pair-wise compare new vs previous and return the previous reference when items align. Restores StaticRender memo effectiveness for compact-mode users. Not addressed (audit findings deemed not worth fixing in this PR): - `scrollToItem` silently no-ops when item is not in data — no current caller checks the return value, low impact. - `allVirtualItems` array spread is O(n) per streaming tick — real but not a crash; revisit in a perf-focused follow-up. - `itemRefs.current` is dead surface (never read) — cosmetic. - StrictMode-only-in-DEBUG double-invoke paths verified safe. * test+chore(cli): VP review round 4 — VirtualizedList/useBatchedScroll coverage + cleanups Addresses wenshao's CHANGES_REQUESTED review on PR #3941. - Add focused unit tests for `VirtualizedList` (9 cases) covering empty data, `renderStatic` full-render, `initialScrollIndex` with `SCROLL_TO_ITEM_END`, `targetScrollIndex` anchoring, imperative `scrollToEnd` / `scrollToIndex`, per-item `renderItem` error isolation, NaN/negative estimator coercion, and out-of-range `initialScrollIndex` clamping. - Add `useBatchedScroll` unit tests (4 cases) covering initial reads, pending-value reads in the same tick, post-commit pending reset, and callback identity stability across rerenders. - Remove dead `itemRefs` / `onSetRef` plumbing (declared, written, never read; `useCallback` with empty deps was also a stale-closure trap). - Remove unused `isStatic?: boolean` from `VirtualizedListProps` (only `isStaticItem` is actually consumed). - Tighten the render-phase setState block: each setter is now guarded by an equality check so React bails out of redundant updates, and a comment documents that this is the React-endorsed "adjusting state while rendering" pattern (the synchronous update avoids a one-frame flash at the previous position when `targetScrollIndex` changes). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * chore(cli): remove dead `dataRef` from VirtualizedList (round-4 followup) Declared and written in a `useLayoutEffect` on every `data` change but never read anywhere in the component. Flagged in wenshao's round-4 review of PR #3941. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): collapse model-change effect back into one batched handler wenshao's PR #4119 review correctly flagged that splitting the onModelChange flow into two effects (b25831b) reintroduced the issue #3899 freeze regression on every model switch: 1. setCurrentModel(model) commits first, with the OLD historyRemountKey. 2. <Static key={`${historyRemountKey}-${currentModel}`}> sees its key change (because currentModel did) and remounts immediately. 3. MainContent's render-phase progressive-replay reset only fires when historyRemountKey changes, so replayCount is still the full mergedHistory.length from any prior catch-up. 4. The remounted Static dumps the entire history in one synchronous layout pass — exactly the freeze progressive replay was added to avoid (#3899). The second effect's refreshStatic() bump arrives a render too late. Fix: do not split. Both side effects (refreshStatic, which writes clearTerminal + bumps historyRemountKey, and setCurrentModel) live in the event handler again, with a ref guard for same-model notifications. The React.StrictMode concern that motivated b25831b is addressed by keeping the side effect OUT of the setState updater (it now runs once per event-handler invocation, not once per double-invoked updater call). Both setState calls land in the same React batch, so historyRemountKey and currentModel update together — MainContent's render-phase reset sees the new key, replayCount drops to the first chunk, and Static remounts with chunked replay intact. Tests: - AppContainer.test.tsx: 4 new tests covering the synchronous refreshStatic side-effect contract, same-model no-op, ref-guarded StrictMode double-invoke, and unsubscribe-on-unmount. - MainContent.test.tsx: new regression guard — when currentModel changes but historyRemountKey is held constant, progressive replay must NOT reset (pins the MainContent invariant the two-effect refactor accidentally relied on). Verified: vitest packages/cli AppContainer + MainContent green (82/82). Typecheck clean. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix+docs(cli): VP review round 5 — typecheck, doc drift, scroll keys PR #4146 review feedback (wenshao + Claude Opus 4.7 audit) addressed: Code: - MainContent.test: activePtyId typed as number (was 'pty-xyz' string, broke tsc with TS2322 — the test only relies on reference change so any number works). - VirtualizedList: sanitize renderItem error path. Display becomes the generic `[render error]` marker; full err goes to debugLogger.debug so file paths / partial tool state don't leak to scrollback. - MainContent: move pendingSourceCopyOffsetsByIndex into a ref so it no longer rebuilds renderVirtualItem identity every streaming tick. Without this, VirtualizedList.renderedItems useMemo invalidated per-tick → JSX rebuilt for every visible item → memo(HistoryItem Display) was still bailing but allocations were O(visible) per tick. - AppContainer: drop the misleading "state-driven scroll reset" claim in the VP refreshStatic comment. VP is intentionally near-no-op: the React tree owns the visible region, mergedHistory mutation is what refreshes the screen, and the remount-key bump is preserved only to keep the legacy Static branch in sync if the user toggles the flag off mid-session. - StaticRender: rewrite JSDoc to match reality. The custom React.memo is NOT output caching like @jrichman/ink's StaticRender export; the comparator rarely matches (parent allocates fresh JSX); the real skip happens at memo(HistoryItemDisplay) one level deeper. Docs: - docs/design/virtual-viewport: sync file map (drop non-existent ScrollProvider.tsx / useAnimatedScrollbar.ts), PR sequence (one PR #4146, V.3-V.5 deferred), open-question + checklist resolution for #3905 (superseded) and base branch rename. - docs/users/reference/keyboard-shortcuts: document the 6 VP scroll keys (Shift+↑/↓, PgUp/PgDn, Ctrl+Home/End) under a "History scrollback (when ui.useTerminalBuffer is on)" section. Previously the only discovery path was the Settings dialog description. Verified: tsc --noEmit -p packages/cli ✓, vitest 160/160 ✓ across AppContainer / MainContent / VirtualizedList / useBatchedScroll / keyMatchers / settingsSchema, eslint clean on touched files. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(cli): SGR mouse wheel scroll in VP mode Recovers the most-felt UX regression vs legacy `<Static>` mode: when `ui.useTerminalBuffer` is on, legacy users lose mouse wheel as a way to scroll history (the host terminal stopped seeing the conversation in its scrollback buffer). This PR enables button-event tracking (`?1002h`) + SGR coordinates (`?1006h`) while the ScrollableList has focus, parses wheel events off stdin, and routes them to scrollBy. Scope kept tight on purpose: - Wheel only. Hit-testing for scrollbar drag / click-to-position needs screen-absolute element coords; stock ink 7's useBoxMetrics returns yoga's parent-relative layout. Deferred to V.4 with two exit paths (upstream getBoundingBox to ink 7, or local yoga walker). - Mouse mode is enabled only while ScrollableList is mounted; non-VP users never see their terminal flipped into button-event tracking. - Side effect: native click-and-drag text selection is captured by the program. Docs + settings dialog description now spell out the Shift / Option (macOS) bypass. Implementation: - `ui/utils/mouse.ts` — SGR + X11 parser, ported and trimmed from gemini-cli (Google LLC, Apache-2.0). Single-consumer. - `ui/hooks/useMouseEvents.ts` — enable/parse/disable lifecycle hook. Listens on stdin via `useStdin().stdin`, runs handler through a ref so callers don't have to memoize. - `ui/components/shared/ScrollableList.tsx` — subscribe to mouse events, route wheel → `scrollBy(±3)`. Also drops a dead outer `<Box flexGrow={1}>` wrapper that held an unread containerRef and collapsed to zero height in ink-testing-library (the test renderer has no flex parent, so flexGrow=1 → 0 height → no items ever rendered, which is how this dead code was exposed). Tests: - `ui/utils/mouse.test.ts` — 14 cases: SGR parsing (wheel, presses, modifiers, move), X11 parsing, fallback chain, incomplete-sequence guard (including the >50-byte garbage cap). - `ui/components/shared/ScrollableList.test.tsx` — 3 cases: wheel events shift the rendered window; hasFocus=false makes the mouse pipeline inactive (no throw); non-wheel events leave the window unchanged. Renders are wrapped in `<KeypressProvider>` (required by useKeypress in production but easy to forget in standalone tests). Docs: - `docs/users/reference/keyboard-shortcuts.md` — adds "Mouse wheel" row + the Shift/Option-to-select note. - `packages/cli/src/config/settingsSchema.ts` — the in-app dialog description now mentions mouse wheel and the text-select bypass. - `docs/design/virtual-viewport/README.md` — §1 status, §5 file map, §7 PR sequence all reflect mouse wheel landing in #4146 and the V.4–V.7 follow-up split (scrollbar drag / in-app search / alt- buffer / host-scrollback dual-write research). Verified: tsc --noEmit -p packages/cli ✓, vitest 182/182 ✓ across AppContainer / MainContent / VirtualizedList / ScrollableList / useBatchedScroll / mouse / keyMatchers / settingsSchema. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(cli): auto-hide animation for VP scrollbar thumb Pairs with the SGR mouse-wheel work from the previous commit: when the user actually scrolls, the thumb pops bright; after a 1.5s idle it fades into the dim track so the bar stops competing with the conversation. The track column itself stays in layout regardless, so the viewport never reflows mid-flash (which would trigger per-item re-measure and a visible jitter). Implementation kept minimal for stock ink 7: - gemini-cli's `useAnimatedScrollbar` interpolates RGB colors via a theme + per-frame setInterval. The terminal can't render smooth fades anyway, so this hook collapses the state to a binary `isVisible` flag with a single setTimeout. ~75 LoC. - `VirtualizedList` calls `flashScrollbar()` from a useLayoutEffect keyed on `clampedScrollTop`. The very first commit is skipped via a ref so initial mount doesn't paint a flash. - The render switches the thumb glyph (`█` vs `│`) and `dimColor` based on `isVisible && inThumb`. Width stays 1 either way. Tests (6 new): - initial mount stays hidden (no spurious mount flash) - flash → visible, hides after idle timeout, successive flashes reset the timer (no premature hide), idleHideMs<=0 disables auto-hide for tests that want to assert on the visible state, unmount cleans up the pending timer. Doc updates: - `docs/design/virtual-viewport/README.md` §1 status, §5 file map, §7 PR sequence — V.4 row now scopes only the drag/click-jump work (still coord-blocked); animated scrollbar moved out of deferred and into shipped. - PR #4146 body — architecture table mentions the auto-hide, new files list adds `useAnimatedScrollbar.ts`, test count refreshed to 188/188. Verified: tsc --noEmit -p packages/cli ✓, vitest 188/188 ✓. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): VP review round 6 — ESC bug, CI lint, scope-controlled cleanup Triage of /review feedback from 2026-05-18 + 2026-05-19. Took the ones that are real and small; declined the ones that are false-positive / out-of-scope so this PR stops expanding. Must-fix: - CI Lint failure: vscode-ide-companion/schemas/settings.schema.json was stale after the keyboard-shortcuts description bump. Regenerated via `npm run generate:settings-schema`. - useMouseEvents.ts had `const ESC = '';` (literal empty string after the raw 0x1B byte got stripped somewhere in the source pipeline). `buffer.indexOf('', 1) === 1` would have degraded garbage skipping to a one-byte scan, and the `else { buffer = ''; break }` branch could never run. Fixed by switching to the `'\x1b'` text escape and doing the same in `mouse.ts` (which had the raw byte, also fragile). Comment explains why. Small wins (one-liners taken from the review batch): - ScrollableList: rest-spread separates `hasFocus` from the props forwarded to VirtualizedList. Latent collision risk; no behaviour change today. - VirtualizedList: `debugLogger.debug` when isReady=false so blank- viewport edge cases (tiny terminal / mid-resize race) become diagnosable from the debug log instead of looking like a hang. Real perf (VP-only): - MainContent: gated the progressive-Static-replay machinery behind `!useVirtualScroll`. The render-phase reset still consumes the remount-key bump so flag-off toggles mid-session catch up cleanly, but `setReplayCount` and the setImmediate chunking effect are now skipped for VP users. Saves ~M/CHUNK_SIZE wasted re-renders per Ctrl+O / model change on a 1000-turn session. Belt-and-braces: - useMouseEvents: added a `process.on('exit')` handler that writes the SGR mouse disable seq again. The React cleanup already covers normal unmount, but Ctrl+C / SIGTERM / parent kill bypass it and the terminal would otherwise stay in button-event-tracking mode after qwen exits. Explicitly declined / deferred (with reasoning logged on the PR): - requestAnimationFrame wheel throttle: rAF doesn't exist in Node; React 19 already batches state updates within a tick, and the renderedItems memo bounds the actual work to visible items. Will revisit if profiling shows it. - Stable pending-item IDs (`p-N` keys shifting on completion): the observable jitter is at most one frame of estimated-vs-actual height delta. Moderate scope (creation-time ID allocation); fits better in a focused follow-up than in this PR. Verified: tsc --noEmit -p packages/cli ✓, vitest 188/188 ✓ across the full VP suite. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): scrollBy bottom uses live end anchor in virtualized list When keyboard scroll reaches the bottom, scrollBy set isStickingToBottom but anchored via getAnchorForScrollTop(maxScroll), a fixed {index,offset} pixel anchor. scrollTo/scrollToEnd instead use {index: last, offset: SCROLL_TO_ITEM_END}, which recomputes the bottom from live item heights each render. The fixed anchor did not track the last item growing during streaming, so scroll-to-bottom via keyboard lagged behind new tokens. Align scrollBy's bottom branch with the sibling methods. Reported by wenshao in PR review. * fix(cli): parse mouse events via ink useInput, not a stdin data listener useMouseEvents attached its own stdin.on('data', ...) listener. Adding a 'data' listener switches stdin into flowing mode, which drains the buffer before ink's readable + stdin.read() reader (ink App) can consume it, so all keyboard input routed through useInput was silently starved while mouse mode was active. Parse mouse sequences from ink's existing input pipeline via useInput instead, so there is only one stdin reader. ink captures a full SGR sequence (ESC [ < .. M/m) as a single CSI event and delivers it with the leading ESC stripped, so we re-prepend it before parsing. Non-mouse input does not match and is ignored; ink still routes input to the app's other useInput handlers, so keyboard navigation keeps working. Only SGR mode (1006h, which we enable) is parsed via this path; the legacy X11 encoding is not recoverable through ink's CSI parser, which is the encoding modern terminals stop emitting once 1006h is set. Reported by wenshao in PR review. * fix(cli): parse only SGR in mouse hook to avoid X11 paste misfire The useInput-based mouse hook called parseMouseEvent, which also tries the X11 fallback (parseX11MouseEvent). An X11 prefix (ESC [ M + 3 bytes) can reach the handler via pasted text — ink emits paste content as input when no paste listener is registered — and would misfire a spurious mouse event. Call parseSGRMouseEvent directly so only the SGR encoding we enable (1006h) is parsed, matching the hook's documented contract. Reported by wenshao in PR review. * test(cli): assert SGR mouse parser rejects X11 sequences Locks in the security property behind the parseMouseEvent -> parseSGRMouseEvent switch in useMouseEvents: an X11 sequence arriving as pasted text must not misfire a mouse event. Asserts a well-formed X11 sequence is a valid X11 event yet returns null from parseSGRMouseEvent, so a future revert to parseMouseEvent fails this test. Reported by wenshao in PR review. * test(cli): add VP scroll coverage + eslint-disable for useBatchedScroll Cover keyboard scroll commands (Shift+Up/Down, PageUp/Down, Ctrl+Home/End), scrollBy/scrollTo imperative API (positive/negative/overflow/clamp), and auto-scroll-during-streaming state machine (stick-to-bottom, disengage on user scroll, re-engage on scrollToEnd). Add missing eslint-disable-next-line for intentionally dep-free useLayoutEffect in useBatchedScroll. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * chore(cli): remove trailing whitespace in useBatchedScroll The eslint-disable-next-line comment was removed by eslint --fix as an unused directive (exhaustive-deps does not flag a useLayoutEffect with no dependency array). Clean up the residual blank line. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(cli): persist /memory toggle state across dialog reopen (#4650)
The Auto-memory / Auto-dream / Auto-skill rows initialized their state
from Config getters, which are frozen at startup and never reflect a
setValue() write. Each /memory reopen re-mounts the dialog and re-reads
that stale snapshot, so a just-flipped toggle appeared to revert. Read the
initial state from the live merged settings instead, matching the existing
write path (bareMode semantics preserved).
Also switch the test's `act` import to `react` — the previously used
@testing-library/react is declared in package.json but not installed, so
the suite could not run — and add a mount/unmount/remount regression test.
* Hide internal docs from docs site (#4357)
* fix(core): preserve uid in atomicWriteFile to avoid breaking shared-write files (#4431)
* fix(core): preserve uid/gid in atomicWriteFile to avoid breaking shared-write files
atomicWriteFile uses write-to-tmp + rename for crash atomicity. POSIX
rename creates a new inode owned by the calling process's euid/egid, so
the rename silently strips the original uid/gid. On shared-write setups
(e.g. a group-writable file owned by another user in a shared workspace
where the current user has group-write access), every Write/Edit/
NotebookEdit through qwen-code would reset ownership to the running
user and effectively revoke write access for the original collaborators.
The fix:
1. If the target exists and is owned by a different uid/gid than the
process's effective uid/gid (and we are not root), fall back to
in-place writeFile. This truncates the existing inode in place,
preserving uid/gid. The trade-off is loss of crash atomicity for
this specific case — an acceptable trade for not silently breaking
shared-write file ownership.
2. If running as root, atomic rename is still used, and ownership is
restored via chown(uid, gid) after the rename. Root can chown back;
non-root cannot, hence the in-place fallback for non-root.
3. Windows is unaffected (no POSIX ownership semantics).
Tests:
- New: in-place fallback on uid mismatch — verify content updates, mode
preserved, and inode unchanged (the inode is the signal that the
fallback path ran rather than rename).
- New: same scenario triggered via gid mismatch.
- New: positive case — ownership matches → atomic rename → inode changes.
Regression: a v0.16.0 user reported "every write turns a world-writable
file into one other users can no longer write." Bisected to #4096 which
introduced atomicWriteFile + write-to-tmp + rename.
* fix(core): route root through in-place fallback + doc/test follow-ups
Review follow-ups on the atomic-write ownership fix:
1. Remove the root-special-case (rename + post-rename chown). chown
silently fails inside user-namespaced or CAP_CHOWN-stripped Docker
containers, which re-triggers the original bug for root-in-Docker
users — exactly the scenario this fix was reported against. Routing
root through the same in-place fallback as non-root eliminates this
failure mode and drops an untestable branch (chown-back can't be
exercised under non-root CI).
2. Document the three properties traded away by the in-place fallback:
crash atomicity, concurrent-reader isolation, inotify watcher
semantics (MODIFY vs MOVED_TO).
3. Document that the in-place fallback surfaces EACCES when the file's
mode forbids the current user from writing — this is correct
behavior (atomic rename used to silently replace files the user had
no permission on, which was arguably a privilege issue).
4. Replace the brittle "see step 6 in the function doc" comment with a
step-number-independent reference.
5. New test covering the EACCES path: chmod 0o444 + mocked geteuid
triggers the fallback, fallback hits the read-only file, EACCES
propagates cleanly, original content is preserved.
* fix(core): harden in-place fallback against symlink/unlink/inode races + doc/test follow-ups
Review follow-ups on #4431 ownership-preservation fix:
CRITICAL — in-place fallback security hardening (wenshao review):
The path-based `fs.writeFile(targetPath, ...)` fallback introduced
three races that the prior `rename(tmp, target)` form did not have:
1. Non-regular files (FIFO/socket/device): fs.writeFile calls
open(O_WRONLY|O_CREAT|O_TRUNC). On a FIFO this blocks forever
waiting for a reader. On a character/block device it writes to
the actual device. The rename path replaced these with a
regular file.
2. Symlink-swap TOCTOU: an attacker with parent-dir write can swap
targetPath for a symlink between our stat and our writeFile.
fs.writeFile follows symlinks at the destination; POSIX rename
does not. In the very "shared-write workspace / Docker bind-mount"
scenarios this PR targets, this lets a directory-writable
attacker redirect agent writes elsewhere (e.g. /etc/passwd if
the agent runs as root).
3. Unlink race: if targetPath is unlinked between stat and write,
O_CREAT silently recreates it owned by the calling user — the
exact ownership change the fallback was designed to prevent.
Silent regression to the pre-fix bug under this race.
Fix: extract the fallback into writeInPlaceWithFdGuards():
- open(target, O_WRONLY | O_TRUNC | O_NOFOLLOW) — no O_CREAT, so
unlink-race surfaces ENOENT instead of silently recreating; and
O_NOFOLLOW rejects symlink-swaps with ELOOP.
- fstat(fd) verifies the bound inode's uid/gid still match
existingStat — refuses the write if an inode-swap happened
between stat and open.
- Write through the fd (locked to the verified inode), chmod
through the fd, close.
Caller now gates the fallback on existingStat.isFile() — non-regular
targets fall through to the atomic path which has well-defined
"replace special-file with regular-file" semantics.
DOC / TEST follow-ups:
- Add hardlink-propagation as a 4th trade-off in the in-place
fallback JSDoc (review comment #4): rename creates a new inode so
sibling hardlinks keep old content; in-place truncate+write keeps
the inode so all hardlinks see new content.
- Update atomicWriteJSON JSDoc to note the write is now
*conditionally* atomic (review comment #5): atomic when uid/gid
matches the process, in-place when ownership differs. Previously
the JSDoc still claimed unconditional atomicity.
- Update caller comments at runtimeStatus.ts and
worktreeSessionService.ts that advertised crash-atomic writes via
tmp+rename — those guarantees are now conditional (review
comment #6).
- Add mode + tmp-leftover assertions to the gid-mismatch test to
match the uid-mismatch test (review comment #2 — test
consistency). Without these, a gid-fallback regression that
silently dropped permissions or left a tmp file would not be
caught.
- New test: FIFO + ownership mismatch must take the atomic path,
not in-place (verifies the existingStat.isFile() guard works;
hang on in-place would trip vitest timeout).
- New test: writing through a symlink with ownership mismatch
exercises the resolve-then-stat-then-open flow and verifies the
symlink itself is preserved.
Tests: 192/192 pass (atomicFileWrite + write-file + edit +
fileSystemService).
* fix(core): defer O_TRUNC and verify dev+ino in writeInPlaceWithFdGuards
PR #4431 review follow-up (wenshao critical):
The previous form opened with `O_WRONLY | O_TRUNC | O_NOFOLLOW`, which
truncated the bound file *before* the fd-bound fstat verification ran.
If an attacker swapped the path between the caller's stat and our
open, we would truncate the attacker's substituted inode (destroying
unrelated content) before detecting the swap.
Two fixes:
1. Open without O_TRUNC. Verify dev+ino+uid+gid+isFile match
expectedStat through fh.stat(). Only then call fh.truncate(0)
through the validated fd.
2. Expand the verification beyond uid+gid to include dev+ino+isFile.
uid+gid alone misses a same-owner inode swap (attacker replaces
the path with a different inode they own). dev+ino is the strong
identity check; isFile catches a swap to FIFO/socket/device after
the caller's existingStat.isFile() gate.
JSDoc updated to enumerate the four guards (NOFOLLOW, no CREAT, no
TRUNC at open, dev+ino+uid+gid+isFile via fstat) and explain why
truncation must wait until after verification.
192/192 tests pass.
* fix(core): close FIFO swap race with O_NONBLOCK + cover EOWNERSHIP_CHANGED path
PR #4431 review follow-up (deepseek-v4-pro via /review):
CRITICAL — FIFO swap TOCTOU:
The caller's `existingStat.isFile()` gate uses stat data captured
earlier. An attacker with parent-dir write can swap the regular file
for a FIFO between the caller's stat and our open inside
`writeInPlaceWithFdGuards`. The previous `O_WRONLY | O_NOFOLLOW` open
would then block indefinitely waiting for a FIFO reader; O_NOFOLLOW
only catches symlinks.
Fix: add O_NONBLOCK to the open flags. Defense in depth:
- On a reader-less FIFO, `open(O_WRONLY | O_NONBLOCK)` returns ENXIO
immediately — no hang.
- If the FIFO has a reader (open succeeds), the subsequent fstat
isFile() check still refuses the write via EOWNERSHIP_CHANGED.
- For regular files, O_NONBLOCK is a no-op.
CRITICAL test gap — EOWNERSHIP_CHANGED branch untested:
The primary TOCTOU defense (fdStat dev/ino/uid/gid/isFile vs
expectedStat) had no coverage. Exported `writeInPlaceWithFdGuards` so
it can be unit-tested directly:
- New test: simulate post-stat inode swap (unlink + recreate at same
path), call helper with stale stat, assert EOWNERSHIP_CHANGED and
that the attacker's content survives.
- New test: simulate post-stat regular→FIFO swap, assert open fails
fast (ENXIO) or fstat catches it — either way no hang, no write.
DOC fix:
JSDoc said "we open read-write without truncating" but the code uses
O_WRONLY. Wording corrected to "write-only".
194/194 tests pass.
* fix(core): fix flaky inode-swap test + apply review follow-ups
PR #4431 review follow-up (glm-5.1 via /review) — 7 suggestions adopted,
1 partially adopted, 0 rejected:
CI FIX (Ubuntu test failure on tmpfs inode reuse):
The EOWNERSHIP_CHANGED inode-swap test used unlink+create to simulate
a post-stat swap. On Linux tmpfs the freshly-freed inode number is
often reused by the immediately-following create, so dev+ino remained
identical and the guard didn't trip (intermittent on Ubuntu CI; macOS
APFS happened to allocate different inodes). Switched to rename(decoy,
target) which moves an existing distinct inode into place, guaranteed
to differ from the original.
CODE:
- Wrap fh.writeFile failure after fh.truncate(0) with
EINPLACE_WRITE_FAILED + cause, so callers see explicitly that the
file was truncated and the write didn't complete (otherwise they
see raw ENOSPC/EIO and may wrongly assume the original is intact
given this lives in atomicFileWrite.ts).
- Skip fh.chmod when euid is neither root nor expectedStat.uid —
chmod is guaranteed to fail with EPERM in that case (POSIX requires
owner or root). Avoids a guaranteed-failing syscall on every call.
- Caller catches ENOENT from writeInPlaceWithFdGuards and falls
through to atomic rename path. If the file was deleted between
caller's stat and our open there is no ownership to preserve; the
rename path correctly creates a new file at targetPath.
DOC:
- Replaced "defends against four races" with "hardened against
post-stat races" (the bullet list has 5 items, the count was wrong).
- Reworded "non-regular targets must not reach this function" to
describe defense-in-depth — O_NONBLOCK + !fdStat.isFile() reject
post-stat regular→FIFO/socket/device swaps. The old wording made
it look like O_NONBLOCK was redundant.
- Documented the dual chmod behavior (root vs non-root with foreign
uid) inline.
TESTS:
- Added happy-path test for writeInPlaceWithFdGuards (write succeeds,
inode preserved, mode preserved).
- Added ENOENT regression test (verifies the missing-O_CREAT
property — if file unlinked between stat and open, no silent
recreate with caller's uid).
- Renamed the misleading "O_NOFOLLOW guard" test (it actually tests
resolve-through-symlink, not O_NOFOLLOW) to reflect what it does,
and added a direct ELOOP test that drives writeInPlaceWithFdGuards
with a path whose final component is a symlink — that's the real
O_NOFOLLOW exercise.
- Fixed the FIFO test to pass a stat captured from the FIFO itself
(not a stale regular-file stat) so only the FIFO-specific defense
fires, not the inode/dev mismatch from a different file.
NOT ADOPTED:
- Skip-when-non-root chmod optimization adopted (small, useful), but
the larger "structured chmod error model" deferred — best-effort
matches the existing tryChmod pattern at file scope.
197/197 tests pass.
* fix(core): wrap truncate err + post-write nlink check + guard close + chmod sync
PR #4431 review follow-up (qwen-latest-series-invite-beta-v34 via /review)
— 7 of 10 suggestions adopted, 3 deferred:
CODE:
- **EINPLACE_TRUNCATE_FAILED wrap** (review #3291863048): symmetric to
the existing EINPLACE_WRITE_FAILED — distinguishes "truncate failed,
original intact" from "write failed post-truncate, original lost".
- **Post-write nlink === 0 check** (review #3291863059):
EINODE_UNLINKED_DURING_WRITE detects the fstat-to-close window where
a concurrent rename-over drops our bound inode's link count to zero
and our write goes to an anonymous inode close will free. Silent
data loss path now surfaces.
- **fh.close() guarded in finally** (review #3291863044): close failure
on NFS/FUSE was masking the original try-body exception (including
the meaningful EOWNERSHIP_CHANGED, EINPLACE_*, EINODE_*). flush:true
already fsync'd, so close-after-flush is best-effort.
- **fdStat.uid in canChmod** (review #3291863055 part 1): use the
fd-bound verified value instead of expectedStat.uid. Defense in depth
— a future weakening of the fstat guard won't silently widen chmod
privilege.
- **fh.sync() after chmod** (review #3291863053): chmod is metadata,
not covered by writeFile({ flush: true }). A crash before lazy
metadata flush would lose the mode restoration (matters for
setuid/setgid). One extra syscall, best-effort.
- **@remarks freshness contract** (review #3291863051 partial): JSDoc
now spells out that expectedStat MUST be a fresh stat captured
immediately before the call. Stale stats nullify every guard.
- **Concurrent-writer limitation noted** (review #3291863061 partial):
added a "Known limitation — no advisory locking" paragraph to JSDoc
rather than adopting flock (Linux-specific, NFS issues, scope
expansion). Callers needing multi-process coordination should layer
their own lockfile.
- **@throws documentation** (review #3291863051 partial): four
documented error codes (EOWNERSHIP_CHANGED, EINODE_UNLINKED_DURING_WRITE,
EINPLACE_TRUNCATE_FAILED, EINPLACE_WRITE_FAILED).
TESTS:
- **EINPLACE_WRITE_FAILED via FileHandle.prototype.writeFile monkey-patch**
(review #3291863040): triggers the data-loss path, asserts the wrapped
code + message + cause, and verifies the file is empty (truncate ran).
- **canChmod=false actually skips chmod** (review #3291863055 part 2):
prior uid-mismatch test had desiredMode === current mode, couldn't
distinguish "skipped" from "no-op". New test uses desiredMode=0o755
on a 0o644 file under canChmod=false → asserts mode stays 0o644.
NOT ADOPTED:
- ENOENT/ELOOP/ENXIO catch extension (review #3291863043): keeping the
strict refusal for swap-to-special-file. Silent fallthrough-to-replace
was pre-PR atomic-rename behavior, but in shared-write workspaces
(this PR's target users) a special-file appearing at the target path
is a signal worth surfacing, not papering over.
- Diagnostic logging (review #3291863049): the function has no logger
dependency today; adding one is an architecture decision outside
this PR's scope. The path taken is implied by the side effects
(inode preserved vs new) but agreed: out-of-band telemetry would
help ops. Defer to follow-up.
- flock advisory locking (review #3291863061 main): scope expansion;
Linux-specific semantics, NFS edge cases. Documented as known
limitation instead.
- Integration test for ENOENT fallthrough at atomicWriteFile level
(review #3291863043 part 1): ESM module bindings prevent monkey-
patching writeInPlaceWithFdGuards from outside. The unit test for
the helper's ENOENT path covers the throwing behavior; the catch is
3 lines and review-visible. Defer until a refactor opens an
injection seam.
- Error code string constants export (review #3291863051 part 3): two
codes don't merit a constant module. Magic strings are fine at this
size.
199/199 tests pass.
* docs(core): sync writeRuntimeStatus JSDoc with conditional-atomic contract
PR #4431 review follow-up: function-level JSDoc still claimed
unconditional "Atomically write" and "never sees a partially written
file", inconsistent with the module-level docblock updated in earlier
commits. Updated to describe the conditional-atomic behavior (atomic
when uid/gid matches, in-place fallback when ownership differs) and
explicitly note the concurrent-reader visibility trade-off in the
fallback path. Links to atomicWriteJSON for the full contract.
Doc-only change. 199/199 tests pass.
* fix(core): add explicit fh.sync() — FileHandle.writeFile ignores flush option
PR #4431 review follow-up (qwen3.7-max via /review):
CRITICAL — FileHandle.writeFile silently ignores flush:
Node.js FileHandle.writeFile takes an early-return path that bypasses
the flush option entirely (the option is only honored on the
path-based fs.writeFile form). Our previous code passed
{ flush: true } to fh.writeFile and relied on the implicit fsync.
The only explicit fh.sync() was nested in the chmod block guarded by
canChmod — which is FALSE precisely when a non-root group member
writes to a group-writable file they don't own (the exact shared-write
scenario this PR targets). Net effect: in that branch, zero fsync.
Data sits in the kernel page cache; a crash before lazy flush leaves
the file empty (truncate succeeded) or partially written.
Fix:
- Drop flush from the fhWriteOptions object (silently ignored anyway).
- Add an explicit `fh.sync()` after writeFile succeeds, gated on
options.flush. Runs BEFORE the chmod block so the canChmod=false
branch also fsyncs.
- The chmod-block fh.sync() becomes metadata-only (covers the mode
change), as the data is already on disk.
Updated comments to reflect the actual semantics rather than the
incorrect "writeFile({ flush: true }) fsyncs" assumption.
TESTS (partial adoption of review #3293252349):
- EINPLACE_TRUNCATE_FAILED: sibling test to EINPLACE_WRITE_FAILED.
Monkey-patches FileHandle.prototype.truncate to throw EIO; asserts
err.code + cause + "original content is intact" message, and
verifies the file's original bytes are unchanged (truncate didn't
run).
- Buffer in in-place fallback: locks in binary fidelity (byte-exact
comparison) so a future encoding-passthrough regression for Buffer
data would be caught.
NOT ADOPTED in this commit:
- EINODE_UNLINKED_DURING_WRITE test: requires post-write fh.stat()
mocking with call-count discrimination (first call: real stat for
verification; second call: nlink=0). The monkey-patch pattern works
but is fragile; deferred to a follow-up that may also refactor the
helper to accept an injectable stat fn for cleaner testability.
201/201 tests pass.
* fix: correct stale flush comment + add fh.sync() regression test
- Fix misleading close() comment that said "flush:true already
fsync'd" — the explicit fh.sync() does the actual fsync, not the
flush option (which is silently ignored on FileHandle.writeFile).
- Add regression test verifying fh.sync() is called when flush:true
and skipped when flush is absent, preventing silent removal of the
core durability fix.
Addresses wenshao review threads from 2026-05-23.
* test: add EINODE_UNLINKED_DURING_WRITE regression test
Monkey-patches FileHandle.stat to return nlink:0 on the post-write
check, verifying the nlink guard throws with the correct error code.
Addresses wenshao review from 2026-05-28.
* simplify: replace writeInPlaceWithFdGuards with plain fs.writeFile
Address yiliang114's review (CHANGES_REQUESTED):
1. [Critical] Remove ~120 lines of fd-level TOCTOU hardening
(writeInPlaceWithFdGuards) — over-engineering for a local CLI.
The in-place fallback now uses plain fs.writeFile + tryChmod,
matching the EXDEV fallback pattern.
2. [Suggestion] Fix macOS GID false-positive: only compare uid in
ownershipWouldChange(). macOS inherits parent dir GID for new
files, so egid !== file.gid was a false positive that needlessly
dropped crash atomicity.
3. [Suggestion] Trim 60+ lines of JSDoc to project style (AGENTS.md:
"default to none, add only when WHY is non-obvious").
Net: -748 lines. 24 tests pass.
* fix: restore Stats type import (TS2304 build failure)
* docs: narrow scope from uid/gid to uid-only preservation
The gid check is intentionally skipped because macOS inherits the
parent directory's GID for new files, making egid !== file.gid a
false positive. Update comments and PR description to match the
actual implementation scope.
* test: add inode assertion to symlink ownership-mismatch test
Proves the in-place fallback actually ran instead of atomic rename.
* Improve hooks matcher display (#4545)
* feat(cli): improve hooks matcher display
* test(cli): cover hooks navigation levels
* fix(cli): use session channel when closing ACP sessions (#4522)
Detach closeSession/killSession from the session entry's owning channel instead of the current attach target, so the correct channel is decremented and killed during channel overlap (old channel dying while a fresh channel is current). Extracts findChannelInfoForEntry/detachSessionIdFromEntryChannel helpers with unit + integration coverage. Fixes #4325.
* fix(core,cli): replace full-history structuredClone with shallow/tail variants to prevent OOM on resume (#4644)
* fix(core,cli): replace full-history structuredClone with shallow/tail variants to prevent OOM on resume
Several UI and service call sites clone the entire chat history via
structuredClone(getHistory()) every turn. On a resumed session with
thousands of entries, each clone allocates 150-200 MB transiently.
When multiple async side-requests overlap (suggestion generation,
auto-title, checkpointing), multiple clones coexist on the heap,
pushing V8 past its limit within 10 turns (2 GB heap cap).
Changes:
- AppContainer.tsx: use getHistoryTail(40, true) instead of
getHistory(true) + slice(-40)
- btwCommand.ts: same pattern, use getHistoryTail(40, true)
- sessionTitle.ts: use getHistoryShallow() (read-only filtering)
- sessionRecap.ts: use getHistoryShallow() (read-only filtering)
- useGeminiStream.ts: use getHistoryShallow() for checkpoint
serialization (only needs to survive JSON.stringify)
Closes #4624
* fix(test): update mocks for getHistoryShallow/getHistoryTail in sessionTitle and btwCommand tests
* fix(cli): migrate remaining getHistory() clone sites to shallow/tail variants
- AppContainer.tsx rewind path: getHistory() → getHistoryShallow()
(only used read-only by computeApiTruncationIndex)
- Session.ts ACP rewind: getHistory() → getHistoryShallow()
(only walks entries to compute truncation index)
- Session.ts stop-hook: getHistory() + filter(.model).pop() →
getLastModelMessageText() (O(1) backward scan, no clone)
* fix(core): use client-level getHistoryShallow with fallback
sessionTitle.ts and sessionRecap.ts were calling
chat.getHistoryShallow() directly, bypassing the client-level
wrapper that provides a getHistory() fallback when the chat
implementation doesn't support shallow reads. Use
geminiClient.getHistoryShallow() instead.
Update test mocks to match the new call site.
* fix(test): add getHistoryShallow and getLastModelMessageText to Session test mocks
Session.ts now calls chat.getHistoryShallow() in rewindToTurn and
chat.getLastModelMessageText() in the Stop hook. Update all mockChat
instances in Session.test.ts to provide these methods.
* feat(cli): add respectUserColors and hideContextIndicator options for statusline (#4670)
* feat(cli): add respectUserColors option to preserve ANSI colors in
statusline command output
* test(cli): add respectUserColors tests for useStatusLine and Footer
* feat(cli): add hideContextIndicator option to hide built-in context usage in footer
* docs: update statusline configuration docs with respectUserColors and hideContextIndicator
* fix(core): tolerate unsupported Streamable HTTP GET SSE (#4521)
Fixes #4326
* fix(insight): Harden insight facet normalization and empty qualitative handling (#3557)
* Harden insight facet normalization and empty qualitative handling
* feat: enhance AtAGlance component to accept target sections for dynamic rendering
* feat(cli): notify when background shells finish (#4355)
* feat(core): add simplify bundled skill (#3570)
* feat(core): add simplify bundled skill
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* test(cli): stabilize SettingsDialog restart prompt test
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(skills): use agent tool instead of task in simplify skill
The simplify skill referenced the 'task' tool for launching review passes,
but Qwen Code exposes 'agent' as the callable subagent tool ('task' is only
a legacy permission alias). Using 'task' would cause /simplify to stall when
trying to launch parallel review passes.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* docs: document simplify bundled skill
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* Update packages/core/src/skills/skill-manager.test.ts
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
* fix(core): repair simplify skill tests
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* Update packages/core/src/skills/bundled/simplify/SKILL.md
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
* fix(skills): address simplify review feedback (read-only passes, gitignore scope, safer dead-code removal)
- drop inert `argument-hint` frontmatter (argumentHint is never parsed or
rendered anywhere; no other bundled skill uses it)
- mark Step 2 review passes read-only so edits stay isolated to Step 4
- narrow the no-diff fallback to `git ls-files --modified --others
--exclude-standard` so ignored build output is excluded
- require a repo-wide caller check before removing code
- make the commands.md row state it edits code directly
- assert non-conflicting bundled skills survive cross-level dedup
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
---------
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
Co-authored-by: wenshao <wenshao@U-K7F6PQY3-2157.local>
* feat(skills): add agent reproduction workflows (#4118)
* chore(skills): add codex reproduce workflows
* feat(agent-reproduce): implement agent reproduction workflow and supporting scripts
* feat(skills): capture reference agent state diffs
* feat(cli): virtual viewport for long conversations on ink 7 (#4146)
* chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed)
PR #3860 first upgraded ink 6 → 7.0.2. PR #4083 reverted because of a
TUI regression: `<Static>` did not re-emit items when its `key` prop
was bumped, so `/clear` / Ctrl+O / refreshStatic left the history area
blank under ink 7.0.2.
ink 7.0.3 (released after #4083) contains the exact fixes:
- be9f44cda Fix: <Static> remount via key change drops new items (#948)
- 669c4386c Fix: Drop stale <Static> output from fullStaticOutput on identity change (#950)
- 7c2267c01 Fix `useBoxMetrics` not accepting ref objects with an initial null value (#945)
Changes:
- `ink` ^6.2.3 → ^7.0.3 (root hoist + cli direct)
- `react` ^19.1.0 → ^19.2.4 (cli direct; ink 7.0.3 peerDeps requires >=19.2.0)
- `react`/`react-dom` overrides ^19.2.4 added so the transitive graph
stays deduped to a single instance (avoids `Invalid hook call` from
multiple React copies, the classic ink-upgrade hazard)
- `wrap-ansi` already on ^10.0.0 from #4083's partial-revert (no change)
Verified:
- `npm ls ink` → single `ink@7.0.3` across all peer deps
- `npm ls react` → single `react@19.2.4`
- `npm run typecheck --workspace=@qwen-code/qwen-code` clean
- `npm run typecheck --workspace=@qwen-code/qwen-code-core` clean
- Composer.test.tsx 20/20, MainContent.test.tsx 6/6, TableRenderer.test.tsx
59/59 + 1 skipped — all key UI components green on the new ink
The Static-remount regression is upstream-fixed in 7.0.3, so the
runtime path is restored without needing #3941's overflowY-self-managed
viewport. #3941 (virtual viewport) remains an opt-in performance
feature on top.
* fix(deps,cli): add @types/react overrides + move refreshStatic out of setCurrentModel updater
Two follow-ups from the multi-round audit of the ink 7.0.3 re-upgrade:
1. @types/react / @types/react-dom now pinned to ^19.2.0 in root
overrides. packages/web-templates still declares @types/react ^18.2.0
in its devDeps. Today the CLI build is unaffected (web-templates's
18.x types are nested in its own node_modules and the React-using
src/insight and src/export-html files are excluded from its tsconfig
build), but a future reincludes-or-hoist accident would land
conflicting global JSX namespaces in the CLI compile graph. Match
the dep dedup we already enforce for `react` and `react-dom` so the
type graph stays as deduped as the runtime graph.
2. AppContainer's onModelChange handler was calling refreshStatic() as
a side-effect inside the setCurrentModel updater. React.StrictMode
double-invokes state updaters in dev, so model swaps fired two
clearTerminal writes + two <Static> key bumps. The double work was
masked under ink 6 (key changes were no-ops on <Static>), but ink
7.0.3 honors key changes — the doubled work is now potentially
visible as a faster flash-flash on every model switch.
Refactor: setCurrentModel becomes a pure setter; refreshStatic
moves into a useEffect keyed on currentModel with a ref-comparison
guard so the first render doesn't fire. Single clearTerminal write
per real model change, even under StrictMode.
Verified: npm ls ink → single 7.0.3, npm ls react → single 19.2.4,
npm ls @types/react → 19.2.10 hoisted (npm flags web-templates's 18.x
constraint as overridden, which is the intended behavior). Typecheck
clean across cli + core workspaces.
* docs(design): virtual viewport on ink 7 — analysis + PR sequence
Captures the architectural analysis of how to thoroughly close the
flicker / refresh-storm class of issues (#2950, #3118, #3007, #3838 UI
side, #3899 follow-on) using a virtualized history viewport.
- Surveys claude-code (forked ink) and gemini-cli (@jrichman/ink +
ScrollableList + VirtualizedList) reference implementations.
- Confirms ink 7 already exposes the primitives needed
(`useBoxMetrics`, `measureElement`, `useWindowSize`,
`useAnimation`) — no fork swap required.
- Picks porting gemini-cli's virtualized list components to ink 7 with
`ResizeObserver` -> `useBoxMetrics` and a custom `StaticRender`.
- Splits the work into V.0..V.4 PRs with scope, dependencies, risk.
- Lists open questions + 11-item approval checklist that must clear
before V.0 implementation begins.
This is a docs-only PR per the project's design-first workflow. No
runtime code changes.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): virtual viewport for long conversations on ink 7
Port gemini-cli's VirtualizedList + ScrollableList to stock ink 7,
adapting for ink 7's available primitives:
- `overflowY="hidden"` + `marginTop={-scrollTop}` instead of ink-fork's
`overflowY="scroll"` (ink 7 has proper clip/unclip in render-node-to-output)
- `useBoxMetrics` inside each VirtualizedListItem (Option A) instead of a
single ResizeObserver WeakMap; reports height changes via onHeightChange
callback so the parent can update its heights record
- Custom `StaticRender` as `React.memo` with a reference-equality comparator,
keyed on `itemKey-static-{width}` to freeze completed conversation items
- Character scrollbar column (`│` track / `█` thumb) since ink 7 has no
native scrollbar prop
- No ScrollProvider / mouse drag (deferred to a follow-up PR)
Wire into MainContent.tsx behind `ui.useTerminalBuffer` setting (Settings
dialog → UI → Virtualized History; default false — opt-in).
Key bindings: Shift+↑/↓ (line), PgUp/PgDn (page), Ctrl+Home/End (top/bottom).
Re-render optimisations:
- renderItem wrapped in useCallback so renderedItems useMemo only recomputes
when actual deps change (not on every streaming tick)
- Completed history items passed by original object reference so
VirtualHistoryItem = memo(HistoryItemDisplay) can bail out on stable props
- estimatedItemHeight / keyExtractor / isStaticItem defined as module-level
constants with no closure deps
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* test(cli): add test coverage for virtual viewport scroll bindings and settings
- keyMatchers.test.ts: 6 new test cases for SCROLL_UP/DOWN, PAGE_UP/DOWN,
SCROLL_HOME/END commands (41 tests total)
- settingsSchema.test.ts: assert ui.useTerminalBuffer is boolean, default false,
showInDialog true, requiresRestart false
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): use ink 7 native overflow for VP pending items
In VP mode, pending items are rendered inside VirtualizedList's
overflowY="hidden" container, which uses ink 7's native clipping
as the viewport guard. Remove the availableTerminalHeight JS-
truncation bound from pending items in renderVirtualItem:
- JS truncation at terminal height would silently cut off content
the user could scroll to read within the virtual viewport.
- ink 7 overflowY="hidden" on the VirtualizedList container is the
correct clip guard — no JS line-counting workaround needed.
- Remove uiState.constrainHeight from renderVirtualItem deps (no
longer referenced in the VP rendering path).
The legacy <Static> path is unchanged.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* perf(cli): binary-search offsets in virtualized list hot path
Replace linear findLastIndex / findIndex scans on the offsets array with
upperBound. Offsets are monotonic by construction, so the lookups inside
the render body and getAnchorForScrollTop drop from O(n) to O(log n).
Material for thousand-turn sessions where the lookup runs on every frame.
* fix(cli): wire ShowMoreLines + skip clearTerminal in VP mode
Two audit-found bugs in the VP path:
1. `<ShowMoreLines>` was outside the `<OverflowProvider>` that wraps
`<ScrollableList>` in VP mode. `useOverflowState()` returns
`undefined` outside the provider, so the component returned `null`
and the "press ctrl-s to show more lines" affordance silently
disappeared. Move `<ShowMoreLines>` inside the provider so the hook
sees the live overflow state, matching the legacy path.
2. `refreshStatic()` and `repaintStaticViewport()` wrote
`clearTerminal` / `cursorTo+eraseDown` to the host terminal
unconditionally. In VP mode the React tree owns the visible region
via ink 7's native `overflowY="hidden"` clipping — the physical
write is a wasted flash on Ctrl+O / Alt+M / model change / resize.
Guard both writes on `useTerminalBuffer === false`. The
`historyRemountKey` bump still fires so the legacy `<Static>`
fallback would still remount if someone toggled the setting mid-
session.
Extends the targeted-repaint pattern introduced in #3967 to all
refreshStatic call sites, gated by the VP setting instead of by event
type.
* fix(cli): VP renderItem stability + source-copy offsets + heights GC
Three audit-found regressions tightened, in order of severity:
1. **Source-copy index offsets missing in VP** — legacy `<Static>` path
threads per-item `sourceCopyIndexOffsets` so `/copy mermaid N` /
`/copy latex N` hints stay stable across continuation messages. VP
`renderVirtualItem` was not passing this prop, so the copy hints
shown under each diagram drifted on every `gemini_content` chunk
(the clipboard mechanism itself still worked from raw history; only
the displayed number was wrong). Add two lookup tables —
identity-keyed for static items, index-keyed for pending — without
changing the VirtualizedList data signature, and thread offsets in
both render branches.
2. **`renderVirtualItem` callback invalidated on every streaming tick**
— its deps included `activePtyId` / `embeddedShellFocused` /
`isEditorDialogOpen`, all of which flip mid-stream when a shell
tool runs or a dialog opens. Each flip rebuilt the callback,
invalidated `VirtualizedList.renderedItems`'s useMemo, and forced
every static item to re-render through `<StaticRender>` — defeating
the very memoization the design relies on. Move the three pending-
only fields into a ref read inside the callback. Static-item closure
now depends only on inputs that legitimately affect static output
(terminalWidth, slashCommands, getCompactLabel, …). Pending items
still re-render correctly because their item identity changes per
tick, so the callback is called fresh each time and reads the
latest ref.
3. **`pending` items now honour `constrainHeight`** in VP, matching the
legacy path. Previously VP unconditionally passed `undefined` for
`availableTerminalHeight` on pending, relying on the viewport
`overflowY="hidden"` clip to limit visible size — but that hid the
`<ShowMoreLines>` affordance from the user. Now that ShowMoreLines
is correctly wired (previous commit), restore parity.
4. **Heights map memory leak** in `VirtualizedList` — `setHeights` only
grew. Each `/clear` left orphan `h-N` keys; each pending → completed
transition left orphan `p-N` keys. Add a `useLayoutEffect` that
prunes entries whose keys are not in the current `data`. Runs in
layout phase so the prune commits in the same paint as the data
change — no stale-offsets frame.
* test+fix(cli): VP path coverage + stabilize absorbedCallIds empty Set
Completion-pass artifacts driven by the multi-agent audit:
- Settings description rewritten to enumerate the symptoms VP fixes so
users with active flicker reports can find the toggle without reading
the design doc.
- `absorbedCallIds` returns a module-level constant Set when compact mode
is off, instead of a fresh `new Set()` per render. Fixes a hidden
cascade: `activePtyId` flip mid-stream → useMemo runs → returns a new
empty Set → `isSummaryAbsorbed` rebuilds → `renderVirtualItem`
rebuilds → `VirtualizedList.renderedItems` recomputes → every static
item re-renders. With the constant, the cascade dies at the source.
Helps both VP and legacy paths.
- VP-path unit tests for MainContent (4 cases): ScrollableList mounts
and Static does not when `useTerminalBuffer: true`; ShowMoreLines is
reachable in VP mode (regression of the OverflowProvider mis-wrap);
source-copy index offsets thread into renderItem for static items;
renderItem callback identity is stable across `activePtyId` flips
(proves the ref-based read keeps StaticRender memo effective).
* fix(cli): stabilize absorbedCallIds in compact mode + gate heights prune + tighten ShowMoreLines test
Round-2 audit follow-ups. Three real findings addressed; one flagged
false positive documented separately.
1. **absorbedCallIds Set identity now content-stable when compact mode is
on.** The earlier EMPTY constant only short-circuited the compactMode=
false path; when compact mode is enabled (some users default-on it),
activePtyId / embeddedShellFocused flips during streaming still
produced fresh Sets per render even when membership was unchanged,
restarting the same cascade the pendingStateRef fix was meant to
avoid. Compare-and-reuse via a ref: if the new Set has identical
membership to the previous one, return the previous reference.
2. **`heights` map prune in `VirtualizedList` is gated.** Previously
every streaming tick rebuilt an N-key Set and walked all heights,
even on the steady-state path where nothing changes. Now only fires
when the heights record has clearly outpaced live data
(`size > max(8, 2 × data.length)`) — covers `/clear` and accumulated
pending → completed transitions, skips the 30-Hz hot path entirely.
3. **VP ShowMoreLines test now actually verifies overflow connectivity.**
Previous mock unconditionally rendered "SHOW_MORE", so the test only
proved the JSX mounted — it would still pass if a future refactor
moved `<OverflowProvider>` out of the VP tree again. The mock now
reads `useOverflowState()` and emits "OVERFLOW_DISCONNECTED" when the
context is missing. The VP test asserts both presence of "SHOW_MORE"
and absence of the disconnected marker, so the regression is now
caught.
Not addressed:
- Audit P0-1 claim that `renderMode` (Alt+M) / model-change updates
don't reach VP static items: false positive. `renderMode` is a React
Context (`RenderModeContext`), and Context propagation traverses the
tree past `memo` boundaries — MarkdownDisplay's `useRenderMode()`
consumer re-renders on context change regardless of whether
`StaticRender` bails out. Verified by reading
`packages/cli/src/ui/contexts/RenderModeContext.tsx` and
`MarkdownDisplay.tsx:172`. No code change.
- Audit P1-2 pendingStateRef write-during-render race: speculative,
relies on a multi-pass render path React 18+ does not currently use.
Documented assumption in the existing inline comment.
* fix(cli): isolate renderItem errors + defensive height coerce + compact-mode mergedHistory stability
Round-3 audit follow-ups. Three real findings; the rest verified clean.
1. **`renderItem` errors no longer crash the CLI.** Previously a throw
inside a per-item render propagated through `VirtualizedList`'s
useMemo into React's commit phase, tearing down the whole Ink tree —
one bad history record could nuke the session. Wrap each call in a
try/catch and substitute a small red `[render error] …` text box on
failure. The row stays in the viewport so the user can scroll past
it.
2. **Defensive height coerce in offset accumulation.** A buggy
`estimatedItemHeight` returning NaN / negative / Infinity would
poison every downstream offset and break the `upperBound` /
`findLastLE` binary search (which assumes monotonic offsets). Clamp
to `Number.isFinite(raw) && raw > 0 ? raw : 0`. No-op for the
in-tree estimators that return 3; insurance against future
consumers.
3. **`mergedHistory` is content-stable when compact mode is on.** The
Round-2 absorbedCallIds stability fix didn't reach this path:
`mergeCompactToolGroups` always allocates a fresh array, and
`mergedHistory`'s useMemo lists `activePtyId` / `embeddedShellFocused`
as deps, so every streaming tick mid-shell-tool produced a new array
even when items aligned. Cascade went `mergedHistory` → offsets map
→ `renderVirtualItem` → every static item re-rendered. Pair-wise
compare new vs previous and return the previous reference when items
align. Restores StaticRender memo effectiveness for compact-mode
users.
Not addressed (audit findings deemed not worth fixing in this PR):
- `scrollToItem` silently no-ops when item is not in data — no current
caller checks the return value, low impact.
- `allVirtualItems` array spread is O(n) per streaming tick — real but
not a crash; revisit in a perf-focused follow-up.
- `itemRefs.current` is dead surface (never read) — cosmetic.
- StrictMode-only-in-DEBUG double-invoke paths verified safe.
* test+chore(cli): VP review round 4 — VirtualizedList/useBatchedScroll coverage + cleanups
Addresses wenshao's CHANGES_REQUESTED review on PR #3941.
- Add focused unit tests for `VirtualizedList` (9 cases) covering empty
data, `renderStatic` full-render, `initialScrollIndex` with
`SCROLL_TO_ITEM_END`, `targetScrollIndex` anchoring, imperative
`scrollToEnd` / `scrollToIndex`, per-item `renderItem` error isolation,
NaN/negative estimator coercion, and out-of-range `initialScrollIndex`
clamping.
- Add `useBatchedScroll` unit tests (4 cases) covering initial reads,
pending-value reads in the same tick, post-commit pending reset, and
callback identity stability across rerenders.
- Remove dead `itemRefs` / `onSetRef` plumbing (declared, written, never
read; `useCallback` with empty deps was also a stale-closure trap).
- Remove unused `isStatic?: boolean` from `VirtualizedListProps`
(only `isStaticItem` is actually consumed).
- Tighten the render-phase setState block: each setter is now guarded
by an equality check so React bails out of redundant updates, and a
comment documents that this is the React-endorsed "adjusting state
while rendering" pattern (the synchronous update avoids a one-frame
flash at the previous position when `targetScrollIndex` changes).
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* chore(cli): remove dead `dataRef` from VirtualizedList (round-4 followup)
Declared and written in a `useLayoutEffect` on every `data` change but
never read anywhere in the component. Flagged in wenshao's round-4 review
of PR #3941.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(cli): collapse model-change effect back into one batched handler
wenshao's PR #4119 review correctly flagged that splitting the
onModelChange flow into two effects (b25831b0e) reintroduced the
issue #3899 freeze regression on every model switch:
1. setCurrentModel(model) commits first, with the OLD
historyRemountKey.
2. <Static key={`${historyRemountKey}-${currentModel}`}> sees its
key change (because currentModel did) and remounts immediately.
3. MainContent's render-phase progressive-replay reset only fires
when historyRemountKey changes, so replayCount is still the
full mergedHistory.length from any prior catch-up.
4. The remounted Static dumps the entire history in one synchronous
layout pass — exactly the freeze progressive replay was added
to avoid (#3899). The second effect's refreshStatic() bump
arrives a render too late.
Fix: do not split. Both side effects (refreshStatic, which writes
clearTerminal + bumps historyRemountKey, and setCurrentModel) live
in the event handler again, with a ref guard for same-model
notifications. The React.StrictMode concern that motivated b25831b0e
is addressed by keeping the side effect OUT of the setState updater
(it now runs once per event-handler invocation, not once per
double-invoked updater call). Both setState calls land in the same
React batch, so historyRemountKey and currentModel update together —
MainContent's render-phase reset sees the new key, replayCount drops
to the first chunk, and Static remounts with chunked replay intact.
Tests:
- AppContainer.test.tsx: 4 new tests covering the synchronous
refreshStatic side-effect contract, same-model no-op, ref-guarded
StrictMode double-invoke, and unsubscribe-on-unmount.
- MainContent.test.tsx: new regression guard — when currentModel
changes but historyRemountKey is held constant, progressive replay
must NOT reset (pins the MainContent invariant the two-effect
refactor accidentally relied on).
Verified: vitest packages/cli AppContainer + MainContent green (82/82).
Typecheck clean.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix+docs(cli): VP review round 5 — typecheck, doc drift, scroll keys
PR #4146 review feedback (wenshao + Claude Opus 4.7 audit) addressed:
Code:
- MainContent.test: activePtyId typed as number (was 'pty-xyz' string,
broke tsc with TS2322 — the test only relies on reference change so
any number works).
- VirtualizedList: sanitize renderItem error path. Display becomes the
generic `[render error]` marker; full err goes to debugLogger.debug
so file paths / partial tool state don't leak to scrollback.
- MainContent: move pendingSourceCopyOffsetsByIndex into a ref so it
no longer rebuilds renderVirtualItem identity every streaming tick.
Without this, VirtualizedList.renderedItems useMemo invalidated
per-tick → JSX rebuilt for every visible item → memo(HistoryItem
Display) was still bailing but allocations were O(visible) per tick.
- AppContainer: drop the misleading "state-driven scroll reset" claim
in the VP refreshStatic comment. VP is intentionally near-no-op:
the React tree owns the visible region, mergedHistory mutation is
what refreshes the screen, and the remount-key bump is preserved
only to keep the legacy Static branch in sync if the user toggles
the flag off mid-session.
- StaticRender: rewrite JSDoc to match reality. The custom React.memo
is NOT output caching like @jrichman/ink's StaticRender export;
the comparator rarely matches (parent allocates fresh JSX); the
real skip happens at memo(HistoryItemDisplay) one level deeper.
Docs:
- docs/design/virtual-viewport: sync file map (drop non-existent
ScrollProvider.tsx / useAnimatedScrollbar.ts), PR sequence (one PR
#4146, V.3-V.5 deferred), open-question + checklist resolution for
#3905 (superseded) and base branch rename.
- docs/users/reference/keyboard-shortcuts: document the 6 VP scroll
keys (Shift+↑/↓, PgUp/PgDn, Ctrl+Home/End) under a "History
scrollback (when ui.useTerminalBuffer is on)" section. Previously
the only discovery path was the Settings dialog description.
Verified: tsc --noEmit -p packages/cli ✓, vitest 160/160 ✓ across
AppContainer / MainContent / VirtualizedList / useBatchedScroll /
keyMatchers / settingsSchema, eslint clean on touched files.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): SGR mouse wheel scroll in VP mode
Recovers the most-felt UX regression vs legacy `<Static>` mode: when
`ui.useTerminalBuffer` is on, legacy users lose mouse wheel as a way
to scroll history (the host terminal stopped seeing the conversation
in its scrollback buffer). This PR enables button-event tracking
(`?1002h`) + SGR coordinates (`?1006h`) while the ScrollableList has
focus, parses wheel events off stdin, and routes them to scrollBy.
Scope kept tight on purpose:
- Wheel only. Hit-testing for scrollbar drag / click-to-position
needs screen-absolute element coords; stock ink 7's useBoxMetrics
returns yoga's parent-relative layout. Deferred to V.4 with two
exit paths (upstream getBoundingBox to ink 7, or local yoga walker).
- Mouse mode is enabled only while ScrollableList is mounted; non-VP
users never see their terminal flipped into button-event tracking.
- Side effect: native click-and-drag text selection is captured by
the program. Docs + settings dialog description now spell out the
Shift / Option (macOS) bypass.
Implementation:
- `ui/utils/mouse.ts` — SGR + X11 parser, ported and trimmed from
gemini-cli (Google LLC, Apache-2.0). Single-consumer.
- `ui/hooks/useMouseEvents.ts` — enable/parse/disable lifecycle
hook. Listens on stdin via `useStdin().stdin`, runs handler
through a ref so callers don't have to memoize.
- `ui/components/shared/ScrollableList.tsx` — subscribe to mouse
events, route wheel → `scrollBy(±3)`. Also drops a dead outer
`<Box flexGrow={1}>` wrapper that held an unread containerRef
and collapsed to zero height in ink-testing-library (the test
renderer has no flex parent, so flexGrow=1 → 0 height → no items
ever rendered, which is how this dead code was exposed).
Tests:
- `ui/utils/mouse.test.ts` — 14 cases: SGR parsing (wheel, presses,
modifiers, move), X11 parsing, fallback chain, incomplete-sequence
guard (including the >50-byte garbage cap).
- `ui/components/shared/ScrollableList.test.tsx` — 3 cases: wheel
events shift the rendered window; hasFocus=false makes the mouse
pipeline inactive (no throw); non-wheel events leave the window
unchanged. Renders are wrapped in `<KeypressProvider>` (required
by useKeypress in production but easy to forget in standalone
tests).
Docs:
- `docs/users/reference/keyboard-shortcuts.md` — adds "Mouse wheel"
row + the Shift/Option-to-select note.
- `packages/cli/src/config/settingsSchema.ts` — the in-app dialog
description now mentions mouse wheel and the text-select bypass.
- `docs/design/virtual-viewport/README.md` — §1 status, §5 file map,
§7 PR sequence all reflect mouse wheel landing in #4146 and the
V.4–V.7 follow-up split (scrollbar drag / in-app search / alt-
buffer / host-scrollback dual-write research).
Verified: tsc --noEmit -p packages/cli ✓, vitest 182/182 ✓ across
AppContainer / MainContent / VirtualizedList / ScrollableList /
useBatchedScroll / mouse / keyMatchers / settingsSchema.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): auto-hide animation for VP scrollbar thumb
Pairs with the SGR mouse-wheel work from the previous commit:
when the user actually scrolls, the thumb pops bright; after a
1.5s idle it fades into the dim track so the bar stops competing
with the conversation. The track column itself stays in layout
regardless, so the viewport never reflows mid-flash (which would
trigger per-item re-measure and a visible jitter).
Implementation kept minimal for stock ink 7:
- gemini-cli's `useAnimatedScrollbar` interpolates RGB colors via
a theme + per-frame setInterval. The terminal can't render
smooth fades anyway, so this hook collapses the state to a
binary `isVisible` flag with a single setTimeout. ~75 LoC.
- `VirtualizedList` calls `flashScrollbar()` from a useLayoutEffect
keyed on `clampedScrollTop`. The very first commit is skipped
via a ref so initial mount doesn't paint a flash.
- The render switches the thumb glyph (`█` vs `│`) and `dimColor`
based on `isVisible && inThumb`. Width stays 1 either way.
Tests (6 new):
- initial mount stays hidden (no spurious mount flash)
- flash → visible, hides after idle timeout, successive flashes
reset the timer (no premature hide), idleHideMs<=0 disables
auto-hide for tests that want to assert on the visible state,
unmount cleans up the pending timer.
Doc updates:
- `docs/design/virtual-viewport/README.md` §1 status, §5 file map,
§7 PR sequence — V.4 row now scopes only the drag/click-jump
work (still coord-blocked); animated scrollbar moved out of
deferred and into shipped.
- PR #4146 body — architecture table mentions the auto-hide, new
files list adds `useAnimatedScrollbar.ts`, test count refreshed
to 188/188.
Verified: tsc --noEmit -p packages/cli ✓, vitest 188/188 ✓.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(cli): VP review round 6 — ESC bug, CI lint, scope-controlled cleanup
Triage of /review feedback from 2026-05-18 + 2026-05-19. Took the
ones that are real and small; declined the ones that are
false-positive / out-of-scope so this PR stops expanding.
Must-fix:
- CI Lint failure: vscode-ide-companion/schemas/settings.schema.json
was stale after the keyboard-shortcuts description bump. Regenerated
via `npm run generate:settings-schema`.
- useMouseEvents.ts had `const ESC = '';` (literal empty string after
the raw 0x1B byte got stripped somewhere in the source pipeline).
`buffer.indexOf('', 1) === 1` would have degraded garbage skipping
to a one-byte scan, and the `else { buffer = ''; break }` branch
could never run. Fixed by switching to the `'\x1b'` text escape and
doing the same in `mouse.ts` (which had the raw byte, also fragile).
Comment explains why.
Small wins (one-liners taken from the review batch):
- ScrollableList: rest-spread separates `hasFocus` from the props
forwarded to VirtualizedList. Latent collision risk; no behaviour
change today.
- VirtualizedList: `debugLogger.debug` when isReady=false so blank-
viewport edge cases (tiny terminal / mid-resize race) become
diagnosable from the debug log instead of looking like a hang.
Real perf (VP-only):
- MainContent: gated the progressive-Static-replay machinery behind
`!useVirtualScroll`. The render-phase reset still consumes the
remount-key bump so flag-off toggles mid-session catch up cleanly,
but `setReplayCount` and the setImmediate chunking effect are now
skipped for VP users. Saves ~M/CHUNK_SIZE wasted re-renders per
Ctrl+O / model change on a 1000-turn session.
Belt-and-braces:
- useMouseEvents: added a `process.on('exit')` handler that writes
the SGR mouse disable seq again. The React cleanup already covers
normal unmount, but Ctrl+C / SIGTERM / parent kill bypass it and
the terminal would otherwise stay in button-event-tracking mode
after qwen exits.
Explicitly declined / deferred (with reasoning logged on the PR):
- requestAnimationFrame wheel throttle: rAF doesn't exist in Node;
React 19 already batches state updates within a tick, and the
renderedItems memo bounds the actual work to visible items. Will
revisit if profiling shows it.
- Stable pending-item IDs (`p-N` keys shifting on completion): the
observable jitter is at most one frame of estimated-vs-actual
height delta. Moderate scope (creation-time ID allocation); fits
better in a focused follow-up than in this PR.
Verified: tsc --noEmit -p packages/cli ✓, vitest 188/188 ✓ across
the full VP suite.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(cli): scrollBy bottom uses live end anchor in virtualized list
When keyboard scroll reaches the bottom, scrollBy set isStickingToBottom
but anchored via getAnchorForScrollTop(maxScroll), a fixed {index,offset}
pixel anchor. scrollTo/scrollToEnd instead use {index: last, offset:
SCROLL_TO_ITEM_END}, which recomputes the bottom from live item heights
each render. The fixed anchor did not track the last item growing during
streaming, so scroll-to-bottom via keyboard lagged behind new tokens.
Align scrollBy's bottom branch with the sibling methods.
Reported by wenshao in PR review.
* fix(cli): parse mouse events via ink useInput, not a stdin data listener
useMouseEvents attached its own stdin.on('data', ...) listener. Adding a
'data' listener switches stdin into flowing mode, which drains the buffer
before ink's readable + stdin.read() reader (ink App) can consume it, so
all keyboard input routed through useInput was silently starved while
mouse mode was active.
Parse mouse sequences from ink's existing input pipeline via useInput
instead, so there is only one stdin reader. ink captures a full SGR
sequence (ESC [ < .. M/m) as a single CSI event and delivers it with the
leading ESC stripped, so we re-prepend it before parsing. Non-mouse input
does not match and is ignored; ink still routes input to the app's other
useInput handlers, so keyboard navigation keeps working.
Only SGR mode (1006h, which we enable) is parsed via this path; the legacy
X11 encoding is not recoverable through ink's CSI parser, which is the
encoding modern terminals stop emitting once 1006h is set.
Reported by wenshao in PR review.
* fix(cli): parse only SGR in mouse hook to avoid X11 paste misfire
The useInput-based mouse hook called parseMouseEvent, which also tries the
X11 fallback (parseX11MouseEvent). An X11 prefix (ESC [ M + 3 bytes) can
reach the handler via pasted text — ink emits paste content as input when
no paste listener is registered — and would misfire a spurious mouse event.
Call parseSGRMouseEvent directly so only the SGR encoding we enable (1006h)
is parsed, matching the hook's documented contract.
Reported by wenshao in PR review.
* test(cli): assert SGR mouse parser rejects X11 sequences
Locks in the security property behind the parseMouseEvent ->
parseSGRMouseEvent switch in useMouseEvents: an X11 sequence arriving as
pasted text must not misfire a mouse event. Asserts a well-formed X11
sequence is a valid X11 event yet returns null from parseSGRMouseEvent, so
a future revert to parseMouseEvent fails this test.
Reported by wenshao in PR review.
* test(cli): add VP scroll coverage + eslint-disable for useBatchedScroll
Cover keyboard scroll commands (Shift+Up/Down, PageUp/Down, Ctrl+Home/End),
scrollBy/scrollTo imperative API (positive/negative/overflow/clamp), and
auto-scroll-during-streaming state machine (stick-to-bottom, disengage on
user scroll, re-engage on scrollToEnd). Add missing eslint-disable-next-line
for intentionally dep-free useLayoutEffect in useBatchedScroll.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* chore(cli): remove trailing whitespace in useBatchedScroll
The eslint-disable-next-line comment was removed by eslint --fix as an
unused directive (exhaustive-deps does not flag a useLayoutEffect with
no dependency array). Clean up the residual blank line.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
---------
Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): background housekeeping for stale file-history dirs (#4414)
PR #4064 introduced ~/.qwen/file-history/{sessionId}/ for /rewind but had
no cross-session cleanup — directories accumulated indefinitely. This adds
a generic background housekeeping framework with file-history cleanup as
its first user.
- 30-day mtime sweep, configurable via general.cleanupPeriodDays
- 10-min startup delay (1-min catch-up if last run >7d ago)
- 24h recurring cadence, idle-gated (defers if user typed in last 1 min)
- O_EXCL lockfile + marker mtime throttle (multi-process safe)
- Current session whitelisted via lazy config.getSessionId() — defends
against long-idle active sessions and /clear minting a new session
- Negative cleanupPeriodDays values clamp to 1h minimum (defends against
schema-bypass: a future cutoff would otherwise sweep everything)
- Zero new prod dependencies; ~70 lines of self-written O_EXCL throttle
primitive in lieu of proper-lockfile (which pulls graceful-fs and
monkey-patches every fs method on first require)
- All setTimeout(...).unref() — never blocks process exit
Closes #4173.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(core): loosen auto-mode classifier timeouts, disable stage-2 thinking (#4680)
* fix(core): loosen auto-mode classifier timeouts, disable stage-2 thinking
The AUTO-mode classifier fails closed on timeout — a timed-out judge call
blocks the action as "unavailable". The tight 3s/10s stage budgets turned
transient slowness (slow network, large transcript, model queueing) into
spurious blocks of otherwise-valid actions. Raise them to 10s/30s so a
slow-but-healthy call is not treated as a hard block.
Also disable thinking in stage 2 (previously the only stage with
includeThoughts: true). This is a latency-sensitive permission gate the
user is actively waiting on; allocating a reasoning budget made the review
path slower and more expensive, which directly worsened the fail-closed
timeout. The model still records its reasoning in the structured
`thinking` output field — it just no longer gets an allocated budget.
Closes #4676
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* docs(core): trim verbose comments in auto-mode classifier
Condense the three comments touched by this change (module docstring
stage-2 note, timeout-budget rationale, stage-2 thinkingConfig) while
keeping the essential "why". No logic changes.
Co-authored-by: Qwen-Coder <noreply@qwenlm.ai>
---------
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <noreply@qwenlm.ai>
* fix(core): coerce hostile-provider usage token counts (#4350 part 1) (#4439)
* fix(core): coerce hostile-provider usage token counts (#4350 part 1)
Hostile providers (broken upstream, OpenAI-compat proxy returning
null/NaN, misconfigured override) can emit non-finite or negative
values for `usageMetadata.{prompt,candidates,cached,total}TokenCount`.
Captured unguarded in `processStreamResponse`, these poison the
compaction gate arithmetic:
- `lastPromptTokenCount + NaN >= hard` is always false → hard-rescue
is silently disabled, eventually OOMing the V8 heap.
- `Infinity >= hard` is always true → hard-rescue fires every send.
Route the four API capture sites through a `coerceUsageCount` helper
that maps unknown / non-finite / negative to 0. `Number.isFinite(-1)`
is true, so an explicit `>= 0` is needed in addition to `isFinite`.
Part 1 of the hostile-provider hardening from #4350. The companion
`computeThresholds` guard depends on the un-merged three-tier ladder
in #4345 and is deferred until that lands.
Covered by parametrized tests in `geminiChat.test.ts` over NaN,
±Infinity, negative, null, undefined, and string inputs, plus a
fallback test asserting …
* fix(cli): persist /memory toggle state across dialog reopen (#4650)
The Auto-memory / Auto-dream / Auto-skill rows initialized their state
from Config getters, which are frozen at startup and never reflect a
setValue() write. Each /memory reopen re-mounts the dialog and re-reads
that stale snapshot, so a just-flipped toggle appeared to revert. Read the
initial state from the live merged settings instead, matching the existing
write path (bareMode semantics preserved).
Also switch the test's `act` import to `react` — the previously used
@testing-library/react is declared in package.json but not installed, so
the suite could not run — and add a mount/unmount/remount regression test.
* Hide internal docs from docs site (#4357)
* fix(core): preserve uid in atomicWriteFile to avoid breaking shared-write files (#4431)
* fix(core): preserve uid/gid in atomicWriteFile to avoid breaking shared-write files
atomicWriteFile uses write-to-tmp + rename for crash atomicity. POSIX
rename creates a new inode owned by the calling process's euid/egid, so
the rename silently strips the original uid/gid. On shared-write setups
(e.g. a group-writable file owned by another user in a shared workspace
where the current user has group-write access), every Write/Edit/
NotebookEdit through qwen-code would reset ownership to the running
user and effectively revoke write access for the original collaborators.
The fix:
1. If the target exists and is owned by a different uid/gid than the
process's effective uid/gid (and we are not root), fall back to
in-place writeFile. This truncates the existing inode in place,
preserving uid/gid. The trade-off is loss of crash atomicity for
this specific case — an acceptable trade for not silently breaking
shared-write file ownership.
2. If running as root, atomic rename is still used, and ownership is
restored via chown(uid, gid) after the rename. Root can chown back;
non-root cannot, hence the in-place fallback for non-root.
3. Windows is unaffected (no POSIX ownership semantics).
Tests:
- New: in-place fallback on uid mismatch — verify content updates, mode
preserved, and inode unchanged (the inode is the signal that the
fallback path ran rather than rename).
- New: same scenario triggered via gid mismatch.
- New: positive case — ownership matches → atomic rename → inode changes.
Regression: a v0.16.0 user reported "every write turns a world-writable
file into one other users can no longer write." Bisected to #4096 which
introduced atomicWriteFile + write-to-tmp + rename.
* fix(core): route root through in-place fallback + doc/test follow-ups
Review follow-ups on the atomic-write ownership fix:
1. Remove the root-special-case (rename + post-rename chown). chown
silently fails inside user-namespaced or CAP_CHOWN-stripped Docker
containers, which re-triggers the original bug for root-in-Docker
users — exactly the scenario this fix was reported against. Routing
root through the same in-place fallback as non-root eliminates this
failure mode and drops an untestable branch (chown-back can't be
exercised under non-root CI).
2. Document the three properties traded away by the in-place fallback:
crash atomicity, concurrent-reader isolation, inotify watcher
semantics (MODIFY vs MOVED_TO).
3. Document that the in-place fallback surfaces EACCES when the file's
mode forbids the current user from writing — this is correct
behavior (atomic rename used to silently replace files the user had
no permission on, which was arguably a privilege issue).
4. Replace the brittle "see step 6 in the function doc" comment with a
step-number-independent reference.
5. New test covering the EACCES path: chmod 0o444 + mocked geteuid
triggers the fallback, fallback hits the read-only file, EACCES
propagates cleanly, original content is preserved.
* fix(core): harden in-place fallback against symlink/unlink/inode races + doc/test follow-ups
Review follow-ups on #4431 ownership-preservation fix:
CRITICAL — in-place fallback security hardening (wenshao review):
The path-based `fs.writeFile(targetPath, ...)` fallback introduced
three races that the prior `rename(tmp, target)` form did not have:
1. Non-regular files (FIFO/socket/device): fs.writeFile calls
open(O_WRONLY|O_CREAT|O_TRUNC). On a FIFO this blocks forever
waiting for a reader. On a character/block device it writes to
the actual device. The rename path replaced these with a
regular file.
2. Symlink-swap TOCTOU: an attacker with parent-dir write can swap
targetPath for a symlink between our stat and our writeFile.
fs.writeFile follows symlinks at the destination; POSIX rename
does not. In the very "shared-write workspace / Docker bind-mount"
scenarios this PR targets, this lets a directory-writable
attacker redirect agent writes elsewhere (e.g. /etc/passwd if
the agent runs as root).
3. Unlink race: if targetPath is unlinked between stat and write,
O_CREAT silently recreates it owned by the calling user — the
exact ownership change the fallback was designed to prevent.
Silent regression to the pre-fix bug under this race.
Fix: extract the fallback into writeInPlaceWithFdGuards():
- open(target, O_WRONLY | O_TRUNC | O_NOFOLLOW) — no O_CREAT, so
unlink-race surfaces ENOENT instead of silently recreating; and
O_NOFOLLOW rejects symlink-swaps with ELOOP.
- fstat(fd) verifies the bound inode's uid/gid still match
existingStat — refuses the write if an inode-swap happened
between stat and open.
- Write through the fd (locked to the verified inode), chmod
through the fd, close.
Caller now gates the fallback on existingStat.isFile() — non-regular
targets fall through to the atomic path which has well-defined
"replace special-file with regular-file" semantics.
DOC / TEST follow-ups:
- Add hardlink-propagation as a 4th trade-off in the in-place
fallback JSDoc (review comment #4): rename creates a new inode so
sibling hardlinks keep old content; in-place truncate+write keeps
the inode so all hardlinks see new content.
- Update atomicWriteJSON JSDoc to note the write is now
*conditionally* atomic (review comment #5): atomic when uid/gid
matches the process, in-place when ownership differs. Previously
the JSDoc still claimed unconditional atomicity.
- Update caller comments at runtimeStatus.ts and
worktreeSessionService.ts that advertised crash-atomic writes via
tmp+rename — those guarantees are now conditional (review
comment #6).
- Add mode + tmp-leftover assertions to the gid-mismatch test to
match the uid-mismatch test (review comment #2 — test
consistency). Without these, a gid-fallback regression that
silently dropped permissions or left a tmp file would not be
caught.
- New test: FIFO + ownership mismatch must take the atomic path,
not in-place (verifies the existingStat.isFile() guard works;
hang on in-place would trip vitest timeout).
- New test: writing through a symlink with ownership mismatch
exercises the resolve-then-stat-then-open flow and verifies the
symlink itself is preserved.
Tests: 192/192 pass (atomicFileWrite + write-file + edit +
fileSystemService).
* fix(core): defer O_TRUNC and verify dev+ino in writeInPlaceWithFdGuards
PR #4431 review follow-up (wenshao critical):
The previous form opened with `O_WRONLY | O_TRUNC | O_NOFOLLOW`, which
truncated the bound file *before* the fd-bound fstat verification ran.
If an attacker swapped the path between the caller's stat and our
open, we would truncate the attacker's substituted inode (destroying
unrelated content) before detecting the swap.
Two fixes:
1. Open without O_TRUNC. Verify dev+ino+uid+gid+isFile match
expectedStat through fh.stat(). Only then call fh.truncate(0)
through the validated fd.
2. Expand the verification beyond uid+gid to include dev+ino+isFile.
uid+gid alone misses a same-owner inode swap (attacker replaces
the path with a different inode they own). dev+ino is the strong
identity check; isFile catches a swap to FIFO/socket/device after
the caller's existingStat.isFile() gate.
JSDoc updated to enumerate the four guards (NOFOLLOW, no CREAT, no
TRUNC at open, dev+ino+uid+gid+isFile via fstat) and explain why
truncation must wait until after verification.
192/192 tests pass.
* fix(core): close FIFO swap race with O_NONBLOCK + cover EOWNERSHIP_CHANGED path
PR #4431 review follow-up (deepseek-v4-pro via /review):
CRITICAL — FIFO swap TOCTOU:
The caller's `existingStat.isFile()` gate uses stat data captured
earlier. An attacker with parent-dir write can swap the regular file
for a FIFO between the caller's stat and our open inside
`writeInPlaceWithFdGuards`. The previous `O_WRONLY | O_NOFOLLOW` open
would then block indefinitely waiting for a FIFO reader; O_NOFOLLOW
only catches symlinks.
Fix: add O_NONBLOCK to the open flags. Defense in depth:
- On a reader-less FIFO, `open(O_WRONLY | O_NONBLOCK)` returns ENXIO
immediately — no hang.
- If the FIFO has a reader (open succeeds), the subsequent fstat
isFile() check still refuses the write via EOWNERSHIP_CHANGED.
- For regular files, O_NONBLOCK is a no-op.
CRITICAL test gap — EOWNERSHIP_CHANGED branch untested:
The primary TOCTOU defense (fdStat dev/ino/uid/gid/isFile vs
expectedStat) had no coverage. Exported `writeInPlaceWithFdGuards` so
it can be unit-tested directly:
- New test: simulate post-stat inode swap (unlink + recreate at same
path), call helper with stale stat, assert EOWNERSHIP_CHANGED and
that the attacker's content survives.
- New test: simulate post-stat regular→FIFO swap, assert open fails
fast (ENXIO) or fstat catches it — either way no hang, no write.
DOC fix:
JSDoc said "we open read-write without truncating" but the code uses
O_WRONLY. Wording corrected to "write-only".
194/194 tests pass.
* fix(core): fix flaky inode-swap test + apply review follow-ups
PR #4431 review follow-up (glm-5.1 via /review) — 7 suggestions adopted,
1 partially adopted, 0 rejected:
CI FIX (Ubuntu test failure on tmpfs inode reuse):
The EOWNERSHIP_CHANGED inode-swap test used unlink+create to simulate
a post-stat swap. On Linux tmpfs the freshly-freed inode number is
often reused by the immediately-following create, so dev+ino remained
identical and the guard didn't trip (intermittent on Ubuntu CI; macOS
APFS happened to allocate different inodes). Switched to rename(decoy,
target) which moves an existing distinct inode into place, guaranteed
to differ from the original.
CODE:
- Wrap fh.writeFile failure after fh.truncate(0) with
EINPLACE_WRITE_FAILED + cause, so callers see explicitly that the
file was truncated and the write didn't complete (otherwise they
see raw ENOSPC/EIO and may wrongly assume the original is intact
given this lives in atomicFileWrite.ts).
- Skip fh.chmod when euid is neither root nor expectedStat.uid —
chmod is guaranteed to fail with EPERM in that case (POSIX requires
owner or root). Avoids a guaranteed-failing syscall on every call.
- Caller catches ENOENT from writeInPlaceWithFdGuards and falls
through to atomic rename path. If the file was deleted between
caller's stat and our open there is no ownership to preserve; the
rename path correctly creates a new file at targetPath.
DOC:
- Replaced "defends against four races" with "hardened against
post-stat races" (the bullet list has 5 items, the count was wrong).
- Reworded "non-regular targets must not reach this function" to
describe defense-in-depth — O_NONBLOCK + !fdStat.isFile() reject
post-stat regular→FIFO/socket/device swaps. The old wording made
it look like O_NONBLOCK was redundant.
- Documented the dual chmod behavior (root vs non-root with foreign
uid) inline.
TESTS:
- Added happy-path test for writeInPlaceWithFdGuards (write succeeds,
inode preserved, mode preserved).
- Added ENOENT regression test (verifies the missing-O_CREAT
property — if file unlinked between stat and open, no silent
recreate with caller's uid).
- Renamed the misleading "O_NOFOLLOW guard" test (it actually tests
resolve-through-symlink, not O_NOFOLLOW) to reflect what it does,
and added a direct ELOOP test that drives writeInPlaceWithFdGuards
with a path whose final component is a symlink — that's the real
O_NOFOLLOW exercise.
- Fixed the FIFO test to pass a stat captured from the FIFO itself
(not a stale regular-file stat) so only the FIFO-specific defense
fires, not the inode/dev mismatch from a different file.
NOT ADOPTED:
- Skip-when-non-root chmod optimization adopted (small, useful), but
the larger "structured chmod error model" deferred — best-effort
matches the existing tryChmod pattern at file scope.
197/197 tests pass.
* fix(core): wrap truncate err + post-write nlink check + guard close + chmod sync
PR #4431 review follow-up (qwen-latest-series-invite-beta-v34 via /review)
— 7 of 10 suggestions adopted, 3 deferred:
CODE:
- **EINPLACE_TRUNCATE_FAILED wrap** (review #3291863048): symmetric to
the existing EINPLACE_WRITE_FAILED — distinguishes "truncate failed,
original intact" from "write failed post-truncate, original lost".
- **Post-write nlink === 0 check** (review #3291863059):
EINODE_UNLINKED_DURING_WRITE detects the fstat-to-close window where
a concurrent rename-over drops our bound inode's link count to zero
and our write goes to an anonymous inode close will free. Silent
data loss path now surfaces.
- **fh.close() guarded in finally** (review #3291863044): close failure
on NFS/FUSE was masking the original try-body exception (including
the meaningful EOWNERSHIP_CHANGED, EINPLACE_*, EINODE_*). flush:true
already fsync'd, so close-after-flush is best-effort.
- **fdStat.uid in canChmod** (review #3291863055 part 1): use the
fd-bound verified value instead of expectedStat.uid. Defense in depth
— a future weakening of the fstat guard won't silently widen chmod
privilege.
- **fh.sync() after chmod** (review #3291863053): chmod is metadata,
not covered by writeFile({ flush: true }). A crash before lazy
metadata flush would lose the mode restoration (matters for
setuid/setgid). One extra syscall, best-effort.
- **@remarks freshness contract** (review #3291863051 partial): JSDoc
now spells out that expectedStat MUST be a fresh stat captured
immediately before the call. Stale stats nullify every guard.
- **Concurrent-writer limitation noted** (review #3291863061 partial):
added a "Known limitation — no advisory locking" paragraph to JSDoc
rather than adopting flock (Linux-specific, NFS issues, scope
expansion). Callers needing multi-process coordination should layer
their own lockfile.
- **@throws documentation** (review #3291863051 partial): four
documented error codes (EOWNERSHIP_CHANGED, EINODE_UNLINKED_DURING_WRITE,
EINPLACE_TRUNCATE_FAILED, EINPLACE_WRITE_FAILED).
TESTS:
- **EINPLACE_WRITE_FAILED via FileHandle.prototype.writeFile monkey-patch**
(review #3291863040): triggers the data-loss path, asserts the wrapped
code + message + cause, and verifies the file is empty (truncate ran).
- **canChmod=false actually skips chmod** (review #3291863055 part 2):
prior uid-mismatch test had desiredMode === current mode, couldn't
distinguish "skipped" from "no-op". New test uses desiredMode=0o755
on a 0o644 file under canChmod=false → asserts mode stays 0o644.
NOT ADOPTED:
- ENOENT/ELOOP/ENXIO catch extension (review #3291863043): keeping the
strict refusal for swap-to-special-file. Silent fallthrough-to-replace
was pre-PR atomic-rename behavior, but in shared-write workspaces
(this PR's target users) a special-file appearing at the target path
is a signal worth surfacing, not papering over.
- Diagnostic logging (review #3291863049): the function has no logger
dependency today; adding one is an architecture decision outside
this PR's scope. The path taken is implied by the side effects
(inode preserved vs new) but agreed: out-of-band telemetry would
help ops. Defer to follow-up.
- flock advisory locking (review #3291863061 main): scope expansion;
Linux-specific semantics, NFS edge cases. Documented as known
limitation instead.
- Integration test for ENOENT fallthrough at atomicWriteFile level
(review #3291863043 part 1): ESM module bindings prevent monkey-
patching writeInPlaceWithFdGuards from outside. The unit test for
the helper's ENOENT path covers the throwing behavior; the catch is
3 lines and review-visible. Defer until a refactor opens an
injection seam.
- Error code string constants export (review #3291863051 part 3): two
codes don't merit a constant module. Magic strings are fine at this
size.
199/199 tests pass.
* docs(core): sync writeRuntimeStatus JSDoc with conditional-atomic contract
PR #4431 review follow-up: function-level JSDoc still claimed
unconditional "Atomically write" and "never sees a partially written
file", inconsistent with the module-level docblock updated in earlier
commits. Updated to describe the conditional-atomic behavior (atomic
when uid/gid matches, in-place fallback when ownership differs) and
explicitly note the concurrent-reader visibility trade-off in the
fallback path. Links to atomicWriteJSON for the full contract.
Doc-only change. 199/199 tests pass.
* fix(core): add explicit fh.sync() — FileHandle.writeFile ignores flush option
PR #4431 review follow-up (qwen3.7-max via /review):
CRITICAL — FileHandle.writeFile silently ignores flush:
Node.js FileHandle.writeFile takes an early-return path that bypasses
the flush option entirely (the option is only honored on the
path-based fs.writeFile form). Our previous code passed
{ flush: true } to fh.writeFile and relied on the implicit fsync.
The only explicit fh.sync() was nested in the chmod block guarded by
canChmod — which is FALSE precisely when a non-root group member
writes to a group-writable file they don't own (the exact shared-write
scenario this PR targets). Net effect: in that branch, zero fsync.
Data sits in the kernel page cache; a crash before lazy flush leaves
the file empty (truncate succeeded) or partially written.
Fix:
- Drop flush from the fhWriteOptions object (silently ignored anyway).
- Add an explicit `fh.sync()` after writeFile succeeds, gated on
options.flush. Runs BEFORE the chmod block so the canChmod=false
branch also fsyncs.
- The chmod-block fh.sync() becomes metadata-only (covers the mode
change), as the data is already on disk.
Updated comments to reflect the actual semantics rather than the
incorrect "writeFile({ flush: true }) fsyncs" assumption.
TESTS (partial adoption of review #3293252349):
- EINPLACE_TRUNCATE_FAILED: sibling test to EINPLACE_WRITE_FAILED.
Monkey-patches FileHandle.prototype.truncate to throw EIO; asserts
err.code + cause + "original content is intact" message, and
verifies the file's original bytes are unchanged (truncate didn't
run).
- Buffer in in-place fallback: locks in binary fidelity (byte-exact
comparison) so a future encoding-passthrough regression for Buffer
data would be caught.
NOT ADOPTED in this commit:
- EINODE_UNLINKED_DURING_WRITE test: requires post-write fh.stat()
mocking with call-count discrimination (first call: real stat for
verification; second call: nlink=0). The monkey-patch pattern works
but is fragile; deferred to a follow-up that may also refactor the
helper to accept an injectable stat fn for cleaner testability.
201/201 tests pass.
* fix: correct stale flush comment + add fh.sync() regression test
- Fix misleading close() comment that said "flush:true already
fsync'd" — the explicit fh.sync() does the actual fsync, not the
flush option (which is silently ignored on FileHandle.writeFile).
- Add regression test verifying fh.sync() is called when flush:true
and skipped when flush is absent, preventing silent removal of the
core durability fix.
Addresses wenshao review threads from 2026-05-23.
* test: add EINODE_UNLINKED_DURING_WRITE regression test
Monkey-patches FileHandle.stat to return nlink:0 on the post-write
check, verifying the nlink guard throws with the correct error code.
Addresses wenshao review from 2026-05-28.
* simplify: replace writeInPlaceWithFdGuards with plain fs.writeFile
Address yiliang114's review (CHANGES_REQUESTED):
1. [Critical] Remove ~120 lines of fd-level TOCTOU hardening
(writeInPlaceWithFdGuards) — over-engineering for a local CLI.
The in-place fallback now uses plain fs.writeFile + tryChmod,
matching the EXDEV fallback pattern.
2. [Suggestion] Fix macOS GID false-positive: only compare uid in
ownershipWouldChange(). macOS inherits parent dir GID for new
files, so egid !== file.gid was a false positive that needlessly
dropped crash atomicity.
3. [Suggestion] Trim 60+ lines of JSDoc to project style (AGENTS.md:
"default to none, add only when WHY is non-obvious").
Net: -748 lines. 24 tests pass.
* fix: restore Stats type import (TS2304 build failure)
* docs: narrow scope from uid/gid to uid-only preservation
The gid check is intentionally skipped because macOS inherits the
parent directory's GID for new files, making egid !== file.gid a
false positive. Update comments and PR description to match the
actual implementation scope.
* test: add inode assertion to symlink ownership-mismatch test
Proves the in-place fallback actually ran instead of atomic rename.
* Improve hooks matcher display (#4545)
* feat(cli): improve hooks matcher display
* test(cli): cover hooks navigation levels
* fix(cli): use session channel when closing ACP sessions (#4522)
Detach closeSession/killSession from the session entry's owning channel instead of the current attach target, so the correct channel is decremented and killed during channel overlap (old channel dying while a fresh channel is current). Extracts findChannelInfoForEntry/detachSessionIdFromEntryChannel helpers with unit + integration coverage. Fixes #4325.
* fix(core,cli): replace full-history structuredClone with shallow/tail variants to prevent OOM on resume (#4644)
* fix(core,cli): replace full-history structuredClone with shallow/tail variants to prevent OOM on resume
Several UI and service call sites clone the entire chat history via
structuredClone(getHistory()) every turn. On a resumed session with
thousands of entries, each clone allocates 150-200 MB transiently.
When multiple async side-requests overlap (suggestion generation,
auto-title, checkpointing), multiple clones coexist on the heap,
pushing V8 past its limit within 10 turns (2 GB heap cap).
Changes:
- AppContainer.tsx: use getHistoryTail(40, true) instead of
getHistory(true) + slice(-40)
- btwCommand.ts: same pattern, use getHistoryTail(40, true)
- sessionTitle.ts: use getHistoryShallow() (read-only filtering)
- sessionRecap.ts: use getHistoryShallow() (read-only filtering)
- useGeminiStream.ts: use getHistoryShallow() for checkpoint
serialization (only needs to survive JSON.stringify)
Closes #4624
* fix(test): update mocks for getHistoryShallow/getHistoryTail in sessionTitle and btwCommand tests
* fix(cli): migrate remaining getHistory() clone sites to shallow/tail variants
- AppContainer.tsx rewind path: getHistory() → getHistoryShallow()
(only used read-only by computeApiTruncationIndex)
- Session.ts ACP rewind: getHistory() → getHistoryShallow()
(only walks entries to compute truncation index)
- Session.ts stop-hook: getHistory() + filter(.model).pop() →
getLastModelMessageText() (O(1) backward scan, no clone)
* fix(core): use client-level getHistoryShallow with fallback
sessionTitle.ts and sessionRecap.ts were calling
chat.getHistoryShallow() directly, bypassing the client-level
wrapper that provides a getHistory() fallback when the chat
implementation doesn't support shallow reads. Use
geminiClient.getHistoryShallow() instead.
Update test mocks to match the new call site.
* fix(test): add getHistoryShallow and getLastModelMessageText to Session test mocks
Session.ts now calls chat.getHistoryShallow() in rewindToTurn and
chat.getLastModelMessageText() in the Stop hook. Update all mockChat
instances in Session.test.ts to provide these methods.
* feat(cli): add respectUserColors and hideContextIndicator options for statusline (#4670)
* feat(cli): add respectUserColors option to preserve ANSI colors in
statusline command output
* test(cli): add respectUserColors tests for useStatusLine and Footer
* feat(cli): add hideContextIndicator option to hide built-in context usage in footer
* docs: update statusline configuration docs with respectUserColors and hideContextIndicator
* fix(core): tolerate unsupported Streamable HTTP GET SSE (#4521)
Fixes #4326
* fix(insight): Harden insight facet normalization and empty qualitative handling (#3557)
* Harden insight facet normalization and empty qualitative handling
* feat: enhance AtAGlance component to accept target sections for dynamic rendering
* feat(cli): notify when background shells finish (#4355)
* feat(core): add simplify bundled skill (#3570)
* feat(core): add simplify bundled skill
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* test(cli): stabilize SettingsDialog restart prompt test
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(skills): use agent tool instead of task in simplify skill
The simplify skill referenced the 'task' tool for launching review passes,
but Qwen Code exposes 'agent' as the callable subagent tool ('task' is only
a legacy permission alias). Using 'task' would cause /simplify to stall when
trying to launch parallel review passes.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* docs: document simplify bundled skill
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* Update packages/core/src/skills/skill-manager.test.ts
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
* fix(core): repair simplify skill tests
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* Update packages/core/src/skills/bundled/simplify/SKILL.md
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
* fix(skills): address simplify review feedback (read-only passes, gitignore scope, safer dead-code removal)
- drop inert `argument-hint` frontmatter (argumentHint is never parsed or
rendered anywhere; no other bundled skill uses it)
- mark Step 2 review passes read-only so edits stay isolated to Step 4
- narrow the no-diff fallback to `git ls-files --modified --others
--exclude-standard` so ignored build output is excluded
- require a repo-wide caller check before removing code
- make the commands.md row state it edits code directly
- assert non-conflicting bundled skills survive cross-level dedup
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
---------
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
Co-authored-by: wenshao <wenshao@U-K7F6PQY3-2157.local>
* feat(skills): add agent reproduction workflows (#4118)
* chore(skills): add codex reproduce workflows
* feat(agent-reproduce): implement agent reproduction workflow and supporting scripts
* feat(skills): capture reference agent state diffs
* feat(cli): virtual viewport for long conversations on ink 7 (#4146)
* chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed)
PR #3860 first upgraded ink 6 → 7.0.2. PR #4083 reverted because of a
TUI regression: `<Static>` did not re-emit items when its `key` prop
was bumped, so `/clear` / Ctrl+O / refreshStatic left the history area
blank under ink 7.0.2.
ink 7.0.3 (released after #4083) contains the exact fixes:
- be9f44cda Fix: <Static> remount via key change drops new items (#948)
- 669c4386c Fix: Drop stale <Static> output from fullStaticOutput on identity change (#950)
- 7c2267c01 Fix `useBoxMetrics` not accepting ref objects with an initial null value (#945)
Changes:
- `ink` ^6.2.3 → ^7.0.3 (root hoist + cli direct)
- `react` ^19.1.0 → ^19.2.4 (cli direct; ink 7.0.3 peerDeps requires >=19.2.0)
- `react`/`react-dom` overrides ^19.2.4 added so the transitive graph
stays deduped to a single instance (avoids `Invalid hook call` from
multiple React copies, the classic ink-upgrade hazard)
- `wrap-ansi` already on ^10.0.0 from #4083's partial-revert (no change)
Verified:
- `npm ls ink` → single `ink@7.0.3` across all peer deps
- `npm ls react` → single `react@19.2.4`
- `npm run typecheck --workspace=@qwen-code/qwen-code` clean
- `npm run typecheck --workspace=@qwen-code/qwen-code-core` clean
- Composer.test.tsx 20/20, MainContent.test.tsx 6/6, TableRenderer.test.tsx
59/59 + 1 skipped — all key UI components green on the new ink
The Static-remount regression is upstream-fixed in 7.0.3, so the
runtime path is restored without needing #3941's overflowY-self-managed
viewport. #3941 (virtual viewport) remains an opt-in performance
feature on top.
* fix(deps,cli): add @types/react overrides + move refreshStatic out of setCurrentModel updater
Two follow-ups from the multi-round audit of the ink 7.0.3 re-upgrade:
1. @types/react / @types/react-dom now pinned to ^19.2.0 in root
overrides. packages/web-templates still declares @types/react ^18.2.0
in its devDeps. Today the CLI build is unaffected (web-templates's
18.x types are nested in its own node_modules and the React-using
src/insight and src/export-html files are excluded from its tsconfig
build), but a future reincludes-or-hoist accident would land
conflicting global JSX namespaces in the CLI compile graph. Match
the dep dedup we already enforce for `react` and `react-dom` so the
type graph stays as deduped as the runtime graph.
2. AppContainer's onModelChange handler was calling refreshStatic() as
a side-effect inside the setCurrentModel updater. React.StrictMode
double-invokes state updaters in dev, so model swaps fired two
clearTerminal writes + two <Static> key bumps. The double work was
masked under ink 6 (key changes were no-ops on <Static>), but ink
7.0.3 honors key changes — the doubled work is now potentially
visible as a faster flash-flash on every model switch.
Refactor: setCurrentModel becomes a pure setter; refreshStatic
moves into a useEffect keyed on currentModel with a ref-comparison
guard so the first render doesn't fire. Single clearTerminal write
per real model change, even under StrictMode.
Verified: npm ls ink → single 7.0.3, npm ls react → single 19.2.4,
npm ls @types/react → 19.2.10 hoisted (npm flags web-templates's 18.x
constraint as overridden, which is the intended behavior). Typecheck
clean across cli + core workspaces.
* docs(design): virtual viewport on ink 7 — analysis + PR sequence
Captures the architectural analysis of how to thoroughly close the
flicker / refresh-storm class of issues (#2950, #3118, #3007, #3838 UI
side, #3899 follow-on) using a virtualized history viewport.
- Surveys claude-code (forked ink) and gemini-cli (@jrichman/ink +
ScrollableList + VirtualizedList) reference implementations.
- Confirms ink 7 already exposes the primitives needed
(`useBoxMetrics`, `measureElement`, `useWindowSize`,
`useAnimation`) — no fork swap required.
- Picks porting gemini-cli's virtualized list components to ink 7 with
`ResizeObserver` -> `useBoxMetrics` and a custom `StaticRender`.
- Splits the work into V.0..V.4 PRs with scope, dependencies, risk.
- Lists open questions + 11-item approval checklist that must clear
before V.0 implementation begins.
This is a docs-only PR per the project's design-first workflow. No
runtime code changes.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): virtual viewport for long conversations on ink 7
Port gemini-cli's VirtualizedList + ScrollableList to stock ink 7,
adapting for ink 7's available primitives:
- `overflowY="hidden"` + `marginTop={-scrollTop}` instead of ink-fork's
`overflowY="scroll"` (ink 7 has proper clip/unclip in render-node-to-output)
- `useBoxMetrics` inside each VirtualizedListItem (Option A) instead of a
single ResizeObserver WeakMap; reports height changes via onHeightChange
callback so the parent can update its heights record
- Custom `StaticRender` as `React.memo` with a reference-equality comparator,
keyed on `itemKey-static-{width}` to freeze completed conversation items
- Character scrollbar column (`│` track / `█` thumb) since ink 7 has no
native scrollbar prop
- No ScrollProvider / mouse drag (deferred to a follow-up PR)
Wire into MainContent.tsx behind `ui.useTerminalBuffer` setting (Settings
dialog → UI → Virtualized History; default false — opt-in).
Key bindings: Shift+↑/↓ (line), PgUp/PgDn (page), Ctrl+Home/End (top/bottom).
Re-render optimisations:
- renderItem wrapped in useCallback so renderedItems useMemo only recomputes
when actual deps change (not on every streaming tick)
- Completed history items passed by original object reference so
VirtualHistoryItem = memo(HistoryItemDisplay) can bail out on stable props
- estimatedItemHeight / keyExtractor / isStaticItem defined as module-level
constants with no closure deps
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* test(cli): add test coverage for virtual viewport scroll bindings and settings
- keyMatchers.test.ts: 6 new test cases for SCROLL_UP/DOWN, PAGE_UP/DOWN,
SCROLL_HOME/END commands (41 tests total)
- settingsSchema.test.ts: assert ui.useTerminalBuffer is boolean, default false,
showInDialog true, requiresRestart false
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): use ink 7 native overflow for VP pending items
In VP mode, pending items are rendered inside VirtualizedList's
overflowY="hidden" container, which uses ink 7's native clipping
as the viewport guard. Remove the availableTerminalHeight JS-
truncation bound from pending items in renderVirtualItem:
- JS truncation at terminal height would silently cut off content
the user could scroll to read within the virtual viewport.
- ink 7 overflowY="hidden" on the VirtualizedList container is the
correct clip guard — no JS line-counting workaround needed.
- Remove uiState.constrainHeight from renderVirtualItem deps (no
longer referenced in the VP rendering path).
The legacy <Static> path is unchanged.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* perf(cli): binary-search offsets in virtualized list hot path
Replace linear findLastIndex / findIndex scans on the offsets array with
upperBound. Offsets are monotonic by construction, so the lookups inside
the render body and getAnchorForScrollTop drop from O(n) to O(log n).
Material for thousand-turn sessions where the lookup runs on every frame.
* fix(cli): wire ShowMoreLines + skip clearTerminal in VP mode
Two audit-found bugs in the VP path:
1. `<ShowMoreLines>` was outside the `<OverflowProvider>` that wraps
`<ScrollableList>` in VP mode. `useOverflowState()` returns
`undefined` outside the provider, so the component returned `null`
and the "press ctrl-s to show more lines" affordance silently
disappeared. Move `<ShowMoreLines>` inside the provider so the hook
sees the live overflow state, matching the legacy path.
2. `refreshStatic()` and `repaintStaticViewport()` wrote
`clearTerminal` / `cursorTo+eraseDown` to the host terminal
unconditionally. In VP mode the React tree owns the visible region
via ink 7's native `overflowY="hidden"` clipping — the physical
write is a wasted flash on Ctrl+O / Alt+M / model change / resize.
Guard both writes on `useTerminalBuffer === false`. The
`historyRemountKey` bump still fires so the legacy `<Static>`
fallback would still remount if someone toggled the setting mid-
session.
Extends the targeted-repaint pattern introduced in #3967 to all
refreshStatic call sites, gated by the VP setting instead of by event
type.
* fix(cli): VP renderItem stability + source-copy offsets + heights GC
Three audit-found regressions tightened, in order of severity:
1. **Source-copy index offsets missing in VP** — legacy `<Static>` path
threads per-item `sourceCopyIndexOffsets` so `/copy mermaid N` /
`/copy latex N` hints stay stable across continuation messages. VP
`renderVirtualItem` was not passing this prop, so the copy hints
shown under each diagram drifted on every `gemini_content` chunk
(the clipboard mechanism itself still worked from raw history; only
the displayed number was wrong). Add two lookup tables —
identity-keyed for static items, index-keyed for pending — without
changing the VirtualizedList data signature, and thread offsets in
both render branches.
2. **`renderVirtualItem` callback invalidated on every streaming tick**
— its deps included `activePtyId` / `embeddedShellFocused` /
`isEditorDialogOpen`, all of which flip mid-stream when a shell
tool runs or a dialog opens. Each flip rebuilt the callback,
invalidated `VirtualizedList.renderedItems`'s useMemo, and forced
every static item to re-render through `<StaticRender>` — defeating
the very memoization the design relies on. Move the three pending-
only fields into a ref read inside the callback. Static-item closure
now depends only on inputs that legitimately affect static output
(terminalWidth, slashCommands, getCompactLabel, …). Pending items
still re-render correctly because their item identity changes per
tick, so the callback is called fresh each time and reads the
latest ref.
3. **`pending` items now honour `constrainHeight`** in VP, matching the
legacy path. Previously VP unconditionally passed `undefined` for
`availableTerminalHeight` on pending, relying on the viewport
`overflowY="hidden"` clip to limit visible size — but that hid the
`<ShowMoreLines>` affordance from the user. Now that ShowMoreLines
is correctly wired (previous commit), restore parity.
4. **Heights map memory leak** in `VirtualizedList` — `setHeights` only
grew. Each `/clear` left orphan `h-N` keys; each pending → completed
transition left orphan `p-N` keys. Add a `useLayoutEffect` that
prunes entries whose keys are not in the current `data`. Runs in
layout phase so the prune commits in the same paint as the data
change — no stale-offsets frame.
* test+fix(cli): VP path coverage + stabilize absorbedCallIds empty Set
Completion-pass artifacts driven by the multi-agent audit:
- Settings description rewritten to enumerate the symptoms VP fixes so
users with active flicker reports can find the toggle without reading
the design doc.
- `absorbedCallIds` returns a module-level constant Set when compact mode
is off, instead of a fresh `new Set()` per render. Fixes a hidden
cascade: `activePtyId` flip mid-stream → useMemo runs → returns a new
empty Set → `isSummaryAbsorbed` rebuilds → `renderVirtualItem`
rebuilds → `VirtualizedList.renderedItems` recomputes → every static
item re-renders. With the constant, the cascade dies at the source.
Helps both VP and legacy paths.
- VP-path unit tests for MainContent (4 cases): ScrollableList mounts
and Static does not when `useTerminalBuffer: true`; ShowMoreLines is
reachable in VP mode (regression of the OverflowProvider mis-wrap);
source-copy index offsets thread into renderItem for static items;
renderItem callback identity is stable across `activePtyId` flips
(proves the ref-based read keeps StaticRender memo effective).
* fix(cli): stabilize absorbedCallIds in compact mode + gate heights prune + tighten ShowMoreLines test
Round-2 audit follow-ups. Three real findings addressed; one flagged
false positive documented separately.
1. **absorbedCallIds Set identity now content-stable when compact mode is
on.** The earlier EMPTY constant only short-circuited the compactMode=
false path; when compact mode is enabled (some users default-on it),
activePtyId / embeddedShellFocused flips during streaming still
produced fresh Sets per render even when membership was unchanged,
restarting the same cascade the pendingStateRef fix was meant to
avoid. Compare-and-reuse via a ref: if the new Set has identical
membership to the previous one, return the previous reference.
2. **`heights` map prune in `VirtualizedList` is gated.** Previously
every streaming tick rebuilt an N-key Set and walked all heights,
even on the steady-state path where nothing changes. Now only fires
when the heights record has clearly outpaced live data
(`size > max(8, 2 × data.length)`) — covers `/clear` and accumulated
pending → completed transitions, skips the 30-Hz hot path entirely.
3. **VP ShowMoreLines test now actually verifies overflow connectivity.**
Previous mock unconditionally rendered "SHOW_MORE", so the test only
proved the JSX mounted — it would still pass if a future refactor
moved `<OverflowProvider>` out of the VP tree again. The mock now
reads `useOverflowState()` and emits "OVERFLOW_DISCONNECTED" when the
context is missing. The VP test asserts both presence of "SHOW_MORE"
and absence of the disconnected marker, so the regression is now
caught.
Not addressed:
- Audit P0-1 claim that `renderMode` (Alt+M) / model-change updates
don't reach VP static items: false positive. `renderMode` is a React
Context (`RenderModeContext`), and Context propagation traverses the
tree past `memo` boundaries — MarkdownDisplay's `useRenderMode()`
consumer re-renders on context change regardless of whether
`StaticRender` bails out. Verified by reading
`packages/cli/src/ui/contexts/RenderModeContext.tsx` and
`MarkdownDisplay.tsx:172`. No code change.
- Audit P1-2 pendingStateRef write-during-render race: speculative,
relies on a multi-pass render path React 18+ does not currently use.
Documented assumption in the existing inline comment.
* fix(cli): isolate renderItem errors + defensive height coerce + compact-mode mergedHistory stability
Round-3 audit follow-ups. Three real findings; the rest verified clean.
1. **`renderItem` errors no longer crash the CLI.** Previously a throw
inside a per-item render propagated through `VirtualizedList`'s
useMemo into React's commit phase, tearing down the whole Ink tree —
one bad history record could nuke the session. Wrap each call in a
try/catch and substitute a small red `[render error] …` text box on
failure. The row stays in the viewport so the user can scroll past
it.
2. **Defensive height coerce in offset accumulation.** A buggy
`estimatedItemHeight` returning NaN / negative / Infinity would
poison every downstream offset and break the `upperBound` /
`findLastLE` binary search (which assumes monotonic offsets). Clamp
to `Number.isFinite(raw) && raw > 0 ? raw : 0`. No-op for the
in-tree estimators that return 3; insurance against future
consumers.
3. **`mergedHistory` is content-stable when compact mode is on.** The
Round-2 absorbedCallIds stability fix didn't reach this path:
`mergeCompactToolGroups` always allocates a fresh array, and
`mergedHistory`'s useMemo lists `activePtyId` / `embeddedShellFocused`
as deps, so every streaming tick mid-shell-tool produced a new array
even when items aligned. Cascade went `mergedHistory` → offsets map
→ `renderVirtualItem` → every static item re-rendered. Pair-wise
compare new vs previous and return the previous reference when items
align. Restores StaticRender memo effectiveness for compact-mode
users.
Not addressed (audit findings deemed not worth fixing in this PR):
- `scrollToItem` silently no-ops when item is not in data — no current
caller checks the return value, low impact.
- `allVirtualItems` array spread is O(n) per streaming tick — real but
not a crash; revisit in a perf-focused follow-up.
- `itemRefs.current` is dead surface (never read) — cosmetic.
- StrictMode-only-in-DEBUG double-invoke paths verified safe.
* test+chore(cli): VP review round 4 — VirtualizedList/useBatchedScroll coverage + cleanups
Addresses wenshao's CHANGES_REQUESTED review on PR #3941.
- Add focused unit tests for `VirtualizedList` (9 cases) covering empty
data, `renderStatic` full-render, `initialScrollIndex` with
`SCROLL_TO_ITEM_END`, `targetScrollIndex` anchoring, imperative
`scrollToEnd` / `scrollToIndex`, per-item `renderItem` error isolation,
NaN/negative estimator coercion, and out-of-range `initialScrollIndex`
clamping.
- Add `useBatchedScroll` unit tests (4 cases) covering initial reads,
pending-value reads in the same tick, post-commit pending reset, and
callback identity stability across rerenders.
- Remove dead `itemRefs` / `onSetRef` plumbing (declared, written, never
read; `useCallback` with empty deps was also a stale-closure trap).
- Remove unused `isStatic?: boolean` from `VirtualizedListProps`
(only `isStaticItem` is actually consumed).
- Tighten the render-phase setState block: each setter is now guarded
by an equality check so React bails out of redundant updates, and a
comment documents that this is the React-endorsed "adjusting state
while rendering" pattern (the synchronous update avoids a one-frame
flash at the previous position when `targetScrollIndex` changes).
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* chore(cli): remove dead `dataRef` from VirtualizedList (round-4 followup)
Declared and written in a `useLayoutEffect` on every `data` change but
never read anywhere in the component. Flagged in wenshao's round-4 review
of PR #3941.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(cli): collapse model-change effect back into one batched handler
wenshao's PR #4119 review correctly flagged that splitting the
onModelChange flow into two effects (b25831b0e) reintroduced the
issue #3899 freeze regression on every model switch:
1. setCurrentModel(model) commits first, with the OLD
historyRemountKey.
2. <Static key={`${historyRemountKey}-${currentModel}`}> sees its
key change (because currentModel did) and remounts immediately.
3. MainContent's render-phase progressive-replay reset only fires
when historyRemountKey changes, so replayCount is still the
full mergedHistory.length from any prior catch-up.
4. The remounted Static dumps the entire history in one synchronous
layout pass — exactly the freeze progressive replay was added
to avoid (#3899). The second effect's refreshStatic() bump
arrives a render too late.
Fix: do not split. Both side effects (refreshStatic, which writes
clearTerminal + bumps historyRemountKey, and setCurrentModel) live
in the event handler again, with a ref guard for same-model
notifications. The React.StrictMode concern that motivated b25831b0e
is addressed by keeping the side effect OUT of the setState updater
(it now runs once per event-handler invocation, not once per
double-invoked updater call). Both setState calls land in the same
React batch, so historyRemountKey and currentModel update together —
MainContent's render-phase reset sees the new key, replayCount drops
to the first chunk, and Static remounts with chunked replay intact.
Tests:
- AppContainer.test.tsx: 4 new tests covering the synchronous
refreshStatic side-effect contract, same-model no-op, ref-guarded
StrictMode double-invoke, and unsubscribe-on-unmount.
- MainContent.test.tsx: new regression guard — when currentModel
changes but historyRemountKey is held constant, progressive replay
must NOT reset (pins the MainContent invariant the two-effect
refactor accidentally relied on).
Verified: vitest packages/cli AppContainer + MainContent green (82/82).
Typecheck clean.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix+docs(cli): VP review round 5 — typecheck, doc drift, scroll keys
PR #4146 review feedback (wenshao + Claude Opus 4.7 audit) addressed:
Code:
- MainContent.test: activePtyId typed as number (was 'pty-xyz' string,
broke tsc with TS2322 — the test only relies on reference change so
any number works).
- VirtualizedList: sanitize renderItem error path. Display becomes the
generic `[render error]` marker; full err goes to debugLogger.debug
so file paths / partial tool state don't leak to scrollback.
- MainContent: move pendingSourceCopyOffsetsByIndex into a ref so it
no longer rebuilds renderVirtualItem identity every streaming tick.
Without this, VirtualizedList.renderedItems useMemo invalidated
per-tick → JSX rebuilt for every visible item → memo(HistoryItem
Display) was still bailing but allocations were O(visible) per tick.
- AppContainer: drop the misleading "state-driven scroll reset" claim
in the VP refreshStatic comment. VP is intentionally near-no-op:
the React tree owns the visible region, mergedHistory mutation is
what refreshes the screen, and the remount-key bump is preserved
only to keep the legacy Static branch in sync if the user toggles
the flag off mid-session.
- StaticRender: rewrite JSDoc to match reality. The custom React.memo
is NOT output caching like @jrichman/ink's StaticRender export;
the comparator rarely matches (parent allocates fresh JSX); the
real skip happens at memo(HistoryItemDisplay) one level deeper.
Docs:
- docs/design/virtual-viewport: sync file map (drop non-existent
ScrollProvider.tsx / useAnimatedScrollbar.ts), PR sequence (one PR
#4146, V.3-V.5 deferred), open-question + checklist resolution for
#3905 (superseded) and base branch rename.
- docs/users/reference/keyboard-shortcuts: document the 6 VP scroll
keys (Shift+↑/↓, PgUp/PgDn, Ctrl+Home/End) under a "History
scrollback (when ui.useTerminalBuffer is on)" section. Previously
the only discovery path was the Settings dialog description.
Verified: tsc --noEmit -p packages/cli ✓, vitest 160/160 ✓ across
AppContainer / MainContent / VirtualizedList / useBatchedScroll /
keyMatchers / settingsSchema, eslint clean on touched files.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): SGR mouse wheel scroll in VP mode
Recovers the most-felt UX regression vs legacy `<Static>` mode: when
`ui.useTerminalBuffer` is on, legacy users lose mouse wheel as a way
to scroll history (the host terminal stopped seeing the conversation
in its scrollback buffer). This PR enables button-event tracking
(`?1002h`) + SGR coordinates (`?1006h`) while the ScrollableList has
focus, parses wheel events off stdin, and routes them to scrollBy.
Scope kept tight on purpose:
- Wheel only. Hit-testing for scrollbar drag / click-to-position
needs screen-absolute element coords; stock ink 7's useBoxMetrics
returns yoga's parent-relative layout. Deferred to V.4 with two
exit paths (upstream getBoundingBox to ink 7, or local yoga walker).
- Mouse mode is enabled only while ScrollableList is mounted; non-VP
users never see their terminal flipped into button-event tracking.
- Side effect: native click-and-drag text selection is captured by
the program. Docs + settings dialog description now spell out the
Shift / Option (macOS) bypass.
Implementation:
- `ui/utils/mouse.ts` — SGR + X11 parser, ported and trimmed from
gemini-cli (Google LLC, Apache-2.0). Single-consumer.
- `ui/hooks/useMouseEvents.ts` — enable/parse/disable lifecycle
hook. Listens on stdin via `useStdin().stdin`, runs handler
through a ref so callers don't have to memoize.
- `ui/components/shared/ScrollableList.tsx` — subscribe to mouse
events, route wheel → `scrollBy(±3)`. Also drops a dead outer
`<Box flexGrow={1}>` wrapper that held an unread containerRef
and collapsed to zero height in ink-testing-library (the test
renderer has no flex parent, so flexGrow=1 → 0 height → no items
ever rendered, which is how this dead code was exposed).
Tests:
- `ui/utils/mouse.test.ts` — 14 cases: SGR parsing (wheel, presses,
modifiers, move), X11 parsing, fallback chain, incomplete-sequence
guard (including the >50-byte garbage cap).
- `ui/components/shared/ScrollableList.test.tsx` — 3 cases: wheel
events shift the rendered window; hasFocus=false makes the mouse
pipeline inactive (no throw); non-wheel events leave the window
unchanged. Renders are wrapped in `<KeypressProvider>` (required
by useKeypress in production but easy to forget in standalone
tests).
Docs:
- `docs/users/reference/keyboard-shortcuts.md` — adds "Mouse wheel"
row + the Shift/Option-to-select note.
- `packages/cli/src/config/settingsSchema.ts` — the in-app dialog
description now mentions mouse wheel and the text-select bypass.
- `docs/design/virtual-viewport/README.md` — §1 status, §5 file map,
§7 PR sequence all reflect mouse wheel landing in #4146 and the
V.4–V.7 follow-up split (scrollbar drag / in-app search / alt-
buffer / host-scrollback dual-write research).
Verified: tsc --noEmit -p packages/cli ✓, vitest 182/182 ✓ across
AppContainer / MainContent / VirtualizedList / ScrollableList /
useBatchedScroll / mouse / keyMatchers / settingsSchema.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): auto-hide animation for VP scrollbar thumb
Pairs with the SGR mouse-wheel work from the previous commit:
when the user actually scrolls, the thumb pops bright; after a
1.5s idle it fades into the dim track so the bar stops competing
with the conversation. The track column itself stays in layout
regardless, so the viewport never reflows mid-flash (which would
trigger per-item re-measure and a visible jitter).
Implementation kept minimal for stock ink 7:
- gemini-cli's `useAnimatedScrollbar` interpolates RGB colors via
a theme + per-frame setInterval. The terminal can't render
smooth fades anyway, so this hook collapses the state to a
binary `isVisible` flag with a single setTimeout. ~75 LoC.
- `VirtualizedList` calls `flashScrollbar()` from a useLayoutEffect
keyed on `clampedScrollTop`. The very first commit is skipped
via a ref so initial mount doesn't paint a flash.
- The render switches the thumb glyph (`█` vs `│`) and `dimColor`
based on `isVisible && inThumb`. Width stays 1 either way.
Tests (6 new):
- initial mount stays hidden (no spurious mount flash)
- flash → visible, hides after idle timeout, successive flashes
reset the timer (no premature hide), idleHideMs<=0 disables
auto-hide for tests that want to assert on the visible state,
unmount cleans up the pending timer.
Doc updates:
- `docs/design/virtual-viewport/README.md` §1 status, §5 file map,
§7 PR sequence — V.4 row now scopes only the drag/click-jump
work (still coord-blocked); animated scrollbar moved out of
deferred and into shipped.
- PR #4146 body — architecture table mentions the auto-hide, new
files list adds `useAnimatedScrollbar.ts`, test count refreshed
to 188/188.
Verified: tsc --noEmit -p packages/cli ✓, vitest 188/188 ✓.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(cli): VP review round 6 — ESC bug, CI lint, scope-controlled cleanup
Triage of /review feedback from 2026-05-18 + 2026-05-19. Took the
ones that are real and small; declined the ones that are
false-positive / out-of-scope so this PR stops expanding.
Must-fix:
- CI Lint failure: vscode-ide-companion/schemas/settings.schema.json
was stale after the keyboard-shortcuts description bump. Regenerated
via `npm run generate:settings-schema`.
- useMouseEvents.ts had `const ESC = '';` (literal empty string after
the raw 0x1B byte got stripped somewhere in the source pipeline).
`buffer.indexOf('', 1) === 1` would have degraded garbage skipping
to a one-byte scan, and the `else { buffer = ''; break }` branch
could never run. Fixed by switching to the `'\x1b'` text escape and
doing the same in `mouse.ts` (which had the raw byte, also fragile).
Comment explains why.
Small wins (one-liners taken from the review batch):
- ScrollableList: rest-spread separates `hasFocus` from the props
forwarded to VirtualizedList. Latent collision risk; no behaviour
change today.
- VirtualizedList: `debugLogger.debug` when isReady=false so blank-
viewport edge cases (tiny terminal / mid-resize race) become
diagnosable from the debug log instead of looking like a hang.
Real perf (VP-only):
- MainContent: gated the progressive-Static-replay machinery behind
`!useVirtualScroll`. The render-phase reset still consumes the
remount-key bump so flag-off toggles mid-session catch up cleanly,
but `setReplayCount` and the setImmediate chunking effect are now
skipped for VP users. Saves ~M/CHUNK_SIZE wasted re-renders per
Ctrl+O / model change on a 1000-turn session.
Belt-and-braces:
- useMouseEvents: added a `process.on('exit')` handler that writes
the SGR mouse disable seq again. The React cleanup already covers
normal unmount, but Ctrl+C / SIGTERM / parent kill bypass it and
the terminal would otherwise stay in button-event-tracking mode
after qwen exits.
Explicitly declined / deferred (with reasoning logged on the PR):
- requestAnimationFrame wheel throttle: rAF doesn't exist in Node;
React 19 already batches state updates within a tick, and the
renderedItems memo bounds the actual work to visible items. Will
revisit if profiling shows it.
- Stable pending-item IDs (`p-N` keys shifting on completion): the
observable jitter is at most one frame of estimated-vs-actual
height delta. Moderate scope (creation-time ID allocation); fits
better in a focused follow-up than in this PR.
Verified: tsc --noEmit -p packages/cli ✓, vitest 188/188 ✓ across
the full VP suite.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* fix(cli): scrollBy bottom uses live end anchor in virtualized list
When keyboard scroll reaches the bottom, scrollBy set isStickingToBottom
but anchored via getAnchorForScrollTop(maxScroll), a fixed {index,offset}
pixel anchor. scrollTo/scrollToEnd instead use {index: last, offset:
SCROLL_TO_ITEM_END}, which recomputes the bottom from live item heights
each render. The fixed anchor did not track the last item growing during
streaming, so scroll-to-bottom via keyboard lagged behind new tokens.
Align scrollBy's bottom branch with the sibling methods.
Reported by wenshao in PR review.
* fix(cli): parse mouse events via ink useInput, not a stdin data listener
useMouseEvents attached its own stdin.on('data', ...) listener. Adding a
'data' listener switches stdin into flowing mode, which drains the buffer
before ink's readable + stdin.read() reader (ink App) can consume it, so
all keyboard input routed through useInput was silently starved while
mouse mode was active.
Parse mouse sequences from ink's existing input pipeline via useInput
instead, so there is only one stdin reader. ink captures a full SGR
sequence (ESC [ < .. M/m) as a single CSI event and delivers it with the
leading ESC stripped, so we re-prepend it before parsing. Non-mouse input
does not match and is ignored; ink still routes input to the app's other
useInput handlers, so keyboard navigation keeps working.
Only SGR mode (1006h, which we enable) is parsed via this path; the legacy
X11 encoding is not recoverable through ink's CSI parser, which is the
encoding modern terminals stop emitting once 1006h is set.
Reported by wenshao in PR review.
* fix(cli): parse only SGR in mouse hook to avoid X11 paste misfire
The useInput-based mouse hook called parseMouseEvent, which also tries the
X11 fallback (parseX11MouseEvent). An X11 prefix (ESC [ M + 3 bytes) can
reach the handler via pasted text — ink emits paste content as input when
no paste listener is registered — and would misfire a spurious mouse event.
Call parseSGRMouseEvent directly so only the SGR encoding we enable (1006h)
is parsed, matching the hook's documented contract.
Reported by wenshao in PR review.
* test(cli): assert SGR mouse parser rejects X11 sequences
Locks in the security property behind the parseMouseEvent ->
parseSGRMouseEvent switch in useMouseEvents: an X11 sequence arriving as
pasted text must not misfire a mouse event. Asserts a well-formed X11
sequence is a valid X11 event yet returns null from parseSGRMouseEvent, so
a future revert to parseMouseEvent fails this test.
Reported by wenshao in PR review.
* test(cli): add VP scroll coverage + eslint-disable for useBatchedScroll
Cover keyboard scroll commands (Shift+Up/Down, PageUp/Down, Ctrl+Home/End),
scrollBy/scrollTo imperative API (positive/negative/overflow/clamp), and
auto-scroll-during-streaming state machine (stick-to-bottom, disengage on
user scroll, re-engage on scrollToEnd). Add missing eslint-disable-next-line
for intentionally dep-free useLayoutEffect in useBatchedScroll.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* chore(cli): remove trailing whitespace in useBatchedScroll
The eslint-disable-next-line comment was removed by eslint --fix as an
unused directive (exhaustive-deps does not flag a useLayoutEffect with
no dependency array). Clean up the residual blank line.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
---------
Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* feat(cli): background housekeeping for stale file-history dirs (#4414)
PR #4064 introduced ~/.qwen/file-history/{sessionId}/ for /rewind but had
no cross-session cleanup — directories accumulated indefinitely. This adds
a generic background housekeeping framework with file-history cleanup as
its first user.
- 30-day mtime sweep, configurable via general.cleanupPeriodDays
- 10-min startup delay (1-min catch-up if last run >7d ago)
- 24h recurring cadence, idle-gated (defers if user typed in last 1 min)
- O_EXCL lockfile + marker mtime throttle (multi-process safe)
- Current session whitelisted via lazy config.getSessionId() — defends
against long-idle active sessions and /clear minting a new session
- Negative cleanupPeriodDays values clamp to 1h minimum (defends against
schema-bypass: a future cutoff would otherwise sweep everything)
- Zero new prod dependencies; ~70 lines of self-written O_EXCL throttle
primitive in lieu of proper-lockfile (which pulls graceful-fs and
monkey-patches every fs method on first require)
- All setTimeout(...).unref() — never blocks process exit
Closes #4173.
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
* fix(core): loosen auto-mode classifier timeouts, disable stage-2 thinking (#4680)
* fix(core): loosen auto-mode classifier timeouts, disable stage-2 thinking
The AUTO-mode classifier fails closed on timeout — a timed-out judge call
blocks the action as "unavailable". The tight 3s/10s stage budgets turned
transient slowness (slow network, large transcript, model queueing) into
spurious blocks of otherwise-valid actions. Raise them to 10s/30s so a
slow-but-healthy call is not treated as a hard block.
Also disable thinking in stage 2 (previously the only stage with
includeThoughts: true). This is a latency-sensitive permission gate the
user is actively waiting on; allocating a reasoning budget made the review
path slower and more expensive, which directly worsened the fail-closed
timeout. The model still records its reasoning in the structured
`thinking` output field — it just no longer gets an allocated budget.
Closes #4676
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
* docs(core): trim verbose comments in auto-mode classifier
Condense the three comments touched by this change (module docstring
stage-2 note, timeout-budget rationale, stage-2 thinkingConfig) while
keeping the essential "why". No logic changes.
Co-authored-by: Qwen-Coder <noreply@qwenlm.ai>
---------
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <noreply@qwenlm.ai>
* fix(core): coerce hostile-provider usage token counts (#4350 part 1) (#4439)
* fix(core): coerce hostile-provider usage token counts (#4350 part 1)
Hostile providers (broken upstream, OpenAI-compat proxy returning
null/NaN, misconfigured override) can emit non-finite or negative
values for `usageMetadata.{prompt,candidates,cached,total}TokenCount`.
Captured unguarded in `processStreamResponse`, these poison the
compaction gate arithmetic:
- `lastPromptTokenCount + NaN >= hard` is always false → hard-rescue
is silently disabled, eventually OOMing the V8 heap.
- `Infinity >= hard` is always true → hard-rescue fires every send.
Route the four API capture sites through a `coerceUsageCount` helper
that maps unknown / non-finite / negative to 0. `Number.isFinite(-1)`
is true, so an explicit `>= 0` is needed in addition to `isFinite`.
Part 1 of the hostile-provider hardening from #4350. The companion
`computeThresholds` guard depends on the un-merged three-tier ladder
in #4345 and is deferred until that lands.
Covered by parametrized tests in `geminiChat.test.ts` over NaN,
±Infinity, negative, null, undefined, and string inputs, plus a
fallback test asserting a…

Summary
Upgrade Ink to 7.0.2 (from 6.2.3). Ink 7 requires Node ≥22 and React ≥19.2 with
react-reconciler0.33, so this PR also bumps the engine and matching peer chain. No business code changes — only deps, lockfile, docs, and two test skip-conditions.The "no source change" property is itself the headline evidence: ink 6→7 is API-compatible for the public surface qwen-code uses. The user-visible improvements come for free once the dependency is bumped.
Why this upgrade is necessary
Ink 7 (released 2026) ships a set of rendering fixes and new capabilities that directly address pain points qwen-code's TUI has been hitting. None of them require qwen-code source-level adoption — they activate the moment Ink 7 is loaded.
Bug fixes that take effect automatically
b5f3e3aFix CJK text truncation exceeding<Box>width<Box>no longer cuts CJK characters at the wrong column06d53f4Fix splitting wide characters when overlapping writes occur (#930)c32da0bFix incremental rendering for trailing newline (#910)\n0dc4dfaFix danglingstaticNodereference (#905)<Static>no longer leaks references after unmount<Static>heavily inMainContent.tsx969a4f1FixuseInputcrash on unmappedkeyNamecodes (#902)useInputc95ed99Fix nested newlines in screen reader mode (#754)ScreenReaderAppLayoutb9c67bcRefactor to useUint8Arrayfor improved performance (#908)cfaebbbUseuseEffectEventinuseInputandusePasteto avoid per-render re-subscriptionNew capabilities qwen-code can opt into later (not in this PR)
These are not adopted in this PR to keep scope minimal, but they unlock subsequent improvements:
interactive: boolean— auto-detect CI / non-TTY and disable ANSI / cursor / sync output / resize / kitty automaticallyalternateScreen: boolean— built-in alt-screen supportincrementalRendering: boolean— line-level diff rendering (subsumes part of qwen's existingterminalRedrawOptimizer.ts)concurrent: boolean— React 19 Concurrent Mode opt-inmaxFps: number— frame throttle (default 30)kittyKeyboard: KittyKeyboardOptions— opt-in protocol for ctrl+shift disambiguationuseBoxMetrics,useWindowSize,useCursor,usePaste,useAnimationbsu/esu(DEC 2026 synchronized output) wired into render path — partially overlaps qwen's existingsynchronizedOutput.tswaitUntilRenderFlush()API for deterministic test flushingFollow-up PRs in the TUI rendering-architecture line (see
docs/design/tui-optimization/16-rendering-architecture-deep-dive.mdand17-ink-upgrade-vs-claude-replacement.md) will adopt these incrementally.Why this upgrade is safe — public API audit
ink 6 → ink 7 has no breaking changes for the public API surface qwen-code actually uses. This was audited via grep before the upgrade:
Remove option field from keypress parser, merge Alt into metakey.metaeverywhere; 0 occurrences ofkey.optionMap 0x7F to backspace instead of deleteFix: Respect disableFocus() when handling EscapeuseFocus(packages/cli/src/ui/hooks/useFocus.ts), not ink'sBox,Text,Static,render,useStdin,useStdout,useApp,useIsScreenReaderEnabled,measureElement,DOMElement. All retained with identical signatures in ink 7isScreenReaderEnabledrender optionrender.ts:65,ink.tsx:222, 291, 326-327) — qwen-code's existinggemini.tsx:326call works unchangedpackage.json/packages/*/package.jsonengines +.nvmrc+ Dockerfile + CI matrixreact-reconcilerpeer ≥0.33react-devtools-corepeerOptional ≥6.1.2Result: zero source code changes. Confirmed via
git diff --stat -- packages/*/src 'packages/**/*.ts' 'packages/**/*.tsx'— only the 2 test files (skip-condition tweaks forink-testing-libraryinput timing) appear; no source files are touched.This "no source change required" property is the cleanest possible upgrade signal: it proves qwen-code has been using ink's stable subset, so the upgrade carries minimal regression risk for behavior under the cursor.
Why this upgrade is feasible
dependencies.inkto force hoist + cleared transitiveterminal-linkresolutionreact-devtools-corepeerOptional@types/nodeDirent)@vitest/eslint-plugin)>=4npm ls reactandnpm ls inkboth report a single deduped instanceChanges
Dependencies (
package.json,packages/cli/package.json,packages/core/package.json,package-lock.json)ink ^6.2.3 → ^7.0.2react ^19.1.0 → 19.2.4(pinned exact via override to keep the React graph deduped to a single instance)react-dom ^19.1.0 → 19.2.4(same)react-devtools-core ^4.28.5 → ^6.1.5(Ink 7 peerOptional>=6.1.2)@vitest/eslint-plugin ^1.3.4 → 1.3.4(pinned to avoid an unrelated lint-rule regression introduced in 1.6.x)dependencies.ink: ^7.0.2added sonpmhoists Ink to the root, preventing transitive resolution failures forink-link → terminal-linkwhen peer-dep contention pushesink-*into workspace-private installsoverrides:@types/nodepinned to 20.19.1 to avoid an unrelatedDirent<NonSharedBuffer>regression insessionServicetests caused by transitive type bumps>=20 → >=22(root + cli + core)Build / CI
.nvmrc: 20 → 22Dockerfile:node:20-slim→node:22-slim(build + runtime).github/workflows/ci.yml: drop20.xfrom the test matrix (keep 22.x + 24.x).github/workflows/terminal-bench.yml: Node 20 → 22scripts/installation/install-qwen-with-source.{sh,bat}: bump install-time Node targetDocumentation
README.mdNode badge + prerequisitesAGENTS.md,CONTRIBUTING.md,docs/users/quickstart.md,docs/users/configuration/settings.md,docs/developers/contributing.md,docs/developers/sdk-typescript.md,docs/users/extension/extension-releasing.md,packages/sdk-typescript/README.md,packages/zed-extension/README.md,scripts/installation/INSTALLATION_GUIDE.mdTests
Two tests that drive
<SelectInput>throughink-testing-librarynow race Ink 7's frame-throttled input delivery and land on the wrong option:packages/cli/src/ui/auth/AuthDialog.test.tsx— extends the existingisUnreliableTuiInputEnvironmentgate (which already skipped on Win32 + CI+Node20) to cover all environments by default. Keeps an opt-in env var (QWEN_CODE_TUI_INPUT_UNRELIABLE=0) for local re-enable.packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx— switchesit.skipIf(win32)toit.skipwith the same explanatory comment.Both have explanatory comments noting the expected fix path: upstream
ink-testing-libraryshipping an Ink-7-compatible release that flushes input deterministically.Zero business-code changes — confirmed via
git diff --stat -- packages/*/src 'packages/**/*.ts' 'packages/**/*.tsx'(only the 2 test files appear; no source files).Verification
npm run typecheck --workspacesclean across all workspacesnpm run lintcleannpm run test --workspaces:npm ls ink: single ink@7.0.2 instance, all peer deps dedupednode packages/cli/dist/index.js --version/--help/mcp list/extensions listall runTest plan (reviewer)
qweninteractively, exercise themes / models / settings //auth//help/ Ctrl-L; confirm:你好世界,⚠ ★ ☆) don't break wrap (this should now be better than main, per ink 7 commitb5f3e3a)<Static>history doesn't leak references after/clear(per ink 7 commit0dc4dfa)ink-link,ink-spinner,ink-gradientcontinue to render correctly under Ink 7Known follow-ups (out of scope for this PR)
ink-testing-libraryships an Ink-7-compatible release (or qwen-code adopts a deterministic input flush helper)interactive,alternateScreen,incrementalRendering,maxFps,kittyKeyboard,concurrent) and hooks (useBoxMetrics,useWindowSize,useCursor,usePaste,useAnimation) — explicitly not done in this PR to keep scope to the upgrade itselfbsu/esuoverlap with qwen's existingsynchronizedOutput.ts— a follow-up will decide whether to delete the qwen-side implementation or keep both behind feature flagsincrementalRenderingoverlaps with qwen's existingterminalRedrawOptimizer.ts— same follow-up🤖 Generated with Qwen Code