Review Nest Refactor

NestJS Refactor Review

A framework-migration review is a different job from a routine feature-PR review (that's what the code-review skill is for). Here the diff is large, touches structure more than behavior, and the two failure modes are opposite: either the "port" quietly changes something that mattered (a regression), or it changes nothing structurally and is just old code wearing @Injectable() (a missed migration). This skill checks for both, plus what's still left to do. It is standalone — not dispatched from code-review's router — because neither checklist here (rediscovering a service's own behavioral contract, and checking it against NestJS's architectural idioms) is a routine FE/BE review concern.

This skill is deliberately generic: it does not hardcode any project's service names, ports, or specific invariants. Every review starts by reading this repo and this service to find out what actually must not change — see BEHAVIOR_PRESERVATION.md for the discovery method. Don't skip that step because a previous review of a different service (or a different repo) already surfaced a list — re-derive it.

Reuses the finding-block format and severity scale from the code-review skill family (see Output Contract below) so review output reads consistently with the rest of that family, but keep the two skills separate: don't route a plain feature PR here, and don't route a framework port through code-review.

When to Use

• A PR/branch ports a service, or one module/route group within a service, from hand-rolled Node/Express-style code to NestJS.
• The user asks "is this idiomatic Nest," "review this Nest migration," or similar, about any service in any repo.
• A partial/incremental port — some routes moved to Nest while the rest of the service is still plain Node — still qualifies; scope the review to what changed but still run both checklists against it.

When Not to Use

• A routine feature change inside an already-fully-Nest service with no architectural migration in play — use code-review instead.
• A service written in a different language/runtime, or a frontend/UI layer — this skill is scoped to Node/TS backend services that are candidates for (or mid-way through) a NestJS port.

Step 1: Resolve Service and Targets

Parse $ARGUMENTS as [service-path] [base-target] [review-target]:

• service-path: the package/directory being ported (e.g. services/api, packages/worker) — whatever this repo actually calls it. If omitted, infer from the diff's file paths. If the diff spans more than one independently-deployed service, say so up front and run the full review once per affected service rather than blending findings across services — each one likely has its own consumers and its own invariants.
• base-target / review-target: same resolution order as code-review's Step 1 — two refs, one range (base..review), one ref (treat as base, use current branch as review), or infer from the active PR. Ask only if still unresolved.
• Prefer origin/<branch>-prefixed refs so the comparison is against latest remote state, not a stale local ref (fetch first if unsure).

Step 2: Gather Context

Beyond the usual diff commands (git diff <base>...<review> --stat, full diff, git log --oneline <base>..<review>), a migration review specifically needs the pre-refactor behavior, not just the new code:

• Read the pre-refactor version of every handler/module being ported (git show <base>:<path>) for anything the diff touches — you cannot check behavior preservation without knowing what the behavior was.
• Follow BEHAVIOR_PRESERVATION.md §1 to find wherever this repo documents its own invariants (project docs, files named for the concept, incident-referencing comments, existing tests, config fallback logic). Do this before judging the diff — you need to know what the service promised before you can tell whether the port kept the promise.
• Check the service's package.json for which Nest packages were actually added (@nestjs/core, @nestjs/config, @nestjs/swagger, @nestjs/terminus, @nestjs/schedule, class-validator, class-transformer, @nestjs/testing) — a port that adds @nestjs/core and @nestjs/common alone, without the ecosystem packages the idiom checklist below expects, is a strong early signal of a partial/cosmetic port before you even read the code.
• Grep the rest of the repo (and any adjacent repos you have access to) for references to this service's routes, ports, or response shapes — other services, a frontend, scripts, dashboards. These are the consumers whose expectations the port must not silently break.

Skip lockfile content review (same as code-review); only note new dependencies if they materially affect risk or confirm/contradict the point above.

Step 3: Correctness / Regression Check (PASS / PARTIAL / FAIL)

Using what Step 2 turned up, walk every invariant/contract you found that the diff's blast radius could plausibly touch, plus the generic drift categories in BEHAVIOR_PRESERVATION.md §2 (auth boundaries, ordering/atomicity, idempotency, fallback-vs-fail-fast, scheduled-work cadence, consumer contract shape, error-handling parity). State PASS (preserved, with evidence), PARTIAL (changed but not obviously broken — explain the uncertainty), or FAIL (behavior drifted), with the specific before/after evidence. Categories clearly outside the diff's blast radius can be noted as "not touched" without a full walk-through — don't pad the review with irrelevant checks, but don't skip one just because it's inconvenient to verify.

Any FAIL, and any PARTIAL where the drift is plausible enough to matter, is a Finding in the Output Contract below (Step 5) — not a footnote. Behavior drift defaults to Critical or High severity (see Severity Scale) regardless of how clean the surrounding refactor is.

Done when: every invariant/contract found in Step 2, plus every generic drift category with plausible blast-radius overlap, has a stated verdict with evidence.

Step 4: NestJS Idiom / Architecture Check (PASS / PARTIAL / FAIL)

Walk NESTJS_IDIOMS.md's twelve sections against the diff. For each, state PASS (idiomatic), PARTIAL (adopted the decorator but not the underlying pattern — e.g. @Injectable() on a class that still reaches for process.env directly), or FAIL / N/A (not attempted, or the section genuinely doesn't apply to this service — e.g. a pure background worker may have no HTTP controllers at all).

This check is about whether the shape of the code changed, not just its decorators. The single most useful question per section: "if I deleted the Nest imports, would this file still basically work as plain Express/Node code?" If yes, that section is at best PARTIAL.

Findings from this section that materially affect testability, maintenance, or the correctness checks in Step 3 (e.g. a missing guard that also happens to be where a permission boundary is enforced) become Findings. Findings that are purely architectural cleanliness with no correctness/testability consequence — e.g. missing Swagger decorators — belong in Step 5 (Migration Gaps) instead of the Findings list; don't double-count the same observation in both places.

Done when: all twelve idiom sections have a stated verdict (including explicit N/A where a section doesn't apply to this service).

Step 5: Migration Gaps (first-class deliverable, not a footnote)

Produce a standalone "Migration Gaps" section separate from Findings. This answers a different question than Steps 3-4: not "is anything wrong with what changed," but "what would still need to happen for this to be a complete idiomatic Nest port." Populate it from every PARTIAL/FAIL/N/A in Step 4 that doesn't already have correctness consequences, plus anything obviously missing that the diff didn't attempt at all (e.g. no tests added for the ported module, no health endpoint yet, no ConfigModule validation schema, DTOs added for the happy path but not for error responses).

Format each gap as a short line, grouped by area, e.g.:

### Migration Gaps
- **Config**: no ConfigModule validation schema yet — `process.env.API_URL`
  still read directly in `PricingService` (pricing.service.ts:41).
- **Testing**: no `@nestjs/testing` TestingModule/e2e suite added for the
  ported `OrdersController` routes; existing coverage was dropped, not moved.
- **Observability**: no global exception filter registered — three
  controller methods still have local try/catch (orders.controller.ts:22,58,90).
- **API docs**: no `@nestjs/swagger` decorators on the new DTOs.

This section is what makes the review useful for planning the next PR in the migration, not just judging the current one — treat it as seriously as the Findings list, even though gaps aren't yet-broken code.

Done when: every PARTIAL/FAIL/N/A from Step 4 without correctness consequence is accounted for in this section, plus any obviously-skipped migration work the diff didn't touch at all.

Output Contract

Structure the full response as:

1. Summary — which service/module, what the diff's blast radius is, overall risk level (is this a behavior-preserving structural refactor, or does it also change behavior?).
2. Correctness / Regression Findings — from Step 3, using the finding block format below. State "No correctness regressions found" explicitly if true — don't imply it by omission.
3. Architecture Findings — from Step 4, same format, only for items with real consequence (see Step 4's note on avoiding double-counting).
4. Migration Gaps — from Step 5, using the gap-list format above, not the finding-block format (gaps aren't findings; don't force severity/certainty onto something that isn't yet a problem).
5. Positive highlights — genuine structural improvements worth preserving (e.g. a fail-fast config schema replacing a silent fallback).
6. Final verdict.

Every Finding (Correctness or Architecture) is a standalone block:

[I1] file.ts:42 — High

Reasoning: what changed, why it's a real risk, and — for Correctness findings — which invariant or contract found in Step 2/3 it affects.

Certainty (0-100%): <number>

Suggested fix:

corrected code here

Stable IDs: [I#] issues, [N#] nitpicks, [Q#] questions, [C#] code-quality/architecture observations (use [C#] for Step 4 Architecture Findings that don't rise to a correctness-affecting [I#]).

Severity scale (shared with code-review, review-fe-prepush, review-be-prepush — do not redefine it):

• Critical: security holes, data loss/corruption, financial-correctness failures, user-blocking regressions. Any confirmed invariant FAIL from Step 3 involving auth/authz boundaries, money, or data-loss risk defaults to this tier.
• High: likely production bugs, broken protected/critical flows, major performance/reliability/state regressions. Most other confirmed invariant FAILs land here.
• Medium: maintainability, clarity, lower-risk consistency, or smaller operational concerns. Most Step 4 Architecture Findings that aren't tied to a Step 3 invariant land here.

Final verdict must include:

• Behavior Preservation: x/10 — reason (this replaces "Backend Risk" for this skill; it's specifically about whether the port changed behavior).
• NestJS Idiom Adoption: x/10 — reason (how much of the shape actually changed vs. decorators-only).
• Migration Completeness: x/10 — reason, informed by the size/severity of the Migration Gaps section.
• Overall Rating: x/10.
• Recommendation: short paragraph — safe to merge as an incremental step, or does a correctness finding block it regardless of how good the architecture looks?

If any required section is missing, regenerate before responding — same enforcement discipline as code-review's Output Contract.

Reference Files

• BEHAVIOR_PRESERVATION.md — the Step 3 method: how to find what a given service's own invariants are, plus the generic categories of drift a framework port tends to introduce regardless of what the service does.
• NESTJS_IDIOMS.md — the Step 4 checklist: what idiomatic Nest looks like per architectural building block, per nestjs.com's own conventions.

Resolve both paths against this skill's own directory, not the repo being reviewed.