Skip to content

Commit f5ae458

Browse files
committed
fix(TP-005): detect repo-level setup failures in mergeWaveByRepo aggregate status
R002 finding #1: mergeWave() can return status='failed' with failedLane=null for pre-lane setup errors (temp branch creation, worktree creation). mergeWaveByRepo() previously only checked failedLane !== null, missing these setup failures entirely. Fix: Track anyRepoFailed flag based on groupResult.status !== 'succeeded' (not just failedLane). This catches both lane-level failures AND setup failures. firstFailureReason is populated with setup error context when failedLane is null. R002 finding #2: Updated test helper computeAggregateStatus to match the real implementation (uses repoStatuses array instead of single firstFailedLane). Added 4 new test cases for setup-failure scenarios: - repo setup failure with no lanes → failed - repo setup failure + other repo success → partial - all repos setup failure → failed - repo setup failure + other repo partial → partial All 207 tests pass.
1 parent e205796 commit f5ae458

2 files changed

Lines changed: 96 additions & 24 deletions

File tree

extensions/taskplane/merge.ts

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -934,6 +934,11 @@ export function mergeWaveByRepo(
934934
const repoOutcomes: RepoMergeOutcome[] = [];
935935
let firstFailedLane: number | null = null;
936936
let firstFailureReason: string | null = null;
937+
// Track repo-level failures independently of lane-level failures.
938+
// mergeWave() can return status="failed" with failedLane=null for
939+
// pre-lane setup errors (temp branch creation, worktree creation).
940+
// We must detect these to avoid misclassifying the aggregate as "succeeded".
941+
let anyRepoFailed = false;
937942

938943
for (const group of repoGroups) {
939944
const groupRepoRoot = resolveRepoRoot(group.repoId, repoRoot, workspaceConfig);
@@ -977,25 +982,33 @@ export function mergeWaveByRepo(
977982
};
978983
repoOutcomes.push(repoOutcome);
979984

980-
// Track first failure across repos (but continue to merge other repos)
981-
if (groupResult.failedLane !== null && firstFailedLane === null) {
982-
firstFailedLane = groupResult.failedLane;
983-
firstFailureReason = groupResult.failureReason
984-
? `[repo:${group.repoId ?? "default"}] ${groupResult.failureReason}`
985-
: null;
985+
// Track failures across repos (but continue to merge other repos).
986+
// Check groupResult.status (not just failedLane) to catch setup failures
987+
// where mergeWave() returns status="failed" with failedLane=null
988+
// (e.g., temp branch creation or worktree creation failure).
989+
if (groupResult.status !== "succeeded") {
990+
anyRepoFailed = true;
991+
992+
if (firstFailureReason === null) {
993+
firstFailedLane = groupResult.failedLane;
994+
firstFailureReason = groupResult.failureReason
995+
? `[repo:${group.repoId ?? "default"}] ${groupResult.failureReason}`
996+
: `[repo:${group.repoId ?? "default"}] Merge failed (setup error)`;
997+
}
986998
}
987999
}
9881000

9891001
// ── Aggregate status ─────────────────────────────────────────
990-
// Base aggregate status on lane-level evidence (not repo-level status)
991-
// to correctly classify "all repos partial" as global partial, not failed.
1002+
// Use both lane-level and repo-level evidence for correct classification:
1003+
// - anyLaneSucceeded: at least one lane merged successfully across all repos
1004+
// - anyRepoFailed: at least one repo had a non-succeeded status (includes
1005+
// both lane-level failures AND repo setup failures with failedLane=null)
9921006
const anyLaneSucceeded = allLaneResults.some(
9931007
r => r.result?.status === "SUCCESS" || r.result?.status === "CONFLICT_RESOLVED",
9941008
);
995-
const anyLaneFailed = firstFailedLane !== null;
9961009

9971010
let status: MergeWaveResult["status"];
998-
if (!anyLaneFailed) {
1011+
if (!anyRepoFailed) {
9991012
status = "succeeded";
10001013
} else if (anyLaneSucceeded) {
10011014
status = "partial";

extensions/tests/merge-repo-scoped.test.ts

Lines changed: 73 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,19 +6,25 @@
66
* 2. groupLanesByRepo — mono-repo no-regression (single group)
77
* 3. Deterministic failure aggregation across repos
88
* 4. mergeWaveByRepo — repo-mode passthrough
9+
* 5. formatRepoMergeSummary — repo-divergence partial summary (Step 1)
910
*
1011
* Run: npx vitest run extensions/tests/merge-repo-scoped.test.ts
1112
*/
1213

1314
import {
1415
groupLanesByRepo,
1516
determineMergeOrder,
17+
formatRepoMergeSummary,
18+
ORCH_MESSAGES,
1619
} from "../task-orchestrator.ts";
1720

1821
import type {
1922
AllocatedLane,
2023
AllocatedTask,
24+
MergeLaneResult,
25+
MergeWaveResult,
2126
ParsedTask,
27+
RepoMergeOutcome,
2228
} from "../task-orchestrator.ts";
2329

2430
// ── Helpers ──────────────────────────────────────────────────────────
@@ -234,21 +240,28 @@ function runAllTests(): void {
234240
assert(groups[2].repoId === "z-repo", "deterministic: z-repo third");
235241
}
236242

237-
// ─── 9. Status rollup: lane-level evidence (not repo status) ─────
243+
// ─── 9. Status rollup: lane-level + repo-level evidence ──────
238244
// Tests the aggregation logic pattern used in mergeWaveByRepo().
239-
// This validates the fix for R002 finding #2 (all-partial misclassified as failed).
240-
console.log("\n── 9. Status rollup: lane-level evidence ──");
245+
// Validates R002 fixes: all-partial misclassification AND setup-failure detection.
246+
console.log("\n── 9. Status rollup: lane-level + repo-level evidence ──");
241247
{
242-
// Helper: simulate the status rollup logic from mergeWaveByRepo()
248+
// Helper: simulate the status rollup logic from mergeWaveByRepo().
249+
// Uses BOTH lane-level evidence (anyLaneSucceeded) and repo-level evidence
250+
// (anyRepoFailed) to match the actual implementation.
251+
//
252+
// Parameters:
253+
// laneResults: simulated MergeLaneResult[] with result status
254+
// repoStatuses: per-repo status values from each mergeWave() call
255+
// (captures setup failures where failedLane=null but status="failed")
243256
function computeAggregateStatus(
244257
laneResults: Array<{ resultStatus: string | null; error: string | null }>,
245-
firstFailedLane: number | null,
258+
repoStatuses: Array<"succeeded" | "failed" | "partial">,
246259
): "succeeded" | "failed" | "partial" {
247260
const anyLaneSucceeded = laneResults.some(
248261
r => r.resultStatus === "SUCCESS" || r.resultStatus === "CONFLICT_RESOLVED",
249262
);
250-
const anyLaneFailed = firstFailedLane !== null;
251-
if (!anyLaneFailed) return "succeeded";
263+
const anyRepoFailed = repoStatuses.some(s => s !== "succeeded");
264+
if (!anyRepoFailed) return "succeeded";
252265
if (anyLaneSucceeded) return "partial";
253266
return "failed";
254267
}
@@ -257,7 +270,7 @@ function runAllTests(): void {
257270
assert(
258271
computeAggregateStatus(
259272
[{ resultStatus: "SUCCESS", error: null }, { resultStatus: "SUCCESS", error: null }],
260-
null,
273+
["succeeded", "succeeded"],
261274
) === "succeeded",
262275
"rollup: all SUCCESS → succeeded",
263276
);
@@ -266,7 +279,7 @@ function runAllTests(): void {
266279
assert(
267280
computeAggregateStatus(
268281
[{ resultStatus: "SUCCESS", error: null }, { resultStatus: "CONFLICT_UNRESOLVED", error: null }],
269-
2,
282+
["partial"],
270283
) === "partial",
271284
"rollup: mixed SUCCESS + failure → partial",
272285
);
@@ -275,7 +288,7 @@ function runAllTests(): void {
275288
assert(
276289
computeAggregateStatus(
277290
[{ resultStatus: "CONFLICT_UNRESOLVED", error: null }, { resultStatus: "BUILD_FAILURE", error: null }],
278-
1,
291+
["failed"],
279292
) === "failed",
280293
"rollup: all failures → failed",
281294
);
@@ -292,22 +305,22 @@ function runAllTests(): void {
292305
{ resultStatus: "CONFLICT_RESOLVED", error: null }, // repo-b lane 1
293306
{ resultStatus: "BUILD_FAILURE", error: null }, // repo-b lane 2 (failure)
294307
],
295-
2, // first failure at lane 2
308+
["partial", "partial"],
296309
) === "partial",
297310
"rollup: all repos partial → global partial (not failed)",
298311
);
299312

300313
// Case E: No lanes at all (vacuous) → succeeded
301314
assert(
302-
computeAggregateStatus([], null) === "succeeded",
315+
computeAggregateStatus([], []) === "succeeded",
303316
"rollup: no lanes → succeeded (vacuous)",
304317
);
305318

306319
// Case F: Error lanes (no result, only error) → failed
307320
assert(
308321
computeAggregateStatus(
309322
[{ resultStatus: null, error: "spawn failed" }],
310-
1,
323+
["failed"],
311324
) === "failed",
312325
"rollup: error lane without result → failed",
313326
);
@@ -316,10 +329,56 @@ function runAllTests(): void {
316329
assert(
317330
computeAggregateStatus(
318331
[{ resultStatus: "SUCCESS", error: null }, { resultStatus: null, error: "timeout" }],
319-
2,
332+
["partial"],
320333
) === "partial",
321334
"rollup: success + error → partial",
322335
);
336+
337+
// Case H: Repo setup failure (failedLane=null, status="failed", no lane results)
338+
// This is R002 finding #1: temp branch or worktree creation fails before
339+
// any lane merges. mergeWave() returns status="failed" with failedLane=null
340+
// and empty laneResults. The aggregate must detect this as a failure.
341+
assert(
342+
computeAggregateStatus(
343+
[], // no lane results (setup failed before lane merges)
344+
["failed"],
345+
) === "failed",
346+
"rollup: repo setup failure with no lanes → failed",
347+
);
348+
349+
// Case I: One repo setup-fails, another succeeds → partial
350+
// Repo A: setup failure (no lanes merged)
351+
// Repo B: all lanes merged successfully
352+
assert(
353+
computeAggregateStatus(
354+
[{ resultStatus: "SUCCESS", error: null }], // only repo B's lanes
355+
["failed", "succeeded"], // repo A failed setup, repo B succeeded
356+
) === "partial",
357+
"rollup: repo setup failure + other repo success → partial",
358+
);
359+
360+
// Case J: All repos setup-fail → failed
361+
assert(
362+
computeAggregateStatus(
363+
[], // no lane results from any repo
364+
["failed", "failed"],
365+
) === "failed",
366+
"rollup: all repos setup failure → failed",
367+
);
368+
369+
// Case K: One repo setup-fails, another is partial → partial
370+
// Repo A: setup failure (no lanes)
371+
// Repo B: partial (some lanes succeeded, some failed)
372+
assert(
373+
computeAggregateStatus(
374+
[
375+
{ resultStatus: "SUCCESS", error: null }, // repo B lane 1
376+
{ resultStatus: "BUILD_FAILURE", error: null }, // repo B lane 2
377+
],
378+
["failed", "partial"], // repo A setup fail, repo B partial
379+
) === "partial",
380+
"rollup: repo setup failure + other repo partial → partial",
381+
);
323382
}
324383

325384
// ─── 10. repoId propagation on MergeLaneResult ───────────────────

0 commit comments

Comments
 (0)