fix(sandbox): stop with-skill builds poisoning no-skill rollouts - #938
fix(sandbox): stop with-skill builds poisoning no-skill rollouts#938bingran-you wants to merge 1 commit into
Conversation
A with-skill build rewrites the task Dockerfile in place: --skills-dir appends `COPY _deps/skills /skills/` plus a symlink into every agent skill path. The image tag, though, was derived from the task name alone (`bf__<task>`), so the with-skill and no-skill builds of one task shared a single tag. Whichever built last decided what both started from. A no-skill rollout could therefore run inside an image that already contained /skills and the agent-side symlinks. benchflow resolved the rollout correctly (skill_mode=no-skill, include_task_skills=False, skills_sandbox_dir=None) and skipped every deployment path — and the agent still read mentor SKILL.md files off disk, because they were baked into the image by a different rollout. Nothing caught it. total_skill_invocations reads 0 whether or not the agent loads a skill from disk, so a contaminated run reports a clean pass. This is silent corruption of the primary no-skill capability metric, and a task author cannot prevent it: the task's own Dockerfile is clean. Observed on a FrontierPhysics task: a no-skill rollout read the full mentor recipe from /home/agent/.opencode/skills/ and solved in 31 tool calls what the uncontaminated condition had previously failed. Fix the tag, then verify the condition: - Derive the image tag from a fingerprint of the build context (Dockerfile bytes plus the staged _deps/skills layout), so builds whose contents differ cannot share a tag. - Add assert_no_skill_isolation(), called after deploy_skills on the agent path when the recorded mode is no-skill. It probes the agent's own discovery paths and /skills, and fails the rollout rather than let a contaminated run be recorded as a result. Only absolute paths ending in /SKILL.md count as findings, so shell noise on the probe's stdout cannot produce a false positive.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1b04468e3
ℹ️ 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".
| await self._planes.assert_no_skill_isolation( | ||
| self._env, self._agent_cfg, cfg.sandbox_user | ||
| ) |
There was a problem hiding this comment.
In no-skill rollouts that supply a custom plane bundle, this unconditional call now raises AttributeError unless every existing implementation adds the new method. This already breaks the synthetic _FakePlanes used by tests/test_task_runtime_primitive.py, and the same failure affects external BYO plane implementations accepted by TaskRuntimeConfig and the TRL integration. Provide a kernel fallback or update all supported implementations before requiring this method.
Useful? React with 👍 / 👎.
| for path in sorted(skills_root.rglob("*")): | ||
| hasher.update(str(path.relative_to(skills_root)).encode()) | ||
| if path.is_file(): | ||
| hasher.update(str(path.stat().st_size).encode()) |
There was a problem hiding this comment.
When two with-skill rollouts use the same task and skill paths but different same-length SKILL.md contents, this fingerprint is identical because it hashes only each path and byte count. start() serializes the builds but releases the image lock before compose up, so the second build can overwrite the shared tag before the first container starts, silently giving that rollout the wrong mentor content. Hash the file bytes so every distinct staged payload receives a distinct tag.
Useful? React with 👍 / 👎.
| if agent_cfg is not None: | ||
| for skill_path in agent_cfg.skill_paths: | ||
| candidates.append(skill_path.replace("$HOME", home)) |
There was a problem hiding this comment.
For agents with $WORKSPACE discovery paths, such as OpenHands and OpenClaw, this only expands $HOME; shlex.quote() then makes $WORKSPACE/.agents/skills a literal relative path. A no-skill sandbox containing guidance only at the actual workspace discovery path therefore passes this guard and can still produce a contaminated result. Pass the agent cwd and expand $WORKSPACE exactly as _link_skill_paths() does.
AGENTS.md reference: AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
| def test_skill_injection_changes_the_fingerprint(tmp_path): | ||
| """The core regression: the two builds must not share an image tag.""" |
There was a problem hiding this comment.
This test explicitly identifies itself as guarding the core regression, but its docstring names neither a PR nor a commit. Add the identifier for the change it guards so future maintainers can trace the intended behavior as required by the repository convention.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
1 participant
A with-skill build and a no-skill build of the same task share one Docker image tag. Whichever builds last decides what both start from — so a no-skill rollout can run inside an image that already contains
/skills, and the agent reads the mentor guidance the condition exists to withhold.The bug
--skills-dirrewrites the task Dockerfile in place (_inject_skills_into_dockerfile):But the tag came from the task name alone (
docker.py):Both modes therefore resolve to
bf__<task>:latest.Evidence
Same task, same host:
SKILL_MD_COUNT=0COUNT=0,/skillsemptyCOUNT=4— all four skills at/skills/COUNT=0✅COUNT=4❌The affected rollout was resolved correctly and still leaked:
Both deployment paths behaved:
_activate_step_skillsreturned early (skills_dir: null) anddeploy_skillsno-opped. The skills were already in the image, put there by a different rollout.Its trajectory then shows the agent loading
ion-shuttling-mentor,surface-ion-trap-bemandinvariant-based-transportfrom/home/agent/.opencode/skills/, and solving in 31 tool calls a task whose uncontaminated no-skill condition had previously failed.Why it went unnoticed
total_skill_invocationsreports0whether or not the agent reads aSKILL.mdoff disk — it was0for the with-skill runs too. A contaminated run reports an ordinary pass and no counter disagrees.This silently corrupts the primary no-skill capability metric, and a task author cannot prevent it — the task's own Dockerfile is clean.
Note this is not the familiar "task bakes its own skills via
COPY skills …" mistake. That one is the task's to fix; this one isn't.The fix
_build_context_fingerprint()hashes the Dockerfile bytes plus the staged_deps/skillslayout, and the tag becomesbf__<task>__<fingerprint>. Builds whose contents differ can no longer share a tag. Hashing what is actually built also means an unrelated Dockerfile edit correctly yields a new image.assert_no_skill_isolation()runs afterdeploy_skillson the agent path when the recorded mode isno-skill. It probes the agent's ownskill_paths(following symlinks) plus/skills, and raises instead of letting a contaminated run be recorded. Only absolute paths ending in/SKILL.mdcount, so shell noise can't produce a false positive.Belt and braces on purpose: (1) removes the cause, (2) means a future regression fails loudly instead of quietly returning a wrong number.
Tests
tests/test_no_skill_image_isolation.py— 7 cases:/skillsSKILL.mdis reachableVerification
-k "docker or skill or install or image");ruff checkclean and formatted.tests/test_trial_install_agent_timeout.pyinitially failed against an early version of the guard — its env double returns canned stdout for everyexec. That surfaced a real over-permissiveness in my check rather than a test problem, so the guard now matches onlySKILL.mdpaths; that suite is green.Reviewer notes
bf__<task>:latestexternally would need updating — I found no such assumption in-tree, but it's the change worth a second pair of eyes.