Repository navigation
feat: add @loquix/adapter-http, and stop sendTimeout cutting off long responses - #17
Merged
Merged
Conversation
Review found the same-origin credentials default was untested (deleting it left all tests green) and that a caller-supplied header differing only in case, e.g. Content-Type, sat beside the adapter's content-type default instead of overriding it. Lowercase caller header keys before merging so HTTP's case-insensitive header names behave as callers expect, and add tests for both the default/override credentials behavior and the header case-folding.
fakeFetch handed the same body stream object to every call it made, so a test that sends twice through one provider found the stream already locked/disturbed on the second call. Fix fakeFetch to tee the body per call instead.
Forking one body with tee() let a second send() get an undisturbed stream, but it silently broke cancellation: cancelling a tee'd branch leaves the underlying source running, and awaiting that cancel never settles. The next task asserts that cancelling the response reaches the body, so this would have hung its suite rather than failed it. A factory gives each call a genuinely fresh stream. Only the one test that sends twice needs it; everything else keeps passing the stream directly, so cancellation propagates straight through.
Adds HttpAgentError and errorFromResponse (per the B6 brief), throws on non-2xx responses before touching the body, and replaces the `response.body as ReadableStream<Uint8Array>` cast with a real null check that throws HttpAgentError(502, ...) instead of letting a body-less response surface a raw TypeError.
Wire @loquix/adapter-http into the root build/test/test:coverage scripts so CI actually runs its suite, and write the changeset for the first release.
Round out packaging for the first release, per review findings: - Add publishConfig.access so the scoped package publishes as public instead of failing on npm's restricted-access default. - Exclude src/test-helpers.ts from the tsconfig build so it stops compiling into dist and landing in the published tarball. - Add a types condition to the "." export, matching @loquix/core's shape. - Add LICENSE (copied from core), repository/homepage/bugs metadata, and a README so the npm page isn't blank. - Fix web-test-runner.config.mjs's coverage exclude, which pointed at a nonexistent src/test-utils.ts instead of src/test-helpers.ts, so the test scaffolding was never actually excluded from coverage. - Add a test locking in that a throwing parse hook errors the stream with the consumer's own exception, unwrapped, rather than being swallowed the way a malformed default-parser line is. - Add a test locking in that the decoder cancels the underlying body after [DONE] even when the server never closes it. - Strengthen the abort test to assert the rejection is a DOMException named AbortError, not just any rejection.
The workflow built core and the React wrappers by name, so a type error in the adapter shipped a green pull request: test:coverage runs through esbuild, which strips types without checking them. The break would only surface in the release job's build, after merge, where it blocks every package's release. Verified by injecting a type error and running CI's exact sequence — lint, format:check, both existing builds, test:coverage — which all passed before this step existed.
Nine review findings addressed ahead of the first npm publish: - changeset config: turn on onlyUpdatePeerDependentsWhenOutOfRange so a core minor release no longer forces adapter-http's first version to 1.0.0 (verified with changeset status --verbose before/after) - honour the parse hook on the text transport instead of silently ignoring it - add tests locking in that [DONE] is ordinary content on text, and that sse never JSON-parses its payload - error the stream (naming transport + content-type) when a non-empty body produces zero chunks, instead of a silent blank message; a genuinely empty body still closes cleanly - carry the real response.status through the null-body error instead of fabricating 502 - README: document the sse/ndjson JSON-parsing asymmetry and replace the dead loquix.dev doc link with a link to the package source - derive an Accept header from the transport, overridable by the caller - sideEffects: false, a second Codecov upload for adapter-http, and reject an unknown transport at construction instead of falling through to ndjson 43/43 tests passing in both Chromium and Webkit (up from 31).
Follow-up to ae2aeac after a second review pass on the same lines: - carry the real response.status into the transport-mismatch error instead of a fabricated 502, via a new DecodeMeta{status,contentType} param on decode() - narrow the mismatch check to only judge the adapter's own default framing: skip it when the caller supplied a `parse` hook (a filtering hook legitimately producing no output for a turn is not evidence of a framing mismatch) or when a [DONE] sentinel was seen anywhere in the response (proves the default framing matched) - fixes a case the first pass broke: a server closing on `data: [DONE]` with no trailing separator and no content now stays a clean empty stream again, via the same [DONE]-seen check reaching the flush path - text transport: filter a parse hook's '' the same way sse/ndjson filter an empty payload, instead of enqueuing a phantom empty chunk - Accept header: add a `, */*;q=0.1` wildcard fallback per transport so a server doing strict negotiation can't 406 a request that previously succeeded with no Accept header at all 46/46 tests passing in both Chromium and Webkit (up from 43).
The mismatch check added in 9dc86d0 used "non-empty body, zero chunks enqueued" as its signal, which is wrong in principle: a correctly-framed sse body of only keep-alive comments, or an ndjson body of only metadata objects / {"text":null} / blank lines, all produce zero chunks by design and are documented as harmless — yet all four hard-errored, telling a developer to change a transport option that was already correct. Replace the signal with frame-shape recognition. A frame is: - sse: recognized if any line starts with data:/event:/id:/retry:/: ; ignorable if the whole frame is blank - ndjson: recognized if JSON.parse succeeds; ignorable if blank - text: never reaches the check (no hook always enqueues) Error only when at least one non-ignorable frame arrived and none was recognized — that is, no frame anywhere had the configured transport's shape. All the existing gates stay: no bytes, something enqueued, a parse hook supplied, or [DONE] seen. Verified against all eight cases from the follow-up review (three error, five clean), each with its own test. 51/51 tests passing in both Chromium and Webkit (up from 46).
sendTimeout composed AbortSignal.timeout() into the same signal handed to provider.send() for that signal's whole lifetime. A provider that forwards the signal to fetch (as @loquix/adapter-http does) had the timer keep running after the response stream was returned, aborting the body of any answer that streamed longer than sendTimeout in total. sendTimeout is now backed by its own AbortController + setTimeout and is cleared the instant provider.send() resolves, so it only bounds the wait for the stream to start, matching its documented intent. A non-finite or negative sendTimeout no longer throws (AbortSignal.timeout(-1) used to raise a RangeError); it's treated as disabled, like 0. Add streamIdleTimeout (default 60_000, 0 disables) to restore the safety sendTimeout used to provide for a different failure mode: a server that returns headers and then goes silent. It lives in StreamingController, which owns chunk delivery, resets on every chunk, and is suspended while paused so a paused consumer is never mistaken for a stalled server.
UploadController's own option sanitizing sends a non-finite value to the default; the new timeout sanitizer sent it to zero, which silently removes the guard. The usual source of NaN here is arithmetic on a missing config value, so disabling every timeout is the wrong failure mode, and two sibling controllers disagreeing about the same class of mistake is how a false claim gets written into the docs later. An explicit zero or negative still means off.
… 0.1.0 Adversarial pre-publish review of @loquix/adapter-http surfaced seven substantive bugs plus several smaller ones, all reproduced against real server response shapes before fixing: - errorFromResponse now extracts body.message / body.error.message / body.error / body.detail (stringifying non-string detail sensibly, e.g. FastAPI's validation-error array), and HttpAgentError carries the parsed body plus a `code: 'http' | 'no_body' | 'transport_mismatch'` so consumers no longer have to string-match messages to tell failure kinds apart. - errorFromResponse only awaits response.json() when content-type says json, so a streaming error body (e.g. 500 with text/event-stream that never closes) can no longer hang send() forever. - transport: 'text' combined with parse now throws at construction: chunk boundaries under 'text' follow arbitrary TCP segmentation, so a parse hook's output is not reproducible. Loosening this later is not breaking; tightening it later would be. - parse now receives the SSE frame's `event`/`id` as a second argument, since some backends (e.g. LangServe) put their only error discriminator in `event:` and never in the payload. - The transport-mismatch comment and test are narrowed to sse: under ndjson a bare JSON error object is well-formed JSON and is honestly documented as an accepted, uncaught gap rather than papered over with a new heuristic (a prior heuristic here was reverted twice for false-positiving on legitimate per-item `error` fields). - A redirected response now names the real cause (the redirect target) in the mismatch error instead of blaming the transport option. - [DONE] is now matched against the trimmed payload, a non-string parse return value is coerced with String(...) instead of enqueued as-is, and a body() builder that returns a string is sent as-is instead of being JSON-encoded a second time. types.ts now documents every option, and the README tells backend authors to close the response or send [DONE]. 51 -> 65 tests passing in both Chromium and Webkit.
…a resolved-but-timed-out send as a real response Final pre-publish review of the agent-timeouts changeset surfaced one real bug and one behavior gap in the fix it was describing: - sendTimeoutId was a send()-local variable, only ever cleared once provider.send()'s promise settled. abort() (and reset()/hostDisconnected(), which both route through it) never touched it, so a provider that ignores its abort signal and never resolves — a wrapped SDK client with no abort support has exactly this shape — left the timer pending for up to sendTimeout, keeping a discarded controller (and, in Node/SSR, the event loop) alive. Promoted to a private field, cleared in abort(); a real-timer tracking test (wtr has no fake timers) proves nothing is left pending after abort()/reset()/hostDisconnected(). - After provider.send() resolves, the code only checked localSignal.aborted, not composedSignal.aborted. A provider that resolves instead of rejecting exactly when sendTimeout fires (not checking its own signal before returning) produced a real response for onResponseStart/onResponseComplete even though the timeout had already fired — an app that opens a message bubble on onResponseStart would flash one for a response that's already dead. Now surfaces the TimeoutError instead. sanitizeTimeoutMs now distinguishes NaN (falls back to the default, since the usual cause is arithmetic on a missing config value) from ±Infinity (now disables the timeout, `0` — a deliberate "run forever" rather than a mistake to recover from). Previously both non-finite cases fell back to the default, silently turning `sendTimeout: Infinity` into 60 seconds. This deliberately diverges from UploadController._sanitizeInt, where Infinity has no sensible meaning for a concurrency/retry count and stays folded into the NaN case — documented inline on both. Updated the three JSDoc blocks (sendTimeout, streamIdleTimeout, sanitizeTimeoutMs itself — publicly exported, so this ships in the .d.ts) that still described the old "non-finite disables" behavior, and added that sendTimeout is advisory: it's enforced by aborting the signal, and a provider that ignores that signal is simply not bounded by it. The changeset now tells upgraders the thing that actually bites: an existing `sendTimeout: 0` opt-out no longer means "no timeouts" now that streamIdleTimeout defaults to on, and spells out the resulting failure (TimeoutError via onError, controller in error, partial response discarded).
… not "[object Object]"
Final pre-publish review turned up one more real bug plus doc drift, both
measured against the fix:
- normalizeParsedText coerced a non-string, non-null/undefined parse-hook
return with bare String(...). The likeliest such mistake —
`parse: c => JSON.parse(c)`, forgetting `.text` — rendered
"[object Object]" into the chat, which carries no information about its
own cause. request.ts's stringifyMessage already solved this exact problem
for HTTP error bodies; normalizeParsedText now mirrors it: JSON.stringify,
falling back to String(...) only if that throws (e.g. a circular
structure). The same mistake now renders `{"text":"Hel"}`, which points
straight at the missing `.text`. `undefined` is unaffected — it was, and
still is, dropped rather than coerced, since it's what a bare/missing
`return` produces, not a mistake; types.ts now says so explicitly instead
of implying every non-string is coerced.
- types.ts's JSDoc for `parse`'s second argument motivated it entirely with
seeing a bare `event: error` frame's discriminator, but decode() returns
before ever calling parse when a frame has no `data:` line at all — so
that exact case is invisible to the hook. Narrowed the doc instead of
widening the code: calling `parse('', frame)` for a data-less frame was
considered and rejected, since a hook doing `JSON.parse(payload)` would
throw on the empty string and error the whole stream over a frame shape
real-world sources (LangServe included) don't send bare. Added a test
locking in that a data-less `event:`/`id:`-only frame never reaches parse.
Also softened the request.ts comment on the content-type gate around
errorFromResponse's `.json()` call: it fixes a non-JSON body that never
closes, not a JSON one that never closes — a caller still needs its own
abort for that case. And noted on HttpAgentError.body that the field is
always assigned, so `'body' in err` is true even when it's undefined; check
`err.body !== undefined` instead.
…end's sendTimeout sendTimeoutId lived only in AgentController's shared `_sendTimeoutId` field, and both settle paths in send() cleared it unconditionally. If cycle N's provider.send() rejection (from abort()) landed as a microtask *after* cycle N+1 had already armed its own timer and written it to the same field — which happens whenever nothing yields between abort() and the next send(), e.g. `agent.abort(); agent.send(x)` — cycle N's catch block clobbered cycle N+1's timer, leaving the second send with no timeout at all. Capture the timer id in a send()-local on top of the field, clear the local unconditionally on both settle paths, and only null the field when it still points at that same id. abort()'s own clearTimeout is untouched. Added a regression test that reproduces the exact race with a HangingProvider (settles only via its abort signal) and asserts the second send still times out.
…sabled
connect() resolved a bare, unconfigured idle timeout to 60_000, so
every pre-existing direct caller of the public StreamingController
export — used by host applications directly, per its own class doc,
not just AgentController — silently gained a 60s idle abort it never
had before. The changeset shipped alongside this asserted the opposite
("The parameter is optional, so existing calls are unaffected."),
which was false.
connect() now defaults idleTimeout to 0 (disabled); only AgentController
opts into the 60s default, resolving it explicitly at its own call site
(previous commit) rather than relying on connect()'s default — needed
because this._options.streamIdleTimeout can be a literal `undefined`
even after the constructor's own default merge, e.g. when a caller
spreads in an unset config value.
Added a test proving a bare connect(stream) arms no setTimeout at all,
and corrected the changeset's false sentence about existing connect()
callers being unaffected.
@loquix/adapter-http's peer range still allowed @loquix/[email protected], the version whose sendTimeout cuts off any response streaming past 60 seconds — exactly the bug this repo's adapter exists to forward a signal into via fetch. With onlyUpdatePeerDependentsWhenOutOfRange, changeset version would leave the range alone, so @loquix/[email protected] would install happily against the broken core version and the adapter would get blamed for core's bug.
…tion, widen its return type, and accept a lone CR in SSE framing
Three issues from code review, all verified with extended tests:
- decode()'s pull() rejected on a throwing `parse` hook but never cancelled
or released the source reader's lock, leaving an open SSE connection
consuming a browser connection slot and server resources indefinitely (a
rejected pull() does not itself invoke the source's cancel algorithm).
pull() now cancels the source body and rethrows the original exception
unchanged before letting the stream error.
- `HttpAgentProviderOptions.parse` was typed to return `string | null`, but
its own JSDoc documents `undefined` as the ordinary "skip this chunk"
result and `normalizeParsedText()` already implements that. Widened the
public type (and decode()'s matching parameter) to `string | null |
undefined`, and dropped the `as unknown as` casts in transports.test.ts
that existed only to route around the gap.
- The SSE spec terminates a line with CRLF, CR, or LF, but the frame
separator and line splitter only handled LF/CRLF, so a body using bare CR
line endings decoded as one malformed payload. Fixed with
`\r\n|\r(?!\n)|\n`: the negative lookahead keeps a real CRLF from ever
being re-split into a lone CR + a lone LF when the frame-separator's `{2}`
quantifier backtracks looking for a match — an early draft without the
lookahead passed the new lone-CR cases but broke a CRLF-only body,
caught by a dedicated regression test added alongside it.
… and correct the timeout changeset/test names
`sanitizeTimeoutMs()` passed every finite positive number straight to
`setTimeout`, which only accepts a 32-bit signed integer. Measured:
`setTimeout(fn, 2_147_483_648)` fires after 1ms instead of after the huge
delay the caller asked for, so a very long `sendTimeout`/`streamIdleTimeout`
failed almost instantly — the opposite of the caller's intent. Now clamped
to `2_147_483_647`, the closest value to what was actually asked for;
`±Infinity` remains the explicit way to disable a timeout entirely.
Also: `.changeset/agent-timeouts.md` still claimed all non-finite values
fall back to the default, contradicting its own later paragraph (correct)
that `±Infinity` disables instead. And two agent.controller.test.ts test
names still described that superseded rule ("non-finite falls back to the
default") even though their bodies never actually asserted the NaN/Infinity
distinction — sanitizeTimeoutMs's own unit test does that. Renamed both to
describe what they actually check.
Cancelling the source before rethrowing was right, but awaiting it was not: a source whose cancel() never settles swallows the consumer's own error, and the read stays pending forever instead of rejecting with what the hook threw. Reproduced with a cancel() returning a promise that never resolves — the read was still pending after 400ms with cancel() invoked once. The regression test asserts the hook's exact error arrives without waiting on cancel(); it fails against the awaited version.
loookashow
force-pushed
the
feat/http-adapter
branch
from
September 23, 2026 13:25
3c4dc96 to
2cf83b3
Compare
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.
Adds
@loquix/adapter-httpand fixes a timeout defect in@loquix/corethat the new package exposed.@loquix/adapter-http0.1.0AgentProviderhas existed since the first release with no implementation to point at, so every integration started by hand-writing streaming, cancellation and error handling. This package covers the shape almost everyone needs: a POST to your own backend returning server-sent events, newline-delimited JSON, or plain text.Zero runtime dependencies.
@loquix/coreis a types-only peer, declared as a plain range rather than the workspace protocol — with apeerDependenciesedge, changesets'shouldBumpMajorwalks a package to 1.0.0 on any non-patch release of its peer, regardless of the range, unlessonlyUpdatePeerDependentsWhenOutOfRangeis set. That flag is now set; without it this package's first published version would have been 1.0.0.72 tests in Chromium and WebKit, 98% coverage.
@loquix/core0.6.0 — ships to existing installsAgentController.sendTimeout(default 60000) was composed viaAbortSignal.timeoutinto the signal handed toprovider.send(). A provider that forwards that signal tofetch— the correct thing to do, and what the new adapter does — had the timer keep running after the response stream was returned. Any answer that streamed for longer than sixty seconds in total was cut off mid-sentence, moved toerror, and never appended tomessages.The controller's own doc comment says the timeout bounds the wait for the stream, so the implementation contradicted its stated intent. There was no test for
sendTimeoutanywhere in core, which is how it survived.sendTimeoutnow bounds only the wait forprovider.send()to resolve. A newstreamIdleTimeout(default 60000,0disables) aborts on silence instead of duration, resetting per chunk and exempt while paused — total duration punishes a long healthy answer, silence does not.Upgrade note: if you set
sendTimeout: 0to disable timeouts, setstreamIdleTimeout: 0as well. It is a separate option and defaults to on.How this was verified
Every behavioural claim here was produced by running code, not by reading it. Three models reviewed the branch and each found defects the others missed; the most expensive one — the
sendTimeoutcut-off — was found only after four review passes had already gone by.Six separate times a fix on this branch introduced a defect of its own: a
tee()that would have hung the cancellation tests, a transport-mismatch check that false-positived on valid responses twice, a timer that survivedabort(), a timer-id guard that disarmed the next send, and a cancel-before-rethrow that swallowed the consumer's own error when awaited. Each was caught by an executed repro and each has a regression test proved to fail without its fix.Not in this PR
The first npm publish needs a human: OIDC trusted publishing is configured per package and cannot be attached to a name that does not exist yet. See
planning/2026-09-22-handoff.mdfor the sequence.Deferred to 0.1.x, none of which touches a frozen surface: two socket-release paths, an uncancelled discarded stream, and idle liveness measured in text chunks rather than transport frames, so SSE keep-alive comments do not reset it.