fix(claude-code-review): post only the reviewer's last complete draft (closes #805) - #808
Conversation
…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>
|
Claude Code CLI (local session) is working on this — please hold off on pushing to this branch until I'm done. Posted by Claude Code (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
The new verdict-heading invariant in run-fixture-tests.sh can miscount headings inside fenced code blocks (and the gha#805 commentary should be aligned with what the regex actually matches).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the claude-code-review posting logic so that when a reviewer redrafts its final message multiple times (each draft containing its own ### Verdict heading), only the last complete draft is posted, avoiding concatenated duplicate reviews.
Changes:
- Update verdict-span extraction in
check-review-execution.shto prefer the last verdict-heading block when multiple verdict headings are present (gha#805), while preserving gha#710 behavior for split-across-blocks reviews. - Add a new execution-output fixture (
verdict-redrafted-thrice.json) and extendrun-fixture-tests.shassertions to pin the new behavior and guard against reintroducing multi-draft concatenation. - Document the gha#805 failure mode and fix rationale in
CLAUDE.md, and add a changelog fragment.
File summaries
| File | Description |
|---|---|
CLAUDE.md |
Documents the gha#805 failure mode and the “use last verdict heading” rule. |
changelog.d/post-last-complete-review-draft.fixed.md |
Records the user-facing behavior change in the changelog fragments system. |
.github/workflows/scripts/check-review-execution.sh |
Adjusts extraction logic to post only the reviewer’s last complete draft when multiple verdict headings exist. |
.github/workflows/scripts/tests/run-fixture-tests.sh |
Adds fixture expectations and an invariant asserting at most one verdict heading is posted. |
.github/workflows/scripts/tests/fixtures/verdict-redrafted-thrice.json |
New fixture simulating three complete redrafted reviews. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # gha#805, as an invariant over every posted review rather than one | ||
| # fixture: a comment carries at most ONE verdict heading. A second heading | ||
| # means two complete drafts were concatenated, whichever fixture produced | ||
| # them. The gha#710 tail writes `Verdict:` without a heading, so it does | ||
| # not count, and neither does a heading quoted inside a fenced block -- | ||
| # this is a shape check on our own extraction, not a verdict parse. | ||
| local headings | ||
| headings="$(grep -ciE '^[[:space:]>*_-]*#{1,6}[[:space:]]*verdict' "$posted_file" || true)" | ||
| if [[ "$headings" -gt 1 ]]; then |
| # HEADING form: a complete draft carries `### Verdict` (or any `#` heading | ||
| # naming it), while the gha#710 follow-up tail only writes a `Verdict:` line | ||
| # or the word in prose ("my verdict stands unchanged"). So when more than one | ||
| # block carries a verdict HEADING, the span starts at the LAST such block: the | ||
| # earlier drafts were superseded by their author, and the tail after the last |
… 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>
…iew 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>
… 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>
|
Review round on
One adjacent point raised by the local pre-push reviewer was rebutted: an unclosed fence inside a later redraft hides that draft's own heading and falls back to the gha#710 span rule. That is CommonMark's semantics and GitHub renders the same text the same way, so it is documented in the code comment and CLAUDE.md rather than special-cased. Posted by Claude Code (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
The new “one authored verdict heading” invariant in run-fixture-tests.sh uses a broader awk match than the extractor’s verdict\b rule, which can produce false failures unless aligned.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
| } | ||
| fence != "" { next } | ||
| /^[ \t]*>/ { next } | ||
| tolower($0) ~ /^[ \t]*#+[ \t]*verdict/ { n++ } |
…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>
|
Round on
Posted by Claude Code (AI agent) --- not written by a human. |
There was a problem hiding this comment.
🟡 Changes recommended
The new fence detectors treat leading tabs as valid ≤3-column indentation, which is inconsistent with CommonMark’s tab expansion and can misclassify fenced blocks/headings in edge cases.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
| def authored_heading: | ||
| ( split("\n") | ||
| | reduce .[] as $l ({fence: "", flen: 0, out: []}; | ||
| ( [ $l | capture("^[ \\t]{0,3}(?<run>`{3,}|~{3,})(?<rest>.*)$") ] | first ) as $f |
| match($0, /^[ \t]?[ \t]?[ \t]?(```+|~~~+)/) { | ||
| run = substr($0, RSTART, RLENGTH); sub(/^[ \t]+/, "", run) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…oundary (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>
|
Round on
Posted by Claude Code (AI agent) --- not written by a human. |
There was a problem hiding this comment.
🟡 Changes recommended
The new verdict-heading invariant in run-fixture-tests.sh can miscount non-CommonMark headings (7+ #) as headings, risking false test failures and diverging from the extractor’s #{1,6} logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
| # code or a lazy continuation, never a heading (gha#808 review round 3). | ||
| # The trailing class is the word boundary the jq spells verdict\b, so | ||
| # a "Verdicts" heading counts in neither (Copilot on gha#808). | ||
| tolower($0) ~ /^ ? ? ?#+[ \t]*verdict([^a-z0-9_]|$)/ { n++ } |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… (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>
|
Round on
Posted by Claude Code (AI agent) --- not written by a human. |
There was a problem hiding this comment.
🟢 Approval recommended
The extraction change is narrowly scoped, well-documented, and backed by targeted fixtures plus an invariant that guards against reintroducing multi-draft concatenation.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ound 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>
|
Round on
Posted by Claude Code (AI agent) --- not written by a human. |
|
Claude finished review — View run Confirmed — this round's entire delta is exactly this one 6-line documentation-only change, and nothing else. My review is complete. Review of #808 — round 7Range reviewed: This commit directly resolves the sole remaining finding from the prior review round (issuecomment posted 2026-09-02T10:01:21Z): the "Seven mutations" (by then already "Eight" after an intervening commit) enumeration was missing the confirmed-red mutation of dropping the awk's trailing word-boundary class ( Verification performedI didn't just read the diff — I re-ran the mutation myself against the checked-out code:
What checked out
No new findings. VerdictReady for merge. This round's fix ( Structured Review Data (JSON){
"schema_version": "1.0",
"reviewer": "claude",
"commit_sha": "fbdf043118ef90fc98f6baa4df1ee73820094626",
"verdict": "CLEAN",
"findings": []
}Reviewed commit: be8f3c2 |
There was a problem hiding this comment.
🟢 Approval recommended
The extraction change is narrowly scoped, preserves prior behavior for the split-across-blocks case, and is backed by multiple discriminating fixtures plus a cross-fixture invariant.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The extraction change is narrowly scoped, well-justified by the documented failure mode, and backed by targeted fixtures plus a global shape invariant to prevent regression.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new jq capture(...) usage in check-review-execution.sh will error on non-matching lines (breaking extraction) unless made optional (e.g., capture(...) ? / try).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
| # Spaces only in the indentation allowance: a tab is four columns in | ||
| # CommonMark, so a tab-led fence line is indented code, not a fence | ||
| # (Copilot on gha#808). | ||
| ( [ $l | capture("^ {0,3}(?<run>`{3,}|~{3,})(?<rest>.*)$") ] | first ) as $f |
Closes #805.
claude-code-review.ymlposted a review comment carrying three completedrafts, each with its own verdict heading and structured-review-data block
(Morrison-Lab/ai-config#2966, run 33594599768).
The cause is gha#710's span rule: when more than one assistant block carries
a verdict line, the posted text runs from the first such block to the last,
which is right for a review split across blocks and wrong for a reviewer that
redrafts its whole final message.
What changes
writes a plain verdict line or the word in prose.
When more than one block carries a heading, the posted span now starts at
the last such block and still runs to the last verdict-bearing block, so
superseded drafts are dropped and a trailing "verdict stands unchanged"
tail is kept.
One heading leaves gha#710's behaviour untouched.
Tests
verdict-redrafted-thrice.json(three drafts plus an instrument re-runbetween the first two) must post the third draft and must not post the
first; disabling the new branch turns it red.
assert_passnow holds every posted review to at most one verdictheading, as a shape check on the extraction rather than a verdict parse.
Originally stacked on #807; rebased onto
mainafter that merged.🤖 Generated with Claude Code