Skip to content

Make npm run build exit 0 by declaring the types the code already depends on - #818

Draft
kriszyp wants to merge 7 commits into
mainfrom
build/typecheck-clean-exit-zero
Draft

Make npm run build exit 0 by declaring the types the code already depends on#818
kriszyp wants to merge 7 commits into
mainfrom
build/typecheck-clean-exit-zero

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 4, 2026

Copy link
Copy Markdown
Member

npm run build (tsc --project tsconfig.json) exited 2 on a clean checkout of main while still emitting a complete dist/, so the build's exit code was not a usable success signal for humans or agents. All 31 diagnostics were declaration gaps rather than code defects: the repo depended on a private ws field, on errno-shaped socket errors, on pprof-format's narrower runtime reality, and on a Node 22 lib, without declaring any of them. This declares each one at its own layer, and changes no runtime statement — every emitted dist/**/*.js file is byte-identical to the pre-change build.

diagnostics cause change
17 × TS2339 _socket replication's keep-alive watchdog and blob-send backpressure read ws's private _socket; @types/ws declares only the public surface one file-local ReplicationWebSocket applied at the four transport ingress boundaries — createWebSocket's return, the stored socket field, replicateOverWS's parameter, and the two test fault-injection helpers — so all 17 reads sit downstream of it
4 × TS2339 code / isHandled ws types its 'error' listener as (err: Error); the handler reads an errno code and sets Harper's isHandled flag annotate that one listener parameter
9 × TS2345 / TS2365 pprof-format declares sample fields as number | bigint; the profiler does number arithmetic on them as number, the idiom the same function already uses on the neighbouring reads
1 × TS2550 Promise.withResolvers harper-pro's tsconfig set no lib, so it got TypeScript's ES2022 default while pinned core code calls a Node 22 API add lib = that same default set plus ES2024.Promise; target is untouched

A unit test pins the ws internals the new type declares, and @types/ws becomes a direct devDependency — production code imports ws, but its types were arriving transitively through the dev-only mqtt.

For the human reviewer

The planning gate changed the design, and that is the thing most worth a second opinion. The obvious shape — a single declare module 'ws' augmentation adding _socket — does not compile. @types/ws@8.18.1 exports the class with export =, so import { WebSocket } from 'ws' resolves to the class instance type, which an interface augmentation cannot merge into; a minimal probe still reported TS2339 on every site. The planning review returned Framing-Verdict: better-alternative-exists and I adopted its alternative: a replication-scoped structural refinement. It has a second benefit the augmentation lacked — Harper's stronger net.Socket assumption stays inside replication, where upstream ws only promises a Duplex.

Open decisions, none of them forced by this change:

  • lib = default-equivalent, not minimal. With target: ES2022 and no lib, TypeScript loads lib.es2022.full.d.ts — exactly the six libs now listed. So this adds only ES2024.Promise to the surface main already compiles against. A reviewer suggested trimming to ES2022 + ES2024.Promise to drop the DOM globals; that is a project-wide type-surface change and should not ride on a build-exit-code PR.
  • _socket: Socket | null. ws initialises it to null pre-handshake, so the union is accurate, but strict: false erases it today — it guarantees nothing until someone enables strictNullChecks, at which point it surfaces the three pre-existing non-optional reads as the real questions they are.
  • as number rather than Number(...) in the profiler, because a coercion would change emitted JavaScript. The assertion keeps the (pre-existing) bigint path latent if a sample ever exceeds safe-integer range.
  • The ws contract test proves the library, not replication's use of it, and it is the only unit test in the suite that binds a socket. It exists because an exact version pin is still bumped by routine dependency PRs, and a reviewer of such a PR has no signal today that replication reads a private field.

Three review findings I overruled rather than fixed, each on a fact rather than a preference:

  • "DOM in lib pollutes the global namespace, so a forgotten import { WebSocket } from 'ws' silently resolves to the browser global and drops TLS options." With target: ES2022 and no lib, TypeScript loads lib.es2022.full.d.ts, which is exactly ES2022 + DOM + DOM.Iterable + DOM.AsyncIterable + WebWorker.ImportScripts + ScriptHost — the six now listed. main compiles against those globals today; this diff adds only ES2024.Promise.
  • "as assertions are incompatible with Node type-stripping and will throw SyntaxError." node v26.2.0 runs a .ts file containing as number[] and as number without complaint, erasableSyntaxOnly exists precisely to permit them, and analytics/profile.ts:100,103 already ships two.
  • "A rocksdbBackup.ts:899 TS2554 means the new lib does not deliver exit 0." Compiling with main's own tsconfig.json — no lib at all — reproduces that diagnostic identically against the same installed tree. It is dependency-tree drift in the reviewer's checkout, not this change.

No CI type-check gate is added, deliberately. Whether main should be gated on tsc exiting 0 is a separate team decision, and bundling it would make this change unreviewable. Note also that build-tools/build-pro.sh and the unit-test CI job still call npm run build || true; only direct callers gain a reliable exit signal from this change.

Verification

Not observable end-to-end by design — the change emits no different JavaScript, so the end-to-end route is the build itself plus a byte-comparison proving that.

  • npm run buildexit 0 (was exit 2 with 31 errors on main); npx tsc --noEmit → 0 errors.
  • Emitted output compared against a dist/ snapshot built from main: 1033 files, identical file list, every non-map file byte-identical. Only the two touched sources' .js.map differ, because sourceMap is on and erased as/annotation syntax shifts source columns.
  • npm run test:unit886 passing (882 on main, plus the 4 new ws contract tests).
  • npm run lint:required clean, prettier --check clean on the changed files.

Pre-existing and left alone: getUserHitCount in analytics/profile.ts accumulates totalHarperCount once per harper stack frame (that branch has no return, unlike the user branch above it) while totalUserCount accumulates once per sample, so the cpu-usage/harper metric is inflated by stack depth. Found during this review, unrelated to the type change, and fixing it would be a runtime change.

Complexity: easy

Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=7 @ 1e4302c

Human-Review-Need: 4 @ 1e4302c

kriszyp and others added 6 commits September 4, 2026 09:33
`tsc --project tsconfig.json` exited 2 on a clean checkout of main with 31 errors while
still emitting a complete dist/, so the build's exit code was not a usable success signal.
Three families, all declaration gaps rather than code defects:

- 17 TS2339 on the `ws` library's private `_socket`, which replication's keep-alive
  watchdog and blob-send backpressure both read. Declared once as a replication-scoped
  `ReplicationWebSocket = WebSocket & { _socket: Socket | null }` applied at the four
  transport ingress boundaries, so all 17 reads type without per-site casts. (A
  `declare module 'ws'` augmentation does not work: @types/ws exports the class via
  `export =`, so the imported type is the class instance type, which an interface
  augmentation cannot merge into.)
- 4 TS2339 on `error.code` / `error.isHandled` in the socket 'error' handler, fixed by
  annotating that one listener parameter.
- 9 TS2345/TS2365 in analytics/profile.ts, where pprof-format declares sample fields as
  `number | bigint`; asserted `as number`, the idiom the same function already uses.
- 1 TS2550 for `Promise.withResolvers` in pinned core code; harper-pro's tsconfig had no
  `lib`, so it defaulted to ES2022. Mirrors core/tsconfig.json's list, which already
  carries ES2024.Promise for this exact call. `target` is unchanged.

Also adds a unit test pinning the `ws` internals the new type declares, and declares
@types/ws directly (production code imports `ws`, but its types arrived transitively
through the dev-only `mqtt`).

Zero runtime change: all 1033 emitted dist/ files are byte-identical to the pre-change
build except the two touched sources' .js.map, whose mappings shift because erased
`as`/annotation syntax moves source columns.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013CwEcEqeKKoP4WcxFXWPaB
- record the @types/ws root edge in package-lock.json (npm install --package-lock-only);
  it resolved to the hoisted transitive copy, so package.json claimed a dependency the
  lockfile's root entry did not
- bound the ws contract test's socket waits: .mocharc.json sets timeout: 0, so a bind or
  connect failure would have hung the whole unit suite instead of failing this file
- correct the tsconfig comment (the lib list is TypeScript's default for target ES2022
  plus ES2024.Promise, which is the reviewable fact; "mirrors core" is not) and the
  test's header and one case title

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013CwEcEqeKKoP4WcxFXWPaB
onceOrFail left its 'error' listener attached after a successful wait, so a later socket
failure resolved into a no-op reject instead of surfacing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013CwEcEqeKKoP4WcxFXWPaB
Nothing outside replicationConnection.ts names the type, and an exported type under
TypeStrip is a footgun: a value-shaped `import { ReplicationWebSocket }` typechecks but
survives stripping and fails at runtime.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013CwEcEqeKKoP4WcxFXWPaB
…est header

Head-side line numbers in replicationConnection.ts rot on its next edit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013CwEcEqeKKoP4WcxFXWPaB
`sendAuditRecord`'s backpressure wait is not the only non-optional `_socket` read; the
open handlers' `unref()` are too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013CwEcEqeKKoP4WcxFXWPaB

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces TypeScript type safety improvements and compatibility updates. It defines a custom ReplicationWebSocket type to access the private _socket field of ws for keep-alive and backpressure handling, adds @types/ws as a dependency, updates tsconfig.json to support Promise.withResolvers(), and introduces a new unit test suite to pin the ws internals. The review feedback highlights a potential synchronous TypeError in the test cleanup hook if the server fails to initialize, recommending proper guards and try-catch blocks for robust resource cleanup.

Comment thread unitTests/replication/wsSocketInternals.test.mjs
If `before` fails before the server is constructed, an unguarded close() throws a
TypeError over the real failure.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013CwEcEqeKKoP4WcxFXWPaB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant