Secure Pool harness binary cache - #2335
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83d78c2f9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from verifiers.v1.types import SystemMessage, TextContentPart, UserMessage | ||
|
|
||
| POOL_DIR = "/tmp/vf-pool-{version}" | ||
| POOL_DIR = ".vf-pool/bin-{version}" |
There was a problem hiding this comment.
Isolate the cache from sibling subprocess rollouts
For Pool runs on SubprocessRuntime, this relative path does not create verifier-owned isolation: every rollout workdir is an enumerable /tmp/vf-* directory and its processes run as the same host user (verifiers/v1/runtimes/subprocess.py:43-55). Since Pool runs with its sandbox disabled and completed run_program descendants are not tracked by cleanup() (which only terminates _background, lines 105-113), a malicious rollout can leave a detached watcher that overwrites .vf-pool/bin-*/pool in subsequently created workdirs before launch. The persistent cross-rollout code-execution issue therefore remains for the subprocess runtime; use OS-level isolation/a dedicated user or reject that runtime for Pool rather than treating a workspace-relative path as a security boundary.
Useful? React with 👍 / 👎.
ApprovabilityVerdict: Needs human review This security-focused change modifies binary cache isolation to prevent cross-rollout tampering. An unresolved P1 review comment questions whether the approach achieves its security goal for SubprocessRuntime, and security-sensitive code warrants human review. You can customize Macroscope's approvability policy. Learn more. |
Motivation
/tmpcache.Description
"/tmp/vf-pool-{version}"to a per-rollout path at".vf-pool/bin-{version}"by updatingPOOL_DIRinverifiers/v1/harnesses/pool/harness.py.shlex.quoteand using a rollout-local lock file (install.lock) so installation and existence checks operate only inside the rollout workspace./tmpcache would let one rollout tamper with the executable trusted by later rollouts.Testing
uv run pre-commit installwhich completed successfully in the environment where it could run.uv run ruff check --fix verifiers/v1/harnesses/pool/harness.pyandpython -m py_compile verifiers/v1/harnesses/pool/harness.py, both of which passed for the modified file.Codex Task
Note
Medium Risk
Touches the Pool harness install/execution path, which is security-sensitive because rollouts can run with model-approved tools. The change is small and clearly reduces cross-rollout binary poisoning risk.
Overview
Hardens Pool binary install so a malicious rollout cannot poison a shared executable used by later runs.
Moves the cached
poolbinary from shared/tmp/vf-pool-{version}to a per-rollout path at.vf-pool/bin-{version}, and scopes the install lock to that workspace. Also quotes install paths withshlex.quoteand documents why a shared/tmpcache was unsafe.Reviewed by Cursor Bugbot for commit 83d78c2. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Move Pool binary cache from
/tmpto workspace-local.vf-pool/bin-{version}and shell-quote pathsPOOL_DIRin harness.py from/tmp/vf-pool-{version}to.vf-pool/bin-{version}relative to the current workspace, scoping the cache per project.PoolHarness.setup(binary path, install directory, lockfile) to handle directories with spaces or special characters./tmp; each workspace maintains its own cache directory.Macroscope summarized 83d78c2.