Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions examples/pr-review-command-smoke.py
Original file line number Diff line number Diff line change
Expand Up @@ -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", [])
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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"],
Expand Down
20 changes: 20 additions & 0 deletions loopx/capabilities/pr_review_queue/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion loopx/capabilities/pr_review_queue/catalog_entry.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down
52 changes: 51 additions & 1 deletion loopx/capabilities/pr_review_queue/review_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
"具体改动",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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 "
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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,
Expand Down
19 changes: 5 additions & 14 deletions skills/loopx-pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
76 changes: 76 additions & 0 deletions tests/capabilities/test_pr_review_contract.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import annotations

import pytest

from loopx.capabilities.pr_review_queue import (
build_agent_response_contract,
build_review_plan,
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -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"]
Loading