review-standards

Review code changes against general quality and writing style standards. This skill covers cross-cutting concerns that apply to every language and every file type.

Mandatory Process

Execute these checks in order. Do not skip steps. Every finding requires file, line, and evidence.

Pass 1: Dead Code Detection

Search for code that serves no purpose:

1. Commented-out code — grep for blocks of commented code (not explanatory comments). Any // oldFunction() or /* former implementation */ is a violation. Version control preserves history; commented code is noise.
2. Unreachable code — statements after unconditional returns, breaks, or throws. Check switch cases that fall through to dead default branches.
3. Unused imports — imports that nothing in the file references. Run the language-appropriate tool or grep for the imported name.
4. Unused variables — declared but never read. Pay attention to destructured values where only some fields are used.
5. Unused functions — exported functions with zero call sites across the codebase. Use Grep to verify: grep -r "functionName" --include="*.ts" (adjust extension). A function with zero callers outside its own file and test file is dead.
6. Feature flags for shipped features — conditionals around features that have been live for months. These are dead branches waiting to confuse someone.

Pass 2: Lint Suppression Audit

Every suppression is a code smell until proven otherwise.

1. Find all suppressions: grep for language-specific patterns:
   • TypeScript/JS: // eslint-disable, // @ts-ignore, // @ts-expect-error, // @ts-nocheck
   • Python: # noqa, # type: ignore, # nosec
   • C#: #pragma warning disable, [SuppressMessage]
   • Go: //nolint
   • General: // noinspection
2. Check for justification: Each suppression must have an inline comment explaining why. Bare suppressions without explanation are always a finding.
3. Check for scope: Line-level suppressions are acceptable. File-level or block-level suppressions are findings unless they address a known tooling bug (and say so).
4. Check for staleness: Does the suppression still apply? If the code around it changed, the reason for the suppression may no longer exist.

Pass 3: Single Responsibility Check

For every function or method in the changed files:

1. Line count: Functions over 40 lines warrant scrutiny. Over 80 lines is a finding unless the function is a clearly linear pipeline (e.g., a build script).
2. Parameter count: More than 4 parameters suggests the function does too much or needs a configuration object.
3. Return type complexity: Functions that return different shapes depending on code paths indicate mixed responsibilities.
4. Naming: If the function name contains "And" or "Or" (e.g., validateAndSave, fetchOrCreate), it handles two concerns.
5. Nesting depth: More than 3 levels of nesting (if inside if inside loop) is a finding. Extract inner logic.

Pass 4: Error Handling Quality

1. Thrown strings: throw "something went wrong" or raise Exception("error") — must use typed/custom errors with enough context to debug.
2. Swallowed errors: Empty catch blocks, catch (e) {}, except: pass, catch { } — always a finding.
3. Generic catches: catch (Exception) at a level where specific exceptions should be handled individually.
4. Error messages: Do error messages include enough context to diagnose the problem? "Failed" is useless. "Failed to fetch user {id}: connection timeout after 5s" is actionable.
5. Error propagation: Are errors from dependencies wrapped with context, or do raw database/HTTP errors leak to callers?

Pass 5: Naming Consistency Across Contexts

When the diff touches multiple modules or bounded contexts, check that the same semantic operation uses the same method name everywhere.

1. Cross-context scan — if the diff spans 2+ directories that represent different domains/modules, identify operations that exist in both
2. Name comparison — the same operation must use the same verb and structure. fail() in one context and record_failed() in another for the same semantic action is a finding
3. Scope — this applies to public/exported methods, command/event names, and API endpoints. Internal helpers are less critical but still worth flagging if the inconsistency is confusing

Pass 6: Writing Style (Docs, Comments, Copy)

Only run this pass if the diff includes documentation, comments, README content, UI copy, or commit messages. (Skip if no text content in the diff.)

1. Banned words — flag any occurrence: delve, tapestry, landscape, nuanced, robust, crucial, vital, realm, foster, leverage, facilitate, utilize, comprehensive, holistic, synergy, paradigm, ecosystem, streamline, empower, cutting-edge, game-changing, dive into, at the end of the day, when it comes to

2. Banned phrases — flag any occurrence:

   • "It's important to note", "It's worth mentioning"
   • "In today's world", "In the modern era"
   • "At its core", "At the heart of"
   • "This allows us to", "This enables us to"
   • "A myriad of", "A plethora of"
   • "Moving forward", "Going forward"
   • "Best practices" (say what the practice actually is)

3. Voice and structure:

   • Passive voice ("was implemented", "is handled by") — rewrite to active
   • Participial phrase openers ("-ing" constructions at sentence start) — rewrite to direct subject-verb
   • Sentences over 30 words — split or tighten
   • Em dashes: 2 per document maximum. More than that indicates overuse as a crutch

4. Comment quality:

   • Comments that restate the code ("increment i by 1") — remove
   • Comments that explain WHY, not WHAT — keep and encourage
   • TODO comments without owner or ticket reference — finding

Anti-Patterns

Do not flag these — they are acceptable:

• Short utility functions (under 10 lines) without doc comments
• Suppressions with a linked issue number and explanation
• Commented code in test files used as examples or templates (but verify)
• Functions with many parameters if they are builder/factory patterns

Evidence Format

Every finding must follow this structure:

### [SEVERITY] [Category]: [Short description]

**File:** `path/to/file.ext:42`
**Evidence:** [exact code or grep output showing the violation]
**Standard:** [which rule from this skill is violated]
**Fix:** [concrete code or action to resolve it]

Severity levels:

• CRITICAL — must fix before merge. Swallowed errors, security-relevant suppressions, dead code hiding bugs.
• IMPORTANT — should fix before merge. Lint suppressions without justification, functions with mixed responsibilities.
• SUGGESTION — improve when convenient. Style issues, minor naming concerns, comment quality.

Output Template

## General Standards Review

### Summary
- Files reviewed: N
- Findings: X critical, Y important, Z suggestions
- Suppressions: N found, M unjustified
- Dead code: N instances

### Findings

[findings grouped by severity, each following the evidence format above]

### Clean Areas
[brief note on what was done well — this matters for calibration]

Zero-Finding Gate

If you find zero issues, state that explicitly: "No findings. All changed files pass general standards review." Do not manufacture findings to appear thorough.

Related Skills

• Language-specific reviews for deeper checks: /coding-standards:review-dotnet, /coding-standards:review-python, /coding-standards:review-typescript
• /coding-standards:review-git — commit message and PR conventions. Run alongside when reviewing PRs.