From 570505c4d484c434686c632f4a79f12b5a77bb9e Mon Sep 17 00:00:00 2001 From: joyzhzh Date: Thu, 10 Sep 2026 22:44:54 +0800 Subject: [PATCH] Add opt-in simplicity review policy module --- docs/modules/simplicity.md | 82 ++++++++++++++ examples/simplicity-review.md | 96 ++++++++++++++++ src/code_review_partner/modules/__init__.py | 1 + src/code_review_partner/modules/simplicity.py | 100 +++++++++++++++++ tests/test_simplicity.py | 103 ++++++++++++++++++ 5 files changed, 382 insertions(+) create mode 100644 docs/modules/simplicity.md create mode 100644 examples/simplicity-review.md create mode 100644 src/code_review_partner/modules/__init__.py create mode 100644 src/code_review_partner/modules/simplicity.py create mode 100644 tests/test_simplicity.py diff --git a/docs/modules/simplicity.md b/docs/modules/simplicity.md new file mode 100644 index 0000000..2e59682 --- /dev/null +++ b/docs/modules/simplicity.md @@ -0,0 +1,82 @@ +# Optional simplicity review + +The simplicity module adds four contextual questions to a second-pass review packet: +reuse a demonstrated capability, name a behavior-preserving replacement, account for +a present boundary, and give deferral a concrete trigger. + +It is opt-in. Ordinary `analyze` and `packet` commands keep their existing behavior. +The module exports a policy; it does not review code, invoke an AI provider, discover +repository context, apply fixes, or add deterministic detectors. + +## Use it + +With Code Review Partner installed, generate a policy and pass it to the existing CLI: + +```bash +python -m code_review_partner.modules.simplicity > simplicity-policy.json +review-partner packet change.diff --policy simplicity-policy.json -o simplicity-review.md +``` + +The first command writes JSON to stdout; shell redirection controls its destination +and can replace an existing file. Choose a new filename when preserving an earlier +export. The packet command retains the CLI's existing overwrite protections. + +From a checkout, Python 3.11 or newer can run the same workflow without installation: + +```bash +PYTHONPATH=src python -m code_review_partner.modules.simplicity > simplicity-policy.json +PYTHONPATH=src python -m code_review_partner packet examples/sample.diff --policy simplicity-policy.json -o simplicity-review.md +``` + +Add `--format json` to the packet command for a structured packet. The extra questions +appear in `policy.principles` in JSON and under **Review questions** in Markdown. No +additional activation flag or model connection is needed. + +To return to the default review, omit `--policy` on the next command. The module writes +no activation state or configuration. + +## What is composed + +Each invocation loads a fresh copy of the installed default policy, appends four +principles, and gives the result an ID ending in `+simplicity-v1`. It preserves the +base principles, thresholds, enabled rules, posture, enumerations, and finding +contract. The existing policy validator checks the result before export. + +The export is a snapshot: regenerate it after upgrading Code Review Partner to pick +up changes to the base policy. Version 1 composes only with the installed default; +it does not merge arbitrary custom policies. + +Passing the export to `analyze` changes policy metadata but leaves deterministic +findings unchanged. Its additional questions are intended for the optional human or +AI second pass. That reviewer continues to use the existing finding fields: located +evidence, impact, smallest credible action, confidence, severity, and a +counter-consideration. No result or packet schema changes are required. + +## Evidence before simplification + +A finding must name the proposed replacement, explain the required behavior it +preserves, and identify evidence that supports the recommendation. A helper name, +one implementation, fewer lines, or an unused-looking configuration does not establish +that a change is safe or worthwhile. + +The CLI only sees the supplied diff. It cannot establish that another helper exists, +that all callers tolerate a change, or that a dependency supports the required +platforms. When those facts matter and are missing, the reviewer should ask for +context and preserve uncertainty. Simplification remains subordinate to correctness, +security, reliability, accessibility, and explicit requirements. + +See the [paired synthetic examples](../../examples/simplicity-review.md) for both +useful simplifications and justified complexity. They illustrate review expectations; +they are not an empirical accuracy benchmark or claims about a model's responses. + +## Privacy and provenance + +The module uses only the existing local policy loader and Python standard library. +It reads no source files, makes no network requests, and adds no runtime dependencies. +The packet still contains the complete, unredacted diff; inspect it before sharing +with another person or service. Existing untrusted-input instructions remain in force. + +The reuse questions and explicit revisit triggers were inspired by +[Ponytail](https://github.com/DietrichGebert/ponytail/tree/356918eba965ee1eac64bd3a7f0dd02108350de5). +This module uses independently written questions and synthetic examples. It copies +no Ponytail source or skill text and has no runtime dependency on that project. diff --git a/examples/simplicity-review.md b/examples/simplicity-review.md new file mode 100644 index 0000000..cd1624e --- /dev/null +++ b/examples/simplicity-review.md @@ -0,0 +1,96 @@ +# Simplicity review: paired examples + +These synthetic cases illustrate the optional [simplicity module](../docs/modules/simplicity.md). +The supplied context is part of each case. A real diff without that context warrants +a question rather than a confident recommendation. These are reviewer examples, +not deterministic detections or measured model results. + +## 1. Reuse: duplication versus different semantics + +Both cases add this wrapper: + +```python +def label_for_customer(customer): + return customer.name.strip() +``` + +**Simplification candidate.** Supplied context includes an existing +`customer_label(customer)` helper with the same body and contract, in the same module. +All callers are internal and the new wrapper has no compatibility role. A useful +finding names `customer_label` as the replacement and explains that it avoids keeping +two implementations synchronized. Its counter-consideration is whether the new name +represents an intentional public contract. + +**Preserve the distinction.** The available helper is instead +`search_key(customer)`, which strips and case-folds the name for lookup. Display labels +must preserve capitalization. Similar code is not evidence of equivalent semantics; +reusing `search_key` would change visible behavior. + +## 2. Replacement: native input versus required behavior + +Both cases introduce a custom date-picker component: + +```jsx + +``` + +**Simplification candidate.** Supplied requirements ask only for one calendar date, +and the supported-browser checks confirm the required keyboard and form behavior of +``. A suggestion can name that native input, explain how value and +change handling would be adapted, and identify the custom dependency it could avoid. +The native element's existence alone would not establish equivalence. + +**Preserve required behavior.** The booking flow must expose available appointment +dates from the server with a tested screen-reader interaction. A plain date input does +not, by itself, implement that workflow. The reviewer should retain the requirement +and ask whether a smaller implementation can satisfy it; replacing the control solely +to reduce lines is unsupported. + +## 3. Abstraction: forwarding versus a useful boundary + +Both cases introduce an interface with one implementation: + +```python +class InvoiceReader(Protocol): + def read(self, invoice_id: str) -> Invoice: ... +``` + +**Simplification candidate.** Supplied context shows a private, single-module caller, +an existing concrete reader with the same contract, and no ownership, test, or +dependency boundary served by the new protocol. A reviewer may suggest using that +reader directly, citing the extra navigation and duplicated contract. The number of +implementations alone is not the evidence. + +**Preserve the boundary.** The same protocol is the application-facing boundary around +a vendor SDK. It prevents vendor types from leaking into billing logic and supports +contract tests. Those are present benefits even with one production implementation; +the reviewer should not recommend removal based on implementation count. + +## 4. Deferral: speculative scale versus an existing promise + +Both cases add machinery to schedule and retry work: + +```python +scheduler.enqueue(job, retry_policy=retry_policy) +``` + +**Simplification candidate.** Supplied requirements describe a local, user-run report +with immediate error reporting and no unattended or durable execution promise. A +measured representative run takes 30 ms against a 500 ms budget. If a proposed worker +queue serves only hypothetical throughput, a reviewer can suggest keeping execution +synchronous and revisiting when representative latency exceeds that budget or +unattended operation becomes a requirement. The suggestion must retain error +reporting and explain what the simpler version cannot do. + +**Preserve an existing promise.** The job submits a payment and must survive a process +restart without losing work or duplicating the payment. Low current traffic does not +remove the need for durable state, idempotency, and appropriate retry handling. A +deferral recommendation must not discard those guarantees. + +## Reporting expectations + +Use the existing review-result contract for any actual finding. Locate the evidence, +explain the impact, suggest the smallest credible action, separate severity from +confidence, and include the reason the current design might be justified. Do not +assign a blocker for line count or stylistic preference, infer missing requirements, +or treat an empty finding list as approval. diff --git a/src/code_review_partner/modules/__init__.py b/src/code_review_partner/modules/__init__.py new file mode 100644 index 0000000..8962692 --- /dev/null +++ b/src/code_review_partner/modules/__init__.py @@ -0,0 +1 @@ +"""Optional policy modules, activated explicitly by the caller.""" diff --git a/src/code_review_partner/modules/simplicity.py b/src/code_review_partner/modules/simplicity.py new file mode 100644 index 0000000..98f7a31 --- /dev/null +++ b/src/code_review_partner/modules/simplicity.py @@ -0,0 +1,100 @@ +"""Export an optional simplicity policy for the existing packet command.""" + +from __future__ import annotations + +import argparse +import json +import sys +from typing import Any, Sequence + +from ..policy import PolicyError, load_policy, validate_policy + + +def build_simplicity_policy() -> dict[str, Any]: + """Extend a fresh default policy without changing its rules or contract.""" + policy = load_policy() + policy["id"] += "+simplicity-v1" + policy["name"] += " with optional simplicity review" + policy["principles"].extend([ + { + "id": "simplicity-reuse", + "title": "Reuse a demonstrated capability", + "question": ( + "Does supplied context identify existing repository code that meets this " + "requirement? Name the helper and evidence that its behavior fits before " + "recommending reuse. If callers or requirements are missing, ask for that " + "context instead of asserting duplication or absence." + ), + "counterweights": [ + "different domain semantics", + "coupling across boundaries", + "unavailable repository context", + ], + }, + { + "id": "simplicity-replacement", + "title": "Preserve behavior when replacing code", + "question": ( + "Could the standard library, a native platform feature, or an already " + "installed dependency meet the requirement with less maintenance? Name " + "the replacement and evidence of equivalent required behavior, including " + "edge cases, compatibility, validation, error handling, security, and " + "accessibility. If equivalence or availability is unknown, ask a question." + ), + "counterweights": [ + "required behavior and supported versions", + "safety and accessibility guarantees", + "measured performance and migration cost", + ], + }, + { + "id": "simplicity-abstraction", + "title": "Account for a present boundary", + "question": ( + "What present boundary, invariant, or meaningful reduction in complexity " + "justifies this layer? Recommend simplification only with evidence that " + "the benefit is absent and the suggested change preserves behavior. One " + "implementation or fewer lines alone does not establish a problem." + ), + "counterweights": [ + "ownership and dependency boundaries", + "public contracts and test seams", + "locality and navigation cost", + ], + }, + { + "id": "simplicity-deferral", + "title": "Give deferral a concrete trigger", + "question": ( + "If a capability can be deferred, what supplied evidence shows it is " + "unneeded now, and what requirement or measured limit would justify " + "adding it later? State the current limitation and revisit trigger in " + "the suggestion. Unknown requirements warrant a question, not removal " + "of requested behavior or necessary safeguards." + ), + "counterweights": [ + "explicit requirements and compatibility promises", + "data loss and irreversible consequences", + "cost of later migration", + ], + }, + ]) + return validate_policy(policy) + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="Export the default policy plus optional simplicity questions as JSON to stdout.", + ) + parser.parse_args(argv) + try: + policy = build_simplicity_policy() + except PolicyError as error: + print(f"error: {error}", file=sys.stderr) + return 2 + sys.stdout.write(json.dumps(policy, ensure_ascii=False, indent=2, sort_keys=True) + "\n") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_simplicity.py b/tests/test_simplicity.py new file mode 100644 index 0000000..72bab70 --- /dev/null +++ b/tests/test_simplicity.py @@ -0,0 +1,103 @@ +import contextlib +import io +import json +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +from code_review_partner.analyzer import analyze +from code_review_partner.cli import main as review_main +from code_review_partner.diff import parse_unified_diff +from code_review_partner.modules.simplicity import build_simplicity_policy, main +from code_review_partner.policy import PolicyError, load_policy, validate_policy + + +ROOT = Path(__file__).resolve().parents[1] +SAMPLE_DIFF = (ROOT / "examples" / "sample.diff").read_text(encoding="utf-8") +NO_SIGNAL_DIFF = ( + "diff --git a/value.py b/value.py\n" + "--- /dev/null\n+++ b/value.py\n" + "@@ -0,0 +1 @@\n+value = 1\n" +) + + +class SimplicityTests(unittest.TestCase): + def test_composition_preserves_the_base_policy_and_has_no_shared_state(self) -> None: + base = load_policy() + extended = build_simplicity_policy() + expected = json.loads(json.dumps(extended)) + self.assertEqual(extended, validate_policy(extended)) + self.assertNotEqual(base["id"], extended["id"]) + for key in base.keys() - {"id", "name", "principles"}: + self.assertEqual(base[key], extended[key], key) + self.assertEqual(base["principles"], extended["principles"][:len(base["principles"])]) + self.assertEqual(len(base["principles"]) + 4, len(extended["principles"])) + + extended["thresholds"]["long_line_chars"] = 1 + extended["principles"][0]["counterweights"].append("caller mutation") + extended["principles"][-1]["counterweights"].append("caller mutation") + self.assertEqual(base, load_policy()) + self.assertEqual(expected, build_simplicity_policy()) + + def test_module_export_loads_through_existing_policy_and_packet_cli(self) -> None: + export = subprocess.run( + [sys.executable, "-m", "code_review_partner.modules.simplicity"], + check=True, capture_output=True, text=True, + ) + self.assertEqual("", export.stderr) + expected = build_simplicity_policy() + self.assertEqual(expected, json.loads(export.stdout)) + with tempfile.TemporaryDirectory() as directory: + policy_path = Path(directory) / "simplicity-policy.json" + policy_path.write_text(export.stdout, encoding="utf-8") + self.assertEqual(expected, load_policy(policy_path)) + packets = {} + for format_name in ("json", "markdown"): + output = io.StringIO() + with patch("sys.stdin", io.StringIO(SAMPLE_DIFF)), contextlib.redirect_stdout(output): + code = review_main([ + "packet", "-", "--policy", str(policy_path), "--format", format_name, + ]) + self.assertEqual(0, code) + packets[format_name] = output.getvalue() + + structured = json.loads(packets["json"]) + self.assertEqual(expected["principles"], structured["policy"]["principles"]) + self.assertEqual(load_policy()["finding_contract"], structured["policy"]["finding_contract"]) + self.assertEqual(SAMPLE_DIFF, structured["unified_diff"]) + for principle in expected["principles"][-4:]: + self.assertIn(principle["title"], packets["markdown"]) + self.assertIn("Unknown requirements warrant a question", packets["markdown"]) + self.assertIn("untrusted data", packets["markdown"]) + + def test_simplicity_does_not_change_signals_or_turn_silence_into_approval(self) -> None: + for diff in (SAMPLE_DIFF, NO_SIGNAL_DIFF): + with self.subTest(diff=diff): + document = parse_unified_diff(diff) + base = analyze(document, load_policy()).to_dict() + extended = analyze(document, build_simplicity_policy()).to_dict() + self.assertEqual(base["findings"], extended["findings"]) + self.assertEqual(base["summary"], extended["summary"]) + self.assertEqual(base["input"], extended["input"]) + self.assertEqual("no-signals", extended["summary"]["status"]) + self.assertTrue(extended["summary"]["limitations"]) + + def test_invalid_base_policy_produces_no_partial_json(self) -> None: + output = io.StringIO() + errors = io.StringIO() + with ( + patch("code_review_partner.modules.simplicity.load_policy", side_effect=PolicyError("invalid base")), + contextlib.redirect_stdout(output), + contextlib.redirect_stderr(errors), + ): + code = main([]) + self.assertEqual(2, code) + self.assertEqual("", output.getvalue()) + self.assertIn("invalid base", errors.getvalue()) + + +if __name__ == "__main__": + unittest.main()