Skip to content

Commit 46150e4

Browse files
zfarrellzfarrell
committed
feat(review): label Bash commands in the artifact
1 parent 9a7f6d7 commit 46150e4

4 files changed

Lines changed: 196 additions & 43 deletions

File tree

‎.github/workflows/claude-pr-review.yml‎

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -366,21 +366,23 @@ jobs:
366366
--allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr review:*),Bash(rg:*),Read,Grep,Glob"
367367
368368
# Grep/Glob above were added because 64% of runs (256/400 sampled) hit at least
369-
# one permission denial — 1,562 denials across 7,819 turns. Search is the only
370-
# tool class the prompt's "read affected files for context" step needs that the
371-
# allowlist withheld. That is a hypothesis, and this is how it gets checked: the
372-
# action writes the full conversation to execution_file but prints only
373-
# permission_denials.length to the job log (and the check-run summary is empty
374-
# under track_progress: false), so the denied tool *names* exist nowhere a later
375-
# run can read, and the file itself dies with the runner.
369+
# one permission denial — 1,562 denials across 7,819 turns. Search was the leading
370+
# hypothesis for what the allowlist withheld, and the first week of these artifacts
371+
# refuted it: 86% of 109 runs still hit a denial, 5.2 per run, and in runtimedb the
372+
# rate went up. The artifact could not say why, because tool *names* are not the
373+
# answer -- 520 of 567 denials were "Bash", and Bash is every command there is.
376374
#
377-
# Never upload that file as-is. It holds every tool input and result, the runner has
378-
# a readable git credential (checkout persists one via includeIf into
375+
# So the projection carries a command label now, and the labels come from the fixed
376+
# vocabulary in CMD_JQ, never from the transcript. That distinction is the whole
377+
# design. Never upload the execution log as-is: it holds every tool input and result,
378+
# the runner has a readable git credential (checkout persists one via includeIf into
379379
# $RUNNER_TEMP/git-credentials-*.config), Read is unrestricted, and ::add-mask::
380-
# scrubs the job log but not artifacts -- so a raw upload turns anything the
381-
# reviewer happened to read into a downloadable file. Tool names and counts answer
382-
# the allowlist question by themselves, so project to those and upload only that.
383-
# The filter below emits no tool input and no result body by construction.
380+
# scrubs the job log but not artifacts -- so a raw upload turns anything the reviewer
381+
# happened to read into a downloadable file. A prefix of the command string would be
382+
# the same leak in miniature: `cat /home/runner/work/_temp/git-credentials-*.config`
383+
# is a path, and paths are what the leak assertions in tests/tool-usage-test.sh exist
384+
# to keep out. Matching each command against a closed set of labels and emitting the
385+
# label bounds the output to strings this file already contains.
384386
#
385387
# The review step is continue-on-error so a failed review still reaches the notify
386388
# step; empty execution_file means it wrote nothing, hence the output guard. Both
@@ -391,10 +393,20 @@ jobs:
391393
continue-on-error: true
392394
if: github.event.pull_request.user.login != 'dependabot[bot]' && steps.review.outputs.execution_file != ''
393395
run: |
394-
# Kept as a single-line assignment so tests/tool-usage-test.sh can extract and
395-
# exercise the shipped expression rather than a copy of it.
396-
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})}'
397-
jq "$TOOL_USAGE_JQ" "$EXECUTION_FILE" > "${RUNNER_TEMP}/claude-tool-usage.json"
396+
# Both kept as single-line assignments so tests/tool-usage-test.sh can extract and
397+
# exercise the shipped expressions rather than copies of them, and composed the
398+
# same way there: jq "$CMD_JQ $TOOL_USAGE_JQ".
399+
#
400+
# norm strips the wrappers the reviewer puts in front of a real command (timeout,
401+
# cd .. &&, env VAR=x) so they do not all collapse into "other". verb returns the
402+
# first matching label or "other" -- the output is always one of these literals.
403+
CMD_JQ='def norm: sub("^\\s+"; "") | sub("^timeout\\s+[0-9]+m?\\s+"; "") | sub("^cd\\s+[^&|;]+&&\\s*"; "") | sub("^env\\s+\\S+=\\S+\\s+"; ""); def verb: . as $c | ([[["^gh\\s+pr\\s+diff", "gh pr diff"], ["^gh\\s+pr\\s+view", "gh pr view"], ["^gh\\s+pr\\s+checks", "gh pr checks"], ["^gh\\s+pr\\s+review", "gh pr review"], ["^gh\\s+pr\\s+comment", "gh pr comment"], ["^gh\\s+api", "gh api"], ["^gh\\s", "gh other"], ["^git\\s+diff", "git diff"], ["^git\\s+log", "git log"], ["^git\\s+show", "git show"], ["^git\\s+blame", "git blame"], ["^git\\s", "git other"], ["^rg\\b", "rg"], ["^grep\\b", "grep"], ["^(fd|find)\\b", "find"], ["^(ls|tree)\\b", "ls"], ["^(sed|awk)\\b", "sed/awk"], ["^(cat|head|tail|wc)\\b", "cat/head/tail"], ["^(pytest|uv|python3?|cargo|npm|pnpm|yarn|bun|node|go|make|ruff|mypy|pyflakes)\\b", "run tests/build"]][] | select(.[0] as $re | $c | test($re))] | .[0] // ["", "other"]) | .[1]; def classify: {cmd: (norm | verb), compound: test("\\||&&|;|>")};'
404+
# commands and denied_commands answer two different questions: what the reviewer
405+
# spends its Bash budget on, and which of those the allowlist refuses. compound is
406+
# carried separately because an allowlisted command still gets denied when it is
407+
# piped or redirected, which no tool name or verb alone would show.
408+
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)), commands: ([.[]? | select(.type=="assistant") | .message.content[]? | select(.type=="tool_use" and .name=="Bash") | (.input.command // "") | classify] | group_by([.cmd, .compound]) | map({cmd: .[0].cmd, compound: .[0].compound, 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)), denied_commands: (([.[]? | select(.type=="result")] | last // {}) | (.permission_denials // []) | map(select(.tool_name == "Bash") | (.tool_input.command // "") | classify) | group_by([.cmd, .compound]) | map({cmd: .[0].cmd, compound: .[0].compound, n: length}) | sort_by(-.n)), result: (([.[]? | select(.type=="result")] | last // {}) | {subtype, is_error, num_turns, duration_ms, total_cost_usd})}'
409+
jq "$CMD_JQ $TOOL_USAGE_JQ" "$EXECUTION_FILE" > "${RUNNER_TEMP}/claude-tool-usage.json"
398410
env:
399411
# Via env, not a ${{ }} interpolation inside the script, so the path cannot be
400412
# spliced into the shell command.

‎README.md‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,17 @@ explanation.
3636
### Tool usage artifact
3737

3838
Each run attaches a `claude-tool-usage-pr-<number>` artifact (14-day retention): tool call counts,
39-
denied tool names, and the run's turn count and cost. It exists to diagnose permission denials
40-
against the workflow's `--allowedTools` list, since the job log records only the number of denials,
41-
never which tools were refused.
39+
Bash command labels with a compound flag, the denied subset of both, and the run's turn count and
40+
cost. It exists to diagnose permission denials against the workflow's `--allowedTools` list, since
41+
the job log records only the number of denials, never what was refused. Tool names alone proved
42+
insufficient — 520 of 567 denials in the first week were `Bash`, which is every command there is.
4243

4344
The artifact is a projection of the action's execution log, never the log itself — that file is the
4445
full conversation, and the runner holds a git credential the reviewer can read, which artifacts
45-
(unlike job logs) would not mask. `TOOL_USAGE_JQ` in the workflow emits names and counts only, and
46-
`tests/tool-usage-test.sh` asserts that tool inputs, tool results, and repository contents cannot
47-
reach the artifact. Both the projection and the upload are non-fatal.
46+
(unlike job logs) would not mask. Command labels come from the fixed vocabulary in `CMD_JQ`, never
47+
from the transcript, so no path, search pattern, or credential can ride along in a "command prefix".
48+
`tests/tool-usage-test.sh` asserts that containment directly. Both the projection and the upload are
49+
non-fatal.
4850

4951
## Setup
5052

‎tests/fixtures/execution-log-denials.json‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,64 @@
104104
]
105105
}
106106
},
107+
{
108+
"type": "assistant",
109+
"message": {
110+
"role": "assistant",
111+
"content": [
112+
{
113+
"type": "tool_use",
114+
"id": "tu_6",
115+
"name": "Bash",
116+
"input": { "command": "gh pr diff 21 > /tmp/pr21.diff && wc -l /tmp/pr21.diff" }
117+
}
118+
]
119+
}
120+
},
121+
{
122+
"type": "assistant",
123+
"message": {
124+
"role": "assistant",
125+
"content": [
126+
{
127+
"type": "tool_use",
128+
"id": "tu_7",
129+
"name": "Bash",
130+
"input": { "command": "rg -n \"ANTHROPIC_API_KEY\" .github" }
131+
}
132+
]
133+
}
134+
},
135+
{
136+
"type": "assistant",
137+
"message": {
138+
"role": "assistant",
139+
"content": [
140+
{
141+
"type": "tool_use",
142+
"id": "tu_8",
143+
"name": "Bash",
144+
"input": {
145+
"command": "cat /home/runner/work/_temp/git-credentials-82efe7dc.config"
146+
}
147+
}
148+
]
149+
}
150+
},
151+
{
152+
"type": "assistant",
153+
"message": {
154+
"role": "assistant",
155+
"content": [
156+
{
157+
"type": "tool_use",
158+
"id": "tu_9",
159+
"name": "Bash",
160+
"input": { "command": "timeout 900 uv run pytest -q tests/" }
161+
}
162+
]
163+
}
164+
},
107165
{
108166
"type": "result",
109167
"subtype": "success",
@@ -126,6 +184,23 @@
126184
"tool_name": "Glob",
127185
"tool_use_id": "tu_5",
128186
"tool_input": { "pattern": "**/*.yml" }
187+
},
188+
{
189+
"tool_name": "Bash",
190+
"tool_use_id": "tu_6",
191+
"tool_input": { "command": "gh pr diff 21 > /tmp/pr21.diff && wc -l /tmp/pr21.diff" }
192+
},
193+
{
194+
"tool_name": "Bash",
195+
"tool_use_id": "tu_8",
196+
"tool_input": {
197+
"command": "cat /home/runner/work/_temp/git-credentials-82efe7dc.config"
198+
}
199+
},
200+
{
201+
"tool_name": "Bash",
202+
"tool_use_id": "tu_9",
203+
"tool_input": { "command": "timeout 900 uv run pytest -q tests/" }
129204
}
130205
]
131206
}

‎tests/tool-usage-test.sh‎

Lines changed: 84 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,37 +7,35 @@
77
# Two jobs. First, the projection has to actually answer the question it exists for: the
88
# action prints only permission_denials.length to the job log, so the denied tool *names*
99
# live nowhere else and a change that drops them would be invisible until someone went
10-
# looking for data that was never collected.
10+
# looking for data that was never collected. Names turned out not to be enough -- 520 of
11+
# 567 denials in the first week were "Bash" -- so the projection labels commands too, and
12+
# the labels have to survive the same way.
1113
#
1214
# Second, and the reason this file is worth more than its assertions: the input is the full
1315
# conversation -- every tool input and every tool result. The runner has a readable git
1416
# credential (checkout persists one via includeIf into $RUNNER_TEMP/git-credentials-*.config)
1517
# and Read is unrestricted, so a transcript can contain a live token. ::add-mask:: scrubs the
1618
# job log but not artifacts. The leak assertions below hold the line that this artifact
17-
# carries names and counts only; widening the projection to "just the tool inputs too" is
18-
# exactly the change that would quietly publish a token.
19+
# carries names, counts, and labels drawn from a closed vocabulary -- never text from the
20+
# transcript. "Just the first token of the command" or "just the command prefix" is exactly
21+
# the change that would quietly publish a credential path, so the vocabulary assertion
22+
# below asserts the containment directly: every label in the artifact appears in CMD_JQ.
1923

2024
set -euo pipefail
2125

2226
cd "$(dirname "$0")/.."
2327

24-
WORKFLOW=.github/workflows/claude-pr-review.yml
28+
# shellcheck source=tests/lib.sh
29+
. tests/lib.sh
2530

26-
TOOL_USAGE_JQ=$(sed -n "s/^ *TOOL_USAGE_JQ='\(.*\)'\$/\1/p" "$WORKFLOW")
27-
if [ -z "$TOOL_USAGE_JQ" ]; then
28-
echo "FAIL: no TOOL_USAGE_JQ='...' assignment found in $WORKFLOW" >&2
29-
exit 1
30-
fi
31-
if [ "$(printf '%s\n' "$TOOL_USAGE_JQ" | wc -l)" -ne 1 ]; then
32-
echo "FAIL: more than one TOOL_USAGE_JQ assignment in $WORKFLOW" >&2
33-
exit 1
34-
fi
31+
CMD_JQ=$(extract_jq CMD_JQ)
32+
TOOL_USAGE_JQ=$(extract_jq TOOL_USAGE_JQ)
3533

3634
failures=0
3735

38-
# project <fixture> -- run the shipped filter over a fixture
36+
# project <fixture> -- run the shipped filter over a fixture, composed as the workflow does
3937
project() {
40-
jq "$TOOL_USAGE_JQ" "tests/fixtures/$1"
38+
jq "$CMD_JQ $TOOL_USAGE_JQ" "tests/fixtures/$1"
4139
}
4240

4341
# expect_jq <fixture> <jq expression> <expected> <description>
@@ -69,14 +67,14 @@ expect_absent() {
6967
fi
7068
}
7169

72-
# The denial names are the whole point: two Grep denials and one Glob, ranked.
70+
# The denial names: three Bash, two Grep, one Glob, ranked.
7371
expect_jq execution-log-denials.json '.denials' \
74-
'[{"name":"Grep","n":2},{"name":"Glob","n":1}]' \
72+
'[{"name":"Bash","n":3},{"name":"Grep","n":2},{"name":"Glob","n":1}]' \
7573
"denied tool names survive with counts"
7674

7775
# Calls are counted per tool, most-used first, independent of whether they were denied.
7876
expect_jq execution-log-denials.json '.tool_calls[0]' \
79-
'{"name":"Grep","n":2}' \
77+
'{"name":"Bash","n":5}' \
8078
"tool calls counted and ranked"
8179
expect_jq execution-log-denials.json '[.tool_calls[].name] | sort' \
8280
'["Bash","Grep","Read"]' \
@@ -86,6 +84,64 @@ expect_jq execution-log-denials.json '[.tool_calls[].name] | sort' \
8684
expect_jq execution-log-denials.json '.result.num_turns' '19' "turn count carried through"
8785
expect_jq execution-log-denials.json '.result.subtype' '"success"' "result subtype carried through"
8886

87+
# Bash calls are labelled, and compound forms are counted apart from bare ones. `gh pr diff`
88+
# appears twice for that reason: allowlisted on its own, refused the moment it is redirected
89+
# into a file and chained -- which is why the artifact has to distinguish them.
90+
expect_jq execution-log-denials.json \
91+
'[.commands[] | .cmd + (if .compound then " (compound)" else "" end)] | sort' \
92+
'["cat/head/tail","gh pr diff","gh pr diff (compound)","rg","run tests/build"]' \
93+
"Bash commands labelled, compound forms kept separate"
94+
95+
# `timeout 900 uv run pytest` has to reach "run tests/build" rather than "other": the
96+
# wrappers the reviewer puts in front of a command are what norm exists to strip.
97+
expect_jq execution-log-denials.json \
98+
'[.commands[] | select(.cmd == "run tests/build")] | length' '1' \
99+
"timeout wrapper stripped before labelling"
100+
101+
# The denied subset is the actionable half: what the allowlist is actually costing.
102+
expect_jq execution-log-denials.json \
103+
'[.denied_commands[] | .cmd + (if .compound then " (compound)" else "" end)] | sort' \
104+
'["cat/head/tail","gh pr diff (compound)","run tests/build"]' \
105+
"denied Bash commands labelled"
106+
107+
# Non-Bash denials carry no command label -- there is no command to label.
108+
expect_jq execution-log-denials.json '[.denied_commands[].n] | add' '3' \
109+
"only Bash denials appear in denied_commands"
110+
111+
# The containment assertion, and the one that has to keep holding: every label the
112+
# projection emits is a literal in CMD_JQ. Nothing derived from the transcript can satisfy
113+
# it, so the artifact cannot grow a credential path, a search pattern, or a file name
114+
# without this failing first.
115+
vocabulary=$(printf '%s' "$CMD_JQ" | grep -o '", "[a-z /]*"\]' | sed 's/^", "//; s/"\]$//' | sort -u)
116+
if [ -z "$vocabulary" ]; then
117+
echo "FAIL vocabulary: no labels found in CMD_JQ, so the containment test proves nothing"
118+
failures=$((failures + 1))
119+
else
120+
emitted=$(project execution-log-denials.json | jq -r '[.commands[], .denied_commands[]] | .[].cmd' | sort -u)
121+
unknown=$(comm -23 <(printf '%s\n' "$emitted") <(printf '%s\n' "$vocabulary" | grep -v '^other$'; echo other))
122+
if [ -z "$unknown" ]; then
123+
echo "ok every emitted label comes from the CMD_JQ vocabulary"
124+
else
125+
echo "FAIL emitted labels outside the CMD_JQ vocabulary: $unknown"
126+
failures=$((failures + 1))
127+
fi
128+
fi
129+
130+
# An unrecognised command must fall back to "other" and carry none of itself across. A bare
131+
# curl with a bearer token is the worst case: the whole command is the secret.
132+
leaky='[{"type":"assistant","message":{"role":"assistant","content":[{"type":"tool_use","id":"t","name":"Bash","input":{"command":"curl -H \"Authorization: Bearer ghs_FAKETOKENFORTESTS\" https://api.github.com"}}]}}]'
133+
leaked=$(printf '%s' "$leaky" | jq -c "$CMD_JQ $TOOL_USAGE_JQ")
134+
if printf '%s' "$leaked" | grep -qF "ghs_FAKETOKENFORTESTS" \
135+
|| printf '%s' "$leaked" | grep -qF "curl"; then
136+
echo "FAIL unrecognised command leaked into the projection: $leaked"
137+
failures=$((failures + 1))
138+
elif printf '%s' "$leaked" | jq -e '.commands == [{"cmd":"other","compound":false,"n":1}]' >/dev/null; then
139+
echo "ok unrecognised command reduces to \"other\""
140+
else
141+
echo "FAIL unrecognised command did not reduce to \"other\": $leaked"
142+
failures=$((failures + 1))
143+
fi
144+
89145
# The leak assertions. The fixture has the reviewer reading the runner's git credentials
90146
# file, which is the concrete path by which a token reaches the transcript.
91147
expect_absent execution-log-denials.json \
@@ -100,6 +156,12 @@ expect_absent execution-log-denials.json \
100156
expect_absent execution-log-denials.json \
101157
"retention-days" \
102158
"search patterns from a denied call do not reach the artifact"
159+
expect_absent execution-log-denials.json \
160+
"ANTHROPIC_API_KEY" \
161+
"search patterns inside a Bash command do not reach the artifact"
162+
expect_absent execution-log-denials.json \
163+
"pr21.diff" \
164+
"file names inside a Bash command do not reach the artifact"
103165

104166
# A clean run: no denials, and the MCP inline-comment tool counted like any other.
105167
expect_jq execution-log-clean.json '.denials' '[]' "clean run reports no denials"
@@ -111,14 +173,16 @@ expect_jq execution-log-clean.json '[.tool_calls[].name] | sort' \
111173
# continue-on-error, but the filter should still produce a usable artifact rather than
112174
# abort, so the tool calls made before the run died are not lost.
113175
expect_jq execution-log-truncated.json '.denials' '[]' "log with no result message yields no denials"
176+
expect_jq execution-log-truncated.json '.denied_commands' '[]' \
177+
"log with no result message yields no denied commands"
114178
expect_jq execution-log-truncated.json '.tool_calls' '[{"name":"Read","n":1}]' \
115179
"tool calls survive a log with no result message"
116180
expect_jq execution-log-truncated.json '.result.num_turns' 'null' \
117181
"missing result message yields null fields, not an error"
118182

119183
# Degenerate input must not crash the filter.
120-
empty=$(printf '[]' | jq -c "$TOOL_USAGE_JQ")
121-
if [ "$empty" = '{"tool_calls":[],"denials":[],"result":{"subtype":null,"is_error":null,"num_turns":null,"duration_ms":null,"total_cost_usd":null}}' ]; then
184+
empty=$(printf '[]' | jq -c "$CMD_JQ $TOOL_USAGE_JQ")
185+
if [ "$empty" = '{"tool_calls":[],"commands":[],"denials":[],"denied_commands":[],"result":{"subtype":null,"is_error":null,"num_turns":null,"duration_ms":null,"total_cost_usd":null}}' ]; then
122186
echo "ok empty log projects to empty counts"
123187
else
124188
echo "FAIL empty log: got $empty"

0 commit comments

Comments
 (0)