Validate relay-directory changes in PR CI (#1711)

* Validate relay-directory changes in PR CI

The weekly automation validates a relay snapshot before it pushes its
branch, but that check lives inside the scheduled job. A change to the
reviewed relay directory that arrives as a pull request is validated by
nothing at all, which is the ground two relay-data PRs were closed on:
"PR CI doesn't run the validator, and untraceable relay-directory
updates aren't something we can merge on faith" (#1632, also #1673).

This runs the validator on a pull request touching relays/ or the
validator itself, against the base branch's reviewed copy, and publishes
the row count, unique-relay count and SHA-256 to the job summary. The
hash is the point: the relay automation already publishes one for every
snapshot it proposes, so a reviewer can now match a proposed file
against it instead of taking provenance on faith.

The gate deliberately runs the base branch's validator rather than the
pull request's copy of it. Otherwise a change that edits the data and
the validator together certifies itself -- dropping 86% of the directory
passes if the same commit relaxes MIN_BASELINE_FRACTION, and the summary
still reports "validated". The scheduled job validates from a main
checkout for the same reason. The pull request's own validator is still
exercised by the unit-test step.

A pull_request run executes the workflow definition from the pull
request's own head, so a pull request can always edit this file and
change its own gate. That path is visible in the diff, which is what
review is for; what the reviewed-validator gate closes is the quiet one.

Read-only and never pull_request_target: the job runs PR-authored code,
so it holds no write scope and no secrets. checkout is @v5 like the
other read-only PR workflows rather than SHA-pinned, since that pin
exists to protect the write-capable token in fetch_georelays.yml.

What it catches: malformed rows, out-of-range coordinates, insecure
hosts, conflicting duplicate relays, and snapshots that lose half or
double the baseline's relays. What it does not: a re-ordered file whose
rows are each valid still passes -- with a different hash, which the
summary makes visible.

The accompanying test asserts the job's security-relevant shape as text,
following the precedent in test_fetch_georelays_workflow.py: the
trigger, the absent write scope, --output never pointed at the tracked
CSV, and that the gate reads the validator from the base branch.

* Split the gate from the code under review

Codex review on d627b9f found three holes, all confirmed by rehearsal.

The gate ran the base branch's validator, but read the candidate from the
worktree and ran the pull request's unit tests first. A test file that
rewrites relays/online_relays_gps.csv at import time therefore decided
what the gate validated: the summary reported 441 rows and the hash of a
file the pull request never proposed, exit 0, while the 60-row CSV in the
commit was what would merge. The published hash is the whole argument for
reviewing relay data on evidence, so a hash that can describe different
bytes than the ones merging is worse than no hash at all.

The workflow is now two jobs with an explicit split:

- gate is the trust boundary. It checks out the base commit, so nothing
  the pull request wrote is present to run, and reads the candidate as a
  git object pinned to the event's head SHA instead of from a worktree.
  Both inputs come from the event, so a base branch that advances while
  the job is queued cannot change the verdict.
- proposed-validator runs pull-request code, so it is not a trust
  boundary. It runs the unit tests and, new here, the proposed validator
  against the real directory. Without that a validator change is only
  ever exercised against small synthetic fixtures: capping MAX_ROWS at
  400 passes the unit tests and the gate, then breaks the weekly
  automation after merge with "CSV exceeds 400 data rows".

Checking out the base commit also removes last round's git show of the
validator -- at base.sha the reviewed validator is simply the one in the
workspace.

Rehearsed per case, with each branch's diff verified first: a malicious
PR that swaps the CSV from a test now exits 1 where it previously passed
green; the current automation snapshot still validates to the hash
published in #1617; a corrupt CSV, a stricter validator, and a CI-only
change all land where they should.

.github/workflows/validate_georelays.yml

@@ -0,0 +1,114 @@
+name: Validate GeoRelay Data
+
+# The weekly automation validates a relay snapshot before pushing its branch,
+# but that check lives inside the scheduled job: a relay-directory change that
+# arrives as a pull request is otherwise reviewed on faith. This runs the same
+# validator against the reviewed baseline and publishes the resulting hash, so
+# a proposed snapshot can be matched against the one the automation published.
+on:
+  pull_request:
+    paths:
+      - "relays/online_relays_gps.csv"
+      - "scripts/validate_georelays.py"
+      - "scripts/tests/**"
+      - ".github/workflows/validate_georelays.yml"
+
+# Both jobs run against pull-request content. Read-only, no secrets, and never
+# pull_request_target.
+permissions:
+  contents: read
+
+# A superseded run has nothing left to report; the sibling PR workflows do not
+# set this, so drop it if the repo prefers them uniform.
+concurrency:
+  group: validate-georelays-${{ github.ref }}
+  cancel-in-progress: true
+
+jobs:
+  # The trust boundary. Nothing authored by the pull request executes in this
+  # job, and the bytes it validates come from a git object rather than a
+  # worktree that PR-authored code could have rewritten.
+  gate:
+    name: Validate relay directory
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+
+    steps:
+      - name: Check out the reviewed base
+        # base.sha, not the merge ref: the pull request's validator and its
+        # tests are never present in this workspace, so the gate cannot be
+        # influenced by code under review.
+        uses: actions/checkout@v5
+        with:
+          ref: ${{ github.event.pull_request.base.sha }}
+          persist-credentials: false
+
+      - name: Validate the proposed relay directory
+        env:
+          PR_NUMBER: ${{ github.event.pull_request.number }}
+          HEAD_SHA: ${{ github.event.pull_request.head.sha }}
+        run: |
+          set -euo pipefail
+
+          # Read the candidate as a git object pinned to the event's head commit.
+          # Reading it from a checked-out worktree would let PR-authored code
+          # swap the file first, so the hash below would describe a snapshot
+          # that never merges.
+          git fetch --no-tags --depth=1 origin "refs/pull/${PR_NUMBER}/head"
+          fetched_sha="$(git rev-parse FETCH_HEAD)"
+          if [[ "$fetched_sha" != "$HEAD_SHA" ]]; then
+            echo "::error::pull request head moved to $fetched_sha since this event ($HEAD_SHA); a newer run validates it"
+            exit 1
+          fi
+          git show "${HEAD_SHA}:relays/online_relays_gps.csv" > "$RUNNER_TEMP/candidate.csv"
+
+          # Validator and baseline are the checked-out base commit's, so a change
+          # cannot relax the rule that judges it. --output is required and gets a
+          # verbatim copy of --input; it stays in RUNNER_TEMP.
+          python3 scripts/validate_georelays.py \
+            --input "$RUNNER_TEMP/candidate.csv" \
+            --baseline relays/online_relays_gps.csv \
+            --output "$RUNNER_TEMP/validated-copy.csv" \
+            --github-output "$RUNNER_TEMP/summary.txt"
+
+          # Publish the hash so a reviewer can match a proposed snapshot against
+          # the one the automation published, instead of taking it on faith.
+          {
+            echo "### GeoRelay validation"
+            echo
+            sed 's/^/- /' "$RUNNER_TEMP/summary.txt"
+          } >> "$GITHUB_STEP_SUMMARY"
+
+  # Runs code authored by the pull request, so it is an honest-mistake check and
+  # not a trust boundary: it catches a validator change that would invalidate the
+  # committed directory and break the weekly automation after merge.
+  proposed-validator:
+    name: Test the proposed validator
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+
+    steps:
+      - name: Check out the pull request
+        uses: actions/checkout@v5
+        with:
+          persist-credentials: false
+
+      - name: Test GeoRelay validator
+        run: python3 -m unittest discover -s scripts/tests -p "test_*.py" -v
+
+      - name: Run the proposed validator against the real directory
+        env:
+          BASE_SHA: ${{ github.event.pull_request.base.sha }}
+        run: |
+          set -euo pipefail
+
+          # Without this the proposed validator only ever sees unit-test
+          # fixtures, and a new constraint can make the committed directory
+          # invalid while this workflow stays green.
+          git fetch --no-tags --depth=1 origin "$BASE_SHA"
+          git show "${BASE_SHA}:relays/online_relays_gps.csv" > "$RUNNER_TEMP/baseline.csv"
+
+          python3 scripts/validate_georelays.py \
+            --input relays/online_relays_gps.csv \
+            --baseline "$RUNNER_TEMP/baseline.csv" \
+            --output "$RUNNER_TEMP/proposed-copy.csv"

scripts/tests/test_georelay_pr_workflow.py

@@ -0,0 +1,89 @@
+import re
+from pathlib import Path
+import unittest
+
+
+REPOSITORY_ROOT = Path(__file__).resolve().parents[2]
+WORKFLOW_PATH = REPOSITORY_ROOT / ".github/workflows/validate_georelays.yml"
+RELAY_DATA_PATH = "relays/online_relays_gps.csv"
+VALIDATOR_PATH = "scripts/validate_georelays.py"
+
+
+class ValidateGeoRelaysWorkflowTests(unittest.TestCase):
+    """The relay directory is reviewed data. A pull request that changes it must
+    be judged by the base branch's validator, on the bytes that would actually
+    merge, and the job that does so must not run anything the pull request
+    wrote -- the `gate` job is the trust boundary, `proposed-validator` is not.
+    """
+
+    @classmethod
+    def setUpClass(cls) -> None:
+        cls.workflow = WORKFLOW_PATH.read_text(encoding="utf-8")
+        # Comments explain why a construct is absent and would otherwise trip
+        # the assertions looking for it, so directives are checked without them.
+        cls.directives = "\n".join(
+            line for line in cls.workflow.splitlines() if not line.lstrip().startswith("#")
+        )
+
+    def job(self, name: str) -> str:
+        block = re.search(rf"^  {name}:\n(.*?)(?=^  \S|\Z)", self.directives, re.MULTILINE | re.DOTALL)
+        self.assertIsNotNone(block, f"no {name} job in the workflow")
+        return block.group(1)
+
+    def test_runs_on_untrusted_pull_request_trigger(self) -> None:
+        self.assertIn("pull_request:", self.directives)
+        # pull_request_target would run PR-authored code with repository secrets.
+        self.assertNotIn("pull_request_target", self.directives)
+
+    def test_no_job_holds_a_write_scope(self) -> None:
+        self.assertIn("permissions:\n  contents: read", self.directives)
+        write_scope = re.search(r"^\s*[\w-]+:\s*write\s*$", self.directives, re.MULTILINE)
+        self.assertIsNone(write_scope, "a job running PR-authored code must not hold a write scope")
+
+    def test_checkout_does_not_persist_credentials(self) -> None:
+        self.assertEqual(2, self.directives.count("persist-credentials: false"))
+
+    def test_relay_data_changes_trigger_the_workflow(self) -> None:
+        paths_block = re.search(r"paths:\n((?:\s+- .+\n)+)", self.directives)
+        self.assertIsNotNone(paths_block)
+        self.assertIn(RELAY_DATA_PATH, paths_block.group(1))
+        self.assertIn(VALIDATOR_PATH, paths_block.group(1))
+
+    def test_gate_checks_out_the_reviewed_base(self) -> None:
+        # Checking out the merge ref would put the pull request's validator and
+        # tests in the gate's workspace.
+        self.assertIn("ref: ${{ github.event.pull_request.base.sha }}", self.job("gate"))
+
+    def test_gate_runs_nothing_the_pull_request_wrote(self) -> None:
+        gate = self.job("gate")
+        self.assertNotIn("unittest discover", gate)
+        # The candidate is read as a git object, never from the worktree, so
+        # code running earlier in the job cannot substitute the bytes validated.
+        self.assertIn('git show "${HEAD_SHA}:' + RELAY_DATA_PATH + '"', gate)
+        self.assertIn('--input "$RUNNER_TEMP/candidate.csv"', gate)
+        self.assertNotIn(f"--input {RELAY_DATA_PATH}", gate)
+
+    def test_gate_pins_both_inputs_to_the_event(self) -> None:
+        gate = self.job("gate")
+        self.assertIn("HEAD_SHA: ${{ github.event.pull_request.head.sha }}", gate)
+        # A head that moved after the event means these bytes are not the ones
+        # this run was asked about.
+        self.assertIn('if [[ "$fetched_sha" != "$HEAD_SHA" ]]; then', gate)
+        self.assertIn("exit 1", gate)
+
+    def test_gate_validation_cannot_write_to_the_reviewed_file(self) -> None:
+        gate = self.job("gate")
+        self.assertIn('--output "$RUNNER_TEMP/', gate)
+        self.assertNotIn(f"--output {RELAY_DATA_PATH}", gate)
+
+    def test_proposed_validator_is_exercised_against_the_real_directory(self) -> None:
+        # Unit-test fixtures are small and synthetic; a new constraint can pass
+        # them while invalidating the committed directory.
+        job = self.job("proposed-validator")
+        self.assertIn("unittest discover", job)
+        self.assertIn(f"--input {RELAY_DATA_PATH}", job)
+        self.assertIn('git show "${BASE_SHA}:' + RELAY_DATA_PATH + '"', job)
+
+
+if __name__ == "__main__":
+    unittest.main()