From 5ced825dfa3a8d5031560e9344288b188fdf8a44 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 03:47:56 +0000 Subject: [PATCH] fix(spec,metadata-protocol,runtime): one place decides the unset-NODE_ENV discovery environment (#5936) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #5936. Direction 1 of the maintainer's 2026-08-07 ruling (decision-inbox round 2), which returned the issue to the queue as a placement question after #5673 had already settled the semantics on 2026-08-06. ## Premise, re-verified against origin/main (3a1d9c7) Every line the issue cites is where it says, and the defect reproduces: - `resolveDiscoveryEnvironment` at `packages/spec/src/api/discovery.zod.ts:341` returned `development` for a non-string input — so `undefined` in. - `packages/metadata-protocol/src/protocol.ts:2900` (`getDiscovery`) passed `process.env.NODE_ENV` in raw, with no default of its own. Served by `packages/rest`. - `packages/runtime/src/http-dispatcher.ts:1331` carried the #5673 default at its own call site as `getEnv('NODE_ENV', 'production')`, with a comment stating this exact residue and naming #5936 as the follow-up. - The mapping table's `unset / anything else -> development` row at `discovery.zod.ts:314` was still there, and had been false for the runtime caller since #5673. Net effect on main: a production deployment whose operator forgot `NODE_ENV` was advertised `production` on the dispatcher's `/discovery` and `development` on the one served through `@objectstack/rest`. Same question, two answers. ## What lands The default moves INTO the shared mapper: an absent value resolves to `production`, and both producers pass the operator's value through as read. The dispatcher's local default is deleted; metadata-protocol never had one to delete and simply inherits the corrected mapper. #4828's rule is untouched and still points the other way — a set-but-unrecognised spelling (`qa`, `preview`) degrades to `development`, because absence is the host declining to answer while an unrecognised spelling is a guess. One judgement call the ruling did not spell out, decided and recorded rather than left implicit: **"unset" includes a blank value**. `NODE_ENV=` exports an empty string; the runtime's `getEnv` has always folded that into its default (it tests with `||`), and #5673's own pin already asserts the dispatcher answers `production` for it. Had the mapper treated blank as "anything else", the two producers would have gone on disagreeing about exactly that input — the drift this consolidation exists to end — so blank is unset. Stated in the TSDoc, in the changeset, and pinned on both producers. Direction 2 (patch metadata-protocol locally) was the alternative and is what the ruling rejected: it would have left a second copy of the default, which is what the shared table was created to prevent (#4828). ## Documentation, both faces of it - `discovery.zod.ts`: the `unset / anything else` row splits into two rows, one per rule, each with its reason, plus the note that the unset default moved here in #5936 and why blank counts as unset. - `http-protocol.mdx` already carried the corrected table from #5673 — but it describes the dispatcher, and until now the table was true of that producer only. It now states that the mapping and the default are shared by every `/discovery` producer, and names the behaviour change for anyone reading `environment` from a REST-served response. - `http-dispatcher.ts`: the comment block that explained *why the default sits at this call site* is replaced by one explaining why it no longer does, and telling the next author not to re-add one. - `protocol.ts`: a short comment at the call site making the same point from the other side — this producer adds no default; the mapper decides. ## Reverse verification — two revert shapes, opposite outcomes, both predicted first Predicted, then measured: - **Full revert to main's shape** (mapper back to `development` for a non-string AND the dispatcher back to `getEnv('NODE_ENV', 'production')`): metadata-protocol's two unset rows go RED, runtime stays **23/23 GREEN**. That asymmetry IS the bug this issue reports, reproduced on demand. Measured: `Tests 2 failed | 17 passed (19)` in metadata-protocol, `Tests 23 passed (23)` in runtime, `Tests 1 failed | 77 passed (78)` in spec's mapper suite. - **Mapper-only revert** (dispatcher keeps its local default deleted): BOTH producers go red — measured `2 failed | 17 passed (19)` and `2 failed | 21 passed (23)`. That is the consolidation stated as a test outcome: the dispatcher no longer has a second place to be right. The unrecognised-spelling rows stayed green under both reverts, as predicted — they are #4828's rule and this change does not touch it. Both source files were restored and verified byte-identical with `diff` afterwards. ## Tests - `packages/spec/src/api/discovery.test.ts`: the single case "never CLAIMS production for an unset or unrecognized value" split into two, because the two rules now point opposite ways. The unset case covers `undefined`, `null`, `''` and `' '`. - `packages/metadata-protocol/src/discovery-schema-conformance.test.ts`: a new block driving the REAL producer — unset and blank advertise `production`, unrecognised spellings still `development`, and the shared table (`test`/`staging`/`production`) reproduced end-to-end so the two producers' suites assert the same facts. - `packages/runtime/src/discovery-schema-conformance.test.ts`: pins unchanged and still green (this producer's behaviour does not move); its comment is rewritten, since the sentence explaining that a spec-side test "cannot see" the default stopped being true the moment the default moved there. Two end-to-end pins rather than one mapper test on purpose: a local default re-added at either call site would satisfy the mapper's own test and still be the drift #5936 removes. Only a producer-level assertion separates them. ## Verification (all foreground, real readings) - `pnpm --filter @objectstack/spec test` -> 340 files, **8727 passed** - `pnpm --filter @objectstack/metadata-protocol test` -> 55 files, **571 passed** - `pnpm --filter @objectstack/runtime test` -> 110 files, **1606 passed** - `pnpm --filter @objectstack/rest test` -> 64 files, **884 passed** - `pnpm --filter @objectstack/objectql test` -> 146 files, **2448 passed** - `pnpm --filter @objectstack/client test` -> 21 files, **263 passed** - `pnpm --filter @objectstack/hono test` -> 2 files, **73 passed** - `turbo run typecheck --filter=./packages/* --filter=./packages/*/*` -> **119 successful, 119 total** - `pnpm --filter @objectstack/spec check:generated` -> **10/10 up to date** (nothing regenerated: no `.describe()`, no export and no authorable key moved) - `pnpm --filter @objectstack/spec gen:openapi` -> ran (ungated generator); writes only the gitignored `json-schema/openapi.json`, no tracked diff - `pnpm lint` -> clean - `check:nul-bytes` / `check:empty-changeset` / `check:adr-0087-registration` / `check:route-envelope` / `check:error-code-casing` / `check:wildcard-fallthrough` / `check:adr-anchors` / `check:doc-authoring` / `check:docs-audit-scope` / `check:quick-reference-counts` / `check:startup-registry-verdict` / `check:service-providers` / `check:spec-parsed-alias` -> all green `check:adr-0087-registration` confirms no declared-breaking changeset, so no disposition marker is owed. The changeset covers all three packages at `patch` and states the behaviour change an operator can observe. One first-run red that was NOT this change: `@objectstack/client` reported 3 failed suites with "Failed to resolve entry for package `@objectstack/runtime`" — a fresh worktree with runtime's `dist` unbuilt (AGENTS.md §9's stale-artefact trap in mirror image). `pnpm --filter '@objectstack/client...' build` fixed it and the suite went 21/21. ## Ruling mapping | Ruling clause | Where it lands | |:---|:---| | unset -> `production`, folded into `resolveDiscoveryEnvironment` | `discovery.zod.ts` — the mapper body | | producers drop their local defaults | `http-dispatcher.ts` (deleted); `protocol.ts` had none and now says so | | correct the stale mapping-table comment | `discovery.zod.ts` — one row becomes two, each with its rule | | pin both producers under unset `NODE_ENV` | new block in metadata-protocol's conformance suite; runtime's #5673 pins kept and re-commented | | one PR across spec + metadata-protocol + runtime | this commit | | changeset covering the three packages, patch | `.changeset/discovery-environment-single-default.md` | ## Out-of-scope findings (recorded, not fixed) 1. `packages/rest` carries its own `discovery-schema-conformance.test.ts` and is the SERVER for the metadata-protocol producer, but it has no NODE_ENV block of its own — it composes over `getDiscovery()`, so the new pins do cover the behaviour it serves. Worth knowing if a third shape ever grows there. 2. `getEnv` (`packages/core/src/utils/env.ts:15`) collapses an empty string into its default via `||`, which is right for `NODE_ENV` but is a repo-wide policy applied to every variable — an `OS_*` value that is legitimately the empty string cannot be expressed. Not touched here; noted because this change depends on that behaviour at one call site. ## 交付摘要(PM 开 PR 素材) - **前提**:对 origin/main 3a1d9c7 逐条复核成立 —— `discovery.zod.ts:341` 非字符串返回 `development`、`protocol.ts:2900` 原样递传、`http-dispatcher.ts:1331` 本地默认连同「本条残留待 #5936」的注释俱在、:314 那行过时表格行仍在。缺陷可复现:同一台忘设 NODE_ENV 的部署,两个生产者给出两个答案。 - **落地**(裁决方向 1):缺省判定收进共享映射(unset → `production`),runtime 删本地默认,metadata-protocol 本无默认、随映射自动纠正;#4828 的「不在猜测上宣称 production」不动,两条规则拆成两行各述其理。 - **裁决未明说而必须定的一点**:空串算 unset。`NODE_ENV=` 导出空串,runtime 的 `getEnv` 一向用 `||` 把它折进默认,#5673 的 pin 也已断言该情形为 production;若映射把空串归入「anything else」,两个生产者就会在这一个输入上继续分歧 —— 正是本次收敛要消灭的漂移。已写进 TSDoc、changeset 并在两侧各 pin 一条。 - **反向验证**:两种回退形态,方向先判后跑,结果相反且都命中 —— 整体回退到 main 形态:metadata-protocol 红 2、runtime 23/23 全绿(**这就是 #5936 报告的不对称本身**);仅回退映射:两侧各红 2(说明 runtime 已无第二处可依赖)。未识别拼写行在两种回退下均保持绿。两个源文件事后 diff 验证逐字节还原。 - **文档面**:`discovery.zod.ts` 表格行订正;`http-protocol.mdx` 原表自 #5673 起就是对的,但只对 dispatcher 成立 —— 现补明该表对**所有** `/discovery` 生产者成立,并写清经 `@objectstack/rest` 读 `environment` 的调用方会看到的行为变化。 - **验证读数**:spec 8727、metadata-protocol 571、runtime 1606、rest 884、objectql 2448、client 263、hono 73 全绿;`turbo typecheck` 119/119;生成物 10/10 且本次无一需要重生成;lint 与门禁族全绿。client 首跑 3 个 suite 红是新 worktree 未建 runtime dist(AGENTS §9 陈旧产物陷阱的镜像),补建后 21/21,与本改动无关。 - **界外发现**两条见上,均未夹带。 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011btrhv6sHn6JkN93YRtGQp --- .../discovery-environment-single-default.md | 42 ++++++++++ .../docs/protocol/kernel/http-protocol.mdx | 9 ++ .../src/discovery-schema-conformance.test.ts | 83 ++++++++++++++++++- packages/metadata-protocol/src/protocol.ts | 9 ++ .../src/discovery-schema-conformance.test.ts | 42 ++++++---- packages/runtime/src/http-dispatcher.ts | 53 ++++++------ packages/spec/src/api/discovery.test.ts | 22 ++++- packages/spec/src/api/discovery.zod.ts | 43 ++++++++-- 8 files changed, 250 insertions(+), 53 deletions(-) create mode 100644 .changeset/discovery-environment-single-default.md 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