diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index 34ce286106..accb244452 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "source-control", - "version": "0.32.0", + "version": "0.32.1", "description": "Git and GitHub delivery workflow: /commit (Conventional Commits + Co-Authored-By trailer via safe heredoc mechanics), /pull-request (prep, create, CI monitoring, review-comment triage, merge, CI-log fetch), /babysit-prs (self-pacing fleet loop — safe by default; opt-in worker/autopilot tiers add gate-checked merge and thread resolution behind a deterministic Python engine), /babysit-loop (the loop-lane merge lane: a standing or drain loop that invokes babysit-prs per cycle, configured through repo-scoped babysit_loop_* keys on the layered source-control.md seam, with merge authority human-only until the target repo's tracked config adopts the lane, a gate-proven C2-mechanical baseline once adopted, and standing merge-rung raises binding from the team-tracked layer only — with one named exception, where an invocation line explicitly typing both the autopilot tier keyword and the dedicated raise argument --merge c3-this-run widens that single invocation's merge authority up to C3 behind a fresh independent frontier-tier resolver, while C4-structural and C5-untrusted-provenance stay unconditionally human-merge), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention merged across its config layers and the babysit-prs config, or apply — interview the repo and write the convention config to a chosen layer), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep — never --abort). The commit-subject / PR-title convention is configurable via a source-control.md config written by a re-runnable setup skill, layered across a ~/.claude user-global file, the tracked team file, and a gitignored .claude/source-control.local.md personal overlay merged per key; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 37032861d5..7ebad05d39 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,26 @@ All notable changes to the `source-control` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.32.1] + +### Fixed + +- **`source-control-babysit-merge`'s `--allow-unpinned-head` guard now strips an `=value` tail + before the prefix comparison (#1522).** The guard refuses the flag and every long-option prefix + of it via `"--allow-unpinned-head" == "$arg"*`, but `--allow-unpinned-head=true` is not itself a + prefix of `--allow-unpinned-head` — the `=true` suffix broke the match, so the wrapper let the + argument through and argparse rejected it instead (the flag is `store_true`, which never accepts + an explicit argument). The refusal was still real today, but incidentally so: it depended on the + interpreter behind the wrapper exactly as this guard exists to not do — the moment the guarded + flag (or an equivalent guarded flag) accepted a value, the same test would have stopped refusing + anything, silently. Fixed by stemming each argument on its first `=` before the prefix test. + `engine.test.sh` gains a `check_wrapper_refusal` helper that asserts the wrapper's own refusal + text on stderr (not just exit code — exit 2 is shared between the wrapper's refusal and + argparse's own usage/rejection errors, so an exit-code-only assertion would have passed before + and after this fix for different reasons) and new rows for `--allow-unpinned-head=true`, + `--allow-unpinned=1`, and `--allow-unpinned-hea=1`, plus no-over-refusal rows for + `--allow-dependency`, `--allow-unprotected`, and `--allowed-owners=owner`. + ## [0.32.0] ### Added diff --git a/plugins/source-control/bin/source-control-babysit-merge b/plugins/source-control/bin/source-control-babysit-merge index 0f58e7b635..9d7741df48 100755 --- a/plugins/source-control/bin/source-control-babysit-merge +++ b/plugins/source-control/bin/source-control-babysit-merge @@ -22,10 +22,22 @@ scripts="$here/../skills/babysit-prs/scripts" # `--allow-unpinned-hea` sail through while argparse (with abbreviation on) # resolved it to the refused flag. A prefix here can only mean the refused flag # or an ambiguous stem of it, so refusing the whole family is exact. +# +# Strip a `--flag=value` tail before the prefix test (#1522). The refused +# flag is argparse store_true, so it never legitimately carries `=value`, but +# `--allow-unpinned-head=true` is NOT itself a prefix of +# `--allow-unpinned-head` -- the `=true` suffix breaks the plain prefix +# comparison, so an unstripped test let it fall through to argparse, which +# happens to also reject it today (store_true takes no explicit argument). +# That is still the wrapper depending on the interpreter behind it, the exact +# thing this guard exists to not do: the moment a guarded flag takes a value, +# or an equivalent guarded flag that takes one is added, the same test would +# stop refusing at all while looking identical. for arg in "$@"; do - if [[ "$arg" == --a* && "--allow-unpinned-head" == "$arg"* ]]; then + stem="${arg%%=*}" + if [[ "$stem" == --a* && "--allow-unpinned-head" == "$stem"* ]]; then printf '%s\n' \ - "source-control-babysit-merge: $arg is not permitted through the wrapper (--allow-unpinned-head or a prefix of it)." \ + "source-control-babysit-merge: $arg is not permitted through the wrapper (--allow-unpinned-head or a prefix of it, with or without an =value tail)." \ 'Invoke babysit_merge.py directly for interactive unpinned use.' >&2 exit 2 fi diff --git a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md index e6383afd3b..ed6759879d 100644 --- a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -50,6 +50,8 @@ Each row is executed. `Refused by` says which layer rejected the invocation. On | `merge.tier-params-without-umbrella` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --lane-logins lane` | 2 | python-cli | -- | `--autopilot-merge-tier` | `babysit_merge.py::main` | Tier parameter sets without --autopilot-merge-tier are a usage error at exit 2, never a silent no-op that reads as configured. | | `merge.unpinned-head-refused-by-wrapper` | `bin/source-control-babysit-merge` | `owner/repo#1 --merge --allow-unpinned-head` | 2 | bash-wrapper | -- | `--allow-unpinned-head` | `bin/source-control-babysit-merge (argument filter loop)` | The bin/ wrapper refuses --allow-unpinned-head in bash, before Python runs: no allow-rule-covered invocation of the bare command can merge an unvetted head. The refusal prints plain text to stderr and emits no JSON envelope, which is how a caller can tell the wrapper -- not the CLI -- rejected it. | | `merge.abbreviated-unpinned-head-refused-by-wrapper` | `bin/source-control-babysit-merge` | `owner/repo#1 --merge --allow-unpinned-hea` | 2 | bash-wrapper | -- | `--allow-unpinned-hea` | `bin/source-control-babysit-merge (argument filter loop)` | The wrapper refusal covers every long-option PREFIX of --allow-unpinned-head, not the exact spelling alone. An equality-only filter would let `--allow-unpinned-hea` through to an abbreviation-resolving parser and reinstate the override the wrapper exists to remove, so the prefix family is refused as one. | +| `merge.equals-value-unpinned-head-refused-by-wrapper` | `bin/source-control-babysit-merge` | `owner/repo#1 --merge --allow-unpinned-head=true` | 2 | bash-wrapper | -- | `--allow-unpinned-head=true` | `bin/source-control-babysit-merge (argument filter loop)` | The wrapper refusal covers an =value spelling of the flag or of any prefix of it, not just a bare long option. `--allow-unpinned-head=true` is not itself a PREFIX of `--allow-unpinned-head` -- the `=true` tail breaks a plain prefix comparison, so an unstemmed filter (#1522) let the argument reach argparse, which happens to also reject it today only because the guarded flag is store_true and takes no explicit value. That made the wrapper's refusal depend on the interpreter behind it, exactly what this guard exists to not do. | +| `merge.equals-value-abbreviated-unpinned-head-refused-by-wrapper` | `bin/source-control-babysit-merge` | `owner/repo#1 --merge --allow-unpinned=1` | 2 | bash-wrapper | -- | `--allow-unpinned=1` | `bin/source-control-babysit-merge (argument filter loop)` | The =value stripping composes with the prefix family: `--allow-unpinned=1` (a prefix of --allow-unpinned-head with an =value tail) is refused by the wrapper too, not only the exact flag's =value spelling. | | `merge.abbreviation-is-not-resolved-by-the-cli` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --merge --allow-unpinned-hea` | 2 | python-cli | -- | `--allow-unpinned-hea` | `babysit_merge.py::main` | The wrapper's prefix filter is belt to the CLI's braces: the parser sets allow_abbrev=False, so an abbreviated flag reaching Python directly is an unrecognized argument at exit 2 rather than a silently resolved override. | | `merge.wrapper-reaches-failclosed-cli` | `bin/source-control-babysit-merge` | `owner/repo#1` | 3 | python-cli | -- | `allowed-owners` | `babysit_merge.py::main` | The wrapper adds no capability of its own: with no --allowed-owners it hands off to the CLI, which refuses at exit 3 exactly as a direct invocation does. | | `resolve.allowlist-absent` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1` | 3 | python-cli | asserted | -- | `babysit_resolve_thread.py::main` | The thread resolver is fail-closed on scope the same way the merge gate is: no --allowed-owners means exit 3. | diff --git a/plugins/source-control/skills/babysit-prs/scripts/engine.test.sh b/plugins/source-control/skills/babysit-prs/scripts/engine.test.sh index 59cffcf532..924f6fc80b 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/engine.test.sh +++ b/plugins/source-control/skills/babysit-prs/scripts/engine.test.sh @@ -56,10 +56,54 @@ check_exit() { fi } +# Exit 2 on this path is overloaded: it is both the wrapper's own refusal AND +# argparse's usage/unrecognized-argument error for the CLI it wraps. A +# code-only assertion cannot distinguish "the wrapper held the boundary" from +# "the wrapper let it through and argparse happened to also reject it" -- the +# --allow-unpinned-head=true spelling is exactly that trap (#1522). This +# helper asserts on the wrapper's own refusal text on stderr, not just the +# exit code. +check_wrapper_refusal() { + local label="$1" + shift + local stderr got + stderr="$(bash "$MERGE_WRAPPER" "$@" 2>&1 >/dev/null)" + got=$? + if [[ "$got" == "2" && "$stderr" == *"is not permitted through the wrapper"* ]]; then + echo "PASS: $label" + else + echo "FAIL: $label (want exit 2 + wrapper refusal text, got $got: $stderr)" >&2 + FAILED=1 + fi +} + # The wrapper refuses the interactive unpinned override so no allow-rule-covered # invocation can merge an unvetted head. -check_exit "merge wrapper rejects --allow-unpinned-head" 2 \ - bash "$MERGE_WRAPPER" "owner/repo#1" --merge --allow-unpinned-head +check_wrapper_refusal "merge wrapper rejects --allow-unpinned-head" \ + "owner/repo#1" --merge --allow-unpinned-head +# The wrapper refuses a long-option prefix too (allow_abbrev on the CLI would +# otherwise resolve it to the guarded flag behind the wrapper's back). +check_wrapper_refusal "merge wrapper rejects --allow-unpinned-hea (prefix spelling)" \ + "owner/repo#1" --merge --allow-unpinned-hea +# The wrapper refuses the =value spelling of the flag and of a prefix of it -- +# the prefix comparison alone missed this because "--allow-unpinned-head=true" +# is not itself a prefix of "--allow-unpinned-head" (#1522). +check_wrapper_refusal "merge wrapper rejects --allow-unpinned-head=true (=value spelling)" \ + "owner/repo#1" --merge --allow-unpinned-head=true +check_wrapper_refusal "merge wrapper rejects --allow-unpinned=1 (=value prefix spelling)" \ + "owner/repo#1" --merge --allow-unpinned=1 +check_wrapper_refusal "merge wrapper rejects --allow-unpinned-hea=1 (=value prefix spelling)" \ + "owner/repo#1" --merge --allow-unpinned-hea=1 +# No over-refusal: sibling flags that share the --allow prefix, including with +# an =value tail, still reach the fail-closed CLI rather than the wrapper. +# --allowed-owners deliberately names an owner NOT in scope, so the owner-scope +# refusal fires and the assertion holds without any network call. +check_exit "merge wrapper does not over-refuse --allow-dependency" 3 \ + bash "$MERGE_WRAPPER" "owner/repo#1" --allowed-owners someone-else --allow-dependency +check_exit "merge wrapper does not over-refuse --allow-unprotected" 3 \ + bash "$MERGE_WRAPPER" "owner/repo#1" --allowed-owners someone-else --allow-unprotected +check_exit "merge wrapper does not over-refuse --allowed-owners=owner" 3 \ + bash "$MERGE_WRAPPER" "owner/repo#1" --allowed-owners=someone-else # The wrapper reaches the fail-closed CLI when no allowlist is supplied. check_exit "merge wrapper reaches fail-closed CLI (no allowlist)" 3 \ bash "$MERGE_WRAPPER" "owner/repo#1" diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py b/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py index c3e54d0d97..492cd1dbc7 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py @@ -338,6 +338,40 @@ class DocCommandSource: refused_by=BASH_WRAPPER, enforced_at="bin/source-control-babysit-merge (argument filter loop)", ), + Refusal( + id="merge.equals-value-unpinned-head-refused-by-wrapper", + claim=( + "The wrapper refusal covers an =value spelling of the flag or of any " + "prefix of it, not just a bare long option. `--allow-unpinned-head=true` " + "is not itself a PREFIX of `--allow-unpinned-head` -- the `=true` tail " + "breaks a plain prefix comparison, so an unstemmed filter (#1522) let " + "the argument reach argparse, which happens to also reject it today " + "only because the guarded flag is store_true and takes no explicit " + "value. That made the wrapper's refusal depend on the interpreter " + "behind it, exactly what this guard exists to not do." + ), + entry_point=MERGE_WRAPPER, + argv=("owner/repo#1", "--merge", "--allow-unpinned-head=true"), + exit_code=2, + error_contains=("--allow-unpinned-head=true",), + refused_by=BASH_WRAPPER, + enforced_at="bin/source-control-babysit-merge (argument filter loop)", + ), + Refusal( + id="merge.equals-value-abbreviated-unpinned-head-refused-by-wrapper", + claim=( + "The =value stripping composes with the prefix family: " + "`--allow-unpinned=1` (a prefix of --allow-unpinned-head with an " + "=value tail) is refused by the wrapper too, not only the exact " + "flag's =value spelling." + ), + entry_point=MERGE_WRAPPER, + argv=("owner/repo#1", "--merge", "--allow-unpinned=1"), + exit_code=2, + error_contains=("--allow-unpinned=1",), + refused_by=BASH_WRAPPER, + enforced_at="bin/source-control-babysit-merge (argument filter loop)", + ), Refusal( id="merge.abbreviation-is-not-resolved-by-the-cli", claim=( @@ -1072,8 +1106,15 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje def wrapper_denies(wrapper: str, flag: str) -> bool: - """Whether `flag`, as spelled, cannot survive `wrapper`'s argument filter.""" - return any(denied.startswith(flag) for denied in WRAPPER_DENIED_FLAGS[wrapper]) + """Whether `flag`, as spelled, cannot survive `wrapper`'s argument filter. + + Strips an `=value` tail before the prefix check, mirroring the wrapper's + own bash filter (#1522): `--allow-unpinned-head=true` is refused because + its STEM `--allow-unpinned-head` is a denied prefix, even though the full + `=value` string is not itself a prefix of any denied entry. + """ + stem = flag.split("=", 1)[0] + return any(denied.startswith(stem) for denied in WRAPPER_DENIED_FLAGS[wrapper]) _PREAMBLE = """# Guard contract