code-simplifier

# Code Simplifier Cleanup pass over a session's changes or a named target. Reviews the code in scope, understands how it fits the existing codebase, and makes targeted improvements — modernizing syntax, removing unnecessary complexity, consolidating duplicated logic, catching efficiency issues. Prioritizes codebase cohesion over local perfection. ## Core philosophy **Every change must earn its keep.** A simplification that doesn't meaningfully improve clarity, correctness, or cohesion is noise. Don't refactor for refactoring's sake. Don't create new files, abstractions, or utilities unless they solve a demonstrated problem. If the existing code works and is readable, leave it alone. The goal is a cohesive codebase, not a pristine one. ## Procedure ### Phase 1: Set the scope Two scopes; the caller's wording picks one, and the diff is the default. - **Diff** (nothing named): run `git status` to see the shape of the working tree, then `git diff HEAD` for all uncommitted changes (staged and unstaged). Untracked files never appear in the diff — list them with `git ls-files --others --exclude-standard` (`git status` collapses a new directory to one line) and read them directly. If the diff is empty and there are no untracked files, review the last commit (`git diff HEAD~1 HEAD`); if that is also empty, say the tree is clean and stop. Don't go hunting through the codebase for files to improve. - **Target** (a named path, module, or "the whole codebase"): the named files are the scope, whatever their git state. Work one module or directory at a time and re-run the gate after each, so a large scan never becomes one unverifiable diff. Take the target as named — don't rank or narrow it by commit history. ### Phase 2: Understand the surrounding codebase Don't review changes in isolation. Before any modifications: 1. **Read the full files** containing changes — not just the diff hunks. Understand imports, surrounding logic, module structure. 2. **Identify the project language(s)** and select the relevant transformation rules. Discard inapplicable rules. 3. **Survey adjacent code** — shared utilities, sibling modules, common patterns. You need to know what already exists before deciding something is missing. 4. **Run the project's gate once before editing** to establish a baseline. Use the gate the project's `CLAUDE.md` / `AGENTS.md` names; absent one, take it from `package.json` scripts — `devcheck` if present, else `check`, else the separate `typecheck` / `lint` scripts — or a `Makefile` `check` target. Python projects gate on `uv run ruff check`, `uv run ruff format --check`, and the configured type checker. Add the test suite when the gate doesn't run it — read the script rather than assume (a `devcheck` often stops at lint and typecheck); without tests, nothing shows behavior survived the pass. In a Bun project that tests with Vitest, run `bun run test` — bare `bun test` bypasses the script and runs Bun's own runner. If the gate is already red, say so in the summary and don't attribute the failure to your changes. ### Phase 3: Review Evaluate the changes across these dimensions. Not every dimension applies to every diff — skip what's irrelevant. #### Codebase cohesion - **Reuse** — Search for existing utilities, helpers, and patterns that could replace newly-written code. Check utility directories, shared modules, and files adjacent to the changed ones. If a function already exists that does what the new code does, use it. - **Consolidation** — Flag copy-paste-with-variation: near-duplicate code blocks that should be unified. Only unify if the shared abstraction is genuinely simpler than the duplicated code. - **Consistency** — Check that new code follows the same patterns as the rest of the codebase: naming conventions, error handling style, import patterns, type annotation style. Normalize toward the better variant when the project is inconsistent. - **Stringly-typed code** — Flag raw strings where constants, string-union types, or branded types already exist in the codebase. #### Code quality - **Redundant state** — State that duplicates existing state, cached values that could be derived. - **Unnecessary complexity** — Deep nesting that could be guard clauses, premature abstractions, over-engineered solutions to simple problems. - **Speculative generality** — Options, parameters, config flags, generic type parameters, and branches that no caller exercises. Flexibility for a hypothetical caller is cost paid now: remove it, and let the first real use add it back. On a published package's public surface it is API — note it instead (see Dead code). - **Pass-through layers** — Apply the deletion test to a wrapper, helper, or module: if deleting it and inlining its body makes the complexity vanish, it was a pass-through — inline it. If the same logic would reappear across several callers, it earns its keep. An interface, port, or injected dependency with a single implementation and no test double is a hypothetical seam, not a real one — collapse it until something actually varies across it. - **Test-only reach** — A function extracted or exported only so a test can get at it is a shape problem, not a cleanup: name it in the summary with the module it belongs to. Don't restructure it here — the tests would have to move with it. - **Dead code** — Unreachable branches, unused variables, commented-out code, and debug leftovers from the session (`console.log`, `print`, `debugger`) that aren't the program's real output or the project's logger. An export nothing imports is dead in an application or a package-internal module; on a published package's public surface it is API — leave it and note it in the summary. - **Defensive code for impossible states** — Guards for cases the type system or upstream validation already prevents. Drop them. - **Type escapes** — `any`, `as` casts that paper over a mismatch, non-null `!`, `@ts-ignore`, and Python's `# type: ignore` / `cast()`. Each is a claim the compiler couldn't check: replace with a narrowed type, a type guard, or a parse at the boundary. Keep the ones documenting a genuine type-system or third-party-types limitation — confirm the limitation is gone before removing one — and prefer `@ts-expect-error` with a one-line reason over `@ts-ignore`. - **Swallowed errors** — Empty `catch {}`, `catch { return null }`, and `try` blocks that log and continue. A fallback that hides a failure is worse than the crash it prevents: rethrow or let it propagate. When wrapping, preserve the chain (`new Error(msg, { cause })`, `raise X from err`). - **Masking defaults** — `?? ''`, `|| []`, `?? 0`, `.get(key, {})` standing in for a value that must exist. The default turns a missing config key or a broken upstream into quietly wrong output further down. When the type already rules out absence, the default is dead — drop it; when the value is optional in the type but required in fact, replace the default with an error that names what's missing, where the value is read. Keep defaults only where absence is a legitimate, expected state. - **Comment noise** — Strip comments that restate the code and comments describing behavior the diff removed. Keep file headers, export JSDoc, and any comment carrying a *why* — a constraint, a workaround, an upstream bug reference. - **Outdated patterns** — Verbose or legacy syntax where modern equivalents exist. See the transformation tables below. #### Efficiency - **Redundant work** — Repeated computations, duplicate file reads, duplicate network/API calls, N+1 query patterns. - **Missed concurrency** — Independent async operations run sequentially that could run in parallel with `Promise.all` / `Promise.allSettled`. - **Unbounded fan-out** — `Promise.all` / `asyncio.gather` over a caller-sized or otherwise unbounded array fires everything at once. Cap it with the project's existing concurrency helper or a batched loop. A fixed handful of independent calls needs no limit. - **No-op updates** — State/store updates inside loops or event handlers that fire unconditionally. Add change-detection so downstream consumers aren't notified when nothing changed. - **TOCTOU** — Pre-checking file/resource existence before operating on it. Operate directly and handle the error instead. - **Overly broad operations** — Reading entire files when only a portion is needed, loading all items when filtering for one. #### mcp-ts-core-specific - **Gate** — `bun run devcheck` plus the test suite (`bun run test`) is the project gate in Phase 2 step 4 and Phase 4 step 5. - **Framework-provided utilities** — Before hand-rolling, check `src/utils/` and `src/errors/` in the project and `node_modules/@cyanheads/mcp-ts-core/` for framework exports: pagination helpers, schema builders, retry primitives, and the `ATTR_*` OTel attribute constants are framework-provided. Raw OTel attribute keys should be `ATTR_*` imports from `@cyanheads/mcp-ts-core/utils`. - **Error throwing patterns** — Prefer framework error factories (`McpError`, `validationError`, `notFound`, `httpErrorFromResponse`) over raw `throw new Error()`. Tool handlers should throw — the framework catches, classifies, and instruments. - **Error codes** — `InvalidParams` only for malformed JSON-RPC params shape. `ValidationError` for domain validation. `NotFound` for missing entities. Don't conflate them. - **Ctx usage** — Use `ctx.log`, `ctx.state`, `ctx.enrich` — don't reach for global loggers or request-scoped storage directly. The `ctx` pattern carries tenant scope and OTel context. - **Zod schemas** — Every tool input/output field needs `.describe()`. Zod 4 requires `z.record(z.string(), z.string())` not `z.record(z.string())`. Use `.optional()` rather than `.nullish()` unless null is semantically distinct from absent. - **Tool annotations** — `readOnlyHint`, `idempotentHint`, `openWorldHint` should reflect reality. A read-only tool with `readOnlyHint: false` gives clients the wrong picture. - **`exactOptionalPropertyTypes` boundaries** — If a downstream type insists on the field being present-or-not-present (not present-as-undefined), use a mapped widening type at the boundary. The pattern is documented in the framework. - **`format()` ↔ `structuredContent` parity** — Different MCP clients forward different surfaces. Tests should assert both surfaces carry equivalent data. - **Framework layering is not a pass-through** — the init/accessor pair (`initFooService()` / `getFooService()`), the tool definition → service split, and a provider interface the framework selects by config are prescribed convention; the deletion test doesn't apply to them, and a single-implementation service accessor is the framework's seam, not a hypothetical one. - **Defensive code** — the "impossible states" the framework already prevents include malformed params (Zod-validated before the handler runs) and unclassified errors (caught and classified after it throws). Guards for either are dead. - **Public surface** — the MCP surface (every tool input/output schema advertised to clients) is public API for the "API compatibility" rule; changing one is a breaking change, not a refactor. ### Phase 4: Apply transformations 1. **Filter findings ruthlessly.** If a finding is a false positive or not worth the churn, skip it. Don't argue with yourself about borderline cases — move on. 2. **Stay in scope.** Edit only files inside the Phase 1 scope — the diff plus files new this session, or the named target. Touch a file outside that set only when a finding requires it — importing an existing helper, deleting a private export the diff just orphaned — and only on the lines that finding names. Anything broader goes in the summary as a recommendation, not into the tree. 3. **Correctness bugs are not this pass's job.** A real defect doesn't get folded into a cleanup diff — name it in the summary with file and line so it can be handled as its own change. 4. **Transform incrementally** — one category of change at a time (modernize syntax, then reduce nesting, then consolidate). 5. **Verify equivalence** — all functionality, types, and public interfaces must remain unchanged. Re-run the gate from Phase 2 after transforming; a simplification that breaks the build is worse than the verbosity it removed. 6. **Keep the diff minimal.** Only touch lines that have a real reason to change. Don't reformat untouched code, add comments to code you didn't modify, or "improve" things that are already fine. Formatting belongs to the formatter (Biome, ruff): never hand-adjust whitespace, quotes, or import order. Hunks the project's formatter writes during a gate run stay, even outside the scope — reverting them only fights the next run; mention them in the summary. 7. **Never stage, commit, tag, push, or stash.** This pass ends with a dirty working tree and a summary; landing the changes is the caller's call. A stash hides the very changes under review — compare against the baseline with `git diff`, never by setting work aside. When done, report: - **Gate** — the result before and after the pass, so a failure that predates the cleanup isn't pinned on it. - **Fixed** — what changed, grouped by category. - **Skipped** — findings deliberately left, each with its reason. - **Defects and recommendations** — correctness bugs and out-of-scope changes, each with `file:line`. When nothing earned a change, say the code was already clean. ## Common transformations The tables below cover TypeScript and Python. For other languages, apply analogous principles: prefer modern idioms, reduce nesting, eliminate dead code, follow project conventions. Check the project's language floor (`tsconfig` target/lib, `pyproject` `requires-python`) before applying a version-gated row. ### TypeScript (modern ESM) | Before | After | Why | | --- | --- | --- | | `const x: Foo = { ... } as Foo` | `const x = { ... } satisfies Foo` | Type-checked without assertion | | `let resource = acquire(); try { ... } finally { release(resource) }` | `using resource = acquire()` | Explicit resource management (TS 5.2+) — only when the resource implements `Symbol.dispose` (`await using` for `Symbol.asyncDispose`); otherwise the `try`/`finally` stays | | `if (x !== null && x !== undefined)` | `if (x != null)` | Idiomatic null/undefined check | | `arr.filter(x => x !== null) as T[]` | `arr.filter(x => x != null)` | TS 5.5+ infers the type predicate — no cast; on older TS use an explicit `(x): x is T` predicate | | `import { foo } from './index.js'` (a module importing through its own barrel) | `import { foo } from './foo.js'` | Inside a module, import siblings directly — routing through the module's own barrel invites import cycles. Across modules, the public barrel is the right entry point; leave those imports alone | | `import { readFile } from 'fs/promises'` | `import { readFile } from 'node:fs/promises'` | `node:` protocol — unambiguous, lint-enforced in Biome | | `async function f() { const a = await x(); const b = await y(); }` | `const [a, b] = await Promise.all([x(), y()])` | Parallel when independent | | `value \|\| fallback` | `value ?? fallback` | `\|\|` also swallows `0`, `''`, and `false` — use `??` unless every falsy value really should take the fallback | | `obj.x !== undefined ? obj.x : fallback` | `obj.x ?? fallback` | Nullish coalescing — equivalent only when `null` should take the fallback too | | `if (a) { if (b) { if (c) { ... } } }` | Guard clauses with early returns | Reduce nesting | | `try { risky() } catch (e: any) { ... }` | `try { risky() } catch (e) { ... }` | Under `strict` the catch binding is already `unknown`; narrow with a type guard before use | | `catch (err) { throw new Error('load failed') }` | `throw new Error('load failed', { cause: err })` | Preserve the cause chain | | `[...arr].sort(cmp)` / `arr.slice().sort(cmp)` | `arr.toSorted(cmp)` | Non-mutating array methods (ES2023) — also `toReversed`, `toSpliced`, `with` | | `arr[arr.length - 1]` | `arr.at(-1)` | ES2022 — typed `T \| undefined`: equivalent under `noUncheckedIndexedAccess`, a new `undefined` to handle otherwise | | `arr.reduce((acc, x) => { (acc[key(x)] ??= []).push(x); return acc }, {})` | `Object.groupBy(arr, key)` | ES2024 — returns a null-prototype object whose values are typed `T[] \| undefined`; `Map.groupBy` for non-string keys | | `let resolve!: (v: T) => void; const p = new Promise<T>((r) => { resolve = r })` | `const { promise, resolve, reject } = Promise.withResolvers<T>()` | ES2024 — the deferred without the captured-variable dance | | `new Set([...a].filter((x) => b.has(x)))` | `a.intersection(b)` | ES2025 `Set` methods — also `union`, `difference`, `symmetricDifference`, `isSubsetOf`; the receiver must be a `Set` | | `const c = new AbortController(); setTimeout(() => c.abort(), ms)` | `AbortSignal.timeout(ms)` | Built-in timeout signal; combine with a caller's signal via `AbortSignal.any([...])` | | `JSON.parse(JSON.stringify(x))` | `structuredClone(x)` | Deep clone that keeps Date, Map, Set, and cycles. Not a drop-in: it throws on functions and strips class prototypes, and Dates stay Dates instead of becoming strings | | `enum Status { A, B, C }` | `const Status = { A: 'A', B: 'B', C: 'C' } as const` | `enum`, `namespace`, and constructor parameter properties are non-erasable syntax rejected by TS 5.8 `erasableSyntaxOnly` and Node type-stripping — but switching numeric values to strings changes serialized output; keep values stable if they're persisted | | `function f(a: string, b: string, c: string, d?: string)` | `function f(opts: FnOptions)` | Internal functions whose same-typed positional params can be swapped and still type-check. An exported signature is API — leave it | | `throw new Error('Bad input')` (in a tool handler) | `throw validationError('Bad input', { field: 'x' })` | Use framework error factories so the framework can classify and instrument | | `const ATTR_KEY = 'mcp.tool.name'` | `import { ATTR_MCP_TOOL_NAME } from '@cyanheads/mcp-ts-core/utils'` | Use framework attribute constants | ### Python (3.12+) | Before | After | Why | | --- | --- | --- | | `Optional[str]` | `str \| None` | Modern union syntax (3.10+) | | `List[str]`, `Dict[str, int]` | `list[str]`, `dict[str, int]` | Built-in generics (3.9+) | | `T = TypeVar("T")` + `def f(x: T) -> T` | `def f[T](x: T) -> T` | PEP 695 generics (3.12+) — also `class C[T]:` |