From 2890a0cefed503c113b06eb2495528af3812efe0 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Sat, 22 Aug 2026 10:13:03 +1200 Subject: [PATCH] feat(launchpad): deterministic pre-review pass for batch PR review (#426) Reviewing a batch of PRs has two halves. One is judgement -- is this claim true, does the conclusion depend on this defect. The other is bookkeeping applied identically to every PR. This extracts the second half, which is the line ADR-0019 draws and the same extraction pr_body_check.py and adr_boundary_check.py already did for their own rules. Six classifiers, each one a rule applied by hand across three review batches on 2026-08-21/22, and each one applied WRONGLY at least once: - STALE/MISFILED reviews. Four PRs carried change-requests already satisfied. #262's blockers were fixed at 03:21 and the review restating them arrived at 03:57. #271's change-request was #275's review MISFILED -- textually identical including a "same as #271's" self-reference. No change to #271 could have addressed it. - CI triage. #268's red CI was setup-mold timing out on a one-markdown-file PR. #288's log printed four inherited warnings above the real blocker. - Independence. #265 carried a commit written in the reviewing session. - Leak scan. #281 quoted a private hook's header in a public file. - Placement, per AGENTS.md section 3. - Drift calibration. #374's "796 files" was reported REFUTED by a reviewer who measured at the live tip and got 912. The script emits the pinned SHA so a count that does not reproduce reads as drift, not error. It emits no severity. Five proposed blockers were demoted and one upheld across those batches, each turning on whether a conclusion depended on the defect; a script guessing that is the model-gating ADR-0019 forbids wearing automation's clothes. test_the_briefing_states_no_severity_anywhere asserts the absence. It also posts nothing, so it can run read-only. Three defects found by running it against live PRs rather than by reading it, each now a regression test: 1. FALSE MISFILED on #374. The only path token in a genuine review was `launchpad/ARCHITECTURE.md`, cited as corroborating evidence rather than as a defect site. Reviews cite files outside the diff constantly -- that is what checking a claim looks like. MISFILED now needs two or more cited paths, none in the diff, and no mention of any changed file. 2. Selecting log lines BY POSITION. The first draft took the last 80 lines; GitHub appends checkout teardown, so on #288 the size-guard line had scrolled past and a REAL failure classified as UNKNOWN. Selection is by content now. 3. The path regex required `:\d+` with no space, so it saw compiler output (`lib.rs:276:15`) but not the file-size guard (`lib.rs: 1000 -> 1001`). Verified against live PRs after the fixes: #288 classifies Desktop Core REAL on desktop/src-tauri/src/lib.rs and Desktop Smoke E2E PRE_EXISTING, matching the hand analysis; #374's calibration returns 796 files at the 67-commit point with tip 9891e64f6, matching the figure reconstructed by hand. 41 tests in test_pr_review_batch, 256 across launchpad/scripts. Registered in test_no_model.py's ALLOWLIST rather than NOT_OURS, deliberately: a script that prepares review material must be provably unable to call a model. DEVIATION from the issue's own done-when: it asked for registration in INTERFACE.md. Not done -- INTERFACE.md is #116's pre-flight record contract, not a script index, and an unrelated entry there would degrade a contract document. The script's module docstring is its interface. Refs #426 Signed-off-by: Serina Mcfall --- launchpad/scripts/pr_review_batch.py | 621 ++++++++++++++++++++++ launchpad/scripts/test_no_model.py | 18 + launchpad/scripts/test_pr_review_batch.py | 425 +++++++++++++++ 3 files changed, 1064 insertions(+) create mode 100644 launchpad/scripts/pr_review_batch.py create mode 100644 launchpad/scripts/test_pr_review_batch.py diff --git a/launchpad/scripts/pr_review_batch.py b/launchpad/scripts/pr_review_batch.py new file mode 100644 index 00000000000..21372075237 --- /dev/null +++ b/launchpad/scripts/pr_review_batch.py @@ -0,0 +1,621 @@ +#!/usr/bin/env python3 +"""Deterministic pre-review pass over a batch of pull requests. Issue #426. + +WHAT THIS IS FOR + +Reviewing a batch of PRs has two halves. One is judgement: is this claim true, +does the conclusion depend on this defect, is it wrong now or wrong later. The +other is bookkeeping applied identically to every PR: which reviews are stale, +which CI failures belong to this diff, am I even allowed to review this. + +The second half is a fixed set of rules, so it belongs in a script. That is the +line ADR-0019 draws -- a deterministic script may gate a merge, a model verdict +may only annotate -- and it is the same extraction `pr_body_check.py` and +`adr_boundary_check.py` already did for their own rules. + +WHAT IT DELIBERATELY DOES NOT DO + +It emits no severity and no verdict on any claim. Across the review batches on +2026-08-21/22, five proposed blockers were demoted to High and one upheld; every +one turned on whether a document's conclusion depended on the defect. A script +guessing that would be the model-gating ADR-0019 forbids, dressed as automation. +`BriefingTests.test_the_briefing_states_no_severity_anywhere` asserts the absence. + +It also posts nothing. The caller decides and acts, so this can run read-only. + +WHY EACH RULE EXISTS + +Every classifier below was applied by hand across those batches, and each one was +applied WRONGLY at least once. The cost is recorded here because a rule whose +failure nobody remembers gets removed as clutter. + + STALE / MISFILED reviews. Four PRs carried change-requests that had already + been satisfied. #262's blockers were fixed at 03:21; the review restating them + was submitted at 03:57, re-posting a body written at 01:38 against a head that + had moved twice. Worse, #271's change-request was #275's review MISFILED -- + textually identical, down to a "same as #271's" self-reference that only makes + sense sitting on the other PR. No change to #271 could ever have addressed it. + Detected here by citation/diff mismatch rather than by comparing bodies, + because the same body legitimately appears on a stacked pair. + + CI triage. #268's red CI was `setup-mold` timing out while fetching a linker, + on a PR that changed one markdown file. #288's log printed four biome items + first -- all warnings, all in files the PR never touched, inherited from + upstream commits dated weeks earlier -- while the actual blocker, a file-size + guard tripping at 1001 lines, sat forty lines further down. Reading the first + recognisable failure would have cleared that PR. + + Independence. #265 contained a commit written in the very session that was + about to review it. Caught by hand, one review too late to be free. + + Drift calibration. The expensive one. #374's headline "796 files" was reported + REFUTED by a competent reviewer who ran the document's own command against the + live `upstream/main` and got 912. Reconstructing the 67-commit point the + document actually measured gave 796 exactly -- and 575/110/52/35 for every + sub-total. A correct document nearly took a change-request because nobody + pinned the tip. Handing reviewers the pinned SHA up front removes the trap. + +USAGE + + python3 pr_review_batch.py --author tucktuck101 --author benmitchell11 + python3 pr_review_batch.py --review-required + python3 pr_review_batch.py --pr 405 --pr 406 --json + python3 pr_review_batch.py --pr 374 --calibrate f8692fa9b --commits 67 + +Requires `gh` on PATH and authenticated. Read-only: every `gh` call is a GET. +""" + +from __future__ import annotations + +import argparse +import datetime as dt +import json +import re +import subprocess +import sys +from dataclasses import dataclass, field + +DEFAULT_REPO = "launchpad-26/buzz" + +#: Substrings whose appearance in an ADDED diff line means private tooling has +#: leaked into a public repository. These live in a separate private repo; the +#: rule is to describe what a gate does, never to name its files. See #281. +DEFAULT_LEAK_PATTERNS = ("pr-gate", "git-safety", "verify-gate") + +#: Trees upstream owns. `launchpad/AGENTS.md` section 3 bars cohort files here. +#: Note this only judges files that look like cohort documentation or tooling -- +#: a PR editing `crates/` is upstream-shaped work, which is legitimate. +_UPSTREAM_ONLY_PREFIXES = ("docs/", "scripts/") + +#: Failure-log signatures that mean the infrastructure broke, not the change. +_FLAKE_SIGNATURES = ( + "connection timed out", + "read error (connection timed out)", + "tar: error is not recoverable", + "could not resolve host", + "temporary failure in name resolution", + "502 bad gateway", + "503 service unavailable", + "the runner has received a shutdown signal", +) + +#: Steps that only fetch a toolchain. A failure inside one is never the diff's +#: fault, whatever the log says. +_TOOLCHAIN_STEPS = ("setup-mold", "rust-cache", "actions/cache", "setup-python", + "setup-node", "activate-hermit", "install tauri dependencies") + +#: `path/to/file.ext:123` and `path/to/file.ext: 123`. +#: +#: The optional space matters and was missed on the first pass. Two formats +#: occur in real CI output: compilers and linters emit `lib.rs:276:15`, while +#: this repo's file-size guard emits `src-tauri/src/lib.rs: 1000 -> 1001`. A +#: regex requiring the digit to touch the colon sees only the first, which is +#: how a REAL failure on #288 was classified UNKNOWN. +_PATH_TOKEN = re.compile(r"(? 1001 (+1) lines + (allowed 1000)` -- had already scrolled past. The classifier answered + UNKNOWN for a failure it should have called REAL. + + Position in a log is not evidence. Content is. + """ + kept = [] + for line in (log_text or "").splitlines(): + if _LOG_NOISE.search(line): + continue + if _LOG_SIGNAL.search(line): + kept.append(line.strip()) + return "\n".join(kept[-limit:]) + + +def classify_failure(check_name, failed_step, log_tail, diff_paths, failing_paths): + """Classify one failing check as REAL, FLAKE, PRE_EXISTING or UNKNOWN. + + REAL outranks PRE_EXISTING because a log can carry both, and #288's did: + inherited warnings printed above the size-guard failure that actually broke + the build. Anything that stopped at the first recognised line would have + reported that PR clean. + """ + haystack = f"{failed_step or ''}\n{log_tail or ''}".lower() + + if any(step in (failed_step or "").lower() for step in _TOOLCHAIN_STEPS): + return Verdict("FLAKE", f"failed inside a toolchain step: {failed_step}") + if any(sig in haystack for sig in _FLAKE_SIGNATURES): + return Verdict("FLAKE", "log carries an infrastructure-failure signature") + + if failing_paths: + owned = {p for p in failing_paths if p in diff_paths} + if owned: + return Verdict("REAL", f"failing path(s) in this diff: {sorted(owned)[:3]}") + return Verdict( + "PRE_EXISTING", + f"failing path(s) absent from this diff: {sorted(failing_paths)[:3]}", + ) + + return Verdict("UNKNOWN", f"no attributable path in {check_name}'s output") + + +def classify_independence(commits, self_login, session_since): + """CONFLICT when the reviewing identity authored a commit in this window. + + The window matters. In this repo every commit carries the operator's git + identity, including ones written weeks ago by other sessions, so an + unwindowed check flags every PR and the signal becomes noise. Only commits + inside the current session count as "mine to disclose". + """ + since = parse_time(session_since) + if since is None: + return Verdict("UNKNOWN", "no session window given; independence unassessed") + + mine = [ + c for c in commits + if c.get("author_login") == self_login + and (parse_time(c.get("committed_at")) or since) >= since + ] + if mine: + oids = ", ".join(c["oid"][:8] for c in mine[:3]) + return Verdict("CONFLICT", f"reviewing identity authored {oids} in this session") + return Verdict("INDEPENDENT", "no commit by the reviewing identity in this window") + + +@dataclass +class Attributed: + """A verdict plus what it is about -- a check name, or a reviewer's login. + + A named type rather than a bare tuple because the first draft stored bare + ``Verdict`` objects in one place and ``(name, Verdict)`` pairs in another. + ``blocks_review()`` accepted both, since it only reads ``.state``, while + ``render()`` crashed on the first shape. One of the two callers was always + going to be wrong and nothing would have said which. + """ + + subject: str + verdict: Verdict + + @property + def state(self): + return self.verdict.state + + def as_dict(self): + return {"subject": self.subject, **self.verdict.as_dict()} + + +@dataclass +class LeakHit: + pattern: str + line: str + + +def scan_leaks(diff_text, patterns): + """Private-tooling references in ADDED lines only. + + Removed lines are skipped: a deleted reference is the fix, and flagging it + would punish the commit that cleaned it up. Diff headers are skipped too, + since `+++ b/path/verify-gate.sh` is a filename, not content. + """ + hits = [] + for line in (diff_text or "").splitlines(): + if not line.startswith("+") or line.startswith("+++"): + continue + low = line.lower() + for pattern in patterns: + if pattern in low: + hits.append(LeakHit(pattern, line.strip()[:160])) + return hits + + +def check_placement(paths): + """Added paths that break `launchpad/AGENTS.md` section 3.""" + bad = [] + for path in sorted(paths): + if path.startswith(_UPSTREAM_ONLY_PREFIXES): + bad.append(path) + continue + # A bare .md directly in launchpad/agents/ is scanned as a subagent + # roster and blocks the commit. Pack docs go in a pack subdirectory. + if path.startswith("launchpad/agents/") and path.endswith(".md"): + if path.count("/") == 2: + bad.append(path) + return bad + + +@dataclass +class Briefing: + number: int + author: str + title: str + head_sha: str + reviews: list = field(default_factory=list) + ci: list = field(default_factory=list) + independence: Verdict | None = None + leaks: list = field(default_factory=list) + placement: list = field(default_factory=list) + calibration: dict | None = None + + def blocks_review(self): + """True when a reviewer should not be dispatched yet. + + Only two things stop a review outright: a real CI failure the author + must fix first, and an independence conflict that disqualifies the + reviewer. Flakes and inherited failures are noted, not blocking. + """ + if self.independence and self.independence.state == "CONFLICT": + return True + return any(v.state == "REAL" for v in self.ci) + + def as_dict(self): + return { + "number": self.number, + "author": self.author, + "title": self.title, + "head_sha": self.head_sha, + "blocks_review": self.blocks_review(), + "reviews": [a.as_dict() for a in self.reviews], + "ci": [a.as_dict() for a in self.ci], + "independence": self.independence.as_dict() if self.independence else None, + "leaks": [{"pattern": h.pattern, "line": h.line} for h in self.leaks], + "placement": self.placement, + "calibration": self.calibration, + } + + def render(self): + out = [f"PR #{self.number} {self.author} {self.head_sha[:9]}", f" {self.title}"] + if self.blocks_review(): + out.append(" DO NOT DISPATCH A REVIEWER YET") + for item in self.reviews: + out.append(f" review by {item.subject}: {item.state} -- {item.verdict.reason}") + for item in self.ci: + out.append(f" check {item.subject}: {item.state} -- {item.verdict.reason}") + if self.independence: + out.append(f" independence: {self.independence.state} -- {self.independence.reason}") + for hit in self.leaks: + out.append(f" LEAK ({hit.pattern}): {hit.line}") + for path in self.placement: + out.append(f" PLACEMENT: {path} is outside the cohort trees") + if self.calibration: + c = self.calibration + out.append( + f" calibration: at {c['commits']} commits from {c['merge_base'][:9]} " + f"the tip is {c['tip'][:9]} and the diff is {c['files']} files" + ) + return "\n".join(out) + + +# --------------------------------------------------------------------------- +# gh / git plumbing. Everything below shells out; everything above is pure. +# --------------------------------------------------------------------------- + + +def _run(args): + proc = subprocess.run(args, capture_output=True, text=True, check=False) + return proc.returncode, proc.stdout, proc.stderr + + +def _gh_json(args): + code, out, err = _run(["gh"] + args) + if code != 0: + print(f"pr_review_batch: gh failed: {' '.join(args)}: {err.strip()}", + file=sys.stderr) + return None + try: + return json.loads(out) + except json.JSONDecodeError: + return None + + +def calibrate(merge_base, commits, upstream_ref="upstream/main"): + """The pinned tip and file count N commits from a merge base. + + This is the anti-drift step. Reviewers get the SHA the document measured + against, so a count that fails to reproduce at the live tip is recognised + as drift rather than reported as an error. See #374 and issue #384. + """ + code, out, _ = _run( + ["git", "rev-list", f"{merge_base}..{upstream_ref}"] + ) + if code != 0: + return None + revs = out.split() + if len(revs) < commits: + return None + tip = revs[-commits] + code, out, _ = _run(["git", "diff", "--name-only", merge_base, tip]) + if code != 0: + return None + return { + "merge_base": merge_base, + "commits": commits, + "tip": tip, + "files": len([line for line in out.splitlines() if line.strip()]), + } + + +def brief(number, repo, self_login, session_since, calibration=None): + meta = _gh_json([ + "pr", "view", str(number), "--repo", repo, + "--json", "number,title,author,headRefOid,reviews,commits,files", + ]) + if meta is None: + return None + + diff_paths = {f["path"] for f in meta.get("files") or []} + commits = [ + { + "oid": c.get("oid", ""), + "author_login": (c.get("authors") or [{}])[0].get("login") + or (c.get("authors") or [{}])[0].get("name"), + "committed_at": c.get("committedDate"), + } + for c in meta.get("commits") or [] + ] + head_at = commits[-1]["committed_at"] if commits else None + + b = Briefing( + number=meta["number"], + author=(meta.get("author") or {}).get("login", "?"), + title=meta.get("title", ""), + head_sha=meta.get("headRefOid", ""), + ) + + for review in meta.get("reviews") or []: + verdict = classify_review(review, head_at, diff_paths) + if verdict: + b.reviews.append( + Attributed((review.get("author") or {}).get("login", "?"), verdict) + ) + + checks = _gh_json(["pr", "checks", str(number), "--repo", repo, + "--json", "name,state,link"]) or [] + for check in checks: + if check.get("state") != "FAILURE": + continue + # Attributing a failure needs the log. Kept to the tail: a full CI log + # is megabytes and the failure is always at the end. + job_id = (check.get("link") or "").rstrip("/").split("/")[-1] + failed_step, log_tail, failing = "", "", set() + if job_id.isdigit(): + job = _gh_json(["api", f"repos/{repo}/actions/jobs/{job_id}"]) or {} + for step in job.get("steps") or []: + if step.get("conclusion") == "failure": + failed_step = step.get("name", "") + break + code, out, _ = _run(["gh", "run", "view", "--job", job_id, + "--repo", repo, "--log-failed"]) + log_tail = relevant_log_lines(out) if code == 0 else "" + failing = {m.group(1) for m in _PATH_TOKEN.finditer(log_tail)} + # Log paths are workspace-relative; diff paths are repo-relative. + failing = {p for p in failing} | { + d for d in diff_paths if any(d.endswith(p) for p in failing) + } + b.ci.append(Attributed( + check.get("name", "?"), + classify_failure(check.get("name", ""), failed_step, + log_tail, diff_paths, failing), + )) + + b.independence = classify_independence(commits, self_login, session_since) + + code, diff_text, _ = _run(["gh", "pr", "diff", str(number), "--repo", repo]) + if code == 0: + b.leaks = scan_leaks(diff_text, DEFAULT_LEAK_PATTERNS) + b.placement = check_placement(diff_paths) + b.calibration = calibration + return b + + +def _select(args): + if args.pr: + return args.pr + query = ["pr", "list", "--repo", args.repo, "--state", "open", + "--limit", str(args.limit), "--json", "number,author,reviewDecision"] + rows = _gh_json(query) or [] + picked = [] + for row in rows: + if args.author and (row.get("author") or {}).get("login") not in args.author: + continue + if args.review_required and row.get("reviewDecision") not in (None, "REVIEW_REQUIRED"): + continue + picked.append(row["number"]) + return picked + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--repo", default=DEFAULT_REPO) + parser.add_argument("--pr", type=int, action="append", default=[]) + parser.add_argument("--author", action="append", default=[]) + parser.add_argument("--review-required", action="store_true") + parser.add_argument("--limit", type=int, default=100) + parser.add_argument("--self", dest="self_login", default=None, + help="identity to check independence against; " + "defaults to the authenticated gh user") + parser.add_argument("--session-since", default=None, + help="ISO timestamp; commits by --self at or after this " + "count as the reviewer's own work") + parser.add_argument("--calibrate", default=None, metavar="MERGE_BASE") + parser.add_argument("--commits", type=int, default=None) + parser.add_argument("--json", action="store_true") + args = parser.parse_args(argv) + + self_login = args.self_login + if self_login is None: + who = _gh_json(["api", "user", "--jq", "{login: .login}"]) or {} + self_login = who.get("login") + + calibration = None + if args.calibrate and args.commits: + calibration = calibrate(args.calibrate, args.commits) + if calibration is None: + print("pr_review_batch: calibration failed; is the upstream remote fetched?", + file=sys.stderr) + + numbers = _select(args) + if not numbers: + print("pr_review_batch: no pull requests matched", file=sys.stderr) + return 1 + + briefings = [b for b in (brief(n, args.repo, self_login, args.session_since, + calibration) for n in numbers) if b] + if args.json: + print(json.dumps([b.as_dict() for b in briefings], indent=2)) + else: + for b in briefings: + print(b.render()) + print() + held = [b.number for b in briefings if b.blocks_review()] + if held: + print(f"Hold: {held} -- real CI failure or independence conflict.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/launchpad/scripts/test_no_model.py b/launchpad/scripts/test_no_model.py index b2e5fcb636f..9ff21f2f97c 100644 --- a/launchpad/scripts/test_no_model.py +++ b/launchpad/scripts/test_no_model.py @@ -70,6 +70,24 @@ # ADR-0005's boundary check. Reads tracked files for upstream-namespace # leftovers; no subprocess, no network. "adr_boundary_check.py": frozenset({"re", "sys", "pathlib"}), + # #426's batch pre-review pass. Belongs on this list rather than in + # NOT_OURS specifically BECAUSE of what it is: ADR-0019 rules that a + # deterministic script may gate a merge while a model verdict may only + # annotate, so a script that prepares review material has to be provably + # incapable of calling one. subprocess is here to spawn `gh` and `git`, + # the same permission preflight_fetch.py holds for the same reason. + "pr_review_batch.py": frozenset( + { + "__future__", + "argparse", + "datetime", + "json", + "re", + "subprocess", + "sys", + "dataclasses", + } + ), } diff --git a/launchpad/scripts/test_pr_review_batch.py b/launchpad/scripts/test_pr_review_batch.py new file mode 100644 index 00000000000..de1b0d2e484 --- /dev/null +++ b/launchpad/scripts/test_pr_review_batch.py @@ -0,0 +1,425 @@ +#!/usr/bin/env python3 +"""Controls for pr_review_batch.py -- issue #426. + +Every test here drives a pure classifier with literal fixtures. No network, no +`gh`, no git. That is deliberate: the whole point of extracting these rules from +a reviewer's head into a script is that they become testable, and a suite that +shelled out to `gh` would only be testable when GitHub agreed to cooperate. + +Each classifier's test set includes the case that was got WRONG by hand during +the 2026-08-21/22 review batches. Those are marked `# regression:` and are the +reason the corresponding rule exists at all -- if one is deleted the rule it +guards has no witness. + +Run: python3 -m unittest test_pr_review_batch (from launchpad/scripts/) + or: python3 test_pr_review_batch.py +""" + +from __future__ import annotations + +import unittest + +import pr_review_batch as m + +NOW = "2026-08-22T00:00:00Z" + + +def review(submitted_at, body="", commit_id="aaaaaaaaa", state="CHANGES_REQUESTED"): + return { + "state": state, + "submittedAt": submitted_at, + "commit_id": commit_id, + "body": body, + "author": {"login": "reviewer"}, + } + + +class ParseTimeTests(unittest.TestCase): + def test_z_suffix_and_offset_forms_both_parse(self): + a = m.parse_time("2026-08-21T03:57:00Z") + b = m.parse_time("2026-08-21T15:57:00+12:00") + self.assertEqual(a, b) + + def test_unparseable_returns_none_rather_than_raising(self): + # A classifier that crashes on a malformed timestamp fails the whole + # batch. Unknown is a verdict; a traceback is not. + self.assertIsNone(m.parse_time("not a time")) + self.assertIsNone(m.parse_time(None)) + + +class CitedPathTests(unittest.TestCase): + def test_extracts_path_line_tokens(self): + body = "found at `security_audit_tracked_files_check.py:56` and foo/bar.py:12" + self.assertEqual( + m.cited_paths(body), + {"security_audit_tracked_files_check.py", "foo/bar.py"}, + ) + + def test_ignores_bare_issue_and_pr_references(self): + # "#271" and "1.2.3" must not read as paths, or every review looks misfiled. + self.assertEqual(m.cited_paths("see #271 and version 1.2.3 and PR #275"), set()) + + def test_ignores_urls(self): + self.assertEqual(m.cited_paths("https://example.com/a.py:12"), set()) + + +class StaleReviewTests(unittest.TestCase): + def test_review_before_head_push_is_stale(self): + # regression: #262. Blockers were fixed at 03:21; the review restating + # them was submitted at 03:57 against a body written at 01:38. + verdict = m.classify_review( + review("2026-08-21T03:57:00Z"), + head_committed_at="2026-08-21T04:16:00Z", + diff_paths={"launchpad/agents/goose_config.py"}, + ) + self.assertEqual(verdict.state, "STALE") + self.assertIn("head moved", verdict.reason) + + def test_review_after_head_push_is_current(self): + verdict = m.classify_review( + review("2026-08-21T20:55:00Z"), + head_committed_at="2026-08-21T04:16:00Z", + diff_paths={"launchpad/agents/goose_config.py"}, + ) + self.assertEqual(verdict.state, "CURRENT") + + def test_review_citing_paths_absent_from_the_diff_is_misfiled(self): + # regression: #271. Its CHANGES_REQUESTED cited two files that exist + # only in #275 -- the same review posted on the wrong PR. Detected + # here by the citation/diff mismatch, not by comparing bodies. + verdict = m.classify_review( + review( + "2026-08-21T03:44:36Z", + body="blockers at `security_audit_tracked_files_check.py:56` " + "and `test_security_audit_ignore_coverage_check.py:20`", + ), + head_committed_at="2026-08-21T03:00:00Z", + diff_paths={"launchpad/scripts/security_audit_secrets_check.py"}, + ) + self.assertEqual(verdict.state, "MISFILED") + + def test_a_review_citing_evidence_outside_the_diff_is_not_misfiled(self): + # regression: the FALSE POSITIVE this script produced on live data + # before the rule was tightened. Reviewing #374, it called a genuine + # review MISFILED because the one path token in the body was + # `launchpad/ARCHITECTURE.md` -- cited as corroborating evidence for a + # quote, not as a defect site. Reviews cite files outside the diff all + # the time; that is what checking a claim looks like. + verdict = m.classify_review( + review( + "2026-08-21T21:09:53Z", + body="`launchpad/ARCHITECTURE.md:99` quotes the row verbatim. " + "The four-tier table in 355-what-the-fork-actually-operates.md " + "double-counts three files.", + ), + head_committed_at="2026-08-21T20:00:00Z", + diff_paths={"launchpad/Research/355-what-the-fork-actually-operates.md"}, + ) + self.assertEqual(verdict.state, "CURRENT") + + def test_one_cited_path_alone_is_never_misfiled(self): + # One out-of-diff citation is not a pattern, and treating it as one is + # how the false positive above happened. + verdict = m.classify_review( + review("2026-08-21T03:00:00Z", body="see `elsewhere/only.py:9`"), + head_committed_at="2026-08-21T09:00:00Z", + diff_paths={"something/else.py"}, + ) + self.assertEqual(verdict.state, "STALE") + + def test_misfiled_outranks_stale(self): + # A review that never applied cannot be "addressed by a later push". + # Reporting STALE would tell the author to expect it to clear on re-review. + verdict = m.classify_review( + review( + "2026-08-21T03:00:00Z", + body="see `only/in/other.py:9` and `also/elsewhere.py:4`", + ), + head_committed_at="2026-08-21T09:00:00Z", + diff_paths={"something/else.py"}, + ) + self.assertEqual(verdict.state, "MISFILED") + + def test_review_citing_no_paths_is_not_misfiled(self): + # Absence of citations is not evidence of misfiling -- plenty of real + # reviews are prose. Falling through to STALE/CURRENT is correct. + verdict = m.classify_review( + review("2026-08-21T03:00:00Z", body="this needs more tests"), + head_committed_at="2026-08-21T09:00:00Z", + diff_paths={"a.py"}, + ) + self.assertEqual(verdict.state, "STALE") + + def test_unknown_when_a_timestamp_cannot_be_read(self): + verdict = m.classify_review( + review("garbage"), + head_committed_at="2026-08-21T09:00:00Z", + diff_paths={"a.py"}, + ) + self.assertEqual(verdict.state, "UNKNOWN") + + def test_non_change_request_reviews_are_skipped(self): + self.assertIsNone( + m.classify_review( + review("2026-08-21T03:00:00Z", state="COMMENTED"), + head_committed_at="2026-08-21T09:00:00Z", + diff_paths={"a.py"}, + ) + ) + + +class CiTriageTests(unittest.TestCase): + def test_toolchain_fetch_timeout_is_a_flake(self): + # regression: #268. setup-mold timed out fetching the linker on a PR + # that changed one markdown file. Reporting that as REAL sends an + # author hunting for a defect in someone else's network. + v = m.classify_failure( + check_name="Desktop Core", + failed_step="Run rui314/setup-mold@9c9c13b", + log_tail="HTTP request sent, awaiting response... Read error (Connection timed out)\n" + "tar: Error is not recoverable: exiting now", + diff_paths={"launchpad/decisions/ADR-0018-cohort-relay-vps-specification.md"}, + failing_paths=set(), + ) + self.assertEqual(v.state, "FLAKE") + + def test_failure_in_files_outside_the_diff_is_pre_existing(self): + # regression: #288. Four biome items in files the PR never touched, + # inherited from upstream commits, printed ABOVE the real blocker. + v = m.classify_failure( + check_name="Desktop Core", + failed_step="Desktop lint and format", + log_tail="src/shared/styles/globals/terminal.css:276:15 lint/complexity/noImportantStyles", + diff_paths={"desktop/src-tauri/src/lib.rs"}, + failing_paths={"desktop/src/shared/styles/globals/terminal.css"}, + ) + self.assertEqual(v.state, "PRE_EXISTING") + + def test_failure_in_a_file_the_pr_touches_is_real(self): + v = m.classify_failure( + check_name="Desktop Core", + failed_step="Desktop lint and format", + log_tail="src-tauri/src/lib.rs: 1000 -> 1001 (+1) lines (allowed 1000)", + diff_paths={"desktop/src-tauri/src/lib.rs"}, + failing_paths={"desktop/src-tauri/src/lib.rs"}, + ) + self.assertEqual(v.state, "REAL") + + def test_real_outranks_pre_existing_when_both_appear(self): + # regression: #288 again. Its log contained BOTH pre-existing warnings + # and the real size-guard failure. A classifier that stopped at the + # first recognised line would have cleared the PR. + v = m.classify_failure( + check_name="Desktop Core", + failed_step="Desktop lint and format", + log_tail="terminal.css:276 noImportantStyles\n" + "- src-tauri/src/lib.rs: 1000 -> 1001 (+1) lines (allowed 1000)", + diff_paths={"desktop/src-tauri/src/lib.rs"}, + failing_paths={ + "desktop/src/shared/styles/globals/terminal.css", + "desktop/src-tauri/src/lib.rs", + }, + ) + self.assertEqual(v.state, "REAL") + + def test_no_attributable_paths_is_unknown_not_clean(self): + # Silence must not read as a pass. + v = m.classify_failure( + check_name="Security", + failed_step="audit", + log_tail="some output with no file references", + diff_paths={"a.py"}, + failing_paths=set(), + ) + self.assertEqual(v.state, "UNKNOWN") + + +class LogSelectionTests(unittest.TestCase): + """regression: taking the LAST N lines of a CI log. + + On #288 the tail was checkout teardown and the size-guard line that broke + the build had scrolled past, so the classifier answered UNKNOWN for a + failure it should have called REAL. Selection is by content now. + """ + + LOG = "\n".join([ + "Desktop Core Install desktop dependencies", + "Desktop Core + @biomejs/biome 2.4.16", + "Desktop Core - src-tauri/src/lib.rs: 1000 -> 1001 (+1) lines (allowed 1000)", + "Desktop Core error: Recipe `desktop-check` failed on line 119 with exit code 1", + "Desktop Core Post job cleanup.", + "Desktop Core [command]/usr/bin/git config --local --unset includeif.gitdir:/x", + "Desktop Core Removing credentials config '/home/runner/work/_temp/git-credentials-x'", + "Desktop Core Cleaning up orphan processes", + ]) + + def test_the_failure_line_survives_teardown_noise(self): + kept = relevant = m.relevant_log_lines(self.LOG) + self.assertIn("1000 -> 1001", kept) + self.assertIn("exit code 1", kept) + + def test_teardown_chatter_is_dropped(self): + kept = m.relevant_log_lines(self.LOG) + for noise in ("git-credentials", "orphan processes", "includeif.gitdir"): + self.assertNotIn(noise, kept) + + def test_the_real_failure_is_then_classified_real(self): + # End to end through the two functions, which is the behaviour that + # actually regressed: selection feeding classification. + kept = m.relevant_log_lines(self.LOG) + failing = {mo.group(1) for mo in m._PATH_TOKEN.finditer(kept)} + verdict = m.classify_failure( + check_name="Desktop Core", + failed_step="Desktop lint and format", + log_tail=kept, + diff_paths={"src-tauri/src/lib.rs"}, + failing_paths=failing, + ) + self.assertEqual(verdict.state, "REAL") + + def test_empty_log_yields_empty_selection(self): + self.assertEqual(m.relevant_log_lines(""), "") + self.assertEqual(m.relevant_log_lines(None), "") + + +class IndependenceTests(unittest.TestCase): + def test_own_commit_in_window_is_a_conflict(self): + # regression: #265. It carried a commit written in the reviewing + # session; the conflict was caught by hand, one review too late. + v = m.classify_independence( + commits=[ + {"oid": "05a96047", "author_login": "serina-mcfall", + "committed_at": "2026-08-21T05:05:18Z"}, + ], + self_login="serina-mcfall", + session_since="2026-08-21T00:00:00Z", + ) + self.assertEqual(v.state, "CONFLICT") + self.assertIn("05a96047", v.reason) + + def test_own_commit_before_the_window_is_independent(self): + # Her identity is on every commit in this repo, including ones written + # weeks ago by other people's sessions. Only the current window counts, + # or every PR looks conflicted and the flag becomes noise. + v = m.classify_independence( + commits=[ + {"oid": "deadbeef", "author_login": "serina-mcfall", + "committed_at": "2026-08-01T00:00:00Z"}, + ], + self_login="serina-mcfall", + session_since="2026-08-21T00:00:00Z", + ) + self.assertEqual(v.state, "INDEPENDENT") + + def test_another_authors_commit_is_independent(self): + v = m.classify_independence( + commits=[{"oid": "cafe", "author_login": "tucktuck101", + "committed_at": "2026-08-21T05:00:00Z"}], + self_login="serina-mcfall", + session_since="2026-08-21T00:00:00Z", + ) + self.assertEqual(v.state, "INDEPENDENT") + + def test_no_session_window_means_no_conflict_claimed(self): + v = m.classify_independence( + commits=[{"oid": "05a96047", "author_login": "serina-mcfall", + "committed_at": "2026-08-21T05:05:18Z"}], + self_login="serina-mcfall", + session_since=None, + ) + self.assertEqual(v.state, "UNKNOWN") + + +class LeakScanTests(unittest.TestCase): + def test_private_tooling_reference_is_found(self): + # regression: #281 quoted a private hook's header verbatim in a file + # destined for a public repository. + hits = m.scan_leaks( + 'diff --git a/x.md b/x.md\n+quotes pr-gate.sh in its header\n', + patterns=m.DEFAULT_LEAK_PATTERNS, + ) + self.assertEqual([h.pattern for h in hits], ["pr-gate"]) + + def test_only_added_lines_are_scanned(self): + # A removed reference is a fix, not a leak. Flagging it would punish + # the commit that cleaned it up. + hits = m.scan_leaks( + "diff --git a/x.md b/x.md\n-mentions pr-gate.sh\n+clean now\n", + patterns=m.DEFAULT_LEAK_PATTERNS, + ) + self.assertEqual(hits, []) + + def test_diff_header_lines_are_not_scanned(self): + # `+++ b/launchpad/scripts/verify-gate.sh` is a filename, not content. + hits = m.scan_leaks( + "--- a/a\n+++ b/launchpad/scripts/verify-gate.sh\n+ok\n", + patterns=m.DEFAULT_LEAK_PATTERNS, + ) + self.assertEqual(hits, []) + + def test_clean_diff_yields_nothing(self): + self.assertEqual( + m.scan_leaks("+just some prose\n", patterns=m.DEFAULT_LEAK_PATTERNS), [] + ) + + +class PlacementTests(unittest.TestCase): + def test_cohort_file_under_launchpad_is_allowed(self): + self.assertEqual(m.check_placement({"launchpad/Research/x.md"}), []) + + def test_cohort_file_in_upstream_docs_tree_is_flagged(self): + # AGENTS.md section 3: docs/ and root scripts/ are upstream's trees. + self.assertEqual(m.check_placement({"docs/x.md"}), ["docs/x.md"]) + + def test_root_scripts_is_flagged(self): + self.assertEqual(m.check_placement({"scripts/x.sh"}), ["scripts/x.sh"]) + + def test_upstream_owned_product_paths_are_not_flagged(self): + # A PR touching crates/ or desktop/ is upstream-shaped work, which is + # legitimate here; placement only judges cohort files. + self.assertEqual(m.check_placement({"crates/buzz-relay/src/lib.rs"}), []) + + def test_bare_md_directly_in_launchpad_agents_is_flagged(self): + # AGENTS.md: a bare .md there is scanned as a subagent roster and + # blocks the commit. Pack docs belong in a subdirectory. + self.assertEqual( + m.check_placement({"launchpad/agents/notes.md"}), + ["launchpad/agents/notes.md"], + ) + + def test_md_inside_an_agent_pack_subdirectory_is_allowed(self): + self.assertEqual( + m.check_placement({"launchpad/agents/the-professor/README.md"}), [] + ) + + +class BriefingTests(unittest.TestCase): + def test_blocking_when_a_real_failure_exists(self): + b = m.Briefing(number=1, author="x", title="t", head_sha="abc") + b.ci.append(m.Attributed("Desktop Core", m.Verdict("REAL", "size guard"))) + self.assertTrue(b.blocks_review()) + + def test_blocking_on_an_independence_conflict(self): + b = m.Briefing(number=1, author="x", title="t", head_sha="abc") + b.independence = m.Verdict("CONFLICT", "own commit 05a96047") + self.assertTrue(b.blocks_review()) + + def test_flake_and_pre_existing_do_not_block(self): + b = m.Briefing(number=1, author="x", title="t", head_sha="abc") + b.ci.append(m.Attributed("Desktop Core", m.Verdict("FLAKE", "setup-mold timeout"))) + b.ci.append(m.Attributed("Desktop", m.Verdict("PRE_EXISTING", "terminal.css"))) + self.assertFalse(b.blocks_review()) + + def test_the_briefing_states_no_severity_anywhere(self): + # The script must not emit severities -- ADR-0019 forbids a model + # verdict gating, and a script that guessed severity would be worse + # than the reviewer it replaced. This asserts the absence. + b = m.Briefing(number=1, author="x", title="t", head_sha="abc") + b.ci.append(m.Attributed("Desktop Core", m.Verdict("REAL", "something"))) + rendered = b.render().lower() + for word in ("blocker", "high", "medium", "low", "severity"): + self.assertNotIn(word, rendered, f"briefing must not grade: {word!r}") + + +if __name__ == "__main__": + unittest.main()