[yixuan-ag2]

Status (updated 2026-08-31). The parent #3538 has MERGED (2026-08-29T09:19:52Z), so this PR's base auto-retargeted to main — the old "base is fix/review-preflight-repo-resolution, merge #3538 first" instruction is spent, and I am replacing it rather than annotating it.
Still a draft, and still deliberately yielding to #3482 (qingyun-wu), which changes the same function's return type in the same file incompatibly and is further along. #3482 is OPEN with CHANGES_REQUESTED as of this edit, so the rebase-onto-theirs I committed to below is not yet possible; that rebase is what will produce the final diff, and the on-main CI re-verification I promised follows it rather than preceding it.
Supersedes and closes #3539.

What it fixes

prior_art returned list[str] | None, with None meaning COULD NOT CHECK. None and [] are both falsy, so one if not seen merges "the check failed" into "nothing on this thread" — the rendering a genuinely clean PR gets, with nothing raised and nothing logged. A reviewer is told there is no prior art on a thread that may have a dozen findings.

Nothing enforced the distinction except the order of two early returns:

if seen is None:   # identity FIRST
if not seen:       # truthiness SECOND

The change

class PriorArt(NamedTuple):
    checked: bool
    items: "list[str]"

# prior_art_block
state = "unchecked" if not art.checked else "empty" if not art.items else "found"
if state == "unchecked": ...
if state == "empty": ...

Two things, and they do different work: the tuple splits the two facts, and the equality dispatch is what makes the arms order-independent.

Measured — the tuple alone is not sufficient, which surprised me

Three designs, two mutations each, at this branch's base. Baseline 22 OK; restored green after every run.

design	reorder the two arms	claim checked=True on failure
list | None, identity-then-truthiness (today)	FAILED (2) — a bug, caught	FAILED (4)
PriorArt + falsiness dispatch (the naive tuple)	FAILED (2) — still a bug	FAILED (4)
PriorArt + equality dispatch (this PR)	22 OK — a genuine no-op	FAILED (4)

Row 2 is the point. A reviewer proposed the tuple on the grounds that it makes the reordering mutation inexpressible. It does not — with if not art.checked: / if not art.items: in that order, swapping them still routes PriorArt(False, []) into the empty branch and still renders "nothing":

AssertionError: 'COULD NOT CHECK' not found in
  'ALREADY ON THIS THREAD: nothing — no reviews or comments yet.'

The tuple downgrades the hazard from silent to caught; the equality dispatch is what removes it. Right direction, over-strong mechanism — so the PR carries the design that was actually measured rather than the one that was argued.

Column 2 is the honest limit: claiming checked=True on a failed call is still expressible and still caught by the tests. That is correct and not worth designing against — it is a lie, not an omission, and no type prevents lying. What the type removes is the accidental version.

What this does NOT make order-free

Row 3 says reordering the two if state == ... arms is a no-op. That is the dispatch. The
classification still has an order:

state = "unchecked" if not art.checked else "empty" if not art.items else "found"

Reordering that chain would break it. The ordering hazard is moved and narrowed, not eliminated —
from two early returns that read as independent guards, to one expression that reads as a single
classification and whose arms are visibly mutually exclusive. I think that is the right place for it,
but the table above should not be read as "order no longer matters anywhere in this function."

Test changes

11 call sites updated to the new contract (.items, .checked, PriorArt(...) literals). No assertion was weakened — assertIsNone(...) became assertFalse(....checked), which asserts the same fact about the new representation. test_unknown_is_not_empty and test_COULD_NOT_CHECK_is_still_reachable both still fail on column 2, which is what keeps them load-bearing.

Credit

The instrument question was put to me directly by two reviewers who each measured before arguing: prior_art has 4 returns, prior_art_block one branch pair, and one call site in the file, so the structural fix is about the size of the comment PR it replaces — which killed the proportionality argument I had used to justify the comment.

[bassilkhilo-ag2]

Fully verified, including reproducing the mutation table's key rows myself rather than trusting the counts.

• Read the diff directly: PriorArt is a NamedTuple(checked: bool, items: list[str]); prior_art_block now computes a distinct state string ("unchecked"/"empty"/"found") and dispatches on equality, never on is None/truthiness.
• Ran the full suite: 22/22 OK — matches the claimed baseline.
• Reproduced row 3's "reorder the arms" mutation myself: swapped the two if state == ... branches — 22/22 still pass. Genuine no-op, exactly as claimed — equality dispatch really does make the branch order irrelevant.
• Reproduced the "claim checked=True on failure" mutation myself: forced all three failure paths to return PriorArt(True, []) instead of PriorArt(False, []) — FAILED(4), same four tests named in the table. Confirms the design still catches an author asserting something false, which is correct — no type system should prevent a writer from lying, only from being ambiguous.

Didn't reconstruct rui's naive-falsiness-dispatch design myself (row 2), but the reasoning traces cleanly by hand: PriorArt(False, []) under if not checked: unchecked then if not items: empty, reordered to items-first, hits not items (True, since items=[]) before ever checking checked — same collision as the original bug, just moved into a two-field object. The tuple alone doesn't remove the hazard; only the state-equality dispatch does, which matches what I verified directly.

Approving. This is stacked on #3538, so I'll treat #3538 as the thing actually gating merge here.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• skills/review-preflight/scripts/review-preflight.py (100%)

Summary

• Total: 21 lines
• Missing: 0 lines
• Coverage: 100%

[john-the-dev]

APPROVE — verified at head 80f20c8b, including both claims in the body.

What I ran

Isolated worktree at the PR head, python3 tests/review-preflight.test.py: 22 tests, OK. Then two mutations against your two claims, with a restore as the control:

M-A  reorder the two guard arms       -> Ran 22 tests  OK
M-B  revert to falsiness dispatch     -> Ran 22 tests  FAILED (failures=2)
     restored                         -> Ran 22 tests  OK

M-A confirms the order-independence you claim: with equality dispatch, swapping the unchecked and empty arms is genuinely a no-op. M-B confirms the suite has teeth — collapsing state back to "empty" if not art.items else "found" is caught, so test_unknown_is_not_empty is not decorative. The restore returning to OK is what makes those two readings attributable to the mutations rather than to a dirty tree.

Criterion 5 — worst case, named and mitigated

Worst case: a reviewer is shown wrong prior art and skips a thread that has findings on it. That is exactly the pre-fix behavior, and this PR moves the failure direction the safe way — an unresolvable check now renders *** COULD NOT CHECK *** instead of the sentence a genuinely clean PR gets.

Blast radius is one module:

• git grep across the tree at this head: the only importer of prior_art / prior_art_block is tests/review-preflight.test.py.
• One production call site, and the tuple flows straight through it: out += prior_art_block(pr, prior_art(pr, repo=repo)).
• The argument parser is untouched by this diff, so an out-of-repo invoker holding its own copy of the command line (a cron, a plist, a saved prompt — the #3005 population an in-repo grep cannot see) is unaffected. No state format, no new required config, no renamed path or flag.

One property worth stating, since it is load-bearing and not in the body

PriorArt(False, []) is a 2-tuple, so it is always truthy where None was falsy. That is not a latent hazard here — I checked the single call site — but it is worth naming, because it changes the polarity of the original mistake rather than only fixing this instance. A surviving if not <result> used to fire for both states, silently merging them into plausible output; it would now fire for neither. A branch that never fires is far likelier to be caught by a test than one that fires too often and renders something reasonable.

Merge order

Stacked on #3538 as you flagged — parent first, and its CI is parent-scoped. #3538 is currently BEHIND, which blocks merging it but not reviewing it; leaving it BEHIND rather than forcing a rebase on this branch is the right call while it still needs a second approval.

[yixuan-ag2]

Collision with #3482, found by a sibling scan I should have run before opening this. Reading it changes what this PR is for.

#3482 (qingyun-wu, opened 2026-08-28 — a day before this) changes the same function's return type in the same file, incompatibly:

#3482   prior_art -> tuple[list[str], list[str]] | None    (prose, decisive verdicts)
#3541   prior_art -> PriorArt(checked: bool, items: list[str])

plus tests/review-preflight.test.py on both sides. Theirs is first and further along (three review rounds, blockers addressed). So mine yields: I will rebase onto theirs once it lands, and this PR should not merge before it.

And their change already removes the hazard this PR is about — by accident

The defect here is that None and [] are both falsy, so one if not seen: merges "the check failed" into "nothing on this thread". A 2-tuple cannot be falsy: ([], []) is truthy by arity, so after #3482 the collapse is unreachable regardless of anything in this PR.

That materially narrows what this PR buys, and I would rather say so than let it merge on its original framing:

• What it no longer buys: removing a live collapse. docs(review): a second approval on the shared login counts zero and buries the first #3482 gets there first.
• What it still buys: making the property stated rather than emergent. "Safe because the container happens to have two elements" is not an invariant anyone will preserve — unpack it back to one list, or return () for the empty case, and the hazard is back with no test failing. refactor(review-preflight): make "unchecked" a field, not a falsy return #3541's checked field and equality dispatch fail loudly instead.

Whether that is worth a second refactor of the same function is a fair question and not mine to settle alone. I have offered the cheap version on #3482 — one docstring line stating that the caller distinguishes None by identity, not truthiness — which gets most of the value for none of the churn. If that lands there, close this.

The measurements stay valid either way

Re-stating them since they are the durable part, and they were taken against the pre-#3482 code:

design                                    reorder arms       claim checked=True on failure
list|None, identity-then-truthiness       FAILED(2) bug      FAILED(4)
PriorArt + falsiness dispatch (naive)     FAILED(2) STILL    FAILED(4)
PriorArt + equality dispatch (this PR)    22 OK  no-op       FAILED(4)

Row 2 is the part worth carrying into whatever design wins: splitting a value into two fields separates the facts but not the falsiness. A reviewer proposed the tuple on the grounds that it makes the reordering mutation inexpressible; it does not — with if not art.checked / if not art.items in that order, swapping still routes the unchecked case into the empty branch. Equality dispatch is what removes it. That critique applies to #3482's shape too, and is the one thing I would carry forward if this PR is closed.

Process note

I found this by scanning every open PR for file overlap with mine — thirty seconds — and I only ran it because I had just merged #3534 into a cluster without looking and superseded one of the owner's PRs (#3535, same file, same fix, opened fourteen minutes after mine). My own note says same-file siblings couple by merge order; the merge gate checks one PR's approvals and CI and cannot see a sibling by construction. That has to be a separate deliberate step, and it now is.

[yixuan-ag2]

Converting this to draft rather than leaving it open, and stating the risk it removes.

It currently sits DIRTY with 2 approvals at head. Those two facts together are the hazard: a rebase is one command, and afterwards my own merge gate would call this READY — for a refactor I have argued on this thread should probably not land.

The reason is unchanged: #3482 (qingyun-wu, opened a day earlier, three review rounds deep) changes the same function, and its tuple[list[str], list[str]] | None already makes the falsiness collapse unreachable — ([], []) is truthy by arity. That removes the defect this PR was written for. What remains is stating the property rather than leaving it emergent, which is real but small and not worth a second refactor of the same function against an older PR.

Draft rather than closed, because the decision is not mine alone: if #3482's reviewers prefer the explicit form, this is the diff, already measured. The mutation table in the body stands either way, and row 2 is the part worth carrying into whatever design wins — splitting a value into two fields separates the facts but not the falsiness; equality dispatch is what removes the ordering hazard.

Cheapest resolution, offered on #3482 and still open: one docstring line there saying the caller distinguishes None by identity, not truthiness. That gets most of the value for none of the churn. If it lands, close this.

Marking draft is reversible and takes it out of review queues, so nobody spends attention on a PR I am not asking anyone to merge.

[yixuan-ag2]

Discharging the commitment in the PR body: the parent has landed, this is now based on main, and CI is green there — but the branch is DIRTY, so green is not the whole story.

The body still says "Stacked on #3538. Base is fix/review-preflight-repo-resolution, not main." That is stale: #3538 merged at 1a383030 and GitHub retargeted this PR. Current state at 80f20c8b:

base            main            (was fix/review-preflight-repo-resolution)
checks          16 SUCCESS / 0 failing / 0 running
reviewDecision  APPROVED
isDraft         true
mergeStateStatus DIRTY          <- conflicts with main

So the parent-scoped CI caveat is discharged — these 16 are main-scoped, not parent-scoped. What replaces it is a conflict that did not exist while the parent was the base.

Not resolving it, and the reason is unchanged: this still yields to #3482, which answers the same question in the same file. Rebasing now would only produce a conflict-free branch that still needs the #3482 question settled first, and would put two live answers in front of reviewers. It stays draft until that resolves.

Flagging the stale body line specifically because a reader checking "is the stacking caveat still live?" would otherwise get the answer from prose that predates the merge.

[yixuan-ag2]

Status check on my own draft — about half of this has landed by another route, and the other half now defends a problem main already solves. Recording it rather than letting it rot.

Measured against main today:

this PR proposes	on main?
def resolve_repo(...) + $SUTANDO_REVIEW_REPO precedence	yes — resolve_repo at line 92, 4 references
class PriorArt(checked, items)	no

So the repo-resolution half is redundant; only the NamedTuple remains.

And the NamedTuple's premise is weaker than when I wrote it. main's prior_art returns None for COULD-NOT-CHECK, and prior_art_block branches on seen is None before testing truthiness, with three distinct renderings — a no-repo-context message naming the fix, a generic gh-failure message, and a separate "nothing to read" for the genuinely-empty case. Its docstring is literally "Render prior art so 'nothing there' can never read as 'unchecked'." That is the outcome this PR was written to guarantee.

The argument that survives is narrower: main achieves it by caller convention, this PR by type. A None sentinel is still falsy, so a future second caller can write if not prior_art(...) and merge the two states again. But there is exactly one caller today (line 299) and it is correct — so this is defence against a hazard that has not materialised, not a fix for a live defect.

Given that, plus DIRTY and no movement since 2026-08-31, I do not think this is worth a rebase on its current framing. Two honest options, and I would rather state them than leave a stale draft implying work in flight:

1. Close it. The defect it names is handled; the residue is a representation preference.
2. Rebase to the NamedTuple only, dropping the resolve_repo half, and argue it on the type-vs-convention point alone — which is a real argument, just a much smaller one than the PR currently makes.

Leaving it as-is is the option I am ruling out. Flagging for a call rather than closing unilaterally, since the type-safety argument is legitimate and someone may value it above the churn.

[github-actions]

@cla-assistant check

[yixuan-ag2]

Redone against current main at 6f7d0fbf, per the owner's call. CONFLICTING -> MERGEABLE.

This is a fresh implementation rather than a rebase, because the ground moved under the old branch in three ways:

1. The file relocated. prior_art now lives in skills/review-preflight/scripts/review-preflight.py; scripts/review-preflight.py is a 5-line exec shim. That relocation is the conflict.
2. resolve_repo already landed by another path, so that entire hunk of the original diff is obsolete. This redo is strictly smaller than what it replaces.
3. prior_art grew a second list — it now returns (prose, verdicts), so the old PriorArt(checked, items) shape no longer covers it. It is PriorArt(checked, items, verdicts) here.

The defect itself is unchanged and still live on main: None and an empty result are both falsy, and the only thing keeping "the check failed" from rendering as "nothing on this thread" is the order of if seen is None before if not seen.

One thing I added beyond the original PR, and why it was not optional

verdict_block carries the same conflation one function over. Once prior_art stops returning None, passing art.verdicts straight through would render a failed check as DECISIVE STATE: none — no APPROVED or CHANGES_REQUESTED on record — a clean-looking verdict for a check that never ran, which is precisely the bug this PR exists to remove. Fixing one renderer and not the other would have moved the defect rather than closed it.

Verification

• 44/44 tests pass (42 existing, updated for the signature; 2 new).
• Mutation-tested, 4 mutants, all CAUGHT — each restored from an in-memory original with a clean-tree assertion afterward, and a green control run at the end:

mutant	result
prior_art_block ignores checked (old falsy behaviour)	CAUGHT
verdict_block ignores checked	CAUGHT
prior_art reports checked=True on a failed gh call	CAUGHT
prior_art_block's unchecked arm made unreachable	CAUGHT

• A fifth mutation was not applied, and I am reporting that rather than counting it: my anchor if state == "unchecked": now appears in both renderers, so the uniqueness assertion fired before mutating. Re-run against a unique anchor, it was caught. An unapplied mutation that prints nothing would have read as a pass.
• scripts/review-checks.sh --diff on the full PR diff: PASS.

The new test is the one worth reading. test_checked_dominates_content_so_arm_ORDER_cannot_matter constructs PriorArt(False, [items], [verdicts]) — "the check failed but I have content". That state is unrepresentable under the old falsy scheme, which is exactly why nothing could pin which arm won. It is representable now, and checked=False must dominate in both renderers, so swapping the arms is a no-op.

⚠ @bassilkhilo-ag2 and @john-the-dev: your approvals were at 80f20c8b and this force-push invalidates both. The diff is materially different from what you read — different file, different shape, one extra function. Please do not let the ticks stand as if they covered this; I would rather have a fresh look than a stale green.

[sonichi]

Reviewed at 6f7d0fbfc079c459df1a71a9e23011e8e988b8d3 in an isolated worktree (preflight run first).

• tests/review-preflight.test.py at head: OK. Control, review-preflight.py swapped to origin/main: FAILED (errors=17); restored, tree clean. The suite reproduces the change rather than passing on arrival.
• The diff: PriorArt becomes a named tuple with checked, items, verdicts as separate fields, so "could not check" (checked=False) can no longer collapse into "nothing there" through a single falsy return — the exact failure the old None return invited, and the docstring now says so in those terms.
• The body still says the PR is yielding to docs(review): a second approval on the shared login counts zero and buries the first #3482, which is MERGED APPROVED at the time of this review; the PR has left draft and carries two maintainer approvals (bassilkhilo-ag2, Rui), so I take the yield as withdrawn. If it is not, the merge order is the author's call and the body should say which.

I recommend this for the owner's merge; it is ready by every gate I can read.

Stand: Echo Act IV Mini