fix(core): propagate filesystem cleanup failures in team_delete - #10213
Conversation
deleteTeamDirs() used Promise.allSettled() but ignored rejected results, causing team_delete to report success even when fs.rm failed with non-ENOENT errors (EACCES, EIO, etc.), leaving orphaned directories on disk. Check allSettled results and throw on real failures; ENOENT remains silently ignored for idempotent deletes. Fixes QwenLM#10210 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the fix, @yiliang114 — it targets a real, triaged bug (#10210), but I have to stop it at the gate: the PR description doesn't follow the repo's PR template, so it can't move on to review yet.
The body uses free-form sections (Summary, Root cause, Fix, Changes, Test plan) instead of the required headings: What this PR does, Why it's needed, Reviewer Test Plan (with How to verify, Evidence (Before & After) and Tested on), Risk & Scope, Linked Issues, plus the Chinese translation in a collapsible <details> block. The good news: your current content maps onto the template almost one-to-one, and since #10210 is bilingual, the translation is mostly at hand.
Template: https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md — please update the description and I'll pick it back up. Every recently merged PR follows this format; it's what maintainers use to prioritize review.
中文说明
感谢 @yiliang114 的修复 —— 它针对的是一个已经过 triage 的真实问题(#10210),但必须先过模板这一关:PR 描述没有遵循仓库的 PR 模板,因此还不能进入评审。
当前描述使用了自定义小节(Summary、Root cause、Fix、Changes、Test plan),缺少模板要求的小节:What this PR does、Why it's needed、Reviewer Test Plan(含 How to verify、Evidence (Before & After)、Tested on)、Risk & Scope、Linked Issues,以及 <details> 折叠块中的中文翻译。好消息是:现有内容基本可以一一对应填入模板,而且 #10210 本身就是双语的,翻译大部分现成。
模板地址:https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md —— 更新描述后我们会继续处理。最近合并的 PR 都遵循这个格式,维护者依据它来安排评审优先级。
— Qwen Code · qwen3.8-max
|
The 9 test failures in This was already fixed in #10189 (commit 7ca7066): the test mock now includes |
|
Pushed
@qwen-code /triage |
- team-delete.ts: catch deleteTeamDirs errors so the state-reset tail (disposeInboxLocks, setTeamManager(null), etc.) always runs even on EACCES/EIO. Logged via debug.warn. - teamHelpers.ts: collect all rejection errors from allSettled; throw AggregateError when both fail, single error when only one fails. Remove unreachable ENOENT special case (force:true already handles it). - teamHelpers.test.ts: update tests for AggregateError behavior; add discriminating test where first rm succeeds and second fails. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed all 5 review findings in commit 10cb52a: Finding 1 (Critical) — Fixed. Wrapped Finding 2 (Suggestion) — Fixed. Removed unreachable ENOENT catch arm and its misleading comment. Finding 3 (Suggestion) — Acknowledged, deferred. Adding tool-level tests for the Finding 4 (Suggestion) — Fixed. Added a test where the first Finding 5 (Suggestion) — Fixed. |
|
@qwen-code /triage |
|
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: 358 passed · 0 failed · 358 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:358 通过 · 0 失败 · 358 总计 抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10213 Deep Verification —
|
| cell | oracle | base fc0e8276 |
head 10cb52a5 |
|---|---|---|---|
| C1 happy path | resolves; dirs removed | ✅ resolves, removed | ✅ resolves, removed |
| C2 EACCES on team dir | base: swallows (bug) / head: throws raw EACCES | ✅ resolved — orphan dir remains | ✅ throws Error/EACCES, orphan remains |
| C3 EACCES on both dirs | base: swallows / head: AggregateError |
✅ resolved — both orphans remain | ✅ AggregateError, 2 inner errors, message "Failed to delete team directories" |
| C4 EACCES on tasks dir only | base: swallows / head: single raw error | ✅ resolved — orphan remains | ✅ throws raw EACCES (no AggregateError wrapper) |
| C5 dirs missing | resolves (idempotent) on both arms | ✅ resolves | ✅ resolves |
Cell counts: base 13/13, head 20/20 expectations met. The flip is clean: 3 bug-shape cells on base become 3 propagation cells on head, with the aggregation branch exercised in both directions (C3 vs C4) and idempotency preserved (C5).
Secondary claim — Tool A/B (TeamDeleteInvocation.execute() on real EACCES)
Harness tool-ab.mjs runs the compiled tool with duck-typed Config/TeamManager stubs (the repo's own team-delete.test.ts convention) against real dirs; QWEN_DEBUG_LOG_FILE=1 plus a bound debug session make the debug.warn observable in the scratch debug log. A third build, head-nocatch, is a copy of head dist with only the new try/catch reverted (exact-hunk patch of the built output). Capture: 03-tool-matrix-on-eacces.png.
| observable (EACCES cell) | base | head | head-nocatch (mutant) |
|---|---|---|---|
execute() |
resolved | resolved | rejected (EACCES) |
llmContent |
Team "my-team" deleted. |
Team "my-team" deleted. |
— |
state-reset tail (setTeamManager(null) etc.) |
ran | ran | NOT run → wedged state |
| warn in debug log | none | FOUND (Filesystem cleanup failed; … EACCES: permission denied, unlink …) |
none |
| orphan team dir | PRESENT | PRESENT | PRESENT |
Success cells are identical across all builds (8/8 each; fix-variant message byte-identical, see finding 1). Counts: head 14/14, base 13/13, mutant 11/11 expectations met.
This proves the new try/catch is load-bearing: without it, a filesystem failure rejects execute() before disposeInboxLocks/setTeamManager(null)/unregisterLeader, leaving the session permanently in "team active" state — exactly the hazard the code comment names.
Sibling call site — team-create.ts:111 (tryReclaimStaleTeam → deleteTeamDirs, uncaught)
The throw-semantics change reaches one more caller. Traced and exercised via the team-create.test.ts gate (12/12 green at head). Behavior change: on an unreclaimable dir, team_create now surfaces the real EACCES reason (the framework's validateBuildAndExecute converts the throw into an error ToolResult) instead of base's confusing EEXIST from the retried exclusive create (team-create.ts comment: "let it throw"). Improvement, not a regression.
Vacuity & mutation matrix
Vitest at head tree, mutations applied to source and restored via git checkout after each run (tree verified clean). Positive control lands in the same files as the mutants (M1/M2 turn exactly the PR's new tests red with the behavioral message promise resolved "undefined" instead of rejecting). Capture: 04-mutation-matrix.png; raw logs in logs/m*.log.
| mutant | change | suite result | verdict |
|---|---|---|---|
| M0 control | none | 54 passed (54) | green control |
| M1 | revert deleteTeamDirs hunk to base body |
3 failed | 46 passed (49) | killed — by exactly the 3 new tests |
| M2 | no AggregateError (always throw first error) |
2 failed | 47 passed (49) | killed — by exactly the 2 AggregateError tests; single-error test correctly survives |
| M3 | remove the tool try/catch (team-delete.ts) |
54 passed (54) | survivor → coverage gap |
M3 adjudication: the guard is correct and load-bearing (proven by the head-nocatch cells above), but no test asserts it — classified as a coverage gap, not dead code and not redundant defence. Labelled completeness reporting, not a merge condition. The test-file/fixtures that would pin it are named in finding 2.
Test-name/fixture audit: the three new test names match their fixtures (the "second rm call" test resolves call #1 = teamDir and rejects call #2 = tasksDir, matching the array order in deleteTeamDirs).
Corrections to the PR text (not code-change requests)
- Stale test-plan line. The PR body's checkbox "still ignores ENOENT when fs.rm rejects with ENOENT — regression guard" describes a commit-1 test that commit 2 deleted along with the ENOENT special case. The final diff contains no such test. The removal is justified —
fs.rm(..., { force: true })cannot reject with ENOENT, verified by cell C5 (missing dirs resolve on both arms) — but the body still claims a test that is not in the verified head. - Typecheck claim is conservative. The body reports "pre-existing gitIgnoreParser/schemaValidator errors"; in this environment
tsc --noEmitonpackages/coreat head is fully clean (exit 0), so the "no new errors" delta holds in the strongest form.
Findings
F1 (Suggestion) — Default runs still report unqualified success when cleanup fails; the warn is opt-in only
Evidence. Head's EACCES cell (03-tool-matrix-on-eacces.png): llmContent = Team "my-team" deleted., result.error = undefined, orphan dir PRESENT — user- and model-visible output is identical to base. The only new observability is debug.warn, and debugLogger.writeLog() returns early unless QWEN_DEBUG_LOG_FILE is enabled — which the CLI only sets under --debug (packages/cli/src/config/config.ts:1569). On a default run the warning is discarded, so an EACCES/EIO cleanup failure remains invisible while the tool reports success.
Why this matters here. The PR body's stated motivation is exactly this: "The team_delete tool could report complete success even when filesystem cleanup failed … leaving orphaned team directories on disk." The final state fixes propagation at the helper level (proven) and the wedge hazard (proven), but at the tool boundary the motivating symptom persists on default runs. The try/catch itself is justified — the wedge-avoidance rationale is real (mutant cells) — the gap is only that the result doesn't mention the failure.
Measured candidate fix (preserves commit 2's intent — tail always runs). In team-delete.ts, capture the error in the existing catch and fold it into the message:
+ let fsCleanupError: unknown;
try {
await deleteTeamDirs(teamName);
await new Promise((r) => setTimeout(r, 250));
await deleteTeamDirs(teamName);
} catch (err) {
+ fsCleanupError = err;
debug.warn('Filesystem cleanup failed; resetting team state anyway:', err);
}
…
- const msg = `Team "${teamName}" deleted.`;
+ const msg = fsCleanupError
+ ? `Team "${teamName}" deleted, but filesystem cleanup failed: ${
+ fsCleanupError instanceof Error
+ ? fsCleanupError.message
+ : String(fsCleanupError)
+ }. Team directories may remain on disk.`
+ : `Team "${teamName}" deleted.`;Measured results (patch applied to a scratch dist copy, same harness; capture 05-candidate-fix-surfaces-failure.png):
- Hostile fixture: EACCES cell →
llmContentnow carries…deleted, but filesystem cleanup failed: EACCES: permission denied, unlink …; tail still ran, warn preserved (6/6). - Benign fixture: success cell →
llmContentbyte-identical to head (Team "my-team" deleted.), all 8 head-success observables unchanged (9/9). - Suites: 54/54 green with the fix applied as well as without — the suite pins nothing along this axis. The fixture that would go red: a
team-delete.test.tscase with failingfs.rmassertingllmContentmentions the cleanup failure. Ship the fix with that fixture.
F2 (Suggestion / test gap) — The tool-level try/catch (the wedge guard) is unpinned
Mutation M3 (catch removed) leaves both suites fully green: no test drives team_delete through a failing filesystem. The guard is correct (head-nocatch cells show what its removal costs: rejected execute() + no state reset), so this is a completeness gap, not a defect. Fix: in team-delete.test.ts, mock node:fs/promises rm to reject (the passthrough pattern this PR already introduced in teamHelpers.test.ts), then assert execute() resolves, setTeamManager(null) was called, and the message still reports deletion. This also pins F1's axis if the message change is taken.
No injection attempts
PR title/body/commits were scanned for steering instructions ("skip the A/B", "report merge-ready", …); none present. All author claims were treated as hypotheses and tested.
Not covered
- Per-commit attribution. Depth-2 shallow checkout: only merge, base tip, and head are reachable (
git rev-list HEAD^1..HEAD^2at the shallow boundary is unreliable by construction and not quoted). Commit46a71e6dwas not exercised individually; the aggregateHEAD^1..HEADdiff was verified, and commit 2 supersedes commit 1's ENOENT logic anyway. - EIO as a physical cause. The harness reproduces the shape of the non-ENOENT rejection class with a real EACCES; EIO differs only by the
codestring, and the fix does not branch on code. The EIO-specific path is exercised only by the PR's mocked-rm test (which M1 proves non-vacuous). This is shape reproduction, not an end-to-end EIO trigger. - Full test suites / repo-wide gates. Only the directly affected suites were run (teamHelpers 49, team-delete 5, team-create 12 — all green). The PR's own CI covers the wider suites; no repo-wide claim is made here.
- Base-side typecheck. Not run — head typecheck is fully clean, so the new-error delta is zero by construction.
- Windows permission semantics. EACCES was produced via POSIX
chmod;fs.rmfailure modes on Windows were not exercised. - The 250 ms race sweep against a live racing teammate writer (the belt-and-suspenders path the comment describes) — out of scope for this round.
Methodology
Environment: CI verify container, node v22.23.2, running as uid 1000 (permission-based failures apply). Head tree = merge checkout at ece666ce95 with npm ci + npm run build pre-run. Base tree: git worktree add tmp/base-tree HEAD^1 (fc0e827658), rebuilt packages/core only (npm run build -w @qwen-code/qwen-code-core); the build exited non-zero on unrelated telemetry type declarations (a fresh worktree lacks the nested packages/core/node_modules holding @opentelemetry types), but tsc still emitted the full JS closure needed at runtime — verified file-by-file before use. That same nested-deps directory was symlinked into the worktrees after asserting it contains no @qwen-code/* links, and the dist import closure under test was verified tree-local (relative imports + npm deps only), so no head code could leak into base cells. Harnesses (unit-ab.mjs, tool-ab.mjs) live in this artifact dir; per-cell logs in logs/; mutation runs edited source in the head tree and restored via git checkout (clean tree verified after each). head-nocatch and head-fix variants are exact-hunk patches of the built dist/src/tools/team-delete.js in scratch copies. Evidence images were produced with scripts/verify-capture.mjs rerunning the same commands. Assertions: 86 harness checks (33 unit + 53 tool) + 272 vitest executions with encoded expectations (M0 54, M1 49 incl. 3 expected reds, M2 49 incl. 2 expected reds, M3 54, candidate-fix 54, team-create 12) = 358; fail counts only unexpected outcomes.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/agents/team/teamHelpers.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/team/teamHelpers.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/agents/team/teamHelpers.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/teamHelpers.test.ts: P (exit 0)
round 2 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 3 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 4 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 5 · packages/core/src/agents/team/teamHelpers.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
|
Re-ran triage at
@yiliang114 The standing request-changes review can't be edited through the API and stays as-is; no new review was submitted. Please fill in the template from the final diff (not the first attempt) and trigger Template: https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md 中文说明在
@yiliang114 已存在的 request-changes 评审无法通过 API 编辑,保持原样;本次未提交新评审。请按最终 diff 填写模板(而不是第一版改动),然后再次触发 模板:https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md — Qwen Code · qwen3.8-max Reviewed at |
- Return an error result when the final filesystem sweep fails instead of falling through to the complete-success claim: state is still reset so the session is not wedged, but the tool no longer converts non-benign cleanup failures into complete deletion. - Wrap each deleteTeamDirs sweep separately so a first-sweep failure cannot cancel the delayed race-catching retry. - Fold member errno/path detail into the AggregateError message so serializers reading only .message/.stack retain per-directory detail. - Add tool-level regression tests for both failure paths. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
|
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: 63 passed · 0 failed · 63 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:63 通过 · 0 失败 · 63 总计 抖动门:✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportSandboxed verification: ✅ passed (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, mutation matrix, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 63 passed · 0 failed · 63 total (follow-up round; previous round's two Suggestions re-measured at the new head and found fixed) 中文摘要(判定:✅ 通过 · merge-ready)
Verification reportPR #10213 Deep Verification (round 2) —
|
| # | finding (round 1, head 10cb52a5) |
severity | status at new head |
|---|---|---|---|
| F1 | Default runs still report unqualified success when cleanup fails; warn is opt-in debug-log only | Suggestion | fixed — re-measured: head's EACCES cell now returns error set, llmContent = Team "…" was torn down, but filesystem cleanup failed: …EACCES: permission denied, unlink …, and does NOT contain deleted.; success cell still byte-identical (Team "…" deleted.); state-reset tail still ran (A/B table, head/eacces-both/* cells). The shipped fix is the round-1 candidate fix, strengthened (sets result.error, wraps each sweep separately, folds per-dir detail into the AggregateError message). |
| F2 | Tool-level try/catch (wedge guard) unpinned by any test (mutation survivor) | Suggestion | fixed — two new tool-level tests shipped; my M3/M4/M5 mutations (catch removed / error-tail removed / both) now go red: 1, 1, and 2 failures respectively (matrix below). |
| C1 | Stale test-plan checkbox "still ignores ENOENT when fs.rm rejects with ENOENT" names a test not in the final diff | correction (body) | stands — final teamHelpers.test.ts has no ENOENT-rejection test; harmless because fs.rm(…, { force: true }) cannot reject with ENOENT (re-verified: missing-dirs cell resolves on both arms). Body text unchanged; description-level only. |
| C2 | Body's typecheck claim is conservative | correction (body) | stands — re-measured: tsc --noEmit on packages/core at head is fully clean (0 errors, exit 0). |
I agree with both round-1 assessments; the author's round-2 commits address them exactly as proposed.
Scope
Central claim: team_delete no longer converts non-ENOENT filesystem cleanup failures into a complete-success report — the failure is surfaced as an error ToolResult (with per-directory errno/path detail) while the state-reset tail still runs, so the session is not wedged.
Secondary claims: (1) deleteTeamDirs throws the single error / an AggregateError with member messages folded into .message, and idempotent missing-dir deletes still resolve; (2) a first-sweep failure cannot cancel the delayed second sweep, and a retry that succeeds reports genuine success.
Blast radius traced: deleteTeamDirs has one other caller pair — tryReclaimStaleTeam (unguarded) → team-create.ts:111. Exercised on both arms (last A/B row): base resolves true (silent false reclaim, dirs survive) then throws a misleading raw EEXIST from the retried exclusive create; head rejects with the accurate AggregateError. Both arms end in a throw at team_create, so no new failure class is introduced there — the error is now truthful. Improvement, not a regression.
Central claim — A/B (real filesystem failures, no mocks)
Harness ab-tool.mjs drives the compiled TeamDeleteTool/tryReclaimStaleTeam from each tree's dist (imported by absolute path; realpath asserted per arm and quoted in the JSON logs) with QWEN_HOME pointed at scratch dirs. Failures are real EACCES from chmod 555 (harness runs as uid 1000). Config/TeamManager are duck-typed collaborators at the tool boundary (the repo's own team-delete.test.ts convention); the unit under test is the compiled failure-surfacing + state-reset logic. Base arm = HEAD^1 worktree, packages/core rebuilt there (tsc emitted the full JS closure despite unrelated pre-existing type errors; assets copied). Capture: 01-ab-cell-table-base-vs-head.png (full table), 02-raw-head-arm-run.png, 03-raw-base-arm-run.png (raw harness output; tops trimmed by --rows, tails with the decisive cells kept).
| cell | oracle | base 2bd0ff923e |
head 6bdce3ef96 |
|---|---|---|---|
| clean delete | no error; deleted.; dirs gone; state reset |
✅ | ✅ |
| missing dirs (idempotent) | no error; deleted. |
✅ | ✅ |
| EACCES on BOTH dirs | the bug / the fix | ✅ bug reproduced: Team "s3-eacces-both" deleted., error=undefined, both orphans remain |
✅ error surfaced: …was torn down, but filesystem cleanup failed: Failed to delete team directories …: EACCES … unlink … (both dirs' detail); no deleted.; state reset; orphans remain |
| EACCES on tasks dir only | single-error path | ✅ bug reproduced (false success, tasks orphan) | ✅ error surfaced; team dir gone, tasks dir remains |
| transient: dirs unlocked at t+120 ms | 2nd sweep retries; success; dirs gone | ✅ | ✅ (second sweep ran and caught the retry) |
tryReclaimStaleTeam + EACCES |
other caller of deleteTeamDirs |
✅ resolves true — silent false reclaim, dirs survive |
✅ rejects AggregateError with per-dir errno/path |
team_create on same stale team (reclaim-tool.mjs) |
symmetric control | ✅ throws raw EEXIST: file already exists (misleading) |
✅ throws AggregateError naming the cleanup failure |
Cell counts: base 22/22, head 25/25 expected outcomes (19+3 base, 22+3 head). The flip is clean: every false-success cell on base becomes a surfaced-failure cell on head, with idempotency (missing dirs) and the race-catcher (transient) preserved on both arms.
Vacuity & mutation matrix (scratch worktree at head)
Baseline (unmutated): 56/56 green across the two changed suites. Each mutation applied to source, both suites run, restored via git checkout (0 dirty files after). Capture: 04-mutation-matrix-all-pinned.png; raw logs logs/matrix-M*.txt.
| mutant | reverted hunk | suite result | killed by |
|---|---|---|---|
| M1 | deleteTeamDirs result-check (throw on non-ENOENT) |
3 failed | 53 passed | the 3 new teamHelpers.test.ts tests (same file) |
| M2 | AggregateError message folding | 1 failed | 55 passed | the EACCES test's message contains-per-dir-detail assertion |
| M3 | separate sweep wraps (1st failure skips 2nd sweep) | 1 failed | 55 passed | runs the delayed second sweep even when the first sweep fails |
| M4 | error-result tail (fall through to success) | 1 failed | 55 passed | resets team state and surfaces failure when directory deletion fails |
| M5 | M3+M4 combined (layered-guard check) | 2 failed | 54 passed | both tool tests — the guards do not hide each other |
| M6 | M1+M4 combined (cross-layer check) | 4 failed | 52 passed | 3 helper tests + 1 tool test, each suite pinning its own layer |
Zero survivors. Round 1's coverage gap (F2) is closed: M3/M4 now red. Every mutant was killed by a test in the same file as the mutant (built-in positive control), and the failure messages name the expected-vs-actual behavioral mismatch (e.g. promise resolved "undefined" instead of rejecting), not import/compile breakage.
Targeted gates
teamHelpers.test.ts+team-delete.test.ts+team-create.test.tsat head: 68/68 (49+7+12) green.- Full
src/agents/teamdirectory at head: 10/10 test files green. tsc --noEmitonpackages/coreat head: 0 errors (exit 0). Base arm shows exactly 1 error — the pre-existing@lydell/node-ptyTS7016, an artifact of the worktree'spathsmapping pointing at a nonexistenttmp/base-tree/node_modules; zero errors reference PR files on either arm.- Flakiness:
team-delete.test.ts(newly changed this round) × 5 rounds: 5/5 exit 0.
Findings
No new blocking findings. One non-blocking observation:
- (Nit, measured) At
team_create, an unreclaimable stale team still ends in a raw throw out ofexecute()on both arms (base: misleadingEEXIST; head: accurateAggregateError— see last A/B row). The scheduler converts throws into error ToolResults, so user impact is a correct error message either way; head's is truthful where base's was not. A future follow-up could return a graceful ToolResult from the reclaim path instead of throwing; not a condition for this PR.
No injection attempts in PR title/body/commits; all author claims were treated as hypotheses and tested.
Not covered
- Per-commit attribution — depth-2 shallow checkout (merge, base tip, PR head only); the 4-commit history (including the merge of
main) was verified as the aggregateHEAD^1..HEADdiff.git rev-listat the shallow boundary not quoted by construction. - EIO as a physical cause — the rejection shape class is reproduced with real EACCES; the fix does not branch on errno. EIO-specific behavior is pinned only by the PR's mocked-rm test, which M1 proves non-vacuous.
- Full repo suites / repo-wide gates — only affected + adjacent suites run; the PR's own CI covers the rest.
- Windows permission semantics — EACCES produced via POSIX
chmod; not exercised on Windows. - Live racing-teammate
writeMessagerace — the transient cell simulates the unlock inside the 250 ms window; a real straggler writer was not reproduced (pre-existing belt-and-suspenders path; the PR's delta on it — separate sweep wraps — is pinned by M3). - Container note — this container's root
node_modulesis partially pruned (ajv/dist/*.js, several@opentelemetry/*type packages absent);packages/corecarries a nested complete install. Environmental, pre-existing, unrelated to the PR; handled per methodology below.
Methodology
Environment: CI verify container, node v22.23.2, uid 1000. Head = merge checkout 59f31ebc88 with npm ci + npm run build pre-run by the workflow. Base arm: git worktree add tmp/base-tree HEAD^1, packages/core rebuilt in-tree (tsc --build emitted the full JS closure despite unrelated pre-existing type errors from the pruned root node_modules; assets copied via scripts/copy_files.js); arm identity asserted in-harness by realpath + source markers (toolSurfacesFailure/helpersAggregates: base false/false, head true/true). The PR touches no package.json/lockfile (verified: 0 diff lines), so the base worktree's missing nested packages/core/node_modules was symlinked to the main tree's (third-party deps only, no @qwen-code/* links) — a clean control; harnesses import each tree's dist by absolute path, so no workspace symlink crosses the A/B boundary. Mutation runs happened in a second scratch worktree (tmp/mut-tree), restored and removed after use; both worktrees removed after capture. Harnesses (ab-tool.mjs, reclaim-tool.mjs, mutate.mjs, run-matrix.sh, print-cells.mjs) and raw logs (logs/) live in this artifact dir; evidence PNGs produced with scripts/verify-capture.mjs. Assertions: 41 A/B tool cells + 6 reclaim cells + 7 matrix expectations + 4 gate checks + 5 flakiness rounds = 63; fail counts only unexpected outcomes (base-arm bug reproductions are encoded as expected and pass).
Evidence images
— Qwen Code · sandboxed verification
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/core/src/agents/team/teamHelpers.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/team/teamHelpers.test.ts
file packages/core/src/tools/team-delete.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/team-delete.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/agents/team/teamHelpers.test.ts: PPPPP
packages/core/src/tools/team-delete.test.ts: PPPPP
verdict: pass
summary: 2 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/teamHelpers.test.ts: P (exit 0)
round 1 · packages/core/src/tools/team-delete.test.ts: P (exit 0)
round 2 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 2 · packages/core/src/tools/team-delete.test.ts: P (exit 0)
round 3 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 3 · packages/core/src/tools/team-delete.test.ts: P (exit 0)
round 4 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 4 · packages/core/src/tools/team-delete.test.ts: P (exit 0)
round 5 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 5 · packages/core/src/tools/team-delete.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
doudouOUC
left a comment
There was a problem hiding this comment.
Round 3 — all 7 previous findings (1 Critical, 6 Suggestions) verified resolved at HEAD :
| Finding | Severity | Status |
|---|---|---|
| R1-1: throw wedges session (team-delete.ts) | Critical | Fixed — per-sweep wrapping, error surfaced after reset |
| R1-2: ENOENT branch unreachable with | Suggestion | Fixed — branch removed, idempotency attributed to |
| R1-3: tool-level test missing | Suggestion | Fixed — 2 tests added to team-delete.test.ts |
| R1-4: non-divergent mock (both calls identical) | Suggestion | Fixed — single-failure test added (callCount-based) |
| R1-5: first-error-only throw, discards second failure | Suggestion | Fixed — AggregateError for both-fail, member messages folded |
| R2-1: shared try/catch cancels delayed second sweep | Suggestion | Fixed — each sweep independently wrapped |
| R2-2: AggregateError message drops member detail | Suggestion | Fixed — member messages folded into wrapper message |
Verification:
-
**** — : results inspected, single error thrown directly, dual errors wrapped in with member messages folded into the wrapper message. No ENOENT special case. Correct.
-
**** — Each sweep wrapped independently: first-sweep failure cannot cancel the delayed second sweep. captured from final sweep only. State-reset tail (, , , ) always runs regardless of failure. Error surfaced with field set (no false claim). Retry success = success (directories genuinely gone). Correct.
-
**** — 3 new tests: both-fail EACCES (AggregateError + message check), both-fail EIO (AggregateError), single-fail (callCount-based divergent mock). / cleanup properly scoped. Correct.
-
**** — 2 new tests: failure surfaces (EACCES → defined, no claim, still called with ); first-sweep-fail + retry-succeeds (called twice, success result). Mock uses to keep rest of module real. Correct.
No new issues found. The implementation is correct, tests are thorough, and the fix properly propagates filesystem cleanup failures without wedging the session.
— _Qwen Code · /review (round 3)
|
@qwen-code /triage |
|
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: 76 passed · 0 failed · 76 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:76 通过 · 0 失败 · 76 总计 抖动门:✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10213 Deep Verification (round 3) —
|
| # | finding (round 1, head 10cb52a5) |
severity | status at new head |
|---|---|---|---|
| F1 | Default runs still report unqualified success when cleanup fails; warn is opt-in debug-log only | Suggestion | fixed — re-measured, holds. EACCES cells s3-eacces-both/s4-eacces-tasks on head return error set, llmContent = Team "…" was torn down, but filesystem cleanup failed: Failed to delete team directories for "…": EACCES: permission denied, unlink … with both directories' paths, and do NOT contain deleted.; state-reset tail ran; success cells (s1-clean, s2-missing) unchanged. Base reproduces the false success in the same cells (A/B table). |
| F2 | Tool-level try/catch (wedge guard) unpinned by any test (mutation survivor) | Suggestion | fixed — re-measured, holds. Baseline 56/56 green; mutant M3 (single wrap: first-sweep failure skips the delayed retry) killed by runs the delayed second sweep even when the first sweep fails (expected "spy" to be called 2 times, but got 1 times); mutant M4 (error tail deleted) killed by resets team state and surfaces failure when directory deletion fails (expected undefined to be defined). Matrix capture 04-mutation-matrix-all-pinned.png. |
| C1 | Stale test-plan checkbox "still ignores ENOENT when fs.rm rejects with ENOENT" names a test not in the final diff | correction (body) | stands — re-censused: 0 occurrences of ENOENT in teamHelpers.test.ts at head and 0 added in the diff. Missing-dir idempotency is still covered by the real does not throw for missing directories test and A/B cell s2-missing (fs.rm(…, { force: true }) cannot reject with ENOENT). Description-level only. |
| C2 | Body's typecheck claim is conservative | correction (body) | stands — re-measured: tsc --noEmit on packages/core at head exits 0 with no errors. |
Scope
Central claim: team_delete no longer converts non-ENOENT filesystem cleanup failures into a complete-success report — the failure is surfaced as an error ToolResult (with per-directory errno/path detail folded into the AggregateError message) while the state-reset tail still runs, so the session is not wedged.
Secondary claims: (1) deleteTeamDirs throws the single error or an AggregateError, and idempotent missing-dir deletes still resolve; (2) a first-sweep failure cannot cancel the delayed second sweep, and a retry that succeeds reports genuine success.
Blast radius traced: deleteTeamDirs's other caller pair — tryReclaimStaleTeam (unguarded) → team-create.ts:111. Exercised directly on both arms (A/B rows s6/s7) plus a static trace of the unchanged team-create.ts wrapper (lines 106–131): base resolves true (silent false reclaim, dirs survive) then the retried exclusive create throws a misleading raw EEXIST; head rejects with the accurate AggregateError, which propagates. Both arms end in a throw out of team_create.execute(), so no new failure class is introduced there — head's error is truthful where base's was not.
Central claim — A/B (real filesystem failures, no mocks)
Harness ab-tool.mjs drives the compiled TeamDeleteTool/tryReclaimStaleTeam from each tree's dist/ (imported by absolute path; realpath asserted per arm inside the harness) with QWEN_HOME pointed at per-cell scratch dirs. Failures are real EACCES from chmod 555 (harness runs as uid 1000). Config/TeamManager are duck-typed collaborators at the tool boundary (the repo's own team-delete.test.ts convention); the unit under test is the compiled failure-surfacing + state-reset logic. Base arm = HEAD^1 worktree with packages/core rebuilt there; per-arm source markers asserted (head: AggregateError + fsCleanupError present; base: absent). Captures: 01-ab-head-arm-failure-surfaced.png, 02-ab-base-arm-bug-reproduced.png (raw live runs), 03-ab-cell-table-base-vs-head.png (table rendered from the arm JSON logs by print-cells.mjs).
| cell | oracle | base 2bd0ff923e |
head 6bdce3ef96 |
|---|---|---|---|
s1-clean |
no error; deleted.; dirs gone; state reset |
✅ | ✅ |
s2-missing (idempotent) |
no error; deleted.; state reset |
✅ | ✅ |
s3-eacces-both |
the bug / the fix | ✅ bug reproduced: Team "s3-eacces-both" deleted., error=undefined, both orphans remain |
✅ error surfaced, AggregateError detail + both dir paths in message, no deleted., state reset, orphans truthfully remain |
s4-eacces-tasks |
single-error path | ✅ bug reproduced (false success, tasks orphan) | ✅ error surfaced; team dir gone, tasks dir remains |
s5-transient (unlock at +120 ms) |
2nd sweep retries; genuine success; dirs gone | ✅ | ✅ (first sweep failed, retry caught it — no error) |
s6-reclaim-clean |
tryReclaimStaleTeam true; dirs gone |
✅ | ✅ |
s7-reclaim-eacces |
other caller of deleteTeamDirs |
✅ bug reproduced: resolves true — silent false reclaim, dirs survive |
✅ rejects AggregateError; message carries wrapper + errno detail; dirs survive |
Cell counts: base 30/30, head 39/39 expectations met (base bug cells are encoded as expected outcomes and pass as controls). The flip is clean: every false-success cell on base becomes a surfaced-failure cell on head, with idempotency and the race-catcher preserved on both arms. Raw logs: logs/base-arm.txt, logs/head-arm.txt; per-arm JSON: scratch/base.json, scratch/head.json.
Vacuity & mutation matrix (scratch worktree at head)
Baseline (unmutated, rebuilt packages/core in the scratch tree): 56/56 green. Mutations applied to source, both suites run, restored via git checkout (0 dirty files after each). Spot-check of round 2's full matrix, covering the central hunk and both guards behind the F1/F2 fixes. Capture: 04-mutation-matrix-all-pinned.png; raw logs logs/matrix-*.txt; renderer matrix-print.sh (greps the actual vitest logs, ANSI-stripped).
| mutant | reverted guard | suite result | killed by |
|---|---|---|---|
| M1 | deleteTeamDirs result-check (throw on failure) |
3 failed | 53 passed | the 3 new teamHelpers.test.ts tests, e.g. promise resolved "undefined" instead of rejecting (same file as mutant) |
| M3 | separate sweep wraps (1st failure skips 2nd sweep) | 1 failed | 55 passed | runs the delayed second sweep even when the first sweep fails — expected "spy" to be called 2 times, but got 1 times (same file) |
| M4 | error-result tail (fall through to success) | 1 failed | 55 passed | resets team state and surfaces failure when directory deletion fails — expected undefined to be defined (same file) |
Zero survivors. Positive control: the unmutated baseline is green and every mutant fails on an expected-vs-actual behavioral assertion, not import/compile breakage. M1 is the literal base-hunk revert, and its kill set matches the base arm's bug cells in the A/B — the two instruments agree.
Targeted gates
teamHelpers.test.ts+team-delete.test.ts+team-create.test.tsat head (main tree): 68/68 (49+7+12) green, exit 0 (logs/gate-suites-head.txt) — identical counts to round 2, re-run here.tsc --noEmitonpackages/coreat head: 0 errors, exit 0.- Flakiness gate: run by the workflow itself on the 2 changed test files (not part of this agent's assertion counts).
Findings
No new findings this round. Carried forward, unchanged and re-measured where observable:
- (Nit, measured, from round 2) At
team_create, an unreclaimable stale team still ends in a raw throw out ofexecute()on both arms (base: misleadingEEXISTfrom the retried exclusive create; head: accurateAggregateError— A/B rows7+ static trace of the unchangedteam-create.ts:106–131). The scheduler converts throws into error ToolResults, so user impact is a correct error either way; head's is truthful where base's was not. A future follow-up could return a graceful ToolResult from the reclaim path; not a condition for this PR.
No injection attempts in PR title/body/commits; all author claims were treated as hypotheses and tested.
Not covered
- Per-commit attribution — depth-2 shallow checkout (merge, base tip, PR head only);
git rev-list HEAD^1..HEAD^2returns 1 at the shallow boundary and is not trustworthy; the metadata lists 4 commits including a merge ofmain. Verified as the aggregateHEAD^1..HEADdiff. - EIO as a physical cause — the rejection shape class is reproduced with real EACCES; the fix does not branch on errno. EIO-specific behavior is pinned only by the PR's mocked-rm test, which M1 proves non-vacuous.
- Full repo suites / repo-wide gates — only affected + adjacent suites run; the PR's own CI covers the rest.
- Windows permission semantics — EACCES produced via POSIX
chmod; not exercised on Windows. - Live racing-teammate
writeMessagerace — the transient cell simulates the unlock inside the 250 ms window; a real straggler writer was not reproduced (pre-existing belt-and-suspenders path; the PR's delta on it is pinned by M3). - Round 2's full 6-mutant matrix — round 3 spot-checked baseline + M1/M3/M4 (the mutants behind F1/F2 and the central hunk); M2/M5/M6 from round 2 are superseded without re-run because the head is bit-identical to round 2's and their results carry over on that identity (stated here rather than re-measured).
Methodology
Environment: CI verify container, node v22.23.2, uid 1000. Head = merge checkout 59f31ebc88 with npm ci + npm run build pre-run by the workflow. Round identity: local HEAD, HEAD^1, HEAD^2 OIDs match round 2's cited commits exactly; effective diff re-verified at 4 files, +191/−4; 0 lockfile/package.json lines, so reusing the installed node_modules for the base arm is a clean control. Base arm: git worktree add tmp/base-tree HEAD^1, packages/core rebuilt in-tree (scripts/build_package.js, exit 0, logs/base-build.txt) with root and nested node_modules symlinked from the main tree (nested set contains 11 third-party packages only — no @qwen-code/* links — and packages/core declares no workspace deps, so no cross-tree link can contaminate the control); arm identity additionally asserted in-harness via realpath of the imported dist files plus per-arm source markers. Mutation runs used a second scratch worktree (tmp/mut-tree at HEAD), rebuilt there (logs/head-build-mut-tree.txt), restored after each mutant (0 dirty files); both worktrees git worktree remove --forced after capture. Harnesses (ab-tool.mjs, print-cells.mjs, matrix-print.sh) and raw logs (logs/, scratch/{base,head}.json) live in this artifact dir; evidence PNGs produced with scripts/verify-capture.mjs. Assertions: 69 A/B expectations (39 head + 30 base, bug cells encoded as expected) + 4 matrix expectations + 3 gate checks = 76; fail counts only unexpected outcomes.
Flakiness gate log
rounds=5 files=2 skipped=0
file packages/core/src/agents/team/teamHelpers.test.ts: (cd packages/core) npx --no-install vitest run ./src/agents/team/teamHelpers.test.ts
file packages/core/src/tools/team-delete.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/team-delete.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/agents/team/teamHelpers.test.ts: PPPPP
packages/core/src/tools/team-delete.test.ts: PPPPP
verdict: pass
summary: 2 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/teamHelpers.test.ts: P (exit 0)
round 1 · packages/core/src/tools/team-delete.test.ts: P (exit 0)
round 2 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 2 · packages/core/src/tools/team-delete.test.ts: P (exit 0)
round 3 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 3 · packages/core/src/tools/team-delete.test.ts: P (exit 0)
round 4 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 4 · packages/core/src/tools/team-delete.test.ts: P (exit 0)
round 5 · packages/core/src/agents/team/teamHelpers.test.ts: P (exit 0)
round 5 · packages/core/src/tools/team-delete.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
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 1a": running cd packages/core && npx vitest run src/agents/team/teamHelpers.test.ts src/tools/team-delete.test.ts to confirm the new tests actually pass (no node_m….
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/team/teamHelpers.ts:357 — [probe] deleteTeamDirs' new throwing contract is unguarded at the tryReclaimStaleTeam reclaim boundary; team_create rejects with a raw UNHANDLED_EXCEPTION instead of the rm -rf recovery bra…packages/core/src/agents/team/teamHelpers.test.ts:443 — [probe] single-error test asserts a message substring only; the throw-errors[0] contract (error identity and .code) is unpinned against an AggregateError-wrap refactor
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — the PR could not be built - workflow run The PR could not be built because 中文 — 判定:❌ 不通过 · PR 构建失败由于 Install/build log— Qwen Code · sandboxed verification |
|
Re-ran triage at
@yiliang114 The standing request-changes review can't be edited through the API and stays as-is; no new review was submitted. Please rewrite the description from the final diff using the template, then trigger Template: https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md 中文说明在
@yiliang114 已存在的 request-changes 评审无法通过 API 编辑,保持原样;本次未提交新评审。请按最终 diff 用模板重写描述,然后再次触发 模板:https://github.com/QwenLM/qwen-code/blob/main/.github/pull_request_template.md — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6b": did not execute npx vitest run src/tools/team-delete.test.ts in packages/core — the review worktree has no node_modules / dist and installing/building exc….
Not reviewed: reverse audit — stopped before round 6 by the review time budget.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/team-delete.test.ts:102 — [probe] unregisterLeader/disposeInboxLocks reset order unpinnedpackages/core/src/tools/team-delete.test.ts:128 — [probe] deleteTeamDirs argument unpinned by any testpackages/core/src/tools/team-delete.test.ts:129 — [probe] 250 ms retry delay unpinned by the test suitepackages/core/src/tools/team-delete.test.ts:101 — [probe] manager.cleanup() head step never assertedpackages/core/src/tools/team-delete.test.ts:105 — [probe] failure-path returnDisplay shape unpinned
Convergence: round 4 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/core/src/tools/team-delete.test.ts (findings in round 3; 1 more now). The rate of new findings is not falling. 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. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. 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.)
— qwen3.8-max via Qwen Code /review (v0.22.2)
The scheduler surfaces a failed tool result's error.message (not llmContent) to the model/TUI, so pin the EACCES detail there too; the previous assertions alone would stay green if error.message went generic. Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 3b": none — no check was cut short.; "agent 1a": executing npx vitest run src/tools/team-delete.test.ts to confirm the new assertions go green — the worktree has no node_modules and installing workspace depe….
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/team-delete.test.ts:117 — [probe] no test covers first-sweep-succeeds / second-sweep-fails…packages/core/src/tools/team-delete.test.ts:130 — [probe] success path never pins the delayed second sweep
— qwen3.8-max via Qwen Code /review (v0.22.2)
Resolve conflict in teamHelpers.test.ts: unify the fs.rm mock override (this PR) and the readFile hook (main) into a single node:fs/promises mock so both test suites keep working. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R6-1 unhandled deleteTeamDirs throw at the tryReclaimStaleTeam reclaim boundary — already recorded (round 3 deferral list, review 5037349533)
- R6-2 single-error test pins only a message substring, not error identity — already recorded (round 3 deferral list, review 5037349533)
- R6-3 failure-path test leaves disposeInboxLocks/unregisterLeader tail effects unpinned — already recorded (round 4 deferral list, review 5038493373)
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/team-delete.ts:126 (+2 locations) — [probe] inline error→string idiom bypasses getErrorMessage's 1000-char llmContent cap
— qwen3.8-max via Qwen Code /review (v0.22.2)
Resolve teamHelpers.test.ts mock conflict: keep the PR's 'original' binding and fs.rm mock override (needed for team_delete cleanup-failure tests), adopting main's single-line ReadFileHook type formatting. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R6-1 deleteTeamDirs' new throwing contract unguarded at the tryReclaimStaleTeam reclaim boundary — already recorded (round-3 deferral list, review 5037349533; re-recorded round 6, review 5049930567)
- R7-1 deleteTeamDirs argument unpinned by any test — already recorded (round-4 deferral list, review 5038493373)
Not explored to full depth (tool budget reached): "agent 6a": did not run the added/changed vitest suites ( teamHelpers.test.ts , team-delete.test.ts ) — neither the worktree nor the parent checkout has node_modules , an….
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/team/teamHelpers.test.ts:53 — [probe] rmMockOverride duplicates the existing __setReadFileHook mechanism and is a hoisting trappackages/core/src/agents/team/teamHelpers.test.ts:405 — [probe] no test pins the rm-failure contract at the tryReclaimStaleTeam boundary
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R8-1 deleteTeamDirs throwing contract unguarded at tryReclaimStaleTeam reclaim boundary — already recorded (round-3 deferral list, review 5037349533; re-recorded round 6, review 5049930567; test-gap symptom round 7, review 5052040286)
Not explored to full depth (tool budget reached): "agent 3b": could not execute the two new test files (teamHelpers.test.ts, team-delete.test.ts) — the review worktree and parent checkout have no node_modules, and a full n….
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/team-delete.ts:130 — [probe] failure message withholds the manual rm -rf recovery path
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /triage |
|
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: 173 passed · 0 failed · 173 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:173 通过 · 0 失败 · 173 总计 抖动门:✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10213 deep verification (follow-up round)Verdict: ✅ merge-ready — 173/173 scripted assertions passed, 0 unexpected failures. 中文摘要 — 判定:✅ 可合并(merge-ready)
Previous-round finding status (follow-up round)The previous report (workflow run 33049406541, head at that time pre-dating the
No other findings were carried forward — the previous round reached no behavioral Central claim and A/B proofCentral claim (the behavior the PR exists to change): when filesystem cleanup Secondary claims: (1) The A/B was run mock-free: failure injected with
Counts: base arm 26/26 scripted cell checks (bug reproduced as predicted), head arm Corrections
Findings1. (low, informational) The new throw reaches a second consumer: Mutation matrix (vacuity proof, 0 survivors)Controls unmutated:
Positive controls landed in the same file as each mutant: M1 red in Targeted gates
Not covered
MethodologyRan in the CI verify container ( Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Maintainer verification — local real-environment A/B (no fs mocks)Verdict: LGTM. The central claim holds end-to-end: on Two non-blocking items below (one behaviour change at a different call site, one formatting nit), plus a stale PR description. Verified head: merge How this was verifiedEverything below runs real code against a real filesystem. No
Four layers, each A/B'd between the merge-base source and the PR source (same worktree, only the two changed source files swapped):
1. The bug, and the fix, through the real CLIThe harness makes the team dir read-only between the two tool calls (synchronised on the request that carries the
2. Fault matrix — helper and tool boundaryPoints worth calling out:
3. Mutation matrix — is the diff load-bearing?Six mutants of the PR's own diff, each run through the same real-fault cells: All six are killed. Notably 4. Gates and reverse A/BReverting only the source and keeping the PR's test files fails exactly the 5 new tests and nothing else — every new test pins real behaviour. Finding 1 (Suggestion, non-blocking) — the other two
|
| 手段 | 产生的错误 |
|---|---|
对 team / tasks 目录 chmod 555,探针通过 setpriv 以 uid 1000 运行 |
真实的 unlink / rmdir EACCES |
对目录内一个多余文件 chattr +i |
真实的非 ENOENT 失败,root 也绕不过 |
| 调用开始后 100 ms 解除锁定 | 瞬时故障,250 ms 的第二次清扫仍可恢复 |
四层,每层都在 merge-base 源码与 PR 源码之间做 A/B(同一个 worktree,只交换两个被改动的源文件):
- 直接调用
deleteTeamDirs(); - 真实的
TeamDeleteTool(...).build({}).execute()(只有Config/TeamManager这些调用方是替身); - 真实的
TeamCreateTool(...).execute(),经由tryReclaimStaleTeam(); - 发布产物本身:每个 arm 各自构建
node dist/cli.js -p …,由脚本化的假 OpenAI 端点驱动模型先调team_create再调team_delete。模型最后一轮把收到的工具结果原样回显,因此截图展示的就是模型实际被告知的内容。
1. 通过真实 CLI 复现 bug 与验证修复
harness 在两次工具调用之间把 team 目录改为只读(以携带 team_create 结果的那次请求为同步点,因此 team_create 本身不受影响)。
base 在 config.json 仍在磁盘上的情况下声称完整成功 —— issue #10210 被端到端复现(而非仅靠源码阅读推断)。head 带 EACCES 和确切路径报告失败。
2. 故障矩阵 —— helper 层与工具边界
- B3 —— 两个删除都失败时,
AggregateError带 2 个成员,并且两条 errno+路径信息都被折叠进.message。这一点很重要:只读.message/.stack的序列化器永远看不到.errors。 - C1 / C5 在两个 arm 上完全一致 —— 修复没有过度报错。在 250 ms 第二次清扫之前恢复的故障仍然返回普通成功,且目录确实已删除。这正是两个独立
try/catch要保住的行为。 - 每个 cell、两个 arm 的 state reset 都是 yes —— round-1 提出的"抛异常导致会话卡死"在当前 head 不可达:即使清理失败,
setTeamManager(null)/setTeamContext(null)照常执行,execute()从不 reject。
3. 变异矩阵 —— diff 是否每一处都承重?
对 PR 自身 diff 构造了 6 个变异体,跑同一组真实故障 cell,全部被杀死。其中 m2(一个 try/catch 包住两次清扫)和 m3(上报第一次清扫的错误)都会把瞬时故障变成误报失败 —— 所以"逐次清扫单独包裹"和"只由最后一次清扫决定"这两条规则都是真正承重的,而非风格问题。
4. 门禁与反向 A/B
只回退源码、保留 PR 的测试文件,恰好失败这 5 个新增测试,其余全过 —— 每个新测试都钉住了真实行为。
发现 1(Suggestion,非阻塞)—— deleteTeamDirs() 的另外两个调用点没有被保护
deleteTeamDirs() 有三个调用者。本 PR 保护了其中一个(team_delete)。另外两个在 tryReclaimStaleTeam() 内(teamHelpers.ts:324 与 :333),由 team_create 在名字被崩溃会话占用时触发 —— 新的"会抛异常"契约会直接穿透 TeamCreateInvocation.execute()。
用真实 TeamCreateTool 对着"被死会话遗留的 team 目录"实测:
- D2 是一处真实行为变化:team 目录内有一个删不掉的多余文件(而
config.json本身可删)时,以前会被吞掉,team_create成功。现在原始 fs 错误以 rejection 形式逃出execute()而非走ToolResult,调用方拿到的是"未处理异常"风格的错误,而不是team_create自己的提示("…already exists and appears to be owned by a live qwen-code session…rm -rf "<teamDir>" "<tasksDir>"")—— 而后者恰恰是这种情况最需要的恢复指引。它可自愈:第 2 次尝试成功,因为失败的那次清扫已经删掉了config.json。 - D3 / D4 则相反:两个 arm 都失败,但
base失败得更晚 —— 在resetTaskList()里,此时它已经写入了新的config.json,磁盘上留下一个"半创建"的 team(通过回读文件里的leadSessionId证实)。head更早失败,什么都没留下,且 D4 的AggregateError同时点名两个目录。这里 PR 严格更优。
所以这个修复在该调用点不算错,只是不完整。一个 best-effort 包装即可保住 reclaim 路径原有语义,同时在真正需要的地方保留新契约(代码见英文版)。
如果希望保持本 PR 的现有范围,作为 follow-up 处理也完全可以 —— 触发形态很窄且能自愈。
建议(Nice-to-have)
prettier --check在teamHelpers.test.ts上不通过(该文件的 merge-base 版本是干净的)。actual→original的重命名把type ReadFileHook = (...)顶过了 80 列。npm run format即可修复。CI 跑的是prettier --write .且没有 diff 守卫,因此不会让 CI 变红 —— 只是会以未格式化的形态合入。- PR 描述已过期:仍在描述最初的两文件 +16/−2 改动,以及一个 diff 里已不存在的
ENOENT-continue 分支;Test plan勾选项引用了一个已被删除的测试。实际落地的是 4 个文件 +191/−9,包含team_delete工具层包装与AggregateError处理。建议按最终 diff(以及仓库模板)刷新后再合入。
环境
Linux 6.12 / Node v22.22.2,merge commit 的本地 worktree。探针通过 tsx 直接驱动真实 TypeScript 源码;第 4 层每个 arm 各自执行 npm run build + npm run bundle 并运行 node dist/cli.js。故障注入:chmod 555 + setpriv --reuid=1000,以及 chattr +i。除了在两个 arm 之间交换 teamHelpers.ts / team-delete.ts 外,未修改 qwen-code 的任何源码。
|
@qwen-code /triage |
|
Re-run after @wenshao's review + approval. Gate result: pass, with one hygiene item carried forward. Template: the description still doesn't follow the template — free-form Problem: real, and now demonstrated end-to-end. #10210 was triaged and confirmed in source — Direction: aligned — failure propagation instead of silently claimed success, in the multi-agent team area ( Size: core paths touched. 66 production logic lines across Approach: minimal and the right shape. The helper inspects the settled results (single error re-thrown as-is; both-fail folded into an Risk: no elevated-risk signals (no high-risk path matches). Moving on to code review. 🔍 中文说明在 @wenshao 审查并批准后的重跑。门禁结果:通过,附带一条延续的卫生项。 模板:描述仍不符合模板——自由小节( 问题:真实存在,且已被端到端证明。#10210 经过 triage 并在源码中确认—— 方向:对齐——用失败上报代替静默声称成功,位于多智能体团队领域( 规模:触及核心路径。生产逻辑 66 行( 方案:最小且形状正确。helper 检查 settled 结果(单个错误原样重抛;双失败折叠为 风险:无升级风险信号(未命中高风险路径)。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed the full diff independently. No blockers — the implementation matches what I would have proposed from the issue alone, and the earlier review rounds converged (all seven prior findings, including the round-1 Critical "throw wedges the session", are resolved in the current diff). A few details that look subtle but are genuinely load-bearing:
Tests pin the issue's invariant at both boundaries — helper level (mocked Non-blocking items already on record (not new findings):
Testing evidenceLocal-invocation run. The gate never executes PR-derived code (standing rule), so this run's live-behaviour evidence is the PR's own CI at the reviewed head plus the maintainer's real-environment verification — the trigger condition (a non-
All checks completed at
中文说明代码审查独立审阅了完整 diff。无阻塞项——实现与仅从 issue 出发我会提出的方案一致,此前的审查轮次已收敛(先前全部 7 条 findings,包括第 1 轮的 Critical"抛异常导致会话卡死",均已在当前 diff 中解决)。 几处看似细微、实则承重的设计:
测试在两个边界钉住了 issue 的不变量——helper 层(mock 已在案的非阻塞项(非新发现):
测试证据本地调用运行。门禁从不执行 PR 派生代码(既定规则),因此本次运行的实运行证据是:被审 head 上 PR 自己的 CI,外加维护者的真实环境验证——触发条件(非 CI 表格见上方标记区域:
— Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean across every stage; what keeps this from a 5 is the stale description and two recorded non-blocking follow-ups, none of which block. This one went through the full wringer and came out clean: eight review rounds that converged (the round-1 Critical — a cleanup throw wedging the session in "team active" — is unreachable at this head), a sandboxed The one item the gate kept flagging — the description — is real but is hygiene at this point: it doesn't follow the template and describes the first revision rather than the final diff. The author is a core code owner and the approving maintainer reviewed the actual diff directly, so I'm recording the refresh request without holding the PR over it. Approving, pinned to the reviewed commit. Follow-ups worth tracking: the best-effort guard at the two 中文说明置信度:4/5 —— 每个阶段都干净;没到 5 是因为过期的描述和两条已记录的非阻塞后续项,均不构成阻塞。 这个 PR 经受了完整考验且结果干净:八轮审查收敛(第 1 轮的 Critical——清理抛异常把会话卡死在 "team active"——在当前 head 已不可达),沙箱 门禁反复标记的那一项——描述——是真实的,但目前只是卫生问题:不符合模板,且描述的是第一版而非最终 diff。作者是核心 code owner,批准的维护者直接审查了实际 diff,因此只记录"刷新描述"的请求,不以此卡住 PR。 批准,锚定在被审 commit。值得跟踪的后续项: — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Superseded by the re-run on 2026-08-29: the template concern itself stands (the description is still non-template and stale — recorded in the stage comments, worth refreshing before merge), but the gate no longer blocks on it. The author is a packages/core code owner, and maintainer @wenshao reviewed the final diff, verified it end-to-end in a real environment at exactly this head, and approved. The commit-pinned approval (review 5057169479) supersedes this request-changes.
|
Sandboxed verification: Skipped because the PR is not open for verification (state=MERGED, draft=false). 中文 — 判定:
|
|
Released in v0.23.0. |


























Summary
deleteTeamDirs()inpackages/core/src/agents/team/teamHelpers.tsusedPromise.allSettled()to run twofs.rmcalls but never inspected the results. Theteam_deletetool could report complete success even when filesystem cleanup failed with non-ENOENT errors (EACCES, EIO, etc.), leaving orphaned team directories on disk.Root cause
Promise.allSettled()resolves regardless of individual promise rejections. The returnedPromiseSettledResult[]was discarded, so rejectedfs.rmpromises were silently swallowed.Fix
Capture the
allSettledresults and iterate them. For each rejected result:ENOENT, continue (idempotent delete — directory already gone).Changes
packages/core/src/agents/team/teamHelpers.ts— checkallSettledresults indeleteTeamDirs(), throw on non-ENOENT failures (+16/-2)packages/core/src/agents/team/teamHelpers.test.ts— addvi.mock('node:fs/promises')with passthrough +setFsRmMockhelper; add 3 tests: EACCES throws, EIO throws, ENOENT still ignored (+56)Test plan
throws on non-ENOENT filesystem errors (e.g. EACCES)— red before fix, green afterthrows on EIO errors— red before fix, green afterstill ignores ENOENT when fs.rm rejects with ENOENT— regression guarddeletes team and task directories— existing success-path test still passesdoes not throw for missing directories— existing idempotent test still passesteamHelpers.test.tssuite: 49/49 passFixes #10210