feat(core): add a live-session registry and qwen sessions ps - #8969
Conversation
Records each interactive session at `~/.qwen/sessions/<pid>.json` while it runs, so "which Qwen Code sessions are on this machine right now" is one readdir instead of a walk over every project's transcript directory. This is the discovery surface cross-session messaging needs (QwenLM#8724), landed on its own because it is useful by itself and changes nothing about how a session behaves. Why not extend the existing runtime.json sidecar: it lives under `<projectDir>/chats/<sessionId>.runtime.json` and is never unlinked, so presence carries no liveness signal. The cost of asking it this question is visible in `isSessionRuntimeActive` — ~150 lines of candidate-directory guessing plus a recursive scan, and that only answers whether one *known* session is alive. Enumerating every live session that way is that cost times N. The two coexist: runtime.json stays the kimi-compatible "which session is PID X serving" sidecar for external observers. Staleness is decided by PID liveness plus a start identity of `<boot_id>:<starttime>` read from /proc, so neither a recycled PID nor a reboot can resurrect a dead session's record. `session-writer-lease.ts` composes the same Linux identity and is deliberately left alone — its token is a persisted format with takeover semantics. The new `process-liveness` helpers replace the private copy in teamHelpers. Registry hygiene worth calling out, each one a real failure mode rather than defensive habit: the directory is chmod 0700 on every register (mkdir's mode is umask-masked and does nothing for an existing directory); records are 0600 and written `noFollow`, so a pre-planted symlink cannot redirect a registration write; only `<digits>.json` is ever considered a record, because a lenient prefix match would read `2026-planning-notes.json` as PID 2026 and delete a file this code never wrote; and a record that fails validation is skipped without being swept, since we cannot reason about what we cannot parse. `qwen sessions ps` prints the live sessions; `--json` emits JSON Lines. Record fields come from other processes, so the table renders them through `sanitizeTerminalText` — ANSI, control bytes, and bidi overrides (CVE-2021-42572 class) all matter when DIRECTORY is the column a user relies on to tell two sessions apart. Registration happens after first paint: nothing on screen depends on it, and it is an mkdir plus an fsync'd write. `/clear` and `/resume` patch the record's session id, and a directory switch patches its cwd, but never its name — that name is the handle a user just read out of `ps`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR — the description is unusually thorough, but it skips the PR template, and some of the missing sections carry real signal for this particular change. Could you restructure the body to follow the template?
@qqqys What's missing:
## Reviewer Test Plan—### How to verify(a concrete walkthrough ofqwen sessions psagainst a couple of live sessions, plus the stale-record / recycled-PID / symlink cases your tests pin),### Evidence (Before & After)— this adds a user-visible command, so a short tmux capture or real example output belongs here — and### Tested on. The OS matrix matters for this one specifically: the registry leans on/proc/<pid>/stat+boot_idon Linux, while on Windows the POSIX//procassertions are guarded skips so far.## Risk & Scope— the tradeoffs are all there in your "Things worth a reviewer's attention" section; they just need to live in the template's bullets (main risk or tradeoff / not validated or out of scope / breaking changes).## Linked Issues—Step one of #8724deserves the section. A plain reference is fine for a first step; add a closing keyword only when the whole plan lands.- The
<details>中文说明 block — the template asks for a paragraph-by-paragraph translation.
## What it does and ## Why not runtime.json read fine as the what/why sections — renaming them to the template headings is a formality; the four items above are the substance.
Once the template is in place, re-run with @qwen-code /triage (or push the update and the CI event re-triggers it) and the gate picks up from here. 🙏
中文说明
感谢提交——PR 描述写得非常详尽,但没有遵循 PR 模板,而其中缺失的几节对这个改动恰恰有实际信息量。能否按模板重新组织一下正文?
@qqqys 缺失的部分:
## Reviewer Test Plan——### How to verify(针对几个存活 session 运行qwen sessions ps的具体步骤,以及测试所钉住的 stale-record / PID 复用 / symlink 场景)、### Evidence (Before & After)——本 PR 新增了用户可见命令,这里应附一段 tmux 捕获或真实示例输出——以及### Tested on。OS 矩阵对本 PR 尤其重要:注册表在 Linux 上依赖/proc/<pid>/stat+boot_id,而 Windows 侧的 POSIX//proc断言目前只是带守卫的 skip。## Risk & Scope——权衡都已写在 "Things worth a reviewer's attention" 一节里,只需搬进模板要求的项目符号(主要风险或取舍 / 未验证或超出范围 / 破坏性变更)。## Linked Issues——Step one of #8724值得单独成节。作为第一步,普通引用即可;等整个计划落地时再考虑关闭关键字。<details>中文说明块——模板要求逐段对应翻译。
## What it does 和 ## Why not runtime.json 作为 what/why 两节内容没有问题——改成模板标题只是形式问题;上面四项才是实质。
模板补齐后,用 @qwen-code /triage 重跑(或推送更新由 CI 事件重新触发),审查会从这里继续。🙏
— Qwen Code · qwen3.8-max
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 101 passed · 0 failed · 101 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:101 通过 · 0 失败 · 101 总计 Verification reportPR 8969 — feat(core): add a live-session registry and
|
| Cell (same scenario both arms) | base 3a593323 |
head e0b92717 |
|---|---|---|
| Interactive boot (TUI banner rendered, global dir written) | boots; no sessions/ dir created (5/5 control asserts) |
boots; 2 records in ≤3 s, dir 0700, files 0600, procStart = <boot_id>:<starttime> |
qwen sessions ps |
exit 1, help offers only list |
exit 0, header + one row per session at declared column widths |
qwen sessions ps --json |
n/a | 2 JSON Lines, full 8-field set, empty stdout when no sessions |
/quit one session |
n/a | record unlinked in ≤2 s, sibling untouched |
| SIGKILL (crash) | n/a | record litters, next ps sweeps it → "No other Qwen Code sessions are running." |
/clear mid-session |
n/a | record patched in place: new sessionId, same name handle, same cwd (4/4) |
Assertion totals per arm: head E2E 23/23, base E2E 5/5, patch E2E 4/4.
Secondary claims
- PID-reuse / reboot identity + conservative liveness — harness
02-registry-harness.png(49/49): dead-PID record swept and unlinked; live PID with a foreign token treated as recycled and swept; live PID withnulltoken kept; live PID whose current token is unreadable kept; the recorded token equals an independent parse of/proc/<pid>/statfield 22 plus/proc/sys/kernel/random/boot_id(W1). Mutation M5/M6 show the conservatism and EPERM rules are pinned by tests. - Safety posture — pre-planted and broken symlinks at
<pid>.jsonare replaced, victim file untouched (W8, and mutant M1 kills the test that pins it); foreign filenames (2026-planning-notes.json,12.json.bak,session-2026.json, …) are never parsed and never deleted (W5, M2/M3); filename/content disagreement skipped without sweeping (W6); oversize (>64 KiB), future-schema, malformed records skipped without sweeping (W7, M4); table output strips ANSI CSI/OSC, C0/C1/DEL, bidi overrides, TAB/LF and keeps the PID column at offset 22 under a hostile name (W11, plus sibling sweep S1–S8: DEL, C1-CSI, BEL, LRM, RLO all neutralized).
Reviewer Test Plan walk-through
| Plan step | Result |
|---|---|
| 1. Two sessions in different dirs → one sanitized row each | PASS — CELL1/CELL2, names proj-alpha-70 / proj-beta-6e, age + directory columns correct |
2. --json one object per line |
PASS — exactly 2 lines, parse with full field set |
| 3. Exit one session → its record disappears | PASS — /quit unlinks within 2 s; crash variant sweeps on next ps |
| 4. Focused suites green | PASS — core session-registry + process-liveness 58/58; cli ps + sessions 18/18 |
Mutation matrix (focused core suites, 58 tests unmutated)
| Mutant (single hunk) | Killed by | Result |
|---|---|---|
M1 drop noFollow in registerSession |
registerSession > replaces a pre-planted symlink instead of writing through it |
killed (1 red) |
M2 RECORD_FILENAME → prefix match |
listLiveSessions > never opens a file that is not named <pid>.json |
killed (1 red) |
| M3 drop filename/content agreement | ignores files that are not <pid>.json + skips a record whose pid disagrees with its filename |
killed (2 red) |
| M4 accept future schema (positive control) | skips malformed and future-schema records without deleting them |
killed (1 red) |
| M5 sweep when current token unreadable | isSameProcess > keeps a live session whose token cannot be read right now |
killed (1 red) |
| M6 EPERM treated as dead | isPidAlive > treats EPERM — another user's process — as alive |
killed (1 red) |
| M7 sweep no longer unlinks (vacuity of central tests) | sweeps a record whose process is gone + treats a recycled PID as stale |
killed (2 red) |
7/7 killed, no survivors; the positive control proves the harness can turn the suite red, and M7 proves the central sweep tests fail with the intended behavioral assertion (record still present), not an import error. Witness: 03-mutation-matrix.png.
Targeted gates
- Focused suites (unmutated control): core 58/58, cli 18/18.
- Full workspace suites, head vs base failing-name comparison: head core
70 failed | 19769 passed, cli12 failed | 19101 passed; base produces a byte-identical failing set (diffof sortedFAILlines: IDENTICAL for both packages). All 82 are environmental in this container (logger, ide-client, memoryDiscovery, file-token-storage, skill-manager, subagent-manager, installationManager, rulesDiscovery; cli settings) — none in the changed surface, none introduced by the PR.
Findings
- Low (informational, non-blocking): the documented
--json | jq -r .cwdexample forwards unsanitized same-user text to the consumer's terminal.ps --jsonintentionally emits raw record values (verified: hostile ANSI/bidi bytes round-trip in the JSON line), and the new docs section recommendsqwen sessions ps --json | jq -r .cwd, which renders them raw. I confirmed this is the pre-existing house convention —sessions list --jsonlikewise emitsJSON.stringifyof transcript-derived fields while sanitizing only its table — sopsinherits an existing exposure class rather than introducing one, and the registry's 0700/0600 modes keep the writer set at same-user. Suggested fix, if desired, is a one-line docs note ("JSON output is raw data; sanitize before rendering"), not a code change. - Informational (self-disclosed tradeoff, verified bounded): the
/clear/directory-switch patch rides theruntimeStatusEnabledgate while startup registration does not. With the sidecar disabled, a/clearleaves the record advertising the previoussessionIduntil exit — exactly the cost the commit comment names. With the gate enabled (default), the patch works end-to-end (patch E2E 4/4). No action required; recorded so the divergence is not mistaken for a bug later.
Not covered
- Windows/macOS liveness degradation (
procStart: null→ plain liveness): not executable in this Linux container; the non-Linux branches run only via the suite's mocks. A recycled PID on those platforms would keep a foreign record listed until the PID dies — the disclosed platform tradeoff. - EPERM cross-user liveness and unreadable-registry-dir (EACCES) →
[]paths: cannot be produced as root here; covered by injected-errno unit tests only. - Directory-switch patch hook (
config.tssecondpatchSessionRecordcall): the identical mechanism is verified live via/clearand in the harness; the switch hook itself was not driven through a live TUI. - Repo-wide
typecheck/lint: left to the PR's own CI; not re-run here. - Per-commit attribution: single commit, locally reachable — no gap.
Methodology
Environment: node:22-bookworm CI container, merge-ref checkout at depth 2 (HEAD merge, HEAD^1 base, HEAD^2 PR head). Head cells drove the compiled dist/ output; interactive sessions ran under script(1) ptys with FIFO stdin, auth satisfied by a loopback OpenAI-compatible peer (fake-openai.mjs), registry isolated per cell via QWEN_HOME. The base control is a git worktree at HEAD^1 (full build, node_modules junctioned with @qwen-code/* re-pointed into the base tree; realpath asserted) and was removed after capture — setup-base.sh recreates it for reruns. Harnesses (registry-harness.mjs, sanitize-siblings.mjs, e2e.sh, base-e2e.sh, patch-e2e.sh, mutation-matrix.sh) are mock-free against the code under test and live in this artifact dir; raw logs in logs/, witnesses in evidence/ (01 command-presence A/B, 02 wire-oracle harness, 03 mutation matrix, 04 lifecycle assertions). Assertion counts: harness 49 + siblings 8 + head E2E 23 + base E2E 5 + patch E2E 4 + focused-suite controls 2 + mutation kills/controls 8 + full-suite attribution 2 = 101.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Re-run on Template ✓ — all required headings present. Problem: real, not theoretical. There is today no way to ask "which Qwen Code sessions are running on this machine" — Direction: aligned. Claude Code ships the same feature area — its changelog shows Size: core paths touched. Of 3849 changed lines: 1251 production logic, 2543 test, 55 docs. A Approach: scope is right — one small record per session, one narrow read path, no speculative hooks, and the PR explicitly rejected its predecessor's accretions (the PID-namespace gate was cut, then a bounded version came back through review rounds — see the hygiene note). Every hardening piece in the diff corresponds to a named failure mode rather than defensive habit. Hygiene note, non-blocking: the PR body has drifted behind the code in three places — "339 lines" (session-registry.ts is now 693), "leaves the namespace machinery out" (it is back since Risk: no Stage 1e high-risk path matches. No elevated risk signals. Moving on to code review. 🔍 中文说明在 模板 ✓ —— 必备小节齐全。 问题:真实存在,不是理论加固。目前没有办法回答"这台机器上正在运行哪些 Qwen Code 会话"—— 方向:对齐。Claude Code 已上线同一领域——其 changelog 显示按名字 规模:触及核心路径。3849 行改动中:生产逻辑 1251 行、测试 2543 行、文档 55 行。 方案:范围合理——每会话一条小记录、一条窄读取路径、无投机性钩子,且明确舍弃了前身 PR 的堆积物(PID namespace 闸门曾被砍掉,后经审查轮次以有界形式回归——见下方备注)。diff 中每一处加固都对应一个具名失效场景,而非防御性习惯。非阻塞备注:PR 正文有三处落后于代码——"339 行"(session-registry.ts 现为 693 行)、"舍弃 namespace 机制"( 风险:未命中 Stage 1e 高风险路径。无升级风险信号。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI sketched my own design for this problem before reading the diff — a 0700 What I verified in the diff:
Non-blocking notes (already raised by @wenshao or named by the PR itself): the PR body lags the code in three places (see Stage 1); the macOS/Windows PID-reuse degradation is a declared tradeoff, now quantified rather than reasoned about; sequenceDiagram
participant P1 as Interactive boot
participant P2 as Config write chain
participant P3 as Registry dir
participant P4 as ps reader
P1->>P2: trackSessionRegistration, not awaited
P2->>P3: write pid.json after first paint
P1->>P2: /clear or /cd transition
P2->>P3: patch queued behind registration
P1->>P2: exit cleanup
P2->>P2: drain queued patches
P2->>P3: unlink own record only
P4->>P3: list, check identity, sweep proven-dead
Files changed (20 of 20 shown)
Test evidence — the PR's own CI on
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ✅ success |
| Test (macos-latest, Node 22.x) | ⏭️ skipped (fork gating) |
| Test (windows-latest, Node 22.x) | ⏭️ skipped (fork gating) |
| Integration Tests (CLI, No Sandbox) | ⏭️ skipped |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ✅ success |
| Desktop Shell (ubuntu-22.04) | ✅ success |
| Desktop Shell (windows-2022) | ✅ success |
| precheck-pr / precheck | ✅ success |
| Secret scan (TruffleHog) | ✅ success |
| Dependency CVE audit | ✅ success |
Two independent verification passes back the behavioural claims, at different heads — attributing both precisely:
- The earlier sandboxed
/verifyrun reported 101/101 assertions, merge-ready, but against the previous heade0b92717— before the serialization fix (9f2dbc1b) and the namespace machinery (fe54a9235b) landed (report). - @wenshao then verified this exact head on a real macOS build: two live TUIs discovered, lifecycle and concurrent teardown confirmed, the serialization
awaitproven load-bearing by A/B (removed → 10/12 early exits leak), 11/11 mutants killed, 22/22 guard-matrix assertions — and quantified the macOS PID-reuse tradeoff by walking the PID counter 169,345 forks around a stranded record (his report). This is maintainer evidence, not bot evidence — I have not re-run it, and this CI run is unattended.
Not verified here: Linux-only identity paths (boot-id prefix, namespace guard, zombie handling) are exercised by unit tests and the earlier /verify head, but not by a real-build run on the current head; Windows is unverified beyond skipped CI.
Sandboxed verification would settle the remainder: @qwen-code /verify — a sponsored run against fd768c90 would A/B the Linux-only identity paths against the current head, which neither the stale /verify report nor the macOS-only maintainer run covers. The author lacks write access, so this takes a maintainer's comment, and the resulting report should be read with the same skepticism as the fork's own CI logs — the sandbox bounds what the code can do, not what the report can say.
中文说明
代码审查
读 diff 之前我先独立勾画了方案——0700 的 ~/.qwen/sessions/ 目录、以 <pid>.json 为键、围绕交互式 UI 的尽力注册/注销、kill(pid, 0) 加平台可用时的 /proc 启动时刻令牌、严格文件名守卫、只清扫可证明已死的记录、输出净化。PR 与该形状几乎完全一致,并在值得的地方走得更远,而不是堆砌。未发现阻塞问题。
已核对:身份与存活判定(field 22 以最后一个 ) 锚定解析、EPERM/EACCES 视为存活、僵尸视为死亡、boot id 缓存不固化失败);注册表(严格文件名守卫、文件名/内容一致、namespace 与 boot 双边界隔离、unlink 前重读闭合竞态、写路径"无法比对即非我方"的正确不对称);生命周期接线(独立串行链、退出前 drain——@wenshao 的 A/B 证明该 await 承重);命令本身(复用既有共享 helper、--json 按设计原样输出且文档已加提醒);顺带改动均为改进(teamHelpers 共享 isPidAlive 修复了 EACCES 误判、atomicFileWrite 的 noFollow lstat 堵住符号链接旁路)。非阻塞备注:PR 正文三处滞后于代码(见 Stage 1);macOS/Windows PID 复用退化是已声明的取舍且已被量化;sessions/list.ts 私有 sanitizer 的 bidi 漏洞属另一 issue。
测试证据
fd768c90 上所有检查已完成、无红色。macOS/Windows 单测任务因 fork PR 门禁被跳过(非失败);该 head 无待处理工作流。两次独立验证支撑行为声明,但 head 不同,分别如实标注:此前的沙箱 /verify 在旧 head e0b92717 上 101/101 通过(早于串行化修复与 namespace 机制);@wenshao 随后在本 head 上以真实 macOS 构建复验(注册/注销/并发拆解、串行化 A/B 证明承重、11/11 变异体被杀、22/22 守卫矩阵,并量化了 macOS PID 复用取舍)。后者是维护者证据而非机器人证据——本次 CI 运行为无人值守,我未重跑。此处未验证:Linux 专有身份路径在真实构建上的运行(仅单测与旧 head 的 /verify 覆盖)、Windows 除被跳过的 CI 外的行为。沙箱验证可补齐余项:由维护者评论发起的 @qwen-code /verify 协办运行可在当前 head 上对 Linux 专有路径做 A/B;作者无写权限,报告应与 fork 自身 CI 日志同样审慎对待。
— Qwen Code · qwen3.8-max
Reviewed at fd768c9058b0b4df8efb4274a52a61649eeec97b · re-run with @qwen-code /triage
|
Confidence: 3/5 — clean review with no blockers found, and the maintainer sign-off this gate asked for last round has arrived; the 3 is pure policy, not doubt — a Stage 0 escalation (core infrastructure, 1251 production lines from a fork) caps the bot below approval no matter how clean the stages look. Stepping back: the shape still matches what I would have built, and the places it goes beyond my sketch are the places the failure modes actually live — the sweep race, the namespace/boot boundaries, the write-path asymmetry. The tests demonstrably bite: two independent mutation passes (the earlier What changed since the last pass, and why I am still not the one approving:
⏸️ Deferring to @wenshao — not because anything is unresolved, but because the only remaining act on this PR is a human one: whatever approvals branch protection still requires beyond yours. If a second maintainer approval is what's missing, the code and the evidence trail above are ready for it. 中文说明置信度:3/5 —— 审查干净、未发现阻塞问题,且门禁上一轮所要求的维护者签字已经到位;3 分纯属政策封顶,而非怀疑——Stage 0 升级(核心基础设施、来自 fork 的 1251 行生产代码)把机器人的评分压在批准线以下,与各阶段看起来多干净无关。 退一步看:形状仍与我会构建的方案一致,而它超出我草图的地方正是失效模式真实存在的地方——清扫竞态、namespace/boot 边界、写路径不对称。测试确有咬合力:两次独立变异检验(此前 上一轮之后发生了什么、为什么批准仍不由我做出:(1) 我此前转交的两个问题已由维护者回答——@wenshao 在本 commit 上以真实构建复验并给出可合入结论,其批准就落在 ⏸️ 转交 @wenshao —— 并非有任何未决问题,而是因为本 PR 剩下的唯一动作是人的动作:分支保护在您的批准之外还要求的任何批准。若缺的是第二位维护者的批准,代码与上方的证据链已就绪。 — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; unit suites only ran on Linux.
Not explored to full depth (tool budget reached): chunk 6: none — all checks I wanted completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget; I did not run the suite (no node_modules in the worktree; a full monorepo install was not justified for a tes…; PR #8969 adds a live per-process session registry at ~/.q...: none — all planned checks completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| pid: number, | ||
| procStart: string | null | undefined, | ||
| ): boolean { | ||
| if (!isPidAlive(pid)) return false; |
There was a problem hiding this comment.
[Critical] Liveness and token checks resolve the recorded PID in the caller's PID namespace/machine, but registry records carry no namespace or machine identity — Failure scenario: two sessions in separate PID namespaces sharing one ~/.qwen (host + devcontainer with a mounted home, sibling CI containers, or NFS/AFS homes across machines). Host-side ps sees the container session's PID as ESRCH or with a mismatched starttime token (boot_id is host-global, so it discriminates boots, not namespaces) → the sweep unlinks the live session's record; the container side sweeps every host session in reverse. Reproduced in both directions against real namespaces: BEFORE [record] → LIVE [] → AFTER [] with the victim process still alive — deterministic on every cross-boundary enumeration, no race required. Nothing recreates a swept record (registerSession is startup-only; patchSessionRecord refuses to resurrect), so affected sessions stay invisible to qwen sessions ps until restarted — violating the module's own invariant "anything we cannot positively prove dead is left alone".
Suggested fix: record the writer's namespace identity (e.g. st_ino of /proc/self/ns/pid, null where unavailable) and have the sweep skip — never unlink — records whose identity differs from the caller's; alternatively document the single-namespace assumption and gate the unlink on same-namespace proof.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // just read out of `qwen sessions ps`, and re-deriving it here | ||
| // would rename a live session on every /clear for no gain — the | ||
| // directory it names has not changed. | ||
| await patchSessionRecord({ sessionId: newSessionId, cwd: workDir }); |
There was a problem hiding this comment.
[Suggestion] This patchSessionRecord seam (and the directory-switch one below) has no test anywhere; deleting either line keeps the entire suite green — both deletion mutants measured green (556 tests each) — Failure scenario: a future refactor of startNewSession's queued runtime-status write drops or reorders this call → after /clear, qwen sessions ps --json silently keeps advertising the pre-clear transcript's sessionId for a live session — the exact stale-pointer bug this line exists to prevent — with no test signal. runtimeStatus.config.test.ts already exercises this identical gated path for the sidecar (markRuntimeStatusEnabled() → startNewSession() → await queued write), so an assertion that the registry record's sessionId followed the swap slots into the existing harness.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // sessions apart, so a mid-session directory switch has to reach it | ||
| // too — otherwise `qwen sessions ps` keeps advertising the folder | ||
| // this session left. | ||
| await patchSessionRecord({ cwd: workDir }); |
There was a problem hiding this comment.
[Suggestion] Untested seam (companion to the comment on the session-swap patch): deleting this line keeps the whole suite green — Failure scenario: config.test.ts's relocateWorkingDirectory test spies writeRuntimeStatus but never inspects the registry record beside it; if this call is dropped, qwen sessions ps keeps showing the directory a session already left, indefinitely, and nothing turns red. Extending that test with a patchSessionRecord spy assertion ({ cwd: newDir }) closes the gap at zero new scaffolding.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // discovery is a convenience and must not be able to abort a session. | ||
| try { | ||
| if ( | ||
| await registerSession({ |
There was a problem hiding this comment.
[Suggestion] The flagship integration point — register at startup with the session id/target dir, cleanup armed only on success, errors swallowed — has no test; harness-validated hunk probe: reverting either the import hunk or this 29-line registration block keeps every affected test green — Failure scenario: a future refactor of the startInteractiveUI startup sequence drops or breaks the registerSession call (or the registerCleanup(() => unregisterSession()) arming) and CI stays fully green → interactive sessions silently stop appearing in qwen sessions ps, or never disappear from it after exit — which is the entire reason this feature exists. A small cli-level test (mocking core's registerSession/unregisterSession as ps.test.ts does) asserting the call arguments (config.getSessionId()/config.getTargetDir()), cleanup-on-success-only, and swallowed rejection would pin it.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| import { Storage } from '../../config/storage.js'; | ||
| import { isNodeError } from '../../utils/errors.js'; | ||
| import { atomicWriteJSON } from '../../utils/atomicFileWrite.js'; | ||
| import { isPidAlive } from '../../utils/process-liveness.js'; |
There was a problem hiding this comment.
[Suggestion] Nothing pins teamHelpers to the shared isPidAlive; harness-validated hunk probe: reverting either this import or the deletion of the local copy keeps all tests green — Failure scenario: the shared helper adds a !Number.isInteger(pid) || pid <= 0 → false guard the old copy lacked, so a corrupt/hand-edited team file carrying leadPid: 0 or a fractional PID takes a different liveness path than before (the old code's process.kill(0, 0) probed the whole process group and reported alive), and a future change could silently re-introduce the duplicate local copy; neither regression turns any test red. A teamHelpers test covering the stale-lead-PID reclaim path (dead PID → reclaim allowed; live/EPERM PID → blocked) would exercise the shared helper through createTeamFile.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| .trim(); | ||
| cachedBootId = /^[0-9a-f-]+$/i.test(value) ? value : null; | ||
| } catch { | ||
| cachedBootId = null; |
There was a problem hiding this comment.
[Suggestion] readBootId() permanently caches a failed boot-id read, so one transient /proc failure disables PID-reuse protection for the entire process lifetime — Failure scenario (probe-measured: boot-id read attempts stay at 1 across two token reads; second token null): EMFILE/ENFILE under fd exhaustion at the first read pins cachedBootId to null forever — and both first-read moments are fd-pressure moments (registerSession at startup; the first concurrent sweep). Writer side: this session's record then ships procStart: null, so after exit + PID recycling the dead session is resurrected in ps indefinitely. Sweeper side: every isSameProcess comparison gets current === null and keeps the record, so this process never sweeps any stale record again. The caching rationale ("it cannot change while this process lives") covers the value, not the failure. Fix: cache only successes and retry failures — note the read must then be return cachedBootId ?? null;, since leaking undefined past the === null check produces "undefined:<ticks>" tokens and breaks the existing bare-tick test (verified).
— qwen3.8-max via Qwen Code /review (v0.21.10)
| listLiveSessions.mockResolvedValue([rec]); | ||
| await run({ json: true }); | ||
|
|
||
| expect(stdout).toEqual([JSON.stringify(rec)]); |
There was a problem hiding this comment.
[Suggestion] This JSON-Lines assertion is structurally blind to in-place mutation: the mock passes the rec object itself to the handler, and JSON.stringify(rec) is computed only after await run(...) returns — Failure scenario: measured mutant — injecting delete record.procStart; into the handler's json loop keeps this suite green at 15/15 while the documented 8-field contract (promised by the test comment and docs/users/features/commands.md) silently breaks for every qwen sessions ps --json consumer; the expectation observes the same reduced object the handler emitted. Projection-style mutants ARE caught; only the mutation class slips through. Snapshot the expectation before running:
const expected = JSON.stringify(rec);
await run({ json: true });
expect(stdout).toEqual([expected]);— probe-verified to fail the mutant and pass on the committed code.
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
|
||
| it('returns a well-formed record verbatim', async () => { | ||
| // The control for every rejection case below, and the only assertion | ||
| // that pins the exact field set a reader gets back. |
There was a problem hiding this comment.
[Suggestion] This control test's comment promises it "pins the exact field set a reader gets back", but readRecord's dropping of unknown fields is unpinned: the input has no extra fields, so toEqual([liveBody()]) cannot observe forwarding — Failure scenario: measured mutant — returning { ...value, schemaVersion, … } (a plausible simplification) keeps the suite green at 38/38; arbitrary keys from any hand-planted <pid>.json would then ride the typed SessionRegistryRecord into ps --json output and — concretely persistent — be re-written to disk by patchSessionRecord's { ...existing, ...patch } merge, permanently re-persisting litter the module doc says readers should neither show nor sweep. Add one case beside the control — probe-verified to fail the mutant:
await writeRaw(`${process.pid}.json`, { ...liveBody(), extraField: 'x' });
expect(await listLiveSessions()).toEqual([liveBody()]);— qwen3.8-max via Qwen Code /review (v0.21.10)
|
|
||
| if (argv.json) { | ||
| for (const record of records) { | ||
| writeStdoutLine(JSON.stringify(record)); |
There was a problem hiding this comment.
[Suggestion] The --json path emits cwd with no sanitization: JSON.stringify escapes C0 controls but not Unicode bidi format characters (U+202A–U+202E / U+2066–U+2069) — Failure scenario: probe-verified — U+202E passes through JSON.stringify raw, and piping such a record through the real jq -r .cwd emits the raw override bytes — while the human-readable path strips exactly this class (citing CVE-2021-42572), and the docs added by this same PR advertise qwen sessions ps --json | jq -r .cwd for terminal rendering: a Trojan-Source-style reordered path on a documented usage. (name is not a vector — deriveSessionName strips bidi at construction — and planting a record already requires the victim's user identity, so this is display-hardening of the PR's own stated threat model rather than a privilege boundary; raw --json also matches the pre-existing sessions list convention.) Either route JSON string fields through the same sanitization, or state the raw-data contract explicitly and annotate/drop the jq -r docs example — plus a --json bidi test mirroring the human-mode one.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| export function isPidAlive(pid: number): boolean { | ||
| if (!Number.isInteger(pid) || pid <= 0) return false; | ||
| try { | ||
| process.kill(pid, 0); |
There was a problem hiding this comment.
[Suggestion] Zombie processes pass process.kill(pid, 0), so an exited-but-unreaped session's record survives every sweep until the parent reaps it — Failure scenario: probe-reproduced with a real zombie — kill -0 succeeds, /proc/<pid>/stat is still present with state Z and the identical starttime token, so isSameProcess returns true and qwen sessions ps advertises the dead session as running for the lifetime of a spawn-but-never-wait parent (some IDE task runners/supervisors); the litter clears only when that parent exits. Interactive shells reap, so the trigger is niche, and the direction is false-liveness only (misleading listing, never a wrongful sweep). Linux can prove death from field 3 (state) of the same /proc/<pid>/stat line this module already parses for the token — treat Z as dead there (this also makes teamHelpers.tryReclaimStaleTeam more correct), keeping the conservative behavior on other platforms.
— qwen3.8-max via Qwen Code /review (v0.21.10)
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 3 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 建议见行内评论。 3 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
| import { | ||
| createDebugLogger, | ||
| isDebugLogFileEnabled, | ||
| registerSession, |
There was a problem hiding this comment.
[Suggestion] startInteractiveUI.tsx: the registerSession import (line 14) is not covered by any test. Deleting it would cause the session-registry registration to silently stop, but no test in this diff would detect the missing import.
Failure scenario: If the registerSession import is accidentally removed during a refactor, the interactive UI would not register sessions with the registry. qwen sessions ps would not show this session's record.
Suggested fix: Add a test that verifies the session-registry integration in startInteractiveUI is correctly wired.
中文说明
startInteractiveUI.tsx 第 14 行的 registerSession 导入未被任何测试覆盖。如果该导入在重构中被意外删除,交互式 UI 将不会向注册表注册会话,但现有测试不会发现此问题。建议增加测试来验证 startInteractiveUI 中会话注册表集成的正确性。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
| ) { | ||
| // Only arm cleanup for a record that exists; registration fails on | ||
| // a read-only home, and there is then nothing to unlink. | ||
| registerCleanup(() => unregisterSession()); |
There was a problem hiding this comment.
[Suggestion] The registerCleanup(() => unregisterSession()) call in startInteractiveUI.tsx (line 312) is not covered by any test. If this cleanup registration is removed, the session record would not be cleaned up on exit.
Failure scenario: A stale ~/.qwen/sessions/<pid>.json record would persist after exit until the next listLiveSessions sweep removes it.
Suggested fix: Add test coverage for the cleanup registration in startInteractiveUI.
中文说明
startInteractiveUI.tsx 第 312 行的 registerCleanup(() => unregisterSession()) 调用未被测试覆盖。如果该清理注册被删除,退出后会话记录将不会被清理。建议增加测试覆盖。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
| * record would be missing whatever else registration would have set. | ||
| */ | ||
| export async function patchSessionRecord( | ||
| patch: Partial<Omit<SessionRegistryRecord, 'pid' | 'schemaVersion'>>, |
There was a problem hiding this comment.
[Suggestion] The patchSessionRecord type allows procStart to be patched, creating a footgun. The Omit excludes pid and schemaVersion but allows procStart.
Failure scenario: If a caller passes procStart in the patch (e.g., patchSessionRecord({ procStart: "wrong-token" })), the spread overwrites the correct procStart token. The next listLiveSessions call would find a mismatch and sweep the session's own registry record. The session would silently disappear from qwen sessions ps.
Suggested fix: Add 'procStart' to the Omit: patch: Partial<Omit<SessionRegistryRecord, 'pid' | 'schemaVersion' | 'procStart'>>
中文说明
patchSessionRecord 的类型允许修补 procStart 字段,这会成为一个隐患。如果调用者传入 procStart,会导致 listLiveSessions 误判并删除当前会话的记录。建议将 procStart 加入 Omit 排除列表。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
| process.kill(pid, 0); | ||
| return true; | ||
| } catch (err) { | ||
| return isNodeError(err) && err.code === 'EPERM'; |
There was a problem hiding this comment.
[Suggestion] isPidAlive does not handle EACCES — the Windows equivalent of EPERM for a process owned by another user.
Failure scenario: On Windows, process.kill(pid, 0) for a process owned by another user throws EACCES, not EPERM. The function returns false (dead) for a process that is alive, potentially causing a live session's registry record to be swept by listLiveSessions.
Suggested fix: Add EACCES to the check:
| return isNodeError(err) && err.code === 'EPERM'; | |
| return isNodeError(err) && (err.code === 'EPERM' || err.code === 'EACCES'); |
中文说明
isPidAlive 没有处理 EACCES 错误——在 Windows 上,另一个用户拥有的进程会抛出 EACCES 而非 EPERM。这会导致函数将存活进程判定为已死亡,可能导致 listLiveSessions 误删该会话的注册表记录。建议在检查中添加 EACCES。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下: Autofix review round — PR #8969All 23 inline findings (1 Critical, 22 Suggestion) are addressed in one Critical
Code hardening (Suggestions)
Test coverage pinned (mutant-verified gaps)
Not addressable in this mode (no code action available)
Two optional sub-suggestions were deliberately not implemented (main fixes both landed): the "rewrite the full record on tokenless platforms" variant of the patch-guard finding (bounded, self-healing on Linux; the token guard is the reviewer-verified fix) and JSON-output sanitization (declined in favor of the documented raw contract, see above). Verification
中文说明Autofix 审查轮次 — PR #8969全部 23 条行内发现(1 条 Critical、22 条 Suggestion)已在单个提交中处理完毕: Critical
代码加固(Suggestion)
钉住的测试覆盖(变异验证过的缺口)
本模式无法处理(无可用代码手段)
两条可选子建议被有意不实现(两条主修复均已落地):补丁守卫发现中"无令牌平台上整体重写记录"的变体(影响有限、Linux 上可自愈;令牌守卫才是评审验证过的修复);以及对 JSON 输出做净化(已拒绝,改为文档化的原始数据契约,见上)。 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Superseded by a later commit; the current head requires a fresh review.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; unit suites only ran on Linux.
Not explored to full depth (tool budget reached): PR #8969 adds a live-session registry plus a qwen sessio...: none — all checks above were completed within budget.; chunk 8: could not execute runtimeStatus.config.test.ts — the review worktree has no node_modules (vitest startup fails with ERR_MODULE_NOT_FOUND) and a monorepo np…; PR #8969 adds a live-session registry plus a qwen sessio...: none — all checks I started were completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): sessions/list.ts — no such file or directory; 617 passing — this review observed 19116, 19757, 1247, 1510, 483, 3269, 523 passed.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; unit suites only ran on Linux。
未探索到全部深度(达到工具调用预算):PR #8969 adds a live-session registry plus a qwen sessio...:none — all checks above were completed within budget.;chunk 8:could not execute runtimeStatus.config.test.ts — the review worktree has no node_modules (vitest startup fails with ERR_MODULE_NOT_FOUND) and a monorepo np…;PR #8969 adds a live-session registry plus a qwen sessio...:none — all checks I started were completed within budget.。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan(非阻断):sessions/list.ts — no such file or directory; 617 passing — this review observed 19116, 19757, 1247, 1510, 483, 3269, 523 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // is alive over there, and a "matching" starttime can belong to | ||
| // an unrelated process. Neither listing nor sweeping is safe — | ||
| // leave it to a reader on the writer's own side. | ||
| if (record.pidNs !== ownNamespace) return; |
There was a problem hiding this comment.
[Critical] R1-1: The round-1 blocker (cross-namespace sweep deletes live sessions' records) is only partially fixed. The new pidNs identity distinguishes namespaces with distinct inodes (containers) but not different machines: the initial PID namespace inode is a kernel compile-time constant (4026531836) identical on every Linux machine, and pidNs is null on all non-Linux platforms — Failure scenario: two machines sharing one ~/.qwen over NFS/AFS (a configuration this blocker's own failure scenario names) both pass this guard; machine A's enumeration then resolves B's record locally — B's PID is dead on A, or a same-numbered PID carries a different boot-id prefix — isSameProcess returns false and the sweep unlinks B's live session record, which stays invisible to qwen sessions ps until restart (registration is one-shot; patchSessionRecord refuses to resurrect). Probe-verified on this commit: both branches delete, and skipping records whose procStart boot-id prefix differs from the local boot id flips the probe. The pinning tests use synthetic pidNs: 1 and replay only the container shape, so green tests do not retire this blocker. Suggested fix: treat records whose boot-id prefix is non-null and differs from the local boot id as foreign — skip, never unlink; and/or record a machine identity (e.g. /etc/machine-id); or state the single-machine assumption in the docstring instead of claiming NFS coverage.
中文说明
第 1 轮的阻断问题(跨命名空间清扫删除存活会话记录)只修复了一部分。新的 pidNs 身份能区分 inode 不同的命名空间(容器),但无法区分不同的机器:初始 PID 命名空间的 inode 是内核编译期常量(4026531836),在每台 Linux 机器上都相同,且所有非 Linux 平台上 pidNs 均为 null。失败场景:两台机器经 NFS/AFS 共享同一个 ~/.qwen(这正是该阻断问题自身失败场景点名的配置),两侧都能通过这个守卫;机器 A 枚举时会在本地解析 B 的记录——B 的 PID 在 A 上不存在,或同号 PID 的 boot-id 前缀不同——isSameProcess 返回 false,清扫于是 unlink 掉 B 的存活会话记录,该会话在重启前对 qwen sessions ps 一直不可见(注册是一次性的,patch 拒绝复活记录)。已在当前提交上探针验证:两条分支都会删除记录;而跳过 boot-id 前缀与本地不同的记录即可翻转探针结果。现有钉住测试使用合成的 pidNs: 1,只复现了容器形态,因此测试全绿并不能关闭该阻断。建议修复:将 procStart boot-id 前缀非空且与本地 boot-id 不同的记录视为外部记录——跳过、绝不 unlink;和/或记录机器身份(如 /etc/machine-id);或在文档字符串中声明单机假设,而不是声称覆盖 NFS。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
|
||
| /** This process's record path. Records are keyed by PID. */ | ||
| export function getSessionRecordPath(): string { | ||
| return path.join(getSessionRegistryDir(), `${process.pid}.json`); |
There was a problem hiding this comment.
[Critical] Record paths are keyed by bare PID, which is unique only within one PID namespace — Failure scenario: host session A and devcontainer session B share a mounted home and both hold numeric PID 42 (likely with sibling containers, where qwen starts at similarly low PIDs). Both call registerSession → both write 42.json; last writer wins. On the loser's side the surviving record fails record.pidNs !== ownNamespace and is skipped — a running session silently never appears in qwen sessions ps. Worse, whichever session exits first runs unregisterSession(), unlinking the shared path and destroying the survivor's record; registration is one-shot and patchSessionRecord no-ops on missing records, so the survivor stays invisible for its remaining lifetime. patchSessionRecord's existing.pid !== process.pid guard is numeric and passes across namespaces by construction of the collision. Suggested fix: key the path on namespace too (e.g. <pid>.<pidNs>.json or per-namespace subdirectories, updating the filename/content agreement check), or at minimum refuse to overwrite a record whose pidNs differs and make unregisterSession unlink only when pidNs matches the caller's.
中文说明
记录路径仅以裸 PID 为键,而 PID 只在单个 PID 命名空间内唯一。失败场景:宿主会话 A 与 devcontainer 会话 B 共享挂载的 home 且恰好同为 PID 42(兄弟容器中很常见,qwen 都以相近的低 PID 启动)。两者都调用 registerSession → 都写 42.json,后写者覆盖。输家一侧,幸存记录因 record.pidNs !== ownNamespace 被跳过——一个正在运行的会话从此在 qwen sessions ps 中静默消失。更糟的是,先退出的会话执行 unregisterSession() 时会 unlink 这个共享路径,摧毁幸存者的记录;注册是一次性的,patchSessionRecord 对缺失记录不做处理,幸存者在其剩余生命周期内一直不可见。patchSessionRecord 的 existing.pid !== process.pid 守卫是数值比较,在这种碰撞下按构造就会通过。建议修复:路径键加入命名空间(如 <pid>.<pidNs>.json 或按命名空间分子目录,并同步更新文件名/内容一致性检查);至少应拒绝覆盖 pidNs 不同的记录,并让 unregisterSession 仅在 pidNs 与调用方一致时才 unlink。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(after!.pid).toBe(process.pid); | ||
| expect(after!.cwd).toBe(tmpDir); |
There was a problem hiding this comment.
[Suggestion] This new /clear round-trip test pins sessionId/pid/cwd but never pins the documented "name is deliberately not patched on /clear" rule (comment at config.ts:3912) — Failure scenario: deriveSessionName(cwd, sessionId) hashes the session id into the suffix, so adding name to the /clear patch (config.ts:3916) renames the session on every /clear — the exact user-visible rename that comment says is deliberately avoided (the name is the handle a user just read out of ps) — and passes every current assertion because after.name is never read.
| expect(after!.pid).toBe(process.pid); | |
| expect(after!.cwd).toBe(tmpDir); | |
| expect(after!.pid).toBe(process.pid); | |
| expect(after!.cwd).toBe(tmpDir); | |
| expect(after!.name).toBe(before!.name); |
(capture const [before] = await listLiveSessions(); ahead of the startNewSession call.)
中文说明
这个新的 /clear 往返测试钉住了 sessionId/pid/cwd,但从未钉住文档化的"/clear 刻意不更新 name"规则(config.ts:3912 的注释)。失败场景:deriveSessionName(cwd, sessionId) 会把 session id 哈希进后缀,因此向 /clear 补丁(config.ts:3916)加入 name 会让每次 /clear 都重命名会话——正是该注释明确要避免的用户可见重命名(name 是用户刚从 ps 中读到的句柄)——并且能通过当前所有断言,因为 after.name 从未被读取。修复:在 startNewSession 调用前先取 const [before] = await listLiveSessions();,再补充对 after!.name 的断言(如 suggestion 块所示)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| it('neutralizes control sequences coming from another process record', async () => { | ||
| listLiveSessions.mockResolvedValue([ |
There was a problem hiding this comment.
[Suggestion] This test embeds raw invisible 0x1B (ESC) bytes in the source — one in the fixture string (line 165) and one in the assertion (line 170) — instead of the escaped form the sibling list.test.ts uses for the identical behavior — Failure scenario: the bytes are invisible in every review/diff rendering (this review's own tooling truncates the moment it serializes them); if an editor, paste, or tooling pass drops the invisible byte, the fixture loses its CSI sequence and the test still passes while no longer exercising ESC neutralization at all — silent coverage loss of exactly the security behavior this PR documents. Prettier does not normalize string-literal content, so nothing in CI catches it. Suggested fix: replace the two raw bytes with escape sequences, as the adjacent bidi test and list.test.ts already do (verified behavior-preserving: 16/16 tests pass before and after).
中文说明
该测试在源码中嵌入了原始的不可见 0x1B(ESC)字节——fixture 字符串(第 165 行)一个、断言(第 170 行)一个——而没有像同目录 list.test.ts 在相同行为上使用转义形式。失败场景:这些字节在任何评审/diff 渲染中都不可见(本次评审的工具链在序列化它们的瞬间就会截断);若编辑器、粘贴或某个工具链环节丢弃了该不可见字节,fixture 将不再含 CSI 序列,而测试仍然通过,却完全不再检验 ESC 中和——恰恰对本 PR 文档化的安全行为造成静默覆盖丢失。Prettier 不会规范化字符串字面量内容,CI 中没有任何环节能捕获。建议修复:把两个原始字节替换为转义序列(与相邻 bidi 测试及 list.test.ts 一致);已验证行为保持不变(前后均 16/16 通过)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(registerCleanup).toHaveBeenCalledTimes(2); | ||
| expect(unregisterSession).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] The test pins that the unregister cleanup is armed (call count) and does not fire during startup, but no test ever invokes the armed callback, so the wiring registerCleanup(() => unregisterSession()) is ungated — probe-verified: replacing the body with registerCleanup(() => {}) ships all 3 tests green — Failure scenario: a future edit emptying or detaching the callback body passes this suite, and every interactive session's ~/.qwen/sessions/<pid>.json survives exit; qwen sessions ps lists dead sessions as running until the next enumeration sweeps them, breaking the registry's "presence means running right now" promise.
| expect(registerCleanup).toHaveBeenCalledTimes(2); | |
| expect(unregisterSession).not.toHaveBeenCalled(); | |
| expect(registerCleanup).toHaveBeenCalledTimes(2); | |
| expect(unregisterSession).not.toHaveBeenCalled(); | |
| const armUnregister = registerCleanup.mock.calls[0]![0]; | |
| await armUnregister(); | |
| expect(unregisterSession).toHaveBeenCalledTimes(1); |
中文说明
测试钉住了 unregister 清理已布防(调用次数)且启动期间不会触发,但没有任何测试真正执行这个已布防的回调,因此 registerCleanup(() => unregisterSession()) 接线没有把关——探针验证:把回调体替换为 registerCleanup(() => {}),3 个测试全部通过。失败场景:未来某次编辑清空或断开回调体,测试套件照样通过,每个交互式会话的 ~/.qwen/sessions/<pid>.json 在退出后残留;在下次枚举清扫之前,qwen sessions ps 会把已死会话列为运行中,违背注册表"存在即正在运行"的承诺。修复如 suggestion 块:取出并执行已布防的回调,断言 unregisterSession 被调用一次。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // lifecycles coincide, and the only divergence (sidecar write | ||
| // failed, registration succeeded) costs a stale `sessionId` in |
There was a problem hiding this comment.
[Suggestion] Both registry patches ride the sidecar's runtimeStatusEnabled gate, whose failure domain (project-local chats/ dir) is independent of the registry's (~/.qwen/sessions) — Failure scenario: qwen starts where the sidecar write fails but home is writable (read-only or full project chats/ dir, transient ENOSPC/EMFILE): markRuntimeStatusEnabled() never runs while registerSession succeeds (ungated). For the whole session both patch gates stay false, so every /clear and /cd skips the patch: ps keeps showing the pre-/cd directory/name in the human table and ps --json keeps the pre-/clear sessionId until exit — the discovery index points at the wrong folder and transcript. This comment prices the divergence as "a stale sessionId in ps --json", omitting the human-table staleness; and its safety rationale ("must not delete a sibling's file") does not apply to PID-keyed records, so riding the gate buys coincidence, not safety. Suggested fix: gate the two patchSessionRecord calls on registration success instead (e.g. Config.markSessionRegistered() set when registerSession returned true); patchSessionRecord's missing-record no-op already makes the inverse divergence safe. If the gate is kept, correct the comment to state the full cost.
中文说明
两个注册表补丁都搭载在 sidecar 的 runtimeStatusEnabled 门控上,而该门控的失败域(项目本地 chats/ 目录)与注册表的失败域(~/.qwen/sessions)相互独立。失败场景:启动时 sidecar 写入失败但 home 可写(项目 chats/ 目录只读或写满、瞬时 ENOSPC/EMFILE):markRuntimeStatusEnabled() 永不执行,而 registerSession 成功(无门控)。整个会话期间两个补丁门控都为 false,每次 /clear 和 /cd 都跳过补丁:ps 的人类表格一直显示 /cd 之前的目录/名称,ps --json 一直保留 /clear 之前的 sessionId,直到退出——发现索引指向错误的目录和会话记录。此注释把该分歧代价描述为"ps --json 中过期的 sessionId",遗漏了人类表格的过期;其安全理由("不能删除兄弟的文件")对以 PID 为键的记录并不适用,因此搭载该门控换来的只是巧合而非安全。建议修复:改为以注册成功作为两个 patchSessionRecord 调用的门控(例如 registerSession 返回 true 时设置 Config.markSessionRegistered());patchSessionRecord 对缺失记录的空操作已使反向分歧安全。若保留现有门控,请修正注释以说明完整代价。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const [record] = await listLiveSessions(); | ||
| expect(record).toMatchObject({ | ||
| sessionId: 'new', | ||
| name: 'renamed', |
There was a problem hiding this comment.
[Suggestion] No test pins that patchSessionRecord preserves startedAt when the patch omits it — this merge test asserts sessionId/name/cwd/qwenVersion but not the sort key and AGE source, and both production patch sites omit startedAt (config.ts:3916 {sessionId, cwd} on /clear; config.ts:3972 {cwd, name} on /cd). Probe-verified: rebuilding the merge as an explicit literal re-stamping startedAt: Date.now() leaves 54/54 green; a startedAt-equality pin catches it. The sidecar half of this same test file already pins the identical property (expect(after?.startedAt).toBe(before?.startedAt)); the registry side lacks the pin. (Distinct from the procStart/pidNs finding on line 328.) — Failure scenario: after every /clear or /cd, ps's AGE column resets to 0s for an hours-old session, the session jumps to the top of the newest-first listing, and ps --json reports a false start time. Suggested fix: capture startedAt before the patch and add it to this toMatchObject.
| name: 'renamed', | |
| name: 'renamed', | |
| startedAt: expect.any(Number), |
(assert equality with the pre-patch value rather than just presence for a real pin.)
中文说明
没有测试钉住 patchSessionRecord 在补丁未包含 startedAt 时会保留它——此合并测试断言了 sessionId/name/cwd/qwenVersion,但没有断言排序键和 AGE 的来源,而两个生产补丁点都不含 startedAt(config.ts:3916 /clear 的 {sessionId, cwd};config.ts:3972 /cd 的 {cwd, name})。探针验证:把合并重写为显式字面量并重新打印 startedAt: Date.now(),54/54 全绿;补上 startedAt 相等断言即可捕获。同一测试文件的 sidecar 部分已钉住相同属性(expect(after?.startedAt).toBe(before?.startedAt)),注册表一侧缺少该钉住。(与第 328 行的 procStart/pidNs 发现是不同的问题。)失败场景:每次 /clear 或 /cd 后,ps 的 AGE 列对已运行数小时的会话重置为 0s,该会话跳到最新列表顶部,ps --json 报告错误的启动时间。建议修复:补丁前记录 startedAt 并加入此 toMatchObject(要真正钉住,请与补丁前的值断言相等,而不仅是存在性)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| itPosixPatch('keeps the record at 0600 across a patch', async () => { | ||
| await registerSession({ sessionId: 's1', cwd: '/w/app' }); | ||
| await patchSessionRecord({ sessionId: 'new' }); |
There was a problem hiding this comment.
[Suggestion] The patch write's atomicity and noFollow properties — the source passes the same options as registration ({ mode, forceMode: true, noFollow: true }), and registration's suite pins them with a symlink-threat-model test — are ungated for patchSessionRecord; only the 0600 mode is pinned. Probe-verified: replacing the patch's atomicWriteJSON with a plain fs.writeFile ships the whole file green (the 0600 test passes because in-place writes preserve the existing mode) — Failure scenario: once non-atomic, a SIGKILL/OOM/power loss inside the patch write window (widened on the slow/NFS homes the docstring names) leaves a torn record: readRecord rejects it, patchSessionRecord then sees existing === null and no-ops forever, malformed records are never swept — the session is invisible in ps for its entire lifetime and the file persists as permanent litter. Dropping noFollow revives the symlink-redirected write the registration-side test exists to prevent, on a path written on every /clear and /cd. Suggested fix: mirror the registration symlink test on the patch path — register, replace the record with a symlink whose target holds a valid record for this pid, patch, assert the outside file untouched and the path no longer a symlink.
中文说明
补丁写入的原子性与 noFollow 属性——源码传入与注册相同的选项({ mode, forceMode: true, noFollow: true }),且注册测试套件用符号链接威胁模型测试钉住了它们——对 patchSessionRecord 没有把关;只有 0600 权限被钉住。探针验证:把补丁的 atomicWriteJSON 换成普通 fs.writeFile,整个文件测试全绿(0600 测试之所以通过,是因为原地写入保留现有权限)。失败场景:一旦非原子,在补丁写入窗口内发生 SIGKILL/OOM/断电(文档点名的慢速/NFS home 会扩大该窗口)将留下撕裂的记录:readRecord 拒绝解析,patchSessionRecord 随后看到 existing === null 并永远空操作,畸形记录永不被清扫——会话在其整个生命周期内对 ps 不可见,文件永久残留。丢弃 noFollow 会在每次 /clear 和 /cd 都写入的路径上复活注册侧测试所要防止的符号链接重定向写入。建议修复:在补丁路径上复刻注册的符号链接测试——注册后,把记录替换为指向含本 pid 有效记录的目标文件的符号链接,执行补丁,断言外部文件未被改动且该路径不再是符号链接。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // The registry's DIRECTORY column is how a user tells two live | ||
| // sessions apart; the switch must reach it (and the directory-derived | ||
| // name) or `qwen sessions ps` keeps showing the folder that was left. | ||
| expect(patchSessionRecordSpy).toHaveBeenCalledWith({ |
There was a problem hiding this comment.
[Suggestion] This new /cd test pins that patchSessionRecord was called, not that the registry update completed before relocateWorkingDirectory resolves — toHaveBeenCalledWith records the call synchronously when the promise is created, so it cannot distinguish await patchSessionRecord(...) from fire-and-forget. Probe-verified on this commit: de-awaiting the patch (void patchSessionRecord({...}) at config.ts:3972) ships the test green — Failure scenario: the diff's user-visible guarantee — after /cd returns, qwen sessions ps already shows the new DIRECTORY/name — silently dies under any future de-await. Note the interaction: the separate /cd-blocks-on-home-dir-IO finding recommends exactly that refactor ("own serialized chain that /cd never awaits"); once landed without a replacement pin, a slow registry write on NFS/hung homes plus an exit shortly after /cd leaves the record advertising the folder the session left — with every test still green. Suggested fix: have the spy return a promise resolved by a deferred and assert it settled before relocateWorkingDirectory's returned promise (or pin the replacement guarantee if the off-path refactor lands).
中文说明
这个新的 /cd 测试钉住了 patchSessionRecord 被调用,但没有钉住注册表更新在 relocateWorkingDirectory 返回前完成——toHaveBeenCalledWith 在 promise 创建时同步记录调用,因此无法区分 await patchSessionRecord(...) 与发起后不管(fire-and-forget)。已在当前提交上探针验证:去掉补丁的 await(config.ts:3972 处 void patchSessionRecord({...})),测试仍为绿色。失败场景:diff 的用户可见保证——/cd 返回后 qwen sessions ps 已显示新的 DIRECTORY/name——在未来任何去 await 改动下都会静默失效。注意相互作用:另一条"/cd 阻塞在 home 目录 I/O"的发现恰好推荐这种重构("/cd 永不 await 的独立串行链");若该重构落地时没有替代钉住,NFS/挂起 home 上的慢速注册表写入加上 /cd 后快速退出,会让记录一直宣传会话已离开的目录——而所有测试仍为绿色。建议修复:让 spy 返回一个由 deferred 解决的 promise,并断言它在 relocateWorkingDirectory 返回的 promise 之前已解决(若去阻塞重构落地,则改为钉住新的保证)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| ).toBe(true); | ||
|
|
||
| const live = await listLiveSessions(); | ||
| expect(live).toHaveLength(1); |
There was a problem hiding this comment.
[Suggestion] No test pins the value registerSession writes for startedAt (Date.now()) — the round-trip's toMatchObject checks schemaVersion/pid/sessionId/cwd/qwenVersion and the name regex but never startedAt, and no other test observes the value as written. Probe-verified: a wrong-epoch refactor (seconds instead of milliseconds, or a constant) ships green; bounds assertions catch it. (A wholesale omission IS caught — readRecord rejects — so only a wrong-but-numeric value is ungated. Distinct from the patch-merge startedAt finding at line 333.) — Failure scenario: ps's AGE column shows ~20,000d for every live session (or a constant), the newest-first ordering key is wrong for every session, and ps --json emits a false start time. Suggested fix: capture bounds around registration (const before = Date.now(); ...; const after = Date.now();) and assert live[0].startedAt within [before, after] — the sidecar suite already pins the analogous property (runtimeStatus.config.test.ts:147).
中文说明
没有测试钉住 registerSession 为 startedAt 写入的值(Date.now())——往返测试的 toMatchObject 检查了 schemaVersion/pid/sessionId/cwd/qwenVersion 和 name 正则,但从未检查 startedAt,也没有其他测试观察写入时的值。探针验证:错误的纪元重构(秒而非毫秒,或常量)能通过全部测试;区间断言可以捕获。(整体省略该字段会被捕获——readRecord 会拒绝——因此只有"数值存在但错误"的情况没有把关。与第 333 行补丁合并的 startedAt 发现是不同的问题。)失败场景:ps 的 AGE 列对每个存活会话显示约 20,000 天(或常量),最新优先排序键对每个会话都是错的,ps --json 输出错误的启动时间。建议修复:在注册前后取区间(const before = Date.now(); ...; const after = Date.now();),断言 live[0].startedAt 落在 [before, after] 内——sidecar 套件已钉住类似属性(runtimeStatus.config.test.ts:147)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下: PR #8969 review feedback — round 3 summaryCommit: All 18 inline findings are addressed; none declined, none escalated. The review-level Critical findings
Suggestions
VerificationEach command was actually run in this checkout after the changes:
中文说明PR #8969 评审反馈——第 3 轮处理总结提交: 18 条内联发现全部处理完毕;没有拒绝项,也没有升级给维护者的项。评审级 Critical 发现
Suggestion 发现
验证以下每条命令都是在本次变更之后、在此检出上实际执行的:
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
… with the registry swap patch (QwenLM#8969)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 3/100 轮)。改动内容与我反驳保留之处如下: Autofix review round for PR #8969Feedback triageNo new review feedback this round — the workflow-reported feedback contained Conflict resolution
Resolution: kept this PR's nested two-gate structure and adopted main's The resolution was audited by diffing the merge result against both parents: Verification
中文说明PR #8969 的 Autofix 审查轮次反馈分类本轮没有新的审查反馈——工作流上报的反馈中,在上次评估之后没有任何新的评审、行内评论、issue 级评论、失败的检查或持续失败的检查。唯一需要处理的事项是上报的与基础分支的冲突( 冲突解决已将
解决方式:保留本 PR 的两层嵌套门槛结构,并采用 main 的 解决结果已通过将合并结果分别与两个父提交做 diff 来审计:相对 验证
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审;第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
已被后续 commit 取代,当前 head 需重新 review
已被后续 commit 9f2dbc1 取代,当前 head 需重新 review。
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; unit suites only ran on Linux.
Not reviewed: build-and-test — packages/web-shell and packages/webui suites did not run (review build-test whole-command budget exhausted after the changed workspaces and their direct dependents).
Not reviewed: reverse audit — ran to the 5-round cap; rounds 4 and 5 added no valid findings (round 5's sole candidate duplicated the already-discussed open comment 3773980297).
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — finished within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I needed completed within budget; I did not attempt a local vitest run (worktree has no node_modules and CI already validated the file on the …; chunk 4: none (≈13 of ~45 calls used).; "You are review agent reverse-audit — Reverse audit agent…": no substantive incomplete checks. libuv question resolved. I used about 12 tool calls.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget., and 8 more.
Test Plan (not a blocker): sessions/list.ts — no such file or directory; 617 passing — this review observed 19563, 1364, 1539, 494 passed.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; unit suites only ran on Linux。
未审查:build-and-test — packages/web-shell and packages/webui suites did not run (review build-test whole-command budget exhausted after the changed workspaces and their direct dependents)。
未审查:reverse audit — ran to the 5-round cap; rounds 4 and 5 added no valid findings (round 5's sole candidate duplicated the already-discussed open comment 3773980297)。
未探索到全部深度(达到工具调用预算):"You are review agent reverse-audit — Reverse audit agent…":none — finished within budget.;"You are review agent reverse-audit — Reverse audit agent…":none — all checks I needed completed within budget; I did not attempt a local vitest run (worktree has no node_modules and CI already validated the file on the …;chunk 4:none (≈13 of ~45 calls used).;"You are review agent reverse-audit — Reverse audit agent…":no substantive incomplete checks. libuv question resolved. I used about 12 tool calls.;"You are review agent reverse-audit — Reverse audit agent…":none — all checks above completed within budget.,另有 8 条。
Test Plan(非阻断):sessions/list.ts — no such file or directory; 617 passing — this review observed 19563, 1364, 1539, 494 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| config.trackSessionRegistration( | ||
| registerSession({ | ||
| sessionId: config.getSessionId(), | ||
| cwd: config.getTargetDir(), |
There was a problem hiding this comment.
[Critical] R7-1: The new session-registration wiring calls config.getTargetDir(), which the partial Config mocks in the pre-existing packages/cli/src/gemini.test.tsx do not define — Failure scenario: npm test --workspace=packages/cli → every test that drives the interactive path through a mocked Config throws TypeError: config.getTargetDir is not a function at this line. Measured on this commit: merge base npx vitest run src/gemini.test.tsx → 76/76 passed; PR worktree → 32 failed / 44 passed. Six mock shapes in gemini.test.tsx (lines ~377, 1687, 1812, 1935, 2057, 2197, 2514) provide getSessionId etc. but not getTargetDir. This is also why the Test (ubuntu-latest, Node 22.x) CI check is red.
Suggested fix: add getTargetDir: () => '/' (matching the mocks' getProjectRoot: () => '/') to those loadCliConfig mock shapes in gemini.test.tsx, or route the registration through an accessor the existing mocks already provide.
中文说明
[Critical] R7-1:新的会话注册接线调用了 config.getTargetDir(),而 packages/cli/src/gemini.test.tsx 中既有的部分 Config mock 并没有定义该方法——失败场景:npm test --workspace=packages/cli → 所有经由 mock Config 驱动交互路径的测试都会在本行抛出 TypeError: config.getTargetDir is not a function。在本提交上实测:merge base 上 npx vitest run src/gemini.test.tsx → 76/76 通过;PR 工作树上 → 32 失败 / 44 通过。gemini.test.tsx 中六处 mock 形状(约第 377、1687、1812、1935、2057、2197、2514 行)提供了 getSessionId 等但没有 getTargetDir。这也是 Test (ubuntu-latest, Node 22.x) CI 检查变红的原因。建议修复:在 gemini.test.tsx 的这些 loadCliConfig mock 形状中加入 getTargetDir: () => '/'(与 mock 中的 getProjectRoot: () => '/' 对齐),或让注册改走既有 mock 已提供的访问器。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
已修复:补齐 6 组 interactive Config mock 与 startInteractiveUI mock 的 session registry 契约,并隔离真实注册副作用;清理顺序断言同步覆盖 registry cleanup。验证:packages/cli 下 gemini.test.tsx 76/76、startInteractiveUI.test.tsx 4/4,合计 80/80;focused ESLint、Prettier 与 git diff --check 通过。
| existingStat !== undefined && | ||
| (!options?.noFollow || existingStat.isFile()) | ||
| ? existingStat.mode & 0o7777 | ||
| : undefined; |
There was a problem hiding this comment.
[Suggestion] R7-6: The rewritten existingMode condition has no paired test for either noFollow branch — Concrete cost: two mutants both ship green across all 354 tests in the 8 affected suites. Mutant A (drop mode preservation whenever noFollow is set): probe-measured, an existing 0o755 file rewritten with {noFollow:true} becomes 0o644 (umask default) under the mutant vs preserved under the real code — a permission widening on user files for the noFollow callers that pass neither mode nor forceMode (extensionSettings.ts:219/275/531 — the extension .env, cronTasksFile.ts:288, indexer.ts:349 — MEMORY.md, cli save-artifact.ts:352). Mutant B (drop the isFile() guard): probe-measured, replacing a symlink with {noFollow:true, mode:0o600} yields 0o777 under the mutant (explicit mode silently overridden by the symlink's lstat mode) vs 0o600 under the real code — exposed at skill-curator (5 sites) and image-gen.ts:112.
Suggested fix (tests, in atomicFileWrite.test.ts beside the existing preservation pair):
it('preserves existing file permissions under noFollow', async () => {
const target = path.join(tmpDir, 'keep-mode');
fsSync.writeFileSync(target, 'old');
fsSync.chmodSync(target, 0o600);
await atomicWriteFile(target, 'new', { noFollow: true });
expect(fsSync.statSync(target).mode & 0o777).toBe(0o600);
});and extend the noFollow symlink-replacement tests with expect(fsSync.statSync(link).mode & 0o777).toBe(0o600) when an explicit mode is passed (skip on win32, like the sibling ownership test).
中文说明
[Suggestion] R7-6:重写后的 existingMode 条件对两个 noFollow 分支都没有配套测试——具体代价:两个变异体在全部 8 个相关套件的 354 个测试下都绿着通过。变异体 A(noFollow 时一律丢弃模式保留):探针实测,0o755 的既有文件以 {noFollow:true} 重写,在变异体下变成 0o644(umask 默认值),真实代码则保留原模式——对既不传 mode 也不传 forceMode 的 noFollow 调用方(extensionSettings.ts:219/275/531——扩展 .env、cronTasksFile.ts:288、indexer.ts:349——MEMORY.md、cli save-artifact.ts:352)构成用户文件权限放宽。变异体 B(去掉 isFile() 门):探针实测,以 {noFollow:true, mode:0o600} 替换符号链接时,变异体得到 0o777(显式 mode 被符号链接的 lstat 模式静默覆盖),真实代码得到 0o600——暴露于 skill-curator(5 处)与 image-gen.ts:112。建议修复:在 atomicFileWrite.test.ts 现有保留测试对旁补两个测试(见上方代码),并在 noFollow 符号链接替换测试中对显式 mode 断言结果权限(win32 跳过)。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const existing = await readRecord(filePath); | ||
| if (existing.status === 'read-error') { | ||
| debugLogger.debug( |
There was a problem hiding this comment.
[Suggestion] R7-7: A transient failure anywhere in registration's file phase permanently strands the session undiscoverable — Failure scenario: at startup, this readRecord hits EMFILE (the exact fd-pressure window the two retry blocks above it explicitly defend against), or NFS ESTALE/EIO, returning read-error → registerSession returns false without writing (likewise if atomicWriteJSON itself throws EMFILE). trackSessionRegistration settles sessionRegistered=false, the sole production call site never retries, and patchSessionRecord explicitly refuses to resurrect a missing record — so the session is invisible to every peer's listLiveSessions for its entire lifetime. Probe-verified: one injected EMFILE on the first record-path stat permanently refused registration; adding one readRecord retry flipped it to registered. Asymmetry: the token/namespace reads get one retry each against this same window; the equally exposed file phase gets none.
Suggested fix — one retry, same pattern as the identity reads above:
let existing = await readRecord(filePath);
if (existing.status === 'read-error') {
// Same transient fd-pressure window as the token/namespace retries above.
existing = await readRecord(filePath);
}(or a bounded lazy retry from a later transition when registration failed transiently rather than identity-refused).
中文说明
[Suggestion] R7-7:注册文件阶段的任何瞬时故障都会让会话永久不可发现——失败场景:启动时这里的 readRecord 命中 EMFILE(其上方两个重试块明确防御的正是这个 fd 压力窗口),或 NFS ESTALE/EIO,返回 read-error → registerSession 未写入即返回 false(atomicWriteJSON 自身抛 EMFILE 同理)。trackSessionRegistration 将 sessionRegistered 定为 false,唯一的生产调用点不会重试,而 patchSessionRecord 明确拒绝复活缺失的记录——该会话在整个生命周期内对所有对端的 listLiveSessions 不可见。探针验证:在首次记录路径 stat 上注入一次 EMFILE 即永久拒绝注册;为 readRecord 增加一次重试后翻转为注册成功。不对称之处:token/namespace 读取在同一窗口各有一次重试,同样暴露的文件阶段却没有任何重试。建议修复:仿照上方身份读取的模式重试一次(见上方代码),或在注册是瞬时失败(而非身份拒绝)时由后续转换做有界延迟重试。
— qwen3.8-max via Qwen Code /review (v0.21.11)
已被后续 commit 取代,当前 head 需重新 review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; unit suites only ran on Linux.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I opened were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none (6 tool calls of ~37).; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I opened were completed (10 tool calls used)., and 4 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): sessions/list.ts — no such file or directory; 617 passing — this review observed 19592, 20003, 1364, 1539, 494, 3502, 559 passed.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI; unit suites only ran on Linux。
未探索到全部深度(达到工具调用预算):"You are review agent reverse-audit — Reverse audit agent…":none — all checks I started completed within budget.;"You are review agent reverse-audit — Reverse audit agent…":none — all checks above were completed within budget.;"You are review agent reverse-audit — Reverse audit agent…":none — all checks I opened were completed within budget.;"You are review agent reverse-audit — Reverse audit agent…":none (6 tool calls of ~37).;"You are review agent reverse-audit — Reverse audit agent…":none — all checks I opened were completed (10 tool calls used).,另有 4 条。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan(非阻断):sessions/list.ts — no such file or directory; 617 passing — this review observed 19592, 20003, 1364, 1539, 494, 3502, 559 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| ); | ||
| const mockUpdateBeforeRelaunch = vi.hoisted(() => vi.fn()); | ||
| const mockGetInstallationInfo = vi.hoisted(() => vi.fn()); | ||
| const mockRegisterSession = vi.hoisted( |
There was a problem hiding this comment.
[Suggestion] R8-2: mockRegisterSession is defined and wired into the core module mock but has zero expectations anywhere in this file — the negative side of the documented "Headless sessions (qwen -p) do not register" contract is unpinned; it holds today only by code placement (the sole production registerSession call lives in startInteractiveUI.tsx). — Failure scenario: if a refactor lifts config.trackSessionRegistration(registerSession(...)) into the shared startup path of gemini.tsx (before the interactive/headless dispatch), qwen -p runs start appearing in qwen sessions ps — and qwen sessions ps would register during its own dispatch and list itself — while every existing non-interactive main() test stays green.
// in one existing non-interactive main() test, after the run completes:
expect(mockRegisterSession).not.toHaveBeenCalled();中文说明
[Suggestion] R8-2:mockRegisterSession 被定义并接入了 core 模块 mock,但整个文件中对它没有任何断言——"Headless 会话(qwen -p)不会注册"这一已写入文档的契约,其"负面"一侧没有被测试钉住;它目前只是靠代码位置成立(生产中唯一的 registerSession 调用在 startInteractiveUI.tsx)。失败场景:如果某次重构把 config.trackSessionRegistration(registerSession(...)) 提升到 gemini.tsx 的共享启动路径(在交互/非交互分发之前),qwen -p 运行将开始出现在 qwen sessions ps 中——而且 qwen sessions ps 会在自己的分发过程中注册并列出自己——与此同时所有既有的非交互 main() 测试仍然全绿。建议修复如上:在某个既有非交互 main() 测试结束后断言 expect(mockRegisterSession).not.toHaveBeenCalled();。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| existingStat = options?.noFollow | ||
| ? await fs.lstat(targetPath) | ||
| : await fs.stat(targetPath); |
There was a problem hiding this comment.
[Suggestion] R8-3: The noFollow symlink fix (lstat + isFile() guard) lands only in the async variant; atomicWriteFileSync — documented "Same semantics" — still derives existingMode via symlink-following statSync, inheriting the target's mode bits (the 0o7777 mask carries setuid/setgid/sticky). Probe-measured this round: with a planted symlink whose target is mode 0o2666, the async noFollow write lands at 0o644 while the sync variant lands at 0o2666 (setgid + world-writable inherited). One live caller reaches the divergent branch: packages/cli/src/commands/review/save-artifact.ts:349 passes { encoding: 'utf8', noFollow: true } with no mode/forceMode (every other sync noFollow caller passes forceMode: true + mode, which skips the derivation). — Failure scenario: on a shared host, a local attacker who can write the workspace directory plants output.json -> <file-with-mode-0o2666> after rejectSymlinkPath validation but before the write; the review artifact is committed with attacker-selected permission bits. The async variant post-fix creates the same file at umask-default mode in the identical scenario.
Suggested fix: mirror the async change in atomicWriteFileSync — use fsSync.lstatSync(targetPath) when options?.noFollow and gate existingMode on (!options?.noFollow || stat.isFile()); or amend the "Same semantics" doc if the divergence is deliberate.
中文说明
[Suggestion] R8-3:noFollow 符号链接修复(lstat + isFile() 门)只落在了异步变体;文档声称"Same semantics"的 atomicWriteFileSync 仍然用跟随符号链接的 statSync 推导 existingMode,从而继承目标的模式位(0o7777 掩码包含 setuid/setgid/sticky)。本轮探针实测:植入一个目标模式为 0o2666 的符号链接时,异步 noFollow 写入落为 0o644,而同步变体落为 0o2666(继承 setgid + 全局可写)。有一个生产调用方会进入该分支:packages/cli/src/commands/review/save-artifact.ts:349 传 { encoding: 'utf8', noFollow: true } 且不带 mode/forceMode(其余同步 noFollow 调用方都传 forceMode: true + mode,会跳过该推导)。失败场景:在共享主机上,能写工作区目录的本地攻击者在 rejectSymlinkPath 校验之后、写入之前放置 output.json -> <模式为0o2666的文件>,评审产物就会以攻击者选定的权限位落盘;修复后的异步变体在同样场景下以 umask 默认模式创建文件。建议修复:在 atomicWriteFileSync 中镜像异步改动——options?.noFollow 时用 fsSync.lstatSync(targetPath),并以 (!options?.noFollow || stat.isFile()) 为 existingMode 的门;若该分歧是有意的,则修改"Same semantics"文档。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| this.sessionRegistered = await registration; | ||
| if (!this.sessionRegistered) this.sessionRegistryActive = false; |
There was a problem hiding this comment.
[Suggestion] R8-5: The refusal-reset of sessionRegistryActive (both this resolve(false) branch and the symmetric reject branch below) is unpinned: the only refused-registration test ("does not unregister when initial registration was refused") asserts the unregister side, which keys on sessionRegistered — and unregisterSessionRegistry sets sessionRegistryActive = false itself — so it cannot observe a regression of these reset lines. — Failure scenario: probe-measured this round — dropping this.sessionRegistryActive = false from both refusal paths ships every existing test green, while the regressed process queues a patchSessionRecord (a HOME readRecord + identity reads) on every /clear and /cd for its remaining lifetime — precisely the fd-pressure conditions under which registration refusals occur.
Suggested fix:
// config.test.ts
config.trackSessionRegistration(Promise.resolve(false));
await new Promise((r) => setTimeout(r, 0)); // let the chain settle
await config.startNewSession('probe-session');
expect(patchSessionRecordSpy).not.toHaveBeenCalled();
// plus a Promise.reject(new Error('boom')) variant for the catch branch中文说明
[Suggestion] R8-5:sessionRegistryActive 的拒绝复位(此处 resolve(false) 分支与下方对称的 reject 分支)没有被测试钉住:唯一的"注册被拒绝"测试("does not unregister when initial registration was refused")只断言了 unregister 一侧,而该侧依赖的是 sessionRegistered——且 unregisterSessionRegistry 自己也会把 sessionRegistryActive 置为 false——因此它观察不到这两行复位的回归。失败场景:本轮探针实测——把两处拒绝路径上的 this.sessionRegistryActive = false 删掉后,所有既有测试仍然全绿,而回归后的进程会在其余生中每次 /clear 与 /cd 都入队一次 patchSessionRecord(一次 HOME readRecord + 身份读取)——恰恰是注册被拒绝时所处的 fd 压力环境。建议修复如上:补一个"注册被拒后 /clear 不触发 patch"的测试,并为 catch 分支补 Promise.reject 变体。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| }, | ||
| ); | ||
|
|
||
| it("unlinks a corrupt record at this process's own path", async () => { |
There was a problem hiding this comment.
[Suggestion] R8-7: Of the four dispositions of a stale dead-previous-incarnation record (same pid/ns/boot, mismatched start token), the unregister side — unregisterSession unlinks it because matchesLocalIdentity has no token comparison — is the only one with no test: register-overwrites (line 384), patch-refuses (line 670) and sweep-reclaims (line 1002) are all pinned. — Failure scenario: a future "consistency" change adding patch's token guard to unregisterSession (the patch path carries an explicit guard for exactly this record shape) ships green; stale records then linger until the next registration on that PID number or the next sweep. The suite's own comment notes "The unregister path has no stale-incarnation guard".
Suggested fix (Linux-gated, mirroring the patch-side test):
it.runIf(process.platform === 'linux')(
'reclaims a stale record left by a dead previous incarnation of this PID',
async () => {
await registerSession({ sessionId: 'mine', cwd: tmpDir });
await writeRaw(
`${process.pid}.json`,
liveBody({ procStart: `${readLocalBootId()}:1` }),
);
await unregisterSession();
await expect(fs.stat(getSessionRecordPath())).rejects.toThrow();
},
);中文说明
[Suggestion] R8-7:对于"同 pid/命名空间/boot 但 start token 不匹配"的陈旧死亡化身记录,四种处置中只有 unregister 一侧没有测试——unregisterSession 会 unlink 它(因为 matchesLocalIdentity 不比较 token);register 覆盖(第 384 行)、patch 拒绝(第 670 行)、清扫回收(第 1002 行)三种处置都有测试钉住。失败场景:未来一次"一致性"修改把 patch 的 token 守卫加到 unregisterSession(patch 路径恰好对这种记录形状有显式守卫)会绿着通过;陈旧记录从此滞留到该 PID 数字的下一次注册或下一次清扫。套件自己的注释也写着"The unregister path has no stale-incarnation guard"。建议修复如上:补一个 Linux 门控测试,镜像 patch 一侧的写法。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| this.queueSessionRegistryWrite(async () => { | ||
| await patchSessionRecord({ sessionId: newSessionId, cwd: workDir }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R8-8: A dropped /cd patch strands the record's name permanently stale: this /clear patch re-sends only sessionId+cwd and deliberately never name — so patchSessionRecord's skip comment ("skip this patch and let a later /clear or /cd retry it") holds for cwd but not for name. Mirror image of the /cd payload never re-sending sessionId (reported in an earlier round). — Failure scenario: probe-reproduced this round with the real functions — /cd /w/backend → /w/frontend queues {cwd, name}; the one-shot token read hits the fd-pressure window and returns null → patch skipped; a later successful /clear patches {sessionId, cwd} — the record now shows DIRECTORY /w/frontend next to NAME backend-XX in qwen sessions ps until another successful /cd or exit, defeating the DIRECTORY/NAME disambiguation the tests pin. The /clear comment's premise ("the directory it names has not changed") silently fails whenever the preceding /cd patch was dropped.
Suggested fix: have transitions re-send the full field set they may repair (e.g. include a re-derived name in the /clear payload when the directory changed — weighing the deliberate no-rename rule's suffix tradeoff), or document that a dropped /cd patch leaves name stale until the next /cd; at minimum correct the overclaiming skip comment in patchSessionRecord.
中文说明
[Suggestion] R8-8:被丢弃的 /cd 补丁会让记录的 name 永久陈旧:此处 /clear 补丁只重发 sessionId+cwd,刻意不发 name——因此 patchSessionRecord 的跳过注释("跳过本次补丁,让之后的 /clear 或 /cd 重试")对 cwd 成立,对 name 不成立。这与"/cd 载荷从不重发 sessionId"互为镜像(早前轮次已报告)。失败场景:本轮用真实函数复现——/cd /w/backend → /w/frontend 入队 {cwd, name};一次性 token 读取撞上 fd 压力窗口返回 null → 补丁被跳过;之后一次成功的 /clear 补丁 {sessionId, cwd}——记录从此在 qwen sessions ps 中显示 DIRECTORY /w/frontend 旁边是 NAME backend-XX,直到下一次成功的 /cd 或进程退出,破坏了测试所钉住的 DIRECTORY/NAME 区分能力。/clear 注释的前提("它所命名的目录没有变化")在前一次 /cd 补丁被丢弃时悄然失效。建议修复:让转换重发其可能修复的全部字段(例如当目录变化时在 /clear 载荷中加入重新推导的 name——需权衡刻意不重命名规则的 suffix 取舍),或文档化"被丢弃的 /cd 补丁会使 name 陈旧到下一次 /cd";至少修正 patchSessionRecord 中过度承诺的跳过注释。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // numeric `qwenVersion` would reach every consumer typed as a string. | ||
| await writeRaw( | ||
| `${process.pid}.json`, | ||
| liveBody({ procStart: 12345, qwenVersion: 7 }), |
There was a problem hiding this comment.
[Suggestion] R8-9: readRecord's pidNs type-coercion branch (typeof pidNs === 'number' && Number.isFinite(pidNs) ? pidNs : null) is the only optional-field validation branch with no test — this wrong-type-optional test pins procStart and qwenVersion but not pidNs, and a full census of the suite finds no wrong-typed pidNs anywhere. — Failure scenario: probe-measured this round — replacing the branch with an unchecked passthrough ships the entire 82-test registry suite green; a record with wrong-typed pidNs (string "4026531836") then fails the strict-equality namespace guard (record.pidNs !== ownNamespace, string vs number) and is silently skipped forever — observable on macOS/Windows, where ownNamespace is null and a correctly-nulled pidNs passes null === null (on Linux the regression is unobservable — both variants skip).
Suggested fix:
await writeRaw(
`${process.pid}.json`,
liveBody({ pidNs: '4026531836' }),
);
expect(await listLiveSessions()).toEqual([liveBody()]);(guarded with it.runIf(process.platform !== 'linux'), mirroring the file's platform-observability pattern)
中文说明
[Suggestion] R8-9:readRecord 的 pidNs 类型归一分支(typeof pidNs === 'number' && Number.isFinite(pidNs) ? pidNs : null)是唯一没有测试的可选字段校验分支——本"错误类型可选字段"测试钉住了 procStart 与 qwenVersion,唯独没有 pidNs;对整个套件普查也未发现任何错误类型的 pidNs。失败场景:本轮探针实测——把该分支换成不做检查的直接透传,全部 82 个注册表测试仍全绿;携带错误类型 pidNs(字符串 "4026531836")的记录会在严格相等的命名空间守卫(record.pidNs !== ownNamespace,字符串对数字)处失配并被永久静默跳过——在 macOS/Windows 上可观测(那里 ownNamespace 为 null,正确归一为 null 的 pidNs 能通过 null === null;在 Linux 上该回归不可观测——两种实现都会跳过)。建议修复如上:补一个 pidNs 用例,并按该文件的平台可观测性惯例加 it.runIf(process.platform !== 'linux') 守卫。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') { | ||
| return UNREADABLE; | ||
| } | ||
| return READ_ERROR; |
There was a problem hiding this comment.
[Suggestion] R8-10: readRecord lumps structurally permanent symlink-resolution failures (ELOOP/ENOTDIR) into the transient read-error bucket — turning a planted symlink loop at <pid>.json into a PID-slot poison nothing ever reclaims, inside the threat model this file itself invokes for the noFollow write guard. — Failure scenario: probe-reproduced this round — a self-referencing symlink at this process's record path: registerSession returns false, listLiveSessions skips without sweeping, patch no-ops, unregister refuses to unlink. Every future session drawing that PID number is refused registration and stays undiscoverable for its lifetime; the loop survives boots until removed by hand. The refusal buys nothing here: the noFollow rename would safely replace the symlink. Distinct from the transient EMFILE-class refusal (which self-heals on retry or the next incarnation): ELOOP is permanent by construction.
| if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') { | |
| return UNREADABLE; | |
| } | |
| return READ_ERROR; | |
| if ((error as NodeJS.ErrnoException)?.code === 'ENOENT') { | |
| return UNREADABLE; | |
| } | |
| if ( | |
| (error as NodeJS.ErrnoException)?.code === 'ELOOP' || | |
| (error as NodeJS.ErrnoException)?.code === 'ENOTDIR' | |
| ) { | |
| return UNREADABLE; | |
| } | |
| return READ_ERROR; |
中文说明
[Suggestion] R8-10:readRecord 把结构上永久性的符号链接解析失败(ELOOP/ENOTDIR)归入瞬时 read-error 桶——在本文件自己为 noFollow 写守卫所援引的威胁模型之内,把一个植入 <pid>.json 的符号链接环变成没有任何路径会回收的 PID 槽位毒物。失败场景:本轮探针复现——在本进程记录路径放置自引用符号链接:registerSession 返回 false,listLiveSessions 跳过但不清扫,patch 不做处理,unregister 拒绝 unlink。此后每一个抽到该 PID 数字的会话都会被拒绝注册,并在其整个生命周期内不可发现;该环在重启后依然存在,直到手工删除。这里的拒绝毫无收益:noFollow 的 rename 本可以安全地替换该符号链接。与瞬时的 EMFILE 类拒绝不同(后者可经重试或下一个化身自愈):ELOOP 按构造就是永久性的。建议修复如上:把不可能瞬时的解析失败(至少 ELOOP 与 ENOTDIR)映射为 UNREADABLE,保留真正瞬时的错误码(EMFILE、EIO、ESTALE、EACCES)为 READ_ERROR。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Maintainer re-verification on
|
| build | early exits | leaked records |
|---|---|---|
head fd768c90 |
12 | 0 |
same build, await this.sessionRegistryWrite removed from Config.unregisterSessionRegistry() |
12 | 10 |
One line, ten leaks. The drain is load-bearing, and a leaked record here is the worst kind — its PID is dead, but nothing unlinks it until some later enumeration happens to sweep it.
2. Live behaviour on the new head
Two real TUIs (each having completed a round-trip to the provider), read from a third shell. Directory 0700, records 0600, procStart/pidNs null as designed on this platform.
Lifecycle and concurrency, all re-confirmed on this head:
/quitand SIGTERM (closing a terminal,kill <pid>) both unregister cleanly. SIGINT does not exit — the TUI owns it — so it is a non-event here.- SIGKILL strands the record; the next enumeration proves the PID dead and sweeps it. Never earlier, never anything else.
qwen -pnever registers, mid-flight or after.- 6 sessions started at once, then torn down three ways simultaneously (3× SIGTERM, 2× SIGKILL, 1 left running): the 3 clean exits removed their own records, one
psswept exactly the 2 orphans, and the live session was untouched. /clearrepointssessionId, keepsname,pidandstartedAt./cdpatchescwdand re-derivesname(proj-alpha-4b→nested-workdir-b6).
3. macOS PID reuse, reproduced rather than reasoned about
The PR states the tradeoff plainly: no start token on macOS, so liveness degrades to kill(pid, 0). Last time I confirmed the degradation; this time I made it bite.
A session was SIGKILL'd (record stranded, pid 10728). I then walked the PID counter all the way around — 169,345 forks, 204 seconds — until /bin/sleep 400 landed on 10728. qwen sessions ps then reports:
NAME PID AGE DIRECTORY
alpha-76 10728 3m /private/tmp/pr8969demo/alpha
/bin/sleep is now a Qwen Code session, in a directory it has never seen, and the record can never be swept while it lives.
To be fair to the PR: this needs a crash (a clean exit unregisters), and nobody running ps before the wrap. But the wrap is not exotic — the PID space here is ~100k, and this machine cycled it several times over during one afternoon of building. This is not a blocker and I would not hold the merge for it, but two things follow:
- Worth one sentence in
docs/users/features/commands.md: on macOS/Windows a crashed session's row can be resurrected by an unrelated process, so treat apsrow as a hint, not proof. - If Cross-session messaging: let Qwen Code sessions on the same machine message each other #8724 sends messages to PIDs read out of this registry, the message target should be re-validated at send time rather than trusted from the row. Enumerating a handful of live records is O(live sessions), not O(all records) — a single
ps -o lstart= -p <pid>on the few that passkill(pid,0)would close this on macOS without the per-record subprocess cost the PR rightly rejected. Follow-up material, not this PR.
4. Hardening, suites, types, and whether the tests still bite
The full guard matrix, driven against the real binary with a hand-built registry directory — 22 assertions, 0 failures: strict ^\d+\.json$ filename guard (2026-planning-notes.json never read, listed or deleted), filename/contents disagreement skipped-not-swept, newer-schema / malformed / oversized records skipped-not-swept, stale temp reaped while a fresh one is spared, newest-first ordering, unreadable registry dir degrading to "no peers" with exit 0, --json emitting exactly the nine documented fields and zero bytes when empty.
- Suites on macOS:
packages/core757 passed / 17 skipped,packages/cli124 passed / 0 skipped. The skips are exactly the documented Linux-onlyit.runIfassertions. Counts are above the PR body's 617/119 because later rounds added tests. tsc --noEmit,eslint,prettier --checkon all 19 changed TS files: clean.- Mutation check — because 17 assertions are skipped on this platform, "green on macOS" needed proving. I introduced 11 single-line mutants into the load-bearing guards (loosen the filename anchor, drop the filename/contents check, drop the size cap, accept a newer schema, reap temps of any age, let
matchesLocalIdentityaccept a foreign PID, count a zombie as alive, flip the tokenless fallback to delete, drop the nonsense-PID rejection, removepssanitization, stop clamping negative age). 11 introduced, 11 killed. The suite still bites here. - A/B against base: same worktree,
packages/cli/src/commands/sessions.tsreverted to main's version and re-bundled. Before:sessions psis not a subcommand. After: it works. Nothing else differs.
5. Two sentences in the PR description still contradict the code
Both are description-only; the code is right in both cases.
-
"
/clearand/resumepatch the record's session id, and a directory switch patches its cwd, but neither patches the name"A directory switch does patch the name — measured
proj-alpha-4b→nested-workdir-b6.config.tssays so explicitly ("Unlike the /clear path,namefollows"), and that is the better behaviour. I raised this last round; the sentence is still there in both the English and Chinese sections. -
"leaves the namespace machinery out"
readPidNamespaceId(), thepidNsrecord field, and the namespace guard inlistLiveSessionsare all present — they came back infe54a9235b, after that paragraph was written. The Linked Issues section reads as if the current diff has no namespace logic, and it has quite a lot.
6. Pre-existing gap this PR names (separate issue, not a blocker)
The PR points at sessions/list.ts's private sanitizer having no bidi pass and leaves it out of scope. Confirmed at byte level with the same Trojan-Source payload through both sibling commands:
sessions ps : clean: 0 bidi override chars
sessions list: LEAKS 3 bidi override char(s)
sessions list renders open the file gpj.exe as open the file exe.jpg. Worth its own issue against the shipped command.
7. Records that can never be reclaimed (by design — listing so it is a known cost)
A record is left forever, needing manual removal, when it is: a filename/contents mismatch, larger than 64 KiB, or a higher schemaVersion. Every one of these resolves toward keeping, which is the right call for a discovery index, and normal code cannot produce any of them. Just noting that ~/.qwen/sessions has no self-cleaning path for hand-planted or corrupted entries.
Not covered here: Linux-only identity paths (boot-id prefix, PID-namespace guard, zombie detection) beyond their unit tests, and Windows entirely. Evidence images: pr-assets/8969-verify.
中文版本(Chinese version)
维护者复验 fd768c90 —— 真实构建 + 真实会话(macOS)
接续上一轮对 e230980 的验证:此后又合入了三个 commit,其中一个是功能性的(9f2dbc1b fix(core): serialize session registry lifecycle)。本次是在当前 head 上的复验,外加上次没做的几项检查。
环境:在 fd768c90 上全新 worktree,npm ci && npm run bundle,macOS 15.6 (arm64),Node v24.18.1。用隔离 QWEN_HOME 在 tmux 里启动真实交互式 TUI,对接本地 OpenAI 兼容端点。下面每一条断言都驱动实际打包产物 dist/cli.js —— 没有单测替身,没有 mock 文件系统。
结论:从我这边看可以合入。 所有行为与文档一致。唯一值得维护者注意的发现并不是本 PR 的缺陷,而是把它已经声明的取舍量化了 —— 见 §3。PR 描述里有两句仍与代码矛盾(§5),但都只是描述问题。
1. 9f2dbc1b 的串行化是必要的,而且有效
这是我上次验证之后新增的 commit,所以我直接针对它设计了用例。注册被刻意设计为不被 UI 启动 await,这意味着一个很早退出的会话可能在注册写入落盘之前就跑到了 cleanup。
我在注册窗口内的 12 个时间点上向真正注册的 UI 子进程(CLI 会 relaunch 自身,启动器不是它)发送 SIGTERM,其中两次发生在注册尚在途中:
| 构建 | 早退次数 | 泄漏记录数 |
|---|---|---|
head fd768c90 |
12 | 0 |
同一构建,从 Config.unregisterSessionRegistry() 删去 await this.sessionRegistryWrite |
12 | 10 |
一行之差,十次泄漏。这个 drain 是承重的;而且这里泄漏的记录是最糟的一类 —— 它的 PID 已死,但在某次枚举恰好扫到它之前,没有任何东西会删除它。
2. 新 head 上的实际行为
两个真实 TUI(各自都完成了一次到 provider 的往返),由第三个 shell 读取。目录 0700、记录 0600,本平台上 procStart/pidNs 按设计为 null。
生命周期与并发,均在本 head 上重新确认:
/quit与 SIGTERM(关闭终端、kill <pid>)都能干净注销。SIGINT 不会退出 —— TUI 自己接管了它 —— 因此在这里不构成场景。- SIGKILL 会留下记录;下一次枚举证明 PID 已死并清扫它。不会更早,也不会由别的东西完成。
qwen -p全程不注册,运行中和结束后都不注册。- 6 个会话同时启动,然后以三种方式同时拆除(3× SIGTERM、2× SIGKILL、1 个继续运行):3 个干净退出各自删除了自己的记录,一次
ps恰好清扫了那 2 个孤儿,存活会话分毫未动。 /clear更新sessionId,保持name、pid、startedAt不变。/cd更新cwd并且重新派生name(proj-alpha-4b→nested-workdir-b6)。
3. macOS 的 PID 复用:不是推理,是复现
PR 把取舍写得很清楚:macOS 上没有 start token,存活判定退化为 kill(pid, 0)。上次我确认了这个退化;这次我让它真的咬人。
先 SIGKILL 掉一个会话(记录残留,pid 10728),然后把 PID 计数器整整绕了一圈 —— 169,345 次 fork、204 秒 —— 直到 /bin/sleep 400 落在 10728 上。此时 qwen sessions ps 报告:
NAME PID AGE DIRECTORY
alpha-76 10728 3m /private/tmp/pr8969demo/alpha
/bin/sleep 就这样成了一个 Qwen Code 会话,工作目录是它从未见过的路径,而且只要它活着,这条记录就永远不会被清扫。
公平地说:这需要一次崩溃(正常退出会注销),并且在回绕之前没有人跑过 ps。但回绕并不罕见 —— 这里的 PID 空间只有约 10 万,这台机器在一个下午的构建过程中就绕了好几圈。这不是阻塞项,我不会因此拦住合并,但由此有两点建议:
- 值得在
docs/users/features/commands.md里加一句:在 macOS/Windows 上,崩溃会话的那一行可能被一个无关进程"复活",所以应把ps的一行视为线索而非证据。 - 如果 Cross-session messaging: let Qwen Code sessions on the same machine message each other #8724 要按本注册表读出的 PID 发送消息,目标应在发送时重新校验,而不是直接信任该行。枚举存活记录的数量是 O(存活会话数) 而非 O(全部记录数) —— 只对通过
kill(pid,0)的那少数几条各跑一次ps -o lstart= -p <pid>,就能在 macOS 上补上这个缺口,而不必付出 PR 正确拒绝的"每条记录派生一个子进程"的代价。这属于后续工作,不属于本 PR。
4. 加固项、测试套件、类型检查,以及测试是否还有牙
完整的守卫矩阵,用手工构造的注册表目录驱动真实二进制 —— 22 条断言,0 失败:严格的 ^\d+\.json$ 文件名守卫(2026-planning-notes.json 从不被读取、列出或删除)、文件名与内容不符时跳过但不清扫、更高 schema / 畸形 / 超大记录跳过但不清扫、陈旧 temp 被回收而新 temp 被保留、最新优先排序、不可读的注册表目录降级为"无对等会话"且退出码为 0、--json 恰好输出文档所述的九个字段且空目录时输出零字节。
- macOS 上的测试套件:
packages/core757 通过 / 17 跳过,packages/cli124 通过 / 0 跳过。跳过的正是文档所述仅限 Linux 的it.runIf断言。数字高于 PR 描述中的 617/119,因为后续轮次新增了测试。 tsc --noEmit、eslint、prettier --check覆盖全部 19 个改动的 TS 文件:干净。- 变异测试 —— 因为本平台跳过了 17 条断言,"macOS 上是绿的"这件事本身需要被证明。我向承重守卫注入了 11 个单行变异体(放松文件名锚点、去掉文件名/内容比对、去掉大小上限、接受更高 schema、回收任意年龄的 temp、让
matchesLocalIdentity接受外来 PID、把僵尸算作存活、把无 token 的兜底从保留翻成删除、去掉非法 PID 拒绝、移除ps的终端消毒、取消负数 age 的钳制)。注入 11 个,杀死 11 个。 测试在这里仍然有牙。 - 与 base 的 A/B:同一 worktree,把
packages/cli/src/commands/sessions.ts还原为 main 的版本后重新打包。之前:sessions ps不是子命令。之后:可以正常工作。其余无任何差异。
5. PR 描述中仍有两句与代码矛盾
两处都只是描述问题,代码本身都是对的。
-
"
/clearand/resumepatch the record's session id, and a directory switch patches its cwd, but neither patches the name"目录切换确实会更新 name —— 实测
proj-alpha-4b→nested-workdir-b6。config.ts里也明确写了("Unlike the /clear path,namefollows"),而且这个行为更合理。上一轮我提过;这句话在英文和中文两处都还在。 -
"舍弃了 namespace 那套机制"
readPidNamespaceId()、记录中的pidNs字段、以及listLiveSessions里的 namespace 守卫全都在 —— 它们在fe54a9235b中回来了,晚于那段文字的撰写时间。关联 Issue 一节读起来像是当前 diff 里没有 namespace 逻辑,而实际上有不少。
6. 本 PR 点名但列为范围外的既有缺口(应独立开 issue,不阻塞)
PR 指出 sessions/list.ts 的私有 sanitizer 没有 bidi 处理,并将其排除在范围之外。用同一个 Trojan-Source 载荷分别经过两个同族命令,在字节层面确认:
sessions ps : clean: 0 bidi override chars
sessions list: LEAKS 3 bidi override char(s)
sessions list 会把 open the file gpj.exe 渲染成 open the file exe.jpg。这是一个已发布命令上的问题,值得单独开 issue。
7. 永远无法回收的记录(属于设计,列出以便知情)
以下情形的记录会永久留存、只能手工删除:文件名与内容不符、大于 64 KiB、schemaVersion 更高。这些都倾向于"保留",对一个发现索引来说是正确的选择,而且正常代码写不出其中任何一种。只是提醒:~/.qwen/sessions 对手工放置或损坏的条目没有自清理路径。
未覆盖:仅限 Linux 的身份路径(boot-id 前缀、PID namespace 守卫、僵尸检测)在单测之外的实测,以及 Windows 全部。证据图片:pr-assets/8969-verify。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 105 passed · 0 failed · 105 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:105 通过 · 0 失败 · 105 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict, follow-up round) - verified head Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 105 passed · 0 failed · 105 total 中文 — 判定:✅ 通过 · 可合入(agent 判定,跟进轮)
Verification reportPR 8969 — feat(core): add a live-session registry and
|
| # | Finding (round 1) | Severity | Status at fd768c9 | Agree? |
|---|---|---|---|---|
| 1 | Documented ps --json | jq -r .cwd example forwards unsanitized same-user text to the consumer's terminal |
Low (informational) | Fixed — the suggested docs note landed: "JSON output is raw data … sanitize before rendering" (docs/users/features/commands.md:720) and the jq -r example now carries its own warning (lines 730–732). Behavior deliberately unchanged and re-measured (H9c/W28: hostile bytes still round-trip raw), consistent with the pre-existing sessions list --json convention. |
Yes |
| 2 | /clear and directory-switch patches rode the sidecar's runtimeStatusEnabled gate, so with the sidecar disabled the record advertised the pre-/clear sessionId until exit |
Informational (disclosed tradeoff) | Fixed — patches now ride the registry's own lifecycle gate (sessionRegistryActive) on a separate never-awaited chain (queueSessionRegistryWrite). Re-measured live in cell H10 (3/3): with chats/ chmod 555 the sidecar write fails, registration still succeeds, /clear still patches the record's sessionId, and chats/ stays empty. The round-1 stale-until-exit behavior no longer reproduces. |
Yes |
Declined/deferred rows from round 1: none. Both informational items were addressed in code/docs; no deferred tradeoff moved (re-measured, not diffed).
Central claim and A/B
Central claim (carried forward, re-measured at the new head): an interactive session registers <QWEN_HOME>/sessions/<pid>.json at boot, qwen sessions ps lists exactly the live sessions (table + JSON Lines), clean exit unlinks the record, and provably-dead records are swept during enumeration.
Both arms ran the identical scenario: real interactive TUI under script(1) ptys (FIFO-fed pty stdin), auth satisfied by a loopback OpenAI-compatible peer, registry isolated per arm via QWEN_HOME. The base arm is a scratch worktree at HEAD^1 with its own build; the root node_modules is junctioned with @qwen-code/qwen-code-core/@qwen-code/qwen-code re-pointed into the base tree (readlink -f asserted to land inside the base tree before any cell ran; lockfile and all package manifests untouched by the PR, so the shared install is a clean control). One setup note: the repo carries non-git-tracked nested packages/*/node_modules version overrides (mime, fdir, ignore, ajv …) that a bare worktree lacks — the first base build failed on them; they were junctioned in identically for both arms' interpretation and are third-party-only (no workspace links), so the control stays pure. Witness: 01-ab-e2e-lifecycle.png (live capture of the 35/35 run).
| Cell (same scenario both arms) | base 5677823a |
head fd768c90 |
|---|---|---|
| Interactive boot (TUI rendered, live process) | boots (typescript 1862 B, live pid); no record in the same 15 s window, no sessions/ dir (B1a–c) |
boots; 2 records ≤25 s, dir 0700, files 0600, procStart == independent /proc oracle, pidNs == namespace inode, name proj-alpha-b2 (H1a–g) |
qwen sessions ps |
exit 1, no table, help offers list only (B2a–b, B3) |
exit 0, header + 2 rows, PIDs at column offset 22, AGE formatted 2/2, DIRECTORY shown (H3a–e) |
qwen sessions ps --json |
n/a | 2 JSON Lines, full 9-field set; empty registry → silent stdout, exit 0 (H4a–b, H8) |
/clear mid-session |
n/a | sessionId patched in place; name + cwd unchanged (H5a–b) |
/quit one session |
n/a | record unlinked ≤12 s; sibling record untouched (H6a–b) |
| SIGKILL (crash) | n/a | record litters; next ps sweeps it → "No other interactive Qwen Code sessions are running." (H7a–c) |
| Hostile record (ANSI + bidi in name/cwd) | n/a | table contains no ESC byte and no bidi override; --json round-trips the bytes raw, as documented (H9a–c) |
Sidecar disabled (chats/ 0555) + /clear |
n/a | registration succeeds, /clear patches, chats/ stays empty — prev finding 2 fixed (H10a–c) |
Assertion totals per arm: head E2E 23 + hostile cell 3 + sidecar cell 3 = 29 head-side, base 6 (35 total, 0 fail).
Delta verification (commits since round 1)
The new commits add identity hardening (pidNs field, cross-machine boot guard, refuse-on-unreadable with one retry, read-error discrimination, sweep re-read-before-unlink, patch token agreement, temp reaping) and lifecycle serialization (Config.trackSessionRegistration / queueSessionRegistryWrite / unregisterSessionRegistry). Each new guard was driven through the compiled head build by a mock-free wire-oracle harness (02-registry-harness-wire-oracle.png, 50/50) and pinned by the mutation matrix (03-mutation-matrix-kills.png):
| Guard (new since e0b9271) | Harness cells | Mutant | Killed by |
|---|---|---|---|
| Cross-namespace records neither listed nor swept | W07a/b | M3 (drop pidNs guard) | "neither lists nor sweeps a record from a different PID namespace, even a dead one" + "does not list a foreign-namespace record under a live PID" (2 red) |
| Cross-machine (boot) records neither listed nor swept, dead or live | W08a/b, W09a/b | M4 (drop boot guard) | "neither lists nor sweeps a record from another machine's boot" + "leaves every boot-prefixed record alone while the local boot id is unreadable" (2 red) |
| Sweep re-read-before-unlink race guard | (window not injectable live) | M2 (drop re-read) | "does not unlink a record replaced between the liveness verdict and the sweep" (1 red) |
| Patch refuses stale-token record (chimera) | W21 | M5 (drop token agreement) | "refuses to merge into a stale record left by a dead previous incarnation of this PID" + "refuses to merge when this process's own start token is unreadable" (2 red) |
| Register refuses unreadable namespace id (retry then refuse) | (refusal path exercised via W17 family) | M6 (accept null ns) | "refuses to write a record when the namespace id stays unreadable" (1 red) |
| Unregister identity guards (foreign / newer-schema / read-error) | W24, W25 | M7 (unconditional unlink) | 5 red incl. "leaves a foreign-identity record at its path alone" |
| Orphaned temp reaping (aged reaped, young spared) | W15a/b | M8 (drop reap) | "sweeps registration temp files orphaned by a crashed write" (1 red) |
| Symlink-safe registration (carry-over positive control) | W16a–c | M1 (drop noFollow) | "replaces a pre-planted symlink instead of writing through it" (1 red) |
Control (unmutated) GREEN; 8/8 killed, no survivors. The positive control (M1) proves the harness can turn the suite red, and every kill names the test that pins the guard.
Zombie exclusion, live (04-zombie-live-probe.png, 4/4): a real unreaped zombie (kernel state Z, produced by a fork-without-waitpid parent) still answers kill(pid, 0) and still exposes its original start token in /proc/<pid>/stat — yet isPidAlive returns false and isSameProcess(zombie, its-own-matching-token) returns false. Without the zombie exclusion the sweep would keep a dead session listed until its parent reaps it; the unit suite also pins the cross-user-zombie and comm-with-) parsing cases.
Lifecycle serialization (9f2dbc1b): pinned by config.test.ts (532 tests, incl. the sessionRegistryActive-gate regression case at line ~6437) and by startInteractiveUI.test.tsx; the vacuity probe removed the registration block from startInteractiveUI.tsx in a scratch copy and all 4 of its tests went red with the intended assertion ("expected spy to be called 1 times, but got 0"), not an import error — the wiring tests are load-bearing. E2E cells H5/H6/H10 drive the same chain through the real TUI.
Name derivation (deriveSessionName NFC/code-point hardening): W26a–e — NFD café normalized, CJK kept, astral letter at the 32-code-point boundary kept whole (no lone surrogate), combining marks kept, invalid-only basename falls back to session-xx.
Reviewer Test Plan walk-through (re-measured at fd768c9)
| Plan step | Result |
|---|---|
| Core suites (session-registry, process-liveness, config, teamHelpers) | PASS — 690/690 (82 + 30 + 532 + 46). The description's "617" predates the round-4 test additions. |
| CLI suites (sessions, ps, list, gemini) | PASS — 124/124 across the four, plus startInteractiveUI.test.tsx (4) and the touched atomicFileWrite + runtimeStatus.config files (89/89). The description's "119" is likewise stale. |
Two sessions in different dirs → one sanitized row each; --json one object per line |
PASS — H2/H3/H4 |
Exit one session → row disappears; crash variant swept on next ps |
PASS — H6/H7 |
Findings
- Informational (non-blocking):
patchSessionRecord's identity-field exclusion is type-level only. ThePartial<Omit<…, 'pid' | 'schemaVersion' | 'procStart' | 'pidNs'>>signature is compile-time; the runtime merge is{ ...record, ...patch }, so a JavaScript caller could passpid/procStart/pidNsand graft them. I verified there is no reachable exploit: every in-repo caller is TypeScript and passes onlysessionId/cwd/name, and the registry is a 0700 same-user directory where a same-uid attacker can rewrite the file directly anyway — the fields are not a security boundary, only sweep-consistency metadata. A one-line runtime omit would make the comment's promise hold at runtime too; not required to merge. - Informational (non-blocking): the description's expected suite counts are stale. It says "617 passing in core, 119 in cli"; measured at the verified head: 690 and 124 for the same file sets (the gap is tests added by the round-4 hardening commits). A reviewer reproducing the plan will see different totals; behavior is green either way.
No blocking findings. No injection attempts detected in the PR text; author claims were treated as hypotheses and tested (all held).
Not covered
- Windows/macOS liveness degradation (
procStart/pidNsnull → plain liveness): not executable in this Linux container; covered by the suites' platform-guarded mocks only. - EPERM cross-user liveness and unreadable-registry-dir (EACCES) →
[]: cannot be produced as uid 1000 single-user here; covered by injected-errno unit tests only. - Local boot-id outage on the reader side:
/proc/sys/kernel/random/boot_idcannot be made unreadable in this container; the guard is pinned by the M4-killed test "leaves every boot-prefixed record alone while the local boot id is unreadable" rather than by a live outage cell. - Sweep re-read race, live: the verdict-to-unlink window is not injectable without instrumentation; pinned by the M2-killed test instead.
- Per-commit attribution: shallow depth-2 checkout (1 of 11 commits reachable); the aggregate diff is verified, per-commit rows are not individually exercised. The two review-manifest commits net to zero against main and were not exercised.
- Repo-wide lint/typecheck: left to the PR's own CI.
- Cross-user zombie (EPERM + Z): unit-tested only; the live probe covered the same-user zombie.
Methodology
Environment: node:22-bookworm CI container, merge-ref checkout at depth 2 (HEAD merge 98877b6b, HEAD^1 base 5677823a, HEAD^2 head fd768c90), uid 1000. Head cells drove the compiled dist/ output; interactive sessions ran under script(1) ptys with FIFO-fed pty stdin (feeding the FIFO into the child's stdin directly trips the CLI's non-TTY headless path — the FIFO must feed script itself), auth satisfied by the repo's loopback OpenAI-compatible mock, registry isolated per arm via QWEN_HOME. The base control is a git worktree at HEAD^1 with the root node_modules junctioned and @qwen-code/* re-pointed into the base tree (realpath asserted), plus junctions for the non-git-tracked nested packages/*/node_modules overrides the first base build failed without; removed after capture. Harnesses (cells/registry-harness.mjs, cells/e2e.sh, cells/mutation-runner.mjs, cells/zombie-probe.sh, cells/get-field.mjs, cells/junit-failures.mjs) are mock-free against the code under test; raw logs in logs/, witnesses in evidence/ (01 A/B E2E live capture, 02 wire-oracle harness, 03 mutation matrix, 04 zombie probe, 05 focused gates). One harness-side flake was observed and fixed (a /clear keystroke raced the TUI input handler under capture load; a settle delay + retry send fixed it — the cell passed 3/3 plain runs and the captured run). Assertion counts: harness 50 + E2E 35 + zombie 4 + mutation control+kills 9 + vacuity 1 + focused gates 3 + full-suite attribution 2 + docs-note 1 = 105. Full-workspace attribution: head core 70 failed | 20166 passed, base core 70 failed | 20044 passed with byte-identical failing-name sets; head cli 12 failed | 20230 passed, base cli 12 failed | 20210 passed, likewise identical — all 82 environmental (logger, ide-client, token-storage, skill/subagent managers, installation/memory/rules discovery, cli settings), none in the changed surface.
Evidence images
— Qwen Code · sandboxed verification
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
The doc was written against a pre-#8969 snapshot while stamping 179c8f8 as baseline. #8969 (merged 2026-08-17, ancestor of that baseline) already shipped the live-session registry, `qwen sessions ps`, and `utils/process-liveness.ts`, so section 1.3 (no discovery of any kind), section 1.5 (absence claim), and Stage 1 (revive #8728) all describe work that has landed. Restate discovery as liveness-only with no messaging channel, mark #8728 superseded, and shrink Stage 1 to carry-forward hardening. Also fix stale citations (TeamManager.ts 312 -> 325, harness 751 -> 1,456 LOC, teamHelpers 378 -> 365, tasks.ts 1,050 -> 1,056).
Two were dissolved by earlier changes rather than answered: - Remote join has no subject left, since there is no join. The question that survives is whether A may give B unasked-for work: yes, but assignment is a proposal, not an obligation. Naming an owner is not the authority to compel one, which is what keeps the peer model from quietly becoming a hierarchy. - Re-engagement over #8724 was a process item in a technical list, and moot: #8969 landed the registry independently and the rest is public on an open branch. Nothing needs anyone's agreement to proceed. Same category error as the conflict retracted in 1.6 — treating a social step as a dependency. Three answered against the code: - Storage roots stay split for now; consolidate before Stage 5 ships the board surface, since that is when the layout first becomes externally readable. The merged registry already carries schemaVersion. - Daemon sessions register. Only startInteractiveUI calls registerSession today and SessionRegistryRecord has no kind field; 2.1 already draws the daemon as a participant, and kind is needed anyway to tell interactive, daemon and wrapped foreign agents apart. - Sub-session residency stays out of scope. create-sub-session states it is NOT kept resident; making it resident rebuilds the spawn model this design exists to avoid. Whoever wants a resident participant starts a session. Kept as a resolutions section rather than deleted, so they are not relitigated.
The strongest objection to a fetch contract is that an idle interactive session never fetches, so a question addressed to it is never answered. That is real, and it is not an argument for push: responsiveness is a property of how often a participant chooses to look, and looking costs one readdir plus a few small reads. A five-second idle timer with a footer badge is below the threshold a human notices, while keeping the property that makes the contract work — the participant chose the moment, so no door is open. Push therefore buys nothing in v1, not even latency. 2.8 settles what a board is on disk: one root per board, an explicit name so the cross-workspace case is expressible (a board that is a directory cannot span two repos), participant records keyed by name that read liveness from #8969's pid-keyed registry rather than heartbeating, name claiming by suffixing through the existing generateUniqueTeammateName, and manual pruning because automatic deletion of a record someone may be mid-read on is a concurrency problem worth not having yet. Also bounds --wait: default 30s timeout and an exit status that distinguishes found from timed out, since a foreign agent running it is blocking its own turn.
|
Released in v0.21.14. |
…rrors - The reused agents/team helpers do NOT supply 0700/0600; mandate the modes explicitly in 2.8/2.9 following session-registry.ts (#8969), and correct 2.5/4 to name the mandated mode rather than claim the substrate provides it. - 0.3: #8724 is the open umbrella; its 7,018-line implementation is #8730. - 1.1: split retry counts (tasks.ts 30, mailbox.ts 10).


















What this PR does
Each interactive session now records itself while it runs, and removes that record when it exits, so the question "which Qwen Code sessions are running on this machine right now" can be answered by reading one small directory instead of walking every project's transcript history. A new
qwen sessions pssubcommand prints them, with--jsonemitting JSON Lines for scripting.A record is considered live only when its process is running and the recorded start identity still matches, so neither a recycled PID nor a reboot can resurrect a dead session. Records that cannot be proven dead are always kept: making a running session invisible is the worst outcome here, so every ambiguous case resolves toward keeping.
Why it's needed
This is the discovery surface the cross-session messaging work in #8724 needs, landed on its own because it is useful by itself and changes nothing about how an existing session behaves.
The obvious alternative is the existing
runtime.jsonsidecar, and it cannot serve this purpose. It is keyed by session id inside a per-project directory and is deliberately never unlinked — not on clean quit, not on crash — so its presence carries no liveness signal at all. The cost of asking it this question is already visible in the codebase:isSessionRuntimeActiveinworktreeSessionService.tsis roughly 150 lines of candidate-directory guessing plus a recursive filesystem scan, and it only answers whether one known session is alive. Enumerating every live session that way costs that times N, and it grows with transcript history rather than with the number of live sessions. The two coexist after this change:runtime.jsonstays the stable, kimi-compatible "which session is PID X serving" sidecar for external observers, and this is the discovery index for the CLI's own features.Reviewer Test Plan
How to verify
Run the two new suites and the suites this change touches:
Expected: 617 passing in core, 119 in cli. Then confirm the feature by hand — start a session in one terminal, run
qwen sessions psin another, and check that the row appears with the right directory; quit the session and check that the row disappears and~/.qwen/sessions/is empty again.Points worth a reviewer's attention, each corresponding to a specific failure this guards against rather than to defensive habit:
<boot_id>:<starttime>, not a bare PID.starttimecounts clock ticks since boot, so it only identifies a process within one boot. A record survives a crash or power loss, and after the reboot an unrelated process can reuse both the PID and the tick count — a phantom session that the reuse guard itself agrees is live, and which therefore can never be swept. When the boot id is unreadable the function returnsnullrather than a bare tick count, because emitting two token shapes on one machine would let a reader that has the boot id "mismatch" a live session recorded without it and delete its record.session-writer-lease.tsbuilds the same Linux identity and is deliberately left alone. Its token is a persisted on-disk format with takeover semantics, so unifying the two changes that file's contract rather than refactoring it. There is a comment saying so, and saying that any third caller imports fromprocess-liveness.tsinstead.noFollow, so a pre-planted symlink at<pid>.jsoncannot redirect a registration write — the same reasonsession-writer-lease.tsopens withO_NOFOLLOW. Reads deliberately do not: the directory is 0700, so an attacker is already same-uid, and a redirected read discards anything unparseable. Only a redirected write is destructive.^\d+\.json$. A lenientparseIntprefix match would read2026-planning-notes.jsonas PID 2026, fail its liveness check, and delete a file this code never wrote.sanitizeTerminalText.nameandcwdare written by another process. ANSI can repaint the table and a control byte can misalign it, but the one that matters is a bidi override (Trojan Source, CVE-2021-42572): DIRECTORY is the column a user relies on to tell two live sessions apart, and it can be made to render a path that does not exist.sessions/list.tshas a private sanitizer with the same hole;ps.tsuses the shared helper rather than copying it a third time, and fixinglist.tsis a separate change to a shipped command./clearand/resumepatch the record's session id, and a directory switch patches its cwd, but neither patches the name — that name is the handle a user just read out ofps.The tests were built by mutation testing rather than by eyeballing coverage: 58 mutants introduced one at a time into the load-bearing lines, 39 of the first 48 survived, and the gaps they exposed are now closed. Six survivors remain and are documented in the test files as provably equivalent mutants. One result shaped the tests and is worth repeating: every loosening of the
^\d+\.json$guard was invisible throughlistLiveSessions's return value, because a later filename-versus-contents check subsumes it — so the guard is pinned at the I/O level with astat-recording mock and decoy filenames chosen to break each anchor individually.Evidence (Before & After)
Before: no way to enumerate running sessions;
qwen sessionsoffered onlylist, which walks saved transcripts. After: theqwen sessions psoutput shown above. The capture is from an earlier revision of this same change, taken during live two-session testing; the command's output format is unchanged since.Tested on
Linux: full unit suites above, plus lint, prettier and
tsc --noEmitclean on every changed file, plus a manual two-session run. macOS and Windows are not verified locally and rely on CI. Note that on non-Linux platforms the start identity is unavailable by design, so PID-reuse protection degrades to a plain liveness check — this is documented in the module and is why the POSIX permission and/procassertions are wrapped init.runIf(process.platform !== 'win32'), so the Windows job reports them as skips rather than passing green while asserting nothing.Environment (optional)
Local:
npm run build && node scripts/start.js, no sandbox. Unit tests vianpx vitest runper package.Risk & Scope
psper record during enumeration.isPidAlivein the repo (this change removes one, inteamHelpers.ts, andprocess-liveness.tsis now the obvious home for the rest). Fixing the bidi hole insessions/list.ts's private sanitizer. A record left behind bySIGKILLis not cleaned at exit; the liveness sweep during enumeration is the designed backstop.Linked Issues
Part of #8724. Replaces #8728, which I closed rather than continue patching: it went through seven rounds of automated review,
session-registry.tsgrew from 379 lines to 1235, and by round 3 most remaining Critical findings were in code the earlier rounds had added — a PID-namespace gate and a machine-id check that never had a stated threat model. This version is 339 lines. It keeps the two findings from that round that were plainly right (symlink-safe writes, Windows guards on POSIX-only assertions) and leaves the namespace machinery out.中文说明
这个 PR 做了什么
每个交互式会话在运行期间会把自己登记下来,退出时删除该登记,因此"这台机器上现在有哪些 Qwen Code 会话在跑"只需读取一个很小的目录,而不必遍历每个项目的历史会话记录。新增
qwen sessions ps子命令用于打印这些会话,--json输出 JSON Lines 以便脚本使用。只有当进程仍在运行并且记录的启动身份仍然吻合时,一条记录才算存活,因此无论是 PID 被复用还是机器重启,都无法让一个已经死亡的会话复活。凡是无法证明已死的记录一律保留:让一个正在运行的会话变得不可见是这里最坏的结果,所以每一个模棱两可的情形都倾向于保留。
为什么需要它
这是 #8724 中跨会话消息功能所需的发现层,单独提交是因为它本身就有用,并且不改变现有会话的任何行为。
最容易想到的替代方案是已有的
runtime.jsonsidecar,但它无法承担这个职责。它以 session id 为键、位于每个项目各自的目录下,并且被刻意设计为永不删除——正常退出不删,崩溃也不删——因此它的存在完全不携带任何存活信号。用它来回答这个问题的代价在代码库中已经有先例:worktreeSessionService.ts里的isSessionRuntimeActive有大约 150 行的候选目录猜测外加一次递归的文件系统扫描,而它只能回答"某一个已知会话是否存活"。用它来枚举全部存活会话的代价是上述成本乘以 N,且其增长取决于历史会话的数量而非存活会话的数量。本次改动之后两者共存:runtime.json仍是面向外部观察者的、与 kimi 兼容的"PID X 正在服务哪个会话"的 sidecar,而本 PR 是 CLI 自身功能所用的发现索引。审阅者测试计划
如何验证
运行两个新增测试套件以及本次改动波及的套件:
预期:core 617 个通过,cli 119 个通过。然后手工确认功能:在一个终端里启动会话,在另一个终端运行
qwen sessions ps,检查该行出现且目录正确;退出会话后检查该行消失、~/.qwen/sessions/重新为空。以下几点值得审阅者重点关注,每一条都对应一个具体的失败场景,而不是出于防御习惯:
<boot_id>:<starttime>,而不是裸 PID。starttime统计的是自开机以来的时钟 tick,因此它只能在一次开机周期内标识一个进程。记录会在崩溃或断电后残留,重启之后一个无关进程可能同时复用相同的 PID 和 tick 数——于是产生一个幽灵会话,而复用防护本身也认同它是存活的,因此它永远无法被清扫。当 boot id 读取不到时,函数返回null而不是裸 tick 数,因为在同一台机器上同时存在两种 token 形状,会让一个持有 boot id 的读者去"错配"某个在没有 boot id 时登记的存活会话,并删除它的记录。session-writer-lease.ts构造了相同的 Linux 身份,此处刻意不动它。 它的 token 是一种带接管语义的、持久化到磁盘的格式,统一两者属于修改那个文件的契约,而不是重构。代码里有注释说明这一点,并说明任何第三个调用方应当从process-liveness.ts导入。noFollow写入,因此预先在<pid>.json位置放置的符号链接无法重定向一次注册写入——这与session-writer-lease.ts使用O_NOFOLLOW的理由相同。读取刻意不加:目录权限为 0700,攻击者本就是同一个 uid,而被重定向的读取会丢弃任何无法解析的内容。只有被重定向的写入才具有破坏性。^\d+\.json$。 宽松的parseInt前缀匹配会把2026-planning-notes.json读成 PID 2026,使其存活检查失败,进而删除一个本代码从未写过的文件。sanitizeTerminalText。name与cwd由其他进程写入。ANSI 序列可以重绘表格、裸控制字节可以打乱对齐,但真正关键的是 bidi 覆写字符(Trojan Source,CVE-2021-42572):DIRECTORY 正是用户用来区分两个存活会话的那一列,而它可以被渲染成一个并不存在的路径。sessions/list.ts里的私有 sanitizer 存在同样的漏洞;ps.ts选择使用共享 helper 而不是把它复制第三份,修复list.ts属于对一个已发布命令的独立改动。/clear与/resume会更新记录中的 session id,切换工作目录会更新 cwd,但两者都不会更新 name——那个 name 正是用户刚从ps中抄下来的句柄。这些测试是通过变异测试构建的,而不是靠肉眼估算覆盖率:向承重代码行逐条注入了 58 个变异体,最初 48 个中有 39 个存活,它们暴露的缺口现已补齐。剩余 6 个存活者已在测试文件中记录为可证明的等价变异体。有一个结果直接影响了测试写法,值得复述:对
^\d+\.json$守卫的任何放松,从listLiveSessions的返回值上都观察不到,因为后续的"文件名与内容比对"检查把它盖住了——因此该守卫改为在 I/O 层钉死,使用记录stat路径的 mock,并配以专门破坏每一个锚点的诱饵文件名。证据(前后对比)
之前:没有任何办法枚举正在运行的会话;
qwen sessions只提供list,而它遍历的是已保存的历史会话。之后:即上文展示的qwen sessions ps输出。该截取来自本改动的一个更早修订版,是在双会话实机测试期间获得的;此后该命令的输出格式没有变化。测试平台
Linux:上述完整单元测试套件,外加 lint、prettier 与
tsc --noEmit在所有改动文件上均干净,另有一次手工双会话实机运行。macOS 与 Windows 未在本地验证,依赖 CI。注意在非 Linux 平台上启动身份按设计不可用,因此 PID 复用防护退化为纯存活检查——这一点已在模块中说明,也正是 POSIX 权限断言与/proc断言被包在it.runIf(process.platform !== 'win32')中的原因,这样 Windows 任务会将它们报告为跳过,而不是"绿着通过但其实什么都没断言"。运行环境(可选)
本地:
npm run build && node scripts/start.js,未使用沙箱。单元测试按包执行npx vitest run。风险与范围
ps进程。isPidAlive副本的去重(本改动删除了teamHelpers.ts中的一处,process-liveness.ts现在是其余各处的明确归宿)。修复sessions/list.ts私有 sanitizer 中的 bidi 漏洞。被SIGKILL杀死的会话留下的记录不会在退出时清理;枚举期间的存活扫描是为此设计的兜底。关联 Issue
属于 #8724。取代 #8728——我选择关闭它而不是继续打补丁:它经历了七轮自动审阅,
session-registry.ts从 379 行涨到 1235 行,而到第 3 轮时,剩余的多数 Critical 问题都出在前几轮新加的代码里——一套从未给出威胁模型的 PID namespace 闸门与 machine-id 校验。本版本为 339 行,保留了那一轮中确实正确的两条结论(防符号链接的写入、对仅适用于 POSIX 的断言加 Windows 守卫),并舍弃了 namespace 那套机制。