feat(reference)!: directory-only references + anti-cheat permission window - #106
Open
akshaylive wants to merge 8 commits into
Open
feat(reference)!: directory-only references + anti-cheat permission window#106akshaylive wants to merge 8 commits into
akshaylive wants to merge 8 commits into
Conversation
…indow
Reference solutions were readable by the agent under evaluation, which shares
a filesystem with the harness — an agent could grep the task directory for the
answer instead of solving the task.
- `task.reference` is now directory-only. `code:`/`file:` are removed (a
directory is the only shape that can be permission-gated as a unit); the
removed keys raise a migration error naming the replacement.
`reference_comparison` gains a required `reference_file`.
- New `orchestration/permissions.py::set_permissions` — an async context
manager that chmods paths for the duration of a block. Windows STACK, so a
nested re-grant restores the enclosing mode rather than the original.
Crash-safe (finally + shield + atexit + SIGINT/SIGTERM).
- `Sandbox.set_permissions` wraps it and enforces only inside a container,
gated on CODER_EVAL_IN_CONTAINER — NOT on sandbox.driver, which the
in-container entrypoint rewrites to "tempdir".
- Docker mounts a throwaway read-write copy of the reference at
/work/references (`:ro` cannot be chmod'd — EROFS), masks its in-task-dir
original with an empty tmpfs, and drops DAC_OVERRIDE/DAC_READ_SEARCH/
FOWNER/CHOWN.
- Criteria address the reference via `$REFERENCE_DIR` (judge `files:`) and the
REFERENCE_DIR env var (`run_command`). `reference_code` is removed from the
criteria SPI; `CheckContext.reference_dir` supersedes it.
- `tasks/anti_cheat_reference` is an adversarial probe wired into CI smoke.
KNOWN GAP (documented in CLAUDE.md, docs/DOCKER_ISOLATION.md and the module):
this is defense-in-depth, not a boundary. chmod(2) is gated on
owner-or-CAP_FOWNER and the container runs as root owning the copy, so an
agent that deliberately runs `chmod 755 /work/references` regains access
(verified on Docker Desktop even with all four caps dropped). Passive reads
are blocked; an adversarial agent is not. Full containment requires running
the agent as a non-root uid — follow-up.
BREAKING CHANGE: `reference: {code: ...}` and `reference: {file: ...}` are no
longer accepted; use `reference: {directory: <dir>}`. `reference_comparison`
now requires `reference_file`. Third-party criteria must drop the
`reference_code` parameter from `_check_impl`/`_check_impl_async`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
akshaylive
requested review from
bai-uipath,
tmatup and
uipreliga
as code owners
August 11, 2026 22:22
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Review fixes: - Move permissions.py to a top-level leaf (fs_permissions.py): sandbox.py importing from orchestration/ was a layering inversion that would become a real cycle the moment the module needed anything from coder_eval. - Hard-fail when in-container with a declared reference but no /work/references mount. The old fallback resolved to the UN-masked reference under the :ro task-dir bind, which the window then cannot chmod (EROFS) — so the run would complete with the solution readable, reporting a normal pass/fail. - Scrub keys now match what the judge was actually shown: render_reference_dir truncates per file but collect_reference_secrets returned the untruncated text, so any file over max_file_chars was echoed verbatim into persisted transcripts and CriterionResult.details. - Bound the inlined reference block (200k chars) with an explicit omission marker; unbounded, a large tree blew the judge's context into a 0.0 score. - One token-matching rule (`path_uses_token`) shared by the judge resolver and the load-time validator; they disagreed, so `$REFERENCE_DIRECTORY/x` was a sandbox path to one and a reference consumer to the other. - Validator now also catches `$REFERENCE_DIR` in a run_command `command`. - `check`/`check_async` take turn_records/context keyword-only, matching `_check_impl*` — an untyped caller could otherwise bind a str to turn_records. - Drop the stale `reference_code` parameter from ~12 test overrides, including one that forwarded it positionally into a now keyword-only base (latent TypeError, unreached only because that checker never runs). - Set the crash-handler installed flag only after a successful install; drop the sub_agent alias; cache the resolved reference source instead of re-stat-ing it; fix the stale `task.reference.file` comment and the docs/EXTENDING.md SPI exemplar. CodeQL: - Replace `pytest.raises` with explicit try/except in three tests — CodeQL cannot model it as catching, so it reported the following asserts as unreachable and their variables as unused (alerts 77/78/80). - Use 0o700 instead of group-readable 0o750 in the mode-preservation test (75). - `await task` no-effect alert cleared by the same restructure (76). - `_handlers_installed` write is now the last statement in the guarded block (79). New coverage (+15): `_setup` actually arms the feature (mutation-verified); `$REFERENCE_DIR` resolver incl. the `$REFERENCE_DIRECTORY` lookalike and the no-reference case; REFERENCE_DIR env var set/unset; chmod-refusal skips its pop; unresolvable paths warn; out-of-order release of differing modes; deterministic render ordering and per-file truncation; rmtree of a tree left at mode 000; and a CI drift guard asserting EXPECTED_SMOKE_PASS_RUN and the smoke-pass globs match the tagged task set (both mutation-verified). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Move the install-once flag from a module-level global onto _PermissionStack. A mutable global read only by its own writer reads as dead to static analysis (py/unused-global-variable), and the state belongs with the registry whose entries the handlers restore. - Use string-target monkeypatch in the two new tests instead of re-importing coder_eval.fs_permissions, which the module already imports with `from ... import` (py/import-and-import-from). Also adds the crash-safety test the review flagged as missing: asserts the atexit hook and signal handlers install on the first push, do not re-install on the second, and that restore_all actually restores. Mutation-verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Windows Smoke Test job runs the full suite on a Windows runner, where `chmod` honours only the read-only bit — so mode 000 never takes and 17 of the new assertions read back 0o555. `os.geteuid` also does not exist there. Skips the host-side POSIX-mode tests on win32 (matching the existing idiom in test_docker_runner_mounts.py) and replaces the geteuid root check with a portable helper. This is NOT a coverage gap for Windows users: the window is enforced only when CODER_EVAL_IN_CONTAINER=1, which only DockerRunner sets, and Docker Desktop on Windows runs LINUX containers — so the in-container orchestrator that performs the chmod is on Linux and behaves exactly as these tests assert. A Windows host only sees the window under `driver: tempdir`, where it is a deliberate no-op on every platform. The real behaviour stays covered by the Linux jobs and by tasks/anti_cheat_reference, which runs inside the container. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…anti-cheat
Works through the pr:106 review (1 critical, 7 high, 15 medium, 11 low) plus
the outstanding CodeQL alerts. The theme is the layer that decides scores.
Scoring correctness (an eval-config error must not read as an agent failure):
* reference_comparison now raises CheckerMisuseError (-> FinalStatus.ERROR) for
a typo'd/unreadable/empty reference_file instead of returning a gating 0.0,
which was counted against the agent's pass rate and silently zeroed every row
of a dataset-fanned suite. reference_file also gained a load-time validator
(non-empty, relative, no ..), and the orchestrator pre-flights it during
_stage_reference so it fails before the agent burns a token.
* Reference integrity: the tree is hashed at staging and re-verified before
grading. The window is per-turn and the docker mount must be writable, so an
agent-backgrounded writer could previously overwrite the reference and drive
reference_comparison to 1.0. Mismatch now raises ReferenceTamperedError.
* Judge scrubbing is keyed on what reached the prompt (JudgeContext records the
reference bytes it attached), not on include_reference. The documented
`include_reference: false` + `files: [$REFERENCE_DIR/rubric.md]` combination
was persisting the solution verbatim into the archived judge transcript.
agent_judge now also passes max_file_chars, so the key matches truncated text.
Anti-cheat, fail closed:
* Stop dropping FOWNER/CHOWN. The in-container orchestrator that APPLIES the
window is the same root process with the same caps, so dropping FOWNER breaks
the harness's own chmod wherever the bind mount preserves a non-root owner
(native Linux). Verified: container root + uid-1000-owned dir + FOWNER dropped
-> "Operation not permitted". The drop only bit where it also disabled the
control. The re-chmod hole stays the documented KNOWN GAP (needs a non-root
agent uid, not a smaller capability set).
* A window that cannot be applied is now a hard error (strict=True whenever
Sandbox actually enforces), not a warning — an unprotected run must not be
indistinguishable from a protected one downstream.
fs_permissions:
* Crash handlers install from the event-loop thread. They were installed from
push(), which only runs on an asyncio.to_thread worker where signal.signal
raises ValueError into a swallowing except — so SIGTERM had NO restore, and
the flag latched anyway. Install now reports success and is retried if it
fails; a failed install logs WARNING.
* The acquire moved inside the try. asyncio.shield protects the inner task, not
the await, so a cancel on __aenter__ skipped the finally while every chmod
completed — leaving the path at 000 with no matching pop and a stale registry
entry that poisoned the next window.
* pop() keeps its entry when the restoring chmod fails, so restore_all still
holds the pre-window mode.
* Precise signal typing removes the blanket `# type: ignore`; SIG_IGN is handled.
Cleanup / dedupe:
* rmtree_restrictive moved to path_utils and WIRED IN — it had no production
caller, while both live cleanup sites used the swallowing rmtree its own
docstring rejects, orphaning mode-000 reference trees.
* _cleanup keys on _reference_staging_root, recorded before the copy, so a
copytree that raises does not leak a partial copy of the solution.
* Reference resolution and the copytree ignore list are shared between the two
drivers (resolve_host_reference_dir, REFERENCE_COPY_IGNORE).
API / validators:
* SuccessChecker.check/check_all/check_all_async take trailing args keyword-only
(reference_code was removed from the middle, so positional callers misbound
silently). scrub_reference is list[str] | None — str satisfied the old union.
* The reference-consumer validator narrows with isinstance instead of untyped
getattr, and matches ${REFERENCE_DIR} via a new command_uses_token seam.
* Sandbox gained a reference_dir constructor kwarg, mirroring task_dir.
Probe:
* verdict.txt/command_executed are weight 0 — `weight` does not soften a strict
AND gate, and this task blocks the e2e-smoke bucket.
* Step 3 used $TASK_DIR, which is NOT in the agent's environment, so it expanded
to empty and the tmpfs-mask check was inert. It now hunts the path with find.
* Verified live against a rebuilt container: 1/1, all six criteria, agent denied.
Lint (each traceable to a defect above): CE033 no unreferenced private helper in
src/, CE034 acquire inside the try of an async CM, CE035 no gating 0.0 from an
except OSError in a checker. All three verified to fire on the original code.
Also: 100% coverage on the in-container branch (was 0%, and docker is the only
driver the feature runs on); fs_permissions 89% -> 98%; assert the dropped-cap
set exactly; pin the probe's canary to its own detector; Makefile smoke globs
pinned to CI's; migration + score-comparability + image-lockstep notes; removed
an inert `# nosec` (bandit does not flag asyncio.create_subprocess_shell).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o wire The module docstring said "No in-tree caller needs the inner form today ... The stack exists so that adding one does not require reworking this module", which reads as textbook speculative generality — and the PR review duly filed it as one, recommending the stack and READ_ONLY_MODE be deleted. They should not be. The re-grant is a designed seam for live success criteria: early-stop verdicts run mid-turn, inside the 000 window, so a live criterion that consults the reference has to read it exactly while the agent cannot. A flat set/restore cannot express that and a refcount actively breaks it. Records that, plus the remaining work and the three non-obvious constraints found while scoping it (all deliberately NOT implemented here): * the window belongs around the watcher's verdict LOOP — tightest placement, which matters because a chmod is global state and the agent runs concurrently, so the re-grant is visible to it for as long as it is open; * that loop is a sync StreamCallback, so it needs a sync twin pushing onto the same registry; * live_verdict gains no parameter — it reads a per-task accessor, which must be a ContextVar and not os.environ, because `run_batch -j 8` shares one process and a process-global would leak one task's reference into a sibling's verdict under parallelism only. (REFERENCE_DIR today is set only in the env= dict for run_command subprocesses, so it is not readable in-process.) Also scopes it: only the reference is shielded, never the sandbox. Reading the static reference mid-turn cannot break LiveVerdict monotonicity; reading the half-written sandbox can, and is the end-state peeking live_verdict rules out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Review addressed — all 8 blockers, 15 non-blocking, 11 nitsPushed as Blockers
On #6 — reproduced the failure directlyThe in-container orchestrator that applies the window is the same root process with the same caps as the agent, so dropping On #2 — declined, with the underlying doc defect fixedThe reviewer's facts were right ( What misled the review was the docstring, which said "No in-tree caller needs the inner form today... The stack exists so that adding one does not require reworking this module." Also note deleting the signal chaining (as #2 proposed) would have deleted the fix for #5. Beyond the listed findings
Verification
Ran the probe live against a rebuilt container: 1/1, all six criteria, agent denied at every step. Step 3's |
| """Dropped reference files change what the judge grades against, so the | ||
| omission must be visible in the prompt rather than read as 'the reference | ||
| doesn't implement that'.""" | ||
| import coder_eval.evaluation.judge_context as jc |
| await started.wait() | ||
| task.cancel() | ||
| with contextlib.suppress(asyncio.CancelledError): | ||
| await task |
…the task dir
The `--cap-drop DAC_OVERRIDE --cap-drop DAC_READ_SEARCH` added for the
reference anti-cheat broke EVERY `driver: docker` task. The container runs as
root but does not own the framework-owned bind mounts -- on native Linux they
preserve the uid that ran coder-eval -- so all its access is an "other" access
that only ever worked via the capability. The in-container orchestrator died on
its first `open('/work/output/task.log', 'w')` with EACCES, taking
byod_smoke_test (which has nothing to do with references) down with it. macOS
Docker Desktop hid this: virtiofs reports the mount as root-owned.
`grant_container_access` widens the framework-owned mounts host-side, so access
goes through the `other` bits instead of a capability. `chmod -R o+rwX`
semantics; read-only for what the container merely consumes. The drop and the
widening are counterparts -- drop without widening kills every docker task,
widen without dropping makes the mode-000 window a no-op.
Also replaces the symmetric `-v <host task dir>:<host task dir>:ro` mount with
a shielded COPY at /work/task_dir, held at mode 000 for every agent turn
alongside the reference. Verified against a real container: `:ro` makes the
window inexpressible (`chmod: Read-only file system`), and read-write without a
copy chmods the operator's own `tasks/` tree (host dir came back 0600; cleanup
then failed with Permission denied). This retires the `--tmpfs` mask and closes
a leak it could not reach -- a flat `tasks/foo.yaml` has parent `tasks/`, so the
old mount exposed every sibling task's reference solution.
Symmetry was never load-bearing: run_task_internal_command uses --task-dir only
to seed TASK_DIR, and never re-reads the path. TASK_DIR is exposed solely to
run_command criteria, so the agent loses nothing legitimate.
Does NOT hide the task definition: task.yaml is also staged at /work/input, and
that mount is untouched by the window.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Code Review — PR #106Reviewed per Review Metadata
Nothing here is 🔴 Critical or 🟠 High. Every finding below was reproduced against the code; two reviewer findings were downgraded after verification (noted inline), and none of the six Mediums blocks the anti-cheat control from working as advertised under docker. 🟡 MediumM1 —
M2 — the DAC cap drop is container-wide, but the compensating widening covers only framework-owned mounts.
M3 — the empty-tmpfs mask is gone from the code but still documented as present on 7 surfaces, including two that overstate the guarantee to an operator.
M4 — a strict (enforced) window fails OPEN when
M5 — a
M6 — the reference is now duplicated into the
🔵 Low (reported, not fixed)L1 — L2 — L3 — YAGNI: Perf note (not a finding): Positives
🤖 Generated with Claude Code — multi-model review ( |
uipreliga
self-requested a review
August 12, 2026 15:29
uipreliga
approved these changes
Aug 12, 2026
uipreliga
left a comment
uipreliga
left a comment
Collaborator
There was a problem hiding this comment.
Fix what you agree with, maybe ask someone else also for review?
bai-uipath
approved these changes
Aug 12, 2026
bai-uipath
left a comment
bai-uipath
left a comment
Collaborator
There was a problem hiding this comment.
Approve — but please do a full suite dry-run before this lands. Not a formality: this changes container-wide access semantics, and there are a lot of features used by only a handful of tasks that all touch these paths. Unit tests and the smoke bucket won't reach them.
It has to be a native-Linux host (the VM or an ADO agent, not Docker Desktop — virtiofs hides the entire cap-drop class), over a slice that covers templates, plugin mounts, a uip-auth task, a simulation and a reference-bearing task, grepping the logs for EACCES. The reason: dropping DAC_OVERRIDE/DAC_READ_SEARCH removes container root's bypass on every bind mount, but only the framework-owned copies get widened. Every auto-mounted host path keeps its host modes and must now be world-readable, and ~/.uipath is mounted read-write and is written to in-container.
Checked and clear from my side: pre_run/post_run, $TASK_DIR criteria, simulation turns, and fixtures reaching the agent all sit outside the window.
Likely risks
Rated 0-10 on likelihood of actually breaking something, 0 = no risk.
| Area | Risk | Note |
|---|---|---|
| Host mounts under the cap drop | 6 | Plugins, template dirs, extra mounts keep host modes; ~/.uipath needs write access it may not have |
| Out-of-tree criteria | 4 | reference_code is gone from the SPI — other suites' custom criteria break at call |
| Downstream task migration | 4 | On the current pin directory: loads but the judge silently drops the reference, so there's no safe intermediate state: migration and pin bump have to be atomic |
| Probe in the blocking smoke bucket | 4 | Gating criteria depend on model behaviour, so it can redden unrelated PRs |
| Judge prompt shape | 3 | One header line for single-file references; agent_judge consumers move from inlined to mounted |
| New strict failures | 3 | A refused chmod now errors the task rather than warning |
| Run dir left world-writable | 3 | Widening is never reverted |
| Per-task task-dir copy | 1 | Negligible nested, whole tree for flat layouts |
Fix if you agree, otherwise lgtm
- The reference exists twice. The task-dir copy carries it too, and the integrity hash only covers the dedicated mount. Excluding
reference.directoryfrom that copytree leaves one shielded copy. - Scope the claim in the docs. Where a suite mounts its repo root as a plugin, the same answer key is readable outside the window for the whole run, and the staged task YAML still carries the full criteria — worth saying plainly so a green probe isn't read as coverage.
- Stale narrative. The tmpfs mask and the
FOWNER/CHOWNdrop are still described as present across several shipped surfaces, including the probe task's own description, which is the artifact people will trust over the argv.
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
4 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.
Reference solutions were readable by the agent under evaluation — it shares a filesystem with the harness, so an agent could grep the task directory for the answer instead of solving the task. This closes the passive-read path and adds an adversarial probe to CI.
Read this first: known gap
This is defense-in-depth, not a boundary.
chmod(2)is gated on owner-or-CAP_FOWNER, and the container runs as root, which owns the bind-mounted reference copy. An agent that deliberately runschmod 755 /work/referencesregains access:Verified on Docker Desktop even with
DAC_OVERRIDE,DAC_READ_SEARCH,FOWNERandCHOWNall dropped — the owner check passes with no capability at all. Dropping those caps does close it where the mount preserves a non-root owner (native Linux), so they are kept.Passive reads are blocked; an adversarial agent is not. Full containment requires running the agent as a non-root uid that does not own the reference — deliberately left as follow-up rather than bolted on here. Documented in
CLAUDE.md,docs/DOCKER_ISOLATION.md,docs/TASK_DEFINITION_GUIDE.md, the module docstring, and the probe task itself, so nobody reads a green probe as "the reference is unreachable".What changed
task.referenceis directory-only.code:/file:removed — a directory is the only shape that can be permission-gated as a unit. The removed keys raise a migration error naming the replacement.reference_comparisongains a requiredreference_file.orchestration/permissions.py::set_permissions— async CM that chmods paths for a block. Windows stack, so a nested re-grant (mode=READ_ONLY_MODE) restores the enclosing mode, not the original; the stack also subsumes what a refcount would do. Crash-safe:finally+asyncio.shieldon both halves +atexit+ chained SIGINT/SIGTERM.Sandbox.set_permissionsenforces only inside a container, gated onCODER_EVAL_IN_CONTAINER— not onsandbox.driver, which the in-container entrypoint rewrites totempdir(a driver-based gate would silently disable the feature on exactly the path that needs it; regression-guarded)./work/references(:rocannot be chmod'd — EROFS), empty tmpfs masking the in-task-dir original, four caps dropped.$REFERENCE_DIRtoken in judgefiles:+REFERENCE_DIRenv forrun_command.reference_coderemoved from the criteria SPI;CheckContext.reference_dirsupersedes it.tasks/anti_cheat_reference— adversarial probe, taggedsmoke-pass, wired intoe2e-smoke(EXPECTED_SMOKE_PASS_RUN7→8; the glob names the subdir explicitly becausetasks/*.yamlis not recursive).The task directory is deliberately not shielded: under docker it is a
:romount (EROFS) and the same YAML is readable at/work/inputregardless, so it only produced a per-turn warning.How the bugs were found
The probe task caught a real bug on its first live run — the agent read
/work/referencesin full, because a:romount can't be chmod'd so the code was shielding an in-container staged copy while the real mount stayed readable. Every unit test passed; only an agent reaching for the actual path exposed it. A subsequent 8-axis review found thechmod-restore bypass above, plus:task_dirshielding was inert under docker,resolve_reference_dirkeyed container detection on a bare/work/referencesprobe (would hijack any host with that path), the acquire half of the window wasn't cancellation-shielded,_stage_referenceleaked its tempdir on copy failure, andagent_judgesilently dropped$REFERENCE_DIRentries wheninclude_reference: false. All fixed here.Verification
make check/make lint(177) /make test(3955 passed, 8 skipped) green.make typecheckunchanged at 3 pre-existingopenai_codeximport errors (confirmed identical on a clean checkout).tasks_run=8matchesEXPECTED_SMOKE_PASS_RUN._setupwiring, and the stack semantics.Breaking changes
reference: {code:}/{file:}→reference: {directory:}.reference_comparisonrequiresreference_file. Third-party criteria must dropreference_codefrom_check_impl/_check_impl_async. Judge prompts change shape for reference-bearing tasks (agent_judgemounts instead of inlining;llm_judgeinlines labelled per-file blocks), so such suites need re-baselining.🤖 Generated with Claude Code