feat(mesh): record run closes and derive thread state from them - #11234
feat(mesh): record run closes and derive thread state from them#11234yiliang114 wants to merge 2 commits into
Conversation
Closing is two writes because a closing tool is called mid-turn and cannot mark its own still-executing runtime finished. The tool records what the run is closing as and moves it to finishing, which ends the turn; the runtime callback records the terminal state, and only there is the thread's status recomputed. A crash between the two leaves a finishing run with a closeKind, which is a complete instruction for restart reconciliation — a status written before the runtime actually stopped would be a lie the next reader cannot detect. A wait is refused when nothing could wake it, and the dependency is walked over parentThreadId rather than rootThreadId so a sibling sub-thread does not count as this thread's delegation. Any close discharges peers' waits on the same thread. A clean exit that never called a closing tool is recorded as unclosed rather than as implicit success. finishRun now delegates here so a run has exactly one way to end, and postMessage discharges outstanding obligations when it books work before applying the aggregate status. Without those two producers the resolver's rules had no writer: an obsolete failure kept the thread blocked after later work succeeded, and a post that booked nothing left it in in_progress with no live run and no explanation.
…xports A structured assignment or parent report is system-authored but must keep the run or human action that caused it, so it is charged as unattended work without being suppressed as an ordinary self-authored post. PostMessageInput now carries authorKind, sourceRunId and triggerKind; all three are derived by the server and none is accepted from a model. Removes three exports from run-lifecycle that had no reader: the booking acknowledger duplicated what postMessage already calls directly, and the live run listing and re-exported author id were never read.
|
Thanks for the PR! Template looks good ✓ — every required heading is present and filled in. (The optional A note on what I reviewed: the head moved from Problem: real, and verifiable rather than asserted. I grepped the base branch ( Direction: aligned. The mesh work is tracked in Size: touches core ( Approach: the scope feels right, and the two-write close is well argued — a Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必需标题都在且填写完整。（模板里可选的 关于我审查的对象：本次运行过程中 head 从 问题： 真实存在，而且是可验证的，不是口头断言。我在 base 分支（ 方向： 对齐。mesh 相关工作在 规模： 触及核心路径（ 方案： 范围合理，两次写入的关闭设计论证充分 —— 带着 风险： 无升级风险信号 —— 改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewRead against the base branch ( Before reading the diff I sketched what I'd do from the title and the "Why it's needed" section: a small Blocker: the admission acknowledgement discharges obligation kinds it shouldn't, and a peer's post can silently swallow a question or a review
The
From there, if no other run leaves an outstanding obligation, the thread goes quiescent and falls through to The The fix is small and the mechanism already exists — the close path uses it correctly ( No test covers this:
|
| Check | Conclusion |
|---|---|
| Qwen Code CI — test | not triggered (base is codex/mesh-step-5-status; ci.yml fires on main and release branches) |
| Qwen Code CI — lint_and_static | not triggered (same filter) |
| Qwen Code CI — typecheck | not triggered (same filter) |
| authorize | success |
| label | success |
| assign | success |
| delay-automatic-review | success |
| Remind on force-push | success |
| review-pr | in_progress |
| precheck-pr, resolve-pr, ack-review-request, review-config, publish-resolution | skipped (bot orchestration, not PR CI) |
Sandboxed verification would settle part of this: @qwen-code /verify — that run-lifecycle.test.ts, thread-actions.test.ts, thread-status.test.ts and mesh-store.test.ts actually pass on this head is currently the author's word only, and that claim predates the second commit, since no CI lane runs against a stacked base. You have write access, so /tmux is also available, but I'd not bother here: nothing calls closeRun and no consumer drains the outbox until step 6, so there is no TUI surface for it to drive. Note that /verify would not catch the blocker above either — the suite passes with that behaviour present, so what's missing is a test case, not a run.
中文说明
代码审查
审查基准是 base 分支（codex/mesh-step-5-status），因为 #11230 还没合并，本 PR 的一切都依赖它的 thread-status.ts 和 mesh-store.ts。审查对象是 cbc043995c35，即两个 commit 的净结果。
看 diff 之前，我先根据标题和"Why it's needed"写了自己的方案：一个小的 run-lifecycle.ts 负责关闭与终态写入，finishRun 委托给它，让一个 run 只有一条结束路径，且状态只在终态写入时重算。你实现的正是这个，而且模块边界比我最初的想法更好（把关闭逻辑留在 thread-actions.ts 之外，那个文件管的是准入）。有两处我会做得不一样，写在下面 —— 其中一处我认为是真的 bug。
拦截项：准入侧的确认（ack）把不该释放的义务也释放了，同伴的一条发言可能悄悄吞掉一个提问或一次评审提交。
postMessageInTransaction 现在调用 acknowledgeCloseObligations(next, storedMessage.sequence) 时没有传 select，因此用了默认的 () => true，会释放所有未决义务 —— 包括 I2 真正要求的 failure 和 unclosed，也包括 blocked、review 和 waiting。而且没有按作者身份做任何限制，所以agent 或 system 的发言同样会触发，不只是人类回复。
review 这一种我认为是 bug，而不是待定的设计问题。走一遍：
- Agent A 以
review关闭并完成终态写入，此时 agent B 仍在运行 →resolveThreadStatus返回in_progress（活跃 run 优先于 review）。不发thread_in_review通知，这是对的 —— 你自己的测试leaves a thread in_progress while another run is still live就钉住了这个行为。 - B 发了一条真正排上工作的消息（触发 dispatch 的提及，或 coalesce 进一个排队 run）。新的 ack 触发，给 A 的 run 盖上
closeAcknowledgedAtSequence。 - A 的 review 义务就此被永久释放 ——
outstandingCloseObligations以acknowledgedAtSequence === undefined过滤，resolver 再也不会看到它。
之后如果没有别的 run 留下未决义务，线程进入静默态并落到 thread.status === 'open' ? 'open' : 'in_progress' —— 零个活跃 run 却报 in_progress，毫无解释。这正是 thread-status.ts 文件头注释说本设计拒绝存在的静默路径；而 A 的提交永远到不了人面前：thread_in_review 和 parent_report 只在状态跃迁到 in_review 时入队，而这次跃迁根本不会发生。types.ts 写的是 agent "可以把线程推到 in_review，但不能越过它" —— 有了这个 ack，繁忙线程根本到不了 in_review。
blocked 是同一机制作用在一个面向人类的提问上：A 问"该走哪条重试路径？"并阻塞；仍在工作的 B 发言并给 C 排上工作；A 的 blocker 义务被释放，线程不再报 blocked，尽管从来没有人回答过。这一种确实是 acknowledgeCloseObligations 文档注释里标注为刻意未决的 §9.11 问题 —— 但 §9.11 讨论的是"人类回复是否应该释放一个它并未回应的 blocker"，而这里的默认值意外地回答了一个宽得多的问题（任意作者、任意义务类型），而且是最宽的那种答案。
修法很小，机制也已经有了 —— 关闭路径就用对了（I1 用 kind === 'waiting' && obligation.runId !== run.id）。准入路径需要自己的谓词，例如 failure | unclosed | waiting：既满足 I2，也满足 I1 的"或人类发言"那一支，同时不动 review，并把 blocked 留给 §9.11 去有意识地决定，而不是由默认值决定。注意：如果只收窄到 failure | unclosed，会回退 I1 的人类发言那一支，所以 waiting 要留在谓词里。
没有测试覆盖这一点：clears an obsolete failure when a later post books real work 覆盖的是 failure（正确），没有任何用例覆盖"同伴的 review 或 blocked 义务未决时，一条发言排上了工作"。也就是说，上面这个行为存在时，测试套件依然是绿的。
listThreads() 的结果在新路径上被无条件信任，而准入路径是拒绝信任它的。 postMessageInTransaction 读的是 const { threads, unreadable } = ...，并在 unreadable.length > 0 时抛错 —— 它不在残缺数据上做准入决策。run-lifecycle.ts 里两个新读取点把这一半丢掉了，两处都完全没有提到 unreadable：
closeRunInTransaction的waiting判据只取threads。如果读不出来的正好是那个活跃子线程，hasLiveDescendant返回 false，等待就会被拒绝并告诉模型"没有子线程打开，等待会搁浅" —— 一次读取失败被当成关于工作区的事实汇报给了模型。applyAggregateStatus同样只取threads，并把它直接喂给 resolver。同一个触发条件，结果不同：waiting义务被判成blocked（"在等已不存在的工作"），还会入队并持久化一条thread_blocked通知 —— 一次瞬时的兄弟记录不可读，换来落盘的错误状态。
unreadable 会收录除 MeshSchemaVersionError 之外任何抛错的读取（JSON 损坏、id 不匹配、I/O 错误），所以是可达的。建议两处都接住 unreadable，并像准入那样抛错。
顺带一提：在 postMessageInTransaction 里，事务在几行之前已经列过一次线程了，所以 applyAggregateStatus 在同一把锁下又做了一次完整的目录读取和全量 JSON 解析。你在 Risk & Scope 里把这次读取写成不可避免的；在这条路径上并非如此 —— 把已取到的列表传进去（或加一个可选的 threads 参数）就能省掉。
unclosed 没有覆盖 cancelled，尽管注释这么写。 注释是"未调用关闭工具就停下的 run 记为 unclosed，绝不当成隐式成功"，但代码是 run.closeKind ?? (input.outcome.status === 'completed' ? 'unclosed' : undefined)。failed 没问题 —— obligationFor 会从状态本身推出 failure 义务。cancelled 掉进了缝里：没有 closeKind，状态也不产生义务，于是 obligationFor 返回 undefined，一个唯一 run 被取消的线程会解析成 in_progress，而实际上什么都没在跑。今天是潜在的（还没有人以 cancelled 调 finishRun），但确立这条规则的正是本 PR，而注释夸大了它实际强制的范围。要么对 cancelled 也记 unclosed，要么把注释收窄为仅 completed，并说明取消将在哪一步落地。
postMessage 的新参数仍然没有生产者，而且现在有两条消息构造路径。 第二个 commit 把 acknowledgeAfterBooking、liveRunsFor 以及重复的 SYSTEM_AUTHOR_ID / HUMAN_AUTHOR_ID 再导出从 run-lifecycle.ts 里去掉了 —— 很好，那正是我会提的同一类问题。留下来的是准入这一侧：authorKind、sourceRunId、triggerKind 以及 thread-actions.ts 自己的 SYSTEM_AUTHOR_ID 在 postMessageInTransaction 里被声明并读取，但本 PR 和 base 分支上没有任何调用方设置它们 —— launcher.ts 和 capability.ts 根本不调用 postMessage，而 closeRunInTransaction 用的是自己的本地 appendMessage。有读取点，没有生产者。
appendMessage 重新实现了 postMessageInTransaction 已有的消息构造（id、sequence、authorKind、from、authorNameSnapshot、sourceRunId、triggerKind、text、mentions、outcomes、at）—— 两者现在的差别只在于是否走准入。我不认为答案是让关闭路径走 postMessage：关闭消息绝不能经过准入、提及解析或 auto-turn 计费，否则结束一个 run 反而会排上新工作。所以独立 append 是对的；不对的是那些没人用的参数。AGENTS.md 的 Simplicity First 明确反对添加未被要求的灵活性，而你自己的 Risk & Scope 把唯一可能的生产者（调度器）放在 step 6。建议把这三个参数和 SYSTEM_AUTHOR_ID 挪到真正会设置它们的那个 PR；或者，如果 step 6 确实是这个栈里的下一步，就在这里说明，让读者知道它们是有意的。
一个小的、非拦截的点：waiting 关闭不追加消息，所以 ack 序号 next.nextMessageSequence - 1 指向的是上一条消息 —— 在没有消息的线程上则是 0，而 isValidRun 接受它（isOptionalNonNegativeInteger）。今天没有任何地方对这个值做数值比较，所以无害，但在一个以"记录即证据"为前提的模块里，它把一次释放记在了一条并不存在的消息上。
测试证据
这个 PR 完全没有跑 CI，而且是结构性的，不是失败：ci.yml 的 pull_request 触发只过滤 main 和 release/**，而本 PR 的 base 是 codex/mesh-step-5-status。我在被审查的那个 commit 上通过 API 确认过 —— cbc043995c350463f9a36e892953718c4ebf3e59 上的 4 个 workflow run 中，event == "pull_request" 的有 0 个，该 commit 上全部 11 个 check-run 都是机器人编排（pull_request_target / issue_comment）。没有任何 test、lint_and_static 或 typecheck 结果可引用，无论绿红 —— 我按这些关键词搜过 check-run 名字，什么都没搜到。
按 triage 规则我没有构建或运行本 PR 的任何代码，所以描述里"4 files / 57 tests passed"和"targeted ESLint clean"是作者本人的说法，不是本门禁复跑或验证过的证据 —— 而且这些数字是针对第一个 commit 写的，早于 cbc043995c35 落地。这点要说清楚，因为这个栈在到达 main 之前任何一层都没有 CI —— 这 344 行测试里的一个类型错误或一条失败断言，要等整个栈重新定向 base 之后才会显现。可静态核对的部分我核对了，针对的是被审查 commit 上的 blob：本 PR 触及的每个字段（closeKind、closeAcknowledgedAtSequence、finalMessageId、failureStage、finishing、RunCloseKind 的 unclosed、authorKind: 'system'、sourceRunId、triggerKind）在 base 的 types.ts 里都已存在；acknowledgeCloseObligations 确实接受可选的第三个 select 参数；transaction.listThreads() 和 readAgents() 都是 *Unlocked 辅助函数，所以在事务内调用不会死锁；exactOptionalPropertyTypes 未开启，因此终态写入里显式的 closeKind: undefined 可以通过编译。
未验证：那 57 个测试、ESLint、tsc，以及描述里声称的所有运行时行为 —— 这个 base 没有 CI，且本门禁从不执行 PR 代码。
上面的 CI 表格用机器可读的区域标记包起来了，CI 落定后 finalize 工作流会原地更新它。
沙箱验证可以解决其中一部分：@qwen-code /verify —— run-lifecycle.test.ts、thread-actions.test.ts、thread-status.test.ts、mesh-store.test.ts 在这个 head 上是否真的通过，目前只有作者的说法，而且那个说法早于第二个 commit，因为栈式 base 没有任何 CI 通道会跑。你有写权限，所以 /tmux 也可用，但这里我建议不用：step 6 之前没有任何东西调用 closeRun，也没有消费者消费 outbox，所以没有 TUI 界面可供驱动。要注意 /verify 同样抓不到上面那个拦截项 —— 该行为存在时测试套件依然通过，所以缺的是一个测试用例，不是一次运行。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at cbc043995c350463f9a36e892953718c4ebf3e59 · re-run with @qwen-code /triage
|
Confidence: 2/5 — the architecture is right and unusually well documented, but the admission acknowledgement discharges obligation kinds the design never asked for, and one of them silently loses an agent's review submission. Stepping back: the two-write close is the correct call and I'd have written it the same way. A What stops me approving is that the producer half wired the resolver's most permissive default into a live write path. It's a narrow fix — one predicate — and I've named the exact scenario and the test case that would pin it in the review comment. I'd want that test written first, because the suite is green today with the behaviour present, and step 6 will build on these semantics. Two honest caveats on how much weight to give this. Nothing is broken for a user right now: no tool calls Also worth your attention as the person who'll retarget this stack: no CI runs at any level below Process note: the head moved from Requesting changes on the acknowledgement predicate. The rest — the 中文说明信心度：2/5 —— 架构是对的，文档质量也少见地好，但准入侧的确认释放了设计从未要求的义务类型，其中一种会悄悄丢掉 agent 的评审提交。 退一步看整体：两次写入的关闭是正确的选择，我也会这么写。带着 让我不批的原因是：生产者这一半把 resolver 最宽松的默认值接进了一条真实写入路径。 修法很窄 —— 一个谓词 —— 我在审查评论里点名了具体场景，以及能钉住它的测试用例。我希望那个测试先写，因为该行为存在时套件今天是绿的，而 step 6 会在这些语义之上继续搭建。 关于这个判断该占多大分量，有两点如实说明。第一，现在没有任何用户侧的东西是坏的：没有工具调用 另外有一点请你作为将来重定向这个栈的人留意： 流程说明：本次运行过程中 head 从 我就确认谓词这一点提出修改请求。其余几项 —— — 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 Requesting changes on one thing — the rest of my notes are in the stage comments above.
postMessageInTransaction calls acknowledgeCloseObligations(next, storedMessage.sequence) with no select, so it takes the default () => true and discharges every outstanding obligation on any post that books work, from any author. That's wider than round-2 finding I2 ("an earlier failure or unclosed return") and it produces a silent wrong state: agent A closes review while B is still live (the case your own leaves a thread in_progress while another run is still live test pins), B then posts something that dispatches or coalesces, and A's review obligation is permanently discharged — outstandingCloseObligations filters on acknowledgedAtSequence === undefined, so the resolver never sees it again. If no other run leaves an obligation outstanding, the thread goes quiescent at in_progress with nothing running and no explanation, and thread_in_review / parent_report never fire because the transition into in_review never happens. Same mechanism clears a peer's blocked question to a human — that half overlaps §9.11, which thread-status.ts declares unresolved, so it wants a deliberate decision rather than a default.
Suggested fix: give the admission path its own predicate (failure | unclosed | waiting — keep waiting so I1's "or human post" arm still holds), leave review alone, and add the missing test: a post that books work while a peer's review obligation is outstanding must still resolve to in_review once the thread goes quiescent. The suite is green with the current behaviour, so step 6 would build on it as-is.
Non-blocking, but I'd like them addressed: applyAggregateStatus and the waiting guard ignore unreadable from listThreads() where the admission path throws on it (an unreadable sibling record becomes a refused wait, or a persisted blocked status plus a thread_blocked notification); unclosed is recorded for completed only, so a cancelled run leaves no obligation and the comment overstates the rule; and authorKind / sourceRunId / triggerKind / SYSTEM_AUTHOR_ID have no producer anywhere in this PR or on the base branch, so they belong with the step-6 dispatcher.
Note for the stack: ci.yml only fires on main / release/**, so no CI has run on this head at all — the 57 tests, ESLint and tsc are your local results only. I verified statically that every field this PR touches already exists in the base types.ts, that acknowledgeCloseObligations really takes an optional third select, and that the transaction helpers are *Unlocked (no deadlock), but nothing has executed these changes.
中文说明
就一件事提出修改请求 —— 其余意见都在上面的阶段评论里。
postMessageInTransaction 调用 acknowledgeCloseObligations(next, storedMessage.sequence) 时没有传 select，因此用了默认的 () => true：任何排上工作的发言、无论作者是谁，都会释放所有未决义务。这比 round-2 的 I2（"先前的 failure 或 unclosed 返回"）宽，并且会产生静默的错误状态：agent A 在 B 仍活跃时以 review 关闭（正是你自己 leaves a thread in_progress while another run is still live 测试钉住的场景），随后 B 发了一条触发 dispatch 或 coalesce 的消息，A 的 review 义务就被永久释放 —— outstandingCloseObligations 以 acknowledgedAtSequence === undefined 过滤，resolver 再也不会看到它。如果没有别的 run 留下未决义务，线程就会静默停在 in_progress：什么都没在跑，也没有解释，而 thread_in_review / parent_report 永远不会发出，因为进入 in_review 的跃迁根本不会发生。同一机制也会清掉同伴向人类提出的 blocked 提问 —— 那一半与 §9.11 重叠，而 thread-status.ts 声明它未决，所以它需要一个有意识的决定，而不是由默认值决定。
建议修法：给准入路径自己的谓词（failure | unclosed | waiting —— 保留 waiting，I1 的"或人类发言"那一支才成立），不要动 review，并补上缺失的测试：同伴的 review 义务未决时，一条排上工作的发言，在线程进入静默态后仍必须解析为 in_review。当前行为下套件是绿的，所以 step 6 会照原样在它之上继续搭建。
非拦截项，但我希望一并处理：applyAggregateStatus 和 waiting 判据忽略了 listThreads() 的 unreadable，而准入路径遇到它会抛错（一条读不出来的兄弟记录会变成一次被拒绝的等待，或者落盘的 blocked 状态加一条 thread_blocked 通知）；unclosed 只在 completed 时记录，所以被 cancelled 的 run 不留任何义务，而注释夸大了这条规则；authorKind / sourceRunId / triggerKind / SYSTEM_AUTHOR_ID 在本 PR 和 base 分支上都没有生产者，因此应该和 step-6 调度器一起提交。
关于这个栈的提示：ci.yml 只在 main / release/** 上触发，所以这个 head 完全没有跑过 CI —— 那 57 个测试、ESLint 和 tsc 只是你本地的结果。我静态核对了本 PR 触及的每个字段在 base 的 types.ts 里都已存在、acknowledgeCloseObligations 确实接受可选的第三个 select、事务辅助函数都是 *Unlocked（不会死锁），但没有任何东西真正执行过这些改动。
— Qwen Code · qwen3.8-max-2026-09-02
|
Qwen Code review timed out. Qwen review timed out after 1153 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review — head cbc04399, base codex/mesh-step-5-status (draft)
Reviewed as a stacked increment on codex/mesh-step-5-status; scope is the 5 changed files at this SHA. Core-infrastructure gate: packages/core/src/agents/**, maintainer-authored and therefore exempt from the two-tier block. I verified the two findings below by reading run-lifecycle.ts and thread-status.ts at this exact commit; I did not run the suite locally.
Critical 1 — a peer's waiting obligation is discharged too late, so a normal close reports the thread blocked
closeRunInTransaction discharges peers' waits at close time:
next = acknowledgeCloseObligations(
next,
next.nextMessageSequence - 1,
(obligation) =>
obligation.kind === 'waiting' && obligation.runId !== run.id,
);and then sets the closing run to status: 'finishing', closeKind: input.request.kind.
The problem is on the reading side. obligationFor checks liveness before it reads closeKind:
if (LIVE_RUN_STATUSES.has(run.status)) return undefined;and LIVE_RUN_STATUSES contains finishing. So while the closing run sits in finishing, it contributes no obligation at all — its closeKind is invisible. A peer that was waiting on it, if its own obligation has not yet been acknowledged, falls into the strandedWait branch and the thread is reported blocked with "waiting on work that no longer exists." Which is the opposite of true: the work exists and is finishing normally.
The discharge and the finishing transition happen in the same transaction here, so the in-transaction path is fine. The exposure is any read that observes the thread between the two, and any ordering where a peer's wait is registered against a run already in finishing.
The test that looks like it covers this does not. run-lifecycle.test.ts:174 pre-sets the peer to a terminal status before the close, so the finishing window is never entered and the ordering under discussion is never exercised.
Critical 2 — a cancelled run leaves no obligation at all, so the thread never leaves in_progress
closeKind:
run.closeKind ??
(input.outcome.status === 'completed' ? 'unclosed' : undefined),A cancelled run keeps closeKind: undefined. In obligationFor, cancelled is not in LIVE_RUN_STATUSES and is not failed, so control reaches if (run.closeKind === undefined) return undefined; and no obligation is produced. With no outstanding obligation, the tail returns status: thread.status === 'open' ? 'open' : 'in_progress' with reason "no outstanding close obligation."
So a thread whose only run was cancelled reports in_progress forever, with nothing outstanding to explain it and nothing that will ever change it. That is the state the status model exists to prevent.
Worth noting that the header comment claims "no ordering of concurrent run completions can leave a stale status behind." Both findings above are counterexamples to that specific sentence, which is why I am treating them as Critical rather than as polish: the comment is the contract, and it is currently stronger than the code.
Dead switches
Five fields are declared and read but never populated by any caller I could find at this SHA: authorKind, sourceRunId, triggerKind, SYSTEM_AUTHOR_ID, failureStage. Per the project's review rule, I checked the read sites, and each one takes its ?? … default unconditionally. If they are placeholders for the next step in the stack, a one-line note saying so would keep the next reviewer from re-deriving this. Suggestion, not Critical.
Verdict
C=2. The two Criticals are the same mechanism seen from two directions — obligationFor's liveness check running ahead of its closeKind read — so one fix at that site likely closes both. Limitation: no local test run, no cross-platform CI evidence.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-5 redundant whole-store thread scan under the workspace lock — already reported (@qwen-code-ci-bot, stage-2 review comment 5564320055)
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): src/agents/mesh/run-lifecycle.test.ts — no such file or directory; src/agents/mesh/thread-actions.test.ts — no such file or directory; src/agents/mesh/thread-status.test.ts — no such file or directory; src/agents/mesh/mesh-store.test.ts — no such file or directory; 57 tests passed — this review observed 23674, 1959, 28851, 298, 1818, 504, 6362 passed.
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
|
||
| **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. | ||
|
|
||
| **Run close and status application landed.** `run-lifecycle.ts` splits closing into two writes: the tool records `closeKind` and moves the run to `finishing`, ending the agent's turn, and the runtime callback records the terminal state — the only place the aggregate status is recomputed. A `waiting` close is refused when nothing could wake it, and a live _descendant_ counts while a mere sibling under the same root does not. Any close discharges peers' waits on the same thread. A clean exit with no closing tool is recorded as `unclosed`, never as implicit success. `finishRun` now delegates to this one path, and `postMessage` discharges outstanding obligations when it books work and then applies the aggregate status, so the I2 and I6 fixes have producers rather than only a resolver. Observed locally: `run-lifecycle.test.ts`, `thread-actions.test.ts`, `thread-status.test.ts`, `mesh-store.test.ts` → 4 files, 57 tests passed; targeted ESLint clean. The six thread tools that call `closeRun` are still to come, so gates (a), (b), (c) and (e) remain unexecuted. |
There was a problem hiding this comment.
[Suggestion] R1-29: The acceptance entry this diff adds names one aggregate-status writer when the same diff adds two, contradicts itself four sentences later on the same line, and attributes closeRun to all six of step 5's tools when only three can call it. This file is the gate ledger the next step is written from.
A step-6 implementer who trusts "the only place the aggregate status is recomputed" reasons about run completion as the sole status writer and never considers that an ordinary human or agent post also recomputes and persists status — which is exactly the write path confirmed as Critical R1-22 (thread-actions.ts:337). Separately, "The six thread tools that call closeRun" is a restrictive relative clause and it is false: only thread_wait/thread_block/thread_review map onto RunCloseRequest's three kinds, thread_post goes through postMessage, and thread_create/thread_read never close a run — so a step-5 implementer wiring from this record calls closeRun from tools that must not.
Witness:
`grep -rn "applyAggregateStatus" --include=*.ts packages/core/src | grep -v '\.test\.ts'` → 2 production writers (`thread-actions.ts:337`, `run-lifecycle.ts:401`), not 1. The phrase is added by this diff: `grep -c "^+.*the only place the aggregate status is recomputed"` on the diff → 1; `git grep -c … e0cef4577f -- docs/plans/` → 0. Step 5's tool list (`acceptance.md:55`) is six tools against `RunCloseRequest`'s three kinds (`run-lifecycle.ts:36-39`). The same added line also says "`postMessage` … then applies the aggregate status", contradicting "the only place".
Suggested fix: Correct that one sentence in both respects: name the two recomputation sites (the terminal callback and the admission path), and scope the closing tools to the three that can call closeRun — e.g. "…the runtime callback records the terminal state; the closing tool never writes the aggregate status itself, and the terminal callback and the admission path are the two places it is recomputed. The three closing tools (thread_wait, thread_block, thread_review) are still to come…".
The fix has to respect this: AGENTS.md requires the design doc and this acceptance file to stay current in the same commit as the code that changes them, and this file is designated the step's contract — so the correction belongs in this PR, not a follow-up. Note the gate-(d) half of the original claim was verified and rejected: the sentence's causal scoping ("…are still to come, so gates (a), (b), (c) and (e) remain unexecuted") legitimately excludes gate (d), which needs the separately-listed prompt assembler.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| expect(result.thread.outbox).toHaveLength(1); | ||
| expect(result.thread.outbox[0]?.payload['event']).toBe('blocker_raised'); |
There was a problem hiding this comment.
[Suggestion] R1-33: No test in the mesh suite asserts the status: 'pending' that the new enqueue writes, although that single literal is the whole handshake between this diff's outbox producer and every consumer of it — and this diff is what makes the outbox production-written.
isValidEvent accepts both members of ThreadEventStatus, so changing enqueue's literal to 'acknowledged' still validates and still writes while every outbox assertion in the suite keys on something else. The mutant then: (i) makes reconcileThreadOutbox skip every event this diff produces (if (event.status !== 'pending') continue;), so step 6's drainer delivers nothing — no blocker page, no thread_in_review, no parent_report to wake a waiting parent; (ii) stops alreadyReported ever matching, so an in_review → in_progress → in_review cycle wakes the parent once per cycle instead of once; (iii) stops deleteThread's pending-event guard protecting a thread holding undelivered reports, making it deletable and dropping them.
Witness:
`BASELINE (unmodified PR) Test Files 9 passed (9) Tests 100 passed (100)`; `MUTANT A run-lifecycle.ts:144 status: 'pending' -> 'acknowledged': Test Files 9 passed (9) Tests 100 passed (100)`; `MUTANT A + suggested assertion: AssertionError: expected 'acknowledged' to be 'pending'; Test Files 1 failed (1) Tests 1 failed | 11 passed (12)`; `MUTANT A reverted, assertion kept: Tests 12 passed (12)`. Consequence driven, not inferred — a real cycle probe: `INTACT: STEP3 status=in_review parentReports=1` vs `MUTANT A: STEP3 status=in_review parentReports=2 outbox=[child_in_review, thread_in_review, child_in_review, thread_in_review]`.
Suggested fix: Pin the produced state where the event is already held — extend the blocker_raised assertions with expect(result.thread.outbox[0]?.status).toBe('pending');, and do the same for the thread_blocked and parent_report events. Note this pins the status literal but not the dedup; only the status-cycle test R1-15 asks for does that.
The fix has to respect this: The asserted literal must be 'pending' — mesh-store.ts:1266 reads if (event.status !== 'pending') continue; and types.ts:254 declares export type ThreadEventStatus = 'pending' | 'acknowledged';, so 'acknowledged' validates and writes: the mutation is silent, not a crash.
Acceptance criterion: The new status assertion goes red when enqueue's status: 'pending' is changed to 'acknowledged', and green when reverted. Please prove it by mutation — apply the fix, then remove it again and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| request: { kind: 'waiting' }, | ||
| }), | ||
| ).rejects.toThrow(MeshCloseRejectedError); |
There was a problem hiding this comment.
[Suggestion] R1-14: The same-thread-peer branch of the waiting gate (otherLive) is never exercised: both kind: 'waiting' tests close a run that is the thread's only run, so the gate is only ever satisfied by the sub-thread branch or refused outright.
Deleting the !otherLive && disjunct from run-lifecycle.ts:186 leaves all tests green — the refusal test still refuses (no peer, no child) and the sub-thread test still passes via hasLiveDescendant. The behaviour that goes silently missing is the design's I1 workflow ("A waits for B; B reviews"): an agent calling thread_wait while a peer run is still queued/running on the same thread, with no sub-thread open, would get no_live_dependency and be told to block or keep working instead of waiting.
Witness:
`MUTATED (`!otherLive &&` disjunct and its binding removed) -> Tests 100 passed (100)`; `PROBE intact: Tests 1 passed (1) (waiting close succeeds, closeKind='waiting')`; `PROBE mutated: MeshCloseRejectedError: Nothing else is running on this thread and no sub-thread is open, so waiting would strand it. Block with a question, submit for review, or keep working.`
Suggested fix: Add a case that closes with waiting while a peer run is live and no sub-thread exists, asserting the close succeeds and closeKind === 'waiting'. Note the interaction with R1-2: the peer must be queued or running, not finishing.
The fix has to respect this: The gate counts only 'queued' | 'running' | 'finishing' (run-lifecycle.ts:184-186), narrower than LIVE_RUN_STATUSES (thread-status.ts:42-47, which also includes 'cancelling'); runs sharing one thread file must have distinct queueSequence values or writeThread rejects the record (mesh-store.ts:382).
Acceptance criterion: The new test throws MeshCloseRejectedError if the !otherLive && disjunct is removed, which no existing test detects. Please prove it by mutation — apply the fix, then remove it again and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| expect(hasLiveDescendant(threads, parent.id)).toBe(false); | ||
| expect(hasLiveDescendant([{ ...parent }, { ...child }], parent.id)).toBe( | ||
| true, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-13: The test named "allows a wait once a sub-thread is open, and not for a mere sibling" never exercises a sibling, and the assertion its comment names is vacuous: the fixture's sibling has parentThreadId: parent.id, making it a second child of the walk root, and both children are forced to done.
Three mutations all survive the suite: (a) re-key hasLiveDescendant on rootThreadId — the exact mistake the code comment and the added plan paragraph say the walk exists to avoid; (b) flatten its BFS recursion, so a legal waiting close on a thread whose sub-thread's own sub-thread is live is refused with no_live_dependency; (c) re-key the closeRun call site to thread.rootThreadId. Under (c) an agent whose own thread is quiescent is allowed to close waiting because an unrelated sibling under the same root is active; nothing on its thread can wake it, so applyAggregateStatus later resolves it blocked instead of the tool refusing up front. The parentThreadId-vs-rootThreadId distinction is nominated as load-bearing in the PR description while the test's name and comment tell the next reviewer it is covered.
Witness:
`MUTATION (a) byParent keyed on rootThreadId -> Tests 100 passed (100)`; `MUTATION (b) queue.push(child.id) removed -> Tests 100 passed (100)`; `MUTATION (c) closeRun uses thread.rootThreadId -> Tests 100 passed (100)`; `PROBE intact: -> Tests 2 passed (2)`.
Suggested fix: Pin the walk from the child's point of view — expect(hasLiveDescendant([{ ...child }, { ...sibling }], child.id)).toBe(false) with sibling left at its created non-done status; add a grandchild case asserting a live grandchild under a done child still counts; and add a closeRun-level refusal case with a live sibling sub-thread and no other live run on the waiting thread.
The fix has to respect this: Liveness is status !== 'done' (run-lifecycle.ts:94) and createThread defaults a new thread to status: 'open' (mesh-store.ts:1139), so a grandchild fixture must be non-done; the closeRun refusal case needs !otherLive, and otherLive counts any other run in queued | running | finishing (:181-187), so the fixture's waiting run must be the thread's only run.
Acceptance criterion: Those added assertions — the first goes red under a root/same-root re-key, the second under a flattened recursion, the third under a root-scoped call site. All green against the implementation as committed. Please prove it by mutation — apply the fix, then remove it again and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| it('refuses a close for a run the caller does not own', async () => { | ||
| const thread = await seed(); |
There was a problem hiding this comment.
[Suggestion] R1-25: The run_not_bound guard refuses on !run || run.agentId !== input.agentId || run.status !== 'running', but only the wrong-agent disjunct is exercised. The run.status !== 'running' disjunct — the replay / double-close protection the function's own docblock names — has no test, so the guard can be narrowed to the wrong-agent check alone with the suite green.
All eight closeRun call sites pass a hand-seeded running run, so deleting || run.status !== 'running' keeps all tests green. Step 5b's six tools will call this gate with model-driven input, where a duplicated closing-tool call is routine (two closing tools in one turn, or a retry after a timeout). A second thread_block on a run whose terminal write already landed flips it from completed/failed back to finishing: LIVE_RUN_STATUSES then hides its recorded obligation, so a failure a person had to see stops being reported and the resolver returns in_progress for a thread with nothing running; since that run's callback already fired, no further write ever moves it out of finishing (restart reconciliation is step 8). The same call appends a duplicate message, overwrites finalMessageId and closeKind — a run that closed review and then hit the block tool becomes blocked, flipping the aggregate from in_review — re-acks peers at a new sequence, and enqueues a second blocker_raised, whose site has no dedup.
Witness:
Mutation arm — delete `|| run.status !== 'running'` from `run-lifecycle.ts:173`: the whole mesh suite stays green (100 tests), while the added replay case fails, showing a terminal run re-opened to `finishing` with its obligation hidden.
Suggested fix: Add a case that closes an already-finishing run (close twice in a row) and one that closes a completed/failed run and a fabricated runId, each asserting MeshCloseRejectedError with code === 'run_not_bound', plus a readThread assertion that the thread file is unchanged by the refusal — no second message, no re-acknowledged peer, one outbox entry.
The fix has to respect this: run-lifecycle.ts:45 declares exactly 'no_live_dependency' | 'run_not_bound' | 'thread_done', so a replayed close must not get a fourth code; and :173-177 raises one MeshCloseRejectedError('run_not_bound', …) for three distinct causes, so a new test can distinguish "not running" from "no such run" only by message text.
Acceptance criterion: The new case goes red when || run.status !== 'running' is dropped from run-lifecycle.ts:173, which no existing test detects. Please prove it by mutation — apply the fix, then remove it again and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| authorKind: | ||
| input.authorKind ?? | ||
| (input.from === HUMAN_AUTHOR_ID ? 'human' : 'agent'), |
There was a problem hiding this comment.
[Suggestion] R1-7: The documented derivation ("Derived from from when absent", thread-actions.ts:39-44) never derives system, so one object literal records a from: SYSTEM_AUTHOR_ID post as authorKind: 'agent' while two lines below (:203) it treats that same from as a system author for authorNameSnapshot.
A step-6 dispatcher posts a structured trigger the way the new doc invites — postMessage(root, threadId, { from: SYSTEM_AUTHOR_ID, text: …, triggerKind: 'parent_report' }) — omitting the optional authorKind because it is documented as derived. The stored message is authorKind: 'agent' with authorNameSnapshot: 'system': self-contradictory, and it passes the store validator, so nothing rejects it. Any consumer keyed on authorKind (a UI badge, or a rule separating agent turns from unattended triggers) mis-attributes a system trigger to an agent and nothing downstream can tell.
Witness:
`postMessage(from=SYSTEM_AUTHOR_ID): {"authorKind":"agent","authorNameSnapshot":"system","from":"system","triggerKind":"parent_report"}`; `round-trip on disk : [{"authorKind":"agent","authorNameSnapshot":"system",…}]`; `postMessage(from=HUMAN) : human`; `postMessage(from=ag_alice) : agent`; `postMessage(authorKind="system"): {"authorKind":"system","snapshot":"system"}` — one `from` value, two different answers in one literal.
Suggested fix: Either derive all three kinds so the contract holds — input.authorKind ?? (input.from === HUMAN_AUTHOR_ID ? 'human' : input.from === SYSTEM_AUTHOR_ID ? 'system' : 'agent') — or drop authorKind/sourceRunId/triggerKind and SYSTEM_AUTHOR_ID until the dispatcher that sets them lands (AGENTS.md Simplicity First). The dead-switch half of this is already on the PR record from both prior reviewers; the new claim is the self-contradiction inside one object literal.
The fix has to respect this: authorKind: 'human' | 'agent' | 'system'; (types.ts:115), accepted verbatim by the record validator at mesh-store.ts:246-248 — the derived value must stay inside that union.
Acceptance criterion: A thread-actions.test.ts case posting { from: SYSTEM_AUTHOR_ID, text: 'assignment', sourceRunId: 'rn_1', triggerKind: 'assignment' } and asserting message.authorKind === 'system' plus that sourceRunId/triggerKind round-trip through readThread. Red today on authorKind ('agent'). Please prove it by mutation — apply the fix, then remove it again and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| dispatched.length > 0 || | ||
| outcomes.some((o) => o.decision.kind === 'coalesce') | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R1-9: The coalesce half of the new acknowledgement condition has no test anywhere, so the I2 regression it exists to prevent can be reintroduced by deleting it with the suite staying green.
Delete || outcomes.some((o) => o.decision.kind === 'coalesce') and the whole mesh suite stays green: the one I2 test reaches only the dispatched.length > 0 half, and the only coalesce test asserts outcomes and autoTurnsUsed but never closeAcknowledgedAtSequence. In production: bob's run failed at launch (obligation failure), alice has a run still queued, and a further post folds into alice's queued run instead of booking a new one. Without the coalesce half bob's failure is never discharged; when alice completes with closeKind: 'review', BLOCKING_KINDS outranks review and the thread is written blocked — "run rn_bob failed and no successor is runnable" — although the successor ran and finished. That is exactly the I2 failure the comment above the line says it fixes.
Witness:
`MUTATED Test Files 9 passed (9) / Tests 100 passed (100) <-- the whole coalesce half is unpinned`; `PROBE (coalesce-only post, failed peer run) mutated: expected undefined to be 1 (rn_bob.closeAcknowledgedAtSequence)`; `PROBE mutated, ack assertions removed: expected 'blocked' to be 'in_review'`; `PROBE intact: Tests 1 passed (1)`.
Suggested fix: Add a test seeding a thread with a failed run for one agent and a queued/running run for another, posting a message that coalesces into that run (dispatched empty), asserting the failed run's closeAcknowledgedAtSequence equals the new message sequence, then finishing the running run cleanly and asserting the thread is not blocked.
The fix has to respect this: decideDispatch returns coalesce only when the target already has a live run on this thread (dispatch-policy.ts:141-145); a fixture whose target has no live run gets dispatch instead and silently re-tests the already-covered clause.
Acceptance criterion: That new case goes red when the coalesce disjunct is deleted, which no existing test does. Please prove it by mutation — apply the fix, then remove it again and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // The status is an aggregate over every run, never last-writer-wins, and it | ||
| // is recomputed here so an admission that books nothing cannot leave the | ||
| // thread sitting in `in_progress` with no live run and no explanation. |
There was a problem hiding this comment.
[Suggestion] R1-8: The applyAggregateStatus call this comment introduces overwrites the two hand-written status: assignments still present in the coalesce and dispatch branches above it (thread-actions.ts:264-269 and :298-303, both unchanged pre-existing code), so the function now carries two contradictory status authorities — and the comment declares there is only one.
Both branches are reachable only after decideDispatch returned coalesce (into a queued/running run) or dispatch (adding a queued run), so resolveThreadStatus always takes its live-run branch and returns in_progress, overwriting whatever the ternary wrote; the done case the ternary guards against is unreachable because decideDispatch returns {kind:'skip', reason:'thread_done'} first. The ~12 lines are dead, and the next person changing the aggregate rules — or debugging a wrong status — must first discover that assignments 70 lines above the resolver do nothing, in a function whose own new comment says the status is "never last-writer-wins".
Witness:
Same input on both arms (an agent-authored post, so the ternary's condition is false and it leaves the stored status alone): the assignments were measured to have no effect on the persisted status, and the deletion keeps the suite green. Verified in scope — this diff is what makes them dead, by adding the `applyAggregateStatus` call at `:337`.
Suggested fix: Delete the status: property (and its ternary) from both the coalesce and dispatch branch objects, leaving applyAggregateStatus as the only writer of next.status in this function.
The fix has to respect this: if (live.length > 0) { returning in_progress (thread-status.ts:168) is what makes the assignments dead — the deletion must not touch that resolver.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| // The status is an aggregate over every run, never last-writer-wins, and it | ||
| // is recomputed here so an admission that books nothing cannot leave the | ||
| // thread sitting in `in_progress` with no live run and no explanation. | ||
| next = await applyAggregateStatus(transaction, next, now); |
There was a problem hiding this comment.
[Critical] R1-22: [certifies-falsely] [regression] resolveThreadStatus's "last admission booked nothing" branch keys positionally on thread.messages[length-1] with no sequence comparison, and sits above the review branch and above the conditional live-child waiting branch (thread-status.ts:207-231). resolveThreadStatus had no production caller at the merge base, so the two applyAggregateStatus sites this diff adds are what make that branch order reachable from a persisted write.
A thread sits in_review with an outstanding review obligation (last message is the close summary, outcomes: []). A human replies @alicce lgtm with a typo: parseMentions yields unknown: ['alicce'] and ids: [], so the only outcome is {skip, agent_unknown}, and the admission write persists status: 'blocked', reason the last post booked no work (agent_unknown), enqueues thread_blocked, and masks the pending review in the only field a UI reads — permanently, if the reason is a disabled assignee or an exhausted token budget, since no later post can reach the review branch. Symmetrically, an agent's self_trigger progress note stays the last message when it then closes waiting, so the parent is written blocked with a false page while its sub-thread runs. And a close message's outcomes: [] masks an earlier real booked-nothing admission, so the same post yields blocked or in_review depending on event ordering.
Witness:
Probe on unmodified PR code: `E3 persisted: status=blocked runs=[rn_alice:completed/review]` with `outbox=[thread_in_review(pending), thread_blocked(pending) reason="the last post booked no work (agent_unknown)"]` — an outstanding review obligation present, `blocked` persisted, and two contradictory notifications pending at once. BASE arm (the added `applyAggregateStatus` call removed from `postMessageInTransaction`, faithful: the symbol does not exist at merge base), same input: the wrong persisted status does not occur.
Suggested fix: In thread-status.ts, move the if (lastMessage && admissionBookedNothing(lastMessage)) branch below the review branch and below the stranded-wait-with-live-child branch, or gate it on outstanding.length === 0, so it decides status only when there is genuinely no obligation to explain the thread. Alternatively resolve it against the most recent admission rather than the final message: const lastMessage = [...thread.messages].reverse().find((m) => m.outcomes.length > 0);. Do not fix it by giving close messages outcomes — a skip outcome on a close message would itself trip admissionBookedNothing.
The fix has to respect this: thread-status.test.ts:201-202 asserts blocked + reason containing agent_unknown for a fixture with a booked-nothing last message and no runs; :205-211 asserts in_progress for outcomes: []; and this diff's own run-lifecycle.test.ts:296-310 pins blocked plus a thread_blocked event for a freshly created unassigned thread. A reorder must keep all three green — the fresh-open/no-obligation case must still return blocked.
Acceptance criterion: A run-lifecycle.test.ts case seeding a run {status:'completed', closeKind:'review'}, asserting finish() gives in_review, then posting {from: HUMAN_AUTHOR_ID, text: '@alicce lgtm'} with agents: [ALICE] and asserting posted.thread.status === 'in_review' with no thread_blocked; plus a case pinning in_progress for a waiting obligation with a live child. Moving the branch back above review turns both red. Please prove it by mutation — apply the fix, then remove it again and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
| return withMeshStoreTransaction(projectRoot, (transaction) => | ||
| finishRunInTransaction(transaction, { threadId, runId, outcome, now }), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-16: The rewritten public finishRun, the entry point step 6's dispatcher will call and the one whose behaviour this diff changes most, has no test; the new suite's finish() helper drives finishRunInTransaction directly and finishRun has no caller anywhere.
Reverting finishRun to the pre-diff inline write — no closeKind: 'unclosed', no applyAggregateStatus, no failureStage — keeps every mesh test green. What ships then: a run that exits cleanly without calling a closing tool is recorded as a bare completed with no obligation, so resolveThreadStatus reports the thread in_progress instead of blocked, and the "never as an implicit success" rule this module's fileoverview exists to enforce is absent on the only path an external caller reaches.
Witness:
`MUTATED (finishRun reverted to the pre-diff inline write, unused import dropped) -> Tests 100 passed (100)`; `PROBE driving the PUBLIC finishRun(projectRoot, threadId, runId, outcome, now): mutated: expected undefined to be 'unclosed' (closeKind); mutated: expected undefined to be 'launch' (failureStage)`. Sweep: `grep -rn "\bfinishRun\b" packages --include=*.ts` finds only its own definition at `thread-actions.ts:392` — no caller.
Suggested fix: Point the finish() helper in run-lifecycle.test.ts at the public finishRun(projectRoot, threadId, runId, outcome, now), keeping one case on finishRunInTransaction for the in-transaction contract.
The fix has to respect this: finishRun still takes now as the fifth positional parameter (thread-actions.ts:392-401), so a helper switch must not fold now into the outcome object, which finishRunInTransaction would ignore.
Acceptance criterion: "records a clean exit with no closing tool as unclosed and blocks" and "carries a typed failure stage onto the run and blocks the thread", driven through the public finishRun, go red if the delegation is reverted or drops failureStage. Please prove it by mutation — apply the fix, then remove it again and confirm that test goes red.
— qwen3.8-max via Qwen Code /review (v0.23.3)
4 participants
What this PR does
Adds
mesh/run-lifecycle.ts: how a mesh run ends, and what the thread does about it. It also gives the aggregate status resolver from #11230 its two producers —finishRunnow delegates here, andpostMessagedischarges outstanding close obligations when it books work before applying the resolved status.Stacked on #11230 (it imports the resolver). Independent of #11229.
Why it's needed
Closing has to be two writes. A closing tool is called mid-turn: the model is still executing, so the tool cannot mark its own runtime finished. It records what the run is closing as and moves it to
finishing, which ends the turn; the runtime callback then records the terminal state, and only there is the thread's status recomputed. A crash between the two leaves afinishingrun carrying acloseKind, which is a complete instruction for restart reconciliation. A status written optimistically before the runtime actually stopped would be a lie the next reader cannot detect.A wait must be refusable.
thread_waitis rejected when no other run is live and no sub-thread is open, because nothing could ever wake it. The dependency is walked overparentThreadId, notrootThreadId: a sibling sub-thread of the same root was never delegated by this thread, and counting it would recreate exactly the stranded wait the resolver exists to catch.The resolver needed writers. #11230 landed the rules; without producers they had no effect.
postMessagenow discharges outstanding obligations when it actually books work, so an obsolete failure stops pinning a thread after later work succeeded (I2), and it applies the aggregate status, so a post that books nothing leaves the threadblockedrather than sitting inin_progresswith no live run and no explanation (I6). Any close discharges peers' waits on the same thread (I1). A clean exit that never called a closing tool is recorded asunclosed, never as implicit success.Cross-file effects go on the source thread's outbox: a blocker notification at close time, and the parent report plus review notification when the whole thread reaches
in_review— not when one agent submits its part while another still works.Reviewer Test Plan
How to verify
cd packages/core npx vitest run src/agents/mesh/run-lifecycle.test.ts \ src/agents/mesh/thread-actions.test.ts \ src/agents/mesh/thread-status.test.ts \ src/agents/mesh/mesh-store.test.tsConfirm a blocked close posts the question and leaves the run
finishingwith the thread stillin_progress; a wait with no dependency is refused; a wait is allowed once a sub-thread exists but a done sibling does not qualify; a close by an agent that does not own the run is refused; a peer's wait is discharged so a later review reportsin_reviewrather thanblocked; a clean exit recordsunclosedand blocks; a child in review enqueues exactly one parent report even when the terminal write is replayed; a human post that books work clears an obsolete failure; and an unassigned post blocks the thread.Evidence (Before & After)
N/A — internal lifecycle, status application, and tests only.
Tested on
Environment (optional)
Node.js 22 workspace, named Vitest files only. Observed: 4 files / 57 tests passed. Targeted ESLint clean.
Risk & Scope
postMessagenow recomputes the thread status on every post, which lists sibling threads inside the workspace transaction. At v1 scale that is a directory read under a lock already held; if it ever matters, the resolver's only I/O need is "is a descendant live", which can be cached on the thread.closeRunyet — the six thread tools are the remaining part of step 5b. Step-5 gates (a), (b), (c) and (e) stay unexecuted, as doesqueueExternalInput(false)detach/rebook (step 6/7). Outbox events are enqueued but no consumer drains them until step 6.finishRundelegates to the new path and gains an optionalfailureStage; its existing signature and callers are unchanged.Linked Issues
Parent delivery PR: #11206. Depends on #11230. Sibling: #11229.