Add review-code-simplicity skill

CHANGELOG.md

@@ -6,7 +6,9 @@ summary: Chronological history of repository and skill changes.
 
 ## 2026-07-19 — Epic workflow and review contract cleanup
 
+- feat: add local code simplicity review
 - feat: add whole-solution simplicity review
+  (8459402e95888047587cf423454f9f8ac42f6881)
 - feat: add goal-first correctness review
   (33feab3570363f8bf0d24ed4295495dc05fa3abf)
 - feat: define shared code review contracts

README.md

@@ -19,6 +19,8 @@ Current skills:
   compatibility, data-integrity, and validation failures in a code change
 - `skills/review-solution-simplicity` — challenge whole-solution machinery that
   is not justified by real requirements or repository constraints
+- `skills/review-code-simplicity` — reduce local cognitive load through
+  behavior-preserving reuse, DRY, control-flow, and test simplification
 
 ## Quick Start
 

review-suite/fixtures/code-simplicity-clean/expected.json

@@ -0,0 +1,9 @@
+{
+  "schema_version": "1.0",
+  "lens": "code_simplicity",
+  "candidate": {"head_sha": "8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c", "comparison_base_sha": "9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c"},
+  "verdict": "clean",
+  "findings": [],
+  "blocking_reasons": [],
+  "next_action": "No material code-simplicity action is required."
+}

review-suite/fixtures/code-simplicity-clean/packet.json

@@ -0,0 +1,20 @@
+{
+  "schema_version": "1.0",
+  "repository": {"identity": "example/profile", "base_branch": "main"},
+  "candidate": {
+    "head_sha": "8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c8c",
+    "comparison_base_sha": "9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c9c",
+    "diff": {"format": "unified_diff", "complete": true, "content": "diff --git a/profile.py b/profile.py\n--- a/profile.py\n+++ b/profile.py\n@@ -1,2 +1,4 @@\n def display_name(user):\n-    return user.name\n+    if user.preferred_name:\n+        return user.preferred_name\n+    return user.name\n"}
+  },
+  "change_contract": {
+    "goal": "Display a preferred name when present.",
+    "acceptance_criteria": ["Preferred names take precedence.", "The existing name remains the fallback."],
+    "non_goals": ["Change name storage or validation."],
+    "preserved_behaviors": ["Users without a preferred name display their existing name."]
+  },
+  "sources": {"repository_instructions": [], "named_documents": [], "nearby_patterns": [{"label": "Simple fallback convention", "location": "contact.py", "summary": "Use an explicit branch when fallback values have different domain meaning."}]},
+  "validation": [
+    {"name": "profile tests", "command": "pytest tests/test_profile.py", "scope": "focused", "status": "passed", "result": "4 passed"},
+    {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "20 passed"}
+  ]
+}

review-suite/fixtures/complexity-relocating-wrapper/expected.json

@@ -0,0 +1,21 @@
+{
+  "schema_version": "1.0",
+  "lens": "code_simplicity",
+  "candidate": {"head_sha": "8989898989898989898989898989898989898989", "comparison_base_sha": "9999999999999999999999999999999999999999"},
+  "verdict": "changes_required",
+  "findings": [{
+    "id": "code-simplicity.remove-policy-adapter",
+    "lens": "code_simplicity",
+    "severity": "strong_recommendation",
+    "confidence": "high",
+    "rule": "Prefer direct use of the existing policy owner when a wrapper does not adapt semantics or remove caller knowledge.",
+    "evidence": [{"location": "access.py:1", "detail": "AccessPolicyAdapter forwards the same actor and resource directly to is_allowed and has one caller."}, {"location": "edit.py:can_edit", "detail": "Nearby code calls policy functions directly when no semantic adaptation exists."}],
+    "concern": "The adapter moves one call behind a class without removing any branch, parameter, or policy concept.",
+    "impact": "It adds a public concept, allocation, method, and call-path hop while leaving the caller's knowledge unchanged.",
+    "proposed_change": "Remove AccessPolicyAdapter and return is_allowed(actor, resource) directly from can_view.",
+    "expected_effect": "Remove one class, one method, one allocation, and one call hop without changing policy behavior.",
+    "location": "access.py:1"
+  }],
+  "blocking_reasons": [],
+  "next_action": "Remove the pass-through adapter and retain direct use of is_allowed."
+}

review-suite/fixtures/complexity-relocating-wrapper/packet.json

@@ -0,0 +1,20 @@
+{
+  "schema_version": "1.0",
+  "repository": {"identity": "example/policies", "base_branch": "main"},
+  "candidate": {
+    "head_sha": "8989898989898989898989898989898989898989",
+    "comparison_base_sha": "9999999999999999999999999999999999999999",
+    "diff": {"format": "unified_diff", "complete": true, "content": "diff --git a/access.py b/access.py\n--- a/access.py\n+++ b/access.py\n@@ -1,2 +1,7 @@\n+class AccessPolicyAdapter:\n+    def evaluate(self, actor, resource):\n+        return is_allowed(actor, resource)\n+\n def can_view(actor, resource):\n-    return is_allowed(actor, resource)\n+    adapter = AccessPolicyAdapter()\n+    return adapter.evaluate(actor, resource)\n"}
+  },
+  "change_contract": {
+    "goal": "Keep resource visibility on the existing access policy.",
+    "acceptance_criteria": ["can_view returns the result of is_allowed."],
+    "non_goals": ["Create a public policy-adapter API."],
+    "preserved_behaviors": ["is_allowed remains the policy owner."]
+  },
+  "sources": {"repository_instructions": [], "named_documents": [], "nearby_patterns": [{"label": "Direct policy use", "location": "edit.py:can_edit", "summary": "Call policy functions directly unless adapting different semantics."}]},
+  "validation": [
+    {"name": "access tests", "command": "pytest tests/test_access.py", "scope": "focused", "status": "passed", "result": "4 passed"},
+    {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "17 passed"}
+  ]
+}

review-suite/fixtures/explicit-tests-preserved/expected.json

@@ -0,0 +1,9 @@
+{
+  "schema_version": "1.0",
+  "lens": "code_simplicity",
+  "candidate": {"head_sha": "8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b", "comparison_base_sha": "9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b"},
+  "verdict": "clean",
+  "findings": [],
+  "blocking_reasons": [],
+  "next_action": "Keep the three authorization invariants explicit; no material local simplification is warranted."
+}

review-suite/fixtures/explicit-tests-preserved/packet.json

@@ -0,0 +1,20 @@
+{
+  "schema_version": "1.0",
+  "repository": {"identity": "example/authorization", "base_branch": "main"},
+  "candidate": {
+    "head_sha": "8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b8b",
+    "comparison_base_sha": "9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b9b",
+    "diff": {"format": "unified_diff", "complete": true, "content": "diff --git a/test_authorization.py b/test_authorization.py\nnew file mode 100644\n--- /dev/null\n+++ b/test_authorization.py\n@@ -0,0 +1,11 @@\n+def test_non_admin_is_forbidden():\n+    actor = actor_for(role='member', suspended=False, tenant='a')\n+    with raises(Forbidden):\n+        export(actor, tenant='a')\n+\n+def test_suspended_admin_is_forbidden():\n+    actor = actor_for(role='admin', suspended=True, tenant='a')\n+    with raises(Forbidden):\n+        export(actor, tenant='a')\n+\n+def test_cross_tenant_admin_is_forbidden():\n+    actor = actor_for(role='admin', suspended=False, tenant='b')\n+    with raises(Forbidden):\n+        export(actor, tenant='a')\n"}
+  },
+  "change_contract": {
+    "goal": "Prove each independent export authorization boundary.",
+    "acceptance_criteria": ["Role, suspension, and tenant boundaries each reject access independently."],
+    "non_goals": ["Collapse distinct authorization guarantees into an opaque table."],
+    "preserved_behaviors": ["Each failed boundary identifies its own contract in test output."]
+  },
+  "sources": {"repository_instructions": [{"label": "Security test convention", "location": "SECURITY.md", "summary": "Keep independent authorization invariants in named tests."}], "named_documents": [], "nearby_patterns": []},
+  "validation": [
+    {"name": "authorization tests", "command": "pytest test_authorization.py", "scope": "focused", "status": "passed", "result": "3 passed"},
+    {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "30 passed"}
+  ]
+}

review-suite/fixtures/manifest.json

@@ -4,6 +4,11 @@
   {"name": "missing-test", "packet_valid": true},
   {"name": "repository-convention-clean", "packet_valid": true},
   {"name": "duplicated-policy", "packet_valid": true},
+  {"name": "multi-pass-control-flow", "packet_valid": true},
+  {"name": "complexity-relocating-wrapper", "packet_valid": true},
+  {"name": "shared-test-setup", "packet_valid": true},
+  {"name": "explicit-tests-preserved", "packet_valid": true},
+  {"name": "code-simplicity-clean", "packet_valid": true},
   {"name": "imagined-machinery", "packet_valid": true},
   {"name": "necessary-complexity", "packet_valid": true},
   {"name": "speculative-backfill", "packet_valid": true},

review-suite/fixtures/multi-pass-control-flow/expected.json

@@ -0,0 +1,21 @@
+{
+  "schema_version": "1.0",
+  "lens": "code_simplicity",
+  "candidate": {"head_sha": "8888888888888888888888888888888888888888", "comparison_base_sha": "9898989898989898989898989898989898989898"},
+  "verdict": "changes_required",
+  "findings": [{
+    "id": "code-simplicity.use-one-pass-total",
+    "lens": "code_simplicity",
+    "severity": "strong_recommendation",
+    "confidence": "high",
+    "rule": "Use the repository's one-pass accumulation pattern when intermediate collections have no required meaning.",
+    "evidence": [{"location": "totals.py:2", "detail": "The candidate materializes four sequential collections used only by the next phase."}, {"location": "credits.py:credit_total", "detail": "The nearby primitive normalizes once and accumulates qualifying rounded values directly."}],
+    "concern": "Four named phases and collections obscure one local filter-and-sum operation.",
+    "impact": "Reviewers must track five representations of the same rows and the runtime allocates four unnecessary collections.",
+    "proposed_change": "Use one direct loop that normalizes each raw row once, skips non-billable rows, and adds round_cents(price(row)) to the total.",
+    "expected_effect": "Reduce four passes and four intermediate representations to one pass and one accumulator while preserving normalization and rounding behavior.",
+    "location": "totals.py:2"
+  }],
+  "blocking_reasons": [],
+  "next_action": "Replace the phase pipeline with the repository's direct accumulation pattern and rerun total tests."
+}

review-suite/fixtures/multi-pass-control-flow/packet.json

@@ -0,0 +1,24 @@
+{
+  "schema_version": "1.0",
+  "repository": {"identity": "example/billing", "base_branch": "main"},
+  "candidate": {
+    "head_sha": "8888888888888888888888888888888888888888",
+    "comparison_base_sha": "9898989898989898989898989898989898989898",
+    "diff": {"format": "unified_diff", "complete": true, "content": "diff --git a/totals.py b/totals.py\nnew file mode 100644\n--- /dev/null\n+++ b/totals.py\n@@ -0,0 +1,6 @@\n+def billable_total(raw_rows):\n+    normalized = [normalize(row) for row in raw_rows]\n+    billable = [row for row in normalized if row.billable]\n+    amounts = [price(row) for row in billable]\n+    rounded = [round_cents(amount) for amount in amounts]\n+    return sum(rounded)\n"}
+  },
+  "change_contract": {
+    "goal": "Calculate the rounded total of billable rows.",
+    "acceptance_criteria": ["Each row is normalized once.", "Only billable rows contribute their rounded price."],
+    "non_goals": ["Retain intermediate collections."],
+    "preserved_behaviors": ["Input order does not change the total."]
+  },
+  "sources": {
+    "repository_instructions": [],
+    "named_documents": [],
+    "nearby_patterns": [{"label": "Streaming totals", "location": "credits.py:credit_total", "summary": "Normalize each input once and accumulate qualifying rounded values in one loop."}]
+  },
+  "validation": [
+    {"name": "total tests", "command": "pytest tests/test_totals.py", "scope": "focused", "status": "passed", "result": "5 passed"},
+    {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "26 passed"}
+  ]
+}

review-suite/fixtures/shared-test-setup/expected.json

@@ -0,0 +1,21 @@
+{
+  "schema_version": "1.0",
+  "lens": "code_simplicity",
+  "candidate": {"head_sha": "8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a", "comparison_base_sha": "9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a"},
+  "verdict": "changes_required",
+  "findings": [{
+    "id": "code-simplicity.reuse-uploader-builder",
+    "lens": "code_simplicity",
+    "severity": "strong_recommendation",
+    "confidence": "high",
+    "rule": "Reuse the repository's uploader builder for irrelevant construction while keeping materially different boundary scenarios explicit.",
+    "evidence": [{"location": "test_uploads.py", "detail": "All three tests repeat the same FakeStore, FixedClock, and Uploader construction."}, {"location": "tests/helpers.py:make_uploader", "detail": "The existing builder supplies exactly that setup with a configurable byte limit."}],
+    "concern": "Repeated irrelevant construction obscures the below, equal, and above-limit scenarios.",
+    "impact": "The same setup policy appears three times and must be reviewed independently without adding scenario meaning.",
+    "proposed_change": "Call make_uploader(max_bytes=10) in each named test while retaining the three separate actions and expectations.",
+    "expected_effect": "Reduce three copies of construction policy to one shared helper while preserving three explicit boundary tests.",
+    "location": "test_uploads.py"
+  }],
+  "blocking_reasons": [],
+  "next_action": "Reuse make_uploader for setup and keep the boundary tests separate."
+}

review-suite/fixtures/shared-test-setup/packet.json

@@ -0,0 +1,20 @@
+{
+  "schema_version": "1.0",
+  "repository": {"identity": "example/uploads", "base_branch": "main"},
+  "candidate": {
+    "head_sha": "8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a8a",
+    "comparison_base_sha": "9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a9a",
+    "diff": {"format": "unified_diff", "complete": true, "content": "diff --git a/test_uploads.py b/test_uploads.py\nnew file mode 100644\n--- /dev/null\n+++ b/test_uploads.py\n@@ -0,0 +1,17 @@\n+def test_small_upload():\n+    store = FakeStore()\n+    clock = FixedClock()\n+    uploader = Uploader(store, clock, max_bytes=10)\n+    assert uploader.upload(b'abc').size == 3\n+\n+def test_limit_upload():\n+    store = FakeStore()\n+    clock = FixedClock()\n+    uploader = Uploader(store, clock, max_bytes=10)\n+    assert uploader.upload(b'0123456789').size == 10\n+\n+def test_large_upload_rejected():\n+    store = FakeStore()\n+    clock = FixedClock()\n+    uploader = Uploader(store, clock, max_bytes=10)\n+    with raises(TooLarge):\n+        uploader.upload(b'01234567890')\n"}
+  },
+  "change_contract": {
+    "goal": "Prove upload size behavior below, at, and above the limit.",
+    "acceptance_criteria": ["Small and boundary uploads succeed.", "Over-limit uploads raise TooLarge."],
+    "non_goals": ["Hide the three boundary scenarios in one parameterized test."],
+    "preserved_behaviors": ["Each boundary remains an explicit named test."]
+  },
+  "sources": {"repository_instructions": [], "named_documents": [], "nearby_patterns": [{"label": "Uploader test builder", "location": "tests/helpers.py:make_uploader", "summary": "Builds Uploader with FakeStore, FixedClock, and configurable max_bytes."}]},
+  "validation": [
+    {"name": "upload tests", "command": "pytest test_uploads.py", "scope": "focused", "status": "passed", "result": "3 passed"},
+    {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "22 passed"}
+  ]
+}

skills/review-code-simplicity/SKILL.md

@@ -0,0 +1,96 @@
+---
+name: review-code-simplicity
+description: Review a code change for local implementation complexity, meaningful DRY and reuse opportunities, unnecessary control flow, and bespoke code replaceable by established repository modules or justified dependencies. Use for implementation-level simplicity review of a PR, branch, patch, or tests, either from raw evidence or the shared review packet. Preserve the chosen architecture and behavior, return only the shared result shape, and never modify the candidate.
+---
+
+# Review Code Simplicity
+
+Reduce the concepts a reviewer and maintainer must hold inside the chosen
+solution. Review only; leave refactoring and correctness revalidation to the
+caller.
+
+## Load the contracts
+
+1. Read the canonical review contract at `../../review-suite/CONTRACT.md` and
+   its packet and result schemas.
+2. Read [the code-simplicity rubric](references/code-simplicity-rubric.md).
+3. Treat the canonical contract as authoritative for evidence, finding fields,
+   severity, confidence, verdicts, candidate identity, and base drift.
+4. Return `blocked` with the missing dependency when the canonical contract is
+   unavailable. Do not invent or copy a local replacement.
+
+## Establish the candidate
+
+- Validate a supplied packet before reviewing. Convert missing essential
+  evidence into a conforming `blocked` result.
+- From raw evidence, establish repository and candidate identity, the complete
+  diff, observable change contract, applicable repository sources, immediate
+  collaborators, and exact validation results before suggesting a refactor.
+- Bind the result to the captured candidate and follow the shared base-drift
+  rules.
+
+## Inspect local implementation
+
+1. Preserve the ticket's selected architecture. Route a materially different
+   implementation strategy to `review-solution-simplicity`.
+2. Read changed functions, modules, tests, and their immediate collaborators.
+3. Search the repository for shared modules, domain primitives, extension
+   points, test helpers, and installed dependencies before proposing new code.
+4. Compare the candidate's policy, transformations, validation, state changes,
+   error handling, queries, and test setup with those existing primitives.
+5. Inspect control-flow depth, repeated passes, redundant representations,
+   pass-through wrappers, and bespoke library-like behavior.
+6. Construct the smallest behavior-preserving local change and state its
+   measurable cognitive reduction.
+
+Prefer direct reuse over a new wrapper. Reject abstractions that only move the
+same branches, parameters, or concepts. Do not optimize for line count alone.
+
+## Apply the material threshold
+
+Every finding must cite concrete candidate and repository evidence, specify the
+smallest behavior-preserving change, and identify a material reduction in
+duplicated policies, concepts, branches, states, passes, call depth, public
+surface, bespoke behavior, or meaningful setup.
+
+- Use `blocking` only when local complexity creates a demonstrated correctness,
+  architecture, or validation hazard.
+- Use `strong_recommendation` for a clear, tractable simplification with a net
+  reduction in reviewer or maintainer burden.
+- Use `defer` only for an evidenced concern outside the active ticket or
+  awaiting a named decision.
+- Omit naming preferences, cosmetic extraction, formatting, mechanical DRY,
+  vague refactoring, and changes that merely relocate complexity.
+
+Keep explicit tests separate when they communicate materially different
+invariants or failures. Share setup only when the scenarios remain immediately
+legible.
+
+## Evaluate dependency reuse
+
+Recommend an existing dependency only after verifying that its semantics match.
+State that it is already installed and identify the bespoke behavior and
+concepts removed.
+
+Recommend a new common dependency only when repository policy allows it and the
+net reduction remains clear after maintenance, security, bundle or runtime, and
+licensing costs. Do not add a dependency for trivial behavior or future use.
+
+## Return the shared result
+
+Return only JSON conforming to
+`../../review-suite/contracts/review-result.schema.json` with lens
+`code_simplicity`.
+
+- Return `clean` when no material local simplification remains.
+- Return `changes_required` when a blocking or strong-recommendation finding
+  remains.
+- Return `blocked` when essential evidence prevents a trustworthy comparison.
+- Keep deferred findings non-gating and do not add prose outside the result.
+
+## Preserve read-only integrity
+
+Do not edit or format files, apply refactors, create repository artifacts,
+commit, push, resolve threads, post reviews, or update tickets. Run only safe
+read-only inspection and validation commands. Preserve supplied pre-review
+candidate state exactly and report unexpected mutation as an integrity failure.