code-review

name	code-review
description	Systematic code review for correctness, security, and growth — not just style enforcement

Code Review Skill

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)

Shared Knowledge Gate

Before reviewing non-trivial, repeated, or consequential changes, consult scout-knowledge-base for relevant prior decisions, failure modes, procedures, and gotchas. Read only records whose signals or preconditions match the change, and treat them as context rather than authority. If unavailable, state that once and continue with the current evidence; do not create a local fallback while reviewing.

3-Pass Review

Pass	Focus	What You're Looking For	Time
1. Orientation	Big picture	Does the approach make sense? Is the scope right? Over-engineered?	2-3 min
2. Logic	Deep read	Edge cases, null handling, error paths, concurrency, off-by-one	10-15 min
3. Polish	Surface	Naming, duplication, test coverage, docs	3-5 min

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

Comment Prefixes

Prefix	Meaning	Author Response
[blocking]	Must fix before merge	Fix it
[suggestion]	Better approach exists	Consider it, explain if declining
[question]	I don't understand	Clarify (in code, not just in reply)
[nit]	Trivial style issue	Fix if easy, skip if not
[praise]	This is well done	Appreciate it

Good vs Bad Comment Examples

Bad	Why	Good
"This is confusing"	Vague, unhelpful	"[suggestion] This nested ternary is hard to follow. Consider extracting to a named function like isEligibleForDiscount()."
"Fix this"	No context	"[blocking] This accepts user input without sanitization. Use escapeHtml() before rendering."
"Why?"	Sounds hostile	"[question] What's the motivation for the custom sort here vs Array.sort()? Is there a performance concern?"
"LGTM" (on 500-line PR)	Rubber stamp	"Pass 1: Approach looks right. Pass 2 comments below. Pass 3: naming is clean."

Reviewer and Author Posture

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

Reviewer	Author
Assume positive intent	Be open to feedback
Ask questions, don't demand	Explain your reasoning
Focus on the code, not the person	Don't take feedback personally
Offer alternatives, not just criticism	Acknowledge good suggestions

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

Anti-Pattern	What Happens	Instead
Rubber-stamp	Bugs ship	Actually read Pass 1-3
Bikeshedding	Hours on naming, ignore logic bugs	Spend 80% on Pass 2
Gatekeeping	Reviewees dread PRs	Teach, don't block
Week-long queue	PRs go stale, conflicts pile up	Review within 4 hours, merge within 24
Style wars	Team friction	Automate style (ESLint, Prettier, etc.)
Everything-is-blocking	Author overwhelmed	Use prefix system honestly

Mission-Critical Review (NASA Standards)

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

Blocking Violations (Must Fix)

Rule	Check For	Detection
R1	Recursive function without maxDepth parameter	grep -rn "function.*\(" | xargs grep -l "walk|traverse|recurse"
R2	while loop without iteration counter	Manual review of all while statements
R3	Unbounded array growth	push() in loops without size checks

High Priority (Strong Recommendation)

Rule	Check For	Detection
R4	Function > 60 lines	Line count per function
R5	Missing entry assertions	Public functions without precondition checks
R8	Nesting > 4 levels	Visual inspection of indentation

Medium Priority (Consider)

Rule	Check For	Detection
R6	Variable declared far from use	Manual review
R7	Unchecked return values	grep for ignored returns
R9	Deep property access without ?.	obj.prop.prop.prop chains
R10	Compiler warnings	Build output

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

Review Timing

PR Size	Expected Review Time	If Larger
< 100 lines	< 30 min	—
100-400 lines	30-60 min	Ideal size
400+ lines	60+ min	Ask author to split
1000+ lines	Don't	Refuse; request breakdown

---

Deep Review Mode

Use deep review mode for architectural changes, multi-file refactors, security-sensitive code, high-stakes merges, or any review where single-pass analysis may miss issues.

Do not use deep review mode for routine single-file edits, formatting-only changes, or documentation-only PRs.

Three perspectives

Perspective	Mindset	Core question	Owns
Advocate	"Why is this correct?"	What problem does this solve, and what design choices are intentional?	Correctness defense
Skeptic	"How can I break this?"	What inputs, states, or assumptions make this fail?	Correctness attack
Architect	"Is this the right direction?"	Does this fit the system and set the right precedent?	Direction and blast radius

Workflow

1. Gather context: diff, related files, tests, and recent history for changed modules.
2. Run Advocate, Skeptic, and Architect perspectives independently.
3. Synthesize agreements as high-confidence findings.
4. Resolve conflicts with evidence: file:line citations beat assertions.
5. Mark unresolved disagreements as disputed for human decision.

Deep review output

## Deep Review: <title>

### Summary
<1-2 sentence overview and verdict>

### Perspectives

**Advocate**: <key defenses and intentional design decisions>

**Skeptic**: <bugs, edge cases, and concerns with evidence>

**Architect**: <patterns, debt, direction, and system-level concerns>

### Consolidated Findings

| # | Issue | Priority | Evidence | Recommendation |
| --- | --- | --- | --- | --- |
| 1 | <issue> | Critical/High/Medium/Low | <file:line or observed behavior> | <fix or decision> |

### Disputed
<issues where perspectives disagree and a human must decide>

Conflict resolution

Conflict	Resolution
Skeptic finds bug, Advocate defends	Advocate must cite evidence that refutes the path; otherwise Skeptic wins.
Advocate says intentional, Skeptic says bug	If Skeptic shows a realistic failure path, it is a bug regardless of intent.
Architect says blocking, Skeptic says non-blocking	Correctness priority belongs to Skeptic; direction and precedent priority belongs to Architect.
No evidence either way	Mark as disputed instead of inventing confidence.

---

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

Dimension	Focus	Tools/Methods	Output
Debug & Logging	Console statements, debug code	grep -r "console\\.log|console\\.debug"	Categorize: legitimate vs removable
Dead Code	Unused imports, orphaned files, broken refs	TypeScript compilation + manual scan	List dead commands, UI, dependencies
Performance	Blocking I/O, sync operations, bottlenecks	grep -r "Sync\(" src/, profiling	Async refactoring candidates
Menu Validation	All commands/buttons work	Manual testing + error logs	Broken commands, missing handlers
Dependencies	Unused packages, leftover references	package.json vs import analysis	Removable dependencies

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

Category	Keep?	Examples
Enterprise compliance	✅	Audit logs, security events, GDPR actions
User feedback	✅	TTS status, long-running ops, critical errors
Debug noise	❌	Setup verbosity, migration logs, info messages
Development artifacts	❌	"Entering function X", temporary debugging

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 deep review mode catches on the same PR, or if the 3-pass model produces consistent false-positive [blocking] comments that authors reasonably decline.