diff --git a/.changeset/22163-doctor-node-env-source-posture.md b/.changeset/22163-doctor-node-env-source-posture.md new file mode 100644 index 00000000000..fb12844be36 --- /dev/null +++ b/.changeset/22163-doctor-node-env-source-posture.md @@ -0,0 +1,11 @@ +--- +'@objectstack/cli': patch +--- + +fix(cli): `os doctor` no longer tells a freshly scaffolded project that it is running in production (#22163) + +Clause-②: no + +- **What changed.** With `NODE_ENV` unset, in a directory whose `objectstack.config.ts` boots as itself, the `NODE_ENV` row now reads `✓ Not set — os dev runs this project as development; os start forces production`, with no fix text. That covers a config alone, and a config beside its own compiled `dist/objectstack.json`, which the first `os dev` writes. Before, the row read `⚠ Not set — this environment is being treated as production` and, under `--verbose`, told the operator to set `NODE_ENV=development`, which `os dev` already sets. +- **What did not change.** In every other directory, the `⚠ … treated as production` warning and its fix text are the same as before. That includes a config beside `OS_ARTIFACT_URL` or beside an `OS_ARTIFACT_PATH` naming another artifact, an artifact with no config, and a directory with neither. `doctor` reads the two variables the way `os dev` does: from the process environment over the `.env*` files loaded for development. +- **Nothing else moves.** The `Environment files` row still reports the `.env*` cascade for `node_env=production`. The `/discovery` `environment` default is unchanged, and so is the way `os serve`, `os start` and `os dev` resolve the mode. A `NODE_ENV` that is set still prints no row, and `doctor`'s exit code is unchanged. diff --git a/packages/cli/src/commands/doctor-env-provenance.test.ts b/packages/cli/src/commands/doctor-env-provenance.test.ts index c44afaa9d13..d85da1eef51 100644 --- a/packages/cli/src/commands/doctor-env-provenance.test.ts +++ b/packages/cli/src/commands/doctor-env-provenance.test.ts @@ -327,11 +327,23 @@ describe('DOCTOR_ENV_INPUTS — the declaration a new env-derived check must joi // nothing, add it to a prose allowance here and say why; if doctor reads // it, declare it in DOCTOR_ENV_INPUTS so its provenance is reported. const PROSE_ONLY = new Set(); + // [#22163] Read by `nodeEnvSourcePosture` only to choose WHICH of the two + // NODE_ENV rows prints; no row reports either value. Not declared above on + // purpose: DOCTOR_ENV_INPUTS is attributed against the cascade doctor + // resolves for doctorNodeEnv() (node_env=production), while these two are + // read through the one `os dev` loads (node_env=development), so a line in + // the `Environment files` row would name the wrong file. And setting either + // can only move that choice back to the #5673 row, whose text does not + // depend on them. Every OTHER name still has to be declared. + const POSTURE_ONLY = new Set(['OS_ARTIFACT_URL', 'OS_ARTIFACT_PATH']); for (const name of named) { - if (PROSE_ONLY.has(name)) continue; + if (PROSE_ONLY.has(name) || POSTURE_ONLY.has(name)) continue; expect(DOCTOR_ENV_INPUTS as readonly string[]).toContain(name); } + // The allowance cannot outlive its reader: a name doctor.ts stopped using + // leaves this set, rather than waving through a future read of it. + for (const name of POSTURE_ONLY) expect(named).toContain(name); // Guard the guard: a regex that matched nothing would pass vacuously. expect(named.size).toBeGreaterThan(0); }); diff --git a/packages/cli/src/commands/doctor-node-env-default.test.ts b/packages/cli/src/commands/doctor-node-env-default.test.ts index 829a6028162..a3bf2aa6103 100644 --- a/packages/cli/src/commands/doctor-node-env-default.test.ts +++ b/packages/cli/src/commands/doctor-node-env-default.test.ts @@ -36,6 +36,17 @@ * * The `/discovery` half of the same ruling lives in * `packages/runtime/src/discovery-schema-conformance.test.ts`. + * + * ── #22163: one posture where the row reads as development ────────────── + * + * The maintainer's direction on #22163: beside an `objectstack.config.ts` with + * no artifact from elsewhere — the source checkout `os dev` serves — an unset + * `NODE_ENV` reads as development; everywhere else #5673's row stands. So the + * cases below that pin the `warning` are the cases OUTSIDE that posture (an + * empty temp directory, or an explicit `sourcePosture: false`), and the + * `#22163` blocks pin the posture itself and the row it selects. The new row's + * wording is pinned only by the subjects it names (`os dev`, `os start`) and by + * the production sentence being absent. */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; @@ -44,7 +55,7 @@ import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import Doctor, { nodeEnvCheck, doctorNodeEnv } from './doctor.js'; +import Doctor, { nodeEnvCheck, nodeEnvSourcePosture, doctorNodeEnv } from './doctor.js'; /** `packages/cli` — the oclif root the real command is loaded against below. */ const CLI_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); @@ -127,6 +138,98 @@ describe('[#5673] nodeEnvCheck — the row, and when it exists', () => { }); }); +describe('[#22163] nodeEnvCheck — which row the posture selects', () => { + it('in the source posture: `ok`, naming `os dev` and `os start`, with no fix', () => { + const finding = nodeEnvCheck({} as NodeJS.ProcessEnv, { sourcePosture: true })!; + + expect(finding.name).toBe('NODE_ENV'); + // `ok` renders `✓` and can never reach `hasWarnings` / `hasErrors`. + expect(finding.status).toBe('ok'); + expect(finding.message).toContain('os dev'); + expect(finding.message).toContain('os start'); + expect(finding.message).not.toContain('treated as production'); + // Nothing to fix, so nothing for `--verbose` to expand. + expect(finding.fix).toBeUndefined(); + }); + + it("outside it: the #5673 warning and its fix, exactly what every existing caller gets", () => { + // The default IS "outside": a caller that never asked about the posture — + // every caller before #22163 — keeps the conservative row. The describe + // block above pins that row's halves; this pins that the explicit `false` + // is the same object, field for field. + const explicit = nodeEnvCheck({} as NodeJS.ProcessEnv, { sourcePosture: false }); + expect(explicit).toEqual(nodeEnvCheck({} as NodeJS.ProcessEnv)); + expect(explicit!.status).toBe('warning'); + expect(explicit!.fix).toBeDefined(); + }); + + it('a SET NODE_ENV prints no row in either posture', () => { + for (const sourcePosture of [true, false]) { + for (const value of ['production', 'development']) { + expect(nodeEnvCheck({ NODE_ENV: value } as NodeJS.ProcessEnv, { sourcePosture }), `${value}/${sourcePosture}`) + .toBeUndefined(); + } + } + }); +}); + +describe('[#22163] nodeEnvSourcePosture — the source checkout `os dev` serves', () => { + let dir: string; + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-22163-posture-')); + }); + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + const write = (rel: string, body: string) => { + fs.mkdirSync(path.dirname(path.join(dir, rel)), { recursive: true }); + fs.writeFileSync(path.join(dir, rel), body); + }; + const NO_ENV = {} as NodeJS.ProcessEnv; + const CONFIG = 'export default {};\n'; + + it('a config alone is the source posture', () => { + write('objectstack.config.ts', CONFIG); + expect(nodeEnvSourcePosture(dir, NO_ENV)).toBe(true); + }); + + it('…and stays so beside its OWN compiled `dist/objectstack.json` — the file the first `os dev` writes', () => { + write('objectstack.config.ts', CONFIG); + write('dist/objectstack.json', '{}\n'); + expect(nodeEnvSourcePosture(dir, NO_ENV)).toBe(true); + }); + + it('a config beside a NAMED artifact is not: OS_ARTIFACT_PATH elsewhere, or OS_ARTIFACT_URL', () => { + write('objectstack.config.ts', CONFIG); + const elsewhere = path.join(dir, 'release', 'objectstack.json'); + expect(nodeEnvSourcePosture(dir, { OS_ARTIFACT_PATH: elsewhere } as NodeJS.ProcessEnv)).toBe(false); + expect(nodeEnvSourcePosture(dir, { OS_ARTIFACT_URL: 'https://example.invalid/a.json' } as NodeJS.ProcessEnv)) + .toBe(false); + }); + + it('an artifact with no config, and a directory with neither, are not', () => { + expect(nodeEnvSourcePosture(dir, NO_ENV)).toBe(false); + write('dist/objectstack.json', '{}\n'); + expect(nodeEnvSourcePosture(dir, NO_ENV)).toBe(false); + }); + + it('reads the artifact variables where `os dev` does: over the node_env=development `.env*` cascade', () => { + // `os dev` loads `.env*` for development BEFORE it resolves the rungs, so a + // reference committed to `.env.development` drives its boot — and is a named + // artifact here too. + write('objectstack.config.ts', CONFIG); + write('.env.development', 'OS_ARTIFACT_URL=https://example.invalid/a.json\n'); + expect(nodeEnvSourcePosture(dir, NO_ENV)).toBe(false); + }); + + it('…and not over the production one, which `os dev` never loads', () => { + write('objectstack.config.ts', CONFIG); + write('.env.production', 'OS_ARTIFACT_URL=https://example.invalid/a.json\n'); + expect(nodeEnvSourcePosture(dir, NO_ENV)).toBe(true); + }); +}); + describe('[#5673] os doctor, end to end — the row reaches the report', () => { /** * `node_modules/` exists in the temp cwd on purpose — without it doctor's @@ -224,4 +327,91 @@ describe('[#5673] os doctor, end to end — the row reaches the report', () => { expect(verbose.out).toContain('NODE_ENV=development'); expect(verbose.out).toContain('NODE_ENV=production'); }, E2E_TIMEOUT); + + // ── #22163 — the posture decides which row, in the real report ────────── + // + // The case above IS the "empty directory" leg: its temp cwd holds only + // `node_modules/`, so it is outside the source posture and keeps the + // `warning`. The legs below add a config to the same cwd. + describe('[#22163] the source checkout `os dev` serves reads as development', () => { + const ARTIFACT_VARS = ['OS_ARTIFACT_URL', 'OS_ARTIFACT_PATH'] as const; + let savedArtifactVars: Record = {}; + + beforeEach(() => { + savedArtifactVars = Object.fromEntries(ARTIFACT_VARS.map((k) => [k, process.env[k]])); + for (const k of ARTIFACT_VARS) delete process.env[k]; + // The scaffold's own first line. Nothing under this temp cwd can resolve + // it (its `node_modules/` is empty), so the config LOAD fails — on + // purpose: the posture is read from the file's presence, never from a + // load, and these legs prove it by passing beside a failed one. + fs.writeFileSync( + path.join(tmp, 'objectstack.config.ts'), + "import { defineStack } from '@objectstack/spec';\nexport default defineStack({});\n", + ); + }); + + afterEach(() => { + for (const k of ARTIFACT_VARS) { + if (savedArtifactVars[k] === undefined) delete process.env[k]; + else process.env[k] = savedArtifactVars[k]; + } + }); + + /** The report rows whose name column is `NODE_ENV`. */ + const nodeEnvRows = (out: string) => out.split('\n').filter((line) => /^\s*\S+\s+NODE_ENV {2,}/.test(line)); + + it('(a) a config alone: the development row, beside a config that did not load', async () => { + delete process.env.NODE_ENV; + + const run = await runDoctor(); + + const rows = nodeEnvRows(run.out); + expect(rows).toHaveLength(1); + expect(rows[0]).toContain('✓'); + expect(rows[0]).toContain('os dev'); + expect(rows[0]).toContain('os start'); + expect(run.out).not.toContain('treated as production'); + // The load really failed, so the row above was not decided by it. + expect(run.out).toContain('Could not load config for analysis'); + expect(run.exitCode).toBeUndefined(); + }, E2E_TIMEOUT); + + it('(b) …and still after the first `os dev` compiled it to `dist/objectstack.json`', async () => { + delete process.env.NODE_ENV; + fs.mkdirSync(path.join(tmp, 'dist')); + fs.writeFileSync(path.join(tmp, 'dist', 'objectstack.json'), '{}\n'); + + const run = await runDoctor(); + + const rows = nodeEnvRows(run.out); + expect(rows).toHaveLength(1); + expect(rows[0]).toContain('os dev'); + expect(run.out).not.toContain('treated as production'); + expect(run.exitCode).toBeUndefined(); + }, E2E_TIMEOUT); + + it('(c) a config beside an OS_ARTIFACT_PATH naming another artifact: the #5673 warning', async () => { + delete process.env.NODE_ENV; + process.env.OS_ARTIFACT_PATH = path.join(tmp, 'release', 'objectstack.json'); + + const run = await runDoctor(); + + const rows = nodeEnvRows(run.out); + expect(rows).toHaveLength(1); + expect(rows[0]).toContain('⚠'); + expect(rows[0]).toContain('treated as production'); + expect(run.exitCode).toBeUndefined(); + }, E2E_TIMEOUT); + + it('(e) NODE_ENV set in the source posture: no row at all', async () => { + for (const value of ['development', 'production']) { + process.env.NODE_ENV = value; + + const run = await runDoctor(); + + expect(nodeEnvRows(run.out), value).toHaveLength(0); + expect(run.exitCode).toBeUndefined(); + } + }, E2E_TIMEOUT * 2); + }); }); diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index 77fad88c99d..e38feeb4a35 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -10,6 +10,7 @@ import { normalizeStackInput } from '@objectstack/spec'; import { referenceCarrierOf } from '@objectstack/spec/data'; import { printHeader, printSuccess, printWarning, printError, printStep, printInfo } from '../utils/format.js'; import { loadConfig, configExists } from '../utils/config.js'; +import { cwdConfigJoinsBoot, resolveArtifactBootSource } from '../utils/artifact-precedence.js'; import { checkProtocolVersionGap } from '../utils/protocol-version-gap.js'; // #5644 — "the optional package is not installed" and "it is installed and // will not load" are two facts, and one `catch` around `import()` cannot tell @@ -164,8 +165,10 @@ export function doctorNodeEnv(env: NodeJS.ProcessEnv = process.env): string { } /** - * Say out loud that `NODE_ENV` is unset — and that the whole stack is therefore - * treating this environment as **production** (#5673). + * Say out loud that `NODE_ENV` is unset, and what that means where doctor + * stands: a deployment is being treated as **production** (#5673); the source + * checkout `os dev` serves runs as development under `os dev` and as production + * under `os start` (#22163). * * `undefined` when the variable is set, so a configured environment gets no row * at all. That is the same shape the tenancy-posture finding has: a value doctor @@ -192,6 +195,32 @@ export function doctorNodeEnv(env: NodeJS.ProcessEnv = process.env): string { * it starts in the mode this row names. `error` is what turns doctor's summary * into `process.exit(1)`, and an unset `NODE_ENV` must not fail a health check. * + * ── Except in the source checkout `os dev` serves (#22163) ─────────────── + * + * The argument above is about a DEPLOYMENT that forgot the variable, and it + * stands everywhere but one place. Beside an `objectstack.config.ts` that boots + * as itself, with no artifact from anywhere else ({@link nodeEnvSourcePosture}), + * the maintainer's direction on #22163 is that an unset `NODE_ENV` reads as + * development. That directory is a project, not a deployment: the command it + * is run with is `os dev`, which runs it as development whatever the shell says + * (its `serve --dev` child sets the variable before any runtime module loads), + * and the only way it becomes production is `os start`, which forces it. A + * newcomer thirty seconds after scaffolding was told they were running + * production — and, under `--verbose`, to set what `os dev` already sets — by + * a row about a posture their project is not in. + * + * So there the row is `ok` (this file's only informational status — see + * {@link scheduledWorkCheck} for why the union is not widened for one row), + * names the two commands and what each does, and carries no `fix`: nothing + * needs fixing. In every other posture it is #5673's `warning` and `fix`, byte + * for byte — including a config beside a NAMED artifact, where a production + * deployment that forgot the variable is exactly what #5673 must still catch. + * + * What the posture does NOT change: {@link doctorNodeEnv} and the `.env*` + * cascade doctor resolves (still `production`, which the `Environment files` + * row keeps saying), the `/discovery` default, and how `os serve`, `os start` + * and `os dev` resolve the mode. Only what this row says, and when. + * * ── Deliberately NOT in `DOCTOR_ENV_INPUTS`, and not read through the overlay ─ * * That list is for variables whose value doctor resolves through the `.env*` @@ -203,11 +232,27 @@ export function doctorNodeEnv(env: NodeJS.ProcessEnv = process.env): string { * * "Unset" here is `!env.NODE_ENV`, character for character the condition under * which {@link doctorNodeEnv} falls back to its default. The row therefore - * appears exactly when the default was taken, which is the only claim it makes. + * appears exactly when the default was taken, in either posture; the posture + * decides only which of the two rows that is. + * + * @param opts.sourcePosture {@link nodeEnvSourcePosture}'s answer for the + * directory doctor runs in. Defaults to `false` — the #5673 row — so a caller + * that has not asked gets the conservative answer. */ -export function nodeEnvCheck(env: NodeJS.ProcessEnv = process.env): HealthCheckResult | undefined { +export function nodeEnvCheck( + env: NodeJS.ProcessEnv = process.env, + opts: { sourcePosture?: boolean } = {}, +): HealthCheckResult | undefined { if (env.NODE_ENV) return undefined; + if (opts.sourcePosture) { + return { + name: 'NODE_ENV', + status: 'ok', + message: 'Not set — os dev runs this project as development; os start forces production', + }; + } + return { name: 'NODE_ENV', status: 'warning', @@ -227,6 +272,60 @@ export function nodeEnvCheck(env: NodeJS.ProcessEnv = process.env): HealthCheckR }; } +/** + * [#22163] Is `cwd` the source checkout `os dev` serves — a cwd + * `objectstack.config.ts` that boots as itself, with no artifact from anywhere + * else? The posture {@link nodeEnvCheck} reads an unset `NODE_ENV` as + * development in. + * + * Answered by THE precedence (`utils/artifact-precedence.ts`), never by a copy + * of it: {@link resolveArtifactBootSource} for the artifact rungs, then + * {@link cwdConfigJoinsBoot} for whether the cwd config takes part — the two + * questions `os dev` asks before it prints its `Config:` row (`dev.ts`), asked + * the way it asks them, with three deliberate details: + * + * • The config is a FILE TEST — `objectstack.config.ts` in `cwd`, the one + * file `os dev` tests for — never a load. A config that cannot load here + * (dependencies not installed yet, a top-level throw) is still the project + * `os dev` would compile, and the load failure already has its own row + * further down the report. + * • The environment is the one `os dev` resolves those rungs in: this process + * over the `.env*` cascade for node_env=development, which `os dev` loads + * before it asks, the shell winning as dotenv-flow has it. That is a second, + * private read of the files: the `Environment files` row still reports the + * cascade doctor resolves for {@link doctorNodeEnv}, which is unchanged. + * • No `homeDir` (the `home-dist` rung is `os start`'s alone; `os dev` passes + * none) and no `configCompiledTo`. `os dev` compiles a config INTO a local + * OS_ARTIFACT_PATH and so counts that file as the config's own; this + * posture does not, because #22163 keeps #5673's row wherever an artifact + * is named. The one artifact it forgives is the config's own compiled + * output at the conventional `dist/objectstack.json` — the file the first + * `os dev` writes, so without it the very project #22163 is about would + * fall back to the production row after its first `os dev`. + * + * So a config alone, or beside its own `dist/objectstack.json`, is the source + * posture; a config beside OS_ARTIFACT_URL or beside an OS_ARTIFACT_PATH naming + * any other artifact, an artifact with no config, and a directory with neither + * are not. + * + * The answer moves in one direction only: setting either variable can take + * the posture AWAY from source and never toward it (unset already answers + * source wherever the conventional path would), so whatever the `.env*` read + * supplies can only put the row back to its #5673 text. + */ +export function nodeEnvSourcePosture(cwd: string, env: NodeJS.ProcessEnv = process.env): boolean { + const configPath = path.resolve(cwd, 'objectstack.config.ts'); + const devEnv: NodeJS.ProcessEnv = { ...env, ...dotenvOverlay(readDotenvFiles(cwd, 'development'), env) }; + const bootSource = resolveArtifactBootSource({ env: devEnv, cwd }); + return cwdConfigJoinsBoot({ + configExists: fs.existsSync(configPath), + configPath, + artifact: bootSource.kind === 'reference' ? { kind: 'reference' } + : bootSource.kind === 'resolved' ? { kind: 'path', path: bootSource.path } + : { kind: 'none' }, + }); +} + /** * [#17396] The EFFECTIVE value of the package-authored scheduled-work switch, * reported unconditionally. @@ -2083,12 +2182,17 @@ export default class Doctor extends Command { // change every such verdict has four possible sources. results.push(environmentSourcesCheck(dotenvReading)); - // #5673 — the mode the row above was resolved FOR, when nobody chose it. - // Placed immediately after the sources row because it answers the question - // that row raises: `node_env=production` appears there whether the operator - // set NODE_ENV or not, and only this row tells the two apart. Only the unset - // case produces a row; a configured environment's report is unchanged. - const nodeEnvFinding = nodeEnvCheck(); + // #5673 / #22163 — what an unset NODE_ENV means here. Placed immediately + // after the sources row because it answers the question that row raises: + // `node_env=production` appears there whether the operator set NODE_ENV or + // not, and only this row tells the two apart. In a deployment it says the + // production default was taken; in the source checkout `os dev` serves it + // says that project runs as development under `os dev` and as production + // under `os start` — and the sources row above still says + // `node_env=production` there, because that is the cascade doctor resolved + // and #22163 changes only this row. Only the unset case produces a row; a + // configured environment's report is unchanged. + const nodeEnvFinding = nodeEnvCheck(process.env, { sourcePosture: nodeEnvSourcePosture(cwd) }); if (nodeEnvFinding) { results.push(nodeEnvFinding); }