From d6ed890f6924a2ae7ae4b04fa95072ee853c9b97 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 19 Jul 2026 20:57:58 -0700 Subject: [PATCH] feat: add local code simplicity review ## Summary - Add a standalone read-only code-simplicity skill with a repository-reuse and cognitive-load rubric - Add policy reuse, one-pass flow, wrapper, test setup, explicit-security-test, and clean evaluations - Prove standalone raw-evidence review and document the skill ## Why - Reduce duplicated policy, unnecessary phases, pass-through indirection, and irrelevant setup without redesigning the solution - Reject mechanical DRY and abstractions that only relocate complexity --- CHANGELOG.md | 2 + README.md | 2 + .../code-simplicity-clean/expected.json | 9 ++ .../code-simplicity-clean/packet.json | 20 ++++ .../expected.json | 21 ++++ .../complexity-relocating-wrapper/packet.json | 20 ++++ .../explicit-tests-preserved/expected.json | 9 ++ .../explicit-tests-preserved/packet.json | 20 ++++ review-suite/fixtures/manifest.json | 5 + .../multi-pass-control-flow/expected.json | 21 ++++ .../multi-pass-control-flow/packet.json | 24 +++++ .../fixtures/shared-test-setup/expected.json | 21 ++++ .../fixtures/shared-test-setup/packet.json | 20 ++++ skills/review-code-simplicity/SKILL.md | 96 +++++++++++++++++++ .../review-code-simplicity/agents/openai.yaml | 4 + .../candidate.diff | 13 +++ .../standalone-duplicated-policy/prompt.md | 6 ++ .../repository-evidence.md | 10 ++ .../standalone-duplicated-policy/result.json | 39 ++++++++ .../standalone-duplicated-policy/ticket.md | 5 + .../validation.md | 7 ++ .../references/code-simplicity-rubric.md | 74 ++++++++++++++ .../scripts/tests/test_skill_contract.py | 82 ++++++++++++++++ 23 files changed, 530 insertions(+) create mode 100644 review-suite/fixtures/code-simplicity-clean/expected.json create mode 100644 review-suite/fixtures/code-simplicity-clean/packet.json create mode 100644 review-suite/fixtures/complexity-relocating-wrapper/expected.json create mode 100644 review-suite/fixtures/complexity-relocating-wrapper/packet.json create mode 100644 review-suite/fixtures/explicit-tests-preserved/expected.json create mode 100644 review-suite/fixtures/explicit-tests-preserved/packet.json create mode 100644 review-suite/fixtures/multi-pass-control-flow/expected.json create mode 100644 review-suite/fixtures/multi-pass-control-flow/packet.json create mode 100644 review-suite/fixtures/shared-test-setup/expected.json create mode 100644 review-suite/fixtures/shared-test-setup/packet.json create mode 100644 skills/review-code-simplicity/SKILL.md create mode 100644 skills/review-code-simplicity/agents/openai.yaml create mode 100644 skills/review-code-simplicity/evals/standalone-duplicated-policy/candidate.diff create mode 100644 skills/review-code-simplicity/evals/standalone-duplicated-policy/prompt.md create mode 100644 skills/review-code-simplicity/evals/standalone-duplicated-policy/repository-evidence.md create mode 100644 skills/review-code-simplicity/evals/standalone-duplicated-policy/result.json create mode 100644 skills/review-code-simplicity/evals/standalone-duplicated-policy/ticket.md create mode 100644 skills/review-code-simplicity/evals/standalone-duplicated-policy/validation.md create mode 100644 skills/review-code-simplicity/references/code-simplicity-rubric.md create mode 100644 skills/review-code-simplicity/scripts/tests/test_skill_contract.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b1bfbc..503a572 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 87d02a5..7e67d1a 100644 --- a/README.md +++ b/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 diff --git a/review-suite/fixtures/code-simplicity-clean/expected.json b/review-suite/fixtures/code-simplicity-clean/expected.json new file mode 100644 index 0000000..0271b4c --- /dev/null +++ b/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." +} diff --git a/review-suite/fixtures/code-simplicity-clean/packet.json b/review-suite/fixtures/code-simplicity-clean/packet.json new file mode 100644 index 0000000..7c89722 --- /dev/null +++ b/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"} + ] +} diff --git a/review-suite/fixtures/complexity-relocating-wrapper/expected.json b/review-suite/fixtures/complexity-relocating-wrapper/expected.json new file mode 100644 index 0000000..45ab831 --- /dev/null +++ b/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." +} diff --git a/review-suite/fixtures/complexity-relocating-wrapper/packet.json b/review-suite/fixtures/complexity-relocating-wrapper/packet.json new file mode 100644 index 0000000..7ffbca2 --- /dev/null +++ b/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"} + ] +} diff --git a/review-suite/fixtures/explicit-tests-preserved/expected.json b/review-suite/fixtures/explicit-tests-preserved/expected.json new file mode 100644 index 0000000..8db8357 --- /dev/null +++ b/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." +} diff --git a/review-suite/fixtures/explicit-tests-preserved/packet.json b/review-suite/fixtures/explicit-tests-preserved/packet.json new file mode 100644 index 0000000..a226a42 --- /dev/null +++ b/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"} + ] +} diff --git a/review-suite/fixtures/manifest.json b/review-suite/fixtures/manifest.json index 6c4b32c..7205686 100644 --- a/review-suite/fixtures/manifest.json +++ b/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}, diff --git a/review-suite/fixtures/multi-pass-control-flow/expected.json b/review-suite/fixtures/multi-pass-control-flow/expected.json new file mode 100644 index 0000000..9b70b3b --- /dev/null +++ b/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." +} diff --git a/review-suite/fixtures/multi-pass-control-flow/packet.json b/review-suite/fixtures/multi-pass-control-flow/packet.json new file mode 100644 index 0000000..c963fcf --- /dev/null +++ b/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"} + ] +} diff --git a/review-suite/fixtures/shared-test-setup/expected.json b/review-suite/fixtures/shared-test-setup/expected.json new file mode 100644 index 0000000..bf541d5 --- /dev/null +++ b/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." +} diff --git a/review-suite/fixtures/shared-test-setup/packet.json b/review-suite/fixtures/shared-test-setup/packet.json new file mode 100644 index 0000000..ea914dd --- /dev/null +++ b/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"} + ] +} diff --git a/skills/review-code-simplicity/SKILL.md b/skills/review-code-simplicity/SKILL.md new file mode 100644 index 0000000..0273460 --- /dev/null +++ b/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. diff --git a/skills/review-code-simplicity/agents/openai.yaml b/skills/review-code-simplicity/agents/openai.yaml new file mode 100644 index 0000000..cc6f217 --- /dev/null +++ b/skills/review-code-simplicity/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Review Code Simplicity" + short_description: "Reduce local code and cognitive complexity" + default_prompt: "Use $review-code-simplicity to find concrete behavior-preserving reuse and DRY improvements in this change." diff --git a/skills/review-code-simplicity/evals/standalone-duplicated-policy/candidate.diff b/skills/review-code-simplicity/evals/standalone-duplicated-policy/candidate.diff new file mode 100644 index 0000000..497cce6 --- /dev/null +++ b/skills/review-code-simplicity/evals/standalone-duplicated-policy/candidate.diff @@ -0,0 +1,13 @@ +diff --git a/actions.py b/actions.py +new file mode 100644 +--- /dev/null ++++ b/actions.py +@@ -0,0 +1,8 @@ ++def delete_user(actor, user): ++ if actor.role != 'admin' or actor.suspended: ++ raise Forbidden() ++ store.delete(user) ++def restore_user(actor, user): ++ if actor.role != 'admin' or actor.suspended: ++ raise Forbidden() ++ store.restore(user) diff --git a/skills/review-code-simplicity/evals/standalone-duplicated-policy/prompt.md b/skills/review-code-simplicity/evals/standalone-duplicated-policy/prompt.md new file mode 100644 index 0000000..a13bdab --- /dev/null +++ b/skills/review-code-simplicity/evals/standalone-duplicated-policy/prompt.md @@ -0,0 +1,6 @@ +Use the `review-code-simplicity` skill to review the candidate described by the +raw evidence in this directory. Read `ticket.md`, `repository-evidence.md`, +`candidate.diff`, and `validation.md`; do not inspect `result.json` or use a +prior conclusion. Reconstruct the change contract before inspecting local reuse. +Return only the shared review result JSON and do not modify files or repository +state. diff --git a/skills/review-code-simplicity/evals/standalone-duplicated-policy/repository-evidence.md b/skills/review-code-simplicity/evals/standalone-duplicated-policy/repository-evidence.md new file mode 100644 index 0000000..e277c47 --- /dev/null +++ b/skills/review-code-simplicity/evals/standalone-duplicated-policy/repository-evidence.md @@ -0,0 +1,10 @@ +# Repository evidence + +Repository: `example/admin` Base branch: `main` Candidate head: +`8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d` Comparison base: +`9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d` + +`auth.py:require_active_admin(actor)` is the repository-owned authorization +boundary. It raises `Forbidden` unless the actor has the admin role and is not +suspended. Existing administrator actions call this helper directly, and the +repository instructs contributors not to duplicate authorization policy. diff --git a/skills/review-code-simplicity/evals/standalone-duplicated-policy/result.json b/skills/review-code-simplicity/evals/standalone-duplicated-policy/result.json new file mode 100644 index 0000000..6119c5a --- /dev/null +++ b/skills/review-code-simplicity/evals/standalone-duplicated-policy/result.json @@ -0,0 +1,39 @@ +{ + "schema_version": "1.0", + "lens": "code_simplicity", + "candidate": { + "head_sha": "8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d", + "comparison_base_sha": "9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d9d" + }, + "verdict": "changes_required", + "findings": [ + { + "id": "code-simplicity.duplicated-active-admin-policy", + "lens": "code_simplicity", + "severity": "strong_recommendation", + "confidence": "high", + "rule": "Reuse the repository-owned authorization boundary instead of duplicating business policy.", + "evidence": [ + { + "location": "actions.py:2-3", + "detail": "delete_user reimplements the active-administrator check with a role comparison, suspended-state branch, and Forbidden error." + }, + { + "location": "actions.py:6-7", + "detail": "restore_user repeats the same authorization policy a second time." + }, + { + "location": "repository-evidence.md", + "detail": "require_active_admin(actor) already owns these exact semantics, existing administrator actions call it directly, and repository guidance forbids duplicating authorization policy." + } + ], + "concern": "The candidate creates two local copies of an established authorization rule, so future policy or failure-semantics changes must remain synchronized across three implementations.", + "impact": "Reviewers and maintainers must reason about the same authorization policy in the shared boundary and both new actions, increasing drift risk without adding useful separation.", + "proposed_change": "Call require_active_admin(actor) at the start of delete_user and restore_user, then retain only each action's store operation.", + "expected_effect": "Reduce three implementations of the active-administrator policy to one repository-owned implementation while preserving the two explicit action flows and their tested behavior.", + "location": "actions.py:1-8" + } + ], + "blocking_reasons": [], + "next_action": "Replace both inline authorization checks with direct calls to require_active_admin(actor), then rerun the focused and full test suites." +} diff --git a/skills/review-code-simplicity/evals/standalone-duplicated-policy/ticket.md b/skills/review-code-simplicity/evals/standalone-duplicated-policy/ticket.md new file mode 100644 index 0000000..808f6c1 --- /dev/null +++ b/skills/review-code-simplicity/evals/standalone-duplicated-policy/ticket.md @@ -0,0 +1,5 @@ +# Add administrator delete and restore actions + +Active administrators may delete and restore users. Every other actor, including +suspended administrators, must remain forbidden. This ticket does not change +role storage or the repository's authorization policy. diff --git a/skills/review-code-simplicity/evals/standalone-duplicated-policy/validation.md b/skills/review-code-simplicity/evals/standalone-duplicated-policy/validation.md new file mode 100644 index 0000000..8eeace4 --- /dev/null +++ b/skills/review-code-simplicity/evals/standalone-duplicated-policy/validation.md @@ -0,0 +1,7 @@ +# Validation evidence + +- Focused: `pytest tests/test_actions.py` passed, 6 tests covering both actions, + active administrators, non-administrators, and suspended administrators. +- Full: `pytest` passed, 24 tests. + +The candidate diff is complete and validation did not modify tracked files. diff --git a/skills/review-code-simplicity/references/code-simplicity-rubric.md b/skills/review-code-simplicity/references/code-simplicity-rubric.md new file mode 100644 index 0000000..1405195 --- /dev/null +++ b/skills/review-code-simplicity/references/code-simplicity-rubric.md @@ -0,0 +1,74 @@ +# Code-simplicity rubric + +Use this rubric after establishing the change contract and chosen solution. +Inspect only the changed code, tests, and immediate collaborators. + +## Search before inventing + +Search for matching repository primitives by behavior, not only by name: + +- shared policy and authorization helpers; +- domain types, validators, normalizers, serializers, and error translators; +- services, repositories, query builders, and extension points; +- test builders, fixtures, assertions, and scenario helpers; and +- already-installed dependencies that own the exact behavior. + +Verify semantic fit, including failure behavior and edge cases. Prefer direct +reuse. A wrapper around an existing primitive is a reduction only when it +removes meaningful caller knowledge rather than hiding one call. + +## Inspect local complexity + +Look for materially repeated: + +- business policy or state-transition rules; +- validation, normalization, serialization, and error mapping; +- queries and transaction boundaries; +- multi-pass transformations and intermediate representations; +- recursion or phase machinery for a direct bounded traversal; +- parameters and branches forwarded through thin layers; +- test setup that obscures the behavior under test; and +- hand-written behavior already maintained by a suitable dependency. + +Do not report repetition that makes different invariants, failure paths, or +security boundaries explicit. + +## Reject complexity relocation + +A proposed helper or abstraction must remove concepts from callers and have a +cohesive repository-owned responsibility. Reject it when it merely: + +- renames an existing operation; +- forwards the same parameters and branches; +- moves a conditional without centralizing policy; +- introduces a generic API for one use; +- replaces direct code with configuration; or +- forces readers to jump elsewhere to understand an unchanged amount of logic. + +## Simplify tests without erasing meaning + +Share stable construction and irrelevant defaults. Keep the action and +materially different expectation visible in each test. Preserve separate cases +for different authorization roles, failure modes, boundaries, state transitions, +concurrency interleavings, and public contracts even when their setup looks +similar. + +Do not parameterize tests merely to reduce lines if the resulting table hides +which invariant failed. + +## Evaluate dependencies + +For an installed dependency, cite its manifest entry and matching semantics. For +a new dependency, include maintenance health, security exposure, bundle or +runtime effect, license compatibility, and repository policy. Recommend it only +when those costs are lower than maintaining the bespoke behavior it replaces. + +## Measure the reduction + +Name the expected reduction using evidence: one policy instead of three, one +pass instead of four, no duplicate representation, two fewer branches, one less +public wrapper, or shared irrelevant setup while preserving explicit scenarios. +Lines may support the explanation but are never the sole measure. + +Route a change that replaces the implementation strategy, storage model, service +boundary, or operational design to the whole-solution lens. diff --git a/skills/review-code-simplicity/scripts/tests/test_skill_contract.py b/skills/review-code-simplicity/scripts/tests/test_skill_contract.py new file mode 100644 index 0000000..8cfe364 --- /dev/null +++ b/skills/review-code-simplicity/scripts/tests/test_skill_contract.py @@ -0,0 +1,82 @@ +from __future__ import annotations + +import importlib.util +import json +import unittest +from pathlib import Path + +SKILL_ROOT = Path(__file__).resolve().parents[2] +REPOSITORY_ROOT = SKILL_ROOT.parents[1] +REVIEW_SUITE = REPOSITORY_ROOT / "review-suite" +SPEC = importlib.util.spec_from_file_location( + "review_contract_validator", REVIEW_SUITE / "scripts" / "validate.py" +) +assert SPEC and SPEC.loader +VALIDATOR = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(VALIDATOR) + + +def load(path: Path): + return json.loads(path.read_text()) + + +class SkillContractTests(unittest.TestCase): + def test_skill_uses_shared_contract_and_is_read_only(self): + skill = (SKILL_ROOT / "SKILL.md").read_text() + self.assertIn("../../review-suite/CONTRACT.md", skill) + self.assertIn("Preserve read-only integrity", skill) + self.assertIn("From raw evidence", skill) + self.assertIn("already installed", skill) + self.assertIn("licensing", skill) + self.assertNotIn("code-review-pro", skill) + + def test_code_simplicity_fixture_results_conform(self): + expectations = { + "duplicated-policy": "changes_required", + "multi-pass-control-flow": "changes_required", + "complexity-relocating-wrapper": "changes_required", + "shared-test-setup": "changes_required", + "explicit-tests-preserved": "clean", + "code-simplicity-clean": "clean", + } + for fixture_name, verdict in expectations.items(): + with self.subTest(fixture=fixture_name): + fixture = REVIEW_SUITE / "fixtures" / fixture_name + packet = load(fixture / "packet.json") + result = load(fixture / "expected.json") + self.assertEqual([], VALIDATOR.validate_pair(packet, result)) + self.assertEqual("code_simplicity", result["lens"]) + self.assertEqual(verdict, result["verdict"]) + + def test_standalone_eval_finds_repository_reuse_from_raw_evidence(self): + evaluation = SKILL_ROOT / "evals" / "standalone-duplicated-policy" + prompt = (evaluation / "prompt.md").read_text() + evidence = "\n".join( + (evaluation / name).read_text() + for name in ( + "ticket.md", + "repository-evidence.md", + "candidate.diff", + "validation.md", + ) + ) + result = load(evaluation / "result.json") + + self.assertNotIn("expected", prompt.lower()) + self.assertNotIn("change_contract", evidence) + self.assertEqual([], VALIDATOR.validate_result(result)) + self.assertEqual("code_simplicity", result["lens"]) + self.assertEqual("changes_required", result["verdict"]) + self.assertEqual( + "8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d8d", + result["candidate"]["head_sha"], + ) + self.assertTrue( + any( + "active-admin-policy" in finding["id"] for finding in result["findings"] + ) + ) + + +if __name__ == "__main__": + unittest.main()