feat(source-control): configurable pr_body_required_sections scaffold + gate (#975)

[kyle-sexton]

Closes #975

Summary

Externalizes the PR-body section scaffold (## Summary / ## Test plan / ## Related) that was
previously hardcoded into create.md's template, so the pull-request skill no longer imposes the
## Related section — and the issue-tracker convention it presumes — on every consumer.

• New config key pr_body_required_sections on .claude/source-control.md, resolved across the
  same three layers as every other key on that surface (per-key, whole-list override) —
  plugins/source-control/reference/config-resolution.md.
• Portable default when no layer sets the key: Summary and Test plan only (no Related) —
  research-grounded (GitHub's own guidance, Google's CL-description doc, GitLab's dogfooded default
  template, a cross-section of OSS PR templates); a Related/linked-issue section presumes an
  issue-tracker convention, so it moved from a hardcoded default to configuration.
• §2.4.2 pre-create gate extended (now §2.4.2.1 closing-keyword + §2.4.2.2 required-sections) —
  the new check verifies every resolved required section is present and non-empty in the assembled
  body, naming the exact missing section and the resolved config source (winning layer's file + the
  key) on failure, so an actor who never saw the convention learns where it lives from the failure
  itself. The scan runs BEFORE the attribution footer is appended (an empty last section can't be
  masked by footer text) and is fence- and HTML-comment-aware (a heading-shaped line inside a code
  sample or a commented-out draft never counts as a real section boundary) — both review-caught.
• Genuine Refs #Y references are never dropped — a ## Related section is still emitted ad hoc
  to carry them even when Related is not configured as required, but that ad hoc section is never
  added to the gate's required list. Related's content resolution (REFS_LINES vs. the N/A
  default) is factored into one helper reused by both the required and ad hoc paths, so they can't
  disagree — also review-caught.
• New marketplace-level owner doc docs/conventions/pr-body-convention/README.md, modeled on the
  commit-convention seam's two-reads pattern (drafting today, a future CI/enforcement consumer
  later) and pointing at config-resolution.md for resolution mechanics rather than restating them.
• Cross-plugin contract fixed: work-items' work skill dispatch brief listed ## Related as a
  standing PR obligation, matching the OLD always-present behavior this PR replaces. Updated to point
  at the new configurable scaffold instead of restating it, and to ensure the section exists (via
  gh pr edit) before citing a deferred-finding follow-up issue, since it can no longer assume
  pull-request already created one. work-items bumped 0.21.2 → 0.21.3 with a matching entry.
• Zero melodic-specific content ships in the plugin. The portable default is agnostic; a repo
  wanting Related (or any other section) declares it in its own pr_body_required_sections. This
  repo (claude-code-plugins) itself now does exactly that — see "Dogfooding" below — as repo-local
  configuration DATA at the seam the mechanism defines, not plugin content.
• Deferred, recorded with a trigger: richer per-section schema — conditional sections,
  placeholder-text rejection, min-content rules — pending melodic-software/standards#173.

Plugin version bumped 0.17.0 → 0.18.0 with a matching CHANGELOG.md entry.

Dogfooding (first fleet-adoption instance)

This repo has no .github/workflows/pr-issue-linkage.yml-satisfying config today, and the new
portable default would make every ordinary future /pull-request create here omit ## Related —
breaking this repo's own required linkage gate. Self-regression atomicity: a change that would break
this repo's own CI ships with its own remedy in the same PR. Added .claude/source-control.md (team
layer, root) setting only pr_body_required_sections (Summary, Test plan, Related, matching this
repo's actual gate) — every other key still falls through to inference. Also added the recursive
.claude/**/*.local.* gitignore line this file's own convention needs, which the repo did not carry
yet. This is the first fleet-adoption instance of the key by hand; every other consuming repo adopts
it the ordinary way, via /source-control:setup apply.

Test plan

• npx markdownlint-cli2 over every changed/added Markdown file: 0 errors.
• node -e "JSON.parse(...)" over the updated evals/evals.json: valid JSON; added evals 12-16
  covering the portable default, the config-driven gate blocking a missing required section, the
  attribution-footer-masking regression, and the fenced-code/HTML-comment heading-lookalike
  regressions — plus a new fixture evals/fixtures/source-control-required-related.md.
• bash scripts/check-changelog-parity.sh --check-bump origin/main: PASS (both version bumps —
  source-control and work-items — have matching entries).
• bash scripts/check-orphaned-fixtures.sh: the new eval fixture shows CONSUMED, not orphaned.
• bash scripts/check-skill-portability.sh origin/main: no unexcused coupling tokens introduced.
• bash scripts/check-silent-skips.sh origin/main and bash scripts/check-cross-plugin-source-drift.sh:
  clean; no new drift.
• bash scripts/validate-plugins.sh: every plugin manifest and the marketplace catalog validate.
• Confirmed via grep (whole repo, not just plugins/source-control) that ## Related appeared
  only in create.md, work-items' work skill, and historical CHANGELOG.md entries before
  editing — caught and fixed the work-items cross-plugin coupling the first grep pass (scoped to
  plugins/source-control only) missed; this pass was widened after review.
• Verified skill-quality:check's eval schema validation (the only CI-run eval check) never executes
  eval prompts against live repo state — it is schema-only (check-jsonschema against
  evals.schema.json) — so eval 12's "no source-control.md config file exists in this repo" scenario
  framing is unaffected by this repo now carrying a real one; the two never interact.
• No reference-doc test harness exists for create.md (.test.sh files cover scripts/ only) —
  prose consistency and intact §-numbering are the deliverable here; reviewed the full file end to
  end after every edit.
• Three review passes (7 findings, all fixed) exercised this PR harder than a single self-review
  would have: a real routing bug, an attribution-masking bug, two heading-lookalike bugs, the
  cross-plugin contract break, the setup-skill gap (tracked separately, setup skill: offer pr_body_required_sections in interview/write template #1032), and this dogfooding
  gap.

Related

• Interview-locked decisions for this issue (2026-07-22 comments on pull-request skill: pre-create gate must verify non-empty '## Related' section #975) — config seam, key shape,
  portable default, gate-must-teach requirement, and the standards#173 convergence trigger.
• melodic-software/standards#173 — PR convention policy-as-data; the deferred richer-schema trigger
  for this seam.
• docs/conventions/commit-convention/README.md — the two-reads (drafting/enforcement) prior art this
  PR's new owner doc follows.
• setup skill: offer pr_body_required_sections in interview/write template #1032 — follow-up: /source-control:setup gains check/apply support for
  pr_body_required_sections, completing the key's adoption path for every consumer that isn't this
  repo.

🤖 Generated with Claude Code

[claude]

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

---

PR Review

• Gather context and read changed files
• Review create.md — gate logic, section resolution, §-numbering
• Review config-resolution.md — new key documentation
• Review docs/conventions/pr-body-convention/README.md — new owner doc
• Review evals — new eval cases and fixture
• Review plugin.json, CHANGELOG.md, PLUGIN-PHILOSOPHY.md
• Post findings

---

This is well-designed and internally consistent. No blocking defects. Three items worth addressing before merge, one of which is a genuine functional gap:

---

Notable gap: /setup skill is not updated

pr_body_required_sections is now a first-class key on the source-control.md config surface, but the /setup check report and /setup apply interview don't know about it. Concretely:

• The check output table (key / value / won by) will never surface pr_body_required_sections even when a consumer has set it.
• The apply interview guidance stops at pr_body_attribution — operators using /setup apply cannot persist the new key through the guided flow.
• The written config template also has no slot for pr_body_required_sections.

The PR description says "mechanism + docs only" and explicitly defers dogfooding — that's a fine scope boundary for the plugin repo itself. But the setup skill gap is not a matter of dogfooding; it's the discoverability and write-path for the feature in consuming repos. A consumer running /setup check today on a repo that has pr_body_required_sections set in its team layer will get a check report that silently omits the effective section list. A consumer running /setup apply has no guided path to write the key.

This seems like an oversight rather than a deliberate deferral (the PR description doesn't call it out, whereas the standards#173 deferral is explicitly named). Worth deciding: either update /setup in this PR or add a TODO(#975-followup) note to SKILL.md recording the gap. Fix this →

---

Minor: awk exact-match could silently miss a heading with trailing whitespace

The gate at create.md:331:

$0==h {found=1; next}

uses string equality, so ## Summary (trailing space) doesn't match ## Summary. The heading is always generated cleanly by the template loop ("## ${section}"$'\n\n'), so in practice the match is fine. But if the model ever emits a heading with trailing punctuation or whitespace, the gate silently reports the section as "missing" and blocks creation with a misleading error.

A small robustness improvement — using a regex anchor instead of equality:

/^## / && $0 ~ ("^## " h_bare "$") {found=1; next}

or simply trimming $0 before comparing — would make the check more forgiving. Not urgent given the deterministic template, but worth noting.

---

Cosmetic: misaligned continuation comment on REFS_LINES declaration

At create.md:150-151:

REFS_LINES=""  # newline-separated `Refs #Y — <why>` lines, populated by the multi-issue or
                # orphan-PR prompts below; never a closing keyword — see §2.4.1 for routing.

The # on the continuation line sits one space to the right of the # on the opening line (16 vs 17 chars of leading whitespace). Cosmetic in pseudocode, but an easy fix.

---

Things that look correct

• Injection safety — REFS_LINES is assembled via parameter expansion ("${REFS_LINES}" concatenated into TEMPLATE, then "$TEMPLATE" into BODY). No shell re-evaluation path. The security rationale at create.md:263-273 correctly names REFS_LINES alongside CLOSES_LINE in the injection discussion.
• awk body extraction logic — correctly handles last-section (no next ## heading), H3/H4 inside a section (not confused with H2 delimiters), and whitespace-only content (tr -d '[:space:]' catches all-blank sections).
• Double-Related prevention — grep -qx "Related" correctly blocks emitting an ad hoc ## Related when Related is already in REQUIRED_SECTIONS, preventing duplicate sections.
• Gate messages — failure names both the exact missing section(s) (${MISSING_SECTIONS[*]}) and the resolved config source (${REQUIRED_SECTIONS_SOURCE}). A consumer who never read the convention gets both the what and the where on first failure.
• Breaking-change handling — ## Related removal from the default scaffold is correctly categorized under "Changed" in the CHANGELOG (not buried under Added). Pre-1.0.0 semver minor bump (0.17.0 → 0.18.0) is correct. The CHANGELOG entry at 0.18.0 is explicit that existing consumers needing Related must declare it.
• Owner doc placement — docs/conventions/pr-body-convention/README.md at marketplace level (not plugin level) matches the established commit-convention seam pattern and correctly anticipates the future CI/enforcement consumer.
• Evals — eval 12 (portable default, no ## Related) and eval 13 (gate blocks missing Related) cover the two new behaviors. Fixture is clearly marked as generic, not melodic policy.
• §-numbering — 2.4.2 Pre-create gate with subsections 2.4.2.1 and 2.4.2.2 is clean. The owner doc's cross-references (§2.4.1 (assembly) / §2.4.2 (gate)) match the actual headings.

[claude]

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

---

Security Review — PR #1029

• Load tools
• Fetch PR diff
• Read changed files
• Analyze for security issues
• Post findings

---

Findings

No security vulnerabilities found.

The eight changed files span markdown docs, a JSON manifest version bump, eval fixtures, and instruction prose. There is no new executable code (no shell scripts, no GitHub Actions workflows, no server-side logic). The security-relevant portions are the shell snippets embedded in create.md as instruction text for the Claude agent to follow — those are analyzed below.

AWK -v assignment from config-resolved section name (§2.4.2.2)

SECTION_BODY=$(printf '%s\n' "$BODY" | awk -v h="## ${section}" '
  $0==h {found=1; next}
  ...
')

Permalink

${section} is config-resolved at the model level from operator-written markdown. The awk -v h= assignment is a string binding, not a code-evaluation path. The comparison $0==h is a string equality check, not a regex — no awk-code-injection surface. No issue.

A minor behavior quirk: awk interprets \n, \t, etc. in -v values, so a section name containing a literal backslash (e.g. Test\nPlan) would silently mismatch. This is a correctness edge case under adversarial operator config, not exploitable, and not a security finding.

Shell injection through $BODY assembly (§2.4.1)

The PR documents the injection analysis explicitly:

Parameter expansion of ${VAR} does NOT re-evaluate the expanded value — if CLOSES_LINE or REFS_LINES contains $(rm -rf ~) (e.g. user typed it into the orphan-PR prompt) … it stays a literal string and is never executed.

The concatenation via TEMPLATE+= and BODY+= (bash parameter expansion) is correct. Unquoted heredoc <<EOF is deliberately avoided for this reason. ATTRIBUTION is single-quoted at the assignment site. The analysis is accurate and the mitigation is sound.

The new REQUIRED_SECTIONS_SOURCE string (a file path emitted only to stderr on gate failure) is informational only and has no downstream execution surface.

gh pr create --body "$BODY" (§2.4.3)

The assembled body — including any ad hoc ## Related section carrying user-supplied REFS_LINES — is passed as a shell-quoted argument to gh. The gh CLI receives this as data, not as a command. GitHub sanitizes the rendered Markdown. No injection surface from the new required-sections content or the ad hoc Related section.

Config layer trust

pr_body_required_sections is read from operator-controlled files (~/.claude/source-control.md, the team-tracked .claude/source-control.md, and the gitignored overlay). These are in the same trust tier as the rest of that config surface — no new trust boundary is opened.

---

Summary: The new pr_body_required_sections key, its three-layer resolution, the AWK-based section-body extraction, and the §2.4.2.2 gate all follow the injection-safe patterns already established in this skill. No CRITICAL, IMPORTANT, or SUGGESTION-level security findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1c9eecb78

ℹ️ 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".

[kyle-sexton]

🔒 merge-drive lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, worker tier). Will fix clear branch-owned CI/bot findings, defer non-DEFER-FORBIDDEN items with reply+resolve, push, then re-check the merge gate.

[claude]

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

---

PR Review — feat/975-required-body-sections

• Load tools
• Check what changed since last review
• Read changed files on current HEAD
• Verify prior findings are addressed
• Assess Codex P2 findings
• Post findings

---

Sync note: The synchronize event was a merge-from-main that only touched plugins/disk-hygiene/ — no changes to source-control. All prior findings remain open.

---

Confirmed bug: REFS_LINES silently discarded when Related is a required section

This is a new confirmation of Codex P2 #2. When a team sets Related in pr_body_required_sections, the loop at create.md:235-237 renders ## Related with the N/A default (per the prose at create.md:222-223: "Related → the literal N/A… when nothing more specific applies"). Simultaneously, the ad hoc append at create.md:238-240 is skipped because Related IS in REQUIRED_SECTIONS:

if [[ -n "$REFS_LINES" ]] && ! printf '%s\n' "${REQUIRED_SECTIONS[@]}" | grep -qx "Related"; then
  TEMPLATE+="## Related"$'\n\n'"${REFS_LINES}"$'\n\n'   # only fires when Related is NOT required
fi

The prose at lines 225-229 addresses only the "not required" case — it never states what to do when REFS_LINES is non-empty AND Related IS required. A model following these instructions in a repo that requires Related will emit N/A even when the user supplied Refs #Y references in the §2.4.0 multi-issue/orphan prompt, silently dropping those refs.

Fix: extend the prose at create.md:222-224 to add: "If REFS_LINES is non-empty and Related IS in REQUIRED_SECTIONS, use REFS_LINES as the body of that section instead of N/A." Fix this →

---

Still open: /setup skill does not expose pr_body_required_sections

As noted in the prior review, the check table at setup/SKILL.md:50-56 lists subject_pattern, pr_title_pattern, trailer_policy, and pr_body_attribution — no row for pr_body_required_sections. The apply interview ends at pr_body_attribution (SKILL.md:220-224) and the generated config template at SKILL.md:227-257 has no slot for the new key. A consumer running /setup check on a repo that already has pr_body_required_sections set will get a silent omission.

This remains the most actionable gap. Please either add it to /setup in this PR or add an explicit TODO(#975-followup) comment in SKILL.md so the gap is recorded. Fix this →

---

Assessment of Codex P2 #1 (work-items Related handoff)

The Codex finding overstates the breakage. Deferred findings from /work-items:work are collected as Refs #Y lines in REFS_LINES via the multi-issue prompt (§2.4.0). The ad hoc ## Related path at create.md:238-240 fires whenever REFS_LINES is non-empty and Related is not required — so the references do land in the PR body. The real concern is softer: the work-items brief at work/SKILL.md:183 lists ## Related as a structural expectation alongside Closes #N, which a worker may interpret as requiring the section to always be scaffolded rather than emitted ad hoc. A brief clarifying note in that brief (pointing to the ad hoc path in create.md) would close the ambiguity. Not a functional bug; a documentation alignment issue.

---

Still open: awk exact-match at §2.4.2.2

create.md:332 still uses $0==h — a heading with trailing whitespace silently reads as "missing". Low risk given the deterministic template, but worth noting as a robustness gap.

---

Still open: cosmetic comment misalignment at line 152

create.md:152-153 — the continuation # sits one character right of the opening #. Trivial.

---

Blocking verdict

The REFS_LINES-drop bug (P2 #2 above) is a correctness defect that is reproducible in any repo where Related is configured as a required section and the user supplied Refs #Y lines during §2.4.0. It should be fixed or explicitly triaged before merge. The /setup gap is the next priority. The remaining items (awk, cosmetic) can follow.

[claude]

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

---

Security Review — PR #1029

• Load tools and get PR diff
• Read changed security-relevant files
• Analyze for injection surfaces, auth gaps, dangerous patterns
• Post findings

---

Findings

No security vulnerabilities found.

The synchronize event adds a single merge commit (e1204ad) that brings origin/main into the branch. That merge touches only files outside the 8 PR-changed files (disk-hygiene, claude-memory, toolchain plugins). The PR's security surface — entirely within create.md — is unchanged from b1c9eec. The analysis below is a complete independent review of the new code paths.

---

New path 1 — REFS_LINES collection and assembly

REFS_LINES is populated from user prompt input (multi-issue or orphan-PR flows) and spliced into TEMPLATE via bash parameter expansion:

TEMPLATE+="## Related"$'\n\n'"${REFS_LINES}"$'\n\n'

Parameter expansion of "${REFS_LINES}" does not re-evaluate the expanded value. A user-typed $(rm -rf ~) or `cmd` inside REFS_LINES is inert — the existing rationale at create.md:263–273 is correct and now explicitly names REFS_LINES alongside CLOSES_LINE. No injection surface.

New path 2 — awk -v h="## ${section}" in the §2.4.2.2 gate

SECTION_BODY=$(printf '%s\n' "$BODY" | awk -v h="## ${section}" '
  $0==h {found=1; next}
  found && /^## / {exit}
  found {print}
')

create.md:331–335

${section} is model-resolved from operator-controlled config. The awk -v flag performs a string binding, not code evaluation. The comparison $0==h is string equality — it does not execute h's content as awk code. A section name like $(evil) would be set as a literal string in h and compared with ==. No awk code injection path exists.

One known behavior: awk processes \n, \t, \\, etc. in -v values. A section name containing a literal backslash (adversarial config only) would cause silent mismatch and a false "missing section" gate failure. This is a correctness edge case (available DoS under malicious operator config, not privilege escalation or data exfiltration) and not a security finding.

New path 3 — grep -qx "Related" duplicate-prevention check

! printf '%s\n' "${REQUIRED_SECTIONS[@]}" | grep -qx "Related"

create.md:238

"Related" is a hardcoded string literal — the section names in ${REQUIRED_SECTIONS[@]} are pipe-fed as input lines, not as the grep pattern. No user/config value reaches the pattern argument. No injection surface.

New path 4 — ${REQUIRED_SECTIONS_SOURCE} in error output

Emitted exclusively to stderr in the gate failure message. This string (a file path or "plugin default" description resolved at model level) has no downstream execution surface. It is never embedded in a shell command, heredoc, or gh call.

Config layer trust

pr_body_required_sections is read from the same three source-control.md layers as every other key on that surface (user-global, team-tracked, gitignored overlay). The trust tier is unchanged — operator-level config, same as type_list, pr_body_attribution, etc. No new trust boundary is opened by this key.

---

Summary: The new REFS_LINES path, the ${REQUIRED_SECTIONS[@]} loop, the awk section-body extraction, and the §2.4.2.2 gate all follow the injection-safe patterns already established in this skill. The merge commit adds no new security-relevant content to the PR's changed files. No CRITICAL, IMPORTANT, or SUGGESTION-level security findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e1204ade88

ℹ️ 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 3m 12s —— View job

---

PR Review — feat/975-required-body-sections (post-amendment re-review)

• Load tools and check what changed since last review
• Read create.md — verify prior findings addressed
• Read setup/SKILL.md — check if pr_body_required_sections was added
• Assess new Codex finding (attribution / empty-section gate defeat)
• Post findings

---

The amendment commit (482ef4d) closes two of the three prior blocking findings. Two issues remain — one new, one still-open — that should be resolved before merge.

---

Fixed: REFS_LINES-drop bug ✅

The content_for_section() helper at create.md:237–242 correctly unifies the resolution path for Related's content ($REFS_LINES when non-empty, else N/A), and both the required-section loop and the ad hoc append now call the same resolver. The prose at lines 221–232 is clear that there is exactly one place Related's content is decided. Finding closed.

Fixed: work-items cross-plugin alignment ✅

work/SKILL.md no longer lists ## Related as a standing dispatch-brief obligation. The deferred-finding path now includes explicit gh pr edit guidance for cases where the section doesn't already exist. evals.json expectation updated to match. Finding closed.

---

Still open: attribution defeats the §2.4.2.2 gate for the last required section

The gate at create.md:339–359 runs against $BODY, which is assembled at lines 282–285 including the attribution line:

BODY=""
[[ -n "$CLOSES_LINE" ]] && BODY="${CLOSES_LINE}"$'\n\n'
BODY+="$TEMPLATE"
[[ -n "$ATTRIBUTION" ]] && BODY+=$'\n\n'"$ATTRIBUTION"   # appended before gate runs

The awk extractor prints everything from ## <last-section> to either the next ## heading or end-of-body. For the final section in ${REQUIRED_SECTIONS[@]}, there is no next ## heading — so SECTION_BODY captures any trailing whitespace plus the attribution text (🤖 Generated with [Claude Code]…). After tr -d '[:space:]' the attribution string is non-empty, so the emptiness check [[ -z "..." ]] returns false, and an actually-empty final section passes the gate silently.

Concrete scenario: repo configures pr_body_required_sections: [Summary, Test plan]; model emits a real ## Summary but an empty ## Test plan; pr_body_attribution is at its default. Gate checks ## Test plan, awk picks up 🤖 Generated with… as section body, gate passes, PR is created with no test plan. The very case the gate exists to catch.

Fix: run §2.4.2.2 against $BODY before $ATTRIBUTION is appended, or split body assembly so attribution is only concatenated after the gate passes. A minimal restructure:

# Gate runs on BODY_GATED (no attribution yet).
BODY_GATED=""
[[ -n "$CLOSES_LINE" ]] && BODY_GATED="${CLOSES_LINE}"$'\n\n'
BODY_GATED+="$TEMPLATE"

# §2.4.2 gate — runs here, before attribution is appended.
# ... (gate code unchanged, but replace $BODY with $BODY_GATED)

# Only assemble the final body (with attribution) after both gate checks pass.
BODY="$BODY_GATED"
[[ -n "$ATTRIBUTION" ]] && BODY+=$'\n\n'"$ATTRIBUTION"

Fix this →

---

Still open: /setup skill does not expose pr_body_required_sections

This finding was raised in the first review and again in the second. The amendment round did not address it — no update to the check table, no update to the apply interview, no update to the generated config template, and no TODO note added to SKILL.md to acknowledge the gap.

The check table at setup/SKILL.md:50–56 still lists only subject_pattern, pr_title_pattern, trailer_policy, and pr_body_attribution. A consumer who runs /setup check on a repo where pr_body_required_sections is set will get a silent omission. A consumer running /setup apply has no guided path to write the new key.

Please either:

• Add pr_body_required_sections to the check table, the apply interview (after pr_body_attribution), and the generated config template; or
• Add a TODO(#975-followup) note at the check table and interview sections recording the gap explicitly — at minimum the gap should be visible in the source rather than invisible.

Fix this →%20the%20check%20output%20table%20at%20lines%2050-56%2C%202)%20the%20apply%20interview%20guidance%20after%20pr_body_attribution%20around%20line%20220%2C%20and%203)%20the%20written%20config%20template%20around%20line%20253.%20Branch%3A%20feat%2F975-required-body-sections%2C%20repo%3A%20melodic-software%2Fclaude-code-plugins.&repo=melodic-software/claude-code-plugins)

---

Low-priority: awk exact-match and cosmetic alignment (unchanged)

These two items from prior reviews are still open but non-blocking:

• awk $0==h at create.md:344 — string equality silently misses a heading with trailing whitespace; low risk given the deterministic template.
• Comment continuation alignment at create.md:152–153 — continuation # sits one column right of the opening #.

---

Blocking verdict

Two items before merge:

1. Gate defeated by attribution (new, functional defect) — the §2.4.2.2 check silently passes an empty last required section when attribution is present. Straightforward fix: run the gate before attribution is concatenated into $BODY.
2. /setup gap unacknowledged — either fix the skill or add a TODO note; the gap is currently invisible in the source.

[claude]

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

---

Security Review — PR #1029 (HEAD 0a75d72)

• Load tools and get PR diff
• Read changed files on current HEAD
• Analyze for injection surfaces, auth gaps, dangerous patterns
• Post findings

---

Findings

One SUGGESTION-level finding. No CRITICAL or IMPORTANT vulnerabilities.

The changed files are: two new markdown docs, two JSON manifest version bumps, eval fixtures, and instruction prose in create.md and work/SKILL.md. No GitHub Actions workflows, no new shell scripts, no server-side code. The security surface is the shell pseudocode embedded in create.md that Claude agents execute as instructions.

---

SUGGESTION (high confidence) — Security-rationale comment misidentifies $(content_for_section) as a "heredoc segment"

create.md:244–248

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).
TEMPLATE=""
for section in "${REQUIRED_SECTIONS[@]}"; do
  TEMPLATE+="## ${section}"$'\n\n'"$(content_for_section "$section")"$'\n\n'
done

The comment claims "nothing inside expands," but $(content_for_section "$section") is a bash command substitution — it executes the function. The comment's label ("quoted heredoc segment") and its stated rationale ("inert; nothing inside expands") are factually wrong for this line.

The code is safe, but for a different reason than the comment states:

1. content_for_section uses printf '%s' "$REFS_LINES" — user-supplied content is in the argument position, not the format string, so format specifiers in $REFS_LINES are emitted literally.
2. The function's return value is then stored in $TEMPLATE via bash +=, which does not re-evaluate the stored string. User-supplied $(…) or ${…} in $REFS_LINES survive as literal text.

The actual injection defence is "function output stored via += is never re-evaluated" — exactly the same guarantee the ${CLOSES_LINE} concat path already carries. The "heredoc inert" framing is a mislabelling of that guarantee.

Why this matters: create.md doubles as its own security rationale document (the # Quoted heredoc segments + concat section at line 288 is the canonical explanation of the injection-safe design). A future Claude agent or human reading "nothing inside expands" and applying it to a new $(…) call — especially one that passes config-resolved or user-supplied input to an eval-containing function — would reach the wrong conclusion about safety. The current content_for_section function is carefully constrained; the comment implies the safety comes from the call site, not from the function's internals.

Suggested fix: Correct the comment to accurately state why the pattern is safe:

# Per-section content is assembled via command substitution ($(content_for_section ...)),
# then stored in $TEMPLATE via +=. Bash += does NOT re-evaluate the stored string, so
# user-supplied content in $REFS_LINES (including any literal $(…) or ${…}) is inert
# once inside $TEMPLATE. Safety guarantee: same as the $CLOSES_LINE concat path below.

---

Confirmed safe: all other new paths

• awk -v h="## ${section}" (create.md:343): -v is a string binding; $0==h is string equality. No awk code injection. Prior review's finding on backslash in section names (silent mismatch under adversarial config) remains a correctness edge case only.
• grep -qx "Related" with ${REQUIRED_SECTIONS[@]} piped as input (create.md:250): "Related" is the hardcoded pattern; config values are piped as input, never as pattern. Safe.
• printf '%s' "$REFS_LINES" inside content_for_section (create.md:239): user content is the argument, not the format string. Format specifiers in $REFS_LINES are output literally.
• ${REQUIRED_SECTIONS_SOURCE} in stderr (create.md:355): informational only; no downstream execution surface.
• gh pr create --body "$BODY" (create.md:377): $BODY is a double-quoted argument to gh; GitHub sanitises rendered Markdown. No injection.
• work/SKILL.md changes: prose only — no new shell execution paths or trust boundary changes.
• No GitHub Actions workflows were modified.

[kyle-sexton]

Classification of the three items in the review above. Freshen-merge to current main pushed as e1204ade (clean fast-forward, disk-hygiene changes only — disjoint from this PR's files). No scope-expanding fixes made autonomously; all three items are advisory and deferred to the author:

#	Finding	Classification	Evidence	Reacted
1	/setup skill not updated for pr_body_required_sections (check table, apply interview, config template)	VALID (defer)	Confirmed against setup/SKILL.md; same gap codex raised inline. Scope expansion into a sibling skill beyond this PR's "mechanism + docs only" scope — recommend folding into this PR or a TODO(#975-followup).	👍
2	awk $0==h exact-match could miss a heading with trailing whitespace	VALID (defer, low value)	Accurate, but the template loop ("## ${section}"$'\n\n') always emits clean headings, so it is a robustness nicety, not a live defect. Regex-anchor/trim hardening is optional; not applied, to avoid churn in the injection-reviewed block.	👍
3	Misaligned continuation comment on REFS_LINES (create.md:150-151)	VALID (defer, cosmetic)	Real 1-space misalignment in pseudocode. Deliberately left out of the freshen-merge push to keep that push a clean merge; trivial for the author to fold into a later edit.	👍

No blocking defects. Security Review (separate claude[bot] comment) passed clean with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a75d72284

ℹ️ 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 3m 3s —— View job

---

PR Review — feat/975-required-body-sections (HEAD cd501a7)

• Check what changed since last review (commit cd501a7)
• Read create.md — verify attribution/gate fix
• Check /setup skill gap status
• Assess Codex P2 (fenced-code headings) and Codex P1 (repo's pr-issue-linkage CI requirement)
• Post findings

---

The latest commit (cd501a7, "keep attribution footer out of the required-section gate scan") closes the last blocking defect from the prior review. Two items remain — one still-open, one new.

---

Fixed: attribution defeating §2.4.2.2 gate ✅

$ATTRIBUTION is now only spliced into $BODY inside §2.4.3, after both gate checks pass (create.md:372–379). The §2.4.1 comment explains the structural rationale for why attribution must stay out of the gated body (create.md:281–288). Eval 14 covers the regression. Fix is correct and structurally sound. Finding closed.

---

Still open (4th raise): /setup skill does not expose pr_body_required_sections

This gap has been raised in every review cycle without resolution or acknowledgment.

The check table at setup/SKILL.md:50–56 still lists subject_pattern, pr_title_pattern, trailer_policy, and pr_body_attribution — no row for pr_body_required_sections. The apply interview ends at pr_body_attribution (SKILL.md:220–224), and the generated config template (SKILL.md:227–257) has no slot for the new key.

At minimum: add a TODO(#975-followup) note at the check table so the gap is visible in the source rather than invisible to the next person who reads the skill. Fix this →

---

New (Codex P2): fenced-code headings can satisfy the §2.4.2.2 gate

The awk at create.md:349–353 has no concept of fenced code blocks:

$0==h {found=1; next}
found && /^## / {exit}
found {print}

If $BODY contains a fenced code block whose content includes ## Related (e.g., a Summary section that documents a PR template), the gate matches that interior heading as the required Related section and reads the surrounding code as its body — a false negative that passes the gate even though no real ## Related section exists in the PR body. This matters in practice for documentation-heavy PRs (documenting templates, configuration examples) but is not an injection surface and cannot be triggered adversarially without operator cooperation.

Assessment: valid, low-priority, non-blocking. The template loop always generates clean headings, so the common path is unaffected; this only misfires on an authored body with mismatched fenced content. Deferred per the standards#173 convergence note (richer per-section schema). Worth recording but not a merge blocker. Fix this →

---

New (Codex P1 — context, not a plugin defect): this repo's pr-issue-linkage CI requires ## Related

pr-issue-linkage.yml in this repo validates every PR body for a non-empty ## Related section (confirmed via /.github/workflows/pr-issue-linkage.yml). With no .claude/source-control.md configured here, the portable default (Summary + Test plan only) would produce PR bodies that fail that CI check. This is not a plugin defect — the PR is correct that consuming repos configure their own requirements — but it means dogfooding the skill in this repo without a source-control.md that includes Related in pr_body_required_sections will immediately hit CI failures. Worth flagging for whoever wires up the dogfooding config.

---

Security review comment mislabeling (unchanged, SUGGESTION)

The comment at create.md:244–245 still reads "Quoted heredoc segments — inert; nothing inside expands" for a block that contains $(content_for_section "$section") — a genuine command substitution. The code is safe (the function uses printf '%s' with user content in the argument position, and += never re-evaluates), but the comment incorrectly attributes the safety to "nothing inside expands" rather than "function output stored via += is never re-evaluated." First raised in the security review; still open. SUGGESTION-level, non-blocking. Fix this →

---

Low-priority: awk $0==h and comment alignment (unchanged)

Both items from prior reviews are still open and non-blocking:

• awk $0==h at create.md:350 — trailing whitespace on a heading silently reads as "missing"; harmless given the deterministic template.
• Comment alignment at create.md:152–153 — continuation # sits one column right of the opening #. Cosmetic.

---

Blocking verdict

One item before merge:

1. /setup gap still unacknowledged — raised in every review round; the source has no TODO or note. Adding a single TODO(#975-followup) comment in SKILL.md at the check table is the minimum acceptable acknowledgment. Fully wiring up the check table, interview, and config template would close it completely.

The attribution fix (cd501a7) is correct. The fenced-code heading issue and security comment mislabeling are non-blocking. The repo CI concern is a dogfooding configuration question, not a plugin defect.

[claude]

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

---

Security Review — PR #1029 (HEAD cd501a7)

• Load tools
• Fetch PR diff
• Read changed files
• Analyze for injection surfaces, auth gaps, dangerous patterns
• Post findings

---

Findings

No security vulnerabilities in the new commit. One carried-over SUGGESTION from the prior review remains unaddressed.

The cd501a7 commit changes exactly three files: create.md, evals.json, and CHANGELOG.md. The evals and changelog are prose/data only. The security surface is solely the restructured body-assembly logic in create.md.

---

Fix confirmed safe: attribution deferred to §2.4.3

The fix moves ATTRIBUTION out of $BODY during assembly and appends it only after both §2.4.2 gates have passed:

# §2.4.3 — after both gate checks
[[ -n "$ATTRIBUTION" ]] && BODY+=$'\n\n'"$ATTRIBUTION"

Permalink

ATTRIBUTION was already single-quoted at its assignment site (line 264) to prevent command substitution at assign time. The BODY+= concat uses parameter expansion — not re-evaluated. Same inertness guarantee as the CLOSES_LINE/TEMPLATE concat already in place. No new injection surface.

The structural fix (defer footer; gate sees only template content) is sound and correctly closes the gate-bypass the prior review flagged.

---

All other new paths confirmed safe

• awk -v h="## ${section}" (create.md:349–353): unchanged from prior review. -v is a string binding; $0==h is string equality, not awk code evaluation. Config-supplied section names cannot inject awk programs.
• grep -qx "Related" with ${REQUIRED_SECTIONS[@]} (create.md:250): "Related" is the hardcoded pattern; config values are piped as input data, never as pattern. Safe.
• content_for_section printf (create.md:239): printf '%s' "$REFS_LINES" — user content is the argument, not the format string. Safe.
• gh pr create --body "$BODY" (create.md:391): $BODY (now including attribution) is a double-quoted argument; GitHub sanitises rendered Markdown. No injection.
• No GitHub Actions workflows were modified.

---

SUGGESTION (carried over, not fixed in cd501a7) — Comment mislabels $(content_for_section) as inert

create.md:244–245

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).
TEMPLATE=""
for section in "${REQUIRED_SECTIONS[@]}"; do
  TEMPLATE+="## ${section}"$'\n\n'"$(content_for_section "$section")"$'\n\n'
done

The comment claims "nothing inside expands," but $(content_for_section "$section") is a bash command substitution — it executes. The code is safe, but the reason is "the function uses printf '%s' "$REFS_LINES" and stores its output via += which does not re-evaluate" — not "nothing expands." A future auditor who reads "nothing inside expands" as a guarantee and applies it to a new $(…) call that IS dangerous would reach the wrong conclusion about safety. This SUGGESTION was raised in the prior security review (0a75d72 review); cd501a7 does not address it.

Confidence: high (the statement is factually incorrect; the downstream risk is auditor confusion, not direct exploitability).

---

Summary: The cd501a7 attribution-deferral fix is structurally sound and introduces no new security surface. The one outstanding item is a documentation-level inaccuracy in the security rationale comment that was already flagged — it does not affect runtime safety of the current code but could mislead future reviewers.

[kyle-sexton]

Addressed all four findings from this review pass:

1. Preserve the work-items Related handoff contract — fixed by updating the contract, not restoring the always-present Related. plugins/work-items/skills/work/SKILL.md §"dispatch brief" no longer lists ## Related as a standing PR obligation, and the post-green deferred-finding step now explicitly ensures the section exists (gh pr edit --body) before citing a follow-up issue in it, since it can no longer assume pull-request's scaffold already created one. Eval 3 updated to match; work-items bumped 0.21.2 → 0.21.3 (commit 482ef4d).

2. Route collected refs when Related is required — real bug, fixed. content_for_section() is now the single place Related's content is decided (REFS_LINES when non-empty, else N/A), reused by both the required-list loop and the ad hoc append, so the two paths can never disagree (commit 482ef4d).

3. Expose required sections through /source-control:setup — confirmed as a real gap. Filed setup skill: offer pr_body_required_sections in interview/write template #1032 to track it as a focused follow-up PR (setup's interview/write template needs the same per-key treatment as pr_title_pattern/trailer_policy/pr_body_attribution); starting that work now.

4. Stop attribution satisfying an empty final section — real bug, fixed. The required-section gate now runs against $BODY BEFORE the attribution footer is appended; the footer is spliced in only in §2.4.3, right before gh pr create, so an empty last required section can never be masked by non-##-prefixed footer text. New eval 14 covers the regression (commit cd501a7).

Good catches on 2 and 4 — real correctness bugs, not just doc drift.

[claude]

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

---

PR Review — feat/975-required-body-sections (HEAD 6eefd41)

• Check what changed since last review (commit 6eefd41)
• Read create.md — verify fenced-code heading fix
• Check /setup skill gap status
• Assess remaining open items
• Post findings

---

The latest commit (6eefd41, "fence-aware heading scan in the required-section gate") closes the fenced-code heading finding from the prior review. One persistent gap remains; everything else is non-blocking.

---

Fixed: fenced-code headings satisfying the gate ✅

The awk at create.md:356–361 now tracks fence state:

/^(```|~~~)/ { in_fence = !in_fence; if (found) print; next }
!in_fence && $0==h {found=1; next}
found && !in_fence && /^## / {exit}
found {print}

A ## <heading> line inside a fenced code block no longer matches as a real section boundary. A fence delimiter that appears while already inside a found section is correctly preserved as that section's content rather than triggering an exit. Eval 15 (pr-body-required-sections-gate-ignores-fenced-heading-lookalikes) covers the regression with three explicit assertions. The inline comment in the prose accurately describes all three behaviors. Finding closed.

---

Still open (5th raise): /setup skill does not expose pr_body_required_sections

This finding has been raised in every review round since the first review, with no fix and no TODO acknowledgment. The commit 6eefd41 touched only CHANGELOG.md, evals.json, and create.md — setup/SKILL.md is unchanged.

Current state of plugins/source-control/skills/setup/SKILL.md:

• Check table (lines 50–56): lists subject_pattern, pr_title_pattern, trailer_policy, pr_body_attribution — no row for pr_body_required_sections.
• Apply interview (lines 210–224): ends at pr_body_attribution; no prompt for the new key.
• Generated config template (lines 227–257): has no slot for pr_body_required_sections.

A consumer running /setup check against a repo that has pr_body_required_sections set at the team layer will receive a check report that silently omits the effective section list. A consumer running /setup apply has no guided path to write the key.

The author's previous reply records "Filed #1032 to track it as a focused follow-up PR." That issue reference has not materialized as a TODO in the source — the gap remains invisible to anyone reading SKILL.md in isolation.

Minimum ask: add one `` comment at line 56 so the gap is visible in the source and the issue link is findable without reading PR comments. Fix this →

---

Still open: security comment mislabeling at create.md:244–245 (SUGGESTION)

The comment still reads:

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).

The block it describes calls $(content_for_section "$section") — a real command substitution. The label "nothing inside expands" is factually wrong. The code is safe for a different reason: content_for_section uses printf '%s' with user content in the argument position, and += never re-evaluates the stored string. A future auditor who reads "nothing inside expands" and applies it to a new $(…) call that IS dangerous would reach the wrong conclusion. Flagged in the security review since 0a75d72; still open. Non-blocking. Fix this →

---

Low-priority: awk $0==h and comment alignment (unchanged)

Both items from prior reviews remain open and non-blocking:

• awk $0==h at create.md:358 — heading with trailing whitespace silently reads as "missing"; harmless given the deterministic template.
• Comment alignment at create.md:152–153 — continuation # sits one column right of the opening #. Cosmetic.

---

Things that are correct

• Fence-aware awk — the three-condition state machine (/^(```)/ → toggle in_fence; heading match only when !in_fence; exit boundary only when !in_fence) correctly handles all documented edge cases. The if (found) print on the fence-toggle line correctly captures a fence delimiter that appears inside a found section as real content.
• Eval 15 coverage — three explicit assertions in expected_behavior map cleanly to the three behaviors the fix claims: (1) a fenced heading is not a real section boundary, (2) the gate still blocks when the only match is fenced, (3) a fence inside a real section is captured in that section's body.
• CHANGELOG entry — 6eefd41's changelog update under 0.18.0 [2026-07-22] correctly classifies the fence fix as "Fixed" (a behavior correction, not a new feature) and gives a complete description of the three behaviors.

---

Blocking verdict

One item before merge:

1. /setup gap unacknowledged in source — five review rounds, no fix and no TODO. Adding a `` note at the check table is the minimum acceptable acknowledgment; the issue number exists, it just needs to appear in SKILL.md.

The fenced-code fix (6eefd41) is correct and complete. All other items are non-blocking.

[claude]

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

---

Security Review — PR #1029 (HEAD 6eefd41)

• Load tools
• Fetch PR diff and read changed files
• Analyze for injection surfaces, auth gaps, dangerous patterns
• Post findings

---

Findings

No security vulnerabilities in the new commit. One carried-over SUGGESTION remains unaddressed.

The 6eefd41 commit touches three files: create.md (the awk fence-aware change), evals.json (new eval 15), and CHANGELOG.md (prose). The evals and changelog are data/prose only. The security surface is the modified awk state machine in create.md.

---

Fence-aware awk confirmed safe

The new code at create.md:356–361:

/^(```|~~~)/ { in_fence = !in_fence; if (found) print; next }
!in_fence && $0==h {found=1; next}
found && !in_fence && /^## / {exit}
found {print}

Four analysis angles:

1. /^(```|~~~)/ regex: Backticks have no special meaning in awk regex patterns. The pipe | is regex alternation. in_fence = !in_fence is a plain boolean toggle — in_fence starts as 0 (awk's uninitialized default). The fence delimiter line flows from stdin through print to SECTION_BODY via command substitution. No execution path. Safe.

2. -v h="## ${section}": Unchanged from prior commits. Awk -v performs a string binding; $0==h is string equality, not a regex match and not code evaluation. A section name containing shell metacharacters (e.g. $(evil)) is stored as the literal string value of h and compared with ==. No awk code injection surface exists. Safe.

3. Can $BODY content manipulate in_fence state maliciously? $BODY arrives as stdin via printf '%s\n' "$BODY". The only effect any line in $BODY can have is toggling in_fence (if it starts with ``` or ~~~) or printing to stdout (which feeds SECTION_BODY). Both are the intended behaviors. A PR body that opens a fence and never closes it would leave in_fence=1 for the rest of the body — heading matches inside would be ignored — but this is a correctness quirk under unusual bodies, not an exploitable security property. No privilege escalation, no data exfiltration, no shell execution path. Safe.

4. awk -v backslash interpretation (edge case, carried from prior reviews): awk processes \n, \t, \\, etc. inside -v values. A section name with a literal backslash (adversarial operator config) causes h to contain an escape-expanded string, producing a silent false negative at gate time (section "missing" even when present). This is a gate-level denial-of-service under malicious operator config only — no privilege escalation or exfiltration path. Not a security finding.

---

SUGGESTION (carried over, still unaddressed) — Comment mislabels $(content_for_section) as "inert"

create.md:244–245

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).
...
  TEMPLATE+="## ${section}"$'\n\n'"$(content_for_section "$section")"$'\n\n'

$(content_for_section "$section") is a bash command substitution — it executes. The "nothing inside expands" rationale is factually wrong for this line. The code is safe, but the reason is:

• content_for_section uses printf '%s' "$REFS_LINES" — user content is the argument, not the format string.
• The output is stored in $TEMPLATE via +=, which does not re-evaluate the string.

The comment was first raised in the security review of 0a75d72 and has not been corrected in the two commits since. A future auditor reading "nothing inside expands" who then writes a new $(…) call that IS dangerous (e.g. one passing user-supplied input as the format string to printf, or calling eval) would reach the wrong conclusion about safety from this comment. The current content_for_section function is carefully constrained; the comment implies the safety comes from the call site, not from the function's internals.

Confidence: high (the statement is factually incorrect; the downstream risk is auditor confusion, not direct exploitability).

---

All other security surfaces confirmed safe (unchanged from prior reviews)

• grep -qx "Related" with ${REQUIRED_SECTIONS[@]} (create.md:250): "Related" is the hardcoded pattern; config values are piped as input, never as the pattern argument. Safe.
• printf '%s' "$REFS_LINES" inside content_for_section (create.md:239): user content in argument position, not format string. Safe.
• ATTRIBUTION single-quoted assignment and $BODY += concat (create.md:264, 387): parameter expansion never re-evaluates. Safe.
• ${REQUIRED_SECTIONS_SOURCE} in stderr (create.md:369): informational only; no downstream execution surface.
• gh pr create --body "$BODY" (create.md:399): $BODY is a double-quoted argument to gh; GitHub sanitises rendered Markdown. Safe.
• No GitHub Actions workflows were modified.

---

Summary: The fence-aware awk change is a sound correctness fix with no new security surface. The one outstanding item is the documentation-level inaccuracy in the security rationale comment ($(content_for_section) described as "inert; nothing inside expands") — it does not affect runtime safety of the current code but could mislead future reviewers adding new $(…) calls to that block. No CRITICAL or IMPORTANT findings.

[kyle-sexton]

Ignore fenced-code headings when validating sections — real bug, fixed (commit 6eefd41). The §2.4.2.2 gate's awk heading scan is now fence-aware: a ## <heading>-shaped line inside a fenced code block no longer counts as a real section boundary in either direction (can't falsely satisfy a required section, and doesn't truncate a real section's own content at an in-fence delimiter). New eval 15 covers it.

Configure this repository's required Related section — good catch, and I want to flag this rather than just pick a side. #975's locked interview decision (see the issue comments) explicitly scoped this PR to mechanism + docs only and said not to write .claude/source-control.md for this repo in this PR, specifically to keep the plugin agnostic and avoid baking melodic's own convention in as a side effect of shipping the mechanism. That was the right call for this PR's scope. But you're right that it has a real, immediate consequence: this repo's own pr-issue-linkage workflow requires non-empty ## Related, and with no config layer set, every ordinary future /pull-request create in this repo now omits it unless a genuine Refs line exists — a live CI regression for this repo's own normal PR flow, not just a theoretical gap.

That's a decision for @kyle-sexton, not something I should resolve unilaterally by either reverting the portable default or unilaterally writing this repo's config in this PR. Flagging it to them now; leaving this thread open until it's decided.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6eefd4186d

ℹ️ 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".

[kyle-sexton]

Ignore HTML-comment headings during section validation — real bug, fixed (commit a7cf2a0), mirroring the fenced-code fix. Generalized the awk state machine to track HTML-comment state (<!--/-->, single- or multi-line) alongside fence state, and rewrote it as one action block instead of several pattern-action rules so exactly one branch fires per line — the old multi-rule form risked double-toggling state on a line matching more than one pattern. New eval 16 covers it.

[claude]

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

---

PR Review — feat/975-required-body-sections (HEAD a7cf2a0)

• Load tools
• Check what changed since last review (commit a7cf2a0)
• Read create.md — verify HTML-comment heading fix
• Check /setup skill gap status
• Assess remaining open items
• Post findings

---

The latest commit (a7cf2a0, "ignore HTML-comment headings in the required-section gate") closes the Codex P2 finding from the prior review. The /setup gap remains the single blocking item.

---

Fixed: HTML-comment headings satisfying the gate ✅

The awk state machine at create.md:362–380 has been generalized from fence-only to fence-and-comment-aware:

{
  if ($0 ~ /^(```|~~~)/) { in_fence = !in_fence; if (found) print; next }
  if (in_fence) { if (found) print; next }
  if ($0 ~ //) in_comment = 0
    next
  }
  if (in_comment) {
    if (found) print
    if ($0 ~ /-->/) in_comment = 0
    next
  }
  if (!found && $0 == h) { found = 1; next }
  if (found && $0 ~ /^## /) { exit }
  if (found) print
}

Analysis of the state machine:

• Priority ordering is correct. Fence check fires first; inside a fence the `` on one line): in_comment = 1 fires, then the same-line `-->` check resets it to 0 before `next`. The heading match block is never reached. ✓
• Multi-line comments: Opening line sets in_comment = 1; subsequent lines (including any ## <heading> shaped lines) go through the if (in_comment) branch with next, bypassing the heading match entirely. ✓
• Comment delimiter inside a found section: A ``. Matches the third expectation in eval 16. ✓
• Rewrite as single action block: The commit message explains the motivation correctly — separate pattern-action rules risk double-firing on a line matching more than one pattern; a single { } block with if/next chains ensures exactly one branch per line. ✓

Eval 16 (pr-body-required-sections-gate-ignores-html-comment-heading-lookalikes) mirrors eval 15's structure with three explicit expectations covering the same three behaviors the code documents. Finding closed.

One non-blocking observation: A section whose only content is HTML-comment text (e.g. ## Test plan\n\n) passes the gate, because the comment delimiter line is printed as section body and is non-empty after tr -d '[:space:]'. This is in the same class as TBD/TODO placeholder-text rejection — explicitly deferred to standards#173 per create.md:343 — not a new issue introduced by this commit.

---

Still open (6th raise): /setup skill does not expose pr_body_required_sections

This gap has been raised in every review round without resolution or a recorded acknowledgment in the source. Commit a7cf2a0 touches only CHANGELOG.md, evals.json, and create.md — setup/SKILL.md is unchanged.

Current state of plugins/source-control/skills/setup/SKILL.md:

• Check table (lines 50–56): lists subject_pattern, pr_title_pattern, trailer_policy, pr_body_attribution — no row for pr_body_required_sections.
• Apply interview and generated config template: unchanged — no guided path to write the new key, no slot in the output.

The prior reply (5049379628) records "Filed #1032 to track it." That issue number has not appeared as a TODO comment in SKILL.md — the gap is invisible to anyone reading the skill in isolation.

Minimum ask for this PR: add one HTML comment at SKILL.md line 56:

Fix this →

---

Still open: security comment mislabeling at create.md:244–245 (SUGGESTION)

The comment at create.md:244–245 still reads "Quoted heredoc segments — inert; nothing inside expands" for a block that calls $(content_for_section "$section") — a genuine command substitution. The safety is real (the function uses printf '%s' with user content in the argument position; += never re-evaluates) but the stated rationale is factually wrong. A future auditor adding a new $(…) call and relying on "nothing inside expands" as their safety guarantee would reach the wrong conclusion. Unaddressed since the 0a75d72 security review. SUGGESTION-level, non-blocking. Fix this →

---

Low-priority: awk $0==h and comment alignment (unchanged)

Both items from prior reviews remain open and non-blocking:

• awk $0==h at create.md:376 — heading with trailing whitespace silently reads as "missing". Harmless given the deterministic template; the same class as the HTML-comment-content placeholder issue, deferred.
• Comment alignment at create.md:152–153 — continuation # sits one column right of the opening #. Cosmetic.

---

Things that are correct

• State machine priority: fence check fires before comment check; the two states are mutually exclusive — no double-toggle scenario exists.
• Comment-inside-section handling: a `` so normal heading-exit detection resumes after the comment closes.
• Single-line comment case: `` sets in_comment=1, immediately checks for `-->` on the same line, resets, and `next` skips heading match — correctly handled.
• CHANGELOG: 0.18.0 Added block now names all three gate robustness improvements (attribution-deferral, fence-aware, HTML-comment-aware) together. Correct categorization under "Added" (all three are enhancements to the new §2.4.2.2 gate, not separate entries).
• Eval 16: Three expectations correctly map to the three behaviors: (1) comment-interior heading is not a boundary, (2) gate blocks when only match is inside a comment, (3) comment delimiter inside a real section is preserved as content.

---

Blocking verdict

One item before merge:

1. /setup gap unacknowledged in source — six review rounds, no fix and no TODO in SKILL.md. A single `` at line 56 is the minimum acceptable acknowledgment; fully wiring up the check table, interview, and config template would close it completely.

The HTML-comment fix (a7cf2a0) is correct and complete. All other items are non-blocking.

[claude]

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

---

Security Review — PR #1029 (HEAD a7cf2a0)

• Load tools
• Fetch PR diff
• Read changed files
• Analyze for injection surfaces, auth gaps, dangerous patterns
• Post findings

---

Findings

No security vulnerabilities in the new commit. One carried-over SUGGESTION remains unaddressed.

The a7cf2a0 commit touches three files: create.md (the HTML-comment state machine), evals.json (new eval 16), and CHANGELOG.md (prose). The evals and changelog are data/prose only. The security surface is the modified awk state machine in create.md.

---

HTML-comment-aware awk confirmed safe

The new code at create.md:361–380:

{
  if ($0 ~ /^(```|~~~)/) { in_fence = !in_fence; if (found) print; next }
  if (in_fence) { if (found) print; next }
  if ($0 ~ //) in_comment = 0
    next
  }
  if (in_comment) {
    if (found) print
    if ($0 ~ /-->/) in_comment = 0
    next
  }
  if (!found && $0 == h) { found = 1; next }
  if (found && $0 ~ /^## /) { exit }
  if (found) print
}

Four analysis angles:

1. $0 ~ // patterns: These are regex matches on stdin data ($BODY). Awk regex patterns `` are entirely literal (no special regex metacharacters in these strings). PR body content flows through stdin only — no path for body content to reach the awk program text itself. No code injection surface.

2. -v h="## ${section}": Unchanged from prior commits. Awk -v is a string binding; $0 == h is string equality. A section name containing `` is stored literally in h and compared with `==`. No awk code injection.

3. Fence-before-comment precedence: The fence check (/^(```|~~~)/) fires first via the leading if, so a <!-- line inside a fence block never sets in_comment. This closes the interaction between the two state machines — in_comment cannot be corrupted by a PR body that embeds <!-- inside a code fence.

4. Can $BODY content manipulate in_comment state maliciously? $BODY flows through stdin. The only effects any line can have are toggling in_fence/in_comment, printing to stdout, or exiting. A PR body with an unclosed <!-- leaves in_comment = 1 for the rest of the body — all subsequent heading matches are ignored. This is a gate-level false negative (section reported "missing" even when present after the unclosed comment), not a bypass. No privilege escalation, no data exfiltration, no shell execution path. Safe.

5. Single-action-block design: Rewriting from multiple pattern-action rules to one { if... } block correctly closes the prior double-fire risk — awk previously ran all matching pattern-action rules per line, risking double-toggle of in_fence on a line that matched both /^(```|~~~)/ and found patterns. The new form fires exactly one branch per line via next. This is a correctness improvement, not a new security surface.

---

Known edge case (correctness, not security)

A PR body line of the form ## Related <!-- inline note (a real heading with a trailing unclosed HTML comment start) enters the <!-- branch rather than the heading-match branch. The gate sets in_comment = 1 and skips the heading. This is a conservative false negative: the gate rejects a body that actually contains the section. GitHub renders ## Related <!-- inline note as a visible heading with the raw HTML in the text, not as a comment, so the gate's behavior is technically incorrect. However, this is conservative (gate rejects) rather than permissive (gate accepts a missing section) — no security consequence. Not an exploitable bypass.

---

SUGGESTION (carried over, 4th raise, unaddressed in a7cf2a0) — Comment mislabels $(content_for_section) as "inert"

create.md:244–245

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).
...
  TEMPLATE+="## ${section}"$'\n\n'"$(content_for_section "$section")"$'\n\n'

$(content_for_section "$section") is a bash command substitution — it executes. The "nothing inside expands" rationale is factually wrong for this line. The code is safe for a different reason: content_for_section uses printf '%s' "$REFS_LINES" with user content in the argument position, and += never re-evaluates the stored string. A future auditor who reads "nothing inside expands" and applies it to a new $(…) call that IS dangerous (e.g. one passing user-supplied input as the format string, or calling eval) would reach the wrong conclusion about safety from this comment. This has been flagged in every security review since 0a75d72; a7cf2a0 does not address it.

Confidence: high. The statement is factually incorrect; the downstream risk is auditor confusion, not direct exploitability.

---

All other security surfaces confirmed safe (unchanged from prior reviews)

• grep -qx "Related" with ${REQUIRED_SECTIONS[@]} (create.md:250): "Related" is the hardcoded pattern; config values are piped as input data, never as the pattern argument. Safe.
• printf '%s' "$REFS_LINES" inside content_for_section (create.md:239): user content in argument position, not format string. Safe.
• ATTRIBUTION single-quoted assignment and $BODY += concat: parameter expansion never re-evaluates. Safe.
• Attribution deferred to §2.4.3 (create.md:399–406): gate-bypass via attribution footer was fixed in cd501a7; unchanged here.
• gh pr create --body "$BODY": $BODY is a double-quoted argument to gh; GitHub sanitises rendered Markdown. Safe.
• No GitHub Actions workflows were modified.

---

Summary: The HTML-comment-aware awk change is a sound correctness fix with no new security surface. The fence-before-comment ordering, the single-action-block design, and the string-binding -v h= are all secure. The one outstanding item is the documentation-level inaccuracy at line 244 — $(content_for_section) described as "inert; nothing inside expands" — flagged in every security review since 0a75d72 and still unaddressed. No CRITICAL or IMPORTANT findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a7cf2a0673

ℹ️ 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 6m 0s —— View job

---

PR Review — feat/975-required-body-sections (HEAD ecc9bf6)

• Load tools
• Check what changed since last review (commit a7cf2a0)
• Read new/changed files at current HEAD
• Verify prior findings are addressed
• Post findings

---

The latest commit (ecc9bf6, "chore(repo): dogfood pr_body_required_sections to keep this repo's own CI green") closes the Codex P1 CI regression finding. One unresolved blocking item and one new correctness issue remain.

---

Fixed: repo CI regression (Codex P1) ✅

.claude/source-control.md now sets pr_body_required_sections: [Summary, Test plan, Related], matching this repo's pr-issue-linkage CI gate. Three things are correct in this addition:

• Format — the bullet-list format (- Summary, - Test plan, - Related) matches the documented format in config-resolution.md ("a flat Markdown bullet list") and the eval fixture used in eval 13.
• Layer placement — the file is at .claude/source-control.md (team layer, tracked), not gitignored.
• Per-key fallthrough — the comment explains that only this key is set here and all others fall through to the plugin's inference; this correctly documents why the file doesn't restate subject_pattern, type_list, etc.

The .gitignore addition (.claude/**/*.local.*) is correctly the wider glob that docs/conventions/consumer-config-layering/README.md §"Overlay naming" (line 105) documents as the canonical form — it covers nested overlay paths that the narrower .claude/*.local.* pattern silently misses (per the README's lines 109–112). The comment in .gitignore accurately cross-references that section.

The CHANGELOG note correctly classifies this under "Changed" (not under a separate "Fixed" bullet) and accurately calls it the first fleet-adoption instance. Finding closed.

---

Consistency note: "Re-run /setup" is misleading given the /setup gap

.claude/source-control.md:7 uses the standard template boilerplate: "Re-run /source-control:setup to change these values."

Since /setup still does not know about pr_body_required_sections (the persistent blocking gap below), a consumer reading this file would be misled: running /setup cannot change the pr_body_required_sections value. This is a direct consequence of the /setup gap rather than a separate defect, but it makes the /setup gap more acute — the first fleet-adoption instance itself tells consumers to use a path that doesn't work for the one key it contains.

---

Still open (7th raise): /setup skill does not expose pr_body_required_sections

This gap has been raised in every review round without resolution or acknowledgment in the source. The commit ecc9bf6 touched only .claude/source-control.md, .gitignore, and plugins/source-control/CHANGELOG.md — setup/SKILL.md is unchanged.

Current state of plugins/source-control/skills/setup/SKILL.md:

• Check table (lines 50–56): lists subject_pattern, pr_title_pattern, trailer_policy, pr_body_attribution — no row for pr_body_required_sections.
• Apply interview (lines 210–224): ends at pr_body_attribution; no prompt for the new key.
• Generated config template (lines 227–257): no slot for pr_body_required_sections.
• No TODO(#1032) note anywhere in the file — the gap is invisible to anyone reading the skill in isolation.

The prior reply (5049379628) recorded "Filed #1032 to track it." That issue number still has not appeared as a TODO comment in SKILL.md, and the first fleet-adoption instance in .claude/source-control.md now actively misdirects consumers to /setup to change values that /setup cannot change.

Minimum ask: add one HTML comment at SKILL.md line 56 so the gap is visible in source:

Fix this →

---

New: gh pr edit --body replaces body, it does not append

work/SKILL.md:189 says:

"append a ## Related section via gh pr edit --body when the current body has none"

gh pr edit --body <text> replaces the entire PR body with <text> — it does not append. An agent following this instruction literally would call:

gh pr edit <PR> --body "## Related

Refs #Y — <why>"

and the result would be a PR body consisting solely of ## Related\n\nRefs #Y, with Closes #N, ## Summary, ## Test plan, and the attribution footer all destroyed. The very CI gate this PR protects (pr-issue-linkage) would then report a missing ## Related on the next run, because the body contains nothing else.

The adapter README the commit history already references documents the correct pattern at lines 168–176:

gh pr view <PR> --json body --jq '.body' | tr -d '\r' > /tmp/pr-body.md
printf '%s\n\n%s\n' "$(cat /tmp/pr-body.md)" "## Related"$'\n\n'"${REFS}" \
  | gh pr edit <PR> --body-file -

The fix is to replace "append a ## Related section via gh pr edit --body" with an explicit read-modify-write instruction, referencing the adapter README's pattern. This was flagged by Codex in the previous review cycle and not addressed in ecc9bf6.

Fix this →

---

Still open: security comment mislabeling at create.md:244–245 (SUGGESTION)

The comment at create.md:244 still reads "Quoted heredoc segments — inert; nothing inside expands" for a block that calls $(content_for_section "$section") — a genuine command substitution. The code is safe (the function uses printf '%s' with user content in the argument position; += never re-evaluates), but the stated rationale is factually wrong. Flagged since the 0a75d72 security review. SUGGESTION-level, non-blocking.

---

Low-priority items (unchanged)

• awk $0==h at create.md:376 — heading with trailing whitespace silently reads as "missing". Harmless given the deterministic template; non-blocking.
• Comment alignment at create.md:152–153 — continuation # sits one column right of the opening #. Cosmetic.

---

Things that are correct in ecc9bf6

• Format fidelity — .claude/source-control.md uses the same bullet-list format as the plugin's documented config shape and the eval 13 fixture.
• Layer semantics — team-layer placement (tracked, not gitignored) is correct; the overlay gitignore pattern (.claude/**/*.local.*) is the wider, correctly-specified form.
• Self-regression atomicity — the CI-breaking default change and its remedy ship in the same PR. The CHANGELOG records this rationale explicitly.
• First-fleet-adoption clarity — the CHANGELOG note correctly flags this as the prototype instance, so later adopters see a real example with inline rationale.

---

Blocking verdict

Two items before merge:

1. /setup gap unacknowledged in source — seven review rounds; minimum ask is one `` at the check table so the gap is visible to anyone reading the skill.
2. gh pr edit --body body-replace bug — the deferred-finding path in work/SKILL.md instructs "append via gh pr edit --body" but that flag replaces the entire body. An agent following it destroys Closes #N, ## Summary, ## Test plan, and attribution. The correct read-modify-write pattern is already documented in the adapter README.

[claude]

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

---

Security Review — PR #1029 (HEAD ecc9bf6)

• Load tools
• Fetch PR diff and read changed files
• Analyze for injection surfaces, auth gaps, dangerous patterns
• Post findings

---

Findings

No security vulnerabilities in the new commit. One carried-over SUGGESTION remains unaddressed.

The ecc9bf6 commit ("dogfood pr_body_required_sections") adds three files: .claude/source-control.md (new config file), .gitignore (pattern addition), and plugins/source-control/CHANGELOG.md (prose). The evals and changelog are data/prose only. The only content that touches the plugin's execution surface is the new .claude/source-control.md.

---

New path: .claude/source-control.md as the team-layer config source

The new file sets pr_body_required_sections to [Summary, Test plan, Related]. These three section-name strings flow into ${REQUIRED_SECTIONS[@]} at the model level when the skill reads this file, then into the shell pseudocode in create.md at:

1. awk -v h="## ${section}" (create.md:361) — -v is a string binding; $0 == h is string equality, not regex or code evaluation. Section names from the config file cannot inject awk programs.

2. content_for_section "$section" (create.md:248) — the function matches $1 against Related or *; the * branch uses printf '<real content for %s>' "$1" where the section name is the format argument, not the format string. Safe.

3. "## ${section}" in TEMPLATE+= — parameter expansion, never re-evaluated as code by +=. Safe.

4. printf '%s\n' "${REQUIRED_SECTIONS[@]}" | grep -qx "Related" (create.md:250) — section names are piped as stdin (data), never as the grep pattern. Safe.

The three literal values shipped (Summary, Test plan, Related) contain no shell metacharacters, but even a hypothetical adversarial section name (e.g., $(evil)) cannot reach code evaluation through any of the above paths. The file lives in the team-tracked config layer — the same operator trust tier as all other keys on the source-control.md surface. No new trust boundary is opened.

---

.gitignore pattern .claude/**/*.local.*

The recursive glob excludes personal-overlay files from being accidentally committed. This is a positive security action (prevents inadvertent exposure of personal config data). No security consequence.

---

SUGGESTION (carried over, 5th raise, still unaddressed) — Comment mislabels $(content_for_section) as "inert"

create.md:244–245

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).
TEMPLATE=""
for section in "${REQUIRED_SECTIONS[@]}"; do
  TEMPLATE+="## ${section}"$'\n\n'"$(content_for_section "$section")"$'\n\n'
done

$(content_for_section "$section") is a bash command substitution — it executes. The "nothing inside expands" rationale is factually wrong for this line. The code is safe, but the reason is that content_for_section uses printf '%s' "$REFS_LINES" with user content in the argument position (not the format string), and += never re-evaluates the stored string. A future auditor who reads "nothing inside expands" and applies it as a safety guarantee to a new $(…) call that IS dangerous (e.g., one passing user-supplied input as the format string, or calling eval) would reach the wrong conclusion. This has been flagged in every security review since 0a75d72; ecc9bf6 does not address it.

Confidence: high. The statement is factually incorrect; the downstream risk is auditor confusion, not direct exploitability. Fix this →

---

All other security surfaces confirmed safe (unchanged from prior reviews)

• awk -v + fence/comment state machine (create.md:362–380): no changes from a7cf2a0; analysis is unchanged. -v h= is a string binding; $0==h is string equality; fence/comment toggles are pure boolean state; no stdin-to-awk-program injection surface exists.
• Attribution deferred to §2.4.3 (create.md:406): unchanged; gate-bypass via attribution footer was fixed in cd501a7.
• gh pr create --body "$BODY" (create.md:418): double-quoted argument to gh; GitHub sanitises rendered Markdown.
• No GitHub Actions workflows were modified.

---

Summary: The ecc9bf6 dogfooding commit introduces no new security surface. The new .claude/source-control.md config file contains three benign section-name literals that flow safely through all downstream shell paths via string binding and parameter expansion. The one outstanding SUGGESTION — the security rationale comment mislabeling $(content_for_section) as "inert; nothing inside expands" — has been flagged in every security review since 0a75d72 and remains the only open item. No CRITICAL or IMPORTANT findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ecc9bf6c63

ℹ️ 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".

[kyle-sexton]

Configure this repository's required Related section — resolved. Decision from @kyle-sexton: write this repo's own .claude/source-control.md in this PR rather than deferring it.

Rationale: instruction #8's original premise (keep this PR mechanism + docs only, no repo dogfooding) was overtaken by this exact finding — the review exposed that this PR changes this repo's own effective PR-body behavior, and a change that would break the repo's own CI must carry its own remedy in the same PR (self-regression atomicity), not a follow-up. Agnosticity is intact: .claude/source-control.md at the repo root is repo-local configuration DATA at the seam the mechanism itself defines — not plugin content. The plugin still ships nothing melodic-specific; this repo is simply the first consumer to configure the key.

Added (commit ecc9bf6): team-layer .claude/source-control.md setting only pr_body_required_sections (Summary, Test plan, Related, matching pr-issue-linkage's requirement) — every other key still falls through to inference, per-key fallthrough. Also added the recursive .claude/**/*.local.* gitignore line this file's own convention needs (the repo didn't have it). Noted as the first fleet-adoption instance in both the PR body and the CHANGELOG — every other consuming repo adopts the key the ordinary way, via /source-control:setup apply (that skill's support for this key is #1032, tracked separately).

Sanity-checked per your ask: eval 12's "no source-control.md config file exists in this repo" scenario is unaffected — skill-quality:check's eval validation is schema-only (check-jsonschema against the bundled schema in CI), it never executes eval prompts against live repo state, so the eval's scenario framing and this repo's real file never interact.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c00aa434a0

ℹ️ 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 4m 2s —— View job

---

PR Review — feat/975-required-body-sections (HEAD 00fd546)

• Check what changed since last review (ecc9bf6 → 00fd546)
• Read changed files at current HEAD
• Verify prior blocking findings are addressed
• Assess Codex P2 (GFM-correct fences) and gh pr edit --body fix
• Post findings

---

Two new commits since the last review:

• c00aa43 — fix(source-control,work-items): GFM-correct fences + read-modify-write body edit — addresses the two prior blocking items
• 00fd546 — merge from origin/main (brings in setup/SKILL.md preamble changes from source-control: setup apply writes self-describing header + draft-aid framing (deliver the #913 f6 fold) #1046; completely disjoint from this PR's changed files)

---

Fixed: GFM-correct fence detection ✅

The prior fence-aware awk recognized only unindented ``` or ~~~ openers and treated any fence character as a valid closer regardless of which character opened the fence. The fix at create.md:361–396 correctly handles both GFM cases Codex raised:

stripped = $0
sub(/^ {0,3}/, "", stripped)
fence_char = ""
if (stripped ~ /^```/) fence_char = "`"
else if (stripped ~ /^~~~/) fence_char = "~"

if (!in_fence && fence_char != "") { in_fence = 1; open_char = fence_char; if (found) print; next }
if (in_fence) {
  if (fence_char == open_char) in_fence = 0
  if (found) print
  next
}

• Indented fences: sub(/^ {0,3}/, "", stripped) strips up to 3 leading spaces before checking for fence delimiters. Per GFM spec (§4.5), a fence may have 0–3 leading spaces; 4+ makes it an indented code block (which this correctly leaves unrecognized). ✓
• Mismatched fence characters: open_char stores the opening delimiter type; a closing line must have fence_char == open_char to exit fence state — a ~~~ line inside an open ``` fence does not close it. ✓
• Fence inside a found section: the if (found) print on both branches correctly captures fence openers/closers and their content as section body. ✓

Eval 15 is updated with two new expectations covering the indented and mismatched-char cases. Finding closed.

---

Fixed: gh pr edit --body body-replace bug ✅

work/SKILL.md:189 now explicitly documents the read-modify-write requirement:

"Adding it is a read-modify-write, never a bare --body replacement — gh pr edit --body/--body-file REPLACES the whole body … read the current body first (gh pr view <N> --json body --jq '.body'), append the ## Related section … and write the combined result back via --body-file -; a bare gh pr edit --body "## Related\n..." would silently drop Closes #N, Summary, and Test plan."

The warning names the specific flag, the correct read pattern, the reference (adapter README), and what would be lost without the read-modify-write. Finding closed.

---

Still open (8th raise): /setup skill does not expose pr_body_required_sections

The merge from origin/main brought in setup/SKILL.md changes for #1046 (self-describing preamble), but these are orthogonal — they add explanatory prose to the generated config template header and the post-write report; they do not touch the check table, apply interview, or the key list.

Current state at setup/SKILL.md:

• Check table (lines 51–56): subject_pattern, pr_title_pattern, trailer_policy, pr_body_attribution — no row for pr_body_required_sections.
• Apply interview (lines 210–224): ends at pr_body_attribution; no prompt for the new key.
• Generated config template (lines 240–261): lists subject_pattern, type_list, pr_title_pattern, trailer_policy, pr_body_attribution — no slot for pr_body_required_sections.
• No TODO(#1032) anywhere in the file — grep confirms zero occurrences.

The gap is additionally acute now that the dogfooding .claude/source-control.md (added in ecc9bf6) instructs readers to "Re-run /source-control:setup to change these values" — but /setup cannot change pr_body_required_sections. The first fleet-adoption instance actively misdirects consumers.

Minimum ask: add one HTML comment at SKILL.md line 56:

Fix this →

---

Still open: security comment mislabeling at create.md:244–245 (SUGGESTION)

create.md:244–245 still reads:

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).

The block calls $(content_for_section "$section") — a real command substitution. The "nothing inside expands" rationale is factually wrong; the code is safe because the function uses printf '%s' with user content in the argument position and += never re-evaluates. Flagged in every security review since 0a75d72; eight review rounds in, still unaddressed. SUGGESTION-level, non-blocking. Fix this →

---

Non-blocking: Codex P2 (HTML-comment-only section body passes emptiness check)

A required section whose body consists entirely of an HTML comment (e.g., ## Test plan\n\n) passes the §2.4.2.2 gate because the comment delimiters are non-whitespace characters. The in_comment branch at create.md:388–391 prints the comment text as section body; tr -d '[:space:]' leaves the `` characters, making the section appear non-empty. This is in the same class as placeholder-text rejection and min-content rules, explicitly deferred to melodic-software/standards#173 per create.md:343. Non-blocking; worth acknowledging.

---

Things that are correct in c00aa43

• sub(/^ {0,3}/, "", stripped) GFM alignment: strips exactly 0–3 leading spaces before fence detection; a 4-space-indented line (indented code block) leaves a residual space that prevents a false fence match. Correctly follows GFM §4.5 without over-engineering.
• open_char persistence: the variable is assigned on fence open and compared on potential close — only a matching delimiter type exits fence state. Correctly prevents ~~~ from closing a ``` fence and vice versa.
• Fence inside found section: both the opener and closer lines are printed (if (found) print) so a code block appearing inside a required section's content is correctly captured. No accidental content loss.
• Eval 15 update: two new expectations added ("indented up to 3 spaces" and "~~~ line … does not close that fence") correctly characterize the two GFM behaviors the fix addresses.
• work/SKILL.md read-modify-write prose: names the exact flag (--body/--body-file), the correct alternative (gh pr view --json body --jq '.body' → append → --body-file -), the reference (adapter README), and the silent data-loss scenario. Complete and actionable.
• Low-priority items (awk $0==h, comment alignment): still open and still non-blocking; no change since prior reviews.

---

Blocking verdict

One item before merge:

1. /setup gap unacknowledged in source — eight review rounds, still no fix and no TODO. Adding a single `` at the check table is the minimum acceptable acknowledgment so the gap is visible to the next person reading the skill. The dogfooding instance in .claude/source-control.md now actively directs readers to `/setup` for changes that `/setup` cannot make — which makes this gap more acute than in prior rounds.

Both items from c00aa43 (GFM fences, read-modify-write) are correct and complete. All other items are non-blocking.

[claude]

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

---

Security Review — PR #1029 (HEAD 00fd546)

• Load tools
• Fetch PR diff
• Read changed files
• Analyze for injection surfaces, auth gaps, dangerous patterns
• Post findings

---

Findings

No security vulnerabilities in the new commits. One carried-over SUGGESTION remains unaddressed.

The merge commit (00fd546) brings origin/main into the branch — it adds no new security surface to the PR's changed files. The last substantive code change was c00aa43 (GFM-correct fences + read-modify-write body edit). This review covers the full diff from origin/main to HEAD across all 14 changed files.

---

GFM fence fix confirmed safe (c00aa43)

The commit rewrites fence detection in the §2.4.2.2 awk to strip up to 3 leading spaces and track the opening fence character:

stripped = $0
sub(/^ {0,3}/, "", stripped)
fence_char = ""
if (stripped ~ /^```/) fence_char = "`"
else if (stripped ~ /^~~~/) fence_char = "~"

if (!in_fence && fence_char != "") { in_fence = 1; open_char = fence_char; ... }
if (in_fence) {
  if (fence_char == open_char) in_fence = 0  # closes only on matching character
  ...
}

Permalink

This directly addresses the prior Codex P2 "Match GitHub's fenced-code rules" finding (indented fences, mismatched opener/closer). The sub-strip approach and open_char tracking are correct. The remaining documented limitation (run-length parity for the closing fence) is acknowledged in the inline comment and is conservative — it can produce a false negative (misidentifying a longer-run closing fence as content of the section) but never a false positive bypass.

The -v h="## ${section}" string binding is unchanged. $0 == h remains string equality, not regex or code evaluation. No code injection surface.

Read-modify-write instruction in work/SKILL.md confirmed safe

The fix at work/SKILL.md:189 explicitly describes a read-modify-write sequence:

"read the current body first (gh pr view <N> --json body --jq '.body'), append the ## Related section (or its content, if the section already exists) to that read, and write the combined result back via --body-file -"

This is instruction prose for the AI agent. The PR number is acquired from a prior gh pr view call (not user-supplied at injection time). The body is read as data and extended with a controlled ## Related string before being written back. No command injection surface in this flow.

.claude/source-control.md config values confirmed safe

The three section-name literals (Summary, Test plan, Related) added by the dogfooding commit flow into ${REQUIRED_SECTIONS[@]} and reach:

1. awk -v h="## ${section}" — string binding; $0 == h is equality, not code evaluation. These literal values contain no backslashes, so no awk escape-sequence ambiguity.
2. TEMPLATE+="## ${section}"$'\n\n'"$(content_for_section "$section")"$'\n\n' — parameter expansion; += never re-evaluates. content_for_section uses printf '%s' "$REFS_LINES" (user data in argument position, not format string). Safe.
3. printf '%s\n' "${REQUIRED_SECTIONS[@]}" | grep -qx "Related" — section names are piped as stdin data; "Related" is the hardcoded grep pattern. Safe.

No new trust boundary is opened — the config file lives in the same operator trust tier as all other source-control.md keys.

HTML-comment body content reaching SECTION_BODY (correctness, not security)

The latest Codex P2 finding ("Strip comments before section emptiness checks") is technically correct: when a section's body contains only an HTML comment (e.g., ## Test plan\n\n), the awk's if (found) print fires on the `` as their test plan is circumventing their own gate. The prior reviews and the inline prose (create.md:343) already note this as deferred to standards#173. It is out of scope for a security review.

---

SUGGESTION (carried over, 6th raise, still unaddressed) — Comment mislabels $(content_for_section) as "inert"

create.md:244–245

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).
TEMPLATE=""
for section in "${REQUIRED_SECTIONS[@]}"; do
  TEMPLATE+="## ${section}"$'\n\n'"$(content_for_section "$section")"$'\n\n'
done

$(content_for_section "$section") is a bash command substitution — it executes. The "nothing inside expands" rationale is factually wrong for this line. The code is safe for a different reason: content_for_section uses printf '%s' "$REFS_LINES" with user content in the argument position (not the format string), and += never re-evaluates the stored string. A future auditor who reads "nothing inside expands" and applies it as a safety guarantee to a new $(…) call that IS dangerous (e.g., one passing user-supplied input as the format string, or calling eval) would reach the wrong conclusion. This has been flagged in every security review since 0a75d72 and remains unaddressed.

Confidence: high. The statement is factually incorrect; the downstream risk is auditor confusion, not direct exploitability. Fix this →

---

Summary: The GFM fence fix (c00aa43) is structurally sound and introduces no new security surface. The read-modify-write instruction in work/SKILL.md is correct. The dogfooding .claude/source-control.md config values flow safely through all downstream shell paths. No CRITICAL or IMPORTANT security findings. The one outstanding item is the documentation-level inaccuracy at line 244 — $(content_for_section) described as "inert; nothing inside expands" — flagged in every security review since 0a75d72 and still unaddressed.

[claude]

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

---

PR Review — feat/975-required-body-sections (HEAD 8228d6f)

• Load tools
• Check what changed since last review (00fd546 → 8228d6f)
• Read changed files at current HEAD
• Verify prior findings are addressed
• Post findings

---

The latest commit (8228d6f, "fix(source-control): comment text never counts as section content") closes the final Codex P2 finding from the prior review. One persistent blocking item remains.

---

Fixed: HTML-comment body content counted as non-empty ✅

The awk state machine at create.md:387–394 now suppresses printing in both the comment-open and inside-comment branches:

if ($0 ~ //) in_comment = 0
  next
}
if (in_comment) {
  if ($0 ~ /-->/) in_comment = 0
  next
}

Previously both branches had if (found) print before next, which passed comment text into SECTION_BODY even when inside a required section. The change correctly implements the fence/comment asymmetry the inline prose at lines 358–365 describes: fences are rendered content and stay in the captured body; comments are never rendered and are now silently dropped.

Real content after a comment is still captured correctly. When found=1 and the scan encounters \nReal content, the comment branches consume the comment lines with next (no exit), so subsequent Real content still reaches the if (found) print catch-all. Eval 16's updated third expectation — "Comment text is never treated as an exit boundary" — accurately captures this: the comment doesn't truncate the section; it's just invisible to the captured body. Finding closed.

Eval 17 (pr-body-required-sections-gate-treats-comment-only-body-as-empty) covers the concrete scenario: a ## Test plan section whose entire body is an unfilled `` placeholder now blocks gate creation. The three expectations correctly reflect both what the gate does (blocks) and why (comments are never rendered, and this matches both GitHub's own render and a comment-stripping PR-body validator). Finding closed.

---

Still open (9th raise): /setup skill does not expose pr_body_required_sections

This gap has been raised in every review round. The 8228d6f commit touches only create.md, evals.json, and CHANGELOG.md — setup/SKILL.md is unchanged.

Current state of plugins/source-control/skills/setup/SKILL.md:

• Check table (lines 51–56): subject_pattern, pr_title_pattern, trailer_policy, pr_body_attribution — no row for pr_body_required_sections.
• Apply interview (lines 210–224): ends at pr_body_attribution; no prompt for the new key.
• Generated config template (lines 240–261): no slot for pr_body_required_sections.
• grep confirms zero occurrences of pr_body_required_sections or #1032 anywhere in the file.

The dogfooding file at .claude/source-control.md:6 still says "Re-run /source-control:setup to change these values" — but /setup cannot change pr_body_required_sections. The first fleet-adoption instance of this key actively misdirects consumers to a write path that doesn't work for the one key it contains.

Minimum ask: add one HTML comment at SKILL.md line 56 so the gap is visible in source:

Fix this →

---

Still open: security comment mislabeling at create.md:244–245 (SUGGESTION)

create.md:244 still reads "Quoted heredoc segments — inert; nothing inside expands" for a block containing $(content_for_section "$section") — a real command substitution. The code is safe because the function uses printf '%s' with user content in argument position and += never re-evaluates; the rationale is simply wrong. Flagged in every security review since 0a75d72. SUGGESTION-level, non-blocking. Fix this →

---

Low-priority: awk $0==h and comment alignment (unchanged)

Both items from prior reviews remain open and non-blocking:

• awk $0==h at create.md:396 — heading with trailing whitespace reads as "missing". Harmless given the deterministic template.
• Comment alignment at create.md:152–153 — continuation # sits one column right of the opening #. Cosmetic.

---

Things that are correct in 8228d6f

• Fence/comment asymmetry — the prose at lines 358–365 correctly explains WHY the two are asymmetric: fences contain rendered content (visible code blocks), comments contain hidden text. The awk now implements exactly this distinction: fences print to SECTION_BODY; comments don't.
• Real content after a comment is preserved — comment branches use next, not exit, so scanning continues; any real content following a comment inside a required section is still captured. The updated eval 16 expectation confirms this correctly.
• Eval 17 — the three expectations map cleanly to the three behaviors: (1) heading is real (matched), (2) body content is comment-only (not printed), (3) rationale cites both GitHub's render and comment-stripping validators as the precedent. The expected_output field is precise about the fence distinction.
• CHANGELOG — "all five review-caught during pull-request skill: pre-create gate must verify non-empty '## Related' section #975" correctly counts: (1) attribution-masking, (2) fence-aware headings, (3) HTML-comment headings, (4) GFM indented/mismatched fences, (5) comment-body-as-nonempty.

---

Blocking verdict

One item before merge:

1. /setup gap unacknowledged in source — nine review rounds, no fix and no TODO in SKILL.md. A single `` comment at the check table is the minimum acceptable acknowledgment. The dogfooding file's "Re-run /source-control:setup to change these values" wording additionally makes the gap actively misleading to the first readers of this key's fleet-adoption instance.

The comment-body fix (8228d6f) is correct and complete. The fence/comment asymmetry is now correctly implemented and documented. All other items are non-blocking.

[claude]

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

---

Security Review — PR #1029 (HEAD 8228d6f)

• Load tools and fetch PR diff
• Read changed files
• Analyze for injection surfaces, auth gaps, dangerous patterns
• Post findings

---

Findings

No security vulnerabilities in the new commit. One carried-over SUGGESTION remains unaddressed.

The 8228d6f commit ("fix(source-control): comment text never counts as section content") modifies three files: create.md (awk logic), evals.json (eval 17 + corrected eval 16), and CHANGELOG.md (prose). The evals and changelog are data/prose only. The security surface is the modified awk state machine in create.md.

---

New commit confirmed safe: if (found) print removal inside comment branches

The fix removes two if (found) print lines from the comment-state branches of the §2.4.2.2 awk:

Before (a7cf2a0):

if ($0 ~ //) in_comment = 0
    next
}
if (in_comment) {
    if (found) print    # ← removed
    if ($0 ~ /-->/) in_comment = 0
    next
}

After (8228d6f) at create.md:387–395:

if ($0 ~ //) in_comment = 0
    next
}
if (in_comment) {
    if ($0 ~ /-->/) in_comment = 0
    next
}

Security analysis of these paths:

1. $0 ~ // regex patterns: These are literal string patterns matched against stdin data (printf '%s\n' "$BODY"). $BODY flows through stdin only — no user-supplied or config-supplied content can reach the awk program text itself. No code injection surface.

2. in_comment state toggle: A plain boolean. Setting it to 1 and then 0 on the same line (single-line comment) is correct. The toggle cannot be influenced to execute arbitrary awk code — it's pure state. No injection surface.

3. Interaction with in_fence: The fence check fires first (lines 381–386), so a `` patterns can only toggle in_comment state (producing false negatives or false positives at gate time) — correctness edge cases, not privilege escalation or data exfiltration.

The correctness fix (comment text is no longer printed into SECTION_BODY even when inside a found section) is structurally sound and introduces no new security surface.

---

All other security surfaces unchanged from prior reviews (confirmed safe)

• awk -v h="## ${section}" (create.md:366): -v is a string binding; $0 == h is string equality, not regex or code evaluation. Config-supplied section names cannot inject awk programs.
• content_for_section() function (create.md:237–242): uses printf '%s' "$REFS_LINES" — user content is the argument, not the format string. Safe.
• TEMPLATE+= loop (create.md:247–252): bash += never re-evaluates stored content. Safe.
• BODY+= assembly (create.md:289–291): parameter expansion; no re-evaluation. Safe.
• ATTRIBUTION single-quoted assignment + deferred footer (create.md:264, 426): single-quoted at assignment; appended via += after both gates pass. Safe.
• printf '%s\n' "${REQUIRED_SECTIONS[@]}" | grep -qx "Related" (create.md:250): "Related" is the hardcoded pattern; section names from config are piped as stdin data, never as the pattern argument. Safe.
• gh pr create --body "$BODY" (create.md:438): $BODY is a double-quoted argument to gh; GitHub sanitises rendered Markdown. Safe.
• .claude/source-control.md (dogfooding config, three benign string literals): flows safely through all downstream shell paths via string binding and parameter expansion. Safe.
• No GitHub Actions workflows were modified.

---

SUGGESTION (carried over, 7th raise, still unaddressed) — Comment mislabels $(content_for_section) as "inert"

create.md:244–245

# Quoted heredoc segments — inert; nothing inside expands. Safe even if a heading's
# real content contains $vars or $(cmds).
TEMPLATE=""
for section in "${REQUIRED_SECTIONS[@]}"; do
  TEMPLATE+="## ${section}"$'\n\n'"$(content_for_section "$section")"$'\n\n'
done

$(content_for_section "$section") is a bash command substitution — it executes the function. The comment's claim that "nothing inside expands" is factually wrong for this block. The code is safe, but the reason is: content_for_section uses printf '%s' "$REFS_LINES" with user content in the argument position (not the format string), and += never re-evaluates the stored string. The actual safety guarantee is identical to the ${CLOSES_LINE} concat path, not to "heredoc inertness."

A future auditor who reads "nothing inside expands" as the safety guarantee and applies it to a new $(…) call that IS dangerous (e.g., one passing user-supplied input as the format string, or calling eval) would reach the wrong conclusion. This has been flagged in every security review since the 0a75d72 commit; 8228d6f does not address it.

Confidence: high — the statement is factually incorrect. The downstream risk is auditor confusion, not direct exploitability of the current code. Fix this →

---

Summary: The 8228d6f commit's removal of if (found) print inside the HTML-comment branches is a correct, safe correctness fix. Comment text is now never included in SECTION_BODY regardless of position relative to the target section — consistent with how GitHub renders HTML comments (they are invisible, not content). No new security surface is introduced. The one outstanding SUGGESTION — the mislabeled comment at line 244, flagged since 0a75d72 — remains the only open item from the security perspective. No CRITICAL or IMPORTANT findings.

[kyle-sexton]

Preserve the existing PR body when adding Related — real bug, fixed (commit c00aa43). The guidance now explicitly documents this as a read-modify-write (gh pr view --json body then extend then gh pr edit --body-file -), matching the GitHub adapter's own PR-body-edit identity note (gh pr edit --body/--body-file REPLACES the whole body), rather than the ambiguous bare "append via gh pr edit --body" wording that could have been read as a lossy replacement. Eval 3 updated.

[kyle-sexton]

Match GitHub's fenced-code rules — real bug, fixed (commit c00aa43). The fence detection now matches GFM's actual rules: up to 3 leading spaces before the opener, and a fence closes only on a matching delimiter character (a ~~~ line never closes an open ``` fence, or vice versa) — not the previous bare column-zero, any-delimiter-closes check. Verified locally against an indented fence, a tilde-inside-backtick-fence line, and an HTML comment together in one body — all correctly excluded. Eval 15 extended.

[kyle-sexton]

Strip comments before section emptiness checks — real bug, fixed (commit 8228d6f). Comment text is now never counted as section content, even inside a genuinely matched section — the fence/comment asymmetry (a fence is rendered content and legitimately counts; a comment is never rendered at all, so it never counts) is now reflected in the scan. A required section whose entire body is an unfilled <!-- ... --> placeholder now correctly reads as empty. New eval 17 covers it; eval 16's now-incorrect "preserved inside a found section" expectation for comments corrected to match.