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. |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR substantially rewires production interception, streaming, retry/idempotency, trace-commit, and tool-gating behavior across core modules rather than making a contained refactor. An unresolved review comment also identifies a transport-cleanup failure path that may incorrectly suppress retries. Notes:
No code changes detected at You can add or adjust custom eligibility rules. Learn more. |
575cf75 to
3bac8cd
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
c42001c to
1558d27
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1558d27247
ℹ️ 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".
| finally: | ||
| await reply.close() |
There was a problem hiding this comment.
Preserve retryability when stream cleanup fails
If reply.close() raises—for example, while a transport is closing a reset streamed connection—the exception originates in this finally block and therefore bypasses all of the preceding except mappings. It reaches InterceptionServer._fail() as a plain exception, which converts it to a non-retryable 400 InterceptionError; this prevents the harness from retrying what is still an upstream transport failure and can also replace the more useful exception from stream consumption. Map cleanup failures to ProviderError/model_error (without masking an existing error) before they leave _complete().
Useful? React with 👍 / 👎.
1558d27 to
edd9f34
Compare
edd9f34 to
b0a3837
Compare
An intercepted model call has one session-owned task for hooks, inference, response encoding, and trace commit. Retries share that task and reuse its completed result. Tool decisions and rewritten results live in one record per call occurrence.
Before —
mainThe server already coalesces running requests and replays completed responses, but maintains a separate in-flight future, response cache, and finalization callbacks. A normal socket disconnect can leave inference running; cancellation of the original handler can cancel its producer. Tool state is split across decision, result, and gate-tracking collections.
flowchart LR H[Original request] --> P[Handler-owned producer] P --> C[Client get_response or relay] C --> F[Finalize attempt] F --> I[In-flight future] F --> R[Completed-response cache] Q[Concurrent retry] --> I L[Later retry] --> RAfter — this PR
One retained task represents both the running attempt and its result. HTTP readers wait through a cancellation shield; a disconnected or cancelled reader does not cancel the model call. Ending the rollout cancels the session's outstanding tasks. The client completes inference while the server owns HTTP delivery, keepalives, and error framing.
flowchart LR H[Original request or retry] --> D[HTTP delivery and keepalives] D -->|Shielded wait| T[Session-owned task] T --> C[Hooks and client completion] C --> E[Encode response before trace commit] E --> R[Task retains result for replay] R --> D X[Rollout ends] -. cancels .-> TFailure handling follows the commit boundary:
ToolStatekeeps the blocked decision, gate consultation, and replacement result together. Repeated histories receive the same rewritten result. Request hooks and tool pre-checks share their evaluator. The retained turn task also acknowledges input delivery after previewing the rewritten request, keeping notification delivery tied to the actual model turn.Matching explicit idempotency keys or marked SDK retries share work; fresh identical unmarked requests remain independent. Replay is bounded and local to the rollout session; it does not survive a process restart. Completion content remains buffered until the turn is ready, with keepalives during the wait. This PR owns lifecycle and tool state. #2675 owns dialects, sampling, and SDK transport; #2676 owns client unification and training records.
Live CI defaults unpinned agent roles to Docker; explicitly configured runtime placements remain intact.
Stack:
main→ #2674 → #2675 → #2676. The server/trainer companion is prime-rl #3660.Validation on current main (e71c69c): all 163 unit tests passed on this branch. The full stack also passes 175 unit tests and contributor style/type checks.
Note
High Risk
Changes core interception idempotency, streaming error/retry semantics, and tool-gate enforcement at the model boundary; incorrect retry or coalescing could duplicate turns or let blocked tools run.
Overview
This PR unifies intercepted model turns around one session-owned
asynciotask that handles hooks,client._complete, response encoding, and trace commit. Concurrent SDK retries and idempotent replays coalesce on that task (shielded from reader disconnect/cancel); retryable failures are dropped from replay storage so a later retry can run again, while completed or post-commit errors stay replayable.Eval client now owns streaming completion:
EvalClient._completerelays SSE, parses the stream, and returns parsedResponseplus raw events;RelayReplyno longer carriescontent-type. The interception server focuses on HTTP delivery, keepalives, and framing errors withretryableonReplayResponse(including SSE after commit).Harness
chat()retries transport errors andAPIErrorbodies withretryable: true; terminal API errors do not retry (new unit coverage).Tool policy moves into per-occurrence
ToolState(blocked, gate consulted, pinned result), replacing split decision/result/gate maps; request rewriting skips re-running hooks on already-pinned tool results and shares_run_request_hookswith pre-checks.Tests/CI: integration eval config defaults each agent seat to
runtime: dockerwhen unset.Reviewed by Cursor Bugbot for commit b0a3837. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Simplify interception turn lifecycle with shared replay tasks and
ToolStateInterceptionServer.handle_requestto coalesce concurrent identical requests onto one sharedReplayResponsetask, replay completed non-retryable results, and drop retryable failures from replay storage in server.pyEvalClient._complete, which returns the parsed response plus original data events and maps timeout/transport failures to defined error types in eval.pyToolStateso blocked tool results cannot enter a model request before their gate is consulted, and request rewriting no longer re-runs hooks on already-pinned results in session.pyRelayReplydrops its content-type field andIdempotentRequestreplaces separate response/future fields with a single task; the removed_collect_streamand response-capture helpers mean any out-of-tree consumers of these shapes must be updatedMacroscope summarized b0a3837.