Repository navigation
Conversation
Signed-off-by: Andrew Suter-Morris <asutermorris@nvidia.com>
Signed-off-by: Andrew Suter-Morris <asutermorris@nvidia.com>
Signed-off-by: Andrew Suter-Morris <asutermorris@nvidia.com>
Signed-off-by: Andrew Suter-Morris <asutermorris@nvidia.com>
aleckhoury
left a comment
aleckhoury
left a comment
There was a problem hiding this comment.
Approving. The live experiment's isolation is solid: root-only /tests and /logs (0700), the submission runs as UID 65534 from a read-only copy, leftover unprivileged processes are killed before grading, output is compared as canonical JSON, and only the two allowlisted summary files are uploaded. I ran the focused anti-cheat/evidence/skill-contract tests locally (574 passed, 1 opt-in Docker skip), and CI is green.
Inline comments are non-blocking, but I'd like to see the medium ones addressed soon, here or in a follow-up:
- Normalizer "sound" grader leaks hidden cases.
/testsis world-readable during grading. I confirmed in real Harbor/Docker that a submission parsingCASESfrom/tests/verify.pygets reward 1. - Live job has no
continue-on-error. Any incomplete run (provider error, malformed tool call, deadline) will turnmainand daily runs red, unlikesemantic-regression. verifier-cheatsuploads raw native evidence from a public repo. This breaks the existing summary-always / native-only-when-private convention.
Also worth confirming with the budget owner: every push to main plus the daily schedule runs up to 8 model-backed cells, each capped at 48k tokens. Separately, because the series key includes the uv.lock bytes, the Docker version and each cell's provider fingerprint, most batches will probably end up in their own series, which limits the longitudinal comparison.
| result = await environment.exec( | ||
| command="mkdir -p /work /controls /tests /logs/verifier; " | ||
| "chown root:root /logs; chmod 700 /logs; " | ||
| "chown root:root /tests /logs/verifier; chmod 755 /tests /logs/verifier; " |
There was a problem hiding this comment.
chmod 755 /tests lets the submission, which runs as 65534 during grading, read /tests/verify.py and its hidden CASES. I checked with Harbor/Docker: a submission that parses CASES with ast and prints the matching output gets reward 1 from the sound grader (semantic_passes=3).
The live panel already handles this (chown root:root /tests; chmod 700 /tests), and the new task-validation.md says to "review actual UID, write access, imports". Could this match the live setup (/tests 0700 root)? Please also consider adding a hidden-case-read control so the panel shows that the sound grader rejects it.
There was a problem hiding this comment.
Fixed in 5630d31. /tests is now root-owned 0700, with native receipts checking that the unprivileged submission cannot read it. Added a hidden-case-read control that attempts to extract CASES during grading: it earns 0 under the sound grader. The expanded real Harbor/Docker panel passed all 18 expected outcomes, including the positive reference and deliberately broken graders.
| live: | ||
| needs: plan | ||
| if: >- | ||
| github.ref == 'refs/heads/main' && | ||
| (github.event_name == 'push' || github.event_name == 'schedule' || | ||
| (github.event_name == 'workflow_dispatch' && inputs.run_live)) | ||
| environment: semantic-regression | ||
| runs-on: ubuntu-24.04 | ||
| timeout-minutes: 60 |
There was a problem hiding this comment.
run_anti_cheat_live.py returns 2 for any run that isn't completed: deadline, unhealthy controls, provider error, malformed tool call. That makes this job fail on main pushes and daily runs. semantic-regression's live job, which this one copies, sets continue-on-error: true for this reason. Since this is described as an experiment rather than a gate, should it do the same? The summary keeps the real status either way.
There was a problem hiding this comment.
Fixed in 5630d31: the live job now sets continue-on-error: true, matching semantic regression. The collector still exits nonzero and preserves its actual incomplete reason; this only makes the observational experiment advisory. Automatic main-push/daily execution and the existing environment gate are unchanged. Added a workflow regression assertion.
| run: uv run --locked python tools/collect_verifier_cheat_evidence.py --output "$RUNNER_TEMP/verifier-cheat-evidence" | ||
| - name: Retain verifier-cheat measurements and native evidence | ||
| if: always() | ||
| uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 | ||
| with: | ||
| name: verifier-cheat-evidence-${{ github.run_id }}-${{ github.run_attempt }} | ||
| path: ${{ runner.temp }}/verifier-cheat-evidence/ | ||
| include-hidden-files: true | ||
| if-no-files-found: error | ||
| retention-days: 30 |
There was a problem hiding this comment.
This uploads the whole verifier-cheat-evidence/ folder: job folders, Harbor logs, docker version / image inspect JSON, absolute runner paths. The repo is public. The convention elsewhere (gym-compatibility, skill-evaluator, and DEVELOPMENT.md "Retained CI measurements") is to upload the summary JSON always, and native evidence only when github.event.repository.private == true. The content here is fixture-only, so the risk is low, but could this follow the same split, and could the DEVELOPMENT.md artifact table be updated?
There was a problem hiding this comment.
Fixed in 5630d31. Public artifacts now contain only verifier-cheat-summary.json, and that file itself is an allowlisted v2 projection: no raw errors, paths, file manifests, or full Docker inspection payloads. Detailed evidence moved to verifier-cheat-private.json and the raw-directory upload is restricted to private repositories. Added workflow/privacy sentinel regressions and updated the DEVELOPMENT.md retention table. Verified 18 native results, 15 input hashes, and 292 private artifact hashes against the final source.
| or self.receipt["tool_calls"] >= MODEL["max_tool_calls"] | ||
| ): | ||
| raise RuntimeError("tool budget or name invalid") | ||
| output = await self.tool(environment, json.loads(call.function.arguments)) |
There was a problem hiding this comment.
With max_retries=0 (L129), one transient 429/5xx from the provider ends the cell. json.loads(call.function.arguments) here and the checks in tool() raise on malformed or over-long model arguments, which also ends the cell instead of sending a tool error back to the model. Combined with the exit-code behavior of the workflow, I'd expect many incomplete runs that measure provider and parsing flakiness, not grader sensitivity. If failing closed is intended, maybe record a distinct reason; otherwise return the error as the tool result and allow a small, bounded retry.
There was a problem hiding this comment.
Fixed in 5630d31. Added at most two transient 429/5xx retries per cell, sharing the original 60-second request deadline and reserving conservative token allowance for failed requests; SDK retries remain disabled. Malformed/overlong tool arguments now return a fixed repairable tool error while consuming the existing tool-call budget. Permissions and overall limits are unchanged. Covered with real SDK response types and mocked provider transport; the real Harbor tool path also passed. No live-provider reliability claim is made.
| remaining(1020 if cell["arm"] in ARMS else 150), | ||
| ) | ||
| rows[index] = native_check(private_output / "jobs" / str(cell["cell_id"]), cell) | ||
| except Exception as exc: | ||
| (private_output / f"{cell['cell_id']}.error").write_text(repr(exc)) | ||
| rows[index] = { | ||
| **cell, | ||
| "status": "incomplete", | ||
| **partial_observation(private_output / "jobs" / str(cell["cell_id"]), cell), | ||
| } | ||
| report["reason"] = "invalid-native-evidence" |
There was a problem hiding this comment.
TimeoutError from remaining(), which is evaluated inside the execute(...) arguments, and per-cell subprocess.TimeoutExpired are both caught here and recorded as reason = "invalid-native-evidence". If that happens on the last cell, the summary reports invalid evidence instead of deadline or a timeout. Could timeouts be handled separately before the general except Exception?
There was a problem hiding this comment.
Fixed in 5630d31. remaining() TimeoutError and per-cell subprocess.TimeoutExpired now have a separate handler before the generic native-evidence exception path. It checkpoints the incomplete cell with reason deadline and stops, including on the final cell. Added regressions for both timeout sources and final-cell behavior.
| "stdout": result.stdout, | ||
| "stderr": result.stderr, | ||
| "return_code": result.returncode, | ||
| "semantic_pass": result.returncode == 0 and actual == expected, |
There was a problem hiding this comment.
Nit: actual == expected uses Python equality, so [True] matches [1]. The live verifier compares canonical JSON for exactly this reason ("not Python's permissive True == 1 equality"). Worth keeping the "sound" fixture consistent.
There was a problem hiding this comment.
Fixed in 5630d31. The sound fixture now uses canonical JSON equality, consistent with the live verifier. Added bool/float-versus-integer regressions and a native boolean-output control; it is rejected by the sound grader and still rewarded by the deliberately weak grader.
| Run the same panel locally with Docker running and the locked development | ||
| environment installed. Choose a new output directory for every attempt: | ||
|
|
||
| ```bash | ||
| uv sync --locked | ||
| uv run --locked python tools/collect_verifier_cheat_evidence.py \ | ||
| --output "$PWD/.eval-author/verifier-cheat-evidence/run-1" | ||
| ``` |
There was a problem hiding this comment.
Nit: on macOS with Docker 29.4.3, Harbor 0.20.0 rejects these tasks with network_mode='no-network' is not supported by EnvironmentType.DOCKER, because egress control needs Linux or kernel support. Please document that runtime requirement. The guided live experiment section has the same issue.
There was a problem hiding this comment.
Documented in 5630d31 for both panels: Harbor's no-network policy requires a Docker runtime/host with the necessary egress enforcement support; a running Docker daemon alone is insufficient. Included the unsupported-network-mode diagnostic and guidance to switch or upgrade the runtime, not disable isolation. Local native runs passed on the tested Docker 29.7.2 setup; this is not a claim that every macOS runtime supports it.
| @pytest.mark.parametrize( | ||
| "submission,reward", | ||
| [("print(7)", 1), ("raise SystemExit(0)", 0), ("raise SystemExit(1)", 0), ("print('ALL TESTS PASSED')", 0)], | ||
| ) | ||
| def test_trusted_driver_rejects_no_work_submission_without_infrastructure_error(submission, reward): | ||
| # Local subprocess contract test, not hostile-code or native-provider isolation proof. | ||
| source = f""" | ||
| import json, subprocess, sys | ||
| submitted = subprocess.run( | ||
| [sys.executable, "-I", "-B", "-c", {submission!r}], | ||
| capture_output=True, text=True, timeout=2, | ||
| ) | ||
| correct = submitted.returncode == 0 and submitted.stdout.strip() == "7" | ||
| print(json.dumps({{"status": "result", "value": int(correct)}})) | ||
| """ | ||
| assert run(source) == {"status": "result", "value": reward} |
There was a problem hiding this comment.
Nit: the pass/fail logic (returncode == 0 and stdout == "7") is written inside the test's own driver string, so this only checks that run_helper passes through a value the test computed. test_transport_does_not_grade_values already covers that. The new cases added to test_helper_faults_raise_instead_of_returning_rejection are the useful ones; suggest dropping this test.
There was a problem hiding this comment.
Removed in 5630d31. The inline toy driver only retested transport of a value computed by the test. Kept the transport contract and useful helper-fault regressions; actual grader behavior is exercised by the native verifier panel.
| def test_construction_routes_require_shared_cheat_screening() -> None: | ||
| reference = "task-validation.md#screen-simple-cheats" | ||
| routes = [ | ||
| _CORE_DIR / "references" / "construction-plan.md", | ||
| _CORE_DIR / "references" / "trace-input.md", | ||
| _CORE_DIR.parent / "eval-author-task-create" / "SKILL.md", | ||
| ] | ||
| for skill in ("eval-author-environment", "eval-author-trace-environment"): | ||
| routes.extend(_CORE_DIR.parent / skill / "references" / name for name in ("task-fidelity.md", "gym-output.md")) | ||
| for path in routes: | ||
| assert reference in path.read_text(encoding="utf-8"), path | ||
| text = (_CORE_DIR / "references" / "task-validation.md").read_text(encoding="utf-8") | ||
| screening = " ".join(text.split("## Screen simple cheats", 1)[1].split("\n## ", 1)[0].split()) | ||
| for required in ( | ||
| "successful early exit", | ||
| "all-failure stub", | ||
| "reference-as-comments", | ||
| "forged success", | ||
| "entry point → guards → invoked checks → reward writer", | ||
| "Proposal-only", | ||
| "every task", | ||
| "unproven", | ||
| "not a passing negative control", | ||
| "verifier-owned", | ||
| "valid neighbor", | ||
| ): | ||
| assert required.lower() in screening.lower() |
There was a problem hiding this comment.
Nit: this pins exact phrases in task-validation.md, so any rewording breaks it even when the routing behavior is unchanged. The link check on the route files is useful; the phrase list is not.
There was a problem hiding this comment.
Removed the exact prose phrase list in 5630d31. Kept the route-link checks and verification that the linked section anchor exists, so routing remains covered without pinning instructional wording.
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-24.04, macos-14, windows-2022] | ||
| os: [ubuntu-24.04, macos-15, windows-2022] |
There was a problem hiding this comment.
Nit: the macos-14 → macos-15 change isn't related to the anti-cheat work. Please split it out or mention it in the PR description.
There was a problem hiding this comment.
Kept the already-published macos-15 change and explicitly documented it in the PR description. It is an ancillary fix for macos-14 runner retirement/brownouts, not an anti-cheat requirement. Python remains 3.12 for that matrix. Keeping it avoids rewriting the published branch while satisfying the requested scope disclosure.
Signed-off-by: Andrew Suter-Morris <asutermorris@nvidia.com>
|
Addressed the 12 inline comments individually in 5630d31; the PR description now reflects the expanded 18-trial panel, privacy split, advisory live execution, ancillary macOS migration, and final validation. The final stable suite passed 3,262 tests (13 skipped); all 18 deterministic native trials and a fresh six-cell repair-control batch passed. No live model requests were made. For the two summary-only points in #164 (review):
The separately reported mainline context failure is addressed by #167, not this anti-cheat change. PR review state is unchanged; nothing has been merged. |
Summary
verifier-cheatsCI panel on same-repository PRs, main pushes, manual dispatch, and daily CI. Six scripted submissions run against sound, deliberately weak, and always-fail graders. Root-only hidden tests, timeout rejection, and type-preserving JSON comparison have native regression controls.semantic-regressionenvironment; manual main dispatch requiresrun_live=true. PRs only plan. The live job is advisory (continue-on-error: true), while summaries retain truthful incomplete statuses. No credential/environment rules or required merge checks are changed.task-search-platformsfrom retired/brownout-pronemacos-14to pinnedmacos-15; Python remains 3.12. This is not an anti-cheat prerequisite.Review fixes
Address hidden-case disclosure and public artifact exposure first, then candidate timeout/type handling, bounded provider retry/tool repair, final-cell deadline classification, and rejection of contradictory incomplete summaries. Remove the redundant toy-driver test and exact instructional-phrase assertions while retaining transport/fault and route-link coverage. Document Docker no-network enforcement requirements and artifact retention.
The deterministic schema is now
nemo.eval_author.verifier_cheat_evidence.v2, protocolharbor-verifier-cheats-v2. Its expanded counts are not directly comparable to the old nine-trial panel.Measured execution
Validation
Limits and cost
The deterministic controls and live experiment remain separate. Fixed-fixture sensitivity does not establish general reward-hacking resistance, authoring-agent compliance, screening of old tasks, or arbitrary-code sandbox security. Explicit adversarial prompting is not evidence of spontaneous cheating.
Default live batches contain eight model-backed cells (two tasks × two arms × two repetitions), each with a 48,000 observed-token cap, plus six credential-free controls. Automatic main-push and daily cadence is intentional; maintainers should review it against their inference budget. At most two transient 429/5xx retries per cell share the original request deadline and conservatively reserve token allowance for failed requests. Partial observed usage is not a complete billing ledger.
The 3,000-second runner deadline remains below the 60-minute CI job limit. Missing, interrupted, unhealthy, and invalid evidence stays incomplete. Runner loss can still prevent artifact publication. Docker must support Harbor's no-network enforcement; daemon availability alone is insufficient.
Trusted live error rates remain unmeasured because the behavioral checker supplies the finite-case reference label. Weak-grader disagreement is not universal correctness evidence. Requested model routes and observed revision hashes do not prove immutable provider backends; missing revisions stay unknown.
History comparison deliberately splits changes in lockfile, Docker version, task/prompt bindings, and per-cell provider identity. This limits longitudinal grouping but avoids pooling incompatible experiments. It is summary consistency checking, not independent native attestation.
The separately reported mainline SkillEvaluator context-budget failure is not caused or fixed by this PR; its follow-up is #167, kept separate from anti-cheat changes.