NAS-142201 / 27.0.0-BETA.1 / Tune the review allowlist on the first round of denial evidence - #8
Conversation
From the denial notices and full transcript of a real run: - Write(.claude-review/**): the reviewer drafts its summary there — the scratch dir exists for exactly this, and the scope cannot touch the tree under review. - Bash(gh pr checks:*): the make/go-vet denials were the reviewer asking "does this compile?" — CI already answers that in jobs that do not hold this job's write token. - extra-allowed-tools input: repo-specific tools (go vet on Go repos) belong in the caller, as that repo's own recorded decision. - Prompt: point the reviewer at `gh pr diff` for the change set; the transcript shows it using `git diff HEAD~1`, which silently misses every commit but the newest. Read-only shell (grep, sed -n, ls) auto-approves in the sandbox, so none of it needs allowlisting; interpreters and builds stay denied.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Two findings, both LOW. Nothing blocking — this set does not fail the check. The MEDIUM from last round is gone, and gone the right way. What else checks out. This run is the change under test — The two LOWs, both inline. The comment explaining why there is no Unrelated to the diff, but it will block the merge: Last round's LOW on |
Review finding (HIGH): .claude-review/ also holds the tooling checkout, so Write(.claude-review/**) covered the gate script a later step executes with node — a reviewer steered by PR content could rewrite the gate to exit 0. Grant .claude-review/scratch/ instead, create it up front, and tell the prompt it is the one writable place. Also document extra-allowed-tools in the README input table (LOW).
| --model ${{ inputs.model }} | ||
| --json-schema '${{ steps.schema.outputs.json }}' | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),Bash(git diff:*)" | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr checks:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),Bash(git diff:*),Write(.claude-review/scratch/**)${{ inputs.extra-allowed-tools != '' && format(',{0}', inputs.extra-allowed-tools) || '' }}" |
There was a problem hiding this comment.
MEDIUM — Write(.claude-review/scratch/**) does not grant the reviewer that write, so the comment above (lines 315–321, "Write scoped to scratch/") and the prompt line at 365–366 (".claude-review/scratch/ — the one place you can write") both describe a capability that isn't there.
Verified from inside this very run, which is claude-review-self.yml calling this file by relative path — so the allowlist under test is exactly this line, with no extra-allowed-tools:
Writeto/home/runner/work/ux-github-workflows/ux-github-workflows/.claude-review/scratch/probe.md→Claude requested permissions to write to … but you haven't granted it yet. The rule did not match the path.echo probe > .claude-review/scratch/probe2.txt→Output redirection … was blocked. For security, Claude Code may only write to files in the allowed working directories for this session: '/home/runner/work/ux-github-workflows/ux-github-workflows'.
pwd is the workspace root and .claude-review/scratch exists (the mkdir -p at line 206 works), so neither failure is a missing directory.
Two separate things to settle, and the second is the one that decides whether this line can work at all:
- The rule form. A relative gitignore-style pattern is the documented shape for file-tool rules, but it did not match here; the absolute form (
Write(//home/runner/work/**/.claude-review/scratch/**)) or a leading**/are the candidates, and only a run can tell which. - The Bash redirect above was blocked for the workspace root too (
probe-root-delete-me.txt, same message), i.e. writes are refused at a layer below--allowedTools. If that layer also governs theWritetool, no allowlist entry fixes this and the honest change is to drop the promise rather than the rule.
Either way this is exactly the loop report-denied-tools.mjs was added for in #7: this run's log should carry a ::notice::reviewer was denied Write: … for probe 1, which is the evidence to tune against. Until a run shows the write landing, the prompt is telling every reviewer in every consumer repo to spend a turn on a tool call that comes back denied.
| --model ${{ inputs.model }} | ||
| --json-schema '${{ steps.schema.outputs.json }}' | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),Bash(git diff:*)" | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr checks:*),Bash(git log:*),Bash(git blame:*),Bash(git show:*),Bash(git diff:*),Write(.claude-review/scratch/**)${{ inputs.extra-allowed-tools != '' && format(',{0}', inputs.extra-allowed-tools) || '' }}" |
There was a problem hiding this comment.
LOW — Bash(gh pr checks:*) admits gh pr checks --watch, which blocks until every check on the PR concludes. The review job is itself one of those checks and is still running, so the call cannot return: it holds the turn until timeout-minutes (default 20) kills the job, the action emits no structured_output, and check-review-threshold.mjs fails closed on an unparseable result. A reviewer that only ever runs the bare form is unaffected — this is the wildcard's tail, not today's behaviour.
The other four gh/git wildcards have no equivalent blocking flag, which is why this one is worth noting when it joins them. There is no permission-rule shape that expresses "any flag except --watch", so if the flag is judged worth ruling out the lever is the prompt, not the allowlist.
Review finding, verified from inside its own run: the scoped Write rule never matched, and even Bash redirects into the workspace are refused by a layer under --allowedTools — no allowlist entry can deliver the write, so drop the rule and the prompt's promise. Comment bodies go via stdin, which already works. Also tell the reviewer never to use `gh pr checks --watch`: this review is one of the checks being watched, so it deadlocks until the job timeout.
| # read-only git history. No Write rule: the sandbox refuses workspace | ||
| # writes at a layer below --allowedTools (verified in-run on PR #8), | ||
| # so a grant here is a promise that layer breaks. Nothing broader | ||
| # (`gh api`, arbitrary Bash): those can write or exfiltrate with this | ||
| # job's token. |
There was a problem hiding this comment.
LOW — the withdrawal is right; the recorded reason for it is not what the run shows, and this comment is what stops anyone re-testing it.
Both probes, re-run under this ref:
Writeto.claude-review/write-probe.txt→Claude requested permissions to write to … but you haven't granted it yet. That is the--allowedToolslayer answering, not one below it. With noWriterule present that is the only answer it can give, so this run cannot tell whether a rule would have been honoured.printf probe > .claude-review/write-probe.txt→Output redirection … was blocked. For security, Claude Code may only write to files in the allowed working directories for this session: '/home/runner/work/ux-github-workflows/ux-github-workflows'. The path is inside the directory that message names as allowed. Same message verbatim for/tmp/write-probe.txt, where the reason does apply. So the workspace refusal is not the path check it announces itself as.
What #8 established is that Write(.claude-review/scratch/**) never matched — a rule-matching outcome, at or above --allowedTools, not beneath it. A pattern resolved against something other than the workspace root fits that evidence as well as an unreachable lower layer does, and it is the cheaper explanation.
Nothing misbehaves today: no rule, no writes, stdin works. The cost is only that "so a grant here is a promise that layer breaks" reads as settled, so the next person does not try Write(//home/runner/work/…/scratch/**) or an absolute pattern. Narrowing the claim to what was seen keeps the withdrawal and leaves the door open:
| # read-only git history. No Write rule: the sandbox refuses workspace | |
| # writes at a layer below --allowedTools (verified in-run on PR #8), | |
| # so a grant here is a promise that layer breaks. Nothing broader | |
| # (`gh api`, arbitrary Bash): those can write or exfiltrate with this | |
| # job's token. | |
| # allowedTools is the auto-approve list, not the whole toolbox — | |
| # Read/Grep/Glob and provably read-only Bash (grep, sed -n, ls) need | |
| # no approval. Listed: posting, PR reads (diff/view/checks), and | |
| # read-only git history. No Write rule: on PR #8 the scoped | |
| # `Write(.claude-review/scratch/**)` never matched (the tool still | |
| # answered "you haven't granted it yet"), and Bash redirects into the | |
| # workspace are refused as well, so the reviewer writes nothing and | |
| # the prompt tells it so. Whether an absolute-path pattern would | |
| # match is untested — do not re-promise a scratch dir without | |
| # verifying the grant lands. Nothing broader (`gh api`, arbitrary | |
| # Bash): those can write or exfiltrate with this job's token. |
| CI results are `gh pr checks <number>` — never `--watch`, which | ||
| waits on every check while this review is itself one of them, and | ||
| so deadlocks until the job timeout. |
There was a problem hiding this comment.
LOW — this paragraph does not do the job it was added for, and points the reviewer at output it has no rule for reading.
Per the commit message, Bash(gh pr checks:*) was allowed because "the make build / go vet denials were the reviewer asking 'does this compile?'" — CI already answers that without this job's write token. But the paragraph never says that. It states where CI results are and forbids one flag, so a reviewer that wants to know whether the change builds has nothing connecting its question to gh pr checks, and will still reach for make/go vet and be denied. The --watch half is the useful half and is correct.
The other side of naming it with no scope: gh pr checks 8 on this PR reports check-ticket … fail, which is caused by the PR title carrying no NAS-… reference and has nothing to do with the diff. A reviewer told to consult CI and not told what CI is for here can report that as a finding — and at MEDIUM it would fail the gate over something no code change caused.
Both are fixed by saying the purpose:
| CI results are `gh pr checks <number>` — never `--watch`, which | |
| waits on every check while this review is itself one of them, and | |
| so deadlocks until the job timeout. | |
| CI results are `gh pr checks <number>` — that is where "does this | |
| build/lint/pass" is already answered, by jobs that do not carry | |
| this job's write token, so read it instead of trying to build or | |
| test the repo yourself. Never `--watch`, which waits on every | |
| check while this review is itself one of them, and so deadlocks | |
| until the job timeout. A red check that the diff did not cause | |
| (a PR title without a ticket, an unrelated flake) is not a | |
| finding. |
bugclerk
changed the title
|
This PR has been merged and conversations have been locked. |
3 participants
Summary
First harvest from the denial notices added in #7, cross-checked against the full
execution_filetranscript of a real file-manager review run.What the transcript showed:
grep/sed -n/lscalls ran without being allowlisted — the sandbox waves through provably read-only commands. Denials were only compounds containingcd, interpreters, builds, and redirects. So no read-tool additions are needed, and the allowlist comment now says so.Changes:
Write(.claude-review/**)— the reviewer tried to draft its summary at exactly that path; the git-excluded scratch dir exists for this, and the scope can't touch the tree under review.Bash(gh pr checks:*)— themake build/go vetdenials were the reviewer asking "does this compile?". CI already computes that in jobs that don't carry this job's write token; reading check results gets the signal without the execution.extra-allowed-toolsinput — repo-specific appetite (e.g.Bash(go vet:*)on Go repos) is appended by the caller as that repo's own recorded decision, with the tradeoff stated in the input description.git diff HEAD~1as "the change", which silently misses every commit but the newest on multi-commit PRs. The prompt now namesgh pr diffas the authoritative range.