thermo-nuclear-code-quality-review

name	thermo-nuclear-code-quality-review
description	An extremely strict, BEHAVIOR-PRESERVING whole-repo structural refactor pass across one or both Orbit repos — split giant files, flatten spaghetti conditionals, deepen thin abstractions, dedup cross-app logic into packages/shared, move logic to its canonical layer — under hard build/test/EF-model gates, delivered as one PR per repo with an Applied table + a complete Deferred ledger. Use ONLY when the user explicitly asks for a thermo-nuclear / nuclear / deep structural-quality pass. This rewrites code; it is not the read-only /audit-code-quality.
argument-hint	<both (default) | ui | api | path>
context	fork

Thermo-Nuclear Code-Quality Review

Input: $ARGUMENTS

Adapted at authoring time from cursor's thermo-nuclear-code-quality-review (https://github.com/cursor/plugins — the strict maintainability rubric in The standard below) and wrapped in the phased, coverage-contracted, hard-gated harness Orbit actually ran it under. That URL is the single WHY-with-URL the comment policy allows.

The one non-negotiable: every change is behavior-preserving. Structure only — never behavior, never UI, never an API/DTO contract, never the EF model/schema. If a refactor could change what the program does, it is out of scope for this skill. The gate in Phase 2 is not "tests pass," it is "behavior is provably identical."

Golden rule: deepen the codebase — more behavior behind a smaller, clearer interface — without moving a single observable byte. A change that merely relocates size (a 1k file becomes ten 100-line files that still do the same tangled thing) is reported honestly as relocation, not celebrated as simplification.

---

The standard — what "thermo-nuclear" flags

Strict, demanding, but never a nit machine. Lead with high-conviction structural wins; quote the exact pattern when you flag it. Ranked by leverage — fix in this order:

1. Structural regressions / missed dramatic simplifications — the "code-judo move": a reframing of the state model or data shape that deletes whole branches. The highest prize; hunt it first.
2. Spaghetti branching — special-case if/else ladders and flag soup that grow every time a case is added. Flatten with early returns, a lookup table, or polymorphism.
3. Boundary / abstraction clarity — thin wrappers that earn nothing (the deletion test: if removing the module makes complexity vanish, it was a pass-through), and "magical" abstractions that hide control flow. Deepen the keepers; delete the indirection.
4. Giant files — anything past ~1k lines, or a file doing many jobs. Split by responsibility into sibling files. (C# giants: partial class — same compiled type, zero behavior risk. TS giants: extract inline subcomponents/helpers to siblings, render tree and hook order unchanged.)
5. Cast / optionality churn — as/null! (C#) or as any/as unknown as X (TS) and needless ?-juggling that a better type or a single parse at the boundary removes.
6. Modularity & cross-app duplication — the same logic in apps/web and apps/mobile belongs in packages/shared; the same logic across handlers belongs in one helper. Dedup at the right level (root CLAUDE.md rule 10).
7. Logic in the wrong layer — business logic leaking into a controller/component/DTO instead of living in its canonical layer (domain entity, CQRS handler, shared util).

Remedies preferred: delete indirection · reframe the state model so branches collapse · extract a well-named pure helper · split a file by responsibility · lift duplication to packages/shared · move logic to its canonical layer · replace a cast tower with one boundary parse. Never introduce a new DI service, reflection, a new MediatR command, a new endpoint, or a behavior flag — those are behavior changes, not refactors.

---

Phase 0 — Baseline + binding coverage contract

Resolve scope from $ARGUMENTS (blank → both repos; ui/api/a path narrows it).

1. Capture the baseline BEFORE touching anything — on clean, up-to-date main:
   • api: dotnet build (0 errors), dotnet test (record the pass count), and dotnet ef migrations has-pending-model-changes (record: no pending model changes).
   • ui: per-workspace type-check / lint / test pass counts. Note known local-only noise (@sentry/*, @playwright/test absent locally fail there but pass in CI — confirm they don't reproduce on clean main before blaming a change).
2. Build the ranked inventory (verification-protocol §1). Fan out read-only Explore scouts, 3 concurrent, to inventory the worst offenders per repo (target the top ~20): largest files, deepest nesting, thinnest abstractions, most-duplicated logic, most layer-leakage — ranked by blast-radius × churn. This list is binding: by the end every entry is either fixed or in the Deferred ledger with a concrete, verified reason. There is no "quietly skipped" bucket — that is the failure this skill exists to prevent.

Read .claude/skills/_shared/verification-protocol.md — its coverage contract (§1) and Deferred ledger (§4) govern this run.

---

Phase 1 — Parallel behavior-preserving refactors

Fan out subagents, 3 concurrent, each owning non-overlapping inventory items, each in its own git worktree (isolation: worktree) so parallel edits never collide. Each subagent:

• Applies only the behavior-preserving techniques above (C# partial class + static helper classes; TS sibling extraction with identical render tree / hook order / public props). No new services, no reflection, no new commands/endpoints.
• Honors cross-platform parity (MANDATORY). A split in apps/web lands the mirror split in apps/mobile and vice-versa. Never claim web↔mobile symmetry without opening both files — a file that looks like a monolith on one platform is often already split on the other; verify, don't assume.
• Reports size honestly. If a giant file becomes many files that still carry the same tangled logic, say "relocated, not simplified" in the Applied table — do not dress relocation up as a code-judo win.

Subagents do the edits only; the orchestrator owns all git/build/test (a shared tree + parallel dotnet/turbo runs corrupt each other). Collect each subagent's diff summary.

---

Phase 2 — Hard verification gates (behavior preservation IS the gate)

Nothing merges until every gate is green. Run from the orchestrator, per touched repo:

• api: dotnet build (0 errors) → dotnet test (0 failures, count ≥ baseline) → dotnet ef migrations has-pending-model-changes (model byte-identical — no pending changes; a pending change means the refactor altered the schema → revert it).
• ui: per-workspace type-check + lint + test, each green and count ≥ baseline.
• Comment gate (orbit-api): ORBIT0001 (no-narration-comments) is silent in local dotnet build (analyzer Roslyn-version mismatch) but enforced in CI — grep changed C# for bare // lines that are not /// and carry no http(s):// URL before pushing, or CI fails on a locally-green build.
• Sonar note: a large relocation refactor will trip SonarCloud's "Coverage on New Code ≥80%" gate (relocated under-tested lines re-count as "new"). This is an attribution artifact, not a regression — resolve by admin-overriding that gate, never by writing ~100 view-component tests against the "test behavior not implementation" rule. Surface it to the user as a decision; don't silently scope-creep into mass test-writing.

A finding that survives all gates ships. Anything that can't be made green is reverted and moved to the Deferred ledger — a refactor is not worth a behavior risk.

---

Phase 3 — Deliver (one PR per repo)

A fresh branch off latest main per touched repo (chore/nuclear-review for orbit-api, chore/nuclear-review-ui for orbit-ui-mobile — never reuse a squash-merged branch). One PR per repo, cross-linked, with:

## Thermo-Nuclear Structural Review — {repo}

**Behavior-preserving** structural refactor. No behavior / UI / contract / schema change.

### Applied
| File(s) | Before → after | Technique | Honest note |
|---|---|---|---|
| ProcessUserChatCommand.cs | 1227 → 275 (+6 partials) | partial class | decomposed by responsibility |
| habit-list.tsx | 1888 → 1604 | sibling extraction | PARTIAL — residual is closure-bound render |

### Deferred (verification-protocol §4)
| Inventory item | Why not done | 
|---|---|
| db-context.cs | single cohesive config; splitting adds indirection for no leverage |
| ... every un-fixed inventory entry, with a concrete reason | |

### Verification
- api: build ✅ · tests {N}/{N} (baseline {B}) ✅ · EF model identical ✅
- ui: type-check ✅ · lint ✅ · tests ✅
- Sonar new-code-coverage gate: expected relocation artifact → override (see note)

Do not merge — hand the PRs to the user. Branch protection + squash-merge are theirs to click.

---

Guardrails — do NOT

• Change behavior, UI, an API/DTO contract, or the EF model. If you can't prove a change is invisible to callers and to the database, it is out of scope — revert + defer.
• Fix two easy files and stop. The Phase-0 inventory is binding; every entry is fixed or in the Deferred ledger.
• Silently omit a big offender. A hard file left alone is a Deferred-ledger line with a reason, never an unstated gap.
• Claim parity without opening both files. Verify web↔mobile symmetry by reading each.
• Report relocation as simplification. Honest Applied notes only.
• Let a subagent run git/build/test on the shared tree — the orchestrator owns those.
• Add a new abstraction for its own sake. Deepen existing modules; the simplicity and no-premature-abstraction rules still hold (this cuts code, never adds speculative layers).