Repository navigation
Description
Problem
isGitCheckpointable in CheckpointService.ts ends in Effect.orElseSucceed(() => false). That makes two very different situations indistinguishable:
- this directory is not a git repository, which is a legitimate "not checkpointable"; and
- git was asked and failed (a timeout, a lock, a corrupt repo), which is an error.
It is called from captureBaseline, capture availability, and capture. So a transient git rev-parse failure at capture time makes the turn commit with status: "missing" instead of a checkpoint. No checkpoint exists, rollback is impossible, and nothing signals that anything went wrong.
Why this is not simply fixed
Removing the orElseSucceed makes git failures propagate, which is what we want in isolation. The problem is the one caller that has no handler:
CheckpointCaptureServicecallsmaterializeBaselineCheckpoint/capturewith no recovery.RunFinalizationService.finalize(RunFinalizationService.tsaround thecheckpointCapture.execute(...)call) maps that error toRunFinalizationErrorand does not catch it.EffectWorkermaps that toOrchestrationEffectExecutionErrorand retries tomaxAttempts(5) beforeoutbox.fail.
So one flaky git rev-parse at the end of a turn would burn five attempts and leave the effect terminally failed. Compare the neighbouring provider-thread.rollback effect case, which attaches Effect.tapCause and dispatches a user-visible failure event instead.
The other call site, RunExecutionService, catches and logs, so it is unaffected.
The question
Should a single transient git failure be able to fail a whole run's finalization?
Three defensible answers:
- No -- capture should degrade to
status: "missing"and continue, but record that it was a failure rather than a non-repo, so the run still settles and the cause is visible. - Yes -- a turn whose checkpoint could not be captured should not be treated as fully settled, because the user may later find they cannot rewind it.
- Retry harder first -- distinguish a timeout from a non-repo, retry the git call, and only propagate after retries are exhausted.
Option 1 preserves current run-settlement behaviour while still fixing the silent-data-loss aspect, and is the smallest change. Option 3 is the most correct but adds retry policy to a path that currently has none.
Suggested shape if (1) is chosen
Keep the distinction in isGitCheckpointable (a repo-detection failure is not a non-repo), and add a recovery at the RunFinalizationService call site that records a missing checkpoint carrying the real cause, then continues. That keeps the honest signal without making finalization fail.
A proof would be a test that drives a failing isGitRepository through RunFinalizationService.finalize and asserts the run still reaches a terminal state.
Environment
Found by a static audit of the merged orchestrator-v2 work (#2829). Re-verified present at main. Deliberately kept out of #16863 so that PR stays a clean two-fix ask.
Activity