From 09fee473cca5925b5644b6f693be4ec6dd9ef1c4 Mon Sep 17 00:00:00 2001 From: song Date: Thu, 1 Oct 2026 09:42:07 -0400 Subject: [PATCH 1/8] feat(pr-review): declare the reviewer and bind the review to its specification A published review can be written by another operator's agent, working from a different host with no shared context or memory. Two things every such review needs were missing: who wrote it, and which specification it judged the change against. reviewer (result field plus one visible Reviewer: line) Declares actor_kind, declaration_source, and for a model_agent the model and provider. It is provenance, never review evidence: no reviewer can verify its own identity, so it lives beside review_body rather than inside evidence, and a missing or malformed declaration makes the result unpublishable instead of manufacturing a code finding against the author. Shape checks reject URL, path, host:port, account and vendor-qualified identifiers, with the known false positive documented. problem_context.spec_basis (sub-assessment) Maps each material acceptance criterion from the target repository's own accepted RFC or contract onto an exact-head symbol or path, with its disposition. Implemented rows name a symbol and a validation; deferred rows name the successor; not_met blocks approval. spec_ref and spec_revision pin the text so another operator opens the same revision, and a change that edits the specification it cites is judged against the pre-change text. A criterion must also appear in the published body, because the structured result stays local and the body is all another operator reads. check_review_body is deliberately untouched: it also recognizes reviews already published on GitHub, so adding a body requirement there would retroactively invalidate every historical review. The publication checks live in the pre-publication path instead. REVIEW_POLICY_REVISION 12 -> 13, which is what rejects a stale saved result. Signed-off-by: song --- .../pr_review_queue/result_check.py | 118 +++++++++++++++++- .../pr_review_queue/review_body.py | 15 +++ .../pr_review_queue/review_contract.py | 87 ++++++++++++- 3 files changed, 217 insertions(+), 3 deletions(-) diff --git a/loopx/capabilities/pr_review_queue/result_check.py b/loopx/capabilities/pr_review_queue/result_check.py index 77c1bbff86..fa9fb5ab0c 100644 --- a/loopx/capabilities/pr_review_queue/result_check.py +++ b/loopx/capabilities/pr_review_queue/result_check.py @@ -1,18 +1,25 @@ """Check a review's declared evidence for consistency, never its truth.""" +import re from collections.abc import Mapping from typing import Any from .review_contract import ( COMPATIBILITY_ASSESSMENT, OUTCOME_IMPACT_ASSESSMENT, + REVIEWER_DECLARATION, SCOPE_COVERAGE_ASSESSMENT, SEMANTIC_CANDIDATE_DECISIONS, + SPEC_BASIS_ASSESSMENT, VALIDATION_FAILURE_ATTRIBUTION, build_review_execution_contract, build_review_plan, ) -from .review_body import check_review_body +from .review_body import ( + check_review_body, + reviewer_declaration_lines, + visible_review_text, +) def _missing(value: object) -> bool: @@ -206,6 +213,107 @@ def _check_outcome_impact(blockers: list[str], value: object) -> None: fields=["acceptance_basis", "bounded_cost_and_recovery"]) +# The declaration names a product family, so the characters that mark a URL, +# path, host:port, account or ARN have no place in it. This is a shape rule, +# not a product-name denylist. Known false positive: router- or +# version-qualified ids such as `vendor/model` or `model@version` are rejected; +# declare the product-family name instead. A typed runtime-reported identity +# would replace this text check once a host supplies one. +_DECLARED_NAME_FORBIDDEN = re.compile(r"[/:@]") +_PUBLISHED_LINE_FORBIDDEN = re.compile(r"://|@") + + +def _reviewer_errors(value: object, body: str) -> list[str]: + key = "reviewer" + contract = REVIEWER_DECLARATION + if not isinstance(value, Mapping): + return [f"{key}:missing_declaration"] + actor_kind = value.get("actor_kind") + if actor_kind not in contract["actor_kinds"]: + return [f"{key}:invalid_actor_kind"] + errors: list[str] = [] + if value.get("declaration_source") not in contract["declaration_sources"]: + errors.append(f"{key}:invalid_declaration_source") + declared: list[str] = [str(actor_kind)] + if actor_kind == "model_agent": + for field in contract["model_agent_fields"]: + text = value.get(field) + if not isinstance(text, str) or not text.strip(): + errors.append(f"{key}:missing_field:{field}") + elif _DECLARED_NAME_FORBIDDEN.search(text): + errors.append(f"{key}:not_a_product_family_name:{field}") + else: + declared.append(text.strip()) + lines = reviewer_declaration_lines(body) + if len(lines) != 1: + errors.append(f"{key}:body_line_missing_or_ambiguous") + elif _PUBLISHED_LINE_FORBIDDEN.search(lines[0]): + errors.append(f"{key}:body_line_carries_runtime_detail") + elif any(token.casefold() not in lines[0].casefold() for token in declared): + errors.append(f"{key}:body_line_disagrees_with_declaration") + return errors + + +def _check_spec_basis(blockers: list[str], value: object) -> None: + key = "problem_context:spec_basis" + contract = SPEC_BASIS_ASSESSMENT + _require_fields(blockers, evidence_id=key, value=value, fields=contract["fields"]) + if not isinstance(value, Mapping): + return + decision = value.get("decision") + if decision not in contract["decision_values"]: + blockers.append(f"{key}:invalid_decision") + return + source = value.get("spec_source") + if source not in contract["spec_source_values"]: + blockers.append(f"{key}:invalid_spec_source") + if decision in contract["blocking_decisions"]: + blockers.append(f"{key}:blocking_decision") + if decision == "no_spec" and source != "none": + blockers.append(f"{key}:no_spec_cannot_cite_a_source") + if decision != "mapped": + return + if source == "none": + blockers.append(f"{key}:mapped_without_spec_source") + _require_fields(blockers, evidence_id=key, value=value, fields=contract["mapped_fields"]) + criteria = _require_items( + blockers, evidence_id=key, row=value, + requirement={"items_field": "criteria", "item_fields": contract["criterion_fields"], + "item_count": {"minimum": 1}}, + ) + seen: set[object] = set() + for criterion in criteria: + criterion_id = criterion.get("criterion_id") + if criterion_id in seen: + blockers.append(f"{key}:duplicate_criterion:{criterion_id}") + seen.add(criterion_id) + disposition = criterion.get("disposition") + if disposition not in contract["disposition_fields"]: + blockers.append(f"{key}:invalid_disposition:{criterion_id}") + continue + _require_fields(blockers, evidence_id=f"{key}:{criterion_id}", value=criterion, + fields=contract["disposition_fields"][disposition]) + if disposition in contract["blocking_dispositions"]: + blockers.append(f"{key}:unmet_criterion:{criterion_id}") + + +def _unpublished_spec_references(value: object, body: str) -> list[str]: + # The structured result stays local; another operator reads only the body. + if not isinstance(value, Mapping) or value.get("decision") != "mapped": + return [] + text = visible_review_text(body).casefold() + references = [value.get("spec_ref")] + criteria = value.get("criteria") + if isinstance(criteria, list): + references += [item.get("criterion_id") for item in criteria if isinstance(item, Mapping)] + return [ + f"review_body:spec_reference_not_published:{reference}" + for reference in references + if isinstance(reference, str) and reference.strip() + and reference.strip().casefold() not in text + ] + + def _check_scope_coverage(blockers: list[str], value: object) -> None: key = "observable_semantics:scope_coverage" contract = SCOPE_COVERAGE_ASSESSMENT @@ -302,6 +410,7 @@ def check_review_result( requirement = requirements[key] if key == "problem_context": _check_outcome_impact(blockers, row.get("outcome_impact")) + _check_spec_basis(blockers, row.get("spec_basis")) if key == "code_volume": _check_compatibility_assessment(blockers, row.get("compatibility_assessment")) if key == "observable_semantics": @@ -402,6 +511,13 @@ def check_review_result( head_oid=str(matches[0].get("head_oid") or ""), behavior_bearing=applicability.get("behavior_bearing_change") is True) errors.extend(f"review_body:{reason}" for reason in body["invalid_reasons"]) + body_text = str(result.get("review_body") or "") + errors.extend(_reviewer_errors(result.get("reviewer"), body_text)) + problem_context = evidence.get("problem_context") + errors.extend(_unpublished_spec_references( + problem_context.get("spec_basis") if isinstance(problem_context, Mapping) else None, + body_text, + )) if body["verdict"] is not None and body["verdict"] != verdict: errors.append("review_body:verdict_mismatch") if verdict not in {"APPROVE", "REQUEST_CHANGES"}: diff --git a/loopx/capabilities/pr_review_queue/review_body.py b/loopx/capabilities/pr_review_queue/review_body.py index afd67a51f0..7351b03ef6 100644 --- a/loopx/capabilities/pr_review_queue/review_body.py +++ b/loopx/capabilities/pr_review_queue/review_body.py @@ -67,6 +67,21 @@ def _visible_lines(body: str) -> Iterator[str]: yield visible +def visible_review_text(body: str) -> str: + return "\n".join(_visible_lines(body)) + + +def reviewer_declaration_lines(body: str) -> list[str]: + # Same visibility rule as the English verdict: a comment or fenced block + # cannot carry the declaration a reader is meant to see. + lines: list[str] = [] + for line in _visible_lines(body): + match = re.match(r"(?i)^reviewer\s*:\s*(.+)$", line.strip().replace("**", "")) + if match: + lines.append(match.group(1).strip()) + return lines + + def _prose_size(lines: list[str]) -> int: prose = "\n".join(dict.fromkeys(lines)) prose = re.sub(r"!?\[([^\]]*)\]\([^)]*\)", r"\1", prose) diff --git a/loopx/capabilities/pr_review_queue/review_contract.py b/loopx/capabilities/pr_review_queue/review_contract.py index f42414e7c3..ae14be3bc8 100644 --- a/loopx/capabilities/pr_review_queue/review_contract.py +++ b/loopx/capabilities/pr_review_queue/review_contract.py @@ -7,7 +7,7 @@ from .review_body import REQUIRED_FINAL_SECTIONS, review_body_requirements # Increment when review requirements change without changing the packet shape. -REVIEW_POLICY_REVISION = 12 +REVIEW_POLICY_REVISION = 13 # 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 @@ -137,6 +137,79 @@ ), } +# A published review speaks for an agent or a person, and a reader from another +# operator has no other way to weigh it. This is provenance carried by the +# result itself, not review evidence: no reviewer can "verify" its own identity, +# so a gap here makes the result unpublishable rather than blocking the author. +REVIEWER_DECLARATION = { + "actor_kinds": ["model_agent", "human_operator"], + "declaration_sources": ["runtime_reported", "self_reported"], + "fields": ["actor_kind", "declaration_source"], + "model_agent_fields": ["declared_model", "declared_provider"], + "body_marker": "Reviewer:", + "rule": ( + "Every result names who wrote it. actor_kind is model_agent or human_operator. " + "A model_agent gives declared_model and declared_provider in ordinary " + "product-family wording, and the published body repeats actor_kind, model and " + "provider on exactly one visible `Reviewer:` line, so a reader on another host " + "or organization can tell a model wrote the review and weigh it without private " + "context. declaration_source records whether the host runtime reported the " + "identity or the reviewer stated it; a reviewer that cannot observe its exact " + "build names the family it knows instead of inventing a version. The declaration " + "is provenance, not a credential: it authenticates nothing, authorizes no private " + "access and gives the verdict no extra weight. Never put an endpoint, gateway, " + "account, credential or router-qualified identifier in it." + ), +} + +# Reviewers from different operators share no context or memory; the accepted +# specification is the one reference both sides can open independently. Map the +# head onto it criterion by criterion instead of onto the author's narrative. +SPEC_BASIS_ASSESSMENT = { + "decision_values": ["mapped", "no_spec", "not_yet_proven"], + "blocking_decisions": ["not_yet_proven"], + "fields": ["decision", "spec_source", "reason"], + "spec_source_values": [ + "accepted_rfc", + "accepted_contract_doc", + "linked_issue_or_task", + "review_thread", + "none", + ], + "mapped_fields": ["spec_ref", "spec_revision", "criteria"], + "criterion_fields": ["criterion_id", "requirement", "disposition"], + "disposition_fields": { + "implemented": ["symbol_or_path", "validation_ref"], + "deferred": ["reason", "successor_or_gap"], + "out_of_scope": ["reason"], + "not_met": ["observed_gap", "minimum_repair"], + }, + "blocking_dispositions": ["not_met"], + "rule": ( + "Read the target repository's own accepted specification for the touched surface " + "before the implementation: an accepted RFC, an accepted contract or protocol " + "document, the linked issue or task, then a maintainer-agreed frame in the review " + "thread. For mapped, give spec_ref as a public path or link and spec_revision as " + "the immutable revision judged against, so a reviewer from another operator opens " + "the same text, and one criteria row per material acceptance criterion with the " + "specification's own identifier when it has one, the requirement as written and " + "its disposition at this exact head. implemented names a real symbol or path and " + "the validation that exercises it; deferred names the reason and the successor or " + "remaining gap; out_of_scope cites the specification or accepted task boundary, " + "not author preference; not_met names the observed gap and minimum repair and " + "blocks approval. Publish spec_ref and every criterion_id in the review body: " + "the body is the only part another operator reads. Derive requirements from the " + "specification, never from the patch. When the change edits the specification it " + "cites, judge the criteria as accepted before this change and treat the edit as a " + "finding to justify; a specification rewritten to match its implementation is not " + "an independent reference. Do not invent criteria, promote a future or aspirational " + "property to a current obligation, or impose this repository's roadmap on another " + "repository. With no written specification use no_spec, spec_source none and a " + "reason; problem_context remains the delivery judgment. An unread or unavailable " + "specification is not_yet_proven." + ), +} + CODE_AREAS = { "product_runtime", "app_or_ui_surface", @@ -210,7 +283,7 @@ def build_review_template(item: Mapping[str, Any]) -> dict[str, Any]: _section( "动机", floors["动机"], - "Use `problem_context`: verified goal basis, old behavior, before/after outcome and delivery verdict. Explain outcome_impact on sustained progress and the user journey, including accepted tradeoffs or scoped inapplicability. Distinguish completing the scoped goal from a justified increment; explain why this is a complete useful slice, not just why the code works.", + "Use `problem_context`: verified goal basis, old behavior, before/after outcome and delivery verdict. Cite `spec_basis.spec_ref` and map each criterion_id to implemented/deferred/out_of_scope/not_met at this head, or state that no written specification exists. Explain outcome_impact on sustained progress and the user journey, including accepted tradeoffs or scoped inapplicability. Distinguish completing the scoped goal from a justified increment; explain why this is a complete useful slice, not just why the code works.", ), _section( "改动思路", @@ -236,6 +309,8 @@ def build_review_template(item: Mapping[str, Any]) -> dict[str, Any]: "review_order": _review_order(key_files), "output_hint": ( "Render the verified structured result using the five sections. " + "Open the body with one `Reviewer:` line (reviewer_declaration.body_marker) " + "carrying result.reviewer's actor_kind, model and provider. " "The capability-owned review_execution_contract is the evidence and completeness authority. Save the exact final Markdown in result.review_body before check-result; publish that checked body and read it back. Section floors reject empty shells, not certify reasoning. Explain concrete paths and counterexamples; do not pad or duplicate evidence to meet a floor." ), } @@ -250,6 +325,10 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An "host skills route this contract but must not reimplement it." ), "evidence_status_values": ["verified", "unverified", "not_applicable"], + # Provenance and spec binding travel with every result, so a host skill + # routes them from here instead of inventing its own wording. + "reviewer_declaration": deepcopy(REVIEWER_DECLARATION), + "spec_basis_assessment": deepcopy(SPEC_BASIS_ASSESSMENT), "decision_procedure": { "order": [ "establish_goal", @@ -335,6 +414,7 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An "smaller_fix_analysis", "observable_outcome", "outcome_impact", + "spec_basis", "non_goals", ], "fields_by_verdict": { @@ -1276,6 +1356,9 @@ def build_review_plan(item: Mapping[str, Any]) -> dict[str, Any]: "findings": [], "residual_risk": "", "review_body": "", + "reviewer": {field: "" for field in ( + *REVIEWER_DECLARATION["fields"], *REVIEWER_DECLARATION["model_agent_fields"], + )}, "verdict": "unverified", }, } From 6d13484f16788091f892570dac39a61f07da6477 Mon Sep 17 00:00:00 2001 From: song Date: Thu, 1 Oct 2026 09:42:07 -0400 Subject: [PATCH 2/8] test(pr-review): cover the declaration, the spec mapping and publication Fixtures gain a declared reviewer and a no_spec basis, and the shared body fixture is prefixed rather than edited so the historical review text stays byte-identical for the independent fixture suites. New cases pin the rules that carry the design: a declaration hidden in a comment or a fenced block is not published, a body line that disagrees with the structured declaration is rejected, unknown or infrastructure-shaped values are rejected, a human operator needs no model, an unmet criterion blocks approval, a mapped basis cannot cite a source it did not read, and every criterion plus spec_ref must reach the body. Signed-off-by: song --- tests/capabilities/test_pr_review_contract.py | 14 ++ .../test_pr_review_result_check.py | 178 +++++++++++++++++- 2 files changed, 191 insertions(+), 1 deletion(-) diff --git a/tests/capabilities/test_pr_review_contract.py b/tests/capabilities/test_pr_review_contract.py index c97d87988e..1946487dd7 100644 --- a/tests/capabilities/test_pr_review_contract.py +++ b/tests/capabilities/test_pr_review_contract.py @@ -88,6 +88,20 @@ def test_execution_contract_owns_deep_review_requirements() -> None: "minimum": 2, "maximum": 5, } + # Every review is bound to the specification it judged, inside the goal + # judgment that owns it, rather than to its own narrative. + assert "spec_basis" in requirements["problem_context"]["fields"] + assert "not_met" in contract["spec_basis_assessment"]["blocking_dispositions"] + # Provenance is carried by the result, never scored as review evidence. + assert "reviewer" not in requirements + assert contract["reviewer_declaration"]["actor_kinds"] == [ + "model_agent", + "human_operator", + ] + assert contract["reviewer_declaration"]["model_agent_fields"] == [ + "declared_model", + "declared_provider", + ] assert "caller_evidence" in requirements["symbol_map"]["item_fields"] assert "negative_fields" in requirements["walkthroughs"] assert "regression_test" in requirements["failure_analysis"]["fields"] diff --git a/tests/capabilities/test_pr_review_result_check.py b/tests/capabilities/test_pr_review_result_check.py index 92960fd2cf..8e99ea52f4 100644 --- a/tests/capabilities/test_pr_review_result_check.py +++ b/tests/capabilities/test_pr_review_result_check.py @@ -40,6 +40,11 @@ def _review(*, area="product_runtime"): if "verdict_values" in requirement: row["verdict"] = requirement["verdict_values"][0] if key == "problem_context": + row["spec_basis"] = { + "decision": "no_spec", + "spec_source": "none", + "reason": "Synthetic formatter fixture has no written specification.", + } row["outcome_impact"] = { dimension: {"decision": "not_applicable", "reason": "Synthetic internal formatter fixture has no durable work or user journey.", @@ -90,10 +95,56 @@ def _review(*, area="product_runtime"): for field in fields } result["verdict"] = "APPROVE" - result["review_body"] = (Path(__file__).parents[2] / "examples/fixtures/pr-review.body.md").read_text().replace("HEAD_OID", "a" * 40).replace("VERDICT", "APPROVE") + result["reviewer"] = { + "actor_kind": "model_agent", + "declaration_source": "self_reported", + "declared_model": "Example Model 1", + "declared_provider": "Example Provider", + } + body = (Path(__file__).parents[2] / "examples/fixtures/pr-review.body.md").read_text() + result["review_body"] = ( + f"{REVIEWER_LINE}\n\n" + + body.replace("HEAD_OID", "a" * 40).replace("VERDICT", "APPROVE") + ) return {"pull_requests": [item]}, result +REVIEWER_LINE = "Reviewer: model_agent · Example Model 1 · Example Provider" + + +def _mapped_spec_basis(): + return { + "decision": "mapped", + "spec_source": "accepted_rfc", + "reason": "The change implements an accepted RFC section.", + "spec_ref": "docs/architecture/rfcs/example-v0.md#acceptance", + "spec_revision": "b" * 40, + "criteria": [ + { + "criterion_id": "EX-1", + "requirement": "Readback reports the persisted value.", + "disposition": "implemented", + "symbol_or_path": "loopx/example.py::read_back", + "validation_ref": "tests/test_example.py::test_read_back", + }, + { + "criterion_id": "EX-2", + "requirement": "Migration of old rows.", + "disposition": "deferred", + "reason": "Old rows are retired by the successor task.", + "successor_or_gap": "The linked migration task.", + }, + ], + } + + +def _publish_spec_references(result): + result["review_body"] += ( + "\n\nSpec basis: docs/architecture/rfcs/example-v0.md#acceptance" + " — EX-1 implemented; EX-2 deferred.\n" + ) + + def test_result_check_is_not_semantic_or_merge_authority(): packet, result = _review() checked = check_review_result(packet, result) @@ -471,6 +522,131 @@ def test_nonblocking_suggestion_does_not_force_rejection(): assert check_review_result(packet, result)["approval_consistent"] +def test_review_must_say_which_model_wrote_it_in_the_published_body(): + packet, result = _review() + assert check_review_result(packet, result)["ok"] + result["review_body"] = result["review_body"].replace(REVIEWER_LINE + "\n\n", "") + assert "reviewer:body_line_missing_or_ambiguous" in check_review_result(packet, result)["errors"] + # A declaration hidden from readers is not a declaration. + result["review_body"] = f"\n\n" + result["review_body"] + assert "reviewer:body_line_missing_or_ambiguous" in check_review_result(packet, result)["errors"] + + +def test_published_reviewer_line_must_match_the_declaration(): + packet, result = _review() + result["reviewer"]["declared_model"] = "Another Model 2" + checked = check_review_result(packet, result) + assert not checked["ok"] + assert "reviewer:body_line_disagrees_with_declaration" in checked["errors"] + + +@pytest.mark.parametrize("field, value, reason", [ + ("actor_kind", "agent", "reviewer:invalid_actor_kind"), + ("declaration_source", "guessed", "reviewer:invalid_declaration_source"), + ("declared_model", "", "reviewer:missing_field:declared_model"), + ("declared_provider", " ", "reviewer:missing_field:declared_provider"), + ("declared_model", "https://gateway.example/v1", "reviewer:not_a_product_family_name:declared_model"), + ("declared_provider", "ops@example", "reviewer:not_a_product_family_name:declared_provider"), +]) +def test_reviewer_declaration_rejects_unknown_or_infrastructure_values(field, value, reason): + packet, result = _review() + result["reviewer"][field] = value + assert reason in check_review_result(packet, result)["errors"] + + +def test_missing_reviewer_declaration_blocks_publication_of_any_verdict(): + packet, result = _review() + del result["reviewer"] + result["verdict"] = "REQUEST_CHANGES" + result["review_body"] = result["review_body"].replace( + "English verdict: APPROVE", "English verdict: REQUEST_CHANGES") + checked = check_review_result(packet, result) + assert "reviewer:missing_declaration" in checked["errors"] + assert not checked["ok"] + + +def test_human_operator_needs_no_model_or_provider(): + packet, result = _review() + result["reviewer"] = {"actor_kind": "human_operator", "declaration_source": "self_reported"} + result["review_body"] = result["review_body"].replace(REVIEWER_LINE, "Reviewer: human_operator") + assert check_review_result(packet, result)["ok"] + + +def test_mapped_spec_basis_binds_criteria_to_the_head_and_the_published_body(): + packet, result = _review() + result["evidence"]["problem_context"]["spec_basis"] = _mapped_spec_basis() + unpublished = check_review_result(packet, result) + assert not unpublished["ok"] + assert "review_body:spec_reference_not_published:EX-1" in unpublished["errors"] + assert ("review_body:spec_reference_not_published:docs/architecture/rfcs/example-v0.md#acceptance" + in unpublished["errors"]) + _publish_spec_references(result) + checked = check_review_result(packet, result) + assert checked["ok"] and checked["approval_consistent"] + + +def test_unmet_specification_criterion_cannot_be_approved(): + packet, result = _review() + basis = _mapped_spec_basis() + basis["criteria"][1] = { + "criterion_id": "EX-2", + "requirement": "Migration of old rows.", + "disposition": "not_met", + "observed_gap": "Old rows keep the previous shape.", + "minimum_repair": "Migrate old rows before readback.", + } + result["evidence"]["problem_context"]["spec_basis"] = basis + _publish_spec_references(result) + checked = check_review_result(packet, result) + assert "problem_context:spec_basis:unmet_criterion:EX-2" in checked["approval_blockers"] + assert "approval_contradicts_evidence" in checked["errors"] + result["verdict"] = "REQUEST_CHANGES" + result["review_body"] = result["review_body"].replace( + "English verdict: APPROVE", "English verdict: REQUEST_CHANGES") + assert check_review_result(packet, result)["ok"] + + +@pytest.mark.parametrize("mutation, blocker", [ + ("unknown_disposition", "problem_context:spec_basis:invalid_disposition:EX-1"), + ("implemented_without_symbol", "problem_context:spec_basis:EX-1:missing_field:symbol_or_path"), + ("deferred_without_successor", "problem_context:spec_basis:EX-2:missing_field:successor_or_gap"), + ("duplicate_criterion", "problem_context:spec_basis:duplicate_criterion:EX-1"), + ("no_revision", "problem_context:spec_basis:missing_field:spec_revision"), + ("mapped_without_source", "problem_context:spec_basis:mapped_without_spec_source"), + ("unread_spec", "problem_context:spec_basis:blocking_decision"), + ("no_spec_with_source", "problem_context:spec_basis:no_spec_cannot_cite_a_source"), + ("missing", "problem_context:spec_basis:value_not_object"), +]) +def test_spec_basis_cannot_approve_with_incomplete_or_contradictory_mapping(mutation, blocker): + packet, result = _review() + basis = _mapped_spec_basis() + if mutation == "unknown_disposition": + basis["criteria"][0]["disposition"] = "mostly_done" + elif mutation == "implemented_without_symbol": + del basis["criteria"][0]["symbol_or_path"] + elif mutation == "deferred_without_successor": + del basis["criteria"][1]["successor_or_gap"] + elif mutation == "duplicate_criterion": + basis["criteria"][1]["criterion_id"] = "EX-1" + elif mutation == "no_revision": + del basis["spec_revision"] + elif mutation == "mapped_without_source": + basis["spec_source"] = "none" + elif mutation == "unread_spec": + basis = {"decision": "not_yet_proven", "spec_source": "accepted_rfc", + "reason": "The cited RFC revision could not be opened."} + elif mutation == "no_spec_with_source": + basis = {"decision": "no_spec", "spec_source": "accepted_rfc", "reason": "x"} + if mutation == "missing": + del result["evidence"]["problem_context"]["spec_basis"] + else: + result["evidence"]["problem_context"]["spec_basis"] = basis + _publish_spec_references(result) + checked = check_review_result(packet, result) + assert blocker in checked["approval_blockers"] + assert not checked["ok"] + + @pytest.mark.parametrize("revision", [None, 0, True, "1", 5, 999]) def test_old_or_invalid_policy_cannot_certify_current_approval(revision): packet, result = _review() From 3fedbe3773a90e9f6c7b05cab9b07609d87660db Mon Sep 17 00:00:00 2001 From: song Date: Thu, 1 Oct 2026 09:42:07 -0400 Subject: [PATCH 3/8] docs: publish the declaration and the spec basis on the author and review surfaces - pr-review SKILL: fill result.reviewer and open the body with reviewer_declaration.body_marker; read problem_context.spec_basis's specification before the implementation and satisfy it criterion by criterion. Compressed to stay inside the skill's 180-line contract rather than raising it; every smoke-locked phrase is preserved. - PR template: an Author Declaration section naming who wrote the change and the specification plus revision it was implemented against, one row per criterion. It is published attribution, so it stays short and carries no runtime detail. Signed-off-by: song --- .github/PULL_REQUEST_TEMPLATE.md | 36 ++++++++++++++++++ skills/loopx-pr-review/SKILL.md | 64 ++++++++++++++++---------------- 2 files changed, 68 insertions(+), 32 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index d3d233d8d9..67e20e10c1 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -14,6 +14,42 @@ concrete gap and consumer. These author facts are independently reviewed. - Observable before → after, with the validation row that proves it: - Issue/task and intended base: +## Author Declaration + + + +- Written by: + +### Implemented against + + + +- Specification and revision: +- Criteria: + +| Criterion (spec clause) | Disposition | Symbol / path | Test or command | +| --- | --- | --- | --- | +| | | | | + +- Self-check before submission: + ## Scope And Continuation