test(core): characterize leader delivery and terminal state per host class - #14855

APPROVE — I ran this at head 670e8e9d and it is green: 3/3 in ~4.0 s (wired 1337 ms, listener-only 1352 ms, bare 1350 ms).

I checked every symbol the file touches against the head rather than trusting the diff: TeamCoordinationHarness.create/cleanup/spawnTeammate and its teamManager / tmpDir fields; TeamManager.setLeaderMessageCallback (1071), getEventEmitter (1587), getLeaderMessages (995), sendMessage (784) and the private pollingInterval (279); TeamEventType.ALL_TEAMMATES_TERMINATED / TEAMMATE_APPROVAL_REQUEST; AgentStatus.COMPLETED. All present with the shapes the test assumes.

The framing is the part I most want to say is right. Pinning today while carrying an unasserted afterM4 column, and saying so in the header, means the diff that lands M4 flips rows at row level instead of a reviewer re-deriving which host class regressed. terminated being probed with a subscription in every class — plus the note that the single emit site sits after the null-sink guard in pollLeaderInbox, so subscribing cannot help a sinkless host — is exactly the distinction that makes the three rows non-redundant rather than three copies of one observation.

Two non-blocking notes:

• POLL_WINDOW_MS = 1300 is a wall-clock margin, and it is the one flake surface here. The wired row needs delivered: 2 and pollArmed: false, i.e. both 500 ms polls must have fired and the interval must have been cleared inside 1300 ms — a 300 ms margin. The sinkless rows are safe in the direction that matters (pollArmed: true only becomes more true with delay), so the exposure is one-sided. On a loaded runner a delayed setInterval callback samples delivered early and the row fails on scheduling, not on behavior — the class CONTRIBUTING rules out. Worth considering driving the poll with fake timers, or asserting on a condition that settles rather than on an elapsed window. Not a blocker: it passes deterministically here and the window is documented.
• pollArmed reaches the private pollingInterval through a cast, so an unrelated rename breaks this test without any behavior change. Given the file's stated purpose is to pin today's internals, I read that as deliberate rather than a smell — flagging only so the coupling is a known cost when M4 lands.

CI at this head had not reported yet (only label concluded; precheck-pr, Classify PR and the tier selection were still queued). That is not a code finding and does not affect this verdict — my approval rests on the local run above.

EN

Read the file at 9b26e7b11 against the head source rather than the diff, and the framing holds up: the
single ALL_TEAMMATES_TERMINATED emit site is behind the null-sink guard in pollLeaderInbox, so
subscribing cannot rescue a sinkless host, and the three rows are measuring three real things. The host
split is also still accurate — the only leader sinks at this head are nonInteractiveCli.ts:1003 and
use-llm-stream.ts:6891. Notes below are all non-blocking.

• TeamManager.host-class.characterization.test.ts:123-127,153-155: delivered is derived only from
  sinkCalls, which only the wired branch populates, so today.delivered: 0 on the listener-only
  and bare rows (99, 104) can never fail. The knock-on effect is on the target column: afterM4.delivered: 2
  (100, 105) is unreachable while observe() attaches no sink for those classes, so a literal
  today → afterM4 flip by the M4 diff would stay red on that cell no matter what converged. Dropping
  delivered from the sinkless rows, or saying in the header that it means "no row to observe" there,
  would keep the column usable.
• :85-90: the afterM4 sinkless target reads as delivery parity (delivered: 2, unread: 0), whereas
  the M4 acceptance in #13785 is that those hosts never see team tools at all. The comment offers both
  readings and then records the second; worth pinning one.
• :66,146: the wall-clock window is still the one flake surface, and the budget covers more of the row
  than the earlier note named — terminated: 1 is emitted inside the same second poll
  (TeamManager.ts:1230-1233), so a runner that services the 1000 ms tick after the 1300 ms timeout fails
  terminated and pollArmed, and one that slips both ticks fails all four cells. The overlap direction
  (two empty-inbox polls both emitting) does not look reachable inside the window, so the exposure stays
  one-sided. Fake timers, or asserting on a settling condition, would remove it.
• Description: the two setLeaderMessageCallback coordinates read nonInteractiveCli.ts:1047 /
  use-llm-stream.ts:6777; at this head they are :1003 and :6891 (:985 detaches). Only affects a
  reader verifying the host split.

新的一行测试配着 head 的源码读了一遍，而不是只看 diff，框架是站得住脚的：ALL_TEAMMATES_TERMINATED
唯一的 emit 点就在 pollLeaderInbox 的 null-sink guard 之后，订阅救不了 sinkless 宿主，三行确实在测
三件不同的事。宿主分类在这个 head 上也仍然准确——只有 nonInteractiveCli.ts:1003 和
use-llm-stream.ts:6891 挂了 leader sink。以下均为不阻塞的说明。

• TeamManager.host-class.characterization.test.ts:123-127,153-155：delivered 只由 sinkCalls 推导，
  而 sinkCalls 只有 wired 分支会写入，所以 listener-only 与 bare 两行的 today.delivered: 0
  （99、104）永远不会失败。连带影响在目标列：在 observe() 不为这两类挂 sink 的前提下，
  afterM4.delivered: 2（100、105）是不可达的——M4 的 diff 若照字面把 today 翻成 afterM4，
  这一格会一直红，与实际是否收敛无关。建议把 delivered 从 sinkless 两行去掉，或在头部注明该处
  意为「没有行可观察」。
• :85-90：afterM4 的 sinkless 目标写成了与 wired 同等的投递（delivered: 2, unread: 0），
  而 #13785 里 M4 的验收是这些宿主根本不注册 team 工具。注释里两种读法都提到了，最后落的是第二种，
  建议只保留一种。
• :66,146：wall-clock 窗口仍是唯一的抖动面，而且它覆盖的格子比之前那条说明更多——terminated: 1 是在
  同一个第二次轮询里 emit 的（TeamManager.ts:1230-1233），所以 1000 ms 的 tick 若被拖到 1300 ms
  超时之后才被调度，失败的是 terminated 与 pollArmed；若两个 tick 都被拖过 1300 ms，四格全红。
  反方向（两个空收件箱轮询各自 emit）在窗口内看起来不可达，所以暴露仍是单向的。换成 fake timers，
  或改为断言一个会稳定的条件，就能消掉。
• PR 描述：两处 setLeaderMessageCallback 坐标写的是 nonInteractiveCli.ts:1047 /
  use-llm-stream.ts:6777，当前 head 是 :1003 与 :6891（:985 是 detach）。只影响按描述核对
  宿主分类的读者。

APPROVE at head 5dcf6502.

I ran this file on a worktree pinned to 5dcf6502 (packages/core, scoped vitest config, no globalSetup): baseline 3 passed (3), test body 86-98 ms, about 3.3 s wall.

Both mutation directions are load-bearing, and each falsifies a different row:

• Null-sink guard in pollLeaderInbox removed, delivery turned into callback?.(...): 2 failed | 1 passed — listener-only and bare go red on unread: 0 vs 2, wired stays green. That is the M4 shape, so the two sinkless rows are precisely the ones M4 has to move.
• allTeammatesTerminated() returning false: 1 failed | 2 passed — only wired goes red, and it burns the full 10 s settle ceiling doing it.

Both restored; git status clean apart from my probe config.

All four of @doudouOUC's notes are addressed at this head, not only in prose: delivered is null on the sinkless rows so the cell can no longer sit green by construction, afterM4 is pinned to the single NOT_ADVERTISED reading, POLL_WINDOW_MS is gone in favour of faking only setInterval/clearInterval and stepping the ticks, and the sink coordinates in the description match head (nonInteractiveCli.ts:1003, use-llm-stream.ts:6891, :985 detaches).

I also re-checked the framing claims against head rather than the diff: ALL_TEAMMATES_TERMINATED has exactly one .emit site (TeamManager.ts:1231) and it sits after the guard, so subscribing cannot rescue a sinkless host; setLeaderMessageCallback has exactly two attach sites in the repo, which is what keeps the three host classes honest.

Two non-blocking notes:

• settle() is called twice on the wired row, each with its own 10 s deadline. If a future breakage makes both conditions unreachable the row spends about 20 s, which is past packages/core's 15 s testTimeout on GitHub-hosted runners (60 s on ecs-qwen-*), so the failure would surface as a vitest timeout instead of the row diff that makes this file useful. Sharing one deadline across both settles would keep the diagnostic. Mutation B above is the single-ceiling case, and it did still report the assertion.
• pollArmed reads the private pollingInterval through a cast. For a characterization file that is the point rather than a smell — flagging only so the coupling is a known cost when M4 lands. No accessor needed on my account.

CI at this head: Integration Tests (no-AK), Managed Agent gate, OpenTUI no-flicker gate and TUI parity snapshots green; Lint & Static and Test (ubuntu-latest) were still in progress when I wrote this. That is not a code finding and does not affect the verdict, which rests on the local runs above.

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and their suites did not run locally.

Not reviewed: reverse audit — stopped before round 4 by the review time budget.

— qwen3.8-max via Qwen Code /review (v0.25.3)

Delta re-review at 9328afa4. My approval on 5dcf6502 was dismissed by the push, so this is a fresh vote on the R1-1..R1-5 revision rather than a re-run of the earlier pass.

Each revision checked against the production source at this head:

• R1-2 — drainLeaderInbox() (TeamManager.ts:1208) is a bare await this.pollLeaderInbox(), and the terminal branch (:1228-1233) emits ALL_TEAMMATES_TERMINATED guarded only by allTeammatesTerminated(), not by "polling was armed". So the post-stop drain genuinely re-emits, and terminated: 2 on the wired row is today's behaviour rather than an aspirational number. The comment at :207-209 says exactly that.
• R1-3 — getInboxesDir() (teamHelpers.ts:62) resolves through getTeamDir() → getTeamsRootDir() (:38-39) → Storage.getGlobalQwenDir(), i.e. the mocked static. The harness builds its own tree straight off os.tmpdir() (coordination-harness.ts:70-78), so the new startsWith(h.tmpDir) pin is what ties the manager's resolver to the harness root — without it the mailbox I/O could quietly walk off into a real ~/.qwen/teams.
• R1-4 — the sink is two-channel (:1071-1077), and with both reports consumed in one batch formatLeaderDisplay takes the multi-message branch (:1331-1333), deduping to **worker** reported back. The single-element displayed array pins the batching shape as a side effect, which is worth having.
• R1-5 — both <team_error> producers (:745-748 and :1720-1721) write the envelope into the model channel, which is the one the new filter reads, so the assertion can actually see them.
• R1-1 — the scope rewrite is accurate: config.ts:13268-13269 gates registerLazy(ToolNames.TEAM_CREATE) behind isAgentTeamEnabled(), upstream of a directly-constructed TeamManager. Dropping afterM4 / NOT_ADVERTISED removes data that was never asserted (observe() had no path to produce that sentinel), so no coverage was lost.

Mutation-tested rather than taken on trust. Ran the file locally at this head (scoped vitest config, no globalSetup): baseline 3 passed (3) in 82ms, then each mutant, then restored.

Every one goes red on exactly the rows it claims to pin, and the restored tree is green again — so all five revisions are load-bearing and the original M4 witness still holds at this head.

Non-blocking, carried forward from my earlier pass: settle() builds its own 10s deadline per call (:147-152) and the wired row calls it twice (:198, :202). If both conditions ever became unreachable that row would burn ~20s, over the 15s testTimeout that packages/core/vitest.config.ts:17-19 uses on non-ecs-qwen-* runners (60s there). It would then surface as a vitest timeout and lose the row-level diff that makes this test worth reading. Sharing one deadline across both settles would fix it. Not a merge blocker.

CI, for accuracy rather than as a code finding: Lint & Static is red at this head, but it dies at step 10 "Check lint gate freshness" before any linter runs — ESLint and Prettier were all skipped. The message is that .github/workflows/ci.yml moved on main (d6abebb4, #15095) after this branch last incorporated it, so the lane wants a main merge or rebase to re-validate under the current gate. Nothing in this diff caused it. Test (ubuntu-latest, Node 22.x) was still in progress when I submitted, so I'm not claiming a green test lane.

fleet-test-docs-review:14855:9328afa4c1c72971981664bc406782fa45812568

Partially reviewed — gaps disclosed. Suggestions are inline.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

• R2-1 settle()'s 10s deadline vs the 15s testTimeout — already reported (@yiliang114, review 5620152715; also review 5606167639)

Not reviewed: verification — both verifier shards were aborted by the workflow wall-clock cap, so no finding received a verdict.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and their suites did not run locally.

Not reviewed: reverse audit — stopped before round 1 by the review time budget.

⚠️ 13 finding(s) still carried the — [unverified] tag when the loop ended — the verifier never ruled on them, and they are not confirmed.

Test Plan (not a blocker): Tests 3 passed — this review observed 38398, 2815, 44842, 1028, 2279, 642, 13184 passed; 3 passed — this review observed 38398, 2815, 44842, 1028, 2279, 642, 13184 passed; 1 passed — this review observed 38398, 2815, 44842, 1028, 2279, 642, 13184 passed; 2 passed — this review observed 38398, 2815, 44842, 1028, 2279, 642, 13184 passed.

— qwen3.8-max via Qwen Code /review (v0.26.0)