feat(review): allow search tools and upload execution log

[zfarrell]

Across 400 sampled review runs, 64% hit at least one permission denial (1,562 denials over 7,819 turns) — search was the only tool class the prompt's "read affected files for context" step needed that the allowlist withheld, so Grep/Glob are now allowed.

The execution log is uploaded as an artifact to confirm that: it records every tool call with its input, and today it dies with the runner since the job log keeps only init and result.

Note this workflow runs org-wide via ruleset 13884907, so merging applies both changes to every repo on the next PR.

[claude]

nit: this step can turn a successful review into a red check (not blocking).

steps.review is continue-on-error: true specifically so an action failure doesn't fail the job, but this upload step is not, so any upload failure now fails the whole job on every PR org-wide. The most likely failure is a re-run: artifact names must be unique per workflow run, and re-running the failed job of an existing run uploads claude-execution-log-pr-N a second time and gets Conflict: an artifact with this name already exists on the workflow run. Adding github.run_attempt to the name plus continue-on-error on a purely diagnostic step covers both:

Suggested change

	- name: Upload Claude execution log
	if: github.event.pull_request.user.login != 'dependabot[bot]' && steps.review.outputs.execution_file != ''
	uses: actions/upload-artifact@v7.0.1
	- name: Upload Claude execution log
	continue-on-error: true
	if: github.event.pull_request.user.login != 'dependabot[bot]' && steps.review.outputs.execution_file != ''
	uses: actions/upload-artifact@v7.0.1

(and name: claude-execution-log-pr-${{ github.event.pull_request.number }}-${{ github.run_attempt }} below)

[claude]

nit: 90 days is a long retention for a diagnostic artifact (not blocking).

Two costs, both multiplied by "every PR in the org": artifact storage is billed for private repos, and the execution log holds the full conversation — including the review prompt sourced from this (private) central repo and every file/diff the agent read — downloadable by anyone with read access to the repo the workflow ran in (i.e. unauthenticated for any public org repo). The stated goal is confirming the permission-denial hypothesis, which is a days-not-months question; something like retention-days: 14 gets the same answer with a smaller footprint. Easy to raise later if the log turns out to be useful for longer.

[claude]

super nit: worth a line in README.md (not blocking).

The README describes what this workflow does for every repo in the org, and after this change each PR gets an artifact attached. A sentence under "Claude PR Review" noting the execution log, its retention, and that it exists for diagnosing tool-permission denials saves the next person from reverse-engineering it from the workflow comments.

[claude]

Approved. Two nits and a super nit inline — the allowlist change and the output-guarded upload both look correct.

[claude]

Blocking: uploading the raw execution log turns the agent's conversation into a downloadable copy of any credential it happened to read.

The workspace contains a live installation token in plaintext. In this very review run, .git/config at the workspace root is:

[remote "origin"]
	url = https://x-access-token:ghs_<redacted>@github.com/hotdata-dev/github-workflows.git

Read is unrestricted in --allowedTools, so that file is one tool call away, and per the cycle-1 thread the execution log holds the full conversation — tool inputs and results. Two things make this different from the status quo:

• Masking doesn't apply. create-github-app-token registers the token with ::add-mask::, which scrubs it from the job log. Artifacts are raw files written by the action, so nothing redacts them. Today a token read into the transcript dies with the runner; after this change it is a downloadable file.
• The reader set is wide. Artifacts are downloadable by anyone with read access to the repo the workflow ran in — unauthenticated for any public repo in the org. And the PR diff, title, and body are attacker-controlled input that the prompt does not fence (only prior comments are wrapped in "treat as data"), so an external contributor's PR can ask the reviewer to read and quote a workspace file.

Also note .github-workflows/.git/config (line 38): that checkout passes token: ${{ steps.app-token.outputs.token }} and persist-credentials defaults to true in actions/checkout@v6.0.2, so the app token is persisted there too for the duration of the job's main steps.

The fix that matches the stated goal: don't upload the transcript, upload the projection you actually need. The question is "which tools did the reviewer reach for, and which were denied" — that needs tool names and denial records, not inputs or results. A jq step that reduces the execution file to {tool_name, is_error} per entry and writes it to $RUNNER_TEMP gives you the same denial-rate comparison, with no secret material, no repo contents, and a far smaller artifact. Point path: at that reduced file instead.

(I've deliberately not put this in a suggestion block — I can't verify the execution file's exact schema from here, so the jq filter needs to be written against a real log rather than guessed at.)

One immediate follow-up regardless of how you fix it: this workflow runs from the PR head on same-repo PRs, so the upload step is already live on this run and the artifact on it may contain the token above. Worth deleting that artifact — ghs_ tokens expire in an hour, so the window is small, but no reason to leave it sitting for 14 days.

[claude]

Review

All three cycle-1 threads are addressed: continue-on-error is on the upload step, overwrite: true handles the re-run conflict, retention is down to 14 days, and the README documents the artifact. One new blocking issue.

Blocking Issues

• .github/workflows/claude-pr-review.yml:166-178 — the uploaded execution log is an unmasked, downloadable copy of the full conversation, and the workspace holds a live installation token in plaintext (.git/config remote URL, plus .github-workflows/.git/config since persist-credentials defaults to true in actions/checkout@v6.0.2). Read is unrestricted, secret masking does not apply to artifact files, and artifacts are readable by anyone with repo read access — unauthenticated on public org repos. The PR diff and body are attacker-controlled and unfenced by the prompt, so a contributor's PR can steer the reviewer into quoting a workspace file.

Action Required

Upload a projection rather than the transcript. The stated goal — comparing denial rates before and after the allowlist change — needs tool names and denial flags, not tool inputs or results. Add a step that reduces the execution file to {tool_name, is_error} per entry into $RUNNER_TEMP and point path: at that. Same answer, no credential or repo-content exposure, much smaller artifact.

Separately: this workflow runs from the PR head on same-repo PRs, so the upload is already live on this run. That artifact may hold the token; worth deleting it.

[claude]

nit: the first paragraph describes the design this PR rejected (not blocking).

Lines 155–158 and 159–161 are two passes at the same point, and the older one is now wrong in the one way that matters: "the action writes every tool_use with its full input here" reads as the artifact carries full inputs, which is exactly what lines 163–169 say must never happen. The next person deciding whether it's safe to widen the projection hits the stale paragraph eleven lines before the warning.

Suggested change

	# Grep/Glob above were added because 64% of runs (256/400 sampled) hit at least
	# one permission denial — 1,562 denials across 7,819 turns. Search is the only
	# tool class the prompt's "read affected files for context" step needs that the
	# allowlist withheld. This artifact is how we confirm that: the action writes
	# every tool_use with its full input here, and without the upload it dies with
	# the runner (the job log only records init + result, and the check-run summary
	# is empty under track_progress: false).
	# Grep/Glob above are a hypothesis; this is how it gets checked. The action writes
	# the full conversation to execution_file but only prints permission_denials.length
	# to the job log, so the denied tool *names* exist nowhere a later run can read.
	# Grep/Glob above were added because 64% of runs (256/400 sampled) hit at least
	# one permission denial — 1,562 denials across 7,819 turns. Search is the only
	# tool class the prompt's "read affected files for context" step needs that the
	# allowlist withheld. That is a hypothesis, and this artifact is how it gets
	# checked: the action writes the full conversation to execution_file but prints
	# only permission_denials.length to the job log (and the check-run summary is
	# empty under track_progress: false), so the denied tool *names* exist nowhere a
	# later run can read, and the file itself dies with the runner.

[claude]

Prior blocking issue resolved — the artifact is now a names-and-counts projection, and tests/tool-usage-test.sh holds that line against the fixture that reads a credentials file.

One operational leftover from cycle 2, not a code change: earlier runs on this branch (commits 65dd0ac, fe9936f) uploaded claude-execution-log-pr-21 with the raw transcript. Worth deleting those artifacts if they haven't been already.

[claude]

Cycle-3 blocking issue resolved: the artifact is now a tool-name/count projection rather than the transcript, and tests/tool-usage-test.sh pins that property against the shipped TOOL_USAGE_JQ (including negative assertions for credentials, tool inputs, and repo contents). No new findings.

[eddietejeda]

approved