Skip to content

feat(cli): list managed Agent View sessions in qwen sessions ps - #10942

Open
yiliang114 wants to merge 18 commits into
mainfrom
feat/agent-view-first-consumer
Open

feat(cli): list managed Agent View sessions in qwen sessions ps#10942
yiliang114 wants to merge 18 commits into
mainfrom
feat/agent-view-first-consumer

Conversation

@yiliang114

@yiliang114 yiliang114 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

qwen sessions ps now lists managed Agent View sessions beside the interactive ones it always listed, and says what each one is doing.

The command walked only the live-process registry, which cannot describe the richer lifecycle state kept by an Agent View supervisor. As a result, managed sessions without a live registry entry were invisible, while registered managed workers appeared only as interactive. This merges both sources into one table: managed sessions first, each labelled needs input, working, ready, stopped or failed, with interactive sessions below them labelled interactive. A session that is both managed and registered is listed once, as managed.

The merge and the labelling live in a new pure module (managed-rows.ts) so they are testable without a filesystem or a running supervisor; the command keeps the readers.

Why it's needed

packages/cli/src/agent-view/ is 11,004 lines of production code with no consumer. The supervisor runtime (#7799), the PTY workers (#7800) and the session lifecycle (#7801, re-landed as #9986) are all merged; the two PRs that would give them an entry point — #7802 (commands) and #7803 (roster TUI) — have been open since 2026-07-27, the second at +31,699 lines across 104 files and currently conflicting. Nothing calls the subsystem, so nothing it knows can reach a user, and it has grown by roughly 8,000 lines in the four weeks since it was last measured.

This gives it its first consumer, at the smallest surface that carries real information. It is deliberately not a new command: it neither competes with #7802/#7803 nor waits for them, and sessions ps is already the place a user asks "what is running right now". listAgentViewSessionSnapshots and deriveAgentViewPresentation now have a caller outside their own directory.

Three decisions worth reviewing rather than skimming:

  • Rows are labelled by task state, not by the roster's display group. The group folds ready, stopped and failed into one completed bucket, which the roster UI can afford because it also paints an icon tone. A one-line table has no second channel, and printing "completed" beside a session that failed is a lie the user has no way to see through.
  • The name comes from deriveAgentViewPresentation, so this listing and the roster cannot drift into describing one session two different ways. Its Untitled session placeholder is the single override — the roster can afford identical rows because a user arrows onto one, while here the session id is the only thing that tells two of them apart. The placeholder is now an exported constant rather than a literal duplicated across two files; that is the only change to presentation.ts.
  • A supervisor store that cannot be read degrades to the registry half and says so on stderr. Silently omitting a session that needs input is the failure this command exists to prevent, and stderr keeps --json stdout parseable.

Reviewer Test Plan

How to verify

Unit level, from packages/cli: npx vitest run src/commands/sessions/ src/agent-view/ --coverage.enabled=false → 17 files, 364 tests passing. 12 new cases in managed-rows.test.ts cover the state labels (including that a failed session is never reported as ready), the worker-then-host pid fallback, a missing pid staying absent rather than becoming 0, the title precedence and its id fallback, an unparseable createdAt, the managed-first ordering, the dedupe, and two records sharing one session id. 8 new cases in ps.test.ts cover a managed session the registry cannot see, the ordering in the rendered table, the managed discriminator in --json, the - placeholders, and the three properties of a store failure: interactive rows still listed, stdout still parseable, and the reason sanitized before it reaches the terminal.

eslint and prettier --check are clean on every changed file.

Behaviourally, with no supervisor running the output is unchanged except for the new STATE column reading interactive, and --json gaining managed: false. To see the managed half before #7802 lands a dispatcher, write a state file by hand under $QWEN_HOME/jobs/<id>/ (sessionState: "needs_input", ownership: "managed") with a roster entry in $QWEN_HOME/daemon/roster.json, and run qwen sessions ps.

Evidence (Before & After)

Before:

NAME                  PID      AGE       DIRECTORY
app-ab                4242     1m        /w/app

After, with one managed session waiting for an answer:

NAME                  PID      AGE       STATE        DIRECTORY
svc-audit             777      5s        needs input  /w/svc
app-ab                4242     1m        interactive  /w/app

Both are the shapes the unit tests pin; the After row is not a live capture, because nothing dispatches a managed session until #7802 lands.

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux ⚠️

Unit tests only, on Linux. npx tsc --noEmit and npm run build were not run: the machine this was written on cannot complete either. Types and build need CI or a second machine.

Environment (optional)

Linux, vitest only.

Risk & Scope

  • Main risk or tradeoff: the --json shape changes. Interactive rows gain managed: false — additive for a jq consumer selecting fields, breaking for one comparing whole objects. The alternative (marking only managed rows, so absence means interactive) is less work for us and more work for every consumer, so the discriminator is on both kinds. The table also gains a column, which shifts DIRECTORY right; the existing column-offset test is updated rather than removed.
  • Not validated / out of scope: typecheck and build (see above). The managed half of the listing is empty in practice until something dispatches a managed session — this PR adds a reader, not a writer. Nothing here attaches to, controls or stops a session.
  • Breaking changes / migration notes: none for the human-readable output beyond the new column. For --json, see the risk above; the documented field lists are updated.

Linked Issues

Gives a first consumer to the subsystem merged by #7799, #7800 and #7801/#9986. Related to #7802 and #7803, which it neither blocks nor depends on.

中文说明

这个 PR 做了什么

qwen sessions ps 现在会把 Agent View 的 managed session 和它一直在列的 interactive session 一起列出,并说明每一个正在做什么。

这个命令此前只走 live-process registry,无法描述 Agent View supervisor 保存的更丰富生命周期状态。因此,没有存活注册记录的 managed session 完全不可见;已经注册的 managed worker 也只会被标成 interactive。本 PR 把两个来源合并成一张表:managed 排在前面,各自标注 needs inputworkingreadystoppedfailed;interactive 排在下面,标注 interactive。同时既是 managed 又已注册的 session 只列一次,按 managed 列。

合并与标注逻辑放在一个新的纯函数模块(managed-rows.ts)里,因此无需文件系统或运行中的 supervisor 即可测试;读取仍留在命令里。

为什么需要

packages/cli/src/agent-view/ 有 11,004 行生产代码,却没有任何消费者。supervisor runtime(#7799)、PTY workers(#7800)、session lifecycle(#7801,由 #9986 重新落地)都已合并;而能给它们提供入口的两个 PR —— #7802(命令)和 #7803(roster TUI)—— 自 2026-07-27 起一直开着,后者 +31,699 行、104 个文件,且当前处于冲突状态。没有任何代码调用这个子系统,因此它掌握的信息也就无法抵达用户;距上次测量后的四周里,它又长了约 8,000 行。

本 PR 以「能承载真实信息的最小表面」给了它第一个消费者。刻意不新增命令:既不与 #7802/#7803 抢位,也不等待它们,而 sessions ps 本来就是用户问「现在有什么在跑」的地方。listAgentViewSessionSnapshotsderiveAgentViewPresentation 现在终于有了目录外的调用者。

有三处判断值得细看而非略过:

  • 按 task state 标注,而不是按 roster 的展示分组。 分组把 readystoppedfailed 折叠进同一个 completed 桶,roster UI 之所以负担得起,是因为它还会绘制图标色调。单行表格没有第二条通道,在一个失败的 session 旁边打上「completed」,是用户无从看穿的谎言。
  • 名字来自 deriveAgentViewPresentation,因此本列表与 roster 不会对同一个 session 给出两种说法。它的 Untitled session 占位符是唯一的例外:roster 可以容忍完全相同的行,因为用户可以用方向键选中其中一行;而在这里,session id 是唯一能区分两者的东西。该占位符现已改为导出常量,而不是在两个文件里各写一份字面量 —— 这也是本 PR 对 presentation.ts 的唯一改动。
  • supervisor 存储读不出来时,降级为只列注册表那一半,并在 stderr 说明原因。 静默漏掉一个正在等待输入的 session,恰恰是这个命令要防止的失败;走 stderr 则能让 --json 的 stdout 保持可解析。

评审者测试计划

如何验证

单测,在 packages/cli 下:npx vitest run src/commands/sessions/ src/agent-view/ --coverage.enabled=false → 17 个文件、364 个测试通过。managed-rows.test.ts 中 12 个新用例覆盖状态标签(含「失败的 session 绝不会被报成 ready」)、worker 优先其次 host 的 pid 回退、缺失 pid 保持缺失而不变成 0、标题优先级及其 id 回退、无法解析的 createdAt、managed 优先的排序、去重,以及两条记录共用同一个 session id 的情形。ps.test.ts 中 8 个新用例覆盖注册表看不见的 managed session、渲染表格中的排序、--json 里的 managed 判别字段、- 占位符,以及存储读取失败的三条性质:interactive 行仍然列出、stdout 仍可解析、原因在抵达终端前已被净化。

所有改动文件的 eslintprettier --check 均干净。

行为上,在没有 supervisor 运行时,输出与此前一致,只多出 STATE 列显示 interactive--json 多出 managed: false。若想在 #7802 落地调度器之前看到 managed 那一半,可手工在 $QWEN_HOME/jobs/<id>/ 下写一个 state 文件(sessionState: "needs_input"ownership: "managed"),并在 $QWEN_HOME/daemon/roster.json 里写一条 roster entry,然后运行 qwen sessions ps

证据(前后对比)

之前:

NAME                  PID      AGE       DIRECTORY
app-ab                4242     1m        /w/app

之后,含一个正在等待回答的 managed session:

NAME                  PID      AGE       STATE        DIRECTORY
svc-audit             777      5s        needs input  /w/svc
app-ab                4242     1m        interactive  /w/app

两者都是单测所钉住的形状;之后那一行不是实时截取,因为在 #7802 落地前没有任何东西会调度出 managed session。

测试环境

操作系统 状态
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux ⚠️

仅 Linux 上的单元测试。npx tsc --noEmitnpm run build 运行:撰写本 PR 的机器无法完成其中任何一个。类型检查与构建需要 CI 或另一台机器。

运行环境(可选)

Linux,仅 vitest。

风险与范围

  • 主要风险或取舍: --json 的形状发生变化。interactive 行新增 managed: false —— 对按字段取值的 jq 消费者是增量变更,对整对象比较的消费者则是破坏性变更。另一种做法(只标记 managed 行,以「缺失即 interactive」)对我们更省事、对每个消费者更费事,因此判别字段两种行都带。表格也多了一列,DIRECTORY 会右移;现有的列偏移测试是被更新而非删除。
  • 未验证 / 范围之外: 类型检查与构建(见上)。在有东西真正调度出 managed session 之前,列表的 managed 那一半在实际使用中是空的 —— 本 PR 加的是读取方,不是写入方。这里没有任何附着、控制或停止 session 的能力。
  • 破坏性变更 / 迁移说明: 人类可读输出除新增一列外无破坏性变更。--json 见上述风险;文档中的字段列表已同步更新。

关联 Issue

#7799#7800#7801/#9986 合并的子系统提供了第一个消费者。与 #7802#7803 相关,但既不阻塞它们,也不依赖它们。

`packages/cli/src/agent-view/` is 11,004 production lines with no
consumer: the supervisor, the PTY workers and the session lifecycle all
merged (#7799, #7800, #7801/#9986), while the two PRs that would give
them an entry point are still open (#7802 commands, #7803 roster TUI,
both since 2026-07-27). Nothing calls the subsystem, so nothing that
subsystem knows can reach a user.

This gives it its first one, at the smallest surface that carries real
information: `qwen sessions ps` now lists managed sessions beside the
interactive ones it always listed.

The command walked only the live-process registry, which a supervisor's
worker never writes — so a background session waiting for an answer was
invisible to every listing. `managed-rows.ts` merges both sources into
one row shape, deduplicating by session id (a managed worker can also
register, and would otherwise be listed twice: once as `interactive`,
once with its real state). Managed rows come first, because a session
waiting on input is the usual reason to run this.

Three decisions worth naming:

- Rows are labelled by task state, not by the roster's display group.
  The group folds `ready`, `stopped` and `failed` into one `completed`
  bucket, which the roster UI can afford because it also paints an icon
  tone; a one-line table has no second channel, and printing "completed"
  beside a session that failed is a lie the user cannot see through.
- The name comes from `deriveAgentViewPresentation`, so this listing and
  the roster cannot drift into describing one session two ways. Its
  `Untitled session` placeholder is the single override — the roster can
  afford identical rows because a user arrows onto one; here the session
  id is the only thing that tells two apart. The placeholder is now
  exported rather than duplicated as a literal.
- A supervisor store that cannot be read degrades to the registry half
  and says so on stderr. Silently omitting a session that needs input is
  the failure this command exists to prevent, and stderr keeps `--json`
  stdout parseable.

The table gains a STATE column; `--json` gains a `managed` discriminator
on both row kinds, and carries each registry record through verbatim
rather than looking it back up by session id — two records can share one
(a stale writer, a restored transcript), which would emit one twice and
drop the other.
The find-simplifications survey uses `packages/cli/src/agent-view` as its
worked example for "everything says dead, git says five days old": a
feature mid-wiring, so drop it silently — do not file it, do not mention
it.

The deletion verdict was right. The silence was not. Measured again on
2026-09-04, one month after that example was written: 11,004 production
lines, still zero external references, up ~8,000 in four weeks. Its five
merged PRs shipped the supervisor, the PTY workers and the lifecycle;
the two that would give it an entry point have been open since
2026-07-27, the second at +31,699 lines across 104 files and
conflicting. The gate kept firing on the directory's recency and the
survey kept dropping it, so nothing ever said out loud that a
five-figure subsystem had no caller.

"Young" is not the fact the gate is reaching for — "still being wired
up" is, and for a subsystem the two come apart. The gate now says to
date the wiring rather than the code when it fires on a subsystem, and
names the outcome: recent commits inside, no external reference, and an
open stalled wiring PR is neither a deletion nor a drop. It is a
different finding with a different owner, owed to that PR stack.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Re-run at 79ca2fc, five commits on from the pass that requested changes. Short version: the Critical from that pass is fixed and — more importantly — pinned by tests, so this pass is looking at a materially better PR. Two new correctness problems have surfaced in the code that landed in response to review, and they are written up in the code-review comment.

Template looks good ✓

Problem: the premise still holds, and I re-checked it at current main rather than trusting the last pass. Nothing outside packages/cli/src/agent-view/ imports it — the only agent-view/ importers in the tree are ui/layouts/DefaultAppLayout.tsx and its test, and those point at the unrelated ui/components/agent-view/ React components, not this subsystem. So the "five figures of production code, zero consumers" claim is accurate as of today, and this PR really would be the first.

What is still unverifiable is the user-facing half, for the same reason as last time: nothing dispatches a managed session until #7802 or #7803 lands, so there is no live capture to take. The After block in the description says so plainly. The only behaviour a user can observe today is the new STATE column reading interactive and managed: false on every --json line.

Direction: my reservation from the last pass is unchanged, and I am recording it rather than repeating it as a blocker. This adds a reader to a subsystem that still has no writer, while changing the --json contract of a command that shipped in #8969 — cost now, benefit when a different PR lands. Whether that sequencing is worth it is a maintainer's call, and you have admin on this repo, so it is yours to make. I would not block on it and I am not.

Size: not applicable — no core paths. Nothing here matches packages/core/src/**, auth/, providers/, models/, config/, tools/ or services/, and the change stays inside one package. Breakdown: 558 production lines (managed-rows.ts 282, ps.ts 146, supervisor-process.ts 50, protocol.ts 46, supervisor-store.ts 23, presentation.ts 11), 855 test lines, 116 docs/skill markdown. Under every threshold, including the 1000-line advisory.

Approach: the shape is right and I would not change it — merge and labelling in a pure module, readers left in the command, and reusing deriveAgentViewPresentation so this listing and the roster cannot drift into two vocabularies. Two scope notes:

  • The diff has grown a third concern since the last pass. It is now (a) the sessions ps reader, (b) a writer-side change to the supervisor — AgentViewWorkerFile gains hostProcStart, workerProcStart and pidNs, and four call sites in supervisor-process.ts now record them — and (c) 48 lines of .qwen/skills/find-simplifications/ docs. (b) is defensible inside this PR: a listing that prints a pid a user may kill needs the identity tokens to print it honestly, and it mirrors what the registry already does. But it does change what the supervisor writes to a durable schemaVersion: 1 file, which is a bigger deal than the PR title suggests, and it is not mentioned in the description at all. Worth a sentence in the body either way.
  • (c) I flagged last round and it is still here, still unmentioned anywhere in the PR body. Different change, different reader. Splitting it out is still the right call.

Risk: no elevated risk signals — none of the changed files match the revert-correlated path set. The real risk is in the merge logic, and it is a correctness defect rather than a path signal; see the code review.

Moving on to code review. 🔍

中文说明

79ca2fc 上重跑 —— 距上次请求修改已过去五个 commit。简短结论:那一轮的 Critical 已修复,而且更重要的是已被测试钉住,所以本轮面对的是一个明显更好的 PR。但作为回应评审而新落地的代码里出现了两个新的正确性问题,详见代码审查评论。

模板完整 ✓

问题: 前提依然成立,而且我是在当前 main 上重新核实的,没有照搬上一轮的结论。packages/cli/src/agent-view/ 之外没有任何文件 import 它 —— 树中唯一的 agent-view/ importer 是 ui/layouts/DefaultAppLayout.tsx 及其测试,而它们指向的是无关的 ui/components/agent-view/ React 组件,不是这个子系统。所以「五位数生产代码、零消费者」的说法截至今天依然准确,本 PR 确实会是第一个消费者。

仍然无法核实的是用户可见的那一半,原因与上次相同:在 #7802#7803 落地之前没有任何东西会调度出 managed session,因此没有实时截取可拿。描述中的 之后 代码块也明确这么说了。今天用户能观察到的唯一行为变化,就是新的 STATE 列显示 interactive,以及每条 --json 多出 managed: false

方向: 上一轮的保留意见没有变,我把它记录下来,而不是当作阻塞项重复一遍。本 PR 给一个仍然没有写入方的子系统加了读取方,同时改变了一个已在 #8969 发布的命令的 --json 契约 —— 成本在现在,收益要等另一个 PR 落地。这个时序是否值得,是维护者的判断;你在本仓库有 admin 权限,所以这个判断权在你手上。我不会以此阻塞,也没有。

规模: 不适用 —— 未触及核心路径。没有任何文件命中 packages/core/src/**auth/providers/models/config/tools/services/,改动全部留在单个 package 内。拆分:生产代码 558 行(managed-rows.ts 282、ps.ts 146、supervisor-process.ts 50、protocol.ts 46、supervisor-store.ts 23、presentation.ts 11),测试 855 行,文档/skill markdown 116 行。低于所有阈值,包括 1000 行大 PR 建议线。

方案: 整体结构是对的,我不会去改它 —— 合并与标注放在纯函数模块、读取留在命令里、复用 deriveAgentViewPresentation 以免本列表与 roster 出现两套说法。两点范围意见:

  • 自上一轮起,diff 长出了第三个关注点。现在是:(a) sessions ps 读取方;(b) supervisor 的写入侧改动 —— AgentViewWorkerFile 新增 hostProcStartworkerProcStartpidNssupervisor-process.ts 四处调用点开始记录它们;(c) .qwen/skills/find-simplifications/ 下 48 行文档。(b) 放在本 PR 里是站得住的:一个会打印出用户可能拿去 kill 的 pid 的列表,需要这些身份令牌才能诚实地打印它,而且它与注册表已有的做法一致。但它确实改变了 supervisor 写入一个持久化 schemaVersion: 1 文件的内容,这比 PR 标题所暗示的分量更重,而描述里对此一字未提。无论如何值得在正文补一句。
  • (c) 我上一轮就提过,它仍然在这里,仍然没有在 PR 正文任何位置被提到。不同的改动、不同的读者。拆出去仍然是对的。

风险: 无升级风险信号 —— 改动文件均未命中与 revert 相关的路径集合。真实风险在合并逻辑本身,属于正确性缺陷而非路径信号;见代码审查。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 79ca2fc451cd6c0148d51eb49e363eff656865d9 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Code review

My independent proposal first, written from the title and the "Why it's needed" section before reading the diff: read the agent-view snapshots inside ps.ts, filter to ownership === 'managed' the way the supervisor's own listing does, map to a row, concatenate ahead of the registry rows, dedupe by session id preferring whichever side knows a live pid, keep the existing columns, and add a discriminator only on the new row kind. About forty lines, no new module.

The PR matches that shape and exceeds it on testability — the pure module is a better call than what I sketched, and the 30-odd new cases are real cases. I would not ask for that to change. It also now exceeds my proposal on pid honesty: I would have printed worker.workerPid behind a bare liveness probe, and the identity-token path it took instead is the correct one. I checked that claim rather than accepting it, and it holds — see "what I checked" below.

The previous pass's Critical is fixed and pinned. managedSessionRows now filters ownership === 'managed', and the tests do exactly what I asked for: one case iterates all three non-managed values asserting no rows, and lets a live registry record survive the adopting window asserts the registry row keeps pid 4242. The --json Suggestion is fixed too — taskState carries the stable token and the display wording lives only in TASK_STATE_LABEL at the render site. Both good.

What follows are two defects in the code that landed after that fix. I verified both against 79ca2fc line by line rather than taking the review thread's word for it.

1. Critical — mergeSessionRows drops the registry record even when the managed row is the degraded half

mergeSessionRows builds managedIds from the managed rows and filters out every record that collides, unconditionally. The managed row always wins. That is the right rule when the managed row knows more than the record — and the wrong rule when it knows less, which the store's own fail-soft behaviour makes reachable.

readJsonRecord (supervisor-store.ts:570-583) returns undefined on any read or parse error, deliberately: "Fail soft on any read or parse error: one corrupt or odd entry under jobs/ must not poison a full listing." So a transient EMFILE, EIO or a truncated file drops launch or worker from an otherwise live, owned snapshot. Two shapes follow, and in both the dropped record was the only other carrier of what the row lost:

(a) The session gets listed under an id that cannot resume it. state.sessionId is the sanitized directory namereadAgentViewSessionState returns normalizeSessionState(raw, path.basename(paths.sessionDir)) (:252), and the directory is sanitizeSessionId(sessionId) (:70). The raw, resumable spelling lives in launch.resumeSessionId, and adoption preserves it precisely because the native session store is case-sensitive. With the launch file transiently unreadable, snapshot.launch?.resumeSessionId ?? snapshot.state.sessionId falls back to the lowercased id; the merge then drops the record that still carries the real spelling. qwen sessions ps reports managed-1 for a session whose id is Managed-1 — a possibly waiting-for-input session listed under an id no consumer can act on.

The suite does not just miss this, it asserts it as correct: lists a mixed-case session once, though the two sources spell it differently builds a snapshot with no launch file plus a record {sessionId: 'Managed-1'} and expects rows[0].sessionId to be 'managed-1'. The sibling case that supplies launch: launchFile({resumeSessionId: 'Managed-1'}) expects 'Managed-1'. So the happy path is pinned and the degraded path is pinned lossy.

(b) A pid the registry just vouched for disappears. With the worker file unreadable, liveWorkerPid(undefined) returns undefined, and the merge drops the record whose pid listLiveSessions verified under the same identity contract the PR spent 100 lines matching. The table prints - and --json omits pid for a session the interactive half of the same listing confirmed alive. lists a session once when it is both managed and registered merges exactly this shape — a worker-less snapshot against a record with pid 4242 — and asserts length, managed and taskState, never pid.

The fix is a carry-over rather than a preference rule: when a record dedupes against a managed row, carry what the degraded row lacks onto the survivor — the record's raw sessionId when it sanitizes onto the row's reported id, and the record's verified pid when the row has none. Both spellings must still collapse to exactly one row, the launch-file spelling must still win when present, and the adopting-window case must stay green. The lossy expectation in the mixed-case test has to flip.

This is already reported as an unresolved inline thread (R5-3) at this commit, and it is the reason the PR currently reads CHANGES_REQUESTED. I am confirming it still stands, not re-filing it.

2. Critical — resumeSessionId is the one launch field nobody validates, and it crashes the whole listing

Finding 1's fallback made launch.resumeSessionId load-bearing for the first time. It is also the one field normalizeLaunch does not check.

normalizeLaunch (supervisor-store.ts:793-818) runs stringValue() over sessionId, entrypoint, projectCwd, activeCwd and initialPromptstringValue returns undefined for anything that is not a non-empty string (:1026-1028) — and then spreads ...raw into the result. resumeSessionId is never passed through it, so a launch.json holding "resumeSessionId": 123 survives normalization typed string and carrying a number. protocol.ts:77 declares resumeSessionId?: string, so nothing downstream suspects otherwise.

Then mergeSessionRows calls sanitizeSessionId(row.sessionId), whose first operation is sessionId.replace(/\\/g, '/') — a TypeError on a non-string.

The throw lands outside every guard the command has. readManagedRows' try/catch wraps listAgentViewSessionSnapshots and managedSessionRows, but mergeSessionRows(records, managedResult.rows) is called in handlePs after the Promise.all, and handler: async (argv) => { await handlePs(argv) } has no catch of its own. One unreadable-typed field in one managed session's launch file therefore takes down qwen sessions ps entirely — including the interactive half, which is the exact failure the module docstring says must not happen ("A supervisor store that cannot be read must not take the command down — the registry half still answers the question").

Worth being precise about why this is not merely hypothetical hardening: normalizeLaunch already treats a hostile launch file as in-scope — its own comment reads "a tampered launch.json cannot impersonate another session", and it validates every field it consumes for exactly that reason. This PR newly consumes a field that validation never covered. No test constructs a non-string resumeSessionId; every occurrence in the test file is the literal 'Managed-1'.

The one-line fix is resumeSessionId: stringValue(raw['resumeSessionId']) in normalizeLaunch, beside the fields it already guards. A typeof === 'string' check at the managed-rows.ts fallback would work too, but fixing it where every other launch field is fixed is the better home.

The /review round recorded this one as R4-1 and then deferred it as "fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect". I would push back on that framing: the merge base had a sessions ps that never read the supervisor store at all, so no launch file could affect it. A crash that also removes the interactive listing is a regression against today's behaviour, not a fail-closed gap on a surface that did not exist. It is small and cheap, and I would fix it in this PR rather than carry it as follow-up work.

sequenceDiagram
    participant P1 as qwen sessions ps
    participant P2 as live-process registry
    participant P3 as supervisor store
    participant P4 as mergeSessionRows
    participant P5 as stdout
    P1->>P2: listLiveSessions, pids verified
    P1->>P3: listAgentViewSessionSnapshots
    P3-->>P1: snapshots, all four ownership values
    P1->>P1: keep only ownership managed
    Note over P1,P3: a transient read error drops launch or worker from a live snapshot
    P1->>P4: records plus managed rows
    P4->>P4: dedupe on sanitizeSessionId
    Note over P4: a colliding record is dropped whole - both Criticals live here
    P4-->>P5: managed rows first, then interactive
Loading
Files changed (11)
File What changed
packages/cli/src/commands/sessions/managed-rows.ts New pure module: row shape, the ownership filter, pid-identity resolution, and the merge. Both Criticals are here.
packages/cli/src/commands/sessions/managed-rows.test.ts New, 591 lines. Thorough on pid identity; pins the degraded-merge outcome as correct.
packages/cli/src/commands/sessions/ps.ts Second reader, STATE column, taskState-to-label map, stderr degradation.
packages/cli/src/commands/sessions/ps.test.ts 8 new cases for ordering, the JSON discriminator, placeholders and store failure.
packages/cli/src/agent-view/protocol.ts Three new optional worker-file identity fields; sanitizeSessionId moved here.
packages/cli/src/agent-view/supervisor-process.ts New workerPidIdentity helper, applied at four worker-file write sites.
packages/cli/src/agent-view/supervisor-store.ts Normalizes the three new fields; re-exports the moved sanitizer.
packages/cli/src/agent-view/presentation.ts Untitled session literal becomes an exported constant. Nothing else.
docs/users/features/commands.md Both JSON row shapes, the STATE column, the degradation contract, a new jq example.
.qwen/skills/find-simplifications/references/survey.md Unrelated: teaches the survey to report stalled wiring instead of dropping it.
.qwen/skills/find-simplifications/SKILL.md Unrelated: one table row for the same rule.

What I checked that is fine

Recorded because several of these are the kind of thing that looks wrong at a glance, and because the PR makes claims in comments that are worth holding to:

  • The pid-identity mirroring claim is literally true. liveWorkerPid's boot-id guard — recordBootId !== null && recordBootId !== ownBootId → skip — is the same shape as listLiveSessions' at session-registry.ts:553-557, including firing when the local boot id is unreadable, which is the subtle part the registry's own comment explains. The namespace guard deliberately diverges (fires only when both sides are known) and says so; that is the right call here, because this declines to print a pid where the registry declines to list at all.
  • The imports resolve. isSameProcess, readLocalBootId, readPidNamespaceId and readProcStartToken all come from packages/core/src/utils/process-liveness.ts, re-exported by packages/core/src/index.ts:703. isSameProcess's real degradation contract matches both the module comment and the test mock.
  • The test mock is honest. It re-implements isSameProcess's fall-through rules instead of stubbing a verdict, so a token-less case exercises the same path production does. That is better than most mocks of this shape.
  • "Both readers already sort newest first" is accurate. listAgentViewSessionSnapshots ends in snapshots.sort((l, r) => r.state.updatedAt.localeCompare(l.state.updatedAt)) (:387-389).
  • The sanitizeSessionId move is safe. All three consumer sites still resolve: the store's own 11 internal uses, supervisor-process.ts:60 via the re-export, and the new module importing from protocol.js. No cycle — protocol.ts only gained a node:path import.
  • deriveAgentViewPresentation is called correctly. The parameter is a union and only the input half carries now; passing field by field rather than spreading is right, and the comment explaining why is accurate.
  • Docs match the code. Both --json row shapes are documented, including that a session in both sources emits the row shape rather than the record. The jq -r 'select(.taskState == "waiting")' example is correct — interactive rows have no taskState, so select drops them.
  • The stderr path is sanitized, and stateLabel can only ever return one of six fixed literals, so the new column cannot carry foreign text into the terminal.

Test evidence

Unattended CI run — I did not build or execute anything from this PR. Everything below is the PR's own CI, read from the check-runs API for 79ca2fc. CI has settled on this commit: all three pull_request workflow runs are completed, so nothing is pending and no result is a guess.

Final CI results for 79ca2fc (auto-updated by the triage finalize job after CI completed):

Check Conclusion
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ❌ failure
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
OpenTUI no-flicker gate ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
TUI parity snapshots (ink vs opentui) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Skipped, so they are evidence of nothing either way: Test (macos-latest), Test (windows-latest), Integration Tests (CLI, No Sandbox).

The one red check is a stale base, not this diff. The failing step is Run transcript document browser gate, and its whole output is:

No test files found, exiting with code 1
filter: chat-transcript-document.test.ts
include: **/*.test.ts

That is a collection failure, not an assertion failure — vitest could not find the file. Three facts pin the cause:

  1. The job checks out the PR head only: the log records ref: refs/pull/10942/head and expected_sha: 79ca2fc..., not a merge commit against current main.
  2. Both the gate step and its test file landed on main together in 74fe3a6 (test(web-shell): render the Session Workflow cockpit in the visuals preview #11014, 2026-09-04 18:33Z) — git log --diff-filter=A on integration-tests/chat-transcript-document.test.ts returns exactly that commit, and git log -S'Run transcript document browser gate' on .github/workflows/ci.yml returns the same one.
  3. integration-tests/chat-transcript-document.test.ts does not exist at 79ca2fc — the contents API 404s for that ref while returning the file for ref=main.

So the workflow definition this run used (from the base) contains a step whose test file only exists on a base the branch has not merged. The PR touches nothing under integration-tests/, packages/web-shell/ or .github/workflows/. This also explains why the same check was ✅ on the earlier commit 8879852: #11014 had not landed yet. Merging or rebasing onto current main clears it — no code change needed, and it needs to happen before branch protection will let this through.

The typecheck gap the description discloses is closed by CI. The Tested-on section says npx tsc --noEmit and npm run build were not run locally, and that types and build "need CI or a second machine". They got it: the Install dependencies step runs npm ci, which fires preparenode scripts/prepare.jsnpm run build (prepare.js:29) → per-package node ../../scripts/build_package.jsexecSync('tsc --build') (build_package.js:38), and QWEN_SKIP_PREPARE is not set anywhere in ci.yml. Both green Test (ubuntu-latest) and Lint & Static therefore compiled packages/cli with tsc before running. That caveat in the PR body can be retired. Note that Lint & Static itself is ESLint + Prettier + actionlint + shellcheck — no tsc --noEmit; the typecheck signal comes from the install-time build, not from that job's name.

Not verified, and why: the 364 tests passing figure is the author's claim from a vitest-only run on Linux — I did not re-run it, and per the round-5 review the full suite observed 28373 passed against it. No live terminal capture of the managed half exists, because nothing dispatches a managed session yet. And neither Critical above is reachable by the suite as written: finding 1's degraded shapes are either pinned as correct or merged without a pid assertion, and finding 2's trigger is never constructed.

Sandboxed verification would settle what CI cannot: @qwen-code /verify — the central claim is behavioural (a managed session appears in the rendered table, ahead of interactive rows, with its real state) and the After block is explicitly not a live capture. More specifically, an A/B against the base build is what would demonstrate the two findings: hand-write an ownership: "managed" state file sharing a session id with a live registry record, truncate its worker.json, and confirm the merged row loses pid 4242; then put "resumeSessionId": 123 in a launch.json and confirm qwen sessions ps exits non-zero with no interactive rows either. @qwen-code /tmux would cover the table surface itself. You have admin on this repo, so neither needs a sponsor.

中文说明

代码审查(要点)

先说我的独立方案:在 ps.ts 内部读 agent-view 快照,按 supervisor 自己的做法过滤 ownership === 'managed',映射成行,排在注册表行之前,按 session id 去重并优先保留知道真实 pid 的一侧,保留现有列,只在新行上加判别字段。约四十行,不新增模块。

本 PR 与之一致,并在可测试性上超过它(纯函数模块、30 多个真实用例),在 pid 诚实性上也超过它 —— 我原本只会用一个裸存活探测,它走了身份令牌的路,这是对的。我核实了这个主张,成立(见下方「核对过没问题」)。

上一轮的 Critical 已修复且已钉住ownership === 'managed' 过滤到位;测试按我的要求补了一个遍历三种非 managed 值断言「不产生行」的用例,以及 lets a live registry record survive the adopting window 断言注册表行保住 pid 4242。--json 那条 Suggestion 也修了 —— taskState 承载稳定标识,展示措辞只留在渲染处的 TASK_STATE_LABEL

以下两个缺陷出现在那次修复之后落地的代码里。两者我都对着 79ca2fc 逐行核实,没有只采信评审线程。

1. Critical —— managed 行是降级的一方时,mergeSessionRows 仍会丢弃注册表记录

去重是无条件的,managed 行永远赢。当 managed 行知道得更少时这个规则就错了,而存储自身的软失败行为让这种情况可达:readJsonRecordsupervisor-store.ts:570-583)在任何读取或解析错误下返回 undefined(注释明确写了「Fail soft on any read or parse error」)。于是一次瞬时 EMFILE/EIO 或一个被截断的文件,就会让一个仍然存活且被拥有的快照丢掉 launchworker

(a) session 会被列在一个无法用于 resume 的 id 下。 state.sessionId净化后的目录名readAgentViewSessionState 返回 normalizeSessionState(raw, path.basename(paths.sessionDir)),目录名来自 sanitizeSessionId);可 resume 的原始拼写在 launch.resumeSessionId 里,收养之所以保留它,正因为原生 session 存储大小写敏感。launch 文件暂时不可读时,回退到小写 id,而合并又丢掉了仍携带真实拼写的记录 —— 一个可能正在等待输入的 session,被列在任何消费者都无法操作的 id 下。

测试不只是漏掉了它,而是把它断言成了正确行为lists a mixed-case session once... 构造无 launch 文件的快照 + {sessionId: 'Managed-1'} 的记录,期望 rows[0].sessionId'managed-1'

(b) 注册表刚刚担保过的 pid 消失了。 worker 文件不可读时 liveWorkerPid(undefined) 返回 undefined,合并丢掉那条 pid 已被 listLiveSessions 用同一身份契约验证过的记录 —— 表格打印 ---json 省略 pidlists a session once when it is both managed and registered 正好合并了这个形态(无 worker 快照 + pid 4242 的记录),却只断言了长度、managedtaskState,从未断言 pid

修复方向是「携带」而非「优先」:记录与 managed 行去重时,把降级行缺失的信息带到存活行上 —— 记录的原始 sessionId(当它净化后与行报告的 id 相撞时),以及行没有 pid 时记录已验证的 pid。两种拼写仍须折叠为恰好一行,launch 拼写在存在时仍须优先,收养窗口用例仍须为绿;混合大小写用例中那个有损断言需要翻转。

该问题已作为未解决的行内线程(R5-3)存在于本 commit,也是 PR 当前显示 CHANGES_REQUESTED 的原因。我是在确认它仍然成立,不是重新提一遍。

2. Critical —— resumeSessionId 是唯一没人校验的 launch 字段,而它会让整个列表崩掉

第 1 条的回退让 launch.resumeSessionId 第一次成为承重字段,而它恰好是 normalizeLaunch 唯一没检查的字段。normalizeLaunch:793-818)对 sessionIdentrypointprojectCwdactiveCwdinitialPrompt 都过了 stringValue()(非空字符串以外一律返回 undefined),然后 ...raw 展开 —— resumeSessionId 从未经过它。于是 "resumeSessionId": 123 会以「类型为 string、实际为 number」的形态活下来,而 protocol.ts:77 声明的就是 resumeSessionId?: string

随后 mergeSessionRows 调用 sanitizeSessionId(row.sessionId),其第一步是 sessionId.replace(...) —— 对非字符串抛 TypeError

这个抛出落在命令所有防护之外:readManagedRows 的 try/catch 包住的是 listAgentViewSessionSnapshotsmanagedSessionRows,而 mergeSessionRows 是在 handlePsPromise.all 之后调用的,handler 自身也没有 catch。因此一个 managed session 的 launch 文件里一个类型不对的字段,会让 qwen sessions ps 整体失败 —— 包括 interactive 那一半,而这正是模块文档字符串声明绝不能发生的失败。

需要说清它为什么不是纯理论加固:normalizeLaunch 自己就把敌意 launch 文件当作范围内威胁(注释写着「a tampered launch.json cannot impersonate another session」,并为此校验了它消费的每个字段)。本 PR 新消费了一个该校验从未覆盖的字段。测试中没有任何用例构造非字符串 resumeSessionId,所有出现都是字面量 'Managed-1'

一行修复:在 normalizeLaunch 中,与其他字段并列加上 resumeSessionId: stringValue(raw['resumeSessionId'])

/review 第 5 轮把它记为 R4-1,随后以「fails-closed on new surface,未认证错误结果,merge base 既无该功能面也无该缺陷」为由延后。我不认同这个定性:merge base 上的 sessions ps 根本不读 supervisor 存储,任何 launch 文件都影响不到它。一个连 interactive 列表都一并抹掉的崩溃,是对今天行为的回归,而不是一个此前不存在的功能面上的 fail-closed 缺口。它很小也很便宜,我建议在同一个 PR 里修掉,而不是作为后续工作带着走。

核对过、确认没问题的部分

  • pid 身份的「镜像」主张字面成立liveWorkerPid 的 boot-id 守卫与 session-registry.ts:553-557 完全同形,包括在本地 boot id 不可读时也要生效这一微妙之处。namespace 守卫有意不同(仅在双方都已知时生效)并做了说明 —— 这里是对的,因为本模块是拒绝打印 pid,而注册表是拒绝列出。
  • import 能解析:四个函数均来自 packages/core/src/utils/process-liveness.ts,由 packages/core/src/index.ts:703 重新导出;isSameProcess 的真实降级契约与模块注释、测试 mock 一致。
  • 测试 mock 是诚实的:它重实现了 isSameProcess 的回退规则,而不是直接钉一个结论。
  • 「两个读取方都已按最新在前排序」准确listAgentViewSessionSnapshots 末尾按 updatedAt 降序排序(:387-389)。
  • sanitizeSessionId 的搬迁安全:三处消费者全部仍可解析(存储内部 11 处、supervisor-process.ts:60 经由再导出、新模块从 protocol.js 导入);无循环依赖。
  • deriveAgentViewPresentation 调用正确:参数是联合类型且只有 input 一侧带 now,逐字段传递而非展开是对的。
  • 文档与代码一致:两种 --json 行形态都写到了,包括「同时存在于两个来源的 session 输出行形态而非记录形态」;jq -r 'select(.taskState == "waiting")' 示例正确(interactive 行没有 taskState,会被 select 过滤掉)。
  • stderr 路径已净化,且 stateLabel 只可能返回六个固定字面量之一,新列不会把外部文本带进终端。

测试证据

无人值守 CI 运行 —— 我没有构建或执行本 PR 的任何代码。以下全部来自本 PR 自己的 CI,通过 check-runs API 读取 79ca2fc 的结果。CI 已跑完:三个 pull_request workflow run 均为 completed,因此没有 pending,也没有任何猜测。

(表格见上方英文部分,由 finalize 任务在 CI 结束后就地更新。)

skipped 因而不构成任何一方证据的:Test (macos-latest)Test (windows-latest)Integration Tests (CLI, No Sandbox)

唯一的红检查是 base 过旧,不是本 diff。 失败步骤是 Run transcript document browser gate,其全部输出是一次收集失败而非断言失败:vitest 找不到 chat-transcript-document.test.ts。三条事实钉住了原因:(1) 该任务只检出 PR head(日志记录 ref: refs/pull/10942/headexpected_sha: 79ca2fc...),不是对当前 main 的合并提交;(2) 该闸门步骤与它的测试文件是一起在 74fe3a6#11014,2026-09-04 18:33Z)落到 main 的 —— 对该测试文件的 --diff-filter=A 与对 ci.yml-S 查询返回同一个 commit;(3) 该测试文件在 79ca2fc不存在(contents API 对该 ref 返回 404,对 ref=main 正常返回)。

也就是说:本次运行使用的工作流定义(来自 base)含有一个步骤,而该步骤所需的测试文件只存在于分支尚未合入的 base 上。本 PR 未触及 integration-tests/packages/web-shell/.github/workflows/ 下任何文件。这也解释了为什么同一个检查在更早的 8879852 上是 ✅ —— 那时 #11014 还没落地。合并或 rebase 到当前 main 即可清除,无需改代码;但在分支保护放行之前必须做。

描述中披露的类型检查缺口已被 CI 关闭。 Tested-on 一节说本地运行 npx tsc --noEmitnpm run build,需要 CI 或另一台机器。它得到了:Install dependencies 步骤运行 npm ci,触发 preparenode scripts/prepare.jsnpm run buildprepare.js:29)→ 各 package 的 build_package.jsexecSync('tsc --build')build_package.js:38),而 ci.yml 中任何位置都没有设置 QWEN_SKIP_PREPARE。因此绿色的 Test (ubuntu-latest)Lint & Static 在运行前都已用 tsc 编译过 packages/cli。PR 正文里那条注意事项可以撤掉了。需要注意:Lint & Static 本身跑的是 ESLint + Prettier + actionlint + shellcheck,并不含 tsc --noEmit;类型检查信号来自安装期的构建,而不是这个任务的名字。

未核实及原因:364 tests passing 是作者在 Linux 上仅跑 vitest 的声明,我没有重跑(第 5 轮评审观测到的完整套件为 28373 passed)。managed 那一半没有任何实时终端截取,因为目前没有任何东西会调度出 managed session。而上述两条 Critical 在现有测试下都不可达:第 1 条的降级形态要么被钉成正确、要么在合并时对 pid 无任何断言;第 2 条的触发条件从未被构造。

沙箱化验证可以了结 CI 无法了结的部分:@qwen-code /verify —— 核心主张是行为性的(managed session 出现在渲染出的表格中、排在 interactive 行之前、带真实状态),而 之后 代码块明确不是实时截取。更具体地说,针对 base 构建的 A/B 正是能证实上述两条发现的手段:手工写一个与存活注册表记录共用 session id 的 ownership: "managed" 状态文件,把它的 worker.json 截断,确认合并后的行丢掉 pid 4242;再在某个 launch.json 里写入 "resumeSessionId": 123,确认 qwen sessions ps 以非零退出且 interactive 行也一并消失。@qwen-code /tmux 可覆盖表格表面本身。你在本仓库有 admin 权限,两者都不需要他人代为触发。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 79ca2fc451cd6c0148d51eb49e363eff656865d9 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 2/5 — the craft here is good and the last round's Critical is genuinely fixed and pinned; but the code that landed in response to review introduced two new correctness defects in the merge, one of which a test currently asserts is correct behaviour, and I cannot approve over that.

Stepping back. Five commits ago I asked for one line — filter ownership — and for the tests to construct the values that filter excludes. Both happened, and happened well: the case that iterates unmanaged/adopting/removing and the adopting-window survival case are exactly the two I named. The taskState-versus-display-wording point was taken too, and taken in the better direction, with the reasoning written down at the render site. On the pid identity work I would have done less: I sketched a bare liveness probe, and this went and matched the registry's full boot-id and namespace contract instead. I checked that mirroring claim line by line against session-registry.ts rather than accepting the comment's word, and it is accurate, including the subtle part about firing when the local boot id is unreadable. That is not a PR that is trying too hard — it is a PR that took review seriously.

Which is why the two findings are worth being blunt about, because they are both children of the fixes rather than leftovers. Making the row honest about pids meant reading the worker file, and making the id resumable meant reading the launch file — and the store fails soft on both, returning undefined for any read or parse error by explicit design. Once that happens the managed row is the degraded half of the pair, and a merge rule that says "managed always wins" then throws away the record that still had the good value. The pid shape loses a number the registry verified one call earlier. The id shape is worse, because lists a mixed-case session once asserts the lossy result as the expected one — so this is not a gap in the suite, it is the suite certifying the defect. A green run here is evidence against the fix, not for it.

The second one is smaller and I keep coming back to it because it contradicts the module's own stated purpose. The docstring says a store that cannot be read must not take the command down, and readManagedRows is built to honour that — but mergeSessionRows is called outside it, and resumeSessionId is the single launch field normalizeLaunch never validated. One non-string in one launch file and qwen sessions ps produces nothing at all, interactive rows included. Today no launch file can affect this command. I would not call that fail-closed on new surface; I would call it a regression, and a one-line one to close.

On the direction question I raised in Stage 1 — reader landing before writer, --json contract of a shipped command changing today for a benefit that arrives with #7802 or #7803 — I am deliberately not blocking on it. It is a sequencing judgement, you have admin on this repo, and you have context on that PR stack that I do not. My honest read is unchanged: I would have landed the reader and the writer together. But that is your call to make, not mine to gate.

Two things that are not findings but would make the next pass faster. The PR body still does not mention the supervisor writer-side change — three new fields on a durable schemaVersion: 1 record, four write sites — which is a larger deal than the title implies and deserves a paragraph. And the 48 lines of .qwen/skills/find-simplifications/ edits are still riding along, still unmentioned; I flagged them last round and my view has not changed.

Verdict: should not merge as-is. I am not submitting a second CHANGES_REQUESTED review — one already stands from this bot on this exact commit (review 5117986596, commit_id 79ca2fc…), which is why the PR reads CHANGES_REQUESTED now, and stacking a duplicate would gate nothing and only add noise to a PR that has already been through five review rounds. What that existing review deferred, though, I would not defer: finding 2 above. And separately, the red web-shell E2E Smoke check needs a merge or rebase onto current main to clear — it is a stale-base artifact (the gate step and its test file both landed in 74fe3a6, after this branch's base), not a code problem, but branch protection will not let it through until the base is caught up.

Everything else in this diff I would keep exactly as written.

中文说明

Confidence: 2/5 —— 工程质量是好的,上一轮的 Critical 也确实修好并被测试钉住了;但作为回应评审而落地的代码,在合并逻辑里引入了两个新的正确性缺陷,其中一个当前正被测试断言为正确行为。在这样的状态下我无法批准。

退一步看整体。五个 commit 之前我要求的只有一行 —— 过滤 ownership —— 以及让测试去构造该过滤所排除的那些值。两件事都做到了,而且做得好:遍历 unmanaged/adopting/removing 的用例、以及收养窗口存活的用例,正是我点名的那两个。taskState 与展示措辞的意见也被采纳,并且朝更好的方向采纳,理由还写在了渲染处。pid 身份这部分我做到的会比它少:我原本只打算用一个裸存活探测,而它去对齐了注册表完整的 boot-id 与 namespace 契约。这个「镜像」主张我是对着 session-registry.ts 逐行核实的,没有只信注释,结论是准确的 —— 包括「本地 boot id 不可读时守卫也必须生效」这个微妙之处。这不是一个用力过猛的 PR,而是一个认真对待评审的 PR。

也正因为如此,这两条发现值得直说,因为它们都是修复的产物,而不是遗留物。让行对 pid 诚实,意味着要读 worker 文件;让 id 可 resume,意味着要读 launch 文件 —— 而存储对这两者都是软失败的,按明确设计在任何读取或解析错误下返回 undefined。一旦如此,managed 行就成了这对数据中降级的那一半,而一条「managed 永远赢」的合并规则,就会把仍持有正确值的那条记录丢掉。pid 形态丢掉的是注册表在前一次调用中刚刚验证过的数字。id 形态更糟,因为 lists a mixed-case session once 把这个有损结果断言成了期望值 —— 所以这不是测试的缺口,而是测试在为缺陷背书。这里的绿色是不利于该修复的证据,而不是支持它的证据。

第二个更小,但我反复回到它,因为它与该模块自己声明的目的相矛盾。文档字符串说:读不出来的存储绝不能拖垮这个命令,而 readManagedRows 正是为此而建的 —— 但 mergeSessionRows 是在它之外调用的,而 resumeSessionIdnormalizeLaunch 唯一从未校验过的 launch 字段。一个 launch 文件里一个非字符串,就能让 qwen sessions ps 什么都不输出,interactive 行也一并消失。今天没有任何 launch 文件能影响这个命令。所以我不会把它称作「新功能面上的 fail-closed」,我会称它为一次回归 —— 而且是一行就能关掉的回归。

关于我在 Stage 1 提出的方向问题 —— 读取方先于写入方落地、一个已发布命令的 --json 契约今天就要变、而收益要等 #7802#7803 —— 我刻意不以此阻塞。那是时序判断,你在本仓库有 admin 权限,而且你掌握着我所没有的那条 PR 栈的上下文。我的真实看法没有变:我会让读取方和写入方一起落地。但这是你的判断,不是我的闸门。

两件不是发现、但能让下一轮更快的事。PR 正文仍未提到 supervisor 写入侧的改动 —— 一个持久化 schemaVersion: 1 记录上新增三个字段、四处写入点 —— 这比标题所暗示的分量更重,值得单独写一段。以及 .qwen/skills/find-simplifications/ 那 48 行仍然搭车在这里,仍然没被提及;我上一轮就提过,看法没有变。

结论:当前状态不应合并。 我不会再提交第二份 CHANGES_REQUESTED 评审 —— 本机器人在这个完全相同的 commit 上已经有一份(评审 5117986596commit_id 79ca2fc…),这也是 PR 现在显示 CHANGES_REQUESTED 的原因;再叠一份不会增加任何闸门效果,只会给一个已经走过五轮评审的 PR 添加噪音。但那份评审所延后的东西,我不会延后:即上面的第 2 条发现。另外,红色的 web-shell E2E Smoke 需要合并或 rebase 到当前 main 才能清除 —— 它是 base 过旧的产物(闸门步骤与它的测试文件都在 74fe3a6 落地,晚于本分支的 base),不是代码问题;但在 base 追上之前,分支保护不会放行。

这个 diff 里的其他部分,我都会照原样保留。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 79ca2fc451cd6c0148d51eb49e363eff656865d9 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yiliang114 Requesting changes on one Critical finding — the full write-up is in the code-review comment above.

managedSessionRows maps every snapshot returned by listAgentViewSessionSnapshots to a row with managed: true, but that API returns all four AgentViewOwnership values (unmanaged, adopting, managed, removing), and every existing consumer on main filters before use (supervisor-process.ts:293, :299, :1187, :1435, :1439, :1606). Two consequences:

  1. Tombstones and teardowns get listed as live sessions — supervisor-dispatch.ts:164 leaves unmanaged/failed/exited state files behind on a failed dispatch, and gcUnmanagedSession (supervisor-process.ts:358) exists specifically to rm -rf them.
  2. A live interactive session can disappear from the listing. supervisor-process.ts:590 adopts an existing session and reuses its id under ownership: 'adopting'; mergeSessionRows then drops that session's registry record, losing its real pid and its whole --json payload, and shows it as ready with PID -. That is a regression of behaviour qwen sessions ps has today.

No test can catch either case: both fixtures hardcode ownership: 'managed'.

Suggested fix, matching the established idiom:

snapshots.filter((snapshot) => snapshot.state.ownership === 'managed')

plus one case per non-managed ownership value asserting it yields no row, and one asserting a registry record sharing a sessionId with an adopting snapshot survives the merge.

Also worth folding in (non-blocking): emit presentation.taskState in --json and keep the display wording (needs input) in the table only, so rewording the STATE column later cannot silently break jq scripts; and split the .qwen/skills/find-simplifications/ edits into their own PR — they are unmentioned in the body and aimed at a different reader.

Everything else here I would keep as written: the pure merge module, reusing deriveAgentViewPresentation so the listing and the roster cannot drift, the stderr degradation, and the sanitized failure reason are all the right calls. The direction question about sequencing this against #7802 is a maintainer's call, not a blocker.

CI had not settled at review time — Test (ubuntu-latest) and Lint & Static were still in flight and the macOS/Windows legs report skipped, so this diff has not been typechecked anywhere yet.

中文说明

基于一条 Critical 发现请求修改 —— 完整分析见上方的代码审查评论。

managedSessionRowslistAgentViewSessionSnapshots 返回的每一个快照都映射成 managed: true 的行,但该 API 会返回全部四种 AgentViewOwnership 值(unmanagedadoptingmanagedremoving),而 main 上每一个现存消费者在使用前都会过滤(supervisor-process.ts:293:299:1187:1435:1439:1606)。两个后果:

  1. 墓碑与拆除中的记录会被当作存活 session 列出 —— supervisor-dispatch.ts:164 在 dispatch 失败时会留下 unmanaged/failed/exited 的状态文件,而 gcUnmanagedSessionsupervisor-process.ts:358)的存在正是为了把它们 rm -rf 掉。
  2. 一个存活的 interactive session 可能从列表中消失。supervisor-process.ts:590 收养已存在的 session 时会在 ownership: 'adopting' 下复用其 id;随后 mergeSessionRows 会丢弃该 session 的注册表记录,丢掉真实 pid 与整条 --json 数据,并把它显示为 ready、PID 为 -。这是对 qwen sessions ps 今天已有行为的回归。

两种情况测试都抓不到:两个 fixture 都硬编码了 ownership: 'managed'

建议的修复,沿用既有写法:

snapshots.filter((snapshot) => snapshot.state.ownership === 'managed')

外加:为每个非 managed 的 ownership 值补一个「不产生行」的用例,以及一个「注册表记录与某个 adopting 快照共用 sessionId 时,合并后仍然存活」的用例。

另有两点建议一并处理(非阻塞):在 --json 中输出 presentation.taskState,把展示措辞(needs input)只留在表格里,这样日后改写 STATE 列文案不会静默破坏 jq 脚本;以及把 .qwen/skills/find-simplifications/ 的改动拆成独立 PR —— 正文没有提到它们,且面向不同的读者。

其余部分我都会照原样保留:纯函数合并模块、复用 deriveAgentViewPresentation 以避免列表与 roster 说法漂移、stderr 降级、以及净化后的失败原因,这些都是正确的判断。关于本 PR 与 #7802 时序安排的方向性问题,属于维护者的判断,不是阻塞项。

审查时 CI 尚未跑完 —— Test (ubuntu-latest)Lint & Static 仍在进行,macOS/Windows 两个 leg 报告 skipped,因此这份 diff 目前在任何地方都还没有被类型检查过。

Qwen Code · qwen3.8-max-2026-09-02

Comment thread packages/cli/src/commands/sessions/managed-rows.ts Outdated
Comment thread packages/cli/src/commands/sessions/ps.ts Outdated
Comment thread packages/cli/src/commands/sessions/managed-rows.ts Outdated
Comment thread packages/cli/src/commands/sessions/ps.ts
Comment thread packages/cli/src/commands/sessions/ps.test.ts
Comment thread packages/cli/src/commands/sessions/ps.ts
Comment thread docs/users/features/commands.md Outdated
Comment thread packages/cli/src/commands/sessions/ps.ts
Comment thread docs/users/features/commands.md Outdated
Comment thread packages/cli/src/commands/sessions/ps.ts Outdated
yiliang114 and others added 4 commits September 4, 2026 07:28
managedSessionRows mapped every snapshot the store returns, but the
store also holds unmanaged tombstones, mid-removal snapshots and
mid-adoption snapshots; an adopting snapshot reuses the id of a live
registered session, so the merge replaced a registry row that knows a
live pid with a pid-less ghost. Filter to ownership === 'managed',
the shape the supervisor's own listing uses.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtlz4ubdgq
The supervisor store is durable and nothing reaps it when no supervisor
runs, so after a crash or a reboot a managed row could carry a pid that
is dead or recycled to an unrelated process, and acting on it would
signal the wrong target. Check both recorded pids with core's isPidAlive
(worker first, then the PTY host, matching the supervisor's own idiom)
and print `-` when neither lives.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtlz4ubdgq
The reason text is foreign input, and a preserved LF would forge extra
lines out of the single stderr note the sibling test pins. Feed a
message carrying all four classes and assert none survive, so a
downgrade to plain sanitizeTerminalText (which keeps LF and TAB) fails.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtlz4ubdgq
The .name example prints session-generated text with jq -r; like the
adjacent .cwd example, it must say that the value is rendered raw and
to sanitize it when untrusted.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtlz4ubdgq

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test Plan (not a blocker): 364 tests passing — this review observed 28158 passed.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/commands/sessions/managed-rows.test.ts:72 — [review] D2-1 fixture builders (record/state/snapshot/workerFile) duplicated across the two adjacent test files; the ps.test.ts copy is untyped and would silently emit stale shape…
  • packages/cli/src/commands/sessions/managed-rows.test.ts:43 — [probe] D2-2 projectCwd/originalCwd/activeCwd collapse to one fixture value and no test asserts row.cwd — the activeCwd source choice is unpinned (both mutants survive)
  • packages/cli/src/commands/sessions/ps.ts:119 — [probe] D2-3 readManagedRows' catch over the synchronous mapping is untested — a narrowed try would crash instead of degrade with all tests green
  • packages/cli/src/commands/sessions/ps.test.ts:89 — [probe] D2-4 managedSnapshot({state: ...}) partial-state overrides are silently clobbered by the trailing ...over spread (observed TypeError in the probe run)
中文说明

Test Plan(非阻断):364 tests passing — this review observed 28158 passed

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 4 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/commands/sessions/managed-rows.ts Outdated
Comment thread packages/cli/src/commands/sessions/managed-rows.ts Outdated
Comment thread docs/users/features/commands.md
Comment thread docs/users/features/commands.md Outdated
Comment thread packages/cli/src/commands/sessions/managed-rows.ts
Comment thread packages/cli/src/commands/sessions/ps.ts Outdated
Comment thread packages/cli/src/commands/sessions/ps.ts
Comment thread packages/cli/src/commands/sessions/ps.ts Outdated
Comment thread packages/cli/src/commands/sessions/ps.ts
Review finding 2 on this PR: `SessionRow.state` was documented as "what
the STATE column can say" and went straight into `--json`, so the
machine contract was pinned to display copy. `'needs input'` carries a
space, and the documented recipe was
`jq -r 'select(.state == "needs input")'` — reword the column later and
every script breaks silently, with no type error anywhere to warn.

The stable enum was already in hand. The row now carries
`presentation.taskState` (`running` | `waiting` | `ready` | `stopped` |
`failed`) and `ps.ts` maps it to English at the one place that renders a
table.

This also removes an asymmetry the reviewer named: `state` was a *kind*
discriminator for registry rows (`'interactive'`) and a *task* state for
managed ones — one field with two meanings, which is why `managed` had
to be added beside it. A registry row now has no `taskState` at all,
which is the truth: it knows a process is alive and nothing more.
CI caught what a vitest-only run cannot: the previous commit annotated a
test case with `AgentViewTaskState` but never imported it, so
`tsc --build` failed with TS2304 and took the TUI gates down with it on
every branch in the stack. The import edit had targeted an import block
that had since gained another symbol, so the anchor never matched and
the change was silently dropped.

Verified the same way it should have been the first time: every file
that names the type now imports it, and nothing anywhere still refers to
the removed `SessionRowState` or `row.state`.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • docs AGE - contract re-derivation — already reported as R1-9 (inline thread at docs/users/features/commands.md)
  • module header interactive-only opening-line re-derivation — already reported as R1-6 (inline thread at packages/cli/src/commands/sessions/ps.ts)
  • supervisor-listing comment-accuracy re-derivation — already reported as R2-2 (comment 3930057660 at packages/cli/src/commands/sessions/managed-rows.ts)
  • AGE dash render-level test gap re-derivation — already reported as R1-3 (inline thread at packages/cli/src/commands/sessions/ps.ts)
  • cwd source-choice unpinned re-derivation — already reported as D2-2 in the round-2 review body deferral list (review 5108278256)

Test Plan (not a blocker): 364 tests passing — this review observed 28159 passed.

Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/commands/sessions/managed-rows.ts:89 — [probe] managedSessionRows' now parameter is dead plumbing — it feeds only the discarded ageLabel (deferred under the code-age rule: unchanged since the previous round reviewed it)
  • docs/users/features/commands.md:790 — [probe] --json field list omits ipcPath and "whole registry record" ignores the ipcToken strip (deferred under the code-age rule: unchanged since the previous round reviewed it)

Convergence: round 3 posted 11 inline comment(s), 2 of them reported for the first time; the previous round posted 9 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/sessions/managed-rows.ts (findings in rounds 1, 2; 1 more now); packages/cli/src/commands/sessions/ps.ts (findings in round 1; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

本轮确认的 5 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

Test Plan(非阻断):364 tests passing — this review observed 28159 passed

收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 3 轮发布了 11 条行内评论,其中 2 条是首次提出;上一轮发布了 9 条(其中 2 条首次提出)。发现反复回到同一批文件:packages/cli/src/commands/sessions/managed-rows.ts(第 1、2 轮已出过发现,本轮又有 1 条);packages/cli/src/commands/sessions/ps.ts(第 1 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。(仅为观察——本轮评审未因此扣留任何内容。)

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/commands/sessions/managed-rows.ts Outdated
Comment thread packages/cli/src/commands/sessions/managed-rows.ts Outdated
Comment thread packages/cli/src/commands/sessions/managed-rows.ts Outdated
Comment thread docs/users/features/commands.md Outdated
Comment thread docs/users/features/commands.md Outdated
Comment thread packages/cli/src/commands/sessions/ps.ts
Comment thread packages/cli/src/commands/sessions/ps.ts
Comment thread packages/cli/src/commands/sessions/ps.ts
Comment thread packages/cli/src/commands/sessions/ps.ts
Comment thread packages/cli/src/commands/sessions/ps.ts
yiliang114 and others added 5 commits September 4, 2026 15:20
Both blockers on this listing are the same mistake in two places: the
merge joins two sources that disagree about what identifies a session,
and each half trusted its own spelling.

Pids. `liveWorkerPid` gated durable recorded pids with a bare
`isPidAlive`, and the worker file recorded no process identity at all.
Nothing reaps that file while no supervisor runs — `clearAgentViewWorkerPids`
does not get to run after a SIGKILL or a reboot — so once the OS recycles
the number, `kill(pid, 0)` answers "alive" about an unrelated process and
`qwen sessions ps` prints it next to a row still reading `working`; a
script reading `--json` kills a stranger. A `~/.qwen` shared between
machines or namespaces (an NFS home, a devcontainer with the home
mounted) needs no recycling to reach the same place. The interactive rows
in the very same table are verified with `isSameProcess` plus namespace
and boot-id guards, so the two halves answered to different evidence
standards.

The supervisor now records the start token for each pid it writes, plus
its own PID namespace, and the reader checks `isSameProcess` against
them. All three fields are optional: `AgentViewWorkerFile` is a durable
`schemaVersion: 1` record, and a file written before they existed carries
none, which `isSameProcess` reads as "no identity recorded" and degrades
to exactly the liveness check this command did before. The namespace
guard fires only on a known disagreement, so an unreadable `/proc` on
either side never blanks a real worker's pid.

Ids. `mergeSessionRows` deduped with a case-sensitive `Set.has`, but the
supervisor store reports the sanitized, lowercased directory name it
files a session under while the registry keeps the raw spelling the
worker registered with — adoption keeps both on purpose, because the
native session store is case-sensitive. A managed session whose id
contains an uppercase letter therefore slipped the filter and listed
twice, once with its real state and once as `interactive`, with `--json`
emitting two different spellings for one session. Both sides now
canonicalize through `sanitizeSessionId`.

That sanitizer moved from `supervisor-store` to `protocol`, which is
where it belongs — it defines the identity two readers must agree on, not
a detail of writing files — and it keeps the pure row module free of a
dependency on the filesystem store. `supervisor-store` re-exports it, so
no caller changes.

Tests pin the two arms liveness cannot see: a live pid whose recorded
token no longer matches, and a foreign-namespace worker file. Two more
pin the degradations, since silently blanking every pre-identity pid
would be the worse regression. The mock mirrors the real `isSameProcess`
contract rather than stubbing a verdict, so the existing dead-pid cases
still exercise the fall-through they were written for.

Typecheck, lint and tests were not run locally; CI is the authority on
this branch.
853d57b taught `managedSessionRows` to verify pids with `isSameProcess`
and `readPidNamespaceId`, and updated the core mock in
`managed-rows.test.ts` — but `ps.test.ts` mocks the same module and
reaches the same code through `ps.ts`, and its factory still exported
only `listLiveSessions` and `isPidAlive`.

So `readPidNamespaceId()` was `undefined` at the call site, the TypeError
propagated out of the snapshot mapping, and `ps.ts` caught it on the path
meant for an unreadable supervisor store: every managed row vanished and
the command reported a store failure. Four tests failed in CI with the
managed half of the listing simply missing — `expected [ false ] to
deeply equal [ true, false ]` — rather than with the type error that
caused it.

The mock now mirrors the real `isSameProcess` contract, the same way
`managed-rows.test.ts` does, so the fixtures — which record no start
token — keep degrading to a bare liveness check and
`isPidAlive.mockReturnValue(false)` still means exactly "this pid is
dead".

Worth a follow-up, filed separately rather than widened into this commit:
`ps.ts` treating a TypeError from row mapping as "the store could not be
read" is why this surfaced as four confusing assertion failures instead
of one stack trace.

Typecheck, lint and tests were not run locally; CI is the authority.
- managed-rows.ts: the managedSessionRows rationale block said the
  supervisor's own listing "skips the same shapes", but
  SupervisorProcess.list() only skips unmanaged and removing snapshots
  and shows sessions mid-adoption. State the difference, and why this
  listing must keep filtering adopting snapshots (R2-2).
- ps.test.ts: render the human table for managed sessions in working,
  idle, stopped and failed states and assert the STATE cell against the
  documented labels, so no swap among the four previously unpinned
  TASK_STATE_LABEL values can ship green (R3-1). Also fix the
  managedSnapshot helper, whose trailing spread shadowed the
  constructed state when a caller passed a partial state override.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtmourmvi4
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Closeout summary for the remaining deferred review threads:

  • Fixed the degraded empty-state claim: when the supervisor store cannot be read and the registry is empty, human output now says only that no interactive sessions were found and that managed sessions could not be listed. JSON stdout remains empty/parseable and the detailed reason remains on stderr.
  • Corrected the module, test, PR-description, and user-documentation claims about managed-session registration. Managed sessions can also have registry records; the managed row wins the dedupe because the supervisor store carries richer lifecycle state.
  • Corrected the PID/AGE - documentation and added render-level coverage for an unusable creation timestamp.
  • Kept the duplicated createdAt parsing as follow-up rather than adding createdAtMs to the shared presentation interface. The current paths intentionally expose different contracts (formatted UI age versus epoch milliseconds for --json), and there is no current behavior divergence; changing that shared interface would expand this already large closeout for speculative future drift.
  • The Dependency CVE audit failure was an npm audit endpoint 503 Service Unavailable; the remaining package audits reported zero vulnerabilities, and this PR changes no dependency files.

Verification on the pushed diff: 51 focused session tests passed; related ESLint, Prettier, and git diff --check passed.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • docs --json field list vs ipcPath/ipcToken wording — already reported in the round-3 review body deferral list (review 5109073154)
  • activeCwd source-choice unpinned — already reported as D2-2 in the round-2 review body deferral list (review 5108278256)
  • R1-2 duplicated createdAt parse — still standing at this commit; author explicitly deferred it as a follow-up with rationale in the closeout summary (comment 5540135921), thread at packages/cli/src/commands/sessions/managed-rows.ts

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

4 Suggestion(s) were drafted inline past the resolved critical posting floor — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s); the CLI moved them into the deferral list below (floor enforcement).

Deferred under the convergence posture (round 4, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round:

  • packages/cli/src/commands/sessions/managed-rows.test.ts:250 — [review] R4-2: This pre-identity degradation test runs only with an unreadable own pid namespace — the beforeEach default mocks pidNamespaceId -> null — so deleting the work…
  • packages/cli/src/agent-view/supervisor-process.ts:511 — [review] R4-3: No test asserts that a written worker file actually records the new identity fields ( hostProcStart / workerProcStart / pidNs ) — the write side of the pid-identity cont…
  • packages/cli/src/commands/sessions/managed-rows.ts:187 — [review] R4-4: The host fallback's identity check — worker.hostProcStart passed to isSameProcess in the second candidate — is pinned by no test: no test file in the package ever s…
  • packages/cli/src/agent-view/supervisor-store.ts:891 — [review] R4-5: The read half of the new pid-identity contract — normalizeWorker materializing hostProcStart / workerProcStart / pidNs from the durable worker file — is pinned by no t…
  • packages/cli/src/commands/sessions/ps.test.ts:365 — [probe] managed --json row shape pinned on 4 of its 7 fields; cwd unpinned along the JSON path
  • packages/cli/src/commands/sessions/ps.test.ts:345 — [probe] handler-level mergeSessionRows dedupe wiring has no end-to-end pin
  • packages/cli/src/commands/sessions/ps.ts:159 — [probe] store-failure partial listing exits 0 — machine consumers cannot tell complete from partial
  • docs/users/features/commands.md:836 — [probe] new jq example breaks the ps/list_agents name equivalence for managed sessions

Convergence: round 4 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 11 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/sessions/managed-rows.ts (findings in rounds 1, 2, 3; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 3 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

4 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论——发布下限因首次发现速率连续 2 轮未下降而提前生效;CLI 已将其移入下方延后清单(下限强制执行)。

收敛姿态下延后(第 4 轮,非阻断)——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录,本轮不要求修改:共 8 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 4 轮发布了 2 条行内评论,其中 2 条是首次提出;上一轮发布了 11 条(其中 2 条首次提出)。发现反复回到同一批文件:packages/cli/src/commands/sessions/managed-rows.ts(第 1、2、3 轮已出过发现,本轮又有 2 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/commands/sessions/managed-rows.ts
Comment thread packages/cli/src/commands/sessions/managed-rows.ts Outdated
yiliang114 and others added 2 commits September 5, 2026 02:59
liveWorkerPid verified recorded pids with the start token and the PID
namespace, but omitted the registry's boot-id pre-guard. Two machines
sharing one ~/.qwen both live in the initial PID namespace, whose inode
is a kernel constant, so the namespace guard never fires between them;
when the local boot id is unreadable — or the reader is non-Linux —
isSameProcess degrades to a bare liveness check and a foreign worker
file's pid, routinely a live unrelated local process, was printed.

Read readLocalBootId() once and skip any candidate whose recorded token
carries a boot prefix that differs from the local boot id, mirroring
listLiveSessions: the guard fires on an unreadable local boot id as
well, not only on a positive mismatch, and token-less pre-identity
candidates keep the liveness fall-through.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtnaadh9j8
The managed row emitted the sanitized (lowercased) store id as
sessionId, and mergeSessionRows dropped the deduped registry record
that was the only other carrier of the raw spelling. For any session
whose id contains an uppercase letter — a supported class, e.g.
arena-agent ids — a script piping the emitted id into `qwen --resume`
hit the case-sensitive native store's exact-match lookup and failed.

Emit launch.resumeSessionId when recorded, falling back to the store
id for sessions created rather than adopted; the name fallback follows
so the table shows the id scripts can act on. The dedupe comparison
stays sanitized on both sides and store keying is untouched — only the
reported value changes.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtnaadh9j8

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R5-1 managed-row cwd source choice unpinned (managed-rows.ts:150) — already reported as D2-2 in the round-2 review body deferral list (review 5108278256)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Test Plan (not a blocker): 364 tests passing — this review observed 28373 passed.

Deferred under the convergence posture (round 5, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:

  • packages/cli/src/commands/sessions/managed-rows.ts:134 — [probe] Critical [fails-closed] [new-surface] R4-1: (fix-induced) unvalidated launch.resumeSessionId — a non-string value crashes mergeSessionRows outside readManagedRows' catch and t…
  • packages/cli/src/commands/sessions/managed-rows.test.ts:238 — [probe] namespace guard worker.pidNs != null escape hatch unpinned — dropping the clause ships green and blanks pids of pidNs-less worker files
  • packages/cli/src/commands/sessions/managed-rows.test.ts:285 — [probe] token-less candidates under unreadable local boot id unpinned — a fail-closed early-return mutant ships green
  • packages/cli/src/commands/sessions/managed-rows.test.ts:322 — [probe] same-boot starttime-mismatch rejection unpinned — a boot-match liveness-shortcut mutant ships green
  • packages/cli/src/commands/sessions/managed-rows.test.ts:338 — [probe] namespace guard whole-row scope unpinned — a per-candidate-skip mutant ships green
  • packages/cli/src/commands/sessions/managed-rows.test.ts:368 — [probe] name precedence (roster display name over activity summary) never tested with both present — branch-swap mutant ships green
  • packages/cli/src/commands/sessions/managed-rows.test.ts:369 — [probe] deriveTitle initialPrompt rung pinned by zero tests — rung deletion ships green

Convergence: round 5 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/sessions/managed-rows.ts (findings in rounds 3, 4; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Test Plan(非阻断):364 tests passing — this review observed 28373 passed

收敛姿态下延后(第 5 轮,非阻断)——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录,本轮不要求修改;其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface:未认证任何错误结果,且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中:共 7 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 5 轮发布了 1 条行内评论,其中 1 条是首次提出;上一轮发布了 2 条(其中 2 条首次提出)。发现反复回到同一批文件:packages/cli/src/commands/sessions/managed-rows.ts(第 3、4 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/commands/sessions/managed-rows.ts
@yiliang114

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run

Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check.

Scripted assertions: 167 passed · 0 failed · 167 total

Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:167 通过 · 0 失败 · 167 总计

抖动门:✅ 2 changed test file(s) x 5 identical rounds, no divergence

Verification report

PR 10942 — deep verification report

Verdict: merge-ready — 167/167 scripted assertions passed, 0 unexpected failures. Verified head 79ca2fc451cd6c0148d51eb49e363eff656865d9 (HEAD^2), base tip 74fe3a659dde2859f152d6c860e04cfddca86d05 (HEAD^1), merge ref 602d60e77c8947eabc943d46f2d9ceaa0bf86338. Central claim proven load-bearing by A/B; every guard the PR introduces is load-bearing (15/15 mutants killed, 2/2 in-file controls killed); targeted gates green and proven live. Non-blocking observations in §5.

中文摘要
  • 结论merge-ready。167 条脚本化断言全部通过,0 条意外失败。中心主张经 A/B 证明为 load-bearing:同一份磁盘 fixture 下,base(74fe3a65)完全看不到 managed session(0/1),head(79ca2fc4)把它列在第一行并标注 needs input(1/1),见 01-ab-base-vs-head-sessions-ps.png 与 §3 的 A/B 表。
  • A/B 结论:base 无 STATE 列、无 managed 判别字段;head 两者都有,且 --json 的 managed 行带稳定 token taskStatewaiting)而非显示文案。pid 身份守卫矩阵 13 种真实 worker.json 形态全部按预期裁决(回收 pid / 外命名空间 / 外 boot 一律打印 -,无 token 的旧文件降级为存活检查),见 02-pid-identity-matrix-head.png
  • 变异矩阵:对 PR 自带的两个新测试文件(59 个测试,baseline 全绿)做 15 个单点变异 + 1 个组合行,15/15 被杀,2/2 同文件阳性对照被杀,无存活者,无因 import/编译错误造成的假杀,见 03-mutation-matrix-15-of-15-killed.png
  • Findings(均不阻塞):(1) 归一化 id 去重在「两条仅大小写不同的 session id 同时存活」时会静默丢弃两个 registry 行(base 列 2 行、head 列 1 行且 stderr 无提示);触发需要用户刻意用 --session-id 传同一 UUID 的两种大小写,故为设计后果而非缺陷,见 §5-F1。(2) managed 行赢得去重时无条件丢弃 registry 行已知的活 pid(base 打印 pid、head 打印 -);未能构造出生产路径,见 §5-F2。(3) 文档字段列表两处不精确:managed 行在缺值时省略 pid/startedAt(不是 null),且 interactive 列表漏列可选字段 ipcPath,见 §5-F3。
  • 未覆盖:逐 commit 归因(快照列 15 个 commit,本地仅可达 1 个,depth-2);真实 supervisor/PTY 端到端(feat(cli): Expose agent view commands #7802 未落地前无调度器,fixture 为按归一化形状手写的 store 文件);--resume 真能恢复的端到端验证;boot id 不可读本机的一侧;Windows/macOS 降级路径;全仓测试与从零构建。详见 §7。

1. Scope

Central claimqwen sessions ps lists managed Agent View sessions (supervisor store) beside the interactive ones (live registry), merged and deduped by session id, managed first, with a STATE column and a managed discriminator in --json.

Secondary claims

  • S1: a managed row prints a pid only when isSameProcess vouches for it against the recorded start token, with the namespace and boot-id guards; a token-less worker file degrades to a bare liveness check.
  • S2: an unreadable supervisor store degrades to the registry half, names the reason on stderr (sanitized), and keeps --json stdout parseable.

Out of scope by choice: everything in §7.

2. Environment and controls

  • CI merge-ref checkout: HEAD = merge, HEAD^1 = base tip, HEAD^2 = PR head; shallow (depth 2). npm ci + npm run build had already run at HEAD before this round; packages/cli/dist/src/commands/sessions/managed-rows.js and .d.ts exist in that build, so the author's "build not run" caveat is answered here.
  • Both arms run through the repo's own dev runner (node scripts/dev.js sessions ps), i.e. real TypeScript source per tree, with @qwen-code/qwen-code-core mapped by that runner to that tree's own core source. Base tree = git worktree add tmp/base-tree HEAD^1 with node_modules symlinked to the root install.
  • Realpath assertion (the skill's monorepo control): from inside the base tree, readlink -f node_modules/@qwen-code/qwen-code-core/__w/qwen-code/qwen-code/packages/core. That is the head tree's core, and git diff HEAD^1..HEAD -- packages/core packages/acp-bridge packages/sdk-typescript packages/channels is empty, so sharing the core build is a clean control; package.json/package-lock.json are untouched by the PR, so sharing node_modules is too. Both quoted in §8.
  • Fixtures are real: live sleep processes with tokens read by core's own readProcStartToken, real <QWEN_HOME>/jobs/<id>/{state,launch,activity,worker}.json, daemon/roster.json and sessions/<pid>.json written in exactly the shapes supervisor-store.ts / session-registry.ts normalize. Nothing about the unit under test is mocked.
  • Fixture defect found and fixed mid-round (my harness, not the PR): readProcStartToken's resolution is one clock tick (CLK_TCK=100 → 10 ms); three sleeps spawned back-to-back got identical tokens, which silently turned the "recycled pid" case into a matching one. The harness's own positive control (exactly 6/13 rows print a real pid) caught it. spawnLive now retries until every issued token is distinct and asserts distinctness. §5-F4 records the mechanism-level consequence.
  • Second harness defect, fully attributed: one captured run of pid-identity returned 39/1; the failing label was head: stderr clean, polluted by Node's own warning "The 'NO_COLOR' env is ignored due to the 'FORCE_COLOR' env being set" — verify-capture.mjs sets FORCE_COLOR while my runner set NO_COLOR. Four standalone re-runs were 40/40; a second captured run reproduced the 39/1 and, with the tally now recording failing labels, named exactly that warning. The runner now deletes FORCE_COLOR from the child env, and the captured run that produced 02-pid-identity-matrix-head.png is 40/40. No PR behaviour was ever in question.

3. Central claim — A/B table

One identical fixture: one managed session (needs_input, live worker pid) + one live registry record. Witness: 01-ab-base-vs-head-sessions-ps.png.

cell environment observable oracle result
base, human tmp/base-tree @​ 74fe3a65, dev runner, QWEN_HOME fixture table rows, columns 1 row (app-ab), no STATE column, svc-audit absent
head, human head tree @​ 79ca2fc4, same runner, same fixture table rows, columns 2 rows; svc-audit first with needs input and the live worker pid; app-ab second with interactive
base, --json same JSON Lines 1 line, no managed key, no ipcToken
head, --json same JSON Lines 2 lines; managed row managed:true + taskState:"waiting" and no display-wording state; registry row managed:false, no taskState, record verbatim minus ipcToken
delta managed session visible base 0/1 → head 1/1

40/40 assertions in ab-central.mjs (log logs/ab-central-console.txt). The base cell is the expected-red control encoded as an assertion that the base arm lacks the row, so it counts as a pass.

4. Secondary claims

S1 — pid identity (40/40, pid-identity.mjs, witness 02-pid-identity-matrix-head.png). 13 real worker.json shapes, oracle = the printed PID column; every row's presence is asserted so a - can never be a missing row:

case worker file printed PID
live-matching live pid + its own token + own ns the pid
recycled-pid live pid, token of a different live process -
dead-pid exited pid -
pre-identity-file live pid, no token fields the pid (degradation preserved)
explicit-null-token live pid, tokens null the pid
foreign-namespace live pid + matching token, pidNs ≠ ours -
null-namespace live pid + matching token, pidNs: null the pid
foreign-boot live pid, token prefixed with another boot id -
malformed-token live pid, token with no boot prefix - (fails the full-token comparison)
no-worker-file absent -
host-fallback dead worker pid, live matching host pid the host pid
host-wins live worker pid w/ foreign token, live matching host pid the host pid (loop continues)
both-dead two dead pids -

Plus the untitled fallback: no roster/activity/launch → the row's name is the session id, and Untitled session never reaches the table.

S2 — store failure (28/28, store-failure.mjs). <home>/jobs made a regular file → readdir throws ENOTDIR (root cannot be permission-locked, so a wrong file type is the failure that survives this container). Head: exit 0, interactive row still listed, stderr Managed sessions could not be listed: ENOTDIR: …, --json stdout still one parseable line. Empty listing says "…managed sessions could not be listed", never the complete-listing wording. An intact store produces no note (positive control). Hostile QWEN_HOME carrying LF, TAB, CR, an ESC sequence and a bidi override inside the path: stderr is exactly one line, no raw ESC byte, no TAB/CR/LF, bidi stripped — while base emits nothing on stderr at all, proving the note is new behaviour.

Id canonicalization (26/26, id-canonicalization.mjs). Mixed-case id (Sess-MixedCase in the registry, sess-mixedcase as the store dir): head lists one row, managed, and --json reports the resumable spelling Sess-MixedCase; base listed only the registry row. Two registry records sharing one session id: both listed at head and at base (the "carried, not looked up" claim holds; no regression).

Docs census (33/33, labels-and-docs.mjs). All seven sessionState values print their documented label end-to-end (starting/workingworking, needs_inputneeds input, idle/completedready, stopped, failed), plus interactive; an unparseable createdAt prints AGE -; the documented --json field lists match exactly for a fully-populated row of each kind; the documented pipeline qwen sessions ps --json | jq -r 'select(.taskState == "waiting") | .name' run verbatim selects exactly the waiting sessions; an empty --json listing prints nothing.

4b. Corrections to the description

Not requests to change code — the description states two things about the verified head that this round measured differently:

  • The Reviewer Test Plan says npx vitest run src/commands/sessions/ src/agent-view/ --coverage.enabled=false gives "17 files, 364 tests passing". At the verified head the same command gives 17 files, 385 tests (log gate-vitest-sessions-agentview.txt); the extra 21 arrived with the later commits in the stack. The file count and the command are right; the test count is stale.
  • The description says npx tsc --noEmit and npm run build "were not run" on the author's machine and that the After table row "is not a live capture". Both are now measured here: tsc --noEmit on packages/cli is clean (§6), the CI build at HEAD contains the compiled new module (§6), and 01-ab-base-vs-head-sessions-ps.png is a live capture of exactly that After shape (plus the base Before shape beside it).

5. Findings (non-blocking)

F1 — the canonicalized dedupe can silently drop live registry rows. Measured: two live registry records whose session ids differ only by case (Sess-ABC, sess-abc) plus a managed session filed under sess-abc → base lists 2 rows, head lists 1 managed row and drops both, with nothing on stderr (id-canonicalization.mjs case 5, logs idcanon-casevar-*.txt; repro: that harness). Reachability is narrow but real: isValidSessionId is case-insensitive (/i in packages/cli/src/config/session-id.ts) and the plain --session-id path does not lowercase (only the serve/ACP admission paths call normalizeSessionIdForLookup), and Arena ids (<uuid>-agent-Foo) keep mixed case by construction. A second, synthetic shape (foo:bar vs foo_bar) collapses the same way but is not reachable through validated ids (charset is hex+hyphen plus the -agent- suffix), so it is reported as a property of the lossy sanitizer, not a live hazard. What it is not: not reachable with randomUUID() ids, and folding case is the same mechanism that fixes the double-listing this PR exists to close. Suggested direction, unapplied: when a registry record is dropped because its canonical id matches a managed row, keep its pid if the managed row has none.

F2 — a managed row that wins the dedupe discards a live registry pid. Measured: managed session with no worker file + a live registry record for the same id → base prints the live pid (20722 in the run), head prints - (logs idcanon-ghost-*.txt). I could not construct a production path where a live registry record coexists with a pid-less managed row: hibernate/stop kill the worker (so its record goes stale), and a foreign-namespace worker file implies a foreign-namespace registry record that listLiveSessions also skips. So this is the measured shape of the merge's preference rule, not a demonstrated regression; F1 is the reachable instance of the same mechanism.

F3 — two doc-precision gaps in the --json field lists the PR wrote. Measured: (a) a managed row with no worker and no usable stamp omits pid and startedAt rather than emitting null ({"name":…,"cwd":…,"taskState":…,"sessionId":…,"managed":true}), while the docs list them unconditionally — jq .pid yields nothing, not null; (b) the optional registry field ipcPath is emitted when present, though the documented interactive list omits it. Both from labels-and-docs.mjs (logs labels-console.txt, labels-head-json.txt).

F4 — the identity guard inherits core's 10 ms token resolution. readProcStartToken is <boot_id>:<starttime> at CLK_TCK=100; two processes started inside one tick share a token (measured directly: three back-to-back sleeps → identical tokens). A pid recycled within one tick of the original start is therefore indistinguishable from it. Pre-existing in packages/core and shared with the registry rows this PR aligns to; not introduced here. Recorded because my first fixture run hit it and because it bounds what "the registry's identity standard" means.

6. Gates

gate command result proven live?
unit tests, affected surface cd packages/cli && npx vitest run src/commands/sessions/ src/agent-view/ --coverage.enabled=false 17 files, 385 tests passed n/a (positive result)
typecheck cd packages/cli && npx tsc --noEmit exit 0, no diagnostics n/a
eslint, changed files npx eslint <8 changed files> exit 0 yes — planted const gateProbeUnused = 1;'gateProbeUnused' is assigned a value but never used (log gate-live-proof.txt), then restored (git status clean)
prettier, changed files + docs npx prettier --check … all clean yes — same planted bad-indent line reported, then restored
build CI build at HEAD, artifacts checked packages/cli/dist/src/commands/sessions/managed-rows.js + .d.ts present with the new symbols n/a

Mutation matrix (witness 03-mutation-matrix-15-of-15-killed.png, logs mut-*.txt, table logs/mutation-matrix-table.txt). Isolated worktree at HEAD; the PR's own two new test files (59 tests) per cell; baseline 59 passed / 0 failed; each file restored with git checkout -- afterwards (worktree verified clean). Controls landed in the same file as each mutant: C1 (pid ?? 0, managed-rows.ts) killed 8 with expected +0 to be undefined; C2 (failed: 'ready', ps.ts) killed 1 with expected 'ready' to be 'failed'.

id mutation killed red tests / assertion
M1 drop ownership === 'managed' filter yes lists only snapshots the supervisor owns, lets a live registry record survive the adopting window
M2 liveWorkerPid → bare isPidAlive yes 5 red, expected 200 to be undefined
M3 namespace guard only yes refuses pids from a worker file written in another PID namespace
M4 boot-id guard only yes 3 red, expected 87 to be undefined
M5 both guards (combination row) yes 4 red
M6 dedupe on raw ids yes lists a mixed-case session once… (length of 1 but got 2)
M7 drop resumeSessionId preference yes reports the resumable spelling…
M8 drop the untitled→id override yes falls back to the session id…
M9 interactive rows first yes 4 red incl. puts managed rows above interactive ones
M10 raw reason on stderr yes neutralizes control sequences… (not to contain '\u001b')
M11 swallow the store failure yes 3 red
M12 leak ipcToken yes strips the inbox auth token…
M13 managed:false only on managed rows yes 2 red
M14 store failure takes the command down yes 4 red
M15 waitingworking yes lists a managed Agent View session…

No survivors, so there is no coverage gap to report; M3 and M4 each kill alone and together, so the two identity guards are independently load-bearing rather than layered redundancy. No kill was invalidated by an import/compile error (checked per log).

7. Not covered

  • Per-commit attribution. The snapshot lists 15 commits; git rev-list HEAD^1..HEAD^2 returns 1 (shallow depth-2). Only the aggregate HEAD^1..HEAD diff was verified; no per-commit table is presented.
  • Real supervisor/PTY end-to-end. Nothing dispatches a managed session until feat(cli): Expose agent view commands #7802 lands, so every managed fixture is a hand-written store file in exactly the shapes the normalizers accept. This reproduces the store shapes and the read path, not the supervisor's write path.
  • --resume actually resuming with the emitted spelling — only the emitted value was asserted.
  • The "our own boot id unreadable" arm of the boot guard (needs an unreadable /proc); the PR's unit tests pin it, I did not.
  • Windows/macOS, where the identity guards degrade to liveness by design.
  • Repo-wide test suite and a from-scratch build; CI had built at HEAD and the targeted gates above were run instead.
  • The .qwen/skills/find-simplifications prose changes (commit 2) — not exercised.
  • Working-tree condition at session start: uncommitted modifications to .qwen/skills/find-simplifications/SKILL.md and references/survey.md (partially reverting commit 2) were present and left untouched. They are markdown skill docs consumed by nothing in this round; git diff HEAD showed only those two files, so every code path ran from committed HEAD content.

8. Methodology

One paragraph: the round ran in the CI verify container on the merge-ref checkout; both arms executed the real CLI from TypeScript source via scripts/dev.js under a scratch QWEN_HOME, with the base arm in a git worktree at HEAD^1 whose node_modules were symlinks into the root install (realpath-asserted; core and the dependency tree are untouched by the PR, so the sharing is a clean control). Fixtures used real spawned processes and core's own readProcStartToken/readPidNamespaceId/readLocalBootId; oracles were the command's stdout table (parsed at its fixed column offsets), its --json lines, its stderr, and exit codes. Mutations ran in a third worktree at HEAD against the PR's own two new test files, each restored afterwards. Raw per-cell logs, tallies and harness scripts live in tmp/pr10942-verify-20260904-214918/{logs,lib,*.mjs}; the three evidence PNGs in evidence/. Scratch worktrees tmp/base-tree and tmp/mutant-tree were removed after the cells were captured.

Flakiness gate log

rounds=5 files=2 skipped=0
file packages/cli/src/commands/sessions/managed-rows.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/sessions/managed-rows.test.ts
file packages/cli/src/commands/sessions/ps.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/sessions/ps.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/cli/src/commands/sessions/managed-rows.test.ts: PPPPP
  packages/cli/src/commands/sessions/ps.test.ts: PPPPP

verdict: pass
summary: 2 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/sessions/managed-rows.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/sessions/ps.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/sessions/managed-rows.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/sessions/ps.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/sessions/managed-rows.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/sessions/ps.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/sessions/managed-rows.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/sessions/ps.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/sessions/managed-rows.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/sessions/ps.test.ts: P (exit 0)

Evidence images

01-ab-base-vs-head-sessions-ps

02-pid-identity-matrix-head

03-mutation-matrix-15-of-15-killed

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

…anaged row

The store fails soft on any read or parse error, so a managed row can be
degraded to the sanitized store id and no pid while the session is still
live, and the registry record the merge dedupes against it may be the only
other carrier of what it lost. Carry the record's raw spelling when the row
fell back to the sanitized id, and its verified pid when the row has none,
so `qwen sessions ps` lists a resumable id and a live pid instead of a
sanitized id no consumer can resume and `-`.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtnn5c4cjw

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • namespace guard worker.pidNs != null clause unpinned (managed-rows.test.ts:254) — already reported in the round-5 review body deferral list (review 5117986596)
  • managed-row cwd source choice unpinned (managed-rows.ts:150) — already reported as D2-2 in the round-2 review body deferral list (review 5108278256)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Test Plan (not a blocker): 364 tests passing — this review observed 28375 passed.

Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:

  • packages/cli/src/commands/sessions/managed-rows.ts:134 — [review] Critical [fails-closed] [new-surface] R4-1: (still standing) unvalidated launch.resumeSessionId — a non-string value crashes mergeSessionRows outside readManagedRows' catch a…
  • packages/cli/src/commands/sessions/managed-rows.ts:288 — [probe] R6-2: carry updates sessionId but never row.name — a degraded merged row reports name != sessionId against the module's own invariant
  • packages/cli/src/commands/sessions/managed-rows.ts:283 — [probe] R6-3: spelling-carry guard infers degradation by string equality — an all-lowercase launch spelling is misclassified as degraded and overwritten by a colliding record
  • packages/cli/src/commands/sessions/managed-rows.ts:286 — [probe] R6-4: pid ??= record.pid keys on the sanitized id alone — a sanitized-id collision hands an interloper record's pid to the managed row
  • packages/cli/src/commands/sessions/managed-rows.test.ts:254 — [probe] R6-5: namespace guard accept path (both sides known and equal — the everyday Linux case) exercised by no test; a blanket-reject mutant ships green
  • packages/cli/src/commands/sessions/managed-rows.test.ts:338 — [probe] R6-6: boot-guard rejection side for the host pid candidate pinned by no test; a worker-branch-only guard mutant ships green
  • packages/cli/src/commands/sessions/managed-rows.test.ts:550 — [probe] R6-7: cross-session isolation guard (managed-rows.ts:279) pinned by no test; deletion ships green and leaks an unrelated record's pid
  • packages/cli/src/commands/sessions/managed-rows.test.ts:537 — [probe] R6-8: pid carry's independence from the spelling guard pinned by no test; narrowing the carry to the degradation branch ships green
  • packages/cli/src/commands/sessions/managed-rows.test.ts:535 — [probe] R6-10: launch-spelling overwrite protection pinned by no test; an unconditional-overwrite mutant ships green

Convergence: round 6 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/cli/src/commands/sessions/managed-rows.ts (findings in round 5; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)

Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (1 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):

standing Critical attack surface attacker-dependency blast radius
(each standing Critical)

Advisory only — it does not block this review.

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Test Plan(非阻断):364 tests passing — this review observed 28375 passed

收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改;其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface:未认证任何错误结果,且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中:共 9 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 6 轮发布了 1 条行内评论,其中 1 条是首次提出;上一轮发布了 1 条(其中 1 条首次提出)。发现反复回到同一批文件:packages/cli/src/commands/sessions/managed-rows.ts(第 5 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,可以避免循环反复推导同一组发现;本 PR 的评审已解析为 critical 发布下限。(仅为观察——本轮评审未因此扣留任何内容。)

残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 1 条 Critical),首次发现的速率没有下降(本轮 1,上一轮 1),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Comment thread packages/cli/src/commands/sessions/managed-rows.ts
Picks up integration-tests/chat-transcript-document.test.ts, which the
web-shell E2E Smoke gate runs by name. The branch was 24 commits behind
main and did not have the file, so the gate failed with "No test files
found" rather than on anything in this PR.
@yiliang114

Copy link
Copy Markdown
Collaborator Author

CI attribution for run 33941831702 (head 075b47db4)

1. Test (ubuntu-latest, Node 22.x) — not PR-caused (shared-runner saturation). All suites were green: cli Test Files 1015 passed (1015), core Test Files 640 passed | 1 skipped (641). The only 3 errors in the whole log are vitest's own RPC layer, not test assertions:

Error: [vitest-worker]: Timeout calling "onTaskUpdate"
DFSAMPLE 04:38:42 tmpdir[/var/tmp/qwen-ci-xb086e] load[251.80 241.28 229.28] hosttests[99] space[... 86% /]

Load 251 with ~100 concurrent test processes on the shared ECS host — same saturation lineage as the recent Test-ubuntu failures on #10917. Every file this PR touches (incl. ps.test.ts / managed-rows.test.ts) passed. Re-running this job only.

2. Integration Tests (no-AK) — deterministic failure, flagged for maintainer. 184/185 passed; the single failure reproduces identically across all 3 attempts in ~19ms (an immediate error, not a timeout):

× qwen-live M4 — multi-backend coexistence > runs a task on each backend and reports both in session_list
  → expected 'error' to be 'ok'   (qwen-live-m4-acp-multibackend.test.ts:87 — session_create status for the ACP worker backend)

The test itself comes from main (#10617) and the sibling ACP tests pass, but this PR's diff touches the worker-identity surface (agent-view/supervisor-process.ts workerPidIdentity) in the same functional area as the open R6-1 thread. Not fixing here (this PR is past the +1500-line scope fuse); surfacing so the maintainer can confirm whether the ACP worker session_create refusal interacts with the pid-identity change.

R6-1. The token-less fall-through in `liveWorkerPid` was written for one
class of file — same-machine, written before the identity fields existed,
where a liveness probe was the prior behaviour in the reader's own pid
space. It does not transfer to a file written on another OS, and darwin
and win32 produce exactly that: `readProcStartToken` and
`readPidNamespaceId` return null there permanently, so the file carries
no token and no namespace, clears the namespace and boot-id guards on
those nulls, and reaches a bare `kill(pid, 0)` against a number that
belongs to another OS's pid space.

The shared-`~/.qwen` topology this module's header already names is the
concrete shape: a macOS supervisor's home mounted into a Linux
devcontainer. If any local process holds that number the row prints it in
the table and in `--json`, and a script acting on it hits a stranger.
That is the same "two halves of one table, two evidence standards" defect
this listing was already being fixed for — `listLiveSessions` skips a
foreign-namespace record outright — so it is the same fix, one class
further out.

Pre-platform files keep the documented degradation: `platformValue`
defaults an absent `platform` to `process.platform`
(supervisor-store.ts:1008-1011), so they read as same-platform.

Both test fixtures pinned `platform: 'linux'`. Left alone that would not
have tested the guard, it would have blanked every managed pid on the
macOS runner and failed the suites there instead. Both now use
`process.platform`, and the new case picks a platform that is definitely
not the reader's, asserting the pid is dropped while `isPidAlive` still
reports it live — so the case fails for the right reason and goes red if
the guard is removed.

Typecheck, lint and tests were not run locally; CI is the authority.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

10 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R4-3/R4-5 writer-side pid-identity test gap (supervisor-process.ts:511, supervisor-store.ts:891) — already reported in the round-4 deferral list (review 5114958165)
  • R6-5 namespace guard accept path untested (managed-rows.test.ts:254) — already reported in the round-6 deferral list (review 5119546341)
  • namespace guard pidNs escape hatch unpinned (managed-rows.test.ts:238) — already reported in the round-5 deferral list (review 5117986596)
  • D2-2 managed-row cwd source choice unpinned (managed-rows.ts:150) — already reported in the round-2 deferral list (review 5108278256)
  • R6-10 spelling-preservation guard unpinned (managed-rows.test.ts:535) — already reported in the round-6 deferral list (review 5119546341)
  • docs --json field list omits ipcPath / 'exactly as recorded' vs ipcToken strip (commands.md:790) — already reported in the round-3 deferral list (review 5109073154)
  • R6-2 carryDedupedRecord never carries the record's name (managed-rows.ts:288) — already reported in the round-6 deferral list (review 5119546341)
  • store-failure partial listing exits 0 (ps.ts:159) — already reported in the round-4 deferral list (review 5114958165)
  • managedSessionRows now-parameter dead plumbing (managed-rows.ts:89) — already reported in the round-3 deferral list (review 5109073154)
  • title precedence / initialPrompt rung unpinned (managed-rows.test.ts:368) — already reported in the round-5 deferral list (review 5117986596)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": none of my assigned range went unread (diff lines 393-578 read in full); the managed-rows.test.ts case bodies below diff line 578 belong to another chunk and ….

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Test Plan (not a blocker): 364 tests passing — this review observed 28624 passed.

Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/commands/sessions/ps.ts:205 — [probe] D7-1 complete flag blind to the store's per-entry fail-soft drops
  • packages/cli/src/agent-view/protocol.ts:139 — [probe] D7-2 clearAgentViewWorkerPids orphans the new identity tokens
  • packages/cli/src/commands/sessions/managed-rows.ts:151 — [probe] D7-3 taskState has no liveness gate; stale working/needs-input survives an unclean death
  • packages/cli/src/commands/sessions/ps.ts:159 — [probe] D7-4 fail-soft catch's stderr note destroys the listing when stderr is broken (fix: ignoreBrokenPipe)
  • docs/users/features/commands.md:785 — [probe] D7-5 documented PID - rule contradicts the code in both directions
  • packages/cli/src/agent-view/supervisor-process.ts:97 — [probe] D7-6 supervisor's own pid gates ignore the identity contract this diff adds (class finding, 7 sites)
  • packages/cli/src/agent-view/supervisor-store.ts:891 — [probe] D7-7 platformValue allowlist narrower than NodeJS.Platform defeats the new cross-OS guard
  • packages/cli/src/commands/sessions/managed-rows.ts:144 — [probe] D7-8 unreachable !presentation.title clause — dead branch in a new file
  • packages/cli/src/commands/sessions/managed-rows.ts:339 — [probe] D7-9 N:1 dedupe collapse makes one of two live same-id processes vanish
  • packages/cli/src/commands/sessions/managed-rows.ts:296 — [probe] D7-10 pid carry undoes a provenance refusal, misattributing a local interactive pid
  • packages/cli/src/commands/sessions/ps.ts:107 — [probe] D7-11 unreadable sessionState laundered into a confident 'failed' verdict
  • packages/cli/src/commands/sessions/ps.test.ts:470 — [probe] D7-12 new empty-listing test duplicates the pre-existing one; added mock line inert
  • packages/cli/src/commands/sessions/managed-rows.test.ts:578 — [probe] D7-13 dedupe→emitter seam unpinned: record===undefined never asserted
  • packages/cli/src/commands/sessions/ps.ts:197 — [probe] D7-14 deduped managed row drops record-only fields, falsifying the docs messaging promise (class finding)
  • packages/cli/src/commands/sessions/managed-rows.ts:334 — [probe] D7-15 dedupe suppression key taken from an untrusted launch-file field
  • packages/cli/src/commands/sessions/ps.ts:44 — [probe] D7-16 STATE column squeezes the never-truncated DIRECTORY column below 80 columns
  • docs/users/features/commands.md:804 — [probe] D7-17 managed-row JSON field list written unconditionally for optional fields
  • .qwen/skills/find-simplifications/references/survey.md:151 — [probe] D7-18 new external-reference probe mis-measures on this repo's relative-import style

Convergence: round 7 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/cli/src/commands/sessions/managed-rows.ts (findings in round 6; 3 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)

Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (4 Critical(s)), the rate of first-time findings is not falling (this round 4, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):

standing Critical attack surface attacker-dependency blast radius
(each standing Critical)

Advisory only — it does not block this review.

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 10 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 5)"none of my assigned range went unread (diff lines 393-578 read in full); the managed-rows.test.ts case bodies below diff line 578 belong to another chunk and …

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

Test Plan(非阻断):364 tests passing — this review observed 28624 passed

收敛姿态下延后(第 7 轮,非阻断)——已记录,本轮不要求修改:共 18 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 7 轮发布了 4 条行内评论,其中 4 条是首次提出;上一轮发布了 1 条(其中 1 条首次提出)。发现反复回到同一批文件:packages/cli/src/commands/sessions/managed-rows.ts(第 6 轮已出过发现,本轮又有 3 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,可以避免循环反复推导同一组发现;本 PR 的评审已解析为 critical 发布下限。(仅为观察——本轮评审未因此扣留任何内容。)

残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 4 条 Critical),首次发现的速率没有下降(本轮 4,上一轮 1),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。

— qwen3.8-max via Qwen Code /review (v0.23.0)

return [
...managed.map((row) => carryDedupedRecord(row, records)),
...records
.filter((record) => !managedIds.has(sanitizeSessionId(record.sessionId)))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R7-1: [certifies-falsely] [new-surface] The merge dedupe treats canonical-id equality as session identity, but sanitizeSessionId is not injective over real session ids: uppercase-hex ids are representable (SESSION_FILE_PATTERN admits [0-9a-fA-F-]{32,36}), the native session store is case-sensitive, adoption accepts any non-empty raw id, and --session-id validates with a case-insensitive regex whose case-twin guard blocks only pre-existing twins. A registry record belonging to a DIFFERENT live session whose id merely canonicalizes onto a managed row's id is therefore silently dropped from the listing, its listLiveSessions-verified pid is carried onto the managed row via pid ??= record.pid, and a degraded managed row can even take the interloper's raw resumable spelling. Concretely: session Managed-1 is adopted (store directory managed-1, row emits Managed-1 from launch.resumeSessionId) while a distinct live interactive session holds id MANAGED-1 — the filter drops that record, so MANAGED-1 appears in neither the table nor --json while its process is alive (the silent omission this command exists to prevent), and its verified pid rides the managed row, so a --json consumer that kills by pid targets the wrong process. This is the class-level finding for the sanitize-collision family; it absorbs the pid-carry arm recorded as R6-4 in the round-6 deferral list.

Witness:

Probe against the real managed-rows.ts (scratch tree):
INTACT: F2-A rows = [{"sessionId":"Managed-1","pid":999,"managed":true}]
        F2-A interloper listed? false | managed row pid: 999
        F2-C degraded row = [{"sessionId":"MANAGED-1","pid":999,"managed":true}]
FIXED (exact raw-spelling match first; sanitized fallback only while the
       row itself reports the sanitized form):
        rows = [{"sessionId":"Managed-1","managed":true},
                {"sessionId":"MANAGED-1","pid":999,"managed":false}]
        interloper listed? true | managed row pid: undefined
        same-session mixed-case dedupe unchanged; all 34 managed-rows tests green under the fix

Suggested fix (two cooperating sites, so no one-click block): in mergeSessionRows/carryDedupedRecord, dedupe a record by exact match against the managed row's known raw spelling (launch.resumeSessionId ?? state.sessionId — adoption always writes resumeSessionId, supervisor-process.ts:649), falling back to the sanitized comparison only when the row itself reports the sanitized form.

The fix must not regress to raw-string comparison in the fallback case — this module's own comment: "Comparing the spellings would let any managed session whose id contains an uppercase letter through this filter, and the table would list it twice", and the mixed-case same-session dedupe pinned at managed-rows.test.ts:526/:547 must keep working. Please add a managed-rows.test.ts case merging record({ sessionId: 'MANAGED-1', pid: 999 }) with a managed row reporting Managed-1 (store id managed-1), asserting two rows — the managed row without pid 999 and the interactive MANAGED-1 row with it — then remove the raw-spelling-first match and confirm that test goes red.

中文说明

R7-1:[certifies-falsely][new-surface] 合并去重把「规范化后 id 相等」当成了 session 同一性,但 sanitizeSessionId 对真实 session id 不是单射:大写十六进制 id 是合法可表示的(SESSION_FILE_PATTERN 接受 [0-9a-fA-F-]{32,36}),原生 session 存储大小写敏感,收养(adoption)接受任意非空原始 id,--session-id 用大小写不敏感的正则校验、其 case-twin 守卫只拦截已存在的孪生 id。因此,属于另一个存活 session、id 恰好规范化后与某个 managed 行相同的注册表记录会被静默丢弃:它经 listLiveSessions 验证过的 pid 通过 pid ??= record.pid 被搬到 managed 行上,降级行甚至会采用闯入者的原始可恢复拼写。具体场景:session Managed-1 被收养(存储目录 managed-1,行从 launch.resumeSessionId 输出 Managed-1),同时另一个存活的 interactive session 持有 id MANAGED-1 —— 过滤器丢弃后者的记录,于是 MANAGED-1 在表格和 --json 中都消失,而它的进程还活着(这正是本命令要防止的静默遗漏),它已验证的 pid 却出现在 managed 行上,按 pid 执行 kill 的 --json 消费者会命中错误的进程。本条是 sanitize 碰撞族的类级发现,吸收了第 6 轮延后清单中记录的 R6-4(pid 搬运分支)。

(证据见上方 probe:未修复时存活的 MANAGED-1 记录消失、其 pid 999 出现在 managed 行上;按建议修复后闯入者正常列出、managed 行不再携带其 pid,同 session 的混合大小写去重不受影响,34 个测试全绿。)

建议修复(涉及两处配合,不用一键 suggestion 块):在 mergeSessionRows/carryDedupedRecord 中,先用 managed 行已知的原始拼写(launch.resumeSessionId ?? state.sessionId —— 收养总会写入 resumeSessionId,supervisor-process.ts:649)做精确匹配去重,仅当行本身报告的是净化形式时才回退到净化比较。

修复不得在回退分支退回原始字符串比较 —— 本模块自己的注释写明:「比较拼写会让任何 id 含大写字母的 managed session 绕过过滤器,表格会把它列两次」;managed-rows.test.ts:526/:547 钉住的同 session 混合大小写去重必须继续通过。请补一个 managed-rows.test.ts 用例:把 record({ sessionId: 'MANAGED-1', pid: 999 }) 与报告 Managed-1(存储 id managed-1)的 managed 行合并,断言输出两行 —— managed 行不携带 pid 999,interactive 行 MANAGED-1 携带 —— 然后移除「原始拼写优先」的匹配并确认该测试变红。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified real at head 294cbe2cc9. Every link in the premise chain holds in the tree:

  • sanitizeSessionId (packages/cli/src/agent-view/protocol.ts:272-280) lowercases, so it is not injective; SESSION_FILE_PATTERN (packages/core/src/services/sessionService.ts:419) admits [0-9a-fA-F-]{32,36}, so both spellings are persistable in the native store.
  • Adoption accepts any non-empty raw id: requireSessionId (supervisor-process.ts:4161-4167) checks only typeof/length, and parseAdoptParams adds just a startsWith('-') guard (:4206) before storing the sanitized form as the key (:4211) and the raw spelling as resumeSessionId (:4223).
  • The --session-id occupancy check is case-insensitive but consults only already-persisted transcripts (findSessionIdIgnoringCase, config/config.ts:2052) and is skipped entirely when sessionIdGenerated (:2050) — so it blocks pre-existing twins only, exactly as described.

Consequence at this head: mergeSessionRows builds managedIds from sanitized row ids (managed-rows.ts:334) and drops every registry record whose sanitized id lands in that set (:339), so the interloper disappears from both the table and --json even though listLiveSessions verified it live. carryDedupedRecord then moves its pid onto the managed row unconditionally inside the match loop (pid ??= record.pid, :298), and when the managed row is degraded to the sanitized store id the :295 arm also hands the row the interloper's raw resumable spelling — an id that resumes the wrong session. There is no compensating gate in the caller: mergeSessionRows runs at ps.ts:174, outside readManagedRows' fail-soft catch.

Fix shape: match the exact raw spelling first and use the sanitized comparison only as a fallback while the managed row itself reports the sanitized form (both at :334/:339 and at :291/:295), and do not carry a pid or a spelling across a sanitize-only match. No code lands this round — the PR is +1582/-78, over this sweep's 1500-addition scope fuse, so this stays unresolved for a follow-up pass.

Comment on lines +2361 to 2363
...workerPidIdentity(host),
...(host.hostId ? { hostId: host.hostId } : {}),
hostEndpoint: worker.hostEndpoint,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R7-2: [certifies-falsely] [new-surface] The reconnect write site re-stamps the worker file's pid identity with pids an already-running PTY host self-reported over its socket, while workerPidIdentity stamps the RECONNECTING supervisor's readPidNamespaceId() and reads readProcStartToken for those numbers from its own /proc — vouching for pids it never verified live in its own namespace, and the merge-style write overwrites the launcher's correct foreign pidNs. The three spawn sites are safe (a plain local spawn shares the writer's namespace); only reconnect reaches a host that may live in another namespace, which defeats the cross-namespace guard these fields exist for. Concretely: a devcontainer bind-mounts ~/.qwen; the container's supervisor spawns a session's detached PTY host (it survives its parent) and then dies (SIGKILL/OOM); supervisor handoff is socket-reachability only (no namespace check), so a host-side qwen spawns its own supervisor, reaches the orphan host through the shared socket, and this site rewrites the identity — pidNs becomes the host namespace and the tokens are read for low container pid numbers that on the host resolve to long-lived system daemons. qwen sessions ps on the host then passes every liveWorkerPid guard and prints that unrelated daemon's pid as the session's live worker pid — a number the docs invite the user to kill — while the real worker sits invisible in the container namespace.

Witness:

Probe driven end-to-end through the real handler (stub socket peer answering
the product's own status protocol; real reconnect → real write → real store →
real managedSessionRows. Declared model: the real container topology was not
exercised — this machine cannot create a PID namespace):
INTACT:  before: worker.json pidNs = 4026531837 (launcher's, foreign); row = no pid
         after handler.logs(): pidNs = 4026531836 (writer's own);
         row = {"name":"Sess-C","pid":2}
         pid 2 in THIS namespace is: kthreadd
         (mis-stamped tokens for pid 1 and pid 2 came out identical — kernel
          threads share a starttime — so the false vouch is permanently stable)
FIX ARM (identity spread removed at :2361 only):
         launcher's identity preserved (pidNs = 4026531837); row = no pid
Suggested change
...workerPidIdentity(host),
...(host.hostId ? { hostId: host.hostId } : {}),
hostEndpoint: worker.hostEndpoint,
...(host.hostId ? { hostId: host.hostId } : {}),
hostEndpoint: worker.hostEndpoint,

Omitting the spread suffices because writeAgentViewWorker merges {...existing, ...worker} (supervisor-store.ts:525-529), so the original launcher's identity — valid in its namespace, describing the same live processes — survives and the namespace guard keeps working; alternatively have the host's status reply report its own namespace id and stamp identity only when it matches readPidNamespaceId(). The fix must not write explicit nulls over a valid recorded token: normalizeWorker coerces absent token fields to null (supervisor-store.ts:891-893), so omission — not nulling — is the shape that preserves the original identity. Please add a supervisor-process.test.ts case for reconnectSessionHost asserting the post-reconnect worker file keeps the original writer's pidNs/hostProcStart/workerProcStart (or omits identity) rather than the reconnecting process's own, then restore the unconditional ...workerPidIdentity(host) at this site and confirm the test goes red.

中文说明

R7-2:[certifies-falsely][new-surface] 重连(reconnect)写入点会用「已在运行的 PTY host 通过 socket 自报的 pid」重新盖章 worker 文件的 pid 身份,而 workerPidIdentity 盖的是重连方 supervisor 自己的 readPidNamespaceId(),并从它自己的 /proc 读取这些编号的 readProcStartToken —— 为它从未在自己命名空间里验证过存活的 pid 背书,且 merge 式写入会覆盖启动方原本正确的外来 pidNs。三个 spawn 写入点是安全的(本地 spawn 的子进程与写入方共享命名空间);只有 reconnect 会连接到可能位于另一个命名空间的 host,从而破坏这些字段本要防御的跨命名空间守卫。具体场景:devcontainer 以 bind-mount 挂载 ~/.qwen;容器内 supervisor 启动了会话的 detached PTY host(host 比父进程长寿)后被 SIGKILL/OOM;supervisor 接管只看 socket 可达性(无命名空间检查),于是宿主机侧的 qwen 启动自己的 supervisor,通过共享 socket 连上孤儿 host,此写入点重写身份:pidNs 变成宿主命名空间,token 按容器内的小 pid 编号在宿主的 /proc 里读出 —— 那些编号在宿主上对应长寿的系统守护进程。宿主机上的 qwen sessions ps 随后通过 liveWorkerPid 的全部守卫,把这个无关守护进程的 pid 当作该会话的存活 worker pid 打印出来(文档正是邀请用户去 kill 这个数字),而真正的 worker 在容器命名空间里不可见。

(证据见上方 probe:通过真实 handler 端到端驱动。未修复时 worker.json 的 pidNs 被改写为重连方自己的命名空间、行输出 pid 2 = kthreadd,且错误盖章的 token 稳定复现;仅移除 :2361 的身份 spread 后,启动方身份得以保留、行不再输出 pid。声明:真实容器拓扑未能实际执行 —— 本机无法创建 PID 命名空间,外方 host 用遵循产品自身 status 协议的 stub socket 对端建模。)

采用上方 suggestion 块即可:省略该 spread 就够了,因为 writeAgentViewWorker{...existing, ...worker} 合并(supervisor-store.ts:525-529),启动方原本的身份(在其命名空间内有效、描述同一批存活进程)会保留下来,命名空间守卫继续有效;或者让 host 的 status 应答报告自己的命名空间 id,仅当其与 readPidNamespaceId() 一致时才盖身份。修复不得用显式 null 覆盖已记录的有效 token:normalizeWorker 会把缺失的 token 字段规范化为 null(supervisor-store.ts:891-893),所以「省略字段」而非「写 null」才是保留原身份的形态。请补一个 supervisor-process.test.tsreconnectSessionHost 用例,断言重连后的 worker 文件保留原写入方的 pidNs/hostProcStart/workerProcStart(或不含身份字段),而不是重连进程自己的;然后在同一位置恢复无条件的 ...workerPidIdentity(host) 并确认该测试变红。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mechanism verified at head 294cbe2cc9. This one is a trust-boundary decision rather than a patch, so it stays unresolved.

What the code does:

  • workerPidIdentity (supervisor-process.ts:105-119) stamps pidNs: readPidNamespaceId() — the writer's namespace — and reads readProcStartToken for host.pid/host.workerPid out of the writer's own /proc.
  • At the reconnect site the write is :2357-2370 (identity spread at :2361), and those two pids are not the writer's children: connectAgentViewPtyHostProcess (pty-host-process.ts:171-197) takes them from the peer's status reply (:1008, :1012-1025), which the host answers with its own process.pid (:882-883).
  • The written object is a fresh literal that carries hostEndpoint, hostAuthToken and recentOutputBytes forward from the file read at :2342 but deliberately not pidNs, so the launcher's namespace stamp is replaced by the writer's. The only peer check before the write is expectedHostId (:2353), which proves "same host process", not "same namespace".
  • The finding's scoping also holds: the other three write sites (:511, :743, :2559) each follow a local spawn, so only reconnect can reach a host in a foreign namespace.

Decision owed: whether a reconnecting supervisor may write pid identity at all for a peer whose namespace it cannot observe. The candidate shapes have different costs and none is obviously right — (a) carry the old file's pidNs/procStart forward and refresh only endpoint/auth, which leaves a legitimate same-namespace reconnect with stale tokens and can blank a real pid; (b) skip the refresh unless the peer is provably local, which needs a notion of "local" the status protocol does not currently carry; (c) extend status so the host reports its own pidNs and compare before writing, which is a protocol change with an old-host compatibility question. Downstream also interacts with the row-admission question raised separately on managed-rows.ts:111.

No code lands this round regardless: the PR is over this sweep's 1500-addition scope fuse.

now: number = Date.now(),
): SessionRow[] {
return snapshots
.filter((snapshot) => snapshot.state.ownership === 'managed')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R7-3: [certifies-falsely] [new-surface] The managed half of the listing admits rows with no machine or namespace identity gate — the only filter is ownership === 'managed' — while the interactive half of the SAME table explicitly refuses foreign records (record.pidNs !== ownNamespace plus boot-prefix skips in listLiveSessions). getAgentViewStorePaths keys the jobs directory on globalDir alone and listAgentViewSessionSnapshots returns every readable job dir, so in a shared ~/.qwen — an NFS home or a bind-mounted devcontainer home, the topology this diff's own protocol.ts comment and the registry's guard comments name — the command lists sessions running on another machine while the docs line this PR adds (commands.md:755) claims "running on this machine right now". The identity this diff added is spent solely on blanking the pid inside liveWorkerPid, never on row admission: a foreign managed session is printed with its real title, real cwd and STATE needs input, PID -, and --json emits taskState: "waiting" — so the diff's own jq example (select(.taskState == "waiting") | .name) hands a script the name of a session waiting on a machine it cannot reach, in a listing documented as this machine's. Pre-PR this command made no such claim (zero agent-view references at the merge base).

Witness:

Probe S1 — real ps on one fabricated shared home holding (a) a managed job
with a foreign pidNs (4026531837 vs local 4026531836) + foreign boot prefix
and (b) an interactive registry record from the same foreign namespace:
PR table: HOST session waitin…  -  0s  needs input  /host/w/app
PR json : {"name":"HOST session waiting for an answer","cwd":"/host/w/app",
           "taskState":"waiting","sessionId":"host-managed","managed":true}
PR      : foreign interactive record NOT listed (registry guards refused it)
BASE    : ps.ts agent-view references → 0 (the claim is new in this diff)
Fix-cost measurement: a strict known-foreign row gate flipped S1 to an empty
listing but broke 5 of this PR's own tests (each pins keep-the-row,
blank-the-pid) and is ineffective while the platformValue coercion stands
(see the deferred platform-allowlist entry) — a design change, not a one-liner.

Suggested fix: either gate the row the way the registry gates records — drop a snapshot whose worker.pidNs is known and differs from readPidNamespaceId(), or whose token boot prefix names another boot — keeping worker-less rows and rewording commands.md:755's "on this machine" contract for them; or state on the row and in the docs that a managed row's locality is unverified. The docs+marking arm is the only complete non-breaking option measured here.

The gate cannot rely on pidNs alone: two machines sharing one ~/.qwen both live in the initial PID namespace, whose inode is a kernel constant, so the boot prefix is the only cross-machine identity (managed-rows.ts:193-197) — and the fields are optional by design (protocol.ts:139; normalizeWorker defaults an absent value to null), so a pre-identity worker file and any worker-less managed session carry no machine identity and must not be dropped by a strict gate. Please add a managed-rows.test.ts case with a managed snapshot whose worker.pidNs differs from the reader's (and a second with a foreign boot prefix) asserting whichever contract is chosen — no row, or a row marked locality-unverified — red when that behaviour is removed, updating the five keep-row-blank-pid tests consistently with the chosen contract.

中文说明

R7-3:[certifies-falsely][new-surface] 列表的 managed 半边在收录行时没有任何机器/命名空间身份门 —— 唯一的过滤是 ownership === 'managed' —— 而同一张表的 interactive 半边明确拒绝外来记录(listLiveSessionsrecord.pidNs !== ownNamespace 与 boot 前缀跳过)。getAgentViewStorePaths 只按 globalDir 定位 jobs 目录,listAgentViewSessionSnapshots 返回每个可读的 job 目录,因此在共享 ~/.qwen(NFS 主目录、bind-mount 的 devcontainer 主目录 —— 本 diff 自己的 protocol.ts 注释与注册表守卫注释都点名了这一拓扑)下,命令会列出运行在另一台机器上的会话,而本 PR 新增的文档行(commands.md:755)声称「running on this machine right now」。本 diff 添加的身份信息只被用于在 liveWorkerPid 内部抹掉 pid,从不用于行的收录:外来 managed 会话带着真实标题、真实 cwd 和 STATE needs input、PID - 被打印,--json 输出 taskState: "waiting" —— 于是本 diff 自己给出的 jq 示例(select(.taskState == "waiting") | .name)会把一个「等待在够不到的机器上」的会话名交给脚本,而列表文档声称这是本机列表。PR 之前本命令没有这个声明(merge base 上 ps.ts 对 agent-view 零引用)。

(证据见上方 probe S1:同一外来命名空间的 interactive 注册表记录被注册表守卫正确拒绝,而外来 managed 会话被完整列出;BASE 臂确认该声明是本 diff 新增。修复成本实测:严格的「已知外来即拒收」行门能让 S1 变空,但会破坏本 PR 自己的 5 个测试 —— 它们都钉住「保留行、抹掉 pid」的契约 —— 且在 platformValue 强制转换存在时对部分平台无效(见延后的平台白名单条目):这是设计级改动,不是一行修复。)

建议修复:要么像注册表那样对行做门控 —— 丢弃 worker.pidNs 已知且与 readPidNamespaceId() 不同、或 token boot 前缀指向另一次启动的快照 —— 同时保留无 worker 的行并相应改写 commands.md:755 的「本机」契约;要么在行与文档中明确 managed 行的本机性未经验证。实测之下,「文档+标注」分支是唯一完整且不破坏现有测试的选项。

门控不能只靠 pidNs:共享一个 ~/.qwen 的两台机器都位于初始 PID 命名空间,其 inode 是内核常量,boot 前缀才是唯一的跨机器身份(managed-rows.ts:193-197);且这些字段按设计是可选的(protocol.ts:139;normalizeWorker 把缺失值规范为 null),因此身份字段出现之前的 worker 文件、以及任何没有 worker 文件的 managed 会话都不携带机器身份,严格门控不得丢弃它们。请补一个 managed-rows.test.ts 用例:managed 快照的 worker.pidNs 与读取方不同(外加一个 boot 前缀为外来的用例),断言所选契约 —— 不输出行,或输出「本机性未验证」的行 —— 移除该行为时变红,并按所选契约同步更新那 5 个「保留行、抹掉 pid」的测试。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at head 294cbe2cc9, including the fix-cost tension — this is a product call, so it stays unresolved.

The asymmetry is real and the two halves of one table are gated differently:

  • Managed admission is ownership === 'managed' and nothing else (managed-rows.ts:111). No pidNs, boot-id or machine check anywhere in managedSessionRows.
  • The interactive half gates hard: listLiveSessions returns early on record.pidNs !== ownNamespace (packages/core/src/services/session-registry.ts:541) and again on a foreign boot prefix (:553-557).
  • The store cannot supply the identity: getAgentViewStorePaths keys jobsDir on globalDir alone (supervisor-store.ts:57-69) and listAgentViewSessionSnapshots (:361) returns every readable job dir.
  • The namespace/boot-id identity this diff added is spent only inside liveWorkerPid (managed-rows.ts:198-239, guards at :213 and :218) — on blanking the pid, never on row admission. So a foreign managed session is listed with real title, real cwd and a live task state, with PID as -.
  • The doc claim is new in this PR: docs/users/features/commands.md:755 "Lists the Qwen Code sessions running on this machine right now." git grep for that phrase at merge base e3d26283e6 returns nothing.

Confirmed the tension the probe measured. A strict known-foreign row gate contradicts behaviour this PR pins in its own tests: managed-rows.test.ts:236 (refuses pids from a worker file written in another PID namespace), :288, :311, :341 and :364 each construct a worker file with foreign identity and assert the row survives with pid undefined. That is five tests pinning keep-the-row/blank-the-pid, and it is the deliberate design stated in the liveWorkerPid docblock ("an unreadable namespace on either side must not blank a real worker's pid").

Decision owed: pick one and make the other half consistent — either the listing admits foreign managed sessions, in which case commands.md:755 has to be reworded and --json should carry the fact that a row is not local (the documented select(.taskState == "waiting") example currently hands a script an unreachable session), or it gates them out, in which case the degrade-don't-blank design and those five pinned tests change. Not settleable by a patch, and no code lands this round anyway (PR is over the sweep's 1500-addition scope fuse).

// an id that cannot resume the session. Store keying stays
// untouched; only the reported value changes.
const sessionId =
snapshot.launch?.resumeSessionId ?? snapshot.state.sessionId;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R4-1: [certifies-falsely] [new-surface] (still standing) launch.resumeSessionId is consumed here as a trusted string, but normalizeLaunch passes it through the ...raw spread unvalidated — the only launch text field this new consumer reads that escapes stringValue — so a well-formed-but-wrong-typed launch.json crashes qwen sessions ps in mergeSessionRows, OUTSIDE readManagedRows' fail-soft catch, or yields a silently wrong empty-id row. This entry was deferred in rounds 5-6 as fails-closed on new surface; the probe below shows the empty-string arm CERTIFIES FALSELY (exit 0, parseable stdout, a row no consumer can act on or identify), so the deferral gate is no longer met and the finding posts. A launch.json containing "resumeSessionId": 123 — tampering (the exact input normalizeLaunch's own comment says it defends against), a hand-edit, a backup restore or sync-tool corruption — makes the row take sessionId = 123 (?? does not fall back for non-nullish); handlePs then calls mergeSessionRows outside the try/catch, where sanitizeSessionId runs .replace on the number: the command exits 1 printing ZERO rows, including the already-fetched verified registry records, defeating the documented degrade-and-still-list contract. "resumeSessionId": "" is the non-crashing arm: exit 0, stdout parseable, "sessionId":"" and "name":"" presented as a normal listing entry. The product's own write path is clean (requireSessionId enforces a string); the trigger is an external writer of the durable file.

Witness:

Probe A2 — real qwen sessions ps --json, identical store except the one JSON
value, with a healthy live registry record present (produced by the real
registerSession in a live process):
CONTROL "resumeSessionId":"Managed-A" → EXIT=0, STDOUT 2 lines
PR CODE "resumeSessionId":123         → EXIT=1, STDOUT 0 lines
  stderr: TypeError: sessionId.replace is not a function
    at sanitizeSessionId (protocol.ts:274)
    at mergeSessionRows (managed-rows.ts:334)
    at handlePs (ps.ts:174)
  <- the verified interactive record was fetched and never printed
PR CODE "resumeSessionId":""          → EXIT=0, STDOUT 2 lines, one being
  {"name":"","startedAt":1788523080000,"cwd":"/w/app","taskState":"running","sessionId":"","managed":true}
FIX ARM (resumeSessionId: stringValue(raw['resumeSessionId']) in normalizeLaunch)
  123 → EXIT=0, 2 lines, row falls back to the sanitized directory id
  ""  → EXIT=0, 2 lines, same fallback

Suggested fix (in normalizeLaunch, supervisor-store.ts — a different file, so a plain block rather than a one-click suggestion at this line):

return stripUndefined({
  ...raw,
  schemaVersion: 1,
  sessionId,
  resumeSessionId: stringValue(raw['resumeSessionId']),
  // ...the remaining fields unchanged
}) as AgentViewLaunchFile;

stringValue is typeof value === 'string' && value.length > 0 ? value : undefined (supervisor-store.ts:1029) and is already applied to every other string field in normalizeLaunch; the fix must not change store keying, whose source of truth is the sanitized directory name (supervisor-store.ts:793-795). Please add a ps.test.ts case resolving listAgentViewSessionSnapshots with a managed snapshot whose launch.resumeSessionId is 123 as unknown as string plus one registry record, asserting the registry row still prints and the handler does not throw, and a managed-rows.test.ts case with the same fixture asserting row.sessionId equals state.sessionId — then remove the coercion and confirm both go red.

中文说明

R4-1:[certifies-falsely]new-surface此处把 launch.resumeSessionId 当作可信字符串消费,但 normalizeLaunch 让它经 ...raw 展开原样通过 —— 它是这个新消费者读取的启动文件文本字段中唯一逃过 stringValue 的 —— 因此一个格式合法但类型错误的 launch.json 会让 qwen sessions psmergeSessionRows 里崩溃,而该调用位于 readManagedRows 的 fail-soft catch 之外;或者产出一个静默错误的空 id 行。本条目在第 5、6 轮曾按「fails-closed + new-surface」延后;下方 probe 表明空字符串分支是认证错误结果(exit 0、stdout 可解析、一行任何消费者都无法使用甚至无法识别的数据),延后门槛不再满足,故本轮发布。launch.json"resumeSessionId": 123 时 —— 篡改(normalizeLaunch 自己的注释写明它就是要防御这种输入)、手工编辑、备份恢复或同步工具损坏 —— 行会取 sessionId = 123(?? 对非 nullish 不回退);handlePs 在 try/catch 之外调用 mergeSessionRows,其中 sanitizeSessionId 对数字执行 .replace:命令以 exit 1 结束、零行输出,连已经取到并验证过的注册表记录也一起丢失,破坏了文档承诺的「降级也要列出」契约。"resumeSessionId": "" 是不崩溃的分支:exit 0、stdout 可解析、"sessionId":"""name":"" 被当作正常列表行输出。产品自身的写入路径是干净的(requireSessionId 强制字符串);触发者是持久文件的外部写入方。

(证据见上方 probe A2:对照臂两行输出;123 臂 exit 1、零行、TypeError 栈止于本 PR 新增的文件与新增行;空串臂输出一行空 id/空名的「正常」数据;在 normalizeLaunch 中加 stringValue 强制转换后,两种输入都回退到净化目录 id,exit 0、两行。)

建议修复(位于 supervisor-store.ts 的 normalizeLaunch,与本行不同文件,故用普通代码块而非一键 suggestion):在返回对象中加 resumeSessionId: stringValue(raw['resumeSessionId']),,其余字段不变。stringValuetypeof value === 'string' && value.length > 0 ? value : undefined(supervisor-store.ts:1029),normalizeLaunch 的其他所有字符串字段都已使用它;修复不得改动存储键控 —— 其唯一事实来源是净化后的目录名(supervisor-store.ts:793-795)。请补一个 ps.test.ts 用例:让 listAgentViewSessionSnapshots 返回一个 launch.resumeSessionId123 as unknown as string 的 managed 快照外加一条注册表记录,断言注册表行仍然打印且 handler 不抛错;再补一个 managed-rows.test.ts 用例,同 fixture 断言 row.sessionId 等于 state.sessionId —— 然后移除该强制转换并确认两个测试变红。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still standing at head 294cbe2cc9 — verified end to end by reading, and the crash really is outside the fail-soft boundary. Stays unresolved.

  • normalizeLaunch (supervisor-store.ts:789-813) spreads ...raw and overrides sessionId, argv, env, entrypoint, initialPrompt, projectCwd, activeCwd, includeDirectories and terminal. resumeSessionId is not in that list — it is the one launch text field this PR's new consumer reads that escapes coercion.
  • The consumer takes it with ?? (managed-rows.ts:134), so a non-nullish non-string (123) becomes the row's sessionId and never falls back to state.sessionId.
  • readManagedRows' try/catch (ps.ts:155-168) wraps only managedSessionRows, which never calls sanitizeSessionId. mergeSessionRows is called at ps.ts:174, and it reaches sanitizeSessionId(row.sessionId) at managed-rows.ts:334sessionId.replace at protocol.ts:273. Exit 1 with zero rows, including the registry records listLiveSessions already verified — the documented degrade-and-still-list contract is what breaks.
  • Empty-string arm confirmed too: "" ?? x is "", and sanitizeSessionId("") returns '_' (protocol.ts:279), so the row is emitted at exit 0 with sessionId: "" and --json consumers get an entry nothing can act on; as a side effect any record canonicalizing to '_' is also dropped by the :339 filter.

The fix is one line and closes both arms: route resumeSessionId through stringValue in normalizeLaunch. That helper returns undefined for a non-string and for an empty string (supervisor-store.ts:1029-1031), and stripUndefined (:872-876) drops it, so ?? falls back to state.sessionId in both cases and the consumer needs no guard of its own.

No code lands this round: the PR is +1582/-78, over this sweep's 1500-addition scope fuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants