diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 001fdc9..23611b0 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -147,7 +147,57 @@ 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. 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 + # $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-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); the question is days-old, not quarters. + 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/.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 77996a2..d65439a 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,17 @@ 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-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 Requires: 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"