[Skills] Add flydsl-code-review skill and workflow

[zhiding512]

What

Adds one project-local FlyDSL code-review skill backed by a single resumable Python runner and publisher.

• run_review.py pins the reviewed base, merge base, head, and diff hash in an isolated checkout; runs two deterministic preflights and nine independent review angles; verifies every candidate; challenges every confirmed result; and builds the final artifact deterministically.
• review_common.py keeps the complete candidate, provenance, failure, and usage history in schema v4. Verdict and severity are independently adjudicated rather than trusting the finder.
• post_review.py validates that complete artifact and submits at most one atomic GitHub review for the pinned diff.
• tests/unit/test_code_review_runner.py restores backend-agnostic coverage for runner completion, resume, provenance, prompt propagation, ranking, publication, and failure handling.

Publication policy

Discovery and verification remain exhaustive inside the artifact; GitHub output is intentionally narrower.

By default publication selects only CONFIRMED P0/P1 findings from the full verified candidate set, then applies the 12-item cap. --publish-severity P0|P1|P2|P3 changes that threshold. P2/P3 and all PLAUSIBLE records remain local, and no review is created when nothing qualifies.

The pinned-diff marker is independent of stochastic evidence wording and unrelated base-tip movement, so rerunning the same merge-base/head/diff cannot duplicate comments. Pagination works with the repository host's gh 2.45 client; publication still rechecks PR identity immediately before its single write and reconciles a lost response without retrying the POST.

Compiler review coverage

Angles D/F/H/I now require reviewers to:

• separate target-neutral Fly/compiler contracts, backend-shared conversion, and target-specific FlyROCDL payloads;
• derive representative scenario families from actual predicates, overloads, legality rules, and dispatch boundaries instead of testing only the immediate reproducer or demanding a Cartesian product;
• place fixes at the layer that owns the invariant and reject one-off target/shape branches only when a concrete sibling, caller, convergence, or invalid-IR failure is shown;
• trace Copy/MMA atoms through verifier, layout/state, reachable memref/full/mixed SSA forms, lowering, intrinsic, final ISA, and observable output;
• keep FileCheck, resource counts, normalized ISA, and device numerics within the evidence boundary each can actually establish.

The owning atom and cleanup skills carry the detailed contracts, including stateful MMA and whole-tile TDM Copy exceptions. The runner now carries each candidate's owning angle into verifier and challenger prompts and injects the actual sweep section rather than a drifting hard-coded summary.

Evidence and limits

A real read-only publisher dry-run against this PR on gh 2.45 selected two synthetic P0/P1 findings and excluded one P2 plus one plausible risk. The restored L0 suite covers 80 runner/publisher behaviors, including filter-before-cap, severity downgrade, same-diff idempotency, moved-head rejection, pagination, and actual verifier/challenger prompt construction.

This update does not claim improved model precision or recall: the frozen corpus has not been rerun on schema v4. It also deliberately leaves the existing worst-case verify-all queue versus fixed phase timeout as a separate capacity follow-up rather than reducing finder breadth in a change whose goal is publication quality.

[Copilot]

🟡 Changes recommended

The documented --comment behavior is not implemented/handled by the workflow path and can be silently ignored or mis-scoped, so the skill’s contract and workflow behavior need to be aligned.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a new project-local “flydsl-code-review” skill and its backing Claude Code workflow to run a structured, multi-angle review (find → independently verify → rank/cap), and updates repository layout docs to include workflow scripts.

Changes:

• Adds .claude/workflows/flydsl-code-review.js workflow implementing Scope → Find → Verify → Sweep → Synthesize.
• Adds .claude/skills/flydsl-code-review/SKILL.md documenting the nine review angles, verifier ladder, and invocation.
• Updates CLAUDE.md repository layout to include .claude/workflows/.

File summaries

File	Description
CLAUDE.md	Documents the new .claude/workflows/ directory in the repo layout.
.claude/workflows/flydsl-code-review.js	Implements the multi-agent review workflow with verification budget and synthesis.
.claude/skills/flydsl-code-review/SKILL.md	Defines the skill’s user-facing contract (angles, invocation, and optional --comment).

Review details

Suppressed comments (1)

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

• The --comment section promises posting inline PR comments, but the workflow implementation (.claude/workflows/flydsl-code-review.js) currently produces findings only and never posts to GitHub. This should be called out here so users don’t assume comments were created when they ran the Workflow path.

Only when `--comment` was passed. After producing the findings list, if the
review target is a GitHub PR, post each finding as an inline PR comment via
`mcp__github_inline_comment__create_inline_comment`, one call per finding.
Include a suggestion block only when it fully fixes the issue. If that tool is
not available in this session, fall back to

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jhinpan]

The finder/verifier separation is a useful direction, but the current implementation does not yet preserve the core review invariant: every reported result must come from a complete run against one immutable base/head pair and retain its verifier provenance through publication. Today an incomplete run can look clean, admission can drop higher-priority candidates, synthesis can emit unverified content, and publishing can target a different head or duplicate comments after partial failure.

Please address the inline issues before merging. Afterward, evaluate the workflow against a frozen labeled corpus and report recall, independently adjudicated precision@k, findings per PR, latency, and cost; the single historical-commit run demonstrates execution but does not establish review quality.

[jhinpan]

This converts a failed or skipped finder into a successful empty result. Verifier failures are also filtered out later, and a failed sweep is ignored; if those losses remove all candidates, the workflow returns a clean-looking No findings survived verification while the stats still claim nine finders. A quota, schema, or tool failure is therefore indistinguishable from a completed clean review.

Track success or failure for each required finder, verifier, and sweep candidate. Return status: INCOMPLETE with failed stage labels and unresolved candidate IDs whenever any required agent fails, and never emit the clean summary from an incomplete run.

[jhinpan]

For a PR target this asks every phase to rerun mutable gh pr diff <n>. A push between Scope, finder, verifier, sweep, or publication can mix revisions, and the report has no base/head identity to expose that.

Resolve the repository, merge-base OID, and head OID once; fetch those objects and make every phase run git diff <merge-base> <head>. Include both OIDs plus a diff hash in the result, and abort or restart if the PR head no longer equals the reviewed head before posting.

[jhinpan]

The five-line bucket drops candidates based only on nearby location, not mechanism, despite the skill's promise that same-line findings with different causes are retained. For example, a bounds bug at line 99 and a barrier bug at line 101 both key to file:100. The adjacent completion-order budget compounds this: at most 54 candidates compete for 40 slots, so early convention results can displace later correctness results before ranking.

Collect all finder candidates first, normalize paths, and deduplicate only candidates with the same exact location and normalized mechanism or root-cause ID. Apply deterministic priority or reserve enough slots for all correctness candidates; if every candidate cannot be verified, return INCOMPLETE rather than presenting the result as complete.

[jhinpan]

The successful path accepts arbitrary schema-valid findings from synthesis. REPORT_SCHEMA has no source candidate ID, kind, or verifier evidence, and no membership, verdict, or order validation occurs here. Synthesis can therefore introduce a new finding, upgrade PLAUSIBLE, or let convention displace correctness without any verifier seeing that output.

Give each verified candidate an immutable ID and have synthesis return only selected or grouped IDs plus its summary. Reconstruct final findings in code from verified candidates, preserve the evidence, and mechanically enforce verdict/kind ordering and the cap.

[jhinpan]

Each inline comment is an independent write. If a later call or the summary fails, earlier comments remain; rerunning duplicates them, and the dry run is not bound to the head used by live posting. Deferred rows below also retain only the summary and drop the verdict and failure scenario.

Require an expected reviewed head and recheck it immediately before posting. Submit all inline comments and the deferred body in one POST /pulls/{pr}/reviews request, include a deterministic review-run or finding-set marker and skip an already-posted set. Reuse body_for for deferred findings so verdict, scenario, and evidence are preserved, and make line an integer in both schemas.

[jhinpan]

Follow-up: end-to-end evaluation on Claude Code 2.1.263

This supplements my request-changes review with actual runs of the user-facing skill and its Workflow.

Protocol

• Tested this PR's current head, 5b0c1ce9, using Claude Code 2.1.263 on the latest channel.
• Compared /flydsl-code-review with a plain baseline review on the exact merge-result trees for #863, #794, and #567.
• Both entry points used --model fable --effort high. The skill additionally selected/inherited Opus for child agents, as reflected in the CLI usage records.
• Disabled repository-network access, later-history inspection, edits, and posting. Gold labels came from the existing fix-to-parent corpus.
• This is one stochastic run per valid case, so it is evidence about current behavior and operability, not a final model-quality estimate.

Results

• Baseline: 3/3 runs completed, 2/3 gold labels found, $8.74 total cost.
• Skill user entry: 2/3 outer runs completed, but only 1/3 returned a complete usable review artifact. It delivered 2/3 gold labels at at least $28.22; incomplete runs emitted no usage record, so this is a lower bound.
• add xcd remap #863: both paths found the stale-import blocker. Baseline returned 3 findings for $1.38 in 9.9 minutes. The skill returned 9 findings for $8.49 in 34.4 minutes: 6.14x the cost and 3.49x the wall time with no gold-recall gain. Independent adjudication classified the skill's first five as 1 TP, 3 still-unproven risks, and 1 FP.
• [Kernel] conv3d: 8-wave double-buffered FP8 implicit-GEMM kernel(gfx950) #794: baseline found the missing row < npq FP8 epilogue guard. The isolated skill run also found and independently confirmed that candidate in its partial transcript, but terminated during synthesis with no final result, exit footer, error reason, or cost record. A previous attempt likewise produced no outer result. Operationally this is INCOMPLETE, not a hit.
• Enable gfx1151 (RDNA3.5 / Strix Halo): LDS, FP8 guards, f16/bf16 WMMA GEMM #567: baseline missed the fixed-width trailing-group swizzle defect; the skill found it. A local mapping probe confirms that grid_m = 9/10/12 produces 7/14/24 out-of-range M tiles and the same number of missing tiles. This is a real marginal-recall gain. However, the skill cost $19.73 versus $3.70 and exited with only the last task-notification text; it referred to an existing ranking that was not present, so the final finding count and complete review artifact cannot be recovered.

The normal /flydsl-code-review entry invoked the available Workflow 0/3 times. It instead chose different ad-hoc paths across cases: two direct agents for #863, four before the incomplete #794 run, and nine for #567. The #863 run also had two permission denials and three task-output timeouts that were absent from its final “complete” report.

I separately forced a direct Workflow({name: "flydsl-code-review", ...}) invocation. Scope completed and all nine finders started, then the run stopped making phase, token, and tool-call progress. I terminated it after roughly 30 minutes with no result.

Required before merge

1. Make the user entry deterministically invoke one implementation; do not leave Workflow versus fallback selection to the reviewing model.
2. Persist one structured result only after all required phases complete. Any failed, timed-out, missing, or unresolved stage must return status: INCOMPLETE, never a clean or successful review.
3. Add finite per-agent and per-phase timeouts, bounded concurrency, cancellation, and a resumable run identifier.
4. Preserve reviewed base/head OIDs, candidate IDs, verifier evidence, stage failures, and usage metrics through synthesis and publication.
5. After those fixes, rerun the frozen corpus and report completion rate, recall@5, independently adjudicated precision@5, findings per PR, wall time, and cost. Keep plausible risks separate from merge-blocking confirmed findings.

The multi-angle approach has demonstrated some value on #567, but the current implementation is not yet a dependable review operation.

[jhinpan]

Re-review at 3c040983.

What this commit adds, and it is a real improvement. The new Challenge phase and the "bar for CONFIRMED" section attack the exact failure mode I hit on #1107: a line-accurate, arithmetically confident chain whose last step is asserted rather than computed. challengeConfirmed at .claude/workflows/flydsl-code-review.js:221-250 spends a second agent only on CONFIRMED, tells it to re-run the arithmetic and walk the chain to an observable, and takes the lower verdict on disagreement. SKILL.md:313-346 mirrors it for the inline path, and the pre-post re-derivation requirement at SKILL.md:407-419 is the right place to put the one check that does not depend on an agent doubting itself. The stale "the live POST has never been exercised" paragraph is gone.

What I verified at this head.

• Diffed every file in the PR against 5b0c1ce9 (fetched via the contents API). The only changes are the CONFIRMED bar in VERDICT_LADDER, the Challenge phase entry, challengeConfirmed, the verifyCandidate hook, and the two SKILL.md sections. .claude/skills/flydsl-code-review/scripts/post_review.py is byte-identical to 5b0c1ce9.
• python3 scripts/check_repo.py → check_docs_api: OK (18 files, 894 symbols, 17 skills), so the new skill's frontmatter and cross-references resolve. The typed-arithmetic check errored only because this checkout is a single-commit clone with no origin/main — an environment gap, not a defect in this PR.
• Syntax: node --check on the workflow (wrapped in a function, since top-level return is by design for a workflow script) → OK. ast.parse on post_review.py → OK. black/ruff are not installed here, so I could not run the Python style gate — reporting that as a gap.

Where this leaves the review. None of the five inline items from my 5b0c1ce9 review are addressed, and there are no author replies on the review-comment threads or on the follow-up evaluation comment. They are all still true at this head, on the same lines. The commit strengthens the content of a verdict; the surviving items are about the integrity of the pipeline that carries it — completeness, base/head identity, admission, and publication. A challenger that lowers a bad CONFIRMED does not help if synthesis can raise it back (REPORT_SCHEMA still accepts a free verdict enum with no membership check), or if the run that produced it silently dropped three finders.

One new item, folded into the completeness comment because it is the same defect class and because it lands squarely on this commit's own goal: at flydsl-code-review.js:246 a challenger that fails, times out, or returns nothing falls through to return c — keeping the unchallenged CONFIRMED. The safeguard fails open in exactly the direction it was built to close.

Still open, each inline below:

1. Stage failure is indistinguishable from a clean review (:284, :246, :314, :344).
2. Every phase re-runs mutable gh pr diff <n>; no base/head OID in the result (:144).
3. Five-line dedup bucket plus completion-order budget can drop correctness candidates (:208).
4. Synthesis output is accepted unvalidated (:375).
5. post_review.py posts each comment as an independent write, with no head recheck and no idempotency marker (:164) — unchanged file.
6. From my follow-up comment: SKILL.md:57 still leaves Workflow-vs-inline to the reviewing model. In three measured runs the user entry chose the Workflow 0/3 times and took a different ad-hoc path each time. Non-blocking on its own, but it means the hardening in this commit is only reliably in force on the path the model happens not to pick.

What would close this: items 1–5 fixed, then a rerun of the frozen corpus reporting completion rate, recall@5, independently adjudicated precision@5, findings per PR, wall time, and cost — with the challenger's downgrade rate called out separately, since that is the number that shows whether this commit works.

[jhinpan]

Still open from my previous review, and this commit adds a third instance of the same pattern.

• :284 — a failed or skipped finder becomes a successful empty result.
• :246 — new here: if (!v || v.verdict === 'CONFIRMED') return c. A challenger that fails, times out, or returns nothing keeps the CONFIRMED verdict unchallenged. The one safeguard this commit adds fails open in precisely the direction it exists to close; !v must not be collapsed with an affirming verdict.
• :255 — a failed verifier returns null and is filtered out at :292.
• :314 — a failed sweep is skipped without a trace.

If those losses remove every candidate, :344 returns No findings survived verification. while stats.finders still reports 9. A quota, schema, or tool failure is not distinguishable from a completed clean review.

Track success/failure per required finder, verifier, challenger and sweep. Return status: INCOMPLETE with the failed stage labels and unresolved candidate IDs whenever any required agent fails, and never emit the clean summary from an incomplete run.

[jhinpan]

Unchanged since my last review. For a PR target this still asks every phase — Scope, nine finders, every verifier, every challenger, the sweep, and publication — to rerun mutable gh pr diff <n>. A push landing mid-run mixes revisions, and nothing in the returned object exposes it: there is no OID anywhere in the result at :381-387.

Resolve the repository, merge-base OID and head OID once; fetch those objects and have every phase run git diff <merge-base> <head>. Put both OIDs and a diff hash in the result, and abort or restart if the PR head no longer equals the reviewed head before posting.

[jhinpan]

Unchanged. Math.round(c.line / 5) * 5 still buckets by proximity alone, not mechanism, contradicting the skill's promise that same-line findings with different causes are both kept: a bounds bug at line 99 and a barrier bug at line 101 both key to file:100, and the second is dropped into dupes.

The budget compounds it — admit consumes slots in finder-completion order, so up to 54 candidates compete for FINDER_VERIFY_BUDGET = 40 and an early convention angle can displace a later correctness candidate before any ranking happens. budgetDropped is logged at :338, but the run still returns as if it were complete.

Collect all finder candidates first, normalize paths, and deduplicate only on identical exact location and normalized mechanism/root-cause. Apply a deterministic priority, or reserve slots for all correctness candidates; if not every candidate can be verified, return INCOMPLETE rather than presenting the result as complete.

[jhinpan]

Unchanged, and this commit makes it matter more. The success path still accepts arbitrary schema-valid findings from synthesis: REPORT_SCHEMA (:81-96) carries no source candidate ID, no kind, and no verifier evidence, and nothing here checks membership, verdict, or ordering.

Concretely, synthesis can now undo the new Challenge phase. challengeConfirmed downgrades a bad CONFIRMED to PLAUSIBLE at :248, and then the synthesizer — which sees the verdict only as text in block — can emit that same finding back as CONFIRMED, because the enum permits it and no code compares against the verified record. It can equally introduce a finding no verifier ever saw, or let a convention finding displace a correctness one.

Give each verified candidate an immutable ID; have synthesis return only selected/grouped IDs plus its summary. Reconstruct the final findings in code from the verified records, preserve the evidence, and mechanically enforce verdict/kind ordering and the cap.

[jhinpan]

This file is byte-identical to 5b0c1ce9; I diffed it against the contents API. Every point from my previous review stands:

• Each inline comment is an independent POST .../pulls/{pr}/comments. If a later call or the summary comment fails, the earlier ones remain and a rerun duplicates them.
• head_sha is read once at :124 and never rechecked before posting, and the --dry-run path is not bound to the head that a subsequent live run will use — so the routing the user approves may not be the routing that gets posted.
• The deferred rows at :180-185 keep only the summary, dropping the verdict and the failure scenario that body_for already formats.

Require an expected reviewed head and recheck it immediately before posting. Submit the inline comments and the deferred body in a single POST /pulls/{pr}/reviews request, include a deterministic review-run or finding-set marker and skip an already-posted set, and reuse body_for for the deferred rows.

[jhinpan]

Re-review at 15740985.

What this commit does, and it is correct. It replaces the inlined legacy-construct and arithmetic list in Angle G with a delegation to the kernel-code-cleanup skill, and adds a review-only path plus lead-not-proof guidance to that skill's §10. I checked each claim it makes:

• .claude/skills/kernel-code-cleanup/SKILL.md exists at the cited path, and §3 does carry the NaN distinction the new text promises (arith.maximumf/minimumf → fx.max/fx.min; arith.maxnumf/minnumf → fx.maxnumf/fx.minnumf — "different NaN semantics", lines 135-138).
• Nothing was lost by dropping the inline list: copy_atom_call/mma_atom_call incl. the single-atom form and the *_ssa carve-out (§7b, lines 358-385), buffer_ops (§2), fx.Index (§1, line 48 keeps the platform-width rationale), raw rocdl.mfma_* (§6), SmemAllocator/SmemPtr (§4), and fx.Int32(fx.Int32(x)) (quick reference, line 502) are all covered by the delegated skill.
• The new §10 grep line is a valid grep -nE pattern; I ran it against a synthetic maximumf(a, b) call and it matches. The "empty search is not evidence" and alias-rebinding notes are the right correction for a lead-based procedure.
• The new CI paragraph is accurate: scripts/check_typed_arithmetic_usage.py exists, is registered in scripts/check_repo.py's CHECKS (line 52), is AST-based (import ast, _dotted_name, _shadowing_bindings), and scopes its diff to -- kernels on added lines (line 194). Demoting it from a required review step to optional corroboration is the honest framing.
• python3 scripts/check_repo.py → check_docs_api: OK (18 files, 894 symbols, 17 skills), so the new cross-reference to the kernel-cleanup skill resolves. typed-arithmetic errored only because this checkout is a single-commit clone with no origin/main — an environment gap, not a defect in this PR.
• Syntax: node --check on the workflow (wrapped, since top-level return and export const meta are by design) → OK; ast.parse on post_review.py → OK. black/ruff are not installed here, so I could not run the Python style gate — reporting that as a gap, unchanged from my last review.

Why this is still CHANGES_REQUESTED. This commit touches only SKILL.md's Angle G and kernel-code-cleanup/SKILL.md. I fetched 3c040983's blobs via the contents API and diffed them against this head: .claude/workflows/flydsl-code-review.js and .claude/skills/flydsl-code-review/scripts/post_review.py are byte-identical to 3c040983. Every one of the five inline items from my last review is therefore still open, verbatim, on the same lines — I re-read each anchor at this head to confirm (:144, :208, :246, :284, :314, :344, :375, and post_review.py:124/:164/:180).

Still open:

1. Stage failure is indistinguishable from a clean review. :284 turns a failed finder into an empty success; :246 keeps an unchallenged CONFIRMED when the challenger returns nothing; :255 nulls a failed verifier and :292 filters it out; :314 skips a failed sweep silently. If those losses empty the candidate list, :344 returns No findings survived verification. while stats.finders still reports 9.
2. Every phase re-runs mutable gh pr diff <n> (:144), and the returned object at :381-387 carries no base/head OID, so a push landing mid-run mixes revisions invisibly.
3. Math.round(c.line / 5) * 5 buckets by proximity, not mechanism (:208), contradicting the skill's own promise that two angles flagging the same line for different reasons are both kept; the completion-order budget compounds it and budgetDropped is logged but the run still returns as complete.
4. Synthesis output is accepted unvalidated (:375). REPORT_SCHEMA has no candidate ID, no kind, no verifier evidence, and nothing checks membership, verdict or ordering — so the synthesizer can hand back as CONFIRMED a verdict the new Challenge phase just downgraded.
5. post_review.py posts each inline comment as an independent write (:164), never rechecks the head it read at :124, has no idempotency marker, and the deferred rows at :180-185 drop the verdict and failure scenario that body_for already formats.

Non-blocking, carried over: SKILL.md:57 still leaves Workflow-vs-inline selection to the reviewing model (0/3 in my measured runs). Angle G no longer states that fx.Index widening is a correctness bug rather than a style fix — the delegated skill keeps the rationale at §1:48, so this is a note, not a blocker.

What would close this: items 1-5 fixed, then a rerun of the frozen corpus reporting completion rate, recall@5, independently adjudicated precision@5, findings per PR, wall time, and cost, with the challenger's downgrade rate called out separately.

[jhinpan]

Still open at 15740985 — this file is byte-identical to 3c040983, which I verified by diffing the blob from the contents API. The full argument is on the existing thread; restating the anchors so the item is not lost:

• :284 failed finder → successful empty result
• :246 if (!v || v.verdict === 'CONFIRMED') return c — a challenger that fails or times out leaves the CONFIRMED verdict unchallenged
• :255/:292 failed verifier → null → filtered out
• :314 failed sweep → skipped with no trace
• :344 emits No findings survived verification. while stats.finders still reports 9

Track success/failure per required finder, verifier, challenger and sweep; return status: INCOMPLETE with the failed stage labels and unresolved candidate IDs, and never emit the clean summary from an incomplete run.

[jhinpan]

Still open at 15740985, unchanged since 5b0c1ce9. Every phase — Scope, nine finders, every verifier, every challenger, the sweep, and publication — reruns mutable gh pr diff <n>, and the returned object at :381-387 contains no OID, so a mid-run push is not detectable from the result.

Resolve the repo, merge-base OID and head OID once; fetch those objects and have every phase run git diff <merge-base> <head>. Put both OIDs and a diff hash in the result, and abort or restart if the PR head no longer equals the reviewed head before posting.

[jhinpan]

Still open at 15740985, unchanged. Math.round(c.line / 5) * 5 buckets on proximity alone: a bounds bug at line 99 and a barrier bug at line 101 both key to file:100, and the second lands in dupes — directly contradicting SKILL.md:66-69 ("if two angles flag the same line for different reasons, record both").

admit also consumes slots in finder-completion order, so up to 54 candidates compete for FINDER_VERIFY_BUDGET = 40 and an early convention angle can displace a later correctness candidate before any ranking. budgetDropped is logged at :338 but the run still returns as complete.

Deduplicate only on identical exact location and normalized mechanism/root-cause; apply a deterministic priority or reserve slots for all correctness candidates; return INCOMPLETE when not every candidate can be verified.

[jhinpan]

Still open at 15740985, unchanged. The success path accepts arbitrary schema-valid findings from synthesis: REPORT_SCHEMA (:81-96) carries no source candidate ID, no kind, and no verifier evidence, and nothing here checks membership, verdict or ordering.

This directly undercuts the Challenge phase this PR added: challengeConfirmed downgrades a bad CONFIRMED to PLAUSIBLE at :248, and the synthesizer — which sees the verdict only as text inside block — can emit that same finding back as CONFIRMED, because the enum permits it and no code compares against the verified record.

Give each verified candidate an immutable ID; have synthesis return only selected/grouped IDs plus its summary; reconstruct the final findings in code from the verified records and mechanically enforce verdict/kind ordering and the cap.

[jhinpan]

Still open at 15740985. This file is byte-identical to both 5b0c1ce9 and 3c040983; I diffed it against the contents API blob at each.

• Each inline comment is an independent POST .../pulls/{pr}/comments. If a later call or the summary comment fails, the earlier ones remain and a rerun duplicates them.
• head_sha is read once at :124 and never rechecked before posting, and the --dry-run path is not bound to the head a subsequent live run will use.
• The deferred rows at :180-185 keep only the summary, dropping the verdict and failure scenario that body_for already formats.

Require an expected reviewed head and recheck it immediately before posting; submit the inline comments and the deferred body in a single POST /pulls/{pr}/reviews; include a deterministic run/finding-set marker and skip an already-posted set; reuse body_for for the deferred rows.

[jhinpan]

Re-review at 1e18661d.

What this commit does, and it is correct. 1e18661 replaces the inlined technical rules in Angles A–D and F–I with delegations to the repository's existing skills, and threads a SHARED_RULES_PROMPT into the finder, verifier, challenger, and sweep prompts. I checked every claim it makes rather than taking the description at its word:

• Every cross-reference resolves. All ten relative links in SKILL.md (../oob-detection/, ../debug-flydsl-kernel/, ../lds-optimization/, ../flydsl-tile-programming/, ../flydsl-kernel-authoring/, ../add-target-atom-op/, ../kernel-code-cleanup/, ../api-stability/, ../format-code/, ../../../CLAUDE.md) exist on disk at this head.
• Every cited section heading exists, verbatim, in the target file: oob-detection §1 Classify the OOB / §2 Static Interval Analysis / §4 Fix Strategy (§4:103 does carry the i32-overflow-and-descriptor guidance the new Angle B text promises); debug-flydsl-kernel §6 Compilation Errors and §7.2 Barrier deadlock; lds-optimization LDS Instruction Model (:100); flydsl-tile-programming Step 5: Add Synchronization (:368); flydsl-kernel-authoring §1 Architecture and Compilation, §3 Control Flow / Runtime vs Compile-Time Conditions / Frontend Semantic Restrictions / Runtime Loops with Loop-Carried Values, §5 Shared Memory (LDS), §6 MFMA Integration; add-target-atom-op §1 Inherent Design and §2 The Files You Will Touch; kernel-code-cleanup §2, §3b, §3c, §4, §§6–8, §9, Cautions, §10; api-stability §1 Producer review / §2 Consumer review; format-code Check only.
• Nothing material was lost by inlining less. Spot-checking the highest-risk deletions: the buffer_load/buffer_store "offset is in elements" contract that Angle B used to state inline is preserved at kernel-code-cleanup/SKILL.md:119-120 ("a classic bug"), which is exactly the section the new Angle B cites.
• The prompt/heading contract still holds. ANGLES[].section in the workflow (:39-49) still matches the nine ## Angle … headings in SKILL.md (:93,105,115,135,148,159,178,202,216) character for character, and the new SHARED_RULES_PROMPT points at ## Reusing existing skills, which exists at :52. The rewrite did not drift the two apart.
• Guards and syntax. python3 scripts/check_repo.py → check_docs_api: OK (18 files, 894 known symbols, 17 skills), so the new skill cross-references pass the repository's own documentation guard. typed-arithmetic errored only because this checkout is a single-commit clone with no origin/main — an environment gap, not a defect in this PR. node --check on the workflow (wrapped, since top-level return and export const meta are by design) → OK; ast.parse on post_review.py → OK. black/ruff are not installed here, so I still could not run the Python style gate — reporting that as an unchanged gap.

Why this is still CHANGES_REQUESTED. This commit touches only prompt text. I diffed 3c040983...1e18661d through the compare API: the only change to .claude/workflows/flydsl-code-review.js is the four SHARED_RULES_PROMPT insertions, and .claude/skills/flydsl-code-review/scripts/post_review.py is byte-identical to 3c040983 and 5b0c1ce9 (blob SHA a72b952f33d4aa767b5fe39035f4b4735ecc44bf at all three). All five structural items from my last two reviews are open verbatim; only their line numbers moved (+9 in the workflow). I re-read each anchor at this head.

Still open:

1. Stage failure is indistinguishable from a clean review. :293 turns a failed finder into an empty success; :255 keeps an unchallenged CONFIRMED when the challenger returns nothing; :265 nulls a failed verifier and :301 filters it out; :324 skips a failed sweep silently. If those losses empty the candidate list, :354 returns No findings survived verification. while stats.finders still reports 9.
2. Every phase re-runs mutable gh pr diff <n> (:144), and the returned object at :391-397 carries no base/head OID, so a push landing mid-run mixes revisions invisibly.
3. Math.round(c.line / 5) * 5 buckets by proximity, not mechanism (:216), contradicting SKILL.md:83-84's own promise that two angles flagging the same line for different reasons are both kept; the completion-order budget compounds it, and budgetDropped is logged at :348 while the run still returns as complete.
4. Synthesis output is accepted unvalidated (:386). REPORT_SCHEMA has no candidate ID, no kind, and no verifier evidence, and nothing checks membership, verdict, or ordering — so the synthesizer can hand back as CONFIRMED a verdict the Challenge phase just downgraded at :257.
5. post_review.py posts each inline comment as an independent write (:164), never rechecks the head it read at :124, has no idempotency marker, and the deferred rows at :180-185 drop the verdict and failure scenario that body_for already formats.

Non-blocking, carried over: SKILL.md:74 still leaves Workflow-vs-inline selection to the reviewing model (0/3 in my measured runs).

What would close this: items 1–5 fixed, then a rerun of the frozen corpus reporting completion rate, recall@5, independently adjudicated precision@5, findings per PR, wall time, and cost, with the challenger's downgrade rate called out separately.

[jhinpan]

Still open at 1e18661d. This commit only inserted SHARED_RULES_PROMPT into the prompts; the failure accounting is unchanged. Restating the anchors at their new line numbers so the item is not lost:

• :293 — if (!r) return { angle: a, candidates: [] }: a failed or skipped finder becomes a successful empty result.
• :255 — if (!v || v.verdict === 'CONFIRMED') return c: a challenger that fails, times out, or returns nothing leaves the CONFIRMED verdict unchallenged. The safeguard fails open in exactly the direction it exists to close.
• :265/:301 — a failed verifier returns null and is filtered out.
• :324 — a failed sweep is skipped with no trace.
• :354 — emits No findings survived verification. while stats.finders still reports 9.

Track success/failure per required finder, verifier, challenger and sweep; return status: INCOMPLETE with the failed stage labels and unresolved candidate IDs, and never emit the clean summary from an incomplete run.

[jhinpan]

Still open at 1e18661d, unchanged since 5b0c1ce9. Every phase — Scope, the nine finders, every verifier, every challenger, the sweep, and publication — reruns mutable gh pr diff <n>. A push landing mid-run mixes revisions, and the returned object at :391-397 contains no OID, so it is not detectable from the result.

Resolve the repository, merge-base OID and head OID once; fetch those objects and have every phase run git diff <merge-base> <head>. Put both OIDs and a diff hash in the result, and abort or restart if the PR head no longer equals the reviewed head before posting.

[jhinpan]

Still open at 1e18661d, unchanged. Math.round(c.line / 5) * 5 buckets on proximity alone, not mechanism: a bounds bug at line 99 and a barrier bug at line 101 both key to file:100, and the second lands in dupes. That contradicts this skill's own instruction at SKILL.md:83-84 — "if two angles flag the same line for different reasons, record both".

admit also consumes slots in finder-completion order, so up to 54 candidates (9 angles × PER_ANGLE) compete for FINDER_VERIFY_BUDGET = 40 and an early convention angle can displace a later correctness candidate before any ranking. budgetDropped is logged at :348 but the run still returns as complete.

Deduplicate only on identical exact location and normalized mechanism/root-cause; apply a deterministic priority or reserve slots for all correctness candidates; return INCOMPLETE when not every candidate can be verified.

[jhinpan]

Still open at 1e18661d, unchanged. The success path accepts arbitrary schema-valid findings from synthesis: REPORT_SCHEMA (:81-96) carries no source candidate ID, no kind, and no verifier evidence, and nothing here checks membership, verdict or ordering.

This undercuts the Challenge phase the PR added. challengeConfirmed downgrades a bad CONFIRMED to PLAUSIBLE at :257, and the synthesizer — which sees the verdict only as text inside block — can emit that same finding back as CONFIRMED, because the enum permits it and no code compares against the verified record. It can equally introduce a finding no verifier ever saw, or let a convention finding displace a correctness one.

Give each verified candidate an immutable ID; have synthesis return only selected/grouped IDs plus its summary; reconstruct the final findings in code from the verified records and mechanically enforce verdict/kind ordering and the cap.

[jhinpan]

Still open at 1e18661d. This file is byte-identical to 5b0c1ce9 and 3c040983 — blob SHA a72b952f33d4aa767b5fe39035f4b4735ecc44bf at all three, which I checked against the contents API.

• Each inline comment is an independent POST .../pulls/{pr}/comments. If a later call or the summary comment fails, the earlier ones remain and a rerun duplicates them.
• head_sha is read once at :124 and never rechecked before posting, and the --dry-run path is not bound to the head a subsequent live run will use.
• The deferred rows at :180-185 keep only the summary, dropping the verdict and failure scenario that body_for already formats.

Require an expected reviewed head and recheck it immediately before posting; submit the inline comments and the deferred body in a single POST /pulls/{pr}/reviews; include a deterministic run/finding-set marker and skip an already-posted set; reuse body_for for the deferred rows.

[jhinpan]

Verdict: REQUEST_CHANGES — every prior item is fixed, but the runner cannot complete a single stage on Claude Code 2.1.263 when the user has verbose: true

This head replaces the JavaScript Workflow with scripts/run_review.py + scripts/review_common.py and adds tests/unit/test_code_review_runner.py. I re-checked all five of my prior items against this commit rather than against the old file, and re-ran the new suite. All five are fixed. I found one new defect that is material, and I reproduced it against the real CLI.

Prior items — all closed

1. Fail-open failure accounting (was flydsl-code-review.js:293/:255/:265/:301/:324/:354) — fixed. required_stages() (review_common.py:152-164) enumerates scope, all nine finders, one verifier per candidate, a challenger per CONFIRMED, sweep and synthesize; build_report() (:193-206) marks any of them without a COMPLETE output as failed, sets status: INCOMPLETE, and forces reported = []. The specific fail-open I called out — if (!v || v.verdict === 'CONFIRMED') — is now closed in the opposite direction at review_common.py:132-134: an unchallenged CONFIRMED gets verdict = None and lands in unresolved_candidate_ids. stats.finders_completed (:237) counts actually-completed finders instead of the constant 9. Covered by test_required_failure_never_returns_clean_review and test_missing_stage_is_not_complete_even_with_empty_findings.
2. Mutable gh pr diff <n> per phase, no OID identity — fixed. pin_scope() (run_review.py:122-235) resolves base/head OIDs from the API, fetches those objects into a private snapshot repo, computes the merge base, records base_oid/merge_base_oid/diff_base_oid/head_oid/diff_sha256/diff_command, and every agent runs against that checkout. check_snapshot() (:238-257) re-verifies head, cleanliness and diff hash before and after the agent phases; validate_report() (review_common.py:259-263) refuses to publish without all four OIDs and the diff hash; check_pr() (post_review.py:126-131) re-asserts both PR OIDs. test_pinned_scope_survives_source_push and test_modified_snapshot_is_incomplete cover it.
3. Proximity dedup + completion-order budget — fixed. collect_candidates() (review_common.py:100-101) keys on digest([file, line, mechanism]) — exact location and normalized mechanism, no Math.round(line/5)*5. The admission budget is gone; run_review.py:573-575 verifies all candidates and test_all_54_candidates_verified_and_correctness_has_priority asserts 54. test_nearby_or_same_line_different_mechanisms_survive is the direct regression for the line-99/line-101 case.
4. Unbounded synthesis — fixed. Synthesis is no longer a model: run_review.py:600 records a fixed stage and rank_findings() (review_common.py:144-149) builds the report deterministically from the verified records. validate_report() (:269-287) recomputes build_report() and rejects any artifact whose findings, risks, reported_ids, candidates, stats or metrics differ. test_challenge_downgrade_and_evidence_survive_synthesis and test_publisher_rejects_changed_provenance cover the CONFIRMED-upgrade path I described.
5. Non-atomic publication in post_review.py — fixed. One POST /pulls/{pr}/reviews at :171 carries the inline comments and the body; finding_set_marker() (:68-75) plus existing_review() (:134-142) make a repeat run a no-op and reconcile a lost response without reposting (:172-178); check_pr() runs both before routing and immediately before the write (:158, :166), so --dry-run and the live write are bound to the same head; deferred rows reuse body_for at :97, keeping the verdict, scenario and evidence; line is validated as a true int in validate_output (review_common.py:78). Covered by test_single_review_preserves_deferred_evidence_and_separates_risks, test_post_rejects_a_changed_head_before_or_during_routing, test_lost_post_response_is_reconciled_without_reposting, test_dry_run_does_not_write.

Copilot's --comment item is outdated — the workflow file is deleted and SKILL.md:22-35, 351-386 route --comment through post_review.py after the runner returns.

What I ran

• python3 -m pytest tests/unit/test_code_review_runner.py -q (Python 3.12, clean venv): 41 passed in 2.04s. No GPU, model or network needed, which matches the module docstring.
• Checked every flag in cli_agent's argv against claude --help on 2.1.263: --json-schema, --no-session-persistence, --disable-slash-commands, --permission-mode dontAsk, --tools, --strict-mcp-config, --mcp-config, --allowedTools, --effort all exist.
• Ran one real claude --print --output-format json ... invocation with the runner's exact flag set. That is what surfaced the blocker below.

The one blocker

run_review.py:369 requires the CLI's stdout to be a single JSON object. On Claude Code 2.1.263, --print --output-format json emits a JSON array of transcript messages whenever verbose is true — and verbose is an ordinary ~/.claude/settings.json setting that the runner never pins. Every agent then fails with CLI returned no terminal result record, so every stage is INCOMPLETE and the skill can never produce a review. Details and the reproduction are on the inline comment.

The existing CLI test cannot catch this: install_fake_cli in tests/unit/test_code_review_runner.py:334-338 only ever emits the single-object shape, so test_cli_requires_success_footer_and_no_permission_denials passes against a shape the real CLI does not always produce.

One gap I could not close

Item 5 of my earlier follow-up asked for a corpus rerun with completion rate, recall@5, adjudicated precision@5, wall time and cost. My measurements at 5b0c1ce9 are outdated — that implementation no longer exists — and I cannot run the corpus in this environment. I am not blocking on the numbers, but I note that a single end-to-end run on this head would have caught the envelope defect immediately, which is the argument for doing it before merge rather than after.

Non-blocking notes

• command() (run_review.py:75) caps every git invocation at 120 s. When neither PR OID is present locally, pin_scope fetches from https://github.com/<repo>.git; on a cold cache for a repo this size that can exceed 120 s and surfaces as a scope failure rather than a fetch timeout. Worth a separate, larger budget for the fetch.

The architecture is now what I asked for: pinned OIDs, per-candidate verification with no budget, deterministic synthesis, checkpointed resume, one atomic publication. Fix the envelope parsing (and pin the shape or accept both) and I expect to approve.

[jhinpan]

Blocker: this rejects the real CLI's output whenever verbose is true, so no stage can ever complete.

--print --output-format json does not always emit a single result object. On Claude Code 2.1.263, with verbose: true in ~/.claude/settings.json, it emits a JSON array of transcript messages whose last element is the result record. isinstance(envelope, dict) is then false and every agent attempt dies with CLI returned no terminal result record — so Find fails, review() returns at :572, and build_report yields status: INCOMPLETE on a machine where nothing is actually wrong.

Reproduced here with this file's exact argv (--print --output-format json --json-schema ... --no-session-persistence --disable-slash-commands --permission-mode dontAsk --tools Read,Grep,Glob,Bash --strict-mcp-config --mcp-config '{"mcpServers":{}}' --allowedTools ...), CLI 2.1.263:

parsed type: list
run_review.py:369 raises -> ValueError("CLI returned no terminal result record")
terminal record is present at index -1: result success {'status': 'COMPLETE', 'limitations': []}

The run itself succeeded — structured_output on the terminal record was exactly the schema-valid {"status": "COMPLETE", "limitations": []} the runner wants. Only the parse fails. Removing verbose (or adding --settings '{"verbose": false}') makes the same command return a single dict again; I confirmed both directions.

This is the same class of leak the argv already guards against everywhere else: --strict-mcp-config, an empty --mcp-config, an explicit --tools/--allowedTools set, --disable-slash-commands and --no-session-persistence all pin the agent's environment, but the output shape is left to whatever the invoking user has configured.

To close: take the terminal record shape-tolerantly — when the payload is a list, use the last element with type == "result" and fail only if there is none — and/or pin the shape by passing --settings with verbose false. Please also extend install_fake_cli (tests/unit/test_code_review_runner.py:334) with an array-shaped case; test_cli_requires_success_footer_and_no_permission_denials currently only exercises the single-object shape, which is why the suite is green against a CLI that breaks the runner.

[jhinpan]

Hi @zhiding512 — I opened #1124 to distill the useful parts of #1047 into your review runner. It targets your add-flydsl-code-review-skill branch directly; the intent is to fold this extraction into #1106 so the combined work goes to main through your PR.

The extraction brings over two deterministic checks: legacy API spellings on added kernel lines, and added tests missing a statically visible script entry path. Their raw leads feed your existing Angles G/I and continue through your verifier/challenger pipeline. There is one review skill and runner. Preflight failures, checkpoint/resume, and pinned-diff identity are integrated with your existing artifact contract.

It also fixes the verbose CLI transcript-array issue from the latest review, plus four scanner edge cases found during independent review. Validation is 111 passing CPU tests, a successful real Claude Code 2.1.263 run with verbose: true, the frozen #481 replay finding all three omitted fused/quant tests, and clean repository/style checks. The final integration received another independent review with no remaining reproduced blocker.

Details and the validation limits are in #1124. The goal is to retain your review architecture and add the deterministic coverage from #1047.

[jhinpan]

Verdict: APPROVE — the envelope blocker is closed, reproduced live against the real CLI at this head

Disclosure first. The head commit bc5c97a is the merge of my own #1124 into this branch, so the fix for my last blocker is code I wrote. I did not take it on trust: everything below is a command I ran against this checkout at bc5c97a, including one live Claude Code 2.1.263 invocation through the runner's own cli_agent.

The one blocker from my last review — fixed, verified end to end

My comment at 62be8677 was that run_review.py required the CLI's stdout to be a single JSON object, so every stage died with CLI returned no terminal result record whenever the invoking user had verbose: true.

run_review.py:377-390 now reduces a transcript array to the last type == "result" record before the dict check. I verified it against the real CLI rather than a stub: I created a temp directory containing .claude/settings.json with {"verbose": true}, used it as the agent cwd, and called runner.cli_agent() directly with the runner's own argv and schema.

raw stdout type: list len: 5
attempt status: COMPLETE error: None
output: {'candidates': [], 'limitations': [], 'status': 'COMPLETE'}

The CLI emitted the array shape that used to break the runner, and the runner completed. The exact failure I reported no longer reproduces.

The test gap I named is closed too. test_cli_requires_success_footer_and_no_permission_denials (tests/unit/test_code_review_runner.py:341-392) is now parametrized over shape ∈ {object, transcript} across all eight modes, and the transcript case deliberately places an earlier successful record with total_cost_usd: 99 before the terminal one — so it pins the property that matters, that a prior success cannot mask the final record's failure, denial, malformed output or usage.

Prior items 1–5 — still fixed at this head

All five were closed at 62be8677 and I confirmed none regressed. post_review.py is not in the 62be8677...bc5c97a compare at all, so item 5 is untouched. review_common.py changed only by SCHEMA_VERSION, PREFLIGHTS, validate_preflight, and the two preflight hooks in required_stages/build_report — collect_candidates's exact location-and-mechanism digest, rank_findings, and validate_report's recompute-and-compare are unchanged. In run_review.py the command → command_result split preserves the old behavior exactly: command() still raises RuntimeError on a nonzero exit, and command_result still raises TimeoutError on cancellation or deadline both before spawning and inside the wait loop.

The new preflight — checked, not waved through

• It cannot fail open. PREFLIGHTS labels are added to required_stages() whenever the scope has files, and build_report additionally runs validate_preflight on a completed stage, so a stage that is missing, errored, timed out, exited with anything other than 0/1, or lost its stdout/stderr forces status: INCOMPLETE and reported = []. Covered by test_preflight_failure_is_incomplete_and_resume_retries_only_failed_scanner, test_publisher_requires_both_preflight_stages, and test_malformed_preflight_cannot_be_published.
• Identity is pinned. The stage fingerprint is digest({head_oid, diff_sha256, scanner source}), a cached-but-changed input raises rather than resuming, implementation_hash() now includes both scanner files, and check_snapshot gained a hash check on the saved diff.patch — which matters because the scanners read that file rather than re-deriving the diff.
• Leads stay leads. preflight_context() injects the raw scanner output into the Angle G / Angle I finder prompts labelled unverified leads, not findings; promotion still goes through the normal verifier and challenger. Preflight stages record under runs, not attempts, so they are correctly excluded from usage_metrics' agent-cost aggregation.
• The scanners do what the doc says. I ran both on a synthetic diff. scan_legacy_spelling.py reported kernels/moe/x.py:5: buffer_ops.* and correctly ignored the identical spelling on the comment line 4 (exit 1); scan_unreachable_tests.py reported test_beta:5: no statically supported entry path found while leaving test_alpha alone because __main__ calls it (exit 1); both returned exit 0 with no candidates on the pytest.main([__file__]) control. Their imports are re/tokenize/ast only — the SKILL.md claim that neither executes or imports reviewed code holds.

What I ran

• pytest tests/unit/test_code_review_runner.py tests/unit/test_review_legacy_spelling.py tests/unit/test_review_unreachable_tests.py -q → 111 passed in 8.15s (Python 3.12, clean venv, no GPU/network/model).
• One live cli_agent run against Claude Code 2.1.263 with verbose: true, output above.
• Both scanners on a positive diff and a clean control, outputs above.
• python3 scripts/check_repo.py → check_docs_api: OK (18 files, 894 symbols, 17 skills). typed-arithmetic errors only because this checkout is a shallow single-commit clone with no origin/main — the same environment gap as my previous rounds, not a defect in this PR.
• The Python style gate, which I could not run before, now runs. black --check and ruff check on the three changed in-scope test files → 3 files would be left unchanged / All checks passed!. .github/scripts/check_python_style.sh excludes the .claude/ prefix, so the skill scripts are out of the gate's scope by design; for information, two lines there exceed 120 columns (run_review.py:609, scan_legacy_spelling.py:181). That gap from my earlier reviews is now closed rather than carried.

Remaining, non-blocking

• command_result still caps every git invocation at 120 s (run_review.py:77). On a cold cache, pin_scope's fetch of a repo this size can exceed it and surface as a scope failure rather than a fetch timeout. Carried over as a note, not a blocker.
• The frozen-corpus numbers (completion rate, recall@5, adjudicated precision@5, wall time, cost) are still not reported. I am not blocking on them — my own measurements were against an implementation that no longer exists, and I cannot run the corpus here. references/preflight-evidence.md is honest about this: it labels its own basis as historical and seeded selection results rather than held-out precision or recall. Worth producing once after merge.

The architecture is what I asked for across five rounds — pinned OIDs, per-candidate verification with no budget, deterministic synthesis, checkpointed resume, one atomic publication — and the last operational defect no longer reproduces against the real CLI. Approving.

[coderfeli]

Approve for kernel only. need compiler parts releted review bullets.