fix(source-control): close the merge wrapper's =value spelling bypass

[kyle-sexton]

Summary

plugins/source-control/bin/source-control-babysit-merge exists to add exactly one refusal on
top of babysit_merge.py: it rejects --allow-unpinned-head so that no unattended,
allow-rule-covered invocation can merge an unvetted head. The guard's own comment says it "must not
depend on the interpreter behind it" — but an =value spelling of the flag did exactly that.

Reproduced on origin/main before touching anything:

$ bash bin/source-control-babysit-merge owner/repo#1 --allow-unpinned-head
source-control-babysit-merge: --allow-unpinned-head is not permitted through the wrapper (--allow-unpinned-head or a prefix of it).
Invoke babysit_merge.py directly for interactive unpinned use.
RC=2                             # the WRAPPER refused

$ bash bin/source-control-babysit-merge owner/repo#1 --allow-unpinned-head=true
usage: babysit_merge.py ...
babysit_merge.py: error: argument --allow-unpinned-head: ignored explicit argument 'true'
RC=2                             # argparse refused — the wrapper let it through

Both exit 2, but for different reasons. The guard is [[ "$arg" == --a* && "--allow-unpinned-head" == "$arg"* ]] — a test that $arg is a prefix of the flag. --allow-unpinned-head=true is not a
prefix of --allow-unpinned-head (the =true tail breaks the comparison), so the wrapper's own
filter never fires; the CLI happens to reject it today only because the flag is argparse store_true, which refuses an explicit value. The refusal is real, but incidental — the moment the
guarded flag (or an equivalent guarded flag) accepts a value, this same test stops refusing
anything while looking identical, and nothing fails loudly to say so.

Fix

Strip a --flag=value tail (stem="${arg%%=*}") before the prefix comparison, so the guard tests
the option name rather than the raw argument. No exact spelling that previously refused changed
behavior — the stem of an already-refused argument is unchanged, and a stemmed sibling flag
(--allow-dependency, --allow-unprotected, --allowed-owners=<value>) still isn't a prefix of
--allow-unpinned-head, so it still reaches the CLI unmolested.

Test plan

• Red first. Wrote the check_wrapper_refusal rows in engine.test.sh and the two new
  guard_contract.py rows against unmodified origin/main and confirmed they fail: 3 bash rows
  FAIL (--allow-unpinned-head=true, --allow-unpinned=1, --allow-unpinned-hea=1 all reach
  argparse instead of the wrapper) and 2 Python test_every_refusal_row subtests FAIL the same
  way.
• Assertions check the wrapper's own refusal text, not exit 2. Exit 2 is overloaded
  between the wrapper's refusal and argparse's own usage/rejection errors on this path, so an
  exit-code-only assertion would have passed before and after this fix for different reasons
  (exactly the trap --allow-unpinned-head=true is). engine.test.sh gained a
  check_wrapper_refusal helper that greps stderr for the wrapper's refusal text;
  guard_contract.py's existing framework already asserts no JSON envelope was emitted (proof
  Python never ran) for bash-wrapper-attributed rows.
• New engine.test.sh rows: --allow-unpinned-head=true, --allow-unpinned=1,
  --allow-unpinned-hea=1 (all refused by the wrapper) plus no-over-refusal rows for
  --allow-dependency, --allow-unprotected, and --allowed-owners=owner (all still reach the
  fail-closed CLI, verified via an out-of-scope-owner exit 3 so no network call is needed).
• New guard_contract.py rows: merge.equals-value-unpinned-head-refused-by-wrapper and
  merge.equals-value-abbreviated-unpinned-head-refused-by-wrapper, each citing fix(source-control): babysit-merge wrapper's --allow-unpinned-head guard misses the =value spelling #1522. Also
  updated wrapper_denies() (used by the doc/parser cross-check
  test_every_wrapper_refusal_row_reaches_the_denial_table) to strip the same =value tail
  before its own prefix check, so the two new rows don't fail that self-consistency test for an
  unrelated reason. Regenerated reference/guard-contract.md via python tests/guard_contract.py --emit.
• Green after the fix: all engine.test.sh wrapper rows PASS; full unittest discover
  suite (442 tests, includes both new rows and the existing 5 unpinned-head-family rows) OK;
  ruff check clean; shellcheck -x on the wrapper clean.
• CHECK-SKILL babysit-prs: PASS via scripts/check-changed-skills.sh origin/main (trigger
  phrases preserved, engine.test.sh passes). Note: this machine's default python3/python on
  PATH resolve to a broken local interpreter install unrelated to this change (a dataclasses
  import failure); the gate was run with a working Python 3.11+ interpreter on PATH, and the
  identical failure reproduces on unmodified origin/main too, confirming it's pre-existing and
  environmental, not a regression.
• Repo gates run locally: scripts/check-changelog-parity.sh --check and --check-bump origin/main ✔; scripts/validate-plugins.sh ✔; markdownlint-cli2 on the touched docs clean.
• Version bumped 0.32.0 → 0.32.1 with the matching CHANGELOG entry.

Related

Closes #1522. Same failure family as #1371 (fixed by #1354, which made the guard prefix-aware) —
this closes the one spelling that fix's prefix comparison still missed. Found during independent
verification of #1405, per #1522's own description; not introduced by that PR.

---

This was generated by AI during work-loop execution.

🤖 Generated with Claude Code

[claude]

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

---

PR Review: fix(source-control): close the merge wrapper's =value spelling bypass

• Gather context from PR description and changed files list
• Read the changed files
• Analyze correctness of the fix
• Review test coverage
• Check security implications
• Post final review

---

Assessment: Approved

The fix is correct, minimal, and well-structured. The core change is exactly right, the testing methodology is sound, and all three layers (bash, Python contract, and generated doc) are consistent with each other.

---

Core fix — bin/source-control-babysit-merge

source-control-babysit-merge lines 36–44

for arg in "$@"; do
  stem="${arg%%=*}"
  if [[ "$stem" == --a* && "--allow-unpinned-head" == "$stem"* ]]; then

%%=* is the right choice. %% strips the longest suffix matching =* — so for --foo=bar=baz it removes =bar=baz, correctly leaving only the option name before the first =. Using %=* (shortest match) would also work for well-formed flags but %% is the principled choice.

The logic composes correctly with the prefix family. After stemming:

• --allow-unpinned-head=true → --allow-unpinned-head → matched ✓
• --allow-unpinned=1 → --allow-unpinned → matched as prefix of --allow-unpinned-head ✓
• --allow-dependency=val → --allow-dependency → --allow-unpinned-head doesn't start with this → passes through ✓
• --allowed-owners=owner → --allowed-owners → passes through ✓

Error message correctly echoes $arg (the original), not $stem. Users see exactly what they typed, not the stripped form. That's correct and good UX.

---

Test methodology — engine.test.sh

check_wrapper_refusal helper, lines 66–78

The decision to assert on the wrapper's own refusal text rather than just exit code 2 is the right call. The PR describes the exact trap: before this fix both the wrapper refusal and argparse's own store_true rejection produce exit 2, so an exit-code-only assertion would have passed on both sides of the fix for different reasons.

The redirect 2>&1 >/dev/null in check_wrapper_refusal is correct but non-obvious. The order matters: 2>&1 redirects stderr to where stdout currently points (the $() capture pipe), then >/dev/null redirects stdout to /dev/null. Net result: $() captures stderr, stdout is discarded. This is what's intended, and it works correctly.

The "no-over-refusal" assertions using exit 3 (owner-scope refusal from the fail-closed CLI) are a good approach — they verify sibling --allow-* flags aren't swallowed by the wrapper without requiring any network call.

---

Contract layer — tests/guard_contract.py

wrapper_denies(), lines 1108–1117

The updated Python function correctly mirrors the bash filter:

stem = flag.split("=", 1)[0]
return any(denied.startswith(stem) for denied in WRAPPER_DENIED_FLAGS[wrapper])

split("=", 1)[0] is the Python equivalent of %%=* (splits at first =). The denied.startswith(stem) check matches the bash "--allow-unpinned-head" == "$stem"* — both test whether the denied flag starts with the stem.

The two new Refusal rows are well-formed: they have distinct IDs, accurate claim prose that explains the why (not just the what), correct refused_by=BASH_WRAPPER attribution, and the right error_contains value (the original $arg form, matching what the error message echoes).

---

Minor observation (non-blocking)

The two new refusal rows (merge.equals-value-unpinned-head-refused-by-wrapper, merge.equals-value-abbreviated-unpinned-head-refused-by-wrapper) aren't cross-referenced in the EntryPoint.backed_by tuple for the merge wrapper (line 916). The existing rows already adequately back the entry-point claim, and backed_by appears to list representative rows rather than an exhaustive set — so this is a completeness note, not a defect.

---

Changelog / versioning

The 0.32.0 → 0.32.1 patch bump is appropriate for a bug fix with no API change. The CHANGELOG entry is accurate and thorough.

[claude]

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

---

Security Review

• Gather context and understand the request
• Read the PR diff and changed files
• Analyze the wrapper script fix for security issues
• Analyze the test scripts for injection surfaces
• Analyze guard_contract.py changes
• Post final security review findings

---

Findings

No security vulnerabilities found.

This PR closes a guard bypass rather than introducing one. Detailed analysis of each changed surface below.

---

bin/source-control-babysit-merge — the core fix

The two-line change:

stem="${arg%%=*}"
if [[ "$stem" == --a* && "--allow-unpinned-head" == "$stem"* ]]; then

• %%=* parameter expansion: purely a bash string operation, no subshell, no command execution, no injection surface.
• printf '%s\n' "...${arg}..." on line 39: uses %s as the format specifier with the expanded string as the positional argument — not a format string vulnerability. The rejected argument is reflected into stderr intentionally (it's a refusal message), and flags are not secrets.
• "$@" throughout: correctly double-quoted; no word-splitting on user arguments.
• set -euo pipefail on line 14 is preserved — fail-fast behavior unchanged.
• Guard direction: the fix makes the filter more restrictive (catches --flag=value spellings that previously escaped it), never less. No bypass is opened.
• Sibling-flag over-refusal: verified by the no-over-refusal rows in engine.test.sh — --allow-dependency, --allow-unprotected, and --allowed-owners=<value> all correctly reach the fail-closed CLI (exit 3 on an out-of-scope owner) rather than being refused by the wrapper.

scripts/engine.test.sh — check_wrapper_refusal helper

stderr="$(bash "$MERGE_WRAPPER" "$@" 2>&1 >/dev/null)"

The redirection order (2>&1 before >/dev/null) is correct: stderr is captured by the command substitution pipe, stdout is discarded. All arguments passed to this helper in the test file are static strings — no user-controlled interpolation, no injection surface.

The new assertion strategy (grep stderr for the wrapper's refusal text rather than checking only exit code) is a security strengthening, not a weakening: it distinguishes "wrapper refused" from "argparse coincidentally refused" — exactly the trap the PR description documents.

tests/guard_contract.py — wrapper_denies() update

stem = flag.split("=", 1)[0]
return any(denied.startswith(stem) for denied in WRAPPER_DENIED_FLAGS[wrapper])

The Python mirror of the bash %%=* logic (split("=", 1)[0]) is semantically identical and consistently applied. denied.startswith(stem) checks "does the denied flag name start with the supplied stem" — the same prefix direction as the bash "--allow-unpinned-head" == "$stem"* test. The two implementations are in agreement; no divergence that could let a crafted flag name pass the Python consistency check while being refused in bash (or vice versa).

---

Summary: the change is a clean, conservative tightening of an existing guard. No new execution paths, no new capability, no injection surfaces, no logic inversions. The test additions enforce the right observable (wrapper's own text, not exit code) and provide regression coverage for the three bypass spellings the old test missed.

[claude]

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

---

PR Review: fix(source-control): close the merge wrapper's =value spelling bypass

• Gather context from PR description and changed files list
• Read the changed files
• Analyze correctness of the fix
• Review test coverage
• Check security implications
• Post final review

---

Assessment: Approved

Minimal, correct fix with well-designed tests. The PR closes a real behavioral gap — the guard was depending on argparse for a case its header explicitly says it must not — and the test methodology is specifically designed to catch that class of incidental pass.

---

Core fix — bin/source-control-babysit-merge lines 36–44

stem="${arg%%=*}"
if [[ "$stem" == --a* && "--allow-unpinned-head" == "$stem"* ]]; then

%%=* is the right operator. %% removes the longest suffix matching =* — for --allow-unpinned-head=true that strips =true, leaving the bare option name. For an argument with no = the expansion is a no-op, so all previously-refused spellings are unchanged.

Verified all seven cases:

Argument	Stem	Refused?	Expected
--allow-unpinned-head=true	--allow-unpinned-head	✓	REFUSED
--allow-unpinned=1	--allow-unpinned	✓	REFUSED
--allow-unpinned-hea=1	--allow-unpinned-hea	✓	REFUSED
--allow-unpinned-head (no =)	unchanged	✓	REFUSED
--allow-dependency=val	--allow-dependency	✓	passes through
--allow-unprotected	unchanged	✓	passes through
--allowed-owners=someone	--allowed-owners	✓	passes through

Error message correctly echoes $arg (the original argument), not $stem. Users see exactly what they typed.

---

Test methodology — engine.test.sh lines 66–78

stderr="$(bash "$MERGE_WRAPPER" "$@" 2>&1 >/dev/null)"
got=$?
if [[ "$got" == "2" && "$stderr" == *"is not permitted through the wrapper"* ]]; then

Asserting on the wrapper's own refusal text rather than only the exit code is the right call — as the PR description explains, both the wrapper refusal and argparse's own store_true rejection produce exit 2, so an exit-code-only assertion would have "passed" on both sides of the fix for different reasons. The helper directly prevents that trap.

Redirect order 2>&1 >/dev/null is correct. The sequence: 2>&1 redirects stderr to where stdout currently points (the $() capture pipe), then >/dev/null discards stdout. Net effect: command substitution captures stderr; stdout is discarded. This is intentional and works correctly.

got=$? is correct. stderr="$(...)" (without local) is a plain assignment, which in bash does propagate the subshell's exit code to $?. local stderr got is on a separate line at the top of the function, so the assignment's $? is clean.

The no-over-refusal assertions using exit 3 (owner-scope refusal from the fail-closed CLI) are a clean approach — verifying sibling --allow* flags reach the CLI without any network call.

---

Contract consistency — guard_contract.py lines 1108–1117

stem = flag.split("=", 1)[0]
return any(denied.startswith(stem) for denied in WRAPPER_DENIED_FLAGS[wrapper])

split("=", 1)[0] is semantically identical to bash %%=* (both split on the first = and take the left side). The denied.startswith(stem) check mirrors "--allow-unpinned-head" == "$stem"*. The two implementations match exactly — no divergence that could produce a split result between the bash filter and the Python consistency check.

The two new Refusal rows are well-formed: distinct IDs, accurate claim prose explaining the mechanism, correct refused_by=BASH_WRAPPER attribution, and error_contains matching what the error message echoes (the original $arg form).

---

Second commit — comment hygiene

The follow-up commit (3652c73) rewrites a comment that referenced "PR #1382" to use a bare description instead, matching the (#1234) convention used elsewhere. Correct and appropriate.

---

Minor observation (non-blocking)

The two new Refusal rows aren't listed in the backed_by tuple for the merge entry-point (line ~916). The existing rows adequately back the entry-point claim and backed_by appears to list representative rather than exhaustive rows, so this is a completeness note rather than a defect.

---

Changelog / versioning

The 0.32.0 → 0.32.1 patch bump is appropriate for a bug fix with no API change. The CHANGELOG entry is accurate and covers the test methodology rationale as well as the fix itself.

[claude]

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

---

Security Review

• Gather context from PR description and changed files
• Read PR diff and changed files
• Analyze wrapper script for injection/bypass surfaces
• Analyze test scripts for injection surfaces
• Analyze guard_contract.py changes
• Post final findings

---

No security vulnerabilities found.

Detailed analysis of each changed surface below.

---

bin/source-control-babysit-merge — core fix

stem="${arg%%=*}"
if [[ "$stem" == --a* && "--allow-unpinned-head" == "$stem"* ]]; then

• %%=* is safe: pure bash parameter expansion — no subshell, no command execution, no injection surface. The double-% (longest suffix) is the principled choice: --foo=bar=baz correctly yields stem --foo.
• Guard direction is strictly tightening: --allow-unpinned-head=true, --allow-unpinned=1, and --allow-unpinned-hea=1 are now caught at the wrapper rather than falling through to argparse. No previously-refused spelling now passes.
• Sibling flags remain unaffected: --allow-dependency=val → stem --allow-dependency; --allow-unpinned-head does NOT start with --allow-dependency, so the condition is false and the flag reaches the CLI unmolested. Same for --allow-unprotected and --allowed-owners=val. The prefix direction of the test ("--allow-unpinned-head" == "$stem"*) means only stems that are prefixes of the denied flag are caught — not the other way around.
• No encoding bypass: a double-equals argument --allow-unpinned-head==true → %%=* strips from the first =, yielding stem --allow-unpinned-head → refused. A space-separated =value as a distinct argv element doesn't reach this loop as part of the same arg — and the bare flag without =value is already refused by the pre-existing logic.
• printf '%s\n' "...$arg..." on line 40: uses %s as format specifier with $arg as a positional argument, not interpolated into the format string — no format-string injection. The raw $arg echoed to stderr is intentional (users see exactly what they typed) and flags are not credentials.

scripts/engine.test.sh — check_wrapper_refusal helper

stderr="$(bash "$MERGE_WRAPPER" "$@" 2>&1 >/dev/null)"
got=$?

• Redirect order is correct: 2>&1 redirects stderr to where stdout currently points (the $() capture pipe); then >/dev/null redirects stdout to /dev/null. Net: $stderr captures the wrapper's stderr, stdout is discarded. $got correctly captures the wrapper exit code — $? after a command substitution assignment reflects the substituted command's exit status in bash.
• No injection: $MERGE_WRAPPER is set to the static literal "../../../bin/source-control-babysit-merge". All arguments to check_wrapper_refusal and check_exit at call sites are static strings — no user-controlled input is interpolated.
• Test strategy is security-relevant (positively): asserting on the wrapper's own refusal text (not just exit 2) closes the ambiguity the PR documents — that exit 2 from argparse and exit 2 from the wrapper are indistinguishable by code alone. A code-only assertion would have masked the bypass. The text check is the correct signal.

tests/guard_contract.py — wrapper_denies() update

stem = flag.split("=", 1)[0]
return any(denied.startswith(stem) for denied in WRAPPER_DENIED_FLAGS[wrapper])

split("=", 1)[0] is the semantic equivalent of bash %%=*. denied.startswith(stem) mirrors the bash "--allow-unpinned-head" == "$stem"* test. The two implementations are in agreement — no divergence that would allow a crafted flag to pass the Python self-consistency check while being refused in bash (or vice versa).

plugin.json, CHANGELOG.md, reference/guard-contract.md

Version bump only (0.32.0 → 0.32.1), changelog prose, and generated documentation. No security surface.