Skip to content

fix(server): exclude between-turn edits from V2 turn diffs - #10197

Open
emmsixx wants to merge 5 commits into
pingdotgg:t3code/codex-turn-mappingfrom
emmsixx:fix/turn-diff-baseline-v2
Open

fix(server): exclude between-turn edits from V2 turn diffs#10197
emmsixx wants to merge 5 commits into
pingdotgg:t3code/codex-turn-mappingfrom
emmsixx:fix/turn-diff-baseline-v2

Conversation

@emmsixx

@emmsixx emmsixx commented Sep 5, 2026

Copy link
Copy Markdown

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.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 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

  • V2: 85 focused tests pass across CheckpointCaptureService, CheckpointService, RunExecutionService, ProviderRuntimeRecoveryService, EffectWorker, and CheckpointDiffQuery; server typecheck and targeted lint pass.
  • Regression coverage uses real Git repositories and checks external commits, uncommitted idle edits, actual turn edits, duplicate baseline capture, preserved completed snapshots, rollback cleanup, and legacy diff fallback.
  • Cleanup coverage checks that interrupted, failed, and cancelled runs release their start ref, that completed turns keep theirs, that a repeated discard is harmless, and that a run whose capture is still queued keeps its baseline.
  • Main backport: 36 focused tests, server typecheck, and targeted lint pass. The backport does not yet carry the abandoned-snapshot cleanup below; it needs porting before or alongside the retarget.
  • Manual comparison of installed nightly 0.0.39-nightly.20260905.1285 and the main backport: nightly shows the false README.md +2 card; the patched instance shows no card; a later agent edit shows only notes.txt +1.
  • Manual restore on main restored notes.txt and preserved the external README update, but Codex rejected conversation rollback with 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

  • Stopped or failed turns leaked their starting snapshot, because only completed turns reached capture and cleanup only handled ready checkpoints. Fixed by the cleanup effect described above.
  • Baseline cleanup resolved a VCS driver to delete the start ref, so it failed with VcsUnsupportedOperationError in a workspace holding no repository and, running from the outbox, retried forever. discardBaseline now gates on isGitCheckpointable, matching captureBaseline, capture, and restore; such a workspace never stored a start ref. Covered by a test that fails with that exact error when the guard is removed.
  • The suggestion to skip cleanup only for a missing checkpoint status was not taken: missing requires a checkpoint row, so it would leave the interrupted, failed, and cancelled paths still failing and retrying.

Before ready for review

  • Repeat the manual client reproduction on V2: browser-controlled verification on aa3907b confirms no card after the idle external commit, notes.txt +1 after the agent edit, and the expanded patch contains only that added line.
  • Attach the captured before/after screenshots to GitHub.

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

  • Adds a start-snapshot checkpoint reference (derived by appending a suffix to the ordinary checkpoint ref) that captures the turn's initial state, so per-turn file diffs compare against that snapshot instead of the preceding checkpoint — excluding idle edits made between turns.
  • CheckpointServiceV2.captureBaseline now idempotently records both the ordinary checkpoint ref and the next ordinal's start ref; duplicate deliveries do not replace the saved baseline.
  • Adds discardBaseline to CheckpointServiceV2 and CheckpointCaptureServiceV2, wired through a new replay-safe checkpoint.baseline.cleanup effect in the outbox and effect executor.
  • Run terminalization and stale-run recovery now queue baseline cleanup for interrupted, failed, cancelled, superseded, or pre-start-aborted runs; completed runs queue checkpoint capture as before.
  • Risk: RunExecutionService.writeFinalRunEvents changed terminal effect selection — non-completed terminal statuses no longer emit capture and instead emit checkpoint.baseline.cleanup; any out-of-tree consumers expecting capture effects on failed/interrupted runs will not receive them.

Macroscope summarized 8a9d8bd.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 5, 2026
@emmsixx

emmsixx commented Sep 6, 2026

Copy link
Copy Markdown
Author

Before — unpatched nightly

An external commit adds two lines to README.md between turns. The next reply edits nothing, but incorrectly shows README.md +2.

image

After — patched V2

The same external-commit scenario produces no changed-files card on the next no-edit reply.

image

Real edits still appear

A subsequent agent edit correctly shows only notes.txt +1. The external README change is excluded.

image

@emmsixx
emmsixx marked this pull request as ready for review September 6, 2026 01:40
@macroscopeapp

macroscopeapp Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: 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:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines 314 to 317
yield* checkpointStore.captureCheckpoint({
cwd: input.scope.cwd,
checkpointRef,
checkpointRef: startRef,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

emmsixx and others added 2 commits September 5, 2026 23:02
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>
@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Sep 6, 2026
(candidate) =>
candidate.scopeId === scope.id && candidate.ordinalWithinScope === run.ordinal,
);
if (checkpoint?.status === "ready") return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant