# Review Nest Refactor > Review any Node/Express-style service's port to the NestJS framework against two standards — behavior-preserving correctness (does the port change what the service actually does?) and idiomatic NestJS architecture (modules, DI, DTOs/pipes, guards, interceptors, filters, ConfigModule, lifecycle hooks, testing, health checks, OpenAPI) per nestjs.com's own conventions. Use whenever a PR or diff ports a hand-rolled Node/Express-style service (or one module/route group of one) to NestJS, or when the user asks to review a "Nest migration," "Nest refactor," "Nest port," or wants to check whether new Nest code is idiomatic vs. just Express-with-decorators. Works on any repo/service — it discovers that service's own invariants and consumers rather than assuming a fixed list. Always surfaces a first-class "Migration Gaps" section — what's still missing relative to a complete idiomatic Nest port — not just pass/fail findings on what changed. - Skill: `ravi-rejolut/review-nest-refactor` (Agent Skill, multi-file: 3 files) - Install (CLI): `npx skillmds@latest add ravi-rejolut/review-nest-refactor` - Raw SKILL.md: https://api.skillmd.com/api/skills/ravi-rejolut/review-nest-refactor/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: Ravi-Rejolut (https://skillmd.com/u/ravi-rejolut) - Updated: 2026-09-22 - Page: https://skillmd.com/skills/ravi-rejolut/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](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/`-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 ... --stat`, full diff, `git log --oneline ..`), 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 :`) for anything the diff touches — you cannot check behavior preservation without knowing what the behavior was. - Follow [BEHAVIOR_PRESERVATION.md](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](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](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](path/to/file.ts#L42) — 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%): \ **Suggested fix:** ```suggestion 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](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](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.