Add an opt-in simplicity review module

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.

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
+<BookingDatePicker value={date} onChange={setDate} />
+```
+
+**Simplification candidate.** Supplied requirements ask only for one calendar date,
+and the supported-browser checks confirm the required keyboard and form behavior of
+`<input type="date">`. 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.

src/code_review_partner/modules/__init__.py

@@ -0,0 +1 @@
+"""Optional policy modules, activated explicitly by the caller."""

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())

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()