feat(tbench2_env): report execution budgets in the reset observation - #1203
KNambiarDJsc wants to merge 13 commits into
Conversation
…uggingface#1026) reset() now includes verifier_timeout_sec in info (task.toml [verifier].timeout_sec, else the 900s fallback that evaluate enforces) in both local and Docker modes, and command_timeout_s in local mode. Clients can derive per-message deadlines from the server instead of guessing. Docker mode omits command_timeout_s because exec_run has no server-side timeout, so the value would not be enforced. Additive: existing info keys are unchanged. Closes huggingface#1026
|
cursor review |
There was a problem hiding this comment.
REQUEST_CHANGES at 6ffbdf63 (Ben ask: cursor review)
Reporting budgets in reset info is a clean improvement, and sharing _DEFAULT_VERIFIER_TIMEOUT_S between reset and evaluate is right. Focused tests pass. Two blockers remain around the new contract:
- Docker fallback ignores the advertised budget — see inline on the Docker reset report.
_read_timeoutfloat safety — reset now always calls it for the new field; malformed/non-finite values break the session or the JSON contract.
Residual (alignment, non-blocking for merge if 1–2 land): local mode can still mutate task.toml between reset and evaluate so reported vs enforced budgets drift. Cache the resolved timeout on the env instance at reset if you want the published value to be authoritative.
Env-only — not 0.6.0 wheel cargo.
Sent by Cursor Automation: Release
|
cursor review |
There was a problem hiding this comment.
APPROVE at eee0504d
Prior Tier-1 blockers are fixed:
- Docker fallback budget —
_fallback_eval_cmd(..., timeout_s=...)now wraps the verifier intimeout; missingtimeoutbinary fails closed (exit 127) instead of running unbounded. _read_timeoutsafety — TypeError/ValueError/OverflowError → default; non-finite / ≤0 → default; covered by the new invalid-budget matrix.
Validation: 51 passed, 1 skipped; lint/format clean.
Non-blocking residual (do not gate merge): evaluate still re-reads task.toml, so local mode without withhold_tests can drift the enforced budget after reset. Env-only; not 0.6.0 wheel cargo. Fork Approve-and-run / exact-head CI still required.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
REQUEST_CHANGES at a8c93ac2 (merge-main only; prior blocker remains)
Rebased onto main after #1178. Feature tip unchanged from eee0504d.
Docker fallback enforcement and finite timeout parsing remain good. Still open: _evaluate_task re-calls _read_timeout(self._task_dir, …) at scoring time, so local evaluation still rereads agent-writable task.toml. The enforced budget can differ from the verifier_timeout_sec advertised in reset metadata after agent mutation.
Cache the reset-time value (or otherwise freeze the contract) before merge, or explicitly document/accept that mutation is in-scope. Env-only; not package cargo.
Sent by Cursor Automation: Release
|
Fixed the reset-time budget finding in 07e56d2. Local and Docker evaluation now reuse the timeout reported by reset; changing task.toml cannot change the current episode budget. The next reset refreshes it, and close clears it. All 12 new reset/mutation regressions fail before this fix and pass after it, covering both scoring paths plus valid, missing, and invalid budgets. Focused suite: 63 passed, 1 optional smoke test skipped; touched-file lint/format checks pass. Docker lifecycle was mocked, not exercised live. |
|
cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 07e56d2. Configure here.
There was a problem hiding this comment.
APPROVE @ 07e56d28
Freeze-verifier-budget fix addresses the residual trust-contract gap: local and Docker paths cache _verifier_timeout_s at reset, observation metadata and evaluation use that value even if the agent mutates task.toml, a later reset refreshes the budget, and close() clears it.
Prior timeout enforcement / _read_timeout hardening remains intact. Local: 63 passed / 1 skipped on tests/envs/test_tbench2_env.py.
Env-only — not 0.6.0 wheel cargo. Fork still needs Approve-and-run for exact-head repository CI before merge.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
Merge-main re-APPROVE at a9c98daf
Ben merged main into this fork tip. Feature diff unchanged: reset advertises budgets; local+Docker freeze _verifier_timeout_s per episode; _read_timeout finite/positive guard still present.
Fork PR: repository CI still needs Approve-and-run. Env-only — not OpenEnv wheel cargo.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
Re-APPROVE at 6b2ef1b4 (merge-main only)
Ben merged main (26c9465e / #1179) into this fork PR. Stable feature patch-id is unchanged vs prior approved tip a9c98daf (f6fe61a5…); tbench2 budget files identical.
Prior technical approval stands. Still needs maintainer Approve and run. Env-only — not 0.6.0 wheel cargo.
Sent by Cursor Automation: Release


Summary
tbench2_env'sresetobservation now reports the time budgets that bound a single env op ininfo, so clients can derive per-message deadlines from the server (budget plus margin) instead of a static guess that has to cover the largest task in the set.verifier_timeout_sec(local and Docker): the task'stask.toml[verifier].timeout_sec, else the 900s fallback. This is the same valueevaluateenforces; the fallback is now one shared constant (_DEFAULT_VERIFIER_TIMEOUT_S) used by bothresetand evaluation, so the reported and enforced budgets cannot drift.command_timeout_s(local only): the per-command budget forexec(TB2_COMMAND_TIMEOUT_S).One deliberate deviation from the literal proposal in #1026: Docker mode does not report
command_timeout_s.Tbench2DockerEnvironmentstorescommand_timeout_sbut never applies it (exec_runhas no server-side timeout), so surfacing it would advertise a budget the server does not enforce and could push clients toward a wrong deadline. Happy to include it if you would rather have the key present in both modes.Purely additive: existing
infokeys (including Docker'sdocker_image) are unchanged.Closes #1026.
Type of Change
Alignment Checklist
Before submitting, verify:
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violated/pre-submit-pr(orbash .claude/hooks/lint.shand tests) and addressed all issuesRFC Status
Environment-only change; nothing under
src/openenv/core/and no public API signature changes.Test Plan
Four new unit tests in
tests/envs/test_tbench2_env.py(no network, no Docker):resetreports both budgets (task.tomltimeout_sec = 3600, constructorcommand_timeout_s=42.0)resetfalls back to the default verifier budget when the task has no[verifier]sectionresetinfo is exactly{"docker_image": ..., "verifier_timeout_sec": ...}, which also guards against advertising the unenforcedcommand_timeout_sresetfalls back to the default verifier budgetResults:
pytest tests/envs/test_tbench2_env.py: 30 passed, 1 skippedruff format --checkandruff checkonsrc/ tests/: clean;usort check: clean apart from the two files already flagged onmain(test_grid_world.py,test_julia_env.py)python scripts/sync_env_docs.py --check: passes (docs stub refreshed from the README)main(the 23 failures present onmainthere are Windows-only, e.g.os.killpg,/bin/bash, CRLF, cp1252, and none are in this area). I am relying on Linux CI as the authoritative run.Claude Code Review
Written with Claude Code. Output of
/alignment-review:Note
Low Risk
Additive
infofields and stricter timeout parsing in the TB2 env only; evaluation behavior tightens Docker verifier bounds but does not change core OpenEnv APIs or auth/data paths.Overview
resetnow exposes execution time budgets inobservation.info, so clients can set per-message deadlines from server-reported limits instead of static guesses.Local mode adds
verifier_timeout_sec(fromtask.toml[verifier].timeout_secor shared 900s default) andcommand_timeout_s(per-execbudget). Docker mode reportsverifier_timeout_secwithdocker_imageonly—command_timeout_sis omitted because containerexechas no server-enforced command timeout.The verifier budget is read once at
reset, stored in_verifier_timeout_s, and reused forevaluateso reported and enforced limits stay aligned even iftask.tomlchanges mid-episode._read_timeoutnow rejects invalid values (non-dict[verifier], NaN/inf, non-positive). Docker scoring wraps canonical and fallback verifiers withtimeout(no optionalcommand -vfallback).Docs (
tbench2.md, env README) describe the newinfokeys; tests cover reset reporting, fallbacks, invalid TOML, frozen budgets, and bounded fallback eval.Reviewed by Cursor Bugbot for commit 5766958. Bugbot is set up for automated code reviews on this repo. Configure here.