Skip to content

feat: Improve code review instructions - #146

Open
larseggert wants to merge 2 commits into
mainfrom
feat-better-code-review
Open

larseggert wants to merge 2 commits into
mainfrom
feat-better-code-review

Conversation

@larseggert

Copy link
Copy Markdown
Collaborator

And report cost.

Copilot AI balanced review requested due to automatic review settings September 18, 2026 08:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread claude-review/action.yml Outdated
Comment thread claude-review/prompt.md Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-review skill, reached through an unpinnable anthropics/claude-code-action@v1. If that skill is renamed or its high argument changes meaning, step 3 becomes a no-op and the run still reports success — the same silent-degradation failure mode the Agent/Task comment in action.yml guards against. Consider a post-run assertion that a Skill(code-review) invocation appears in claude-execution-output.json, next to the existing denial check.
  • Step 1 duplicates the workflow gate. .github/workflows/claude-review.yml already filters draft and user.type == 'Bot' (Renovate, Dependabot), and pull_request_target never fires on closed. 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_call consumer inherits 10.00 on 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_SUMMARY makes tuning possible without opening logs, at one extra line.

Comment thread claude-review/action.yml Outdated
Comment thread claude-review/action.yml
Comment thread claude-review/action.yml Outdated
Comment thread claude-review/prompt.md
Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 09:19
@larseggert
larseggert force-pushed the feat-better-code-review branch from e0e5776 to c221624 Compare September 18, 2026 09:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread claude-review/action.yml Outdated
Comment thread README.md Outdated
@larseggert
larseggert force-pushed the feat-better-code-review branch from 4966fc0 to 0c2c4be Compare September 18, 2026 09:21
Copilot AI review requested due to automatic review settings September 18, 2026 09:21

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 inside anthropics/claude-code-action@v1, which action.yml:146-147 notes 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.

Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated
Comment thread README.md Outdated
Comment thread claude-review/prompt.md Outdated
Comment thread README.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 waive error_max_budget_usd. Run the Claude step with continue-on-error, run this check with always(), 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

Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 inside anthropics/claude-code-action@v1, which action.yml:146-147 records 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 to action.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-action fails its own step on a non-success result, the if: steps.trust.outputs.trusted == 'true' condition still carries an implicit success(), 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-error on the Claude step plus a decision from steps.claude.outcome is the robust shape.

  • The 👎 documentation mismatch is already raised on README.md by both Copilot and my previous review; not repeating it inline.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md
Comment thread claude-review/prompt.md Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 10:19
@larseggert
larseggert force-pushed the feat-better-code-review branch from 0c2c4be to 3843578 Compare September 18, 2026 10:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread claude-review/action.yml Outdated
Comment thread .github/workflows/claude-review.yml Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.sh would make it shellcheck-able and unit-testable against a recorded claude-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 — a result entry exists and .total_cost_usd is 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.1 comment in action.yml and the prose in prompt.md step 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.

Comment thread claude-review/action.yml Outdated
Comment thread .github/workflows/claude-review.yml Outdated
Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 10:47
@larseggert
larseggert force-pushed the feat-better-code-review branch from 3843578 to 6cf0f3a Compare September 18, 2026 10:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread claude-review/action.yml Outdated
Comment thread claude-review/action.yml Outdated
Comment thread claude-review/action.yml Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 second prompt block) would drop the Read(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.yml has no successful execution on this branch, and the new step is ~35 lines of bash/jq/GraphQL whose failure modes (fail paths, 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.

Comment thread claude-review/action.yml Outdated
Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/action.yml Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 11:46
@larseggert
larseggert force-pushed the feat-better-code-review branch from 6cf0f3a to 2041d4b Compare September 18, 2026 11:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 APPROVE and REQUEST_CHANGES as well as COMMENT; 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 sends COMMENT.
          --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

Comment thread claude-review/prompt.md Outdated
Comment thread claude-review/prompt.md Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread README.md
Comment thread claude-review/prompt.md
Comment thread claude-review/action.yml Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 13:11
@larseggert
larseggert force-pushed the feat-better-code-review branch from 1ca8b5b to 559e792 Compare September 18, 2026 13:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread claude-review/action.yml

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-error is not in the documented runs.steps[*] schema for using: composite. Since the README now promises a budget cap passes the job, and !cancelled() alone already defeats the implicit success() 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; --slurpfile removes the ARG_MAX exposure; the zero-finding escape hatch, the error_max_budget* prefix match, delete_pending_pull_request_review, and the stale README concurrency text are all in.

Comment thread claude-review/prompt.md Outdated
Comment thread README.md Outdated
Comment thread claude-review/action.yml Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 13:35
@larseggert
larseggert force-pushed the feat-better-code-review branch from 559e792 to 40d89e3 Compare September 18, 2026 13:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread claude-review/action.yml
Comment thread claude-review/action.yml Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_review has no output the action reads, and REST has no "the review I just made" selector — so Submit review has to guess from state, which is where three of the four comments below come from. ReportFindings is already allowlisted: having Claude emit findings to a file and having the action do create_and_submit_pull_request_review in one call removes the guess, the orphan class, and the prompt's delete-first dance. That is the shape the earlier "post findings from ReportFindings output" note pointed at; this PR did the submit half but kept the fragile identification half.

  • Three of four new defects are in bash/jq that no test touches. Submit review joins Fetch review history and Check review outcome at ~180 lines of untested shell whose failure paths are reachable only on a live PR — and --paginate+--jq semantics bit this file once already (the removed gh api allowlist nit). A fixture test over canned gh JSON, or at minimum shellcheck on extracted .sh files, would have caught the multi-line ID statically. Still no green run on this branch.

Comment thread claude-review/action.yml Outdated
Comment thread claude-review/action.yml Outdated
Comment thread claude-review/action.yml Outdated
Comment thread claude-review/action.yml
Copilot AI review requested due to automatic review settings September 18, 2026 13:56
@larseggert
larseggert force-pushed the feat-better-code-review branch from 40d89e3 to e485d27 Compare September 18, 2026 13:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread claude-review/action.yml Outdated
Comment thread claude-review/action.yml Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.yml is not linted. The actionlint job runs actionlint with no path arguments, so it only covers .github/workflows/*; composite action manifests are outside its default glob, and prettier --check '**/*.yml' does not look inside block scalars. This PR adds ~150 lines of non-trivial bash/jq to that file — pagination, GraphQL, sharding, log forensics — with no shellcheck and no test. Extracting each run: body into claude-review/*.sh and passing those paths to actionlint (or shellcheck directly) 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 the PR_NUMBER expression shareable.
  • The code-review skill is load-bearing but the coupling is undeclared. prompt.md step 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.

Comment thread claude-review/action.yml
Comment thread claude-review/action.yml
Comment thread claude-review/action.yml Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 14:13
@larseggert
larseggert force-pushed the feat-better-code-review branch from e485d27 to 026f39b Compare September 18, 2026 14:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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-review workflow stops when Claude has already commented on the PR. On a later synchronize run, 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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread claude-review/action.yml Outdated
Comment thread claude-review/action.yml Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 14:34
@larseggert
larseggert force-pushed the feat-better-code-review branch from 026f39b to 65b3b08 Compare September 18, 2026 14:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment on lines +11 to +13
# Cancelling would orphan a pending review.
concurrency:
group: claude-review-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
Comment thread README.md
Comment on lines 82 to +84
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.
Comment thread claude-review/action.yml
gh api --method DELETE "$PR/reviews/$ID" >/dev/null 2>&1 || true
exit 0
fi
BODY="_No whole-change observations._"

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread claude-review/action.yml
Comment on lines +263 to +264
--allowedTools "Read(/${{ github.workspace }}/**)" "Glob(/${{ github.workspace }}/**)" "Grep(/${{ github.workspace }}/**)"
--allowedTools "Read(/${{ runner.temp }}/review-history/**)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
--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/**)"

Comment on lines +11 to +13
# Cancelling would orphan a pending review.
concurrency:
group: claude-review-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# 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

Comment thread claude-review/action.yml
Comment on lines +292 to +294
# A COMMENT event needs a body.
case "$(tr -d '[:space:]' <<< "$BODY")" in
"" | NOREVIEWBODY)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants