Repository navigation
Fix the high findings from the e2e test review - #47
Merged
Merged
Conversation
In the default mode (one step, verifier in the agent's container) /logs/verifier is world-writable and was never emptied before grading, and reward.json outranks the reward.txt test.sh writes. So a reward.json the agent left there decided the trial, with no trace in result.json. /tests merged rather than replaced the same way. Multi-step shared grading already cleared both; the single-step path now does too.
Sources matched on result.task_name, which is [task].name when set (hbench/hello-world), while targets used the lock's name, which is always the directory (hello-world). They never agreed for a named task, so --diff reran every example and every hbench-authored task instead of reusing trials within an epoch. Both sides now use Task::name(). Adds the first test of diff::compare.
- docker cp keeps container symlinks with absolute targets, and special files. hbench's own host code then followed them: trajectory.json writes overwrote the link target, filter_logs deleted outside the trial dir, a reward.json linked to /dev/zero read until out of memory, and the viewer served linked host files. Every download now removes symlinks and non-regular files, and logs how many. - The egress sidecar image was inspected once per process (OnceCell), so an image pruned mid-run failed that trial and every later restricted one with "pull access denied". It is now checked before each up, built under a lock if missing, never pulled (pull_policy: never), and rebuilt and retried if up still finds it gone. - compose build had no retry. It now shares up's retry, which also covers BuildKit losing its builder or connection and registry 500/504, and gives up at once on a full disk.
…dirs - New fixture planted-reward: the agent leaves a reward.json and a /tests file; it scores 1 only if both are gone when test.sh runs. - The egress probe counted any status but 403 as reachable, and gost answers 503 when it can't reach an allowed host, so a dead model stand-in passed. Only 2xx, 3xx and 404 count now. The stand-in must answer before the egress runs start, and serves an empty dir instead of the job dirs. - E2E_JOBS_DIR keeps the job dirs; CI uploads them when the suite fails or is cancelled, under a step timeout that leaves room for the upload. The fixtures checker reports every mismatch with each failed trial's exception, trial.log tail and verifier output.
Merged
Contributor
|
@greptileai review |
1 similar comment
Contributor
|
@greptileai review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes six of the ten high findings from the 2026-10-23 e2e test review (written at cdd8195, re-checked against master).
Changes
H1: planted reward.json outranks the verifier (single-step shared mode).
/logs/verifieris world-writable and was never emptied before grading.reward.jsontakes precedence overreward.txt, so a file the agent left there decided the trial. The single-step path now clears/logs/verifierand/testsbefore verifying, as multi-step already did. A new fixture,planted-reward, covers it.H3: agent symlinks reach the host.
docker cpkeeps symlinks with absolute targets, and hbench's own host code then followed them: thetrajectory.jsonwrite,filter_logs,parse_rewards(a link to/dev/zeroreads until the host runs out of memory) and the viewer routes.download_dir,download_dir_excludinganddownload_filenow remove symlinks and non-regular files after copying, and log how many.H5:
run --diffnever reused named tasks. Sources were matched on[task].name, targets on the directory name. Both now useTask::name(). This adds the firstdiff::comparetest.H8: pruned egress sidecar image. The image is checked before every
upinstead of once per process, getspull_policy: never, and is rebuilt and retried ifupstill reports it missing.H9:
compose buildhad no retry. It now sharesup's retry loop. The loop also treats BuildKit builder or connection loss and registry 500/504 as transient, and gives up at once on a full disk.H2: egress probe false pass. gost answers 503 when it can't reach an allowed host, and the probe counted any status except 403 as reachable. Only 2xx, 3xx and 404 count now. The model stand-in must answer before the egress runs start, and it serves an empty dir instead of the job dirs.
H7: a red E2E run was hard to diagnose. Adds
E2E_JOBS_DIR. On a failed or cancelled run, CI uploads the job dirs; a step-level timeout leaves time for that upload. The fixtures checker reports every mismatch, with the exception, thetrial.logtail and the verifier output.Not in this PR
release.ymlalready gates publishing on CI (Release faster: one job per binary, CI on the tagged commit gates publishing #45).Notes
/testsbefore single-step grading assumes no prebuilt image ships content there. Multi-step shared grading already makes the same assumption.docker buildstill has no retry.Testing
cargo test(359 passed, including new tests forstrip_links,diff::compareand the build-retry patterns),cargo clippy --all-targets -D warnings,cargo fmt --check.