* test(#2852): add failing-first regression coverage for wave-cleanup isolation Adds the #2852 test matrix to executeWorktreeWaveCleanupPlan: per-entry block reasons must isolate to the blocked entry instead of aborting the rest of the wave, and a deletion must only block when another wave member's branch still touches the deleted path. These fail against the current implementation (RED) — the fix lands in the next commit. * fix(#2852): isolate wave-cleanup blocks to their own entry and scope the deletions guard to real dependents executeWorktreeWaveCleanupPlan aborted the rest of a cleanup wave on the first blocked entry (branch_mismatch, base_mismatch, worktree_dirty, merge_failed, etc.), dumping every remaining entry into `pending` untouched instead of evaluating it. Every per-entry block reason now isolates via `continue` instead of `break` + bulk pending push. The one exception is a failed --no-ff merge, which can leave repoRoot itself mid-merge: that path now attempts `git merge --abort` and only halts the remaining wave if the abort itself fails (an unrecoverable repo-level failure), matching every other block reason's isolation. The `branch_contains_deletions` guard also blocked any deletion unconditionally, even one nothing else in the wave depends on (the reported repro: folding a test file into a sibling and deleting the original). It now blocks only when another wave member's branch still touches the deleted path — computed lazily per wave so a run with no deletions pays no extra git call, and fails closed (still blocks) when a sibling's diff cannot be determined. * fix(#2852): eagerly cache each entry's own diff to fix an ordering bug in the deletions-overlap check The deletions cross-entry overlap check (previous commit) computed each "other" entry's touched-files set lazily, the first time some later entry's overlap check needed it. That is wrong: once an entry has already been merged earlier in the same loop pass, its branch becomes an ancestor of HEAD, and `git diff --name-only HEAD...branch` silently collapses to empty. A dependent entry that appears BEFORE the deleting entry in the manifest (and has therefore already merged by the time the deletion check runs) would be missed, letting a genuinely-depended-on deletion through undetected — a live violation of the negative-space acceptance criterion, caught by /code-review's Spec-axis before this shipped. Fixed by populating each entry's touched-files cache eagerly, during that entry's own turn in the loop, immediately before its own merge attempt (the only step that can move HEAD) — so every entry's diff is captured before it could possibly have been merged, regardless of manifest order. Adds a regression test reproducing the exact broken ordering (dependent merges first, then a later entry tries to delete the file it depends on). * refactor(#2852): extract shared git name-only line parser /code-review's Standards axis flagged duplicated parsing logic: `stdout.split('\n').map((l) => l.trim()).filter(Boolean)` appeared at both the per-entry deletion list and the cross-entry touched-files cache added by this fix. Extracted into parseGitNameOnlyLines(), used by both call sites, so the two can't silently drift apart. * revert(#2852): scope the fix to wave-isolation only, restore unconditional deletions guard #2852's own triage comment explicitly deferred the deletions-guard policy question as a separate product decision ("Policy/enhancement ask, not a defect ... Out of scope: deciding or implementing an opt-in mechanism for intentional deletions"). All four of the issue's actual acceptance criteria concern wave isolation only. The prior two commits on this branch built a cross-entry deletion-dependency heuristic that substituted a derived judgment for that deferred product decision — out of scope for a confirmed-bug fix. Reverts: getEntryChangedFiles, touchedFilesCache, the overlapUnknown fail-closed branch, parseGitNameOnlyLines, and the eager per-turn cache-population call. `branch_contains_deletions` now blocks unconditionally again (byte-identical trigger condition to pre-fix); the only change is `continue` instead of `break` + bulk `pending.push`, same as the other 7 block reasons. Keeps: the full wave-isolation fix (all 8 sites) and the merge_failed / git merge --abort recovery-and-carve-out, both squarely inside the issue's actual acceptance criteria. The deferred opt-in-for-intentional-deletions decision is filed as #3003, citing #2852's triage as origin. * refactor(#2852): extract blockEntry() helper to remove duplicated block-assembly across 8 sites /code-review's Standards axis flagged the repeated "result.status='blocked'; result.reason=...; result.stderr=...; results.push(result); ok=false;" shape at every one of the 8 per-entry block sites this fix touches. Extracted into blockEntry(), called at each site; each call site still owns its own continue/break decision. No behavior change. * fix(#2852): check actual repo state instead of git merge --abort's exit code The merge_failed recovery path decided "genuinely unrecoverable, halt the wave" based on whether `git merge --abort` itself exited successfully. That is not a reliable signal: git refuses many merges (e.g. "your local changes would be overwritten by merge") WITHOUT ever creating a MERGE_HEAD, in which case repoRoot's tree was never touched — but `git merge --abort` still fails with "There is no merge to abort (MERGE_HEAD missing)?" in that exact safe case. Trusting that exit code alone misclassified an ordinary per-entry merge failure as a repo-level one and stranded the rest of the wave — the exact defect #2852 exists to fix, reintroduced through the recovery path (caught in review). Fixed by checking repoRoot's actual state directly via `git rev-parse --verify -q MERGE_HEAD` after the abort attempt: MERGE_HEAD present means genuinely still mid-merge (unrecoverable, halt); absent means safe (isolate and continue), whether because no merge state was ever entered or because abort successfully cleared it. An unexpected git error or timeout degrades to the conservative "still mid-merge" answer rather than throwing or guessing. Rewrites the "unrecoverable merge_failed" test, which previously used the safe "There is no merge to abort" string as its unrecoverable example — that pinned the defect as correct behavior. Adds the missing case: an ordinary merge_failed that never entered a merge state must not abort the wave. * test(#2852): cover repoRootStillMidMerge's fail-closed branches /code-review flagged that the two conservative fail-closed branches of repoRootStillMidMerge (a timeout on the post-abort MERGE_HEAD check, and an unexpected non-0/1 exit code such as a fatal git error) had no test coverage — exactly the branches most likely to hide a mutation survivor (e.g. a flipped `timedOut` check or a flipped final `return true`). Adds both cases: each must halt the wave (fail closed) rather than assume repoRoot is safe when its state cannot be verified. * chore(#2852): backfill changeset PR number to 3009 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -519,6 +519,36 @@ function gitResultOk(result: GitResult | null | undefined): boolean {
|
||||
return !!(result && result.exitCode === 0 && !result.timedOut);
|
||||
}
|
||||
|
||||
/**
|
||||
* #2852: after a failed `git merge` + a `git merge --abort` attempt, determine
|
||||
* whether `repoRoot` is STILL mid-merge — the only condition that genuinely
|
||||
* invalidates the rest of a cleanup wave.
|
||||
*
|
||||
* `git merge --abort`'s own exit code is NOT a reliable signal here: git refuses
|
||||
* many merges (e.g. "your local changes to the following files would be
|
||||
* overwritten by merge") WITHOUT ever creating a `MERGE_HEAD`, in which case
|
||||
* `repoRoot`'s tree was never touched and `git merge --abort` correctly fails
|
||||
* with "fatal: There is no merge to abort (MERGE_HEAD missing)?" — a SAFE
|
||||
* outcome, not a broken one. Trusting that exit code alone would misclassify an
|
||||
* ordinary per-entry merge failure as a repo-level one and strand the rest of
|
||||
* the wave (caught in review).
|
||||
*
|
||||
* Checked directly via `git rev-parse --verify -q MERGE_HEAD` against the git
|
||||
* ref itself rather than the filesystem: exit 0 means a merge is genuinely still
|
||||
* in progress (unrecoverable — halt); exit 1 (the ref simply doesn't exist) means
|
||||
* repoRoot is clean, whether because no merge state was ever entered or because
|
||||
* abort successfully cleared it (safe — isolate and continue). Anything else
|
||||
* (a timeout, or an unexpected git error) is treated conservatively as "still
|
||||
* mid-merge" — degrade to the safe/halting answer rather than throw or guess.
|
||||
*/
|
||||
function repoRootStillMidMerge(execGit: ExecGitFn, repoRoot: string): boolean {
|
||||
const check = execGit(['rev-parse', '--verify', '-q', 'MERGE_HEAD'], { cwd: repoRoot });
|
||||
if (check.timedOut) return true; // fail closed — cannot confirm safety
|
||||
if (check.exitCode === 0) return true; // MERGE_HEAD exists — genuinely still mid-merge
|
||||
if (check.exitCode === 1) return false; // ref not found — repoRoot is not mid-merge
|
||||
return true; // any other exit code (e.g. a fatal git error) — fail closed
|
||||
}
|
||||
|
||||
/**
|
||||
* Walk <worktreePath>/.planning/ recursively and collect absolute paths of
|
||||
* all files whose names match *SUMMARY.md. Returns [] when the directory
|
||||
@@ -679,6 +709,19 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
|
||||
const pending: CleanupManifestEntry[] = [];
|
||||
let ok = true;
|
||||
|
||||
// #2852: every per-entry failure site marks the SAME shape — status='blocked',
|
||||
// a reason code, the captured stderr, push to results, flip the overall `ok`
|
||||
// flag — and then either `continue` (isolate, the default) or, for the one
|
||||
// repo-level-failure carve-out, `break`. Factored out so the 8 call sites below
|
||||
// don't repeat the assembly; each site still owns its own control-flow decision.
|
||||
function blockEntry(result: WaveCleanupEntryResult, reason: string, stderr: string): void {
|
||||
result.status = 'blocked';
|
||||
result.reason = reason;
|
||||
result.stderr = stderr;
|
||||
results.push(result);
|
||||
ok = false;
|
||||
}
|
||||
|
||||
for (let i = 0; i < entries.length; i += 1) {
|
||||
const entry = entries[i];
|
||||
const result: WaveCleanupEntryResult = {
|
||||
@@ -690,13 +733,10 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
|
||||
|
||||
const branchCheck = execGit(['-C', entry.worktree_path, 'rev-parse', '--abbrev-ref', 'HEAD'], { cwd: plan.repoRoot });
|
||||
if (!gitResultOk(branchCheck) || branchCheck.stdout.trim() !== entry.branch) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'branch_mismatch';
|
||||
result.stderr = branchCheck?.stderr || '';
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
blockEntry(result, 'branch_mismatch', branchCheck?.stderr || '');
|
||||
// #2852: isolate — this entry's problem does not touch repoRoot's git state,
|
||||
// so every remaining entry is still independently evaluated.
|
||||
continue;
|
||||
}
|
||||
|
||||
const mergeBase = execGit(['merge-base', 'HEAD', entry.branch], { cwd: plan.repoRoot });
|
||||
@@ -704,33 +744,23 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
|
||||
? entry.allowed_bases
|
||||
: [entry.expected_base];
|
||||
if (!gitResultOk(mergeBase) || !allowedBases.includes(mergeBase.stdout.trim())) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'base_mismatch';
|
||||
result.stderr = mergeBase?.stderr || '';
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
blockEntry(result, 'base_mismatch', mergeBase?.stderr || '');
|
||||
continue; // #2852: isolate
|
||||
}
|
||||
|
||||
const deletions = execGit(['diff', '--diff-filter=D', '--name-only', `HEAD...${entry.branch}`], { cwd: plan.repoRoot });
|
||||
if (!gitResultOk(deletions)) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'deletion_check_failed';
|
||||
result.stderr = deletions?.stderr || '';
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
blockEntry(result, 'deletion_check_failed', deletions?.stderr || '');
|
||||
continue; // #2852: isolate
|
||||
}
|
||||
if (deletions.stdout) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'branch_contains_deletions';
|
||||
result.stderr = deletions.stdout;
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
// Unconditional: any deletion in this entry's branch blocks THIS entry. Whether
|
||||
// that guard should have an opt-in for intentional deletions is a deferred
|
||||
// product decision (issue #2852's own triage scoped it out — tracked in #3003);
|
||||
// this fix only isolates the block to this one entry (#2852) instead of aborting
|
||||
// the rest of the wave, same as every other block reason below.
|
||||
blockEntry(result, 'branch_contains_deletions', deletions.stdout);
|
||||
continue; // #2852: isolate
|
||||
}
|
||||
|
||||
// Safety net: rescue uncommitted SUMMARY.md artifacts before the dirty check.
|
||||
@@ -738,24 +768,14 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
|
||||
// orchestrator commits it. Mirrors quick.md shell fallback (#2296, #2070, #2838, #3804).
|
||||
const { rescuedRelPaths, failures: rescueFailures } = rescueSummaryArtifacts(entry.worktree_path, plan.repoRoot, deps);
|
||||
if (rescueFailures.length > 0) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'summary_rescue_failed';
|
||||
result.stderr = rescueFailures.map((f) => `${f.relPath}: ${f.error}`).join('; ');
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
blockEntry(result, 'summary_rescue_failed', rescueFailures.map((f) => `${f.relPath}: ${f.error}`).join('; '));
|
||||
continue; // #2852: isolate
|
||||
}
|
||||
|
||||
const worktreeStatus = execGit(['-C', entry.worktree_path, 'status', '--porcelain', '--untracked-files=all'], { cwd: plan.repoRoot });
|
||||
if (!gitResultOk(worktreeStatus)) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'worktree_dirty';
|
||||
result.stderr = worktreeStatus?.stderr || '';
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
blockEntry(result, 'worktree_dirty', worktreeStatus?.stderr || '');
|
||||
continue; // #2852: isolate
|
||||
}
|
||||
// Filter rescued SUMMARY paths out of the porcelain output before deciding dirty.
|
||||
// A line like "?? .planning/q1-SUMMARY.md" should not block when the SUMMARY
|
||||
@@ -769,24 +789,32 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
|
||||
return !rescuedRelPaths.has(filePath);
|
||||
});
|
||||
if (dirtyLines.length > 0) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'worktree_dirty';
|
||||
result.stderr = dirtyLines.join('\n');
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
blockEntry(result, 'worktree_dirty', dirtyLines.join('\n'));
|
||||
continue; // #2852: isolate
|
||||
}
|
||||
|
||||
const merge = execGit(['merge', entry.branch, '--no-ff', '--no-edit', '-m', `chore: merge executor worktree (${entry.branch})`], { cwd: plan.repoRoot });
|
||||
if (!gitResultOk(merge)) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'merge_failed';
|
||||
result.stderr = merge?.stderr || merge?.stdout || '';
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
blockEntry(result, 'merge_failed', merge?.stderr || merge?.stdout || '');
|
||||
// #2852: a failed --no-ff merge MIGHT leave repoRoot itself mid-merge
|
||||
// (MERGE_HEAD set, conflict markers in the tree) — unlike every other block
|
||||
// reason above, that specific state is NOT scoped to this one entry: a second
|
||||
// `git merge` cannot even start while one is in progress, so every remaining
|
||||
// entry would be corrupted by it. But git also refuses many merges WITHOUT ever
|
||||
// entering a merge state (e.g. "your local changes would be overwritten by
|
||||
// merge") — in that case repoRoot's tree was never touched and this failure is
|
||||
// scoped to this entry, same as everything else. Attempt the abort as a
|
||||
// best-effort cleanup, then check repoRoot's ACTUAL state directly — not
|
||||
// `git merge --abort`'s own exit code, which fails "There is no merge to abort"
|
||||
// in the safe case too and would misclassify it as unrecoverable (caught in
|
||||
// review). Only a repo genuinely still mid-merge afterward legitimately halts
|
||||
// the rest of the wave (the brief's "infrastructure-level failure" carve-out).
|
||||
execGit(['merge', '--abort'], { cwd: plan.repoRoot });
|
||||
if (repoRootStillMidMerge(execGit, plan.repoRoot)) {
|
||||
pending.push(...entries.slice(i + 1));
|
||||
break;
|
||||
}
|
||||
continue; // #2852: isolate — repoRoot is not (or no longer) mid-merge
|
||||
}
|
||||
|
||||
let remove = execGit(['worktree', 'remove', entry.worktree_path, '--force'], { cwd: plan.repoRoot });
|
||||
@@ -798,13 +826,10 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work
|
||||
remove = execGit(['worktree', 'remove', entry.worktree_path, '--force'], { cwd: plan.repoRoot });
|
||||
}
|
||||
if (!gitResultOk(remove)) {
|
||||
result.status = 'blocked';
|
||||
result.reason = 'worktree_remove_failed';
|
||||
result.stderr = remove?.stderr || '';
|
||||
results.push(result);
|
||||
pending.push(...entries.slice(i + 1));
|
||||
ok = false;
|
||||
break;
|
||||
blockEntry(result, 'worktree_remove_failed', remove?.stderr || '');
|
||||
// #2852: isolate — the merge already landed on repoRoot; only this entry's
|
||||
// worktree/branch teardown is affected.
|
||||
continue;
|
||||
}
|
||||
|
||||
const branchDelete = execGit(['branch', '-D', entry.branch], { cwd: plan.repoRoot });
|
||||
|
||||
Reference in New Issue
Block a user