fix: pre-v0.6.0 release hardening#126
Merged
Merged
Conversation
Add pre-publish artifact verification to catch broken files/exports, wrong entry paths, or incorrect types resolution before 0.6.0 ships. Runs as a dedicated verify-package job on Node 22 LTS (artifact check, not runtime check) that needs the build matrix to succeed first. - npm pack --dry-run: confirm tarball manifest is producible - publint: lint package.json exports/main/module/types/files - @arethetypeswrong/cli: verify TS consumers (node16/bundler) resolve working types for both CJS and ESM entrypoints Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace Array(48) + pad()/parseInt()/substr() chain with Buffer.alloc(48) plus direct byte assignment and buf.writeBigUInt64BE for the origin and transmit timestamps. Drops the now-unused pad() helper. The new implementation splits the NTP 64-bit timestamp into whole seconds and fractional seconds before combining via BigInt so the fractional portion no longer loses low-order bits to the Number mantissa during the legacy (seconds * 2**32) float round-trip.
acceptResponse previously validated only originTimestamp, leaving transmitTimestamp, receiveTimestamp, and precision as non-null-asserted in collectSamples. Add explicit undefined checks for those fields in acceptResponse (throwing the same Format error pattern as existing checks) so malformed replies are rejected before they become samples, and drop the ! assertions in collectSamples. A guard in the forEach provides type narrowing that is now backed by the validator.
The previous ordering registered client.once("message") inside the
send() callback, so an NTP reply that arrived after the packet was on
the wire but before Node fired the send callback would hit no listener
and be silently dropped; the caller would then wait the full
replyTimeout before rejecting. Attach the message listener up front and
keep hasFinished/timeoutHandler guards so the timeout, send errors, and
socket errors still short-circuit the promise correctly.
The retry counter only advanced when ntpResults was still empty, so any partial success that produced fewer than numSamples usable packets (e.g. one server responding, the rest persistently timing out) could spin the do/while loop indefinitely because retry stayed at 0. Snapshot the result count before each round and bump retry when the post-round count is unchanged, keeping the existing retry < 3 cap.
Two related leaks: (1) dgram.send can throw synchronously when the socket is in a bad state, and the throw was not caught, so the timeout handler, UDP socket, and registered listeners all leaked and the surrounding promise was never rejected; wrap the send call so we clear the timeout, run NtpTimeSync.cleanup, and reject exactly once. (2) NtpTimeSync.cleanup only called client.close(), leaving "message" and "error" listeners attached on an abandoned socket where a late event could still fire resolve/reject or prevent the process from exiting; call client.removeAllListeners() before close().
….lock
The cache key used hashFiles('**/yarn.lock'), but yarn.lock is
gitignored and thus absent from CI checkouts. hashFiles returned an
empty hash, collapsing the key to a constant (e.g. Linux-modules--22)
that never invalidated across commits.
This caused verify-package to restore a stale node_modules from
before commit 1d20851 and skip install (cache-hit guard), so newly
added devDependencies publint and @arethetypeswrong/cli were missing:
/bin/sh: 1: publint: not found (exit 127).
Switch all three jobs (build, verify-package, deno) to key on
package.json, which is tracked and changes when deps change.
Applies the open Dependabot update (#128, prettier 3.8.3 -> 3.9.3) and bumps the ntp-packet-parser runtime dependency to 0.6.1. Verified with unit + integration tests, publint, and attw. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
attw 0.18.2's `--pack` mode parses `npm pack --json` internally and
crashes ("Cannot read properties of undefined (reading 'filename')") on
the npm bundled with node 22.23.1 in CI, while publint's own pack of the
same tree succeeds. Pack the tarball ourselves and hand the .tgz to attw,
which bypasses the broken parser. Tarball is cleaned up and *.tgz is
already gitignored.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@arethetypeswrong/cli 0.18.2 (core 0.18.2) decompresses the packed tarball with a streaming Gunzip callback that keeps only the *last* chunk. With fflate 0.8.3 (which CI resolves fresh since yarn.lock is gitignored) the tarball is emitted in multiple chunks, so the untar sees no data and attw crashes with "Cannot read properties of undefined (reading 'filename')". core 0.18.3 fixed this to collect all chunks. Reproduced on node 22.23.1 with a node_modules install; verified 0.18.4 passes under the same conditions. Reverts the earlier explicit-pack workaround, which did not help since the bug is in extraction, not pack. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three correctness fixes flagged during the pre-v0.6.0 review:
- recursiveResolveOptions rejected unknown options by iterating over the
defaults, so the guard could never fire and typos were silently
ignored. Iterate the caller's keys instead (and only recurse when both
sides are plain objects). Unknown options now throw "Invalid option".
- Server parsing used server.split(":", 2), which mangles IPv6 literals
(host became the first hextet, port NaN). Add parseServer() supporting
bracketed IPv6 with optional port, bare IPv6 literals (host-only), and
the existing host / host:port / IPv4[:port] forms, with port-range
validation.
- Clock offset used ((|T2-T1| + |T3-T4|) / 2) * sign(T3>T4), which gives
the wrong result when the inbound and outbound legs disagree in sign.
Use the signed RFC 5905 formula ((T2-T1) + (T3-T4)) / 2.
Adds 10 tests (option validation, server parsing) plus an offset-sign
regression that fails against the old heuristic. BREAKING: unknown
options now throw, and computed offsets change for asymmetric paths.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pre-release hardening and test scaffolding for v0.6.0.
Core library fixes
new NtpTimeSync(...)constructionscreatePacketwithBuffer.writeBigUInt64BE(6b2b18f) — removes deprecatedsubstrusage, ends per-packet string allocations, bit-accurate timestampsacceptResponse(4602083) — replaces!assertions ontransmitTimestamp/receiveTimestamp/precisionsend(f8377c3) — closes a race where fast replies were missedcreatePacket, usesremoveAllListenersbeforecloseReview-driven hardening (024b211)
recursiveResolveOptionsiterated over the defaults, so the "Invalid option" guard could never fire and typos were silently dropped. Now iterates the caller's keys and throws; recursion only descends when both sides are plain objects.server.split(":", 2)mangled IPv6 literals (host became the first hextet, portNaN). NewparseServer()handleshost,host:port,IPv4[:port], bracketed IPv6[::1][:port], and bare IPv6 literals (host-only), with port-range validation.((|T2-T1| + |T3-T4|)/2) * sign(T3>T4)with the RFC 5905 formula((T2-T1) + (T3-T4))/2, which was returning the wrong sign when the inbound and outbound legs disagreed.Public API
src/index.tsfromexport *to explicit named exports; deep-freezeNtpTimeSyncDefaultOptions(5c1032e). Surface area unchanged (5 names), runtime mutation now blocked.Dependencies
ntp-packet-parserto^0.6.1(runtime) andprettierto3.9.3(402a794); supersedes Dependabot build(deps-dev): bump prettier from 3.8.3 to 3.9.3 #128.@arethetypeswrong/clito0.18.4(dd2cd47).Tests
CI
verify-packagejob runsnpm pack --dry-run,publint, and@arethetypeswrong/clion Node 22 (1d20851). Catchesexports/files/types regressions before publish.repository.urlwithgit+prefix for publint compliance (8f40049).node_modulescache onpackage.jsoninstead of the gitignoredyarn.lock(00c02b0).verify-packagecrash (dd2cd47): attw-core 0.18.2 decompressed the packed tarball with a streamingGunzipcallback that kept only the last chunk. CI resolves dependencies fresh (yarn.lock is gitignored), so it pulledfflate@0.8.3, which emits the tarball in multiple chunks → empty untar →Cannot read properties of undefined (reading 'filename'). core 0.18.3 collects all chunks. Reproduced on Node 22.23.1 with a node_modules install and verified 0.18.4 passes under the same conditions.Invalid option: <key>instead of being ignored.src/index.tsno longer re-exports viaexport *(explicit named exports only).Test plan
yarn buildexits 0yarn test:unit— 32/32 passingyarn verify:package— pack + publint + attw all greenyarn test:integration:cjsandyarn test:integration:esm— passingpackage.jsonto0.6.0before tagging