Repository navigation
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e4dca8d01
ℹ️ 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".
| if err != nil { | ||
| if s.steerAgainstRecoveringTurn(turnID) { | ||
| return agent.ErrSteeringNotReady |
There was a problem hiding this comment.
When recovery starts after turn/steer has been written but before its response arrives, cancelling the RPC reaches this branch and returns ErrSteeringNotReady. The router defines that result as “nothing was sent,” so Core retries the pending input, even though native may already have accepted it and the rebuilt history may already contain it; this can duplicate a user message or its side effects. Return the existing outcome-unknown result once the write callback has fired, and reserve not-ready for requests proven not to have been sent.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6eb4a67: once the steer frame has been written, a cancelled/failed RPC now returns the outcome-unknown error instead of ErrSteeringNotReady, so Core will not resend. The takeover test now asserts this.
| // The codex adapter can rebuild a thread after a recoverable upstream 400; | ||
| // other adapters ignore the controls a selection makes possible. | ||
| headers.Set(proto.UpstreamRecoveryHeader, proto.UpstreamRecoveryV1) |
There was a problem hiding this comment.
This connection advertises recovery unconditionally even though the comment acknowledges that only the Codex adapter implements it. A daemon connection can serve Codex, mcode, and Claude harnesses, and Core gates controls only on this connection-wide bit, so enabled recovery controls are also sent to unsupported harnesses and silently ignored rather than validated as an unsupported combination. Recovery support needs to be declared and checked for the selected agent kind, not inferred from a daemon-wide header.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this in the PR, for three reasons:
- The master switch (core.upstream_recovery.enabled / OAC_UPSTREAM_RECOVERY_ENABLED) defaults to off. While it is off, Core does not send the recovery control block at all (withUpstreamRecovery returns early when the policy is disabled).
- Non-Codex adapters (mcode, claudesdk) do not implement recovery. I checked that neither reads ExecutionControls.UpstreamRecovery (they only read OutputFormat, WebSearch and TextVerbosity), so a block sent to them is ignored and behaves as if recovery were off. No side effects.
- Declaring and validating recovery per harness would change the protocol and Core's validation, which is a larger scope than this PR. We will follow up as a separate requirement.
| if p.path != "" { | ||
| line += " (path: " + p.path + ")" |
There was a problem hiding this comment.
For a historical localImage whose filesystem path contains a newline followed by </recovered_history> or a transcript label—valid characters in a POSIX filename—the path is inserted without escapeTranscript, allowing it to close or forge the supposedly bounded history section. The imageView/imageGeneration path rendering has the same problem. Escape these paths like other untrusted transcript fields, or omit them from placeholders.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6eb4a67: localImage, imageView and imageGeneration paths now go through escapeTranscript; added a forged-path regression test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eb4a67fd1
ℹ️ 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".
| for _, raw := range turn.Items { | ||
| history = append(history, decodeItem(raw, false, stats)...) | ||
| } | ||
| } | ||
| return history, tail |
There was a problem hiding this comment.
When an enabled recovery receives a nonempty but stale/partial history that does not contain failedTurnID, this function silently treats every returned turn as completed history and lets planRecovery proceed based on older reasoning or images. The current request is then reissued without any commands or tool effects from the omitted failed turn in the transcript, so the model can repeat already-completed side effects. Track whether the exact failed turn was found and abort with history_unreadable when it was not.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cdc164f: planRecovery now aborts with history_unreadable when the failed turn is not found in the returned history (new test added).
| func splitRecoveredInput(parts []userPart) (carried string, current []userPart, ok bool) { | ||
| if len(parts) == 0 || !strings.HasPrefix(parts[0].text, recoveryPreambleMarker) { | ||
| return "", nil, false | ||
| } | ||
| head := parts[0].text | ||
| open := strings.Index(head, recoveryHistoryOpen+"\n") | ||
| closing := strings.LastIndex(head, "\n"+recoveryHistoryClose) |
There was a problem hiding this comment.
When a historical user message starts with [oac-recovery-preamble] and contains the expected tags, it is accepted as a previously generated recovery input based only on this prefix. Its extracted carried text is later rendered verbatim as entryCarried, so an embedded earlier </recovered_history> can close the bounded section and make following historical text look like the live request. Escape the extracted content again or use provenance that ordinary user input cannot forge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cdc164f: extracted carried text now has the recovered_history open/close tags neutralized, so a forged earlier message cannot close the bounded section (new test added).
| // ASSUMPTION (design V2, to be calibrated against a live codex 0.153.4): | ||
| // thread/read with includeTurns returns each Turn's items for a root thread, and | ||
| // a thread created with historyMode "paginated" instead lists them through | ||
| // thread/turns/list. The first form is tried first; the second is the form this | ||
| // adapter already relies on for subagent history. When neither yields Turns the |
There was a problem hiding this comment.
This recovery path explicitly depends on an uncalibrated assumption about Codex's history response, and the new HTTP 400 matching also documents unverified native wording, but the change adds neither assumption to contracts/agents-api/index.md or its Chinese mirror. That leaves the canonical coverage ledger claiming no such qualification for behavior that controls automatic retries and possible side effects; add both unverified behaviors to the ledger before enabling recovery.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cdc164f: added the unverified thread/read history shape and the provider 400 wording to the coverage ledger (en and zh).
…alize forged markers; record assumptions in ledger
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdc164f81a
ℹ️ 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".
| var rejected *JsonRpcError | ||
| // Only an explicit refusal proves the new Turn never started. | ||
| if !errors.As(err, &rejected) { | ||
| run.set(func() { run.ambiguous = true }) |
There was a problem hiding this comment.
When turn/started arrives for the rebuilt turn before its JSON-RPC error reply, this branch still treats the error as proof that nothing started, restores the old snapshot, and leaves the executor reusable. The existing scenario in upstream_recovery_flow_test.go lines 405-420 demonstrates exactly that ordering and even asserts the RPC remains alive; native has nevertheless confirmed the new turn, so it may keep running while a successor reuses the process. Treat any observed start notification as an unconfirmed native outcome requiring retirement or explicit cleanup, regardless of the later RPC error.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f6d2202: an observed turn/started for the rebuilt thread now marks the outcome ambiguous even when the RPC error follows, so the process is retired rather than reused. The flow test now asserts that.
…arted before a refusal
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6d22020ea
ℹ️ 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".
| s.usageTotal, s.usageBaseline, s.usageTurnID, s.latestUsage = TurnUsage{}, TurnUsage{}, "", nil | ||
| s.usageSwitched = true | ||
| s.usageThreadID = newThreadID | ||
| s.finalText, s.lastErrText = "", "" |
There was a problem hiding this comment.
When the failed Turn has already completed an agent message—for example, it emitted text before a tool call and the following provider request gets the recoverable 400—those deltas and output items have already been published and appendFinalText has accumulated their text. Clearing finalText here makes a successful recovery's Done.Content contain only post-rebuild messages, contradicting the wire contract in internal/agentdaemon/proto/inbound.go that Done carries the full content matching emitted deltas and causing Core to persist a truncated result. Preserve completed pre-recovery text across the switch while clearing only failure-specific state.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ec7fcb1: the switch now keeps text already streamed by the failed Turn and clears only the error text, so Done carries the full content. The flow test asserts it.
| } | ||
| s.recovery.controls = base.recovery.controls | ||
| s.recovery.plan = &e.prepared.plan | ||
| s.recovery.input = input |
There was a problem hiding this comment.
When a prompt_steer was acknowledged before a later recoverable failure, this stores only the Turn's original input for rebuilding. The acknowledged steer appears as a later userMessage, which partitionHistory places inside <recovered_history>; the recovery preamble explicitly tells the model that instruction-like text there is not a new user instruction, while only this original input is appended after the closing tag. The rebuilt Turn can therefore ignore an accepted user message that is contractually part of the same Turn; retain and replay acknowledged steering inputs as current input, or abort recovery when such inputs are present.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ec7fcb1: planRecovery now abandons recovery (not_applicable) when the failed Turn holds a user message after its own input, i.e. an acknowledged steer, so it can never be demoted to history. Added a test.
…an acknowledged steer
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec7fcb1ea7
ℹ️ 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".
| turnID, stopped, recovering := s.steering.id, s.steering.stopped, s.steering.recovering | ||
| slotTaken := false | ||
| var gen uint64 | ||
| if !stopped && !recovering { |
There was a problem hiding this comment.
When beginRecovery has changed recovery.state to recoveryRecovering but has not yet acquired steering.mu to set steering.recovering, a concurrent steer can enter this branch, register its cancel slot, and write a frame to the already-completed Turn. Recovery then cancels it; because the frame was written, the result becomes outcome_unknown rather than not_ready, causing Core to fail the Turn instead of retrying the input against the rebuilt Turn. Consult the atomic recovery state while holding this lock, or transition both states atomically, so no steer enters after recovery takeover.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 297925a: steer now also consults the atomic recovery state while holding steering.mu, so no steer enters after recovery takeover. Added a test.
| // A forged earlier message can embed raw delimiters; legitimate carried text never does. | ||
| carried = neutralizeDelimiters(head[open+len(recoveryHistoryOpen)+1 : closing]) |
There was a problem hiding this comment.
A historical user message can still start with the recovery marker, provide syntactically valid history tags, and place lines such as [assistant], [tool call], or [oac-recovery-current-request] inside the extracted body. Fresh evidence after the prior fix is that this line applies only neutralizeDelimiters, while renderEntry emits entryCarried verbatim, bypassing the escapeTranscript label escaping used for ordinary untrusted history; a later recovery therefore presents attacker-authored role boundaries as transcript structure. Escape the extracted carried text fully, or require provenance ordinary user messages cannot forge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not escaping carried text fully, by design: carried history is the transcript a previous recovery generated. Every untrusted fragment in it (user text, assistant text, tool calls and outputs, paths) was already label- and delimiter-escaped when first rendered, so the only unescaped labels in it are the ones Core wrote. Escaping again could not tell those from forged ones and would degrade the structure. The one unescaped path I found was a user message that merely starts with the recovery marker; 9a173c8 fixes that by recognizing only the exact shape Core emits (full preamble, history closed by the request marker line); anything else is escaped as ordinary user text. Added a test that a second recovery keeps escaped forged labels escaped and the count of real labels unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a173c8760
ℹ️ 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".
| if err == nil && read.Thread.ID == threadID && len(read.Thread.Turns) > 0 { | ||
| return historyTurns(read.Thread.Turns), nil |
There was a problem hiding this comment.
When thread/read returns nonempty Turn shells or partial items—the exact uncalibrated response shape for which the pagination fallback exists—this accepts them without verifying itemsView or item completeness. Fresh evidence beyond the prior missing-turn fix is that a present failed-Turn shell passes this check: partitionHistory marks it found, older reasoning or images can satisfy the recovery precondition, and the rebuilt request then omits tool effects from the failed Turn and may repeat their side effects. Validate that the inline Turns contain full items or fall back to thread/turns/list before recovering.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5bad289: inline thread/read Turns are now accepted only when every Turn has an id and itemsView == "full"; shells or partial Turns fall back to thread/turns/list. Added a test.
| params = { fingerprint: p.fingerprint as UpstreamFingerprint, summary: p.summary, ...(withId ? { upstream_request_id: p.upstream_request_id as string } : {}) }; | ||
| if (withRecovery) { | ||
| const fingerprints = p.recovery_fingerprints, outcomes = p.recovery_outcomes; | ||
| if (!safeCount(p.recovery_count, 2) || !safeCount(p.recovery_dropped_reasoning_items, 1_000_000) || !safeCount(p.recovery_dropped_images, 1_000_000) || |
There was a problem hiding this comment.
Core accepts up to two recovery records with as many as 1,000,000 dropped reasoning items or images per record, and DiagnosticRecoveries sums those values, so a valid response can contain an aggregate of 2,000,000. This client validator caps the aggregate at 1,000,000 and consequently throws invalidAdminResponse for a response that the shared Go protocol and Core API accept; use the two-record aggregate bound or constrain the owning protocol consistently.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5bad289: the client now allows the two-record aggregate (2,000,000) for dropped reasoning items and images, matching Core. Added a test.
…record drop aggregates
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bad289314
ℹ️ 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".
| if req.ExecutionControls != nil && req.ExecutionControls.UpstreamRecovery != nil { | ||
| s.recovery.controls = req.ExecutionControls.UpstreamRecovery.Normalized() | ||
| } |
There was a problem hiding this comment.
When recovery is enabled and an execution_prepare carries this block on a WebSocket whose 101 response omitted X-Oac-Upstream-Recovery, this code activates recovery solely from the request payload. Although transport.Conn records the immutable negotiated selection, pumpConn never passes it to the router and Conn.UpstreamRecovery() has no caller, so a stripped header or mismatched Core can silently bypass the documented base-protocol downgrade and trigger thread rebuilding. Propagate the connection selection into preparation and reject the unsupported combination rather than trusting the block alone.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 83b183a: the connection's negotiated selection is now passed into the router (dispatch.Config.UpstreamRecovery), and execution_prepare carrying recovery controls on a connection that did not select the extension is rejected with unsupported_configuration. Test added.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83b183a8d2
ℹ️ 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".
| Turn Turn `json:"turn"` | ||
| } | ||
| _ = json.Unmarshal(raw, &res) | ||
| run.set(func() { run.newTurnID, run.acked = res.Turn.ID, true }) |
There was a problem hiding this comment.
When cancellation sets steering.stopped after turn/start was accepted but before this result hook runs, bindTurnResult silently fails to bind the new Turn because startSteering returns false, yet this line still marks the recovery acknowledged. runRecovery then interrupts the new Turn, but its completion is rejected by isRootTurn because steering.id was never updated, so no terminal frame is emitted and cancellation waits until Core reports cancel_unconfirmed. Treat this ordering as ambiguous and retire the process, or bind the returned Turn identity before acknowledging it.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3253282: acknowledgeRecovery now checks that the rebuilt Turn was actually bound; if cancellation won the race the recovery fails as ambiguous (process retired) instead of acknowledged.
| } | ||
| return nil |
There was a problem hiding this comment.
With recovery enabled, an unknown or shape-incompatible history item is silently omitted here. If the failed Turn also satisfies the selected recovery precondition, a completed side-effecting tool item introduced by a newer Codex version can therefore disappear from the rebuilt transcript, allowing the model to repeat an external action it cannot see. Propagate an unsupported-history result and abort with history_unreadable instead of recovering from a transcript known to be incomplete.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3253282: unknown or shape-incompatible history items are counted and planRecovery aborts with history_unreadable. Test added.
| case proto.TypeDone: | ||
| observeUpstreamRecoveries(ctx, sessionID, runID, result) |
There was a problem hiding this comment.
When delivery exits through cancellation or another early terminal path while the Runtime's done frame is already queued, the deferred journal.drain merges and persists its recovery records, but only this direct TypeDone branch calls observeUpstreamRecoveries. Those recoveries consequently lack the documented per-record executor upstream recovery log even though they appear in the stored outcome; invoke the observer after the final drain or centralize it with Done merging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3253282: the final drain now calls observeUpstreamRecoveries when it merged new recovery records.
…g final-drain recoveries
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3253282429
ℹ️ 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".
| s.threadIDMu.Lock() | ||
| s.reportedID = newThreadID | ||
| s.recovery.gateThread = "" |
There was a problem hiding this comment.
When the rebuilt native Turn emits turn/started and then quickly emits turn/completed before the turn/start RPC reply, the notification handler accepts and settles it while reportedID still names the old thread and before the successful recovery record is appended. This ordering is explicitly supported for pre-reply notifications elsewhere in this change, so the emitted Done can persist the failed thread as the native Session ID and omit the recovery record, breaking later cold continuation. Gate the rebuilt terminal notification until acknowledgement has committed the new thread and recovery metadata.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in baf383f: a rebuilt Turn's turn/completed that arrives before the turn/start reply is held and replayed only after acknowledgement commits the new thread id and recovery record (dropped if the recovery is abandoned). Test added.
| if json.Unmarshal(raw, &message) != nil { | ||
| return nil |
There was a problem hiding this comment.
With recovery enabled, a historical userMessage whose content has a shape this build cannot decode—for example, a native upgrade changes it from an array—makes decodeUserParts return nil without incrementing stats.unsupported; recovery then proceeds with that user message rendered as empty. Fresh evidence beyond the prior top-level-item fix is that this nested decode failure still bypasses the history_unreadable guard, so instructions can disappear from the rebuilt transcript and completed work may be repeated. Return decode validity and abort recovery for malformed or unrecognized content blocks.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in baf383f: decodeUserParts now reports validity; undecodable content or unrecognized content block types count as unsupported, so planRecovery aborts with history_unreadable. Test added.
…med user history content
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baf383fd94
ℹ️ 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".
| Result *struct { | ||
| Content []rawContent `json:"content"` | ||
| } `json:"result"` |
There was a problem hiding this comment.
When recovery is enabled and an MCP call returns material data in result.structuredContent, or a failed call reports its outcome only through error, this struct decodes only result.content and renders the call as having an empty or partial result. Both shapes are already accepted native fixtures in apps/daemon/internal/agent/codex/session_tools_test.go lines 16-17, so planRecovery can treat this incomplete history as supported and rebuild a transcript that may cause the model to repeat a side-effecting call. Preserve these outcome fields or abort with history_unreadable when they cannot be rendered.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 565e68e: mcpToolCall recovery rendering now includes result.structuredContent and error alongside content. Test added.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 565e68e2db
ℹ️ 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".
| s.steering.mu.Lock() | ||
| if s.steering.recovering && p.Turn.ID != s.steering.recoveringTurnID { | ||
| s.steering.deferredCompleted = append(json.RawMessage(nil), raw...) | ||
| s.steering.mu.Unlock() | ||
| return |
There was a problem hiding this comment.
When a rebuilt Turn emits turn/started and then turn/failed before the turn/start reply, this guard does not apply because onTurnFailed still emits its terminal immediately. The result therefore uses the old reportedID and omits the recovery record before acknowledgeRecovery can commit either, so later continuation can resume the discarded thread. Defer both rebuilt terminal notification types until acknowledgement.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5eb4f3: a rebuilt Turn's turn/failed is now deferred until the recovery is acknowledged and replayed afterwards (test added).
| for i := len(blocks) - 1; i > first; i-- { | ||
| cost := runeLen(blocks[i]) + 2 | ||
| if cost > remaining { | ||
| break | ||
| } |
There was a problem hiding this comment.
When the transcript exceeds transcript_max_chars, this suffix-selection loop can omit completed command, MCP, or dynamic-tool entries from the middle while recovery still proceeds. For an externally side-effecting call, the rebuilt model then has no evidence that the action already occurred and can repeat it; the omission is not represented in RecoveryDropped either. Abort recovery when truncation would remove a tool effect, or retain a bounded summary of every such call.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5eb4f3: when truncation would omit a tool call or carried recovery record, recovery is abandoned as history_unreadable instead of proceeding (test added).
| var lines []string | ||
| for _, b := range blocks { | ||
| switch b.Type { | ||
| case "text", "inputText", "input_text": | ||
| lines = append(lines, escapeTranscript(b.Text)) |
There was a problem hiding this comment.
When an MCP or dynamic-tool result contains an unrecognized content type, this switch silently ignores the block without incrementing stats.unsupported. For example, inputAudio is explicitly treated as unrepresentable by tool_observations_test.go, but here it produces an incomplete transcript that passes the history_unreadable guard; a rebuilt model can consequently repeat a side-effecting tool call whose outcome disappeared. Mark unknown blocks unsupported and abandon recovery.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d5eb4f3: unrecognized tool-result content blocks now count as unsupported and abandon recovery (test added).
…very on dropped tool calls or unknown tool-result blocks
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5eb4f3ebc
ℹ️ 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".
| // Already rendered, escaped and bounded by the recovery that produced it. | ||
| block = e.text | ||
| } | ||
| return truncateRunes(block, limit) |
There was a problem hiding this comment.
On a second recovery, the previous transcript is represented as one entryCarried, but this common return truncates every entry to half of the configured budget. A first recovery can legitimately carry nearly the full budget, so the next recovery can silently discard its latter half—including completed tool calls—before renderTranscript checks whether any tool or carried entry was omitted; the shortened block can then pass as complete and let the model repeat side effects. Preserve carried entries intact when they fit the overall budget, or abort recovery if any part of one would be truncated.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d6c55a1: carried recovery entries are no longer truncated by the per-entry limit; if one cannot fit the budget, recovery is abandoned instead (test added).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7df7a904b
ℹ️ 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".
| return false | ||
| } | ||
| func toolEntry(call, output string) transcriptEntry { | ||
| return transcriptEntry{kind: entryTool, call: escapeTranscript(truncateRunes(strings.ReplaceAll(call, "\n", " "), maxToolCallChars)), output: output} |
There was a problem hiding this comment.
When a completed command contains shell-significant newlines, this replacement irreversibly changes its meaning in the recovery transcript—for example, echo safe\nrm target becomes echo safe rm target, so the rebuilt model cannot tell that rm target was a separate completed operation and may repeat the side effect. Preserve line boundaries with an unambiguous escape or reject calls whose identity cannot be rendered faithfully.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 82fed44: line breaks in tool calls now render as visible \n/\r escapes instead of spaces, so separate commands stay distinct. Added a test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82fed44fc8
ℹ️ 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".
| s.threadIDMu.Lock() | ||
| s.recovery.gateThread = "" | ||
| s.threadIDMu.Unlock() | ||
| s.recovery.snapshot.Store(nil) |
There was a problem hiding this comment.
When a process or stream terminal races a blocked recovery RPC, the terminal path sets terminal and cancels the recovery before calling recordInflightRecoveryAbort; the awakened recovery goroutine can therefore observe the terminal, call endRecovery, and clear the only snapshot that the recorder uses here. The resulting Done omits the admitted aborted attempt. Fresh evidence beyond the prior terminal-recording fix is that snapshot clearing remains unsynchronized with the new recorder; serialize both operations through recovery.mu or record the abort immediately after winning terminal settlement.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 90aa010: endRecovery now records the aborted attempt under recovery.mu before clearing the snapshot when a terminal already won, so Done keeps the admitted attempt. Added a test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90aa010ed7
ℹ️ 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".
| } | ||
| var paths []string | ||
| for _, c := range item.Changes { | ||
| if c.Path == "" { |
There was a problem hiding this comment.
When recovered history contains a completed fileChange whose path is only spaces or tabs, this equality check accepts it and renders a call with no usable file identity. The rebuilt model cannot determine which file was already modified and may repeat the edit; validate paths with strings.TrimSpace, as the command and tool identifier checks do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fedf2d: file-change paths are now checked with strings.TrimSpace; added a whitespace-path test.
| if ok && r.ErrorCode == "engine_failed" && recoveries[len(recoveries)-1].Outcome == proto.RecoveryOutcomeRecovered { | ||
| ok = false |
There was a problem hiding this comment.
When a Runtime reports an aborted recovery for fingerprint A but the terminal engine error carries fingerprint B, this validation accepts and persists both even though an abort occurs before any thread switch and the adapter subsequently settles the original failure that triggered that same recovery. Compare the final aborted record with the accepted terminal fingerprint when present, otherwise diagnostics can expose an impossible recovery audit from a buggy Runtime.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fedf2d: a final aborted record must match the terminal engine fingerprint when one is present, otherwise the recovery audit is dropped. Added a test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fedf2d0a2
ℹ️ 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".
| s.finalTextMu.Unlock() | ||
| s.usageMu.Unlock() | ||
| s.threadIDMu.Unlock() | ||
| s.recovery.snapshot.Store(snap) |
There was a problem hiding this comment.
When execution-owner cancellation wins after recoveryRecovering is stored but before this snapshot is published, emitTerminalFailure calls recordInflightRecoveryAbort, observes a nil snapshot, and emits Done without the admitted recovery record. The recovery goroutine can add the abort later, but the terminal payload is already gone. Fresh evidence beyond the prior terminal-race fixes is this remaining initialization window; install the snapshot atomically with the recovering transition or otherwise serialize it with terminal recording.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 508aabe: beginRecovery holds an init lock from before the recovering transition until the snapshot is published, and the terminal abort recorder waits on it, so Done never misses the admitted recovery record.
| case "items": | ||
| err = decodeBoundedItems(dec, &out.Items) |
There was a problem hiding this comment.
When a malformed or partial history response marks an earlier Turn with itemsView: "full" but omits items or sets it to null, this decoder leaves Items nil and the later completeness checks still accept the Turn. Its completed tool calls then disappear from the rebuilt transcript, so recovery may repeat their side effects if another Turn supplies the triggering reasoning or image history. Track field presence and reject missing or null items rather than treating them as an empty full view.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 508aabe: the decoder tracks non-null items presence; full-view turns without items are rejected as incomplete history. Added a decode test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 508aabeb1e
ℹ️ 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".
| normalized := policy.Normalized() | ||
| req.ExecutionControls.UpstreamRecovery = &normalized |
There was a problem hiding this comment.
When recovery is enabled, disabled, or reconfigured while a hosted Environment is suspended, Quiesce retains its idle executors, but this assignment changes the request hashed by executorFingerprint; after resume, handleExecutorPrepare therefore returns configuration_conflict for the next Turn because the retained owner's fingerprint contains the old policy. The owner is not retired by that rejection, so existing sessions can remain unable to run until its idle timeout closes it, contradicting the documented switch-only rollout and rollback path; refresh the per-executor policy or retire/recreate the retained owner when this process-level policy changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ffc601a (option A with per-prepare application). The executor fingerprint now excludes the recovery policy, so toggling or reconfiguring it no longer yields configuration_conflict on a retained owner. To keep rollback effective, Core sends the current policy with every prepare (omitted when recovery is off) and the daemon applies it to the retained codex executor (nil = disabled); each Turn snapshots that policy at StartTurn, so shouldRecover follows the latest Core setting rather than the owner-creation snapshot. A Turn already running keeps the policy it started with. Tests: on->off->on->changed (attempts/classes) reuses the owner without conflict or retirement, and the codex executor test checks the per-Turn controls. No daemon lock or native-session continuation logic changed.
| } | ||
| call = "web search: open page " + escapeTranscript(*a.URL) | ||
| case "findInPage", "find_in_page": | ||
| if (a.URL == nil || strings.TrimSpace(*a.URL) == "") && (a.Pattern == nil || strings.TrimSpace(*a.Pattern) == "") { |
There was a problem hiding this comment.
Fresh evidence beyond the earlier blank-target fix is that the pinned WebSearchActionFind schema requires both url and pattern, while this && rejects the action only when both are blank. A completed action with a URL but no pattern, or a pattern but no URL, is therefore accepted into the rebuilt transcript even though it does not identify the operation that ran, allowing recovery to repeat the page search; reject when either required value is absent or blank.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ab81dc6: find-in-page history now requires both url and pattern; added tests for each missing case.
| if (!safeCount(p.recovery_count, 2) || !safeCount(p.recovery_dropped_reasoning_items, 2_000_000) || !safeCount(p.recovery_dropped_images, 2_000_000) || | ||
| !Array.isArray(fingerprints) || !Array.isArray(outcomes) || fingerprints.length < 1 || fingerprints.length > 2 || outcomes.length !== fingerprints.length || | ||
| !fingerprints.every((item) => isOneOf(recoverableFingerprints, item)) || !outcomes.every((item) => item === "exhausted" || item === "aborted")) return invalidAdminResponse(); |
There was a problem hiding this comment.
A response with recovery_count: 2, two exhausted outcomes, and the same recoverable fingerprint twice passes these checks, even though both the shared parser and adapter permit each fingerprint only once per Turn. The client consequently accepts an impossible recovery audit instead of raising invalidAdminResponse; require the fingerprint array to contain unique values.
AGENTS.md reference: AGENTS.md:L22-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ab81dc6: client rejects duplicate recovery fingerprints; added a test and corrected the aggregate fixture to use distinct fingerprints.
…licate client fingerprints
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab81dc63f7
ℹ️ 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".
| s.recovery.mu.Lock() | ||
| snap.recorded = true | ||
| s.appendRecoveryLocked(proto.UpstreamRecovery{ | ||
| Fingerprint: run.fingerprint, Action: proto.RecoveryActionRebuildThread, Attempt: 0, | ||
| Outcome: proto.RecoveryOutcomeAborted, Reason: reason, OldNativeSessionID: snap.oldThreadID, | ||
| }) |
There was a problem hiding this comment.
Fresh evidence beyond the prior terminal-race fixes is that abandonRecovery checks terminal before restoring the snapshot but then appends unconditionally. If the executor-close path wins during that restore, recordInflightRecoveryAbort can mark and append the cancellation first; this block then appends the same attempt again, and Core rejects the duplicate-fingerprint sequence and drops the entire recovery audit. Check snap.recorded while holding recovery.mu before appending, as the terminal recorder does.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4dc179c: abandonRecovery now checks snap.recorded under recovery.mu before appending, so an executor-close record is not duplicated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc179ca63
ℹ️ 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".
| s.steering.mu.Unlock() | ||
|
|
||
| s.threadIDMu.Lock() | ||
| s.reportedID = newThreadID |
There was a problem hiding this comment.
Fresh evidence after the earlier terminal-race fixes is that this acknowledgement path publishes reportedID before taking recovery.mu and marking the snapshot recovered. If execution-owner shutdown or another terminal path wins in that window, terminal emission reads the new ID while recordInflightRecoveryAbort marks the same snapshot aborted; this function can then unconditionally append a recovered record, producing either an old-ID abort paired with the new reported ID or contradictory duplicate records, both of which Core rejects and drops. Commit the identity and recovery record atomically with respect to terminal recording.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 440073a: acknowledgeRecovery now takes recovery.mu before publishing reportedID and only commits the new ID and the recovered record if no abort was already recorded for the snapshot.
| } | ||
| owner.idleLease++ | ||
| // Core sends the current policy with every prepare and omits it when recovery is off. | ||
| if applier, ok := owner.native.(recoveryPolicyApplier); ok { |
There was a problem hiding this comment.
When recovery controls change for a retained executor, this private type assertion silently skips the update unless the concrete executor happens to implement recoveryPolicyApplier, while executorFingerprint deliberately excludes those controls. A capability-valid replacement or wrapper can therefore be reused with stale enabled/classes/attempt settings—even after an operator disables recovery—instead of being updated or rejected. Put this operation on the declared Executor/Harness protocol or retire and recreate the owner; the repository explicitly forbids optional side interfaces for one implementation.
AGENTS.md reference: AGENTS.md:L42-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f3b7cfc without widening agent.Executor (about 12 implementers for little gain in this PR). The optional recoveryPolicyApplier stays but now fails safe: a compile-time assertion pins the codex Executor to it, and when a retained owner is reused with a non-off policy but cannot apply it, the prepare is rejected with unsupported_configuration instead of silently keeping the old policy (rejecting was the smaller change versus retire/recreate). Off policy needs no action. Test added: an executor without an applier never receives a policy, gets no configuration_conflict, and stays reused for off. Promoting policy application to a Harness-declared protocol capability is recorded under Known follow-ups in the PR description.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 440073a827
ℹ️ 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".
| } | ||
| paths = append(paths, c.Path) | ||
| } | ||
| if len(paths) == 0 && item.Status == "completed" { |
There was a problem hiding this comment.
When history contains a terminal fileChange with status: "failed" but omits changes, this condition accepts it and renders only file change (status: failed):. A failed multi-file edit can leave partial filesystem changes, so the rebuilt model cannot identify what may already have been modified and may retry the edit against that partial state; require paths for failed as well as completed file changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c60b89d: any terminal fileChange (completed or failed) without affected paths is now treated as unsupported history. Test covers both statuses.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c60b89dc21
ℹ️ 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".
| if tid := s.reportedThread(); tid != "" { | ||
| doneMeta[proto.DoneMetaAgentSessionID] = tid | ||
| doneMeta[proto.DoneMetaAgentSessionType] = "codex_thread" | ||
| } | ||
| s.recordInflightRecoveryAbort() |
There was a problem hiding this comment.
When executor shutdown wins terminal while the rebuilt turn/start acknowledgement is finishing, this reads the old reportedID before recordInflightRecoveryAbort acquires recovery.mu; the acknowledgement can then acquire that mutex first, publish the new ID, and append a recovered record. The resulting Done pairs the old agent_session_id with an old-to-new recovered record, so Core drops the audit as inconsistent and persists the failed old thread as the resume target. Fresh evidence after the atomic acknowledgement fix is this remaining pre-lock identity read; snapshot the reported identity and recovery outcome under the same lock.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in db89d31: the abort is now recorded before the reported thread id is read, so Done metadata and recovery records stay consistent.
| Changes boundedList[struct { | ||
| Path string `json:"path"` | ||
| }] `json:"changes"` |
There was a problem hiding this comment.
When a completed fileChange represents a rename, Codex carries the destination in changes[].kind.move_path, but this decoder retains only path. A move from a.go to b.go is therefore rendered as affecting only a.go; the rebuilt model cannot identify the resulting file and may repeat the rename or operate on the obsolete path. Decode and render the move destination, or reject such history as unreadable.
AGENTS.md reference: AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in db89d31: move_path is decoded and rendered as 'old -> new' in the recovery history; test added.
| const exhausted = outcomes.filter((item) => item === "exhausted").length, aborted = outcomes.length - exhausted; | ||
| if (aborted > 1 || (aborted === 1 && outcomes[outcomes.length - 1] !== "aborted") || | ||
| (aborted === 1 ? p.recovery_count !== exhausted : p.recovery_count !== exhausted && p.recovery_count !== exhausted + 1)) return invalidAdminResponse(); |
There was a problem hiding this comment.
For an upstream failure whose base fingerprint is A, a response ending with an aborted recovery for fingerprint B passes this validation, although an abort settles the same failure that triggered that recovery and Core now rejects this combination at delivery. The client consequently accepts an impossible admin response instead of raising invalidAdminResponse. Fresh evidence beyond the Core-side fix is that this TypeScript projection still checks only ordering and counts; require the final aborted fingerprint to match the base upstream fingerprint.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in db89d31: the client now rejects an aborted recovery whose fingerprint differs from the base upstream fingerprint; tests updated.
…atch aborted fingerprint in client
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db89d319e5
ℹ️ 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".
| Turns boundedTurns `json:"turns"` | ||
| } `json:"thread"` | ||
| } | ||
| if rerr := r.call(ctx, "thread/read", map[string]any{"threadId": threadID, "includeTurns": true}, &read); rerr == nil && |
There was a problem hiding this comment.
When thread/turns/list is unavailable and the thread history exceeds 16 MiB, this fallback requests the entire history in one thread/read frame even though rpcStdoutBufferMax is only 16 MiB. The scanner rejects that response before the 64 MiB history bound can be evaluated, and readStdoutLoop then retires and closes the RPC client; instead of merely abandoning recovery and reporting the original provider failure, a long existing thread can therefore make the executor unavailable. The fallback must use responses guaranteed to fit the transport limit or the transport/history bounds must be aligned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 14cf1a2: removed the whole-thread thread/read fallback. History is read only via paginated thread/turns/list; if that is unavailable, recovery is abandoned (history_unreadable), the original failure is reported, and the RPC client stays open. Test added; limitation noted in the PR description.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14cf1a2dea
ℹ️ 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".
| Data boundedTurns `json:"data"` | ||
| NextCursor *string `json:"nextCursor"` | ||
| } | ||
| args := map[string]any{"threadId": threadID, "itemsView": "full", "sortDirection": "asc", "limit": historyPageLimit, "cursor": cursor} |
There was a problem hiding this comment.
After removing the whole-thread fallback, pagination still does not guarantee that a response fits the transport: limit caps turns, not their encoded size, so 20 individually valid turns whose full items total more than 16 MiB produce one frame above rpcStdoutBufferMax. The scanner fails before nativeTurnBytes can enforce the 64 MiB history bound, and readStdoutLoop then retires and closes the RPC client, turning an otherwise abandoned recovery into executor loss. Align the page/transport bounds or read these responses with a limit that can accommodate the permitted history size.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4739fa5: history is now paged one Turn per request, so a frame only exceeds the 16 MiB scanner limit if a single Turn does; page cap raised to 2000 to keep the 64 MiB byte bound reachable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4739fa56f5
ℹ️ 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".
| // historyPageLimit is one Turn per page: limit caps Turns, not bytes, so any | ||
| // larger page could exceed rpcStdoutBufferMax and retire the RPC client even | ||
| // when every Turn is individually valid. | ||
| const historyPageLimit = 1 |
There was a problem hiding this comment.
When one historical Turn contains more than 16 MiB across multiple individually valid items, limit: 1 still returns the entire Turn in one NDJSON response. readStdoutLoop caps frames at 16 MiB and retires the RPC client on scan failure, while this reader otherwise permits up to 64 MiB of history, so recovery turns the original recoverable 400 into executor loss instead of merely reporting history_unreadable. Fresh evidence after the prior pagination fix is that limiting the page to one Turn does not bound the encoded bytes within that Turn; paginate its items or align the history and transport bounds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepting this as a known limit, not changing code. thread/turns/list has no item-level pagination, so the reader cannot bound a single Turn >16 MiB, and raising the global rpcStdoutBufferMax would raise the memory ceiling of every RPC. Normal conversations do not reach 16 MiB in one Turn. Checked in code that a retired client is not permanently unavailable: Executor.StartTurn rejects a non-alive client ("executor unavailable"), and in dispatch/prepared_handoff.go a Turn with nativeErr or a non-reusable settlement marks the owner invalid, closes it and deletes it from r.executors, so the next prepare builds a fresh executor that resumes the native session (resumeID). The recoverable 400 degrades to executor loss for that Turn, not to a stuck executor. Recorded under Known limitation in the PR description.
This branch has not been deployed
1 participant
Summary
Agents conversations were being cut off mid-way by upstream model 400 errors. In 7 days of Singapore production traffic from
oac-daemon, 12 of 407 requests hit an upstream 400 across 9 threads, and 8 threads ended at that request. The gateway does not retry or fail over on 4xx, so the executor reported the turn as failed with no usable cause.This PR adds two things in Core and the daemon:
/diagnosticsexposesfingerprint, a redacted length-limitedsummaryandupstream_request_id.Fingerprints
provider_messages_requiredfield messages is requiredreasoning_stateimage_inputtool_result_imageinput_imagerejected insidetool_resulttrailing_assistantAny 400 outside the allow-list keeps today's behaviour.
Safety limits
effectiveUsageLocked) so a recovered turn is not double counted.Steerduring recovery returnsErrSteeringNotReadyso Core keeps and resends the input instead of failing the turn.Notable decisions
X-Oac-Upstream-Recovery: v1(wire version stays0.16.0) instead of a newAgentKindCapabilitiesfield. Capability decoding rejects unknown and missing fields, so adding one would make a new Core reject every existing Runtime heartbeat and break rollback.beta(the pre-release line production deploys from), notmain.turns.outcome.recoveriesand Core logs only. It is not shown in the UI and/diagnosticsdoes not expose it for non-failed turns. The product owner has accepted this.Release order
failure.goallow-list change first (new codes must not be treated as contract errors).betaand deploy Core with the switch off.reasoning_statefirst, thenimage_input. Observe, then widen.Rollback: turn the switch off. It is read per connection, no data migration.
Testing
Passing:
make check-runtime-contract,make check-openapi,make check-names,go test -race ./apps/daemon/internal/agent/codex/...,go veton the codex and execution packages. New cases cover each fingerprint, attempt and time limits, cancellation, non-allow-listed 400, switch off, usage, steering during recovery and snapshot rollback.Not verified:
make check-coredatabase integration was skipped (noOAC_TEST_DATABASE_URL).mcodetests fail, and they fail onorigin/betaas well on macOS.b8819563deploy went to still needs confirming with ops.Known follow-ups:
packages/agents-client/src/session-diagnostics.tshand-copies the fingerprint/code/summary table from Go/Core, so it can drift. A mechanical test pins the TS code set to the four contract codes (Go has the matching test). Replacing the copy with a shared fixture or generated projection is tracked as a separate follow-up.Core chooses to send recovery controls by
AgentKind == "codex"(services/core/internal/execution/request.go). Only codex supports recovery today, Core sends the block only to codex, and the daemon also rejects it for other Harnesses. Declaring recovery support per Harness needs a new protocol capability field (e.g. inAgentKindCapabilities) and Core validation, which would make Core reject existing Runtime heartbeats, so it needs its own compatible-migration design and is tracked as a separate requirement.Applying recovery policy to a retained executor is an optional
recoveryPolicyApplierinterface in the daemon, not part of theagent.Executorprotocol. A reused owner that cannot apply a non-off policy is rejected withunsupported_configuration(fail safe); the codex executor is pinned by a compile-time assertion. Promoting policy application to a protocol capability declared by the Harness (about 12 implementers plus contract tests) is tracked as a follow-up next to the Harness recovery capability declaration above.Recovery reads history only through paginated
thread/turns/list; there is no whole-threadthread/readfallback (a response above the 16 MiB RPC frame cap would retire the RPC client). Older Codex withoutthread/turns/listcannot auto-recover: recovery is abandoned ashistory_unreadableand the original provider failure is reported, the RPC client stays open and the executor remains reusable. Production codex 0.153.4 must be confirmed to supportthread/turns/listduring beta calibration.Known limitation: a single historical Turn larger than 16 MiB still arrives as one oversize
thread/turns/listframe (the protocol has no item-level pagination), so the stdout scanner fails and the RPC client is retired. A recoverable 400 then degrades to executor loss; the dispatch layer invalidates and closes that owner and the next prepare builds a fresh executor that resumes the native session. Normal conversations do not reach this size. We do not raise the globalrpcStdoutBufferMax, which would raise the memory ceiling of every RPC. Item-level pagination can be negotiated with Codex later.