From 93c09d5664a60cd91dd6a84ba57fad9fd754f8e1 Mon Sep 17 00:00:00 2001 From: huangruiteng Date: Mon, 7 Sep 2026 00:41:46 +0800 Subject: [PATCH] fix(review): require caller-observable semantic parity evidence Signed-off-by: huangruiteng --- docs/development/testing-and-quality.md | 29 +++++++ examples/control_plane/todo-cli-smoke.py | 9 ++- .../pr_review_queue/review_contract.py | 73 ++++++++++++++++- tests/capabilities/test_pr_review_contract.py | 80 +++++++++++++++++++ .../test_local_coordination_authority.py | 18 ++++- 5 files changed, 205 insertions(+), 4 deletions(-) diff --git a/docs/development/testing-and-quality.md b/docs/development/testing-and-quality.md index 109229166d..e3c6838696 100644 --- a/docs/development/testing-and-quality.md +++ b/docs/development/testing-and-quality.md @@ -75,6 +75,35 @@ golden 来让测试通过。 ### Refactor Real-Path Gate / 重构真实路径门 +The PR review capability's `observable_semantics` evidence gate applies to +behavior-bearing changes, including extractions and backend migrations; it +does not infer equivalence from a refactor title. Reviewers inventory legacy +caller branches at an immutable baseline and compare the same synthetic +inputs at the exact head through the public entrypoint. Include successful +and rejected paths, full diagnostics/remediation, validation precedence, +supplied/omitted/empty/clear arguments, persisted readback, ownership and +receipts, plus replay/concurrency where relevant. A provider suite whose +implementations share the new rule is not a before/after compatibility test. + +重构评审必须区分“相同判定”和“相同可观察语义”：同样拒绝但丢失对象状态或修复 +提示仍是回归；新增认领前提可能阻断旧实现允许的未认领文案修正；`--note` 被 parser +接受或出现在成功响应中，也不能证明它经过 dispatch 后落盘并能独立回读。先从旧调用 +方及公开契约列出合法／非法分支，再运行基线与精确 head 的对照，不从新实现生成期望。 + +Show regression sensitivity: the focused case must fail on the historical +defect or a deliberate dropped-field/detail or stronger-precondition mutation, +then pass on the fix. Preserve intended changes as explicit justified deltas; +do not freeze a known baseline bug or normalize away meaningful differences. +Re-review the full inventory after a correction, not only the previous finding. +Missing comparison evidence blocks an equivalence-based approval. This is a +reviewer-executed requirement projected in the packet, not a machine proof +that the comparison ran. The packet tests protect this requirement's delivery; +runtime regressions must still test actual behavior. + +保留“旧缺陷／语义 mutation 失败、修复后通过”的证据；有意变更需单列理由与验证， +不能盲目追求字节一致。复审重查完整兼容清单，不沿着上一条发现自动走向批准。缺少 +对照证据就不能宣称等价；packet 测试只证明要求被投影，不证明 agent 已执行或产品无缺陷。 + Refactors must exercise the affected production entrypoint and real backend before delivery. Unit tests, mocks, and in-memory conformance remain useful, but cannot replace that proof. For changes affecting PostgreSQL authority, diff --git a/examples/control_plane/todo-cli-smoke.py b/examples/control_plane/todo-cli-smoke.py index b1d5c37145..b520f1a4b3 100644 --- a/examples/control_plane/todo-cli-smoke.py +++ b/examples/control_plane/todo-cli-smoke.py @@ -87,7 +87,7 @@ def main() -> int: root = Path(tmp) registry_path, state_file = write_fixture(root) legacy_registry_path, _ = write_fixture(root / "legacy", register_agents=False) - claim_state_registry_path, _ = write_fixture(root / "claim-state") + claim_state_registry_path, claim_state_file = write_fixture(root / "claim-state") original = state_file.read_text(encoding="utf-8") blocked_todo = run_cli( @@ -105,6 +105,7 @@ def main() -> int: "--task-class", "advancement_task", ) + before_rejected_claim = claim_state_file.read_text(encoding="utf-8") blocked_claim = run_cli_error( claim_state_registry_path, "todo", @@ -118,7 +119,11 @@ def main() -> int: "--agent-id", "codex-main-control", ) - assert "todo claim requires status=open" in blocked_claim["error"], blocked_claim + assert blocked_claim["error"] == ( + f"todo claim requires status=open; todo_id '{blocked_todo['todo_id']}' " + "is status='blocked'" + ), blocked_claim + assert claim_state_file.read_text(encoding="utf-8") == before_rejected_claim bare_user_error = run_cli_error( registry_path, diff --git a/loopx/capabilities/pr_review_queue/review_contract.py b/loopx/capabilities/pr_review_queue/review_contract.py index f49d35226a..307164a860 100644 --- a/loopx/capabilities/pr_review_queue/review_contract.py +++ b/loopx/capabilities/pr_review_queue/review_contract.py @@ -91,7 +91,7 @@ def build_review_template(item: Mapping[str, Any]) -> dict[str, Any]: _section( "我的整体评价", "150-300字", - "Use `code_volume`, `change_proportionality`, `default_off_isolation`, `authority_semantics`, validation results, residual risk, and exact-head freshness to state the verdict and the evidence needed for re-review.", + "Use `observable_semantics` to report baseline/head comparisons and remaining compatibility gaps; equal decision codes are insufficient. Use `code_volume`, `change_proportionality`, `default_off_isolation`, `authority_semantics`, validation results, residual risk, and exact-head freshness to state the verdict and the evidence needed for re-review.", ), ], "review_order": _review_order(key_files), @@ -179,6 +179,69 @@ def build_review_execution_contract() -> dict[str, Any]: "automatic similarity detector or proof that a search ran." ), }, + { + "evidence_id": "observable_semantics", + "required_when": "behavior_bearing_change", + "verdict_values": [ + "equivalent", + "intentional_change_validated", + "unintended_drift", + "not_yet_proven", + ], + "fields": [ + "baseline_revision", + "reviewed_head", + "caller_branch_inventory", + "comparison_rows", + "intentional_deltas", + "regression_sensitivity", + "unverified_dimensions", + "verdict", + ], + "comparison_dimensions": [ + "accepted_inputs_and_defaults", + "eligibility_and_rejection_precedence", + "full_diagnostics_and_remediation", + "argument_to_persistence_readback", + "state_receipts_and_no_effects", + "replay_and_concurrent_updates", + ], + "row_fields": [ + "input_and_pre_state", + "entrypoint_and_backend", + "baseline_observation", + "head_observation", + "expected_invariant_source", + "validation_evidence", + ], + "rule": ( + "Inventory changed and bypassed caller branches from the immutable " + "pre-change baseline, not just the new helper or PR title. Compare " + "identical synthetic inputs through the real public entrypoint and " + "affected backend at baseline and exact head. Include legal paths " + "as well as rejection paths: stricter eligibility is not inherently " + "compatible. For migrations compare before/after promotion, not " + "only multiple providers sharing the new decision. Trace supplied, " + "omitted, empty and clear argument intent through dispatch, mutation, " + "persistence and a separate readback; a success payload or parser " + "acceptance does not prove a note was saved or ownership unchanged. " + "Compare exception/exit type, full diagnostic details and remediation, " + "not just reason codes, prefixes or 'both reject'. Exercise overlapping " + "invalid conditions to check rejection precedence and assert no " + "unintended state/receipt/ownership effects. Mark non-applicable " + "dimensions with reasons. Derive expected invariants from the public " + "contract and legacy callers, never from the replacement output; " + "baseline bugs require explicitly disclosed, justified changes rather " + "than blind byte parity. Show a regression failing on the old defect " + "or a deliberate semantic mutation (dropped argument/detail, stronger " + "precondition), then passing on the fix. Normalize only documented " + "nondeterminism, never away a semantic delta. Re-review the full " + "inventory after fixes, not only the last finding. Missing baseline " + "or real-path evidence is not_yet_proven, not equivalent. This is a " + "reviewer-executed evidence gate, not automated execution or proof " + "of equivalence from CI, metadata, or a completed review template." + ), + }, { "evidence_id": "changed_line_classification", "required_when": "always", @@ -550,6 +613,7 @@ def build_review_execution_contract() -> dict[str, Any]: "stale_head_verdict_allowed": False, "blocking_evidence_verdicts": { "repository_reuse": ["unjustified_duplication", "not_yet_proven"], + "observable_semantics": ["unintended_drift", "not_yet_proven"], "change_proportionality": [ "disproportionate", "not_yet_proven", @@ -567,6 +631,11 @@ def build_review_execution_contract() -> dict[str, Any]: }, "verdict_policy": { "open_pr_blocking_finding": "REQUEST_CHANGES", + "open_pr_unresolved_semantics": ( + "REQUEST_CHANGES when observable_semantics is unintended_drift or " + "not_yet_proven; equal decision codes, green suites, stricter checks " + "and resolution of the previous finding cannot establish compatibility" + ), "open_pr_unresolved_reuse": ( "REQUEST_CHANGES when repository_reuse is unjustified_duplication " "or not_yet_proven, including missing/unverified search evidence; " @@ -629,6 +698,7 @@ def build_review_plan(item: Mapping[str, Any]) -> dict[str, Any]: required_evidence.append("typed_state_rule") if behavior_bearing_change: required_evidence.insert(3, "repository_reuse") + required_evidence.append("observable_semantics") if "scope_fit" not in required_evidence: required_evidence.append("scope_fit") required_evidence.append("default_off_isolation") @@ -659,6 +729,7 @@ def build_review_plan(item: Mapping[str, Any]) -> dict[str, Any]: "symbol_map_required": code_change, "scope_fit_required": behavior_bearing_change, "repository_reuse_required": behavior_bearing_change, + "observable_semantics_required": behavior_bearing_change, "change_proportionality_required": code_change, "default_off_isolation_required": behavior_bearing_change, "authority_semantics_required": code_change, diff --git a/tests/capabilities/test_pr_review_contract.py b/tests/capabilities/test_pr_review_contract.py index e3897526cb..44f12182c8 100644 --- a/tests/capabilities/test_pr_review_contract.py +++ b/tests/capabilities/test_pr_review_contract.py @@ -43,6 +43,7 @@ def test_execution_contract_owns_deep_review_requirements() -> None: "problem_context", "architecture_flow", "repository_reuse", + "observable_semantics", "changed_line_classification", "scope_fit", "symbol_map", @@ -121,6 +122,7 @@ def test_execution_contract_owns_deep_review_requirements() -> None: assert contract["completion_gate"]["stale_head_verdict_allowed"] is False assert contract["completion_gate"]["blocking_evidence_verdicts"] == { "repository_reuse": ["unjustified_duplication", "not_yet_proven"], + "observable_semantics": ["unintended_drift", "not_yet_proven"], "change_proportionality": ["disproportionate", "not_yet_proven"], "default_off_isolation": ["not_isolated", "not_yet_proven"], "authority_semantics": ["misleading", "not_yet_proven"], @@ -341,3 +343,81 @@ def test_reuse_evidence_compares_semantics_beyond_the_diff() -> None: 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"] + + +@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_observable_parity_is_required_without_refactor_title_detection( + area: str, +) -> None: + item = _item(areas={area: 1}) + item["title"] = "Extract shared decision helper" + item["checks"] = {"counts": {"success": 51, "failure": 0}} + plan = build_review_plan(item) + assert plan["applicability"]["observable_semantics_required"] is True + assert plan["result_template"]["evidence"]["observable_semantics"] == { + "status": "unverified" + } + + +@pytest.mark.parametrize("area", ["public_docs", "test_or_example"]) +def test_non_behavior_changes_do_not_invent_parity_execution(area: str) -> None: + plan = build_review_plan(_item(areas={area: 1})) + assert plan["applicability"]["observable_semantics_required"] is False + assert "observable_semantics" not in plan["required_evidence_ids"] + + +def test_observable_semantics_covers_diagnostics_and_claim_neutral_note_paths() -> None: + contract = build_agent_response_contract()["review_execution_contract"] + parity = next( + row + for row in contract["evidence_requirements"] + if row["evidence_id"] == "observable_semantics" + ) + assert parity["required_when"] == "behavior_bearing_change" + assert { + "baseline_revision", + "reviewed_head", + "caller_branch_inventory", + "comparison_rows", + "intentional_deltas", + "regression_sensitivity", + "unverified_dimensions", + "verdict", + } <= set(parity["fields"]) + assert { + "accepted_inputs_and_defaults", + "eligibility_and_rejection_precedence", + "full_diagnostics_and_remediation", + "argument_to_persistence_readback", + "state_receipts_and_no_effects", + "replay_and_concurrent_updates", + } <= set(parity["comparison_dimensions"]) + assert { + "input_and_pre_state", + "entrypoint_and_backend", + "baseline_observation", + "head_observation", + "expected_invariant_source", + "validation_evidence", + } <= set(parity["row_fields"]) + assert parity["verdict_values"] == [ + "equivalent", + "intentional_change_validated", + "unintended_drift", + "not_yet_proven", + ] + assert "reviewer-executed" in parity["rule"] + assert ( + "observable_semantics" + in contract["verdict_policy"]["open_pr_unresolved_semantics"] + ) diff --git a/tests/control_plane/test_local_coordination_authority.py b/tests/control_plane/test_local_coordination_authority.py index f64da4ae2d..300d9b65b7 100644 --- a/tests/control_plane/test_local_coordination_authority.py +++ b/tests/control_plane/test_local_coordination_authority.py @@ -525,6 +525,22 @@ def test_real_shadow_projection_promotes_complete_complex_todo_semantics( assert corrected_item["last_actor_agent_id"] == "agent-b" assert not state_file.exists() + # A note-only CLI edit must survive an independent provider readback too; + # accepting the option or exercising the text branch cannot prove this. + note_command = [*correction_command[:-2], "--note", "Correction context"] + note_result = subprocess.run( + note_command, capture_output=True, text=True, check=True, timeout=30 + ) + assert json.loads(note_result.stdout)["ok"] is True + noted = list_goal_todos(registry_path=registry_path, goal_id="goal-a") + noted_item = next( + item for item in noted["todos"] if item["todo_id"] == "todo_claimable" + ) + assert noted_item["note"] == "Correction context" + assert noted_item["text"] == "Corrected before claiming" + assert not noted_item.get("claimed_by") + assert not state_file.exists() + claim_command = [ sys.executable, "-m", "loopx.cli", "--format", "json", "--registry", str(registry_path), "todo", "claim", "--goal-id", "goal-a", @@ -606,7 +622,7 @@ def test_real_shadow_projection_promotes_complete_complex_todo_semantics( assert created_item["text"] == "Create directly against promoted provider" assert created_item["claimed_by"] == "agent-a" assert after_create["authority_read"]["todo_read_model"]["todo_count"] == 4 - assert claimed_item["note"] == claimable["note"] + assert claimed_item["note"] == "Correction context" # Real CLI, no Markdown file: provider data feeds an in-memory editor and # only requested fields return through TS CAS. Complex sibling fields do