Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions docs/plans/2026-09-06-multi-agent-board-collaboration.md
Original file line number Diff line number Diff line change
Expand Up @@ -522,14 +522,15 @@ action on the child caused it.
Pre-existing on this branch (five production files plus two tests; nothing
starts an agent yet):

| File | Responsibility |
| ----------------------------------------- | -------------------------------------------------- |
| `core/src/agents/mesh/types.ts` | Entities and limits |
| `core/src/agents/mesh/mesh-store.ts` | Paths, validation, locking, CRUD |
| `core/src/agents/mesh/mentions.ts` | `@name` → agent ids |
| `core/src/agents/mesh/dispatch-policy.ts` | `decideDispatch` — pure |
| `core/src/agents/mesh/thread-actions.ts` | `postMessage` — append and book under one lock |
| `core/src/agents/mesh/thread-status.ts` | Aggregate status over every run's close obligation |
| File | Responsibility |
| ----------------------------------------- | ----------------------------------------------------- |
| `core/src/agents/mesh/types.ts` | Entities and limits |
| `core/src/agents/mesh/mesh-store.ts` | Paths, validation, locking, CRUD |
| `core/src/agents/mesh/mentions.ts` | `@name` → agent ids |
| `core/src/agents/mesh/dispatch-policy.ts` | `decideDispatch` — pure |
| `core/src/agents/mesh/thread-actions.ts` | `postMessage` — append and book under one lock |
| `core/src/agents/mesh/thread-status.ts` | Aggregate status over every run's close obligation |
| `core/src/agents/mesh/run-lifecycle.ts` | Run close, terminal state, status application, outbox |

### 5.1 Local review correction — committed and verified

Expand Down
2 changes: 2 additions & 0 deletions docs/plans/2026-09-07-mesh-implementation-acceptance.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ Evidence: the assembled prompt text for cases first-entry / delta / gap / retry,

**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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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)


### Step 6 — Minimal in-process dispatcher, no recovery

Lands: pick the lowest `queueSequence` queued run per agent; branch on registry state `completed+resident → continue`, `completed → revive`, `paused → resume`, `unbound → launch`; `startRun` / `finishRun`; consume the parent-report outbox; leave `capacity_wait` queued.
Expand Down
344 changes: 344 additions & 0 deletions packages/core/src/agents/mesh/run-lifecycle.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,344 @@
/**
* @license
* Copyright 2026 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import * as fs from 'node:fs/promises';
import * as os from 'node:os';
import * as path from 'node:path';
import { afterEach, beforeEach, describe, expect, it } from 'vitest';

import { Storage } from '../../config/storage.js';
import {
createThread,
readThread,
updateMeshAgents,
writeThread,
} from './mesh-store.js';
import {
closeRun,
finishRunInTransaction,
hasLiveDescendant,
MeshCloseRejectedError,
} from './run-lifecycle.js';
import { withMeshStoreTransaction } from './mesh-store.js';
import { postMessage } from './thread-actions.js';
import {
HUMAN_AUTHOR_ID,
type MeshAgent,
type Thread,
type ThreadRun,
} from './types.js';

const PROJECT_ROOT = '/mesh-lifecycle-test';
const ALICE: MeshAgent = { id: 'ag_alice', name: 'alice', createdAt: 1 };
const BOB: MeshAgent = { id: 'ag_bob', name: 'bob', createdAt: 1 };

function run(overrides: Partial<ThreadRun> = {}): ThreadRun {
return {
id: 'rn_alice',
agentId: ALICE.id,
status: 'running',
triggerMessageIds: [],
acceptedMessageIds: [],
consumedMessageIds: [],
usageByRound: [],
// Well clear of the workspace counter: these fixtures are hand-written and
// must not collide with a sequence the store allocates during the test.
queueSequence: 100,
queuedAt: 1_000,
attempts: 1,
...overrides,
};
}

async function seed(overrides: Partial<Thread> = {}): Promise<Thread> {
const created = await createThread(PROJECT_ROOT, { title: 'Investigate' });
const thread: Thread = {
...created,
status: 'in_progress',
runs: [run()],
...overrides,
};
await writeThread(PROJECT_ROOT, thread);
return thread;
}

function finish(
threadId: string,
runId: string,
outcome: Parameters<typeof finishRunInTransaction>[1]['outcome'],
) {
return withMeshStoreTransaction(PROJECT_ROOT, (transaction) =>
finishRunInTransaction(transaction, { threadId, runId, outcome }),
);
}

describe('mesh run lifecycle', () => {
let runtimeDir: string;

beforeEach(async () => {
runtimeDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mesh-lifecycle-'));
Storage.setRuntimeBaseDir(runtimeDir);
await updateMeshAgents(PROJECT_ROOT, () => [ALICE, BOB]);
});

afterEach(async () => {
Storage.setRuntimeBaseDir(null);
await fs.rm(runtimeDir, { recursive: true, force: true });
});

it('posts the question, records the close, and ends the turn without finishing the run', async () => {
const thread = await seed();

const result = await closeRun(PROJECT_ROOT, {
threadId: thread.id,
runId: 'rn_alice',
agentId: ALICE.id,
request: { kind: 'blocked', question: 'which retry path?' },
});

expect(result.message?.text).toBe('which retry path?');
expect(result.message?.authorKind).toBe('agent');
expect(result.message?.sourceRunId).toBe('rn_alice');
expect(result.message?.authorNameSnapshot).toBe('alice');
// The runtime is still executing, so the run may not be marked terminal.
expect(result.thread.runs[0]?.status).toBe('finishing');
expect(result.thread.runs[0]?.closeKind).toBe('blocked');
expect(result.thread.runs[0]?.finalMessageId).toBe(result.message?.id);
expect(result.thread.status).toBe('in_progress');
expect(result.thread.outbox).toHaveLength(1);
expect(result.thread.outbox[0]?.payload['event']).toBe('blocker_raised');
Comment on lines +111 to +112

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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)

});

it('refuses a wait that nothing could ever wake', async () => {
const thread = await seed();

await expect(
closeRun(PROJECT_ROOT, {
threadId: thread.id,
runId: 'rn_alice',
agentId: ALICE.id,
request: { kind: 'waiting' },
}),
).rejects.toThrow(MeshCloseRejectedError);
Comment on lines +123 to +125

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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)

});

it('allows a wait once a sub-thread is open, and not for a mere sibling', async () => {
const parent = await seed();
const child = await createThread(PROJECT_ROOT, {
title: 'read the code',
parentThreadId: parent.id,
});

const waited = await closeRun(PROJECT_ROOT, {
threadId: parent.id,
runId: 'rn_alice',
agentId: ALICE.id,
request: { kind: 'waiting' },
});
expect(waited.thread.runs[0]?.closeKind).toBe('waiting');

// A sibling under the same root is not this thread's dependency.
const sibling = await createThread(PROJECT_ROOT, {
title: 'unrelated',
parentThreadId: parent.id,
});
const threads = [
{ ...parent },
{ ...child, status: 'done' as const },
{ ...sibling, status: 'done' as const },
];
expect(hasLiveDescendant(threads, parent.id)).toBe(false);
expect(hasLiveDescendant([{ ...parent }, { ...child }], parent.id)).toBe(
true,
);
Comment on lines +153 to +156

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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();
Comment on lines +159 to +160

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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)


await expect(
closeRun(PROJECT_ROOT, {
threadId: thread.id,
runId: 'rn_alice',
agentId: BOB.id,
request: { kind: 'review', summary: 'done' },
}),
).rejects.toThrow(/not a running run of agent "ag_bob"/);
});

it('discharges a peer wait so a review is not reported as blocked', async () => {
const thread = await seed({
runs: [
run({ id: 'rn_wait', status: 'completed', closeKind: 'waiting' }),
run({
id: 'rn_bob',
agentId: BOB.id,
status: 'running',
queueSequence: 101,
}),
],
});

const closed = await closeRun(PROJECT_ROOT, {
threadId: thread.id,
runId: 'rn_bob',
agentId: BOB.id,
request: { kind: 'review', summary: 'the flake is the retry path' },
});
expect(
closed.thread.runs.find((entry) => entry.id === 'rn_wait')
?.closeAcknowledgedAtSequence,
).toBe(1);

const finished = await finish(thread.id, 'rn_bob', { status: 'completed' });
expect(finished.status).toBe('in_review');
});

it('records a clean exit with no closing tool as unclosed and blocks', async () => {
const thread = await seed();

const finished = await finish(thread.id, 'rn_alice', {
status: 'completed',
});

expect(finished.runs[0]?.closeKind).toBe('unclosed');
expect(finished.status).toBe('blocked');
expect(
finished.outbox.some(
(event) => event.payload['event'] === 'thread_blocked',
),
).toBe(true);
});

it('reports a child in review to its parent exactly once', async () => {
const parent = await createThread(PROJECT_ROOT, { title: 'parent' });
const created = await createThread(PROJECT_ROOT, {
title: 'child',
parentThreadId: parent.id,
});
await writeThread(PROJECT_ROOT, {
...created,
status: 'in_progress',
runs: [run()],
});

await closeRun(PROJECT_ROOT, {
threadId: created.id,
runId: 'rn_alice',
agentId: ALICE.id,
request: { kind: 'review', summary: 'root cause found' },
});
const finished = await finish(created.id, 'rn_alice', {
status: 'completed',
});

expect(finished.status).toBe('in_review');
const reports = finished.outbox.filter(
(event) => event.kind === 'parent_report',
);
expect(reports).toHaveLength(1);
expect(reports[0]?.payload['parentThreadId']).toBe(parent.id);

// Re-running the terminal write must not enqueue a second report.
const again = await finish(created.id, 'rn_alice', { status: 'completed' });
Comment on lines +245 to +246

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-26: finishRunInTransaction's terminal-state precondition — the guard its docblock says exists "so a late completion cannot overwrite a cancellation" — has no test. The suite's only re-entry is completedcompleted, which passes through the applyAggregateStatus early return and never touches the guard.

Every finishRunInTransaction caller in tests is the finish() helper, always with completed or failed against a running or finishing run; no test seeds cancelled or cancelling. Deleting the four-status precondition keeps all tests green. What ships unguarded: a person cancels a run (status:'cancelled', closeKind undefined), the runtime's late callback reports {status:'completed'}, and the record flips to completed with closeKind defaulted to 'unclosed'. 'unclosed' is in BLOCKING_KINDS, so the thread is persisted blocked with reason "run X ended without a hand-off" and a thread_blocked notification wakes a human about a run they deliberately cancelled — the cancellation is unrecoverable from the record, precisely the lie the two-write split exists to prevent.

Witness:

Mutation arm — drop the four-status precondition at `run-lifecycle.ts:379-383`: the suite stays green, while the added cancelled-run case shows the run flipped to `completed` with `closeKind: 'unclosed'`, the thread persisted `blocked`, and a `thread_blocked` event enqueued for a deliberately cancelled run. The baseline arm separately reproduced the opposite, already-reported gap (`cancelled` leaving no obligation at all).

Suggested fix: Add a test seeding a run with status:'cancelled', calling finish(thread.id, runId, {status:'completed'}), and asserting the persisted run is still cancelled with closeKind undefined and the thread status unchanged — plus a cancellingcompleted case asserting the transition is applied, pinning the guard's boundary on both sides. Distinct from the open Critical 2 on this PR, which is the opposite direction.

The fix has to respect this: thread-status.ts:70-76BLOCKING_KINDS is ['blocked','failure','unclosed'], read through the closeKind: run.closeKind ?? (input.outcome.status === 'completed' ? 'unclosed' : undefined) default at run-lifecycle.ts:386-389. The test must assert closeKind stays undefined, not merely that status stays 'cancelled'.

Acceptance criterion: The new cancelled-run case goes red when the four-status precondition is dropped, 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(again.outbox.filter((e) => e.kind === 'parent_report')).toHaveLength(
1,
);
});

it('carries a typed failure stage onto the run and blocks the thread', async () => {
const thread = await seed();

const finished = await finish(thread.id, 'rn_alice', {
status: 'failed',
error: 'definition missing',
failureStage: 'launch',
Comment on lines +255 to +258

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-36: The only launch-failure test seeds the run in a state a launch failure cannot be in (running), so the run.status === 'queued' disjunct of the terminal-write guard — the state a real failureStage: 'launch' is recorded against — is exercised by none of the 12 tests.

In production a launch failure is recorded against a run that is still queued: the plan's dispatcher reaches failed → terminal failed(failureStage=launch); release queue slot on the branch parallel to accepted → startRun, and startRun is the only queued → running transition. With the disjunct gone, step 6's launch_failed handler writes nothing — the run keeps status: 'queued' with no endedAt, no error, no failureStage; LIVE_RUN_STATUSES counts queued as live, so the resolver's live.length > 0 branch returns in_progress before any blocking obligation is consulted; the thread persists in_progress forever, no thread_blocked event is enqueued, the queue slot is never released, and no human is told. The same disjunct is load-bearing for the dispatcher's cancelled row.

Witness:

Intact baseline `Tests 100 passed (100)`; **mutant M1** (`run.status === 'queued' ||` deleted from `run-lifecycle.ts:379`) → `Test Files 9 passed (9) / Tests 100 passed (100)`. Adding the suggested `queued`-seeded case: under M1 → `FAIL … expected { runStatus: 'queued', … } to deeply equal { runStatus: 'failed', … }` with `blockedEvents: 0, endedAt: undefined, failureStage: undefined, persistedStatus: 'in_progress'`; against intact source → `✓ 1 passed`. Premise confirmed: `thread-actions.ts:371-378` is the only `queued → running` transition (one production `status: 'running'` hit module-wide) and runs are born `queued` at `:283`. Distinctness from R1-26 executed: M2 (delete the whole status guard) → `Tests 101 passed`, and the new `queued` case **passes** under M2 while failing under M1 — two mutations, two tests, neither fix satisfies the other.

Suggested fix: Seed the launch-failure case in the state production reaches it in — const thread = await seed({ runs: [run({ status: 'queued' })] }); — then assert runs[0].status === 'failed', runs[0].endedAt is a finite number, thread status === 'blocked', and that readThread agrees. Keep the existing running-fixture case as a second it() so both accepted states are pinned.

The fix has to respect this: The guard's accepted set must stay a superset of LIVE_RUN_STATUSES (thread-status.ts:42-47) — any live status the terminal write refuses is a status the thread can never leave, because resolveThreadStatus returns in_progress on live.length > 0 before consulting obligations (thread-status.ts:167-174).

Acceptance criterion: That case goes red when run.status === 'queued' || is deleted from run-lifecycle.ts:379; today all 12 stay green. 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(finished.runs[0]?.failureStage).toBe('launch');
expect(finished.status).toBe('blocked');
Comment on lines +261 to +262

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-35: Two tests pass outcome.error into the terminal write and neither asserts it lands on the run, so the only human-readable failure diagnosis this diff persists is deletable with the suite green — while the same test pins failureStage, the field the PR's own review record already calls a dead switch.

The run validator makes error optional, so deleting the conditional spread at run-lifecycle.ts:392 still validates and still writes. Neither test reads .error back; the thread_blocked assertions use .some(payload['event'] === …) without touching reason, and resolveThreadStatus has no .error read. Deleting the spread keeps all tests green while a failed run persists with no error text: the thread is stamped blocked, a human is paged, and the record they inspect — and the failure notification step 6 builds from it — carries no reason at all, only the typed stage. launcher.ts:28-29 is the producer that will supply that string ({ status: 'launch_failed'; error: string }), so the value is real.

Witness:

`MUTANT B delete run-lifecycle.ts:392 ...(input.outcome.error ? { error: input.outcome.error } : {}): Test Files 9 passed (9) Tests 100 passed (100)`; `MUTANT B + suggested assertion: AssertionError: expected undefined to be 'definition missing'; × carries a typed failure stage onto the run and blocks the thread; Test Files 1 failed (1) Tests 1 failed | 11 passed (12)`; `MUTANT B reverted, assertion kept: Tests 12 passed (12)`. Scope settled from base: the write is pre-existing (`git show e0cef4577f:…thread-actions.ts:371` has the same spread) and no test at base called `finishRun` at all — what is new and in scope is `run-lifecycle.ts:392` and the new suite that pins the neighbour `failureStage` while never reading `.error`. Sweep: `failureStage` has **no producer** anywhere in `packages/core/src`; `error` has one (`launcher.ts:28-29`, returned at `:51` and `:99-100`).

Suggested fix: Add expect(finished.runs[0]?.error).toBe('definition missing'); to the failure-stage test and the equivalent in the unclosed/failed case.

The fix has to respect this: mesh-store.ts:307(value['error'] === undefined || typeof value['error'] === 'string'), so the field is optional and an absent value validates: the mutation is silent. Cost is bounded — no code reads run.error today, so the loss is in the persisted record a human inspects and in the step-6 failure notification, which keeps this a Suggestion.

Acceptance criterion: The new error assertion goes red when the conditional spread at run-lifecycle.ts:392 is removed, and green when restored. 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 any close on a thread a person already marked done', async () => {
const thread = await seed({ status: 'done' });

await expect(
closeRun(PROJECT_ROOT, {
threadId: thread.id,
runId: 'rn_alice',
agentId: ALICE.id,
request: { kind: 'review', summary: 'late' },
}),
).rejects.toThrow(/is done/);
});

it('clears an obsolete failure when a later post books real work', async () => {
const thread = await seed({ assigneeAgentId: ALICE.id });
const failed = await finish(thread.id, 'rn_alice', {
status: 'failed',
error: 'launch failed',
});
expect(failed.status).toBe('blocked');

const posted = await postMessage(PROJECT_ROOT, thread.id, {
from: HUMAN_AUTHOR_ID,
text: 'try again please',
});
Comment on lines +286 to +289

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] R1-10: The only test offered as the producer-side witness for I2 posts as a human, but I2's defect is precisely that acknowledgement happened only by human feedback — so this test does not discriminate the fix from the defect it claims to pin.

Gate the new admission call on authorship (if (input.from === HUMAN_AUTHOR_ID && …)) and all tests stay green: no test posts from an agent id and asserts closeAcknowledgedAtSequence. The PR description's I2 claim — "postMessage now discharges outstanding obligations when it actually books work" — would then be false for every agent-authored booking, which is the exact case I2's remedy added over the old rule, and step 6's dispatcher (which books agent-triggered posts) would build on it as-is.

Witness:

`MUTATED (authorship gate) Test Files 9 passed (9) / Tests 100 passed (100)`; `NEW PROBE (post from BOB.id '@alice retry the launch' after rn_alice failed): mutated: expected undefined to be 1 (rn_alice.closeAcknowledgedAtSequence vs message.sequence); intact: Tests 1 passed (1)`.

Suggested fix: Add one case that books work as an agent: after a failed run, postMessage(PROJECT_ROOT, thread.id, { from: BOB.id, text: '@alice retry the launch' }), then assert posted.dispatched has length 1, rn_alice.closeAcknowledgedAtSequence === posted.message.sequence, and posted.thread.status === 'in_progress'.

The fix has to respect this: The design's aggregate paragraph (docs/plans/2026-09-06-multi-agent-board-collaboration.md:481-486) is what makes agent-authored booking the case that matters; a human-only witness cannot pin it.

Acceptance criterion: That new case goes red if the acknowledgement is gated on input.from === HUMAN_AUTHOR_ID; 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)


expect(posted.dispatched).toHaveLength(1);
expect(posted.thread.status).toBe('in_progress');
expect(
posted.thread.runs.find((entry) => entry.id === 'rn_alice')
?.closeAcknowledgedAtSequence,
).toBe(1);
});

it('blocks a quiescent thread whose post books nothing at all', async () => {
const created = await createThread(PROJECT_ROOT, { title: 'unassigned' });

const posted = await postMessage(PROJECT_ROOT, created.id, {
from: HUMAN_AUTHOR_ID,
text: 'anyone?',
});

expect(posted.dispatched).toHaveLength(0);
expect(posted.thread.status).toBe('blocked');
expect(
posted.thread.outbox.some(
(event) => event.payload['event'] === 'thread_blocked',
),
).toBe(true);
});

it('leaves a thread in_progress while another run is still live', async () => {
const thread = await seed({
runs: [
run(),
run({
id: 'rn_bob',
agentId: BOB.id,
status: 'queued',
queueSequence: 101,
}),
],
});

await closeRun(PROJECT_ROOT, {
threadId: thread.id,
runId: 'rn_alice',
agentId: ALICE.id,
request: { kind: 'review', summary: 'my part is done' },
});
const finished = await finish(thread.id, 'rn_alice', {
status: 'completed',
});

expect(finished.status).toBe('in_progress');
expect(await readThread(PROJECT_ROOT, thread.id)).toMatchObject({
status: 'in_progress',
});
});
});
Loading