Skip to content

fix(tbench2_env): treat a nan or inf reward.txt as no verdict - #1570

Merged
burtenshaw merged 3 commits into
openenv:mainfrom
zuraiz-anjum:fix-tbench2-non-finite-reward
Nov 9, 2026
Merged

burtenshaw merged 3 commits into
openenv:mainfrom
zuraiz-anjum:fix-tbench2-non-finite-reward

Conversation

@zuraiz-anjum

@zuraiz-anjum zuraiz-anjum commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

_parse_canonical_reward in tbench2_env returns None for an empty or non-numeric reward.txt, and the callers raise so the episode is reported as a scoring error. But float() also accepts nan, inf and -inf, so a reward.txt holding one of those went through as the reward. This makes a value that is not finite count as no verdict too.

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)

Details

Since #1025, a missing verdict raises in _require_canonical_verdict, so RL consumers can drop the episode instead of training on a false 0.0. On main, __TB2_REWARD__:nan is parsed as float("nan"), _evaluate_task and _evaluate_docker return reward=nan with tests_passed: False, and the NaN goes to the trainer. inf goes through the same way.

The other envs that read a reward file already reject this case: _parse_reward_override in coding_tools_env, jupyter_env and terminus_env returns no reward when math.isfinite(value) is false. This applies the same check in _parse_canonical_reward, so both the local and the Docker path raise "canonical harness produced no verdict", the same as for an empty reward.txt. I also updated the docstring.

Test Plan

Two tests in tests/envs/test_tbench2_env.py, next to the missing-verdict tests from #1025:

  • test_evaluate_non_finite_verdict_raises, for nan, inf and -inf on the local path. It also checks that the staged tests and the verifier dir are still wiped.
  • test_evaluate_docker_non_finite_verdict_raises for nan on the Docker path, with the fake container.

Run with PYTHONPATH=src:envs pytest tests/envs/test_tbench2_env.py:

  • main with only the new tests: the 4 new cases fail (DID NOT RAISE), the existing verdict tests pass.
  • this branch: the 4 cases pass and nothing else changes.

The three checks from .claude/hooks/lint.sh (usort check, ruff format --check, ruff check) are clean on the files this PR touches; the findings a tree-wide run reports are in other files and are the same on main.


Note

Low Risk
Scoring-only guard on reward parsing; finite 0.0/1.0 behavior unchanged and covered by new tests.

Overview
TB2 canonical scoring used to treat reward.txt values like nan, inf, and -inf as real rewards because float() accepts them. _parse_canonical_reward now returns no verdict when the parsed value is not finite (math.isfinite), matching how other envs handle reward overrides.

When that happens, local and Docker evaluation still fail the same way as a missing or empty verdict: _require_canonical_verdict raises with an updated message (“missing or not a finite number”) so trainers get a scoring error instead of propagating NaN/inf into RL loops.

Tests cover nan/inf/-inf on the local path and nan in Docker mode.

Reviewed by Cursor Bugbot for commit 67ce3c3. Bugbot is set up for automated code reviews on this repo. Configure here.

_parse_canonical_reward returns None for an empty or non-numeric
reward.txt, and the callers raise so the episode is reported as a
scoring error instead of a reward. float() also accepts "nan", "inf"
and "-inf", so those values went through as the reward, and a NaN or
infinite reward reached the trainer.

Return None for a value that is not finite, the same rule the
coding_tools, jupyter and terminus envs already apply to their reward
files. Tests cover the local and the Docker evaluate paths.
@cursor cursor Bot mentioned this pull request Nov 9, 2026
2 tasks done

@burtenshaw burtenshaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is ready to merge. A nan or inf in reward.txt now takes the same "no verdict" path as an empty or non-numeric value.

Optional

  • envs/tbench2_env/server/tbench2_env_environment.py:244: the error still says "reward.txt missing after tests/test.sh ran", which reads wrong when reward.txt holds nan. Main already says this for non-numeric values, so it is not new. The log tail in the message also shows the marker line. If you want to touch it, "reward.txt missing or not a finite number" covers every case, and the tests match on "produced no verdict", so they still pass.

What I checked

  • On main, _parse_canonical_reward returns nan, inf and -inf as rewards. An evaluate step with nan in reward.txt returns reward=nan, done=True, success=True and no error. Over the WebSocket, pydantic writes the NaN as "reward": null next to "done": true. A remote trainer sees a finished episode with no reward and no error, and an in-process caller gets the NaN itself. On this branch the same step returns success=False, done=False, reward=None and the "produced no verdict" error, like the existing missing-verdict case.
  • math is already imported in the module. The same math.isfinite check already exists in _parse_reward_override in coding_tools_env, jupyter_env and terminus_env.
  • PYTHONPATH=src:envs pytest tests/envs/test_tbench2_env.py -q: 67 passed, 1 skipped on this branch. The skip is the opt-in smoke test (TB2_ENABLE_TESTS not set). Main with your test file: the 4 new cases fail with DID NOT RAISE, and the rest pass.
  • The branch merges cleanly onto current main, and the same tests pass there.
  • ruff format --check and ruff check are clean on both changed files. usort check reports the same finding in the env module on main.
Reproduction

Run from the repo root with PYTHONPATH=src:envs python repro.py. It points the verifier paths at a temp dir.

import tempfile
from pathlib import Path

from openenv.core.env_server.serialization import serialize_observation
from openenv.core.env_server.types import WSObservationResponse
from tbench2_env.models import Tbench2Action
from tbench2_env.server import tbench2_env_environment as mod

for raw in ["1", "", "abc", "nan", "inf", "-inf"]:
    print(f"{raw!r}: {mod._parse_canonical_reward(f'__TB2_REWARD__:{raw}')!r}")


class Toolkit:
    def shell_exec(self, **kwargs):
        return "__TB2_REWARD__:nan"


root = Path(tempfile.mkdtemp())
mod._VERIFY_TESTS_DIR = str(root / "tests")
mod._VERIFIER_LOG_DIR = str(root / "logs")
(root / "task" / "tests").mkdir(parents=True)
(root / "task" / "tests" / "test.sh").write_text("")
env = mod.Tbench2Environment(withhold_tests=False)
env._task_dir, env._workdir, env._terminal_toolkit = root / "task", str(root), Toolkit()
obs = env.step(Tbench2Action(action_type="evaluate"))
print(obs.reward, obs.done, obs.success, repr(obs.error[:44]))
print(WSObservationResponse(data=serialize_observation(obs)).model_dump_json()[-30:])
main:
'1': 1.0
'': None
'abc': None
'nan': nan
'inf': inf
'-inf': -inf
nan True True ''
}},"reward":null,"done":true}}

this branch:
'1': 1.0
'': None
'abc': None
'nan': None
'inf': None
'-inf': None
None False False 'canonical harness produced no verdict (rewar'
},"reward":null,"done":false}}

@zuraiz-anjum

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review. I took the wording: b55de2c changes the message to "reward.txt missing or not a finite number". The tests still pass (67 passed, 1 skipped), and ruff is clean.

@burtenshaw
burtenshaw merged commit 435d5be into openenv:main Nov 9, 2026
14 checks passed
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.

2 participants