fix(codex): fail closed on ambiguous Web Rewind boundaries - #1707
fix(codex): fail closed on ambiguous Web Rewind boundaries#1707techotaku39 wants to merge 6 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
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, 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.