Skip to content

NAS-142201 / 27.0.0-BETA.1 / Tune the review allowlist on the first round of denial evidence #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
william-gr merged 3 commits into from
Aug 17, 2026
Merged

NAS-142201 / 27.0.0-BETA.1 / Tune the review allowlist on the first round of denial evidence #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00469da
Tune the review allowlist on the first round of denial evidence
william-gr Aug 14, 2026
064c971
Scope the reviewer's Write to a scratch subdir, not .claude-review/
william-gr Aug 14, 2026
21fbce2
Withdraw the Write grant; the sandbox refuses it below --allowedTools
william-gr Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions .github/workflows/claude-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,14 @@
description: 'Checkout depth. Needs to cover the PR range for the diff.'
type: number
default: 10
extra-allowed-tools:
Comment thread
william-gr marked this conversation as resolved.
description: >-
Comma-separated permission rules appended to --allowedTools, for
repo-specific needs like `Bash(go vet:*)`. Anything that executes
repo code (make, go generate, package scripts) runs PR-controlled
code next to this job's write token — allow that knowingly.
type: string
default: ''
tooling-ref:
description: >-
Ref of this repository to take the schema, rubric and scripts from.
Expand Down Expand Up @@ -305,13 +313,17 @@
# nothing itself, hence gh and the inline-comment tool below.
#
# allowedTools is the auto-approve list, not the whole toolbox —
# Read/Grep/Glob need no approval. Listed: posting, the PR diff, and
# read-only git history. Nothing broader (`gh api`, arbitrary Bash):
# those can write or exfiltrate with this job's token.
# 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: the sandbox refuses workspace
# writes at a layer below --allowedTools (verified in-run on PR #8),

Check notice on line 319 in .github/workflows/claude-review.yml

View workflow job for this annotation

GitHub Actions / claude-review / Automatic PR review 

LOW: Comment blames a layer below --allowedTools for the refused write, but both denials point at the permission layer, so the no-grant-can-work conclusion is unproven and reads as settled.
# 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.
Comment on lines +318 to +322

@github-actions github-actions Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

  • Write to .claude-review/write-probe.txt → Claude requested permissions to write to … but you haven't granted it yet. That is the --allowedTools layer answering, not one below it. With no Write rule 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:

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

claude_args: |
--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:*)${{ inputs.extra-allowed-tools != '' && format(',{0}', inputs.extra-allowed-tools) || '' }}"
# No use_sticky_comment either — it only applies to tag mode.
prompt: |
REPO: ${{ github.repository }}
Expand All @@ -320,6 +332,14 @@
Please review this pull request using the guidelines below.
It should be already checked out in the current directory.

The authoritative change set is `gh pr diff` — the checkout's
`HEAD~1` is only the newest commit, not the PR range, and a review
of it silently misses every earlier commit in the PR.

CI results are `gh pr checks <number>` — never `--watch`, which

Check notice on line 339 in .github/workflows/claude-review.yml

View workflow job for this annotation

GitHub Actions / claude-review / Automatic PR review 

LOW: New CI paragraph never says what gh pr checks is for, so it does not replace the denied build attempts, and unscoped it invites findings on red checks the diff did not cause.
waits on every check while this review is itself one of them, and
so deadlocks until the job timeout.
Comment on lines +339 to +341

@github-actions github-actions Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

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


Put each finding that has a file and line on that line, as an
inline comment. Use one top-level comment for the summary and
anything not tied to a specific line, posted with:
Expand Down Expand Up @@ -347,7 +367,8 @@

`.claude-review/` holds the files this run generated for you. It is
tooling, not part of the change under review — do not review it, and
do not comment on it.
do not comment on it. You cannot write files anywhere — compose
comment bodies on stdin, as the posting command above does.

The tool you are running in also moves this repository's `.claude/`
directory aside to `.claude-pr/.claude/` before you start, so
Expand Down
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ either, so granting it in a caller has no effect on the token the job runs with.
| `skip-label` | `skip-claude` | |
| `timeout-minutes` | `20` | |
| `fetch-depth` | `10` | Must cover the PR range |
| `extra-allowed-tools` | `''` | Comma-separated permission rules appended to the reviewer's `--allowedTools`, e.g. `Bash(go vet:*)`. Empty by default on purpose: anything that executes repo code runs PR-controlled code next to the job's write token, so each repo opts in as its own recorded decision |
| `tooling-ref` | `master` | Ref this repo's `review/` assets come from; see below |

The secret is named, not inherited, because the repos call it different things
Expand Down
Loading