Skip to content

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

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
zfarrell merged 4 commits into from
Jul 31, 2026
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
65dd0ac
feat(review): allow search tools and upload execution log
zfarrell Jul 31, 2026
fe9936f
fix(review): make execution-log upload non-fatal and short-lived
zfarrell Jul 31, 2026
8e9815c
fix(review): upload tool-usage projection, not the raw transcript
zfarrell Jul 31, 2026
cc29162
docs(review): merge duplicated execution-log comment blocks
zfarrell Jul 31, 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
30 changes: 29 additions & 1 deletion .github/workflows/claude-pr-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,35 @@ jobs:

${{ steps.prompt.outputs.content }}
claude_args: |
--allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Read"
--allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Read,Grep,Glob"

@claude claude Bot Jul 31, 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.

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.


# 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).
# The review step is continue-on-error, so this runs even when it failed and
# wrote nothing; an empty execution_file would make upload-artifact fail on a
# missing `path`, hence the output guard rather than if-no-files-found alone.
# This step is diagnostic only and gates a required org-wide check, so it must
# never turn a passing review red: continue-on-error covers upload failures, and
# overwrite covers re-running a job in a run that already uploaded this name
# (v4+ artifacts are immutable, so a same-name upload is otherwise a conflict).
- 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
with:
name: claude-execution-log-pr-${{ github.event.pull_request.number }}
path: ${{ steps.review.outputs.execution_file }}
if-no-files-found: ignore
overwrite: true
# Long enough to compare denial rates before and after the allowlist change
# (~100 review runs/week org-wide), short enough that a full-conversation log
# of every PR in the org is not sitting in billable storage for a quarter.
retention-days: 14

@claude claude Bot Jul 31, 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.

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.


- name: Notify on review failure
if: github.event.pull_request.user.login != 'dependabot[bot]' && (steps.review.outcome == 'failure' || steps.review.outcome == 'cancelled')
Expand Down
5 changes: 5 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ Automated code review on every pull request using [claude-code-action](https://g

The review prompt lives in [`docs/claude-pr-review-prompt.md`](docs/claude-pr-review-prompt.md).

Each run attaches a `claude-execution-log-pr-<number>` artifact (14-day retention) holding the
agent's full tool-call record. The job log itself keeps only the start and end of the run, so this
artifact is the only way to see which tools the reviewer reached for — it exists to diagnose
permission denials against the workflow's `--allowedTools` list. Upload failures are non-fatal.

## Setup

Requires:
Expand Down