# Clean Code Review

> Run a comprehensive Clean Code audit against the codebase — module size, function complexity, long parameter lists, TODO/FIXME markers, commented-out code, duplication (DRY), Boy Scout delta, dead exports, test smells, shell-parity (PS/Bash twins), dep boundaries (cross-package imports), frozen-arrays drift (enums.mjs), and ESLint violations — then produce a structured findings report with option…

- **Type:** Skill
- **Install:** `agentstack add skill-srnichols-plan-forge-clean-code-review`
- **Verified:** Yes — security-reviewed for prompt injection and unsafe behavior
- **Seller:** [srnichols](https://agentstack.voostack.com/s/srnichols)
- **Installs:** 0
- **Category:** [Developer Tools](https://agentstack.voostack.com/c/developer-tools)
- **Latest version:** 0.1.0
- **License:** MIT
- **Upstream author:** [srnichols](https://github.com/srnichols)
- **Source:** https://github.com/srnichols/plan-forge/tree/master/.github/skills/clean-code-review

## Install

```sh
agentstack add skill-srnichols-plan-forge-clean-code-review
```

Requires the [AgentStack CLI](https://agentstack.voostack.com/docs/cli). Works with Claude Code, Cursor, and any MCP-compatible agent.

## About

# `/clean-code-review` Skill

## Trigger

"Run a clean code review" / "Audit the codebase for Clean Code violations" / "Check code quality" / `/clean-code-review`

## Purpose

Orchestrates the existing audit scripts in `scripts/audit/` plus the custom ESLint config (`scripts/audit/eslint-clean-code.config.mjs`) into a single pass. Produces a structured report covering all six Phase 42 finding categories, with an optional `--fix-suggestions` mode that emits concrete refactoring guidance for each violation.

## Inputs

| Flag | Required | Default | Description |
|------|----------|---------|-------------|
| `--scope ` | No | `pforge-mcp/**/*.mjs,pforge-master/**/*.mjs` | Comma-separated globs limiting the audit scope |
| `--fix-suggestions` | No | off | When present, each finding includes a concrete fix suggestion (extract helper, rename, wrap params in options object, etc.) |
| `--out ` | No | stdout (formatted) | Write the full JSON report to this path instead of printing a summary |
| `--severity ` | No | `warn` | Minimum severity to report: `error`, `warn`, or `info` |

## Steps

### 1. Run module-size audit

```bash
node scripts/audit/measure-modules.mjs
```

Parse `docs/plans/cleanup-findings/raw/module-metrics.json`. Flag files exceeding the G14 thresholds:

| LOC | Severity | Action |
|-----|----------|--------|
| >3,000 | **error** | Blocking — extract sub-modules |
| 1,000–3,000 | **warn** | Monitor — extract on next feature touch |
| 5 positional arguments.

### 4. Run ESLint with clean-code config

```bash
npx eslint --no-eslintrc -c scripts/audit/eslint-clean-code.config.mjs --format json "pforge-mcp/**/*.mjs" "pforge-master/**/*.mjs"
```

Parse ESLint JSON output. Categorise violations:

| Rule | Category | Severity |
|------|----------|----------|
| `complexity-error` | Cyclomatic complexity | error |
| `complexity-warn` | Cyclomatic complexity | warn |
| `max-lines-per-function-error` | Function length | error |
| `max-lines-per-function-warn` | Function length | warn |
| `max-params-error` | Parameter count | error |
| `max-params-warn` | Parameter count | warn |
| `max-depth` | Nesting depth | warn |
| `max-nested-callbacks` | Callback nesting | warn |
| `no-magic-numbers` | Magic numbers | warn |

### 5. Run duplication detection (DRY)

```bash
node scripts/audit/run-jscpd.mjs
```

Parse `docs/plans/cleanup-findings/raw/duplication-report.json`. For each `duplicates[]` entry report:
- First file + line range, second file + line range, token count
- Group by token-count descending; show top 10

> **Why this matters**: The Phase 41 enums centralization existed because the same string literal had been copy-pasted across 50+ files. `jscpd` catches duplicated *code blocks* mechanically; the literal/symbol patterns documented in `clean-code.instructions.md` (DRY section) still require human eyes at review time.

### 6. (Optional) Run architecture scan

```bash
node scripts/audit/scan-architecture.mjs
```

Parse `docs/plans/cleanup-findings/raw/architecture-report.json`. Report:
- Dependency cycles (Tarjan SCC with ≥2 nodes)
- Cross-layer imports (inner circle depending on outer)
- High fan-in volatile modules
- High fan-out unstable modules

### 7. Run Boy Scout delta check

```bash
node scripts/audit/boyscout-delta.mjs
node scripts/audit/boyscout-delta.mjs --base origin/master
node scripts/audit/boyscout-delta.mjs --base HEAD~1 --include "pforge-mcp/**"
```

Parse `docs/plans/cleanup-findings/raw/boyscout-delta-report.json`. For every file changed since the merge-base, report:
- `boy-scout-violation` — file was edited but ESLint violation count did **not** decrease
- `regression` — file was edited and violation count **increased** (treat as error)
- `improved` — violation count decreased (Boy Scout pass; surface as positive signal in summary)

> **Why this matters**: The Boy Scout Rule in [architecture-principles.instructions.md](../../instructions/architecture-principles.instructions.md) says "every commit touching a file must leave it cleaner." Without a delta check the rule is aspirational. This step makes it enforceable — a PR that touches `orchestrator.mjs` for a feature fix must also clean up at least one existing warning in that file.

### 8. Dead-exports scan

```bash
node scripts/audit/dead-exports.mjs
node scripts/audit/dead-exports.mjs --scope "pforge-mcp"
```

Parse `docs/plans/cleanup-findings/raw/dead-exports-report.json`. For every exported name that no other tracked file imports, report:
- `file` — the module that exports the dead symbol(s)
- `deadExports[]` — symbol names with no consumer
- `totalExports` — how many exports the file has total (ratio dead/total signals refactor candidates)

Entry-point modules (`server.mjs`, `*-cli.mjs`, `scripts/audit/*.mjs`) and files consumed via `import *` are skipped automatically. Dynamic `import()` and external-tool consumers are not tracked — apply judgment before deleting.

> **Why this matters**: Dead exports widen the public surface (so `surface-diff` flags more "breaking" candidates to triage), obscure which symbols are real API, and keep dead code paths alive. A high dead-export ratio in a single file (e.g. 8/12) is a strong signal the module's responsibility has drifted — split it or prune.

### 9. Test-smells scan

```bash
node scripts/audit/test-smells.mjs
node scripts/audit/test-smells.mjs --severity error
```

Parse `docs/plans/cleanup-findings/raw/test-smells-report.json`. Findings are categorised by smell:

| Smell | Severity | Meaning |
|-------|----------|---------|
| `FOCUS-LEAK` | error | `.only(` committed — would skip every other test in the file under vitest |
| `TAUTOLOGY` | error | `expect(true).toBe(true)` and similar — asserts nothing |
| `EMPTY-TEST` | error | `it("...", () => {})` empty body |
| `SKIP-LEAK` | warn | `.skip(` / `xit(` / `xtest(` — silently disabled tests |
| `TIME-FLAKE` | warn | `setTimeout` / `Math.random` / `Date.now` without `useFakeTimers` or `+Nms` tolerance comment |
| `CONSOLE-LEAK` | warn | `console.log/error/warn` in tests — debug leftover |
| `TODO-MARKER` | info | `it.todo(` — track in an issue |

The script **exits non-zero** if any error-severity finding is present. Use this as a pre-merge gate.

> **Why this matters**: Phase 41 S5 timeline-core flaked because a `+5ms` tolerance was too tight for the Windows scheduler. The fix was bumping to `+50ms`. `TIME-FLAKE` catches the class of bug — any time-sensitive test without an explicit tolerance comment is a future flake waiting for the worst possible PR to land on.

### 10. Aggregate and report

Merge all findings into a unified report grouped by category:

```
┌─────────────────────────────────────────────┐
│  Clean Code Review —             │
├─────────────────────────────────────────────┤
│  Category           │ Errors │ Warnings     │
│  ───────────────────┼────────┼──────────    │
│  Module size (G14)  │   3    │    5         │
│  Function length    │   2    │   14         │
│  Complexity         │   8    │   22         │
│  Parameter count    │   0    │    6         │
│  Markers (TODO/etc) │   —    │    4         │
│  Commented code     │   —    │    2         │
│  console.log        │   —    │  bulk        │
│  Duplication (DRY)  │   —    │    7         │
│  Architecture       │   1    │    3         │
│  Boy Scout delta    │   N    │    M         │
│  Dead exports       │   —    │    N         │
│  Test smells        │   K    │    L         │
├─────────────────────────────────────────────┤
│  Total: 14 errors, 56 warnings              │
└─────────────────────────────────────────────┘
```

If `--out ` is provided, write the full JSON report. Otherwise print the summary table and the top 10 highest-severity findings with file paths and line numbers.

### 11. (Optional) Generate fix suggestions (`--fix-suggestions`)

When `--fix-suggestions` is present, append a concrete remediation for each finding:

| Finding type | Fix suggestion pattern |
|-------------|----------------------|
| Function >300 LOC | "Extract `` into a helper function `` in the same module" |
| Complexity >20 | "Replace nested conditionals at line N with early-return guard clauses" |
| >6 positional params | "Wrap parameters into an `options` object: `{ paramA, paramB, ... }`" |
| TODO/FIXME marker | "Convert to a tracked issue via `forge_bug_file` or remove if resolved" |
| Commented-out code | "Delete lines N–M; the code is preserved in git history (`git log -p -- `)" |
| Module >3,000 LOC | "Split by responsibility: extract `` into `.mjs`" |
| Magic number | "Extract `` at line N to a named constant: `const  = `" |
| Dependency cycle | "Break cycle by extracting shared interface into a new module depended on by both sides" |
| Duplicated block (jscpd) | "Extract the duplicated block at : into a shared helper in the nearest common module" |
| Boy Scout violation | "You edited  without reducing violations. Either fix one existing warning in this file (preferred), or document why this PR explicitly avoids touching unrelated code" |
| Dead export | "Either delete the unused export at : (preferred — git preserves history), or document why it's a public API (e.g. plugin contract) and add a `// @public` comment" |
| Test smell FOCUS-LEAK | "Remove `.only` from : — focused tests skip every other test in the file when committed" |
| Test smell TIME-FLAKE | "Wrap the test in `vi.useFakeTimers()` + `vi.advanceTimersByTime()`, or add an explicit tolerance assertion like `expect(elapsed).toBeLessThan(target + 50)`" |
| Test smell CONSOLE-LEAK | "Remove `console.log/error/warn` at : — if you need debugging output, use `vi.spyOn(console, 'log')` and assert on it" |

Fix suggestions are advisory — they do NOT modify code. The agent or user applies them in a follow-up step.

## Conditional: No audit scripts found

> If `scripts/audit/` does not exist or is empty, tell the user to run `setup.ps1` / `setup.sh` to install Plan Forge, then stop. Do **not** attempt to recreate the audit scripts from memory.

## Conditional: ESLint not available

> If `npx eslint` fails (not installed), skip Step 4 and note "ESLint scan skipped — install eslint to enable complexity/params/function-length checks" in the report. The remaining steps still produce useful output.

## Safety Rules

- **Read-only**: This skill analyses code. It MUST NOT modify any source files.
- **No false positives invented**: Every finding must come from a script output or ESLint result. Do not add findings from general knowledge.
- **Scope-bound**: Only scan files matching `--scope`. Do not expand scope silently.
- **Deterministic**: Running the skill twice on the same codebase must produce the same findings.

## Temper Guards

| Shortcut | Why It Breaks |
|----------|--------------|
| "I'll eyeball the code instead of running the scripts" | Misses findings the scripts catch mechanically; inconsistent coverage between runs |
| "Skip ESLint — the other scripts cover enough" | ESLint is the only tool that measures cyclomatic complexity and function length with AST precision; regex approximations miss edge cases |
| "Report all console.log as individual findings" | There are hundreds; the grep-matrix intentionally bulk-triages them as one advisory. Individual reporting floods the report with noise |
| "Generate fix suggestions without `--fix-suggestions` flag" | Unsolicited suggestions clutter the report and distract from triage. The user opts in when ready to remediate |
| "Modify the source code to fix findings" | This is a review skill, not a fix skill. Modifying code without explicit user intent violates read-only safety |

## Warning Signs

- Report shows zero findings in a codebase with known high-severity files — script likely errored silently; check raw JSON outputs
- ESLint reports only warnings but no errors on `orchestrator.mjs` — config may not have loaded; verify `--no-eslintrc -c` path
- `--fix-suggestions` output recommends splitting a file that is <500 LOC — threshold miscalibrated; review against G14 thresholds
- Architecture scan shows no cycles but `scan-architecture.mjs` had madge errors — report the errors, don't suppress them

## Exit Proof

After completing this skill, confirm:

- [ ] All available audit scripts were executed (measure-modules, grep-matrix, long-param-walker, ESLint, run-jscpd, boyscout-delta, dead-exports, test-smells)
- [ ] Findings are grouped by category with error/warning counts
- [ ] If `--fix-suggestions` was requested, each finding has a concrete remediation
- [ ] If `--out` was specified, JSON report exists at the given path
- [ ] No source files were modified during the review

## Relationship to Other Tools

| Tool / Instruction | Relationship |
|-------------------|-------------|
| `.github/instructions/clean-code.instructions.md` | Defines the thresholds and checklist this skill enforces mechanically |
| `.github/instructions/architecture-principles.instructions.md` | Provides the architectural rules the architecture scan validates |
| `scripts/audit/*.mjs` | The actual audit implementations this skill orchestrates |
| `scripts/audit/eslint-clean-code.config.mjs` | Custom ESLint config with aliased clean-code rules |
| `scripts/audit/run-jscpd.mjs` | Duplication detection (jscpd) — wired into Step 5 |
| `scripts/audit/boyscout-delta.mjs` | Boy Scout Rule enforcement — compares per-file violation counts at merge-base vs HEAD; wired into Step 7 |
| `scripts/audit/dead-exports.mjs` | Whole-codebase unused-export scan — wired into Step 8 |
| `scripts/audit/test-smells.mjs` | Test-quality scan (focus leaks, time flakes, tautologies, console leaks) — wired into Step 9 |
| `forge_sweep` | Lighter-weight marker scan (TODO/FIXME only); this skill is the comprehensive version |
| `/code-review` skill | **Run `/clean-code-review` FIRST, then `/code-review`.** This skill is the mechanical/quantitative pass (LOC, complexity, params, duplication, ESLint). `/code-review` is the qualitative/judgment pass (architecture, security, patterns, tests). Mechanical findings clear the noise so the human-judgment review can focus on what actually requires judgment. |

## Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

- **Author:** [srnichols](https://github.com/srnichols)
- **Source:** [srnichols/plan-forge](https://github.com/srnichols/plan-forge)
- **License:** MIT

Install and usage instructions live in the source repository linked above.

## Pricing

- **Free** — Free

## Security capabilities

Automated source analysis of v0.1.0 — what this tool can access:

- **Network access:** no
- **Filesystem access:** no
- **Shell / process execution:** no
- **Environment & secrets:** no
- **Dynamic code execution:** no

*"Yes" means the capability is present in the source — more access means more to trust, not that it is unsafe.*


## Versions

- **0.1.0** — security scan: passed — Imported from the upstream source.

## Links

- Listing page: https://agentstack.voostack.com/l/skill-srnichols-plan-forge-clean-code-review
- Seller: https://agentstack.voostack.com/s/srnichols
- Browse the marketplace: https://agentstack.voostack.com/browse

---
Listed on AgentStack — the marketplace for AI agent skills and MCP servers. Every listing is security-reviewed. Creators keep 70%.