[Skills] Add deterministic preflight to the review runner

.claude/skills/flydsl-code-review/SKILL.md

@@ -3,8 +3,8 @@ name: flydsl-code-review
 description: >
   Review a FlyDSL diff, branch, commit range, or PR for correctness bugs and
   convention violations using the repository's existing skills and policy docs.
-  Uses one resumable runner to pin the reviewed tree, run nine independent review
-  angles, verify every candidate, and preserve the evidence in a structured result.
+  Uses one resumable runner to pin the reviewed tree, run deterministic checks
+  and nine independent review angles, verify every candidate, and preserve the evidence.
   Pass --comment to publish a completed PR review. Use when asked to review a diff,
   review a PR, or check changes before pushing.
 allowed-tools: Read Bash
@@ -14,7 +14,7 @@ allowed-tools: Read Bash
 
 Find real defects in a change, then prove each one before reporting it.
 
-The sole execution entry is `.claude/skills/flydsl-code-review/scripts/run_review.py`. It runs independent finders,
+The sole execution entry is `.claude/skills/flydsl-code-review/scripts/run_review.py`. It runs a deterministic preflight, independent finders,
 one verifier per candidate, a challenger for every CONFIRMED, and a fresh sweep.
 Code constructs the final ranked report directly from those records. The sections
 below supply its review method; they are not an alternative manual execution path.
@@ -63,15 +63,48 @@ Defaults are 3 concurrent agents, 600 seconds per agent and 1800 seconds per
 phase including queue time. Override with `--concurrency`, `--agent-timeout` and
 `--phase-timeout`. Ctrl-C/SIGTERM cancels child process groups. Completed stages
 and all attempt logs are checkpointed in `state.json`; `--resume` retries only
-incomplete stages with the saved scope, model and configuration. Changed runner
-or skill content requires a new run. Model and effort use the CLI defaults unless
+incomplete stages with the saved scope, model and configuration. Changed runner,
+scanner or skill content requires a new run. Model and effort use the CLI defaults unless
 the user supplies `--model`/`--effort`; do not silently select a different model.
 
 Read `result.json` after the runner exits. Exit 0 means COMPLETE; exit 1 means
 INCOMPLETE. A missing result, running process, task notification or partial
 transcript is not a completed review. Preserve the run directory when reporting
 an interruption so the user can resume it.
 
+## Deterministic preflight
+
+After pinning a nonempty diff, the runner runs two source scanners from its own
+`scripts/` directory against that diff and checkout. Both are required stages:
+exit `0` means no leads in the supported scope, `1` means leads need inspection,
+and any error or timeout makes the review INCOMPLETE. The artifact retains each
+scanner's output, exit status and run history; resume reuses completed checks.
+Neither scanner executes or imports the reviewed code.
+
+- **Angle G:** `scan_legacy_spelling.py` checks added `kernels/**/*.py` lines,
+  excluding `kernels/common/buffer_ops.py`, for raw IR/unwraps, SCF builders,
+  `buffer_ops.*`, `SmemAllocator` and `make_ptr`. It filters visible comments,
+  string literals and imports, but does not resolve API identity. Ordinary
+  pointer construction and raw IR at implementation boundaries can be valid.
+- **Angle I:** `scan_unreachable_tests.py` matches added test definition lines
+  against the head AST, follows direct local calls from `__main__`, and recognizes
+  unfiltered `pytest.main([__file__])` with common aliases. Before reporting a gap,
+  identify the actual test or benchmark command: pytest coverage and a script's
+  direct call path are different contracts.
+
+The corresponding finder receives the full raw output. Inspect the complete
+source and the applicable policy at the reviewed revision before promoting a
+lead to a candidate. Group repeated spellings with the same corrective action;
+the six-candidate limit applies to the resulting defect candidates. Raw leads
+are retained for inspection, not individually certified as bugs or as clean.
+All promoted candidates go through the same independent verifier and challenge
+rules as other candidates. An empty scan never skips the semantic review.
+
+Aliases, partial diff lexical context, dynamic dispatch, runtime branches,
+pytest selectors, fixtures, decorators and plugins can require manual review.
+See [preflight-evidence.md](references/preflight-evidence.md) for the selection
+evidence and its limits; these checks do not establish review precision or recall.
+
 ## Reusing existing skills
 
 The linked skills and policy docs own the technical rules. Each angle below

.claude/skills/flydsl-code-review/references/preflight-evidence.md

@@ -0,0 +1,26 @@
+# Why these two checks
+
+The scanners and their CPU regressions are adapted from
+[PR #1047 at `2480f877`](https://github.com/ROCm/FlyDSL/tree/2480f877aa7f74a24d1ce857b48ba92ff0699cf5/.claude/skills/flydsl-review-checks).
+They supplement the existing review runner; their output is evidence to inspect,
+not an independent review verdict or another skill entry point.
+
+That PR's [selection record](https://github.com/ROCm/FlyDSL/blob/2480f877aa7f74a24d1ce857b48ba92ff0699cf5/.claude/skills/flydsl-review-checks/references/review-evidence.md)
+reports two useful signals:
+
+- Legacy API spellings were recurring maintainer feedback, and prompt-only
+  reviewers did not reliably surface the same candidates.
+- In a small seeded comparison the baseline caught four of five candidate
+  families. Checking script entry paths supplied the missing family. A replay
+  of [#481](https://github.com/ROCm/FlyDSL/pull/481) at
+  `7e117e29c9c4582158aed33da4ee6dfe14d2c76e` identified three added fused/quant
+  tests omitted from the file's script path.
+
+Those are historical and seeded selection results, not held-out precision,
+recall, or proof of incremental benefit over this runner. Other proposed rule
+families already had baseline coverage in that experiment, so they are not
+duplicated here. Existing technical skills continue to own their semantic rules.
+
+A new deterministic check should have a reproducible positive case, a clean
+control, and evidence of coverage beyond the existing reviewer. Preserve the
+inputs and outputs before claiming a review-quality improvement.

.claude/skills/flydsl-code-review/scripts/review_common.py

@@ -12,7 +12,7 @@
 import posixpath
 import re
 
-SCHEMA_VERSION = 1
+SCHEMA_VERSION = 2
 PER_ANGLE = 6
 SWEEP_MAX = 8
 MAX_FINDINGS = 12
@@ -27,6 +27,10 @@
     ("reuse", "convention", "Angle H — reuse, simplification, and altitude"),
     ("test-doc", "convention", "Angle I — test and documentation contract"),
 )
+PREFLIGHTS = (
+    ("preflight:conventions", "scan_legacy_spelling.py"),
+    ("preflight:test-doc", "scan_unreachable_tests.py"),
+)
 VERDICTS = ("CONFIRMED", "PLAUSIBLE", "REFUTED")
 SEVERITIES = ("P0", "P1", "P2", "P3")
 
@@ -87,6 +91,14 @@ def stage_output(stages: dict, label: str) -> dict | None:
     return stage.get("output") if stage.get("status") == "COMPLETE" else None
 
 
+def validate_preflight(output: dict) -> dict:
+    if not isinstance(output, dict) or type(output.get("exit_code")) is not int or output["exit_code"] not in (0, 1):
+        raise ValueError("completed preflight must have scanner exit code 0 or 1")
+    if not all(isinstance(output.get(key), str) for key in ("stdout", "stderr")):
+        raise ValueError("completed preflight must retain scanner stdout and stderr")
+    return output
+
+
 def collect_candidates(stages: dict) -> list[dict]:
     """Exact location AND mechanism, after every finder has had a chance to finish."""
     unique: dict[str, dict] = {}
@@ -152,6 +164,7 @@ def rank_findings(candidates: list[dict]) -> list[dict]:
 def required_stages(scope: dict | None, candidates: list[dict]) -> list[str]:
     labels = ["scope"]
     if scope and scope.get("files"):
+        labels += [label for label, _ in PREFLIGHTS]
         labels += ["find:" + label for label, _, _ in ANGLES]
         labels += ["verify:" + c["id"] for c in candidates]
         labels += [
@@ -195,6 +208,13 @@ def build_report(state: dict) -> dict:
         for label in required
         if stage_output(stages, label) is None
     ]
+    for label, _ in PREFLIGHTS:
+        output = stage_output(stages, label)
+        if label in required and output is not None:
+            try:
+                validate_preflight(output)
+            except ValueError as exc:
+                failed.append({"stage": label, "reason": str(exc)})
     # Integrity checks and cancellation can fail outside an agent stage.
     failed += [
         {"stage": label, "reason": stage.get("error", "stage failed")}

.claude/skills/flydsl-code-review/scripts/run_review.py

@@ -37,6 +37,7 @@
 from review_common import (
     ANGLES,
     PER_ANGLE,
+    PREFLIGHTS,
     SCHEMA_VERSION,
     SEVERITIES,
     SWEEP_MAX,
@@ -49,6 +50,7 @@
     normalize_path,
     stage_output,
     validate_output,
+    validate_preflight,
 )
 
 SCRIPTS = Path(__file__).resolve().parent
@@ -65,13 +67,13 @@ def atomic_json(path: Path, value: dict) -> None:
     temporary.replace(path)
 
 
-def command(
+def command_result(
     *argv: str,
     cwd: Path,
     stdin: str | None = None,
     deadline: float | None = None,
     cancelled: threading.Event | None = None,
-) -> str:
+) -> subprocess.CompletedProcess:
     deadline = min(deadline or float("inf"), time.monotonic() + 120)
     if (cancelled and cancelled.is_set()) or time.monotonic() >= deadline:
         raise TimeoutError("command cancelled or phase deadline exceeded")
@@ -96,9 +98,14 @@ def command(
                 first = False
     finally:
         stop_process(process)
-    if process.returncode:
-        raise RuntimeError(f"{shlex.join(argv)}: {stderr.strip()}")
-    return stdout
+    return subprocess.CompletedProcess(argv, process.returncode, stdout, stderr)
+
+
+def command(*argv: str, **options) -> str:
+    result = command_result(*argv, **options)
+    if result.returncode:
+        raise RuntimeError(f"{shlex.join(argv)}: {result.stderr.strip()}")
+    return result.stdout
 
 
 def git(root: Path, *args: str, **options) -> str:
@@ -255,6 +262,8 @@ def check_snapshot(run_dir: Path, scope: dict, **control) -> None:
     )
     if hashlib.sha256(diff.encode()).hexdigest() != scope["diff_sha256"]:
         raise ValueError("pinned diff hash changed")
+    if hashlib.sha256((run_dir / "diff.patch").read_bytes()).hexdigest() != scope["diff_sha256"]:
+        raise ValueError("saved diff.patch does not match the pinned diff")
 
 
 def section(text: str, title: str) -> str:
@@ -366,6 +375,17 @@ def cli_agent(
                 # Also reap tool descendants if the CLI exited without waiting for them.
                 stop_process(process)
         envelope = json.loads(stdout_path.read_text())
+        # Verbose CLI settings emit a transcript array instead of one result.
+        # The last result owns the verdict and usage, including a failed result.
+        if isinstance(envelope, list):
+            envelope = next(
+                (
+                    record
+                    for record in reversed(envelope)
+                    if isinstance(record, dict) and record.get("type") == "result"
+                ),
+                None,
+            )
         if not isinstance(envelope, dict) or envelope.get("type") != "result":
             raise ValueError("CLI returned no terminal result record")
         attempt["usage"] = {
@@ -434,6 +454,61 @@ def context(self) -> str:
     def task(self, label: str, prompt: str, limit: int | None = None) -> dict:
         return {"label": label, "prompt": self.context() + prompt, "limit": limit, "schema": output_schema(limit)}
 
+    def preflight(self) -> bool:
+        """Run trusted scanners over the pinned data; their matches are only leads."""
+        stages = self.state["stages"]
+        scope = self.state["scope"]
+        deadline = time.monotonic() + self.config["phase_timeout"]
+        for label, script in PREFLIGHTS:
+            fingerprint = digest(
+                {"head": scope["head_oid"], "diff": scope["diff_sha256"], "script": (SCRIPTS / script).read_text()}
+            )
+            stage = stages.setdefault(label, {"runs": []})
+            if stage.get("status") == "COMPLETE":
+                validate_preflight(stage.get("output"))
+                if stage.get("input_sha256") != fingerprint:
+                    raise ValueError(f"cached input changed for {label}; start a new run")
+                continue
+            for prior in stage["runs"]:
+                if prior["status"] == "RUNNING":
+                    prior.update(status="INCOMPLETE", error="parent stopped before recording the scanner result")
+            stage.update(status="RUNNING", output=None, input_sha256=fingerprint)
+            stage.pop("error", None)
+            record = {"status": "RUNNING"}
+            stage["runs"].append(record)
+            self.save()
+            argv = [sys.executable, str(SCRIPTS / script), "--diff", str(self.run_dir / "diff.patch")]
+            if label == "preflight:test-doc":
+                argv += ["--head", str(self.snapshot)]
+            print(f"{label}: scanning pinned diff", file=sys.stderr, flush=True)
+            started = time.monotonic()
+            try:
+                result = command_result(*argv, cwd=self.snapshot, deadline=deadline, cancelled=self.cancelled)
+                record.update(exit_code=result.returncode, stdout=result.stdout, stderr=result.stderr)
+                if result.returncode not in (0, 1):
+                    raise ValueError(f"scanner exited {result.returncode}: {result.stderr.strip()}")
+                record["status"] = "COMPLETE"
+                stage["output"] = {key: record[key] for key in ("exit_code", "stdout", "stderr")}
+            except (OSError, ValueError, TimeoutError) as exc:
+                record.update(status="INCOMPLETE", error=str(exc))
+                stage["error"] = str(exc)
+            record["wall_time_seconds"] = time.monotonic() - started
+            stage["status"] = record["status"]
+            self.save()
+        return all(stage_output(stages, label) is not None for label, _ in PREFLIGHTS)
+
+    def preflight_context(self, angle: str) -> str:
+        output = stage_output(self.state["stages"], "preflight:" + angle)
+        if output is None:
+            return ""
+        return (
+            "\nDeterministic preflight observations (unverified leads, not findings):\n"
+            + canonical(output)
+            + "\n"
+            + section(self.skill, "Deterministic preflight")
+            + "\n"
+        )
+
     def phase(self, name: str, tasks: list[dict]) -> bool:
         print(f"{name}: {len(tasks)} stage(s)", file=sys.stderr, flush=True)
         stages = self.state["stages"]
@@ -555,11 +630,14 @@ def review(self) -> None:
             cancelled=self.cancelled,
         )
         if self.state["scope"]["files"]:
+            if not self.preflight():
+                return
             finders = [
                 self.task(
                     "find:" + label,
                     "Review only this angle:\n"
                     + section(self.skill, title)
+                    + self.preflight_context(label)
                     + f"\nReturn up to {PER_ANGLE} candidates with a specific mechanism/root cause, "
                     "severity (P0 critical, P1 high, P2 normal, P3 low), exact file/line and failure scenario. "
                     "Pass every candidate with a nameable failure scenario to independent verification. "
@@ -570,7 +648,7 @@ def review(self) -> None:
             ]
             if not self.phase("Find", finders):
                 return
-            # No completion-order admission and no verification budget: verify all <=54 candidates.
+            # No completion-order admission or verification budget: verify every finder candidate.
             candidates = collect_candidates({k: v for k, v in stages.items() if k != "sweep"})
             if not self.verify(candidates):
                 return
@@ -616,7 +694,8 @@ def run(self) -> dict:
 
 
 def implementation_hash() -> str:
-    return digest([p.read_text() for p in (Path(__file__), SCRIPTS / "review_common.py", SKILL)])
+    paths = [Path(__file__), SCRIPTS / "review_common.py", SKILL, *(SCRIPTS / script for _, script in PREFLIGHTS)]
+    return digest([p.read_text() for p in paths])
 
 
 def positive(value: str) -> int: