Repository navigation
fix(opencode): retain sandbox identity and report session errors - #4741
Conversation
|
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. |
f4ccfd1 to
5f19114
Compare
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for the OpenCode fix. The classification is conservative and the tests assert real behavior (567 passed, 57 skipped locally). Main points: failed responses reaching /verify without a masking policy to receive them, status-less provider errors still raising 502, and a stale README line. Also optional: sandbox_id can be empty for ECS handles and the sync Sandbox has no equivalent, the serialized native error is unbounded, and the classifier's rejection guards are untested.
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
| output=output, | ||
| usage=usage, | ||
| status=status, | ||
| error={"code": "server_error", "message": json.dumps(model_error)} if model_error else None, |
There was a problem hiding this comment.
This now returns recognized OpenCode 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, which the environment server retries or drops, 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—the stack includes #4834’s shared policy, which skips verification for provider execution failures and retries transient failures while leaving grading of 400/413/422 configurable.
| return None | ||
| info = assistants[-1] | ||
| error = info.get("error") | ||
| if not isinstance(error, dict) or error.get("name") != "APIError": |
There was a problem hiding this comment.
Does OpenCode emit an APIError without a statusCode for connection resets, header timeouts and stream errors, and a separate context-overflow error? If so those still end up as a 502 here instead of a failed response, which seems at odds with "terminal provider failure => failed response" (and Hermes grades the equivalent 413 as a rejection). Could you accept status-less APIError, or note that these are intentionally still raised?
There was a problem hiding this comment.
Status-less APIErrors now return classified failed responses using OpenCode’s retryable transport marker, while separate non-APIError failures, including context overflow, still use the existing exception path.
There was a problem hiding this comment.
Done in 90ed544: status-less retryable API errors remain classified transport failures, while native ContextOverflowError returns incomplete with partial work retained and independent cleanup failures still raised.
| ) | ||
|
|
||
| @staticmethod | ||
| def _terminal_model_error(export: dict[str, Any], cleanup: dict | None) -> dict[str, Any] | None: |
There was a problem hiding this comment.
Docs: README.md:122
This line still says "Runtime/model errors fail before 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 API errors governed by the Environment Server policy from worker and cleanup execution failures.
4fc749f to
30c3e0b
Compare
|
|
||
| @staticmethod | ||
| def _reported_model_error(native_result: Mapping[str, Any], cleanup: dict | None) -> bool: | ||
| """Recognize a completed API error or context limit; tested on OpenCode 1.17.11. |
There was a problem hiding this comment.
Do we expect this to hold across harness versions? If not, is there a way we could flag that this check might not give the correct result on a different version?
There was a problem hiding this comment.
Added a warning in 4605a7ecd: session setup flags any configured OpenCode version other than the tested 1.17.11, explaining that error classification is untested on that version. An alternative exact version remains usable. Tests cover both warning and no-warning cases.
30c3e0b to
4605a7e
Compare
|
|
||
| Independent worker failures, cleanup errors and timeouts are not provider reports. | ||
| """ | ||
| finishes = {"APIError": None, "ContextOverflowError": "error"} |
There was a problem hiding this comment.
It would be good to define constants - we use these strings in several places
There was a problem hiding this comment.
Done in 71f37cfb8: APIError and ContextOverflowError now have shared constants used by error recognition, classification and context-limit handling. Existing tests retain the literal upstream names to catch accidental value changes. All 254 focused OpenCode tests and scoped pre-commit checks passed.
| if isinstance(failure, asyncio.CancelledError): | ||
| record.error_type = "cancelled" | ||
| elif context_limit: | ||
| record.error_type = "context_limit" |
There was a problem hiding this comment.
double checking: we don't rely on particular values anywhere, right? this is more like metadata/comment than a classification that is consumed downstream?
There was a problem hiding this comment.
Yes, for this field: current Gym consumers do not branch on the "context_limit" value in AgentInvocation.error_type; it is descriptive observation metadata. Grading/retry decisions use the response status and the separate typed ng_agent_outcome.failure_kind. This context-limit path returns incomplete, so it remains gradable.
There was a problem hiding this comment.
@marta-sd for now with these PRs I'm trying to add as much coverage as I can so that components can make decisions upstream on what to do with this outcome (the main use-cases now for using this info is to decide is whether to "retry", "grade even if incomplete", "exclude from scoring")
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39785736592 |
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>
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
|
/ok to test |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/39799272539 |
Review follow-up on 4605a7e (2026-10-22)
Warn during agent-session setup when the configured OpenCode version differs from the tested outcome-classifier version, 1.17.11. Keep explicitly pinned alternative versions usable.
Validation: 198 OpenCode session/review tests passed, including the warning and no-warning cases; scoped pre-commit passed.
Follow-up on 30c3e0b (2026-10-22)
252 OpenCode session, continuation and error-formatting tests passed; scoped pre-commit passed. Explicit
ContextOverflowErrorreports usemodel_context_limit; HTTP 400/413 alone remainsmodel_request_error. Context overflow still returnsincompleteand retains partial work. The renamedformat_error_messagehelper retains the same 16,000-character bound.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.Retain the provider-assigned sandbox ID and reported API failures in agent sessions. Classify status-less retryable API errors as transport failures; retain native context-overflow stops as incomplete with partial work, while independent worker/cleanup failures still raise. Provider error text includes only the message and status, bounded to 16,000 characters; structured classification is retained separately.
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 OpenCode 7/7; this is combined-stack evidence, not an isolated-base claim.python -m pytest responses_api_agents/opencode_agent/tests --import-mode=importlib -q -o addopts='' -o log_cli=false python scripts/run_harness_conformance.py --harness opencode --timeout 300 --output /tmp/fresh-opencode-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).