diff --git a/.changeset/discovery-environment-single-default.md b/.changeset/discovery-environment-single-default.md new file mode 100644 index 0000000000..3e8b105126 --- /dev/null +++ b/.changeset/discovery-environment-single-default.md @@ -0,0 +1,42 @@ +--- +"@objectstack/spec": patch +"@objectstack/metadata-protocol": patch +"@objectstack/runtime": patch +--- + +fix(spec,metadata-protocol,runtime): one place decides what an unset `NODE_ENV` advertises (#5936) + +A deployment whose operator never exported `NODE_ENV` must not describe itself as +`development` on `/discovery`: `environment` is a machine-readable field, a client +reads it to answer "am I talking to production?", and it may skip production warnings +or loosen a destructive action's confirmation on the answer. #5673 ruled that in and +fixed it — but only for one of the two producers, because that dispatch put +`packages/spec` out of scope. The other one, `MetadataProtocol.getDiscovery()` (served +by `@objectstack/rest`), went on answering `development` for exactly that input. + +The default now lives in the shared mapper, `resolveDiscoveryEnvironment`: an absent — +or blank — value resolves to `production`, and both producers pass the operator's value +through as they read it, neither carrying a default of its own. That is what makes it +one decision instead of two copies, and it means the next discovery producer inherits +the right answer without anyone remembering to copy a line. Patching only +metadata-protocol would have left a second copy of the default — precisely the drift the +shared table was created to prevent (#4828). + +"Unset" includes a blank value: `NODE_ENV=` exports an empty string, the runtime's +`getEnv` has always folded that into its default, and had the mapper treated blank as +"anything else" the two producers would have drifted again on that one input. + +**#4828's rule is untouched, and it points the other way on purpose.** A value that IS +set but is not a spelling this repo recognises (`qa`, `preview`) still degrades to +`development`, so nothing ever claims `production` on a guess. Absence is not a guess — +it is the host declining to say. + +Behaviour change to expect: a host that exports no `NODE_ENV` and serves `/discovery` +through `@objectstack/rest` now advertises `environment: "production"` where it +previously advertised `"development"`. A deployment that genuinely is development should +say so — `NODE_ENV=development` — which is what the runtime dispatcher has already +required since #5673. + +The mapping table above `NODE_ENV_TO_DISCOVERY_ENVIRONMENT` is corrected in the same +pass: its `unset / anything else -> development` row had been false for the runtime +caller since #5673 and is now two rows, one per rule. diff --git a/content/docs/protocol/kernel/http-protocol.mdx b/content/docs/protocol/kernel/http-protocol.mdx index dc3c97423c..2b6c12104a 100644 --- a/content/docs/protocol/kernel/http-protocol.mdx +++ b/content/docs/protocol/kernel/http-protocol.mdx @@ -187,6 +187,15 @@ field is machine-readable — a client uses it to decide whether it is talking t `development`. **An unrecognised spelling** (`qa`, `preview`, `uat`) is a different case: it is a guess, and this field never claims production on a guess. +This table is the whole answer for **every** producer of `/discovery` (#5936). The mapping +and the unset default both live in one shared function, so the dispatcher and the +`@objectstack/metadata-protocol` builder served by `@objectstack/rest` cannot disagree, and +a future producer inherits the same answers without copying anything. Until #5936 the unset +default lived at the dispatcher's own call site, so a deployment with no `NODE_ENV` was +advertised as `production` there and `development` through `@objectstack/rest`. If you read +`environment` from a REST-served `/discovery` and relied on the old answer, set +`NODE_ENV=development` explicitly. + Local development is unaffected: `os dev` runs `serve --dev`, which sets `NODE_ENV=development` in-process before the runtime loads. Anything that boots the runtime *without* `os dev` — a bare `os serve`, an embedded host, a hand-written container entry diff --git a/packages/metadata-protocol/src/discovery-schema-conformance.test.ts b/packages/metadata-protocol/src/discovery-schema-conformance.test.ts index 20c7ec36f0..e5a62b3c44 100644 --- a/packages/metadata-protocol/src/discovery-schema-conformance.test.ts +++ b/packages/metadata-protocol/src/discovery-schema-conformance.test.ts @@ -29,7 +29,7 @@ // from the schema instead of a hand-listed array is what stops this gate // from becoming a third dialect of the contract. -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, afterEach } from 'vitest'; import { ApiRoutesSchema, DiscoverySchema, @@ -197,6 +197,87 @@ describe('[#4828] getDiscovery() conforms to DiscoverySchema', () => { expect(['production', 'sandbox', 'development']).toContain(discovery.environment); }); + // ═══════════════════════════════════════════════════════════════════════════ + // [#5936] The unset-NODE_ENV default, asserted on THIS producer + // ═══════════════════════════════════════════════════════════════════════════ + // + // `/discovery` has two producers. #5673 ruled that a deployment whose operator + // never set `NODE_ENV` must not call itself `development` — `environment` is + // machine-readable and a client may loosen a destructive action's + // confirmation on it — but that dispatch was scoped to the runtime dispatcher + // and forbade touching `packages/spec`, so the default landed at that + // producer's own call site and THIS producer went on answering `development` + // for the same input. The 2026-08-07 ruling (direction 1) folded the default + // into `resolveDiscoveryEnvironment`, which is what makes one decision reach + // both. + // + // This case is the half a mapper test cannot cover: that this producer passes + // the operator's value through AS READ and adds no default of its own. Its + // sibling lives in `packages/runtime/src/discovery-schema-conformance.test.ts` + // and asserts the identical fact about the dispatcher — the pair is what makes + // "the two producers agree" a checked fact rather than a comment. + // + // Reverse verification, direction predicted BEFORE running — and the + // interesting part is that the two revert shapes go DIFFERENT ways: + // + // * Revert the whole change (the mapper back to `development` for a + // non-string AND the dispatcher back to `getEnv('NODE_ENV', 'production')`) + // and the two unset rows HERE go RED reading `development` while the + // runtime's sibling rows stay GREEN. That asymmetry IS the bug #5936 + // reports, reproduced on demand. + // * Revert only the mapper and BOTH producers go red, because the + // dispatcher no longer carries a default of its own to fall back on — + // which is the point of the consolidation, stated as a test outcome. + // + // The unrecognised-spelling rows stay green under every revert; they are + // #4828's rule, which this change deliberately leaves alone. + describe('[#5936] NODE_ENV defaults are the shared mapper\'s decision, not this producer\'s', () => { + const OLD_NODE_ENV = process.env.NODE_ENV; + afterEach(() => { + if (OLD_NODE_ENV === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = OLD_NODE_ENV; + }); + + it.each([ + ['unset', undefined], + // `NODE_ENV=` exports an empty string; the mapper reads a blank value as + // "the host did not say", the same absence `os serve` / `os doctor` and + // the runtime producer already read as production. + ['empty', ''], + ])('NODE_ENV %s advertises production — never development', async (_label, raw) => { + if (raw === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = raw; + + const discovery: any = await makeImpl().getDiscovery(); + + expect(discovery.environment).toBe('production'); + expect(DiscoverySchema.safeParse(discovery).success).toBe(true); + }); + + it.each(['qa', 'preview', 'nonsense'])( + 'NODE_ENV=%s is an unrecognised spelling — still development, never production (#4828)', + async (raw) => { + process.env.NODE_ENV = raw; + + const discovery: any = await makeImpl().getDiscovery(); + + expect(discovery.environment).toBe('development'); + }, + ); + + it.each([ + ['test', 'development'], + ['staging', 'sandbox'], + ['production', 'production'], + ])('NODE_ENV=%s advertises %s — the same table the dispatcher reads', async (raw, expected) => { + process.env.NODE_ENV = raw; + + const discovery: any = await makeImpl().getDiscovery(); + + expect(discovery.environment).toBe(expected); + }); + }); + it('reports a `locale` block derived from the i18n service when one is registered', async () => { const services = new Map([ ['i18n', { diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 8031298444..2c8e0afaa4 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -2897,6 +2897,15 @@ export class ObjectStackProtocolImplementation implements name, /** @deprecated Use `name`. Removed in protocol 18 (#4828). */ apiName: name, + // [#5936] The operator's value, passed as read — no local default. + // What an ABSENT `NODE_ENV` advertises is decided once, inside + // `resolveDiscoveryEnvironment` (`production`, per the 2026-08-07 + // ruling, direction 1), so this producer and the runtime dispatcher + // cannot drift on it. Before that ruling the default lived at the + // dispatcher's own call site and this producer had no equivalent, so + // a deployment that forgot the variable was told `development` here + // and `production` there — the exact drift the shared mapper exists + // to prevent (#4828). Do not re-introduce a default here. environment: resolveDiscoveryEnvironment( (globalThis as { process?: { env?: Record } }) .process?.env?.NODE_ENV, diff --git a/packages/runtime/src/discovery-schema-conformance.test.ts b/packages/runtime/src/discovery-schema-conformance.test.ts index 747b653bef..3c487e0542 100644 --- a/packages/runtime/src/discovery-schema-conformance.test.ts +++ b/packages/runtime/src/discovery-schema-conformance.test.ts @@ -275,23 +275,37 @@ describe('[#4828] getDiscoveryInfo() conforms to DiscoverySchema', () => { expect(DiscoverySchema.safeParse(info).success).toBe(true); }); - // [#5673] The pin for this issue, and the reason it is driven through the - // REAL producer rather than through `resolveDiscoveryEnvironment` alone: - // the UNSET default is decided at THIS call site (`getEnv`'s second - // argument), so a green mapper test in `packages/spec` cannot see it. The - // whole setup is deleting the variable — that is precisely the state of a - // production deployment whose operator never set it. + // [#5673] The pin for that issue: the whole setup is deleting the variable + // — precisely the state of a production deployment whose operator never set + // it — driven through the REAL producer rather than through + // `resolveDiscoveryEnvironment` alone. // - // Reverse verification, direction predicted BEFORE running: restore the old - // `getEnv('NODE_ENV', 'development')` and these two cases go RED (they read - // `development`), while every `it.each` row above stays green — the old - // default was only ever consulted when NODE_ENV was absent, so nothing that - // sets it can detect the change. Measured both ways. + // [#5936] It stays here, and driven end-to-end, for a reason that survived + // the default moving. When #5673 landed, the default WAS this call site + // (`getEnv`'s second argument) and a green mapper test in `packages/spec` + // could not have seen it. The 2026-08-07 ruling folded the default into the + // mapper, so the spec-side case now covers the decision itself — and this + // one covers the wiring: that this producer passes the operator's value + // through as read and adds no default of its own. A local default here + // would satisfy the mapper's test and still be the drift #5936 removed; + // only an end-to-end assertion can tell the two apart. Its sibling in + // `@objectstack/metadata-protocol` asserts the same fact about the other + // producer, which is the pair that makes "one decision" checkable. + // + // Reverse verification, direction predicted BEFORE running: restore the + // mapper's pre-#5936 `development` default and these two cases go RED + // reading `development`, while every `it.each` row above stays green, + // because the default is only ever consulted when NODE_ENV is absent. Note + // this producer no longer has a second place to be fixed: the call site + // carries no default, so the mapper's answer IS this producer's answer. + // Measured both ways. it.each([ ['unset', undefined], - // `getEnv` collapses `''` to its default (`process.env[key] || default`), - // so `NODE_ENV=` is the same absence as never exporting it — and the same - // absence `doctorNodeEnv()` and `os serve` already read as production. + // `NODE_ENV=` exports an empty string. `getEnv` collapses it to its + // default (`process.env[key] || default`) and, since #5936, the mapper + // reads a blank string as unset too — so this is the same absence + // `doctorNodeEnv()` and `os serve` already read as production, and it + // answers the same on both producers. ['empty', ''], ])('NODE_ENV %s advertises production — never development (#5673)', async (_label, raw) => { if (raw === undefined) delete process.env.NODE_ENV; diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index 3e42ee1790..322e3d3c22 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -1293,24 +1293,31 @@ export class HttpDispatcher { // enum on a machine-readable surface. The mapping table and the // reasoning per row live with the enum, in `@objectstack/spec/api`. // - // [#5673] The DEFAULT — what this producer says when the host set no - // `NODE_ENV` at all — flipped from `development` to `production` per - // the maintainer's 2026-08-06 ruling. Two facts made the old default - // the wrong one: + // [#5673] The DEFAULT — what a producer says when the host set no + // `NODE_ENV` at all — is `production`, per the maintainer's + // 2026-08-06 ruling, because `environment` is a MACHINE-READABLE + // field: a client reads it to answer "am I talking to production?" + // and may skip production warnings or loosen a destructive action's + // confirmation on the answer. Of the two ways to be wrong, claiming + // `development` on a real production deployment whose operator + // forgot the variable is the dangerous one. (Every other reader of + // that absence already said `production`: `os start` forces + // `NODE_ENV='production'` when unset, `os serve` resolves its + // `.env*` cascade for `NODE_ENV || 'production'`, `os doctor` + // derives the same expression. Discovery was the one surface + // reading it the other way.) // - // • Every other reader of the same absence already said - // `production`. `os start` forces `NODE_ENV='production'` when - // unset (`packages/cli/src/commands/start.ts:248`), `os serve` - // resolves its `.env*` cascade for `NODE_ENV || 'production'` - // (`serve.ts:532-533`), and `os doctor` derives the identical - // expression (`doctor.ts` `doctorNodeEnv()`). Discovery was the - // one surface reading that absence the other way. - // • `environment` is a MACHINE-READABLE field: a client reads it to - // answer "am I talking to production?" and may skip production - // warnings or loosen a destructive action's confirmation on the - // answer. Of the two ways to be wrong here, claiming - // `development` on a real production deployment whose operator - // forgot the variable is the dangerous one. + // [#5936] That default no longer lives HERE. #5673's ruling put + // `packages/spec` out of scope, so this producer carried the default + // at its own call site — and the second producer (`getDiscovery()` + // in `@objectstack/metadata-protocol`, served by + // `@objectstack/rest`) went on answering `development` for the unset + // case, which is the drift the shared mapper was built to prevent + // (#4828). The 2026-08-07 ruling (direction 1) folded the default + // into `resolveDiscoveryEnvironment`, so both producers now inherit + // one decision and the next producer gets it without remembering to + // copy a line. Pass the operator's value as read; do NOT re-add a + // local default here or anywhere else. // // #4828's rule is untouched and is a DIFFERENT rule: a value that IS // set but is not a spelling this repo recognises (`qa`, `preview`) @@ -1318,17 +1325,7 @@ export class HttpDispatcher { // ever CLAIMS production on a guess. Absence is not a guess — it is // the host declining to say, and the conservative answer to that is // `production`. - // - // The default is passed as `getEnv`'s second argument rather than - // moved into `resolveDiscoveryEnvironment` because the mapper lives - // in `@objectstack/spec`, which this issue's ruling put out of scope. - // Consequence, stated rather than hidden: the second discovery - // producer (`getDiscovery()` in `@objectstack/metadata-protocol`, - // served by `@objectstack/rest`) passes a genuinely-absent - // `NODE_ENV` straight into the mapper and therefore still answers - // `development` for the unset case. Filed as a follow-up (#5936); - // do not "fix" it by re-defaulting a consumer somewhere else. - environment: resolveDiscoveryEnvironment(getEnv('NODE_ENV', 'production')), + environment: resolveDiscoveryEnvironment(getEnv('NODE_ENV')), routes, // [#4828] `endpoints` (a verbatim duplicate of `routes`, commented // "Alias for backward compatibility with some clients") and the diff --git a/packages/spec/src/api/discovery.test.ts b/packages/spec/src/api/discovery.test.ts index 50596c9ce7..6c045f13fa 100644 --- a/packages/spec/src/api/discovery.test.ts +++ b/packages/spec/src/api/discovery.test.ts @@ -1181,9 +1181,25 @@ describe('[#4828] resolveDiscoveryEnvironment (decision 4 — enum, not passthro expect(resolveDiscoveryEnvironment('STAGING')).toBe('sandbox'); }); - it('never CLAIMS production for an unset or unrecognized value', () => { - for (const raw of [undefined, null, '', 'qa', 'preview', 'nonsense']) { - expect(resolveDiscoveryEnvironment(raw as any), String(raw)).toBe('development'); + // [#5936] These were ONE case until the 2026-08-07 ruling folded the unset + // default into this mapper (direction 1). They are two different rules and + // they now point opposite ways, so they are two cases: absence is the host + // declining to answer and resolves conservatively to `production`; a spelling + // this repo does not recognise is a GUESS and never claims production. + // Collapsing them back into one is the regression these two guard. + it('an UNSET value advertises production — the host declined to say (#5673, #5936)', () => { + // Blank counts as unset: `NODE_ENV=` exports an empty string, and the + // runtime's `getEnv` has always folded that into its default. Were it + // treated as "anything else" the two producers would drift again on exactly + // that input — the drift this consolidation ends. + for (const raw of [undefined, null, '', ' ']) { + expect(resolveDiscoveryEnvironment(raw as any), JSON.stringify(raw)).toBe('production'); + } + }); + + it('never CLAIMS production for an unrecognized spelling (#4828)', () => { + for (const raw of ['qa', 'preview', 'nonsense']) { + expect(resolveDiscoveryEnvironment(raw), raw).toBe('development'); } }); diff --git a/packages/spec/src/api/discovery.zod.ts b/packages/spec/src/api/discovery.zod.ts index 7786cf197e..5c79d54268 100644 --- a/packages/spec/src/api/discovery.zod.ts +++ b/packages/spec/src/api/discovery.zod.ts @@ -311,11 +311,32 @@ export type DiscoveryEnvironment = z.input; * | `development`, `dev` | `development` | exact / short spelling | * | `test` | `development` | ephemeral developer-class run (vitest/CI), not a provisioned pre-production copy | * | `staging` | `sandbox` | pre-production and production-LIKE; certainly not `production`, and `sandbox` is the enum's pre-production member | - * | unset / anything else | `development` | preserves the pre-existing `getEnv('NODE_ENV', 'development')` default, and never CLAIMS production on a guess | + * | unset / blank | `production` | the host declined to say; every other reader of that absence already says `production`, and of the two ways to be wrong, calling a real production deployment `development` is the dangerous one (#5673, #5936) | + * | anything else | `development` | an unrecognised spelling is a GUESS, and this function never claims `production` on a guess (#4828) | * - * The last row is the safety-relevant one: an unknown spelling degrades to - * `development`, so this function can never advertise `production` for an - * environment it failed to recognise. + * The last two rows carry the whole safety argument, and they point opposite + * ways on purpose. An unrecognised spelling (`qa`, `preview`) degrades to + * `development`, so nothing here ever advertises `production` for an + * environment it failed to recognise. **Absence is not a guess** — it is the + * host declining to answer, and the conservative response to that is + * `production`: `environment` is machine-readable, and a client may skip + * production warnings or loosen a destructive action's confirmation on it. + * + * The unset row moved from `development` to `production` in #5673 (maintainer + * ruling 2026-08-06) and moved HERE, into the shared mapper, in #5936 (ruling + * 2026-08-07, direction 1). #5673 could only reach its own producer — the + * runtime dispatcher, which flipped the default at its call site — so the + * second producer (`getDiscovery()` in `@objectstack/metadata-protocol`, served + * by `@objectstack/rest`) went on passing a genuinely absent `NODE_ENV` in and + * answering `development`. One default in one place is what stops that: a + * producer cannot forget to copy a line it never has to write, so the next + * discovery producer inherits the right answer. + * + * "Unset" includes a **blank** value, not only an absent one. `NODE_ENV=` + * exports an empty string, and the runtime's `getEnv('NODE_ENV', …)` has always + * folded that into its default (it tests with `||`). Had this treated blank as + * "anything else" the two producers would have drifted again on exactly that + * input — the drift this consolidation exists to end. */ const NODE_ENV_TO_DISCOVERY_ENVIRONMENT: Readonly> = { production: 'production', @@ -335,12 +356,20 @@ const NODE_ENV_TO_DISCOVERY_ENVIRONMENT: Readonly