fix: give the verdict tests their own env file so they pass in CI

The new test file could not pass in CI, and the review caught it with the
failing run: `Fast tests` was red on this PR while local `quality-check.sh` was
green. Reproduced both sides before fixing.

Cause: request-pr-review.sh posts its trigger through scripts/gh-agent.sh,
which reads a real BESS_AGENT_TOKEN and exits 1 before `gh` is reached.
Shimming `gh` on PATH does not help -- gh-agent.sh is invoked by a
repo-relative path, not looked up on PATH. Worse, it resolves its env file from
the MAIN checkout (`dirname $(git rev-parse --git-common-dir)`), so this
worktree having no `.env` of its own was irrelevant: the developer's real token
was read anyway. CI provisions no `.env` and no such secret, so the suite failed
unconditionally there. The abandoned `(d / "scripts").mkdir(...)` in the fixture
was an attempt at this that did nothing.

Fixed with the seam gh-agent.sh already documents for exactly this
(`BESS_ENV_FILE`, "a seam tests use to point at a fixture .env instead"), so no
production code changes: each test now supplies its own env file carrying a
dummy token.

Also adds a test that PINS that dependency, because the fix is otherwise
invisible and could be dropped again silently: point the seam at an empty file
and the script must fail before polling, naming the missing token.

Worth recording how I nearly mis-verified this: my first attempt set
BESS_ENV_FILE from OUTSIDE pytest and saw the tests still pass, which looked
like proof of CI-safety. It was not -- the test sets that variable in the
subprocess env, so it overrides any outer value and the experiment could not
fail. The in-test pin above is the version that can actually discriminate.

Full backend suite: 507 passed.

Author: johanzander

backend/tests/test_request_pr_review.py

@@ -32,11 +32,31 @@ def _write(path: Path, body: str) -> None:
 def bin_dir(tmp_path: Path) -> Path:
     d = tmp_path / "bin"
     d.mkdir()
-    # The trigger comment goes through gh-agent.sh; make it a no-op.
-    (d / "scripts").mkdir(parents=True, exist_ok=True)
     return d
 
 
+@pytest.fixture
+def env_file(tmp_path: Path) -> Path:
+    """A fixture `.env` for `scripts/gh-agent.sh`, via its `BESS_ENV_FILE` seam.
+
+    The script posts its trigger comment through gh-agent.sh, which reads a real
+    token and exits 1 before `gh` is ever reached:
+
+        token="${!token_var:-}"
+        if [ -z "$token" ]; then echo "... is not set in ${env_file}"; exit 1; fi
+
+    Shimming `gh` on PATH does not help, because gh-agent.sh is invoked by a
+    repo-relative path rather than looked up on PATH. Without this the suite
+    needs a real `BESS_AGENT_TOKEN` in the gitignored `.env`, so it passes on a
+    developer machine and fails unconditionally in CI — which is exactly what
+    happened: `Fast tests` went red while local `quality-check.sh` was green,
+    because a real local `.env` papered over the gap.
+    """
+    p = tmp_path / "fixture.env"
+    p.write_text("BESS_AGENT_TOKEN=dummy-token-for-tests\n")
+    return p
+
+
 def _gh(bin_dir: Path, reviews: list, run_state: str) -> None:
     """A `gh` answering the two queries the script makes.
 
@@ -91,11 +111,14 @@ def _review(state: str, at: str = "2099-01-01T00:00:01Z", body: str = "x") -> di
     return {"state": state, "submittedAt": at, "body": body, "author": {"login": "bot"}}
 
 
-def _run(bin_dir: Path, timeout: int = 2) -> subprocess.CompletedProcess:
+def _run(
+    bin_dir: Path, env_file: Path, timeout: int = 2
+) -> subprocess.CompletedProcess:
     env = dict(
         os.environ,
         PATH=f"{bin_dir}:{os.environ['PATH']}",
         REVIEW_POLL_INTERVAL="1",
+        BESS_ENV_FILE=str(env_file),
     )
     return subprocess.run(
         ["bash", str(SCRIPT), "622", str(timeout)],
@@ -106,21 +129,49 @@ def _run(bin_dir: Path, timeout: int = 2) -> subprocess.CompletedProcess:
     )
 
 
-def test_approved_returns_immediately(bin_dir: Path) -> None:
+def test_a_missing_token_fails_loudly_and_is_why_the_seam_exists(
+    bin_dir: Path, tmp_path: Path
+) -> None:
+    """Pins the dependency the other tests satisfy, so the seam cannot be
+    quietly dropped again.
+
+    Every other test passes its own `BESS_ENV_FILE`. Without one, gh-agent.sh
+    resolves the env file from the MAIN checkout — `dirname $(git
+    rev-parse --git-common-dir)` — so a worktree with no `.env` of its own still
+    read the developer's real token and the suite passed locally while failing
+    unconditionally in CI, where no `.env` is provisioned.
+
+    Point the seam at an empty file and the script must fail before polling,
+    with the reason named.
+    """
+    empty = tmp_path / "empty.env"
+    empty.write_text("")
+    _gh(bin_dir, [_review("APPROVED")], "finished")
+
+    proc = _run(bin_dir, empty)
+
+    assert proc.returncode != 0
+    assert "VERDICT" not in proc.stdout
+    assert "BESS_AGENT_TOKEN is not set" in proc.stderr
+
+
+def test_approved_returns_immediately(bin_dir: Path, env_file: Path) -> None:
     _gh(bin_dir, [_review("APPROVED")], "running")
-    proc = _run(bin_dir)
+    proc = _run(bin_dir, env_file)
     assert proc.returncode == 0
     assert "VERDICT APPROVED" in proc.stdout
 
 
-def test_changes_requested_returns_immediately(bin_dir: Path) -> None:
+def test_changes_requested_returns_immediately(bin_dir: Path, env_file: Path) -> None:
     _gh(bin_dir, [_review("CHANGES_REQUESTED")], "running")
-    proc = _run(bin_dir)
+    proc = _run(bin_dir, env_file)
     assert proc.returncode == 0
     assert "VERDICT CHANGES_REQUESTED" in proc.stdout
 
 
-def test_commented_while_running_is_not_a_verdict(bin_dir: Path) -> None:
+def test_commented_while_running_is_not_a_verdict(
+    bin_dir: Path, env_file: Path
+) -> None:
     """The bug, three rounds running.
 
     The bot posts an early permission-check comment and keeps working for
@@ -133,49 +184,57 @@ def test_commented_while_running_is_not_a_verdict(bin_dir: Path) -> None:
         [_review("COMMENTED", body="test permission check - ignore")],
         "running",
     )
-    proc = _run(bin_dir)
+    proc = _run(bin_dir, env_file)
 
     assert proc.returncode == 2
     assert "VERDICT" not in proc.stdout
     assert "still running" in proc.stderr
 
 
-def test_commented_after_the_run_finished_is_the_verdict(bin_dir: Path) -> None:
+def test_commented_after_the_run_finished_is_the_verdict(
+    bin_dir: Path, env_file: Path
+) -> None:
     """The opposite failure. `pr-review.yml` lists COMMENT as one of three final
     verdicts, so once the run is over a COMMENTED last word IS the answer —
     swallowing it made the script report "no summary" while findings sat on the
     PR."""
     _gh(bin_dir, [_review("COMMENTED")], "finished")
-    proc = _run(bin_dir)
+    proc = _run(bin_dir, env_file)
 
     assert proc.returncode == 0
     assert "VERDICT COMMENTED" in proc.stdout
 
 
-def test_a_failed_run_reports_at_once_instead_of_waiting(bin_dir: Path) -> None:
+def test_a_failed_run_reports_at_once_instead_of_waiting(
+    bin_dir: Path, env_file: Path
+) -> None:
     """A dead run and a thinking one are both silence if you only poll reviews.
     #623's run died on `Reached maximum number of turns (60)` and the wait
     continued for 16 minutes."""
     _gh(bin_dir, [], "failed")
-    proc = _run(bin_dir, timeout=60)
+    proc = _run(bin_dir, env_file, timeout=60)
 
     assert proc.returncode == 2
     assert "FAILED" in proc.stderr
     assert "not a slow one" in proc.stderr
 
 
-def test_no_run_at_all_is_reported_as_a_trigger_fault(bin_dir: Path) -> None:
+def test_no_run_at_all_is_reported_as_a_trigger_fault(
+    bin_dir: Path, env_file: Path
+) -> None:
     """#619 failed this way twice: the trigger never reached the workflow, which
     needs a different response from a stalled review."""
     _gh(bin_dir, [], "none")
-    proc = _run(bin_dir)
+    proc = _run(bin_dir, env_file)
 
     assert proc.returncode == 2
     assert "No PR Review run started at all" in proc.stderr
     assert "actor gate" in proc.stderr
 
 
-def test_a_decisive_verdict_wins_over_an_earlier_commented(bin_dir: Path) -> None:
+def test_a_decisive_verdict_wins_over_an_earlier_commented(
+    bin_dir: Path, env_file: Path
+) -> None:
     """Ordering, not recency of any state: the stub is older, the verdict newer."""
     _gh(
         bin_dir,
@@ -185,7 +244,7 @@ def test_a_decisive_verdict_wins_over_an_earlier_commented(bin_dir: Path) -> Non
         ],
         "running",
     )
-    proc = _run(bin_dir)
+    proc = _run(bin_dir, env_file)
 
     assert proc.returncode == 0
     assert "VERDICT CHANGES_REQUESTED" in proc.stdout