refactor(review): run the test-efficacy probe in a disposable worktree - #6836
Merged
Conversation
The probe used to revert the PR's source to base IN the shared review worktree and restore it in a `finally`. That shared tree is the one every Step 3 review agent reads, and the in-place mutate/restore was the root of two findings on #6790: - a concurrent reader could observe the tree half-reverted to base for the probe's whole duration (Critical), and the later restore cannot un-produce a finding written from the wrong source; - the restore's in-place delete followed a PR-controlled symlink out of the tree and removed an outside file (P0, band-aided with `safeRmWithin`). Both share one cause — mutating a live, shared tree — and one fix retires both. The probe now runs in its OWN disposable worktree, checked out at the PR head as a sibling of the shared one (`.qwen/tmp/review-pr-<n>-probe`) and removed wholesale when it finishes: - the shared tree is never touched, so no reader can see a reverted state; - there is no in-place restore, so the delete that followed a symlink is gone with it — `safeRmWithin` stays only as belt-and-suspenders on the revert-phase delete of added files; - `node_modules` needs no per-tree install: the probe tree is nested under the repo, so `npx vitest` resolves upward to the repo-root `node_modules`, exactly as the shared worktree already does. (Confirmed empirically before relying on it — this is what had the refactor deferred.) Because the shared tree is no longer mutated, the dirty-worktree guard is gone (nothing the caller has uncommitted is ever discarded), and the loud `restoreFailure` / non-zero exit becomes a soft `cleanupFailure` warning: a leftover probe worktree does not corrupt anything and is swept at the next run's `worktree add` and by `cleanup.ts`. Verified by driving the real handler (new `test-efficacy.integration.test.ts`, real git worktrees, a stub vitest bin): verdicts are unchanged (gated/inert), the shared tree is byte-identical before and after, the probe tree is always discarded, and the symlink P0 repro leaves the outside file intact WITHOUT `safeRmWithin` having to refuse — isolation alone protects it. Closes #6832.
6 tasks
Collaborator
|
Thanks for the PR! Template: The headings differ from the template ( Problem: This is a well-documented, observed issue. #6832 describes two concrete defects found during #6790 review — concurrent readers seeing a half-reverted shared worktree (Critical), and an in-place restore deleting through a PR-controlled symlink (P0). Both have linked discussion threads with reproductions. This is not theoretical hardening. Direction: Clearly aligned. The review pipeline is core qwen-code infrastructure, and eliminating a class of shared-state mutation bugs by design (isolation) rather than guards (the Size: Not applicable — all changes are in Approach: The scope is tight and well-justified. Three files, one concern. The cleanup.ts sweep of stale probe worktrees is a necessary companion change. The dirty-worktree guard removal follows logically from the fresh-checkout invariant. Moving on to code review. 🔍 中文说明感谢贡献！ 模板： 标题与模板有差异（ 问题： 这是一个有充分文档记录的已观测问题。#6832 描述了 #6790 审查过程中发现的两个具体缺陷——并发读取者看到半回滚的共享 worktree（Critical），以及就地恢复通过 PR 控制的符号链接删除外部文件（P0）。两者都有链接的讨论线程和复现。这不是理论性加固。 方向： 明确对齐。审查流水线是 qwen-code 核心基础设施，通过设计层面（隔离）而非守卫（ 规模： 不适用——所有变更在 方案： 范围紧凑且合理。三个文件，一个关注点。cleanup.ts 对残留 probe worktree 的清理是必要的伴随变更。dirty-worktree 守卫的移除是从全新检出不变量自然推导出的。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
yiliang114
approved these changes
Jul 13, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — the disposable worktree cleanly eliminates shared-tree mutation. Three-layer cleanup (finally, pre-sweep, global) is robust, and the integration tests prove real isolation with real git.
Collaborator
Code ReviewIndependent proposal: Before reading the diff, my approach would be identical — create a disposable sibling worktree at PR HEAD, revert source to base there, run vitest, discard the whole tree in a Comparison: The PR's implementation matches this proposal exactly. The modified/added revert partition is reused in the throwaway tree (proven behavior), No critical blockers found. The code is focused, well-commented, and follows project conventions. A few observations:
Reuse check: All existing utilities are reused ( Test ResultsThe Additional checks:
Total: 27 tests, all green. No lint or type errors. 中文说明代码审查独立方案： 在读 diff 之前，我的方案完全一致——在 PR HEAD 创建一个一次性兄弟 worktree，在那里将源码回滚到 base，运行 vitest，最后在 对比： PR 的实现与此方案完全一致。modified/added 回滚分区在一次性 tree 中复用（已验证的行为）， 未发现阻断性问题。 代码聚焦、注释良好、遵循项目约定。 测试结果所有 22 个测试通过（20 个单元测试 + 2 个集成测试）。集成测试直接验证了 #6832 的两个验收标准（共享 tree 字节一致、probe tree 已丢弃、符号链接 P0 通过隔离而非守卫解决）。ESLint 和 TypeScript 检查均通过。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, well-motivated refactor that solves a real problem at the design level rather than piling on guards. The two defects in #6832 — concurrent readers seeing a half-reverted shared worktree, and restore deletes following PR-controlled symlinks — share one root cause: mutating a live, shared tree. This PR eliminates that root cause by giving the probe a disposable worktree and discarding it wholesale. The approach is the minimum viable change; there's no simpler path that addresses both defects simultaneously. The diff is tight: three files, one concern. The dirty-worktree guard removal follows logically from the fresh-checkout invariant. The All 27 tests green, ESLint and TypeScript clean. The PR description is one of the better ones I've reviewed — clear before/after table, explicit scope statement, and honest risk assessment. Approving. ✅ 中文说明这是一个干净、动机充分的重构，在设计层面解决了真实问题，而不是堆砌守卫。 #6832 中的两个缺陷——并发读取者看到半回滚的共享 worktree、恢复阶段通过 PR 控制的符号链接删除外部文件——共享一个根因：变更一个活跃的共享 tree。本 PR 通过给 probe 一个一次性 worktree 并在使用后整体丢弃来消除这个根因。方案是最小可行变更；没有更简单的路径能同时解决两个缺陷。 Diff 紧凑：三个文件，一个关注点。dirty-worktree 守卫的移除从全新检出不变量自然推导。 27 个测试全部通过，ESLint 和 TypeScript 检查干净。批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 13, 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. ✅
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. |
doudouOUC
reviewed
Jul 13, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| // repo-root `node_modules` — exactly how the shared review worktree already | ||
| // runs vitest. | ||
| const headSha = gitOut(worktree, 'rev-parse', 'HEAD'); | ||
| const probeTree = `${resolve(worktree)}-probe`; |
Collaborator
There was a problem hiding this comment.
[Suggestion] The -probe suffix is constructed independently here (${resolve(worktree)}-probe) and in cleanup.ts (${wt}-probe), with no shared constant or helper. If someone renames the suffix in one file and misses the other, cleanup.ts silently stops sweeping stale probe trees.
Suggested change
| const probeTree = `${resolve(worktree)}-probe`; | |
| const probeTree = probeWorktreePath(worktree); |
Extract a shared helper (e.g., probeWorktreePath(wt: string): string in lib/paths.ts) and have both test-efficacy.ts and cleanup.ts call it. Note also that this line uses resolve(worktree) (absolute) while cleanup.ts uses wt from worktreePath() (relative). If worktreePath() ever returns absolute paths, or cleanup runs from a different cwd, the two would diverge. The shared helper should normalize consistently.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Good call — done in #6841. Extracted probeWorktreePath(worktree) into lib/paths.ts; both this site and cleanup.ts now call it, so the -probe suffix lives in one place. It also settles the normalisation you flagged: the helper returns an absolute path, because the probe drives git worktree add/remove with the shared worktree as cwd — a relative path would resolve against that worktree and nest the probe tree inside it.
| // The test-efficacy probe runs in a disposable sibling worktree and removes | ||
| // it itself; sweep one a crashed probe left behind so it does not block the | ||
| // next run's `git worktree add` (see #6832 / test-efficacy.ts). | ||
| const probeWt = `${wt}-probe`; |
Collaborator
There was a problem hiding this comment.
[Suggestion] test-efficacy.ts creates the probe tree at ${resolve(worktree)}-probe (absolute path), but this line sweeps ${wt}-probe where wt is the relative worktreePath(prNumber). Today both resolve to the same directory because cleanup runs from the repo root — but if worktreePath() is ever refactored to return absolute paths, or cleanup is invoked from a different cwd, the paths diverge and the probe tree is orphaned.
Suggested change
| const probeWt = `${wt}-probe`; | |
| const probeWt = probeWorktreePath(wt); |
See the sibling comment on test-efficacy.ts:399 — a shared helper would unify both sites and normalize path resolution.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed together with the sibling comment in #6841 — this line is now probeWorktreePath(wt), sharing the one helper (which resolves to absolute), so the two sites can't drift.
| const notAtHead: string[] = []; | ||
| for (const p of revert) { | ||
| (existsAtHead(worktree, p) ? atHead : notAtHead).push(p); | ||
| // Could not isolate — probe nothing rather than fall back to mutating the |
Collaborator
There was a problem hiding this comment.
[Suggestion] Four new code paths lack test coverage in the integration tests:
git worktree addfailure (this catch block) — all probes goinconclusivewith"probe worktree could not be created". Exercise by pre-creating the probe tree directory.- Cleanup failure path (the
catchinsidefinallythat setscleanupFailure) — tested only on the success path (toBeUndefined). - Pre-sweep of stale probe trees (line ~404) — the crash-recovery sweep before
worktree add. Exercise by leaving a stalewt-probeworktree before invoking the handler. cleanup.tsprobe worktree sweep — no test verifiesreleaseWorktree(probeWt)actually removes a leftover probe tree.
Since the tests use real git, these could be exercised by manipulating worktree state before handler invocation.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Addressed in #6841, partially by design. Tested directly: the pre-sweep of a stale registered probe tree (3), and the unregistered-leftover create-robustness path (a variant of 1) — the latter fails without the rmSync fallback. cleanup.ts's sweep (4) reuses releaseWorktree, already covered in git.integration.test.ts, and its path derivation is unit-tested via the shared helper. The remaining two — a genuine worktree add failure (1) and the cleanupFailure path (2) — I left untested on purpose: forcing them deterministically needs mocking git, which defeats an integration test built on real git bookkeeping, and both are non-corrupting (a recoverable wedge; a soft warning). Happy to add mocked unit coverage if you'd prefer it over the real-git tests.
| ); | ||
| // Sweep a stale probe tree left by a crashed run — it would fail `add`. | ||
| // Best-effort (no stale tree is the normal case), so it does not go through | ||
| // the throwing `git()` wrapper. |
Collaborator
There was a problem hiding this comment.
[Suggestion] The stale-tree sweep discards the entire spawnSync result. When it fails for a non-obvious reason (git lock, permission, the path exists but isn't a registered worktree), the next line throws with "already exists" and no context about why the sweep couldn't clear it.
Suggested change
| // the throwing `git()` wrapper. | |
| const sweep = spawnSync('git', ['worktree', 'remove', '--force', probeTree], { | |
| cwd: worktree, | |
| encoding: 'utf8', | |
| }); |
Capture the sweep result and, if the subsequent worktree add fails, append the sweep's stderr to the error detail: probe worktree could not be created: <add error> (stale sweep also failed: <sweep stderr>).
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Done in #6841 — the sweep's spawnSync result is now captured, and its stderr is appended to the create-failure detail: probe worktree could not be created: <add error> (stale-tree sweep also reported: <sweep stderr>). (In practice the new rmSync fallback removes the most common cause of that add failure, but the context is there when add fails for another reason.)
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.
| // Sweep a stale probe tree left by a crashed run — it would fail `add`. | ||
| // Best-effort (no stale tree is the normal case), so it does not go through | ||
| // the throwing `git()` wrapper. | ||
| spawnSync('git', ['worktree', 'remove', '--force', probeTree], { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The pre-sweep spawnSync('git', ['worktree', 'remove', '--force', probeTree]) only removes directories registered as git worktrees. If a stale directory exists at the probe path but is no longer registered (e.g., after .git/worktrees/ metadata loss or partial cleanup), the remove fails silently and the subsequent git worktree add throws fatal: '<path>' already exists — permanently marking all probes inconclusive until manually removed.
Consider adding an rmSync fallback after the sweep:
Suggested change
| spawnSync('git', ['worktree', 'remove', '--force', probeTree], { | |
| spawnSync('git', ['worktree', 'remove', '--force', probeTree], { | |
| cwd: worktree, | |
| }); | |
| if (existsSync(probeTree)) { | |
| rmSync(probeTree, { recursive: true, force: true }); | |
| } |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Confirmed and fixed in #6841 — and you're right about the exact mechanism. Reproduced on git 2.47: an unregistered non-empty leftover at the probe path makes git worktree remove --force report "not a working tree" and leave it, then git worktree add fails already exists, wedging every probe. The sweep now follows the unregister with rmSync(probeTree, { recursive, force }) (which unlinks a symlink rather than following it), so add always gets a clean path. Integration test added that fails without the fallback. (An empty unregistered dir, for what it's worth, git reuses fine — but the fallback covers both.)
qwen-code-dev-bot
pushed a commit
to doudouOUC/qwen-code
that referenced
this pull request
Jul 14, 2026
…ale-tree sweep (QwenLM#6841) * refactor(review): share the probe-worktree path helper; harden the stale-tree sweep Follow-up to the disposable-probe-worktree change (QwenLM#6836), addressing review suggestions on it. - Extract `probeWorktreePath(worktree)` into `lib/paths.ts` and call it from both the probe (`test-efficacy.ts`) and `cleanup.ts`. The `-probe` suffix was constructed independently in the two files; renaming it in one and missing the other would silently stop cleanup from sweeping. The helper also settles the normalisation: it returns an absolute path, because the probe drives `git worktree add`/`remove` with the shared worktree as cwd, where a relative path would resolve against that worktree and nest the probe tree inside it. - Make the pre-`add` sweep clear an UNREGISTERED leftover too. `git worktree remove --force` only unregisters a tree git still tracks; a directory left at the probe path after metadata loss or a partial cleanup is reported as "not a working tree" and left in place, and a non-empty one then makes `worktree add` fail `already exists` — wedging every probe as `inconclusive` until it is cleared by hand. Follow the unregister with an `rmSync` of whatever remains (which unlinks a symlink rather than following it), so `add` always gets a clean path. The sweep's stderr is now kept and appended to the create-failure detail so a genuine `add` failure explains itself. Tests: `probeWorktreePath` unit tests (suffix, relative→absolute, shared source); integration tests that a stale REGISTERED probe tree is swept and the probe still runs, and that an UNREGISTERED non-empty leftover is cleared rather than wedging the probe. Full review suite green (416). * fix(review): make releaseWorktree actually remove an unregistered leftover Follow-up review on QwenLM#6841 pointed out that the `rmSync` fallback added to the probe's own sweep was missing from `cleanup.ts`, which sweeps the probe worktree through `releaseWorktree`. That helper returns `existed` (true when the path was there on entry) but only runs `git worktree remove --force` — which does nothing to a directory git no longer tracks as a worktree. So an unregistered non-empty leftover survives while `cleanup.ts` prints "Removed probe worktree", and it still blocks the next `git worktree add` with `already exists`. Fix it at the root: `releaseWorktree` now `rmSync`s whatever remains after the unregister, so a `true` return means the path is actually gone. This covers the probe sweep and the main review-worktree sweep in one place, rather than duplicating the fallback at each call site. `rmSync` unlinks a symlink instead of following it, so a tampered leftover cannot redirect the delete. Test: `git.integration.test.ts` — an unregistered non-empty leftover is removed and the path is add-able again. * refactor(review): apply the leftover-safe sweep to the probe's finally too More review on QwenLM#6841. Two accuracy/consistency fixes: - The probe's post-run `finally` still discarded the tree with the throwing `git()` wrapper only — the one sweep site that had not learned the unregistered/non-empty-leftover lesson the pre-sweep and `releaseWorktree` already did. Give it the same two steps (`worktree remove` best-effort, then `rmSync` whatever remains) and key `cleanupFailure` off whether the path still exists afterward, which is the honest signal. - `releaseWorktree`'s JSDoc still said it returns "whether a live worktree was there to remove"; after the rmSync fallback it also returns true for an untracked leftover it clears, and a true return now means the path is gone. Reworded to match. Also noted at the probe pre-sweep why it keeps `releaseWorktree`'s two-step inline rather than calling it: the probe must target this repo via `cwd: worktree`, and it keeps the sweep's stderr for the create-failure detail. No behaviour change on the happy path (full review suite green, 417). * test(review): pin the probe create-failure detail with a pure helper Closes the last open thread on QwenLM#6841: the `sweepErr` diagnostic had no coverage. The branch it lives on fires only when `git worktree add` fails, and there is no portable way to force that in a real-git test. The one lever — making `.git/worktrees` unwritable — is bypassed by root and behaves differently under CI's unprivileged user, so a test built on it would assert one thing locally and another in CI, which is worse than no test. So extract the part that actually has logic in it. `probeCreateFailureDetail` is pure: it names the `add` failure and folds in the stale-sweep's stderr, which is usually what explains it. Unit tests pin the fold, the omission of an empty sweep clause (a dangling "(stale-tree sweep also reported: )" would be report noise), and a non-Error throw. What stays untested is only the try/catch wiring. * fix(review): keep releaseWorktree non-throwing, and keep the discard's reason Three findings on the last round, two of them regressions I introduced. - **`releaseWorktree` must not throw.** The `rmSync` I added gave it a throw path it never had: `force` suppresses ENOENT but not EPERM or EBUSY, and this runs on the cleanup path, where an exception masks the error that got us there — which is exactly what its `gitOpt`-only body was avoiding, and what its "does not throw when git itself fails" test pins. The remove is now swallowed like every other failure here, and the outcome is reported through the return value instead: `true` now means the path is GONE (`existed && !existsSync`), not merely unregistered, so a caller can print "Removed …" without lying. - **The discard lost its reason.** Keying `cleanupFailure` off "does the path still exist" was right, but it dropped the `: ${e.message}` the old code carried. A bare "could not remove <path>" tells whoever has to delete the tree by hand nothing about why they must. Restored, preferring the `rmSync` exception and falling back to what git said when it refused to unregister. - **The two-step was duplicated within the file.** Pre-sweep and post-run discard now share `discardWorktree()`, whose doc carries the reason it is not a call to `releaseWorktree` (it needs `cwd: worktree` and the sweep's stderr). `probeCleanupFailureDetail` is extracted and unit-tested for the same reason its sibling `probeCreateFailureDetail` was: the branch cannot be forced portably, but the composition is where the logic is. Full review suite green (423). * fix(review): stop releaseWorktree losing the reason a path survived Two findings on the last round, and the first is a regression I shipped. **A leftover we could not delete went silent.** Making `releaseWorktree` return "the path is free now" stopped it lying — cleanup no longer prints "Removed …" over a directory that is still on disk — but it swapped one failure for another: `cleanup.ts` only prints on `true`, so a path it could NOT free now produces no output at all. The leftover survives, wedges the next `git worktree add`, and nobody is told. A boolean cannot say both "there is still something there" and "here is why", and the caller needs both. So it returns a result: `existed`, `freed`, and `reason` when it is still there (the `rmSync` errno if there was one — EPERM/EBUSY, which `force` does not suppress — otherwise a plain statement of the situation). `cleanup.ts` now reports in both directions: "Removed …" only when the path is actually gone, and "Failed to remove … : <reason>" on stderr when it is not. **And the failure branch is now tested.** It needs `rmSync` to hit EPERM/EBUSY, which nothing portable forces: root bypasses the permission lever, CI's unprivileged user behaves differently, and a `node:fs` module mock does not reach this module under the suite's config (verified — the mock applies to the fs namespace but `git.ts`'s binding stays real). So the ruling is a pure function, `worktreeReleaseResult`, tested for all five shapes — same treatment the two probe-detail helpers got, for the same reason. Full review suite green (428). * fix(review): un-strand releaseWorktree's doc; stop "Nothing to clean" contradicting stderr Two from review, both mine. - Extracting `worktreeReleaseResult` left `releaseWorktree`'s doc comment stranded: the block describing it (the `prune`-before-`branch -D` deadlock, the registered-but-missing worktree) ended up above the new pure helper, and `releaseWorktree` itself was left undocumented. Moved back where it belongs. - `cleanup.ts` treated "failed to remove" as if nothing had happened: the stderr line went out, but `removedAny` stayed false, so the run went on to announce `Nothing to clean for target "…"` on stdout — the two streams contradicting each other, and the stdout half being the one a script reads. "Nothing to clean" is a claim about the tree, not about this run's luck: it is only true when there was nothing there, not when there was and we could not get rid of it. Tracked with a separate `failedAny`, which also closes the same latent contradiction on the temp-file removal path, where it predates this PR.
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
4 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
The test-efficacy probe (
qwen review test-efficacy) used to revert the PR's source to base in the shared review worktree and restore it in afinally. That shared tree is the one every Step 3 review agent reads, and the in-place mutate/restore was the root cause of two findings on #6790. This runs the probe in its own disposable worktree instead, and discards it wholesale.Closes #6832.
Why
Both findings share one cause — mutating a live, shared tree — and this one change retires both:
safeRmWithinstays only as belt-and-suspendersHow
.qwen/tmp/review-pr-<n>-probe), reverted there, and removed withgit worktree remove --forcewhen the probe finishes.node_modulesneeds no per-tree install: the probe tree is nested under the repo, sonpx vitestresolves upward to the repo-rootnode_modules— exactly how the shared worktree already runs vitest. This is the assumption that had the refactor deferred; it was confirmed empirically before relying on it.restoreFailure/ non-zero exit becomes a softcleanupFailurewarning — a leftover probe worktree corrupts nothing and is swept at the next run'sworktree addand bycleanup.ts.Verification
New
test-efficacy.integration.test.tsdrives the real handler over real git worktrees (with a stubvitestbin so it is fast, deterministic, and network-free). It locks down:gated; one that passes isinert;git statusporcelain + HEAD compared);safeRmWithinrefusing — isolation alone protects it (asserted by the absence of a "refusing to delete through a symlink" inconclusive probe).prettier/eslint/tsc --noEmitclean; full review suite green (276 tests, +2 integration).Scope
Pure refactor of where the probe runs and what it cleans up. The verdict logic (
classifyProbeRun,planTestEfficacy,isWorkspaceMember) and the report shape are unchanged exceptrestoreFailure→cleanupFailure, which no consumer reads.