Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
52 changes: 51 additions & 1 deletion .github/workflows/claude-pr-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"

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. 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')
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
11 changes: 11 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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-<number>` 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:
Expand Down
45 changes: 45 additions & 0 deletions tests/fixtures/execution-log-clean.json
Original file line number Diff line number Diff line change
@@ -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": []
}
]
132 changes: 132 additions & 0 deletions tests/fixtures/execution-log-denials.json
Original file line number Diff line number Diff line change
@@ -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" }
}
]
}
]
22 changes: 22 additions & 0 deletions tests/fixtures/execution-log-truncated.json
Original file line number Diff line number Diff line change
@@ -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" }
}
]
}
}
]
Loading