Skip to content

feat(tbench2_env): report execution budgets in the reset observation - #1203

Open
KNambiarDJsc wants to merge 13 commits into
huggingface:mainfrom
KNambiarDJsc:feat/tbench2-reset-budgets
Open

KNambiarDJsc wants to merge 13 commits into
huggingface:mainfrom
KNambiarDJsc:feat/tbench2-reset-budgets

Conversation

@KNambiarDJsc

@KNambiarDJsc KNambiarDJsc commented Sep 18, 2026

Copy link
Copy Markdown

Summary

tbench2_env's reset observation now reports the time budgets that bound a single env op in info, 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's task.toml [verifier].timeout_sec, else the 900s fallback. This is the same value evaluate enforces; the fallback is now one shared constant (_DEFAULT_VERIFIER_TIMEOUT_S) used by both reset and evaluation, so the reported and enforced budgets cannot drift.
  • command_timeout_s (local only): the per-command budget for exec (TB2_COMMAND_TIMEOUT_S).

One deliberate deviation from the literal proposal in #1026: Docker mode does not report command_timeout_s. Tbench2DockerEnvironment stores command_timeout_s but never applies it (exec_run has 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 info keys (including Docker's docker_image) are unchanged.

Closes #1026.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • New environment
  • Refactoring

Alignment Checklist

Before submitting, verify:

  • I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
  • I have checked .claude/docs/INVARIANTS.md and no invariants are violated
  • I have run /pre-submit-pr (or bash .claude/hooks/lint.sh and tests) and addressed all issues

RFC Status

  • Not required (bug fix, docs, minor refactoring)
  • RFC exists: #___
  • RFC needed (will create before merge)

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):

  • local reset reports both budgets (task.toml timeout_sec = 3600, constructor command_timeout_s=42.0)
  • local reset falls back to the default verifier budget when the task has no [verifier] section
  • Docker reset info is exactly {"docker_image": ..., "verifier_timeout_sec": ...}, which also guards against advertising the unenforced command_timeout_s
  • Docker reset falls back to the default verifier budget

Results:

  • pytest tests/envs/test_tbench2_env.py: 30 passed, 1 skipped
  • ruff format --check and ruff check on src/ tests/: clean; usort check: clean apart from the two files already flagged on main (test_grid_world.py, test_julia_env.py)
  • python scripts/sync_env_docs.py --check: passes (docs stub refreshed from the README)
  • Full suite run locally on Windows: no new failures compared with an unmodified main (the 23 failures present on main there 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:

## Alignment Review Report

### Automated Checks
- Lint: PASS for the lines changed here. The repo lint hook scans the whole tree
  (including env README code blocks) and reports findings that already exist on main;
  none are in this diff. CI-scoped format/ruff/usort are clean as described above.
- Debug code: CLEAN in this diff (the hook's print matches are pre-existing, in src/).

### Open RFCs Context
RFC 008 (In Review) mentions envs/tbench2_env only as an existing consumer of task.toml.
This change reads the same [verifier].timeout_sec the env already reads; no new schema.

### Tier 1: Fixes Required
None.

### Tier 2: Alignment Discussion
Principle conflicts: None identified. reset/step/state signatures are unchanged, the new
values are JSON-compatible floats in an existing dict, no client/server import changes,
rewards are untouched.
RFC conflicts: None identified.

### Summary
- 0 mechanical issues, 0 alignment points, 0 RFC conflicts
- Reviewer note (not a flag): the key names `verifier_timeout_sec` and `command_timeout_s`
  differ in suffix style. They follow the names proposed in #1026 (matching task.toml's
  `timeout_sec` and the existing `command_timeout_s` attribute).

Note

Low Risk
Additive info fields 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
reset now exposes execution time budgets in observation.info, so clients can set per-message deadlines from server-reported limits instead of static guesses.

Local mode adds verifier_timeout_sec (from task.toml [verifier].timeout_sec or shared 900s default) and command_timeout_s (per-exec budget). Docker mode reports verifier_timeout_sec with docker_image only—command_timeout_s is omitted because container exec has no server-enforced command timeout.

The verifier budget is read once at reset, stored in _verifier_timeout_s, and reused for evaluate so reported and enforced limits stay aligned even if task.toml changes mid-episode. _read_timeout now rejects invalid values (non-dict [verifier], NaN/inf, non-positive). Docker scoring wraps canonical and fallback verifiers with timeout (no optional command -v fallback).

Docs (tbench2.md, env README) describe the new info keys; 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.

…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 cursor Bot mentioned this pull request Sep 19, 2026
21 tasks
@burtenshaw

Copy link
Copy Markdown
Collaborator

cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Docker fallback ignores the advertised budget — see inline on the Docker reset report.
  2. _read_timeout float 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.

Open in Web View Automation 

Sent by Cursor Automation: Release

Comment thread envs/tbench2_env/server/tbench2_env_environment.py Outdated
Comment thread envs/tbench2_env/server/tbench2_env_environment.py Outdated
@burtenshaw

Copy link
Copy Markdown
Collaborator

cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at eee0504d

Prior Tier-1 blockers are fixed:

  1. Docker fallback budget_fallback_eval_cmd(..., timeout_s=...) now wraps the verifier in timeout; missing timeout binary fails closed (exit 127) instead of running unbounded.
  2. _read_timeout safety — 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.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Release

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Release

@burtenshaw

Copy link
Copy Markdown
Collaborator

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.

@burtenshaw

Copy link
Copy Markdown
Collaborator

cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Release

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at e9b63027 (merge-main only)

Rebased onto main after #1219. Feature unchanged: reset-time verifier budget freeze + timeout enforcement remain.

Env-only; not wheel cargo. Fork may still need Approve-and-run.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Release

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Release

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Release

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tbench2_env: expose the task's execution budgets to the client

2 participants