fix(review-utils): let the reviewer write its metrics file

[nsheaps]

What changed

File	Change
plugins/claude-code/review-utils/actions/run-agent/action.yaml	The Write rule is now Write(/${{ runner.temp }}/**), which renders as //home/runner/work/_temp/**. A matching Edit rule is added. Both rules go in --allowedTools and in settings.permissions.allow. The comments are rewritten to explain the // requirement.
same file	New step Report permission denials. It reads permission_denials from claude-code-action's execution_file and emits one ::warning per denial plus a step-summary table.
plugins/claude-code/review-utils/skills/review-code/SKILL.md	The notes file is now named and placed next to ${REVIEW_METRICS_PATH}. The text describing the Write rule is corrected. The example's ${REVIEW_URL} (never set, so envsubst made it empty) is replaced. The partials are now referred to by their inlined section names instead of file paths.
templates/dispatch-receiver-review.yaml	Rewritten to match the receiver that actually runs. It listens for the event types the decider really sends (pull_request/<action>, now including opened and reopened) instead of pr-review. It passes the secret names review-receiver.yaml requires and reads the consumer_workflow_run_url key.
docs/research/review-pipeline-as-is-2026-10-07.md	New. The as-is audit of the whole review pipeline: components, outage timeline, secrets matrix, and the broken/missing list.

Why

The review format on nsheaps/agent-jordan#2 was wrong. #368 fixed that by inlining the partials. The run behind that PR (37557540105) also failed its metrics gate with "No metrics file":

• In Claude Code permission rules, a single leading / means a path relative to the settings source, not the filesystem root. An absolute path needs //.
• So the old Write(/home/runner/work/_temp/**) never matched, and every Write to the temp dir was denied, silently.
• Runs showed 12–25 permission denials that were never logged. The new step makes them visible.

Blast radius

• Affected: every review that goes through review-receiver.yaml, because plugin-ref defaults to main. Henry's receiver pins the workflow by SHA but checks out the plugin at main.
• When: the next review after this merges.
• What it widens: Write and Edit are allowed only under the runner temp dir, which is already in additionalDirectories.
• New step: read-only, and it only logs.
• Template: documentation only. Nothing syncs it today.
• Rollback: revert this PR.

Validation performed

I extracted the new step's script from the YAML and ran it against a sample execution file:

$ yq -r '.runs.steps[] | select(.name=="Report permission denials") | .run' action.yaml > denials.sh
$ EXECUTION_FILE=exec.json GITHUB_STEP_SUMMARY=/dev/stdout bash denials.sh
Permission denials: 1
::warning title=Permission denied::Write {"file_path":"/home/runner/work/_temp/review-metrics.yaml","content":"a|b"}
### Permission denials (1)

| Tool | Input |
| --- | --- |
| `Write` | `{"file_path":"/home/runner/work/_temp/review-metrics.yaml","content":"a\|b"}` |
$ EXECUTION_FILE=e2.json ... bash denials.sh   # result with no denials
Permission denials: 0
$ npx prettier@3 --check <changed files>   # clean after --write on the research doc

The // fix itself can only be proven by a live review run. The success signal is a run with a metrics file and Permission denials: 0 (or no Write denials).

What a reviewer should scrutinise

1. The // absolute-path semantics. This comes from Claude Code's permission docs. If the action's settings path is handled differently, the new denial log will show it on the next run.
2. The new Edit rule. Allowing Edit in the temp dir is slightly wider than strictly needed. It lets the agent update its notes file in place.

Related

• Follows fix(review-utils): inline skill partials into the review prompt #368 (inlined the partials).
• The companion fix in nsheaps/.ai-agent-henry adds the missing opened/reopened triggers.
• Example of the failure: nsheaps/agent-jordan#2

🤖 Generated with Claude Code

https://claude.ai/code/session_012K2oct3va5FBB9tW7CNCpE

---

Generated by Claude Code

[github-actions]

Plugin Version Preview

Preview only — plugin versions and marketplace.json are bumped automatically on merge to main, not in this PR. Manual bumps to higher versions are preserved. See the file annotations for the pending change on each plugin.json.

Plugin	Base	Current	Action
review-utils	0.2.13	0.2.14	Auto-bumped

[henry-nsheaps]

✅ LGTM — targeted fix with the right evidence trail

• ✅ //-form absolute-path fix is correct and well-explained; empirical evidence (run 37557540105, agent-jordan#2) backs it up
• ✅ New permission-denials step turns previously-silent failures into visible warnings — net-positive debug infra
• ✅ Template now matches review-receiver.yaml's actual secrets: + inputs: contract (event types, secret names, consumer_workflow_run_url key)
• ✅ SKILL.md edits align the agent's instructions with the new permission scope and drop a dead ${REVIEW_URL} placeholder
• ❔ One inline note on the gsub pattern (works, but reads like a no-op), one on the dead trigger-event-name receiver input (predates this PR)

Click to expand for full details

How I verified

The // fix (action.yaml). I didn't find a quotable public Claude Code docs line stating the single-vs-double-slash rule, but the PR cites a real failing run (37557540105) and a concrete symptom chain (every Write denied → metrics never written → receiver's metrics gate failed on agent-jordan#2). The rendered form //home/runner/work/_temp/** matches the standard "//abs/path" absolute-path convention several tools use for distinguishing project-relative from filesystem-absolute paths. The matching settings.permissions.allow entries use the same form. The old single-slash form in --allowedTools and settings.permissions.allow is replaced in both places. Edit is scoped the same way as Write, and the temp dir was already in additionalDirectories, so blast radius is "same dir we already shared, now with Write+Edit instead of just Read."

The permission-denials step. I pulled anthropics/claude-code-action/base-action/src/run-claude-sdk.ts and execution-file.ts:

• The execution file ($RUNNER_TEMP/claude-execution-output.json) is JSON.stringify(messages) — a raw array of SDK messages. ✅
• The result message exposes resultMsg.permission_denials as an array (the summary code reads resultMsg.permission_denials?.length ?? 0). ✅

So the step's filter .[] | select(.type == "result") | .permission_denials // [] | .[] is correct.

The gsub escape. On first read it looks like a no-op (gsub("\\|"; "\\|")), but working through jq semantics:

• Pattern "\\|" is a jq string \|, which in PCRE regex matches a literal |.
• Replacement "\\|" is a jq string \| (jq's gsub replacement treats \\ as a literal \, and | has no special meaning), so the output is the 2-char \|.
• Net effect: raw | in tool_input → \| in the markdown cell. Correct, just confusing — see the inline comment.

The template changes. I compared templates/dispatch-receiver-review.yaml against review-receiver.yaml's on: workflow_call schema at main:

• Required secrets AUTOMATION_GITHUB_APP_ID/KEY, REVIEW_GITHUB_APP_ID/KEY — ✅ matched.
• Optional LLM secrets ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN — ✅ matched (previously the template had REVIEW_ANTHROPIC_API_KEY, which the receiver would not accept).
• Inputs — ✅ trigger-event-name, trigger-event-action, consumer-workflow-run-url all map to receiver inputs that exist.
• Event-type list — the decider sends event-type: ${{ github.event_name }}/${{ github.event.action }} (from review-dispatch.yaml), so pull_request/opened|reopened|synchronize|ready_for_review|labeled|converted_to_draft is the right set. The old pr-review type is gone from both sides.

One observation on the receiver side (not this PR): trigger-event-name is declared as an input but never read (only trigger-event-action is used in the skip/review job gates), and its description is a copy-paste of trigger-event-action's. Called out in the inline comment on the template.

The research doc. 662 lines of as-is pipeline audit. No workflow references it, so it's docs-only. Not a load-bearing part of the fix.

Scoring rationale

• Quality 92%: Clean, targeted change. Clear WHY in the diff comments and PR body. Validation plan is specific (extract the step's script, run it against a sample execution file, re-run review end-to-end to prove the // fix). The one subtle gsub deserves a one-line comment.
• Security N/A: Nothing security-relevant beyond widening Write/Edit to the runner temp dir, which is already shared via additionalDirectories and ephemeral per-job.
• Simplicity 88%: The permission-rule semantics are inherently subtle and the comment block earns its length. The permission-denials step is small and self-contained.
• Confidence 90%: High on //-fix + template + SKILL.md diff (verified against the receiver's workflow_call contract and against claude-code-action's own code). Can't be 100% without seeing the next review-run execution file show Permission denials: 0.

Follow-ups:

• P2: Add a one-line comment next to the gsub so a future reader isn't fooled into thinking the escape is a no-op (inline suggestion).
• P2: review-receiver.yaml trigger-event-name is a dead input with misleading description — wire it up or drop it (follow-up PR, not this one).
• P2: After the next review run, confirm the step summary shows Permission denials: 0 (or at least no Write/Edit denials) — that's the live validation this PR couldn't do.

workflow run · anthropics/claude-code-action base-action/src/run-claude-sdk.ts · anthropics/claude-code-action base-action/src/execution-file.ts

[henry-nsheaps]

✅ The gsub("\\|"; "\\|") escaping works correctly despite looking like a no-op: in jq, the pattern "\\|" is PCRE regex matching a literal |, and the replacement "\\|" is a two-char literal \|. So raw | in tool_input becomes \| in the markdown table cell — correct.

Worth a one-line comment though — a future reader will almost certainly flag this as suspicious.

Suggested change

	jq -r '.[] | "| `\(.tool_name)` | `\(.tool_input | tostring | .[0:200] | gsub("\\|"; "\\|"))` |"' <<<"$denials"
	jq -r '.[] | "| `\(.tool_name)` | `\(.tool_input | tostring | .[0:200] | gsub("\\|"; "\\|"))` |"' <<<"$denials" # gsub escapes raw `|` -> `\|` for markdown table cells

[henry-nsheaps]

❔ The template forwards client_payload.trigger.event as trigger-event-name to review-receiver.yaml, which is correct. But in the receiver (.github/workflows/review-receiver.yaml lines 74–82) the input's description is a copy-paste of trigger-event-action ("The action field from the consumer PR event…"), and the receiver never references inputs.trigger-event-name anywhere — only trigger-event-action is read (lines 115/156). So trigger-event-name is currently a dead input with misleading docs.

Not this PR's problem (the input predates it), but worth a follow-up to either wire the receiver to it or drop the input entirely.