fix(server): exclude between-turn edits from V2 turn diffs - #10197
fix(server): exclude between-turn edits from V2 turn diffs#10197emmsixx wants to merge 5 commits into
Conversation
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This change introduces per-turn Git baselines and a new cleanup workflow across checkpoint capture, terminal handling, recovery, and outbox execution, so its runtime and persistence impact is broader than a small bug fix. An unresolved concrete concern about abandoned refs and cleanup behavior further requires human verification. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edbfa5afa3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| yield* checkpointStore.captureCheckpoint({ | ||
| cwd: input.scope.cwd, | ||
| checkpointRef, | ||
| checkpointRef: startRef, | ||
| }); |
There was a problem hiding this comment.
Remove start refs for runs that do not complete
When a provider turn is interrupted, fails, or is cancelled after captureBaseline, this newly created *-start ref is never reclaimed: RunExecutionService only schedules checkpoint capture for completed terminals, and rollback cleanup only receives projected ready checkpoints. Repeated stopped or failed runs therefore accumulate untracked Git refs indefinitely and pin their snapshot commits; add terminal cleanup for start refs that will never receive a completed checkpoint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f536a0b. A replay-safe checkpoint.baseline.cleanup effect is emitted when a run settles without a checkpoint, when a start is cancelled before the provider launches, when capture permanently fails, and during restart recovery for orphaned runs. The handler re-reads the projection and discards only a terminal run's own start ref under the workspace lock, so it is idempotent and completed turns keep their baseline for historical diffs.
Interrupted, failed, and cancelled V2 turns never reach checkpoint capture, so the `-start` snapshot taken before the turn stayed behind and kept its objects reachable. Cleanup only ran for ready checkpoints. Emit a `checkpoint.baseline.cleanup` effect when a run settles without a checkpoint, when a start is cancelled before the provider launches, when capture permanently fails, and during recovery for runs orphaned by a restart. The handler re-reads the projection and discards only a terminal run's own start ref under the workspace lock, so it is idempotent and leaves completed turns' baselines intact for historical diffs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| (candidate) => | ||
| candidate.scopeId === scope.id && candidate.ordinalWithinScope === run.ordinal, | ||
| ); | ||
| if (checkpoint?.status === "ready") return; |
There was a problem hiding this comment.
🟡 Medium orchestration-v2/CheckpointCaptureService.ts:266
A terminal run with a missing checkpoint enqueues cleanup, but checkpoints.discardBaseline fails in non-Git workspaces because CheckpointStore.deleteCheckpointRefs cannot resolve a repository, leaving the outbox effect failing and retrying indefinitely. Skip cleanup when the checkpoint status is missing.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration-v2/CheckpointCaptureService.ts around line 266:
A terminal run with a `missing` checkpoint enqueues cleanup, but `checkpoints.discardBaseline` fails in non-Git workspaces because `CheckpointStore.deleteCheckpointRefs` cannot resolve a repository, leaving the outbox effect failing and retrying indefinitely. Skip cleanup when the checkpoint status is `missing`.
There was a problem hiding this comment.
Real bug, fixed in 8a9d8bd — but guarded differently than suggested.
Skipping only on missing status would leave the leak half-open: missing requires a checkpoint row to exist, and the interrupted, failed, and cancelled paths enqueue cleanup with no row at all. Those would still resolve a driver, still fail, still retry.
The precondition for deleting a ref is that the workspace is a Git repository, so discardBaseline now gates on isGitCheckpointable, matching captureBaseline, capture, and restore — it was the only write path missing it. A workspace with no repository never stored a start ref.
Covered by a test that fails with exactly VcsUnsupportedOperationError: No supported VCS repository was detected when the guard is removed.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
Baseline cleanup resolves a VCS driver to delete the start ref, which fails with VcsUnsupportedOperationError when the workspace holds no repository. The cleanup runs from the outbox, so that failure retried forever instead of settling. Gate discardBaseline on isGitCheckpointable, matching captureBaseline, capture, and restore. A workspace with no repository never stored a start ref, so there is nothing to reclaim on any of the paths that enqueue cleanup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>



What Changed
Capture a separate starting snapshot for each V2 turn and use it for the changed-files summary and individual turn diff. Completed checkpoints remain intact for history and restore. Older checkpoints fall back to the existing baseline, and rollback cleanup removes the corresponding starting snapshots.
Turns that never reach a checkpoint now release their starting snapshot. A
checkpoint.baseline.cleanupeffect is emitted when a run settles without a checkpoint, when a start is cancelled before the provider launches, when capture permanently fails, and during recovery for runs orphaned by a restart. The handler re-reads the projection and discards only a terminal run's own start ref under the workspace lock, so it is idempotent and leaves completed turns' baselines intact for historical diffs.Why
An external commit made between turns currently appears as changes from the next agent reply, even when that reply edits no files. For example, append two lines to README.md and commit them while the thread is idle, then ask the agent to reply without editing files: the reply gets a misleading README.md +2 card.
This addresses edits made between turns. Changes made during a turn remain part of that turn's diff; this does not attempt Git authorship attribution.
Draft against the orchestration V2 branch from #2829, related to #4249. Closes #10348. The main backport is available at https://github.com/emmsixx/t3code/tree/fix/turn-diff-baseline-main. Retargeting to main will be needed after V2 lands.
Validation
paginated threads do not support thread/rollback. The same error was reproduced on the unpatched nightly. Browser-controlled V2 verification reproduced the same outcome: files restored correctly, but conversation rollback failed with the same Codex pagination error. Full client rollback is therefore not marked as passing.Review follow-ups
VcsUnsupportedOperationErrorin a workspace holding no repository and, running from the outbox, retried forever.discardBaselinenow gates onisGitCheckpointable, matchingcaptureBaseline,capture, andrestore; such a workspace never stored a start ref. Covered by a test that fails with that exact error when the guard is removed.missingcheckpoint status was not taken:missingrequires a checkpoint row, so it would leave the interrupted, failed, and cancelled paths still failing and retrying.Before ready for review
Implemented with GPT-6 through the Codex harness in T3 Code. Review follow-ups were implemented with Opus 5 in T3 Code after Codex usage ran out.
Note
Exclude between-turn edits from V2 turn diffs via start-snapshot baselines
CheckpointServiceV2.captureBaselinenow idempotently records both the ordinary checkpoint ref and the next ordinal's start ref; duplicate deliveries do not replace the saved baseline.discardBaselinetoCheckpointServiceV2andCheckpointCaptureServiceV2, wired through a new replay-safecheckpoint.baseline.cleanupeffect in the outbox and effect executor.RunExecutionService.writeFinalRunEventschanged terminal effect selection — non-completed terminal statuses no longer emit capture and instead emitcheckpoint.baseline.cleanup; any out-of-tree consumers expecting capture effects on failed/interrupted runs will not receive them.Macroscope summarized 8a9d8bd.