fix(codex): fail closed on ambiguous Web Rewind boundaries - #1707
fix(codex): fail closed on ambiguous Web Rewind boundaries#1707techotaku39 wants to merge 9 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Fail-closed parsing still drops or accepts incomplete native records —
hasCompleteItemsis set true for any items array, while a native turn without an ID is silently omitted by the surrounding parser. SincenumTurnsis computed from that filtered array, malformedthread/readdata can undercount the raw turns and let rollback mutate native context while HAPI truncates a different transcript suffix. Evidencecli/src/codex/conversationHistory.ts:298; related declared wire shapecli/src/codex/appServerTypes.ts:133andcli/src/codex/appServerTypes.ts:168.
Suggested fix:const record = asRecord(entry) const id = asString(record?.id) if (!record || !id) throw new Error(AMBIGUOUS_REWIND_ERROR) let hasCompleteItems = true for (const item of items) { const itemRecord = asRecord(item) const type = asString(itemRecord?.type) ?? asString(itemRecord?.itemType) if (!itemRecord || !type) { hasCompleteItems = false continue } // existing item classification } // return hasCompleteItems instead of true
Summary
Review mode: initial. One Major correctness issue: incomplete native records can still bypass the ambiguity guard and recreate the rewind divergence this change is intended to prevent.
Testing
- Not run (automation). Added tests cover steering, compaction, a missing items array, and a missing client ID, but not a missing turn ID or malformed item record/type. The current GitHub
integrationjob is failing inbun run test:cli:integration; logs were not yet available while the workflow remained in progress.
HAPI Bot
| clientIds | ||
| clientIds, | ||
| userMessageCount, | ||
| hasCompleteItems: true, |
There was a problem hiding this comment.
[MAJOR] Fail closed on malformed turn/item records
hasCompleteItems becomes true for any array, even if an element is not a record or has no type. More importantly, ThreadReadResponse.turns[].id is optional, and the preceding if (!id) return [] silently removes that raw turn. Because numTurns later uses the filtered array length, such a response can undercount rollback turns and recreate transcript/native divergence.
Suggested fix:
const record = asRecord(entry)
const id = asString(record?.id)
if (!record || !id) throw new Error(AMBIGUOUS_REWIND_ERROR)
let hasCompleteItems = true
for (const item of items) {
const itemRecord = asRecord(item)
const type = asString(itemRecord?.type) ?? asString(itemRecord?.itemType)
if (!itemRecord || !type) {
hasCompleteItems = false
continue
}
// existing item classification
}
// return hasCompleteItems instead of truePlease also add regression cases proving rollbackThread is not called for a missing turn ID and for a malformed item record/type.
There was a problem hiding this comment.
Findings
- [Major] Duplicate native identifiers bypass the ambiguity guard — the new checks validate each turn independently, so two turns can share an ID or client ID. A duplicate turn ID makes the existing
findIndexselect the first occurrence and over-countnumTurns, allowing native rollback to remove turns that the HAPI transcript keeps. Evidencecli/src/codex/conversationHistory.ts:204; related boundary lookupcli/src/codex/conversationHistory.ts:193.
Suggested fix:const seenTurnIds = new Set<string>() const seenClientIds = new Set<string>() const hasAmbiguousBoundary = turns.some((turn) => { const clientId = turn.clientIds[0] if ( !turn.id || !turn.hasCompleteItems || turn.userMessageCount !== 1 || turn.clientIds.length !== 1 || !clientId || turn.hasContextCompaction || seenTurnIds.has(turn.id) || seenClientIds.has(clientId) ) return true seenTurnIds.add(turn.id) seenClientIds.add(clientId) return false })
Summary
- Review mode: follow-up after new commits. The prior missing-turn-ID/malformed-item finding is fixed; one Major boundary-validation gap remains.
Testing
- Not run (automation). Added tests cover the prior finding, but no case verifies rejection of duplicate native turn IDs or duplicate client IDs. The integration check is currently failing; logs were unavailable while the workflow remained in progress.
HAPI Bot
| // turn is known to contain exactly one identified user message. Steering, | ||
| // compaction, and incomplete item data can otherwise leave the model | ||
| // context out of sync with the transcript after a successful rollback. | ||
| const hasAmbiguousBoundary = turns.some( |
There was a problem hiding this comment.
[MAJOR] Reject duplicate native identifiers
This predicate checks each turn in isolation. A thread/read result with two turns sharing the same id still passes; resolving the later HAPI message then reaches findIndex, which selects the first duplicate and makes numTurns too large. The rollback can therefore remove an earlier native turn while the hub keeps its HAPI message. Duplicate clientId values are likewise non-bijective with HAPI messages, whose local IDs are unique per session.
Suggested fix:
const seenTurnIds = new Set<string>()
const seenClientIds = new Set<string>()
const hasAmbiguousBoundary = turns.some((turn) => {
const clientId = turn.clientIds[0]
if (
!turn.id ||
!turn.hasCompleteItems ||
turn.userMessageCount !== 1 ||
turn.clientIds.length !== 1 ||
!clientId ||
turn.hasContextCompaction ||
seenTurnIds.has(turn.id) ||
seenClientIds.has(clientId)
) return true
seenTurnIds.add(turn.id)
seenClientIds.add(clientId)
return false
})Please add regression cases for duplicate turn IDs and duplicate client IDs and assert rollbackThread is not called.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
- Review mode: follow-up after new commits. No issues found in the latest full diff. The prior Major duplicate native identifier finding is addressed. Residual risk: correctness still depends on the Codex app server continuing to expose complete
thread/readitem metadata; the implementation now rejects incomplete or ambiguous histories before rollback.
Testing
- Not run (automation). Added unit coverage exercises ordinary rollback, steering, compaction, incomplete/malformed items, missing IDs, and duplicate turn/client IDs. GitHub
testandintegrationchecks were pending at review time.
HAPI Bot
There was a problem hiding this comment.
Findings
-
[Major] Gate the fork fallback on an exact native fork boundary —
ambiguous_native_boundaryalso covers steering and malformed histories, not only cases where historical fork is safe. For a steered message that is the second user message in one Codex turn, the new fallback atweb/src/components/SessionChat.tsx:600calls historical fork for that local ID. Codex forks before the entire native turn (cli/src/codex/conversationHistory.ts:127), while the hub copies the HAPI transcript up to only the selected message (hub/src/sync/forkTranscript.ts:29). The child can therefore display the first user message from that turn even though its native model context excluded the whole turn, recreating the transcript/context divergence this PR is intended to prevent.Suggested fix:
const selected = turns[index]! const previousId = index > 0 ? turns[index - 1]?.id : null const forkFallbackSafe = selected.clientIds[0] === messageLocalId && turns.flatMap((turn) => turn.clientIds) .filter((clientId) => clientId === messageLocalId).length === 1 && turns.filter((turn) => turn.id === selected.id).length === 1 && (index === 0 || ( previousId !== null && turns.filter((turn) => turn.id === previousId).length === 1 )) // Only this code should open the Web fork dialog. code: forkFallbackSafe ? 'ambiguous_native_boundary_fork_safe' : 'ambiguous_native_boundary'
Questions
- None.
Summary
Review mode: follow-up after new commits. One Major issue found in the newly added Web fallback: it can offer and create a historical fork whose hydrated HAPI transcript does not match the native Codex context.
Testing
- Not run (review automation; PR code was not executed).
- Missing coverage: a turn containing
local-bplus steeredlocal-steer, with fallback requested atlocal-steer; assert that no fork fallback is offered unless the CLI proves an exact fork boundary. - Current CI: typecheck and the targeted Playwright step passed; the CLI integration job failed in four runner spawn/session tests. Those files are outside this diff, so attribution to this PR was not established. The main unit-test step was still running when reviewed.
HAPI Bot
| props.onRefresh() | ||
| } catch (error) { | ||
| if (isAmbiguousRewindError(error)) { | ||
| setRewindForkFallback(messageLocalId) |
There was a problem hiding this comment.
[MAJOR] Gate this fallback on an exact fork boundary
ambiguous_native_boundary also covers a steered message that is the second userMessage in one Codex turn (cli/src/codex/conversationHistory.ts:206). This fallback then calls fork(messageLocalId); Codex forks before the whole native turn (cli/src/codex/conversationHistory.ts:127), but the hub hydrates every invoked HAPI message before only the selected local ID (hub/src/sync/forkTranscript.ts:29). A fallback at local-steer can therefore copy the turn's initial local-b into the child transcript even though the native child excluded that entire turn.
Suggested fix:
const forkFallbackSafe =
selected.clientIds[0] === messageLocalId &&
turns.filter((turn) => turn.id === selected.id).length === 1
code: forkFallbackSafe
? 'ambiguous_native_boundary_fork_safe'
: 'ambiguous_native_boundary'Only open this dialog for the dedicated *_fork_safe code, and add a regression test for selecting the second steered message in a native turn.
There was a problem hiding this comment.
Findings
-
[Major] Reject unresolved native histories before boundary lookup — the new ambiguity guard runs only after the selected turn has been resolved.
ThreadReadResponse.turnsis optional, so an omitted/non-array list or a response missing the selected turn throws before returning the deterministic rejection. The hub then marks the untouched session as diverged and blocks future Fork/Rewind actions even thoughthread/rollbackwas never called. Evidencecli/src/codex/conversationHistory.ts:206; related contextcli/src/codex/appServerTypes.ts:167,hub/src/sync/syncEngine.ts:1624,hub/src/sync/syncEngine.ts:1132.Suggested fix:
const turns = await this.listTurns() const turnId = await this.resolveTurnId(messageLocalId, turns).catch(() => null) const index = turnId ? turns.findIndex((turn) => turn.id === turnId) : -1 if (index < 0) { return { success: false, error: AMBIGUOUS_REWIND_ERROR, code: 'ambiguous_native_boundary', outcome: 'rejected' } }
Questions
- None.
Summary
Review mode: follow-up after new commits. The prior unsafe Fork-fallback finding is addressed. One Major deterministic-rejection gap remains for incomplete native histories that cannot resolve the selected turn.
Testing
- Not run (review automation; PR code was not executed).
- GitHub
testand fixture drift checks passed.integrationfailed in fourcli/src/runner/runner.integration.test.tscases outside this diff; attribution to this PR was not established. - Missing coverage: omitted/non-array
thread.turnsand a missing selected native turn should returnambiguous_native_boundary, avoidthread/rollback, and leaveconversationHistoryDivergedunset.
HAPI Bot
| // context out of sync with the transcript after a successful rollback. | ||
| const seenTurnIds = new Set<string>() | ||
| const seenClientIds = new Set<string>() | ||
| const hasAmbiguousBoundary = turns.some((turn) => { |
There was a problem hiding this comment.
[MAJOR] Reject unresolved native histories before boundary lookup
This guard is reached only after resolveTurnId() and findIndex(). The declared ThreadReadResponse.turns field is optional (cli/src/codex/appServerTypes.ts:167), so an omitted/non-array list—or any incomplete response missing the selected turn—throws first. SyncEngine treats that rejected RPC as an unknown mutation, sets conversationHistoryDiverged, and blocks later Fork/Rewind even though rollback was never called (hub/src/sync/syncEngine.ts:1624, hub/src/sync/syncEngine.ts:1132).
Suggested fix:
const turns = await this.listTurns()
const turnId = await this.resolveTurnId(messageLocalId, turns).catch(() => null)
const index = turnId ? turns.findIndex((turn) => turn.id === turnId) : -1
if (index < 0) {
return {
success: false,
error: AMBIGUOUS_REWIND_ERROR,
code: 'ambiguous_native_boundary',
outcome: 'rejected'
}
}Add a regression with cached local-b -> turn-b plus thread: { id: 'thread-1' }; assert structured rejection and that rollbackThread is not called.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits. The prior deterministic-rejection gap is addressed. No Blocker, Major, Minor, or Nit issue was found in the added or modified lines. Residual risk remains around native Codex thread/read shape changes; the implementation fails closed for the malformed and ambiguous cases covered by the added tests.
Testing
- Not run (automation; PR code was not executed).
- GitHub drift-gate passed; test was pending at review time.
- GitHub integration failed in bun run test:cli:integration; failure logs were unavailable while the workflow remained in progress, so attribution to this PR was not established.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Minor] The safe-Fork fallback is not gated on the independently probed historical-fork capability; see the inline finding.
Questions
- None.
Summary
Review mode: follow-up after new commits. The follow-up commit contains no file changes, so the full latest PR diff was reviewed again. One Minor correctness issue remains: a rollback-only Codex server can receive a safe-Fork response even though the offered action cannot run. No other high-confidence issues were found. Residual risk remains around future native thread/read shape changes.
Testing
- Not run (automation; PR code was not executed).
- GitHub drift-gate and integration checks passed; the main test check was still pending at review time.
- Missing coverage:
thread/rollbacksupported withthread/forkunsupported should returnambiguous_native_boundary, not the Fork fallback code.
HAPI Bot
| return { | ||
| success: false, | ||
| error: AMBIGUOUS_REWIND_ERROR, | ||
| code: this.isForkFallbackSafe(messageLocalId, turns, index) |
There was a problem hiding this comment.
[MINOR] Gate the fallback code on historical-fork capability. probeCapabilities() resolves thread/fork and thread/rollback independently, so a session can advertise rewind while forkAtMessage is unsupported. This branch currently emits the _fork_safe code based only on boundary structure; the web then offers "Fork here", but fork() immediately rejects with "Historical fork is not supported".
Suggested fix:
code: this.states.forkAtMessage === 'supported'
&& this.isForkFallbackSafe(messageLocalId, turns, index)
? 'ambiguous_native_boundary_fork_safe'
: 'ambiguous_native_boundary',Please add a test where supportsMethod returns true only for thread/rollback.
There was a problem hiding this comment.
Findings
- [Major] Safe-Fork can misidentify a later user item as the first item —
clientIdsomits user items without a client ID, soselected.clientIds[0] === messageLocalIddoes not prove the selected message is first in the native turn. If an unidentified user item precedes it, the fallback forks before the whole turn while the copied HAPI transcript can retain that earlier message, recreating the context mismatch this guard is intended to prevent. Evidencecli/src/codex/conversationHistory.ts:289.
Suggested fix:|| selected.userMessageCount !== selected.clientIds.length || selected.clientIds[0] !== messageLocalId
Questions
- None.
Summary
Review mode: follow-up after new commits. The new commit resolves the prior historical-fork capability-gating finding. One Major correctness issue remains in the full latest diff: incomplete client IDs can still produce an unsafe Fork fallback.
Testing
- Not run (automation; PR code was not executed).
- Add a regression with a selected turn containing an unidentified
userMessagebefore the identified selected message; expectambiguous_native_boundary, not_fork_safe. - At review time: integration and fixture drift-gate passed; the main test check remained in progress.
HAPI Bot
| || !selected.id | ||
| || !selected.hasCompleteItems | ||
| || selected.hasContextCompaction | ||
| || selected.clientIds[0] !== messageLocalId |
There was a problem hiding this comment.
[MAJOR] Require complete user-message IDs before offering the fallback. clientIds only records identified user items, so this comparison does not prove that the selected message is the first user item. For example, [userMessage without clientId, userMessage with clientId=messageLocalId] leaves clientIds[0] === messageLocalId; the UI then offers Fork, but native Fork excludes the whole turn while the copied HAPI prefix may retain the earlier message.
Suggested fix:
|| selected.userMessageCount !== selected.clientIds.length
|| selected.clientIds[0] !== messageLocalIdPlease add that ordering case to conversationHistory.test.ts and assert ambiguous_native_boundary.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
- Review mode: follow-up after new commits. No issues found in the full latest diff. The new commit resolves the prior Major finding by rejecting the Fork fallback when any user item in the selected native turn lacks a client ID. Residual risk: committed web coverage verifies structured-error classification, but does not exercise the complete fallback-dialog → Fork → navigation interaction.
Testing
- Not run (automation; PR code was not executed).
- At review time: integration and drift-gate passed; the main test job remained pending.
HAPI Bot
1 participant
Summary
Web Rewind derives thread/rollback.numTurns from raw native turns after the selected HAPI message. Codex histories containing steering, context compaction, or incomplete native metadata can make that boundary ambiguous. A rollback can then appear successful while the model context no longer matches HAPI's truncated transcript.
This change:
Ambiguous histories remain unavailable for destructive Web Rewind until the native boundary is unambiguous. When the exact Fork boundary can be proven and the native operation is available, users can explicitly create a new session from the fallback dialog instead.
Validation
Related Issues
Refs #1703
AI Disclosure
Implemented and tested with OpenAI Codex (GPT-5.6). The follow-up change was driven by review feedback on this PR and validated with focused tests plus an isolated Full environment.