diff --git a/.github/workflows/guardian.yml b/.github/workflows/guardian.yml index a3c09250..8a8e4e82 100644 --- a/.github/workflows/guardian.yml +++ b/.github/workflows/guardian.yml @@ -53,14 +53,31 @@ jobs: BASE_BRANCH=$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json baseRefName --jq '.baseRefName') echo "BASE_BRANCH=$BASE_BRANCH" >> $GITHUB_ENV - - name: Checkout PR branch + # The PR is data, never code (#730). Everything that runs — uv sync, the + # guardian scripts, cgis itself — comes from the default branch checked + # out here; the PR lands in pr/ and is only diffed, read and parsed. + # This step and the next keep the job's provider keys out of reach of + # whoever wrote the PR. + - name: Checkout trusted tooling (default branch) + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + + - name: Checkout PR branch as data uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: # PR_NUMBER validated as digits-only in previous step ref: refs/pull/${{ env.PR_NUMBER }}/head + path: pr fetch-depth: 0 + persist-credentials: false + # Anonymous, like the checkout above it: fine for this public repository. + # A copy of this job for a private one must pass a token here, or the + # fetch fails the job. - name: Fetch base branch for diff + working-directory: pr run: git fetch origin "$BASE_BRANCH" - name: Install uv @@ -70,8 +87,11 @@ jobs: run: uv sync --frozen --extra guardian - name: Build code graph - run: uv run --frozen cgis ingest ./src --output graph.db + run: uv run --frozen cgis ingest pr/src --output graph.db + # Fetches anonymously: the root checkout keeps no credentials (#730). + # Fine while this repository is public; on a private one this restore + # fails into `|| true` and save-metrics would overwrite the history. - name: Restore metrics from data branch run: | git fetch origin data/guardian-metrics 2>/dev/null && \ @@ -94,12 +114,10 @@ jobs: GUARDIAN_FEATURES: ${{ vars.GUARDIAN_FEATURES }} GUARDIAN_SKEPTIC: ${{ vars.GUARDIAN_SKEPTIC }} GUARDIAN_SKEPTIC_MODEL: ${{ vars.GUARDIAN_SKEPTIC_MODEL }} - # Static-evidence pass for the skeptic (#401). Wired but not set: - # the repository variable is deliberately absent, so enabling it is a - # decision made in repo settings rather than a code change nobody - # reviews. An unset variable expands to "", which the flag reads as - # off — only the literal "1" enables it. - GUARDIAN_EVIDENCE: ${{ vars.GUARDIAN_EVIDENCE }} + # GUARDIAN_EVIDENCE (#401) is deliberately not passed: the evidence + # pass runs `uv run mypy/ruff` inside the reviewed checkout, which is + # the PR, while this step holds the provider keys (#730). + # guardian_review.py refuses that combination with --repo-root. run: | uv run --frozen python scripts/guardian_review.py \ --output guardian_report.md \ @@ -108,6 +126,7 @@ jobs: --metrics guardian_metrics.jsonl \ --record-finder guardian_finder.json \ --inline \ + --repo-root pr \ --base-branch "$BASE_BRANCH" - name: Upload metrics artifact diff --git a/scripts/guardian_bench.py b/scripts/guardian_bench.py index 36c3d928..12f17ceb 100644 --- a/scripts/guardian_bench.py +++ b/scripts/guardian_bench.py @@ -146,9 +146,11 @@ async def _run_one( # hardcode None on the reasoning that a recording is not a checkout — # true, and beside the point: the commit is named by the fixture, so a # worktree gets one, which is exactly what `guardian_replay_skeptic` - # does. Production runs with GUARDIAN_EVIDENCE=1, so judging without it - # measured a configuration nobody runs and quietly attributed the - # difference to whatever else the arm changed (#246). + # does. Production ran with GUARDIAN_EVIDENCE=1 at the time, so judging + # without it measured a configuration nobody ran and quietly attributed + # the difference to whatever else the arm changed (#246). Since #730 + # guardian.yml runs no evidence pass (the reviewed checkout is the PR); + # the bench keeps collecting it so its numbers stay comparable. evidence = await asyncio.to_thread( _evidence_at_head, truth.head, recording.diff, _REPO_ROOT ) diff --git a/scripts/guardian_review.py b/scripts/guardian_review.py index 3fa01737..450d9f06 100644 --- a/scripts/guardian_review.py +++ b/scripts/guardian_review.py @@ -3,12 +3,14 @@ import argparse import asyncio import os +from collections.abc import Mapping from pathlib import Path import structlog import cgis from cgis.guardian.collector import ContextCollector, parse_features +from cgis.guardian.evidence import EVIDENCE_FLAG from cgis.guardian.metrics import reject_metrics_path from cgis.guardian.review_fingerprint import ( compute_fingerprint, @@ -31,6 +33,29 @@ def _append_github_output(path: str, posted_inline: bool) -> None: fh.write(f"posted_inline={'true' if posted_inline else 'false'}\n") +def reviewed_root(requested: Path | None, project_root: Path, env: Mapping[str, str]) -> Path: + """The checkout to review: `--repo-root`, or this repository when it is not given. + + A foreign checkout is data only (#730): the evidence pass would run + `uv run mypy/ruff` inside it, which executes code and config the PR's author + controls while the job holds provider keys, so that combination is refused. + """ + this_repository = project_root.resolve() + if requested is None: + return this_repository + root = requested.resolve() + if not root.is_dir(): + _msg = f"--repo-root is not a directory: {requested}" + raise ValueError(_msg) + if root != this_repository and (env.get(EVIDENCE_FLAG) or "").strip() == "1": + _msg = ( + f"{EVIDENCE_FLAG}=1 runs the reviewed checkout's own toolchain, and {requested} " + f"is not this repository; unset it for a --repo-root review (#730)." + ) + raise ValueError(_msg) + return root + + async def main() -> None: """Run Guardian review and write the result to stdout or a file.""" parser = argparse.ArgumentParser(description="Run CGIS Guardian AI code review.") @@ -69,6 +94,15 @@ async def main() -> None: default=None, help="Write the finder pass (findings + diff) to this .json for offline re-scoring.", ) + parser.add_argument( + "--repo-root", + type=Path, + default=None, + help=( + "The checkout to review (default: this repository). guardian.yml passes the PR " + "checkout here, so the PR is read and parsed but none of its code runs (#730)." + ), + ) parser.add_argument( "--inline", action="store_true", @@ -85,6 +119,9 @@ async def main() -> None: _msg = f"Refusing metrics path '{args.metrics}': {refusal}." raise ValueError(_msg) + project_root = Path(__file__).parent.parent.absolute() + repo_root = reviewed_root(args.repo_root, project_root, os.environ) + provider, model = build_provider(os.environ) # Was an isinstance sniff that returned "gemini" for an Ollama provider, # which then picked the wrong default skeptic. The provider knows its own @@ -93,14 +130,13 @@ async def main() -> None: skeptic = build_skeptic_provider(os.environ, primary=primary) features = parse_features(os.environ.get("GUARDIAN_FEATURES", "")) inline_repo = os.environ.get("GITHUB_REPOSITORY") if args.inline else None - project_root = Path(__file__).parent.parent.absolute() # Computed here rather than inside runner.py: nothing in the review closure # may import the fingerprint module, or the hasher enters its own hashed set # (#375 §4.2). project_root is this repository, not the reviewed checkout. active = resolve_active_providers(provider.name, skeptic[0].name if skeptic else None) review_fingerprint = compute_fingerprint(disk_reader(project_root), active) collector = ContextCollector( - project_root=project_root, db_path=args.db, base_branch=args.base_branch, features=features + project_root=repo_root, db_path=args.db, base_branch=args.base_branch, features=features ) log.info("Running guardian review...", model=model) report, posted_inline = await run_guardian( diff --git a/scripts/skeptic_arms.py b/scripts/skeptic_arms.py index 315ded12..4dbfd2c2 100644 --- a/scripts/skeptic_arms.py +++ b/scripts/skeptic_arms.py @@ -23,12 +23,12 @@ The original single-orientation design cannot tell those apart. **Evidence is collected, not skipped.** `guardian_bench`'s replay path hardcodes -`evidence=None`, which would measure a configuration production does not run -(`GUARDIAN_EVIDENCE=1`). Here it is collected once per PR in a worktree at the -fixture head — the same mechanism `guardian_replay_skeptic` uses. Four of the six -fixtures touch Python and yield evidence, covering 96 of 135 findings; the other -two touch none, so their findings are judged with `evidence=None` because that is -also what production would do for them. +`evidence=None`, which would have measured a configuration production did not +run (`GUARDIAN_EVIDENCE=1`; production has run without it since #730). Here it +is collected once per PR in a worktree at the fixture head — the same mechanism +`guardian_replay_skeptic` uses. Four of the six fixtures touch Python and yield +evidence, covering 96 of 135 findings; the other two touch none, so their +findings are judged with `evidence=None`, as production judged such PRs then. No finder call is made. The cost is one skeptic call per finding per arm. """ diff --git a/src/cgis/guardian/collector.py b/src/cgis/guardian/collector.py index 6445eca0..305ce0cc 100644 --- a/src/cgis/guardian/collector.py +++ b/src/cgis/guardian/collector.py @@ -67,6 +67,24 @@ def features_setting(env: Mapping[str, str]) -> tuple[frozenset[str], FeaturesSo return parse_features(raw), "env" +def contained(root: Path, relative_path: str) -> Path | None: + """`root / relative_path`, resolved, when it lies inside `root`; else None. + + The reviewed checkout is a PR, and a PR is data its author controls (#730). + A symlink or a `..` is data that reads somewhere else: a CONTRIBUTING.md + linked to /proc/self/environ would put the job's provider keys in the + prompt, and from there in a public review comment. `cgis ingest` refuses + such links the same way. A link that stays inside the checkout is allowed. + """ + try: + resolved = (root / relative_path).resolve() + if resolved.is_relative_to(root.resolve()): + return resolved + except (OSError, RuntimeError, ValueError): # a loop of links, or a null byte + return None + return None + + class SourceCollector: """What the PR changed and what its files say: git and the working tree.""" @@ -131,19 +149,20 @@ def get_changed_source_files(self) -> list[str]: return [p for p in result.stdout.splitlines() if is_supported(p)] def read_file(self, relative_path: str) -> str: - """Reads a file from the project root; returns "" when it does not exist. + """Reads a file from the project root; "" when it is missing or resolves outside it. An empty string (not an error marker) lets the prompt builder omit the corresponding section entirely — a repo without CONTRIBUTING.md or docs/ontology/ should review cleanly, not get "Error: File ..." injected as if it were the standards/ontology text. `is_file()` (not `exists()`) so a path that resolves to a directory degrades to "" instead of raising - IsADirectoryError on read. + IsADirectoryError on read. Undecodable bytes are replaced: the file is the + PR's, and one bad byte must not end the review. """ - file_path = self.project_root / relative_path - if not file_path.is_file(): + file_path = contained(self.project_root, relative_path) + if file_path is None or not file_path.is_file(): return "" - return file_path.read_text(encoding="utf-8") + return file_path.read_text(encoding="utf-8", errors="replace") def collect_full_files(self, files: list[str] | None = None) -> str: """Full HEAD text of given (default: changed) source files, smallest-first. @@ -156,10 +175,13 @@ def collect_full_files(self, files: list[str] | None = None) -> str: sized: list[tuple[int, str, str]] = [] omitted: list[str] = [] for rel_path in changed: - path = self.project_root / rel_path - if not path.exists(): # deleted in this PR — nothing to show at HEAD + path = contained(self.project_root, rel_path) + if path is None: + omitted.append(f"file omitted: links outside the checkout ({rel_path})") + continue + if not path.is_file(): # deleted in this PR, or not a file — nothing to show continue - text = path.read_text(encoding="utf-8") + text = path.read_text(encoding="utf-8", errors="replace") if len(text.splitlines()) > _MAX_FILE_LINES: omitted.append(f"file omitted: too large ({rel_path})") continue @@ -187,8 +209,9 @@ def __init__(self, project_root: Path, db_path: Path | None, source_root: str = """Set the graph DB, the ingest root its FQNs are relative to, and the project root. source_root must match the ingest root used to build the graph DB (CI - runs `cgis ingest ./src`): changed-file paths are stripped of it before - lookup. project_root locates docs/ontology/patterns.yaml for drift. + runs `cgis ingest pr/src` with project_root `pr`): changed-file paths + are stripped of it before lookup. project_root locates + docs/ontology/patterns.yaml for drift. """ self.project_root = project_root self.db_path = db_path @@ -243,8 +266,8 @@ def collect_drift(self) -> str: """ if not self.has_db(): return "" - patterns = self.project_root / "docs" / "ontology" / "patterns.yaml" - if not patterns.exists(): + patterns = contained(self.project_root, "docs/ontology/patterns.yaml") + if patterns is None or not patterns.exists(): return "" try: scorer = DriftScorer(str(patterns)) diff --git a/src/cgis/guardian/evidence.py b/src/cgis/guardian/evidence.py index a1770ed6..8d6873f0 100644 --- a/src/cgis/guardian/evidence.py +++ b/src/cgis/guardian/evidence.py @@ -10,12 +10,14 @@ This module runs that command. It does not lower the bar; it makes the bar reachable. -Scope, stated rather than implied: the verbs are Python-only. `guardian.yml` and -`guardian_bench` review this repository, where `uv` and the checkers are -installed; `guardian_martian` reviews five foreign repositories whose toolchains -are not ours, and there this returns None and the skeptic behaves exactly as it -does today. `collect_evidence` is the seam a TypeScript implementation would -extend, not replace. +Scope, stated rather than implied: the verbs are Python-only. `guardian_bench` +reviews this repository, where `uv` and the checkers are installed; +`guardian_martian` reviews five foreign repositories whose toolchains are not +ours, and there this returns None and the skeptic behaves exactly as it does +today. `guardian.yml` never runs it: there the reviewed checkout is the PR, and +running its checkers would run its author's code beside the provider keys +(#730), so `guardian_review.py --repo-root` refuses the flag. `collect_evidence` +is the seam a TypeScript implementation would extend, not replace. Every failure is absence, never a verdict. A missing toolchain, a crashing checker, a timeout and a path outside the repository all produce None. Reporting diff --git a/tests/unit/test_guardian_collector.py b/tests/unit/test_guardian_collector.py index 794bd97f..cc158927 100644 --- a/tests/unit/test_guardian_collector.py +++ b/tests/unit/test_guardian_collector.py @@ -8,7 +8,7 @@ from cgis.core.models import Edge, EdgeType, Node, NodeNamespace, NodeType from cgis.guardian.chunker import Chunk -from cgis.guardian.collector import ContextCollector, parse_features +from cgis.guardian.collector import ContextCollector, contained, parse_features from cgis.languages import extractors from cgis.pipeline import IngestionPipeline from cgis.storage.sqlite_store import SQLiteStore @@ -527,3 +527,93 @@ def test_chunked_path_keeps_ts_files(tmp_path: Path) -> None: assert "app.handler" in context["graph_context"] assert "```ts\n" in context["full_files"] assert "README.md" not in context["full_files"] + + +# A PR is data (#730), and a symlink is data that reads somewhere else: one +# pointing at /proc/self/environ would put the job's provider keys in the prompt, +# and from there in a public review comment. Reads stay inside the checkout. + + +def _outside_secret(tmp_path: Path) -> tuple[Path, Path]: + """(the reviewed checkout, a file outside it holding a fake secret).""" + root = tmp_path / "pr" + root.mkdir() + secret = tmp_path / "environ" + secret.write_text("FAKE_API_KEY=sk-LEAKME\n") + return root, secret + + +def test_read_file_refuses_a_symlink_out_of_the_checkout(tmp_path: Path) -> None: + """CONTRIBUTING.md is read on every review, so a link there needs no feature flag.""" + root, secret = _outside_secret(tmp_path) + (root / "CONTRIBUTING.md").symlink_to(secret) + collector = ContextCollector(project_root=root) + assert collector.source.read_file("CONTRIBUTING.md") == "" + + +def test_read_file_refuses_a_parent_path(tmp_path: Path) -> None: + """The relative path is the caller's, but `..` must not climb out either.""" + root, _secret = _outside_secret(tmp_path) + collector = ContextCollector(project_root=root) + assert collector.source.read_file("../environ") == "" + + +def test_read_file_follows_a_link_that_stays_inside(tmp_path: Path) -> None: + """A repository may link one of its own files to another; that is not an escape.""" + root, _secret = _outside_secret(tmp_path) + (root / "docs.md").write_text("standards\n") + (root / "CONTRIBUTING.md").symlink_to(root / "docs.md") + collector = ContextCollector(project_root=root) + assert collector.source.read_file("CONTRIBUTING.md") == "standards\n" + + +def test_collect_full_files_refuses_a_symlink_out_of_the_checkout(tmp_path: Path) -> None: + """A changed source file that links out is left out, not read.""" + root, secret = _outside_secret(tmp_path) + (root / "leak.py").symlink_to(secret) + collector = ContextCollector(project_root=root, features=frozenset({"full_files"})) + with patch.object(collector.source, "get_changed_source_files", return_value=["leak.py"]): + result = collector.source.collect_full_files() + assert "sk-LEAKME" not in result + + +def test_collect_drift_refuses_a_symlinked_patterns_file(tmp_db: Path, tmp_path: Path) -> None: + """patterns.yaml is read from the reviewed checkout too; a link out is never opened.""" + root, secret = _outside_secret(tmp_path) + (root / "docs" / "ontology").mkdir(parents=True) + (root / "docs" / "ontology" / "patterns.yaml").symlink_to(secret) + collector = ContextCollector(project_root=root, db_path=tmp_db, features=frozenset({"drift"})) + with patch("cgis.guardian.collector.DriftScorer") as scorer: + assert collector.graph.collect_drift() == "" + scorer.assert_not_called() + + +def test_collect_full_files_skips_a_changed_path_that_is_a_directory(tmp_path: Path) -> None: + """A PR can make a changed `.py` path a directory (or a link to one); the review goes on.""" + (tmp_path / "pkg.py").mkdir() + collector = ContextCollector(project_root=tmp_path, features=frozenset({"full_files"})) + with patch.object(collector.source, "get_changed_source_files", return_value=["pkg.py"]): + assert collector.source.collect_full_files() == "" + + +def test_reads_survive_bytes_that_are_not_utf8(tmp_path: Path) -> None: + """Undecodable bytes in a PR file are replaced, not raised: one file must not end a review.""" + (tmp_path / "CONTRIBUTING.md").write_bytes(b"rules \xff\n") + (tmp_path / "bad.py").write_bytes(b"x = '\xff'\n") + collector = ContextCollector(project_root=tmp_path, features=frozenset({"full_files"})) + assert collector.source.read_file("CONTRIBUTING.md").startswith("rules ") + with patch.object(collector.source, "get_changed_source_files", return_value=["bad.py"]): + assert "#### `bad.py`" in collector.source.collect_full_files() + + +def test_read_file_refuses_a_path_with_a_null_byte(tmp_path: Path) -> None: + """A path the OS cannot even name reads as absent, not as a crash.""" + collector = ContextCollector(project_root=tmp_path) + assert collector.source.read_file("CONTRIBUTING\0.md") == "" + + +def test_contained_returns_the_resolved_path(tmp_path: Path) -> None: + """What is read is the path that was checked, not a spelling that resolves again later.""" + (tmp_path / "real.md").write_text("x\n") + (tmp_path / "alias.md").symlink_to(tmp_path / "real.md") + assert contained(tmp_path, "alias.md") == (tmp_path / "real.md").resolve() diff --git a/tests/unit/test_guardian_review_script.py b/tests/unit/test_guardian_review_script.py index 32162175..6c39dff2 100644 --- a/tests/unit/test_guardian_review_script.py +++ b/tests/unit/test_guardian_review_script.py @@ -15,7 +15,7 @@ import guardian_review as gr -_NO_KEYS = ("GEMINI_API_KEY", "MISTRAL_API_KEY", "GUARDIAN_PROVIDER") +_NO_KEYS = ("GEMINI_API_KEY", "MISTRAL_API_KEY", "GUARDIAN_PROVIDER", "GUARDIAN_EVIDENCE") def _argv(monkeypatch: pytest.MonkeyPatch, *args: str) -> None: @@ -54,3 +54,85 @@ async def test_a_usable_metrics_path_gets_past_the_check( _argv(monkeypatch, "--metrics", str(tmp_path / "guardian_metrics.jsonl")) with pytest.raises(RuntimeError, match="API_KEY"): await gr.main() + + +@pytest.mark.asyncio +async def test_a_repo_root_that_is_not_a_directory_is_refused_before_building_a_provider( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """`--repo-root` names the reviewed checkout (#730); a typo must not review nothing.""" + _argv(monkeypatch, "--repo-root", str(tmp_path / "missing")) + with pytest.raises(ValueError, match="--repo-root"): + await gr.main() + + +@pytest.mark.asyncio +async def test_evidence_on_a_foreign_checkout_is_refused( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The evidence pass runs `uv run mypy/ruff` inside the reviewed checkout. + + With `--repo-root` that checkout is the PR — code its author controls — and + the job holds provider keys (#730). Refused loudly, not skipped quietly: + whoever passes the flag with a foreign checkout learns it cannot work. + guardian.yml itself does not pass it, and says why. + """ + _argv(monkeypatch, "--repo-root", str(tmp_path)) + monkeypatch.setenv("GUARDIAN_EVIDENCE", "1") + with pytest.raises(ValueError, match="GUARDIAN_EVIDENCE"): + await gr.main() + + +@pytest.mark.asyncio +async def test_a_foreign_checkout_without_evidence_gets_past_the_checks( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The normal #730 case: a separate PR checkout, evidence off, reaches the provider.""" + _argv(monkeypatch, "--repo-root", str(tmp_path)) + with pytest.raises(RuntimeError, match="API_KEY"): + await gr.main() + + +class _CollectedError(Exception): + """Raised by the stand-in collector to stop the review once its root is known.""" + + +@pytest.fixture +def roots(tmp_path: Path) -> tuple[Path, Path]: + """(the reviewed checkout as resolved, this repository) — resolved outside the event loop.""" + return tmp_path.resolve(), Path(gr.__file__).parent.parent.absolute() + + +@pytest.mark.asyncio +async def test_the_review_reads_the_repo_root_and_fingerprints_this_repository( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, roots: tuple[Path, Path] +) -> None: + """The flag must reach the collector. + + Validating it and then reviewing `.` would pass every test above. + """ + seen: dict[str, Path] = {} + + class _Provider: + name = "stub" + + def _collector(**kwargs: object) -> None: + seen["root"] = Path(str(kwargs["project_root"])) + raise _CollectedError + + def _fingerprint(reader: object, _active: object) -> str: + seen["fingerprint_reader"] = Path(str(reader)) + return "fp" + + _argv(monkeypatch, "--repo-root", str(tmp_path)) + monkeypatch.setenv("GUARDIAN_SKEPTIC", "off") + monkeypatch.setattr(gr, "build_provider", lambda _env: (_Provider(), "stub-model")) + monkeypatch.setattr(gr, "build_skeptic_provider", lambda _env, **_kw: None) + monkeypatch.setattr(gr, "disk_reader", lambda root: root) + monkeypatch.setattr(gr, "compute_fingerprint", _fingerprint) + monkeypatch.setattr(gr, "ContextCollector", _collector) + expected_root, this_repository = roots + with pytest.raises(_CollectedError): + await gr.main() + assert seen["root"] == expected_root + assert seen["fingerprint_reader"] == this_repository diff --git a/tests/unit/test_guardian_workflow.py b/tests/unit/test_guardian_workflow.py new file mode 100644 index 00000000..468a56c1 --- /dev/null +++ b/tests/unit/test_guardian_workflow.py @@ -0,0 +1,133 @@ +"""The guardian review job treats the PR as data, never as code (#730). + +`/guardian review` runs with provider keys in its environment, and the PR it +reviews may come from anyone. The job used to check the PR out at the root and +run its lockfile, its scripts and its checkers there. These tests pin the shape +that closes that: trusted tooling at the root, the PR in its own directory, and +nothing running from inside it but one fixed `git fetch`. +""" + +import re +from pathlib import Path, PurePosixPath +from typing import Any + +import pytest +import yaml + +WORKFLOW = Path(__file__).parent.parent.parent / ".github" / "workflows" / "guardian.yml" +PR_DIR = "pr" +#: The only command allowed to run inside the PR checkout. +PR_FETCH = 'git fetch origin "$BASE_BRANCH"' +#: The only two ways a command may name the PR checkout: parse it, review it. +ALLOWED_MENTIONS = ( + re.compile(rf"cgis ingest {PR_DIR}/src(?=\s)"), + re.compile(rf"--repo-root[ =]{PR_DIR}(?=\s)"), +) +#: Ways a command could reach into the PR checkout, after the allowed mentions are removed. +#: Out of scope, as contrived rather than natural respellings: indirection through a +#: shell variable, `git -C`, UV_PROJECT in env, and writes to $GITHUB_ENV. +FORBIDDEN_MENTIONS = ( + re.compile(rf"(? bool: + """Whether a working directory is the PR checkout or below it, however it is spelt.""" + parts = PurePosixPath(str(directory or ".")).parts + return parts[:1] == (PR_DIR,) + + +@pytest.fixture(scope="module") +def workflow() -> dict[Any, Any]: + """The parsed workflow file.""" + result: dict[Any, Any] = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + return result + + +@pytest.fixture(scope="module") +def steps(workflow: dict[Any, Any]) -> list[dict[str, Any]]: + """The steps of the `review` job.""" + result: list[dict[str, Any]] = workflow["jobs"]["review"]["steps"] + return result + + +def _with(step: dict[str, Any]) -> dict[str, Any]: + result: dict[str, Any] = step.get("with") or {} + return result + + +def _checkouts(steps: list[dict[str, Any]]) -> list[dict[str, Any]]: + return [s for s in steps if str(s.get("uses", "")).startswith("actions/checkout@")] + + +def _holds_secret(step: dict[str, Any]) -> bool: + return any("secrets." in str(v) for v in (step.get("env") or {}).values()) + + +def test_the_pr_is_checked_out_into_its_own_directory(steps: list[dict[str, Any]]) -> None: + """A PR checkout at the root would put its scripts and lockfile where the job runs them.""" + pr = [s for s in _checkouts(steps) if "refs/pull/" in str(_with(s).get("ref", ""))] + assert len(pr) == 1 + assert _with(pr[0]).get("path") == PR_DIR + assert _with(pr[0]).get("persist-credentials") is False + + +def test_the_root_checkout_is_the_default_branch(steps: list[dict[str, Any]]) -> None: + """The tooling that runs must come from reviewed, merged code.""" + root = [s for s in _checkouts(steps) if not _with(s).get("path")] + assert len(root) == 1 + assert _with(root[0]).get("ref") == "${{ github.event.repository.default_branch }}" + + +def test_only_the_base_fetch_runs_inside_the_pr_checkout(steps: list[dict[str, Any]]) -> None: + """Exactly one fixed git command runs in pr/; `git fetch … && uv sync` must not pass.""" + inside = [s for s in steps if _in_pr(s.get("working-directory"))] + assert [str(s.get("run", "")).strip() for s in inside] == [PR_FETCH] + assert not any(_holds_secret(s) for s in inside) + + +def test_no_action_runs_from_or_inside_the_pr_checkout(steps: list[dict[str, Any]]) -> None: + """A local action (`uses: ./…`) is checkout code; an action aimed at pr/ reads its config.""" + for step in steps: + assert not str(step.get("uses", "")).startswith("./"), step.get("name") + if "uses" in step: + assert not _in_pr(_with(step).get("working-directory")), step.get("name") + + +def test_no_command_reaches_into_the_pr_checkout(steps: list[dict[str, Any]]) -> None: + """`uv sync --directory pr` or `python pr/scripts/…` at the root: the same leak, respelt.""" + for step in steps: + run = str(step.get("run", "")) + " " + for allowed in ALLOWED_MENTIONS: + run = allowed.sub(" ", run) + for pattern in FORBIDDEN_MENTIONS: + assert not pattern.search(run), (step.get("name"), pattern.pattern) + + +def test_the_review_runs_this_repositorys_script_on_the_pr(steps: list[dict[str, Any]]) -> None: + """The step with the keys runs scripts/ from the root and reviews pr/ through --repo-root.""" + review = [s for s in steps if _holds_secret(s)] + assert [s["name"] for s in review] == ["Run Guardian Review"] + run = str(review[0]["run"]) + " " + assert "uv run --frozen python scripts/guardian_review.py" in run + assert ALLOWED_MENTIONS[1].search(run) + assert "working-directory" not in review[0] + + +def test_the_graph_is_built_from_the_pr_sources(steps: list[dict[str, Any]]) -> None: + """The graph must describe the code under review, parsed by the root's cgis.""" + ingest = [s for s in steps if "cgis ingest" in str(s.get("run", ""))] + assert len(ingest) == 1 + assert str(ingest[0]["run"]).startswith(f"uv run --frozen cgis ingest {PR_DIR}/src ") + assert "working-directory" not in ingest[0] + + +def test_the_evidence_flag_reaches_no_level_of_the_review_job( + workflow: dict[Any, Any], steps: list[dict[str, Any]] +) -> None: + """Evidence runs the PR's own checkers; not at step, job or workflow level.""" + levels = [workflow.get("env") or {}, workflow["jobs"]["review"].get("env") or {}] + levels += [s.get("env") or {} for s in steps] + assert not any("GUARDIAN_EVIDENCE" in env for env in levels)