Add review-solution-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 whole-solution simplicity review
 - feat: add goal-first correctness review
+  (33feab3570363f8bf0d24ed4295495dc05fa3abf)
 - feat: define shared code review contracts
   (5600132585c502b21434a938e0319ba58521ee67)
 - feat: add epic sequence implementation skill

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
 

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."}],

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}

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."
+}

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."]}
+}

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."
+}

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."]}
+}

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.

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."

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)

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.

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.

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."
+}

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.

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.