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