From d1745d5f2b2ff8b7315d5da02e4384808540c712 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:45:17 -0400 Subject: [PATCH 1/8] feat(source-control): bind babysit guard semantics to an executable contract The facts a host permission classifier must know about the babysit lane -- which entry points mutate, which flags gate which guard, where a refusal is enforced, and how a mutation is actually performed -- were restated in prose by every consumer with nothing detecting drift. They are now a table in scripts/tests/guard_contract.py, executed row by row against the real entry points by test_guards.py, and rendered to a citable reference/guard-contract.md that CI proves current. Every row carries the prose claim it backs, so a changed guard fails naming the downstream claim rather than reporting an exit-code mismatch. Five binding kinds, chosen per fact rather than uniformly: - refusals, invoked for real; a bin/ row additionally asserts WHICH layer refused, via the observable discriminator that a bash-wrapper refusal emits no JSON envelope - predicates, calling the classifier directly, because --autonomous's isOutdated requirement is a condition over fetched API data that no argument shape can express - effects, run offline against a throwaway state dir; this is what proves manage_babysit_lease.py acquire writes with no --apply, contrary to what its flag names suggest - mechanisms, asserted against source, because refresh_pr_branch.py never pushes and a consumer reasoning about push guards reasons about the wrong thing - documented command lines, checked against the backing CLI's own parser Catalogue gates fail when a new entry point, wrapper, or command-spelling document arrives without a row. The argparse flag catalogue stays deferred with its trigger recorded at the module docstring. Closes #1265 --- .../source-control/.claude-plugin/plugin.json | 2 +- plugins/source-control/CHANGELOG.md | 22 + .../skills/babysit-prs/SKILL.md | 2 + .../babysit-prs/reference/guard-contract.md | 109 ++ .../skills/babysit-prs/reference/safety.md | 7 + .../scripts/tests/guard_contract.py | 1092 +++++++++++++++++ .../babysit-prs/scripts/tests/test_guards.py | 488 ++++++-- 7 files changed, 1590 insertions(+), 132 deletions(-) create mode 100644 plugins/source-control/skills/babysit-prs/reference/guard-contract.md create mode 100644 plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index c0a4f1457f..5985a9039b 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.26.0", + "version": "0.27.0", "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 merge-rung raises binding from the team-tracked layer only), /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 3d81dd77cc..7e5bfb17f8 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,28 @@ 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.27.0] + +### Added + +- **`babysit-prs` guard semantics are now an executable contract (`#1265`).** The facts a host + permission classifier has to know about this lane — which entry points mutate, which flags gate + which guard, where a refusal is enforced, and how a mutation is actually performed — were + restated in prose by every consumer and had nothing detecting drift. They are now a table in + `skills/babysit-prs/scripts/tests/guard_contract.py`, executed row by row against the real entry + points by `test_guards.py`, and rendered to a citable + `skills/babysit-prs/reference/guard-contract.md`. Every row carries the prose claim it backs, so + a changed guard fails CI with a message naming the downstream claim that just became false. Five + binding kinds: refusals (invoked, exit code and message asserted), predicates (the classifier + called directly, because `--autonomous`'s `isOutdated` requirement is a condition over fetched + API data that no argument shape expresses), effects (run offline against a throwaway state dir — + this is what proves `manage_babysit_lease.py acquire` writes with no `--apply`, contrary to what + its flag names suggest), mechanisms (`refresh_pr_branch.py` uses GitHub's server-side + `update-branch` and never pushes), and documented command lines (every `bin/`-path wrapper + command spelled in `reference/safety.md` and `reference/orchestration.md` is checked against the + backing CLI's own parser). Catalogue gates fail when a new entry point, wrapper, or + command-spelling document arrives without a row. + ## [0.26.0] ### Added diff --git a/plugins/source-control/skills/babysit-prs/SKILL.md b/plugins/source-control/skills/babysit-prs/SKILL.md index 0a98ba225f..ce4593de22 100644 --- a/plugins/source-control/skills/babysit-prs/SKILL.md +++ b/plugins/source-control/skills/babysit-prs/SKILL.md @@ -187,6 +187,8 @@ That gate merges **only when every criterion holds** — the criteria and the sa The two mutation gates are invoked ONLY through their wrapper scripts, by the bundled `bin/`-path form — never the bare command name nor the raw Python behind them. Each `source-control-babysit- …` spelled in the bullets below is that wrapper launched by its `bin/`-path form; the exact form is the single home in [reference/safety.md](reference/safety.md). Both fail closed without `--allowed-owners`. +What each gate refuses, which invocations mutate, and where each refusal is enforced is the +executable contract in [reference/guard-contract.md](reference/guard-contract.md). - **Merge readiness** — `source-control-babysit-merge owner/repo#N --allowed-owners --self-logins @me,` (read-only; add `--merge --expected-head diff --git a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md new file mode 100644 index 0000000000..8bd01a6c3d --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -0,0 +1,109 @@ +# Guard contract + + + +What a host permission classifier, an orchestration prompt, or a downstream +consumer may assume about this lane's entry points. Every statement below is +executed as an assertion by `scripts/tests/test_guards.py`; a guard change that +falsifies one fails CI with a message naming the claim. + +Cite a row by its ID. IDs are stable; rows are removed only when the behavior is. + +Source anchors are `file::symbol`, deliberately not line numbers. + +## Entry points: what mutates + +`gate` is the condition under which the mutation happens at all. A blank wrapper column means the entry point has no `bin/` wrapper and is invoked through the interpreter. + +| Entry point | Wrapper | Class | Mutates | Gate | Claim | Backed by | +| --- | --- | --- | --- | --- | --- | --- | +| `skills/babysit-prs/scripts/babysit_merge.py` | `bin/source-control-babysit-merge` | conditionally mutating | merges the PR on GitHub; writes queue state | --merge (absent: readiness check only, exit 0 ready / 10 not ready) | Without --merge this is a readiness reporter. With it, the TOCTOU guard requires --expected-head unless --allow-unpinned-head is passed -- and the bin/ wrapper refuses that override outright. | `merge.allowlist-absent`, `merge.unpinned-head-refused-by-wrapper`, `merge.wrapper-filters-unpinned-head-in-bash` | +| `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `bin/source-control-babysit-resolve-thread` | conditionally mutating | marks review threads resolved on GitHub | --resolve (absent: classification report only) | Which threads it may touch is decided per fetched thread by the classifier, not by the argument shape -- see the predicate rows. The wrapper adds no refusal of its own. | `resolve.allowlist-absent`, `resolve.autonomous-bulk-refused`, `classify.interactive-does-not-require-outdated`, `resolve.wrapper-carries-no-filter` | +| `skills/babysit-prs/scripts/manage_babysit_lease.py` | -- | mutating | writes, renews, and deletes lease files under --state-dir | none for acquire / heartbeat / release -- they write unconditionally; --apply gates reap's deletion ONLY | Local filesystem writes only, no GitHub write. The --apply flag does not make this script read-only in its other three actions. | `lease.acquire-writes-without-apply`, `lease.reap-without-apply-is-a-dry-run` | +| `skills/babysit-prs/scripts/manage_feedback_ledger.py` | -- | conditionally mutating | records feedback dispositions and fix rounds in queue state | --apply, additionally requiring --lease-token | Local state only, no GitHub write. The lease token requirement means an --apply invocation still refuses without proof of lease ownership. | `ledger.apply-requires-lease-token` | +| `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | -- | conditionally mutating | runs `git worktree remove` on eligible worktrees under --root | --apply (absent: dry run) | The most destructive local helper in the lane. --root is required, so it cannot sweep a directory the caller did not name. | `prune.root-is-required`, `prune.removes-worktrees-via-git` | +| `skills/babysit-prs/scripts/refresh_pr_branch.py` | -- | conditionally mutating | asks GitHub to update the PR branch server-side; writes queue state | --apply | Mutates the PR branch on GitHub WITHOUT pushing: the update is a pinned server-side update-branch API call. | `refresh.uses-server-side-update-branch` | +| `skills/babysit-prs/scripts/request_review.py` | -- | conditionally mutating | POSTs an issue comment requesting an AI re-review; writes queue state | --apply | A GitHub write despite the read-sounding name. | `review.posts-a-comment` | +| `skills/babysit-prs/scripts/pr_queue_snapshot.py` | -- | conditionally mutating | writes the queue snapshot to local state | --write-state (absent: reports the snapshot without persisting it) | No GitHub write on any path. The only mutation is the local snapshot file. | `snapshot.write-state-gates-the-only-write` | +| `skills/babysit-prs/scripts/babysit_findings.py` | -- | read-only | nothing | n/a | Pure classification over supplied input; no write of any kind. | -- | + +## Refusals: guards that fire on argument shape alone + +Each row is executed. `Refused by` says which layer rejected the invocation. On a `bin/` entry point the distinction is asserted, using the observable discriminator: a bash-wrapper refusal never reaches the interpreter, so it emits plain stderr and no JSON envelope. On a CLI entry point the layer is always `python-cli` and only the exit code and message are asserted. + +| ID | Entry point | Invocation | Exit | Refused by | Error names | Enforced at | Claim | +| --- | --- | --- | --- | --- | --- | --- | --- | +| `merge.allowlist-absent` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1` | 3 | python-cli | `allowed-owners` | `babysit_merge.py::main` | The merge gate is fail-closed on scope: with no --allowed-owners it refuses every PR at exit 3 rather than defaulting to the current repo's owner. | +| `merge.owner-out-of-scope` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners someone-else` | 3 | python-cli | -- | `babysit_merge.py::main` | An owner outside --allowed-owners is refused at exit 3, so a permission rule may assume the gate never acts on a repository the operator did not name. | +| `merge.owner-check-precedes-self-login-resolution` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners someone-else --self-logins @me` | 3 | python-cli | -- | `babysit_merge.py::main` | The owner check runs before '@me' self-login resolution, which is a network call: an out-of-scope owner refuses at exit 3 with no gh invocation at all. | +| `merge.short-expected-head` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --merge --expected-head abc` | 2 | python-cli | `expected-head` | `babysit_merge.py::main` | --expected-head must be a long-enough SHA prefix; a short value is a usage error at exit 2, never a loose prefix match against the live head. | +| `merge.malformed-ref` | `skills/babysit-prs/scripts/babysit_merge.py` | `not-a-ref --allowed-owners owner` | 2 | python-cli | -- | `babysit_merge.py::main` | The PR reference must parse as owner/repo#number or a PR URL; anything else is a usage error at exit 2. | +| `merge.autopilot-tier-without-required-sets` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier` | 3 | python-cli | `--lane-logins`, `--approver-bot-logins`, `--block-labels` | `babysit_merge.py::main` | The autopilot merge tier is fail-closed: the umbrella flag alone, with none of --lane-logins, --approver-bot-logins, or --block-labels, refuses at exit 3 before any network access. | +| `merge.autopilot-tier-partial-config` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier --lane-logins lane --approver-bot-logins bot` | 3 | python-cli | `--block-labels` | `babysit_merge.py::main` | A partially configured autopilot tier still refuses at exit 3, naming only the missing set -- there is no partial-credit tier. | +| `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.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 | -- | `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. | +| `resolve.count-pin-without-thread-id` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --expected-comment-count 3` | 2 | python-cli | `thread-id` | `babysit_resolve_thread.py::main` | --expected-comment-count without --thread-id is a usage error at exit 2: a pin with no thread to pin would silently fall through to resolving every eligible thread. | +| `resolve.last-updated-pin-without-thread-id` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --expected-last-updated 2026-07-17T10:00:00Z` | 2 | python-cli | `thread-id` | `babysit_resolve_thread.py::main` | --expected-last-updated without --thread-id is a usage error at exit 2, for the same silent-bulk-fallthrough reason as the count pin. | +| `resolve.thread-id-without-pins` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --resolve --thread-id PRRT_abc` | 2 | python-cli | `--expected-comment-count`, `--expected-last-updated` | `babysit_resolve_thread.py::main` | A --thread-id resolve requires BOTH TOCTOU pins; supplying neither is a usage error at exit 2 naming both missing flags. | +| `resolve.autonomous-bulk-refused` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --autonomous --resolve` | 2 | python-cli | `thread-id`, `bulk-resolve` | `babysit_resolve_thread.py::main` | --autonomous --resolve without --thread-id is refused at exit 2: --autonomous means SINGLE PINNED THREAD, never bulk. A worker's own push marks a thread isOutdated, so a bulk autonomous resolve would clear threads with no proof the finding was addressed. | +| `resolve.autonomous-allow-unpinned-refused` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --autonomous --resolve --thread-id PRRT_abc --allow-unpinned-thread` | 2 | python-cli | `allow-unpinned-thread` | `babysit_resolve_thread.py::main` | There is no unpinned autonomous resolve: --allow-unpinned-thread is an interactive-only override and is refused at exit 2 under --autonomous even with a single --thread-id. | +| `resolve.wrapper-reaches-failclosed-cli` | `bin/source-control-babysit-resolve-thread` | `owner/repo#1` | 3 | python-cli | -- | `babysit_resolve_thread.py::main` | The resolve wrapper is a pure passthrough: no --allowed-owners refuses at exit 3 from the CLI, unchanged by the wrapper. | +| `resolve.wrapper-filters-nothing` | `bin/source-control-babysit-resolve-thread` | `owner/repo#1 --allowed-owners owner --autonomous --resolve --thread-id PRRT_abc --allow-unpinned-thread` | 2 | python-cli | `allow-unpinned-thread` | `babysit_resolve_thread.py::main` | ASYMMETRY: unlike the merge wrapper, the resolve wrapper filters no argument. Its --allow-unpinned-thread refusal comes from Python -- proven by the JSON envelope on stdout -- so an operator may not assume a bash-level refusal exists on this entry point just because one exists on the other. | +| `ledger.apply-requires-lease-token` | `skills/babysit-prs/scripts/manage_feedback_ledger.py` | `dispose --pr owner/repo#1 --expected-head-sha 0123456789abcdef0123456789abcdef01234567 --feedback-id F1 --reason contract-row --state-dir {state_dir} --apply` | 2 | python-cli | `lease token` | `manage_feedback_ledger.py::main` | A ledger write is lease-gated: --apply without --lease-token refuses at exit 2 rather than writing an unowned entry. | +| `prune.root-is-required` | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | `--state-dir {state_dir}` | 2 | python-cli | `--root` | `prune_babysit_worktrees.py::main` | prune_babysit_worktrees.py has no default root: omitting --root is a usage error at exit 2, so it can never sweep an unintended directory. | +| `lease.state-dir-is-required` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap` | 2 | python-cli | `--state-dir` | `manage_babysit_lease.py::main` | State-dir resolution is flag-only with no environment fallback: omitting --state-dir is a usage error at exit 2. | +| `lease.scope-required-for-non-reap` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `acquire --state-dir {state_dir}` | 2 | python-cli | `--scope` | `manage_babysit_lease.py::main` | --scope is required for acquire, heartbeat, and release; only reap runs without it. | + +## Predicates: guards over runtime data, not flags + +These decide per fetched thread, so no argument shape can express them and no argparse introspection can recover them. `thread` is the projected thread record; `flags` are the classifier's keyword arguments. + +| ID | Thread | Flags | Result | Enforced at | Claim | +| --- | --- | --- | --- | --- | --- | +| `classify.human-thread-refused-by-default` | isResolved=False, botOnly=False, isOutdated=True | autonomous=False, only_outdated=False, include_human=False | `skipped-human-thread` | `babysit_resolve_thread.py::classify` | Bot-only is a bright line, not a heuristic: a thread with ANY human participant is skipped whenever --include-human is absent, regardless of every other flag. | +| `classify.include-human-opts-in` | isResolved=False, botOnly=False, isOutdated=True | autonomous=False, only_outdated=False, include_human=True | `eligible` | `babysit_resolve_thread.py::classify` | --include-human is the ONLY thing that lifts the bot-only line. A permission rule asserting 'bot threads only' is false for any invocation carrying it. | +| `classify.autonomous-requires-outdated` | isResolved=False, botOnly=True, isOutdated=False | autonomous=True, only_outdated=False, include_human=False | `skipped-not-outdated` | `babysit_resolve_thread.py::classify` | --autonomous requires the deterministic isOutdated 'addressed' signal: a still-current bot thread is skipped so an unattended worker cannot resolve a live finding and self-satisfy the merge gate. | +| `classify.only-outdated-requires-outdated` | isResolved=False, botOnly=True, isOutdated=False | autonomous=False, only_outdated=True, include_human=False | `skipped-not-outdated` | `babysit_resolve_thread.py::classify` | --only-outdated imposes the same isOutdated requirement as --autonomous, independently of it. | +| `classify.interactive-does-not-require-outdated` | isResolved=False, botOnly=True, isOutdated=False | autonomous=False, only_outdated=False, include_human=False | `eligible` | `babysit_resolve_thread.py::classify` | THE CLAIM CONSUMERS GET WRONG: 'resolves only outdated bot threads' is TRUE ONLY under --autonomous or --only-outdated. With neither flag, a current (not outdated) bot thread is eligible. | +| `classify.include-human-does-not-lift-outdated` | isResolved=False, botOnly=False, isOutdated=False | autonomous=True, only_outdated=False, include_human=True | `skipped-not-outdated` | `babysit_resolve_thread.py::classify` | --include-human lifts only the bot-only line. Under --autonomous the isOutdated requirement still applies, so the two guards are independent and neither flag opens the other. | +| `classify.already-resolved-precedes-every-guard` | isResolved=True, botOnly=False, isOutdated=False | autonomous=True, only_outdated=False, include_human=False | `skipped-already-resolved` | `babysit_resolve_thread.py::classify` | An already-resolved thread is skipped before any other guard is consulted, so a re-run never re-resolves and never reports a human thread it declined. | + +## Effects: what reaches disk + +Executed offline against a throwaway state directory seeded with `Fixture`. `State changes` is the observed before/after difference of that directory, not a reading of the flag names. + +| ID | Entry point | Invocation | Fixture | Exit | State changes | Claim | +| --- | --- | --- | --- | --- | --- | --- | +| `lease.acquire-writes-without-apply` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `acquire --scope worker --pr owner/repo#1 --state-dir {state_dir}` | empty | 0 | yes | THE TRAP: manage_babysit_lease.py's --apply help reads 'reap only', but acquire writes a lease file unconditionally, with no --apply and no network. A permission rule that derives read-only from flag names classifies acquire wrong. | +| `lease.reap-without-apply-is-a-dry-run` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap --state-dir {state_dir}` | expired-worker-lease | 0 | no | --apply gates reap alone: a reap without it reports which lease files are expired and deletes none of them. | +| `lease.reap-with-apply-deletes` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap --state-dir {state_dir} --apply` | expired-worker-lease | 0 | yes | reap --apply deletes expired worker lease files. This is the one action on this script the --apply flag actually gates. | + +## Mechanisms: how a mutation is performed + +Asserted against the entry point's source, because the mechanism -- not just the fact of mutation -- decides which permission rule is the relevant one. + +| ID | Entry point | Claim | +| --- | --- | --- | +| `refresh.uses-server-side-update-branch` | `skills/babysit-prs/scripts/refresh_pr_branch.py` | refresh_pr_branch.py never pushes. It asks GitHub to update the branch server-side via PUT repos/{repo}/pulls/{number}/update-branch, pinned with expected_head_sha. A consumer reasoning about push guards for this script is reasoning about the wrong mechanism -- it needs no local write access to the branch at all. | +| `review.posts-a-comment` | `skills/babysit-prs/scripts/request_review.py` | request_review.py --apply performs a GitHub write: it POSTs an issue comment carrying the review-trigger phrase. It is a mutating helper despite its read-sounding name. | +| `prune.removes-worktrees-via-git` | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | prune_babysit_worktrees.py --apply runs `git worktree remove`, a destructive local filesystem operation. Its dry-run default is what makes the flagless form safe, not anything about the name. | +| `snapshot.write-state-gates-the-only-write` | `skills/babysit-prs/scripts/pr_queue_snapshot.py` | pr_queue_snapshot.py's single write is the local snapshot, and --write-state is the only thing that reaches it. Nothing on any path writes to GitHub, so a flagless invocation is a pure read of the queue. | +| `merge.wrapper-filters-unpinned-head-in-bash` | `bin/source-control-babysit-merge` | The merge wrapper's refusal is a bash argument loop over "$@" that exits before the interpreter shim is sourced -- it is not, and must not become, an argparse flag on the Python side. | +| `resolve.wrapper-carries-no-filter` | `bin/source-control-babysit-resolve-thread` | ASYMMETRY, stated at the source: the resolve wrapper contains no argument filter loop. Adding one here without a matching row is the drift this contract exists to catch. | + +## Documented command lines + +Documents that spell out copyable `bin/`-path wrapper commands. Every wrapper path is checked to resolve and every flag against the backing CLI's own parser, so a renamed or removed flag fails here instead of in an operator's terminal. + +| ID | Document | Claim | +| --- | --- | --- | +| `safety.pinned-command-degradation` | `skills/babysit-prs/reference/safety.md` | reference/safety.md hard-codes fully-argument-pinned bin/-path wrapper command lines for the operator to run when the runtime denies a mutation the gate already proved ready. Every wrapper path in it resolves, and every flag it names is one the backing CLI's parser accepts. | +| `orchestration.worker-dispatch-commands` | `skills/babysit-prs/reference/orchestration.md` | reference/orchestration.md spells out the same bin/-path wrapper commands for the dispatched-worker and thread-resolve paths. It is a second copy of the same argument shapes and drifts independently of safety.md. | + +## Not covered here + +- Flag names, types, and defaults are not catalogued. A behavior row exercises the flags it names; a rename or default change elsewhere is not detected. See the deferral note in `scripts/tests/guard_contract.py`. +- Anything requiring a live GitHub response. Every row above runs with no network access, which is what keeps the suite free of a `gh` stub. diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index 72bc142f17..74ca8dd95d 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -317,6 +317,13 @@ the wrapper gate has already proven ready and in-tier. That denial is an environ ceiling this skill's own contract has no authority over — a normal, expected outcome to plan for, not a bug in this skill, a stalled worker, or a reason to retry with broader permissions. +Configuring that host layer means deciding which of this lane's entry points mutate, which flags +gate which guard, and where each refusal is enforced. Those facts are in +[reference/guard-contract.md](guard-contract.md), generated from the table +`scripts/tests/test_guards.py` executes against the real entry points — so a rule written against +a row cannot silently outlive the guard it cites. Cite a row ID; do not restate the behavior in +the consuming configuration. + ### Pinned-Command Degradation When the runtime denies a guarded mutation that this skill's own gate already proved ready — 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 new file mode 100644 index 0000000000..403738442c --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/guard_contract.py @@ -0,0 +1,1092 @@ +"""The guard contract: every lane fact a consumer is allowed to rely on. + +This module is the single source of truth for two artifacts that must never +disagree: the executable assertions in `test_guards.py` and the citable prose in +`../reference/guard-contract.md`. A consumer configuring a host permission +classifier cites the markdown; CI proves the markdown is what the code does. + +Each row carries the prose claim it backs, so a changed guard predicate fails +with a message naming the downstream documentation claim that just became false +rather than `AssertionError: 3 != 2`. + +Regenerate the markdown after editing any table: + + python tests/guard_contract.py --emit + +Source anchors are `file.py::symbol`, never line numbers -- a line number in a +drift-detection artifact is the drift it exists to catch. + +DEFERRED: an argparse flag catalogue (every flag's name, type, and default per +entry point) would additionally catch a renamed flag or a changed default that +no behavior row exercises. It needs a `build_parser()` extraction across all +nine entry points, whose parsers are built inside `main()`. Trigger to build it: +the first time a flag rename or default change ships without a matching update +here. Tracked on issue #1265. +""" + +from __future__ import annotations + +import argparse +import pathlib +import sys +from dataclasses import dataclass, field + +PLUGIN_ROOT = pathlib.Path(__file__).resolve().parents[4] +SCRIPTS = pathlib.Path(__file__).resolve().parent.parent +GENERATED_DOC = SCRIPTS.parent / "reference" / "guard-contract.md" + +MERGE_CLI = "skills/babysit-prs/scripts/babysit_merge.py" +RESOLVE_CLI = "skills/babysit-prs/scripts/babysit_resolve_thread.py" +LEASE_CLI = "skills/babysit-prs/scripts/manage_babysit_lease.py" +LEDGER_CLI = "skills/babysit-prs/scripts/manage_feedback_ledger.py" +PRUNE_CLI = "skills/babysit-prs/scripts/prune_babysit_worktrees.py" +REFRESH_CLI = "skills/babysit-prs/scripts/refresh_pr_branch.py" +REVIEW_CLI = "skills/babysit-prs/scripts/request_review.py" +SNAPSHOT_CLI = "skills/babysit-prs/scripts/pr_queue_snapshot.py" +FINDINGS_CLI = "skills/babysit-prs/scripts/babysit_findings.py" +MERGE_WRAPPER = "bin/source-control-babysit-merge" +RESOLVE_WRAPPER = "bin/source-control-babysit-resolve-thread" + +# Where a refusal is enforced. The distinction is observable: a bash-wrapper +# refusal never reaches Python, so it prints plain text to stderr and emits no +# JSON; a python-cli refusal emits the engine's JSON envelope on stdout. +BASH_WRAPPER = "bash-wrapper" +PYTHON_CLI = "python-cli" + + +def plugin_path(relative: str) -> pathlib.Path: + return PLUGIN_ROOT / relative + + +@dataclass(frozen=True) +class Refusal: + """A guard that rejects on argument shape alone, before any network call.""" + + id: str + claim: str + entry_point: str + argv: tuple[str, ...] + exit_code: int + error_contains: tuple[str, ...] + refused_by: str + enforced_at: str + + +@dataclass(frozen=True) +class Predicate: + """A guard whose condition is a boolean over runtime API data, not flags. + + No argument shape can reach these -- they decide per fetched thread -- so + they are asserted by calling the classifier directly with a projected + thread record. + """ + + id: str + claim: str + enforced_at: str + thread: dict[str, object] + flags: dict[str, bool] + expected: str + + +@dataclass(frozen=True) +class Effect: + """An observable filesystem effect, run offline against a temp state dir. + + These exist because a flag's name does not say whether an invocation + mutates: `manage_babysit_lease.py --apply` reads "reap only", yet `acquire` + writes a lease file with no `--apply` at all. + """ + + id: str + claim: str + entry_point: str + argv: tuple[str, ...] + fixture: str + state_changes: bool + exit_code: int + + +@dataclass(frozen=True) +class Mechanism: + """A source-level claim about HOW an entry point mutates. + + A consumer reasoning about the wrong mechanism writes the wrong permission + rule -- `refresh_pr_branch.py` never pushes, so a push guard is irrelevant + to it. + """ + + id: str + claim: str + entry_point: str + must_contain: tuple[str, ...] + must_not_contain: tuple[str, ...] = () + + +@dataclass(frozen=True) +class EntryPoint: + """The mutating / read-only classification a permission rule may assume.""" + + path: str + wrapper: str | None + mutation: str + mutates_what: str + gate: str + claim: str + backed_by: tuple[str, ...] = field(default_factory=tuple) + + +@dataclass(frozen=True) +class DocCommandSource: + """A document that spells out wrapper command lines a reader may copy. + + Every `bin/`-path wrapper command fenced in these files is validated: the + wrapper file exists, and every flag it names is a flag the backing CLI's + parser actually accepts. + """ + + id: str + claim: str + doc: str + + +# -------------------------------------------------------------------------- +# Refusals -- fail-closed guards, asserted by invoking the entry point. +# -------------------------------------------------------------------------- + +REFUSALS: tuple[Refusal, ...] = ( + Refusal( + id="merge.allowlist-absent", + claim=( + "The merge gate is fail-closed on scope: with no --allowed-owners it " + "refuses every PR at exit 3 rather than defaulting to the current repo's owner." + ), + entry_point=MERGE_CLI, + argv=("owner/repo#1",), + exit_code=3, + error_contains=("allowed-owners",), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="merge.owner-out-of-scope", + claim=( + "An owner outside --allowed-owners is refused at exit 3, so a permission rule " + "may assume the gate never acts on a repository the operator did not name." + ), + entry_point=MERGE_CLI, + argv=("owner/repo#1", "--allowed-owners", "someone-else"), + exit_code=3, + error_contains=(), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="merge.owner-check-precedes-self-login-resolution", + claim=( + "The owner check runs before '@me' self-login resolution, which is a network " + "call: an out-of-scope owner refuses at exit 3 with no gh invocation at all." + ), + entry_point=MERGE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "someone-else", + "--self-logins", + "@me", + ), + exit_code=3, + error_contains=(), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="merge.short-expected-head", + claim=( + "--expected-head must be a long-enough SHA prefix; a short value is a usage " + "error at exit 2, never a loose prefix match against the live head." + ), + entry_point=MERGE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--merge", + "--expected-head", + "abc", + ), + exit_code=2, + error_contains=("expected-head",), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="merge.malformed-ref", + claim=( + "The PR reference must parse as owner/repo#number or a PR URL; anything else " + "is a usage error at exit 2." + ), + entry_point=MERGE_CLI, + argv=("not-a-ref", "--allowed-owners", "owner"), + exit_code=2, + error_contains=(), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="merge.autopilot-tier-without-required-sets", + claim=( + "The autopilot merge tier is fail-closed: the umbrella flag alone, with none " + "of --lane-logins, --approver-bot-logins, or --block-labels, refuses at exit 3 " + "before any network access." + ), + entry_point=MERGE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--autopilot-merge-tier", + ), + exit_code=3, + error_contains=("--lane-logins", "--approver-bot-logins", "--block-labels"), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="merge.autopilot-tier-partial-config", + claim=( + "A partially configured autopilot tier still refuses at exit 3, naming only " + "the missing set -- there is no partial-credit tier." + ), + entry_point=MERGE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--autopilot-merge-tier", + "--lane-logins", + "lane", + "--approver-bot-logins", + "bot", + ), + exit_code=3, + error_contains=("--block-labels",), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="merge.tier-params-without-umbrella", + claim=( + "Tier parameter sets without --autopilot-merge-tier are a usage error at " + "exit 2, never a silent no-op that reads as configured." + ), + entry_point=MERGE_CLI, + argv=("owner/repo#1", "--allowed-owners", "owner", "--lane-logins", "lane"), + exit_code=2, + error_contains=("--autopilot-merge-tier",), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="merge.unpinned-head-refused-by-wrapper", + claim=( + "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." + ), + entry_point=MERGE_WRAPPER, + argv=("owner/repo#1", "--merge", "--allow-unpinned-head"), + exit_code=2, + error_contains=("--allow-unpinned-head",), + refused_by=BASH_WRAPPER, + enforced_at="bin/source-control-babysit-merge (argument filter loop)", + ), + Refusal( + id="merge.wrapper-reaches-failclosed-cli", + claim=( + "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." + ), + entry_point=MERGE_WRAPPER, + argv=("owner/repo#1",), + exit_code=3, + error_contains=("allowed-owners",), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), + Refusal( + id="resolve.allowlist-absent", + claim=( + "The thread resolver is fail-closed on scope the same way the merge gate is: " + "no --allowed-owners means exit 3." + ), + entry_point=RESOLVE_CLI, + argv=("owner/repo#1",), + exit_code=3, + error_contains=(), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), + Refusal( + id="resolve.count-pin-without-thread-id", + claim=( + "--expected-comment-count without --thread-id is a usage error at exit 2: a " + "pin with no thread to pin would silently fall through to resolving every " + "eligible thread." + ), + entry_point=RESOLVE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--expected-comment-count", + "3", + ), + exit_code=2, + error_contains=("thread-id",), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), + Refusal( + id="resolve.last-updated-pin-without-thread-id", + claim=( + "--expected-last-updated without --thread-id is a usage error at exit 2, for " + "the same silent-bulk-fallthrough reason as the count pin." + ), + entry_point=RESOLVE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--expected-last-updated", + "2026-07-17T10:00:00Z", + ), + exit_code=2, + error_contains=("thread-id",), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), + Refusal( + id="resolve.thread-id-without-pins", + claim=( + "A --thread-id resolve requires BOTH TOCTOU pins; supplying neither is a " + "usage error at exit 2 naming both missing flags." + ), + entry_point=RESOLVE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--resolve", + "--thread-id", + "PRRT_abc", + ), + exit_code=2, + error_contains=("--expected-comment-count", "--expected-last-updated"), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), + Refusal( + id="resolve.autonomous-bulk-refused", + claim=( + "--autonomous --resolve without --thread-id is refused at exit 2: --autonomous " + "means SINGLE PINNED THREAD, never bulk. A worker's own push marks a thread " + "isOutdated, so a bulk autonomous resolve would clear threads with no proof " + "the finding was addressed." + ), + entry_point=RESOLVE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--autonomous", + "--resolve", + ), + exit_code=2, + error_contains=("thread-id", "bulk-resolve"), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), + Refusal( + id="resolve.autonomous-allow-unpinned-refused", + claim=( + "There is no unpinned autonomous resolve: --allow-unpinned-thread is an " + "interactive-only override and is refused at exit 2 under --autonomous even " + "with a single --thread-id." + ), + entry_point=RESOLVE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--autonomous", + "--resolve", + "--thread-id", + "PRRT_abc", + "--allow-unpinned-thread", + ), + exit_code=2, + error_contains=("allow-unpinned-thread",), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), + Refusal( + id="resolve.wrapper-reaches-failclosed-cli", + claim=( + "The resolve wrapper is a pure passthrough: no --allowed-owners refuses at " + "exit 3 from the CLI, unchanged by the wrapper." + ), + entry_point=RESOLVE_WRAPPER, + argv=("owner/repo#1",), + exit_code=3, + error_contains=(), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), + Refusal( + id="resolve.wrapper-filters-nothing", + claim=( + "ASYMMETRY: unlike the merge wrapper, the resolve wrapper filters no argument. " + "Its --allow-unpinned-thread refusal comes from Python -- proven by the JSON " + "envelope on stdout -- so an operator may not assume a bash-level refusal " + "exists on this entry point just because one exists on the other." + ), + entry_point=RESOLVE_WRAPPER, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--autonomous", + "--resolve", + "--thread-id", + "PRRT_abc", + "--allow-unpinned-thread", + ), + exit_code=2, + error_contains=("allow-unpinned-thread",), + refused_by=PYTHON_CLI, + enforced_at="babysit_resolve_thread.py::main", + ), + Refusal( + id="ledger.apply-requires-lease-token", + claim=( + "A ledger write is lease-gated: --apply without --lease-token refuses at " + "exit 2 rather than writing an unowned entry." + ), + entry_point=LEDGER_CLI, + argv=( + "dispose", + "--pr", + "owner/repo#1", + "--expected-head-sha", + "0123456789abcdef0123456789abcdef01234567", + "--feedback-id", + "F1", + "--reason", + "contract-row", + "--state-dir", + "{state_dir}", + "--apply", + ), + exit_code=2, + error_contains=("lease token",), + refused_by=PYTHON_CLI, + enforced_at="manage_feedback_ledger.py::main", + ), + Refusal( + id="prune.root-is-required", + claim=( + "prune_babysit_worktrees.py has no default root: omitting --root is a usage " + "error at exit 2, so it can never sweep an unintended directory." + ), + entry_point=PRUNE_CLI, + argv=("--state-dir", "{state_dir}"), + exit_code=2, + error_contains=("--root",), + refused_by=PYTHON_CLI, + enforced_at="prune_babysit_worktrees.py::main", + ), + Refusal( + id="lease.state-dir-is-required", + claim=( + "State-dir resolution is flag-only with no environment fallback: omitting " + "--state-dir is a usage error at exit 2." + ), + entry_point=LEASE_CLI, + argv=("reap",), + exit_code=2, + error_contains=("--state-dir",), + refused_by=PYTHON_CLI, + enforced_at="manage_babysit_lease.py::main", + ), + Refusal( + id="lease.scope-required-for-non-reap", + claim=( + "--scope is required for acquire, heartbeat, and release; only reap runs " + "without it." + ), + entry_point=LEASE_CLI, + argv=("acquire", "--state-dir", "{state_dir}"), + exit_code=2, + error_contains=("--scope",), + refused_by=PYTHON_CLI, + enforced_at="manage_babysit_lease.py::main", + ), +) + + +# -------------------------------------------------------------------------- +# Predicates -- guards over runtime API data, asserted against the classifier. +# -------------------------------------------------------------------------- + +CLASSIFY_ANCHOR = "babysit_resolve_thread.py::classify" + + +def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, object]: + return {"isResolved": resolved, "botOnly": bot_only, "isOutdated": outdated} + + +PREDICATES: tuple[Predicate, ...] = ( + Predicate( + id="classify.human-thread-refused-by-default", + claim=( + "Bot-only is a bright line, not a heuristic: a thread with ANY human " + "participant is skipped whenever --include-human is absent, regardless of " + "every other flag." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread(resolved=False, bot_only=False, outdated=True), + flags={"autonomous": False, "only_outdated": False, "include_human": False}, + expected="skipped-human-thread", + ), + Predicate( + id="classify.include-human-opts-in", + claim=( + "--include-human is the ONLY thing that lifts the bot-only line. A permission " + "rule asserting 'bot threads only' is false for any invocation carrying it." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread(resolved=False, bot_only=False, outdated=True), + flags={"autonomous": False, "only_outdated": False, "include_human": True}, + expected="eligible", + ), + Predicate( + id="classify.autonomous-requires-outdated", + claim=( + "--autonomous requires the deterministic isOutdated 'addressed' signal: a " + "still-current bot thread is skipped so an unattended worker cannot resolve " + "a live finding and self-satisfy the merge gate." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread(resolved=False, bot_only=True, outdated=False), + flags={"autonomous": True, "only_outdated": False, "include_human": False}, + expected="skipped-not-outdated", + ), + Predicate( + id="classify.only-outdated-requires-outdated", + claim=( + "--only-outdated imposes the same isOutdated requirement as --autonomous, " + "independently of it." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread(resolved=False, bot_only=True, outdated=False), + flags={"autonomous": False, "only_outdated": True, "include_human": False}, + expected="skipped-not-outdated", + ), + Predicate( + id="classify.interactive-does-not-require-outdated", + claim=( + "THE CLAIM CONSUMERS GET WRONG: 'resolves only outdated bot threads' is TRUE " + "ONLY under --autonomous or --only-outdated. With neither flag, a current " + "(not outdated) bot thread is eligible." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread(resolved=False, bot_only=True, outdated=False), + flags={"autonomous": False, "only_outdated": False, "include_human": False}, + expected="eligible", + ), + Predicate( + id="classify.include-human-does-not-lift-outdated", + claim=( + "--include-human lifts only the bot-only line. Under --autonomous the " + "isOutdated requirement still applies, so the two guards are independent and " + "neither flag opens the other." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread(resolved=False, bot_only=False, outdated=False), + flags={"autonomous": True, "only_outdated": False, "include_human": True}, + expected="skipped-not-outdated", + ), + Predicate( + id="classify.already-resolved-precedes-every-guard", + claim=( + "An already-resolved thread is skipped before any other guard is consulted, " + "so a re-run never re-resolves and never reports a human thread it declined." + ), + enforced_at=CLASSIFY_ANCHOR, + thread=_thread(resolved=True, bot_only=False, outdated=False), + flags={"autonomous": True, "only_outdated": False, "include_human": False}, + expected="skipped-already-resolved", + ), +) + + +# -------------------------------------------------------------------------- +# Effects -- what actually touches disk, run offline against a temp state dir. +# -------------------------------------------------------------------------- + +EMPTY_STATE = "empty" +EXPIRED_WORKER_LEASE = "expired-worker-lease" + +EFFECTS: tuple[Effect, ...] = ( + Effect( + id="lease.acquire-writes-without-apply", + claim=( + "THE TRAP: manage_babysit_lease.py's --apply help reads 'reap only', but " + "acquire writes a lease file unconditionally, with no --apply and no network. " + "A permission rule that derives read-only from flag names classifies acquire " + "wrong." + ), + entry_point=LEASE_CLI, + argv=( + "acquire", + "--scope", + "worker", + "--pr", + "owner/repo#1", + "--state-dir", + "{state_dir}", + ), + fixture=EMPTY_STATE, + state_changes=True, + exit_code=0, + ), + Effect( + id="lease.reap-without-apply-is-a-dry-run", + claim=( + "--apply gates reap alone: a reap without it reports which lease files are " + "expired and deletes none of them." + ), + entry_point=LEASE_CLI, + argv=("reap", "--state-dir", "{state_dir}"), + fixture=EXPIRED_WORKER_LEASE, + state_changes=False, + exit_code=0, + ), + Effect( + id="lease.reap-with-apply-deletes", + claim=( + "reap --apply deletes expired worker lease files. This is the one action on " + "this script the --apply flag actually gates." + ), + entry_point=LEASE_CLI, + argv=("reap", "--state-dir", "{state_dir}", "--apply"), + fixture=EXPIRED_WORKER_LEASE, + state_changes=True, + exit_code=0, + ), +) + + +# -------------------------------------------------------------------------- +# Mechanisms -- HOW an entry point mutates, asserted against its source. +# -------------------------------------------------------------------------- + +MECHANISMS: tuple[Mechanism, ...] = ( + Mechanism( + id="refresh.uses-server-side-update-branch", + claim=( + "refresh_pr_branch.py never pushes. It asks GitHub to update the branch " + "server-side via PUT repos/{repo}/pulls/{number}/update-branch, pinned with " + "expected_head_sha. A consumer reasoning about push guards for this script is " + "reasoning about the wrong mechanism -- it needs no local write access to the " + "branch at all." + ), + entry_point=REFRESH_CLI, + must_contain=('pulls/{number}/update-branch', 'expected_head_sha='), + must_not_contain=('"push"', "'push'"), + ), + Mechanism( + id="review.posts-a-comment", + claim=( + "request_review.py --apply performs a GitHub write: it POSTs an issue comment " + "carrying the review-trigger phrase. It is a mutating helper despite its " + "read-sounding name." + ), + entry_point=REVIEW_CLI, + must_contain=('"--method"', '"POST"'), + ), + Mechanism( + id="prune.removes-worktrees-via-git", + claim=( + "prune_babysit_worktrees.py --apply runs `git worktree remove`, a destructive " + "local filesystem operation. Its dry-run default is what makes the flagless " + "form safe, not anything about the name." + ), + entry_point=PRUNE_CLI, + must_contain=('"worktree", "remove"',), + ), + Mechanism( + id="snapshot.write-state-gates-the-only-write", + claim=( + "pr_queue_snapshot.py's single write is the local snapshot, and --write-state " + "is the only thing that reaches it. Nothing on any path writes to GitHub, so " + "a flagless invocation is a pure read of the queue." + ), + entry_point=SNAPSHOT_CLI, + must_contain=("if args.write_state:", "save_state("), + ), + Mechanism( + id="merge.wrapper-filters-unpinned-head-in-bash", + claim=( + "The merge wrapper's refusal is a bash argument loop over \"$@\" that exits " + "before the interpreter shim is sourced -- it is not, and must not become, " + "an argparse flag on the Python side." + ), + entry_point=MERGE_WRAPPER, + must_contain=('for arg in "$@"', "--allow-unpinned-head"), + ), + Mechanism( + id="resolve.wrapper-carries-no-filter", + claim=( + "ASYMMETRY, stated at the source: the resolve wrapper contains no argument " + "filter loop. Adding one here without a matching row is the drift this " + "contract exists to catch." + ), + entry_point=RESOLVE_WRAPPER, + must_contain=("babysit_python",), + must_not_contain=('for arg in "$@"',), + ), +) + + +# -------------------------------------------------------------------------- +# Entry points -- the mutating / read-only catalogue a permission rule cites. +# -------------------------------------------------------------------------- + +READ_ONLY = "read-only" +MUTATING = "mutating" +CONDITIONAL = "conditionally mutating" + +ENTRY_POINTS: tuple[EntryPoint, ...] = ( + EntryPoint( + path=MERGE_CLI, + wrapper=MERGE_WRAPPER, + mutation=CONDITIONAL, + mutates_what="merges the PR on GitHub; writes queue state", + gate="--merge (absent: readiness check only, exit 0 ready / 10 not ready)", + claim=( + "Without --merge this is a readiness reporter. With it, the TOCTOU guard " + "requires --expected-head unless --allow-unpinned-head is passed -- and the " + "bin/ wrapper refuses that override outright." + ), + backed_by=( + "merge.allowlist-absent", + "merge.unpinned-head-refused-by-wrapper", + "merge.wrapper-filters-unpinned-head-in-bash", + ), + ), + EntryPoint( + path=RESOLVE_CLI, + wrapper=RESOLVE_WRAPPER, + mutation=CONDITIONAL, + mutates_what="marks review threads resolved on GitHub", + gate="--resolve (absent: classification report only)", + claim=( + "Which threads it may touch is decided per fetched thread by the classifier, " + "not by the argument shape -- see the predicate rows. The wrapper adds no " + "refusal of its own." + ), + backed_by=( + "resolve.allowlist-absent", + "resolve.autonomous-bulk-refused", + "classify.interactive-does-not-require-outdated", + "resolve.wrapper-carries-no-filter", + ), + ), + EntryPoint( + path=LEASE_CLI, + wrapper=None, + mutation=MUTATING, + mutates_what="writes, renews, and deletes lease files under --state-dir", + gate=( + "none for acquire / heartbeat / release -- they write unconditionally; " + "--apply gates reap's deletion ONLY" + ), + claim=( + "Local filesystem writes only, no GitHub write. The --apply flag does not " + "make this script read-only in its other three actions." + ), + backed_by=( + "lease.acquire-writes-without-apply", + "lease.reap-without-apply-is-a-dry-run", + ), + ), + EntryPoint( + path=LEDGER_CLI, + wrapper=None, + mutation=CONDITIONAL, + mutates_what="records feedback dispositions and fix rounds in queue state", + gate="--apply, additionally requiring --lease-token", + claim=( + "Local state only, no GitHub write. The lease token requirement means an " + "--apply invocation still refuses without proof of lease ownership." + ), + backed_by=("ledger.apply-requires-lease-token",), + ), + EntryPoint( + path=PRUNE_CLI, + wrapper=None, + mutation=CONDITIONAL, + mutates_what="runs `git worktree remove` on eligible worktrees under --root", + gate="--apply (absent: dry run)", + claim=( + "The most destructive local helper in the lane. --root is required, so it " + "cannot sweep a directory the caller did not name." + ), + backed_by=("prune.root-is-required", "prune.removes-worktrees-via-git"), + ), + EntryPoint( + path=REFRESH_CLI, + wrapper=None, + mutation=CONDITIONAL, + mutates_what="asks GitHub to update the PR branch server-side; writes queue state", + gate="--apply", + claim=( + "Mutates the PR branch on GitHub WITHOUT pushing: the update is a pinned " + "server-side update-branch API call." + ), + backed_by=("refresh.uses-server-side-update-branch",), + ), + EntryPoint( + path=REVIEW_CLI, + wrapper=None, + mutation=CONDITIONAL, + mutates_what="POSTs an issue comment requesting an AI re-review; writes queue state", + gate="--apply", + claim="A GitHub write despite the read-sounding name.", + backed_by=("review.posts-a-comment",), + ), + EntryPoint( + path=SNAPSHOT_CLI, + wrapper=None, + mutation=CONDITIONAL, + mutates_what="writes the queue snapshot to local state", + gate="--write-state (absent: reports the snapshot without persisting it)", + claim=( + "No GitHub write on any path. The only mutation is the local snapshot file." + ), + backed_by=("snapshot.write-state-gates-the-only-write",), + ), + EntryPoint( + path=FINDINGS_CLI, + wrapper=None, + mutation=READ_ONLY, + mutates_what="nothing", + gate="n/a", + claim="Pure classification over supplied input; no write of any kind.", + backed_by=(), + ), +) + + +# -------------------------------------------------------------------------- +# Documents that spell out copyable wrapper command lines. +# -------------------------------------------------------------------------- + +DOC_COMMAND_SOURCES: tuple[DocCommandSource, ...] = ( + DocCommandSource( + id="safety.pinned-command-degradation", + claim=( + "reference/safety.md hard-codes fully-argument-pinned bin/-path wrapper " + "command lines for the operator to run when the runtime denies a mutation the " + "gate already proved ready. Every wrapper path in it resolves, and every flag " + "it names is one the backing CLI's parser accepts." + ), + doc="skills/babysit-prs/reference/safety.md", + ), + DocCommandSource( + id="orchestration.worker-dispatch-commands", + claim=( + "reference/orchestration.md spells out the same bin/-path wrapper commands " + "for the dispatched-worker and thread-resolve paths. It is a second copy of " + "the same argument shapes and drifts independently of safety.md." + ), + doc="skills/babysit-prs/reference/orchestration.md", + ), +) + + +# -------------------------------------------------------------------------- +# Markdown emission. +# -------------------------------------------------------------------------- + +WRAPPER_BACKING_CLI = { + MERGE_WRAPPER: MERGE_CLI, + RESOLVE_WRAPPER: RESOLVE_CLI, +} + +_PREAMBLE = """# Guard contract + + + +What a host permission classifier, an orchestration prompt, or a downstream +consumer may assume about this lane's entry points. Every statement below is +executed as an assertion by `scripts/tests/test_guards.py`; a guard change that +falsifies one fails CI with a message naming the claim. + +Cite a row by its ID. IDs are stable; rows are removed only when the behavior is. + +Source anchors are `file::symbol`, deliberately not line numbers. +""" + + +def _cell(text: str) -> str: + return " ".join(text.split()).replace("|", "\\|") + + +def _argv(argv: tuple[str, ...]) -> str: + return "`" + " ".join(argv) + "`" if argv else "(none)" + + +def render_markdown() -> str: + lines = [_PREAMBLE.rstrip(), ""] + + lines += [ + "## Entry points: what mutates", + "", + "`gate` is the condition under which the mutation happens at all. A blank" + " wrapper column means the entry point has no `bin/` wrapper and is invoked" + " through the interpreter.", + "", + "| Entry point | Wrapper | Class | Mutates | Gate | Claim | Backed by |", + "| --- | --- | --- | --- | --- | --- | --- |", + ] + for entry in ENTRY_POINTS: + wrapper = f"`{entry.wrapper}`" if entry.wrapper else "--" + backed = ", ".join(f"`{row}`" for row in entry.backed_by) or "--" + lines.append( + f"| `{entry.path}` | {wrapper} | {entry.mutation} | {_cell(entry.mutates_what)} " + f"| {_cell(entry.gate)} | {_cell(entry.claim)} | {backed} |" + ) + + lines += [ + "", + "## Refusals: guards that fire on argument shape alone", + "", + "Each row is executed. `Refused by` says which layer rejected the invocation." + " On a `bin/` entry point the distinction is asserted, using the observable" + " discriminator: a bash-wrapper refusal never reaches the interpreter, so it" + " emits plain stderr and no JSON envelope. On a CLI entry point the layer is" + " always `python-cli` and only the exit code and message are asserted.", + "", + "| ID | Entry point | Invocation | Exit | Refused by | Error names | Enforced at | Claim |", + "| --- | --- | --- | --- | --- | --- | --- | --- |", + ] + for row in REFUSALS: + names = ", ".join(f"`{token}`" for token in row.error_contains) or "--" + lines.append( + f"| `{row.id}` | `{row.entry_point}` | {_argv(row.argv)} | {row.exit_code} " + f"| {row.refused_by} | {names} | `{row.enforced_at}` | {_cell(row.claim)} |" + ) + + lines += [ + "", + "## Predicates: guards over runtime data, not flags", + "", + "These decide per fetched thread, so no argument shape can express them and no" + " argparse introspection can recover them. `thread` is the projected thread" + " record; `flags` are the classifier's keyword arguments.", + "", + "| ID | Thread | Flags | Result | Enforced at | Claim |", + "| --- | --- | --- | --- | --- | --- |", + ] + for row in PREDICATES: + thread = ", ".join(f"{k}={v}" for k, v in row.thread.items()) + flags = ", ".join(f"{k}={v}" for k, v in row.flags.items()) + lines.append( + f"| `{row.id}` | {thread} | {flags} | `{row.expected}` " + f"| `{row.enforced_at}` | {_cell(row.claim)} |" + ) + + lines += [ + "", + "## Effects: what reaches disk", + "", + "Executed offline against a throwaway state directory seeded with `Fixture`." + " `State changes` is the observed before/after difference of that directory," + " not a reading of the flag names.", + "", + "| ID | Entry point | Invocation | Fixture | Exit | State changes | Claim |", + "| --- | --- | --- | --- | --- | --- | --- |", + ] + for row in EFFECTS: + lines.append( + f"| `{row.id}` | `{row.entry_point}` | {_argv(row.argv)} | {row.fixture} " + f"| {row.exit_code} | {'yes' if row.state_changes else 'no'} " + f"| {_cell(row.claim)} |" + ) + + lines += [ + "", + "## Mechanisms: how a mutation is performed", + "", + "Asserted against the entry point's source, because the mechanism -- not just" + " the fact of mutation -- decides which permission rule is the relevant one.", + "", + "| ID | Entry point | Claim |", + "| --- | --- | --- |", + ] + for row in MECHANISMS: + lines.append(f"| `{row.id}` | `{row.entry_point}` | {_cell(row.claim)} |") + + lines += [ + "", + "## Documented command lines", + "", + "Documents that spell out copyable `bin/`-path wrapper commands. Every wrapper" + " path is checked to resolve and every flag against the backing CLI's own" + " parser, so a renamed or removed flag fails here instead of in an operator's" + " terminal.", + "", + "| ID | Document | Claim |", + "| --- | --- | --- |", + ] + for row in DOC_COMMAND_SOURCES: + lines.append(f"| `{row.id}` | `{row.doc}` | {_cell(row.claim)} |") + + lines += [ + "", + "## Not covered here", + "", + "- Flag names, types, and defaults are not catalogued. A behavior row exercises" + " the flags it names; a rename or default change elsewhere is not detected. See" + " the deferral note in `scripts/tests/guard_contract.py`.", + "- Anything requiring a live GitHub response. Every row above runs with no" + " network access, which is what keeps the suite free of a `gh` stub.", + "", + ] + return "\n".join(lines) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--emit", + action="store_true", + help="write the generated markdown to reference/guard-contract.md", + ) + args = parser.parse_args() + rendered = render_markdown() + if args.emit: + GENERATED_DOC.write_text(rendered, encoding="utf-8", newline="\n") + print(f"wrote {GENERATED_DOC}") + else: + sys.stdout.write(rendered) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py index a4dd5062cf..d83b520c4e 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py @@ -1,161 +1,387 @@ -"""Security-boundary tests for the two guarded mutation CLIs. +"""Executes the guard contract in `guard_contract.py` against the real entry points. -Every assertion here exercises a refusal that must fire BEFORE any network call, -so the tests need no gh stub: a fail-closed guard rejects on argument shape -alone. These encode the trust contract -- an allowlist that is absent fails -closed, an ambiguous head pin is refused, a thread resolve without its TOCTOU -pins is refused -- that nothing else in the suite covers. +Every assertion here exercises a fact a consumer is told to rely on, and fails +with the prose claim that just became false rather than a bare exit-code +mismatch. The refusal rows need no gh stub: a fail-closed guard rejects on +argument shape alone, before any network call. The effect rows run offline +against a throwaway state directory, and the predicate rows call the classifier +directly because their conditions are boolean expressions over fetched API data +that no argument shape can express. + +The contract's markdown rendering in `../reference/guard-contract.md` is what +consumers cite; `GeneratedDocIsCurrent` is what keeps it honest. """ import json +import os import pathlib +import re +import shutil import subprocess import sys +import tempfile import unittest +from datetime import UTC, datetime, timedelta + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent)) +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) + +import babysit_resolve_thread # noqa: E402 +import guard_contract as contract # noqa: E402 -SCRIPTS = pathlib.Path(__file__).resolve().parent.parent -MERGE = SCRIPTS / "babysit_merge.py" -RESOLVE = SCRIPTS / "babysit_resolve_thread.py" +BASH = shutil.which("bash") -def run(script, *args): - proc = subprocess.run( - [sys.executable, str(script), *args], +def invoke(entry_point: str, argv: tuple[str, ...]) -> subprocess.CompletedProcess[str]: + """Run an entry point the way an operator would -- wrapper via bash, CLI via Python.""" + target = contract.plugin_path(entry_point) + command = ( + [BASH, str(target), *argv] + if entry_point.startswith("bin/") + else [sys.executable, str(target), *argv] + ) + return subprocess.run( + command, capture_output=True, text=True, + # A foreign cwd proves the wrappers' self-location, and keeps a stray + # repo-relative default from being read as a guard holding. + cwd=tempfile.gettempdir(), ) - payload = {} - if proc.stdout.strip(): - try: - payload = json.loads(proc.stdout) - except json.JSONDecodeError: - payload = {} - return proc.returncode, payload - - -class MergeGuardFailsClosed(unittest.TestCase): - def test_absent_allowlist_refuses_exit_3(self): - code, payload = run(MERGE, "owner/repo#1") - self.assertEqual(code, 3) - self.assertFalse(payload.get("inScope")) - self.assertIn("allowed-owners", payload.get("error", "")) - - def test_owner_out_of_scope_refuses_exit_3(self): - code, payload = run(MERGE, "owner/repo#1", "--allowed-owners", "someone-else") - self.assertEqual(code, 3) - self.assertFalse(payload.get("inScope")) - - def test_self_logins_at_me_refuses_out_of_scope_before_resolving(self): - # '@me' resolution is a network call; passing it with an out-of-scope - # owner must still refuse at exit 3 (owner check precedes resolution), - # so this runs with no gh available and must not hang or error. - code, payload = run( - MERGE, "owner/repo#1", "--allowed-owners", "someone-else", - "--self-logins", "@me", - ) - self.assertEqual(code, 3) - self.assertFalse(payload.get("inScope")) - def test_short_expected_head_is_usage_error_exit_2(self): - code, payload = run( - MERGE, "owner/repo#1", "--allowed-owners", "owner", - "--merge", "--expected-head", "abc", - ) - self.assertEqual(code, 2) - self.assertIn("expected-head", payload.get("error", "")) - - def test_malformed_ref_is_usage_error_exit_2(self): - code, _ = run(MERGE, "not-a-ref", "--allowed-owners", "owner") - self.assertEqual(code, 2) - - def test_autopilot_tier_without_required_sets_refuses_exit_3(self): - # The tier's fail-closed core: the umbrella flag alone, with none of its - # three required sets, refuses before any network access. - code, payload = run( - MERGE, "owner/repo#1", "--allowed-owners", "owner", - "--autopilot-merge-tier", - ) - self.assertEqual(code, 3) - error = payload.get("error", "") - for flag in ("--lane-logins", "--approver-bot-logins", "--block-labels"): - self.assertIn(flag, error) - - def test_autopilot_tier_partial_config_refuses_exit_3(self): - # Two of three supplied still refuses, naming only the missing set. - code, payload = run( - MERGE, "owner/repo#1", "--allowed-owners", "owner", - "--autopilot-merge-tier", "--lane-logins", "lane", - "--approver-bot-logins", "bot", + +def envelope(proc: subprocess.CompletedProcess[str]) -> dict[str, object]: + """The engine's JSON refusal envelope, or {} when the refusal never reached Python.""" + if not proc.stdout.strip(): + return {} + try: + parsed = json.loads(proc.stdout) + except json.JSONDecodeError: + return {} + return parsed if isinstance(parsed, dict) else {} + + +def state_fingerprint(state_dir: pathlib.Path) -> dict[str, bytes]: + """Durable state contents, ignoring the advisory .lock files every run creates.""" + return { + str(path.relative_to(state_dir)): path.read_bytes() + for path in sorted(state_dir.rglob("*")) + if path.is_file() and path.suffix != ".lock" + } + + +def seed(fixture: str, state_dir: pathlib.Path) -> None: + if fixture == contract.EMPTY_STATE: + return + if fixture == contract.EXPIRED_WORKER_LEASE: + # Acquire a real lease so the file shape stays the engine's, then age it + # past expiry -- a hand-written record would drift from the writer. + proc = invoke( + contract.LEASE_CLI, + ( + "acquire", + "--scope", + "worker", + "--pr", + "owner/repo#1", + "--state-dir", + str(state_dir), + ), ) - self.assertEqual(code, 3) - self.assertIn("--block-labels", payload.get("error", "")) - - def test_tier_params_without_umbrella_are_usage_error_exit_2(self): - # The parameter sets are meaningless without the umbrella flag; supplying - # them alone is a usage error, never a silent no-op. - code, payload = run( - MERGE, "owner/repo#1", "--allowed-owners", "owner", - "--lane-logins", "lane", + if proc.returncode != 0: + raise AssertionError(f"lease fixture setup failed: {proc.stderr}") + lease_path = pathlib.Path(str(json.loads(proc.stdout)["path"])) + record = json.loads(lease_path.read_text(encoding="utf-8")) + stale = (datetime.now(UTC) - timedelta(days=1)).isoformat() + record.update({"expires_at": stale, "updated_at": stale}) + lease_path.write_text(json.dumps(record), encoding="utf-8") + return + raise AssertionError(f"unknown fixture: {fixture}") + + +def because(row_id: str, claim: str, detail: str = "") -> str: + suffix = f"\n observed: {detail}" if detail else "" + return ( + f"\nGUARD CONTRACT ROW `{row_id}` NO LONGER HOLDS." + f"\n claim: {claim}" + f"\n Fix the guard, or update the row in scripts/tests/guard_contract.py and" + f" regenerate reference/guard-contract.md." + f"{suffix}" + ) + + +class RefusalsFireOnArgumentShape(unittest.TestCase): + def test_every_refusal_row(self) -> None: + for row in contract.REFUSALS: + with self.subTest(row=row.id): + if row.entry_point.startswith("bin/") and BASH is None: + self.skipTest("bash unavailable; wrapper rows need it") + with tempfile.TemporaryDirectory() as tmp: + argv = tuple(arg.format(state_dir=tmp) for arg in row.argv) + proc = invoke(row.entry_point, argv) + payload = envelope(proc) + message = str(payload.get("error", "")) + combined = "\n".join((message, proc.stdout, proc.stderr)) + self.assertEqual( + proc.returncode, + row.exit_code, + because(row.id, row.claim, f"exit {proc.returncode}: {combined[:400]}"), + ) + for token in row.error_contains: + self.assertIn( + token, combined, because(row.id, row.claim, combined[:400]) + ) + if row.entry_point.startswith("bin/"): + # The observable bash-vs-Python discriminator: a wrapper-level + # refusal never reaches the interpreter, so it emits no envelope. + if row.refused_by == contract.BASH_WRAPPER: + self.assertEqual( + payload, + {}, + because(row.id, row.claim, "a JSON envelope was emitted"), + ) + self.assertTrue( + proc.stderr.strip(), + because(row.id, row.claim, "no stderr from the wrapper"), + ) + else: + self.assertNotEqual( + payload, + {}, + because(row.id, row.claim, "no JSON envelope: bash refused"), + ) + + def test_scope_refusal_precedes_every_network_call(self) -> None: + # Belt-and-braces on the property that makes this suite stub-free: with + # PATH emptied of gh, the fail-closed rows must behave identically. + env = dict(os.environ, PATH=tempfile.gettempdir()) + proc = subprocess.run( + [sys.executable, str(contract.plugin_path(contract.MERGE_CLI)), "owner/repo#1"], + capture_output=True, + text=True, + env=env, ) - self.assertEqual(code, 2) - self.assertIn("--autopilot-merge-tier", payload.get("error", "")) + self.assertEqual(proc.returncode, 3, proc.stderr) + +class PredicatesHoldOverRuntimeData(unittest.TestCase): + def test_every_predicate_row(self) -> None: + for row in contract.PREDICATES: + with self.subTest(row=row.id): + observed = babysit_resolve_thread.classify(dict(row.thread), **row.flags) + self.assertEqual( + observed, + row.expected, + because(row.id, row.claim, f"classify returned {observed!r}"), + ) -class ResolveGuardFailsClosed(unittest.TestCase): - def test_absent_allowlist_refuses_exit_3(self): - code, payload = run(RESOLVE, "owner/repo#1") - self.assertEqual(code, 3) - self.assertFalse(payload.get("inScope")) + def test_anchor_symbol_exists(self) -> None: + # The anchors are the contract's substitute for line numbers; a rename + # must fail here rather than leave the doc pointing at nothing. + for row in contract.PREDICATES: + module, _, symbol = row.enforced_at.partition("::") + with self.subTest(row=row.id): + self.assertEqual(module, "babysit_resolve_thread.py") + self.assertTrue( + hasattr(babysit_resolve_thread, symbol), + because(row.id, row.claim, f"no symbol named {symbol!r}"), + ) - def test_count_pin_without_thread_id_is_usage_error(self): - code, payload = run( - RESOLVE, "owner/repo#1", "--allowed-owners", "owner", - "--expected-comment-count", "3", + +class EffectsReachDiskAsClaimed(unittest.TestCase): + def test_every_effect_row(self) -> None: + for row in contract.EFFECTS: + with self.subTest(row=row.id): + with tempfile.TemporaryDirectory() as tmp: + state_dir = pathlib.Path(tmp) + seed(row.fixture, state_dir) + before = state_fingerprint(state_dir) + argv = tuple(arg.format(state_dir=tmp) for arg in row.argv) + proc = invoke(row.entry_point, argv) + after = state_fingerprint(state_dir) + self.assertEqual( + proc.returncode, + row.exit_code, + because(row.id, row.claim, f"exit {proc.returncode}: {proc.stderr[:400]}"), + ) + self.assertEqual( + before != after, + row.state_changes, + because( + row.id, + row.claim, + f"state {'changed' if before != after else 'was untouched'}", + ), + ) + + +class MechanismsMatchTheSource(unittest.TestCase): + def test_every_mechanism_row(self) -> None: + for row in contract.MECHANISMS: + with self.subTest(row=row.id): + source = contract.plugin_path(row.entry_point).read_text(encoding="utf-8") + for token in row.must_contain: + self.assertIn( + token, source, because(row.id, row.claim, f"{token!r} is absent") + ) + for token in row.must_not_contain: + self.assertNotIn( + token, source, because(row.id, row.claim, f"{token!r} appeared") + ) + + +class EntryPointCatalogueIsComplete(unittest.TestCase): + def test_every_executable_script_is_classified(self) -> None: + # A new entry point must arrive with its mutation classification, or a + # consumer's permission rule silently has no row to cite for it. + catalogued = {entry.path for entry in contract.ENTRY_POINTS} + present = { + f"skills/babysit-prs/scripts/{path.name}" + for path in contract.SCRIPTS.glob("*.py") + if "__main__" in path.read_text(encoding="utf-8") + } + self.assertEqual( + present - catalogued, + set(), + "entry points with no guard-contract classification; add them to" + " ENTRY_POINTS in scripts/tests/guard_contract.py", ) - self.assertEqual(code, 2) - self.assertIn("thread-id", payload.get("error", "")) + self.assertEqual(catalogued - present, set(), "classified scripts that no longer exist") - def test_last_updated_pin_without_thread_id_is_usage_error(self): - code, payload = run( - RESOLVE, "owner/repo#1", "--allowed-owners", "owner", - "--expected-last-updated", "2026-07-17T10:00:00Z", + def test_every_wrapper_is_classified(self) -> None: + wrappers = { + f"bin/{path.name}" + for path in (contract.PLUGIN_ROOT / "bin").iterdir() + if path.is_file() and path.name.startswith("source-control-babysit-") + } + self.assertEqual( + wrappers, + {entry.wrapper for entry in contract.ENTRY_POINTS if entry.wrapper}, + "a babysit wrapper is missing from ENTRY_POINTS", ) - self.assertEqual(code, 2) - self.assertIn("thread-id", payload.get("error", "")) - def test_resolve_thread_id_without_pins_is_refused(self): - code, payload = run( - RESOLVE, "owner/repo#1", "--allowed-owners", "owner", - "--resolve", "--thread-id", "PRRT_abc", + def test_classifications_cite_only_real_rows(self) -> None: + known = {row.id for row in contract.REFUSALS} + known |= {row.id for row in contract.PREDICATES} + known |= {row.id for row in contract.EFFECTS} + known |= {row.id for row in contract.MECHANISMS} + for entry in contract.ENTRY_POINTS: + for row_id in entry.backed_by: + with self.subTest(entry=entry.path, row=row_id): + self.assertIn(row_id, known, "backed_by names a row that does not exist") + + def test_row_ids_are_unique(self) -> None: + ids = [ + row.id + for table in ( + contract.REFUSALS, + contract.PREDICATES, + contract.EFFECTS, + contract.MECHANISMS, + contract.DOC_COMMAND_SOURCES, + ) + for row in table + ] + self.assertEqual(sorted(ids), sorted(set(ids)), "row IDs are citation anchors") + + def test_every_mutating_entry_point_is_backed(self) -> None: + for entry in contract.ENTRY_POINTS: + if entry.mutation == contract.READ_ONLY: + continue + with self.subTest(entry=entry.path): + self.assertTrue( + entry.backed_by, + f"`{entry.path}` is classified {entry.mutation} with nothing asserting it", + ) + + +WRAPPER_COMMAND = re.compile( + r'bash "\$\{CLAUDE_PLUGIN_ROOT\}/(bin/source-control-babysit-[a-z-]+)"([^`]*)' +) + + +def documented_commands(text: str) -> list[tuple[str, str]]: + """Wrapper commands in a document, each paired with its argument tail. + + A command runs to the end of its inline-code span or fenced block, which + prose wraps across lines, so the tail is truncated at the first blank line + rather than the first newline. The truncation is done here instead of in the + pattern: expressing "not a backtick, and not a blank line" as an alternation + backtracks catastrophically on a document this size. + """ + return [ + (wrapper, " ".join(tail.split("\n\n", 1)[0].split())) + for wrapper, tail in WRAPPER_COMMAND.findall(text) + ] + + +class DocumentedCommandsMatchTheParsers(unittest.TestCase): + def _accepted_flags(self, cli: str) -> set[str]: + proc = subprocess.run( + [sys.executable, str(contract.plugin_path(cli)), "--help"], + capture_output=True, + text=True, ) - self.assertEqual(code, 2) - self.assertIn("expected-comment-count", payload.get("error", "")) - self.assertIn("expected-last-updated", payload.get("error", "")) - - def test_autonomous_bulk_resolve_without_thread_id_is_refused(self): - # The bug this guard closes: an unattended worker's own push marks a - # thread isOutdated, and a bulk (no --thread-id) autonomous resolve would - # clear it with no proof the finding was addressed. Refused before any - # network fetch -- the fix-closed contract the docs already describe. - code, payload = run( - RESOLVE, "owner/repo#1", "--allowed-owners", "owner", - "--autonomous", "--resolve", + self.assertEqual(proc.returncode, 0, f"{cli} --help failed: {proc.stderr}") + return set(re.findall(r"--[a-z0-9][a-z0-9-]*", proc.stdout)) + + def test_every_documented_wrapper_command(self) -> None: + accepted: dict[str, set[str]] = {} + for row in contract.DOC_COMMAND_SOURCES: + text = contract.plugin_path(row.doc).read_text(encoding="utf-8") + commands = documented_commands(text) + self.assertTrue( + commands, + because(row.id, row.claim, f"no wrapper command lines found in {row.doc}"), + ) + for wrapper, tail in commands: + with self.subTest(doc=row.doc, wrapper=wrapper): + self.assertTrue( + contract.plugin_path(wrapper).is_file(), + because(row.id, row.claim, f"{wrapper} does not exist"), + ) + cli = contract.WRAPPER_BACKING_CLI[wrapper] + if cli not in accepted: + accepted[cli] = self._accepted_flags(cli) + for flag in re.findall(r"(? None: + # A second document growing its own copy of a command line must be added + # to DOC_COMMAND_SOURCES, not left unchecked. + covered = {row.doc for row in contract.DOC_COMMAND_SOURCES} + skill_root = contract.PLUGIN_ROOT / "skills" / "babysit-prs" + naming = { + str(path.relative_to(contract.PLUGIN_ROOT)).replace("\\", "/") + for path in skill_root.rglob("*.md") + if WRAPPER_COMMAND.search(path.read_text(encoding="utf-8")) + } + self.assertEqual( + naming - covered, + set(), + "documents spelling out wrapper command lines with no DOC_COMMAND_SOURCES row", ) - self.assertEqual(code, 2) - self.assertIn("thread-id", payload.get("error", "")) - self.assertIn("bulk-resolve", payload.get("error", "")) - - def test_autonomous_allow_unpinned_thread_is_refused(self): - # There is no unpinned autonomous resolve: --allow-unpinned-thread is an - # interactive-only override and must not open a bypass around the pins in - # unattended mode, even with a single --thread-id. - code, payload = run( - RESOLVE, "owner/repo#1", "--allowed-owners", "owner", - "--autonomous", "--resolve", "--thread-id", "PRRT_abc", - "--allow-unpinned-thread", + + +class GeneratedDocIsCurrent(unittest.TestCase): + def test_reference_doc_matches_the_tables(self) -> None: + # splitlines(), not bytes: the checkout's line endings are not the claim. + expected = contract.render_markdown().splitlines() + actual = contract.GENERATED_DOC.read_text(encoding="utf-8").splitlines() + self.assertEqual( + actual, + expected, + "reference/guard-contract.md is stale -- regenerate it with" + " `python tests/guard_contract.py --emit`", ) - self.assertEqual(code, 2) - self.assertIn("allow-unpinned-thread", payload.get("error", "")) if __name__ == "__main__": From 455022b13dea1a1be77479f00362e45116e37fda Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:22:59 -0400 Subject: [PATCH 2/8] fix(source-control): assert the refusal envelope and widen the doc sweep Three tightenings on the guard contract: - Scope refusals now assert `inScope` is present and false, not merely falsy. A consumer branches on that field, and `payload.get("inScope")` passed vacuously when the key was absent. - The unclassified-document gate sweeps the whole plugin rather than the babysit skill alone: a wrapper command line copied into a command, an agent, or a sibling skill drifts the same way. - The deferred flag catalogue is recorded in the sanctioned TODO(#issue) form the comment-hygiene policy allows. --- .../scripts/tests/guard_contract.py | 19 +++++++++++++------ .../babysit-prs/scripts/tests/test_guards.py | 14 ++++++++++++-- 2 files changed, 25 insertions(+), 8 deletions(-) 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 403738442c..b99e4330e2 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 @@ -16,12 +16,12 @@ Source anchors are `file.py::symbol`, never line numbers -- a line number in a drift-detection artifact is the drift it exists to catch. -DEFERRED: an argparse flag catalogue (every flag's name, type, and default per -entry point) would additionally catch a renamed flag or a changed default that -no behavior row exercises. It needs a `build_parser()` extraction across all -nine entry points, whose parsers are built inside `main()`. Trigger to build it: -the first time a flag rename or default change ships without a matching update -here. Tracked on issue #1265. +TODO(#1265): add an argparse flag catalogue -- every flag's name, type, and +default per entry point -- which would additionally catch a renamed flag or a +changed default that no behavior row exercises. Deferred because it needs a +`build_parser()` extraction across all nine entry points, whose parsers are +built inside `main()`. Trigger: the first time a flag rename or default change +ships without a matching update here. """ from __future__ import annotations @@ -70,6 +70,9 @@ class Refusal: error_contains: tuple[str, ...] refused_by: str enforced_at: str + # Envelope fields a caller may branch on, asserted by identity. A consumer + # reading `inScope` needs it present and false, not merely falsy-or-absent. + envelope_fields: tuple[tuple[str, object], ...] = () @dataclass(frozen=True) @@ -167,6 +170,7 @@ class DocCommandSource: error_contains=("allowed-owners",), refused_by=PYTHON_CLI, enforced_at="babysit_merge.py::main", + envelope_fields=(("inScope", False),), ), Refusal( id="merge.owner-out-of-scope", @@ -180,6 +184,7 @@ class DocCommandSource: error_contains=(), refused_by=PYTHON_CLI, enforced_at="babysit_merge.py::main", + envelope_fields=(("inScope", False),), ), Refusal( id="merge.owner-check-precedes-self-login-resolution", @@ -199,6 +204,7 @@ class DocCommandSource: error_contains=(), refused_by=PYTHON_CLI, enforced_at="babysit_merge.py::main", + envelope_fields=(("inScope", False),), ), Refusal( id="merge.short-expected-head", @@ -327,6 +333,7 @@ class DocCommandSource: error_contains=(), refused_by=PYTHON_CLI, enforced_at="babysit_resolve_thread.py::main", + envelope_fields=(("inScope", False),), ), Refusal( id="resolve.count-pin-without-thread-id", diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py index d83b520c4e..ab22764e0b 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py @@ -131,6 +131,15 @@ def test_every_refusal_row(self) -> None: self.assertIn( token, combined, because(row.id, row.claim, combined[:400]) ) + for key, value in row.envelope_fields: + self.assertIn( + key, payload, because(row.id, row.claim, f"no `{key}` in the envelope") + ) + self.assertEqual( + payload[key], + value, + because(row.id, row.claim, f"{key}={payload[key]!r}"), + ) if row.entry_point.startswith("bin/"): # The observable bash-vs-Python discriminator: a wrapper-level # refusal never reaches the interpreter, so it emits no envelope. @@ -358,10 +367,11 @@ def test_every_doc_naming_a_wrapper_is_covered(self) -> None: # A second document growing its own copy of a command line must be added # to DOC_COMMAND_SOURCES, not left unchecked. covered = {row.doc for row in contract.DOC_COMMAND_SOURCES} - skill_root = contract.PLUGIN_ROOT / "skills" / "babysit-prs" + # The whole plugin, not just the babysit skill: a command line copied + # into a command, agent, or sibling skill drifts exactly the same way. naming = { str(path.relative_to(contract.PLUGIN_ROOT)).replace("\\", "/") - for path in skill_root.rglob("*.md") + for path in contract.PLUGIN_ROOT.rglob("*.md") if WRAPPER_COMMAND.search(path.read_text(encoding="utf-8")) } self.assertEqual( From 5515b19d37083b80029aac5e126b39c27199db78 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:39:11 -0400 Subject: [PATCH 3/8] docs(source-control): keep the guard-contract pointer in safety.md alone SKILL.md is at its 500-line hard cap; a second pointer would have left the file exactly at the cap for the next editor. SKILL.md already routes the guarded-mutation section to reference/safety.md, which now points at the contract, so one canonical pointer carries it. --- plugins/source-control/skills/babysit-prs/SKILL.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/SKILL.md b/plugins/source-control/skills/babysit-prs/SKILL.md index ce4593de22..0a98ba225f 100644 --- a/plugins/source-control/skills/babysit-prs/SKILL.md +++ b/plugins/source-control/skills/babysit-prs/SKILL.md @@ -187,8 +187,6 @@ That gate merges **only when every criterion holds** — the criteria and the sa The two mutation gates are invoked ONLY through their wrapper scripts, by the bundled `bin/`-path form — never the bare command name nor the raw Python behind them. Each `source-control-babysit- …` spelled in the bullets below is that wrapper launched by its `bin/`-path form; the exact form is the single home in [reference/safety.md](reference/safety.md). Both fail closed without `--allowed-owners`. -What each gate refuses, which invocations mutate, and where each refusal is enforced is the -executable contract in [reference/guard-contract.md](reference/guard-contract.md). - **Merge readiness** — `source-control-babysit-merge owner/repo#N --allowed-owners --self-logins @me,` (read-only; add `--merge --expected-head From 4bf9eb97ba5bbba15963482eb72f9271e3c83ddb Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 25 Jul 2026 03:08:28 -0400 Subject: [PATCH 4/8] fix(source-control): scope the guard contract to domain mutations Four review findings, each a claim the contract published that CI did not actually prove. - babysit_merge.py's entry row claimed it "writes queue state". It imports no state module and calls write_state zero times. The clause stays on refresh_pr_branch.py and request_review.py, which call it three times each. - The entry-point table declared `gate` the condition under which the mutation happens "at all", but every state-touching script creates its state directory and a `.lock` sibling on entering state_lock, before any --apply check. The table now scopes `mutation` to domain state and names the housekeeping it deliberately excludes, so a classifier reading it still grants the script writes inside --state-dir rather than treating a dry run as writing nothing. - The snapshot mechanism claimed a flagless invocation is "a pure read". load_state runs unconditionally, so an already-corrupt state file is quarantined by rename whatever the flags say. - The documented-command parser's tail ran to the closing backtick, so a fenced block yielded only its first command. reference/safety.md was already hitting this: the resolve-wrapper invocation was swallowed into the merge command's tail and its flags validated against the merge CLI. Each invocation is now bounded by the next one, surfacing six commands there where five were seen. --- .../babysit-prs/reference/guard-contract.md | 8 +++--- .../scripts/tests/guard_contract.py | 27 +++++++++++++------ .../babysit-prs/scripts/tests/test_guards.py | 20 +++++++++----- 3 files changed, 36 insertions(+), 19 deletions(-) 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 8bd01a6c3d..4acdb634c4 100644 --- a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -14,18 +14,18 @@ Source anchors are `file::symbol`, deliberately not line numbers. ## Entry points: what mutates -`gate` is the condition under which the mutation happens at all. A blank wrapper column means the entry point has no `bin/` wrapper and is invoked through the interpreter. +This table scopes `mutation` to DOMAIN state -- GitHub, the queue state file, worktrees, and leases -- and `gate` is the condition under which that domain mutation happens at all. It is deliberately not a filesystem-write audit: a state-touching script performs housekeeping under `--state-dir` whatever its flags say, so entering `state_lock` creates the state directory and a `.lock` sibling, and `load_state` quarantines an already-corrupt state file by renaming it. A classifier granting a gate-less invocation therefore still needs the script to be able to write inside `--state-dir`; what the gate withholds is the domain mutation, not every byte. A blank wrapper column means the entry point has no `bin/` wrapper and is invoked through the interpreter. | Entry point | Wrapper | Class | Mutates | Gate | Claim | Backed by | | --- | --- | --- | --- | --- | --- | --- | -| `skills/babysit-prs/scripts/babysit_merge.py` | `bin/source-control-babysit-merge` | conditionally mutating | merges the PR on GitHub; writes queue state | --merge (absent: readiness check only, exit 0 ready / 10 not ready) | Without --merge this is a readiness reporter. With it, the TOCTOU guard requires --expected-head unless --allow-unpinned-head is passed -- and the bin/ wrapper refuses that override outright. | `merge.allowlist-absent`, `merge.unpinned-head-refused-by-wrapper`, `merge.wrapper-filters-unpinned-head-in-bash` | +| `skills/babysit-prs/scripts/babysit_merge.py` | `bin/source-control-babysit-merge` | conditionally mutating | merges the PR on GitHub | --merge (absent: readiness check only, exit 0 ready / 10 not ready) | Without --merge this is a readiness reporter. With it, the TOCTOU guard requires --expected-head unless --allow-unpinned-head is passed -- and the bin/ wrapper refuses that override outright. | `merge.allowlist-absent`, `merge.unpinned-head-refused-by-wrapper`, `merge.wrapper-filters-unpinned-head-in-bash` | | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `bin/source-control-babysit-resolve-thread` | conditionally mutating | marks review threads resolved on GitHub | --resolve (absent: classification report only) | Which threads it may touch is decided per fetched thread by the classifier, not by the argument shape -- see the predicate rows. The wrapper adds no refusal of its own. | `resolve.allowlist-absent`, `resolve.autonomous-bulk-refused`, `classify.interactive-does-not-require-outdated`, `resolve.wrapper-carries-no-filter` | | `skills/babysit-prs/scripts/manage_babysit_lease.py` | -- | mutating | writes, renews, and deletes lease files under --state-dir | none for acquire / heartbeat / release -- they write unconditionally; --apply gates reap's deletion ONLY | Local filesystem writes only, no GitHub write. The --apply flag does not make this script read-only in its other three actions. | `lease.acquire-writes-without-apply`, `lease.reap-without-apply-is-a-dry-run` | | `skills/babysit-prs/scripts/manage_feedback_ledger.py` | -- | conditionally mutating | records feedback dispositions and fix rounds in queue state | --apply, additionally requiring --lease-token | Local state only, no GitHub write. The lease token requirement means an --apply invocation still refuses without proof of lease ownership. | `ledger.apply-requires-lease-token` | | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | -- | conditionally mutating | runs `git worktree remove` on eligible worktrees under --root | --apply (absent: dry run) | The most destructive local helper in the lane. --root is required, so it cannot sweep a directory the caller did not name. | `prune.root-is-required`, `prune.removes-worktrees-via-git` | | `skills/babysit-prs/scripts/refresh_pr_branch.py` | -- | conditionally mutating | asks GitHub to update the PR branch server-side; writes queue state | --apply | Mutates the PR branch on GitHub WITHOUT pushing: the update is a pinned server-side update-branch API call. | `refresh.uses-server-side-update-branch` | | `skills/babysit-prs/scripts/request_review.py` | -- | conditionally mutating | POSTs an issue comment requesting an AI re-review; writes queue state | --apply | A GitHub write despite the read-sounding name. | `review.posts-a-comment` | -| `skills/babysit-prs/scripts/pr_queue_snapshot.py` | -- | conditionally mutating | writes the queue snapshot to local state | --write-state (absent: reports the snapshot without persisting it) | No GitHub write on any path. The only mutation is the local snapshot file. | `snapshot.write-state-gates-the-only-write` | +| `skills/babysit-prs/scripts/pr_queue_snapshot.py` | -- | conditionally mutating | writes the queue snapshot to local state | --write-state (absent: reports the snapshot without persisting it) | No GitHub write on any path. The only domain-state mutation is the local snapshot file. | `snapshot.write-state-gates-the-only-write` | | `skills/babysit-prs/scripts/babysit_findings.py` | -- | read-only | nothing | n/a | Pure classification over supplied input; no write of any kind. | -- | ## Refusals: guards that fire on argument shape alone @@ -90,7 +90,7 @@ Asserted against the entry point's source, because the mechanism -- not just the | `refresh.uses-server-side-update-branch` | `skills/babysit-prs/scripts/refresh_pr_branch.py` | refresh_pr_branch.py never pushes. It asks GitHub to update the branch server-side via PUT repos/{repo}/pulls/{number}/update-branch, pinned with expected_head_sha. A consumer reasoning about push guards for this script is reasoning about the wrong mechanism -- it needs no local write access to the branch at all. | | `review.posts-a-comment` | `skills/babysit-prs/scripts/request_review.py` | request_review.py --apply performs a GitHub write: it POSTs an issue comment carrying the review-trigger phrase. It is a mutating helper despite its read-sounding name. | | `prune.removes-worktrees-via-git` | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | prune_babysit_worktrees.py --apply runs `git worktree remove`, a destructive local filesystem operation. Its dry-run default is what makes the flagless form safe, not anything about the name. | -| `snapshot.write-state-gates-the-only-write` | `skills/babysit-prs/scripts/pr_queue_snapshot.py` | pr_queue_snapshot.py's single write is the local snapshot, and --write-state is the only thing that reaches it. Nothing on any path writes to GitHub, so a flagless invocation is a pure read of the queue. | +| `snapshot.write-state-gates-the-only-write` | `skills/babysit-prs/scripts/pr_queue_snapshot.py` | pr_queue_snapshot.py's only queue-state write is the local snapshot, and --write-state is the only thing that reaches it. Nothing on any path writes to GitHub, so a flagless invocation reads the queue and persists nothing of it. Housekeeping is out of scope per the entry-point table's mutation definition: this script loads state unconditionally, so load_state may quarantine an already-corrupt state file whatever the flags say. | | `merge.wrapper-filters-unpinned-head-in-bash` | `bin/source-control-babysit-merge` | The merge wrapper's refusal is a bash argument loop over "$@" that exits before the interpreter shim is sourced -- it is not, and must not become, an argparse flag on the Python side. | | `resolve.wrapper-carries-no-filter` | `bin/source-control-babysit-resolve-thread` | ASYMMETRY, stated at the source: the resolve wrapper contains no argument filter loop. Adding one here without a matching row is the drift this contract exists to catch. | 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 b99e4330e2..78052c955a 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 @@ -737,9 +737,12 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje Mechanism( id="snapshot.write-state-gates-the-only-write", claim=( - "pr_queue_snapshot.py's single write is the local snapshot, and --write-state " - "is the only thing that reaches it. Nothing on any path writes to GitHub, so " - "a flagless invocation is a pure read of the queue." + "pr_queue_snapshot.py's only queue-state write is the local snapshot, and " + "--write-state is the only thing that reaches it. Nothing on any path writes " + "to GitHub, so a flagless invocation reads the queue and persists nothing of " + "it. Housekeeping is out of scope per the entry-point table's mutation " + "definition: this script loads state unconditionally, so load_state may " + "quarantine an already-corrupt state file whatever the flags say." ), entry_point=SNAPSHOT_CLI, must_contain=("if args.write_state:", "save_state("), @@ -781,7 +784,7 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje path=MERGE_CLI, wrapper=MERGE_WRAPPER, mutation=CONDITIONAL, - mutates_what="merges the PR on GitHub; writes queue state", + mutates_what="merges the PR on GitHub", gate="--merge (absent: readiness check only, exit 0 ready / 10 not ready)", claim=( "Without --merge this is a readiness reporter. With it, the TOCTOU guard " @@ -882,7 +885,8 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje mutates_what="writes the queue snapshot to local state", gate="--write-state (absent: reports the snapshot without persisting it)", claim=( - "No GitHub write on any path. The only mutation is the local snapshot file." + "No GitHub write on any path. The only domain-state mutation is the local " + "snapshot file." ), backed_by=("snapshot.write-state-gates-the-only-write",), ), @@ -964,9 +968,16 @@ def render_markdown() -> str: lines += [ "## Entry points: what mutates", "", - "`gate` is the condition under which the mutation happens at all. A blank" - " wrapper column means the entry point has no `bin/` wrapper and is invoked" - " through the interpreter.", + "This table scopes `mutation` to DOMAIN state -- GitHub, the queue state file," + " worktrees, and leases -- and `gate` is the condition under which that domain" + " mutation happens at all. It is deliberately not a filesystem-write audit: a" + " state-touching script performs housekeeping under `--state-dir` whatever its" + " flags say, so entering `state_lock` creates the state directory and a `.lock`" + " sibling, and `load_state` quarantines an already-corrupt state file by renaming" + " it. A classifier granting a gate-less invocation therefore still needs the" + " script to be able to write inside `--state-dir`; what the gate withholds is the" + " domain mutation, not every byte. A blank wrapper column means the entry point" + " has no `bin/` wrapper and is invoked through the interpreter.", "", "| Entry point | Wrapper | Class | Mutates | Gate | Claim | Backed by |", "| --- | --- | --- | --- | --- | --- | --- |", diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py index ab22764e0b..90bda59a67 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py @@ -305,7 +305,7 @@ def test_every_mutating_entry_point_is_backed(self) -> None: WRAPPER_COMMAND = re.compile( - r'bash "\$\{CLAUDE_PLUGIN_ROOT\}/(bin/source-control-babysit-[a-z-]+)"([^`]*)' + r'bash "\$\{CLAUDE_PLUGIN_ROOT\}/(bin/source-control-babysit-[a-z-]+)"' ) @@ -315,13 +315,19 @@ def documented_commands(text: str) -> list[tuple[str, str]]: A command runs to the end of its inline-code span or fenced block, which prose wraps across lines, so the tail is truncated at the first blank line rather than the first newline. The truncation is done here instead of in the - pattern: expressing "not a backtick, and not a blank line" as an alternation - backtracks catastrophically on a document this size. + pattern: expressing the stop set as an alternation backtracks + catastrophically on a document this size. A fenced block may hold several + commands, so a tail also stops at the next wrapper invocation -- one greedy + tail swallowing its successors would check the later commands' flags against + the first command's CLI, or skip them entirely. """ - return [ - (wrapper, " ".join(tail.split("\n\n", 1)[0].split())) - for wrapper, tail in WRAPPER_COMMAND.findall(text) - ] + matches = list(WRAPPER_COMMAND.finditer(text)) + commands = [] + for index, match in enumerate(matches): + limit = matches[index + 1].start() if index + 1 < len(matches) else len(text) + tail = text[match.end() : limit].split("`", 1)[0].split("\n\n", 1)[0] + commands.append((match.group(1), " ".join(tail.split()))) + return commands class DocumentedCommandsMatchTheParsers(unittest.TestCase): From 0689a1d95696aad6d6278e665bbb36e1fa8f5760 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 25 Jul 2026 15:53:35 -0400 Subject: [PATCH 5/8] fix(source-control): scope the contract's own executed-assertion claim The preamble promised "Every statement below is executed as an assertion by scripts/tests/test_guards.py". Four rendered columns are not: the entry-point table's Mutates, Gate, and Claim, and the refusal table's Enforced at. test_guards.py reads entry.path, entry.wrapper, entry.mutation, and entry.backed_by and nothing else from an EntryPoint, and row.enforced_at only over PREDICATES, so Refusal.enforced_at is unread. A consumer writing a permission rule against those columns believed CI proved them. That is the failure mode this contract exists to close, so publishing it here was self-refuting. The preamble now names which tables are executed, and "Not covered here" gains the four columns with what IS asserted about entry-point rows, so the disclosure is usable rather than a bare disclaimer. Binding those columns to real assertions is a separate change; this one stops the document overstating what CI proves today. Co-authored-by: Claude Opus 5 (1M context) --- .../babysit-prs/reference/guard-contract.md | 10 +++++++--- .../babysit-prs/scripts/tests/guard_contract.py | 17 ++++++++++++++--- 2 files changed, 21 insertions(+), 6 deletions(-) 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 4acdb634c4..7d18b8f1c4 100644 --- a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -4,9 +4,12 @@ `python tests/guard_contract.py --emit`. Hand edits fail CI. --> What a host permission classifier, an orchestration prompt, or a downstream -consumer may assume about this lane's entry points. Every statement below is -executed as an assertion by `scripts/tests/test_guards.py`; a guard change that -falsifies one fails CI with a message naming the claim. +consumer may assume about this lane's entry points. Rows in the refusal, +predicate, effect, and documented-command tables are executed as assertions by +`scripts/tests/test_guards.py`; a guard change that falsifies one fails CI with +a message naming the claim. The columns listed under "Not covered here" are +rendered from the same data but are not asserted — read those as annotation, not +as proof. Cite a row by its ID. IDs are stable; rows are removed only when the behavior is. @@ -107,3 +110,4 @@ Documents that spell out copyable `bin/`-path wrapper commands. Every wrapper pa - Flag names, types, and defaults are not catalogued. A behavior row exercises the flags it names; a rename or default change elsewhere is not detected. See the deferral note in `scripts/tests/guard_contract.py`. - Anything requiring a live GitHub response. Every row above runs with no network access, which is what keeps the suite free of a `gh` stub. +- Four rendered columns are annotation rather than assertion, and a rule must not be written against them as if CI proved them: the entry-point table's **Mutates**, **Gate**, and **Claim**, and the refusal table's **Enforced at**. The entry-point rows assert the path, the wrapper, the class, and that a mutating row names backing rows; nothing reads the prose in those three columns. `Predicate.enforced_at` *is* checked -- `test_anchor_symbol_exists` resolves it -- but `Refusal.enforced_at` is not, so a refusal's location may drift without failing CI. 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 78052c955a..7b2ca2ed3d 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 @@ -944,9 +944,12 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje `python tests/guard_contract.py --emit`. Hand edits fail CI. --> What a host permission classifier, an orchestration prompt, or a downstream -consumer may assume about this lane's entry points. Every statement below is -executed as an assertion by `scripts/tests/test_guards.py`; a guard change that -falsifies one fails CI with a message naming the claim. +consumer may assume about this lane's entry points. Rows in the refusal, +predicate, effect, and documented-command tables are executed as assertions by +`scripts/tests/test_guards.py`; a guard change that falsifies one fails CI with +a message naming the claim. The columns listed under "Not covered here" are +rendered from the same data but are not asserted — read those as annotation, not +as proof. Cite a row by its ID. IDs are stable; rows are removed only when the behavior is. @@ -1084,6 +1087,14 @@ def render_markdown() -> str: " the deferral note in `scripts/tests/guard_contract.py`.", "- Anything requiring a live GitHub response. Every row above runs with no" " network access, which is what keeps the suite free of a `gh` stub.", + "- Four rendered columns are annotation rather than assertion, and a rule" + " must not be written against them as if CI proved them: the entry-point" + " table's **Mutates**, **Gate**, and **Claim**, and the refusal table's" + " **Enforced at**. The entry-point rows assert the path, the wrapper, the" + " class, and that a mutating row names backing rows; nothing reads the" + " prose in those three columns. `Predicate.enforced_at` *is* checked --" + " `test_anchor_symbol_exists` resolves it -- but `Refusal.enforced_at` is" + " not, so a refusal's location may drift without failing CI.", "", ] return "\n".join(lines) From ac6af1647670a3d370e5fd2931f64cf5c0e699d4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 25 Jul 2026 18:52:02 -0400 Subject: [PATCH 6/8] fix(source-control): bind each guard-contract row to its own claim Six review findings, all the same shape: a row asserted a proxy for its claim rather than the claim. - A refusal claiming "no gh invocation at all" now replays against a recording `gh` shim that is the only executable on PATH and fails if the shim ran. Emptying PATH proved only that the refusal survives gh being unreachable. - An effect row records WHICH way the state directory's file set moved, so a reap that rewrote the expired lease can no longer pass a deletion claim. Compared over the key set, not named paths, to avoid coupling the contract to the lease writer's layout. - Documented flags are read from the parser's usage block. Scraping whole `--help` output admitted `--admin`, which appears only in the merge gate's prose and exits 2 when passed; a stale documented flag would have passed. - The completeness scan now covers `scripts/babysit-*.sh`, catching the plugin-level readiness gate SKILL.md invokes by name. - The wrapper's prefix refusal and both parsers' `allow_abbrev=False` now carry contract rows, so `--allow-unpinned-hea` is covered by assertion. - Class is bound where offline evidence exists -- a read-only entry point may not cite mutation evidence -- and the generated doc now names the gap where it cannot: the four entry points whose mutation is a GitHub write cannot be witnessed by a suite that runs without network access. --- plugins/source-control/CHANGELOG.md | 11 +- .../babysit-prs/reference/guard-contract.md | 72 ++++---- .../scripts/tests/guard_contract.py | 167 ++++++++++++++++-- .../babysit-prs/scripts/tests/test_guards.py | 158 +++++++++++++++-- 4 files changed, 342 insertions(+), 66 deletions(-) diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 8972d515c4..57c50730f8 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -23,7 +23,16 @@ All notable changes to the `source-control` plugin are documented here. Format f `update-branch` and never pushes), and documented command lines (every `bin/`-path wrapper command spelled in `reference/safety.md` and `reference/orchestration.md` is checked against the backing CLI's own parser). Catalogue gates fail when a new entry point, wrapper, or - command-spelling document arrives without a row. + command-spelling document arrives without a row — including the plugin-level + `scripts/babysit-readiness-gate.sh`, the one lane entry point outside the skill's scripts + directory. Each binding asserts the specific claim rather than a proxy for it: a row claiming + the refusal precedes every network call is replayed against a recording `gh` shim and fails if + the shim ran at all, an effect row records which way the state directory's file set moved so a + rewrite cannot pass as a deletion, and documented flags are checked against the parser's usage + block rather than scraped `--help` prose that names flags the CLI rejects. What CI does not + bind is stated in the generated doc's "Not covered here" section rather than left to inference: + the entry-point **Class** column cannot be proven for the four entry points whose mutation is a + GitHub write, because every row runs without network access. ## [0.26.9] 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 7d18b8f1c4..917e57acfa 100644 --- a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -21,8 +21,8 @@ This table scopes `mutation` to DOMAIN state -- GitHub, the queue state file, wo | Entry point | Wrapper | Class | Mutates | Gate | Claim | Backed by | | --- | --- | --- | --- | --- | --- | --- | -| `skills/babysit-prs/scripts/babysit_merge.py` | `bin/source-control-babysit-merge` | conditionally mutating | merges the PR on GitHub | --merge (absent: readiness check only, exit 0 ready / 10 not ready) | Without --merge this is a readiness reporter. With it, the TOCTOU guard requires --expected-head unless --allow-unpinned-head is passed -- and the bin/ wrapper refuses that override outright. | `merge.allowlist-absent`, `merge.unpinned-head-refused-by-wrapper`, `merge.wrapper-filters-unpinned-head-in-bash` | -| `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `bin/source-control-babysit-resolve-thread` | conditionally mutating | marks review threads resolved on GitHub | --resolve (absent: classification report only) | Which threads it may touch is decided per fetched thread by the classifier, not by the argument shape -- see the predicate rows. The wrapper adds no refusal of its own. | `resolve.allowlist-absent`, `resolve.autonomous-bulk-refused`, `classify.interactive-does-not-require-outdated`, `resolve.wrapper-carries-no-filter` | +| `skills/babysit-prs/scripts/babysit_merge.py` | `bin/source-control-babysit-merge` | conditionally mutating | merges the PR on GitHub | --merge (absent: readiness check only, exit 0 ready / 10 not ready) | Without --merge this is a readiness reporter. With it, the TOCTOU guard requires --expected-head unless --allow-unpinned-head is passed -- and the bin/ wrapper refuses that override outright. | `merge.allowlist-absent`, `merge.unpinned-head-refused-by-wrapper`, `merge.abbreviated-unpinned-head-refused-by-wrapper`, `merge.abbreviation-is-not-resolved-by-the-cli`, `merge.wrapper-filters-unpinned-head-in-bash`, `merge.parser-refuses-abbreviation` | +| `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `bin/source-control-babysit-resolve-thread` | conditionally mutating | marks review threads resolved on GitHub | --resolve (absent: classification report only) | Which threads it may touch is decided per fetched thread by the classifier, not by the argument shape -- see the predicate rows. The wrapper adds no refusal of its own. | `resolve.allowlist-absent`, `resolve.autonomous-bulk-refused`, `classify.interactive-does-not-require-outdated`, `resolve.wrapper-carries-no-filter`, `resolve.parser-refuses-abbreviation` | | `skills/babysit-prs/scripts/manage_babysit_lease.py` | -- | mutating | writes, renews, and deletes lease files under --state-dir | none for acquire / heartbeat / release -- they write unconditionally; --apply gates reap's deletion ONLY | Local filesystem writes only, no GitHub write. The --apply flag does not make this script read-only in its other three actions. | `lease.acquire-writes-without-apply`, `lease.reap-without-apply-is-a-dry-run` | | `skills/babysit-prs/scripts/manage_feedback_ledger.py` | -- | conditionally mutating | records feedback dispositions and fix rounds in queue state | --apply, additionally requiring --lease-token | Local state only, no GitHub write. The lease token requirement means an --apply invocation still refuses without proof of lease ownership. | `ledger.apply-requires-lease-token` | | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | -- | conditionally mutating | runs `git worktree remove` on eligible worktrees under --root | --apply (absent: dry run) | The most destructive local helper in the lane. --root is required, so it cannot sweep a directory the caller did not name. | `prune.root-is-required`, `prune.removes-worktrees-via-git` | @@ -30,35 +30,40 @@ This table scopes `mutation` to DOMAIN state -- GitHub, the queue state file, wo | `skills/babysit-prs/scripts/request_review.py` | -- | conditionally mutating | POSTs an issue comment requesting an AI re-review; writes queue state | --apply | A GitHub write despite the read-sounding name. | `review.posts-a-comment` | | `skills/babysit-prs/scripts/pr_queue_snapshot.py` | -- | conditionally mutating | writes the queue snapshot to local state | --write-state (absent: reports the snapshot without persisting it) | No GitHub write on any path. The only domain-state mutation is the local snapshot file. | `snapshot.write-state-gates-the-only-write` | | `skills/babysit-prs/scripts/babysit_findings.py` | -- | read-only | nothing | n/a | Pure classification over supplied input; no write of any kind. | -- | +| `scripts/babysit-readiness-gate.sh` | -- | read-only | nothing | n/a | The lane's one entry point outside the skill's own scripts directory, invoked by name from SKILL.md. It counts findings and classification rows over fetched comments and reports READINESS_OK / READINESS_BLOCKED; it writes no file and performs no GitHub write. | -- | ## Refusals: guards that fire on argument shape alone Each row is executed. `Refused by` says which layer rejected the invocation. On a `bin/` entry point the distinction is asserted, using the observable discriminator: a bash-wrapper refusal never reaches the interpreter, so it emits plain stderr and no JSON envelope. On a CLI entry point the layer is always `python-cli` and only the exit code and message are asserted. -| ID | Entry point | Invocation | Exit | Refused by | Error names | Enforced at | Claim | -| --- | --- | --- | --- | --- | --- | --- | --- | -| `merge.allowlist-absent` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1` | 3 | python-cli | `allowed-owners` | `babysit_merge.py::main` | The merge gate is fail-closed on scope: with no --allowed-owners it refuses every PR at exit 3 rather than defaulting to the current repo's owner. | -| `merge.owner-out-of-scope` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners someone-else` | 3 | python-cli | -- | `babysit_merge.py::main` | An owner outside --allowed-owners is refused at exit 3, so a permission rule may assume the gate never acts on a repository the operator did not name. | -| `merge.owner-check-precedes-self-login-resolution` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners someone-else --self-logins @me` | 3 | python-cli | -- | `babysit_merge.py::main` | The owner check runs before '@me' self-login resolution, which is a network call: an out-of-scope owner refuses at exit 3 with no gh invocation at all. | -| `merge.short-expected-head` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --merge --expected-head abc` | 2 | python-cli | `expected-head` | `babysit_merge.py::main` | --expected-head must be a long-enough SHA prefix; a short value is a usage error at exit 2, never a loose prefix match against the live head. | -| `merge.malformed-ref` | `skills/babysit-prs/scripts/babysit_merge.py` | `not-a-ref --allowed-owners owner` | 2 | python-cli | -- | `babysit_merge.py::main` | The PR reference must parse as owner/repo#number or a PR URL; anything else is a usage error at exit 2. | -| `merge.autopilot-tier-without-required-sets` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier` | 3 | python-cli | `--lane-logins`, `--approver-bot-logins`, `--block-labels` | `babysit_merge.py::main` | The autopilot merge tier is fail-closed: the umbrella flag alone, with none of --lane-logins, --approver-bot-logins, or --block-labels, refuses at exit 3 before any network access. | -| `merge.autopilot-tier-partial-config` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier --lane-logins lane --approver-bot-logins bot` | 3 | python-cli | `--block-labels` | `babysit_merge.py::main` | A partially configured autopilot tier still refuses at exit 3, naming only the missing set -- there is no partial-credit tier. | -| `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.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 | -- | `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. | -| `resolve.count-pin-without-thread-id` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --expected-comment-count 3` | 2 | python-cli | `thread-id` | `babysit_resolve_thread.py::main` | --expected-comment-count without --thread-id is a usage error at exit 2: a pin with no thread to pin would silently fall through to resolving every eligible thread. | -| `resolve.last-updated-pin-without-thread-id` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --expected-last-updated 2026-07-17T10:00:00Z` | 2 | python-cli | `thread-id` | `babysit_resolve_thread.py::main` | --expected-last-updated without --thread-id is a usage error at exit 2, for the same silent-bulk-fallthrough reason as the count pin. | -| `resolve.thread-id-without-pins` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --resolve --thread-id PRRT_abc` | 2 | python-cli | `--expected-comment-count`, `--expected-last-updated` | `babysit_resolve_thread.py::main` | A --thread-id resolve requires BOTH TOCTOU pins; supplying neither is a usage error at exit 2 naming both missing flags. | -| `resolve.autonomous-bulk-refused` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --autonomous --resolve` | 2 | python-cli | `thread-id`, `bulk-resolve` | `babysit_resolve_thread.py::main` | --autonomous --resolve without --thread-id is refused at exit 2: --autonomous means SINGLE PINNED THREAD, never bulk. A worker's own push marks a thread isOutdated, so a bulk autonomous resolve would clear threads with no proof the finding was addressed. | -| `resolve.autonomous-allow-unpinned-refused` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --autonomous --resolve --thread-id PRRT_abc --allow-unpinned-thread` | 2 | python-cli | `allow-unpinned-thread` | `babysit_resolve_thread.py::main` | There is no unpinned autonomous resolve: --allow-unpinned-thread is an interactive-only override and is refused at exit 2 under --autonomous even with a single --thread-id. | -| `resolve.wrapper-reaches-failclosed-cli` | `bin/source-control-babysit-resolve-thread` | `owner/repo#1` | 3 | python-cli | -- | `babysit_resolve_thread.py::main` | The resolve wrapper is a pure passthrough: no --allowed-owners refuses at exit 3 from the CLI, unchanged by the wrapper. | -| `resolve.wrapper-filters-nothing` | `bin/source-control-babysit-resolve-thread` | `owner/repo#1 --allowed-owners owner --autonomous --resolve --thread-id PRRT_abc --allow-unpinned-thread` | 2 | python-cli | `allow-unpinned-thread` | `babysit_resolve_thread.py::main` | ASYMMETRY: unlike the merge wrapper, the resolve wrapper filters no argument. Its --allow-unpinned-thread refusal comes from Python -- proven by the JSON envelope on stdout -- so an operator may not assume a bash-level refusal exists on this entry point just because one exists on the other. | -| `ledger.apply-requires-lease-token` | `skills/babysit-prs/scripts/manage_feedback_ledger.py` | `dispose --pr owner/repo#1 --expected-head-sha 0123456789abcdef0123456789abcdef01234567 --feedback-id F1 --reason contract-row --state-dir {state_dir} --apply` | 2 | python-cli | `lease token` | `manage_feedback_ledger.py::main` | A ledger write is lease-gated: --apply without --lease-token refuses at exit 2 rather than writing an unowned entry. | -| `prune.root-is-required` | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | `--state-dir {state_dir}` | 2 | python-cli | `--root` | `prune_babysit_worktrees.py::main` | prune_babysit_worktrees.py has no default root: omitting --root is a usage error at exit 2, so it can never sweep an unintended directory. | -| `lease.state-dir-is-required` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap` | 2 | python-cli | `--state-dir` | `manage_babysit_lease.py::main` | State-dir resolution is flag-only with no environment fallback: omitting --state-dir is a usage error at exit 2. | -| `lease.scope-required-for-non-reap` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `acquire --state-dir {state_dir}` | 2 | python-cli | `--scope` | `manage_babysit_lease.py::main` | --scope is required for acquire, heartbeat, and release; only reap runs without it. | +`No gh` marks the rows whose claim is that the refusal precedes every network call. Those are replayed a second time with a recording `gh` shim as the only executable on `PATH`, and the assertion is that the shim was never invoked -- merely removing `gh` from `PATH` would let a swallowed lookup failure fall through to the same exit code and read as proof. + +| ID | Entry point | Invocation | Exit | Refused by | No gh | Error names | Enforced at | Claim | +| --- | --- | --- | --- | --- | --- | --- | --- | --- | +| `merge.allowlist-absent` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1` | 3 | python-cli | asserted | `allowed-owners` | `babysit_merge.py::main` | The merge gate is fail-closed on scope: with no --allowed-owners it refuses every PR at exit 3 rather than defaulting to the current repo's owner. | +| `merge.owner-out-of-scope` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners someone-else` | 3 | python-cli | asserted | -- | `babysit_merge.py::main` | An owner outside --allowed-owners is refused at exit 3, so a permission rule may assume the gate never acts on a repository the operator did not name. | +| `merge.owner-check-precedes-self-login-resolution` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners someone-else --self-logins @me` | 3 | python-cli | asserted | -- | `babysit_merge.py::main` | The owner check runs before '@me' self-login resolution, which is a network call: an out-of-scope owner refuses at exit 3 with no gh invocation at all. | +| `merge.short-expected-head` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --merge --expected-head abc` | 2 | python-cli | -- | `expected-head` | `babysit_merge.py::main` | --expected-head must be a long-enough SHA prefix; a short value is a usage error at exit 2, never a loose prefix match against the live head. | +| `merge.malformed-ref` | `skills/babysit-prs/scripts/babysit_merge.py` | `not-a-ref --allowed-owners owner` | 2 | python-cli | -- | -- | `babysit_merge.py::main` | The PR reference must parse as owner/repo#number or a PR URL; anything else is a usage error at exit 2. | +| `merge.autopilot-tier-without-required-sets` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier` | 3 | python-cli | -- | `--lane-logins`, `--approver-bot-logins`, `--block-labels` | `babysit_merge.py::main` | The autopilot merge tier is fail-closed: the umbrella flag alone, with none of --lane-logins, --approver-bot-logins, or --block-labels, refuses at exit 3 before any network access. | +| `merge.autopilot-tier-partial-config` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier --lane-logins lane --approver-bot-logins bot` | 3 | python-cli | -- | `--block-labels` | `babysit_merge.py::main` | A partially configured autopilot tier still refuses at exit 3, naming only the missing set -- there is no partial-credit tier. | +| `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.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. | +| `resolve.count-pin-without-thread-id` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --expected-comment-count 3` | 2 | python-cli | -- | `thread-id` | `babysit_resolve_thread.py::main` | --expected-comment-count without --thread-id is a usage error at exit 2: a pin with no thread to pin would silently fall through to resolving every eligible thread. | +| `resolve.last-updated-pin-without-thread-id` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --expected-last-updated 2026-07-17T10:00:00Z` | 2 | python-cli | -- | `thread-id` | `babysit_resolve_thread.py::main` | --expected-last-updated without --thread-id is a usage error at exit 2, for the same silent-bulk-fallthrough reason as the count pin. | +| `resolve.thread-id-without-pins` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --resolve --thread-id PRRT_abc` | 2 | python-cli | -- | `--expected-comment-count`, `--expected-last-updated` | `babysit_resolve_thread.py::main` | A --thread-id resolve requires BOTH TOCTOU pins; supplying neither is a usage error at exit 2 naming both missing flags. | +| `resolve.autonomous-bulk-refused` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --autonomous --resolve` | 2 | python-cli | -- | `thread-id`, `bulk-resolve` | `babysit_resolve_thread.py::main` | --autonomous --resolve without --thread-id is refused at exit 2: --autonomous means SINGLE PINNED THREAD, never bulk. A worker's own push marks a thread isOutdated, so a bulk autonomous resolve would clear threads with no proof the finding was addressed. | +| `resolve.autonomous-allow-unpinned-refused` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | `owner/repo#1 --allowed-owners owner --autonomous --resolve --thread-id PRRT_abc --allow-unpinned-thread` | 2 | python-cli | -- | `allow-unpinned-thread` | `babysit_resolve_thread.py::main` | There is no unpinned autonomous resolve: --allow-unpinned-thread is an interactive-only override and is refused at exit 2 under --autonomous even with a single --thread-id. | +| `resolve.wrapper-reaches-failclosed-cli` | `bin/source-control-babysit-resolve-thread` | `owner/repo#1` | 3 | python-cli | -- | -- | `babysit_resolve_thread.py::main` | The resolve wrapper is a pure passthrough: no --allowed-owners refuses at exit 3 from the CLI, unchanged by the wrapper. | +| `resolve.wrapper-filters-nothing` | `bin/source-control-babysit-resolve-thread` | `owner/repo#1 --allowed-owners owner --autonomous --resolve --thread-id PRRT_abc --allow-unpinned-thread` | 2 | python-cli | -- | `allow-unpinned-thread` | `babysit_resolve_thread.py::main` | ASYMMETRY: unlike the merge wrapper, the resolve wrapper filters no argument. Its --allow-unpinned-thread refusal comes from Python -- proven by the JSON envelope on stdout -- so an operator may not assume a bash-level refusal exists on this entry point just because one exists on the other. | +| `ledger.apply-requires-lease-token` | `skills/babysit-prs/scripts/manage_feedback_ledger.py` | `dispose --pr owner/repo#1 --expected-head-sha 0123456789abcdef0123456789abcdef01234567 --feedback-id F1 --reason contract-row --state-dir {state_dir} --apply` | 2 | python-cli | -- | `lease token` | `manage_feedback_ledger.py::main` | A ledger write is lease-gated: --apply without --lease-token refuses at exit 2 rather than writing an unowned entry. | +| `prune.root-is-required` | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | `--state-dir {state_dir}` | 2 | python-cli | -- | `--root` | `prune_babysit_worktrees.py::main` | prune_babysit_worktrees.py has no default root: omitting --root is a usage error at exit 2, so it can never sweep an unintended directory. | +| `lease.state-dir-is-required` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap` | 2 | python-cli | -- | `--state-dir` | `manage_babysit_lease.py::main` | State-dir resolution is flag-only with no environment fallback: omitting --state-dir is a usage error at exit 2. | +| `lease.scope-required-for-non-reap` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `acquire --state-dir {state_dir}` | 2 | python-cli | -- | `--scope` | `manage_babysit_lease.py::main` | --scope is required for acquire, heartbeat, and release; only reap runs without it. | ## Predicates: guards over runtime data, not flags @@ -76,13 +81,13 @@ These decide per fetched thread, so no argument shape can express them and no ar ## Effects: what reaches disk -Executed offline against a throwaway state directory seeded with `Fixture`. `State changes` is the observed before/after difference of that directory, not a reading of the flag names. +Executed offline against a throwaway state directory seeded with `Fixture`. `State delta` is the observed before/after movement of that directory's file set, not a reading of the flag names, and it is directional rather than a changed/unchanged boolean: a row claiming deletion asserts that the file set strictly shrank, so a reap that rewrote the expired lease -- or touched some unrelated file -- fails it instead of passing on "something changed". The assertion is over the file set, deliberately not over named paths: a path literal here would couple this contract to the lease writer's internal layout. Advisory `.lock` siblings are excluded from the comparison. -| ID | Entry point | Invocation | Fixture | Exit | State changes | Claim | +| ID | Entry point | Invocation | Fixture | Exit | State delta | Claim | | --- | --- | --- | --- | --- | --- | --- | -| `lease.acquire-writes-without-apply` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `acquire --scope worker --pr owner/repo#1 --state-dir {state_dir}` | empty | 0 | yes | THE TRAP: manage_babysit_lease.py's --apply help reads 'reap only', but acquire writes a lease file unconditionally, with no --apply and no network. A permission rule that derives read-only from flag names classifies acquire wrong. | -| `lease.reap-without-apply-is-a-dry-run` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap --state-dir {state_dir}` | expired-worker-lease | 0 | no | --apply gates reap alone: a reap without it reports which lease files are expired and deletes none of them. | -| `lease.reap-with-apply-deletes` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap --state-dir {state_dir} --apply` | expired-worker-lease | 0 | yes | reap --apply deletes expired worker lease files. This is the one action on this script the --apply flag actually gates. | +| `lease.acquire-writes-without-apply` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `acquire --scope worker --pr owner/repo#1 --state-dir {state_dir}` | empty | 0 | files added | THE TRAP: manage_babysit_lease.py's --apply help reads 'reap only', but acquire writes a lease file unconditionally, with no --apply and no network. A permission rule that derives read-only from flag names classifies acquire wrong. | +| `lease.reap-without-apply-is-a-dry-run` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap --state-dir {state_dir}` | expired-worker-lease | 0 | unchanged | --apply gates reap alone: a reap without it reports which lease files are expired and deletes none of them. | +| `lease.reap-with-apply-deletes` | `skills/babysit-prs/scripts/manage_babysit_lease.py` | `reap --state-dir {state_dir} --apply` | expired-worker-lease | 0 | files removed | reap --apply deletes expired worker lease files. This is the one action on this script the --apply flag actually gates. | ## Mechanisms: how a mutation is performed @@ -95,6 +100,8 @@ Asserted against the entry point's source, because the mechanism -- not just the | `prune.removes-worktrees-via-git` | `skills/babysit-prs/scripts/prune_babysit_worktrees.py` | prune_babysit_worktrees.py --apply runs `git worktree remove`, a destructive local filesystem operation. Its dry-run default is what makes the flagless form safe, not anything about the name. | | `snapshot.write-state-gates-the-only-write` | `skills/babysit-prs/scripts/pr_queue_snapshot.py` | pr_queue_snapshot.py's only queue-state write is the local snapshot, and --write-state is the only thing that reaches it. Nothing on any path writes to GitHub, so a flagless invocation reads the queue and persists nothing of it. Housekeeping is out of scope per the entry-point table's mutation definition: this script loads state unconditionally, so load_state may quarantine an already-corrupt state file whatever the flags say. | | `merge.wrapper-filters-unpinned-head-in-bash` | `bin/source-control-babysit-merge` | The merge wrapper's refusal is a bash argument loop over "$@" that exits before the interpreter shim is sourced -- it is not, and must not become, an argparse flag on the Python side. | +| `merge.parser-refuses-abbreviation` | `skills/babysit-prs/scripts/babysit_merge.py` | babysit_merge.py builds its parser with allow_abbrev=False. Argparse's default would resolve any unambiguous prefix, so a permission rule written against a flag's exact spelling would not cover the prefixes that reach the same flag. | +| `resolve.parser-refuses-abbreviation` | `skills/babysit-prs/scripts/babysit_resolve_thread.py` | babysit_resolve_thread.py sets allow_abbrev=False for the same reason, and it carries more weight here: the resolve wrapper filters nothing, so the parser is the only layer refusing an abbreviated --allow-unpinned-thread. | | `resolve.wrapper-carries-no-filter` | `bin/source-control-babysit-resolve-thread` | ASYMMETRY, stated at the source: the resolve wrapper contains no argument filter loop. Adding one here without a matching row is the drift this contract exists to catch. | ## Documented command lines @@ -110,4 +117,5 @@ Documents that spell out copyable `bin/`-path wrapper commands. Every wrapper pa - Flag names, types, and defaults are not catalogued. A behavior row exercises the flags it names; a rename or default change elsewhere is not detected. See the deferral note in `scripts/tests/guard_contract.py`. - Anything requiring a live GitHub response. Every row above runs with no network access, which is what keeps the suite free of a `gh` stub. -- Four rendered columns are annotation rather than assertion, and a rule must not be written against them as if CI proved them: the entry-point table's **Mutates**, **Gate**, and **Claim**, and the refusal table's **Enforced at**. The entry-point rows assert the path, the wrapper, the class, and that a mutating row names backing rows; nothing reads the prose in those three columns. `Predicate.enforced_at` *is* checked -- `test_anchor_symbol_exists` resolves it -- but `Refusal.enforced_at` is not, so a refusal's location may drift without failing CI. +- Five rendered columns are annotation rather than assertion, and a rule must not be written against them as if CI proved them: the entry-point table's **Class**, **Mutates**, **Gate**, and **Claim**, and the refusal table's **Enforced at**. The entry-point rows assert the path and the wrapper; nothing reads the prose in those columns. `Predicate.enforced_at` *is* checked -- `test_anchor_symbol_exists` resolves it -- but `Refusal.enforced_at` is not, so a refusal's location may drift without failing CI. +- **Class** carries two partial bindings and one real gap, so read it precisely. Bound: a non-read-only row must name at least one backing row, and a `read-only` row may not be cited by any effect row that moves state or by any mechanism row -- so a read-only entry point that grows mutation evidence fails CI. Not bound: that a `mutating` or `conditionally mutating` row's cited evidence actually demonstrates the class it declares. It cannot be, for the four entry points whose mutation is a GitHub write -- the merge gate, the thread resolver, `refresh_pr_branch.py`, and `request_review.py`. Every row in this contract runs with no network access, by design, so no offline assertion can witness their mutation; binding the column would mean either a `gh` stub (asserting the stub, not the behavior) or a live-credential CI suite. An entry point that turns from conditional to unconditional mutation therefore still passes. Trigger to revisit: the first time a class changes without a matching row change, or the first offline-observable mutation appearing on a GitHub-writing entry point. 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 7b2ca2ed3d..ec7d5c73e4 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 @@ -44,6 +44,7 @@ REVIEW_CLI = "skills/babysit-prs/scripts/request_review.py" SNAPSHOT_CLI = "skills/babysit-prs/scripts/pr_queue_snapshot.py" FINDINGS_CLI = "skills/babysit-prs/scripts/babysit_findings.py" +READINESS_GATE = "scripts/babysit-readiness-gate.sh" MERGE_WRAPPER = "bin/source-control-babysit-merge" RESOLVE_WRAPPER = "bin/source-control-babysit-resolve-thread" @@ -73,6 +74,11 @@ class Refusal: # Envelope fields a caller may branch on, asserted by identity. A consumer # reading `inScope` needs it present and false, not merely falsy-or-absent. envelope_fields: tuple[tuple[str, object], ...] = () + # Rows claiming the refusal precedes every network call. These are replayed + # against a recording `gh` shim that is the only executable on PATH, and the + # shim must never be invoked -- an emptied PATH alone would let a swallowed + # `gh` failure fall through to the same exit code and read as proof. + gh_free: bool = False @dataclass(frozen=True) @@ -106,7 +112,10 @@ class Effect: entry_point: str argv: tuple[str, ...] fixture: str - state_changes: bool + # WHICH way state moved, not merely that it moved: a boolean lets a reap + # that rewrites the expired lease -- or touches an unrelated file -- pass a + # row whose claim is deletion. See DELTA_KINDS. + delta: str exit_code: int @@ -171,6 +180,7 @@ class DocCommandSource: refused_by=PYTHON_CLI, enforced_at="babysit_merge.py::main", envelope_fields=(("inScope", False),), + gh_free=True, ), Refusal( id="merge.owner-out-of-scope", @@ -185,6 +195,7 @@ class DocCommandSource: refused_by=PYTHON_CLI, enforced_at="babysit_merge.py::main", envelope_fields=(("inScope", False),), + gh_free=True, ), Refusal( id="merge.owner-check-precedes-self-login-resolution", @@ -205,6 +216,7 @@ class DocCommandSource: refused_by=PYTHON_CLI, enforced_at="babysit_merge.py::main", envelope_fields=(("inScope", False),), + gh_free=True, ), Refusal( id="merge.short-expected-head", @@ -308,6 +320,43 @@ class DocCommandSource: refused_by=BASH_WRAPPER, enforced_at="bin/source-control-babysit-merge (argument filter loop)", ), + Refusal( + id="merge.abbreviated-unpinned-head-refused-by-wrapper", + claim=( + "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." + ), + entry_point=MERGE_WRAPPER, + argv=("owner/repo#1", "--merge", "--allow-unpinned-hea"), + exit_code=2, + error_contains=("--allow-unpinned-hea",), + 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=( + "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." + ), + entry_point=MERGE_CLI, + argv=( + "owner/repo#1", + "--allowed-owners", + "owner", + "--merge", + "--allow-unpinned-hea", + ), + exit_code=2, + error_contains=("--allow-unpinned-hea",), + refused_by=PYTHON_CLI, + enforced_at="babysit_merge.py::main", + ), Refusal( id="merge.wrapper-reaches-failclosed-cli", claim=( @@ -334,6 +383,7 @@ class DocCommandSource: refused_by=PYTHON_CLI, enforced_at="babysit_resolve_thread.py::main", envelope_fields=(("inScope", False),), + gh_free=True, ), Refusal( id="resolve.count-pin-without-thread-id", @@ -646,6 +696,16 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje EMPTY_STATE = "empty" EXPIRED_WORKER_LEASE = "expired-worker-lease" +# Observed movement of the state directory's file set, asserted over the +# fingerprint's keys rather than named paths -- a path literal here would couple +# the contract to the lease writer's internal layout, which is exactly the drift +# this module refuses to encode. +UNCHANGED = "unchanged" +ADDED = "files added" +REMOVED = "files removed" +REWRITTEN = "same files, new contents" +DELTA_KINDS = (UNCHANGED, ADDED, REMOVED, REWRITTEN) + EFFECTS: tuple[Effect, ...] = ( Effect( id="lease.acquire-writes-without-apply", @@ -666,7 +726,7 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje "{state_dir}", ), fixture=EMPTY_STATE, - state_changes=True, + delta=ADDED, exit_code=0, ), Effect( @@ -678,7 +738,7 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje entry_point=LEASE_CLI, argv=("reap", "--state-dir", "{state_dir}"), fixture=EXPIRED_WORKER_LEASE, - state_changes=False, + delta=UNCHANGED, exit_code=0, ), Effect( @@ -690,7 +750,7 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje entry_point=LEASE_CLI, argv=("reap", "--state-dir", "{state_dir}", "--apply"), fixture=EXPIRED_WORKER_LEASE, - state_changes=True, + delta=REMOVED, exit_code=0, ), ) @@ -757,6 +817,28 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje entry_point=MERGE_WRAPPER, must_contain=('for arg in "$@"', "--allow-unpinned-head"), ), + Mechanism( + id="merge.parser-refuses-abbreviation", + claim=( + "babysit_merge.py builds its parser with allow_abbrev=False. Argparse's " + "default would resolve any unambiguous prefix, so a permission rule " + "written against a flag's exact spelling would not cover the prefixes " + "that reach the same flag." + ), + entry_point=MERGE_CLI, + must_contain=("allow_abbrev=False",), + ), + Mechanism( + id="resolve.parser-refuses-abbreviation", + claim=( + "babysit_resolve_thread.py sets allow_abbrev=False for the same reason, " + "and it carries more weight here: the resolve wrapper filters nothing, " + "so the parser is the only layer refusing an abbreviated " + "--allow-unpinned-thread." + ), + entry_point=RESOLVE_CLI, + must_contain=("allow_abbrev=False",), + ), Mechanism( id="resolve.wrapper-carries-no-filter", claim=( @@ -794,7 +876,10 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje backed_by=( "merge.allowlist-absent", "merge.unpinned-head-refused-by-wrapper", + "merge.abbreviated-unpinned-head-refused-by-wrapper", + "merge.abbreviation-is-not-resolved-by-the-cli", "merge.wrapper-filters-unpinned-head-in-bash", + "merge.parser-refuses-abbreviation", ), ), EntryPoint( @@ -813,6 +898,7 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje "resolve.autonomous-bulk-refused", "classify.interactive-does-not-require-outdated", "resolve.wrapper-carries-no-filter", + "resolve.parser-refuses-abbreviation", ), ), EntryPoint( @@ -899,6 +985,20 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje claim="Pure classification over supplied input; no write of any kind.", backed_by=(), ), + EntryPoint( + path=READINESS_GATE, + wrapper=None, + mutation=READ_ONLY, + mutates_what="nothing", + gate="n/a", + claim=( + "The lane's one entry point outside the skill's own scripts directory, " + "invoked by name from SKILL.md. It counts findings and classification " + "rows over fetched comments and reports READINESS_OK / " + "READINESS_BLOCKED; it writes no file and performs no GitHub write." + ), + backed_by=(), + ), ) @@ -1003,14 +1103,22 @@ def render_markdown() -> str: " emits plain stderr and no JSON envelope. On a CLI entry point the layer is" " always `python-cli` and only the exit code and message are asserted.", "", - "| ID | Entry point | Invocation | Exit | Refused by | Error names | Enforced at | Claim |", - "| --- | --- | --- | --- | --- | --- | --- | --- |", + "`No gh` marks the rows whose claim is that the refusal precedes every network" + " call. Those are replayed a second time with a recording `gh` shim as the only" + " executable on `PATH`, and the assertion is that the shim was never invoked --" + " merely removing `gh` from `PATH` would let a swallowed lookup failure fall" + " through to the same exit code and read as proof.", + "", + "| ID | Entry point | Invocation | Exit | Refused by | No gh | Error names" + " | Enforced at | Claim |", + "| --- | --- | --- | --- | --- | --- | --- | --- | --- |", ] for row in REFUSALS: names = ", ".join(f"`{token}`" for token in row.error_contains) or "--" lines.append( f"| `{row.id}` | `{row.entry_point}` | {_argv(row.argv)} | {row.exit_code} " - f"| {row.refused_by} | {names} | `{row.enforced_at}` | {_cell(row.claim)} |" + f"| {row.refused_by} | {'asserted' if row.gh_free else '--'} | {names} " + f"| `{row.enforced_at}` | {_cell(row.claim)} |" ) lines += [ @@ -1037,17 +1145,22 @@ def render_markdown() -> str: "## Effects: what reaches disk", "", "Executed offline against a throwaway state directory seeded with `Fixture`." - " `State changes` is the observed before/after difference of that directory," - " not a reading of the flag names.", + " `State delta` is the observed before/after movement of that directory's file" + " set, not a reading of the flag names, and it is directional rather than a" + " changed/unchanged boolean: a row claiming deletion asserts that the file set" + " strictly shrank, so a reap that rewrote the expired lease -- or touched some" + " unrelated file -- fails it instead of passing on \"something changed\". The" + " assertion is over the file set, deliberately not over named paths: a path" + " literal here would couple this contract to the lease writer's internal" + " layout. Advisory `.lock` siblings are excluded from the comparison.", "", - "| ID | Entry point | Invocation | Fixture | Exit | State changes | Claim |", + "| ID | Entry point | Invocation | Fixture | Exit | State delta | Claim |", "| --- | --- | --- | --- | --- | --- | --- |", ] for row in EFFECTS: lines.append( f"| `{row.id}` | `{row.entry_point}` | {_argv(row.argv)} | {row.fixture} " - f"| {row.exit_code} | {'yes' if row.state_changes else 'no'} " - f"| {_cell(row.claim)} |" + f"| {row.exit_code} | {row.delta} | {_cell(row.claim)} |" ) lines += [ @@ -1087,14 +1200,30 @@ def render_markdown() -> str: " the deferral note in `scripts/tests/guard_contract.py`.", "- Anything requiring a live GitHub response. Every row above runs with no" " network access, which is what keeps the suite free of a `gh` stub.", - "- Four rendered columns are annotation rather than assertion, and a rule" + "- Five rendered columns are annotation rather than assertion, and a rule" " must not be written against them as if CI proved them: the entry-point" - " table's **Mutates**, **Gate**, and **Claim**, and the refusal table's" - " **Enforced at**. The entry-point rows assert the path, the wrapper, the" - " class, and that a mutating row names backing rows; nothing reads the" - " prose in those three columns. `Predicate.enforced_at` *is* checked --" - " `test_anchor_symbol_exists` resolves it -- but `Refusal.enforced_at` is" - " not, so a refusal's location may drift without failing CI.", + " table's **Class**, **Mutates**, **Gate**, and **Claim**, and the refusal" + " table's **Enforced at**. The entry-point rows assert the path and the" + " wrapper; nothing reads the prose in those columns." + " `Predicate.enforced_at` *is* checked -- `test_anchor_symbol_exists`" + " resolves it -- but `Refusal.enforced_at` is not, so a refusal's location" + " may drift without failing CI.", + "- **Class** carries two partial bindings and one real gap, so read it" + " precisely. Bound: a non-read-only row must name at least one backing" + " row, and a `read-only` row may not be cited by any effect row that moves" + " state or by any mechanism row -- so a read-only entry point that grows" + " mutation evidence fails CI. Not bound: that a `mutating` or" + " `conditionally mutating` row's cited evidence actually demonstrates the" + " class it declares. It cannot be, for the four entry points whose" + " mutation is a GitHub write -- the merge gate, the thread resolver," + " `refresh_pr_branch.py`, and `request_review.py`. Every row in this" + " contract runs with no network access, by design, so no offline assertion" + " can witness their mutation; binding the column would mean either a `gh`" + " stub (asserting the stub, not the behavior) or a live-credential CI" + " suite. An entry point that turns from conditional to unconditional" + " mutation therefore still passes. Trigger to revisit: the first time a" + " class changes without a matching row change, or the first offline-" + "observable mutation appearing on a GitHub-writing entry point.", "", ] return "\n".join(lines) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py index 90bda59a67..8bc7b8555b 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py @@ -70,6 +70,27 @@ def state_fingerprint(state_dir: pathlib.Path) -> dict[str, bytes]: } +def observed_delta(before: dict[str, bytes], after: dict[str, bytes]) -> str: + """Which way the state directory's file set moved. + + Directional, because a boolean cannot tell deletion from a rewrite: a reap + that rewrote the expired lease instead of removing it would satisfy + `before != after` and pass a row whose claim is deletion. Compared over the + key set rather than named paths -- a path literal in a drift-detection + artifact couples it to the writer's internal layout. + """ + if before == after: + return contract.UNCHANGED + kept, arrived = set(before), set(after) + if arrived > kept: + return contract.ADDED + if arrived < kept: + return contract.REMOVED + if arrived == kept: + return contract.REWRITTEN + return f"mixed: +{sorted(arrived - kept)} -{sorted(kept - arrived)}" + + def seed(fixture: str, state_dir: pathlib.Path) -> None: if fixture == contract.EMPTY_STATE: return @@ -99,6 +120,30 @@ def seed(fixture: str, state_dir: pathlib.Path) -> None: raise AssertionError(f"unknown fixture: {fixture}") +def write_gh_shim(directory: pathlib.Path, sentinel: pathlib.Path) -> None: + """A `gh` that records the fact it ran and then fails. + + Exit 127 mimics the not-found status a caller is most likely to swallow, so + a guard that resolved `@me` before checking scope reaches the same refusal + exit code -- and is caught by the sentinel rather than by the exit code. + """ + posix = directory / "gh" + posix.write_text( + f'#!/usr/bin/env bash\nprintf called > "{sentinel.as_posix()}"\nexit 127\n', + encoding="utf-8", + newline="\n", + ) + posix.chmod(0o755) + if os.name == "nt": + # Windows resolves a bare `gh` through PATHEXT, never the extensionless + # file above, so the shim needs a .cmd sibling to be reachable at all. + (directory / "gh.cmd").write_text( + f'@echo off\r\n(echo called) > "{sentinel}"\r\nexit /b 127\r\n', + encoding="utf-8", + newline="", + ) + + def because(row_id: str, claim: str, detail: str = "") -> str: suffix = f"\n observed: {detail}" if detail else "" return ( @@ -161,16 +206,51 @@ def test_every_refusal_row(self) -> None: ) def test_scope_refusal_precedes_every_network_call(self) -> None: - # Belt-and-braces on the property that makes this suite stub-free: with - # PATH emptied of gh, the fail-closed rows must behave identically. - env = dict(os.environ, PATH=tempfile.gettempdir()) - proc = subprocess.run( - [sys.executable, str(contract.plugin_path(contract.MERGE_CLI)), "owner/repo#1"], - capture_output=True, - text=True, - env=env, - ) - self.assertEqual(proc.returncode, 3, proc.stderr) + # The rows claiming "no gh invocation at all" are replayed against a + # recording shim rather than an emptied PATH. Emptying PATH proves only + # that the refusal survives gh being unreachable -- a swallowed lookup + # failure falling through to the same exit code would pass it while the + # claim stayed false. The shim is the only executable on PATH, so any + # attempt to resolve `@me` leaves a sentinel behind. + rows = [row for row in contract.REFUSALS if row.gh_free] + self.assertTrue(rows, "no refusal row claims network-free refusal") + for row in rows: + with self.subTest(row=row.id): + if BASH is None: + self.skipTest("bash unavailable; the gh shim needs it") + with tempfile.TemporaryDirectory() as tmp: + shim_dir = pathlib.Path(tmp) + sentinel = shim_dir / "gh-was-called" + write_gh_shim(shim_dir, sentinel) + env = dict(os.environ, PATH=str(shim_dir)) + proc = subprocess.run( + [ + sys.executable, + str(contract.plugin_path(row.entry_point)), + *row.argv, + ], + capture_output=True, + text=True, + env=env, + cwd=tempfile.gettempdir(), + ) + called = sentinel.exists() + self.assertFalse( + called, + because(row.id, row.claim, "the gh shim was invoked"), + ) + self.assertEqual( + proc.returncode, + row.exit_code, + because(row.id, row.claim, f"exit {proc.returncode}: {proc.stderr[:400]}"), + ) + payload = envelope(proc) + for key, value in row.envelope_fields: + self.assertEqual( + payload.get(key), + value, + because(row.id, row.claim, f"{key}={payload.get(key)!r}"), + ) class PredicatesHoldOverRuntimeData(unittest.TestCase): @@ -214,12 +294,12 @@ def test_every_effect_row(self) -> None: because(row.id, row.claim, f"exit {proc.returncode}: {proc.stderr[:400]}"), ) self.assertEqual( - before != after, - row.state_changes, + observed_delta(before, after), + row.delta, because( row.id, row.claim, - f"state {'changed' if before != after else 'was untouched'}", + f"observed {observed_delta(before, after)!r}", ), ) @@ -249,6 +329,14 @@ def test_every_executable_script_is_classified(self) -> None: for path in contract.SCRIPTS.glob("*.py") if "__main__" in path.read_text(encoding="utf-8") } + # The lane is not confined to the skill's own scripts directory: SKILL.md + # invokes the plugin-level readiness gate by name, and a shell entry point + # there would otherwise evade this gate entirely. + present |= { + f"scripts/{path.name}" + for path in (contract.PLUGIN_ROOT / "scripts").glob("babysit-*.sh") + if not path.name.endswith(".test.sh") + } self.assertEqual( present - catalogued, set(), @@ -303,6 +391,25 @@ def test_every_mutating_entry_point_is_backed(self) -> None: f"`{entry.path}` is classified {entry.mutation} with nothing asserting it", ) + def test_read_only_entry_points_cite_no_mutation_evidence(self) -> None: + # The one direction of the Class column CI can bind offline. The reverse + # -- proving a mutating class -- cannot be bound for the four entry + # points whose mutation is a GitHub write, because every row here runs + # without network access; the generated doc's "Not covered here" section + # states that gap rather than letting the column read as proven. + moves_state = {row.id for row in contract.EFFECTS if row.delta != contract.UNCHANGED} + moves_state |= {row.id for row in contract.MECHANISMS} + for entry in contract.ENTRY_POINTS: + if entry.mutation != contract.READ_ONLY: + continue + with self.subTest(entry=entry.path): + self.assertEqual( + set(entry.backed_by) & moves_state, + set(), + f"`{entry.path}` is classified read-only but cites rows asserting" + " a mutation; reclassify it or move the evidence", + ) + WRAPPER_COMMAND = re.compile( r'bash "\$\{CLAUDE_PLUGIN_ROOT\}/(bin/source-control-babysit-[a-z-]+)"' @@ -332,13 +439,36 @@ def documented_commands(text: str) -> list[tuple[str, str]]: class DocumentedCommandsMatchTheParsers(unittest.TestCase): def _accepted_flags(self, cli: str) -> set[str]: + """The parser's registered long options, read from its usage block only. + + Scraping the whole `--help` output would treat any flag NAMED IN PROSE as + accepted: every one of these CLIs describes the broad `gh pr merge + --admin` grant it exists to replace, so `--admin` would be admitted here + while the parser exits 2 on it, and a document carrying that stale flag + would pass. Argparse renders the usage block from the registered option + strings and nothing else, so it -- not the description, not the help + text -- is the registry. It runs from `usage:` to the first blank line. + """ proc = subprocess.run( [sys.executable, str(contract.plugin_path(cli)), "--help"], capture_output=True, text=True, ) self.assertEqual(proc.returncode, 0, f"{cli} --help failed: {proc.stderr}") - return set(re.findall(r"--[a-z0-9][a-z0-9-]*", proc.stdout)) + self.assertTrue( + proc.stdout.startswith("usage:"), + f"{cli} --help does not open with a usage block; the registry moved", + ) + usage = proc.stdout.split("usage:", 1)[1].split("\n\n", 1)[0] + flags = set(re.findall(r"(? None: + # The regression this parse exists to prevent, pinned: `--admin` appears + # in the merge gate's description and is rejected by its parser. + self.assertNotIn("--admin", self._accepted_flags(contract.MERGE_CLI)) def test_every_documented_wrapper_command(self) -> None: accepted: dict[str, set[str]] = {} From 5cda868eab5fa3c931cc389de0cee69616b40e16 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 25 Jul 2026 19:04:41 -0400 Subject: [PATCH 7/8] fix(source-control): make the recording gh shim reachable under the isolated PATH Two review findings, both about the network-free replay asserting a negative that could never fire. The shim's `/usr/bin/env bash` shebang resolved `bash` through PATH, which the replay narrows to the shim directory alone. On POSIX the shim therefore failed to exec, wrote no sentinel, and every row passed while proving nothing -- the false negative reproduced by the shim added to close the finding it answers. The interpreter is now named absolutely. Because the replay asserts a negative, a broken shim is silent. It now carries its own reachability probe: the shim is resolved and invoked through `babysit_gh.gh_capture`, the lane's own subprocess seam, and the probe fails if the sentinel is absent. A change to how `gh` is located cannot leave the probe agreeing with a shim the product would miss. `merge.autopilot-tier-without-required-sets` claims refusal "before any network access" but was not opted into the replay, so tier validation moving below a gh query would have kept CI green while the rendered ordering guarantee went false. Co-authored-by: Claude Opus 5 (1M context) --- .../babysit-prs/reference/guard-contract.md | 2 +- .../scripts/tests/guard_contract.py | 1 + .../babysit-prs/scripts/tests/test_guards.py | 40 ++++++++++++++++++- 3 files changed, 41 insertions(+), 2 deletions(-) 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 917e57acfa..d8d3c4edc0 100644 --- a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -45,7 +45,7 @@ Each row is executed. `Refused by` says which layer rejected the invocation. On | `merge.owner-check-precedes-self-login-resolution` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners someone-else --self-logins @me` | 3 | python-cli | asserted | -- | `babysit_merge.py::main` | The owner check runs before '@me' self-login resolution, which is a network call: an out-of-scope owner refuses at exit 3 with no gh invocation at all. | | `merge.short-expected-head` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --merge --expected-head abc` | 2 | python-cli | -- | `expected-head` | `babysit_merge.py::main` | --expected-head must be a long-enough SHA prefix; a short value is a usage error at exit 2, never a loose prefix match against the live head. | | `merge.malformed-ref` | `skills/babysit-prs/scripts/babysit_merge.py` | `not-a-ref --allowed-owners owner` | 2 | python-cli | -- | -- | `babysit_merge.py::main` | The PR reference must parse as owner/repo#number or a PR URL; anything else is a usage error at exit 2. | -| `merge.autopilot-tier-without-required-sets` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier` | 3 | python-cli | -- | `--lane-logins`, `--approver-bot-logins`, `--block-labels` | `babysit_merge.py::main` | The autopilot merge tier is fail-closed: the umbrella flag alone, with none of --lane-logins, --approver-bot-logins, or --block-labels, refuses at exit 3 before any network access. | +| `merge.autopilot-tier-without-required-sets` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier` | 3 | python-cli | asserted | `--lane-logins`, `--approver-bot-logins`, `--block-labels` | `babysit_merge.py::main` | The autopilot merge tier is fail-closed: the umbrella flag alone, with none of --lane-logins, --approver-bot-logins, or --block-labels, refuses at exit 3 before any network access. | | `merge.autopilot-tier-partial-config` | `skills/babysit-prs/scripts/babysit_merge.py` | `owner/repo#1 --allowed-owners owner --autopilot-merge-tier --lane-logins lane --approver-bot-logins bot` | 3 | python-cli | -- | `--block-labels` | `babysit_merge.py::main` | A partially configured autopilot tier still refuses at exit 3, naming only the missing set -- there is no partial-credit tier. | | `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. | 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 ec7d5c73e4..f310ac9cf4 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 @@ -269,6 +269,7 @@ class DocCommandSource: error_contains=("--lane-logins", "--approver-bot-logins", "--block-labels"), refused_by=PYTHON_CLI, enforced_at="babysit_merge.py::main", + gh_free=True, ), Refusal( id="merge.autopilot-tier-partial-config", diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py index 8bc7b8555b..cb0b2716a4 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py @@ -126,10 +126,15 @@ def write_gh_shim(directory: pathlib.Path, sentinel: pathlib.Path) -> None: Exit 127 mimics the not-found status a caller is most likely to swallow, so a guard that resolved `@me` before checking scope reaches the same refusal exit code -- and is caught by the sentinel rather than by the exit code. + + The interpreter is named absolutely. A `/usr/bin/env bash` shebang resolves + `bash` through PATH, which the replay below deliberately narrows to this + directory alone: the shim would fail to exec, record nothing, and every row + would pass without proving anything. """ posix = directory / "gh" posix.write_text( - f'#!/usr/bin/env bash\nprintf called > "{sentinel.as_posix()}"\nexit 127\n', + f'#!{BASH}\nprintf called > "{sentinel.as_posix()}"\nexit 127\n', encoding="utf-8", newline="\n", ) @@ -205,6 +210,39 @@ def test_every_refusal_row(self) -> None: because(row.id, row.claim, "no JSON envelope: bash refused"), ) + def test_the_recording_shim_is_reachable(self) -> None: + # Guards the guard. The replay below asserts a NEGATIVE -- the sentinel + # was never written -- so a shim the isolated PATH cannot execute makes + # every row pass while proving nothing. Resolve and invoke it through + # the lane's own subprocess seam, so a change to how `gh` is located + # cannot leave this probe agreeing with a shim the product would miss. + if BASH is None: + self.skipTest("bash unavailable; the gh shim needs it") + with tempfile.TemporaryDirectory() as tmp: + shim_dir = pathlib.Path(tmp) + sentinel = shim_dir / "gh-was-called" + write_gh_shim(shim_dir, sentinel) + probe = subprocess.run( + [ + sys.executable, + "-c", + "import sys; sys.path.insert(0, sys.argv[1]);" + " import babysit_gh; babysit_gh.gh_capture(['--version'])", + str(contract.SCRIPTS), + ], + capture_output=True, + text=True, + env=dict(os.environ, PATH=str(shim_dir)), + cwd=tempfile.gettempdir(), + ) + recorded = sentinel.exists() + self.assertTrue( + recorded, + "the recording gh shim is unreachable under the isolated PATH, so" + " test_scope_refusal_precedes_every_network_call cannot detect a gh" + f" call at all: {probe.stderr[:400]}", + ) + def test_scope_refusal_precedes_every_network_call(self) -> None: # The rows claiming "no gh invocation at all" are replayed against a # recording shim rather than an emptied PATH. Emptying PATH proves only From 3d9bae8a0a56a5146d8367eaa098d67a7f0decde Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 25 Jul 2026 19:21:12 -0400 Subject: [PATCH 8/8] fix(source-control): check documented commands at the wrapper boundary A documented wrapper command was validated against the backing CLI's parser alone, so `--allow-unpinned-head` spelled through `bin/source-control-babysit-merge` passed the contract while always exiting 2 -- the parser registers the flag, the wrapper refuses it before Python runs. The wrapper's accepted set is the narrower of the two, and blessing that invocation is exactly what the wrapper exists to prevent. `WRAPPER_DENIED_FLAGS` records the narrowing as prefix families, mirroring the wrapper's own bash test, and renders as a table so a reader of the contract sees it. It is not free-standing data: `test_wrapper_denied_flags_are_proven_by_a_ refusal_row` requires every listed flag to be one a `bash-wrapper` refusal row actually invokes the wrapper to prove. --- .../babysit-prs/reference/guard-contract.md | 7 ++++ .../scripts/tests/guard_contract.py | 32 +++++++++++++++++++ .../babysit-prs/scripts/tests/test_guards.py | 32 +++++++++++++++++++ 3 files changed, 71 insertions(+) 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 fc46574b5a..5483c5984d 100644 --- a/plugins/source-control/skills/babysit-prs/reference/guard-contract.md +++ b/plugins/source-control/skills/babysit-prs/reference/guard-contract.md @@ -108,6 +108,13 @@ Asserted against the entry point's source, because the mechanism -- not just the Documents that spell out copyable `bin/`-path wrapper commands. Every wrapper path is checked to resolve and every flag against the backing CLI's own parser, so a renamed or removed flag fails here instead of in an operator's terminal. +Checked against the WRAPPER boundary, not the parser alone. A wrapper's accepted set is the narrower of the two, so a flag the CLI registers but the wrapper refuses -- `bin/source-control-babysit-merge` and `--allow-unpinned-head`, including its prefixes -- is a documented command that always exits 2, and the parser check alone would bless it. Refusals are the source: every flag listed below is one a `bash-wrapper` refusal row proves that wrapper rejects. + +| Wrapper | Refuses | +| --- | --- | +| `bin/source-control-babysit-merge` | `--allow-unpinned-head` (and prefixes) | +| `bin/source-control-babysit-resolve-thread` | nothing | + | ID | Document | Claim | | --- | --- | --- | | `safety.pinned-command-degradation` | `skills/babysit-prs/reference/safety.md` | reference/safety.md hard-codes fully-argument-pinned bin/-path wrapper command lines for the operator to run when the runtime denies a mutation the gate already proved ready. Every wrapper path in it resolves, and every flag it names is one the backing CLI's parser accepts. | 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 8e91656135..226dbf2db1 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 @@ -1040,6 +1040,21 @@ def _thread(*, resolved: bool, bot_only: bool, outdated: bool) -> dict[str, obje RESOLVE_WRAPPER: RESOLVE_CLI, } +# Flags the backing CLI accepts but the wrapper refuses, so a documented wrapper +# command naming one is a command that always exits 2. The wrapper boundary is +# narrower than the parser, and validating documented commands against the +# parser alone would bless exactly the invocation the wrapper exists to prevent. +# Matched as prefix families, mirroring the wrapper's own bash test. +WRAPPER_DENIED_FLAGS: dict[str, tuple[str, ...]] = { + MERGE_WRAPPER: ("--allow-unpinned-head",), + RESOLVE_WRAPPER: (), +} + + +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]) + _PREAMBLE = """# Guard contract