Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d577d2ca43
ℹ️ 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".
| ...(forkBoundary.lastTurnId === undefined | ||
| ? {} | ||
| : { lastTurnId: forkBoundary.lastTurnId }), |
There was a problem hiding this comment.
Update the fork replay fixture for lastTurnId
When the source turn has a native reference, this now adds lastTurnId to every thread/fork request. The replay harness compares normalized outbound frames exactly and does not remove this field, while testkit/fixtures/thread_fork_native_prior_turn/codex_transcript.ndjson still expects a fork containing only threadId followed by thread/rollback; consequently the focused ThreadFork.integration.test.ts scenario fails before validating the new behavior. Update that transcript to expect the native boundary and remove the obsolete rollback exchange.
AGENTS.md reference: AGENTS.md:L109-L109
Useful? React with 👍 / 👎.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a focused Codex server fix that changes existing fork behavior by using native turn boundaries and retaining a compatibility fallback. Unresolved substantive review comments concern replay validation and null-identity fallback behavior, so the final implementation should receive human review. You can add or adjust custom eligibility rules. Learn more. |
ceea97b to
d2f1f51
Compare
|
Note: GPT-5.6 on behalf of shivam (@shivamhwp). Please rebase this onto the latest After rebasing, please update the Codex replay fixtures for the new Once the diff is clean and those tests pass, we can review the actual fix. |
d577d2c to
fb05690
Compare
Dismissing prior approval to re-evaluate 2822d10
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2822d10f05
ℹ️ 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".
| const nativeTurnRef = boundaryTurn?.nativeTurnRef; | ||
| if (nativeTurnRef === null || nativeTurnRef === undefined) { | ||
| return { rollbackTurnCount }; | ||
| } | ||
|
|
||
| return { lastTurnId: nativeTurnRef.nativeId, rollbackTurnCount: 0 }; |
There was a problem hiding this comment.
Treat null native IDs as missing fork boundaries
When a persisted Codex turn has an identity-less provider ref whose nativeId is null (a valid OrchestrationV2ProviderRef), this returns lastTurnId: null and resets rollbackTurnCount to zero. forkThread then sends a null boundary, which is equivalent to omitting the boundary, while also skipping the compatibility rollback; forking from an earlier run therefore retains later turns. Check nativeTurnRef.nativeId for null before disabling rollback.
Useful? React with 👍 / 👎.
9eeed8c to
8af5734
Compare
2822d10 to
8d034c8
Compare
Dismissing prior approval to re-evaluate 8d034c8
877b9de to
b82facd
Compare
…4457) Update every deterministic Codex turn/start expectation with the explicit user approvals reviewer emitted by the adapter, restoring exact replay coverage and unblocking the merge lane.
…ojects (pingdotgg#3640) Restore versioned shell-cache hydration, reconcile repository enrichment across multiple environments, and decode non-null settled shell timestamps correctly.
…dotgg#3754) Expose MCP tools for worktree handoff and status inspection, with focused server and contract coverage. Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: shivam <91240327+shivamhwp@users.noreply.github.com>
…dotgg#9930) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: shivam <91240327+shivamhwp@users.noreply.github.com>
…tgg#9903) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
8d034c8 to
e3fc9dd
Compare
ed5dc41 to
2b1139a
Compare
e03e376 to
e39429e
Compare
Retry/edit of an earlier turn in a paginated native Codex thread forked at head and then issued thread/rollback, which paginated threads reject. Resolve the selected provider turn nativeTurnRef and pass it as lastTurnId to thread/fork so later turns are excluded atomically and no rollback is needed. Fall back to fork-then-rollback only when no native reference is available, and fail honestly when that fallback hits a paginated fork. rollbackThread now probes historyMode and reports an explicit unsupported error for paginated threads instead of sending a request Codex will reject. Adapts the approach from pingdotgg#9169 by Yusuf007R. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
600a8a5 to
d8c75ec
Compare
Retry/edit of an earlier turn in a paginated native Codex thread forked at head and then issued thread/rollback, which paginated threads reject. Resolve the selected provider turn nativeTurnRef and pass it as lastTurnId to thread/fork so later turns are excluded atomically and no rollback is needed. Fall back to fork-then-rollback only when no native reference is available, and fail honestly when that fallback hits a paginated fork. rollbackThread now probes historyMode and reports an explicit unsupported error for paginated threads instead of sending a request Codex will reject. Adapts the approach from pingdotgg#9169 by Yusuf007R. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
463d0c6 to
e10a1e2
Compare
Retry/edit of an earlier turn in a paginated native Codex thread forked at head and then issued thread/rollback, which paginated threads reject. Resolve the selected provider turn nativeTurnRef and pass it as lastTurnId to thread/fork so later turns are excluded atomically and no rollback is needed. Fall back to fork-then-rollback only when no native reference is available, and fail honestly when that fallback hits a paginated fork. rollbackThread now probes historyMode and reports an explicit unsupported error for paginated threads instead of sending a request Codex will reject. Adapts the approach from pingdotgg#9169 by Yusuf007R. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Stale duplicate of #11490 — same native |
Codex 0.151 makes durable threads paginated by default, but T3's retry/edit fork path still truncates a completed native fork with the deprecated
thread/rollbackendpoint. Codex rejects that endpoint for paginated threads, leaving the provider-turn start failed and the T3 run stuck in its starting state.What Changed
lastTurnIdin the originalthread/forkrequest so Codex omits later turns atomically.Why
The existing flow first creates a complete native fork. If the user selected an earlier turn, T3 counts the terminal turns after that boundary and calls
thread/rollbackon the newly created thread. Forking at the current head works because the count is zero; retrying or editing an earlier turn reaches the rollback call.Paginated Codex threads reject that second request with
paginated threads do not support thread/rollback. Although Codex has already created the fork, T3 receives an error and fails the durableprovider-turn.starteffect, so the run appears unresponsive.Codex already supports the intended operation directly:
thread/fork.lastTurnIdcreates a fork through the specified turn, inclusive. Supplying the known native turn ID removes the incompatible second request without adding recovery, persistence, or provider-version detection machinery.UI Changes
None. This changes only the Codex provider adapter's native fork request.
Validation
vp test run apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.tsvp run --filter t3 typecheckvp fmt apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts --checkChecklist
Implemented with GPT-5.6 in the Codex desktop harness.
Note
Medium Risk
Changes native fork semantics for retry/edit-at-earlier-turn flows; incorrect boundary mapping could fork with wrong history, though fallback rollback preserves legacy behavior when native ids are missing.
Overview
Codex same-provider forks now cut history at the selected turn in one
thread/forkcall instead of forking at the head and callingthread/rollback, which paginated Codex threads reject.The adapter adds
resolveCodexForkBoundary, which maps the orchestrator’s source provider turn to its native Codex turn id whennativeTurnRefis present.forkThreadincludes that id aslastTurnIdonthread/forkand sets rollback to zero so the post-forkthread/rollbackpath is skipped. When there is no native turn reference, behavior stays the same: fork latest state and roll back by the count of later terminal provider turns.Unit tests cover native-boundary vs identityless fallback. Replay fixtures and orchestration docs were updated to expect
lastTurnIdand no rollback in the prior-turn fork scenario.Reviewed by Cursor Bugbot for commit 8d034c8. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fork Codex threads at native turn boundary instead of rollback
resolveCodexForkBoundarytoCodexAdapterV2, which returns the source provider turn's native turn id as the fork boundary when available and resets rollback count to zeroforkThreadhandler to include the resolved native turn id in the native fork request and skip fork-local rollback in that case; turns without native identity still use the existing latest-state fork plus rollback fallbackthread_fork_native,thread_fork_native_prior_turn,thread_merge_back_continue, andthread_merge_back_siblingsto expect native boundary fork requests and removes explicit rollback exchanges where applicableMacroscope summarized 8d034c8.