feat: Improve code review instructions - #146
larseggert wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Broad write-capable API access and skipped correctness passes on subsequent runs must be addressed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves Claude review quality, suppression behavior, and cost visibility.
Changes:
- Adds structured review and suppression instructions.
- Enables subagents and review-thread API access.
- Raises and reports the review budget.
File summaries
| File | Description |
|---|---|
README.md |
Documents review behavior and cost reporting. |
claude-review/prompt.md |
Defines the new review workflow. |
claude-review/action.yml |
Expands tooling and reports execution cost. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Rewrites prompt.md into an ordered pipeline (gate → suppression ledger → code-review skill → domain pass → filter → post), widens the tool allowlist for subagents and gh api, doubles the budget default, and reports cost. No prior reviews on this PR, so nothing to re-check.
Whole-change observations:
- Upstream coupling. The correctness pass now depends on Claude Code's built-in
code-reviewskill, reached through an unpinnableanthropics/claude-code-action@v1. If that skill is renamed or itshighargument changes meaning, step 3 becomes a no-op and the run still reports success — the same silent-degradation failure mode theAgent/Taskcomment inaction.ymlguards against. Consider a post-run assertion that aSkill(code-review)invocation appears inclaude-execution-output.json, next to the existing denial check. - Step 1 duplicates the workflow gate.
.github/workflows/claude-review.ymlalready filtersdraftanduser.type == 'Bot'(Renovate, Dependabot), andpull_request_targetnever fires onclosed. Those bullets only bind direct composite-action users, and paying a model turn plus PR fetches to reach a decision the trigger can make for free inverts the PR's own cost goal. The lockfile/generated-only bullet is the part that genuinely needs the model. - Default budget doubling is a downstream cost change. Every
workflow_callconsumer inherits10.00on upgrade without touching their workflow; worth a release note. - Cost visibility.
Review cost $X of the $Y budget.lands in the job log only. Appending to$GITHUB_STEP_SUMMARYmakes tuning possible without opening logs, at one extra line.
And report cost.
e0e5776 to
c221624
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Budget-limit handling is skipped when the upstream action marks its step failed, and suppression documentation disagrees with the prompt.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
4966fc0 to
0c2c4be
Compare
There was a problem hiding this comment.
Every finding from my previous review is resolved in c2216244: the raw gh api allowlist entries are gone, ReportFindings is allowlisted, the budget subtype is prefix-matched, the cap no longer fails the job, step 5's drop list closes the de-duplication loop, and step 6 has the zero-finding escape hatch. Cost now also lands in $GITHUB_STEP_SUMMARY.
Whole-change observations:
-
The prompt pins an interface the action cannot pin. Step 2 names a specific MCP tool, its
method, its result fields (is_resolved,is_outdated) and its pagination protocol (perPage,after,pageInfo). That server ships insideanthropics/claude-code-action@v1, whichaction.yml:146-147notes cannot be pinned — so the prompt is one upstream rename away from being wrong, with no signal in the run. The inline comment is today's instance; the class is worth avoiding by stating the intent ("read every existing review comment and reply, including outdated ones") and letting the model pick the tool. -
Responding to Copilot on
prompt.md:47: I cannot confirm an "already reviewed" guard in the built-in skill, and its description documents no such behaviour, so I would not treat that as the defect here. The concrete step 3 problem is the missing target, which I have commented on separately. That said, Copilot's underlying concern is real in a different form: step 3 has no post-condition, so both an "already reviewed" bail-out and an empty-diff no-op leave the correctness pass silently skipped while the job stays green.
There was a problem hiding this comment.
🟡 Changes recommended
Budget exhaustion still fails the workflow, and documented downvote suppression is not implemented.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
claude-review/action.yml:215
- Budget exhaustion still fails the job. The upstream action marks non-success result subtypes as a failed step, so this later step is skipped by GitHub's implicit
success()guard and cannot waiveerror_max_budget_usd. Run the Claude step withcontinue-on-error, run this check withalways(), and explicitly re-fail every non-budget action failure.
# The action sets conclusion=failure for hard errors (is_error, non-success subtype).
# A budget cap is expected and warned about above, so it must not fail the job.
if [[ "$CONCLUSION" == "failure" && "$SUBTYPE" != error_max_budget* ]]; then
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
All findings from my earlier review are resolved: raw gh api is gone from the allowlist (and the --paginate nit with it), ReportFindings is allowlisted, the budget subtype is prefix-matched, a cap no longer fails the job, step 5's drop list closes the de-duplication loop, step 6 has the zero-finding escape hatch, and cost lands in $GITHUB_STEP_SUMMARY.
Whole-change observations:
-
Step 2 pins an interface this action cannot pin. It names a tool, a
method, result fields and a pagination protocol, all supplied by the github-mcp-server insideanthropics/claude-code-action@v1, whichaction.yml:146-147records as unpinnable. The inline comments show the mismatch is real today, not hypothetical. Stating the intent — "read every existing review comment and reply, including outdated ones, and each thread's resolution state where available" — survives upstream renames; naming tools does not. The same reasoning applies toaction.yml:165-173, where an allowlist entry for a tool the server does not expose is silently inert. -
Agreeing with Copilot on the outcome check: if
claude-code-actionfails its own step on a non-success result, theif: steps.trust.outputs.trusted == 'true'condition still carries an implicitsuccess(), so the whole check — cost report, budget warning and the new non-fatal branch — is skipped and the job is red anyway. Worth confirming against a real capped run before relying on the new branch;continue-on-erroron the Claude step plus a decision fromsteps.claude.outcomeis the robust shape. -
The 👎 documentation mismatch is already raised on
README.mdby both Copilot and my previous review; not repeating it inline.
There was a problem hiding this comment.
All seven findings from my previous review are resolved: raw gh api is gone from the allowlist, ReportFindings is granted, the budget subtype is prefix-matched, SUBTYPE is initialized and the cap is non-fatal, step 5 now drops prior-review duplicates, step 6 has a zero-finding exit, and step 2's pagination moved off gh api.
Agreeing with @Copilot on claude-review/action.yml:215: the outcome step's if carries an implicit success(), so when the upstream action fails its own step on a non-success result, the whole step is skipped — the cost report and the budget warning never run, and the new non-fatal cap path is unreachable. continue-on-error: true on the Claude step plus a steps.claude.outcome check here is the fix, and it's a prerequisite for the two README paragraphs this PR adds.
Architectural note: prompt.md now hard-codes assumptions about Claude Code and MCP internals — skill name and argument grammar, subagent fan-out, MCP tool names and their field shapes, an undocumented result subtype. Each drifts silently and degrades the review to a weaker pass with a green check. A periodic self-review against a PR with known defects, or asserting in CI that a review was actually posted, would surface that drift; today nothing does.
The budget default doubling to 10.00 raises the cap for every downstream caller of the reusable workflow, not just this repo.
0c2c4be to
3843578
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The available GitHub tools cannot fully implement the promised suppression ledger, and concurrency setup documentation is stale.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
Rewrites prompt.md from a checklist into a 6-step procedure (skip gate → suppression ledger → code-review skill → domain pass → scenario filter → post), adds cost reporting and budget-cap tolerance to the outcome check, doubles the default budget, and adds workflow concurrency.
Previously raised and now resolved: raw gh api removed from the allowlist; the non-existent pull_request_read, add_reply_to_pull_request_comment, get_repository_tree and label tools dropped; ReportFindings allowlisted; error_max_budget* matched as a prefix; the cap no longer fails the job; step 5's drop list now closes the de-duplication loop; the zero-finding escape hatch exists; code-review gets an explicit PR target; the 👎 rule is in the prompt, so README.md matches; SUBTYPE is pre-initialised.
Still open from earlier rounds, not re-raised inline: the scenario bar at lines 77-78 (wrong result, panic, hang, leak, or vulnerability) still discards the documentation, test, performance and public-API categories step 4 mandates, and > [!NOTE] at line 115 is still unreachable behind the "nothing reaches WARNING" gate at line 91. Responding to Copilot's comment on prompt.md:47: the concern is about the /code-review slash command's "already reviewed" guard, but step 3 invokes the skill via the Skill tool with an explicit target, which does not carry that guard — I read it as a non-issue.
Architectural notes:
- The outcome check is now ~45 lines of embedded jq. Extracting it to
claude-review/check-outcome.shwould make it shellcheck-able and unit-testable against a recordedclaude-execution-output.json; as embedded YAML it is neither. - Every signal in that check depends on undocumented shapes of
claude-execution-output.json(.subtype,.total_cost_usd,.permission_denials,.message.content[].name,.input.skill). An upstream rename degrades all of them to "emit no warning", indistinguishable from a clean run. Consider one assertion — aresultentry exists and.total_cost_usdis present — and warning when it does not, so drift surfaces instead of silently disabling the diagnostics. - Tool inventory is now asserted in two places that will drift independently: the
mcp__github__ names match github-mcp-server v0.17.1comment inaction.ymland the prose inprompt.mdstep 2. A denied-or-missing-tool warning in the outcome check (a missing MCP tool is not a permission denial today) would catch drift at runtime rather than by inspection.
3843578 to
6cf0f3a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Raw untrusted review text enables prompt injection, and large histories can break assembly.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
Moving the review-history fetch out of Claude's hands and into a fixed, non-steerable step is the right call — it removes the dead pull_request_read / add_reply_to_pull_request_comment allowlist entries, the MCP pagination ceiling, and the raw gh api write hole in one go.
Resolved since earlier reviews: budget-subtype prefix match, non-fatal budget cap via steps.claude.outcome, the zero-finding escape hatch in step 6, step 5's missing "already raised" drop rule, the dead NOTE level, and the README/prompt.md 👎 mismatch.
Still open from earlier threads, not re-raised here: continue-on-error in a composite step and the scenario bar excluding doc/test/perf/API findings.
Whole-change observations:
- The ledger crosses a file boundary for no gain. The step already has the assembled JSON; appending it to
steps.prompt.outputs.content(or a secondpromptblock) would drop theRead(file_path:…)allowlist entry, the "named above" indirection in the preamble, and the truncation class of bug entirely. The counter-argument is prompt size — but the projection the inline comment proposes makes the two comparable. - No green run.
.github/workflows/claude-review.ymlhas no successful execution on this branch, and the new step is ~35 lines ofbash/jq/GraphQL whose failure modes (failpaths, GraphQL--paginate,-F pr=typing) are only reachable at runtime. A throwaway PR against a fork before merging would be cheap insurance, given every consumer picks this up from@main.
6cf0f3a to
2041d4b
Compare
ddcb73b to
1ca8b5b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Review submission remains vulnerable to unsafe events, and history or file-list truncation can produce incomplete reviews.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
claude-review/action.yml:226
- Object-count sharding cannot guarantee readable files because each body remains one JSON line. One long comment can exceed
Read's line limit, and offsets cannot recover its truncated suffix, producing an incomplete suppression ledger. Serialize bodies into bounded chunks or precompute the ledger.
jq --arg k "$K" --argjson o $((i * PER)) --argjson n "$PER" '.[$k][$o:$o+$n]' "$FULL" \
> "$(printf '%s/%s-%02d.json' "$DIR" "$K" "$i")" || fail "shard $K $i"
claude-review/action.yml:266
- The submit tool accepts
APPROVEandREQUEST_CHANGESas well asCOMMENT; the later tripwire only detects misuse after GitHub has applied it. Malicious reviewed content can induce an approval before the job fails. Keep submission outside the model or expose an argument-restricted wrapper that always sendsCOMMENT.
--allowedTools mcp__github__create_pending_pull_request_review,mcp__github__add_comment_to_pending_review,mcp__github__submit_pending_pull_request_review,mcp__github__delete_pending_pull_request_review
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
Most of the earlier threads are addressed in the current head: raw gh Bash removed, pull_request_read/add_reply_to_pull_request_comment/get_repository_tree dropped, delete_pending_pull_request_review restored, budget subtype matched by prefix and made non-fatal, SUBTYPE pre-initialized, the per-review N+1 comment loop replaced by the flat /comments list, index poisoned on a short fetch, READ_SHARDS gated on SKILL_CALLS, the concurrency group namespaced, and README's 👎/settled wording now matches prompt.md.
Two prior findings are still open and not re-raised here: #4045893512 (continue-on-error on a composite-action step) and #4046457808 / #4046667896 (body still emitted as one physical line, so Read's per-line cap silently truncates long suppression replies). The first is the one thing a single green run would settle; there is still none on this branch.
Architecturally: step 2 makes reading the entire history mandatory and unbounded. Cost scales with comment count, is paid up front, and competes with the same $10 cap that funds the review — a long-lived PR can spend most of its budget re-reading its own history and then post a truncated review. The action already holds all the data and joins nothing; it could instead emit one derived record per comment — {id, path, line, user, suppressed, body}, with suppressed computed in jq from isResolved plus the 👎 count — and drop threads from the shards. That removes the databaseId join, the thread/comment count reconciliation, the OVER warning, and roughly a third of the bytes Claude must read, and makes the suppression rule deterministic rather than model-inferred. Replies saying "won't fix" would still need the model, but that is a much smaller slice.
1ca8b5b to
559e792
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Large review bodies can still be truncated, producing an incomplete suppression ledger.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Deterministic history fetch plus a computed suppressed flag is the right call — it removes the model's ability to be steered into skipping suppression, and it fixes the tool-availability problems earlier rounds hit.
Architectural observation: the trust boundary is now half-deterministic. Fetch review history computes suppression in jq, but the use of it — dedup, the 10-comment cap, COMMENT-only — stays prompt-level, and the five tripwires in Check review outcome only ::warning:: after the review has already posted. A degraded review still lands; the log just explains why afterwards. That is a defensible trade-off, but it means the ~90 lines of jq buy provenance, not enforcement. Worth asking whether the dedup itself could move into the action (emit the ledger as a list of already-raised (path, line, topic) tuples and drop the "do not re-raise" prose), or whether submit_pending_pull_request_review should be replaced by an action step that posts findings from ReportFindings output — that would make the COMMENT-only and cap guarantees real rather than advisory.
None of this is exercised by CI: there is no test for Fetch review history, so the fail() paths, the GOT/EXPECTED poisoning, the OVER nested-page warning, and the shard/index arithmetic are validated only by live runs on this PR. A fixture-driven test of that step (canned gh responses, assert the emitted index.json and shard set) would be cheap and would cover the branches a live run never hits.
Two notes on prior threads:
- #4045893512 (composite steps may not support
continue-on-error) still looks unresolved to me;continue-on-erroris not in the documentedruns.steps[*]schema forusing: composite. Since the README now promises a budget cap passes the job, and!cancelled()alone already defeats the implicitsuccess()skip, it is worth confirming against one real run before merge — a rejected manifest breaks every consumer at step 1. - Resolved since earlier rounds, for the record: 👎 suppression is now actually implemented and matches the README;
--slurpfileremoves theARG_MAXexposure; the zero-finding escape hatch, theerror_max_budget*prefix match,delete_pending_pull_request_review, and the stale README concurrency text are all in.
559e792 to
40d89e3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Pending-review handling can suppress or publish stale findings, and multipage review lookup can prevent submission.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
Deterministic submission is the right move — it makes the COMMENT-only guarantee real instead of prompt-level, and it retires the BAD_EVENT tripwire's need to exist. Resolved since my last review: the composite now gets continue-on-error + steps.claude.outcome, the group is namespaced, the capped readers are out of the allowlist, READ_SHARDS is guarded and gated, the README.md L226-230 concurrency text is fixed, and the allowlist-drift caveat is documented. The whole-line body truncation thread (#4046457808) now reads as deliberately declined by the Bodies stay whole comment at line 191; continue-on-error on a composite step (#4045893512) is still unconfirmed by a green run.
Whole-change observations:
-
The pending review is still the handoff, and it is not addressed.
create_pending_pull_request_reviewhas no output the action reads, and REST has no "the review I just made" selector — soSubmit reviewhas to guess from state, which is where three of the four comments below come from.ReportFindingsis already allowlisted: having Claude emit findings to a file and having the action docreate_and_submit_pull_request_reviewin one call removes the guess, the orphan class, and the prompt's delete-first dance. That is the shape the earlier "post findings fromReportFindingsoutput" note pointed at; this PR did the submit half but kept the fragile identification half. -
Three of four new defects are in
bash/jqthat no test touches.Submit reviewjoinsFetch review historyandCheck review outcomeat ~180 lines of untested shell whose failure paths are reachable only on a live PR — and--paginate+--jqsemantics bit this file once already (the removedgh apiallowlist nit). A fixture test over cannedghJSON, or at minimumshellcheckon extracted.shfiles, would have caught the multi-lineIDstatically. Still no green run on this branch.
40d89e3 to
e485d27
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The history permission rule is ineffective, and concurrent direct action runs can corrupt pending reviews.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
Moving the history fetch and the review submission out of the model and into fixed shell is the right call — neither the queries nor the review event can be steered by injected comment text any more, and the suppressed flag being computed in jq rather than inferred is a real improvement.
Two whole-change observations:
- The shell in
claude-review/action.ymlis not linted. Theactionlintjob runsactionlintwith no path arguments, so it only covers.github/workflows/*; composite action manifests are outside its default glob, andprettier --check '**/*.yml'does not look inside block scalars. This PR adds ~150 lines of non-trivialbash/jqto that file — pagination, GraphQL, sharding, log forensics — with noshellcheckand no test. Extracting eachrun:body intoclaude-review/*.shand passing those paths toactionlint(orshellcheckdirectly) would put the bulk of the new logic under a linter, and would make the duplicated pending-review lookup (lines 169 and 286) and the four copies of thePR_NUMBERexpression shareable. - The
code-reviewskill is load-bearing but the coupling is undeclared.prompt.mdstep 3 delegates the entire correctness pass to a skill that ships with the CLI, whose recipe, arguments, and output contract can change under a floating@v1. The three new tripwires in Check review outcome detect some of that after the fact and after the spend. Worth considering whether the domain pass in step 4 should be self-sufficient enough that a degraded skill invocation is a quality loss rather than a silent hole.
Resolved since earlier rounds, for the record: the 👎 criterion is now actually implemented; the orphaned-pending-review path is recoverable via the stale-review deletion; the multi-page --jq ID bug is fixed by the streaming select; the budget cap no longer fails the job; and the README's concurrency and allowlist-verification text now matches the code.
e485d27 to
026f39b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Subsequent reviews can skip correctness checks, while concurrent composite-action runs can delete active pending reviews.
Review details
Suppressed comments (2)
claude-review/action.yml:172
- This selects any pending review visible to the shared token, without proving it came from an interrupted run. The composite action has no concurrency guarantee, so overlapping direct invocations let the later run delete the earlier run's active review, breaking its subsequent comment and submission calls. Enforce per-PR serialization for composite callers or track ownership before deleting.
# Clear an interrupted run's pending review: any pending review after this is ours.
STALE=$(gh api --paginate "$PR/reviews" --jq '.[] | select(.state == "PENDING") | .id' 2>/dev/null | tail -n1 || echo "")
if [[ -n "$STALE" ]]; then
echo "Deleting pending review $STALE left by an earlier run."
gh api --method DELETE "$PR/reviews/$STALE" >/dev/null 2>&1 || true
claude-review/prompt.md:54
- The built-in
code-reviewworkflow stops when Claude has already commented on the PR. On a latersynchronizerun, that skips the general correctness pass, so a newly introduced non-domain bug can go unreviewed. Require a fallback correctness pass when the skill exits for prior-review state; the suppression ledger should remove duplicates without skipping new changes.
Invoke the `code-review` skill with the pull request number named above and `high` as its
arguments. The skill's recipe reaches for `git diff` and `gh`; neither is available here — the
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Two new points inline; most of what I'd otherwise flag is already on the page from earlier rounds, so I've left those alone.
Resolved since the last round (noting per the review instructions): the budget subtype now uses a prefix match (was #4045280953); step 5's drop list now closes the de-duplication loop (was #4045282435); README.md:247 no longer says concurrency is the caller's job (was #4046916085); the Read(file_path:…) rules are now path rules.
Architecture. action.yml is now ~250 lines of embedded bash doing GraphQL assembly, sharding, and post-hoc log forensics, none of it reachable by a test. Moving the three run: blocks into claude-review/*.sh would make them shellcheck- and bats-testable, and would let the Check review outcome jq filters be exercised against a captured claude-execution-output.json fixture. As it stands a jq typo in a tripwire is only discovered on a live $10 run — and there is still no green run on this branch, so none of the new bash has executed end to end.
Tripwire policy. Six of the seven new checks in Check review outcome emit ::warning:: and nothing else. Warnings on a green job are not read. Worth deciding per check whether it should fail the job (skill skipped, partial ledger) or be dropped; as a set they mostly add surface without adding signal.
Remaining gap from an earlier round. #4046680695 (unguarded jq reddening a job that already posted) was mostly addressed — USES, COST, SUBTYPE, DENIALS all default now — but RESULT=$(jq … "$LOG" 2>/dev/null) on line 320 still has no || echo '{}', and GOT/OVER in the fetch step are likewise unguarded under -e.
Prompt/implementation coupling. prompt.md now encodes runtime facts — the shard layout, which tools exist, that submission is external. Each is duplicated in action.yml (allowlist, INDEX, the --append-system-prompt) and in README.md, with nothing keeping the three in sync; several of the findings in this PR's history are exactly that drift. A single generated source for the paths at least would cut it down.
026f39b to
65b3b08
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent and stale runs can mishandle pending reviews, and documented optional review bodies do not match runtime behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| # Cancelling would orphan a pending review. | ||
| concurrency: | ||
| group: claude-review-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} |
| The easiest way to use this is to copy [`.github/workflows/claude-review.yml`](.github/workflows/claude-review.yml) | ||
| into your repository — it includes the trigger and permission gating. Add a | ||
| [concurrency group](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/control-the-concurrency-of-workflows-and-jobs) | ||
| and an `ANTHROPIC_API_KEY` secret and you're done. | ||
| into your repository — it includes the trigger, permission gating and a concurrency group. Add an | ||
| `ANTHROPIC_API_KEY` secret and you're done. |
| gh api --method DELETE "$PR/reviews/$ID" >/dev/null 2>&1 || true | ||
| exit 0 | ||
| fi | ||
| BODY="_No whole-change observations._" |
There was a problem hiding this comment.
Substantial rework: a bash-side review-history fetch feeding a "suppression ledger", action-side review submission with a hard-coded COMMENT event, a much narrower tool allowlist, post-hoc tripwires, and a rewritten prompt.md. The tightened allowlist (no Bash, no submit/approve tools, no create_inline_comment) is the right direction — the injection surface is now genuinely small.
Architectural concern. The ledger's correctness rests entirely on the model voluntarily reading every shard and honouring the count fields; Check review outcome can only warn after the review is already posted. Consider computing the already-raised set in bash instead — e.g. path + line + a digest of each thread's first comment line, plus resolved — and handing Claude a compact list rather than whole bodies. That makes suppression deterministic, removes the sharding/truncation protocol entirely, and stops per-run token cost from growing with history length (this PR already has 71 comments; the cost of reviewing it is now partly a function of how much it has been reviewed).
Responding to @Copilot's continue-on-error notes (and the earlier bot comment on the same line): agreed, and worth confirming against a real run before merge, because the README's new "hits the cap … but still passes" contract and both downstream steps' reachability depend on it. continue-on-error is not in the documented composite-action step schema (metadata syntax); if the runner rejects the key the whole manifest fails, and if it silently ignores it Submit review's steps.claude.outcome != 'skipped' guard still passes but the job is red with the review posted. An alternative needing no runner support: keep the hard failure and move the budget-cap tolerance into Check review outcome only, documenting the job as red-but-posted on a cap.
Previously raised points that look addressed at 65b3b08: mcp__github__pull_request_read is gone from the allowlist; error_max_budget* is now a prefix match; the multi-page --jq problem on /reviews is handled with tail -n1; 👎 suppression is out of both the README and the prompt; get_pull_request_files pagination is now mandated; and the README no longer tells copiers to add their own concurrency group.
One small doc drift: the README says "the one exception is a blocking correctness or security defect with new evidence", but prompt.md:92-94 now has a second one (a defect the diff reintroduces after it was fixed).
| --allowedTools "Read(/${{ github.workspace }}/**)" "Glob(/${{ github.workspace }}/**)" "Grep(/${{ github.workspace }}/**)" | ||
| --allowedTools "Read(/${{ runner.temp }}/review-history/**)" |
There was a problem hiding this comment.
Warning
Leading / makes every path rule start with //, because github.workspace and runner.temp are already absolute.
These expand to Read(//home/runner/work/actions/actions/**) and Read(//home/runner/work/_temp/review-history/**). Unless the matcher normalises // to /, no Read/Glob/Grep is permitted: the suppression ledger cannot be read and step 4 cannot read surrounding code, and Check review outcome reports it only as a denial warning on a green job.
| --allowedTools "Read(/${{ github.workspace }}/**)" "Glob(/${{ github.workspace }}/**)" "Grep(/${{ github.workspace }}/**)" | |
| --allowedTools "Read(/${{ runner.temp }}/review-history/**)" | |
| --allowedTools "Read(${{ github.workspace }}/**)" "Glob(${{ github.workspace }}/**)" "Grep(${{ github.workspace }}/**)" | |
| --allowedTools "Read(${{ runner.temp }}/review-history/**)" |
| # Cancelling would orphan a pending review. | ||
| concurrency: | ||
| group: claude-review-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} |
There was a problem hiding this comment.
Warning
The orphaning rationale no longer holds, and queueing instead of cancelling multiplies spend at the new $10 cap.
Fetch review history (claude-review/action.yml:169-174) deletes any stale pending review at the start of every run, so a cancelled run is already recoverable. With cancel-in-progress unset it defaults to false: five pushes in quick succession queue five serial reviews of (eventually) the same head, each billed up to $10, and every run after the first suppresses its own findings against the previous run's comments and posts nothing.
| # Cancelling would orphan a pending review. | |
| concurrency: | |
| group: claude-review-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | |
| # A cancelled run's pending review is deleted by the next run's history fetch. | |
| concurrency: | |
| group: claude-review-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | |
| cancel-in-progress: true |
| # A COMMENT event needs a body. | ||
| case "$(tr -d '[:space:]' <<< "$BODY")" in | ||
| "" | NOREVIEWBODY) |
There was a problem hiding this comment.
Note
Sentinel match is exact, so any decoration around it leaks the sentinel into the posted review body.
tr -d '[:space:]' only strips whitespace. prompt.md:138 says "exactly NO REVIEW BODY", and Claude routinely wraps such tokens in backticks or adds a period; `NO REVIEW BODY` or NO REVIEW BODY. misses the case and becomes the review body readers see.
| # A COMMENT event needs a body. | |
| case "$(tr -d '[:space:]' <<< "$BODY")" in | |
| "" | NOREVIEWBODY) | |
| # A COMMENT event needs a body. | |
| case "$(tr -dc '[:alnum:]' <<< "$BODY" | tr '[:lower:]' '[:upper:]')" in | |
| "" | NOREVIEWBODY) |
And report cost.