Merge pull request #159 from script-development/war-room/wr0049-fs-http-r2-r3-hardening

fs-http: document + pin the middleware contract (R2/R3, no runtime change)

Author: Goosterhof

packages/http/README.md

@@ -70,6 +70,18 @@ Every registered middleware body is wrapped in `guarded()` **by default** (ADR-0
 - `registerResponseErrorMiddleware(fn, opts?)` — Returns unregister function
 - `guarded(fn, onError?)` — Manual middleware-body guard; still exported for the `{guard: false}` + manual-wrap case
 
+#### Middleware contract
+
+The behaviours below are the full middleware contract — pinned by `tests/middleware-contract.spec.ts` (and the guard-by-default suite in `tests/http.spec.ts`), so a regression fails CI. Written against fs-http **0.6.0** (guard-by-default, ADR-0037).
+
+- **Guarded by default; opt out per call.** Every registered body is wrapped in `guarded()` internally, so a **synchronous** throw is loud-swallowed (`onMiddlewareError` / `console.error`) and cannot reject a resolved 200 nor mask the real API error. Register with `{guard: false}` to run the raw body unguarded — then a sync throw propagates: it rejects the request on the success paths, and on the error path it replaces (masks) the original `AxiosError`.
+- **Sync-only, fire-and-forget.** The interceptor loops run each middleware synchronously and are **never `await`ed**. A middleware body that returns a Promise is not awaited — a never-resolving body does not block response delivery, and post-`await` work in an async body races the caller's continuation. Keep middleware bodies synchronous; treat any async work as detached side-channel work.
+- **`guarded()` catches synchronous throws only — NOT async rejections.** Because the loop does not await and `guarded()`'s `try`/`catch` is synchronous, a rejected Promise returned by an async-bodied middleware is **not** caught by the guard and does **not** reach `onMiddlewareError`; it surfaces as an unhandled promise rejection (unchanged by ADR-0037). This is the one failure mode guard-by-default does not close — another reason to keep bodies synchronous.
+- **FIFO execution order.** Middleware runs in registration order, deterministically, for both the request and the response paths.
+- **Per-instance scoping.** Each `createHttpService(...)` owns independent middleware stacks. A middleware registered on service A never fires for service B. Pass one service instance around to share a stack; create multiple to isolate them.
+- **Registration is not idempotent.** Registering the same function reference twice fires it twice per request/response. Each registration returns its own `unregister` — call each to remove that registration; one `unregister()` removes only its own copy.
+- **Mutation is visible down the chain; response objects are not reused.** All middleware on one response receive the same response object, so a mutation from an earlier middleware is visible to a later one. Axios constructs a fresh response object per request, so a mutation on one response does not bleed into the next.
+
 ### Utilities
 
 - `isAxiosError<T>(error)` — Type-safe axios error check

packages/http/src/types.ts

@@ -77,8 +77,46 @@ export type HttpService = {
      * to render and `URL.revokeObjectURL(...)` on cleanup.
      */
     previewRequest: (endpoint: string, options?: AxiosRequestConfig) => Promise<AxiosResponse<Blob>>;
+    /**
+     * Register a request middleware. The body runs synchronously in FIFO order
+     * before each request leaves the service. See the shared middleware contract
+     * below — the same invariants govern all three `register*` functions.
+     *
+     * Middleware contract (fs-http 0.6.0, ADR-0037; pinned by
+     * `tests/middleware-contract.spec.ts`):
+     * - **Guarded by default.** The body is wrapped in `guarded()`, so a
+     *   synchronous throw is loud-swallowed (`onMiddlewareError` / `console.error`)
+     *   and cannot reject a resolved 200 nor mask the real API error. Pass
+     *   `{guard: false}` to run the raw body — then a sync throw propagates.
+     * - **Sync-only, fire-and-forget.** The loop never `await`s the body. A
+     *   Promise-returning body is not awaited; `guarded()` catches sync throws
+     *   only — an async rejection escapes as an unhandled rejection. Keep bodies
+     *   synchronous.
+     * - **FIFO, per-instance, not idempotent.** Runs in registration order;
+     *   each service instance owns independent stacks; registering the same
+     *   reference twice fires it twice.
+     * - **Mutation visible down the chain; response objects are not reused.**
+     * @returns an idempotent `unregister` that removes this registration.
+     */
     registerRequestMiddleware: (fn: RequestMiddlewareFunc, opts?: RegisterMiddlewareOptions) => UnregisterMiddleware;
+    /**
+     * Register a response (success-path) middleware. Runs synchronously in FIFO
+     * order on every resolved response, before the caller's `await` resumes. Same
+     * middleware contract as {@link HttpService.registerRequestMiddleware}
+     * (guarded-by-default, sync-only/fire-and-forget, FIFO, per-instance,
+     * not idempotent, mutation-visible, response-not-reused).
+     * @returns an idempotent `unregister` that removes this registration.
+     */
     registerResponseMiddleware: (fn: ResponseMiddlewareFunc, opts?: RegisterMiddlewareOptions) => UnregisterMiddleware;
+    /**
+     * Register a response-error middleware. Runs synchronously in FIFO order for
+     * **axios errors only** (non-axios errors reject without invoking any error
+     * middleware). Guarded by default: with the guard, a throwing body cannot
+     * mask the original `AxiosError`; under `{guard: false}` a sync throw
+     * replaces it. Same middleware contract as
+     * {@link HttpService.registerRequestMiddleware}.
+     * @returns an idempotent `unregister` that removes this registration.
+     */
     registerResponseErrorMiddleware: (
         fn: ResponseErrorMiddlewareFunc,
         opts?: RegisterMiddlewareOptions,

packages/http/tests/guarded.spec.ts

@@ -91,6 +91,25 @@ describe('guarded', () => {
             expect(onError).toHaveBeenCalledWith('string failure');
         });
 
+        it('is synchronous-only — a body returning a rejected promise is NOT caught (no onError)', () => {
+            // Arrange — guarded()'s try/catch is synchronous. A body that returns a
+            // rejected promise (i.e. an `async () => { throw }` body from the loop's
+            // perspective) does NOT throw synchronously, so the catch never fires.
+            // The promise is pre-handled so it never floats as an unhandled rejection
+            // in the runner — this is a leak-safe stand-in for the async body, not a
+            // suppression of the behavior under test.
+            const onError = vi.fn<(error: unknown) => void>();
+            const rejected = Promise.reject(new Error('async body'));
+            rejected.catch(() => {});
+            const wrapped = guarded<string>(() => rejected, onError);
+
+            // Act
+            wrapped('x');
+
+            // Assert — the async rejection is invisible to guarded's sync try/catch.
+            expect(onError).not.toHaveBeenCalled();
+        });
+
         describe('default onError', () => {
             let consoleErrorSpy: ReturnType<typeof vi.spyOn>;
 

packages/http/tests/middleware-contract.spec.ts

@@ -0,0 +1,195 @@
+import type {AxiosResponse} from 'axios';
+
+import axios from 'axios';
+import MockAdapter from 'axios-mock-adapter';
+import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest';
+
+import type {ResponseMiddlewareFunc} from '../src/types';
+
+import {createHttpService} from '../src/index';
+
+// Contract pins for the fs-http middleware mechanism. Each test locks one
+// documented invariant (README §Middleware contract / the register* JSDoc) so a
+// future regression fails CI rather than silently drifting. Written against
+// fs-http 0.6.0 (guard-by-default, ADR-0037): throw-isolation itself is pinned
+// in http.spec.ts §"middleware guarding by default"; this file pins the
+// invariants that survive UNCHANGED across ADR-0037 — ordering, per-instance
+// scoping, non-idempotency, mutation visibility, response-object freshness, and
+// the sync-only / fire-and-forget execution shape (including the one gap
+// guard-by-default does NOT close: async rejections).
+
+const BASE_URL = 'https://api.example.com';
+
+let mock: MockAdapter;
+
+beforeEach(() => {
+    mock = new MockAdapter(axios);
+    mock.onGet(/.*/).reply(200, {ok: true});
+});
+
+afterEach(() => {
+    mock.restore();
+});
+
+describe('fs-http middleware contract', () => {
+    describe('ordering', () => {
+        it('runs response middleware in FIFO registration order', async () => {
+            // Arrange
+            const service = createHttpService(BASE_URL);
+            const order: number[] = [];
+            service.registerResponseMiddleware(() => order.push(1));
+            service.registerResponseMiddleware(() => order.push(2));
+            service.registerResponseMiddleware(() => order.push(3));
+
+            // Act
+            await service.getRequest('/test');
+
+            // Assert — insertion order preserved on the response path (request path
+            // is pinned in http.spec.ts; both share the same iteration shape).
+            expect(order).toEqual([1, 2, 3]);
+        });
+    });
+
+    describe('per-instance scoping', () => {
+        it('scopes middleware to the service instance it was registered on', async () => {
+            // Arrange — two independent services; middleware only on A.
+            const serviceA = createHttpService(BASE_URL);
+            const serviceB = createHttpService(BASE_URL);
+            const onA = vi.fn();
+            serviceA.registerResponseMiddleware(onA);
+
+            // Act — a request through B must not touch A's stack.
+            await serviceB.getRequest('/test');
+
+            // Assert
+            expect(onA).not.toHaveBeenCalled();
+
+            // Act — a request through A does fire A's middleware.
+            await serviceA.getRequest('/test');
+
+            // Assert
+            expect(onA).toHaveBeenCalledTimes(1);
+        });
+    });
+
+    describe('non-idempotent registration', () => {
+        it('fires a body once per registration — the same reference registered twice fires twice', async () => {
+            // Arrange — each register* call wraps fn in a fresh guarded() wrapper,
+            // so both registrations are live and both invoke fn.
+            const service = createHttpService(BASE_URL);
+            const fn = vi.fn();
+            const unregisterFirst = service.registerResponseMiddleware(fn);
+            service.registerResponseMiddleware(fn);
+
+            // Act
+            await service.getRequest('/first');
+
+            // Assert — not deduped: two registrations => two calls.
+            expect(fn).toHaveBeenCalledTimes(2);
+
+            // Act — one unregister removes only its own copy; the other stays live.
+            unregisterFirst();
+            fn.mockClear();
+            await service.getRequest('/second');
+
+            // Assert
+            expect(fn).toHaveBeenCalledTimes(1);
+        });
+    });
+
+    describe('mutation semantics', () => {
+        it('makes a mutation from one middleware visible to a later middleware on the same response', async () => {
+            // Arrange — same response object is threaded through the whole chain.
+            const service = createHttpService(BASE_URL);
+            let sawInjected: string | undefined;
+            service.registerResponseMiddleware((response) => {
+                (response as AxiosResponse & {injected?: string}).injected = 'from-A';
+            });
+            service.registerResponseMiddleware((response) => {
+                sawInjected = (response as AxiosResponse & {injected?: string}).injected;
+            });
+
+            // Act
+            await service.getRequest('/test');
+
+            // Assert
+            expect(sawInjected).toBe('from-A');
+        });
+
+        it('does not reuse the response object across responses — a mutation does not bleed into the next request', async () => {
+            // Pins axios's own per-request object-identity guarantee, not an fs-http
+            // invariant — if this ever fails, triage upstream (axios), not src/http.ts.
+            // Arrange
+            const service = createHttpService(BASE_URL);
+            const seen: AxiosResponse[] = [];
+            service.registerResponseMiddleware((response) => {
+                (response as AxiosResponse & {marked?: boolean}).marked = true;
+                seen.push(response);
+            });
+
+            // Act
+            await service.getRequest('/first');
+            await service.getRequest('/second');
+
+            // Assert — axios hands back a fresh response object per request.
+            expect(seen).toHaveLength(2);
+            expect(seen[0]).not.toBe(seen[1]);
+        });
+    });
+
+    describe('execution timing (sync-only, fire-and-forget)', () => {
+        it('completes the synchronous middleware loop before the caller await resumes', async () => {
+            // Arrange
+            const service = createHttpService(BASE_URL);
+            let ranDuringLoop = false;
+            service.registerResponseMiddleware(() => {
+                ranDuringLoop = true;
+            });
+
+            // Act
+            const response = await service.getRequest('/test');
+
+            // Assert — by the time the caller resumes, the loop has run to completion.
+            expect(ranDuringLoop).toBe(true);
+            expect(response.status).toBe(200);
+        });
+
+        it('does not await an async middleware body — a never-resolving body does not block response delivery', async () => {
+            // Arrange — a body that returns a forever-pending promise. If the
+            // interceptor awaited middleware, getRequest would hang; it resolves
+            // because the loop is fire-and-forget. The promise never rejects, so
+            // there is no unhandled rejection.
+            const service = createHttpService(BASE_URL);
+            const neverResolves: ResponseMiddlewareFunc = () => new Promise<void>(() => {});
+            service.registerResponseMiddleware(neverResolves);
+
+            // Act
+            const response = await service.getRequest('/test');
+
+            // Assert
+            expect(response.status).toBe(200);
+        });
+
+        it('does NOT catch an async rejection — guard-by-default is sync-only; the request still resolves', async () => {
+            // Arrange — models an `async () => { throw }` body's rejected RETURN.
+            // guarded()'s try/catch is synchronous, so it never observes this
+            // rejection: onMiddlewareError does not fire and the request resolves.
+            // The rejection is pre-handled so the test runner never sees a floating
+            // unhandled rejection — this is a leak-safe stand-in for the real
+            // async body, not a suppression of the behavior under test.
+            const onMiddlewareError = vi.fn();
+            const service = createHttpService(BASE_URL, {onMiddlewareError});
+            const rejected = Promise.reject(new Error('async middleware boom'));
+            rejected.catch(() => {});
+            const asyncRejecting: ResponseMiddlewareFunc = () => rejected;
+            service.registerResponseMiddleware(asyncRejecting);
+
+            // Act
+            const response = await service.getRequest('/test');
+
+            // Assert — the async rejection is invisible to guard-by-default.
+            expect(response.status).toBe(200);
+            expect(onMiddlewareError).not.toHaveBeenCalled();
+        });
+    });
+});