Fix: three Phase 10 defects and correct the Phase 10 documents - #59
Merged
Conversation
…bug it found
Audits all 17 entries of the Phase 10 reconciled ledger
(docs/sdk-design-nodejs/10-deliberate-deviations-from-the-reference-contract.md)
against the as-built code rather than against the phase specs that produced
it. Fourteen hold and are permanently uncorrectable; three did not survive
contact with the source. One of those three was a live defect.
Fixes the defect:
- Page, FetchTransport, and UndiciTransport each declared
[Symbol.asyncDispose] as a plain computed class member. The symbol arrived
in Node 20.4; every package declares engines.node ">=20.3". On the declared
floor the computed key evaluates to `undefined`, so the method bound to the
string key "undefined" — junk on the prototype, no working disposal, and a
.d.ts promising AsyncDisposable regardless. Verified against the built
artifact on real Node 20.3.0:
before → Page.prototype keys: [ 'constructor', 'undefined' ]
after → Page.prototype keys: [ 'constructor', 'close' ]
All three now install it via Object.defineProperty behind
`typeof Symbol.asyncDispose === m. Phase 3b had
already found this hazard and pinned Response's freedom from it
(http/response.test.ts); 6c and classes.
- test/node-conformance/pagination
`typeof page[Symbol.asyncDispose] === 'function'`. On the floor that reads
page['undefined'], which *was* ttion passed over
a Page that could not be disposed — the 20.3.0 matrix leg reported
conformance it did not have. It nd asserts the
junk key's absence on both legs: 20.3.0 → 3 pass / 1 skip, current → 4 pass.
Breaking, in the type system only (changeset included; pre-1.0 minor):
Page no longer declares `implementhTransport() /
undiciTransport() return `Transport` rather than `Transport & AsyncDisposable`.
`await using` therefore stops types. Deliberate:
the declaration was only ever true on 20.4+, and on the floor it type-checked
a call that silently did nothing —ant leaking every
pooled connection. close() is the supported teardown path and is unchanged.
Closes NFR-12 on evidence:
- Two clean builds of an identical tree (every dist/ and *.tsbuildinfo swept
between them) emit 644 byte-idendexpace/core
twice yields an identical tarball digest.
- scripts/verify-reproducible-builCI step, and is
registered in ci-preflight's hardcoded STEPS list too — otherwise the
preflight silently under-covers ci.yml. Negative-tested by injecting a
Date.now() into gen-version.mjs: the gate fails naming
dist/generated/version.js.
- The ledger's "cannot execute without a real build artifact" premise expired
once Phases 1-9 shipped code. This needed *code*, not a *publish*, and sat
open two phases longer than it had to (open-items.md B3, now closed).
Corrects four ledger entries that misdescribed the code, each with an explicit
"corrected 2026-08-29" note rather than a silent overwrite:
item 4 "never bare structural interfaces" — core.api.md exports 61
interfaces against 58 classes, and Configuration is builder-built,
frozen, and exported structurally. Narrowed to the http/ wire models
item 7 "three tiers, not four" — the code implements CFG-1's full
override → env → property → default chain, with CFG-3 key
normalization; withPropertySource()/getRawProperty() are public API.
Only the default production binding of the property layer is empty
item 11 claimed Body/Response add the member (they never have — two tests
assert the opposite), claimed all sites were guarded (only SseStream
was), and omitted both transports while asserting consistency
"across all four sites"
item 14 claimed `npm publish --provenance` "is scripted (Phase 0 Task 3)".
It is not, anywhere. NFR-12 split out and closed; NFR-16 stays open
Adds docs/deviations.md: the 14 uncorrectable deviations, each with the
file:line proving the claim and why the reference mechanism cannot be restored.
Not done here, deliberately: the NFR-16 release workflow. Authoring it with
--provenance and id-token: write is actionable now — only running it against a
real registry is blocked, so the ledger's "blocked until first release" framing
was half wrong and is corrected. It is left out because a release workflow's
trigger, environment, tag convention, and publish permissions are a project
decision, not a defect repair. Recorded as actionable-now in open-items.md,
alongside a WATCH item: raising engines.node to >=20.4 would let all four
disposal sites drop the guard and restore `await using` honestly.
…install A three-pass review (shape, adversarial, reader) over 27fb81f. The shape held: the guarded installs are NFR-10's repair, not a workaround, and the Node floor stays >=20.3. Reading the running code rather than the diff turned up three more live defects, all on the same seam. - scripts/verify-dual-consumption.mjs asserted `typeof transport[Symbol.asyncDispose] === 'function'` unguarded, inside a blocking CI gate. On the >=20.3 floor that reads transport['undefined'] — which, before 27fb81f, resolved to the junk prototype entry the bug itself created. The gate's greenness depended on the defect it existed to catch; fixing the defect broke the gate, and CI could not see it because the `ci` job runs setup-bun's default node. Reproduced on real Node 20.3.0: AssertionError: 'undefined' !== 'function' (exit 1) Now guarded, and asserts the junk key's absence on both legs. - UndiciTransport.close() walked `owned` with a bare `for … await`, so the first rejecting destroy() abandoned the rest. The set is walked reversed, so with a proxy configured the leaked dispatcher is the ProxyAgent holding the pooled connections — a teardown path that leaked exactly what it was called to release. The raw undici error also escaped a public method untyped. Every dispatcher is now destroyed before anything throws, and failures surface as TransportFailureError with {cause}, AggregateError when several fail. close() memoizes the rejection, so TRANSPORT-16's "stable state" holds: the same error object comes back every time. - UndiciTransport.send() called prepareBody() before toUndiciHeaders(), and header mapping reads request.body.mediaType — a getter on a caller-supplied Body that can throw. pumpBody starts its producer eagerly, so the throw escaped with a live producer nothing could abandon and a rejection headed for Node's default unhandledRejection policy: the SEAM-30 hazard body-pump.ts documents. Header mapping is hoisted above body preparation. FetchTransport was already correct by object-literal evaluation order, which is implicit and easy to regress, so it gets the same guard test. - The undici disposal test asserted only that the member existed; an asyncDispose wired to a bare resolved promise passed it. It now swaps undici's Agent binding for a capturing subclass and asserts the owned dispatcher is destroyed by dispose alone. Closes the sideEffects question 27fb81f opened. Four packages now carry a module-scope Object.defineProperty install while declaring "sideEffects": false. packages/shrink-test/ asserts the symbol survives a real esbuild bundle + minify + treeShaking pass, negative-tested by stripping the install from the built page.js. sideEffects was deliberately not narrowed to four paths — a narrow list goes stale on the next file move and fails open. Widens the NFR-12 gate to the leg 27fb81f verified by hand. npm pack is deterministic here (npm normalizes tar entries rather than stamping wall-clock time), so all 9 publishable packages are packed and digest-compared on both clean builds. Injecting Date.now() into gen-version.mjs now fails both legs, naming dist/generated/version.js and npm-pack:dexpace-core-0.0.0.tgz. The gate's registration was also one place short: ci-preflight's SKILL.md playbook table had no row for it, and five step counts across that skill were stale. Makes the doc tree describe what Phase 10 became: roadmap :65, design :15 asserted "review only" / "ships no package". It shipped a breaking type change across three packages, two changesets, a blocking CI step, and four defect fixes — the same assert-without- checking this phase spent 86 lines correcting §10 ↔ docs/deviations.md two ledgers on one numbering scheme with no cross-reference, and a third file with a nearly identical name. All three now disambiguated knowledge corpus deliberate-deviations.md pins sha f9ecb6e7d87b, traced to 05d649a — three revisions back. Flagged at the head, and inline on the two entries that are substantively false, so the correction reaches `bun run knowledge` output and not just a reader roadmap :169 maxRetries was already fixed in cba4721; the row claiming Phase 10 owned it is marked resolved F2, F7, CONSTANT_CASE project-wide convention sweeps, recorded Not scheduled with triggers. Phase 10 is the last roadmap row, so there was no honest owner to name Records the floor decision where it will be found. Raising engines.node to >=20.4 to restore `implements AsyncDisposable` was recorded as a pending unblock; it is now decided against, in open-items.md and the two other places the recommendation had spread to. NFR-10 requires a higher-floor capability to be isolated into its own unit rather than lifting the general-purpose core's floor, and requires the emitted target and visible-API level to agree — the clause the unguarded member violated. >=20.3 is derived, not chosen: the lowest Node that runs what these packages emit, set by globalThis.crypto (absent from ESM on every Node 18) and AbortSignal.any() (20.3.0). Phase 4b already decided this shape once, shipping a guarded suppress() rather than moving the floor for SuppressedError. Verified: preflight 15/15 --clean and 16/16 --node-floor on the pinned Bun 1.3.14, including the real Node 20.3.0 leg; 2175 tests.
Wahbeh-Mohammad
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Defects fixed
scripts/verify-dual-consumption.mjsreadSymbol.asyncDisposewithout aguard. The symbol is absent on Node 20.3.0, which is the declared floor. This
blocking gate failed on that floor. The script now uses a guard.
UndiciTransport.close()stopped at the first dispatcher that failed todestroy. It leaked the other dispatchers. With a proxy, it leaked the
ProxyAgentthat holds the pooled connections. The method now destroys alldispatchers. It reports failures as
TransportFailureError.UndiciTransport.send()prepared the body before it mapped the headers.Header mapping reads a caller-supplied getter. If that getter throws, an
active body producer stays behind. The method now maps the headers first.
nothing. The test now asserts that dispose destroys the dispatcher.
Gates
packages/shrink-test/asserts that the disposal symbol stays after esbuildremoves unused code. This keeps
"sideEffects": falsecorrect.npm packtarballs. Before,only
dist/was compared.Documents
code. Both documents now record the change of scope.
>=20.3. NFR-10 does not permit a higher floor forthe core. This decision is now recorded.
Reviews performed
Three passes. Each pass used a separate subagent.
The guarded install satisfies NFR-10.
test. Each test failed before the fix.
documents that did not match the code.
Verification: preflight passes 15 of 15 steps with
--clean, and 16 of 16 with--node-floor. The floor leg runs on Node 20.3.0. 2175 tests pass.Open items and deferred items
--provenance. Not done. The trigger,the tag convention, and the publish permissions are project decisions.
CONSTANT_CASE— three project-wide convention sweeps. Theroadmap listed Phase 10 as the owner. Phase 10 is the last row of the phase
table, so no later phase can own them. All three are now marked as not
scheduled, each with a trigger.
docs/knowledge/deliberate-deviations.mdneeds a newharvest. A harvest is out of scope for this branch.
bun run test:scripts— not part of CI. See open-items H13.