feat(ipc): tell senders when a message is refused, and expire held ones - #10809
Conversation
A send_message call returns as soon as the frame is handed over; what became of it arrives later as a receipt. Two of those receipts were wrong or missing. A session whose agents.crossSessionInbound is "refuse" turns every peer message away at admission — nobody sees it — but the sender was told "denied", which means a person reviewed it and said no. The sending model cannot tell those apart, and they call for opposite behaviour: a decision is worth raising with that person, a policy refusal means stop. There is now a "refused" status that says so, and whose description tells the sender not to re-send. A message that was already parked when the user switches the setting to "refuse" still settles as "denied" — someone chose, just after the fact — so "refused" is reachable only from pending, never from held. And a hold had no end. It waited for a review that might never come, because the user may not be at that terminal, and the sender could not distinguish "still waiting" from "never coming"; the only thing that ever settled a hold was the session exiting. A parked message now expires after agents.crossSessionHeldExpiry — 1m, 5m, 10m or never, five minutes by default — and the sender is told. /peers shows how much time each message has left, because a review screen that hides its own deadline invites decisions that arrive after the sender has stopped listening. The gate arms one unref'd timer for whichever message expires first rather than one per message, and re-arms it after every change to the buffer. It also sweeps overdue entries at each entry point instead of trusting the timer: a laptop that suspends for an hour must not wake up and deliver a message from before it slept. Expiry is judged against the lifetime configured now, so shortening the setting settles a backlog that is already too old and lengthening it extends what is still waiting — the reading under which what /peers shows as remaining is what actually happens. An unset or unrecognized setting value falls back to the default rather than to never: failing closed here means bounding how long a sender waits, not extending it indefinitely on a typo. The wire gains a value, not a version. An older sender that does not know "refused" drops the receipt as unparseable and learns nothing, which is where it stood before.
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Both gaps are structural properties of the receipt state machine, and both come with before/after evidence over a real socket — a Direction: clearly aligned. Claude Code's CHANGELOG already shipped this exact semantics — "sending to a session on this machine that refuses inbound messages now reports 'refused' to the sender instead of a silent success" — plus honest receipts when an inbox drops messages. This PR brings the qwen-code receipt channel to the same place, and it follows #10764 (merged today) in the same IPC work stream; the author also introduced the inbound gate itself (#9576). Size: core paths touched ( Approach: the scope feels right. Two receipts, one new status with a reachable-only-from- Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题: 已观测到的问题,不是理论性的。两个缺口都是回执状态机的结构性问题,且都附有经由真实 socket 的 before/after 证据—— 方向: 明确对齐。Claude Code 的 CHANGELOG 已经发布了完全相同的语义——"向拒收消息的会话发送时,向发送方报告 'refused' 而非静默成功"——以及收件箱丢弃消息时的诚实回执。本 PR 让 qwen-code 的回执通道达到同一水平;它追随今天刚合并的 #10764,属于同一 IPC 工作流,作者本人也是入站闸门(#9576)的引入者。 规模: 触及核心路径( 方案: 范围合理。两个回执、一个仅可从 风险: 无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
I read this against my own independent pass at the same problem (a new One blocking finding. One non-blocking note: sequenceDiagram
participant P1 as Sender session
participant P2 as InboundGate (receiver)
participant P3 as Expiry timer
participant P4 as Peers review
P1->>P2: send_message frame
P2->>P2: sweep overdue entries on admit
P2->>P1: receipt held
P2->>P3: arm for earliest deadline (unrefd)
Note over P2,P3: every buffer change re-arms the timer
P4->>P2: decide (approve or deny)
P2->>P2: sweep overdue first, expired reads as gone
alt decided in time
P2->>P1: receipt delivered or denied
else timer or sweep finds it overdue
P2->>P1: receipt expired
end
Testing — this section carries the PR's own CI signal, read via the API; no PR code was built or run here. As of this pass, on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 中文说明我按自己对同一问题的独立方案读了一遍这个 PR(准入处新增 一个阻塞性问题。 一条非阻塞备注: 测试:本节携带的是该 PR 自己的 CI 信号(经 API 读取),此处未构建或运行任何 PR 代码。截至本次审查, — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — one concrete correctness bug against the PR's own fail-closed invariant; everything else is about as clean as this gate sees. Stepping back: this PR's shape is exactly the one I'd have proposed independently — new It is not mergeable as-is, though, for the one finding in my review above: 中文说明Confidence: 2/5 —— 针对该 PR 自身 fail-closed 不变量的一个具体正确性 bug;除此之外,以本门禁的标准看几乎无可挑剔。 退一步看:这个 PR 的形状与我的独立方案完全一致——准入处判定、仅可从 但它目前不可合并,原因就是我上面审查中的那一条发现: — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs one rework before this can merge — see my review comment above for the detail. 🙏
The short version: parseHeldExpiry uses value in HELD_EXPIRY_VALUES, and in walks the prototype chain, so unrecognized values like 'constructor' or 'toString' skip the fallback and return an inherited function as the expiry — held messages then never expire and the re-arm loop spins at ~1 ms. That breaks the very invariant this PR documents and tests ("unrecognized values fall back to the default, not to never"). One-line fix — Object.hasOwn(HELD_EXPIRY_VALUES, value) — plus a regression case next to the existing fallback tests, and this is ready.
@qqqys everything else here is genuinely well done — the state-machine split between refused and denied, the single re-armed timer with entry-point sweeps, and the test coverage are all exactly right.
中文说明
合并前需要一处修改——详见我上方的审查评论。
简版:parseHeldExpiry 使用 value in HELD_EXPIRY_VALUES,而 in 会走原型链,因此像 'constructor'、'toString' 这类无法识别的取值会绕过回落逻辑,把继承来的函数当作过期时长返回——被留置的消息将永不过期,重武装循环会以约 1 毫秒的间隔空转。这打破了本 PR 自己记录并测试过的不变量("无法识别的取值回落到默认值,而非 never")。一行修复——Object.hasOwn(HELD_EXPIRY_VALUES, value)——加上现有回退测试旁的一个回归用例,就可以合并了。
@qqqys 其余部分做得非常好——refused 与 denied 的状态机划分、带回重新武装的单个定时器加入口清扫,以及测试覆盖,全都恰到好处。
— Qwen Code · qwen3.8-max
Reviewed at a01f4b74e7850babe1b9ee660270ea69090e253a · re-run with @qwen-code /triage
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its E2E suite did not run locally; Agent 7 ran the changed packages' unit suites green (169 core ipc + 93 cli peer tests).
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its E2E suite did not run locally; Agent 7 ran the changed packages' unit suites green (169 core ipc + 93 cli peer tests)。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
…xpiry Reviewer round 1 on QwenLM#10809. R1-1: nothing re-ran the gate when `agents.crossSessionHeldExpiry` or `crossSessionInbound` changed at runtime, so the live-application behaviour this PR documents never fired. Both reload live, and parking under `never` arms no timer at all -- so editing to `1m` left the backlog held until session exit with no `expired` receipt, while `/peers` counted down from the new value. Added an AppContainer effect beside the approval-mode one, keyed on the parsed lifetime and the policy rather than on any settings edit, since `reevaluate` also settles a parked backlog as `denied` under a refuse policy. R1-2: the expiry deadline was wall-clock while the timer was monotonic. `heldAt` stays wall-clock because the UI renders a countdown from it, but the age is now the larger of the wall and monotonic elapsed times. That keeps a suspended machine expiring holds (CLOCK_MONOTONIC does not tick across suspend) while making a backward NTP correction a no-op instead of stretching a five-minute hold past sixty. The re-armed delay is also clamped to setTimeout's 32-bit ceiling, above which Node clamps to 1 ms and the re-arm spins. `expireOverdue` re-arms after a sweep, so a survivor's deadline no longer waits on unrelated gate activity. R1-3: `reevaluate` appended a failed release at the end of the buffer while keeping its original, older timestamp, so the buffer stopped being oldest-first -- misaiming both the timer armed from the head and the `held.shift()` eviction, which would evict the newest message. The buffer is sorted on rebuild, and the timer scans for the earliest deadline rather than trusting position. R1-13: the staleness guard compared lengths, so an expiry firing between a `/peers` listing and an accept bounced a handle that still resolved to exactly one message. Removals are no longer a change; arrivals and a re-sent id with a fresh `heldAt` still are. R1-14: `/peers accept|deny all` counted only 'done' and 'failed', so a bulk decision over an expired listing reported "Released 0 messages." with no reason. 'gone' has its own counter and clause, kept separate from 'failed' -- an expired message is settled, a failed release is not. R1-4: the socket-driving expiry block is `describe.skipIf(isWindows)` like its two siblings. R1-5: the rounding fixture moved off the 60_000 boundary, where `ceil` and `floor` agree and a one-millisecond gap between two `Date.now()` reads flips the assertion.
Reviewer round 1 on QwenLM#10809, second pass. R1-6: `getHeldExpiryMs`'s two fail-closed fallbacks had no seam and no test, unlike the mode and policy getters beside them. Added `throwOnExpiry` to the harness and cases for both branches. R1-7 / R1-8: the two production wirings of the setting were untested. The command-level fake returned null, which is what omitting the argument produces, so dropping it at the one `formatHeldList` call site kept the suite green; and no test read `getHeldExpiryMs` out of the options `PeerMessaging.start` receives, so the setting could become a dead switch with `never` silently meaning five minutes. R1-9: the only test for `decide()`'s sweep used `advanceTimersByTime`, which fires the armed timer first -- so the guard never ran and deleting it left the test green. Switched to `setSystemTime`, which is the suspended-clock case the guard exists for. R1-10 / R1-16: the `refused` tombstone had no re-send test, and `delivered -> refused` was the one unpinned row of the ledger table. Both are defence against a contradicting peer, which is exactly why the sibling rows are pinned. R1-11: the sender-side notice for `refused` was unpinned, so the user-visible half of the refused/denied distinction could be dropped with the whole file green. R1-15: the hold-lifetime vocabulary was triplicated with nothing coupling the copies. Core exports `HELD_EXPIRY_OPTIONS` and the schema test asserts the option list against it; a schema option added without a core entry now fails instead of silently downgrading to the default. R1-12 / R1-18: two claims in the design doc were wrong. An older sender that does not know `refused` is not "in the same position as before" -- it previously parsed `denied` and now gets silence, which is a regression the doc now states along with why it is accepted anyway. And the consumer of the distinction is the sending user's transcript, not the sending model, which is told nothing by design and by `send_message`'s own result text.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its E2E suite did not run locally; the packages/cli and packages/core full unit suites timed out on this runner (infrastructure), the changed test files ran green via verification probes (AppContainer.test.tsx 181/181, inbound-gate.test.ts 78/78, peers-command + peer-messaging 97/97), and the efficacy probe measured nothing (harnessValidated: null).
Not reviewed: issue-fidelity — closing-issue references could not be fetched (gh 2.45.0 < 2.72.0); scope ruled from the PR's own Linked Issues section (no target issue; #10764 fetched as lineage context, no ask about receipts or expiry).
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/config/settingsSchema.ts:3370 — [probe] agents.crossSessionHeldExpiry missing from the docs/users/configuration/settings.md reference table while both sibling settings are listed (D2-1)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its E2E suite did not run locally; the packages/cli and packages/core full unit suites timed out on this runner (infrastructure), the changed test files ran green via verification probes (AppContainer.test.tsx 181/181, inbound-gate.test.ts 78/78, peers-command + peer-messaging 97/97), and the efficacy probe measured nothing (harnessValidated: null)。
未审查:issue-fidelity — closing-issue references could not be fetched (gh 2.45.0 < 2.72.0); scope ruled from the PR's own Linked Issues section (no target issue; #10764 fetched as lineage context, no ask about receipts or expiry)。
未审查:反向审计——评审时间预算不足,未能开始第 4 轮。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Reviewer round 2 on QwenLM#10809 — all seven findings are fix-induced. R1-13 (Critical): round 1 relaxed `heldSetChangedSinceListing` so a departure is not a change, on the premise that shrinking the set only narrows what a printed handle can mean. That premise fails when a survivor's id extends the departed one. `msgId` is peer-chosen and only shape-checked, so a peer can park `abc` beside `abc12345`; while both are held `resolveHeld`'s exact-match tier gives `abc` to the shorter, and once `abc` expires that same handle falls through to prefix-matching and releases `abc12345` under the reviewed one's handle. The guard now also reports a change when a departed pin is a canonicalized prefix of a surviving id, canonicalized the way `resolveHeld` canonicalizes. Plain shrinkage stays a non-change, as its two existing tests pin. R1-3: the rebuilt buffer sorted on wall-clock `heldAt` while expiry judges `ageOf` (the larger of wall and monotonic). After a backward clock step, entries admitted since the step sort ahead of genuinely older ones, so `held.shift()` evicts a newer message at the cap and receipts its sender `expired` early -- the inversion the sort exists to prevent. Sorted by `ageOf` now, descending for oldest-first. Display followed the same split: `describeRemaining` read the wall clock alone and would promise an hour the gate settles in a minute. It now ages the way the gate does. The clamp test claimed to pin the 32-bit `setTimeout` ceiling but never reached it: vitest's faked `performance.now` moves with `setSystemTime`, so `ageOf` stayed ~0 and the delay was 60_000. Rewritten to drive the only path that can overflow -- an entry with no `monotonicAt`, aged on the wall clock alone -- and it now goes red (2592060000) when the clamp is removed. Tests added for what round 1 left unpinned: `ageOf`'s max-of-clocks (wall-only survived the whole suite), the eviction victim after the clocks diverge, and the effect's policy dependency (dropping `inboundPolicyForPeers` from the deps array was invisible). The stranded expiry-semantics JSDoc, orphaned above `ageOf` when it was inserted, now sits on `expireOverdue` again.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): chunk 5: running packages/core/src/ipc/inbound-gate.test.ts under vitest — the review worktree has no node_modules / dist , and a full monorepo install plus build exc….
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/ipc/inbound-gate.ts:621 — [review] admit() cap eviction evicts positionally via held.shift(), re-breaking the ageOf-ordered eviction invariant after two opposing wall-clock steps — wrong message permanently dropped under '…packages/core/src/ipc/inbound-gate.ts:796 — [review] no test re-sends a timer-expired id to pin recordSettled('expired') in expireOverdue, unlike every other settled verdict (D3-2)
Convergence: round 3 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: packages/cli/src/peerMessaging/peer-messaging.ts (findings in round 1; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):chunk 5:running packages/core/src/ipc/inbound-gate.test.ts under vitest — the review worktree has no node_modules / dist , and a full monorepo install plus build exc…。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 3 轮发布了 1 条行内评论,其中 1 条是首次提出;上一轮发布了 7 条(其中 7 条首次提出)。发现反复回到同一批文件:packages/cli/src/peerMessaging/peer-messaging.ts(第 1 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const liveIds = current.map((entry) => | ||
| canonicalizeMsgId(entry.frame.msgId), |
There was a problem hiding this comment.
[Suggestion] R3-1: The canonicalizeMsgId calls in this new departure exception — the part the comment says exists so the check would not miss the dashed forms resolveHeld matches on — are exercised by no test. Both new tests use only dash-free lowercase ids (abc / abc12345), for which canonicalization is the identity.
MSG_ID_RE admits dashes and uppercase and buildUserFrame emits dashed randomUUID()s, so dashed ids are the ordinary case. A future edit replacing the two canonicalizeMsgId calls with raw ids stays green under both existing tests (startsWith behaves identically for dash-free lowercase ids), shipping a hole: a peer parks ab-c beside abc12345; ab-c expires; the guard compares 'abc12345'.startsWith('ab-c') → false → the listing is declared fresh; the user types the departed message's printed dash-free handle abc, and resolveHeld prefix-matches abc12345 — releasing a different message under the reviewed one's handle, the exact regression the guard was added to stop.
Witness:
probe (peer-messaging.test.ts):
INTACT (dashed-id test added): Tests 53 passed (53)
MUTANT (canonicalizeMsgId calls replaced with raw ids):
Tests 1 failed | 52 passed
AssertionError: expected false to be true
at expect(messaging.heldSetChangedSinceListing()).toBe(true)
(both existing tests stayed green under the mutant)
Fix: extend one of the two existing tests with a dashed or mixed-case departed id — e.g. pin ab-c and survivor abc12345, expire the dashed one, and assert heldSetChangedSinceListing() is true. The test ids must pass MSG_ID_RE (/^[A-Za-z0-9][A-Za-z0-9_-]{0,63}$/, peer-frames.ts:131), which is also what makes dashes/uppercase reachable. Replacing either canonicalizeMsgId call in heldSetChangedSinceListing with the raw id must turn the extended test red.
中文说明
R3-1:这个新的“离开例外”中的 canonicalizeMsgId 调用——按注释的说法,它们的存在正是为了让该检查不漏掉 resolveHeld 会匹配的带连字符形式——没有被任何测试执行。两个新测试都只使用无连字符的小写 id(abc / abc12345),对这类 id 而言规范化是恒等变换。
MSG_ID_RE 允许连字符与大写字母,且 buildUserFrame 发出带连字符的 randomUUID(),因此带连字符的 id 才是常态。未来若有人把两处 canonicalizeMsgId 调用替换为原始 id,在现有两个测试下仍会全绿(对无连字符小写 id,startsWith 行为完全相同),从而带着漏洞上线:某个对端把 ab-c 与 abc12345 一起留置;ab-c 过期;守卫比较 'abc12345'.startsWith('ab-c') → false → 列表被宣布仍然有效;用户键入已离开消息打印出的无连字符句柄 abc,resolveHeld 前缀匹配到 abc12345——一条与用户审阅过的不同的消息,以被审阅者的句柄被放行,正是该守卫要阻止的回归。
证据:探针(peer-messaging.test.ts)——原样(新增带连字符 id 测试):53/53 通过;变异体(两处 canonicalizeMsgId 调用替换为原始 id):1 失败 | 52 通过,expected false to be true(expect(messaging.heldSetChangedSinceListing()).toBe(true)),且现有两个测试在变异体下仍全绿。
修复:在现有两个测试之一中加入带连字符或混合大小写的离开 id——例如锚定 ab-c 与幸存者 abc12345,让带连字符的一条过期,断言 heldSetChangedSinceListing() 为 true。测试 id 必须通过 MSG_ID_RE(/^[A-Za-z0-9][A-Za-z0-9_-]{0,63}$/,peer-frames.ts:131),这也是让连字符/大写可达的前提。验证:把 heldSetChangedSinceListing 中任一 canonicalizeMsgId 调用替换为原始 id 后,扩展的测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the full production surface at 675bb9f (frames, transitions, inbound gate, wiring, /peers UI). No blockers.
What I checked
refusedvsdeniedsemantics: the new status is terminal, reachable only frompending— a refused receipt cannot followheld(a parked message was admitted, not turned away) nordelivered, so a contradicting peer cannot flip a message the recipient already has into "don't re-send". Parser, description text and transition table are all consistent and tested.- Held-message expiry:
ageOftakes the larger of wall-clock and monotonic elapsed — suspend still ages a hold while a backward NTP step becomes a no-op, and the re-armed delay is clamped to [1ms, 2^31-1] so neither a same-tick recursion nor Node's 1ms clamp busy-loop can occur. One shared timer armed for the earliest deadline (scanned, not read from the head, since a failed release re-parks at its original position), every entry point sweeps before reading, settings changes sweep + re-arm, and a failed release deliberately does not restart the clock. - The
heldSetChangedSinceListingrework: departures are harmless for prefix resolution, with the one real exception handled — a peer-chosen id that is a canonicalized prefix of a live id (abcbesideabc12345) must still bounce, or the reviewed handle would silently resolve to a different message after the shorter one expires. - Wiring:
crossSessionHeldExpiry(1m/5m/10m/never, default 5m) is parsed fail-closed to the default on typos, wired in startInteractiveUI with a test that catches an accidental property drop, and AppContainer re-evaluates keyed on the parsed lifetime + policy rather than any settings edit (so an unrelated key edit can't settle a backlog as denied). /peersshows the remaining time computed the same way the gate ages it, and bulk-decide reports swept-expired ids asgonewith their own explanation instead of a bare "Released 0".
CI note: Test ubuntu and web-shell E2E Smoke were both job-timeout cancellations ('The operation was canceled', no test failures before the cut) — same cap pattern as other recent PRs; this PR touches no web-shell code. Lint & Static and the no-AK integration lane are green.
LGTM, approving.
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head 675bb9f0.
- History: no open Criticals at this head. The one unresolved thread (R3-1) is a test-thinness Suggestion — the dashed-form path through the departure guard's canonicalization behaves correctly as implemented (both sides canonicalized, mirroring
resolveHeld); it just lacks a dashed-id case. Worth taking ci-bot's offered one-line test extension, but per the repo's own rule it is a Suggestion, not a blocker. The earlier stage-3 correctness bug was fixed ine0b9f6cdand re-verified here. - Independent pass found no new Criticals. Checked at this head:
refusedis reachable only from pending (its receipt-transition set is empty) while a parked message settled under a switched-to-refuse policy correctly reportsdenied— someone chose;ageOftaking max(wall, monotonic) closes the backward-NTP stretch, the suspend-blindness, and setTimeout's 32-bit clamp in one reading, and the/peerscountdown ages identically so it never promises time the gate won't grant; the single unref'd earliest-deadline timer is cleared on shutdown, clamped to ≥1 ms to avoid same-tick recursion, scanned rather than trusting buffer order, and every entry point sweeps so a starved or slept-through timer degrades to sweep-on-next-touch rather than an undead hold; a failed release re-parks at the old timestamp and the re-sort is age-ordered, keeping "evict oldest" true under MAX_HELD pressure; the schema options are pinned againstHELD_EXPIRY_OPTIONSso the three vocab copies cannot drift silently; and theheldSetChangedSinceListingrelaxation is sound —resolveHeldprefix-matches over the current set only, so any shrink genuinely cannot flip a uniquely-named handle, and the extension case is exactly guarded. - yiliang114's approval at this head reviewed the same production surface; no contradictions with my pass.
- CI facts: 17 pass / 26 skip, and the two reds (
Test (ubuntu)killed at its 2 h cap on pool host hk3-10 after 1003 test files passed 28,125 tests with the knownvitest-worker onTaskUpdatetransport flake at the tail;web-shell E2E Smokekilled at its 20 m cap) are cancellations on a congested host, not assertion failures — per the channel convention the call is on the review itself.
|
Post-merge review of head Verified against the code at this head
Executed: core ipc 309/309 · cli peerMessaging + peers-command 100/100 · AppContainer + startInteractiveUI + settingsSchema 246/246. Three mutation probes, all killed: allowing Cross-check: the two earlier ci-bot blockers are both closed at this head — the Not covered: live two-process IPC E2E on real terminals; Windows/macOS behaviour (linux only). No blockers found in what merged. Reviewed with AI assistance. |
What this PR does
A
send_messagecall returns as soon as the frame is handed over; what became of it arrives later as a receipt. Two of those receipts were wrong or missing, and this PR fixes both.A refusal reported itself as a decision. A session whose
agents.crossSessionInboundisrefuseturns every peer message away at admission — nobody sees it — but the sender was tolddenied, which means a person reviewed the message and said no. The sending model cannot tell those apart, and they call for opposite behaviour: a decision may be worth raising with that person, a policy refusal means stop. There is now arefusedstatus whose description says the session does not accept messages from other sessions and that re-sending is pointless. A message that was already parked when the user switches the setting torefusestill settles asdenied— someone chose, just after the fact — sorefusedis reachable only frompending, never fromheld.A hold had no end. It waited for a review that might never come, because the user may not be at that terminal, and the sender had no way to distinguish "still waiting" from "never coming"; the only thing that ever settled a hold was the session exiting. A parked message now expires after
agents.crossSessionHeldExpiry—1m,5m,10m, ornever, five minutes by default — and the sender is told nobody answered./peersshows how much time each held message has left, because a review screen that hides its own deadline invites decisions that arrive after the sender has stopped listening.The gate arms one unref'd timer for whichever message expires first rather than one per message, and re-arms it after every change to the buffer. It also sweeps overdue entries at each entry point instead of trusting the timer alone: a laptop that suspends for an hour must not wake up and deliver a message from before it slept. Expiry is judged against the lifetime configured now, so shortening the setting settles a backlog that is already too old and lengthening it extends what is still waiting — the reading under which what
/peersshows as remaining is what actually happens. An unset or unrecognized setting value falls back to the default rather than tonever: failing closed here means bounding how long a sender waits, not extending it indefinitely on a typo.Why it's needed
Both gaps leave the sending session's model reasoning about a message it can no longer learn anything about. Silence and
deniedare the two worst answers a sender can get — one is indistinguishable from "delivered and ignored", the other invites a follow-up aimed at a person who never saw anything. A bounded hold and an honest refusal are what make the receipt channel worth having: after this, every message a session sends reaches a terminal state within a known window, and the state says what actually happened to it.Reviewer Test Plan
How to verify
Unit level:
cd packages/core && npx vitest run src/ipc(302 tests; new cases cover the refusal receipt and its distinctness from a decision, the receipt state machine refusingheld → refused, expiry with its timer, per-message deadlines, expiry on arrival when the timer never fired, a shortened and a lengthened lifetime applied to a waiting backlog,never, a failed release not restarting the clock, and the setting parser).cd packages/cli && npx vitest run src/peerMessaging src/ui/commands/peers-command.test.ts src/ui/startInteractiveUI.test.tsx(105 tests, including the/peersremaining-time wording and the end-to-end expiry and refusal over a real socket).npx tsc --noEmitis clean in both packages, as areeslintandprettier --checkon the changed files.End to end, with
{ "agents": { "crossSessionMessaging": true } }on both sides:crossSessionInbound: "refuse". Send it a message: the sender gets arefusedreceipt saying the session does not accept messages from other sessions, and the receiver shows and holds nothing.crossSessionInbound: "hold"andcrossSessionHeldExpiry: "1m". Send it a message: the receiver holds it and/peersshows the time left; after a minute with no decision the held list is empty and the sender has anexpiredreceipt.crossSessionHeldExpiry: "never":/peersshows no deadline and the message keeps waiting.Evidence (Before & After)
Before: a refused message came back as
denied, indistinguishable from a person declining it. A held message that nobody reviewed produced no further receipt at all until the receiving session exited.After — Linux, bundled build of this branch. The sender here is a small script that binds a socket, sends one frame, and prints the receipts that come back over the same wire a real session uses. Step 3 is covered by unit test rather than live, since it asserts an absence over a long window.
1. Refusal receipt
The receiving session printed nothing and held nothing, which is the point — the receipt says so rather than claiming a person declined.
2. Hold expiry with crossSessionHeldExpiry: "1m"
Sender:
Receiver, in between:
Tested on
The inbox is POSIX-only today, so Windows is N/A; macOS shares the code path but was not exercised locally.
Environment (optional)
Bundled build of this branch, run under tmux with a dummy OpenAI-compatible provider.
Risk & Scope
neverrestores the old behaviour. Five minutes is a judgement call: long enough for someone at the keyboard to notice/peers, short enough that a sender is not blocked for a whole session.neverwas verified by unit test rather than live; headless and ACP sessions bind no inbox today, so neither receipt reaches them; no UI beyond the/peersline tells the receiving user that something expired.denied, which an older parser accepted and rendered; it now arrives asrefused, which that parser rejects, so the sender learns nothing and its ledger stayspendinguntil eviction. Accepted deliberately — emittingdeniedfor a policy refusal is the conflation this change removes, and the exposure is bounded to two sessions on one machine at different versions, ending on upgrade. Rationale is recorded in the design doc. Existing settings keep working; the new setting is additive with a default.Linked Issues
Follows #10764. Independent of #10800, which touches the same two files in different places.
中文说明
这个 PR 做了什么
send_message在帧交出去的那一刻就返回;它最终的下场稍后以回执的形式传回。其中两种回执要么是错的,要么根本不存在,本 PR 把两者都修好。拒收把自己报成了决定。 一个把
agents.crossSessionInbound设为refuse的会话,会在准入阶段把所有对端消息挡回去——没有任何人看到过它——但发送方收到的是denied,而这个词的含义是"有人审阅过并说不"。发送方的模型分不出这两者,而它们要求的行为恰好相反:一个决定也许值得再去找那个人谈,而策略性拒收意味着到此为止。现在有了refused状态,其说明会告诉发送方该会话不接收其他会话的消息、重发没有意义。若消息已经被留置,此时用户才把设置改成refuse,它仍然结算为denied——毕竟有人做了选择,只是事后做的——所以refused只能从 pending 到达,永远不会从 held 到达。留置没有尽头。 它在等一次可能永远不会到来的审阅,因为用户未必在那个终端前;而发送方无从区分"还在等"和"永远等不到"——过去唯一能结算一次留置的,只有会话退出。现在被留置的消息会在
agents.crossSessionHeldExpiry之后过期——1m、5m、10m或never,默认五分钟——并告知发送方无人应答。/peers会显示每条消息还剩多少时间,因为一个把自己的截止时间藏起来的审阅界面,只会诱使用户做出在发送方早已不再倾听之后才送达的决定。闸门只为"最先到期的那条"武装一个 unref 定时器,而不是每条一个,并在缓冲区每次变化后重新武装。它还在每个入口处清扫已过期的条目,而不是只信任定时器:一台挂起一小时的笔记本,醒来后不应该再投递挂起之前的消息。过期以当前配置的时长判定,因此缩短设置会结算掉那些已经太老的积压,延长设置则会给仍在等待的消息更长时间——这正是"
/peers显示的剩余时间就是实际会发生的事"这一读法。未设置或无法识别的取值回落到默认值而非never:这里的 fail-closed 意味着限制发送方等待的时长,而不是因为一个拼写错误就把它无限延长。为什么需要
这两个缺口都让发送会话的模型面对一条它再也无法获知任何信息的消息。沉默和
denied是发送方能得到的两个最糟糕的答案——前者与"已送达但被忽略"无法区分,后者会引出一条指向某个其实什么都没看到的人的追问。有边界的留置和诚实的拒收,才让回执这条通道值得存在:在此之后,会话发出的每条消息都会在一个已知的窗口内到达终态,而这个终态说明的正是它实际的遭遇。评审验证计划
如何验证
单元层面:
cd packages/core && npx vitest run src/ipc(302 个用例;新增用例覆盖拒收回执及其与决定的区别、回执状态机拒绝held → refused、带定时器的过期、每条消息各自的截止时间、定时器未触发时在到达处清扫、把缩短与延长后的时长应用到等待中的积压、never、投递失败不重置计时,以及设置解析器)。cd packages/cli && npx vitest run src/peerMessaging src/ui/commands/peers-command.test.ts src/ui/startInteractiveUI.test.tsx(105 个用例,含/peers剩余时间措辞,以及经由真实 socket 的端到端过期与拒收)。两个包的npx tsc --noEmit均通过,改动文件的eslint与prettier --check亦通过。端到端,双方均配置
{ "agents": { "crossSessionMessaging": true } }:crossSessionInbound: "refuse"。向它发一条消息:发送方收到refused回执,说明该会话不接收其他会话的消息;接收方既不显示也不留置任何东西。crossSessionInbound: "hold"与crossSessionHeldExpiry: "1m"。向它发一条消息:接收方留置它,/peers显示剩余时间;一分钟后仍无决定,留置列表为空,发送方拿到expired回执。crossSessionHeldExpiry: "never":/peers不显示截止时间,消息继续等待。证据(前后对比)
之前:被拒收的消息回来的是
denied,与某个人主动拒绝无法区分。无人审阅的留置消息在接收会话退出之前,根本不会再产生任何回执。之后——Linux,本分支的打包构建。这里的发送方是一个小脚本:绑定一个 socket、发一帧、打印回来的回执,走的是与真实会话相同的线协议。步骤 3 由单元测试而非实机覆盖,因为它断言的是一段长窗口内的"什么都没发生"。英文部分的两个折叠块是实机记录:拒收回执,以及一分钟过期的完整过程(含
/peers中的剩余时间显示)。测试平台
见英文表格:Linux 已测;inbox 目前仅支持 POSIX,因此 Windows 为 N/A;macOS 走同一条代码路径但本地未实测。
运行环境(可选)
本分支的打包构建,在 tmux 下配合一个 dummy 的 OpenAI 兼容 provider 运行。
风险与范围
never可恢复旧行为。五分钟是一个判断:足够坐在键盘前的人注意到/peers,又短到不至于让发送方被阻塞整整一个会话。never由单元测试而非实机验证;headless 与 ACP 会话目前不绑定 inbox,两种回执都到不了它们;除了/peers那一行之外,没有其他界面告诉接收方用户有东西过期了。refused的旧发送方会把该回执当作无法解析而丢弃,从而什么也学不到,这与它此前的处境相同。既有设置照常工作;新设置是附加的,并带默认值。关联 Issue
接续 #10764。与 #10800 相互独立——两者改到同样的两个文件,但位置不同。