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 — read new files 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. Find it in `package.json` scripts — `devcheck` if present, else `check`, else the separate `typecheck` / `lint` / `test` scripts; Python projects gate on `uv run ruff check`, `uv run ruff format --check`, and the configured type checker and test runner. 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. - **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. 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 `!`, and `@ts-ignore`. 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, 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`). - **Comment noise** — Strip comments that restate the code, commented-out 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, and never let a formatting-only hunk into the diff. 7. **Never stage, commit, tag, or push.** This pass ends with a dirty working tree and a summary; landing the changes is the caller's call. When done, briefly summarize what was fixed, what was deliberately skipped, and any defects or out-of-scope recommendations — or confirm 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, TS 5.x+) | 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 disposal (TS 5.2+) | | `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 | | `export { foo } from './foo/index.js'` | Direct imports at call sites | Avoid barrel re-exports inside the package; barrel exports are for public APIs only | | `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` | | `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 preserves Date, Map, Set, and cycles | | `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)` | Options object when >3 params | | `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]:` | | `TypeAlias = Union[A, B, C]` | `type ABC = A \| B \| C` | `type` statement (3.12+) | | `if isinstance(x, Foo): a = x.a; b = x.b` | `match x: case Foo(a=a, b=b): ...` | Structural pattern matching (3.10+) where it destructures — not as a replacement for a flat equality `if/elif` chain | | `class Config: def __init__(self, a, b, c): self.a = a ...` | `@dataclass(slots=True) class Config: a: str; b: int; c: float` | Less boilerplate, built-in eq/repr; `frozen=True` when instances shouldn't mutate | | `results = []; for item in items: results.append(transform(item))` | `results = [transform(item) for item in items]` | Idiomatic comprehension | | `f = open('x'); try: ... finally: f.close()` | `with open('x') as f: ...` | Context manager for resources | | `os.path.join(d, n)`, `os.path.exists(p)`, `open(p).read()` | `Path(d) / n`, `p.exists()`, `p.read_text()` | `pathlib` over `os.path` string juggling | | `datetime.utcnow()` / `datetime.utcfromtimestamp(t)` | `datetime.now(UTC)` / `datetime.fromtimestamp(t, UTC)` | Deprecated in 3.12 — the old calls return naive datetimes that compare wrong against aware ones | | `zip(a, b)` | `zip(a, b, strict=True)` | 3.10+ — silently truncating to the shorter input hides bugs | | `m = pattern.match(s)` then `if m: use(m)` | `if (m := pattern.match(s)): use(m)` | Walrus operator where it removes a throwaway assignment | | `"Hello " + name + "!"` | `f"Hello {name}!"` | f-string over concatenation | | `except Exception as e: pass` | `except SpecificError as e: log(e)` | Catch specific, never bare except/pass | | `from module import *` | `from module import specific_name` | Explicit imports only |