feat(source-control): bind babysit guard semantics to an executable contract - #1285
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".
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".
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".
…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".
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac6af16476
ℹ️ 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".
…act-suite # Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md
…solated 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) <noreply@anthropic.com>
…into feat/1265-guard-contract-suite # Conflicts: # plugins/source-control/skills/babysit-prs/reference/guard-contract.md # plugins/source-control/skills/babysit-prs/scripts/tests/test_guards.py
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87b209127a
ℹ️ 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".
…act-suite # Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
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.
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
…act-suite # Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
#1285's guard contract landed on main, so this branch's additions now have to satisfy it. Resolutions: - Version: this branch's 0.27.0 slot was taken by main's 0.28.0. Re-slotted to 0.29.0 and the changelog section renamed to match. - safety.md: both sides added prose after the harness-ceiling paragraph. Kept both -- main's guard-contract pointer, then this branch's classifier-denial and Lane-Script Reachability sections. - test_guards.py: the branch's setup-canary class read a module-level `SCRIPTS` that main's rewrite moved into guard_contract. Now `contract.plugin_path`. Three drift gates then fired correctly and are answered rather than muted: - `test_skill_contract` pins the READINESS_UNPROVEN reason vocabulary verbatim; `checklist-unreadable` is now in safety.md's copy too. - The completeness gate found wrapper command lines in `skills/setup/SKILL.md` and `CHANGELOG.md` with no DOC_COMMAND_SOURCES row. Both are covered rather than exempted -- an operator reaches for the canary from either. - Covering them exposed a real gap: accepted flags are read from the parser's usage block, where argparse renders the `--help` pair as `-h`, so the canary's own flag read as unaccepted. `--help` is added back on the evidence of the check's own call -- that invocation IS `--help` and exits 0. 384 python tests and 122 gate tests pass. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#1420) No linked issue — a gap found and closed while driving #1285 to merge, small enough that a tracking issue would outlive its usefulness before anyone read it. Follow-up to #1285 (`#1265`). Closes the reverse direction of a binding that #1285 established in one direction only. ## The gap `WRAPPER_DENIED_FLAGS` records the flags a `bin/` wrapper refuses before Python runs, so a documented wrapper command naming one is a command that always exits 2. `test_wrapper_denied_flags_are_proven_by_a_refusal_row` proves every **listed** flag is one a `bash-wrapper` refusal row invokes the wrapper to demonstrate. Nothing proved the converse. A new bash-wrapper refusal row could demonstrate a second refused flag while the table stayed silent about it — and a document could then spell that flag unchallenged, because the documented-command check consults the table, not the rows. The table would be a *subset* of the wrapper's behavior while reading as a *statement* of it. That is the unbacked-claim shape this contract exists to catch, pointed the other way. ## Changes - `test_every_wrapper_refusal_row_reaches_the_denial_table` — every flag a bash-wrapper refusal row names must be covered by `WRAPPER_DENIED_FLAGS`. Read from `error_contains` rather than `argv`: a row's argv also carries the flags that reached the wrapper legitimately, while the error names the one the refusal is about. - `test_the_denied_flag_is_one_its_own_cli_accepts` — pins the premise the separate wrapper check rests on. The merge parser *does* register `--allow-unpinned-head`, which is precisely why a CLI-only check cannot see the wrapper's refusal. If this stops holding, the two checks have collapsed into one and the narrowing is no longer load-bearing. ## Verification `python -m unittest tests.test_guards` — 20 tests, OK. No production code changes; test and changelog only, `source-control` 0.28.0 → 0.28.1. ## Related - #1285 — established `WRAPPER_DENIED_FLAGS` and the forward binding this PR completes - #1265 — the guard-contract issue #1285 closed - #1264 — carries a separate `--help` fix to `_accepted_flags` in the same file; both touch `test_guards.py` and whichever lands second will need a trivial merge - #1422 — the topic-docs convention gap filed in the same pass; unrelated to this change, listed so the sweep is traceable --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ilure paths (#1264) ## Summary `babysit-readiness-gate.sh`'s header promised a machine-readable verdict on every check run, but exit 3 (invalid argument) and exit 4 (jq missing / fetch failed) wrote to stderr only. A caller grepping stdout for a verdict saw *nothing* on those paths — identical to what it sees when the gate was never invoked at all. This PR closes that output-hygiene gap and pins it with tests. **Re-scoped**: it no longer claims to make the unproven readiness verdict mechanically enforced, because it does not. That work is now tracked separately in #1387. ## Fix **The gate emits a third verdict.** Every exit now routes through one `unproven` helper, so `READINESS_UNPROVEN reason=<bad-args|identity-unresolved|prereq-missing|comments-unreadable|fetch-failed> pr=<n>` joins `READINESS_OK` and `READINESS_BLOCKED` — exactly one `READINESS_*` line on every check run. **Exit codes are unchanged**, so existing callers keyed on them are unaffected; the token is purely additive. **A malformed comment payload no longer reads as readiness.** The counters consumed `--comments-json` (or the live fetch) as a JSON array with jq's stderr suppressed and its exit status unchecked, so a snapshot that was truncated, hand-edited, or simply not an array yielded zero findings and `READINESS_OK findings=0` — a ready verdict derived from data the gate never read. The resolved payload is now shape-checked once (`type == "array"`, so a valid scalar or object is rejected too), which covers the snapshot path and the live-fetch path together; the two body extractions no longer swallow their own failures; and every such path routes through `READINESS_UNPROVEN reason=comments-unreadable` at exit 4. **An identity-lookup failure is no longer reported as a bad argument.** With neither `--self` nor `--extra-self` supplied and the supported `gh api user` default failing — expired auth, an unreachable API, an offline snapshot replay — the arguments were valid but stdout said `reason=bad-args`. Since §5.5 quotes that verdict verbatim, it directed operators and automation at flags that were already correct rather than at the identity prerequisite to repair. That path now emits `reason=identity-unresolved`, holding exit 3 so callers keyed on the code are unaffected. **`usage()` is derived rather than hardcoded.** It previously printed a fixed `sed -n '2,65p'` line range, which silently drifted as the header grew. It is now derived by `awk` from the comment block, and the help output no longer leaks a bare `READINESS_*` token that a prefix-only parser would mistake for a machine verdict. **The report quotes the verdict verbatim.** `loop.md` §5.5's per-PR status gains a **Gate verdict** line carrying the gate's stdout as printed, or `not emitted — harness denied: <exact command>`. The NEVER-do list forbids backfilling it from `mergeStateStatus` or the check rollup — evidence that misses the very cross-checks the gate runs (dependency author, unprotected base, self-login exemption, head match). This half is prose, and the PR now says so rather than implying otherwise. `reference/safety.md` states the limit directly: *"a gate the harness never let run cannot report its own non-invocation, which is why the quoted-verdict requirement lives on the report rather than inside the script."* Making that mechanical is #1387. **The evidence is re-based.** #787's reproduction was `python .../babysit_merge.py …` — a raw wildcarded-interpreter form auto mode drops *by design*, reached for (per #787's own body) because the bare wrapper was not on PATH. Commit `3fc72d351c`, which made the `bin/`-path form the mandated spelling, landed ~16 hours *after* #787 was filed. So #787 does not show the sanctioned form being denied. `safety.md` now says so plainly and rests the prerequisite on the evidence that does hold: `melodic-software/dotfiles#315`, where `autoMode.classifyAllShell: true` suspended every narrow Bash allow rule including twelve purpose-built lane-script grants. Stated as a generalization, not a repro. **The setup probe gains a canary.** The prior revision told the operator to check the scopes "the classifier actually reads", for which no executable path exists (server-managed settings are delivered from Anthropic's servers, not a locally readable file; endpoint-managed settings live at OS-specific MDM/registry locations). That clause is dropped in favour of `claude auto-mode config`, which prints the effective merge across all three `autoMode` sources — user, `--settings`/SDK, and managed; the doc had named only two. Alongside it is a non-mutating canary: `bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" --help`. **Known limit of that canary, stated rather than papered over:** it exercises `--help`, not the production argument shapes. Classifier decisions are per call, so a host that permits `--help` and denies `owner/repo#N --allowed-owners …` yields a PASS from a lane that cannot prove readiness. Raised by the Codex reviewer on `skills/setup/SKILL.md:146` and `:147`, and carried into #1387 as part of what a real fix has to include. **#455 is scoped around, not resolved.** This section restates the "never retry a harness permission denial" rule that #455 disputes with a classifier denial whose retry succeeded. A note marks that question open and links #455, so the restatement does not read as fresh confirmation. ## Test plan - `plugins/source-control/scripts/babysit-readiness-gate.test.sh` — **92/92 PASS**. New cases: every failure path carries `READINESS_UNPROVEN` with the right reason and PR number; a malformed snapshot and every non-array JSON shape yield `reason=comments-unreadable` at exit 4 and never `READINESS_OK`; an identity-lookup failure yields `reason=identity-unresolved` at exit 3 and is never reported as `bad-args`; both verdict paths still emit exactly one `READINESS_*` line and never claim UNPROVEN; no bare `exit 3`/`exit 4` survives in the script; `--help` emits no verdict line. - `plugins/source-control/scripts/babysit-wrapper-help.test.sh` — **9/9 PASS** (new file). `--help` on both `bin/` wrappers exits 0 against a `gh` stub that fails loudly on any invocation, and `--allow-unpinned-head` is still refused alongside it. - `python -m unittest discover -s plugins/source-control/skills/babysit-prs/scripts/tests` — **354 tests, OK**. Gates run locally: `markdownlint-cli2` clean across all touched markdown; `shellcheck -x` clean; `shfmt -d` clean on both new/changed shell files; `check-changelog-parity.sh --check-bump` PASS; `check-silent-skips.sh` PASS. ## Independent verification Audited by a fresh context that did not write the branch, with the rationale withheld. Its report is in the comments. It returned FAIL against the original "make the enforcement mechanical" framing, and its two critical findings are the reason for this re-scope: - the enforcement is prose, and the branch concedes it in `safety.md`; - the `READINESS_UNPROVEN` token has zero executable consumers — `git grep -n "READINESS_"` excluding the gate and its own tests returns only markdown, `evals.json`, two Python *comments*, and test files. It also raised a fail-open path in the jq counting (`jq … 2>/dev/null` with no exit-status check, so malformed JSON yielded `READINESS_OK` and exit 0), which the Codex reviewer independently raised on the same lines. It was previously deferred to #1387 as pre-existing; that deferral no longer stands — a false-ready verdict directly falsifies this PR's own every-check-run contract, so it is **fixed here** (see the Fix section) rather than shipped alongside the contract it breaks. ## Related **No linked issue** — this PR closes none of the issues it references, for the reasons stated below. Refs #787 — the originating report. **Deliberately not closed.** Its causal premise is falsified (see the evidence re-basing above) and its substantive ask — that a blocked gate cannot yield a reported-ready verdict — is not delivered by this PR. Refs #1387 — the mechanical enforcement and the production-shape canary. The `jq` fail-open it also carried is resolved here and drops off that list. Refs #455 — flagged as an open dispute over the retry semantics this section restates; explicitly not settled here. Consumer-side evidence: `melodic-software/dotfiles#315`. **Version reconciliation:** `main` has moved past this branch's base; `origin/main` is merged in and the `0.27.0` minor bump stands above it (`main` is at `0.26.4`). Landing order among the `plugins/source-control` queue is this PR, then #1285. If that order changes, re-merge and take the next free number. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…point (#1428) Closes #1371. Replaces #1382, which is closed in favour of this. #1382 was opened before #1354 landed and is now a strict superset of it: re-scoped here to the residual only, on a fresh base, with the property bound to the directory rather than to a list of files. ## The gap A permission grant states its condition as the literal presence or absence of a flag in the command text — above all *"no `--merge` means check-only"*. Argparse's default prefix abbreviation lets `--mer` resolve to `--merge` while the command text contains no such flag, so the written command and the resolved behavior diverge. That is exactly what such a condition has to be able to rule out. #1354 closed this on `babysit_merge.py` and `babysit_resolve_thread.py`. Seven entry points still inherited the default: `babysit_findings.py` · `manage_babysit_lease.py` · `manage_feedback_ledger.py` · `pr_queue_snapshot.py` · `prune_babysit_worktrees.py` · `refresh_pr_branch.py` · `request_review.py` All nine now set `allow_abbrev=False`. ## Why a gate, not seven more edits Hardening entry points one at a time is what let the gap survive #1354 for seven files. The guard contract gains a check over the whole catalogue: every catalogued Python entry point is invoked with `--hel` and must not exit 0. `--help` is registered on every parser and short-circuits parsing, so an abbreviation that *resolves* exits 0 before required-argument validation ever runs, while one that does not is a usage error. That makes the exit code a sufficient discriminator without a per-CLI argument shape — which is what makes this a gate over the catalogue rather than a hand-maintained list of cases. The message is deliberately not asserted: several of these parsers have a required mutually exclusive group that errors before any unrecognized argument is reported. A companion test asserts that discrimination against argparse itself rather than assuming it. ## Verification - `python -m unittest discover -s tests` — 387 tests, OK. - Detector verified rather than assumed: reverting `request_review.py`'s `allow_abbrev=False` fails the gate naming that file (`resolved the abbreviation --hel to --help and exited 0`), and it passes again on restore. ## Compatibility Abbreviated invocations that previously worked are now usage errors. That is the intent, and the version takes a minor bump for it — `source-control` 0.29.0 → 0.31.0 (0.30.0 is claimed by #1264, open). ## Related - #1354 — closed the same defect on the first two entry points - #1382 — the superset PR this replaces - #1285 — the guard contract this gate is added to --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.