ci(claude-review): grant Task and pass --comment so reviews stop stubbing - #50
Merged
Merged
Conversation
…bing The code-review@claude-code-plugins command spawns subagents (Task tool) for its own review steps and gates all posting behind a --comment flag the workflow never supplied. Neither was in scope before: claude_args only allowlisted three gh Bash patterns, and the prompt invoked the slash command with no --comment. Both silently degrade to a run that does real work (turns, cost) and never calls gh pr comment. Closes #49
Correction to the previous commit on this branch (16e1887): adding Task to allowedTools was based on a wrong mechanism claim -- verified locally that claude --allowedTools excluding Task still lets a Task call through with zero permission denials, so Task was never actually gated the way Bash subcommands are. The real story is broader: this file was a hand-rolled, pre-gha-migration copy of the review job -- literally the 'qwt' template's original version, one of the three repos (serodynamics / qwt / rme) gha's own claude-code-review.yml says it was ported FROM. wai never migrated to consume its own canonical port, so it carried the original config gaps gha has since fixed across 15+ documented incidents: the plugin's own sub-agents need gh pr list/issue view/issue list/search (not just view/diff/comment), and having the agent itself call gh pr comment (gated behind --comment) is fragile compared to gha's approach of denying that tool and having the WORKFLOW post the review from the agent's final message, with an automatic same-run retry on a detected stub. Replaces the whole hand-rolled job with a thin caller matching this repo's own established pattern for consuming gha (check-links.yml, check-bibliography-dois.yml, check-non-standard-chars.yaml): same job id, same triggers, same pr_number interface claude.yml already dispatches through, the repo's two submodules checked out, and the qwt-specific Quarto/R prompt addendum carried over verbatim. Closes #49
Collaborator
Author
|
Self-review, since this PR's own review is skipped by design (self-mod guard -- confirmed the new gha-delegated version fires it identically, all 21 downstream steps in Verdict: clean, no findings. PR is ready. What I checked:
What I can't verify from inside this PR (same limitation as before, stated plainly rather than assumed): the actual review path -- gha's multi-agent fan-out, the retry-on-stub logic, the posted verdict -- only exercises once this merges, since the self-mod guard skips it here by design. |
|
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
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.
Closes #49
Correction to the first commit on this branch (16e1887): adding
Taskto
allowedToolswas based on a wrong mechanism claim. I verified locallythat
claude --allowedToolsexcludingTaskstill lets aTaskcallthrough with zero permission denials -- it isn't gated the way
Bashsubcommands are. That fix was a no-op.
What's actually going on: this file was a hand-rolled, pre-migration
copy of the review job -- literally the "qwt" template's original version,
one of the three repos (serodynamics / qwt / rme) that
gha's own
claude-code-review.ymlsays it was ported from. wai never migrated to consume its own canonical
port, so it carried the original config gaps gha has since fixed across
15+ documented incidents (gha#173, #185, #198, #201, #218, #219, #235,
#244, #312, #318, #381, #392, and cross-referenced incidents in
Lacaedemon/sparta and d-morrison/rme):
/code-reviewcommand declaresBash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh search:*)astools its 4 parallel sub-agents need -- wai's allowlist only granted
gh pr view/diff/comment. Missing tools means denied calls across thesub-agent fan-out.
gh pr comment(gated behind--comment,my second commit's fix) is fragile compared to gha's approach: gha
denies that tool outright and has the workflow post the review
from the agent's final text message instead, with an automatic
same-run retry when a stub (no verdict) is detected.
This PR now replaces the whole hand-rolled job with a thin caller, matching
wai's own established pattern for consuming gha
(
check-links.yml,check-bibliography-dois.yml,check-non-standard-chars.yaml):same job id (so the status-check context doesn't change), same triggers,
same
pr_numberinterfaceclaude.ymlalready dispatches through, therepo's two submodules checked out (
checkout-submodules: true), and theqwt-specific Quarto/R prompt addendum carried over verbatim.
As before: this PR edits
claude-code-review.ymlitself, so its ownreview is skipped by the self-mod guard (401s on workflow validation
until merged) -- gets a real review, from gha's own hardened workflow,
on the next PR once this lands.
actionlintand a YAML parse both passclean.
Worth a separate follow-up, not bundled into this PR:
claude.ymlalso calls
anthropics/claude-code-action@v1directly rather thandelegating to
Morrison-Lab/gha's reusableclaude.yml, and severalother workflows in this repo (
check-spelling.yaml,preview.yml,preview-deploy.yml,publish.yml,summary.yml) look like similarstale, hand-rolled duplicates of gha capabilities. Filed as #51 rather
than scope-creeping this one.