feat(source-control): gate PR bodies against pr-issue-linkage at authoring time

[kyle-sexton]

No linked issue

Summary

The pr-issue-linkage / pr-issue-linkage check is a required merge gate, but nothing enforced
its contract at the moment a PR body was written. A body missing a closing keyword or a
## Related section was therefore only ever caught post-hoc — one CI round trip after the PR was
already open — which is what happened on most PRs filed directly with gh pr create during the
2026-07-29 queue drain.

This adds the missing authoring-time enforcement: a PreToolUse hook on the Bash tool, owned by the
source-control plugin, that validates a gh pr create / gh pr edit body against the same
contract before the call runs and blocks with the missing half named, so the authoring agent
self-corrects in the same turn instead of on the next CI cycle.

/source-control:pull-request create has always run the equivalent pre-create gate
(skills/pull-request/reference/create.md §2.4.2). This hook covers the calls that never go through
the skill; the skill's own path is unaffected, since its gate runs first and the hook then sees a
body that already passes.

Enforcement is keyed to the consumer's own policy

The gate runs only when the repository root carries .github/workflows/pr-issue-linkage.yml (or
.yaml). A repository that does not run the check is never gated, so the hook cannot drift away
from what its consumer actually enforces.

This is deliberately not the pr_body_required_sections seam
(docs/conventions/pr-body-convention/). That key is the repo's configurable section scaffold, and
its portable default excludes Related on purpose; the authority for this gate is the workflow
file that defines the check.

The validator is mirrored, not approximated

Ported from the reusable melodic-software/ci-workflows/.github/workflows/pr-issue-linkage.yml
github-script step, including the three places a hand port silently diverges:

• Both HTML-comment strips, in order — every terminated comment span, then an unterminated
  comment opener swallowing the rest of the body. Without this an unedited PR template, whose
  instructional prose names the very markers the gate looks for, passes vacuously.
• Heading-level semantics — only a heading at the same level or higher closes ## Related, so a
  nested ### ... subsection is that section's content. A naive "next line starting with #"
  reading calls such a section empty and false-blocks a compliant body.
• JavaScript word boundaries, which POSIX ERE has no equivalent for, transcribed as explicit
  non-word characters around a newline-wrapped probe — so Closes #12abc and unclosed #5 stay
  non-matches exactly as they are in CI.

Fail-open on extraction, fail-closed on a determinable bad body

Judged: a --body/-b literal, a readable --body-file/-F path, and the sole heredoc feeding
--body-file - or a --body "$(cat <<EOF ... EOF)" substitution.

Allowed: an unexpanded variable, several heredocs (which one reaches gh is not statically
knowable), an unterminated heredoc, an unreadable body file, an absent body flag (--fill,
--template, --editor, the interactive prompt), and any --repo-targeted invocation, whose
target may not be the repository whose workflow file the scope guard read. Guessing at a body the
hook cannot see would block compliant calls, which costs more than a miss.

The PowerShell tool and direct gh api .../pulls calls are documented as out of scope at the hook's
own site, alongside the --repo limit.

Test plan

• plugins/source-control/hooks/pr-body-linkage-gate.test.sh — 53 black-box cases, all passing:
  the scope guard, both halves independently, all nine closing keywords plus the colon and
  owner/repo#N forms, both no-issue markers, the two word-boundary non-matches, three
  comment-stripping cases, four section-boundary cases (including the deeper-subsection case),
  every body source and every undeterminable-body path, gh pr edit, env/env(1)/sh -c
  wrappers, --repo, and the kill switch.
• Repo gates run locally, all green: shellcheck (with .shellcheckrc), shfmt,
  check-silent-skips, check-hook-userconfig-argv, check-shell-portability (vs origin/main),
  check-cross-plugin-source-drift, sync-hook-utils --check, check-changelog-parity
  (--check and --check-bump), check-plugin-manifest-presence, validate-plugin-contracts,
  validate-plugins, and markdownlint-cli2 on every changed markdown file.
• Dogfooded: this PR's own body was run through the hook before gh pr create fired — and the
  first draft was blocked, correctly. That draft spelled the comment delimiters out literally
  while describing the comment-stripping rule, so the strip ate everything after them, ## Related
  included. CI would have rejected it identically. The hook caught it before the PR existed, which
  is the whole point.

Related

• Refs fix(discipline): make do-your-research state the bar its triggers claim #1748, feat(ci): stage forge/branch/remote portability tokens with class-scoped guards #1745, fix(plugins): bring five ephemeral-file producers onto the tier contract #1708 — PRs whose bodies failed pr-issue-linkage post-hoc during the
  2026-07-29 queue drain, which is the recurring failure this hook removes at the source.
• docs/conventions/pr-body-convention/README.md reserves the enforcement seam for the
  pr_body_required_sections key; this hook deliberately does not consume that key, for the reason
  given under "Enforcement is keyed to the consumer's own policy" above.
• plugins/guardrails/hooks/block-convention-violation.sh gates the gh pr create title
  against the tracked team convention. Different field, different source of truth; the two hooks
  compose rather than overlap.

[claude]

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

---

Security Review — PR #1751

• Gather PR diff and changed files
• Read hook-utils.sh (1425 lines — main attack surface)
• Read pr-body-linkage-gate.sh (406 lines)
• Read hooks.json and plugin.json
• Analyze for injection surfaces, logic flaws, trust boundary issues
• Post findings

---

Findings

IMPORTANT — Telemetry sink executes an arbitrary binary from a consumer-controlled env var

Confidence: PLAUSIBLE

hook-utils.sh:726

printf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

$sink resolves from HOOK_TELEMETRY_SINK. When the value is relative (no leading / or drive letter), it is joined onto the consuming repo root at line 715:

sink="${root%/}/$sink"

A project's .claude/settings.json can set HOOK_TELEMETRY_SINK as an environment variable. If a malicious repository ships:

{ "env": { "HOOK_TELEMETRY_SINK": ".claude/bin/collect.sh" } }

…and that script exfiltrates data or runs arbitrary code, it executes in the background on every gh pr create / gh pr edit invocation in any repository whose $REPO_ROOT carries .github/workflows/pr-issue-linkage.yml. The hook-trigger condition is met by the repository that also controls HOOK_TELEMETRY_SINK — a project can satisfy its own scope guard.

The JSON envelope piped to the sink is safely constructed through jq --arg throughout (no injection there). The threat is the execution of the sink binary itself, which is completely unconstrained: the script the project ships can do anything the user's shell can do.

What matters for risk calibration: Claude Code's permission model gates whether project-level env entries are applied to hook processes without explicit user approval. If they are applied automatically (the common case for project-scoped settings that are already committed to the repo the user opened), this is a code-execution primitive available to any project that co-opts a user who has this plugin installed. This warrants explicit documentation that HOOK_TELEMETRY_SINK is an operator-only configuration (machine-level or user-level settings, never project-level), plus a note in hook-utils.sh alongside the existing comment block at line 719.

---

SUGGESTION — --body-file path is not confined to the repository or project directory

Confidence: CONFIRMED

pr-body-linkage-gate.sh:381

local path="$body_val"
[[ "$path" == /* || "$path" =~ ^[A-Za-z]:[\\/] ]] || path="${HOOK_CWD:-$REPO_ROOT}/$path"
[[ -r "$path" ]] || return 0
body=$(cat -- "$path") || return 0

The hook reads any file the agent supplies via --body-file, including absolute paths or relative paths that traverse outside the repo (../../sensitive-file). The file content is never echoed in the block output — only a structural verdict (pass/fail) is returned — so there is no direct exfiltration path here.

The residual risk is a covert side-channel: an attacker who can influence Claude into running gh pr create --body-file /etc/hostname can probe file readability and infer one bit of content per hook invocation (did it contain a closing keyword?). This is very low impact given that the agent controls its own commands, but it is an undocumented filesystem access pattern. Adding a membership check against REPO_ROOT (analogous to the hook::read_file_path guard in the same library) would close it and is consistent with the defensive posture elsewhere in the hook.

---

What looks good

• No command injection in the parser: hook::bash_parse_segments is a pure static tokenizer. It never passes user-supplied strings to eval, exec, or a subshell — the callback (check_segment) receives an already-split word array.
• HTML-comment stripping is a state machine, not a regex: correctly handles nested and unterminated comment openers so an unedited PR template cannot pass vacuously, matching the CI validator exactly.
• Word-boundary emulation is correct: wrapping the probe in $'\n'...$'\n' and using [^a-z0-9_] anchors faithfully reproduces JavaScript \b, including the #12abc/unclosed #5 non-match cases the PR description calls out.
• hook::ansi_c_decode %-escapes before printf: format-string injection from an ANSI-C quoted token is prevented at the only call site where user-controlled data reaches printf's format argument.
• sole_heredoc_body correctly refuses multiple heredocs: judging the wrong body (when several heredocs appear) would block compliant calls; returning 1 preserves the fail-open contract.
• HOOK_CWD and REPO_ROOT path construction is safe: all paths reach git -C, cat --, and [[ -f ]] as properly quoted positional arguments — no word-splitting or glob expansion.
• Session marker sanitisation in hook::notice_once strips all non-[A-Za-z0-9_-] characters, preventing path traversal in the $dir/${key}.${session} filename.

[claude]

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

---

Security Review — PR #1751

• Gather PR diff and changed files
• Read hook-utils.sh (1425 lines — main attack surface)
• Read pr-body-linkage-gate.sh (406 lines)
• Read hooks.json and plugin.json
• Read test file and README
• Analyze for injection surfaces, logic flaws, trust boundary issues
• Post findings

---

Overall assessment

No critical security vulnerabilities found. The hook's failure modes are all fail-open (ambiguous parse → enforcement skipped), not code-executing or incorrectly-blocking. The five findings below are correctness gaps and minor edge cases; none creates an exploitable security vector given the hook's stated purpose (authoring-time quality enforcement, not a security sandbox).

---

Finding 1 — check_segment's env | command stripper lacks a command -v bail-out

Severity: Low (correctness, no false block)

hook::git_resolve_index in hook-utils.sh correctly bails when it encounters command -v or command -V (lines 1033–1035):

if ((is_command)) && [[ "${w[i]}" == -*[vV]* ]]; then
  return 1
fi

The analogous strip loop in check_segment (pr-body-linkage-gate.sh:312–319) has no such guard. A call like command -v gh pr create would be mis-parsed as gh pr create.

In practice no false block results — there is no --body flag in command -v gh pr create, so check_segment finds body_flag == "" and returns 0. But the semantic divergence from the git resolver is a latent correctness gap that could matter if the segment ever did carry a body flag after command -v. Worth aligning with the git resolver's posture.

---

Finding 2 — printf '%(...)T' in hook::emit_telemetry requires bash 4.2+; the hook documents 3.2+ support

Severity: Very low (malformed telemetry timestamp on macOS, no functional impact)

hook-utils.sh:689:

timestamp=$(TZ=UTC printf '%(%Y-%m-%dT%H:%M:%SZ)T' -1)

printf '%(...)T' is a bash 4.2+ extension. macOS ships bash 3.2. On that host the printf format is emitted literally as the timestamp field value, producing a malformed (but syntactically valid-JSON) timestamp in the telemetry envelope. Since telemetry is opt-in and the hook itself still functions correctly, this is low-impact — but it silently degrades telemetry quality on macOS without any notice.

hook::buffer_stdin's own docblock (hook-utils.sh:329) documents the same 3.2+/4.1+ split and guards read -N behind a version check. The same guard pattern (or date -u +%Y-%m-%dT%H:%M:%SZ as a fallback) would close the gap here.

---

Finding 3 — --repo= (empty value) bypasses enforcement despite not naming a foreign repo

Severity: Low (theoretical bypass, gh would error on empty --repo)

pr-body-linkage-gate.sh:335:

-R | --repo | -R?* | --repo=*) return 0 ;;

The glob --repo=* matches zero-or-more characters, so --repo= (empty value) triggers the early return. The documented intent is to skip enforcement when the target repository differs from the local one. An empty --repo doesn't satisfy that intent — it's not a foreign-repo invocation. gh would reject --repo= at runtime, so no PR is created via this path; but the hook's contract says it validated it when it didn't. A tighter pattern like --repo=?* (requires at least one character after =) would match the stated intent.

---

Finding 4 — Relative --body-file paths are resolved against HOOK_CWD, not the command's effective CWD

Severity: Low (correctness gap; fails open, never false-blocks)

pr-body-linkage-gate.sh:381:

[[ "$path" == /* || "$path" =~ ^[A-Za-z]:[\\/] ]] || path="${HOOK_CWD:-$REPO_ROOT}/$path"

HOOK_CWD is Claude Code's working directory at hook-invocation time. If a command includes a cd before the gh call (e.g. in a compound statement cd /tmp && gh pr create --body-file body.md), the hook resolves body.md relative to the Claude Code CWD, not /tmp. Two outcomes:

• File found at the wrong path → hook validates the wrong file (may false-allow or incorrectly surface as non-compliant).
• File not found → return 0 (fail-open, no false block).

The fail-open leg avoids correctness harm in most cases. Worth documenting as a known limitation at the hook's site, or — if pre-split segment CWD tracking is available from the hook input — using the command's inferred CWD instead.

---

Finding 5 — sole_heredoc_body delimiter-unquoting order can misparse mixed-quote delimiters

Severity: Very low (fail-open; no false block)

pr-body-linkage-gate.sh:224–229:

delim="${delim#\\}"
delim="${delim#\'}"
delim="${delim%\'}"
delim="${delim#\"}"
delim="${delim%\"}"

For a delimiter like <<"'EOF'" (a double-quoted token containing a single-quoted string — unusual but legal bash), the raw captured string is "'EOF'". After stripping the leading \ (no-op), then leading/trailing ' — which don't appear at the outermost level — then leading/trailing ":

• Strip " prefix → 'EOF'"
• Strip " suffix → 'EOF'
• delim = "'EOF'"

The actual bash heredoc closes on 'EOF'; the function would correctly use 'EOF' as the delimiter. But this breaks for the similar <<'"EOF"' case: stripping leading ' gives "EOF"' and trailing ' gives "EOF", then no " stripping occurs — delim = '"EOF"'. The heredoc closes on "EOF" but the function looks for "EOF"'. Mismatch → return 1 (fail-open, body not read). Obscure and fail-open, but documents a divergence from bash's actual quoting semantics.

---

Design observations (not vulnerabilities)

Fail-open surface accumulation. Seven documented bypass paths are individually justified, but their union is substantial: any command the agent structures with dynamic values, multiple heredocs, an unreadable file, or --repo is never gated. This matches the stated "friction, not sandbox" posture — the CI gate still enforces on the server side — but is worth keeping visible in the hook's site documentation so future authors don't remove one bypass thinking another covers its cases.

Word-boundary transcription is correct. has_linkage wraps the probe in \n on both sides before matching against KEYWORD_ERE. Verified: Closes #12abc fails because a satisfies [a-z0-9_] in the trailing [^a-z0-9_] class; unclosed #5 fails because d in unclosed is a word character that precedes #, so the leading [^a-z0-9_] never fires. Both cases mirror JavaScript's \b semantics as claimed.

Comment-stripping logic mirrors the CI validator. The two-pass structure (terminated spans, then unterminated opener swallowing the rest) is correct and verified against the test cases at lines 132–137.

Test coverage is thorough. The 53 cases cover scope guard, both validation halves, nine keyword variants, both word-boundary non-matches, both comment-stripping forms, all four section-boundary shapes, every body-source form, wrapper unwrapping (env, env(1), sh -c), and the kill switch. The self-contained fixture repos with an unrelated CWD correctly exercise the cwd-anchored scope guard.

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66432f0855

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

---

Security Review — PR #1751

• Gather PR diff
• Read pr-body-linkage-gate.sh
• Read hook-utils.sh (trust boundary / sink execution)
• Read hooks.json and plugin.json
• Analyze trust boundaries, injection surfaces, and logic flaws
• Post findings

---

Finding 1 — HOOK_TELEMETRY_SINK executes an arbitrary binary from a project-controlled env var

Severity: IMPORTANT — Confidence: CONFIRMED

hook-utils.sh line 726:

printf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

$sink is resolved from HOOK_TELEMETRY_SINK. When relative, it is joined onto REPO_ROOT at line 715:

sink="${root%/}/$sink"

root falls back to CLAUDE_PROJECT_DIR when no explicit root is passed. A project's .claude/settings.json env block can set HOOK_TELEMETRY_SINK; Claude Code applies project-scoped env entries to hook processes. The same project can carry .github/workflows/pr-issue-linkage.yml to satisfy its own scope guard. This creates a complete, self-contained code-execution primitive: a malicious repository ships the workflow file, a .claude/settings.json env entry pointing to a relative path, and the executable at that path. On any gh pr create or gh pr edit invocation in that session, the sink is executed in the background.

The JSON envelope piped to the sink is safely constructed through jq --arg throughout — there is no injection into the envelope itself. The threat is the execution of the sink binary, which is entirely unconstrained: it can exfiltrate data, modify files, or perform any action the user's shell permits.

The README update in a0ae5d5 now documents HOOK_TELEMETRY_SINK as opt-in telemetry, but there is no enforcement that the variable originates from user-level or machine-level settings rather than project-level. The sink binary path is also unconstrained (no extension check, executable bit check, or path-membership guard like hook::read_file_path applies to the file hooks).

Mitigation options (in ascending order of strength):

1. Document HOOK_TELEMETRY_SINK as operator-only (user-level or machine-level settings.json, never project-level) in hook-utils.sh alongside the existing comment block at the sink resolution step — the current README addition is a start but doesn't reach the library itself.
2. Confine the resolved sink path to a known-safe directory (e.g., CLAUDE_PROJECT_DIR/.claude/ or a user-level config dir) and reject any sink that does not resolve inside it.
3. Require HOOK_TELEMETRY_SINK to be an absolute path, eliminating the project-relative resolution leg entirely — team portability can be preserved by documenting a wrapper pattern using an absolute path to a script that reads its target from the project config.

The first automated review flagged this as PLAUSIBLE; the code path is clear enough to call it CONFIRMED. This is the only finding in this review that warrants a design-level response before merge.

---

Finding 2 — --body-file path is not confined to the repository

Severity: SUGGESTION — Confidence: CONFIRMED

pr-body-linkage-gate.sh line 381:

[[ "$path" == /* || "$path" =~ ^[A-Za-z]:[\\/] ]] || path="${HOOK_CWD:-$REPO_ROOT}/$path"
[[ -r "$path" ]] || return 0
body=$(cat -- "$path") || return 0

The hook reads any file the agent supplies via --body-file, including paths outside the repository (absolute paths or ../ traversal). File content is never echoed in the block output — only a structural verdict (pass/fail) is returned. The residual risk is a covert read side-channel: an actor who can influence the agent into calling gh pr create --body-file /etc/hostname can probe file readability and infer one bit of structural content per invocation. This is very low impact given the agent controls its own commands, but it is undocumented filesystem access. Adding a path-membership check against REPO_ROOT (analogous to the hook::read_file_path guard in the same library) would close the channel consistently with the defensive posture elsewhere.

---

Finding 3 — --repo= (empty value) satisfies the foreign-repo bypass glob

Severity: SUGGESTION — Confidence: CONFIRMED

pr-body-linkage-gate.sh line 335:

-R | --repo | -R?* | --repo=*) return 0 ;;

The glob --repo=* matches zero-or-more characters, so --repo= (empty value) triggers the early return. The documented intent is to skip enforcement when the target differs from the local repository. An empty --repo= does not satisfy that intent. gh rejects --repo= at runtime so no PR is ever created through this path, but the hook misclassifies the invocation as "foreign repo — enforcement skipped" when gh would have rejected it regardless. --repo=?* (at least one character after =) would align the glob with the stated intent.

---

What was examined and found clean

• hook::bash_parse_segments + check_segment parser: no eval, exec, or subshell over user-controlled strings. The tokenizer operates on the literal command text only. The callback receives an already-split word array.
• Envelope construction: all fields injected via jq --arg/--argjson. $FORM and $status are hook-internal literals, never derived from PR body content or user input.
• strip_html_comments / related_section / has_linkage: process body content through pure string operations and bash =~ regex matching — no exec paths. A large or adversarial body is bounded by the 15-second hook timeout.
• sole_heredoc_body and is_dynamic: correctly fail open (return 1 / true) rather than judging an unresolvable body. The multiple-heredoc guard prevents validating the wrong text when several heredocs appear.
• hook::notice_once session marker: strips all non-[A-Za-z0-9_-] characters before using the key in a filename, preventing path traversal in the $dir/${key}.${session} path.
• hook::repo_root: uses git -C "$hint" rev-parse --show-toplevel with a properly quoted argument — no word-splitting or glob expansion.

[claude]

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

---

PR Review — #1751 feat(source-control): gate PR bodies against pr-issue-linkage at authoring time

• Read changed files
• Analyze correctness, security, architecture, and test coverage
• Post findings

---

Overall Assessment

Well-structured addition. The scope guard, fail-open design, and test suite are all solid. Two prior Claude security reviews and one Codex review have already covered the main attack surfaces; both Codex-flagged issues (exec bit, telemetry disclosure) were correctly addressed in follow-on commits. What follows covers the remaining open items and a few correctness observations not yet addressed.

---

Remaining Open Issues from Prior Reviews

1 — HOOK_TELEMETRY_SINK operator-only constraint is still undocumented at the code level

The first security review flagged that a project's .claude/settings.json can set HOOK_TELEMETRY_SINK as an env var, causing the hook to execute an arbitrary consumer-controlled binary in the background on every gated invocation. The a0ae5d5 commit disclosed the telemetry in the README's Security section, but neither the README nor hook-utils.sh states that HOOK_TELEMETRY_SINK must be set in user- or machine-level settings, never in a tracked project settings.json.

A project can satisfy its own scope guard (ship .github/workflows/pr-issue-linkage.yml) and simultaneously set HOOK_TELEMETRY_SINK to a relative path in that same repo — a combination that executes the named script on every gh pr create in any repository with the plugin installed. The risk materialises only if a user opens a malicious repo, so it is not a remote-code-execution vector without social engineering; but it is undocumented, and the mitigating note belongs in the code.

Suggested addition in the hook::emit_telemetry block header in hook-utils.sh around line 637:

# SECURITY: HOOK_TELEMETRY_SINK must be set in user- or machine-level settings only —
# never in a tracked project settings.json. A project controls its own scope-guard
# file and could otherwise use a project-relative sink to execute arbitrary code on
# every hook invocation in any repository where the plugin is installed.

And a note in the README's #### Telemetry (opt-in) subsection.

---

2 — --repo= (empty value) short-circuits enforcement without naming a foreign repo

pr-body-linkage-gate.sh:335

-R | --repo | -R?* | --repo=*) return 0 ;;

--repo=* matches zero-or-more characters, so --repo= (empty value) returns 0 before reading the body. gh rejects an empty --repo at runtime, so no PR is created either way — the hook's bypass doesn't create a practical security problem. But the contract says it skips enforcement because the target repo may differ; an empty --repo doesn't satisfy that. --repo=?* (one or more characters after =) would match intent.

---

3 — command -v gh pr create is mis-parsed as a live gh pr create

pr-body-linkage-gate.sh:312-319

The check_segment wrapper-stripping loop steps past env/command tokens but does not apply the hook::git_resolve_index guard that bails on command -v/-V (hook-utils.sh:1033–1035). A command like command -v gh pr create would be parsed as a live gh pr create invocation. In practice no body flag accompanies command -v gh pr create, so body_flag == "" → return 0 — no false block. It is a latent correctness gap, not a correctness defect today.

---

4 — printf '%(...)T' in hook::emit_telemetry is Bash 4.2+; the hook supports 3.2+

hook-utils.sh:689

timestamp=$(TZ=UTC printf '%(%Y-%m-%dT%H:%M:%SZ)T' -1)

macOS ships Bash 3.2. The printf '%(...)T' format is a Bash 4.2 extension; on 3.2 it emits the format string verbatim as the timestamp, producing a malformed (but syntactically valid JSON) string in the telemetry envelope. No functional impact to the hook itself — telemetry is opt-in and the block/allow decision is unaffected — but the degradation is silent. date -u +%Y-%m-%dT%H:%M:%SZ as a fallback (with the version-gate pattern already used elsewhere in this file at line 329) would close it.

---

Correctness Observations

5 — --repo in takes_value is dead code

pr-body-linkage-gate.sh:282

The scanning loop matches -R | --repo | -R?* | --repo=* as an early-return case arm before the *) fallthrough where takes_value is called. The -R/--repo entries in takes_value are therefore never reached. Harmless, but a future reader may wonder whether --repo's argument is being consumed, or rely on takes_value to do so while inadvertently widening the early-return guard.

6 — Stray second argument on the success emit_tel call

pr-body-linkage-gate.sh:405

emit_tel "ok" ""

emit_tel only uses $1; the trailing "" is silently ignored. The emit_tel "blocked" call at line 255 passes one argument. The extra argument here is harmless but inconsistent.

7 — Skill KEYWORD_REGEX (§2.4.2.1) does not handle owner/repo#N or word-boundary constraints

create.md:326

KEYWORD_REGEX='^(close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved):? #[0-9]+'

The hook's KEYWORD_ERE is a closer mirror of the CI validator: it handles owner/repo#N and transcribes JavaScript word boundaries. The skill's own gate predates this PR and is a separate concern (it uses ^ line anchoring and requires a space between :? and #), but the divergence means a body with Closes melodic-software/foo#5 passes the hook and CI but would fail the skill's own gate. This is a pre-existing inconsistency, not introduced here, but it's now more visible now that both gates exist side-by-side.

---

What Looks Good

• Scope guard keyed to the consumer's own workflow file is the right authority signal — the hook cannot drift from CI enforcement regardless of plugin version.
• Fail-open design is correctly implemented. Every undeterminable path ($VAR in body, multiple heredocs, unreadable file, absent body flag, --repo) exits 0. The test suite confirms all these pass.
• HTML-comment stripping is a state machine, not a one-shot regex. The two-pass structure (terminated spans → unterminated <!-- swallows remainder) matches the CI validator exactly, including the unedited-template case the PR description calls out.
• Word-boundary transcription is correct. Wrapping the probe in $'\n'...$'\n' and using [^a-z0-9_] anchors faithfully reproduces \b — Closes #12abc and unclosed #5 fail exactly as they do in CI.
• ## Related section boundary is heading-level-aware. ### subsections count as section content (not terminators), matching the CI validator's semantics.
• sole_heredoc_body correctly refuses multiple heredocs (returns 1), preserving the fail-open contract for ambiguous stdin feeding.
• block() output is agent-actionable. Naming the missing half and the exact lines to add lets the agent self-correct in the same turn.
• Test suite is thorough. 53 black-box cases across all documented paths, with fixture repos that exercise the CWD-anchored scope guard correctly.
• Plugin manifest, README, and CHANGELOG are all updated in a single coherent version bump.

---

[kyle-sexton]

Port verified differentially against the real validator

Beyond the 53 committed black-box cases, I ran a one-off differential harness: the
melodic-software/ci-workflows github-script body transcribed verbatim into a Node script, and
the hook, judging the same 46 body fixtures and compared verdict-for-verdict.

46 compared, 46 agree, 0 disagree. The corpus deliberately targets where a hand port diverges:

• Comment handling — terminated span, unterminated opener, comment split across a line boundary, a
  comment that hides only the marker, one that hides only ## Related, a nested-looking
  <!-- <!-- ... -->, and a keyword broken in half by an inline comment (Clos<!-- x -->es #5).
• Section boundaries — ### Sub as content, a same-level heading terminating the section, a #
  heading terminating it, a seven-hash line that is not a heading, ## Related work,
  ##\trelated, ##Related with no space, an indented heading, whitespace-only content, and the
  section running to EOF.
• Keyword boundaries — #12abc, #12_x, #12., unclosed #5, _closes #5, 1closes #5,
  fixe #5, resolvess #5, a newline between keyword and #, owner/repo#N, a dotted owner, and
  the marker near-misses No linked issue, Nolinked issue, No linked issues.
• CRLF bodies, and an empty body.

The harness is scratch, not committed: making it permanent would mean vendoring a hand-copy of
upstream's JavaScript into this repo, which is exactly the drift the pointer-not-copy rule exists to
prevent. The committed suite covers the same ground with fixtures this repo owns.

Worth recording: this PR's own first-draft body was blocked by its own hook. The draft spelled
the comment delimiters out literally while explaining the comment-stripping rule, so the strip ate
everything after them, ## Related included. CI would have rejected it identically — the hook just
said so before the PR existed.

[claude]

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

---

PR Review — #1751 feat(source-control): gate PR bodies against pr-issue-linkage at authoring time

• Gather PR diff and identify changed files
• Read pr-body-linkage-gate.sh (main hook — 412 lines)
• Read hook-utils.sh (utility library — 1425 lines, telemetry section)
• Read hooks.json, plugin.json, README.md, test file
• Assess prior review findings against current HEAD (9e29350)
• Post findings

---

Overall Assessment

Three prior security reviews and one Codex review have covered this thoroughly. The fixes from those reviews are all present in current HEAD:

Fix	Commit
Exec bit on hook and test (100755)	5db7e3e
Telemetry disclosure in README	a0ae5d5
Fail-open when --body/--body-file has no trailing word	c57fae3

The implementation is sound. What follows is the residual open items as of 9e29350, ordered by impact.

---

Finding 1 — hook-utils.sh comment actively recommends the unsafe HOOK_TELEMETRY_SINK configuration pattern

Severity: IMPORTANT — Confidence: CONFIRMED

hook-utils.sh:653–659:

# Sink path resolution: HOOK_TELEMETRY_SINK may be absolute OR relative to the
# consuming repo root. …Relative is the portable, team-shared wiring form:
# CC injects settings.json env values literally (no ${VAR} expansion), so a
# relative path tracked in settings.json is the only clone-portable,
# worktree-safe option.

This comment guides users toward tracking HOOK_TELEMETRY_SINK in a project's committed settings.json and calls it "the portable, team-shared wiring form." That is exactly the configuration that creates the code-execution primitive: a malicious repository ships .github/workflows/pr-issue-linkage.yml (to satisfy its own scope guard), a tracked .claude/settings.json with HOOK_TELEMETRY_SINK pointing at a relative path inside the repo, and an executable at that path. Any user who opens that repository and runs a gh pr create has the sink executed in the background.

The README's Telemetry (opt-in) section documents the feature without mentioning the project-level risk. The README Security section mentions no telemetry unless opted in, but gives no guidance on where the opt-in should live.

Minimum change before merge — add a SECURITY note to the hook::emit_telemetry docblock alongside the sink-resolution paragraph:

# SECURITY: HOOK_TELEMETRY_SINK must be configured in user-level (~/.claude/settings.json)
# or machine-level settings only — never in a tracked project .claude/settings.json.
# A project controls its own scope-guard file and could otherwise use a relative
# sink to execute arbitrary code on every gated hook invocation in any repository
# where the plugin is installed. A relative value in tracked settings.json is
# "portable" only in the non-malicious case; the portability is real but the
# trust model is user-level, not project-level.

And mirror the constraint in the README's Telemetry (opt-in) subsection.

Fix this →

---

Finding 2 — --repo=* glob matches the empty-value form

Severity: SUGGESTION — Confidence: CONFIRMED

pr-body-linkage-gate.sh:335:

-R | --repo | -R?* | --repo=*) return 0 ;;

--repo=* matches zero or more characters, so --repo= (empty value, no repo named) returns 0 and skips enforcement. gh rejects --repo= at runtime, so no PR is ever created through this path — no practical impact. But the hook's contract says it skips because "the target repository may not be the one whose gate file was read," and an empty --repo doesn't satisfy that. --repo=?* (one or more characters after =) would match the stated intent.

Fix this →

---

Finding 3 — command -v gh pr create is parsed as a live gh pr create

Severity: Low (no false block today) — Confidence: CONFIRMED

pr-body-linkage-gate.sh:312–319

The wrapper-stripping loop strips command as a prefix then continues, landing on gh. It does not apply the command -v/-V bail-out that hook::git_resolve_index uses (hook-utils.sh:1033–1035). In practice there is no false block — command -v gh pr create carries no --body, so body_flag == "" → return 0. It is a latent correctness gap and a semantic divergence from the git resolver worth closing in a follow-up.

---

Finding 4 — printf '%(...)T' requires Bash 4.2+; the hook documents 3.2+ support

Severity: Very low — Confidence: CONFIRMED

hook-utils.sh:689:

timestamp=$(TZ=UTC printf '%(%Y-%m-%dT%H:%M:%SZ)T' -1)

printf '%(…)T' is a Bash 4.2 extension. macOS ships Bash 3.2; on that host the format string is emitted literally, producing a malformed (but syntactically valid JSON) timestamp in the telemetry envelope. No functional impact — the hook's block/allow decision is unaffected and telemetry is opt-in — but the degradation is silent. The version guard at hook-utils.sh:329 (hook::read_supports_nchars) shows the pattern; date -u +%Y-%m-%dT%H:%M:%SZ as a fallback would close it consistently with that approach.

---

Minor Correctness Observations

Dead --repo entry in takes_value (pr-body-linkage-gate.sh:283): -R | --repo is matched by the early-return case arm at line 335 before control ever reaches takes_value. The entry is dead but could mislead a future reader wondering whether --repo's argument is being consumed.

Stray second argument on the success emit_tel call (pr-body-linkage-gate.sh:411): emit_tel "ok" "" — emit_tel uses only $1; the "" is silently ignored. emit_tel "blocked" at line 255 passes one argument. Harmless but inconsistent.

---

What Looks Good

• Scope guard is the right authority signal: keyed to the consumer's own .github/workflows/pr-issue-linkage.yml, so the hook cannot drift from CI enforcement regardless of plugin version.
• Fail-open design is correctly implemented and tested: all undeterminable paths ($VAR body, multiple heredocs, unreadable file, no body flag, --repo, trailing --body with no word) return 0. The c57fae3 trailing-flag fix is in place.
• HTML-comment stripping is a state machine, not a one-shot regex: matches the CI validator's two-pass structure (terminated spans → unterminated opener swallows remainder).
• Word-boundary transcription is correct: $'\n'-wrapped probe + [^a-z0-9_] anchors faithfully reproduce JavaScript \b; Closes #12abc and unclosed #5 are non-matches exactly as in CI.
• ## Related section boundary is heading-level-aware: ### subsections count as content, not terminators — matches the validator semantics and the PR's own description.
• sole_heredoc_body correctly refuses multiple heredocs: returns 1 rather than guessing which heredoc reaches gh.
• block() output is agent-actionable: names the missing half and the exact text to add.
• 53-case test suite covers every documented path, with fixture repos from an unrelated CWD to correctly exercise the scope guard.
• Plugin manifest, README, CHANGELOG, and version bumped in a coherent 0.37.0 increment.

---

Summary

One actionable item before merge: Finding 1 — the hook-utils.sh comment block should include a SECURITY: note warning that HOOK_TELEMETRY_SINK belongs in user- or machine-level settings only, never in a tracked project settings.json, with a matching note in the README. All other findings are low-to-cosmetic. The core implementation is correct and the fail-open contract is solid throughout.

[claude]

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

---

Security Review — PR #1751

• Gather PR diff and identify changed files
• Read pr-body-linkage-gate.sh
• Read hook-utils.sh (trust boundary / sink execution)
• Read hooks.json, plugin.json, README.md
• Analyze trust boundaries, data-flow, injection surfaces, and logic
• Post findings

---

Addressed since the initial commits

Two findings from the Codex review were resolved in-branch:

• Exec bit (5db7e3e) — pr-body-linkage-gate.sh is now 100755 in the index. Confirmed via git ls-files -s.
• Telemetry disclosure (a0ae5d5) — the README Security section now says "no telemetry unless you opt in" and a Telemetry (opt-in) subsection documents the envelope payload. This is a necessary disclosure, but it does not close the architectural concern below.

---

IMPORTANT — HOOK_TELEMETRY_SINK operator-only constraint is absent from both the code and the consumer-facing documentation

Confidence: CONFIRMED

Three prior reviews in this thread raised this. It is still open.

hook-utils.sh line 709–726:

local sink="$HOOK_TELEMETRY_SINK"
case "$sink" in
/* | [A-Za-z]:[/\\]*) ;;
*)
  local root="${repo_root:-${CLAUDE_PROJECT_DIR:-}}"
  [[ -n "$root" ]] || return 0
  sink="${root%/}/$sink"
  ;;
esac
printf '%s\n' "$envelope" | ("$sink" >/dev/null 2>&1) &

HOOK_TELEMETRY_SINK is an environment variable. Claude Code applies env entries from a project's .claude/settings.json to hook processes. A malicious repository can:

1. Carry .github/workflows/pr-issue-linkage.yml — satisfying the hook's own scope guard.
2. Carry .claude/settings.json with "env": { "HOOK_TELEMETRY_SINK": ".evil/collect.sh" } — pointing to a project-relative executable.
3. Ship .evil/collect.sh at that path.

On any gh pr create or gh pr edit the user or Claude issues while that repository is open, $CLAUDE_PROJECT_DIR/.evil/collect.sh executes in a background subshell with full access to the user's environment (credentials, filesystem, network). The hook's timeout (15 s from hooks.json) does not bound the background process — the subshell is detached with &.

The a0ae5d5 README addition says the telemetry is opt-in, but nowhere — in the README, the hook-utils.sh comment block, or plugin.json — does it say that HOOK_TELEMETRY_SINK must not be set in a tracked project settings.json. A user who reads the README has no signal that this distinction matters.

What makes this different from a general "don't open malicious repos" caveat: the plugin is a quality gate, and the team workflow explicitly instructs contributors to clone or check out foreign repositories (review, CI triage, babysit-prs across owners). The sink attack is reachable in that normal workflow without the user taking any unusual action.

Minimum viable fix: add a security note to the hook::emit_telemetry block header in hook-utils.sh (lines 637–662) and to the README's #### Telemetry (opt-in) subsection stating that HOOK_TELEMETRY_SINK must be set at user-level or machine-level settings (~/.claude/settings.json or /etc/claude/settings.json), never in a project's .claude/settings.json. This does not prevent a determined attacker but converts an undocumented trap into a documented constraint.

A stronger fix would confine the resolved sink path to a safe directory (e.g., require an absolute path, or check that the resolved path falls outside CLAUDE_PROJECT_DIR). Requiring an absolute path eliminates the project-relative leg entirely and is the most straightforward structural fix.

---

SUGGESTION — --body-file reads arbitrary filesystem paths

Confidence: CONFIRMED

pr-body-linkage-gate.sh line 386–389:

[[ "$path" == /* || "$path" =~ ^[A-Za-z]:[\\/] ]] || path="${HOOK_CWD:-$REPO_ROOT}/$path"
[[ -r "$path" ]] || return 0
body=$(cat -- "$path") || return 0

The hook reads any readable file the agent supplies via --body-file, including absolute paths (/etc/shadow) and relative paths that traverse outside the repository (../../sensitive). File content is never echoed in the blocking output — only a structural verdict is returned — so there is no direct exfiltration path. The residual risk is a covert side-channel: an actor who can steer Claude into running gh pr create --body-file /etc/hostname can probe file readability and infer one structural bit per invocation (does the file contain a closing keyword?). Impact is low given the agent controls its own commands. Adding a path-membership check against REPO_ROOT (using the same hook::normalize_path / hook::physical_path pattern already implemented in hook::read_file_path in the same library) would close the channel consistently with the defensive posture elsewhere.

---

SUGGESTION — --repo= (empty value) bypasses enforcement

Confidence: CONFIRMED

pr-body-linkage-gate.sh line 335:

-R | --repo | -R?* | --repo=*) return 0 ;;

The glob --repo=* matches zero-or-more characters, so --repo= (empty value) triggers the early return and skips enforcement. gh rejects an empty --repo at runtime so no PR is actually created through this path — the practical impact is nil. But the hook's documented intent is "skip when the target repository differs from the local one"; an empty --repo= does not satisfy that condition. --repo=?* (one or more characters after =) would align the pattern with the stated contract.

---

What looks clean (independent verification)

• Parser is injection-free: hook::bash_parse_segments and check_segment perform no eval, exec, or subshell over user-controlled strings. The ANSI-C decode at hook::ansi_c_decode pre-escapes % to %% before passing content to printf, preventing format-string injection.
• Envelope construction is safe: all fields reach jq --arg / --argjson; no PR body content touches the format string.
• HTML comment stripping is a correct state machine: the two-pass logic (terminated spans, then unterminated <!-- swallowing the rest) faithfully mirrors the CI validator and handles the unedited-PR-template case.
• Word-boundary transcription is correct: the \n…\n wrapping with [^a-z0-9_] anchors reproduces JavaScript \b semantics, including the Closes #12abc and unclosed #5 non-match cases.
• Heading-level-aware ## Related boundary: lvl <= 2 gates the terminator check, so ### Sub subsections are correctly treated as section content, not terminators.
• sole_heredoc_body multiple-heredoc guard: returns 1 on more than one heredoc, correctly refusing to judge an ambiguous stdin source.
• hook::notice_once key sanitization: strips all non-[A-Za-z0-9_-] characters before constructing the marker filename — path traversal in $dir/${key}.${session} is not possible.
• Scope guard reads only file existence, not content: no injection surface from the workflow file itself.
• hook::repo_root is injection-safe: git -C "$hint" passes the hint as a positional argument, no word-splitting or glob expansion.