Skip to content

Commit 7ad57e1

Browse files
os-trumpclaude
andauthored
fix(rest): state the four IHttpResponse members once, in a test-layer builder (#14358)
* fix(rest): state the four IHttpResponse members once, in a test-layer builder The two remaining ledgered TS2345 in `packages/rest` are hand-built `IHttpResponse` literals supplying `json` and `status` where the contract requires `json`, `send`, `status` and `header`. They were never absent — tsc reports at most one argument-assignability error per call expression, so the non-conforming request literals at the same two sites masked them until the request half was repaired. `src/http-response-test-builder.ts` states the four members once. Test layer only: nothing in `src/index.ts` reaches it, so tsup never emits it into `dist`. Its required-member set is computed from the contract, so a new required member fails there rather than leaving a helper that still compiles and lies. `write`/`end` are deliberately absent — both are feature-detected, and supplying them would route streaming handlers down their streaming path with no test edited. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza * chore(rest): empty the test-typecheck ledger and rewrite its authored note `gen:test-typecheck-debt` deleted the last entry: the test layer measures 0 files / 0 errors / 0 signatures. `_note` is the authored field the generator preserves verbatim and never writes, so it is hand-maintained by design; it claimed #13454 "holds the one entry that remains", which is no longer true. Rewritten to record the end state, keep the masking mechanism (not rest-specific), name where each of the three classes is now stated once, and say what an empty ledger does NOT claim: the `as any` call sites and the three `makeRes()` files are green because nothing checks them, not because they conform. Re-running the generator over the rewritten note is byte-identical. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e808890 commit 7ad57e1

3 files changed

Lines changed: 147 additions & 8 deletions

File tree

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* The one place this package builds an `IHttpResponse` for a test (#13454).
5+
*
6+
* Test layer only — nothing in `src/index.ts` reaches it, so tsup (entry:
7+
* `src/index.ts`) never emits it into `dist` and it is not published. Same
8+
* placement, and for the same reason, as `src/http-request-test-builder.ts`
9+
* and `src/xlsx-test-loader.ts`.
10+
*
11+
* ## The defect, stated once
12+
*
13+
* `IHttpResponse` (`packages/spec/src/contracts/http-server.ts`) declares FOUR
14+
* required members — `json`, `send`, `status`, `header` — and a route handler's
15+
* SECOND parameter IS that interface. So a test that hands a handler an object
16+
* literal owes all four. This package hands it two:
17+
*
18+
* ```
19+
* src/rest.test.ts(2065,7): error TS2345: Argument of type
20+
* '{ json: Mock<Procedure>; status: Mock<Procedure>; }'
21+
* is not assignable to parameter of type 'IHttpResponse'.
22+
* Type '{ json: Mock<Procedure>; status: Mock<Procedure>; }' is missing the
23+
* following properties from type 'IHttpResponse': send, header
24+
* ```
25+
*
26+
* ⚠️ Those two errors were never absent — they were MASKED. `tsc` reports at
27+
* most one argument-assignability error per call expression, so while argument
28+
* 1 was a non-conforming request literal it hid argument 2 entirely; repairing
29+
* the request half (#13377) is what made them visible, at the very same two
30+
* sites and with the per-file ledger count unmoved at 2. That mechanism has not
31+
* gone away and is why the repair here is one builder rather than two edits:
32+
* the decision about what a mock response IS belongs in a place a reader can
33+
* find, not spread across the 28 literals of this shape that this file holds.
34+
*
35+
* ## Why each decision is the decision
36+
*
37+
* **All four required members are present, always — including the two the
38+
* literals omit.** `send` and `header` are absent from those literals not
39+
* because a fixture means "this response cannot do that", but because whoever
40+
* wrote it stopped at the members the handler happened to call. Supplying them
41+
* is strictly better-formed than omitting them: had a handler reached
42+
* `res.header(...)`, the literal would have THROWN, and the test would have
43+
* failed for a reason unrelated to the thing it names. Same argument as
44+
* `headers: {}` in the request builder.
45+
*
46+
* **`status` and `header` return the double, and are TYPED as returning the
47+
* interface.** The contract declares `status(code: number): IHttpResponse`, and
48+
* that return is load-bearing rather than decorative — it is what makes
49+
* `res.status(404).json(...)` chain, which is how `src/rest-server.ts` writes
50+
* essentially every response it sends (138 `.status(` call sites). The literals
51+
* spell this `vi.fn().mockReturnThis()`, which is right at runtime only while
52+
* the member is invoked as a method on the response, and is typed
53+
* `Mock<Procedure>` — returning `any` — either way. Returning `double`
54+
* explicitly is true under any call shape, and it means a chained `.json(...)`
55+
* lands on the SAME spy the test asserts on whether the handler chained or not.
56+
*
57+
* **`write` and `end` are deliberately ABSENT, and have no default at all.**
58+
* This is the `remoteAddress` argument of the request builder, and it is the
59+
* one default here that could silently change what a test measures. Both are
60+
* optional AND feature-detected: the contract on `write` (#3607, ADR-0076
61+
* OQ#10) says consumers emitting streaming results "feature-detect this member
62+
* and fall back to buffered `send()` when absent". A double that supplied them
63+
* by default would therefore route every streaming handler down its streaming
64+
* path — no test edited, every such test now measuring something else. Absent
65+
* is both legal and true; a test that is ABOUT streaming says so, by stating
66+
* them.
67+
*
68+
* **The double records through its spies and nowhere else.** This is the design
69+
* question the card raised — what a mock response records, and what a test may
70+
* assert on it — and the answer is that it already has exactly one record:
71+
* `res.status.mock.calls` IS the status record, `res.json.mock.calls` IS the
72+
* body record, and both are what the existing assertions read
73+
* (`expect(res.json).toHaveBeenCalledWith(...)`,
74+
* `res.json.mock.calls.at(-1)![0]`). A mirrored `statusCode` / `body` pair
75+
* would be a SECOND, derived record of the same call, and the two can disagree:
76+
* a mirror keeps only the last status where `mock.calls` keeps every one, and
77+
* `mockClear()` empties one and not the other. One record, not two.
78+
*
79+
* ⚠️ Three other files in this package — `analytics-dataset-dimension-gate`,
80+
* `meta-public-book-grant`, `rest-batch-size-cap` — assert on a mirrored
81+
* `res.statusCode` / `res.body` built by a local `makeRes()` typed `any`. They
82+
* are green for the forbidden reason rather than conforming, but they cannot
83+
* adopt this builder by substitution: converting those reads into spy reads
84+
* CHANGES the assertion, so it is a decision of its own rather than a mechanical
85+
* edit. Recorded here so the omission is not read as an oversight.
86+
*
87+
* **It takes no parameters.** `httpRequestForRoute` needs them because a
88+
* request carries fixture DATA that differs per test. A response double carries
89+
* none — it is a pure recorder — so there is nothing per-site to state, and an
90+
* options bag would be surface with no caller.
91+
*/
92+
93+
import { vi, type Mock } from 'vitest';
94+
import type { RouteHandler } from '@objectstack/core';
95+
96+
/**
97+
* The response type a handler is actually handed, read off the handler's own
98+
* signature instead of spelled by hand — the discipline `xlsx-test-loader.ts`
99+
* applies to its dependency and `http-request-test-builder.ts` applies to the
100+
* request half. It resolves to `IHttpResponse`.
101+
*/
102+
type HandlerResponse = Parameters<RouteHandler>[1];
103+
104+
/** Any callable — `Mock<T>` demands `T` be one, and this states it locally. */
105+
type AnyProcedure = (...args: any[]) => any;
106+
107+
/**
108+
* The members the contract REQUIRES, computed from the contract rather than
109+
* listed. A required member added to `IHttpResponse` therefore fails HERE,
110+
* loudly, in one file — the double below stops satisfying its own type — rather
111+
* than leaving a helper that still compiles and lies.
112+
*/
113+
type RequiredResponseMember = {
114+
[K in keyof HandlerResponse]-?: undefined extends HandlerResponse[K] ? never : K;
115+
}[keyof HandlerResponse];
116+
117+
/**
118+
* A complete `IHttpResponse` whose required members are vitest spies, so a test
119+
* can pass it to a handler AND assert on what the handler did with it. The
120+
* optional `write` / `end` stay optional and absent — see the header.
121+
*/
122+
export type HttpResponseTestDouble = HandlerResponse & {
123+
[K in RequiredResponseMember]: Mock<Extract<HandlerResponse[K], AnyProcedure>>;
124+
};
125+
126+
/**
127+
* Build a conforming response double for a handler under test.
128+
*
129+
* @returns a response satisfying every required member of `IHttpResponse`,
130+
* recording each call on the corresponding spy, with `status` and
131+
* `header` returning the same double so handler chains land on the
132+
* spies the test reads.
133+
*/
134+
export function httpResponseTestDouble(): HttpResponseTestDouble {
135+
const double: HttpResponseTestDouble = {
136+
json: vi.fn<HandlerResponse['json']>(),
137+
send: vi.fn<HandlerResponse['send']>(),
138+
status: vi.fn<HandlerResponse['status']>(() => double),
139+
header: vi.fn<HandlerResponse['header']>(() => double),
140+
};
141+
return double;
142+
}

packages/rest/src/rest.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { createRestApiPlugin } from './rest-api-plugin';
77
import type { RestApiPluginConfig } from './rest-api-plugin';
88
import { loadXlsxWorkbook } from './xlsx-test-loader.js';
99
import { httpRequestForRoute } from './http-request-test-builder.js';
10+
import { httpResponseTestDouble } from './http-response-test-builder.js';
1011

1112
// ---------------------------------------------------------------------------
1213
// Mocks & Helpers
@@ -2059,7 +2060,7 @@ describe('RestServer project-scoped routing', () => {
20592060
.find(r => r.path === '/api/v1/environments/:environmentId/data/:object' && r.method === 'GET');
20602061
expect(listRoute).toBeDefined();
20612062

2062-
const res = { json: vi.fn(), status: vi.fn().mockReturnThis() };
2063+
const res = httpResponseTestDouble();
20632064
await listRoute!.handler(
20642065
httpRequestForRoute(listRoute!, { params: { environmentId: 'proj-123', object: 'task' } }),
20652066
res,
@@ -2084,7 +2085,7 @@ describe('RestServer project-scoped routing', () => {
20842085
.find(r => r.path === '/api/v1/data/:object' && r.method === 'GET');
20852086
expect(unscoped).toBeDefined();
20862087

2087-
const res = { json: vi.fn(), status: vi.fn().mockReturnThis() };
2088+
const res = httpResponseTestDouble();
20882089
await unscoped!.handler(
20892090
httpRequestForRoute(unscoped!, { params: { object: 'task' } }),
20902091
res,
Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,5 @@
11
{
22
"_comment": "Per-file tsc error debt of the @objectstack/rest TEST layer (#5286). `tsconfig.test.json` compiles `src/**/*.test.ts` — which `tsconfig.json` excludes and therefore no gate ever read — and every file below still carries errors from before that gate existed. THIS FIELD IS GENERATED: every regeneration rewrites it from scripts/check-test-typecheck.mts, and the EXACT ratchet below requires a regeneration on every repair — so an edit made here is gone by the next one. Anything true of THIS package goes in the sibling `_note` field, which is authored, is preserved verbatim, and is never written by the generator (#12624). This comment states NO cause for the errors, deliberately: the classes differ per package and per file, they move as the debt is paid down, and a cause written here is rewritten verbatim into every ledger by every regeneration — so it outlives its own repair and cannot be corrected in the file where it is read. Measure instead, before repairing anything: `tsc --noEmit --pretty false -p tsconfig.test.json` in the package prints the real classes with their TS codes. Each entry maps a file to its per-SIGNATURE error counts, never to a bare total (#13470): a signature is the TS code plus the diagnostic message with structural type blobs collapsed, and it carries NO line or column — so the pin survives edits that move code around, and only stops matching when the error itself becomes a different error. EXACT ratchet, judged by re-running tsc: a file that gains errors is red, a file that loses them is red until its number is re-recorded, a file that reaches zero is red until its entry is deleted, a signature that ARRIVES or VANISHES is red even when the file total is unchanged, and a file NOT listed here may have no errors at all. Regenerate with: pnpm --filter @objectstack/rest gen:test-typecheck-debt",
3-
"_note": "Everything still recorded here is held by its own card, and none of it is an annotation repair. #13454 holds the one entry that remains: src/rest.test.ts's two hand-built IHttpResponse literals, which omit the send and header members that interface requires and whose status is not typed as returning the interface, so res.status(...).json(...) does not chain. WARNING, and the reason this note exists: the count did NOT move when #13377 landed, but the errors underneath it were replaced wholesale. This file was recorded at 2 before that card and measures 2 after, and neither of the two is the same error. tsc reports at most ONE argument-assignability error per call expression, so the request literals #13377 removed had been masking these response literals at the very same two call sites. That blindness is now CLOSED (#13470): an entry is no longer one integer but a map of normalized error SIGNATURE to count, so this file pins WHICH errors it carries and not merely how many, and the very substitution described above now reds with the signature that ARRIVED and the one that VANISHED both named. The history is kept because the mechanism is not rest-specific and has not gone away: tsc still reports at most one argument error per call, so a repair here can still uncover a different error at the same site. What changed is that the signature keys below - never their sum - are what say so. The request literals #13377 held are gone from this package: the five members IHttpRequest requires are stated once, in src/http-request-test-builder.ts, and a request built there takes its method and its path from the route under test instead of from a default, so the two cannot disagree. The exceljs call that #13378 held is likewise no longer here: that dependency declares its own module-local Buffer, which shadows Node's inside every exceljs signature, so no Node Buffer can be passed to Workbook.xlsx.load - the assertion that costs is stated once, in src/xlsx-test-loader.ts, and every xlsx-reading test in this package goes through it.",
4-
"entries": {
5-
"src/rest.test.ts": {
6-
"TS2345: Argument of type '…' is not assignable to parameter of type 'IHttpResponse'.": 2
7-
}
8-
}
3+
"_note": "This package's test layer is at ZERO: `entries` is empty, and that is the end state the EXACT ratchet was built for rather than a gap in it. Nothing in this layer is exempt any more, so an error arriving in any src/**/*.test.ts of this package is red on the PR that introduces it, with no entry to widen and nothing to re-record. #13454 closed the last one. The history below is kept because the mechanism is not rest-specific and has not gone away. tsc reports at most ONE argument-assignability error per call expression, so a repair can uncover a different error at the very same site, and a per-file COUNT can hold while the errors underneath it are replaced wholesale: this file was recorded at 2 before #13377 and measured 2 after, and neither of the two was the same error - the request literals that card removed had been masking the response literals beside them. That blindness is closed (#13470): an entry is a map of normalized error SIGNATURE to count, so such a substitution reds with the signature that ARRIVED and the one that VANISHED both named. The count was never the pin; the signature keys were. None of the three classes recorded here was paid down by an annotation, and each ended as one statement a reader can find: the five members IHttpRequest requires are stated in src/http-request-test-builder.ts (#13377), where a request takes its method and its path from the route under test so the two cannot disagree; the four members IHttpResponse requires are stated in src/http-response-test-builder.ts (#13454), whose required-member set is computed from the contract so a new required member fails there rather than in a fixture; and exceljs's module-local Buffer, which shadows Node's inside every exceljs signature, is asserted around once in src/xlsx-test-loader.ts (#13378). What an empty ledger here does NOT say: that every fixture in this package is well-typed. Most `.handler(` call sites still cast their arguments `as any`, and three files (analytics-dataset-dimension-gate, meta-public-book-grant, rest-batch-size-cap) build their response through a local makeRes() typed `any` and assert on a mirrored res.statusCode/res.body. Those are green because nothing is checked there, not because they conform. This ledger can only ever speak for the CHECKED layer.",
4+
"entries": {}
95
}

0 commit comments

Comments
 (0)