From 8459402e95888047587cf423454f9f8ac42f6881 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Sun, 19 Jul 2026 20:43:51 -0700 Subject: [PATCH] feat: add whole-solution simplicity review ## Summary - Add a standalone read-only solution-simplicity skill with a mechanism-to-requirement rubric - Add over-engineering, required-complexity, missing-requirement, and raw standalone evaluations - Align explicit ticket-scope violations with the shared blocking severity and document the skill ## Why - Identify materially smaller requirement-complete designs without trading away correctness or operational invariants - Keep speculative frameworks, compatibility layers, and migrations out of narrowly scoped changes --- CHANGELOG.md | 2 + README.md | 2 + .../fixtures/imagined-machinery/expected.json | 2 +- review-suite/fixtures/manifest.json | 2 + .../expected.json | 10 ++ .../packet.json | 29 ++++++ .../speculative-backfill/expected.json | 24 +++++ .../fixtures/speculative-backfill/packet.json | 29 ++++++ skills/review-solution-simplicity/SKILL.md | 97 +++++++++++++++++++ .../agents/openai.yaml | 4 + .../candidate.diff | 29 ++++++ .../standalone-provider-framework/prompt.md | 6 ++ .../repository-evidence.md | 10 ++ .../standalone-provider-framework/result.json | 38 ++++++++ .../standalone-provider-framework/ticket.md | 7 ++ .../validation.md | 7 ++ .../references/solution-simplicity-rubric.md | 82 ++++++++++++++++ .../scripts/tests/test_skill_contract.py | 76 +++++++++++++++ 18 files changed, 455 insertions(+), 1 deletion(-) create mode 100644 review-suite/fixtures/missing-simplification-requirements/expected.json create mode 100644 review-suite/fixtures/missing-simplification-requirements/packet.json create mode 100644 review-suite/fixtures/speculative-backfill/expected.json create mode 100644 review-suite/fixtures/speculative-backfill/packet.json create mode 100644 skills/review-solution-simplicity/SKILL.md create mode 100644 skills/review-solution-simplicity/agents/openai.yaml create mode 100644 skills/review-solution-simplicity/evals/standalone-provider-framework/candidate.diff create mode 100644 skills/review-solution-simplicity/evals/standalone-provider-framework/prompt.md create mode 100644 skills/review-solution-simplicity/evals/standalone-provider-framework/repository-evidence.md create mode 100644 skills/review-solution-simplicity/evals/standalone-provider-framework/result.json create mode 100644 skills/review-solution-simplicity/evals/standalone-provider-framework/ticket.md create mode 100644 skills/review-solution-simplicity/evals/standalone-provider-framework/validation.md create mode 100644 skills/review-solution-simplicity/references/solution-simplicity-rubric.md create mode 100644 skills/review-solution-simplicity/scripts/tests/test_skill_contract.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4429a68..5b1bfbc 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 whole-solution simplicity review - feat: add goal-first correctness review + (33feab3570363f8bf0d24ed4295495dc05fa3abf) - feat: define shared code review contracts (5600132585c502b21434a938e0319ba58521ee67) - feat: add epic sequence implementation skill diff --git a/README.md b/README.md index 48f208f..87d02a5 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,8 @@ Current skills: deterministic chain of smaller, reviewable changesets and GitHub PRs - `skills/review-correctness` — find material behavioral, security, 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 ## Quick Start diff --git a/review-suite/fixtures/imagined-machinery/expected.json b/review-suite/fixtures/imagined-machinery/expected.json index a722409..cdee87b 100644 --- a/review-suite/fixtures/imagined-machinery/expected.json +++ b/review-suite/fixtures/imagined-machinery/expected.json @@ -6,7 +6,7 @@ "findings": [{ "id": "solution-simplicity.remove-provider-framework", "lens": "solution_simplicity", - "severity": "strong_recommendation", + "severity": "blocking", "confidence": "high", "rule": "Remote providers and an extension API are explicit non-goals.", "evidence": [{"location": "export.py:1", "detail": "A registry, factory, provider interface, and selection parameter exist only for unspecified providers."}], diff --git a/review-suite/fixtures/manifest.json b/review-suite/fixtures/manifest.json index a0fca44..6c4b32c 100644 --- a/review-suite/fixtures/manifest.json +++ b/review-suite/fixtures/manifest.json @@ -6,6 +6,8 @@ {"name": "duplicated-policy", "packet_valid": true}, {"name": "imagined-machinery", "packet_valid": true}, {"name": "necessary-complexity", "packet_valid": true}, + {"name": "speculative-backfill", "packet_valid": true}, + {"name": "missing-simplification-requirements", "packet_valid": true}, {"name": "clean-change", "packet_valid": true}, {"name": "missing-evidence", "packet_valid": false}, {"name": "unrelated-base-drift", "packet_valid": true} diff --git a/review-suite/fixtures/missing-simplification-requirements/expected.json b/review-suite/fixtures/missing-simplification-requirements/expected.json new file mode 100644 index 0000000..1617b4c --- /dev/null +++ b/review-suite/fixtures/missing-simplification-requirements/expected.json @@ -0,0 +1,10 @@ +{ + "schema_version": "1.0", + "lens": "solution_simplicity", + "candidate": {"head_sha": "8686868686868686868686868686868686868686", "comparison_base_sha": "9696969696969696969696969696969696969696"}, + "verdict": "blocked", + "findings": [], + "blocking_reasons": ["The deployment topology, available durable storage primitive, and required retention and availability are missing, so no smaller design can be shown to satisfy retrieval after worker exit."], + "validation_limitations": ["Passing fake-backend tests do not resolve which persistence behavior is required in the real deployment."], + "next_action": "Decide the deployment storage boundary and durability requirements, then compare the candidate with the smallest repository-supported implementation." +} diff --git a/review-suite/fixtures/missing-simplification-requirements/packet.json b/review-suite/fixtures/missing-simplification-requirements/packet.json new file mode 100644 index 0000000..501c9f2 --- /dev/null +++ b/review-suite/fixtures/missing-simplification-requirements/packet.json @@ -0,0 +1,29 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/artifacts", "base_branch": "main"}, + "candidate": { + "head_sha": "8686868686868686868686868686868686868686", + "comparison_base_sha": "9696969696969696969696969696969696969696", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/storage.py b/storage.py\nnew file mode 100644\n--- /dev/null\n+++ b/storage.py\n@@ -0,0 +1,13 @@\n+class ArtifactStore:\n+ def __init__(self, backend):\n+ self.backend = backend\n+\n+ def put(self, key, value):\n+ return self.backend.put(key, value)\n+\n+def build_store(config):\n+ if config.backend == 'filesystem':\n+ return ArtifactStore(FileBackend(config.path))\n+ if config.backend == 'bucket':\n+ return ArtifactStore(BucketBackend(config.bucket))\n+ raise UnknownBackend(config.backend)\n" + } + }, + "change_contract": { + "goal": "Keep generated artifacts after the worker process exits.", + "acceptance_criteria": ["A generated artifact can be retrieved by key after its creating worker exits."], + "non_goals": [], + "preserved_behaviors": ["Artifact bytes remain unchanged."] + }, + "sources": { + "repository_instructions": [], + "named_documents": [{"label": "Deployment decision", "location": "DESIGN.md", "summary": "The deployment topology and durable storage service are undecided."}], + "nearby_patterns": [] + }, + "validation": [ + {"name": "storage tests", "command": "pytest tests/test_storage.py", "scope": "focused", "status": "passed", "result": "6 passed with fake backends"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "19 passed"} + ], + "context": {"operational": ["It is unknown whether workers share a filesystem, which storage service is available, and what retention or availability is required."]} +} diff --git a/review-suite/fixtures/speculative-backfill/expected.json b/review-suite/fixtures/speculative-backfill/expected.json new file mode 100644 index 0000000..da9eadc --- /dev/null +++ b/review-suite/fixtures/speculative-backfill/expected.json @@ -0,0 +1,24 @@ +{ + "schema_version": "1.0", + "lens": "solution_simplicity", + "candidate": {"head_sha": "8585858585858585858585858585858585858585", "comparison_base_sha": "9595959595959595959595959595959595959595"}, + "verdict": "changes_required", + "findings": [{ + "id": "solution-simplicity.remove-speculative-timezone-migration", + "lens": "solution_simplicity", + "severity": "blocking", + "confidence": "high", + "rule": "Historical migration and legacy-field compatibility are explicit non-goals, and no deployed data or callers exist.", + "evidence": [ + {"location": "timezones.py:1", "detail": "The candidate adds a feature flag, dual reads, dual writes, and a backfill solely for legacy timezone state."}, + {"location": "README.md", "detail": "The settings service is unreleased and has no production records to migrate."} + ], + "concern": "The solution carries a migration protocol for historical state that cannot exist.", + "impact": "It adds configuration, duplicate fields, a backfill lifecycle, and divergent read paths without satisfying any current requirement.", + "proposed_change": "Store one timezone field directly on new settings records and default it to UTC when absent; remove the flag, legacy field path, dual write, and backfill.", + "expected_effect": "Preserve all required timezone behavior while eliminating the migration state, compatibility branch, and duplicate source of truth.", + "location": "timezones.py:1" + }], + "blocking_reasons": [], + "next_action": "Replace the speculative migration layer with the direct timezone field before detailed review continues." +} diff --git a/review-suite/fixtures/speculative-backfill/packet.json b/review-suite/fixtures/speculative-backfill/packet.json new file mode 100644 index 0000000..0a65532 --- /dev/null +++ b/review-suite/fixtures/speculative-backfill/packet.json @@ -0,0 +1,29 @@ +{ + "schema_version": "1.0", + "repository": {"identity": "example/settings", "base_branch": "main"}, + "candidate": { + "head_sha": "8585858585858585858585858585858585858585", + "comparison_base_sha": "9595959595959595959595959595959595959595", + "diff": { + "format": "unified_diff", + "complete": true, + "content": "diff --git a/timezones.py b/timezones.py\nnew file mode 100644\n--- /dev/null\n+++ b/timezones.py\n@@ -0,0 +1,17 @@\n+class TimezoneMigration:\n+ def read(self, account):\n+ if flags.enabled('timezone_v2') and account.timezone_v2:\n+ return account.timezone_v2\n+ return account.legacy_timezone or 'UTC'\n+\n+ def write(self, account, timezone):\n+ account.timezone_v2 = timezone\n+ account.legacy_timezone = timezone\n+\n+ def backfill(self, accounts):\n+ for account in accounts:\n+ if not account.timezone_v2:\n+ account.timezone_v2 = account.legacy_timezone or 'UTC'\n+\n+def timezone_for(account):\n+ return TimezoneMigration().read(account)\n" + } + }, + "change_contract": { + "goal": "Store a timezone on newly created account settings.", + "acceptance_criteria": ["New account settings store and return the selected timezone."], + "non_goals": ["Migrate historical account settings.", "Support a legacy timezone field."], + "preserved_behaviors": ["UTC remains the default when a timezone is not selected."] + }, + "sources": { + "repository_instructions": [], + "named_documents": [{"label": "Release status", "location": "README.md", "summary": "The settings table and service have not been deployed and contain no production data."}], + "nearby_patterns": [{"label": "Account settings", "location": "settings.py", "summary": "New fields are stored directly on the unreleased settings record."}] + }, + "validation": [ + {"name": "timezone tests", "command": "pytest tests/test_timezones.py", "scope": "focused", "status": "passed", "result": "5 passed"}, + {"name": "full tests", "command": "pytest", "scope": "full", "status": "passed", "result": "21 passed"} + ], + "context": {"data": ["No account settings records exist outside test fixtures."], "compatibility": ["There are no released callers or stored legacy timezone values."]} +} diff --git a/skills/review-solution-simplicity/SKILL.md b/skills/review-solution-simplicity/SKILL.md new file mode 100644 index 0000000..22e0282 --- /dev/null +++ b/skills/review-solution-simplicity/SKILL.md @@ -0,0 +1,97 @@ +--- +name: review-solution-simplicity +description: Review a code change for whole-solution over-engineering by mapping its major mechanisms to stated requirements and proposing a materially smaller requirement-complete design. Use for architecture-level simplicity review of a PR, branch, or patch, either from raw ticket and repository evidence or from the shared review packet. Preserve justified safety and operational complexity, return only the shared result shape, and never modify the candidate. +--- + +# Review Solution Simplicity + +Determine whether the candidate's implementation strategy is materially larger +than its real problem. Review only; leave redesign and workflow mutations 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 solution-simplicity rubric](references/solution-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 shared review packet before reviewing it. Convert missing + essential evidence into a conforming `blocked` result. +- From raw evidence, establish repository and candidate identity, the complete + diff, observable goal, acceptance criteria, explicit non-goals, preserved + behavior, applicable repository sources, and exact validation results before + judging the design. +- Do not infer product requirements, compatibility promises, operational + constraints, or historical data from the implementation. Return `blocked` when + a requirement-complete comparison depends on a missing decision. +- Bind the result to the captured candidate and follow the shared base-drift + rules. + +## Compare whole solutions + +1. Restate the observable change contract without implementation terminology. +2. Inventory the candidate's major mechanisms: services, abstractions, states, + data models, compatibility paths, queues, caches, frameworks, migrations, + configuration, repair logic, and operational machinery. +3. Map each mechanism to a stated requirement, verified invariant, repository + architecture rule, or evidenced current operational constraint. +4. Challenge only unsupported or disproportionate mechanisms. +5. Construct the smallest concrete alternative that still satisfies every real + requirement and preserves required failure semantics. +6. Compare concepts, states, branches, ownership boundaries, migration and + operational burden, and failure modes. Do not use line count as the measure. +7. Report a change only when the alternative is specific and + requirement-complete. + +Correctness, security, concurrency, migration, compatibility, rollout, and +recovery requirements override simplicity. Treat the signals in the rubric as +questions, not automatic findings. + +## Apply the finding threshold + +Every finding must identify the unsupported mechanism, cite the requirements and +repository evidence used for comparison, describe a concrete smaller design, +show how it preserves required behavior and failure semantics, and name the +material reduction in concepts, states, ownership, or operational burden. + +- Use `blocking` only when the design violates ticket scope or required + architecture, or creates a demonstrated correctness or operational hazard. +- Use `strong_recommendation` for a clear, tractable, requirement-complete + simplification with material cognitive or operational value. +- Use `defer` only for an evidenced concern outside the active ticket or + awaiting a named decision. +- Omit aesthetic disagreement, vague requests to simplify, numerical complexity + rules, speculative product direction, and alternatives that merely relocate + complexity. + +Do not perform line-level DRY, naming, formatting, or helper-extraction review. +Do not duplicate the correctness lens or remove explicit tests to shrink a +change. + +## Return the shared result + +Return only JSON conforming to +`../../review-suite/contracts/review-result.schema.json` with lens +`solution_simplicity`. + +- Return `clean` when every major mechanism is justified and no gating finding + remains. +- Return `changes_required` when a blocking or strong-recommendation finding + remains. +- Return `blocked` when missing requirements or decisions prevent 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 the alternative, 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-solution-simplicity/agents/openai.yaml b/skills/review-solution-simplicity/agents/openai.yaml new file mode 100644 index 0000000..5a3ab40 --- /dev/null +++ b/skills/review-solution-simplicity/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Review Solution Simplicity" + short_description: "Find requirement-free solution complexity" + default_prompt: "Use $review-solution-simplicity to find a materially smaller requirement-complete design for this change." diff --git a/skills/review-solution-simplicity/evals/standalone-provider-framework/candidate.diff b/skills/review-solution-simplicity/evals/standalone-provider-framework/candidate.diff new file mode 100644 index 0000000..3f0cbda --- /dev/null +++ b/skills/review-solution-simplicity/evals/standalone-provider-framework/candidate.diff @@ -0,0 +1,29 @@ +diff --git a/export.py b/export.py +new file mode 100644 +--- /dev/null ++++ b/export.py +@@ -0,0 +1,24 @@ ++from pathlib import Path ++ ++class ExportProviderRegistry: ++ def __init__(self): ++ self.providers = {} ++ ++ def register(self, name, factory): ++ self.providers[name] = factory ++ ++ def create(self, name): ++ return self.providers[name]() ++ ++class LocalFileProvider: ++ def write(self, path, value): ++ Path(path).write_text(value) ++ ++def build_registry(): ++ registry = ExportProviderRegistry() ++ registry.register('local', LocalFileProvider) ++ return registry ++ ++def export(path, value, provider='local'): ++ registry = build_registry() ++ registry.create(provider).write(path, value) diff --git a/skills/review-solution-simplicity/evals/standalone-provider-framework/prompt.md b/skills/review-solution-simplicity/evals/standalone-provider-framework/prompt.md new file mode 100644 index 0000000..8a19db3 --- /dev/null +++ b/skills/review-solution-simplicity/evals/standalone-provider-framework/prompt.md @@ -0,0 +1,6 @@ +Use the `review-solution-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 review conclusion. Reconstruct the observable change contract before +comparing the whole solution. Return only the shared review result JSON and do +not modify files or repository state. diff --git a/skills/review-solution-simplicity/evals/standalone-provider-framework/repository-evidence.md b/skills/review-solution-simplicity/evals/standalone-provider-framework/repository-evidence.md new file mode 100644 index 0000000..7cdcd21 --- /dev/null +++ b/skills/review-solution-simplicity/evals/standalone-provider-framework/repository-evidence.md @@ -0,0 +1,10 @@ +# Repository evidence + +Repository: `example/exporter` Base branch: `main` Candidate head: +`8787878787878787878787878787878787878787` Comparison base: +`9797979797979797979797979797979797979797` + +`AGENTS.md` says to add extension surfaces only for current consumers. The +repository has no provider registry or remote-storage dependency. Nearby local +output code calls `Path(path).write_text(value)` directly and lets filesystem +errors propagate to the caller. diff --git a/skills/review-solution-simplicity/evals/standalone-provider-framework/result.json b/skills/review-solution-simplicity/evals/standalone-provider-framework/result.json new file mode 100644 index 0000000..1dd454b --- /dev/null +++ b/skills/review-solution-simplicity/evals/standalone-provider-framework/result.json @@ -0,0 +1,38 @@ +{ + "schema_version": "1.0", + "lens": "solution_simplicity", + "candidate": { + "head_sha": "8787878787878787878787878787878787878787", + "comparison_base_sha": "9797979797979797979797979797979797979797" + }, + "verdict": "changes_required", + "findings": [ + { + "id": "solution-simplicity.provider-framework", + "lens": "solution_simplicity", + "severity": "blocking", + "confidence": "high", + "rule": "Do not add extension surfaces without current consumers or introduce a public provider-extension API when the ticket explicitly excludes one.", + "evidence": [ + { + "location": "ticket.md:3-7", + "detail": "The required behavior is a local export(path, value) operation; remote providers and a public provider-extension API are explicit non-goals, and existing filesystem exception behavior is sufficient." + }, + { + "location": "repository-evidence.md:7-11", + "detail": "Repository instructions allow extension surfaces only for current consumers, no provider registry or remote-storage dependency exists, and nearby code writes through Path(path).write_text(value) directly." + }, + { + "location": "candidate.diff:6-29", + "detail": "The candidate adds a registry, registration and factory protocol, provider class, registry builder, provider-name dispatch, and a new provider parameter despite having only one local implementation." + } + ], + "concern": "The candidate turns one local filesystem operation into an unsupported provider framework and exposes provider selection beyond the requested API.", + "impact": "This breaches the ticket's explicit scope and repository architecture rule while adding registry ownership, factory indirection, dispatch state, and additional failure modes such as unknown provider names, none of which serve a current consumer.", + "proposed_change": "Replace ExportProviderRegistry, LocalFileProvider, build_registry, and the provider parameter with `def export(path, value): Path(path).write_text(value)`. Keep the focused tests for successful output and propagated filesystem exceptions; report generation remains untouched.", + "expected_effect": "The required local export and existing filesystem failure semantics remain unchanged while eliminating the extension contract, registry lifecycle, provider dispatch branch, and extra public concepts." + } + ], + "blocking_reasons": [], + "next_action": "Replace the provider framework with the direct local write operation and rerun the focused and full test suites." +} diff --git a/skills/review-solution-simplicity/evals/standalone-provider-framework/ticket.md b/skills/review-solution-simplicity/evals/standalone-provider-framework/ticket.md new file mode 100644 index 0000000..ba5d658 --- /dev/null +++ b/skills/review-solution-simplicity/evals/standalone-provider-framework/ticket.md @@ -0,0 +1,7 @@ +# Export a report to a local file + +Add an `export(path, value)` operation that writes a generated report to the +caller-provided local path. Report generation must remain unchanged. + +Remote storage providers and a public provider-extension API are outside this +ticket. The existing filesystem exception behavior is sufficient. diff --git a/skills/review-solution-simplicity/evals/standalone-provider-framework/validation.md b/skills/review-solution-simplicity/evals/standalone-provider-framework/validation.md new file mode 100644 index 0000000..e2360d1 --- /dev/null +++ b/skills/review-solution-simplicity/evals/standalone-provider-framework/validation.md @@ -0,0 +1,7 @@ +# Validation evidence + +- Focused: `pytest tests/test_export.py` passed, 3 tests covering the local + export result and filesystem errors. +- Full: `pytest` passed, 8 tests. + +The candidate diff is complete and validation did not modify tracked files. diff --git a/skills/review-solution-simplicity/references/solution-simplicity-rubric.md b/skills/review-solution-simplicity/references/solution-simplicity-rubric.md new file mode 100644 index 0000000..8f08cb0 --- /dev/null +++ b/skills/review-solution-simplicity/references/solution-simplicity-rubric.md @@ -0,0 +1,82 @@ +# Solution-simplicity rubric + +Use this rubric after establishing the ticket's observable contract and the +candidate's complete boundary. Inspect only dimensions that apply. + +## Map mechanisms to evidence + +For each major mechanism, record both its cost and its justification. + +| Mechanism | Typical cost | Sufficient justification | +| ------------------------------- | ---------------------------------------------- | --------------------------------------------------------- | +| Service or module boundary | ownership, calls, failure surface | repository boundary or distinct lifecycle | +| Interface, registry, or plugin | indirection, configuration, extension contract | multiple real consumers or required public extension | +| State machine | states, transitions, recovery paths | evidenced lifecycle and illegal-transition protection | +| Queue, retry, or reconciler | delivery semantics, operations, observability | real asynchronous or partial-failure behavior | +| Cache or duplicate store | invalidation, consistency, repair | measured need or required availability boundary | +| Migration or compatibility path | dual behavior, cleanup, rollout burden | existing data, callers, or explicit compatibility promise | +| Configuration or feature flag | combinations, ownership, retirement | documented rollout or user-selectable behavior | +| Generic framework | concepts and future maintenance | more than one current use or explicit platform goal | + +Do not treat an unsupported mechanism as unnecessary until the repository's +instructions, named design documents, nearby patterns, and operational context +have been checked. + +## Look for disproportionate machinery + +Ask whether the candidate introduces: + +- an abstraction with one speculative consumer; +- a generic framework for one narrow operation; +- compatibility handling for callers or data that do not exist; +- a pre-release migration or backfill with no historical state; +- extension points or configuration without a stated need; +- retry, reconciliation, or state beyond the evidenced failure model; +- another source of truth for a future possibility; +- custom infrastructure where an established repository primitive already meets + the contract; +- indirection that moves complexity without removing it; or +- platform work embedded in a PR-sized feature. + +These are investigation prompts. A finding still needs a smaller, +requirement-complete alternative. + +## Protect required complexity + +Preserve machinery that enforces: + +- ordering, uniqueness, atomicity, or fencing; +- concurrency safety and idempotency; +- authentication, authorization, or data protection; +- partial-failure handling, bounded retry, cancellation, or recovery; +- migration of real schema or persisted data; +- documented public compatibility; +- required rollout controls and observability; or +- a demonstrated repository architecture boundary. + +Do not replace explicit safety semantics with an informal sequence of operations +or assume success-only behavior. + +## Construct the smaller alternative + +A sufficient alternative names the concrete repository primitive or direct flow, +identifies which candidate mechanisms disappear, preserves every acceptance +criterion and invariant, and explains its failure behavior. Reject an +alternative that depends on a new unstated assumption. + +Measure the reduction in human and operational terms: fewer concepts, states, +branches, owners, configuration combinations, migration phases, failure modes, +or sources of truth. Fewer lines alone are not evidence. + +## Choose severity + +- `blocking`: the unsupported design breaches explicit scope or architecture, or + creates a demonstrated correctness or operational hazard. +- `strong_recommendation`: the candidate works, but a tractable alternative + removes material unjustified machinery while preserving the contract. +- `defer`: the concern is real but belongs to another ticket or awaits a named + decision. + +If a product, architecture, compatibility, data, or operational decision is +required to know which design is sufficient, return `blocked` instead of +inventing the answer. diff --git a/skills/review-solution-simplicity/scripts/tests/test_skill_contract.py b/skills/review-solution-simplicity/scripts/tests/test_skill_contract.py new file mode 100644 index 0000000..a455de2 --- /dev/null +++ b/skills/review-solution-simplicity/scripts/tests/test_skill_contract.py @@ -0,0 +1,76 @@ +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.assertNotIn("code-review-pro", skill) + + def test_solution_simplicity_fixture_results_conform(self): + expectations = { + "imagined-machinery": "changes_required", + "necessary-complexity": "clean", + "speculative-backfill": "changes_required", + "missing-simplification-requirements": "blocked", + } + 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("solution_simplicity", result["lens"]) + self.assertEqual(verdict, result["verdict"]) + + def test_standalone_eval_reconstructs_contract_from_raw_evidence(self): + evaluation = SKILL_ROOT / "evals" / "standalone-provider-framework" + 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("solution_simplicity", result["lens"]) + self.assertEqual("changes_required", result["verdict"]) + self.assertEqual( + "8787878787878787878787878787878787878787", + result["candidate"]["head_sha"], + ) + self.assertTrue( + any("provider-framework" in finding["id"] for finding in result["findings"]) + ) + + +if __name__ == "__main__": + unittest.main()