feat(source-control): bind babysit guard semantics to an executable contract#1285
feat(source-control): bind babysit guard semantics to an executable contract#1285kyle-sexton wants to merge 6 commits into
Conversation
…ontract 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
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.
|
Claude finished @kyle-sexton's task in 4m 23s —— View job PR Review: feat(source-control): bind babysit guard semantics to an executable contractTasks
OverviewThis is a well-conceived addition that converts a prose-only contract into an executable one. The five binding kinds (Refusals, Predicates, Effects, Mechanisms, DocCommandSources) are appropriately chosen per fact rather than applied uniformly — that's the right design decision. The A few things to consider: Findings1.
gate=(
"none for acquire / heartbeat / release -- they write unconditionally; "
"--apply gates reap's deletion ONLY"
),The The 2. lease_path = pathlib.Path(str(json.loads(proc.stdout)["path"]))If 3. Refusal(
id="resolve.allowlist-absent",
...
error_contains=(), # ← empty
envelope_fields=(("inScope", False),),
),
4. if row.entry_point.startswith("bin/"):
if row.refused_by == contract.BASH_WRAPPER:
...
else:
...For non- 5. present = {
f"skills/babysit-prs/scripts/{path.name}"
for path in contract.SCRIPTS.glob("*.py")
if "__main__" in path.read_text(encoding="utf-8")
}
Minor observations (no action required)
SummaryItems 1–4 are the actionable ones. Items 1 and 4 can be addressed as follow-up (item 1 is explicitly deferred territory; item 4 is a two-line assertion), while item 2 (parse fragility in CHANGELOG entry, version bump (0.26.0 → 0.27.0), |
|
Claude finished @kyle-sexton's task in 3m 27s —— View job Security Review — PR #1285Tasks
ScopeAll 7 changed files reviewed:
FindingsNo CRITICAL or IMPORTANT security issues found. This PR is entirely test infrastructure. All subprocess calls use the list form (no SUGGESTION — confidence: medium
SUGGESTION — confidence: low
Positive observations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 455022b13d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path=MERGE_CLI, | ||
| wrapper=MERGE_WRAPPER, | ||
| mutation=CONDITIONAL, | ||
| mutates_what="merges the PR on GitHub; writes queue state", |
There was a problem hiding this comment.
The mutation catalogue says babysit_merge.py writes queue state, but that entry point has no state-directory argument or state-writing path; after gh pr merge it only prints the result and returns (babysit_merge.py:1077-1092). Consumers of this new contract will therefore infer a local mutation that never occurs, and the tests do not catch the false claim because test_every_mutating_entry_point_is_backed merely checks that backed_by is nonempty.
Useful? React with 👍 / 👎.
| 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." |
There was a problem hiding this comment.
When the configured state file contains malformed JSON, a flagless snapshot is not read-only: build_snapshot always calls babysit_state.load_state, whose corruption path renames the file to a .corrupt sibling (babysit_state.py:78-102). Thus --write-state does not gate every mutation, and a permission classifier relying on this row can authorize an invocation under the false assumption that it cannot alter local state.
Useful? React with 👍 / 👎.
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.
|
Claude finished @kyle-sexton's task in 3m 50s —— View job PR Review: feat(source-control): bind babysit guard semantics to an executable contractTasks
What the latest commits addressed
Confirmed Codex findings1. mutates_what="merges the PR on GitHub; writes queue state",
Fix: change 2. 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("),
The practical impact for a permission classifier is limited (the quarantine is a rename, not a GitHub write, and only occurs on corrupt state), but the contract explicitly promises to be what CI proves — and CI does not prove this. Fix options: (a) narrow the claim to "no GitHub write on any path; the only intentional local write is the local snapshot file, gated by Still-open findings from prior reviewsThese were not addressed in the latest two commits — noting them here for completeness so they are not lost:
The two Codex items above are the ones most worth resolving before merge: Item 1 is a factual error in the published contract, and Item 2 is an inaccuracy in a claim the contract's own prose says CI proves. |
|
Claude finished @kyle-sexton's task in 4m 0s —— View job Security Review — PR #1285Tasks
ScopeAll 6 changed files reviewed:
FindingsNo CRITICAL or IMPORTANT security issues found. SUGGESTION —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5515b19d37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| wrapper=None, | ||
| mutation=CONDITIONAL, | ||
| mutates_what="records feedback dispositions and fix rounds in queue state", | ||
| gate="--apply, additionally requiring --lease-token", |
There was a problem hiding this comment.
Unlike the already-reported corrupt-snapshot path, even a valid flagless ledger invocation mutates the filesystem: manage_feedback_ledger.run() enters state_lock(queue-state.json) before checking args.apply, and that lock creates the state directory and queue-state.json.lock when absent. Therefore --apply is not the condition under which mutation happens at all, so a permission classifier relying on this row can incorrectly treat a dry-run invocation as non-writing; either account for lock artifacts in the gate or explicitly narrow the contract to domain-state mutations.
Useful? React with 👍 / 👎.
| WRAPPER_COMMAND = re.compile( | ||
| r'bash "\$\{CLAUDE_PLUGIN_ROOT\}/(bin/source-control-babysit-[a-z-]+)"([^`]*)' | ||
| ) |
There was a problem hiding this comment.
When a fenced block contains multiple commands, this pattern's ([^]*)consumes every line after the first wrapper through the closing backtick, sofindall()returns only the first command. This already happens inreference/safety.md`, where the adjacent resolve-wrapper command is swallowed into the merge command's tail; consequently its wrapper path is never checked and any flags are validated against the merge CLI rather than the resolve CLI. Parse each wrapper invocation independently so a typo or stale flag on a later command cannot pass this contract.
Useful? React with 👍 / 👎.
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.
|
Claude finished @kyle-sexton's task in 2m 47s —— View job Security Review — PR #1285Tasks
ScopeAll 6 changed files reviewed across all 4 commits (d1745d5 → 455022b → 5515b19 → 4bf9eb9):
No CRITICAL or IMPORTANT security issues found.This is test infrastructure. All subprocess calls use the list form throughout — no The two IMPORTANT factual errors in the contract document ( SUGGESTION —
|
|
Claude finished @kyle-sexton's task in 2m 26s —— View job PR Review: feat(source-control): bind babysit guard semantics to an executable contractTasks
Latest commit (
|
| Finding | Location | Status |
|---|---|---|
heartbeat/release write unconditionally but have no effect rows; not mentioned in "Not covered here" |
guard_contract.py L824–825 |
Open — low priority |
seed() parses proc.stdout without .strip() — fragile if output grows trailing whitespace |
test_guards.py L93 |
Open |
resolve.allowlist-absent has empty error_contains; merge.allowlist-absent names "allowed-owners" — asymmetric coverage |
guard_contract.py L333 |
Open |
refused_by for non-bin/ rows is never validated — a typo silently documents the wrong refuser |
test_guards.py L143 |
Open |
"__main__" in text catches docstrings/comments; 'if __name__ == "__main__"' in text would be tighter |
test_guards.py L250 |
Open |
arg.format(state_dir=tmp) uses full format mini-language; arg.replace("{state_dir}", tmp) would be safer |
test_guards.py L120 |
Open |
seed() builds lease_path from subprocess stdout without asserting it is inside the temp dir |
test_guards.py L93 |
Open |
The heartbeat/release gap is the most worth a one-liner mention in "Not covered here" before merge — a consumer looking for heartbeat write coverage in the contract should find a note saying it was consciously deferred rather than hitting a dead end.
Summary
The latest commit cleanly addresses all four actionable findings from the Codex and prior Claude reviews: the false state-write claim on babysit_merge.py, the lock-file/domain mutation scope ambiguity, the snapshot "pure read" inaccuracy, and the multi-command parser that silently skipped the resolve-thread wrapper. The contract is now factually accurate on the claims that were wrong. The remaining open items above are robustness and cosmetic improvements that can be addressed as follow-on work.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bf9eb97ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "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, " |
There was a problem hiding this comment.
When the wrapper receives an unambiguous prefix such as --allow-unpinned-hea, its Bash loop does not match the exact forbidden spelling, while Python's argparse accepts the prefix as --allow-unpinned-head and sets the override. Against this commit, the full flag exits from the wrapper but the abbreviated form reaches the Python scope check, so an allow-rule-covered invocation can merge a ready PR without --expected-head, contradicting this contract. Disable argparse abbreviation or reject abbreviated forms in the wrapper and cover that invocation here.
Useful? React with 👍 / 👎.
…oop.md §5.3 (#1307) *This was generated by AI during an autonomous work-loop execution session.* Closes #653 ## Summary `babysit-prs`'s SKILL.md described a cadence-ownership split that no longer exists. #652 added the engine-backed `recommended_cadence` → `ScheduleWakeup.delaySeconds` mapping table to `reference/loop.md` §5.3, alongside the static Python-free degrade ladder already living there. `reference/cadence.md` owns only the cadence *states and thresholds* — and has said so since #322 ("`loop.md` owns the wake mechanics, this file owns the states and thresholds behind the recommendation"). SKILL.md still routed readers the old way: | Surface | Before | After | |---|---|---| | Runbook step 9 | `recommended_cadence` per `cadence.md`; `loop.md` §5.3 named only as the "static ladder … Python-free degrade" | `Schedule the next wake per the cadence contract in reference/loop.md §5.3.` | | Reporting closing line | "Recommend the exact next interval per `cadence.md`" | same sentence, now citing `loop.md` §5.3 | | References entry for `loop.md` | "…checklist, static cadence ladder" | "…checklist, and the §5.3 cadence contract" | | Step-5 progressive-disclosure trigger | load `cadence.md` "only before **recommending cadence**" | load `cadence.md` "only before **interpreting a cadence state**" | The filed issue named step 9. The other three are the same defect: leaving them would have made the file internally inconsistent about which section owns the seconds. **The Reporting line was a live wrong-number risk, not just imprecision.** `cadence.md` states `idle` = **daily**; §5.3 documents `ScheduleWakeup` clamping `delaySeconds` to `[60, 3600]`, so inside `/loop` `idle` and `quiet` both wake hourly. "Recommend the exact next interval per `cadence.md`" could therefore surface an interval the loop will never schedule. The `cadence.md` link is dropped from step 9 rather than kept alongside `loop.md`: §5.3 already hops to `cadence.md` for the states in one line, so a second pointer here is the gratuitous cross-reference `CLAUDE.md`'s pointer-not-copy rule forbids. `cadence.md` keeps its own References entry and its step-5 disclosure trigger. The new step-9 wording matches vocabulary the file already uses at its Step 7 checklist line ("schedule the next wake per the cadence contract (§5.3)") and in the sibling `babysit-loop` SKILL.md ("that mapping owns the seconds"). Docs-only. No behavior change, no script or engine change. `source-control` `0.26.1` → `0.26.2` with the matching CHANGELOG entry. ## Test plan Every command below was run from the branch worktree; all passed. - `plugins/skill-quality/scripts/check-skill.sh babysit-prs` (`CHECK_SKILL_BASE_REF=origin/main`) — **exit 0**, `PASS — 0 errors`. This is the gate CI runs via `scripts/check-changed-skills.sh`. Includes the 500-line hard cap, all 9 base-ref trigger phrases preserved, broken-internal-ref check, and `scripts/engine.test.sh`. - **SKILL.md is now 497 lines** (base `origin/main`: 499). An intermediate revision of this branch landed at exactly 500 and *failed* the hard cap (`LINE_COUNT >= LINE_HARD_CAP`, cap 500) — caught by an independent reviewer before the PR was opened. Collapsing step 9 to a single pointer and the References bullet back to two lines both fixed the cap and made the diff better follow pointer-not-copy. The one remaining `WARN` is the pre-existing 200-line soft target, untouched by this change. - `scripts/check-changelog-parity.sh --check` — exit 0. - `scripts/check-changelog-parity.sh --check-bump origin/main` — exit 0 (the manifest bump has a matching `## [0.26.2]` entry). - `markdownlint-cli2` over both changed markdown files, repo config auto-discovered — exit 0, 0 errors. - `jq empty` on `plugins/source-control/.claude-plugin/plugin.json` — exit 0. - `typos` over both changed markdown files — exit 0. - Every relative link in the edited regions resolved by hand (`reference/loop.md`, `cadence.md`, `safety.md`, `orchestration.md`, `freshness.md`, `stuck-checks.md`, `feedback.md`, `review-trigger.md`); the `§5.3` anchor exists at `reference/loop.md:426`. No external URLs introduced, so lychee's absence changes nothing. - Repo-wide `grep` for any remaining reference sending a reader to `cadence.md` for wake seconds, or describing §5.3 as holding only the static ladder — none remain. The four surviving `cadence.md` citations in `plugins/source-control/` were each checked and are correct. Claims verified against the actual files rather than the issue text: §5.3 does contain both the mapping table (`loop.md:435-440`) and the degrade ladder (`loop.md:456-462`); `cadence.md:4-5` does disclaim the wake mechanics; `cadence.md:27` does state `idle` = daily against `loop.md:449-452`'s `[60, 3600]` clamp. The CHANGELOG entry's causal account was corrected after `git show --stat e9cef6e` showed #652 never touched `cadence.md`, and `git log -L4,5` dated that file's disclaimer to `fe78acb6` (#322) — so the references were always imprecise; #652 only made the correct target concrete. Per this repo's fresh-docs mandate, <https://code.claude.com/docs/en/skills> was fetched this session. It confirms the progressive-disclosure model this diff operates on — SKILL.md body plus bundled supporting files loaded on demand — and imposes no constraint on how a SKILL.md cites its own `reference/` files, so the choice of pointer is a repo-convention question, decided above by `CLAUDE.md`'s pointer-not-copy rule. ## Related - Refs #652 / #504 — the PR and issue that added the §5.3 mapping table this diff points at. - Refs #322 — where `reference/cadence.md` first disclaimed ownership of the wake mechanics. - Version note: open PRs #1285 and #1264 both claim `source-control` `0.27.0`. If either merges first this branch's `0.26.2` becomes a regression — it self-surfaces as a git conflict on both the manifest line and the CHANGELOG insert point, so rebase rather than force-merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Declined: a
|
Independent verification — VERDICT: PASS-WITH-FINDINGSAudited by a fresh context that did not write this branch, given the artifact and #1265's requirement only, rationale withheld. The requirement is met in substance: all four fact families #1265 names are present, and the two that were gotten wrong three times — resolver flag semantics and refusal location (bash wrapper vs Python CLI) — are genuinely executably bound. Findings are over-claim and coverage-hole tier. CRITICAL 1 — the generated doc over-claims about itself
The entry-point table's Failure scenario: a consumer writes a permission rule citing CRITICAL 2 — "which helpers mutate" is behavior-bound for one script onlyAll three Confirmed by mutation testing on a scratch copy of the plugin — three guard-destroying edits, applied simultaneously, all survive green (
IMPORTANT 3 — the checker cannot catch the one flag the wrapper refuses
IMPORTANT 4 — the doc sweep misses the spelling the docs use most
IMPORTANT 5 — the one
|
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) <noreply@anthropic.com>
Version queue: main reached 0.26.4 and PR #1264 holds 0.27.0 ahead of this branch, so this section moves to 0.28.0. Landing order is #1264 then this. One content conflict, in the plugin CHANGELOG, where both sides added a new top section. Both are kept, newest first. No source file conflicted. Verified after the merge: `python -m unittest discover -s tests` 366 OK (the suite grew with main's arrivals), the generated guard contract regenerated from guard_contract.py and unchanged by the merge, markdownlint clean on both changed documents. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 5m 47s —— View job PR Review: feat(source-control): bind babysit guard semantics to an executable contractTasks
What the latest commits addressedThe merge commit
All four findings from the prior review round (false state-write claim, lock-file scope, snapshot "pure read", multi-command parser) remain fixed. The contract is factually accurate on all previously-wrong claims. New findings from the merge (things the contract does not yet cover)1. Severity-marking guard added by main is tested but has no contract row (IMPORTANT)
if autonomous and thread.get("severityFlagged", True):
return "skipped-severity-marked"Under There is no corresponding predicate row in The fail-closed default is the most rule-relevant fact: a flagless invocation against a thread whose comment page was truncated behaves as if severity-marked. This is exactly the kind of non-obvious guard behavior the contract exists to document. The existing autonomous-mode predicate rows ( Minimum fix: add a predicate row with 2. Wrapper prefix-catch fix is not bound by any contract assertion (IMPORTANT) The wrapper now checks: if [[ "$arg" == --a* && "--allow-unpinned-head" == "$arg"* ]]; thenThe existing refusal row If the wrapper reverted to exact equality (removing the Minimum fix: add a second refusal row with Still-open items from independent verification (none are blockers)The independent verification's CRITICAL 2 (mutation testing surviving for prune, snapshot, review, refresh) remains true: those entry points are covered only by MECHANISMS substring checks, not EFFECTS invocation rows. This is now a disclosed gap — the "Not covered here" section correctly says the Mutates and Gate columns are annotation, not proof. The practical risk to a consumer is lower now that the disclosure is accurate. The following minor items from prior review rounds are still open:
SummaryThe two new findings above are the most actionable ones before merge. Finding 1 is the more important: the severity-marking guard is the kind of non-obvious behavioral gate the contract exists to document, and its fail-closed default ( The contract's core structure is sound. The disclosure fix in |
|
Claude finished @kyle-sexton's task in 5m 20s —— View job Security Review — PR #1285Tasks
ScopeAll 6 changed files reviewed at HEAD (
No CRITICAL or IMPORTANT security issues found.This is test infrastructure. All subprocess calls use the list form throughout — no The two IMPORTANT factual errors in the contract document ( The flag-abbreviation bypass (the only IMPORTANT 3 security gap named in the independent review — SUGGESTION —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b3ea15223
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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)) |
There was a problem hiding this comment.
Scraping the entire --help output treats option names mentioned only in prose as accepted flags. For example, babysit_merge.py --help mentions the prohibited --admin, so _accepted_flags() includes it even though invoking the CLI with --admin exits 2 with “unrecognized arguments”; a documented command containing that stale flag would therefore pass this contract despite being unusable. Inspect the parser's registered option strings or actually parse each documented command instead.
Useful? React with 👍 / 👎.
| present = { | ||
| f"skills/babysit-prs/scripts/{path.name}" | ||
| for path in contract.SCRIPTS.glob("*.py") | ||
| if "__main__" in path.read_text(encoding="utf-8") |
There was a problem hiding this comment.
The completeness scan only considers Python files under skills/babysit-prs/scripts, but the skill directly invokes the product entry point plugins/source-control/scripts/babysit-readiness-gate.sh in SKILL.md:349. Consequently that existing shell entry point is absent from the generated mutation catalogue, and any future babysit shell entry point outside this one Python directory will also evade the advertised completeness gate.
Useful? React with 👍 / 👎.
| 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", |
There was a problem hiding this comment.
When an existing entry point changes from read-only to mutating, or from conditional to unconditional mutation, this check remains green because it only requires some backed_by ID and never verifies that the cited row proves the declared class. Fresh evidence in this final revision is that guard-contract.md explicitly describes the Class column as asserted while excluding only Mutates, Gate, and Claim; consumers can therefore rely on a class that CI does not actually bind to behavior.
Useful? React with 👍 / 👎.
1 participant
Closes #1265
Summary
The facts a host permission classifier has to 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. This makes them
a table that CI executes, and renders that table to a citable reference doc.
Issue #1265 sketched three options. Option 1 (a capability manifest derived by argparse
introspection) was investigated and falsified: across the nine CLI entry points,
add_mutually_exclusive_groupappears in two parsers and neither guards a mutation. Everyinter-flag constraint on a mutating path is imperative post-
parse_args()code, and the guardpredicates that matter are boolean expressions over runtime API data —
(autonomous or only_outdated) and not thread["isOutdated"]— which argparse cannot see. Mutating-vs-read-only isnot derivable from flag names either:
manage_babysit_lease.py's--applyhelp says "reap only",yet
acquire,heartbeat, andreleasewrite lease files unconditionally, so a flag-name-derivedmanifest would call
acquireread-only. The real axis is not manifest-vs-prose; it is whether eachclaim is bound to an executable assertion.
Fix
skills/babysit-prs/scripts/tests/guard_contract.pyholds the table as the single source of truth.test_guards.pyexecutes every row.skills/babysit-prs/reference/guard-contract.mdis generatedfrom the same tables and asserted current, so consumers cite a document CI proves is what the code
does. Every row carries the prose claim it backs — a broken guard fails naming the downstream claim,
not
AssertionError: 3 != 2.Five binding kinds, chosen per fact rather than uniformly:
bin/row additionally asserts which layer refused, using the observable discriminator that abash-wrapper refusal never reaches the interpreter and so emits no JSON envelope. This is what
pins the bash-vs-Python asymmetry:
source-control-babysit-mergefilters--allow-unpinned-headin bash, while
source-control-babysit-resolve-threadis a pure passthrough whose--allow-unpinned-threadrefusal demonstrably comes from Python.--autonomous'sisOutdatedrequirement and the bot-only line are conditions over fetched API data that no argument shape
expresses. Includes the claim consumers get wrong: "resolves only outdated bot threads" is true
only under
--autonomousor--only-outdated.is what proves
manage_babysit_lease.py acquirewrites with no--apply.refresh_pr_branch.pycalls GitHub'sserver-side
update-branchand never pushes; a consumer reasoning about push guards is reasoningabout the wrong mechanism.
bin/-path wrapper command spelled in a plugin document ischecked against the backing CLI's own parser. This covers
reference/safety.md§Pinned-Command Degradation, the live instance of this issue inside the plugin's own docs.
Catalogue gates fail when a new entry point, wrapper, or command-spelling document arrives without a
row, and when an entry point's mutation classification has nothing asserting it.
Deferred with its trigger recorded at
guard_contract.py's docstring: an argparse flag catalogue(names, types, defaults) would additionally catch a renamed flag or changed default that no behavior
row exercises, but needs a
build_parser()extraction across all nine entry points, whose parsersare built inside
main().The existing hand-written cases in
test_guards.pywere ported to rows with their assertionsintact and strengthened — the scope rows now assert
inScopeis present andfalserather thanpayload.get("inScope")being falsy, which passed vacuously when the key was absent. The bash-levelwrapper checks in
engine.test.share left in place untouched.Verification
bash plugins/source-control/skills/babysit-prs/scripts/engine.test.sh:Fault injection — relaxing
classify'sisOutdatedcondition toonly_outdated and not thread["isOutdated"]produced:Two real defects were caught by the gates while building them: the doc-coverage gate found that
reference/orchestration.mdcarries its own copy of the wrapper command lines (now a covered row),and markdownlint caught a dropped
## [0.26.0]heading in the CHANGELOG.Also run clean:
markdownlint-cli2over the four changed markdown files,scripts/check-changelog-parity.sh --check-bump origin/main,scripts/check-silent-skips.sh.scripts/check-orphaned-fixtures.shexceeded a local timeout on this machine and is left to CI;this PR adds no eval fixtures.
Related
/source-control:setup's lane-script reachability probe, which names the plugin'sscripts from prose and could enumerate
ENTRY_POINTSinstead.isOutdateda weak signal.source-controlto0.27.0on top of0.26.0. PRs docs: document shell test-helper duplication and exit-code divergence as deliberate #853,fix(source-control): emit a readiness verdict on the gate's silent failure paths #1264, and chore(source-control): document plugin as sole worktree-convention SSOT (#401) #1021 also bump the same manifest and append to the same CHANGELOG, landing in the
order docs: document shell test-helper duplication and exit-code divergence as deliberate #853 → fix(source-control): emit a readiness verdict on the gate's silent failure paths #1264 → chore(source-control): document plugin as sole worktree-convention SSOT (#401) #1021. Whichever lands last before this one, the version number and
CHANGELOG position here need rebasing onto it.