fix(guardian): review the PR as data, never run its code beside the keys

.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

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
         )

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(

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.
 """

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))

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