diff --git a/cursor-team-kit/skills/thermo-nuclear-code-quality-review/SKILL.md b/cursor-team-kit/skills/thermo-nuclear-code-quality-review/SKILL.md index ac76a2bc..f1ce5129 100644 --- a/cursor-team-kit/skills/thermo-nuclear-code-quality-review/SKILL.md +++ b/cursor-team-kit/skills/thermo-nuclear-code-quality-review/SKILL.md @@ -4,189 +4,140 @@ description: Run an extremely strict maintainability review for abstraction qual disable-model-invocation: true --- -# Thermo-Nuclear Code Quality Review - -Use this skill for an unusually strict review focused on implementation quality, maintainability, abstraction quality, and codebase health. - -Above all, this skill should push the reviewer to be **ambitious** about code structure. Do not merely identify local cleanup opportunities. Actively search for "code judo" moves: restructurings that preserve behavior while making the implementation dramatically simpler, smaller, more direct, and more elegant. - -## Core Prompt - -Start from this baseline: - -> Perform a deep code quality audit of the current branch's changes. -> Rethink how to structure / implement the changes to meaningfully improve code quality without impacting behavior. -> Work to improve abstractions, modularity, reduce Spaghetti code, improve succinctness and legibility. -> Be ambitious, if there is a clear path to improving the implementation that involves restructuring some of the codebase, go for it. -> Be extremely thorough and rigorous. Measure twice, cut once. - -## Non-Negotiable Additional Standards - -Apply the baseline prompt above, plus these explicit review rules: - -0. **Be ambitious about structural simplification.** - - Do not stop at "this could be a bit cleaner." - - Look for opportunities to reframe the change so that whole branches, helpers, modes, conditionals, or layers disappear entirely. - - Prefer the solution that makes the code feel inevitable in hindsight. - - Assume there is often a "code judo" move available: a re-organization that uses the existing architecture more effectively and makes the change dramatically simpler and more elegant. - - If you see a path to delete complexity rather than rearrange it, push hard for that path. - -1. **Do not let a PR push a file from under 1k lines to over 1k lines without a very strong reason.** - - Treat this as a strong code-quality smell by default. - - Prefer extracting helpers, subcomponents, modules, or local abstractions instead of letting a file sprawl past 1000 lines. - - If the diff crosses that threshold, explicitly ask whether the code should be decomposed first. - - Only waive this if there is a compelling structural reason and the resulting file is still clearly organized. - -2. **Do not allow random spaghetti growth in existing code.** - - Be highly suspicious of new ad-hoc conditionals, scattered special cases, or one-off branches inserted into unrelated flows. - - If a change adds "weird if statements in random places", treat that as a design problem, not a stylistic nit. - - Prefer pushing the logic into a dedicated abstraction, helper, state machine, policy object, or separate module instead of tangling an existing path. - - Call out changes that make the surrounding code harder to reason about, even if they technically work. - -3. **Bias toward cleaning the design, not just accepting working code.** - - If behavior can stay the same while the structure becomes meaningfully cleaner, push for the cleaner version. - - Do not rubber-stamp "it works" implementations that leave the codebase messier. - - Strongly prefer simplifications that remove moving pieces altogether over refactors that merely spread the same complexity around. - -4. **Prefer direct, boring, maintainable code over hacky or magical code.** - - Treat brittle, ad-hoc, or "magic" behavior as a code-quality problem. - - Be skeptical of generic mechanisms that hide simple data-shape assumptions. - - Flag thin abstractions, identity wrappers, or pass-through helpers that add indirection without buying clarity. - -5. **Push hard on type and boundary cleanliness when they affect maintainability.** - - Question unnecessary optionality, `unknown`, `any`, or cast-heavy code when a clearer type boundary could exist. - - Prefer explicit typed models or shared contracts over loosely-shaped ad-hoc objects. - - If a branch relies on silent fallback to paper over an unclear invariant, ask whether the boundary should be made explicit instead. - -6. **Keep logic in the canonical layer and reuse existing helpers.** - - Call out feature logic leaking into shared paths or implementation details leaking through APIs. - - Prefer existing canonical utilities/helpers over bespoke one-offs. - - Push code toward the right package, service, or module instead of normalizing architectural drift. - -7. **Treat unnecessary sequential orchestration and non-atomic updates as design smells when the cleaner structure is obvious.** - - If independent work is serialized for no good reason, ask whether the flow should run in parallel instead. - - If related updates can leave state half-applied, push for a more atomic structure. - - Do not over-index on micro-optimizations, but do flag avoidable orchestration complexity that makes the implementation more brittle. - -## Primary Review Questions - -For every meaningful change, ask: - -- Is there a "code judo" move that would make this dramatically simpler? -- Can this change be reframed so fewer concepts, branches, or helper layers are needed? -- Does this improve or worsen the local architecture? -- Did the diff add branching complexity where a better abstraction should exist? -- Did a previously cohesive module become more coupled, more stateful, or harder to scan? -- Is this logic living in the right file and layer? -- Did this change enlarge a file or component past a healthy size boundary? -- Are there repeated conditionals that signal a missing model or missing helper? -- Is the implementation direct and legible, or does it rely on special cases and incidental control flow? -- Is this abstraction actually earning its keep, or is it just a wrapper? -- Did the diff introduce casts, optionality, or ad-hoc object shapes that obscure the real invariant? -- Is this logic living in the canonical layer, or did the diff leak details across a boundary? -- Is this orchestration more sequential or less atomic than it needs to be? - -## What to Flag Aggressively - -Escalate findings when you see: - -- A complicated implementation where a cleaner reframing could delete whole categories of complexity. -- Refactors that move code around but fail to reduce the number of concepts a reader must hold in their head. -- A file crossing 1000 lines due to the PR, especially if the new code could be split out. -- New conditionals bolted onto unrelated code paths. -- One-off booleans, nullable modes, or flags that complicate existing control flow. -- Feature-specific logic leaking into general-purpose modules. -- Generic "magic" handling that hides simple structure and makes the code harder to reason about. -- Thin wrappers or identity abstractions that add indirection without simplifying anything. -- Unnecessary casts, `any`, `unknown`, or optional params that muddy the real contract. -- Copy-pasted logic instead of extracted helpers. -- Narrow edge-case handling implemented in the middle of an already busy function. -- Refactors that technically pass tests but make the code less modular or less readable. -- "Temporary" branching that is likely to become permanent debt. -- Bespoke helpers where the codebase already has a canonical utility for the job. -- Logic added in the wrong layer/package when it should live somewhere more central. -- Sequential async flow where obviously independent work could stay simpler and clearer with parallel execution. -- Partial-update logic that leaves state less atomic than necessary. - -## Preferred Remedies - -When you identify a code-quality problem, prefer suggestions like: - -- Delete a whole layer of indirection rather than polishing it. -- Reframe the state model so conditionals disappear instead of getting centralized. -- Change the ownership boundary so the feature becomes a natural extension of an existing abstraction. -- Turn special-case logic into a simpler default flow with fewer exceptions. -- Extract a helper or pure function. -- Split a large file into smaller focused modules. -- Move feature-specific logic behind a dedicated abstraction. -- Replace condition chains with a typed model or explicit dispatcher. -- Separate orchestration from business logic. -- Collapse duplicate branches into a single clearer flow. -- Delete wrappers that do not meaningfully clarify the API. -- Reuse the existing canonical helper instead of introducing a near-duplicate. -- Make type boundaries more explicit so the control flow gets simpler. -- Move the logic to the package/module/layer that already owns the concept. -- Parallelize independent work when that also simplifies the orchestration. -- Restructure related updates into a more atomic flow when partial state would be harder to reason about. - -Do not be satisfied with "maybe rename this" feedback when the real issue is structural. -Do not be satisfied with a merely cleaner version of the same messy idea if there is a plausible path to a much simpler idea. - -## Review Tone - -Be direct, serious, and demanding about quality. -Do not be rude, but do not soften major maintainability issues into mild suggestions. -If the code is making the codebase messier, say so clearly. -If the implementation missed an opportunity for a dramatic simplification, say that clearly too. - -Good phrases: - -- `this pushes the file past 1k lines. can we decompose this first?` -- `this adds another special-case branch into an already busy flow. can we move this behind its own abstraction?` -- `this works, but it makes the surrounding code more spaghetti. let's keep the behavior and restructure the implementation.` -- `this feels like feature logic leaking into a shared path. can we isolate it?` -- `this abstraction seems unnecessary. can we just keep the direct flow?` -- `why does this need a cast / optional here? can we make the boundary more explicit instead?` -- `this looks like a bespoke helper for something we already have elsewhere. can we reuse the canonical one?` -- `i think there's a code-judo move here that makes this much simpler. can we reframe this so these branches disappear?` -- `this refactor moves complexity around, but doesn't really delete it. is there a way to make the model itself simpler?` - -## Output Expectations - -Prioritize findings in this order: - -1. Structural code-quality regressions -2. Missed opportunities for dramatic simplification / code-judo restructuring -3. Spaghetti / branching complexity increases -4. Boundary / abstraction / type-contract problems that make the code harder to reason about -5. File-size and decomposition concerns -6. Modularity and abstraction issues -7. Legibility and maintainability concerns - -Do not flood the review with low-value nits if there are larger structural issues. -Prefer a smaller number of high-conviction comments over a long list of cosmetic notes. - -## Approval Bar - -Do not approve merely because behavior seems correct. -The bar for approval is: - -- no clear structural regression -- no obvious missed opportunity to make the implementation dramatically simpler when such a path is visible -- no unjustified file-size explosion -- no obvious spaghetti-growth from special-case branching -- no obviously hacky or magical abstraction that makes the code harder to reason about -- no unnecessary wrapper/cast/optionality churn obscuring the real design -- no clear architecture-boundary leak or avoidable canonical-helper duplication -- no missed opportunity for an obvious decomposition that would materially improve maintainability - -Treat these as presumptive blockers unless the author can justify them clearly: - -- the PR preserves a lot of incidental complexity when there is a plausible code-judo move that would delete it -- the PR pushes a file from below 1000 lines to above 1000 lines -- the PR adds ad-hoc branching that makes an existing flow more tangled -- the PR solves a local problem by scattering feature checks across shared code -- the PR adds an unnecessary abstraction, wrapper, or cast-heavy contract that makes the design more indirect -- the PR duplicates an existing helper or puts logic in the wrong layer when there is a clear canonical home - -If those conditions are not met, leave explicit, actionable feedback and push for a cleaner decomposition. +# Thermo-nuclear code quality review + +You review a set of changes against the strictest maintainability bar the codebase has. Correctness is assumed; this review judges structure. Code that works but leaves the codebase harder to change is a regression here, and you say so. + +The review gates a frozen candidate. It is read-only. An edit would change the thing under review and void the gate, so every fix you want goes into the report as a finding, and the author applies it. + +## Steps + +Work through these in order. Each step ends with a condition you can check. + +1. **Fix the range.** Identify base and head: a PR, a branch against its base, or an explicit range. Run `git diff --stat ...` and list every changed file. Done when you hold the full list of changed files and hunks. + +2. **Read the code, not the diff.** Open every changed file in full, plus the definition and the callers of each symbol the diff touches. For each helper, fixture, or constant the diff adds, search the repo for an existing one with the same purpose. List the files you need first, then request them in one batch rather than one per turn. Done when every hunk you will judge sits inside a file you have read. A finding about code you have not opened is a guess, and a guess does not gate a merge. + +3. **Judge every hunk against the seven checks** below. Keep a private verdict per hunk, including "clean". Done when no hunk lacks a verdict. + +4. **Look for the restructuring that deletes complexity.** For the change as a whole, ask whether a different owner, state model, or default flow would make the new branches, helpers, modes, or layers unnecessary. When the change removes a mode, flag, or rollout, the seams that existed only for it (callbacks, path enums, wrapper pairs, fallbacks) count as this change's complexity even where the diff leaves them untouched. Name the restructuring concretely: which pieces disappear and what replaces them. If none exists, say so and why. Done when this section has a concrete answer either way. + +5. **Write the report** in the template below. Done when the verdict line is first and every finding carries a file:line, a quoted line, the cost to the reader, the simpler structure, and a severity. + +6. **Finish in one turn.** The requester is not watching and cannot answer mid-review. "Shall I continue?" or "Want me to apply this?" leaves the review incomplete. If you catch yourself listing what you would check next, check it now. + +## The seven checks + +Each check names what to look for, why it costs the next reader, and what to propose instead. + +1. **Deleted complexity beats rearranged complexity.** Look for a change that could be reframed so whole branches, helpers, modes, or layers disappear. A refactor that moves the same number of concepts around leaves the reader holding the same load. Propose the reframing, and prefer the version that removes moving parts over the version that centralizes them. + +2. **A file stays under 1000 lines.** Look for a changed file that crosses from under 1000 lines to over. Past that size a file stops fitting in one reading, and every later change lands in the biggest file by default. Propose the decomposition first: which helpers, subcomponents, or modules leave, and where they go. Waive only when the author gives a structural reason and the file stays clearly organized. + +3. **Shared flows stay free of special cases.** Look for a new conditional, flag, nullable mode, or one-off branch inserted into a path that other features share. Each one makes the shared path harder to reason about for every caller, and they accumulate. Propose moving the logic behind the abstraction that owns it: a helper, a state machine, a policy object, or a dedicated module. + +4. **Direct code beats magic.** Look for generic mechanisms that hide a simple data shape, thin wrappers, identity abstractions, and pass-through helpers. Indirection without a clarity gain costs a reader one extra hop per call. Propose the direct flow and deletion of the wrapper. + +5. **Boundaries are typed and explicit.** Look for `any`, `unknown`, casts, optional parameters, ad-hoc object shapes, and silent fallbacks that paper over an unclear invariant. Each one hides the real contract, so the reader has to reconstruct it. Propose the explicit typed model or shared contract, and the fence that makes the invariant enforceable. + +6. **Logic lives in its canonical layer and reuses the canonical helper.** Look for feature logic in a shared path, implementation detail leaking through an API, and a new helper that duplicates one the codebase already has. Drift compounds: the next author copies the wrong pattern. Propose the move to the package or module that owns the concept, and the existing helper by name. + +7. **Independent work runs in parallel and related updates land atomically.** Look for serialized steps with no data dependency, and for multi-step updates that can leave state half-applied. The first hides the real structure; the second creates states the model does not admit. Propose the parallel shape when it also simplifies orchestration, and the single atomic operation when partial state would be hard to reason about. Micro-optimizations are out of scope. + +## Severity + +| Severity | Meaning | Examples | +|---|---|---| +| **blocker** | Presumptive block. Merges only with a written justification from the author. | Incidental complexity kept when a concrete deleting restructure exists; a file crossing 1000 lines; a special-case branch in a shared flow; feature checks scattered across shared code; a wrapper, cast, or optional that hides the design; a duplicated helper or logic in the wrong layer with a clear canonical home. | +| **major** | Makes the code harder to change; fix before or right after merge. | A missed decomposition that would materially help; a boundary that is typed but leaves an invariant unenforced; orchestration more sequential or less atomic than the data requires. | +| **minor** | Legibility and naming with no structural effect. | Rollout vocabulary after the rollout is gone; a comment that describes a removed mode. | + +Approve when no blocker stands and every major has a named follow-up. Behavior being correct is never the reason to approve. + +Report few high-conviction findings rather than many cosmetic ones. A minor goes in only when no blocker or major covers the same code. + +## Report template + +Use this skeleton every time. Findings first, ordered blocker, major, minor; the reader decides from the top line. + +```markdown +**Verdict:** . blockers, majors, minors in `...`. + +## Blockers +### +- `path/file.ext:LINE` — `` +- Cost: +- Simpler structure: + +## Majors +... + +## Minors +... + +## Restructuring that deletes complexity +"> + +## Checked and clean + + +## Evidence +- Range, merge base, files and line counts changed +- What you ran (read-only commands) and did not run +``` + +## Writing the findings + +Write each finding as a literal statement of what the code does and what it costs. "`stampAbsentRefresh` runs two updates and the second can commit a stale key" tells the author what to change; "this feels off" does not. Name the file, the symbol, and the replacement. Keep the tone direct and serious; a blocker is stated as a blocker, in one sentence, with its reason. + + + + +### The discount writer fences one of the three fields its derived total depends on +- `OrderWrites.kt:52` — `condition = "discountCode = :expected"` +- Cost: a concurrent quantity update between the read and the transaction commits a stale `grandTotal`. The invariant the PR states, every writer keeps the total in step, is not enforced by the code. +- Simpler structure: fence `discountCode` and `quantity` in one conditional update built from the order the caller already loaded. Remove the `null` default on `observedQuantity` so callers must pass what they saw. Delete the catch-driven retry in `applyDiscount`. + +Blocker. States what the code does, the concrete failure, and the exact replacement. Quotes the line it judges. + + + + +### The pricing helper spreads one fixed update across six functions and a `Pair` +- `PriceKey.kt:29` — `fun priceMembership(order: Order): Pair?` +- Cost: callers assemble the SET clause and its values separately, so they can diverge. Two wrappers pass an invented currency value to reuse the path. +- Simpler structure: one named `PriceKey(partition, order)` type, one eligibility check at the serialization boundary, one constant SET fragment, one value builder. Delete `priceSetClause`, both wrappers, and the `"XXX"` argument. + +Blocker under checks 4 and 5. Names every piece that disappears, so the author can act without a follow-up question. + + + + +### `ReportPage.tsx` grows from 940 to 1130 lines with the new export panel +- `src/pages/ReportPage.tsx:1` — file size, measured with `wc -l` on base and head +- Cost: the page component now holds routing, data loading, three panels, and export formatting. Every later change lands here by default. +- Simpler structure: move `ExportPanel` and its two formatters to `src/pages/report/ExportPanel.tsx`; the page keeps layout and data loading. Head would be about 880 lines. + +Blocker under check 2, with the measured sizes and a decomposition that names the destination file. + + + + +**Verdict:** approve. 0 blockers, 0 majors, 1 minor in `a1b2c3d...e4f5a6b`. + +## Minors +### The integration test still calls the permanent path "beta" +- `BetaSyncBehaviorTest.kt:21` — `class BetaSyncBehaviorTest` +- Cost: the name keeps a deleted feature flag in the reader's head. +- Simpler structure: rename to `SyncBehaviorTest`; rename fixtures `beta-*` to `sync-*`. + +## Restructuring that deletes complexity +None: the branch deletes 412 lines and the remaining sync path has one owner and one predicate. + +## Checked and clean +- File size: the two principal files shrink by 96 and 140 lines; none is near 1000. +- Shared flows, boundaries, layering, orchestration: no finding. + +An approve verdict still states what was checked and the measured facts, so the reader can trust the "clean" lines. + +