diff --git a/.changeset/client-unannotated-return-erasure.md b/.changeset/client-unannotated-return-erasure.md new file mode 100644 index 0000000000..78e1627a0c --- /dev/null +++ b/.changeset/client-unannotated-return-erasure.md @@ -0,0 +1,52 @@ +--- +"@objectstack/client": minor +--- + +fix(client): bind the three verifiable methods of the unannotated return-type erasure population to their spec contracts (#11925) + +**Return-type narrowing on a published SDK.** No runtime change — the value each +method resolves to is byte-identical before and after. Only the DECLARED type +moved, off `any`, which is precisely why a runtime test cannot observe it and +the pins for it are type-level. + +`any` is assignable to everything and admits every property read, so for each +method below a consumer's code could stop compiling where it previously did +not: assigning the result to an unrelated annotation, reading a property the +bound type does not declare, or forwarding the value to a differently-typed +parameter. + +## What changed, per family + +**`client.packages.list` → `{ packages: InstalledPackage[]; total: number }`** +(was `{ packages: any[]; total: number }`). A consumer stops compiling if it +reads any key off a row that `InstalledPackage` does not declare. Note in +particular `source` (`'database' | 'registry' | 'both'`): the REST surface +spreads it onto each row, the dispatcher surface does not, and it is therefore +deliberately NOT declared — code reading `pkg.source` off this result compiles +today and will not after. `total` and the array envelope are unchanged. + +**`client.packages.update` → `InstalledPackage`** (was `any`). This method +declared no envelope before, so nothing about the shape claim changed; a +consumer stops compiling if it reads an undeclared key off the returned row, or +assigns the result somewhere `InstalledPackage` does not fit. + +**`ScopedProjectClient.packages.get` → `{ package: InstalledPackage }`** (was +`{ package: any }`). The `{ package }` envelope is unchanged; only the member +narrowed. A consumer stops compiling if it reads a key off `.package` that +`InstalledPackage` does not declare — again including `source`, which this +route does send and which stays undeclared for consistency with its already +bound `list` sibling. + +## What deliberately did NOT change + +The other 36 methods in the measured population keep their erased `any`, each +with a docblock stating why and pointing at the issue that carries it: +`meta.*` history/diagnostics (9) and eight `packages.*` routes have no published +response contract to bind to (#12038); `client.packages.get` has two mounted +surfaces that emit different envelopes and `install`/`enable`/`disable` declare +an envelope no surface emits (#12034); the 15 cloud `projects.*` methods call a +control plane that speaks snake_case while the `@objectstack/spec/cloud` rows are +camelCase, so binding to them would compile and be false (#12036). + +No consumer loses anything by those staying `any` — they are exactly as +permissive as before. diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 90edd61eda..65548bfe4b 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -981,6 +981,27 @@ export class ObjectStackClient { /* [#3563 PR-5] The three meta routes that had no SDK expression. */ + /** + * ⛔ [#11925] The nine `meta.*` history / diagnostics methods below keep + * `unwrapResponse< any >` DELIBERATELY — Class C, a missing contract + * rather than a missing annotation (#12038). + * + * `getPublished`, `listDrafts`, `migrateStored`, `getDiagnostics`, + * `getReferences`, `getBookTree`, `getAudit`, `rollbackItem`, `diffItem`. + * + * Nothing in `@objectstack/spec` declares any of these nine route + * responses: every ledger row reports `responseSchema=None`, and the + * producers hand back whatever the protocol service returns. The one named + * type that exists — `StoredMigrationReport`, which `migrateStored`'s + * docblock points at — lives in `@objectstack/metadata-protocol`, which is + * not a dependency of this package. + * + * `publishItem` below is the counter-example and the precedent: it is + * annotated only because #7294 declared `PublishMetaItemResponseSchema` + * first. Same order applies here — schema, then ledger row, then + * annotation. ⛔ Do not reach for a same-named neighbour instead. + */ + /** * ADR-0033: the published version of a metadata item. Compound names are * passed through unencoded (e.g. `getPublished('lead', 'views/all_leads')`), @@ -1341,8 +1362,18 @@ export class ObjectStackClient { packages = { /** * List all installed packages with optional filters. - */ - list: async (filters?: { status?: string; type?: string; enabled?: boolean }) => { + * + * [#11925] Bound to `InstalledPackage`. Both mounted surfaces answer the + * SAME envelope for this route, which is what makes it bindable: + * `runtime`'s `/packages` domain sends `success({ packages, total: + * packages.length })` and `rest`'s `GET {base}/packages` sends + * `sendOk(res, { packages, total: packages.length })`. The REST rows carry + * an extra `source: 'database' | 'registry' | 'both'` discriminator that is + * deliberately NOT declared here — the dispatcher rows have no such key, so + * declaring it would be false on that surface. #8140 bound the identical + * scoped sibling (`ScopedProjectClient.packages.list`) the same way. + */ + list: async (filters?: { status?: string; type?: string; enabled?: boolean }): Promise<{ packages: InstalledPackage[]; total: number }> => { const route = this.getRoute('packages'); const params = new URLSearchParams(); if (filters?.status) params.set('status', filters.status); @@ -1351,11 +1382,24 @@ export class ObjectStackClient { const qs = params.toString(); const url = `${this.baseUrl}${route}${qs ? '?' + qs : ''}`; const res = await this.fetch(url); - return this.unwrapResponse<{ packages: any[]; total: number }>(res); + return this.unwrapResponse<{ packages: InstalledPackage[]; total: number }>(res); }, /** * Get a specific installed package by its ID (reverse domain identifier). + * + * ⛔ [#11925] NOT bound, and the `{ package }` envelope is left exactly as + * it was — the two mounted surfaces answer this route with DIFFERENT + * envelopes, so no single declaration is true (#12034). `runtime`'s + * `/packages` domain sends `success(pkg)` — the bare row — while `rest`'s + * `GET {base}/packages/:id` sends `sendOk(res, { package: { ...pkg, + * source } })`, and the REST routes "shadow live dispatcher twins" only + * where a `package` service is registered. Binding the member here would + * harden a claim that is already false on one of the two. + * + * Its SCOPED twin `ScopedProjectClient.packages.get` IS bound, because + * only the REST registrar serves the scoped mount — one surface, one + * shape. */ get: async (id: string) => { const route = this.getRoute('packages'); @@ -1366,6 +1410,15 @@ export class ObjectStackClient { /** * Install a new package from its manifest. * + * ⛔ [#11925] NOT bound. This method and its `enable` / `disable` + * neighbours declare `{ package; message? }`, and the ONLY surface that + * serves them — `runtime`'s `/packages` domain; `rest` mounts no twin for + * any of the three — answers `success(pkg)`, the bare row (#12034). The + * declared envelope is not merely erased, it is false, and the `any` + * member is what keeps that invisible. Correcting it is a response-shape + * decision with its own clause-② analysis, not the `any`-binding this card + * carries, so the shape is left untouched here. + * * By default the server rejects a manifest whose `id` is already * installed with **409 Conflict** (duplicate-id guard) instead of * silently overwriting the existing package. Intentional upgrade / @@ -1430,16 +1483,43 @@ export class ObjectStackClient { * Edit a package's manifest (partial: name / description / version). * Identity (`id` / `scope` / `type`) and lifecycle state are not editable * here; the server rejects an empty patch and non-semantic versions. - */ - update: async (id: string, patch: { name?: string; description?: string; version?: string }) => { + * + * [#11925] Bound to `InstalledPackage` — the BARE row, with no envelope. + * `PATCH /packages/:id` is served only by `runtime`'s `/packages` domain + * (`rest` mounts no PATCH twin), and that handler answers + * `success((updated as any)?.package ?? updated)` on the protocol path and + * `success(pkg)` on the registry fallback — the row either way. This method + * declared no envelope before the binding, so the shape claim is unchanged + * and only the erased `any` moved. + */ + update: async (id: string, patch: { name?: string; description?: string; version?: string }): Promise => { const route = this.getRoute('packages'); const res = await this.fetch(`${this.baseUrl}${route}/${encodeURIComponent(id)}`, { method: 'PATCH', body: JSON.stringify(patch), }); - return this.unwrapResponse(res); + return this.unwrapResponse(res); }, + /** + * ⛔ [#11925] The eight methods from here down — `publish`, + * `discardDrafts`, `listCommits`, `revertCommit`, `rollback`, `export`, + * `adoptOrphans`, `duplicate` — keep `unwrapResponse< any >` on purpose: + * Class C, a missing contract (#12038). Their handlers reach the protocol + * service through an `any` cast (`(protocol as any).listCommits`, + * `.revertCommit`, `.rollbackToPackageCommit`, `.duplicatePackage`, …), so + * there is no declared type anywhere on the path to lift, and every ledger + * row reports `responseSchema=None`. + * + * ⭐ `rollback` in particular: `PackageRollbackResponse` sits one import + * away in `@objectstack/spec/api` and is the WRONG type for it. That + * schema declares `{ success, restoredVersion?, message? }` — a VERSION + * rollback — while this method posts `{ commitId }` and the dispatcher + * routes it to `rollbackToPackageCommit`, the ADR-0067 COMMIT rollback. + * Binding it would compile and be false. `return-type-precision.test.ts` + * holds a compile-time guard against that substitution. + */ + /** Publish the package's metadata snapshot. */ publish: async (id: string, opts?: Record) => { const route = this.getRoute('packages'); @@ -1577,6 +1657,27 @@ export class ObjectStackClient { * * @see docs/adr/0002-project-database-isolation.md */ + /** + * ⛔ [#11925] Every unannotated method in this namespace, and in the + * environment-scoped `packages` block nested inside it, keeps its erased + * `any` DELIBERATELY (#12036). + * + * `@objectstack/spec/cloud` does declare row contracts that look like the + * obvious binding — `Environment`, `EnvironmentCredential`, + * `EnvironmentPackageInstallation` — and they are camelCase + * (`displayName`, `organizationId`, `isDefault`, `databaseUrl`; zero + * snake_case keys across all three schemas). The `/api/v1/cloud/*` control + * plane this namespace calls speaks **snake_case**: the in-repo CLI + * consumers read `p.display_name`, `p.organization_id`, `p.is_default`, + * `res.database.database_url`, `res.membership.role`, and send + * `organization_id` / `display_name` / `clone_from_environment_id`. + * + * So those contracts are not this wire's types. Binding to them would + * typecheck, be false, and break the CLI at compile time while telling it + * that it is wrong when it is right — the `SearchResult` near-miss class + * #8140 recorded, at family scale. The control-plane implementation is not + * in this repo, so the casing cannot be settled from here. + */ projects = { /** * List environments visible to the current session. Optionally filter @@ -5662,10 +5763,26 @@ export class ScopedProjectClient { const res = await this.parent._fetch(this.url('/packages')); return this.parent._unwrap<{ packages: InstalledPackage[]; total: number }>(res); }, - get: async (id: string, version?: string) => { + /** + * [#11925] The asymmetry #8140 recorded, now closed. Its neighbour `list` + * above carried BOTH a return annotation and a type argument, so #8140 + * bound it; this method carried neither and was left erased — same object + * literal, same route family, opposite treatment purely because one lacked + * the annotation. + * + * The scoped mount is unambiguous, which is what makes it bindable while + * the GLOBAL `client.packages.get` is not: `registerPackageRoutes` is + * mounted at both `{base}/packages` and + * `{base}/environments/:environmentId/packages`, and only the REST + * registrar serves the scoped path — so the `{ package }` envelope + * declared here is the one that route actually sends. The handler also + * spreads a `source: 'database' | 'registry'` discriminator onto the row, + * left undeclared for the same reason `list` leaves it undeclared. + */ + get: async (id: string, version?: string): Promise<{ package: InstalledPackage }> => { const qs = version ? `?version=${encodeURIComponent(version)}` : ''; const res = await this.parent._fetch(this.url(`/packages/${encodeURIComponent(id)}${qs}`)); - return this.parent._unwrap<{ package: any }>(res); + return this.parent._unwrap<{ package: InstalledPackage }>(res); }, }; diff --git a/packages/client/src/return-type-precision.test.ts b/packages/client/src/return-type-precision.test.ts index db2a7a6320..79d9211e40 100644 --- a/packages/client/src/return-type-precision.test.ts +++ b/packages/client/src/return-type-precision.test.ts @@ -48,6 +48,8 @@ import type { import type { ActionDescriptor, ExecutionLog, FlowParsed } from '@objectstack/spec/automation'; import type { ExplainDecision } from '@objectstack/spec/security'; import type { InstalledPackage } from '@objectstack/spec/kernel'; +import type { PackageRollbackResponse } from '@objectstack/spec/api'; +import type { Environment } from '@objectstack/spec/cloud'; declare const client: ObjectStackClient; declare const scoped: ScopedProjectClient; @@ -177,6 +179,114 @@ export function searchResultIsNotTheGlobalSearchShape(): void { void mismatched; } +/** + * [#11925] The FIFTH erasure spelling: methods with no return annotation at + * all, whose published type was inferred from `unwrapResponse< …any… >`. + * + * Re-measured at `origin/main` the population is **39**, not the 38 the card + * recorded — its own single-line reproducer cannot see `projects.get`, whose + * type argument spans several lines. Of the 39, exactly **three** had a + * verifiable published type to bind to; the other 36 are recorded on the + * methods themselves and filed (#12034, #12036, #12038). The bar was not "a + * plausible type exists" but "the route this method calls demonstrably sends + * this shape", which is what disqualified most of the population. + * + * These pins are type-level for the reason the header of this file gives: a + * runtime test cannot observe a return-type narrowing at all. + */ +export async function returnTypePrecisionPins11925(): Promise { + // ── bound 1/3: both surfaces agree on this envelope ────────────────── + // `runtime`'s `/packages` domain and `rest`'s `GET {base}/packages` both + // send `{ packages, total: packages.length }`. The REST rows also carry a + // `source` discriminator the dispatcher rows lack, so it stays undeclared + // — the same treatment #8140 gave the scoped sibling directly above. + expectTypeOf(await client.packages.list()).toEqualTypeOf<{ + packages: InstalledPackage[]; + total: number; + }>(); + + // ── bound 2/3: the BARE row, no envelope ───────────────────────────── + // `PATCH /packages/:id` is dispatcher-only and answers `success(pkg)`. + // This method declared no envelope before the binding either, so only the + // erased `any` moved. + expectTypeOf(await client.packages.update('com.acme.crm', { name: 'Acme' })) + .toEqualTypeOf(); + + // ── bound 3/3: the asymmetry named on the card, closed ─────────────── + // `scoped.packages.list` was bound by #8140 because it happened to carry + // an annotation; its neighbour `get` was not, purely because it lacked + // one. Same object literal, same route family. The scoped mount is served + // ONLY by the REST registrar, so unlike the global `client.packages.get` + // there is one surface and one shape. + expectTypeOf(await scoped.packages.get('com.acme.crm')).toEqualTypeOf<{ + package: InstalledPackage; + }>(); + + // ── direction 2: a WRONG shape must now be rejected ─────────────────── + // ⚠️ Only ONE of the three below is red before this change, and the split + // is stated here rather than glossed, because a suppression that was + // already used is a regression guard and not evidence the binding was + // needed. Ablation (revert `index.ts` to `origin/main`, keep this file) + // measured it: the ablated run reports TS2578 at `wrongUpdate` ONLY. + // + // `packages.update` was bare `any` before, and `any` IS assignable to + // `string`, so its suppression went unused → TS2578. The other two were + // never bare: they declared a real envelope (`{ packages: any[]; total }` + // and `{ package: any }`) whose MEMBER was the erased part, and an + // envelope is not assignable to a bare row or array in either state. Their + // suppressions are used before AND after — regression guards against a + // future "narrowing" that flattens the envelope away. + + // GREEN IN BOTH STATES — regression guard, not red-before evidence. + // @ts-expect-error the route answers `{ packages, total }`, not a bare array + const wrongList: InstalledPackage[] = await client.packages.list(); + + // RED BEFORE: `any` is assignable to `string`, so this suppression is + // unused (TS2578) until `packages.update` is bound. + // @ts-expect-error `packages.update` answers the row, not a string + const wrongUpdate: string = await client.packages.update('com.acme.crm', { name: 'Acme' }); + + // GREEN IN BOTH STATES — regression guard, not red-before evidence. + // @ts-expect-error the scoped detail route answers `{ package }`, not the bare row + const wrongScopedGet: InstalledPackage = await scoped.packages.get('com.acme.crm'); + + void wrongList; + void wrongUpdate; + void wrongScopedGet; +} + +/** + * ⚠️ GREEN IN BOTH STATES — regression guards, recorded as such rather than + * counted as evidence that this card's change was needed. Each pins a + * near-miss in a DEPENDENCY that the next sweep would otherwise reach for. + * + * 1. `PackageRollbackResponse` sits one import away from + * `client.packages.rollback` and is the wrong type for it: it declares the + * VERSION rollback (`{ success, restoredVersion?, message? }`, per its + * file header `POST /api/v1/packages/:packageId/rollback — Rollback a + * package`), while the client method posts `{ commitId }` and the + * dispatcher routes it to `rollbackToPackageCommit` — the ADR-0067 COMMIT + * rollback. Binding it would compile and be false. + * + * 2. `Environment` is the obvious-looking binding for `client.projects.*` and + * is camelCase, while the `/api/v1/cloud/*` control plane those methods + * call speaks snake_case (measured from this repo's own CLI consumers: + * `p.display_name`, `p.organization_id`, `p.is_default`). Binding it would + * typecheck, be false, and break those callers. + */ +declare const versionRollbackPayload: PackageRollbackResponse['data']; +declare const specEnvironmentRow: Environment; + +export function packageRollbackResponseIsNotTheCommitRollbackShape(): void { + // @ts-expect-error the VERSION-rollback payload carries no commit identity + void versionRollbackPayload.commitId; +} + +export function environmentIsNotTheCloudWireRow(): void { + // @ts-expect-error the control plane sends `display_name`; this row declares `displayName` + void specEnvironmentRow.display_name; +} + describe('client SDK return-type precision (#8140)', () => { it('exposes the type-level pins to tsc without executing a request', () => { // The assertions above are evaluated by `tsc` under @@ -186,6 +296,9 @@ describe('client SDK return-type precision (#8140)', () => { // narrowing at all. expect(typeof returnTypePrecisionPins).toBe('function'); expect(typeof searchResultIsNotTheGlobalSearchShape).toBe('function'); + expect(typeof returnTypePrecisionPins11925).toBe('function'); + expect(typeof packageRollbackResponseIsNotTheCommitRollbackShape).toBe('function'); + expect(typeof environmentIsNotTheCloudWireRow).toBe('function'); }); it('unwraps exactly one `{ success, data }` envelope — the premise the annotations rest on', async () => {