Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nsheaps merged 1 commit into from
Oct 7, 2026
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 14 additions & 3 deletions plugins/claude-code/review-utils/actions/run-agent/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,12 +212,23 @@ runs:
# the prompt text. The skill defines `name` + `description` for skill
# tooling — the action only needs the methodology that follows the second
# `---` delimiter.
#
# The skill's partials (formatting + thread management) are appended
# INLINE. Relying on the model to Read them by relative path was not
# reliable even with Read granted: reviews kept improvising their own
# format (observed on nsheaps/greasemonkey-scripts#54, nsheaps/ai-mktpl#746
# and nsheaps/agent-jordan#2). Inlining makes the format part of the prompt.

@henry-nsheaps henry-nsheaps Bot Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

- name: Extract prompt body from skill
shell: bash
run: |
awk 'BEGIN{p=0} /^---$/{p++; next} p>=2' \
"./agent-repo/plugins/claude-code/review-utils/skills/review-code/SKILL.md" \
> "$RUNNER_TEMP/review-prompt-template.md"
SKILL_DIR="./agent-repo/plugins/claude-code/review-utils/skills/review-code"
{
awk 'BEGIN{p=0} /^---$/{p++; next} p>=2' "$SKILL_DIR/SKILL.md"
for partial in "$SKILL_DIR"/partials/*.md; do

@henry-nsheaps henry-nsheaps Bot Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

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"

@henry-nsheaps henry-nsheaps Bot Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ 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.

echo "PROMPT_TEMPLATE=$RUNNER_TEMP/review-prompt-template.md" >> $GITHUB_ENV

# TODO: investigate a Claude Code plugin hook for internal prompt interpolation
Expand Down
Loading