feat(core): add a live-session registry and qwen sessions ps - #8728
feat(core): add a live-session registry and qwen sessions ps#8728qqqys wants to merge 21 commits into
qwen sessions ps#8728Conversation
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 that 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`, so enumeration costs a read per *historical* session and grows with transcript history; and it is deliberately never deleted, so its presence carries no liveness signal. The two now coexist — runtime.json stays the stable, kimi-compatible "which session is PID X serving" sidecar for external observers. Staleness is decided by PID liveness plus a start-time token read from /proc, so a recycled PID cannot resurrect a dead session's record. The new `process-liveness` helpers replace the private copy in teamHelpers. Registry hygiene worth calling out: 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 only `<digits>.json` is ever considered a record — a lenient prefix match would read `2026-planning-notes.json` as PID 2026 and delete a file this code never wrote. `qwen sessions ps` prints the live sessions; `--json` emits JSON Lines. It sits next to `qwen sessions list`, which walks saved transcripts and answers the other question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
startInteractiveUI now calls registerSession({ sessionId:
config.getSessionId(), cwd: config.getTargetDir(), ... }), but the mock
Config objects in gemini.test.tsx predate it and expose neither getter,
so all 32 tests that reach startInteractiveUI died with
"config.getTargetDir is not a function".
Add the two getters to the mocks that feed those tests, and stub
registerSession/unregisterSession so the suite does not write a real
record into the global Qwen dir on every run — the registry has its own
coverage in packages/core/src/services/session-registry.test.ts.
Tests: packages/cli src/gemini.test.tsx 69 passed (was 32 failed | 37
passed); npm run build, npm run typecheck, eslint and prettier on the
changed file all clean.
This branch forked from main at 8fd0162 and had fallen 76 commits behind, which broke the required "Test (ubuntu-latest, Node 22.x)" job: main's CI now runs `npm run check:voice-guard-sync`, but the script only exists in package.json from a later commit, so the step died with `Missing script: "check:voice-guard-sync"` before vitest ever started. Merging QwenLM#8728's head fixes that and one more thing the aborted job was hiding. QwenLM#8730 is stacked on QwenLM#8728 and carried b92fde1 — the session registry commit — without a9e9cee, the follow-up that repairs the gemini.test.tsx Config mocks for it. Those 32 tests would have failed as soon as the voice-guard gate stopped short-circuiting the job. Taking 8728's head brings both the up-to-date package.json and that fix. No conflicts, and no source change of my own — this is the merge only. Verified: npm run build, npm run typecheck, check:voice-guard-sync, check:lockfile, check:desktop-isolation and audit:runtime:critical all exit 0; packages/cli src/gemini.test.tsx 69 passed; packages/core src/ipc 99 passed; packages/cli peer-messaging + peers-command 32 passed; packages/core session-registry 23 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; it is the suite that would exercise the new qwen sessions ps command and startup registration. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths of this diff unexercised by CI. Not reviewed: reverse audit — stopped before round 2 by the review time budget. Test Plan (not a blocker): src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 8 more.
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if ( | ||
| await registerSession({ | ||
| sessionId: config.getSessionId(), |
There was a problem hiding this comment.
[Suggestion] This register-on-start / unregister-on-exit wiring — the only production call sites of registerSession/unregisterSession and the mechanism that makes qwen sessions ps show anything — is stubbed everywhere (gemini.test.tsx mocks both) and asserted nowhere. The test-efficacy probe (harness validated) confirmed it: reverting these lines, or the whole block, leaves every test green.
Failure scenario: a follow-up refactor that drops the call — or registers the cleanup unconditionally when registration returned false — ships with green CI, and qwen sessions ps silently lists no interactive sessions.
Suggested fix: in the startInteractiveUI describe block, assert registerSession was called with { sessionId, cwd, kind: 'interactive', qwenVersion } and registerCleanup once; add one case where registration resolves false and registerCleanup is not called.
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Fixed in dc42240 — new startInteractiveUI > session registry announcement block in packages/cli/src/gemini.test.tsx:
announces the session and unregisters it on exit— assertsregisterSessionis called once with{ sessionId, cwd, kind: 'interactive', qwenVersion }, then runs every callback handed toregisterCleanupand asserts exactly one of them calledunregisterSession.skips the unregister cleanup when registration failed—registerSessionresolvesfalse; running all registered cleanups must leaveunregisterSessionuntouched.
Mutants both caught, nothing else moved:
| mutant | result |
|---|---|
delete the whole if (await registerSession(...)) { ... } block |
both new tests fail |
await registerSession(...) + unconditional registerCleanup(() => unregisterSession()) |
skips the unregister cleanup when registration failed fails, the other passes |
Both restored and re-run green afterwards.
Verification: npm run build OK, npm run typecheck OK, eslint + prettier --check clean; packages/cli src/gemini.test.tsx + src/commands/sessions/ = 110 passed (gemini.test.tsx 69 -> 71 tests).
| return; | ||
| } | ||
| expect(token).toMatch(/^\d+$/); |
There was a problem hiding this comment.
[Suggestion] This assertion survives an off-by-one mutation of the /proc/<pid>/stat field index. Ran the mutant: fields[19] → fields[18] reads field 21 (itrealvalue, hardcoded 0 on modern kernels — observed 0 vs 84220758 on this host), so every process records token '0'… and all 33 tests across the two suites stay green ('0' matches /^\d+$/, is stable, equals itself; the recycled-PID tests use recorded tokens that still mismatch '0'). The shipping code is correct — the tests just cannot defend it.
Failure scenario: a future one-character edit to the field index silently voids the PID-recycle guard — isSameProcess accepts any live PID regardless of recycle, and dead sessions resurrect when their PID is reused.
Suggested fix: add an assertion a constant field cannot satisfy — spawn two short-lived child processes separated by more than one clock tick and assert the second's token is strictly greater than the first's.
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Fixed in dc42240 — new readProcStartToken > grows with start order, so a later process reads a larger token in packages/core/src/utils/process-liveness.test.ts (Linux-guarded).
It spawns two idling children 80ms apart — starttime is reported in clock ticks at a fixed USER_HZ of 100, so 80ms is ~8 ticks — and asserts Number(secondToken) > Number(firstToken). A constant field cannot satisfy strict increase, which is exactly what the neighbouring indices are.
Mutants, run one at a time and each restored + re-run green afterwards:
| mutant | field read | result |
|---|---|---|
fields[19] -> fields[18] |
itrealvalue (0 on modern kernels) |
only the new test fails — 1 failed / 35 passed across process-liveness.test.ts + session-registry.test.ts |
fields[19] -> fields[20] |
vsize (equal for two copies of the same binary) |
only the new test fails — 1 failed / 11 passed |
Verification: npm run build OK, npm run typecheck OK, eslint + prettier --check clean; packages/core process-liveness.test.ts + session-registry.test.ts + runtimeStatus.config.test.ts = 44 passed.
Review 4888791383 on QwenLM#8728 found the /clear-and-/resume swap wired so that the machine-wide registry patch could be skipped in two ways that nothing detected. Both halves of the swap ran in one queued closure whose rejection is swallowed, so a sidecar write that failed mid-swap (unwritable chats/, full disk) took patchSessionRecord down with it and the record kept advertising the pre-swap session id. The two sidecar awaits now sit in their own try/catch, so the registry patch always runs. The patch was also gated on runtimeStatusEnabled, which registerSession never consults — a startup where the sidecar write failed but registration succeeded left the record stranded on the old session id for the life of the process. Unlike clearRuntimeStatus, the patch is keyed by this PID and no-ops without a record, so it cannot trample a sibling and needs no ownership gate; the gate now covers only the sidecar half it was written for. Tests for the gaps the review probed, each flip-verified against the mutant that motivated it: - runtimeStatus.config.test.ts: the registry half of the swap had no coverage at all — deleting the call kept the suite green. Four cases now pin it, including the two failure modes fixed above. - session-registry.test.ts: enumerate as a foreign PID so the record goes through isSameProcess rather than the self-pid shortcut, and assert the written procStart. `procStart: null` previously stayed green while silently voiding the recycled-PID guard. - process-liveness.test.ts: cover the degrade branch where the current token cannot be read. `return true` -> `return false` there sweeps live records on every platform without /proc, and passed 33/33. - ps.test.ts: assert the exact truncated cell for a full-width name; `contains '...'` survived a loop guard replaced with `if (true)`. Also cover the registry-read failure branch (stderr text + exit 1). - ps.test.ts carried raw ESC bytes, so the ANSI assertion rendered as `not.toContain('')` and a byte-stripping tool would have voided the test silently. Now `\x1b`, matching repo convention. Also drops the "Tests pass an explicit value" claim from the `pid` option's doc comment: no caller in production or tests passes one. Verified: npm run build, npm run typecheck, eslint and prettier on the touched files, plus core config/runtimeStatus/session-registry/ process-liveness (562 passed) and cli commands/sessions (39 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — worked through review #4888791383. Pushed Behaviour fixes
Coverage for the gaps you probed — each one flip-verified against the mutant that motivated it (mutant in → the new test fails; mutant out → green):
Also
Still open, queued for a follow-up commit — both need more than a local assertion and I did not want to rush them into this one:
Verification on |
Closes the two review comments left open on QwenLM#8728 after 0f0af41. - gemini.test.tsx: `startInteractiveUI`'s register-on-start / unregister-on-exit block was the only production call site of `registerSession`/`unregisterSession` and was asserted nowhere, so deleting it — or registering the cleanup even when registration returned false — kept every test green. Two cases now pin the announced fields and that exactly one registered cleanup unlinks the record, and none does when registration failed. - process-liveness.test.ts: every existing assertion survived an off-by-one on the `/proc/<pid>/stat` field index, which silently voids the PID-recycle guard. Two children started 80ms apart must now read strictly increasing tokens — a constant neighbour field cannot. Verified: both mutants (unconditional `registerCleanup`, and the whole block deleted) fail exactly the new CLI tests; `fields[19]` -> `[18]` and -> `[20]` each fail exactly the new core test. npm run build, npm run typecheck, eslint and prettier --check clean; packages/core process-liveness + session-registry + runtimeStatus.config = 44 passed; packages/cli gemini.test.tsx + commands/sessions = 110 passed.
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; it is the suite that would exercise the new qwen sessions ps command and startup registration.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths and the Windows mode assertions of this diff unexercised.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; it is the suite that would exercise the new qwen sessions ps command and startup registration。
未审查:build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths and the Windows mode assertions of this diff unexercised。
未审查:反向审计——评审时间预算不足,未能开始第 5 轮。
Test Plan(非阻断):src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| function sanitize(value: string): string { | ||
| const stripped = value.replace(/[\r\n\t]/g, ''); | ||
| const escaped = escapeAnsiCtrlCodes(stripped); |
There was a problem hiding this comment.
[Suggestion] sanitize — and padDisplay/truncate below it — are near-verbatim copies of the private helpers in sibling sessions/list.ts (its JSDoc even says "Mirrors sessions list"). Terminal-sanitization logic for attacker-influenced registry fields now exists in two places. textUtils.ts already exports sanitizeTerminalText documented as the single source of truth, though its TAB/LF policy differs, so a shared sessions-table helper is the cleaner extraction point. — Concrete cost: when a sanitization gap is later found (a missed control class, a bidi strip, a new ANSI vector), the fix must be applied twice in lockstep; patching list.ts only leaves qwen sessions ps rendering un-sanitized records read from another process's on-disk file. The two truncate accumulation loops are already written differently. Suggested fix: extract the three helpers into one shared module (e.g. sessions/common.ts, which already exists) and import it from both commands.
中文说明
sanitize——及其下方的 padDisplay/truncate——是兄弟文件 sessions/list.ts 中私有辅助函数的近乎逐字拷贝(其 JSDoc 甚至写着 "Mirrors sessions list")。针对可被攻击者影响的注册表字段的终端净化逻辑现在存在于两处。textUtils.ts 已导出 sanitizeTerminalText 并被文档称为唯一权威来源,但其 TAB/LF 策略不同,因此提取一个共享的 sessions 表格辅助模块是更干净的做法。具体代价:日后发现净化缺口(遗漏的控制字符类、bidi 剥离或新的 ANSI 向量)时,修复必须两处同步进行;只改 list.ts 会让 qwen sessions ps 继续渲染来自其他进程磁盘记录、未经净化的内容。两个 truncate 累积循环的写法已经不同。建议修复:把三个辅助函数提取到一个共享模块(如已存在的 sessions/common.ts),两个命令都从那里导入。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const base = path | ||
| .basename(cwd) | ||
| .replace(/[^\w.-]+/g, '-') | ||
| .slice(0, 32); |
There was a problem hiding this comment.
[Suggestion] deriveSessionName strips every non-ASCII character (JS \w is ASCII-only), so sessions launched from CJK/accented directory names get NAME labels like --fa. Verified by execution: '/home/u/我的项目' → --fa; 'café-api' → caf--api-e8. — Failure scenario: registerSession is called from startInteractiveUI without an explicit name, so every interactive session in a fully non-ASCII directory advertises an uninformative NAME in qwen sessions ps; two sessions in different non-ASCII directories are distinguishable only by the 2-hex suffix in that column. The display layer already anticipates CJK names (ps.test.ts exercises width-aware truncation with '中'.repeat(15)) yet the producer can never emit one.
| const base = path | |
| .basename(cwd) | |
| .replace(/[^\w.-]+/g, '-') | |
| .slice(0, 32); | |
| const base = Array.from( | |
| path.basename(cwd).replace(/[^\p{L}\p{N}._-]+/gu, '-'), | |
| ) | |
| .slice(0, 32) | |
| .join(''); |
中文说明
deriveSessionName 会剥掉所有非 ASCII 字符(JS 的 \w 仅为 ASCII),因此从 CJK/带重音目录名启动的会话得到的 NAME 标签形如 --fa。已执行验证:'/home/u/我的项目' → --fa;'café-api' → caf--api-e8。失败场景:registerSession 由 startInteractiveUI 调用且不传显式 name,所以任何位于纯非 ASCII 目录下的交互式会话在 qwen sessions ps 中都只能显示无信息量的 NAME;两个位于不同非 ASCII 目录的会话在该列仅靠两位十六进制后缀区分。展示层已经预期了 CJK 名字(ps.test.ts 用 '中'.repeat(15) 测试了宽度感知截断),生产者却永远发不出这样的名字。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| expect(JSON.parse(stdout[0]).pid).toBe(4242); | ||
| expect(JSON.parse(stdout[1]).pid).toBe(7); |
There was a problem hiding this comment.
[Suggestion] The --json test pins only .pid per emitted line, so the full-record contract of the JSONL output is untested — production ps.ts emits JSON.stringify(record), and the complete SessionRegistryRecord is the machine-readable contract for scripts. — Failure scenario: probe-verified — the mutation JSON.stringify({ pid: record.pid, sessionId: record.sessionId }) keeps all 11 tests green while a consumer piping qwen sessions ps --json and reading .cwd, .name, .startedAt or .procStart silently gets undefined; the suggested strengthening makes the mutant fail.
| expect(JSON.parse(stdout[0]).pid).toBe(4242); | |
| expect(JSON.parse(stdout[1]).pid).toBe(7); | |
| expect(JSON.parse(stdout[0])).toMatchObject({ pid: 4242, sessionId: 'sess-1', cwd: '/w/app', name: 'app-ab', kind: 'interactive', qwenVersion: '1.0.0' }); | |
| expect(JSON.parse(stdout[1]).pid).toBe(7); |
中文说明
--json 测试对每行输出只断言了 .pid,因此 JSONL 输出的完整记录契约没有被测试——生产代码 ps.ts 输出 JSON.stringify(record),完整的 SessionRegistryRecord 才是面向脚本的机器可读契约。失败场景:已用探针验证——变异 JSON.stringify({ pid: record.pid, sessionId: record.sessionId }) 下全部 11 个测试仍然通过,而管道消费 qwen sessions ps --json 并读取 .cwd、.name、.startedAt 或 .procStart 的脚本会悄悄得到 undefined;按建议加强断言后该变异会被捕获。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| // eslint-disable-next-line no-control-regex | ||
| return escaped.replace(/[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f]/g, ''); |
There was a problem hiding this comment.
[Suggestion] sanitize strips C0/C1 control bytes and ANSI sequences but not the Unicode bidi embedding/isolate characters (U+202A–202E, U+2066–2069), which all three canonical untrusted-string sanitizers in textUtils.ts strip explicitly for visual-spoofing prevention (the CVE-2021-42572 class). — Failure scenario: probe-verified — a record carrying U+202E in name renders the bidi control to the terminal through qwen sessions ps (any same-user process can hand-write <own-pid>.json — its own live PID passes the liveness check), visually reordering the rendered cell so a hostile directory can display as a different benign-looking path in a command whose purpose is to tell the user where sessions run. Extending the regex with the bidi ranges makes the probe pass with all existing tests still green.
| // eslint-disable-next-line no-control-regex | |
| return escaped.replace(/[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f]/g, ''); | |
| // eslint-disable-next-line no-control-regex | |
| return escaped.replace( | |
| /[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f\u202a-\u202e\u2066-\u2069]/g, | |
| '', | |
| ); |
中文说明
sanitize 剥掉了 C0/C1 控制字节和 ANSI 序列,但没有剥掉 Unicode bidi 嵌入/隔离字符(U+202A–202E、U+2066–2069),而 textUtils.ts 中三个权威的非受信字符串净化器都明确为防视觉欺骗(CVE-2021-42572 一类)剥离了这些字符。失败场景:已用探针验证——name 中带有 U+202E 的记录会经由 qwen sessions ps 把 bidi 控制字符渲染到终端(任何同用户进程都可以手写 <自己的pid>.json——它自己的存活 PID 能通过存活检查),从而视觉上重排行内容,使恶意目录在这个专门告诉用户「会话在哪里运行」的命令里显示为另一个看似无害的路径。把正则扩展上 bidi 区间后探针通过,且现有测试全部保持绿色。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| describe('Config.startNewSession session-registry swap', () => { | ||
| const sessionA = 'aaaaaaaa-1111-2222-3333-aaaaaaaaaaaa'; | ||
| const sessionB = 'bbbbbbbb-1111-2222-3333-bbbbbbbbbbbb'; |
There was a problem hiding this comment.
[Suggestion] This block never calls startNewSession twice on one Config, so serialization of consecutive registry patches through queueRuntimeStatusWrite — the reason that queue exists — is unpinned for the patch path this PR adds. — Failure scenario: a user runs /clear then /new in one session; both swaps queue patchSessionRecord read-modify-write cycles against the same <pid>.json. Today the promise chain serializes them (probe-verified: the suggested test passes, and dropping the chain link makes it fail), but if a future change makes the patch fire-and-forget, the two cycles interleave and the record can end advertising the previous session id — qwen sessions ps points at the wrong transcript with every test still green. Suggested fix:
it('patches in order when two swaps queue back-to-back', async () => {
const sessionC = 'cccccccc-1111-2222-3333-cccccccccccc';
const config = makeConfig(sessionA);
config.markRuntimeStatusEnabled();
config.startNewSession(sessionB);
config.startNewSession(sessionC);
await waitFor(async () => (patchCalls.length >= 2 ? patchCalls[1] : null));
expect(patchCalls.map((p) => p.sessionId)).toEqual([sessionB, sessionC]);
});中文说明
该测试块从不对同一个 Config 连续调用两次 startNewSession,因此通过 queueRuntimeStatusWrite 串行化连续注册表补丁——这个队列存在的理由——对本 PR 新增的补丁路径没有被测试钉住。失败场景:用户在一个会话里先 /clear 再 /new;两次切换都会向同一个 <pid>.json 排队一次 patchSessionRecord 的读-改-写。当前的 promise 链会将它们串行化(已用探针验证:建议的测试通过,拆掉链的一环则失败),但若未来改动使补丁变成发射即忘,两次读-改-写会交错,记录最终可能仍宣告上一个会话 id——qwen sessions ps 指向错误的会话记录,而所有测试仍然通过。建议修复见上方代码。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| export function isPidAlive(pid: number): boolean { | ||
| if (!Number.isInteger(pid) || pid <= 0) return false; | ||
| try { |
There was a problem hiding this comment.
[Suggestion] isPidAlive reports zombie (exited-but-unreaped) processes as alive, so a dead session's record is kept and listed as live until its parent reaps it — even though /proc/<pid>/stat, which this module already parses, carries positive proof of death in the state field (Z, at fields[0] after the comm anchor). — Failure scenario: probe-verified with a real zombie (isPidAlive(zombie) === true, token still matches): in environments without a prompt reaper (a container PID 1 lacking an init, a supervisor that never waits) an exited session lingers in qwen sessions ps indefinitely, and a user trying to connect hits a confusing failure. The module header keeps records only for processes "we cannot positively prove dead" — state Z is exactly that proof on Linux, so this falls outside the documented trade-off. Self-heals once reaped; no PID-recycling identity confusion since the zombie holds the PID. Suggested fix: surface the state field from the /proc read and treat current state Z as dead in the liveness path; keep the plain process.kill behavior on platforms without /proc.
中文说明
isPidAlive 会把僵尸进程(已退出但未被回收)报告为存活,于是已死会话的记录会被保留并显示为存活,直到其父进程回收它——尽管本模块已经在解析的 /proc/<pid>/stat 中就有死亡的正面证据(状态字段 Z,位于 comm 锚点之后的 fields[0])。失败场景:已用真实僵尸进程验证(isPidAlive(zombie) === true,令牌仍然匹配):在没有及时回收者的环境(缺少 init 的容器 PID 1、从不 wait 的监管进程)中,已退出的会话会无限期留在 qwen sessions ps 里,尝试连接的用户会遇到令人困惑的失败。模块头注释声明只保留「无法确证已死」的进程的记录——状态 Z 在 Linux 上恰恰就是确证,因此这种情况不在文档声明的权衡范围内。被回收后自愈;僵尸进程占住 PID,故不存在 PID 复用导致的身份混淆。建议修复:让 /proc 读取同时返回状态字段,并在存活判定路径中把当前状态 Z 视为已死;在没有 /proc 的平台保持原有的 process.kill 行为。
— qwen3.8-max via Qwen Code /review (v0.21.7)
Addresses the three Critical findings in review 4889504524 on QwenLM#8728. **The sweep deleted live sessions' records across a namespace boundary** (comment 3741404815). `listLiveSessions` read a PID that is invisible (ESRCH) or token-mismatched as proof of death, but `sandbox.ts` mounts the host's global qwen dir — `sessions/` included — into a container that gets its own PID namespace, so both sides read each other's records while neither can see the other's processes. Registration is startup-only and `patchSessionRecord` no-ops on a missing record, so a sweep from the wrong side hid a live session for the rest of its life. Each record now carries the namespace its PID was allocated in, read from `/proc/self/ns/pid` by the new `readPidNamespaceId()`, and enumeration ignores — never lists, never sweeps — any record it cannot attribute to its own namespace. Two nulls is the no-namespaces case (every non-Linux platform) and stays on the original path. The field is additive under schema version 1, so an older reader still parses these records. **Three POSIX mode assertions had no win32 guard** (comment 3741404812): Windows synthesizes `st_mode` from file attributes, so `0o700`/`0o600` cannot hold and `test_windows` was deterministically red. Same `it.skipIf` as `atomicFileWrite.test.ts` and `session-writer-lease.test.ts`. **`gemini.test.tsx`'s registerSession stub was order-dependent** (comment 3741404813): the earlier describes' `vi.restoreAllMocks()` wiped its implementation, so every `startInteractiveUI` test silently took the registration-failed branch. Re-armed in the describe's `beforeEach`, and the stale `registerCleanup` count updated to the 2 that production registers; that test now reads the last cleanup, since the first is the registry's. Verification: npm run build, npm run typecheck, eslint and prettier --check on all six files — all clean. core process-liveness + session-registry + runtimeStatus.config = 50 passed; cli ps.test.ts = 11 passed; cli gemini.test.tsx = 71 passed, and the previously order- dependent case now passes on its own too (it failed 1-vs-2 before). Two-way probes: dropping the namespace guard turns exactly the two new tests red (a live foreign record gets listed, an invisible one gets unlinked); pointing `readPidNamespaceId` at `/proc/self/ns/mnt` turns both of its tests red; flipping the skipIf predicate to 'linux' skips exactly those three mode cases and no others.
|
Worked through review #4889504524 — all three Criticals are fixed in [3741404815] The sweep deleted live sessions' records across a PID-namespace boundary. This was the real bug. Each record now carries the namespace its PID was allocated in (new I took the record-side fix rather than excluding [3741404812] Three POSIX mode assertions had no [3741404813] The Verification. Two-way probes, so the new guards are not decorative:
The Suggestions from this review are not addressed here; I kept the commit to the Criticals. |
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; it is the suite that would exercise the new qwen sessions ps command and startup registration.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths and the Windows mode assertions of this diff unexercised.
Not explored to full depth (tool budget reached): chunk 1: run npx vitest run src/commands/sessions/ps.test.ts src/commands/sessions.test.ts — npm ci failed in this sandbox (no node_modules present, install aborted ….
Test Plan (not a blocker): src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; it is the suite that would exercise the new qwen sessions ps command and startup registration。
未审查:build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths and the Windows mode assertions of this diff unexercised。
未探索到全部深度(达到工具调用预算):chunk 1:run npx vitest run src/commands/sessions/ps.test.ts src/commands/sessions.test.ts — npm ci failed in this sandbox (no node_modules present, install aborted …。
Test Plan(非阻断):src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| * containing an ANSI sequence or a stray `\r` could otherwise repaint or | ||
| * misalign this table. Mirrors `sessions list`. | ||
| */ | ||
| function sanitize(value: string): string { |
There was a problem hiding this comment.
[Suggestion] R2-4 (carried forward from round 2 — still standing at this commit; the original thread remains open): sanitize — and padDisplay/truncate below it — are near-verbatim copies of the private helpers in sibling sessions/list.ts; terminal-sanitization logic for attacker-influenced input now lives in two private copies that can drift — and already have: the canonical textUtils policy gained a bidi strip neither copy carries. — Concrete cost: any future fix to control-char policy or string-width edge cases must be applied twice, in files pinned by two separate test suites; missing one copy leaves sessions list and sessions ps sanitizing the same attacker-influenced data differently. Extract the cell sanitizer (plus padDisplay/truncate) into a shared helper (e.g. sessions/common.ts).
中文说明
R2-4(自第二轮携带——在当前提交仍然存在;原线程仍然开放):sanitize——及其下方的 padDisplay/truncate——是姊妹文件 sessions/list.ts 私有helper的近乎逐字拷贝;针对攻击者可影响输入的终端净化逻辑现在存在于两份私有拷贝中,且已经发生漂移:textUtils 的规范策略后来加入了两个拷贝都没有的 bidi 剥离。具体代价:未来对控制字符策略或字符串宽度边界的任何修复都必须在两个文件各改一次,且由两套测试分别把关;漏改一份就会让 sessions list 与 sessions ps 对同样的攻击者可影响数据做不同的净化。建议把单元格净化器(连同 padDisplay/truncate)提取为共享helper(如 sessions/common.ts)。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| export function deriveSessionName(cwd: string, sessionId: string): string { | ||
| const base = path | ||
| .basename(cwd) | ||
| .replace(/[^\w.-]+/g, '-') |
There was a problem hiding this comment.
[Suggestion] R2-5 (carried forward from round 2 — still standing; re-verified by execution this round): deriveSessionName strips every non-ASCII character (JS \w is ASCII-only), so sessions launched from CJK/accented directory names degenerate to a leading-double-dash label: deriveSessionName('/w/项目', 's1') → base '-' (truthy, so the || 'session' fallback never fires) → --<suffix>. — Concrete cost: any session running in a directory whose basename is entirely CJK/Cyrillic/accented — routine for this project's user base — shows up in qwen sessions ps as --xx instead of a short, stable, human-readable label; a leading-dash token is also shell-option-shaped. Strip leading/trailing dashes after the character replacement so the fallback engages (e.g. .replace(/^-+|-+$/g, '')), and add a non-ASCII basename test.
中文说明
R2-5(自第二轮携带——仍然存在;本轮已重新执行验证):deriveSessionName 剥掉所有非 ASCII 字符(JS 的 \w 仅为 ASCII),因此从 CJK/带音符目录名启动的会话退化为双连字符开头的标签:deriveSessionName('/w/项目', 's1') → base '-'(为真值,|| 'session' 回退永不触发)→ --<后缀>。具体代价:任何运行在 basename 全为 CJK/西里尔/带音符字符目录中的会话——对本项目用户群是常态——在 qwen sessions ps 中显示为 --xx,而不是简短、稳定、人类可读的标签;连字符开头的词元还是 shell 选项形状。建议在字符替换后剥离首尾连字符使回退生效(如 .replace(/^-+|-+$/g, '')),并补一个非 ASCII basename 测试。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| await run({ json: true, all: false }); | ||
|
|
||
| expect(stdout).toHaveLength(2); | ||
| expect(JSON.parse(stdout[0]).pid).toBe(4242); |
There was a problem hiding this comment.
[Suggestion] R2-6 (carried forward from round 2 — still standing at this commit): the --json test pins only .pid per emitted line, so the full-record contract of the JSONL output is untested — production ps.ts emits JSON.stringify(record), and the complete SessionRegistryRecord is the machine-facing contract. — Concrete cost: a field dropped from or renamed in the emitted record (or the serialization format itself changing) leaves the test green while every scripting consumer of qwen sessions ps --json breaks. Assert the full record shape (or toMatchObject on the documented fields).
中文说明
R2-6(自第二轮携带——在当前提交仍然存在):--json 测试对每行输出只钉住 .pid,因此 JSONL 输出的完整记录契约未被测试——生产代码 ps.ts 输出 JSON.stringify(record),完整的 SessionRegistryRecord 才是面向机器的契约。具体代价:输出记录中被删掉或改名的字段(或序列化格式本身变化)不会让测试变红,而 qwen sessions ps --json 的所有脚本消费者都会坏掉。建议断言完整记录形状(或对文档化字段做 toMatchObject)。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const waitForPatch = () => | ||
| waitFor(async () => (patchCalls.length > 0 ? patchCalls[0] : null)); |
There was a problem hiding this comment.
[Suggestion] R2-8 (carried forward from round 2 — still standing at this commit): this block never calls startNewSession twice on one Config, so serialization of consecutive registry patches through queueRuntimeStatusWrite — the reason that queue exists — is unpinned for the patch path this PR adds. — Concrete cost: a regression that lets a second swap's patch interleave with or overtake the first (e.g. capturing newSessionId at the wrong time or breaking the chain) leaves all four new tests green; discovery would then point at the wrong transcript mid-double-swap. Add a two-consecutive-swaps test asserting both patches land in order.
中文说明
R2-8(自第二轮携带——在当前提交仍然存在):这个块从不在同一个 Config 上调用两次 startNewSession,因此连续注册表 patch 经由 queueRuntimeStatusWrite 的串行化——该队列存在的理由——对本 PR 新增的 patch 路径没有被钉住。具体代价:让第二次切换的 patch 与第一次交错或超车的回归(例如在错误时机捕获 newSessionId 或破坏链式串行)不会让四个新测试变红;双重切换中途发现机制会指向错误的转录。建议补一个连续两次切换的测试,断言两个 patch 按序落地。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| try { | ||
| process.kill(pid, 0); | ||
| return true; |
There was a problem hiding this comment.
[Suggestion] R2-9 (carried forward from round 2 — still standing; re-verified empirically this round): isPidAlive reports zombie (exited-but-unreaped) processes as alive, so a dead session's record is kept and listed as live until its parent reaps it — even though /proc/<pid>/stat, which this module already parses, carries positive proof of death (state Z). Verified on this runner: kill(pid, 0) against a zombie succeeds and its stat still shows state Z with the original starttime intact, so isSameProcess returns true. — Concrete cost: a session SIGKILLed/OOM-killed — the exact case the sweep exists for — whose parent never reaps it (a container PID-1 entrypoint without wait(), the deployment the module doc targets) stays a zombie indefinitely: qwen sessions ps shows a session that no longer exists, pointing discovery at a stale transcript, and the held PID blocks the recycled-PID sweep path from ever firing. Inspect the state field in the same stat read the token already performs and treat Z as dead.
中文说明
R2-9(自第二轮携带——仍然存在;本轮已重新实测验证):isPidAlive 把僵尸(已退出但未被回收)进程报告为存活,因此已死会话的记录会被保留并被列为存活,直到其父进程回收它——尽管本模块已经在解析的 /proc/<pid>/stat 携带死亡的正面证据(状态 Z)。已在本机验证:对僵尸进程 kill(pid, 0) 成功,其 stat 仍显示状态 Z 且原始 starttime 完好,因此 isSameProcess 返回 true。具体代价:被 SIGKILL/OOM 杀死的会话——清理机制正是为之存在——若父进程从不回收(不带 wait() 的容器 PID-1 入口,正是模块文档针对的部署),僵尸会无限期存在:qwen sessions ps 显示一个已不存在的会话,把发现机制指向陈旧转录,且被占用的 PID 让"复用 PID 清理"路径永远无法触发。建议在令牌读取所用的同一次 stat 读取中检查状态字段,把 Z 视为死亡。
— qwen3.8-max via Qwen Code /review (v0.21.8)
Round-3 review found five ways `<pid>.json` is treated as proof of ownership when it is only proof of a number. R3-1: the self-PID shortcut ran before the namespace gate, so a record written in another PID namespace whose PID happens to equal ours was adopted as this session — the gate added last round never ran for the one collision that matters. The gate now runs first. R3-10: that gate was also machine-blind. The initial PID namespace inode is the same constant on every non-containerized Linux host, so two machines sharing one registry directory (NFS home, `QWEN_HOME` on a shared volume) pass it, and each sweeps the other's records on a purely local ESRCH — no PID collision required. Records now carry a machineId and the gate is (machine, namespace). `/etc/machine-id` rather than the suggested `boot_id`: a boot id would make every pre-reboot record permanently unattributable, hence unsweepable and — with the write guard below — able to block registration at that PID forever. Reboot-recycled PIDs are already `procStart`'s job, and its token is boot-relative. R3-2: only the read/sweep path was guarded. `registerSession` overwrote, `patchSessionRecord` merged into, and `unregisterSession` unlinked whatever record sat at the path. All three now check the origin first; register refuses rather than clobbering, which leaves the loser of a bare-PID collision absent from discovery instead of destroying the winner's record. R3-14: both write sites called `atomicWriteJSON` without `noFollow`, so a symlink planted at `<pid>.json` — in a directory the sandbox shares across a trust boundary — redirected the write, and its forced 0600, to a file outside the mounts. R3-7: `relocateWorkingDirectory` refreshed the runtime.json sidecar on `/cd` but never the registry, leaving `cwd` and the derived name advertising the directory the session had left. Patched on the same write queue, and deliberately not gated on runtimeStatusEnabled for the reason startNewSession already documents.
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; it is the suite that would exercise the new qwen sessions ps command and startup registration.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths and the Windows-guarded permission assertions of this diff unexercised.
Test Plan (not a blocker): src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; it is the suite that would exercise the new qwen sessions ps command and startup registration。
未审查:build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths and the Windows-guarded permission assertions of this diff unexercised。
Test Plan(非阻断):src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| .command(listCommand) | ||
| .command(psCommand) |
There was a problem hiding this comment.
[Suggestion] R4-2: issue #8724's work breakdown names this deliverable qwen ps ("PR 1 — session registry + qwen ps … the user-visible payoff"), but the PR ships it only as qwen sessions ps. Verified against the real CLI: the root parser registers $0 [query..], so qwen ps does not error as an unknown command — it is absorbed as a one-shot prompt and silently spends a model call on the prompt "ps" (qwen sessions ps prints the listing). — Failure scenario: a user or script following issue #8724 runs qwen ps and gets a model response to the prompt "ps" instead of the live-session listing; the issue's named payoff is unreachable under its own name, and worse than an error because it is silent.
中文说明
issue #8724 的工作拆分把这个交付物命名为 qwen ps("PR 1 — session registry + qwen ps ……用户可见的收益"),但 PR 只实现了 qwen sessions ps。已在真实 CLI 上验证:根解析器注册了 $0 [query..],所以 qwen ps 不会报未知命令错误——它被当作一次性 prompt 吸收,悄悄花掉一次模型调用(prompt 内容是 "ps");而 qwen sessions ps 才打印列表。失败场景:按 issue #8724 操作的用户或脚本运行 qwen ps,得到的是对 prompt "ps" 的模型回复而不是存活会话列表;issue 指定的入口以自己的名字不可达,而且比报错更糟——它是静默的。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| if ( | ||
| await registerSession({ | ||
| sessionId: config.getSessionId(), |
There was a problem hiding this comment.
[Suggestion] R4-3: issue #8724's design says "Each top-level session (not subagents, not teammates) writes a small JSON record at startup and unlinks it on exit", but registration's only production call site is startInteractiveUI (reached only for interactive sessions), so top-level headless sessions (qwen -p …) never register — although SessionRegistryRecord's kind union declares 'headless' and no production code ever writes it. The narrowing to interactive appears only in the PR description, unargued against the issue text. — Failure scenario: a long-running headless invocation is a live Qwen Code session, yet qwen sessions ps (self-described "List Qwen Code sessions running right now") never shows it; later stack PRs that build on enumeration inherit the blind spot.
中文说明
issue #8724 的设计写明"每个顶层会话(不含 subagent、不含 teammate)在启动时写一个小 JSON 记录、退出时删除",但注册唯一的生产调用点是 startInteractiveUI(只有交互式会话才会走到),因此顶层 headless 会话(qwen -p …)从不注册——尽管 SessionRegistryRecord 的 kind 联合类型声明了 'headless' 而没有任何生产代码写入它。收窄到交互式只出现在 PR 描述里,没有对照 issue 原文论证。失败场景:长时间运行的 headless 调用是一个存活会话,但 qwen sessions ps(自我描述为"列出正在运行的 Qwen Code 会话")永远不显示它;后续依赖枚举的 stack PR 会继承这个盲区。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| it('treats a recycled PID as stale', async () => { | ||
| // Our own PID is alive, but the recorded start token belongs to a | ||
| // different process — so the record describes a session that is gone. | ||
| if (process.platform !== 'linux') return; |
There was a problem hiding this comment.
[Suggestion] R4-4 (1 of 4): this Linux-only test guards with if (process.platform !== 'linux') return; as its first statement — on Windows/macOS CI gates it executes zero assertions yet reports as passed, so the platform gap is invisible in gate results. The same file already establishes the honest pattern four times (it.skipIf(process.platform === 'win32') with rationale comments). Pattern spans 4 locations across the PR's two new test files (3 in process-liveness.test.ts, posted separately). — Failure scenario: on the Windows/macOS gates a regression in the recycled-PID guard shows as "all tests passing"; the report gives no signal that the check never ran.
| it('treats a recycled PID as stale', async () => { | |
| // Our own PID is alive, but the recorded start token belongs to a | |
| // different process — so the record describes a session that is gone. | |
| if (process.platform !== 'linux') return; | |
| it.skipIf(process.platform !== 'linux')('treats a recycled PID as stale', async () => { | |
| // Our own PID is alive, but the recorded start token belongs to a | |
| // different process — so the record describes a session that is gone. | |
| await writeRaw(`${process.pid}.json`, { |
中文说明
R4-4(4 处之 1):这个仅 Linux 的测试以 if (process.platform !== 'linux') return; 作为第一句——在 Windows/macOS CI 门禁上它一个断言都不执行却报告为通过,平台缺口在门禁结果里不可见。同一文件已有 4 处诚实写法(带理由注释的 it.skipIf(process.platform === 'win32'))。该模式在本 PR 的两个新测试文件中共 4 处(process-liveness.test.ts 中 3 处,另行张贴)。失败场景:在 Windows/macOS 门禁上,recycled-PID 防护的回归会显示为"全部测试通过";报告不会给出该检查从未运行的任何信号。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| it('grows with start order, so a later process reads a larger token', async () => { | ||
| if (process.platform !== 'linux') return; |
There was a problem hiding this comment.
[Suggestion] R4-4 (2 of 4): same pattern — this Linux-only test (the only one pinning the /proc/<pid>/stat field index for starttime, per its own comment) early-returns with zero assertions on Windows/macOS instead of it.skipIf(process.platform !== 'linux'), so those gates report it green without running it. — Failure scenario: a field-index regression ships with green Windows/macOS gates and no skip signal in the report.
| it('grows with start order, so a later process reads a larger token', async () => { | |
| if (process.platform !== 'linux') return; | |
| it.skipIf(process.platform !== 'linux')( | |
| 'grows with start order, so a later process reads a larger token', | |
| async () => { |
中文说明
R4-4(4 处之 2):同一模式——这个仅 Linux 的测试(按其自身注释,唯一钉住 /proc/<pid>/stat 中 starttime 字段索引的测试)在 Windows/macOS 上以零断言提前 return,而不是 it.skipIf(process.platform !== 'linux'),于是那些门禁报告它为绿而实际从未运行。失败场景:字段索引回归随着 Windows/macOS 门禁全绿出货,报告里没有任何 skip 信号。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| it.skipIf(process.platform === 'win32')( | ||
| 'writes the record as 0600', |
There was a problem hiding this comment.
[Suggestion] R3-25: 'writes the record as 0600' only exercises new-file creation; the forceMode: true tightening of a PRE-EXISTING loose record file (the upgrade case the option exists for) is untested. (carried forward from round 3 — still standing at this commit; the original thread remains open) — Failure scenario: a regression dropping forceMode (or skipping the chmod-on-overwrite path) ships green; records written by an older looser build stay world-readable after re-registration.
中文说明
R3-25:'writes the record as 0600' 只覆盖新建文件;forceMode: true 对已存在的宽松记录文件的收紧(该选项为之存在的升级场景)未被测试。(第 3 轮遗留——在当前提交仍然存在;原线程仍开放。)失败场景:去掉 forceMode(或跳过覆盖时 chmod 路径)的回归会全绿出货;旧版本宽松写入的记录在重新注册后仍可被所有用户读取。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| async function handlePs(argv: PsArgs): Promise<void> { | ||
| let records: SessionRegistryRecord[]; | ||
| try { | ||
| records = await listLiveSessions({ includeSelf: argv.all ?? false }); |
There was a problem hiding this comment.
[Suggestion] R3-26: sweepStale is a dead switch in production: this call — the only non-test listLiveSessions site — passes only { includeSelf }, so every read-only qwen sessions ps listing destructively sweeps (unlinks records its liveness check deems dead), and the option's own doc ("read-only callers can turn it off") is never used. The exact-match test assertions (toHaveBeenLastCalledWith({ includeSelf })) certify this call shape, so fixing it fails the tests until they move. (carried forward from round 3 — still standing at this commit; the original thread remains open) — Failure scenario: a polling usage (watch -n1 qwen sessions ps) multiplies the unlink TOCTOU confirmed as R4-6 this round: each run is one roll of the verdict-then-unlink window against recycled PIDs.
| records = await listLiveSessions({ includeSelf: argv.all ?? false }); | |
| records = await listLiveSessions({ | |
| includeSelf: argv.all ?? false, | |
| sweepStale: false, | |
| }); |
中文说明
R3-26:sweepStale 在生产中是死开关:这个调用——唯一非测试的 listLiveSessions 调用点——只传 { includeSelf },因此每次只读的 qwen sessions ps 列表都会破坏性地清理(unlink 被存活检查判定为已死的记录),而该选项自己的文档("只读调用方可以关掉它")从未被使用。精确匹配的测试断言(toHaveBeenLastCalledWith({ includeSelf }))认证了这个调用形状,因此修复它会在测试跟着改之前先让测试失败。(第 3 轮遗留——在当前提交仍然存在;原线程仍开放。)失败场景:轮询用法(watch -n1 qwen sessions ps)会放大本轮已确认的 R4-6 unlink TOCTOU:每次运行都是针对 PID 复用的一次"判定后删除"窗口抽奖。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| it('returns an empty list when the registry does not exist', async () => { | ||
| expect(await listLiveSessions({ includeSelf: true })).toEqual([]); |
There was a problem hiding this comment.
[Suggestion] R3-27: the documented "missing or unreadable directory → empty list, never throws" contract is pinned only for ENOENT: the catch-all around fs.readdir that tolerates EACCES/EIO is untested. (carried forward from round 3 — still standing at this commit; the original thread remains open) — Failure scenario: narrowing the catch (or letting readdir errors propagate) ships green; a permission-restricted registry dir then makes qwen sessions ps throw instead of listing nothing.
中文说明
R3-27:文档承诺的"目录缺失或不可读 → 空列表、绝不抛错"契约只为 ENOENT 钉住:fs.readdir 周围容忍 EACCES/EIO 的兜底 catch 未被测试。(第 3 轮遗留——在当前提交仍然存在;原线程仍开放。)失败场景:收窄 catch(或让 readdir 错误向外传播)会全绿出货;权限受限的注册表目录随后会让 qwen sessions ps 抛错而不是列出空。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const dir = getSessionRegistryDir(); | ||
| await fs.mkdir(dir, { recursive: true, mode: REGISTRY_DIR_MODE }); |
There was a problem hiding this comment.
[Suggestion] R3-28: the registry directory path itself is symlink-followed — distinct from the record-write symlink finding (R3-14), whose noFollow fix does not reach fs.mkdir/fs.chmod/the temp-file placement: all of them traverse a planted sessions → <victim> directory symlink. (carried forward from round 3 — still standing at this commit; the original thread remains open) — Failure scenario: an actor with write access to ~/.qwen (the sandbox-shared mount case the module doc names) renames or pre-empts sessions/ with a symlink to a victim directory; the next host session's registerSession then fs.chmods the victim directory to 0700 (stripping group/other bits from a directory the host process would otherwise never touch) and writes records inside it. Independently re-raised twice this round as Critical/low; round-3 severity kept pending a maintainer call. Suggested fix: lstat the would-be directory and refuse to proceed when it is a symlink.
中文说明
R3-28:注册表目录路径本身会跟随符号链接——与记录写入的符号链接问题(R3-14)不同,后者的 noFollow 修复覆盖不到 fs.mkdir/fs.chmod/临时文件落点:它们都会穿过预先植入的 sessions → <受害目录> 目录符号链接。(第 3 轮遗留——在当前提交仍然存在;原线程仍开放。)失败场景:拥有 ~/.qwen 写权限的行为者(模块文档点名的沙箱共享挂载场景)把 sessions/ 重命名或抢先替换为指向受害目录的符号链接;下一个宿主会话的 registerSession 随即对受害目录执行 fs.chmod 0700(剥掉宿主进程本不会触碰的目录的 group/other 位)并在其中写入记录。本轮被独立重提两次(均为 Critical/low);在维护者定夺前维持第 3 轮严重级。建议修复:对将成为目录的路径 lstat,是符号链接时拒绝继续。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const pid = value['pid']; | ||
| const sessionId = value['sessionId']; | ||
| const cwd = value['cwd']; |
There was a problem hiding this comment.
[Suggestion] R3-29: readRecord's required-field type guards (sessionId/cwd/name strings, pid integer > 0, kind union, startedAt finite) have zero test coverage — distinct from the size-cap/isFile guard finding (R3-4); only the kind union and future-schema branches are exercised. (carried forward from round 3 — still standing at this commit; the original thread remains open) — Failure scenario: relaxing any guard (e.g. accepting pid: -1 or non-string cwd) ships green; the invalid record then flows into ps rendering and the sort comparator.
中文说明
R3-29:readRecord 的必填字段类型防护(sessionId/cwd/name 为字符串、pid 为大于 0 的整数、kind 联合、startedAt 有限)测试覆盖为零——与大小上限/isFile 防护的发现(R3-4)不同;只有 kind 联合与未来 schema 分支被覆盖。(第 3 轮遗留——在当前提交仍然存在;原线程仍开放。)失败场景:放宽任一防护(例如接受 pid: -1 或非字符串 cwd)会全绿出货;非法记录随后流入 ps 渲染与排序比较器。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
Addresses Critical R4-24 from review round 4. `readMachineId` rejected only the empty string, so a `/etc/machine-id` holding systemd's literal `uninitialized` — the state `machine-id(5)` reserves for a provisioned-but-uncommitted host, which is where every OSTree-style image (Fedora CoreOS, rpm-ostree) and every host between `systemd-firstboot` and `machine-id-setup --commit` sits — was returned as the machine identity. The file exists and is readable there, so the hostname fallback never fires and each such host reports the same id. Paired with the kernel-constant initial PID-namespace inode, two of them sharing a registry directory (an NFS home, a `QWEN_HOME` on a shared volume) pass each other's origin gate: one host's `qwen sessions ps` probes the other's PID locally, finds nothing, and unlinks a live session's record. Registration is startup-only, so that session stays invisible to discovery for the rest of its life. Secondary mode: once a host commits its real id, its own earlier records become foreign to it — never listed, never swept, and able to block re-registration at a recycled PID. The sentinel is now skipped like an unreadable source, so the lookup falls through to `/var/lib/dbus/machine-id` and then to the hostname. `readMachineId` had no direct coverage at all (review R4-7); it now has five cases, including both fall-through paths. Reverting the one-line guard turns exactly the two sentinel cases red.
|
Round 4's Critical is fixed in R4-24 — Took the suggested edit as written, with the sentinel lifted to a named constant carrying the
Verification — The 37 Suggestions from this round are not addressed in this commit; keeping the push to the Critical. |
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; it is the suite that would exercise the new qwen sessions ps command and startup registration.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths and the Windows-guarded permission assertions of this diff unexercised.
Not explored to full depth (tool budget reached): Context: PR #8728 (QwenLM/qwen-code) adds a live-session ...: ran gemini.test.tsx only with -t filters for the tests this diff touches (3 of 71); did not re-run the full 71-test file, so pre-existing tests elsewhere in….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; it is the suite that would exercise the new qwen sessions ps command and startup registration。
未审查:build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI, leaving the non-Linux liveness fallback paths and the Windows-guarded permission assertions of this diff unexercised。
未探索到全部深度(达到工具调用预算):Context: PR #8728 (QwenLM/qwen-code) adds a live-session ...:ran gemini.test.tsx only with -t filters for the tests this diff touches (3 of 71); did not re-run the full 71-test file, so pre-existing tests elsewhere in…。
未审查:反向审计——评审时间预算不足,未能开始第 4 轮。
Test Plan(非阻断):src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more。
— qwen3.8-max via Qwen Code /review (v0.21.8)
…sted Addresses review round 5 on QwenLM#8728. R5-2 (Critical): atomicWriteFile's ownership-preservation fallback ran before any rename and stat'd the target through a planted symlink, so `fs.writeFile` + `chmod` landed the payload and the forced 0600 on the link's target — the clobber `noFollow` exists to prevent, and the one the EXDEV fallback already guards with unlink + O_EXCL. Under `noFollow` the target is now `lstat`ed and a symlink is discarded outright, so it is neither a mode-preservation source nor an ownership-fallback trigger and the replacing rename takes it. Applied the same lstat to the sync twin, which has no ownership fallback but would still copy a link target's mode onto the replacement. R5-7 (Critical): a same-origin record with no start token degraded `isSameProcess` to a bare liveness check, so any locally-live PID could be dressed in attacker-chosen sessionId/cwd/name — the origin fields needed to pass the gate are plaintext in every sibling record. Where a token is readable this build always writes one, so such a record is now withheld from callers. It is not swept: the PID is live and the writer may be a future version, and registration is startup-only, so a wrong unlink hides a session for good. Dead-PID records still sweep exactly as before. R5-7 also flagged the collision refusal as silent. Nothing removes an ownerless foreign record and registration happens once, so the session — and every later one drawing that PID — is missing from `qwen sessions ps` indefinitely. registerSession now reports it through an optional `onOriginConflict` callback (a bare `false` cannot be told apart from a transient I/O error) and the CLI turns it into a startup warning. R5-1: reject the all-zero machine id, which `machine-id(5)` reserves as invalid — the legacy form of the uncommitted state the `uninitialized` sentinel already covers, and the same cross-machine sweep hole. R5-12: the reboot-recycled-PID docstring claimed a boot-relative token bounds cross-reboot collisions; boot-relativeness is what allows them. Corrected to name what actually bounds them. R5-9 / R5-10: cover the origin guard's overwrite branch (a same-origin record left at this PID by an unclean exit — the blanket-refuse mutation the review measured as undetected now fails), and pin the "leave what this code did not write" contract for unparseable and future-schema records in unregisterSession and patchSessionRecord. Verification: packages/core session-registry (44), process-liveness (22), atomicFileWrite (84) and the five other noFollow consumers (162) pass; packages/cli gemini.test.tsx (72) passes; tsc --noEmit clean for both packages; core builds; eslint clean on all changed files.
|
@qwen-code /takeover stop |
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放:autofix 循环不再介入此 PR(在飞的一轮如有,将完成其有界工作)。重新打上 |
A regular file planted at `~/.qwen/sessions` permanently disabled the whole registry. `mkdir(recursive)` throws against it, nothing else in the module ever created that directory so nothing would ever clear it, and `listLiveSessions`' readdir failed with ENOTDIR into its catch-all and reported an empty machine. Under this module's own threat model — a co-tenant with write access to the shared qwen dir — that is two syscalls for a permanent registration blackout, the silent failure mode the directory's own comments call out as the wrong one. `registerSession` now lstats the path when mkdir fails and, if what is there is not a directory, unlinks it and retries once. That is the rule already applied one level down, where an unattributable entry at `<pid>.json` is replaceable: a non-directory at a path that must be a directory carries no record anyone could lose. A directory found there means mkdir failed for another reason (EACCES on a parent) and is rethrown to the existing best-effort handler, which still reports false. Scope was measured, not assumed: mkdir throws EEXIST on a regular file and on a symlink to one, and ENOENT on a dangling symlink — all three are repaired. It succeeds on a symlink resolving to a real directory, so that case never reaches this path and is documented as unaddressed rather than implied fixed; refusing it belongs with the directory's own hardening. Tests: three cases that each fail without the fix (planted file, dangling symlink, symlink to a file — the last also pinning that the unlink takes the link and never its target), plus a control that a healthy directory and a sibling's record are left untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses wenshao's review batch on QwenLM#8728. `sessions ps` truncated the NAME cell to `NAME_COL - 2`, folding the column gutter into the content budget. A name of display width 21-22 fits its cell and was ellipsized anyway, and what the ellipsis ate was the hash suffix `deriveSessionName` appends — the one part of the name that distinguishes two sessions started in the same directory, and the one part that cannot be inferred from the DIRECTORY column beside it. The suggested one-liner (budget = NAME_COL, cell = NAME_COL) fixes that band by regressing the common one: `deriveSessionName` caps its basename at 32, so names long enough to truncate are the norm, and every one of them would then fill the cell exactly and touch the PID digits. Cells are now joined by an explicit space and truncated to the full column width, which is what sibling `sessions list` already does — the gutter survives for every row and the 21-22 band renders whole. Test gaps, each verified by mutation: - `gemini.test.tsx` mocked `getProjectRoot` and `getTargetDir` to the same `/root`, so the registerSession contract test could not tell which one production read. Swapping the production call to `getProjectRoot()` kept the suite green; with the mocks given distinct values it fails (8 tests). - The "never registered" test asserted only the list output. A stub record written past the `existing === null` guard is rejected by `readRecord`, so the list stays empty while the stub sits at `<pid>.json` forever, never swept and invisible to readers. It now also asserts the filesystem. The registry directory has to exist first for this to bite at all — without it the stub write fails with ENOENT and the guard's removal is unobservable, which is why the first version of this assertion passed under mutation. - The multi-width truncation row hardcoded its PID/AGE padding, failing on column-width changes it does not test, and derived its age cell from wall-clock — a >30s stall between building the record and the handler's `Date.now()` rendered "2m" and failed spuriously. Paddings now come from the exported constants and the clock is frozen (Date only, so the handler's own awaits still run). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`listLiveSessions` swallowed every non-ENOENT readdir failure into a debug log and returned `[]`, so "nothing is running on this machine" and "this process could not read the registry" rendered identically. For a diagnostic command that is the answer most likely to be believed and least likely to be true: with the directory re-created by another uid, a restrictive NFS export, or a sandbox uid mapping, `qwen sessions ps` printed "No other Qwen Code sessions are running." and exited 0. It also made `ps`'s own error branch dead code — its test passed only by mocking a rejection the real function could never produce, which blessed an unreachable path. ENOENT keeps returning `[]`, because that one is an answer: no session has ever registered here. Everything else now throws, and `ps` already catches, prints the reason and exits non-zero. `ps` is the only production caller on this branch, so nothing else changes shape. The new test injects the errno through the fs mock rather than chmod'ing the directory: a permission fixture proves nothing when the suite runs as root, which CI containers and this repo's sandbox commonly do, and would have degraded to a silently passing assertion there. 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.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; it is the suite that would exercise the new qwen sessions ps command and startup registration.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI, leaving the non-Linux liveness/machine-id fallback paths of this diff unexercised.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, leaving the Windows symlink-guard behavior of this diff unexercised.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — finished within budget (~12 of ~38 calls).; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget..
Test Plan (not a blocker): src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; it is the suite that would exercise the new qwen sessions ps command and startup registration。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI, leaving the non-Linux liveness/machine-id fallback paths of this diff unexercised。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, leaving the Windows symlink-guard behavior of this diff unexercised。
未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — finished within budget (~12 of ~38 calls).;You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all planned checks completed within budget.。
Test Plan(非阻断):src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| replacing = unusable.entry | ||
| ? { entry: unusable.entry, requireFile: false } | ||
| : undefined; |
There was a problem hiding this comment.
[Critical] A directory planted at <pid>.json is unreplaceable: this branch judges the entry unattributable and therefore replaceable, but the replacing write commits through rename(2), which fails EISDIR when the target is a directory, and renameWithRetry retries only EPERM/EACCES. Nothing removes the obstruction — ensureRegistryDir repairs only the sessions directory itself, unregisterSession early-returns when readRecord is null, and the sweep skips unreadable entries — so registerSession returns false permanently and silently for every session drawing that PID. Same permanent-blackout class as the fixed Criticals R8-1 (file planted at the sessions dir) and R7-7 (uid-preserving fallback EACCES), at the cheapest attack cost of the three: one mkdir per targeted PID, under this module's own documented threat model. This re-asserts the still-open R6-7 thread (filed as a Suggestion in round 6) at Critical severity; probe-verified this round with a flip arm (registration fails on the unmodified code and succeeds once the directory is cleared). — Failure scenario: a co-tenant with write access to the shared sessions dir runs mkdir ~/.qwen/sessions/<pid>.json → the victim session drawing that PID fails registration at startup (registration only runs at startup) → absent from qwen sessions ps for its entire life, with no onOriginConflict and no warning — the "blackout that never lifts" the diff itself says silent is the wrong failure mode for.
Suggested fix (sketch — the repair must happen before the replacing write):
// An unattributable DIRECTORY carries no record anyone could lose —
// clear it and re-decide, mirroring ensureRegistryDir one level up.
if (unusable.origin === null && unusable.entry !== null) {
const st = await fs.lstat(filePath).catch(() => null);
if (st?.isDirectory()) {
await fs.rm(filePath, { recursive: true });
continue;
}
}中文说明
[Critical] 植入到 <pid>.json 的目录无法被替换:此分支把该条目判定为无法归属来源、因此可替换,但替换写入最终通过 rename(2) 提交——目标是目录时 rename 失败并返回 EISDIR,而 renameWithRetry 只在 EPERM/EACCES 时重试。没有任何路径会清除这个障碍——ensureRegistryDir 只修复 sessions 目录本身,unregisterSession 在 readRecord 返回 null 时提前返回,清理逻辑会跳过无法读取的条目——于是对每个抽到该 PID 的会话,registerSession 都会永久且静默地返回 false。这与已修复的两个 Critical 属于同一类「永不解除的致盲」:R8-1(在 sessions 目录路径植入文件)与 R7-7(uid 保留回退 EACCES),而本条是三者中攻击成本最低的:在本模块自己声明的威胁模型下,对每个目标 PID 只需一次 mkdir。本条重新主张仍处于 open 状态的 R6-7 线程(第 6 轮以 Suggestion 提交),并将其定级为 Critical;本轮已用探针验证(含翻转对照:未修改代码上注册失败,清除目录后注册成功)。失败场景:拥有共享 sessions 目录写权限的同租户执行 mkdir ~/.qwen/sessions/<pid>.json → 抽到该 PID 的受害会话在启动时注册失败(注册只在启动时发生)→ 该会话在其整个生命周期内缺席 qwen sessions ps,没有 onOriginConflict、没有任何警告——正是 diff 自己声称「静默是错误失败模式」的那种「永不解除的致盲」。建议修复(示意——修复必须发生在替换写入之前):对无法归属来源的目录条目先清除再重新决策(见英文代码块),与上一层 ensureRegistryDir 的规则保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
已修复:对无法归属且确认为目录的 .json 条目先校验 dev/ino,再清除非空目录并重新进入既有独占注册流程;新增非空目录回归测试。验证证据:cd packages/core && npx vitest run src/services/session-registry.test.ts(67/67);npm run typecheck;npm run build;目标文件 ESLint 与 Prettier 均通过。Commit: 375bb56。
| // No `includeSelf`: this process is not a session and never registers | ||
| // one. `qwen sessions ps` is resolved and run during yargs' argument | ||
| // parsing, which finishes and exits long before `startInteractiveUI` |
There was a problem hiding this comment.
[Suggestion] The PR description still advertises the removed --all flag. Both language sections say --all includes the calling process, but this diff deliberately removed the flag — the builder exposes only json (pinned by the test 'exposes no flag that claims to include this process'), and handlePs calls listLiveSessions() with no includeSelf. The removal was the chosen resolution of the round-7 --all blocker, but the description was never updated to match. — Failure scenario: the description is the contract a reviewer verifies against (its own Reviewer Test Plan says so) and becomes the merge-commit body; anyone following it runs qwen sessions ps --all, and since the sessions parser runs .strict() (config.ts:1102 — probe-verified against a mirror of the wiring with yargs 17.7.2), that invocation fails with "Unknown argument: all". The documented behavior is unreachable. Suggested fix: update both description sections to drop the --all clause, e.g. "The command lists only other processes: it runs and exits inside yargs' argument parsing, before any session registration, so there is no self record to include." No code change needed — the code side is deliberate and test-pinned.
中文说明
[Suggestion] PR 描述仍在宣传已被移除的 --all 选项。描述的中英文两节都写着 --all 会把调用方进程也算进去,但本 diff 已刻意移除该选项——builder 只暴露 json(由测试 'exposes no flag that claims to include this process' 固定),handlePs 调用 listLiveSessions() 时也没有任何 includeSelf。移除本身是第 7 轮 --all blocker 选定的解决方案,但描述没有同步更新。失败场景:描述是审查者据以验证的契约(其自带的 Reviewer Test Plan 也这么说),也会成为合并提交的信息;按其操作执行 qwen sessions ps --all 时,由于 sessions 解析器启用了 .strict()(config.ts:1102,已用 yargs 17.7.2 的等价 wiring 探针验证),该调用会以 "Unknown argument: all" 失败——文档承诺的行为实际不可达。建议修复:更新两节描述、删去 --all 相关句子(示意:「该命令只列出其他进程:它在 yargs 参数解析阶段运行并退出,早于任何会话注册,因此没有当前进程的记录可供包含」)。无需改代码——代码侧是刻意为之且有测试固定。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| * Returns an empty list rather than throwing when the registry directory | ||
| * is missing or unreadable — "no peers" and "cannot look" are the same | ||
| * outcome for every caller, and this sits on interactive paths. |
There was a problem hiding this comment.
[Suggestion] The JSDoc contradicts the implementation: this doc block says listLiveSessions "Returns an empty list rather than throwing when the registry directory is missing or unreadable", but the implementation returns [] only for ENOENT and rethrows every other readdir failure (EACCES, ENOTDIR, ...) — a deliberate contract change made in the HEAD commit, pinned by the test 'throws rather than reporting an empty machine it could not read'. The in-body comment now argues the exact opposite of this doc block. — Failure scenario: a future caller of this exported core-barrel API trusts the doc and calls listLiveSessions() without try/catch; on a machine where the sessions dir exists but is unreadable (re-created by another uid, restrictive NFS export, sandbox uid mapping) the promise rejects → unhandled rejection on an interactive path. The current sole caller (ps.ts) happens to catch.
| * Returns an empty list rather than throwing when the registry directory | |
| * is missing or unreadable — "no peers" and "cannot look" are the same | |
| * outcome for every caller, and this sits on interactive paths. | |
| * Returns an empty list when the registry directory does not exist | |
| * (`ENOENT`); any other readdir failure (e.g. `EACCES`) is rethrown so the | |
| * caller can distinguish "no peers" from "cannot look". |
中文说明
[Suggestion] JSDoc 与实现矛盾:此文档块声称当注册表目录缺失或不可读时 listLiveSessions「返回空列表而不是抛错」,但实现只对 ENOENT 返回 [],其余 readdir 失败(EACCES、ENOTDIR 等)都会重新抛出——这是 HEAD 提交刻意引入的契约变更,并由测试 'throws rather than reporting an empty machine it could not read' 固定。函数体内的注释现在主张的恰好与该文档块相反。失败场景:未来某个调用方信任文档、不加 try/catch 直接调用 listLiveSessions();在 sessions 目录存在但不可读的机器上(被其他 uid 重建、受限 NFS 导出、沙箱 uid 映射)promise 会 reject → 交互路径上出现未处理的 rejection。当前唯一调用方(ps.ts)恰好做了 catch。建议修复:把 JSDoc 改为与实现一致(见 suggestion 块)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| */ | ||
| /** | ||
| * Read at most {@link MAX_RECORD_BYTES} through an already-open handle. |
There was a problem hiding this comment.
[Suggestion] Orphaned JSDoc: readRecord's doc block (immediately above) is separated from its function by readCapped, which carries its own doc block — so editors and generated docs leave readRecord undocumented while readCapped appears to carry two stacked doc comments. The orphaned block holds the O_NOFOLLOW/single-handle rationale that justifies every write path's assertSameEntry pinning. — Concrete cost: documentation misattachment on the security-load-bearing reader; a maintainer reading down to readCapped sees a doc block describing a different function directly above it. Suggested fix: move readCapped and its doc block above readRecord's doc block, so each comment sits immediately before its function.
中文说明
[Suggestion] 孤立的 JSDoc:readRecord 的文档块(紧邻上方)与函数本体之间隔了一个 readCapped(后者有自己的文档块),于是编辑器/生成文档中 readRecord 没有任何文档,而 readCapped 看起来像是顶着两个叠加的文档注释。这个孤立的文档块承载的正是 O_NOFOLLOW/单句柄设计的理由——它是所有写路径 assertSameEntry 固定策略的依据。具体代价:安全关键读取函数的文档错位;往下读到 readCapped 的维护者会在它正上方看到一段描述别的函数的注释。建议修复:把 readCapped 及其文档块移到 readRecord 文档块的上方,使每段注释紧贴其函数。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // The write paths keep their hands off it too. | ||
| await patchSessionRecord({ sessionId: 'mine' }, DEAD_PID); | ||
| await unregisterSession(DEAD_PID); |
There was a problem hiding this comment.
[Suggestion] The sentinel-vs-sentinel patch assertion is vacuous through cross-test state: the preceding symlink test's unregisterSession(DEAD_PID) (line 1399) adds DEAD_PID to the module-level retiredPids set, which is never reset between tests, so this test's patchSessionRecord bails at the retired-PID guard (session-registry.ts:820) before readRecord/isSameOrigin ever run. Probe-verified: the record is opened 0 times for DEAD_PID vs 1 open + origin-gate refusal for a never-retired PID. The unlink half is unaffected (unregisterSession never consults retiredPids), and no other test covers the patch-side sentinel gate. — Failure scenario: a regression removing or breaking the origin gate inside patchSessionRecord leaves this test green — the sentinel-vs-sentinel patch case, the exact case this describe exists for, has no effective coverage; the assertion also silently changes meaning if tests are reordered. Suggested fix: plant the sentinel record at a PID this file never retires (e.g. DEAD_PID - 1) and aim the patchSessionRecord/unregisterSession calls at that PID, so the patch actually reaches the origin gate.
中文说明
[Suggestion] sentinel 对 sentinel 的 patch 断言因跨测试状态而失效:前面 symlink 测试里的 unregisterSession(DEAD_PID)(第 1399 行)已把 DEAD_PID 加入模块级 retiredPids 集合,而该集合在测试之间从不重置,于是本测试的 patchSessionRecord 在 readRecord/isSameOrigin 运行之前就在 retired-PID 守卫(session-registry.ts:820)处提前返回。已用探针验证:对 DEAD_PID 的 patch 打开记录文件 0 次;换一个从未被 retire 的 PID 则打开 1 次并被 origin 门禁拒绝。unlink 那一半不受影响(unregisterSession 从不查询 retiredPids),而其他测试也没有覆盖 patch 侧的 sentinel 门禁。失败场景:一个移除或破坏 patchSessionRecord 内 origin 门禁的回归不会让本测试变红——sentinel 对 sentinel 的 patch 分支(正是该 describe 存在的目的)实际上没有覆盖;测试顺序变化时断言的含义还会悄悄改变。建议修复:把 sentinel 记录植入一个本文件从未 retire 过的 PID(例如 DEAD_PID - 1),并让 patchSessionRecord/unregisterSession 指向该 PID,使 patch 真正走到 origin 门禁。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| throw err; | ||
| } | ||
| } | ||
| if (existingStat?.isSymbolicLink()) existingStat = undefined; |
There was a problem hiding this comment.
[Suggestion] The async symlink-discard guard's mode-preservation role has zero coverage: every async noFollow+symlink test either passes forceMode: true + mode (which skips the preservation branch) or never asserts the replacement's mode, while the sync twin IS covered by the new test at :484 (no forceMode, asserts 0o600). Probe-verified at HEAD: deleting this guard line leaves all 30 noFollow tests green while the replacement inherits the link's 0o777 (world-writable) via existingMode → desiredMode → tryChmod. — Failure scenario: production callers in the affected class exist — extensionSettings.ts writes with { noFollow: true } and no mode at three sites, plus ~10 more mode-less noFollow callers; a co-tenant who plants a symlink at such a path gets the next write committed world-writable, so any local user can then rewrite that file's contents. A future edit dropping or weakening this guard ships green. Suggested fix: add an async mirror of the new sync test — plant a symlink, call atomicWriteFile(planted, 'payload', { mode: 0o600, noFollow: true }) without forceMode, and assert the replacement's mode is 0o600 rather than the link's 0o777.
中文说明
[Suggestion] 异步 symlink 丢弃守卫在「模式保留」这一作用上完全没有覆盖:所有异步 noFollow+symlink 测试要么传了 forceMode: true + mode(跳过保留分支),要么根本不断言替换文件的模式;而同步版本已由 :484 的新测试覆盖(无 forceMode、断言 0o600)。已在 HEAD 上探针验证:删除这行守卫后全部 30 个 noFollow 测试仍为绿,而替换文件会经由 existingMode → desiredMode → tryChmod 继承链接的 0o777(全局可写)。失败场景:受影响的生产调用方确实存在——extensionSettings.ts 有三处以 { noFollow: true } 且不带 mode 写入,另有约十余处不带 mode 的 noFollow 调用;同租户在这样的路径上植入 symlink 后,下一次写入提交的文件就是全局可写,任何本地用户都能改写其内容。未来删除或弱化该守卫的改动可以静默通过。建议修复:为异步路径补一个与同步新测试对应的测试——植入 symlink,用不带 forceMode 的 atomicWriteFile(planted, 'payload', { mode: 0o600, noFollow: true }) 调用,断言替换文件模式为 0o600 而不是链接的 0o777。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(patchCalls).toHaveLength(1); | ||
| expect(patchCalls[0]).toMatchObject({ cwd: expected }); |
There was a problem hiding this comment.
[Suggestion] Both relocate tests (this non-owner one and its owner twin at ~273-292) assert only the registry patch, never that the runtime.json sidecar was left untouched — while the startNewSession twin asserts both halves ("nothing written", expect(await readRuntimeStatus(bPath)).toBeNull()), and this file's header says it exists to pin that sidecar writes happen only when runtimeStatusEnabled is on. The guard protecting the relocate path is the single early return in refreshCurrentRuntimeStatus (config.ts:3942). — Failure scenario: if a future change deletes or inverts that early return — plausible, since this PR's own comments argue the patch must not inherit the gate, and the two code paths sit lines apart — a non-interactive process's /cd rewrites a concurrent sibling shell's chats/<sessionId>.runtime.json, advertising the wrong workDir for the sibling session, and every test in this file and in config.test.ts stays green (the only other relocate runtime-status test, config.test.ts:5912, calls markRuntimeStatusEnabled() first — owner case only). Suggested fix: mirror the swap test's second half in both relocate tests — after the relocate, add expect(await readRuntimeStatus(config.storage.getRuntimeStatusPath(sessionA))).toBeNull(); (storage is unreplaced here because skipArtifactMigration: true, so the path is observable under tmpDir).
中文说明
[Suggestion] 两个 relocate 测试(本条的非 owner 测试与约 273-292 行的 owner 孪生测试)都只断言了注册表 patch,从未断言 runtime.json 旁路文件没有被写入——而 startNewSession 的对应测试两半都断言了("nothing written"、expect(await readRuntimeStatus(bPath)).toBeNull()),且本文件开头的注释说明它存在的目的就是固定「只有 runtimeStatusEnabled 打开时才写旁路」。保护 relocate 路径的守卫只是 refreshCurrentRuntimeStatus(config.ts:3942)里的一个提前 return。失败场景:如果未来改动删除或反转那个提前 return——这很有可能,因为本 PR 自己的注释主张 patch 不应继承该门禁,而且两条代码路径只隔几行——非交互进程的 /cd 就会改写并发兄弟 shell 的 chats/<sessionId>.runtime.json,为兄弟会话发布错误的 workDir,而本文件与 config.test.ts 中的所有测试仍然为绿(config.test.ts 中唯一的另一个 relocate runtime-status 测试 :5912 事先调用了 markRuntimeStatusEnabled()——只覆盖 owner 场景)。建议修复:在两个 relocate 测试中都补上 swap 测试的第二半——relocate 之后加 expect(await readRuntimeStatus(config.storage.getRuntimeStatusPath(sessionA))).toBeNull();(这里因为 skipArtifactMigration: true,storage 未被替换,该路径在 tmpDir 下可观察)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| * `Config.refreshSessionId` queues its patch on a fire-and-forget chain | ||
| * and returns without awaiting it, so a `/clear` immediately before quit |
There was a problem hiding this comment.
[Suggestion] This rationale comment names Config.refreshSessionId — a method that does not exist anywhere in the repo (verified by grep at HEAD and at the merge base). The producer it describes is Config.startNewSession (config.ts:3873), which queues the registry patch on queueRuntimeStatusWrite and returns without awaiting it. A sibling new comment repeats the stale name (session-registry.test.ts:1510 — please fix both; the third occurrence at startInteractiveUI.tsx:91 pre-dates this PR). — Failure scenario: a maintainer auditing the patch-vs-withdraw serialization guarantee — the whole reason writeQueue + retiredPids exist — greps for Config.refreshSessionId to verify the fire-and-forget claim and finds nothing; the rationale for a correctness-critical ordering mechanism becomes unverifiable from its own documentation.
| * `Config.refreshSessionId` queues its patch on a fire-and-forget chain | |
| * and returns without awaiting it, so a `/clear` immediately before quit | |
| * `Config.startNewSession` queues its patch on a fire-and-forget chain | |
| * and returns without awaiting it, so a `/clear` immediately before quit |
中文说明
[Suggestion] 这段说明注释引用了 Config.refreshSessionId——仓库中并不存在这个方法(已在 HEAD 与 merge base 上 grep 验证)。它描述的生产者其实是 Config.startNewSession(config.ts:3873):该方法把注册表 patch 排入 queueRuntimeStatusWrite 并在不 await 的情况下返回。另一处新注释重复了这个过时的名字(session-registry.test.ts:1510——请一并修改;startInteractiveUI.tsx:91 处的第三处早于本 PR,不在本次修复范围)。失败场景:维护者在审计 patch 与注销的串行化保证(正是 writeQueue + retiredPids 存在的全部理由)时,按注释去 grep Config.refreshSessionId 验证 fire-and-forget 的说法,却什么都找不到——一个正确性关键机制的设计理由因此无法从其自身文档得到验证。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Handle the unresolved Critical on QwenLM#8728 by removing an unattributable directory at the current PID record path before retrying the existing exclusive registration flow. Cover a non-empty planted directory so the permanent registration blackout cannot regress. Verified: packages/core session-registry 67 passed; core typecheck and build; targeted ESLint and Prettier.
Superseded by head 375bb56; no current unreplied Critical remains. Clearing stale CHANGES_REQUESTED state at the PR author request.
yiliang114
left a comment
There was a problem hiding this comment.
Stacked review pass (reviewed at stack head bbb4d057, findings attributed to this PR's increment). Two new findings on the registry, both in the same adversarial-same-uid threat model the module's own comments adopt. The rest of the registry (procStart token, noFollow writes, filename-vs-content pid guard) verified solid.
| * creates the directory, so nothing else would ever clear it: the blackout | ||
| * would last until a human deleted the file by hand. Under this module's | ||
| * threat model — a co-tenant with write access to the shared qwen dir — | ||
| * that is two syscalls for a permanent denial of discovery. |
There was a problem hiding this comment.
[P2] The sweep unlinks on a stale read — read→validate→unlink with no re-validation in between.
Failure scenario: session A (pid N) crashes leaving N.json; PID N is recycled by a fresh qwen session whose registerSession() writes a new N.json in the window between the old record's read and this unlink — the sweep then deletes the live session's brand-new record. That session silently disappears from sessions ps, list_agents, and peer discovery until a session-swap patch happens to rewrite it.
Suggested fix: re-read and re-validate (filename/pid/procStart) immediately before unlinking, or compare mtime/content before deleting.
There was a problem hiding this comment.
已修复 + 验证证据:commit 55cf254 以原子 rename 隔离待清理 inode;若移动到的是并发发布的新记录,则恢复原路径并保留数据。验证:packages/core 下 npx vitest run src/services/session-registry.test.ts(69 passed);ESLint、Prettier、core typecheck 通过。
| return path.join(Storage.getGlobalQwenDir(), 'sessions'); | ||
| } | ||
|
|
||
| export function getSessionRecordPath(pid: number = process.pid): string { |
There was a problem hiding this comment.
[P2] Directory preparation here lacks the symlink/owner guard the sibling uds-inbox.ts already has (isOwnDirectory: lstat + ownership check before chmod, explicitly because mkdir accepts a pre-planted symlink-to-directory and chmod follows it).
If ~/.qwen/sessions does not exist yet and a party that can write ~/.qwen plants it as a symlink to a directory the victim owns, the chmod 0700 lands on the attacker-chosen target (clobbering its mode bits) and records containing cwd/sessionId get written there. The comment right below treats same-uid processes as adversarial, so the guard belongs here too.
Suggested fix: reuse the isOwnDirectory-style check — lstat, reject non-real-directories and foreign owners before chmod.
There was a problem hiding this comment.
已修复 + 验证证据:commit 55cf254 在 chmod/写记录前 lstat 并拒绝 symlink-to-directory 或非当前 uid 目录;回归断言目标目录权限和内容不变。验证:session-registry.test.ts(69 passed);ESLint、Prettier、core typecheck 通过。
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; it is the suite that would exercise the new qwen sessions ps command and startup registration.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI, leaving the non-Linux liveness/machine-id fallback paths of this diff unexercised.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, leaving the Windows-specific paths of this diff unexercised.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; Context: PR #8728 (QwenLM/qwen-code) adds a live-session ...: none — all checks I opened were finished within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — completed the walk within budget., and 11 more.
Test Plan (not a blocker): src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally; it is the suite that would exercise the new qwen sessions ps command and startup registration。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI, leaving the non-Linux liveness/machine-id fallback paths of this diff unexercised。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI, leaving the Windows-specific paths of this diff unexercised。
未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:none — all checks I started were completed within budget.;Context: PR #8728 (QwenLM/qwen-code) adds a live-session ...:none — all checks I opened were finished within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks above were completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — completed the walk within budget.,另有 11 条。
Test Plan(非阻断):src/utils/process-liveness.test.ts — no such file or directory; src/services/session-registry.test.ts — no such file or directory; src/agents/team/teamHelpers.test.ts — no such file or directory; src/commands/sessions/ps.test.ts — no such file or directory; src/commands/sessions.test.ts — no such file or directory; and 7 more。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(JSON.parse(await fs.readFile(filePath, 'utf8')).sessionId).toBe( | ||
| 'replacement', | ||
| ); |
There was a problem hiding this comment.
[Critical] R10-1: This new test fails reproducibly — it failed in this review's official test run and reproduces 8/8 in isolation; base-delta measurement confirms it does not fail at the merge base (netNew). The rename mock publishes the replacement via rm + writeFile, which frees the stale record's inode; on ext4 the replacement gets the same inode back (measured 29/30), so sweepStaleEntry's dev+ino check on the quarantined file passes and it unlinks the live replacement — this final readFile then fails with ENOENT. — Failure scenario: npm test --workspace=packages/core exits non-zero in CI. The one observed green run under full-suite parallel load was inode contention masking, not a fix. The same inode-reuse ordering can also arise in production (two concurrent sweepers fully freeing a stale inode before a recycled-PID registration stages its temp file), so this may expose a real hole in the dev+ino pinning, not only a mock artifact. Suggested fix: publish the replacement the way production does — stage a temp file and rename it onto the path so its inode is allocated while the stale record still exists (distinct inode guaranteed); alternatively/additionally harden sweepStaleEntry to verify the quarantined record's contents (origin/procStart fields read at validation) before unlinking, not dev+ino alone.
中文说明
R10-1:这条新测试可复现地失败——在本评审的正式测试运行中失败,单独运行时 8/8 复现;base-delta 测量确认它在合并基上不会失败(netNew)。rename mock 用 rm + writeFile 发布替换记录,这会释放旧记录的 inode;在 ext4 上替换文件会拿回同一个 inode(实测 29/30),于是 sweepStaleEntry 对被隔离文件的 dev+ino 检查通过,进而 unlink 了存活的替换记录——最后这个 readFile 以 ENOENT 失败。失败场景:npm test --workspace=packages/core 在 CI 中非零退出。全量套件并行下观察到的唯一一次绿是 inode 竞争的遮蔽,不是修复。同样的 inode 复用顺序在生产中也可能出现(两个并发清理者在一个复用 PID 的注册暂存临时文件之前彻底释放了旧 inode),因此这可能暴露了 dev+ino 固定机制的真实漏洞,而不只是 mock 伪影。建议修复:用生产的方式发布替换——先暂存临时文件再 rename 到路径上,使其 inode 在旧记录仍存在时分配(保证不同 inode);或者/同时加固 sweepStaleEntry,在 unlink 前校验被隔离记录的内容(验证时读到的 origin/procStart 字段),而不只是 dev+ino。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| await ensureRegistryDir(dir); | ||
| // mkdir's mode is masked by the umask, and does nothing at all when | ||
| // the directory already exists — chmod is what actually guarantees | ||
| // 0700 on an upgrade from a build that created it more loosely. | ||
| await fs.chmod(dir, REGISTRY_DIR_MODE); |
There was a problem hiding this comment.
[Critical] R10-2: Probe-verified TOCTOU — the directory validation added in this very commit (ensureRegistryDir's final path-based lstat rejecting symlink-to-directory / foreign-uid directories) and this path-based fs.chmod are two separate syscalls, and chmod(2) follows symlinks. A co-tenant (the module's documented same-uid threat model) swapping ~/.qwen/sessions for a symlink in the window redirects the mode change to an arbitrary target and the record write into an attacker-chosen directory. — Failure scenario: a deterministic interleaving probe shows the chmod follows a swapped-in symlink (victim path forced to 0o700) and the record lands in the attacker's directory; a real two-process race won 4.30% and 5.44% of registrations across two arms — not a microsecond needle. In the cross-uid configurations the module names (sandboxed root co-tenant sharing the host's ~/.qwen, sudo-run qwen) this extends the attacker's mutation surface from inside the shared directory to arbitrary victim-owned paths. Suggested fix: pin the check and the mode change on one descriptor — open the directory O_RDONLY|O_NOFOLLOW|O_DIRECTORY, fstat to verify isDirectory+uid, then fchmod through the handle (keep the path-based flow on win32). Note the record-write limb also resolves through the directory path, so a complete fix should anchor the validated directory across the whole registration, not just the chmod.
中文说明
R10-2:探针验证的 TOCTOU——本提交新增的目录校验(ensureRegistryDir 最后那次基于路径的 lstat,拒绝 symlink-to-directory / 非本 uid 所有的目录)与这个基于路径的 fs.chmod 是两个独立的系统调用,而 chmod(2) 会跟随符号链接。同租户(本模块文档化的同 uid 威胁模型)可以在窗口期内把 ~/.qwen/sessions 换成符号链接,将权限修改重定向到任意目标,并把记录写入攻击者选定的目录。失败场景:确定性交错探针显示 chmod 跟随了换入的符号链接(受害者路径被强制改为 0o700),且记录落进了攻击者的目录;真实双进程竞争在两组实验中分别赢得 4.30% 和 5.44% 的注册——这不是微秒级的针尖窗口。在模块点名的跨 uid 部署下(共享宿主 ~/.qwen 的沙箱 root 同租户、sudo 运行的 qwen),这把攻击者的变更面从共享目录内部扩展到任意受害者拥有的路径。建议修复:把校验与权限修改固定在同一个描述符上——以 O_RDONLY|O_NOFOLLOW|O_DIRECTORY 打开目录,fstat 校验 isDirectory+uid,再通过句柄 fchmod(win32 保留基于路径的流程)。注意记录写入同样经过目录路径解析,完整修复应把已验证的目录固定在整个注册过程,而不只是 chmod。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| (requireFile && !stat.isFile()) || | ||
| stat.dev !== expected.dev || | ||
| stat.ino !== expected.ino | ||
| ) { |
There was a problem hiding this comment.
[Critical] R10-4: Probe-verified — assertSameEntry pins the validated entry by dev+ino only, at every mutation site (patchSessionRecord's assertCanCommit, unregisterSession's pre-unlink assert, registerSession's pinned replace and directory-clear, sweepStaleEntry's post-quarantine check). An unlinked entry whose freed inode is reused by a foreign-origin record passes the check — ext4 same-directory reuse measured 30/30. — Failure scenario: probe-verified attack: a co-tenant unlinks <pid>.json after readRecord validation and plants a foreign live record on the reused inode during temp-file staging; the patch arm overwrote it and the unregister arm deleted it, while the origin gate refuses the same record at validation time. Registration is startup-only, so the victim session is absent from discovery for its entire life. Distinct from R10-1: adversarial trigger, cross-origin corruption/deletion on the write paths — fixing sweepStaleEntry alone leaves these open. Suggested fix: bind the commit to content as well as identity — before committing, re-open the entry through readEntryFlags() and re-verify the validated origin fields (machineId/pidNamespace, plus procStart/sessionId) still match; a planted foreign record fails that check even on the recycled inode.
中文说明
R10-4:探针验证——assertSameEntry 在所有变更点(patchSessionRecord 的 assertCanCommit、unregisterSession 的 unlink 前断言、registerSession 的固定替换与目录清除、sweepStaleEntry 的隔离后检查)都只用 dev+ino 固定已验证条目。被 unlink 的条目若其释放的 inode 被一条异源记录复用,就能通过检查——ext4 同目录复用实测 30/30。失败场景:探针验证的攻击:同租户在 readRecord 验证之后 unlink <pid>.json,并在临时文件暂存期间把一条存活的异源记录植入复用的 inode;patch 分支覆盖了它、unregister 分支删除了它,而 origin 门禁在验证时本会拒绝同一条记录。注册只在启动时发生,受害会话在其整个生命周期内缺席发现机制。与 R10-1 不同:这是对抗性触发、写路径上的跨源破坏/删除——只修 sweepStaleEntry 无法覆盖。建议修复:提交前除身份外再绑定内容——重新以 readEntryFlags() 打开条目,复核验证过的 origin 字段(machineId/pidNamespace,以及 procStart/sessionId)仍然匹配;植入的异源记录即使在复用的 inode 上也会失败于该校验。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| .command(listCommand) | ||
| .command(psCommand) | ||
| .demandCommand(1, 'You need at least one command before continuing.') |
There was a problem hiding this comment.
[Suggestion] R10-3: The new user-facing subcommand qwen sessions ps ships with no entry in docs/users/features/commands.md, which documents the sibling qwen sessions list with a table row, flag table, output description, and examples; a repo-wide grep finds zero sessions ps hits in docs/, and this diff touches no docs/ files. — Concrete cost: users consulting the official command reference cannot discover qwen sessions ps or its --json flag; the docs drift from the shipped CLI surface is created at merge time. Suggested fix: add a qwen sessions ps row to the Session Management table and a sibling section mirroring sessions list (flags: --json; human table columns NAME/PID/AGE/DIRECTORY; JSON Lines record fields).
中文说明
R10-3:新的用户可见子命令 qwen sessions ps 发布时在 docs/users/features/commands.md 中没有任何条目,而该文档为同族的 qwen sessions list 记录了表格行、选项表、输出说明和示例;全仓库 grep 在 docs/ 中找不到任何 sessions ps,且本 diff 未触碰任何 docs/ 文件。具体代价:查阅官方命令参考的用户无法发现 qwen sessions ps 及其 --json 选项;文档与已发布 CLI 面的漂移在合并时即被制造。建议修复:在 Session Management 表格中新增 qwen sessions ps 行,并仿照 sessions list 增加小节(选项:--json;人类可读表列 NAME/PID/AGE/DIRECTORY;JSON Lines 记录字段)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| builder: (yargs: Argv) => | ||
| yargs.option('json', { | ||
| type: 'boolean', | ||
| describe: 'Output as JSON Lines', | ||
| default: false, | ||
| }), |
There was a problem hiding this comment.
[Suggestion] R9-2 (re-report from round 9 — still standing at this commit): the PR description still advertises the removed --all flag — both language sections say --all includes the calling process, but this builder exposes only json and ps.test.ts pins the absence (expect(Object.keys(options)).toEqual(['json']), with a comment saying it cannot come back without a registration to go with it). — Concrete cost: readers of the PR description expect a flag that does not exist; description and shipped CLI surface contradict each other at merge time. Suggested fix: update both language sections of the PR description to drop the --all sentence (or restore the flag together with a registration to go with it).
中文说明
R9-2(第 9 轮重报——在本提交仍然存在):PR 描述仍在宣传已被移除的 --all 选项——中英文两部分都写着 --all 会包含调用方进程,但这个 builder 只暴露 json,且 ps.test.ts 已把缺失钉死(expect(Object.keys(options)).toEqual(['json']),注释说明没有配套注册它就不能回来)。具体代价:PR 描述的读者会期待一个不存在的选项;描述与发布的 CLI 面在合并时互相矛盾。建议修复:更新 PR 描述的两个语言版本,删掉 --all 那句(或连同配套注册一起恢复该选项)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| * from here on, and `listLiveSessions`' `readdir` fails with `ENOTDIR` into | ||
| * its catch-all and reports an empty machine. Nothing else in this module |
There was a problem hiding this comment.
[Suggestion] R10-7: Second location of the R9-3 stale-doc pattern — this ensureRegistryDir JSDoc claims listLiveSessions' readdir fails with ENOTDIR 'into its catch-all and reports an empty machine', but listLiveSessions rethrows every non-ENOENT readdir error (implemented and test-pinned by 'throws rather than reporting an empty machine it could not read'). — Failure scenario: a future caller codes against the documented catch-all behavior and omits error handling; on any EACCES/ENOTDIR readdir failure the call rejects instead of returning the documented empty list. Suggested fix: rewrite the sentence to match the implemented contract — ENOENT → empty list; every other readdir failure is rethrown for the caller to report.
中文说明
R10-7:R9-3 过期文档模式的第二处——ensureRegistryDir 的这段 JSDoc 声称 listLiveSessions 的 readdir 遇到 ENOTDIR 会『落入兜底 catch 并报告空机器』,但 listLiveSessions 对一切非 ENOENT 的 readdir 错误都重新抛出(已实现且被 'throws rather than reporting an empty machine it could not read' 测试钉住)。失败场景:未来调用方按文档的兜底行为写代码并省略错误处理;任何 EACCES/ENOTDIR 的 readdir 失败都会变成 reject,而不是文档承诺的空列表。建议修复:把这句话改写为与实现契约一致——ENOENT → 空列表;其他一切 readdir 失败都重新抛出,由调用方报告。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // 'unsupported' — no hard links on this filesystem. Fall | ||
| // through to the replacing write, which is where this path has | ||
| // always been; the exclusivity gap is the price of the | ||
| // filesystem. |
There was a problem hiding this comment.
[Suggestion] R10-17: Mutation-verified — the no-hard-link degradation (linkRecordExclusive returns 'unsupported'; registerSession falls through to the replacing write) is pinned by zero tests: the node:fs/promises mock has seams for readdir/rename/open/writeFile but none for fs.link, so linkRecordExclusive only ever returns 'created' in the suite. Mutation: if (outcome === 'unsupported') return false; ships green. — Failure scenario: a regression in the errno classification (EPERM/ENOSYS/ENOTSUP/EOPNOTSUPP/EMLINK/EXDEV no longer mapped to 'unsupported') or in the fall-through write ships green and then, only on filesystems without hard links (some NFS/FUSE/container mounts), turns every startup registration into a silent false. Suggested fix: add a link wrapper to the existing mock (throw an ENOTSUP-coded error once, mirroring readdirFails) and assert registerSession still returns true with a present, parseable record.
中文说明
R10-17:突变验证——无硬链接降级(linkRecordExclusive 返回 'unsupported';registerSession 落入替换写入)没有任何测试钉住:node:fs/promises mock 有 readdir/rename/open/writeFile 的缝,唯独没有 fs.link 的缝,因此套件中 linkRecordExclusive 永远只返回 'created'。突变:if (outcome === 'unsupported') return false; 可以绿着上线。失败场景:errno 分类的回归(EPERM/ENOSYS/ENOTSUP/EOPNOTSUPP/EMLINK/EXDEV 不再映射为 'unsupported')或 fall-through 写入的回归会绿着上线,随后在没有硬链接的文件系统(某些 NFS/FUSE/容器挂载)上把每次启动注册变成静默 false。建议修复:给现有 mock 加一个 link 包装(仿照 readdirFails 抛一次 ENOTSUP 错误),断言 registerSession 仍返回 true 且记录存在可解析。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| /** | ||
| * Read and validate one record. Returns null for anything unusable. |
There was a problem hiding this comment.
[Suggestion] R9-4 (re-report from round 9 — still standing at this commit): readRecord's JSDoc block is orphaned — it sits stacked above readCapped's own doc block, so readCapped carries two doc comments (the first describing a different function) while readRecord, the most security-critical read in the module, is undocumented. — Concrete cost: IDE/TypeDoc surface the single-handle/identity-binding contract on nothing; a future refactor of readRecord (splitting the stat from the read, re-opening by path) gets no editor-visible warning about the invariant assertSameEntry/sweepStaleEntry depend on. Suggested fix: move the 'Read and validate one record' block below readCapped so it directly precedes async function readRecord.
中文说明
R9-4(第 9 轮重报——在本提交仍然存在):readRecord 的 JSDoc 块成了孤儿——它叠在 readCapped 自己的文档块之上,于是 readCapped 顶着两段文档(第一段描述的是另一个函数),而模块中安全上最关键的读取 readRecord 反而没有文档。具体代价:IDE/TypeDoc 无处展示单句柄/身份绑定的契约;未来对 readRecord 的重构(把 stat 与 read 拆开、按路径重新打开)在编辑器里看不到任何关于 assertSameEntry/sweepStaleEntry 所依赖不变量的提示。建议修复:把 'Read and validate one record' 块移到 readCapped 之下,使其紧邻 async function readRecord。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| throw err; | ||
| } | ||
| } | ||
| if (existingStat?.isSymbolicLink()) existingStat = undefined; |
There was a problem hiding this comment.
[Suggestion] R9-6 (re-report from round 9 — still standing at this commit): the async symlink-discard guard's mode-preservation role has zero coverage — every async noFollow+symlink test either passes forceMode: true + mode (the test at atomicFileWrite.test.ts:438, which skips the preservation branch) or never asserts the replacement's mode at all (the tests at ~1139-1166 assert content/type only). — Failure scenario: deleting this discard ships green: with it gone, existingStat becomes the link's own stat and the replacement inherits the symlink's mode (a 0o777 link donates its mode to the credential file being written) — no test observes the mode outcome. Suggested fix: add an async noFollow symlink test without forceMode that asserts the replacement's mode does NOT come from the link target (mirror the sync test's shape with an explicitly chmod'd victim).
中文说明
R9-6(第 9 轮重报——在本提交仍然存在):异步符号链接丢弃守卫的模式保留作用零覆盖——每个异步 noFollow+symlink 测试要么传 forceMode: true + mode(atomicFileWrite.test.ts:438 的测试,跳过保留分支),要么完全不断言替换文件的模式(~1139-1166 的测试只断言内容/类型)。失败场景:删除这个丢弃逻辑可以绿着上线:没有它,existingStat 变成链接自身的 stat,替换文件继承符号链接的模式(0o777 的链接把模式捐给正在写入的凭证文件)——没有测试观察模式结果。建议修复:新增一个不带 forceMode 的异步 noFollow 符号链接测试,断言替换文件的模式不来自链接目标(仿照同步测试的形态,用显式 chmod 过的受害者)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(patchCalls).toHaveLength(1); | ||
| expect(patchCalls[0]).toMatchObject({ cwd: expected }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R9-7 (re-report from round 9 — still standing at this commit): both relocate tests assert only the registry patch, never that the runtime.json sidecar state is unaffected — and neither test bootstraps a sidecar (no markRuntimeStatusEnabled), so the sidecar-ENABLED /cd path (refreshCurrentRuntimeStatus write + registry patch ordering) is never exercised at all. — Failure scenario: a regression where relocateWorkingDirectory skips the registry patch when the sidecar write fails (or corrupts the sidecar) ships green: no test combines an enabled/failing sidecar with the relocate flow, and the patch-only assertions cannot see the sidecar half. Suggested fix: add a relocate test that calls markRuntimeStatusEnabled() first (and one that fails the sidecar write via the existing failSidecarWrite seam) and asserts both the sidecar post-state and the registry patch.
中文说明
R9-7(第 9 轮重报——在本提交仍然存在):两个 relocate 测试都只断言注册表 patch,从不断言 runtime.json 旁路文件状态未受影响——而且两个测试都没有启用旁路(均未调用 markRuntimeStatusEnabled),因此启用旁路的 /cd 路径(refreshCurrentRuntimeStatus 写入 + 注册表 patch 的顺序)完全没有被执行。失败场景:relocateWorkingDirectory 在旁路写入失败时跳过注册表 patch(或破坏旁路)的回归可以绿着上线:没有测试把启用/失败的旁路与 relocate 流程组合,而只看 patch 的断言看不到旁路那一半。建议修复:新增先调用 markRuntimeStatusEnabled() 的 relocate 测试(以及一个用现有 failSidecarWrite 缝使旁路写入失败的测试),同时断言旁路事后状态与注册表 patch。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
Closing this one too, and restarting it from a clean base. The branch grew past the point where reviewing it was useful: What comes back will be the same feature at roughly the original size, rebased on current main, carrying the findings from this round that are genuinely right — symlink-safe registry writes, Windows guards on the POSIX-only assertions — and leaving out the PID-namespace and machine-id machinery, which has no stated threat model here and is where most of the churn came from. Branch is kept. Tracking issue: #8724. |
|
The PR is closed (not merged). No review is needed. |
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).
First step of #8724. Landed on its own because it is useful by itself and changes nothing about how a session behaves — no new transport, no new tool, no new message path.
What this PR does
Each interactive session records itself at
~/.qwen/sessions/<pid>.jsonwhile it runs and unlinks the record on exit, so "which Qwen Code sessions are running on this machine right now" is onereaddirplus a few small reads. A newqwen sessions pscommand reads that index and prints the live sessions;--jsonemits JSON Lines for scripting, and--allincludes the calling process.isPidAlivealso moves out ofteamHelpersintoutils/process-liveness.ts(same semantics, now tested) so the registry does not become a third copy.Nothing else in the product reads the registry yet. The registry gains an
ipcPathfield only when the transport lands; nothing reads or writes it in this PR.Why it's needed
#8724needs a way to enumerate live sessions before any of it can work, and there is no existing index that answers the question.<projectDir>/chats/<sessionId>.runtime.jsonalready records(pid, session_id, work_dir, …), but it answers a different question and cannot serve this one:The two coexist.
runtime.jsonstays the stable, kimi-compatible "which session is PID X serving" sidecar for external observers; this registry is the discovery index for the CLI's own features.Reviewer Test Plan
How to verify
Correctness details worth a look during review — each is covered by a named test, and each is a case where the obvious implementation is wrong:
starttimefrom/proc/<pid>/stat, parsed by anchoring on the last)— a process namedmy ) procis legal and would misalign every field otherwise. Platforms without/procrecordnulland fall back to a plain liveness check rather than paying a subprocess per record.pidfield disagrees with its filename is skipped and left on disk — we cannot reason about what it describes, so we must not delete it.^\d+\.json$. A lenientparseIntprefix match would read2026-planning-notes.jsonas PID 2026, fail its liveness check, and delete a file this code never wrote.chmod 0700on every register, not just atmkdir—mkdir's mode is masked by the umask and is ignored entirely when the directory already exists, sochmodis what actually tightens a directory created by an older build. Records are0600.EPERMmeans alive. A process owned by another user exists; reporting it dead would let one user's session sweep another's record out of a shared directory./clear,/resumeand friends change the session id under a stable PID, sorefreshSessionIdpatches the record alongside the runtime.json refresh, under the same ownership rule. Without it the record points discovery at the previous transcript.To reproduce by hand: start two interactive sessions in different directories, run
qwen sessions psin a third, thenkill -9one of them and runqwen sessions psagain — the killed session's row disappears and its~/.qwen/sessions/<pid>.jsonis gone, swept by the next registry read.Unit tests:
Lint and format are clean on every changed file, and
tsc --noEmitreports no error in any of them.Evidence (Before & After)
qwen sessions psis a new command, so "before" is the absence of any way to ask the question — there is no prior output to show.Before — no such command; enumerating live sessions meant walking every project directory and reading one file per historical session, with no liveness signal in any of them.
After — two sessions running in different directories:
--jsonon the same state emits one JSON object per line for scripting.Tested on
Linux is where the command output above was captured and where the unit tests were run. macOS and Windows are exercised by CI only — see Risk & Scope for what that leaves unverified.
Environment (optional)
Local
npm run dev, Node 22, Linux. No sandbox.Risk & Scope
/proc/<pid>/stat, which exists on Linux only. On macOS and Windows the token is recorded asnulland liveness degrades to a plainkill(pid, 0)check, so a recycled PID can make a dead session look live until its record is overwritten. The alternative — spawningpsper record — costs a subprocess per lookup on the hot path, which is worse for a command whose whole point is being cheap.packages/core/src/config/config.test.tsdoes not load in my checkout —packages/core/node_modules/ajvis an empty directory even though core declaresajv@^8.17.1. It is pre-existing and unrelated:src/utils/schemaValidator.test.tsfails identically on a clean tree, and the same unresolved imports (ajv,fdir,ignore,mime/lite) appear innpm run typecheckon files this PR does not touch. Worth a second pair of eyes on CI for that file.isPidAlivemoving modules is internal; its semantics are unchanged.Linked Issues
Part of #8724. Does not close it — the socket transport, the inbound gate, and the sender/addressing changes are separate PRs in the same stack.
中文说明
#8724 的第一步。之所以单独提出来,是因为它本身就有用,而且完全不改变一个会话的行为——没有新的传输层,没有新工具,没有新的消息通路。
这个 PR 做了什么
每个交互式会话在运行期间把自己登记到
~/.qwen/sessions/<pid>.json,退出时删除该记录,于是"这台机器上现在有哪些 Qwen Code 会话在跑"就变成一次readdir加几次小文件读取。新增的qwen sessions ps命令读取这个索引并打印存活会话;--json按 JSON Lines 输出便于脚本处理,--all把调用方进程也算进去。同时把
isPidAlive从teamHelpers移到utils/process-liveness.ts(语义不变,现在有测试),避免注册表成为第三份拷贝。产品里目前还没有别的地方读这个注册表。只有传输层落地后,记录才会新增
ipcPath字段;本 PR 中没有任何代码读写它。为什么需要
#8724的所有功能都要先能枚举存活会话,而现有的任何索引都回答不了这个问题。<projectDir>/chats/<sessionId>.runtime.json确实记录了(pid, session_id, work_dir, …),但它回答的是另一个问题,无法承担这个职责:两者共存。
runtime.json继续作为稳定的、兼容 kimi 的"PID X 正在服务哪个会话"旁路文件供外部观察者使用;本注册表则是 CLI 自身功能的发现索引。审查者测试计划
如何验证
以下是审查时值得留意的正确性细节——每一条都有对应的具名测试,而且每一条都是"想当然的写法会出错"的地方:
/proc/<pid>/stat的starttime,解析时以最后一个)为锚点——进程名叫my ) proc是合法的,否则会让后面每个字段都错位。没有/proc的平台记为null,退化为单纯的存活检查,而不是为每条记录付一个子进程的代价。pid字段与文件名不一致的,一律跳过并保留在磁盘上——我们无法推断它描述的是什么,就不能删它。^\d+\.json$。 宽松的parseInt前缀匹配会把2026-planning-notes.json读成 PID 2026,存活检查失败,然后删掉一个本代码从未写过的文件。chmod 0700,而不只在mkdir时——mkdir的 mode 会被 umask 掩掉,且目录已存在时完全被忽略,所以真正能收紧旧版本所建目录权限的是chmod。记录文件为0600。EPERM意味着存活。 属于其他用户的进程是存在的;把它报成已死,会让一个用户的会话把另一个用户的记录从共享目录里清掉。/clear、/resume等会在 PID 不变的情况下更换 session id,因此refreshSessionId会在刷新 runtime.json 的同时按同样的归属规则修补记录。没有这一步,发现机制会指向上一份会话记录。手工复现:在不同目录下启动两个交互式会话,在第三个终端运行
qwen sessions ps,然后kill -9掉其中一个再运行一次——被杀会话那一行消失,其~/.qwen/sessions/<pid>.json也已被下一次注册表读取顺带清除。单元测试:
所有改动文件的 lint 与 format 均干净,
tsc --noEmit在这些文件上无报错。证据(前后对比)
qwen sessions ps是新命令,所以"之前"就是根本没有办法问这个问题——没有旧输出可以对照。之前——没有这个命令;枚举存活会话意味着遍历每个项目目录、为每个历史会话读一个文件,而且其中没有任何一个带存活信号。
之后——两个会话运行在不同目录下:
同样状态下加
--json会每行输出一个 JSON 对象,便于脚本处理。测试环境
上面的命令输出和单元测试都是在 Linux 上跑的。macOS 与 Windows 仅由 CI 覆盖——未验证的部分见"风险与范围"。
运行环境(可选)
本地
npm run dev,Node 22,Linux。未使用沙箱。风险与范围
/proc/<pid>/stat,而它只在 Linux 上存在。在 macOS 和 Windows 上该令牌记为null,存活判定退化为单纯的kill(pid, 0),因此被复用的 PID 可能让一个已死会话看起来还活着,直到它的记录被覆盖。另一种做法——为每条记录 spawn 一个ps——会在热路径上为每次查询付一个子进程的代价,对一个以"便宜"为全部意义的命令来说更糟。packages/core/src/config/config.test.ts在我的检出里加载失败——packages/core/node_modules/ajv是个空目录,尽管 core 声明了ajv@^8.17.1。这是既有问题且与本改动无关:src/utils/schemaValidator.test.ts在干净的工作树上以同样方式失败,且npm run typecheck在本 PR 未触碰的文件上报出同样的未解析导入(ajv、fdir、ignore、mime/lite)。这个文件值得在 CI 上再看一眼。isPidAlive换模块属于内部改动,语义未变。关联 Issue
隶属于 #8724。不关闭它——socket 传输层、入站门禁、以及发送方/寻址改动都是同一 stack 中的独立 PR。