Skip to content

fix(source-control): emit a readiness verdict on the gate's silent failure paths#1264

Open
kyle-sexton wants to merge 11 commits into
mainfrom
feat/babysit-automode-prereq
Open

fix(source-control): emit a readiness verdict on the gate's silent failure paths#1264
kyle-sexton wants to merge 11 commits into
mainfrom
feat/babysit-automode-prereq

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

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.sh92/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.sh9/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/tests354 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.

kyle-sexton and others added 2 commits July 24, 2026 17:59
…lity as a prerequisite (#787)

The babysit lane's own bundled scripts can be denied by a host permission
classifier — including the read-only merge-readiness check, which mutates
nothing. When that happens the lane cannot gate-prove readiness, and the
observed failure was worse than the denial: the orchestrator substituted live
`gh` state (`mergeStateStatus`, check rollup) for the gate's verdict, evidence
that misses the gate's dependency-author, unprotected-base, self-login, and
head-match cross-checks.

Declares reachability as a lane prerequisite alongside Python, with the
difference that matters stated explicitly: it has no degrade tier, because the
Python-free path also proves readiness with a bundled script. Adds the
never-substitute rule so a denied check reports "readiness unproven — harness
blocked the gate" instead of silently downgrading, and surfaces the prerequisite
through `/source-control:setup`'s babysit check so it lands before a cycle
rather than mid-cycle.

Deliberately names no allow-rule string: the lane's invocation forms are
interpreter-led, bare wrapper names do not resolve (#843), and
`autoMode.classifyAllShell` suspends shell allow rules outright — so no verified
`permissions.allow` shape holds today. The prerequisite is stated outcome-shaped
and points at the host's auto-mode configuration reference and the marketplace's
permission-rule-hygiene convention rather than restating either.

This is arm (b) of #787's Expected; arm (a) is classifier-side and out-of-repo.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNNCY6Go6K3yNGYYCBFWJw
…width

Review of the preceding commit. The added paragraph was a single 735-character
line in a file whose prose averages 81, and the same commit unwrapped an
unrelated existing line to 110 characters as incidental reflow.

`MD013` is disabled in this repo's markdownlint config, so neither failed
lint -- which is exactly why it needed catching by eye rather than by tooling.
Both now match the surrounding text, and the unrelated reflow is reverted so
the diff carries only the change it claims.
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand the request
  • Read the PR diff and changed files
  • Review plugin.json version bump
  • Review CHANGELOG.md entry
  • Review babysit-prs/SKILL.md prerequisite declaration
  • Review babysit-prs/reference/safety.md new content
  • Review setup/SKILL.md reachability probe
  • Synthesize findings and post review

Overall: This is a sound, well-scoped documentation change. The design — declare the prerequisite, forbid the substitution, surface it before a cycle — is the correct response to the observed failure in #787. One minor concern about baked-in host semantics in setup/SKILL.md is worth addressing; everything else looks good.

CI is still running (markdownlint and suite pending). The PR author reports 0 lint errors.


Findings

setup/SKILL.md — Item 4 restates host-owned scope specifics

setup/SKILL.md line 141

whether an `autoMode` block exists in the scopes the classifier actually reads
(user-global and managed settings; not a project's `.claude/` settings)

The parenthetical bakes in which scopes the auto-mode classifier reads: user-global and managed, not project. That is exactly the kind of Anthropic-owned semantic that safety.md correctly handles by saying:

read auto-mode-config for the current semantics of autoMode.classifyAllShellnever infer them from this file

If Anthropic later adds project-scope autoMode support, this parenthetical would lead the setup check to probe the wrong scopes — the same class of staleness the PR explicitly avoids in safety.md. Suggest removing the parenthetical and relying on the auto-mode-config link (already present two lines below) for the current scope list:

-        classifier actually reads (user-global and managed settings; not a project's `.claude/`
-        settings), and whether `claude auto-mode config` shows effective rules covering this plugin's
+        classifier actually reads, and whether `claude auto-mode config` shows effective rules covering this plugin's

Fix this →


What looks good

safety.md pointer-not-copy discipline is correct. The new "Lane-Script Reachability" subsection links to the official auto-mode-config doc and carries an explicit "never infer them from this file" guard. The PR delivers exactly what it promises: the plugin states only the outcome it requires, never the classifier's internal semantics. Lines 336–344

The check/mutation distinction is cleanly drawn. The new clause covers the denied-check case; Pinned-Command Degradation continues to cover the denied-mutation case. Both end up under ## Harness Permission Layer where they belong, and each is correctly scoped — no overlap, no gap. Lines 346–354

The raw.githubusercontent.com link pattern is correct for cross-referencing convention docs — consistent with reference/config-resolution.md, skills/pull-request/reference/create.md, and others throughout the plugin. The target file (docs/conventions/permission-rule-hygiene/README.md) exists. Line 334

No degrade tier is the right call. The PR's reasoning holds: the Python-free path also relies on a bundled script, so a denied lane script leaves no weaker-but-valid tier to fall back to. The asymmetry vs. Python (which does have a degrade tier) is clearly explained both in SKILL.md and in the CHANGELOG. SKILL.md line 329–336

Version bump, CHANGELOG, and frontmatter update are all correct. Minor version bump (0.26.0 → 0.27.0) is appropriate for additive non-breaking changes. The CHANGELOG omits dates, consistent with all prior entries. The setup/SKILL.md description frontmatter correctly adds "lane-script permission reachability" to the check surface list.


@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather context
  • Read PR diff
  • Audit changed files for security issues
  • Post findings

Scope: All 5 files changed in this PR — plugin.json, CHANGELOG.md, babysit-prs/SKILL.md, babysit-prs/reference/safety.md, setup/SKILL.md. No executable code, shell scripts, CI workflows, or credential-handling paths were modified.


SUGGESTION — Confidence: PLAUSIBLE

Non-pinned raw.githubusercontent.com URL in an AI-readable skill file creates a post-install override surface

skills/babysit-prs/reference/safety.md line 334

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

The referenced file exists inside this same plugin package at docs/conventions/permission-rule-hygiene/README.md. In plugin-form it ships in the installed cache and is reachable at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. The raw-GitHub pointer is pinned to main, not to a commit SHA.

Why this matters in an AI-readable skill file: An agent following the skill instructions that encounters this URL may WebFetch it at runtime. When it does, it gets the current main branch content — not the content that shipped with the installed version (0.27.0). This creates two compounding problems:

  1. Versioning bypass. A consumer who installed v0.27.0 gets permission-guidance from whatever main currently says, not what 0.27.0 was reviewed against. Legitimate updates to that file after install silently supersede the installed version's guidance without a plugin bump or consumer consent.

  2. Post-install override surface. Any future commit to main that modifies the permission-rule-hygiene README immediately affects all running babysit sessions that fetch the URL. An attacker who gains write access to main (compromised maintainer, confused-deputy PR merge) could replace the convention file's guidance with text that loosens or misdirects permission decisions for agents already running the installed plugin.

Recommended fix: Reference the in-package copy instead of the raw-GitHub URL — ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md — which the installed plugin cache already contains at the version the consumer installed. This keeps the versioning guarantee intact and removes the runtime fetch dependency. If the intent is specifically to surface the latest guidance regardless of installed version, pin to a full commit SHA rather than main.

Fix this →


No other security findings. The rest of the PR is net-positive from a security standpoint:

  • The never-downgrade rule (safety.md lines 346–354) is a correct, positive security control — it closes the authorization bypass where a blocked check gate was silently substituted with weaker gh state evidence.
  • The setup probe (setup/SKILL.md lines 134–145) correctly never writes settings and never FAILs on absence; no unauthorized configuration modification surface.
  • No shell scripts, Python, CI workflows, GitHub Actions permissions, or credential-handling paths were touched.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 60a8e288b5

ℹ️ 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".

Comment thread plugins/source-control/skills/babysit-prs/reference/safety.md Outdated
…at its cap

The `skill-quality-gate` refused the branch: `SKILL.md is 508 lines (hard cap
500)`. `babysit-prs/SKILL.md` on `main` is **499** lines and the cap is
exclusive, so the file has room for nothing at all -- not the paragraph this
branch added, and not the one-line pointer it was reduced to.

Rather than buy room by deleting neighbouring prose this change has no
business touching, the declaration stays where the repo's own convention puts
it. `reference/safety.md` is already cited as the "single home" from five
places in that SKILL.md, and the gate's soft target (200 lines) exists to
push exactly this kind of detail into spokes. So the contract lives in
safety.md, `/source-control:setup`'s babysit `check` surfaces it before a
cycle, and `babysit-prs/SKILL.md` is byte-identical to `main`.

Worth flagging to a maintainer independently of this change: that file is one
line under a hard cap, so the next addition of any size hits the same wall.
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

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


PR Review (re-review after synchronize)

  • Read the new commit and diff vs origin/main
  • Check if previous findings were addressed
  • Review new changes for correctness
  • Post final assessment

What the new commit does

Commit 9656521 removes the paragraph that was added to babysit-prs/SKILL.md (which hit the 500-line hard cap — the file is at 499 lines and the cap is exclusive). babysit-prs/SKILL.md is now byte-identical to main. The declaration lives solely in safety.md (as the "single home" it already was) and setup/SKILL.md item 4 surfaces it for operators before a cycle. This is the right structural call: reference/safety.md is already cited from five places in babysit-prs/SKILL.md, and the gate's soft-target exists to push this kind of detail into spokes.

The commit message also flags that babysit-prs/SKILL.md at 499 lines means the next addition of any size hits the same wall — worth a separate follow-up.


Prior findings — status

From the previous PR review (60a8e28):

setup/SKILL.md lines 141–142 — parenthetical restating host-owned scope semantics — still present, not yet addressed:

classifier actually reads (user-global and managed settings; not a project's `.claude/`
settings)

setup/SKILL.md L141–142 — as the previous review noted, this bakes in Anthropic-owned semantics that safety.md explicitly guards against. If the host later adds project-scope autoMode support, this parenthetical silently misdirects the probe. Suggest removing it and letting the auto-mode-config link two lines below carry the current scope list:

-        classifier actually reads (user-global and managed settings; not a project's `.claude/`
-        settings), and whether `claude auto-mode config` shows effective rules covering this plugin's
+        classifier actually reads, and whether `claude auto-mode config` shows effective rules covering this plugin's

Fix this →

From the previous security review (60a8e28):

safety.md line 334 — non-pinned raw.githubusercontent.com/main URL — still present, not yet addressed:

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

safety.md L334 — the file is inside the plugin package at docs/conventions/permission-rule-hygiene/README.md and reachable at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. A main-pinned raw URL lets any future commit to main silently change the permission guidance seen by all installed 0.27.0 instances. The fix is to reference the in-package path or pin to a commit SHA.

From the Codex review:

The Codex P2 finding was on safety.md line 328 — "guarded wrappers under bin/" being listed in the reachability prerequisite (no-degrade tier) may conflict with the existing Pinned-Command Degradation contract, which handles a denied mutation by handing the pinned command to a human rather than declaring readiness unproven. The concern: an operator who intentionally denies merge wrappers for safety would trigger the no-degrade path when Pinned-Command Degradation is the correct and expected handling.

The "never downgraded" clause is scoped to "when the harness blocks the read-only merge-readiness check" — so the distinction is drawn, but it lives in the same prerequisite block as the mutation wrappers. Worth a clarifying sentence (e.g., "this prerequisite covers scripts required to prove readiness; denied mutation wrappers fall under Pinned-Command Degradation below") to make the scope boundary explicit and defuse the ambiguity.


What looks good

The structural fix is correct. Removing the paragraph from babysit-prs/SKILL.md (rather than deleting unrelated prose to make room) is the right trade. The contract living in safety.md + a pointer in setup/SKILL.md item 4 is a clean split.

safety.md pointer-not-copy discipline holds. The "Lane-Script Reachability" section links auto-mode-config and carries the "never infer them from this file" guard consistently.

The check/mutation distinction is preserved. The never-downgrade clause correctly targets the denied-check case; Pinned-Command Degradation continues to handle the denied-mutation case.


Summary

Two prior findings remain open (scope parenthetical in setup/SKILL.md; non-pinned URL in safety.md). The Codex ambiguity about mutation wrappers in the reachability prerequisite is worth a one-sentence clarification. Everything else is sound.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 29s —— View job


Security Review — PR #1264

  • Gather context and understand the request
  • Read the PR diff (git diff origin/main...HEAD)
  • Read changed files in full
  • Audit for security issues
  • Post findings

Scope: All 4 files changed in this PR — plugin.json, CHANGELOG.md, babysit-prs/reference/safety.md, setup/SKILL.md. No executable code, shell scripts, CI workflows, or credential-handling paths were modified. All changes are documentation and skill-instruction text.


SUGGESTION — Confidence: PLAUSIBLE

Non-SHA-pinned raw.githubusercontent.com URL in an AI-readable skill file creates a post-install content-override surface

skills/babysit-prs/reference/safety.md line 334

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

The file at docs/conventions/permission-rule-hygiene/README.md ships inside this same plugin package, reachable at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. The URL is pinned to the main branch tip, not to a commit SHA.

Why it matters: An agent reading safety.md may WebFetch this URL to follow the allow-rule guidance it references. When it does, it receives current main content — not the content reviewed against v0.27.0 and installed by the consumer. Two issues follow:

  1. Versioning bypass. A consumer who installed v0.27.0 silently receives post-install updates to permission guidance without a plugin bump or consumer consent.
  2. Post-install override surface. Any future commit to main touching this file immediately affects agents running the installed plugin. A write to main by a compromised maintainer account or a confused-deputy PR merge could redirect the permission-rule guidance agents follow, loosening or misdirecting permission decisions on running instances.

The safety.md file itself handles the auto-mode semantics link correctly — pointing to the official host doc (code.claude.com) with an explicit "never infer from this file" guard. The permission-rule-hygiene URL needs the same treatment: either route it through ${CLAUDE_PLUGIN_ROOT} to the in-package copy, or pin it to a full commit SHA.

Fix this →

Note: this finding was also raised in the earlier automated security review on this PR. Confirming it independently.


No other security findings. The rest of the PR is net-positive from a security standpoint:

  • The never-downgrade rule (safety.md lines 346–354) closes the authorization bypass in source-control:babysit-prs: read-only babysit_merge.py invocation blocked by host auto-mode permission classifier #787 where a blocked gate was substituted with weaker live gh evidence. This is a positive security control.
  • The no-degrade-tier declaration means the lane halts rather than continuing with unproven readiness — safer than the previous behavior.
  • The setup probe (setup/SKILL.md lines 134–145) is explicitly constrained to never write settings and never FAIL on absence; no unauthorized configuration write surface.
  • No shell scripts, CI workflows, GitHub Actions permissions, or credential-handling paths were touched.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9656521512

ℹ️ 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".

Comment thread plugins/source-control/skills/setup/SKILL.md Outdated
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Update after review — babysit-prs/SKILL.md is now byte-identical to main; the declaration lives only in reference/safety.md and the setup probe. CI is green.

Why. The skill-quality-gate refused the branch with SKILL.md is 508 lines (hard cap 500). That file on main is 499 lines and the cap is exclusive, so it has room for nothing — not the original paragraph, not the one-line pointer I reduced it to (500 still fails). The alternative was deleting neighbouring prose this change has no business touching, which is a worse trade than living in the spoke.

That is also where the repo's own convention puts it: reference/safety.md is cited as the "single home" from five separate places in that same SKILL.md, and the gate's 200-line soft target exists to push detail into spokes.

Worth a maintainer's attention independently of this PR: plugins/source-control/skills/babysit-prs/SKILL.md sits one line under a hard cap, so the next addition of any size hits the same wall. That is a structural condition, not something this branch caused.

Two self-review findings recorded rather than quietly fixed, since both are the class of thing tooling here does not catch:

  • The first commit landed the new paragraph as a single 735-character line in a file whose prose averages 81, plus an unrelated reflow of an existing line. MD013 is disabled in this repo, so markdownlint passed both.
  • Fixing that wrapping is what pushed the file over the cap — the original single-line form was under it by accident, not by design.

…787)

Two independent reviews concluded the branch declared a prerequisite it
never enforced. It now enforces one, in the place this plugin already puts
enforcement: the readiness gate, whose own header says "prose 'MANDATORY' is
a word; this gate counts rows, not intentions".

`babysit-readiness-gate.sh` promised a machine-readable verdict on every
check run, but its exit-3 and exit-4 paths wrote stderr only. A caller
grepping stdout saw nothing there -- byte-identical to what it sees when the
gate was never invoked, which is exactly how a blocked gate gets reported as
readiness. Every exit now routes through one `unproven` helper, so a third
verdict, `READINESS_UNPROVEN reason=<bad-args|prereq-missing|fetch-failed>`,
joins OK and BLOCKED. Exit codes are untouched, so callers keyed on them are
unaffected.

A script cannot report its own non-invocation, so the other half sits on the
report: loop.md's per-PR status gains a Gate verdict line carrying the gate's
stdout verbatim, or `not emitted -- harness denied: <command>`. Since the
gate always prints one, a readiness claim with nothing to quote is unproven
on its face. Not a PermissionDenied hook: the audit hook next door is
sink-less telemetry that exits 0 always, so it can record a denial but cannot
constrain what the orchestrator reports afterward.

The evidence is also re-based. #787's denial was of a raw wildcarded-
interpreter form that auto mode drops by design, reached for because the
`bin/` wrapper was not yet the mandated spelling -- so it does not show the
sanctioned form being denied. dotfiles#315 does the work instead:
`classifyAllShell: true` suspends every narrow Bash allow rule, twelve
lane-script grants included, so even the compliant `bin/`-path form reaches
the classifier. Stated as a generalization, not a repro.

setup's probe becomes executable: a non-mutating `--help` through the real
`bin/` path, where a tool-call denial is a FAILED prerequisite. The scope-
enumeration clause is dropped -- managed settings are not locally readable
files, and `claude auto-mode config` already prints the effective merge
across all three `autoMode` sources (user, `--settings`/SDK, managed), the
third of which the doc had omitted.

Finally, #455 disputes the never-retry rule this section sits beneath and
restates. A note marks that question open rather than letting the
restatement read as fresh confirmation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNNCY6Go6K3yNGYYCBFWJw
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


PR Review (re-review — commit c5f0c89)

  • Gather context and understand the request
  • Read the full PR diff vs origin/main
  • Review new commit: babysit-readiness-gate.sh changes
  • Review new commit: babysit-readiness-gate.test.sh (new tests)
  • Review new commit: babysit-prs/reference/loop.md changes
  • Verify status of previous open findings
  • Synthesize and post final review

What the new commit does

Commit c5f0c89 makes the never-downgrade rule mechanical. babysit-readiness-gate.sh adds READINESS_UNPROVEN reason=<cause> pr=<n|unknown> as a third stdout verdict emitted on every non-verdict exit — so a caller that greps stdout for READINESS_* can no longer mistake a failed gate for one that was never attempted. The script, its tests, and loop.md are updated together as a coherent unit.


Previous findings — status

setup/SKILL.md parenthetical (raised in two prior reviews) — RESOLVED.

The new text correctly avoids baking in scope names. Where the old text said (user-global and managed settings; not a project's .claude/ settings), the replacement delegates to claude auto-mode config output:

"It already prints the merged result across every scope the classifier reads autoMode from — user settings, a --settings/SDK-supplied file, and managed settings — so read that output rather than hunting for the underlying files"

This also addresses the Codex P2 finding about --settings scope (commit 9656521's open finding): because the scope description is now a quote of what claude auto-mode config prints rather than a claim the plugin makes directly, any future change to which scopes the classifier reads autoMode from is reflected automatically in what the probe reports. The fix is structurally correct.

safety.md line 357 — non-SHA-pinned raw.githubusercontent.com/main URL — still open.

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

This has now been raised in every review of this PR (two automated reviews, one manual review on commit 60a8e28, one re-review on 9656521). The file it points to ships in the installed cache at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. A main-pinned raw URL lets any commit to main silently replace the permission guidance seen by all installed v0.27.0 instances. The fix is the same each time: use the in-package path or pin to a commit SHA.

Fix this →

Codex P2 — mutation wrappers in the no-degrade prerequisite — partially addressed.

The last paragraph of the Lane-Script Reachability section now explicitly says "Pinned-Command Degradation below covers the denied-mutation case; this clause covers the denied-check case." That is the right place for the distinction. What remains is that the opening paragraph of the section still lists "the guarded wrappers under bin/" as part of the no-degrade prerequisite, which is where the Codex finding's concern lands. The current text is defensible — the distinction is drawn before the reader reaches the Pinned-Command Degradation section — but a one-sentence scoping note in the opening paragraph (e.g., "this prerequisite is scoped to scripts that prove readiness; denied mutation wrappers fall under Pinned-Command Degradation") would close the reading ambiguity for good. Not blocking, but the finding is still formally open.


What looks good in the new commit

unproven() implementation is correct. ${PR_NUMBER:-unknown} correctly handles early-failure paths (arg-parsing failures before PR_NUMBER is set). Exit codes are unchanged — the function is purely additive.

Source-level bare-exit assertion is a strong contract test.

bare_exits=$(grep -cE '^[[:space:]]*exit [34]$' "$GATE" || true)
assert_eq "no bare exit 3/4 survives (every failure path emits a verdict)" 0 \
  "${bare_exits//[^0-9]/}"

This does what a per-path test alone cannot: it enforces that no future maintainer silently adds a bare exit 3 or exit 4. The unproven() function uses exit "$2" (variable, not literal), so it correctly doesn't trip the pattern. And because it greps the actual script file, it would catch a regression immediately.

The jq-prerequisite test is handled correctly. Rather than trying to hide jq from PATH (which would also hide the interpreter on the CI image), the test asserts unproven prereq-missing 4 appears in the script source. Same contract-level style as the bare-exit check. No test gap.

verdict_lines() helper and the exactly-one-verdict invariant are correct. The four tests (UNPROVEN paths) + two counter-tests (OK and BLOCKED never claim UNPROVEN and emit exactly one verdict line) give good coverage of the invariant without over-specifying the format.

assert_not_contains is properly defined in test-helpers.sh. No phantom helper.

usage() sed range update (65→77) is correct. Line 77 is # (READINESS_UNPROVEN reason=prereq-missing|fetch-failed) — the last line of the expanded header. The --help documents READINESS_UNPROVEN test would catch a stale range.

loop.md additions are consistent and self-reinforcing. §5.1.3 (note on exactly-one-line invariant), the NEVER-do list entry (never report a verdict the gate didn't emit), and §5.5 (quoted-verbatim requirement with the "not emitted — harness denied: <exact command>" fallback) form a closed loop with the script change. The template now surfaces READINESS_UNPROVEN and the harness-denied form together rather than leaving the "what do I write if it was blocked?" question to inference.

The #455 open-question caveat is honest and correctly scoped. Adding a note that the never-retry rule's applicability to classifier denials is disputed — and flagging #455 — is the right call rather than silently inheriting a rule whose correctness for this case is unresolved. The caveat is correctly positioned: "read its restatement of the denial contract as inherited from the bullet above, not as fresh confirmation of it."


Summary

The new commit delivers the mechanical half the previous commits declared: the gate now enforces the never-downgrade rule in code, not just in documentation. The setup/SKILL.md parenthetical is resolved. One prior finding remains open (non-SHA-pinned raw URL in safety.md); the Codex P2 mutation-wrapper ambiguity is partially addressed. Everything else is sound.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Security Review — PR #1264

  • Gather context and read PR diff
  • Read changed files in full
  • Audit for security issues
  • Post findings

Scope: 7 changed files — plugin.json, CHANGELOG.md, babysit-readiness-gate.sh, babysit-readiness-gate.test.sh, babysit-prs/reference/loop.md, babysit-prs/reference/safety.md, setup/SKILL.md. No GitHub Actions workflows, credential-handling paths, or network-facing code were modified.


SUGGESTION — Confidence: PLAUSIBLE

Non-SHA-pinned raw.githubusercontent.com URL in an AI-readable skill file

skills/babysit-prs/reference/safety.md line 357

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

The file at docs/conventions/permission-rule-hygiene/README.md ships inside this plugin package, reachable at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. The URL is pinned to the main branch tip.

Why it matters: safety.md is an AI-readable skill instruction file. An agent following the skill's permission-rule guidance may WebFetch this URL to read the allow-rule shape guidance it references. When it does, it receives current main content — not the content reviewed against v0.27.0. Two issues follow:

  1. Versioning bypass. A consumer who installed v0.27.0 silently receives post-install updates to permission-rule guidance without a plugin bump or consumer consent.
  2. Post-install override surface. Any future commit to main touching this file immediately affects agents running the installed plugin. A write to main — compromised maintainer, confused-deputy PR merge — could redirect the guidance agents follow for allow-rule shape decisions, loosening or misdirecting permission checks on running instances.

The surrounding text already handles the analogous risk correctly: the auto-mode semantics link points to the official host doc with an explicit "never infer them from this file" guard. The permission-rule-hygiene URL needs the same treatment — either route through ${CLAUDE_PLUGIN_ROOT} to the in-package copy, or pin to a full commit SHA.

Note: this finding was raised in the two earlier automated security reviews on this PR (commits 60a8e28 and 9656521). Confirming it independently against the current HEAD.

Fix this →


No other security findings. The rest of the PR is net-positive:

  • The READINESS_UNPROVEN shell changes are fail-closed. The unproven() function (babysit-readiness-gate.sh line 98–101) uses printf '%s' — no format-string vulnerability. The $1 argument is always a hardcoded literal at each callsite (bad-args, prereq-missing, fetch-failed), never user input. exit "$2" receives a literal integer. No command-injection or path-injection surface in the new code.
  • The never-downgrade rule (safety.md lines 379–387) closes the authorization bypass in source-control:babysit-prs: read-only babysit_merge.py invocation blocked by host auto-mode permission classifier #787 where a blocked check gate was substituted with weaker live gh evidence. Positive security control.
  • The setup probe (setup/SKILL.md lines 138–465) explicitly forbids retrying a denied call and forbids respelling it as a raw interpreter invocation to bypass the denial — correct constraint for an agent that must not self-escalate permissions. The probe also never writes settings and never FAILs on absence of config; no unauthorized config-write surface.
  • The bare-exit source check in tests (babysit-readiness-gate.test.sh lines 259–261) is a clever integrity assertion: it greps the gate script for surviving exit 3/exit 4 statements and asserts zero — a source-level guarantee that no failure path silently skips the verdict.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5f0c89750

ℹ️ 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".

Comment thread plugins/source-control/skills/setup/SKILL.md
Comment thread plugins/source-control/skills/setup/SKILL.md Outdated
kyle-sexton and others added 2 commits July 24, 2026 22:34
…#787)

The gate's UNPROVEN verdict is already covered by its own suite, but two
halves of the #787 contract sat in prose with nothing holding them: the
report-side rule that a readiness claim quotes the gate's verdict verbatim,
and the setup canary's choice of target.

The prose assertions guard against silent deletion restoring the exact
ambiguity the branch exists to close -- including the evidence framing, since
a future editor trimming "does not demonstrate that the sanctioned form gets
denied" would leave the section overclaiming #787 as a reproduction.

The canary's harmlessness gets a bash suite rather than a Python one. Its
whole framing is that a denial there is a FAILED prerequisite BECAUSE the
invocation is provably non-mutating -- so the wrapper is exercised against a
gh stub that fails loudly, proving --help reaches no network, needs no
allowlist, and still refuses --allow-unpinned-head. It lands in
plugins/source-control/scripts/ where the runner's plugins/**/*.test.sh glob
picks it up, and where bash tests already live.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNNCY6Go6K3yNGYYCBFWJw
Its three siblings in this directory are 100755; it landed 100644.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SNNCY6Go6K3yNGYYCBFWJw
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


PR Review (re-review — commits 6d98156 + 4e0eb24)

  • Gather context
  • Read the new commits and diff vs origin/main
  • Verify status of all open findings from prior reviews
  • Check babysit-wrapper-help.test.sh and new Python test additions
  • Synthesize and post final review

What the new commits do

6d98156 ("test: pin the unproven-readiness and canary contracts") adds three test sets that machine-enforce prose contracts that were previously held only by documentation:

  1. babysit-wrapper-help.test.sh (new bash suite) — pins the canary's harmlessness: a fail-loud gh stub proves --help never reaches the network, --allow-unpinned-head is refused even alongside --help, and the usage output confirms fast-path short-circuit. This directly answers the Codex P2 "probe the readiness command" finding from c5f0c89 — the canary is --help specifically because that path is provably non-mutating, and now the suite proves it.

  2. test_skill_contract.py additions — two new tests: test_unproven_readiness_contract_is_stated_on_both_sides asserts both halves of the source-control:babysit-prs: read-only babysit_merge.py invocation blocked by host auto-mode permission classifier #787 contract (gate-side verdict + report-side quoting rule) against silent deletion; test_lane_script_prerequisite_names_its_actual_evidence asserts safety.md correctly attributes its evidence to dotfiles#315 rather than overclaiming a source-control:babysit-prs: read-only babysit_merge.py invocation blocked by host auto-mode permission classifier #787 repro.

  3. test_guards.py additionSetupReachabilityCanaryContract pins the exact canary invocation in setup/SKILL.md, asserts FAILED verdict on denial, and asserts the dropped scope-enumeration clause (not a project's .claude/ settings) cannot creep back. This is well-targeted: it tests the exact string bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" --help against the live prose, so any rename or re-spelling fails the suite immediately.

4e0eb24 (chmod) — babysit-wrapper-help.test.sh landed 100644; this makes it 100755 to match babysit-readiness-gate.test.sh, fetch-all-pr-comments.test.sh, and worktree-create.test.sh. Simple and correct.


Prior findings — status

safety.md line 357 — non-SHA-pinned raw.githubusercontent.com/main URL — STILL OPEN.

This has been raised in every automated review of this PR (commits 60a8e28, 9656521, c5f0c89) and confirmed in the manual re-reviews. The current file:

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

safety.md L357

The file it references ships in the installed cache at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. Pinning to main lets any future commit silently replace the permission guidance seen by all installed v0.27.0 instances. Fix: use the in-package path or pin to a commit SHA.

Fix this →

Codex P2 — mutation wrappers in the no-degrade prerequisite — PARTIALLY ADDRESSED.

The text at safety.md L385–386 correctly draws the check/mutation boundary in the closing sentence. The opening paragraph still lists "the guarded wrappers under bin/" in the no-degrade prerequisite without narrowing to readiness-only scripts. The distinction is present but requires reading through the section to find; an operator who permits read-only gates but intentionally denies merge wrappers would reach the opening paragraph first. Not blocking, but the one-sentence scoping note remains outstanding.

Codex P2 (c5f0c89) — probe the readiness command rather than merge help — RESOLVED.

babysit-wrapper-help.test.sh makes the canary's harmlessness a machine-enforced invariant. The concern was that a --help probe might succeed while the real readiness gate gets denied. The new suite proves the canary is honest about its scope: it cannot be passed through the fail-closed guard, it cannot be made to reach the network, and --allow-unpinned-head cannot subvert it. The framing — a denial of this provably non-mutating call is a FAILED prerequisite — holds.

Codex P2 (c5f0c89) — forward launch-time --settings into the config probe — PARTIALLY OPEN.

setup/SKILL.md L153–156

The current text asserts that claude auto-mode config "already prints the merged result across every scope the classifier reads autoMode from — user settings, a --settings/SDK-supplied file, and managed settings." The scope-enumeration parenthetical from the prior commit was rightly removed; this text delegates to claude auto-mode config output rather than restating scopes. The residual concern is that a bare claude auto-mode config child invocation may not see a --settings file the parent process was launched with. The text handles this by saying "read that output rather than hunting for the underlying files" — which tells the operator to rely on the command's own merged view, not to verify the individual files themselves. That is reasonable guidance, and the text correctly avoids claiming the child invocation reproduces the parent's launch context. The Codex finding is weakened by this delegation but not fully closed; no action is forced.


What looks good in the new commits

--allow-unpinned-head refused alongside --help is the most important case. L57–60 The guard fires before the interpreter runs; this test ensures --help cannot be used as a bypass vector. Without it, an agent that learns to append --help to avoid the allowlist check would have no CI backstop.

The "inScope" payload check is the right proxy. L45–46 Rather than asserting on the word "allowed-owners" (which appears in --help output), it asserts on the JSON payload key that only the refusal emits. The comment explains this precisely.

test_lane_script_prerequisite_names_its_actual_evidence closes the overclaiming gap. L248–265 A future editor trimming the "does not demonstrate that the sanctioned form gets denied" caveat — perhaps to simplify the section — would immediately break this test. That is exactly the regression the PR exists to prevent.

The assertNotIn for the dropped scope clause in SetupReachabilityCanaryContract. L184 Pinning the absence of the dropped text (not a project's .claude/ settings) ensures the host-owned scope claim cannot creep back, which was the substance of the prior review finding. This is exactly the right use of assertNotIn.


Summary

The two new commits close the Codex "probe readiness command" finding by making the canary's harmlessness a tested invariant, and pin both halves of the #787 contract against silent prose deletion. One finding remains open through the full history of this PR: the non-SHA-pinned raw.githubusercontent.com/main URL in safety.md line 357. Everything else is sound.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Security Review — PR #1264

  • Gather context and understand the request
  • Read the PR diff
  • Read changed files in full
  • Audit for security issues
  • Post findings

Scope: 10 changed files — plugin.json, CHANGELOG.md, babysit-readiness-gate.sh, babysit-readiness-gate.test.sh, babysit-wrapper-help.test.sh, babysit-prs/reference/loop.md, babysit-prs/reference/safety.md, babysit-prs/scripts/tests/test_guards.py, babysit-prs/scripts/tests/test_skill_contract.py, setup/SKILL.md. No GitHub Actions workflows were modified.


SUGGESTION — Confidence: PLAUSIBLE

Non-SHA-pinned raw.githubusercontent.com URL in an AI-readable skill file

skills/babysit-prs/reference/safety.md line 357

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

The file at docs/conventions/permission-rule-hygiene/README.md ships inside this plugin package, reachable at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. The URL is pinned to the main branch tip, not a commit SHA.

Why it matters: safety.md is an AI-readable skill instruction file. An agent following the skill's permission-rule guidance may WebFetch this URL. When it does, it receives current main content — not the content reviewed against v0.27.0. Two issues follow:

  1. Versioning bypass. A consumer who installed v0.27.0 silently receives post-install updates to permission-rule guidance without a plugin bump or consumer consent.
  2. Post-install override surface. Any future commit to main touching this file immediately affects agents running the installed plugin. Write access to main — compromised maintainer account, confused-deputy PR merge — could redirect the permission guidance agents follow for allow-rule shape decisions.

The surrounding text already handles the analogous risk correctly: the auto-mode semantics link points to the official host doc with an explicit "never infer them from this file" guard. The permission-rule-hygiene URL needs the same treatment — either route through ${CLAUDE_PLUGIN_ROOT} to the in-package copy, or pin to a full commit SHA.

This finding was raised and confirmed in every prior security review of this PR (commits 60a8e28, 9656521, c5f0c89). Confirming independently against HEAD 4e0eb24.

Fix this →


No other security findings. The rest of the PR is net-positive:

  • unproven() helper in babysit-readiness-gate.sh (lines 98–101) uses printf '%s' — no format-string vulnerability. All three callsites pass hardcoded literals (bad-args, prereq-missing, fetch-failed); exit "$2" receives a literal integer. No injection surface.
  • PR_NUMBER, COMMENTS_JSON, CHECKLIST, SELF_CSV, EXTRA_SELF_CSV are all passed as properly-quoted arguments through the script. COMMENTS content flows through pipes to jq/grep with hardcoded patterns — no command injection, no format-string risk.
  • The babysit-wrapper-help.test.sh stub gh is written to a mktemp -d temp directory (owner-writable only) with a trap cleanup. PATH prepending is scoped to the individual test invocation. Standard test isolation; no security issue.
  • The never-downgrade rule (safety.md lines 379–387) closes the authorization bypass in source-control:babysit-prs: read-only babysit_merge.py invocation blocked by host auto-mode permission classifier #787 where a blocked check gate was substituted with weaker live gh evidence. Positive security control.
  • The setup probe (setup/SKILL.md lines 139–161) uses a hardcoded ${CLAUDE_PLUGIN_ROOT} path (set by the plugin runtime, not user input), explicitly forbids retrying or re-spelling the denied call, never writes settings, and never FAILs on absence. No unauthorized config-write or self-escalation surface.
  • The bare-exit source check in tests (babysit-readiness-gate.test.sh lines ~258–262) is a source-level integrity assertion: it greps the gate script for surviving exit 3/exit 4 statements and asserts zero — a static guarantee that no failure path can silently skip the verdict.

@kyle-sexton kyle-sexton changed the title feat(source-control): declare babysit lane-script permission reachability as a prerequisite feat(source-control): make the unproven readiness verdict mechanical Jul 25, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e0eb24ed8

ℹ️ 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".

Comment thread plugins/source-control/scripts/babysit-readiness-gate.sh Outdated
Exempt --help from the every-run verdict contract. usage() exits 0 without a
verdict, so the header's claim of one READINESS_* line on every run was false
for a documented invocation. Worse, the header prose describing the token
un-indented to column 0 once usage() stripped the comment markers, so a caller
grepping ^READINESS_ matched help text as a malformed verdict on a run that
checked nothing. The contract is now scoped to check runs, and no header line
starts with the token. usage() also derives the header block from the comment
run rather than a hardcoded line range that truncated as the header grew.

Probe the readiness gate, not only the merge wrapper. The reachability canary
invoked bin/source-control-babysit-merge alone, but the lane proves readiness
through scripts/babysit-readiness-gate.sh -- a different path prefix that no
allow rule or classifier decision covering the wrapper reaches. Both forms are
canaries now.

Forward the launch-time settings scope into the config probe. --settings is a
global flag consumed at launch, not an input auto-mode config accepts, so a
bare probe spawned from a session launched with one silently omits that scope.
Verified empirically: a marker set in a --settings file is absent from bare
output and present when the flag is forwarded.

Limit the no-degrade prerequisite to the paths that prove readiness. It read as
blanketing denied mutations too, which Pinned-Command Degradation already
degrades to an operator handoff; only a denied check has no handoff, because
nothing was ever proven ready.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kb58mL6JnfEos5zrhgn4GD
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


PR Review (re-review — commit e7dd60e)

  • Gather context
  • Read the new commit diff vs origin/main
  • Verify status of all open findings from prior reviews
  • Review new changes for correctness and security
  • Post final assessment

What the new commit does

Commit e7dd60e ("fix: address the four open Codex findings") closes all four P2/P4 Codex findings that had remained after prior iterations. Each fix is mechanical and testable, and the commit message gives the "verified empirically" evidence for the --settings scope case.


Prior findings — status

Codex P4 — --help and the every-run verdict contract — RESOLVED

The usage() function now derives the header block from the actual comment run via awk instead of a hardcoded line range that silently truncated as the header grew. More importantly, the line that previously un-indented to column 0 — READINESS_UNPROVEN is the fail-closed third verdict... — has been reworded to The READINESS_UNPROVEN token is the fail-closed third verdict..., so it no longer matches a caller's ^READINESS_ grep. The header also carries an explicit invariant note at babysit-readiness-gate.sh L74–76:

Every header line describing a verdict stays indented or mid-sentence: an unindented READINESS_* here would leave --help output matching a caller's ^READINESS_ verdict grep.

New tests at babysit-readiness-gate.test.sh L488–501 pin the invariant: --help emits 0 verdict lines, still reaches the end of the header, and never spills past it into code. The boundary check (set -uo pipefail must not appear) is the right proxy for "awk stopped at the first non-comment line."

Codex P2 — probe the readiness gate, not only the merge wrapper — RESOLVED

setup/SKILL.md L141–158 now mandates both canary forms:

bash "${CLAUDE_PLUGIN_ROOT}/bin/source-control-babysit-merge" --help
bash "${CLAUDE_PLUGIN_ROOT}/scripts/babysit-readiness-gate.sh" --help

The reasoning is stated clearly: "an allow rule or classifier decision covering the bin/ wrapper says nothing about the scripts/ helper, so a canary that ran only the first would certify a path the lane's readiness verdict never travels." test_guards.py now asserts both strings are present and that the denial message is "denial on either is a FAILED prerequisite."

Codex P2 — forward launch-time --settings into the config probe — RESOLVED

setup/SKILL.md L163–169 now says explicitly:

Forward the launch-time scope when there is one. --settings is a global flag consumed at launch, not an input the subcommand accepts, so a bare claude auto-mode config spawned from a session that was itself launched with --settings <file> reports without that scope and under-states the effective rules. Probe with claude --settings <file> auto-mode config in that case…

The Agent SDK edge case (no file to re-supply) is handled: report the probe as "scope-incomplete." The prior text listing "user settings, a --settings/SDK-supplied file, and managed settings" is corrected to "user settings and managed settings" (the accurate description of what the bare command sees), with the forwarding instruction for the --settings case moved to its own sentence. This is more accurate than before, and the commit message notes empirical verification.

Codex P2 — limit the no-degrade prerequisite to paths that prove readiness — RESOLVED

safety.md L339–346 now has a dedicated paragraph:

The no-degrade half is narrower than the prerequisite, and that distinction is the point. It binds the paths that prove readiness… A denied mutation is not in that set: there the gate has already proven the PR ready, so Pinned-Command Degradation below degrades it to a ready-to-execute operator handoff. So the prerequisite covers reachability of every bundled script; the no-degrade rule covers the check paths only.

This is exactly the scoping the Codex finding asked for. The check/mutation split now appears in the opening paragraph rather than only in the closing sentence, which is where an operator reading top-to-bottom would encounter it first.


Non-SHA-pinned raw.githubusercontent.com/main URL — STILL OPEN

safety.md L365

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

This has been flagged in every automated and manual review across every commit in this PR. The file it references ships inside the installed plugin package at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. A main-pinned raw URL lets any future commit silently replace the permission guidance seen by all installed v0.27.0 instances without a bump or consumer consent. The fix is the same every time: replace the URL with ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md (or pin to a commit SHA if live-from-main is intentional). The surrounding text already handles the analogous risk correctly — the auto-mode semantics link points to the official host doc with an explicit "never infer them from this file" guard.

Fix this →


What looks good in the new commit

The awk-based usage() is strictly better than the hardcoded range. sed -n '2,77p' broke silently whenever the header grew or shrank; the new form stops at the first non-comment line by construction and never needs a line number update. The new tests verify both that it reaches the end (completeness) and that it does not spill past it (boundary). Together they make a stale range impossible to reintroduce accidentally.

The --settings scope guidance is accurate and actionable. The wording correctly distinguishes what a bare probe reports (user + managed) from what a session with --settings sees (user + managed + file scope), and gives the operator a concrete form to use. The Agent SDK edge case ("no file to re-supply → report as scope-incomplete") is the right call — better to flag under-counting than to claim completeness.

The no-degrade paragraph placement is an improvement. It now appears before the reader reaches the Pinned-Command Degradation section, so the check/mutation distinction is established before the handoff path is described. An operator reading linearly encounters the narrowing before the broader degradation contract, removing the ambiguity the Codex finding identified.


Summary

All four Codex findings are now closed. One finding remains open through the full PR history: the non-SHA-pinned raw.githubusercontent.com/main URL in safety.md L365. The fix is a one-line replacement.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


Security Review — PR #1264 (re-review — commit e7dd60e)

  • Gather context
  • Read PR diff (latest commit e7dd60e)
  • Read changed files in full
  • Audit for security issues
  • Post findings

Scope: 6 files changed in commit e7dd60eCHANGELOG.md, babysit-readiness-gate.sh, babysit-readiness-gate.test.sh, babysit-prs/reference/safety.md, babysit-prs/scripts/tests/test_guards.py, setup/SKILL.md. No GitHub Actions workflows, credential-handling paths, or network-facing code were modified.


SUGGESTION — Confidence: PLAUSIBLE

Non-SHA-pinned raw.githubusercontent.com URL in an AI-readable skill file

skills/babysit-prs/reference/safety.md line 365

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

The file at docs/conventions/permission-rule-hygiene/README.md ships inside this plugin package, reachable at ${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md. The URL is pinned to the main branch tip, not a commit SHA.

Why it matters: safety.md is an AI-readable skill instruction file. An agent following the skill's permission-rule guidance may WebFetch this URL. When it does, it receives current main content — not the content reviewed against v0.27.0. Two issues follow:

  1. Versioning bypass. A consumer who installed v0.27.0 silently receives post-install updates to permission-rule guidance without a plugin bump or consumer consent.
  2. Post-install override surface. Any future commit to main touching this file immediately affects agents running the installed plugin. Write access to main — compromised maintainer account, confused-deputy PR merge — could redirect the guidance agents follow for allow-rule shape decisions.

The surrounding text already handles the analogous risk correctly: the auto-mode semantics link points to the official host doc with an explicit "never infer them from this file" guard. The permission-rule-hygiene URL needs the same treatment — either route through ${CLAUDE_PLUGIN_ROOT} to the in-package copy, or pin to a full commit SHA.

This finding was raised and confirmed in every prior security review of this PR (commits 60a8e28, 9656521, c5f0c89, 4e0eb24). Confirming independently against HEAD e7dd60e.

Fix this →


No other security findings. The rest of the PR is net-positive:

  • The four Codex P2 issues addressed in this commit are all positive security changes. The --help exemption from the verdict contract (babysit-readiness-gate.sh lines 56–59) closes a case where help text containing an unindented READINESS_* token was parseable as a machine verdict on a run that checked nothing — removing a false-positive surface that could have allowed a blocked gate to appear active. The awk-based usage() derivation (line 100) reads only from ${BASH_SOURCE[0]}, which is the script itself and not user-controlled; no injection surface.
  • The dual-canary probe in setup/SKILL.md (lines 144–146) adds scripts/babysit-readiness-gate.sh --help alongside bin/source-control-babysit-merge --help. This prevents a false "reachable" certification when an allow rule covers only one path prefix; from a security standpoint it closes an incomplete probe that could have let a blocked readiness gate go undetected in the setup check.
  • The unproven() helper (babysit-readiness-gate.sh lines 109–112) uses printf '%s' for all variable substitutions — no format-string vulnerability. All three callsites pass hardcoded literals (bad-args, prereq-missing, fetch-failed); exit "$2" receives a literal integer. ${PR_NUMBER:-unknown} flows through printf '%s' with no shell-expansion risk.
  • The no-degrade narrowing in safety.md (lines 339–346) is a positive security control: it draws an explicit boundary between the denied-check path (no degrade, unproven readiness) and the denied-mutation path (Pinned-Command Degradation, already-proven readiness). This eliminates the reading ambiguity that could have led an agent to treat a denied readiness gate as equivalent to a denied merge wrapper.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e7dd60ebbf

ℹ️ 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".

Comment thread plugins/source-control/skills/setup/SKILL.md
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Classification — PR-level security-review findings

One finding, raised once per Security Review pass. Classified per occurrence so
the count is auditable; the substance is identical in all five.

# Finding Classification Evidence Reacted
1 SUGGESTION (60a8e28) — non-SHA-pinned raw.githubusercontent.com/.../main/... URL in safety.md:365 VALID (defer) — risk real, stated remedy INCORRECT, pin choice is repo-wide convention see below 👍
2 SUGGESTION (9656521) — same finding, re-raised VALID (defer) — same disposition same evidence 👍
3 SUGGESTION (c5f0c89) — same finding, re-raised VALID (defer) — same disposition same evidence 👍
4 SUGGESTION (4e0eb24) — same finding, re-raised VALID (defer) — same disposition same evidence 👍
5 SUGGESTION (e7dd60e) — same finding, re-raised against current HEAD VALID (defer) — same disposition same evidence 👍

The security reasoning is sound. A main-pinned URL in an AI-readable skill
file is a post-install override surface: an agent that WebFetches it receives
current main, not the content shipped with the installed version. Agreed.

The stated remedy rests on a false premise. The review asserts the target
"ships inside this plugin package, reachable at
${CLAUDE_PLUGIN_ROOT}/docs/conventions/permission-rule-hygiene/README.md". It
does not. Verified two ways:

  • In the branch: plugins/source-control/docs/conventions/permission-rule-hygiene/
    does not exist. The file lives at the repo root (docs/conventions/...),
    which is outside ${CLAUDE_PLUGIN_ROOT} (plugins/source-control/).
  • In the installed package: ls of the plugin cache root shows only bin,
    CHANGELOG.md, README.md, reference, scripts, skills — no docs/.

So the in-package route is not a rewrite; it would require copying a
repo-root convention doc into the plugin, which this repo forbids ("never
hand-copy upstream/external content"; prefer a pointer over a duplicate). That
remedy would trade a versioning gap for a drifting duplicate of a shared
convention.

Deferring the SHA-pin remedy because it is convention-scoped, not a one-off.
The same main-pinned raw-URL pattern appears 10 times across 4 files in this
plugin (reference/config-resolution.md, skills/pull-request/reference/create.md,
skills/setup/reference/apply-convention.md, and this file). Pinning only the
one line this PR happened to add makes the plugin internally inconsistent while
leaving nine identical exposures open — the finding is a property of the
plugins cross-repo pointer convention, not of this diff.

Recorded for the author as a convention decision (SHA-pin all ten and adopt a
refresh path, or accept main as the deliberate always-current pointer). Not
taken unilaterally in this pass.

kyle-sexton added a commit that referenced this pull request Jul 25, 2026
…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>
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Independent verification — VERDICT: FAIL

Audited by a fresh context that did not write this branch, given the artifact and #787's requirement only, with the builder's rationale withheld. Findings below were spot-checked from the main thread before posting.

CRITICAL 1 — the enforcement is not mechanical, and the branch says so in writing

skills/babysit-prs/reference/safety.md:383-385:

"That is both the mechanical half of this rule and its limit: 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."

The whole guard against "harness denied the call → report merge-ready anyway" is reference/loop.md:484-488 (a NEVER-do bullet) and loop.md:536-541 (§5.5 "Gate verdict, quoted verbatim"). Both are instructions to an agent. The decision this lane was built on was that #787's fail-closed rule is enforced by the readiness-gate predicate rather than by prose, and this is prose.

CRITICAL 2 — the mechanical addition has zero executable consumers

The new refusal point is scripts/babysit-readiness-gate.sh:109-112, an unproven() helper that prints READINESS_UNPROVEN reason=… pr=…. Exit codes are unchanged (3 and 4) — the CHANGELOG states this outright: "Exit codes are unchanged, so existing callers keyed on them are unaffected."

Spot-checked from the main thread: grep -rn "READINESS_" across plugins/source-control, excluding the gate and its own tests, returns only CHANGELOG, reference/review-discipline.md, reference/loop.md, reference/safety.md, evals.json, two Python source comments (babysit_classify.py, babysit_findings.py), and test files. No script, module, or wrapper parses a READINESS_* verdict. The sole consumer is an LLM following loop.md. That is a new string an agent is told to quote, not a predicate that refuses.

IMPORTANT 3 — the gate #787 actually names is untouched

#787's denied command was python .../babysit_merge.py <pr> --allowed-owners … --self-logins @me — the merge-readiness gate. This branch modifies scripts/babysit-readiness-gate.sh, the finding-decomposition gate. Verified from the main thread: git diff --name-only $(git merge-base HEAD origin/main)..HEAD returns 10 files and babysit_merge.py is not among them. Its stdout is still a JSON object with exit codes 0/10/2/3 and no state distinguishing "readiness could not be proven" from "never invoked".

IMPORTANT 4 — fail-open: malformed input yields a positive verdict

babysit-readiness-gate.sh:247 and :250 run jq … 2>/dev/null with no exit-status check, so a jq failure yields empty corpora → findings=0READINESS_OK, exit 0. Observed with BABYSIT_READINESS_BASH_ONLY=1 --comments-json <file> --self me:

fixture output exit
this is not json at all READINESS_OK findings=0 classified=0 checklist=n/a 0
[{"foo":"bar"}] READINESS_OK findings=0 classified=0 checklist=n/a 0
{} READINESS_OK findings=0 classified=0 checklist=n/a 0

This is pre-existing on origin/main (the origin/main copy behaves identically against the same fixture), but this branch newly escalates the surrounding claim to a fail-closed one — babysit-readiness-gate.sh:56-58 "EXACTLY ONE verdict line on EVERY check run… an absent verdict line can only mean the gate never ran", :64 "the fail-closed third verdict". A data-integrity failure producing a positive verdict falsifies that framing. There is a reason=fetch-failed; there is nothing for "what came back is not the expected shape".

SUGGESTION 5 — a markdown claim the code does not do

loop.md:537-538: "The gate prints exactly one such line on every run, so the only way to have none is that the gate never ran." False — --help is a run, prints no verdict, exits 0, and the branch's own test case 78 asserts exactly that. The script header is careful (:55 "every check run"; :57-58 "--help is NOT a check run"); loop.md dropped the qualifier. It matters because the new setup canary invokes --help and treats exit 0 as PASS.

Test quality — split

Genuinely behavioral: babysit-readiness-gate.test.sh cases 64–80 (stdout content, verdict-line counts, the --help-emits-nothing rule) and the new babysit-wrapper-help.test.sh (9 cases, fail-loud gh stub proving no network, --allow-unpinned-head still exits 2).

String-presence-in-markdown only: test_skill_contract.py::test_unproven_readiness_contract_is_stated_on_both_sides, ::test_lane_script_prerequisite_names_its_actual_evidence, and test_guards.py::SetupReachabilityCanaryContract — whose own docstring concedes it: "what is pinned here is that the skill still names that exact invocation." The tests covering the #787 contract are the weaker class.

What actually ran

  • bash scripts/babysit-readiness-gate.test.sh → exit 0, 80 PASS: lines, no FAIL
  • bash scripts/babysit-wrapper-help.test.sh → 9/9 PASS
  • python -m unittest tests.test_guards tests.test_skill_contract -vRan 31 tests in 5.560s / OK

pytest is not installed here (CPython 3.14 via uv); both Python files are plain unittest.TestCase, so the substitution is faithful.

Version queue

plugins/source-control/.claude-plugin/plugin.json 0.26.00.27.0; CHANGELOG gains ## [0.27.0] with 5 ### Added and 1 ### Changed bullets. Note origin/main is now at 0.26.2 (#853 and #1307 landed since this branch's base), so the merge needs the intervening 0.26.1/0.26.2 sections interleaved. 0.27.0 remains the correct target.

Credit where due

The branch is honest about its own limits — safety.md:355-357 states plainly that #787 "does not demonstrate that the sanctioned form gets denied… this section is a generalization from other evidence", and pins that concession in a test. usage() derived by awk from the comment block instead of a hardcoded sed -n '2,65p' range is a real improvement. Shell correctness is otherwise sound: set -uo pipefail, ${PR_NUMBER:-unknown} pinned by test 66, the fetch-failed call in a { } group rather than a subshell so printf reaches real stdout, pinned by test 72.

Not verified

The actual classifier denial cannot be reproduced from a subagent context, so "does the sanctioned bin/ form get denied under classifyAllShell" remains unproven here — as the branch itself already states.

…-prereq

# Conflicts:
#	plugins/source-control/.claude-plugin/plugin.json
#	plugins/source-control/CHANGELOG.md
…over-claiming (#787)

A permitted `--help` proves the mandated spelling is reachable; it does not
prove the production argument shapes are, because the host classifier decides
per call. The canary section read as if a PASS certified the lane could
gate-prove readiness, so a check run could report PASS while the next cycle
stalls.

The probes stay `--help`-only deliberately — the merge wrapper's read-only
production shape is a live GitHub call, and a read-only `check` must not start
touching the fleet it was asked to inspect. So the fix is to state the residual
gap and name what already covers it: a mid-cycle denial is fail-honest through
`READINESS_UNPROVEN` and loop.md's verbatim Gate-verdict quote, both added by
this PR.

`SetupReachabilityCanaryContract` pins the limitation alongside the two
invocation strings, and the 0.27.0 CHANGELOG entry carried the same over-claim
and is corrected with it.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 97f257d835

ℹ️ 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".

Comment thread plugins/source-control/scripts/babysit-readiness-gate.sh
@kyle-sexton kyle-sexton changed the title feat(source-control): make the unproven readiness verdict mechanical fix(source-control): emit a readiness verdict on the gate's silent failure paths Jul 25, 2026
Version queue: main reached 0.26.4 while this branch was open. The 0.27.0
minor bump stands above it and both CHANGELOG sections are kept in order.

Three content conflicts, all where #601's two-gates split touched the same
text as this branch's verdict work:

- babysit-readiness-gate.sh usage(): main widened the hardcoded line range
  to `sed -n '2,67p'`; this branch replaced the range with an awk derivation
  from the comment block. Kept the derivation -- it removes the drift the
  range bump was compensating for.
- loop.md NEVER-do list: both sides added a distinct rule. Kept both. One
  forbids reporting a readiness verdict the gate did not emit; the other
  forbids reading MERGE-READY off a classification verdict.
- loop.md 5.5 status block: main split one status box into three fields;
  this branch added a verbatim-quoting requirement. The three fields stand,
  with the quoting requirement folded into the classification-gate field.
  Both sides' contract tests assert exact strings, so the wording preserves
  the literals `- [ ] Finding-classification gate: READINESS_OK /
  READINESS_BLOCKED`, `Never report a readiness verdict the gate did not
  emit`, and `readiness unproven`.

Also drops a `READINESS_OK` token from the start of a header line that #601
introduced. The header is the --help output, and test 78 asserts --help
emits no verdict line -- a caller's `^READINESS_` grep would otherwise read
documentation as a malformed verdict.

Verified: babysit-readiness-gate.test.sh 80/80, babysit-wrapper-help.test.sh
9/9, `python -m unittest discover -s tests` 354 OK, shellcheck -x clean,
markdownlint clean on both changed documents.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

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


PR Review (re-review — post-merge sync 9d87bbf)

  • Gather context and understand the request
  • Read the full PR diff vs origin/main
  • Review changed files
  • Verify status of all open findings from prior reviews
  • Synthesize and post final assessment

What the merge commit does

Commit 9d87bbf merges origin/main (which reached 0.26.4 while the branch was open) and resolves three content conflicts, all from #601's two-gates split touching the same text as this branch's verdict work:

  1. babysit-readiness-gate.sh usage() — main widened the hardcoded sed -n '2,67p' range; this branch replaced it with the awk derivation. The derivation was correctly kept — it removes the drift the range bump was compensating for, and test 78 pins the boundary.

  2. loop.md NEVER-do list — both sides added distinct rules. Both were kept: "never report a readiness verdict the gate did not emit" (this branch) plus "never report a PR MERGE-READY off READINESS_OK" (main, source-control:babysit-prs: docs don't clearly distinguish babysit-readiness-gate.sh (classification count) from babysit_merge.py (actual merge readiness) — caused a false MERGE-READY report #601). They are complementary and non-overlapping.

  3. loop.md §5.5 status block — main split one status box into three separate fields; this branch added the verbatim-quoting requirement. The three fields stand with the quoting requirement folded into the classification-gate field. Both sides' contract tests assert the same literal strings, so the wording is preserved correctly.

Bonus fix in the merge commit: #601 had introduced a READINESS_OK token at the start of a header line in the script. The merge commit correctly drops it — test 78 (--help emits no verdict line) would have caught that regression on merge.

Test count: 354/354 pass post-merge (up from 348 before the merge; the 6 additional tests are from other commits on main).


Prior findings — status

Non-SHA-pinned raw.githubusercontent.com/main URL — formally classified, still open

safety.md L423

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

The author classified this as VALID (defer) after every review pass, with the explicit explanation that the stated remedy (${CLAUDE_PLUGIN_ROOT}/docs/…) rests on a false premise — the file lives at the repo root, not inside the plugin cache (plugins/source-control/), so the in-package rewrite is not a rewrite but a copy of a shared convention file, which the repo explicitly forbids. The same main-pinned raw-URL pattern appears 10 times across 4 files in this plugin; fixing one occurrence in isolation would leave the plugin internally inconsistent. The deferral classification is well-reasoned and the risk is understood.

Nothing to add. This remains a valid convention-level concern, correctly deferred to a holistic fix.

Codex P2 (97f257d835) — Reject malformed comment snapshots — pre-existing, deferred

babysit-readiness-gate.sh L256–257

non_self_bodies="$(printf '%s' "$COMMENTS" |
  jq -r … 2>/dev/null)"

The 2>/dev/null silences jq errors, so malformed JSON (not-json, {}, [{"foo":"bar"}]) yields empty output → findings=0READINESS_OK exit 0. This is pre-existing on origin/main (verified by the independent reviewer against identical fixtures) and tracked in #1387. The new "fail-closed" language in the header (lines 66–72) is technically overstated against this path.

The Codex comment on 97f257d835 for this finding has not received an explicit author classification yet. The independent verification classified it as IMPORTANT 4 (pre-existing, deferred). Formally noting for the record: the framing tension between "fail-closed" and this fail-open path is real, is pre-existing, and is correctly tracked in #1387. If a classification entry for the Codex thread would help close the review loop, it would be worth adding.

All other Codex P2 findings — RESOLVED

  • --help exempt from every-run verdict contract — resolved in e7dd60e
  • Dual canary probes (bin/ + scripts/) — resolved in e7dd60e
  • --settings scope forwarding into config probe — resolved in e7dd60e
  • No-degrade prerequisite narrowed to check paths only — resolved in e7dd60e
  • Canary pass-side limit over-claim — resolved in 97f257d

What looks good in the merge commit

The conflict resolution is correct. All three conflicts preserve both sides' contracts without one overwriting the other, and the NEVER-do list now has the right two distinct rules: the readiness-verdict backfill rule (this PR) and the MERGE-READY-off-READINESS_OK rule (main). These are complementary constraints that cover different failure modes.

The READINESS_OK header line fix is the right call. Dropping a bare READINESS_OK from the start of a header line that #601 introduced is consistent with this PR's own invariant (lines 84–86: "every header line describing a verdict stays indented or mid-sentence"), and test 78 continues to enforce it.

Version ordering is correct. 0.27.0 stands above 0.26.4 (the highest version on main at merge time), and all four interleaved 0.26.x sections are present in order.


Summary

The merge commit is clean: three conflicts correctly resolved, a minor header-token regression from main fixed, and all tests passing (354). Two findings remain open: the non-SHA-pinned URL (VALID, deferred with sound reasoning) and the jq fail-open (pre-existing on main, deferred to #1387). The latest Codex P2 thread on the jq fail-open has no author classification entry yet — worth adding for completeness. Everything else is sound.

@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 26s —— View job


Security Review — PR #1264

  • Gather context
  • Read the PR diff and changed files
  • Audit for security issues
  • Post findings

Scope: 10 changed files — plugin.json, CHANGELOG.md, babysit-readiness-gate.sh, babysit-readiness-gate.test.sh, babysit-wrapper-help.test.sh, babysit-prs/reference/loop.md, babysit-prs/reference/safety.md, babysit-prs/scripts/tests/test_guards.py, babysit-prs/scripts/tests/test_skill_contract.py, setup/SKILL.md. No GitHub Actions workflows were modified.


IMPORTANT — Confidence: CONFIRMED

Malformed --comments-json content bypasses all new failure-verdict paths, emitting false READINESS_OK

babysit-readiness-gate.sh lines 255–261

non_self_bodies="$(printf '%s' "$COMMENTS" |
  jq -r --argjson self "$SELF_JSON" '
    .[] | select((.author as $a | $self | index($a)) | not) | .body // ""' 2>/dev/null)"
self_bodies="$(printf '%s' "$COMMENTS" |
  jq -r --argjson self "$SELF_JSON" '
    .[] | select((.author as $a | $self | index($a))) | .body // ""' 2>/dev/null)"

jq exits with code 5 and no stdout when its input is not valid JSON; 2>/dev/null suppresses the error message. Both body variables are assigned empty strings, all grep counts resolve to 0, and the gate exits 0 emitting READINESS_OK findings=0 classified=0 checklist=n/a. Verified: printf 'not-json' | jq -r '.[] | .body // ""' 2>/dev/null produces no output and exits 5.

Why this matters here: The PR's core addition is the unproven() helper that closes the silent-exit gap — "EXACTLY ONE verdict line on EVERY check run, including every failure path." Malformed JSON in a --comments-json file is a failure path the PR's own frame classifies as needing a verdict (READINESS_UNPROVEN reason=fetch-failed covers the live-fetch case; nothing covers the content-is-bad case). An attacker or malfunctioning tool that writes an invalid-JSON comment snapshot to the --comments-json path receives a false-positive gate pass rather than READINESS_UNPROVEN.

Pre-existing on origin/main, as the independent verification's IMPORTANT 4 confirms; the origin/main copy behaves identically. But this PR escalates the surrounding claims — safety.md now says the gate "emits exactly one READINESS_* line on stdout on every run, failure paths included" — making the gap more prominent than it was when the script made no such claim. The fix (check the exit status of each jq call and route through unproven fetch-failed) is tracked in #1387.

Raised by the independent verification (IMPORTANT 4) and the latest Codex review (commit 97f257d835, babysit-readiness-gate.sh:188); the author's responses to those threads address the canary-probe finding, not this one.

Fix this →


SUGGESTION — Confidence: PLAUSIBLE

Non-SHA-pinned raw.githubusercontent.com/main URL in an AI-readable skill file

skills/babysit-prs/reference/safety.md line 423

<https://raw.githubusercontent.com/melodic-software/claude-code-plugins/main/docs/conventions/permission-rule-hygiene/README.md>

The file lives at repo-root docs/conventions/permission-rule-hygiene/README.md — confirmed absent from plugins/source-control/ (the installed plugin package root). An agent reading safety.md may WebFetch this URL; when it does, it receives current main content, not the content reviewed against v0.27.0. A future commit to main that modifies the permission-rule-hygiene guidance takes effect immediately on all running installed instances without a plugin bump. The in-package-path remedy proposed by prior automated reviews is incorrect (the file does not ship with the plugin); the real fix is pinning to a commit SHA.

The author has correctly classified this as VALID (defer): the same pattern appears ten times across four files in the plugin, and patching only this occurrence while leaving nine others open would leave the plugin internally inconsistent without closing the exposure. Confirming independently against HEAD 9d87bbf.


No other security findings. The rest of the PR is net-positive:

  • unproven() at lines 119–122 uses printf '%s' for all substitutions — no format-string risk. All three callsites pass hardcoded string literals (bad-args, prereq-missing, fetch-failed); exit "$2" receives a literal integer. ${PR_NUMBER:-unknown} flows through printf '%s' with no expansion risk.
  • usage() at lines 109–113 reads from ${BASH_SOURCE[0]} (the script itself, not user input); no injection surface.
  • babysit-wrapper-help.test.sh gh stub is written to a mktemp -d directory with a single-quoted heredoc (no variable expansion), cleaned up with trap 'rm -rf "$NO_NET_BIN"' EXIT. PATH prepending is limited to each individual test invocation. No injection surface.
  • The --allow-unpinned-head guard fires before the interpreter runs; the corresponding test correctly verifies the guard holds even when --help is also present.
  • The never-downgrade rule (safety.md lines 435–453) is a correct, positive security control closing the authorization bypass in source-control:babysit-prs: read-only babysit_merge.py invocation blocked by host auto-mode permission classifier #787.
  • The bare-exit source assertion in babysit-readiness-gate.test.sh (lines 460–462) is a strong static guarantee: it greps the gate script itself for surviving exit 3/exit 4 literals and asserts zero, making it impossible for a future maintainer to silently reintroduce a bare exit that bypasses the verdict.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9d87bbf191

ℹ️ 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".

Comment thread plugins/source-control/scripts/babysit-readiness-gate.sh Outdated
kyle-sexton added a commit that referenced this pull request Jul 25, 2026
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>
…en paths

A malformed or wrong-shaped comment payload reached the counters with jq's
stderr suppressed and its exit status unchecked, so zero findings were counted
and the gate printed READINESS_OK -- a ready verdict derived from data it never
read. The resolved payload is now shape-checked once (covering both the
--comments-json snapshot and the live fetch), the body extractions surface their
own failures, and every such path routes through the fail-closed verdict as
reason=comments-unreadable at exit 4.

An identity lookup that failed with valid arguments reported reason=bad-args,
which the loop report quotes verbatim -- pointing an operator at flags that were
already correct instead of at the gh auth to repair. That path now emits
reason=identity-unresolved, holding exit 3 for callers keyed on the code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a46bc23806

ℹ️ 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".

# ready verdict derived from data the gate never read, which is exactly the
# fail-open this script exists to close. Validated once here, on the RESOLVED
# payload, so the snapshot path and the live-fetch path are both covered.
printf '%s' "$COMMENTS" | jq -e 'type == "array"' >/dev/null || {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate every comment-array element

When a syntactically valid snapshot contains null or an object missing the documented author/body fields (for example, [null] or [{}]), this check accepts it because it validates only the top-level array; the later jq expressions coalesce the absent body to an empty string, and the gate exits 0 with READINESS_OK findings=0. Fresh evidence beyond the earlier malformed-snapshot comment is that these malformed array elements still reproduce the false-ready verdict after the new parse check, so validate the element schema before counting.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant