fix(spec,metadata-protocol,runtime): one place decides the unset-NODE_ENV discovery environment

.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.

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

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<string, any>([
       ['i18n', {

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<string, string | undefined> } })
                     .process?.env?.NODE_ENV,

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;

packages/runtime/src/http-dispatcher.ts

@@ -1293,42 +1293,39 @@ 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`)
             // still degrades to `development` inside the mapper, so nothing here
             // 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

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