ums: review-only scope, exhaustive posting, math vs code by d-morrison · Pull Request #2277 · Morrison-Lab/ai-config

d-morrison · 2026-08-26T07:02:35Z

Summary

Bank review-scope lessons from UCD-SERG/shigella#31: leftover-artifact findings follow what the PR is landing; the owner's scope expansions win over the author's mechanical box; posting a review is not working the PR.
Extend the exhaustive-review-pass rule so a reviewer posts every finding already in hand (2nd occurrence).
Fact-check displayed equations against the Stan/R/JAGS claimed to implement them.

Test plan

Read the new "Reviewing someone else's PR" bullets in memories/preferences.md against the shigella#31 thread (section 5 withdrawn; leftover nits posted only after "did you post all that?").
Confirm shared/workflow/ardi.md and CLAUDE.md agree that review-only does not start ARDI.

Bank shigella#31 lessons: leftover-artifact findings follow what the PR is landing; post every finding already in hand; displayed equations must match the Stan/R/JAGS; posting a review is not working the PR. Co-authored-by: Cursor <cursoragent@cursor.com>

preferences.md was already at the 1200-line gate, so the validate job failed on the append. Move those bullets to a satellite file, register it, and break the clause-check lines. Co-authored-by: Cursor <cursoragent@cursor.com>

Local adversarial review of ee81011: the general equation-vs-runner bullet was missing a noun, and treated a shigella-specific branch as a universal checklist item. Co-authored-by: Cursor <cursoragent@cursor.com>

Local review of 92c65d5: the CLAUDE.md carve-out still ordered ARDI on, then off, then on; the satellite overclaimed the gha#412 one-pass prompt rule; the displayed-equation cases did not match their trigger. Re-dispatch after each push that moves HEAD. Co-authored-by: Cursor <cursoragent@cursor.com>

Local review of 28c0219: babysit/stop-watching sat after Review-only and re-armed the loop; the never-infected case named no equation; the post-push re-dispatch sentence fought the pre-push gate. Co-authored-by: Cursor <cursoragent@cursor.com>

d-morrison · 2026-08-26T10:19:45Z

Local adversarial review (HEAD `459127d2`)

Dispatched adversarial-reviewer after this push (per standing request to re-run after each push on this PR).

Summary of Changes

This branch banks shigella#31 review-scope lessons: a memories/reviewing-prs.md satellite (split out of preferences.md at the 1200-line gate), Driving vs Review-only carve-outs in CLAUDE.md and shared/workflow/ardi.md, a displayed-equation-vs-implementation check in shared/writing/fact-check-prose.md, and re-dispatch/SHA-fingerprint guidance in shared/workflow/adversarial-self-review.md.

Findings

No actionable findings identified.

Verdict: Ready for merge

Reviewed-Commit: 459127d

Posted by Cursor (local session) --- not written by a human.

d-morrison · 2026-08-26T10:19:47Z

Session note: re-run the local adversarial review after every push on this PR (not only after the last planned fix). Encoded in shared/workflow/adversarial-self-review.md as: do not report a HEAD as reviewed until a dispatched review of that SHA has returned.

Posted by Cursor (local session) --- not written by a human.

Resolve adversarial-self-review.md: keep main's cross-model merge gate and this branch's SHA re-dispatch rule; drop the superseded chase-cross-vendor bullet. Co-authored-by: Cursor <cursoragent@cursor.com>

d-morrison · 2026-08-26T10:47:23Z

Local adversarial review (post-merge-with-main)

Reviewed-Commit: e7936ee81bd7fd7ffe44d9fa411adbc744822bc8

Conflict resolution in shared/workflow/adversarial-self-review.md checked: main's cross-model/harness merge gate and pre-push vs merge-time Do bullets retained; this branch's SHA re-dispatch bullets retained; superseded "chase a cross-vendor reviewer" Do dropped.

No actionable findings.

Verdict: Ready for merge

Posted by Cursor (local session) --- not written by a human.

new-line-breaks flagged the label+sentence lines as packing more than one sentence; put each bold label on its own line. Co-authored-by: Cursor <cursoragent@cursor.com>

… merge Present-tense "is at the 1200-line gate" was true at split time; after merging main it is 1130. State the history and keep routing new review-scope lessons here. Co-authored-by: Cursor <cursoragent@cursor.com>

d-morrison · 2026-08-26T10:57:29Z

Local adversarial review (post-conflict + SemBr + gate wording)

Reviewed-Commit: e6124529c31b8a166a3dde0c7e097746912a138c

Prior finding on memories/reviewing-prs.md ("preferences.md is at the 1200-line gate") addressed: past tense + current 1130-line count. Conflict resolution, SemBr Driving/Review-only labels, and SHA re-dispatch checked again.

No actionable findings.

Verdict: Ready for merge

Posted by Cursor (local session) --- not written by a human.

#2277 was reported Ready for merge from the GraphQL rollup; the complete instrument exited 1. Extend no-incomplete-check-enumeration and correct mistake-patterns 5c, which had recommended the short rollups. Co-authored-by: Cursor <cursoragent@cursor.com>

…ssion statusCheckRollup on #2277 matched the endpoint (8==8); the false Ready-for-merge claim failed on missing automated review. Keep matching it as a partial reading without claiming it omits check runs the way gh pr checks can. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

…ia too Co-authored-by: Cursor <cursoragent@cursor.com>

Reaffirm autonomous push/PR delivery; Cursor Auto-review blocks are not a stop. Co-authored-by: Cursor <cursoragent@cursor.com>

d-morrison · 2026-08-26T16:41:22Z

Pushed the incomplete-check-enumeration UMS (statusCheckRollup as a partial reading; Pattern 5c corrected; check-runs qualified as the check half). Also banked standing "always push and PR" --- do not leave commits ahead of origin or close with an offer to push.

Local adversarial review on 4256dfa1 was Ready for merge; tip after this memorize commit needs its own pass on the next push cycle.

Posted by Cursor (local session) --- not written by a human.

Paginated check-runs stays the check half; SemBr the always-push Do bullets; note statusCheckRollup in fully-clean.md. Co-authored-by: Cursor <cursoragent@cursor.com>

d-morrison · 2026-08-26T16:45:05Z

Addressed local adversarial findings on a52d94c6: SemBr on the always-push Do; narrowed RX_COMPLETE so only check-pr-fully-clean.py clears a terminal fully-clean / ready-to-merge claim (paginated check-runs / get_check_runs no longer count as complete for that); noted statusCheckRollup in fully-clean.md.

Posted by Cursor (local session) --- not written by a human.

… claims Co-authored-by: Cursor <cursoragent@cursor.com>

…nal claims Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

Resolve MEMORY.md: keep reviewing-prs.md index row and main's gh-cli split. Co-authored-by: Cursor <cursoragent@cursor.com>

d-morrison · 2026-08-26T21:26:39Z

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

Posted by Cursor (local session) --- not written by a human.

d-morrison · 2026-08-26T21:26:40Z

@jules review

Posted by Cursor (local session) --- not written by a human.

github-actions · 2026-08-26T21:26:53Z

🤖 Jules Review

Summary

This PR tightens the requirements for a PR to be considered "fully clean" by explicitly blocking the use of short/partial check lists like gh pr checks and statusCheckRollup for terminal claims. It also clarifies the distinction between a "review-only" pass and "driving" a PR, updating agent instructions appropriately.

Strengths

Real-world incidents (e.g., ai-config#2277, shigella#31) are thoroughly documented to provide clear rationale for the updated rules.
The Python hook script properly extends its regex checks to catch the new statusCheckRollup edge case, supported by appropriate unit tests.
Splitting reviewing-prs.md out of preferences.md is a good maintainability move to avoid breaching the 1200-line limit for context.

Findings

[NIT]

hooks/test-no-incomplete-check-enumeration.py, line 52: The test case ([PARTIAL_ROLLUP, CHECKER, say("#2277 is fully clean.")], False, ...) combines PARTIAL_ROLLUP (which queries PR 2277) with CHECKER (which uses PR 651). The test passes because the RX_COMPLETE regex doesn't enforce PR number consistency, but the fixture is logically inconsistent. Consider defining a CHECKER_2277 fixture.

VERDICT: comment

Session: 15904558555921558150

Close hook bypass when a terminal clean claim rests only on paginated check-runs or MCP get_check_runs without check-pr-fully-clean.py. Co-authored-by: Cursor <cursoragent@cursor.com>

d-morrison · 2026-08-26T21:28:53Z

Claude finished reviewing this PR (self-review fallback --- @claude review skipped for API credential/quota).

Summary

Reviewed the UMS scope split from shigella#31 plus the follow-on fully-clean instrumentation (no-incomplete-check-enumeration, check-pr-fully-clean.py-only terminal claims, always-push-and-PR memory).

Findings addressed in `c6a7d99`

Hook bypass (fixed) --- get_check_runs and paginated commits/<sha>/check-runs were demoted from RX_COMPLETE but not listed in RX_PARTIAL, so check-runs-only terminal claims bypassed the Stop hook. Added both to RX_PARTIAL with two new test cases (19/19 pass).

Remaining (non-blocking)

Pattern 4 in mistake-patterns.md still cites unconditional "Watch and ARDI every PR" without the driving/review-only qualifier --- cosmetic drift from this PR's scope split.

Verdict: Ready for merge

Reviewed-Commit: c6a7d99

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

github-actions · 2026-08-26T21:29:07Z

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

…e-out Jules review nit: the #2277 fully-clean test case reused the #651 CHECKER fixture. Added a CHECKER_2277 fixture matching the PR/repo under test. Self-review's own remaining finding: Pattern 4 in mistake-patterns.md cited "Watch and ARDI every PR" unconditionally; note the review-only carve-out this PR adds via memories/reviewing-prs.md.

github-actions · 2026-08-26T21:31:42Z

No late @claude requests to absorb. Both findings from the self-review's "Remaining (non-blocking)" note and Jules's NIT are now addressed in commit 106f8c67, staged for the post-step to push.

Stopping Point: Not a clean stopping point — PR #2277 is still open and unmerged; the post-step will push this commit and the driving session should continue monitoring for the next review round.

_{— posted by @claude post-step from workflow run}

💰 Cost: $2.212812

Bot push left validate/claude-review at action_required with no jobs; empty commit from driving session to re-run CI on 106f8c6 content. Co-authored-by: Cursor <cursoragent@cursor.com>

Split multi-sentence line flagged by new-line-breaks CI on afdb7d3. Co-authored-by: Cursor <cursoragent@cursor.com>

When concurrency cancel-in-progress leaves a cancelled run beside a later success with the same job name on one SHA, treat the head clean. Co-authored-by: Cursor <cursoragent@cursor.com>

Only ignore a cancelled check run when a success with the same job name comes from the same workflow file, not a coincidental name collision across workflows (#1869). Co-authored-by: Cursor <cursoragent@cursor.com>

github-actions · 2026-08-26T22:17:46Z

Claude finished review — View run

CI is still running (that's this review run itself plus validate). Everything relevant passes; nothing failing. The fix in the latest commit fully addresses the prior should-fix finding, and I found no new issues in the delta or elsewhere in the diff.

Code review — #2277

Delta reviewed. The last review round (Reviewed commit: 89ebaaba54c26c234da59ee2c31bf3bf7744309f) gave one should-fix finding: the cancelled-check suppression added in 89ebaaba matched purely on check-run name, which this repo's own history (ai-config#1869) shows is not unique across workflows — risking a false-clean verdict if an unrelated workflow's job shares a name with a genuinely-cancelled one. One commit has landed since: 52ea76b0d4c11cdea55db5e2e4e3ed3fe5981b05 ("fix(check-pr-fully-clean): scope cancelled suppression by workflow path"), touching only scripts/check-pr-fully-clean.py and its test file (+38/+61 lines).

Verification performed:

git diff 89ebaaba 52ea76b0 — confirmed the fix resolves each check run's originating workflow file path via gh api repos/{repo}/actions/runs/{run_id} (parsed from the run id embedded in html_url, per-call cached in workflow_cache), and now keys suppression on (name, workflow_path) tuples instead of name alone.
Confirmed the fix correctly implements suggested fix (a) from the prior finding — resolving the originating workflow via the run id in html_url — exactly as I'd suggested, rather than only adding a caveat comment.
Traced the fail-safe direction: if _workflow_path_from_check_run can't resolve a path (missing/malformed html_url, or a gh api failure caught as RuntimeError → ""), the cancelled run is not suppressed and is reported as an issue — the same safe-by-default behavior the rest of this function already follows for duplicated names.
Ran python3 scripts/test_check_pr_fully_clean.py at this head (via a temporary worktree): 177 passed, 0 failed, including the new "cancelled run is not ignored when success is from a different workflow" case, which mocks the exact R-CMD-check.yaml / check-readme.yaml collision scenario from the prior finding and asserts not collision_ok.
Confirmed the delta since the last fully-clean round (7a402684) is scoped to exactly these two files (git diff 7a402684 52ea76b0 --stat) — nothing else changed.
gh pr view: state=OPEN, mergeable=MERGEABLE, head is 52ea76b0….
CI: no failing checks at this head; the only non-passing rows are pending (this review run itself, and a validate job still in flight at read time).

No new issues found. The prior round's sole should-fix finding is resolved; everything else in the PR remains clean per the earlier rounds' verdicts (#2277 (comment), #2277 (comment)).

Verdict

Ready for merge

Reviewed commit: 52ea76b

github-actions · 2026-08-26T22:17:46Z

💰 Cost: $1.3561 (review) — run