Fix claude-code-review republishing a raw gh pr comment as the review body

[d-morrison]

Problem

On UCD-SERG/serocalculator#614, the "Claude finished review" summary comment posted a raw, unexecuted shell command as its body — a gh pr comment N --body "$(cat <<HEREDOC ... HEREDOC)" invocation, shown literally instead of run.

The review itself posted fine as a separate claude[bot] comment, so the content appeared twice — once correctly, once as a mangled gh pr comment ... <<HEREDOC ... block.

Root cause

check-review-execution.sh builds its review-text candidates from assistant blocks. A gh pr comment / gh api .../comments Bash call is a deliberate candidate (so a verdict the agent posted directly, but never restated in plain text, is still recognized) — but only when permission_denials_count == 0, since those calls are otherwise denied (gha#218 finding 1).

The bug: when that candidate was selected, the code used the whole command string as the review text. That string feeds two consumers — the pass/fail verdict scan (fine, the verdict is inside the heredoc) and review_text_file, which is posted verbatim. So the wrapper republished the command.

It's intermittent, which is why prior rounds looked clean: it only triggers when the agent's final action is a gh pr comment self-post rather than plain narration or the inline-comment MCP tool.

Fix

Unwrap the heredoc body from such a command before using it as the posted text; fall back to the command unchanged when there is no heredoc (a --body/--body-file form the scan still reads a verdict from). Two implementation notes, both load-bearing:

• The single quote in the heredoc-delimiter match is written as the \x27 hex escape, so the jq program carries no literal quote to collide with the surrounding shell single-quoting.
• capture() is wrapped in [...] | first, because it yields empty (not null) on no match — which would annihilate the whole pipeline. (Hit this in a first draft; the block came out empty.)

review_text_file and all_text_file still derive from the same unwrapped block, so the gha#218 finding-2 invariant (posted text and scanned text never diverge) holds.

Test

Adds fixture verdict-via-gh-comment-heredoc.json — a denials=0 heredoc self-post plus a trailing no-verdict narration block — wired into run-fixture-tests.sh with:

• expected = pass
• must_contain = 'One real finding on line 12' (the unwrapped review)
• must_not_contain = 'gh pr comment' (proves the command string is no longer posted)

Parallels the existing verdict-via-inline-comment-tool.json, which asserts the same posted-vs-scanned distinction for the other posting path.

Verification: macOS ships bash 3.2, so the declare -A runner can't execute locally — I drove the real check-review-execution.sh directly against the new fixture and the neighbours (genuine-finished-review, verdict-via-inline-comment-tool, denied-bash-comment-not-trusted, empty-review-text), confirming the new case posts the unwrapped body and every existing case still behaves. The full declare -A suite runs on CI (Ubuntu / bash 5).

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Code review

Reviewed the diff, check-review-execution.sh's full surrounding context, CLAUDE.md, and changelog.d/README.md. Two findings posted as inline comments:

1. Tactical correctness bug (inline comment): the new unwrap_posted_body heredoc-closing-delimiter regex (\n[ \t]*\k<tag>\b) has no end-of-line anchor after the tag, only a word-boundary assertion. Because the body capture is lazy, a review body containing a line that starts with the tag word followed by a non-word character (very plausible here, since this script's own domain is reviewing heredoc/shell code) would cause the match to terminate early, silently truncating the posted review. Confirmed independently by hand-tracing the regex and by an adversarial validation pass that empirically reproduced the truncation with a concrete example.
2. Process nit, non-blocking (inline comment): the changelog entry was added directly to CHANGELOG.md rather than as a changelog.d/ fragment, per CLAUDE.md's documented (though not CI-enforced) convention — the immediately preceding PR (Tolerate extra whitespace in @claude review dispatch matcher #317) followed the fragment convention correctly.

Everything else checks out: the \x27 hex-escape / \\s/\\S/\\k/\\b double-backslash escaping is correct (verified by hand-tracing jq's JSON-string-escaping semantics against what reaches Oniguruma), the capture() empty-vs-null handling and set -euo pipefail safety are correct, the new fixture and its wiring into run-fixture-tests.sh match the existing pattern, and the CHANGELOG prose is accurate and free of AI-authorship tells.

Verdict

Needs more work — the regex-truncation bug (finding 1) should be fixed, and ideally pinned with a fixture where the review body itself contains a line starting with the heredoc tag, before merge.

[github-actions]

Claude finished review — View run

---

Review posted. Summary:

PR #318 fixes a real bug where check-review-execution.sh republished a raw gh pr comment ... <<EOF shell command as the posted PR review text instead of its heredoc body. The fix itself introduces a new bug:

1. Blocking — tactical correctness bug (check-review-execution.sh#L143-L146): the new heredoc-unwrapping regex's closing-delimiter pattern (\n[ \t]*\k<tag>\b) has no end-of-line anchor, only a word-boundary check — so a review body containing a line that starts with the heredoc tag followed by punctuation (quite plausible in this file's own domain of reviewing heredoc/shell code) truncates the posted review early. Confirmed independently by two separate reasoning passes and an adversarial validation agent that empirically reproduced the truncation.
2. Non-blocking — process nit (CHANGELOG.md#L141-L153): the changelog entry was hand-added to CHANGELOG.md instead of as a changelog.d/ fragment, per CLAUDE.md's documented (but not CI-enforced) convention.

Both inline comments and a top-level summary comment with the verdict were posted to the PR.

Verdict

Needs more work — finding 1 should be fixed before merge.

[github-actions]

💰 Cost: $7.1919 (review) — run

[d-morrison]

Working on this — paws off until I'm done.

---

Generated by Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[d-morrison]

ARD round 1 — both findings addressed (a412539)

#	Finding	Disposition
1	Heredoc terminator has no end-of-line anchor, truncating the posted review	Addressed (fixed differently than suggested — see below)
2	Changelog entry hand-added to CHANGELOG.md instead of a changelog.d/ fragment	Addressed

Finding 1 — confirmed, and the suggested fix was insufficient

The bug is real and reproduced before touching anything. It's also a bit worse than described: truncating the body drops the verdict, which then fails the guard's own verdict scan — so the pre-fix code can turn a genuine review into a stub failure (exit=1), not just a mangled comment.

But the suggested \k<tag>[ \t]*(?:\r?\n|$) replacement, tested against the same harness, does not fix the indented EOF case from the review comment's own example. The terminator's leading [ \t]* still accepts a space-indented closing line that real bash <<EOF rejects, so the body still ends at body.

Rather than add another anchor, the regex now matches only the heredoc opener and the terminator is found by comparing whole lines against the tag — how bash itself ends a heredoc. That removes the lazy-quantifier/anchor failure mode outright instead of narrowing it, and drops the \k<tag> backreference from an already heavily-escaped jq-inside-shell string. <<-'s leading-tab stripping is mirrored too, so a tab-indented heredoc posts de-indented markdown instead of an accidental code block. With no terminator line at all (transcript truncated mid-command) it falls back to the raw command — posting a shell-looking comment beats dropping review text.

Verification

Two fixtures added, each checked in both directions — failing against the pre-fix script, passing against this one:

Fixture	Covers	Pre-fix
...-tag-in-body.json	body line starting with EOF, plus the indented EOF lookalike, verdict below both	exit=1, verdict truncated away
...-dash-tab.json	<<-'EOF' with tab-indented body/terminator	passes but posts tab-indented text

All 16 fixtures pass. main was also merged in (it had advanced past this branch with #317).

Note

Per the "can't self-verify" section of CLAUDE.md, this PR edits check-review-execution.sh, which reaches this repo's own review only through @v2 — so every review of this PR runs the pre-fix guard. The fixture suite is the real evidence here, not the review check's own behavior.

---

Generated by Claude Code

[github-actions]

Claude finished review — View run

---

Code review — #318

I re-reviewed the current head commit (a412539), which is the author's round-1 fix for both findings from the prior review (posted at issuecomment-5082989604, verdict Needs more work). Both are resolved:

Finding 1 (blocking, tactical correctness bug) — fixed, and independently verified.
The prior finding was that unwrap_posted_body's heredoc-terminator regex (\n[ \t]*\k<tag>\b) lacked an end-of-line anchor, letting a body line that merely started with the tag terminate the lazy body-capture early and truncate the posted review.

The new commit replaces the inline-regex terminator entirely: the regex now matches only the heredoc opener, and the terminator is located by splitting the body into lines and finding the first line that is exactly the tag (mirroring real bash <<TAG/<<-TAG semantics, including <<-'s leading-tab stripping). I extracted the exact unwrap_posted_body jq function from check-review-execution.sh:154-167 and ran it directly (not just hand-traced) against:

• verdict-via-gh-comment-heredoc-tag-in-body.json — a body containing "EOF markers must stand alone..." and an indented EOF lookalike before the real terminator → correctly returns the full body through **Ready for merge**, past both lookalikes.
• verdict-via-gh-comment-heredoc-dash-tab.json — a <<-'EOF' tab-indented heredoc → correctly de-indents and finds the real tab-stripped terminator.
• A synthetic no-heredoc command → correctly falls back to the raw command string.
• A synthetic heredoc with no closing terminator line → correctly falls back to the raw command rather than guessing.

This also correctly handles the case the previous review's own suggested fix (\k<tag>[ \t]*(?:\r?\n|$)) would still have gotten wrong (the indented EOF lookalike) — the author's PR description explains this and I confirmed it by execution, not just by reading the explanation.

Finding 2 (non-blocking, process nit) — fixed. The CHANGELOG.md direct edit is gone from this diff; the entry now lives at changelog.d/fix-review-heredoc-republish.fixed.md, correctly named (<slug>.<category>.md, category=fixed), with no heading, matching changelog.d/README.md's convention. Content is accurate against the code and free of AI-authorship tells.

Minor observation, not blocking: in the new unwrap_posted_body, \r is stripped only for the terminator-matching array ($lines | map(rtrimstr("\r")) | index($h.tag)), not from the array actually sliced and returned ($lines[0:$end]). I confirmed with a synthetic CRLF-heredoc input that this leaves trailing \r characters in the posted body when the source transcript uses CRLF line endings. This is a real but very narrow edge case — Claude Code execution transcripts are effectively always LF-only — and doesn't regress anything the old code handled better, so I'm not treating it as blocking, just flagging it for awareness.

No other issues found: the test wiring in run-fixture-tests.sh (expected/must_contain/must_not_contain/expected_cost tables for the three new fixtures) is consistent with the existing pattern, and the fixtures themselves match the scenarios described.

Verdict

Ready for merge — both findings from the prior review round are fixed and independently verified by executing the actual jq logic against the new fixtures and synthetic edge cases; no new blocking issues found.

[github-actions]

💰 Cost: $1.8311 (review) — run

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[d-morrison]

ARD round 2 — the non-blocking observation is addressed too (b3d60d8)

Round 2's verdict was Ready for merge, with one "minor observation, not blocking": rtrimstr("\r") was applied only to the array used to locate the terminator, not to the array actually sliced and posted, so a CRLF transcript would carry stray carriage returns into the PR comment.

Correct, and a one-line fix — so it's fixed rather than left standing. The normalization now happens once, in the same map as the <<- tab stripping, before both the terminator comparison and the slice:

( $h.body | split("\n")
  | map(rtrimstr("\r")
        | if $h.dash == "-" then sub("^\t+"; "") else . end) ) as $lines
| ( $lines | index($h.tag) ) as $end

Added verdict-via-gh-comment-heredoc-crlf.json to pin it, verified in both directions like the other two: it fails against the round-1 code (exit 0, but the body still carries \r) and passes here. Its must_not_contain assertion is a bare \r, so the fixture fails if carriage returns ever reach the posted text again.

All 17 fixtures pass. main is unchanged since the last sync, so no merge needed.

Agreed on the severity read — Claude Code transcripts are LF-only in practice, so this was defensive rather than a live bug.

Status

Both round-1 findings and round-2's observation are now addressed; all four review threads' items are settled and the inline threads are resolved. Reporting ready for merge rather than merging — that's yours to call.

One caveat worth repeating from the round-1 summary: this PR edits check-review-execution.sh, which this repo's own review only reaches through @v2, so every review of this PR runs the pre-fix guard. The fixture suite (review-fail-check, green) is the real evidence for the fix, not the review job's own behavior.

---

Generated by Claude Code

[claude]

Code review

Confirming review — re-checked the current head commit (b3d60d88f8c6291f0637dbc7bab9b1adc63ed746, the ARD round 2 push) against the prior review's outstanding item.

The prior review gave a verdict of Ready for merge, with both blocking findings from round 1 (the heredoc-terminator regex's missing end-of-line anchor, and the CHANGELOG.md-vs-changelog.d/ process nit) confirmed fixed. It also raised a non-blocking "minor observation": rtrimstr("\r") was applied only to the array used for index($h.tag) terminator matching, not to the array actually sliced and posted, so a CRLF-transcript review would still carry stray \r characters into the posted PR comment.

This new commit fixes exactly that: check-review-execution.sh's unwrap_posted_body now builds a single $lines array, with rtrimstr("\r") (and, for <<-, the leading-tab strip) applied once, and uses that same normalized array for both the terminator lookup and the $lines[0:$end] | join("\n") slice that becomes the posted/scanned text. A new fixture, verdict-via-gh-comment-heredoc-crlf.json (a CRLF-joined heredoc body), pins this with a must_not_contain: $'\r' assertion alongside the existing must_contain/expected_cost checks.

I hand-traced this fixture's bytes through the updated jq function line by line (\r-suffixed lines stripped before both the index("EOF") lookup and the final join) and confirmed the output has the correct Verdict heading, No findings text, and Ready for merge line, with no embedded \r, matching what run-fixture-tests.sh asserts. I also independently re-verified, via four parallel review passes (two for CLAUDE.md compliance, two dedicated bug scans) plus my own manual trace, that round 1's two fixes are still correctly in place in the current diff: whole-line terminator matching ($lines | index($h.tag), not a regex anchor) genuinely prevents the tag-in-body truncation case, and the changelog entry is a proper changelog.d/fix-review-heredoc-republish.fixed.md fragment.

One review agent raised a possible test-coverage gap (no fixture exercises the $h == null then $c no-heredoc fallback for a trusted gh pr comment/gh api ... comments call), but on validation this is unchanged, pre-existing behavior this PR doesn't touch, and CLAUDE.md doesn't mandate a fixture per branch, so it's not a genuine finding.

No new issues found.

Verdict: Ready for merge — the CRLF fix in b3d60d8 correctly closes the prior review's last outstanding (non-blocking) observation, and independent re-verification of the round-1 fixes found nothing amiss.

[github-actions]

Claude finished review — View run

---

The confirming review is posted correctly: comment link.

Summary

Reviewed PR #318 at its current head commit (b3d60d88, the ARD round 2 push). This round only needed to address one thing: the prior review's non-blocking "minor observation" that CRLF characters could leak into the posted review body because rtrimstr("\r") was applied only to the terminator-comparison array, not the array actually sliced for output.

I independently verified the fix by hand-tracing the updated unwrap_posted_body jq function against the new verdict-via-gh-comment-heredoc-crlf.json fixture byte-by-byte, and confirmed it now normalizes CRLF once into a single $lines array used for both the terminator lookup and the output slice — correctly closing that gap. I also ran 4 independent parallel review agents (2 CLAUDE.md-compliance, 2 dedicated bug-scans) plus my own manual trace of all four fixture scenarios (plain heredoc, tag-in-body, <<- tab-stripping, CRLF), reconfirming that round 1's two fixes (whole-line terminator matching, changelog fragment convention) are genuinely in place. One agent-raised concern (missing fixture coverage for the unchanged $h == null fallback branch) was validated and rejected as out-of-scope, pre-existing behavior with no explicit CLAUDE.md mandate.

Verdict

Ready for merge — no new issues found; the CRLF fix is correct and independently verified.

[github-actions]

💰 Cost: $7.6651 (review) — run

[d-morrison]

Confirming the all-clear now that CI has actually finished — the round-3 verdict posted while several jobs were still in flight, so this is the state at completion rather than at verdict time.

At head b3d60d8:

• All 26 check runs completed, every one success or skipped. That includes review / claude-review and review / require-review (both success), coverage, and codecov/patch — no check left running or red.
• Both inline review threads resolved, with a reply on each naming the commit that addressed it. The only open conversation is this all-clear exchange.
• main has not moved since the sync in a412539; re-checked with git merge-tree against the current merge base — zero conflicts.
• 17/17 fixtures pass, with each of the three new ones verified to fail against the code it was written to pin.

On the coverage gap one of your review agents raised and then rejected (no fixture for the $h == null no-heredoc fallback on a trusted gh pr comment call): agreed it's not a finding, and I can confirm the reasoning independently. Before this PR, that branch returned .input.command unchanged; unwrap_posted_body returns $c — the same raw command — when the opener regex doesn't match, so the no-heredoc path is byte-identical to pre-PR behavior. I did exercise it while developing (a --body 'Verdict: ok' command correctly falls through), it's just not pinned as a committed fixture. Happy to add one if you'd rather have it covered, but it isn't gating anything.

Ready for merge — leaving the merge itself to you.

---

Generated by Claude Code