claude.yml: diagnose a rejected push, preserve the work, and stop narrating it as success

[dem-extra1]

Closes #360.

The two defects

A rejected post-step push in claude.yml did two bad things at once.
The agent's commits existed only on the runner, so they died with the job, with a raw git error in a failed step as the only signal.
And because the PR head SHA never moved, the later "Post Claude's response if no code was committed" step could not tell the rejection apart from a genuinely code-free run, so it posted Claude's prose describing the fixes it had just made onto a branch carrying none of them.

What this does

Diagnose. A new classify-push-failure.sh reads a failed push's output and names the failure kind (workflows-permission, push-protection, non-fast-forward, other, plus no-push-attempt which the composite assigns when no log exists) plus advice for it.
The workflows-permission case names WORKFLOW_TOKEN and links the README's Permissions section.
It keys on the refusing to allow ... to create or update workflow clause rather than the trailing scope name, because GitHub words that tail differently per credential: a GitHub App is rejected for lacking the workflows permission, a PAT for lacking the workflow scope.

Preserve. A new report-push-failure composite action emits the ::error::, generates a git format-patch of the commits that could not be pushed, and comments the whole thing on the issue or PR, so the work comes back with git am instead of being redone.
Both of claude.yml's push sites capture their output and call it.

Withhold when publishing would leak. The classifier emits a second output, withhold-patch, computed from GitHub's secret-scanning markers independently of kind, and the composite gates patch publication on that.
Those commits carry the secret the push was blocked to contain, and Actions' masking does not cover commit content.
Keeping it separate from kind is the point: kind is a first-match chain, so a push that both edits a workflow file and carries a secret classifies as workflows-permission, and a kind-keyed gate would publish the credential anyway.

Stop the false claim. The response-post step is now gated on the PR push not having failed.
Only the PR path can have that gate: the issue-trigger push runs several hundred lines further down, so its outcome does not exist yet at that point.

Notes on the implementation

• Credential redaction is load-bearing, not defensive. Actions masks secrets in a run log but not in a comment body, so a raw copy of git's output would have published the push token rather than starring it out. The selftest asserts the redaction against a real call.
• The report step gates on the push step's outcome, not on a flag that step wrote. outcome is set by the runner, so nothing about the report's inputs depends on the step that failed. It is also the wider signal: the push step can fail before reaching the push at all (the auto-commit sweep, the fork lookup), which the action reports as no-push-attempt rather than standing down — since the response-post step is gated off on that same failure, standing down would leave the thread silent, the exact outcome claude.yml: a rejected workflow-file push discards the agent's commit silently, then a post-step claims the fix shipped #360 exists to prevent.
• Fenced blocks measure the longest backtick run in their content and open with one more, the same reasoning strip-non-invoking-markup.sh uses. A patch touching a Markdown file carries ``` lines of its own, which a fixed three-backtick fence would let close the block early.
• The byte budget bounds the whole body, not just the patch: the log gets a fixed slice and the patch takes the remainder, because a verbose rejection could carry the total past GitHub's comment limit on its own and 422 the post.
• Truncation reads a file, never a pipe. printf ... | head -c leaves printf writing to a closed pipe once head has its bytes, so any patch past the ~64 KiB pipe buffer raised SIGPIPE, which pipefail promotes and set -e turns into an aborted report — losing the comment precisely for the large patches that most need preserving.
• || push_status=$? rather than a bare call, because the step runs under the default bash -e, which would exit on the failing push before the log could be echoed.

Verification

• run-classify-push-failure-tests.sh: 24 cases. Covers the verbatim claude.yml: a rejected workflow-file push discards the agent's commit silently, then a post-step claims the fix shipped #360 rejection plus its PAT and OAuth App wordings, two non-fast-forward phrasings, the cases that must fall through to other, both secret-scanning wordings, a log where the workflow-permission and secret-scanning markers co-occur (asserting kind=workflows-permission and withhold-patch=true), the five-line output contract the composite parses, and that a generic failure's advice does not name WORKFLOW_TOKEN — naming the secret is only worth anything if it is named solely when it is the cause.
• The composite's shell body was extracted from action.yml and run against a throwaway git repo whose commit carries a planted credential. Both round-5 fixes were confirmed to fail against pre-fix code: the credential reached the comment body and the run log, and the withheld-patch case appended "see the workflow run for the full log" under advice saying the run log omits it too.
• _selftest.yml's review-fail-check job runs the offline suite and calls report-push-failure through four real uses: steps with dry-run: true — one per classified kind. The push-protection one asserts no patch is rendered and that no run-log pointer contradicts that.

Corrections to earlier revisions of this description

Two claims this body previously made were wrong, and are retracted here rather than only in the review threads:

1. The GH013 envelope claim. An earlier revision said GitHub wraps a workflow-permission rejection in the generic GH013: Repository rule violations envelope, and used that to justify the classifier's chain order. It does not. The real claude.yml: a rejected workflow-file push discards the agent's commit silently, then a post-step claims the fix shipped #360 log has no GH013 line; that line came from a fixture in this PR, added under a comment claiming the fixture was verbatim. The design no longer depends on the claim either way, since withhold-patch is now computed independently of the chain.

2. The errexit mechanism. An earlier revision said bash suppresses errexit "inside a command substitution that is part of an assignment." That is false — x="$(false)" does abort. The actual mechanism is that a command substitution's subshell does not inherit errexit unless shopt -s inherit_errexit is set, which makes it a default rather than a guarantee. The fence_for bug this described was real and is fixed by tolerating the empty grep match where it happens, so the behaviour no longer depends on call context; only the explanation was wrong.

What is not covered

claude.yml's own layer above the composite cannot be exercised until @v2 advances past this merge — it calls the action via Morrison-Lab/gha/...@v2 — the same bootstrapping gap detect-review-request and report-failure already carry.
_selftest.yml never invokes claude.yml, so this does not redden any check in the meantime.
Per that same gap, this PR's own review runs the pre-fix claude.yml@v2.

[Copilot]

Copilot wasn't able to review any files in this pull request.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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

[github-actions]

🤖 Jules is reviewing this PR. Results will appear here shortly (typically 2–5 minutes).

[Copilot]

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

[github-actions]

⚠️ Jules PR review failed to complete.

Jules did not return a review within 15 minutes. Session: `2454458090736366679`. Re-run the workflow or check the Jules dashboard.

See the workflow logs for details.

[Copilot]

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

[github-actions]

🤖 Jules Review

Summary

This PR gracefully intercepts and diagnoses rejected git push attempts, preventing the agent from falsely declaring success when its commits are lost on the runner. By extracting and preserving the unpushed work as a format-patch in the PR/issue comments, it provides an excellent recovery path while securely masking credentials in the raw git output.

Strengths

• Exceptional bash engineering: Highlights include robust dynamic markdown fence sizing, byte-exact truncation with multibyte character safety (iconv), and disciplined set -euo pipefail usage throughout.
• High attention to security: The credential redaction correctly anticipates that GitHub Actions secret masking applies only to logs, not to the payloads sent to the API for comment bodies.
• Thorough test strategy: The combination of an offline script suite exercising various git rejections and an end-to-end composite call via dry-run: true gives very high confidence in the changes.

VERDICT: approve

---

Session: 3985559600232265083

[dem-extra1]

Self-review (no external verdict available)

Neither configured reviewer produced one on this head:

• Jules errored twice with "Jules did not return a review within 15 minutes" (jules/review on 0cd7751).
• review / claude-review's first attempt failed hard at $0.0000 in 16 seconds, which reads as a credential or quota problem rather than anything in the diff; the run on 0cd7751 sat in_progress for over 20 minutes without posting.

Per the "do the review yourself when the workflow doesn't produce a verdict" rule, here is a self-review rather than a stalled PR. It is not a substitute for an external one -- if either reviewer comes back, I'll disposition its findings normally.

Findings I raised against myself, and fixed in 6111003

1. Latent abort on a backtick-free push log (correctness, would have shipped).
fence_for sizes a Markdown fence by counting the longest backtick run in the text. grep exits non-zero when there are none -- which is the ordinary shape of a network or auth failure, exactly the other branch. Under the composite's own pipefail, that fails the pipeline, and set -e turns it into an aborted report: the push failure would go unexplained precisely when the classifier had the least to say about it.

It survives on main-as-proposed only by accident. Every call site happens to wrap it in x="$(fence_for ...)", and bash suppresses errexit inside a command substitution that is part of an assignment. Verified directly rather than reasoned about -- the same function called as a plain statement:

old      EXIT=1          (aborts, emits nothing)
fixed  ``` <- survived   EXIT=0

Fixed by tolerating the empty match explicitly, so the behaviour no longer depends on call context, and pinned with a second real uses: call in _selftest.yml against a backtick-free log. The original fixture cannot catch this: a workflows-permission rejection always carries backticks.

2. other's headline said "Push rejected" (accuracy).
That branch also covers failures the remote never saw -- a DNS failure, a timeout, a bad credential. Changed to "Push failed".

3. The non-fast-forward advice referred to "the patch below" (accuracy).
The patch is omitted when no baseline SHA is available or git format-patch fails, so the sentence could name something absent. Reworded to "any patch included in this comment".

What I checked and found clean

• DRW. No existing composite here posts a diagnosis-plus-patch comment; open-failure-issue files issues against a title key, a different job. The CSV/split and cost helpers were reused where relevant rather than re-rolled.
• Modularity. Pure classification is in a script with offline tests; the gh calls, redaction, and patch generation are in the composite -- the same split open-failure-issue uses.
• Security. The redaction is the one thing that must not regress, since Actions masks secrets in a run log but not in a comment body. Asserted in CI against a real call, not just locally.
• Prose. Punctuation and semantic line breaks were swept over added lines; three multi-sentence lines in CLAUDE.md were split before the first push.

Residual limitation, stated rather than papered over

claude.yml's own layer above the composite cannot be exercised until @v2 advances past this merge, since it calls the action at that tag. That is the same bootstrapping gap detect-review-request and report-failure already carry, and _selftest.yml never invokes claude.yml, so nothing goes red in the meantime. Per this repo's own docs, a PR editing claude.yml also cannot exercise its own fix before merge -- the dogfood workflow runs the released, pre-fix version.

---

Generated by Claude Code

[claude]

Code review

Reviewed the diff (gh pr diff 361), the full root CLAUDE.md, and the actual merged .github/workflows/claude.yml / _selftest.yml state at 6111003a7f09bb450db582a02ca030786e961b8e, using four independent parallel passes (two CLAUDE.md-compliance, two diff-only bug scans) plus my own manual verification of every claim before including it below.

CLAUDE.md compliance: no violations found. The github.action_path resolution, the @v2-in-same-PR bootstrapping-gap pattern, the changelog fragment, the Tests-section e2e/offline pairing, and the doc-sync across README.md/website/permissions.qmd/website/reference/claude.qmd all check out against the precedents this file itself documents.

Bugs found (posted inline):

1. High — the patch-truncation pipe (printf | head -c | iconv) in report-push-failure/action.yml runs under pipefail + errexit and can SIGPIPE-abort the entire report for any patch over ~64 KB, silently dropping the comment (no patch, no gh issue comment) in exactly the large-patch case this feature exists to preserve.
2. Medium — claude.yml's comment claims report-push-failure "reports" a pre-push failure (auto-commit sweep, fork lookup); the action actually stands down silently on a missing push-log. Combined with the new steps.push_pr.outcome != 'failure' gate on the response-post step, the auto-commit-sweep-failure case now leaves the PR thread with no comment at all, where before it at least got Claude's (misleading) prose reply.
3. Low — git format-patch --stdout ... 2>&1 merges stderr into the patch payload on the success path too, risking a corrupted git am target if git emits any warning.
4. Low — a code comment states GitHub's comment cap as "65,536 bytes," contradicting this same CLAUDE.md's own correct documentation of the limit as 65,536 characters elsewhere. Not a functional bug given the conservative 40000-byte patch cap.
5. Low/optional — the credential-redaction regex doesn't cover the userinfo-only URL form (https://<token>@host); not reachable from either current caller, but worth hardening in a reusable action.

One thing I checked but did not flag as a bug, since it came up in two of the four parallel passes: the fence_for() comment's claim that "bash suppresses errexit inside a command substitution that is part of an assignment" for a whole-function call. I verified this against mywiki.wooledge.org/BashFAQ/105 directly — shopt -s inherit_errexit (default off) specifically unsets errexit inside a command-substitution subshell for multi-statement sequences (which a function-body invocation is), while a bare single-substitution assignment like x=$(false) is a distinct case that does trigger errexit normally. Read carefully, the PR's comment is scoped to the former case and is technically correct, not backwards.

Not flagged inline, worth a look regardless: the PR description says "Both of claude.yml's push sites capture their output and call it," but there's a third raw, uncaptured git push at claude.yml:602 (the eager_pr up-front seed-commit push, before Claude runs). Likely a deliberate and defensible scope narrowing — no Claude-authored commits are at stake in that push, only an empty scaffolding commit — but the description doesn't disclose the exclusion, so a reader could reasonably conclude every push site was hardened.

Everything else — the github.action_path arithmetic, every steps.*.outputs.* reference, the kind=/headline=/blank/advice parsing contract, the fence_for backtick-fence arithmetic, the selftest step ordering and permissions, and the prose/doc additions — checked out correctly.

Verdict

Needs more work — findings 1 and 2 are real correctness gaps in the new feature's core failure-recovery path (the first can silently defeat the whole PR's purpose for large patches); the rest are minor and non-blocking.

[github-actions]

Claude finished review — View run

---

Posted the review on PR #361: 5 inline comments plus a summary with the verdict.

Findings:

1. High — report-push-failure/action.yml's patch-truncation pipe (printf | head -c | iconv) runs under pipefail+errexit and can SIGPIPE-abort the whole report for any patch over ~64 KB — silently dropping the comment in exactly the large-patch case the PR exists to preserve. (Independently confirmed by two separate Opus bug-scan passes.)
2. Medium — claude.yml's comment claims the report action covers a pre-push failure (auto-commit sweep), but the action actually stands down silently on a missing push-log; combined with the new response-gate, that edge case now leaves the PR thread with zero comments.
3. Low — git format-patch ... 2>&1 merges stderr into the patch on the success path too, risking a corrupted git am target.
4. Low — a comment says the GitHub comment cap is "65,536 bytes," contradicting this same CLAUDE.md's own documented "65,536 characters" fact.
5. Low/optional — credential redaction misses the userinfo-only URL form (not reachable by current callers).

I also verified and rejected one claim two of the four parallel reviewers raised (that a bash-errexit/command-substitution comment was "backwards") — checked it against BashFAQ 105 directly and confirmed the PR's comment is actually correct once you account for inherit_errexit semantics on multi-statement function calls.

Verdict posted: Needs more work (findings 1 and 2 are genuine correctness gaps; the rest are minor).

[github-actions]

💰 Cost: $21.6932 (review) — run

[Copilot]

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

[github-actions]

⚠️ Jules PR review failed to complete.

Jules did not return a review within 15 minutes. Session: `7357028752440614744`. Re-run the workflow or check the Jules dashboard.

See the workflow logs for details.

[claude]

Code review

This PR has already been through 3 rounds of review; round 3 gave Ready for merge with one remaining minor finding (push-protection missing from 3 enum-documentation sites), which is fixed in the current head commit e3f763b. This round re-reviewed the current diff for CLAUDE.md compliance, bugs, and security, and found two new issues worth blocking on plus a few doc-accuracy nits.

New findings (posted inline where the diff allowed it):

1. Security (blocking): .github/workflows/scripts/classify-push-failure.sh checks workflows-permission before push-protection in a first-match if/elif chain, and report-push-failure/action.yml's patch-suppression gate keys only on the winning kind. A push rejected for both reasons at once (edits a workflow file without WORKFLOW_TOKEN and contains a secret) would be classified workflows-permission, and the secret-bearing commit gets published verbatim in a public comment and the run log -- the exact class of leak round 2 fixed, reachable through a second door. See inline comment on classify-push-failure.sh#L36-L88.
2. Bug (blocking): For kind == push-protection, report-push-failure/action.yml's comment body always ends with "No patch could be generated; see the workflow run for the full log" (since RUN_URL is always set by both call sites) directly under text saying "No patch is included in this comment, deliberately ... The run log omits the patch for the same reason." Self-contradictory, and points the reader at a run log that also has no patch. See inline comment on action.yml#L277-L281.
3. Doc accuracy: CLAUDE.md's new report-push-failure paragraph calls the issue-trigger call site "the issue-trigger finalize" -- it's actually gated on the push step (push_issue), not finalize_issue (which never calls report-push-failure). See inline comment on CLAUDE.md#L256-L258.
4. Doc accuracy: this PR splits the old combined step "Push branch and finalize PR for issue trigger" into push_issue + finalize_issue, but three existing comments still reference the old combined name and weren't swept: .github/workflows/claude.yml#L22 (top-of-file summary -- inline comment posted here); .github/workflows/claude.yml#L781-L785 (the "Extract run cost" step's justification -- could not anchor inline, outside the diff's line range; the claim there should now name finalize_issue, since $COST_USD is only read in that step's body); CLAUDE.md#L124-L128 (the extract-total-cost Layout entry -- could not anchor inline, outside the diff's line range; same fix).
5. Minor: the new comment at claude.yml#L962-L966 ("Only the PR path needs that gate ... the check below already reads 'committed' and this step stands down") overclaims -- it holds only when Claude committed directly on the issue path. If Claude leaves only uncommitted changes, this step (which runs before push_issue's later auto-commit sweep) still posts success-shaped prose, since push_issue hasn't run yet and COMMITTED reads false. Not silent (a later report step corrects it if the push then fails), but the comment's reasoning doesn't hold for that case. See inline comment on claude.yml#L962-L966.

All 8 fixes from rounds 1-2 were re-verified present and correct in the current diff (SIGPIPE-safe truncation, stderr no longer spliced into the patch, credential redaction broadened, the push_issue/finalize_issue split correctly fixing the round-2 gate bug, byte-budget math, etc.), and round 3's enum-documentation fix is confirmed in e3f763b.

Verdict: Needs more work

[github-actions]

Claude finished review — View run

---

The summary comment posted successfully. All findings have been posted: 5 inline comments plus a summary comment covering the 2 stale-reference locations that GitHub's diff anchor rejected.

Review complete

PR #361 had already been through 3 rounds of review (round 3: "Ready for merge" with one minor fix, confirmed present in the current head e3f763b). This round found 2 new blocking issues:

1. Security bug: classify-push-failure.sh checks workflows-permission before push-protection in a first-match chain, and the patch-suppression gate in report-push-failure/action.yml keys only on the winning kind. A push rejected for both reasons at once would republish a secret-bearing commit verbatim in a public comment — the same class of leak round 2 fixed, reachable through a second door.
2. Self-contradicting output: for kind == push-protection, the composite's comment body always appends "No patch could be generated; see the workflow run" right under text saying the patch was withheld deliberately and the run log has it too — reproduced empirically by the validation agent.

Plus 3 lower-severity doc-accuracy findings (CLAUDE.md misnaming a step, stale references to a step name this PR renamed, and an overclaiming code comment).

All 5 posted as inline comments (2 doc findings couldn't be anchored inline since they sit outside the diff's changed-line range, so those went into the summary comment instead). Full summary posted at #361 (comment).

Verdict

Needs more work

[github-actions]

💰 Cost: $18.1027 (review) — run

[Copilot]

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

[github-actions]

🤖 Jules Review

Summary

This PR expertly resolves an issue where a rejected git push in the claude.yml workflow would silently discard unpushed commits and incorrectly narrate a successful run. It introduces a resilient composite action that diagnoses the raw git rejection, redacts any leaked tokens in the output, and securely posts the explanation alongside a git format-patch of the stranded commits to the issue or PR thread.

Strengths

• Outstanding security design: Independently gates the withhold-patch logic to definitively prevent leaking credentials caught by GitHub's secret scanning. Effectively redacts x-access-token from the raw PUSH_LOG before it reaches the unmasked comment context, and dynamically sizes Markdown code block fences to completely neutralize Markdown injection from arbitrary commit content.
• Deep robustness: Thoughtfully anticipates and navigates GitHub Actions bash quirks, such as preventing head -c SIGPIPE pipeline abortions, handling multibyte Unicode truncation with iconv -c, and properly detecting a no-push-attempt scenario if the runner crashes before ever reaching the git push.
• Excellent testing structure: Extensively exercises the classifier offline through a matrix of regex edge-cases and tests the composite action end-to-end within _selftest.yml.

VERDICT: approve

---

Session: 17239025379218020877

[dem-extra1]

Round 5 — all five addressed in 7916a20

First, a correction I owe from round 2, because I published it and you had to be the one to catch it.

I wrote, in a review reply, a code comment, and a commit message, that GitHub wraps a workflow-permission rejection in the generic GH013: Repository rule violations envelope, and used that to justify the chain order. That is false. The real gha#360 log has no GH013 line at all. What made it look true was a GH013 line in this repo's own app_rejection test fixture — which I added, in this PR, earlier in the same session. So I "verified" the claim against my own invention and then asserted it as a fact about GitHub. You spotted exactly that, and it was the load-bearing part of the reasoning, not a footnote.

The fixture is now verbatim (no GH013), with a separate both_markers fixture carrying both, and its comment records why so nobody re-adds the line. The design no longer depends on the claim in either direction.

---

1. Security: co-occurring markers misfiled as workflows-permission — fixed, as suggested

Implemented your decoupling exactly. classify-push-failure.sh now emits withhold-patch= as its own line, computed from the secret-scanning markers before the kind chain runs, and action.yml gates publication on that rather than on kind. Ordering is now purely about which explanation the reader gets.

When the markers fire but another kind wins, the classifier appends the no-patch paragraph to that kind's advice — otherwise the omission would be silent, which is the failure mode the no-push-attempt text was already fixed for.

Confirmed the bug was real and confirmed the fix, by running the composite's script body against a throwaway repo whose commit carries a planted credential and a log matching both clauses:

assertion	pre-fix	post-fix
no patch rendered	FAIL	ok
credential absent from comment body	FAIL	ok
credential absent from run log	FAIL	ok
body explains the missing patch	FAIL	ok
body still names WORKFLOW_TOKEN	ok	ok

So pre-fix it reached both destinations, as you said.

2. Self-contradicting fallback — fixed, plus the selftest gap you named

elif [[ "$withhold_patch" != "true" && -n "$RUN_URL" ]], keyed on the withhold flag rather than on kind so it tracks finding 1's fix. And you were right that the selftest could not have caught it: that step omitted run-url unlike both production call sites, so the branch was unreachable. It now passes run-url and asserts the string is absent. That assertion fails against pre-fix code (last row of the same harness).

3. CLAUDE.md "the issue-trigger finalize" — fixed

Now names both steps by title. finalize_issue doesn't call the action at all, as you said.

4. Stale combined-step-name references — fixed, all three

claude.yml header, the extract-run-cost comment, and the CLAUDE.md extract-total-cost entry. Grepped for the old name repo-wide afterward; those three were all of them.

5. Response-post comment overclaims — fixed

Rewritten to say the real reason: push_issue runs several hundred lines below that step, so its outcome does not exist yet and an equivalent condition would always be empty. The uncommitted-changes case is now described accurately rather than folded into the committed one.

---

Coverage: classifier suite is 24 cases (was 18), including your explicit ask — a log where both patterns co-occur, asserting kind=workflows-permission and withhold-patch=true, plus the withhold value for the three other kinds and the new 5-line output contract. _selftest.yml's four real uses: calls are unchanged in count; the push-protection one gained run-url and the contradiction assertion.

Both new behaviours were checked against pre-fix code and fail there, per the repo's regression-test rule.

---

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.

[claude]

Code review

This PR has already been through 5 rounds of review (round 3: "Ready for merge" with one minor fix; round 4: 2 blocking + 3 doc-accuracy findings; round 5/the author's own reply: all five addressed in 7916a20, the current head). I independently re-verified the current state at HEAD (7916a20a) rather than trusting the prior rounds' claims at face value:

• Round 5's security fix (co-occurring workflows-permission + secret markers) — confirmed withhold-patch is now computed independently of kind in classify-push-failure.sh (before the kind chain runs), and report-push-failure/action.yml's patch-suppression gate reads withhold_patch, not kind. Verified against the both_markers test fixture and the co-occurring markers still withhold test case.
• The self-contradicting "no patch could be generated ... see the workflow run" fallback — confirmed the elif guarding that line now reads withhold_patch != "true" && -n "$RUN_URL", and _selftest.yml's push-protection e2e step now passes run-url and asserts the contradiction string is absent.
• The gh pr create-after-successful-push false report — confirmed claude.yml's issue-trigger path is now split into "Push branch for issue trigger" (id: push_issue, whose last action is the push, pushed=true written only after success) and "Finalize PR for issue trigger" (id: finalize_issue, gated on steps.push_issue.outputs.pushed == 'true') — matching the PR-trigger site's existing guarantee.
• The three stale "Push branch and finalize PR for issue trigger" references — confirmed all three now read the split names, in claude.yml's header comment, its "Extract run cost" step comment, and CLAUDE.md's extract-total-cost entry.
• The response-post step's overclaiming comment — confirmed it now correctly distinguishes the committed vs. uncommitted-changes cases for the issue trigger.

All confirmed fixed and consistent with their own test coverage (the classify-push-failure suite's 24 cases, and _selftest.yml's four real uses: steps against report-push-failure, including the run-url-bearing push-protection case).

One new finding, posted inline: CLAUDE.md's Tests-section paragraph on run-classify-push-failure-tests.sh still describes "the three-part output contract... kind=, headline=, blank line, advice" — stale since round 5 added withhold-patch= as its own output line (the script's header comment and the Layout section's own report-push-failure paragraph both already reflect the newer, five-line shape). Minor doc-accuracy gap, not a functional issue.

Verdict

Ready for merge

[github-actions]

Claude finished review — View run

---

I've completed the review of PR #361 and posted both the inline finding and the summary comment.

Summary: This is round 6 of review on a PR that's already been through 5 rounds. I independently re-verified (not just trusted) that all fixes claimed in round 5's reply commit (7916a20) are actually present and correct at HEAD:

• The withhold-patch/kind decoupling (the round-5 security fix for co-occurring workflows-permission + secret markers)
• The self-contradicting "no patch ... see the workflow run" fallback fix
• The push_issue/finalize_issue split fixing the post-successful-push gh pr create false-failure report
• The three stale step-name references
• The response-post comment's overclaim fix

All confirmed correct against the actual code and test fixtures, not just the commit message's narrative.

One new finding (posted inline on CLAUDE.md): the Tests-section paragraph describing run-classify-push-failure-tests.sh still calls it a "three-part output contract" (kind=, headline=, blank, advice), which predates round 5's addition of withhold-patch= as its own output line — a doc-sync gap that slipped through the same PR that added the field. Minor, non-blocking.

Verdict

Ready for merge

[github-actions]

💰 Cost: $4.5633 (review) — run

[Copilot]

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

[github-actions]

🤖 Jules Review

Summary

This PR implements a robust, secure, and incredibly thoughtful mechanism for preserving an agent's commits when a post-run git push fails. By correctly trapping the failure, parsing the git output, and rendering a recovered patch directly to the thread, it prevents work from silently dying on the runner.

Strengths

• Exceptional Bash & Runner Mechanics: Handling SIGPIPE in byte-truncation by avoiding pipes, using iconv to protect UTF-8 boundaries, dynamically sizing Markdown backtick fences to prevent collisions, and segregating git format-patch standard error to keep the patch pristine for git am are all masterclasses in shell scripting.
• Flawless Security Design: Decoupling withhold_patch from the first-match kind chain is a brilliant defense against edge cases. This guarantees a secret caught by push protection will never leak into a comment simply because another error type (like workflows-permission) matched the log first.
• Careful Workflow State Gating: Tying the reporting step strictly to the push step's outcome—and correctly managing push_status so the failure bubbles up accurately—shows a deep understanding of GitHub Actions' execution engine.

VERDICT: approve

---

Session: 7770707610905300072

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.

_{2026-07-29 22:46 UTC}

[github-actions]

Claude finished review — View run

---

Code Review — #361

This PR has already been through 6 rounds of review. I independently re-verified the current state at HEAD (a1357c75) against every finding raised in the prior round rather than trusting the round summaries at face value, reading the actual diffs (not just commit messages) for .github/actions/report-push-failure/action.yml, .github/workflows/scripts/classify-push-failure.sh, .github/workflows/claude.yml, and _selftest.yml.

All 8 findings from the prior round are fixed, confirmed against current code:

1. SIGPIPE truncation abort (action.yml) — truncate_to_bytes() now writes to a temp file first, then head -c reads from the file, not a live pipe. No producer left for head to SIGPIPE.
2. claude.yml:889 comment claim mismatch — fixed from the action side: the missing-push-log branch now actually posts a no-push-attempt report (with patch) instead of standing down silently, so the comment's claim is now accurate.
3. stderr spliced into patch via 2>&1 — git format-patch's stderr is now captured to a separate patch_err file and only surfaced via ::warning::, never mixed into $patch.
4. 65,536 bytes vs. characters inaccuracy — corrected to "CHARACTERS" in the comment, consistent with CLAUDE.md's own documented limit.
5. Credential redaction regex gap (bare token@host form) — the pattern is now (https://)[^/@[:space:]]+@ → ***@, covering both the user:pass@ and userinfo-only forms.
6. gh pr create failure after successful push mislabeled as push failure — claude.yml's issue-trigger path is now split into push_issue (push is the step's last action, pushed=true written only on success) and finalize_issue (gated on steps.push_issue.outputs.pushed == 'true'), matching the PR-trigger site's existing guarantee. The "Report a rejected issue-branch push" step is gated only on steps.push_issue.outcome, which can no longer be tripped by a later gh pr create failure.
7. GH013 push-protection secret republished — classify-push-failure.sh now computes withhold-patch independently of (and before) the kind chain specifically to avoid the co-occurring-marker gap round 5 found; action.yml gates both the comment-body patch and the run-log dump on withhold_patch, not kind.
8. Unbounded raw_log/total comment body — raw_log is now capped via MAX_LOG_BYTES (10000), and the patch budget is computed as the remainder of a TOTAL_BODY_BYTES (55000) budget after the log and fixed prose, rather than a fixed 40 KB — so the total is bounded, not just the patch.

The final commit (a1357c75, "docs: correct the output contract's part count in CLAUDE.md") is a small, scoped, and correct doc fix for round 6's one remaining finding — I diffed it in isolation and confirmed it changes only the "three-part" → "four-part" wording, consistent with the existing counting convention (kind=, withhold-patch=, headline=, and the blank-line+advice block counted as one part), and verified its new added claim ("reads each field by fixed line offset") against the actual sed -n '1s/...'/'2s/...'/'3s/...'/tail -n +5 parsing in action.yml — accurate.

I also spot-checked the new _selftest.yml steps (four real uses: calls: workflows-permission, backtick-free other, no-push-attempt, and push-protection) and classify-push-failure.sh in full — both match what the round-6 summary claimed and contain no new issues.

This confirms round 6's verdict at Morrison-Lab/gha#361 comment ("Ready for merge," with the one doc-sync gap noted) still stands, and that gap is now closed.

No new findings; no inline comments posted (--comment was not passed for this invocation, and there is nothing new to flag regardless).

Verdict

Ready for merge

[github-actions]

💰 Cost: $2.1243 (review) — run

[dem-extra1]

Ready for merge — a1357c7

Seven review rounds, closing out. Recording what the clean call rests on, since not every configured reviewer weighed in.

Verdicts at this head

reviewer	verdict at a1357c7
review / claude-review	Ready for merge, no new findings — re-verified all 8 prior findings against the code rather than the round summaries
Jules (jules/review)	approve — status success, "Review complete (verdict: approve)"
Copilot	never weighed in, at any head — quota-refused every request across all seven rounds

Two independent external approvals at the current head, so this is not resting on self-review. Copilot's absence is stated rather than papered over: it refused on every head, so it was never "reachable" in the sense criterion 2 requires.

CI

All 30 check runs completed, every one success or skipped; the jules/review commit status is success. Nothing queued, nothing in progress.

Worth noting for anyone reading the timeline: mergeable_state read unstable for about a minute after the last check run finished, because jules/review landed at 22:30:21 and the rollup hadn't caught up. That was the rollup lagging, not a failing check.

Threads

All 17 inline threads resolved, swept by thread id rather than from memory of which findings were answered.

Freshness

main unmoved at 267ad3a; git merge-tree reports zero conflicts; local HEAD matches this PR's head.sha; the 24-case classifier suite passes locally at this commit.

What the review rounds actually cost, and what they caught

Two of the findings were bugs that would have shipped the feature broken in precisely the situations it exists for, which is worth recording rather than glossing:

• The SIGPIPE truncation abort killed the whole report for any patch past the pipe buffer — the large-patch case where preserving the work matters most.
• The co-occurring-marker leak published a secret-bearing patch to a public comment whenever a push both edited a workflow file and carried a credential. Confirmed by running the composite against a repo with a planted secret: pre-fix it reached the comment body and the run log.

I also retracted two claims I had published, both in the thread and in the PR body: the GH013-envelope claim (which came from a fixture I wrote in this PR, not from the real #360 log) and the wrong errexit mechanism. The first was load-bearing — the sole justification for the ordering that hid the leak above.

Two follow-ups are tracked rather than folded in: gha#368 for claude-code-review's no-verdict modes, now including the hung-retry variant seen on this PR, and the @v2 bootstrapping gap noted in the PR body, which resolves when the tag advances past this merge.

Not merging — that's a human call.

---

Generated by Claude Code