Repository navigation
fix(codex): retain session evidence and report model errors - #4740
giuliolovisotto merged 7 commits into
Conversation
giuliolovisotto
added
bug
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
giuliolovisotto
added
the
needs-review
yaoyu-33
added
the
complexity:medium
giuliolovisotto
changed the title
b1c3313 to
03c195b
Compare
github-actions
Bot
added
the
sla:triage-overdue
yaoyu-33
left a comment
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for the Codex fix. Main points: failed responses reaching /verify without a masking policy, no runner-level test of the proxy wiring, Retry-After: nan killing the handler thread, and a stale README line. Optional: in-stream failures after HTTP 200 still raise 502, error.code is always server_error, the native transcript parse drops event-derived tool records without a gap, rollout files are read unsorted, and the seeded sandbox_transport.py upload has no test.
Review context: these five PRs (#4782 contract; #4739 Pi, #4740 Codex, #4741 OpenCode, #4742 Hermes) are a split of one change. This PR's diff vs main also contains #4782's conformance files, so please retarget/rebase once #4782 merges. Note that harness_conformance runs all four probes for any shared-file change, so model_error should fail for every harness until its fix lands; if that check is required, #4782 and the siblings need to land together.
PR hygiene: the validation commands and live-smoke artifacts reference an author-local path, and current-head CI isn't in the evidence (the combined 28/28 run is for 118f1de37). Could you attach the exact commands and the CI run for this head?
Generated by Claude Code
| status=status, | ||
| incomplete_details={"reason": "max_output_tokens"} if output_limited else None, | ||
| error=None, | ||
| error={"code": "server_error", "message": terminal_error} if reported_model_error else None, |
There was a problem hiding this comment.
This now returns recognized Codex provider errors (any 400-599) as a failed response that goes on to /verify, which matches the #4782 contract. But I couldn't find anything on the environment-server side that masks it: single_agent_turn forwards the response unconditionally and mask_sample is only set by individual verifiers. Before this change a provider error raised a 502 execution failure, so a transient provider error can now become an unmasked reward-0 sample. Could you point to where that is masked, or land the masking/retry policy first (or limit this to model-caused statuses, as Hermes does for 400/413/422)?
There was a problem hiding this comment.
Done—#4834 now applies the shared grading/retry policy before verification, and this PR is stacked above it so provider execution failures retain their evidence without becoming scored reward-zero samples.
| # Load only the adapter file staged beside this worker. | ||
| import runpy | ||
|
|
||
| proxy = runpy.run_path(str(Path(__file__).with_name("sandbox_transport.py")))["rate_limit_proxy"] |
There was a problem hiding this comment.
Nothing exercises sandbox_runner.run() with model_base_url: the runpy load of sandbox_transport.py, the -c model_providers... insertion into the command and the model_attempts.jsonl output are only covered by direct calls and fake-sandbox fixtures. Could you add a runner-level test with a fake codex script and a 429-then-200 upstream?
There was a problem hiding this comment.
Done—test_runner_installs_proxy_and_retains_retry executes the real runner with a fake Codex script and a 429→200 upstream, asserting command wiring, invocation correlation and saved attempts.
| delay = float(upstream.headers.get("Retry-After", 2**attempt)) | ||
| except ValueError: | ||
| delay = 2**attempt | ||
| sleep(min(max(delay, 0), 30)) |
There was a problem hiding this comment.
float(Retry-After) accepts nan, and sleep(nan) raises ValueError in the handler thread, so Codex sees a dropped connection and no attempt is recorded. Could you reject non-finite delays (fall back to the backoff) and also catch URLError/OSError around urlopen, so a transport failure is recorded instead of leaving an earlier HTTP status as the last one?
There was a problem hiding this comment.
Done—the proxy falls back to finite backoff for invalid Retry-After values and records connection failures separately from prior HTTP statuses, with regression tests.
| ) | ||
|
|
||
|
|
||
| def _terminal_model_error( |
There was a problem hiding this comment.
Docs: README.md:159
This line still says provider/API/runtime failures "raise execution errors and bypass verification", but a recognized terminal provider error now returns a failed response that can reach verification. Could you update it to describe that, and that execution/cleanup failures still raise?
There was a problem hiding this comment.
Done—the README now distinguishes reported provider failures handled by Environment Server policy from worker and cleanup failures that still raise execution errors.
github-actions
Bot
removed
the
sla:triage-overdue
e5240e1 to
2475011
Compare
2475011 to
baa9a75
Compare
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39785743381 |
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39799329842 |
Review follow-up on baa9a75 (2026-10-22)
Build session observations once from the selected source with the final invocation outcome. Name the retry limit and outcome markers; warn for Codex versions other than the tested 0.144.4. Unknown transcript record types produce explicit gaps, and an invalid latest token-count record yields unavailable usage rather than an older total.
Validation: 208 focused Codex tests passed, 9 skipped on macOS; 25 Linux parser/CLI tests passed, including all three tests using the real pinned Codex executable. The new CLI regression completes a tool call, receives HTTP 400, and retains the actual transcript's 49-token total despite no turn.completed event. All seven fresh Codex conformance scenarios passed. Scoped pre-commit passed. Linux checks use a scripted local model endpoint, not live inference; Codex source files are identical after the ancestor-only restack.
Follow-up on 2475011 (2026-10-22)
Rebased with the shared policy and harness changes; Codex implementation is unchanged in this follow-up. Earlier in this rebase, its adapter and rollout-collection test selection passed 301 tests with 48 skipped. Scoped pre-commit on the final combined stack passed.
Rebase validation before the label/formatter follow-up: the combined tip
e5240e16epassed 28/28 Linux conformance scenarios, seven per harness. The policy-only heada928f859epassed 4/7 Hermes scenarios; the dependent stack fixes its evidence gaps. Both returned seven Hermes rollout records with no episode-failure rows; these scenarios do not establish exhausted-provider retry counts. These results are pinned to those revisions, not a claim of fresh CI or a new live-model run on the latest heads.Restore native transcript, tool identity and cumulative usage in agent sessions, including custom-tool and web-search records. Stage the sandbox-local 429 proxy, retain HTTP outcomes and post-header transport failures, and reject non-finite Retry-After values. Test native Codex 0.144.4 error strings and staged proxy contents; report confirmed 429 as
rate_limit_exceededwhile keeping the precise failure kind inAgentOutcome.Stack: #4834 (policy) → #4742 (Hermes) → #4739 (Pi) → #4741 (OpenCode) → #4740 (Codex), registered as GitHub stack #4899. These changes must land together. Do not merge #4834 alone: the Hermes and Codex classifier fixes are in #4742 and #4740.
Scope: agent-session execution only; legacy paths are not part of this follow-up. No conformance expectations or shared policy defaults are changed.
Validation (2026-10-21)
d0b6b950a34bfbc63dfbfdf23c4abbf6d2e1bf34: 28/28, including Codex 7/7; this is combined-stack evidence, not an isolated-base claim.python -m pytest responses_api_agents/codex_agent/tests --import-mode=importlib -q -o addopts='' -o log_cli=false python scripts/run_harness_conformance.py --harness codex --timeout 300 --output /tmp/fresh-codex-conformanceFresh CI on combined stack tip
d0b6b950a: this harness passed 7/7 (download its report/tests/logs); all four stack harnesses passed, while the separate Kilocodemodel_errorfailure keeps the non-blocking aggregate red (full CI summary).Final CI validation: this combined head also passed the core unit tests and all eight server-test shards; the only failing checks are the separate Kilocode conformance probe and its aggregate reporter.