feat: doc-only fast-skip + SHA marker + prior-comment collapse - #47
Merged
Conversation
…viewer
Three CI-cost reductions aimed at the failure modes documented in the
2026-04-17 AAR. All three land together because they share the same
`Check for doc-only diff` step output and the same marker contract.
1. Doc-only fast-skip (new Check for doc-only diff step, lines 126-170)
Before spinning up the Claude action, check whether every changed
file matches a doc/meta pattern (*.md, LICENSE, CHANGELOG, .gitignore,
.github/ISSUE_TEMPLATE/**, docs/, etc.). If yes, skip the paid review
and post a SKIPPED verdict to the step summary. The BLOCK criteria
(bug, reliability regression, security, data-loss) don't apply to
prose changes, so the review was adding cost and flake surface area
for zero signal. kebab-tax#1162 was a doc-only PR that failed three
times in a row on infrastructure flakes this week — this skip would
have saved the full cost of all three runs.
2. SHA marker in every posted review (prompt Step 3 change)
Each review comment now starts with:
<!-- claude-blocking-review sha=<head_sha> run=<run_id> -->
Downstream consumers (status scrapers, the new minimize step below,
future tooling) can filter to the review for the current HEAD and
ignore phantom findings from prior runs. kebab-tax#1165's post-push
loop saw FINDINGS=1→2→3→4 across four iterations precisely because
top-level issue comments carry no original_commit_id; the marker
closes that gap without changing the comment's rendered body.
3. Collapse prior review comments (new Minimize prior step, lines 240-284)
Before the Claude action runs, query the PR for comments whose body
begins with the claude-blocking-review marker and minimize them via
the minimizeComment GraphQL mutation (classifier: OUTDATED). Keeps
the PR conversation readable across iterations — GitHub collapses
minimized comments under a "Show resolved" affordance and
`gh pr view --comments` excludes them by default. Non-fatal: any
failure here logs a warning but does not block the review.
Implementation notes:
- Estimate, Minimize, and Run Claude steps are all guarded with
`if: steps.doc-check.outputs.skip != 'true'`. The final Check review
verdict step keeps `if: always()` and short-circuits at the top when
DOC_SKIP=true so doc-only PRs exit 0 cleanly.
- The marker emission is three sequential commands (printf > file,
cat >> file, gh pr comment --body-file file) rather than a pipeline
so every command stays within the existing allowed-tools prefix
allowlist at line 294 without broadening it.
- The minimize query uses --jq to filter for `.isMinimized == false`
AND body starts-with the marker, so repeated runs are idempotent
(already-minimized comments are skipped).
- OUTDATED classifier confirmed valid via GraphQL introspection;
requires pull-requests: write (already declared).
Expected cost impact:
- Doc-only PRs: eliminated (was the full ~5-10 min Claude review cost).
- PRs with multiple push iterations: each run still costs one review,
but prior reviews collapse instead of accumulating noise.
- Diff-unrelated: no change.
No change to the BLOCK/PASS contract, no change to the required status
check name, no change to the reusable workflow's inputs or secrets.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tighten the doc-only fast-skip classification per local code-reviewer feedback. Both files live under .github/ alongside true display-only config (FUNDING.yml, issue templates), but they have real security impact: - .github/CODEOWNERS controls which reviewers are required to approve changes. A malicious PR that weakens CODEOWNERS (e.g., removes an approval requirement on a sensitive path, or adds the attacker as an owner of security-critical code) should not skip review. - .github/dependabot.yml controls which ecosystems, registries, and directories get dependency updates, and what the update cadence looks like. Changes here can silently weaken supply-chain posture (e.g., pointing to an attacker-controlled registry, or disabling security updates). FUNDING.yml stays in the allowlist — it's purely display metadata for the "Sponsor this project" button and has no authorization or dependency semantics. Issue templates and .gitignore-class meta files also stay; those are build-time noise, not runtime behavior. Follow-up to the preceding commit in this branch. No other changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address two non-blocking observations from the pre-push codebase reviewer (issues #42 and #43). #42: GraphQL `comments(first: 100)` returns the OLDEST 100 comments first. On a PR that has collected >100 comments before any claude-blocking-review ran, or on a very active PR that accumulates non-bot comments faster than the minimize step can keep up, recent markers would fall outside the window and never be minimized. `last: 100` gets the newest 100 instead, which is the natural fit for "find my recent review markers to collapse" and aligns the window with how reviews accumulate chronologically. #43: `gh api -F` auto-detects argument types — repo names that happen to parse as integers (e.g. a name like "123456") would be coerced to int and fail the `String!` schema check. Switch owner/name to `-f` (explicit string). Keep `-F number=...` where int coercion is desired for the Int! PR number variable. No behavior change on the current PRs in flight. Follow-up to the two preceding commits in this branch. Closes #42 and #43 on merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Two correctness bugs surfaced by PR #47's own self-review run (which failed with an "INCOMPLETE" verdict that the step output confirmed was a false BLOCK). 1. Fallback verdict grep was unanchored (pre-existing bug, finally bit). The Check review verdict step reads the last PR comment when the verdict file isn't present, using `grep -q "VERDICT: BLOCK"` / `grep -q "VERDICT: PASS"`. These have no line anchors, so any occurrence of those strings ANYWHERE in the comment body counts as a match. PR #47's review prose discussed the workflow's own grep pattern and included the literal string `VERDICT: BLOCK` in its analysis — the fallback parser treated that as the verdict and reported the review as BLOCK despite the real terminal verdict line reading PASS. Fix: anchor both greps to line start/end (`grep -qE '^VERDICT: X$'`), so only a real terminal verdict line matches. Verified locally against the actual broken PR #47 comment: unanchored: BLOCK=1 (false), PASS=2 anchored: BLOCK=0, PASS=1 2. Verdict file was written AFTER the comment post (also pre-existing). If the Claude action ran out of turns or wall-clock time during the three-command comment post (or the comment post itself hung), the verdict file never got written and CI fell through to the more fragile fallback grep path. Reordering so the verdict file is written immediately after the VERDICT line is appended to /tmp/review.md means a partial run still produces the primary signal. The comment post is the nice-to-have; the verdict file is the contract. Step order is now: 1. Write review to /tmp/review.md 2. Append VERDICT line to review.md 3. Write verdict file <- moved up (was Step 4) 4. Post review comment <- moved down (was Step 3) Both changes are in the Check review verdict step and the prompt's Step 3/4 ordering — no change to public inputs, outputs, or the required status check name. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Claude Code Review — PR #47: doc-only fast-skip + SHA marker + prior-comment collapseSummaryThree coordinated improvements: doc-only skip, SHA-tagged comment markers, and prior-comment minimization. The changes are well-scoped and address documented failure modes. No blocking issues found. SecurityShell injection — clean. All
CODEOWNERS / dependabot.yml exclusion — correct. The explicit exclusions from the doc-only allowlist are appropriate and well-documented. ReliabilityGrep-anchoring fix ( Verdict-before-comment reordering — improvement, not a regression. Old steps 3/4 (post comment, then write verdict file) are now swapped: write file first, then post comment. If Claude exhausts turns mid-post, CI still reads a usable verdict file. No consumer depends on the old order. ✓ Doc-skip Minimize step failure isolation — correct. Every external call in the minimize step is guarded: Non-Blocking Observations
VERDICT: PASS |
5 tasks
twistedmelonman
pushed a commit
that referenced
this pull request
Apr 18, 2026
Closes #46. Under the previous implementation, `gh pr diff --name-only` returned only the destination paths for renamed files, so a rename from `src/foo.py` → `docs/foo.md` would appear as only `docs/foo.md`, classify as doc-only, and skip review — even though the diff contains significant code deletion at the source path. Switch to the `repos/{owner}/{repo}/pulls/{pr}/files` API and emit the UNION of `previous_filename` (non-null when renamed or copied) and `filename` for every entry. The doc-only check then requires every pre-rename AND post-rename path to match the allowlist — closing the blind spot. For non-renamed files `previous_filename` is null and filtered by the jq `select(. != null)` clause, so non-rename behavior is unchanged. Verified against PRs #39, #47, #52 (all modified-only) — output is identical to the prior `--name-only` approach. An actual rename case wasn't available in recent history to test empirically, but the jq emits `previous_filename, filename` separately so the case is covered by construction: if ANY of those paths is non-doc, the final classification flips to non-doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
twistedmelonman
added a commit
that referenced
this pull request
Apr 18, 2026
) (#53) * fix: use -f (explicit string) for minimizeComment ID variable Consistency with the preceding query that already uses -f for owner and name. GitHub Relay node IDs are base64-encoded and always contain non-digit characters, so the practical risk of -F type-coercing an ID to an integer is zero today — but the stated rationale in the comment above (line 262-264) recommends -f for String-typed variables, and ID values serialize the same way as String. This aligns the code with the documented pattern. Closes #44. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: tighten doc-only allowlist — remove *.txt, PR_TEMPLATE; add images Three allowlist adjustments in one commit — they share the same case block and ship together to avoid rewriting it three times. 1. Remove *.txt (resolves #48). Matched dependency manifests like requirements.txt, constraints.txt, packages.txt, which are code-adjacent and deserve review on change. `.txt` is rarely used for prose in modern repos; README/docs are conventionally .md. If a consumer genuinely has prose .txt files, they'll get a cheap review — the failure mode of skipping a requirements.txt change is significantly worse. 2. Remove .github/PULL_REQUEST_TEMPLATE.md (resolves #49). Templates can embed required security checklists, reviewer sign-offs, or label instructions. A PR that removes a required security checklist should not sail through unreviewed. ISSUE_TEMPLATE/* stays in the allowlist — those are user-facing forms with lower enforcement significance. Note: this required reordering the case statement — the explicit NON-DOC exclusion for PULL_REQUEST_TEMPLATE.md / CODEOWNERS / dependabot.yml now comes FIRST, because they'd otherwise match the general *.md / *.yml patterns that follow. The CODEOWNERS and dependabot.yml rules were already correct (they don't match *.md or the ISSUE_TEMPLATE glob) but collecting them into the same explicit exclusion block documents the "security-critical meta file" category cleanly. 3. Add image assets (resolves #45). *.png, *.jpg, *.jpeg, *.gif, *.svg, *.webp, *.ico, *.bmp. README screenshot updates, docs figures, and logo swaps don't need a paid review — they can't meet any BLOCK criterion (no runtime behavior). Conservative set — excludes *.pdf, *.psd, and other heavier binary formats. No functional change to the rest of the workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: detect renames in doc-only classification via files API Closes #46. Under the previous implementation, `gh pr diff --name-only` returned only the destination paths for renamed files, so a rename from `src/foo.py` → `docs/foo.md` would appear as only `docs/foo.md`, classify as doc-only, and skip review — even though the diff contains significant code deletion at the source path. Switch to the `repos/{owner}/{repo}/pulls/{pr}/files` API and emit the UNION of `previous_filename` (non-null when renamed or copied) and `filename` for every entry. The doc-only check then requires every pre-rename AND post-rename path to match the allowlist — closing the blind spot. For non-renamed files `previous_filename` is null and filtered by the jq `select(. != null)` clause, so non-rename behavior is unchanged. Verified against PRs #39, #47, #52 (all modified-only) — output is identical to the prior `--name-only` approach. An actual rename case wasn't available in recent history to test empirically, but the jq emits `previous_filename, filename` separately so the case is covered by construction: if ANY of those paths is non-doc, the final classification flips to non-doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: fall back to github.sha in SHA marker for non-pull_request triggers Closes #50. The marker previously interpolated `github.event.pull_request.head.sha`, which is only populated when the reusable workflow is called from a `pull_request` caller. A consumer that invokes it from `workflow_dispatch`, `push`, or another trigger would produce: <!-- claude-blocking-review sha= run=12345 --> with an empty `sha=` value. Downstream consumers that filter by SHA (e.g. post-push status scrapers) would then see every review for that commit as "unknown SHA" and fall through to phantom-finding behavior. Use the GitHub Actions expression `||` fallback so the marker always has a non-empty SHA. The operator returns the first truthy operand — empty strings from unpopulated nested paths like `github.event.pull_request.head.sha` evaluate falsy, so the fallback to `github.sha` fires on non-PR triggers. Behavior by trigger: - pull_request (this repo's self-review): PR head SHA, unchanged. - workflow_dispatch / push / schedule: the triggering commit SHA, which is the closest analogue to "what this review is reviewing." - scheduled / manual runs with no associated commit: github.sha is still populated with the workflow file's commit SHA, so the marker has a value rather than being empty. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Code Bot <claude-code@smartwatermelon.github> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three coordinated CI-cost reductions targeting failure modes documented in the 2026-04-17 AAR:
*.md,LICENSE,CHANGELOG,.gitignore,docs/,.github/ISSUE_TEMPLATE/**,.github/FUNDING.yml) and short-circuits the review withVERDICT: SKIPPED. PR bodies like kebab-tax#1162 (doc-only, hit 3× flakes this week) would cost $0 instead of 3× timeout-out runs.<!-- claude-blocking-review sha=<sha> run=<id> -->). Downstream scrapers can filter to the current HEAD. Fixes the post-push-status.sh phantom-findings issue seen on kebab-tax#1165.minimizeCommentGraphQL mutation (classifier:OUTDATED). Keeps the PR conversation tidy across iterations without breaking the rendered review body.Intentional scope boundary
.github/CODEOWNERSand.github/dependabot.ymlare explicitly excluded from the doc-only allowlist. They look meta but have real security impact (approval authority, dependency sourcing). Changes there should still get a Claude review. See commit0b45b33for the rationale.What this PR does NOT change
claude-review / run-review).printf/cat/gh pr commentcommands that already match the existing prefix allowlist.Expected cost impact
Commits
7e5d8f3— feat: add doc-only skip, SHA marker, and prior-comment collapse0b45b33— fix: exclude CODEOWNERS and dependabot.yml from doc-only allowlist8584ec1— fix: uselast: 100and explicit-string GraphQL varsTest plan
comments(last: 100)query verified against PR chore: restore self-applying caller for dogfooding + required status check #41OUTDATEDclassifier verified via GraphQL introspectionFollow-ups (non-blocking, filed during local review)
-fvs-Fusage for GraphQL ID variable #44 — minor-fvs-Fconsistency on the minimize mutation's ID variable*.png,*.jpg,*.svg) so README screenshot-only PRs also short-circuit--name-only)Closes
comments(first: 100)vslast: 100-Fflag passes string args that could be type-coerced by gh api #43 —-Ftype coercion on owner/name strings🤖 Generated with Claude Code