fix(source-control): externalize hardcoded bot logins + branch-to-issue grammar

[kyle-sexton]

Summary

Two independent posture fixes from umbrella #912, batched under one
source-control bump (0.16.2 → 0.16.3).

W1 — dependency-manager hold-merge login set is configurable. The babysit
merge gate held only the built-in dependabot/renovate product bots
(DEPENDENCY_MANAGER_LOGINS), so a non-dependabot/renovate dependency bot an
operator runs slipped the cross-tier hold. is_dependency_author now also
matches any login in the new babysit_extra_dependency_manager_logins
userConfig, threaded as the --extra-dependency-manager-logins merge-wrapper
flag (mirroring the existing --approver-bot-logins arg-threading through
evaluate()). Logins normalize identically on both sides (casefold, strip
app/ and [bot]). Ships empty → unconfigured installs match the built-in
set alone. Wired only to the merge gate (the single is_dependency_author
call site), not the snapshot.

W2 — branch-to-issue grammar is configurable. parse-branch-issue.sh
hardcoded the <type>/<N>-<slug> (and routine-issue-<N>) convention, so a
repo on a different scheme (e.g. Jira keys feature/PROJ-123-slug) silently
failed to derive a Closes #N line. The script now accepts an ERE pattern
positional (last capture group = issue id), passed from the new
branch_issue_pattern userConfig at the /pull-request create call site; the
built-in convention stays the default when unset (an unsubstituted
${user_config…} placeholder is treated as absent). Per the plugins-reference,
CLAUDE_PLUGIN_OPTION_* reaches hook processes only — not skill-invoked
scripts — so the value is passed as an arg rather than read from the
environment.

Testing

• is_dependency_author extra-login normalization cases (casefold, app/,
  [bot]; empty extra never widens the built-in set).
• An evaluate()-level integration test that flips the dependency hold via the
  config on the same PR — a pure-function test would pass even with broken
  wiring, so this exercises the CLI-arg-shaped frozenset → evaluate() param →
  line-715 hold path.
• Full babysit Python suite: 348 passed.
• parse-branch-issue.test.sh: 10 passed (incl. Jira-key custom pattern and
  the unsubstituted-placeholder fallback). shellcheck clean.

Docs

plugin.json userConfig (both keys), babysit SKILL.md config table,
reference/feedback.md, source-control README config table, and the
create.md call site.

Related

• Part of umbrella audit: guardrails + source-control hardening — bypass gaps, convention-enforcement SSOT, decoupling #912 (contract + design there — not diverged).
• Sibling merged this session: fix(guardrails): resolve source-control enablement across user-global scope #928 (guardrails 0.9.6), fix(guardrails): configurable hook-manager bypass + broader machine-path roots #932 (guardrails 0.9.7).

Closes #917

[claude]

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

---

PR Review

• Gather context (PR diff, changed files)
• Review babysit_classify.py — extra dependency-manager logins
• Review babysit_merge.py — CLI arg threading
• Review parse-branch-issue.sh — configurable ERE pattern
• Review tests (test_babysit_merge.py, test_babysit_feedback.py, parse-branch-issue.test.sh)
• Review plugin.json userConfig additions
• Review docs/SKILL.md/README/create.md changes
• Synthesize findings and post review

---

Summary

Solid posture work — both fixes are well-reasoned and the test suites are comprehensive. One gap in the skill-level prose instruction for the new merge flag, plus a few minor observations.

---

Finding: --extra-dependency-manager-logins is not wired in the merge command prose

Where the gap lives: SKILL.md lines 191–195 (merge readiness bullet) and reference/safety.md lines 237 and 328 (canonical merge command templates).

All optional-but-conditional base-path flags have an explicit conditional threading instruction in SKILL.md's merge readiness bullet:

• --method <merge-method> → "add … when configured"
• --self-logins @me,<self-logins> → "drop the trailing ,<self-logins> when that value is empty"

The new --extra-dependency-manager-logins has no equivalent instruction there. The config table at SKILL.md:305 names the flag correctly ((merge gate)), but the table is reference material, not the prose instruction an agent follows when assembling the command. Neither of safety.md's canonical forms is updated:

# safety.md line 328 (base merge handoff — unchanged)
bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" owner/repo#42 \
  --allowed-owners <watched-owners> --merge --expected-head <post-push-head-sha> \
  --method <merge-method>
  # ^ --extra-dependency-manager-logins is absent here and on line 237 (autopilot form)

Without an explicit conditional like "add --extra-dependency-manager-logins <value> when babysit_extra_dependency_manager_logins is set and not an unsubstituted placeholder", an agent constructing the merge command from the prose will omit the flag and the hold won't fire for the configured logins — which is precisely the bug this PR is fixing.

Fix this →

---

Minor observations (no action required)

normalize_dependency_login_set called on every is_dependency_author invocation. When extra is non-empty, a new frozenset is re-created each call at:

# babysit_classify.py:272
return normalized in normalize_dependency_login_set(extra)

The merge gate calls this once per PR, so there's no real cost. But if it ever moves to a hot path (e.g., snapshot classification), pre-normalizing the set at the call site in main() (alongside frozenset(parse_csv_set(...))) and passing it pre-normalized would be cleaner. Not worth changing now.

parse-branch-issue.sh: zero-capture-group custom pattern returns the full match silently. If someone passes a pattern with no capture groups, ${BASH_REMATCH[n-1]} falls back to BASH_REMATCH[0] (the whole match). The docs say "last capture group" so this is a user misconfiguration, but a brief stderr warning when n==1 after a non-default pattern would help debugging. Very low priority.

plugin.json: branch_issue_pattern is placed in the numbers/configs block. Looking at the diff, branch_issue_pattern lands after fetch_logs_max_bytes (a number-typed numeric config). The field is type: string, so it should ideally be grouped near the other string configs (like babysit_review_gate_context) rather than trailing the numeric block. Does not affect behavior.

---

Correctness checks that passed

• is_dependency_author normalization (casefold + removeprefix("app/") + removesuffix("[bot]")) is symmetric between the built-in check and the extra-login normalization path.
• if not extra: return False short-circuit is correct — empty frozenset() is falsy in Python, so an unconfigured caller never reaches normalize_dependency_login_set.
• parse_csv_set(None) returns set() (handled by if not raw: return set()), so frozenset(parse_csv_set(None)) == frozenset() — safe default.
• BASH_REMATCH[n-1] last-capture-group logic is correct for both the default two-group pattern (optional prefix + number) and single-group custom patterns.
• Unsubstituted ${user_config…} placeholder detection (*'${user_config'*) in the shell script is reliable and correctly falls back to the built-in default.
• Integration test DependencyHoldIntegrationTests exercises the full evaluate() wiring path, not just the unit function — good call given the PR description's rationale.
• test_extra_login_config_form_is_normalized covers the case where the operator's configured value itself carries app/ or [bot], which is a non-obvious normalization edge case.

[claude]

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

---

Security Review — PR #950

• Gather PR diff and read changed files
• Review parse-branch-issue.sh for injection surfaces
• Review babysit_classify.py / babysit_merge.py for logic and authorization gaps
• Review plugin.json userConfig changes for secrets/credential exposure
• Synthesize findings and post review

---

No critical or important security vulnerabilities found. One suggestion below.

---

W2 — parse-branch-issue.sh

Shell injection — not present. The user-controlled $PATTERN is expanded inside [[ "$BRANCH" =~ $PATTERN ]]. In bash's [[ context the right-hand side of =~ is subject to parameter expansion but NOT to word-splitting, globbing, or command substitution — the entire expanded value is used as a POSIX ERE and never executed as shell code. The unquoted form is intentional and required for regex interpretation; quoting it would force a literal-string match. No injection surface.

CLAUDE_PLUGIN_OPTION_BRANCH_ISSUE_PATTERN env-var path — low risk. The script reads this env var as a secondary fallback. Since it's only available in hook processes (not skill-invoked scripts, as the PR body correctly notes), the realistic path is the CLI arg. If the env var were present in a skill context it would be used, but with the same ERE-not-executed semantics.

SUGGESTION — Confidence: HIGH: Pattern with zero capture groups outputs full match silently

n=${#BASH_REMATCH[@]}
echo "${BASH_REMATCH[n - 1]}"

When $PATTERN matches but has no capture groups, BASH_REMATCH contains only element 0 (the full match), so n=1 and BASH_REMATCH[n-1] = BASH_REMATCH[0] = the entire matched substring of the branch name. A misconfigured pattern like ^[a-z]+/ (no parens) exits 0 and emits the whole branch prefix, which would produce a Closes #feat/ line in the PR body. The script currently has no validation that the pattern contains at least one capture group. Not a security issue — the output is used only for a PR body annotation, not for access control — but worth hardening against operator misconfiguration. A guard like [[ $n -gt 1 ]] || exit 1 after the match would make zero-capture patterns fail fast.

Fix this →

---

W1 — babysit_classify.py / babysit_merge.py

Authorization direction is correct. The extra login set is strictly additive — it can only hold more PRs, never fewer. Built-in dependency managers (dependabot, renovate) are checked first via DEPENDENCY_MANAGER_LOGINS before consulting extra, so no value of extra can bypass a hold on the built-in set.

Login normalization is symmetric and consistent. Both the incoming login and the configured extra entries are normalized identically: casefold() → removeprefix("app/") → removesuffix("[bot]"). The filter if str(login).strip() on the config side correctly drops blank entries before normalization (though a config entry of "app/[bot]" would normalize to "" and enter the frozenset — see note below).

Edge case, not a vulnerability: An operator who configures app/[bot] or similar stripped-to-empty entry would insert "" into the frozenset. This would match any PR whose author_login is empty/null (which GitHub never returns for real PRs). No real-world impact; the protection is only ever tightened, not loosened.

parse_csv_set is safe. The function splits on commas and strips whitespace tokens — no shell execution, no injection surface. Feeding it None or empty string correctly yields an empty set.

---

plugin.json userConfig additions

Both new keys (babysit_extra_dependency_manager_logins, branch_issue_pattern) are "type": "string" scalars with no secret: true — appropriate since neither holds a credential. No new egress surface or trust-delegation is introduced.

---

Summary: The two changes are security-neutral-to-positive: W1 tightens the merge hold surface (additive only, correctly wired), and W2 is a pure pattern-routing change with no code-execution risk. The one suggestion is a defensive hardening against operator misconfiguration rather than an exploitable vulnerability.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b474ceec9

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

---

PR Review — #950

• Gather PR diff and context
• Review SKILL.md / safety.md — verify --extra-dependency-manager-logins prose wiring
• Review parse-branch-issue.sh — Jira pattern numeric capture / zero-capture guard
• Review all changed files for correctness and completeness
• Synthesize findings and post review

---

Summary

The Python wiring (W1) and shell script logic (W2) are both correct. However one blocker from the previous review remains open, and the Jira-key documentation introduces a new concern about misleading operator guidance.

---

Blocker — --extra-dependency-manager-logins is still not wired in merge command prose

This was the primary finding in the previous review. It remains unaddressed.

SKILL.md lines 191–195 (merge readiness bullet) — unchanged by this PR:

- **Merge readiness** — `source-control-babysit-merge owner/repo#N --allowed-owners
  <watched-owners> --self-logins @me,<self-logins>` (read-only; add `--merge --expected-head
  <vetted-head-sha>` to merge, and `--method <merge-method>` when configured).

Other optional flags have explicit conditional instructions: --method <merge-method> has "when configured", --self-logins has "drop the trailing ,<self-logins> when that value is empty". --extra-dependency-manager-logins has neither — an agent assembling the merge command from this prose will omit the flag and the extra-login hold will never fire.

safety.md line 328 (Pinned-Command Degradation handoff) — also unchanged:

bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" owner/repo#42 \
  --allowed-owners <watched-owners> --merge --expected-head <post-push-head-sha> \
  --method <merge-method>

safety.md line 237 (autopilot tier command) — also unchanged.

The config table at SKILL.md:304 names the flag correctly as reference material, but the prose instruction is what agents follow when constructing the command. Without a conditional note like "add --extra-dependency-manager-logins <value> when babysit_extra_dependency_manager_logins is set and not an unsubstituted placeholder", the hold is silently inactive for any configured extra logins.

Fix this →

---

New concern — Jira-key documentation overpromises end-to-end integration

plugin.json (line 168), README.md (line 176), and parse-branch-issue.test.sh (line 43) all frame the feature as enabling "Jira key" branch schemes like feature/PROJ-123-slug with a pattern ^[a-z]+/([A-Z]+-[0-9]+)-. The test confirms this captures PROJ-123.

But tracing the full path in create.md §2.4.0:

ISSUE_NUM=$(bash ... parse-branch-issue.sh "" "${user_config.branch_issue_pattern}")
# ISSUE_NUM="PROJ-123" with Jira custom pattern

ISSUE_STATE=$(gh issue view "$ISSUE_NUM" --json state --jq '.state' 2>/dev/null || true)
# gh issue view "PROJ-123" → fails → ISSUE_STATE=""

if [[ "$ISSUE_STATE" == "OPEN" ]]; then   # false
  CLOSES_LINE="Closes #${ISSUE_NUM}"
else
  echo "⚠ Branch suggests Closes #PROJ-123, but that issue is missing or not open…"
  ISSUE_NUM=""  # falls through to orphan-PR prompt
fi

A Jira-key branch with the custom pattern configured produces a misleading warning and then the orphan-PR prompt — identical behavior to an unmatched branch, but with an extra confusing error. The feature only helps repos using a non-standard format for an actual numeric GitHub issue number (e.g. issue-42/slug needing ^issue-([0-9]+)/). The PROJ-123 Jira-key example never produces a Closes #N line regardless of the pattern.

Recommended fix: replace the Jira-key example with a numeric-GitHub-issue-number example (e.g. ^issue-([0-9]+)/) in plugin.json, README.md, and the test. The test for PROJ-123 can stay as a unit test of the shell script itself, but the operator-facing docs should not imply this enables Jira Closes linkage.

Fix this →

---

Minor — zero-capture-group guard still absent

Previously noted by both the prior Claude review and the Codex security review. Still not addressed. When $PATTERN matches but has zero capture groups, n=1 and BASH_REMATCH[n-1] = BASH_REMATCH[0] = the full match, silently emitting a non-issue-number string. A guard at parse-branch-issue.sh:41:

n=${#BASH_REMATCH[@]}
[[ $n -gt 1 ]] || { echo "pattern matched but has no capture groups" >&2; exit 1; }
echo "${BASH_REMATCH[n - 1]}"

This only triggers on operator misconfiguration so low priority, but shipping the guard alongside the feature would close the gap cleanly.

---

Correctness checks that passed

• is_dependency_author extra-login path in babysit_classify.py:259-273 correctly short-circuits on empty extra (if not extra: return False), so unconfigured installs never reach normalize_dependency_login_set.
• babysit_merge.py:1019 frozenset(parse_csv_set(None)) == frozenset() — safe default when flag is absent.
• CLI arg --extra-dependency-manager-logins at babysit_merge.py:842 → parse_csv_set → frozenset → evaluate(extra_dependency_manager_logins=...) at line 1030 — wiring is correct in code.
• Normalization (casefold, removeprefix("app/"), removesuffix("[bot]")) is symmetric between the incoming login check and the extra-login set normalization path.
• parse-branch-issue.sh: unsubstituted ${user_config…} placeholder detection (*'${user_config'*) is correct; default pattern falls back properly.
• DependencyHoldIntegrationTests exercises the full evaluate() wiring path, not just is_dependency_author in isolation — good call per the PR description.
• plugin.json new keys are type: string, no secret: true — correct for non-credential values.
• branch_issue_pattern in plugin.json lands after fetch_logs_max_bytes (a numeric config). Grouping is cosmetic and does not affect behavior, but string configs are conventionally grouped together.

---

Bottom line: The Python/shell implementation is sound. Two doc gaps need resolution before this safely ships: the prose wiring gap (blocker — the feature is functionally inert without it for agents using the prose) and the Jira-key documentation concern (overpromises end-to-end integration that doesn't exist).

[claude]

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

---

Security Review — PR #950

• Read PR diff and identify changed files
• Review parse-branch-issue.sh — injection surfaces, pattern handling
• Review babysit_classify.py — authorization logic, login normalization
• Review babysit_merge.py — CLI arg threading, trust boundaries
• Review plugin.json userConfig additions — credential/secret exposure
• Review remaining changed files (tests, docs, CHANGELOG)
• Synthesize findings and post review

---

No critical or important security vulnerabilities found. One suggestion carries over from the prior automated review; I independently verified it and the remaining security surface. Detailed findings below.

---

W2 — parse-branch-issue.sh

Shell injection — not present. $PATTERN is unquoted on the right-hand side of =~ intentionally and correctly. In bash's [[ context the ERE operand undergoes parameter expansion only — no word-splitting, globbing, or command substitution — so an operator-controlled pattern value cannot execute shell code, regardless of its contents (including ;, $(...), or backticks). Quoting would defeat the regex interpretation.

Confidence: HIGH — verified against bash ERE semantics for [[...=~...]].

---

SUGGESTION — Confidence: HIGH: Zero-capture-group custom pattern silently emits the full match

n=${#BASH_REMATCH[@]}
echo "${BASH_REMATCH[n - 1]}"

When a custom $PATTERN has no capture groups, BASH_REMATCH holds only element [0] (the full match). n=1, so BASH_REMATCH[n-1] = BASH_REMATCH[0] = the entire matched substring of the branch name. A pattern like ^[a-z]+/([A-Z]+)- with a typo dropping the parens exits 0 and emits the branch prefix (e.g. feature/) as the issue id. The downstream gh issue view "$ISSUE_NUM" call (double-quoted, so no further injection) then fails or resolves a wrong issue — not a security issue, but a misconfiguration that exits cleanly and produces a misleading Closes #feature/ line.

A guard after the match block would make zero-capture patterns fail fast:

[[ $n -gt 1 ]] || { echo "parse-branch-issue: pattern has no capture groups" >&2; exit 1; }

Fix this →

---

CLAUDE_PLUGIN_OPTION_BRANCH_ISSUE_PATTERN env-var fallback — minor inconsistency, not a vulnerability. The $2 arg path filters unsubstituted ${user_config…} placeholders at line 33, but the env-var fallback at line 34 has no equivalent guard. If CLAUDE_PLUGIN_OPTION_BRANCH_ISSUE_PATTERN were somehow set to the literal string ${user_config.branch_issue_pattern}, it would be used as the ERE. That pattern matches a branch literally containing {user_config.branch_issue_pattern} — no real branch does, so the script exits 1 safely. The PR body notes this env-var path is hook-process-only, making it a near-zero-probability path in practice. No exploitable surface.

---

W1 — babysit_classify.py / babysit_merge.py

Authorization direction is correct and additive-only. is_dependency_author tests the built-in DEPENDENCY_MANAGER_LOGINS first, then the operator-supplied extra set. No value of extra can remove dependabot/renovate from the held set — the merge hold surface is DEPENDENCY_MANAGER_LOGINS ∪ extra, never a subset of it. An operator who does not configure extra sees exactly the prior behavior (built-in set only). Confidence: HIGH.

Login normalization is symmetric. The incoming author_login and every configured entry in extra traverse the same pipeline (casefold() → removeprefix("app/") → removesuffix("[bot]")), so app/MyDepBot[bot] in config matches MyDepBot[bot] from GitHub. The parse_csv_set strip step ensures entries are already whitespace-clean before reaching normalize_dependency_login_set, so the per-entry str(login).strip() guard there is redundant but harmless.

Edge case, not a vulnerability: a configured entry that strips entirely to "" (e.g. app/[bot]) would enter the frozenset as the empty string and could theoretically match a PR whose author_login is null/empty. GitHub never returns an empty author_login for real PRs; the practical impact is nil and the hold is only ever tightened, never loosened.

parse_csv_set(None) is safe. The if not raw: return set() short-circuit correctly produces an empty frozenset when --extra-dependency-manager-logins is absent, which the if not extra: return False guard in is_dependency_author correctly converts to built-in-set-only behavior.

---

plugin.json userConfig additions

Both new keys (babysit_extra_dependency_manager_logins, branch_issue_pattern) are "type": "string" scalars without secret: true — appropriate since neither holds a credential or token. No new egress surface or trust-delegation is introduced.

---

GitHub Actions

No workflow files were changed in this PR. No supply-chain or Actions security surface to audit.

---

Summary: The two changes are security-neutral to positive. W1 tightens the merge hold surface (strictly additive, correctly wired, symmetric normalization). W2 is a pure ERE-routing change with no code-execution risk. The one actionable suggestion — adding a zero-capture-group guard in parse-branch-issue.sh — is a defensive hardening against operator misconfiguration, not an exploitable vulnerability.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aee2ed9796

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

---

PR Review — #950

• Read changed files and diff
• Check if prior review blockers are addressed (SKILL.md/safety.md prose wiring, Jira-key docs, Codex bad-substitution)
• Review W1 — babysit_classify.py / babysit_merge.py
• Review W2 — parse-branch-issue.sh / create.md
• Review tests, plugin.json, docs
• Synthesize findings and post review

---

Summary

All three blockers/concerns raised by previous Claude and Codex reviews are now addressed. The implementation is correct and the documentation is accurate. One minor hardening (zero-capture-group guard) remains absent but is low-priority — the downstream handles it gracefully.

---

Previous blockers — resolved

1. --extra-dependency-manager-logins prose wiring (prior blocker)

Both locations are now updated:

SKILL.md:202–205 now carries the explicit conditional note:

"held set: built-in dependabot/renovate plus any babysit_extra_dependency_manager_logins, which you MUST append via --extra-dependency-manager-logins "<value>" when set — see safety.md's merge command forms — or those extra bots are silently not held"

safety.md:197–200 provides the authoritative instruction:

"supply it on every merge command below, exactly as --method is, or those extra bots are not held"

Both canonical command forms in safety.md are updated:

• Line 241 — autopilot enabled-path: includes --extra-dependency-manager-logins <extra-dependency-manager-logins>
• Line 334 — Pinned-Command Degradation base form: includes --extra-dependency-manager-logins <extra-dependency-manager-logins>

2. Jira-key documentation overpromise (prior new concern)

All operator-facing docs now use numeric GitHub issue number examples:

• plugin.json:169: '^[^/]+/([0-9]+)-' (alice/1234-slug) and '-([0-9]+)$' (feat/add-widget-1234)
• README.md:176: same examples
• create.md:136: explicitly states "a non-numeric capture — e.g. a bare Jira key — is looked up below, found absent, and dropped to the no-closure path"
• parse-branch-issue.test.sh:45–46: test cases now use alice/1234-fix and feat/add-widget-1234 (numeric-only)

3. Bash bad substitution for dotted ${user_config.branch_issue_pattern} (Codex concern)

create.md:139 now uses single-quoting:

ISSUE_NUM=$(bash "..." parse-branch-issue.sh "" '${user_config.branch_issue_pattern}' ...)

And create.md:136 documents the mechanic explicitly: single quotes prevent Bash's own bad substitution on the dotted name; Claude Code substitutes the value before Bash sees the line; when unset, the literal string reaches parse-branch-issue.sh where the *'${user_config'* guard at line 37 detects and clears it.

---

Remaining minor — zero-capture-group guard still absent

parse-branch-issue.sh:45–46 still has no guard for n==1:

n=${#BASH_REMATCH[@]}
echo "${BASH_REMATCH[n - 1]}"

A zero-capture-group pattern silently emits the full match. However, the downstream gh issue view call fails on the non-numeric result → ISSUE_STATE="" → falls to the orphan-PR prompt — so the user experience degrades gracefully, not catastrophically. Low priority; a [[ $n -gt 1 ]] || { echo "..." >&2; exit 1; } guard after line 45 would make it fail fast on misconfiguration, but this doesn't block merge.

Fix this →

---

Correctness checks

• is_dependency_author wiring (babysit_classify.py:259–273): short-circuits on if not extra: return False — empty frozenset from an unconfigured install never reaches normalize_dependency_login_set. Correct.
• Normalization symmetry: incoming login and configured extra entries traverse identical casefold() → removeprefix("app/") → removesuffix("[bot]") — no asymmetric match edge.
• evaluate() wiring (babysit_merge.py:1019–1033): frozenset(parse_csv_set(args.extra_dependency_manager_logins)) → evaluate(extra_dependency_manager_logins=...) — CLI arg → evaluate path is complete.
• parse_csv_set(None) returns set() (via if not raw: return set()), so frozenset(parse_csv_set(None)) == frozenset() — safe default when flag is absent.
• BASH_REMATCH last-group logic: BASH_REMATCH[n-1] is correct for both the default two-group pattern (optional routine-issue- prefix + number) and custom single-group patterns.
• Unsubstituted placeholder detection (*'${user_config'* at parse-branch-issue.sh:37): correct — fires on any surviving placeholder literal, falls back to built-in default.
• plugin.json new keys: both babysit_extra_dependency_manager_logins and branch_issue_pattern are "type": "string" with no secret: true — correct for non-credential values.
• DependencyHoldIntegrationTests in test_babysit_merge.py: exercises the full evaluate() wiring path rather than just is_dependency_author in isolation — right call given the PR description's rationale.

---

Bottom line: All material concerns from prior reviews are resolved. The implementation is sound, the docs are now accurate, and the test coverage is solid. Ready to merge once the author is satisfied — the zero-capture-group guard is a minor follow-up, not a blocker.

[claude]

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

---

Security Review — PR #950

• Get PR diff and identify changed files
• Review parse-branch-issue.sh — shell injection, pattern handling, env-var surfaces
• Review babysit_classify.py — authorization logic, login normalization
• Review babysit_merge.py — CLI arg threading, trust boundaries
• Review plugin.json — userConfig credential/secret exposure
• Review create.md — arg passing, placeholder expansion
• Synthesize findings and post review

---

No critical or important security vulnerabilities found. One suggestion carries over from prior reviews; independently re-verified below against the current HEAD (28024479).

---

W2 — parse-branch-issue.sh

Shell injection — not present. Confidence: HIGH.

$PATTERN appears unquoted on the right-hand side of =~ at line 41. In bash [[ ... =~ ... ]], the right-hand operand undergoes parameter expansion only — no word-splitting, globbing, or command substitution. A pattern containing ;, $(...), or backticks is interpreted as ERE syntax, not shell code. The unquoted form is intentional and required; quoting would force a literal-string comparison.

create.md single-quoted placeholder — correctly designed. Confidence: HIGH.

The skill template at create.md:140 passes '${user_config.branch_issue_pattern}' in single quotes. When the userConfig key is set, the plugin system substitutes the ERE before the agent runs the command; single quotes prevent subsequent bash expansion of ERE metacharacters (e.g., a $ end-anchor). When unset, the literal string ${user_config.branch_issue_pattern} reaches the script, which detects the ${user_config substring at line 37 and clears PATTERN, falling back to the built-in default. The bad substitution path noted in the Codex review is avoided because single-quoting prevents bash from attempting to expand a dotted variable name at all.

CLAUDE_PLUGIN_OPTION_BRANCH_ISSUE_PATTERN env-var path — minor inconsistency, not a vulnerability. Confidence: HIGH.

The $2 positional path clears the placeholder at line 37; the env-var fallback at line 38 has no equivalent guard. If set to the literal placeholder string, the ERE ${user_config.branch_issue_pattern} would be used: it matches only a branch literally containing that string (no real branch does), so the script exits 1 cleanly. Even with an attacker-controlled env-var, the [[...=~...]] semantics prevent code execution. Per the PR body, this path is hook-process-only in practice.

SUGGESTION — Confidence: HIGH: Zero-capture-group custom pattern silently emits full match as issue id

n=${#BASH_REMATCH[@]}
echo "${BASH_REMATCH[n - 1]}"

When a custom $PATTERN has no capture groups, BASH_REMATCH contains only element [0] (the full match). n=1, so BASH_REMATCH[n-1] = BASH_REMATCH[0] = the matched prefix of the branch name. The script exits 0 and the caller emits Closes #<full-match>. The downstream gh issue view "$ISSUE_NUM" is double-quoted (no further injection), so the worst case is a failed lookup and the orphan-PR prompt — not a security issue. A guard would make misconfigured patterns fail loudly:

[[ $n -gt 1 ]] || { echo "parse-branch-issue: pattern has no capture groups" >&2; exit 1; }

Flagged by all three prior automated reviews; not a blocker, low priority.

---

W1 — babysit_classify.py / babysit_merge.py

Authorization direction is strictly additive. Confidence: HIGH.

is_dependency_author at babysit_classify.py:259–273 tests DEPENDENCY_MANAGER_LOGINS first; no value of extra can remove dependabot or renovate from the held set. The if not extra: return False short-circuit at line 271 guarantees unconfigured callers see exactly the prior behavior. The held set is DEPENDENCY_MANAGER_LOGINS ∪ extra, never a subset of it.

Login normalization is symmetric. Confidence: HIGH.

Incoming author_login and every configured entry in extra traverse the same pipeline (casefold() → removeprefix("app/") → removesuffix("[bot]")), so app/MyDepBot[bot] in config matches MyDepBot[bot] from GitHub.

parse_csv_set(None) is safe. The if not raw: return set() guard at babysit_merge.py:133–135 produces an empty frozenset when --extra-dependency-manager-logins is absent; the if not extra: return False guard converts that to built-in-set-only behavior. Wiring at babysit_merge.py:1016–1030 is correct.

Edge case, not a vulnerability: a configured entry that normalizes to "" (e.g., app/[bot]) enters the frozenset as the empty string, matching a PR with an empty author_login. GitHub never returns empty author_login for real PRs; the hold surface is only ever tightened.

---

plugin.json userConfig additions

Both new keys (babysit_extra_dependency_manager_logins, branch_issue_pattern) are "type": "string" without secret: true — appropriate since neither holds a credential. No new egress surface or trust-delegation is introduced.

---

SKILL.md / safety.md prose additions

No new code-execution surface. The updated merge command templates in safety.md correctly add --extra-dependency-manager-logins <extra-dependency-manager-logins> to both the base and autopilot forms (lines 238 and 331), addressing the wiring gap flagged by prior reviews.

---

Summary: Both changes are security-neutral to positive. W1 tightens the merge hold surface (additive-only, correctly wired, symmetric normalization). W2 is a pure ERE-routing change with no code-execution risk. The one open suggestion — zero-capture-group guard in parse-branch-issue.sh — is a defensive hardening against operator misconfiguration, not an exploitable vulnerability.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2802447944

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

---

PR Review — #950

• Gather context (PR diff, changed files)
• Review parse-branch-issue.sh — zero-capture-group guard + final state
• Review SKILL.md / safety.md — merge command prose wiring
• Review babysit_classify.py / babysit_merge.py
• Review create.md — placeholder handling
• Review plugin.json, tests, docs
• Synthesize findings and post review

---

Summary

All prior blockers from previous review rounds are resolved. The implementation is correct, docs are accurate, and test coverage is solid. One minor defensive hardening (zero-capture-group guard) remains absent but degrades gracefully and doesn't block merge.

---

Prior blockers — confirmed resolved at HEAD 1b283ed

1. --extra-dependency-manager-logins prose wiring (blocker from reviews 1 & 2)

All locations updated:

• SKILL.md:202–207: explicit conditional note — "you MUST append via --extra-dependency-manager-logins "<value>" when set … or those extra bots are silently not held" ✓
• safety.md:197–200: authoritative instruction with "supply it on every merge command below, exactly as --method is" ✓
• safety.md:241 (autopilot enabled-path) and safety.md:334 (Pinned-Command Degradation): both include --extra-dependency-manager-logins <extra-dependency-manager-logins> ✓

2. Jira-key documentation overpromise (new concern from review 2)

All operator-facing docs now use numeric GitHub issue number examples:

• plugin.json:169: '^[^/]+/([0-9]+)-' and '-([0-9]+)$' ✓
• README.md:176: same numeric examples ✓
• create.md:148: explicitly states "a non-numeric capture — e.g. a bare Jira key — is looked up below, found absent, and dropped to the no-closure path" ✓
• parse-branch-issue.test.sh:45–46: alice/1234-fix and feat/add-widget-1234 (numeric-only) ✓

3. bad substitution / placeholder resolution (Codex concern)

create.md:142–148 correctly uses the <angle-bracket> slot form '<branch-issue-pattern>' — the agent fills this from the substituted value in SKILL.md's "Branch-to-issue grammar" line (SKILL.md:36), which is the substituted surface (${user_config.branch_issue_pattern} resolves there at load time, not in the raw reference file). The latest Codex concern ("configured value still passes the literal placeholder") is resolved by this design — reference files use <angle-bracket> slots, SKILL.md is the substituted surface ✓

---

Remaining minor — zero-capture-group guard still absent

parse-branch-issue.sh:45–46:

n=${#BASH_REMATCH[@]}
echo "${BASH_REMATCH[n - 1]}"

A zero-capture-group custom pattern silently emits the full match. However, the downstream gh issue view "$ISSUE_NUM" call on a non-numeric result fails → ISSUE_STATE="" → falls to the orphan-PR prompt — graceful degradation, not a silent bad outcome. Low priority; flagged by all prior reviews. A [[ $n -gt 1 ]] || { echo "parse-branch-issue: pattern has no capture groups" >&2; exit 1; } guard after line 45 would make operator misconfiguration fail loudly rather than silently.

Fix this →

---

Correctness checks

• is_dependency_author wiring (babysit_classify.py:259–273): if not extra: return False short-circuit — empty frozenset from unconfigured install never reaches normalize_dependency_login_set. Correct.
• Normalization symmetry: incoming login and configured extra entries both traverse casefold() → removeprefix("app/") → removesuffix("[bot]"). No asymmetric match edge.
• evaluate() wiring (babysit_merge.py:1019–1033): frozenset(parse_csv_set(args.extra_dependency_manager_logins)) → evaluate(extra_dependency_manager_logins=...) — wiring complete.
• parse_csv_set(None) returns set() (via if not raw: return set()), so frozenset(parse_csv_set(None)) == frozenset() — safe default when flag absent.
• BASH_REMATCH[n-1] last-group logic: correct for both the default two-group pattern and custom single-group patterns.
• Placeholder detection (*'${user_config'* at parse-branch-issue.sh:37): correct — fires on any surviving placeholder literal, falls back to built-in default.
• plugin.json new keys: babysit_extra_dependency_manager_logins and branch_issue_pattern are "type": "string" without secret: true — correct for non-credential values.
• DependencyHoldIntegrationTests in test_babysit_merge.py: exercises the full evaluate() wiring path rather than just is_dependency_author in isolation — right call per PR description.
• test_extra_login_config_form_is_normalized in test_babysit_feedback.py: covers the non-obvious case where the operator's configured value itself carries app/[bot].

---

Bottom line: All material concerns from prior reviews are resolved. The PR is ready to merge. The zero-capture-group guard is a follow-up quality-of-life hardening, not a correctness or security concern.

[claude]

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

---

Security Review — PR #950

• Gather PR diff and identify changed files
• Review parse-branch-issue.sh — injection surfaces, pattern handling
• Review babysit_classify.py / babysit_merge.py — authorization logic, login normalization
• Review plugin.json — userConfig credential/secret exposure
• Review create.md — arg passing, placeholder expansion
• Review remaining changed files (tests, docs, CHANGELOG)
• Synthesize findings and post review

---

No critical or important security vulnerabilities found. One low-priority suggestion carries over from all prior reviews. Full analysis below, independently verified at HEAD 1b283ed9.

---

W2 — parse-branch-issue.sh

Shell injection — not present. Confidence: HIGH.

$PATTERN is unquoted on the right-hand side of =~ at line 41 intentionally and correctly. In bash [[ ... =~ ... ]], the right-hand operand undergoes parameter expansion only — no word-splitting, globbing, or command substitution. A pattern containing ;, $(...), or backticks is interpreted as ERE syntax, not as shell code. Quoting would force a literal-string comparison and break the feature entirely.

create.md single-quoted placeholder — correctly designed. Confidence: HIGH.

The updated create.md line 145 instructs the agent to pass the resolved ERE value as a single-quoted positional ('<branch-issue-pattern>'). Single-quoting shields ERE metacharacters like the $ end-anchor from the shell. The note that this reference file is "Read raw" (so the substitution of ${user_config.branch_issue_pattern} happens in SKILL.md, not here) is correctly documented — the bad substitution concern from earlier Codex review is fully addressed in this approach.

CLAUDE_PLUGIN_OPTION_BRANCH_ISSUE_PATTERN env-var fallback — low risk. Confidence: HIGH.

The env-var path at line 38 has no unsubstituted-placeholder guard (unlike the $2 positional path at line 37). If CLAUDE_PLUGIN_OPTION_BRANCH_ISSUE_PATTERN were literally set to ${user_config.branch_issue_pattern}, it would be used as an ERE — and since [[...=~...]] does not execute code, no injection is possible. The script exits 1 safely (no real branch matches that string). Per the PR description, this env-var path is hook-process-only in practice, making it near-zero-probability.

SUGGESTION — Confidence: HIGH: Zero-capture-group custom pattern silently emits the full match

n=${#BASH_REMATCH[@]}
echo "${BASH_REMATCH[n - 1]}"

This was flagged by all three prior automated reviews and remains unaddressed. When a custom $PATTERN matches but has no capture groups, BASH_REMATCH contains only element [0] (the full match), so n=1 and BASH_REMATCH[n-1] = BASH_REMATCH[0] = the matched prefix of the branch name. The script exits 0, the caller passes the non-numeric string to gh issue view "$ISSUE_NUM" (double-quoted, so no further injection), the lookup fails, and the flow drops to the orphan-PR prompt — not a security issue, and the downstream degradation is graceful. But a guard would make misconfigured patterns fail fast:

[[ $n -gt 1 ]] || { echo "parse-branch-issue: pattern has no capture groups" >&2; exit 1; }

Fix this →

---

W1 — babysit_classify.py / babysit_merge.py

Authorization direction is strictly additive. Confidence: HIGH.

is_dependency_author checks DEPENDENCY_MANAGER_LOGINS first; no value of extra can remove dependabot or renovate from the held set. The if not extra: return False short-circuit guarantees unconfigured callers see exactly the prior behavior. The merge hold surface is DEPENDENCY_MANAGER_LOGINS ∪ extra, never a subset of it.

Login normalization is symmetric. Confidence: HIGH.

The incoming author_login at line 268 and every configured entry in normalize_dependency_login_set at line 252–253 traverse the identical pipeline (casefold() → removeprefix("app/") → removesuffix("[bot]")). An operator entry of app/MyDepBot[bot] correctly matches a GitHub API-delivered MyDepBot[bot] login.

parse_csv_set(None) is safe. Confidence: HIGH.

parse_csv_set at babysit_merge.py:133–136 returns set() when raw is None or empty. frozenset(parse_csv_set(None)) == frozenset(), which if not extra: return False in is_dependency_author converts to built-in-set-only behavior. The wiring at babysit_merge.py:1016–1030 is correct.

No subprocess injection surface. The --extra-dependency-manager-logins CLI arg value flows only through parse_csv_set (comma-split, string-strip) → frozenset → string comparison inside is_dependency_author. No subprocess calls, no shell execution.

Edge case, not a vulnerability: A configured entry that strips entirely to "" after normalization (e.g. "app/[bot]" → "") would enter the frozenset as the empty string, matching a PR whose author_login normalizes to "". GitHub never returns an empty author_login for real PRs; the merge hold surface is only ever tightened, never loosened. parse_csv_set's if part.strip() guard already drops whitespace-only tokens before they reach normalization, so the only path to an empty normalized entry is a valid-looking value that strips via removeprefix/removesuffix — an edge case with no real-world impact.

---

plugin.json userConfig additions

Both new keys (babysit_extra_dependency_manager_logins, branch_issue_pattern) are "type": "string" without secret: true — appropriate since neither holds a credential or token. No new egress surface or trust-delegation is introduced.

---

SKILL.md / safety.md prose additions

No new code-execution surface. The updated merge command templates in safety.md at line 241 (autopilot path) and line 334 (Pinned-Command Degradation) correctly add --extra-dependency-manager-logins <extra-dependency-manager-logins> to both canonical forms. The conditional note at line 197–200 correctly instructs agents to omit it when the value is empty or an unexpanded placeholder.

---

Summary: Both changes are security-neutral to positive. W1 tightens the merge hold surface (additive-only, correctly wired, symmetric normalization, no subprocess execution). W2 is a pure ERE-routing change with no code-execution risk — the [[...=~...]] context prevents shell injection regardless of the pattern value. The one open suggestion — zero-capture-group guard — is a defensive hardening against operator misconfiguration and has been flagged in every prior review; not a blocker.