feat(source-control): pin plugin install --config behaviours and move branch_issue_pattern to the cascade

[kyle-sexton]

No related issue: the source item is a local handoff queue entry; the one follow-up it produced is #4572, linked under Related, and this PR does not close it.

Summary

Records four claude plugin install --config behaviours in the hook-config-delivery facts table,
corrects the reconfigure scope caveat that those measurements contradict, rules on where
source-control's babysit identity and topology keys live, moves branch_issue_pattern onto the
.claude/source-control.md cascade, and makes the two os_toast option descriptions name their
platform.

Fix

• hook-config-delivery (contract 1.3.0): facts 9-12 with a per-row Verified column, re-measured
  in a sandbox on Claude Code 2.1.283 (2026-09-27):
  • D1: --config writes pluginConfigs to user settings whatever -s says; -s governs only the
    install record and enabledPlugins.
  • D2: a wrong-type boolean, an undeclared key, and an empty value are rejected with a warning and
    exit 0; a prose-enumerated string and a non-existent directory are stored. A declared
    options fixed list is enforced (doc-stated, plugins need v2.1.271 to load it).
  • D3: no config/configure subcommand and no CLI unset path.
  • D4: a same-scope rerun is a pure config write (string at user and project scope, boolean and
    directory at local scope).
  • Four new recheck triggers.
• plugin-reconfiguration: verified-version record moved from 2.1.240 to 2.1.283 with the measured
  coverage. Caveat 2's rationale ("or the write lands at a scope that does not load") was wrong: the
  value always lands in user settings. The measured risk is the reverse: a rerun at another scope adds
  an install record there and enables the plugin at that scope (a default -s user rerun enables it
  machine-wide). New caveat 4: read the CLI output, not the exit code. plugin-philosophy,
  extensibility-contract-smoke-tests, and migration-playbook now point at this record instead of
  restating 2.1.240.
• ADR 0039: the 17 babysit keys split three ways. Six authority-bearing keys stay userConfig,
  with the multi-domain consequence stated. branch_issue_pattern moves now. Ten repository-tooling
  keys move later under source-control: migrate ten babysit repository-tooling keys from userConfig to the source-control.md cascade #4572 with default-branch resolution and union/unit merge modes.
  config-resolution.md states the consequence and cites the ADR.
• source-control 0.61.0: parse-branch-issue.sh resolves branch_issue_pattern from the three
  cascade layers (local > team > user-global) before the now-deprecated userConfig value, which
  prints a deprecation note on stderr and is kept as a fallback until a later minor release no
  earlier than 2026-12-27. Hardening: a fenced value is read, a fenced heading is ignored, a
  backreference pattern is rejected, output must be digits, and notes never echo the raw pattern.
  setup checks and writes the key. create.md passes CLAUDE_PROJECT_DIR="$WT" on the
  out-of-tree path and no longer discards the script's stderr.
• autonomy 0.24.2, desktop-notification 0.6.49: os-toast descriptions name macOS (osascript)
  and Linux (notify-send), with no effect on Windows; README option blocks regenerated.
• typos-format 0.6.63, desktop-notification, source-control setup skills: caveat-2 rationale
  corrected. The uninstall recipe the item names was already removed in typos-format 0.6.23 (fix(setup): unstamped --config claim prescribes a destructive reinstall across 18 setup skills #3111).
• Fleet sweep, one patch bump each: the same caveat-2 rationale corrected in the setup skills of
  actionlint, ai-briefing, bash-format, biome-format, bugs, context-budget, disk-hygiene, education,
  eol-normalizer, go-format, knowledge, machine-health, markdown-format, powershell-format,
  rate-limit-guard, repo-hygiene, ruff-format, session-flow, and skill-quality, plus the setup evals
  of actionlint, bash-format, and repo-hygiene that asserted the wrong reason. The advice is
  unchanged; only the reason is.

Verification

• Sandbox probe: CLAUDE_CONFIG_DIR, HOME, USERPROFILE, APPDATA, LOCALAPPDATA under the
  session scratchpad, a local directory marketplace, and a fixture git repo. Positive control before
  any write: fixture claude plugin list --json returned []. After both batches, hashes of the real
  ~/.claude/settings.json, installed_plugins.json, and known_marketplaces.json were unchanged.
• parse-branch-issue.test.sh: 36 passed, 0 failed, 36 test calls. shellcheck clean on the script
  and test.
• sync-plugin-options-docs.py --check, check-changelog-parity.sh --check-bump 039578b98, and
  markdownlint on touched markdown: pass. No U+2014 in added lines. The three touched evals.json
  load and pass check-evals-quality.sh with 0 warnings.
• Fresh-context verifier, rationale withheld: all eight acceptance criteria and the item's four
  acceptance bullets PASS, with no CLI overclaim against the probe record.
• check-changed-skills.sh: its one failure is push-branch.test.sh, a file this PR does not touch.
  It fails because this host's global gpg signing applies to the test's commits, and it passes 21/0
  with GIT_CONFIG_GLOBAL=/dev/null.
• skill-evidence block absent: the installed source-control and claude-ops predate feat(source-control): seat the mandatory reviews on the operator's session and retire the OAuth review lanes #4210, so no ledger sha is stamped; the owed reviews ran as nested agents: plan reviewer, code reviewer, security reviewer, verifier.

Related

• Follow-up: source-control: migrate ten babysit repository-tooling keys from userConfig to the source-control.md cascade #4572 (the ten migrate-later keys; mandatory security review there).
• This PR is an unattended interview. Open questions and answers:
  • Q1, migrate or declare the babysit keys: the lane default was migrate. A fresh-context opus
    validator, with the rationale withheld, returned HYBRID, and that verdict was adopted (ADR 0039).
  • Q2, the fleet-wide caveat-2 copies: fixed here in every plugin except seven owned by another
    lane (see deferred below).
  • Q3, adopting the upstream options field: deferred as a plugin-philosophy gate decision, because
    declaring it raises the load floor to v2.1.271.
• USER-RESERVED: any design that lets a repo-writable layer set an authority-bearing babysit key, or
  replace a hold list under plain per-key override, is security-policy-loosening and is left out.
• Deferred to another lane: the wrong caveat-2 rationale remains in the setup skills of planning,
  guardrails, and claude-ops, which belong to a different session.
• Not re-measured: a sensitive option, and a same-scope string rerun at local scope.
• Security note: branch_issue_pattern moving to a repo-writable layer lets a repository choose which
  open issue Closes # names. The value carries no authority, and the output is now digits-only.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-27T15:59:49.405768Z	a486b3a	Draft marked ready

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Security review in progress

• Gather context (PR diff, changed files)
• Review security-relevant surface (parse-branch-issue.sh hardening, config-resolution/ADR cascade change, plugin install --config docs)
• Adversarially validate any candidate findings
• Post inline comments for anchored findings (if any)
• Final summary

View job run

[claude]

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

---

Code review complete

• Gather context (PR body, diff, CLAUDE.md/AGENTS.md, REVIEW.md)
• Run /review:code-review criteria against changed files
• Post inline findings (if any)
• Final summary

No blocking or high-signal findings. No inline comments were posted.

What I reviewed

This PR touches 87 files, but the substantive logic lives in a small set of them; the rest is a mechanical fleet-wide caveat-text correction (documented in the PR body) plus version/CHANGELOG bumps. I focused review effort accordingly:

• plugins/source-control/skills/pull-request/scripts/parse-branch-issue.sh (blob) — traced the full cascade-resolution path line by line: the ## branch_issue_pattern section parser (section_value, including fence handling and the "headings inside fences are ignored" rule), the ERE/backreference validity gate (usable_pattern), the layer-precedence loop (local > team > user-global), the deprecated-userConfig/placeholder fallback, and the last-capture-group → numeric-id extraction. I did not find a correctness bug; edge cases I specifically checked (non-participating optional capture groups, duplicate headings inside an earlier fence, a syntactically-valid layer pattern that simply doesn't match the branch, ${2:-} vs the env-var fallback ordering) all resolve the way the script's own header comment and reference/config-resolution.md describe.
• parse-branch-issue.test.sh — read against the script; the 36 cases (cascade precedence, fenced/unterminated/empty fences, backreference and invalid-ERE rejection, non-numeric capture, placeholder handling, stderr content/absence assertions) match the script's actual behavior by inspection. Note: I was not able to execute this test file or parse-branch-issue.sh myself (Bash tool execution required approval I couldn't obtain in this session), so the "36 passed" result in the PR body is author-claimed and unverified by me, not independently confirmed here.
• Docs cross-consistency: docs/adr/0039-...md, docs/conventions/hook-config-delivery/README.md (facts 9-12), docs/conventions/plugin-reconfiguration/README.md (verified-version record + 4 caveats), docs/migration-playbook.md, docs/plugin-philosophy.md, docs/extensibility-contract-smoke-tests.md, and plugins/source-control/reference/config-resolution.md. Checked the specific numeric/scope claims against each other (e.g. which option-type × scope combinations were actually measured vs. left as "not covered") — these agree everywhere I cross-checked.
• Fleet sweep spot-checks: diffed the caveat-2 rationale fix across actionlint, bugs, desktop-notification, knowledge, typos-format, and the three touched evals.json files — each correctly replaces the wrong "or the write lands at a scope that does not load" rationale with the measured one, worded independently per plugin as the convention doc permits.
• Version/CHANGELOG bumps: spot-checked plugin.json version vs. CHANGELOG.md top entry for source-control (0.60.0 → 0.61.0, Added/Changed/Deprecated sections match the shipped behavior), autonomy, and desktop-notification (os-toast description + platform naming applied consistently across plugin.json, README.md options table, and the compact table).

Verification claims in the PR body (sandbox probe hashes, check-changelog-parity.sh, markdownlint, the fresh-context verifier's PASS verdict, check-changed-skills.sh) are author-claimed; I did not re-run them.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a486b3acad

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

---

Security review complete

• Load /review:security-review skill instructions
• Gather PR diff (origin/main...HEAD) and read changed files
• Hunt for security-relevant issues in the babysit-key/config-cascade and parse-branch-issue.sh changes
• Adversarially reason through candidate findings (empirical execution was blocked by Bash-tool approval in this sandbox; see caveat below)
• Post inline comment for the confirmed-by-reasoning finding
• Final summary

Scope: Of the 87 changed files, the vast majority are version bumps, CHANGELOG entries, and doc/caveat-text corrections with no executable surface. The only new code with a security-relevant attack surface is plugins/source-control/skills/pull-request/scripts/parse-branch-issue.sh, which now resolves branch_issue_pattern from a three-layer, partially repo-writable config cascade (.claude/source-control.local.md > .claude/source-control.md > $HOME/.claude/source-control.md) and matches it against the branch name.

Finding (1)

IMPORTANT — regex-DoS (catastrophic backtracking) bypasses usable_pattern()'s validation
parse-branch-issue.sh#L104-L117 — posted inline.

usable_pattern() rejects a pattern only if it fails to compile or contains a literal \1-\9 backreference. It does not reject a syntactically valid ERE with nested unbounded quantifiers (e.g. ^([a-z]+-)+[0-9]+$). Bash's [[ =~ ]] uses glibc's backtracking POSIX regex engine, which is known to run exponentially long on such patterns even with no backreference present. Since .claude/source-control.md (the team layer, read at L123) is repo-writable, a merged pattern of this shape would hang [[ "$BRANCH" =~ $PATTERN ]] at L150 for any teammate whose branch name doesn't cleanly match — i.e. every future /pull-request create in that repo. The existing backreference check shows the authors already modeled "malicious repo-writable pattern" as a threat here; nested-quantifier ReDoS is the same threat class, just uncovered.

Suggested fixes (inline comment has details): wrap the match in timeout and treat a timeout like an invalid pattern (note + skip/fall through), add a heuristic rejecting nested repetition inside a capture group, or cap BRANCH length before matching.

Caveat on this finding: I could not run a live timing test in this environment — Bash tool calls beyond simple git read commands required interactive approval that wasn't available in this non-interactive CI session — so this is reasoned from the well-documented glibc-regex catastrophic-backtracking class, not empirically measured here. Worth a quick local repro (time bash -c '[[ "$(printf a%.0s {1..30})" =~ ^(a+)+$ ]]') before triage.

What I checked and ruled out

• No command/shell injection: the script never evals or interpolates untrusted values into a command line; PATTERN/BRANCH only ever flow into [[ =~ ]], and the captured issue id is validated ^[0-9]+$ before being echoed.
• The digits-only output constraint and the design decision to keep branch_issue_pattern "no authority" (per the PR's own Security note and ADR 0039) hold up — a hostile pattern can, at most, cause a wrong/missing Closes #N, not close an arbitrary issue with elevated trust, and not execute code.
• note() never echoes the raw pattern text into stderr (verified against the new test cases), so no prompt-injection-via-stderr vector into an agent transcript.
• Symlink-following in section_value()'s file reads doesn't cross a privilege boundary (same-user read access).
• The hook-config-delivery/plugin-reconfiguration doc corrections (facts 9-12: --config always writing to user settings regardless of -s) describe pre-existing upstream Claude Code CLI behavior being documented accurately, not new behavior introduced by this PR — no new finding there.

[claude]

Claude finished @kyle-sexton's task in 59s —— View job

---

Security review complete

• Load /review:security-review skill instructions
• Check skip gate (closed PR / no security surface / already-reviewed head)
• Gather PR diff (origin/main...HEAD at 6b86950) and read changed files
• Hunt for security-relevant issues
• Adversarially validate any candidate findings
• Post inline comments for anchored findings (if any)
• Final summary

Skip gate: criterion 3 (already-reviewed head, surface unchanged). The only file in this PR with executable security-relevant surface is
parse-branch-issue.sh,
which resolves branch_issue_pattern from a three-layer, partially repo-writable config cascade and matches it against the branch name. I diffed
a486b3a..6b86950 (the range since this PR's last completed security review) and confirmed that file is byte-identical across it —
git diff a486b3a..6b86950 -- plugins/source-control/skills/pull-request/scripts/parse-branch-issue.sh returns empty.

Everything in that range is documentation/skill-text only: .claude-plugin/CHANGELOG bumps, and updates to
plugins/source-control/skills/setup/SKILL.md
and
plugins/source-control/skills/setup/reference/apply-convention.md
documenting how apply branch_issue_pattern=<ERE> routes, plus the caveat-2 rationale/exit-0 wording fix repeated across the fleet's setup skills
(actionlint, ai-briefing, bash-format, biome-format, bugs, context-budget, disk-hygiene, education, eol-normalizer, go-format, knowledge,
machine-health, markdown-format, powershell-format, rate-limit-guard, repo-hygiene, ruff-format, session-flow, skill-quality). None of these
introduce new executable surface, new input handling, or a new trust boundary — they're prose describing existing (already-reviewed) script
behavior.

The prior security review on this PR (commit a486b3a) already covered parse-branch-issue.sh in full and raised one finding: a
regex-DoS-shaped gap in usable_pattern() (nested-quantifier EREs aren't rejected the way backreferences are). The PR author
responded with timing measurements showing no
exponential blowup on their host's POSIX regexec-backed =~ for the pattern shapes tested, and declined the code change. That finding and its
disposition stand unchanged; I found no new information in this range that bears on it, and no new candidate findings.

No new findings on this head. No inline comments were posted.

[claude]

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

---

Code review complete

• Gather context (PR diff, CLAUDE.md/AGENTS.md, REVIEW.md)
• Run /review:code-review criteria against changed files (security findings scoped out — claude-security-review.yml exists and already ran on this PR)
• Adversarially validate candidate findings
• Post inline comments for anchored findings (none survived — see below)
• Final summary

No blocking or high-signal findings. No inline comments were posted.

Scope

HEAD moved from a486b3a (already reviewed by this lane, no blocking findings) to 6b86950dcfc3bbbcc5bfaab651d8aea0e402d343. I diffed a486b3a...6b86950 to scope the incremental review — 41 files, none of them parse-branch-issue.sh, parse-branch-issue.test.sh, the ADR, config-resolution.md, or the hook-config-delivery/plugin-reconfiguration convention docs (all already covered in the prior review rounds and unchanged since). The new commit is purely a response to the three Codex P2 review comments on a486b3a:

1. "Pass the legacy pattern to the confirmation command" — plugins/source-control/skills/setup/SKILL.md#L68-L70 now passes '${user_config.branch_issue_pattern}' as the confirmation command's second argument. I traced this against parse-branch-issue.sh's actual placeholder handling (scripts/parse-branch-issue.sh#L136-L147): the script strips the literal placeholder text when unset and otherwise treats arg2 as the deprecated userConfig value, which matches what the doc now instructs the agent to substitute. Correctly closes the gap.
2. "Define branch-only apply routing" — reference/apply-convention.md#L76-L101 adds the branch_issue_pattern= alone and combined-with-subject_pattern= routes (validation, overlay-dedup rule reuse, write template's new ## branch_issue_pattern section, post-write confirmation). I checked this against the script's actual validation rules (ERE compile check, backreference rejection, capture-group requirement) — consistent.
3. "Tell all updated setup skills to inspect command output" — verified the A rejected value prints a warning yet exits 0, so read the output. sentence was added to all 19 plugins Codex named plus typos-format/desktop-notification/source-control (already fixed in the prior commit), and that the 3 evals.json files whose expected-output text asserted the old ("write lands at a scope that does not load") rationale were corrected to match (actionlint, bash-format, repo-hygiene). source-control's own evals.json has no eval touching this rationale, so it needed no change. Confirmed planning/guardrails/claude-ops still carry the stale rationale, matching the PR body's explicit "deferred to another lane" note — not a gap in this PR's own claims.

Spot-checked parse-branch-issue.test.sh's "heading inside an earlier fence is ignored" and "empty fence reported" cases by hand-tracing section_value()'s state machine line-by-line; both resolve the way the test expects.

Security-relevant surface (the ReDoS finding on usable_pattern()) is out of scope for this lane per REVIEW.md and was already reviewed/discussed on the security lane's own comment thread.