Skip to content

UMS from #2913: Copilot re-request tell, suppressed findings, one-push restatements - #2966

Merged
d-morrison merged 15 commits into
mainfrom
claude/ums-2913-review-rounds
Sep 2, 2026
Merged

UMS from #2913: Copilot re-request tell, suppressed findings, one-push restatements#2966
d-morrison merged 15 commits into
mainfrom
claude/ums-2913-review-rounds

Conversation

@d-morrison

@d-morrison d-morrison commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

What

Three learnings from driving #2913 through thirty-seven Copilot rounds, recorded per CLAUDE.md's "Wrap up a merged PR with UMS" and run-ums-proactively.md.

memories/github-mcp-tools.md (two bullets in the Copilot section):

  • Copilot's per-push review is not guaranteed and its absence is silent. Measured on reviewing-prs, ardia, gia: only work PRs the user opened, is assigned to, or asked for, or the Actions app authored #2913 (2026-09-01 PT, UTC clock) from public, re-runnable sources: commit times from git log, per-SHA commits/<sha>/check-runs, the PR timeline's review_requested events, and get_reviews. Every Copilot run on the four measured heads followed a request by twelve to sixteen seconds; none started on its own in the time it was given, and the two heads that waited went seven and two minutes without one. The tell is get_check_runs; one minute is the operational heuristic, not a guarantee. The Do bullet applies to ready-for-review heads, keeps pr-on-claim.md's no-re-POST rule while a run exists, and requests only when none appeared, recording that sequence as observed rather than as proven cause.
  • A review reporting Comments generated: 0 new can still carry findings under Suppressed comments in the get_reviews body, invisible to get_review_comments; a success check run plus zero open threads is not a clean round. Page through every review page, then select the entry by user.login and commit_id at the current head. Cross-links fully-clean.cases.md's collapsed-block case, which is the same shape from the gh side.

memories/reviewing-prs.md (new section, "Inspect every restatement of a rule in one pass"): when a finding adds an arm, a veto, or a recheck to one restatement of a rule, find every restatement by three routes (phrase grep, mechanism-term grep, and a topic-and-filename pass over every repository surface that lists PRs or mutates PR state), read each candidate against the widened rule, and change only the ones that carry the gap. #2913 spent rounds thirty-two through thirty-five landing one exclusion veto and one pre-write recheck site by site (files per commit from git show --stat); round thirty-six was a separate live-claim gap in check-history, recorded as such rather than as an instance. The section carries the measured grep against the 28c20e5 tree and its hit list. A "before each write" sentence has two populations to derive from the file rather than recall: the procedure's mutating commands and the predicate's mutable inputs. Pattern and anti-pattern stated as a pair.

shared/workflow/pr-on-claim.md, memories/gh-cli.md, skills/pr-on-claim/SKILL.md: the three restatements of "do not re-request Copilot on an auto-requesting repo" are scoped to "while a check run exists on the head", with the no-run case on a ready-for-review head pointed at the measurement above, so the corpus gives one instruction rather than two.

memories/MEMORY.md: the reviewing-prs.md row names the new section.

Verification

markdownlint-cli2@0.23.0, test_check_memory_file_size.py (the CI test, since the standalone checker is advisory; github-mcp-tools.md is at exactly 1250 lines), validate-skills.py, check-links.py, new-line-breaks (run after each commit), and check-ascii-punctuation.py --diff --base origin/main all pass on the pushed head. Follow-ups: #2969 (split the memory file) and #2970 (advisory checker versus blocking test).

Merge order

Disjoint from every other open PR of mine: none of the six files is edited by another open PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F

…h restatements

Three learnings from driving #2913 through thirty-seven Copilot rounds.
Copilot's per-push review is not guaranteed and its absence is silent,
so confirm the check run on the new head and re-request when missing.
A review reporting zero new comments can still carry findings under
"Suppressed comments" in the get_reviews body, invisible to
get_review_comments. And a finding that adds an arm, a veto, or a
recheck to one restatement of a rule is open at every restatement, so
grep and land them all in one push.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new guidance overstates the review history and presents a limited timing observation as a universal guarantee.

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

Pull request overview

Records lessons from PR review orchestration and propagates them through the memory index.

Changes:

  • Documents Copilot review timing and suppressed findings.
  • Adds guidance for synchronizing rule restatements.
  • Updates the memory catalog.

Reviewed at 2026-09-01 21:09 PDT.

File summaries
File Description
memories/reviewing-prs.md Adds rule-propagation guidance.
memories/MEMORY.md Updates the memory index.
memories/github-mcp-tools.md Records Copilot review behaviors.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread memories/MEMORY.md Outdated
Comment thread memories/github-mcp-tools.md Outdated
Comment thread memories/reviewing-prs.md Outdated
…a heuristic

Copilot round one on #2966. The restatement lesson now says to read
each grep hit against the widened rule and change only the ones that
carry the gap, cites address-every-comment's broadening-fix section,
limits the example to rounds thirty-two through thirty-five, and records
round thirty-six as a different gap. The Copilot timing entry labels
the one-minute window a heuristic from the measured pushes, not a
guarantee. The index row follows the corrected section.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The case history omits changed sites, and review selection needs reviewer and head filtering.

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

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

memories/github-mcp-tools.md:365

  • “Newest review” is not sufficient on a PR with multiple reviewers or reviews from older heads: it can select a human review or a stale Copilot round. The existing contract in shared/workflow/review-verdict-pitfalls.md:303-315,328-329 requires filtering get_reviews by Copilot's user.login and checking commit_id against the current head. Include those filters here so this operational instruction cannot miss the suppressed block on the round being assessed.
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread memories/reviewing-prs.md Outdated
…in and head

Copilot round two on #2966. The case history now names every file each
cited commit changed, read from git show --stat, including this file in
3b32086 and chores in 988b545 and ab89045. The suppressed-findings
entry now selects the Copilot review at the current head by user.login
and commit_id rather than taking the newest entry, per
review-verdict-pitfalls.md.

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

Copy link
Copy Markdown
Collaborator Author

ARD for Copilot round two (on e5f0d81)

Both Addressed in 44b4dd1.

  • memories/reviewing-prs.md:131 (inline thread, case history omitted changed sites): every file per cited commit is now listed from git show --stat; reply and resolution on the thread.
  • memories/github-mcp-tools.md:365 (suppressed finding, no thread: "newest review" is not sufficient): the Do bullet now selects the get_reviews entry whose user.login is copilot-pull-request-reviewer[bot] and whose commit_id is the current head, notes that the newest entry alone can be a human review or a stale round, and points at shared/workflow/review-verdict-pitfalls.md's reviewer-login table for the field and value per surface.

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The case record is incomplete and unreproducible in places, and the PR description contradicts the corrected round history.

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

Review details

Suppressed comments (1)

memories/reviewing-prs.md:140

  • The PR description still says rounds 32 through 36 all propagated the exclusion veto and pre-write recheck, while these corrected lines identify round 36 as a separate live-claim gap. Update the description to limit that sequence to rounds 32-35 and mention round 36 separately.
Round thirty-six was a different gap (a live-claim check in
`check-history`, distinct from scope), so it is not an instance of this
lesson and a restatement sweep would not have caught it.
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread memories/reviewing-prs.md Outdated
Comment thread memories/reviewing-prs.md Outdated
…file list

Copilot round three on #2966. The counterfactual is now a measured
query against the 28c20e5 tree with its hit list, which reaches every
file rounds thirty-three through thirty-five changed, and names the one
wrapped restatement a single-line grep lands beside. The 28c20e5 file
list adds scripts/validate-skills.py.

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

Copy link
Copy Markdown
Collaborator Author

ARD for Copilot round three (on 44b4dd1)

All three Addressed.

  • memories/reviewing-prs.md:137 (counterfactual not reproducible): f8fc949 records the measured git grep against 28c20e5 and its hit list; reply and resolution on the thread.
  • memories/reviewing-prs.md:131 (28c20e5 file list omitted scripts/validate-skills.py): added in f8fc949; reply and resolution on the thread.
  • Suppressed, memories/reviewing-prs.md:140 (PR description still said rounds thirty-two through thirty-six): the description now limits the sequence to rounds thirty-two through thirty-five, records round thirty-six as a separate live-claim gap, and reflects the login-and-head selection and the measured grep.

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


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Newly added PR references violate the repository requirement for clickable forge links.

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

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

memories/MEMORY.md:16

  • Link the ai-config#2913 reference in this documentation index, as required by the repository's PR/issue-link rule in AGENTS.md:393-398.
    memories/github-mcp-tools.md:338
  • Link this PR reference. AGENTS.md:393-398 requires clickable forge URLs for PR and issue mentions in documentation.

This issue also appears on line 360 of the same file.

memories/github-mcp-tools.md:362

  • Link both PR references. Bare #2913 and #1029 conflict with the documentation rule in AGENTS.md:393-398 that PR and issue mentions use clickable forge URLs.
  Rounds thirty-five and thirty-six on `#2913` each carried two such findings.
  The same shape from the `gh` side is `fully-clean.cases.md`'s
  collapsed-block case (`#1029`).
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread memories/reviewing-prs.md Outdated
Copilot round four on #2966: AGENTS.md's link rule wants every PR or
issue mention in documentation as a clickable forge URL. Linked all five
references the diff added, swept by grepping the added lines rather
than fixing the one the inline finding quoted.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
The reviewing-prs.md row this PR edits carried a pre-existing bare
UCD-SERG/shigella#31; a grep of the added lines after f6a5366 found it,
so link it under the same AGENTS.md rule rather than leave it for the
next round.

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

Copy link
Copy Markdown
Collaborator Author

ARD for Copilot round four (on f8fc949)

All four Addressed: the inline thread on memories/reviewing-prs.md:128 (reply and resolution there) and the three suppressed findings on memories/MEMORY.md:16, memories/github-mcp-tools.md:338, and memories/github-mcp-tools.md:360-362. f6a5366 links every PR reference the diff added, found by grepping the added lines for #<digits>; 09d998b links the pre-existing UCD-SERG/shigella#31 on the touched MEMORY.md row, which the same grep surfaced. A final grep of the added lines for a #<digits> outside link text returns nothing.

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


Generated by Claude Code

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

Follow-ups filed for the validate failure above: #2969 (split memories/github-mcp-tools.md's Copilot section into a satellite; the file sits at 1248 of 1250 lines) and #2970 (the standalone size checker is advisory while validate's test blocks, so a local gate passes files CI rejects).

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


Generated by Claude Code

…eview time

Copilot round ten and the automated review on 2f7d9f5. The re-request
sentences in github-mcp-tools.md and pr-on-claim.md now state the
observed sequence (minutes with no run, then a run within seconds of the
request) and say it is not proof of cause. The topic pass covers every
repository surface that lists or mutates PRs, root manuals and scripts
included. The 28c20e5 review time is 03:23:12, per GET pulls/2913/reviews.

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

Copy link
Copy Markdown
Collaborator Author

ARD for Copilot round ten and the automated review, both on 2f7d9f5

All Addressed in af9856c (on top of 75de074, which fixed the validate failure).

  • Copilot, memories/github-mcp-tools.md:373, suppressed (sequence is not cause): the Do bullet now records that on the two heads that waited, minutes with no run were followed by a run within seconds of the request, and says in the same sentence that this is the observed sequence rather than proof of cause and that a merely delayed run would make the request a harmless duplicate.
  • Copilot, shared/workflow/pr-on-claim.md:111 (same overstatement): the same wording there; reply and resolution on the thread.
  • Copilot, memories/reviewing-prs.md:189 (topic pass omits root manuals and scripts): now every repository surface that lists or mutates PRs, root manuals, hooks, and scripts named; reply and resolution on the thread.
  • Automated review (28c20e5 review time is 03:23:12Z, not 03:23:02): corrected, after re-reading GET pulls/2913/reviews (review 5085256358, submitted_at 03:23:12Z). The other three review times re-checked the same way and unchanged.

memories/github-mcp-tools.md is at 1250 lines exactly on af9856c; scripts/test_check_memory_file_size.py passes on it, and #2969 holds the split.

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


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new procedures can omit corpus files and unread same-head Copilot reviews.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

memories/github-mcp-tools.md:387

  • Filtering by reviewer and head does not identify a unique review: multiple Copilot rounds can target the same unchanged head after a rebuttal or duplicate request. If a monitoring gap spans a finding-bearing review and a later clean review, selecting an arbitrary or latest match can skip the earlier suppressed findings, contrary to CLAUDE.md:579-583's requirement to process every unread round. Require processing every unhandled matching review in submission order.
    memories/reviewing-prs.md:189
  • The fallback search still omits restatements outside skills, fragments, and memories. AGENTS.md:561-586 is itself canonical, while scripts/pr-sweep.py:40-44 and scripts/pr-overlap.py:85-89 also restate the scope rule; if those files use different vocabulary, neither grep is guaranteed to find them, recreating the one-site-per-round failure this section is meant to prevent. Make the topic-and-filename pass corpus-wide rather than limiting it to three file classes.
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new guidance conflicts with existing restatements and incorrectly requests reviews for draft PR pushes.

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

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread memories/github-mcp-tools.md Outdated
Comment thread shared/workflow/pr-on-claim.md Outdated
…statements

Copilot round eleven on #2966. The per-push instruction now applies to
PRs that are ready for review, since draft pushes defer review per
hooks/no-unreviewed-pr.py. memories/gh-cli.md and skills/pr-on-claim
carried the older "never call the endpoint on an auto-requesting repo"
wording; both now use the check-run discriminator and point at the
per-push measurement. github-mcp-tools.md stays at 1250 lines.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The check-run existence rule blocks required no-push re-reviews and includes an inaccurate cost rationale.

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

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

memories/gh-cli.md:559

  • This restatement has the same permanent-veto problem: a completed check continues to exist on an unchanged head. It therefore conflicts with skills/ardi/SKILL.md:205-208, which requires re-requesting after a Rebut/Defer-only round with no push. Limit this instruction to an active run and defer completed rounds to the ARDI disposition.
    memories/github-mcp-tools.md:367
  • Where one exists also includes an old completed run, which remains attached when a Rebut/Defer-only round leaves the head unchanged. The blanket no-repost instruction then contradicts skills/ardi/SKILL.md:205-208 and prevents the required confirming review. Scope the veto to queued/in-progress runs and explicitly preserve the no-push re-request path.
    shared/workflow/pr-on-claim.md:111
  • Using exists as the veto makes it permanent for an unchanged head: completed check runs remain attached to the commit. After a Rebut/Defer-only ARDI round, no push changes the SHA, yet skills/ardi/SKILL.md:205-208 requires a fresh request. Distinguish an active run from the historical completed run so the confirming review is not blocked.
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread skills/pr-on-claim/SKILL.md Outdated
…e-request

A completed copilot-pull-request-reviewer run stays attached to an
unchanged head, so "while a check run exists" made the no-re-POST rule a
permanent veto and contradicted skills/ardi/SKILL.md's requirement to
re-request after a Rebut/Defer-only round with no push.
Scope the veto to a queued or in-progress run in every restatement
(memories/gh-cli.md, memories/github-mcp-tools.md,
shared/workflow/pr-on-claim.md, skills/pr-on-claim/SKILL.md) and name the
no-push round as the second request case.

Describe a duplicate request as one spent call, the accepted risk, rather
than as cost-free, since memories/gh-cli.md records that probing spends
per-user quota; the same wording lands in pr-on-claim.rationale.md, which
carried the same "costs nothing" claim.

Work every Copilot review matching login and head in submission order,
not "the entry", since several can match one head.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017trXKZFoCYC4kP7NKNnN6F
@github-actions

github-actions Bot commented Sep 2, 2026

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

2026-09-02 05:31 UTC

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The guidance is internally consistent, evidence-backed, and addresses the prior review findings.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


All checks pass; only the in-progress build, two validate (dupe workflow), and this review run remain, none failed.

Review

This round covers git log 3321ec2a..c5cb8d1, which includes several main-branch commits pulled in by the merge (c71c5d4, e046c40, 60c4ec2 — already reviewed/merged separately) plus the one commit new to this PR branch: e479cd8, "Copilot re-request: veto only an active check run; keep the no-push re-request." That commit touches five memory/skill files: memories/gh-cli.md, memories/github-mcp-tools.md, shared/workflow/pr-on-claim.md, shared/workflow/pr-on-claim.rationale.md, skills/pr-on-claim/SKILL.md.

What the commit does

It fixes a real logical bug the prior round's own content introduced: the rule "don't re-POST a Copilot review request while a copilot-pull-request-reviewer check run exists on the head" is a permanent veto once any run has ever completed on that head, because a completed run stays attached to an unchanged head — this directly contradicted skills/ardi/SKILL.md's requirement to re-request after an all-Rebut/Defer round that pushes no code. The fix narrows the veto to "queued or in progress" and adds the no-push-round case as a second trigger for a fresh request, applied consistently across all five restated sites.

Verification performed

  • Consistency sweep across all five restatement sites — confirmed each of memories/gh-cli.md, memories/github-mcp-tools.md, shared/workflow/pr-on-claim.md, shared/workflow/pr-on-claim.rationale.md, skills/pr-on-claim/SKILL.md now uses "queued or in progress" (not bare "exists") and "accepted risk"/"spends one call" (not "costs nothing"/"harmless duplicate") — no leftover contradictory phrasing found (git grep for check run exists, harmless, costs nothing turned up nothing related to this rule left unfixed elsewhere in the corpus).
  • skills/ardi/SKILL.md actually supports the claim cited — the "No code pushed (all Rebut/Defer)" bullet (skills/ardi/SKILL.md:205-207) does require explicit re-request ("post @claude review, or the forge's equivalent") when no push occurred, which is a reasonable basis for the new text's claim that a no-push round needs a fresh Copilot request too.
  • No unfixed sibling restatement found — checked shared/workflow/review-verdict-pitfalls.md's "Keep re-requesting each round anyway" bullet (a different scenario: quota-exhausted refusal, not the queued/in-progress timing case) and confirmed it doesn't need the same fix.
  • Line-count regression from the prior round's near-miss is resolved, not introduced: memories/github-mcp-tools.md is now exactly 1250 lines (was 1254 last round, flagged as crossing the 1250 advisory threshold); this round's edits shrank the file back to the threshold, not over it.

Mechanical checks

  • scripts/check-links.py: 3198 relative links across 665 files, none broken.
  • scripts/check-ascii-punctuation.py --diff --base 2f7d9f57: clean (683 added lines checked across the cumulative diff).
  • scripts/check-memory-file-size.py: passes, no file over 1250 lines.
  • CI's own lint-markdown job succeeded on both validate runs for this head.
  • PR state confirmed live: state=OPEN, mergeable=MERGEABLE, head SHA e479cd854307bd3193721ce0a7e57e33fecec167 matches this checkout's non-merge parent. mergeStateStatus=BLOCKED reflects only the pending review-approval gate; no check has failed.

No new findings

This round's diff is a correctness fix to memory content from the prior round, not new content with new risk. No fabricated links, commits, or citations. No CLAUDE.md violations. No prose ambiguity issues.

Verdict

Ready for merge

Structured Review Data (JSON)
{
  "schema_version": "1.0",
  "reviewer": "claude",
  "commit_sha": "e479cd854307bd3193721ce0a7e57e33fecec167",
  "verdict": "CLEAN",
  "findings": []
}

Confirmed via the authoritative instrument: exit 1, five in-progress bullets (copilot-pull-request-reviewer, this review / claude-review run, two validate jobs, and this review not yet posted) — no failures, all in-progress/self-referential per the task's Verdict semantics section. My content verdict stands unchanged.

Review

This round covers git log 3321ec2a..c5cb8d1: several main-branch commits pulled in by the merge (already reviewed/merged separately as #2972, #2971, #2967) plus the one commit new to this PR branch, e479cd8, "Copilot re-request: veto only an active check run; keep the no-push re-request." It touches five memory/skill files: memories/gh-cli.md, memories/github-mcp-tools.md, shared/workflow/pr-on-claim.md, shared/workflow/pr-on-claim.rationale.md, skills/pr-on-claim/SKILL.md.

What the commit does

It fixes a real logical bug from the prior round's own content: "don't re-POST while a copilot-pull-request-reviewer check run exists on the head" is a permanent veto once any run has ever completed on that head (a completed run stays attached to an unchanged head), contradicting skills/ardi/SKILL.md's requirement to re-request after an all-Rebut/Defer round that pushes no code. The fix narrows the veto to "queued or in progress" and adds the no-push round as a second re-request trigger, applied across all five restated sites.

Verification performed

  • Consistency sweep across all five restatement sites: each of the five files now uses "queued or in progress" (not bare "exists") and "accepted risk"/"spends one call" (not "costs nothing"/"harmless duplicate"). git grep found no unfixed leftover instance of the old phrasing tied to this rule elsewhere in the corpus.
  • skills/ardi/SKILL.md supports the cited claim: its "No code pushed (all Rebut/Defer)" bullet (lines 205-207) does require explicit re-request ("post @claude review, or the forge's equivalent") when no push occurred — a reasonable basis for extending that requirement to Copilot's re-request.
  • No unfixed sibling restatement found: shared/workflow/review-verdict-pitfalls.md's "Keep re-requesting each round anyway" bullet covers a different scenario (quota-exhausted refusal), not the queued/in-progress timing case, so it correctly needed no change.
  • Prior round's line-count near-miss is resolved, not worsened: memories/github-mcp-tools.md is now exactly 1250 lines (was 1254 last round); this round's edits shrank the file back to the threshold.

Mechanical checks

  • scripts/check-links.py: 3198 relative links across 665 files, none broken.
  • scripts/check-ascii-punctuation.py --diff --base 2f7d9f57: clean.
  • scripts/check-memory-file-size.py: passes, no file over 1250 lines.
  • CI's lint-markdown job succeeded on both validate runs for this head; no check has failed.
  • scripts/check-pr-fully-clean.py 2966 -R Morrison-Lab/ai-config: exit 1, five bullets, all in-progress or self-referential (this review run not yet posted) — no failure among them.

No new findings

This round's diff is a correctness fix to the prior round's own memory content, not new content with new risk. No fabricated links, commits, or citations; no CLAUDE.md violations; no prose ambiguity.

Verdict

Ready for merge

My judgment on the diff's content is CLEAN, per this task's explicit Verdict-semantics instruction that a verdict must not be conditioned on this review's own in-flight sibling checks or on the fully-clean instrument's self-referential exit status. The instrument run above is reported for transparency, not as the basis for the verdict: none of its five bullets is a failed check or a content defect — they are copilot-pull-request-reviewer (in progress), this review / claude-review run (in progress — it is the run producing this comment), two validate jobs (in progress), and "no review posted for this head yet" (also this run). Actual merge readiness still depends on those in-progress checks finishing green, which is the merge gate's business, not this content verdict's.

Structured Review Data (JSON)
{
  "schema_version": "1.0",
  "reviewer": "claude",
  "commit_sha": "e479cd854307bd3193721ce0a7e57e33fecec167",
  "verdict": "CLEAN",
  "findings": []
}

Stopping Point: Not a clean stopping point / work remains queued: this PR's copilot-pull-request-reviewer, review / claude-review (this run), build, and two validate jobs are still in progress as of this writing; the PR remains open, unmerged, and under standing ARDI/PR-watch monitoring. No further action is required from this review turn itself — this is a review-only run and does not drive or merge the PR.

Review

This round covers git log 3321ec2a..c5cb8d1: several main-branch commits pulled in via merge (already reviewed separately as #2972, #2971, #2967) plus the one commit new to this PR branch, e479cd8, "Copilot re-request: veto only an active check run; keep the no-push re-request." It touches five files: memories/gh-cli.md, memories/github-mcp-tools.md, shared/workflow/pr-on-claim.md, shared/workflow/pr-on-claim.rationale.md, skills/pr-on-claim/SKILL.md.

What the commit does

It fixes a real logical bug in the prior round's own content: "don't re-POST while a copilot-pull-request-reviewer check run exists on the head" is a permanent veto once any run has ever completed on that head, because a completed run stays attached to an unchanged head — contradicting skills/ardi/SKILL.md's requirement to re-request after an all-Rebut/Defer round that pushes no code (see its "No code pushed (all Rebut/Defer)" bullet at lines 205-207, which does require an explicit re-request). The fix narrows the veto to "queued or in progress" and adds the no-push round as a second re-request trigger, applied across all five restated sites.

Verification performed

  • Consistency sweep across all five restatement sites: each now says "queued or in progress" (not bare "exists") and "accepted risk"/"spends one call" (not "costs nothing"/"harmless duplicate"). git grep found no leftover instance of the old phrasing tied to this rule elsewhere in the corpus.
  • No unfixed sibling restatement: shared/workflow/review-verdict-pitfalls.md's "Keep re-requesting each round anyway" bullet covers a different scenario (quota-exhausted refusal), so it correctly needed no change.
  • Prior round's line-count near-miss resolved: memories/github-mcp-tools.md is now exactly 1250 lines (was 1254 last round).

Mechanical checks

  • scripts/check-links.py: 3198 relative links across 665 files, 0 broken.
  • scripts/check-ascii-punctuation.py --diff --base 2f7d9f57: 0 issues.
  • scripts/check-memory-file-size.py: 0 files over the 1250-line threshold.
  • scripts/check-pr-fully-clean.py 2966 -R Morrison-Lab/ai-config: exit 1. Latest output lists three open items — copilot-pull-request-reviewer in progress, this review / claude-review run in progress, and "no review posted for HEAD e479cd8 yet" (also this run). None is a failed check or a content defect; the two validate and build jobs mentioned in the prior instrument run have since completed and dropped off the list.

Findings

None. No fabricated links, commits, or citations. No CLAUDE.md violations. No prose ambiguity.

Verdict

Ready for merge

This is a content-only judgment on the diff, per this task's Verdict-semantics instruction: a verdict must not be conditioned on this same review run's own in-flight status or on check-pr-fully-clean.py's self-referential exit code (it cannot read 0 while the review producing this comment is still executing). Whether the PR can actually merge depends on the remaining in-progress checks finishing, which is a separate, later fact this review cannot observe from inside its own run.

Structured Review Data (JSON)
{
  "schema_version": "1.0",
  "reviewer": "claude",
  "commit_sha": "e479cd854307bd3193721ce0a7e57e33fecec167",
  "verdict": "CLEAN",
  "findings": []
}

Stopping Point: Not a clean stopping point / work remains queued: copilot-pull-request-reviewer and this review run are still in progress on the PR as of this writing; the PR remains open and unmerged under standing ARDI/PR-watch monitoring by whichever session is driving it. This turn is review-only and does not drive, push to, or merge the PR.

Reviewed commit: e479cd8

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

💰 Cost: $1.4077 (review) — run

@d-morrison
d-morrison merged commit 66b9e3f into main Sep 2, 2026
20 checks passed
@d-morrison
d-morrison deleted the claude/ums-2913-review-rounds branch September 2, 2026 05:31
d-morrison added a commit to Morrison-Lab/gha that referenced this pull request Sep 2, 2026
…closes #805)

gha#710 widened the posted text to the span from the first verdict-bearing
block to the last. A reviewer that redrafts its final message, each draft a
complete review with its own verdict heading, then gets every draft
concatenated (three reviews, three verdicts, two Stopping-Point lines on
Morrison-Lab/ai-config#2966).

A complete draft carries a verdict HEADING; the gha#710 follow-up tail only
writes a verdict line or the word in prose. So when more than one block
carries a heading, the span starts at the last such block and still runs
to the last verdict-bearing block. One heading is unchanged.

verdict-redrafted-thrice.json pins it (must-contain the third draft,
must-not-contain the first); assert_pass holds every posted review to at
most one verdict heading. Reverting to the span rule turns the fixture red.

Stacked on #807 (fix/804-quota-notice-reason).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
d-morrison added a commit to Morrison-Lab/gha that referenced this pull request Sep 2, 2026
…closes #805)

gha#710 widened the posted text to the span from the first verdict-bearing
block to the last. A reviewer that redrafts its final message, each draft a
complete review with its own verdict heading, then gets every draft
concatenated (three reviews, three verdicts, two Stopping-Point lines on
Morrison-Lab/ai-config#2966).

A complete draft carries a verdict HEADING; the gha#710 follow-up tail only
writes a verdict line or the word in prose. So when more than one block
carries a heading, the span starts at the last such block and still runs
to the last verdict-bearing block. One heading is unchanged.

verdict-redrafted-thrice.json pins it (must-contain the third draft,
must-not-contain the first); assert_pass holds every posted review to at
most one verdict heading. Reverting to the span rule turns the fixture red.

Stacked on #807 (fix/804-quota-notice-reason).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
d-morrison added a commit to Morrison-Lab/gha that referenced this pull request Sep 2, 2026
…closes #805) (#808)

* fix(claude-code-review): post only the reviewer's last complete draft (closes #805)

gha#710 widened the posted text to the span from the first verdict-bearing
block to the last. A reviewer that redrafts its final message, each draft a
complete review with its own verdict heading, then gets every draft
concatenated (three reviews, three verdicts, two Stopping-Point lines on
Morrison-Lab/ai-config#2966).

A complete draft carries a verdict HEADING; the gha#710 follow-up tail only
writes a verdict line or the word in prose. So when more than one block
carries a heading, the span starts at the last such block and still runs
to the last verdict-bearing block. One heading is unchanged.

verdict-redrafted-thrice.json pins it (must-contain the third draft,
must-not-contain the first); assert_pass holds every posted review to at
most one verdict heading. Reverting to the span rule turns the fixture red.

Stacked on #807 (fix/804-quota-notice-reason).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(claude-code-review): count only authored verdict headings (review round 1)

A heading inside a fenced code block or a blockquote is quoted, not
authored. With the bare regex a later block that merely showed the heading
shape, or blockquoted the previous verdict, read as a fresh draft, and the
span then started there and dropped the entire real review -- the failure
gha#710 exists to prevent. Both the jq detector and the bash invariant now
skip fenced and blockquoted lines (no awk interval expression, per the
mawk rule).

- verdict-then-quoted-heading.json pins that a quoted heading is not a draft.
- verdict-redrafted-thrice.json gains a tail block after the last draft and
  a second must-contain needle, so tail retention is pinned.
- Prose names the tail shape the regex actually matches (a line-start
  verdict line), not a mid-sentence mention.

Three mutations turn a named case red: disabling the multi-heading branch,
narrowing the span end to the last heading block, dropping the exclusion.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(claude-code-review): close a fence only on its own delimiter (review round 2)

A fence closes on a run of the same character at least as long as the
opener (CommonMark, as strip-non-invoking-markup.sh implements); the first
draft closed on any fence line, so a backtick fence holding a tilde line
leaked the heading after it. Both the jq detector and the awk invariant now
track the opener. An unclosed fence runs to the end of its block, which is
how GitHub renders it too, so that case is documented rather than
special-cased.

verdict-then-mismatched-fence.json pins it; closing on any delimiter turns
it red.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(claude-code-review): a fence closer carries only whitespace after its run (review round 3)

CommonMark: a closing fence line may hold nothing but whitespace after the
delimiter run; a run followed by text is fence content. Both the jq
detector and the awk invariant now require that, matching
strip-non-invoking-markup.sh. verdict-then-trailing-text-closer.json pins
it; dropping the check turns it red.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(claude-code-review): a heading is indented at most three spaces (review round 3)

CommonMark's third literal construct, the indented code block, needs no
state: a heading may be indented by at most three spaces, so a line at
four columns or a tab is never a heading, whether it is code or a lazy
paragraph continuation. Both the jq detector and the awk invariant now say
so. verdict-then-indented-heading.json pins it; widening the indent back
to any whitespace turns it red.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(claude-code-review): spaces only in fence indentation; awk word boundary (review round 4)

A tab is four columns in CommonMark, so a tab-led backtick line is indented
code rather than a fence; the jq capture and the awk now allow spaces only
in the three-column indentation, and the awk spells the jq's word boundary
as a trailing class so a "Verdicts" heading counts in neither (Copilot on
gha#808). verdict-redraft-after-tab-fence.json pins the first; a plural
heading in verdict-then-quoted-heading.json pins the second. Both mutations
turn a named case red.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(claude-code-review): one to six hashes and a space make a heading (review round 5)

CommonMark: an ATX heading has one to six hashes followed by a space or
tab; seven hashes, or hashes run into the word, are paragraph text. The jq
test and the awk invariant now both say so, the awk by run length rather
than an interval expression. verdict-then-quoted-heading.json gains both
non-heading shapes; lifting the limit in either place turns it red.

Prose corrected: verdict-then-tab-inside-fence.json pins both halves, the
extractor's silent drop of the real review and the awk's double count, not
the awk alone.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(claude-code-review): list the ninth confirmed mutation (review round 6)

The awk's trailing word-boundary class has its own confirmed red mutation
on verdict-then-quoted-heading.json and was missing from the enumeration.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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.

3 participants