fix: match documented [skip-claude-review: reason] form in grep - #39
Merged
Merged
Conversation
twistedmelonman
pushed a commit
that referenced
this pull request
Apr 18, 2026
…s check Branch protection on main requires the `claude-review / run-review` status check, but no workflow in this repo produces it. The caller that used to produce it (.github/workflows/claude-code-review.yml) was removed in commit 52e688b as part of the v1 rename, and nothing was added back. Since then every PR (#33–#36, #39) has been BLOCKED on main's branch protection and only mergeable via `--admin`, losing both the enforcement and the dogfooding of the reusable workflow on its own changes. Add .github/workflows/self-review.yml, a minimal caller that: - Triggers on pull_request (opened/synchronize/ready_for_review/reopened) - Invokes the reusable workflow via LOCAL path (./.github/workflows/...) so PR branches review their proposed changes to the reusable workflow against themselves before release. A tag-pinned reference (@v1) would run the released version instead and miss bugs introduced in the PR. - Names the calling job `claude-review`, matching the required status check name `claude-review / run-review` in branch protection. - Passes repo-specific extra_instructions flagging the sensitive surfaces in this repo: shell-injection in PR-data interpolation, verdict contract stability, allowed-tools broadening, and the escape-hatch regex (which just bit us in #38). No functional changes to the reusable workflow itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
twistedmelonman
added a commit
that referenced
this pull request
Apr 18, 2026
…s check (#41) Branch protection on main requires the `claude-review / run-review` status check, but no workflow in this repo produces it. The caller that used to produce it (.github/workflows/claude-code-review.yml) was removed in commit 52e688b as part of the v1 rename, and nothing was added back. Since then every PR (#33–#36, #39) has been BLOCKED on main's branch protection and only mergeable via `--admin`, losing both the enforcement and the dogfooding of the reusable workflow on its own changes. Add .github/workflows/self-review.yml, a minimal caller that: - Triggers on pull_request (opened/synchronize/ready_for_review/reopened) - Invokes the reusable workflow via LOCAL path (./.github/workflows/...) so PR branches review their proposed changes to the reusable workflow against themselves before release. A tag-pinned reference (@v1) would run the released version instead and miss bugs introduced in the PR. - Names the calling job `claude-review`, matching the required status check name `claude-review / run-review` in branch protection. - Passes repo-specific extra_instructions flagging the sensitive surfaces in this repo: shell-injection in PR-data interpolation, verdict contract stability, allowed-tools broadening, and the escape-hatch regex (which just bit us in #38). No functional changes to the reusable workflow itself. Co-authored-by: Claude Code Bot <claude-code@smartwatermelon.github> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The escape-hatch grep at line 305 used `grep -qF '[skip-claude-review]'` (fixed-string, bare brackets), but every user-facing error message and the workflow header comment advertise the `[skip-claude-review: reason]` form. That form does not contain `[skip-claude-review]` as a substring — the `]` sits after the reason, not after `review` — so the grep never matched the documented marker. Users who followed the advice literally (like PR #1162 last session) watched their reruns fail identically, with no diagnostic pointing at the grep/message mismatch. The only working form was the bare token the docs never mentioned. Switch to an extended regex that matches both shapes: - [skip-claude-review] (existing behavior, unchanged) - [skip-claude-review: reason] (what every error message promises) - [skip-claude-review:] (degenerate, harmless) Tested locally against the match matrix from the AAR: MATCH : [skip-claude-review] MATCH : [skip-claude-review: doc-only diff, infrastructure flake] MATCH : [skip-claude-review:] NOMATCH: [skip-claude-review (unclosed/malformed) NOMATCH: [skip-claude-reviewfoo] (similar-looking prefix) NOMATCH: no marker here NOMATCH: skip-claude-review without brackets Keep all documentation/error messages on the `: reason` form — it's the better UX because it forces an audit trail in the bypass. Closes #38. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
twistedmelonman
force-pushed
the
claude/fix-skip-marker-grep-20260418
branch
from
April 18, 2026 19:52
a7f6d27 to
376a5ff
Compare
|
PR 39 Review: fix match documented skip-claude-review reason form in grep File: .github/workflows/claude-blocking-review.yml (around line 305) Summary Replaces fixed-string grep (-qF) with extended-regex grep (-qE) so the documented [skip-claude-review: reason] escape-hatch form actually fires the bypass. The old form silently failed for anyone using the documented syntax. Correctness The regex pattern is correct:
Non-matches confirmed: unclosed token without ] or : does not match; skip-claude-reviewfoo does not match. Bare-token backward compatibility is preserved. Shell-injection risk No injection risk. PR_BODY is fetched via gh and piped via stdin to grep, never interpolated into the pattern or shell command. Parsing-contract / consumer impact Escape-hatch logic is self-contained in one if-block. Verdict file and comment-posting contract are unchanged. Consumers on @v1 pick up the fix automatically. Non-blocking observations
Verdict Clean, targeted bug fix. Old behavior fully preserved, documented form now works, no security or reliability regression. VERDICT: PASS |
|
VERDICT: PASS |
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 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
grep -qF '[skip-claude-review]'at line 305 with extended regex\[skip-claude-review(\]|:)so both the bare token and the: reasonform the workflow advertises actually trigger the bypass: reasonform — it's the better UX (forces an audit trail in the bypass)Why now
Session on 2026-04-17 hit this on kebab-tax#1162: three consecutive infrastructure-failure runs on the same SHA, added
[skip-claude-review: infrastructure flake — …]to the PR body exactly as the error message instructs, reran → same failure because the grep never matched. Only the bare[skip-claude-review]form (undocumented) actually fired the bypass. Full report in the user's local AAR.Match matrix (verified locally)
-qF)-qE)[skip-claude-review][skip-claude-review: reason][skip-claude-review:][skip-claude-review(unclosed)[skip-claude-reviewfoo]Test plan
Claude Blocking Review) passeskebab-tax, etc.) pick up the fix automatically via@v1tag once a new tag is cutCloses
Closes #38.
🤖 Generated with Claude Code