Skip to content

Guard the scored env at the object; follow imports for the planner check - #176

Open
yixuanhuang98 wants to merge 4 commits into
mainfrom
fix/scored-env-guard-169
Open

Guard the scored env at the object; follow imports for the planner check#176
yixuanhuang98 wants to merge 4 commits into
mainfrom
fix/scored-env-guard-169

Conversation

@yixuanhuang98

Copy link
Copy Markdown
Collaborator

Fixes #169.

Both generated-approach anti-cheats in load_generated_approach read only approach.py, while the loader puts the sandbox dir on sys.path so approach.py can import siblings the agent wrote. Each check was wrong in a different direction.

set_state / sample_next_state: guard the object, not the source

_reject_state_mutation regex-scanned for .set_state / .sample_next_state. It missed the real channel — a program reached the same call by moving it one file over — and it rejected programs that do nothing wrong: planning in a private environment the program builds itself is the ordinary way to write a TAMP policy and cannot affect scoring, but it calls set_state.

The real invariant is that the environment being scored is not mutated, and there is exactly one path to it from generated code: env-bound primitives close over the live env (check_action_collision is a partial over it), so a program granted one can pull it out of fn.args. build_primitives now binds a read-only view instead. The mutating entry points raise ScoredEnvMutationError; reads, geometry handles and collision queries pass through, and isinstance still resolves to the wrapped class since primitives dispatch on it.

This is exact where the scan was approximate: aliasing, getattr and sibling modules all reach the same guarded object, and a private environment is untouched. With primitive_level=none there is no path at all and the guard has no job.

This half is b492d5a from pr2-packed-tamp-env, recovered as the issue suggests.

Planner references: walk the import graph

_reject_planner_references had the same single-file blind spot but no runtime chokepoint to move to — SeSamE is a library call, not an object the harness hands out. Alternatives considered were an import hook (would break the in-process bilevel_planning_approach baseline) and scanning every .py in the sandbox (false positives on the scratch files agents leave behind, which is the failure mode this PR is trying to remove).

It now walks the import graph: approach.py plus every sandbox module it transitively imports, resolved with ast, handling packages and relative imports. Writing the call in planner.py no longer evades it, the error names the offending file, and an unimported scratch file is not grounds for rejection.

Verification

Loaded replicate 42's rejected program (multirun/.../2026-08-24_20-51-26/replicate_42.rejected_by_old_anticheat/) unchanged against PR2PackedVariableCountEnv:

blocks=3 solved=True steps=19
blocks=5 solved=True steps=31

It solves both. (The issue cites 40 and 56 steps; measured here on seed 0 driving get_action directly, so the counts differ.)

Also confirmed replicate 24's planner.py builds its own PR2PackedEnv(num_blocks=self.n) — it was not actually cheating either, it just demonstrated that the channel was open.

./run_ci_checks.sh passes: mypy clean, pylint clean, 775 passed / 1 skipped.

Tests

  • The three tests that asserted the scan's behaviour are rewritten against the runtime guarantee, including the red-team primitive-closure teleport, which now checks that the write raises and that the scored env is left where it was.
  • New tests/utils/test_scored_env_guard.py: pass-through reads, blocked mutators, blocked attribute assignment, isinstance dispatch, idempotency, the primitive-closure path against a real PR2PackedEnv, and that private environments are unaffected.
  • Three new tests for the planner-check import walk: sibling module, sibling package via a relative import, and an unimported scratch file that must not trigger rejection.

Not addressed here

  • mcp/local_render.py binds a raw env when rebuilding primitives in-sandbox. That env is not the scored one, so it is not a hole, but it is now inconsistent with build_primitives.
  • The blackbox env_client.make_primitives() returns bound methods whose __self__ is a BlackboxEnv, so set_state is reachable there — but make_env() creates a fresh host-side instance per call, not the scored one. Scoring goes through build_primitives, which is guarded.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Bx7uCXEqziaAxTKbdkmp1g

yixuanhuang98 and others added 2 commits August 30, 2026 14:23
Fixes #169. Both generated-approach anti-cheats read only `approach.py`, while the
loader puts the sandbox dir on `sys.path` so `approach.py` can import siblings the
agent wrote. Each check was wrong in a different direction.

`_reject_state_mutation` regex-scanned for `.set_state` / `.sample_next_state`. It
missed the real channel -- a program reached the same call by moving it one file over
(replicate 24 of the 2026-08-21 pr2packed whitebox sweep did exactly that) -- and it
rejected programs that do nothing wrong: planning in a *private* environment the
program builds itself is the ordinary way to write a TAMP policy and cannot affect
scoring, but it calls `set_state`. Replicate 42 of the 2026-08-24 sweep was killed by
this after running to completion, for inlining its planner rather than importing it.
The check was crude enough to reject a string literal, as its own test asserted.

The real invariant is that the environment being *scored* is not mutated, and there
is exactly one path to it from generated code: env-bound primitives close over the
live env (`check_action_collision` is a `partial` over it), so a program granted one
can pull it out of `fn.args`. `build_primitives` now binds a read-only view instead.
The mutating entry points raise `ScoredEnvMutationError`; reads, geometry handles and
collision queries pass through, and `isinstance` still resolves to the wrapped class
since primitives dispatch on it. This is exact where the scan was approximate:
aliasing, `getattr` and sibling modules all reach the same guarded object, and a
private environment is untouched. Verified by loading replicate 42's rejected program
unchanged -- it now solves 3 and 5 blocks in 19 and 31 steps.

`_reject_planner_references` had the same single-file blind spot but no runtime
chokepoint to move to: SeSamE is a library call, not an object the harness hands out.
It now walks the import graph instead -- `approach.py` plus every sandbox module it
transitively imports, resolved with `ast` -- so writing the call in `planner.py` no
longer evades it, and the error names the offending file. Only reachable modules are
scanned, so a scratch file left in the sandbox is not grounds for rejection.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bx7uCXEqziaAxTKbdkmp1g
The autoformat job floats to the latest isort (`isort-action` with `latest`), which
is now 9.0.1, while the pinned dev dependency is isort 7. They disagree on two
points, and the job failed on the first:

- The new import was written as a trailing-comma block, which isort 7 keeps under
  `split_on_trailing_comma` but isort 9 collapses. It fits on one line, so write it
  that way and neither touches it.
- `_thread` is third-party to isort 7 and stdlib to isort 9, so each moves it out of
  where the other puts it. `extra_standard_library` pins the classification. This is
  pre-existing -- main would fail the same check on a re-run -- but it lands in the
  file this branch already touches.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bx7uCXEqziaAxTKbdkmp1g
@yixuanhuang98

Copy link
Copy Markdown
Collaborator Author

Yixuan's comments: I’m not very confident about the changes my Claude made for the anti-cheating fixes and would appreciate a detailed review from @tomsilver and @merlerm!

@merlerm merlerm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think in general this is a good fix and we should implement it. I agree that the code is a bit convoluted, I had a look and it seems mostly fine but I can have my agent review it more closely. One point though: I think it would be good to have some examples of code into the tests, so that it's also more clear for us to see what programs would pass and what wouldn't. This should make it clear if the behavior is what we want

Review asked for example programs in the tests, so that what passes and what does not
is legible rather than inferred from the guards' unit tests.

`tests/utils/test_generated_approach_examples.py` holds thirteen complete programs,
each run to an outcome. The toy env terminates after 51 honest steps of +0.1 and after
1 from a teleport, so the step count in each assertion says which happened rather than
the assertion just naming a rule.

Accepted: an honest policy; a program planning in a simulator it owns (the false
positive that cost replicate 42, calling `set_state` throughout); the same program with
its planner in a sibling module; a program naming a mutator in a string; a program
reading the scored env through a primitive's closure; and one composing the granted
bilevel models.

Rejected: the teleport through a primitive's closure; the same teleport with the method
name assembled at runtime, so it appears nowhere in the source; an attribute write; a
mid-episode `reset`; `run_sesame` in `approach.py`; and `run_sesame` in a sibling. Each
of the four runtime cases also asserts the scored env is left where it was.

The last example is the boundary the other direction: a planner reference in a scratch
file nothing imports is not grounds for rejection.

Checked that the examples are load-bearing rather than vacuous -- binding the raw env
instead of the read-only view fails exactly the four teleport tests, and restoring the
single-file scan fails exactly the sibling-planner test.

`_StatefulGoalEnv` moves from `test_episode.py` to a `tests/utils/conftest.py` fixture
so both modules share one definition. The sibling modules in two examples are given
distinct names: `sys.modules` persists across loads, so two examples both shipping a
`planner.py` would resolve to whichever ran first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bx7uCXEqziaAxTKbdkmp1g
@yixuanhuang98

yixuanhuang98 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks @merlerm — added in 8c00c7c.

tests/utils/test_generated_approach_examples.py is twelve complete programs across fourteen tests, each run to an outcome rather than just checked for load/reject. The toy env terminates after 51 honest steps of +0.1 and after 1 from a teleport, so the step count in each assertion is what says whether the program cheated.

Accepted

  • an honest policy (the 51-step baseline)
  • a program that plans in a simulator it constructs itself, calling set_state throughout — this is the false positive that killed replicate 42
  • the same program with its planner in a sibling module
  • a program that names a mutator inside a string
  • a program that reaches the scored env through a primitive's closure and reads it
  • a program that composes the granted bilevel_models without running SeSamE

Rejected — worth noting these are stopped by two different mechanisms at two different times. The planner check stays a load-time source refusal (ValueError out of load_generated_approach, program never runs) because there is no runtime chokepoint for it; the scored-env check moved to runtime (ScoredEnvMutationError mid-episode, from inside get_action). That difference is not cosmetic: a runtime block is caught by the episode runner and scored as a crashed, i.e. unsolved, episode, whereas a load refusal kills the whole replicate — which is the failure mode the issue was filed about.

  • the teleport through a primitive's closure
  • the same teleport with the method name assembled at runtime (getattr(env, "set_" + "state")), so set_state appears nowhere in the source — the test asserts that
  • an attribute write on the scored env
  • a mid-episode reset()
  • run_sesame imported in approach.py
  • run_sesame imported in a sibling module — the gap this PR closes

Each of the four runtime cases also asserts the scored env is left where it was, so a blocked teleport is visibly a no-op rather than a partial one.

One more is the boundary in the other direction: a planner reference in a scratch file that nothing imports is not grounds for rejection. Agents leave scratch files all over the sandbox, and rejecting a run over dead code would be the same class of false positive this PR is removing.

I checked the examples are load-bearing rather than vacuous: binding the raw env instead of the read-only view fails exactly the four teleport tests, and restoring the old single-file scan fails exactly the sibling-planner test.

On "a bit convoluted" — agreed about the read-only proxy, and it's the part worth your agent's attention. The one piece of real subtlety is that __getattribute__ forwards __class__ to the wrapped env so that isinstance still picks the right primitive implementation; everything else routes through the guard. If that trade reads wrong to you, say so — the alternative is registering the env type explicitly at each primitive dispatch site, which is more code but less magic.

CI is green, and I also ran the checks at CI's scope locally (which is wider than run_ci_checks.sh — the local script skips src/robocode/mcp in mypy and tests/mcp in pytest).

@merlerm

merlerm commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Thank you, this is perfect and super comprehensive! One final question: the READS_THE_SCORED_ENV policy is allowed, but this looks like it might cause problems if we ever run blackbox with primitives, right? Since the agent would be able to read the env when not allowed. Can we make sure this can't happen?

Besides that this seems good to me!

Review asked whether the allowed `READS_THE_SCORED_ENV` policy is a problem for
blackbox runs. It is, and the combination is reachable today: `constraints.py` excludes
blackbox x bilevel but permits blackbox x low_level, which grants
`check_action_collision` -- an ENV_DEPENDENT primitive bound as a partial over the live
env. The sandbox is careful here (the manifest turns env-bound primitives into host
proxies precisely so the env structure does not leak), but eval is not: the same
program is loaded on the host with `build_primitives`' dict, whose closure holds the
scored env.

The read-only view does not help. It stops writes and deliberately passes reads
through, which is right under whitebox -- the agent has the env source anyway -- and
wrong under blackbox. Reads cannot be closed off in-process either: whatever the
binding looks like, a closure's contents are reachable.

So the combination is refused rather than served unsafely. `build_primitives` takes a
`blackbox` flag and raises when an env-bound primitive is requested under it;
`run_experiment` passes the flag it already computes; `constraints.py` stops campaigns
scheduling the condition. Lifting the restriction means proxying env-bound primitives
through the env server at eval time, the way the sandbox already does -- a bigger
change than this PR.

Nothing on disk is affected: every blackbox run so far is primitive_level=none, where
there is no env-bound primitive and so no closure to read.

Not a regression this PR introduced -- before it the same closure held the raw env, so
reads were available along with the writes it removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bx7uCXEqziaAxTKbdkmp1g
@yixuanhuang98

Copy link
Copy Markdown
Collaborator Author

Good catch — you're right, and it's reachable today rather than hypothetical. Fixed in 5523f78.

The hole. constraints.py excluded blackbox × bilevel but permitted blackbox × low_level, which grants check_action_collision — an ENV_DEPENDENT_PRIMITIVE, bound as partial(check_action_collision, env). The sandbox side is careful about exactly this: blackbox_primitive_manifest turns env-bound primitives into host proxies so the env structure never reaches the container. But eval doesn't go through the manifest — agentic_base._load_generated hands the generated program self._primitives, the host-side dict from build_primitives, whose closure holds the live scored env. So the blackbox boundary that holds during synthesis doesn't hold at scoring time.

Why the read-only view doesn't fix it. It stops writes and deliberately passes reads through. That's right under whitebox — the agent has the env source anyway — and wrong under blackbox. And reads can't be closed off in-process: it doesn't matter whether the env is bound via partial (fn.args), a closure (fn.__closure__[0].cell_contents), or an instance attribute; in a shared process the object is reachable. There's no version of this that's safe and still hands the program an env-bound callable.

So it fails closed. build_primitives takes a blackbox flag and raises when an env-bound primitive is requested under it; run_experiment passes the flag it already computes at line 125; constraints.py stops campaigns scheduling the condition. Error names the primitive and the way out (run whitebox, or primitive_level=none).

Two things worth saying plainly:

  • Nothing on disk is affected. Every blackbox run in multirun/ so far is primitive_level=none — no env-bound primitive, so no closure to read. This closes a latent hole, it doesn't invalidate results.
  • It isn't a regression from this PR. Before it, the same closure held the raw env, so reads were available alongside the writes this PR removed. The PR narrowed the channel; this commit closes what's left of it in the mode where it matters.

What this costs. blackbox × low_level is no longer runnable. If you want it back, the structural fix is to proxy env-bound primitives through the env server at eval time, mirroring what the sandbox already does — a real change, and bigger than this PR should carry. Happy to file it as a follow-up issue if that combination is on the roadmap; if you'd rather I do that now, say the word.

I also added a note on the READS_THE_SCORED_ENV example and its test marking it whitebox-only and pointing at the new guard, so the next reader doesn't have to re-derive what you just asked.

@merlerm
merlerm self-requested a review September 3, 2026 09:35

@merlerm merlerm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, at some point we may want to do a blackbox + primitives run but in general I think the primitives are not that important for us right now so we can just think about it later on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generated-approach anti-cheat scans only approach.py: bypassable via siblings, and rejects legitimate private-env planning

2 participants