diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index ac0ab4b70a..0c0285067d 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -267,7 +267,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: if req.get("evidence_id") == "scope_fit" ] assert scope_fit_reqs, "scope_fit evidence requirement missing from contract" - assert scope_fit_reqs[0]["required_when"] == "code_change", scope_fit_reqs + assert scope_fit_reqs[0]["required_when"] == "behavior_bearing_change", scope_fit_reqs proportionality_reqs = [ req for req in contract.get("evidence_requirements", []) @@ -288,7 +288,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: if req.get("evidence_id") == "default_off_isolation" ] assert isolation_reqs, "default_off_isolation evidence requirement missing" - assert isolation_reqs[0]["required_when"] == "code_change" + assert isolation_reqs[0]["required_when"] == "behavior_bearing_change" assert "paired_counterfactual_validation" in isolation_reqs[0]["fields"] authority_reqs = [ req @@ -321,6 +321,9 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: assert code_pr["review_plan"]["applicability"]["scope_fit_required"] is True, ( code_pr["review_plan"] ) + reuse = code_pr["review_plan"]["result_template"]["evidence"]["repository_reuse"] + assert reuse == {"status": "unverified"}, reuse + assert code_pr["review_plan"]["applicability"]["repository_reuse_required"] is True assert ( code_pr["review_plan"]["applicability"]["change_proportionality_required"] is True @@ -343,6 +346,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: ) assert docs_pr is not None, "fixture must include a docs-only PR" assert docs_pr["review_plan"]["applicability"]["scope_fit_required"] is False + assert "repository_reuse" not in docs_pr["review_plan"]["required_evidence_ids"] assert ( docs_pr["review_plan"]["applicability"]["change_proportionality_required"] is False @@ -686,6 +690,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: assert set(requirements) == { "problem_context", "architecture_flow", + "repository_reuse", "changed_line_classification", "scope_fit", "symbol_map", @@ -752,6 +757,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: assert execution["completion_gate"]["metadata_only_verdict_allowed"] is False assert execution["completion_gate"]["stale_head_verdict_allowed"] is False assert execution["completion_gate"]["blocking_evidence_verdicts"] == { + "repository_reuse": ["unjustified_duplication", "not_yet_proven"], "change_proportionality": ["disproportionate", "not_yet_proven"], "default_off_isolation": ["not_isolated", "not_yet_proven"], "authority_semantics": ["misleading", "not_yet_proven"], diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index b9e3ae987d..5a71ae626e 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -178,6 +178,14 @@ typed evidence groups before a verdict: - problem context and active caller; - architecture and ownership flow; +- repository reuse for behavior-bearing changes: search base and exact-head + code, including unchanged siblings, by caller outcome/resource rather than + only new filenames. Record revisions, queries, paths and candidate callers; + compare scope/filters, ordering/paging, authority/sanitization and state/retry + ownership. Prefer the nearest existing owner; justify independent boundaries + with evidence, not green CI or conflict-free coexistence. For alternative + views of one resource, validate consumer switching and concurrent updates + where relevant. Repeat the comparison after base integration or head changes; - exact changed-line classification across production, tests/fixtures, docs, generated output, and mechanical moves; - a 2-5 item exact-head symbol map for code-changing PRs, including caller, @@ -221,6 +229,18 @@ hints, and green CI cannot upgrade evidence to `verified`. A stale-head verdict is prohibited. Missing evidence remains `unverified` with a reason instead of being replaced by confident prose. +`repository_reuse` starts unverified in every applicable plan. Its conclusions +are `reused`, `separation_justified`, `no_existing_candidate`, +`unjustified_duplication`, or `not_yet_proven`. A negative search must name its +scope and limitations; an empty candidate list is not proof of absence. +Unjustified duplication or missing evidence requires a request-changes +conclusion. Similar-looking code with distinct invariants or compatibility +needs may legitimately remain separate. Ordinary docs retain their existing +review path; smoke-only changes retain `durable_smoke_value` coverage review. +This is a reviewer-executed contract projected by the packet, not an automatic +repository search or a semantic validator of published prose. Tests establish +packet applicability and verdict policy, not guaranteed model compliance. + When `--state all` is used, the command must preserve both lifecycle groups. The `--limit` value is applied per group so a busy open queue cannot consume the whole packet and make `review_groups.merged` empty while merged PRs exist in the diff --git a/loopx/capabilities/pr_review_queue/catalog_entry.py b/loopx/capabilities/pr_review_queue/catalog_entry.py index 718890481b..afa317cd78 100644 --- a/loopx/capabilities/pr_review_queue/catalog_entry.py +++ b/loopx/capabilities/pr_review_queue/catalog_entry.py @@ -110,7 +110,7 @@ ], "docs": ["loopx/capabilities/pr_review_queue/README.md"], "boundaries": [ - "The shared execution contract owns review depth, evidence completeness, exact-head freshness, symbol-map, walkthrough, validation, failure, code-volume, change-proportionality, default-off isolation, and authority-semantics requirements; host skills only route and publish it.", + "The shared execution contract owns review depth, evidence completeness, repository-reuse comparison, exact-head freshness, symbol-map, walkthrough, validation, failure, code-volume, change-proportionality, default-off isolation, and authority-semantics requirements; host skills only route and publish it.", "A queue is observed only when result_completeness.complete=true; partial or failed reads are not_observed and never count as unchanged.", "Fingerprints cover exact head, formal conclusion, next action, check state, draft state, and mergeability for every open PR.", "Current-head review_ready_at, not updatedAt, owns age-fair ordering; one new head after REQUEST_CHANGES may use a bounded fast-feedback slot.", diff --git a/loopx/capabilities/pr_review_queue/review_contract.py b/loopx/capabilities/pr_review_queue/review_contract.py index 1af7cdc6d8..f49d35226a 100644 --- a/loopx/capabilities/pr_review_queue/review_contract.py +++ b/loopx/capabilities/pr_review_queue/review_contract.py @@ -76,7 +76,7 @@ def build_review_template(item: Mapping[str, Any]) -> dict[str, Any]: _section( "改动思路", "300-500字", - "Use `architecture_flow` and `walkthroughs`: entry point, authoritative state, decision boundary, positive path, alternative, and ownership trade-off.", + "Use `architecture_flow`, `repository_reuse`, and `walkthroughs`: entry point, authoritative state, decision boundary, positive path, existing implementation comparison, and ownership trade-off.", ), _section( "具体改动", @@ -137,6 +137,48 @@ def build_review_execution_contract() -> dict[str, Any]: "failure_or_retry_owner", ], }, + { + "evidence_id": "repository_reuse", + "required_when": "behavior_bearing_change", + "verdict_values": [ + "reused", "separation_justified", "no_existing_candidate", + "unjustified_duplication", "not_yet_proven", + ], + "fields": [ + "searched_revisions", "queries_and_paths", "existing_candidates", + "semantic_comparison", "reuse_or_separation_reason", + "validation_evidence", "verdict", + ], + "comparison_dimensions": [ + "resource_and_caller", "data_scope_and_filters", + "ordering_and_pagination", "authority_and_sanitization", + "state_retry_and_failure_owner", + ], + "rule": ( + "Before judging the proposed architecture, search the base and " + "exact-head repository by caller outcome, resource, routes and " + "symbols, not only new filenames. Record immutable searched " + "revisions, queries, paths and candidate definitions/callers, " + "including unchanged siblings outside the diff. Compare the " + "applicable dimensions for each candidate and explain why the " + "nearest existing owner can be reused or extended, or why an " + "independent boundary is necessary. Same-resource views must " + "retain the same semantics unless a deliberate difference is " + "disclosed and validated; exercise switching consumers and " + "concurrent updates when state or paging is involved. Green CI, " + "shared low-level readers, different names, and conflict-free " + "coexistence do not justify a second contract or state owner. " + "A no_existing_candidate verdict requires a bounded negative " + "search with its scope and limitations, not an empty candidate " + "list. Similar code alone is not duplication: preserve distinct " + "invariants or compatibility boundaries when evidence justifies " + "separation. After base integration or head changes, repeat the " + "comparison rather than inheriting an earlier approval. Missing " + "or unverified evidence is not_yet_proven and cannot approve. " + "This is a reviewer-executed evidence requirement, not an " + "automatic similarity detector or proof that a search ran." + ), + }, { "evidence_id": "changed_line_classification", "required_when": "always", @@ -507,6 +549,7 @@ def build_review_execution_contract() -> dict[str, Any]: "exact_head_recheck_required": True, "stale_head_verdict_allowed": False, "blocking_evidence_verdicts": { + "repository_reuse": ["unjustified_duplication", "not_yet_proven"], "change_proportionality": [ "disproportionate", "not_yet_proven", @@ -524,6 +567,11 @@ def build_review_execution_contract() -> dict[str, Any]: }, "verdict_policy": { "open_pr_blocking_finding": "REQUEST_CHANGES", + "open_pr_unresolved_reuse": ( + "REQUEST_CHANGES when repository_reuse is unjustified_duplication " + "or not_yet_proven, including missing/unverified search evidence; " + "request reuse, an evidence-backed separation, or further investigation" + ), "open_pr_unresolved_proportionality": ( "REQUEST_CHANGES when change_proportionality is disproportionate " "or not_yet_proven; correctness, green CI, and resolved earlier " @@ -580,6 +628,7 @@ def build_review_plan(item: Mapping[str, Any]) -> dict[str, Any]: required_evidence.append("authority_semantics") required_evidence.append("typed_state_rule") if behavior_bearing_change: + required_evidence.insert(3, "repository_reuse") if "scope_fit" not in required_evidence: required_evidence.append("scope_fit") required_evidence.append("default_off_isolation") @@ -609,6 +658,7 @@ def build_review_plan(item: Mapping[str, Any]) -> dict[str, Any]: "docs_only": docs_only, "symbol_map_required": code_change, "scope_fit_required": behavior_bearing_change, + "repository_reuse_required": behavior_bearing_change, "change_proportionality_required": code_change, "default_off_isolation_required": behavior_bearing_change, "authority_semantics_required": code_change, diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index c6de7044b6..6db38fd7d9 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -125,20 +125,11 @@ artifacts back. ## Full PR Interpretation Depth -A complete review is a whole-PR interpretation, not a checklist or findings -summary. For each selected PR: - -1. Read every changed file and map each file to its responsibility, inputs, - outputs, and key symbols. -2. Pick 2-5 behavior-bearing symbols and explain before/after behavior, - critical branches, callers/callees, side effects, and failure paths. -3. Walk one positive path from user/host action to observable result. -4. Walk one negative path (invalid input, permission, timeout, corrupt state, - private boundary, or rollback) and show where it fails closed. -5. Cover all changed surfaces in the five sections: motivation, approach, - concrete changes, main risk, overall judgment. -6. List validation per surface and name anything not independently verified. -7. State overall judgment for the entire PR, not only for the top finding. +Use the packet's `repository_reuse`, `symbol_map`, `walkthroughs`, and +`validation_matrix` evidence across the whole PR, including unchanged callers +and sibling implementations. Walk one positive path to the observable result. +Walk one negative path and explain its failure owner. Render the verified +evidence in the five sections; do not maintain a second checklist here. A review that only repeats the PR body, only discusses one blocker, or omits whole files/modules is incomplete and must be reworked. diff --git a/tests/capabilities/test_pr_review_contract.py b/tests/capabilities/test_pr_review_contract.py index 1b96e0f0f0..e3897526cb 100644 --- a/tests/capabilities/test_pr_review_contract.py +++ b/tests/capabilities/test_pr_review_contract.py @@ -1,5 +1,7 @@ from __future__ import annotations +import pytest + from loopx.capabilities.pr_review_queue import ( build_agent_response_contract, build_review_plan, @@ -40,6 +42,7 @@ def test_execution_contract_owns_deep_review_requirements() -> None: assert set(requirements) == { "problem_context", "architecture_flow", + "repository_reuse", "changed_line_classification", "scope_fit", "symbol_map", @@ -117,6 +120,7 @@ def test_execution_contract_owns_deep_review_requirements() -> None: assert contract["completion_gate"]["metadata_only_verdict_allowed"] is False assert contract["completion_gate"]["stale_head_verdict_allowed"] is False assert contract["completion_gate"]["blocking_evidence_verdicts"] == { + "repository_reuse": ["unjustified_duplication", "not_yet_proven"], "change_proportionality": ["disproportionate", "not_yet_proven"], "default_off_isolation": ["not_isolated", "not_yet_proven"], "authority_semantics": ["misleading", "not_yet_proven"], @@ -265,3 +269,75 @@ def test_test_only_plan_skips_runtime_lenses() -> None: assert plan["applicability"]["guidance_vs_obligation_required"] is False assert "typed_state_rule" not in plan["required_evidence_ids"] assert "domain_neutrality" not in plan["required_evidence_ids"] + + +@pytest.mark.parametrize( + "area", + [ + "product_runtime", + "app_or_ui_surface", + "ci_or_release", + "build_or_config", + "agent_instruction_surface", + "public_entry_or_policy", + ], +) +def test_behavior_review_requires_repository_reuse_even_with_green_checks( + area: str, +) -> None: + item = _item(areas={area: 1, "test_or_example": 1}) + # A narrow changed-file list and passing CI cannot establish that an + # unchanged sibling already implements the same caller outcome. + item["key_files"] = [{"path": "src/history_list.py", "additions": 80}] + item["checks"] = {"counts": {"success": 4, "failure": 0}} + plan = build_review_plan(item) + assert plan["applicability"]["repository_reuse_required"] is True + assert "repository_reuse" in plan["required_evidence_ids"] + assert plan["result_template"]["evidence"]["repository_reuse"] == { + "status": "unverified" + } + + +@pytest.mark.parametrize("area", ["public_docs", "test_or_example"]) +def test_non_behavior_review_keeps_existing_coverage_policy(area: str) -> None: + plan = build_review_plan(_item(areas={area: 1})) + assert plan["applicability"]["repository_reuse_required"] is False + assert "repository_reuse" not in plan["required_evidence_ids"] + + +def test_reuse_evidence_compares_semantics_beyond_the_diff() -> None: + contract = build_agent_response_contract()["review_execution_contract"] + reuse = next( + row + for row in contract["evidence_requirements"] + if row["evidence_id"] == "repository_reuse" + ) + assert reuse["required_when"] == "behavior_bearing_change" + assert reuse["verdict_values"] == [ + "reused", + "separation_justified", + "no_existing_candidate", + "unjustified_duplication", + "not_yet_proven", + ] + assert { + "searched_revisions", + "queries_and_paths", + "existing_candidates", + "semantic_comparison", + "reuse_or_separation_reason", + "validation_evidence", + "verdict", + } <= set(reuse["fields"]) + assert { + "resource_and_caller", + "data_scope_and_filters", + "ordering_and_pagination", + "authority_and_sanitization", + "state_retry_and_failure_owner", + } <= set(reuse["comparison_dimensions"]) + assert "unchanged" in reuse["rule"] + assert "negative search" in reuse["rule"] + assert "coexistence" in reuse["rule"] + assert "not an automatic similarity detector" in reuse["rule"] + assert "repository_reuse" in contract["verdict_policy"]["open_pr_unresolved_reuse"]