Code Audit

Code Audit Patterns

Recurring smell categories worth hunting periodically. Each category has a grep pattern, a real example from the repo (if one exists), and a "why it matters" so you know whether a hit is a real smell or expected.

Related skills: See post-implementation-review for the full second-read ritual after implementation. See refactoring for the methodology of fixing what you find. See one-sentence-test for the cohesion audit that frames whether an abstraction earns its keep.

The categories below were validated against the actual codebase by repeated agent audits. They're not generic style nits: each one signals a specific kind of contract problem in a TypeScript-heavy framework codebase.

1. Duck-Typing at System Boundaries

Pattern: (x as { foo?: unknown }).foo or as Record<string, unknown> accesses where the shape isn't statically known.

grep -rn "as\s*{" packages apps --include="*.ts" -B2 -A2

Why it matters: Each duck-type is a contract gap. The receiver doesn't know what it's getting; the producer doesn't know what's expected. Often justified at literal system boundaries (e.g., parsing user-provided configs, reading from third-party APIs), but unjustified inside the framework's own code.

Example (justified): a loader that duck-types fields off a user-authored module it imports at runtime, because the module's shape is arbitrary by design. What makes it justified is the comment block above the access explaining exactly why static typing is impossible there.

Example (unjustified): if (entry.handle.whenReady) was duck-typing whenReady until it became a typed optional on Document. The diagnostic told us the contract was incomplete; the fix was to declare the field.

How to triage a hit: read the ~30 lines above the duck-type. If there's a comment explaining why static typing isn't possible at this boundary, leave it. If there's no explanation and the producer is an internal type, either widen the producer's type to include the field, or consolidate the access into a duck-typing helper that's itself well-documented.

2. Promise-Shape Ceremony (.then(() => {}))

Pattern: explicit value-discarding tails on promise chains.

grep -rn "\.then\s*\(\s*(\(\)|=> \{\}|=> undefined)" packages --include="*.ts"

Why it matters: usually indicates the type at the receiving end is too narrow. Authors write .then(() => {}) to convert Promise<T> into Promise<void> because the field accepting the promise is typed Promise<void> instead of Promise<unknown>. The ceremony adds zero semantic value.

Recipe: when you see this pattern, look at where the promise is being assigned. If the consumer awaits-and-discards (typical for readiness/teardown barriers), widen the field type to Promise<unknown>. The tail disappears.

Example fix: this codebase's readiness fields such as whenReady, whenLoaded, and whenConnected were widened from Promise<void> to Promise<unknown> exactly to eliminate this ceremony. See spec specs/20260424T000000-self-gating-attachments.md for the rationale.

False positive: if the .then(() => result) returns a meaningful transformed value, leave it. The smell is specifically the "discard the value" form.

3. Unstructured Logging in Library Code

Pattern: console.log / console.error / console.warn outside of CLIs, tests, and benchmarks.

grep -rn "console\.(log|error|warn)" packages/workspace packages/sync --include="*.ts" | grep -v test

Why it matters: library code that calls console directly can't be redirected, silenced, or piped to a custom sink (in-memory test introspection, telemetry shipping). Every console.log is a hardcoded output decision the consumer can't override.

The framework's logger: wellcrafted/logger provides createLogger(name) plus pluggable sinks (consoleSink, memorySink, composeSinks). Three established injection patterns:

// Module-scoped (default)
const log = createLogger('module-name');

// Optional parameter (for libraries that may want a caller-supplied logger)
function createX({ log = createLogger('x') }: { log?: Logger } = {}) { ... }

// Config-injected (used in openCollaboration)
const log = config.log ?? createLogger('collaboration');

Triage: this category is verified periodically: the codebase has historically been clean. Treat any hit as a regression and route through wellcrafted/logger before merging. New console.* in library code should be refused at PR review unless the call site is explicitly a CLI command, test, or benchmark.

Two evasions the console\. grep misses. Both import from wellcrafted/logger and are therefore invisible to the pattern above, and both were found live in apps/whispering:

grep -rn "consoleSink({" apps packages --include="*.ts" --include="*.svelte"
grep -rn "log\.(warn|error)(new Error" apps packages --include="*.ts" --include="*.svelte"

A direct consoleSink({ ... }) call skips createLogger, and with it the Logger type that makes warn/error unary over a LoggableError. Whispering's $lib/report did this and grew a parallel log whose extra data argument silently discarded the error object. The only legitimate mention of consoleSink is composing it into a sink passed to createLogger.

A new Error(...) inside a warn/error call type-checks (native Error satisfies LoggableError structurally) but hands the sink a message string and no filterable name. It usually means the boundary above types its failure callback (cause: unknown) and ships no vocabulary, so each implementer invents one. Fix it at the boundary: export the defineErrors variants from the file that declares the callback. See the logging skill.

4. Exhaustive never Checks as Union-Churn Signals

Pattern: const _exhaustive: never = x after a switch on a discriminated union.

grep -rn ": never\s*=" packages --include="*.ts" -B10

Why it matters: each never check is a contract promise: "if this union grows a variant, every consumer with this check will break." That's good: it's the type system catching missed cases. But if adding a single variant breaks five different switches, the variants are probably overlapping, the union is too broad, or the same logic is being implemented in too many places.

Example (justified): one canonical formatter that exhaustively switches over a tagged error union with well under five variants, which every other consumer delegates to rather than re-implementing. Single point of enforcement, healthy use of the pattern.

Triage: count call sites on the discriminated type. If 1-2 switches enforce exhaustiveness, fine. 5+ is a smell: consider:

• Moving the switch into a method on the union members themselves (each variant exports its own behavior).
• Splitting the union if some switches only care about a subset of variants.
• Extracting a single canonical handler that other consumers delegate to.

False positive: type-narrowing on a result type at a boundary is healthy, even if there are several. Look for the pattern of "every consumer has to know about every variant" as the actual signal.

5. Single-Method Pick Dependencies

Pattern: dependency injection shaped as Pick<Thing, 'method'>.

rg "Pick<[^>]+,\s*['\"][^'\"|]+['\"]\s*>" packages apps

Why it matters: a single-method pick can keep an old object boundary alive after the caller only needs one operation. The type looks narrow, but the object name still tells readers to think about the whole capability family.

Example fix: packages/workspace/src/daemon/unix-socket.ts used Pick<Hono, 'fetch'> for the socket binder. The binder's job is "bind one request handler to a unix socket and harden the socket file", so the dependency became a UnixSocketRequestHandler function instead of a Hono-shaped object.

Triage: write the one-sentence job of the caller and mentally inline the picked method. Keep the object only when that sentence names the object or the caller coordinates the object's life cycle. If the sentence only names one verb, accept a named capability function in the caller's language and update tests to fake that function. Do not replace Pick<Thing, 'method'> with Thing['method'] unless Thing is still the caller's real concept.

False positive: Pick is fine for data projection, DTO trimming, and multi-field view models. A single non-method field like Pick<Session['session'], 'expiresAt'> is not this smell.

6. Copied TypeScript Boundary Shapes

Pattern: local helper types that copy upstream shapes instead of deriving from the owner.

rg "type\s+\w+Like\b|interface\s+\w+Like\b" packages apps
rg "as\s+\w+Like\b" packages apps
rg "as\s+Record<string, unknown>" packages apps
rg "Pick<[^>]+,\s*['\"][^'\"|]+['\"]\s*>" packages apps
rg "Parameters<typeof\s+[^>]+>\[[0-9]+\]" packages apps

Why it matters: copied shapes create two sources of truth. A Like type, single-method Pick, or Parameters<typeof fn>[n] contortion can look precise, but often says "this layer knows an upstream object exists and only wants one piece of it." That is a boundary leak. Either derive from the owning runtime type, schema, factory, or function signature, or name the one capability the caller actually needs.

Triage: read nearby context before judging. Classify each hit as:

• justified boundary: external input, protocol compatibility, or a real shared contract
• test fake only: acceptable when contained in *.test.ts or setup helpers
• refactor candidate: internal code copies a local upstream shape or production code widened only to satisfy a test seam
• false positive: data projection, DTO trimming, or a named capability that is the actual caller-owned contract

Recipe: clear candidates usually collapse one of four ways: derive the type from the owner, replace object-shaped dependency injection with a named capability function, move incomplete fake objects into tests with satisfies, or delete one-property option aliases that only rename the function signature.

False positive: Parameters<typeof fn>[n] is fine when it derives a public helper type from a stable exported function. It becomes a smell when tests use it to reverse-engineer an unnamed seam, or when the index gymnastics hide the fact that the caller wants a smaller named capability.

7. Non-Exhaustive Union Folds (if/else Where a switch Belongs)

Pattern: an if (x.type === '...') / else chain that folds an entire closed discriminated union (every branch maps a variant to a different output) with no satisfies never pin. The inverse of category 4.

grep -rn "\.error\.name === '" packages apps --include="*.ts" | grep -v test

Why it matters: the else silently absorbs any variant added later; a switch with default: x satisfies never makes the producer's new variant break the consumer's build instead.

Validated: a sweep of workspace/sync/cli/api found four (run-handler.ts, run.ts, list.ts, materializer.ts), all fixed. Re-run after adding variants to a wire or IPC error union.

The pattern itself lives elsewhere: the rule, the predicate/guard/fold triage, and the before/after are in the define-errors skill ("Consuming a Variant Union"). This entry is only the detection recipe: use it to find hits, use that skill to classify them.

What This Skill Doesn't Catch (Reject from the Hunt)

Tested and rejected as not-actually-smells in this codebase:

• Dead kind: branches: exhaustiveness via never enforces this aggressively. No dead branches survive.
• Dead exported types: many exports across packages/workspace; spot-checks always hit consumers. Would need deeper dependency analysis to find real dead ones.
• Stale file names (*-manager.ts, *-handler.ts): none found. Naming matches responsibility.
• Async wrappers doing no work: Yjs is sync-friendly; the codebase doesn't fake async.
• Casual @ts-expect-error / @ts-ignore: ~234 of them, but all justified (test doubles, duck-typing at system boundaries with comments).

If a future audit finds patterns consistent with these categories, they should be added here. If a category proves false (zero hits across multiple sweeps), demote it.

Workflow

When kicking off a review pass:

1. Run the greps against the relevant scope (single package, full monorepo, or recently-changed files).
2. For each hit, read the ±5 lines of context and decide: justified, refactor, or false-positive.
3. Group findings by category and impact before fixing: refactoring scattered hits one-at-a-time loses the pattern.
4. Document the fix in a single PR with the audit log. The pattern matters more than the individual instances.

The goal is systematic detection, not ad-hoc cleanup. These patterns repeat; codifying them here means future review passes don't re-derive them.