feat(mesh): derive thread status from every run's close obligation - #11230
feat(mesh): derive thread status from every run's close obligation#11230yiliang114 wants to merge 1 commit into
Conversation
Several agents work one thread. If each stamped the status when its own run ended, the last to finish would decide: an agent reviewing its part would hide another still working, and a blocker raised by one would be erased by another's clean exit. No run writes the status now. Each leaves a durable close obligation and the status is recomputed from the ones outstanding, so no ordering of concurrent completions can leave a stale state behind. Three rules exist because a review found each missing, and each failure left a thread nobody could clear. A same-thread wait is discharged by any later close, so 'A waits for B, B reviews without @-ing A' reports in_review rather than blocked. Any later successful booking discharges an earlier failure or unclosed return, so one launch failure no longer pins the thread after another agent did the work. A quiescent thread whose last admission booked nothing becomes blocked instead of sitting in in_progress with no live run and no explanation. Whether a human reply should discharge a blocker raised by an agent it did not address is 9.11 and stays open; until it is decided the selector discharges everything, and narrowing it is a change to that predicate rather than to its callers.
|
Thanks for the PR! Template looks good ✓ — every required heading is present and filled in with real substance. One small gap: the template's Problem: this is a Direction: aligned. Multi-agent collaboration is already in the codebase ( Size: touches Approach: the scope feels right. It is a pure function with its one I/O dependency (
Risk: no high-risk-path matches — none of the revert-correlated files are touched. The real risk is verification: CI does not run on this PR at all. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必需标题都在，内容也是实打实填写的。一个小缺口：模板里的 问题： 这是一个 方向： 对齐。多智能体协作在代码库里已经存在（ 规模： 触及 方案： 范围合理。它是一个纯函数，唯一的 I/O 依赖（
风险： 没有命中高风险路径——与回滚相关的文件一个都没碰。真正的风险在于验证：这个 PR 完全没有跑 CI。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewIndependent proposal first (written from the title and "Why it's needed", before reading the diff): I'd model each finished run as leaving one typed obligation, then resolve with a single precedence ladder over the obligations still outstanding — blocked-class first, then a That last point is where the diff and my proposal diverge, and it is the finding below. Otherwise the approach matches what I'd have written — pure function, I/O injected by the caller, obligations as durable records rather than status stamping. Critical — the admission-booked-nothing branch outranks
Now look at what a closing post's
Either way
This is the same failure shape the fileoverview says round two fixed in I1 — "blocked-class outranks review, so the thread reported Why no test catches it: the fixture's default Honest limit on this finding: Two directions, not prescriptive: move the Non-blocking:
Testing evidence — what this PR's CI actually producedNothing. There are no
So there is no independent verification of the 272 production lines or 287 test lines here. Not verified: that The plan-doc line "Observed locally: Worth flagging beyond this PR: the same gap covers the whole stacked chain. #11229 and #11225 target the same base, so none of them get CI either, and the suite presumably only runs once the foundation branch is finally PR'd into Sandboxed verification would settle this: Real-scenario tmux testing: N/A — unattended CI run; on this path the live-behaviour signal comes from the lane named above, not from driving the product here. 中文说明审查方法： 先只看标题和「Why it's needed」，在读 diff 之前写下我自己的方案——把每个结束的 run 建模为留下一条带类型的义务，然后用一条优先级阶梯在仍未清偿的义务上做判定：阻塞类优先，其次是 最后这一点正是 diff 与我的方案分歧之处，也就是下面的问题。除此之外整体思路与我会写的一致——纯函数、I/O 由调用方注入、义务作为持久记录而非状态盖章。 严重问题——「未派发工作」分支压过了
再看收尾消息的
两种情况下
这与文件头注释声称第二轮已修复的 I1 是同一种失效形态——「阻塞类压过 review，于是 thread 在已经可以交给人时上报 为什么没有测试发现： 夹具里默认的 对本结论的诚实限定： 两个方向，不做硬性规定：把 非阻塞：
测试证据——本 PR 的 CI 实际产出了什么： 什么都没有。 因此这 272 行生产代码和 287 行测试代码没有任何独立验证。未验证： 计划文档里那句「Observed locally: 值得在本 PR 之外提一句：同样的缺口覆盖整条堆叠链。#11229 和 #11225 的 base 相同，所以它们同样拿不到 CI，而测试套件大概要等基础分支最终向 沙箱验证可以定论： 真实场景 tmux 测试： N/A——本次为无人值守 CI 运行；该路径下的实时行为信号来自上面点名的通道，而不是在这里驱动产品。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 2/5 — the design premise is right and the module is well built, but the precedence ladder it exists to establish has a rung in the wrong place, and nothing on this PR would have caught it. Going back to my independent proposal: I wrote down, before reading the diff, that the "last post booked nothing" signal belongs at the bottom of the ladder, because a closing post is the one post that almost never books work. The diff puts it above The rest of it I'd genuinely thank the author for in six months. Obligations as durable per-run records instead of last-writer-wins status is the correct model for several agents on one thread, keeping the resolver pure and injecting What tips this to request-changes rather than a comment is the combination, not the defect alone:
That second point outlives this PR and is the thing I'd most want a maintainer's eyes on: #11225 and #11229 sit on the same base, so the whole stacked chain lands core work with no automated gate until the foundation branch is finally PR'd into On the checks I owe this stage: the problem is real rather than theoretical (three concrete stuck states from a prior review round, each with a named failure), the direction is squarely inside the codebase's existing multi-agent work and a committed plan with three steps already merged, and the scope is minimal for what it does — the only excess is the identity ternary at One caveat I want stated plainly rather than buried: Not approving. Requesting changes on the ordering, with the two non-blocking notes above left to the author's judgement. The author holds admin on this repo and is therefore the accountable maintainer for this core change; no other human reviewer is on the PR and no area label resolves an owner, so @yiliang114 is the right person to make the call. 中文说明信心：2/5 —— 设计前提是对的，模块也做得扎实，但它之所以存在的那条优先级阶梯有一级放错了位置，而且这个 PR 上没有任何东西能发现它。 回到我的独立方案：在读 diff 之前我就写下，「最后一条消息没有派发任何工作」这个信号应该放在阶梯最底部，因为收尾消息恰恰是最不可能派发工作的那一条。而 diff 把它放在了 其余部分我六个月后会真心感谢作者。把义务建模为每个 run 的持久记录、而不是让最后写入者决定状态，对「多个 agent 共用一个 thread」来说是正确的模型；保持 resolver 为纯函数、由调用方注入 把它推向 request-changes 而非仅留评论的，是组合因素，而不只是缺陷本身：
第二点的影响超出本 PR，也是我最希望维护者关注的事情：#11225 和 #11229 在同一个 base 上，所以整条堆叠链都在没有自动化门禁的情况下落地核心代码，直到基础分支最终向 关于本阶段我该交代的检查：问题是真实的而非理论性的（来自上一轮 review 的三个具体卡死状态，每个都有具名失效）；方向完全在代码库已有的多智能体工作范围内，并且有已提交的计划、三个步骤已合入；范围对它要做的事来说是最小的——唯一的多余是 有一点我要明说而不是埋起来： 不予批准。就排序问题提交 request changes，上面两条非阻塞建议留给作者自行判断。作者在本仓库持有 admin 权限，因此是这项核心改动的责任维护者；本 PR 上没有其他人类 reviewer，也没有 area 标签能解析出负责人，所以 @yiliang114 是做出这个判断的合适人选。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 Needs some rethinking before this lands — full notes in the Stage 2 and Stage 3 comments above. 🙏
The one blocking issue: in resolveThreadStatus, the admissionBookedNothing(lastMessage) branch (thread-status.ts:208) sits above the review branch (:218) and above the live-child waiting branch (:227). An agent's own closing post almost never books work — decideDispatch refuses a self-trigger by design (dispatch-policy.ts:114-115), and postMessageInTransaction records no_target when nothing resolves (thread-actions.ts:208) — so its outcomes are non-empty with no dispatch/coalesce and admissionBookedNothing is true. The result: an agent posting a review summary with closeKind: 'review' reports the thread as blocked instead of in_review, and a waiting close with a live child reports blocked instead of in_progress. That is the same outcome round two fixed in I1, arriving by a different route.
The suite doesn't reach it: the default fixture's last message is a dispatch, so every in_review/waiting case resolves against a post that did book work, and the I6 case sets runs: [] so it can't meet a review obligation.
Compounding it, no CI runs on this PR at all — ci.yml branch-filters pull_request to main and release/**, and this targets codex/multi-agent-mesh-foundation. So the 272 production lines have no unit, lint or typecheck signal on this head, and the only test claim is the author's local run, which I did not re-execute.
The design premise is right and I don't want this read as a rejection of it — obligations as durable per-run records instead of last-writer-wins is the correct model, and the module is pure with its I/O properly injected. It's specifically the rung ordering, plus a test that would pin it.
Two non-blocking notes are in Stage 2: the nested ternary at :90-97 is a type-level identity (RunCloseKind is a strict subset of CloseObligationKind), and three symbols are exported with no consumers yet.
中文说明
@yiliang114 这个在合入前需要再斟酌一下——完整意见见上方 Stage 2 和 Stage 3 评论。🙏
唯一的阻塞问题：在 resolveThreadStatus 里，admissionBookedNothing(lastMessage) 分支（thread-status.ts:208）位于 review 分支（:218）和有活跃子线程的 waiting 分支（:227）之上。而 agent 自己的收尾消息几乎不可能派发工作——decideDispatch 按设计拒绝自我触发（dispatch-policy.ts:114-115），postMessageInTransaction 在无可解析目标时记录 no_target（thread-actions.ts:208）——所以它的 outcomes 非空却不含 dispatch/coalesce，admissionBookedNothing 为 true。结果是：agent 发出 review 总结并记录 closeKind: 'review' 时，thread 被上报为 blocked 而不是 in_review；waiting 收尾配合活跃子线程时被上报为 blocked 而不是 in_progress。这正是第二轮在 I1 中修掉的结果，只是从另一条路径再次出现。
测试套件覆盖不到：默认夹具的最后一条消息是 dispatch，所以每个 in_review/waiting 用例面对的都是确实派发了工作的消息；而 I6 用例设了 runs: []，因此不可能遇上 review 义务。
雪上加霜的是，这个 PR 完全没有跑 CI——ci.yml 把 pull_request 分支过滤为 main 和 release/**，而本 PR 的 base 是 codex/multi-agent-mesh-foundation。所以这 272 行生产代码在本 head 上没有任何单元、lint 或 typecheck 信号，唯一的测试说法是作者的本地运行结果，我没有重新执行。
设计前提是对的，请不要把这条读成对它的否定——把义务建模为每个 run 的持久记录、而不是让最后写入者决定状态，是正确的模型，模块也是纯函数且 I/O 注入得当。问题具体在于阶梯的排序，以及缺一个能钉住它的测试。
两条非阻塞意见在 Stage 2 里：:90-97 的嵌套三元表达式是类型层面的恒等变换（RunCloseKind 是 CloseObligationKind 的真子集），另有三个符号被导出但目前没有消费方。
— Qwen Code · qwen3.8-max-2026-09-02
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): src/agents/mesh/thread-status.test.ts — no such file or directory; 13 tests passed — this review observed 23651, 1959, 298, 1818, 504, 6361 passed.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const lastMessage = thread.messages[thread.messages.length - 1]; | ||
| if (lastMessage && admissionBookedNothing(lastMessage)) { |
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] [new-surface] This branch sits above the review branch and the live-child waiting branch, so an agent's own closing post reports a thread that is ready for a person as blocked, and both of those later branches become unreachable on the most common closing-post shape there is.
The only message-append path in the tree is postMessageInTransaction, and it always persists at least one outcome. An agent-authored post that names no other agent gets a non-booking skip: self_trigger when the author is the assignee (dispatch-policy.ts:114-115 — an agent's own post never wakes it), or no_target when the thread has no assignee (thread-actions.ts:206-208). So admissionBookedNothing is true for exactly the post a closing tool appends. A run that recorded closeKind: 'review' then resolves to blocked / the last post booked no work (self_trigger) instead of in_review; a run that recorded closeKind: 'waiting' with a live child resolves to blocked instead of in_progress. The waiting half needs no assumption about how step 5b appends a review summary, because thread_wait appends nothing at all — the agent's own earlier post is already the last message.
This is the same failure shape the file overview credits round two with fixing in I1 ("blocked-class outranks review, so the thread reported blocked when it was ready for a person"), arriving by a different route. It also contradicts the aggregate rule the design states for quiescence, which does not list "last admission booked nothing" as a blocked trigger: docs/plans/2026-09-06-multi-agent-board-collaboration.md:481-484 — "an unacknowledged blocker, terminal failure, unclosed run, or wait whose dependency vanished without a bookable parent event takes precedence and yields blocked; otherwise at least one unacknowledged review close yields in_review and emits the parent report". Nothing in the suite reaches it: every in_review and waiting fixture inherits the default messages: [message({ outcomes: booked })], a dispatch, and the I6 test sets runs: [] so it cannot meet a review obligation.
Witness (drove the real committed store and admission path — createThread → postMessage → readThread — then the resolver):
INTACT, assignee-authored closing post:
persisted outcomes: [{"kind":"skip","reason":"self_trigger"}]
resolveThreadStatus: {"status":"blocked",
"reason":"the last post booked no work (self_trigger)",
"outstanding":[{..."kind":"review"}]}
INTACT, no assignee:
persisted outcomes: [{"kind":"skip","reason":"no_target"}]
resolveThreadStatus: {"status":"blocked", ... "outstanding":[{..."kind":"review"}]}
INTACT, waiting obligation + live child:
resolveThreadStatus: {"status":"blocked",
"reason":"the last post booked no work (self_trigger)",
"outstanding":[{..."kind":"waiting"}]}
Sweep over the SkipReason population PARSED from dispatch-policy.ts (8 members):
[obligation=review, expected in_review] suppressed: 8/8
[obligation=waiting, expected in_progress] suppressed: 8/8
(corrected on review to 7 of 8: thread_done is unreachable in that
combination, because the sticky-done return at :159 fires first)
FIXED ARM (this branch moved below review and live-child wait):
[obligation=review] suppressed: 0/8 -> in_review
[obligation=waiting] suppressed: 0/8 -> in_progress
CONTROL I6 (booked nothing, runs: []) -> blocked (unchanged)
Committed suite: Tests 13 passed (13)
The narrower change is to move this whole lastMessage block below the review return and the live-child waiting return, so it fires only when no obligation explains the quiescence — which is the I6 case it exists for ("books nothing and leaves no runnable target"). The alternative, excluding self_trigger/no_target from admissionBookedNothing, loses the "named nobody at all" half of the documented intent.
The existing I6 behaviour must survive: docs/plans/2026-09-06-multi-agent-board-collaboration.md:473 requires "any admission books/delivers nothing and leaves no runnable target | any non-done | persist all outcomes, set blocked, enqueue one deduplicated notification; includes gates, unavailable/unknown assignees, and no_target", which the test blocks a quiescent thread whose last admission booked nothing pins with runs: [] — measured still blocked under the reorder.
Please add a case to thread-status.test.ts with runs: [run({ closeKind: 'review' })] whose last message carries outcomes: [{ kind: 'skip', reason: 'self_trigger' }] asserting status === 'in_review', plus a sibling with closeKind: 'waiting' and hasLiveChildDependency: true under the same post shape asserting 'in_progress'; both are red at this commit, so removing the reorder afterwards must red them again.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if (run.closeKind === undefined) return undefined; | ||
| const kind: CloseObligationKind = |
There was a problem hiding this comment.
[Critical] R1-2: [certifies-falsely] [new-surface] obligationFor derives an obligation from run status only for 'failed', so a terminal cancelled run leaves no obligation at all and a quiescent thread resolves to in_progress / no outstanding close obligation with nothing live and nothing able to run again.
'cancelled' is a committed member of ThreadRunStatus (types.ts:147), validated by the store (mesh-store.ts:179-188) and writable by the committed finishRun (thread-actions.ts:349-353), which writes status/endedAt/error only and never touches closeKind. RunCloseKind (types.ts:150) has no member that could express a cancellation, so no producer can give a cancelled run an obligation — this is structural rather than a question of producer ordering. Trigger: a human posts @alice … (admission books a dispatch, so the booked-nothing branch does not fire), then the run is cancelled — the dispatcher result row cancelled ("mark queued runs cancelled immediately; request runtime cancellation for running work", design :437) and the step-9 thread-view cancel route, which is a per-run human stop distinct from marking the thread done and therefore not covered by the sticky-done return at :159. The board then shows work in flight on a thread with no run, no queue entry and nothing that will ever restart it — the exact state this file's own overview names at :26-29 as "the silent path this design refuses to have". It also starves invariant 13 (design :185), which makes "terminal run failure/cancellation" a state-carrying child transition whose stated consequence is "a waiting parent is always woken or visibly stranded"; with no obligation the aggregate never reaches blocked, so that event never fires.
The rationale for deriving failure from status at :52-53 — "a run that died never reached a closing tool, so it has no closeKind to read" — is equally true of a cancelled run, and the asymmetry is what makes this read as an oversight rather than a decision.
Witness (drove the committed finishRun plus a real store round-trip):
INTACT:
finishRun(cancelled) persisted run: {"status":"cancelled","endedAt":"number"}
<- closeKind absent
last message outcomes (booked work?): [{..."kind":"dispatch","runId":"rn_9f4e..."}]
resolveThreadStatus: {"status":"in_progress",
"reason":"no outstanding close obligation","outstanding":[]}
Direct status probe:
status=cancelled -> in_progress / "no outstanding close obligation"
status=completed (no close) -> in_progress / "no outstanding close obligation"
status=failed -> blocked
Adjacent control (a stranded wait DOES surface):
-> blocked / "run rn_bob is waiting on work that no longer exists"
The completed-without-closeKind half is a separate question and this finding does not claim it: design :476 assigns "record closeKind=unclosed" to the producer for a clean exit, so that state would be a step-5b producer bug rather than a resolver gap. cancelled has no such row — nothing assigns it a close kind, and no RunCloseKind member could carry one.
// in obligationFor, ahead of the closeKind === undefined early return
if (run.status === 'cancelled') {
return { ...base, kind: 'unclosed', ...acknowledged };
}Or widen the existing status branch to run.status === 'failed' || run.status === 'cancelled', or add a distinct 'cancelled' member to CloseObligationKind and BLOCKING_KINDS if a person-stopped thread should read differently to a person from a failure.
Two existing facts must survive: 'cancelling' must stay inside LIVE_RUN_STATUSES (thread-status.ts:42-47) because design :501 says "cancelling serves the same restart-safe purpose for a human stop", and the sticky-done early return at :159 must keep winning because marking done itself cancels that thread's runs (design :477: "set done, cancel queued runs, request running cancellation"), which keeps done sticky against a late post pins.
Please add a case resolving thread({ runs: [run({ status: 'cancelled' })] }) whose last message booked a dispatch, asserting blocked rather than in_progress, and asserting outstandingCloseObligations reports one obligation for that run; both are red at this commit, so deleting the new branch afterwards must red them again.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| export function acknowledgeCloseObligations( | ||
| thread: Thread, | ||
| atSequence: number, | ||
| select: (obligation: CloseObligation) => boolean = () => true, |
There was a problem hiding this comment.
[Critical] R1-3: [certifies-falsely] [new-surface] This default discharges every obligation kind, including blocked and review, on an agent-authored booking — so one agent's unrelated post permanently erases another agent's unanswered question to a person.
Walk it: alice's run closes closeKind: 'blocked' and a blocker notification is enqueued for a person. Bob, unrelated, posts and his @carol mention dispatches. That admission calls acknowledgeCloseObligations(thread, seq) with no selector, stamping closeAcknowledgedAtSequence on alice's blocker. Nothing in the committed tree ever clears that field — a repo-wide grep finds 9 references and zero clearing writers — so the obligation drops out of outstandingCloseObligations for good. The thread reads in_progress, then in_review when carol closes, and alice's question appears in no status, no reason string and no parent report. That is precisely the outcome this file's own overview says the design prevents: "a blocker raised by one would be erased by another's clean exit".
The §9.11 deferral does not cover this case. The PR's Risk section, design :468 and :491, and docs/plans/2026-09-07-mesh-implementation-acceptance.md:93 all scope blocker acknowledgement to a human post; what design :486-487 authorises any later booking to discharge is narrower still — "earlier terminal failure and unclosed records". The PR body states the broad default's own purpose the same way ("any later successful booking discharges an earlier failure or unclosed return"). Agent-authored bookings discharging blockers is therefore not the deferred question, and it is the trigger here — the admission path is author-agnostic, and design :504 charges agent-caused work explicitly. One reviewer also noted thread text is an acknowledged prompt-injection surface (types.ts:206-209), so an injected instruction that makes any agent post one @mention erases a human-facing blocker.
Witness:
INTACT (default select = () => true):
before: {"status":"blocked",
"reason":"run rn_alice asked a question and is waiting for a person",
"outstanding":[{..."kind":"blocked"}]}
after an unrelated AGENT booking:
{"acknowledged":[5],"outstanding":[],
"resolved":{"status":"in_progress",
"reason":"no outstanding close obligation","outstanding":[]}}
a review obligation is discharged the same way:
{"acknowledged":[6],"resolved":{"status":"in_progress",...}}
FIXED ARM (default narrowed to failure|unclosed, the scope of design :486-487):
{"acknowledged":[null],"outstanding":[{..."kind":"blocked"}],
"resolved":{"status":"blocked",
"reason":"run rn_alice asked a question and is waiting for a person"}}
review case -> {"status":"in_review","reason":"run rn_alice submitted a summary for review"}
Committed suite: Tests 13 passed (13)
| select: (obligation: CloseObligation) => boolean = () => true, | |
| select: (obligation: CloseObligation) => boolean = (obligation) => | |
| obligation.kind === 'failure' || obligation.kind === 'unclosed', |
Keep a discharge-everything predicate reachable for the human-feedback caller as an explicitly named selector (e.g. HUMAN_FEEDBACK_DISCHARGE), with the §9.11 note attached to that name rather than to the default, and correct the docstring's scope claim at :245-248 in the same edit.
Three existing facts must survive: docs/plans/2026-09-07-mesh-implementation-acceptance.md:93 — "acknowledgement of every open blocker on a human post that books" — must stay reachable via an explicit selector; design :491 — "Human blocker acknowledgement remains target-scoped product work in §9.11" — means the narrowing must not invent a target-scoping rule; and if (outstanding.size === 0) return thread; at :263 keeps its identity return, pinned by expect(none).toBe(partial).
Please extend thread-status.test.ts with runs [{ id: 'rn_alice', closeKind: 'blocked' }, { id: 'rn_bob', status: 'failed' }] calling acknowledgeCloseObligations(thread, 3) with no selector, asserting the failure is acknowledged while closeAcknowledgedAtSequence stays undefined on rn_alice and resolveThreadStatus still returns blocked naming rn_alice; it is red at this commit, so removing the narrowed default afterwards must red it again. The existing I2 test (does not pin the thread to a failure that later work superseded) calls the function with no selector and must stay green — measured green under the narrowed default.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| export const LIVE_RUN_STATUSES = new Set([ | ||
| 'queued', |
There was a problem hiding this comment.
[Suggestion] R1-6: This set has no type argument, so it infers Set<string> and the liveness gate that every branch of the resolver passes through has no compile-time tie to ThreadRunStatus. Every sibling status set in this area is typed — BLOCKING_KINDS ten lines below (new Set<CloseObligationKind>([), mesh-store.ts:179 (RUN_STATUSES = new Set<ThreadRunStatus>([) and mesh-store.ts:197 (CLOSE_KINDS = new Set<RunCloseKind>([) — so this one is the outlier.
The cost is that the check cannot fail when it should. A misspelling ('canceling') or a new live member added to ThreadRunStatus compiles clean, .has(run.status) then returns false for a run that is still executing, the live-run early return at :168 is skipped, and the thread reports blocked or in_review while an agent is mid-turn — the misreport this module exists to prevent, with no compiler and no test to catch it. Being exported and mutable also lets any importer .add()/.delete() and change classification process-wide.
Witness (two arms, the repository's own tsc):
ARM1 (as committed) — probe contains LIVE_RUN_STATUSES.has('canceling')
and LIVE_RUN_STATUSES.has(<arbitrary string>):
npx tsc --noEmit --strict -> exit 0, no output
(a typed control's `// @ts-expect-error` WAS consumed, so the directive was live)
ARM2 (fix applied, new Set<ThreadRunStatus>([...])):
error TS2345: Argument of type '"canceling"' is not assignable to
parameter of type 'ThreadRunStatus'.
error TS2345: Argument of type 'string' is not assignable to
parameter of type 'ThreadRunStatus'.
import type { ThreadRunStatus /* … */ } from './types.js';
export const LIVE_RUN_STATUSES: ReadonlySet<ThreadRunStatus> =
new Set<ThreadRunStatus>(['queued', 'running', 'finishing', 'cancelling']);Membership must stay exactly those four: types.ts:141-148 defines the seven-member union, and 'cancelling' must remain inside because thread-actions.ts:366 treats it as a still-finishable state.
Worth knowing but deliberately not part of this finding: this is the fourth independent spelling of the same four-status enumeration in the package, alongside mesh-store.ts:904-907 (trimThread), mesh-store.ts:1194-1197 (deleteThread) and thread-actions.ts:363-366 (finishRun). Consolidating those three onto this constant is real work but edits files this PR does not touch, so it belongs in a follow-up. Do not fold dispatch-policy.ts:142 into any shared predicate — it deliberately coalesces on the narrower (run.status === 'queued' || run.status === 'running'), and widening it would let a post join a run that is already closing.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * `failure` is derived from the run status rather than a close kind: a run that | ||
| * died never reached a closing tool, so it has no `closeKind` to read. |
There was a problem hiding this comment.
[Suggestion] R1-11: This rationale says a failed run "has no closeKind to read", but the code 30 lines below explicitly overrides a recorded close kind and a committed test pins that behaviour.
:84-88 reads "// A failed run outranks whatever it managed to record first" and returns kind: 'failure' regardless of any recorded closeKind; thread-status.test.ts:238-247 (reports a failed run as a failure even when it recorded a close kind) uses run({ status: 'failed', closeKind: 'review' }); and isValidRun (mesh-store.ts:273-292) checks RUN_STATUSES.has(value['status']) and value['closeKind'] === undefined || CLOSE_KINDS.has(...) independently, with no cross-field exclusion, so failed + review persists. A maintainer trusting this comment concludes the if (run.status === 'failed') override is redundant, removes it, and a failed run that recorded review reports in_review instead of blocked.
One correction to the cost as originally filed: that regression would not be silent — the suite pins it twice — so what remains is a misleading rationale comment rather than an unguarded behaviour.
Witness (mutation: the if (run.status === 'failed') branch deleted, suite re-run):
FAIL > resolveThreadStatus > does not pin the thread to a failure that later work superseded
AssertionError: expected 'in_progress' to be 'blocked'
FAIL > close obligations > reports a failed run as a failure even when it recorded a close kind
Tests 2 failed | 11 passed (13)
| * `failure` is derived from the run status rather than a close kind: a run that | |
| * died never reached a closing tool, so it has no `closeKind` to read. | |
| * `failure` is derived from the run status, not `closeKind`: a failed run may | |
| * still have recorded a close kind, but the failure must supersede it. |
The override itself must not change — it is pinned by thread-status.test.ts:238-247 and by the I2 test, which together measured 2 failures when the branch was deleted.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| status: ThreadStatus; | ||
| /** Why, in a form a UI can show beside the status. */ | ||
| reason: string; | ||
| outstanding: CloseObligation[]; |
There was a problem hiding this comment.
[Suggestion] R1-27: the outstanding array on a ThreadStatusResolution is never asserted. The tests read only .status and .reason off every resolveThreadStatus result, and exercise the obligation list through the standalone outstandingCloseObligations helper instead (thread-status.test.ts:239, :251).
So blanking outstanding at any one of the eight return sites — or at all of them — ships green, and the payload a caller reads to list what a person must answer is unpinned while status and reason are not. The concrete cost is a green suite over a resolver that reports blocked and hands back an empty list of why.
Witness (baseline Tests 13 passed (13), mutation applied, suite re-run, reverted):
M4b all 8 return sites -> "outstanding: []," (blanked sites: 8)
Tests 13 passed (13) SURVIVES
// in the existing `lets a blocker outrank a review from another agent` case
expect(result.outstanding).toEqual([
{ runId: 'rn_alice', agentId: 'ag_alice', kind: 'review' },
{ runId: 'rn_bob', agentId: 'ag_bob', kind: 'blocked' },
]);Blanking outstanding at the blocking return site must turn that assertion red. One or two existing cases are enough — the point is that at least one return site per shape is pinned, not that all eight are.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| * A post with no outcomes at all is not an admission — a system audit append on | ||
| * a `done` thread, say — and says nothing about whether the thread is stuck. |
There was a problem hiding this comment.
[Suggestion] R1-23: This comment's only example is wrong twice over, and it is the example a maintainer would use to validate the outcomes.length > 0 conjunct.
First, a done thread never reaches the guard: resolveThreadStatus returns at :159-165 before the lastMessage check at :207-208, and admissionBookedNothing has exactly one call site (:207), so there is no other path that could consult it. Second, a late post on a done thread is not outcome-less at all — dispatch-policy.ts:110 returns { kind: 'skip', reason: 'thread_done' } for thread.status === 'done', thread-actions.ts:291 persists it, and this diff's own test builds that shape at thread-status.test.ts:213-228. The predicate returns true for it, so the conjunct the comment cites does not exclude its own example; the early done return does.
The shape the guard actually exists for is a future system-authored append on a thread that is not done — and no committed writer produces one either (thread-actions.ts:187 initialises outcomes empty but every path pushes at least one, at :191, :208 and :291, and :200 is the only message-append site in the mesh). Being exported does not rescue the comment: it names a specific reachable scenario to justify a specific conjunct rather than describing the predicate's semantics for a direct caller.
Witness:
done + last message outcomes=[] -> {"status":"done","reason":"a person marked this thread done"}
done + last message outcomes=[skip thread_done] -> {"status":"done","reason":"a person marked this thread done"}
admissionBookedNothing(skip thread_done) = true
in_progress + last message outcomes=[] -> {"status":"in_progress","reason":"no outstanding close obligation"}
call-site sweep: admissionBookedNothing -> exactly one call site, thread-status.ts:207
| * A post with no outcomes at all is not an admission — a system audit append on | |
| * a `done` thread, say — and says nothing about whether the thread is stuck. | |
| * A post with no outcomes at all is not an admission — a system-authored append | |
| * on a thread that is not `done`, say — and says nothing about whether it is stuck. |
The outcomes.length > 0 conjunct itself is load-bearing and must not be dropped: it is what distinguishes a non-admission append from an admission that booked nothing, and the related finding about scanning backwards for the latest admission depends on that distinction.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| BLOCKING_KINDS.has(obligation.kind), | ||
| ); | ||
| if (blocking.length > 0) { | ||
| const first = blocking[0]!; |
There was a problem hiding this comment.
[Suggestion] R1-18: The reported cause is chosen by run-array position rather than by kind, and the failure arm hard-codes "and no successor is runnable" — a condition this resolver never evaluates. So the reason can be contradicted by the outstanding array in the very object it is returned in.
blocking[0]! is the only selection, and blocking filters listCloseObligations, which maps thread.runs in array order. Three consequences, all measured: [unclosed rn_a, failed rn_b] reports "run rn_a ended without a hand-off" and the launch failure is never named; the same obligation set in the opposite array order reports the failure, so the cause is decided purely by position; and [failed rn_a, review rn_b] reports "run rn_a failed and no successor is runnable" while outstanding in the same object carries review:rn_b — a successor did run and is awaiting a person. reason is a user-facing contract rather than internal prose (its own doc at :141 says "Why, in a form a UI can show beside the status"), and the design gives a terminal failure its own person-visible duty (:474 "append system failure, set blocked, enqueue failure notification"; decision 22 lists "run failed after retry" as one of the things that need a person), so a reason that omits the failure is a fidelity defect rather than a wording nitpick. The word "successor" occurs in this file exactly once, inside the string literal at :188 — no such condition is evaluated anywhere, yet design :474/:476 make it spec language the resolver asserts instead of computing.
Witness:
INTACT:
[unclosed rn_a, failed rn_b] -> "run rn_a ended without a hand-off" (failure never named)
[failed rn_b, unclosed rn_a] -> "run rn_b failed and no successor is runnable"
[blocked rn_a, failed rn_b] -> "run rn_alice asked a question and is waiting for a person"
(no unclosed involved -> independent of that finding)
[failed rn_a, review rn_b] -> "run rn_a failed and no successor is runnable"
outstanding: ["failure:rn_a","review:rn_b"]
FIX (severity-ordered pick):
[unclosed rn_a, failed rn_b] -> "run rn_b failed and no successor is runnable"
reversed -> identical (order-independence restored)
Tests 13 passed (13)
const BLOCKING_SEVERITY: readonly CloseObligationKind[] = [
'failure',
'blocked',
'unclosed',
];
const first =
blocking.find((o) => o.kind === BLOCKING_SEVERITY[0]) ??
blocking.find((o) => o.kind === BLOCKING_SEVERITY[1]) ??
blocking[0]!;And either drop the unevaluated "and no successor is runnable" clause from the failure reason, or actually derive it (append it only when no run in thread.runs started after the failed one).
expect(resolve(failed).status).toBe('blocked') at thread-status.test.ts:139 must still hold — this changes which obligation is reported and the wording, not that an unacknowledged failure blocks the thread. Two cautions for whoever writes it: the comment at :84-85 is intra-run ("A failed run outranks whatever it managed to record first", pinned by thread-status.test.ts:238-247) and so does not establish the inter-run precedence a severity pick introduces — that has to come from design :474 and the reason contract at :141; and the [failed, review] scenario needs the failure to postdate the booking, because design :487-489 has the admission path discharge an earlier failure when later work is booked (pinned at thread-status.test.ts:133-152).
Please add two cases: [unclosed, failed] asserting the reason names the failed run's id, and a failed run plus an outstanding review asserting the reason does not claim no successor is runnable while outstanding carries the review. Both are red at this commit, so reverting the severity pick must red them again.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| /** | ||
| * Discharges outstanding close obligations at a message sequence. | ||
| * | ||
| * `select` narrows which ones. Two callers use it today: the close path passes |
There was a problem hiding this comment.
[Suggestion] R1-5: This docstring states as present fact that two callers exist. At this commit there are zero, and the plan line this same commit adds says the opposite.
docs/plans/2026-09-07-mesh-implementation-acceptance.md:59 — added by this diff — reads "The producers that write closeKind are the thread tools in 5b, so nothing calls this resolver yet", and the PR body agrees ("no existing caller"). Two documents in one commit assert opposite facts about the same code. The cost lands on whoever writes 5b: they read "two callers use it today" plus a fully specified selector contract, and either hunt for wiring that does not exist or treat the I1/I2 wiring as already done and skip it — at which point blocked-class outranks review again and round-2 finding I1 returns. This is not a missing comment but a false one, which is why it is worth the edit.
Witness (sweep; oracle = the repository's own contents):
grep 'thread-status' over all of packages/ -> 1 match:
packages/core/src/agents/mesh/thread-status.test.ts:13: } from './thread-status.js';
grep acknowledgeCloseObligations|resolveThreadStatus|outstandingCloseObligations|
listCloseObligations|admissionBookedNothing|LIVE_RUN_STATUSES over the worktree
-> thread-status.ts, thread-status.test.ts, and the two plan docs
Production callers: 0
Reword to the planned state, and say plainly what the default actually discharges (see the separate finding on this selector's scope):
/**
* …
* `select` narrows which ones. Two callers are planned in 5b: the close path
* will pass `waiting` so a later close on the same thread releases a peer's
* wait, and the admission path will pass nothing — which, until §9.11 is
* settled, discharges every outstanding obligation, not only failures and
* unclosed returns.
*/The reworded comment must stay consistent with docs/plans/2026-09-07-mesh-implementation-acceptance.md:59 — "The producers that write closeKind are the thread tools in 5b, so nothing calls this resolver yet."
— qwen3.8-max via Qwen Code /review (v0.23.0)
| Gate: (a) a mutating tool invoked with a model-supplied `threadId` argument is rejected by schema, and one invoked outside a run context is rejected at execution; (b) `thread_create` under thread A from a body whose previous turn was on thread B creates the child under A, tested by running two turns on one `AgentHeadless` instance; (c) `thread_wait` without a live dependency returns a typed rejection; (d) the assembled prompt for a second wake contains title, body, status, the last N posts, and the delta after `committedThroughSequence`, and a retention gap renders the GAP line; (e) a `USAGE_METADATA` sequence across a `finishingInputs` continuation records rounds `[1, 2]` on one run. | ||
| Evidence: the assembled prompt text for cases first-entry / delta / gap / retry, committed as snapshot fixtures. | ||
|
|
||
| **Aggregate status landed.** `thread-status.ts` derives the status from every run's close obligation rather than letting the last run to finish stamp it, and the three round-2 findings are each pinned by a test: a same-thread wait is discharged by a later close (I1), any later successful booking discharges an earlier failure or unclosed return (I2), and a quiescent thread whose last admission booked nothing becomes `blocked` (I6). Also covered: a live run outranks another agent's review, a blocker outranks a review, a wait is `in_progress` only while a child can wake it, `done` is sticky against a late post, and a failed run reports as a failure even when it recorded a close kind. Observed locally: `thread-status.test.ts` → 1 file, 13 tests passed; targeted ESLint clean. The producers that write `closeKind` are the thread tools in 5b, so nothing calls this resolver yet. |
There was a problem hiding this comment.
[Suggestion] R1-22: This added line records round-2 finding I2 as pinned by a test for both halves, but no test in this commit exercises an outstanding unclosed obligation at all — so the artifact that tracks which round-2 findings are closed closes one that is not.
The claim is "any later successful booking discharges an earlier failure or unclosed return (I2)". 'unclosed' occurs exactly once in thread-status.test.ts (at :108) with closeAcknowledgedAtSequence: 1 on the next line, so outstandingCloseObligations filters that run out before any branch reads it; hand-off occurs zero times; and none of the three acknowledgeCloseObligations calls (:141, :161, :275/:284) touches an unclosed obligation. Two mutations that falsify exactly that sentence both leave the cited suite green. The consequence is not just an imprecise sentence: a later reviewer reads this file to decide whether I2 is settled, and it records the unclosed half as closed when nothing pins it.
Witness:
MUTATION A BLOCKING_KINDS minus 'unclosed' -> Tests 13 passed (13)
intact: cause=unclosed status=blocked reason="run rn_x ended without a hand-off"
mutated: cause=unclosed status=in_progress reason="no outstanding close obligation"
MUTATION C default select = kind !== 'unclosed' -> Tests 13 passed (13)
(removes precisely "discharges an earlier ... unclosed return")
intact: stamped => [9] after ack => {"status":"in_progress","outstanding":[]}
mutated: stamped => [null] after ack => {"status":"blocked",
"reason":"run rn_a ended without a hand-off"}
Greps: 'unclosed' -> 1 hit (test :108, with closeAcknowledgedAtSequence: 1 on :109)
'hand-off' -> 0 hits
13 it( blocks, matching this line's own "13 tests passed"
Either narrow the claim to the half that is pinned — "any later successful booking discharges an earlier failure (I2)" — or add the missing case and name it here:
it('discharges an unclosed return when later work books', () => {
const unclosed = thread({ runs: [run({ closeKind: 'unclosed' })] });
expect(resolve(unclosed).status).toBe('blocked');
const discharged = acknowledgeCloseObligations(unclosed, 2);
expect(outstandingCloseObligations(discharged)).toEqual([]);
});If the test is the route taken, assert the discharge rather than unconditional blocking: design docs/plans/2026-09-06-multi-agent-board-collaboration.md:476 makes this row conditional ("record closeKind=unclosed; block only if no successor is runnable"), and a separate finding disputes whether the unconditional block is correct — so a test that pins unconditional blocking here would have to be edited when that is settled.
The underlying coverage gap is reported separately at the four untested resolver dimensions; what is distinct here, in a different file, is the false evidence claim in the acceptance record.
— qwen3.8-max via Qwen Code /review (v0.23.0)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review — head e0cef457, base codex/multi-agent-mesh-foundation (draft)
Reviewed as a stacked increment on codex/multi-agent-mesh-foundation; scope is the 4 changed files at this SHA. Core gate: packages/core/src/agents/**, maintainer-authored, exempt from the two-tier block. Verified by reading thread-status.ts at this commit; no local test run.
What checks out
I enumerated the full run-status × close-kind product (7 statuses × 5 close-kind values, including undefined) against obligationFor and the tail of deriveThreadStatus. Two properties hold:
- Monotonicity — no combination moves a thread backwards from
in_progresstoopen; the tail only readsthread.status === 'open', which is set elsewhere and never regressed here. - Live-run suppression is intentional and correct for
queued/running/cancelling: a run still in flight should not be described by a close obligation.
Critical 1 — the status function is not total: two combinations fall through to a silent default
obligationFor reads liveness first, then failed, then bails on a missing closeKind:
if (LIVE_RUN_STATUSES.has(run.status)) return undefined;
...
if (run.status === 'failed') { return { ...base, kind: 'failure', ...acknowledged }; }
if (run.closeKind === undefined) return undefined;completed with closeKind: undefined and cancelled with closeKind: undefined therefore produce no obligation. With nothing outstanding, the tail returns:
return {
status: thread.status === 'open' ? 'open' : 'in_progress',
reason: 'no outstanding close obligation',
outstanding,
};A thread whose only run ended in either of those two states parks in in_progress with a reason that says there is nothing to wait for — permanently, since no later event revisits it. The cancelled case is reachable today: the sibling change in the stack leaves closeKind: undefined on cancellation by construction.
The fix is a decision about which state a run that ended without declaring a close kind should map to, and then making the function total over the product so a future status can't reopen the hole. Both combinations warrant a test.
Critical 2 — finishing is in LIVE_RUN_STATUSES, which hides a legitimate close from readers
export const LIVE_RUN_STATUSES = new Set([
'queued', 'running', 'finishing', 'cancelling',
]);Because the liveness check precedes the closeKind read, a run in finishing — which by then has a closeKind — contributes nothing. A peer still holding a waiting obligation against it lands in the strandedWait branch and the thread is reported blocked with "waiting on work that no longer exists," while the run is in fact finishing normally. Same root cause as Critical 1: ordering the liveness gate ahead of the close-kind read makes finishing indistinguishable from running.
Reading closeKind before the liveness gate, or removing finishing from the live set, would resolve both this and Critical 1 at one site.
Note on the header comment
:150-151 asserts that "no ordering of concurrent run completions can leave a stale status behind." The cancelled-without-closeKind case contradicts that sentence directly. I am flagging it because the comment is what a future reader will trust instead of re-deriving the table.
Verdict
C=2, both at the same site. Fourteen unresolved threads were already open on this PR; I did not re-adjudicate all of them, and the two findings here are mine from reading the current head, so they may overlap with existing ones. Limitation: no local test run.
4 participants
What this PR does
Adds
mesh/thread-status.ts: thread status derived from every run's close obligation, rather than written by whichever run finished last. A finished run records what it left behind —blocked,review,waiting,unclosed, or afailurederived from its run status — and the resolver recomputes the thread's status from the obligations still outstanding.Independent of #11229 (step 5a); the two child PRs can merge in either order.
Why it's needed
Several agents work one thread. If each could stamp the thread's status when its own run ended, the last one to finish would decide. An agent reviewing its part would hide another agent still working; a blocker raised by one would be erased by another's clean exit. Recomputing from scratch on every write means no ordering of concurrent run completions can leave a stale status behind.
Three rules here exist because the round-two review found each of them missing, and each failure left a thread in a state nobody could clear:
A waits for B; B reviews without @-ing Aleft A's wait looking orphaned, and blocked-class outranks review, so the thread reportedblockedwhen it was ready for a person.blockedforever, even after another agent did the work.blocked. Without it a post whose assignee was disabled, or that named nobody at all, left the thread inin_progresswith no live run and no explanation — the silent path this design refuses to have.A
waitingobligation is conditional rather than blocking: it keeps the threadin_progresswhile a descendant can still wake it, and becomesblockedonce nothing can. A failed run reports as afailurewhatever close kind it managed to record first, because the failure is the thing a person has to see.Reviewer Test Plan
How to verify
cd packages/core npx vitest run src/agents/mesh/thread-status.test.tsConfirm a live run outranks another agent's review; a blocker outranks a review; a wait is
in_progresswith a live child andblockedwithout one;donesurvives a late post; and the three round-two cases above each flip status as described.Evidence (Before & After)
N/A — internal status derivation and tests only.
Tested on
Environment (optional)
Node.js 22 workspace, named Vitest file only. Observed: 1 file / 13 tests passed. Targeted ESLint clean.
Risk & Scope
closeKindyet. The producers arethread_wait/thread_block/thread_reviewin step 5b, andhasLiveChildDependencyis resolved by the caller because reading sibling threads is I/O.Linked Issues
Parent delivery PR: #11206. Sibling child PR: #11229.