[mpatel31415]

OpenSandbox allocation, readiness, lifecycle, command, file and PTY failures use the shared NgFailureEvent, registered kinds and typed details. Providers preserve observed evidence and causes; caller policies select actions.

Bottom of #5127 → #5128 → #5129 → #5605, targeting main. Refs #4685. Includes the CI corrections from #5671.

• Uncertain command delivery is never replayed. Status recovery follows the same execution ID; retries, diagnostics, cleanup and downloads retain their existing budgets. Successful recovery retains failure observations.
• Confirmed OOM requires current user-container evidence. Exit 137, arbitrary output and sidecar OOM are insufficient. fix(sandbox): confirm OOM from control-plane lifecycle reports #5605 completes the trusted lifecycle-report path.
• Expected pre-execution root-user refusal uses control flow without a failure observation. AnySWE grading failures stay ungraded; Harbor retains its image-pull retry predicate and budget.
• failure_kinds.py owns FailureStage, FailureClass (model, evaluation, unknown) and the configurable stage-aware classification mapping. Existing model/server views derive from that mapping; server remains an evaluation subcategory.
• Action priority is HTTP status → failure kind → supplied failure class → default. Boundaries validate supported actions and execute them within their existing budgets. Changing an action does not change classification.
• The shared defaults count admitted agent-container OOM as zero/model, allow safe bounded seed-OOM retry/evaluation, and treat admitted inconclusive verification as evaluation-side. Unknown context stays conservative. OutcomeDecision reuses FailureClass; no new persistence container is introduced.

#5128 supplies OpenCode stages. #5129 applies the shared policy and connects saved decisions to recovery and health. Other providers, general reverification/completion migration and a new seed retry engine remain outside this stack.

Compatibility and limits

The removed OpenSandboxCreate*Error, OpenSandboxSetupError and SandboxBackendUnreachableError names are replaced by NgFailureEvent plus an explicit boundary/kind check. Other providers' migration is separate.

The old elapsed-time/signal heuristic could relabel an unconfirmed exit as (124, "timeout"); this change preserves an observed (-1, None) instead. Consumers that grade on error_type == "timeout" must not assume that unknown outcome is a confirmed deadline kill. A session deadline that Gym actually observes still produces its timeout result.

Diagnosis stays inside the execution hard cap: once it expires, no new diagnostic I/O starts, so OOM may remain unconfirmed. When time remains, an initial nonterminal inspect can be followed by one final inspect after lifecycle termination, under the same deadline. #5605 adds recognized control-plane lifecycle confirmation by default; #5127 alone retains opt-in inspect confirmation.

Validation

• Assembled stack: 1,354 tests passed; after final reader changes, 552 integration tests passed. Core: 202 tests; health/recovery: 849 tests plus 169 targeted tests after the last reader adjustment. These selections overlap and are not an aggregate count.
• Docs and SWE-bench checks passed. Scoped pre-commit/import-linter passed. Fern: 0 errors, 1 warning.
• Claude independently reviewed each implementation area; one fix pass addressed the findings. Commits are SSH-signed with DCO.
• Fresh model-backed OpenCode → DeepSWE cases passed: healthy reward 1 and confirmed agent OOM reward 0, unmasked, with its canonical event and count_as_zero/model decision preserved. Inspect fallback was disabled. Three model calls succeeded, capture was retained, and all four disposable sandboxes were confirmed deleted.

The live source was 1408b918243a0c3203c0175b1d4e495cdb57007d. Subsequent changes affect only downstream health/recovery readers; the final stack 34c793c8e0ff37669720925e9973e93b5fca7181 reloaded both real artifacts, preserved both scores and selected neither task for rerun. A changed classification configuration did not override the OOM result's saved class.

Controlled cases validate failure handling, not representative benchmark scores or a complete run-health verdict. These fixtures lack the complete grading/coverage evidence required to certify health; that verdict remains UNVERIFIED. Full repository CI is not claimed by these local checks.

Latest review fixes: the final assembled selection at 6f247486a3ff58b72ca9d83933960f4c547e6c2d passed 949 tests; scoped pre-commit and import-linter passed. Individual fixes received independent Claude review and one fix pass. These checks do not assert full repository CI or benchmark-wide score invariance.

Final-source live snapshot check at 6f247486a3ff58b72ca9d83933960f4c547e6c2d: a real-model OpenCode → DeepSWE control retained reward 1, primary output/root usage and the original failure event after an injected optional snapshot download timeout. The preceding attempt stopped before injection with an unknown command exit; it is retained as a failed attempt, not counted as validation. Two calls on the successful retry plus one on the first attempt were captured, and all five disposable sandboxes were confirmed deleted. This is a synthetic plumbing check, not representative benchmark qualification.

[copy-pr-bot]

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.

[github-actions]

🌿 Preview your docs: https://nvidia-preview-pr-5127-1d4658ced9cd.docs.buildwithfern.com/nemo/gym

[giuliolovisotto]

Please address these points in the provider failure handling:

1. Require authoritative OOM evidence everywhere. Remove message-based OOM heuristics, including the PTY path. Report OOM only when the current sandbox/container explicitly reports it. A timeout, disconnected terminal, exit 137, or suggestive message text is insufficient. When evidence is unavailable, preserve the original failure with its cause unknown.

2. Consolidate evidence collection. Give execution and PTY callers one shared entry point, with small helpers for fetching and parsing evidence. Pass the remaining recovery deadline so repeated calls cannot restart the diagnostic budget. Return structured evidence, including its source, rather than relying on a notice string to carry the classification.

3. Make timeout and retry policy configurable, following OutcomePolicy. Move command timeout headroom, status recovery, evidence polling, and per-request diagnostic timeouts into OpenSandbox configuration. Replace the hardcoded retryable HTTP-status set with configurable policy rules. Follow the existing OutcomePolicy implementation in nemo_gym/agent_outcome.py: AgentOutcomePolicyConfig and agent_outcome_policy(): status/kind rules, overrides merged into defaults, and explicit precedence. Reuse the rule-selection/configuration approach while keeping supported actions, retry execution, and budgets scoped to the operation. A timed-out command submission must not be retried when it could execute twice.

4. Keep failures structured and preserve their causes. Catching TimeoutError is fine, but retain the failed operation, failure kind, HTTP status where available, and original exception cause as structured information. Centralize translation into the current exception types so callers do not need to infer meaning from messages or provider-specific subclasses. A request timeout means the command outcome is unknown unless there is separate evidence of command termination.

5. Clarify the inspect API dependency. This code parses a deprecated, human-readable endpoint. Is there a supported structured source for the same evidence in the deployed OpenSandbox version? If this fallback is necessary, document the supported server versions and formats, and preserve the existing tests rejecting stale state, sidecar OOM, and ambiguous output. Docker support itself is valid because OpenSandbox can use that backend.

[giuliolovisotto]

Follow-up to the bot review: idempotent lifecycle reads now honor configured retries, foreground diagnosis stays within the execution deadline, and AnySWE retains consumed agent-phase events. The default-user fallback now uses a provider-confirmed pre-launch control result without recording an expected refusal as a failure; regression coverage exercises the provider translation and rejects uncertain replay.

[giuliolovisotto]

Follow-up to Yuhe's review: the default-user fallback, PTY busy classification, deadline-test reliability, client-wait classification and migration guidance are addressed, with details in the inline replies. The combined selection passed 696 tests (6 skipped; one independently reproduced macOS archive case deselected); no new live rollout was run for these follow-ups.

[yuhezhang-ai]

Thanks for the follow-ups. Rechecked 952f3133: my four earlier threads are addressed, so I am resolving them. The bot's default-user fallback finding is also addressed: fallback requires confirmed pre-launch refusal, while uncertain submissions remain single-attempt. I am recording that verification here without opening a duplicate finding.

Two small remaining corrections are inline: the new policy table fails an existing documentation check, and a nonblocking note distinguishes a container exit code from an unobserved command exit. I also checked the relevant fixes in #5129 and #5605 and am adding the results to the existing threads. Validation was local tests and source tracing; I did not run a new live model/OpenSandbox rollout.

[AdamRajfer]

Thanks for the rework. The lifecycle OOMKilled attribution, the terminal-state typing, the stage policy not widening MODEL_FAULT_KINDS and the OOM docs all address my earlier points. Not approving yet:

• CI is red on two failures from this PR; #5671 (into this branch) fixes both, merge it and CI reruns.
• Exec and PTY diagnosis can still miss a real OOM (inline).
• Removing timeout detection changes scoring for existing consumers (inline).
• With default config the 410 / OOMKilled case is still provider_unavailable, since both confirmations are opt-in. Fine if the enabling lands in #5605, but please say so in the description.

Nits:

• The stage policy and the new SandboxObservation.stage have no consumer in this PR. Say so, or move them.
• Removed public exception names need a line in the description.

[AdamRajfer]

Inspect runs once per exec/PTY call. If the container is not yet terminated then, a later terminal lifecycle state returns provider_unavailable without re-inspecting, downgrading a real OOM. Re-inspect when the lifecycle turns terminal, within budget.

[yuhezhang-ai]

Independently reproduced with controlled provider inputs on #5127 5582d7199 and the included #5605 34c793c8, for both foreground exec and PTY. With inspect_oom=True, lifecycle first reports Running and inspect has no termination evidence; the next lifecycle read reports Failed/OOMKilled without a detailed container report. Only one inspect occurs, so the call returns provider_unavailable while diagnostic time remains. An immediate new diagnostic call against the same stubbed provider confirms provider_oom_killed.

This is the optional inspect fallback path; #5605's matching persisted lifecycle report can already confirm OOM without inspect. Rechecking inspect on the terminal transition within the remaining budget would address this case without restarting the deadline. This was a controlled reproduction, not a live sandbox rerun.

[giuliolovisotto]

Fixed in 1d4658c: after an initial nonterminal inspect, a later terminal lifecycle observation permits one final inspect within the original deadline. Regressions cover late OOM, missing/failed evidence and exhausted budgets, with at most two inspect calls per helper invocation.

[yuhezhang-ai]

Verified at #5127 1d4658ced9 and the included #5605 6f247486a3: the same controlled Running/empty-inspect → terminal-lifecycle sequence now confirms provider_oom_killed for both foreground exec and PTY on the second inspect, before the original deadline. The added missing/failed-evidence and exhausted-budget cases also pass; the helper performs at most one final recheck after termination. The specific issue I reproduced is addressed. No new live sandbox rollout was run during this verification.

[AdamRajfer]

On the hard-cap path the diagnostic deadline is already past, so the loop never runs and a wedged OOMed command stays plain provider_io_timeout. Give diagnosis its own bounded budget, as the PTY path does.

[yuhezhang-ai]

The no-read behavior after the hard cap is intentional: it implements the execution-bound fix requested in the earlier bot review. I reran the controlled foreground deadline tests at 5582d7199; they verify that diagnosis uses remaining time and starts no request after the outer cap.

If we need an OOM lookup for a wedged command too, I would reserve diagnostic time inside the total execution bound, or explicitly redefine/document that bound. Starting a fresh budget after the existing cap would restore the overrun that the earlier fix removed. Until evidence is available, keeping the outcome unknown is consistent with the current contract.

[giuliolovisotto]

I kept diagnosis inside the execution hard cap: starting a new budget after expiry would reintroduce the unbounded-by-execution diagnostic tail fixed in the earlier review. Once the cap is exhausted we retain provider_io_timeout and its cause, with remote outcome unknown; the description now states that this path can leave OOM unconfirmed.

[AdamRajfer]

Dropping timeout detection now returns (-1, None) instead of (124, "timeout"). Callers keying on error_type == "timeout" (session helper, oracle, testbench runner) stop seeing timeouts. This changes scoring beyond OOM; call it out and agree it, or split it.

[giuliolovisotto]

This is intentional and now called out in the description: elapsed time plus a signal/unknown exit cannot establish a command deadline kill, so we no longer rewrite that evidence to (124, "timeout"). Gym-observed session deadlines still produce a timeout result; preserving this distinction avoids assigning a score from an unconfirmed cause, while broader consumer timeout policy remains separate.

[giuliolovisotto]

The review fixes are published at 1d4658c, and the inline replies explain the retained execution-cap and timeout-evidence behavior. #5671's two CI corrections were already incorporated before this round (that PR is now closed); the description also names the removed exception APIs, identifies #5128/#5129 as the stage-policy consumers, and makes clear that #5605 enables trusted lifecycle confirmation by default. The assembled local selection passed 949 tests; that does not certify repository CI.

[yuhezhang-ai]

Thanks for the follow-ups. Rechecked 1d4658ced9; the issues from my review are addressed, including the verified fixes in the included #5129/#5605 stack. The late-inspect reproduction now confirms OOM within the original deadline, focused checks pass, and current CI is green.

[AdamRajfer]

Thanks for the fix round. The late OOM re-inspect with its tests covers my main point, and the description now states where OOM can stay unconfirmed under the hard cap and that a kill is no longer read as a command timeout. Fine with the trade-offs as documented. Approving.

[github-actions]

🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/42069655984

[github-actions]

🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/42076364663