fix(source-control): emit a readiness verdict on the gate's silent failure paths#1264
fix(source-control): emit a readiness verdict on the gate's silent failure paths#1264kyle-sexton wants to merge 11 commits into
Conversation
…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 finished @kyle-sexton's task in 2m 40s —— View job PR 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 CI is still running (markdownlint and suite pending). The PR author reports 0 lint errors. Findings
|
|
Claude finished @kyle-sexton's task in 2m 7s —— View job Security Review
Scope: All 5 files changed in this PR — SUGGESTION — Confidence: PLAUSIBLENon-pinned
The referenced file exists inside this same plugin package at 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
Recommended fix: Reference the in-package copy instead of the raw-GitHub URL — No other security findings. The rest of the PR is net-positive from a security standpoint:
|
There was a problem hiding this comment.
💡 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".
…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 finished @kyle-sexton's task in 2m 13s —— View job PR Review (re-review after synchronize)
What the new commit doesCommit The commit message also flags that Prior findings — statusFrom the previous PR review (
- 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'sFrom the previous security review (
From the Codex review: The Codex P2 finding was on 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 goodThe structural fix is correct. Removing the paragraph from
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. SummaryTwo prior findings remain open (scope parenthetical in |
|
Claude finished @kyle-sexton's task in 1m 29s —— View job Security Review — PR #1264
Scope: All 4 files changed in this PR — SUGGESTION — Confidence: PLAUSIBLENon-SHA-pinned
The file at Why it matters: An agent reading
The 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:
|
There was a problem hiding this comment.
💡 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".
|
Update after review — Why. The That is also where the repo's own convention puts it: Worth a maintainer's attention independently of this PR: Two self-review findings recorded rather than quietly fixed, since both are the class of thing tooling here does not catch:
|
…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 finished @kyle-sexton's task in 4m 11s —— View job PR Review (re-review — commit
|
|
Claude finished @kyle-sexton's task in 3m 8s —— View job Security Review — PR #1264
Scope: 7 changed files — SUGGESTION — Confidence: PLAUSIBLENon-SHA-pinned
The file at Why it matters:
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 Note: this finding was raised in the two earlier automated security reviews on this PR (commits No other security findings. The rest of the PR is net-positive:
|
There was a problem hiding this comment.
💡 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".
…#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 finished @kyle-sexton's task in 2m 35s —— View job PR Review (re-review — commits
|
|
Claude finished @kyle-sexton's task in 2m 58s —— View job Security Review — PR #1264
Scope: 10 changed files — SUGGESTION — Confidence: PLAUSIBLENon-SHA-pinned
The file at Why it matters:
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 This finding was raised and confirmed in every prior security review of this PR (commits No other security findings. The rest of the PR is net-positive:
|
There was a problem hiding this comment.
💡 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".
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 finished @kyle-sexton's task in 3m 14s —— View job PR Review (re-review — commit
|
|
Claude finished @kyle-sexton's task in 2m 30s —— View job Security Review — PR #1264 (re-review — commit
|
There was a problem hiding this comment.
💡 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".
Classification — PR-level security-review findingsOne finding, raised once per Security Review pass. Classified per occurrence so
The security reasoning is sound. A The stated remedy rests on a false premise. The review asserts the target
So the in-package route is not a rewrite; it would require copying a Deferring the SHA-pin remedy because it is convention-scoped, not a one-off. Recorded for the author as a convention decision (SHA-pin all ten and adopt a |
…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>
Independent verification — VERDICT: FAILAudited 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
The whole guard against "harness denied the call → report merge-ready anyway" is CRITICAL 2 — the mechanical addition has zero executable consumersThe new refusal point is Spot-checked from the main thread: IMPORTANT 3 — the gate #787 actually names is untouched#787's denied command was IMPORTANT 4 — fail-open: malformed input yields a positive verdict
This is pre-existing on SUGGESTION 5 — a markdown claim the code does not do
Test quality — splitGenuinely behavioral: String-presence-in-markdown only: What actually ran
Version queue
Credit where dueThe branch is honest about its own limits — Not verifiedThe actual classifier denial cannot be reproduced from a subagent context, so "does the sanctioned |
…-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.
There was a problem hiding this comment.
💡 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".
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 finished @kyle-sexton's task in 3m 42s —— View job PR Review (re-review — post-merge sync
|
|
Claude finished @kyle-sexton's task in 5m 26s —— View job Security Review — PR #1264
Scope: 10 changed files — IMPORTANT — Confidence: CONFIRMEDMalformed
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)"
Why this matters here: The PR's core addition is the Pre-existing on Raised by the independent verification (IMPORTANT 4) and the latest Codex review (commit SUGGESTION — Confidence: PLAUSIBLENon-SHA-pinned
The file lives at repo-root 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 No other security findings. The rest of the PR is net-positive:
|
There was a problem hiding this comment.
💡 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".
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>
There was a problem hiding this comment.
💡 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 || { |
There was a problem hiding this comment.
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 👍 / 👎.
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
unprovenhelper, soREADINESS_UNPROVEN reason=<bad-args|identity-unresolved|prereq-missing|comments-unreadable|fetch-failed> pr=<n>joinsREADINESS_OKandREADINESS_BLOCKED— exactly oneREADINESS_*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 andREADINESS_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 throughREADINESS_UNPROVEN reason=comments-unreadableat exit 4.An identity-lookup failure is no longer reported as a bad argument. With neither
--selfnor--extra-selfsupplied and the supportedgh api userdefault failing — expired auth, an unreachable API, an offline snapshot replay — the arguments were valid but stdout saidreason=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 emitsreason=identity-unresolved, holding exit 3 so callers keyed on the code are unaffected.usage()is derived rather than hardcoded. It previously printed a fixedsed -n '2,65p'line range, which silently drifted as the header grew. It is now derived byawkfrom the comment block, and the help output no longer leaks a bareREADINESS_*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, ornot emitted — harness denied: <exact command>. The NEVER-do list forbids backfilling it frommergeStateStatusor 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.mdstates 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. Commit3fc72d351c, which made thebin/-path form the mandated spelling, landed ~16 hours after #787 was filed. So #787 does not show the sanctioned form being denied.safety.mdnow says so plainly and rests the prerequisite on the evidence that does hold:melodic-software/dotfiles#315, whereautoMode.classifyAllShell: truesuspended 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 threeautoModesources — 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--helpand deniesowner/repo#N --allowed-owners …yields a PASS from a lane that cannot prove readiness. Raised by the Codex reviewer onskills/setup/SKILL.md:146and:147, and carried into #1387 as part of what a real fix has to include.#455 is scoped around, not resolved. This section restates the "never retry a harness permission denial" rule that #455 disputes with a classifier denial whose retry succeeded. A note marks that question open and links #455, so the restatement does not read as fresh confirmation.
Test plan
plugins/source-control/scripts/babysit-readiness-gate.test.sh— 92/92 PASS. New cases: every failure path carriesREADINESS_UNPROVENwith the right reason and PR number; a malformed snapshot and every non-array JSON shape yieldreason=comments-unreadableat exit 4 and neverREADINESS_OK; an identity-lookup failure yieldsreason=identity-unresolvedat exit 3 and is never reported asbad-args; both verdict paths still emit exactly oneREADINESS_*line and never claim UNPROVEN; no bareexit 3/exit 4survives in the script;--helpemits no verdict line.plugins/source-control/scripts/babysit-wrapper-help.test.sh— 9/9 PASS (new file).--helpon bothbin/wrappers exits 0 against aghstub that fails loudly on any invocation, and--allow-unpinned-headis still refused alongside it.python -m unittest discover -s plugins/source-control/skills/babysit-prs/scripts/tests— 354 tests, OK.Gates run locally:
markdownlint-cli2clean across all touched markdown;shellcheck -xclean;shfmt -dclean on both new/changed shell files;check-changelog-parity.sh --check-bumpPASS;check-silent-skips.shPASS.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:
safety.md;READINESS_UNPROVENtoken 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/nullwith no exit-status check, so malformed JSON yieldedREADINESS_OKand 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
jqfail-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:
mainhas moved past this branch's base;origin/mainis merged in and the0.27.0minor bump stands above it (mainis at0.26.4). Landing order among theplugins/source-controlqueue is this PR, then #1285. If that order changes, re-merge and take the next free number.