Repository navigation
fix(server): reject overlapping v2 checkpoint restore workspaces - #12338
juliusmarminge wants to merge 544 commits into
Conversation
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Effect Service Conventions found 4 namespace-boundary violations. Inline review comments identify the required import and reference updates in Posted via Macroscope — Effect Service Conventions |
1 similar comment
|
Effect Service Conventions found 4 namespace-boundary violations. Inline review comments identify the required import and reference updates in Posted via Macroscope — Effect Service Conventions |
Thread transfer impact
This comment will update automatically after the next completed run. |
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a focused, well-tested server bug fix that rejects checkpoint file restores when another thread owns an overlapping workspace, with dependency wiring confined to the orchestration runtime. An unresolved High-severity finding identifies a race between the overlap check and restore because overlapping scopes do not share a canonical lock, so that issue must be resolved before merge. 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. |
f41150b to
56f1ef9
Compare
This comment has been minimized.
This comment has been minimized.
Translate TodoWrite and ExitPlanMode tool input into canonical todo-list and proposed-plan artifacts so every client can render Claude planning state. Finding: R05 Model: GPT-5.6 Sol via Codex
Load at most one turn-item page per thread in a fork lineage before decoding, keyed by the stable history cursor. Restrict message, plan, and handoff reads to that page plus live actionable state so cold opens and older-page requests no longer decode complete historical tables. Finding: P01 Implemented by GPT-5.6 Sol in Codex.
Budget the serialized bounded projection after retaining live control state. Cap historical control arrays and large plan or handoff details only on the bounded route; the full thread-detail route remains available for complete text. Finding: P02 Implemented by GPT-5.6 Sol in Codex.
Pass the automatic compaction window to Claude and route resume-return dialogs through structured user input so users can compact, continue, or permanently dismiss the prompt. Finding: R06 Model: GPT-5.6 Sol via Codex
Permit the canonical orchestration protocol header in browser API preflights so cross-origin web and desktop clients can negotiate compatibility while retaining authorization and DPoP headers. Finding: D03 Model: GPT-5.6 Sol via Codex
Merge terminal provider updates with stored context usage before replacing the SQLite payload. Keep newer usage reports authoritative and verify the persisted projection after reload. Finding: R07 follow-up Model: GPT-5.6 Sol via Codex
Keep typed plan and todo records distinct from generic tool events, activate captured plans, and supersede older planning state within the owning thread. Ignore nested todo snapshots for the parent and retain identity across duplicate SDK messages. Finding: R05 follow-up Model: GPT-5.6 Sol via Codex
R04 follow-up Convert client multi-select answer arrays to the comma-separated string shape required by the pinned Claude SDK while preserving single-select strings. Implemented by GPT-5.6 Sol via Codex.
Stale cached user and status events could admit and complete a newly submitted OpenCode prompt. Generate the native message ID before submission and only advance admission when that exact message is observed. Finding: R14 Implemented by GPT-5.6 Sol with Codex.
R12 follow-up Insert persistent feedback blocks by their timestamp within the canonical timeline while preserving projected row order. Keep real optimistic sends appended and suppress duplicate local messages already committed by the server. Implemented by GPT-5.6 Sol via Codex.
R20 follow-up Give inspector reasoning markdown its projected source thread and retain the explicit environment fallback for proposed plans without a thread reference. Workspace links and images now resolve through their owning environment after removal of the active-environment fallback. Implemented by GPT-5.6 Sol via Codex.
Read the inclusive cursor, a full history page, and a look-behind row so older history does not terminate after one page. Finding: P01 pagination termination Model: GPT-5.6 Sol via Codex
Cancel pending SDK requests before aborting the native session. Preserve per-admission cancellation state and treat stopped initial prompts as interruption instead of provider failure. Finding: R14 prompt cancellation Model: GPT-5.6 Sol via Codex
Keep the original cursor owner through ancestor traversal and preserve the history budget across empty intermediate forks. Verify exact paged history against the complete nested projection. Finding: P01 nested lineage Model: GPT-5.6 Sol via Codex
Retain pending admission after transient status failures and use one generation-owned retry worker. Ignore stale timers and duplicate evidence so older prompts cannot finish newer steering. Finding: R14 status reconciliation Model: GPT-5.6 Sol via Codex
Keep hidden local and inherited rows from consuming history pages. Preserve stop-request dependencies, source-run cutoffs, and imported history while loading related metadata from the selected cohort and using indexed watermark lookups. Finding: P01 bounded history visibility Model: GPT-5.6 Sol via Codex
56f1ef9 to
02dbeef
Compare
Dismissing prior approval to re-evaluate 02dbeef
| !relative.startsWith(`..${path.sep}`)) | ||
| ); | ||
| }; | ||
| if (otherCwd !== null && (contains(cwd, otherCwd) || contains(otherCwd, cwd))) { |
There was a problem hiding this comment.
🟠 High orchestration-v2/CheckpointRestoreSafety.ts:71
isCheckpointRestoreIsolated can return true and allow checkpoints.restore to proceed even when another thread starts in an overlapping parent or child directory immediately afterward. Because CheckpointService locks the literal scope.cwd, those scopes acquire different locks, so the restore can overwrite the other thread's files; hold a common canonical workspace lock across this check and restore, or revalidate under a lock that serializes overlapping scopes.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/CheckpointRestoreSafety.ts around line 71:
`isCheckpointRestoreIsolated` can return `true` and allow `checkpoints.restore` to proceed even when another thread starts in an overlapping parent or child directory immediately afterward. Because `CheckpointService` locks the literal `scope.cwd`, those scopes acquire different locks, so the restore can overwrite the other thread's files; hold a common canonical workspace lock across this check and restore, or revalidate under a lock that serializes overlapping scopes.
There was a problem hiding this comment.
Confirmed the check-to-restore race. It already exists in the base branch's equality-only guard: another thread can acquire the same cwd after the final observation. This PR rejects more existing owners but does not reserve workspace ownership atomically. I documented the limitation in the PR body and tracked the separate fix in #12341. Changing only CheckpointService's semaphore key is insufficient because thread/workspace admission and provider writes do not participate in that semaphore. Coordinating those paths is required. Leaving this thread open rather than claiming the race is fixed.
There was a problem hiding this comment.
Thanks for confirming. Agreed that this needs coordinated admission/provider locking, not just a CheckpointService key change. Would you like me to prepare the separate fix against main?
7 participants
V2 could allow file rewind when another thread occupied a parent or child directory, or used the project root or a provider cwd before producing a checkpoint scope. Restoring the whole checkout could remove that thread's changes.
Check canonical path containment in both directions at command admission and again before rollback. Include archived threads, historical checkpoint scopes, project roots, and provider sessions that have not stopped. Conversation-only rewind remains available.
Targets the rebased v2 branch from #2829. Main's shared checkpoint fixes are already inherited by that branch.
Verification: six new destructive-restore regressions failed before the fix; all ten safety cases pass afterward. The existing rollback suite passes (22 tests combined), including symlink aliases and conversation-only rewind. All 24 runtime composition tests pass; the production admission regression rejects before creating outbox effects. Server typecheck and focused lint pass. Backend-only change; no visual evidence applies.
Known limit: ownership checks are observations, not an atomic reservation. A different thread can register an overlapping workspace after the final check. This predates this PR and is tracked separately in #12341; this PR does not close that race.
Prepared with Codex.