Skip to content

fix(source-control): close the merge wrapper's argparse prefix-abbreviation bypass#1382

Closed
kyle-sexton wants to merge 3 commits into
mainfrom
fix/1371-babysit-merge-wrapper-abbrev-bypass
Closed

fix(source-control): close the merge wrapper's argparse prefix-abbreviation bypass#1382
kyle-sexton wants to merge 3 commits into
mainfrom
fix/1371-babysit-merge-wrapper-abbrev-bypass

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

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. That refusal was bypassable by dropping
one character.

The two halves disagreed. The wrapper filtered by exact string equality; the CLI it wraps built
its parser with argparse's allow_abbrev left at the default True
(CPython docs — allow_abbrev:
"Normally, when you pass an argument list to the parse_args() method of an ArgumentParser, it
recognizes abbreviations of long
options"). So any unambiguous prefix was a valid spelling of the flag to the CLI and an unrecognized
string to the wrapper.

Reproduced on origin/main before touching anything (RC=3 is the owner-allowlist refusal
downstream of parsing — proof the argument was consumed as a recognized option; RC=2 from the
wrapper would be the guard firing):

$ bash bin/source-control-babysit-merge owner/repo#1 --allow-unpinned-head   # guard fires
RC=2
$ bash bin/source-control-babysit-merge owner/repo#1 --allow-unpinned-hea    # guard does NOT fire
RC=3
$ bash bin/source-control-babysit-merge owner/repo#1 --allow-unpinned        # guard does NOT fire
RC=3
$ bash bin/source-control-babysit-merge owner/repo#1 --allow-unpi            # guard does NOT fire
RC=3

Both halves are fixed, because either alone leaves a gap

  1. bin/source-control-babysit-merge now refuses any ---prefixed argument that is a prefix
    of the flag, after stripping a --flag=value tail. It therefore holds independently of how the
    CLI's parser happens to be configured — a guard that is correct only because the thing it guards
    is configured a particular way is not a guard. The comparison is a length-sliced literal compare
    with the RHS quoted (never a glob match). It deliberately over-refuses a prefix appearing
    after a -- end-of-options separator or as another option's value: no legitimate invocation
    carries one, and a merge guard fails closed. Sibling flags are untouched — --allowed-owners,
    --allow-dependency, and --allow-unprotected are not prefixes of --allow-unpinned-head, and
    there are explicit no-over-refusal test rows for all three.

  2. All nine babysit-prs lane parsers now pass allow_abbrev=False (babysit_merge.py,
    babysit_resolve_thread.py, babysit_findings.py, manage_babysit_lease.py,
    manage_feedback_ledger.py, prune_babysit_worktrees.py, request_review.py,
    pr_queue_snapshot.py, refresh_pr_branch.py). This is the root cause, and it closes the
    class rather than the one flag.

The systemic finding worth stating outright

The same root cause defeats any control anchored on a flag's exact text, not just this wrapper's
refusal. Verified empirically on origin/main:

$ python babysit_merge.py owner/repo#1 --allowed-owners someone-else --mer
RC=3     # --mer was an accepted spelling of --merge (--method starts --met, so --mer is unambiguous)
$ ... --merg  -> RC=3     ... --allow-d -> RC=3   (an accepted spelling of --allow-dependency)

A consumer permission rule that grants the wrapper but not with --merge — which is how the
issue describes melodic-software/dotfiles' autoMode.allow, i.e. the configuration that bounded
the reported severity — was bypassable by the same mechanism. That exposure is closed by
allow_abbrev=False, not by the wrapper change; it is why this PR fixes the parser class rather
than the one flag.

Blast radius

No exact spelling changed behavior. A repo-wide scan of every .md/.sh/.py/.json/.yml line
mentioning these nine CLIs or the two wrappers found no invocation using an abbreviated flag
(two hits were prose lines naming multiple scripts, not invocations). SKILL.md and
reference/safety.md had published the refusal as unconditional; both now state that it covers
every spelling including every prefix, and that the CLI's parser is allow_abbrev=False.

Test plan

  • Red first. Wrote the assertions against unmodified origin/main and confirmed they fail:
    8 Python failures and 9 of the new bash rows failing (FAIL: merge wrapper rejects --allow-unpinned-hea (want exit 2 + wrapper refusal text, got exit 3: )).
  • The bash rows assert the wrapper's refusal text, not exit 2. Exit 2 is overloaded three
    ways on this path — the wrapper's refusal, argparse's usage/ambiguity errors, and
    babysit_merge.py's own usage errors — so a code-only assertion cannot tell a held guard from
    an accidental parse failure. --allow-unp is exactly that trap: it returns 2 on unfixed main
    via argparse ambiguity, so a code-only row would have been green before and after. New
    check_wrapper_refusal helper in engine.test.sh greps stderr for the refusal text.
  • The two halves are verified independently. Bash rows go through the wrapper (9 spellings:
    --allow-unpinned-hea, --allow-unpinned-h, --allow-unpinned, --allow-unpi,
    --allow-unp, --allow-u, --allow, plus --allow-unpinned-head=1 and --allow-unpi=1);
    Python rows in test_guards.py invoke babysit_merge.py directly and assert argparse's
    unrecognized arguments on stderr (the existing run() helper discarded stderr and parsed
    stdout as JSON — an argparse rejection writes nothing to stdout, so any assertion on the JSON
    payload would have tested nothing; added a run_raw() that returns stderr).
  • Anti-regression on the class, not the flag. test_every_lane_parser_disables_abbreviation
    AST-scans every ArgumentParser(...) under babysit-prs/scripts/ and fails if any lacks
    allow_abbrev=False, so a new lane script cannot silently reopen this.
  • No over-refusal. Rows assert --allow-dependency, --allow-unprotected, and
    --allowed-owners still reach the CLI (exit 3), and that the exact --merge --allow-unpinned-head --allow-dependency combination still parses to the owner-scope refusal
    rather than argparse's exit 2.
  • Green after the fix: engine.test.sh356 unittests OK, ruff clean, all 16 wrapper rows
    PASS, exit 0. Every plugins/source-control/**/*.test.sh (9 suites) passes.
  • Repo gates run locally: shellcheck -x on the wrapper and engine.test.sh clean;
    scripts/validate-plugins.sh ✔; scripts/check-changelog-parity.sh --check and
    --check-bump origin/main ✔; scripts/check-changed-skills.shCHECK-SKILL babysit-prs: PASS; markdownlint clean on the three touched docs.
  • Version bumped 0.26.40.26.5 with the matching CHANGELOG entry.

Related

Closes #1371.


This was generated by AI during work-loop execution.

🤖 Generated with Claude Code

…iation bypass

`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. It filtered by exact
string equality while the CLI behind it built its parser with argparse's
`allow_abbrev` left at the default `True`, so any unambiguous prefix
(`--allow-unpinned-hea`, `--allow-unpinned`, `--allow-unpi`) was a valid
spelling of the flag to the CLI and an unrecognized string to the wrapper.
Dropping one character therefore reached `--merge` without a pinned head --
precisely the outcome the wrapper's existence is justified by.

Fix both halves, because either alone leaves a gap:

- The wrapper now refuses any `--`-prefixed argument that is a prefix of the
  flag, after stripping a `--flag=value` tail, so it holds independently of how
  the CLI's parser happens to be configured. Sibling flags are unaffected --
  `--allowed-owners`, `--allow-dependency`, and `--allow-unprotected` are not
  prefixes of the flag.
- All nine babysit-prs lane CLIs now build their parsers with
  `allow_abbrev=False`. This closes the class rather than the one flag: `--mer`
  was an accepted spelling of `--merge`, so a consumer permission rule granting
  the wrapper *but not with `--merge`* was bypassable by the same mechanism.

Regression coverage asserts the wrapper's own refusal text rather than exit 2
(overloaded three ways on this path) across nine spellings, asserts argparse
reports `unrecognized arguments` at the CLI, and holds the whole scripts/
directory to `allow_abbrev=False` so a new lane script cannot reopen the class.

Closes #1371

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Code Review: fix(source-control) — merge wrapper argparse prefix-abbreviation bypass

  • Gather context and understand the request
  • Review changed files
    • bin/source-control-babysit-merge — wrapper prefix-match fix
    • scripts/babysit_merge.py and sibling parsers — allow_abbrev=False
    • scripts/engine.test.sh — new bash test rows
    • scripts/tests/test_guards.py — new Python assertions
    • SKILL.md, reference/safety.md — doc updates
    • CHANGELOG.md, plugin.json — version bump
  • Assess correctness, security, and test coverage
  • Post review findings

Summary

This is a well-executed, defense-in-depth security fix. The root cause (argparse allow_abbrev default) and the symptomatic gap (wrapper equality check) are both closed independently, which is the right call — a guard that only holds because of a downstream implementation detail isn't a guard. The test strategy is sound: red-first, text-match on the refusal (not just exit code), and an AST-based regression lock on the whole scripts/ directory.

A few observations below, ranging from a genuine correctness note to style.


Findings

1. Redirect order in check_wrapper_refusal is correct but non-obvious

engine.test.sh line 69

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

The redirect order 2>&1 >/dev/null (stderr-then-stdout) is the correct way to capture stderr while suppressing stdout inside a command substitution — because 2>&1 points stderr at the capture pipe before >/dev/null redirects stdout away — but it reads as backwards to most bash authors, who know >/dev/null 2>&1 as the idiom for suppressing both. A wrong edit here (swapping to >/dev/null 2>&1) would suppress stderr silently, making every check_wrapper_refusal call vacuously pass if the guard failed. A short comment at the line (# captures stderr; stdout suppressed) would defend it.

2. test_every_lane_parser_disables_abbreviation glob is non-recursive

test_guards.py line 163

for path in sorted(SCRIPTS.glob("*.py")):

This scans only the top level of scripts/. A future contributor who adds a script in a subdirectory of scripts/ would escape the check silently. Changing to SCRIPTS.rglob("*.py") and excluding the tests/ subdirectory explicitly would close that gap:

for path in sorted(p for p in SCRIPTS.rglob("*.py") if "tests" not in p.parts):

This is a future-proofing concern, not a present bug.

3. assertIn("unrecognized arguments", stderr) couples to argparse's error-message format

test_guards.py line 123

self.assertIn("unrecognized arguments", stderr)

The string "unrecognized arguments" is argparse's canonical phrasing and has been stable across CPython versions. It is a better discriminator than exit code alone (since exit 2 is overloaded), so this is the right call. Worth knowing the coupling exists in case a future Python upgrade ever changes it.

4. Minor: SKILL.md line 199 introduces a very long line

SKILL.md line 199

The updated sentence runs "…every prefix of it, so no unattended unpinned merge exists. The pin is carried to GitHub's server-side match-head-commit guard. It refuses" on a single line that is significantly past 120 characters. The PR states markdownlint passed, so this may be within the configured limit — but a rewrap here would match the surrounding prose style.


What is correct and well done

  • Wrapper prefix logic (lines 33–44): ${unpinned_flag:0:${#candidate}} with the RHS quoted is a literal substring compare — not a glob match — and Bash won't error on an overlong slice. It correctly refuses the exact flag, every prefix, and the --flag=value form, while passing all three sibling flags (--allow-dependency, --allow-unprotected, --allowed-owners) because none of them is a leading substring of --allow-unpinned-head.

  • [[ "$candidate" == --?* ]] guard: Correctly excludes -- (the end-of-options marker), single-dash short options, and empty strings from the prefix comparison.

  • =value stripping (candidate="${arg%%=*}"): %% strips the longest trailing match, so --flag=val=extra--flag. ✓

  • Defense-in-depth posture: The wrapper's guard holds independently of allow_abbrev=False; the CLI's allow_abbrev=False holds independently of the wrapper. Either alone closes a gap; both together close the class. This is the right architecture for a security boundary.

  • Text-match refusal assertion in check_wrapper_refusal: Exit code 2 is overloaded (wrapper refusal, argparse ambiguity error, babysit_merge.py usage error). Matching the refusal string is what distinguishes a fired guard from a coincidental argparse failure. The --allow-unp case (noted in the PR description) is exactly the trap a code-only assertion would miss.

  • AST-based regression lock in test_every_lane_parser_disables_abbreviation: Scanning the scripts/ directory by AST rather than grep means it handles both argparse.ArgumentParser(...) and a directly-imported ArgumentParser(...), and it can't be confused by a comment. A new lane script that omits allow_abbrev=False surfaces immediately.

  • run_raw refactor: Exposing stderr separately without breaking existing run() callers is clean. The original helper discarding stderr was fine for JSON-payload tests but was silently broken for argparse rejection assertions.


Verdict: Approve with the redirect-ordering comment (finding 1) as a suggested follow-up — the logic is correct as written, and the other findings are non-blocking.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Security Review — PR #1382

  • Gather context and understand request
  • Read PR diff and changed files
  • Review bash wrapper (bin/source-control-babysit-merge) for injection / logic flaws
  • Review Python argparse changes for parser-level issues
  • Review test harness changes for security gaps
  • Synthesize and post findings

Findings

No CRITICAL or IMPORTANT vulnerabilities found. One SUGGESTION-level gap is noted below.


Bash wrapper — prefix comparison logic

bin/source-control-babysit-merge lines 33–43

The core comparison:

if [[ "${unpinned_flag:0:${#candidate}}" == "$candidate" ]]; then

The RHS is "$candidate" (double-quoted), so this is a literal string compare, never a glob match. The %%=* strip is correct for --flag=value forms. The --?* filter correctly requires at least three characters (two dashes plus one). Sibling flags --allowed-owners, --allow-dependency, and --allow-unprotected all differ from the flag at or before position 9 or 11 and are correctly passed through. No logic flaw in the prefix check as-specified.

Printf — no format-string injection

Line 39–42

$arg is interpolated into the second argument of printf '%s\n%s\n', not into the format string. No format-string injection risk. The value is not re-evaluated for shell metacharacters since it is in a double-quoted string, not a $(…) context.

-- end-of-options: correctly fails closed

The for arg in "$@" loop iterates every positional argument unconditionally. -- alone does not match --?* (pattern requires at least one character after --), so it is skipped, and any guarded flag appearing after a -- separator is still caught. Correctly fails closed.

Python argparse changes

All nine changes are a single-keyword addition (allow_abbrev=False) to ArgumentParser(...) constructors, each in list form. No new subprocess invocations, credential handling, or exec surfaces are introduced. The change is mechanically uniform across the lane. No vulnerabilities introduced.

Test harness — check_wrapper_refusal redirect order

engine.test.sh lines 65–68

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

Within a $(…) capture context, 2>&1 redirects fd 2 to where fd 1 currently points (the capture pipe); the subsequent >/dev/null then redirects fd 1 to /dev/null. Net result: stderr is captured, stdout is discarded. The intent is correct and the redirect order achieves it.

The captured $err is used only inside echo "… $err" (not re-executed as a command). No injection surface.

run_raw helper — no shell injection

test_guards.py lines 23–30

subprocess.run([sys.executable, str(script), *args], …) uses a list, not shell=True. No injection surface.


SUGGESTION — Bash guard has a superstring blind spot (confidence: PLAUSIBLE)

The prefix check refuses any candidate that is a prefix of --allow-unpinned-head, but does not refuse a candidate that is a strict superstring of the flag — e.g. --allow-unpinned-head-also.

When #candidate > #unpinned_flag, bash's ${unpinned_flag:0:N} returns the complete 19-character string (it does not pad), so the comparison becomes --allow-unpinned-head == --allow-unpinned-head-also → false → the argument passes through. No exploitable path exists today (no such flag is defined, and the Python parser would reject any unrecognized flag). The gap is forward-looking: if a future contributor adds a flag whose name starts with --allow-unpinned-head and also bypasses the pin check, the bash guard would not catch it even though allow_abbrev=False on the Python side wouldn't catch it either (it would be a recognized, distinct flag).

The PR's stated design goal is that the wrapper "holds on its own regardless of how the CLI's parser is configured." That claim is true for prefixes and for the exact flag, but not for superstrings. No action is required today; noting it so the design boundary is explicit.


Summary

The cryptographic invariant of the guard — refuse every spelling of --allow-unpinned-head that argparse would have accepted, plus the exact flag — is correctly implemented. The allow_abbrev=False change closes the reported class at the parser level. No injection surfaces, credential exposures, privilege escalation paths, or logic bypasses were found in the changed files. The one noted gap is theoretical and carries no current exploitability.

…rden the new rows

Addresses the two automated review passes on the PR:

- The prefix test now runs in both directions, so a future flag that STARTS
  with `--allow-unpinned-head` is refused too. `allow_abbrev=False` cannot catch
  that case -- such a flag would be a recognized, distinct option -- so only the
  wrapper half can, and the PR's claim that this half holds on its own is now
  true for superstrings as well as abbreviations. New row covers it.
- Documented the load-bearing `2>&1 >/dev/null` redirect order in
  `check_wrapper_refusal`: the familiar `>/dev/null 2>&1` spelling would discard
  stderr and make every row vacuously pass.
- The AST scan now uses `rglob` (excluding `tests/`), so a parser added in a
  subdirectory of `scripts/` cannot escape the `allow_abbrev=False` check.
- Rewrapped the overlong SKILL.md line the doc edit introduced.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Both automated review passes triaged. Summary of dispositions:

Applied (commit 36373c0):

  1. Superstring blind spot (security-review, SUGGESTION). VALID and worth closing rather than documenting: the PR's own claim is that this half "holds on its own regardless of how the CLI's parser is configured," and that claim was true for abbreviations but not for a longer flag starting with --allow-unpinned-head. allow_abbrev=False structurally cannot catch that case — such a flag would be a recognized, distinct option — so the wrapper is the only half that can. The test now runs the prefix relation in both directions, with a --allow-unpinned-head-also row.
  2. Redirect-order comment (both reviewers, finding 1). Applied verbatim in spirit: the comment now names the failure mode explicitly ("the familiar >/dev/null 2>&1 spelling would discard BOTH and make every row here pass vacuously — do not 'fix' it to that idiom"), because the danger is a silent vacuous pass, not just non-obviousness.
  3. Non-recursive glob (code review, finding 2). Applied as suggested — rglob excluding tests/.
  4. Overlong SKILL.md line (code review, finding 4). Rewrapped. It was within markdownlint's configured limit (the gate passed both before and after), but it broke the surrounding prose style; SKILL.md is now 498 lines, still under the 500-line cap.

Noted, no change (code review, finding 3): assertIn("unrecognized arguments", stderr) does couple to argparse's phrasing. Accepted deliberately — exit 2 is overloaded three ways on this path, so the message is the only discriminator that distinguishes an argparse rejection from the wrapper's refusal or the CLI's own usage errors. A phrasing change in a future CPython would surface as a loud test failure, not a silent gap, which is the correct failure direction for a security assertion.

Re-verified after the changes: engine.test.sh → 356 unittests OK, ruff clean, 17/17 wrapper rows PASS, exit 0; shellcheck -x clean; markdownlint clean.


This was generated by AI during work-loop execution.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


PR Review — follow-up pass after 36373c0

  • Gather context and understand the request
  • Read changed files (wrapper, tests, docs)
  • Verify superstring fix correctness
  • Verify rglob / test harness changes
  • Check redirect-order comment addition
  • Post review findings

Reviewing the follow-up commit (36373c0) that addressed the four findings from the prior review passes.

Superstring fix — bidirectional prefix check

bin/source-control-babysit-merge lines 41–42

if [[ "${unpinned_flag:0:${#candidate}}" == "$candidate" \
   || "${candidate:0:${#unpinned_flag}}" == "$unpinned_flag" ]]; then

The second condition is correct. Tracing through the --allow-unpinned-head-also case (unpinned_flag = 21 chars, candidate = 26 chars):

  • First condition: ${unpinned_flag:0:26} = --allow-unpinned-head (bash returns the full 21-char string, not padded) ≠ --allow-unpinned-head-also → false
  • Second condition: ${candidate:0:21} = --allow-unpinned-head == --allow-unpinned-headtrue → refused

All three sibling flags still pass through correctly: --allow-dependency, --allow-unprotected, and --allowed-owners differ from --allow-unpinned-head at character positions well before their respective lengths, so neither direction of the prefix test fires for any of them. The existing no-over-refusal test rows verify this empirically.

--allow-unpinned-head-also test row

engine.test.sh lines 95–108

The new spelling is in the same for loop as the abbreviation rows, so it goes through check_wrapper_refusal, which asserts both exit 2 and the presence of the refusal text. That is the right assertion here: the guard must fire, not just argparse rejecting an unrecognized flag (the Python parser would also reject --allow-unpinned-head-also with unrecognized arguments, and exit 2 alone would not distinguish them).

rglob with tests/ exclusion

test_guards.py line 177

for path in sorted(p for p in SCRIPTS.rglob("*.py") if "tests" not in p.parts):

SCRIPTS resolves to the scripts/ directory. p.parts is the full tuple of absolute path components, so "tests" not in p.parts is checking for a path component literally named tests — correct for excluding scripts/tests/*.py. The check works, though p.relative_to(SCRIPTS).parts would be slightly more precise if there were ever a tests-named ancestor higher in the filesystem tree. Not a real concern here, and the current form is idiomatic.

Redirect-order comment

engine.test.sh lines 69–72

The comment names the failure mode explicitly ("would discard BOTH and make every row here pass vacuously") rather than just calling the idiom surprising. That is precisely what is needed — someone reaching for >/dev/null 2>&1 as a cleanup would see not just "non-obvious" but "this would silently break all of these assertions."

Minor: safety.md mentions "every prefix" but not the new superstring direction

reference/safety.md lines 249–252

The updated sentence says "every spelling of that flag, including every prefix of it." The wrapper now also refuses superstrings (--allow-unpinned-head-also), but the docs don't mention this. This is non-blocking — the superstring case is a forward-looking guard against a hypothetical future flag, and the wrapper's own block comment explains the full bidirectional logic. Worth a one-word tighten if the file is touched again ("every prefix of it and any flag name that starts with it"), but not worth a follow-up commit on its own.


Summary

All four findings from the prior passes are addressed correctly. The bidirectional prefix logic is sound, the test rows exercise both directions of the check with text-match assertions (not code-only), and the rglob change closes the systemic regression lock against future subdirectory additions. No new issues found. Ready to merge.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Security Review — PR #1382 (after 36373c0)

  • Gather PR diff
  • Review bash wrapper for injection / logic flaws
  • Review Python argparse changes
  • Review test harness changes
  • Review docs/changelog/manifest changes
  • Synthesize and post findings

No security issues found in the updated diff. Detailed reasoning below.


Bash wrapper — bin/source-control-babysit-merge

printf with $arg is not format-string injection

Line 43–45

printf '%s\n%s\n' \
  "source-control-babysit-merge: $unpinned_flag is not permitted through the wrapper (refused '$arg')." \
  'Invoke babysit_merge.py directly for interactive unpinned use.' >&2

$arg appears in the second positional argument to printf, not in the format string. The format string '%s\n%s\n' is a single-quoted literal; it is never user-controlled. No format-string injection surface.

No shell injection on $arg

$arg is used in two places: candidate="${arg%%=*}" (pure parameter expansion; no subshell) and in the printf message above. It is never passed to eval, $(...), exec, or any similar surface. The for arg in "$@" loop and the final babysit_python ... "$@" both use proper double-quoting; word splitting and glob expansion are absent. No injection surface.

Prefix comparison logic is correct on both sides

For the previously-reported superstring blind spot (prior run finding): commit 36373c0 adds the second condition:

|| "${candidate:0:${#unpinned_flag}}" == "$unpinned_flag"

When candidate is longer than the flag (e.g. --allow-unpinned-head-also, 26 chars), Bash's ${candidate:0:21} returns exactly the first 21 characters, which equals --allow-unpinned-head. The comparison is == with a double-quoted RHS — a literal string compare, not a glob match. This correctly closes the superstring gap; allow_abbrev=False on the Python side would not catch that case because such an option would be a recognized, distinct flag.

Sibling flags remain unaffected — none of --allow-dependency, --allow-unprotected, or --allowed-owners satisfies either prefix relation with --allow-unpinned-head, confirmed by character-level inspection.

The -- end-of-options separator ([[ "$candidate" == --?* ]] requires at least three characters) and single-dash arguments are correctly filtered by continue before reaching the comparison.

%%=* stripping is correct

For --flag=val=extra, %% matches the longest trailing pattern of the form =*, stripping =val=extra and leaving --flag. For bare --flag, there is no =, so the value is unchanged. Both cases produce the correct candidate. No bypass via a crafted =-containing value.


Python changes — nine allow_abbrev=False additions

Each of the nine lane script changes is a single-keyword addition to ArgumentParser(...). No new subprocess calls, no credential handling, no exec surfaces, no shell=True added anywhere. The changes are mechanically uniform and introduce no new attack surface.


Test harness — engine.test.sh, test_guards.py

check_wrapper_refusal captures stderr with the correct redirect order (2>&1 >/dev/null). The captured $err is only used in an echo output (not re-executed), and $MERGE_WRAPPER is a hardcoded path, not user-supplied. No injection surface.

run_raw in test_guards.py calls subprocess.run([sys.executable, str(script), *args], ...) with a list and no shell=True. No injection surface.


Other changed files

CHANGELOG.md, SKILL.md, reference/safety.md, plugin.json — documentation and version bump only. No executable content.

No GitHub Actions workflow files were changed.


Summary

The one exploitable finding from the prior security review pass (superstring blind spot, 3735d71 — SUGGESTION/PLAUSIBLE) is correctly closed by the second condition added in 36373c0. No CRITICAL or IMPORTANT vulnerabilities found in the full diff. The guard now correctly handles every prefix, the exact flag, and every superstring of the flag that starts with it, with no new injection surfaces introduced anywhere in the changeset.

…e-wrapper-abbrev-bypass

# Conflicts:
#	plugins/source-control/.claude-plugin/plugin.json
#	plugins/source-control/CHANGELOG.md
#	plugins/source-control/bin/source-control-babysit-merge
#	plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py
#	plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Closed in favour of #1428, which carries Closes #1371.

This PR was opened before #1354 landed and is now a strict superset of it: main already has allow_abbrev=False on babysit_merge.py and babysit_resolve_thread.py, so re-basing this branch would mean re-resolving conflicts across a diff that is half already-merged. It was also stuck at 0.26.5 while main has moved to 0.29.0.

#1428 is the residual on a fresh base — the seven parsers #1354 did not reach, plus the directory-wide gate this PR proposed, restated as a guard-contract check now that #1285 gave it a home. Nothing from here is dropped: #1371 stays open until #1428 merges, and the branch remains on the remote if any of this diff is wanted back.

kyle-sexton added a commit that referenced this pull request Jul 26, 2026
…point (#1428)

Closes #1371.

Replaces #1382, which is closed in favour of this. #1382 was opened
before #1354 landed and is now a strict superset of it: re-scoped here
to the residual only, on a fresh base, with the property bound to the
directory rather than to a list of files.

## The gap

A permission grant states its condition as the literal presence or
absence of a flag in the command text — above all *"no `--merge` means
check-only"*. Argparse's default prefix abbreviation lets `--mer`
resolve to `--merge` while the command text contains no such flag, so
the written command and the resolved behavior diverge. That is exactly
what such a condition has to be able to rule out.

#1354 closed this on `babysit_merge.py` and `babysit_resolve_thread.py`.
Seven entry points still inherited the default:

`babysit_findings.py` · `manage_babysit_lease.py` ·
`manage_feedback_ledger.py` · `pr_queue_snapshot.py` ·
`prune_babysit_worktrees.py` · `refresh_pr_branch.py` ·
`request_review.py`

All nine now set `allow_abbrev=False`.

## Why a gate, not seven more edits

Hardening entry points one at a time is what let the gap survive #1354
for seven files. The guard contract gains a check over the whole
catalogue: every catalogued Python entry point is invoked with `--hel`
and must not exit 0.

`--help` is registered on every parser and short-circuits parsing, so an
abbreviation that *resolves* exits 0 before required-argument validation
ever runs, while one that does not is a usage error. That makes the exit
code a sufficient discriminator without a per-CLI argument shape — which
is what makes this a gate over the catalogue rather than a
hand-maintained list of cases. The message is deliberately not asserted:
several of these parsers have a required mutually exclusive group that
errors before any unrecognized argument is reported.

A companion test asserts that discrimination against argparse itself
rather than assuming it.

## Verification

- `python -m unittest discover -s tests` — 387 tests, OK.
- Detector verified rather than assumed: reverting `request_review.py`'s
`allow_abbrev=False` fails the gate naming that file (`resolved the
abbreviation --hel to --help and exited 0`), and it passes again on
restore.

## Compatibility

Abbreviated invocations that previously worked are now usage errors.
That is the intent, and the version takes a minor bump for it —
`source-control` 0.29.0 → 0.31.0 (0.30.0 is claimed by #1264, open).

## Related

- #1354 — closed the same defect on the first two entry points
- #1382 — the superset PR this replaces
- #1285 — the guard contract this gate is added to

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

source-control:babysit-prs: the merge wrapper's --allow-unpinned-head refusal is bypassable by argparse prefix abbreviation

1 participant