chore: sync upstream QwenLM/qwen-code (2 commits)#50
Conversation
) 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.
…rite files (QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#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.
* feat(cli): improve hooks matcher display * test(cli): cover hooks navigation levels
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 QwenLM#4325.
… variants to prevent OOM on resume (QwenLM#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 QwenLM#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.
… statusline (QwenLM#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
…e handling (QwenLM#3557) * Harden insight facet normalization and empty qualitative handling * feat: enhance AtAGlance component to accept target sections for dynamic rendering
* 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>
* chore(skills): add codex reproduce workflows * feat(agent-reproduce): implement agent reproduction workflow and supporting scripts * feat(skills): capture reference agent state diffs
) * chore(deps): re-upgrade ink 6 → 7.0.3 (upstream Static remount fix landed) PR QwenLM#3860 first upgraded ink 6 → 7.0.2. PR QwenLM#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 QwenLM#4083) contains the exact fixes: - be9f44cda Fix: <Static> remount via key change drops new items (QwenLM#948) - 669c4386c Fix: Drop stale <Static> output from fullStaticOutput on identity change (QwenLM#950) - 7c2267c01 Fix `useBoxMetrics` not accepting ref objects with an initial null value (QwenLM#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 QwenLM#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 QwenLM#3941's overflowY-self-managed viewport. QwenLM#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 (QwenLM#2950, QwenLM#3118, QwenLM#3007, QwenLM#3838 UI side, QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#4119 review correctly flagged that splitting the onModelChange flow into two effects (b25831b) reintroduced the issue QwenLM#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 (QwenLM#3899). The second effect's refreshStatic() bump arrives a render too late. Fix: do not split. Both side effects (refreshStatic, which writes clearTerminal + bumps historyRemountKey, and setCurrentModel) live in the event handler again, with a ref guard for same-model notifications. The React.StrictMode concern that motivated b25831b is addressed by keeping the side effect OUT of the setState updater (it now runs once per event-handler invocation, not once per double-invoked updater call). Both setState calls land in the same React batch, so historyRemountKey and currentModel update together — MainContent's render-phase reset sees the new key, replayCount drops to the first chunk, and Static remounts with chunked replay intact. Tests: - AppContainer.test.tsx: 4 new tests covering the synchronous refreshStatic side-effect contract, same-model no-op, ref-guarded StrictMode double-invoke, and unsubscribe-on-unmount. - MainContent.test.tsx: new regression guard — when currentModel changes but historyRemountKey is held constant, progressive replay must NOT reset (pins the MainContent invariant the two-effect refactor accidentally relied on). Verified: vitest packages/cli AppContainer + MainContent green (82/82). Typecheck clean. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix+docs(cli): VP review round 5 — typecheck, doc drift, scroll keys PR QwenLM#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 QwenLM#4146, V.3-V.5 deferred), open-question + checklist resolution for QwenLM#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 QwenLM#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 QwenLM#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>
…M#4414) PR QwenLM#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 QwenLM#4173. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…king (QwenLM#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 QwenLM#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>
…rt 1) (QwenLM#4439) * fix(core): coerce hostile-provider usage token counts (QwenLM#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 QwenLM#4350. The companion `computeThresholds` guard depends on the un-merged three-tier ladder in QwenLM#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 hostile `promptTokenCount` falls through to a coerced `totalTokenCount`. * docs(core): narrow coerceUsageCount JSDoc to fields it actually coerces The JSDoc for coerceUsageCount enumerated {prompt,candidates,cached,total}TokenCount as the protected fields, but candidatesTokenCount is never passed through coerceUsageCount in this PR -- only promptTokenCount, totalTokenCount, and cachedContentTokenCount are. The original wording created a misleading impression of coverage. Rewrite the JSDoc to (a) list exactly the three fields the function is called on, (b) state explicitly that candidatesTokenCount is intentionally left raw because it does not feed the compaction gate, and (c) call out that candidatesTokenCount still flows unguarded into OTel spans via loggingContentGenerator -- to be tightened separately. No behavior change. Resolves the geminiChat.ts:109 review thread on this PR. * fix(core): address review feedback on coerceUsageCount - Fix JSDoc: 'This PR' → 'This function' for long-term documentation - Add optional field name parameter to coerceUsageCount for debug logging - Log hostile values via debugLogger.warn when coercion fires - Coerce candidatesTokenCount (was missing despite JSDoc mentioning it) - Build sanitized usageMetadata with coerced values for recordAssistantTurn so JSONL persistence and --resume don't re-poison the compaction gate * fix(core): reuse coerced usage values in recordAssistantTurn The streaming loop was calling coerceUsageCount twice for the same usageMetadata: once for telemetry updates and again inline when building the tokens object for recordAssistantTurn. This left candidatesTokenCount declared but unused in the first block (TS6133). Now stashes all four coerced values in a coercedUsage object during the streaming loop, then spreads it into the tokens object when recording. This eliminates the duplicate coercion calls and uses candidatesTokenCount consistently. Resolves review thread: PRRT_kwDOPB-92c6Ed4WT * test(core): assert debugLogger.warn for hostile usage counts Address PR QwenLM#4439 review thread on geminiChat.test.ts: the hostile-provider parametrized test asserted token-count coercion behaviour but never verified the operator-facing diagnostic emitted by `coerceUsageCount`. A future refactor could remove or misformat the warning with no test failure. - Mock `createDebugLogger` via vi.hoisted/vi.mock so the module-level `debugLogger` in geminiChat.ts routes warnings to a spy. - Inside the existing it.each, assert that hostile defined values (NaN/Infinity/-Infinity/negative/string) emit warns mentioning both `hostile promptTokenCount` and `hostile totalTokenCount`, and that the stringified bad value is included so logs are actionable. - Add a negative assertion that the field-omitted cases (null, undefined) do NOT trigger any warn — verifying the `value != null` guard.
…ell subprocesses (QwenLM#4649) * feat(core): inject context env vars (session/agent/prompt ID) into shell subprocesses When SubAgents execute SQL or Python scripts via Bash tool, the scripts have no way to know their execution context. This adds automatic injection of QWEN_CODE_SESSION_ID, QWEN_CODE_AGENT_ID, and QWEN_CODE_PROMPT_ID into all shell subprocess environments, enabling downstream scripts to perform trace correlation, audit logging, and business context attribution. Closes QwenLM#4645 * fix(core): use module-level flag to guard session env claim Prevents nested qwen-code processes from inheriting the parent's session ID. The previous `if (!process.env[...])` check would keep the parent's value; a module-level flag ensures each process claims its own session ID on first Config construction. * fix(test): use bracket notation for index signature properties CI tsc --build enforces noPropertyAccessFromIndexSignature; use env['KEY'] instead of env.KEY to satisfy strict type checking. * fix(core): guard process.env assignment for mocked process environments Some test suites mock node:process without providing env, causing TypeError when Config constructor assigns QWEN_CODE_SESSION_ID. Add defensive check before env writes. * test(config): add coverage for sessionEnvClaimed guard Addresses reviewer feedback (wenshao) requesting test coverage for the module-level `sessionEnvClaimed` guard in Config constructor. Tests verify: 1. First Config instance sets process.env['QWEN_CODE_SESSION_ID'] 2. Subsequent Config instances do not overwrite the env var * test(config): add startNewSession env var test and clarify comment - Add test verifying startNewSession updates process.env to new session ID - Add comment explaining why startNewSession bypasses sessionEnvClaimed guard (only callable on the canonical Config instance that already claimed)
* feat(core): add auto-mode denial observability * fix(core): prune unused auto denial reason * fix(core): scope auto fallback counter resets * test(core): cover denied fallback cancellation * fix(core): tighten auto-mode denial fallback tracking * test(core): cover permission denied hook events * fix(core): preserve auto fallback reasons * test(core): cover auto mode denial helpers * fix(core): reset auto fallback after hook approval * fix(core): log auto fallback reset recovery * fix(core): clarify auto fallback reset logging * test(core): assert auto fallback reset logging
QwenLM#4654) * feat(core): auto-dump memory diagnostics to disk on pressure detection When the MemoryPressureMonitor (QwenLM#4403) detects hard or critical pressure, write a lightweight diagnostics JSON to .qwen/<project>/diagnostics/ before running cleanup. The file survives even if a subsequent operation triggers OOM, giving maintainers actionable data from bug reports without requiring the user to manually run /doctor memory after a crash. Design follows Claude Code's heapDumpService approach: write the cheap JSON first (small write, won't OOM), heavy snapshot second. Diagnostics include process memory stats, V8 heap stats, session history size, and an actionable suggestion for the user. Per-session limits: max 3 dumps, 30s cooldown between dumps. Closes QwenLM#4651 * ci: retrigger CI after Windows flaky failure * test(core): use path.join in memoryDiagnosticsDumper test for cross-platform The assertion hard-coded POSIX separators ('/tmp/test-project/diagnostics/'), which fails on Windows where path.join produces backslashes. Build the expected substring with path.join + path.sep so it matches the dumper's actual output on every platform. * fix(core): two-phase memory diagnostics write to survive OOM Two critical issues from review: 1. The async collectMemoryDiagnostics() runs before writeFileSync, but it spawns a `ps` subprocess and reads /proc — fork() under critical memory pressure can fail or be OOM-killed, leaving no file on disk despite the "cheap write first" design comment. 2. dumpCount and lastDumpTime were updated after the await, so concurrent dumps (e.g. hard→critical escalation) would both pass the cap/cooldown guards and overwrite each other. Fix: - Reserve the dump slot synchronously (++dumpCount, lastDumpTime) before any await, so concurrent calls correctly hit the cap. - Phase 1: synchronously write a minimal JSON (process.memoryUsage + v8.getHeapStatistics, no fork/exec) with collectionComplete=false. Because async functions execute synchronously up to the first await, this is guaranteed on disk before the caller's next statement runs. - Phase 2: enrich with full diagnostics asynchronously and overwrite the file with collectionComplete=true. If Phase 2 crashes, the minimal Phase 1 file still survives for debugging. Tests updated for the two-phase write and gain two new cases covering the sync-Phase-1 guarantee and the synchronous slot reservation. * fix(core): point memory diagnostics suggestion at /compress (the actual command) The suggestion text told users to run /compact, which does not exist in this repository — the actual command is /compress (see compressCommand.ts). Pointing users at a nonexistent slash command in a diagnostics report makes the suggestion unactionable.
…NL (closes QwenLM#3681, QwenLM#4095 Phase 2) (QwenLM#4333) * feat(core): add atomicWriteFileSync + forceMode option Sync mirror of atomicWriteFile for code paths that can't await (settings persistence on exit, sync config writers). Same semantics: symlink chain resolution, permission preservation, fsync via flush:true, EPERM/EACCES rename retry, EXDEV fallback to direct write. Add forceMode option on AtomicWriteFileOptions — when true, ignore the existing target's permission bits and apply options.mode regardless. Needed for credential files that must heal historically over-permissive files (e.g. a 0o644 token restored from backup must be forced to 0o600). Honored by both async and sync paths. Default false preserves existing behavior. Reuses Atomics.wait for true blocking sleep in renameWithRetrySync — no busy-wait, no extra dep. Refs: QwenLM#4095 Phase 2 * refactor(core): migrate credential writes to atomicWriteFile (QwenLM#4095 Tier 1) Route all OAuth credential persistence through atomicWriteFile with forceMode: true, so a process crash mid-write cannot leave the user with a half-written token file, and historically over-permissive files (e.g. 0o644 from a manual restore) are healed to 0o600 on the next write. - oauth-token-storage.ts: setCredentials, deleteCredentials - file-token-storage.ts: saveTokens (encrypted MCP token storage) - qwenOAuth2.ts: cacheQwenCredentials (also fixes missing mode — was inheriting 0o644 from umask, now forced to 0o600) - sharedTokenManager.ts: saveCredentialsToFile — drops ~15 lines of hand-rolled tmp + rename in favor of the shared helper Lock-file writes using flag: 'wx' (sharedTokenManager.ts:720) are intentionally left untouched — they rely on exclusive-create semantics that atomic write does not preserve. Tests updated to mock atomicWriteFile instead of fs.writeFile. Refs: QwenLM#4095 Phase 2 * refactor(core): migrate memory state writes to atomicWriteFile (QwenLM#4095 Tier 2) Route all auto-memory state persistence through atomicWriteFile so a process crash during a dream/extract/forget cycle cannot corrupt the metadata sidecar, extraction cursor, or topic body files. Touched: manager (writeDreamMetadata), extract (writeExtractCursor + bumpMetadata), indexer (rebuild), dream (bumpDreamMetadata), forget (bumpMetadata + topic body rewrite). manager.ts:362 acquireDreamLock uses flag: 'wx' for exclusive create — left untouched, atomic write does not preserve that semantic. Uses atomicWriteFile (not atomicWriteJSON) to preserve the trailing newline these files have always had. Refs: QwenLM#4095 Phase 2 * refactor: migrate config + logger + state writes to atomic helpers (QwenLM#4095 Tier 3a) Route the remaining state-file write paths through atomic helpers so a crash mid-write cannot corrupt config, log, or session-scoped state: - trustedFolders.ts (sync): atomicWriteFileSync — sole path that flips workspace trust, must not half-write - logger.ts (4 sites): atomicWriteFile — full-file JSON rewrites for logs.json and per-checkpoint files - tipHistory, installationManager, projectSummary, todoWrite, trustedHooks: bonus sites with the same shape (state JSON written multiple times per session) todoWrite is on the hot path — writes every time the todo list mutates — so the added rename + fsync cost is measurable (a few ms per write on SSD). Trade-off accepted to avoid a half-written todos file silently breaking the next session's resume. Export atomicWriteFile / atomicWriteFileSync from the core public index so CLI-side callers (trustedFolders, tipHistory) can reach them. Tests updated: - logger.test.ts uses vi.importActual to re-export the real helper and override per-test via vi.mocked(atomicWriteFile).mockRejectedValueOnce - trustedFolders.test.ts and todoWrite.test.ts mock the helper directly Refs: QwenLM#4095 Phase 2 * fix(core): flush JSONL appends to disk (QwenLM#4095 Tier 3b, closes QwenLM#3681) QwenLM#3656 fixed the read side of glued '}{' JSONL records — when a process was killed mid-appendFile, the trailing '\n' was lost and the next record was concatenated. The write side was left for a follow-up (QwenLM#3681). This adds flush:true (fsync) to every per-line append: - jsonl-utils.ts writeLine / writeLineSync (session transcripts, auto-titles, prompt history) - debugLogger.ts appendFile (per-session debug log) jsonl-utils.ts write() (full-file replace) now goes through atomicWriteFileSync so a crash during overwrite cannot corrupt the session transcript either. Trade-off: fsync on every append adds disk-sync latency (single-digit ms on SSD, more on spinning disk / network FS). Acceptable for a few writes per turn; the alternative is silently losing the last record of every interrupted session, which QwenLM#3681 explicitly flagged. Refs: QwenLM#4095 Phase 2 Tier 3b Closes: QwenLM#3681 * refactor(core): migrate extension config + LSP edit to atomic write Catch up two sites where Claude Code's equivalent path is atomic but qwen-code's isn't (verified against /Users/jinye.djy/Projects/claude-code on 2026-05-19): - extension/extensionManager.ts:533, :1073 — enablement config and install metadata writes. Claude Code's plugin install-counts and zip cache use atomic temp+rename via writeFileSyncAndFlush_DEPRECATED. - lsp/NativeLspService.ts:1351 — applying an LSP edit to a user file. Claude Code's FileWriteTool/FileEditTool both route through atomic writeTextContent → writeFileSyncAndFlush_DEPRECATED. A bare writeFileSync here could half-write the user's source file if the process is killed during an LSP-driven rename or quick-fix. Also clean up stale fs.rename mock setups in sharedTokenManager.test.ts that became no-ops after Tier 1 migration (rename is no longer called by saveCredentialsToFile). The fs.writeFile mocks stay because the wx-flag lock path still uses them. Refs: QwenLM#4095 Phase 2 * chore: cosmetic cleanups from PR review - packages/core/src/index.ts: move atomicFileWrite export to its alphabetical position (before browser.js) - tipHistory.ts: add forceMode: true to atomicWriteFileSync for consistency with other 0o600 sites — heals legacy 0o644 files even though tips are non-critical Refs: QwenLM#4095 Phase 2 * fix(core): address Codex review findings on Phase 2 PR Three issues caught by post-merge Codex review of the QwenLM#4095 Phase 2 branch — none had user-visible symptoms yet but all were latent bugs. 1. atomicFileWrite: forceMode without mode silently downgraded perms `if (!options?.forceMode)` skipped the existing-mode stat whenever forceMode was true, regardless of whether `mode` was also supplied. Calling `atomicWriteFile(p, data, { forceMode: true })` (no mode) on an existing 0o600 file produced 0o644 (umask default) instead of preserving 0o600. Tightened the guard to also require `options.mode` to be defined; mirrored fix in atomicWriteFileSync. Added two regression tests (async + sync) that assert mode preservation. 2. logger.test.ts: vi.resetAllMocks() blanked the atomicWriteFile shim The vi.fn(actual.atomicWriteFile) factory implementation gets reset to a no-op by `vi.resetAllMocks()` in beforeEach, which would make `logger.initialize()` silently skip creating logs.json on disk. Tests passed by coincidence (file pre-existence from prior runs). Captured the real implementation at module load and re-attach it via `mockImplementation` after each reset. 3. NativeLspService.applyTextEdits: atomic write bypassed file unwritability The read catch swallowed every error and treated it as "new empty file". With atomic write (tmp + rename), an unreadable target on a writable parent could be replaced with edits applied to an empty buffer — the old fs.writeFileSync would have errored on the target permission. Now only ENOENT is treated as new-file; other read errors (EACCES, EISDIR, etc.) propagate. Refs: QwenLM#4095 Phase 2 * fix(lsp): refuse LSP edits to chmod 0444 files (Codex round 2) The previous fix only handled "read failed → propagate the error". Codex round 2 caught the remaining gap: a file that's readable but chmod 0444 (read-only) would still be replaced by the atomic rename, because rename only needs parent-directory write access. Add an explicit fs.accessSync(W_OK) check before the atomic write. ENOENT is allowed through so LSP can still create new files via edits. Refs: QwenLM#4095 Phase 2 * fix(core): drop withTimeout around atomic credential write (Codex round 3) `saveCredentialsToFile` wrapped `atomicWriteFile` in `withTimeout(5000)`. If the call hits the 5s budget (e.g. slow NFS home, network-backed storage, fsync added by Phase 2), withTimeout rejects but the atomicWriteFile internal write+rename keeps running unobserved: 1. withTimeout rejects → saveCredentialsToFile throws 2. performTokenRefresh `finally` releases the refresh lock 3. Another process acquires the lock and writes newer credentials 4. The original atomicWriteFile finally completes its rename and overwrites the newer credentials — silent token rollback Pre-migration the code awaited the tmp write and the rename in two separate withTimeout calls; a timed-out tmp write never reached the rename so there was no race against the target file. The migration collapsed both into one inseparable atomicWriteFile, which made the timeout actively unsafe (the work cannot be cancelled after the timeout fires — fs.rename is not abortable). Atomic write is durable by design — accept the I/O latency. The mkdir and stat timeouts are kept (idempotent and read-only respectively, no corruption risk on late completion). Refs: QwenLM#4095 Phase 2 * test(core): add rename-retry + EXDEV-fallback coverage (QwenLM#4333 review) Address PR review suggestions from wenshao (via qwen-latest /review): neither renameWithRetry/Sync nor the EXDEV cross-device fallback had direct test coverage. Both paths are critical (Windows AV contention, Docker tmpfs /tmp) and a regression would degrade silently. Vitest can't spy on ESM exports of `node:fs` (`Cannot redefine property: renameSync`), so add narrow internal test seams instead: - renameWithRetry / renameWithRetrySync take an optional `_renameImpl` parameter, defaulting to fs.rename / fs.renameSync. - atomicWriteFile / atomicWriteFileSync take an optional `_testFs` parameter with `rename` and `writeFile` overrides, forwarded to the retry helper and used in the EXDEV fallback branch. The seams are underscore-prefixed and JSDoc-tagged as "Internal test seam — production callers never pass this", which keeps the public API clean while making the behavior testable. New coverage (+9 tests, 36 → 45): - renameWithRetry: retry-EPERM-then-succeed, give-up after retries, no-retry on non-retryable (ENOSPC) - renameWithRetrySync: same 3 patterns (EACCES, EPERM exhausted, EINVAL) - EXDEV fallback: async direct write + tmp cleanup, sync ditto, non-EXDEV failure propagates without fallback (rejects EIO + tmp cleanup) Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(test): update telemetry sdk.test.ts appendFile assertions for flush:true CI failure on all 3 OSes (macos / ubuntu / windows): sdk.test.ts asserted `fs.appendFile` was called with `'utf8'` as the 3rd positional argument, but commit b7badc7 (QwenLM#4095 Tier 3b — JSONL fsync) changed the `debugLogger.ts` appendFile call from string-form to options-form `{ encoding: 'utf8', flush: true }` to enable per-line fsync. Update the 3 assertions in the telemetry diagnostics test to match the new shape. No behavior change — debugLogger still flushes per append; only the assertion in this previously-unrelated suite needed updating. Refs: QwenLM#4333, QwenLM#4095 Phase 2 Tier 3b * test: cover LSP error branches + sync EXDEV cleanup + JSONL writes (QwenLM#4333 review) Address three review suggestions from wenshao (via qwen-latest /review), each pointing at a real coverage gap introduced by this PR: 1. NativeLspService.applyTextEdits error branches (round-2 LSP fix): the ENOENT-only read guard and the fs.accessSync(W_OK) refusal had no automated coverage. Added 3 tests accessing applyTextEdits via a typed cast (the method is private; making it public for one verification inflates API surface). Tests use chmod 0000 / chmod 0444 reproducers and assert (a) read failure propagates EACCES without silently overwriting with empty content, (b) W_OK rejects with EACCES/EPERM before the atomic rename touches the target, (c) nonexistent files are still accepted so LSP can create via edits. 2. atomicWriteFileSync non-EXDEV rename failure cleanup: the async counterpart had an explicit EIO-rename test asserting tmp cleanup; the sync variant did not. Added the mirror — injects a sync rename throwing EIO via the existing _testFs seam and asserts `readdirSync(tmpDir).length === 0`. 3. jsonl-utils writeLine / writeLineSync / write smoke tests: the three write paths are the core fix for QwenLM#3681 (the PR's headline goal) but downstream callers (chatRecordingService, sessionService) mock them entirely. Without direct unit tests, a regression that dropped `flush: true` or reverted `write()` to bare writeFileSync would go undetected. Added 3 real-fs roundtrip tests. Test count delta: - NativeLspService.test.ts: 15 → 18 - atomicFileWrite.test.ts: 45 → 46 - jsonl-utils.test.ts: 22 → 25 Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core,test): wrap atomic write errors + guard root user + cover save failure (QwenLM#4333 review) Three review fold-ins from wenshao (via qwen-latest /review): 1. atomicFileWrite: error messages reference the random `.tmp.<hex>` path, not the logical target — many callers (memory subsystem, extension manager) don't wrap the error, making debug logs unhelpful. Add `annotateWriteError(error, targetPath)` that mutates the error message in-place to prefix `atomicWriteFile("<targetPath>"): ` while preserving `code` / `errno` / `syscall` / `stack` / the prototype chain so downstream `err.code === 'ENOENT'` checks and `instanceof` narrowing keep working. Applied to both async and sync variants; only the final propagated throw (not the EXDEV fallback path) is annotated. 2. NativeLspService.test.ts: the chmod 0444 and chmod 0000 tests rely on `accessSync(W_OK)` and `readFileSync` failing — but on POSIX with UID 0 (root, including most Docker CI runners), permission bits are bypassed and `accessSync` always succeeds. The tests would silently pass even with the W_OK guard removed entirely. Add `process.getuid?.() === 0` to the skip guard on both tests. 3. sharedTokenManager.test.ts: the catch block in saveCredentialsToFile that maps disk-full / permission-denied to `TokenManagerError(FILE_ACCESS_ERROR)` was never exercised — every prior test mocked atomicWriteFile as always-successful. Added a regression test that rejects atomicWriteFile with ENOSPC and asserts the wrapped TokenManagerError surfaces with the right type and carries the original message. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core,lsp): annotate sync errors correctly + cover EXDEV fallback + async LSP IO (QwenLM#4333 review) Three review fold-ins from wenshao (via qwen-latest /review), all real correctness/consistency issues: 1. annotateWriteError hardcoded "atomicWriteFile" prefix regardless of caller. Sync write failures produced misleading `atomicWriteFile("/path"):` prefixes in incident logs. Add optional `fnName` parameter (defaults to async name) and have sync call sites pass "atomicWriteFileSync". 2. EXDEV fallback path in both async and sync variants did NOT route the inner writeFileImpl/tryChmod failures through annotateWriteError. On a cross-device write that subsequently hit ENOSPC, the propagated error had a bare syscall message without the target-path prefix — breaking the function's documented error-shape contract. Wrap both fallback branches in try/catch + annotate. 3. NativeLspService.applyTextEdits is declared `async` and all callers `await` it, but the round-2 fix mixed in sync IO: readFileSync, accessSync, atomicWriteFileSync. The sync helper's renameWithRetrySync blocks the event loop up to ~350ms under Atomics.wait EPERM backoff — particularly bad for LSP workspace edits that loop over many files. Switch to async throughout: fsp.readFile, fsp.access, atomicWriteFile. Behavior preserved (same ENOENT-vs-other distinction, same W_OK gate). Existing tests pass unchanged (they already use the async typed-cast entry point). Refs: QwenLM#4333, QwenLM#4095 Phase 2 * test(core): cover EXDEV-fallback-write-failure annotation path (QwenLM#4333 review) The previous fold-in added try/catch around the EXDEV fallback write in both async and sync variants, routing fallback failures through `annotateWriteError(err, target, fnName)`. The sync-specific `fnName='atomicWriteFileSync'` differentiation is ONLY exercised on that path, so a regression that dropped or misapplied the annotation on sync would otherwise go undetected. Two new tests inject both a failing rename (EXDEV) and a failing writeFile (ENOSPC) via the `_testFs` seam, then assert (a) the original `code === 'ENOSPC'` propagates intact, and (b) the message matches `/atomicWriteFile(Sync)?\(<target>\):.*ENOSPC/` — verifying target-path prefix AND correct fn-name differentiation. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): annotate guard, drop debug fsync, add noFollow for creds (QwenLM#4333 review) Four review fold-ins from wenshao (via qwen-latest /review): 1. annotateWriteError guard `!error.message.includes(targetPath)` was always false in production. tmpPath is `${targetPath}.<hex>.tmp`, so any real fs error like `ENOSPC ... '/path/creds.json.a1b2c3.tmp'` *contains* targetPath as substring → guard returned false → the annotation prefix was silently skipped on every real failure path. Tests passed only because mock errors used bare `new Error('ENOSPC')` messages without paths. Change guard to idempotency check on our own prefix (`startsWith(`${fnName}(`)`), and update the two existing EXDEV-fallback tests to use realistic path-embedding fs messages. 2. debugLogger.appendFile dropped `flush: true`. 1050+ call sites, default-enabled, fire-and-forget — per-line fsync creates sustained I/O pressure / SSD wear with no user benefit (debug logs are best-effort, the module already tracks `hasWriteFailure` for the degraded-mode UI). Kept the kernel page-cache flush; revert debugLogger.test.ts and telemetry/sdk.test.ts assertions back to plain `'utf8'`. The QwenLM#3681 closure target is jsonl-utils writeLine, not debug logs. 3. Symlink security regression: the old `fs.writeFile(tmp) + fs.rename(tmp, filePath)` pattern atomically *replaced* a pre-placed symlink at `filePath`. atomicWriteFile's default `resolveSymlinkChain(filePath)` follows the link and writes through, redirecting tokens to wherever the link points (real concern on shared hosts with weaker-than-expected dir perms). Add `noFollow?: boolean` option that skips chain resolution; apply `noFollow: true` to all 4 credential write sites (oauth-token-storage [2 sites], file-token-storage, sharedTokenManager) to match the pre-migration replace-symlink semantics. 4a. Test seam `_testFs` was a 4th positional arg → considered moving into options. Punted: positional with underscore + JSDoc is materially the same surface as options field with underscore, and the only realistic collision (future production option as 5th arg) is bounded by review. 4b. Sync/async code duplication (~110 lines mirror) → DECLINED. Refactoring to a sync/async-polymorphic helper introduces a new abstraction layer with worse type ergonomics; the duplication is mechanical and lined up for easy diffing. Tracked as Phase 2.5 candidate if divergence actually accumulates. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): EXDEV fallback honors noFollow + fix test correctness (QwenLM#4333 review) Three review fold-ins from wenshao (via qwen-latest /review): 1. **[CRITICAL]** EXDEV fallback path silently bypassed `noFollow` protection. The fallback `writeFileImpl(targetPath, ...)` is `fs.writeFile` / `fs.writeFileSync`, both of which follow symlinks. When a credential write site set `noFollow: true` and rename threw EXDEV (realistic on Docker OverlayFS / union mounts), the fallback would write credentials *through* a pre-placed symlink to an attacker-controlled target — the exact attack noFollow was meant to prevent. Fix: when `noFollow` is set, the EXDEV fallback now `unlink`s any existing entry, then opens with `O_WRONLY|O_CREAT|O_EXCL` to refuse to write through a symlink that races back. Applied to both async and sync variants. 2. **Test correctness**: the two EXDEV-fallback-write-failure tests added in the previous round had a bug — the `failingWrite` mock threw on every call, including the first call which is the tmp-file write. The tmp write failed → outer catch caught ENOSPC → EXDEV check returned false (code is ENOSPC) → fell through to the outer annotateWriteError. The inner EXDEV-fallback annotation was never exercised. The assertions passed only because annotateWriteError produces the same format in both catch blocks. Fix: selective-failure mock that succeeds on the first call (tmp write) and fails on the second (fallback write), genuinely reaching the EXDEV branch. 3. **Behavioral noFollow tests**: previously `noFollow: true` was only verified at the "option is passed to mock" level. Added 4 real-fs tests (async + sync × happy-path + EXDEV-fallback) that pre-place a symlink, call atomicWriteFile with noFollow, and assert: (a) the symlink is replaced by a regular file, (b) the new file holds the new data, (c) the real file behind the symlink is untouched. A regression flipping the noFollow ternary or skipping the noFollow-aware EXDEV fallback now fails directly. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): close TOCTOU window on noFollow EXDEV fallback + cover ENOENT path Two PR QwenLM#4333 round-7 review items folded in: 1. TOCTOU race: the path-based `tryChmod(targetPath)` after the EXDEV noFollow branch ran AFTER `fd.close()` released the inode reference. Between close and chmod, an attacker with parent-directory write access could replace the regular file with a symlink, redirecting the `chmod 0o600` onto an attacker-chosen target — silently defeating the `noFollow` protection that the `unlink + O_EXCL` pattern was added to provide. Fix: switch to `fd.chmod`/`fchmodSync` on the open fd before close (operates on the inode, immune to symlink swap), and skip the path-based chmod for the noFollow branch (path-based chmod remains for the non-noFollow direct-write branch, where following symlinks was already in scope). 2. Missing test coverage: all 4 existing noFollow EXDEV tests pre-place a symlink at the target, so `unlink(targetPath)` always succeeded — the ENOENT-swallow branch (first-write scenarios, e.g. initial credential provisioning on a cross-device mount) had no coverage. Added 2 tests (async + sync) verifying the fallback creates a new file with the requested mode when the target never existed. Test results: 54/54 atomicFileWrite tests pass (was 52). Async + sync parity preserved. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * test(core): skip new noFollow EXDEV-fallback tests on Windows (NTFS perm bits) The two new-file noFollow EXDEV tests added in 7c47664 assert the file ends up at 0o600. NTFS reports 0o666 for any file that isn't read-only regardless of chmod/fchmod, so the assertion fails on Windows runners (`AssertionError: expected 438 to be 384`). Match the existing `it.skipIf(process.platform === 'win32')` pattern already used by all mode-asserting tests in this file. Linux/macOS coverage of the ENOENT-swallow + new-file path is unchanged. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): narrow fchmod catch + verify mechanism on noFollow EXDEV Two PR QwenLM#4333 round-8 review items: 1. The `catch {}` around `fd.chmod(desiredMode)` (and `fchmodSync`) was intended to tolerate filesystems without POSIX permissions (FAT/exFAT) but silently swallowed every error code: EPERM under a hardened sandbox, EIO on flaky NFS/CIFS, EROFS if the filesystem is remounted read-only mid-write. Because this path operates on the *final credential file* — not a tmp file — a silent fchmod failure leaves the target at the umask-masked open() mode with no diagnostic trail. Narrowed the catch to ENOSYS/ENOTSUP so the FAT/exFAT case still tolerates failure but security-relevant errors propagate. 2. The round-7 noFollow-EXDEV-new-file tests asserted the final mode (0o600) but didn't verify the *mechanism*. Under typical umask 0o022, `open(O_EXCL, 0o600)` already creates the file at 0o600, so a regression that swapped `fd.chmod()` back to a path-based `tryChmod(targetPath)` (the pre-fix TOCTOU-vulnerable form) would leave the mode assertion passing — defeating the round-7 fix undetected. Added a `chmod` test seam to `atomicWriteFile` / atomicWriteFileSync` (mirroring the existing `rename` / `writeFile` seams) and asserted that path-based chmod is never invoked against the credential target on this code path. 54/54 atomicFileWrite tests pass. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): clean up O_EXCL orphan on noFollow EXDEV fchmod failure + cover both branches Two PR QwenLM#4333 round-9 review items, one critical correctness bug introduced by the round-8 catch narrowing: 1. **[Critical]** When `fd.chmod(desiredMode)` (or `fchmodSync`) on the noFollow EXDEV fallback throws a propagating error (EPERM under seccomp, EIO on flaky NFS, EROFS after a remount), the file created by `open(targetPath, O_CREAT|O_EXCL)` remained on disk after the error rethrew. Every subsequent credential write retry then hit EEXIST from O_EXCL and failed permanently — turning a transient sandbox EPERM into a permanent OAuth-refresh deadlock that requires manual file removal. All three credential write sites (`sharedTokenManager`, `oauth-token-storage` save+delete, `file-token-storage`) hit this code path. Fix: a `writeOk` flag plus nested try/catch — fd is closed in the inner finally, then if write/ sync/fchmod failed, the orphan is unlinked best-effort before the error rethrows. 2. The fchmod catch-narrowing (the headline behavior of round 8) had zero test coverage on either branch — `_testFs` exposed `rename`, `writeFile`, and path-based `chmod`, but no hook for the open-fd `fchmod`. A one-line revert from the narrowed catch back to `catch {}` would pass every existing test. Added `fchmod` field to `_testFs` (async + sync) and four tests: - ENOSYS swallowed → write succeeds (FAT/exFAT happy path) - EPERM propagates AND `targetPath` is absent (regression-tests #1) for both async and sync. 58/58 atomicFileWrite tests pass (was 54). Async + sync parity preserved. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): annotate symlink errors + cover ENOTSUP/EACCES + log orphan unlink Four PR QwenLM#4333 round-10 review items, all small bounded fixes: 1. Parameterized the FAT/exFAT fchmod-swallow tests over both ENOSYS (Linux) and ENOTSUP (macOS). Round-9 only covered ENOSYS — a one-token regression dropping `ENOTSUP` from the catch condition would have passed every existing test. 2. The orphan-unlink catch block was empty (`/* best effort */`), leaving no diagnostic trail when the cleanup itself fails (EROFS, immutable flag, sandboxed container). Added a `createDebugLogger` import (`'ATOMIC_WRITE'` category) and a debug log so incident response can correlate the original write error with a subsequent EEXIST loop. Async + sync. 3. The pre-open `unlink(targetPath)` correctly propagates non-ENOENT errors (EACCES on parent dir, EROFS), but no test exercised that path. Added an `unlink` field to the `_testFs` seam (async + sync, matching the existing `rename` / `writeFile` / `chmod` / `fchmod` pattern) and two tests verifying EACCES propagates instead of getting hidden behind a downstream EEXIST from O_EXCL. 4. `resolveSymlinkChain(filePath)` ran before the function's main try-block, so symlink-resolution errors (EACCES on intermediate dir, ELOOP from circular chain) bypassed the `atomicWriteFile("path"): ...` annotation that every other failure path applies — leaving `err.path` referencing an internal intermediate directory the caller never asked about. Wrapped with `.catch(err => throw annotateWriteError(err, filePath))` (async) and the equivalent try/catch for sync. Added a real-fs ELOOP regression test for both variants (skipped on Windows). 64/64 atomicFileWrite tests pass (was 58). Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): widen atomicWriteJSON options + tighten trustedHooks perms + cover backoff/mkdir branches Five PR QwenLM#4333 round-11 review items, all small and bounded: 1. **atomicWriteJSON option-type widening**: Was typed as the narrower `AtomicWriteOptions` (retries / delayMs only), so credential-grade options added in this PR (`mode`, `forceMode`, `noFollow`) and pre-existing ones (`flush`, `encoding`) were silently dropped at the type level even though the body spread them at runtime. A future maintainer calling `atomicWriteJSON(credPath, creds, {noFollow: true, mode: 0o600, forceMode: true})` would have typechecked but silently lost noFollow + forceMode + mode. Widened to `AtomicWriteFileOptions`. 2. **trustedHooks 0o600 + forceMode**: This file lists user-approved *executable hook commands* — strictly more sensitive than the sibling state files (`trustedFolders.json`, `tipHistory.json`) that already use `{mode: 0o600, forceMode: true}`. Was dropping to the process umask (0o644 by default), and a backup-restored looser mode was never healed. Now matches the sibling pattern. 3. **renameWithRetry exponential-backoff coverage**: Existing tests covered retry count and error propagation but not the `delayMs * 2 ** attempt` curve itself. A regression to linear, constant, or — worst — regressive backoff (which intensifies under Windows AV-scan stress) would have passed every existing test. Added a test using `vi.useFakeTimers()` that records gaps between mock-rename invocations and asserts `[delayMs, 2*delayMs, 4*delayMs]` for `(retries=3, delayMs=50)`. 4. **jsonl-utils write() parent-dir creation**: The other `write()` test targets a path inside the pre-created `tmpRoot`, so the `!existsSync(dir) → mkdirSync(dir, {recursive: true})` branch was never exercised. Added a one-liner test that targets a deeply nested non-existent path. 5. **writeLineSync docstring accuracy**: The docstring claimed "uses a simple flag-based locking mechanism (less robust than async version)" but there is no flag-based locking — and `writeLine` serializes via per-file `Mutex` that this function bypasses. Now accurately documents the lack of locking, the bypass, and the `flush: true` rationale (closes QwenLM#3681). Test results: 65/65 atomicFileWrite tests pass (was 64), 26/26 jsonl-utils tests pass (was 25). Typecheck clean. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): force JS slow path for appendFile flush + correct debugLogger format Two PR QwenLM#4333 round-12 review items, both real bugs: 1. **flush:true was silently a no-op for string payloads on appendFile**. Node's C++ fast path (binding.writeFileUtf8) for string + utf8 + appendFile bypasses the JS-side flush/fsync logic entirely — empirically string+flush:true takes ~0.05ms/op (identical to no flush) while Buffer+flush:true takes ~4.9ms/op (91× slower, proving fsync only runs for Buffer payloads). The data still reaches the kernel page cache (the syscall is synchronous), so `kill -9` is fine, but power-loss durability — the actual QwenLM#3681 guarantee — was silently absent. Fix: pass `Buffer.from(line, 'utf8')` to both writeLine (async) and writeLineSync. This forces the JS slow path that honors `flush: true` and actually fsyncs the file. Updated the JSDoc on both functions to document the C++ fast-path bypass so a future maintainer doesn't revert to the simpler string form. 2. **`debugLogger.debug` doesn't do printf substitution**. `debugLogger`'s `formatArgs` (debugLogger.ts:67-77) just joins args with spaces — no `util.format()`. The round-10 calls used `'orphan unlink failed for %s: %s'` which rendered the literal `%s` markers in the log: orphan unlink failed for %s: %s /path/to/target Error: EACCES instead of: orphan unlink failed for /path/to/target: Error: EACCES Switched both async and sync sites to template literals, matching every other `debugLogger` call site in the codebase. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): narrow tryChmod catch + writeFileSync(fd) + cover credential write failures Three PR QwenLM#4333 round-13 review items + one comment-wording softening: 1. **`tryChmod`/`tryChmodSync` catch narrowed (wenshao)** — was bare `catch {}` swallowing all errors, while the round-8 `fchmod` catch was already narrowed to ENOSYS/ENOTSUP. Same security rationale applies — and *specifically* on the EXDEV non-noFollow fallback, `tryChmod(targetPath)` is the *sole* mode-setting mechanism for an existing target (writeFile ignores `mode` when the target exists), so a silent EPERM/EROFS would leave credentials at the old mode. Non-credential callers don't pass `mode` → `desiredMode === undefined` short-circuits, so they're unaffected. 2. **Sync EXDEV `writeSync` → `writeFileSync(fd)` (yiliang114)** — `fsSync.writeSync(fd, buf)` returns bytes-actually-written and can short-write; the current code ignored the return, so a partial write would silently truncate the credential file with the call still returning success after fsync+fchmod. Switched to `fsSync.writeFileSync(fd, buf)` which loops internally per Node spec. The async sibling (`fd.writeFile`) already handles short-writes; this brings sync parity. 3. **`file-token-storage` failure-path coverage (wenshao)** — both `setCredentials` and `deleteCredentials` propagate `atomicWriteFile` rejections (no try/catch around the call), but no test exercised that path. Added two tests mirroring the round-1 sharedTokenManager precedent: ENOSPC on `setCredentials` and EROFS on `deleteCredentials` both rethrow. 4. **Round-12 comment wording softened (wenshao verification report)** — strace on Node v22/v24 confirms string + utf8 + flush:true does fsync correctly today, counter to my round-12 "silent no-op" framing. Buffer is still the safer documented form (forward-compat insurance against any future fast-path optimization), but the commit's claim that it was *fixing* a confirmed bug overstated what reproduces. Reframed both writeLine and writeLineSync comments accordingly without changing the code behavior. Test results: 109/109 affected suites pass (atomicFileWrite 65, jsonl-utils 26, file-token-storage 18). Broader credential/state suites also green: 216/216 across sharedTokenManager + oauth-token-storage + qwenOAuth2 + logger + trustedFolders. Typecheck clean. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): route orphan unlink through seam + cover tryChmod + trustedHooks tests Three PR QwenLM#4333 round-14 review items: 1. **Path-level tryChmod / tryChmodSync catch narrowing had zero direct coverage**. Round-13 narrowed the bare `catch {}` to ENOSYS/ENOTSUP only (matching the round-8 fd-level fchmod narrowing), but every existing EXDEV test passed `options: undefined`, so `desiredMode === undefined` short-circuited before any chmod was attempted. A regression that inverted the catch condition (missing `!` prefix) would silently swallow EPERM on every sync credential write (trustedHooks, tipHistory, trustedFolders, etc.) with zero diagnostic signal. Added 6 tests via the existing `_testFs.chmod` seam: parameterized ENOSYS/ENOTSUP swallow + EPERM propagation, for both async (`atomicWriteFile`) and sync (`atomicWriteFileSync`). 2. **Orphan-cleanup unlink on the noFollow EXDEV failure path was using raw `fs.unlink` / `fsSync.unlinkSync` instead of the injected `unlinkImpl` seam**. The pre-open unlink correctly used the seam, but the round-9 orphan cleanup added later bypassed it, making it the only fs operation in `atomicWriteFile` not flowing through the test seam. Routed both async and sync orphan cleanup through `unlinkImpl`, and added 2 tests that inject a spy and assert orphan cleanup is invoked against targetPath after a simulated fchmod EPERM. 3. **`trustedHooks.ts` had no test coverage**. Round-11 migrated it to `atomicWriteFileSync` with `{ mode: 0o600, forceMode: true }` — strictly the most security-sensitive write in the PR since the file stores user-approved executable hook commands — but unlike the sibling files (trustedFolders, tipHistory) it had no test file. A regression that dropped `forceMode: true` or weakened the mode would have passed all existing tests. Created `trustedHooks.test.ts` covering: write goes through atomicWriteFileSync with `{ mode: 0o600, forceMode: true }`, write targets the global qwen dir path, the persisted content matches the hook key derived from the hook config, and round-trip trust/untrust behavior. Test results: 73/73 atomicFileWrite tests (was 65) + 5/5 trustedHooks (new). Typecheck clean. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): add noFollow to trustedHooks/trustedFolders + route tmp cleanup through seam Two PR QwenLM#4333 round-15 review items: 1. **`trustedHooks.ts` and `trustedFolders.ts` were missing `noFollow: true`**. The credential write sites (`sharedTokenManager`/`oauth-token-storage`/`file-token-storage`) all pass `{ mode: 0o600, forceMode: true, noFollow: true }` to prevent pre-placed symlink attacks. The trustedHooks comment already called it "strictly more sensitive than trustedFolders / tipHistory" — yet credential paths got symlink protection it didn't. A pre-placed symlink at `~/.qwen/trusted_hooks.json` (or `trustedFolders.json`) could redirect the atomic write to an attacker-controlled path, either leaking the executable-trust list / trusted-folder list, or leaving the user's real config silently stale. Added `noFollow: true` to both write sites and updated the assertions in `trustedHooks.test.ts` and `trustedFolders.test.ts`. 2. **Tmp-file cleanup at L240 (async) and L568 (sync) used raw `fs.unlink` / `fsSync.unlinkSync` instead of the injected `unlinkImpl` seam**. Pre-open unlink and orphan cleanup correctly routed through `unlinkImpl`, making the tmp-cleanup branch the only outlier — `_testFs.unlink`-injecting tests couldn't intercept this path, weakening the seam abstraction. Behavioral impact is nil (cleanup is best-effort, errors swallowed), but consistency matters for future test authors. Routed both async and sync variants through `unlinkImpl`. Test results: 99/99 affected suites pass (atomicFileWrite 73, trustedHooks 5, trustedFolders 21). Typecheck clean on core. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core): use path.join in trustedHooks test to fix Windows CI PR QwenLM#4333 round-15 review item (Critical, from claude-opus-4-8 /qreview): trustedHooks.test.ts:68 hardcoded a POSIX path ('/mock/home/.qwen/trusted_hooks.json'), but production builds the path with path.join(Storage.getGlobalQwenDir(), 'trusted_hooks.json') (trustedHooks.ts:29-31). On Windows path.join emits '\' separators, so the mocked atomicWriteFileSync receives '\mock\home\.qwen\trusted_hooks.json' and the toBe assertion fails — the cause of the red Test (windows-latest, Node 22.x) check. macOS/Linux runs are green because forward slashes match. Build the expected path with path.join so it matches the platform separator, and add the node:path import. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * fix(core,cli): round-16 review — tipHistory noFollow + atomicWrite test coverage + LSP concurrency note Four PR QwenLM#4333 follow-up review items from wenshao: 1. tipHistory.ts was missing noFollow: true while the sibling 0o600+forceMode config sites (trustedFolders.ts:192, trustedHooks.ts:63) both set it. The cleanup commit added forceMode "for consistency with other 0o600 sites" but stopped short of noFollow. All three store user-trusted paths and should share the same pre-placed-symlink protection. Added noFollow: true. 2. annotateWriteError idempotency guard had no direct coverage. The guard switched from includes(targetPath) to startsWith(fnName+"(") because real syscall errors embed the *tmp* path (which contains the target as a substring), so the old guard silently skipped annotation on every real failure. Added a test where the rename error message embeds a tmp-style path containing the target; the correct startsWith guard still annotates it, and reverting to includes would fail it. 3. non-EXDEV rename failure tests only asserted /EIO/, not the atomicWriteFile(...): / atomicWriteFileSync(...): annotation prefix that the production re-throw applies on that path. Tightened both async and sync assertions to match the prefix. 4. applyTextEdits concurrency constraint was undocumented. The async read-modify-write has await points between read and write; atomicWriteFile prevents torn files but does not serialize writers, so concurrent same-path edits can lose updates. Latent today (no production caller / no workspace/applyEdit handler), so documented the per-file-serialization requirement instead of adding a lock. Not taking: extending the _testFs seam with open/openSync to fault-inject O_EXCL EEXIST — the noFollow/O_EXCL security behavior is already covered by the real-fs behavioral tests (noFollow EXDEV fallback refuses to follow symlinks); a seam-injected EEXIST would only exercise error propagation, not the guarantee. Test results: atomicFileWrite 74/74 pass, eslint clean. Refs: QwenLM#4333, QwenLM#4095 Phase 2 * test(core): route atomicWriteFile open through _testFs seam + assert O_EXCL (PR QwenLM#4333 review) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…M#4699) * docs: add /diff command documentation to commands.md Add section 1.8 documenting the /diff interactive diff viewer, including source picker (Current + per-turn diffs), keyboard shortcuts, dialog example, and non-interactive mode output format. Also add /diff entry to the 1.2 Interface and Workspace Control table. * docs: add auto theme detection section to themes.md Document the 'auto' theme setting and its detection fallback chain (COLORFGBG → OSC 11 → macOS system appearance → default dark), including notes for tmux/SSH environments. * docs: fix checkpointing default description in /diff section Checkpointing defaults to false, not true. Updated from "on by default" to "disabled by default" per reviewer feedback. * docs: fix file checkpointing default in /diff section File checkpointing (used by per-turn diffs and /rewind) defaults to enabled in interactive mode. Session checkpointing (/restore) is the one that defaults to disabled. Corrected the description accordingly. --------- Co-authored-by: 克竟 <dingbingzhi.dbz@alibaba-inc.com>
- feat(core): atomic write rollout for credentials, memory, config, JSONL - docs: add /diff command and auto theme detection documentation Rebranding applied: qwen/* -> hopcode/*, YOLO -> Izn/IZN/izn approval mode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds robust atomic file-write primitives (async + sync) and migrates approval-mode naming/behavior from YOLO→IZN; replaces many direct fs writes with atomic writes across tokens, logs, hooks, settings, memory, LSP edits, todos, and updates tests, docs, and UI wiring accordingly. ChangesCore Migration & Hardening
🎯 4 (Complex) | ⏱️ ~75 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d12280d39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const targetPath = options?.noFollow | ||
| ? filePath | ||
| : await resolveSymlinkChain(filePath).catch((err) => { | ||
| throw annotateWriteError(err, filePath); | ||
| }); |
There was a problem hiding this comment.
Preserve noFollow during ownership fallback
When noFollow is set and filePath is a symlink, leaving targetPath as the symlink means the later fs.stat(targetPath) and ownership fallback still follow it; if the symlink target is owned by a different uid, the fallback path writes through the link instead of replacing it. This affects credential/trusted-file callers that pass noFollow: true and reopens the symlink redirection the option is meant to prevent.
Useful? React with 👍 / 👎.
| 'izn', | ||
| 'yolo', | ||
| 'izn', |
There was a problem hiding this comment.
Remove duplicate daemon approval mode
This exported SDK list now advertises izn twice while the core APPROVAL_MODES enum only has five entries (plan, default, auto-edit, auto, izn), so consumers rendering DAEMON_APPROVAL_MODES will show a duplicate option and the SDK approval-mode drift test will fail its exact array comparison.
Useful? React with 👍 / 👎.
| - Setting `QWEN_SANDBOX` environment variable. | ||
| - Setting `tools.sandbox` in settings. | ||
|
|
||
| > ⚠️ **`--izn` does _not_ automatically enable a sandbox.** IZN mode only auto-approves tool calls; sandboxing must still be opted into via `--sandbox`, `HOPCODE_SANDBOX`, or `tools.sandbox`. In headless / non-interactive runs with `--izn` (or `--approval-mode=izn`) and no sandbox, the model can execute shell, write, and edit tools at the current process's privilege level — HopCode prints a warning to stderr in that case. Suppress with `HOPCODE_SUPPRESS_IZN_WARNING=1` once you've reviewed the trade-off. | ||
| > ⚠️ **`--izn` does _not_ automatically enable a sandbox.** IZN mode only auto-approves tool calls; sandboxing must still be opted into via `--sandbox`, `QWEN_SANDBOX`, or `tools.sandbox`. In headless / non-interactive runs with `--izn` (or `--approval-mode=izn`) and no sandbox, the model can execute shell, write, and edit tools at the current process's privilege level — HopCode prints a warning to stderr in that case. Suppress with `QWEN_CODE_SUPPRESS_IZN_WARNING=1` once you've reviewed the trade-off. |
There was a problem hiding this comment.
Keep documented sandbox env vars branded
This section tells users to enable sandboxing with QWEN_SANDBOX and suppress the IZN warning with QWEN_CODE_SUPPRESS_IZN_WARNING, but the CLI actually reads HOPCODE_SANDBOX in packages/cli/src/config/sandboxConfig.ts:41 and HOPCODE_CODE_SUPPRESS_IZN_WARNING in packages/cli/src/utils/headlessSafetyWarnings.ts:12 and :42. Users following these docs will still run without the requested sandbox and will not suppress the warning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR syncs two upstream commits into HopCode, primarily introducing an atomic file-write utility and rolling it out across multiple persistence paths (credentials, config/state, JSONL, logs), plus documentation updates (including /diff and auto theme detection) and the removal of the legacy yolo approval mode in favor of izn.
Changes:
- Add/expand
atomicWriteFile/atomicWriteFileSyncand migrate many write sites to use atomic + safer permission/symlink semantics. - Update approval-mode surface area to remove
yoloand standardize onizn(core, CLI, SDK, schemas, and tests). - Update user docs (commands, themes, settings) and Insight templates/types to reflect new/renamed fields.
Reviewed changes
Copilot reviewed 79 out of 79 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web-templates/src/insight/src/Qualitative.tsx | Updates Insight qualitative rendering (incl. HOPCODE.md additions wiring). |
| packages/web-templates/src/insight/src/App.tsx | Updates Insight feature visibility gating for renamed improvements field. |
| packages/vscode-ide-companion/schemas/settings.schema.json | Removes yolo from approvalMode schema/description. |
| packages/sdk-typescript/src/daemon/types.ts | Updates daemon approval mode tuple/types for SDK consumers. |
| packages/core/src/utils/projectSummary.ts | Migrates persisted welcome-back state write to atomic writes. |
| packages/core/src/utils/jsonl-utils.ts | Improves JSONL durability (flush:true) and uses atomic full-file writes. |
| packages/core/src/utils/jsonl-utils.test.ts | Adds smoke coverage for JSONL write paths and atomic wiring. |
| packages/core/src/utils/installationManager.ts | Migrates installation id persistence to atomic sync writes. |
| packages/core/src/utils/debugLogger.ts | Adds rationale comment for best-effort debug log appends (no fsync). |
| packages/core/src/utils/atomicFileWrite.ts | Adds atomic write implementation with retry, chmod semantics, and sync variants. |
| packages/core/src/tools/todoWrite.ts | Migrates todo persistence to atomic writes. |
| packages/core/src/tools/todoWrite.test.ts | Updates tests to mock atomic writes and validate call semantics. |
| packages/core/src/tools/shell.ts | Updates approval-mode wording in command-substitution commentary. |
| packages/core/src/tools/monitor.ts | Removes BOM and updates approval-mode wording. |
| packages/core/src/tools/monitor.test.ts | Removes BOM and updates approval-mode wording in tests. |
| packages/core/src/skills/skill-manager.test.ts | Updates skill-manager tests around provider dirs / expected paths. |
| packages/core/src/services/memoryDiagnosticsDumper.ts | Updates license header branding. |
| packages/core/src/permissions/permission-manager.ts | Updates approval-mode wording in docs/comments. |
| packages/core/src/permissions/permission-manager.test.ts | Updates approval-mode wording in tests. |
| packages/core/src/memory/manager.ts | Migrates dream metadata persistence to atomic writes. |
| packages/core/src/memory/indexer.ts | Migrates auto-memory index writes to atomic writes. |
| packages/core/src/memory/forget.ts | Migrates auto-memory mutation writes to atomic writes. |
| packages/core/src/memory/extract.ts | Migrates extract cursor/metadata writes to atomic writes. |
| packages/core/src/memory/dream.ts | Migrates dream metadata writes to atomic writes. |
| packages/core/src/mcp/token-storage/file-token-storage.ts | Uses atomic write with mode/forceMode/noFollow for credential storage. |
| packages/core/src/mcp/token-storage/file-token-storage.test.ts | Updates tests to mock atomic writes and assert option usage/error propagation. |
| packages/core/src/mcp/oauth-token-storage.ts | Migrates MCP OAuth token persistence to atomic writes + safer options. |
| packages/core/src/mcp/oauth-token-storage.test.ts | Updates tests to mock atomic writes and assert option usage. |
| packages/core/src/lsp/NativeLspService.ts | Makes applyTextEdits async + atomic write and adds permission checks. |
| packages/core/src/lsp/NativeLspService.test.ts | Adds coverage for applyTextEdits error branches and permissions behavior. |
| packages/core/src/index.ts | Re-exports atomic file write utilities from core package. |
| packages/core/src/hopcode/sharedTokenManager.ts | Migrates credential save path to atomic writes with safer semantics. |
| packages/core/src/hopcode/sharedTokenManager.test.ts | Updates tests for atomic write path and adds failure-path coverage. |
| packages/core/src/hooks/trustedHooks.ts | Persists trusted hooks via atomic sync write with secure options. |
| packages/core/src/hooks/trustedHooks.test.ts | Adds unit tests for TrustedHooksManager persistence and behavior. |
| packages/core/src/hooks/hookRunner.ts | Simplifies signal extraction and clarifies env var comment. |
| packages/core/src/extension/extensionManager.ts | Migrates extension config/metadata writes to atomic writes. |
| packages/core/src/core/permissionFlow.ts | Removes yolo auto-approve behavior; keeps izn only. |
| packages/core/src/core/logger.ts | Migrates log/checkpoint persistence to atomic writes. |
| packages/core/src/core/logger.test.ts | Refactors tests to use atomic writes and HOME redirection for isolation. |
| packages/core/src/core/coreToolScheduler.test.ts | Renames YOLO test coverage to IZN and updates defaults. |
| packages/core/src/config/config.ts | Removes YOLO approval mode; standardizes IZN display text. |
| packages/cli/src/utils/housekeeping/throttledOnce.ts | Updates license header branding. |
| packages/cli/src/utils/housekeeping/scheduler.ts | Minor rename for hopcodeDir variable and license header branding. |
| packages/cli/src/utils/housekeeping/scheduler.test.ts | Updates license header branding. |
| packages/cli/src/utils/housekeeping/cleanup.ts | Updates license header branding. |
| packages/cli/src/utils/housekeeping/cleanup.test.ts | Updates license header branding. |
| packages/cli/src/ui/hooks/useAutoAcceptIndicator.test.ts | Updates approval-mode cycling expectations for IZN. |
| packages/cli/src/ui/components/shared/VirtualizedList.tsx | Updates license header branding. |
| packages/cli/src/ui/components/SettingsDialog.test.tsx | Updates approvalMode enum wrap-around test from YOLO→IZN. |
| packages/cli/src/ui/components/MainContent.tsx | Renames pendingGeminiHistoryItems to pendingHistoryItems. |
| packages/cli/src/ui/components/hooks/sourceLabels.ts | Updates license header branding. |
| packages/cli/src/ui/components/hooks/sourceLabels.test.ts | Updates license header branding. |
| packages/cli/src/ui/components/hooks/matcherGrouping.test.ts | Updates license header branding. |
| packages/cli/src/ui/components/hooks/HooksManagementDialog.test.tsx | Minor formatting/comment cleanup. |
| packages/cli/src/ui/components/hooks/HookMatcherDetailStep.test.tsx | Updates license header branding. |
| packages/cli/src/ui/components/hooks/HandlerListBody.tsx | Updates license header branding. |
| packages/cli/src/ui/components/hooks/HandlerListBody.test.tsx | Updates license header branding. |
| packages/cli/src/ui/components/Footer.tsx | Moves config init progress into footer row and adds init messaging/spinner. |
| packages/cli/src/ui/commands/hooksCommand.ts | Simplifies return typing/default result handling. |
| packages/cli/src/services/tips/tipHistory.ts | Migrates tip history persistence to atomic sync write with secure options. |
| packages/cli/src/services/insight/generators/DataProcessor.ts | Updates Insight schema (Hopcode_md_additions) and header branding. |
| packages/cli/src/services/insight/generators/DataProcessor.test.ts | Updates license header branding. |
| packages/cli/src/serve/server.test.ts | Updates allowed approval-mode literals list. |
| packages/cli/src/nonInteractive/types.ts | Updates approval-mode union type to remove yolo. |
| packages/cli/src/gemini.tsx | Renames yolo warning var to izn warning var. |
| packages/cli/src/config/trustedFolders.ts | Migrates trusted folders persistence to atomic sync write with secure options. |
| packages/cli/src/config/trustedFolders.test.ts | Updates tests to mock/assert atomic sync write usage. |
| packages/cli/src/config/settingsSchema.ts | Removes YOLO option from settings schema; adjusts label casing. |
| packages/cli/src/acp-integration/session/types.ts | Removes yolo from ApprovalModeValue union. |
| packages/cli/src/acp-integration/session/Session.ts | Removes yolo mapping; updates approval-mode commentary. |
| packages/cli/src/acp-integration/session/Session.test.ts | Updates license header branding and fixes mojibake arrows/dashes. |
| docs/users/features/worktree.md | Updates examples to use --approval-mode izn. |
| docs/users/features/commands.md | Adds /diff documentation and updates command tables/sections. |
| docs/users/configuration/themes.md | Adds auto theme detection documentation. |
| docs/users/configuration/settings.md | Updates various settings/environment variable documentation and examples. |
| docs/users/configuration/_meta.ts | Adjusts configuration docs navigation keys. |
| docs/e2e-tests/worktree-phase-d.md | Updates e2e instructions from yolo to izn. |
|
|
||
| return ( | ||
| <div className="hopcode-md-section"> | ||
| <div className="qwen-md-section"> |
| </p> | ||
|
|
||
| <div className="hopcode-md-actions" style={{ marginBottom: '12px' }}> | ||
| <div className="qwen-md-actions" style={{ marginBottom: '12px' }}> |
|
|
||
| {additions.map((item, idx) => ( | ||
| <div key={idx} className="hopcode-md-item"> | ||
| <div key={idx} className="qwen-md-item"> |
| 'izn', | ||
| 'yolo', | ||
| 'izn', |
| | 'izn' | ||
| | 'yolo'; | ||
| | 'izn'; |
| const validSkillConfig: SkillConfig = { | ||
| name: 'test-skill', | ||
| description: 'A test skill', | ||
| level: 'project', | ||
| filePath: '/test/project/.hopcode/skills/test-skill/SKILL.md', | ||
| filePath: '/test/project/.qwen/skills/test-skill/SKILL.md', | ||
| body: 'You are a helpful assistant with this skill.', | ||
| }; |
| display: 'hidden', | ||
| }, | ||
| 'hopcode-ignore': 'Ignoring Files', | ||
| 'qwen-ignore': 'Ignoring Files', |
|
|
||
| ## Auto Theme Detection | ||
|
|
||
| When the theme is set to `"auto"` (or left unset), Qwen Code automatically detects whether your terminal uses a dark or light background and selects the matching Qwen theme (`Qwen Dark` or `Qwen Light`). |
|
|
||
| ### Detection methods | ||
|
|
||
| Qwen Code uses multiple detection methods in a fallback chain. At startup (async path), the order is: |
| | Standalone `!` | Switch Shell mode, any input is executed directly as Shell command | `!`(enter) → Input command → `!`(exit) | | ||
|
|
||
| Environment Variables: Commands executed via `!` will set the `HOPCODE=1` environment variable. | ||
| Environment Variables: Commands executed via `!` will set the `HOPCODE_CODE=1` environment variable. |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
packages/core/src/hooks/hookRunner.ts (1)
572-579:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
$HOPCODE_PROJECT_DIRto command placeholder expansion for HopCode compatibility.
hookRunner.tsinjectsHOPCODE_PROJECT_DIR, butexpandCommand()only rewrites$GEMINI_PROJECT_DIRand$CLAUDE_PROJECT_DIR, so hooks that use$HOPCODE_PROJECT_DIRwon’t getcwdsubstituted (cross-shell mismatch, including PowerShell).Suggested fix
return command .replace(/\$GEMINI_PROJECT_DIR/g, () => escapedCwd) - .replace(/\$CLAUDE_PROJECT_DIR/g, () => escapedCwd); // For compatibility + .replace(/\$CLAUDE_PROJECT_DIR/g, () => escapedCwd) // For compatibility + .replace(/\$HOPCODE_PROJECT_DIR/g, () => escapedCwd); // For HopCode compatibility🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/hooks/hookRunner.ts` around lines 572 - 579, expandCommand() currently rewrites $GEMINI_PROJECT_DIR and $CLAUDE_PROJECT_DIR but not $HOPCODE_PROJECT_DIR, so hooks using that placeholder won't be substituted; update the expandCommand function to also recognize and replace HOPCODE_PROJECT_DIR (including cross-shell variants like $HOPCODE_PROJECT_DIR, ${HOPCODE_PROJECT_DIR}, and PowerShell-style $env:HOPCODE_PROJECT_DIR) with the same input.cwd value that hookRunner.ts injects into env, ensuring consistency between the env construction and placeholder expansion.packages/core/src/utils/atomicFileWrite.ts (1)
184-186:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
noFollowstill dereferences attacker symlinks on the uid-preservation path.Line 186 uses
fs.stat(targetPath), sonoFollow: truestill follows a pre-placed symlink during metadata lookup. If that target is a regular file owned by another uid, Lines 249-255 take the ownership-preservation fallback andfs.writeFile(targetPath, ...)writes through the symlink instead of replacing it. That reintroduces the credential-redirection bug this option is supposed to prevent on shared hosts. Uselstat/symlink detection innoFollowmode and skip the in-place fallback for symlinked paths; add a regression test fornoFollow + ownershipWouldChange().Also applies to: 249-255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/utils/atomicFileWrite.ts` around lines 184 - 186, The code currently uses fs.stat(targetPath) (setting existingStat) which follows symlinks and allows an attacker-supplied symlink to trigger the ownership-preservation in-place fallback; change the lookup to use fs.lstat when the noFollow option is set so symlinks are detected, and if lstat shows a symlink then skip the in-place ownership-preservation fallback (the branch around ownershipWouldChange()/fs.writeFile for targetPath) to avoid writing through symlinks; update the logic in the atomicFileWrite implementation (referencing targetPath, existingStat, ownershipWouldChange) to treat symlinked targets as non-writable-without-replace and add a regression test for the noFollow + ownershipWouldChange() scenario to ensure the fallback is not used for symlinks.packages/core/src/mcp/token-storage/file-token-storage.ts (1)
126-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle the first credential write when no token file exists.
setCredentials()still callsloadTokens()first, andloadTokens()throws onENOENT. That makes the fresh-install path fail beforesaveTokens()can create the file.Suggested fix
async setCredentials(credentials: OAuthCredentials): Promise<void> { this.validateCredentials(credentials); - const tokens = await this.loadTokens(); + let tokens: Map<string, OAuthCredentials>; + try { + tokens = await this.loadTokens(); + } catch (error) { + if ( + error instanceof Error && + error.message === 'Token file does not exist' + ) { + tokens = new Map(); + } else { + throw error; + } + } const updatedCredentials: OAuthCredentials = { ...credentials, updatedAt: Date.now(), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/mcp/token-storage/file-token-storage.ts` around lines 126 - 136, The setCredentials flow fails on fresh installs because setCredentials() calls loadTokens(), which throws on ENOENT before saveTokens() can create the token file; update setCredentials() to handle a missing token file by catching the ENOENT error from loadTokens() (or check for file existence) and treating it as an empty Map/initial token store, then proceed to set the new OAuthCredentials (with updatedAt) into the tokens Map and call saveTokens() as usual—refer to the methods setCredentials, loadTokens, and saveTokens when implementing this change.packages/core/src/mcp/oauth-token-storage.ts (2)
92-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate the token directory inside
setCredentials().
setCredentials()is public on this storage implementation, but the plaintext-file path still writes directly tomcp-oauth-tokens.jsonwithout callingensureConfigDir(). A direct caller on a fresh profile will getENOENTinstead of persisting credentials.Suggested fix
async setCredentials(credentials: OAuthCredentials): Promise<void> { if (this.useEncryptedFile) { return this.hybridTokenStorage.setCredentials(credentials); } + await this.ensureConfigDir(); const tokens = await this.getAllCredentials(); tokens.set(credentials.serverName, credentials);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/mcp/oauth-token-storage.ts` around lines 92 - 107, setCredentials currently writes the plaintext token file directly (via getTokenFilePath and atomicWriteFile) and will ENOENT on a fresh profile; update OAuthTokenStorage.setCredentials to call ensureConfigDir() (or the existing directory-creation helper used elsewhere) before computing tokenFile/getTokenFilePath so the directory exists when atomicWriteFile is called, keeping the early return for this.useEncryptedFile to delegate to hybridTokenStorage.setCredentials and preserving getAllCredentials behavior.
178-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate delete persistence failures.
If the atomic rewrite or final
unlink()fails here, the method only logs and returns. Callers then believe the credential was deleted even though the token file still contains it.Suggested fix
} catch (error) { debugLogger.error( `Failed to remove MCP OAuth token: ${getErrorMessage(error)}`, ); + throw error; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/mcp/oauth-token-storage.ts` around lines 178 - 193, The delete block currently swallows errors from atomicWriteFile or fs.unlink and only logs them, causing callers to think a token was removed when it may still exist; update the method in oauth-token-storage.ts (the function that manipulates tokenFile and tokenArray using atomicWriteFile and fs.unlink) to rethrow the caught error after logging (or remove the try/catch and let the error bubble up), so callers receive a rejected promise on persistence failures; keep the debugLogger.error call with getErrorMessage(error) but ensure the function propagates the error instead of returning silently.packages/core/src/memory/manager.ts (1)
288-296:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSerialize auto-memory metadata updates across all writers.
This is still a plain read-modify-write on a shared JSON file.
packages/core/src/memory/extract.ts(bumpMetadata()),packages/core/src/memory/forget.ts(bumpMetadata()),packages/core/src/memory/dream.ts(updateDreamMetadataResult()), and thiswriteDreamMetadata()path can all load the same stale snapshot and then atomically replace each other’s fields. The write is crash-safe now, but overlapping extract/dream/forget activity can still lose metadata updates.Please funnel these writes through one helper that locks per
projectRoot(or does a retry-on-change merge) before reading and writing the metadata file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/memory/manager.ts` around lines 288 - 296, The current writeDreamMetadata implementation (and other writers like bumpMetadata in packages/core/src/memory/extract.ts and forget.ts, and updateDreamMetadataResult in dream.ts) perform plain read-modify-write and can clobber concurrent updates; create a single helper (e.g., with a name like writeAutoMemoryMetadataSafely) that serializes writes per projectRoot by acquiring an in-process lock or by implementing read-merge-retry semantics: read the file via getAutoMemoryMetadataPath, merge incoming changes with the latest on-disk metadata (or retry if the file changed), and then persist using atomicWriteFile; replace direct calls to atomicWriteFile in writeDreamMetadata, bumpMetadata, and updateDreamMetadataResult with calls to this helper so all writers funnel through the same per-projectRoot serialization logic.packages/core/src/tools/todoWrite.test.ts (1)
766-783:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the HopCode default path in this assertion.
Hardcoding
.qwen/todosreintroduces the old branding into source and weakens the rename contract this PR is supposed to finish. IfStoragealready moved to the HopCode default, this test will fail for the wrong reason; if it did not, this test now blesses the stale path.Suggested fix
- it('should use default ~/.qwen path when no custom dir is configured', async () => { + it('should use the default HopCode runtime path when no custom dir is configured', async () => { @@ - expect(writePath).toContain(path.join('.qwen', 'todos')); + expect(writePath).toContain(path.join('.hopcode', 'todos')); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/tools/todoWrite.test.ts` around lines 766 - 783, Replace the hardcoded assertion that checks for ".qwen/todos" with an assertion that uses the Storage module's canonical default path instead: after building/executing the tool (tool.build -> invocation.execute) and capturing writePath from mockAtomicWrite, assert that writePath contains path.join(Storage.getDefaultPath(), 'todos') (or the appropriate exported constant name like Storage.DEFAULT_PATH / Storage.DEFAULT_DIR if present) and import/require Storage in the test file if needed so the test validates against the current HopCode default path rather than the old hardcoded string.packages/core/src/tools/todoWrite.ts (1)
336-343:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-validate
modified_contentbefore using it as the source of truth.When
modified_by_useris set,data.todosbypassesvalidateToolParams()entirely. That means malformed editor content can persist duplicate IDs / invalid statuses / empty fields and also feed bad state into the hook phase logic.Suggested fix
if (modified_by_user && modified_content !== undefined) { // User modified the content in external editor, parse it directly const data = JSON.parse(modified_content); finalTodos = Array.isArray(data.todos) ? data.todos : []; } else { // Use the normal todo logic - simply replace with new todos finalTodos = todos; } + + const validationError = this.config + ? new TodoWriteTool(this.config).validateToolParams({ todos: finalTodos }) + : null; + if (validationError) { + return { + llmContent: `Failed to modify todos. + +<system-reminder> +Todo list modification failed validation: ${validationError}. +</system-reminder>`, + returnDisplay: validationError, + }; + }As per coding guidelines, "Validate all user input server-side using schema validators like zod, joi, or class-validator; never rely on client-side validation alone."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/tools/todoWrite.ts` around lines 336 - 343, When modified_by_user is true and you parse modified_content into data.todos (used to set finalTodos), re-validate and sanitize that parsed list before accepting it as the source of truth: run each item through the same validation used elsewhere (e.g., validateToolParams() or the todo schema) to enforce unique ids, allowed status values, and required non-empty fields, remove or dedupe invalid entries, and if validation fails or results in an empty/invalid set, fall back to the original todos and emit a warning/error; apply this validation step where modified_content is parsed (the block that assigns finalTodos from data.todos) so editor changes cannot bypass server-side schemas.packages/core/src/lsp/NativeLspService.ts (1)
1311-1381:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProtect
applyTextEdits()with a per-file lock.This path is now an async read-modify-write with several
awaits betweenreadFile()andatomicWriteFile(). Two overlapping edits for the samefilePathcan both diff against the same base content and the second rename will silently clobber the first.atomicWriteFile()avoids partial writes, but it does not prevent lost updates.Please serialize by resolved path before the read step, or route all file edits for a path through a single queue/mutex.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/lsp/NativeLspService.ts` around lines 1311 - 1381, The applyTextEdits path must be serialized per resolved file path to avoid lost updates: acquire a per-file mutex/lock (use jsonl-utils' getFileLock or the existing file lock facility) for the resolved filePath before the initial readFile() and hold it until after atomicWriteFile() completes, wrapping the read-modify-write in a try/finally that releases the lock; if you don't have a lock helper, create a Map<resolvedPath, Mutex> and await lock.acquire(resolvedPath) before reading and always lock.release/resolvedPath in finally after atomicWriteFile().packages/sdk-typescript/src/daemon/types.ts (1)
706-713:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the duplicated
iznentry from the exported mode list.
DAEMON_APPROVAL_MODESis a runtime contract, not just a type helper. Withiznlisted twice, SDK consumers that render the advertised sequence will surface a duplicate approval mode option.Proposed fix
export const DAEMON_APPROVAL_MODES = [ 'plan', 'default', 'auto-edit', 'auto', 'izn', - 'izn', ] as const;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk-typescript/src/daemon/types.ts` around lines 706 - 713, DAEMON_APPROVAL_MODES contains a duplicated 'izn' entry which causes consumers to see a repeated approval option; remove the duplicate so the array contains each mode only once (i.e., keep a single 'izn') and preserve the as const assertion on DAEMON_APPROVAL_MODES so the runtime export and inferred literal types remain unchanged.packages/cli/src/serve/server.test.ts (1)
2287-2294:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove duplicated
iznfrom allowed approval modes expectation.The expected
allowedarray contains'izn'twice, which makes this contract assertion inaccurate and can hide regressions in mode-list shape.Suggested fix
expect(missing.body.allowed).toEqual([ 'plan', 'default', 'auto-edit', 'auto', 'izn', - 'izn', ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/serve/server.test.ts` around lines 2287 - 2294, The test assertion for the response in server.test.ts incorrectly expects a duplicated 'izn' value; update the expectation on missing.body.allowed to contain each approval mode exactly once by removing the duplicate 'izn' entry so the array becomes unique (refer to the test variable "missing" and its property "body.allowed" where the expect(...).toEqual([...]) is defined).packages/cli/src/ui/hooks/useAutoAcceptIndicator.test.ts (1)
216-225:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate
IZNexpectation in Shift+Tab cycling testIn
packages/cli/src/ui/hooks/useAutoAcceptIndicator.test.ts, theshould cycle approval modes when Shift+Tab is pressedtest expectsApprovalMode.IZNagain on the next Shift+Tab after reachingIZN, butuseAutoAcceptIndicatorcycles sequentially throughAPPROVAL_MODES(DEFAULT → AUTO_EDIT → AUTO → IZN → PLAN) and there’s already a dedicated test assertingIZN → PLANon Shift+Tab. Update the expectations around the secondIZNassertion (lines ~216-225) toApprovalMode.PLAN.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/ui/hooks/useAutoAcceptIndicator.test.ts` around lines 216 - 225, The test for useAutoAcceptIndicator incorrectly expects ApprovalMode.IZN twice when simulating Shift+Tab; change the second assertion to expect ApprovalMode.PLAN and update the corresponding mockConfigInstance.setApprovalMode expectation and result.current check to ApprovalMode.PLAN (references: useAutoAcceptIndicator, ApprovalMode, capturedUseKeypressHandler, mockConfigInstance.setApprovalMode, result.current).
🧹 Nitpick comments (2)
packages/core/src/mcp/token-storage/file-token-storage.test.ts (1)
122-209: ⚡ Quick winAdd a fresh-store
setCredentials()regression test.All of the updated
setCredentials()cases preload an encrypted file. That misses the empty-profile path whereloadTokens()rejects onENOENTbefore the first write happens.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/mcp/token-storage/file-token-storage.test.ts` around lines 122 - 209, Add a regression test exercising the "fresh-store" path where loadTokens() hits ENOENT before the first write: in the describe('setCredentials') suite add a new it('should create store and save credentials when no file exists') that mocks mockFs.readFile to reject with an ENOENT ErrnoException, mocks mockFs.mkdir to resolve, vi.mocked(atomicWriteFile) to resolve, then calls storage.setCredentials(...) and asserts atomicWriteFile was called, mockFs.mkdir was called with path.join('/home/test', '.hopcode') and { recursive: true, mode: 0o700 }, and that the written payload (decrypt via storage['decrypt'] on the atomicWriteFile call arg) contains the new credentials under the serverName; this covers the empty-profile branch where loadTokens() throws ENOENT before the first write.packages/core/src/core/logger.test.ts (1)
32-50: ⚡ Quick winPrefer deriving the temp path from
Storageinstead of duplicating its formula.This hard-codes
.qwen/tmp/<hash>in the test, so any futureStorage.getProjectTempDir()layout change will break the suite for storage-internal reasons rather than logger behavior. Please compute the expected path fromnew Storage(projectDir).getProjectTempDir()after the HOME override and reuse that value here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/core/logger.test.ts` around lines 32 - 50, The test currently hard-codes the temp path (.qwen/tmp/<hash>); instead instantiate Storage with the test projectDir (new Storage(projectDir)) after you set process.env.HOME to TEST_HOME_DIR and call storage.getProjectTempDir() to compute the expected testGeminiDir/testLogFilePath/testCheckpointFilePath in setTestPaths; update setTestPaths to use Storage.getProjectTempDir() (referencing the Storage class and its getProjectTempDir method) and derive LOG_FILE_NAME and CHECKPOINT_FILE_NAME from that returned directory so the test follows storage's canonical path logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/users/configuration/_meta.ts`:
- Line 4: The branding key 'qwen-ignore' in the exported metadata map should be
renamed to match the actual doc filename 'hopcode-ignore.md'; update the key in
docs/users/configuration/_meta.ts from 'qwen-ignore' to 'hopcode-ignore' (where
the object entry currently reads 'qwen-ignore': 'Ignoring Files') so the
branding/substitution and route mapping align with the hopcode-ignore.md
document.
In `@docs/users/configuration/themes.md`:
- Line 41: Replace the outdated HopCode branding in the sentence that currently
reads "Qwen Code" and references "Qwen Dark" / "Qwen Light": update the text to
"HopCode" and "HopCode Dark" / "HopCode Light" (i.e., find the string "Qwen
Code" and "Qwen Dark" / "Qwen Light" in docs/users/configuration/themes.md and
replace them with "HopCode" and "HopCode Dark" / "HopCode Light" respectively).
In `@docs/users/features/commands.md`:
- Around line 241-251: The markdown examples showing terminal/UI mockups in the
docs (the fenced code blocks around the diff header and the "3 files changed,
+45 / -12" lines) are missing language specifiers; update those fenced code
blocks to include a language identifier such as `text` or `console` (e.g.,
change fenced blocks starting with ``` to ```text) so they get proper syntax
highlighting and satisfy markdownlint-cli2 rules—apply this to both the block
around the diff header and the subsequent example output block referenced in the
comment.
In `@packages/core/src/config/config.ts`:
- Around line 249-252: Update the ApprovalMode.IZN entry in APPROVAL_MODE_INFO
so its description matches the runtime behavior of needsConfirmation(): change
the misleading "Automatically approve all tools" to a precise phrase like
"Automatically approve tool actions except 'ask_user_question', which still
requires user confirmation" (or similar), ensuring the description references
ApprovalMode.IZN and the needsConfirmation() exception so UI/protocol consumers
render the correct behavior.
In `@packages/core/src/core/logger.ts`:
- Line 191: Atomic writes to persisted conversation/checkpoint files are
currently using default file mode and symlink behavior; update every
atomicWriteFile call in packages/core/src/core/logger.ts (including the call at
the shown location and the other occurrences around the ranges noted) to pass
secure options: include encoding:'utf-8' plus mode:0o600, forceMode:true, and
noFollow:true so new writes enforce owner-only permissions and avoid following
symlinks; locate calls to atomicWriteFile in the Logger class (and any
checkpoint write helpers) and add these options to their option argument
objects.
In `@packages/core/src/hooks/trustedHooks.test.ts`:
- Around line 17-20: The test mock and path assertion still reference the old
Storage.getGlobalQwenDir API; update the mock to implement
Storage.getGlobalHopCodeDir (e.g., vi.mock('../config/storage.js', () => ({
Storage: { getGlobalHopCodeDir: vi.fn(() => '/mock/home/.hopcode'), }, }))) so
TrustedHooksManager receives the expected value, and update the path
assertion(s) that previously checked the Qwen directory to assert the HopCode
directory instead; ensure both occurrences (lines ~17-20 and ~68-70) are updated
to use getGlobalHopCodeDir and the new expected path.
In `@packages/core/src/hopcode/sharedTokenManager.test.ts`:
- Around line 275-307: The test references a non-existent helper
createMockQwenClient causing type errors; replace that call with the existing
helper createMockHopCodeClient (or import/define createMockQwenClient
equivalently) so the test uses the defined mock helper, e.g., change
createMockQwenClient(createExpiredCredentials()) to
createMockHopCodeClient(createExpiredCredentials()) where
tokenManager.getValidCredentials(mockClient) is invoked and ensure any
mockClient shape matches expected methods (refreshAccessToken) used in the test.
In `@packages/core/src/utils/atomicFileWrite.ts`:
- Around line 521-535: The sync path in atomicFileWrite.ts lost the
uid-preserving overwrite behavior: when existing file ownership differs we must
avoid replacing the inode (which changes ownership) like the async helper does;
modify the sync flow (around the existingMode logic and before calling
renameWithRetrySync) to call the same ownershipWouldChange check used by the
async helper and, on true, perform an inode-preserving overwrite (mirror the
async branch that avoids rename/replace) so ownership is preserved; update
atomicWriteFileSync/renameWithRetrySync-related logic accordingly and add the
same inode-preservation regression test used for the async helper to the sync
test suite to prevent regressions.
In `@packages/core/src/utils/installationManager.ts`:
- Around line 36-38: The atomic write for installationId
(atomicWriteFileSync(installationIdFile, installationId, ...)) currently uses
default umask; update the call to explicitly set restrictive file options to
match other user-scoped state files: pass a mode of 0o600 (owner read/write
only) and use flags/options that prevent symlink-following (O_NOFOLLOW or
equivalent/noFollow flag or 'wx' semantics depending on the atomicWriteFileSync
implementation) so the installationIdFile is created with private, no-follow
permissions; ensure you reference atomicWriteFileSync, installationIdFile and
installationId when making the change.
In `@packages/core/src/utils/projectSummary.ts`:
- Around line 89-91: The atomic write call using atomicWriteFile with statePath
can follow symlinks and allow escapes; update the code that computes and writes
statePath (the variable statePath and the atomicWriteFile(...) call) to (1)
resolve the final path with path.resolve(process.cwd(), '.hopcode', '<file>')
and validate the resolved path startsWith the repository .hopcode root to
prevent traversal, and (2) pass { noFollow: true, encoding: 'utf-8' } to
atomicWriteFile so symlink targets are not followed; ensure the code handles a
failed validation by aborting the write and returning/logging an error for files
such as welcome-back-state.json.
---
Outside diff comments:
In `@packages/cli/src/serve/server.test.ts`:
- Around line 2287-2294: The test assertion for the response in server.test.ts
incorrectly expects a duplicated 'izn' value; update the expectation on
missing.body.allowed to contain each approval mode exactly once by removing the
duplicate 'izn' entry so the array becomes unique (refer to the test variable
"missing" and its property "body.allowed" where the expect(...).toEqual([...])
is defined).
In `@packages/cli/src/ui/hooks/useAutoAcceptIndicator.test.ts`:
- Around line 216-225: The test for useAutoAcceptIndicator incorrectly expects
ApprovalMode.IZN twice when simulating Shift+Tab; change the second assertion to
expect ApprovalMode.PLAN and update the corresponding
mockConfigInstance.setApprovalMode expectation and result.current check to
ApprovalMode.PLAN (references: useAutoAcceptIndicator, ApprovalMode,
capturedUseKeypressHandler, mockConfigInstance.setApprovalMode, result.current).
In `@packages/core/src/hooks/hookRunner.ts`:
- Around line 572-579: expandCommand() currently rewrites $GEMINI_PROJECT_DIR
and $CLAUDE_PROJECT_DIR but not $HOPCODE_PROJECT_DIR, so hooks using that
placeholder won't be substituted; update the expandCommand function to also
recognize and replace HOPCODE_PROJECT_DIR (including cross-shell variants like
$HOPCODE_PROJECT_DIR, ${HOPCODE_PROJECT_DIR}, and PowerShell-style
$env:HOPCODE_PROJECT_DIR) with the same input.cwd value that hookRunner.ts
injects into env, ensuring consistency between the env construction and
placeholder expansion.
In `@packages/core/src/lsp/NativeLspService.ts`:
- Around line 1311-1381: The applyTextEdits path must be serialized per resolved
file path to avoid lost updates: acquire a per-file mutex/lock (use jsonl-utils'
getFileLock or the existing file lock facility) for the resolved filePath before
the initial readFile() and hold it until after atomicWriteFile() completes,
wrapping the read-modify-write in a try/finally that releases the lock; if you
don't have a lock helper, create a Map<resolvedPath, Mutex> and await
lock.acquire(resolvedPath) before reading and always lock.release/resolvedPath
in finally after atomicWriteFile().
In `@packages/core/src/mcp/oauth-token-storage.ts`:
- Around line 92-107: setCredentials currently writes the plaintext token file
directly (via getTokenFilePath and atomicWriteFile) and will ENOENT on a fresh
profile; update OAuthTokenStorage.setCredentials to call ensureConfigDir() (or
the existing directory-creation helper used elsewhere) before computing
tokenFile/getTokenFilePath so the directory exists when atomicWriteFile is
called, keeping the early return for this.useEncryptedFile to delegate to
hybridTokenStorage.setCredentials and preserving getAllCredentials behavior.
- Around line 178-193: The delete block currently swallows errors from
atomicWriteFile or fs.unlink and only logs them, causing callers to think a
token was removed when it may still exist; update the method in
oauth-token-storage.ts (the function that manipulates tokenFile and tokenArray
using atomicWriteFile and fs.unlink) to rethrow the caught error after logging
(or remove the try/catch and let the error bubble up), so callers receive a
rejected promise on persistence failures; keep the debugLogger.error call with
getErrorMessage(error) but ensure the function propagates the error instead of
returning silently.
In `@packages/core/src/mcp/token-storage/file-token-storage.ts`:
- Around line 126-136: The setCredentials flow fails on fresh installs because
setCredentials() calls loadTokens(), which throws on ENOENT before saveTokens()
can create the token file; update setCredentials() to handle a missing token
file by catching the ENOENT error from loadTokens() (or check for file
existence) and treating it as an empty Map/initial token store, then proceed to
set the new OAuthCredentials (with updatedAt) into the tokens Map and call
saveTokens() as usual—refer to the methods setCredentials, loadTokens, and
saveTokens when implementing this change.
In `@packages/core/src/memory/manager.ts`:
- Around line 288-296: The current writeDreamMetadata implementation (and other
writers like bumpMetadata in packages/core/src/memory/extract.ts and forget.ts,
and updateDreamMetadataResult in dream.ts) perform plain read-modify-write and
can clobber concurrent updates; create a single helper (e.g., with a name like
writeAutoMemoryMetadataSafely) that serializes writes per projectRoot by
acquiring an in-process lock or by implementing read-merge-retry semantics: read
the file via getAutoMemoryMetadataPath, merge incoming changes with the latest
on-disk metadata (or retry if the file changed), and then persist using
atomicWriteFile; replace direct calls to atomicWriteFile in writeDreamMetadata,
bumpMetadata, and updateDreamMetadataResult with calls to this helper so all
writers funnel through the same per-projectRoot serialization logic.
In `@packages/core/src/tools/todoWrite.test.ts`:
- Around line 766-783: Replace the hardcoded assertion that checks for
".qwen/todos" with an assertion that uses the Storage module's canonical default
path instead: after building/executing the tool (tool.build ->
invocation.execute) and capturing writePath from mockAtomicWrite, assert that
writePath contains path.join(Storage.getDefaultPath(), 'todos') (or the
appropriate exported constant name like Storage.DEFAULT_PATH /
Storage.DEFAULT_DIR if present) and import/require Storage in the test file if
needed so the test validates against the current HopCode default path rather
than the old hardcoded string.
In `@packages/core/src/tools/todoWrite.ts`:
- Around line 336-343: When modified_by_user is true and you parse
modified_content into data.todos (used to set finalTodos), re-validate and
sanitize that parsed list before accepting it as the source of truth: run each
item through the same validation used elsewhere (e.g., validateToolParams() or
the todo schema) to enforce unique ids, allowed status values, and required
non-empty fields, remove or dedupe invalid entries, and if validation fails or
results in an empty/invalid set, fall back to the original todos and emit a
warning/error; apply this validation step where modified_content is parsed (the
block that assigns finalTodos from data.todos) so editor changes cannot bypass
server-side schemas.
In `@packages/core/src/utils/atomicFileWrite.ts`:
- Around line 184-186: The code currently uses fs.stat(targetPath) (setting
existingStat) which follows symlinks and allows an attacker-supplied symlink to
trigger the ownership-preservation in-place fallback; change the lookup to use
fs.lstat when the noFollow option is set so symlinks are detected, and if lstat
shows a symlink then skip the in-place ownership-preservation fallback (the
branch around ownershipWouldChange()/fs.writeFile for targetPath) to avoid
writing through symlinks; update the logic in the atomicFileWrite implementation
(referencing targetPath, existingStat, ownershipWouldChange) to treat symlinked
targets as non-writable-without-replace and add a regression test for the
noFollow + ownershipWouldChange() scenario to ensure the fallback is not used
for symlinks.
In `@packages/sdk-typescript/src/daemon/types.ts`:
- Around line 706-713: DAEMON_APPROVAL_MODES contains a duplicated 'izn' entry
which causes consumers to see a repeated approval option; remove the duplicate
so the array contains each mode only once (i.e., keep a single 'izn') and
preserve the as const assertion on DAEMON_APPROVAL_MODES so the runtime export
and inferred literal types remain unchanged.
---
Nitpick comments:
In `@packages/core/src/core/logger.test.ts`:
- Around line 32-50: The test currently hard-codes the temp path
(.qwen/tmp/<hash>); instead instantiate Storage with the test projectDir (new
Storage(projectDir)) after you set process.env.HOME to TEST_HOME_DIR and call
storage.getProjectTempDir() to compute the expected
testGeminiDir/testLogFilePath/testCheckpointFilePath in setTestPaths; update
setTestPaths to use Storage.getProjectTempDir() (referencing the Storage class
and its getProjectTempDir method) and derive LOG_FILE_NAME and
CHECKPOINT_FILE_NAME from that returned directory so the test follows storage's
canonical path logic.
In `@packages/core/src/mcp/token-storage/file-token-storage.test.ts`:
- Around line 122-209: Add a regression test exercising the "fresh-store" path
where loadTokens() hits ENOENT before the first write: in the
describe('setCredentials') suite add a new it('should create store and save
credentials when no file exists') that mocks mockFs.readFile to reject with an
ENOENT ErrnoException, mocks mockFs.mkdir to resolve, vi.mocked(atomicWriteFile)
to resolve, then calls storage.setCredentials(...) and asserts atomicWriteFile
was called, mockFs.mkdir was called with path.join('/home/test', '.hopcode') and
{ recursive: true, mode: 0o700 }, and that the written payload (decrypt via
storage['decrypt'] on the atomicWriteFile call arg) contains the new credentials
under the serverName; this covers the empty-profile branch where loadTokens()
throws ENOENT before the first write.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c0adc7e-cd55-4976-9d06-d18a08485a08
📒 Files selected for processing (79)
docs/e2e-tests/worktree-phase-d.mddocs/users/configuration/_meta.tsdocs/users/configuration/settings.mddocs/users/configuration/themes.mddocs/users/features/commands.mddocs/users/features/worktree.mdpackages/cli/src/acp-integration/session/Session.test.tspackages/cli/src/acp-integration/session/Session.tspackages/cli/src/acp-integration/session/types.tspackages/cli/src/config/settingsSchema.tspackages/cli/src/config/trustedFolders.test.tspackages/cli/src/config/trustedFolders.tspackages/cli/src/gemini.tsxpackages/cli/src/nonInteractive/types.tspackages/cli/src/serve/server.test.tspackages/cli/src/services/insight/generators/DataProcessor.test.tspackages/cli/src/services/insight/generators/DataProcessor.tspackages/cli/src/services/tips/tipHistory.tspackages/cli/src/ui/commands/hooksCommand.tspackages/cli/src/ui/components/Footer.tsxpackages/cli/src/ui/components/MainContent.tsxpackages/cli/src/ui/components/SettingsDialog.test.tsxpackages/cli/src/ui/components/hooks/HandlerListBody.test.tsxpackages/cli/src/ui/components/hooks/HandlerListBody.tsxpackages/cli/src/ui/components/hooks/HookMatcherDetailStep.test.tsxpackages/cli/src/ui/components/hooks/HooksManagementDialog.test.tsxpackages/cli/src/ui/components/hooks/matcherGrouping.test.tspackages/cli/src/ui/components/hooks/sourceLabels.test.tspackages/cli/src/ui/components/hooks/sourceLabels.tspackages/cli/src/ui/components/shared/VirtualizedList.tsxpackages/cli/src/ui/hooks/useAutoAcceptIndicator.test.tspackages/cli/src/utils/housekeeping/cleanup.test.tspackages/cli/src/utils/housekeeping/cleanup.tspackages/cli/src/utils/housekeeping/scheduler.test.tspackages/cli/src/utils/housekeeping/scheduler.tspackages/cli/src/utils/housekeeping/throttledOnce.tspackages/core/src/config/config.tspackages/core/src/core/coreToolScheduler.test.tspackages/core/src/core/logger.test.tspackages/core/src/core/logger.tspackages/core/src/core/permissionFlow.tspackages/core/src/extension/extensionManager.tspackages/core/src/hooks/hookRunner.tspackages/core/src/hooks/trustedHooks.test.tspackages/core/src/hooks/trustedHooks.tspackages/core/src/hopcode/sharedTokenManager.test.tspackages/core/src/hopcode/sharedTokenManager.tspackages/core/src/index.tspackages/core/src/lsp/NativeLspService.test.tspackages/core/src/lsp/NativeLspService.tspackages/core/src/mcp/oauth-token-storage.test.tspackages/core/src/mcp/oauth-token-storage.tspackages/core/src/mcp/token-storage/file-token-storage.test.tspackages/core/src/mcp/token-storage/file-token-storage.tspackages/core/src/memory/dream.tspackages/core/src/memory/extract.tspackages/core/src/memory/forget.tspackages/core/src/memory/indexer.tspackages/core/src/memory/manager.tspackages/core/src/permissions/permission-manager.test.tspackages/core/src/permissions/permission-manager.tspackages/core/src/services/memoryDiagnosticsDumper.tspackages/core/src/skills/skill-manager.test.tspackages/core/src/tools/monitor.test.tspackages/core/src/tools/monitor.tspackages/core/src/tools/shell.tspackages/core/src/tools/todoWrite.test.tspackages/core/src/tools/todoWrite.tspackages/core/src/utils/atomicFileWrite.test.tspackages/core/src/utils/atomicFileWrite.tspackages/core/src/utils/debugLogger.tspackages/core/src/utils/installationManager.tspackages/core/src/utils/jsonl-utils.test.tspackages/core/src/utils/jsonl-utils.tspackages/core/src/utils/projectSummary.tspackages/sdk-typescript/src/daemon/types.tspackages/vscode-ide-companion/schemas/settings.schema.jsonpackages/web-templates/src/insight/src/App.tsxpackages/web-templates/src/insight/src/Qualitative.tsx
| display: 'hidden', | ||
| }, | ||
| 'hopcode-ignore': 'Ignoring Files', | ||
| 'qwen-ignore': 'Ignoring Files', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for branding consistency in related files
fd -e md -e ts -e json . docs/users/configuration/ | xargs rg -l "qwen-ignore|qwen_ignore|qwenignore" -i
# Verify the actual filename
fd "ignore" docs/users/configuration/Repository: TaimoorSiddiquiOfficial/HopCode
Length of output: 152
Fix incorrect branding key in docs/users/configuration/_meta.ts
docs/users/configuration/_meta.ts contains 'qwen-ignore', but the corresponding doc file is docs/users/configuration/hopcode-ignore.md, so the branding substitution is mismatched/reversed and will likely break navigation/route mapping.
🔧 Proposed fix
- 'qwen-ignore': 'Ignoring Files',
+ 'hopcode-ignore': 'Ignoring Files',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'qwen-ignore': 'Ignoring Files', | |
| 'hopcode-ignore': 'Ignoring Files', |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/users/configuration/_meta.ts` at line 4, The branding key 'qwen-ignore'
in the exported metadata map should be renamed to match the actual doc filename
'hopcode-ignore.md'; update the key in docs/users/configuration/_meta.ts from
'qwen-ignore' to 'hopcode-ignore' (where the object entry currently reads
'qwen-ignore': 'Ignoring Files') so the branding/substitution and route mapping
align with the hopcode-ignore.md document.
|
|
||
| ## Auto Theme Detection | ||
|
|
||
| When the theme is set to `"auto"` (or left unset), Qwen Code automatically detects whether your terminal uses a dark or light background and selects the matching Qwen theme (`Qwen Dark` or `Qwen Light`). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any remaining "Qwen" references in documentation
rg -n -i "qwen" docs/ --type=md
# Check for any qwen references in theme-related code
rg -n "qwen.*theme|theme.*qwen" --type=ts packages/ -iRepository: TaimoorSiddiquiOfficial/HopCode
Length of output: 50388
🏁 Script executed:
#!/bin/bash
rg -n "Qwen Code" docs/ --type=md
rg -n "Qwen Dark|Qwen Light" docs/ --type=md
rg -n -i "qwen\s+dark|qwen\s+light" docs/ --type=mdRepository: TaimoorSiddiquiOfficial/HopCode
Length of output: 2034
🏁 Script executed:
rg -n "HopCode Dark|HopCode Light" docs/ --type=md
rg -n "Dark|Light" docs/users/configuration/themes.mdRepository: TaimoorSiddiquiOfficial/HopCode
Length of output: 2620
🏁 Script executed:
#!/bin/bash
sed -n '35,90p' docs/users/configuration/themes.md
rg -n "Qwen Dark|Qwen Light" docs/users/configuration/themes.md docs/ --type=md
rg -n "HopCode Dark|HopCode Light" docs/users/configuration/themes.md docs/ --type=mdRepository: TaimoorSiddiquiOfficial/HopCode
Length of output: 3761
Fix HopCode branding in Auto Theme Detection text (themes.md).
docs/users/configuration/themes.md line 41 still says “Qwen Code” and selects “Qwen Dark / Qwen Light”; it should use HopCode branding: “HopCode” and “HopCode Dark / HopCode Light”.
🔧 Proposed fix
-When the theme is set to `"auto"` (or left unset), Qwen Code automatically detects whether your terminal uses a dark or light background and selects the matching Qwen theme (`Qwen Dark` or `Qwen Light`).
+When the theme is set to `"auto"` (or left unset), HopCode automatically detects whether your terminal uses a dark or light background and selects the matching HopCode theme (`HopCode Dark` or `HopCode Light`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| When the theme is set to `"auto"` (or left unset), Qwen Code automatically detects whether your terminal uses a dark or light background and selects the matching Qwen theme (`Qwen Dark` or `Qwen Light`). | |
| When the theme is set to `"auto"` (or left unset), HopCode automatically detects whether your terminal uses a dark or light background and selects the matching HopCode theme (`HopCode Dark` or `HopCode Light`). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/users/configuration/themes.md` at line 41, Replace the outdated HopCode
branding in the sentence that currently reads "Qwen Code" and references "Qwen
Dark" / "Qwen Light": update the text to "HopCode" and "HopCode Dark" / "HopCode
Light" (i.e., find the string "Qwen Code" and "Qwen Dark" / "Qwen Light" in
docs/users/configuration/themes.md and replace them with "HopCode" and "HopCode
Dark" / "HopCode Light" respectively).
| ``` | ||
| ┌ /diff · Turn 3 "refactor the auth middleware" ──── 3 files +45 -12 ┐ | ||
| │ │ | ||
| │ ◀ Current · T3 · T2 · T1 ▶ │ | ||
| │ │ | ||
| │ › src/utils/parser.ts +30 -8 │ | ||
| │ src/utils/parser.test.ts +12 -2 │ | ||
| │ README.md +3 -2 │ | ||
| │ │ | ||
| │ ←/→ source · ↑/↓ file · Enter view · Esc close │ | ||
| └─────────────────────────────────────────────────────────────────────┘ |
There was a problem hiding this comment.
Add language specifiers to fenced code blocks.
The code blocks at lines 242-252 and 259-263 should have language identifiers for proper syntax highlighting and accessibility. Since these appear to be example terminal output/UI mockups, consider using text or console as the language identifier.
📝 Proposed fix
-```
+```text
┌ /diff · Turn 3 "refactor the auth middleware" ──── 3 files +45 -12 ┐-```
+```text
3 files changed, +45 / -12As per coding guidelines, this addresses the markdownlint-cli2 warning about missing language specifiers in fenced code blocks.
Also applies to: 258-263
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 241-241: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/users/features/commands.md` around lines 241 - 251, The markdown
examples showing terminal/UI mockups in the docs (the fenced code blocks around
the diff header and the "3 files changed, +45 / -12" lines) are missing language
specifiers; update those fenced code blocks to include a language identifier
such as `text` or `console` (e.g., change fenced blocks starting with ``` to
```text) so they get proper syntax highlighting and satisfy markdownlint-cli2
rules—apply this to both the block around the diff header and the subsequent
example output block referenced in the comment.
| [ApprovalMode.IZN]: { | ||
| id: ApprovalMode.IZN, | ||
| name: 'IZN', | ||
| description: 'IZN mode - Automatically approve all tools', | ||
| name: 'Izn', | ||
| description: 'Automatically approve all tools', |
There was a problem hiding this comment.
Keep the IZN description aligned with the runtime exception.
needsConfirmation() still requires a prompt for ask_user_question in IZN mode, so advertising this as “Automatically approve all tools” is misleading to any UI or protocol consumer that renders APPROVAL_MODE_INFO.
Suggested fix
[ApprovalMode.IZN]: {
id: ApprovalMode.IZN,
name: 'Izn',
- description: 'Automatically approve all tools',
+ description: 'Automatically approve all tools except ask_user_question',
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/config/config.ts` around lines 249 - 252, Update the
ApprovalMode.IZN entry in APPROVAL_MODE_INFO so its description matches the
runtime behavior of needsConfirmation(): change the misleading "Automatically
approve all tools" to a precise phrase like "Automatically approve tool actions
except 'ask_user_question', which still requires user confirmation" (or
similar), ensuring the description references ApprovalMode.IZN and the
needsConfirmation() exception so UI/protocol consumers render the correct
behavior.
| this.logs = await this._readLogFile(); | ||
| if (!fileExisted && this.logs.length === 0) { | ||
| await fs.writeFile(this.logFilePath, '[]', 'utf-8'); | ||
| await atomicWriteFile(this.logFilePath, '[]', { encoding: 'utf-8' }); |
There was a problem hiding this comment.
Lock down persisted conversation files.
These new atomic writes still use the default mode and symlink-following behavior. logs.json and checkpoint files persist raw prompt/conversation content, so on a typical umask 022 they end up readable by other local users, and rewrites preserve any pre-existing permissive mode unless you force it. Please pass restrictive options here (mode: 0o600, forceMode: true, noFollow: true) on every logger/checkpoint write.
Also applies to: 262-265, 465-468, 543-545
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/core/logger.ts` at line 191, Atomic writes to persisted
conversation/checkpoint files are currently using default file mode and symlink
behavior; update every atomicWriteFile call in packages/core/src/core/logger.ts
(including the call at the shown location and the other occurrences around the
ranges noted) to pass secure options: include encoding:'utf-8' plus mode:0o600,
forceMode:true, and noFollow:true so new writes enforce owner-only permissions
and avoid following symlinks; locate calls to atomicWriteFile in the Logger
class (and any checkpoint write helpers) and add these options to their option
argument objects.
| // forceMode without mode falls back to permission preservation — otherwise | ||
| // we'd silently downgrade an existing file's perms to the process umask. | ||
| let existingMode: number | undefined; | ||
| if (!options?.forceMode || options?.mode === undefined) { | ||
| try { | ||
| const stat = fsSync.statSync(targetPath); | ||
| existingMode = stat.mode & 0o7777; | ||
| } catch (err) { | ||
| if (!isNodeError(err) || err.code !== 'ENOENT') { | ||
| throw err; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const desiredMode = existingMode ?? options?.mode; |
There was a problem hiding this comment.
Sync helper dropped the uid-preserving overwrite behavior.
Lines 521-564 only preserve mode; they never mirror the async ownershipWouldChange() guard before renameWithRetrySync(...). On POSIX, overwriting a file owned by another uid will therefore replace the inode and transfer ownership to the current user, which is exactly what the async helper avoids. packages/core/src/extension/extensionManager.ts already uses atomicWriteFileSync(...), so this can affect real config writes. Mirror the async ownership check here and add the same inode-preservation regression test to the sync suite.
Also applies to: 561-564
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/utils/atomicFileWrite.ts` around lines 521 - 535, The sync
path in atomicFileWrite.ts lost the uid-preserving overwrite behavior: when
existing file ownership differs we must avoid replacing the inode (which changes
ownership) like the async helper does; modify the sync flow (around the
existingMode logic and before calling renameWithRetrySync) to call the same
ownershipWouldChange check used by the async helper and, on true, perform an
inode-preserving overwrite (mirror the async branch that avoids rename/replace)
so ownership is preserved; update
atomicWriteFileSync/renameWithRetrySync-related logic accordingly and add the
same inode-preservation regression test used for the async helper to the sync
test suite to prevent regressions.
| atomicWriteFileSync(installationIdFile, installationId, { | ||
| encoding: 'utf-8', | ||
| }); |
There was a problem hiding this comment.
Persist the installation ID with restrictive file options.
The atomic write is good, but this file still inherits default permissions from the process umask. Since the installation ID is a stable user identifier, it should be written with the same private/no-follow posture as the other user-scoped state files in this PR.
Suggested fix
atomicWriteFileSync(installationIdFile, installationId, {
encoding: 'utf-8',
+ mode: 0o600,
+ forceMode: true,
+ noFollow: true,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| atomicWriteFileSync(installationIdFile, installationId, { | |
| encoding: 'utf-8', | |
| }); | |
| atomicWriteFileSync(installationIdFile, installationId, { | |
| encoding: 'utf-8', | |
| mode: 0o600, | |
| forceMode: true, | |
| noFollow: true, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/utils/installationManager.ts` around lines 36 - 38, The
atomic write for installationId (atomicWriteFileSync(installationIdFile,
installationId, ...)) currently uses default umask; update the call to
explicitly set restrictive file options to match other user-scoped state files:
pass a mode of 0o600 (owner read/write only) and use flags/options that prevent
symlink-following (O_NOFOLLOW or equivalent/noFollow flag or 'wx' semantics
depending on the atomicWriteFileSync implementation) so the installationIdFile
is created with private, no-follow permissions; ensure you reference
atomicWriteFileSync, installationIdFile and installationId when making the
change.
| await atomicWriteFile(statePath, JSON.stringify(state, null, 2), { | ||
| encoding: 'utf-8', | ||
| }); |
There was a problem hiding this comment.
Prevent symlink escapes when saving repo-local state.
statePath lives under ${process.cwd()}/.hopcode, so the repository controls that path. atomicWriteFile() follows symlinks by default, which means a malicious checkout can point .hopcode or welcome-back-state.json outside the workspace and turn this into an arbitrary-file overwrite. Please set noFollow: true here and keep the resolved target constrained to the project-owned .hopcode directory.
As per coding guidelines, "Avoid path traversal in file operations; resolve paths and validate they stay within the allowed directory using path.resolve and startsWith checks".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/utils/projectSummary.ts` around lines 89 - 91, The atomic
write call using atomicWriteFile with statePath can follow symlinks and allow
escapes; update the code that computes and writes statePath (the variable
statePath and the atomicWriteFile(...) call) to (1) resolve the final path with
path.resolve(process.cwd(), '.hopcode', '<file>') and validate the resolved path
startsWith the repository .hopcode root to prevent traversal, and (2) pass {
noFollow: true, encoding: 'utf-8' } to atomicWriteFile so symlink targets are
not followed; ensure the code handles a failed validation by aborting the write
and returning/logging an error for files such as welcome-back-state.json.
…tests Remove duplicate 'izn' from: - packages/cli/src/serve/server.test.ts (test assertion array) - packages/cli/src/nonInteractive/types.ts (PermissionMode union) - packages/sdk-typescript/src/daemon/types.ts (DAEMON_APPROVAL_MODES array) - packages/vscode-ide-companion/src/types/approvalModeValueTypes.ts (ApprovalModeValue union) These duplicates were merge artifacts from resolving upstream's AUTO mode addition alongside the existing IZN (formerly YOLO) mode entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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. |
- Update comment-level references in settingsWriter.ts, skillReviewAgentPlanner.ts, throttledOnce.ts, core/config/config.ts, settings.ts - Update user-facing description strings in settingsSchema.ts (~/.qwen/* -> ~/.hopcode/*) - Fix .qwenignore references in policy.ts and workspaceFileSystem.ts - Rename isQwenScopedEnvFile -> isHopCodeScopedEnvFile in settings.ts - Fix notebook-edit.test.ts test title (qwenignored -> hopcodeignored) - Apply all staged fixes from prior sessions: worktreeStartup, install-state, gitWorktreeService, skill-manager, useAutoAcceptIndicator, paths, claude-converter, SettingsCorruptedDialog, gitWorktreeService.symlinks.integ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ainContent test MainContent.tsx reads uiState.pendingHistoryItems (the merged slash + gemini list). The test was setting pendingGeminiHistoryItems which MainContent ignores, causing sourceCopyIndexOffsets on the pending item to be undefined. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The YAML mock checked for 'From qwen dir'/'From agent dir' but the test data now uses 'From hopcode dir'/'From agents dir' after the upstream sync. Also rename projectQwenDir variable to projectHopCodeDir. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/startup/worktreeStartup.ts (1)
462-466:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the re-attach hint to the renamed CLI binary.
This notice still tells users to run
qwen --worktree ..., so the post-rename recovery guidance is now wrong andworktreeStartup.test.tsis asserting the stale command too. Switch the hint tohopcode --worktree ...and update the expectation alongside it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/startup/worktreeStartup.ts` around lines 462 - 466, The user-visible re-attach hint still references the old CLI name "qwen" in the string returned when override?.overrodeResumedWorktree is true and override.overriddenSlug exists; update that literal to "hopcode" (i.e., change `qwen --worktree ${override.overriddenSlug}` to `hopcode --worktree ${override.overriddenSlug}`) and also update the corresponding assertion in worktreeStartup.test.ts to expect the new "hopcode" text; locate the string in worktreeStartup.ts (the return block that composes the Note message) and the test that asserts that message and change both to the new CLI name.packages/cli/src/config/settings.ts (1)
824-842:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't bypass
excludedEnvVarsfor workspace.hopcode/.envfiles.
isHopCodeScopedEnvFilenow exempts any<repo>/.hopcode/.envfromadvanced.excludedEnvVars, not just user-level env files. That means a trusted workspace can reintroduce variables the user explicitly chose to block from project env loading just by moving them from.envto.hopcode/.env, which breaks the setting contract.Suggested fix
- // hopCodeScoped: any `.env` whose immediate parent is `.hopcode` (including - // `<repo>/.hopcode/.env`) — exempt from the user `excludedEnvVars` list. const isHomeScopedEnvFile = userLevelPaths.has(normalizedEnvFilePath); - const isHopCodeScopedEnvFile = - isHomeScopedEnvFile || - path.basename(path.dirname(normalizedEnvFilePath)) === HOPCODE_DIR; for (const key in parsedEnv) { if (Object.hasOwn(parsedEnv, key)) { if ( !isHomeScopedEnvFile && PROJECT_ENV_HARDCODED_EXCLUSIONS.includes(key) ) { continue; } - if (!isHopCodeScopedEnvFile && excludedVars.includes(key)) { + if (!isHomeScopedEnvFile && excludedVars.includes(key)) { continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/config/settings.ts` around lines 824 - 842, The code currently treats any `.hopcode/.env` as "hopCodeScoped" and uses isHopCodeScopedEnvFile to skip the user's excludedVars, allowing workspace `.hopcode` files to bypass user exclusions; update the logic so excludedVars are only bypassed for true home-scoped env files. Concretely, keep the existing isHopCodeScopedEnvFile detection but change the excludedVars check to reference isHomeScopedEnvFile (or otherwise restrict the bypass to isHomeScopedEnvFile only) so PROJECT_ENV_HARDCODED_EXCLUSIONS and excludedVars (the excludedEnvVars equivalent) are still enforced for workspace `.hopcode/.env` files when iterating parsedEnv in that loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/tools/computer-use/install-state.ts`:
- Around line 22-23: installStatePathFor now returns the new path under
"~/.hopcode" but loadInstallState() only reads that path, which loses existing
approvals stored at "~/.qwen/computer-use/installed.json"; add a legacy fallback
or one-time migration: when loadInstallState() (or the code that calls
installStatePathFor) does not find the new file, check the legacy path
"~/.qwen/computer-use/installed.json" (use the same homedir() and join logic)
and if present either copy/rename it atomically to the new
installStatePathFor(home) location or load and persist its contents to the new
path, and log the migration; reference installStatePathFor and
loadInstallState() when implementing this fallback/migration.
---
Outside diff comments:
In `@packages/cli/src/config/settings.ts`:
- Around line 824-842: The code currently treats any `.hopcode/.env` as
"hopCodeScoped" and uses isHopCodeScopedEnvFile to skip the user's excludedVars,
allowing workspace `.hopcode` files to bypass user exclusions; update the logic
so excludedVars are only bypassed for true home-scoped env files. Concretely,
keep the existing isHopCodeScopedEnvFile detection but change the excludedVars
check to reference isHomeScopedEnvFile (or otherwise restrict the bypass to
isHomeScopedEnvFile only) so PROJECT_ENV_HARDCODED_EXCLUSIONS and excludedVars
(the excludedEnvVars equivalent) are still enforced for workspace
`.hopcode/.env` files when iterating parsedEnv in that loop.
In `@packages/cli/src/startup/worktreeStartup.ts`:
- Around line 462-466: The user-visible re-attach hint still references the old
CLI name "qwen" in the string returned when override?.overrodeResumedWorktree is
true and override.overriddenSlug exists; update that literal to "hopcode" (i.e.,
change `qwen --worktree ${override.overriddenSlug}` to `hopcode --worktree
${override.overriddenSlug}`) and also update the corresponding assertion in
worktreeStartup.test.ts to expect the new "hopcode" text; locate the string in
worktreeStartup.ts (the return block that composes the Note message) and the
test that asserts that message and change both to the new CLI name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c86bd51c-3469-4e00-b559-757162322773
📒 Files selected for processing (32)
packages/cli/src/config/config.tspackages/cli/src/config/settings.test.tspackages/cli/src/config/settings.tspackages/cli/src/config/settingsSchema.tspackages/cli/src/nonInteractive/types.tspackages/cli/src/serve/fs/paths.test.tspackages/cli/src/serve/fs/policy.tspackages/cli/src/serve/fs/workspaceFileSystem.tspackages/cli/src/serve/server.test.tspackages/cli/src/startup/worktreeStartup.test.tspackages/cli/src/startup/worktreeStartup.tspackages/cli/src/ui/components/MainContent.test.tsxpackages/cli/src/ui/components/SettingsCorruptedDialog.test.tsxpackages/cli/src/ui/hooks/useAutoAcceptIndicator.test.tspackages/cli/src/utils/housekeeping/throttledOnce.tspackages/core/src/config/config.tspackages/core/src/core/logger.test.tspackages/core/src/extension/claude-converter.test.tspackages/core/src/hooks/trustedHooks.test.tspackages/core/src/hopcode/sharedTokenManager.test.tspackages/core/src/memory/skillReviewAgentPlanner.tspackages/core/src/services/gitWorktreeService.symlinks.integ.test.tspackages/core/src/services/gitWorktreeService.tspackages/core/src/skills/skill-manager.test.tspackages/core/src/tools/computer-use/install-state.test.tspackages/core/src/tools/computer-use/install-state.tspackages/core/src/tools/notebook-edit.test.tspackages/core/src/tools/todoWrite.test.tspackages/sdk-typescript/src/daemon/types.tspackages/vscode-ide-companion/schemas/settings.schema.jsonpackages/vscode-ide-companion/src/services/settingsWriter.tspackages/vscode-ide-companion/src/types/approvalModeValueTypes.ts
💤 Files with no reviewable changes (4)
- packages/sdk-typescript/src/daemon/types.ts
- packages/vscode-ide-companion/src/types/approvalModeValueTypes.ts
- packages/cli/src/serve/server.test.ts
- packages/cli/src/ui/hooks/useAutoAcceptIndicator.test.ts
✅ Files skipped from review due to trivial changes (7)
- packages/cli/src/utils/housekeeping/throttledOnce.ts
- packages/core/src/tools/notebook-edit.test.ts
- packages/core/src/memory/skillReviewAgentPlanner.ts
- packages/cli/src/config/config.ts
- packages/cli/src/serve/fs/workspaceFileSystem.ts
- packages/cli/src/serve/fs/policy.ts
- packages/vscode-ide-companion/src/services/settingsWriter.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/hooks/trustedHooks.test.ts
- packages/core/src/hopcode/sharedTokenManager.test.ts
- packages/core/src/core/logger.test.ts
| if (isWithinRoot(sourceAbs, hopcodeDirAbs)) { | ||
| debugLogger.warn( | ||
| `symlinkConfiguredDirectories: refusing path "${raw}" — ` + | ||
| `the .qwen tree is CLI-managed; symlinking any of it could ` + | ||
| `the .hopcode tree is CLI-managed; symlinking any of it could ` + | ||
| `create a worktrees-inside-worktrees loop or alias CLI metadata.`, | ||
| ); |
There was a problem hiding this comment.
Don't block the entire .hopcode subtree here.
SkillManager now treats <repo>/.hopcode/skills as a first-class project skills directory, so this blanket guard prevents users from sharing project-local skills into worktrees via worktree.symlinkDirectories. That makes untracked project skills disappear inside the worktree even though .hopcode/skills is a legitimate shared path. Please narrow the blocklist to the truly unsafe managed subpaths (.hopcode/worktrees, session metadata, temp dirs, etc.) and keep the realpath check in sync.
Also applies to: 1776-1779
| export function installStatePathFor(home: string = homedir()): string { | ||
| return join(home, '.qwen', 'computer-use', 'installed.json'); | ||
| return join(home, '.hopcode', 'computer-use', 'installed.json'); |
There was a problem hiding this comment.
Preserve legacy install approvals during the path rename.
Line 23 moves the persisted state to ~/.hopcode, but loadInstallState() now only reads the new location. Existing ~/.qwen/computer-use/installed.json approvals will be ignored after upgrade, so users get re-prompted until they approve again. If this rename is meant to be transparent, add a legacy fallback or one-time migration before treating the state as missing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/tools/computer-use/install-state.ts` around lines 22 - 23,
installStatePathFor now returns the new path under "~/.hopcode" but
loadInstallState() only reads that path, which loses existing approvals stored
at "~/.qwen/computer-use/installed.json"; add a legacy fallback or one-time
migration: when loadInstallState() (or the code that calls installStatePathFor)
does not find the new file, check the legacy path
"~/.qwen/computer-use/installed.json" (use the same homedir() and join logic)
and if present either copy/rename it atomically to the new
installStatePathFor(home) location or load and persist its contents to the new
path, and log the migration; reference installStatePathFor and
loadInstallState() when implementing this fallback/migration.
Summary
Syncs 2 new upstream commits from QwenLM/qwen-code into HopCode main:
Branding applied
How to verify
pm run build\ — passes with no errors
pm run typecheck\ — clean
Summary by CodeRabbit
New Features
Documentation
Improvements