feat(core): address other sessions by name from send_message and list_agents - #8733
feat(core): address other sessions by name from send_message and list_agents#8733qqqys wants to merge 25 commits into
Conversation
Records each interactive session at `~/.qwen/sessions/<pid>.json` while it runs, so "which Qwen Code sessions are on this machine right now" is one readdir instead of a walk over every project's transcript directory. This is the discovery surface that cross-session messaging needs (QwenLM#8724), landed on its own because it is useful by itself and changes nothing about how a session behaves. Why not extend the existing runtime.json sidecar: it lives under `<projectDir>/chats/<sessionId>.runtime.json`, so enumeration costs a read per *historical* session and grows with transcript history; and it is deliberately never deleted, so its presence carries no liveness signal. The two now coexist — runtime.json stays the stable, kimi-compatible "which session is PID X serving" sidecar for external observers. Staleness is decided by PID liveness plus a start-time token read from /proc, so a recycled PID cannot resurrect a dead session's record. The new `process-liveness` helpers replace the private copy in teamHelpers. Registry hygiene worth calling out: the directory is chmod 0700 on every register (mkdir's mode is umask-masked and does nothing for an existing directory), records are 0600, and only `<digits>.json` is ever considered a record — a lenient prefix match would read `2026-planning-notes.json` as PID 2026 and delete a file this code never wrote. `qwen sessions ps` prints the live sessions; `--json` emits JSON Lines. It sits next to `qwen sessions list`, which walks saved transcripts and answers the other question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second step of QwenLM#8724. A session can now be reached by another session on the same machine, and every message that arrives is gated before the model can act on it. Transport is one UNIX domain socket per session, NDJSON over the wire, one frame per line, connection dropped past 1 MiB without a newline. The socket directory is 0700 and the socket 0600, and that is the whole access-control story: Node cannot read SO_PEERCRED without a native addon, so a frame's claimed `from` is not authenticated. Everything downstream assumes that. The gate is the reason the transport and the policy land together. With an explicit `agents.crossSessionInbound` setting the user decides (accept / hold / refuse). Unset, the policy follows approval-mode parity: a message auto-delivers only when acting on it cannot do more than the sender could already have done itself. A prompting receiver accepts freely because each action still faces its own gate; a YOLO receiver has no such backstop, so a sender that is prompting — or that asserts nothing — is parked for review. An unreadable mode holds. Held messages are settled, never stranded: the buffer is bounded and evicts oldest as expired, shutdown expires the rest, and every terminal outcome (held / denied / expired / delivered) goes back to the sender as a control frame. Without receipts a sender cannot tell "parked" from "delivered and ignored". Content reaches the model inside a <cross_session_message> envelope with the delimiter defanged in the body, so a peer cannot close the envelope early and forge one attributed to the user. The envelope carries a fixed notice that a peer holds none of the user's authority, and the auto-mode classifier gains the matching rule. Those two plus the sender-side prompt have to agree — the specific failure this blocks is a session that was denied an action asking a second session to run it. `/peers` lists and releases held messages; without it, holding would be indistinguishable from dropping. Off by default behind `agents.crossSessionMessaging`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
startInteractiveUI now calls registerSession({ sessionId:
config.getSessionId(), cwd: config.getTargetDir(), ... }), but the mock
Config objects in gemini.test.tsx predate it and expose neither getter,
so all 32 tests that reach startInteractiveUI died with
"config.getTargetDir is not a function".
Add the two getters to the mocks that feed those tests, and stub
registerSession/unregisterSession so the suite does not write a real
record into the global Qwen dir on every run — the registry has its own
coverage in packages/core/src/services/session-registry.test.ts.
Tests: packages/cli src/gemini.test.tsx 69 passed (was 32 failed | 37
passed); npm run build, npm run typecheck, eslint and prettier on the
changed file all clean.
This branch forked from main at 8fd0162 and had fallen 76 commits behind, which broke the required "Test (ubuntu-latest, Node 22.x)" job: main's CI now runs `npm run check:voice-guard-sync`, but the script only exists in package.json from a later commit, so the step died with `Missing script: "check:voice-guard-sync"` before vitest ever started. Merging QwenLM#8728's head fixes that and one more thing the aborted job was hiding. QwenLM#8730 is stacked on QwenLM#8728 and carried b92fde1 — the session registry commit — without a9e9cee, the follow-up that repairs the gemini.test.tsx Config mocks for it. Those 32 tests would have failed as soon as the voice-guard gate stopped short-circuiting the job. Taking 8728's head brings both the up-to-date package.json and that fix. No conflicts, and no source change of my own — this is the merge only. Verified: npm run build, npm run typecheck, check:voice-guard-sync, check:lockfile, check:desktop-isolation and audit:runtime:critical all exit 0; packages/cli src/gemini.test.tsx 69 passed; packages/core src/ipc 99 passed; packages/cli peer-messaging + peers-command 32 passed; packages/core session-registry 23 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…_agents Final step of QwenLM#8724. `list_agents` now shows the other Qwen Code sessions running on this machine alongside this session's background agents, and `send_message` can reach one of them by name. The name is the address. A socket path changes every restart; a name survives one, reads back to the user, and is already what `qwen sessions ps` prints. Names are not unique, so each session also carries a six-character `ref` derived from its session id — and `list_agents` only appends it when two rows would otherwise be indistinguishable, so the common case stays a bare, typeable name. An ambiguous bare name is an error listing the candidates, never a guess. Injecting a message into the wrong session cannot be undone by retrying, because that session has already acted on it. That choice also removes the need to pin a name to a session for the length of a conversation: a name that could mean two sessions never silently switches between them, because it never resolves at all. Routing order is background task, then teammate, then peer. In-process wins a name collision — a teammate is part of this session's own work, and quietly routing off-process would be the more surprising of the two. A structured control message (`shutdown_request`) never crosses a session boundary; it is a leader/teammate protocol, and shipping it across would let a peer request this session's shutdown. Sending requires this session to have an inbox of its own, which is also what the feature flag gates. That is deliberate rather than incidental: a message with no reply address is a message the recipient cannot answer. Failures are described, not collapsed. A stale address tells the model to re-discover; a busy pipe tells it to retry the same name. "Send failed" would make it guess. Broadcast (`to: "*"`) is removed. It was linear in team size, and it has no sensible meaning once "everyone" could include sessions doing unrelated work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
/peers shipped with a plain string description, so the strict-parity locale coverage test saw zh-CN and zh-TW fall back to English and failed Test (ubuntu-latest, Node 22.x) with `expected [ 'peers' ] to deeply equal []`. Route the description through t() the way every other built-in command does, and add the string to the en, zh and zh-TW dictionaries. check-i18n stays green.
|
Live-run note from testing two real sessions (details on #8730). Discovery and name resolution both worked: two sessions in different directories listed as One observation about the I do not think that argues for removing it. It is the correct behaviour for the collision that does happen, and it becomes load-bearing the moment sessions can be renamed by the user — which is the obvious next thing someone will want. But it does mean the ambiguity branch is effectively untested outside the unit tests, so it should not be treated as battle-worn. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR @qqqys — the write-up on the design is genuinely useful, but the PR body doesn't follow the repository's PR template: none of the required sections are present. Could you restructure the body to match it?
- What this PR does / Why it's needed — the existing prose covers both; it just needs to live under the expected headings.
- Reviewer Test Plan (
How to verify+Evidence (Before & After)+Tested on) — the part maintainers need most here. Peer discovery, name-addressedsend_message, and the/peerscommand are user-visible behavior, so a before/after capture (e.g. tmux) showing a peer session being listed and messaged, plus the OS matrix, is what makes this reviewable. Unit test counts alone don't substitute for it. - Risk & Scope — main tradeoffs, what's not validated, and breaking notes. Removing
to: "*"changes an existing tool contract; that belongs here explicitly. - Linked Issues — reference #8724 with a closing keyword if applicable.
- The template also asks for a Chinese translation of the body in a
<details>block.
If the other PRs in this stack (#8728, #8730) are missing the same sections, they'll need the same treatment.
中文说明
感谢 @qqqys 的 PR——设计说明写得很好,但 PR 正文没有遵循仓库的 PR 模板:所有必需章节都缺失。请按模板重组正文:
- What this PR does / Why it's needed —— 现有内容已覆盖这两部分,只需放到对应标题下。
- Reviewer Test Plan(
How to verify+Evidence (Before & After)+Tested on)—— 这是 maintainer 最需要的部分。peer 发现、按名称寻址的send_message、/peers命令都是用户可见的行为,请提供 before/after 记录(例如 tmux),展示 peer session 被列出并收到消息的过程,并附上操作系统测试矩阵。仅有单元测试数量不足以支撑审查。 - Risk & Scope —— 主要权衡、未验证的内容、破坏性变更说明。移除
to: "*"改变了现有工具契约,应在此明确说明。 - Linked Issues —— 如适用,用关闭关键词引用 #8724。
- 模板还要求在
<details>块中提供正文的中文翻译。
— Qwen Code · qwen3.8-max
|
Both points are addressed. PR template — the body is restructured onto the template's headings: The specific gaps you named:
Also fixed in this push (
Verification on |
Merging current main surfaced two failures in Test (ubuntu-latest, Node 22.x) that the textual merge could not catch. team-lifecycle.test.ts still asserted that `to: "*"` broadcasts. This PR removes broadcast from the tool entry point deliberately, so the E2E test now asserts the rejection the same way send-message.test.ts already does. TeamManager.broadcast itself is untouched. environmentContext.test.ts guards that "completed tasks are revived" survives the 160-character truncation in the deferred-tools reminder. The peer-addressing clause had pushed that phrase past the cut, so the first sentence is shortened to "a teammate or peer session" and "on this machine" moves into the following sentence, which has no length budget. Nothing is lost from the description.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its platform suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its platform suite did not run locally.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 18488, 19674, 481, 1124, 1466, 2941, 454 passed; and 3 more.
中文说明
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its platform suite did not run locally。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its platform suite did not run locally。
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
Test Plan(非阻断):src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 18488, 19674, 481, 1124, 1466, 2941, 454 passed; and 3 more。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| describe('resolvePeerSocketPath', () => { | ||
| it('prefers XDG_RUNTIME_DIR', () => { | ||
| process.env['XDG_RUNTIME_DIR'] = '/run/user/1000'; |
There was a problem hiding this comment.
[Critical] This new test file has no Windows platform guard, but the required merge-queue test_windows job runs packages/core vitest run with no exclusions. — Failure scenario: on win32 path.win32.join yields backslash paths, so the three resolvePeerSocketPath forward-slash expectations fail (including the /tmp/qwen-socks-<uid> regex), and isLocalIpcPath('/run/user/1000/...') returns false while 'accepts an absolute posix path' asserts true → 4 red tests block the merge queue. The sibling uds-inbox.test.ts added by this same PR uses describe.skipIf(isWindows); the repo convention guards every mode/platform assertion. Fix: wrap the affected describes with describe.skipIf(process.platform === 'win32') (for isLocalIpcPath, skip only the POSIX-specific case — the relative/UNC/NUL cases are platform-independent).
中文说明
该新测试文件没有 Windows 平台守卫,而必需的 merge-queue test_windows 作业会以无排除的 vitest run 运行 packages/core。失败场景:在 win32 上 path.win32.join 生成反斜杠路径,三个 resolvePeerSocketPath 正斜杠断言失败(含 /tmp/qwen-socks-<uid> 正则),且 isLocalIpcPath('/run/user/1000/...') 返回 false 而测试断言 true → 4 个测试变红,阻塞合并队列。同一 PR 新增的姊妹测试 uds-inbox.test.ts 使用了 describe.skipIf(isWindows);仓库惯例对所有权限/平台断言均有守卫。修复:用 describe.skipIf(process.platform === 'win32') 包裹相关 describe(isLocalIpcPath 仅跳过 POSIX 专用用例——相对路径/UNC/NUL 用例与平台无关)。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| expect(live[0].name).toMatch(/^app-[0-9a-f]{2}$/); | ||
| }); | ||
|
|
||
| it('creates the registry directory as 0700', async () => { |
There was a problem hiding this comment.
[Critical] Three POSIX mode-bit assertions (0700/0700/0600) have no win32 guard, and the merge-queue test_windows gate runs this suite. — Failure scenario: on Windows fs.chmod only toggles the read-only attribute and stat().mode never yields owner-only bits, so 'creates the registry directory as 0700', 'tightens a pre-existing loose registry directory' and 'writes the record as 0600' all fail → 3 red tests. Repo convention: every existing mode assertion in packages/core is win32-guarded (it.skipIf(process.platform === 'win32') ×18), and this PR's sibling uds-inbox.test.ts wraps its identical 0700/0600 tests in describe.skipIf(isWindows). Fix: guard the three tests the same way.
中文说明
三处 POSIX 权限位断言(0700/0700/0600)没有 win32 守卫,而 merge-queue 的 test_windows 门禁会运行该套件。失败场景:Windows 上 fs.chmod 只切换只读属性,stat().mode 永远不会得到 owner-only 位,因此 'creates the registry directory as 0700'、'tightens a pre-existing loose registry directory'、'writes the record as 0600' 全部失败 → 3 个测试变红。仓库惯例:packages/core 所有既有权限位断言都有 win32 守卫(it.skipIf(process.platform === 'win32') ×18),本 PR 的姊妹测试 uds-inbox.test.ts 也用 describe.skipIf(isWindows) 包裹了相同的 0700/0600 断言。修复:以同样方式守卫这三个测试。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const peer = resolved.peer; | ||
| const address = formatPeerAddress(peer, peers); | ||
| const frame = buildUserFrame({ | ||
| content: options.message, |
There was a problem hiding this comment.
[Critical] sendToPeer returns kind:'sent' for an empty message that the receiver's wire contract silently drops. — Failure scenario: the send_message schema declares message required with maxLength but no minLength, buildUserFrame accepts empty content, parsePeerFrame rejects it (content.length === 0) and uds-inbox drops unparseable frames with only a debug log — no delivery_status ever comes back. Verified by execution: parsePeerFrame(encodePeerFrame(buildUserFrame({content:''}))) → null, and the tool layer validates {to, message:''} as OK; adding minLength: 1 rejected the call (fix flip verified). The model is told the message was sent while the recipient never sees it and neither side gets a receipt. Fix: guard in sendToPeer before building the frame (return a described failure outcome) and/or add minLength: 1 to the schema.
中文说明
sendToPeer 对空消息返回 kind:'sent',但接收端线协议会静默丢弃它。失败场景:send_message schema 声明 message 必填、有 maxLength 却没有 minLength,buildUserFrame 接受空内容,parsePeerFrame 拒绝空内容(content.length === 0),uds-inbox 仅打 debug 日志就丢弃无法解析的帧——永远不会有 delivery_status 回执。已执行验证:parsePeerFrame(encodePeerFrame(buildUserFrame({content:''}))) → null,且工具层校验 {to, message:''} 通过;加上 minLength: 1 后该调用被拒绝(修复翻转已验证)。模型被告知消息已发出,接收方却永远看不到,双方都没有回执。修复:在 sendToPeer 构建帧之前拦截(返回一个有描述的失败结果),并/或在 schema 中加 minLength: 1。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (held.length === 0) return; | ||
| const newest = held[held.length - 1]; |
There was a problem hiding this comment.
[Critical] Held-message notices fire on removals too: notifyHeldChange() is called on every held-set mutation (including decide() removals), and this listener renders every non-empty state as a new hold (guarding only held.length === 0). — Failure scenario: with 3 messages held, /peers accept all loops decide() per id → each removal re-notifies with the remaining list → N−1 false "Held a message from another session… N waiting — /peers to review" INFO lines (probe reproduced exactly 2 false notices after approving 3); deny and partial reevaluate releases hit the same path. This misdescribes user decisions as new inbound holds and degrades the one alert that exists so held messages don't go unnoticed. Fix: distinguish the change kind at the source (e.g. onHeldChange(held, 'added'|'removed'), notice only on 'added') or dedupe in the listener by tracking already-announced ids/length growth.
中文说明
释放/拒绝 held 消息也会触发"新 hold"通知:notifyHeldChange() 在 held 集合的每次变更(包括 decide() 的移除)都会调用,而该监听器把每个非空状态都当作新的 hold 渲染(只守卫 held.length === 0)。失败场景:3 条消息被 hold 时,/peers accept all 逐个调用 decide() → 每次移除都以剩余列表再次通知 → 产生 N−1 条错误的 "Held a message from another session… N waiting — /peers to review" INFO(探针复现:批准 3 条后恰好出现 2 条假通知);deny 与 reevaluate 的部分释放同样触发。这把用户的决定误报为新的入站 hold,恰好削弱了"held 消息不被忽略"这一提醒本身。修复:在源头区分变更类型(如 onHeldChange(held, 'added'|'removed'),仅 'added' 时通知),或在监听器里按已通知的 msgId/长度增长去重。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| /** Reply address — what the receiver copies into `to` to answer. */ | ||
| from: string; |
There was a problem hiding this comment.
[Critical] The envelope documents this field as "Reply address — what the receiver copies into to to answer", but send_message/resolvePeerTarget only resolve names, 4–12-hex refs, and name [ref] — a PID-keyed socket path never resolves, so the documented reply mechanism fails for every input. — Failure scenario (probe): resolvePeerTarget(peers, '/run/user/1000/qwen-socks/4242.sock') → {kind:'none'} with empty suggestions (the tool then errors "no reachable session by that name"), while the bare name from the same envelope resolves fine. Aggravators: the path is stale after any sender restart; a frame without fromName (hand-sent frames are invited by the module doc) is unanswerable; an ambiguous sender name carries no ref in the envelope. Fix: present a resolvable reply handle (the sender's registry name, with [ref] when contested, à la formatPeerAddress) and keep the socket path frame-internal; or accept isLocalIpcPath targets in sendToPeer.
中文说明
信封把这个字段描述为"回复地址——接收方复制到 to 即可回复",但 send_message/resolvePeerTarget 只解析名称、4–12 位十六进制 ref 和 name [ref]——按 PID 命名的 socket 路径永远无法解析,因此文档承诺的回复机制对所有输入都失败。失败场景(探针):resolvePeerTarget(peers, '/run/user/1000/qwen-socks/4242.sock') → {kind:'none'} 且建议为空(工具随后报"no reachable session by that name"),而同一信封中的裸名称却能正常解析。加重因素:该路径在发送方每次重启后即失效;fromName 缺失时(模块文档鼓励用 socat 手工发帧)消息无法回复;发送方名称歧义时信封又没有 ref 可用。修复:在信封中给出可解析的回复句柄(发送方的注册表名称, contested 时带 [ref],参照 formatPeerAddress),socket 路径仅保留在帧内部;或让 sendToPeer 接受 isLocalIpcPath 目标。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| /** Mode class this session advertises to peers when it sends. */ | ||
| selfModeClass(): 'bypass' | 'prompting' | undefined { | ||
| const mode = this.options.getApprovalMode(); |
There was a problem hiding this comment.
[Suggestion] Dead declared surface (pattern R1-38 — declared/written but no live consumer, per the repo's grep-read-sites review rule). selfModeClass() has zero production call sites (grep: only its unit test); the wire fromMode is produced independently in sendToPeer from the tool's approvalMode. The doc comment ("this session advertises to peers when it sends") is factually wrong, and the green test asserting 'bypass' gives false confidence about the sender-side mode path. Fix: delete the method and its test, or route the send path through it.
中文说明
死的声明面(模式 R1-38——已声明/写入但无生产消费方,按仓库 grep-read-sites 审查规则)。selfModeClass() 没有任何生产调用方(grep:仅其单元测试);线上的 fromMode 由 sendToPeer 从工具的 approvalMode 独立产生。文档注释("发送时向 peer 声明本会话的模式类")与事实不符,断言 'bypass' 的绿色测试给了发送侧模式路径虚假的信心。修复:删除该方法及其测试,或让发送路径经过它。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: dead-surface removal. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:移除无用接口。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| export interface HeldMessage { | ||
| frame: PeerUserFrame; | ||
| cause: HoldCause; | ||
| heldAt: number; | ||
| } |
There was a problem hiding this comment.
[Suggestion] Dead declared surface (pattern R1-38 — declared/written but no live consumer, per the repo's grep-read-sites review rule). heldAt is set on every hold but never read by any production code (grep: declaration, set site, one test fixture); the name implies elapsed-time behavior that does not exist — eviction is purely count-based (MAX_HELD_MESSAGES) and the UI shows no age. Fix: drop heldAt (and the fixture), or use it (e.g. render hold age in /peers).
中文说明
死的声明面(模式 R1-38——已声明/写入但无生产消费方,按仓库 grep-read-sites 审查规则)。heldAt 每次 hold 都会写入,但生产代码从不读取(grep:声明、写入点、一个测试夹具);名字暗示的时长行为并不存在——驱逐纯粹按数量(MAX_HELD_MESSAGES),界面也不显示时长。修复:删除 heldAt(及夹具),或真正使用它(如在 /peers 显示暂扣时长)。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: dead-surface removal. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:移除无用接口。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| /** How a delivered message competes with whatever the user is typing. */ | ||
| export type PeerMessagePriority = 'now' | 'next'; |
There was a problem hiding this comment.
[Suggestion] Dead declared surface (pattern R1-38 — declared/written but no live consumer, per the repo's grep-read-sites review rule). priority is a dead wire field: documented as controlling delivery urgency, parsed/normalized/tested, but no producer can emit 'now' (the send_message schema has no such parameter) and no consumer reads it (deliver/submit ignore it; PeerSubmitFn cannot carry it). A conforming sender emitting priority: 'now' is silently downgraded to queued delivery. Fix: remove the field until it is implemented, or wire it end-to-end (tool parameter → buildUserFrame → honored in deliver).
中文说明
死的声明面(模式 R1-38——已声明/写入但无生产消费方,按仓库 grep-read-sites 审查规则)。priority 是死线字段:文档称其控制投递紧迫度,解析/归一化/测试俱全,但没有任何生产方能发出 'now'(send_message schema 无此参数),也没有任何消费方读取(deliver/submit 忽略它;PeerSubmitFn 无法携带)。遵循契约发送 priority: 'now' 的一方会被静默降级为排队投递。修复:在实现前移除该字段,或端到端接线(工具参数 → buildUserFrame → 在 deliver 中生效)。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: dead-surface removal. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:移除无用接口。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| export { PeerMessaging, type PeerSubmitFn } from './peer-messaging.js'; | ||
| export { | ||
| PeerMessagingContext, |
There was a problem hiding this comment.
[Suggestion] Dead declared surface (pattern R1-38 — declared/written but no live consumer, per the repo's grep-read-sites review rule). the new barrel has zero import sites repo-wide — every consumer imports the leaf modules directly; and unlike a neutral re-export it changes what an import pulls in: PeerMessagingContext.tsx uses import type (no runtime edge), while the barrel's runtime re-export of peer-messaging.js pulls the whole socket/inbox implementation (and the core barrel behind it) into any future consumer that only needs the context. Fix: delete peerMessaging/index.ts, or route the four existing leaf imports through it.
中文说明
死的声明面(模式 R1-38——已声明/写入但无生产消费方,按仓库 grep-read-sites 审查规则)。新 barrel 在全仓库零导入点——所有消费方都直接导入叶子模块;且它并非中性再导出:PeerMessagingContext.tsx 用 import type(无运行时依赖),而 barrel 对 peer-messaging.js 的运行时再导出会把整个 socket/inbox 实现(及其背后的 core barrel)拉进任何只需要 context 的未来消费方。修复:删除 peerMessaging/index.ts,或把现有四处叶子导入改走 barrel。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: dead-surface removal. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:移除无用接口。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| * echo '{"msgV":1,"type":"user","message":{"role":"user","content":"hi"}}' \ | ||
| * | socat - UNIX-CONNECT:/run/user/1000/qwen-socks/1234.sock |
There was a problem hiding this comment.
[Suggestion] The header's hand-delivery socat example omits msgId, which parsePeerFrame requires — the documented debug command cannot deliver a frame. — Failure scenario (probe): parsePeerFrame on the docblock's verbatim line returns null (rejected at typeof msgId !== 'string' || msgId.length === 0); adding "msgId":"m1" parses successfully — msgId is the sole missing field (from/fromName/fromMode/priority are genuinely optional). uds-inbox drops the frame with only a debug-level log, so an engineer debugging a silent peer inbox copies the one-liner, sees nothing, and concludes the inbox/feature is broken. Fix: add the required field to the example: {"msgV":1,"msgId":"manual-test","type":"user",...}.
中文说明
文件头的手工投递 socat 示例漏掉了 parsePeerFrame 必需的 msgId——文档给出的调试命令无法投递帧。失败场景(探针):parsePeerFrame 对 docblock 中原样的一行返回 null(在 typeof msgId !== 'string' || msgId.length === 0 处被拒);加上 "msgId":"m1" 后解析成功——msgId 是唯一缺失的字段(from/fromName/fromMode/priority 确实可选)。uds-inbox 仅以 debug 级日志丢弃该帧,于是调试静默 peer inbox 的工程师照抄这条命令、什么都看不到,进而断定 inbox/功能坏了。修复:在示例中补上必需字段:{"msgV":1,"msgId":"manual-test","type":"user",...}。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: documentation-only change. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:仅文档的改动。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
Review 4889219236 on 34b5eec filed six Criticals. Each one below, with the failure it removes. Two would have turned the merge queue red. `socket-path.test.ts` and the three registry mode-bit assertions in `session-registry.test.ts` had no win32 guard, and the merge_group-only `test_windows` job runs packages/core with no exclusions: `path.win32.join` yields backslashes so every `resolvePeerSocketPath` expectation fails, and Windows synthesizes st_mode from file attributes so 0700/0600 can never hold. Guarded the same way the sibling `uds-inbox.test.ts` already guards its identical assertions. Only the POSIX-specific `isLocalIpcPath` case is skipped; the relative/UNC/NUL cases are platform-independent and stay live. `sendToPeer` reported `kind:'sent'` for an empty message. The receiver's `parsePeerFrame` rejects empty content and `uds-inbox` drops an unparseable frame with only a debug log, so no delivery status ever came back — the model was told a message landed that nobody would ever see. Guarded before the frame is built, surfaced as a new 'empty' outcome, and `minLength: 1` added to the schema so the tool layer rejects it first. The envelope documents `from` as "what the receiver copies into `to` to answer", but a socket path resolved against nothing — the documented reply mechanism failed for every input, and a frame with a missing or contested sender name was unanswerable. `resolvePeerTarget` now matches a `isLocalIpcPath` target literally against the live peer list, so a path from a session that has since exited falls through to 'none' rather than dialing whichever process inherited the pid. Held-message notices fired on removals: `notifyHeldChange()` runs on every mutation including `decide()`, and the listener treated every non-empty state as a new hold, so `/peers accept all` over three messages printed two false "Held a message…" notices. The gate now names the message a change parked, and the notice fires only when there is one. `send_message`'s in-process-wins gate checked only `teamFile.members`, which excludes the leader by definition — but TeamManager also routes "leader" and `leadAgentId`, and the team prompt tells teammates to report with `to: "leader"`. With a peer named `leader-*` present, a teammate's report was offered to peer resolution and bounced with "no reachable session". Both leader addresses now count as in-process. Verification: npm run build and npm run typecheck exit 0; eslint and prettier --check clean. packages/core socket-path + peer-directory + inbound-gate + peer-send + session-registry + send-message = 131 passed; packages/cli peer-messaging + peers-command = 32 passed. Reverting the four behavioural fixes turns exactly the six new behavioural cases red. Flipping the three platform guards to 'linux' skips exactly the ten intended cases (5 + 3 + 2) and no others; reverted after.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its platform suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its platform suite did not run locally.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 18488, 19681, 481, 1124, 1466, 2941, 454 passed; and 3 more.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its platform suite did not run locally。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its platform suite did not run locally。
未审查:反向审计——评审时间预算不足,未能开始第 4 轮。
Test Plan(非阻断):src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 18488, 19681, 481, 1124, 1466, 2941, 454 passed; and 3 more。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| export async function readOwnSessionRecord( | ||
| pid: number = process.pid, | ||
| ): Promise<SessionRegistryRecord | null> { | ||
| return readRecord(getSessionRecordPath(pid)); | ||
| } |
There was a problem hiding this comment.
[Critical] readOwnSessionRecord (and getOwnPeerIdentity built on it) trusts a stale registry record left by a crashed session whose PID was recycled: schema-only validation, no provenance check, and listLiveSessions exempts the self-PID record from sweeping — so the peer-send gate (ipcPath presence) opens under the dead session's identity. — Failure scenario: a messaging-enabled session is SIGKILLed (record survives) and its PID is reused by a session whose registerSession failed (ENOSPC/EROFS — a designed, tolerated state) or which runs headless and never registers: that session has no inbox, yet send_message passes its gate and sends frames carrying the dead session's from/fromName; replies and receipts dial the dead socket and are lost, and nothing ever self-corrects. Probe-verified on the unmodified PR; the proposed fix flips the probe with all 39 registry/peer-send tests green. Fix: verify provenance before trusting the record as self — return null when !isSameProcess(record.pid, record.procStart) (degrades to existing trust on token-less platforms).
中文说明
readOwnSessionRecord(以及基于它的 getOwnPeerIdentity)会信任崩溃会话在 PID 被复用时遗留的陈旧注册表记录:只做 schema 校验、不校验来源,且 listLiveSessions 对本进程 PID 的记录豁免清理——于是 peer 发送门禁(ipcPath 存在性)会以死掉会话的身份打开。失败场景:启用消息的会话被 SIGKILL(记录残留),其 PID 被一个 registerSession 失败(ENOSPC/EROFS——这是设计上容忍的状态)或从不注册的 headless 会话复用:该会话没有 inbox,却能通过门禁、以死会话的 from/fromName 发帧;回复与回执拨打死掉的 socket 而丢失,且永远不会自愈。已在未修改的 PR 上用探针验证;按建议修复后探针翻转,且全部 39 个 registry/peer-send 测试保持绿色。修复:在把记录当作自身之前校验来源——当 !isSameProcess(record.pid, record.procStart) 时返回 null(在无 token 的平台退化为现有信任)。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const receiver = approvalModeClass(mode); | ||
| if (receiver === 'prompting') { | ||
| return { policy: 'accept', cause: 'explicit-setting' }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] resolvePolicy labels derived (non-explicit) accept policies with cause: 'explicit-setting', which is factually false on that path — no explicit setting exists (same for the bypass+bypass accept branch below). — Failure scenario: with crossSessionInbound unset and a prompting receiver, the method's documented contract ('Resolve the policy for a frame, and explain it. Exposed for tests and for the UI') yields a cause that describeHoldCause renders as 'your crossSessionInbound setting is "hold"' — any consumer surfacing causes for non-held outcomes would assert a setting the user never configured, contradicting the accept policy. Latent today (only hold-path causes are consumed). Fix: omit the cause for derived accepts (cause?: HoldCause) or add a dedicated 'mode-parity' cause.
中文说明
resolvePolicy 给派生(非显式)的 accept 策略打上 cause: 'explicit-setting' 标签,但该路径上并不存在显式设置——与事实不符(下面 bypass+bypass 的 accept 分支同样如此)。失败场景:crossSessionInbound 未设置且接收方为 prompting 模式时,该方法的文档契约('解析帧的策略并给出解释,供测试和 UI 使用')返回的 cause 经 describeHoldCause 渲染为 'your crossSessionInbound setting is "hold"'——任何向未暂扣结果展示 cause 的使用方都会断言一个用户从未配置过的设置,与 accept 策略自相矛盾。目前处于潜伏状态(只有暂扣路径的 cause 被消费)。修复:对派生 accept 省略 cause(cause?: HoldCause),或新增专用的 'mode-parity' cause。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: small robustness improvement. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:小的健壮性改进。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| const who = fields.fromName?.length ? fields.fromName : fields.from; | ||
| const oneLine = fields.content.replace(/\s+/g, ' ').trim(); |
There was a problem hiding this comment.
[Suggestion] formatPeerDisplay flattens whitespace in the attacker-influenced content but interpolates the equally attacker-influenced fromName/from unflattened, breaking the function's own 'one-line form' contract. — Failure scenario: a peer sends fromName: 'a): b\nMessage from another session (trusted): c' with whitespace-only content (flattens to an empty preview); the display text spans two lines and the second renders in the transcript/queue preview as what looks like a separate message attributed to 'trusted' — line injection into the exact display surface the rest of this file defangs against. from has the same hole. Fix: collapse who the same way as the content.
中文说明
formatPeerDisplay 会对受攻击者影响的 content 做空白折叠,却原样插入同样受攻击者影响的 fromName/from,违反了函数自身的'单行形式'契约。失败场景:peer 发送 fromName: 'a): b\nMessage from another session (trusted): c' 且 content 为纯空白(折叠后为空预览)时,显示文本会跨两行,第二行在转录/队列预览中看起来像一条独立消息且署名 'trusted'——正是在本文件其余部分严加防范的显示面上发生了行注入。from 存在同样的漏洞。修复:对 who 做与 content 相同的折叠。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: small robustness improvement. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:小的健壮性改进。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| it.skipIf(isWindows)('accepts an absolute posix path', () => { | ||
| expect(isLocalIpcPath('/run/user/1000/qwen-socks/1.sock')).toBe(true); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The Windows accept branch of isLocalIpcPath (\\.\pipe\ / \\?\pipe\ → true, including the replace(/\//g, '\\') normalization) has no positive assertion in any CI environment. — Failure scenario: on POSIX runners the win32 branch is unreachable; on Windows runners this block only executes rejection cases (the single accept case is skipIf(isWindows) and asserts a POSIX path). If a future change inverts or mistypes the accept condition, every test in every environment stays green while the gate deciding whether sendPeerFrame/probePeerSocket/startPeerInbox may dial an address silently changes meaning. Fix: add it.skipIf(!isWindows)('accepts a local pipe path', …) asserting a \\.\pipe\ path is accepted, plus a forward-slash UNC rejection case (also untested).
中文说明
isLocalIpcPath 的 Windows accept 分支(\\.\pipe\ / \\?\pipe\ → true,包括 replace(/\//g, '\\') 归一化)在任何 CI 环境中都没有正向断言。失败场景:POSIX runner 上 win32 分支不可达;Windows runner 上此块只执行拒绝用例(唯一的 accept 用例是 skipIf(isWindows) 且断言的是 POSIX 路径)。若未来改动反转或写错 accept 条件,所有环境的测试仍然全绿,而决定 sendPeerFrame/probePeerSocket/startPeerInbox 能否拨打某地址的门禁已悄然变义。修复:新增 it.skipIf(!isWindows)('accepts a local pipe path', …) 断言接受 \\.\pipe\ 路径,并补一个正斜杠 UNC 拒绝用例(同样未测试)。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| startedAt, | ||
| qwenVersion: typeof qwenVersion === 'string' ? qwenVersion : null, | ||
| peerProtocol: typeof peerProtocol === 'number' ? peerProtocol : 0, | ||
| ...(typeof ipcPath === 'string' && ipcPath.length > 0 ? { ipcPath } : {}), |
There was a problem hiding this comment.
[Suggestion] The ipcPath round-trip through the real registry is tested nowhere: no test writes a record with ipcPath and reads it back via listLiveSessions; every consumer above this layer mocks it. — Failure scenario: ipcPath is the lynchpin of the feature — toPeer() filters on !record.ipcPath, so discovery is dead without it. Deleting this spread (e.g. during a field-list refactor of readRecord) makes listMessageablePeers() return [] for every real session — list_agents reports no sessions and peer send_message falls through to not-found — while the entire suite stays green. Fix: one test that registers, patches { ipcPath }, and reads it back (plus the empty-string-drop branch).
中文说明
ipcPath 经过真实注册表的往返写入完全没有测试:没有任何测试写入带 ipcPath 的记录再通过 listLiveSessions 读回;该层之上的所有消费者都 mock 了它。失败场景:ipcPath 是功能的枢纽——toPeer() 以 !record.ipcPath 过滤,没有它发现机制即瘫痪。删除这个展开(例如在重构 readRecord 的字段列表时)会让 listMessageablePeers() 对每个真实会话返回 []——list_agents 报告没有会话、peer send_message 落入 not-found——而整个测试套件全绿。修复:补一个注册、patch { ipcPath }、再读回的测试(外加空字符串丢弃分支)。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| held({ | ||
| msgId: 'aaaaaa11-0000-4000-8000-000000000000', | ||
| fromName: 'app-ab', | ||
| content: 'please run the deploy', | ||
| cause: 'mode-mismatch', | ||
| }), |
There was a problem hiding this comment.
[Suggestion] Pattern: the /peers held-list rendering and decision branches are untested — wording ternaries, the deny path, the cause pass-through, and the sender fallback each ship green under mutants that corrupt exactly what the user reads when deciding accept/deny. — Failure scenario (probe-verified mutants, each shipping green): (1) the single-deny confirmation ('Dropped. The sending session has been told.') is asserted nowhere — a wording mutant tells the user 'Released' after an irreversible deny. (2) the sender fallback chain fromName ?? from ?? 'unknown session' is never asserted (only fromName fixtures) — dropping the fallbacks renders undefined in the sender column of the approval surface. (4) every formatHeldList fixture uses cause 'mode-mismatch' — hardcoding describeHoldCause('mode-mismatch') shows a factually wrong hold reason for 'explicit-setting'/'no-mode-asserted' holds, misleading the accept/deny decision. (The deny-all angle at :233-235 was dropped as overlap with round-1 R1-33.) Fix: assert the 'Dropped' wording, the fallbacks, and a second cause.
中文说明
模式:/peers 暂扣列表的渲染与决策分支没有测试——措辞三元表达式、deny 路径、cause 透传、发送者兜底,各自在恰好破坏用户决定 accept/deny 时所读内容的变异下全绿。失败场景(经探针验证、各自全绿通过的变异):(1) 单条 deny 确认('Dropped. The sending session has been told.')没有任何断言——措辞变异会在不可逆的 deny 之后告诉用户 'Released'。(2) 发送者兜底链 fromName ?? from ?? 'unknown session' 从未被断言(fixture 只有 fromName)——去掉兜底会在审批界面的发送者列渲染 undefined。(4) 每个 formatHeldList fixture 都用 cause 'mode-mismatch'——硬编码 describeHoldCause('mode-mismatch') 会为 'explicit-setting'/'no-mode-asserted' 暂扣显示与事实不符的暂扣原因,误导 accept/deny 决定。(:233-235 的 deny-all 角度因与第 1 轮 R1-33 重叠而剔除。)修复:断言 'Dropped' 措辞、兜底链,以及第二种 cause。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| it('accepts a bare ref', () => { | ||
| expect(resolvePeerTarget([a, b], b.ref)).toEqual({ kind: 'one', peer: b }); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Pattern: case-normalization in peer-directory's resolution helpers is unpinned — every fixture and needle is lowercase, so dropping either toLowerCase() ships green and silently breaks the documented affordances for mixed-case input. — Failure scenario (probe-verified): removing the needle's toLowerCase() in suggestPeerNames leaves all 25 tests green, and a mixed-case typed name ('Qwen-Code-F7' — plausible; names are quoted in prose and tool output) then gets an empty suggestion list from sendToPeer's error path, so the 'did you mean' recovery silently disappears. The same mutant class applies to the bare-ref branch (peer.ref === trimmed.toLowerCase()): an uppercase bare ref quoted from an error message resolves to none instead of the peer. Fix: add case-variant tests for both.
中文说明
模式:peer-directory 解析辅助函数中的大小写归一化未被钉住——所有 fixture 与 needle 都是小写,因此去掉任一 toLowerCase() 都全绿通过,并静默破坏混合大小写输入下文档承诺的能力。失败场景(探针验证):移除 suggestPeerNames 中 needle 的 toLowerCase() 后全部 25 个测试仍绿,混合大小写输入的名字('Qwen-Code-F7'——可能出现;名字常在叙述与工具输出中被引用)会在 sendToPeer 的错误路径得到空建议列表,'did you mean' 恢复机制静默消失。同样的变异类适用于裸 ref 分支(peer.ref === trimmed.toLowerCase()):从错误信息中引用的大写裸 ref 会解析为 none 而非目标 peer。修复:为两者补大小写变体测试。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| it('suggests names sharing a prefix', () => { | ||
| expect(suggestPeerNames([a, b, c], 'qwen-code')).toEqual([ | ||
| 'qwen-code-f7', |
There was a problem hiding this comment.
[Suggestion] Pattern: case-normalization in peer-directory's resolution helpers is unpinned — every fixture and needle is lowercase, so dropping either toLowerCase() ships green and silently breaks the documented affordances for mixed-case input. — Failure scenario (probe-verified): removing the needle's toLowerCase() in suggestPeerNames leaves all 25 tests green, and a mixed-case typed name ('Qwen-Code-F7' — plausible; names are quoted in prose and tool output) then gets an empty suggestion list from sendToPeer's error path, so the 'did you mean' recovery silently disappears. The same mutant class applies to the bare-ref branch (peer.ref === trimmed.toLowerCase()): an uppercase bare ref quoted from an error message resolves to none instead of the peer. Fix: add case-variant tests for both.
中文说明
模式:peer-directory 解析辅助函数中的大小写归一化未被钉住——所有 fixture 与 needle 都是小写,因此去掉任一 toLowerCase() 都全绿通过,并静默破坏混合大小写输入下文档承诺的能力。失败场景(探针验证):移除 suggestPeerNames 中 needle 的 toLowerCase() 后全部 25 个测试仍绿,混合大小写输入的名字('Qwen-Code-F7'——可能出现;名字常在叙述与工具输出中被引用)会在 sendToPeer 的错误路径得到空建议列表,'did you mean' 恢复机制静默消失。同样的变异类适用于裸 ref 分支(peer.ref === trimmed.toLowerCase()):从错误信息中引用的大写裸 ref 会解析为 none 而非目标 peer。修复:为两者补大小写变体测试。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| const h = harness({ mode: ApprovalMode.DEFAULT }); | ||
| h.gate.shutdown(); | ||
| expect(h.gate.admit(frame())).toBe('accept'); |
There was a problem hiding this comment.
[Suggestion] Pattern: inbound-gate tests assert decisions without asserting their effects — a mutant that skips delivery while returning the same decision/receipt ships green in both the shutdown-accept and the reevaluate paths. — Failure scenario (probe-verified mutants, each shipping green): (1) wrapping the accept branch's deliver in if (!this.shuttingDown) — 'still delivers after shutdown' checks only toBe('accept'), so the frame is discarded while the sender is told 'delivered': loss unobservable to both sides, precisely what the test's own comment forbids ('Shutdown must not silently start dropping messages'). (2) an early break in reevaluate's loop — every reevaluate test parks exactly one message, so only the first of a 2+ backlog is released/dropped and the rest stay parked with no receipts, senders blocked waiting on decisions that never come. Fix: assert delivery effects and add a two-frame reevaluate case.
中文说明
模式:inbound-gate 的测试断言了决定却没有断言其效果——跳过投递但仍返回相同决定/回执的变异在 shutdown-accept 与 reevaluate 两条路径上都全绿。失败场景(经探针验证、各自全绿通过的变异):(1) 把 accept 分支的 deliver 包进 if (!this.shuttingDown)——'still delivers after shutdown' 只检查 toBe('accept'),于是帧被丢弃而发送方被告知 'delivered':双方都无法察觉的丢失,恰是该测试自身注释所禁止的('Shutdown must not silently start dropping messages')。(2) reevaluate 循环中提前 break——每个 reevaluate 测试只暂扣一条消息,因此 2+ 条积压中只有第一条被释放/丢弃,其余继续暂扣且无回执,发送方被阻塞在永远不会到来的决定上。修复:断言投递效果,并补一个双帧 reevaluate 用例。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| h.setMode(ApprovalMode.DEFAULT); | ||
| expect(h.gate.reevaluate('mode-changed')).toBe(1); | ||
| expect(h.delivered).toEqual([f]); |
There was a problem hiding this comment.
[Suggestion] Pattern: inbound-gate tests assert decisions without asserting their effects — a mutant that skips delivery while returning the same decision/receipt ships green in both the shutdown-accept and the reevaluate paths. — Failure scenario (probe-verified mutants, each shipping green): (1) wrapping the accept branch's deliver in if (!this.shuttingDown) — 'still delivers after shutdown' checks only toBe('accept'), so the frame is discarded while the sender is told 'delivered': loss unobservable to both sides, precisely what the test's own comment forbids ('Shutdown must not silently start dropping messages'). (2) an early break in reevaluate's loop — every reevaluate test parks exactly one message, so only the first of a 2+ backlog is released/dropped and the rest stay parked with no receipts, senders blocked waiting on decisions that never come. Fix: assert delivery effects and add a two-frame reevaluate case.
中文说明
模式:inbound-gate 的测试断言了决定却没有断言其效果——跳过投递但仍返回相同决定/回执的变异在 shutdown-accept 与 reevaluate 两条路径上都全绿。失败场景(经探针验证、各自全绿通过的变异):(1) 把 accept 分支的 deliver 包进 if (!this.shuttingDown)——'still delivers after shutdown' 只检查 toBe('accept'),于是帧被丢弃而发送方被告知 'delivered':双方都无法察觉的丢失,恰是该测试自身注释所禁止的('Shutdown must not silently start dropping messages')。(2) reevaluate 循环中提前 break——每个 reevaluate 测试只暂扣一条消息,因此 2+ 条积压中只有第一条被释放/丢弃,其余继续暂扣且无回执,发送方被阻塞在永远不会到来的决定上。修复:断言投递效果,并补一个双帧 reevaluate 用例。
— qwen3.8-max via Qwen Code /review (v0.21.7)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
`readOwnSessionRecord` validated schema only, so a record filed under our PID was assumed to be ours. It need not be: a SIGKILLed session leaves its record behind, and the PID can then be recycled by a session that never registered — `registerSession` deliberately tolerates ENOSPC/EROFS, and headless runs skip it altogether. That session has no inbox, yet it inherited the dead session's `ipcPath`, which is exactly what the peer-send gate tests, so `send_message` opened and stamped frames with the dead session's `from`/`fromName`. Replies and receipts then dialled a socket nobody was listening on and were lost, with nothing to self-correct. Both readers now check `isSameProcess(record.pid, record.procStart)` before trusting a self-PID record. `listLiveSessions` still refuses to sweep it — our PID is alive, so nothing there can prove the record dead — it just stops reporting it as live; the stale comment claiming `isSameProcess` on self "is always true" was the bug, and is replaced with the real reason. On platforms with no start token this degrades to the previous trust-by-PID behaviour. Addresses review comment 3741867484 (R2-1, Critical). Verification: build 0, typecheck 0, eslint + prettier clean on both changed files; core src/ipc + list-agents + send-message + session-registry + process-liveness = 204 passed. Two-way probes: dropping either guard turns the new recycled-PID test red.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its platform suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its platform suite did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: full-source confirmation that sendDeliveryStatus / sendPeerFrame validate the attacker-supplied from path via isLocalIpcPath in uds-client.ts (inferred ….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 18491, 19684, 481, 1124, 1466, 2941, 454 passed; and 3 more.
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its platform suite did not run locally。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its platform suite did not run locally。
未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:full-source confirmation that sendDeliveryStatus / sendPeerFrame validate the attacker-supplied from path via isLocalIpcPath in uds-client.ts (inferred …。
未审查:反向审计——评审时间预算不足,未能开始第 3 轮。
Test Plan(非阻断):src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 18491, 19684, 481, 1124, 1466, 2941, 454 passed; and 3 more。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| await run({ json: false, all: false }); | ||
|
|
||
| const row = stdout[1]; | ||
| expect(row).not.toContain(''); |
There was a problem hiding this comment.
[Suggestion] R3-1: the test embeds raw 0x1B control bytes in source literals (the fixture at :116 and this assertion) instead of \u001b escape sequences, making the assertions invisible and fragile — the repo convention is escape sequences (669 \x1b uses across cli test files). — Failure scenario: any reader/diff view sees not.toContain('') — the intent is invisible; if any pipeline tool strips control characters from source, the assertion becomes not.toContain('') which fails unconditionally (every string contains ''), or if stripped from the fixture the test silently exercises no ESC at all and passes while testing nothing. Fix: record({ name: 'ev\u001b[31mil\r', cwd: '/w/a\nb' }) and expect(row).not.toContain('\u001b');
中文说明
测试在源码字面量中直接嵌入了原始 0x1B 控制字节(:116 的 fixture 与此处断言),而不是 \u001b 转义序列,使断言不可见且脆弱——仓库惯例是使用转义序列(cli 测试文件中有 669 处 \x1b)。失败场景:任何读者/diff 视图看到的是 not.toContain('')——意图不可见;若任何流水线工具剥离源码中的控制字符,断言会变成 not.toContain('') 而无条件失败(任何字符串都包含空串),或者若被剥离的是 fixture,测试将在完全没有 ESC 的情况下静默通过。修复:record({ name: 'ev\u001b[31mil\r', cwd: '/w/a\nb' }) 与 expect(row).not.toContain('\u001b');
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| ): Promise<void> { | ||
| const filePath = getSessionRecordPath(pid); | ||
| try { | ||
| const existing = await readRecord(filePath); |
There was a problem hiding this comment.
[Suggestion] R3-4: patchSessionRecord merges into whatever record sits at this PID's path without the isSameProcess provenance check that readOwnSessionRecord applies, so it can adopt and rewrite a dead predecessor's record. — Failure scenario (probe): session A is SIGKILLed leaving its record; the PID is recycled by process B whose registerSession failed (ENOSPC/EROFS — a designed, tolerated state); B hits this patch call site via the config.ts session-swap path — the probe observed B's fields merged over A's stale record on disk (stale procStart/startedAt/ipcPath adopted). On Linux every read path re-validates the token (contained); on token-less platforms the merged record passes the liveness-only check. The probe-fix (if (!isSameProcess(existing.pid, existing.procStart)) return; after the null check) flipped the result with all 26 existing tests green. Note the peer-messaging advertise call site is NOT reachable in this scenario (start requires registered === true); the swap path suffices.
中文说明
patchSessionRecord 在合并时不做 readOwnSessionRecord 所拥有的 isSameProcess 来源校验,直接合并位于本 PID 路径下的任何记录,因此可能采纳并重写已死前任会话的记录。失败场景(探针):会话 A 被 SIGKILL 留下记录;PID 被注册失败(ENOSPC/EROFS——设计上容忍的状态)的进程 B 复用;B 经 config.ts 的会话切换路径触达该 patch 调用点——探针观察到 B 的字段在磁盘上合并覆盖了 A 的陈旧记录(采纳了过期的 procStart/startedAt/ipcPath)。在 Linux 上所有读取路径会重新校验 token(影响受限);在无 token 的平台,合并后的记录仅靠存活检查即可通过。探针修复(在 null 检查后加 if (!isSameProcess(existing.pid, existing.procStart)) return;)使结果翻转且现有 26 个测试全绿。注意 peer-messaging 的地址发布调用点在该场景下不可达(start 要求 registered === true);切换路径已足以触发。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: small robustness improvement. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:小的健壮性改进。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; | ||
|
|
||
| /** Fixed column widths for the human-readable table (exported for tests). */ | ||
| export const NAME_COL = 22; |
There was a problem hiding this comment.
[Suggestion] RA1-b: NAME_COL/PID_COL/AGE_COL are documented "exported for tests" but no test or other code imports them (ps.test.ts imports only psCommand and formatAge), and the table geometry they define is pinned by nothing — the header regex and toContain row assertions pass for any padding widths. — Failure scenario: editing any width, or the deliberate NAME_COL - 2 truncate-vs-pad gap that keeps a ≥2-space gutter between NAME and PID, ships green (e.g. truncating to NAME_COL makes the name cell abut the PID cell), while the false comment discourages anyone from adding the pinning test it claims already exists — the dead-export shape of the AGENTS.md grep-the-read-sites rule. Fix: import the constants in ps.test.ts and assert real column offsets, or drop the exports and the "(exported for tests)" claim.
中文说明
NAME_COL/PID_COL/AGE_COL 注释写着 "exported for tests",但没有任何测试或其他代码导入它们(ps.test.ts 只导入 psCommand 与 formatAge),且它们定义的表格几何完全没有断言约束——表头正则与 toContain 行断言对任意列宽都通过。失败场景:修改任一列宽、或修改刻意保留 NAME 与 PID 之间 ≥2 空格间距的 NAME_COL - 2 截断/填充差,都会静默通过(例如截断到 NAME_COL 会使名称列紧贴 PID 列),而这条不实注释还会打消他人补测试的念头——AGENTS.md "grep 读取点" 规则的典型死导出。修复:在 ps.test.ts 中导入这些常量并断言真实列偏移,或去掉 export 与 "(exported for tests)" 的说法。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: dead-surface removal. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:移除无用接口。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| await run({ json: true, all: false }); | ||
|
|
||
| expect(stdout).toHaveLength(2); | ||
| expect(JSON.parse(stdout[0]).pid).toBe(4242); |
There was a problem hiding this comment.
[Suggestion] RA2-1: the --json output contract (the scripted-consumption mode, "Output as JSON Lines") is pinned only on line count and pid; the rest of the serialized record is unasserted. — Failure scenario (mutation probe): narrowing the serialization to a projection (the sibling list.ts already uses an explicit toJsonItem) ships 9/9 green; scripts consuming qwen sessions ps --json then break silently — ipcPath in particular is the documented "messageable" hint a caller uses to decide whether to dial a peer. Fix: assert the whole object round-trips, e.g. expect(JSON.parse(stdout[0])).toEqual(fixture) (capture the fixture; a fresh record() call in the expectation would fail on startedAt: Date.now() drift).
中文说明
--json 输出契约(脚本消费模式,"Output as JSON Lines")只被断言了行数与 pid,序列化记录的其余字段均无断言。失败场景(变异探针):把序列化收窄为投影(姊妹命令 list.ts 已使用显式 toJsonItem)后 9/9 测试仍全绿;消费 qwen sessions ps --json 的脚本会静默损坏——尤其 ipcPath 是文档中判断"可发送消息"、决定是否拨号对端的依据。修复:断言整个对象往返一致,例如 expect(JSON.parse(stdout[0])).toEqual(fixture)(先捕获 fixture;在期望处新建 record() 会因 startedAt: Date.now() 漂移而失败)。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| expect(outcome.kind).toBe('ambiguous'); | ||
| if (outcome.kind === 'ambiguous') { | ||
| expect(outcome.matches).toHaveLength(2); | ||
| expect(outcome.matches[0]).toContain('/w/one'); |
There was a problem hiding this comment.
[Suggestion] R3-AGG3 (1/2) — Pattern: sendToPeer outcome fields the model acts on are unpinned. This location: the ambiguous-match rendering `${peer.name} [${peer.ref}] in ${peer.cwd}` is pinned by no test — this assertion checks only match count and a cwd substring, so the [ref] part can be dropped with every test green (send-message.test.ts mocks sendToPeer entirely; peer-directory.test.ts only sees PeerSessionInfo objects). — Failure scenario (mutation probe): dropping [${peer.ref}] shipped 70/70 green across all three suites; the model then receives "app-ab matches more than one live session: app-ab in /w/one, app-ab in /w/two. Re-send with the full 'name [ref]'..." — an instruction it cannot follow because no ref was supplied, forcing an extra list_agents round-trip; when two sessions share both name and cwd (two terminals in one project) the candidates become indistinguishable at all. Fix: assert the full line: expect(outcome.matches[0]).toBe(\app-ab [${peerRef('s1')}] in /w/one`)`.
中文说明
模式:sendToPeer 供模型使用的输出字段未被固定。此位置:歧义匹配渲染 `${peer.name} [${peer.ref}] in ${peer.cwd}` 没有任何测试固定——该断言只检查匹配数量与 cwd 子串,因此去掉 [ref] 部分所有测试仍绿(send-message.test.ts 完整 mock 了 sendToPeer;peer-directory.test.ts 只见 PeerSessionInfo 对象)。失败场景(变异探针):去掉 [${peer.ref}] 后三个套件 70/70 全绿;模型将收到 "app-ab matches more than one live session: app-ab in /w/one, app-ab in /w/two. Re-send with the full 'name [ref]'..."——一条因未提供 ref 而无法执行的指令,被迫多一轮 list_agents;两个会话同名同 cwd(同一项目两个终端)时候选将完全无法区分。修复:断言完整行:expect(outcome.matches[0]).toBe(\app-ab [${peerRef('s1')}] in /w/one`)`。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| target: 'self-00', | ||
| message: 'hi', | ||
| approvalMode: ApprovalMode.DEFAULT, |
There was a problem hiding this comment.
[Suggestion] R3-AGG3 (2/2) — Pattern: sendToPeer outcome fields the model acts on are unpinned. This location: the "never addresses itself" test asserts only outcome.kind (+ sendPeerFrame not called), leaving the suggestions payload unchecked. — Failure scenario (mutation probe): feeding suggestions from the unfiltered directory list (suggestPeerNames(allPeers, ...)) shipped 16/16 green; the diff showed the leak directly — self's name self-00 in the suggestions — so a regression would answer a send to 'self-00' with "No reachable session is named 'self-00'. Did you mean: self-00?", steering the model straight into the loopback the self-exclusion filter exists to prevent. The sibling not-found test already uses strict toEqual, so the fix matches house style: expect(outcome).toEqual({ kind: 'not-found', suggestions: [] });
中文说明
模式:sendToPeer 供模型使用的输出字段未被固定。此位置:"never addresses itself" 测试只断言 outcome.kind(+ sendPeerFrame 未被调用),suggestions 负载未检查。失败场景(变异探针):用未过滤的对端列表提供 suggestions(suggestPeerNames(allPeers, ...))后 16/16 全绿;diff 直接显示了泄漏——自身名称 self-00 出现在建议中——回归后对 'self-00' 的发送会得到 "No reachable session is named 'self-00'. Did you mean: self-00?",把模型径直引向自排除过滤器要防止的自回环。姊妹 not-found 测试已用严格 toEqual,修复与现有风格一致:expect(outcome).toEqual({ kind: 'not-found', suggestions: [] });
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| from: string; | ||
| content: string; | ||
| }): string { | ||
| const who = fields.fromName?.length ? fields.fromName : fields.from; |
There was a problem hiding this comment.
[Suggestion] R3-AGG4 — Pattern: the attacker-controlled sender field is interpolated uncapped into the one-line display paths (content is capped, sender is not). This location: formatPeerDisplay (the delivery path) truncates only content to ~120 chars; who is interpolated verbatim. — Failure scenario: a peer sends a user frame with fromName = 'x'.repeat(1000000) — within MAX_FRAME_BYTES (1 MiB), so uds-inbox accepts it; PeerMessaging.submit passes it here, so the "one-line form for the transcript and the queue preview" becomes a ~1 MiB line rendered/logged per message, while the content beside it is neatly truncated. The threat model is the PR's own (uds-inbox.ts: any same-uid process "can write any from it likes"). Distinct from the already-discussed whitespace-flattening item (that one is newlines; this one is length). Fix: cap who here and in formatHeldList (a shared truncate to ~40 chars with ellipsis), and add a test asserting bounded output for a long fromName.
中文说明
模式:攻击者可控的发送方字段在两条一行显示路径中未加上限插值(内容有上限,发送方没有)。此位置:formatPeerDisplay(投递路径)只把 content 截断到约 120 字符;who 原样插值。失败场景:对端发送 fromName = 'x'.repeat(1000000) 的用户帧——在 MAX_FRAME_BYTES(1 MiB)之内,uds-inbox 会接受;PeerMessaging.submit 把它传到这里,"转录与队列预览用的一行形式"变成每条消息渲染/记录约 1 MiB 的一行,而旁边的内容却被整齐截断。威胁模型正是本 PR 自己的(uds-inbox.ts:任何同 uid 进程"can write any from it likes")。与已讨论的空白压平条目不同(那是换行问题;这是长度问题)。修复:在此处与 formatHeldList 中给 who 加上限(共享的约 40 字符截断 + 省略号),并增加长 fromName 输出受限的测试。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: small robustness improvement. This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:小的健壮性改进。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| reject(new PeerSendError(error.message, error.code)); | ||
| }; | ||
|
|
||
| socket.setTimeout(SEND_TIMEOUT_MS, () => { |
There was a problem hiding this comment.
[Suggestion] R3-AGG5 (1/2) — Pattern: neither timeout in uds-client.ts is exercised by any test; deleting either ships green. This location: SEND_TIMEOUT_MS — the only bound on a peer send. — Failure scenario (mutation probe): no test file references SEND_TIMEOUT_MS; the only ETIMEDOUT in peer tests is a hand-constructed PeerSendError formatting test. Deleting the socket.setTimeout(SEND_TIMEOUT_MS, ...) block shipped uds-inbox (21) + peer-send (16) green; a probe against a listener that accepts but never drains/closes hung the full 15 s probe budget under the mutant, vs rejecting with PeerSendError code ETIMEDOUT at 5.0 s on the original. A future broken timer leaves sends hanging past 5 s with no failure report. Fix: add a test with a stalling peer (or injected/faked timers) asserting rejection with { name: 'PeerSendError', code: 'ETIMEDOUT' }.
中文说明
模式:uds-client.ts 中两个超时都没有任何测试演练;删除任一个都能静默通过。此位置:SEND_TIMEOUT_MS——对端发送的唯一时限。失败场景(变异探针):没有测试文件引用 SEND_TIMEOUT_MS;peer 测试中唯一的 ETIMEDOUT 是手工构造 PeerSendError 的格式化测试。删除 socket.setTimeout(SEND_TIMEOUT_MS, ...) 后 uds-inbox(21)+ peer-send(16)全绿;对接受但从不排空/关闭的 listener 的探针在变异体下挂满 15 秒探针预算,原始代码则在 5.0 秒以 PeerSendError code ETIMEDOUT 拒绝。未来定时器损坏会让发送挂过 5 秒而无任何失败报告。修复:增加停滞对端(或注入/伪造定时器)的测试,断言以 { name: 'PeerSendError', code: 'ETIMEDOUT' } 拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
| socket.on('error', (error: NodeJS.ErrnoException) => | ||
| settle(error.code === 'EBUSY'), | ||
| ); | ||
| socket.setTimeout(PROBE_TIMEOUT_MS, () => settle(false)); |
There was a problem hiding this comment.
[Suggestion] R3-AGG5 (2/2) — Pattern: neither timeout in uds-client.ts is exercised by any test; deleting either ships green. This location: PROBE_TIMEOUT_MS — the only thing enforcing listMessageablePeers' documented "each capped at 250 ms" guarantee (Promise.all over probes). — Failure scenario (mutation probe): all four probePeerSocket tests resolve through fast connect/error paths; peer-directory.test.ts and peer-send.test.ts mock the probe out entirely; PROBE_TIMEOUT_MS appears in no test. Deleting the setTimeout call shipped 62/62 green across the three suites. If a future change breaks the timer, a peer whose connect never completes promptly holds the Promise.all in listMessageablePeers open indefinitely — /peers listing and send-message recipient discovery hang with no bound, the exact failure the 250 ms cap was written to prevent. Fix: add a probe test that makes connect/error unreachable (a stalling socket path, or faked timers) and asserts probePeerSocket resolves false after PROBE_TIMEOUT_MS.
中文说明
模式:uds-client.ts 中两个超时都没有任何测试演练;删除任一个都能静默通过。此位置:PROBE_TIMEOUT_MS——listMessageablePeers 文档承诺"each capped at 250 ms"的唯一保障(对探测 Promise.all)。失败场景(变异探针):四个 probePeerSocket 测试都经快速 connect/error 路径返回;peer-directory.test.ts 与 peer-send.test.ts 完全 mock 掉探测;PROBE_TIMEOUT_MS 未出现在任何测试中。删除该 setTimeout 调用后三个套件 62/62 全绿。若未来改动破坏了定时器,connect 迟迟不完成的对端会让 listMessageablePeers 的 Promise.all 无限期挂起——/peers 列表与 send-message 的接收方发现无界挂起,正是 250 ms 上限要防止的失败。修复:增加使 connect/error 不可达的探测测试(停滞的 socket 路径或伪造定时器),断言 probePeerSocket 在 PROBE_TIMEOUT_MS 后返回 false。
— qwen3.8-max via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
Deferred to a follow-up: test-hardening suggestion (mutation pin / efficacy probe). This PR is past five change-producing review rounds, so per the repo's review budget this round takes only the Critical fixes; this finding is recorded in the round summary so it is tracked, not silently dropped. The open thread keeps it visible for the follow-up.
中文说明
推迟到后续处理:测试加固建议(变异钉/测试效力探针)。本 PR 已超过五个产生变更的评审轮次,按仓库的评审轮次预算,本轮只接收 Critical 修复;该发现已记录在本轮总结中,会被跟踪而不会被静默丢弃。保持打开的线程让它在后续处理中仍然可见。
RA2-15: `sendPeerFrame` resolves on the transport `'close'` event, which carries no delivery information, so a frame the receiver discarded still reports as `sent`. The receiving side is where that is fixable: a frame the peer has already written can be sitting unread when the user quits, and `PeerInbox.close()` called `destroy()` on every live connection — Linux then delivers a clean EOF to the sender, whose `'close'` handler resolves, and `send_message` reports a delivery that never happened. `close()` now ends its write half and resumes the read side of each live connection, so the rest of the ordered stream (the frame precedes the peer's FIN) is delivered before the socket closes. A peer that holds the connection open is still bounded: after CLOSE_DRAIN_MS the socket is destroyed as before, and the timer is unref'd so it never keeps the loop alive.
|
Pushed Taking the receiver-side fix you outlined, since
Left Verification:
The R3-1 suggestion about raw 0x1B bytes in |
|
🤖 AutoFix ran out of time before finishing (idle-timeout (no output for 1200000ms — the sandbox likely hung at startup)) (attempt 2/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31421767926 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. 3 of those were silent-sandbox (idle) timeouts that no budget increase can cure — investigate the sandbox image and runner docker daemon for those. A human should investigate the sandbox image and runner docker daemon, then comment What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31425679407 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停:本 PR 达到轮次上限(100/100)。评论 |
|
@qwen-code /takeover stop |
|
👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply 中文说明👋 已释放:autofix 循环不再介入此 PR(在飞的一轮如有,将完成其有界工作)。重新打上 |
|
@qwen-code /resolve |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution for PR #8733Root cause. Main's #8831 ( Textual, not semantic. Both sides touched the same JSX block but disjoint concerns: main added one prop to <PeerMessagingContext.Provider value={peerMessaging}>
<RemoteInputContext.Provider value={remoteInputWatcher}>
...
<AppContainer
...
extensionRefreshState={options.extensionRefreshState}
repaintViewport={resizeReflow.repaint}
/>
...
</RemoteInputContext.Provider>
</PeerMessagingContext.Provider>Load-bearing.
Not verified. No build or tests were run here. 中文说明根因。 main 上的 #8831( 仅文本冲突,无语义冲突。 双方改的是同一 JSX 块中互不相关的部分:main 只给 关键点。
未验证。 本次未运行构建或测试。 |
Superseded by later commits through 16b1e61. Clearing the stale CHANGES_REQUESTED state; the current head must be reviewed separately.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; chunk 9: running useMessageQueue.test.ts under vitest — neither the review worktree nor the parent checkout has node_modules installed, and a full npm install was …; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget (~10 tool calls).; chunk 7: did not execute the test file — neither the review worktree nor the parent checkout has node_modules , and a fresh monorepo npm install exceeded the tool bud…; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget., and 4 more.
Test Plan (not a blocker): src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 19130, 19791, 481, 1224, 1505 passed; and 3 more.
[Critical] R6-2 (existing thread 3746505725, packages/core/src/ipc/inbound-gate.ts): still stands — resolvePolicy still accepts a bypassing receiver on the sender's self-asserted wire field fromMode with no verification anywhere (the registry records no approval-mode field; the code is unchanged at this commit). The author explicitly escalated this as a threat-model decision (whether same-uid is the trust boundary, or peer frames must be attributable) and is waiting on a maintainer; the previously suggested option (document fromMode as an unverified honor-system assertion and state that the parity gate only mediates honest senders) remains implementable today. Re-confirmed again this round.
中文说明
未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:none — all checks above were completed within budget.;chunk 9:running useMessageQueue.test.ts under vitest — neither the review worktree nor the parent checkout has node_modules installed, and a full npm install was …;You are review agent reverse-audit — Reverse audit agen...:none — all checks I started were completed within budget (~10 tool calls).;chunk 7:did not execute the test file — neither the review worktree nor the parent checkout has node_modules , and a fresh monorepo npm install exceeded the tool bud…;You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.,另有 4 条。
Test Plan(非阻断):src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 19130, 19791, 481, 1224, 1505 passed; and 3 more。
[Critical] R6-2 (existing thread 3746505725, packages/core/src/ipc/inbound-gate.ts): still stands — resolvePolicy still accepts a bypassing receiver on the sender's self-asserted wire field fromMode with no verification anywhere (the registry records no approval-mode field; the code is unchanged at this commit). The author explicitly escalated this as a threat-model decision (whether same-uid is the trust boundary, or peer frames must be attributable) and is waiting on a maintainer; the previously suggested option (document fromMode as an unverified honor-system assertion and state that the parity gate only mediates honest senders) remains implementable today. Re-confirmed again this round.
— qwen3.8-max via Qwen Code /review (v0.21.9)
| shortId(entry.frame.msgId).toLowerCase().startsWith(needle) || | ||
| entry.frame.msgId.toLowerCase().startsWith(needle), |
There was a problem hiding this comment.
[Critical] /peers displays the held-message handle as sanitizeTerminalText(shortId(msgId)), but resolveHeld matches the user's token against the unsanitized raw shortId/msgId. parsePeerFrame accepts any non-empty msgId, so a peer can send msgId = "ab\u0007cdef-…": the listing shows handle abcde, yet resolveHeld('abcde') matches nothing (BEL is stripped for display but present in the compared string). — Failure scenario: probe-verified at this commit — resolveHeld returns none for every typeable token; a leading-BEL id matches nothing at all. The message becomes individually undecidable and only accept all/deny all can reach it — on the exact screen built to decide accept/deny. Sibling of the fixed R7-1/R7-5, different mechanism. Fix: resolve against the same strings that are displayed.
| shortId(entry.frame.msgId).toLowerCase().startsWith(needle) || | |
| entry.frame.msgId.toLowerCase().startsWith(needle), | |
| sanitizeTerminalText(shortId(entry.frame.msgId)).toLowerCase().startsWith(needle) || | |
| sanitizeTerminalText(entry.frame.msgId).toLowerCase().startsWith(needle), |
中文说明
[Critical] /peers 以 sanitizeTerminalText(shortId(msgId)) 显示被暂扣消息的句柄,但 resolveHeld 却用未净化的原始 shortId/msgId 去匹配用户输入的 token。parsePeerFrame 接受任意非空 msgId,因此对端可以发送 msgId = "ab\u0007cdef-…":列表显示句柄 abcde,但 resolveHeld('abcde') 匹配不到任何内容(BEL 在显示时被剥离,却仍存在于被比较的字符串中)。失败场景(已在本提交上探针验证):对这样的 id,resolveHeld 对用户可输入的任何 token 都返回 none;以 BEL 开头的 id 则完全无法匹配。该消息变得无法单独裁决,只能靠 accept all/deny all 处理——而这恰好发生在本应用于逐一决定 accept/deny 的界面上。这是已修复的 R7-1/R7-5 的同类问题,但机制不同。修复:用与显示相同的字符串进行解析。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
已修复 + 验证证据:4a19a90e6c;peers-command.test.ts 25/25 通过。
| const msgId = parsed['msgId']; | ||
| if (typeof msgId !== 'string' || msgId.length === 0) return null; |
There was a problem hiding this comment.
[Critical] Degenerate wire-supplied msgIds yield held entries that can never be decided individually: a dashes-only id ("------") makes shortId return '' (blank handle), and a whitespace-only id (" ") renders an invisible handle that no typed token can match (args are trim().split(/\s+/).filter(Boolean)). — Failure scenario: probe-verified at this commit — both frames parse and are held; resolveHeld returns none for every token shape tried; the entries sit undecided until eviction, and only accept all/deny all can touch them. A same-uid peer (the documented threat model invites hand-built frames) can force bulk decisions in the individual-review UI with one line. Related: duplicate msgIds are also admitted unchecked, making entries with identical handles permanently 'ambiguous' (dedupe in admit() as well).
| const msgId = parsed['msgId']; | |
| if (typeof msgId !== 'string' || msgId.length === 0) return null; | |
| if (typeof msgId !== 'string' || msgId.replace(/[-\s]/g, '').length === 0) return null; |
中文说明
[Critical] 退化的 wire 层 msgId 会产生永远无法单独裁决的暂扣条目:纯短横线 id("------")使 shortId 返回 ''(空句柄),纯空白 id(" ")渲染出任何输入 token 都无法匹配的不可见句柄(args 经过 trim().split(/\s+/).filter(Boolean))。失败场景(已在本提交上探针验证):两种帧都能解析并被暂扣;resolveHeld 对尝试过的所有 token 形态都返回 none;条目一直滞留到被驱逐,只有 accept all/deny all 能处理它们。同一 uid 的对端(文档中的威胁模型明确欢迎手工构造帧)只需一行即可在逐条审查界面上强制走批量决定。相关问题:重复的 msgId 也未被检查就准入,使句柄相同的条目永远处于 'ambiguous'(建议一并在 admit() 中去重)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
已修复 + 验证证据:4a19a90e6c;peer-frames/inbound-gate 定向测试 63/63 通过。
| reject(new PeerSendError(error.message, error.code)); | ||
| }; | ||
|
|
||
| socket.setTimeout(SEND_TIMEOUT_MS, () => { |
There was a problem hiding this comment.
[Critical] SEND_TIMEOUT_MS is enforced with socket.setTimeout() — an idle timer that received data resets. A same-uid peer that accepts the connection, trickles one byte every few seconds and never end()s defeats the timeout entirely. — Failure scenario: probe-verified against this commit's code: with a half-open trickling listener the sendPeerFrame promise was still pending after 4× SEND_TIMEOUT_MS (control arm without the trickle rejects ETIMEDOUT at ~1×). The await chain sendPeerFrame → sendToPeer → trySendToPeer → tool execute has no deadline of its own, so the send_message tool call hangs indefinitely — a same-uid DoS of any session whose model/user addresses the hostile peer (registrable via its own record's ipcPath, or reachable as an envelope's wire-supplied reply address). Fix: a wall-clock deadline, e.g. const timer = setTimeout(() => fail(Object.assign(new Error(…), { code: 'ETIMEDOUT' })), SEND_TIMEOUT_MS); timer.unref(); with clearTimeout(timer) in fail and the 'close' settle path; applying this flipped the probe to reject at ~1× the timeout.
中文说明
[Critical] SEND_TIMEOUT_MS 用 socket.setTimeout() 实现——这是一个空闲计时器,收到数据即被重置。同一 uid 的对端只要接受连接、每隔几秒写一个字节且从不 end(),就能完全绕过该超时。失败场景(已对本提交代码做探针验证):对一个半开且持续滴漏字节的监听端,sendPeerFrame 的 promise 在 4 倍 SEND_TIMEOUT_MS 后仍未 settle(不滴漏的对照分支在约 1 倍时以 ETIMEDOUT 拒绝)。sendPeerFrame → sendToPeer → trySendToPeer → tool execute 的 await 链自身没有任何截止时间,因此 send_message 工具调用会无限期挂起——任何模型/用户向该恶意对端寻址的会话都会遭到这个同 uid DoS(攻击者可用自己记录的 ipcPath 注册,或作为信封 wire 层提供的回信地址被访问)。修复:改用墙钟截止时间,例如 const timer = setTimeout(() => fail(Object.assign(new Error(…), { code: 'ETIMEDOUT' })), SEND_TIMEOUT_MS); timer.unref();,并在 fail 与 'close' settle 路径中 clearTimeout(timer);应用该修复后探针在约 1 倍超时处拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
已修复 + 验证证据:4a19a90e6c;Core 定向测试 64/64,Core build/typecheck 通过。
| // eslint-disable-next-line no-control-regex | ||
| return escaped.replace(/[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f]/g, ''); |
There was a problem hiding this comment.
[Suggestion] This hand-rolled third stage strips ANSI and C0/C1 controls but not bidi override/isolate characters, which the codebase's own sanitizeTerminalText (same textUtils module, already imported here) strips as a Trojan-Source defense. name/cwd are attacker-influenced registry fields (another process's on-disk record, per this file's own comment). — Concrete cost: probe-verified — a record with U+202E / U+2066 / U+2069 in name/cwd renders a visually reordered path in qwen sessions ps, defeating the user's ability to identify what a peer session is actually doing. Composing sanitizeTerminalText also removes the duplication with list.ts's sanitizer (its drift is already visible).
| // eslint-disable-next-line no-control-regex | |
| return escaped.replace(/[\x00-\x08\x0b\x0c\x0e-\x1f\x7f-\x9f]/g, ''); | |
| return sanitizeTerminalText(stripped); |
中文说明
[Suggestion] 这个手写的第三段处理会剥离 ANSI 与 C0/C1 控制字符,但不剥离 bidi 覆盖/隔离字符——而代码库自己的 sanitizeTerminalText(同属 textUtils 模块,且本文件已导入)作为 Trojan-Source 防御会剥离这些字符。name/cwd 是攻击者可影响的注册表字段(按本文件自己的注释,是另一个进程写入磁盘的记录)。具体代价(探针验证):name/cwd 含 U+202E / U+2066 / U+2069 的记录会在 qwen sessions ps 中渲染出视觉顺序被重排的路径,使用户无法辨别对端会话实际在做什么。改用 sanitizeTerminalText 还能消除与 list.ts 净化器的重复(二者的漂移已经可见)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const msg = | ||
| 'No active team, no task_id, and no reachable session by that name. ' + |
There was a problem hiding this comment.
[Suggestion] When type is set (e.g. shutdown_request), no team is active, and to names a live reachable peer session, this error claims "no reachable session by that name" — but the peer route was skipped by trySendToPeer's type guard without any reachability check. — Failure scenario: probe-verified at this commit — with sendToPeer mocked reachable, call count was 0 while the tool reported the session unreachable; list_agents then contradicts the error, inviting wasted retry loops with name variants. The real reason (structured control messages are team-only and cannot cross a session boundary) is never surfaced. Fix: when params.type is set and no team address matched, return a dedicated message saying structured types are team-only, instead of the unreachability claim.
中文说明
[Suggestion] 当设置了 type(如 shutdown_request)、没有活跃团队、且 to 指向一个存活且可达的对端会话时,这个错误声称"没有该名称的可达会话"——但 peer 路由是被 trySendToPeer 的 type 守卫跳过的,根本没有做任何可达性检查。失败场景(已在本提交上探针验证):将 sendToPeer mock 为可达时,其调用次数为 0,而工具却报告会话不可达;list_agents 随后会与该错误自相矛盾,诱使模型用名称变体反复重试。真正的原因(结构化控制消息仅限团队内部、不能跨会话)从未被说明。修复:当设置了 params.type 且没有匹配的团队地址时,返回专门说明"结构化类型仅限团队"的消息,而不是不可达的断言。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| case 'sent': { | ||
| const preview = this.params.summary ?? this.params.message.slice(0, 50); |
There was a problem hiding this comment.
[Suggestion] Registry-derived name/cwd are rendered unsanitized into tool output here (outcome.address, outcome.peer.cwd), in the 'ambiguous' match lines (${peer.name} [${peer.ref}] in ${peer.cwd} in peer-send.ts), the 'not-found' suggestions, and list-agents.ts's sessions rows. The TUI's global escapeAnsiCtrlCodes neutralizes ESC-prefixed sequences in returnDisplay, but raw \r, bidi override characters and newlines survive it — and the llmContent/model-context half has no sanitization at all. — Failure scenario: a same-uid process registers a record with bidi/newline-laden name/cwd and a live ipcPath; when the victim's model addresses it, forged lines are injected into the sender's transcript and model context — e.g. fake match entries steering which recipient the model retries with. ps.ts sanitizes exactly these fields for exactly this reason ("attacker-influenced"); the send/list paths lack the equivalent. Fix at the source: sanitizeTerminalText on record.name/record.cwd in peer-directory.ts toPeer, which covers every downstream renderer.
中文说明
[Suggestion] 注册表派生的 name/cwd 在这里(outcome.address、outcome.peer.cwd)、'ambiguous' 匹配行(peer-send.ts 中的 ${peer.name} [${peer.ref}] in ${peer.cwd})、'not-found' 建议以及 list-agents.ts 的 sessions 行中都未经净化就渲染进工具输出。TUI 的全局 escapeAnsiCtrlCodes 能中和 returnDisplay 中带 ESC 前缀的序列,但裸 \r、bidi 覆盖字符和换行可以穿过——而 llmContent/模型上下文那一半完全没有任何净化。失败场景:同一 uid 的进程注册一条 name/cwd 含 bidi/换行且 ipcPath 存活的记录;当受害会话的模型向其寻址时,伪造的行会被注入发送方的转录与模型上下文——例如伪造的匹配条目诱导模型选择重试对象。ps.ts 正是因为同样的原因("攻击者可影响")对这两个字段做了净化;send/list 路径缺少等价处理。在源头修复:在 peer-directory.ts 的 toPeer 中对 record.name/record.cwd 做 sanitizeTerminalText,即可覆盖所有下游渲染点。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| verb === 'accept' | ||
| ? 'Released to this session. It will be picked up on the next turn.' | ||
| : 'Dropped. The sending session has been told.', |
There was a problem hiding this comment.
[Suggestion] /peers deny unconditionally prints "The sending session has been told", but the denial receipt is skipped when the frame carries no reply address (reportStatus in peer-messaging.ts: if (!frame.from) return;). Anonymous frames are holdable (from is optional by contract; a bypass/explicit-hold receiver parks them under no-mode-asserted/explicit-setting). — Failure scenario: denying a held anonymous message tells the user a notification was sent that never was and never could be — the receiver acts on false feedback, and the sender's model never learns of the denial. Fix: look up the held entry before deciding and branch the message, e.g. 'Dropped. The sender gave no reply address, so it could not be told.' when from is absent. (The 'Dropped' wording and the whole deny all branch are also unasserted by tests — flipping the ternary arm ships green.)
中文说明
[Suggestion] /peers deny 无条件打印"The sending session has been told"(已通知发送方会话),但当帧没有回信地址时,拒绝回执会被跳过(peer-messaging.ts 中的 reportStatus:if (!frame.from) return;)。匿名帧是可能被暂扣的(按契约 from 可选;bypass/显式 hold 的接收方会以 no-mode-asserted/explicit-setting 暂扣它们)。失败场景:拒绝一条被暂扣的匿名消息会告诉用户已发出通知,而该通知从未发出也不可能发出——接收方基于错误反馈行动,发送方的模型也永远不知道该消息被拒绝。修复:在裁决前查找暂扣条目并按情况分支文案,例如 from 缺失时显示"Dropped. The sender gave no reply address, so it could not be told."。('Dropped' 文案和整个 deny all 分支也没有测试断言——翻转三元运算符的任一分支都能绿色通过。)
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const who = fields.fromName?.length ? fields.fromName : fields.from; | ||
| const oneLine = fields.content.replace(/\s+/g, ' ').trim(); |
There was a problem hiding this comment.
[Suggestion] formatPeerDisplay normalizes whitespace and caps content at 120 chars, but leaves the wire-supplied sender label (who) neither whitespace-normalized nor length-capped — bounded only by the 1 MiB frame cap. — Failure scenario: probe-verified — (a) fromName containing newlines forges extra one-line-preview lines in the transcript, the queue preview and /peers-adjacent renders (sanitizeTerminalText downstream deliberately preserves LF), letting a peer inject layout into exactly the surfaces labeled "one-line form"; (b) a ~1 MiB fromName flows verbatim into notificationDisplayText → recordNotification and persists for /resume, while the 120-char content cap it exists beside is buried. Fix: normalize and cap who like content.
| const who = fields.fromName?.length ? fields.fromName : fields.from; | |
| const oneLine = fields.content.replace(/\s+/g, ' ').trim(); | |
| const whoRaw = (fields.fromName?.length ? fields.fromName : fields.from).replace(/\s+/g, ' ').trim(); | |
| const who = whoRaw.length > 40 ? `${whoRaw.slice(0, 39)}…` : whoRaw; | |
| const oneLine = fields.content.replace(/\s+/g, ' ').trim(); |
中文说明
[Suggestion] formatPeerDisplay 会对 content 做空白规范化并限制在 120 字符,但对 wire 层提供的发送方标签(who)既不做空白规范化也不限长——其上限只有 1 MiB 帧上限。失败场景(探针验证):(a) 含换行的 fromName 会在转录、队列预览等渲染中伪造出额外的"单行预览"行(下游的 sanitizeTerminalText 有意保留 LF),使对端能在明确标注为"单行格式"的界面上注入排版;(b) 约 1 MiB 的 fromName 会原样流入 notificationDisplayText → recordNotification 并为 /resume 持久化,而与之并存、用于限制 content 的 120 字符上限形同虚设。修复:对 who 做与 content 相同的规范化和长度限制。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| export function isPidAlive(pid: number): boolean { | ||
| if (!Number.isInteger(pid) || pid <= 0) return false; |
There was a problem hiding this comment.
[Suggestion] A zombie (exited-but-unreaped) process passes both liveness checks, so the registry treats a dead session as alive until its parent reaps it — possibly never. — Failure scenario: empirically reproduced on this host with a real zombie: process.kill(pid, 0) succeeds and /proc/<pid>/stat keeps the original starttime, so isSameProcess returns true and the record survives every sweep — qwen sessions ps (this PR's flagship command, which consumes listLiveSessions directly) lists the dead session indefinitely; a same-uid process can also register against a zombie it owns for a permanently unsweepable fabricated entry. (Peer discovery masks this via dial-probe, but sessions ps does not.) Fix: the Linux branch already parses the stat line — the state char lands at fields[0]; treat Z/X/x as dead.
中文说明
[Suggestion] 僵尸进程(已退出但未被回收)能通过两项存活检查,因此注册表会把死会话当作存活,直到其父进程回收为止——而父进程可能永远不回收。失败场景(已在本机用真实僵尸进程实证复现):process.kill(pid, 0) 成功,且 /proc/<pid>/stat 保留原始 starttime,于是 isSameProcess 返回 true,记录在每一次清扫中存活——qwen sessions ps(本 PR 的旗舰命令,直接消费 listLiveSessions)会无限期列出这个死会话;同一 uid 的进程还可以针对自己持有的僵尸进程注册,制造一条永远无法清扫的伪造条目。(peer 发现因拨号探测掩盖了这一点,但 sessions ps 没有。)修复:Linux 分支本来就解析 stat 行——状态字符位于 fields[0];把 Z/X/x 视为死亡即可。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| * treats as the cautious case rather than as a match. | ||
| */ | ||
| fromMode?: 'bypass' | 'prompting'; | ||
| priority: PeerMessagePriority; |
There was a problem hiding this comment.
[Suggestion] Several fields added by this PR are declared, documented and even tested but have no production read or write sites (AGENTS.md: "for every added field, grep its read sites"): PeerUserFrame.priority here — 'now' is never produced (the only sender passes the 'next' default) and never consumed (submit() discards it), yet the contract documents it as live behavior and the header invites hand-built frames that set it to no effect; PeerMessagingOptions.selfName (peer-messaging.ts — fromName actually comes from the registry); HeldMessage.heldAt (inbound-gate.ts); registry peerProtocol (written and parsed, but the version gate it exists for is never enforced in toPeer); PeerMessaging.selfModeClass() (unit-tested, never called — the send path derives fromMode independently, so the green test manufactures false confidence); ListLiveSessionsOptions.sweepStale/selfPid (set only by the module's own tests); and the RegisterSessionFields.pid + pid-override seams (set by no caller, production or test, contradicting the "Tests pass an explicit value" doc). — Concrete cost: each advertises a contract that does not exist; priority: 'now' silently behaves as 'next', and future wiring work will build against untested dead paths. Fix: wire each end-to-end or drop it until a consumer exists (document test-only seams the way the doc claims).
中文说明
[Suggestion] 本 PR 新增的若干字段被声明、被文档化甚至被测试,但没有任何生产读写点(AGENTS.md:"对每个新增字段,grep 其读取点"):此处的 PeerUserFrame.priority——'now' 从未被产生(唯一的发送方传 'next' 默认值)也从未被消费(submit() 丢弃它),但契约将其描述为有效行为,且文件头邀请手工构造帧使其设置了也无效;PeerMessagingOptions.selfName(peer-messaging.ts——fromName 实际来自注册表);HeldMessage.heldAt(inbound-gate.ts);注册表 peerProtocol(被写入和解析,但它所服务版本门从未在 toPeer 中执行);PeerMessaging.selfModeClass()(有单元测试、从未被调用——发送路径独立推导 fromMode,绿色测试制造了虚假的覆盖信心);ListLiveSessionsOptions.sweepStale/selfPid(仅被模块自己的测试设置);以及 RegisterSessionFields.pid 与 pid 覆盖参数(没有任何调用方设置,无论生产还是测试,与"Tests pass an explicit value"的文档相矛盾)。具体代价:每一项都在宣传一个不存在的契约;priority: 'now' 静默地表现为 'next',未来的接线工作会建立在未经测试的死路径上。修复:要么端到端接通,要么在有消费者之前删除(把仅供测试的参数按文档声称的方式标注清楚)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
已被后续 commit 取代,当前 head 需重新 review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its platform suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its platform suite did not run locally.
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 — ended at the 3-round huge-diff cap with round 3 still reporting findings; two consecutive dry rounds were not reached (no convergence claim).
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; PR #8733 adds cross-session peer addressing: list_agents ...: did not run npm run build /typecheck; cross-file verification is grep/read-based against the checked-out worktree.; chunk 12: could not execute vitest run src/ipc/inbound-gate.test.ts — node_modules is absent from the review worktree and installing was out of budget; verification was…; chunk 5: none — all planned checks completed within budget., and 14 more.
Test Plan (not a blocker): src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 19135, 19795, 481, 1224, 1505 passed; and 3 more.
中文说明
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its platform suite did not run locally。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its platform suite did not run locally。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:reverse audit — ended at the 3-round huge-diff cap with round 3 still reporting findings; two consecutive dry rounds were not reached (no convergence claim)。
未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks I started were completed within budget.;PR #8733 adds cross-session peer addressing: list_agents ...:did not run npm run build /typecheck; cross-file verification is grep/read-based against the checked-out worktree.;chunk 12:could not execute vitest run src/ipc/inbound-gate.test.ts — node_modules is absent from the review worktree and installing was out of budget; verification was…;chunk 5:none — all planned checks completed within budget.,另有 14 条。
Test Plan(非阻断):src/ipc/peer-directory.test.ts — no such file or directory; src/ipc/peer-send.test.ts — no such file or directory; src/tools/send-message.test.ts — no such file or directory; src/tools/list-agents.test.ts — no such file or directory; 23 passed — this review observed 19135, 19795, 481, 1224, 1505 passed; and 3 more。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| function sanitize(value: string): string { | ||
| const stripped = value.replace(/[\r\n\t]/g, ''); | ||
| const escaped = escapeAnsiCtrlCodes(stripped); |
There was a problem hiding this comment.
[Suggestion] sanitize() hand-rolls a parallel copy of the terminal-sanitization pipeline instead of composing the canonical sanitizeTerminalText from the already-imported textUtils.ts — this duplication is the mechanism that let the table ship without the bidi pass (thread R8-4), and padDisplay/truncate below are likewise copied verbatim from sessions/list.ts. — Failure scenario: every other new untrusted-text render site in this PR calls sanitizeTerminalText, so the next hardening pass (a new spoofing/invisible class — exactly how the bidi pass was added for Trojan Source) lands on every surface EXCEPT sessions ps, and fixing R8-4 by appending the regex locally leaves the identical copy in sessions list as the remaining hand-rolled surface. Fix: compose the canonical helper — return sanitizeTerminalText(value).replace(/[\r\n\t]/g, ''); (preserves behavior: canonical keeps TAB/LF and already strips CR) — and share padDisplay/truncate via commands/sessions/common.ts.
中文说明
[Suggestion] sanitize() 手搓了一套终端净化的并行副本,而不是组合已 import 的 textUtils.ts 中的标准 sanitizeTerminalText——这种重复正是让该表格在没有 bidi 处理(R8-4 线索)的情况下发布的机制;下面的 padDisplay/truncate 同样是从 sessions/list.ts 原样复制。失败场景:本 PR 中其他所有新的不可信文本渲染点都调用 sanitizeTerminalText,因此下一次加固(新的欺骗/不可见字符类别——bidi 处理正是这样因 Trojan Source 被加上的)会落在除 sessions ps 之外的所有表面;而通过本地追加正则来修 R8-4 又会留下 sessions list 中那份相同的副本作为残留的手搓表面。修复:组合标准辅助函数——return sanitizeTerminalText(value).replace(/[\r\n\t]/g, '');(行为保持不变:标准函数保留 TAB/LF 且已去除 CR)——并通过 commands/sessions/common.ts 共享 padDisplay/truncate。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| 'Review messages held from other Qwen Code sessions (accept | deny)': | ||
| 'Review messages held from other Qwen Code sessions (accept | deny)', |
There was a problem hiding this comment.
[Suggestion] The new /peers hint key is translated only in en.js, zh.js, zh-TW.js; ca, de, fr, ja, pt, ru have no entry and t() falls back to the English key (translations[key] ?? key). The adjacent key added in the same batch is present in all 9 locales. — Failure scenario: a user with QWEN_CODE_LANG=ja opens the /peers review view and sees the English hint inside an otherwise-localized UI. Fix: add the key to the six missing locale files.
中文说明
[Suggestion] 新增的 /peers 提示键只在 en.js、zh.js、zh-TW.js 中有翻译;ca、de、fr、ja、pt、ru 没有条目,t() 会回退到英文键(translations[key] ?? key)。同一批新增的相邻键在全部 9 个语言文件中都存在。失败场景:QWEN_CODE_LANG=ja 的用户打开 /peers 审查视图时,会在 otherwise 本地化的界面中看到英文提示。修复:在缺失的 6 个语言文件中补上该键。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| export { PeerMessaging, type PeerSubmitFn } from './peer-messaging.js'; | ||
| export { | ||
| PeerMessagingContext, |
There was a problem hiding this comment.
[Suggestion] This new barrel has zero consumers — all five import sites (startInteractiveUI.tsx, AppContainer.tsx, slashCommandProcessor.ts, ui/commands/types.ts, both test files) import peer-messaging.js or PeerMessagingContext.js directly (grep-verified). — Failure scenario (concrete cost): the barrel must be kept in sync with its two source modules' exports forever with no consumer; an export renamed or removed in peer-messaging.ts forces a barrel edit nobody is forced to notice until a future importer hits a compile error. Speculative API surface the repo's simplicity rule asks not to ship. Fix: delete packages/cli/src/peerMessaging/index.ts, or route the existing importers through it — one import convention, not two.
中文说明
[Suggestion] 这个新 barrel 没有任何消费者——全部五个 import 点(startInteractiveUI.tsx、AppContainer.tsx、slashCommandProcessor.ts、ui/commands/types.ts、两个测试文件)都直接 import peer-messaging.js 或 PeerMessagingContext.js(已 grep 验证)。失败场景(具体代价):这个 barrel 必须永远与两个源模块的导出保持同步却没有任何消费者;peer-messaging.ts 中任何导出的重命名或删除都会强制产生一次没人会被提醒去做的 barrel 修改,直到未来某个 importer 撞上编译错误。这是仓库简洁性规则要求不要发布的投机性 API 表面。修复:删除 packages/cli/src/peerMessaging/index.ts,或让现有 importer 改走它——一种 import 约定,而不是两种。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| InboundGate: class { | ||
| constructor( | ||
| private readonly options: { |
There was a problem hiding this comment.
[Suggestion] PeerMessaging.start → InboundGate getPolicySetting passthrough (peer-messaging.ts) is pinned by no test: this startup test mocks InboundGate entirely (the constructor ignores every option but deliver/reportStatus), and every real-socket test in peer-messaging.test.ts hardcodes getPolicySetting: () => undefined. The sibling getApprovalMode passthrough IS pinned (real-socket tests pass real modes and assert hold/deliver outcomes). — Failure scenario: probe-verified: replacing the passthrough with the shape-preserving getPolicySetting: () => undefined leaves all 16 tests green — the user's agents.crossSessionInbound setting (the only opt-out/refuse control for inbound peer messages besides disabling the feature) would be silently ignored and every session fall back to parity behavior, with no test red. Fix: add one real-socket test starting PeerMessaging with getPolicySetting: () => 'refuse' (or 'hold') and asserting the frame is refused/held instead of delivered.
中文说明
[Suggestion] PeerMessaging.start → InboundGate 的 getPolicySetting 透传(peer-messaging.ts)没有任何测试钉住:这个启动测试完整 mock 了 InboundGate(构造函数忽略除 deliver/reportStatus 之外的所有选项),而 peer-messaging.test.ts 中每个真实 socket 测试都把 getPolicySetting 硬编码为 () => undefined。兄弟项 getApprovalMode 透传是有钉住的(真实 socket 测试传入真实模式并断言暂扣/投递结果)。失败场景(探针验证):把透传替换为保持形状的 getPolicySetting: () => undefined,16 个测试全部保持绿色——用户的 agents.crossSessionInbound 设置(除禁用功能外唯一对入站 peer 消息的拒绝/退出控制)会被静默忽略,每个会话都回退到 parity 行为,且没有任何测试变红。修复:新增一个真实 socket 测试,以 getPolicySetting: () => 'refuse'(或 'hold')启动 PeerMessaging,断言帧被拒绝/暂扣而不是被投递。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| MAX_HELD_MESSAGES: 50, | ||
| patchSessionRecord: vi.fn(), | ||
| sendDeliveryStatus: vi.fn(async () => undefined), |
There was a problem hiding this comment.
[Suggestion] PeerMessaging.start's address advertisement (await patchSessionRecord({ ipcPath: inbox.socketPath }), peer-messaging.ts) is pinned by no test: here it is an unasserted vi.fn() mock, and in peer-messaging.test.ts the real patchSessionRecord is a silent no-op because the test process never registers a record; gemini.test.tsx mocks registerSession for hygiene and never enables the flag. — Failure scenario: drop or misspell the advertise call and the whole suite stays green while the session binds a listening socket but never publishes it: toPeer filters out records with no ipcPath, listMessageablePeers returns nothing, sendToPeer answers not-found — the feature silently dead while appearing enabled. Fix: assert the mocked patchSessionRecord was called with { ipcPath } (and after startPeerInbox resolved); alternatively register a real record in a tmp-dir registry in peer-messaging.test.ts and assert ipcPath appears after start and is cleared after close().
中文说明
[Suggestion] PeerMessaging.start 的地址公告(await patchSessionRecord({ ipcPath: inbox.socketPath }),位于 peer-messaging.ts)没有任何测试钉住:在这里它只是一个未被断言的 vi.fn() mock,而在 peer-messaging.test.ts 中真实的 patchSessionRecord 是静默的空操作(因为测试进程从未注册过记录);gemini.test.tsx 只是出于卫生考虑 mock 了 registerSession,且从未启用该标志。失败场景:丢弃或写错该公告调用,整个套件依旧绿色,而会话绑定了监听 socket 却从未发布它:toPeer 会过滤掉没有 ipcPath 的记录,listMessageablePeers 返回空,sendToPeer 回答 not-found——功能看似启用实则静默失效。修复:断言被 mock 的 patchSessionRecord 以 { ipcPath } 被调用(且在 startPeerInbox resolve 之后);或者在 peer-messaging.test.ts 中于 tmp 目录注册表里注册一条真实记录,断言 start 之后出现 ipcPath、close() 之后被清除。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| /** Include the calling process's own record. Defaults to false. */ | ||
| includeSelf?: boolean; |
There was a problem hiding this comment.
[Suggestion] includeSelf (and sessions ps --all, its only production setter) can never include anything in production: registration happens only in startInteractiveUI.tsx (interactive TUI startup), while qwen sessions ps runs as a fresh yargs subcommand process that never registers; even PID recycling can't trigger inclusion (the stale record's token fails isSameProcess against the ps process's own token). The self-branch is exercised only by tests. — Failure scenario: a user runs qwen sessions ps --all expecting their own session per the flag's help text ("Include this process, if it is itself a registered session"); it can never appear — a user-facing flag with a dead promise (dead-switch class at the read site per the project rules). The default listing already shows the interactive session (it is not "self" from the ps process's viewpoint), so --all hides nothing — it is vestigial. Fix: remove --all (and the includeSelf/selfPid options if no future caller needs them), or repoint the flag at something real — e.g. also list records that failed the provenance check (ghost/zombie entries).
中文说明
[Suggestion] includeSelf(以及它唯一的生产设置者 sessions ps --all)在生产中永远包含不了任何东西:注册只发生在 startInteractiveUI.tsx(交互式 TUI 启动),而 qwen sessions ps 作为全新的 yargs 子命令进程运行、从不注册;即便 PID 复用也无法触发包含(陈旧记录的令牌对 ps 进程自己的令牌过不了 isSameProcess)。self 分支只在测试中被执行。失败场景:用户按标志帮助文本("Include this process, if it is itself a registered session")运行 qwen sessions ps --all 期待看到自己的会话;它永远不可能出现——一个带着死承诺的面向用户标志(按项目规则属于读取点的死开关类别)。默认列表本来就会显示该交互式会话(从 ps 进程的视角它并非 "self"),所以 --all 什么也没隐藏——它是退化残留。修复:移除 --all(如果没有未来调用者需要,连同 includeSelf/selfPid 选项),或把标志指向真实的东西——例如同时列出未通过 provenance 检查的记录(幽灵/僵尸条目)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(parsed.sessions).toBeUndefined(); | ||
| expect(parsed.agents).toHaveLength(1); |
There was a problem hiding this comment.
[Suggestion] No test covers background agents AND peer sessions both present in one result: every existing test holds one of the two arrays at length 0, so the two-element parts.join(' and ') returnDisplay ("Listed N background agents and M other sessions.") and the llmContent payload carrying both agents and sessions keys populated are unexercised. — Failure scenario: a regression in the combined branch (wrong separator, wrong pluralization, or dropping one of the two keys when both are populated) ships green. Fix: add a test that registers a background agent while listMessageablePeers resolves to a peer; assert both parsed.agents and parsed.sessions are present and returnDisplay is "Listed 1 background agent and 1 other session."
中文说明
[Suggestion] 没有测试覆盖"同一次结果中同时存在后台 agent 和对等会话"的情况:现有测试总把两个数组之一保持为长度 0,因此两元素的 parts.join(' and ') returnDisplay("Listed N background agents and M other sessions.")以及同时携带 agents 和 sessions 两个非空键的 llmContent 载荷都未被执行。失败场景:组合分支中的回归(错误的分隔符、错误的单复数、或两个键都有值时丢掉其一)会静默合入。修复:新增一个测试——注册一个后台 agent,同时让 listMessageablePeers 解析出一个 peer;断言 parsed.agents 与 parsed.sessions 均存在,且 returnDisplay 为 "Listed 1 background agent and 1 other session."
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(sendToPeer).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: 'docs-cd', | ||
| message: 'check the tests', | ||
| }), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] trySendToPeer's approvalMode forwarding (config.getApprovalMode() → sendToPeer({ approvalMode }), send-message.ts) is pinned by no test — this call-args assertion omits approvalMode; peer-send.test.ts pins only the downstream half (approvalMode → frame fromMode). — Failure scenario: probe-verified: hardcoding approvalMode: null in trySendToPeer ships send-message 30/30 + peer-send 16/16 green; adding approvalMode: DEFAULT_MODE to this assertion fails under the mutant and passes on correct code. Shipped, every peer frame goes out without fromMode, and per InboundGate.resolvePolicy any bypassing receiver then holds EVERY inbound message under 'no-mode-asserted' — the documented bypass-to-bypass auto-deliver parity silently degrades to permanent manual review, with a hold cause that misstates reality. Fix: extend the assertion with approvalMode: DEFAULT_MODE (and optionally one YOLO case).
中文说明
[Suggestion] trySendToPeer 的 approvalMode 转发(config.getApprovalMode() → sendToPeer({ approvalMode }),位于 send-message.ts)没有任何测试钉住——这个调用参数断言漏掉了 approvalMode;peer-send.test.ts 只钉住了下游一半(approvalMode → 帧的 fromMode)。失败场景(探针验证):在 trySendToPeer 中把 approvalMode: null 硬编码,send-message 30/30 + peer-send 16/16 保持绿色;在本断言中补上 approvalMode: DEFAULT_MODE 后,变异下失败、正确代码上通过。若该回归合入,每个 peer 帧都不带 fromMode 发出,按 InboundGate.resolvePolicy,任何 bypass 类接收方会把所有入站消息以 'no-mode-asserted' 暂扣——文档化的 bypass 对 bypass 自动投递 parity 会静默退化为永久人工审查,且暂扣原因与现实不符。修复:在断言中补上 approvalMode: DEFAULT_MODE(可再加一个 YOLO 用例)。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (to === '*') { | ||
| const msg = | ||
| 'Broadcast (to: "*") is no longer supported — send one message per recipient.'; |
There was a problem hiding this comment.
[Suggestion] Removing the tool-level broadcast leaves TeamManager.broadcast with zero production callers — a repo-wide grep finds only the definition (TeamManager.ts:619-647) and two coordination-harness.test.ts call sites — while the PR's stated design is that broadcast "has no sensible meaning" once recipients can include sessions doing unrelated work. — Failure scenario (concrete cost): a team-wide broadcast primitive plus its dedicated tests continues to be maintained against the stated design, and any future code can re-wire team-wide broadcast one layer below this rejection (TeamManager.ts:622 is one call away) without confronting the decision recorded here. Fix: delete TeamManager.broadcast and rewrite its two test call sites as per-recipient sends — or keep it deliberately, with a comment saying why it outlives its entry point.
中文说明
[Suggestion] 移除工具层广播之后,TeamManager.broadcast 在生产代码中已无任何调用者——全仓库 grep 只找到定义(TeamManager.ts:619-647)和 coordination-harness.test.ts 中的两处调用——而 PR 明确给出的设计理由是:一旦接收者可能包含在做无关工作的会话,广播就"没有合理含义"。失败场景(具体代价):一个团队级广播原语及其专门测试继续违背已声明的设计被维护着,且未来任何代码都可以在这个拒绝逻辑的下一层重新接上团队级广播(TeamManager.ts:622 只差一次调用),而无需直面这里记录的设计决定。修复:删除 TeamManager.broadcast,把两处测试调用改写为逐个接收者发送——或者刻意保留它,并加注释说明它为何比其入口活得更久。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| .split(/\s+/); | ||
| const startTicks = fields[19]; |
There was a problem hiding this comment.
[Suggestion] The /proc/<pid>/stat field-22 offset (fields[19]) is pinned by no ground-truth assertion: all four readProcStartToken/isSameProcess tests are self-consistent (format regex /^linux:[0-9a-f-]+:\d+$/, stability compares the function against itself, the mismatch test uses a fabricated boot id). Adjacent indices parse "successfully" — measured on this host: fields[18] (itrealvalue) is 0 on every kernel ≥2.6 and fields[20] (vsize) is a plausible integer, both matching ^\d+$. — Failure scenario: probe-verified: drifting to fields[18] ships all 11 tests green and yields token linux:<boot>:0 for EVERY process — isSameProcess then confirms ANY recycled PID as the original session, silently disabling machine-wide the PID-recycle protection this file exists to close (the registry attributes a session record and its peer-messaging identity to an unrelated process). A ground-truth probe (two processes started at different times must yield different tokens) fails under the mutant and passes once reverted. The offset itself is correct per proc(5); the finding is the missing pin. Fix: add a ground-truth test — assert the tick component matches an independently parsed starttime, or that two processes started at different times yield different tokens.
中文说明
[Suggestion] /proc/<pid>/stat 的 field-22 偏移(fields[19])没有任何基于事实的断言钉住:全部四个 readProcStartToken/isSameProcess 测试都是自洽的(格式正则 /^linux:[0-9a-f-]+:\d+$/、稳定性测试是函数自己与自己比较、mismatch 测试用的是伪造的 boot id)。相邻索引也能"成功"解析——已在本机实测:fields[18](itrealvalue)在所有 ≥2.6 的内核上都是 0,fields[20](vsize)是像样的整数,两者都匹配 ^\d+$。失败场景(探针验证):漂移到 fields[18] 后 11 个测试全部保持绿色,且每个进程都得到令牌 linux:<boot>:0——isSameProcess 从此会把任何复用的 PID 都确认为原会话,本文件存在的全部意义(PID 复用防护)在整机范围被静默废除(注册表会把会话记录及其 peer-messaging 身份归属给一个不相干的进程)。事实探针(两个不同时间启动的进程必须产生不同令牌)在变异下失败、恢复后通过。偏移本身按 proc(5) 是正确的;问题在于缺少钉扎。修复:补一个基于事实的测试——断言 tick 分量与独立解析出的 starttime 一致,或断言两个不同时间启动的进程产生不同令牌。
— qwen3.8-max via Qwen Code /review (v0.21.10)
已被后续 commit 3e5a76d 取代,当前 head 需重新 review。
|
Closing for now to concentrate review on the first step of the stack, #8728. Nothing here is abandoned and nothing is lost — the branch stays, and this PR gets reopened once #8728 lands. The reasoning: all four PRs sit on the same stack, so this one currently shows the whole stack in its diff, which makes it expensive to review and duplicates findings the reviewer has already filed against the earlier commits. Landing #8728 first shrinks each later PR back to its own change. Tracking issue: #8724. |
|
PR #8733 is closed (not merged). The review window has closed — there is nothing to add. |
Final step of #8724.
list_agentsnow shows the other Qwen Code sessions running on this machine alongside this session's background agents, andsend_messagecan reach one of them by name.What this PR does
list_agentsgains asessionsarray listing the other live Qwen Code sessions on the machine, andsend_messageaccepts one of those session names in its existingtofield. A name is the address: a socket path changes every restart, but a name survives one, reads back to the user, and is already whatqwen sessions psprints.Names are derived from the working directory and so are not unique. Each session therefore also carries a six-character
refderived from its session id, andlist_agentsappends it only when two rows would otherwise be indistinguishable — the common case stays a bare, typeable name, and the model is never handed a UUID to copy.Ambiguity is an error, not a guess. A bare name matching two live sessions returns the candidates with their directories and asks for the full
name [ref]. It does not pick one. Injecting a message into the wrong session cannot be undone by retrying — that session has already read it and may have acted. This is also what removes the need for the obvious alternative, pinning a name to a session for the length of a conversation: a name that could mean two sessions never silently switches between them, because it never resolves at all. Less state, stronger guarantee.Routing order is background task (
task_id) → teammate (to) → peer session (to), with four rules. In-process wins a name collision, because a teammate is part of this session's own work and quietly routing off-process would be the more surprising of the two. A structured control message never crosses a session boundary —shutdown_requestis a leader/teammate protocol, and shipping it across would let a peer request this session's shutdown, so peer sends are plain text only. Sending requires an inbox of this session's own, which is also what the feature flag gates: a message with no reply address is one the recipient cannot answer, and the receipt path from #8730 depends on it. And a session never addresses itself, since its own record can appear in the directory and without the filter a message would loop back into its own queue.Failures are described, not collapsed, so the model can tell re-discovering apart from retrying:
ENOENT/ECONNREFUSEDEBUSYETIMEDOUTDiscovery itself is defensive: a registry record only becomes a listed peer after its socket answers a dial. A record can outlive its process by the width of a crash, and a stale socket file still stats fine. Probes run concurrently, so the call costs about one probe's latency however many sessions are registered.
Broadcast is removed.
to: "*"now returns an error telling the caller to send one message per recipient. It was linear in team size, and it has no sensible meaning once "everyone" could include sessions doing unrelated work.TeamManager.broadcastis untouched and still reachable internally; only the tool's entry point is gone.Why it's needed
Two Qwen Code sessions on the same machine currently cannot see or reach each other, so coordinating them is a manual copy-paste job through the human sitting between them. #8728 made sessions discoverable and #8730 gave them a gated inbox; without this PR neither is reachable from the model's own tools, so the feature is not usable from a conversation. Addressing by name rather than by socket path is what makes it usable at all — the model can read a name out of
list_agentsand pass it straight back intosend_message, and the user can read the same name inqwen sessions ps.Reviewer Test Plan
How to verify
Two real sessions on one machine, both on this branch, with
agents.crossSessionMessaging: true. Build and launch withnpm run build && node scripts/start.js(note:scripts/dev.jssetsDEV=trueand does not paint the TUI in a tmux pane — that is unrelated to this branch).sessions, each with ato,name,refandcwd.<cross_session_message>envelope carrying the sender's name.kill -9and list again from the survivor. Expect the dead session not to be listed, even though its socket file is still on disk — discovery dials rather than stats.Unit tests for the parts a manual run does not reach:
Everything from the earlier steps re-runs green: the rest of
src/ipc/(99),session-registry(23), and all ofsrc/agents/team/(227) to confirm removing the broadcast entry point regressed nothing.npm run build,npm run typecheck, lint and prettier clean on every changed file.Two existing
send_messagetests needed updating rather than just passing: the broadcast test now asserts the rejection, and theTeamManagermocks gainedgetTeamFile()because the teammate-vs-peer decision has to enumerate members. The mocks were incomplete against the realTeamManagertype; that is a test fix, not a production workaround.Evidence (Before & After)
Before — the two sessions cannot see each other at all.
list_agentsreturns only this session's own background agents, with nosessionsarray, and there is no name thatsend_messagewill accept for another session.After — from a live run of two real sessions in different directories, they resolve, list and deliver by name:
list_agentsthrough the model returned the live peers ("Listed 5 other sessions"), andsend_messageto a peer by bare name resolved and delivered; the model's own summary of the result correctly warned that the message may be held before the other session acts on it. The receiving session rendered the envelope as designed:Discovery correctly ignored a socket file left behind by a
kill -9'd session — it dials rather than stats, so the stale inode resolved tonot-foundinstead of a phantom peer.Full live-run write-ups, including the held/accept path from #8730 that this PR's send side feeds: #8730 first run, #8730 UI verification, and #8733 name-resolution note.
Tested on
macOS is not hand-tested; the IPC path is POSIX and is expected to behave as on Linux, but nobody has watched it. Windows is N/A — named pipes are explicitly out of scope (see below), so there is no peer transport to exercise there.
Environment (optional)
Linux, two sessions launched with
npm run build && node scripts/start.js,agents.crossSessionMessaging: true, sockets under$XDG_RUNTIME_DIR/qwen-socks/.Risk & Scope
deriveSessionNameis the cwd basename plus two hex characters of the session id, so two sessions started in the same directory still get different names 255 times out of 256. I am not arguing for removing it — it is the correct behaviour for the collision that does happen, and it becomes load-bearing the moment sessions can be renamed by the user — but it should not be treated as battle-worn. Also out of scope and not planned here: cross-machine transport, file transfer, and Windows named pipes; the IPC path is abstracted behindresolvePeerSocketPath/isLocalIpcPathso the last of those can be added without touching anything above it.send_messagewithto: "*"no longer broadcasts — it returns an error directing the caller to send one message per recipient. This changes an existing tool contract. Only the tool entry point is affected;TeamManager.broadcastis untouched and still reachable internally, so in-process callers are unaffected. Everything else here is additive and gated off by default behindagents.crossSessionMessaging.Linked Issues
Closes #8724.
Stacked on #8728 and #8730 — both must merge first.
中文说明
#8724 的最后一步。
list_agents现在会在本会话的后台 agent 之外,一并列出本机正在运行的其他 Qwen Code 会话,send_message也可以按名称直接向其中一个发送消息。这个 PR 做了什么
list_agents新增sessions数组,列出本机其他存活的 Qwen Code 会话;send_message的现有to字段可以接受其中的会话名称。名称就是地址:socket 路径每次重启都会变,而名称能跨越重启、对用户可读,并且已经是qwen sessions ps打印的内容。名称由工作目录派生,因此并不唯一。所以每个会话还带有一个由 session id 派生的六位
ref,并且list_agents只在两行原本无法区分时才追加它——常见情况下仍然是一个简短、可直接输入的名称,模型也永远不需要复制 UUID。歧义是错误,不是靠猜。 当一个简短名称同时匹配两个存活会话时,返回候选列表(含各自的工作目录)并要求使用完整的
name [ref],而不会替用户挑一个。把消息注入错误的会话无法通过重试撤销——那个会话已经读到了,并且可能已经据此行动。这同时也让另一种常见方案变得不必要,即在一次对话期间把名称固定绑定到某个会话:一个可能指向两个会话的名称永远不会在两者之间悄悄切换,因为它根本不会解析成功。状态更少,保证更强。路由顺序是:后台任务(
task_id)→ 队友(to)→ 对等会话(to),并有四条规则。名称冲突时进程内优先,因为队友属于本会话自己的工作,悄悄路由到进程外才是更令人意外的行为。结构化控制消息永远不跨越会话边界——shutdown_request是 leader/teammate 协议,跨会话传递会让一个对等方能请求关闭本会话,因此对等发送只允许纯文本。发送方自己必须拥有 inbox,这也正是特性开关所控制的:没有回信地址的消息,接收方无法回应,而 #8730 的回执路径依赖于此。会话也永远不会给自己发消息,因为它自己的记录可能出现在目录中,没有这个过滤消息会回流进自己的队列。失败被明确描述,而不是被压平,这样模型才能区分"重新发现"和"直接重试":
ENOENT/ECONNREFUSEDEBUSYETIMEDOUT发现过程本身是防御式的:注册表记录只有在其 socket 应答拨号之后,才会成为被列出的对等会话。记录可能在进程崩溃的瞬间存活得更久,而残留的 socket 文件 stat 起来依然正常。探测并发执行,因此无论注册了多少会话,这次调用的开销大约只相当于一次探测的延迟。
广播已移除。
to: "*"现在返回错误,提示调用方对每个接收者分别发送。它的开销与团队规模成正比,而且一旦"所有人"可能包含正在做无关工作的会话,它就失去了合理含义。TeamManager.broadcast未改动,内部仍可调用;移除的只是工具的入口。为什么需要
同一台机器上的两个 Qwen Code 会话目前彼此不可见、也无法互相访问,因此协调它们只能靠中间的人手工复制粘贴。#8728 让会话可被发现,#8730 提供了带门禁的 inbox;但没有本 PR,二者都无法从模型自己的工具中触达,功能在对话里根本用不起来。按名称而非 socket 路径寻址正是可用性的关键——模型可以从
list_agents读出名称并直接传回send_message,用户也能在qwen sessions ps中看到同一个名称。审查者测试计划
如何验证
同一台机器上的两个真实会话,均基于本分支,设置
agents.crossSessionMessaging: true。使用npm run build && node scripts/start.js构建并启动(注意:scripts/dev.js会设置DEV=true,在 tmux pane 中不渲染 TUI,这与本分支无关)。sessions下,各自带有to、name、ref和cwd。<cross_session_message>信封中。kill -9杀掉一个会话,再从存活会话列出。预期已死会话不会被列出,即使其 socket 文件仍在磁盘上——发现过程是拨号而非 stat。手工运行覆盖不到的部分由单元测试覆盖:
前几步的测试全部重跑通过:
src/ipc/其余部分(99)、session-registry(23),以及src/agents/team/全部(227),用以确认移除广播入口没有造成回归。npm run build、npm run typecheck、lint 与 prettier 在所有改动文件上均干净。有两个已有的
send_message测试需要修改而非直接通过:广播测试现在断言其被拒绝;TeamManagermock 增加了getTeamFile(),因为"队友还是对等会话"的判定需要枚举成员。这些 mock 相对真实的TeamManager类型本就是不完整的;这属于测试修正,不是为绕过生产代码而做的妥协。证据(前后对比)
之前——两个会话完全看不到彼此。
list_agents只返回本会话自己的后台 agent,没有sessions数组,也不存在任何能被send_message接受的、指向另一个会话的名称。之后——来自两个位于不同目录的真实会话的实测运行,它们能够按名称解析、列出并送达:
通过模型调用
list_agents返回了存活的对等会话("Listed 5 other sessions");send_message按简短名称发送给对等会话并成功送达,模型对结果的总结也正确地提示该消息在对方处理之前可能被暂扣。接收方会话按设计渲染了信封:发现过程正确忽略了被
kill -9杀掉的会话留下的 socket 文件——它是拨号而非 stat,因此这个残留 inode 解析为not-found,而不是一个幽灵对等会话。完整的实测记录(包括 #8730 中由本 PR 发送侧驱动的暂扣/接受路径):#8730 首次实测、#8730 UI 验证,以及 #8733 名称解析记录。
测试环境
macOS 未做手工测试;IPC 路径是 POSIX 的,预期表现与 Linux 一致,但没有人实际观察过。Windows 为 N/A——命名管道明确不在本 PR 范围内(见下),因此那里没有可供验证的对等传输。
运行环境(可选)
Linux,两个会话以
npm run build && node scripts/start.js启动,agents.crossSessionMessaging: true,socket 位于$XDG_RUNTIME_DIR/qwen-socks/。风险与范围
deriveSessionName是工作目录的 basename 加上 session id 的两位十六进制字符,因此即便在同一目录下启动的两个会话,也有 255/256 的概率得到不同名称。我并不主张移除它——对于确实会发生的冲突,它的行为是正确的,而且一旦支持用户重命名会话,它就会变得关键——但它不应被视为久经考验。同样不在范围内、也不计划在此处实现的还有:跨机器传输、文件传输,以及 Windows 命名管道;IPC 路径被抽象在resolvePeerSocketPath/isLocalIpcPath之后,因此最后一项可以在不触动其上任何代码的情况下补充。send_message的to: "*"不再广播——它会返回错误,提示调用方对每个接收者分别发送。这改变了一个已有的工具契约。仅影响工具入口;TeamManager.broadcast未改动且内部仍可调用,因此进程内调用方不受影响。其余改动均为新增,且默认由agents.crossSessionMessaging关闭。关联 Issue
Closes #8724。
叠加在 #8728 与 #8730 之上——两者需先合并。