From 9a33c9a9791c7523870e299d17b3fd0fe5c9bce7 Mon Sep 17 00:00:00 2001 From: Dmitry Kropachev Date: Sun, 25 Oct 2026 21:28:49 -0400 Subject: [PATCH] Harden receipt resumption and validate installed workflows Validate retained receipt kinds and integer versions before resume accounting or Docker activity. Preserve malformed evidence and prevent additional trials. Use immutable local image IDs for reproducible Docker lifecycle tests with a fake reviewer and synthetic authentication. Exercise the installed CLI from unrelated directories and run the container checks in a separate CI job. Package validation, 83 offline tests and six real Docker cases pass without inference or calibration. --- .github/workflows/runtime.yml | 27 +++ README.md | 7 + .../code-review-c/references/onboarding.md | 5 +- .../code-review-c/references/operations.md | 3 + .../scripts/reviewlib/onboarding.py | 4 +- .../scripts/reviewlib/records.py | 3 +- scripts/test_runtime.py | 184 ++++++++++++++++++ tests/runtime/Dockerfile | 4 + tests/runtime/fake_codex.py | 42 ++++ tests/test_installation.py | 82 ++++++++ tests/test_onboarding.py | 32 ++- 11 files changed, 387 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/runtime.yml create mode 100644 scripts/test_runtime.py create mode 100644 tests/runtime/Dockerfile create mode 100644 tests/runtime/fake_codex.py create mode 100644 tests/test_installation.py diff --git a/.github/workflows/runtime.yml b/.github/workflows/runtime.yml new file mode 100644 index 0000000..49422e8 --- /dev/null +++ b/.github/workflows/runtime.yml @@ -0,0 +1,27 @@ +name: Docker lifecycle + +on: + push: + pull_request: + +permissions: + contents: read + +concurrency: + group: runtime-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + runtime: + name: Real Docker lifecycle + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 + with: + persist-credentials: false + - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6 + with: + python-version: '3.11' + - name: Exercise fake reviewer in real Docker + run: python scripts/test_runtime.py diff --git a/README.md b/README.md index 8bfa96b..c8dc6c1 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,13 @@ see only pinned source/evidence and temporary Codex authentication, keeping answ keys outside the reviewer filesystem. Three matched repetitions, held-out fixtures, critical-defect recall, precision and completion gates precede profile promotion. +On a POSIX host with Linux Docker, run `python3 scripts/test_runtime.py` for the +same lifecycle checks run by Docker CI. The tests build an image with a fake Codex +executable and synthetic authentication; they run no inference. They cover stdin, +read-only source isolation, output capture, retained failures, trial timeout and +interruption, and version-probe cleanup. The pinned Python base image may need to +be downloaded. These checks do not measure review quality or model capacity. + Keep review artifacts, campaign data, local profiles, credentials, private rules, caches and benchmark results outside the package and reviewed repository. Provenance for adapted review guidance is pinned in [PROVENANCE.json](PROVENANCE.json), with diff --git a/codex/skills/code-review-c/references/onboarding.md b/codex/skills/code-review-c/references/onboarding.md index 25d952a..9b7555c 100644 --- a/codex/skills/code-review-c/references/onboarding.md +++ b/codex/skills/code-review-c/references/onboarding.md @@ -61,8 +61,11 @@ keep all other evidence identical within each matched trial. ## Run isolated trials -Provide an immutable Docker image (`IMAGE@sha256:...`) containing the desired Codex +Provide an immutable Docker image (`IMAGE@sha256:...` or a local `sha256:IMAGE_ID`) containing the desired Codex version and its runtime dependencies, with `codex` on PATH and no custom entrypoint. +Local image IDs must already exist in the selected Docker engine; mutable tags +are rejected. The Docker lifecycle test builds its own fake reviewer and uses its +immutable local ID without a registry adapter or model calls. Provide a sanitized host config containing **only** `model` and `model_reasoning_effort`, matching the frozen host-selected identity. The harness does not invent candidate overrides. Provide the existing host's `auth.json`; it diff --git a/codex/skills/code-review-c/references/operations.md b/codex/skills/code-review-c/references/operations.md index 9a65393..e3d352a 100644 --- a/codex/skills/code-review-c/references/operations.md +++ b/codex/skills/code-review-c/references/operations.md @@ -6,6 +6,9 @@ The first-release support target is Linux with CPython 3.11–3.15, Git, and a C host providing native agents. GitHub PR pinning additionally requires authenticated `gh`. Container trials require a working Linux Docker engine and an immutable image containing the selected Codex version. Review and preview do not need Docker. +Use a registry reference pinned as `NAME@sha256:DIGEST`, or an immutable local +`sha256:IMAGE_ID` from `docker image inspect`. Local IDs are specific to the Docker +engine and must be loaded on another host before use; mutable tags are rejected. Other host platforms and Python versions have not been validated for this release. Install the `v0.1.0` tag as described in the repository README. Keep an old installed diff --git a/codex/skills/code-review-c/scripts/reviewlib/onboarding.py b/codex/skills/code-review-c/scripts/reviewlib/onboarding.py index b98e2f1..0849809 100644 --- a/codex/skills/code-review-c/scripts/reviewlib/onboarding.py +++ b/codex/skills/code-review-c/scripts/reviewlib/onboarding.py @@ -235,14 +235,14 @@ def run(root, image, host_config, auth, max_calls, max_wall_seconds, trial_secon """ root = external(root) c = read(root) - if not re.fullmatch(r"[\w./:-]+@sha256:[0-9a-f]{64}", image): + if not re.fullmatch(r"(?:[\w./:-]+@)?sha256:[0-9a-f]{64}", image): raise ValueError("trial image must be pinned by sha256 digest") limits = c["limits"] for key, value in (("max_calls", max_calls), ("max_wall_seconds", max_wall_seconds), ("trial_seconds", trial_seconds)): if type(value) is not int or value <= 0 or value > limits[key]: raise ValueError("explicit positive run limits must fit the frozen campaign") # A previously started trial still spends its call allowance after a crash. - attempts = [load(p) for p in (root / "attempts").glob("*/receipt.json")] + attempts = [load(p, "trial-receipt") for p in (root / "attempts").glob("*/receipt.json")] used_calls = sum(bool(r.get("launched")) for r in attempts) used_time = sum(max(r.get("reserved_seconds", trial_seconds), r.get("elapsed_seconds", trial_seconds)) if r.get("status") in {"started", "interrupted", "cleanup-failed"} diff --git a/codex/skills/code-review-c/scripts/reviewlib/records.py b/codex/skills/code-review-c/scripts/reviewlib/records.py index c84e436..112b707 100644 --- a/codex/skills/code-review-c/scripts/reviewlib/records.py +++ b/codex/skills/code-review-c/scripts/reviewlib/records.py @@ -19,7 +19,8 @@ def record(kind, **values): def load(path, kind=None): value = json.loads(Path(path).read_text()) - if kind and (not isinstance(value, dict) or value.get("kind") != kind or value.get("version") != VERSION): + if kind and (not isinstance(value, dict) or value.get("kind") != kind + or type(value.get("version")) is not int or value["version"] != VERSION): raise ValueError(f"expected {kind} version {VERSION}: {path}") return value diff --git a/scripts/test_runtime.py b/scripts/test_runtime.py new file mode 100644 index 0000000..24d2928 --- /dev/null +++ b/scripts/test_runtime.py @@ -0,0 +1,184 @@ +#!/usr/bin/env python3 +"""Exercise the real Docker/CLI lifecycle with a fake reviewer; never calls a model.""" +import json +import os +import re +import signal +import subprocess +import sys +import tempfile +import time +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT / "tests")) +from support import GitCase, SKILL +from reviewlib import onboarding +from reviewlib.records import digest, load, validate_output + + +class RuntimeTests(GitCase): + images = {} + + def setUp(self): + super().setUp() + self.campaign_spec() + self.croot = self.root / "campaign" + onboarding.freeze(self.root / "spec.json", self.croot) + self.config = self.root / "host.toml" + self.config.write_text('model = "synthetic-exact-model"\nmodel_reasoning_effort = "high"\n') + self.auth = self.root / "auth.json" + self.auth.write_text('{"synthetic": true, "mode": "finished"}') + self.auth.chmod(0o600) + self.trial = self.croot / "attempts/train0--focused--0" + self.containers = set() + + def tearDown(self): + try: + # Clean only this test's named containers, after outcome assertions. + for trial in (self.croot / "attempts").glob("*"): + self.containers.add("crc-" + digest(str(trial))[:20]) + for name in sorted(self.containers): + subprocess.run(["docker", "rm", "-f", name], capture_output=True, timeout=15, check=True) + finally: + super().tearDown() + + def command(self, image="finished", seconds=10): + return [sys.executable, str(SKILL / "scripts/review.py"), "onboard", "run", + "--campaign", str(self.croot), "--image", self.images[image], + "--host-config", str(self.config), "--auth", str(self.auth), + "--max-calls", "1", "--max-wall-seconds", "60", "--trial-seconds", str(seconds)] + + def container_absent(self, name): + # A daemon error must not masquerade as successful container removal. + names = subprocess.check_output(["docker", "ps", "-a", "--format", "{{.Names}}"], + text=True, timeout=15).splitlines() + self.assertNotIn(name, names, "Container survived production cleanup") + + def assert_trial_removed(self, trial=None): + self.container_absent("crc-" + digest(str(trial or self.trial))[:20]) + + def run_mode(self, mode, seconds=10): + self.auth.write_text(json.dumps({"synthetic": True, "mode": mode})) + result = subprocess.run(self.command(seconds=seconds), capture_output=True, text=True, timeout=60) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertEqual(json.loads(result.stdout)["launched"], 1) + receipt = load(self.trial / "receipt.json", "trial-receipt") + self.assertEqual(receipt["status"], mode) + self.assertTrue(receipt["launched"]) + self.assert_trial_removed() + return receipt + + def test_success_captures_stdin_output_usage_and_removes_container(self): + receipt = self.run_mode("finished") + self.assertEqual(receipt["tokens"], 120) + self.assertEqual(receipt["trace_hash"], digest((self.trial / "trace.jsonl").read_bytes())) + self.assertEqual(receipt["result_hash"], digest((self.trial / "result.json").read_bytes())) + result = load(self.trial / "result.json", "calibration-result") + validate_output(result, "calibration-result") + self.assertEqual(result["status"], "complete") + self.assertTrue(result["inspected_hunks"]) + + def test_process_failure_is_retained_on_resume(self): + self.run_mode("failed") + before = (self.trial / "receipt.json").read_bytes() + result = subprocess.run(self.command(), capture_output=True, text=True, timeout=60) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertEqual(json.loads(result.stdout)["launched"], 1) + self.assertEqual((self.trial / "receipt.json").read_bytes(), before) + second = self.croot / "attempts/train0--focused--1" + self.assertEqual(load(second / "receipt.json", "trial-receipt")["status"], "failed") + self.assert_trial_removed(second) + + def test_timeout_retains_incomplete_evidence_and_removes_container(self): + receipt = self.run_mode("timeout", seconds=3) + self.assertIsNone(receipt["result_hash"]) + self.assertIsNone(receipt["tokens"]) + self.assertTrue(receipt["trace_hash"]) + + def interrupted_run(self, image, ready): + stderr_path = self.root / "runner-stderr.txt" + with (self.root / "runner-stdout.txt").open("wb") as stdout, stderr_path.open("wb") as stderr: + with subprocess.Popen(self.command(image, seconds=30), stdout=stdout, stderr=stderr) as process: + try: + deadline = time.monotonic() + 20 + while time.monotonic() < deadline: + if ready(stderr_path): + break + if process.poll() is not None: + self.fail("Runner exited before fixture started: " + stderr_path.read_text()) + time.sleep(0.05) + else: + self.fail("Fixture did not start: " + stderr_path.read_text()) + process.send_signal(signal.SIGINT) + process.wait(timeout=25) + self.assertNotEqual(process.returncode, 0) + finally: + if process.poll() is None: + process.kill() + process.wait(timeout=10) + return stderr_path.read_text() + + def test_interrupt_retains_attempt_and_removes_container(self): + self.auth.write_text('{"synthetic": true, "mode": "interrupted"}') + trace = self.trial / "trace.jsonl" + self.interrupted_run("finished", lambda _: trace.exists() and "runtime.fake.started" in trace.read_text()) + receipt = load(self.trial / "receipt.json", "trial-receipt") + self.assertEqual(receipt["status"], "interrupted") + self.assertTrue(receipt["launched"]) + self.assertGreater(receipt["elapsed_seconds"], 0) + self.assertIsNone(receipt["result_hash"]) + self.assert_trial_removed() + + def probe_name(self, stderr): + match = re.search(r"Version probe container: (crc-version-[0-9a-f]{32})", stderr) + self.assertIsNotNone(match, stderr) + name = match.group(1) + self.containers.add(name) + return name + + def test_version_probe_timeout_removes_container_without_starting_trial(self): + result = subprocess.run(self.command("timeout"), capture_output=True, text=True, timeout=60) + self.assertNotEqual(result.returncode, 0) + name = self.probe_name(result.stderr) + self.assertIn("timed out", result.stderr) + self.assertFalse((self.croot / "attempts").exists()) + self.container_absent(name) + + def test_version_probe_interrupt_removes_container_without_starting_trial(self): + def running(path): + stderr = path.read_text() + if "Version probe container:" not in stderr: + return False + name = self.probe_name(stderr) + result = subprocess.run(["docker", "inspect", "--format", "{{.State.Running}}", name], + capture_output=True, text=True, timeout=15) + return result.returncode == 0 and result.stdout.strip() == "true" + + stderr = self.interrupted_run("interrupted", running) + self.assertFalse((self.croot / "attempts").exists()) + self.container_absent(self.probe_name(stderr)) + + +def main(): + if os.name != "posix": + raise SystemExit("Docker lifecycle tests require a POSIX host") + engine = subprocess.check_output(["docker", "info", "--format", "{{.OSType}}"], text=True, timeout=30).strip() + if engine != "linux": + raise SystemExit("Docker lifecycle tests require Linux containers") + with tempfile.TemporaryDirectory(prefix="crc-runtime-build-") as temp: + for mode in ("finished", "timeout", "interrupted"): + iid = Path(temp) / mode + subprocess.run(["docker", "build", "--iidfile", str(iid), "--build-arg", "PROBE_MODE=" + mode, + str(ROOT / "tests/runtime")], check=True, timeout=300) + image = iid.read_text().strip() + if not re.fullmatch(r"sha256:[0-9a-f]{64}", image): + raise ValueError("Docker did not return an immutable image ID") + RuntimeTests.images[mode] = image + suite = unittest.defaultTestLoader.loadTestsFromTestCase(RuntimeTests) + return not unittest.TextTestRunner(verbosity=2).run(suite).wasSuccessful() + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/runtime/Dockerfile b/tests/runtime/Dockerfile new file mode 100644 index 0000000..6651887 --- /dev/null +++ b/tests/runtime/Dockerfile @@ -0,0 +1,4 @@ +FROM python:3.11-slim@sha256:b7a9e5f6a93d6d4a31f129bbd4bdee69efccb6ded0fb230d78aafaf37af98377 +ARG PROBE_MODE=finished +ENV CRC_TEST_PROBE_MODE=${PROBE_MODE} +COPY --chmod=0755 fake_codex.py /usr/local/bin/codex diff --git a/tests/runtime/fake_codex.py b/tests/runtime/fake_codex.py new file mode 100644 index 0000000..2ee0d7f --- /dev/null +++ b/tests/runtime/fake_codex.py @@ -0,0 +1,42 @@ +#!/usr/bin/env python3 +"""Docker protocol fixture: synthetic credentials, no inference or network calls.""" +import errno +import json +import os +import re +import sys +import time +from pathlib import Path + +if sys.argv[1:] == ["--version"]: + if os.environ["CRC_TEST_PROBE_MODE"] != "finished": + time.sleep(120) + print("codex-cli synthetic") + sys.exit(0) + +assert "--ephemeral" in sys.argv and "read-only" in sys.argv +auth = json.loads(Path("/codex-home/auth.json").read_text()) +assert auth["synthetic"] is True +assert Path("/input/schema.json").is_file() +assert {p.name for p in Path("/workspace").iterdir()} == {"base", "head"} +assert not Path("/grading/keys.json").exists() +try: + Path("/workspace/head/a.py").write_text("mutated") +except OSError as error: + assert error.errno == errno.EROFS, "Pinned workspace must use a read-only mount" +else: + raise AssertionError("Pinned workspace is writable") + +prompt = sys.stdin.read() +assert "OWNED CHANGED HUNKS" in prompt, "Docker must forward the task prompt on stdin" +print(json.dumps({"type": "runtime.fake.started", "synthetic": True}), flush=True) +if auth["mode"] in {"timeout", "interrupted"}: + time.sleep(120) +if auth["mode"] == "failed": + sys.exit(23) + +result = {"kind": "calibration-result", "version": 1, "status": "complete", + "inspected_hunks": re.findall(r"^HUNK ([0-9a-f]+)$", prompt, flags=re.M), + "extra_visible_bytes": 0, "findings": [], "gaps": []} +Path(sys.argv[sys.argv.index("-o") + 1]).write_text(json.dumps(result)) +print(json.dumps({"type": "turn.completed", "usage": {"input_tokens": 100, "output_tokens": 20}}), flush=True) diff --git a/tests/test_installation.py b/tests/test_installation.py new file mode 100644 index 0000000..69464a8 --- /dev/null +++ b/tests/test_installation.py @@ -0,0 +1,82 @@ +"""Fresh-install CLI integration with synthetic receipts, without inference.""" +import json +import shutil +import subprocess +import sys + +from support import GitCase, ROOT, SKILL, receipts +from reviewlib.records import save + + +class InstallationTests(GitCase): + def exercise_installation(self, plugin): + install = self.root / "fresh install" + if plugin: + skill = install / "codex/skills/code-review-c" + for relative in (".codex-plugin/plugin.json", ".agents/plugins/marketplace.json"): + target = install / relative + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(ROOT / relative, target) + else: + skill = install / "skills/code-review-c" + shutil.copytree(SKILL, skill, ignore=shutil.ignore_patterns("__pycache__")) + helper = skill / "scripts/review.py" + cwd = self.root / "unrelated working directory" + cwd.mkdir() + + def cli(*args, success=True): + result = subprocess.run([sys.executable, str(helper), *map(str, args)], cwd=cwd, + capture_output=True, text=True, timeout=30) + if success: + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + return json.loads(result.stdout) + self.assertNotEqual(result.returncode, 0) + return json.loads(result.stderr) + + bundle, plan_path = self.root / "bundle", self.root / "plan.json" + cli("inventory", "--repo", self.repo, "--base", self.base, "--head", self.head, "--out", bundle) + plan = cli("plan", "--bundle", bundle, "--out", plan_path) + common = ("--bundle", bundle, "--plan", plan_path) + self.assertTrue(cli("validate", *common)["valid"]) + results = self.root / "results" + results.mkdir() + schedule_args = (*common, "--results", results, "--capacity", "2") + initial = cli("next", *schedule_args) + self.assertEqual(initial["gate"], "pending") + self.assertEqual(len(initial["launch"]), 2) + self.assertTrue(initial["queued"]) + + for task, receipt in zip(plan["tasks"], receipts(plan)): + prompt = self.root / (task["id"] + ".txt") + cli("prompt", *common, "--task", task["id"], "--out", prompt) + self.assertIn("OWNED CHANGED HUNKS", prompt.read_text()) + self.assertTrue(set(task["hunks"])) + for hunk in task["hunks"]: + self.assertIn(hunk, prompt.read_text()) + save(results / (task["id"] + ".json"), receipt) + report = cli("report", *common, "--results", results) + self.assertEqual(report["gate"], "clean") + self.assertEqual(report["coverage_gaps"], []) + self.assertEqual(report["findings"], []) + + # Resume retained work, but reject receipts for stale task inputs. + receipt_path = results / (plan["tasks"][0]["id"] + ".json") + original = json.loads(receipt_path.read_text()) + receipt_path.unlink() + resumed = cli("next", *schedule_args) + self.assertEqual(resumed["launch"], [original["task_id"]]) + self.assertEqual(resumed["gate"], "pending") + save(receipt_path, {**original, "input_hash": "0" * 64}) + error = cli("report", *common, "--results", results, success=False) + self.assertIn("stale task receipt", error["error"]) + save(receipt_path, original) + self.assertEqual(cli("report", *common, "--results", results)["gate"], "clean") + + for forbidden in (skill / "state", skill / "profiles", self.repo / "results"): + self.assertFalse(forbidden.exists()) + + def test_copied_skill_runs_from_unrelated_directory(self): + self.exercise_installation(plugin=False) + + def test_plugin_layout_runs_from_unrelated_directory(self): + self.exercise_installation(plugin=True) diff --git a/tests/test_onboarding.py b/tests/test_onboarding.py index b47091b..1db31c8 100644 --- a/tests/test_onboarding.py +++ b/tests/test_onboarding.py @@ -472,7 +472,7 @@ def test_combined_layout_cannot_be_promoted(self): with self.assertRaisesRegex(ValueError, "combined controls"): onboarding.promote(self.croot, "combined", self.root / "profiles") - def fake_runner(self, timeout=False, interrupt=False, expected_launches=1): + def fake_runner(self, timeout=False, interrupt=False, expected_launches=1, image=None): config = self.root / "host.toml" config.write_text('model = "synthetic-exact-model"\nmodel_reasoning_effort = "high"\n') auth = self.root / "auth.json" @@ -507,7 +507,7 @@ def run(command, **kwargs): patch.object(onboarding.subprocess, "check_output", return_value="codex-cli synthetic\n"), \ patch.object(onboarding.subprocess, "run", side_effect=run): try: - result = onboarding.run(self.croot, "example/codex@sha256:" + "a" * 64, config, auth, 1, 10, 5) + result = onboarding.run(self.croot, image or "example/codex@sha256:" + "a" * 64, config, auth, 1, 10, 5) finally: self.assertEqual(len(cleanups), expected_launches + 1) self.assertEqual(result["launched"], expected_launches) @@ -522,6 +522,34 @@ def test_runner_captures_trace_and_enforces_call_limit_without_models(self): self.assertTrue(receipt["result_hash"]) self.assertEqual(receipt["status"], "finished") + def test_runner_accepts_immutable_local_image_id(self): + self.freeze() + receipt = self.fake_runner(image="sha256:" + "b" * 64) + self.assertEqual(receipt["status"], "finished") + + def test_resume_rejects_invalid_receipts_before_docker_or_new_attempts(self): + self.freeze() + receipt = self.fake_runner() + path = self.croot / "attempts/train0--focused--0/receipt.json" + cases = [("kind", "task-result"), ("version", 2), ("version", True), ("version", 1.0)] + for field, value in cases: + with self.subTest(field=field, value=value): + save(path, {**receipt, field: value}) + before = path.read_bytes() + with patch.object(onboarding.subprocess, "check_output", return_value="codex-cli synthetic\n") as probe, \ + patch.object(onboarding.subprocess, "run", side_effect=AssertionError("Invalid receipt reached Docker")) as runner: + with self.assertRaisesRegex(ValueError, "expected trial-receipt version 1"): + onboarding.run(self.croot, "example/codex@sha256:" + "b" * 64, + self.root / "host.toml", self.root / "auth.json", 1, 10, 5) + probe.assert_not_called() + runner.assert_not_called() + self.assertEqual(path.read_bytes(), before) + self.assertEqual(list((self.croot / "attempts").iterdir()), [path.parent]) + save(path, receipt) + self.fake_runner() + self.assertEqual(load(path), receipt) + self.assertTrue((self.croot / "attempts/train0--focused--1/receipt.json").is_file()) + def test_runner_timeout_retains_failed_attempt(self): self.freeze() receipt = self.fake_runner(timeout=True)