feat: narrow review prompt to BLOCK-only + lower max_turns floor - #52
Merged
Conversation
Shifts the CI reviewer from "full code review" duplicating local work to a narrow BLOCK/PASS gate. Every consumer repo of this reusable workflow already runs code-reviewer and adversarial-reviewer locally on pre-commit, plus formatters/linters/typecheckers in CI. The CI review was doing that work a second time on GitHub — paying per-turn for it — and raising max_turns repeatedly (commits 2c4a92f → 1abacdd → b0c12ba → 88c61e7, each raising the floor) just to keep up with a prompt that got wider each time somebody added a concern. Prompt changes: - Explicitly frames Claude as a BLOCK/PASS gate, not a reviewer. States that local reviewers cover style/naming/coverage/docs, so CI must NOT duplicate that work. - Removed "Code quality and best practices" and "Test coverage for changed behavior" bullets. Kept only the five BLOCK criteria (bug, reliability, security, async-error, data-loss). - Removed "Use the repository's CLAUDE.md for guidance" directive. CLAUDE.md is style/convention material — reading it costs turns and produces nothing useful for a BLOCK gate. On repos with large CLAUDE.md files (400+ lines is common), this alone can eat 1-2 turns every run. - Removed "When listing non-blocking observations, order them by maintenance impact" paragraph. In the narrow regime, Claude should NOT be listing non-blocking observations at all — local reviewers already covered them, and repeating them here dilutes the BLOCK signal. - Consolidated BLOCK criteria — they used to be listed twice (once as "please review this PR and provide feedback on..." and once under VERDICT INSTRUCTIONS). Now listed once, in-line with the scope directive. - Kept the EVIDENCE STANDARD block intact. It's the load-bearing part of the prompt — without it, Claude tends to issue BLOCKs on speculated regressions. This is where the prompt earns its keep. Turn estimation changes: - Base: 10 → 8 (lower fixed overhead in narrow mode) - Denominator: 150 → 200 (diff-driven scaling is milder) - Floor: 25 → 15 (narrow prompt needs less headroom) - Ceiling: 50 → 40 (large diffs still scale; upper bound tightened) - Caller override range unchanged (still 1-50 via Validate inputs) Floor rationale: a narrow-mode review is roughly read diff (1-2) + hazard scan (2-4) + write review (2) + verdict file (1) + 3-command post (3) = ~9-12 turns. 15 provides ~25% headroom. Consumer repos that hit the floor can override via max_turns input. Expected consumer impact: - Typical PASS runs should complete in ~10-15 turns, vs ~20-25 previously. Fewer turns = less wall-clock = less $. - PRs that previously produced long non-blocking-observation lists will now produce short PASS runs. That list-of-nits was mostly noise when local reviewers already caught or explicitly passed on those same items. - Real BLOCKs continue to produce a full, cited finding — that path is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Update both places that document the max_turns auto-estimation formula to match the new narrow-prompt values landed in the previous commit. - claude-blocking-review.yml header comment: reflect new formula (8 + lines/200, floor 15, ceiling 40) and explain the rationale — the prompt is BLOCK-only so the budget is intentionally tight. - README.md parameters table: same formula/range updates, plus a paragraph above the table explaining why the budget is tighter than callers might expect given their prior calibration. Also clarifies caller override range (1-50) which was not explicit before — distinct from the default estimation range (15-40). Closes #51. Non-functional change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No bugs, reliability regressions, security issues, or data-loss risks found. The prompt restructuring moves BLOCK criteria to the top as scope constraints and removes the redundant 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>
twistedmelonman
pushed a commit
that referenced
this pull request
Apr 18, 2026
Local reviewer flagged that the preceding comment overstated the role of the floor and ceiling clamps. With MAX_TURNS bounded to [15, 40] by the estimator (PR #52) the computed timeout is always 9-24 minutes, so neither the 4 min floor nor the 30 min ceiling ever activates in normal operation. They are defensive against future parameter drift, not active bounds today. Closes #54. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
twistedmelonman
added a commit
that referenced
this pull request
Apr 18, 2026
* docs: clarify that timeout uses 44% buffer (1.20 × 1.20), intentional Closes #26. The prior comment advertised "+20% buffer" but the math actually applies 1.20 twice — once in the MAX_TURNS calculation and once in TIMEOUT_SECS = MAX_TURNS * 30 * 1.20, compounding to 1.44 × the raw turns-times-30s baseline. The compounding is INTENTIONAL: the two buffers protect against different failure modes. - The turn-side buffer protects against Claude using more turns than estimated — "exceeded turn limit" in the step error. - The wall-clock-side buffer protects against infrastructure flakes (backend latency, network stalls) that don't consume turns but still eat wall time. Before the AAR 2026-04-17, a single session saw three consecutive infra-flake timeouts on kebab-tax#1162 — extra wall-clock slack reduces that rate. Closes the docs-vs-code mismatch flagged in #26 by documenting the compound factor and the rationale. No code change — the intentional extra slack stays. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: mark timeout 4m/30m bounds as defensive-only at current MAX_TURNS Local reviewer flagged that the preceding comment overstated the role of the floor and ceiling clamps. With MAX_TURNS bounded to [15, 40] by the estimator (PR #52) the computed timeout is always 9-24 minutes, so neither the 4 min floor nor the 30 min ceiling ever activates in normal operation. They are defensive against future parameter drift, not active bounds today. Closes #54. No code change. 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
Shifts the CI reviewer from "full code review" (duplicating local reviewers' work at paid per-turn cost) to a narrow BLOCK/PASS gate.
What changes
Prompt:
code-reviewer,adversarial-reviewer, linters, test suites) already cover style, coverage, docs, and organization.EVIDENCE STANDARD FOR RELIABILITY REGRESSION BLOCKsblock verbatim — that's the load-bearing part that keeps BLOCKs grounded.Turn budget:
108DIFF_LINES / 150DIFF_LINES / 200Floor rationale: a narrow-mode review is ~9-12 turns (read diff + hazard scan + write review + verdict file + 3-command post). 15 provides ~25% headroom.
Docs: header and README updated to match the new formula (closes #51).
Expected impact
Commits
c8ada19— feat: narrow review prompt to BLOCK-only + lower max_turns floor09b208d— docs: sync workflow header and README with narrow-prompt parametersCloses
Test plan
@v1picks this up; verify no reviews hit the new 15 floorNon-blocking follow-ups carried from #38→#47
Open issues filed during the session's local reviews that remain open:
-fvs-Fusage for GraphQL ID variable #44-f/-FID var consistency (trivial)*.txtmatchingrequirements.txt-style files (legit, worth a follow-up PR)PULL_REQUEST_TEMPLATE.mdin doc-allowlist (arguably risky)🤖 Generated with Claude Code