Merge pull request #34 from hotdata-dev/fix/reland-subst-flag

feat(review): flag command substitution in the tool usage artifact

Author: zfarrell

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

@@ -277,15 +277,38 @@ jobs:
           # norm strips the wrappers the reviewer puts in front of a real command (timeout,
           # cd .. &&, env VAR=x) so they do not all collapse into "other". verb returns the
           # first matching label or "other" -- the output is always one of these literals.
-          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 unquoted: gsub("\"[^\"]*\""; "") | gsub("\u0027[^\u0027]*\u0027"; ""); def classify: {cmd: (norm | verb), compound: (unquoted | test("\\||&&|;|>"))}; def toolname: if type == "string" and test("^[A-Za-z0-9_-]{1,64}$") then . else "unknown" end;'
+          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 unquoted: gsub("\"[^\"]*\""; "") | gsub("\u0027[^\u0027]*\u0027"; ""); def classify: {cmd: (norm | verb), compound: (unquoted | test("\\||&&|;|>")), has_subst: test("`|\\$\\(")}; def toolname: if type == "string" and test("^[A-Za-z0-9_-]{1,64}$") then . else "unknown" end;'
+          #
+          # has_subst is the same kind of flag for the denials that outlived the frontloaded
+          # context. Reads mostly stopped being refused once the context arrived in the prompt
+          # -- denials fell from 5.2 a run to 0.5 -- and what is left is the *write* path:
+          # `gh pr review` is allowlisted and still refused on 29% of its 241 attempts across
+          # 170 settled-window runs and 8 repos, 1.6 times per affected run before the review
+          # lands, costing those runs +$0.46 and +73s each (issue #33). compound reported 1 of
+          # those 71, which is the point: it cannot see this. The standing hypothesis is the
+          # review body rather than the command -- a body is markdown, and a backtick inside a
+          # double-quoted argument is command substitution to anything parsing shell. So the
+          # flag is tested against the *raw* command, not the unquoted form compound uses:
+          # stripping quoted spans first would remove precisely the backticks in question. It
+          # rides on `commands` as well as `denied_commands` because a denial rate needs its
+          # base rate to mean anything. Boolean, like compound: a label, never a span of the
+          # command.
+          #
+          # This shipped once before, in #26, and was reverted in 8b04393 -- not on its own
+          # merits: #26 pushed the *context* step's run block to 22,016 characters, past the
+          # 21,000-character expression limit, and the revert took this with it. #29 then moved
+          # that step out to scripts/gather-review-context.sh, so the budget that forced the
+          # revert is gone. The step below is 3.3k with the flag; the guard in
+          # tests/workflow-lint-test.sh now fails before a run block can reach the limit again.
+          #
           # commands and denied_commands answer two different questions: what the reviewer
           # spends its Bash budget on, and which of those the allowlist refuses. compound is
           # carried separately because an allowlisted command still gets denied when it is
           # piped or redirected, which no tool name or verb alone would show -- and it is
           # tested against the command with quoted spans removed, because `rg -n \"a|b\"` is
           # one allowlisted command and counting its alternation as a pipe would inflate
           # exactly the number the flag exists to produce.
-          TOOL_USAGE_JQ='{tool_calls: ([.[]? | select(.type=="assistant") | .message.content[]? | select(.type=="tool_use") | .name | toolname] | 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 | toolname) | 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})}'
+          TOOL_USAGE_JQ='{tool_calls: ([.[]? | select(.type=="assistant") | .message.content[]? | select(.type=="tool_use") | .name | toolname] | 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, .has_subst]) | map({cmd: .[0].cmd, compound: .[0].compound, has_subst: .[0].has_subst, n: length}) | sort_by(-.n)), denials: (([.[]? | select(.type=="result")] | last // {}) | (.permission_denials // []) | map(.tool_name | toolname) | 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, .has_subst]) | map({cmd: .[0].cmd, compound: .[0].compound, has_subst: .[0].has_subst, n: length}) | sort_by(-.n)), result: (([.[]? | select(.type=="result")] | last // {}) | {subtype, is_error, num_turns, duration_ms, total_cost_usd})}'
           jq "$CMD_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

README.md

@@ -51,10 +51,21 @@ them would otherwise end the data block early and land the rest where it reads a
 ### Tool usage artifact
 
 Each run attaches a `claude-tool-usage-pr-<number>` artifact (14-day retention): tool call counts,
-Bash command labels with a compound flag, the denied subset of both, 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 what was refused. Tool names alone proved
-insufficient — 520 of 567 denials in the first week were `Bash`, which is every command there is.
+Bash command labels with a compound flag and a command-substitution flag, the denied subset of both,
+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 what was refused.
+Tool names alone proved insufficient — 520 of 567 denials in the first week were `Bash`, which is
+every command there is.
+
+The two flags are deliberately measured differently, and the difference is the point. `compound`
+strips quoted spans before looking for `| && ; >`, because `rg -n "a|b"` is one allowlisted command
+and counting its alternation as a pipe would inflate the number the flag exists to produce.
+`has_subst` tests the raw command for `` ` `` and `$(`, because the suspected trigger lives *inside*
+the quoted body: a review body is markdown, and a backtick in a double-quoted argument is command
+substitution to anything parsing shell. Frontloading the context fixed the read path — denials fell
+from 5.2 a run to 0.5 — but `gh pr review` is allowlisted and still refused on 29% of its 241
+attempts across 170 runs and 8 repos, at +$0.46 and +73s per affected run, with `compound` reporting
+1 of those 71. Measuring `has_subst` the same way as `compound` would have kept that invisible.
 
 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

tests/fixtures/execution-log-review-body.json

@@ -0,0 +1,50 @@
+[
+  {
+    "type": "assistant",
+    "message": {
+      "role": "assistant",
+      "content": [
+        {
+          "type": "tool_use",
+          "id": "t1",
+          "name": "Bash",
+          "input": {
+            "command": "gh pr review 308 --request-changes --body \"**Blocking:** `components/GetUpdates.tsx:5` hardcodes the endpoint. Route it through `app/api/get-updates/route.ts` and apply `rateLimit(ip, 5, 60000 * 10)`.\""
+          }
+        }
+      ]
+    }
+  },
+  {
+    "type": "assistant",
+    "message": {
+      "role": "assistant",
+      "content": [
+        {
+          "type": "tool_use",
+          "id": "t2",
+          "name": "Bash",
+          "input": {
+            "command": "gh pr review 308 --request-changes --body \"Blocking: components/GetUpdates.tsx line 5 hardcodes the endpoint. Route it through app/api/get-updates/route.ts and apply the rate limiter.\""
+          }
+        }
+      ]
+    }
+  },
+  {
+    "type": "result",
+    "subtype": "success",
+    "is_error": false,
+    "num_turns": 11,
+    "duration_ms": 151402,
+    "total_cost_usd": 0.8021,
+    "permission_denials": [
+      {
+        "tool_name": "Bash",
+        "tool_input": {
+          "command": "gh pr review 308 --request-changes --body \"**Blocking:** `components/GetUpdates.tsx:5` hardcodes the endpoint. Route it through `app/api/get-updates/route.ts` and apply `rateLimit(ip, 5, 60000 * 10)`.\""
+        }
+      }
+    ]
+  }
+]

tests/tool-usage-test.sh

@@ -141,6 +141,64 @@ expect_compound "rg -n 'a|b' src/ | head -20" true \
 expect_compound 'rg -n foo src/' false \
   "a plain search is not compound"
 
+# has_subst answers the denial the frontloaded context did not remove: an allowlisted
+# `gh pr review` refused on the way to posting. Unlike compound it is tested against the raw
+# command, because the suspected trigger lives *inside* the quoted body -- a review body is
+# markdown, and backticks in a double-quoted argument are command substitution to anything
+# parsing shell. Running it through `unquoted` first would delete the evidence.
+#
+# Measured before this landed: `gh pr review` is refused on 29% of its 241 attempts across
+# 170 settled-window runs, on 8 repos, costing the affected runs +$0.46 and +73s each --
+# while `compound` reported 1 of 71. See issue #33.
+subst_of() {
+  printf '%s' "$1" | jq -R -r "$CMD_JQ classify | .has_subst | tostring"
+}
+# expect_subst <command> <true|false> <description>
+expect_subst() {
+  local actual
+  actual=$(subst_of "$1")
+  if [ "$actual" = "$2" ]; then
+    echo "ok   $3"
+  else
+    echo "FAIL $3: expected has_subst=$2, got $actual for: $1"
+    failures=$((failures + 1))
+  fi
+}
+
+expect_subst 'gh pr review 21 --approve --body "nit: `foo` is wrong"' true \
+  "a backtick inside the review body is flagged"
+expect_subst 'gh pr comment 21 --body "see $(basename x)"' true \
+  "an explicit command substitution is flagged"
+expect_subst 'gh pr review 21 --approve --body "no markdown here"' false \
+  "a plain body is not flagged"
+expect_subst 'rg -n foo src/' false \
+  "a plain search is not flagged"
+# The distinction from compound, stated as an assertion: quoted spans are removed for one
+# flag and kept for the other, so a body whose only shell-ish characters are backticks is
+# has_subst without being compound. Getting these the same way round would make the two
+# columns redundant and lose the write-path denials again.
+expect_compound 'gh pr review 21 --approve --body "nit: `foo` is wrong"' false \
+  "a backtick in a quoted body is not compound"
+
+# End to end over the shape actually seen in production: the reviewer's first
+# `gh pr review --request-changes` was refused, and the retry that landed carried the same
+# feedback with the backticks removed. Both rows are `gh pr review`; has_subst is the only
+# thing that tells them apart, which is the whole reason it is grouped on.
+expect_jq execution-log-review-body.json \
+  '[.commands[] | {cmd, has_subst, n}] | sort_by(.has_subst)' \
+  '[{"cmd":"gh pr review","has_subst":false,"n":1},{"cmd":"gh pr review","has_subst":true,"n":1}]' \
+  "the flagged and unflagged attempts are counted apart"
+expect_jq execution-log-review-body.json '.denied_commands' \
+  '[{"cmd":"gh pr review","compound":false,"has_subst":true,"n":1}]' \
+  "the denied review post is flagged and not compound"
+
+# Same boundary as every other label: the flag is a boolean, so no part of the body it was
+# computed from may ride along with it.
+expect_absent execution-log-review-body.json "GetUpdates.tsx" \
+  "the review body does not reach the artifact"
+expect_absent execution-log-review-body.json "rateLimit" \
+  "code quoted in the review body does not reach the artifact"
+
 # The containment assertion, and the one that has to keep holding: every label the
 # projection emits is a literal in CMD_JQ. Nothing derived from the transcript can satisfy
 # it, so the artifact cannot grow a credential path, a search pattern, or a file name
@@ -202,7 +260,7 @@ if printf '%s' "$leaked" | grep -qF "ghs_FAKETOKENFORTESTS" \
   || printf '%s' "$leaked" | grep -qF "curl"; then
   echo "FAIL unrecognised command leaked into the projection: $leaked"
   failures=$((failures + 1))
-elif printf '%s' "$leaked" | jq -e '.commands == [{"cmd":"other","compound":false,"n":1}]' >/dev/null; then
+elif printf '%s' "$leaked" | jq -e '.commands == [{"cmd":"other","compound":false,"has_subst":false,"n":1}]' >/dev/null; then
   echo "ok   unrecognised command reduces to \"other\""
 else
   echo "FAIL unrecognised command did not reduce to \"other\": $leaked"