Review: PR #243 — fix(node): T3 correctness batch

Verdict: Approve (non-blocking). I checked out the branch, read every touched file in full, and ran the suites. pnpm vitest run test/node-adapters.test.ts test/node.test.ts → 159 passed / 6 skipped. Full pnpm test (after pnpm build so the srvx self-imports resolve) → 985 passed / 35 skipped, no type errors, lint clean. Diff is scoped strictly to the Node adapter + loader + types + tests; no scope creep, no upgrade listener added, F17/F18 in response.ts/node.ts left untouched (owned by #230). No correctness bugs found — the changes are careful and the reasoning in the comments holds up. A few informational notes and one expected merge conflict below.

Per-item checklist (1–11)

1. F53 types.ts ✅ _request JSDoc now correctly describes the undici web Request (not IncomingMessage, points at runtime.node.req); both _request/_url marked supported/frozen (D7).
2. Loader --import message ✅ Verified loader.ts:166 already reads srvx --import jiti/register on main; this PR does not touch it. Claim correct.
3. Loader arity heuristic ✅ Comment/JSDoc added only; mod.default.length < 2 condition byte-identical. No behavior change.
4. handleSendError diagnostics ✅ console.errors unless silent; logged once (only from the detached catch — sendNodeResponse still rejects, streaming errors stay .catch(()=>{})). silent is threaded via this.options.silent; the changed function is @internal (sendNodeResponse, the public re-export, is unchanged) → no public signature change. Client still gets a bare 500, no detail leak (test asserts empty body).
5. HEAD stream cancel ✅ stream.cancel().catch(()=>{}) is error-guarded; headers are already written by the caller before streamBody, so content-type/length still emitted; non-stream HEAD bodies fall to Node's own _hasBody=false suppression. Correct.
6. statusText → "" ✅ Defaults to spec empty phrase; Node still fills a reason phrase into the status line on the wire (_storeHeader uses STATUS_CODES when statusMessage is falsy), so wire output stays valid. Test asserts at the web-Response layer, which is the right layer.
7. Empty-string body ✅ != null (not truthy) now emits text/plain; charset=UTF-8 + content-length: 0. See note A on the exact string.
8. text()/json() on locked/disturbed stream ✅ new Response(stream) synchronous throw wrapped → Promise.reject. Test drives it via a directly-disturbed req.body and asserts a TypeError rejection.
9. GET/HEAD null-body ✅ Verified #hasBody() already on main (#228); PR only adds the locked-stream regression test.
10. Materialization identity (headers) ✅ for the requirement in the plan — see note B for the residual divergence.
11. Synthetic IncomingMessage + chunked fix ✅ — scrutinized hard, see below.

On item 11 (the one that needed the hardest look): the useChunkedEncodingByDefault = false in web/response.ts is correct and necessary, not incidental. Reporting HTTP/1.1 flips Node's ServerResponse default: previously the synthetic httpVersionMajor was null (null < 1 → true), so Node's constructor took the <1.1 branch and set useChunkedEncodingByDefault = false; now that branch is skipped and it inherits true from OutgoingMessage. The bridge's WebRequestSocket._write captures the post-header bytes verbatim (no de-chunking) into _webResBody, so without this line every non-content-length streamed bridge response would be chunk-framed and corrupted. The bridge socket is a Duplex, never a real wire, so disabling the default has no on-wire effect. rawHeaders is built as flat [name, value, …] preserving case, with set-cookie expanded per-entry and stored as an array on headers (matching Node) — good.

Findings

A. [informational] Empty-string content-type differs from native undici. _node/response.ts:130 emits text/plain; charset=UTF-8 (space, UTF-8) whereas native Response("") gives text/plain;charset=UTF-8. This is the adapter's long-standing convention for all string bodies, not new to this change — the PR's own test documents the mismatch. Consistent within the adapter; flagging only so it's a conscious call.

B. [low] req.headers and req._request.headers diverge after materialization. _node/request.ts now keeps #headers (the NodeRequestHeaders) canonical across _request materialization — which correctly satisfies the plan's "a reference taken before materialization stays live." But the native Request built in the _request getter copies header entries into its own Headers at construction. So after void req._request, mutating req.headers (or the pre-materialization ref) is not visible on req._request.headers, and vice-versa. Since _request is now documented supported API, a consumer that materializes, mutates req.headers, then hands req._request to undici/fetch will silently lose the mutation. The added test only exercises the req.headers/old-ref direction, not the _request direction. Not a regression (pre-PR the pre-materialization ref detached instead), and arguably acceptable, but the boundary is worth a doc line or a follow-up.

C. [informational] Body identity across materialization is punted. The _request comment addresses headers only; #bodyStream = undefined is retained so post-materialization body comes from the native request. No regression (body reads funnel consistently through one source), but the plan flagged body identity too and the PR neither covers nor explicitly calls it out. A one-line "body identity intentionally deferred" note would close the loop.

D. [informational] rawHeaders cannot fully reconstruct wire form. web/incoming.ts builds rawHeaders from req.headers.entries(), which comma-joins duplicate non-set-cookie headers, so repeated headers collapse to one entry and original wire order isn't preserved. The comment acknowledges "best-effort." Acceptable for the morgan/:http-version use case.

E. [merge — expected conflict with #230] Both PRs edit _node/response.ts in the same region. #243 deletes const STATUS_CODES = … and rewrites statusText to drop it; #230 keeps STATUS_CODES (its statusText still reads it) and inserts NULL_BODY_STATUS + a null-body constructor throw immediately after that line. This will conflict textually and semantically — resolution must keep #243's statusText/STATUS_CODES removal and #230's null-body constructor throw. types.ts (#230 removes netlify/stormkit/vercel at ~347–353; #243 edits _request/_url JSDoc at ~361–386) and node.ts (#230 at lines 6/54/140/194; #243 at 86–92) are non-adjacent and should auto-merge.

Nice work — the hot-path reasoning (chunked-default restoration, once-only error logging, canonical-headers identity) is sound and well-commented.

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/adapters/_node/web/incoming.ts`:
- Around line 30-45: The synthetic transfer-encoding handling added after header
conversion must update both representations. In the body-bearing request path
around the transfer-encoding logic, append the same “transfer-encoding”,
“chunked” pair to rawHeaders whenever it is added to this.headers, while
preserving existing behavior for requests with length metadata.

In `@test/node-adapters.test.ts`:
- Around line 777-805: Update the HEAD streaming-body test to synchronize
cancellation through a promise resolved by the stream’s cancel() callback, then
await that promise before asserting cancelled and pulls. Remove the fixed 50 ms
timeout while preserving the existing cancellation and pull-count assertions.

---

Nitpick comments:
In `@test/node-adapters.test.ts`:
- Around line 920-945: Extend the “synthetic IncomingMessage exposes httpVersion
and rawHeaders” test to send two ordered set-cookie values, then have the
handler return both the ordered raw header pairs and req.headers["set-cookie"].
Assert that the response preserves both cookie values in order and retains the
existing HTTP version expectations.