Skip to content

fix(server): harden the rollback failure marker - #13911

Merged
juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/rollback-failure-hardening
Sep 28, 2026
Merged

juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/rollback-failure-hardening

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

#13887 added a rollbackFailure marker so a rewind that fails for good rejects instead of hanging. Macroscope flagged four problems with it after the merge: any client could forge the marker, a late failure from an older rollback could overwrite a newer one, raw lower-level error text reached clients, and one import broke the Effect service convention. This PR fixes all four.

What changed

  • Clients can't send the fail command anymore. checkpoint.rollback.fail was a member of OrchestrationV2Command, which is the orchestration.dispatchCommand payload. That meant any operate-scope client could plant a rollbackFailure and make a real rewind reject. It now lives in a server-only OrchestrationV2InternalCommand, following the V1 InternalOrchestrationCommand split. OrchestratorV2.dispatch and ThreadManagementService.dispatch accept OrchestrationV2ServerCommand (client plus internal). The RPC schema still uses the client union, so the socket rejects the command at decode time.
  • A superseded failure is ignored. Accepting a rollback now records thread.rollbackRequestId (same pattern as titleRegeneration.requestId) and clears any old failure in the same thread.metadata-updated event. checkpoint.rollback.fail is rejected when its requestId is no longer the current rollback. Before this, A's last retry could run after B was accepted, because the outbox claims by available_at and doesn't serialize per thread. A would then write a failure on top of B.
  • Clients get a fixed message. The worker now records ROLLBACK_FAILED_MESSAGE instead of Cause.findErrorOption(cause).message. The full cause is still logged server-side by the worker's existing per-attempt "Orchestration effect execution failed" warning (Cause.pretty, nested causes included).
  • Effect convention. EffectWorker.ts imports CheckpointRollbackService as a namespace.

Side effects of the change: a rollback now always emits one thread.metadata-updated before checkpoint.rollback-requested, and two existing assertions were updated for that. Four test fakes that record dispatched commands are retyped to the server union, and a dead scenario-testkit case is removed.

Verification

  • packages/contracts: vp test run src/rpc.test.ts src/orchestrationV2.test.ts: 32 passed. The new test decodes checkpoint.rollback.fail against the real dispatchCommand payload schema and expects a failure. I confirmed it fails against the old contracts.
  • apps/server: vp test run src/orchestration-v2/runtimeLayer.test.ts: 46 passed. That includes the fix(server): a failed rollback shows an error instead of hanging #13887 test and the new "ignores a late failure from a rollback that a newer one superseded": rollback A spends 4 of 5 attempts, B is accepted, A's last attempt fails. The test asserts no marker appears, then that B's own failure is still recorded. With the guard removed this test fails.
  • apps/server: EffectWorker, CheckpointRollbackService, ThreadManagementService, ThreadMessageIntake, linkCreatedPullRequest, pullRequests handlers, and Scheduler integration tests: all passed. ThreadFork integration: 6 passed. Rollback replay fixtures (OrchestratorReplayFixtures.integration.test.ts -t rollback): 5 passed.
  • apps/web: vp test run src/components/ChatView.logic.test.ts -t waitForRevertedMessage (the fix(server): a failed rollback shows an error instead of hanging #13887 web tests): 2 passed.
  • tsc --noEmit clean for contracts, server, client-runtime, web, mobile. vpr knip:check clean. vp lint on touched files: no new warnings (4 existing warnings are on lines this PR doesn't touch).
  • Not run: repo-wide test and typecheck (left to CI), any live provider run, any real client.

Model: Claude Opus 5.5 (Claude Code)

🤖 Generated with Claude Code


Devin Review

checkpoint.rollback.fail moves out of the public OrchestrationV2Command
union into a server-only internal command, so no client can dispatch it
through orchestration.dispatchCommand. The thread now records its current
rollback request, and a failure from a superseded rollback is rejected
instead of overwriting the marker. The worker records a fixed message
rather than the lower-level error text, which stays in the server log.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 4.9 KiB — 6.8 KiB ✅
Codex Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Codex Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.4 KiB — 29.3 KiB ✅
Codex Live turn messages — 2 — 8 ✅
Claude Total thread wire — 4.9 KiB — 6.8 KiB ✅
Claude Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Claude Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Claude Live turn WebSocket decoded — 20.8 KiB — 29.3 KiB ✅
Claude Live turn messages — 2 — 8 ✅

Baseline: unavailable · PR result: d7df34d · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 106.1 KiB
  • Claude decoded thread snapshot: 106.4 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@macroscopeapp

macroscopeapp Bot commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This hardening changes both the client/server command boundary and the production rollback state machine, including rejection of forged or superseded failure markers. The scope is focused and tested, but the direct security and integrity implications warrant additional human review.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge
juliusmarminge merged commit b2ed076 into t3code/codex-turn-mapping Sep 28, 2026
23 of 24 checks passed
@juliusmarminge
juliusmarminge deleted the v2/rollback-failure-hardening branch September 28, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant