fix(review-utils): inline skill partials into the review prompt

[nsheaps]

Summary

Henry's AI reviews keep ignoring the required review format. Example: nsheaps/agent-jordan#2 got flat verdict/scope/risk/tests badges and a "Scores" table instead of the Quality/Security/Simplicity/Confidence for-the-badge badges. It also had no outer <details>/<summary>, no workflow-run reference, and made-up inline-comment anchors (#discussion_r_pending1).

Root cause (confirmed by reading run-agent/action.yaml): the CI prompt contains only the body of skills/review-code/SKILL.md. That file mentions partials/review-formatting.md and partials/review-thread-management.md by relative path, and the model is expected to Read them. Granting Read permission (the earlier fix for greasemonkey-scripts#54 and ai-mktpl#746) was not enough: the model still often skips reading them and makes up a format instead.

Fix: the "Extract prompt body from skill" step now appends every partials/*.md file to the prompt template word for word, so the required format is always in the prompt.

Rollout: review-receiver.yaml checks out the plugin at plugin-ref (default main), so this applies to every org review as soon as it merges. No Renovate digest bump is needed in Henry's repo.

Test plan

• Ran the new assembly script locally. The output template contains the SKILL body, </job-context>, then both partials (243 lines).
• prettier --check passes on the changed file.
• After merge: the next review on any consumer PR uses the Quality/Security/Simplicity/Confidence badges inside a collapsible <details> and ends with the workflow-run reference.

This is the first piece of a larger cleanup of the review pipeline. The org-wide audit and as-is documentation will follow on this branch.

🤖 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.12	0.2.13	Auto-bumped

[henry-nsheaps]

✅ Targeted, well-documented fix — a few P2 nice-to-haves

• ✅ Root cause (model skipping Read even when granted) correctly diagnosed and matched by the fix
• ✅ Inline header <!-- Inlined from partials/… do not try to Read it --> prevents the model from re-reading the file
• ✅ Dropped-in-prompt approach verified empirically: this very review session's prompt contains both partials inlined, matching PR body's "243 lines" claim
• ⚠️ P2 — partials/*.md glob fails loudly if the directory ever empties; shopt -s nullglob would make intent explicit
• ❔ P2 — downstream envsubst would silently blank any future $VAR reference inside a partial. Not an issue today (checked both files), but worth hardening if this inlining pattern is intended to be durable
• ❔ P2 — SKILL.md lines 28 & 98 still say "See partials/…"; the body is self-consistent but reads awkwardly now that those partials appear below

Click to expand for full details

Scope

Changes plugins/claude-code/review-utils/actions/run-agent/action.yaml lines 212–231: the Extract prompt body from skill step previously stripped frontmatter from SKILL.md and wrote the body to $RUNNER_TEMP/review-prompt-template.md. It now additionally appends each partials/*.md file, each preceded by a --- separator and an HTML comment identifying the source file.

Single file, +14 / −3. No tests exist in the plugin for this action (shell + GitHub Actions composite). The PR author verified behavior by running the assembly locally — I verified independently by inspecting the prompt I was invoked with, which contains both partials inlined, confirming the end-to-end path works.

Correctness

The awk 'BEGIN{p=0} /^---$/{p++; next} p>=2' pattern is unchanged and correctly skips frontmatter. The appended block uses printf '\n\n---\n\n...' to ensure a blank line + horizontal rule before each partial regardless of whether the previous cat ended with a newline. Good.

Glob behavior: for partial in "$SKILL_DIR"/partials/*.md is correct — the quoted portion is literal and the glob expands only on the trailing *.md. With bash's default (nullglob off, which GitHub Actions does not override), an empty directory would yield the literal pattern and cat would exit non-zero, failing the step. The current partials/ has two files; a nullglob guard would be more a documentation improvement than a behavioral change.

Interactions with downstream envsubst

The template is handed to nsheaps/ai-mktpl/.github/actions/interpolate-prompt, which runs envsubst. I scanned both current partials for $VAR / ${VAR} references: none. review-thread-management.md does contain $(...) inside a backticked gh pr view --json url --jq .url fragment, but envsubst does not treat $(...) as substitutable — only $VAR and ${VAR} — so this is safe.

The silent-blanking risk only materializes if a future partial drops a literal $VAR into a code example (e.g. $HOME, $GITHUB_TOKEN). Mentioned inline as P2.

Documentation accuracy

The added comment block (lines 215–220) is precise: it names the specific prior failure cases (greasemonkey-scripts#54, ai-mktpl#746, agent-jordan#2) and explains why granting Read alone was insufficient. This is exactly the kind of context a future maintainer needs to resist "simplifying" the step back to a plain awk call.

Minor inconsistency: SKILL.md lines 28 and 98 still say "See partials/review-thread-management.md / partials/review-formatting.md". The new inline header mitigates confusion ("do not try to Read it"), but a short edit to those two lines in a follow-up would make the SKILL body self-consistent.

PR description accuracy

• "The output template contains the SKILL body, </job-context>, then both partials (243 lines)" — matches what I received
• "Rollout: review-receiver.yaml checks out the plugin at plugin-ref (default main)" — not verified in this PR (file unchanged), but consistent with how I understand the system

Simplicity

Shell is longer than before but each line is still doing one obvious thing. The alternative (a separate assembly script in scripts/) would be worse — one more indirection to understand the prompt. Keep inline.

Security

No new attack surface. Partial contents are prompt text; a malicious partial would need to already be in the plugin repo. No new secrets, tokens, or network calls.

Follow-ups:

• P0: none
• P1: none
• P2:
  • Add shopt -s nullglob to make empty-partials behavior explicit (inline comment above)
  • Guard against future partials introducing $VAR references that get blanked by envsubst (lint check or escape convention)
  • Update SKILL.md lines 28 & 98 to point at the inlined content below rather than at relative partial paths

References

• Workflow run: https://github.com/nsheaps/.ai-agent-henry/actions/runs/37563186948
• Prior failure cases cited in the PR body (all verified via PR metadata): nsheaps/greasemonkey-scripts#54, nsheaps/ai-mktpl#746, nsheaps/agent-jordan#2
• envsubst substitution scope (confirms $() is not expanded): GNU gettext manual — envsubst

Note on tools

• mcp__github__get_me returned 403 Resource not accessible by integration under this bot token. Did not affect the review — identity is implicit from the workflow context.
• Write(/home/runner/work/_temp/review-metrics.yaml) was denied in this session despite the documented Write(${{ runner.temp }}/**) allowlist. Flagging outside the details block in case this is the same silent-denial pattern the PR body's "Rollout" claim is meant to avoid elsewhere.

[henry-nsheaps]

⚠️ P2 — glob fails loudly if partials/ ever becomes empty.

With the default bash shell flags that GitHub Actions uses (set -eo pipefail), an empty partials/ directory means the glob falls back to the literal string ./.../partials/*.md, which cat can't open, and the step exits non-zero. That's loud (good) but surprising for anyone who notices the glob and assumes it tolerates emptiness. Low priority since the dir has two files and will likely always have at least one, but a one-liner would make intent explicit:

Suggested change

	for partial in "$SKILL_DIR"/partials/*.md; do
	SKILL_DIR="./agent-repo/plugins/claude-code/review-utils/skills/review-code"
	shopt -s nullglob
	{
	awk 'BEGIN{p=0} /^---$/{p++; next} p>=2' "$SKILL_DIR/SKILL.md"
	for partial in "$SKILL_DIR"/partials/*.md; do
	printf '\n\n---\n\n<!-- Inlined from partials/%s (the file referenced above; do not try to Read it) -->\n\n' "$(basename "$partial")"
	cat "$partial"
	done
	} > "$RUNNER_TEMP/review-prompt-template.md"

Alternatively leave as-is: the loud failure is arguably the right behavior if someone accidentally deletes the partials dir and ships a prompt with no formatting rules at all.

[henry-nsheaps]

❔ P2 — assembled template now flows through envsubst in the downstream interpolate-prompt step.

The partial contents are concatenated into review-prompt-template.md, which interpolate-prompt then envsubsts. Today's partials have no $VAR / ${VAR} references, so this is fine. But any future partial that happens to drop a literal $VAR into a code block (e.g. an example shell snippet referencing $HOME, $GITHUB_TOKEN, etc.) will get silently blanked. If this fix is intended to be durable, consider either:

1. Running the partials through their own escape pass before concatenating (e.g. sed 's/\$/$$/g' is a non-starter for envsubst, so you'd need ${DOLLAR} convention + a prelude export), or
2. Adding a lint check that fails CI if a partial contains $-prefixed identifiers.

Not blocking — just worth noting since the whole point of this PR is removing a silent-failure mode. Leaving a different one in place would be unfortunate.

[henry-nsheaps]

✅ Comment block clearly captures the root cause (model skipping Read even when granted), lists the concrete observations (greasemonkey-scripts#54, ai-mktpl#746, agent-jordan#2), and names the fix (inline). This is exactly the context a future maintainer needs to understand why the step grew. Nice.

Minor follow-up: SKILL.md lines 28 and 98 still say "See partials/review-formatting.md..." / "See partials/review-thread-management.md...". Those references now point at content the model will see inlined below. The inline header you add (do not try to Read it) handles the "don't duplicate the read" case well, but a brief edit to SKILL.md to change "See partials/..." → "See the inlined partials appended below" would make the SKILL body self-consistent and avoid future contributors re-introducing the Read dependency. Not required.