feat: make Legal Agent Bench agent harness and sandbox-backends configurable - #2172
feat: make Legal Agent Bench agent harness and sandbox-backends configurable#2172rystewart-nvidia wants to merge 27 commits into
Conversation
|
This looks good to me, thanks for opening @rystewart-nvidia! Do we have any quick validations on running the different harnesses against any model for verification? Otherwise, I think these changes are good. |
|
Hi @rystewart-nvidia, what are your plans to move the PR from draft, so we can assign reviewers? |
|
@roclark yes, I've run validations across all 4, will add some example results to the PR description shortly @ritaneves moved to ready for review |
There was a problem hiding this comment.
Very elegant implementation, I left a couple of comments, mostly related to possible race-conditions.
Some additional recommendations:
- testing on other sandbox env than docker to check stability, eg. opensandbox, ECS
- I'm not sure if harbor should be the default - usually we aim at one of the native configs for this; but I don't have enough knowledge about this benchmark to fully recommend it, if you feel harbor should be default then please keep it
| f"{task_dir / 'documents'}:/workspace/vdr:ro", | ||
| f"{skills_dir}:/workspace/skills:ro", | ||
| f"{paths['runtime']}:/trajectories_mount", | ||
| f"{paths['agent']}:/logs/agent", |
There was a problem hiding this comment.
Gym mounts /logs/agent writable, so the agent can make stdout.log a symlink to another host file. Gym later follows that link when writing the log and may overwrite the target as the host user. Please keep host-written files outside agent mounts, write them only after sandbox teardown, and add a symlink regression test.
There was a problem hiding this comment.
I updated it to remove writable host mounts and to write the host-side artifacts after sandbox teardown. Also added the requested symlink regression test. Addressed as part of this commit - 5942214#diff-8f3dbaaa67a8f1bae7d997d01053d82cb9a87a594832a35b17a3d38766af14cb
| resources=_sandbox_resources(task_dir), | ||
| provider_options={ | ||
| "volumes": [ | ||
| f"{paths['lab_run']}:/logs/agent/artifacts/lab-run:ro", |
There was a problem hiding this comment.
Addressed in commit linked in comment above
| "--user", | ||
| f"{os.getuid()}:{os.getgid()}", | ||
| "--volume", | ||
| f"{PARENT_DIR}:/nemo_gym_mount:ro", |
There was a problem hiding this comment.
Runtime provisioning mounts the entire repository into a networked container that runs pip and npm installers. That mount can include gitignored env.yaml with judge credentials. Please stage and mount only the files required for installation, and test that credential-bearing paths are unavailable.
There was a problem hiding this comment.
Addressed in this commit - 168bf83 - now mounts only the files needed for installation, and added tests to assert that credential files are not mounted
| return "\n".join(parts).strip() | ||
|
|
||
|
|
||
| def agent_response_failure(response: NeMoGymResponse, agent_server_module: str) -> Optional[str]: |
There was a problem hiding this comment.
This check uses trajectory content to infer success, but the selected harness can return partial output while dropping its failure state. The failed run can then reach the verifier and be scored. Please propagate exit, timeout, and adapter errors separately, set mask_sample=true, skip verification, and test partial output followed by failure for each harness.
There was a problem hiding this comment.
Handled here - 486c6ea - exit, timeout, and adapter errors are propogated separately, mask_sample is set and verification is skipped on agent failures, and I added tests for each harness to confirm expected behavior after partial output + failure
There was a problem hiding this comment.
Added similar failure handling for Harbor here - c687a5d
| return deps_dir | ||
|
|
||
| if deps_dir.exists(): | ||
| shutil.rmtree(deps_dir) |
There was a problem hiding this comment.
The lock protects only one server process, but all processes share .deps/<harness>. Two evaluations can delete or rewrite the same runtime while another is building or using it.
There was a problem hiding this comment.
Addressed in 168bf83 - runtime deps now built under os-level cross process lock and published in immutable temporary dir unique to each build
| ## Run a larger evaluation | ||
|
|
||
| Remove `--limit 1` from the desired smoke command. Choose a new output filename | ||
| and increase `--concurrency` only after confirming that Docker, the policy |
There was a problem hiding this comment.
The README recommends raising --concurrency, but every configurable agent keeps its server-side concurrency at 1, so the requests remain serial. Please document the matching server override or make both settings use one limit, and verify that two requests can overlap.
| ) | ||
| await sandbox.start() | ||
| started = time.time() | ||
| agent_result = await sandbox.exec( |
There was a problem hiding this comment.
The agent runs as container root, so on native Linux it can leave root-owned files in the host-mounted output directory. Please run the agent with the host UID/GID or repair ownership before teardown, and add a Linux ownership test.
There was a problem hiding this comment.
Addressed in 5942214 - agent now runs as nobody, files are pulled from the sandbox through the sandbox api, and added tests to verify that files are owned by the UID/GID that invoked the process.
99931fa to
cb79b55
Compare
|
Two blocking concerns before merge:
mask_sample is a training hint, not a routing signal. Without _ng_failure_class, connection errors, sandbox crashes, and verifier failures land in the main rollout JSONL as completed zero-reward rows — they don't route to the failure sidecar and won't retry.
config.yaml now points the unqualified legal_agent_bench at the native loop, but the PR only includes 3-task smoke results per harness. Please either:
|
|
@Glorf Addressed _ng_failure_class tagging and sidecar routing, and unmasked the max-turn and context limit stoppages so that they will be scored. The added native harness is directly aligned with Harvey official benchmark repo, so that's the sensible default. I've attached aggregate metrics from a full run across 1,749 tasks using Nemotron Ultra as policy and GPT 5.5 as judge. Full-task pass rate and criteria pass rate are in line with published metrics for Nemotron Ultra on Artificial Analysis and Vals. Full-run validation results (Nemotron 3 Ultra): Also added some README updates to clarify recommendations on max output token and timeout settings, as there are no official/canonical values. legal_agent_bench_native_nemotron_3_ultra_gpt_5_5_medium_judge_full_v4_aggregate_metrics.json |
|
/claude review |
|
SHIP WITH CARE — no BLOCKER found. Large, thoroughly-tested addition (native LAB loop + configurable Hermes/Claude Code/Codex/Harbor harnesses, sandbox isolation, failure routing). I read the full diff and verified the failure-classification and scoring paths against the head files. What I checked and found solid:
Two low-severity notes left inline (scoring Operability caveat for reviewers: CLI harness pins ( |
|
|
||
| SKIP_DIRS = {"node_modules", ".npm", "__pycache__", ".git", "venv", ".venv"} | ||
| SKIP_EXTENSIONS = {".lock", ".map"} | ||
| SKIP_EXTENSIONS = {".lock", ".map", ".rels", ".xml"} |
There was a problem hiding this comment.
RISK — scoring change: adding .xml/.rels to SKIP_EXTENSIONS globally excludes those files from _load_all_output, which feeds the judge for every criterion that specifies no deliverables.
WHAT BREAKS: for a deliverable-less criterion whose intended output is legitimately an .xml file, that content is now silently dropped from the judge prompt, lowering the score. Deliverable-based criteria are unaffected (_match_deliverables/direct reads bypass SKIP_EXTENSIONS), so the blast radius is narrow: deliverable-less criteria + genuine .xml/.rels output across the 1,749-task set.
The intent (drop raw OOXML working files from an unzipped docx in the workdir — confirmed by test_full_output_ignores_raw_ooxml_working_files) is reasonable, and for docx/pptx/xlsx/pdf legal deliverables the collision is unlikely. FIX: confirm no LAB task's deliverable-less criterion expects .xml/.rels full-output content; if any do, scope the skip to the workdir/ OOXML tree rather than by extension globally.
| agent_metadata.get("agent_failed", False) or failed_during_agent_phase or agent_timed_out | ||
| ) | ||
| model_connection_failed = bool(agent_metadata.get("model_connection_failed", False)) | ||
| verifier_failed = bool( |
There was a problem hiding this comment.
NOTE — verifier_failed fires when verifier_result is None and the agent didn't fail and no context-limit. But a genuine zero-reward run where the verifier legitimately produced no result object (vs. produced reward=0) is indistinguishable here. Verified against the test matrix this matches intended semantics (a missing verifier_result with a started/completed agent = infra failure → mask). Flagging only so it's a conscious contract: any future task type whose verifier can legitimately emit no verifier_result would be masked as verifier_failed and excluded from scoring. No change needed if that can't happen for LAB.
|
Validation run results with criteria pass rates for each combination of harness and sandbox-provider across five tasks
|
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
…ne to address changes introduced in main Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
385c752 to
c7c19af
Compare
Signed-off-by: Ryan Stewart <rystewart@nvidia.com>
Summary
This PR makes the Legal Agent Bench (LAB) agent harness configurable. Currently, LAB can only be run using the Harbor harness.
This adds three additional variants (in addition to Harbor) that enables running the same 1,749 tasks, skills, verifier, and reward logic with Gym's Hermes, Claude Code, and Codex harnesses:
legal_agent_bench/config_hermeslegal_agent_bench/config_claude_codelegal_agent_bench/config_codexBuilds on PRs #1976 and #2075. Related to issue #1974.
Design
Validation
Harness validation
Ran three LAB tasks for each harness using
nemotron-3-ultra-nvfp4as policy and judge model.full_taskrewardAll 12 rollouts produced nonzero token usage, completed output files, agent
trajectories, and verifier artifacts. The 0.0
full_taskrewards are expected as a full task pass requires 100% criteria pass rate