diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 794f40b997..8116387efc 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -991,7 +991,13 @@ A first implementation is acceptable when: `pull_request_review.wait_for_ci` defaults to `true`. Machine defaults use the existing capability editor. A Goal may override the complete review namespace; clearing that override restores live machine defaults. Local required validation -and exact-head review/thread gates apply in both modes. Disabling CI waiting +and exact-head review/thread gates apply in both modes. Attribute a red required +check before selecting the review verdict: an unchanged failure reproduced on +the immutable base and exact head, or an independently evidenced external +outage, is not a reason to request code changes on an unrelated PR when its +changed invariant has separate passing coverage. Record the red check and its +owner; approval does not make a blocked merge ready. A new, worsened or +unattributed failure remains a review blocker. Disabling CI waiting also removes CI requests and waiting instructions; legacy supplied summaries are diagnostic only. It grants no publication, merge, or admin-bypass authority. diff --git a/loopx/capabilities/pr_review_queue/result_check.py b/loopx/capabilities/pr_review_queue/result_check.py index 00547a3083..77c1bbff86 100644 --- a/loopx/capabilities/pr_review_queue/result_check.py +++ b/loopx/capabilities/pr_review_queue/result_check.py @@ -8,6 +8,7 @@ OUTCOME_IMPACT_ASSESSMENT, SCOPE_COVERAGE_ASSESSMENT, SEMANTIC_CANDIDATE_DECISIONS, + VALIDATION_FAILURE_ATTRIBUTION, build_review_execution_contract, build_review_plan, ) @@ -97,6 +98,61 @@ def _required_validation_case_ids( return required +def _check_validation_failures( + blockers: list[str], items: list[Mapping[str, Any]], +) -> None: + """Separate review causality from the merge gate's red-check observation.""" + + for item in items: + case_id = str(item.get("case_id") or "unknown") + key = f"validation_matrix:{case_id}" + status = item.get("status") + if not isinstance(status, str) or status not in { + "passed", "failed", "skipped", "pending", "unverified", "not_applicable", + }: + blockers.append(f"{key}:invalid_status") + if type(item.get("required")) is not bool: + blockers.append(f"{key}:required_not_boolean") + if not isinstance(status, str): + continue + if item.get("required") is not True or status == "passed": + continue + if status in {"pending", "unverified", "not_applicable"}: + blockers.append(f"{key}:required_validation_not_proven") + continue + attribution = item.get("failure_attribution") + if not isinstance(attribution, Mapping): + blockers.append(f"{key}:failure_attribution_missing") + continue + disposition = attribution.get("disposition") + if disposition not in VALIDATION_FAILURE_ATTRIBUTION["dispositions"]: + blockers.append(f"{key}:invalid_failure_disposition") + continue + if disposition not in VALIDATION_FAILURE_ATTRIBUTION["non_blocking_dispositions"]: + blockers.append(f"{key}:attributable_or_unresolved_failure") + continue + _require_fields( + blockers, evidence_id=key, value=attribution, + fields=VALIDATION_FAILURE_ATTRIBUTION["common_fields"], + ) + if disposition == "pre_existing_unrelated": + _require_fields( + blockers, evidence_id=key, value=attribution, + fields=VALIDATION_FAILURE_ATTRIBUTION["pre_existing_fields"], + ) + baseline = attribution.get("baseline_failure_signature") + head = attribution.get("head_failure_signature") + if not isinstance(baseline, str) or not baseline.strip() or baseline != head: + blockers.append(f"{key}:failure_signature_changed") + if attribution.get("base_revision") == attribution.get("head_revision"): + blockers.append(f"{key}:base_and_head_not_distinct") + else: + _require_fields( + blockers, evidence_id=key, value=attribution, + fields=VALIDATION_FAILURE_ATTRIBUTION["external_fields"], + ) + + def _check_compatibility_assessment(blockers: list[str], value: object) -> None: key = "code_volume:compatibility_assessment" contract = COMPATIBILITY_ASSESSMENT @@ -286,6 +342,8 @@ def check_review_result( row=row, requirement=requirement, ) + if key == "validation_matrix": + _check_validation_failures(blockers, items) positive_field = requirement.get("positive_field") if isinstance(positive_field, str): _require_fields( @@ -350,6 +408,8 @@ def check_review_result( errors.append("unsupported_verdict") if verdict == "APPROVE" and blockers: errors.append("approval_contradicts_evidence") + if verdict == "REQUEST_CHANGES" and not blockers: + errors.append("request_changes_without_blocker") return { "ok": not errors, "schema_version": "pull_request_review_result_check_v0", diff --git a/loopx/capabilities/pr_review_queue/review_contract.py b/loopx/capabilities/pr_review_queue/review_contract.py index 9df3d2ef02..f9d9bd4856 100644 --- a/loopx/capabilities/pr_review_queue/review_contract.py +++ b/loopx/capabilities/pr_review_queue/review_contract.py @@ -7,7 +7,36 @@ from .review_body import REQUIRED_FINAL_SECTIONS, review_body_requirements # Increment when review requirements change without changing the packet shape. -REVIEW_POLICY_REVISION = 10 +REVIEW_POLICY_REVISION = 11 + +# A red check is an observation, not evidence that the reviewed PR caused it. +# This contract belongs to review judgment; merge readiness still owns whether +# an unresolved required check permits integration. +VALIDATION_FAILURE_ATTRIBUTION = { + "dispositions": [ + "pr_regression", "pre_existing_unrelated", "external_unrelated", "unresolved", + ], + "non_blocking_dispositions": ["pre_existing_unrelated", "external_unrelated"], + "common_fields": ["disposition", "causal_scope_analysis", "affected_invariant_evidence"], + "pre_existing_fields": [ + "base_revision", "head_revision", "same_command", + "baseline_observation", "head_observation", + "baseline_failure_signature", "head_failure_signature", + ], + "external_fields": ["independent_evidence", "retry_or_recovery_owner"], + "rule": ( + "Classify every required failed or skipped validation before choosing a review verdict. " + "A pre-existing failure is non-blocking for review only when the same check on an " + "immutable base and exact head has the same normalized failing identity and detail, " + "the PR does not alter that failure's causal path, and the changed invariant has " + "independent passing evidence. Equal aggregate counts alone are insufficient. " + "An external failure needs independent outage or infrastructure evidence, a recovery " + "owner, and separate coverage of the changed invariant. Otherwise classify it as " + "pr_regression or unresolved and request changes. Report unrelated red checks and " + "their recovery separately from the PR verdict: APPROVE may be correct while merge " + "readiness remains on hold. Never relax a hard limit or required check to make it green." + ), +} OUTCOME_IMPACT_ASSESSMENT = { "dimensions": ["long_horizon", "user_experience"], @@ -718,13 +747,16 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An "ci_policy": "required" if wait_for_ci else "not_consulted", "wait_for_ci": wait_for_ci, "validation_source": ( - "Repository-native local validation and final CI are required." + "Repository-native local validation and final CI observation are required. " + "Attribute failed checks before judging the PR; an unrelated red check " + "may hold merging without requiring code changes on this PR." if wait_for_ci else "Repository-native local validation at the reviewed head. " "Do not fetch, poll, or wait for GitHub CI. Missing, pending, " - "or failed remote CI is not a review evidence gap. Local " - "required validation failures and skips remain blocking." + "or failed remote CI is not a review evidence gap. Attribute local " + "failures against the base and changed invariant before judging the PR." ), + "failure_attribution": VALIDATION_FAILURE_ATTRIBUTION, "required_when": "always", "items_field": "items", "item_fields": [ @@ -1063,6 +1095,13 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An }, "verdict_policy": { "open_pr_blocking_finding": "REQUEST_CHANGES", + "unrelated_validation_failure": ( + "APPROVE when a required red check is independently attributed to an unchanged " + "pre-existing failure or external infrastructure, and the PR's changed " + "invariant is covered. Record the separate merge-readiness hold; do not ask " + "this PR to repair unrelated code or budgets. Unattributed, introduced, or " + "worsened failures still block approval." + ), "open_pr_unjustified_delivery": ( "REQUEST_CHANGES when problem_context is off_goal, fragmented or " "not_yet_proven. Green checks cannot replace an evidenced goal delta; " @@ -1288,7 +1327,7 @@ def build_agent_response_contract(*, wait_for_ci: bool = True) -> dict[str, Any] "Before evidence commands, obey pull_requests[].review_action_kind. A null action stays in pull_requests inventory but is excluded from review_sequence, carries no execution artifacts, and remains readback-only; generic re-review wording selects the PR but does not force duplicate evidence for an already concluded or merged no-action row.", "Execute each non-null pull_requests[].review_plan against the shared review_execution_contract before drafting prose.", "Do not infer verified evidence from title, labels, changed-file counts, metadata_risk_hint, or green CI alone.", - ("Require final CI in addition to repository-native local validation." if wait_for_ci else "Do not fetch, poll, or wait for CI for approval or merge readiness. repository_required_checks means repository-native local validation; missing required local evidence remains blocking."), + ("Observe final CI in addition to repository-native local validation, then attribute red checks before judging this PR; review approval and merge readiness are separate." if wait_for_ci else "Do not fetch, poll, or wait for CI for review or merge readiness. repository_required_checks means repository-native local validation; attribute base-equivalent failures and keep missing affected-invariant evidence blocking."), "Recheck the exact remote head before verdict and publication.", "Render the verified result through a non-null pull_requests[].review_template; host skills must not maintain a competing depth checklist.", ], diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index 50704eb736..440bd17d90 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -94,7 +94,7 @@ When `review_action_kind` is null, the row stays in `pull_requests` inventory bu Each PR needs independent evidence and a standalone card; a queue table is only a preface. -For managed review, pass `--goal-id GOAL` and follow the packet’s resolved `wait_for_ci`: false means never fetch, poll, or wait for CI; true retains CI validation. Required local failures/skips always block. Configure one Goal with `configure-goal --goal-id GOAL --no-pr-review-wait-for-ci --execute`; clear with `--clear-pr-review-configuration --execute`. +For managed review, pass `--goal-id GOAL` and follow the packet’s resolved `wait_for_ci`: false means never fetch, poll, or wait for CI; true retains CI observation. Apply the packet's `validation_matrix.failure_attribution` before treating a red required check as a PR blocker. An independently verified unchanged baseline failure or external outage can hold merge readiness without forcing `REQUEST_CHANGES` on an unrelated PR; missing attribution or missing affected-invariant coverage still blocks approval. Configure one Goal with `configure-goal --goal-id GOAL --no-pr-review-wait-for-ci --execute`; clear with `--clear-pr-review-configuration --execute`. ## Publish And Read Back For an open PR, publish validated actionable findings by default unless the user diff --git a/tests/capabilities/test_pr_review_contract.py b/tests/capabilities/test_pr_review_contract.py index e029cb7992..c97d87988e 100644 --- a/tests/capabilities/test_pr_review_contract.py +++ b/tests/capabilities/test_pr_review_contract.py @@ -104,6 +104,15 @@ def test_execution_contract_owns_deep_review_requirements() -> None: "silent behavior changes" in requirements["behavior_change_disclosure"]["rule"] ) assert "must_attempt_work" in requirements["guidance_vs_obligation"]["rule"] + attribution = requirements["validation_matrix"]["failure_attribution"] + assert attribution["non_blocking_dispositions"] == [ + "pre_existing_unrelated", "external_unrelated", + ] + assert "same normalized failing identity" in attribution["rule"] + assert "merge readiness remains on hold" in attribution["rule"] + assert "APPROVE when a required red check" in contract["verdict_policy"][ + "unrelated_validation_failure" + ] proportionality = requirements["change_proportionality"] assert proportionality["required_when"] == "code_change" assert proportionality["verdict_values"] == [ diff --git a/tests/capabilities/test_pr_review_result_check.py b/tests/capabilities/test_pr_review_result_check.py index d2ba00ea8a..bbf89e800c 100644 --- a/tests/capabilities/test_pr_review_result_check.py +++ b/tests/capabilities/test_pr_review_result_check.py @@ -72,6 +72,8 @@ def _review(*, area="product_runtime"): field: ( case_id if field == "case_id" + else "passed" + if field == "status" else True if field == "required" else "Synthetic consistency fixture, not a real review." @@ -101,6 +103,97 @@ def test_result_check_is_not_semantic_or_merge_authority(): assert not checked["external_writes_performed"] +def _required_red_review(): + packet, result = _review() + row = next(item for item in result["evidence"]["validation_matrix"]["items"] + if item["case_id"] == "repository_required_checks") + row.update(status="failed", result="The same maintained-twin rule fails at base and head.", + skip_or_failure_reason="The same rule_a/rule_b pair remains over the 43 limit at both revisions.") + return packet, result, row + + +def test_unrelated_baseline_red_check_does_not_force_request_changes(): + packet, result, row = _required_red_review() + row["failure_attribution"] = { + "disposition": "pre_existing_unrelated", + "causal_scope_analysis": "The reviewed change edits a presentation helper, not the vocabulary scanner or its inputs.", + "affected_invariant_evidence": "Focused presentation tests pass at the exact head.", + "base_revision": "b" * 40, + "head_revision": "a" * 40, + "same_command": "python examples/semantic-vocabulary-drift-smoke.py", + "baseline_observation": "Exit 1: maintained twin rule_a/rule_b, budget 44/43.", + "head_observation": "Exit 1: maintained twin rule_a/rule_b, budget 44/43.", + "baseline_failure_signature": "semantic-vocabulary-drift: rule_a/rule_b: maintained twin 44/43", + "head_failure_signature": "semantic-vocabulary-drift: rule_a/rule_b: maintained twin 44/43", + } + checked = check_review_result(packet, result) + assert checked["ok"] and checked["approval_consistent"] + result["verdict"] = "REQUEST_CHANGES" + result["review_body"] = result["review_body"].replace( + "English verdict: APPROVE", "English verdict: REQUEST_CHANGES") + assert "request_changes_without_blocker" in check_review_result(packet, result)["errors"] + + +def test_required_red_check_needs_causal_attribution_and_unchanged_failure(): + packet, result, row = _required_red_review() + checked = check_review_result(packet, result) + assert "validation_matrix:repository_required_checks:failure_attribution_missing" in checked["approval_blockers"] + row["failure_attribution"] = { + "disposition": "pre_existing_unrelated", + "causal_scope_analysis": "The changed code does not feed this check.", + "affected_invariant_evidence": "Focused affected-path test passes.", + "base_revision": "b" * 40, + "head_revision": "a" * 40, + "same_command": "python examples/semantic-vocabulary-drift-smoke.py", + "baseline_observation": "Exit 1 with failure A.", + "head_observation": "Exit 1 with failure B.", + "baseline_failure_signature": "failure A", + "head_failure_signature": "failure B", + } + assert "validation_matrix:repository_required_checks:failure_signature_changed" in check_review_result(packet, result)["approval_blockers"] + row["failure_attribution"]["disposition"] = "pr_regression" + assert "validation_matrix:repository_required_checks:attributable_or_unresolved_failure" in check_review_result(packet, result)["approval_blockers"] + + +def test_equal_red_count_with_different_failure_identity_still_blocks_approval(): + packet, result, row = _required_red_review() + row["failure_attribution"] = { + "disposition": "pre_existing_unrelated", + "causal_scope_analysis": "The reviewed change does not edit the vocabulary scanner.", + "affected_invariant_evidence": "Focused changed-path tests pass.", + "base_revision": "b" * 40, + "head_revision": "a" * 40, + "same_command": "python examples/semantic-vocabulary-drift-smoke.py", + "baseline_observation": "Exit 1: maintained twin rule_a/rule_b, budget 44/43.", + "head_observation": "Exit 1: maintained twin rule_c/rule_d, budget 44/43.", + "baseline_failure_signature": "semantic-vocabulary-drift: rule_a/rule_b: maintained twin 44/43", + "head_failure_signature": "semantic-vocabulary-drift: rule_c/rule_d: maintained twin 44/43", + } + checked = check_review_result(packet, result) + assert not checked["approval_consistent"] + assert "validation_matrix:repository_required_checks:failure_signature_changed" in checked["approval_blockers"] + + +def test_malformed_required_validation_status_is_reported_not_raised(): + packet, result, row = _required_red_review() + row["status"] = {"unexpected": "object"} + assert "validation_matrix:repository_required_checks:invalid_status" in check_review_result(packet, result)["approval_blockers"] + + +def test_external_required_failure_is_review_only_when_independently_attributed(): + packet, result, row = _required_red_review() + row["failure_attribution"] = { + "disposition": "external_unrelated", + "causal_scope_analysis": "The provider outage predates this head; changed behavior has separate coverage.", + "affected_invariant_evidence": "Focused real-path test passes at this head.", + "independent_evidence": "Provider status incident and retry on unchanged base fail identically.", + "retry_or_recovery_owner": "CI operator retries after provider recovery; merge remains held.", + } + assert check_review_result(packet, result)["approval_consistent"] + del row["failure_attribution"]["independent_evidence"] + assert not check_review_result(packet, result)["approval_consistent"] + + def _outcome_review(dimension): packet, result = _review() impact = result["evidence"]["problem_context"]["outcome_impact"][dimension]