From 65dd0ac68340cdfc573b83ae1a59ba2dc867a0c8 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 31 Jul 2026 13:32:35 -0700 Subject: [PATCH 1/4] feat(review): allow search tools and upload execution log --- .github/workflows/claude-pr-review.yml | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 001fdc9..4f481a5 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -147,7 +147,26 @@ 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" + + # 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. + - 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 + with: + name: claude-execution-log-pr-${{ github.event.pull_request.number }} + path: ${{ steps.review.outputs.execution_file }} + if-no-files-found: ignore + retention-days: 90 - name: Notify on review failure if: github.event.pull_request.user.login != 'dependabot[bot]' && (steps.review.outcome == 'failure' || steps.review.outcome == 'cancelled') From fe9936ff52c4e0bc2eaf3260bb81fe7e03344964 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 31 Jul 2026 13:41:21 -0700 Subject: [PATCH 2/4] fix(review): make execution-log upload non-fatal and short-lived --- .github/workflows/claude-pr-review.yml | 11 ++++++++++- README.md | 5 +++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 4f481a5..5bcf67a 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -159,14 +159,23 @@ jobs: # 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 - retention-days: 90 + 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 - name: Notify on review failure if: github.event.pull_request.user.login != 'dependabot[bot]' && (steps.review.outcome == 'failure' || steps.review.outcome == 'cancelled') diff --git a/README.md b/README.md index 77996a2..eecfc17 100644 --- a/README.md +++ b/README.md @@ -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-` 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: From 8e9815cf7d19f64424a3500b600795d072881e62 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 31 Jul 2026 14:00:36 -0700 Subject: [PATCH 3/4] fix(review): upload tool-usage projection, not the raw transcript --- .github/workflows/claude-pr-review.yml | 48 +++++-- .github/workflows/tests.yml | 3 + README.md | 14 ++- tests/fixtures/execution-log-clean.json | 45 +++++++ tests/fixtures/execution-log-denials.json | 132 ++++++++++++++++++++ tests/fixtures/execution-log-truncated.json | 22 ++++ tests/tool-usage-test.sh | 132 ++++++++++++++++++++ 7 files changed, 380 insertions(+), 16 deletions(-) create mode 100644 tests/fixtures/execution-log-clean.json create mode 100644 tests/fixtures/execution-log-denials.json create mode 100644 tests/fixtures/execution-log-truncated.json create mode 100755 tests/tool-usage-test.sh diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 5bcf67a..dad3fc9 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -156,25 +156,49 @@ jobs: # 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 + # 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. + # + # Never upload that file as-is. It holds every tool input and result, the runner has + # a readable git credential (checkout persists one via includeIf into + # $RUNNER_TEMP/git-credentials-*.config), Read is unrestricted, and ::add-mask:: + # scrubs the job log but not artifacts -- so a raw upload turns anything the + # reviewer happened to read into a downloadable file. Tool names and counts answer + # the allowlist question by themselves, so project to those and upload only that. + # The filter below emits no tool input and no result body by construction. + # + # The review step is continue-on-error so a failed review still reaches the notify + # step; empty execution_file means it wrote nothing, hence the output guard. Both + # steps here are diagnostic and gate a required org-wide check, so both are + # continue-on-error -- losing a metric must never turn a passing review red. + - name: Reduce execution log to tool usage + id: tool-usage continue-on-error: true if: github.event.pull_request.user.login != 'dependabot[bot]' && steps.review.outputs.execution_file != '' + run: | + # Kept as a single-line assignment so tests/tool-usage-test.sh can extract and + # exercise the shipped expression rather than a copy of it. + TOOL_USAGE_JQ='{tool_calls: ([.[]? | select(.type=="assistant") | .message.content[]? | select(.type=="tool_use") | .name] | group_by(.) | map({name: .[0], n: length}) | sort_by(-.n)), denials: (([.[]? | select(.type=="result")] | last // {}) | (.permission_denials // []) | map(.tool_name // "unknown") | group_by(.) | map({name: .[0], n: length}) | sort_by(-.n)), result: (([.[]? | select(.type=="result")] | last // {}) | {subtype, is_error, num_turns, duration_ms, total_cost_usd})}' + jq "$TOOL_USAGE_JQ" "$EXECUTION_FILE" > "${RUNNER_TEMP}/claude-tool-usage.json" + env: + # Via env, not a ${{ }} interpolation inside the script, so the path cannot be + # spliced into the shell command. + EXECUTION_FILE: ${{ steps.review.outputs.execution_file }} + + - name: Upload Claude tool usage + continue-on-error: true + if: github.event.pull_request.user.login != 'dependabot[bot]' && steps.tool-usage.outcome == 'success' uses: actions/upload-artifact@v7.0.1 with: - name: claude-execution-log-pr-${{ github.event.pull_request.number }} - path: ${{ steps.review.outputs.execution_file }} + name: claude-tool-usage-pr-${{ github.event.pull_request.number }} + path: ${{ runner.temp }}/claude-tool-usage.json if-no-files-found: ignore + # v4+ artifacts are immutable, so re-running a job that already uploaded this + # name is otherwise a conflict. 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. + # (~100 review runs/week org-wide); the question is days-old, not quarters. retention-days: 14 - name: Notify on review failure diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index c77e53d..cf625d6 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -18,3 +18,6 @@ jobs: - name: Review cycle counter run: tests/review-cycle-test.sh + + - name: Tool usage projection + run: tests/tool-usage-test.sh diff --git a/README.md b/README.md index eecfc17..d65439a 100644 --- a/README.md +++ b/README.md @@ -14,10 +14,16 @@ 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-` 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. +Each run attaches a `claude-tool-usage-pr-` artifact (14-day retention): tool call counts, +denied tool names, and the run's turn count and cost. It exists to diagnose permission denials +against the workflow's `--allowedTools` list, since the job log records only the number of denials, +never which tools were refused. + +The artifact is a projection of the action's execution log, never the log itself — that file is the +full conversation, and the runner holds a git credential the reviewer can read, which artifacts +(unlike job logs) would not mask. `TOOL_USAGE_JQ` in the workflow emits names and counts only, and +`tests/tool-usage-test.sh` asserts that tool inputs, tool results, and repository contents cannot +reach the artifact. Both the projection and the upload are non-fatal. ## Setup diff --git a/tests/fixtures/execution-log-clean.json b/tests/fixtures/execution-log-clean.json new file mode 100644 index 0000000..3aa7920 --- /dev/null +++ b/tests/fixtures/execution-log-clean.json @@ -0,0 +1,45 @@ +[ + { + "type": "system", + "subtype": "init", + "message": "Claude Code initialized", + "model": "claude-opus-5[1m]" + }, + { + "type": "assistant", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "tu_1", + "name": "Bash", + "input": { "command": "gh pr view 21" } + } + ] + } + }, + { + "type": "assistant", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "tu_2", + "name": "mcp__github_inline_comment__create_inline_comment", + "input": { "path": "README.md", "line": 15, "body": "nit: typo" } + } + ] + } + }, + { + "type": "result", + "subtype": "success", + "is_error": false, + "duration_ms": 41200, + "num_turns": 7, + "total_cost_usd": 0.21, + "permission_denials": [] + } +] diff --git a/tests/fixtures/execution-log-denials.json b/tests/fixtures/execution-log-denials.json new file mode 100644 index 0000000..9964794 --- /dev/null +++ b/tests/fixtures/execution-log-denials.json @@ -0,0 +1,132 @@ +[ + { + "type": "system", + "subtype": "init", + "message": "Claude Code initialized", + "model": "claude-opus-5[1m]" + }, + { + "type": "assistant", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "tu_1", + "name": "Bash", + "input": { "command": "gh pr diff 21" } + } + ] + } + }, + { + "type": "user", + "message": { + "role": "user", + "content": [ + { + "type": "tool_result", + "tool_use_id": "tu_1", + "content": "diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml" + } + ] + } + }, + { + "type": "assistant", + "message": { + "role": "assistant", + "content": [ + { "type": "text", "text": "Searching for the allowlist." }, + { + "type": "tool_use", + "id": "tu_2", + "name": "Grep", + "input": { "pattern": "allowedTools" } + } + ] + } + }, + { + "type": "user", + "message": { + "role": "user", + "content": [ + { + "type": "tool_result", + "tool_use_id": "tu_2", + "content": "Claude requested permissions to use Grep, but you have not granted it yet.", + "is_error": true + } + ] + } + }, + { + "type": "assistant", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "tu_3", + "name": "Grep", + "input": { "pattern": "retention-days" } + } + ] + } + }, + { + "type": "assistant", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "tu_4", + "name": "Read", + "input": { + "file_path": "/home/runner/work/_temp/git-credentials-82efe7dc.config" + } + } + ] + } + }, + { + "type": "user", + "message": { + "role": "user", + "content": [ + { + "type": "tool_result", + "tool_use_id": "tu_4", + "content": "[http \"https://github.com/\"]\n\textraheader = AUTHORIZATION: basic eC1hY2Nlc3MtdG9rZW46Z2hzX0ZBS0VUT0tFTkZPUlRFU1RT" + } + ] + } + }, + { + "type": "result", + "subtype": "success", + "is_error": false, + "duration_ms": 94238, + "num_turns": 19, + "total_cost_usd": 0.46520325, + "permission_denials": [ + { + "tool_name": "Grep", + "tool_use_id": "tu_2", + "tool_input": { "pattern": "allowedTools" } + }, + { + "tool_name": "Grep", + "tool_use_id": "tu_3", + "tool_input": { "pattern": "retention-days" } + }, + { + "tool_name": "Glob", + "tool_use_id": "tu_5", + "tool_input": { "pattern": "**/*.yml" } + } + ] + } +] diff --git a/tests/fixtures/execution-log-truncated.json b/tests/fixtures/execution-log-truncated.json new file mode 100644 index 0000000..666db0b --- /dev/null +++ b/tests/fixtures/execution-log-truncated.json @@ -0,0 +1,22 @@ +[ + { + "type": "system", + "subtype": "init", + "message": "Claude Code initialized", + "model": "claude-opus-5[1m]" + }, + { + "type": "assistant", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "tu_1", + "name": "Read", + "input": { "file_path": ".github/workflows/claude-pr-review.yml" } + } + ] + } + } +] diff --git a/tests/tool-usage-test.sh b/tests/tool-usage-test.sh new file mode 100755 index 0000000..081df2d --- /dev/null +++ b/tests/tool-usage-test.sh @@ -0,0 +1,132 @@ +#!/usr/bin/env bash +# +# Guards TOOL_USAGE_JQ in claude-pr-review.yml, which projects the Claude execution log +# down to the artifact the workflow uploads. The jq program is extracted from the workflow +# rather than copied, so the test exercises the shipped expression. +# +# Two jobs. First, the projection has to actually answer the question it exists for: the +# action prints only permission_denials.length to the job log, so the denied tool *names* +# live nowhere else and a change that drops them would be invisible until someone went +# looking for data that was never collected. +# +# Second, and the reason this file is worth more than its assertions: the input is the full +# conversation -- every tool input and every tool result. The runner has a readable git +# credential (checkout persists one via includeIf into $RUNNER_TEMP/git-credentials-*.config) +# and Read is unrestricted, so a transcript can contain a live token. ::add-mask:: scrubs the +# job log but not artifacts. The leak assertions below hold the line that this artifact +# carries names and counts only; widening the projection to "just the tool inputs too" is +# exactly the change that would quietly publish a token. + +set -euo pipefail + +cd "$(dirname "$0")/.." + +WORKFLOW=.github/workflows/claude-pr-review.yml + +TOOL_USAGE_JQ=$(sed -n "s/^ *TOOL_USAGE_JQ='\(.*\)'\$/\1/p" "$WORKFLOW") +if [ -z "$TOOL_USAGE_JQ" ]; then + echo "FAIL: no TOOL_USAGE_JQ='...' assignment found in $WORKFLOW" >&2 + exit 1 +fi +if [ "$(printf '%s\n' "$TOOL_USAGE_JQ" | wc -l)" -ne 1 ]; then + echo "FAIL: more than one TOOL_USAGE_JQ assignment in $WORKFLOW" >&2 + exit 1 +fi + +failures=0 + +# project -- run the shipped filter over a fixture +project() { + jq "$TOOL_USAGE_JQ" "tests/fixtures/$1" +} + +# expect_jq +expect_jq() { + local fixture=$1 expr=$2 want=$3 desc=$4 actual + actual=$(project "$fixture" | jq -c "$expr") + if [ "$actual" = "$want" ]; then + echo "ok $desc" + else + echo "FAIL $desc: expected $want, got $actual" + failures=$((failures + 1)) + fi +} + +# expect_absent -- the needle is in the fixture and must +# not survive the projection +expect_absent() { + local fixture=$1 needle=$2 desc=$3 + if ! grep -qF -- "$needle" "tests/fixtures/$fixture"; then + echo "FAIL $desc: fixture no longer contains '$needle', so the test proves nothing" + failures=$((failures + 1)) + return + fi + if project "$fixture" | grep -qF -- "$needle"; then + echo "FAIL $desc: '$needle' leaked into the projection" + failures=$((failures + 1)) + else + echo "ok $desc" + fi +} + +# The denial names are the whole point: two Grep denials and one Glob, ranked. +expect_jq execution-log-denials.json '.denials' \ + '[{"name":"Grep","n":2},{"name":"Glob","n":1}]' \ + "denied tool names survive with counts" + +# Calls are counted per tool, most-used first, independent of whether they were denied. +expect_jq execution-log-denials.json '.tool_calls[0]' \ + '{"name":"Grep","n":2}' \ + "tool calls counted and ranked" +expect_jq execution-log-denials.json '[.tool_calls[].name] | sort' \ + '["Bash","Grep","Read"]' \ + "every called tool appears once" + +# Enough of the result message to compare runs before and after an allowlist change. +expect_jq execution-log-denials.json '.result.num_turns' '19' "turn count carried through" +expect_jq execution-log-denials.json '.result.subtype' '"success"' "result subtype carried through" + +# The leak assertions. The fixture has the reviewer reading the runner's git credentials +# file, which is the concrete path by which a token reaches the transcript. +expect_absent execution-log-denials.json \ + "eC1hY2Nlc3MtdG9rZW46Z2hzX0ZBS0VUT0tFTkZPUlRFU1RT" \ + "credential from a tool result does not reach the artifact" +expect_absent execution-log-denials.json \ + "git-credentials-82efe7dc.config" \ + "file path from a tool input does not reach the artifact" +expect_absent execution-log-denials.json \ + "diff --git" \ + "repository contents from a tool result do not reach the artifact" +expect_absent execution-log-denials.json \ + "retention-days" \ + "search patterns from a denied call do not reach the artifact" + +# A clean run: no denials, and the MCP inline-comment tool counted like any other. +expect_jq execution-log-clean.json '.denials' '[]' "clean run reports no denials" +expect_jq execution-log-clean.json '[.tool_calls[].name] | sort' \ + '["Bash","mcp__github_inline_comment__create_inline_comment"]' \ + "mcp tool names counted" + +# A cancelled or crashed run leaves a log with no result message. The step is +# continue-on-error, but the filter should still produce a usable artifact rather than +# abort, so the tool calls made before the run died are not lost. +expect_jq execution-log-truncated.json '.denials' '[]' "log with no result message yields no denials" +expect_jq execution-log-truncated.json '.tool_calls' '[{"name":"Read","n":1}]' \ + "tool calls survive a log with no result message" +expect_jq execution-log-truncated.json '.result.num_turns' 'null' \ + "missing result message yields null fields, not an error" + +# Degenerate input must not crash the filter. +empty=$(printf '[]' | jq -c "$TOOL_USAGE_JQ") +if [ "$empty" = '{"tool_calls":[],"denials":[],"result":{"subtype":null,"is_error":null,"num_turns":null,"duration_ms":null,"total_cost_usd":null}}' ]; then + echo "ok empty log projects to empty counts" +else + echo "FAIL empty log: got $empty" + failures=$((failures + 1)) +fi + +if [ "$failures" -ne 0 ]; then + echo "$failures test(s) failed" + exit 1 +fi +echo "all tests passed" From cc2916285e17cc7b291955749b41d5ac50b91385 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Fri, 31 Jul 2026 14:18:45 -0700 Subject: [PATCH 4/4] docs(review): merge duplicated execution-log comment blocks --- .github/workflows/claude-pr-review.yml | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index dad3fc9..23611b0 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -152,13 +152,11 @@ jobs: # 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. + # allowlist withheld. That is a hypothesis, and this 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. # # Never upload that file as-is. It holds every tool input and result, the runner has # a readable git credential (checkout persists one via includeIf into