diff --git a/AGENTS.md b/AGENTS.md index 164cc1ea87..d6a4ddbbc0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -178,12 +178,15 @@ capability-owned review. Control-plane, runtime and product surfaces (`loopx/**`, `apps/**`, `packages/**`, permissions/authority, persisted state/receipts, quota/scheduler/todo rules, and CLI/API contracts) require explicit maintainer merge authorization. Propose and -review the exact head, pass required validation, and obtain ready merge-readiness +review the exact head, pass required validation, and run merge-readiness (`uv run --extra test loopx pr-review --check-merge-readiness NUMBER@HEAD_OID`, -plus `--goal-id GOAL` for Goal-managed work) for that unchanged head before any -explicitly authorized merge or admin bypass. Small diffs, green CI or published -reviews do not grant merge authority; without authorization, leave the merge to -the maintainer. +plus `--goal-id GOAL` for Goal-managed work) for that unchanged head. A +`ready=true` result permits the normal path. Explicit maintainer authorization +may use admin bypass to override a `ready=false` readiness or GitHub policy +hold, but never missing exact-head review, required validation, unresolved +findings or threads, a changed head, an unmergeable conflict, or absent merge +authority. Small diffs, green CI or published reviews do not grant merge +authority; without authorization, leave the merge to the maintainer. For author-owned PRs, GitHub blocks formal self-approval: publish a `COMMENTED` review on the exact head carrying the approval conclusion and an English verdict. diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index 7c991d5b56..e1a61fa22d 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -87,7 +87,7 @@ def main() -> int: "Read the published review back", "Route approval, merge, self-merge, and admin bypass to `loopx-pr-merge`", "--check-merge-readiness NUMBER@HEAD_OID", - "admin bypass never overrides this gate", + "admin bypass may override a readiness or GitHub policy hold", "Full PR Review And Bilingual Format", "findings-only or blocker-only body is incomplete", "Cover every changed surface and key symbols", @@ -161,9 +161,9 @@ def main() -> int: "loopx --format json pr-review --state all", "agent_response_contract.review_execution_contract", "--check-merge-readiness NUMBER@HEAD_OID", - "ready=true", + "Record the readiness result", "completion_gate", - "admin bypass never overrides this gate", + "admin bypass may override a readiness or GitHub policy hold", "A merge decision without this evidence is not authorized", ): assert phrase in merge_text, phrase diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 0b7ef7374c..21173cc08d 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -933,10 +933,15 @@ update is not required merely because GitHub reports `BEHIND`: the gate also reads GitHub's independent `mergeable` result and permits a conflict-free `MERGEABLE` head when every other check passes. `CONFLICTING` or `DIRTY` still requires repair; `BEHIND` with missing or unknown mergeability remains unverified. -Branch protection may still require a separately authorized admin bypass. An admin -bypass may satisfy GitHub's author-owned -self-review limitation, but it never overrides this capability gate or supplies -user merge authority. +Branch protection may still require a separately authorized admin bypass. An +explicit maintainer admin bypass may override a `ready=false` readiness or +GitHub policy hold, including GitHub's author-owned self-review limitation. It +does not make missing exact-head review, required validation, unresolved review +threads or findings, a changed head, or an unmergeable conflict optional, and +it never supplies merge authority by itself. +The `admin_bypass_overrides_this_gate` authority field describes whether every +blocker on this exact observation is in that override scope; it is false when +non-bypassable review, thread, head, or mergeability evidence is missing. `pull_request_merge_readiness_observation_v0` is the Goal-scoped scheduling receipt for that gate. It persists only the exact-head material fingerprint and @@ -1262,10 +1267,14 @@ The packet should let a reviewer move through PRs in order: 7. Treat `metadata_risk_hint` only as queue-ordering metadata. It must not be copied as the final risk judgement. 8. Recheck the exact head, then decide `approve`, `request changes`, `defer`, or - `merge after checks`. Immediately before merge, require - `loopx pr-review --check-merge-readiness NUMBER@HEAD_OID` to - return `ready=true`. Add `--goal-id GOAL` only for already Goal-managed work; - standalone delivery requires no registration or binding. + `merge after checks`. Immediately before merge, run + `loopx pr-review --check-merge-readiness NUMBER@HEAD_OID` and record its + result. Add `--goal-id GOAL` only for already Goal-managed work; standalone + delivery requires no registration or binding. `ready=true` permits the + normal path. Explicit maintainer authorization may use admin bypass to + override a `ready=false` readiness or GitHub policy hold, but never missing + exact-head review, required validation, unresolved findings or threads, a + changed head, an unmergeable conflict, or absent merge authority. A response that only lists `Open` and `Merged` PRs, scale, and recommended next order is incomplete for `/loopx-pr-review`; it should continue into the diff --git a/loopx/capabilities/pr_review_queue/merge_readiness.py b/loopx/capabilities/pr_review_queue/merge_readiness.py index ba1114dd5a..9725d4213b 100644 --- a/loopx/capabilities/pr_review_queue/merge_readiness.py +++ b/loopx/capabilities/pr_review_queue/merge_readiness.py @@ -12,6 +12,16 @@ SCHEMA_VERSION = "pull_request_merge_readiness_v0" +_ADMIN_BYPASS_OVERRIDABLE_BLOCKERS = frozenset( + { + "github_review_decision_not_approved", + "repository_merge_state_blocked", + "status_checks_failed", + "status_checks_incomplete", + "status_checks_missing", + "status_checks_pending", + } +) def _mapping(value: object) -> Mapping[str, Any]: @@ -31,7 +41,7 @@ def build_merge_readiness( source: str, wait_for_ci: bool = True, ) -> dict[str, Any]: - """Return a read-only pre-merge gate without granting merge authority.""" + """Report readiness; explicit admin bypass can cover policy holds, not evidence.""" number = item.get("number") head_oid = _text(item.get("head_oid")) @@ -97,6 +107,9 @@ def build_merge_readiness( blockers.append("repository_merge_state_blocked") blockers = list(dict.fromkeys(blockers)) + admin_bypass_overrides_this_gate = not any( + blocker not in _ADMIN_BYPASS_OVERRIDABLE_BLOCKERS for blocker in blockers + ) material_state = readiness_material_state( repository=repository, item=item, @@ -131,7 +144,14 @@ def build_merge_readiness( "material_fingerprint": readiness_material_fingerprint(material_state), "authority": { "grants_merge_authority": False, - "admin_bypass_overrides_this_gate": False, + "admin_bypass_overrides_this_gate": admin_bypass_overrides_this_gate, + "admin_bypass_scope": "readiness_and_github_review_policy", + "admin_bypass_requires": [ + "explicit_user_or_maintainer_merge_authorization", + "valid_exact_head_review_and_required_validation", + "complete_review_threads_without_unresolved_findings", + "open_unchanged_mergeable_head", + ], "required_timing": "immediately_before_merge", "head_change_action": "restart_review_and_rerun_gate", }, diff --git a/skills/loopx-pr-merge/SKILL.md b/skills/loopx-pr-merge/SKILL.md index d18a901059..c0f0c96302 100644 --- a/skills/loopx-pr-merge/SKILL.md +++ b/skills/loopx-pr-merge/SKILL.md @@ -34,16 +34,18 @@ authorized: a diff read, green CI, or pull-request metadata is not a substitute. remote reviewed head. The source-checkout `loopx canary premerge` includes the same direct blocking check. A failed check holds merge regardless of CI waiting or authorized admin bypass. Working-tree changes cannot satisfy it. -4. Immediately before the merge, run `loopx --format json pr-review --repo - OWNER/REPO --check-merge-readiness NUMBER@HEAD_OID`. Merge only when it - returns `ready=true` for that unchanged head. For work already managed by a - Goal, pass `--goal-id GOAL` to retain its configured CI policy and record the - compact readiness observation. Standalone work needs no Goal, registration - or binding; its readiness check performs no Goal write. - -A rebase or head update restarts review, and admin bypass never overrides this -gate: it needs explicit owner authorization and never substitutes for the -evidence. +4. Record the readiness result immediately before merge: run `loopx --format + json pr-review --repo OWNER/REPO --check-merge-readiness NUMBER@HEAD_OID`. + `ready=true` permits the normal merge path. With explicit maintainer + authorization, admin bypass may override a readiness or GitHub policy hold. + The bypass does not replace valid exact-head review, + required validation, complete review threads without unresolved findings, an + open unchanged mergeable head, or the separately granted merge authority. + For Goal-managed work, pass `--goal-id GOAL` to retain its configured CI + policy and record the compact readiness observation. Standalone work needs + no Goal, registration or binding; its readiness check performs no Goal write. + +A rebase or head update restarts review and readiness qualification. ## Configured CI Waiting @@ -82,11 +84,13 @@ Control-plane, runtime and product surfaces require explicit maintainer merge authorization. This includes behavior in `loopx/**`, `apps/**` or `packages/**`, permissions or authority, persisted state/receipts, quota/scheduler/todo rules, and CLI/API contracts. Propose and review the exact head, pass required -validation, and obtain ready merge-readiness for that unchanged head. Only then -may an explicitly authorized merge or delivery batch use admin bypass. Small -diffs, green CI or published reviews do not grant merge authority. Unclear -affected boundaries use the same authorization rule; without it, leave the -merge to the maintainer. Bypass never replaces review, validation or readiness. +validation, and run readiness for that unchanged head. An explicit maintainer +authorization may then use admin bypass when readiness remains false for a +readiness or GitHub policy hold. Small diffs, green CI or published reviews do +not grant merge authority. Unclear affected boundaries use the same +authorization rule; without it, leave the merge to the maintainer. Bypass never +replaces exact-head review, validation, thread/finding resolution or merge +authority. Maintainers may self-merge single-purpose, low-risk public-doc, contributor metadata, test, example or narrow-cleanup PRs when all these conditions hold: diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index 4826bd4f38..a520a27588 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -135,11 +135,13 @@ owner/GitHub authority, preserving discussion and unresolved reviews. Merge rout `loopx-pr-merge`; `APPROVE` is not merge authority; a public blocker belongs on the PR. Immediately before every merge, run `loopx --format json pr-review --repo OWNER/REPO ---check-merge-readiness NUMBER@HEAD_OID`; require `ready=true`. Pass `--goal-id GOAL` -when the work is already Goal-managed; its compact observation suppresses only unchanged -requalification. Standalone work requires no Goal and writes no Goal observation. -Material change reopens qualification, admin bypass never overrides this gate, and author -fallback needs user authority. +--check-merge-readiness NUMBER@HEAD_OID`. Pass `--goal-id GOAL` when the work is already +Goal-managed; its compact observation suppresses only unchanged requalification. +Standalone work requires no Goal and writes no Goal observation. `ready=true` needs no +bypass. Explicit maintainer authorization means admin bypass may override a readiness +or GitHub policy hold, but never missing exact-head review, +required validation, unresolved findings or threads, a changed head, an unmergeable +conflict, or absent merge authority. Material change reopens qualification. ## Full PR Review And Bilingual Format diff --git a/skills/loopx-self-repair/references/repair-patterns.md b/skills/loopx-self-repair/references/repair-patterns.md index 2f02d796ee..b25788374b 100644 --- a/skills/loopx-self-repair/references/repair-patterns.md +++ b/skills/loopx-self-repair/references/repair-patterns.md @@ -350,6 +350,20 @@ raw logs and private traces stay in ignored local paths. | A prompt migration reports success but the App restores the old body | A running host caches automation rows; disk CAS cannot invalidate scheduler memory. Use native automation_update and read back through that host, or close the host for offline migration and verify after restart. | Keep update-time reconciliation fail-closed while the App runs, and cover unchanged SQLite/TOML plus a complete native update request. | | A newly started heartbeat persists execution rules despite a successful runtime update | Startup and migration used different renderers, and installed skill guidance still told the agent to save raw thin output. Generate the v2 loader from the activation command and replace stale lifecycle instructions. | Exercise onboarding to bootstrap to one-level fresh CLI load, authentic legacy migration, and absent/duplicate-command rejection. | +## Readiness bypass is misreported as impossible + +Symptom: a reviewer has explicit maintainer authorization for an admin-bypass +merge and completed exact-head review plus required validation, but the +readiness payload, skills, AGENTS policy, and smoke independently describe +admin bypass as unable to override `ready=false`. Treat readiness as a truthful +diagnostic and two separate decisions: whether the normal merge path is ready, +and whether an authorized bypass may cover a readiness or GitHub policy hold. +Keep exact-head review, required validation, changed-head detection, merge +conflicts, review-thread/finding resolution, and merge authority as separate +non-bypass prerequisites. Repair the payload field plus every instruction and +smoke that repeats the old rule, then exercise one denied-by-default path and +one explicitly authorized bypass path. + ## Review ignores a Goal's CI waiting configuration Symptom: a managed review ignores `pull_request_review.wait_for_ci=false` and waits on remote CI after local evidence is complete. Read the named Goal's configuration and pass `--goal-id` through both review and readiness. Repair the capability transport, packet, and readiness owner together; retain required local checks, exact-head binding, valid approval, unresolved-thread rejection, and merge authority. Never change the global default to repair one Goal. Validate both default-on and explicit-off paths through the production CLI and the shared configuration editor. diff --git a/tests/test_pr_review_github_scan.py b/tests/test_pr_review_github_scan.py index f617374dd7..aea5ce2c84 100644 --- a/tests/test_pr_review_github_scan.py +++ b/tests/test_pr_review_github_scan.py @@ -1062,9 +1062,44 @@ def test_merge_readiness_binds_review_body_checks_and_threads_to_exact_head() -> assert ready["ready"] is True, ready assert ready["blocking_reasons"] == [], ready assert ready["authority"]["grants_merge_authority"] is False, ready + assert ready["authority"]["admin_bypass_overrides_this_gate"] is True, ready + assert ready["authority"]["admin_bypass_scope"] == ( + "readiness_and_github_review_policy" + ), ready assert ready["review_conclusion"]["verdict"] == "APPROVE", ready +def test_explicit_admin_bypass_may_override_a_readiness_policy_hold() -> None: + pr = _merge_ready_pr(review_decision="REVIEW_REQUIRED") + pr["mergeStateStatus"] = "BLOCKED" + readiness = merge_readiness_module.build_pr_merge_readiness_packet( + pull_request=pr, + repository="owner/repo", + expected_exact_head=f"4110@{HEAD_1}", + reviewer_login="maintainer", + review_threads=_complete_review_threads(), + source="fixture", + wait_for_ci=False, + ) + + assert readiness["ready"] is False, readiness + assert readiness["blocking_reasons"] == ["github_review_decision_not_approved"] + assert readiness["admin_bypass_required"] is True + assert readiness["authority"] == { + "grants_merge_authority": False, + "admin_bypass_overrides_this_gate": True, + "admin_bypass_scope": "readiness_and_github_review_policy", + "admin_bypass_requires": [ + "explicit_user_or_maintainer_merge_authorization", + "valid_exact_head_review_and_required_validation", + "complete_review_threads_without_unresolved_findings", + "open_unchanged_mergeable_head", + ], + "required_timing": "immediately_before_merge", + "head_change_action": "restart_review_and_rerun_gate", + } + + def test_merge_readiness_rejects_review_text_for_pre_update_head() -> None: # GitHub may associate a preserved review with the merge-from-main commit even # though its public evidence still names the earlier reviewed head. @@ -1083,6 +1118,7 @@ def test_merge_readiness_rejects_review_text_for_pre_update_head() -> None: assert stale["ready"] is False, stale assert "current_head_review_missing_or_invalid" in stale["blocking_reasons"] + assert stale["authority"]["admin_bypass_overrides_this_gate"] is False assert ( "review_body_missing_standalone_bilingual_format" in (stale["review_conclusion"]["invalid_reasons"])