feat(workflow): mandate querying all available providers for adversarial review

[d-morrison]

No description provided.

[cursor]

Independent adversarial review of HEAD fe53d4209235c4bb5786f7e964dbe1c603f5db91. First review of this PR.

The diff is a six-line append to shared/workflow/adversarial-self-review.md requiring a clean verdict from every available provider on the same commit, queried sequentially.

Findings

1. [Defect] The new section requires every available provider to return a clean verdict on the same commit. A single Needs more work forces a fix, a new SHA, and a restart of the whole set. That is the unbounded hold the paragraph immediately above forbids: do not wait for a future clean round before pushing. An agent following the new heading will sit on unpushed commits until five serial clean results land on one SHA, which is the Enforce local adversarial self-review before git push #1911 failure this file already records.

2. [Defect] hooks/no-push-without-self-review.py admits a verdict only from a foreground adversarial-reviewer Agent tool_result. A Codex, OpenCode, or agy CLI report never becomes that tool_result. The fragment already names ALLOW_UNREVIEWED_PUSH=1 as the override. The new mandate never restates that floor, and "Do not stop after one provider returns clean" treats the hook's sufficient condition as insufficient.

3. [Factual Error] Antigravity (agy) is listed as an available provider. shared/workflow/self-review-fallback.md and skills/agy-review-workflow/SKILL.md state it is permanently out of service (user directive, 2026-08-20). Dispatching it burns a run and leaves a red check.

4. [Factual Error] The list conflates harnesses with vendors. This same file already says a same-vendor subagent buys independence of intent, not of vendor. Cursor and Claude listed as two sign-offs are typically that same-vendor pair. Copilot, the requestable PR reviewer self-review-fallback.md actually names, is absent. OpenCode is a cheap mechanical-work delegate, not a peer whose clean verdict this corpus treats as coverage.

5. [Defect] The fallback fragment requires requesting a different configured reviewer in parallel with posting the self-review. The new text requires sequential, one-at-a-time queries. If those sequential reviews are posted, later providers also receive the PR-history channel this same file just forbade as rationale.

6. [Convention] "we need" is first person. Shared fragments must stay audience-neutral. "Do not stop after one provider returns clean. Query them sequentially, one at a time." is two sentences on one newly added Markdown line (new-line-breaks).

Verdict: Needs more work

Reviewed-Commit: fe53d42

Posted by Cursor Grok 4.6 (AI agent) --- not written by a human.

[cursor]

Cursor Grok 4.6 (cloud session) is reviewing this PR --- please hold off on pushing to this branch until the review comment lands.

Posted by Cursor Grok 4.6 (AI agent) --- not written by a human.

[cursor]

Claude finished adversarial review of this PR.

Reviewed-Commit: 8fcdb14

The diff appends ## Query all available providers sequentially to shared/workflow/adversarial-self-review.md. Delayed comments on fe53d420 and earlier SHAs are not this HEAD. Claude skip notices are not reviews.

Findings

1. [Defect] shared/workflow/adversarial-self-review.md:213-222 reinstates the unbounded hold the preceding section forbids. Every pinned provider must confirm zero open findings on the same commit and working tree; a fix is a new commit, so the quorum recedes with every round. The circuit breaker covers contradiction/oscillation, not successive legitimate findings (Enforce local adversarial self-review before git push #1911).
2. [Defect] :225-226 wait-and-retry on offline or 500s has no timeout. self-review-fallback.md already says not to stall for a down bot.
3. [Factual Error] :213 lists Antigravity (agy) as a pinned provider. It is permanently out of service (2026-08-20; ai-config#1776). Wait-and-retry on it never returns.
4. [Factual Error] The hook no-push-without-self-review.py admits only a foreground adversarial-reviewer Agent tool_result. Codex/OpenCode/agy CLI verdicts never become that result. The new quorum is invisible to the only gate that exists.
5. [Defect] :218 treats a working-tree diff as fingerprintable. The hook compares shipped commits, not the index.
6. [Defect] :219-221 require submitting rebuttals and obtaining explicit acceptance. This file forbids briefing the reviewer with the case for the change. "Zero findings remain open" also drops Defer.
7. [Defect] Sequential every-provider quorum contradicts the floor already in this file (one adversarial-reviewer plus a reachable cross-vendor chase in parallel). Cursor and Claude listed as two seats typically double-count one vendor. Copilot is absent.
8. [Convention] First person "we need" in a shared/ fragment. Several added lines pack stacked clauses (semantic line breaks).

Verdict: Needs more work

Posted by Cursor Grok 4.6 (AI agent) --- not written by a human.

[cursor]

Review posted --- unclaiming.

Posted by Cursor Grok 4.6 (AI agent) --- not written by a human.

[d-morrison]

@claude review

[github-actions]

👀 Picked up by workflow run #33034476946. Setup runs first; Claude itself responds after that.

[d-morrison]

@claude review

[github-actions]

👀 Picked up by workflow run #33034900782. Setup runs first; Claude itself responds after that.

[github-actions]

PR Preview Action v1.8.1
🚀 Preview available at: https://Morrison-Lab.github.io/ai-config/pr-preview/pr-2256/

_{Built to branch gh-pages at 2026-08-27 03:00 UTC.}

[github-actions]

👀 Claude Review Dispatched — run reviewing PR #2256 at 5bda5eb.

[github-actions]

Claude finished review — View run

---

I have everything needed. Here is the review.

---

Code review

Summary: This round of PR #2256 adds a --quorum N flag to scripts/check-pr-fully-clean.py for multi-provider review admission, adds Cursor/Codex/OpenCode bot recognition, and rewrites several shared/workflow/* and skills/* docs to mandate querying every reachable adversarial-review provider before a PR counts as fully clean. This is round ~10+ of review on this PR; I re-verified the fixes from the most recent posted rounds (Cursor bot-login recognition, _reviewer_identity-keyed dedup, doctrine reconciliation across fully-clean.md/skills/mwc/SKILL.md) and confirmed those are genuinely present at the current head (5bda5eb). I did not re-raise any of those. I did find two new, execution-verified issues that have not been raised in any prior round, plus one minor artifact.

Findings

1. [Security regression] COLLABORATOR was reintroduced into the review-admission trust boundary after a prior round explicitly removed it as a fix — with no visible justification
scripts/check-pr-fully-clean.py:1146 and scripts/check-pr-fully-clean.py:1171:

is_bot_author = _is_bot_author(author_login) or (
    author_assoc in ("OWNER", "MEMBER", "COLLABORATOR") and _reviewer_identity(body, author_login) not in (author_login, "unknown")
)

The prior Claude review round (head 8e535bcf..., posted 2026-08-27T01:51) explicitly flagged that COLLABORATOR doesn't reliably imply push/merge access ("an invited collaborator can have read-only or triage-only permission... an account without push/merge rights could post a comment containing a recognized marker string... and have it counted toward the quorum"). The very next round (head 4673ec2b..., posted 2026-08-27T02:12) confirmed this was fixed: "COLLABORATOR was removed from both check_review_comments admission checks, narrowing to OWNER/MEMBER. Not re-raised." This was reconfirmed as fixed by the round after that (head cf48d413..., whose quoted code snippet shows author_assoc in ("OWNER", "MEMBER")).

The current head's own commit message is "Address Round 19 adversarial review findings: fix semantic line breaks and restore COLLABORATOR to author_assoc" — i.e. this is a deliberate re-addition, not an accidental merge artifact. I checked the full PR comment history for any rebuttal or explanation of why the narrowing was reverted (no test requires COLLABORATOR — I confirmed grep -n COLLABORATOR scripts/test_check_pr_fully_clean.py returns nothing) and found none: the most recent ARD summary comment predates this change, and no comment since explains the reversal. This reintroduces exactly the risk the fix addressed: an account with COLLABORATOR association (which GitHub does not guarantee has push/merge rights) can post a comment or review containing a recognized agent-marker phrase and have it counted toward --quorum, satisfying the mwc autonomous-merge gate.

2. [Correctness/security] --quorum 0 bypasses the review gate entirely when the PR has some automated review in its history but none at the current HEAD, contradicting this same PR's own "fail closed" doctrine
scripts/check-pr-fully-clean.py:1227-1231:

if not matching_items:
    if quorum == 0:
        print(f"✓ Found 0 clean review(s) evaluating HEAD SHA {sha[:8]}, meeting quorum of 0.")
        return True, issues
    issues.append(f"No review comment has been posted evaluating HEAD SHA {sha[:8]} yet")
    return False, issues

This PR's own new prose in shared/workflow/adversarial-self-review.md states: "If the quorum size is zero at the start of the cycle, or drops to zero at any point during the cycle, you must fail closed and wait until at least one becomes reachable... Do not bypass the review gate." The code does the opposite: when matching_items is empty (nothing evaluates the current HEAD SHA — the exact "no review at HEAD" case criterion 2 of fully-clean.md exists to catch) and --quorum 0 is passed, the function returns (True, issues), i.e. exit code 0, "fully clean," with zero reviews having ever evaluated the current commit. I reproduced this by execution:

# PR has one stale Claude review from an OLDER commit, nothing at the new HEAD:
checker.check_review_comments("9999", "newsha000", "owner/repo", quorum=0)
# => ok=True, issues=[]   (prints "Found 0 clean review(s) ... meeting quorum of 0")

No test in the diff exercises quorum=0 at all (grep -n quorum scripts/test_check_pr_fully_clean.py shows no quorum=0 case). Given --quorum <number-of-reachable-providers> is the exact invocation ardi.md/fully-clean.md/skills/ardi/SKILL.md now mandate, a session that (incorrectly, per the "fail closed" instruction) computes 0 reachable providers and still invokes the checker will get a false "PR is fully clean" result instead of the intended block. This directly undermines the review-gate this PR is written to strengthen.

Minor / non-blocking

3. Stray leftover comment at end of test file
scripts/test_check_pr_fully_clean.py:2376-2377, appended after if __name__ == "__main__": sys.exit(main()):

# ---
# To be appended before the `sys.exit` part

This reads as an unactioned editing note accidentally committed rather than executed — its own text says content was meant to be inserted "before the sys.exit part," but instead the note itself was left dangling after it. Dead/confusing; should be deleted.

Verdict

Needs more work

Reviewed commit: 5bda5eb

[github-actions]

💰 Cost: $4.4412 (review) — run