diff --git a/.claude/skills/add-target-atom-op/SKILL.md b/.claude/skills/add-target-atom-op/SKILL.md index 445fab5ae..c07d17974 100644 --- a/.claude/skills/add-target-atom-op/SKILL.md +++ b/.claude/skills/add-target-atom-op/SKILL.md @@ -75,11 +75,11 @@ methods — the wrapper and the kernel-level ops (`fly.mma_atom_call`, `fly.copy `include/flydsl/Dialect/Fly/IR/FlyInterfaces.td`: -| Interface | Required for... | Methods — **mandatory** / *optional* (see §1.3) | +| Interface | Required for... | Methods — **mandatory** / pipeline-dependent (see §1.3) | |-----------------------------------|-----------------|----------------------| -| `Fly_MayStaticTypeInterface` | Stateless atoms (CopyOp with *no* mutable state; all MmaOps today) | **`isStatic`**, **`rebuildStaticValue`** | -| `Fly_CopyOpTypeInterface` | All CopyOps | **`getThrLayout`**, **`getThrBitLayoutSrc/Dst/Ref`**, **`emitAtomCall`** (mem + pred), *`emitAtomCallSSA`* (mem + pred — only if `fly-convert-atom-call-to-ssa-form` is in the pipeline) | -| `Fly_MmaOpTypeInterface` | All MmaOps | **`getThrLayout`**, **`getShapeMNK`**, **`getValTypeA/B/C/D`**, **`getThrValLayoutA/B/C`**, **`emitAtomCall`**, *`emitAtomCallSSA`* (only if SSA-promotion pass is active) | +| `Fly_MayStaticTypeInterface` | Stateless atoms (CopyOp or MmaOp with no mutable state) | **`isStatic`**, **`rebuildStaticValue`** | +| `Fly_CopyOpTypeInterface` | All CopyOps | **`getThrLayout`**, **`getThrBitLayoutSrc/Dst/Ref`**, **`emitAtomCall`** (mem + pred), `emitAtomCallSSA` when a legal call is SSA-reachable | +| `Fly_MmaOpTypeInterface` | All MmaOps | **`getThrLayout`**, **`getShapeMNK`**, **`getValTypeA/B/C/D`**, **`getThrValLayoutA/B/C`**, **`emitAtomCall`**, `emitAtomCallSSA` when a legal call is SSA-reachable | | `Fly_StatefulOpTypeInterface` | Atoms that carry mutable per-call state (e.g. `soffset`, `imm_offset`) | **`getConvertedType`**, **`getDefaultState`**, **`setAtomState`** | Backend dialect could provide four convenience base classes that pre-declare the right interface @@ -96,7 +96,7 @@ class FlyROCDL_StatefulMmaOp // stateful MmaOp : MmaOp + Stateful Mnemonic: **stateful => no `MayStaticTypeInterface`**; the mutable state *is* the dynamic component, so the type is never "fully static" in the canonical-rebuild sense. -### 1.3 `emitAtomCall` vs `emitAtomCallSSA` — only `emitAtomCall` is mandatory +### 1.3 `emitAtomCall` and `emitAtomCallSSA` are one semantic contract Two kernel-IR ops carry the atom invocation, and they correspond to the two interface methods: @@ -104,16 +104,15 @@ Two kernel-IR ops carry the atom invocation, and they correspond to the two inte |--------------------------|-----------------------------------------------|-----------------------|-----------------------| | `fly.copy_atom_call` | `src/dst : !fly.memref<...>` | `emitAtomCall` | **Required** | | `fly.mma_atom_call` | `a/b/c/d : !fly.memref<...>` | `emitAtomCall` | **Required** | -| `fly.copy_atom_call_ssa` | `src/dst : SSA value or !fly.memref<..., addressSpace != Register>` | `emitAtomCallSSA` | **Optional** — only needed if `fly-convert-atom-call-to-ssa-form` appears in the pipeline | -| `fly.mma_atom_call_ssa` | `a/b/c : SSA value or !fly.memref<..., addressSpace != Register>` | `emitAtomCallSSA` | **Optional** (same condition) | +| `fly.copy_atom_call_ssa` | promoted SSA values plus retained non-register memrefs | `emitAtomCallSSA` | Required when FlyROCDL's default SSA pass can rewrite a legal call | +| `fly.mma_atom_call_ssa` | promoted SSA values plus retained non-register memrefs | `emitAtomCallSSA` | Required when FlyROCDL's default SSA pass can rewrite a legal call | -**Default path (memref / `emitAtomCall`).** Every `fly.copy_atom_call` / `fly.mma_atom_call` in the -IR lowers through `emitAtomCall`. The Op receives the operand *pointers* into register memory +**Memref path (`emitAtomCall`).** An unpromoted `fly.copy_atom_call` / `fly.mma_atom_call` +lowers through `emitAtomCall`. The Op receives operand pointers/memrefs (`!fly.memref<..., register, layout>`), is expected to issue `llvm.load` / `llvm.store` itself to -read/write threads' registers, and emit the backend intrinsic in between. This is sufficient for the -full compile-to-binary pipeline — no SSA version required. +read/write register operands, and emits or delegates to the backend operation. -**Optional path (SSA / `emitAtomCallSSA`).** A pipeline may insert the +**SSA path (`emitAtomCallSSA`).** A pipeline may insert the `fly-convert-atom-call-to-ssa-form` pass (see `lib/Dialect/Fly/Transforms/ConvertAtomCallToSSAForm.cpp`). That pass inspects every `AtomCall` and, for operands whose `register`-address-space memref has a **coalescable** layout @@ -126,34 +125,33 @@ for operands whose `register`-address-space memref has a **coalescable** layout 3. For output-producing cases, a `PtrStoreOp` writes the SSA result back to the original register memref. -At lowering time, `AtomCallSSA` dispatches to `emitAtomCallSSA` instead of `emitAtomCall`. The Op's -job there is **just the intrinsic + any required `LLVM::BitcastOp` between the SSA `vector<...>` and -the intrinsic's expected packed type** — no loads or stores because the SSA values already live in -registers. +At lowering time, `AtomCallSSA` dispatches to `emitAtomCallSSA`. Only eligible +register operands become SSA; global/shared/buffer memrefs remain, so this can be +a mixed form. A payload may bitcast SSA vectors and emit the intrinsic directly, +or delegate to memref lowering when the operation still consumes memrefs. **Concrete differences between the two methods:** | | `emitAtomCall` | `emitAtomCallSSA` | |--------------------------|-------------------------------------------------|-------------------------------------------------| -| Operand kinds | `Value`s of type `!fly.memref<..., register>` (lowered to `!llvm.ptr`) | `Value`s of scalar / `vector` type | -| What the method does | `LLVM::LoadOp` to fetch operands → intrinsic → `LLVM::StoreOp` to write result | (optional bitcast to intrinsic's packed type) → intrinsic → return `Value` / `failure` | +| Operand kinds | `Value`s of `!fly.memref<...>` types | promoted scalar/vector values plus retained non-register memrefs | +| What the method does | Load/store register operands and emit/delegate the operation | Lower the reachable full/mixed form; may bitcast and emit or delegate to memref form | | Return type | `LogicalResult` | `FailureOr` (the result SSA value, or `failure`) | | Needs layout/cosize info | No — operand type already carries it | No — caller already packed operands into `vector` | | Bitcast dance | Typically unnecessary (load yields the right type) | Often necessary (SSA vector width may not match intrinsic's expected operand width) | | Backend intrinsic emitted | Same | Same | -In practice every reference Op implements `emitAtomCall` as a thin shim over `emitAtomCallSSA` — -load operands, call `emitAtomCallSSA`, store the result. See `MmaOpCDNA3_MFMAType::emitAtomCall` in -`CDNA3/MmaAtom.cpp` for the canonical shim and `CopyOpCDNA3BufferAtomicType::emitAtomCall` in -`CDNA3/CopyAtom.cpp` for a CopyOp instance. **If your downstream pipeline never runs -`fly-convert-atom-call-to-ssa-form`, you may skip `emitAtomCallSSA` entirely and write a -self-contained `emitAtomCall`** — but the shim pattern is strictly better because it keeps the two -paths in sync for free. +FlyROCDL's default pipeline always runs `fly-convert-atom-call-to-ssa-form`, +so each payload must handle every form its legal calls can reach. Sharing may go +either direction: CDNA3 MFMA loads then calls SSA, while async/TDM copies can +delegate their SSA entry back to memref lowering. A different backend may omit +SSA support only when its complete pipeline cannot create that call form. ### 1.4 ThrVal layouts describe the per-thread register footprint -Every MmaOp / CopyOp must publish layouts that describe *which thread holds which element* of the -tile. This is consumed by `TiledCopy` / `TiledMma` in the layout-lowering pass. +Every MmaOp / CopyOp publishes layouts consumed by `TiledCopy` / `TiledMma`. +Value-granular atoms describe which thread holds each element; whole-tile target +operations may instead publish a documented sentinel layout interpreted by their payload. | Method (MmaOp) | What it describes | |------------------------|-------------------| @@ -165,7 +163,7 @@ tile. This is consumed by `TiledCopy` / `TiledMma` in the layout-lowering pass. | Method (CopyOp) | What it describes | |----------------------------|-------------------| | `getThrLayout` | thread count participating in one atom call | -| `getThrBitLayoutSrc/Dst/Ref` | layout in **bit-granularity** — shape is `(num_threads, num_bits)` — one bit per leaf | +| `getThrBitLayoutSrc/Dst/Ref` | value-granular copies use bit layouts; whole-tile payloads may use a sentinel contract | The base `CopyAtomType::getThrValLayoutSrc()` then "recasts" the bit layout into a `valBits`-granularity layout (see `CopyAtomType::getThrValLayout{Src,Dst,Ref}` in @@ -181,7 +179,7 @@ Use the `FxLayout / FxShape / FxStride / FxThr / FxVal / FxC` macros from A wrong ThrVal/ThrBit layout is the #1 source of silent-wrong-result bugs in FlyDSL: the compiler accepts it, the kernel runs, and the output is garbage. There are no good runtime diagnostics for -this. Before you commit any new `getThrValLayout*` / `getThrBitLayout*`, verify **every** rule below +this. Before you commit any new `getThrValLayout*` / `getThrBitLayout*`, verify every applicable rule below on paper or in a scratch test. #### 1.5.1 Shape must be a top-level 2-tuple `((thr...), (val...))` @@ -211,29 +209,34 @@ Let `|·|` denote "total number of elements". Then: | MmaOp | `getThrValLayoutC` (and D) | `\|thr\| * \|val\|` == `M * N` | | MmaOp | `\|thr\|` of ThrValLayout{A,B,C} | matches `\|thr\|` of `getThrLayout` | | MmaOp | `\|val\|` of ThrValLayout | matches the thread's register vector width used in `emitAtomCallSSA` (e.g. `accVecSize` for C; `vecSize` of `abTyA` for A) | -| CopyOp | `getThrLayout` | `\|thr\|` == number of threads participating in one atom call (e.g. 1 for a per-thread load, 16 for AMD `ds_read_tr16_b64`) | -| CopyOp | `getThrBitLayoutSrc/Dst/Ref` | `\|val\|` == `bitSize` (the Op's `bitSize` parameter or per-atom constant). Shape is always `(|thr|, bitSize)`. | -| CopyOp | `\|thr\|` of ThrBitLayout{Src,Dst,Ref} | all three equal and equal to `\|thr\|` of `getThrLayout` | +| Value-granular CopyOp | `getThrLayout` | `\|thr\|` == participating threads (e.g. 1 for a per-thread load, 16 for `ds_read_tr16_b64`) | +| Value-granular CopyOp | `getThrBitLayoutSrc/Dst/Ref` | `\|val\|` == the payload's `bitSize`; all three thread modes match `getThrLayout` | +| Whole-tile CopyOp | layout methods | Follow the payload's documented operation-level contract; do not infer a `bitSize` footprint | -Violating any of these still compiles but yields undefined behavior. Thread-count mismatch is +Violating an applicable invariant can still compile but yield undefined behavior. Thread-count mismatch is especially insidious: a wave64 MFMA registered with `FxC(32)` (or a 32-thread NVIDIA warp MMA registered with `FxC(16)`) will happily emit the intrinsic, but half the threads will compute on stale registers. -#### 1.5.3 Reference coordinate system is *column-major*, not row-major +#### 1.5.3 Value-granular reference coordinates are *column-major* | Op | Operand | Reference tile | Column-major interpretation | |----------|---------|----------------|-----------------------------| | MmaOp | A | `(M, K)` | stride `(1, M)` is baseline | | MmaOp | B | `(N, K)` | stride `(1, N)` is baseline | | MmaOp | C, D | `(M, N)` | stride `(1, M)` is baseline | -| CopyOp | src/dst | `(M, N)` | stride `(1, M)` is baseline | +| Value-granular CopyOp | src/dst | `(M, N)` | stride `(1, M)` is baseline | + +Whole-tile CopyOps do not use this `(M,N)` table; follow the payload's own +rank, dimension, state, and coordinate contract. #### 1.5.4 CopyOp bit-layout vs. value-layout — do not confuse them -The interface publishes `getThrBitLayout*` (bit granularity); the `CopyAtomType` wrapper computes -`getThrValLayout*` by calling `layoutRecast(bitLayout, /*oldBits=*/1, /*newBits=*/valBits)` (see -`CopyAtomType::getThrValLayout{Src,Dst,Ref}` in `FlyTypeDefs.cpp`). +For value-granular copies, the interface publishes `getThrBitLayout*` and the +`CopyAtomType` wrapper computes `getThrValLayout*` with +`layoutRecast(bitLayout, /*oldBits=*/1, /*newBits=*/valBits)`. Whole-tile +operations such as GFX1250 TDM can use a `(1,1)` sentinel and carry no payload +`bitSize`; validate the payload consumer instead of applying the recast footprint rules. Consequences: - A 32b buffer copy writes `FxShape(FxC(1), FxC(32))` for an f32 → the recast at `valBits=32` trivially @@ -328,8 +331,8 @@ it. don't support, with a clear `emitError()` message; otherwise an invalid config silently hits `return failure()` in `emitAtomCallSSA` with no diagnostic. -**Step 4 — `emitAtomCallSSA`** (optional, only needed when `fly-convert-atom-call-to-ssa-form` is in -the pipeline; see §1.3). The only place you touch backend intrinsics. Pattern from +**Step 4 — `emitAtomCallSSA`** (required when FlyROCDL can create this form; see §1.3). +For a direct SSA implementation this is the backend intrinsic boundary. Pattern from `MmaOpCDNA3_MFMAType::emitAtomCallSSA` in `CDNA3/MmaAtom.cpp`: derive the intrinsic's exact operand types, `LLVM::BitcastOp` each SSA operand to match, then dispatch to lowered dialect ops. Find intrinsic names in `llvm/include/llvm/IR/IntrinsicsAMDGPU.td` (ROCDL), `NVVMOps.td` (NVVM), or @@ -410,8 +413,10 @@ simple per-thread copy: `FxLayout(FxShape(FxC(1), FxC(getBitSize())), FxStride(F Src/Dst/Ref. If Src ≠ Dst (e.g. LDS-read-transpose), Ref usually mirrors the register side — see `CDNA4/CopyAtom.cpp`. All three layouts must satisfy the invariants in §1.5. -**Step 4 — `emitAtomCallSSA`** (optional, see §1.3). Extract state fields with -`LLVM::ExtractValueOp`, then dispatch to the backend intrinsic. Pattern: the unpredicated +**Step 4 — `emitAtomCallSSA`** (required when FlyROCDL can create this form; see §1.3). +Lower the reachable operands directly or delegate to the memref form. For a direct +implementation, extract state fields with `LLVM::ExtractValueOp` and dispatch to +the backend intrinsic. Pattern: the unpredicated `CopyOpCDNA3BufferCopyType::emitAtomCallSSA` overload in `CDNA3/CopyAtom.cpp`. **Step 5 — Predicated SSA variant.** Wrap the unpredicated form in `scf::IfOp` — load side yields @@ -447,9 +452,34 @@ automatically via `AtomSetValueOp`. --- +## 6. Review and verification + +Treat construction, lowering, final instruction selection, and device behavior +as separate evidence boundaries. + +- The TableGen verifier's accepted parameter set must equal the configurations + handled by layout construction and `emitAtomCall{SSA}`. Add a positive case + for each new dispatch branch and a negative case at each accepted/rejected + boundary; a constructible atom that falls through during conversion is a bug. +- Derive reachable call forms from the SSA pass's eligibility and each operand's + address space. Cover unpromoted memref and every reachable full/mixed SSA form; + do not require an impossible pure-SSA form for whole-tile or async copies. + Copy changes also cover predicates; stateful atoms cover changed state fields. + For MMA, exactly one layer must write D in each reachable form. +- Build a matrix from distinct target/ABI, layout, dtype/packing, shape, + state/modifier, and call-path branches. Use representative and interacting + pairs rather than a Cartesian product, plus a sibling-target control for + shared changes. +- FileCheck proves only the checked intermediate IR. Inspect normalized final + ISA for opcode, operand order, and modifiers; use resource diff only for + resource claims. Layout, signedness, packing, state, or predicate changes also + require a numerical test on matching hardware against an independent oracle. + +--- + ### Recommended reading order -Files (4) and (8) are target-neutral; the rest are ROCDL templates a new backend mirrors in its own +Files (4) and (7) are target-neutral; the rest are ROCDL templates a new backend mirrors in its own tree. 1. `include/flydsl/Dialect/FlyROCDL/IR/Dialect.td` — base classes diff --git a/.claude/skills/flydsl-code-review/SKILL.md b/.claude/skills/flydsl-code-review/SKILL.md new file mode 100644 index 000000000..62d3134b0 --- /dev/null +++ b/.claude/skills/flydsl-code-review/SKILL.md @@ -0,0 +1,499 @@ +--- +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 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 +--- + +# FlyDSL Code Review + +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 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. + +## Invocation + +```text +/flydsl-code-review # current branch vs main, including local changes +/flydsl-code-review HEAD~3 # a ref or ref range +/flydsl-code-review 1100 # a PR number +/flydsl-code-review kernels/attention/ # restrict to a path +/flydsl-code-review focus on the LDS changes # free-form instruction +/flydsl-code-review 1100 --comment # publish confirmed P0/P1 findings +/flydsl-code-review 1100 --comment --publish-severity P0 +``` + +Pass only review scope and runner options to `run_review.py`; `--comment` and +`--publish-severity` belong to `post_review.py` after a COMPLETE result. Honor +user scope restrictions verbatim. Use `--instructions` for free-form focus, +`--path` for paths, and explicit `--base`/`--head` for a frozen comparison. + +## Step 1 — Run the review + +Invoke the runner with Bash from the repository root: + +```bash +python3 .claude/skills/flydsl-code-review/scripts/run_review.py 1100 +python3 .claude/skills/flydsl-code-review/scripts/run_review.py --base HEAD~3 --head HEAD +python3 .claude/skills/flydsl-code-review/scripts/run_review.py --path kernels/attention --instructions 'focus on LDS' +python3 .claude/skills/flydsl-code-review/scripts/run_review.py --resume /tmp/flydsl-review- +``` + +Do not invoke Workflow or improvise an inline Agent sequence. The runner requires +Python 3.10+, Git, and Claude Code CLI with `--json-schema`; PR targets also need +authenticated `gh`. If it cannot run, report INCOMPLETE with the actual error. + +The runner prints its run ID and directory immediately. Its default directory is +under `/tmp`; use `--run-dir ` outside the checkout for +longer-lived artifacts. +It copies the required Git objects into an independent checkout, fixes base, +merge-base and head OIDs, and hashes the diff. Default branch/path reviews include +local tracked and untracked changes in a synthetic commit there. Explicit refs +and PRs review committed trees. Every agent reads that checkout and fixed diff; +read the enclosing functions as well as changed lines. No phase rereads a moving +PR diff or the caller's working tree. + +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, +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. +The runner requests `--json` and requires a COMPLETE result whose exit code +matches the process exit code. An exception, missing/malformed result or timeout +makes the review INCOMPLETE; exit code `1` alone never proves success. Artifact +schema v4 requires this explicit completion record and verified severity, so older results must be rerun. +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 +selects the relevant sources; read their applicable sections at the reviewed +revision before judging a candidate. Follow their conditions, exceptions, and +semantic constraints rather than treating the angle's topic labels as rules. +If linked guidance disagrees, check the policy and implementation at that +revision before reporting. + +Reuse their analysis or check-only procedures within the scope from Step 1, +leaving the reviewed files unchanged. Authoring, migration, formatting fixes, and +intrusive debugging are separate tasks. Use this skill's candidate format, +verification, and final report instead of concatenating standalone skill reports. +A scoped review does not establish a full API audit's PASS or STABLE-ONLY result. + +For a finding based on a shared rule, cite the source file and section in +`failure_scenario`, alongside the code evidence and concrete consequence. +Verifiers and challengers must read that source and check its applicability. + +## Severity and publication + +Severity measures impact **if the candidate is true**; verdict measures confidence. Finders propose +severity, but the verifier owns it and a challenger may only keep or lower it. + +- **P0** — broad critical failure: widespread silent corruption, an unsafe boundary, or a primary path unusable with no practical escape. +- **P1** — concrete merge blocker on a supported path: wrong output, OOB, crash/hang, API break, required CI failure, or measured contract regression. +- **P2** — real but non-blocking localized defect, unproven performance concern, or bounded diagnostic/test/documentation/convention gap. +- **P3** — optional cleanup or maintainability improvement with no demonstrated present correctness, compatibility, CI, or performance effect. + +`result.json` retains every candidate and adjudication. GitHub publication defaults to CONFIRMED +P0/P1, filtering the full verified set before its 12-item cap. `--publish-severity +P0|P1|P2|P3` changes the threshold; PLAUSIBLE and lower severities remain artifact-only. + +## Step 2 — Run the nine angles + +The runner starts nine independent finders, **up to 6 candidates each**, one +angle per agent. It collects every result before admission. Do not let one angle's +conclusions suppress another's: if two angles flag the same line for different +reasons, record both. Each candidate needs a repository-relative `file`, a positive +integer `line` (or null), a one-line `summary`, a specific `mechanism`/root cause, +`severity` (P0–P3), and a concrete `failure_scenario`. + +Angles A–F hunt correctness bugs. Angles G–I hunt convention violations and +cleanup; for those, `failure_scenario` states the concrete cost (what breaks in +CI, what is duplicated, what becomes arch-fragile) rather than a crash. + +## Angle A — trace-time vs runtime semantics + +Read the **flydsl-kernel-authoring** skill +([SKILL.md](../flydsl-kernel-authoring/SKILL.md)), §3: **Control Flow**, +**Runtime vs Compile-Time Conditions**, **Frontend Semantic Restrictions**, and +**Runtime Loops with Loop-Carried Values**. The **debug-flydsl-kernel** skill +([SKILL.md](../debug-flydsl-kernel/SKILL.md)), §6 **Compilation Errors**, supplies +concrete failure patterns. + +Trace values from their definitions to uses across branches, helpers, and loop +boundaries. Identify where the changed code violates those frontend contracts. + +## Angle B — memory addressing and out-of-bounds + +Use the **oob-detection** skill ([SKILL.md](../oob-detection/SKILL.md)), §1 +**Classify the OOB** and §2 **Static Interval Analysis**, plus §4's layout and +integer-overflow guidance. For raw buffer access, read the offset contract in the +**kernel-code-cleanup** skill ([SKILL.md](../kernel-code-cleanup/SKILL.md)), §2. + +Apply the analysis to changed accesses and their corresponding writer/reader +layouts. Connect the failing range to an observable result under Step 3. + +## Angle C — synchronization, LDS, and value lifetime + +Read the sources relevant to the changed synchronization or storage: + +- **debug-flydsl-kernel** skill ([SKILL.md](../debug-flydsl-kernel/SKILL.md)), + §7.2 **Barrier deadlock**. +- **lds-optimization** skill ([SKILL.md](../lds-optimization/SKILL.md)), + **LDS Instruction Model**, for dependency and cross-wave synchronization. +- **flydsl-tile-programming** skill ([SKILL.md](../flydsl-tile-programming/SKILL.md)), + **Step 5: Add Synchronization**, for target-specific wait operations. +- **kernel-code-cleanup** skill ([SKILL.md](../kernel-code-cleanup/SKILL.md)), + §3c and §4, for wait-counter migration constraints and shared-view lifetime. +- **flydsl-kernel-authoring** skill ([SKILL.md](../flydsl-kernel-authoring/SKILL.md)), + §5 **Shared Memory (LDS)**, and [CLAUDE.md](../../../CLAUDE.md)'s + **GPU Architecture Support** and **Kernel Authoring Conventions**, for + allocation, launch, and capacity contracts. + +Trace producer/consumer ordering and value lifetime across branches, loops, +pipeline stages, and the launch boundary. + +## Angle D — architecture and atom contracts + +Read [CLAUDE.md](../../../CLAUDE.md)'s **GPU Architecture Support** and the +**flydsl-kernel-authoring** skill ([SKILL.md](../flydsl-kernel-authoring/SKILL.md)), +§6 **MFMA Integration**, for target capabilities and operand contracts. Reuse the +**kernel-code-cleanup** skill ([SKILL.md](../kernel-code-cleanup/SKILL.md)), +§3b and §§6–7, for pointer boundaries, fragments, and TV layouts. + +When the diff implements backend atoms, also apply the **add-target-atom-op** +skill ([SKILL.md](../add-target-atom-op/SKILL.md)), §1 **Inherent Design** and +§6 **Review and verification**. +Check lane math, dtype support, dispatch, and operand/layout assumptions against +every target the changed code claims to support. + +### Compiler target decisions + +For compiler-side target selection, identify each target property's owner and every architecture +admitted by the predicate. Compare wave size, dialect/intrinsic support, address-space mapping, and +pass options with the selected backend and `CLAUDE.md`; do not infer one property from another +classification. Name an admitted target and the wrong emitted IR, option, diagnostic, ISA, or result. + +## Angle E — removed-behavior auditor + +For every line the diff **deletes or replaces**, name the invariant or behavior +it enforced, then search the new code for where that invariant is +re-established. If you cannot find it, that is a candidate. + +In this repo the recurring instances are: a dropped bounds guard or mask; a +removed `s_waitcnt` or `gpu.barrier()`; a NaN or divide-by-zero guard removed +during a refactor; a narrowed dtype or arch validation; a `.mlir` FileCheck line +deleted rather than updated; a test case deleted because it started failing. + +## Angle F — cross-layer tracer + +Two directions. + +**Horizontal.** For each changed function, search its callers and callees for +assumptions the diff invalidates, including interactions between changed +functions. Report concrete broken call sites; Angle G owns public-API stability +classification. + +**Vertical.** Trace changed operations through definitions, lowering, bindings, +and consumers. Use the **flydsl-kernel-authoring** skill +([SKILL.md](../flydsl-kernel-authoring/SKILL.md)), §1 **Architecture and +Compilation**, as the layer map. For atom changes, use the **add-target-atom-op** +skill ([SKILL.md](../add-target-atom-op/SKILL.md)), §2 **The Files You Will +Touch** and the applicable recipe's integration and verification steps. + +Check that the affected layers, supported targets, and FileCheck expectations +remain consistent with the changed behavior. + +### Compiler, dialect, and conversion changes + +Classify the changed boundary: target-neutral compiler protocol/Fly interface +or transform; backend-shared ROCm pipeline/conversion; or target-specific +FlyROCDL payload and `expr/rocdl` factory. Backend payloads, intrinsics, address +spaces, and chip options must not leak into neutral owners. Inspect every +in-tree implementation/consumer of a neutral contract and every target admitted +by a changed backend dispatch. + +Trace the contract through Python producer, TableGen verifier/type inference, +neutral transforms, type conversion/legality, backend payload and intrinsic, +pass registration/order, final ISA, and observable output. Derive scenario +families from real predicates and overloads: applicable static/dynamic, +scalar/vector, pointer/memref, predicate, reachable memref/full-SSA/mixed, +supported/rejected type/layout, shape boundary, and target dispatch. Cover one +reachable representative per distinct path and interacting pairs, not an +irrelevant Cartesian product; name the uncovered family and prove it reaches +the changed assumption. + +## Angle G — repo conventions and API stability + +- **Kernel conventions.** Read the **kernel-code-cleanup** skill + ([SKILL.md](../kernel-code-cleanup/SKILL.md)), **Cautions** and §10's review-only + **Find / Triage** procedure. Apply the relevant replacement tables and their + semantic constraints, including §3, to the resolved usages. +- **API stability.** Read the **api-stability** skill + ([SKILL.md](../api-stability/SKILL.md)). Apply §1 **Producer review** to public + surface changes and §2 **Consumer review** to FlyDSL usage in the review scope. + Use its classifications, severity rules, and evidence requirements. Keep + stability judgments distinct from kernel-migration preferences. +- **Formatting and lint.** For a style question or suspected style-gate failure, + use the **format-code** skill ([SKILL.md](../format-code/SKILL.md)), + **Check only**. Report which part of the review scope the check actually covers. +- **Other repo conventions.** Read [CLAUDE.md](../../../CLAUDE.md)'s **Kernel + Authoring Conventions** and **Environment Variables**. For pre-check changes, + check registration against `scripts/check_repo.py` and + `.github/workflows/pre-checks.yaml`. + +CI independently enforces a subset of the arithmetic rules on added kernel +lines with `scripts/check_typed_arithmetic_usage.py`, invoked through +`scripts/check_repo.py`. Its AST scan is optional corroboration for a CI-failure +claim; it is not a required review step. + +## Angle H — reuse, simplification, and altitude + +Use [CLAUDE.md](../../../CLAUDE.md)'s **Kernel Authoring Conventions**, especially +**Helper placement**, for reuse and module ownership. Read the +**kernel-code-cleanup** skill ([SKILL.md](../kernel-code-cleanup/SKILL.md)), +§8 **Trim comments and dead code** and §9 **Cut launch overhead**, for cleanup +criteria and performance suggestions. + +Search for an existing implementation before proposing reuse; name the helper +and its home. Assess redundant state, copy-paste, and new abstractions against +their actual call sites. For a special case added to shared infrastructure, +identify whether the underlying mechanism should handle it generally and state +the concrete maintenance or performance cost. + +### Compiler extension generality + +Search sibling overloads, interfaces, type converters, mapping tables, and +callers before accepting a local special case. Put the rule at the lowest layer +that owns the invariant; use backend data/interfaces instead of arch strings in +neutral code. Check paired overloads for drift, rewrites for a decreasing +measure and multi-use values, and unsupported states for an early verifier or +diagnostic. “Could be more general” alone is not a finding: name the missed +sibling/caller, duplicated owner, non-converging rewrite, or reachable invalid IR. + +## Angle I — test and documentation contract + +Read `tests/README.md` and `tests/pytest.ini` for tier, backend, and marker +contracts, and [CLAUDE.md](../../../CLAUDE.md)'s **Testing Notes** and **Kernel +Entry Points** for multi-GPU requirements and new-kernel test/documentation +coverage. Check new or moved tests against their actual dependencies and device +requirements. + +For changed atoms, use the **add-target-atom-op** skill +([SKILL.md](../add-target-atom-op/SKILL.md))'s applicable verification steps. +For other changed ops or lowerings, inspect the corresponding FileCheck coverage +described in `tests/README.md`. + +Do not flag general "needs more tests" — only these specific contract breaks. + +### Compiler regression coverage + +Map changed predicates, overloads, legality rules, and target dispatch to tests. +For each distinct reachable path, identify a test or prove another case exercises +the same branch. Include the trigger, unaffected boundary, sibling-target +control for shared code, and positive/negative verifier or dispatch cases. Use +`tests/mlir/Transforms` for neutral rewrites, +`tests/mlir/Conversion` for ROCm lowering, Python unit/system tests for tracing, +protocol/cache behavior, and L2 only for a hardware-observable contract. Verify +each MLIR `RUN` line reaches the changed pass and FileCheck asserts the semantic +invariant and absence of the old failure. + +--- + +## Step 3 — Verify every candidate + +The runner deduplicates only on the same normalized file, exact line and mechanism, +preserving all source observations. Different mechanisms on nearby or identical +lines remain distinct. It verifies every remaining candidate, without a shared +admission budget, in a deterministic order that puts correctness first. + +Each assigned **independent verifier** receives the owning angle text, reads the +fixed diff and relevant files, and returns one verdict, one independently assigned +severity, and evidence. A verifier judges its assigned candidate; it does not +launch other agents. + +- **CONFIRMED** — can name the inputs, state, or target that trigger it and the + resulting wrong output, crash, hang, or CI failure. Quote the line. +- **PLAUSIBLE** — the mechanism is real but the trigger is uncertain (timing, + architecture, config, shape). State what would confirm it. +- **REFUTED** — factually wrong, or already guarded. Quote the line that proves it. + +**Default to PLAUSIBLE.** Do not refute a candidate for being "speculative" or +for "depending on runtime state" when the state is realistic. On a GPU the +following are all PLAUSIBLE, not REFUTED: a race between waves, an OOB on a +boundary tile the code does not exclude, a NaN on an all-masked partition, a +divergent barrier on a path taken only by the last workgroup, a wave32 target +the kernel was not tested on, an `i32` overflow at a large shape. + +**REFUTED only when constructible from the code:** factually wrong (quote the +actual line); provably impossible from a type, constant, or invariant (show it); +already handled in this diff (cite the guard); or pure style with no observable +effect. + +### The bar for CONFIRMED + +The ladder above is built to stop you refuting real bugs. These two rules exist +to stop the opposite failure, which is worse: a detailed, line-accurate, +arithmetically confident causal chain whose last step is simply asserted. Detail +is not evidence. Both rules cap the verdict at PLAUSIBLE when unmet — PLAUSIBLE +is not a demotion, it is the honest label for an unfinished proof. + +**Run the arithmetic; do not narrate it.** If the argument depends on index +arithmetic, offsets, strides, shapes, bounds, or bitfield widths, write a short +script that enumerates the actual index ranges over every relevant loop and wave +variable, run it, and paste its output into `evidence`. Prose arithmetic caps at +PLAUSIBLE no matter how carefully it reads. Watch for unit confusion in +particular — a 16-row tile index is not a 32-row super-row index, an element +offset is not a byte offset, a dword count is not a byte count. Substituting one +for the other produces a chain that is wrong only in its final number, which is +exactly the error that survives review. + +**Walk the chain to an observable.** A defect that never reaches an output is +not a defect. For any memory, numeric, or OOB candidate, name the specific +stored element or returned value that carries the corruption, then show it is +**not** discarded downstream — check masks, `col_valid`-style guards, buffer +descriptor `num_records` bounds, and grid tails. Kernels here routinely compute +garbage for rows past `c_m` and rely on the C descriptor to drop the stores; +that is the design, not a bug. If every affected element turns out to be +discarded, the verdict is REFUTED. + +**Do not promote evidence across compiler layers.** A type round-trip proves +construction; Fly/ROCDL FileCheck proves only the checked intermediate lowering; +resource counts prove resources, not instruction identity; normalized final ISA +proves opcode/operand/modifier equivalence for that specialization; target +execution against an independent oracle proves observable semantics. Atom +layout, operand, state, signedness, predicate, or packing changes require both +the relevant lowering/ISA evidence and hardware numerics before CONFIRMED. + +### Challenge the CONFIRMED ones + +The runner gives every candidate marked CONFIRMED one more agent whose only job is +to refute it, told to assume the prior verifier narrated its arithmetic instead +of running it and to re-derive every number itself. If the challenger returns +PLAUSIBLE or REFUTED, take the lower verdict. Only CONFIRMED pays for this — +typically a handful of candidates, and a wrong CONFIRMED costs more credibility +than six hedged findings. + +Keep candidates whose verdict is CONFIRMED or PLAUSIBLE. A failed or absent +challenger leaves the candidate unresolved, not CONFIRMED. Both verifier and +challenger evidence are retained, including when they agree. + +## Step 4 — Sweep for gaps + +The runner injects this section into one more finder holding the verified list. Re-read the +diff and the enclosing functions looking **only** for defects not already +listed — do not re-derive or re-confirm anything on it. + +Focus on what a first pass misses: a bug in unchanged lines of a touched +function; code that moved between files and lost a guard or an anchor on the +way; setup/teardown asymmetry in tests; a default value flipped; a constant +changed in one place but not its mirror; an interaction between two separately +correct hunks. Up to 8 additional candidates, each verified like the rest. If +nothing new, return nothing — do not pad. +For compiler scopes, sweep for an uncovered scenario family, stale sibling +implementation, neutral/backend ownership leak, or test that stops before the +changed pass, final ISA, or observable boundary. + +## Step 5 — Report + +Synthesis is deterministic code. It retains candidate IDs, kinds, independently +adjudicated severities, source observations, verdicts and evidence; a model cannot add an unverified +finding or upgrade a verdict while rewriting the report. **Correctness findings +(A–F) always outrank convention findings (G–I) when the cap forces a cut.** +CONFIRMED outranks PLAUSIBLE within each group, then severity and stable location +break ties. Keep at most **12** across confirmed findings and plausible risks. + +The artifact's `findings` contains CONFIRMED only; `risks` contains PLAUSIBLE, +reported separately and not as merge blockers. `reported_ids` preserves their +combined rank. The full candidate list, refutations, stage attempts, failures, +usage and OIDs remain in the artifact even when the display cap excludes them. +Cost is labelled as a lower bound if any attempt lacks a usage record. + +Any failed, timed-out, denied, skipped or unresolved required stage yields +`status: INCOMPLETE`, stage failure labels and unresolved candidate IDs. Valid +partial findings are retained as `partial_findings`, never as a clean review or +a publishable result. Only a completed run may report that nothing survived. + +## Posting to GitHub (`--comment`) + +Only publish when the user requests it and the artifact identifies a GitHub PR. +Use `.claude/skills/flydsl-code-review/scripts/post_review.py` with the runner's complete `result.json`; do not +extract a findings array, rewrite the artifact, or hand-roll API calls. + +```bash +python3 .claude/skills/flydsl-code-review/scripts/post_review.py \ + --findings /tmp/flydsl-review-/result.json \ + --publish-severity P1 --dry-run +``` + +Show the dry-run payload and routing. Independently recheck the load-bearing +step of each finding selected for publication, executing arithmetic where +needed; retain the artifact's verdict and evidence. Publish the checked payload +only with the user's authorization, using the same command without `--dry-run`. +Existing explicit authorization applies; do not ask for it again unnecessarily. + +The publisher reconstructs findings from the saved verifier records and rejects +incomplete, malformed or altered results. Repository, PR, base and head come from +that artifact; optional `--repo`, `--pr` and `--expected-head` assert equality. +It checks both PR OIDs again after reading the patches and immediately before +posting. A moved or closed PR requires a new review. + +All selected inline findings and deferred text go in one +`POST /pulls/{pr}/reviews` with `event: COMMENT` and the reviewed `commit_id`. +Deferred findings retain verdict, scenario and verifier/challenger evidence. +Plausible and below-threshold records never enter the GitHub payload. The body +includes published candidate IDs, threshold, run ID, reviewed OIDs, diff hash, +counts and usage metrics. + +A pinned-diff marker ignores unrelated base-tip movement and stochastic reruns; the first successful +threshold owns that merge-base/head/diff. Lost responses are reconciled by marker, never by retrying or switching routes. +GitHub has no conditional review-write API: a push racing the final check may +make the review outdated, but cannot change its pinned commit. Stop and report +any publication error with the artifact path. A denied write is not permission +to use another posting route. diff --git a/.claude/skills/flydsl-code-review/references/preflight-evidence.md b/.claude/skills/flydsl-code-review/references/preflight-evidence.md new file mode 100644 index 000000000..70ca9b674 --- /dev/null +++ b/.claude/skills/flydsl-code-review/references/preflight-evidence.md @@ -0,0 +1,26 @@ +# Why these two checks + +The scanners 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. diff --git a/.claude/skills/flydsl-code-review/scripts/post_review.py b/.claude/skills/flydsl-code-review/scripts/post_review.py new file mode 100644 index 000000000..250083eb0 --- /dev/null +++ b/.claude/skills/flydsl-code-review/scripts/post_review.py @@ -0,0 +1,259 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +# Copyright (c) 2026 FlyDSL Project Contributors + +"""Publish a COMPLETE runner artifact in one GitHub review request. + + post_review.py --findings /tmp/flydsl-review-/result.json --publish-severity P1 --dry-run + +The artifact supplies the repository, PR, reviewed OIDs, candidate IDs, evidence +and metrics. Bare arrays and incomplete or edited findings are rejected. Repeating +the same pinned diff checks its marker before writing, including after an ambiguous +network failure. No POST is retried automatically. +""" + +from __future__ import annotations + +import argparse +import fcntl +import json +import re +import subprocess +import sys +from pathlib import Path + +from review_common import MAX_FINDINGS, SEVERITIES, canonical, digest, finding_order, validate_report + +HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") + + +def gh(*args: str, stdin: str | None = None) -> str: + proc = subprocess.run(["gh", *args], input=stdin, capture_output=True, text=True, timeout=120) + if proc.returncode: + raise RuntimeError(f"gh {' '.join(args)} failed: {proc.stderr.strip()}") + return proc.stdout + + +def json_documents(raw: str) -> list: + """Decode the concatenated JSON documents emitted by gh api --paginate.""" + decoder = json.JSONDecoder() + documents = [] + index = 0 + while index < len(raw): + while index < len(raw) and raw[index].isspace(): + index += 1 + if index == len(raw): + break + document, index = decoder.raw_decode(raw, index) + documents.append(document) + return documents + + +def pages(endpoint: str) -> list[dict]: + documents = json_documents(gh("api", "--paginate", endpoint)) + if not all(isinstance(page, list) for page in documents): + raise ValueError("paginated GitHub response must contain JSON arrays") + return [item for page in documents for item in page] + + +def commentable_lines(patch: str) -> set[int]: + """RIGHT-side added and context lines; deleted lines have no RIGHT-side number.""" + lines: set[int] = set() + cursor = 0 + for raw in patch.splitlines(): + header = HUNK.match(raw) + if header: + cursor = int(header.group(1)) + continue + if cursor == 0 or raw.startswith(("-", "\\")): + continue + if raw.startswith(("+", " ")): + lines.add(cursor) + cursor += 1 + return lines + + +def body_for(f: dict) -> str: + return ( + f"**{f['verdict']} · {f['severity']} · {f['kind']}** — {f['summary'].strip()}\n\n" + f"_Failure scenario:_ {f['failure_scenario'].strip()}\n\n" + f"
Verifier evidence\n\n{f['evidence'].strip()}\n\n
\n\n" + f"Candidate: `{f['id']}`\n" + ) + + +def finding_set_marker(report: dict) -> str: + scope = report["scope"] + # One publication owns one pinned diff. Stochastic wording or hidden + # lower-priority candidates must not create another review on the same tree. + identity = {k: scope[k] for k in ("repo", "pr", "merge_base_oid", "diff_base_oid", "head_oid", "diff_sha256")} + return "" + + +def publishable_findings(report: dict, publish_severity: str = "P1") -> list[dict]: + if publish_severity not in SEVERITIES: + raise ValueError(f"invalid publish severity: {publish_severity!r}") + cutoff = SEVERITIES.index(publish_severity) + eligible = [ + candidate + for candidate in report["candidates"] + if candidate["verdict"] == "CONFIRMED" and SEVERITIES.index(candidate["severity"]) <= cutoff + ] + return sorted(eligible, key=finding_order)[:MAX_FINDINGS] + + +def severity_range(publish_severity: str) -> str: + return "P0" if publish_severity == "P0" else f"P0-{publish_severity}" + + +def payload_for(report: dict, files: list[dict], publish_severity: str = "P1") -> dict: + scope = report["scope"] + findings = publishable_findings(report, publish_severity) + diff_lines = {f["filename"]: commentable_lines(f.get("patch") or "") for f in files} + inline, deferred = [], [] + for finding in findings: + path, line = finding["file"], finding["line"] + if line is not None and line in diff_lines.get(path, set()): + inline.append({"path": path, "line": line, "side": "RIGHT", "body": body_for(finding)}) + else: + deferred.append(finding) + eligible_count = sum( + candidate["verdict"] == "CONFIRMED" + and SEVERITIES.index(candidate["severity"]) <= SEVERITIES.index(publish_severity) + for candidate in report["candidates"] + ) + omitted_count = sum(candidate["verdict"] in ("CONFIRMED", "PLAUSIBLE") for candidate in report["candidates"]) - len( + findings + ) + level = severity_range(publish_severity) + body = [ + "### FlyDSL code review", + f"Published {len(findings)} confirmed {level} finding(s); " + f"{omitted_count} lower-priority or capped record(s) remain in the local artifact.", + finding_set_marker(report), + ] + if deferred: + body.append("#### Confirmed findings outside the diff") + for finding in deferred: + location = finding["file"] + (f":{finding['line']}" if finding["line"] is not None else "") + body.append(f"`{location}`\n\n" + body_for(finding)) + provenance = { + "run_id": report["run_id"], + "status": report["status"], + "implementation_sha256": report["implementation_sha256"], + "config": report["config"], + "paths": scope.get("paths", []), + "instructions": scope.get("instructions", ""), + "base_oid": scope["base_oid"], + "merge_base_oid": scope["merge_base_oid"], + "diff_base_oid": scope["diff_base_oid"], + "head_oid": scope["head_oid"], + "diff_sha256": scope["diff_sha256"], + "publish_severity": publish_severity, + "eligible_count": eligible_count, + "published_ids": [finding["id"] for finding in findings], + "omitted_count": omitted_count, + "stage_failures": report["stage_failures"], + "metrics": report["metrics"], + "stats": report["stats"], + } + body.append( + "
Run provenance and usage\n\n```json\n" + + json.dumps(provenance, indent=2) + + "\n```\n
" + ) + payload = {"commit_id": scope["head_oid"], "event": "COMMENT", "body": "\n\n".join(body), "comments": inline} + if any(len(text) > 65000 for text in [payload["body"], *(c["body"] for c in inline)]): + raise ValueError("review body exceeds GitHub's size limit; evidence was not truncated or posted") + return payload + + +def check_pr(scope: dict) -> None: + pr = json.loads(gh("api", f"repos/{scope['repo']}/pulls/{scope['pr']}")) + if pr["state"] != "open": + raise ValueError(f"PR is {pr['state']}; refusing to post") + if pr["head"]["sha"] != scope["head_oid"] or pr["base"]["sha"] != scope["base_oid"]: + raise ValueError("PR base/head changed since this review; start a new run") + + +def existing_review(endpoint: str, marker: str, head: str) -> dict | None: + for review in pages(endpoint): + if marker in (review.get("body") or ""): + if review.get("commit_id") != head or review.get("state") == "PENDING": + raise ValueError( + "matching marker is on an unexpected head or pending review; inspect it before retrying" + ) + return review + return None + + +def publish(report: dict, *, dry_run: bool, publish_severity: str = "P1") -> int: + validate_report(report) + scope = report["scope"] + if not isinstance(scope.get("repo"), str) or not re.fullmatch(r"[\w.-]+/[\w.-]+", scope["repo"]): + raise ValueError("artifact is not a GitHub PR review") + if type(scope.get("pr")) is not int or scope["pr"] < 1: + raise ValueError("artifact is not a GitHub PR review") + findings = publishable_findings(report, publish_severity) + if not findings: + print(f"Completed review has no confirmed {severity_range(publish_severity)} findings to post.") + return 0 + endpoint = f"repos/{scope['repo']}/pulls/{scope['pr']}" + reviews = endpoint + "/reviews" + marker = finding_set_marker(report) + check_pr(scope) + existing = existing_review(reviews, marker, scope["head_oid"]) + if existing: + print(f"Already posted: {existing.get('html_url', existing['id'])}") + return 0 + payload = payload_for(report, pages(endpoint + "/files"), publish_severity) + # Pins both routing and the live write. commit_id still pins the review if a + # push races the final GET; GitHub provides no compare-and-swap POST primitive. + check_pr(scope) + if dry_run: + print(json.dumps(payload, indent=2, ensure_ascii=False)) + return 0 + try: + response = json.loads(gh("api", "--method", "POST", reviews, "--input", "-", stdin=canonical(payload))) + except (RuntimeError, subprocess.TimeoutExpired, json.JSONDecodeError): + # A lost response does not mean the write failed. Reconcile, never repost. + existing = existing_review(reviews, marker, scope["head_oid"]) + if existing: + print(f"Posted (response recovered): {existing.get('html_url', existing['id'])}") + return 0 + raise + print(f"Posted review: {response.get('html_url', response.get('id'))}") + return 0 + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument("--findings", "--result", required=True, type=Path, dest="result") + parser.add_argument("--pr", type=int, help="optional assertion; must match the artifact") + parser.add_argument("--repo", help="optional assertion; must match the artifact") + parser.add_argument("--expected-head", help="optional assertion; the artifact's reviewed head is always required") + parser.add_argument( + "--publish-severity", + choices=SEVERITIES, + default="P1", + help="publish confirmed findings from P0 through this severity (default: P1)", + ) + parser.add_argument("--dry-run", action="store_true", help="print the exact review payload without posting") + args = parser.parse_args() + try: + report = validate_report(json.loads(args.result.read_text())) + scope = report["scope"] + for supplied, key in ((args.pr, "pr"), (args.repo, "repo"), (args.expected_head, "head_oid")): + if supplied is not None and supplied != scope[key]: + raise ValueError(f"requested {key} does not match the reviewed artifact") + # Serialize local invocations sharing an artifact; remote retries use the marker. + with args.result.with_suffix(".publish.lock").open("w") as lock: + fcntl.flock(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) + return publish(report, dry_run=args.dry_run, publish_severity=args.publish_severity) + except (OSError, ValueError, KeyError, TypeError, RuntimeError, subprocess.TimeoutExpired) as exc: + print(f"Review was not published: {exc}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.claude/skills/flydsl-code-review/scripts/review_common.py b/.claude/skills/flydsl-code-review/scripts/review_common.py new file mode 100644 index 000000000..e8117b14d --- /dev/null +++ b/.claude/skills/flydsl-code-review/scripts/review_common.py @@ -0,0 +1,395 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +# Copyright (c) 2026 FlyDSL Project Contributors + +"""The review artifact contract, shared by the runner and publisher.""" + +from __future__ import annotations + +import contextlib +import hashlib +import io +import json +import math +import posixpath +import re +import sys +import traceback + +SCHEMA_VERSION = 4 +PER_ANGLE = 6 +SWEEP_MAX = 8 +MAX_FINDINGS = 12 +ANGLES = ( + ("trace-time", "correctness", "Angle A — trace-time vs runtime semantics"), + ("addressing", "correctness", "Angle B — memory addressing and out-of-bounds"), + ("sync-lds", "correctness", "Angle C — synchronization, LDS, and value lifetime"), + ("arch-atom", "correctness", "Angle D — architecture and atom contracts"), + ("removed", "correctness", "Angle E — removed-behavior auditor"), + ("cross-layer", "correctness", "Angle F — cross-layer tracer"), + ("conventions", "convention", "Angle G — repo conventions and API stability"), + ("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") + + +def canonical(value) -> str: + return json.dumps(value, sort_keys=True, separators=(",", ":"), ensure_ascii=False, allow_nan=False) + + +def digest(value) -> str: + return hashlib.sha256(canonical(value).encode()).hexdigest() + + +def severity_rank(value: str) -> int: + try: + return SEVERITIES.index(value) + except ValueError as exc: + raise ValueError(f"invalid severity: {value!r}") from exc + + +def conservative_severity(*values: str) -> str: + """Return the least severe independently assigned impact.""" + if not values: + raise ValueError("at least one severity is required") + return max(values, key=severity_rank) + + +def normalize_path(value: str, snapshot: str | None = None) -> str: + if not isinstance(value, str) or not value or "\0" in value: + raise ValueError("a finding needs a repository-relative file path") + value = value.replace("\\", "/") + if snapshot and value.startswith(snapshot.rstrip("/") + "/"): + value = value[len(snapshot.rstrip("/")) + 1 :] + value = posixpath.normpath(value) + if value in (".", "..") or value.startswith(("/", "../")) or re.match(r"^[A-Za-z]:", value): + raise ValueError(f"file is outside the reviewed tree: {value}") + return value + + +def validate_output(output: dict, *, candidate_limit: int | None = None, snapshot: str | None = None) -> dict: + """Validate independently of the model's structured-output schema.""" + if not isinstance(output, dict) or output.get("status") != "COMPLETE": + raise ValueError(f"agent did not complete: {output!r}") + if output.get("limitations") != []: + raise ValueError(f"agent reported unresolved limitations: {output.get('limitations')!r}") + if candidate_limit is None: + if ( + output.get("verdict") not in VERDICTS + or output.get("severity") not in SEVERITIES + or not isinstance(output.get("evidence"), str) + ): + raise ValueError("invalid verifier verdict, severity or evidence") + if not output["evidence"].strip(): + raise ValueError("empty verifier evidence") + else: + candidates = output.get("candidates") + if not isinstance(candidates, list) or len(candidates) > candidate_limit: + raise ValueError("invalid or over-limit candidate list; candidates must not be silently truncated") + for c in candidates: + if not isinstance(c, dict): + raise ValueError("candidate must be an object") + for key in ("summary", "mechanism", "failure_scenario"): + if not isinstance(c.get(key), str) or not c[key].strip(): + raise ValueError(f"candidate has no {key}") + c["file"] = normalize_path(c.get("file"), snapshot) + c["mechanism"] = " ".join(c["mechanism"].casefold().split()) + line = c.get("line") + if line is not None and (type(line) is not int or line < 1): + raise ValueError("line must be a positive integer or null") + if c.get("severity") not in SEVERITIES: + raise ValueError("candidate needs a P0/P1/P2/P3 severity") + return output + + +def stage_output(stages: dict, label: str) -> dict | None: + stage = stages.get(label, {}) + return stage.get("output") if stage.get("status") == "COMPLETE" else None + + +def scanner_main(main) -> int: + """Emit a completion record only after the scanner returns normally.""" + argv = sys.argv[1:] + + def invoke(): + try: + code = main(argv) + if type(code) is not int or code not in (0, 1, 2): + raise ValueError(f"invalid scanner exit code: {code!r}") + return code + except SystemExit as exc: + # argparse uses SystemExit for help and invalid arguments. + return 0 if exc.code in (None, 0) else 2 + except Exception: + traceback.print_exc() + return 2 + + if "--json" not in argv: + return invoke() + stdout, stderr = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(stdout), contextlib.redirect_stderr(stderr): + code = invoke() + print( + canonical( + { + "status": "COMPLETE" if code in (0, 1) else "INCOMPLETE", + "exit_code": code, + "stdout": stdout.getvalue(), + "stderr": stderr.getvalue(), + } + ) + ) + return code + + +def validate_preflight(output: dict) -> dict: + if not isinstance(output, dict) or output.get("status") != "COMPLETE": + raise ValueError("scanner did not return a COMPLETE result") + if 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] = {} + sources = [(f"find:{label}", kind) for label, kind, _ in ANGLES] + [("sweep", "correctness")] + for label, kind in sources: + output = stage_output(stages, label) + if output is None: + continue + validate_output(output, candidate_limit=SWEEP_MAX if label == "sweep" else PER_ANGLE) + for index, raw in enumerate(output["candidates"]): + identity = [raw["file"], raw.get("line"), raw["mechanism"]] + cid = digest(identity) + source = {"stage": label, "index": index} + if cid in unique: + unique[cid]["sources"].append(source) + unique[cid]["severity"] = min(unique[cid]["severity"], raw["severity"], key=severity_rank) + if kind == "correctness": + unique[cid]["kind"] = kind + continue + unique[cid] = { + **{key: raw[key] for key in ("file", "mechanism", "summary", "severity", "failure_scenario")}, + "line": raw.get("line"), + "id": cid, + "kind": kind, + "sources": [source], + } + return sorted(unique.values(), key=candidate_order) + + +def candidate_order(c: dict) -> tuple: + severity = c.get("severity") or c.get("source_severity") + return (c["kind"] == "convention", severity_rank(severity), c["file"], c["line"] or 0, c["id"]) + + +def finding_order(c: dict) -> tuple: + return (c["kind"] == "convention", c["verdict"] == "PLAUSIBLE", *candidate_order(c)[1:]) + + +def judged_candidates(stages: dict) -> list[dict]: + candidates = collect_candidates(stages) + for c in candidates: + verification = stage_output(stages, "verify:" + c["id"]) + challenge = stage_output(stages, "challenge:" + c["id"]) + c.update( + source_severity=c["severity"], + severity=None, + verification=verification, + challenge=challenge, + verdict=None, + evidence=None, + ) + if verification is None: + continue + validate_output(verification) + if verification["verdict"] == "CONFIRMED": + if challenge is None: + continue # An unchallenged CONFIRMED is unresolved, never reportable. + validate_output(challenge) + c["verdict"] = challenge["verdict"] + c["severity"] = conservative_severity(verification["severity"], challenge["severity"]) + c["evidence"] = verification["evidence"] + "\n\nChallenger: " + challenge["evidence"] + else: + c["verdict"] = verification["verdict"] + c["severity"] = verification["severity"] + c["evidence"] = verification["evidence"] + return candidates + + +def rank_findings(candidates: list[dict]) -> list[dict]: + surviving = [c for c in candidates if c["verdict"] in ("CONFIRMED", "PLAUSIBLE")] + return sorted(surviving, key=finding_order)[:MAX_FINDINGS] + + +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 += [ + "challenge:" + c["id"] + for c in candidates + if c.get("verification") and c["verification"]["verdict"] == "CONFIRMED" + ] + labels.append("sweep") + labels.append("synthesize") + return labels + + +def usage_metrics(stages: dict, elapsed_seconds: float) -> dict: + attempts = [a for stage in stages.values() for a in stage.get("attempts", [])] + costs = [] + tokens: dict[str, int] = {} + for attempt in attempts: + usage = attempt.get("usage") or {} + cost = usage.get("total_cost_usd") + costs.append(cost if type(cost) in (int, float) and math.isfinite(cost) and cost >= 0 else None) + for key, value in (usage.get("tokens") or {}).items(): + if type(value) is int: + tokens[key] = tokens.get(key, 0) + value + return { + "wall_time_seconds": elapsed_seconds, + "agent_attempts": len(attempts), + "tokens": tokens, + "known_cost_usd": sum(c for c in costs if c is not None), + "cost_is_complete": all(c is not None for c in costs), + "attempts_without_cost": sum(c is None for c in costs), + } + + +def build_report(state: dict) -> dict: + stages = state["stages"] + scope = state.get("scope") + candidates = judged_candidates(stages) + required = required_stages(scope, candidates) + failed = [ + {"stage": label, "reason": stages.get(label, {}).get("error", "stage has not completed")} + 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")} + for label, stage in stages.items() + if label not in required and stage.get("status") != "COMPLETE" + ] + complete = not failed + surviving = [c for c in candidates if c["verdict"] in ("CONFIRMED", "PLAUSIBLE")] + all_confirmed = [c for c in surviving if c["verdict"] == "CONFIRMED"] + all_risks = [c for c in surviving if c["verdict"] == "PLAUSIBLE"] + ranked = rank_findings(candidates) + reported = ranked if complete else [] + confirmed = [c for c in reported if c["verdict"] == "CONFIRMED"] + risks = [c for c in reported if c["verdict"] == "PLAUSIBLE"] + if not complete: + summary = f"Review INCOMPLETE: {len(failed)} required stage(s) failed or have not completed." + elif not scope["files"]: + summary = "No changes in the pinned review scope." + elif not surviving: + summary = "Review complete. No findings survived verification." + elif len(surviving) > len(ranked): + summary = ( + f"Review complete. {len(all_confirmed)} confirmed finding(s); {len(all_risks)} plausible risk(s); " + f"{len(ranked)} selected for the capped artifact view." + ) + else: + summary = f"Review complete. {len(all_confirmed)} confirmed finding(s); {len(all_risks)} plausible risk(s)." + return { + "schema_version": SCHEMA_VERSION, + "run_id": state["run_id"], + "implementation_sha256": state["implementation_sha256"], + "config": state["config"], + "failure_history": state.get("failure_history", []), + "elapsed_seconds": state.get("elapsed_seconds", 0), + "status": "COMPLETE" if complete else "INCOMPLETE", + "summary": summary, + "scope": scope, + "stages": stages, + "stage_failures": failed, + "unresolved_candidate_ids": [c["id"] for c in candidates if c["verdict"] is None], + "candidates": candidates, + "reported_ids": [c["id"] for c in reported], + "findings": confirmed, + "risks": risks, + "partial_findings": [] if complete else ranked, + "metrics": usage_metrics(stages, state.get("elapsed_seconds", 0)), + "stats": { + "finders_completed": sum(stage_output(stages, "find:" + a[0]) is not None for a in ANGLES), + "candidates": len(candidates), + "duplicates": sum(len(c["sources"]) - 1 for c in candidates), + "verified": sum(c["verification"] is not None for c in candidates), + "challenged": sum(c["challenge"] is not None for c in candidates), + "challenge_downgraded": sum( + c["challenge"] is not None + and ( + c["challenge"]["verdict"] != "CONFIRMED" + or severity_rank(c["challenge"]["severity"]) > severity_rank(c["verification"]["severity"]) + ) + for c in candidates + ), + "confirmed": len(all_confirmed), + "plausible": len(all_risks), + "survived": len(surviving), + "selected": len(ranked), + "refuted": sum(c["verdict"] == "REFUTED" for c in candidates), + "reported": len(reported), + }, + } + + +def validate_report(report: dict) -> dict: + if not isinstance(report, dict) or report.get("schema_version") != SCHEMA_VERSION: + raise ValueError("expected a versioned runner result, not a bare findings array") + if report.get("status") != "COMPLETE": + raise ValueError("refusing to publish an INCOMPLETE review") + scope = report.get("scope") + if not isinstance(scope, dict): + raise ValueError("missing reviewed scope") + for key in ("base_oid", "merge_base_oid", "diff_base_oid", "head_oid"): + if not re.fullmatch(r"[0-9a-f]{40}|[0-9a-f]{64}", scope.get(key, "")): + raise ValueError(f"missing or invalid scope.{key}") + if not re.fullmatch(r"[0-9a-f]{64}", scope.get("diff_sha256", "")): + raise ValueError("missing diff hash") + if not isinstance(scope.get("files"), list): + raise ValueError("missing changed files") + for file in scope["files"]: + if normalize_path(file) != file: + raise ValueError("noncanonical changed-file path") + expected = build_report(report) + if expected["status"] != "COMPLETE": + raise ValueError("required stages are missing or unresolved") + if stage_output(report["stages"], "scope") != scope: + raise ValueError("scope does not match the completed scope stage") + for key in ( + "summary", + "candidates", + "reported_ids", + "findings", + "risks", + "partial_findings", + "stage_failures", + "unresolved_candidate_ids", + "metrics", + "stats", + ): + if report.get(key) != expected[key]: + raise ValueError(f"{key} does not match the verified records") + return report diff --git a/.claude/skills/flydsl-code-review/scripts/run_review.py b/.claude/skills/flydsl-code-review/scripts/run_review.py new file mode 100644 index 000000000..46ea2aa89 --- /dev/null +++ b/.claude/skills/flydsl-code-review/scripts/run_review.py @@ -0,0 +1,836 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +# Copyright (c) 2026 FlyDSL Project Contributors + +"""Run or resume a review against a pinned checkout; stdout is the result JSON. + +Examples: + run_review.py 1106 + run_review.py --base HEAD~3 --head HEAD + run_review.py --path kernels/attention --instructions 'focus on LDS changes' + run_review.py --resume /tmp/flydsl-review- + +Each agent is a separate Claude Code CLI process. No Workflow/inline fallback, +model override, publication, or permission bypass is implicit in this runner. +""" + +from __future__ import annotations + +import argparse +import concurrent.futures +import fcntl +import hashlib +import json +import os +import re +import shlex +import shutil +import signal +import subprocess +import sys +import tempfile +import threading +import time +import uuid +from pathlib import Path + +from review_common import ( + ANGLES, + PER_ANGLE, + PREFLIGHTS, + SCHEMA_VERSION, + SEVERITIES, + SWEEP_MAX, + VERDICTS, + build_report, + canonical, + collect_candidates, + digest, + judged_candidates, + normalize_path, + stage_output, + validate_output, + validate_preflight, +) + +SCRIPTS = Path(__file__).resolve().parent +SKILL = SCRIPTS.parent / "SKILL.md" + + +def atomic_json(path: Path, value: dict) -> None: + temporary = path.with_suffix(".tmp") + with temporary.open("w", encoding="utf-8") as stream: + json.dump(value, stream, indent=2, ensure_ascii=False, allow_nan=False) + stream.write("\n") + stream.flush() + os.fsync(stream.fileno()) + temporary.replace(path) + + +def command_result( + *argv: str, + cwd: Path, + stdin: str | None = None, + deadline: float | None = None, + cancelled: threading.Event | None = None, +) -> 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") + process = subprocess.Popen( + argv, + cwd=cwd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + start_new_session=True, + ) + try: + first = True + while True: + if (cancelled and cancelled.is_set()) or time.monotonic() >= deadline: + raise TimeoutError("command cancelled or phase deadline exceeded") + try: + stdout, stderr = process.communicate(input=stdin if first else None, timeout=0.2) + break + except subprocess.TimeoutExpired: + first = False + finally: + stop_process(process) + 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: + return command("git", *args, cwd=root, **options) + + +def revision(root: Path, ref: str, **options) -> str: + return git(root, "rev-parse", "--verify", "--end-of-options", ref + "^{commit}", **options).strip() + + +def default_base(root: Path, **options) -> str: + # A branch's tracking ref often points at its own head, yielding an empty review. + for ref in ("origin/main", "main", "HEAD^"): + try: + return revision(root, ref, **options) + except RuntimeError: + pass + raise ValueError("cannot resolve a review base; pass --base and --head") + + +def pin_scope(root: Path, run_dir: Path, config: dict, cancelled: threading.Event | None = None) -> dict: + control = {"deadline": time.monotonic() + config["phase_timeout"], "cancelled": cancelled} + + def scope_git(root, *args, **kwargs): + return git(root, *args, **kwargs, **control) + + target = config["target"].strip() + repo = config["repo"] + pr = config["pr"] + base_ref, head_ref = config["base"], config["head"] + paths = list(config["paths"]) + instructions = config["instructions"] + three_dot = True + url = re.fullmatch(r"https://github.com/([^/]+/[^/]+)/pull/(\d+)/?", target) + if url: + repo, pr = url.group(1), int(url.group(2)) + elif target.isdecimal(): + pr = int(target) + elif target: + if base_ref or head_ref or pr: + raise ValueError("use either a positional target or explicit revision/PR options") + if (root / target).exists(): + paths.append(str((root / target).resolve().relative_to(root))) + elif ".." in target and not any(c.isspace() for c in target): + separator = "..." if "..." in target else ".." + base_ref, head_ref = target.split(separator, 1) + base_ref, head_ref = base_ref or "HEAD", head_ref or "HEAD" + three_dot = separator == "..." + else: + try: + base_ref = revision(root, target, **control) + except RuntimeError: + instructions = "\n".join(s for s in (instructions, target) if s) + paths = sorted({normalize_path(p) for p in paths if p.rstrip("/") not in (".", "./")}) + snapshot = run_dir / "repo" + snapshot.mkdir(exist_ok=True) + scope_git(snapshot, "init", "--quiet") + include_worktree = not (pr or base_ref or head_ref) + if pr: + if base_ref or head_ref: + raise ValueError("--pr cannot be combined with --base or --head") + repo = ( + repo + or command( + "gh", "repo", "view", "--json", "nameWithOwner", "--jq", ".nameWithOwner", cwd=root, **control + ).strip() + ) + if not re.fullmatch(r"[\w.-]+/[\w.-]+", repo) or pr < 1: + raise ValueError("invalid GitHub repository or PR number") + metadata = json.loads(command("gh", "api", f"repos/{repo}/pulls/{pr}", cwd=root, **control)) + base_oid, head_oid = metadata["base"]["sha"], metadata["head"]["sha"] + # Fetch the OIDs, not a moving pull ref. All writes go to the run's own repo. + for oid in (base_oid, head_oid): + if not re.fullmatch(r"[0-9a-f]{40}|[0-9a-f]{64}", oid): + raise ValueError("GitHub returned an invalid commit OID") + try: + revision(root, oid, **control) + remote = str(root) + except RuntimeError: + remote = f"https://github.com/{repo}.git" + scope_git(snapshot, "fetch", "--quiet", "--no-tags", remote, oid) + else: + head_oid = revision(root, head_ref or "HEAD", **control) + base_oid = revision(root, base_ref, **control) if base_ref else default_base(root, **control) + scope_git(snapshot, "fetch", "--quiet", "--no-tags", str(root), base_oid, head_oid) + scope_git(snapshot, "checkout", "--quiet", "--detach", head_oid) + source_head_oid = head_oid + if include_worktree: + patch = scope_git(root, "diff", "--binary", "--no-ext-diff", "HEAD", "--") + if patch: + scope_git(snapshot, "apply", "--binary", "-", stdin=patch) + for name in scope_git(root, "ls-files", "--others", "--exclude-standard", "-z").split("\0"): + if not name: + continue + source = root / name + destination = snapshot / name + destination.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(source, destination, follow_symlinks=False) + scope_git(snapshot, "add", "-A") + tree = scope_git(snapshot, "write-tree").strip() + if tree != scope_git(snapshot, "rev-parse", head_oid + "^{tree}").strip(): + head_oid = scope_git( + snapshot, + "-c", + "user.name=FlyDSL review", + "-c", + "user.email=review@localhost", + "commit-tree", + tree, + "-p", + head_oid, + stdin="Pinned working tree for code review\n", + ).strip() + scope_git(snapshot, "checkout", "--quiet", "--detach", head_oid) + merge_base = scope_git(snapshot, "merge-base", base_oid, head_oid).strip() + diff_base = merge_base if three_dot else base_oid + diff_args = ["diff", "--binary", "--no-ext-diff", "--no-renames", diff_base, head_oid, "--", *paths] + diff = scope_git(snapshot, *diff_args) + (run_dir / "diff.patch").write_text(diff, encoding="utf-8") + files = scope_git(snapshot, "diff", "--name-only", "--no-renames", "-z", diff_base, head_oid, "--", *paths) + return { + "repo": repo, + "pr": pr, + "base_oid": base_oid, + "merge_base_oid": merge_base, + "diff_base_oid": diff_base, + "head_oid": head_oid, + "source_head_oid": source_head_oid, + "diff_sha256": hashlib.sha256(diff.encode()).hexdigest(), + "diff_command": shlex.join(["git", *diff_args]), + "paths": paths, + "files": [f for f in files.split("\0") if f], + "instructions": instructions, + } + + +def check_snapshot(run_dir: Path, scope: dict, **control) -> None: + snapshot = run_dir / "repo" + if revision(snapshot, "HEAD", **control) != scope["head_oid"]: + raise ValueError("reviewed checkout moved from the pinned head") + if git(snapshot, "status", "--porcelain", "--untracked-files=all", **control).strip(): + raise ValueError("an agent or another process modified the reviewed checkout") + diff = git( + snapshot, + "diff", + "--binary", + "--no-ext-diff", + "--no-renames", + scope["diff_base_oid"], + scope["head_oid"], + "--", + *scope["paths"], + **control, + ) + 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: + marker = "## " + title + "\n" + if marker not in text: + raise ValueError(f"missing skill section: {title}") + return text.split(marker, 1)[1].split("\n## ", 1)[0].strip() + + +def output_schema(limit: int | None) -> dict: + properties = { + "status": {"enum": ["COMPLETE", "INCOMPLETE"]}, + "limitations": {"type": "array", "items": {"type": "string"}}, + } + if limit is not None: + fields = { + "file": {"type": "string"}, + "line": {"type": ["integer", "null"], "minimum": 1}, + "summary": {"type": "string"}, + "mechanism": {"type": "string"}, + "severity": {"enum": list(SEVERITIES)}, + "failure_scenario": {"type": "string"}, + } + properties["candidates"] = { + "type": "array", + "maxItems": limit, + "items": {"type": "object", "properties": fields, "required": list(fields), "additionalProperties": False}, + } + else: + properties.update( + verdict={"enum": list(VERDICTS)}, + severity={"enum": list(SEVERITIES)}, + evidence={"type": "string"}, + ) + return {"type": "object", "properties": properties, "required": list(properties), "additionalProperties": False} + + +def stop_process(process: subprocess.Popen) -> None: + """Terminate the whole agent process group, including a running tool.""" + try: + os.killpg(process.pid, signal.SIGTERM) + except ProcessLookupError: + pass + try: + process.wait(timeout=1) + except subprocess.TimeoutExpired: + pass + try: + os.killpg(process.pid, signal.SIGKILL) + except ProcessLookupError: + pass + process.wait() + + +def cli_agent( + task: dict, config: dict, snapshot: Path, logs: Path, deadline: float, cancelled: threading.Event +) -> dict: + started = time.monotonic() + argv = [ + "claude", + "--print", + "--output-format", + "json", + "--json-schema", + canonical(task["schema"]), + "--no-session-persistence", + "--disable-slash-commands", + "--permission-mode", + "dontAsk", + "--tools", + "Read,Grep,Glob,Bash", + "--strict-mcp-config", + "--mcp-config", + '{"mcpServers":{}}', + "--allowedTools", + "Read", + "Grep", + "Glob", + "Bash(git diff *)", + "Bash(git show *)", + "Bash(git status *)", + "Bash(python3 -c *)", + "Bash(rg *)", + ] + if config["model"]: + argv += ["--model", config["model"]] + if config["effort"]: + argv += ["--effort", config["effort"]] + stdout_path, stderr_path = logs.with_suffix(".stdout.json"), logs.with_suffix(".stderr.txt") + attempt = {"status": "INCOMPLETE", "usage": {}, "stdout": str(stdout_path), "stderr": str(stderr_path)} + deadline = min(deadline, started + config["agent_timeout"]) + try: + with stdout_path.open("w") as stdout, stderr_path.open("w") as stderr: + process = subprocess.Popen( + argv, + cwd=snapshot, + stdin=subprocess.PIPE, + stdout=stdout, + stderr=stderr, + text=True, + start_new_session=True, + ) + try: + first = True + while True: + if cancelled.is_set() or time.monotonic() >= deadline: + raise TimeoutError("cancelled" if cancelled.is_set() else "agent/phase deadline exceeded") + try: + process.communicate(input=task["prompt"] if first else None, timeout=0.2) + break + except subprocess.TimeoutExpired: + first = False + finally: + # 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"] = { + "total_cost_usd": envelope.get("total_cost_usd"), + "tokens": envelope.get("usage", {}), + "model_usage": envelope.get("modelUsage", {}), + "duration_api_ms": envelope.get("duration_api_ms"), + "num_turns": envelope.get("num_turns"), + } + attempt["session_id"] = envelope.get("session_id") + attempt["permission_denials"] = envelope.get("permission_denials", []) + if process.returncode or envelope.get("is_error") or envelope.get("subtype") != "success": + raise ValueError(f"CLI failed: exit={process.returncode}, subtype={envelope.get('subtype')}") + if attempt["permission_denials"]: + raise ValueError("agent encountered permission denials; see the saved CLI result") + attempt["output"] = validate_output( + envelope.get("structured_output"), + candidate_limit=task["limit"], + snapshot=str(snapshot), + ) + attempt["status"] = "COMPLETE" + except (OSError, ValueError, TimeoutError) as exc: + attempt["error"] = str(exc) + attempt["wall_time_seconds"] = time.monotonic() - started + return attempt + + +class ReviewRun: + def __init__(self, run_dir: Path, state: dict, backend=cli_agent): + self.run_dir, self.state, self.backend = run_dir, state, backend + self.snapshot = run_dir / "repo" + self.config = state["config"] + self.cancelled = threading.Event() + self.started = time.monotonic() + self.previous_elapsed = state.get("elapsed_seconds", 0) + self.skill = SKILL.read_text() + (run_dir / "agents").mkdir(exist_ok=True) + + def save(self): + self.state["elapsed_seconds"] = self.previous_elapsed + time.monotonic() - self.started + atomic_json(self.run_dir / "state.json", self.state) + + def context(self) -> str: + scope = self.state["scope"] + return ( + "Perform a read-only FlyDSL code review in this pinned checkout. Do not edit repository files, " + "post comments, spawn agents, inspect later history, or access the repository network. " + "Run small arithmetic probes with python3 -c when needed.\n" + f"Reviewed base: {scope['base_oid']}\nReviewed head: {scope['head_oid']}\n" + f"Diff: {scope['diff_command']}\nDiff SHA256: {scope['diff_sha256']}\n" + "Read source and policy from this checkout (git show : also works), " + "never from a moving branch or the caller's workspace. Read enclosing functions for touched hunks.\n" + f"Changed files: {canonical(scope['files'])}\nUser scope/instructions: {scope['instructions']}\n\n" + + section(self.skill, "Reusing existing skills") + + "\n\n" + + section(self.skill, "Severity and publication") + + "\n\n" + "The supplied skill excerpts describe the review method. Apply repository rules only where " + "they exist and apply at the reviewed revision; do not impose later migrations on old code.\n" + "Resolve relative links in these excerpts from .claude/skills/flydsl-code-review/SKILL.md; " + "the referenced technical skills live under .claude/skills/.\n" + "Return status COMPLETE with limitations [] only if you finished the assigned task. " + "A tool failure, permission denial, missing required evidence, or unresolved stage means " + "status INCOMPLETE with explicit limitations, never a successful empty list. " + "An uncertain bug trigger is PLAUSIBLE; that alone is not an execution failure.\n\n" + ) + + def candidate_guidance(self, candidate: dict) -> str: + """Carry each source angle's rules into independent adjudication.""" + angle_titles = {f"find:{label}": title for label, _, title in ANGLES} + titles = [] + for source in candidate["sources"]: + stage = source["stage"] + title = angle_titles.get(stage) + if stage == "sweep": + title = "Step 4 — Sweep for gaps" + if title and title not in titles: + titles.append(title) + return "\n\nOwning review guidance:\n\n" + "\n\n".join(section(self.skill, title) for title in titles) + + 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"), "--json"] + 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) + output = json.loads(result.stdout) + if not isinstance(output, dict): + raise ValueError("scanner did not return a structured result object") + if result.returncode not in (0, 1): + raise ValueError(f"scanner exited {result.returncode}: {output.get('stderr') or result.stderr}") + validate_preflight(output) + if output["exit_code"] != result.returncode: + raise ValueError("scanner result exit code does not match the process exit code") + record["status"] = "COMPLETE" + stage["output"] = output + 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"] + pending = [] + for task in tasks: + fingerprint = digest({"prompt": task["prompt"], "schema": task["schema"]}) + stage = stages.setdefault(task["label"], {"attempts": []}) + if stage.get("status") == "COMPLETE": + if stage.get("input_sha256") != fingerprint: + raise ValueError(f"cached input changed for {task['label']}; start a new run") + continue + for attempt in stage["attempts"]: + if attempt.get("status") == "RUNNING": + attempt.update(status="INCOMPLETE", error="parent stopped before recording this attempt's result") + stage["input_sha256"] = fingerprint + pending.append(task) + deadline = time.monotonic() + self.config["phase_timeout"] + with concurrent.futures.ThreadPoolExecutor(max_workers=self.config["concurrency"]) as pool: + active = {} + while pending or active: + expired = self.cancelled.is_set() or time.monotonic() >= deadline + while pending and len(active) < self.config["concurrency"] and not expired: + task = pending.pop(0) + stage = stages[task["label"]] + stage.update(status="RUNNING", output=None) + stage.pop("error", None) + attempt_number = len(stage["attempts"]) + 1 + logs = self.run_dir / "agents" / f"{task['label'].replace(':', '-')}-{attempt_number}" + logs.with_suffix(".prompt.txt").write_text(task["prompt"]) + stage["attempts"].append( + { + "status": "RUNNING", + "usage": {}, + "started_at": time.time(), + "stdout": str(logs.with_suffix(".stdout.json")), + "stderr": str(logs.with_suffix(".stderr.txt")), + } + ) + self.save() # A killed parent leaves an explicit RUNNING stage to retry. + future = pool.submit(self.backend, task, self.config, self.snapshot, logs, deadline, self.cancelled) + active[future] = task + if expired: + for task in pending: + stages[task["label"]].update( + status="INCOMPLETE", error="cancelled or phase deadline before start" + ) + pending.clear() + if active: + done, _ = concurrent.futures.wait( + active, timeout=0.2, return_when=concurrent.futures.FIRST_COMPLETED + ) + for future in done: + task = active.pop(future) + stage = stages[task["label"]] + try: + attempt = future.result() + if attempt.get("status") == "COMPLETE": + validate_output( + attempt.get("output"), candidate_limit=task["limit"], snapshot=str(self.snapshot) + ) + except Exception as exc: + attempt = {"status": "INCOMPLETE", "error": str(exc), "usage": {}} + stage["attempts"][-1].update(attempt) + stage["status"] = attempt["status"] + stage["output"] = attempt.get("output") if attempt["status"] == "COMPLETE" else None + if attempt["status"] != "COMPLETE": + stage["error"] = attempt.get("error", "agent returned no result") + self.save() + return all(stage_output(stages, t["label"]) is not None for t in tasks) + + def verify(self, candidates: list[dict], phase_prefix: str = "") -> bool: + ladder = section(self.skill, "Step 3 — Verify every candidate") + tasks = [] + for c in candidates: + observations = [self.state["stages"][s["stage"]]["output"]["candidates"][s["index"]] for s in c["sources"]] + guidance = self.candidate_guidance(c) + tasks.append( + self.task( + "verify:" + c["id"], + "Independently verify this candidate and assign severity from the supplied contract.\n" + + canonical(c) + + "\nOriginal observations:\n" + + canonical(observations) + + guidance + + "\n\n" + + ladder, + ) + ) + if not self.phase(phase_prefix + "Verify", tasks): + return False + challenges = [] + for c in candidates: + verdict = stage_output(self.state["stages"], "verify:" + c["id"]) + if verdict["verdict"] == "CONFIRMED": + guidance = self.candidate_guidance(c) + challenges.append( + self.task( + "challenge:" + c["id"], + "Challenge this CONFIRMED finding. Try to refute it. Independently execute its arithmetic " + "and trace the defect to an observable output, checking downstream masks and bounds. " + "Independently assign severity and never increase the verifier's impact rating. Keep " + "CONFIRMED only if both checks succeed; otherwise return PLAUSIBLE or REFUTED with evidence.\n" + + canonical(c) + + "\nPrior verifier:\n" + + canonical(verdict) + + guidance + + "\n\n" + + ladder, + ) + ) + return self.phase(phase_prefix + "Challenge", challenges) + + def review(self) -> None: + stages = self.state["stages"] + if not self.state.get("scope"): + scope = pin_scope(Path(self.state["source_root"]), self.run_dir, self.config, self.cancelled) + self.state["scope"] = scope + stages["scope"] = {"status": "COMPLETE", "output": scope} + self.save() + check_snapshot( + self.run_dir, + self.state["scope"], + deadline=time.monotonic() + self.config["phase_timeout"], + 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 from the supplied contract, exact file/line and failure scenario. " + "Pass every candidate with a nameable failure scenario to independent verification. " + "For conventions, describe the concrete CI or maintenance cost. Do not invent crashes.", + PER_ANGLE, + ) + for label, _, title in ANGLES + ] + if not self.phase("Find", finders): + return + # 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 + known = judged_candidates({k: v for k, v in stages.items() if k != "sweep"}) + sweep = self.task( + "sweep", + section(self.skill, "Step 4 — Sweep for gaps") + + "\n\nKnown candidates:\n" + + canonical(known) + + f"\nReturn at most {SWEEP_MAX} new candidates.", + SWEEP_MAX, + ) + if not self.phase("Sweep", [sweep]): + return + known_ids = {c["id"] for c in candidates} + fresh = [c for c in collect_candidates(stages) if c["id"] not in known_ids] + if not self.verify(fresh, "Sweep "): + return + check_snapshot( + self.run_dir, + self.state["scope"], + deadline=time.monotonic() + self.config["phase_timeout"], + cancelled=self.cancelled, + ) + # Synthesis is code, not a model: it cannot invent findings or change verdicts/evidence. + stages["synthesize"] = {"status": "COMPLETE", "output": {"method": "deterministic ranking of verified IDs"}} + + def run(self) -> dict: + self.state["stages"].pop("run", None) + try: + self.review() + if self.cancelled.is_set(): + raise ValueError("review cancelled") + except Exception as exc: + self.state["stages"]["run"] = {"status": "INCOMPLETE", "error": str(exc)} + self.state.setdefault("failure_history", []).append(str(exc)) + self.save() + report = build_report(self.state) + # One terminal artifact. Checkpoints and raw attempts are never presented as a completed result. + atomic_json(self.run_dir / "result.json", report) + return report + + +def implementation_hash() -> str: + 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: + number = int(value) + if number < 1: + raise argparse.ArgumentTypeError("must be positive") + return number + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument("target", nargs="?", default="") + parser.add_argument("--pr", type=positive) + parser.add_argument("--repo") + parser.add_argument("--base") + parser.add_argument("--head") + parser.add_argument("--path", action="append", default=[]) + parser.add_argument("--instructions", default="") + parser.add_argument("--model", help="omit to use the CLI's configured model") + parser.add_argument("--effort", choices=("low", "medium", "high", "xhigh", "max")) + parser.add_argument("--concurrency", type=positive, help="concurrent agents (default: 3)") + parser.add_argument("--agent-timeout", type=positive, help="seconds per agent (default: 600)") + parser.add_argument( + "--phase-timeout", type=positive, help="seconds per phase, including queued agents (default: 1800)" + ) + parser.add_argument("--run-dir", type=Path, help="new empty directory; defaults to a temporary directory") + parser.add_argument("--resume", type=Path, help="existing run directory; retries only incomplete stages") + parser.add_argument("--comment", action="store_true", help=argparse.SUPPRESS) + parser.add_argument("--publish-severity", choices=SEVERITIES, help=argparse.SUPPRESS) + args = parser.parse_args() + if args.comment or args.publish_severity: + parser.error( + "--comment and --publish-severity are publisher options; run the review first, then use post_review.py" + ) + if args.resume: + if any( + ( + args.target, + args.pr, + args.repo, + args.base, + args.head, + args.path, + args.instructions, + args.run_dir, + args.model, + args.effort, + args.concurrency, + args.agent_timeout, + args.phase_timeout, + ) + ): + parser.error("--resume uses the saved scope, configuration and model; do not supply new ones") + run_dir = args.resume.resolve() + else: + run_dir = args.run_dir.resolve() if args.run_dir else Path(tempfile.mkdtemp(prefix="flydsl-review-")) + run_dir.mkdir(parents=True, exist_ok=True) + with (run_dir / ".lock").open("w") as lock: + try: + fcntl.flock(lock, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError: + parser.error("this run is active; cancel it before resuming") + if args.resume: + state = json.loads((run_dir / "state.json").read_text()) + if state.get("schema_version") != SCHEMA_VERSION: + parser.error("saved review schema changed; start a new run") + if state["implementation_sha256"] != implementation_hash(): + parser.error("runner or skill changed since the checkpoint; start a new run") + else: + if any(p.name != ".lock" for p in run_dir.iterdir()): + parser.error("--run-dir must be empty; use --resume for an existing run") + root = Path(command("git", "rev-parse", "--show-toplevel", cwd=Path.cwd()).strip()).resolve() + if run_dir == root or root in run_dir.parents: + parser.error("--run-dir must be outside the source checkout to avoid reviewing its own artifacts") + state = { + "schema_version": SCHEMA_VERSION, + "run_id": str(uuid.uuid4()), + "source_root": str(root), + "implementation_sha256": implementation_hash(), + "stages": {}, + "scope": None, + "config": { + "target": args.target, + "pr": args.pr, + "repo": args.repo, + "base": args.base, + "head": args.head, + "paths": args.path, + "instructions": args.instructions, + "model": args.model, + "effort": args.effort, + "concurrency": args.concurrency or 3, + "agent_timeout": args.agent_timeout or 600, + "phase_timeout": args.phase_timeout or 1800, + }, + } + print(f"Run {state['run_id']}: {run_dir}\nResult: {run_dir / 'result.json'}", file=sys.stderr, flush=True) + runner = ReviewRun(run_dir, state) + for sig in (signal.SIGINT, signal.SIGTERM): + signal.signal(sig, lambda *_: runner.cancelled.set()) + runner.save() + result = runner.run() + print(json.dumps(result, indent=2, ensure_ascii=False, allow_nan=False)) + return 0 if result["status"] == "COMPLETE" else 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.claude/skills/flydsl-code-review/scripts/scan_legacy_spelling.py b/.claude/skills/flydsl-code-review/scripts/scan_legacy_spelling.py new file mode 100644 index 000000000..052ccc121 --- /dev/null +++ b/.claude/skills/flydsl-code-review/scripts/scan_legacy_spelling.py @@ -0,0 +1,190 @@ +#!/usr/bin/env python3 +"""List legacy-spelling review candidates on added kernel consumer lines. + +Only kernels/**/*.py is checked, excluding kernels/common/buffer_ops.py. These +spelling matches require source review; in particular, ordinary make_ptr pointer +construction is valid. Comments, strings, and imports visible in each diff hunk +are ignored. A hunk starting inside a string or import whose opening is omitted +does not provide enough lexical context; check the full source in those cases. + +Exit status: 0 = no candidates, 1 = candidates, 2 = input or tool failure. +""" + +import argparse +import io +import re +import sys +import tokenize +from pathlib import Path + +RULES = [ + ( + "raw ir.* / ArithValue", + r"(? --head +""" + +import argparse +import ast +import re +import sys +from collections import Counter +from pathlib import Path + +_HUNK = re.compile(r"@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") +_FUNCTIONS = (ast.FunctionDef, ast.AsyncFunctionDef) +_REPORTING_FLAGS = { + "-q", + "-qq", + "--quiet", + "-v", + "-vv", + "--verbose", + "-s", + "-ra", + "-rA", + "--disable-warnings", + "--tb=short", + "--tb=long", + "--tb=no", + "--color=yes", + "--color=no", + "--color=auto", + "--capture=no", +} + + +def added_lines_from_diff(diff): + """Map Python head paths to (added line numbers, expected head lines).""" + files = {} + path = None + old_left = new_left = 0 + next_line = None + saw_header = False + has_head = False + for line in diff.splitlines(): + if old_left or new_left: + prefix = line[:1] + if line == "\\ No newline at end of file": + continue + if prefix not in {"+", "-", " "}: + raise ValueError("incomplete or malformed diff hunk") + if prefix != "+": + old_left -= 1 + if prefix != "-": + new_left -= 1 + if path is not None: + added, expected = files[path] + expected[next_line] = line[1:] + if prefix == "+": + added.add(next_line) + next_line += 1 + if old_left < 0 or new_left < 0: + raise ValueError("diff hunk exceeds its declared line counts") + continue + if line.startswith("diff --git "): + path = None + saw_header = True + has_head = False + elif line.startswith("+++ "): + name = line[4:].split("\t", 1)[0] + saw_header = True + has_head = True + if name == "/dev/null": + path = None + elif name.startswith("b/"): + candidate = Path(name[2:]) + if candidate.is_absolute() or ".." in candidate.parts: + raise ValueError(f"unsafe head path: {name}") + path = candidate if candidate.suffix == ".py" else None + if path is not None: + files.setdefault(path, (set(), {})) + else: + raise ValueError(f"expected unquoted b/ head path, got: {name}") + elif line.startswith("@@"): + match = _HUNK.match(line) + if not match or not has_head: + raise ValueError("malformed unified diff hunk header") + old_left = int(match[2]) if match[2] is not None else 1 + next_line = int(match[3]) + new_left = int(match[4]) if match[4] is not None else 1 + elif line.startswith("--- ") or line == "\\ No newline at end of file": + continue + elif line.startswith(("+", "-", " ")): + raise ValueError("diff content outside a hunk") + if old_left or new_left: + raise ValueError("incomplete diff hunk") + if diff.strip() and not saw_header: + raise ValueError("expected a unified diff") + return files + + +def scope_nodes(statements): + """Walk one scope, without treating uncalled nested definitions as calls.""" + for node in statements: + yield node + if isinstance(node, ast.GeneratorExp): + # Creation evaluates only the outer iterable; the body needs consumption. + yield from scope_nodes([node.generators[0].iter]) + elif not isinstance(node, (*_FUNCTIONS, ast.ClassDef, ast.Lambda)): + yield from scope_nodes(ast.iter_child_nodes(node)) + + +def bound_names(nodes, include_functions=True): + names = set() + for node in nodes: + if isinstance(node, ast.Name) and isinstance(node.ctx, (ast.Store, ast.Del)): + names.add(node.id) + elif isinstance(node, ast.ClassDef) or include_functions and isinstance(node, _FUNCTIONS): + names.add(node.name) + elif isinstance(node, (ast.Import, ast.ImportFrom)): + for alias in node.names: + names.add(alias.asname or alias.name.split(".", 1)[0]) + elif isinstance(node, ast.ExceptHandler) and node.name: + names.add(node.name) + elif isinstance(node, (ast.MatchAs, ast.MatchStar)) and node.name: + names.add(node.name) + elif isinstance(node, ast.MatchMapping) and node.rest: + names.add(node.rest) + return names + + +def pytest_aliases(nodes, inherited=None, parameters=()): + aliases = dict(inherited or {}) + imports = {} + non_imports = [node for node in nodes if not isinstance(node, (ast.Import, ast.ImportFrom))] + shadowed = bound_names(non_imports) | set(parameters) + for node in nodes: + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "pytest": + imports[alias.asname or "pytest"] = "module" + else: + shadowed.add(alias.asname or alias.name.split(".", 1)[0]) + elif isinstance(node, ast.ImportFrom): + for alias in node.names: + if node.module == "pytest" and not node.level and alias.name == "main": + imports[alias.asname or "main"] = "main" + else: + shadowed.add(alias.asname or alias.name) + # Assignments/parameters/other imports cannot be assumed to retain an alias. + for name in bound_names(nodes) | set(parameters): + aliases.pop(name, None) + aliases.update({name: kind for name, kind in imports.items() if name not in shadowed}) + return aliases + + +def is_main_guard(node): + if not isinstance(node, ast.If) or not isinstance(node.test, ast.Compare): + return False + test = node.test + if len(test.ops) != 1 or not isinstance(test.ops[0], ast.Eq): + return False + left, right = test.left, test.comparators[0] + return any( + isinstance(name, ast.Name) + and name.id == "__name__" + and isinstance(value, ast.Constant) + and value.value == "__main__" + for name, value in ((left, right), (right, left)) + ) + + +def test_definitions(body, prefix="", *, unambiguous=False): + if unambiguous: + definitions = (*_FUNCTIONS, ast.ClassDef) + counts = Counter(node.name for node in body if isinstance(node, definitions)) + rebound = bound_names(node for node in scope_nodes(body) if not isinstance(node, definitions)) + for node in body: + if unambiguous and isinstance(node, (*_FUNCTIONS, ast.ClassDef)): + if counts[node.name] != 1 or node.name in rebound: + continue + if isinstance(node, _FUNCTIONS) and node.name.startswith("test_"): + yield (prefix + node.name, node.lineno) + elif isinstance(node, ast.ClassDef) and node.name.startswith("Test"): + yield from test_definitions(node.body, prefix + node.name + ".", unambiguous=unambiguous) + + +def full_file_pytest(call): + """Accept an explicit whole-file target with only known reporting flags.""" + if len(call.args) == 1 and not call.keywords: + args = call.args[0] + elif not call.args and len(call.keywords) == 1 and call.keywords[0].arg == "args": + args = call.keywords[0].value + else: + return False + if not isinstance(args, (ast.List, ast.Tuple)): + return False + file_count = 0 + for arg in args.elts: + if isinstance(arg, ast.Name) and arg.id == "__file__": + file_count += 1 + elif not isinstance(arg, ast.Constant) or arg.value not in _REPORTING_FLAGS: + return False + return file_count == 1 + + +def analyse(tree): + tests = set(test_definitions(tree.body)) + pytest_tests = set(test_definitions(tree.body, unambiguous=True)) + main_body = [stmt for node in tree.body if is_main_guard(node) for stmt in node.body] + if not main_body: + return tests, set(), False, False + + module_nodes = list(scope_nodes(tree.body)) + definitions = [node for node in tree.body if isinstance(node, _FUNCTIONS)] + counts = Counter(node.name for node in definitions) + rebound = bound_names(module_nodes, include_functions=False) + funcs = {node.name: node for node in definitions if counts[node.name] == 1 and node.name not in rebound} + module_aliases = pytest_aliases(module_nodes) + reached, visited = set(), set() + unknown_pytest = False + pending = [(main_body, ())] + while pending: + body, parameters = pending.pop() + nodes = list(scope_nodes(body)) + shadowed = bound_names(nodes) | set(parameters) + aliases = pytest_aliases(nodes, module_aliases, parameters) + for node in nodes: + if not isinstance(node, ast.Call): + continue + target = node.func + is_pytest = ( + isinstance(target, ast.Name) + and aliases.get(target.id) == "main" + or isinstance(target, ast.Attribute) + and target.attr == "main" + and isinstance(target.value, ast.Name) + and aliases.get(target.value.id) == "module" + ) + if is_pytest: + if full_file_pytest(node) and "__file__" not in rebound | shadowed: + reached.update(pytest_tests) + else: + unknown_pytest = True + if not isinstance(target, ast.Name) or target.id in shadowed or target.id not in funcs: + continue + func = funcs[target.id] + # Calling async/generator functions does not execute their bodies. + # Await/iteration is outside the direct synchronous call graph. + if ( + isinstance(func, ast.AsyncFunctionDef) + or func.name in visited + or any(isinstance(n, (ast.Yield, ast.YieldFrom)) for n in scope_nodes(func.body)) + ): + continue + visited.add(func.name) + reached.add((func.name, func.lineno)) + args = func.args + parameters = [a.arg for a in (*args.posonlyargs, *args.args, *args.kwonlyargs)] + parameters += [a.arg for a in (args.vararg, args.kwarg) if a is not None] + pending.append((func.body, parameters)) + return tests, reached, True, unknown_pytest + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--diff", required=True, type=Path) + parser.add_argument("--head", required=True, type=Path, metavar="DIR") + parser.add_argument("--json", action="store_true", help="emit a structured completion result for the runner") + args = parser.parse_args(argv) + try: + root = args.head.resolve(strict=True) + if not root.is_dir(): + raise ValueError(f"worktree root is not a directory: {root}") + files = added_lines_from_diff(args.diff.read_text()) + results = [] + for rel, (added, expected) in files.items(): + path = (root / rel).resolve(strict=True) + if not path.is_relative_to(root): + raise ValueError(f"head path is outside worktree root: {rel}") + source = path.read_text() + lines = source.splitlines() + for line, content in expected.items(): + if line < 1 or line > len(lines) or lines[line - 1] != content: + raise ValueError(f"{rel}:{line}: diff does not match head source") + tests, reached, dual_entry, unknown = analyse(ast.parse(source, filename=str(rel))) + new_tests = {test for test in tests if test[1] in added} + if new_tests: + results.append((rel, new_tests, reached, dual_entry, unknown)) + except (OSError, UnicodeError, SyntaxError, ValueError) as error: + print(f" input error: {error}", file=sys.stderr) + return 2 + + flagged = False + for rel, new_tests, reached, dual_entry, unknown in results: + if not dual_entry: + print(f" {rel}: no supported __main__ guard; script coverage not assessed") + continue + missing = new_tests - reached + if not missing: + print(f" {rel}: {len(new_tests)} added test definition line(s) have a statically visible __main__ path") + continue + flagged = True + print(f" {rel}: {len(missing)} of {len(new_tests)} ADDED test definition line(s) need manual review") + for name, line in sorted(missing, key=lambda test: test[1]): + print(f" {name}:{line}: no statically supported entry path found") + if unknown: + print(" pytest invocation uses selectors or unknown arguments; whole-file coverage is unknown") + print(" -> Check script wiring or identify the pytest job that exercises these tests.") + if not results: + print(" no test definitions added by this diff") + return 1 if flagged else 0 + + +if __name__ == "__main__": + from review_common import scanner_main + + sys.exit(scanner_main(main)) diff --git a/.claude/skills/isa-resource-diff/SKILL.md b/.claude/skills/isa-resource-diff/SKILL.md index 12704badf..bc7f48d64 100644 --- a/.claude/skills/isa-resource-diff/SKILL.md +++ b/.claude/skills/isa-resource-diff/SKILL.md @@ -28,6 +28,10 @@ catch resource regressions that functional tests do not surface. This skill measures **resources, not time**. A clean result here does not mean performance is unchanged — it means register/LDS/spill pressure is unchanged. +A clean result also does not establish ISA equivalence: equal resource values +and instruction-category counts can hide a different opcode, operand order, +modifier, or schedule. Compare normalized final ISA or disassembled `.text` +separately when instruction identity is the contract. A regression here is a strong, cheap signal that is usually worth acting on before profiling, because spilling and occupancy cliffs dominate most kernel slowdowns. See §7 of `docs/kernel_tuning_guide.md` for what to do about one. @@ -117,7 +121,7 @@ untrustworthy items is printed above it. | `lds_static_bytes` | yes | `.group_segment_fixed_size` metadata | Statically allocated LDS per work-group | | `lds_read` / `lds_write` | no | instruction count | `ds_read`/`ds_load` and `ds_write`/`ds_store` sites | | `scratch_store` / `scratch_load` | no | instruction count | `scratch_*` sites; `n/a` where spilling goes through `buffer_*` | -| `matrix_ops` | no | instruction count | MFMA / WMMA / sparse-MFMA (`v_smfmac_*`) sites | +| `matrix_ops` | no | instruction count | MFMA / WMMA / sparse-MFMA (`v_smfmac_*`) sites; equal counts do not prove equal instructions | Three things are easy to misread: diff --git a/.claude/skills/kernel-code-cleanup/SKILL.md b/.claude/skills/kernel-code-cleanup/SKILL.md index f86918ba1..9245f6a6f 100644 --- a/.claude/skills/kernel-code-cleanup/SKILL.md +++ b/.claude/skills/kernel-code-cleanup/SKILL.md @@ -419,8 +419,12 @@ fx.gemm(mma, frag_C, frag_A, frag_B, frag_C, scale_a=sa, scale_b=sb) # atom st - **Keep** `copy_atom_call_ssa` / `mma_atom_call_ssa` (the SSA-*returning* variants are a different primitive) and any raw atom call whose operands have no tensor/ partition form to pass. Prefer `fx.copy` / `fx.gemm` for supported tensor forms. -- Diff numerics and ISA; for scheduler-sensitive hot loops compare repeated, - paired graph timings. Unchanged resources alone do not prove unchanged time. +- Compare the same target and specialization through the full default pipeline. + Require numerical equivalence and normalized final ISA for an atom-call + migration; use resource diff only as complementary evidence. Identical ISA + does not prove unchanged layout-to-lane mapping, and identical sample outputs + do not prove identical ISA. For scheduler-sensitive hot loops also compare + repeated, paired graph timings. --- @@ -475,9 +479,14 @@ _run_compiled(compiled["launch"], out.data_ptr(), a.data_ptr(), b.data_ptr(), ## 10. Procedure -1. **Find** legacy usage (under `kernels/`): +For review-only requests, use **Find** and **Triage**, then report the location, +resolved API, suggested replacement, and relevant semantic constraints. Stop +before migration or formatting. Use the caller's review scope. + +1. **Find** legacy usage, starting with these searches: ```bash rg -n 'ArithValue|_to_raw|arith\.(unwrap|index|index_cast)|fx\.Index\(' + rg -n 'maximumf|minimumf|maxnumf|minnumf|maxsi|maxui|minsi|minui|ceildivsi|ceildivui' rg -n 'buffer_ops\.(create_buffer_resource|buffer_load|buffer_store)' rg -n '_mlir\.dialects|from flydsl\.expr import' rg -n '\b(scf\.(For|If)Op|vector\.(extract|bitcast|splat)|llvm\.(load|store|mlir))' @@ -488,10 +497,19 @@ _run_compiled(compiled["launch"], out.data_ptr(), a.data_ptr(), b.data_ptr(), rg -n 's_waitcnt\(|_encode_waitcnt|_s_waitcnt|CNT_[0-9A-Z_]*=|0x[Cc]07[Ff]' rg -n 'LOG2E|log2e|def .*sigmoid|def .*tanh|def .*ceildiv|def .*ptr' ``` - Resolve import aliases and inspect nested definitions/callers; text hits are - candidates, not proof of duplication or dead code. -2. **Triage:** do mechanical swaps (operators, casts, `vector.extract/bitcast`) - first; structural ones (control flow, `buffer_ops` offsets, MMA loops) next. + Treat these searches as leads. Read the imports, enclosing functions, and + nested definitions/callers. Follow module aliases and direct imports to their + calls, and account for local rebinding. `from flydsl.expr.arith import maximumf as old_max` makes + `old_max(a, b)` a candidate even though the call uses a different name. + Text hits are not proof of duplication or dead code. + An empty search is not evidence that the review scope is clean. +2. **Triage:** classify operator/cast/`vector.extract/bitcast` replacements as + mechanical, and control flow, `buffer_ops` offsets, or MMA loops as structural. + Prioritize mechanical changes before structural ones. + Match resolved calls against the tables above and check operand types, + signedness, and explicit `fastmath` flags. Preserve §3's distinct NaN behavior + for `maximumf`/`minimumf` and `maxnumf`/`minnumf`; recommend a replacement only + when those semantics are preserved. 3. **Migrate in small commits**, one family at a time, matching local style. 4. **Verify:** ```bash diff --git a/tests/unit/test_code_review_runner.py b/tests/unit/test_code_review_runner.py new file mode 100644 index 000000000..e82bae269 --- /dev/null +++ b/tests/unit/test_code_review_runner.py @@ -0,0 +1,913 @@ +# SPDX-License-Identifier: Apache-2.0 +# Copyright (c) 2026 FlyDSL Project Contributors + +"""Review pipeline regressions; no model calls, GitHub writes or GPU dependencies.""" + +import copy +import importlib.util +import json +import os +import subprocess +import sys +import threading +import time +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.l0_backend_agnostic +SCRIPTS = Path(__file__).resolve().parents[2] / ".claude/skills/flydsl-code-review/scripts" + + +def load_script(name): + spec = importlib.util.spec_from_file_location(name, SCRIPTS / (name + ".py")) + module = importlib.util.module_from_spec(spec) + sys.modules[name] = module + spec.loader.exec_module(module) + return module + + +common = load_script("review_common") +runner = load_script("run_review") +publisher = load_script("post_review") + + +def candidate(line=1, mechanism="missing bounds guard", file="kernel.py", severity="P1"): + return { + "file": file, + "line": line, + "mechanism": mechanism, + "severity": severity, + "summary": mechanism, + "failure_scenario": "tail row reaches an invalid stored output", + } + + +def found(*candidates): + return {"status": "COMPLETE", "limitations": [], "candidates": list(candidates)} + + +def verdict(value="CONFIRMED", evidence="Executed probe: row 9 stores the incorrect value 17.", severity="P1"): + return { + "status": "COMPLETE", + "limitations": [], + "verdict": value, + "severity": severity, + "evidence": evidence, + } + + +def done(output): + return {"status": "COMPLETE", "output": output} + + +def configuration(**overrides): + return { + "target": "", + "pr": None, + "repo": None, + "base": None, + "head": None, + "paths": [], + "instructions": "", + "model": None, + "effort": None, + "concurrency": 3, + "agent_timeout": 1, + "phase_timeout": 10, + **overrides, + } + + +@pytest.fixture +def source_repo(tmp_path): + root = tmp_path / "source" + root.mkdir() + runner.git(root, "init", "--quiet", "-b", "main") + runner.git(root, "config", "user.name", "Test") + runner.git(root, "config", "user.email", "test@example.invalid") + (root / "kernel.py").write_text("x = 1\n") + runner.git(root, "add", ".") + runner.git(root, "commit", "--quiet", "-m", "base") + base = runner.revision(root, "HEAD") + runner.git(root, "checkout", "--quiet", "-b", "feature") + (root / "kernel.py").write_text("x = 2\n") + runner.git(root, "commit", "--quiet", "-am", "head") + return root, base, runner.revision(root, "HEAD") + + +def new_run(tmp_path, source_repo, backend, **options): + root, base, head = source_repo + run_dir = tmp_path / "review" + run_dir.mkdir() + state = { + "schema_version": common.SCHEMA_VERSION, + "run_id": "test-run", + "implementation_sha256": runner.implementation_hash(), + "source_root": str(root), + "scope": None, + "stages": {}, + "config": configuration(base=base, head=head, **options), + } + return runner.ReviewRun(run_dir, state, backend) + + +class Backend: + def __init__(self, failure=None, *, fail_once=False, many=False, downgrade=False, duplicate_sweep=False, delay=0): + self.failure, self.fail_once, self.many = failure, fail_once, many + self.downgrade, self.duplicate_sweep, self.delay = downgrade, duplicate_sweep, delay + self.calls, self.active, self.peak = [], 0, 0 + self.lock = threading.Lock() + + def __call__(self, task, config, snapshot, logs, deadline, cancelled): + label = task["label"] + with self.lock: + self.calls.append(label) + self.active += 1 + self.peak = max(self.peak, self.active) + fail = self.failure and label.startswith(self.failure) + if fail and self.fail_once: + self.failure = None + try: + if self.delay: + time.sleep(self.delay) + if fail or cancelled.is_set() or time.monotonic() >= deadline: + return {"status": "INCOMPLETE", "error": "injected stage failure", "usage": {}} + if label.startswith("find:"): + if self.many: + output = found(*(candidate(i + 1, label + str(i)) for i in range(6))) + else: + output = found(candidate()) if label == "find:trace-time" else found() + elif label == "sweep": + output = found(candidate()) if self.duplicate_sweep else found() + else: + output = verdict( + "PLAUSIBLE" if self.many or (self.downgrade and label.startswith("challenge:")) else "CONFIRMED" + ) + return { + "status": "COMPLETE", + "output": output, + "usage": {"total_cost_usd": 0.01, "tokens": {"output_tokens": 10}}, + } + finally: + with self.lock: + self.active -= 1 + + +@pytest.mark.parametrize("second_line", [99, 101]) +def test_nearby_or_same_line_different_mechanisms_survive(second_line): + stages = { + "find:trace-time": done(found(candidate(99, "bounds"))), + "find:addressing": done(found(candidate(second_line, "barrier"))), + } + candidates = common.collect_candidates(stages) + assert len(candidates) == 2 + assert len({c["id"] for c in candidates}) == 2 + + +def test_dedup_normalizes_location_and_mechanism_and_keeps_sources(): + stages = { + "find:conventions": done(found(candidate(99, " Missing GUARD ", "./kernel.py"))), + "find:trace-time": done(found(candidate(99, "missing guard", "sub/../kernel.py"))), + } + candidates = common.collect_candidates(stages) + assert len(candidates) == 1 + assert candidates[0]["kind"] == "correctness" + assert len(candidates[0]["sources"]) == 2 + + +def test_sweep_correctness_classification_is_not_hidden_by_earlier_convention(): + stages = {"find:conventions": done(found(candidate())), "sweep": done(found(candidate()))} + candidates = common.collect_candidates(stages) + assert len(candidates) == 1 + assert candidates[0]["kind"] == "correctness" + assert len(candidates[0]["sources"]) == 2 + + +@pytest.mark.parametrize("line", [1.5, True, 0, -1, "1"]) +def test_line_is_integer_not_coerced(line): + with pytest.raises(ValueError, match="positive integer"): + common.validate_output(found(candidate(line)), candidate_limit=6) + + +def test_verifier_schema_requires_independent_severity(): + schema = runner.output_schema(None) + assert schema["properties"]["severity"] == {"enum": list(common.SEVERITIES)} + assert "severity" in schema["required"] + with pytest.raises(ValueError, match="severity"): + common.validate_output({"status": "COMPLETE", "limitations": [], "verdict": "CONFIRMED", "evidence": "proof"}) + + +@pytest.mark.parametrize("failure", ["find:addressing", "verify:", "challenge:", "sweep"]) +def test_required_failure_never_returns_clean_review(tmp_path, source_repo, failure): + review = new_run(tmp_path, source_repo, Backend(failure)) + report = review.run() + assert report["status"] == "INCOMPLETE" + assert report["stage_failures"] + assert report["findings"] == report["risks"] == [] + assert "No findings survived" not in report["summary"] + assert json.loads((review.run_dir / "result.json").read_text())["status"] == "INCOMPLETE" + if failure == "challenge:": + assert report["candidates"][0]["verdict"] is None + assert report["unresolved_candidate_ids"] == [report["candidates"][0]["id"]] + with pytest.raises(ValueError, match="INCOMPLETE"): + publisher.publish(report, dry_run=False) + + +def test_all_54_candidates_verified_and_correctness_has_priority(tmp_path, source_repo): + backend = Backend(many=True) + report = new_run(tmp_path, source_repo, backend).run() + assert report["status"] == "COMPLETE" + assert sum(c.startswith("verify:") for c in backend.calls) == 54 + assert report["stats"]["verified"] == 54 + assert len(report["risks"]) == 12 + assert all(c["kind"] == "correctness" for c in report["risks"]) + assert backend.peak <= 3 + + +def test_challenge_downgrade_and_evidence_survive_synthesis(tmp_path, source_repo): + report = new_run(tmp_path, source_repo, Backend(downgrade=True)).run() + assert report["status"] == "COMPLETE" + assert not report["findings"] + risk = report["risks"][0] + assert risk["verification"]["verdict"] == "CONFIRMED" + assert risk["challenge"]["verdict"] == "PLAUSIBLE" + assert "Challenger:" in risk["evidence"] + assert report["stats"]["challenge_downgraded"] == 1 + common.validate_report(report) + risk["verdict"] = "CONFIRMED" + with pytest.raises(ValueError, match="verified records"): + common.validate_report(report) + + +def test_verifier_and_challenger_own_final_severity(): + stages = {"find:trace-time": done(found(candidate(severity="P0")))} + cid = common.collect_candidates(stages)[0]["id"] + stages["verify:" + cid] = done(verdict(severity="P1")) + stages["challenge:" + cid] = done(verdict(severity="P2")) + judged = common.judged_candidates(stages)[0] + assert judged["source_severity"] == "P0" + assert judged["verification"]["severity"] == "P1" + assert judged["challenge"]["severity"] == "P2" + assert judged["severity"] == "P2" + + +def test_resume_retries_only_failed_stages_and_keeps_prior_usage(tmp_path, source_repo): + backend = Backend("verify:", fail_once=True, duplicate_sweep=True) + review = new_run(tmp_path, source_repo, backend) + first = review.run() + assert first["status"] == "INCOMPLETE" + state = json.loads((review.run_dir / "state.json").read_text()) + resumed = runner.ReviewRun(review.run_dir, state, backend).run() + assert resumed["status"] == "COMPLETE" + assert all(backend.calls.count("find:" + label) == 1 for label, _, _ in common.ANGLES) + assert backend.calls.count("verify:" + resumed["candidates"][0]["id"]) == 2 + assert resumed["metrics"]["attempts_without_cost"] == 1 + assert resumed["metrics"]["cost_is_complete"] is False + calls = len(backend.calls) + state = json.loads((review.run_dir / "state.json").read_text()) + again = runner.ReviewRun(review.run_dir, state, backend).run() + assert again["status"] == "COMPLETE" # A duplicate sweep must not change cached prompt identity. + assert len(backend.calls) == calls + + +def test_phase_deadline_and_concurrency_account_for_queued_tasks(tmp_path, source_repo): + backend = Backend(delay=0.05) + review = new_run(tmp_path, source_repo, backend, concurrency=2) + review.state["scope"] = runner.pin_scope(source_repo[0], review.run_dir, review.config) + review.state["stages"]["scope"] = done(review.state["scope"]) + review.config["phase_timeout"] = 0.02 + tasks = [review.task("find:" + a[0], "test finder", 6) for a in common.ANGLES] + assert review.phase("Find", tasks) is False + report = common.build_report(review.state) + assert report["status"] == "INCOMPLETE" + assert backend.peak == 2 + assert len(backend.calls) == 2 + assert all(report["stages"]["find:" + a[0]]["status"] == "INCOMPLETE" for a in common.ANGLES) + + +def test_interrupted_attempt_keeps_its_log_and_unknown_cost_on_resume(tmp_path, source_repo): + review = new_run(tmp_path, source_repo, Backend("verify:")) + report = review.run() + label = "verify:" + report["candidates"][0]["id"] + state = json.loads((review.run_dir / "state.json").read_text()) + stage = state["stages"][label] + stage["status"] = stage["attempts"][0]["status"] = "RUNNING" + previous_log = stage["attempts"][0]["stdout"] + resumed = runner.ReviewRun(review.run_dir, state, Backend()).run() + assert resumed["status"] == "COMPLETE" + attempts = resumed["stages"][label]["attempts"] + assert len(attempts) == 2 + assert attempts[0]["status"] == "INCOMPLETE" + assert attempts[0]["stdout"] == previous_log != attempts[1]["stdout"] + assert resumed["metrics"]["attempts_without_cost"] == 1 + + +def test_pinned_scope_survives_source_push(tmp_path, source_repo): + root, base, head = source_repo + run_dir = tmp_path / "pin" + run_dir.mkdir() + scope = runner.pin_scope(root, run_dir, configuration(base=base, head=head)) + (root / "kernel.py").write_text("x = 3\n") + runner.git(root, "commit", "--quiet", "-am", "later push") + runner.check_snapshot(run_dir, scope) + assert scope["head_oid"] == head != runner.revision(root, "HEAD") + assert (run_dir / "repo/kernel.py").read_text() == "x = 2\n" + assert head in scope["diff_command"] and base in scope["diff_command"] + + +def test_working_tree_gets_own_commit_without_mutating_source(tmp_path, source_repo): + root, _, head = source_repo + (root / "kernel.py").write_text("x = 4\n") + (root / "untracked.py").write_text("y = 5\n") + before = runner.git(root, "status", "--porcelain") + run_dir = tmp_path / "dirty" + run_dir.mkdir() + scope = runner.pin_scope(root, run_dir, configuration()) + assert scope["source_head_oid"] == head != scope["head_oid"] + assert runner.git(root, "status", "--porcelain") == before + assert runner.revision(root, "HEAD") == head + assert (run_dir / "repo/untracked.py").read_text() == "y = 5\n" + runner.check_snapshot(run_dir, scope) + + +def test_modified_snapshot_is_incomplete(tmp_path, source_repo): + review = new_run(tmp_path, source_repo, Backend()) + assert review.run()["status"] == "COMPLETE" + (review.snapshot / "kernel.py").write_text("tampered\n") + state = json.loads((review.run_dir / "state.json").read_text()) + result = runner.ReviewRun(review.run_dir, state, Backend()).run() + assert result["status"] == "INCOMPLETE" + assert "modified" in result["stages"]["run"]["error"] + + +def test_scope_commands_have_a_cancellable_deadline(tmp_path): + started = time.monotonic() + with pytest.raises(TimeoutError, match="phase deadline"): + runner.command(sys.executable, "-c", "import time; time.sleep(30)", cwd=tmp_path, deadline=started + 0.1) + assert time.monotonic() - started < 3 + + +def test_cancellation_before_scope_returns_incomplete(tmp_path, source_repo): + backend = Backend() + review = new_run(tmp_path, source_repo, backend) + review.cancelled.set() + report = review.run() + assert report["status"] == "INCOMPLETE" + assert "cancelled" in report["stages"]["run"]["error"] + assert backend.calls == [] + + +def install_fake_cli(tmp_path, monkeypatch, body): + binary = tmp_path / "claude" + binary.write_text("#!" + sys.executable + "\n" + body) + binary.chmod(0o755) + monkeypatch.setenv("PATH", str(tmp_path) + os.pathsep + os.environ["PATH"]) + + +@pytest.mark.parametrize("shape", ["object", "transcript"]) +@pytest.mark.parametrize( + "mode", ["success", "no_footer", "invalid_json", "denied", "null_output", "error", "bad_subtype", "nonzero_exit"] +) +def test_cli_requires_success_footer_and_no_permission_denials(tmp_path, monkeypatch, mode, shape): + envelope = { + "type": "result", + "subtype": "success", + "is_error": False, + "structured_output": found(), + "total_cost_usd": 0.25, + "usage": {"output_tokens": 12}, + "permission_denials": [], + } + if mode == "denied": + envelope["permission_denials"] = [{"tool_name": "Bash"}] + if mode == "null_output": + envelope["structured_output"] = None + if mode == "error": + envelope["is_error"] = True + if mode == "bad_subtype": + envelope["subtype"] = "error_max_turns" + payload = {"type": "assistant", "message": "No terminal result"} if mode == "no_footer" else envelope + if shape == "transcript": + messages = [{"type": "system", "subtype": "init"}] + if mode != "no_footer": + # An earlier successful record must not hide the final result's + # failure, denial, malformed output, or usage. + messages.append( + { + **envelope, + "subtype": "success", + "is_error": False, + "permission_denials": [], + "structured_output": found(), + "total_cost_usd": 99, + } + ) + payload = [*messages, payload, {"type": "assistant", "message": "Trailing transcript message"}] + stdout = "" if mode == "invalid_json" else json.dumps(payload) + install_fake_cli( + tmp_path, monkeypatch, f"import sys\nprint({stdout!r})\nsys.exit({1 if mode == 'nonzero_exit' else 0})\n" + ) + task = {"prompt": "test", "schema": runner.output_schema(6), "limit": 6} + attempt = runner.cli_agent( + task, configuration(), tmp_path, tmp_path / "attempt", time.monotonic() + 3, threading.Event() + ) + assert attempt["status"] == ("COMPLETE" if mode == "success" else "INCOMPLETE") + if mode not in {"no_footer", "invalid_json"}: + assert attempt["usage"]["total_cost_usd"] == 0.25 + else: + assert attempt["usage"] == {} + + +def test_timeout_cancels_agent_and_its_tool_process(tmp_path, monkeypatch): + child_pid = tmp_path / "child.pid" + install_fake_cli( + tmp_path, + monkeypatch, + "import subprocess, sys, time\nfrom pathlib import Path\n" + "child = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(30)'])\n" + "stat = Path('/proc') / str(child.pid) / 'stat'\n" + f"Path({str(child_pid)!r}).write_text(str(child.pid) + ' ' + stat.read_text().split()[21])\n" + "time.sleep(30)\n", + ) + task = {"prompt": "test", "schema": runner.output_schema(6), "limit": 6} + started = time.monotonic() + attempt = runner.cli_agent( + task, configuration(agent_timeout=0.4), tmp_path, tmp_path / "timeout", started + 5, threading.Event() + ) + assert time.monotonic() - started < 3 + assert attempt["status"] == "INCOMPLETE" + assert "deadline" in attempt["error"] + assert child_pid.exists() + pid, start_time = child_pid.read_text().split() + stat = Path("/proc") / pid / "stat" + deadline = time.monotonic() + 1 + while stat.exists(): + fields = stat.read_text().split() + if fields[21] != start_time or fields[2] == "Z": + break + if time.monotonic() >= deadline: + pytest.fail("agent tool process survived process-group cancellation") + time.sleep(0.02) + + +def test_command_line_entry_persists_one_result_and_resumes(tmp_path, source_repo, monkeypatch): + root, base, head = source_repo + record = candidate() + install_fake_cli( + tmp_path, + monkeypatch, + "import json, sys\n" + "schema = json.loads(sys.argv[sys.argv.index('--json-schema') + 1])\n" + "sys.stdin.read()\n" + f"output = {found(record)!r} if 'candidates' in schema['properties'] else {verdict()!r}\n" + "print(json.dumps({'type': 'result', 'subtype': 'success', 'is_error': False, " + "'structured_output': output, 'total_cost_usd': 0.01, 'usage': {'output_tokens': 10}}))\n", + ) + run_dir = tmp_path / "cli-run" + entry = [sys.executable, str(SCRIPTS / "run_review.py")] + process = subprocess.run( + [*entry, "--base", base, "--head", head, "--run-dir", str(run_dir)], + cwd=root, + capture_output=True, + text=True, + timeout=10, + ) + assert process.returncode == 0, process.stderr + report = json.loads(process.stdout) + assert report == json.loads((run_dir / "result.json").read_text()) + assert report["status"] == "COMPLETE" + assert report["metrics"]["agent_attempts"] == 12 + assert report["stats"]["verified"] == report["stats"]["challenged"] == 1 + resumed = subprocess.run([*entry, "--resume", str(run_dir)], cwd=root, capture_output=True, text=True, timeout=10) + assert resumed.returncode == 0, resumed.stderr + second = json.loads(resumed.stdout) + assert second["run_id"] == report["run_id"] + assert second["reported_ids"] == report["reported_ids"] + assert second["metrics"]["agent_attempts"] == 12 + + +def test_runner_rejects_publisher_options_with_actionable_error(tmp_path): + process = subprocess.run( + [sys.executable, str(SCRIPTS / "run_review.py"), "--comment", "--publish-severity", "P0"], + cwd=tmp_path, + capture_output=True, + text=True, + ) + assert process.returncode == 2 + assert "publisher options" in process.stderr + assert "post_review.py" in process.stderr + + +def test_resume_rejects_old_schema_before_loading_stages(tmp_path): + run_dir = tmp_path / "old-review" + run_dir.mkdir() + (run_dir / "state.json").write_text( + json.dumps({"schema_version": common.SCHEMA_VERSION - 1, "implementation_sha256": runner.implementation_hash()}) + ) + process = subprocess.run( + [sys.executable, str(SCRIPTS / "run_review.py"), "--resume", str(run_dir)], + cwd=tmp_path, + capture_output=True, + text=True, + ) + assert process.returncode == 2 + assert "schema changed" in process.stderr + + +def test_skill_change_invalidates_implementation_hash(tmp_path, monkeypatch): + before = runner.implementation_hash() + changed_skill = tmp_path / "SKILL.md" + changed_skill.write_text(runner.SKILL.read_text() + "\nchanged review contract\n") + monkeypatch.setattr(runner, "SKILL", changed_skill) + assert runner.implementation_hash() != before + + +@pytest.fixture +def complete_report(): + scope = { + "repo": "ROCm/FlyDSL", + "pr": 1106, + "base_oid": "a" * 40, + "merge_base_oid": "a" * 40, + "diff_base_oid": "a" * 40, + "head_oid": "b" * 40, + "diff_sha256": "c" * 64, + "files": ["kernel.py"], + } + stages = {"scope": done(scope), "sweep": done(found()), "synthesize": done({})} + stages.update( + { + label: done({"status": "COMPLETE", "exit_code": 0, "stdout": "", "stderr": ""}) + for label, _ in common.PREFLIGHTS + } + ) + stages.update({"find:" + label: done(found()) for label, _, _ in common.ANGLES}) + stages["find:trace-time"] = done( + found(candidate(10), candidate(90, "second defect"), candidate(11, "uncertain race")) + ) + for c in common.collect_candidates(stages): + if c["mechanism"] == "uncertain race": + stages["verify:" + c["id"]] = done(verdict("PLAUSIBLE")) + else: + stages["verify:" + c["id"]] = done(verdict()) + stages["challenge:" + c["id"]] = done(verdict()) + state = { + "schema_version": common.SCHEMA_VERSION, + "run_id": "test-run", + "implementation_sha256": "d" * 64, + "config": {}, + "scope": scope, + "stages": stages, + } + return common.build_report(state) + + +def rebuild_report(template, finder_candidates, adjudications): + state = copy.deepcopy(template) + stages = state["stages"] + for label in list(stages): + if label.startswith(("verify:", "challenge:")): + del stages[label] + elif label.startswith("find:"): + stages[label] = done(found()) + stages["sweep"] = done(found()) + for label, candidates in finder_candidates.items(): + stages["find:" + label] = done(found(*candidates)) + for item in common.collect_candidates(stages): + verdict_value, verify_severity, challenge_verdict, challenge_severity = adjudications[item["mechanism"]] + stages["verify:" + item["id"]] = done(verdict(verdict_value, severity=verify_severity)) + if verdict_value == "CONFIRMED": + stages["challenge:" + item["id"]] = done(verdict(challenge_verdict, severity=challenge_severity)) + return common.build_report(state) + + +class GitHub: + def __init__(self, report, *, advance_at=None, lost_response=False): + self.scope = report["scope"] + self.advance_at, self.lost_response = advance_at, lost_response + self.head_reads, self.posts, self.reviews = 0, [], [] + + def __call__(self, *args, stdin=None): + endpoint = next(a for a in args if a.startswith("repos/")) + if "POST" in args: + assert endpoint.endswith("/reviews") + payload = json.loads(stdin) + self.posts.append(payload) + self.reviews.append( + {"id": 1, "body": payload["body"], "commit_id": payload["commit_id"], "state": "COMMENTED"} + ) + if self.lost_response: + raise RuntimeError("response lost after server committed the review") + return '{"id":1}' + if endpoint.endswith("/reviews"): + return json.dumps(self.reviews) + if endpoint.endswith("/files"): + return json.dumps([{"filename": "kernel.py", "patch": "@@ -10,2 +10,2 @@\n-old\n+new\n context"}]) + "[]" + self.head_reads += 1 + head = "e" * 40 if self.head_reads == self.advance_at else self.scope["head_oid"] + return json.dumps({"state": "open", "head": {"sha": head}, "base": {"sha": self.scope["base_oid"]}}) + + +def test_publish_severity_thresholds_are_inclusive(complete_report): + records = [candidate(line=i + 1, mechanism=f"severity-{level}") for i, level in enumerate(common.SEVERITIES)] + adjudications = { + f"severity-{level}".casefold(): ("CONFIRMED", level, "CONFIRMED", level) for level in common.SEVERITIES + } + report = rebuild_report(complete_report, {"trace-time": records}, adjudications) + for threshold, expected in (("P0", ["P0"]), ("P1", ["P0", "P1"]), ("P2", ["P0", "P1", "P2"])): + assert [item["severity"] for item in publisher.publishable_findings(report, threshold)] == expected + assert [item["severity"] for item in publisher.publishable_findings(report, "P3")] == list(common.SEVERITIES) + + +def test_publish_filter_precedes_artifact_cap(complete_report): + correctness = [candidate(line=i + 1, mechanism=f"risk-{i}", severity="P3") for i in range(12)] + blocker = candidate(line=100, mechanism="confirmed blocker", severity="P3") + finder_candidates = { + "trace-time": correctness[:6], + "addressing": correctness[6:], + "conventions": [blocker], + } + adjudications = { + **{item["mechanism"]: ("PLAUSIBLE", "P3", "REFUTED", "P3") for item in correctness}, + blocker["mechanism"]: ("CONFIRMED", "P1", "CONFIRMED", "P1"), + } + report = rebuild_report(complete_report, finder_candidates, adjudications) + assert report["findings"] == [] + assert len(report["risks"]) == common.MAX_FINDINGS + assert report["stats"]["confirmed"] == 1 + assert report["stats"]["plausible"] == 12 + assert report["stats"]["survived"] == 13 + assert report["stats"]["selected"] == 12 + assert "12 selected for the capped artifact view" in report["summary"] + assert [item["mechanism"] for item in publisher.publishable_findings(report)] == ["confirmed blocker"] + + +def test_no_publishable_findings_makes_no_github_calls(monkeypatch, complete_report): + item = candidate(mechanism="nonblocking", severity="P0") + report = rebuild_report( + complete_report, + {"trace-time": [item]}, + {item["mechanism"]: ("CONFIRMED", "P2", "CONFIRMED", "P2")}, + ) + api = GitHub(report) + monkeypatch.setattr(publisher, "gh", api) + assert publisher.publish(report, dry_run=False) == 0 + assert api.head_reads == 0 + assert api.posts == [] + + +def test_marker_ignores_stochastic_evidence_on_same_diff(complete_report): + changed = copy.deepcopy(complete_report) + challenge = next(label for label in changed["stages"] if label.startswith("challenge:")) + changed["stages"][challenge]["output"]["evidence"] = "Independent evidence with different wording." + changed = common.build_report(changed) + assert publisher.finding_set_marker(changed) == publisher.finding_set_marker(complete_report) + + +def test_marker_ignores_unrelated_base_tip_advance(complete_report): + advanced = copy.deepcopy(complete_report) + advanced["scope"]["base_oid"] = "d" * 40 + advanced["stages"]["scope"]["output"]["base_oid"] = "d" * 40 + assert publisher.finding_set_marker(advanced) == publisher.finding_set_marker(complete_report) + + +def test_single_review_preserves_deferred_evidence_and_omits_risks(monkeypatch, complete_report): + api = GitHub(complete_report) + monkeypatch.setattr(publisher, "gh", api) + assert publisher.publish(complete_report, dry_run=False) == 0 + assert len(api.posts) == 1 + payload = api.posts[0] + assert payload["commit_id"] == complete_report["scope"]["head_oid"] + assert len(payload["comments"]) == 1 + assert payload["comments"][0]["line"] == 10 + assert "CONFIRMED" in payload["body"] and "PLAUSIBLE" not in payload["body"] + assert "uncertain race" not in payload["body"] + assert "tail row reaches" in payload["body"] and "Executed probe" in payload["body"] + assert "known_cost_usd" in payload["body"] and complete_report["run_id"] in payload["body"] + assert publisher.publish(complete_report, dry_run=False) == 0 + assert len(api.posts) == 1 + + +@pytest.mark.parametrize("advance_at", [1, 2]) +def test_post_rejects_a_changed_head_before_or_during_routing(monkeypatch, complete_report, advance_at): + api = GitHub(complete_report, advance_at=advance_at) + monkeypatch.setattr(publisher, "gh", api) + with pytest.raises(ValueError, match="base/head changed"): + publisher.publish(complete_report, dry_run=False) + assert api.posts == [] + + +def test_lost_post_response_is_reconciled_without_reposting(monkeypatch, complete_report): + api = GitHub(complete_report, lost_response=True) + monkeypatch.setattr(publisher, "gh", api) + assert publisher.publish(complete_report, dry_run=False) == 0 + assert publisher.publish(complete_report, dry_run=False) == 0 + assert len(api.posts) == 1 + + +def test_dry_run_does_not_write(monkeypatch, complete_report, capsys): + api = GitHub(complete_report) + monkeypatch.setattr(publisher, "gh", api) + assert publisher.publish(complete_report, dry_run=True) == 0 + assert api.posts == [] + assert json.loads(capsys.readouterr().out)["commit_id"] == complete_report["scope"]["head_oid"] + + +@pytest.mark.parametrize("field", ["verdict", "evidence", "kind", "id", "line"]) +def test_publisher_rejects_changed_provenance(complete_report, field): + report = copy.deepcopy(complete_report) + report["findings"][0][field] = "tampered" + with pytest.raises(ValueError, match="verified records"): + common.validate_report(report) + + +def test_unpublished_candidate_tampering_is_rejected(complete_report): + item = candidate(mechanism="hidden p2", severity="P0") + report = rebuild_report( + complete_report, + {"trace-time": [item]}, + {item["mechanism"]: ("CONFIRMED", "P2", "CONFIRMED", "P2")}, + ) + report["candidates"][0]["evidence"] = "tampered hidden evidence" + with pytest.raises(ValueError, match="verified records"): + common.validate_report(report) + + +def test_preflight_routes_raw_leads_without_promoting_them(tmp_path, source_repo): + root, base, _ = source_repo + (root / "kernels").mkdir() + (root / "kernels/example.py").write_text("scratch = SmemAllocator()\n") + (root / "test_example.py").write_text('def test_unwired():\n pass\n\nif __name__ == "__main__":\n pass\n') + runner.git(root, "add", ".") + runner.git(root, "commit", "--quiet", "-m", "add preflight leads") + prompts = {} + + def backend(task, *_): + prompts[task["label"]] = task["prompt"] + return {**done(found()), "usage": {"total_cost_usd": 0}} + + review = new_run(tmp_path, (root, base, runner.revision(root, "HEAD")), backend) + report = review.run() + assert report["status"] == "COMPLETE" + for label, _ in common.PREFLIGHTS: + assert report["stages"][label]["output"]["exit_code"] == 1 + assert len(report["stages"][label]["runs"]) == 1 + assert "kernels/example.py:1" in prompts["find:conventions"] + assert "test_unwired:1" in prompts["find:test-doc"] + assert "test_unwired:1" not in prompts["find:conventions"] + assert "kernels/example.py:1" not in prompts["find:addressing"] + assert "Compiler target decisions" in prompts["find:arch-atom"] + assert "Compiler, dialect, and conversion changes" in prompts["find:cross-layer"] + assert "Compiler extension generality" in prompts["find:reuse"] + assert "Compiler regression coverage" in prompts["find:test-doc"] + assert "code that moved between files" in prompts["sweep"] + assert "For compiler scopes, sweep" in prompts["sweep"] + assert report["findings"] == report["risks"] == report["candidates"] == [] + assert report["metrics"]["agent_attempts"] == 10 + assert report["metrics"]["cost_is_complete"] is True + common.validate_report(report) + + +def test_actual_verifier_challenger_and_sweep_prompts_receive_owning_guidance(tmp_path, source_repo): + headings = { + "arch-atom": "Compiler target decisions", + "cross-layer": "Compiler, dialect, and conversion changes", + "reuse": "Compiler extension generality", + "test-doc": "Compiler regression coverage", + } + prompts = {} + + def backend(task, *_): + label = task["label"] + prompts[label] = task["prompt"] + if label.startswith("find:"): + angle = label.removeprefix("find:") + output = ( + found(candidate(line=len(prompts), mechanism="candidate " + angle)) if angle in headings else found() + ) + elif label == "sweep": + output = found(candidate(line=99, mechanism="sweep gap")) + else: + output = verdict() + return {**done(output), "usage": {"total_cost_usd": 0}} + + report = new_run(tmp_path, source_repo, backend).run() + assert report["status"] == "COMPLETE" + candidates = {item["mechanism"]: item for item in report["candidates"]} + for angle, heading in headings.items(): + item = candidates["candidate " + angle] + assert heading in prompts["verify:" + item["id"]] + assert heading in prompts["challenge:" + item["id"]] + sweep = candidates["sweep gap"] + assert "code that moved between files" in prompts["verify:" + sweep["id"]] + assert "For compiler scopes, sweep" in prompts["challenge:" + sweep["id"]] + + +@pytest.mark.parametrize("failure", [2, 17, "timeout"]) +def test_preflight_failure_is_incomplete_and_resume_retries_only_failed_scanner( + tmp_path, source_repo, monkeypatch, failure +): + command_result = runner.command_result + + def failed_scanner(*argv, **options): + if len(argv) > 1 and Path(argv[1]).name == "scan_legacy_spelling.py": + if failure == "timeout": + raise TimeoutError("scanner deadline exceeded") + return subprocess.CompletedProcess(argv, failure, "partial scanner output", "scanner input error") + return command_result(*argv, **options) + + monkeypatch.setattr(runner, "command_result", failed_scanner) + backend = Backend() + review = new_run(tmp_path, source_repo, backend) + report = review.run() + assert report["status"] == "INCOMPLETE" + assert report["findings"] == [] + assert backend.calls == [] + assert report["stages"]["preflight:conventions"]["status"] == "INCOMPLETE" + assert report["stages"]["preflight:test-doc"]["status"] == "COMPLETE" + if failure != "timeout": + attempt = report["stages"]["preflight:conventions"]["runs"][0] + assert attempt["exit_code"] == failure + assert attempt["stdout"] == "partial scanner output" + assert attempt["stderr"] == "scanner input error" + with pytest.raises(ValueError, match="INCOMPLETE"): + publisher.publish(report, dry_run=False) + + monkeypatch.setattr(runner, "command_result", command_result) + state = json.loads((review.run_dir / "state.json").read_text()) + resumed = runner.ReviewRun(review.run_dir, state, backend).run() + assert resumed["status"] == "COMPLETE" + assert len(resumed["stages"]["preflight:conventions"]["runs"]) == 2 + assert len(resumed["stages"]["preflight:test-doc"]["runs"]) == 1 + assert resumed["metrics"]["cost_is_complete"] is True + + +def test_saved_diff_cannot_diverge_from_agent_scope_on_resume(tmp_path, source_repo): + backend = Backend() + review = new_run(tmp_path, source_repo, backend) + assert review.run()["status"] == "COMPLETE" + (review.run_dir / "diff.patch").write_text("") + calls = len(backend.calls) + state = json.loads((review.run_dir / "state.json").read_text()) + resumed = runner.ReviewRun(review.run_dir, state, backend).run() + assert resumed["status"] == "INCOMPLETE" + assert "saved diff.patch" in resumed["stages"]["run"]["error"] + assert len(backend.calls) == calls + + +@pytest.mark.parametrize("label", [label for label, _ in common.PREFLIGHTS]) +def test_publisher_requires_both_preflight_stages(complete_report, label): + del complete_report["stages"][label] + with pytest.raises(ValueError, match="required stages"): + common.validate_report(complete_report) + + +def test_publisher_requires_current_preflight_artifact_version(complete_report): + complete_report["schema_version"] = 1 + with pytest.raises(ValueError, match="versioned runner result"): + common.validate_report(complete_report) + + +@pytest.mark.parametrize( + "output", + [ + {}, + {"exit_code": 2, "stdout": "", "stderr": "scanner input failed"}, + {"exit_code": -9, "stdout": "", "stderr": "killed"}, + {"exit_code": False, "stdout": "", "stderr": ""}, + {"exit_code": 0, "stderr": ""}, + {"exit_code": 1, "stdout": [], "stderr": ""}, + ], +) +def test_malformed_preflight_cannot_be_published(monkeypatch, complete_report, output): + complete_report["stages"]["preflight:conventions"]["output"] = output + api = GitHub(complete_report) + monkeypatch.setattr(publisher, "gh", api) + with pytest.raises(ValueError, match="required stages"): + publisher.publish(complete_report, dry_run=False) + assert api.posts == [] + assert common.build_report(complete_report)["status"] == "INCOMPLETE" + + +def test_missing_stage_is_not_complete_even_with_empty_findings(complete_report): + del complete_report["stages"]["sweep"] + with pytest.raises(ValueError, match="required stages"): + common.validate_report(complete_report) + rebuilt = common.build_report(complete_report) + assert rebuilt["status"] == "INCOMPLETE" + assert rebuilt["stats"]["selected"] == len(rebuilt["partial_findings"]) > 0 + assert rebuilt["stats"]["reported"] == 0 + + +def test_diff_hunks_exclude_deleted_and_outside_lines(): + patch = "@@ -44,3 +44,3 @@\n before\n-old\n+new\n after\n\\ No newline at end of file\n" + assert publisher.commentable_lines(patch) == {44, 45, 46} + + +def test_json_documents_supports_gh_245_paginated_output(): + assert publisher.json_documents('[{"page":1}]\n[{"page":2}]\n[]') == [ + [{"page": 1}], + [{"page": 2}], + [], + ]