Skip to content

Fail fully-clean when any reviewer's latest is not-clean - #2298

Merged
d-morrison merged 13 commits into
mainfrom
cursor/reviews-disagree-veto-mwc-b210
Aug 26, 2026
Merged

Fail fully-clean when any reviewer's latest is not-clean#2298
d-morrison merged 13 commits into
mainfrom
cursor/reviews-disagree-veto-mwc-b210

Conversation

@dem-extra1

Copy link
Copy Markdown
Collaborator

Closes #2274.

If one review is all-clear and another raises blocking issues, nits, or any other flagged items, do not merge, even with mwc active. ARD every item from every review, then request fresh reviews.

This encodes that in the existing fully-clean / mwc path, not a parallel process:

  • scripts/check-pr-fully-clean.py criterion 4 is per-reviewer. A later all-clear from a different reviewer does not supersede a standing not-clean. Claude and Antigravity share github-actions[bot], so identity is the first-line agent marker; Jules is keyed on login.
  • ## Nits / **Nits** / ## Non-blocking count as findings even when ### Verdict says Ready for merge, including at an earlier SHA.
  • skills/mwc/SKILL.md Scope Limit, shared/workflow/fully-clean.md, skills/ardi/SKILL.md, and the Strict Merge sections in AGENTS.md / CLAUDE.md state the same veto.

Same-reviewer iterate still works: a later clean from the same reviewer supersedes. A later GitHub APPROVED clears that author's own earlier CHANGES_REQUESTED (Copilot/Jules), not a sibling bot's.

Independent of open PRs that also touch some of these files (#2256, #2272, #2243, #2254); this targets main.

Open in Web Open in Cursor 

cursoragent and others added 4 commits August 26, 2026 09:28
A later all-clear from a different reviewer used to supersede an earlier
not-clean, so mwc could merge over standing findings, nits included.
Criterion 4 of check-pr-fully-clean.py is now per-reviewer; mwc's Scope
Limit and the ARDI loop require ARD of every review, then fresh reviews.

Closes #2274.

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
Unanchored **Nits** / **Non-blocking** matched inline prose and false-failed
clean reviews. An empty later APPROVED did not update the global latest, so
the Copilot CR-then-APPROVED path never reached the per-reviewer skip.

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
Main moved by #2290 and #2292 while this branch was in review.

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
A Claude body with ## Nits and no classifiable verdict was stored as a NOTE,
so a later all-clear from another reviewer still reported fully clean.

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
@github-actions

This comment has been minimized.

cursoragent and others added 2 commits August 26, 2026 09:55
validate new-line-breaks failed on four added markdown lines that packed
two sentences or rewrote a semicolon-heavy bullet. Restore the original
fully-clean bullet and put the per-reviewer rule on its own lines.

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
Dict order made an Antigravity first line that quoted Claude inherit Claude
and report fully clean. The ardi checklist and merge-it still named the
globally latest review as the fully-clean bar.

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
@cursor

cursor Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Fallback self-review after claude-review skipped (API credential or quota unavailable). This is the dispatched adversarial-reviewer report for HEAD e5ab4a915d9034e45dbd1e07a5fd7192b553c3bb. It does not satisfy mwc.

Summary of Changes

The branch makes fully-clean per-reviewer: a later all-clear from one reviewer does not clear another reviewer's standing not-clean, including nits and including under mwc. check-pr-fully-clean.py keys shared-login bots on the leftmost agent marker on the first non-empty line, treats ## Nits / ## Non-blocking as findings even beside a Ready verdict, and fails when any reviewer's latest is not-clean. Docs and the ardi / merge-it / mwc skills are updated to match. python3 scripts/test_check_pr_fully_clean.py reported 202 passed, 0 failed.

Findings

No actionable findings identified.

Verdict: Ready for merge

Reviewed-Commit: e5ab4a9

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

@cursor

cursor Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Driving this PR to clean --- please hold off until done.

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

cursoragent and others added 3 commits August 26, 2026 10:28
Bring in #2294 (no-incomplete-check-enumeration hook registration).

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
The adversarial review of the main-sync HEAD found the rationale still
describing a global last verdict, and ardi's GitHub example still taking
the last Claude-login comment. Both would re-teach the #2274 hole.
Name the unmarked shared-login identity residual in the checker docstring
and pin that an unmarked later all-clear does not clear a marked Claude
not-clean.

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
Bring in #2255 (pre-push-review / adv skill).

Co-authored-by: dem-extra1 <dem-extra1@users.noreply.github.com>
@cursor

cursor Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Fallback self-review after claude-review skipped (API credential or quota unavailable). This is the dispatched adversarial-reviewer report for HEAD 1e5786395b463c9c5a3c23dcfb8e502201ac54b7. It does not satisfy mwc.

Summary of Changes

HEAD 1e5786395b463c9c5a3c23dcfb8e502201ac54b7 is a merge of origin/main (13061c91, #2255) onto d9b2f210. The unique file set versus origin/main is the same 14 files; those 14 are byte-identical to d9b2f210. The #2255 paths match origin/main and are not in this PR's unique diff.

The unique change makes fully-clean criterion 4 per-reviewer rather than globally last: check-pr-fully-clean.py groups by _reviewer_identity (exclusive bot login first, else the leftmost known agent marker on the first non-empty line, else login), fails when any reviewer's latest is not-clean (nits included), and only lets a later GitHub APPROVED clear that same identity. Docs and mwc/ardi/merge-it follow that contract. skills/ardi/SKILL.md fetches issue comments by body marker (**Claude finished|### Verdict|Antigravity Agent Report) and emits every match, not | last.

Confirmed in this tree: per-reviewer grouping and _approval_clears; the ardi body-marker fetch (jq -s / .[][] holds on one- and two-page shapes); the unmarked shared-login residual (docstring plus test: unmarked github-actions does not clear marked Claude). python3 scripts/test_check_pr_fully_clean.py: 204 passed, 0 failed.

Findings

No actionable findings identified.

Verdict: Ready for merge

Reviewed-Commit: 1e57863

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

@cursor

cursor Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Driving this PR to clean --- please hold off until done.

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

2 similar comments
@cursor

cursor Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Driving this PR to clean --- please hold off until done.

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

@cursor

cursor Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Driving this PR to clean --- please hold off until done.

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

Copy link
Copy Markdown
Collaborator

The prior hold-off claim expired (no push or comment since 15:03 UTC, past the 2-hour convention), so taking this over: driving this PR to clean --- please hold off until done. A fresh Claude review has been dispatched at the current head.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

claude added 3 commits August 26, 2026 22:05
…dings

The review round on this PR reproduced a false positive: an empty
section phrased as "## Nits" followed by "**None.**" was flagged as a
standing finding because NOT_CLEAN_NEGATION_SUFFIX tolerated only
[:.-] between the heading and the negation word, not markdown emphasis
markers. Widen the class to [*_:.-] and add three regression cases for
the bold-wrapped forms (plain forms were already covered). Verified the
real-finding direction still flags: a bolded "**1. Rename...**" item
under the same heading is still reported.

207/207 tests pass.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016MyNR6ZrvN6M4Rr8eZbHts
The pre-push adversarial round showed the shared-prefix widening
extended the unguarded nothing/0/no-... branches to bold text, so a
bold "Nothing major, but <real finding>" was swallowed. Split the
alternation: emphasis markers are tolerated only before none/n-a (the
alternatives that are unambiguous when they open an emphasized span);
the risky branches keep their original plain-punctuation prefix. Add
the round's four counterexamples as negative-control tests alongside
the three positive cases.

211/211 tests pass; each counterexample verified by direct execution
in both directions.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016MyNR6ZrvN6M4Rr8eZbHts

Copy link
Copy Markdown
Collaborator

Pushed 33133fc; the push-triggered review will evaluate the new head.

ARD for the two rounds since taking this PR over:

# Round Finding Disposition
1 claude-review at 1e57863 NOT_CLEAN_NEGATION_SUFFIX false-positives on bold-wrapped **None.** under ## Nits/## Non-blocking, permanently recording a clean round as a standing not-clean Addressed --- emphasis markers now tolerated before the negation, with regression tests for the three bold forms.
2 pre-push adversarial round The first fix's shared-prefix widening extended the unguarded nothing/0/no ... branches to bold text, so a bold "Nothing major, but [real finding]" was swallowed --- the dangerous direction Addressed --- alternation split: emphasis tolerance only for the whole-negation alternatives (none/n/a); the risky branches keep their plain prefix. The round's four counterexamples are now negative-control tests.
3 pre-push adversarial round No negative-direction tests accompanied the first fix Addressed --- four negative controls added (211/211 passing).
4 pre-push adversarial round (root-cause note) The plain-text nothing/0/no ... branches were always unguarded --- predates this PR Deferred --- tracked as a separate checker hardening in the issue filed today: #2357.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Correction to the ARD table above: the deferred checker-hardening issue is #2359, not #2357 --- the comment was written before the issue's number was assigned.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

The adversarial round noted the illustrative jq test() lacked the "i"
flag the Python checker itself applies, so a differently-cased verdict
heading would be dropped by the suggested command while the checker
caught it.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016MyNR6ZrvN6M4Rr8eZbHts

Copy link
Copy Markdown
Collaborator

Pushed 25a2694. ARD for the adversarial round at 33133fc (verdict: Ready for merge, three non-blocking observations):

# Finding Disposition
1 Unmarked shared-login reviewers still collapse into one identity bucket (disclosed residual of the fixed bug class) Deferred --- tracked in #2362 with the reviewer's reproducer and candidate fixes; not testable until the design changes.
2 Illustrative jq marker match case-sensitive, unlike the checker itself Addressed --- "i" flag added in 25a2694.
3 Whole-history finding scan now admits any bot's headings (safe direction, but undocumented/untested widening) Deferred --- same issue #2362, second item.

Posted by Claude Code (AI agent) --- not written by a human.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


Confirmed: that's the only change since the last reviewed commit.

Summary

PR #2298 makes check-pr-fully-clean.py's criterion 4 evaluate every reviewer's latest verdict independently (via new _reviewer_identity() / _approval_clears() helpers) instead of only the globally-last comment, so a later all-clear from one reviewer no longer masks a standing not-clean from another, including under mwc. It also extends the shared finding-pattern scan to treat ## Nits / **Nits** / ## Non-blocking headings as findings even beside a "Ready for merge" verdict. Docs across AGENTS.md, CLAUDE.md, memories/preferences.md, several shared/workflow/*.md files, and skills/ardi|mwc|merge-it/SKILL.md are updated to state the same veto rule.

Findings

None. The prior review round (comment posted 2026-08-26T22:15:26Z, #2298 (comment) — reviewed commit 33133fc7434cbc2340c3c7c7be30cdecc7a90b95) verified the previously-reported bold-negation false positive was fixed and gave Ready for merge, with the full test suite passing (211/211).

Since that commit, exactly one further commit landed: 25a26942 ("fix: case-insensitive jq marker match in ardi recipe"), which changes a single line in skills/ardi/SKILL.md — adding jq's "i" (case-insensitive) flag to an illustrative test(...) command, so the documented command matches the Python checker's own re.IGNORECASE behavior for verdict-marker detection. I reproduced the fix directly:

echo '{"body":"### VERDICT clean"}' | jq -s '[.[] | select(.body | test("### Verdict"; "i"))]'   # matches
echo '{"body":"### VERDICT clean"}' | jq -s '[.[] | select(.body | test("### Verdict"))]'          # empty, without the flag

This is a correct, minimal, doc-only change with no functional/code impact — no new logic bugs, no CLAUDE.md violations, and no hallucinated references introduced.

Verdict

Ready for merge — the prior verdict (#2298 (comment), reviewed commit 33133fc7) stands; the only change since then is a verified, correct one-line doc fix with no functional impact.

Reviewed commit: 25a2694

Reviewed commit: 25a2694

@github-actions

Copy link
Copy Markdown
Contributor

💰 Cost: $1.1922 (review) — run

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.

2026-08-26 22:37 UTC

@d-morrison
d-morrison merged commit b6f7fcd into main Aug 26, 2026
11 checks passed
@d-morrison
d-morrison deleted the cursor/reviews-disagree-veto-mwc-b210 branch August 26, 2026 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

when reviews disagree, any of them can veto mwc

4 participants