From a525312e7b0d6c11a03193238dc5a341f41fa0f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 16:48:13 +0000 Subject: [PATCH] fix(rest): parse `api.projectResolution` at the construction seam (#12450) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes `projectResolution` from the `.omit()` in `buildDeclaredApiConfigSchema`, so the declared enum is finally executed at that seam. `RestApiConfigSchema` is a non-strict object, so omitting the key did not merely skip a rule: the undeclared strategy arrived as an unknown key, was silently stripped, and took `'auto'`'s branch by fallthrough. Measured red-before / green-after at the real construction. Retires the green `KEEPS` case that defended the omit rather than rewording it. Its premise died when #11999 (PR #12444) migrated the runtime onto the declared `'auto'`, and it could not go red — it called the constructor with a hand-written literal and never read the producer. A reworded version would have kept exactly that property. The refusal and its bound are pinned in packages/rest. The producer coupling that CAN fail on a regression lives at the producer, in packages/runtime/src/standalone-stack.test.ts, which drives the real emitted `api` block through a real RestServer construction: `@objectstack/runtime` depends on `@objectstack/rest`, so the import cannot be written in the other direction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- .changeset/rest-project-resolution-parsed.md | 30 ++++++++ .../src/rest-config-parse-not-cast.test.ts | 76 +++++++++++++++---- packages/rest/src/rest-server.ts | 44 ++++++----- packages/runtime/src/standalone-stack.test.ts | 37 +++++++++ 4 files changed, 154 insertions(+), 33 deletions(-) create mode 100644 .changeset/rest-project-resolution-parsed.md diff --git a/.changeset/rest-project-resolution-parsed.md b/.changeset/rest-project-resolution-parsed.md new file mode 100644 index 0000000000..36390145dd --- /dev/null +++ b/.changeset/rest-project-resolution-parsed.md @@ -0,0 +1,30 @@ +--- +'@objectstack/rest': minor +--- + +**BREAKING (accept-set tightening)**: `RestServer` now parses `api.projectResolution` +against `RestApiConfigSchema` at construction, instead of `.omit()`ing it out of that +parse. + +The exemption existed because `@objectstack/runtime`'s standalone stack shipped +`projectResolution: 'none'` — a value the declared enum has never contained — and +`os serve` forwarded it straight in. Because `RestApiConfigSchema` is a non-strict +object, omitting the key meant the undeclared value arrived, was silently stripped as +an unknown key, and took `'auto'`'s branch by fallthrough, while the discovery handler +copied it verbatim into `discovery.scoping.resolution` — publishing a payload the +platform's own `DiscoverySchema` rejects, on every boot. #11999 (PR #12444) settled the +disagreement by migrating the producer onto the declared `'auto'`; this change withdraws +the exemption it justified. + +**What changes for a caller:** `api.projectResolution` must now be one of the three +values the schema has always declared — `'required'`, `'optional'` or `'auto'`. Anything +else, `'none'` included, is refused at construction with a message naming the key. A +census of every `projectResolution` value in this repo found exactly those three plus +the retired one, and the retired one now survives only inside assertions that it is no +longer emitted — so no in-repo boot path is affected. + +If a config of yours is refused, correct the value at its producer. Do not re-add the key +to the `.omit()`: a strategy outside the enum is wrong where it is written, not where it +is read. + + diff --git a/packages/rest/src/rest-config-parse-not-cast.test.ts b/packages/rest/src/rest-config-parse-not-cast.test.ts index 05283eb994..96f576c19f 100644 --- a/packages/rest/src/rest-config-parse-not-cast.test.ts +++ b/packages/rest/src/rest-config-parse-not-cast.test.ts @@ -30,11 +30,31 @@ * * ⚠️ §C is also where the first round's census miss is pinned. That census was * scoped to this package; the risk surface is EVERY package that constructs a - * REST server, and `packages/cli`'s `os serve` ships + * REST server, and `packages/cli`'s `os serve` shipped * `projectResolution: 'none'` — a value the declared enum does not contain and - * `@objectstack/runtime` declares as a literal type. Five `packages/cli` e2e + * `@objectstack/runtime` declared as a literal type. Five `packages/cli` e2e * boots went red in CI. The lesson, written where the next author will hit it: * the search radius belongs where the CONSUMERS live, not where the change does. + * + * [#12450] That miss is what bought `projectResolution` an `.omit()` from the + * declared parse — and this file then carried a GREEN §C case defending the + * exemption: "KEEPS `projectResolution: \"none\"` — the value this platform + * actually ships". #11999 migrated the producer off that value (PR #12444) and + * the case did not notice: it called `construct()` with a hand-written literal, + * so its premise could die while it stayed green. ⛔ THE LESSON, and the reason + * the retired value is now a REFUSAL in §A rather than a reworded guard here: + * **a test that cannot fail when its premise dies is not protected by the + * suite — it is hidden by it. The passing status is what stops anyone looking.** + * + * ⚠️ The other half of that premise is NOT measurable from this package, and + * saying so here is part of the fix. "No boot path emits the retired value any + * more" is a claim about the PRODUCER, and the producer (`@objectstack/runtime`) + * depends on this package — so the coupling cannot be imported in this + * direction without a cycle, and it lives at the producer instead: + * `packages/runtime/src/standalone-stack.test.ts` drives the REAL emitted `api` + * block through a REAL `RestServer` construction. THAT is the case that goes red + * if the platform ever emits the retired value again; the two below only pin + * what this seam does with a value once it arrives. */ import { describe, it, expect, vi } from 'vitest'; @@ -94,6 +114,31 @@ describe('[#11637] §A RestServer construction runs RestApiConfigSchema', () => expect(() => construct({ enableCrud: 'yes' as never })).toThrow(/api\.enableCrud/); }); + it('[#12450] refuses the retired `projectResolution: "none"` — the parse finally reaches this key', () => { + // `.omit()`ed from the declared parse until #12450, so this seam + // ACCEPTED a value `RestApiConfigSchema` has never declared. Measured on + // the pre-change tree: `construct({ projectResolution: 'none' })` + // returned a server and `getApiBasePath()` answered `/api/v1`. + // Declared-but-never-executed is the defect this whole file is named + // after (#11637); #11999 removed the reason for the exemption by + // migrating `@objectstack/runtime` onto the declared `'auto'`. + let message = ''; + try { + construct({ projectResolution: 'none' as never }); + } catch (err: any) { + message = String(err?.message ?? err); + } + // POSITIVE CONTROL for the negative assertion below, and not a substring + // of it: an empty `message` (nothing thrown) would satisfy any + // `not.toContain` vacuously, so the refusal has to be proven present + // before its shape can be measured. + expect(message, 'the retired value must be refused, and the refusal must name the key').toContain('api.projectResolution'); + expect( + message, + 'a projectResolution refusal must not diagnose `version`, a key this config never wrote', + ).not.toContain('/api//'); + }); + it('appends the version rationale ONLY when `version` is what failed', () => { // Caught by this change's own ablation: a `projectResolution` refusal // printed the whole "an empty version mounts the entire API at /api//" @@ -205,20 +250,19 @@ describe('[#11637] §C regression guards — the narrowing is exactly the declar expect((rest as any).config.api.enableSearch).toBe(false); }); - it('KEEPS `projectResolution: "none"` — the value this platform actually ships', () => { - // ⛔ The case CI caught and this file did not. `RestApiConfigSchema` - // declares `z.enum(['required', 'optional', 'auto'])`, but the value - // `os serve` forwards is `'none'`: `@objectstack/runtime`'s - // `StandaloneStackResult.api` DECLARES the literal type - // `{ enableProjectScoping: false; projectResolution: 'none' }`, and - // `serve.ts` passes it through (`?? 'auto'` does not fire — `'none'` is - // not nullish). Parsing this key would turn every `os serve` boot into - // a crash; five packages/cli e2e boots did exactly that before the key - // was `.omit()`ed. Which spelling is right is a contract question about - // project-scoping semantics, filed as #11999 — NOT this seam's to - // settle by refusing the value the platform ships. - expect(() => construct({ enableProjectScoping: false, projectResolution: 'none' })).not.toThrow(); - expect(construct({ projectResolution: 'none' as never }).getApiBasePath()).toBe('/api/v1'); + it('[#12450] accepts every `projectResolution` the declared enum contains — the narrowing is the retired value ONLY', () => { + // The BOUND on the change, and the half that makes "exactly one value + // starts being rejected" a measurement rather than a claim. Census over + // the tree at the time of #12450: four spellings appear anywhere as a + // value for this key — `'required'`, `'optional'`, `'auto'` and the + // retired `'none'` — so these three ARE the population that must keep + // constructing. Read back off the normalized config rather than off a + // mount path: a strategy that parsed and was then dropped in + // normalization would still answer `/api/v1`. + for (const projectResolution of ['required', 'optional', 'auto'] as const) { + const rest = construct({ enableProjectScoping: true, projectResolution }); + expect((rest as any).config.api.projectResolution, projectResolution).toBe(projectResolution); + } }); it('KEEPS the retired `api.requireAuth` warn-and-ignore posture (#3963)', () => { diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index d23f5c009b..ec8252f3f3 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -713,7 +713,7 @@ type NormalizedRestServerConfig = { }; /** - * The declared `api` contract, minus the retired keys whose posture this seam + * The declared `api` contract, minus the ONE retired key whose posture this seam * does not own (see {@link RestServer.assertDeclaredApiConfig}). * * Built on first use, not at module load: `RestApiConfigSchema` is a @@ -723,7 +723,7 @@ type NormalizedRestServerConfig = { * `RestServer` is constructed per boot (and per test). */ function buildDeclaredApiConfigSchema() { - return RestApiConfigSchema.omit({ requireAuth: true, projectResolution: true }); + return RestApiConfigSchema.omit({ requireAuth: true }); } let declaredApiConfigSchemaCache: ReturnType | undefined; @@ -2989,23 +2989,33 @@ export class RestServer { * tombstone ages out of `packages/spec` this line fails `tsc` — the * drift cannot go silent. * - * - `api.projectResolution` is `.omit()`ed for a DIFFERENT reason, and it - * is the one CI caught: the declared enum is + * - `api.projectResolution` USED to be `.omit()`ed here too, for a + * DIFFERENT reason, and it was the one CI caught: the declared enum is * `z.enum(['required', 'optional', 'auto'])`, and the value this - * platform actually ships is `'none'` — produced by - * `@objectstack/runtime`'s standalone stack, whose - * `StandaloneStackResult.api` DECLARES the literal type - * `{ enableProjectScoping: false; projectResolution: 'none' }`, and - * forwarded by `os serve` straight into this config + * platform shipped was `'none'` — produced by `@objectstack/runtime`'s + * standalone stack and forwarded by `os serve` straight into this config * (`apiConfig.projectResolution ?? 'auto'` — `'none'` is not nullish, so - * it passes through). Three packages disagree about this key's - * vocabulary, and they have disagreed silently for exactly as long as - * nothing ran the schema. Parsing it here does not settle that - * disagreement, it just turns every `os serve` boot into a crash. - * ⛔ Which spelling is right — teach the enum `'none'`, or migrate the - * runtime onto `'auto'` — is a contract question about project-scoping - * semantics that this seam cannot answer and this card does not own - * (`packages/spec` is `domain:spec`'s surface). Filed as #11999. + * it passed through). Three packages disagreed about this key's + * vocabulary, silently, for exactly as long as nothing ran the schema. + * Parsing it THEN would not have settled the disagreement, only turned + * every `os serve` boot into a crash — so it was filed as #11999 rather + * than decided here. + * + * [#11999 / PR #12444] settled it at the producer: the runtime migrated + * onto the declared `'auto'`. `'none'` read as "no scoping at all" and + * got `'auto'`'s behaviour by fallthrough anyway, because every reader + * that acts on the key is gated on `enableProjectScoping` first — but + * the discovery handler below copies the value into + * `discovery.scoping.resolution` UNCONDITIONALLY, and `DiscoverySchema` + * declares that field as the same three-member enum, so shipping + * `'none'` published a payload the platform's own schema rejects. + * + * [#12450] withdrew the exemption: the key is parsed here now, so an + * undeclared strategy is refused at construction instead of being + * stripped as an unknown key (this `z.object()` is non-strict) and + * taking `'auto'`'s branch in silence. ⛔ Do not re-add it to the + * `.omit()` to make some config boot — a strategy outside the enum is + * wrong where it is WRITTEN, not where it is read. * * The sibling sub-objects (`crud`, `metadata`, `batch`, `routes`) are still * cast, not parsed, and carry declared constraints of their own diff --git a/packages/runtime/src/standalone-stack.test.ts b/packages/runtime/src/standalone-stack.test.ts index f66861b64d..d148d3f139 100644 --- a/packages/runtime/src/standalone-stack.test.ts +++ b/packages/runtime/src/standalone-stack.test.ts @@ -25,6 +25,15 @@ import { createDefaultHostConfig, resolveDefaultArtifactPath } from './default-h // the value `createStandaloneStack` actually returns, never against a copy of // it. `@objectstack/spec` is a plain `dependencies` entry of this package. import { RestApiConfigSchema } from '@objectstack/spec/api'; +// [#12450] The CONSUMER seam itself, imported at MODULE LOAD (the #10126 rule +// below): `@objectstack/rest` is a plain `dependencies` entry of this package, +// and this package's vitest config aliases it to REST's SOURCE — so the case +// that drives it measures the seam as it stands in this checkout, not as it +// stands in some build artifact. The dependency only runs this way: +// `@objectstack/runtime` depends on `@objectstack/rest`, so the producer→consumer +// coupling cannot be written from inside `packages/rest` without a cycle, and +// this file is where it has to live. +import { RestServer } from '@objectstack/rest'; // The REAL resolution, imported — not reproduced. `@objectstack/plugin-security` // is a plain `dependencies` entry of this package (and another test in this same // package, src/domains/share-links-enforcement-context.test.ts, already imports @@ -155,6 +164,34 @@ describe('createStandaloneStack — surfaces app RBAC from the artifact (ADR-005 expect(issue?.code).toBe('invalid_value'); }); + it('[#12450] the emitted `api` block also survives the REAL RestServer construction', () => { + // The case above proves the emitted value is DECLARED. It cannot prove that + // the seam which CONSUMES it runs that declaration — and until #12450 that + // seam did not: `RestServer` `.omit()`ed `projectResolution` out of its own + // parse, so the undeclared strategy constructed a server happily for as long + // as this factory shipped it. A schema pin is not an execution pin, and that + // gap is exactly #11637's defect class. + // + // ⭐ THIS is the case that goes RED if this factory ever emits an undeclared + // strategy again — measured against `result.api`, the REAL boot output, never + // a restatement of it. Its sibling in + // `packages/rest/src/rest-config-parse-not-cast.test.ts` pins what the seam + // does with such a value once it arrives; only this one can see what the + // platform actually hands it. + const httpServer = { + get: () => {}, post: () => {}, put: () => {}, delete: () => {}, patch: () => {}, + use: () => {}, listen: () => {}, close: () => {}, + } as any; + const protocol = { + getMetaItems: async ({ type }: { type: string }) => ({ type, items: [] }), + } as any; + const rest = new RestServer(httpServer, protocol, { api: { ...result.api } } as any); + // Read back off the normalized config: a strategy that parsed and was then + // dropped in normalization would still answer a correct mount path. + expect((rest as any).config.api.projectResolution).toBe('auto'); + expect((rest as any).config.api.enableProjectScoping).toBe(false); + }); + it('the surfaced config feeds the REAL appSecurityPluginOptions → the app profile', () => { // Reproduce serve.ts's merge: `config = { ...originalConfig, ...standaloneStack }`, // then `new SecurityPlugin(appSecurityPluginOptions(config))`.