Skip to content

Commit 8965398

Browse files
os-litantclaude
andauthored
fix(rest): parse api.projectResolution at the construction seam (#12450) (#12765)
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. Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd Co-authored-by: Claude <noreply@anthropic.com>
1 parent 87d3f9a commit 8965398

4 files changed

Lines changed: 154 additions & 33 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
---
2+
'@objectstack/rest': minor
3+
---
4+
5+
**BREAKING (accept-set tightening)**: `RestServer` now parses `api.projectResolution`
6+
against `RestApiConfigSchema` at construction, instead of `.omit()`ing it out of that
7+
parse.
8+
9+
The exemption existed because `@objectstack/runtime`'s standalone stack shipped
10+
`projectResolution: 'none'` — a value the declared enum has never contained — and
11+
`os serve` forwarded it straight in. Because `RestApiConfigSchema` is a non-strict
12+
object, omitting the key meant the undeclared value arrived, was silently stripped as
13+
an unknown key, and took `'auto'`'s branch by fallthrough, while the discovery handler
14+
copied it verbatim into `discovery.scoping.resolution` — publishing a payload the
15+
platform's own `DiscoverySchema` rejects, on every boot. #11999 (PR #12444) settled the
16+
disagreement by migrating the producer onto the declared `'auto'`; this change withdraws
17+
the exemption it justified.
18+
19+
**What changes for a caller:** `api.projectResolution` must now be one of the three
20+
values the schema has always declared — `'required'`, `'optional'` or `'auto'`. Anything
21+
else, `'none'` included, is refused at construction with a message naming the key. A
22+
census of every `projectResolution` value in this repo found exactly those three plus
23+
the retired one, and the retired one now survives only inside assertions that it is no
24+
longer emitted — so no in-repo boot path is affected.
25+
26+
If a config of yours is refused, correct the value at its producer. Do not re-add the key
27+
to the `.omit()`: a strategy outside the enum is wrong where it is written, not where it
28+
is read.
29+
30+
<!-- adr-0087: not-required (no-migration-prescription) Nothing authorable is removed or renamed: `RestApiConfigSchema` is untouched, its enum has carried the same three members throughout, and no spec key, export or config field changes spelling. There is also no old declared spelling for the ledger to name — a conversion entry rewrites a previously-LEGAL authorable value into its new one, and `'none'` was never declared; it was accepted only because this seam skipped the parse, so `objectstack migrate meta` has nothing to rewrite. And no conversion could decide it: `'none'` read as "no scoping at all", which is the `enableProjectScoping` switch rather than the strategy, so the producer's own migration to `'auto'` (#11999) rests on an analysis that holds only when scoping is OFF — every reader short-circuits on that flag first. A third-party config that wrote `'none'` WITH scoping enabled states an intent no mechanical rewrite can pick between, exactly as with the `api.version` refusals this seam already carries. Nor is the ledger the only channel that reaches an upgrader here, which is the D7 discriminator: unlike a runtime interface with no metadata surface, this value now meets a loud schema rejection at construction naming the key and the declared rule. -->

packages/rest/src/rest-config-parse-not-cast.test.ts

Lines changed: 60 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,31 @@
3030
*
3131
* ⚠️ §C is also where the first round's census miss is pinned. That census was
3232
* scoped to this package; the risk surface is EVERY package that constructs a
33-
* REST server, and `packages/cli`'s `os serve` ships
33+
* REST server, and `packages/cli`'s `os serve` shipped
3434
* `projectResolution: 'none'` — a value the declared enum does not contain and
35-
* `@objectstack/runtime` declares as a literal type. Five `packages/cli` e2e
35+
* `@objectstack/runtime` declared as a literal type. Five `packages/cli` e2e
3636
* boots went red in CI. The lesson, written where the next author will hit it:
3737
* the search radius belongs where the CONSUMERS live, not where the change does.
38+
*
39+
* [#12450] That miss is what bought `projectResolution` an `.omit()` from the
40+
* declared parse — and this file then carried a GREEN §C case defending the
41+
* exemption: "KEEPS `projectResolution: \"none\"` — the value this platform
42+
* actually ships". #11999 migrated the producer off that value (PR #12444) and
43+
* the case did not notice: it called `construct()` with a hand-written literal,
44+
* so its premise could die while it stayed green. ⛔ THE LESSON, and the reason
45+
* the retired value is now a REFUSAL in §A rather than a reworded guard here:
46+
* **a test that cannot fail when its premise dies is not protected by the
47+
* suite — it is hidden by it. The passing status is what stops anyone looking.**
48+
*
49+
* ⚠️ The other half of that premise is NOT measurable from this package, and
50+
* saying so here is part of the fix. "No boot path emits the retired value any
51+
* more" is a claim about the PRODUCER, and the producer (`@objectstack/runtime`)
52+
* depends on this package — so the coupling cannot be imported in this
53+
* direction without a cycle, and it lives at the producer instead:
54+
* `packages/runtime/src/standalone-stack.test.ts` drives the REAL emitted `api`
55+
* block through a REAL `RestServer` construction. THAT is the case that goes red
56+
* if the platform ever emits the retired value again; the two below only pin
57+
* what this seam does with a value once it arrives.
3858
*/
3959

4060
import { describe, it, expect, vi } from 'vitest';
@@ -94,6 +114,31 @@ describe('[#11637] §A RestServer construction runs RestApiConfigSchema', () =>
94114
expect(() => construct({ enableCrud: 'yes' as never })).toThrow(/api\.enableCrud/);
95115
});
96116

117+
it('[#12450] refuses the retired `projectResolution: "none"` — the parse finally reaches this key', () => {
118+
// `.omit()`ed from the declared parse until #12450, so this seam
119+
// ACCEPTED a value `RestApiConfigSchema` has never declared. Measured on
120+
// the pre-change tree: `construct({ projectResolution: 'none' })`
121+
// returned a server and `getApiBasePath()` answered `/api/v1`.
122+
// Declared-but-never-executed is the defect this whole file is named
123+
// after (#11637); #11999 removed the reason for the exemption by
124+
// migrating `@objectstack/runtime` onto the declared `'auto'`.
125+
let message = '';
126+
try {
127+
construct({ projectResolution: 'none' as never });
128+
} catch (err: any) {
129+
message = String(err?.message ?? err);
130+
}
131+
// POSITIVE CONTROL for the negative assertion below, and not a substring
132+
// of it: an empty `message` (nothing thrown) would satisfy any
133+
// `not.toContain` vacuously, so the refusal has to be proven present
134+
// before its shape can be measured.
135+
expect(message, 'the retired value must be refused, and the refusal must name the key').toContain('api.projectResolution');
136+
expect(
137+
message,
138+
'a projectResolution refusal must not diagnose `version`, a key this config never wrote',
139+
).not.toContain('/api//');
140+
});
141+
97142
it('appends the version rationale ONLY when `version` is what failed', () => {
98143
// Caught by this change's own ablation: a `projectResolution` refusal
99144
// 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
205250
expect((rest as any).config.api.enableSearch).toBe(false);
206251
});
207252

208-
it('KEEPS `projectResolution: "none"` — the value this platform actually ships', () => {
209-
// ⛔ The case CI caught and this file did not. `RestApiConfigSchema`
210-
// declares `z.enum(['required', 'optional', 'auto'])`, but the value
211-
// `os serve` forwards is `'none'`: `@objectstack/runtime`'s
212-
// `StandaloneStackResult.api` DECLARES the literal type
213-
// `{ enableProjectScoping: false; projectResolution: 'none' }`, and
214-
// `serve.ts` passes it through (`?? 'auto'` does not fire — `'none'` is
215-
// not nullish). Parsing this key would turn every `os serve` boot into
216-
// a crash; five packages/cli e2e boots did exactly that before the key
217-
// was `.omit()`ed. Which spelling is right is a contract question about
218-
// project-scoping semantics, filed as #11999 — NOT this seam's to
219-
// settle by refusing the value the platform ships.
220-
expect(() => construct({ enableProjectScoping: false, projectResolution: 'none' })).not.toThrow();
221-
expect(construct({ projectResolution: 'none' as never }).getApiBasePath()).toBe('/api/v1');
253+
it('[#12450] accepts every `projectResolution` the declared enum contains — the narrowing is the retired value ONLY', () => {
254+
// The BOUND on the change, and the half that makes "exactly one value
255+
// starts being rejected" a measurement rather than a claim. Census over
256+
// the tree at the time of #12450: four spellings appear anywhere as a
257+
// value for this key — `'required'`, `'optional'`, `'auto'` and the
258+
// retired `'none'` — so these three ARE the population that must keep
259+
// constructing. Read back off the normalized config rather than off a
260+
// mount path: a strategy that parsed and was then dropped in
261+
// normalization would still answer `/api/v1`.
262+
for (const projectResolution of ['required', 'optional', 'auto'] as const) {
263+
const rest = construct({ enableProjectScoping: true, projectResolution });
264+
expect((rest as any).config.api.projectResolution, projectResolution).toBe(projectResolution);
265+
}
222266
});
223267

224268
it('KEEPS the retired `api.requireAuth` warn-and-ignore posture (#3963)', () => {

packages/rest/src/rest-server.ts

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,7 @@ type NormalizedRestServerConfig = {
713713
};
714714

715715
/**
716-
* The declared `api` contract, minus the retired keys whose posture this seam
716+
* The declared `api` contract, minus the ONE retired key whose posture this seam
717717
* does not own (see {@link RestServer.assertDeclaredApiConfig}).
718718
*
719719
* Built on first use, not at module load: `RestApiConfigSchema` is a
@@ -723,7 +723,7 @@ type NormalizedRestServerConfig = {
723723
* `RestServer` is constructed per boot (and per test).
724724
*/
725725
function buildDeclaredApiConfigSchema() {
726-
return RestApiConfigSchema.omit({ requireAuth: true, projectResolution: true });
726+
return RestApiConfigSchema.omit({ requireAuth: true });
727727
}
728728
let declaredApiConfigSchemaCache: ReturnType<typeof buildDeclaredApiConfigSchema> | undefined;
729729

@@ -2989,23 +2989,33 @@ export class RestServer {
29892989
* tombstone ages out of `packages/spec` this line fails `tsc` — the
29902990
* drift cannot go silent.
29912991
*
2992-
* - `api.projectResolution` is `.omit()`ed for a DIFFERENT reason, and it
2993-
* is the one CI caught: the declared enum is
2992+
* - `api.projectResolution` USED to be `.omit()`ed here too, for a
2993+
* DIFFERENT reason, and it was the one CI caught: the declared enum is
29942994
* `z.enum(['required', 'optional', 'auto'])`, and the value this
2995-
* platform actually ships is `'none'` — produced by
2996-
* `@objectstack/runtime`'s standalone stack, whose
2997-
* `StandaloneStackResult.api` DECLARES the literal type
2998-
* `{ enableProjectScoping: false; projectResolution: 'none' }`, and
2999-
* forwarded by `os serve` straight into this config
2995+
* platform shipped was `'none'` — produced by `@objectstack/runtime`'s
2996+
* standalone stack and forwarded by `os serve` straight into this config
30002997
* (`apiConfig.projectResolution ?? 'auto'` — `'none'` is not nullish, so
3001-
* it passes through). Three packages disagree about this key's
3002-
* vocabulary, and they have disagreed silently for exactly as long as
3003-
* nothing ran the schema. Parsing it here does not settle that
3004-
* disagreement, it just turns every `os serve` boot into a crash.
3005-
* ⛔ Which spelling is right — teach the enum `'none'`, or migrate the
3006-
* runtime onto `'auto'` — is a contract question about project-scoping
3007-
* semantics that this seam cannot answer and this card does not own
3008-
* (`packages/spec` is `domain:spec`'s surface). Filed as #11999.
2998+
* it passed through). Three packages disagreed about this key's
2999+
* vocabulary, silently, for exactly as long as nothing ran the schema.
3000+
* Parsing it THEN would not have settled the disagreement, only turned
3001+
* every `os serve` boot into a crash — so it was filed as #11999 rather
3002+
* than decided here.
3003+
*
3004+
* [#11999 / PR #12444] settled it at the producer: the runtime migrated
3005+
* onto the declared `'auto'`. `'none'` read as "no scoping at all" and
3006+
* got `'auto'`'s behaviour by fallthrough anyway, because every reader
3007+
* that acts on the key is gated on `enableProjectScoping` first — but
3008+
* the discovery handler below copies the value into
3009+
* `discovery.scoping.resolution` UNCONDITIONALLY, and `DiscoverySchema`
3010+
* declares that field as the same three-member enum, so shipping
3011+
* `'none'` published a payload the platform's own schema rejects.
3012+
*
3013+
* [#12450] withdrew the exemption: the key is parsed here now, so an
3014+
* undeclared strategy is refused at construction instead of being
3015+
* stripped as an unknown key (this `z.object()` is non-strict) and
3016+
* taking `'auto'`'s branch in silence. ⛔ Do not re-add it to the
3017+
* `.omit()` to make some config boot — a strategy outside the enum is
3018+
* wrong where it is WRITTEN, not where it is read.
30093019
*
30103020
* The sibling sub-objects (`crud`, `metadata`, `batch`, `routes`) are still
30113021
* cast, not parsed, and carry declared constraints of their own

packages/runtime/src/standalone-stack.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,15 @@ import { createDefaultHostConfig, resolveDefaultArtifactPath } from './default-h
2525
// the value `createStandaloneStack` actually returns, never against a copy of
2626
// it. `@objectstack/spec` is a plain `dependencies` entry of this package.
2727
import { RestApiConfigSchema } from '@objectstack/spec/api';
28+
// [#12450] The CONSUMER seam itself, imported at MODULE LOAD (the #10126 rule
29+
// below): `@objectstack/rest` is a plain `dependencies` entry of this package,
30+
// and this package's vitest config aliases it to REST's SOURCE — so the case
31+
// that drives it measures the seam as it stands in this checkout, not as it
32+
// stands in some build artifact. The dependency only runs this way:
33+
// `@objectstack/runtime` depends on `@objectstack/rest`, so the producer→consumer
34+
// coupling cannot be written from inside `packages/rest` without a cycle, and
35+
// this file is where it has to live.
36+
import { RestServer } from '@objectstack/rest';
2837
// The REAL resolution, imported — not reproduced. `@objectstack/plugin-security`
2938
// is a plain `dependencies` entry of this package (and another test in this same
3039
// 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
155164
expect(issue?.code).toBe('invalid_value');
156165
});
157166

167+
it('[#12450] the emitted `api` block also survives the REAL RestServer construction', () => {
168+
// The case above proves the emitted value is DECLARED. It cannot prove that
169+
// the seam which CONSUMES it runs that declaration — and until #12450 that
170+
// seam did not: `RestServer` `.omit()`ed `projectResolution` out of its own
171+
// parse, so the undeclared strategy constructed a server happily for as long
172+
// as this factory shipped it. A schema pin is not an execution pin, and that
173+
// gap is exactly #11637's defect class.
174+
//
175+
// ⭐ THIS is the case that goes RED if this factory ever emits an undeclared
176+
// strategy again — measured against `result.api`, the REAL boot output, never
177+
// a restatement of it. Its sibling in
178+
// `packages/rest/src/rest-config-parse-not-cast.test.ts` pins what the seam
179+
// does with such a value once it arrives; only this one can see what the
180+
// platform actually hands it.
181+
const httpServer = {
182+
get: () => {}, post: () => {}, put: () => {}, delete: () => {}, patch: () => {},
183+
use: () => {}, listen: () => {}, close: () => {},
184+
} as any;
185+
const protocol = {
186+
getMetaItems: async ({ type }: { type: string }) => ({ type, items: [] }),
187+
} as any;
188+
const rest = new RestServer(httpServer, protocol, { api: { ...result.api } } as any);
189+
// Read back off the normalized config: a strategy that parsed and was then
190+
// dropped in normalization would still answer a correct mount path.
191+
expect((rest as any).config.api.projectResolution).toBe('auto');
192+
expect((rest as any).config.api.enableProjectScoping).toBe(false);
193+
});
194+
158195
it('the surfaced config feeds the REAL appSecurityPluginOptions → the app profile', () => {
159196
// Reproduce serve.ts's merge: `config = { ...originalConfig, ...standaloneStack }`,
160197
// then `new SecurityPlugin(appSecurityPluginOptions(config))`.

0 commit comments

Comments
 (0)