feat(core): add simplify bundled skill#3570
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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>
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] Documentation gap — not in this PR's diff, so flagged here rather than inline.
docs/users/features/commands.md has a "Built-in Skills" table (lines 86-88) that enumerates /review, /loop, and /qc-helper. This PR adds a new bundled skill /simplify but does not add a matching row, so the new skill will ship invisible to users browsing the command reference.
Suggested addition to the table:
| `/simplify` | Review recent changes for reuse, quality, efficiency, then apply safe cleanup | `/simplify`, `/simplify focus on duplication` |
— claude-opus-4-7 via Claude Code /qreview
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
已处理 wenshao 的两个 review 点:
|
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
left a comment
There was a problem hiding this comment.
All three review items are addressed: task → agent in both allowedTools and Step 2, and the isBundled path check no longer excludes .qwen paths. The latest commit also fixes the test mock to return simplify only from the bundled directory, keeping project/user level mocks clean.
The skill itself is well-structured — scoped diff detection, parallel review passes, apply-then-verify workflow. LGTM.
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
| --- | ||
| name: simplify | ||
| description: Review recent code changes for reuse, code quality, and efficiency, then directly apply straightforward cleanup improvements. Use when the user wants a post-implementation cleanup pass, pre-PR polish, or asks to simplify/refine recent changes. Invoke with `/simplify` or `/simplify <focus>`. | ||
| argument-hint: '[focus]' |
There was a problem hiding this comment.
[Suggestion] The new argument-hint frontmatter is not parsed or propagated anywhere. SkillConfig has no argument hint field, the skill parsers ignore this key, and the skill command loaders do not set SlashCommand.argumentHint, so /simplify will still not expose an argument hint through slash command metadata/completion. If this PR intends to add the visible hint, parse argument-hint into SkillConfig, validate it as an optional string, and pass it through as argumentHint.
— gpt-5.5 via Qwen Code /review
|
|
||
| Use `git diff HEAD` whenever staged changes exist. Otherwise use `git diff`. | ||
|
|
||
| ## Step 2: Launch three review passes in parallel |
There was a problem hiding this comment.
[Suggestion] Step 2 launches 3 parallel sub-agents but provides no error handling for agent failures (timeout, crash, tool rejection). If an agent silently fails, the LLM may hang, skip findings, or hallucinate results with no diagnostic output. The /review skill provides a template for handling this scenario.
| ## Step 2: Launch three review passes in parallel | |
| Use the `agent` tool and launch all review passes in a single response so they run concurrently. Each pass must receive the same review scope and diff command. | |
| If any single pass fails (timeout, tool rejection, no output), log a warning and continue with remaining passes. If all passes fail, stop and tell the user: "All review passes failed — cannot produce simplification suggestions." |
— deepseek-v4-pro via Qwen Code /review
|
|
||
| Do not force a cleanup if it would require speculative architectural changes. | ||
|
|
||
| ## Step 4: Apply straightforward improvements |
There was a problem hiding this comment.
[Suggestion] Step 4 directly modifies files via write_file/edit without instructing the agent to check for uncommitted changes, create a backup (git stash), or provide a rollback path when Step 5 verification fails. If the user has dirty working-tree changes, the agent's edits become intermixed.
| ## Step 4: Apply straightforward improvements | |
| Directly implement safe cleanup improvements. | |
| Before editing any file, check `git status --porcelain`. If there are uncommitted tracked changes, run `git stash push -m "simplify: pre-cleanup backup"` to preserve them. | |
| Examples of good automatic fixes: |
— deepseek-v4-pro via Qwen Code /review
|
|
||
| Skip items that are uncertain, risky, or too invasive. Do not spend time debating rejected findings; simply move on. | ||
|
|
||
| ## Step 5: Verify the cleanup |
There was a problem hiding this comment.
[Suggestion] Step 5 instructs the agent to run tests/typecheck/lint after cleanup but does not specify what to do when verification fails — revert the changes, fix and retry, or just report to the user. This leaves the agent's behavior unpredictable on failure.
| ## Step 5: Verify the cleanup | |
| After making changes: | |
| 1. Run focused tests for the changed area when they exist. | |
| 2. Run the relevant project quality checks you can identify for the touched code. | |
| 3. If there are no applicable tests, at least run a targeted build, typecheck, or lint command that covers the edited files. | |
| If any verification step fails, revert all changes with `git checkout .` (or `git stash pop` if you stashed earlier), and report the failure to the user. Do not attempt to fix verification failures. |
— deepseek-v4-pro via Qwen Code /review
Review Comment AnalysisReviewed all 7 comments on this PR. Here's the assessment: Comment 1 —
|
| # | Severity | Verdict |
|---|---|---|
| 1 | Critical | ✅ Fixed |
| 3 | Critical | ✅ Fixed |
| 4 | Suggestion | ✅ Already handled |
| 5 | Suggestion | 📋 Follow-up, not a merge blocker |
| 6 | Suggestion | ❌ Reject — prompts don't need programmatic error handling |
| 7 | Suggestion | ❌ Reject — contradicts the skill's use case |
| 8 | Suggestion | ❌ Reject — auto git checkout . risks data loss |
The two Critical issues are resolved. Comment 5 is a valid enhancement for later. Comments 6/7/8 are over-defensive suggestions from AI review tools (gpt-5.5 / deepseek-v4-pro) that don't fit the prompt-as-code paradigm or Qwen Code's safety model.
🇨🇳 中文版
Review Comment 分析
审查了本 PR 上的全部 7 条评论,以下是评估结果:
Comment 1 — task → agent 工具命名 (Critical) ✅ 已修复
观察正确——运行时暴露的可调用工具是 agent,而非 task。已在 commit 17c5f7c 中修复。
Comment 3 — isBundled 路径判断问题 (Critical) ✅ 已修复
移除 !pathStr.includes('.qwen') 条件是正确的做法。改为 pathStr.endsWith(bundledDirSegment) 的修复已在 diff 中。
Comment 4 — argument-hint frontmatter (Suggestion) ✅ 已处理
argument-hint: '[focus]' 元数据已存在于当前 SKILL.md 中。
Comment 5 — argument-hint 未被 SkillConfig 解析 (Suggestion) ⚠️ 后续处理
观察正确——argument-hint 存在于 frontmatter 中,但 SkillConfig 没有解析它,因此 slash command 补全不会展示该提示。但这属于增强功能,而非 bug。当前 frontmatter 起文档作用即可,解析和补全支持可作为后续 PR 处理。不阻塞合并。
Comment 6 — 子 agent 错误处理 (Suggestion) ❌ 拒绝
SKILL.md 是给 LLM 的 prompt 指令,不是应用程序代码。agent 框架已在基础设施层面处理超时和异常。在 prompt 中添加冗长的错误处理分支会增加 token 消耗,并稀释核心指令的清晰度。/review skill 也没有这样做。
Comment 7 — 编辑前 git stash (Suggestion) ❌ 拒绝
这与 skill 的核心使用场景矛盾。/simplify 的设计目的就是对用户未提交的工作区变更进行 cleanup——stash 反而会把需要处理的代码藏起来。此外,Qwen Code 的 agent 编辑支持用户逐个撤销。
Comment 8 — 验证失败时自动回滚 (Suggestion) ❌ 拒绝
git checkout . 会丢弃所有未提交更改,包括与 /simplify 无关的工作。自动执行破坏性 git 操作违反 Qwen Code 的安全准则。验证失败时正确的做法是报告给用户由其决定,而不是静默销毁工作。
总结
| # | 严重程度 | 结论 |
|---|---|---|
| 1 | Critical | ✅ 已修复 |
| 3 | Critical | ✅ 已修复 |
| 4 | Suggestion | ✅ 已处理 |
| 5 | Suggestion | 📋 后续处理,不阻塞合并 |
| 6 | Suggestion | ❌ 拒绝——prompt 不需要程序式错误处理 |
| 7 | Suggestion | ❌ 拒绝——与 skill 使用场景矛盾 |
| 8 | Suggestion | ❌ 拒绝——自动 git checkout . 有数据丢失风险 |
两个 Critical 问题已解决。Comment 5 是合理的后续增强。Comment 6/7/8 是 AI review 工具(gpt-5.5 / deepseek-v4-pro)生成的过度防御性建议,不适合 prompt-as-code 范式和 Qwen Code 的安全模型。
| ## Step 2: Launch three review passes in parallel | ||
|
|
||
| Use the `agent` tool and launch all review passes in a single response so they run concurrently. Each pass must receive the same review scope and diff command. | ||
|
|
There was a problem hiding this comment.
[Critical] Sub-agents launched in Step 2 inherit full tool permissions (write_file, edit, run_shell_command) with no read-only constraint.
The agent tool calls in Step 2 do not specify subagent_type and do not instruct sub-agents to be read-only. This means 3 parallel "review" passes could accidentally modify files concurrently during the review phase — causing race-condition corruption, stale data for aggregation, and duplicate edits when the main agent applies fixes in Step 4.
| Use the `agent` tool with `subagent_type: "general-purpose"` and launch all review passes in a single response so they run concurrently. Each pass must receive the same review scope and diff command. | |
| In each pass prompt, add: "You are a read-only reviewer. Do NOT modify any files. Only inspect, read, and report findings." |
— deepseek-v4-pro via Qwen Code /review
| 3. Otherwise review the current uncommitted diff. | ||
| 4. If there is no git diff, fall back to recently modified files in the working tree. | ||
| 5. If recently modified files are not obvious, fall back to files edited in this conversation. | ||
| 6. If you still cannot identify a meaningful scope, stop and tell the user there are no recent changes to simplify. |
There was a problem hiding this comment.
[Suggestion] Step 1 includes staged changes in the auto-cleanup scope: "If there are staged changes, review against HEAD so both staged and unstaged tracked changes are included."
A user who carefully curated their staging via git add -p will have their staged commits silently modified by auto-cleanup. Consider either (a) limiting scope to unstaged changes only, or (b) warning the user when staged changes exist and asking for confirmation.
| 6. If you still cannot identify a meaningful scope, stop and tell the user there are no recent changes to simplify. | |
| 1. First inspect the current git state. | |
| 2. If there are staged changes, warn the user: "Staged changes detected. /simplify will review and may modify both staged and unstaged tracked changes. Continue?" and only proceed if the user confirms. | |
| 3. Otherwise review the current uncommitted diff. |
— deepseek-v4-pro via Qwen Code /review
| - reduce unnecessary state or wrapper code | ||
| - remove low-value comments | ||
| - align code with nearby conventions | ||
|
|
There was a problem hiding this comment.
[Suggestion] "remove dead or redundant code" is listed as an example of a safe automatic fix, but LLMs cannot reliably identify dead code without full call-graph visibility.
Code that has no callers within the diff scope may still be active elsewhere in the repository. This could lead to silent deletion of live code paths.
| - remove redundant code (confirmed by grep-searching the full repository for callers) | |
| - simplify conditionals or control flow |
— deepseek-v4-pro via Qwen Code /review
|
|
||
| expect(commands).toHaveLength(1); | ||
| expect(commands[0].name).toBe('simplify'); | ||
| expect(commands[0].description).toBe('Simplify recent changes'); |
There was a problem hiding this comment.
[Suggestion] The new simplify loading test is missing the listSkills call assertion that existing similar tests include.
Add after expect(commands[0].kind).toBe(CommandKind.SKILL);:
| expect(commands[0].description).toBe('Simplify recent changes'); | |
| expect(mockSkillManager.listSkills).toHaveBeenCalledWith({ | |
| level: 'bundled', | |
| }); |
— deepseek-v4-pro via Qwen Code /review
| @@ -714,8 +747,11 @@ Review content`); | |||
| const skills = await manager.listSkills({ force: true }); | |||
There was a problem hiding this comment.
[Suggestion] The cross-level priority tests ("should prioritize project-level over bundled" / "user-level over bundled") verify review deduplication but do not assert that simplify — which has no name conflict — still survives in results.
If a future change causes all bundled skills to be dropped whenever a project-level skill exists (rather than per-name deduplication), these tests would not catch the regression.
| const skills = await manager.listSkills({ force: true }); | |
| expect(skills.some((s) => s.name === 'simplify')).toBe(true); | |
| expect(skills.some((s) => s.name === 'review')).toBe(true); |
— deepseek-v4-pro via Qwen Code /review
| | ------------ | ------------------------------------------------------------------- | ------------------------------------------------- | | ||
| | `/review` | Review code changes with 5 parallel agents + deterministic analysis | `/review`, `/review 123`, `/review 123 --comment` | | ||
| | `/loop` | Run a prompt on a recurring schedule | `/loop 5m check the build` | | ||
| | `/simplify` | Review recent changes, then apply safe cleanup improvements | `/simplify`, `/simplify focus on duplication` | |
There was a problem hiding this comment.
[Suggestion] The /simplify table description says "Review recent changes, then apply safe cleanup improvements" but does not disclose that the skill uses parallel agents or auto-applies edits — unlike the /review row which explicitly states "with 5 parallel agents + deterministic analysis."
Users scanning the table may not realize /simplify is an auto-editing tool rather than a passive analysis tool.
| | `/simplify` | Review recent changes, then apply safe cleanup improvements | `/simplify`, `/simplify focus on duplication` | | |
| | `/simplify` | Review recent changes with 3 parallel agents, then apply safe cleanup improvements | `/simplify`, `/simplify focus on duplication` | |
— deepseek-v4-pro via Qwen Code /review
| 1. First inspect the current git state. | ||
| 2. If there are staged changes, review against `HEAD` so both staged and unstaged tracked changes are included. | ||
| 3. Otherwise review the current uncommitted diff. | ||
| 4. If there is no git diff, fall back to recently modified files in the working tree. |
There was a problem hiding this comment.
[Suggestion] Step 1's fallback chain (#4 and #5) tells the agent to pick up "recently modified files in the working tree" / "files edited in this conversation" with no scope filter. When no diff is present, an LLM will improvise — typically find . -mmin -60, ls -lt, or similar — and the result will include node_modules/, dist/, coverage/, generated .d.ts, build caches, and lockfiles whose mtimes are freshest right after npm install/tsc. Step 4 then auto-applies edits to those files. A user running /simplify after a build will see dist/cli.js or package-lock.json mangled. Recommend: replace the vague "recently modified files" fallback with a concrete gitignore-respecting command (e.g., git ls-files --modified --others --exclude-standard), and add an explicit exclusion list (node_modules/, dist/, build/, coverage/, *.lock/*-lock.*, files matching .gitignore).
— claude-opus-4-7 via Claude Code /qreview
| function setupReviewSkillMocks() { | ||
| vi.mocked(fs.access).mockResolvedValue(undefined); | ||
| vi.mocked(fs.readFile).mockResolvedValue(`--- | ||
| vi.mocked(fs.readFile).mockImplementation(async (filePath) => { |
There was a problem hiding this comment.
[Suggestion] The new fs.readFile.mockImplementation has an unconditional fall-through that returns the review YAML for any path not containing ${path.sep}simplify${path.sep}. There is no path-shape validation, no error default. When a 3rd bundled skill is added later (e.g., a qc-helper bundled variant or any new entry), a test using setupReviewSkillMocks will silently receive the review SKILL.md body for that skill's path read — yielding a false positive (skill loads with wrong content) rather than a clear failure.
Also: this mock pairs with the mockParseYaml.mockImplementation below, where the two branches must stay in lock-step (path-based for readFile, YAML-content-based for parseYaml). Drift between them produces confusing test failures.
Recommend: a table-driven registry, e.g.
| vi.mocked(fs.readFile).mockImplementation(async (filePath) => { | |
| vi.mocked(fs.readFile).mockImplementation(async (filePath) => { | |
| const pathStr = String(filePath); | |
| for (const [name, fixture] of bundledSkillFixtures) { | |
| if (pathStr.includes(`${path.sep}${name}${path.sep}`)) { | |
| return fixture.markdown; | |
| } | |
| } | |
| throw new Error(`Unexpected fs.readFile path in bundled-skill test: ${pathStr}`); | |
| }); |
with a single bundledSkillFixtures map that drives both readFile (markdown) and parseYaml (parsed object) from one source of truth.
— claude-opus-4-7 via Claude Code /qreview
| Review content`; | ||
| }); | ||
|
|
||
| mockParseYaml.mockImplementation((yamlString: string) => { |
There was a problem hiding this comment.
[Suggestion] setupReviewSkillMocks calls mockParseYaml.mockImplementation(...), which replaces the global dispatcher set up in beforeEach at line 72. The global impl has a hooks: branch (lines 74–76) that uses the real yaml.parse so frontmatter with hooks: parses correctly. The new bundled-skill impl drops that branch — any test that runs after setupReviewSkillMocks will mis-parse SKILL.md frontmatter containing hooks:. SkillManager.parseSkillContent (skill-manager.ts:434) re-parses for hooks, so a future bundled skill that legitimately uses hooks frontmatter will silently fail in tests.
Recommend either delegate to yaml.parse for unrecognized fixtures, or keep parity with the global beforeEach dispatcher by re-including the hooks: branch:
| mockParseYaml.mockImplementation((yamlString: string) => { | |
| mockParseYaml.mockImplementation((yamlString: string) => { | |
| if (yamlString.includes('hooks:')) { | |
| return yaml.parse(yamlString); | |
| } | |
| if (yamlString.includes('name: simplify')) { | |
| return { | |
| name: 'simplify', | |
| description: 'Simplify recent changes', | |
| }; | |
| } | |
| return { | |
| name: 'review', | |
| description: 'Review code changes', | |
| }; | |
| }); |
— claude-opus-4-7 via Claude Code /qreview
…gnore 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>
|
Thanks for the thorough review. Addressed the open items in c9307c6. Since this skill is an LLM instruction doc rather than executable code, I kept it lean instead of adding defensive prose, and split the feedback into applied and respectfully declined. Applied (kept minimal)
Respectfully declined
Re the earlier Tests: 中文说明感谢细致的 review。已在 c9307c6 处理。这个 skill 是给 LLM 看的指令文档、不是可执行代码,所以我按 KISS 保持精简,把意见分成已采纳和已驳回两类。 已采纳(都做了最小化处理):
已驳回:
另外关于早前的 测试: |
wenshao
left a comment
There was a problem hiding this comment.
No new review findings from this pass (qwen3.7-max). Previous review used gpt-5.5 — re-reviewed for a second opinion. Downgraded from Approve to Comment: CI failing (Test (windows-latest, Node 22.x)). — qwen3.7-max via Qwen Code /review
Stage 1: Template Gate — ❌ Missing Required Headings@pomelo-nwu This PR is missing all three required template headings:
The PR template at The content in 中文说明此 PR 缺少模板要求的全部三个标题:
请按 — Qwen Code |
Local Verification ReportVerified PR #3570 ( Environment
1. Unit tests
2. Discovery / autocompletionTyping
3.
|
* 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
simplifyskill for structured cleanup of recent changes/simplifywas recognized as a skill slash command, invoked thesimplifyskill, created a multi-step todo workflow, inspected git status/diff, launched review agents, applied low-risk cleanup edits, ran targeted verification, and returned to the prompt with a completion summary.Steps tested
npm run dev -- --approval-mode yoloin a 200x50 tmux session.01 initial launch./simplifyusing the helpertype-submitaction.02 submitted /simplify./simplifyworkflow and captured intermediate state in sections03 wait timed out current stateand04 after simplify agents.05 final simplify state before finish.finishaction.Runner output summary:
> @qwen-code/qwen-code@0.15.1 dev > node scripts/dev.js --approval-mode yolo ▄▄▄▄▄▄ ▄▄ ▄▄ ▄▄▄▄▄▄▄ ▄▄▄ ▄▄ ┌──────────────────────────────────────────────────────────┐ ██╔═══██╗██║ ██║██╔════╝████╗ ██║ │ >_ Qwen Code (vdev) │ ██║ ██║██║ █╗ ██║█████╗ ██╔██╗ ██║ │ │ ██║▄▄ ██║██║███╗██║██╔══╝ ██║╚██╗██║ │ API Key | gpt-5.5 (/model to change) │ ╚██████╔╝╚███╔███╔╝███████╗██║ ╚████║ │ ~/Desktop/workspace/qwen-code-2 │ ╚══▀▀═╝ ╚══╝╚══╝ ╚══════╝╚═╝ ╚═══╝ └──────────────────────────────────────────────────────────┘ Tips: 试试 /insight,从聊天记录中生成个性化洞察。 > /simplify ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ Skill Use skill: "simplify" │ │ │ │ Review recent code changes for reuse, code quality, and efficiency, then directly apply straightforward cleanup improvements. Use when the user wants a post-implementation cleanup pass, │ │ pre-PR polish, or asks to simplify/refine recent changes. Invoke with `/simplify` or `/simplify <focus>`. │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ TodoWrite Create todos │ │ │ │ ◐ 检查 git 状态并确定 simplify 审查范围 │ │ ○ 并行执行复用、质量、效率三轮审查 │ │ ○ 汇总发现并实施低风险清理 │ │ ○ 运行针对性验证 │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ✦ 先检查当前 git 状态与 diff 范围;这些命令只读,不会修改文件。 ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ Shell git status --short && git diff --staged --name-only && git diff --name-only && git diff HEAD --name-only [in /Users/pomelo/Desktop/workspace/qwen-code-2] [timeout: 120000ms] (Inspect… │ │ │ │ ?? .qwen/skills/tmux-real-user-testing/ │ │ ?? tmp/ │ │ │ │ ✓ Shell git diff --stat HEAD [in /Users/pomelo/Desktop/workspace/qwen-code-2] [timeout: 120000ms] (Show diff summary) │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ✦ need to follow a specific workflow, focusing only on untracked files in the .qwen skill and tmp directories. I should identify recently modified files and might use list or glob to do that while inspecting directories. Since the review scope likely includes meaningful untracked files, I won’t mention them directly. I'll update my to-do list and then launch three agents in parallel, but first, I need to gather the file list, maybe using list_directory. ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ TodoWrite Update todos │ │ │ │ ● 检查 git 状态并确定 simplify 审查范围 │ │ ◐ 并行执行复用、质量、效率三轮审查 │ │ ○ 汇总发现并实施低风险清理 │ │ ○ 运行针对性验证 │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ✦ need to create a fallback for recent modifications and inspect untracked directories. I think I might use something like list_directory to help with that. Then I can process agents in parallel. I want to ensure they are in the same scope or using different commands. If there’s no difference, I’ll focus on untracked directories like .qwen/skills/tmux-real-user-testing and tmp. I’m also wondering if tmp likely contains logs that I should list. ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ ListFiles .qwen/skills/tmux-real-user-testing │ │ │ │ Listed 2 item(s) │ │ │ │ ✓ ListFiles tmp │ │ │ │ Listed 2 item(s) │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ Agent 审查代码复用 │ │ │ │ general-purpose ● Completed │ │ │ │ Execution Summary: 17 tool uses · 377,459 tokens · 1m 12s │ │ │ │ ✓ Agent 审查代码质量 │ │ │ │ general-purpose ● Completed │ │ │ │ Execution Summary: 23 tool uses · 462,326 tokens · 1m 17s │ │ │ │ ✓ Agent 审查执行效率 │ │ │ │ general-purpose ● Completed │ │ │ │ Execution Summary: 11 tool uses · 173,667 tokens · 55.4s │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ✦ need to continue inspecting the files and checking our tasks. I might need to edit the shell script and adjust SKILL. There are probably temporary directories that aren't tracked. Removing them might be destructive since they're generated files. The user asked for simplification, which means cleanup, but deleting these files is risky. I should confirm before using the rm command. Maybe I could also edit the .gitignore instead. Let's take a closer look! ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ TodoWrite Update todos │ │ │ │ ● 检查 git 状态并确定 simplify 审查范围 │ │ ● 并行执行复用、质量、效率三轮审查 │ │ ◐ 汇总发现并实施低风险清理 │ │ ○ 运行针对性验证 │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ ReadFile .qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh (lines 1-220) │ │ │ │ ✓ ReadFile .qwen/skills/tmux-real-user-testing/SKILL.md (lines 1-220) │ │ │ │ Read lines 1-220 of 225 from .qwen/skills/tmux-real-user-testing/SKILL.md │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ✦ need to continue with the edits. It seems like I should focus on things like slugifying the script, defining a wait-for variable, and maybe adjusting the document path. Also, I think that temporary untracked files probably shouldn't be deleted without confirmation, since that could be destructive. I’m considering using shellcheck to ensure everything runs smoothly. So, let me edit and also remember I need that read_file function ready before proceeding! ✦ 将做本地低风险清理:统一 scenario slug、降低 wait-for 轮询写盘、修正文档中的硬编码/重复示例。 ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ Edit .qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh: require_args() {... => require_args() {... │ │ │ │ 35 fi │ │ 36 } │ │ 37 │ │ 38 + slugify_scenario() { │ │ 39 + local scenario=$1 │ │ 40 + local slug │ │ 41 + slug=$(printf '%s' "$scenario" | tr -cs '[:alnum:]_.-' '-') │ │ 42 + slug=${slug#-} │ │ 43 + slug=${slug%-} │ │ 44 + printf '%s' "${slug:-scenario}" │ │ 45 + } │ │ 46 + │ │ 47 cmd=${1:-help} │ │ 48 shift || true │ │ 49 │ │ 54 workdir=$2 │ │ 55 shift 2 │ │ 56 ts=$(date +%Y%m%d-%H%M%S) │ │ 48 - outdir="$workdir/tmp/${scenario}-tmux-${ts}" │ │ 49 - session="${scenario}-tmux-${ts}" │ │ ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════ │ │ 57 + scenario_slug=$(slugify_scenario "$scenario") │ │ 58 + outdir="$workdir/tmp/${scenario_slug}-tmux-${ts}" │ │ 59 + session="${scenario_slug}-tmux-${ts}" │ │ 60 mkdir -p "$outdir" │ │ 61 tmux new-session -d -s "$session" -x 200 -y 50 -c "$workdir" "$*" │ │ 62 printf 'SESSION=%s\nOUTDIR=%s\nLOG=%s/tmux-readable-full.log\n' "$session" "$outdir" "$outdir" │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ Edit .qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh: wait-for)... => wait-for)... │ │ │ │ 104 sleep_seconds=${5:-2} │ │ 105 scrollback=${6:--400} │ │ 106 mkdir -p "$outdir" │ │ 107 + pane="" │ │ 108 for _ in $(seq 1 "$attempts"); do │ │ 109 sleep "$sleep_seconds" │ │ 109 - tmux capture-pane -t "$session" -p -S "$scrollback" > "$outdir/current-pane.txt" │ │ 110 - if grep -Eq "$regex" "$outdir/current-pane.txt"; then │ │ 110 + pane=$(tmux capture-pane -t "$session" -p -S "$scrollback") │ │ 111 + if grep -Eq "$regex" <<< "$pane"; then │ │ 112 + printf '%s\n' "$pane" > "$outdir/current-pane.txt" │ │ 113 cat "$outdir/current-pane.txt" │ │ 114 exit 0 │ │ 115 fi │ │ 116 done │ │ 117 + printf '%s\n' "$pane" > "$outdir/current-pane.txt" │ │ 118 cat "$outdir/current-pane.txt" │ │ 119 exit 1 │ │ 120 ;; │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ Edit .qwen/skills/tmux-real-user-testing/SKILL.md: Always resolve the script path... => Always resolve the script path... │ │ │ │ 56 Use `scripts/tmux-real-user-log.sh` to avoid rewriting shell glue. The script can │ │ 57 start a session, append labeled snapshots, send keys, wait for text, and finish. │ │ 58 │ │ 59 - Always resolve the script path from the skill directory: │ │ 59 + Always resolve the script path from this skill directory before running it. │ │ 60 + Use the resolved absolute path consistently, because agents may run from a │ │ 61 + workspace other than the repository root. │ │ 62 │ │ 61 - ```bash │ │ 62 - /Users/pomelo/Desktop/workspace/qwen-code-2/.qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh │ │ 63 - ``` │ │ 64 - │ │ 63 Show the helper usage before running a new scenario when the exact sequence is │ │ 64 unclear: │ │ 65 │ │ 66 ```bash │ │ 69 - bash .qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh help │ │ 67 + bash "$SCRIPT" help │ │ 68 ``` │ │ 69 │ │ 70 ## Manual workflow │ │ 71 │ │ 74 - ### 1. Start the TUI │ │ 72 + Prefer the helper script as the single source of truth for tmux setup, snapshots, │ │ 73 + key sending, polling, and cleanup. Use manual `tmux` commands only when debugging │ │ 74 + the helper itself or when the helper cannot express a necessary interaction. │ │ 75 │ │ 76 - Use a large tmux viewport so dialogs render fully: │ │ 76 + Typical sequence: │ │ 77 │ │ 78 ```bash │ │ 79 - TS=$(date +%Y%m%d-%H%M%S) │ │ 80 - OUT="/Users/pomelo/Desktop/workspace/qwen-code-2/tmp/<scenario>-$TS" │ │ 81 - SESSION="<scenario>-$TS" │ │ 82 - mkdir -p "$OUT" │ │ 83 - tmux new-session -d -s "$SESSION" -x 200 -y 50 \ │ │ 84 - -c "/Users/pomelo/Desktop/workspace/qwen-code-2" \ │ │ 85 - "npm run dev -- --approval-mode yolo" │ │ 86 - sleep 4 │ │ 79 + bash "$SCRIPT" start <scenario> "$WORKDIR" npm run dev -- --approval-mode yolo │ │ 80 + bash "$SCRIPT" snapshot "$SESSION" "$OUTDIR" "01 initial screen" │ │ 81 + bash "$SCRIPT" type-submit "$SESSION" /auth │ │ 82 + bash "$SCRIPT" wait-for "$SESSION" "$OUTDIR" "Successfully configured OpenRouter|Error|failed" │ │ 83 + bash "$SCRIPT" finish "$SESSION" "$OUTDIR" │ │ 84 ``` │ │ 85 │ │ 86 Use `node dist/cli.js` instead of `npm run dev` only when verifying a built │ │ 87 bundle. Use the globally installed `qwen` only when reproducing a user-reported │ │ 88 installed-version bug. │ │ 89 │ │ 93 - ### 2. Append labeled readable snapshots │ │ 90 + For text input into Ink fields, prefer `send` with one key at a time if bulk text │ │ 91 + is ignored: │ │ 92 │ │ 95 - After each meaningful action, append a section header plus `capture-pane -p` to │ │ 96 - the full log: │ │ 97 - │ │ 93 ```bash │ │ 99 - LOG="$OUT/tmux-readable-full.log" │ │ 100 - { │ │ 101 - printf '\n===== 01 /auth dialog =====\n' │ │ 102 - tmux capture-pane -t "$SESSION" -p -S -240 │ │ 103 - } >> "$LOG" │ │ 94 + bash "$SCRIPT" send "$SESSION" e n a b l e d │ │ 95 ``` │ │ 96 │ │ ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════ │ │ 106 - Increase `-S` as the session grows. The important part is that each section is a │ │ 107 - rendered frame, not raw ANSI output. │ │ 97 + Append `current-pane.txt` into the readable log with a label after polling when │ │ 98 + that final polled frame is part of the evidence. │ │ 99 │ │ ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════ │ │ 109 - ### 3. Send keys like a user │ │ 110 - │ │ 111 - Split typing and Enter to avoid swallowed submissions: │ │ 112 - │ │ 113 - ```bash │ │ 114 - tmux send-keys -t "$SESSION" "/auth" │ │ 115 - sleep 0.5 │ │ 116 - tmux send-keys -t "$SESSION" Enter │ │ 117 - sleep 2 │ │ 118 - ``` │ │ 119 - │ │ 120 - For navigation: │ │ 121 - │ │ 122 - ```bash │ │ 123 - tmux send-keys -t "$SESSION" Down │ │ 124 - tmux send-keys -t "$SESSION" Space │ │ 125 - tmux send-keys -t "$SESSION" Escape │ │ 126 - ``` │ │ 127 - │ │ 128 - For text input into Ink fields, prefer one key at a time if bulk text is ignored: │ │ 129 - │ │ 130 - ```bash │ │ 131 - tmux send-keys -t "$SESSION" e n a b l e d │ │ 132 - ``` │ │ 133 - │ │ 134 - ### 4. Poll for completion instead of blind sleeping │ │ 135 - │ │ 136 - Use text on the screen as the completion condition: │ │ 137 - │ │ 138 - ```bash │ │ 139 - for i in $(seq 1 60); do │ │ 140 - sleep 2 │ │ 141 - tmux capture-pane -t "$SESSION" -p -S -400 > "$OUT/current-pane.txt" │ │ 142 - if grep -q "Successfully configured OpenRouter\|Error\|failed" \ │ │ 143 - "$OUT/current-pane.txt"; then │ │ 144 - break │ │ 145 - fi │ │ 146 - done │ │ 147 - ``` │ │ 148 - │ │ 149 - Append `current-pane.txt` into the readable log with a label. │ │ 150 - │ │ 151 - ### 5. Finish cleanly │ │ 152 - │ │ 153 - Capture the final screen, append it, then kill the session: │ │ 154 - │ │ 155 - ```bash │ │ 156 - tmux capture-pane -t "$SESSION" -p -S -10000 > "$OUT/tmux-final-capture.log" │ │ 157 - { │ │ 158 - printf '\n===== final capture before cleanup =====\n' │ │ 159 - cat "$OUT/tmux-final-capture.log" │ │ 160 - } >> "$LOG" │ │ 161 - tmux kill-session -t "$SESSION" │ │ 162 - ``` │ │ 163 - │ │ 100 ## Reporting expectations │ │ 101 │ │ 102 Write `report.md` with: │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ TodoWrite Update todos │ │ │ │ ● 检查 git 状态并确定 simplify 审查范围 │ │ ● 并行执行复用、质量、效率三轮审查 │ │ ● 汇总发现并实施低风险清理 │ │ ◐ 运行针对性验证 │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ✦ 运行 shell 语法检查与 helper 的 help 子命令,验证脚本改动不破坏基本执行。 ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ Shell bash -n /Users/pomelo/Desktop/workspace/qwen-code-2/.qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh [in /Users/pomelo/Desktop/workspace/qwen-code-2] [timeout: 12000… │ │ │ │ ✓ Shell bash /Users/pomelo/Desktop/workspace/qwen-code-2/.qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh help [in /Users/pomelo/Desktop/workspace/qwen-code-2] [timeout: 120… │ │ │ │ ... first 13 lines hidden ... │ │ tmux-real-user-log.sh snapshot qwen-openrouter-20260424-105008 /repo/tmp/run "01 /auth dialog" │ │ tmux-real-user-log.sh type-submit qwen-openrouter-20260424-105008 /auth │ │ tmux-real-user-log.sh send qwen-openrouter-20260424-105008 Down Enter │ │ tmux-real-user-log.sh wait-for qwen-openrouter-20260424-105008 /repo/tmp/run "Successfully configured OpenRouter|Error" │ │ tmux-real-user-log.sh finish qwen-openrouter-20260424-105008 /repo/tmp/run │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ Shell git status --short && git diff -- .qwen/skills/tmux-real-user-testing/SKILL.md .qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh [in /Users/pomelo/Desktop/workspace/q… │ │ │ │ ?? .qwen/skills/tmux-real-user-testing/ │ │ ?? tmp/ │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ ✓ TodoWrite Update todos │ │ │ │ ● 检查 git 状态并确定 simplify 审查范围 │ │ ● 并行执行复用、质量、效率三轮审查 │ │ ● 汇总发现并实施低风险清理 │ │ ● 运行针对性验证 │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ✦ 已完成 /simplify 清理。 改动范围: - .qwen/skills/tmux-real-user-testing/scripts/tmux-real-user-log.sh - 增加 slugify_scenario(),统一 session/outdir 命名并避免非法字符。 - 优化 wait-for:轮询时在内存匹配,只在命中或失败时写 current-pane.txt。 - .qwen/skills/tmux-real-user-testing/SKILL.md - 移除本机绝对路径示例。 - 将重复 manual tmux 流程收敛为 helper-first 用法,降低文档/脚本漂移风险。 验证已通过: - bash -n .../tmux-real-user-log.sh - bash .../tmux-real-user-log.sh help 注意:当前仍有未跟踪目录: - .qwen/skills/tmux-real-user-testing/ - tmp/ ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── * 输入您的消息或 @ 文件路径 ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── 🤖 gpt-5.5 | 🌿 feat/add-simplify-skill | 📁 /Users/pomelo/Desktop/workspace/qwen-code-2 | ⚡ 1582380 15.5% 上下文已用 YOLO 模式 (shift + tab 切换)