fix(review): persist findings even when finalize is cancelled

80dd3d8→main·

dipree·2w ago·2 files·+47 added/-0 removed

When the user Ctrl+C's out of a slow final-report/synthesis step, the run context is cancelled AFTER summary.Cancelled was computed for already- finished workers. The summary.Cancelled guard therefore passes, but the git/disk work in writePostReviewManifest then ran on the cancelled context and failed with "write to disk failed: resolve git common dir: git rev-parse: context canceled" — silently discarding findings the user waited minutes for.

Detach persistence from the run context (context.WithoutCancel) with its own 30s deadline so completed findings survive a cancel during finalize. A genuinely cancelled run still skips via the existing summary.Cancelled guard.

Sessions

f565e1556184View transcript

Changes

2

• cmd/entire/cli/review

• Mcmd.go+11

• Mmanifest_test.go+36

1442 unmodified lines

1443
1444
1445
1446
1447
1448
1449
1450
1451
1452
1453
1454
1455
1456
1457
1458
1459

1442 unmodified lines

if summary.Cancelled || len(summary.AgentRuns) == 0 {
        return
    }

// The findings were produced by worker sessions that already finished. If
    // the user Ctrl+C's out of a slow final-report/synthesis step, the run
    // context is cancelled AFTER summary.Cancelled was computed (so the guard
    // above doesn't fire), and the git/disk work below would fail with
    // "context canceled" — silently discarding findings the user waited
    // minutes for. Detach from the run context and give persistence its own
    // short deadline so completed findings survive a cancel during finalize.
    ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second)
    defer cancel()

manifest, states, err := localReviewManifestFromCurrentState(ctx, worktreeRoot, headSHA, summary, aggregateOutput)
    if err != nil {
        logging.Debug(ctx, "review manifest not written", slog.String("error", err.Error()))

Mcmd/entire/cli/review/cmd.go+11

331 unmodified lines

332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373

331 unmodified lines

}
}

// TestWritePostReviewManifest_SurvivesCancelledRunContext pins the fix for
// findings being silently discarded when the user Ctrl+C's out of a slow
// final-report step: the workers already finished (summary not cancelled), so
// persistence must run on a detached context and not fail with
// "context canceled" just because the run context was cancelled afterward.
func TestWritePostReviewManifest_SurvivesCancelledRunContext(t *testing.T) {
    repoRoot := t.TempDir()
    testutil.InitRepo(t, repoRoot)
    t.Chdir(repoRoot)

// Simulate the run context cancelled during finalization.
    ctx, cancel := context.WithCancel(context.Background())
    cancel()

var out strings.Builder
    summary := reviewtypes.RunSummary{
        StartedAt: time.Now(),
        AgentRuns: []reviewtypes.AgentRun{
            {Name: "claude-code", Status: reviewtypes.AgentStatusSucceeded},
        },
    }

writePostReviewManifest(ctx, &out, repoRoot, "abc123", summary, "")

got := out.String()
    // The git/disk work must have run despite the cancelled input context: we
    // reach the normal "no matching sessions" path, not a context-canceled
    // failure.
    if strings.Contains(got, "context canceled") {
        t.Fatalf("persistence used the cancelled run context; findings would be lost:\n%s", got)
    }
    if !strings.Contains(got, "no session states found") {
        t.Fatalf("expected persistence to proceed to the no-session-state path; got:\n%s", got)
    }
}

func TestExplainEmptyManifest_NoStates(t *testing.T) {
    t.Parallel()
    summary := reviewtypes.RunSummary{