diff --git a/.changeset/2026-08-29-guard-symbol-asyncdispose-installs.md b/.changeset/2026-08-29-guard-symbol-asyncdispose-installs.md new file mode 100644 index 0000000..579110a --- /dev/null +++ b/.changeset/2026-08-29-guard-symbol-asyncdispose-installs.md @@ -0,0 +1,25 @@ +--- +"@dexpace/core": minor +"@dexpace/transport-fetch": minor +"@dexpace/transport-undici": minor +--- + +Guard every `[Symbol.asyncDispose]` install behind a runtime check, so disposal is never promised on a Node version that does not have the symbol. + +`Page`, `FetchTransport`, and `UndiciTransport` each declared `[Symbol.asyncDispose]` as a plain computed class member. `Symbol.asyncDispose` arrived in Node **20.4**, but every package here declares `engines.node: ">=20.3"`. On the declared floor the computed key evaluates to `undefined`, so the method was bound to the string key `"undefined"` — leaving a junk prototype entry and **no working disposal**, while the emitted `.d.ts` promised `AsyncDisposable` unconditionally. `SseStream` was already guarded, and `Response` carries a regression test asserting the absence of exactly this junk key (`http/response.test.ts`); these three sites had reintroduced it. + +The installs now match `SseStream`: `Object.defineProperty` behind `typeof Symbol.asyncDispose === 'symbol'`. Disposal works unchanged on Node 20.4+. + +Breaking, in the type system only: + +- `Page` no longer declares `implements AsyncDisposable`, and its `.d.ts` no longer declares `[Symbol.asyncDispose]`. +- `fetchTransport()` returns `Transport` rather than `Transport & AsyncDisposable`. +- `undiciTransport()` returns `Transport` rather than `Transport & AsyncDisposable`. + +`await using page = ...` / `await using transport = ...` therefore no longer type-checks. This is deliberate: the declaration was only ever true on Node 20.4+, and on the floor it type-checked a call that silently did nothing — for `undiciTransport` that meant leaking every pooled connection. Call `close()` instead, which has always been the real teardown path and is unchanged. Consumers pinned to Node 20.4+ who want `await using` back can reach the installed symbol through a cast. + +The floor will not be raised to `>=20.4` to restore the declaration. `NFR-10` requires a capability that needs a newer runtime to be isolated into its own unit declaring that higher floor, never to raise the floor of the general-purpose core; it also requires the emitted-artifact target and the visible-API level to agree, which is the clause the unguarded member violated. `>=20.3` is in any case derived rather than chosen — it is the lowest Node that runs what these packages emit, set by `globalThis.crypto` (absent from ESM on every Node 18 release) and `AbortSignal.any()` (20.3.0). The guarded install is the permanent shape. + +`Paginator.pages()`'s published TSDoc is corrected to match: it had discharged `PAGE-12`'s "consumers MUST be told to wrap the view in a scoped/auto-close construct" clause by naming `await using` alongside `for await`, which no longer type-checks. It now names the two constructs that do give the guarantee — a `for await` loop, or `.return()` from a `finally` when you drive the iterator by hand — and says why `await using` is not a third. + +Kept as **minor** rather than major because these packages are pre-1.0 (`0.0.0`), per the same semver initial-development carve-out the earlier `Body` narrowing used. diff --git a/.changeset/2026-08-30-undici-teardown-and-producer-ordering.md b/.changeset/2026-08-30-undici-teardown-and-producer-ordering.md new file mode 100644 index 0000000..b8e9fee --- /dev/null +++ b/.changeset/2026-08-30-undici-teardown-and-producer-ordering.md @@ -0,0 +1,20 @@ +--- +"@dexpace/transport-undici": patch +--- + +Fix two teardown defects in `undiciTransport()`. + +`close()` no longer strands owned dispatchers when one fails to release. It previously walked the +owned set with a bare `for … await` loop, so the first rejecting `destroy()` aborted the walk — and +because the set is walked in reverse, a configured proxy meant the `ProxyAgent` actually holding the +pooled connections was the one left leaked. Every owned dispatcher is now destroyed before any +failure is reported, and the failure surfaces as a `TransportFailureError` carrying the underlying +cause (an `AggregateError` when more than one dispatcher failed) rather than a raw `undici` error +escaping a public method untyped. + +`send()` now maps request headers before preparing the request body. `prepareBody()` starts a +streaming producer eagerly while header mapping reads `request.body.mediaType` — a getter on a +caller-supplied `Body` that may throw. In the old order such a throw left a live producer that +nothing could abandon, whose own later rejection reached Node's default `unhandledRejection` policy +(TRANSPORT-19, SEAM-30). `@dexpace/transport-fetch` already evaluated the two in this order and is +unaffected; both transports now carry a regression test pinning it. diff --git a/.claude/skills/ci-preflight/SKILL.md b/.claude/skills/ci-preflight/SKILL.md index 683c1ca..fe213cb 100644 --- a/.claude/skills/ci-preflight/SKILL.md +++ b/.claude/skills/ci-preflight/SKILL.md @@ -7,7 +7,7 @@ description: Use before pushing a branch, opening or updating a PR, or whenever ## Overview -`.github/workflows/ci.yml` is 14 blocking steps across two jobs. Every one of them can run +`.github/workflows/ci.yml` is 15 blocking steps across two jobs. Every one of them can run locally, so a red CI run is always avoidable — `bun test` passing is not evidence, and it is the single most common reason work gets handed over broken. @@ -19,10 +19,10 @@ node .claude/skills/ci-preflight/run-ci.mjs ~2.5 minutes warm on a green tree. Full output per step goes to `node_modules/.cache/ci-preflight/.log`; only a summary and a tail of each failure -reach stdout, so a red run costs a few hundred tokens rather than the ~40k that thirteen +reach stdout, so a red run costs a few hundred tokens rather than the ~40k that fourteen raw `bun run` calls would. -Do not hand-run the thirteen commands instead. Two things go wrong when you do: +Do not hand-run the fourteen commands instead. Two things go wrong when you do: - **Order is load-bearing.** `test`, `api`, `lint:publish` and every `verify:*` gate resolve `@dexpace/core` by package name, which lands in `packages/core/dist/`. Run any of them @@ -35,7 +35,7 @@ Do not hand-run the thirteen commands instead. Two things go wrong when you do: 1. **Run it.** Add `--skip-install` only if you have not touched `package.json` since the last install. **Before you push, add `--clean`** — a warm tree is blind to a whole class of defect CI hits on its first step. (The pinned Bun needs no flag; it is the default.) -2. **All green** → say so plainly: CI is all good, naming the count (`all 14 steps passed`). +2. **All green** → say so plainly: CI is all good, naming the count (`all 15 steps passed`). Nothing else to do. 3. **Anything red** → report the findings to the user *first*: which gates failed, what each one means, and the fix you intend. One line per finding, not a transcript dump. @@ -88,7 +88,7 @@ Both of these will make you report a passing gate that CI rejects. This is not hypothetical. PR #52 failed exactly this way: Phase 8a made `@dexpace/transport-shared` the second published package imported by name from another package's `src/`, `typecheck` and `lint` still pre-built only core, and a warm preflight - passed all 14 steps on the commit CI rejected. Fixed by `build:deps` — see CLAUDE.md, and + passed every step on the commit CI rejected. Fixed by `build:deps` — see CLAUDE.md, and keep that list current when a new package crosses the same line. - **The coverage floor fails silently.** `bun test` enforces `bunfig.toml`'s `coverageThreshold` (0.8) by **exit code alone**. It prints no threshold message, and the @@ -105,7 +105,7 @@ Both of these will make you report a passing gate that CI rejects. | `install` | `bun.lock` disagrees with a `package.json`. The tree CI installs is not yours, so nothing after it is measuring the right thing — the runner stops here. | `bun install`, then commit `bun.lock`. | | `typecheck` | `tsc --noEmit` over all 9 projects. | `Cannot find module '@dexpace/…'` means a build prerequisite is missing from `build:deps`, not a bad import — check with `--clean`. Otherwise a real fix; usual suspects: a missing `.js` extension on a relative import (NodeNext), a type import without `import type` (`verbatimModuleSyntax`), an enum/namespace/parameter property (`erasableSyntaxOnly`). | | `lint` | Formatting **and** type-aware rules; formatting is an error, not a warning. | `bun run fix` first — it clears every prettier finding. Hand-fix what survives: 70-line function cap, `max-depth` 3, `max-params` 3, explicit return types on exported members. Every `eslint-disable` needs a `-- reason`. | -| `build` | Emit failed. **Blocks the ten gates below it**, which the runner reports `SKIP`. | Fix this before reading anything else; the skipped gates are unknown, not passing. | +| `build` | Emit failed. **Blocks the eleven gates below it**, which the runner reports `SKIP`. | Fix this before reading anything else; the skipped gates are unknown, not passing. | | `test` | A failing test, *or* the silent coverage floor (see above). | If the tail says `0 fail`, it is coverage — find the file that dropped below 0.8 in the printed table and test it. Otherwise fix the test or the code. | | `api` | The committed `etc/.api.md` no longer matches the built surface, or an export lacks TSDoc. | Intended export change: `cd packages/ && bun run api:local`, then commit the regenerated report. `(undocumented)` in the diff means the export needs a `@public` block, plus `@throws` naming each catchable error class. **Unintended** change: revert the export, don't bless the report. | | `lint:publish` | `publint` + `attw` on every built package's `exports` map, `types`/`main` fields, and declaration resolution. | Fix the manifest. `cjs-resolves-to-esm` is already ignored by design (ESM-only); every other rule is real. | @@ -114,9 +114,18 @@ Both of these will make you report a passing gate that CI rejects. | `verify:seam-1` | A package gained a runtime dependency outside the allow-list, or dropped its committed empty `dependencies` object (an omitted field is a violation too). | Remove the dependency — SEAM-1 is the constraint, not the gate. `@dexpace/core` is a **peer** of the satellites, never a dependency. | | `verify:sse-37` | Core's SSE code reached for serde or a codec package. | Remove the import; SSE-37/38 forbid the coupling. | | `verify:runtime-floor` | `engines.node` and the `target`/`lib` a package compiles to have drifted apart. | Move both together, deliberately — never raise one to silence this. | +| `verify:reproducible-build` | Two clean builds of an identical source tree disagreed (NFR-12) — either in an emitted `dist/` file or in an `npm pack` tarball. | The assertion names what differed. A wall-clock or random value reaching a build-time codegen step is the usual cause; `packages/core/scripts/gen-version.mjs` is the only such step today, and injecting a `Date.now()` there is this gate's own negative test (it fails naming `packages/core/dist/generated/version.js` and `npm-pack:dexpace-core-0.0.0.tgz`). If the log instead ends in `tsc` errors, the **build inside the gate** failed and there is no difference to read — fix that first. The gate sweeps every `dist/` and rebuilds twice itself, so it is last in the job and leaves the tree freshly built. | | `audit` | A high-severity advisory in production dependencies. | `bun audit --prod` for detail. Note the tree is tiny (zero runtime deps by design), so a hit here is usually a transitive dev-dep misclassification worth reading carefully. | | `test:node` | Bun-vs-Node runtime divergence, almost always in `packages/core/src/io/` — Web Streams, `AbortSignal`, `Uint8Array` chunking. | Fix against Node's semantics. A phase touching a runtime-divergent surface should be *adding* cases here; see `test/node-conformance/README.md`. | +**A `timeout` verdict is not the same finding as a red one.** Every step is capped at +`STEP_TIMEOUT_MS` (10 minutes) in `run-ci.mjs`, and a step that hits the cap is reported `timeout` +whether it hung or was merely slow. Nearly all of them finish in seconds; +`verify:reproducible-build` is the exception, performing **two full swept builds plus two `npm pack` +passes** — ~34s observed warm, but a cold or loaded machine multiplies that, and it is the one step +with any real chance of approaching the cap. If *it* comes back `timeout`, raise `STEP_TIMEOUT_MS` +and re-run before reading the verdict as a reproducibility defect. + ## Local-vs-CI divergences worth stating The runner reproduces CI's steps, not CI's machine. Two gaps survive, and both belong in @@ -153,7 +162,7 @@ consumer-facing change still needs `bun run changeset`). | `--skip-install` | Skip the frozen-lockfile install. Safe when `package.json` is untouched. | | `--node-floor` | Also run `test:node` under Node 20.3.0 via mise/fnm/nvm. | | `--tail N` | Lines of a failing log to print (default 30). Raise for a wall of tsc errors. | -| — | Each step is capped at 10 minutes and reported `timeout` if it hangs. A gate *can* hang rather than fail — a conformance test holding the event loop open on an unclosed server does exactly that. | +| — | Each step is capped at 10 minutes (`STEP_TIMEOUT_MS`) and reported `timeout` if it hangs. A gate *can* hang rather than fail — a conformance test holding the event loop open on an unclosed server does exactly that. It can also just be slow: `verify:reproducible-build` builds the workspace twice and packs it twice, so raise the cap rather than diagnosing a `timeout` there as a real failure. | | `--list` | Step ids and the command each runs. | Exit code is 0 only when every selected step ran and passed. `SKIP` is never a pass. diff --git a/.claude/skills/ci-preflight/run-ci.mjs b/.claude/skills/ci-preflight/run-ci.mjs index b6aa2a8..287f63c 100644 --- a/.claude/skills/ci-preflight/run-ci.mjs +++ b/.claude/skills/ci-preflight/run-ci.mjs @@ -4,16 +4,16 @@ // Runs every blocking step of `.github/workflows/ci.yml` against the working tree, in CI's own // order, and reports all failures at once rather than stopping at the first. // -// Two things make this more than a shell alias for thirteen `bun run` calls: +// Two things make this more than a shell alias for fourteen `bun run` calls: // // * Ordering is load-bearing. `bun test`, `api`, `lint:publish` and every `verify:*` gate resolve // `@dexpace/core` by package name, which lands in `packages/core/dist/`. Run them before // `build` and they either fail with unresolved-module noise or, worse, pass green against // yesterday's artifact. CI is safe because its Build step precedes its Test step; a human // running gates ad hoc is not. -// * A failed `build` invalidates the ten gates downstream of it. Running them anyway produces ten -// spurious findings that all say "cannot resolve @dexpace/core". They are reported SKIP here, -// so the summary names the one real defect. +// * A failed `build` invalidates the eleven gates downstream of it. Running them anyway produces +// eleven spurious findings that all say "cannot resolve @dexpace/core". They are reported SKIP +// here, so the summary names the one real defect. // // Logs go to node_modules/.cache/ci-preflight/.log — full output stays on disk, only the // summary and a tail of each failure reach stdout. @@ -105,6 +105,14 @@ const STEPS = [ cmd: 'bun run verify:runtime-floor', tier: 'gate', }, + { + // Last among the gates, matching ci.yml: it sweeps every dist/ and rebuilds twice, so running it + // earlier would pull the tree out from under any step that resolves a workspace package by name. + id: 'verify:reproducible-build', + ci: 'Reproducible-build check (NFR-12)', + cmd: 'bun run verify:reproducible-build', + tier: 'gate', + }, {id: 'audit', ci: 'Dependency audit', cmd: 'bun run audit', tier: 'gate'}, { id: 'test:node', @@ -117,7 +125,11 @@ const STEPS = [ // engines.node across every publishable package, and the floor leg of ci.yml's node-conformance // matrix. The other leg is `lts/*`, which resolves at run time and so cannot be pinned here. const NODE_FLOOR = '20.3.0'; -// Comfortably past the slowest gate (`api`, ~50s) without letting a hung one stall the run. +// Comfortably past the slowest gates (`api` ~50s, `verify:reproducible-build` ~34s warm) without +// letting a hung one stall the run. `verify:reproducible-build` is the one worth watching as this +// grows: it does two full swept builds and two `npm pack` passes, so a cold or loaded machine +// multiplies its wall time. If it ever reports `timeout`, raise this rather than reading the verdict +// as a reproducibility defect. const STEP_TIMEOUT_MS = 10 * 60 * 1000; // setup-bun resolves this file, so it is the Bun every CI step actually runs on. const PINNED_BUN = readFileSync('.bun-version', 'utf8').trim(); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 426f48e..a3a4025 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,6 +62,12 @@ jobs: - name: Runtime-floor consistency check run: bun run verify:runtime-floor + # NFR-12. Deliberately last in this job: it sweeps every dist/ and rebuilds + # the workspace twice, so it would otherwise pull the rug from under any + # step above that resolves a workspace package through its dist/. + - name: Reproducible-build check (NFR-12) + run: bun run verify:reproducible-build + - name: Dependency audit run: bun run audit diff --git a/docs/deviations.md b/docs/deviations.md new file mode 100644 index 0000000..ea46c0e --- /dev/null +++ b/docs/deviations.md @@ -0,0 +1,377 @@ +# Deviations That Cannot Be Corrected + +Audit of `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md` (the Phase 10 +reconciled ledger, 17 items) performed against the **as-built code**, not against the phase specs that +produced it. Every item was re-derived from source; each entry below records the file and line that proves +the claim. + +**Scope of this file:** the deviations that are *permanently uncorrectable* — where restoring the reference +contract's own mechanism is impossible on this platform, forbidden by a project constraint, or would be a +regression. Items found to be **correctable** are deliberately **not** listed here — they were fixed instead. + +**This file is the audit, not the ledger** (cross-reference added 2026-08-30; until then nothing in the repo +linked the two, and the numbering they share had no stated owner): + +- **`docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md` (§10) is the normative + ledger** — the canonical list of deliberate deviations, and the **owner of the item numbers**. Every `## N` + heading and every table row below is keyed to §10's numbering and has no independent identity. **If §10 + renumbers, this file must be renumbered in the same commit.** §10 now carries the matching pointer back here. +- **This file is the as-built audit of that ledger** — it re-derives each item from source, carries the + `file:line` evidence, and records which of §10's claims did not survive contact with the code. +- **A new deviation is recorded in neither.** It goes in the owning phase spec's own `## Deviation Ledger (for + Phase 10)` section; §10 is the consolidated **output** of those, not their intake. +- **Do not confuse either with `docs/knowledge/deliberate-deviations.md`.** Despite the near-identical name it + is a harvested corpus topic file queried by `bun run knowledge`, derived from an older revision of §10 and + currently stale. It is `knowledge-harvest`'s output and is never hand-edited as a ledger. + +Audited 2026-08-29 against `25-phase-10-deviation-reconciliation` @ `d8217af`; the audit's own changes landed +on that branch as `27fb81f`, which is the tree this file describes. + +**What the audit changed** (committed as `27fb81f`): + +| Was | Outcome | +|---|---| +| Item 11 — `Symbol.asyncDispose` declared as a plain class member on `Page`, `FetchTransport`, `UndiciTransport` | **Code fixed.** All three now install it guarded, matching `SseStream`. On the `>=20.3` floor the computed key evaluated to `undefined`, leaving a junk `"undefined"` prototype entry and no working disposal — verified on real Node 20.3.0, before and after. `implements AsyncDisposable` and the factories' `& AsyncDisposable` return types dropped as untrue on the floor. Changeset added; API reports regenerated | +| Item 11 — the `Page` node-conformance test asserted `typeof page[Symbol.asyncDispose] === 'function'` | **Test fixed.** On the floor that read `page['undefined']`, which *was* the junk method, so the assertion passed over a `Page` that could not be disposed. Now branches on the symbol and asserts the junk key's absence on both matrix legs | +| Item 14 — `NFR-12` recorded as unverifiable | **Closed on evidence.** 644 emitted files **and 9 `npm pack` tarballs** byte-identical across two clean builds. Added `bun run verify:reproducible-build` as a blocking CI step, negative-tested by injecting a `Date.now()` into `gen-version.mjs`. (Widened 2026-08-30: the tarball comparison was a by-hand check of `@dexpace/core` alone at audit time; it is now a second leg inside the gate, over every publishable package, on both builds) | +| Item 7 — "three tiers, not four" | **Ledger corrected.** The code implements all four; only the default production binding of the property layer is empty | +| Item 4 — "never bare structural interfaces" | **Ledger corrected.** 61 exported interfaces vs 58 classes; `Configuration` is builder-built and exported structurally | +| Item 14 — "`npm publish --provenance` is scripted" | **Ledger corrected.** It is not scripted anywhere; only `prepublishOnly` is | + +Everything below is what remains genuinely uncorrectable: **fifteen** of the ledger's seventeen items. Items +**7** and **11** are gone from the count because they were correctable and were corrected; item **14** stays, +because only its `NFR-12` half closed. (`27fb81f`'s commit message says "fourteen"; it counted the split item +14 as gone. The list below is the authoritative count — fifteen `##` sections, fifteen table rows.) + +| Ledger item | Verdict | +|---|---| +| 1 Single execution model | Uncorrectable — platform | +| 2 Byte-stream provider seam | Uncorrectable — platform + `SEAM-1` | +| 3 Retry stacks unified | Uncorrectable — spec-sanctioned by `RETRY-28` | +| 4 Structural-typing encapsulation gap | Uncorrectable — language (its mitigation clause was wrong; corrected) | +| 5 Schema-as-witness | Uncorrectable — language | +| 6 Vendored MD5 | Uncorrectable — platform + `SEAM-1` | +| 8 `AbortSignal` cancellation | Uncorrectable — platform | +| 9 Freeze-once collections | Uncorrectable — and strictly better | +| 10 `NFR-8` not applicable | Uncorrectable — no surface to configure | +| 12 Cross-origin marker header | Uncorrectable — the alternative is broken | +| 13 Transport-adapter platform gaps | Uncorrectable — platform (4 of 5 clauses) | +| 14 `NFR-16` publish provenance | Uncorrectable *here* — needs a real registry (`NFR-12` split out and closed) | +| 15 ETag obs-text does not round-trip | Uncorrectable — MUST outranks SHOULD | +| 16 No async-runtime adapter fragmentation | Uncorrectable — no second ecosystem exists | +| 17 Three-level error tree | Uncorrectable — the subtyping *is* the requirement | + +Items **7**, **11**, and the `NFR-12` half of **14** are absent from this list on purpose. They were correctable, and have been corrected — see the table above. + +--- + +## 1. Single execution model eliminates every thread/CAS/interrupt-flag primitive + +**Verified.** `Transport.send()` is `Promise`-only, one method satisfying `SEAM-11` and `SEAM-16` at once +(`packages/core/src/seams/transport.ts:49`). `ContextStore` holds a plain `Map` +(`packages/core/src/context/store.ts:24`) with a fresh `Symbol()` per call +(`packages/core/src/context/context.ts:104,125,149`). `Next` is `(request?) => Promise` with no sync +twin (`packages/core/src/pipeline/step.ts:23`). `wrapCancellation` degenerates to `failure(error)` and says so +(`packages/core/src/recovery/cancellation.ts:31`). `ASYNC-18` holds: SSE parses `retryMs` +(`packages/core/src/sse/parser.ts:131`) but never acts on it — reconnection is caller-owned, so no adapter +schedules a delay outside the retry engine. + +**Why it cannot be corrected.** There is no thread to interrupt, no CAS to perform, and no clearable +interrupt flag to restore. `AbortSignal.aborted` is latched by specification — re-asserting it is not merely +unnecessary, it is not expressible. Reintroducing the distinction would mean shipping a synchronous blocking +transport, which Node's I/O model cannot provide without a worker thread and a `SharedArrayBuffer` + +`Atomics.wait` handshake — a mechanism strictly worse than the one it replaced, and one that would break +`SEAM-1`'s zero-dependency floor for the browser/Workers half of the runtime target. + +--- + +## 2. The byte-stream provider seam and its discovery machinery are removed + +**Verified.** `packages/core/src/io/index.ts` exports concrete types only; the sole surviving mention of +"provider" in `io/` is a comment recording that `IO-30`'s *resolution* half was not built +(`packages/core/src/io/factories.ts:13`). There is no registry, no install precedence, no conflict +resolution — `IO-39` ships nothing. + +**Why it cannot be corrected.** `SEAM-3`–`SEAM-10` exist to keep a *third-party* stream library out of a +zero-dependency core. Web Streams are a runtime built-in. A discovery mechanism needs at least two +candidate implementations to discover between; there is exactly one, and adding a second would require a +runtime dependency that `verify:seam-1` fails the build over. The machinery would be ceremony with an empty +registry behind it. + +`SEAM-18`'s three bridge clauses (caller-supplied executor, async-wrapper unwrapping, interruptible blocking +wait) inherit item 1's impossibility — they presuppose the blocking transport that cannot exist. Its one +non-bridge clause survives and is enforced as an ordinary obligation on `send()` +(`packages/core/src/seams/transport.ts:49`). + +--- + +## 3. Two retry stacks collapse into one, with the total-timeout budget explicitly opt-in + +**Verified.** One engine — `runWithRetry` (`packages/core/src/retry/engine.ts:354`) — with exactly two thin +callers: the pillar step (`packages/core/src/retry/retry-step.ts:137`) and the dispatch adapter +(`packages/core/src/retry/retry-dispatch.ts:53`). `totalTimeoutMs` is `readonly totalTimeoutMs?: number | +undefined` and undefined by default (`packages/core/src/retry/settings.ts:27`), pinned by a test named for +`RETRY-28` (`packages/core/src/retry/settings.test.ts:20`). + +**Why it cannot be corrected.** This is not a deviation the port chose against the spec — `RETRY-28` is the +spec instructing a unifying port to make the budget opt-in. "Correcting" it means splitting one engine into +two that differ in no observable behavior, then re-deriving `RECOV-17`–`RECOV-34` against the duplicate. +The ledger entry documents conformance, not a gap. + +--- + +## 4. True runtime encapsulation of domain models is not fully achievable + +**Verified.** Domain models use `#private` fields and a TS `private` constructor reached through the +`createX` friend hook, so `build()` cannot be bypassed for a *class* — e.g. +`packages/core/src/http/request-conditions.ts:59-76`. + +**Why it cannot be corrected.** TypeScript's type system is structural and erased. Nothing at runtime +distinguishes a `Headers` instance from an object literal that satisfies the same shape, and no compiler flag +changes that. A nominal-typing emulation (a branded `#private` witness field) would only move the check to +call sites that must then be written to perform it, and would still be defeated by a cast. This is a +language-level ceiling. + +> **Corrected in the ledger 2026-08-29 — the text was wrong, the deviation is not.** Item 4's stated mitigation — "exporting only +> concrete classes, never bare structural interfaces, from each package's public entry point" — is false as +> written. `packages/core/etc/core.api.md` exports **61 interfaces** against 58 classes, and at least one is +> a builder-built, validated, frozen type: `Configuration` is `export interface Configuration` +> (`packages/core/src/config/configuration.ts:72`) returned from `ConfigurationBuilder.build()`, and +> `setGlobalConfiguration()` / `resolveProxyOptions()` accept any hand-rolled object of that shape. Most +> other exported interfaces are seams (`Transport`, `Serde`, `Logger`) or options records, where structural +> typing is the point. The *deviation* is real and uncorrectable; the *mitigation sentence* overstated what +> the package actually does and has been narrowed to the `http/` wire-model types. + +--- + +## 5. Schema-as-witness replaces reflective generic-type capture + +**Verified.** `Serde` is not generic in `T` (`packages/core/src/seams/serde.ts:182`); the witness is a +decode-time parameter, `deserialize(data, schema: Schema, typeName?)` +(`packages/core/src/seams/serde.ts:145`). The codec-configuration knobs are absent and documented as +absent — `packages/codec-json/src/json-serde.ts:236,242` explain that `SERDE-21`/`22` have no coercion +setting because there is no coercing codec, and `SERDE-23`'s unknown-field policy belongs to the schema. +`packages/codec-json/src/conformance.test.ts:8` states outright that no code implements `SERDE-21` or +`SERDE-22`. + +**Why it cannot be corrected.** `SERDE-5`–`SERDE-8` are worded around a reflectively reconstructed type +token. JVM generics erasure leaves a raw `Class` behind; TypeScript erases to nothing — there is no runtime +artifact of `T` at all to reflect over. Nor can the missing knobs be added: `JSON.parse`/`JSON.stringify` +expose no coercion, unknown-field, or date-format hooks to gate. A hand-rolled JSON parser could expose +them, at the cost of correctness, performance, and a large maintenance surface, to gate settings the schema +witness already decides more precisely. + +--- + +## 6. Digest MD5 needs a vendored implementation; SHA-256 does not + +**Verified.** `packages/core/src/auth/md5.ts` is a hand-rolled RFC 1321 implementation whose header states +the reason. SHA-256 goes through `globalThis.crypto.subtle.digest('SHA-256', bytes)` +(`packages/core/src/auth/digest.ts:103`). + +**Why it cannot be corrected.** Web Crypto excludes MD5 by design, on security grounds — it is not an +oversight to work around, and no flag re-enables it. The two alternatives are both closed: an npm MD5 +dependency fails `verify:seam-1`'s zero-runtime-dependency gate, and `node:crypto` would forfeit the +browser/Deno/Workers portability that motivated choosing Web Crypto in the first place. RFC 7616 still +requires MD5/MD5-sess for interop with servers that have not moved to SHA-256, so dropping it is not an +option either. + +--- + +## 8. Cancellation is `AbortController`/`AbortSignal` end-to-end + +**Verified.** `composeSignal` folds a caller signal and a timeout into one via `AbortSignal.any` +(`packages/core/src/seams/transport.ts:74`). The timeout-vs-cancellation split reads the structured +`reason.name` field, explicitly *not* `instanceof`, because `instanceof` is realm-bound +(`packages/core/src/seams/transport.ts:109`). + +**Why it cannot be corrected.** `Promise` has no `cancel()`, unlike `CompletableFuture`; cancellation in +JavaScript is cooperative by construction. The `reason.name` check is not a shortcut around a class +hierarchy that could be built — `AbortSignal.timeout()` and a caller `abort()` both deliver a `DOMException` +through the same signal type, and the runtime chooses the class. There is no seam at which the port could +substitute its own. + +> Minor wording nit: the ledger says "the abort reason's *constructor name*". The code reads the `name` +> **property** (`reason.name === 'TimeoutError'`), which is deliberate and stronger — a constructor-name +> check would break across realms exactly as `instanceof` does. + +--- + +## 9. Frozen collections are computed once, not wrapped on every read + +**Verified.** `Headers` freezes each value array, the lookup map, the casing map, and the insertion order +once at build time, then freezes the instance (`packages/core/src/http/headers.ts:314-319,345`). + +**Why it cannot be corrected.** The reference's per-access unmodifiable wrapper exists to guard a mutable +backing collection. These models are immutable after construction, so there is no window in which a +re-wrap could observe a different value. Restoring per-access wrapping would allocate on every getter to +defend against a mutation that cannot occur. The one genuine residue is already recorded elsewhere in the +project: `Request.url` clones per access because the native `URL` really is mutable. + +--- + +## 10. `NFR-8` (shrinker keep/retain configuration) is not applicable + +**Verified.** `packages/shrink-test/` exists and targets the dual-package `instanceof` hazard +(`packages/shrink-test/src/fixture-app.ts:111`, `run-shrink-guard.test.ts:8` — both line numbers refreshed +2026-08-30 after the fixture grew a third probe). Nothing in the workspace carries a keep-rule, and there is +no reflective lookup for one to protect. + +**Why it cannot be corrected.** A keep-rule names a symbol a static analyzer cannot see is reachable. +`NFR-8`'s premise is JVM reflection; JS bundlers have no equivalent blind spot, and item 2 already retired +the one discovery mechanism the port might have had. There is no symbol to keep-configure, so the +configuration file would be empty by construction. The structurally equivalent JS risk is covered instead. + +**What the guard now also pins, and why it is the standing evidence for a manifest decision (added +2026-08-30).** The `[Symbol.asyncDispose]` repair replaced three class members with a **module-scope +`Object.defineProperty` statement** — a top-level side effect — in four files across three packages that all +declare `"sideEffects": false` (`packages/core/src/sse/stream.ts:209`, +`packages/core/src/pagination/page.ts:114`, `packages/transport-fetch/src/fetch-transport.ts:314`, +`packages/transport-undici/src/undici-transport.ts:566`; the manifest field at `packages/core/package.json:20`, +`packages/transport-fetch/package.json:21`, `packages/transport-undici/package.json:21`). That field entitles a +bundler to drop a module whose exports go unused, and nothing forbids a future one from also dropping a +top-level statement it judges inert — which would silently un-install disposal in the shipped artifact while +every type still checks, the same failure shape this guard already exists for. `fixture-app.ts`'s +`probeDisposalSymbol` therefore constructs a `Page` and a `FetchTransport` **inside the bundled, minified, +tree-shaken artifact** and asserts the member is present and callable; `run-shrink-guard.ts` exits non-zero on +any `false` field of `FixtureResult`, so the check is blocking. + +**`sideEffects` was deliberately not narrowed** to the four file paths that carry an install. Narrowing is +more fragile, not less: the list would silently go stale on any file move or rename, and a stale narrow list +fails *open* — the bundler drops the module and nothing complains. The shrink guard tests the property that +actually matters (the install survives a real `bundle + minify + treeShaking` pass) rather than a manifest +proxy for it, and it needs no maintenance when a file moves. Budget note: the added probe took the measured +bundle from 16,671 to 17,689 bytes against a 24 KiB budget (`packages/shrink-test/shrink-test.config.ts`). + +--- + +## 12. The redirect/auth cross-origin marker is a real header, not a `WeakSet` + +**Verified.** `CROSS_ORIGIN_MARKER_HEADER` is set, cleared, and tested per hop +(`packages/core/src/redirect/cross-origin.ts:80,93,104,118`). The auth step reads it before deciding whether +to react to a challenge, not just whether to stamp (`packages/core/src/auth/auth-step.ts:387-390`). An +independent `POST_AUTH` backstop strips it even in a pipeline with no auth step +(`packages/core/src/redirect/strip-marker-step.ts`), satisfying `REDIR-11(c)`'s porter caveat. + +**Why it cannot be corrected.** The reference's in-process marker was tried and withdrawn during Phase 5b's +own drafting: retry's attempt-stamping sits between redirect and auth and produces a **fresh `Request` +copy**, which a `WeakSet` keyed on object identity no longer recognizes — the marker would silently +vanish exactly on the hop that most needs it. Restoring the identity-based marker means either removing +attempt-stamping (breaking `RETRY-38`) or making `Request` mutable (breaking `HTTP-2`/`HTTP-5`). + +The two interpretive questions Phase 10 settled here — `REDIR-20`'s predicate scope and Basic/Digest never +stamping preemptively — are confirmed against the code and stand as decided. Both are security-conservative +readings; reversing either would widen an attack surface for a caller convenience the spec never asked for. + +--- + +## 13. Transport adapters have platform-shaped gaps the reference does not + +**Verified.** `Protocol.HTTP_1_1` is hardcoded in both adapters +(`packages/transport-fetch/src/fetch-transport.ts:178`, `packages/transport-undici/src/undici-transport.ts:346`). +`transport-fetch` documents having no `proxy` option at all +(`packages/transport-fetch/src/fetch-transport.ts:62-65`). The proxy `challengeHandler` is surfaced with a +warning rather than dispatched (`packages/transport-undici/src/challenge-handler.ts:27,50`). + +**Why it cannot be corrected.** Four of the five clauses are closed by the platform, not by choice: + +- **Negotiated protocol version.** Neither `fetch`'s `Response` nor undici's `ResponseData` carries it. + There is no API to read, so the best-effort default is the only honest answer available. +- **Zero-copy `sendfile(2)` (`TRANSPORT-28`, SHOULD).** No user-space path in either client reaches the + syscall; a raw `node:net` transport would be a different product. +- **`TRANSPORT-8`'s native-cancel-vs-timeout distinction.** §17's own text scopes the clause to transports + that *have* an internal cancel path. `transport-fetch` does not, so the clause does not bind it. +- **Proxy `challengeHandler` on undici.** undici's `ProxyAgent` takes its credential solely from its own + constructor and rejects a per-request `Proxy-Authorization` with `InvalidArgumentError` — a deliberate + security fix upstream. The constructor runs before any challenge exists, so a handler-minted credential + can never reach the exchange that provoked it. This is unfixable without vendoring undici internals. Note + that the Phase 8a *plan* specified a retry-with-stamped-credential flow that is simply not implementable + on this platform; the shipped fallback (WARN at construction, WARN on first `407`, Basic via + `ProxyOptions.credentials`, `407` returned untouched) is the correct disposition. + +The fifth clause, `transport-fetch` shipping no proxy support (`TRANSPORT-30`), is a **deliberate scope +boundary rather than an impossibility** — it is achievable, at the cost of depending on `undici` internals, +which would defeat the package's zero-dependency purpose. `@dexpace/transport-undici` is the supported +answer for callers who need proxying. Recorded here for completeness, not as a platform limit. + +--- + +## 14. `NFR-16` — publish provenance + +**Verified.** `prepublishOnly` is wired in all nine publishable packages (e.g. +`packages/core/package.json:29`). There is **no** release workflow — `.github/workflows/` contains `ci.yml` +only — and the string `provenance` appears in no `package.json`, no workflow, and no `.npmrc` (there is no +`.npmrc`). + +**Why it cannot be corrected here.** `NFR-16`'s conformance test is behavioral: "a CI/release build fails an +unsigned publication; a local build without keys still publishes unsigned." Satisfying it requires a real +`npm publish --provenance` against a real registry with a real OIDC token. Nothing in this repository can +produce that evidence; it unblocks at first release and not before. + +> **Corrected in the ledger 2026-08-29.** Item 14 claimed `prepublishOnly` *and* `npm publish --provenance` +> "are scripted (Phase 0 Task 3)". Only the first is. `docs/open-items.md:264` already recorded this +> accurately ("`prepublishOnly` wired; nothing published yet"); §10 did not, and now does. +> +> **Still actionable, and not done here:** authoring the release workflow with `--provenance` and +> `id-token: write` is doable today — it is only *exercising* it that needs a registry. That is the one +> remaining piece of work this audit identified and deliberately did not perform, because a release workflow +> is an outward-facing artifact whose shape (trigger, environment, tag convention, who may publish) is a +> project decision rather than a defect repair. +> +> **`NFR-12` was split out of this row and closed on evidence** — 644 emitted files and 9 `npm pack` tarballs +> byte-identical across two clean builds, and a new blocking CI gate +> (`scripts/verify-reproducible-build.mjs`). It is no longer part of this file's scope. *Widened 2026-08-30: +> at audit time the tarball evidence was a by-hand `npm pack` of `@dexpace/core` alone, asserted rather than +> gated. Both legs now run inside the gate — `digestTarballs()` packs every non-`private` package on each of +> the two builds and diffs the SHA-256 maps — so the claim above is verified on every CI run rather than on +> the day it was written.* + +--- + +## 15. A server-issued ETag containing obs-text does not round-trip + +**Verified.** `RequestConditions.applyTo` writes every entity tag through the outbound `Headers` builder's +`set` (`packages/core/src/http/request-conditions.ts:129-142`), which enforces `HTTP-18`'s HTAB + printable +ASCII 0x20–0x7E rule (`packages/core/src/http/ascii-validation.ts:16`). The inbound path is separately laxer +and permits obs-text, exactly as `HTTP-19` requires +(`packages/core/src/http/ascii-validation.ts:41`, `packages/core/src/http/headers.ts:246,262`). + +**Why it should not be corrected.** This one is *technically* correctable — a relaxed emit path for replayed +ETags could be added — and the decision is that it must not be. `HTTP-18` is **MUST**-level and its rationale +is header-injection safety, reinforced by `XCUT-18`, which the product spec treats as a universal invariant +that binds "even if each subsystem individually appears to work." `HTTP-48`'s obs-text permission is +**SHOULD**-level RFC conformance for a rare case, mostly legacy servers. A SHOULD-level nicety does not +outrank a MUST-level cross-cutting security invariant, and adding the relaxed path would create precisely +the two-emit-paths condition that makes splitting defenses fail in practice. Permanent by decision. + +--- + +## 16. Async-runtime adapter fragmentation does not exist + +**Verified.** `packages/rx/` is the only adapter, and its `sseEvents$`/`typedSse$` are documented as +single-subscription because `SseStream` wraps an already-consumed-once response body +(`packages/rx/src/sse.ts:17,41`). No coroutine, reactor, netty, or virtual-thread equivalents exist. + +**Why it cannot be corrected.** The reference's adapter set exists because the JVM has several competing +async ecosystems the SDK must pivot between. Node has one: `Promise`. There is no second ecosystem to bridge +to, so the adapters have no counterpart to be written against. `@dexpace/rx` is sugar over a genuinely +different *data shape* (push-based `Observable`), not the same plumbing under another name — and its +single-subscription behavior is forced by HTTP itself, since a consumed response body cannot be re-read. + +--- + +## 17. `TransportFailureError` adds a third level to a two-level error tree + +**Verified.** `IoError extends DexpaceError` (`packages/core/src/io/errors.ts:13`); the four I/O leaves — +`EndOfStreamError`, `SourceContractViolationError`, `ClosedResourceError`, `AllocationLimitError` — each +extend `DexpaceError` **directly** (lines 29, 49, 65, 80) and are grouped by the `isIoError` predicate +(line 102) rather than by a middle tier. `TransportFailureError extends IoError` (line 126) is the single +three-level branch. + +**Why it cannot be corrected.** `TRANSPORT-20` requires `TransportFailureError` to *be* an `IoError` — the +subtyping is the requirement, not an artifact of modelling. It is also load-bearing: `classify.ts`'s +cause-walk returns retryable for any `IoError`, so the `extends` is what makes a no-response transport +failure retryable with zero edits to the retry layer. A flat sibling would have to be enumerated by hand in +the retry classifier, and again for every transport added later — trading one level of depth for an +open-ended maintenance obligation that the styleguide's own rule exists to prevent. Held at exactly three; +a fourth level is not sanctioned by this entry. diff --git a/docs/knowledge/INDEX.md b/docs/knowledge/INDEX.md index 36d9ff7..5511baa 100644 --- a/docs/knowledge/INDEX.md +++ b/docs/knowledge/INDEX.md @@ -10,7 +10,7 @@ | configuration | `configuration.md` | 51 | design, spec | 0 | 2026-07-25 | | cross-cutting-invariants | `cross-cutting-invariants.md` | 7 | spec | 0 | 2026-07-25 | | data-modeling | `data-modeling.md` | 27 | styleguide | 0 | 2026-07-25 | -| deliberate-deviations | `deliberate-deviations.md` | 13 | design | 0 | 2026-07-25 | +| deliberate-deviations | `deliberate-deviations.md` | 13 | design | 0 | 2026-07-25 — **STALE, see below** | | documentation | `documentation.md` | 21 | styleguide | 0 | 2026-07-25 | | error-handling | `error-handling.md` | 43 | spec, styleguide | 0 | 2026-07-25 | | execution-context | `execution-context.md` | 33 | spec | 0 | 2026-07-25 | @@ -41,3 +41,16 @@ | typescript-idioms | `typescript-idioms.md` | 19 | styleguide | 0 | 2026-07-25 | | url-and-query-encoding | `url-and-query-encoding.md` | 20 | design, spec | 0 | 2026-07-25 | | variables-and-declarations | `variables-and-declarations.md` | 14 | styleguide | 0 | 2026-07-25 | + +## Stale topics + +One row above is annotated rather than refreshed, because re-harvesting is `knowledge-harvest`'s job and that +skill is user-invoked only. Flagged 2026-08-30 by Phase 10. + +- **`deliberate-deviations`** — harvested from the **12-item pre-implementation prediction** in + `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md` (`05d649a`, 62 lines). That + source is now a 238-line, 17-item as-built ledger, corrected twice since (`a0d734d`, `27fb81f`). All 13 + entries are mis-anchored, the `sha:f9ecb6e7d87b` pin in `SOURCES.md` no longer matches (current: + `301f1d519cd8`), and two entries are substantively false — both carry an inline `**Stale…**` marker so the + correction reaches `bun run knowledge` output. Full detail in the topic file's own head banner. **Unblock:** + a `knowledge-harvest` run over that one source, which re-pins `SOURCES.md`, the `` lines, and this row. diff --git a/docs/knowledge/SOURCES.md b/docs/knowledge/SOURCES.md index 2873015..f14c5ab 100644 --- a/docs/knowledge/SOURCES.md +++ b/docs/knowledge/SOURCES.md @@ -49,4 +49,4 @@ | `docs/sdk-design-nodejs/07-pagination-sse-and-serialization.md` | design | `d546f9973c4e` | 2026-07-25 | | `docs/sdk-design-nodejs/08-instrumentation-and-configuration.md` | design | `35281a426195` | 2026-07-25 | | `docs/sdk-design-nodejs/09-toolchain-and-quality-gates.md` | design | `2d2fd9dcfee4` | 2026-07-25 | -| `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md` | design | `f9ecb6e7d87b` | 2026-07-25 | +| `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md` | design | `f9ecb6e7d87b` — **stale, source is now `301f1d519cd8`** | 2026-07-25 — **needs re-harvest**, see `INDEX.md`'s "Stale topics" | diff --git a/docs/knowledge/deliberate-deviations.md b/docs/knowledge/deliberate-deviations.md index c0c6983..cc051c3 100644 --- a/docs/knowledge/deliberate-deviations.md +++ b/docs/knowledge/deliberate-deviations.md @@ -1,5 +1,28 @@ # deliberate-deviations +> **STALE — do not treat these entries as current (flagged 2026-08-30, Phase 10).** Every entry below was +> harvested on 2026-07-25 from the **pre-implementation** revision of +> `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md` (`05d649a`) — the 12-item +> *prediction*, not the as-built ledger. That source has been rewritten and then corrected repeatedly since: +> Phase 10's reconciliation replaced the 12 predictions with a 16-item as-built ledger (`293f2e5`), Phase 8 +> added item 17 for `TransportFailureError`'s third error-tree level (`a0d734d`), and Phase 10's as-built audit +> corrected items 4, 7, 11 and 14 against source (`27fb81f`). Consequences for a `bun run knowledge` consumer: +> +> - **The `sha:f9ecb6e7d87b` pin on every `` line matches only `05d649a`, the oldest revision in that +> file's history.** The source file's current sha-256 prefix is `301f1d519cd8`. +> - **Every `file:line` anchor below is wrong.** They point into a 62-line file; the source is now 238 lines and +> restructured, so the ranges resolve to unrelated text. +> - **Two entries are substantively false**, not merely mis-anchored — the structural-interfaces mitigation and +> the "three tiers, not four" configuration claim. Both carry an inline `**Stale…**` marker below so the +> correction travels with the entry in CLI output. +> +> **Trigger for removing this banner:** a `knowledge-harvest` run over +> `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md`, re-pinning +> `docs/knowledge/SOURCES.md` and the `` lines to the current sha and refreshing +> `docs/knowledge/INDEX.md`'s row. Phase 10 did **not** re-harvest: `knowledge-harvest` is explicitly +> user-invoked only, which its own design doc records as a scope boundary. Until then, read §10 and +> `docs/deviations.md` directly — see §10's own three-file disambiguation table. + ## Rules ## Constraints @@ -15,7 +38,7 @@ design · `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md:17-21` · high · sha:f9ecb6e7d87b - The two retry stacks collapse into one, with the total-timeout budget made explicitly opt-in, a substitution the spec itself sanctions by requiring that a port that unifies retry entry points MUST make that budget explicitly opt-in. design · `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md:22-24` · high · sha:f9ecb6e7d87b -- True runtime encapsulation of domain models is not fully achievable because TypeScript's structural typing means a hand-built object literal can still impersonate a public interface type and bypass builder validation, even though ECMAScript `#private` fields close the "official construction path" hole; this acknowledged language-level limitation is mitigated, not eliminated, by exporting only concrete classes rather than bare structural interfaces from each package's public entry point. +- True runtime encapsulation of domain models is not fully achievable because TypeScript's structural typing means a hand-built object literal can still impersonate a public interface type and bypass builder validation, even though ECMAScript `#private` fields close the "official construction path" hole; this acknowledged language-level limitation is mitigated, not eliminated, by exporting only concrete classes rather than bare structural interfaces from each package's public entry point. **Stale (Phase 10 audit, 2026-08-30): the limitation holds; the mitigation clause is false as stated.** §10 item 4 was corrected against the API report — `packages/core/etc/core.api.md` exports 61 interfaces against 58 classes, and `Configuration` is a builder-built type exported as a bare interface and accepted structurally by `setGlobalConfiguration()`. The mitigation is real for the `http/` wire models it was written about, not a package-wide property. See `docs/deviations.md` §4. design · `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md:25-30` · high · sha:f9ecb6e7d87b - The generic-erasure defense uses schema-as-witness rather than reflective type capture, because TypeScript erases types more completely than JVM generic erasure and leaves no raw class token to reflect over, and this substitution is argued to be at least as strong a guarantee, not a weaker one. design · `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md:31-35` · high · sha:f9ecb6e7d87b @@ -23,7 +46,7 @@ design · `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md:36-40` · high · sha:f9ecb6e7d87b - Digest MD5 needs a vendored implementation while SHA-256 does not, because the Web Crypto API deliberately excludes MD5, so the port vendors a small, dependency-free MD5 implementation for RFC 7616 interoperability and uses `crypto.subtle` directly for SHA-256/SHA-256-sess. design · `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md:41-43` · high · sha:f9ecb6e7d87b -- Configuration layering has three tiers rather than four because the system-property tier is lost outright, Node having no ambient key/value store distinct from environment variables to fill that slot, and the port does not fabricate one. +- Configuration layering has three tiers rather than four because the system-property tier is lost outright, Node having no ambient key/value store distinct from environment variables to fill that slot, and the port does not fabricate one. **Stale (Phase 10 audit, 2026-08-30): false as stated.** §10 item 7 was corrected against the code — all four of `CFG-1`'s tiers are implemented, and the property layer is a first-class caller-supplyable seam (`ConfigurationBuilder.withPropertySource()` and `getRawProperty()` are both public API), so a host that *does* have an ambient store can bind it. What deviates is only the default production wiring: `defaultConfiguration()` binds a property source that always returns `undefined`. See §10 item 7; this item is not in `docs/deviations.md` precisely because it was correctable and was corrected. design · `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md:44-46` · high · sha:f9ecb6e7d87b - Cancellation is `AbortController`/`AbortSignal` end-to-end rather than an interrupt-and-restore-a-flag discipline, composing the same signal type across the transport call, the retry backoff wait, and a derived per-call timeout; since `Promise` has no public `cancel()` unlike `CompletableFuture`, cancellation is cooperative end-to-end, and a `send()` implementation must check `signal.aborted` after resuming from an `await` before treating a resolved value as deliverable. design · `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md:47-53` · high · sha:f9ecb6e7d87b diff --git a/docs/open-items.md b/docs/open-items.md index f8904b4..e188108 100644 --- a/docs/open-items.md +++ b/docs/open-items.md @@ -189,10 +189,30 @@ Phase 1 established the convention ("every new source file opens with `// SPDX-L siblings look accidental rather than scoped. NFR-13 is a review convention, not a mechanical gate, so this is a one-line cleanup on the one file left. Phase 9's `NFR-13` sweep owns it if it is not done sooner. -### B3 — NFR-12: reproducible builds asserted, never proven — **WATCH** - -`bun install --frozen-lockfile` plus plain `tsc` are deterministic by construction, but nothing demonstrates -it. Becomes real at first publish (~Phase 10): build twice, diff artifact digests. +### B3 — NFR-12: reproducible builds asserted, never proven — **CLOSED 2026-08-29** + +Was: `bun install --frozen-lockfile` plus plain `tsc` are deterministic by construction, but nothing +demonstrates it. + +Now proven, and kept proven. Two clean builds of an identical tree (every `dist/` and `*.tsbuildinfo` swept +between them) emit **644 byte-identical files**, and **all 9 publishable packages produce byte-identical +`npm pack` tarballs** across the same two builds. `bun run verify:reproducible-build` +(`scripts/verify-reproducible-build.mjs`) is now a blocking CI step and a `ci-preflight` step, so the assertion +cannot silently rot back into a claim. It was negative-tested by injecting a `Date.now()` into +`packages/core/scripts/gen-version.mjs` — the one build-time codegen step — and confirming the gate fails +naming the offending file. + +**Widened 2026-08-30.** The tarball half was originally a **by-hand** check of `@dexpace/core` alone, +asserted in the closing note rather than gated — the same asserted-not-verified shape this item exists to +catch, one level down. It is now a second leg *inside* the gate: `digestTarballs()` packs every non-`private` +package into a temp dir it owns and SHA-256s each tarball, on both builds, and the two maps are diffed +alongside the emitted-file maps. The leg is cheap (~7s) and deterministic because `npm pack` normalizes tar +entries rather than stamping wall-clock time; what it really pins is that normalization plus any future +`files`/`.npmignore` change that starts shipping something time-varying from outside `dist/`. A missing `npm` +on `PATH` fails the gate loudly rather than skipping the leg. + +The "becomes real at first publish" framing was wrong in one respect worth recording: this needed *code*, not +a *publish*. It was verifiable from Phase 1 onward and stayed open two phases longer than it had to. ### B4 — NFR-14: `expect-type` breaks the single-source-of-versions convention — **WATCH** @@ -261,7 +281,8 @@ No action now. Each is already owned by a named phase; this table exists so none | `contextsEqual()`, value equality over `ExecutionContext` | CTX-5 (equality framing) | none | Built only if 4b or 4c needs one. `CTX-5`'s operative half — pinning an explicit shared key — ships via `ContextInit.key` | | `FakeTransport` test double | — | 4c | 4a never touches `Transport`; `PIPE-9`'s empty-pipeline dispatch is the likely first real consumer | | Self-identifying version metadata (real `User-Agent`) | NFR-15 | 7/8 | | -| Publish + provenance CI job | NFR-16 | release | `prepublishOnly` wired; nothing published yet | +| Publish + provenance CI job | NFR-16 | release | `prepublishOnly` wired; nothing published yet. **Sharpened 2026-08-29:** there is no release workflow at all and `--provenance` appears in no manifest, workflow, or `.npmrc`. §10's ledger claimed the flag "is scripted"; it never was. Authoring the workflow is actionable **now** — only running it against a real registry is blocked | +| `await using` support on `Page`, `fetchTransport()`, `undiciTransport()` | NFR-10 | **none — decided against 2026-08-30** | These three declared `[Symbol.asyncDispose]` as a plain class member; on the `>=20.3` floor the computed key is `undefined`, so the method bound to the string key `"undefined"` — junk on the prototype, no disposal, and a `.d.ts` promising `AsyncDisposable` regardless. Fixed 2026-08-29 to `SseStream`'s guarded install, which costs the type-level `await using` affordance (`close()` is unaffected). **This row previously read "raising the floor to `>=20.4` restores the declaration honestly and lets all four sites drop the guard." That is now a rejected option, not a pending one — the floor stays `>=20.3` and all four guarded installs stay.** Four reasons, in the order that decides it. (1) `NFR-10` is **MUST**-level and requires that "the emitted-artifact target and the visible-API level must agree" (`docs/product-spec/20-non-functional-requirements-and-quality-bar.md:29`); the unguarded class member violated it directly, and the guarded install *is* the repair — not a workaround waiting to be undone. (2) The same requirement's next clause: "A capability that genuinely requires a newer runtime MUST be isolated into its own unit that declares the higher floor explicitly; that unit MUST NOT be a hard dependency of the general-purpose core." Raising core's floor to recover `await using` is the exact inverse — it drags every consumer onto a higher runtime for one syntactic affordance. (3) **The floor is derived, not chosen.** `scripts/verify-runtime-floor.mjs:33` pairs language level `es2023` with `>=20.3`, and its own banner comment (`:22-29`) says the floor is "set by the runtime built-ins the SDK calls rather than by the syntax it emits" and that "adding or moving a row here is a reviewed choice about what runtimes the SDK supports, never a mechanical bump." `>=20.3` is the *minimum* Node that runs what this project emits — `globalThis.crypto` is absent from ESM on every Node 18, and `AbortSignal.any()` landed in 20.3.0. Moving it to satisfy a type-level convenience inverts what the gate is for. (4) **There is a decided precedent.** `docs/superpowers/plans/2026-07-26-phase4-execution-context-and-pipelines-checklist.md:208` rejected raising the floor for `SuppressedError` on the same reasoning and shipped a guarded shim instead — `packages/core/src/suppress.ts`. `close()` remains the supported teardown on every runtime; a consumer who has raised *their own* floor to 20.4+ can still reach the installed member through a cast. See §10 ledger item 11 and I3/J3 below | | NFR-8 re-confirmed as a documented non-applicability | NFR-8 | 10 | No reflection-driven discovery surface exists by design | | Redirect structured logging — hop, rejection, and permitted-downgrade events | REDIR-28, REDIR-15 (surfacing clause), XCUT-17(d) | 7b | Task 9. 5b executes before 7b and 7b needs 5b's step, so the import cannot run either way until then. See G2 | | Redirect's loop-detected and malformed-Location events | REDIR-28 | none | Blocked behind a reason discriminant on `decide()`'s `'return-current'` variant, which no phase owns. See G3 | @@ -1040,8 +1061,24 @@ Node 20.3 (the pinned floor verified by `verify:runtime-floor` and CI `node-conf `Symbol.asyncDispose` (which landed in Node 20.4). TypeScript does not polyfill the well-known symbol for a library that declares the member, so declaring it on the interface would cause `.d.ts` compilation failures for consumers on standard `ES2023` lib without `esnext.disposable`. `SseStream` therefore installs -`[Symbol.asyncDispose]` at run time only when the symbol exists, matching `Response` (HTTP-38). Becomes an -unconditional `implements AsyncDisposable` when `engines.node` moves past Node 20.4. +`[Symbol.asyncDispose]` at run time only when the symbol exists. `Response` (HTTP-38) goes further and ships +no disposal member at all — `close()` is its whole teardown surface, and `http/response.test.ts` pins the +absence of the `"undefined"` prototype key an unguarded declaration would leave behind. ~~Becomes an +unconditional `implements AsyncDisposable` when `engines.node` moves past Node 20.4.~~ + +**Widened 2026-08-30 (Phase 10).** `Page`, `FetchTransport`, and `UndiciTransport` were the three sites that +had declared the member unguarded; all four now share `SseStream`'s shape. See the deferred-items row +"`await using` support on `Page`, `fetchTransport()`, `undiciTransport()`" and §10 ledger item 11. + +**Corrected 2026-08-30 (Phase 10): the "becomes unconditional when the floor moves" sentence is struck, not +merely deferred.** Raising `engines.node` to `>=20.4` to recover the declaration is **decided against** — +`NFR-10` (MUST) both requires the emitted target and the visible API level to agree *and* forbids making a +higher-floor capability a hard requirement of the general-purpose core, and the floor is derived from the +runtime built-ins the SDK calls (`scripts/verify-runtime-floor.mjs:22-29,33`), not chosen. The guarded install +is the permanent shape here, matching the `SuppressedError` precedent (`packages/core/src/suppress.ts`). Full +reasoning and citations in the deferred-items row named above. This item stays **WATCH** only for the narrower +thing it was always about: if a *future* TypeScript or `lib` change makes an optionally-typed declaration +honest on the floor, revisit the typing — never the floor. ### I4 — `SSE-21` hash equality is N/A in JavaScript — **RECORDED** @@ -1069,11 +1106,23 @@ An erratum callout was added to `07-pagination-sse-and-serialization.md` §7.1 a **Trigger:** none. -### J3 — `Page` implements `AsyncDisposable` with `Symbol.asyncDispose` — **RESOLVED** +### J3 — `Page` disposal is a runtime-guarded install, not `implements AsyncDisposable` — **RESOLVED** -`Page` implements `AsyncDisposable` unconditionally (`[Symbol.asyncDispose](): Promise`), delegating to `close()`. Consumers utilizing Explicit Resource Management (`await using`) against `Page` must include `"ESNext.Disposable"` in their compiler `lib`. +**Superseded 2026-08-30 (Phase 10).** This row previously read "`Page` implements `AsyncDisposable` +unconditionally (`[Symbol.asyncDispose](): Promise`)". That was the defect Phase 10's audit found: the +symbol arrived in Node 20.4 and `engines.node` is `>=20.3`, so on the declared floor the computed key +evaluated to `undefined` and the method bound to the string key `"undefined"` — junk on the prototype, no +disposal, and a `.d.ts` promising `AsyncDisposable` regardless. -**Trigger:** none. +`Page` now installs `[Symbol.asyncDispose]` via `Object.defineProperty` at module scope, guarded on the +symbol existing, exactly as `SseStream` does — and deliberately does **not** declare `implements +AsyncDisposable`. `close()` is the supported teardown path; `Paginator.pages()`'s TSDoc names the scoped +constructs that actually discharge `PAGE-12` (`for await`, or `.return()` from a `finally`). A consumer who +has raised their own floor to 20.4+ can still reach the installed member through a cast. + +**Trigger:** none. ~~the `engines.node` bump to `>=20.4`~~ — that bump is **decided against** as of 2026-08-30 +(`NFR-10`, and the floor is derived rather than chosen); the guarded install is permanent. See I3 and the +deferred-items row "`await using` support on `Page`, `fetchTransport()`, `undiciTransport()`" for the citations. ### J4 — WHATWG encode-set boundary & verbatim query splice (PAGE-21, PAGE-22) — **RESOLVED BY DESIGN** diff --git a/docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md b/docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md index 62846d2..537a152 100644 --- a/docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md +++ b/docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md @@ -7,6 +7,23 @@ MUST-level correctness guarantee; each is a case where the JVM-specific mechanis does not exist in Node, and an equivalent, differently-shaped mechanism is substituted instead. Reconciled by Phase 10 (`docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-design.md`), 2026-07-28. +**Three files carry "deviations" in their name. They are not interchangeable** (cross-reference added +2026-08-30, after the first of them was corrected against source without the other two being touched): + +| File | What it is | Numbering | +|---|---|---| +| **This section (§10)** | The **normative ledger** of deliberate deviations — the canonical, as-built list. Every item's number is the one the other files cite. | Owns items 1-17 | +| `docs/deviations.md` | The **as-built audit** of this ledger, performed against source rather than against the phase specs that produced it. Carries the `file:line` evidence for each item, and the record of which items this ledger got wrong. Restates §10's item numbers; it does not assign its own. | Follows §10's | +| `docs/knowledge/deliberate-deviations.md` | Neither. A **harvested corpus topic file** queried by `bun run knowledge`, derived from an *older* revision of this section. Confusingly named; it is not a ledger and must not be edited as one — it is `knowledge-harvest`'s output. **Currently stale** — see its own head banner. | None | + +**Renumbering this section renumbers `docs/deviations.md`.** Its section headings and its two summary tables are +keyed to the numbers above, with no independent identity to fall back on; change one and the other must change in +the same commit. + +**Where a *new* deviation is recorded:** in the owning phase spec's own `## Deviation Ledger (for Phase 10)` +section, never here directly. This section is Phase 10's **output** — the consolidation of those per-phase +ledgers — not their intake. + 1. **Single execution model eliminates every thread/CAS/interrupt-flag primitive, and collapses the sync/async transport seam into one.** **SEAM-11** describes a synchronous, blocking transport contract as distinct from **SEAM-16**'s asynchronous one; Node has no blocking-I/O execution model to give that distinction meaning, so @@ -47,9 +64,15 @@ Phase 10 (`docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-de 4. **True runtime encapsulation of domain models is not fully achievable.** ECMAScript `#private` fields close the "official construction path" hole **HTTP-2**/**SEAM-29** care about, but TypeScript's structural typing means a hand-built object literal can still impersonate a public interface type and bypass builder validation entirely. - This is an acknowledged, language-level limitation, not an oversight; the mitigation — exporting only concrete - classes, never bare structural interfaces, from each package's public entry point — narrows but does not - eliminate the gap (Phase 1). + This is an acknowledged, language-level limitation, not an oversight; the mitigation — exporting the + **`http/` wire-model types** as concrete classes rather than bare structural interfaces — narrows but does not + eliminate the gap (Phase 1). *Corrected 2026-08-29: the mitigation previously read "exporting only concrete + classes, never bare structural interfaces, from each package's public entry point", which the API report + contradicts — `packages/core/etc/core.api.md` exports 61 interfaces against 58 classes. Most are seams + (`Transport`, `Serde`, `Logger`) or options records, where structural typing is the point and no builder + validation is being bypassed. At least one is not: `Configuration` is a builder-built, frozen type exported as + a bare interface and accepted structurally by `setGlobalConfiguration()` and `resolveProxyOptions()`. The + mitigation is real for the domain models it was written about; it is not a package-wide property.* 5. **Schema-as-witness replaces reflective generic-type capture, and the codec-configuration surface it would have carried does not exist.** **SERDE-5**-**SERDE-8**'s mechanism (a reflectively-reconstructed type token) has no TypeScript equivalent — TypeScript erases types more completely than JVM generics erasure, leaving no raw class @@ -63,9 +86,18 @@ Phase 10 (`docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-de portable across non-Node runtimes deliberately excludes MD5. The port vendors a small, dependency-free MD5 implementation for RFC 7616 interoperability and uses `crypto.subtle` directly for SHA-256/SHA-256-sess (Phase 5c). -7. **Configuration layering has three tiers, not four.** **CFG-1**'s override → environment → system-property → - default chain loses its system-property tier outright; Node has no ambient key/value store distinct from - environment variables to fill that slot, and the port does not fabricate one (Phase 7a). +7. **Configuration keeps all four layering tiers, but the platform supplies nothing to bind the third to.** + **CFG-1**'s override → environment → system-property → default chain is implemented in full: `getString` + resolves an exact-key override, then the environment source under the exact key, then the *property* source + under **CFG-3**'s normalized key (lower-cased, `_` → `.`), then the caller's default. The property layer is a + first-class, caller-supplyable `SourceFn` seam — `ConfigurationBuilder.withPropertySource()` and + `getRawProperty()` are both public API — so a host that *does* have an ambient key/value store can bind it. + What deviates is only the **default production wiring**: `defaultConfiguration()` binds a property source that + always returns `undefined`, because Node has no ambient store distinct from `process.env`, and routing a + synthetic "system property" back through `process.env` under a different key would invent a layer the platform + does not have (Phase 7a). *Corrected 2026-08-29: this entry previously read "three tiers, not four — the + system-property tier is lost outright", which understated the as-built code. The tier exists and is + substitutable; only its default binding is empty.* 8. **Cancellation is `AbortController`/`AbortSignal` end-to-end, not "interrupt-and-restore-a-flag."** Every cancellable operation in the port — the transport call, the retry backoff wait, a derived per-call timeout — composes the same signal type. `Promise` has no public `cancel()` unlike `CompletableFuture`; cancellation is @@ -86,13 +118,32 @@ Phase 10 (`docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-de keep/retain configuration) is not applicable by design, full stop — this port has no reflection-driven discovery surface to keep-configure at all, the same discovery machinery Item 2 above already retired. This closes the item permanently rather than leaving it re-flagged for a future phase. -11. **`Symbol.asyncDispose` is adopted opportunistically, not uniformly, and this is deliberate, not drift.** - Internal `io/` primitives ship `close()` only — the symbol postdates the package's declared Node floor - (`>=20.3` since 2026-08-26; on the 20.x line the symbol arrives in 20.4.0), and these types are `@internal` and never surface to a consumer who'd use the ergonomic disposal syntax - (Phase 3a). Public, consumer-facing disposable resources added in later phases — `Body`/`Response` (Phase 3b), - `SseStream` (Phase 6b), `Page` (Phase 6c) — each add `[Symbol.asyncDispose]` as optional and runtime-guarded - rather than declaring `implements AsyncDisposable`, so the type works whether or not the running Node version - supports the symbol, without raising the package's declared floor. Confirmed consistent across all four sites. +11. **`Symbol.asyncDispose` is adopted opportunistically, not uniformly, and every install is runtime-guarded.** + The symbol postdates the packages' declared Node floor (`>=20.3` since 2026-08-26; on the 20.x line the + symbol arrives in 20.4.0), so nothing may declare it as a plain class member: on the floor the computed key + evaluates to `undefined` and binds the method to the string key `"undefined"`, leaving a junk prototype entry + and no working disposal, while the emitted `.d.ts` promises `AsyncDisposable` unconditionally. The port + therefore runs a **two-tier policy**, verified against the code 2026-08-29: + - **`close()` only, no disposal member at all.** Internal `io/` primitives — `@internal`, never surfaced to a + consumer who would use the ergonomic syntax (Phase 3a). Also `Body`/`Response` (Phase 3b), which are + *public* but deliberately teardown-by-`close()`; `http/response.test.ts` and + `body/response-body-logging.test.ts` each pin the **absence** of the `"undefined"` key, and are the origin + of the rule the other tier follows. + - **Guarded runtime install.** `SseStream` (Phase 6b), `Page` (Phase 6c), and both transports — + `FetchTransport` and `UndiciTransport` (Phase 8a) — install `[Symbol.asyncDispose]` via + `Object.defineProperty` behind `typeof Symbol.asyncDispose === 'symbol'`. None declares + `implements AsyncDisposable` and none emits the member into its `.d.ts`, so nothing promises a consumer on + the floor a method that is not there. + + **This entry previously misdescribed the code on three counts and is corrected here rather than restated.** + It claimed `Body`/`Response` add the member (they never have, and two tests assert they do not); it claimed + all sites were "optional and runtime-guarded" (only `SseStream` was — `Page`, `FetchTransport`, and + `UndiciTransport` each declared a plain class member *and* `implements AsyncDisposable`, and both transport + factories publicly returned `Transport & AsyncDisposable`); and it omitted the two transport sites entirely + while asserting consistency "across all four sites". The three unguarded sites were repaired on 2026-08-29 — + see the changeset `2026-08-29-guard-symbol-asyncdispose-installs.md`. The type-system cost of keeping the + floor at `>=20.3` is that `await using` does not type-check against these types; `close()` is the supported + teardown path, and raising the floor to `>=20.4` in a later release would restore the declaration honestly. 12. **The redirect/auth cross-origin marker is a real header, not a `WeakSet`, and its two interpretive questions are now settled by Phase 10 directly, not by a Phase 9 conformance sweep that was never going to run them.** An earlier `WeakSet` design was rejected mid-draft: it breaks once retry's attempt-stamping sits @@ -139,14 +190,24 @@ Phase 10 (`docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-de `Proxy-Authorization` is dropped from the outbound pass — logged by name like any other drop — rather than turning every proxied send into a hard failure. The Phase 8a *plan* had specified a retry-with-stamped- credential flow instead; that flow is not implementable on this platform (Phase 8a). -14. **Reproducible builds and publish provenance stay open, unblocking only at first real release.** **NFR-12** - (byte-identical builds from identical source) and **NFR-16** (publish provenance enforced on the release path) - are soft gaps: `bun install --frozen-lockfile` and plain `tsc` are deterministic by construction, and - `prepublishOnly` + `npm publish --provenance` are scripted (Phase 0 Task 3), but neither has been exercised — - no build artifact or real publish exists yet. Phase 10 does not manufacture a false close here: **NFR-12** - unblocks when the workspace is built twice and the output digests diffed identical; **NFR-16** unblocks when - the scripted publish path actually runs against a real registry. Both remain open, target "first real - release." +14. **`NFR-12` is closed on evidence; `NFR-16` alone stays open until first real release.** These two were + recorded together as soft gaps that "cannot be verified without a real artifact" — true while the repository + was docs-only, and no longer true for the first of them once Phases 1-9 shipped code. They are now separated: + - **NFR-12 (byte-identical builds) — closed 2026-08-29, verified.** Two clean builds of an identical source + tree (every `dist/` and `*.tsbuildinfo` swept between them) produce **644 emitted files, byte-identical**; + `npm pack` of `@dexpace/core` twice produces an identical tarball digest. The check is now a blocking CI + step rather than an assertion — `bun run verify:reproducible-build` + (`scripts/verify-reproducible-build.mjs`), which sweeps, builds twice, and diffs a SHA-256 per emitted + file. It was negative-tested by injecting a `Date.now()` into the one build-time codegen step + (`packages/core/scripts/gen-version.mjs`) and confirming it fails naming the offending file. + - **NFR-16 (publish provenance) — still open, target "first real release."** Its conformance test is + behavioral ("a CI/release build fails an unsigned publication; a local build without keys still publishes + unsigned") and needs a real registry and a real OIDC token. *Corrected 2026-08-29: this entry previously + claimed `npm publish --provenance` was "scripted (Phase 0 Task 3)". It is not — the string appears in no + `package.json`, no workflow, and no `.npmrc`; there is no `.npmrc` and no release workflow at all + (`.github/workflows/` holds `ci.yml` only). Only `prepublishOnly` is wired, exactly as + `docs/open-items.md`'s own row has always said. Authoring the release workflow with `--provenance` and + `id-token: write` is actionable **now** and is the unblocking work; only exercising it needs the registry.* 15. **A server-issued ETag containing obs-text does not round-trip through a conditional request, by deliberate choice.** `RequestConditions.applyTo` writes entity tags through `Headers`' outbound `set`, which enforces **HTTP-18**'s MUST-level restriction (HTAB plus printable ASCII 0x20-0x7E only, rejecting any byte ≥ 0x80). diff --git a/docs/superpowers/plans/2026-07-28-phase9-cross-cutting-conformance-checklist.md b/docs/superpowers/plans/2026-07-28-phase9-cross-cutting-conformance-checklist.md index e63c4c5..b7b9841 100644 --- a/docs/superpowers/plans/2026-07-28-phase9-cross-cutting-conformance-checklist.md +++ b/docs/superpowers/plans/2026-07-28-phase9-cross-cutting-conformance-checklist.md @@ -58,7 +58,7 @@ floor), `api`, `lint:publish`, `verify:dual-consumption`, `verify:consumer-types | `NFR-5` | ✅ | `bunfig.toml` `coverageThreshold = 0.8`, blocking. Actual: 99.72% lines / 98.76% funcs. **Verified live twice** — raising the threshold to 0.999 makes the run exit 1, and a single new file at 66.67% function coverage failed the run on its own while the aggregate stayed at 98.5%. So Bun enforces the floor **per file**, not only in aggregate: stricter than `NFR-5`'s "minimum aggregate" wording requires, and the gate is demonstrably not dormant | | `NFR-6` | ✅ | `tsc --noEmit` per package under `strict`; `typecheck` now covers `shrink-test` and `tests/` too | | `NFR-7` | ✅ | `gts` + `strictTypeChecked`/`stylisticTypeChecked`, fatal. Every `eslint-disable` carries a `-- reason`, including the one added this phase in `error-taxonomy.conformance.test.ts` | -| `NFR-8` | 📋 | **Not applicable by design** — no reflection-driven discovery surface to keep-configure. Deviation ledger, Phase 9 row 2; `docs/knowledge/deliberate-deviations.md:32` | +| `NFR-8` | 📋 | **Not applicable by design** — no reflection-driven discovery surface to keep-configure. Deviation ledger, Phase 9 row 2; `docs/knowledge/deliberate-deviations.md:55` (stale as of 2026-08-30 — read `docs/deviations.md` §10 instead) | | `NFR-9` | ✅ | `@dexpace/shrink-test` (Tasks 1–3): esbuild bundle+minify+tree-shake, 24 KiB budget against a measured 16,671 bytes, then a **child-process** round trip. Guard proven non-vacuous: a separately-bundled `IoError` has a different class identity and `instanceof` is false across the boundary | | `NFR-10` | ✅ | All 10 published packages declare `engines.node >= 20.3`; `verify:runtime-floor` gates target-vs-floor; `test:node` runs the floor and current LTS in CI | | `NFR-11` | ✅ | No `Observable`/`rxjs`/`Subscriber`/`EventEmitter` anywhere in `core.api.md`; `rxjs` appears in no core source file | diff --git a/docs/superpowers/specs/2026-07-23-nodejs-sdk-v1-roadmap-design.md b/docs/superpowers/specs/2026-07-23-nodejs-sdk-v1-roadmap-design.md index 60e2f4f..333b96d 100644 --- a/docs/superpowers/specs/2026-07-23-nodejs-sdk-v1-roadmap-design.md +++ b/docs/superpowers/specs/2026-07-23-nodejs-sdk-v1-roadmap-design.md @@ -62,7 +62,7 @@ spec file. | 8a | Transport Adapters | `@dexpace/transport-fetch`, `@dexpace/transport-undici`, `@dexpace/body-file`, `@dexpace/transport-shared` | §17 | §3.2 (single `Promise` primitive collapses JVM's SEAM-11/SEAM-16 fragmentation) — see [Phase 8 segmentation design](./2026-07-28-phase8-segmentation-design.md) and [Phase 8a design](./2026-07-28-phase8a-transport-design.md) | | 8b | Async-Runtime Bridge | `@dexpace/rx` | §18 | §3.2 (RxJS `Observable` is the only Node-worthwhile async adapter) — see [Phase 8 segmentation design](./2026-07-28-phase8-segmentation-design.md) and [Phase 8b design](./2026-07-28-phase8b-async-runtime-design.md) | | 9 | Cross-Cutting Invariants & Conformance | all packages, `@dexpace/shrink-test` | §19, §20, appendix B | — see [Phase 9 design](./2026-07-28-phase9-cross-cutting-conformance-design.md) and [Phase 9 plan](../plans/2026-07-28-phase9-cross-cutting-conformance.md) | -| 10 | Deviation Reconciliation | — (review only) | — | §10 | +| 10 | Deviation Reconciliation | `@dexpace/core`, `@dexpace/transport-fetch`, `@dexpace/transport-undici`, `@dexpace/shrink-test` — **corrected 2026-08-30**; this cell read `— (review only)` and the phase shipped code. See the Phase 10 status note below | — | §10 | **Status note (2026-07-27).** Phases 5a/5b/5c have a design **and** a written implementation plan; none of the three has been executed — no `src/retry/`, `src/redirect/`, or `src/auth/` exists yet. 5b's and 5c's plans were @@ -135,11 +135,11 @@ permanent simplification, not a postponement. | `NFR-2` — each optional capability a separately installable unit (core + ≤1 external lib) | Phase 0 | **Codec half resolved in Phase 6a** (transport half stays **Phase 8a**) — retargeted 2026-07-28, codec half closed 2026-08-27 | **Closed for the codec half:** `packages/codec-json` ships with `dependencies: {}` hard-committed and zero external libraries, and `scripts/verify-seam-1.mjs` now asserts that for every package under `packages/` rather than for core alone. Originally "Phase 8, no adapter packages exist yet." The Phase 6 segmentation review found the premise false one phase early: `@dexpace/codec-json` is the workspace's first separately installable unit and takes **zero** external libraries — the cleanest instance of the requirement in the whole roadmap. 6a disposes of the codec half; `transport-fetch`/`transport-undici` close the rest in 8a — `transport-fetch` trivially (zero external libs), `transport-undici` with exactly one (`undici`). See the [Phase 8 segmentation design](./2026-07-28-phase8-segmentation-design.md) | | `NFR-9` — automated shrink-survival regression guard | Phase 0 | **Resolved in Phase 9 (design)** | Explicitly out of scope per the scaffold design's own "Out of scope" list. Phase 9's design ships `@dexpace/shrink-test` (private, unpublished devDependency): an esbuild bundle/minify/tree-shake step, a dual-package-hazard fixture app, and a child-process round-trip guard wired into the default build as `bun run shrink-test`. Lands when Phase 9's plan executes | | `NFR-11` — concurrency-model agnosticism, no async-framework type leak | Phase 0 | **Resolved in Phase 4c** | 4c's `Step`/`Next`/`Runtime` public surface is `Promise`-only — no RxJS, no generator, no framework-specific async type appears anywhere in the pipeline layer. Deferral closed | -| `NFR-12` — reproducible, byte-identical builds | Phase 0 | Phase 10 / first real release | Still open — Phase 10's reconciled ledger (`docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md`, Item 14) records the intended verification (double-build the workspace, diff output digests) but cannot execute it without a real build artifact. Unblocks at first real release | +| `NFR-12` — reproducible, byte-identical builds | Phase 0 | **Closed 2026-08-29** | **Verified, not asserted.** Two clean builds of an identical tree emit 644 byte-identical files, and all 9 publishable packages produce byte-identical `npm pack` tarballs across the same two builds. Now a blocking CI step, `bun run verify:reproducible-build` (`scripts/verify-reproducible-build.mjs`), negative-tested by injecting a `Date.now()` into `gen-version.mjs`. (Widened 2026-08-30: the tarball comparison was a by-hand check of `@dexpace/core` alone when this row was written; it is now a second leg inside the gate, over every publishable package, on both builds.) The "cannot execute without a real build artifact" premise expired once Phases 1–9 shipped code. See Phase 10's reconciled ledger, Item 14 | | `NFR-13` — SPDX license header per source file | Phase 0 | Phase 1 onward — **written into Phase 1's plan (2026-07-28)** | Soft gap; the spec itself calls this "a review convention, not a mechanical gate". A 2026-07-28 plans review found no phase plan actually carried the convention, so Phase 1's plan now states it in its Global Constraints (`// SPDX-License-Identifier: MIT`, line 1 of every new file, all phases onward) — enforcement stays review-level | | `NFR-14` — single source of truth for dependency/tool versions (Bun `catalog:`-equivalent) | Phase 0 | **Resolved in Phase 6a** — retargeted 2026-07-28, closed 2026-08-27 | **Closed.** The workspace root's `workspaces.catalog` block is the single source of version truth for `typescript`, `@microsoft/api-extractor`, `expect-type`, and `fast-check`; the root's own `devDependencies` and both member packages reference them as `"catalog:"`, so a bump is a one-line edit. Bun 1.4.0 local / 1.3.14 pinned both support catalogs, so the fallback Task 8 allowed for was not needed. Was: trivially true (one package, zero deps); the row's own text said it "becomes a real decision the moment a second package with its own dependencies exists." That moment is 6a scaffolding `@dexpace/codec-json`, not Phase 8. 6a picks the Bun equivalent of the pnpm `catalog:` protocol `sdk-design-nodejs/02` specifies, confirmed against `styleguide/typescript-bun/` | | `NFR-15` — self-identifying version metadata (real `User-Agent`, never a placeholder) | Phase 0 | **Resolved in Phase 7a (design)** / **Phase 8a** | 7a's design ships `CFG-36`'s build/runtime descriptor (version via build-time codegen, never a runtime placeholder) and `RECOV-33`'s client-identity step that stamps it into `User-Agent`. Node-transport wiring (the header actually reaching the wire) still waits for 8a's concrete transports — a conformance test confirming `TRANSPORT-11`'s header-drop pass leaves it untouched, not new stamping logic. See the [Phase 8 segmentation design](./2026-07-28-phase8-segmentation-design.md) | -| `NFR-16` — publish provenance enforced on the release path | Phase 0 | Phase 10 / first real release | Still open — Phase 10's reconciled ledger (`docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md`, Item 14) records the intended verification (run the scripted `prepublishOnly` + `npm publish --provenance` path for real) but cannot execute it without a real publish. Unblocks at first real release | +| `NFR-16` — publish provenance enforced on the release path | Phase 0 | Phase 10 / first real release | Still open — Phase 10's reconciled ledger (`docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md`, Item 14) records the intended verification (run `prepublishOnly` + `npm publish --provenance` for real) but cannot execute it without a real publish. Unblocks at first real release. **Correction 2026-08-29:** only `prepublishOnly` is wired — `--provenance` appears in no `package.json`, workflow, or `.npmrc`, and there is no release workflow at all. Authoring it is actionable now; only exercising it needs a registry | | `NFR-8` — shrinker keep/retain configuration | Phase 0 | Phase 10 (Deviation Reconciliation) — closed 2026-07-28 | Re-confirmed as not applicable by design in Phase 10's reconciled ledger (`docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md`, Item 10) — this port has no reflection-driven discovery surface to keep-configure. Closed 2026-07-28 | | Peer-dependency dedup for `@dexpace/core` (dual-package-hazard guard) | Phase 0 | **Resolved in Phase 6a** — retargeted 2026-07-28, closed 2026-08-27 | **Closed.** `@dexpace/codec-json` declares the `@dexpace/core` peer plus its `peerDependenciesMeta` entry; `scripts/verify-seam-1.mjs` asserts both for every non-core package, and `packages/codec-json/src/cross-package.test.ts` proves the consequence rather than the declaration — a `Tristate` constructed in core is recognized by the codec's replacer, because `TRISTATE_BRAND` is a registry-global `Symbol.for`. Mechanism specified in `sdk-design-nodejs/02` §2. `@dexpace/codec-json` is the first package to declare the `@dexpace/core` peer, so the guard installs in 6a. Not theoretical for this package specifically: `sdk-design-nodejs/02` names the `Tristate` discriminant and the `Outcome` sum type as exactly the branded-symbol checks two non-identical copies of core would break — and `Tristate` is 6a's own deliverable | | `NFR-10`/`NFR-17` residual — CI running the built artifact against the *declared minimum* Node version (18.17), not just whatever the runner defaults to | Phase 0 | **Resolved in Phase 2 (plan)** (pulled forward from Phase 3) | Low-risk while the only export was a trivial `ping()`. Phase 2 is where it stops being trivial: `composeSignal()` calls `AbortSignal.any()`, which landed in **exactly** Node 18.17.0 — the declared floor to the patch version. Phase 2's plan Task 7 ships the `node-floor-conformance` CI job (`actions/setup-node` pinned to 18.17.0 running `scripts/verify-node-floor.mjs`, which forces the `AbortSignal.any()` branch); its checklist marks the row ✅. Lands when Phase 2's plan executes | @@ -166,7 +166,7 @@ permanent simplification, not a postponement. | `RETRY-29` — opt-in server-driven retry-classification override header | Phase 5a brainstorm | Not scheduled | `MAY`. Lets a response header force or suppress the retry classification. Widens the classifier's input surface to server-controlled values, which is a trust decision deserving its own deliberation rather than a default. No caller identified | | `RECOV-33` — client-identity header step (Append/Replace token composition, blank-line suppression) | Phase 5a brainstorm | **Resolved in Phase 7a (design)** | One of only two appendix-C `RECOV-17`–`RECOV-34` rows with no `§9` `RETRY-*` twin (the other, `RECOV-32`'s idempotency key, shipped in 5a because retry preserves it per `RETRY-38`). Pure configuration-driven header composition with zero retry coupling, so it travels with `CFG-*` in 7a, ships as `clientIdentityStep()` consuming `CFG-36`'s build/runtime descriptor, and closes `NFR-15` alongside it. Lands when 7a's plan executes | | `StepContext.signal` **and** `StepContext.options` — exposing the call's `AbortSignal` and per-call `RequestOptions` to steps | Phase 5a brainstorm (`signal`); 2026-07-28 plans review (`options`) | **Phase 5a, Task 1** | Found during 5a's spec self-review: 4c's `Cursor` accepts and threads a `signal` but `StepContext` never exposed it, so no step could observe cancellation — `RETRY-26`'s cancellable wait and `RETRY-32`'s "no attempts after cancellation" were both unimplementable. A 2026-07-28 review found the identical gap for `options`: `Cursor` threads them to terminal dispatch but `PIPE-17`'s "readable by any step" MUST was unsatisfied, and with it `RETRY-41`'s per-call override (`RequestOptions.maxRetries`, `HTTP-35`'s "0 disables retries for this call") had no wire — Phase 1 designed the knob, nothing read it. Both fields land as one additive amendment in 5a Task 1; 5a Task 9 wires the retry override, 5c Task 14 wires the per-call auth descriptor. **2026-07-29:** 4c's own design and plan now record the `PIPE-17` half as a deferral naming 5a Task 1, so the MUST is no longer deferred silently (4c validation review, F1); 4c's plan also forbids adding the two fields early, since their shape belongs to their first reader | -| `RequestOptionsBuilder.maxRetries` accepts `Infinity`, `NaN`, and fractional values | Phase 5a code review (2026-08-26) | **Phase 10 (Deviation Reconciliation)** — or a Phase 1 fix with a changeset | `HTTP-35`'s stated intent is that an out-of-range retry count is a loud error, never silently reinterpreted, and the builder implements only the `< 0` half. `Number.isFinite`/integer are unchecked, so `maxRetries: Infinity` reaches a consumer as a budget that never terminates. Phase 5a found it because its per-call override feeds `maxAttempts` directly; 5a closed its own exposure at both ends (`retryStep`'s `effectiveSettings` and a precondition in `runWithRetry`), but the **builder** still accepts the value, so any future reader of the option inherits the trap. Tightening a public setter changes observable API behavior and needs a changeset, so it is recorded rather than folded into 5a | +| `RequestOptionsBuilder.maxRetries` accepts `Infinity`, `NaN`, and fractional values | Phase 5a code review (2026-08-26) | **Resolved — closed in Phase 5's merge `cba4721` (2026-08-27)**, by the row's own second option (a Phase 1 fix with a changeset), *not* by Phase 10. Re-verified against source 2026-08-30 | `HTTP-35`'s stated intent is that an out-of-range retry count is a loud error, never silently reinterpreted, and the builder implements only the `< 0` half. `Number.isFinite`/integer are unchecked, so `maxRetries: Infinity` reaches a consumer as a budget that never terminates. Phase 5a found it because its per-call override feeds `maxAttempts` directly; 5a closed its own exposure at both ends (`retryStep`'s `effectiveSettings` and a precondition in `runWithRetry`), but the **builder** still accepts the value, so any future reader of the option inherits the trap. Tightening a public setter changes observable API behavior and needs a changeset, so it is recorded rather than folded into 5a. **Closed:** the setter is now `if (value !== undefined && !(Number.isInteger(value) && value >= 0))` throwing `RequestOptionsValidationError` (`packages/core/src/http/request-options.ts:178`), which covers `Infinity`, `NaN` and fractional values in one predicate; its TSDoc `@param`/`@throws` say so (`:170-175`). Shipped with `.changeset/2026-08-26-max-retries-range-check.md` (`@dexpace/core`, patch), authored 2026-08-26 and merged in `cba4721`. This row stayed open past its own resolution — **Phase 10 did not fix it and should never have been named as its owner**; the correction here is bookkeeping, not work | | The two structured retry log events (`retry.attemptFailed`, `retry.exhausted`) and `RETRY-40`'s "log the failure" clause | Phase 5a execution (2026-08-26) | **Phase 7b, Task 9** | 5a's plan specifies all three emission points but its own 2026-07-29 correction forbids writing them: 5a executes before 7b, so an `observability/logger.js` import would not resolve, and 7b needs 5a's `FakeTransport`, so the dependency cannot run the other way. `engine.ts` carries a head comment marking the sites and naming 7b Task 9 as owner. `RETRY-40`'s non-fatal fall-back half **is** implemented in 5a; only the diagnostic half waits | | Phase 7a Tasks 1-3 (`config/{clock,http-date,retryable}.ts`) executed early, as 5a's prerequisite | Phase 5a execution (2026-08-26) | **Executed — 7a's plan should mark Tasks 1-3 done, not rebuild them** | 5a's plan Prerequisite requires 7a's `config/` module to exist first (Task 8 consumes `Clock`, Task 4 imports `parseHttpDate`, Task 2 re-exports `isRetryableStatus`), and its Global Constraints ban shipping private copies. The three files were built verbatim from [7a's plan](../plans/2026-07-28-phase7a-configuration.md) Tasks 1-3 with their tests (22 tests, `CFG-15`-`CFG-17`, `CFG-29`-`CFG-31`, `CFG-35`). 7a's Tasks 4-10 are untouched, and none of the three is promoted to the public barrel — 7a Task 10 still owns that decision | | `SEAM-30` cleanup (cancel an orphaned response on the completion race) | Phase 2 | **Phase 8a** | Documented as a TSDoc contract obligation on `Transport.send()` in Phase 2; only a real Transport implementation has a response to actually cancel. Collapses onto `TRANSPORT-9` (and `ASYNC-5`, which collapses onto the same thing) per the [Phase 8 segmentation design](./2026-07-28-phase8-segmentation-design.md) §5.1 — closes as part of 8a's conformance suite, not separate work | @@ -200,6 +200,9 @@ permanent simplification, not a postponement. | Whether `standardResilience()` should also accept a `tracerFactory`/`meter` pass-through convenience | Phase 7b brainstorm | **Resolved in Phase 9 (design)** — no friction found | No requirement mandates preset-level convenience wiring beyond installing the `LOGGING` step itself. Phase 9's `tests/conformance/xcut/fixtures/composed-pipeline.ts` configures logging/tracing/metrics the same way 7b's own tests do — a `LoggingStepSettings` object passed to `standardResilience()`'s existing `logging` option, plus `setGlobalLogger()` for a spy `Logger` — with no need for a separate `tracerFactory`/`meter` preset-level parameter. Closed, not just deferred again | | A real `@opentelemetry/sdk-metrics`-backed `Meter` adapter package | Phase 7b brainstorm | Not scheduled | `OBS-31` only requires the no-op default and that core not depend on a metrics runtime; no package in the roadmap's phase table ships a concrete metrics backend, unlike tracing's duck-typed zero-adapter path | | Phase 8 split into 8a (Transport Adapters, `§17`) / 8b (Async-Runtime Bridge, `§18`) | Phase 8 brainstorm (2026-07-28) | — | 52 nominal combined IDs (`TRANSPORT` 30, `ASYNC` 22) — well under the ~76–79 that forced the Phase 3/4 splits — but §17 is paid twice (two full `Transport` implementations, `transport-fetch` and `transport-undici`) and nine Deferred Items Log rows land here, pushing effective weight to Phase-7-before-its-split territory. Cut along the package boundary the roadmap table already implied, verified empty by the same test Phase 6 applied: `@dexpace/rx` depends only on Phase 6's `Page`/`SseStream`, never on `Transport`, and nothing in `Transport`'s collapsed `Promise`-returning contract (`sdk-design-nodejs/03` §3.2) references RxJS or any `ASYNC-*` id. **No segment depends on the other; the 8a→8b order is convenience, not dependency** (8a leads only because it is the larger, riskier half). A large share of `§18`'s `ASYNC-*` IDs collapse onto their `§17` `TRANSPORT-*` twin (the SEAM-11/SEAM-16 collapse restated at the async-adapter layer) or are inapplicable outright — Node has no blocking-transport/worker-thread-pool model for `ASYNC-3`/`4`/`7`/`14` to bite on, the same premise that already closed `SEAM-18` as "Never." Full rationale, per-segment ownership, the collapsed-ID disposition tables, and open items (notably `FileBody`'s package placement and whether Node's HTTP stack has any zero-copy dispatch path at all) in the [Phase 8 segmentation design](./2026-07-28-phase8-segmentation-design.md) | +| Assertion-density rule applied project-wide (`assertions.md:6-7`, styleguide Rule 8) | Phase 4b validation review F2 (2026-07-28) | **Not scheduled — re-deferred out of Phase 10, 2026-08-30** | Named Phase 10 by 4b's F2 resolution, but a project-wide convention sweep is not deviation reconciliation, and Phase 10 is the last row of the table above — so this becomes unscheduled with an explicit trigger rather than being handed to an invented phase. As-built the shape has changed: `invariant()` is now called from thirteen modules across `packages/core/src/` and `packages/body-file/src/`, and `recovery/` is the lone holdout at zero (`packages/core/src/recovery/outcome.ts:3` imports `assertNever` only). **Trigger:** the next defect traced to an unasserted precondition, or an assertion/naming convention sweep commissioned as its own phase. Full disposition in the Phase 4b review section below | +| `#private`-vs-`private` field style settled project-wide, with the runtime-privacy justification stated | Phase 4b validation review F7 (2026-07-28) | **Not scheduled — re-deferred out of Phase 10, 2026-08-30; largely moot** | Also named Phase 10, same reasoning as the row above. Mostly discharged in the meantime by a different route than a sweep: `CLAUDE.md:172-173` now mandates "`#private` fields only. Not TS `private`." project-wide and cites styleguide 6.7's library carve-out as the justification F7 asked for. The residue is cosmetic (no per-class comment). **Trigger:** a lint rule mechanizing the convention, or a styleguide revision withdrawing the 6.7 carve-out. Full disposition in the Phase 4b review section below | +| `CONSTANT_CASE`-vs-`lowerCamelCase` for module-level immutable collections (`STAGE_ORDER`, `PILLAR_STAGES`, Phase 1's `Protocol`/`Status` statics, 4b's constants) | Phase 4c validation review (2026-07-29) | **Not scheduled — re-deferred out of Phase 10, 2026-08-30** | Also named Phase 10; a naming-convention call is not a deviation from the reference contract. `naming-conventions.md:14`'s worked example keeps a module-level `new Set(...)` in `lowerCamelCase` because its contents can mutate, and a `ReadonlySet` type does not deep-freeze the underlying `Set`. Unchanged and self-consistent as-built — `STAGE_ORDER`/`PILLAR_STAGES` remain the pipeline's only such pair (`packages/core/src/pipeline/builder.ts:12`). **Trigger:** the next module-level immutable collection added outside `pipeline/`, which makes the fork visible in a third place. Full disposition in the Phase 4c review section below | **Status note (2026-07-28, Phase 7).** Phase 7 was brainstormed and split into 7a (Configuration & Platform Primitives, `§16`) / 7b (Instrumentation & Observability, `§15`) — see the @@ -292,6 +295,42 @@ Also closed as part of this pass: three `unresolved 2026-07-25` markers in `docs had already decided but never back-ported into the corpus itself — directly relevant here since `NFR-5`/`NFR-6`/ `NFR-7` are exactly the rows those stale markers left unconfirmed. +**Status note (2026-08-30, Phase 10 EXECUTED — scope corrected).** Phase 10 is executed, and it **shipped code**. +The phase-table row above and this phase's own design (`2026-07-28-phase10-deviation-reconciliation-design.md:15`, +"Phase 10 ships no package") both said the opposite; both are corrected in place rather than overwritten, because +an unrecorded scope change is the exact failure mode this phase spent its audit correcting elsewhere. What +actually landed, on `25-phase-10-deviation-reconciliation`: + +- **A live defect, found by auditing the ledger against source rather than against the specs that produced it.** + `Page`, `FetchTransport` and `UndiciTransport` each declared `[Symbol.asyncDispose]` as a plain computed class + member. The symbol arrived in Node 20.4 and every package declares `engines.node ">=20.3"`, so on the declared + floor the computed key evaluated to `undefined` and the method bound to the string key `"undefined"` — junk on + the prototype, no disposal, and a `.d.ts` promising `AsyncDisposable` regardless (`NFR-10`). All three now + install it through a guarded module-scope `Object.defineProperty`, matching `SseStream` + (`packages/core/src/pagination/page.ts:114`, `packages/transport-fetch/src/fetch-transport.ts:314`, + `packages/transport-undici/src/undici-transport.ts:566`, `packages/core/src/sse/stream.ts:209`). +- **A breaking type change across three packages,** with two changesets: `Page` no longer declares `implements + AsyncDisposable` and the two transport factories no longer return `Transport & AsyncDisposable`, so `await + using` stops type-checking. Pre-1.0, so `minor` per the same initial-development carve-out the earlier `Body` + narrowing used. +- **A new blocking CI step** closing `NFR-12` on evidence — `bun run verify:reproducible-build` + (`scripts/verify-reproducible-build.mjs`), see the `NFR-12` row above. +- **Three further defects, from three subsequent review passes:** a `verify-dual-consumption` assertion that + passed on the floor only *because* of the junk prototype key, a dispatcher leak in `UndiciTransport.close()` + where the first rejecting `destroy()` aborted the reverse walk and stranded the `ProxyAgent` holding the pooled + connections, and a stranded body producer in `send()` from evaluating `prepareBody()` before header mapping. +- **An extended shrink guard** — `packages/shrink-test/` now asserts the disposal installs survive a real esbuild + `bundle + minify + treeShaking` pass, which is the standing evidence for keeping `"sideEffects": false` on the + three packages carrying one. + +**Why the "review only" scope was right to break, and where that judgment is recorded.** The audit's method — +re-derive every ledger claim from as-built source — is what surfaced the defect; a documents-only phase would +have copied the wrong claim forward. Fixing a live correctness defect found *by* the audit is inside the phase's +purpose, and leaving it recorded-but-unfixed would have shipped a `.d.ts` that lies on the declared floor. The +project-wide **convention sweeps** that also named Phase 10 were held to the original scope and re-deferred +instead — see the three rows added to the Deferred Items Log above and the dated dispositions on 4b's F2/F7 and +4c's `CONSTANT_CASE` note below. Per-item evidence: `docs/deviations.md` (the as-built audit). + ## Open Findings — Phase 3b Validation Review (2026-07-28) A validation pass over `specs/2026-07-25-phase3b-body-lifecycle-design.md` and @@ -453,12 +492,12 @@ parallel edits, so the cross-phase obligation is discharged by 4b: each of the o | # | Sev | Finding | Where | Resolution | |---|---|---|---|---| | F1 | **blocker** — ✅ closed | `SuppressedError` does not exist on the declared runtime floor. `engines.node` is `">=18.17"`, raised at most to `18.18.0` at the 2026-07-25 checkpoint (which exposes `Symbol.dispose`/`Symbol.asyncDispose` only — Node backported those two symbols; `SuppressedError` is a V8 global from the full Explicit Resource Management proposal). `esnext.disposable` in `lib` supplies its *type*, so `new SuppressedError(...)` type-checks and then throws `ReferenceError` at call time — the exact `NFR-10` trap `tooling-and-quality-gates.md:60-61` describes. `bun test` passes locally; the `node-floor-conformance` job pinned to `18.17.0`, `verify:node-floor` and `test:node` all fail | PLAN:19-20 (Tech Stack, claims it is "already available since Phase 3b's checkpoint lib bump" — false), PLAN:804, SPEC:124; also 5a plan:36, 6b design:163, 6c design:192 | **Resolved 2026-08-26: take branch (b)** — the runtime-guarded `suppress()` helper. The "confirm the first supporting Node release" condition this row left open is now discharged, and it settles the choice rather than merely informing it; two of this row's own premises also turn out to be false. See "F1 resolution — the verified version facts" below the table. **Partially applied 2026-07-28:** the false Tech Stack claim is deleted and replaced with a blocking notice at the top of the plan stating the real constraint; **Applied 2026-08-26:** `packages/core/src/suppress.ts` ships the guarded helper, `response-chain.ts` calls it, and assertions are written against its shape rather than `instanceof SuppressedError` — the `instanceof` form would silently assert nothing on the floor runtime | -| F2 | major — ✅ closed | Zero assertions across the whole `recovery/` module — a dozen functions, no `invariant()` call, against `assertions.md:6-7`'s 2-per-function module average (and `styleguide-overview.md:22-23` Rule 8). Neither document acknowledges the rule or argues an exemption. Concretely: no `apply()` checks that a step returned a value at all, so a step returning `undefined` poisons the fold silently. Project-wide inconsistency, not 4b's alone — Phases 1/2/3b/4a ship zero, 4c ships fifteen | PLAN:463-479, 818-859, 964-966, 1352-1370 | **Resolved 2026-08-26: Deviation Ledger row.** Recorded in 4b's design with the concrete cost named (a step returning `undefined` poisons the fold silently). Assertions added to 4b alone would deepen the 0-vs-15 split with 4c rather than close it, so the density rule is settled once at Phase 10 and applied project-wide | +| F2 | major — ✅ closed | Zero assertions across the whole `recovery/` module — a dozen functions, no `invariant()` call, against `assertions.md:6-7`'s 2-per-function module average (and `styleguide-overview.md:22-23` Rule 8). Neither document acknowledges the rule or argues an exemption. Concretely: no `apply()` checks that a step returned a value at all, so a step returning `undefined` poisons the fold silently. Project-wide inconsistency, not 4b's alone — Phases 1/2/3b/4a ship zero, 4c ships fifteen | PLAN:463-479, 818-859, 964-966, 1352-1370 | **Resolved 2026-08-26: Deviation Ledger row.** Recorded in 4b's design with the concrete cost named (a step returning `undefined` poisons the fold silently). Assertions added to 4b alone would deepen the 0-vs-15 split with 4c rather than close it, so the density rule is settled once at Phase 10 and applied project-wide. **Re-deferred 2026-08-30 (Phase 10): NOT SCHEDULED.** Phase 10's scope is deviation reconciliation; a project-wide assertion sweep is neither a deviation nor a reconciliation, and Phase 10 is the last row of the phase table, so there is no later phase to hand it to and none is invented here. The picture has changed since 4b: `invariant()` is now called from thirteen modules across `packages/core/src/` and `packages/body-file/src/` (`body`, `auth`, `observability`, `io`, `retry`, `pagination`, `config`, `sse`, `redirect`, `pipeline`, `serde`, `context`, `testing`), so the 0-vs-15 split is no longer the shape of the problem — `recovery/` is now the outlier, still with zero (`packages/core/src/recovery/` imports only `assertNever`, `outcome.ts:3`). **Trigger:** the next defect traced to an unasserted precondition, or a naming/assertion convention sweep commissioned as its own phase — whichever comes first. Logged in the Deferred Items Log above so it is tracked rather than silent | | F3 | major — ✅ applied | SPEC:270 still says "the only new failure surface is `wrapCancellation()`'s `invariant()` crash" — stale text from a superseded draft. SPEC:194-204, SPEC:279 and PLAN:63-74 all state the opposite. An agent executing from the File Layout section would restore the `invariant()`, and because the helper runs inside `dispatchWithRecovery`'s own `catch`, that throw bypasses the response and recovery chains — the one failure mode `RECOV-2` exists to prevent | SPEC:270-271 | Replace with `assertNever`'s `InvariantViolation` crash, matching the already-correct PLAN:89-90 | | F4 | minor — ✅ applied | Spec never designs the `assertNever` addition Task 1 builds. PLAN modifies `packages/core/src/invariant.ts` (new exported symbol, two tests, its own commit); SPEC's File Layout lists only `recovery/` | SPEC:258-268 vs PLAN:102-103, 124-197 | Add the `invariant.ts` line to the spec's File Layout with a one-line note that `fold()` is the codebase's first discriminated-union `switch` | | F5 | minor — ✅ applied | `RECOV-14`'s second normative sentence (steps safe for concurrent invocation; per-request state never on the step instance) is claimed but neither designed nor tested — both documents cite `RECOV-14` for the defensive copy only. The design does satisfy it (all per-call state is local), but nothing records or guards that | SPEC:141-144, PLAN:49-51 | One sentence in the design + one plan test interleaving two `apply()` calls on one chain | | F6 | minor — ✅ applied | `RECOV-32`/`RECOV-33` read as silent drops. 4b's deferral sentence covers "backoff, budget, pacing headers → Phase 5"; neither an idempotency-key header injector nor `User-Agent` composition is any of those. Both *are* built — `RECOV-32` in Phase 5a Task 11, `RECOV-33` in Phase 7a Task 9 — but 4b names neither, and 7a is not "Phase 5" | SPEC:18-20 | Extend the Scope sentence to name `RECOV-17`–`RECOV-31`/`RECOV-34` → 5a, `RECOV-32` → 5a, `RECOV-33` → 7a | -| F7 | minor — ✅ applied | `#private` fields with no justifying comment, against `data-modeling.md:20-23` (`private` is the default; `#private` needs a stated runtime-privacy requirement). Neither chain class needs it — unlike 3b's `Response`, whose `#closed` genuinely must survive `Object.freeze(this)`. Inherited pattern: 4a's `ContextStore` does the same | SPEC:64, 78-79; PLAN:464, 819-820, 833, 847 | Ledger row recording `#private` as the package-wide field style with no runtime-privacy claim; project-wide reconciliation is Phase 10's | +| F7 | minor — ✅ applied | `#private` fields with no justifying comment, against `data-modeling.md:20-23` (`private` is the default; `#private` needs a stated runtime-privacy requirement). Neither chain class needs it — unlike 3b's `Response`, whose `#closed` genuinely must survive `Object.freeze(this)`. Inherited pattern: 4a's `ContextStore` does the same | SPEC:64, 78-79; PLAN:464, 819-820, 833, 847 | Ledger row recording `#private` as the package-wide field style with no runtime-privacy claim; project-wide reconciliation is Phase 10's. **Re-deferred 2026-08-30 (Phase 10): NOT SCHEDULED, and mostly moot.** The finding's actual ask was a *stated* runtime-privacy requirement, and the project has since stated one: `CLAUDE.md:172-173` makes "`#private` fields only. Not TS `private`." a mandated construction rule and cites styleguide 6.7's carve-out for libraries whose internals must stay unreachable reflectively. That is the justification F7 asked for, adopted project-wide rather than argued per class — `packages/core/src/http/status.ts:22-23` and `packages/core/src/recovery/request-chain.ts:26` are the same shape. What is left is cosmetic (no per-class comment) and has no owner: Phase 10 is the last phase, and a convention already written into `CLAUDE.md` does not need a sweep to enforce it. **Trigger:** a lint rule that mechanizes the convention, or a styleguide revision that withdraws the 6.7 carve-out. Logged in the Deferred Items Log above | | F8 | minor — ✅ applied | Plan's `ResponseRecoveryChain` property test drops half of what the spec specifies. SPEC promises the property also proves the response-step phase never runs on a `Failure` input (`RECOV-4`); the plan's generator emits recovery steps only and never seeds a `Failure`, asserting only that `apply()` settles | SPEC:293-295 vs PLAN:754-773 | **Applied 2026-07-28 — generator extended**, not spec narrowed: the property now generates response *and* recovery steps over a seed that is arbitrarily `Success` or `Failure`, and asserts `responseStepRuns === 0` on every `Failure` seed. Task 3's expected test count moves 12 → 13 | | F9 | minor — ✅ applied | `fold(outcome, onSuccess, onFailure)` takes three positional parameters, tripping `function-design.md:22-23` ("options object at 3 or more"), which is one stricter than the lint gate (`max-params: ['error', 3]` errors at four). Passes CI while violating the corpus. Phase 2's shipped `Transport.send(request, options?, signal?)` is the same shape | SPEC:36, PLAN:320 | Ledger row recording it as deliberate (matching `Transport.send`), or `fold(outcome, {onSuccess, onFailure})`. See the corpus conflict below | | F10 | minor — ✅ applied | `statusMappingStep` is a module-level `const` arrow, against `function-design.md:18-21` ("top-level named `function` declarations… arrows are reserved for inline callbacks"). `func-style`'s `allowArrowFunctions: true` will not catch it, and named declarations survive in stack traces — which matters for a function whose whole job is to `throw` | SPEC:227, PLAN:1081 | `export async function statusMappingStep(...)` plus `statusMappingStep satisfies ResponseStep` to keep the conformance check | @@ -533,3 +572,13 @@ module-level `new Set(...)` staying `lowerCamelCase` because its contents can mu not make the underlying `Set` deeply immutable and `Object.freeze` cannot fix a `Set`. Left alone because the casing question is project-wide (Phase 1's `Protocol`/`Status` statics, 4b's constants) and renaming one phase's two constants would fork the convention rather than settle it — Phase 10's reconciliation owns it. + +**Re-deferred 2026-08-30 (Phase 10): NOT SCHEDULED.** Phase 10 does not own it and did not settle it. The +`CONSTANT_CASE`-vs-`lowerCamelCase` question for module-level immutable collections is a naming-convention +call, not a deviation from the reference contract, so it is outside a reconciliation phase's scope; Phase 10 is +also the last row of the phase table, so there is no later phase to hand it to and none is invented here. The +state is unchanged and still consistent within itself — `STAGE_ORDER` and `PILLAR_STAGES` remain `CONSTANT_CASE` +and remain the pipeline's only such pair (`packages/core/src/pipeline/builder.ts:12`, `:179`, `:248`, `:269`). +**Trigger:** the next module-level immutable collection added outside `pipeline/`, which would make the fork +visible in a third place and force the choice — or a naming-convention sweep commissioned as its own phase. +Logged in the Deferred Items Log above so it is tracked rather than silent. diff --git a/docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-design.md b/docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-design.md index cbc6622..99d6b47 100644 --- a/docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-design.md +++ b/docs/superpowers/specs/2026-07-28-phase10-deviation-reconciliation-design.md @@ -25,6 +25,21 @@ draft's revision voids: Phase 9 has since shipped its own design and plan (2026- conformance only, and will never produce that evidence. Phase 10 decides those four directly instead (§Group L); nothing in this phase's own scope is left waiting on Phase 9. +> **Corrected 2026-08-30 — the scope above is what was planned, not what shipped.** Phase 10 shipped code, in +> three published packages. Two premises this paragraph rests on were false by the time it executed. The first +> is "docs-only repository state": Phases 1-9 had all shipped by then, so `NFR-12` needed a *build*, not a +> *release*, and it closed on evidence rather than staying open (see the correction on Group N below). The +> second is "consolidation and cross-referencing, not new investigation": the audit was performed against +> as-built source rather than against the phase specs that produced the ledger, and that method found a live +> defect — `Page`, `FetchTransport` and `UndiciTransport` declared `[Symbol.asyncDispose]` as a plain computed +> class member, which on the declared `engines.node ">=20.3"` floor bound the method to the string key +> `"undefined"` (`NFR-10`; the symbol landed in Node 20.4). Fixing it is a breaking type change with two +> changesets, and three later review passes found three more defects behind it. The full inventory, with the +> reasoning for breaking this scope and for holding the line on the project-wide convention sweeps that also +> named this phase, is the roadmap's **Status note (2026-08-30, Phase 10 EXECUTED — scope corrected)**; the +> per-item as-built evidence is `docs/deviations.md`. This paragraph is left standing rather than rewritten so +> the planned-versus-actual gap stays legible. + **Governing documents:** `docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md` (the document this phase rewrites), the roadmap's own Deferred Items Log, and every Phase 2, 3a–8b, and 9 spec's Deviation Ledger section plus the handful of plan-level "Deviation Ledger Additions" sections (5c, 6a, 6b, 6c) @@ -203,6 +218,19 @@ the already-scripted `prepublishOnly` + `npm publish --provenance` path for real open with "first real release" as the unblock trigger, exactly as the roadmap's Deferred Items Log already states — Phase 10 doesn't manufacture a false close here. +> **Corrected 2026-08-30 — `NFR-12`'s half of this group was wrong, and it closed.** The premise "needs a real +> build artifact… neither exists in this docs-only repo state" expired the moment Phases 1-9 shipped code. +> `NFR-12` needed a *build*, not a *publish*, and was verifiable from Phase 1 onward; it sat open two phases +> longer than it had to. It is now closed on evidence and kept closed by a gate: +> `scripts/verify-reproducible-build.mjs` sweeps every `dist/` and `*.tsbuildinfo`, builds twice, and compares a +> SHA-256 per emitted file **and** per `npm pack` tarball across all nine publishable packages — 644 emitted +> files and 9 tarballs byte-identical. It is a blocking CI step and a `ci-preflight` step, and was +> negative-tested by injecting a `Date.now()` into `packages/core/scripts/gen-version.mjs`, the one build-time +> codegen step. **`NFR-16` is unaffected and this group's disposition still holds for it:** its conformance test +> is behavioral and needs a real registry and a real OIDC token. One sub-claim about `NFR-16` was also wrong and +> is corrected in §10 itself — `npm publish --provenance` was never scripted; only `prepublishOnly` is. See +> `docs/deviations.md` §14 and the roadmap's `NFR-12` / `NFR-16` rows. + **Group O — HTTP-18 vs. HTTP-48/50 ETag obs-text replay tension (decision made now).** Not in the original 12; flagged by Phase 1's plan as unresolved and explicitly targeted at Phase 10. `RequestConditions.applyTo` writes entity tags through `Headers`' outbound `set`, which enforces `HTTP-18`'s **MUST**-level restriction (HTAB + diff --git a/package.json b/package.json index f439e49..900b974 100644 --- a/package.json +++ b/package.json @@ -64,7 +64,8 @@ "verify:consumer-types": "node scripts/verify-consumer-types.mjs", "verify:seam-1": "node scripts/verify-seam-1.mjs", "verify:sse-37": "node scripts/verify-sse-37.mjs", - "verify:runtime-floor": "node scripts/verify-runtime-floor.mjs" + "verify:runtime-floor": "node scripts/verify-runtime-floor.mjs", + "verify:reproducible-build": "node scripts/verify-reproducible-build.mjs" } } diff --git a/packages/core/etc/core.api.md b/packages/core/etc/core.api.md index e88a83e..391cc52 100644 --- a/packages/core/etc/core.api.md +++ b/packages/core/etc/core.api.md @@ -726,8 +726,7 @@ export interface OperationDescriptor { } // @public -export class Page implements AsyncDisposable { - [Symbol.asyncDispose](): Promise; +export class Page { constructor(response: Response_2, items: readonly T[]); close(): Promise; readonly headers: Headers_2; diff --git a/packages/core/src/body/response-body-logging.test.ts b/packages/core/src/body/response-body-logging.test.ts index c35a20e..d118273 100644 --- a/packages/core/src/body/response-body-logging.test.ts +++ b/packages/core/src/body/response-body-logging.test.ts @@ -108,7 +108,7 @@ describe('withResponseLogging lifecycle (BODY-27, 28)', () => { expect([...logged.snapshot()]).toEqual([1, 2]); }); - test('teardown is close() only -- no [Symbol.asyncDispose] on the >=18.17 floor', () => { + test('teardown is close() only -- no [Symbol.asyncDispose] on the >=20.3 floor', () => { // See Response's matching assertion: the symbol is undefined on the declared floor, so declaring // it binds the method to the string "undefined". Absence is the assertion. const logged = withResponseLogging(readableOf([1]), 100); diff --git a/packages/core/src/http/response.test.ts b/packages/core/src/http/response.test.ts index 547493e..3e06b76 100644 --- a/packages/core/src/http/response.test.ts +++ b/packages/core/src/http/response.test.ts @@ -230,8 +230,8 @@ describe('close (HTTP-41/BODY-15, HTTP-43)', () => { expect(cancelled).toBe(true); }); - test('teardown is close() only -- no [Symbol.asyncDispose] on the >=18.17 floor', () => { - // The symbol postdates engines.node ">=18.17", where the computed key evaluates to `undefined` + test('teardown is close() only -- no [Symbol.asyncDispose] on the >=20.3 floor', () => { + // The symbol postdates engines.node ">=20.3" (it arrived in Node 20.4), where the computed key evaluates to `undefined` // and binds the method to the string "undefined" instead. Asserting its ABSENCE is what keeps it // from being reintroduced ahead of the floor bump that would make it real on every resource owner. const response = baseResponse(readableOf('x')); diff --git a/packages/core/src/pagination/page.test.ts b/packages/core/src/pagination/page.test.ts index 3a3b230..9bf8487 100644 --- a/packages/core/src/pagination/page.test.ts +++ b/packages/core/src/pagination/page.test.ts @@ -85,18 +85,43 @@ test('an empty items list with a next request is a valid non-terminal page (PAGE expect(info.nextRequest).toBe(next); }); -test('await using releases the page via Symbol.asyncDispose (PAGE-3, PAGE-12)', async () => { +test('the disposal member releases the page exactly once where the runtime has it (PAGE-3, PAGE-12)', async () => { const {response, closes} = fakeResponse(); const page = makePage(response, [1]); + // Read through a cast rather than `Symbol.asyncDispose` directly: on the pinned floor (Node 20.3, + // which predates the symbol's 20.4 arrival) it is `undefined`, and a bare index would silently read + // the string key `"undefined"` instead. Same shape as `sse/stream.test.ts`. + const asyncDispose = (Symbol as {asyncDispose?: symbol}).asyncDispose; - { - await using scoped = page; - expect(scoped.items).toEqual([1]); - expect(closes()).toBe(0); + if (typeof asyncDispose !== 'symbol') { + // The guarded install is correctly a no-op here; close() is the whole teardown surface. + await page.close(); + expect(closes()).toBe(1); + return; } + + const dispose = ( + page as unknown as Record Promise) | undefined> + )[asyncDispose]; + expect(dispose).toBeDefined(); + expect(page.items).toEqual([1]); + expect(closes()).toBe(0); + + await dispose?.call(page); expect(closes()).toBe(1); // Dispose delegates to close, so it inherits Response.close()'s idempotence rather than adding a second guard. await page.close(); expect(closes()).toBe(1); }); + +test('no "undefined" prototype key survives the guarded install (PAGE-12)', () => { + // The regression this pins: an unguarded `async [Symbol.asyncDispose]()` class member binds to the + // string key "undefined" on the >=20.3 floor, leaving junk on the prototype and no working disposal. + // `http/response.test.ts` carries the same assertion for `Response`. + const {response} = fakeResponse(); + const page = makePage(response, [1]); + expect(Object.getOwnPropertyNames(Object.getPrototypeOf(page))).not.toContain( + 'undefined', + ); +}); diff --git a/packages/core/src/pagination/page.ts b/packages/core/src/pagination/page.ts index 578beb5..9553812 100644 --- a/packages/core/src/pagination/page.ts +++ b/packages/core/src/pagination/page.ts @@ -51,7 +51,7 @@ export function pageInfo( * * @public */ -export class Page implements AsyncDisposable { +export class Page { /** Materialized, frozen items that remain readable after close (PAGE-2). */ readonly items: readonly T[]; /** The HTTP response status code and reason phrase (PAGE-1). */ @@ -93,11 +93,29 @@ export class Page implements AsyncDisposable { async close(): Promise { await this.#response.close(); } +} - /** - * Scoped teardown for `await using`, delegating to {@link Page.close} (PAGE-12). - */ - async [Symbol.asyncDispose](): Promise { - await this.close(); - } +// PAGE-12's scoped teardown, installed at run time only when the symbol exists — the same guarded +// shape `SseStream` uses. `Response` ships no disposal member at all (HTTP-38), and +// `http/response.test.ts` pins the absence of the junk key this guard exists to prevent. +// +// Because the install is conditional, this class deliberately does NOT declare `implements +// AsyncDisposable`: `await using page` therefore does not type-check on the declared floor, where the +// method is genuinely absent. `close()` is the supported teardown path — see `Paginator.pages()`, +// which tells consumers which scoped constructs actually give PAGE-12's guarantee. +// +// DO NOT restore this as a plain `async [Symbol.asyncDispose]()` class member. Node 20.3 is this +// package's declared floor (`engines.node`, checked by verify:runtime-floor) and predates the symbol, +// which arrived in 20.4. On the floor the computed key evaluates to `undefined` and binds the method +// to the string key `"undefined"` — a junk prototype entry, and no working disposal. Declaring it on +// the class would also emit it into the `.d.ts` unconditionally, promising consumers on the floor a +// method that is not there. +if (typeof Symbol.asyncDispose === 'symbol') { + Object.defineProperty(Page.prototype, Symbol.asyncDispose, { + value: function asyncDispose(this: Page): Promise { + return this.close(); + }, + writable: true, + configurable: true, + }); } diff --git a/packages/core/src/pagination/paginator.ts b/packages/core/src/pagination/paginator.ts index 6d0396b..e331316 100644 --- a/packages/core/src/pagination/paginator.ts +++ b/packages/core/src/pagination/paginator.ts @@ -122,8 +122,14 @@ export class Paginator { * `.return()` on every exit path, including `break` and `throw`, so the held page is always released. Driving * the iterator by hand is the case to be careful with: if you call `[Symbol.asyncIterator]()` yourself and * then abandon it without calling `.return()`, the generator never resumes, its `finally` never runs, and the - * page it is holding stays open until the process exits. Either stay in a `for await`, or bind the pages you - * pull with `await using` (see {@link (Page:class)}), or call `.return()` on the iterator yourself. + * page it is holding stays open until the process exits. Two constructs give you the guarantee: stay inside a + * `for await`, or, when you drive the iterator yourself, call `.return()` on it from a `finally`. + * + * `await using` is deliberately **not** a third. {@link (Page:class)} installs `[Symbol.asyncDispose]` at run + * time only where the runtime has it, so it does not declare `AsyncDisposable` and `await using page` does not + * type-check against this package's `engines.node >=20.3` floor — the symbol arrived in Node 20.4. Every page + * this view yields is closed for you as the walk advances; `Page.close()` is the manual counterpart, and is + * idempotent. * * Single-use (PAGE-14) — **per view, not per paginator**. A second `[Symbol.asyncIterator]()` on *this* * returned view fails loudly rather than silently restarting the walk. Calling `pages()` again is the diff --git a/packages/core/src/sse/stream.ts b/packages/core/src/sse/stream.ts index e7d59ab..8f10e4a 100644 --- a/packages/core/src/sse/stream.ts +++ b/packages/core/src/sse/stream.ts @@ -193,11 +193,18 @@ export class SseStream implements AsyncIterable { } } -// Guarded install: installed at run time only when the symbol exists, matching Response (HTTP-38). -// Node 20.3 (the pinned floor verified by verify:runtime-floor) predates Symbol.asyncDispose (which -// landed in Node 20.4). TypeScript does not polyfill the well-known symbol for a library that declares -// the method, so declaring it on the interface would break consumers compiling on ES2023 without -// esnext.disposable. +// Scoped teardown for `await using` (styleguide 13.1/13.2), installed at run time only when the symbol +// exists. This is the original of the shape `Page` and both transport adapters now repeat. +// +// DO NOT restore this as a plain `[Symbol.asyncDispose]()` class member. Node 20.3 is this package's +// declared floor (`engines.node`, checked by verify:runtime-floor) and predates the symbol, which +// arrived in 20.4. On the floor the computed key evaluates to `undefined` and binds the method to the +// string key `"undefined"` — a junk prototype entry, and no working disposal. TypeScript does not +// polyfill the well-known symbol either, so declaring it on the class would emit it into the `.d.ts` +// unconditionally and break consumers compiling on ES2023 without esnext.disposable. +// +// `Response` (HTTP-38) goes one step further and ships no disposal member at all — `close()` is its +// whole teardown surface, and `http/response.test.ts` pins the junk key's absence there. if (typeof Symbol.asyncDispose === 'symbol') { Object.defineProperty(SseStream.prototype, Symbol.asyncDispose, { value: function asyncDispose(this: SseStream): Promise { diff --git a/packages/shrink-test/shrink-test.config.ts b/packages/shrink-test/shrink-test.config.ts index 7fbd58a..be3b72b 100644 --- a/packages/shrink-test/shrink-test.config.ts +++ b/packages/shrink-test/shrink-test.config.ts @@ -19,11 +19,13 @@ export interface ShrinkTestConfig { } /** - * Measured at 16,671 bytes on 2026-08-29 (esbuild 0.28.2; `@dexpace/core` + `@dexpace/transport-fetch` - * + `@dexpace/codec-json`, all three reached through their published entry points). The budget is - * 24 KiB -- ~47% headroom, which absorbs ordinary growth while still catching the failure this guard - * exists for: a tree-shaking regression pulls in core's barrel wholesale and shows up as a multiple - * of this figure, not a few percent over it. A loose budget would catch nothing. + * Measured at 16,671 bytes on 2026-08-29, and 17,689 bytes on 2026-08-30 once the fixture also + * constructed a `Page` to probe the disposal-symbol install (esbuild 0.28.2; `@dexpace/core` + + * `@dexpace/transport-fetch` + `@dexpace/codec-json`, all three reached through their published entry + * points). The budget is 24 KiB -- ~39% headroom, which absorbs ordinary growth while still catching + * the failure this guard exists for: a tree-shaking regression pulls in core's barrel wholesale and + * shows up as a multiple of this figure, not a few percent over it. A loose budget would catch + * nothing. */ export const SHRINK_TEST_CONFIG: ShrinkTestConfig = Object.freeze({ budgetBytes: 24_576, diff --git a/packages/shrink-test/src/fixture-app.ts b/packages/shrink-test/src/fixture-app.ts index e59174f..1f0b21e 100644 --- a/packages/shrink-test/src/fixture-app.ts +++ b/packages/shrink-test/src/fixture-app.ts @@ -1,15 +1,29 @@ // SPDX-License-Identifier: MIT // packages/shrink-test/src/fixture-app.ts import {jsonSerde} from '@dexpace/codec-json'; -import {IoError, Request, type Schema} from '@dexpace/core'; +import { + IoError, + Page, + Request, + type Response, + type Schema, + type Transport, +} from '@dexpace/core'; import {fetchTransport} from '@dexpace/transport-fetch'; -/** What {@link runFixtureApp} reports back to the guard, over stdout, from the child process. */ +/** + * What {@link runFixtureApp} reports back to the guard from the child process. + * + * Every field is a boolean the guard requires to be `true`; the generated runner fails on any `false` + * one without needing to be edited, so a new probe only has to be added here. + */ export interface FixtureResult { /** True when the error thrown by `transport-fetch` matched `IoError` imported from `core`. */ readonly caughtViaCoreImport: boolean; /** True when a serialize/deserialize round trip through `codec-json` returned the input. */ readonly serdeRoundTripOk: boolean; + /** True when the module-scope `[Symbol.asyncDispose]` installs survived the tree-shaking pass. */ + readonly disposalSymbolSurvived: boolean; } /** The one shape the round trip carries; a hand-written `Schema` keeps the fixture codec-agnostic. */ @@ -35,23 +49,74 @@ const shrinkProbeSchema: Schema = { }, }; +/** + * The narrowest stand-in for the three fields `Page`'s constructor reads. + * + * A real `Response` needs a live transport exchange to produce, and {@link probeDisposalSymbol} only + * needs an instance whose prototype came out of the bundle -- the response is never read again. + */ +function stubResponse(): Response { + return { + status: {code: 204}, + headers: {get: (): undefined => undefined}, + request: {method: 'GET'}, + close: (): Promise => Promise.resolve(), + } as unknown as Response; +} + +/** True when `value` carries a callable `[Symbol.asyncDispose]`, however it was installed. */ +function hasAsyncDispose(value: object, disposeSymbol: symbol): boolean { + return ( + typeof (value as Record)[disposeSymbol] === 'function' + ); +} + +/** + * Proves that the module-scope `[Symbol.asyncDispose]` installs survive a bundle round trip. + * + * `Page` and `FetchTransport` (and `SseStream`, and `UndiciTransport`) do not declare disposal as a + * class member: Node 20.3 is the workspace floor and predates the symbol, so declaring it would emit + * a `.d.ts` promise the floor cannot keep (NFR-10). The method is instead installed by a guarded + * `Object.defineProperty` **statement that runs when the module is evaluated** -- a module-level side + * effect, in packages that all declare `"sideEffects": false`. + * + * That manifest field entitles a bundler to drop a module whose exports go unused, and nothing stops + * a future one from also dropping a top-level statement it judges inert. Here the classes *are* used, + * so the modules are kept and the install runs; this asserts that outcome rather than assuming it, + * inside the same real `bundle + minify + treeShaking` pass the rest of the guard uses. + * + * Read through a cast rather than a bare `Symbol.asyncDispose` index, matching the guarded install: + * on the declared floor the symbol is `undefined` and the index would read the string key + * `"undefined"`. Absent symbol means the install is *supposed* to leave nothing behind, so the probe + * is vacuously true there. + */ +function probeDisposalSymbol(transport: Transport): boolean { + const disposeSymbol = (Symbol as {asyncDispose?: symbol}).asyncDispose; + if (typeof disposeSymbol !== 'symbol') return true; + return ( + hasAsyncDispose(new Page(stubResponse(), []), disposeSymbol) && + hasAsyncDispose(transport, disposeSymbol) + ); +} + /** * Runs inside the bundled, tree-shaken artifact -- never against `src/` directly, which is the whole * point (see `run-shrink-guard.ts`). * - * Proves the two properties a bundler round trip can silently break. First, cross-package + * Proves the three properties a bundler round trip can silently break. First, cross-package * `instanceof`: `TransportFailureError` is thrown by `@dexpace/transport-fetch` and its base class * `IoError` is imported here from `@dexpace/core`, so the check passes only if the bundle contains * exactly ONE copy of core's class identity. Two copies -- the dual-package hazard * `docs/knowledge/tooling-and-quality-gates.md` names, and the risk this port substitutes for the * reference's reflective keep-rules (`NFR-8`, deviation-ledger item 10) -- make it silently false * while every type still checks. Second, that a real serde round trip still works once the codec has - * been through the same minifier. + * been through the same minifier. Third, that the module-scope disposal installs are still there -- + * see {@link probeDisposalSymbol}. * * Port 1 is chosen because nothing listens there: the connection is refused immediately, so the * guard needs no fixture server and cannot hang on a slow socket. * - * @returns both checks, for the parent process to assert on. + * @returns every check, for the parent process to assert on. */ export async function runFixtureApp(): Promise { const transport = fetchTransport(); @@ -70,5 +135,9 @@ export async function runFixtureApp(): Promise { const bytes = serde.serializer.serialize({shrinkTest: true}); const decoded = serde.deserializer.deserialize(bytes, shrinkProbeSchema); - return {caughtViaCoreImport, serdeRoundTripOk: decoded.shrinkTest}; + return { + caughtViaCoreImport, + serdeRoundTripOk: decoded.shrinkTest, + disposalSymbolSurvived: probeDisposalSymbol(transport), + }; } diff --git a/packages/shrink-test/src/run-shrink-guard.test.ts b/packages/shrink-test/src/run-shrink-guard.test.ts index e887bd6..fc1a6fc 100644 --- a/packages/shrink-test/src/run-shrink-guard.test.ts +++ b/packages/shrink-test/src/run-shrink-guard.test.ts @@ -1,9 +1,13 @@ // SPDX-License-Identifier: MIT // packages/shrink-test/src/run-shrink-guard.test.ts // Exercises: NFR-9 (shrink-and-run regression guard, wired into the default build via the root -// `shrink-test` script), NFR-17 (that gate is blocking, not advisory). +// `shrink-test` script), NFR-17 (that gate is blocking, not advisory), PAGE-12 and NFR-10 (the +// guarded, module-scope `[Symbol.asyncDispose]` installs that keep the emitted artifact on the +// declared Node floor are still present and callable after tree-shaking -- see +// `fixture-app.ts`'s `probeDisposalSymbol`). // Substitutes for NFR-8's keep-configuration, which this port ships nothing for by design -- see the -// Phase 9 deviation ledger and docs/knowledge/deliberate-deviations.md:32. +// Phase 9 deviation ledger and docs/knowledge/deliberate-deviations.md:55 (that corpus file is flagged +// stale as of 2026-08-30; docs/deviations.md section 10 is the current statement). import {describe, expect, test} from 'bun:test'; import {runShrinkGuard} from './run-shrink-guard.js'; diff --git a/packages/shrink-test/src/run-shrink-guard.ts b/packages/shrink-test/src/run-shrink-guard.ts index b8bacee..49d66aa 100644 --- a/packages/shrink-test/src/run-shrink-guard.ts +++ b/packages/shrink-test/src/run-shrink-guard.ts @@ -40,8 +40,10 @@ function runInChild(runnerPath: string): Promise { * emitted, which is what a downstream consumer ships. * * The child is spawned with `stdio: 'ignore'` and reports through its exit code alone; the runner it - * executes exits non-zero when either fixture check comes back false, so a stripped `instanceof` - * surfaces as a failed guard rather than as parsed output this function would have to trust. + * executes exits non-zero when any fixture check comes back false, so a stripped `instanceof` or a + * dropped `[Symbol.asyncDispose]` install surfaces as a failed guard rather than as parsed output + * this function would have to trust. Which check failed is not carried back — read `fixture-app.ts`, + * whose `FixtureResult` names them all. * * @returns the measured size, the configured budget, and whether the artifact still worked. The * caller decides what fails the build -- see `run-shrink-guard.test.ts`. @@ -58,7 +60,10 @@ export async function runShrinkGuard(): Promise { [ `import {runFixtureApp} from ${JSON.stringify(entryPath)};`, 'const result = await runFixtureApp();', - 'process.exit(result.caughtViaCoreImport && result.serdeRoundTripOk ? 0 : 1);', + // Every FixtureResult field is a check that must come back true, so this stays correct when + // the fixture grows a new probe -- there is no list here to forget to update. + 'const failed = Object.values(result).filter(ok => ok !== true);', + 'process.exit(failed.length === 0 ? 0 : 1);', '', ].join('\n'), 'utf8', diff --git a/packages/transport-fetch/etc/transport-fetch.api.md b/packages/transport-fetch/etc/transport-fetch.api.md index 072dfe8..61fd06d 100644 --- a/packages/transport-fetch/etc/transport-fetch.api.md +++ b/packages/transport-fetch/etc/transport-fetch.api.md @@ -13,7 +13,7 @@ export type FetchLike = (input: string, init: RequestInit & { }) => Promise; // @public -export function fetchTransport(options?: FetchTransportOptions): Transport & AsyncDisposable; +export function fetchTransport(options?: FetchTransportOptions): Transport; // @public export interface FetchTransportOptions { diff --git a/packages/transport-fetch/src/fetch-transport.test.ts b/packages/transport-fetch/src/fetch-transport.test.ts index c4b3796..8eea329 100644 --- a/packages/transport-fetch/src/fetch-transport.test.ts +++ b/packages/transport-fetch/src/fetch-transport.test.ts @@ -6,7 +6,8 @@ // TRANSPORT-2 (no retrying/redirecting dispatcher is ever composed), TRANSPORT-15/16 // (close is a documented no-op), TRANSPORT-17/19 (single-use body written once, abandoned producer // unblocked), TRANSPORT-22 (an adaptation throw still closes the native response), TRANSPORT-30 -// (no proxy option exists at all) +// (no proxy option exists at all), SEAM-30 (no producer is left running for its rejection to reach +// Node's default unhandledRejection policy) import {describe, expect, test} from 'bun:test'; import { byteArrayBody, @@ -192,6 +193,39 @@ describe('fetchTransport request-body failures', () => { expect(recorder.calls.length).toBe(0); }); + test('a header-mapping throw never strands a started body producer (TRANSPORT-19, SEAM-30)', async () => { + // This transport builds its DispatchPlan as one object literal, so the safety here rests on + // property EVALUATION ORDER: `headers` must be computed before `prepared`. `prepareBody` starts + // a streaming producer eagerly and `toNativeHeaders` reads `request.body.mediaType`, a + // caller-supplied getter that may throw -- reversing the two would leave a live producer nobody + // can abandon, whose later rejection reaches Node's default unhandledRejection policy. The + // undici twin had exactly that ordering bug; this row keeps it from appearing here. + let producerStarted = false; + const recorder = recordingFetch(); + const transport = fetchTransport({fetch: recorder.fetch}); + const request = Request.newBuilder() + .method('POST') + .url('http://127.0.0.1:1/x') + .body({ + kind: 'stream', + get mediaType(): string | undefined { + throw new Error('mediaType getter exploded'); + }, + // -1 / non-replayable forces the streaming branch rather than the buffered one. + contentLength: -1, + replayable: false, + writeTo: () => { + producerStarted = true; + return Promise.resolve(); + }, + }) + .build(); + + await rejection(transport.send(request)); + expect(producerStarted).toBe(false); + expect(recorder.calls.length).toBe(0); + }); + test('a network failure is wrapped as TransportFailureError with its cause kept', async () => { const cause = new Error('connect ECONNREFUSED'); const transport = fetchTransport({fetch: () => Promise.reject(cause)}); @@ -215,9 +249,25 @@ describe('fetchTransport lifecycle', () => { expect(recorder.calls.length).toBe(1); }); - test('asyncDispose is the same teardown as close', async () => { + test('asyncDispose is the same teardown as close, where the runtime has it', async () => { const transport = fetchTransport(); - await transport[Symbol.asyncDispose](); + // Cast rather than a bare `Symbol.asyncDispose` index: on the pinned floor (Node 20.3, which + // predates the symbol's 20.4 arrival) it is `undefined` and the index would read the string key + // `"undefined"`. The install in fetch-transport.ts is guarded to match. + const asyncDispose = (Symbol as {asyncDispose?: symbol}).asyncDispose; + if (typeof asyncDispose === 'symbol') { + const dispose = ( + transport as unknown as Record< + symbol, + (() => Promise) | undefined + > + )[asyncDispose]; + expect(dispose).toBeDefined(); + await dispose?.call(transport); + } + expect( + Object.getOwnPropertyNames(Object.getPrototypeOf(transport)), + ).not.toContain('undefined'); await transport.close(); }); diff --git a/packages/transport-fetch/src/fetch-transport.ts b/packages/transport-fetch/src/fetch-transport.ts index 1da8a72..7900f88 100644 --- a/packages/transport-fetch/src/fetch-transport.ts +++ b/packages/transport-fetch/src/fetch-transport.ts @@ -192,7 +192,7 @@ interface DispatchPlan { readonly fork: ForkedSignal; } -class FetchTransport implements Transport, AsyncDisposable { +class FetchTransport implements Transport { readonly #logDrops: (dropped: readonly string[]) => void; readonly #fetch: FetchLike; readonly #defaultTimeoutMs: number | undefined; @@ -299,15 +299,25 @@ class FetchTransport implements Transport, AsyncDisposable { close(): Promise { return Promise.resolve(); } +} - /** - * Single teardown path, delegating to {@link FetchTransport.close}. - * - * @returns a promise that resolves once teardown is complete. - */ - [Symbol.asyncDispose](): Promise { - return this.close(); - } +// Single teardown path for `await using`, delegating to `FetchTransport.close()` and installed at run +// time only when the symbol exists — the same guarded shape `SseStream` and `Page` use. +// +// DO NOT restore this as a plain `[Symbol.asyncDispose]()` class member. Node 20.3 is this package's +// declared floor (`engines.node`, checked by verify:runtime-floor) and predates the symbol, which +// arrived in 20.4. On the floor the computed key evaluates to `undefined` and binds the method to the +// string key `"undefined"` — a junk prototype entry, and no working disposal. Declaring it on the +// class would also emit it into the `.d.ts` unconditionally, promising consumers on the floor a method +// that is not there. +if (typeof Symbol.asyncDispose === 'symbol') { + Object.defineProperty(FetchTransport.prototype, Symbol.asyncDispose, { + value: function asyncDispose(this: FetchTransport): Promise { + return this.close(); + }, + writable: true, + configurable: true, + }); } /** @@ -316,17 +326,18 @@ class FetchTransport implements Transport, AsyncDisposable { * `close()` is a sanctioned no-op and `send()` keeps working after it (SEAM-15). There is no proxy * support at all; see {@link FetchTransportOptions}. * - * The returned transport is `AsyncDisposable`, so `await using transport = fetchTransport(...)` - * releases it at scope exit — the single teardown path `docs/knowledge/resource-management.md` asks - * for. + * `close()` is the single teardown path `docs/knowledge/resource-management.md` asks for. A + * `[Symbol.asyncDispose]` delegating to it is installed at run time **when the runtime has the + * symbol**, which this package's declared floor (`engines.node >=20.3`) does not — it arrived in Node + * 20.4. The return type therefore does not promise `AsyncDisposable`: claiming it would type-check + * `await using` for a consumer sitting on the floor, where the method is genuinely absent. Call + * `close()`, or raise your own floor to 20.4+ and reach the symbol through a cast. * * @param options - optional transport settings. - * @returns a transport ready to send, disposable through `await using`. + * @returns a transport ready to send; release it with `close()`. * * @public */ -export function fetchTransport( - options: FetchTransportOptions = {}, -): Transport & AsyncDisposable { +export function fetchTransport(options: FetchTransportOptions = {}): Transport { return new FetchTransport(options); } diff --git a/packages/transport-undici/etc/transport-undici.api.md b/packages/transport-undici/etc/transport-undici.api.md index 752c8eb..1b39350 100644 --- a/packages/transport-undici/etc/transport-undici.api.md +++ b/packages/transport-undici/etc/transport-undici.api.md @@ -11,7 +11,7 @@ import { ProxyOptions } from '@dexpace/core'; import { Transport } from '@dexpace/core'; // @public -export function undiciTransport(options?: UndiciTransportOptions): Transport & AsyncDisposable; +export function undiciTransport(options?: UndiciTransportOptions): Transport; // @public export interface UndiciTransportOptions { diff --git a/packages/transport-undici/src/undici-transport.test.ts b/packages/transport-undici/src/undici-transport.test.ts index 053deda..862a718 100644 --- a/packages/transport-undici/src/undici-transport.test.ts +++ b/packages/transport-undici/src/undici-transport.test.ts @@ -7,7 +7,9 @@ // throws nor blocks), // TRANSPORT-15/16 (ownership-aware, idempotent close), TRANSPORT-22 (an adaptation throw destroys the // native body), TRANSPORT-20 (a permanent argument error is terminal, a no-response failure is -// retryable), TRANSPORT-28 (a file body dispatches its declared byte range), SEAM-14 +// retryable), TRANSPORT-28 (a file body dispatches its declared byte range), SEAM-14, +// TRANSPORT-19 (a header-mapping throw leaves no started body producer stranded), SEAM-30 (so no +// producer rejection reaches Node's default unhandledRejection policy) import {createRequire} from 'node:module'; import {mkdtemp, rm, writeFile} from 'node:fs/promises'; import {createServer, type Server} from 'node:http'; @@ -23,10 +25,11 @@ import { IoError, setGlobalLogger, TransportFailureError, + type Body, type FileBodyDescriptor, type Logger, } from '@dexpace/core'; -import type {Dispatcher} from 'undici'; +import type {Agent, Dispatcher, ProxyAgent} from 'undici'; import {undiciTransport} from './undici-transport.js'; const require = createRequire(import.meta.url); @@ -79,6 +82,120 @@ function captureDroppedHeaders(): { }; } +/** + * DispatcherBase's public `destroyed` getter, which undici's shipped `Dispatcher` and `ProxyAgent` + * types omit even though every concrete dispatcher exposes it. + */ +interface DestroyableDispatcher { + readonly destroyed: boolean; +} + +/** + * A streaming body whose `mediaType` getter throws, recording whether its producer was ever started. + * The shape header mapping trips over: `mediaType` is read during mapping, while `writeTo` only runs + * once `pumpBody` has taken ownership. + */ +function bodyWithThrowingMediaType(): { + body: Body; + producerStarted: () => boolean; +} { + let started = false; + return { + body: { + kind: 'stream', + get mediaType(): string | undefined { + throw new Error('mediaType getter exploded'); + }, + // -1 / non-replayable forces the streaming branch rather than the buffered one. + contentLength: -1, + replayable: false, + writeTo: () => { + started = true; + return Promise.resolve(); + }, + }, + producerStarted: () => started, + }; +} + +/** + * Swaps undici's exported `Agent` binding for a capturing subclass, so the dispatcher a transport + * constructs for *itself* is reachable from the test. `destroyed` is DispatcherBase's own public getter, + * so teardown stays observable without patching `destroy` at all. + */ +function captureOwnedAgents(): { + agents: DestroyableDispatcher[]; + restore: () => void; +} { + const bindings = undici as unknown as Record; + const RealAgent = undici.Agent; + const agents: DestroyableDispatcher[] = []; + + class CapturingAgent extends RealAgent { + constructor(opts?: Agent.Options) { + super(opts); + // No cast needed here: undici's shipped `Agent` type declares `destroyed`, unlike its bare + // `Dispatcher` and `ProxyAgent` types. + agents.push(this); + } + } + + bindings.Agent = CapturingAgent; + return { + agents, + restore: () => { + bindings.Agent = RealAgent; + }, + }; +} + +/** + * Swaps undici's exported `Agent` / `ProxyAgent` bindings so a transport built afterwards gets a + * direct `Agent` whose `destroy()` rejects, and captures the `ProxyAgent` constructed alongside it. + * + * The exported CLASS BINDINGS are swapped, not `DispatcherBase.prototype.destroy`: the transport + * reads `undici.Agent` / `undici.ProxyAgent` off this exports object at construction time, while + * ProxyAgent's own internal Agent comes from its private `require('./agent')`. Patching the shared + * prototype instead makes the injected failure fire inside ProxyAgent's internals too, which is a + * different bug than the one under test. + * + * The ProxyAgent is captured rather than intercepted: overriding its `destroy` would also catch + * DispatcherBase's internal `this.destroy(err, callback)` re-dispatch and recurse. `destroyed` is + * DispatcherBase's own public getter, so the effect is observable without touching teardown at all. + */ +function explodeDirectAgentDestroy(): { + proxyAgents: DestroyableDispatcher[]; + restore: () => void; +} { + const bindings = undici as unknown as Record; + const RealAgent = undici.Agent; + const RealProxyAgent = undici.ProxyAgent; + const proxyAgents: DestroyableDispatcher[] = []; + + class ExplodingAgent extends RealAgent { + override destroy(): Promise { + return Promise.reject(new Error('agent destroy exploded')); + } + } + class CapturingProxyAgent extends RealProxyAgent { + constructor(opts: ProxyAgent.Options) { + super(opts); + // Cast because undici's shipped ProxyAgent type omits DispatcherBase's `destroyed` getter. + proxyAgents.push(this as unknown as DestroyableDispatcher); + } + } + + bindings.Agent = ExplodingAgent; + bindings.ProxyAgent = CapturingProxyAgent; + return { + proxyAgents, + restore: () => { + bindings.Agent = RealAgent; + bindings.ProxyAgent = RealProxyAgent; + }, + }; +} + /** Records every request body the server received, so a file body's byte range is checkable. */ let server: Server; let origin: string; @@ -148,6 +265,40 @@ describe('undiciTransport construction and ownership', () => { await transport.close(); }); + test('a failing dispatcher destroy still releases every other owned dispatcher (TRANSPORT-15/16)', async () => { + // owned is [ProxyAgent, Agent] and close() walks it reversed, so the direct Agent is destroyed + // first. When that destroy rejects, a naive `for … await` loop propagates immediately and the + // ProxyAgent -- the dispatcher actually holding the pooled proxy connections -- is never + // released. + const {proxyAgents, restore} = explodeDirectAgentDestroy(); + try { + const transport = undiciTransport({ + proxy: createProxyOptions({ + type: 'http', + host: '127.0.0.1', + port: 3128, + }), + }); + expect(proxyAgents.length).toBe(1); + // The failure is reported -- teardown must not swallow it (TRANSPORT-16) -- and it is reported + // with the underlying cause intact rather than flattened to a message. + const error = await rejection(transport.close()); + expect(error).toBeInstanceOf(TransportFailureError); + expect(error).toMatchObject({ + cause: {message: 'agent destroy exploded'}, + }); + // Idempotent even on the failure path: the rejection is memoized, so a second close reports the + // same failure rather than falsely claiming a clean teardown (TRANSPORT-16, XCUT-13). + expect(await rejection(transport.close())).toBe(error); + // ... but every other owned dispatcher is released regardless. This is the leak: with the + // naive `for … await` loop the direct Agent's rejection aborts the walk and this stays false. + expect(proxyAgents[0]?.destroyed).toBe(true); + } finally { + // Restoration must never be skipped, so this block stays assertion-free. + restore(); + } + }); + test('supplying both a dispatcher and a proxy fails loudly at construction', () => { const agent = new undici.Agent(); expect(() => @@ -160,6 +311,43 @@ describe('undiciTransport construction and ownership', () => { }); }); +describe('undiciTransport disposal (TRANSPORT-15/16)', () => { + test('asyncDispose is the same teardown as close, where the runtime has it', async () => { + // The owned Agent is captured so "same teardown as close" is an assertion about the dispatcher + // this transport constructed, not merely about the member existing: an asyncDispose wired to + // anything other than close() -- or to a bare resolved promise -- leaves `destroyed` false below. + const {agents, restore} = captureOwnedAgents(); + try { + const transport = undiciTransport(); + expect(agents.length).toBe(1); + // Cast rather than a bare `Symbol.asyncDispose` index: on the pinned floor (Node 20.3, which + // predates the symbol's 20.4 arrival) it is `undefined` and the index would read the string key + // `"undefined"`. The install in undici-transport.ts is guarded to match. + const asyncDispose = (Symbol as {asyncDispose?: symbol}).asyncDispose; + if (typeof asyncDispose === 'symbol') { + const dispose = ( + transport as unknown as Record< + symbol, + (() => Promise) | undefined + > + )[asyncDispose]; + expect(dispose).toBeDefined(); + await dispose?.call(transport); + expect(agents[0]?.destroyed).toBe(true); + } + // Both legs: an unguarded `[Symbol.asyncDispose]()` class member would leave this junk key on + // the prototype on the >=20.3 floor, with no working disposal behind it. + expect( + Object.getOwnPropertyNames(Object.getPrototypeOf(transport)), + ).not.toContain('undefined'); + await transport.close(); + } finally { + // Restoration must never be skipped, so this block stays assertion-free. + restore(); + } + }); +}); + describe('undiciTransport dispatch', () => { test('TRANSPORT-2/11: redirects are pinned off and Connection is forwarded, not dropped', async () => { const dispatched: Dispatcher.RequestOptions[] = []; @@ -293,6 +481,25 @@ describe('undiciTransport request-body failures', () => { await transport.close(); }); + test('a header-mapping throw never strands a started body producer (TRANSPORT-19, SEAM-30)', async () => { + // `pumpBody` starts the producer EAGERLY, and header mapping reads `request.body.mediaType` -- + // a getter on a caller-supplied Body, which may throw. If the producer is started first, that + // throw escapes before anything can abandon it and the producer's own later rejection reaches + // Node's default unhandledRejection policy. Mapping headers first closes the window, which is + // the order the fetch twin already evaluates them in. + const {body, producerStarted} = bodyWithThrowingMediaType(); + const transport = undiciTransport(); + const request = Request.newBuilder() + .method('POST') + .url(`${origin}/upload`) + .body(body) + .build(); + + await rejection(transport.send(request)); + expect(producerStarted()).toBe(false); + await transport.close(); + }); + test('TRANSPORT-22: an adaptation throw destroys the native body before propagating', async () => { let destroyed = false; const hostile = { @@ -420,12 +627,6 @@ describe('undiciTransport proxy dispatch (TRANSPORT-30)', () => { await response.close(); await transport.close(); }); - - test('asyncDispose is the same teardown as close', async () => { - const transport = undiciTransport(); - await transport[Symbol.asyncDispose](); - await transport.close(); - }); }); describe('undiciTransport cancellation (TRANSPORT-8)', () => { diff --git a/packages/transport-undici/src/undici-transport.ts b/packages/transport-undici/src/undici-transport.ts index 9d0a815..d5cdfd8 100644 --- a/packages/transport-undici/src/undici-transport.ts +++ b/packages/transport-undici/src/undici-transport.ts @@ -359,7 +359,26 @@ interface DispatchContext { readonly fork: ForkedSignal; } -class UndiciTransport implements Transport, AsyncDisposable { +/** + * Destroys every dispatcher in reverse acquisition order and returns whatever failed, rather than + * stopping at the first rejection. Teardown is best-effort by definition: a dispatcher that cannot be + * released is not a reason to leak the ones behind it (TRANSPORT-15/16). + */ +async function releaseAll( + dispatchers: readonly Dispatcher[], +): Promise { + const failures: unknown[] = []; + for (const dispatcher of [...dispatchers].reverse()) { + try { + await dispatcher.destroy(); + } catch (error) { + failures.push(error); + } + } + return failures; +} + +class UndiciTransport implements Transport { readonly #dispatchers: DispatcherSet; readonly #proxy: ProxyOptions | undefined; readonly #logDrops: (dropped: readonly string[]) => void; @@ -396,11 +415,21 @@ class UndiciTransport implements Transport, AsyncDisposable { ); if (composed?.aborted) throw abortToSdkError(composed, composed.reason); + // Headers BEFORE the body, deliberately -- the fetch twin evaluates them in this order too. + // `prepareBody` starts a streaming producer eagerly, while `toUndiciHeaders` reads + // `request.body.mediaType`, a getter on a caller-supplied Body that may throw. Preparing the + // body first leaves such a throw with a live producer nobody can abandon, whose own later + // rejection then reaches Node's default unhandledRejection policy (TRANSPORT-19, SEAM-30). + const headers = toUndiciHeaders( + request, + this.#forbiddenHeaders, + this.#logDrops, + ); const prepared = await prepareBody(request.body); // Dispatched with a fork the caller cannot reach: cancellation stays live for the whole in-flight // window and goes inert the moment the response is handed over (SEAM-16). const context: DispatchContext = { - headers: toUndiciHeaders(request, this.#forbiddenHeaders, this.#logDrops), + headers, body: prepared.init, fork: forkSignal(composed), }; @@ -491,43 +520,78 @@ class UndiciTransport implements Transport, AsyncDisposable { * SEAM-15 post-close mode: a send issued after `close()` cannot succeed over a dispatcher that no * longer exists, so it is not reported as a retryable failure. * + * A dispatcher that fails to release does not strand the rest: every owned dispatcher is destroyed + * before the failure is reported, so one bad pool cannot leak the others. + * * @returns a promise that resolves once the owned dispatchers are released. + * @throws `TransportFailureError` when one or more owned dispatchers failed to release. The + * rejection is memoized like the success path, so a later `close()` reports the same failure rather + * than falsely claiming a clean teardown. */ close(): Promise { this.#closing ??= (async () => { - for (const dispatcher of [...this.#dispatchers.owned].reverse()) { - await dispatcher.destroy(); + // Every owned dispatcher is destroyed even when an earlier one rejects. A bare `for … await` + // loop propagates on the first failure and leaks the pooled connections of every dispatcher + // after it -- and `owned` is walked in reverse, so with a proxy configured the ProxyAgent + // actually holding those connections is the one destroyed last. + const failures = await releaseAll(this.#dispatchers.owned); + if (failures.length > 0) { + // A raw undici error would otherwise escape a public method untyped (NFR-7); the causes are + // preserved rather than flattened to a message. + throw new TransportFailureError( + 'one or more owned dispatchers failed to release', + { + cause: + failures.length === 1 + ? failures[0] + : new AggregateError(failures), + }, + ); } })(); return this.#closing; } +} - /** - * Single teardown path, delegating to {@link UndiciTransport.close}. - * - * @returns a promise that resolves once teardown is complete. - */ - [Symbol.asyncDispose](): Promise { - return this.close(); - } +// Single teardown path for `await using`, delegating to `UndiciTransport.close()` and installed at run +// time only when the symbol exists — the same guarded shape `SseStream` and `Page` use. +// +// DO NOT restore this as a plain `[Symbol.asyncDispose]()` class member. Node 20.3 is this package's +// declared floor (`engines.node`, checked by verify:runtime-floor) and predates the symbol, which +// arrived in 20.4. On the floor the computed key evaluates to `undefined` and binds the method to the +// string key `"undefined"` — a junk prototype entry, and no working disposal. Declaring it on the +// class would also emit it into the `.d.ts` unconditionally, promising consumers on the floor a method +// that is not there. +if (typeof Symbol.asyncDispose === 'symbol') { + Object.defineProperty(UndiciTransport.prototype, Symbol.asyncDispose, { + value: function asyncDispose(this: UndiciTransport): Promise { + return this.close(); + }, + writable: true, + configurable: true, + }); } /** * Creates a `Transport` backed by `undici` — the full-featured option, with connection-pool control, * proxy support, and real `close()` semantics over the dispatchers it owns. * - * The returned transport is `AsyncDisposable`, so `await using transport = undiciTransport(...)` - * releases it at scope exit — the single teardown path `docs/knowledge/resource-management.md` asks - * for. + * `close()` is the single teardown path `docs/knowledge/resource-management.md` asks for, and the one + * that actually destroys the dispatchers this transport owns. A `[Symbol.asyncDispose]` delegating to + * it is installed at run time **when the runtime has the symbol**, which this package's declared floor + * (`engines.node >=20.3`) does not — it arrived in Node 20.4. The return type therefore does not + * promise `AsyncDisposable`: claiming it would type-check `await using` for a consumer sitting on the + * floor, where the method is genuinely absent, and leak every pooled connection. Call `close()`, or + * raise your own floor to 20.4+ and reach the symbol through a cast. * * @param options - optional transport settings. - * @returns a transport ready to send, disposable through `await using`. + * @returns a transport ready to send; release it with `close()`. * @throws `TypeError` when both `dispatcher` and `proxy` are supplied. * * @public */ export function undiciTransport( options: UndiciTransportOptions = {}, -): Transport & AsyncDisposable { +): Transport { return new UndiciTransport(options); } diff --git a/scripts/verify-dual-consumption.mjs b/scripts/verify-dual-consumption.mjs index b3f34a4..2e62d3b 100644 --- a/scripts/verify-dual-consumption.mjs +++ b/scripts/verify-dual-consumption.mjs @@ -103,9 +103,27 @@ assert.equal(inbound.headers.get('content-type'), 'text/plain'); // Exercise both transports far enough to prove the module graph resolved and construction runs -- // not far enough to need a network. `close()` is the one lifecycle call that is safe with no peer. +// Never index with a bare `Symbol.asyncDispose` here. This gate runs on whatever `node` is on PATH, +// which includes the declared `engines.node` floor of >=20.3 -- and the symbol arrived in 20.4. On the +// floor the computed key is `undefined`, so `transport[Symbol.asyncDispose]` reads the STRING key +// `"undefined"`. That used to resolve to the junk prototype entry left by an unguarded +// `[Symbol.asyncDispose]()` class member, so this assertion passed over a transport that could not be +// disposed; since the guarded install it resolves to `undefined` and the assertion fails outright. +// Branch on the symbol, and assert the junk key's absence on BOTH legs -- the same shape +// `packages/transport-fetch/src/fetch-transport.test.ts` uses. +const asyncDispose = Symbol.asyncDispose; for (const transport of [fetchTransport(), undiciTransport()]) { assert.equal(typeof transport.send, 'function'); - assert.equal(typeof transport[Symbol.asyncDispose], 'function'); + if (typeof asyncDispose === 'symbol') { + assert.equal(typeof transport[asyncDispose], 'function'); + await transport[asyncDispose](); + } + assert.ok( + !Object.getOwnPropertyNames(Object.getPrototypeOf(transport)).includes( + 'undefined', + ), + 'transport prototype carries an "undefined" key: [Symbol.asyncDispose] was declared as a plain class member ahead of the floor bump', + ); await transport.close(); } // Exercise @dexpace/rx bridge diff --git a/scripts/verify-reproducible-build.mjs b/scripts/verify-reproducible-build.mjs new file mode 100644 index 0000000..3550e2b --- /dev/null +++ b/scripts/verify-reproducible-build.mjs @@ -0,0 +1,222 @@ +// SPDX-License-Identifier: MIT +// scripts/verify-reproducible-build.mjs +// +// NFR-12: an identical source tree MUST produce a byte-identical build. +// +// This row sat open through Phase 10 on the stated grounds that it "cannot execute without a real +// build artifact" — true while the repository was docs-only, and false from Phase 1 on. The check is +// mechanical: build the workspace twice from a swept tree and compare a SHA-256 of every emitted +// file. Asserting reproducibility without running it is exactly the kind of claimed-but-unverified +// conformance `docs/open-items.md` exists to catch. +// +// Both builds sweep `dist/` and every `*.tsbuildinfo` first. Without the sweep the second `tsc` is +// incremental and rewrites nothing, so the comparison passes by not having run — the failure mode +// that makes a naive version of this gate worthless. +// +// Non-determinism this would catch: a timestamp or absolute path baked into emitted output, a +// `Math.random()`/`Date.now()` reaching a build-time codegen step +// (`packages/core/scripts/gen-version.mjs` is the one such step today, and injecting a `Date.now()` +// there is this gate's negative test), or a `tsc` upgrade that starts emitting map keys in hash +// order. +// +// TWO LEGS, because "the artifact" means two different things. The emit leg compares every file under +// each package's `dist/`. The pack leg then runs `npm pack` on every publishable package and compares +// the tarball digests — that is the byte sequence a consumer actually installs, and it is what the +// Phase 10 commit message asserted was reproducible on the strength of a by-hand check. Gating it is +// the difference between an asserted property and a verified one. +// +// The pack leg is deliberately kept: it is deterministic here because `npm pack` normalizes tar +// entries (fixed mtime, sorted order, portable mode bits) rather than stamping wall-clock time into +// the header — verified by packing `@dexpace/core` twice, seconds apart, on npm 12.0.1 and getting one +// digest. Both packs also happen inside a single run on a single npm, so an npm upgrade cannot make +// this flap. It adds ~7s and a dependency on `npm` being on PATH, which the Node toolchain CI already +// installs; a missing `npm` fails the gate loudly rather than skipping the leg. +// +// What the pack leg does NOT add much of, stated so nobody over-reads it: with `files: ["dist"]` on +// every publishable package, the tarball is a pure function of the `dist/` bytes the first leg already +// compared plus static manifest files. Its real job is to pin `npm pack`'s own normalization, and to +// catch a future `files`/`.npmignore` change that starts shipping something time-varying from outside +// `dist/`. +import assert from 'node:assert/strict'; +import {execFileSync, spawnSync} from 'node:child_process'; +import {createHash} from 'node:crypto'; +import { + mkdtempSync, + readFileSync, + readdirSync, + rmSync, + statSync, +} from 'node:fs'; +import {tmpdir} from 'node:os'; +import {join, relative} from 'node:path'; +import {fileURLToPath} from 'node:url'; + +const repoRoot = fileURLToPath(new URL('..', import.meta.url)); +const packagesDir = join(repoRoot, 'packages'); + +/** Every file under a package's `dist/`, repo-relative, sorted for a stable comparison order. */ +function collectArtifacts() { + const files = []; + const walk = dir => { + for (const entry of readdirSync(dir, {withFileTypes: true})) { + const full = join(dir, entry.name); + if (entry.isDirectory()) walk(full); + else if (entry.isFile()) files.push(full); + } + }; + for (const pkg of readdirSync(packagesDir, {withFileTypes: true})) { + if (!pkg.isDirectory()) continue; + const dist = join(packagesDir, pkg.name, 'dist'); + try { + if (statSync(dist).isDirectory()) walk(dist); + } catch { + // No dist/ for this package (private, or source-resolved) — nothing to compare. + } + } + return files.sort(); +} + +/** Sweep every build output so the next build starts from the tree CI checks out, not a warm one. */ +function sweep() { + const walkAndDelete = dir => { + for (const entry of readdirSync(dir, {withFileTypes: true})) { + if (entry.name === 'node_modules') continue; + const full = join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === 'dist') rmSync(full, {recursive: true, force: true}); + else walkAndDelete(full); + } else if (entry.name.endsWith('.tsbuildinfo')) { + rmSync(full, {force: true}); + } + } + }; + walkAndDelete(packagesDir); +} + +function build(label) { + process.stdout.write(`verify-reproducible-build: ${label} build…\n`); + execFileSync('bun', ['run', 'build'], {cwd: repoRoot, stdio: 'inherit'}); +} + +/** Map of repo-relative path → SHA-256 of the file's bytes. */ +function digestArtifacts() { + const digests = new Map(); + for (const file of collectArtifacts()) { + digests.set( + relative(repoRoot, file), + createHash('sha256').update(readFileSync(file)).digest('hex'), + ); + } + return digests; +} + +/** + * Every package `npm pack` produces a publishable tarball for. `private: true` packages + * (`shrink-test`, `transport-conformance`) never ship, so their bytes are not an artifact. + */ +function publishablePackages() { + const names = []; + for (const pkg of readdirSync(packagesDir, {withFileTypes: true})) { + if (!pkg.isDirectory()) continue; + const manifest = join(packagesDir, pkg.name, 'package.json'); + try { + if (JSON.parse(readFileSync(manifest, 'utf8')).private !== true) { + names.push(pkg.name); + } + } catch { + // No manifest — not a package, so nothing to pack. + } + } + return names.sort(); +} + +/** + * Map of `npm-pack:` → SHA-256 of the tarball, packed into a temp dir this owns. + * + * Packing outside the repo keeps the tarballs out of `collectArtifacts()`'s walk and out of + * `git status`; the dir is removed even when a pack throws. + */ +function digestTarballs(label) { + process.stdout.write(`verify-reproducible-build: ${label} pack…\n`); + const dest = mkdtempSync(join(tmpdir(), `dexpace-repro-${label}-`)); + try { + for (const name of publishablePackages()) { + execFileSync('npm', ['pack', '--pack-destination', dest], { + cwd: join(packagesDir, name), + // `npm pack` narrates the whole tarball manifest on stderr; the digests are the signal. + stdio: ['ignore', 'ignore', 'ignore'], + }); + } + const digests = new Map(); + for (const file of readdirSync(dest).sort()) { + digests.set( + `npm-pack:${file}`, + createHash('sha256') + .update(readFileSync(join(dest, file))) + .digest('hex'), + ); + } + return digests; + } finally { + rmSync(dest, {recursive: true, force: true}); + } +} + +/** The three ways two digest maps can disagree, rendered for the assertion message. */ +function diffDigests(a, b) { + return [ + ...[...a.keys()] + .filter(path => !b.has(path)) + .map(path => ` only in build 1: ${path}`), + ...[...b.keys()] + .filter(path => !a.has(path)) + .map(path => ` only in build 2: ${path}`), + ...[...a.entries()] + .filter(([path, hash]) => b.has(path) && b.get(path) !== hash) + .map(([path]) => ` differing bytes: ${path}`), + ]; +} + +// Fail here rather than inside the first `npm pack`, where an ENOENT from execFileSync reads as a +// packaging defect instead of a missing tool. +assert.equal( + spawnSync('npm', ['--version'], {stdio: 'ignore'}).status, + 0, + 'NFR-12: `npm` is not on PATH, so the pack leg cannot run. Install Node’s npm, or run the' + + ' emit leg alone by hand.', +); + +sweep(); +build('first'); +const first = digestArtifacts(); +const firstTarballs = digestTarballs('first'); + +assert.ok( + first.size > 0, + 'NFR-12: the build emitted no artifacts at all — nothing to compare', +); +assert.ok( + firstTarballs.size > 0, + 'NFR-12: no publishable package produced a tarball — nothing to compare', +); + +sweep(); +build('second'); +const second = digestArtifacts(); +const secondTarballs = digestTarballs('second'); + +const problems = [ + ...diffDigests(first, second), + ...diffDigests(firstTarballs, secondTarballs), +]; + +assert.equal( + problems.length, + 0, + `NFR-12 violation: two clean builds of an identical source tree differed.\n${problems.join('\n')}`, +); + +process.stdout.write( + `verify-reproducible-build: OK — ${String(first.size)} emitted files and ` + + `${String(firstTarballs.size)} npm-pack tarballs byte-identical across two clean builds (NFR-12)\n`, +); diff --git a/test/node-conformance/pagination.test.mjs b/test/node-conformance/pagination.test.mjs index 7e892a9..5bcc4bb 100644 --- a/test/node-conformance/pagination.test.mjs +++ b/test/node-conformance/pagination.test.mjs @@ -4,7 +4,9 @@ // Phase 6c's runtime-divergent surface, run against the BUILT artifact on real Node. // // Three things in this phase are runtime-divergent across Bun and Node: -// 1. Explicit Resource Management: `Page` implements `[Symbol.asyncDispose]` delegating to `close()`. +// 1. Explicit Resource Management: `Page` installs `[Symbol.asyncDispose]` delegating to `close()`, +// guarded on the symbol existing — it postdates `engines.node`'s >=20.3 floor (it arrived in 20.4), +// so this suite's 20.3.0 matrix leg must assert its ABSENCE, not skip the check. // 2. `AbortSignal` integration: threading signal into every request exchange and halting pagination walks at boundaries. // 3. `Response.close()` cancelling active `ReadableStream` bodies upon advance, early break, or completion. import assert from 'node:assert/strict'; @@ -16,7 +18,30 @@ import { } from '../../packages/core/dist/testing/fake-transport.js'; describe('Page explicit resource management on Node (PAGE-3, PAGE-12)', () => { - it('disposes the page via Symbol.asyncDispose, releasing the response body', async () => { + // Never index with a bare `Symbol.asyncDispose`. On the >=20.3 floor it is `undefined`, so + // `page[Symbol.asyncDispose]` reads `page['undefined']` — which used to resolve to a junk prototype + // entry left by an unguarded `async [Symbol.asyncDispose]()` class member and made this very + // assertion pass on a Page that could not be disposed at all. Branch on the symbol instead, and + // check the junk key is gone on both legs. + it('leaves no "undefined" prototype key on any Node version (guarded install)', () => { + const {response} = countingResponse(200); + const page = new Page(response, ['item-1']); + assert.ok( + !Object.getOwnPropertyNames(Object.getPrototypeOf(page)).includes( + 'undefined', + ), + 'Page.prototype carries an "undefined" key: [Symbol.asyncDispose] was declared as a plain class member ahead of the floor bump', + ); + assert.equal(typeof page.close, 'function'); + }); + + it('disposes the page via Symbol.asyncDispose, releasing the response body', async t => { + if (typeof Symbol.asyncDispose !== 'symbol') { + t.skip( + `Symbol.asyncDispose is absent on ${process.version} (arrives in 20.4); the guarded install is correctly a no-op here`, + ); + return; + } const {response, cancelCount} = countingResponse(200); const page = new Page(response, ['item-1', 'item-2']);