docs(source-control): wire babysit autopilot §3 to the tier flags, specify the approve mechanic and review-context precondition (#675)#706
Conversation
|
Claude finished @kyle-sexton's task in 2m 22s —— View job PR Review
Overall this is a well-executed prose/contract PR. The design choices are sound and the implementation is coherent. No correctness bugs or security issues found. A few observations below, mostly minor.
|
| Priority | Location | Finding |
|---|---|---|
| Low | SKILL.md:181 |
"are the single home" — grammatically awkward for a three-item list |
| Low | safety.md:202-210 |
"the review workflow" is unnamed — operator may not know which check context to require |
| Cosmetic | test_skill_contract.py:106 |
Implicit backtick-passthrough assertion; a comment would clarify intent |
All are non-blocking. The PR achieves its stated goal — closing the three coherence gaps that were prerequisites for ever flipping the tier on — cleanly and verifiably.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3936ea57c9
ℹ️ 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".
… surfaces to the tier flags too (#675) Codex P1 on #706: the §3 fix wired only the autopilot worker-final-push merge path, but the "Fan out" section also sends already-clean zero-blocker autopilot PRs straight to the DIRECT merge gate with the flagless base command (`--merge --expected-head <snapshotted-head-sha>`), so an ENABLED tier would merge those without the lane / approver / block-label / issue-link / human-veto criteria — the same fail-open the flip precondition must close, and it made this PR's own "never merges via the base path" claim false for clean PRs. Sweep every autopilot merge surface, not just the one Codex caught: - Zero-blocker direct-gate path (SKILL.md): an enabled autopilot merge tier now adds the tier flags (reference/safety.md), never the flagless base command. Unpinned prose in the same paragraph is tightened so the caveat lands net-zero against the 499/500 line cap (no cap raise — the operator rejected that). - Pinned-Command Degradation operator handoff (safety.md): when the tier is enabled the degraded merge handoff reproduces the tier-flagged command, so a harness-blocked but gate-proven merge is not handed to the operator as a flagless base merge. The base-command occurrences that remain (generic Merge readiness wrapper contract; worker-tier §7 push path, which never has a merge tier) are correct as base and left unchanged. New skill-contract assertion pins the direct-gate paragraph's tier-enabled behavior so the universal claim is tested, not just prose-asserted. No script change — the gate already fails closed when handed the flags on a PR lacking the distinct-bot approval. Refs #675 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 55s —— View job PR Review
This PR now carries two commits. The second commit ( Codex P1 — resolution verifiedThe P1 finding was valid: the Fan-out section's zero-blocker direct-gate path retained the flagless Commit 1. SKILL.md — Fan-out paragraph (lines 257–261) 2. 3. 4. Merge-surface sweep completeness
5. Line cap: SKILL.md is at exactly 499/500 — the commit's "net-zero" claim holds. Remaining low-priority observations (unchanged from prior review)These were identified in the previous review pass and are still present. None are blocking.
SummaryThe Codex P1 finding is correctly resolved. No new correctness bugs, security issues, or behavioral regressions are introduced. Both autopilot merge surfaces (§3 worker-final-push and the Fan-out direct-gate) now consistently point at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f6e56d5cf
ℹ️ 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".
…pprove mechanic (#675) Codex P2 on #706: the enabled-tier second-account approve mechanic told the agent to "run a genuine review pass through the review plugin", but source-control declares no dependency on a review plugin and the skill offered no fallback — so in a source-control-only install the step read as an unsatisfiable hard prerequisite. Degrade the instruction instead of adding a dependency: the review pass runs through a review skill/plugin when one is installed, otherwise an equivalent thorough manual review. The gate's actual criterion is only that a distinct-bot approval exists on the live head, not that any particular tool produced it, so a hard plugin dependency would over-couple against the repo's repo-agnostic / configurable-without-editing design rules. Docs-only; no gate/script change and the tier still ships DISABLED. Refs #675 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e780eae33
ℹ️ 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".
…, not just be required (#675) Codex P1 (re-review of 8e780ea) on #706: the review-workflow requiredness precondition over-claimed. It said making the review workflow a required status context "closes that hole deterministically", but requiredness alone does not guarantee the review RAN. A conditionally-skipped review job can report a SKIPPED conclusion that is counted as a passing state — this skill's own babysit_checks.py classifies SKIPPED as success (CHECK_SUCCESS_STATES) — so a required-but-skipped review still reads mergeStateStatus == CLEAN without having gated anything, and the enabled tier would proceed on its bot approval alone. Strengthen the operator precondition (docs-only, in #706's own scope): - Enabling now requires the review workflow to be a required status context AND to always run to a non-skipped conclusion on every PR to the protected base. - Qualify "deterministically": requiring the context closes the hole only when the workflow cannot conditionally skip on the tier's PRs. - Extend "do not enable the tier" to the skippable-on-those-PRs case. Code-side enforcement (a merge-gate predicate that verifies the review context actually executed, or exempting the review context from SKIPPED-as-success) belongs in the feature (#476/#665), not this docs PR, and is left for that surface. Refs #675 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed71ed698d
ℹ️ 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".
…ecify the approve mechanic and review-context precondition (#675) Close the three coherence gaps that gate the autopilot merge tier's (#476) gate-off flip. All are prose/contract changes plus test-pinning; no behavioral change to the merge gate or any script, and the tier still ships DISABLED. - §3 wiring (fork b, reference-file restructure): autopilot's step 3 in SKILL.md no longer inlines a base-only merge command that would ignore the tier flags when the tier is enabled. It points at reference/safety.md, now the single home for both the base and the enabled-tier merge paths, so an ENABLED config can no longer merge via the flagless base path. Chosen over raising the skill-quality line cap (SKILL.md sits at 499/500); the restructure buys no lines but removes the duplicated four-flag list from the hub. - Second-account approve mechanic: the concrete out-of-band approval the gate's distinct-bot criterion requires is now specified — `gh pr review … --approve` under a distinct <approver-bot-logins> identity (GH_TOKEN or gh auth switch, never the PR author or a lane identity), only after a genuine clean review pass, on the live head so the --expected-head pin holds. - Review-workflow requiredness precondition (fork a): enabling the tier now carries a documented operator precondition — the base branch ruleset must make the review workflow a required status context, so mergeStateStatus == CLEAN actually proves the review ran. Chosen over a merge-gate review-context config (rejected fork b) to keep the gate deterministic with nothing new to wire. The skill-contract tests are split/extended to pin all three contracts against drift; the worker-tier push paragraph (no merge tier) keeps its inline command assertion. Version bumped to 0.14.1 with a CHANGELOG entry. Closes #675 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
… surfaces to the tier flags too (#675) Codex P1 on #706: the §3 fix wired only the autopilot worker-final-push merge path, but the "Fan out" section also sends already-clean zero-blocker autopilot PRs straight to the DIRECT merge gate with the flagless base command (`--merge --expected-head <snapshotted-head-sha>`), so an ENABLED tier would merge those without the lane / approver / block-label / issue-link / human-veto criteria — the same fail-open the flip precondition must close, and it made this PR's own "never merges via the base path" claim false for clean PRs. Sweep every autopilot merge surface, not just the one Codex caught: - Zero-blocker direct-gate path (SKILL.md): an enabled autopilot merge tier now adds the tier flags (reference/safety.md), never the flagless base command. Unpinned prose in the same paragraph is tightened so the caveat lands net-zero against the 499/500 line cap (no cap raise — the operator rejected that). - Pinned-Command Degradation operator handoff (safety.md): when the tier is enabled the degraded merge handoff reproduces the tier-flagged command, so a harness-blocked but gate-proven merge is not handed to the operator as a flagless base merge. The base-command occurrences that remain (generic Merge readiness wrapper contract; worker-tier §7 push path, which never has a merge tier) are correct as base and left unchanged. New skill-contract assertion pins the direct-gate paragraph's tier-enabled behavior so the universal claim is tested, not just prose-asserted. No script change — the gate already fails closed when handed the flags on a PR lacking the distinct-bot approval. Refs #675 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…pprove mechanic (#675) Codex P2 on #706: the enabled-tier second-account approve mechanic told the agent to "run a genuine review pass through the review plugin", but source-control declares no dependency on a review plugin and the skill offered no fallback — so in a source-control-only install the step read as an unsatisfiable hard prerequisite. Degrade the instruction instead of adding a dependency: the review pass runs through a review skill/plugin when one is installed, otherwise an equivalent thorough manual review. The gate's actual criterion is only that a distinct-bot approval exists on the live head, not that any particular tool produced it, so a hard plugin dependency would over-couple against the repo's repo-agnostic / configurable-without-editing design rules. Docs-only; no gate/script change and the tier still ships DISABLED. Refs #675 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…, not just be required (#675) Codex P1 (re-review of 8e780ea) on #706: the review-workflow requiredness precondition over-claimed. It said making the review workflow a required status context "closes that hole deterministically", but requiredness alone does not guarantee the review RAN. A conditionally-skipped review job can report a SKIPPED conclusion that is counted as a passing state — this skill's own babysit_checks.py classifies SKIPPED as success (CHECK_SUCCESS_STATES) — so a required-but-skipped review still reads mergeStateStatus == CLEAN without having gated anything, and the enabled tier would proceed on its bot approval alone. Strengthen the operator precondition (docs-only, in #706's own scope): - Enabling now requires the review workflow to be a required status context AND to always run to a non-skipped conclusion on every PR to the protected base. - Qualify "deterministically": requiring the context closes the hole only when the workflow cannot conditionally skip on the tier's PRs. - Extend "do not enable the tier" to the skippable-on-those-PRs case. Code-side enforcement (a merge-gate predicate that verifies the review context actually executed, or exempting the review context from SKIPPED-as-success) belongs in the feature (#476/#665), not this docs PR, and is left for that surface. Refs #675 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…strengthened precondition (#675) The [0.15.1] entry's review-context bullet still summarized requiredness as sufficient ("a required status context, so mergeStateStatus == CLEAN actually proves the review ran"), which the strengthened safety.md precondition (commit "require the review workflow to actually execute, not just be required") explicitly refutes: a required-but-skipped review reads CLEAN without having gated anything, so requiredness is necessary but not sufficient. Align the CHANGELOG summary with the reference doc so the two do not contradict — the workflow must also always run to a non-skipped conclusion. Docs-only. Refs #675 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
ed71ed6 to
e09e4ae
Compare
|
Claude finished @kyle-sexton's task in 4m 16s —— View job PR Review
This review covers the full diff against Open P1: direct-gate path missing the review/approve step (unresolved)The Codex P1 posted on commit The gap. The Fan-out paragraph ( The tension with the tier paragraph.
Concrete fix. The Fan-out paragraph's tier-enabled sentence (lines 258–259) needs to expand from "but an enabled autopilot merge tier adds the tier flags (reference/safety.md), never the flagless base command" to also direct the review/approve step first. Something like:
And the Remaining low-priority observations (carried from prior reviews, still present)
SummaryThe P1 Codex finding from commit |
Closes #675
Gate-off flip precondition for the autopilot merge tier (#476). All changes are prose/contract + test-pinning: no behavioral change to the merge gate or any script, and the tier still ships DISABLED.
Implements the operator-approved RECOMMENDED options recorded on #675:
SKILL.mdno longer inlines a base-only merge command that would ignore the tier flags when the tier is enabled. It now points atreference/safety.md, the single home for both the base and the enabled-tier merge paths, so an ENABLED config can no longer merge via the flagless base path. Chosen over raising the skill-quality line cap (SKILL.mdsits at 499/500).gh pr review … --approveunder a distinct<approver-bot-logins>identity (GH_TOKENorgh auth switch, never the PR author or a lane identity), only after a genuine clean review pass, on the live head so--expected-headholds.mergeStateStatus == CLEANactually proves the review ran. Chosen over a merge-gate review-context config (rejected fork b) to keep the gate deterministic with nothing new to wire — and deliberately kept distinct from the existing snapshot-sidebabysit_review_gate_context(a review-trigger timing signal, not a merge predicate).The skill-contract tests are split/extended to pin all three contracts against drift (the worker-tier push paragraph, which has no merge tier, keeps its inline-command assertion).
Related
feat/476-autopilot-merge-tier) and must land after it — this PR is based on that branch, notmain; retarget tomainonce feat(source-control): babysit autopilot merge tier, gate-enforced and disabled by default #665 merges.0.14.1, continuous with the current base (feat/476@0.14.0). feat(source-control): babysit autopilot merge tier, gate-enforced and disabled by default #665's plugin version is being re-composed to0.15.0(collision with feat(source-control): layer convention config across three layers #660's0.14.0); when feat(source-control): babysit autopilot merge tier, gate-enforced and disabled by default #665 lands, rebase this and re-bump to one increment past whatevermainthen carries (expected0.15.1).Verification
engine.test.sh: 312 tests pass, ruff clean, guarded-wrapper behavior checks pass.check-skill.sh babysit-prs: PASS (SKILL.md 499/500, internal refs resolve, markdownlint clean).safety.md+CHANGELOG.md: 0 errors.🤖 Generated with Claude Code