Code Review

Code Review Skill

Good reviews catch bugs. Great reviews teach the author something.

Review Priority (What Matters Most)

1. Correctness — Does it do what it's supposed to?
2. Security — Can it be exploited?
3. Maintainability — Will the next person understand this?
4. Performance — Will it scale?
5. Style — Is it consistent? (ideally enforced by linters, not humans)

3-Pass Review

Pass 1 shortcut: Read the PR description and test names first. They reveal intent faster than code.

Comment Prefixes

Good vs Bad Comment Examples

Reviewer and Author Posture

Most review friction is posture, not substance. Both sides carry an obligation:

A review exists to catch bugs before users do, raise code quality through collaboration, spread knowledge across the team, keep patterns consistent, and leave a written record of why a decision was made. If a comment serves none of those, it is noise.

Review Checklist

Security

• No secrets, tokens, or API keys in code
• User input validated/sanitized before use
• Auth checks on protected endpoints
• No SQL/command injection vectors
• Sensitive data not logged

Logic

• Edge cases handled (empty input, null, boundary values)
• Error paths return meaningful messages
• Async operations have timeout/cancellation
• State changes are atomic (no partial updates)
• All new branches have test coverage

Quality

• Tests cover the changed behavior, not just the changed lines
• No debug code (console.log, TODO-hacks)
• Public API changes documented
• Backward compatibility considered
• Every export is called by production — grep the consumer code for each exported symbol; an exported function tested by unit tests but never called by production is dead weight that lies about what the system does (often surfaces "the feature is promised in CHANGELOG but never wired"). If the test suite passes but production doesn't import the function, either wire it in or remove it.
• Filter-style guards have test data that exercises them — a filter(x => x !== 'local') on a list with zero local entries is unverified; for mutation-testing coverage seed the input. See mutation-testing.

Architecture

• Change is in the right layer (not business logic in the controller)
• New dependencies justified
• No unnecessary coupling introduced

Anti-Patterns

Mission-Critical Review (NASA Standards)

For safety-critical projects, apply NASA/JPL Power of 10 rules during review:

Blocking Violations (Must Fix)

High Priority (Strong Recommendation)

Medium Priority (Consider)

Trigger: User mentions "mission-critical", "NASA standards", "high reliability", or "safety-critical"

Review Timing

Extension Audit Methodology (VS Code Extensions)

When: Before release, after major refactoring, or on quality concerns

Scope: Multi-dimensional code quality analysis beyond standard code review

5-Dimension Audit Framework

Audit Report Template

## Executive Summary
- Console statements: X remaining (Y legitimate, Z removable)
- Dead code: [commands/UI/dependencies list]
- Performance: [blocking operations count]
- Menu validation: [working/broken ratio]

## Recommendations
1. [Category]: [Issue] → [Action] (Priority: Critical/High/Medium)
2. [Category]: [Issue] → [Action] (Priority: Critical/High/Medium)

Console Statement Categorization

Performance Red Flags

• Synchronous file I/O in UI thread: fs.readFileSync, fs.existsSync, fs.readdirSync
  • Fix: Convert to fs-extra async: await fs.readFile, await fs.pathExists, await fs.readdir
• Blocking operations in activation: Heavy computation before extension ready
  • Fix: Defer to background, show loading state, or lazy-load
• Serial operations that could be parallel: Sequential awaits for independent tasks
  • Fix: Promise.all([op1(), op2(), op3()])

Dead Code Detection Pattern

1. Scan command registrations: vscode.commands.registerCommand('command.id', ...)
2. Scan UI references: Search HTML/views for command IDs
3. Cross-check: Commands in UI but not registered = broken; registered but unused = dead
4. Verify disposables: Removed commands should have disposable cleanup too

Post-Audit Verification

• TypeScript compiles: npm run compile → exit 0
• No orphaned imports: All imports resolve
• Version aligned: package.json, CHANGELOG, copilot-instructions match
• Smoke test: Extension activates, 3 random commands work

Pattern applies to: VS Code extensions, Electron apps, Node.js services with UI

Would Revise If

Revise if reviews repeatedly miss security or correctness defects that adversarial review (deep-review skill) catches on the same PR, or if the 3-pass model produces consistent false-positive [blocking] comments that authors reasonably decline.