polish: resolve doc-only allowlist + marker follow-ups (#44-#46, #48-#50) - #53
Merged
Merged
Conversation
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>
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>
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>
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>
|
Four changes in the diff:
VERDICT: PASS |
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
Six follow-up issues filed by local reviewers during the three main session PRs (#39, #47, #52). Bundled into one PR because they share the same files (doc-only
caseblock and the markerprintfline) and four small atomic commits are easier to read than a rewrite.Issues resolved
d42d068-F id→-f idin theminimizeCommentmutation. Consistency with surrounding-ffor String-typed vars.8324219*.txt(matchedrequirements.txt-class files), removePULL_REQUEST_TEMPLATE.md(can embed required security checklists), add image extensions (*.png,*.jpg,*.jpeg,*.gif,*.svg,*.webp,*.ico,*.bmp).81587efgh pr diff --name-onlyto the files API, so renamed files contribute both pre-rename and post-rename paths to doc-only classification. Asrc/foo.py→docs/foo.mdrename can no longer hide code removal behind a doc path.d311d3fBehavioral impact
requirements.txt,packages.txt,constraints.txtPRs: doc-only skip*.png): reviewedPULL_REQUEST_TEMPLATE.mdedits: doc-only skipgithub.shaminimizeComment(ID)typed via-F(auto)-f(explicit string) for consistency with adjacent codeWhat this PR does NOT change
max_turns/ timeout estimation unchanged.Test plan
requirements.txt→ non-doc ✓,README.md→ doc ✓,*.png→ doc ✓,PULL_REQUEST_TEMPLATE.md→ non-doc ✓,CODEOWNERS→ non-doc ✓Closes
Closes #44, #45, #46, #48, #49, #50.
Next step after this merges
Cut new
v1tag so consumer repos pick up the full stack (#39 grep, #47 doc-skip + marker + minimize, #52 narrow prompt, plus this one).🤖 Generated with Claude Code