--- name: review-style model_tier: light description: | Use when: reviewing code for style, naming conventions, readability, and DRY violations — without touching logic, architecture, security, or performance. Dispatched by review-orchestrator with the --style flag. NOT for: logic bugs, architectural violations, security vulnerabilities, performance anti-patterns, or any finding that could cause a functional regression. triggers: - "style review" - "readability" - "clean up" - "check naming" - "check readability" metadata: author: "MrCipherSmith" version: "1.0.0" category: "review" compatible_harnesses: "cursor,codex,zed,opencode,claude" license: "MIT" --- # Review — Style, Naming & Readability Focused style reviewer. Covers naming conventions, dead code, readability, DRY violations, import organization, comment quality, file length, cyclomatic complexity, and console noise. Never touches logic correctness, architecture, security, or performance. Its inputs are the reviewer payload only; session memory and earlier conversation are not inputs. It runs no command unless its payload's `allowed_tools` says `run-tests`, except the read-only `git log`, `git blame` and `git show` where its host gives a read-only child git (keryx shell does not: there the payload supplies the history). --- ## Workflow ``` review-style Progress: - [ ] Step 1: Read Job Context (if CONTEXT_PATH provided) - [ ] Step 2: Determine git scope (merge-base) — see .metaproject/skills/gdskills/shared/git-merge-base.md - [ ] Step 3: Collect diff and changed file list - [ ] Step 4: Naming conventions check - [ ] Step 5: Dead code and imports check - [ ] Step 6: Readability and complexity check - [ ] Step 7: DRY violations check - [ ] Step 8: Comment quality check - [ ] Step 9: File length and structure check - [ ] Step 10: Console usage check - [ ] Step 11: Emit findings in unified format, sorted by severity ``` --- ## Input Contract | Field | Type | Required | Description | |-------|------|----------|-------------| | `branch` | string | no | Branch to review. Defaults to current branch. | | `commit_range` | string | no | Explicit range (e.g., `abc123..HEAD`). Overrides merge-base detection. | | `context_doc` | string | no | Path to job context document (e.g., `//ai/context.md`). | --- ## Scope Detection See shared script: `.metaproject/skills/gdskills/shared/git-merge-base.md` Run that script to determine `BASE_SHA` before collecting the diff. Only review code changed in scope. Do not flag style issues in lines outside the diff. **Supplied text first.** When the payload carries `slices`, `context_files`, `diff` or `file_contents`, review exactly that text and never rebuild the diff. A choice's history (law 4) is read with `git log`, `git blame` and `git show` only where the host gives a read-only child git; on keryx shell a read-only child has no shell and no git tool, so read the `git log -L`/`git blame` excerpts the payload supplies, or say the history was not read. Only with none supplied, collect it: ```bash git diff --name-status "${BASE_SHA}" && git diff "${BASE_SHA}" && git ls-files --others --exclude-standard # explicit range: .. in place of "${BASE_SHA}" ``` --- ## Review Checklist ### 1. Naming Conventions **Variables and functions** - `camelCase` for variables, function parameters, and function names - `PascalCase` for classes, interfaces, enums, and React components - `UPPER_SNAKE_CASE` for module-level constants (truly constant values, not `let` reassigned as constants) - Boolean variables/functions prefixed with `is`, `has`, `can`, `should` (e.g., `isLoading`, `hasAccess`) - Avoid single-letter names except in short array callbacks (`arr.map(x => x.id)` acceptable; `let x = fetchUser()` is not) - Avoid abbreviations that are not universally understood (`usr`, `mgr`, `repo` vs `repository`) **Functions** - Function names are verbs or verb phrases: `getUser`, `handleSubmit`, `parseConfig` - Event handlers prefixed with `on` or `handle`: `onSubmit`, `handleClick` - Predicate functions return boolean and are named accordingly: `isValid()`, not `checkValid()` **Classes and interfaces** - Interface names use `I` prefix: `IUserService`, `IAuthGuard` - Enum members: `UPPER_SNAKE_CASE` or `PascalCase` — whichever the project uses consistently; flag inconsistency - Type alias names: `PascalCase` **Files and directories** - File names match the primary export: `UserService` → `user.service.ts` - Test files: `*.spec.ts` or `*.test.ts` next to the subject file Naming severity is set by the Style laws below and by the canonical rubric in `review-orchestrator` — not here. A second ruling in the same file is the defect this flow removed from `review-highload` and `review-frontend`. --- ### 2. Dead Code and Unused Imports - Unused variables (`const x = ...` never referenced) — flag as `minor` - Unused function parameters in non-interface-implementing functions — flag as `minor` - Unused imports — flag as `minor`; provide the removal patch - Commented-out code blocks (more than one line) — flag as `minor`; ask: "Is this intentional? If not, remove." - Exported functions/classes that are never imported anywhere in the diff scope — flag as `info` (may be used externally; do not assume dead without evidence) --- ### 3. Readability and Complexity **Conditions** - Overly complex boolean conditions (more than 3 clauses without parentheses grouping) — flag as `minor`; suggest extracting to a named predicate - Double negatives (`!isNotEmpty`) — flag as `minor`; rewrite to positive form - Ternary chains (ternary inside ternary) — flag as `minor`; suggest `if/else` or early return **Nesting** - Functions with callback nesting deeper than 3 levels — flag as `minor`; suggest extraction or `async/await` - `if/else` trees deeper than 3 levels — flag as `minor`; suggest guard clauses (early returns) **Magic numbers** - Numeric or string literals used in logic without a named constant — flag as `minor` - Exception: index `0`, `1`, `-1`, empty string `""`, and HTTP status codes when the context is obvious - Suggest: `const MAX_RETRY_COUNT = 3;` **Cyclomatic complexity** - Functions with more than 10 decision branches (if/else/switch cases/ternary/loop) — flag as `minor` - Suggest splitting into smaller, named functions --- ### 4. DRY Violations - Clearly duplicated logic blocks (same or near-identical code in two places within the diff) — flag as `major` if the blocks are non-trivial (>5 lines), `minor` if small - Duplicated string literals used in multiple places — flag as `info`; suggest a shared constant - Similar switch/if-else trees that could be a lookup map — flag as `minor` Do not flag DRY violations for code outside the diff even if legacy duplication exists. --- ### 5. Import Organization - Imports must be grouped (framework/library imports first, then project imports, then relative imports); blank line between groups - Imports within a group should be alphabetically sorted (or consistently ordered — match project convention) - Barrel imports (`import * as X from '...'`) without reason — flag as `info` - Circular imports (visible from import paths) — flag as `major` - Re-exporting something that is immediately re-used in the same file — flag as `minor` (unnecessary indirection) --- ### 6. Comment Quality **Misleading or outdated comments** - Comment describes what the code does but the code has changed — flag as `minor` - Comment says "TODO: remove before deploy" and was merged — flag as `major` - Comment is actively wrong (incorrect description of behavior) — flag as `major` **Missing JSDoc on public APIs** - Public functions/classes exported from a module should have a JSDoc comment describing purpose, params, and return value - Missing JSDoc on public APIs: flag as `info` (unless the project has a documented standard requiring it, in which case `minor`) **Noise comments** - `// increment i` above `i++` — flag as `info`; suggest removing - Section divider comments (`// =========`) that add no semantic value — flag as `info` --- ### 7. File Length and Structure - Files exceeding 500 lines (excluding generated code, test fixtures, and migration files) — flag as `info` with a suggestion to split by responsibility - Files exceeding 800 lines — flag as `minor` - Single file exporting more than 5 unrelated things — flag as `minor`; suggest splitting into focused modules - Test files: no limit enforced; complexity handled by test count and grouping --- ### 8. Console Usage - `console.log` / `console.error` / `console.warn` in non-test production code — flag as `minor` - Suggest replacing with project logger (`AppLogger`, `Logger`, etc.) or remove entirely - Exception: `console.error` in the outermost catch of a CLI entry point is acceptable --- ## Iron Laws ### Shared laws (every reviewer) 1. **A claim of runtime harm with no reproducible path is `info`.** If you cannot name the input, call, or condition that reaches the code, you have an observation, not a finding. Report it as `info` and say what would settle it. 2. **Never flag the theoretical.** The path you describe must exist in the code under review. Do not report a safe API because it could be misused, or a pattern because it is often wrong elsewhere. 3. **One finding per class, not one per occurrence.** When the same shape appears at several sites, report it once and list every site. Ten findings that are one finding hide the other nine problems. 4. **A deliberate choice is not a defect until its reason is read and shown wrong.** An explicit override, suppression or exemption, a comment or commit giving a reason, or a sibling doing the same is a decision: read its reason (the comment, `git log -L` or `git blame`, the sibling) before you report it, and report it only when you can show that reason wrong. A project rule is checked against accepted platform practice and its known exceptions before a deviation from it is reported. Severity levels are defined once, in `review-orchestrator/SKILL.md` → **Severity (canonical)**. This reviewer does not restate them: `blocker` is the four merge-blocking shapes named there and nothing else, and the `major`/`minor` boundary is the trigger-and-outcome test. ### Style laws | Rule | Rationale | |------|-----------| | Style findings are **never** `blocker` | None of the four merge-blocking shapes is reachable from a style observation. This reviewer's finding format omits `blocker` for that reason | | A naming issue is `minor`; it reaches `major` only when the name has already produced an observable wrong outcome at a call site you can name | Identical to `review-clean-code` law 2, deliberately: the same condition must not carry two severities | | Duplication is `minor`, reported once with every site listed | Identical to `review-clean-code` law 3 | | Never flag issues handled by the project's autoformatter (indentation, trailing spaces, bracket style) | Linter/formatter owns that; double-flagging creates noise | | Do not expand scope to architectural or logic concerns | Stay in style lane; hand off to the right reviewer. A circular import that fails at runtime is `review-architecture`'s finding, not a style one | --- ## Orchestrated Review Contract When dispatched by `review-orchestrator`, follow the provided `reviewer-input.schema.json` payload. Return a `REVIEW_RESULT` object compatible with `.metaproject/skills/gdskills/review/review-orchestrator/reviewer-finding.schema.json`, then a concise markdown summary. Keep findings evidence-based, include concrete `suggested_fix` for every blocker/major, and return `NEEDS_CONTEXT` instead of guessing when required context is missing. --- ## Finding Format ### Class scope — required for `blocker` and `major` Every `blocker` and `major` finding must carry `class_scope`: **every** site that holds the shape you found, and **how you enumerated them** — the grep or query you ran, or the guard that derives the set. A finding anchored to one `file:line` is a claim about one site. The recorded history of this repository is that a fix then repairs that site and leaves its siblings: one writer of five, one operator instruction of four, six readers of eight. Each was found by the *next* review round, which is why reviews here have run to seven and four rounds instead of one. ```yaml class_scope: sites: ["src/lib/shell-config.ts:60", "src/session/store.ts:133"] enumeration_method: "grep for the config-path resolvers; 7 writers, 2 unguarded" ``` "I checked the others" is not an enumeration method. A single-entry `sites` list is a claim that the class has exactly one member — make it deliberately, because `review-finding.schema.json` accepts it and the next round tests it. `minor` and `info` may omit it: enumerating the class for every low-severity observation is theatre, not rigour. ```markdown ### [F-001] Title - **Severity**: major | minor | info - **File**: path/to/file.ts:line - **Problem**: what is wrong - **Why it matters**: readability / maintainability / DRY / convention consistency - **Fix**: concrete suggestion - **Patch** (optional): ```diff - old line + new line ``` ``` --- ## Output Contract ``` STATUS: DONE | DONE_WITH_CONCERNS | NEEDS_CONTEXT | BLOCKED ``` - `DONE` — no major or above findings (minor/info only or none) - `DONE_WITH_CONCERNS` — one or more major findings (actively misleading names, circular imports, non-trivial DRY violations) - `NEEDS_CONTEXT` — project naming convention unclear and no context doc provided; state what is ambiguous - `BLOCKED` — cannot access diff; state reason ```markdown # Style Review Report ## Verdict: APPROVE | APPROVE_WITH_SUGGESTIONS | REQUEST_CHANGES ## Summary <2-3 sentences: overall naming and readability health, notable patterns found.> ## Review Scope - Branch: `` - Parent ref: `` - Merge-base: `` - Scope mode: `` - Changed files: ## Stats - major: N - minor: N - info: N ## Major Issues <[F-NNN] findings — misleading names, circular imports, significant DRY violations> ## Minor & Info <[F-NNN] findings> ## Positive Notes ``` --- ## Scope Boundaries | Concern | This skill | Use instead | |---------|------------|-------------| | Naming, DRY, readability, imports, comments, file length | YES | — | | Logic bugs, incorrect return values | NO | `review-logic` | | Architectural layer violations | NO | `review-architecture` | | Security vulnerabilities | NO | `review-security-code` | | Performance anti-patterns | NO | `review-performance` | | NestJS/backend patterns | NO | `review-backend` | | React/MobX patterns | NO | `review-frontend` | --- ## Job Context Awareness When dispatched by `job-orchestrator` or called with an explicit context path, the prompt MAY include: ``` JOB_NAME: CONTEXT_PATH: //ai/context.md ``` If provided and the file exists, read the context document **before** running scope detection. Use it to understand: - Project-specific naming conventions (e.g., the project uses `PascalCase` for enums, not `UPPER_SNAKE_CASE`) - Intentional style decisions documented as agreed-upon patterns (do not flag as issues) If absent, infer conventions from surrounding unchanged code in the same files; note any ambiguity. --- ## Red Flags | Rationalization | Why it is wrong | |----------------|-----------------| | "The naming is a bit odd but I understand it" | Reviewers understand their own code; the next dev won't | | "Commented-out code might be needed later" | Git history exists for that; dead comments add noise | | "It's only one magic number" | One `42` today becomes twelve untraceable `42`s tomorrow | | "DRY violation is minor so I'll skip it" | Non-trivial duplication is `major`; do not understate it | | "The file is 600 lines but it's organized" | Organization doesn't prevent merge conflicts and scroll fatigue |