fix(evals): use patched Pier egress proxy via local submodule - #1576
Merged
Conversation
Assistant-model: GPT-5.5
claude
Bot
changed the title
PR Review:
|
flora131
added a commit
that referenced
this pull request
Aug 16, 2026
Follows the shape of #1576: the enforcement lives in the pier fork, and the atomic side carries the pin, the lock, and a doc note. Deleted: - run_manifest.py. With it go both setup() version-probe overrides, _observed_model, and _selected_model - every one of them existed only to fill a manifest field. Provenance is the job's own config.json plus `git rev-parse HEAD:evals/vendor/pier`. - The preflight's corpus, Docker, and credential checks. The corpus is pinned by SHA, so counting its tasks on every run re-verifies what the pin already guarantees; Docker and credentials announce themselves within seconds. prerequisites.py keeps the install helpers, the shared provider map, and verify_submodules(), which proves the fork's guarantees are the code running. - evals/DEV_SETUP.md. The root DEV_SETUP.md already documented submodule init, uv sync, the editable-install refresh, and a single-task run; the genuinely new notes are folded in there instead. Its example pinned version=next, which is the trap this branch already hit: an Atomic older than 0.9.11 reads the prompt terminator as a flag and starts with no task. Now pinned. evals/ is net negative against main: +510/-684.
lavaman131
pushed a commit
that referenced
this pull request
Aug 16, 2026
…work_mode enforcement, and harness preflight (#2414) * chore(evals): repoint pier submodule at bastani-inc fork and fast-forward to v0.3.1+forbid S1: the vendored pier submodule pointed at lavaman131/pier, a personal-account fork holding zero unique commits and sitting 17 commits behind its parent. Repoint it at bastani-inc/pier, an org-owned fork of datacurve-ai/pier. S2: fast-forward the pin from fefa7475 to upstream v0.3.1 (df89f994), which contains PR #29 ([[verifier.collect]]) and PR #31 (network_mode). evals/uv.lock records datacurve-pier 0.3.1. S3: pin 90e24d6 on bastani-inc/pier's atomic/v0.3.1-extra-forbid branch, which adds ConfigDict(extra="forbid") to the task-config models so an unknown key raises a ValidationError naming it instead of being silently dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(evals): enforce network, corpus, artifact, and manifest contracts S4: network_policy.py raises a named EmptyEgressAllowlistError instead of returning an empty NetworkAllowlist. An empty allowlist under restricted egress drops pier's filtered-egress proxy overlay, leaving the sandbox with no route to any provider, which surfaced as a generic connection error that read like bad credentials. Unit tests pin no-network -> allow_internet=False for the [agent] and [verifier] scopes. S5: prerequisites.py gains a preflight that parses every task.toml with stdlib tomllib (so it survives an uninitialized vendor/pier), asserts the task count, one [[verifier.collect]] hook per task, and zero compose files, and checks Docker and credentials through injectable runners. It skips with an explicit "run git submodule update --init --recursive" message when evals/deep-swe is uninitialized, so a fresh clone does not fail the suite. Submodule SHAs are read from the superproject gitlink, never `git -C <sub> rev-parse HEAD`, which prints the superproject SHA when the submodule is empty. S6: trial_audit.py adds the artifact contract. The adapters now record an explicit status (agent/atomic-status.json plus context.metadata) for a missing or empty atomic.txt and for malformed session JSONL, instead of returning early or skipping the line. model.patch cannot be checked from the adapter -- pier runs populate_context_post_run before the collect hooks -- so it is audited after the run: a trial with no, or an empty, artifacts/model.patch is a failure, not a completed trial. S7: run_manifest.py records run ID, seed, model, Atomic version, deep-swe SHA, and Pier SHA next to the results, and compare_manifests raises ManifestMismatchError naming every field two runs disagree on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(evals): correct two contract defects a live Deep SWE trial exposed A real one-task pier run against the new pin surfaced both: 1. Every healthy trial was flagged `malformed-session-jsonl`. Atomic writes human-readable diagnostics into the same stream as its JSON events (one banner in a 3.3 MB atomic.txt), and any unparsable line was counted as corruption. Only a line that opens a JSON value and does not finish it is a truncated record now; plain-text lines stay tolerated, like blank lines. 2. Every manifest recorded `seed: null`. Pier writes `sample_seed` on the dataset entry (`datasets[].sample_seed`), not at the top level of the job config. Read both, preferring the top-level key when present. Both are covered by regression tests using the exact shapes observed live, and re-running the adapter over the real trial directory now yields `status: ok` and `seed: 0`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(evals): close the review's harness findings (F-B..F-J) F-B/F-C: compare_manifests now refuses an absent or incomplete manifest before diffing. Two manifests full of nulls previously compared as identical, and the comparison command documented in evals/README.md raised AttributeError when a directory had no manifest. Both now raise IncompleteManifestError, a subclass of ManifestMismatchError, naming the side and every missing field. Harbor records no seed at all, so a Harbor manifest refuses comparison naming `seed`. F-D: the Harbor adapter writes the run manifest its README already promised, before the missing-atomic.txt early return, exactly like the Pier twin. F-E: session .jsonl parsing is strict again. The first-byte heuristic exists only because Atomic interleaves plain-text diagnostics into atomic.txt; a session transcript is machine-written, so any undecodable line there is corruption. _read_jsonl takes strict/count_malformed flags so atomic.txt keeps the tolerance and each file is tallied exactly once. F-F: an initialized but empty corpus now fails instead of skipping. The skip is decided by initialization state, not by finding zero tasks. F-G: collect hooks are counted per hook, not per task, and the total is asserted, so two tasks with two hooks each no longer reads as a 2-hook corpus. F-H: the preflight credential set is the adapter's full provider map, moved into prerequisites.py as the single source of truth (the import direction stays one-way), and auth.json content is validated rather than its existence — `{}` no longer counts as a credential. F-I: submodule checks verify the checked-out SHA against the gitlink. A file named .git no longer reads as an initialized submodule, the probe cannot walk up to the superproject, and drift fails naming both SHAs. F-J: the unknown-task-config-key negative now lives in the evals suite, which `uv run pytest` actually collects, covering top-level, [environment], [agent], and [[verifier.collect]] keys. evals suite: 91 -> 135 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(evals): re-pin pier so a missing model.patch errors the trial (F-A) Advance evals/vendor/pier from 90e24d6 to adc23b3 on bastani-inc/pier@atomic/v0.3.1-extra-forbid. Those commits make an artifact that never arrived — or a model.patch that arrived empty — a real trial failure instead of a completed trial with a silent manifest entry. This closes the review finding that the artifact contract lived only in evals/trial_audit.py, which nothing in the run path called: pier reported n_completed_trials: 1, n_errored_trials: 0 for a trial with no model.patch. The audit helpers stay as the host-side reader; the enforcement is now where the trial actually runs. Emptiness is fatal for model.patch alone. Any other declared artifact that arrives empty is still recorded as "empty" in the artifacts manifest and left informational, because a task may legitimately declare a log a given run leaves empty; erroring the trial for that would invent a rule no task asked for. A download that failed stays fatal whatever it was fetching. The pin still contains PR #29 (0daf53d3), PR #31 (1b38ae9a), and v0.3.1 (df89f994), verified with git merge-base --is-ancestor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(evals): advance the deep-swe corpus to 435ee89e S8: with S2-S7 green, move the corpus pin from e016041a to 435ee89e, which replaces the 113 deleted pre_artifacts.sh scripts with one [[verifier.collect]] hook per task writing /logs/artifacts/model.patch. Pier v0.3.1 models that hook, so it now executes instead of being silently discarded. Lock the new shape in the suite: the corpus preflight and the corpus-wide network-mode assertion now run against the real tasks (113 tasks, 113 collect hooks, 0 compose files, every task resolving allow_internet=False for the [environment] and [verifier] scopes), and skip with a clear message when the submodule is uninitialized. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(evals): document the fresh-clone path, submodules, preflight, and Harbor S9: evals/README.md began at `uv run pytest` and jumped to `uv run pier run`, so none of it ran in a fresh clone. Add the setup steps that only lived in DEV_SETUP.md (submodule init, uv sync, the pier import check, pier --help, and the reinstall after a pointer change), plus `git submodule sync --recursive`, which an existing clone needs now that evals/vendor/pier moved remote. Also document what the preceding slices added: the submodule table and the gitlink read that does not lie about an uninitialized submodule, the preflight command and its skip semantics, the empty-allowlist error, atomic-status.json / model.patch / atomic-manifest.json, and the Harbor run command the README omitted (harbor takes -a, and has no --sample-seed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(evals): describe the enforced artifact contract and the stricter preflight Match the README to what the code now does: model.patch enforcement lives in the run path (MissingArtifactError -> n_errored_trials), the preflight counts hooks rather than tasks and checks submodule drift, credentials mean a valid auth entry rather than a file that exists, both adapters write the manifest, and compare_manifests refuses an absent or incomplete manifest. Records why a Harbor manifest's seed is null and what pier_sha means there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(evals): address code quality review Assistant-model: GPT-5.6 Sol * fix(evals): close the review findings and slim the README Harness fixes, each from a reviewer repro: - preflight rejects a dirty submodule worktree, not just a drifted HEAD. A tracked edit inside evals/vendor/pier left HEAD equal to the pin, so the check passed and the manifest recorded the clean SHA. The manifest now appends -dirty so such a run cannot compare equal to a clean one. - neither adapter records a moving --version it failed to resolve. Recording 'next' let two different builds compare as equal; the manifest now records nothing and the status carries unresolved-atomic-version. - Harbor resolves the installed version after setup, which only Pier did. - Harbor decodes the session header with replacement. A first line truncated mid-UTF-8 raised UnicodeDecodeError - neither OSError nor JSONDecodeError - and killed the run before any status was written. - Harbor records the candidate the session launched on, so a cancelled or metadata-light fallback run no longer records a model that never ran. - Harbor builds its provider maps from PROVIDER_AUTH_ENV_KEYS. It omitted Kimi, Moonshot and ZAI, which made the README's claim that provider setup works the same on both paths false for exactly those three. - audit_job discovers a trial by pier's root-level markers as well as its agent/artifacts directories. A trial that failed during environment setup has a result and neither directory, so a job with one healthy trial and one dead one reported success. Pier re-pinned to f1bcf9c: only model.patch is fatal again. Erroring a trial because an unrelated declared log failed to download widened pier beyond this task and would reject runs other datasets permit. The documented version moves to 0.9.13. A one-task live run at the previously documented 0.9.5 spent the whole trial printing 'Error: Unknown option: --': the adapters pass the task after the end-of-options terminator, which that build read as a flag, so the agent never saw the task and collected an empty patch. The adapters target the current CLI and carry no compatibility path. Docs: evals/README.md keeps only what a consumer needs to run the benchmark (157 lines, was 419); the pinning, contract, manifest, and Harbor material moves to evals/DEV_SETUP.md. Removes evals/tests entirely, and pytest/basedpyright with it. The harness is verified by running it; the pier fork keeps upstream's suite. * chore(evals): track the pier fork's main instead of a side branch The Atomic pier commits lived on atomic/v0.3.1-*, so the gitlink pointed at a branch that existed only to hold them, and upstream work merged into the fork's main never reached the pin. Rebases the five Atomic commits onto bastani-inc/pier main (which had taken two upstream Modal fixes), pushes them there, and re-pins the submodule at that main. Pier's suite: 189 passed. Both side branches are deleted; every commit they held is reachable from main. .gitmodules now names branch = main, so `git submodule update --remote` fetches the right branch. The pin is still the gitlink - --remote moves the working tree off it, which the preflight reports as drift. * chore(evals): relock for the pier pin's modal>=1.5.3 requirement The upstream Modal commits the fork's main took raise pier's modal floor from 1.4.2 to 1.5.3. Relocked so evals/uv.lock agrees with the pinned pier's pyproject. * fix(evals): keep critique metadata out of trial discovery Greptile P1, reproduced: a job with pier critique output flipped from ok to failed. `.critiques/{run}/{trial}/` creates its own agent/ and artifacts/ directories, the recursive search found them, and the critique run was audited as a benchmark trial that owns no model.patch. The metadata exclusion existed but only guarded the marker-based search over direct children, which never sees those paths anyway. It now applies to the recursive search too, tested against the whole relative path rather than the final component. Verified: healthy job stays ok with critique metadata present; a setup-failure trial is still discovered and still fails the job; a multi-step trial is still audited at its steps rather than twice. * refactor(evals): drop the trial-audit, manifest-compare, and network-policy layers Three host-side layers re-derived or wrapped things that already existed. trial_audit.py (322 lines) is deleted. Its discovery half re-derived pier's own trial list from the filesystem and produced two P1 review findings doing it - first missing setup-failure trials, then auditing critique metadata as a trial. Its status half wrote a verdict nobody read: the one dead agent seen live was reported `ok`, while the thing that actually caught it was the empty model.patch, which the pinned pier already errors the trial for. A dead agent changes nothing, so the collect hook writes a zero-byte diff either way. run_manifest.py keeps recording and loses comparison (342 -> 214 lines). compare_manifests and friends had no caller; `diff <(jq -S . a) <(jq -S . b)` is the same check without the API. network_policy.py is deleted. require_non_empty_allowlist could never fire: the allowlist is seeded with the union of all 15 providers' domains, a constant 21 entries, and only grows. The one real case - a --model with no provider prefix - is raised a line earlier and is now a plain ValueError at that guard. The adapters keep what runs: the manifest write, and the egress guard. * refactor(evals): keep only what the fork cannot do from inside a trial Follows the shape of #1576: the enforcement lives in the pier fork, and the atomic side carries the pin, the lock, and a doc note. Deleted: - run_manifest.py. With it go both setup() version-probe overrides, _observed_model, and _selected_model - every one of them existed only to fill a manifest field. Provenance is the job's own config.json plus `git rev-parse HEAD:evals/vendor/pier`. - The preflight's corpus, Docker, and credential checks. The corpus is pinned by SHA, so counting its tasks on every run re-verifies what the pin already guarantees; Docker and credentials announce themselves within seconds. prerequisites.py keeps the install helpers, the shared provider map, and verify_submodules(), which proves the fork's guarantees are the code running. - evals/DEV_SETUP.md. The root DEV_SETUP.md already documented submodule init, uv sync, the editable-install refresh, and a single-task run; the genuinely new notes are folded in there instead. Its example pinned version=next, which is the trap this branch already hit: an Atomic older than 0.9.11 reads the prompt terminator as a flag and starts with no task. Now pinned. evals/ is net negative against main: +510/-684. * refactor(evals): drop the submodule checker; git already answers it verify_submodules() was Python wrapping `git submodule status`. Git reports drift with a leading +, an uninitialized submodule with -, and local edits inside one through `git status`. The docs now say that. prerequisites.py is back to what the adapters actually import: the sandbox install commands, the shared provider credential map, and the auth-entry check. 184 lines, from 749. Also re-pins pier at 2f09d17, which declares extra="forbid" once on a StrictTaskModel base instead of pasting the same model_config into thirteen classes - three of which inherited it already. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Summary
Wires the eval harness to a local editable Pier submodule containing the Docker egress proxy hardening needed for Atomic/DeepSWE workflow evals, replacing the upstream git dependency with an editable path install.
The Pier fix is tracked upstream in:
Changes
evals/vendor/pieras a submodule pointing at thelavaman131/pierfork (fix/docker-egress-proxy-resiliencecommit) and registers it in.gitmodules.evals/pyproject.tomlandevals/uv.locksodatacurve-pierresolves from the local editablevendor/piercheckout instead of the upstream git source.DEV_SETUP.mdwith:uvand Docker prerequisites for the eval harnessgit clone --recurse-submodulesguidance and manualgit submodule update --init --recursivefallbackuv syncsetup steps and how to refresh the editable Pier install after pulling submodule changesuv run pier runValidation
cd evals && uv run python -c 'import pier, pathlib; print(pathlib.Path(pier.__file__).resolve())'bun run lintbun run check:file-lengthbun run test:unit