feat(core): phase 3 — byte-stream I/O contracts and request/response body lifecycle - #34
Open
Wahbeh-Mohammad wants to merge 8 commits into
Open
feat(core): phase 3 — byte-stream I/O contracts and request/response body lifecycle#34Wahbeh-Mohammad wants to merge 8 commits into
Wahbeh-Mohammad wants to merge 8 commits into
Conversation
… TeeSink. (#31) * feat(core): add I/O contracts — ByteQueue, BufferedSource/Sink, views, TeeSink. * test(core): close per-file coverage gaps in invariant, io/errors, rejection helper. * fix(core): resolve I/O contract review findings — copy semantics, lifecycle, and encoding symmetry (IO-1..IO-42).
* feat(core): add request/response body lifecycle: Body model, materialize(), TypedResponse, HttpStatusError, logging tees (BODY-1..37, HTTP-36..52). * fix(core): resolve body-lifecycle review findings (BODY-3..37, HTTP-26/39/42/43/44/51). * test(core): cover withBodyWriter teardown paths (RECOV-12, RETRY-2).
Nine defects found reviewing the shipped io/ and body/ layers against the phase 3a/3b plans. Seven are wire-correctness or public-API. Sink ownership, three sites and one root cause. A WritableStream adapter declaring write and close but no abort silently swallows the delegate's abort — the default abort algorithm is a no-op — so withRequestLogging left the caller's sink open, locked, and never told the message was broken. It now forwards both teardown paths, and closes on behalf of a delegate that resolves without doing so. StreamBody.writeTo passes preventCancel: true: pipeTo's default cancels the caller's source when the sink fails, taking ownership BODY-8 leaves with the caller, and disagreeing with the declared-length path, which only releases its reader. Close guarantees. Response.bytes/text and toHttpError acquire the body reader inside the try. getReader() itself throws when an external consumer holds the lock — which BODY-15 forbids assuming away — so the one failure BODY-16's guarantee most needs to cover was the one that skipped the close and held the connection. Declared length vs bytes written. MultipartBody.writeTo verifies its total against contentLength, refusing an overrunning chunk before it is written. The shared framing routine keeps the framing honest but takes each part's own length on trust, and MultipartPart.body is the public Body interface, so a caller implementation could report one length and write another (HTTP-51). Every Body variant is frozen at construction, so contentLength cannot be reassigned after the fact (HTTP-1, XCUT-15). Public API. Response regained the private constructor and createResponse friend hook a rewrite had dropped, which had published a field-wise constructor (HTTP-2). [Symbol.asyncDispose] is removed from Response and LoggedResponseBody: it postdates engines.node ">=18.17", where the computed key binds the method to the string "undefined", and its type reached this package only through a dev-only global — so the published .d.ts did not compile for a consumer on the lib this package itself declares. The API report is back to zero (undocumented) members, from 62. New blocking gate verify:consumer-types compiles a throwaway consumer against the built .d.ts on the declared lib with types: [], which is the gate whose absence let the asyncDispose defect clear every other one. Verified to fail on the reintroduced defect and pass once reverted. Also: assertCount single-sourced in io/limits.ts and applied to TeeSink, the fourth size-taking surface, which had none (IO-3); BODY-25's zero-chunk rule applied on the exceeds-cap tail path, not only the drain; an over-reporting primitive source raises SourceContractViolationError instead of surfacing as an exhausted stream (IO-17); http/charset.ts's decodeText renamed decodeBodyText so it stops colliding with io/text-codec's deliberately different one.
The phase 3b plan lists the 2026-07-25 checkpoint as a signed-off prerequisite. It has no commit and every box is unchecked — but §5.1 landed in bunfig.toml and half of §5.3 landed in errors.ts, which is exactly what made the claim look true to a spot check. Records the measured status of all twelve §5 items rather than the flat "it did not run". Grows the phase 3b execution findings from two rows to seven. E1 and E2 gain verified version numbers and measured blast radius; E3-E7 are new: §5.3 applied to 2 of 10 error leaves and stopped, §5.7 no isolated linker configured, §5.9 no test:node script exists although the 3b plan's own gate sequence calls it, §5.10 none of the eleven model files carries the #private justification, §5.8 stale NFR-14 reason. Resolves phase 4b's F1 to branch (b). Two of its premises were false: the floor was never raised, and SuppressedError arrived in Node 24.0.0 with the full Explicit Resource Management proposal rather than in the 18.18.0/20.4.0 symbols backport — so branch (a) means dropping Node 18, 20 and 22 outright. esnext.disposable supplies Symbol.asyncDispose's type but not SuppressedError's runtime, so E1's floor bump does not fix F1 and must not be read as doing so, including by 5a, 6b and 6c. Adds the phase 3b checklist, which was missing entirely, and records this phase's own residuals separately from the checkpoint's — among them the multipart boundary non-appearance limitation, which is documented rather than partially checked because a StreamBody part's bytes do not exist until the write.
`@changesets/write` names every changeset with a random `human-id`
(`dry-candles-unite.md`), and there is no config knob for it — the ID comes
from a hardcoded `humanId()` call, and `.changeset/config.json`'s schema has
no filename field. The names were already being hand-corrected after the fact.
Add `scripts/changeset.mjs`, wired as `bun run changeset`: it forwards every
argument to the CLI, then renames whatever changeset the run produced to
`YYYY-MM-DD-<slug>.md`, matching `docs/superpowers/{specs,plans}`. The slug is
prompted for and defaults to the changeset's own first sentence; a non-TTY
caller takes that default rather than hanging on a prompt nobody can answer.
Subcommands that create nothing (`version`, `status`, `publish`, `tag`, `pre`,
`init`) pass straight through.
Renaming after the fact is safe because nothing reads the filename back: the
CLI globs `.changeset/*.md` and takes every decision from the frontmatter. The
seven existing changesets are backfilled with the date of the commit that
added each one.
No CI gate — a changeset written by hand or by another tool is not checked.
`bun test` runs the whole unit suite on Bun's runtime and proves nothing about the runtime this SDK ships to. Audited before writing anything: 319 of the 516 unit tests, across 21 of 43 files, exercise a runtime-divergent surface — Web Streams, AbortSignal, async iteration, or ByteQueue's Uint8Array handling — against exactly two assertions of Node coverage in scripts/verify-node-floor.mjs, neither of which touched io/. The ci job pinned no Node at all, so its three node-executed gates ran on an undeclared runner default, and node-floor-conformance pinned 18.17.0 alone, leaving current LTS unexercised against sdk-design-nodejs/09:52-54's "in addition to current LTS". Implements §5.9's own prescription rather than a substitute. bun test stays the unit runner untouched — docs/knowledge/testing.md mandates bun:test symbol imports, setSystemTime and --concurrent, so migrating to node:test would be a styleguide deviation plus a whole-suite rewrite — and is now scoped to packages/ via bunfig's [test] root so the two layers cannot blur. Without that scoping bun test collected the new .mjs files too, which would have run the Node-only layer on Bun and erased the distinction it exists to draw. Adds test/node-conformance/: 30 `node --test` cases over the BUILT artifact, never src/. Public surface arrives through the @dexpace/core specifier, the path a real consumer takes; io/ is @internal with no public subpath in exports, so it is reached by direct dist/ file path. Seeded with composeSignal, Phase 3a's byte-stream surface (chunk-straddling CRLF, slice views not advancing the parent, reader-lock release on close, tee mirror-and-forward, writeAll), and Phase 3b's body surface (reader-lock discipline on bytes/text/close, pipeTo preventCancel ownership, multipart framing through Web Crypto, toHttpError buffering). scripts/verify-node-floor.mjs is retired and its two AbortSignal.any assertions folded in as the suite's first cases, per §5.9:375's "rather than keeping two parallel Node entry points". The CI job is renamed node-conformance and is now a fail-fast:false matrix over ['18.17.0', 'lts/*']; lts/* resolves at run time so the LTS half cannot go stale as LTS moves. The 3b plan's Task 13 Step 3 called `bun run test:node` when no such script existed, so that gate sequence could not be executed as written; it is corrected, along with the two blocking gates it had never listed. NOTE: the CI job name changed. Branch protection requiring `node-floor-conformance` must be updated to `node-conformance`.
`changeset --empty`, deliberately, rather than no changeset at all. Commit e3d0b18 touched zero files under packages/ — everything in it is repository infrastructure that ships to nobody — so there is nothing for @dexpace/core to bump, and a patch would put a changelog line in front of consumers that means nothing to them. The empty changeset is what distinguishes "this change needs no release" from "somebody forgot a changeset". `changeset status` is unchanged by it: still one minor for @dexpace/core, from the five existing non-empty changesets. Created through scripts/changeset.mjs so the filename follows the repo's YYYY-MM-DD-<slug> convention; --empty produces no summary to derive a slug from, so the wrapper fell back to a generic name and it was renamed using the wrapper's own toSlug logic once the summary was written.
Open
The PR's node-conformance job failed on the pinned floor and passed on `lts/*`. Two unrelated defects, both invisible to `bun test` by construction. `MultipartBody` generates its boundary from `crypto.getRandomValues` — a bare global — while `engines.node` declared `">=18.17"`. Node exposes `globalThis.crypto` unflagged only from 19.0.0, and never to an ES module on any 18.x release: verified on 18.17.0 and 18.20.8, where `typeof globalThis.crypto` is `undefined` in `.mjs` and an object in CJS. So every `multipartBody(...)` call threw `ReferenceError: crypto is not defined` on the version the package promised, and a CommonJS probe would have reported that floor as satisfied. Bun supplies the global, which is why 516 unit tests never saw it and E5's suite caught it the first time it ran the built artifact on the pinned floor. The floor moves to `>=20.3`, chosen over the two options that keep Node 18. A `node:crypto` fallback puts a Node-only specifier in a package documented as running on browsers, Deno, Bun and Workers, and cannot be reached synchronously from the constructor that needs it. A non-crypto RNG silently downgrades the unguessable-boundary mitigation HTTP-51 leans on against multipart injection, on exactly the runtime CI pins. Node 18 went EOL in April 2025, so no supported runtime is dropped. 20.3 and not 20.0: `AbortSignal.any()` — `composeSignal`'s own floor-defining call, backported to 18.17.0 — reached the 20.x line only in 20.3.0, confirmed by running the suite against a pinned 20.0.0. `lib`/`target` move to ES2023 so `verify:runtime-floor` stays consistent; its `es2023` row is amended to `>=20.3` with the built-ins, not the syntax, named as the reason the floor sits above the language level's own minimum. The CI matrix pin moves 18.17.0 -> 20.3.0, and `seams.test.mjs` gains a case asserting `globalThis.crypto.getRandomValues` is a function *in ESM* — verified to fail on 18.17.0 and pass on 20.3.0 — so this cannot regress silently. Second defect: `seams.test.mjs` awaited an `AbortSignal.timeout()` abort with nothing else scheduled. That timer is unref'd on every Node version by design, so the loop drained before it fired and 18.17.0's runner cancelled the rest of the file (`Promise resolution is still pending but the event loop has already resolved`). Newer runners hold the loop open through handles of their own, which is the whole reason it passed on LTS. It now holds a ref'd deadline that both keeps the loop alive and fails the case if the abort never arrives. `sdk-design-nodejs/02`'s runtime line claimed Node >=18.17 supplies `globalThis.crypto.subtle`; corrected. Recorded as roadmap finding E8, which also renumbers E1: Symbol.dispose/asyncDispose reached the 20.x line in 20.4.0, so §5.4's bump now reads `>=20.3` -> `>=20.4`. The symbol is still declared nowhere. Gates: typecheck, lint, build, bun test (516), api, lint:publish, verify:dual-consumption, verify:consumer-types, verify:seam-1, verify:runtime-floor, audit, and test:node on both 20.3.0 and current Node.
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.
feat(core): phase 3 — byte-stream I/O contracts and request/response body lifecycle
Description
What this does
Adds Phase 3 of the Node SDK: the byte-stream I/O layer (3a) and the request/response
body lifecycle built on it (3b). Also adds a Node-runtime conformance suite, because
bun testruns on Bun and proves nothing about the runtime the SDK ships to.Changes
I/O contracts (3a, IO-1–IO-42) —
ByteQueue,BufferedSource,BufferedSink,slice views,
TeeSink,RetentionWindow, text codecs. Internal; no public API.Body lifecycle (3b, BODY-1–37, HTTP-36–52) — the
Bodyinterface and its fiveimplementations (
ByteArrayBody,StringBody,FormUrlEncodedBody,StreamBody,MultipartBody), plusmaterialize,TypedResponse,HttpStatusError,toHttpError,and the request/response logging tees.
Node conformance suite — 30
node --testcases against the built artifact, runby CI as a matrix over the declared
engines.nodefloor and current LTS. Replacesscripts/verify-node-floor.mjs.Tooling — new blocking gate
verify:consumer-typescompiles a throwaway consumeragainst the built
.d.tson the declaredlibwithtypes: [].scripts/changeset.mjsrenames changesets to
YYYY-MM-DD-<slug>.md.Review fixes included
Two review passes found 14 defects, most on the wire-correctness or public-API surface:
abortto the caller's sink. A truncated bodycould be committed downstream as a complete one (BODY-17, RECOV-12).
StreamBody.writeTocancelled the caller's stream on sink failure. Cancellationownership stays with the caller (BODY-8).
Response.bytes(),text()andtoHttpError()acquired the reader outside the try,so a locked body skipped the close and held the connection (BODY-15/16).
allowed header injection and a forged closing boundary (HTTP-26, HTTP-51).
MultipartBody.writeToandStreamBody.writeTonow verify written bytes against thedeclared
contentLengthbefore the write, and abort the sink on a mismatch(HTTP-39, HTTP-51).
Responseregained its private constructor andcreateResponsehook. A rewrite hadpublished a field-wise constructor that let a caller bypass
build()(HTTP-2).[Symbol.asyncDispose]is removed fromResponseand the logging wrapper. It postdatesthe
>=18.17floor and broke the published.d.tsfor consumers.close()is the onlyteardown interface.
Breaking
RequestBuilder.bodyandResponseBuilder.bodynarrow fromunknowntoBody | undefinedand
ReadableStream<Uint8Array> | null. Released as minor — the package is pre-1.0.