fix(core): remove ghost members from persisted team roster after failed concurrent spawn - #10223
Open
yiliang114 wants to merge 13 commits into
Open
fix(core): remove ghost members from persisted team roster after failed concurrent spawn#10223yiliang114 wants to merge 13 commits into
yiliang114 wants to merge 13 commits into
Conversation
…am roster When a concurrent teammate spawn fails after another spawn has already persisted the shared members array, the failed member remains in config.json as a ghost entry. Add a compensating writeTeamFile() on the rollback path so persisted membership matches committed membership. Fixes #10208 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
requested review from
LaZzyMan,
doudouOUC,
qqqys,
tanzhenxin and
wenshao
as code owners
August 27, 2026 01:54
Collaborator
Collaborator
|
Re-run after three fix commits — gate re-checked on Template looks good ✓ Problem: unchanged — observed bug with evidence. #10208 documents the source-confirmed interleaving, and the earlier sandboxed verification's base cell reproduced the failure byte-for-byte ( Direction: unchanged — a data-consistency fix in the Agent Team subsystem ( Size: core paths touched. Production logic is 105 lines in Approach: the three new commits do exactly what the outstanding review findings asked — nothing more. Snapshot the roster synchronously at the counted write start (closes the one-mkdir-await window the gate left open; that window was my non-blocking observation last round, got promoted to the Critical finding R2-6 by the review job, and the author picked the airtight of the two options I named), reuse Risk: no elevated risk signals (no high-risk path matches). Moving on to code review. 🔍 中文说明三个修复提交后的 re-run——在 模板完整 ✓ 问题：结论不变——已观测到的 bug，有证据。#10208 记录了源码确认的交错，此前沙箱验证的 base 单元逐字节复现了失败（ 方向：结论不变——Agent Team 子系统的数据一致性修复（ 规模：触及核心路径。生产代码为 方案：三个新提交恰好只做了未决评审意见要求的事。在计数写入起点同步快照 roster（堵上了门控留下的一个 mkdir await 宽度的窗口——该窗口是我上一轮的非阻塞观察，被评审任务升级为 Critical 发现 R2-6，作者选择了我点名的两个方案中更严密的那个）；用 风险：无升级风险信号（未命中高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewRe-reviewed at the current head. The delta since the last reviewed head (
Everything else from the last pass carries over byte-identical: queue ordering, gate semantics, leader-notice shape, the five original regression tests. Standing non-blocking note: declining the counter-decrement Suggestion was the right call. The author's interleaving (A's write rejects → decrement → C's write reuses the watermark → gate skips a needed repair) provably re-persists the #10208 ghost, and commit-aware gating is a counter redesign — tracked in #10297 rather than bolted onto this fix. Testing evidenceThe unit suite was still running on this head at review time — no polling per triage rules; the table below auto-updates via the finalize job when CI lands. Everything else is green: Secret scan, Dependency CVE audit, both Desktop Shell matrix cells, Classify PR. The skipped cells are workflow design (merge_group-gated), same as prior rounds. Per triage rules I did not run the PR's tests myself; the evidence is the PR's own CI, read via the API. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification is already in flight for this head: the run triggered by this same Not verified: live TUI behavior — nothing user-visible changes in this PR (internal roster-persistence mechanics), so N/A for real-scenario testing. The author's "7/7 green + typecheck clean" is their claim; the CI table above is the evidence. 中文说明代码审查在当前 head 复审。自上一个已审 head（
上一轮通过的其余部分逐字节未变：队列顺序、门控语义、leader 通知形态、原有五个回归测试。 延续的非阻塞说明：拒绝计数器递减的 Suggestion 是正确的。作者给出的交错（A 的写入被拒 → 递减 → C 的写入复用水位 → 门控跳过本应执行的修复）可证明会重新持久化 #10208 的 ghost；提交感知的门控属于计数器机制的重新设计——已记录在 #10297，而不是硬塞进本修复。 测试证据审查时本 head 上的单元测试套件仍在运行——按规则不轮询；下表由 finalize 任务在 CI 落定后自动更新。其余均为绿色：Secret scan、依赖 CVE 审计、两个 Desktop Shell 矩阵、Classify PR。skipped 项为工作流设计（merge_group 门控），与之前几轮一致。按规则我没有亲自运行 PR 的测试；以上证据是 PR 自己的 CI，通过 API 读取。 沙箱验证已在本 head 上运行中：由本条 未验证：真实 TUI 行为——本 PR 没有用户可见的变化（内部 roster 持久化机制），真实场景测试不适用。作者自述的 "7/7 green + typecheck clean" 是其声明；上方 CI 表格才是证据。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — code side fully converged: the Critical from the last review round is fixed and pinned by a regression test, the remaining Suggestions are fixed or legitimately declined with a follow-up filed, and the delta is minimal. The only open items are the unit suite still running on this head and the in-flight sandboxed verification. This is what the loop is supposed to look like when it converges. My independent proposal for #10208 from the first pass was compensating write plus serialized roster writes — the PR landed exactly that, and each review round since narrowed the ghost's hiding places: round 1 caught the unsynchronized writer, round 2 caught the compensating write serializing the live roster, round 3 caught the one-mkdir-await window the gate left open. This round's three commits close those last findings without touching anything they didn't need to — the snapshot is taken at exactly the counted point, the leader notice reuses the house error utility, and each fix has a test that would fail if the fix were reverted or weakened. The declined Suggestion is the right kind of decline: the proposed counter decrement provably re-opens the bug (the author wrote out the interleaving), and the redesign lives in #10297 instead of bloating this PR. If I'm maintaining this in six months, the queue/counter/snapshot mechanism reads as one coherent invariant with a test naming each hole it plugs. The unit suite has not finished on this head, so I won't attest to a result that doesn't exist yet: the approval is deferred until CI lands green on the reviewed commit — the finalize job posts it pinned to exactly that commit, which also supersedes this bot's round-3 Approval deferred until CI lands green on the reviewed commit. 中文说明置信度：4/5 —— 代码层面已完全收敛：上一轮评审的 Critical 已修复并被回归测试钉住，其余 Suggestion 或已修复、或附有证明地拒绝并开了后续 issue，增量改动极小。唯一未决的是本 head 上仍在运行的单元测试套件和进行中的沙箱验证。 这是审查循环收敛时应有的样子。我对 #10208 的独立方案从第一轮起就是补偿写入加串行化 roster 写入——PR 落地的正是这个方案，此后每一轮评审都在收窄 ghost 的藏身之处：第一轮抓到未同步的写者，第二轮抓到补偿写入序列化 live roster，第三轮抓到门控留下的一个 mkdir await 窗口。本轮三个提交闭合了最后这些发现，且没有碰任何不需要碰的东西——快照恰好取在计数点、leader 通知复用既有错误工具、每个修复都有一个"回退或削弱即失败"的测试。被拒绝的 Suggestion 是正确的拒绝方式：提议的计数器递减可证明会重新打开该 bug（作者写出了完整交错），重新设计放在 #10297 而不是塞进本 PR。六个月后维护这段代码时，队列/计数器/快照机制读起来是一个完整不变量，每个测试命名了它所堵住的一个漏洞。 本 head 上的单元测试套件尚未结束，我不会在证据存在之前批准：批准推迟到 CI 在被审提交上落绿——finalize 任务会发布批准并精确钉住该提交，同时取代本 bot 第三轮的 批准推迟到 CI 在被审提交上落绿。 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Verification report (head
|
qwen-code-ci-bot
requested changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…e window Route both the success-path write and the failed-spawn compensating write through a per-TeamManager promise-chain queue (persistTeamFile). Each queued task serializes the live roster when it runs, so commits land in call order and a stale snapshot can no longer out-commit a later compensating write and re-persist a removed member (#10208). Also log via debug.warn when the best-effort compensating write itself fails, so a resurfaced ghost member is diagnosable. Co-authored-by: Qwen-Coder <qwen-coder@alibaba-cloud.com>
… failure - Add an out-of-order completion case: hold the first (stale) roster write until the failed spawn's compensating write is queued, then assert the persisted roster excludes the rolled-back member. Goes red without the persistTeamFile queue. - Add a case where the compensating write throws, asserting spawnTeammate still rejects with the original spawn error and the member is rolled back from in-memory state. - Fix the stale comment describing the pre-fix behaviour. Co-authored-by: Qwen-Coder <qwen-coder@alibaba-cloud.com>
Collaborator
Author
|
Review closeout (round 1 — 4 of 5 findings addressed) Changed:
Verified: all 4 tests in Intentionally not changed this round:
CI note: the one failing check ( |
Picks up #10220's getToolRegistry mock fix, which is what the 9 failing client.telemetrySwap tests on this branch were missing. The branch was cut before that landed, so the failure was inherited, not introduced.
#10220 and #9729 independently gave this suite's config mock a `getToolRegistry`, and merging them produced a clean Git merge and a broken object literal: the same key twice, which is TS1117 ("An object literal cannot have multiple properties with the same name"). PR CI does not catch it — it runs only `typecheck:integration`, not the workspace `npm run typecheck` — so it fails for the next person to run preflight rather than in a check anyone is watching. Keep #10220's copy: its comment names the call that needs the registry and the error an empty one avoids.
Replace the hand-rolled mkdtemp / teams-dir / TeamFile fixture / FakeBackend().init() / new TeamManager(...) setup in all four ghost-member regression tests with TeamCoordinationHarness.create(), matching the sibling TeamManager.plan-approval.test.ts. Assertions are unchanged; the deferred-spawn interception now targets h.backend, and teardown uses harness.cleanup(). This keeps the fixture shape in sync with the centrally maintained harness as TeamFile evolves. The triplicated Storage vi.mock factory extraction suggested in review is intentionally deferred: it spans 11+ test files and is out of scope for this PR. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 25 passed · 0 failed · 25 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：25 通过 · 0 失败 · 25 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10223 verification — fix(core): remove ghost members from persisted team roster after failed concurrent spawnVerdict: 中文摘要
Central claim + A/BCentral claim: when a concurrent teammate spawn fails after another spawn has already serialized the shared members array, the persisted Cells (same runner vitest 3.2.7, same scratch vitest config, same nested
Witnesses: Both halves are load-bearing, and they defend different doors of the same room: (A) fixes the content (a failed spawn must trigger a rewrite), (B) fixes the order (a stale snapshot must not out-commit the rewrite). The base cell proves the pair; the intermediate cells separate the halves. Mutation matrix (head, ghost test file unless noted)
Combination row (both halves removed) is the base cell: red. Queue-poison probe (rejected write must not poison the chain — the Reviewer Test Plan walk-through
FindingsF1 (Suggestion) — the PR's serialization test is a probabilistic witness of the queue mutation
Reproducing commands: # 10x on the no-queue mutant (reconstructed in a scratch worktree):
npx vitest run src/agents/team/TeamManager.ghost-member.test.ts -t "write serialization" ... # 5/10 red
# deterministic witness (artifact: queue-witness-probe.test.ts): red on mutant, green on headMinimal suggested fix (measured, not eyeballed)Adopt the probe's release protocol in test 3: after F2 (Nit) — orphaned JSDoc:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Move persistTeamFile (with its own JSDoc) above the spawnTeammate doc block so the "Spawn a new teammate..." JSDoc is no longer orphaned between the two and each comment stays attached to the method it describes. Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Keep the debug.warn but also deliver a concise <team_error> notice through leaderMessageCallback (guarded by try/catch), mirroring the fireAndForget pattern, since debug logging alone is invisible in production. Also extract a single gateSpawns helper for the three pasted spawn-gating wrappers in the ghost-member suite, and extend the compensating-write-failure test to witness both the leader notification and that a rejected write does not poison the queue (a follow-up spawn persists normally afterwards). Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
…rites The compensating write serializes the live roster, which can still contain sibling members whose own spawnTeammate is pending. When the failed spawn is the first write to land, that persisted the in-flight sibling — a ghost member if the process exits before the sibling resolves, the #10208 symptom in an interleaving that wrote nothing before this change. Track roster writes that have started (their snapshot point) in `teamFileWritesStarted`, capture the counter at member push, and skip the compensating write when no write could have persisted the failed member. Writes that started inside the member's window still trigger it, preserving the repair semantics. Witness test: concurrent alpha/beta spawns with deferred gates, reject alpha before any write lands, assert no roster write runs and the persisted roster stays empty; removing the gate turns the test red. The compensating-write-failure test switches to concurrent spawns so its compensating write stays necessary under the gate. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: Skipped because the PR has merge conflicts, so refs/pull/10223/merge is unavailable — resolve conflicts and re-run. 中文 — 判定：
|
# Conflicts: # packages/core/src/core/client.telemetrySwap.test.ts
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 24 passed · 0 failed · 24 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：24 通过 · 0 失败 · 24 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10223 verification (follow-up round 2) — fix(core): remove ghost members from persisted team roster after failed concurrent spawnVerdict: 中文 — 判定：`findings`（发现一个值得评审关注的新问题）
Previous-finding status (follow-up round)
No declined or deferred rows existed in the previous report. Central claim + A/B (re-measured at the new head)Central claim: when a concurrent teammate spawn fails after another spawn has already serialized the shared members array, the persisted Cells (same vitest 3.2.7, identical scratch config in each worktree; PR leaves
Witness: Delta probes (new mechanisms since the previous round)
Witness: Mutation matrix (new head; every row executed, logs in
|
| mutant | oracle | result | classification |
|---|---|---|---|
| M1: entire compensating-write block removed | suite + G1 | killed — t1, t3, t4 red; G1 red (m1-no-comp.log) |
(A) pinned by 3 of 5 tests + probe |
| M2: queue removed (counter + rejection propagation kept) | G2 witness | killed 1/1 — landed=[comp:alpha | stale:alpha,beta] final=["alpha","beta"] (m2-no-queue.log) |
(B) load-bearing; the PR suite alone read 5/5 green on a single run of the same mutant, and test 3 killed it only 3/10 (m2-test3-stability.txt) — F1 stands |
| M3: rollback splice deleted (positive control) | suite | killed — 4 red with the exact ghost signature (m3-no-splice.log) |
harness collects tests exercising the mutated file |
M4: gate condition removed (if (true)) |
suite + G1 | killed by t5 — expected "writeTeamFile" to not be called at all, but actually been called 1 times; G1 flips GREEN — counterAfter=2 writeCalls=2 persisted=["alpha"] (m4-gate-removed.log) |
t5 is non-vacuous; the gate is also what causes the F-NEW skip |
| M5: leader notification removed (debug.warn kept) | test 4 | killed — expect(notice).toBeDefined() red (m5-no-notice.log) |
the 8ce25967 notification delta is pinned |
M7: queue poison guard removed (teamFileWriteQueue = write) |
test 4 | killed — the rejected compensating write escapes the chain (ENOSPC), gamma's spawn cannot persist (m7-poison.log) |
the .catch(() => {}) clause is pinned |
| unmutated head | suite + G2 | 5/5 green, 5/5 flake rounds, G2 green | control |
Combination row (A+B removed) is the base cell: red. Witness: 04-g2-queue-witness-m2-red.png.
Reviewer Test Plan walk-through (per step)
- "Run the new regression test" — the exact command
npx vitest run packages/core/src/agents/team/TeamManager.ghost-member.test.ts --no-coveragefrom the repo root runs and passes 5/5 (logs/testplan-as-written.log). - "Before fix: test fails with
expected [ 'alpha', 'beta' ] to not include 'beta'" — reproduced byte-for-byte on the base cell. - "A second test verifies concurrent successful spawns preserve both members" — green on base and head (that path was never broken).
- "Existing team tests: 57/57 pass. Core typecheck: clean." — head team suite 297/297 across 11 files (
logs/head-team-suite.log); typecheck exit 0 (logs/head-typecheck.log). The body's "2/2" and "57/57" remain stale relative to the final commit state (5 tests in the ghost suite, 297 in the team directory) — drift, not a defect; unchanged from the previous round.
Findings
F-NEW (Suggestion, correctness) — the compensating-write gate's invariant is false for writes that are "started" but not yet snapshotted
The gate decides whether a failed spawn's compensating write is needed by comparing teamFileWritesStarted against the value captured at member push: the comment at the capture site asserts "Roster writes that started before this push cannot have persisted the member." That invariant does not hold. In persistTeamFile, the queue task increments teamFileWritesStarted and then calls writeTeamFile, whose snapshot — the JSON.stringify inside atomicWriteJSON — is taken only after await fs.mkdir(...) resolves (teamHelpers.ts:270-277, atomicFileWrite.ts:404-412). Between the increment and the snapshot the event loop is free, so a member pushed inside that window is:
- serialized by the started write (the snapshot is taken after the push), yet
- counted by the gate as pushed after the write started —
teamFileWritesStarted == writesStartedAtPushat failure time, so the compensating write is skipped and the member stays on disk: the exact Agent Team: failed concurrent spawn can persist a ghost member #10208 symptom.
Probe G1 (probe-gate-window.test.ts) widens that window by holding the writeTeamFile delegate — the same timing-interception technique the PR's own test 3 uses, and equivalent to an arbitrarily slow fs.mkdir; the increment→await→snapshot ordering it exposes is production code, not an artifact of the spy:
G1 on head: counterAtCharliePush=1 counterAfter=1 writeCalls=1 persisted=["alpha","charlie"] ← ghost
G1 on M4 (gate gone): counterAtCharliePush=1 counterAfter=2 writeCalls=2 persisted=["alpha"] ← repaired
G1 on head+FIX1: counterAtCharliePush=1 counterAfter=1 writeCalls=1 persisted=["alpha"] ← prevented
Reproduce (probe source is in this artifact directory):
cp tmp/pr10223-verify-20260827-112111/probe-gate-window.test.ts packages/core/src/agents/team/
cd packages/core && npx vitest run src/agents/team/probe-gate-window.test.ts --no-coverage
# RED at HEAD: persisted=["alpha","charlie"]Scope and attribution. Blast radius is the single gate check in spawnTeammate's catch (TeamManager.ts:603); the census of production writers is unchanged from the previous round (persistTeamFile is the only caller of writeTeamFile, verified by grep at the new head), so the counter sees every writer. The gate commit 1a1eed10 trades one window for a narrower one: it correctly closes the "failed spawn is the first write" interleaving (pinned by test 5 — without the gate, the compensating write would persist in-flight siblings), but the pre-gate unconditional write had closed this interleaving, so the newest commit reopens a ghost path an earlier commit of this same PR had shut. The natural window is fs.mkdir latency (the probe widens it; the frequency of concurrent pushes landing inside it under real load is not measured — see Not covered). Head remains strictly better than base: every scenario the PR's suite pins is fixed.
Minimal suggested fix (measured, not eyeballed)
Snapshot synchronously at the counter increment, so increment and content selection are atomic and the documented invariant becomes true (logs/fix1.diff):
this.teamFileWritesStarted++;
- return writeTeamFile(this.teamFile.name, this.teamFile);
+ const snapshot: TeamFile = JSON.parse(JSON.stringify(this.teamFile));
+ return writeTeamFile(this.teamFile.name, snapshot);Measured in a scratch copy, three results (witness: 05-g1-gate-window-fix1-green.png):
- hostile fixture: G1 green —
persisted=["alpha"], the member is never serialized by the pre-push write (prevention, no repair needed); - benign fixture: ghost suite 5/5 + G2 green (7/7 combined,
logs/fix1-run.log) — the queue's last-write-wins semantics are unchanged because each queued task still snapshots at its own turn; - affected suite counts unchanged: team directory 299/299 (297 + 2 probes) under the fix vs 297/297 at head (
logs/fix1-team-suite.log).
The deep clone is faithful: writeTeamFile would JSON.stringify the same object anyway, so serialization is byte-identical. The suite is green with and without the patch on head along every axis it pins — G1 is the fixture that would go red, and it should ship together with the fix.
F1 (Suggestion, carried forward) — test 3 remains a probabilistic witness of the queue mutation
Unchanged since the previous round: test 3 rejects beta and releases the held stale write in the same synchronous block, before the compensating write has started; on a queue-less build the outcome is a rename race. Re-measured at the new head on mutant M2: 3/10 red over 10 runs of test 3 alone (previous round: 5/10 on the equivalent variant), and a single full-suite run of the same mutant read 5/5 green — a reviewer running the mutant once sees a misleading clean result. The queue is proven load-bearing by the deterministic G2 witness (04-g2-queue-witness-m2-red.png), so the fix itself is not in question; the suite's own serialization test would catch a future queue regression only probabilistically. The previous round's suggested strengthening (poll until the rollback is observable, add a short quiescence window, only then release — the protocol G2 uses) was not adopted and remains the recommendation.
Not covered
- Per-commit attribution: shallow checkout (depth 2) reaches only the merge commit and its two parents —
git rev-list HEAD^1..HEAD^2returns 1 with--is-shallow-repositorytrue, while the metadata lists 10 commits. The aggregateHEAD^1..HEADdiff was verified; mutants M1/M2/M4 approximate the intermediate commits but were constructed, not checked out. - Cross-process concurrent spawns: declared out of scope by the PR; consistent with the design —
persistTeamFileis the only production writer (census re-verified at the new head), and cross-session ownership is enforced bycreateTeamFile's exclusivewxcreate. - G1's natural frequency: the probe reproduces the mechanism (increment-before-snapshot ordering) deterministically by holding the write delegate; it does not measure how often a concurrent push lands inside a natural
fs.mkdirwindow, and this environment has no production traffic to sample. Shape reproduced, frequency not. - Repo-wide suites / cli and web-shell workspaces: not run (net diff touches only
packages/core/src/agents/team/; targeted gates ran instead). PR CI covers the rest. - telemetrySwap conflict resolution: the merge conflict in
client.telemetrySwap.test.tsresolved byte-identical to the base tip (net diff 0 lines), so the suite was run once as a gate (10/10,logs/head-telemetryswap.log) rather than A/B'd. - The scratch vitest config omits the
globalSetupdist guard (worktrees have nodist/); main-tree gates used the real config. The FIX1 team-suite run initially tripped that guard exactly as designed (logs/fix1-team-suite.logreplaced after adding the scratch config).
Methodology
Environment: CI-designated sandbox container, merge-ref checkout (HEAD = merge 2304c5784d, HEAD^1 = base d9342ad31e, HEAD^2 = PR head d9f5f272), npm ci + npm run build pre-run at head. A/B arms and all mutants ran in scratch worktrees under tmp/ (removed after capture) with a scratch vitest config identical to packages/core/vitest.config.ts minus the dist guard and reporters; packages/core/node_modules was symlinked into each worktree after asserting realpaths, because ajv v8 lives there (root has v6) and the PR leaves the lockfile untouched. Harnesses are mock-free with respect to the unit under test: real TeamManager, real FakeBackend via the shipped TeamCoordinationHarness, real filesystem writes to per-run temp dirs; only writeTeamFile's timing is intercepted (held delegate / held gate), mirroring the PR's own test-3 technique — and each probe logs its observable state line (G1:/G2:) so the tables above can be checked against logs/head-probes.log. Mutants were applied as minimal edits in the head worktree (logs/m*.diff carries each diff), run, and reverted with git checkout --. All raw logs, probe sources (probe-gate-window.test.ts, probe-queue-witness.test.ts, print-matrix.mjs), and captures live in this artifact directory.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/agents/team/TeamManager.ghost-member.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/team/TeamManager.ghost-member.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/agents/team/TeamManager.ghost-member.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/agents/team/TeamManager.ghost-member.test.ts: P (exit 0)
round 2 · packages/core/src/agents/team/TeamManager.ghost-member.test.ts: P (exit 0)
round 3 · packages/core/src/agents/team/TeamManager.ghost-member.test.ts: P (exit 0)
round 4 · packages/core/src/agents/team/TeamManager.ghost-member.test.ts: P (exit 0)
round 5 · packages/core/src/agents/team/TeamManager.ghost-member.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
approved these changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated) and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1b": none — I completed the deleted-line walk and the supporting source reads without hitting the tool ceiling..
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: packages/core/src/agents/team/TeamManager.ts (findings in rounds 1, 2; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-gated) and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1b"：none — I completed the deleted-line walk and the supporting source reads without hitting the tool ceiling.。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 7 条（其中 7 条首次提出）。发现反复回到同一批文件：packages/core/src/agents/team/TeamManager.ts（第 1、2 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
requested changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 duplicated guarded leader-notification policy — already reported on this head (comment 3871602686)
- R3-2 throwing-callback guard branch untested — already reported on this head (comment 3871602705)
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: packages/core/src/agents/team/TeamManager.ts (findings in rounds 1, 2; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 7 条（其中 7 条首次提出）。发现反复回到同一批文件：packages/core/src/agents/team/TeamManager.ts（第 1、2 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
`persistTeamFile` counted a write as started before `writeTeamFile` serialized the roster: the real writer awaits `fs.mkdir` before stringifying, so a member pushed during that fs hop was persisted by a write the compensating-write gate counted as pre-push — the failed member's compensating write was wrongly skipped and the ghost member from #10208 resurfaced on the next team load. Snapshot the roster synchronously at the counted point and hand the writer that snapshot. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…rror The inner try/catch around the compensating-write failure notification had no coverage: every existing setLeaderMessageCallback site installs a non-throwing capture. If the callback throws while the compensating write fails, the guard keeps spawnTeammate rejecting with the original spawn error instead of the callback error. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The inner catch around the leader notification hand-rolled the cbErr stringification; for a thrown non-Error with a message field — a shape getErrorMessage explicitly handles — String(cbErr) logs [object Object], dropping the diagnostic trail for a failed compensating write. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
🔬 Sandboxed verification is running — watch live progress. The report will be posted here when the run completes. 🔬 沙箱验证正在运行 —— 查看实时进度。运行结束后验证报告会发布在这里。 |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds a compensating
writeTeamFile()call in thespawnTeammate()catch block so that when a concurrent teammate spawn fails, the persistedconfig.jsonis rewritten to match the post-rollback in-memory state. This prevents ghost members from lingering in the team roster file.Why it's needed
When two teammate spawns run concurrently, spawn A's
writeTeamFile()serializes the entire sharedteamFile.membersarray — including spawn B, which is still pending. If B then fails, B's rollback removes it from the in-memory array but performs no compensating write. The persistedconfig.jsoncontinues to list B even though B never committed and has no backend handle.This ghost member remains discoverable until a later successful roster write, and any code that reads
config.jsonfor peer discovery or capacity accounting sees a member that does not exist.Fixes #10208.
Reviewer Test Plan
How to verify
Run the new regression test:
The test uses deferred promises to control
backend.spawnAgent()resolution timing:writeTeamFile()serializes both membersconfig.jsoncontains alpha but NOT betaA second test verifies that concurrent successful spawns still preserve both members.
Evidence (Before & After)
Before fix: test fails with
expected [ 'alpha', 'beta' ] to not include 'beta'After fix: both tests pass (2/2)
Existing team tests: 57/57 pass. Core typecheck: clean.
Tested on
Environment (optional)
Unit test — no runtime environment needed.
Risk & Scope
Linked Issues
Fixes #10208
中文说明
这个 PR 做了什么
在
spawnTeammate()的 catch 块中增加一次补偿性writeTeamFile()调用，确保并发 teammate spawn 失败时，持久化的config.json被重写为 rollback 后的内存状态，防止 ghost member 残留在团队 roster 文件中。为什么需要
两个并发 spawn 共享
teamFile.members数组。先成功的 A 调用writeTeamFile()时会把仍在 pending 的 B 也序列化进去。B 随后失败时，rollback 只从内存删除 B，不做补偿写入，导致config.json继续列出从未提交的 B。修复方案：rollback 之后补一次
writeTeamFile()，让磁盘 membership 与内存一致。评审者验证方法
运行新增回归测试：
修复前测试失败（beta 残留在 config.json），修复后 2/2 通过。现有 team 测试 57/57 通过，core typecheck 无报错。