Skip to content

Fix release review findings - #2

Merged
mattteufel-e2b merged 6 commits into
codex/cybergym-public-releasefrom
fix/review-findings
Sep 8, 2026
Merged

mattteufel-e2b merged 6 commits into
codex/cybergym-public-releasefrom
fix/review-findings

Conversation

@mattteufel-e2b

@mattteufel-e2b mattteufel-e2b commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #1 addressing confirmed findings from the release review, plus a second hardening pass after reviewing this PR itself. Stacked on codex/cybergym-public-release.

Round 1

  • Fix the openai-compatible.patch hunk offset so upstream's anthropic call_llm branch keeps its return (was left unreachable after the new branch).
  • _verify_upstream rejects a dirty vendored checkout, so run/smoke/batch/preflight cannot ship locally edited validator scripts under the pinned commit.
  • Result-archive traversal guard normalizes the joined path so .. members are actually rejected.
  • Remove gemini-cli from --agent. Gemini CLI speaks Google's protocol and both supported providers are OpenAI-compatible, so it always failed and was graded as a model failure.
  • Both packaged network policies declare bedrock-mantle.*.api.aws and the model-host check matches patterns, so --bedrock-region works outside us-west-2.
  • build_code_bundle copies the hardened install_codex.sh unconditionally; a mistyped --remote-install-codex path raises instead of silently shipping upstream's unhardened installer.
  • README: network-locked.json requires preloaded agent tooling that this release does not ship.

Round 2

  • Regenerate the compatibility patch with context and apply it without --unidiff-zero, so a misplaced hunk fails loudly instead of applying at the wrong line.
  • Refuse kind=run under a policy whose runtime allowlist is only the model host, in both preflight and execution. Such runs previously failed during tooling install and were recorded as eligible model failures that batch reuse never retried.
  • Drop symlink and hardlink members when extracting result archives, so a stray agent symlink neither escapes the artifact root nor fails collection of a completed run.
  • Validate the four asset override paths once at CLI entry instead of once per task after E2B calls.
  • Dirty-checkout error names a remedy that works (sync-upstream also refuses dirty trees).
  • RunOptions.agent typed as codex | openhands; stale gemini test assertion and comment removed; README lists supported agents; test asserts both packaged policies share host lists.

Round 4 (found by running a task end to end)

  • Docker Hub allows about 100 anonymous manifest requests per hour per address, so a lock that must resolve all 506 mutable tags before any task can run never completes without an authenticated Docker Hub plan. images lock --task <project>/<task> (repeatable) now resolves only the images those tasks need and merges into the existing lock. The loader accepts a partial lock; each task is still refused at resolution time if its image is absent, with the exact command to add it. The Buildx resolver backs off briefly on 429 and surfaces the registry's error text.

Verification

  • uv run pytest -q: 66 passed (52 existing + 14 in tests/test_review_fixes.py, each written before its fix and observed failing)
  • ruff check, ruff format --check, scripts/check_public_tree.py, actionlint pass
  • uv build and twine check pass
  • Regenerated patch applied to pinned upstream b861317 with plain git apply; anthropic branch returns

Release gate evidence (2026-09-08, this branch at e59fb5f)

PR #1 listed four outstanding gates and asked that they be reviewed before merge. All were exercised live on E2B with this branch:

Gate Evidence
Upstream network synchronization sync-upstream pins b861317 in CI (both Python jobs and the sdist job) and locally
Registry resolution of mutable image tags Full 506-tag lock is blocked by Docker Hub's anonymous limit (100 manifest requests/hour/address); task-scoped locks resolved curl/arvo_66012 (0 lookups, image already pinned) and ffmpeg/oss-fuzz_368729566 (1 lookup)
E2B template builds Base cybergym-e2e-dind:recipe-3b3d5f9d827b70b3 built and receipt-verified; FFmpeg cybergym-e2e-ffmpeg:recipe-e8010b6665ef25fa built in 94 s with the Opus archive checksum and FFmpeg image digest verified; preflight --kind smoke on the FFmpeg task routes to the hot template with a verified tag-to-build receipt
Live task smoke smoke curl/arvo_66012: completed, exit 0, benchmark.status=passed, outcome=ground_truth_passed (upstream S4 validator), 4.3 min end to end, sandbox i2wxj4q2v07fvrimwd9ff
Live agent run (extra) run curl/arvo_66012 --agent codex --provider bedrock --model openai.gpt-5.4: completed, artifacts collected, one Codex turn completed with no API errors, benchmark.status=failed at S1 (a genuine model miss on one attempt), 10.5 min, sandbox ibkmt2q3pfnufbi2ytx4u

Deferred findings are tracked in FOLLOW-UPS.md (issues are disabled on this repository).

Not changed

  • Per-process caching of the template tag-to-build verification is intentional; rebuilding per run is not wanted for this minimal setup.

🤖 Generated with Claude Code

- Correct the openai-compatible patch hunk so the upstream anthropic
  branch of call_llm keeps its return statement.
- Reject a dirty vendored upstream checkout at run time, not only in
  sync-upstream, so shipped validator code always matches the pin.
- Fix the result-archive traversal guard: normalize the joined path so
  ".." members are rejected lexically (filter="data" remains).
- Drop gemini-cli from --agent; it speaks the Gemini API, not the
  OpenAI-compatible transport the adapter wires, and failed silently.
- Declare bedrock-mantle.*.api.aws in both packaged policies and match
  model hosts as patterns so --bedrock-region works outside us-west-2.
- Copy the hardened install_codex.sh unconditionally so a mistyped
  --remote-install-codex path errors instead of shipping upstream's
  unhardened installer.
- Document that network-locked.json needs preloaded agent tooling that
  this release does not ship.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@cla-bot cla-bot Bot added the cla-signed label Sep 8, 2026
mattteufel-e2b and others added 5 commits September 8, 2026 14:03
- Regenerate openai-compatible.patch with three lines of context and
  apply it without --unidiff-zero, so a misplaced hunk fails loudly
  instead of applying silently at the wrong line.
- Refuse agent runs under a policy whose runtime allowlist is only the
  model host; agent tooling is installed in the task container and such
  runs previously failed and were graded as eligible model failures.
- Drop symlink and hardlink members when extracting result archives so
  a stray agent symlink neither escapes the artifact root nor fails
  collection of a completed run.
- Validate --network-policy, --patch-file, --remote-smoke, and
  --remote-install-codex once at CLI entry instead of once per task.
- Point the dirty-checkout error at a remedy that works; sync-upstream
  also refuses dirty trees.
- Type RunOptions.agent as codex|openhands, remove the stale gemini
  test assertion and comment, document supported agents, and assert the
  two packaged policies share their host lists.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- The dirty-checkout remedy ran `git clean -fd` without -C, which would
  clean the caller's repository; both steps now target the vendored path.
- Only scripts/ and projects/ are checked for local changes; those are
  the trees shipped to the sandbox, and IDE metadata elsewhere is harmless.
- Result extraction keeps in-tree hardlinks and symlinks and drops only
  members the tarfile data filter rejects.
- batch evaluates the network policy once before resolving or submitting
  any task, matching preflight and run.
- Type the run kind as a Literal and update the README to say the locked
  policy is refused up front for agent runs while smoke still works.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
main() requires E2B_API_KEY before dispatching to batch. The test passed
locally only because the key was set in the developer shell; set a dummy
value so CI exercises the policy gate instead of the key check.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Docker Hub allows about 100 anonymous manifest requests per hour per
address, so a lock that must resolve all 506 mutable tags before any
task can run never completes without an authenticated Docker Hub plan.

- `images lock --task <project>/<task>` (repeatable) resolves only the
  images those tasks need and merges into the existing lock for the same
  upstream inputs, so the inventory can be walked in slices.
- The lock loader no longer rejects a partial lock; every task is still
  refused at resolution time if its image is absent, with the exact
  command to add it. Extra, stale, mutable, and provenance-free entries
  are rejected as before.
- The Buildx resolver backs off and retries briefly on a 429 and reports
  the registry's error text instead of a bare exit status.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Issues are disabled on this repository, so the confirmed reliability
findings from the release review that PR #2 leaves alone are tracked
in-tree.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mattteufel-e2b
mattteufel-e2b marked this pull request as ready for review September 8, 2026 22:34
@mattteufel-e2b
mattteufel-e2b merged commit 42c476a into codex/cybergym-public-release Sep 8, 2026
4 checks passed
mattteufel-e2b added a commit that referenced this pull request Sep 8, 2026
Land PR #2 on main, remove dead code, rewrite README for customers
@mattteufel-e2b
mattteufel-e2b deleted the fix/review-findings branch September 15, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant