Skip to content

fix(claude-review): tell the reviewer it cannot run checks - #196

Merged
mashkovd merged 2 commits into
mainfrom
fix/review-prompt-static-only
Oct 18, 2026
Merged

mashkovd merged 2 commits into
mainfrom
fix/review-prompt-static-only

Conversation

@mashkovd

Copy link
Copy Markdown
Member

What and why

On mctl-academy#322 the reviewer read the repository's CLAUDE.md and tried npm ci and npm run lint:content. The narrowed --allowedTools refused both, and the reviewer then stopped with "Verdict: not submitted. I could not run the lint or tests": no inline comments and no review. REVIEW_PROMPT never says the review is static, so the model read the refusal as a reason to withhold judgement.

This adds one paragraph to REVIEW_PROMPT, which both review steps (primary and fallback token) share. The paragraph says:

  • the only tools are git diff and the two review endpoints;
  • the reviewer cannot install dependencies or run builds, tests, linters or repository scripts, even when the repository's CLAUDE.md lists them;
  • this is deliberate, because the job holds secrets and those commands would execute PR-head code;
  • CI runs the checks as separate required status checks;
  • it must always finish with the verdict.

Not changed: the tool allowlist. Allowing npm (or adding it to a base image) would execute package.json scripts and install hooks from the PR head in a job that holds CLAUDE_CODE_OAUTH_TOKEN and a pull-requests: write token. That is the PR-controlled-input case AGENTS.md rules out.

Callers pin this workflow by SHA, so each repository picks the change up on its next pin bump. mctl-academy is bumped in a follow-up.

Checks

  • actionlint .github/workflows/claude-review.yml clean, YAML parses
  • CI is green

🤖 Generated with Claude Code

https://claude.ai/code/session_01KMPGAe5mmsUMmPD9AZP8Zk

On mctl-academy#322 the reviewer read the repository CLAUDE.md, tried
npm ci and npm run lint:content, was refused by the narrowed tool
allowlist, and stopped without submitting a verdict. The prompt never
said the review is static, so a refused command read as a reason to
withhold judgement. Running PR-head scripts in a job that holds the
OAuth token would be unsafe, and CI already runs those checks.

Claude-Session: https://claude.ai/code/session_01KMPGAe5mmsUMmPD9AZP8Zk
@claude

claude Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 25s —— View job


Review: approved (0 P1, 0 P2, 1 P3)

  • Read the pinned diff fa82c11...53e22ad
  • Compared the prompt text with the --allowedTools on both review steps (lines 1028 and 1085)
  • Posted inline findings and the verdict, bound to 53e22ad

The new paragraph in REVIEW_PROMPT is plain text. It adds no ${{ }} expressions, so it creates no new injection surface. The tool allowlist is unchanged and still limited to git diff plus the two review endpoints for this PR. Leaving npm out keeps PR-head code from running in a job that holds CLAUDE_CODE_OAUTH_TOKEN and a write-scoped token. Both the primary and the fallback step pick up the paragraph through the shared env var.

P3, .github/workflows/claude-review.yml:330: "Your only tools are git diff and the two review endpoints" is true for shell commands only. The action's built-in file tools (Read, Grep, Glob) are still available, and the same paragraph asks the reviewer to read the files the diff touches. Saying "Your only shell commands are …" would stop the model from skipping that surrounding context. Fix this →

Comment thread .github/workflows/claude-review.yml Outdated
and may point at a newer commit while this invocation is running.
Files in the checkout and both git object IDs are pinned.

This is a static review. Your only tools are `git diff` and the two

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 — "Your only tools are git diff and the two review endpoints" is accurate for Bash, but the action's built-in file tools (Read/Grep/Glob) are still available and the paragraph later asks the reviewer to read the files the diff touches. Consider "Your only shell commands are …" so the model doesn't skip reading surrounding context.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 96ae908: the paragraph now says Read, Grep and Glob work on the pinned checkout and that the only shell commands are git diff and the two review endpoints.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No P1/P2 findings (1 P3). Good to merge.

@github-actions

Copy link
Copy Markdown

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit 53e22ad.

No significant issues found.

@claude

claude Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 24s —— View job


Review: approved (0 P1, 0 P2, 0 P3)

  • Read the pinned diff fa82c11...96ae908
  • Checked the prompt text against --allowedTools on both review steps (lines 1029 and 1086)
  • Submitted the verdict, bound to 96ae908

The new paragraph is static text with no ${{ }} expressions, so it adds no injection surface. Commit 96ae908 handles the earlier P3. The prompt now says "your only shell commands are git diff and the two review endpoints" and separately names Read, Grep and Glob. This matches the allowlist, which still covers only Bash(git diff:*) and the two PR-scoped gh api endpoints. The file tools are available too: this review used Grep. The primary step and the fallback step both pick up the text through the shared REVIEW_PROMPT env var. Keeping npm off the allowlist is right: PR-head code never runs in a job that holds secrets.

@github-actions

Copy link
Copy Markdown

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit 96ae908.

No significant issues found.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No P1/P2 findings (0 P3). Good to merge.

@mashkovd
mashkovd merged commit c0cf863 into main Oct 18, 2026
7 checks passed
@mashkovd
mashkovd deleted the fix/review-prompt-static-only branch October 18, 2026 09:14
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.

1 participant