State SaveReportInput's requirements at the reports.save door (#12421)

* fix(client,rest): state SaveReportInput's requirements at the reports.save door

`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd

* refactor(rest): raise the reports door refusal through handleValidation

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-litant

.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.
+
+<!-- adr-0087: not-required (no-migration-prescription) A validity narrowing over keys that already exist and are already required by the service contract: no key is removed, renamed or re-shaped, so there is no tombstone and nothing mechanical for `objectstack migrate meta` to rewrite. `SaveReportInput` is a cross-package TS contract with no spec schema and no stored-metadata form, so no authored artifact at rest carries the affected shape. What a query-less report definition was *meant* to query is authoring intent no migration entry can supply on an upgrader's behalf; the 400 at the door is the channel that reaches the author, naming the missing keys. Mirrors the disposition of the #11519 and #11842 accept-set narrowings. -->

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 () => {

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<SavedReport> => {
+    /**
+     * 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<SavedReport> => {
         const res = await this.fetch(`${this.baseUrl}/api/v1/reports`, {
             method: 'POST',
             body: JSON.stringify(report ?? {}),

packages/rest/src/rest-server.ts

@@ -10225,6 +10225,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).
+        // 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) {
+                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)) {
+                throw new Error('VALIDATION_FAILED: query must be a ReportQuery object');
+            }
+        };
         const handleValidation = (res: any, err: any): boolean => {
             const msg = String(err?.message ?? err ?? '');
             if (msg.startsWith('VALIDATION_FAILED')) {
@@ -10278,6 +10312,11 @@ export class RestServer {
                     const svc = await resolveService(environmentId);
                     if (!svc) return respond501(res);
                     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) {

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 }));