From 3b424332c9e309f8cee3e5cfaee739ccef989b79 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 01:40:13 +0000 Subject: [PATCH 1/2] fix(client,rest): state SaveReportInput's requirements at the reports.save door MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `IReportService.saveReport` takes a `SaveReportInput`, on which `name`, `object` and `query` are all required. Nothing on the path said so: the SDK method declared its parameter `any` and the route forwarded `req.body ?? {}` unchecked, so the requirement held only as far as each reports implementation chose to re-derive it privately. - `client.reports.save` now takes `SaveReportInput` instead of `any`. - `POST /api/v1/reports` refuses a body missing any of the three required keys, and a `query` that is not a `ReportQuery` envelope, with 400 / VALIDATION_FAILED — ordered after the existing 501 for an unmounted service. The query-less literal that `client.test.ts` had been constructing invisibly is preserved verbatim and becomes a `@ts-expect-error` pin asserting the refusal. The REST pass-through test is re-driven through a door-valid body so it keeps pinning the service-raised VALIDATION_FAILED mapping instead of going vacuous. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- ...reports-save-input-contract-at-the-door.md | 46 ++++++++++++ packages/client/src/client.test.ts | 30 +++++++- packages/client/src/index.ts | 14 +++- packages/rest/src/rest-server.ts | 37 ++++++++++ packages/rest/src/rest.test.ts | 70 ++++++++++++++++++- 5 files changed, 190 insertions(+), 7 deletions(-) create mode 100644 .changeset/reports-save-input-contract-at-the-door.md diff --git a/.changeset/reports-save-input-contract-at-the-door.md b/.changeset/reports-save-input-contract-at-the-door.md new file mode 100644 index 0000000000..e779eb052a --- /dev/null +++ b/.changeset/reports-save-input-contract-at-the-door.md @@ -0,0 +1,46 @@ +--- +"@objectstack/client": minor +"@objectstack/rest": minor +--- + +fix(client,rest): state `SaveReportInput`'s requirements at the `reports.save` door (#11926) + +**BREAKING** accept-set narrowing on `POST /api/v1/reports` and on the +`client.reports.save` parameter type, shipped as `minor` under the repo's +launch-window convention for breaking changes. + +`IReportService.saveReport` takes a `SaveReportInput`, on which `name`, `object` +and `query` are all required. Nothing on the path said so. The SDK method +declared its parameter `any`, and the route forwarded `req.body ?? {}` straight +through, so the requirement held only as far as each reports implementation +chose to re-derive it privately — the bundled `@objectstack/plugin-reports` does +re-derive all three, but a third-party implementation need not, and a caller +could not tell which one it was talking to. This is the ADR-0078 +declared-but-unenforced shape arriving at an authoring surface: the producer +accepted off-spec input and handed it to a service that requires more. + +Both halves now state the contract: + +- **`client.reports.save(report)`** takes `SaveReportInput` instead of `any`. + Omitting `query` (or `name`, or `object`) is now a compile error at the call + site rather than a surprise from whichever implementation is mounted. The SDK + remains a transport and adds no runtime validation — it is not a second + validator. +- **`POST /api/v1/reports`** refuses a body missing any of the three required + keys, and a `query` that is not a `ReportQuery` envelope (a scalar or an + array), with `400` / `VALIDATION_FAILED` — the same envelope the route + already produced for a service-raised validation error (ADR-0112). A + JavaScript or `curl` caller that never sees the TypeScript type is refused + too. The refusal is ordered **after** the existing `501` for an unmounted + reports service: "no reports service on this deployment" is a deployment fact + and outranks anything about the body. An empty `query: {}` stays legal — + every field on `ReportQuery` is optional — and is pinned as such. + +**Migration.** A caller that omitted `query` was already relying on +implementation-specific behaviour; supply the `ReportQuery` envelope the report +should run (`{}` for "no filters"). Callers already sending a complete +definition are unaffected, and the bundled reports implementation already +refused all three omissions, so no deployment running it changes behaviour — +only the layer that produces the refusal moves, from the service to the door. + + diff --git a/packages/client/src/client.test.ts b/packages/client/src/client.test.ts index e2d0314f5b..72ca94c7fd 100644 --- a/packages/client/src/client.test.ts +++ b/packages/client/src/client.test.ts @@ -375,11 +375,37 @@ describe('Reports namespace (#3587 gap closure)', () => { it('reports.save pins POST /reports', async () => { const { client, fetchMock } = createMockClient({ id: 'r1' }); - await client.reports.save({ name: 'Pipeline', object: 'lead' }); + await client.reports.save({ name: 'Pipeline', object: 'lead', query: { fields: ['id'] } }); const [url, init] = fetchMock.mock.calls[0]; expect(String(url)).toBe('http://localhost:3000/api/v1/reports'); expect(init.method).toBe('POST'); - expect(JSON.parse(init.body)).toEqual({ name: 'Pipeline', object: 'lead' }); + expect(JSON.parse(init.body)).toEqual({ name: 'Pipeline', object: 'lead', query: { fields: ['id'] } }); + }); + + // [#11926] The literal below is the ORIGINAL fixture of the test above, + // preserved verbatim rather than repaired. For as long as `reports.save` + // took `any` it sat there constructing an input the service contract + // REFUSES — `SaveReportInput.query` is required — against a mock transport + // that never reaches a service, so no run could ever have failed on it. It + // is evidence, and giving it a `query` would have silenced the evidence + // without closing anything. So it moves here, and the compiler asserts the + // refusal instead. + // + // This is a bidirectional pin, not a comment. `client.test.ts` is compiled + // by `tsconfig.test.json` — named by this package's `typecheck` script — + // and holds no `test-typecheck-debt.json` entry, so an unlisted file must + // have zero errors. Widen the parameter back to `any` and the directive + // below stops matching an error: tsc reds with TS2578, "unused + // '@ts-expect-error' directive". It cannot rot into a phantom check. + it('[#11926] reports.save refuses a query-less report at the type level', async () => { + const { client, fetchMock } = createMockClient({ id: 'r1' }); + // @ts-expect-error — `query` is required by `SaveReportInput`. + await client.reports.save({ name: 'Pipeline', object: 'lead' }); + // The SDK is a transport, not a second validator: the request still + // goes out unaltered. The refusal ON THE WIRE belongs to the route and + // is pinned in packages/rest/src/rest.test.ts — see + // 'POST /reports refuses a body the service contract requires more of'. + expect(JSON.parse(fetchMock.mock.calls[0][1].body)).toEqual({ name: 'Pipeline', object: 'lead' }); }); it('reports.get / delete pin /reports/:id and delete tolerates 204', async () => { diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 8f8dc38d3f..073f14e578 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -104,6 +104,7 @@ import type { RemoteTable, ReportRunResult, ReportSchedule, + SaveReportInput, SavedReport, SchemaValidationReport, ScreenSpec, @@ -4383,8 +4384,17 @@ export class ObjectStackClient { return Array.isArray(body) ? body : (body?.data ?? []); }, - /** Create or update a saved report definition. 400 [VALIDATION_FAILED] on a bad spec. */ - save: async (report: any): Promise => { + /** + * Create or update a saved report definition. + * + * [#11926] The parameter is the service contract's own `SaveReportInput`, + * not `any`: `name`, `object` and `query` are required, and omitting one is + * a compile error here rather than a surprise from whichever reports + * implementation the deployment mounts. The wire refusal is the route's — + * `POST /reports` answers 400 [VALIDATION_FAILED] for the same three keys, + * so a JavaScript caller that never sees this type is refused too. + */ + save: async (report: SaveReportInput): Promise => { const res = await this.fetch(`${this.baseUrl}/api/v1/reports`, { method: 'POST', body: JSON.stringify(report ?? {}), diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 8ac62c8093..dbbf65ca84 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -10227,6 +10227,40 @@ export class RestServer { code: 'NOT_IMPLEMENTED', message: 'Reports service is not configured on this deployment', }); + // [#11926] The door states the contract for `POST /reports` below. + // `IReportService.saveReport` takes a `SaveReportInput` + // (`packages/spec/src/contracts/report-service.ts`), on which `name`, + // `object` and `query` are all REQUIRED — but an HTTP body is untyped, + // so forwarding it unchecked handed the service a value merely CLAIMED + // to be a `SaveReportInput`. That left the requirement for every + // implementation to re-derive privately: the bundled + // `@objectstack/plugin-reports` does re-derive it, a third-party one + // need not, and a caller could not tell which one it was talking to. + // Refusing here makes the contract true for every implementation, in + // the same envelope `handleValidation` already produces (400 / + // VALIDATION_FAILED, ADR-0112). + const refuseIncompleteReport = (res: any, body: any): boolean => { + const missing = (['name', 'object', 'query'] as const) + .filter((field) => body?.[field] === undefined || body?.[field] === null); + if (missing.length > 0) { + res.status(400).json({ + code: 'VALIDATION_FAILED', + error: `${missing.join(', ')} ${missing.length === 1 ? 'is' : 'are'} required`, + }); + return true; + } + // `query` is a `ReportQuery` envelope — never a scalar and never a + // list. A string here is the shape an authoring mistake actually + // takes, and it reaches storage as a stringified scalar otherwise. + if (typeof body.query !== 'object' || Array.isArray(body.query)) { + res.status(400).json({ + code: 'VALIDATION_FAILED', + error: 'query must be a ReportQuery object', + }); + return true; + } + return false; + }; const handleValidation = (res: any, err: any): boolean => { const msg = String(err?.message ?? err ?? ''); if (msg.startsWith('VALIDATION_FAILED')) { @@ -10279,6 +10313,9 @@ export class RestServer { if (this.enforceAuth(req, res, context)) return; const svc = await resolveService(environmentId); if (!svc) return respond501(res); + // AFTER the 501 on purpose: "no reports service is mounted" + // is a deployment fact and outranks anything about the body. + if (refuseIncompleteReport(res, req.body ?? {})) return; try { const row = await svc.saveReport(req.body ?? {}, context ?? {}); res.status(201).json(row); diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index b9bd0287c8..3d9edb7cd3 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -1605,16 +1605,80 @@ describe('RestServer', () => { expect(saveReport).toHaveBeenCalled(); }); - it('POST /reports surfaces VALIDATION_FAILED as 400', async () => { - const saveReport = vi.fn(async () => { throw new Error('VALIDATION_FAILED: name is required'); }); + // [#11926] This pin's subject is the PASS-THROUGH: a VALIDATION_FAILED + // raised by the SERVICE is mapped to 400 by `handleValidation`. It used to + // drive `body: {}`, which the route now refuses at the door before the + // service is ever consulted — the assertions below would still have gone + // green, but on the door's refusal rather than the service's, pinning + // nothing. So the body is now one the door ACCEPTS and the double throws + // for a reason only a service can have, and `saveReport` is asserted to + // have actually been called so this cannot quietly go vacuous again. + it('POST /reports surfaces a service-raised VALIDATION_FAILED as 400', async () => { + const saveReport = vi.fn(async () => { throw new Error('VALIDATION_FAILED: format must be one of csv, json, html_table'); }); const rest = makeRest(async () => ({ saveReport })); const { save } = getReportRoutes(rest); const res = { json: vi.fn(), status: vi.fn().mockReturnThis() }; - await save!.handler({ body: {} } as any, res as any); + await save!.handler({ body: { name: 'X', object: 'lead', query: {}, format: 'pdf' } } as any, res as any); + expect(saveReport).toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(400); expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ code: 'VALIDATION_FAILED' })); }); + // [#11926] The door itself. `IReportService.saveReport` takes a + // `SaveReportInput`, on which `name`, `object` and `query` are required; + // the route used to forward `req.body ?? {}` unchecked, so the requirement + // held only as far as each implementation chose to re-derive it. These pin + // that the ROUTE states it — note `saveReport` is asserted NOT to have been + // called, which is the whole difference from the pass-through pin above. + it('POST /reports refuses a body the service contract requires more of', async () => { + const cases: Array<{ label: string; body: any }> = [ + { label: 'no query', body: { name: 'X', object: 'lead' } }, + { label: 'null query', body: { name: 'X', object: 'lead', query: null } }, + { label: 'no object', body: { name: 'X', query: {} } }, + { label: 'no name', body: { object: 'lead', query: {} } }, + { label: 'empty body', body: {} }, + ]; + for (const { label, body } of cases) { + const saveReport = vi.fn(async (input: any) => ({ id: 'rpt_new', ...input })); + const rest = makeRest(async () => ({ saveReport })); + const { save } = getReportRoutes(rest); + const res = { json: vi.fn(), status: vi.fn().mockReturnThis() }; + await save!.handler({ body } as any, res as any); + expect(res.status, label).toHaveBeenCalledWith(400); + expect(res.json, label).toHaveBeenCalledWith(expect.objectContaining({ code: 'VALIDATION_FAILED' })); + expect(saveReport, label).not.toHaveBeenCalled(); + } + }); + + // [#11926] `query` is a `ReportQuery` envelope. A scalar or a list is the + // shape an authoring mistake actually takes, and it is present, so the + // required-key check above cannot catch it. + it('POST /reports refuses a query that is not a ReportQuery envelope', async () => { + for (const query of ['object=lead', 42, true, [{ field: 'id' }]]) { + const saveReport = vi.fn(async (input: any) => ({ id: 'rpt_new', ...input })); + const rest = makeRest(async () => ({ saveReport })); + const { save } = getReportRoutes(rest); + const res = { json: vi.fn(), status: vi.fn().mockReturnThis() }; + await save!.handler({ body: { name: 'X', object: 'lead', query } } as any, res as any); + expect(res.status, JSON.stringify(query)).toHaveBeenCalledWith(400); + expect(saveReport, JSON.stringify(query)).not.toHaveBeenCalled(); + } + }); + + // [#11926] An empty `ReportQuery` is LEGAL — every field on it is optional + // — so the door must not over-refuse. Without this, tightening `query` to + // "present and an object" could drift into "present and non-empty" with no + // test noticing. + it('POST /reports accepts an empty query object', async () => { + const saveReport = vi.fn(async (input: any) => ({ id: 'rpt_new', ...input })); + const rest = makeRest(async () => ({ saveReport })); + const { save } = getReportRoutes(rest); + const res = { json: vi.fn(), status: vi.fn().mockReturnThis() }; + await save!.handler({ body: { name: 'X', object: 'lead', query: {} } } as any, res as any); + expect(saveReport).toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(201); + }); + it('GET /reports/:id returns 404 when missing', async () => { const getReport = vi.fn(async () => null); const rest = makeRest(async () => ({ getReport })); From 9d4791c2d6f466cb78f732568e119ee1b936fb6f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 01:47:00 +0000 Subject: [PATCH 2/2] refactor(rest): raise the reports door refusal through handleValidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The door check wrote its own 400 body. That put a second VALIDATION_FAILED construction site on one route — so the same refusal could reach a client in two different envelopes depending on whether the door or the service raised it — and added two non-conforming bodies to the `check:route-envelope` ratchet, which only ticks down (stringError 46 vs 44, siblingCode 71 vs 69). It now throws `VALIDATION_FAILED: …` from inside the existing try, so the route's single `handleValidation` builds the body exactly as it already did for a service-raised refusal. No new response body; the ratchet is untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- packages/rest/src/rest-server.ts | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index dbbf65ca84..7b977048b1 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -10239,27 +10239,27 @@ export class RestServer { // Refusing here makes the contract true for every implementation, in // the same envelope `handleValidation` already produces (400 / // VALIDATION_FAILED, ADR-0112). - const refuseIncompleteReport = (res: any, body: any): boolean => { + // It THROWS rather than writing a response, and that is the design, not + // a detour: `handleValidation` below is this surface's single place for + // building a VALIDATION_FAILED body. Writing a second one here would + // make one route answer the same refusal in two different envelopes — + // and would add a non-conforming body to the `check:route-envelope` + // ratchet, which only ticks down. Raised before `saveReport` is called, + // so the door refuses rather than the service. + const assertSaveReportInput = (body: any): void => { const missing = (['name', 'object', 'query'] as const) .filter((field) => body?.[field] === undefined || body?.[field] === null); if (missing.length > 0) { - res.status(400).json({ - code: 'VALIDATION_FAILED', - error: `${missing.join(', ')} ${missing.length === 1 ? 'is' : 'are'} required`, - }); - return true; + throw new Error( + `VALIDATION_FAILED: ${missing.join(', ')} ${missing.length === 1 ? 'is' : 'are'} required`, + ); } // `query` is a `ReportQuery` envelope — never a scalar and never a // list. A string here is the shape an authoring mistake actually // takes, and it reaches storage as a stringified scalar otherwise. if (typeof body.query !== 'object' || Array.isArray(body.query)) { - res.status(400).json({ - code: 'VALIDATION_FAILED', - error: 'query must be a ReportQuery object', - }); - return true; + throw new Error('VALIDATION_FAILED: query must be a ReportQuery object'); } - return false; }; const handleValidation = (res: any, err: any): boolean => { const msg = String(err?.message ?? err ?? ''); @@ -10313,10 +10313,12 @@ export class RestServer { if (this.enforceAuth(req, res, context)) return; const svc = await resolveService(environmentId); if (!svc) return respond501(res); - // AFTER the 501 on purpose: "no reports service is mounted" - // is a deployment fact and outranks anything about the body. - if (refuseIncompleteReport(res, req.body ?? {})) return; try { + // AFTER the 501 on purpose: "no reports service is + // mounted" is a deployment fact and outranks anything + // about the body. Inside the try so the refusal reaches + // `handleValidation` like any other VALIDATION_FAILED. + assertSaveReportInput(req.body ?? {}); const row = await svc.saveReport(req.body ?? {}, context ?? {}); res.status(201).json(row); } catch (err: any) {