chore: sync upstream QwenLM/qwen-code (19 commits)#49
Merged
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.
Merges upstream changes from QwenLM/qwen-code into HopCode, resolving all branding conflicts (HopCode/@hoptrendy/hopcode-core vs Qwen Code/@qwen-code/qwen-code-core). Key upstream changes adopted: - Virtual viewport / VirtualizedList for large conversation histories - Hooks UI overhaul: matcher-aware routing (HookEventMatcherListStep / HookEventHandlerListStep) - New hook events: PostCompact, StopFailure; hookEventSupportsMatcher API - Shell context env vars: getShellContextEnvVars() in hookRunner - Memory diagnostics improvements - AUTO mode improvements - Cleanup housekeeping: general.cleanupPeriodDays setting - Status line: respectUserColors + hideContextIndicator options - /simplify bundled skill added - Qualitative.tsx: conditional hasFeatureSuggestions / hasUsagePatterns guards - docs-site updates and NavToc configurable sections Branding preserved: - Package names: @hoptrendy/hopcode-core - Env vars: HOPCODE_PROJECT_DIR, HOPCODE_EMIT_TOOL_USE_SUMMARIES - Paths: ~/.hopcode/, .hopcode/ - Skill exclusion: .hopcode dirs not misidentified as bundled skill dirs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Review limit reached
More reviews will be available in 18 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds virtualized terminal viewport and input handling, matcher-grouped hooks UI, background housekeeping scheduler and diagnostics, trace capture/normalize/compare tooling, AUTO-mode denial-tracking refinements, shell-context env propagation, defensive core fixes (token coercion, atomic write ownership), extensive tests, and documentation/site updates. ChangesCLI UI, Core, and Alignment Tooling
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Mitm as mitmdump
participant Normalizer as normalize_trace.py
participant Comparator as compare_traces.py
User->>CLI: run_pair_capture.sh (ref_cmd, qwen_cmd)
CLI->>Mitm: start mitmdump with llm_dump.py
Mitm-->>Normalizer: write http.jsonl
CLI->>Normalizer: normalize reference/http.jsonl
CLI->>Normalizer: normalize qwen/http.jsonl
Normalizer->>Comparator: produce normalized JSONs
Comparator-->>CLI: trace.diff (pass/fail)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 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: 322a162b06
ℹ️ 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".
| import { Box, Text } from 'ink'; | ||
| import { theme } from '../../semantic-colors.js'; | ||
| import { useTerminalSize } from '../../hooks/useTerminalSize.js'; | ||
| import { HookType } from '@qwen-code/qwen-code-core'; |
There was a problem hiding this comment.
Use the HopCode core package name
This new file imports from @qwen-code/qwen-code-core, but this workspace only declares and maps @hoptrendy/hopcode-core in packages/cli/package.json and packages/cli/tsconfig.json. The same upstream package name appears in the new housekeeping and virtual-list files, so a normal CLI build/package install will fail module resolution before the CLI can start; switch these new imports to the HopCode core package name used elsewhere in packages/cli/src.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Syncs 19 upstream commits from QwenLM/qwen-code into HopCode, bringing in new UI/UX features (hooks UI + virtualized conversation viewport), new housekeeping/retention behavior, and additional bundled skills—while adapting changes to HopCode’s branding and package layout.
Changes:
- Adds new settings + documentation for terminal-buffer virtualization and
file-historycleanup retention. - Introduces shell subprocess context env var injection for tracing (session/agent/prompt) and updates monitor tool to pass them through.
- Adds bundled
/simplifyskill and updates skill manager tests to include it.
Reviewed changes
Copilot reviewed 137 out of 138 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vscode-ide-companion/schemas/settings.schema.json | Adds/updates settings schema entries (cleanup retention, terminal buffer virtualization, status line options). |
| packages/core/src/utils/shellContextEnv.ts | New helper to inject context env vars into spawned shell subprocesses. |
| packages/core/src/utils/shellContextEnv.test.ts | Unit tests for the new shell context env helper. |
| packages/core/src/utils/runtimeStatus.ts | Updates runtime status file docs to reflect atomicWriteJSON behavior. |
| packages/core/src/utils/atomicFileWrite.ts | Enhances atomic write to preserve ownership in more cases (uid-aware fallback). |
| packages/core/src/tools/monitor.ts | Injects shell context env vars into hook subprocess environment. |
| packages/core/src/skills/skill-manager.test.ts | Extends bundled-skill scanning tests to include simplify. |
| packages/core/src/skills/bundled/simplify/SKILL.md | Adds new /simplify bundled skill definition and workflow. |
| packages/cli/src/config/settingsSchema.ts | Adds settings descriptions (incl. cleanup retention + terminal buffer virtualization). |
| docs/users/reference/keyboard-shortcuts.md | Documents virtualized scrolling behavior and selection bypass. |
| packages/cli/src/ui/components/hooks/HookEventHandlerListStep.tsx | Hooks UI step text/flow updates as part of upstream hooks overhaul. |
| packages/cli/src/ui/components/hooks/HookEventMatcherListStep.tsx | Hooks UI step text/flow updates as part of upstream hooks overhaul. |
| packages/cli/src/ui/components/hooks/matcherGrouping.test.ts | Adds/updates matcher grouping test coverage for hooks UI changes. |
| packages/cli/src/ui/components/hooks/HandlerListBody.test.tsx | Hooks handler list UI test updates. |
| packages/cli/src/ui/components/hooks/HookMatcherDetailStep.test.tsx | Hooks matcher detail UI test updates. |
| packages/cli/src/utils/housekeeping/cleanup.ts | Implements cleanup of old file-history backups based on retention settings. |
| packages/cli/src/utils/housekeeping/throttledOnce.ts | Adds/updates “run at most once per day” throttling helper for housekeeping. |
| packages/cli/src/ui/components/hooks/HandlerListBody.tsx | Hooks UI component updates for upstream hooks overhaul. |
| packages/cli/src/ui/components/hooks/sourceLabels.ts | Adds/updates hook source labeling logic. |
| packages/cli/src/ui/components/hooks/sourceLabels.test.ts | Tests for hook source labeling changes. |
Comment on lines
+9
to
+13
| import { | ||
| Storage, | ||
| FILE_HISTORY_DIR, | ||
| createDebugLogger, | ||
| } from '@qwen-code/qwen-code-core'; |
| opts: CleanupOptions, | ||
| ): Promise<CleanupResult> { | ||
| const result: CleanupResult = { removed: 0, errors: 0 }; | ||
| const root = join(Storage.getGlobalQwenDir(), FILE_HISTORY_DIR); |
Comment on lines
+7
to
+11
| import { mkdir, open, stat, unlink, writeFile } from 'node:fs/promises'; | ||
| import { dirname } from 'node:path'; | ||
| import { createDebugLogger } from '@qwen-code/qwen-code-core'; | ||
|
|
||
| const debugLogger = createDebugLogger('HOUSEKEEPING'); |
Comment on lines
+7
to
+12
| import { Box, Text } from 'ink'; | ||
| import { theme } from '../../semantic-colors.js'; | ||
| import { useTerminalSize } from '../../hooks/useTerminalSize.js'; | ||
| import { HookType } from '@qwen-code/qwen-code-core'; | ||
| import type { HookConfigDisplayInfo } from './types.js'; | ||
| import { getConfigSourceDisplay } from './sourceLabels.js'; |
Comment on lines
+7
to
+10
| import { HooksConfigSource } from '@qwen-code/qwen-code-core'; | ||
| import type { HookConfigDisplayInfo } from './types.js'; | ||
| import { getTranslatedSourceDisplayMap } from './constants.js'; | ||
| import { t } from '../../../i18n/index.js'; |
Comment on lines
+44
to
+48
| <Box marginTop={1}> | ||
| <Text color={theme.text.secondary}> | ||
| {t('To add hooks, edit settings.json directly or ask Qwen.')} | ||
| </Text> | ||
| </Box> |
Comment on lines
+50
to
+54
| <Box minWidth={2}> | ||
| <Text | ||
| color={isSelected ? theme.text.accent : theme.text.primary} | ||
| > | ||
| {isSelected ? '❯' : ' '} |
Comment on lines
+4
to
+12
| allowedTools: | ||
| - agent | ||
| - run_shell_command | ||
| - grep_search | ||
| - read_file | ||
| - write_file | ||
| - edit | ||
| - glob | ||
| --- |
Comment on lines
+40
to
+44
| ## Step 2: Launch three review passes in parallel | ||
|
|
||
| Use the `agent` tool and launch all review passes in a single response so they run concurrently. Each pass must receive the same review scope and diff command. These passes are read-only: each one inspects and reports findings only and must not modify files — all edits happen later in Step 4. | ||
|
|
||
| Keep each review prompt short and focused. Do not paste the full diff into the prompt. Tell each pass to read the diff itself and inspect only files relevant to its findings. |
Comment on lines
+23
to
+38
| export function getShellContextEnvVars(): Record<string, string> { | ||
| const env: Record<string, string> = {}; | ||
|
|
||
| const sessionId = process.env['QWEN_CODE_SESSION_ID']; | ||
| if (sessionId) { | ||
| env['QWEN_CODE_SESSION_ID'] = sessionId; | ||
| } | ||
|
|
||
| // For agent/prompt IDs: explicitly set empty string when no ALS context | ||
| // exists, so that stale values inherited from a parent qwen-code process | ||
| // (via process.env spread) are overwritten rather than leaked. | ||
| const agentId = getCurrentAgentId(); | ||
| env['QWEN_CODE_AGENT_ID'] = agentId ?? ''; | ||
|
|
||
| const promptId = promptIdContext.getStore(); | ||
| env['QWEN_CODE_PROMPT_ID'] = promptId ?? ''; |
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs-site/README.md (1)
9-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Node.js prerequisite in docs-site/README.md (line 9) to match Next.js 16
docs-site/README.mdsaysNode.js 18+, but the app depends onnext@^16.0.8, and Next.js 16.0.x requires Node.js 20.9+ (minimum supported). Update the setup prerequisites accordingly to avoid contributor install failures.🤖 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-site/README.md` at line 9, Update the Node.js prerequisite string "Node.js 18+" in the README (the line containing that exact text) to "Node.js 20.9+" to match the project's Next.js dependency (next@^16.0.8); ensure any adjacent setup notes or install commands referencing Node version are consistent with Node.js 20.9+ and, if present, add a brief note that Next.js 16 requires Node 20.9 or newer.packages/cli/src/ui/commands/btwCommand.test.ts (1)
173-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the tail-history API is the one actually used.
Right now these tests pass even if the implementation falls back to
getHistory(), because both mocks return the same data. Please add assertions thatgetHistoryTail(40, true)is called andgetHistory()is not, so this change really guards the new path.As per coding guidelines, "Write tests for all new features and bug fixes" and "Ensure tests cover edge cases and error handling".
Also applies to: 232-247
🤖 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/commands/btwCommand.test.ts` around lines 173 - 190, The tests currently mock both geminiClient.getHistory and getHistoryTail with identical return values, so add explicit assertions that the tail API is used: after exercising the code, assert that geminiClient.getHistoryTail was called with (40, true) and that geminiClient.getHistory was not called; do the same for the other test block referenced (the one around the second mock group). Locate the mocks for geminiClient and the test cases in btwCommand.test.ts and add vi.expect/expect assertions for getHistoryTail and getHistory to ensure the implementation actually calls getHistoryTail(40, true) and never falls back to getHistory.packages/core/src/core/geminiChat.ts (1)
2678-2717:⚠️ Potential issue | 🟠 Major | ⚡ Quick winZero-valued coercions never clear the previous token state.
When hostile counts are coerced to
0, these truthy guards skip the assignment, sothis.lastPromptTokenCountandlastCachedContentTokenCountkeep their previous non-zero values. That means the next send can still make compaction decisions from stale metadata even though this response was explicitly sanitized.Suggested fix
coercedUsage = { promptTokenCount, totalTokenCount, candidatesTokenCount, cachedContentTokenCount, }; const lastPromptTokenCount = promptTokenCount || totalTokenCount; - if (lastPromptTokenCount) { - // Always update the per-chat counter so this chat (including - // subagents) can make its own compaction decisions. - this.lastPromptTokenCount = lastPromptTokenCount; - // Mirror to the global telemetry only when wired — subagents - // pass `telemetryService=undefined` to keep their context usage - // out of the main session's UI counters. - this.telemetryService?.setLastPromptTokenCount( - lastPromptTokenCount, - ); - } - if (cachedContentTokenCount && this.telemetryService) { - this.telemetryService.setLastCachedContentTokenCount( - cachedContentTokenCount, - ); - } + this.lastPromptTokenCount = lastPromptTokenCount; + this.telemetryService?.setLastPromptTokenCount(lastPromptTokenCount); + this.telemetryService?.setLastCachedContentTokenCount( + cachedContentTokenCount, + );🤖 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/geminiChat.ts` around lines 2678 - 2717, The bug is that falsy zero values from coerceUsageCount are skipped by truthy checks, leaving previous token counters stale; update the guards to check for null/undefined instead of truthiness so zeros are preserved: when handling lastPromptTokenCount (the local variable) assign this.lastPromptTokenCount whenever lastPromptTokenCount != null (not using if (lastPromptTokenCount)), and call this.telemetryService.setLastPromptTokenCount only when telemetryService exists (preserve the telemetryService? pattern) but still allow zero to be passed; do the same for cachedContentTokenCount and setLastCachedContentTokenCount — i.e., replace truthy checks with null/undefined checks around lastPromptTokenCount and cachedContentTokenCount while keeping coercedUsage, coerceUsageCount, and telemetryService references intact.packages/core/src/skills/skill-manager.test.ts (1)
1063-1086:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
simplify.levelin the precedence tests too.These two cases only verify that
simplifysurvives deduplication, not that it still reportslevel === 'bundled'. Adding that assertion would catch source-metadata regressions in the shadowing path.As per coding guidelines, "Write tests for all new features and bug fixes" and "Ensure tests cover edge cases and error handling".
🤖 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/skills/skill-manager.test.ts` around lines 1063 - 1086, Add an assertion that the surviving 'simplify' skill still reports the expected source level in both precedence tests: after obtaining skills from manager.listSkills({ force: true }) in the "should prioritize project-level over bundled skills with same name" and "should prioritize user-level over bundled skills with same name" tests (which use mockReaddirForLevels and setupReviewSkillMocks), assert that the skill with name 'simplify' has level === 'bundled' to ensure shadowing preserves source metadata.
🟠 Major comments (29)
.qwen/skills/agent-reproduce-feature/scripts/run_tmux_capture.sh-25-34 (1)
25-34: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winKeep the pane alive long enough to capture short-lived commands.
A detached tmux session exits as soon as
COMMANDfinishes, so after the default 2-second settle a quick command likeecho hiis usually gone and Line 34 fails with “can't find pane/session”. Please enableremain-on-exitor wrap the command so the pane survives until capture completes.Possible fix
tmux new-session -d -s "${session}" "$@" +tmux set-option -t "${session}" remain-on-exit on cleanup() {🤖 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 @.qwen/skills/agent-reproduce-feature/scripts/run_tmux_capture.sh around lines 25 - 34, The detached tmux session may exit before capture, causing "can't find pane/session"; modify the session creation in run_tmux_capture.sh so the pane survives until capture—either start the session with remain-on-exit enabled (use tmux option remain-on-exit for the created session) or wrap the user command so it blocks until capture completes (e.g., run a shell that executes "$@" then sleeps or waits on a sentinel); ensure the change is applied where tmux new-session -d -s "${session}" "$@" is invoked and keep cleanup(), trap cleanup EXIT and tmux capture-pane -t "${session}" -p -S - > "${out_dir}/tmux-pane.txt" intact.packages/cli/src/services/insight/generators/DataProcessor.ts-164-168 (1)
164-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize the same helpfulness key that the analysis schema requests.
analyzeSession()asks the model forHOPCODE_helpfulness, but this normalizer only readsQwen_helpfulness. In the real path that means every freshly generated facet falls back to'moderately_helpful', so the helpfulness metric is silently lost before caching and aggregation.Possible fix
- Qwen_helpfulness: normalizeInsightEnum( - rawFacet['Qwen_helpfulness'], + Qwen_helpfulness: normalizeInsightEnum( + rawFacet['HOPCODE_helpfulness'] ?? rawFacet['Qwen_helpfulness'], QWEN_HELPFULNESS_LEVELS, 'moderately_helpful', ),🤖 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/services/insight/generators/DataProcessor.ts` around lines 164 - 168, The normalizer in DataProcessor.ts is reading rawFacet['Qwen_helpfulness'] but analyzeSession() produces 'HOPCODE_helpfulness', causing value loss; update the normalization for Qwen_helpfulness to pull the actual key used by the analysis schema (read rawFacet['HOPCODE_helpfulness'] instead of rawFacet['Qwen_helpfulness'], or defensively check both keys and prefer HOPCODE_helpfulness) when calling normalizeInsightEnum(..., QWEN_HELPFULNESS_LEVELS, 'moderately_helpful'); ensure this change is applied where Qwen_helpfulness is constructed so normalizeInsightEnum receives the real helpfulness value.docs-site/scripts/link-public-docs.mjs-6-16 (1)
6-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve script paths from the module, not
process.cwd().These paths are all cwd-relative today. Running
node docs-site/scripts/link-public-docs.mjsfrom the repo root will delete/create the wrongcontent/directory and then miss../docs/.... Anchor everything toimport.meta.urlso the script behaves the same regardless of where it is invoked from.Suggested fix
import { cp, mkdir, rm, symlink } from 'node:fs/promises'; -import { join } from 'node:path'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { PUBLIC_DOC_ROOTS } from '../src/app/public-docs.js'; -const contentDir = 'content'; +const scriptDir = dirname(fileURLToPath(import.meta.url)); +const docsSiteDir = resolve(scriptDir, '..'); +const repoDocsDir = resolve(docsSiteDir, '..', 'docs'); +const contentDir = resolve(docsSiteDir, 'content'); async function linkPublicDocs() { try { await rm(contentDir, { force: true, recursive: true }); await mkdir(contentDir); - await cp('../docs/index.md', join(contentDir, 'index.md')); - await cp('../docs/_meta.ts', join(contentDir, '_meta.ts')); + await cp(resolve(repoDocsDir, 'index.md'), join(contentDir, 'index.md')); + await cp(resolve(repoDocsDir, '_meta.ts'), join(contentDir, '_meta.ts')); for (const root of PUBLIC_DOC_ROOTS) { - await symlink(join('..', '..', 'docs', root), join(contentDir, root)); + await symlink(resolve(repoDocsDir, root), join(contentDir, root)); } } catch (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 `@docs-site/scripts/link-public-docs.mjs` around lines 6 - 16, The script uses cwd-relative paths causing wrong files to be manipulated; in linkPublicDocs change path resolution to be anchored to the script module by deriving a baseDir from import.meta.url (e.g., using fileURLToPath and dirname) and then compute contentDir and docs paths from that baseDir instead of relying on process.cwd(); update the uses of contentDir, the cp source paths ('../docs/index.md', '../docs/_meta.ts') and the symlink target join('..','..','docs', root) to use path.join(baseDir, ...) or a docsDir variable so all rm/mkdir/cp/symlink operations reference resolved absolute paths, and keep PUBLIC_DOC_ROOTS iteration but join each root against the resolved docsDir.packages/cli/src/ui/components/hooks/sourceLabels.test.ts-8-8 (1)
8-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the branded core package import here.
This test still pulls enums from
@qwen-code/qwen-code-core, but the PR objective says the upstream sync should preserve HopCode package names. Leaving the upstream package here can break install/typecheck in the branded workspace.Suggested fix
-import { HooksConfigSource, HookType } from '`@qwen-code/qwen-code-core`'; +import { HooksConfigSource, HookType } from '`@hoptrendy/hopcode-core`';Verify that this workspace no longer depends on the upstream package name and that this import is just a leftover:
#!/bin/bash set -euo pipefail rg -n '`@qwen-code/qwen-code-core`|`@hoptrendy/hopcode-core`' \ --glob 'package.json' \ --glob '*.ts' \ --glob '*.tsx'🤖 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/components/hooks/sourceLabels.test.ts` at line 8, The test imports enums from the upstream package name; replace the import source so it uses the branded core package instead: change the import that brings in HooksConfigSource and HookType to import them from '`@hoptrendy/hopcode-core`' (ensuring the named symbols HooksConfigSource and HookType remain unchanged) and run the workspace search to confirm no other references to '`@qwen-code/qwen-code-core`' remain.packages/cli/src/ui/components/hooks/HookMatcherDetailStep.test.tsx-9-13 (1)
9-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace upstream
@qwen-code/qwen-code-coreimports with@hoptrendy/hopcode-corein hook tests
packages/cli/src/ui/components/hooks/HookMatcherDetailStep.test.tsxstill imports the enums from@qwen-code/qwen-code-core; this should come from the branded@hoptrendy/hopcode-core.rgalso found remaining@qwen-code/qwen-code-coreimports insourceLabels.test.ts,matcherGrouping.test.ts, andHandlerListBody.test.tsx.Suggested fix
import { HookEventName, HooksConfigSource, HookType, -} from '`@qwen-code/qwen-code-core`'; +} from '`@hoptrendy/hopcode-core`';🤖 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/components/hooks/HookMatcherDetailStep.test.tsx` around lines 9 - 13, The tests import HookEventName, HooksConfigSource, and HookType from the wrong package; update the import source in HookMatcherDetailStep.test.tsx (and the other listed tests: sourceLabels.test.ts, matcherGrouping.test.ts, HandlerListBody.test.tsx) to import these enums from `@hoptrendy/hopcode-core` instead of `@qwen-code/qwen-code-core`; locate the import statements that reference HookEventName, HooksConfigSource, or HookType and change only the package specifier to `@hoptrendy/hopcode-core` so the symbols remain the same but come from the branded core package.packages/cli/src/ui/components/hooks/matcherGrouping.test.ts-8-12 (1)
8-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
@qwen-code/qwen-code-coreimport with@hoptrendy/hopcode-core.
packages/cli/src/ui/components/hooks/matcherGrouping.test.tsimportsHookEventName,HooksConfigSource, andHookTypefrom@qwen-code/qwen-code-core, but this workspace doesn’t have that package as a dependency and doesn’t alias it inpackages/cli/tsconfig.jsonorpackages/cli/vitest.config.ts, so the import can fail to resolve/typecheck.Suggested fix
import { HookEventName, HooksConfigSource, HookType, -} from '`@qwen-code/qwen-code-core`'; +} from '`@hoptrendy/hopcode-core`';🤖 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/components/hooks/matcherGrouping.test.ts` around lines 8 - 12, The test imports HookEventName, HooksConfigSource, and HookType from the wrong package; update the import source in packages/cli/src/ui/components/hooks/matcherGrouping.test.ts to import those symbols from "`@hoptrendy/hopcode-core`" instead of "`@qwen-code/qwen-code-core`" (update the import statement that references HookEventName, HooksConfigSource, HookType), then run typecheck/tests to confirm the symbols resolve correctly.packages/core/src/services/memoryDiagnosticsDumper.ts-67-132 (1)
67-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFreeze session-scoped fields before the first await.
resetForNewSession()can run whilecollectMemoryDiagnostics()is in flight. Line 128 then re-readsthis.collectSessionStats()from the new session and overwrites the old dump file with mixed-session metadata. Capture the session stats once up front (or guard phase 2 with a dumper generation/session token) so a pressure event from session A cannot be rewritten with session B state.Suggested fix
async dump( trigger: 'hard' | 'critical', ): Promise<MemoryDumpResult | undefined> { @@ try { const diagnosticsDir = this.ensureDiagnosticsDir(); const sessionId = this.config.getSessionId(); + const sessionStats = this.collectSessionStats(); const timestamp = new Date() .toISOString() .replace(/:/g, '-') .replace(/\./g, '_'); @@ const minimalPayload = { trigger, dumpNumber, timestamp: new Date().toISOString(), memoryUsage: process.memoryUsage(), v8HeapStats: v8.getHeapStatistics(), - session: this.collectSessionStats(), + session: sessionStats, suggestion: this.getSuggestion(trigger), collectionComplete: false, }; @@ const fullPayload = { trigger, dumpNumber, ...diagnostics, - session: this.collectSessionStats(), + session: sessionStats, suggestion: this.getSuggestion(trigger), collectionComplete: 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/services/memoryDiagnosticsDumper.ts` around lines 67 - 132, The dump() implementation currently calls collectMemoryDiagnostics() and then re-reads session-scoped data (this.collectSessionStats(), this.getSuggestion()) after the await, allowing resetForNewSession() to change session state and corrupt the file; fix by capturing all session-scoped fields before the await (e.g. call and store sessionStats = this.collectSessionStats(), suggestion = this.getSuggestion(trigger), sessionId = this.config.getSessionId(), qwenVersion = this.config.getCliVersion() prior to calling collectMemoryDiagnostics()) and then use those captured values when building fullPayload, or alternatively add a dumper-generation token checked before writing Phase 2 to ensure the same session that started the dump is still active; update references in dump(), collectMemoryDiagnostics() call site, and where fullPayload is assembled (use the frozen variables instead of re-calling this.collectSessionStats()/this.getSuggestion()).packages/core/src/services/backgroundShellRegistry.ts-424-490 (1)
424-490:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly mark a shell as notified after delivery succeeds.
Right now a missing callback or a thrown callback still flips
entry.notifiedtotrue, so that terminal transition is dropped forever. Since this feature is the only completion side-channel for background shells, a transient delivery failure becomes permanent data loss.Suggested fix
private emitNotification(entry: ShellTask): void { if (entry.notified) return; - entry.notified = true; if (!this.notificationCallback) { debugLogger.debug( `Notification dropped for shell ${entry.shellId}: no callback registered`, ); @@ try { this.notificationCallback(displayText, xmlParts.join('\n'), meta); + entry.notified = true; } catch (error) { debugLogger.error('Failed to emit shell notification:', error); } }If you want late subscribers to recover missed events too,
setNotificationCallback()should also sweep terminal!notifiedentries and emit them on registration.🤖 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/services/backgroundShellRegistry.ts` around lines 424 - 490, emitNotification currently sets entry.notified = true before delivery, causing lost terminal notifications when delivery fails or no callback is registered; change emitNotification so it does not mark entry.notified until after a successful invocation of this.notificationCallback (i.e., verify this.notificationCallback exists, call it inside try/catch, and only set entry.notified = true after the call completes without throwing), and ensure that if no callback is registered you do not flip notified; additionally, update setNotificationCallback (or on registering a callback) to sweep existing ShellTask entries where !notified and call emitNotification for each so late subscribers receive missed terminal events.packages/cli/src/utils/housekeeping/throttledOnce.ts-53-62 (1)
53-62:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStale-lock takeover can steal an actively running task.
The lock file’s mtime is never refreshed after acquisition, so any task that runs longer than
staleLockMscan have its live lock unlinked by a second process and execute twice. That violates the cross-process exclusivity this helper promises. Please switch this to a renewable lease/heartbeat, or store owner metadata and only steal locks from definitively dead processes.🤖 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/utils/housekeeping/throttledOnce.ts` around lines 53 - 62, The stale-lock takeover currently unlinks opts.lockPath based solely on mtime and can steal locks from active tasks; update the locking in tryAcquire/where lock is created to write owner metadata (e.g., PID and a heartbeat timestamp) into the lock file and either (a) refresh that heartbeat periodically while the owner task runs or (b) on takeover, verify the owner is dead (e.g., check PID liveness) before unlinking; ensure the staleLockMs check uses the heartbeat timestamp from the lock file (not stale mtime), and only perform unlink/tryAcquire when verification fails to confirm the original process is not alive.packages/cli/src/utils/housekeeping/cleanup.ts-56-61 (1)
56-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate unexpected root scan failures instead of treating them as a successful pass.
If
readdir(root)fails with anything other thanENOENT, this function logs and returns a normal{ removed: 0, errors: 0 }result. Inscheduler.ts, that meansrunThrottledOnce()still records the housekeeping marker, so retries are suppressed for the next interval even though nothing was scanned. Please let unexpected root access failures reject (or surface them distinctly to the caller) so the pass is not marked successful.Suggested direction
try { entries = await readdir(root, { withFileTypes: true }); } catch (e) { if (isENOENT(e)) return result; - debugLogger.error('readdir failed', e); - return result; + debugLogger.error('readdir failed', e); + throw e; }🤖 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/utils/housekeeping/cleanup.ts` around lines 56 - 61, The catch in the cleanup routine that calls readdir(root, { withFileTypes: true }) currently logs unexpected errors and returns a normal result; instead, after checking if isENOENT(e) and returning the empty result, rethrow any other errors so callers (e.g., scheduler.ts's runThrottledOnce) can detect and handle scan failures; keep the debugLogger.error call but follow it with throw e for non-ENOENT exceptions so the pass is not marked successful.packages/cli/src/ui/components/shared/VirtualizedList.tsx-522-540 (1)
522-540:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a row-level ErrorBoundary around each item’s rendered content (VirtualizedList.tsx ~522-540)
- The current
try/catchonly guards the synchronousrenderItem(...)call; it won’t catch exceptions thrown later while React renders the returned subtree.VirtualizedListItemrenders{content}directly with no error boundary, so a render-time failure can still unmount the whole list.🤖 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/components/shared/VirtualizedList.tsx` around lines 522 - 540, The per-item try/catch only catches synchronous errors from renderItem but not errors thrown later during React rendering, so add a row-level ErrorBoundary class (e.g., ItemErrorBoundary) and wrap each item's rendered output with it (where VirtualizedListItem renders {content}); on error the boundary should log via debugLogger.debug(`renderItem threw at index ${i}`, err) and render the same fallback Box/Text ("[render error]") to preserve the viewport and avoid leaking details. Ensure the ErrorBoundary accepts an index prop to include in logs and is used around the result of renderItem({ item, index: i }) instead of relying solely on the try/catch.packages/web-templates/src/insight/src/App.tsx-161-164 (1)
161-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe feature gate is checking the wrong branded field name.
This looks for
Qwen_md_additions, but the branded renderer path usesHopcode_md_additions. If a report only contains HOPCODE.md suggestions,showFeaturesstays false and the Improvements section never renders.Qualitative.tsx'shasFeatureSuggestionscheck needs the same rename.Suggested change
const showFeatures = !!data.qualitative && - (hasMeaningfulArray(data.qualitative.improvements?.Qwen_md_additions) || + (hasMeaningfulArray(data.qualitative.improvements?.Hopcode_md_additions) || hasMeaningfulArray(data.qualitative.improvements?.features_to_try));🤖 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/web-templates/src/insight/src/App.tsx` around lines 161 - 164, The feature-gate is checking the wrong branded field: change references of Qwen_md_additions to Hopcode_md_additions so the Improvements section renders when only HOPCODE.md suggestions exist; update the conditional assigning showFeatures (replace data.qualitative.improvements?.Qwen_md_additions) and the helper hasFeatureSuggestions in Qualitative.tsx to check data.qualitative.improvements?.Hopcode_md_additions instead, keeping the existing hasMeaningfulArray validation and logic intact..qwen/skills/agent-reproduce-align/scripts/normalize_trace.py-190-192 (1)
190-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStream the trace file instead of reading it all at once.
read_text().splitlines()loads the entire capture into memory before you normalize anything. Large MITM traces can get big enough to make this tool slow or fail outright.Suggested change
def normalize(path: Path) -> dict[str, Any]: requests = [] - for line_num, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + with path.open(encoding="utf-8") as handle: + for line_num, line in enumerate(handle, 1): if not line.strip(): 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 @.qwen/skills/agent-reproduce-align/scripts/normalize_trace.py around lines 190 - 192, The normalize function currently calls path.read_text().splitlines() which loads the whole file into memory; change it to stream the file by opening the Path (path.open(encoding="utf-8")) and iterate over the file object with enumerate(file, 1) so you process one line at a time (preserve line_num usage and existing logic that consumes each line) to avoid high memory usage for large traces; update any assumptions about trailing newlines accordingly and ensure the file is closed (use context manager) while keeping the function signature normalize(path: Path) unchanged..qwen/skills/agent-reproduce-align/scripts/normalize_trace.py-212-218 (1)
212-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nested record shapes before calling
.get().You skip malformed JSON and non-object top-level lines, but a record like
{"request": "oops"}still throws here and aborts the whole run. Coerce non-dictrequest/responsevalues to{}or skip that record with a warning.Suggested change
- req = raw.get("request") or {} - resp = raw.get("response") or {} + req = raw.get("request") + resp = raw.get("response") + if not isinstance(req, dict) or not isinstance(resp, dict): + print( + f"Warning: skipping invalid record shape on line {line_num} in {path}", + file=sys.stderr, + ) + continue parsed = urlparse(req.get("url", ""))🤖 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 @.qwen/skills/agent-reproduce-align/scripts/normalize_trace.py around lines 212 - 218, The request/response extraction in normalize_trace.py still assumes nested objects and can crash on records like {"request": "oops"}; update the parsing around the req/resp handling in the trace normalization path to validate that raw.get("request") and raw.get("response") are dicts before using .get(), coercing invalid shapes to {} or skipping the record with a warning so the run continues safely.packages/cli/src/ui/components/hooks/sourceLabels.ts-7-7 (1)
7-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
HooksConfigSourceimport with the HopCode core package:packages/cli/src/ui/components/hooks/sourceLabels.tsimportsHooksConfigSourcefrom@qwen-code/qwen-code-core(line 7), but repo manifests only depend on@hoptrendy/hopcode-core, so this import can break builds / cause enum drift. The same@qwen-code/qwen-code-coreimport appears in several hooks UI tests/components, so keep them consistent.Suggested fix
-import { HooksConfigSource } from '`@qwen-code/qwen-code-core`'; +import { HooksConfigSource } from '`@hoptrendy/hopcode-core`';🤖 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/components/hooks/sourceLabels.ts` at line 7, The file imports the enum HooksConfigSource from the wrong package; replace the import of HooksConfigSource from '`@qwen-code/qwen-code-core`' with the equivalent from '`@hoptrendy/hopcode-core`' so the UI component and related hooks/tests use the repo's actual core package (update the import statement that references HooksConfigSource in sourceLabels.ts and any other hooks UI files/tests that import from '`@qwen-code/qwen-code-core`' to instead import from '`@hoptrendy/hopcode-core`').packages/cli/src/config/keyBindings.ts-232-238 (1)
232-238:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake Shift+arrow scroll bindings exclusive.
SCROLL_UP/SCROLL_DOWNcurrently overlap with the existingup/downcommands because this matcher system ignores unspecified modifiers. As a result,Shift+UpandShift+Downstill satisfyNAVIGATION_*,SELECTION_*, andCOMPLETION_*, so the new viewport scroll commands can be swallowed before they ever run.Suggested fix
[Command.NAVIGATION_UP]: [{ key: 'up' }], - [Command.NAVIGATION_DOWN]: [{ key: 'down' }], + [Command.NAVIGATION_DOWN]: [{ key: 'down', shift: false }], // Selection-list nav: arrows + k/j + Ctrl+P/Ctrl+N // ctrl: false on bare k/j skips Ctrl+K and Ctrl+J [Command.SELECTION_UP]: [ - { key: 'up' }, + { key: 'up', shift: false }, { key: 'k', ctrl: false }, { key: 'p', ctrl: true }, ], [Command.SELECTION_DOWN]: [ - { key: 'down' }, + { key: 'down', shift: false }, { key: 'j', ctrl: false }, { key: 'n', ctrl: true }, ], // Auto-completion [Command.ACCEPT_SUGGESTION]: [{ key: 'tab' }, { key: 'return', ctrl: false }], // Completion navigation uses only arrow keys - [Command.COMPLETION_UP]: [{ key: 'up' }], - [Command.COMPLETION_DOWN]: [{ key: 'down' }], + [Command.COMPLETION_UP]: [{ key: 'up', shift: false }], + [Command.COMPLETION_DOWN]: [{ key: 'down', shift: false }], // History navigation - [Command.NAVIGATION_UP]: [{ key: 'up' }], - [Command.NAVIGATION_DOWN]: [{ key: 'down' }], + [Command.NAVIGATION_UP]: [{ key: 'up', shift: false }], + [Command.NAVIGATION_DOWN]: [{ key: 'down', shift: false }],🤖 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/keyBindings.ts` around lines 232 - 238, The Shift+arrow scroll bindings (Command.SCROLL_UP / Command.SCROLL_DOWN) are currently non-exclusive because unspecified modifiers are ignored, so create explicit modifier constraints to prevent matching when other modifiers are present; update the key descriptors for Command.SCROLL_UP and Command.SCROLL_DOWN to include shift: true and explicitly set ctrl: false, meta: false, alt: false (and similarly ensure any other scroll bindings like Command.PAGE_UP, Command.PAGE_DOWN, Command.SCROLL_HOME, Command.SCROLL_END explicitly declare all modifier booleans where needed) so Shift+Up/Down only match the intended SCROLL_* handlers and won’t be swallowed by NAVIGATION_/SELECTION_/COMPLETION_* matchers..qwen/skills/agent-reproduce-align/scripts/compare_traces.py-17-21 (1)
17-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not collapse duplicate tool names during comparison.
Lines 17-21 reduce
toolsto a single entry per name, socompare_request()only inspects the last occurrence for any duplicated tool. If both traces contain two tools with the samenamebut different schemas/required fields,tool_name_counts()still matches and this script can silently miss a real diff.Compare duplicate names per occurrence instead of overwriting them in a flat dict.
Also applies to: 88-108
🤖 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 @.qwen/skills/agent-reproduce-align/scripts/compare_traces.py around lines 17 - 21, The current tool_index function collapses duplicate tool names into one entry, losing earlier occurrences; change tool_index (and the similar construction around lines 88-108) to preserve duplicate tools by occurrence instead of overwriting by name — e.g., return a list of tools or a dict keyed by (name, occurrence_index) or otherwise include the occurrence index in the key so compare_request() and tool_name_counts() iterate and compare tools by position/occurrence rather than by unique name; update any callers (compare_request, tool_name_counts) to consume the per-occurrence structure so differences between tools with identical names are detected.packages/cli/src/ui/components/MainContent.tsx-101-103 (1)
101-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
id === 0as a static history item here too.The virtual path reserves negative IDs for pending entries, but
virtualIsStaticItemusesitem.id > 0. That misclassifies completed history rowid === 0as non-static, even thoughvirtualKeyExtractorstill treats it as a history item (h-0). The result is an inconsistent contract for the first row whenever zero-based history IDs are present.Suggested fix
-const virtualIsStaticItem = (item: HistoryItem) => item.id > 0; +const virtualIsStaticItem = (item: HistoryItem) => item.id >= 0;🤖 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/components/MainContent.tsx` around lines 101 - 103, The virtual history item classification in MainContent is inconsistent: `virtualKeyExtractor` already treats `id === 0` as a history entry, but `virtualIsStaticItem` only returns true for `item.id > 0`. Update `virtualIsStaticItem` so zero-valued history IDs are also considered static, keeping it aligned with the `HistoryItem` ID contract and the existing `virtualKeyExtractor` behavior..qwen/skills/agent-reproduce-feature/scripts/run_with_mitm.sh-82-96 (1)
82-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedaction misses common flag-style credentials before writing
env.txt.The sanitizer only catches token literals and
KEY=valuepatterns. Secrets passed as--api-key <secret>,--token <secret>, or bearer headers can still be persisted in clear text viacommand=${redacted_command}.🔒 Proposed hardening for command redaction
redacted_command="$( # Note: avoid the GNU-only /I (case-insensitive) sed flag — BSD sed # (macOS pre-Sequoia) silently fails to match with /I, so previously # `API_KEY=…`, `Secret=…`, etc. would not be redacted on macOS. Use # explicit per-letter character classes for the case-insensitive # token-name matches; both BSD and GNU sed accept them. printf '%q ' "$@" | sed -E \ -e 's/sk-[A-Za-z0-9_-]{12,}/sk-<redacted>/g' \ -e 's/AKIA[0-9A-Z]{16}/AKIA<redacted>/g' \ -e 's/AIza[0-9A-Za-z_-]{20,}/AIza<redacted>/g' \ -e 's/(ghp|gho|ghu|ghs)_[A-Za-z0-9_]{20,}/gh_<redacted>/g' \ -e 's/github_pat_[A-Za-z0-9_]{20,}/github_pat_<redacted>/g' \ - -e 's/([A-Za-z0-9_.-]*([Aa][Pp][Ii][-_]?[Kk][Ee][Yy]|[Tt][Oo][Kk][Ee][Nn]|[Ss][Ee][Cc][Rr][Ee][Tt]|[Cc][Rr][Ee][Dd][Ee][Nn][Tt][Ii][Aa][Ll])[A-Za-z0-9_.-]*=)[^[:space:]]+/\1<redacted>/g' + -e 's/([A-Za-z0-9_.-]*([Aa][Pp][Ii][-_]?[Kk][Ee][Yy]|[Tt][Oo][Kk][Ee][Nn]|[Ss][Ee][Cc][Rr][Ee][Tt]|[Cc][Rr][Ee][Dd][Ee][Nn][Tt][Ii][Aa][Ll])[A-Za-z0-9_.-]*=)[^[:space:]]+/\1<redacted>/g' \ + -e 's/(--?[Aa][Pp][Ii][-_]?[Kk][Ee][Yy]|--?[Tt][Oo][Kk][Ee][Nn]|--?[Ss][Ee][Cc][Rr][Ee][Tt]|--?[Pp][Aa][Ss][Ss][Ww][Oo][Rr][Dd])(=|\\[[:space:]]+)[^[:space:]]+/\1\2<redacted>/g' \ + -e 's/([Aa]uthorization:\\[[:space:]]+[Bb]earer\\[[:space:]]+)[^[:space:]]+/\1<redacted>/g' )"Also applies to: 98-103
🤖 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 @.qwen/skills/agent-reproduce-feature/scripts/run_with_mitm.sh around lines 82 - 96, The redaction in redacted_command misses flag-style secrets (e.g. --api-key VALUE, --token VALUE) and bearer/header forms, so update the sanitizer (around redacted_command and the sed -E expressions) to also strip flag/value pairs and common header formats before writing env.txt: add regexes that match (case-insensitively) flag names like --api-?key|--token|--secret|--password followed by a non-space token and replace the value with <redacted>, and add patterns for Authorization: Bearer <token> and --header 'Authorization: Bearer ...' or -H forms; ensure these new sed rules run after the existing token and KEY= patterns so all variants are covered.packages/core/src/core/coreToolScheduler.ts-2295-2297 (1)
2295-2297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't clear AUTO fallback state before the abort check.
Line 2295 resets fallback tracking based only on
outcome, but Lines 2297-2318 can still turn that same confirmation into a cancellation whensignal.abortedraces in afteroriginalOnConfirm()resolves. That means an aborted approval incorrectly clears the AUTO denial counters even though this path ultimately records the call as cancelled. MoverecordAutoModeFallbackResolution()after the cancel/abort branch, or gate it on!signal.aborted.🤖 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/coreToolScheduler.ts` around lines 2295 - 2297, The code currently calls recordAutoModeFallbackResolution(callId, outcome) before checking cancellation, which can clear AUTO fallback counters when signal.aborted later flips the result to Cancel; update the logic in coreToolScheduler (around the confirmation handling where originalOnConfirm() is awaited) to either move the recordAutoModeFallbackResolution(callId, outcome) call to after the if (outcome === ToolConfirmationOutcome.Cancel || signal.aborted) branch so it only runs for final non-cancelled outcomes, or wrap that call with a guard that checks !signal.aborted before invoking it; ensure you reference the existing outcome variable, the signal.aborted check, and the recordAutoModeFallbackResolution(callId, outcome) call so the change is applied in the same confirmation handling flow.packages/core/src/config/config.ts-1170-1179 (1)
1170-1179:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTrack session-env ownership per instance, not by “first
Configwins”.
sessionEnvClaimedmakes whicheverConfighappens to be constructed first the permanent owner ofQWEN_CODE_SESSION_ID. If a bootstrap/throwaway instance is created before the real interactive session, the interactiveConfignever claims the env var, andstartNewSession()can later overwrite it from a non-owner instance because ownership is not stored on the instance. That breaks the cross-process/session contract for anything reading this env marker.Also applies to: 2104-2109
🤖 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 1170 - 1179, The module-level sessionEnvClaimed causes the first-constructed Config to permanently own QWEN_CODE_SESSION_ID; change this to instance-owned tracking by replacing the module flag with an instance property (e.g. this.sessionEnvClaimed) in the Config constructor and related methods, set process.env['QWEN_CODE_SESSION_ID']=this.sessionId only when this instance claims it and mark this.sessionEnvClaimed=true, and update startNewSession() to check and honor the instance ownership flag before overwriting the env var (only allow overwrite if the current instance is the owner or explicitly reclaims it), so ownership is tracked per Config instance rather than by a global first-wins flag.packages/cli/src/ui/components/hooks/matcherGrouping.ts-29-41 (1)
29-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t merge mixed sequential and non-sequential handlers into one matcher group.
Right now the group lookup keys only on
matcher, so a latersequential: trueconfig upgrades the entire existing group to sequential. That loses the original per-config behavior for same-matcher mixed setups and can make the matcher-detail UI report the wrong execution mode. Key the group on both matcher and sequential, or stop storingsequentialat the group level.Suggested fix
- let group = hookInfo.matcherGroups.find( - (candidate) => candidate.matcher === normalizedMatcher, - ); + let group = hookInfo.matcherGroups.find( + (candidate) => + candidate.matcher === normalizedMatcher && + (candidate.sequential ?? false) === normalizedSequential, + ); if (!group) { group = { matcher: normalizedMatcher, sequential: normalizedSequential, configs: [], }; hookInfo.matcherGroups.push(group); - } else if (normalizedSequential) { - group.sequential = 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/cli/src/ui/components/hooks/matcherGrouping.ts` around lines 29 - 41, The current grouping finds by matcher only (hookInfo.matcherGroups.find comparing candidate.matcher to normalizedMatcher) and then mutates group.sequential when a later config has normalizedSequential=true, which mixes sequential and non-sequential handlers; update the grouping so groups are keyed by both matcher and sequential (change the find predicate to compare candidate.matcher === normalizedMatcher && candidate.sequential === normalizedSequential) and push a new group when either differs, ensuring you set sequential on the new group via normalizedSequential; alternatively, remove group.sequential entirely and keep sequential on each config in group.configs—use the symbols hookInfo.matcherGroups, normalizedMatcher, normalizedSequential, and group.sequential to locate and modify the logic.packages/cli/src/ui/components/hooks/HandlerListBody.tsx-10-10 (1)
10-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch
HookTypeimports back to@hoptrendy/hopcode-core
packages/cli/src/ui/components/hooks/HandlerListBody.tsximportsHookTypefrom@qwen-code/qwen-code-core; switch it to@hoptrendy/hopcode-coreto match the rest of the hook UI.packages/cli/src/ui/components/hooks/HandlerListBody.test.tsxhas the same dependency mismatch; update it in the same patch.🤖 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/components/hooks/HandlerListBody.tsx` at line 10, The import for HookType is pointing to the wrong package; update the import statement that currently reads "import { HookType } from '`@qwen-code/qwen-code-core`';" in the HandlerListBody component (file containing the HandlerListBody React component) and the corresponding HandlerListBody.test.tsx to instead import HookType from '`@hoptrendy/hopcode-core`' so the HookType source matches the rest of the hook UI..qwen/skills/agent-reproduce-feature/scripts/llm_dump.py-98-104 (1)
98-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact URL userinfo credentials before writing captures.
_redact_urlcurrently preserves credentials embedded in URLs (for example,https://user:pass@host/...), which can leak directly into the JSONL output.🔧 Proposed fix
def _redact_url(url: str) -> str: parsed = urlparse(url) query = [] for key, value in parse_qsl(parsed.query, keep_blank_values=True): query.append((key, "[REDACTED]" if SENSITIVE_KEY_RE.search(key) else value)) - return urlunparse(parsed._replace(query=urlencode(query, doseq=True))) + netloc = parsed.netloc + if "@" in netloc: + _, host_part = netloc.rsplit("@", 1) + netloc = f"[REDACTED]@{host_part}" + return urlunparse( + parsed._replace(netloc=netloc, query=urlencode(query, doseq=True)) + )As per coding guidelines, "Never echo or log secrets (environment variables, tokens, passwords); use masked secrets in CI/CD logs."
🤖 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 @.qwen/skills/agent-reproduce-feature/scripts/llm_dump.py around lines 98 - 104, _redact_url currently leaves URL userinfo (username:password@) intact; update the function (_redact_url) to strip or mask any credentials in the parsed URL's netloc before reassembling. Specifically, detect parsed.username and parsed.password and replace the userinfo portion of netloc with a masked value (e.g. "[REDACTED]" or omit it) while preserving host and port, then use parsed._replace(netloc=clean_netloc, query=...) when calling urlunparse/urlencode so no credentials are written to the JSONL output..qwen/skills/agent-reproduce-feature/scripts/llm_dump.py-106-121 (1)
106-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-UTF8 fallback writes unredacted raw payloads.
On Line 117-Line 121, decode failures are stored as base64 without redaction. That bypasses token masking and can leak secrets present in binary/compressed bodies.
🔧 Proposed fix
def _decode(content: bytes | None) -> dict[str, Any]: if not content: return {"kind": "empty", "text": ""} truncated = len(content) > MAX_BODY content_sample = content[:MAX_BODY] try: text = content_sample.decode("utf-8") except UnicodeDecodeError: - if truncated: - text = content_sample.decode("utf-8", errors="ignore") - else: - return { - "kind": "base64", - "base64": base64.b64encode(content_sample).decode("ascii"), - "truncated": truncated, - } + return { + "kind": "binary", + "size": len(content_sample), + "truncated": truncated, + }As per coding guidelines, "Never echo or log secrets (environment variables, tokens, passwords); use masked secrets in CI/CD logs."
🤖 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 @.qwen/skills/agent-reproduce-feature/scripts/llm_dump.py around lines 106 - 121, The _decode function returns raw base64 for non-UTF8 content which can leak secrets; update the except UnicodeDecodeError branch so it never emits unredacted raw payloads: instead of returning base64(...) directly, either (a) run the base64 string through the project’s secret-masking routine (e.g., mask_secrets(...) or whatever redaction helper exists) before placing it in the "base64" field, or (b) replace the base64 output with a safe placeholder (e.g., {"kind":"binary","text":"<redacted-binary>","truncated":truncated}) if no masker is available; apply this change in the _decode function where the current base64 return is created and ensure the "truncated" flag is preserved.packages/core/src/tools/mcp-client.ts-168-177 (1)
168-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not log raw server error bodies here.
This warning logs arbitrary response text from a remote MCP server on a normal fallback path. Those bodies can include tokens, stack traces, or tenant data, so emitting them at warn level creates a production log-leak surface. Log only the status/server name here, or gate/redact the excerpt behind a debug-only path.
As per coding guidelines, "Never log sensitive data (passwords, tokens, secrets, SSN, credit card numbers); use structured logging with redaction rules."
🤖 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/mcp-client.ts` around lines 168 - 177, The warning currently logs the raw responseBody from readResponseBodyExcerpt which can leak sensitive data; update the block around debugLogger.warn in mcp-client.ts to stop including responseBody in the warn message and instead log only the mcpServerName and response.status; if you still want the body for debugging, emit it at debug level (e.g., debugLogger.debug) or apply a redaction/sanitization step before logging and reference the same symbols (readResponseBodyExcerpt, responseBody, debugLogger.warn, mcpServerName) when making the change.packages/core/src/tools/mcp-client.ts-129-145 (1)
129-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
Requestinputs in the compatibility wrapper.This wrapper only inspects
init, sofetch(new Request(url, { method: 'GET', headers: { Accept: 'text/event-stream' } }))bypasses the 400→405 fallback entirely. That makes the exportedtypeof fetchwrapper diverge from normal fetch semantics and could disable the compatibility path if the SDK starts passingRequestobjects instead of URL+init.Suggested fix
-function isStreamableHttpGetSseRequest(init?: RequestInit): boolean { - const method = (init?.method ?? 'GET').toUpperCase(); +function isStreamableHttpGetSseRequest( + input: RequestInfo | URL, + init?: RequestInit, +): boolean { + const request = input instanceof Request ? input : undefined; + const method = (init?.method ?? request?.method ?? 'GET').toUpperCase(); if (method !== 'GET') { return false; } - const headers = new Headers(init?.headers); + const headers = new Headers(request?.headers); + if (init?.headers) { + for (const [key, value] of new Headers(init.headers).entries()) { + headers.set(key, value); + } + } if (headers.has('last-event-id')) { return false; } @@ export function createStreamableHttpCompatibilityFetch( mcpServerName: string, fetchFn: typeof fetch = globalThis.fetch.bind(globalThis), ): typeof fetch { return async (input, init) => { const response = await fetchFn(input, init); if ( - !isStreamableHttpGetSseRequest(init) || + !isStreamableHttpGetSseRequest(input, init) || !STREAMABLE_HTTP_GET_SSE_FALLBACK_STATUSES.has(response.status) ) { return response; }In the WHATWG fetch API, when `fetch` is called with a `Request` object plus an optional `init`, should method and headers default from the `Request` input and then be overridden by `init`? Also, in `@modelcontextprotocol/sdk` v1.29.0, does `StreamableHTTPClientTransport` call the injected `fetch` with a `Request` object or with `url, init`?Also applies to: 159-163
🤖 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/mcp-client.ts` around lines 129 - 145, isStreamableHttpGetSseRequest currently only inspects the init object and ignores when callers pass a Request, so update it to accept and honor a Request input by first deriving method and headers from the Request (if the first arg is a Request) and then applying overrides from the optional init per fetch semantics (init.method and init.headers should override Request values); ensure header merging follows the same precedence and then detect 'text/event-stream' exactly as before. Apply the same change to the analogous streamability check around lines referenced (the function used at 159-163) so both code paths treat Request objects and url+init calls identically..qwen/skills/agent-reproduce-feature/scripts/capture_state.py-255-291 (1)
255-291:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSymlink TOCTOU gap can make snapshot read outside the intended root.
collect_entries()decides file-vs-symlink first, butentry_for_file()later re-opens the same path by name. A concurrent symlink swap between those steps can redirect reads/hashes to a different target, bypassing the initial classification and potentially leaking unexpected content.Also applies to: 223-244, 129-137
🤖 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 @.qwen/skills/agent-reproduce-feature/scripts/capture_state.py around lines 255 - 291, collect_entries has a TOCTOU: it checks path.is_symlink() then re-opens the same path by name in entry_for_file, which a concurrent symlink swap can exploit; fix by using lstat + safe open-by-fd semantics: replace the name-based open with os.lstat(path) to decide symlink vs file, and for non-symlinks open the file with os.open(..., os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)) then fstat the returned fd to ensure it's a regular file (S_ISREG) before reading/hashing; update entry_for_file (or add entry_for_fd) to accept a file descriptor (or handle reading from the fd) and close the fd after use; apply the same lstat+open-by-fd pattern wherever collect_entries or entry_for_file is used (also at the other mentioned ranges).packages/cli/src/acp-integration/session/Session.test.ts-31-43 (1)
31-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Vitest mock target so
debugLoggerWarnSpycapturesSession.tswarnings
packages/cli/src/acp-integration/session/Session.tsimportscreateDebugLoggerfrom@hoptrendy/hopcode-core(then callsdebugLogger.warn(...)).packages/cli/src/acp-integration/session/Session.test.tsmocks@qwen-code/qwen-code-core, so the spy will only be hit if@hoptrendy/hopcode-coredelegates/re-exports to that module; otherwise the warning assertion can miss.Proposed fix
-vi.mock('`@qwen-code/qwen-code-core`', async (importOriginal) => { +vi.mock('`@hoptrendy/hopcode-core`', async (importOriginal) => { const actual = - await importOriginal<typeof import('`@qwen-code/qwen-code-core`')>(); + await importOriginal<typeof import('`@hoptrendy/hopcode-core`')>(); return { ...actual, createDebugLogger: () => ({ debug: vi.fn(), info: vi.fn(), warn: debugLoggerWarnSpy, error: vi.fn(), }), }; });🤖 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/acp-integration/session/Session.test.ts` around lines 31 - 43, The test currently mocks createDebugLogger on `@qwen-code/qwen-code-core` so debugLoggerWarnSpy never captures warnings from Session.ts which imports createDebugLogger from `@hoptrendy/hopcode-core`; update the mock in Session.test.ts to target the actual module imported by Session.ts by mocking '`@hoptrendy/hopcode-core`' (not '`@qwen-code/qwen-code-core`') and return createDebugLogger that uses debugLoggerWarnSpy (preserving other original exports via importOriginal if needed) so calls to createDebugLogger and subsequent debugLogger.warn(...) in Session.ts hit the spy.
- Replace @qwen-code/qwen-code-core with @hoptrendy/hopcode-core in 12 files - Fix Storage.getGlobalQwenDir() → getGlobalHopCodeDir() in cleanup.ts and scheduler.ts - Fix qwenVersion → hopcodeVersion in memoryDiagnosticsDumper.ts - Fix Qwen_helpfulness → HOPCODE_helpfulness in DataProcessor.ts and test 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. |
Upstream tests stub QWEN_HOME, but Storage.getGlobalHopCodeDir() reads HOPCODE_HOME. Update both test fixtures to use the correct env var so cleanup and scheduler tests can find the temp directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…log tests
The useConfig mock previously created a new object on every call:
useConfig: vi.fn(() => ({ ... }))
Since fetchHooksData = useCallback(..., [config]), a new config reference
on every render caused fetchHooksData to be recreated, re-running the
useEffect([fetchHooksData]) each render. This infinite loop prevented
keypress navigation from working in the three navigation tests.
Fix: create stableConfig once inside the vi.mock factory closure and
always return the same reference from useConfig(). This breaks the
render loop and lets all 8 tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ss tests
The afterEach was calling cleanup() from ink-testing-library, but the
tests use renderWithProviders() from test-utils/render.tsx which maintains
its own separate instances array. The ink-testing-library cleanup is a
no-op for these instances, so components were never unmounted after each
test.
This caused deferred React re-renders from tests 4 and 5 (which use
mockReturnValueOnce and trigger extra renders) to fire during test 6,
contaminating mockedUseKeypress.mock.calls with stale handlers. When
pressKey('return') used mock.calls.at(-1) it would invoke the wrong
handler from a previous test, preventing navigation.
Fix: import cleanup from render.tsx so instances are properly unmounted
between tests. Also retain NAV_TIMEOUT on waitFor calls to handle CI
runners under heavy integration-test load.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merges 19 upstream commits from QwenLM/qwen-code into HopCode's main branch, adapting all changes to HopCode branding and package names.
Upstream Changes Adopted
Branding Preserved
How to Verify
Summary by CodeRabbit
New Features
Enhancements
Documentation