From 9f28d58496a33fe8eddf15e742010aa142203ce2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 07:26:52 +0000 Subject: [PATCH] fix(cli): `os doctor` reads serve's `.env*` cascade and attributes every value (#5387) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `serve` / `dev` / `start` load `.env*` through dotenv-flow before reading a single `OS_*` variable (serve.ts:520, dev.ts, start.ts); `doctor` loaded none, so a posture committed to a shared `.env` reached the server and never reached the diagnostic — doctor green, `os serve` refusing to boot the same directory. Doctor now resolves the same cascade dotenv-flow picks for serve (node_env derived as `NODE_ENV || production`, serve's expression minus the `--dev` flag doctor does not have), without merging it into `process.env`: the overlay is applied around the read that needs it and removed in a `finally`. A new always-on `Environment files` check reports which files were loaded and where each declared input came from — source only, never the value — and the posture finding's `.env` sentence now states what was read instead of what was skipped. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VkPSGsX9o17MsGv3Lbxu2w --- .changeset/doctor-dotenv-provenance.md | 64 +++ .../commands/doctor-env-provenance.test.ts | 483 ++++++++++++++++++ .../doctor-tenancy-posture-report.test.ts | 94 +++- packages/cli/src/commands/doctor.ts | 360 ++++++++++++- 4 files changed, 964 insertions(+), 37 deletions(-) create mode 100644 .changeset/doctor-dotenv-provenance.md create mode 100644 packages/cli/src/commands/doctor-env-provenance.test.ts diff --git a/.changeset/doctor-dotenv-provenance.md b/.changeset/doctor-dotenv-provenance.md new file mode 100644 index 0000000000..6fc1ae2a4a --- /dev/null +++ b/.changeset/doctor-dotenv-provenance.md @@ -0,0 +1,64 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os doctor` 按 `os serve` 的顺序读 `.env*`,并逐值注明来源(#5387) + +`serve` / `dev` / `start` 三条命令都在读第一个 `OS_*` 变量之前用 dotenv-flow 载入 +`.env*`(`serve.ts:520`、`dev.ts`、`start.ts`);`doctor` **一个都不载入** —— +`grep -n dotenv packages/cli/src/commands/doctor.ts` 此前只匹配到一句「我不载入」的注释。 +于是被诊断的环境和诊断者看到的环境,不是同一个环境: + +``` +# .env,提交进仓库、团队共享 +OS_TENANCY_POSTURE=isolatd + +$ os doctor # 看不到 .env,posture 解析成 single,报绿,exit 0 +$ os serve # 载入 .env,读到 isolatd,FATAL 拒绝启动(PR #5381 的闸门) +``` + +`.env` 恰恰是这个变量最常见的来源 —— PR #5381 把 serve 的闸门刻意放在 dotenv 载入 +**之后**,理由正是这个。在 #5382 之前 doctor 没有任何 env 派生检查,这条不一致不产生 +可观察差异;#5382 加了第一项(posture),它才有了落点,也才成为「诊断面与运行时不一致」 +(#4801 / cloud#1020 家族)在 `.env` 这条来源上的残留。 + +**现在的行为。** doctor 用 dotenv-flow 自己的 `listFiles()` 取到 `os serve` 会载入的 +同一份文件清单(`node_env` 按 serve 同款推导:`NODE_ENV || production`;doctor 没有 +`--dev`,serve 表达式里的 `'test'` 分支在 `flags.dev` 为假时是恒等的),逐个文件解析, +让每个变量记住它是在**哪个文件**里胜出的。两条约束是这次改动的实质: + +- **不静默合并进 `process.env`。** 文件里的值只在需要它的那一次读取周围套上( + `withDotenvOverlay`),`finally` 里再摘掉 —— 用的是 dotenv-flow 自己 `unload()` 的判定 + (只删掉仍然等于写入值的那些),所以回调故意改写的值不会被误删。运行中后续的一切 + (config 打包、外部命令)不会莫名其妙继承一个跟昨天不一样的环境。 +- **逐值注明来源。** 报告新增一行常驻体检项,说明载入了哪些文件、以及每个 env 输入来自 + shell 还是哪个文件: + +``` + ✓ Environment files .env, .env.production (node_env=production), the cascade `os serve` loads — OS_TENANCY_POSTURE from .env.production, OS_MULTI_ORG_ENABLED from .env +``` + +只静默合并、不注明来源,只是把盲区从「doctor 没读我的 `.env`」平移成「doctor 到底信了我 +四个 `.env*` 里的哪一个」—— 同一类缺陷往前挪一层。该行**只报来源、从不报值**,所以 +`DOCTOR_ENV_INPUTS` 将来加入带密钥的变量也不会因此泄露(唯一被打印的值是**非法**的 +`OS_TENANCY_POSTURE`,由 posture 那条 finding 原样引回给作者看自己的拼写)。 + +**posture 那条 finding 的文案随之改写。** #5382 写的是「unlike `os serve`, `os doctor` +does not load `.env*` files」—— 在当时如实,也正是 #5387 被开出来的原因。现在同一个位置 +说的是它**读到了什么**: + +``` + Read from .env.production — `os doctor` loaded the same `.env*` cascade + `os serve` does (node_env=production: .env, .env.production). +``` + +优先级与 serve 一致:shell 里已存在的变量胜过所有文件(dotenv-flow 用 `hasOwnProperty` +判定,所以 shell 里显式写空 `OS_TENANCY_POSTURE=` 也胜过 `.env` 里的值),cascade 里靠后 +的文件胜过靠前的。 + +**一个如实说明的影响面:** 本改动改变的是 doctor **每一项 env 派生检查**的输入。今天落在 +两处 —— posture 报告,以及 ADR-0120 D5e 的 unique-scope 建议(它只在 posture 为 +`isolated` 时运行,所以一个只写在 `.env` 里的 `isolated` 现在会让它在 doctor 里跑起来, +和 `os serve` 一致)。这正是本单要修的东西,不是副作用。用户配置文件( +`objectstack.config.ts`)自身读 `process.env` 的那条路径**没有**套上 overlay,以免改变既有 +config 检查的判定,另行记录。 diff --git a/packages/cli/src/commands/doctor-env-provenance.test.ts b/packages/cli/src/commands/doctor-env-provenance.test.ts new file mode 100644 index 0000000000..e59cdfe6e1 --- /dev/null +++ b/packages/cli/src/commands/doctor-env-provenance.test.ts @@ -0,0 +1,483 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os doctor` reads the `.env*` cascade `os serve` reads, and attributes every + * value it read (#5387). + * + * ── The defect ─────────────────────────────────────────────────────────── + * + * `serve` / `dev` / `start` load `.env*` through dotenv-flow before reading a + * single `OS_*` variable (`serve.ts:520`, `dev.ts`, `start.ts`). `doctor` loaded + * none — `git grep -n dotenv packages/cli/src/commands/doctor.ts` matched only a + * comment saying so. So the environment the diagnostic judged and the + * environment the diagnosed command runs in were not the same environment: + * + * # .env, committed to the repo and shared by the team + * OS_TENANCY_POSTURE=isolatd + * + * $ os doctor # never read .env → posture resolves to single → exit 0 + * $ os serve # loads .env → FATAL, refuses to boot (PR #5381's gate) + * + * Until #5382 doctor had zero env-derived checks, so this produced no + * observable difference; #5382 added the first one and this became the residual + * half of "doctor disagrees with serve" (#4801 / cloud#1020's family). + * + * ── What is pinned here ────────────────────────────────────────────────── + * + * Both halves of the PM ruling on #5387, because either alone is a defect: + * + * 1. doctor mirrors serve's READ ORDER — dotenv-flow, `node_env` derived the + * way serve derives it, files beating each other in dotenv-flow's own + * precedence and the shell beating all of them; + * 2. every value is REPORTED WITH ITS SOURCE, and the files are NOT merged + * into `process.env` for the run. A silent merge would only move the blind + * spot: from "doctor never read my `.env`" to "which of my four `.env*` + * files did doctor believe?", with the added surprise of every later + * reader in the process inheriting a changed environment. + * + * The shell-sourced half of the posture finding stays in + * `doctor-tenancy-posture-report.test.ts` (#5382), whose call sites this change + * amended. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import Doctor, { + DOCTOR_ENV_INPUTS, + doctorNodeEnv, + readDotenvFiles, + provenanceOf, + effectiveEnvValue, + describeProvenance, + withDotenvOverlay, + environmentSourcesCheck, + resolveTenancyPostureOrFinding, +} from './doctor.js'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +/** `packages/cli` — the oclif root the real command is loaded against below. */ +const CLI_ROOT = path.resolve(HERE, '..', '..'); + +/** + * The escape is written as `\x1b`, never as the byte itself: one raw control + * character makes `grep` treat the whole file as binary, and a test file no + * `git grep` can find stops being maintained (#4890 / #5157). + */ +const SGR = /\x1b\[[0-9;]*m/g; +const plain = (s: string) => s.replace(SGR, ''); + +/** Every variable these cases touch, restored between them. */ +const TOUCHED = ['OS_TENANCY_POSTURE', 'OS_MULTI_ORG_ENABLED', 'OS_5387_PROBE'] as const; +let saved: Record = {}; +let dir: string; + +beforeEach(() => { + saved = Object.fromEntries(TOUCHED.map((k) => [k, process.env[k]])); + for (const k of TOUCHED) delete process.env[k]; + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5387-')); +}); + +afterEach(() => { + for (const k of TOUCHED) { + if (saved[k] === undefined) delete process.env[k]; + else process.env[k] = saved[k]; + } + fs.rmSync(dir, { recursive: true, force: true }); +}); + +const write = (name: string, body: string) => fs.writeFileSync(path.join(dir, name), body); + +describe('doctorNodeEnv — the mode the cascade is resolved for', () => { + it("is serve's derivation with `--dev` out of the picture: NODE_ENV || production", () => { + // serve.ts:517-519 reads + // flags.dev ? 'development' : (NODE_ENV === 'test' ? 'test' : (NODE_ENV || 'production')) + // and doctor has no `--dev` flag (the PM ruling on #5387 declined to invent + // one for this first version). With `flags.dev` false the 'test' branch is a + // no-op, so these two really are the same expression. + expect(doctorNodeEnv({} as NodeJS.ProcessEnv)).toBe('production'); + expect(doctorNodeEnv({ NODE_ENV: '' } as NodeJS.ProcessEnv)).toBe('production'); + expect(doctorNodeEnv({ NODE_ENV: 'development' } as NodeJS.ProcessEnv)).toBe('development'); + expect(doctorNodeEnv({ NODE_ENV: 'test' } as NodeJS.ProcessEnv)).toBe('test'); + expect(doctorNodeEnv({ NODE_ENV: 'staging' } as NodeJS.ProcessEnv)).toBe('staging'); + }); +}); + +describe("readDotenvFiles — serve's file list, with per-key attribution", () => { + it("lists only files that exist, in dotenv-flow's ascending priority order", () => { + write('.env', 'A=base\n'); + write('.env.production', 'A=prod\n'); + + const reading = readDotenvFiles(dir, 'production'); + + expect(reading.files.map((f) => path.basename(f))).toEqual(['.env', '.env.production']); + expect(reading.nodeEnv).toBe('production'); + // Later file wins — the same merge `dotenvFlow.parse(list)` performs, which + // is why the list is walked in this order rather than reversed. + expect(reading.fileValues.get('A')).toBe('prod'); + expect(path.basename(reading.fileOrigin.get('A')!)).toBe('.env.production'); + }); + + it("keeps each variable pointing at the file it won in, not at the last file read", () => { + write('.env', 'SHARED=base\nONLY_BASE=1\n'); + write('.env.production', 'SHARED=prod\n'); + + const reading = readDotenvFiles(dir, 'production'); + + // The whole deliverable of #5387 in one assertion: two variables, two + // different source files, one report. A merged parse cannot say this. + expect(path.basename(reading.fileOrigin.get('SHARED')!)).toBe('.env.production'); + expect(path.basename(reading.fileOrigin.get('ONLY_BASE')!)).toBe('.env'); + }); + + it("skips `.env.local` for node_env=test, exactly as dotenv-flow does for serve", () => { + write('.env', 'A=base\n'); + write('.env.local', 'A=local\n'); + + // dotenv-flow deliberately excludes `.env.local` under "test" so a test run + // is the same for everyone. Doctor inherits that by CALLING dotenv-flow's + // `listFiles()` rather than reimplementing the naming convention — a + // hand-rolled list would have to remember this exception, and would drift. + expect(readDotenvFiles(dir, 'test').files.map((f) => path.basename(f))).toEqual(['.env']); + expect(readDotenvFiles(dir, 'development').files.map((f) => path.basename(f))) + .toEqual(['.env', '.env.local']); + }); + + it('reports an unreadable file instead of throwing out of the whole diagnostic', () => { + // A directory named `.env` is the cheap portable stand-in for "readFileSync + // fails": `os serve` loads with `silent: true` and boots without those + // values, so doctor must not be the one that dies here. + fs.mkdirSync(path.join(dir, '.env')); + + const reading = readDotenvFiles(dir, 'production'); + expect(reading.error).toBeDefined(); + expect(path.basename(reading.error!.file)).toBe('.env'); + }); +}); + +describe("provenanceOf — serve's precedence, reported rather than merged", () => { + it("gives the shell priority over every file, matching dotenv-flow's `load()`", () => { + write('.env', 'OS_TENANCY_POSTURE=group\n'); + process.env.OS_TENANCY_POSTURE = 'isolated'; + + const reading = readDotenvFiles(dir, 'production'); + expect(provenanceOf(reading, 'OS_TENANCY_POSTURE')).toEqual({ + name: 'OS_TENANCY_POSTURE', + source: 'shell', + }); + expect(effectiveEnvValue(reading, 'OS_TENANCY_POSTURE')).toBe('isolated'); + }); + + it('treats an explicitly EMPTY shell value as set — `hasOwnProperty`, not truthiness', () => { + write('.env', 'OS_TENANCY_POSTURE=group\n'); + process.env.OS_TENANCY_POSTURE = ''; + + // dotenv-flow's `load()` skips any name `process.env` already HAS, so + // `OS_TENANCY_POSTURE=` in the shell really does beat a populated `.env` + // under `os serve`. Testing truthiness here instead would give doctor a + // second, quieter precedence rule — and it would disagree with the server + // on exactly the environments people get wrong. + const reading = readDotenvFiles(dir, 'production'); + expect(provenanceOf(reading, 'OS_TENANCY_POSTURE').source).toBe('shell'); + expect(effectiveEnvValue(reading, 'OS_TENANCY_POSTURE')).toBe(''); + }); + + it('names the winning FILE when the shell has nothing, and `unset` when nobody does', () => { + write('.env', 'OS_TENANCY_POSTURE=group\n'); + write('.env.production', 'OS_TENANCY_POSTURE=isolated\n'); + + const reading = readDotenvFiles(dir, 'production'); + const p = provenanceOf(reading, 'OS_TENANCY_POSTURE'); + expect(p.source).toBe('file'); + expect(path.basename(p.file!)).toBe('.env.production'); + expect(effectiveEnvValue(reading, 'OS_TENANCY_POSTURE')).toBe('isolated'); + + expect(provenanceOf(reading, 'OS_MULTI_ORG_ENABLED')).toEqual({ + name: 'OS_MULTI_ORG_ENABLED', + source: 'unset', + }); + }); +}); + +describe('withDotenvOverlay — bounded, reverted, never a background merge', () => { + it('exposes file values to the callback and removes them again afterwards', () => { + write('.env', 'OS_5387_PROBE=from-file\n'); + const reading = readDotenvFiles(dir, 'production'); + + expect(process.env.OS_5387_PROBE).toBeUndefined(); + const seen = withDotenvOverlay(reading, () => process.env.OS_5387_PROBE); + expect(seen).toBe('from-file'); + + // The half that separates this from `dotenvFlow.config()`: after the read, + // the process is exactly as it was. Nothing later in the run — the config + // bundle, a spawned tool — silently inherits a different environment. + expect(Object.prototype.hasOwnProperty.call(process.env, 'OS_5387_PROBE')).toBe(false); + }); + + it('never overwrites a value the shell already set, and leaves it alone on the way out', () => { + write('.env', 'OS_5387_PROBE=from-file\n'); + process.env.OS_5387_PROBE = 'from-shell'; + const reading = readDotenvFiles(dir, 'production'); + + expect(withDotenvOverlay(reading, () => process.env.OS_5387_PROBE)).toBe('from-shell'); + expect(process.env.OS_5387_PROBE).toBe('from-shell'); + }); + + it('reverts even when the callback throws — the posture resolver throws by design', () => { + write('.env', 'OS_5387_PROBE=from-file\n'); + const reading = readDotenvFiles(dir, 'production'); + + expect(() => withDotenvOverlay(reading, () => { throw new Error('boom'); })).toThrow('boom'); + // `resolveTenancyPosture()` refuses an unrecognized value BY THROWING, so + // the throwing path is the normal path here, not the exotic one. A `finally` + // that only ran on success would leak the overlay on exactly the runs that + // report a problem. + expect(Object.prototype.hasOwnProperty.call(process.env, 'OS_5387_PROBE')).toBe(false); + }); + + it('leaves a variable the callback deliberately changed alone', () => { + write('.env', 'OS_5387_PROBE=from-file\n'); + const reading = readDotenvFiles(dir, 'production'); + + withDotenvOverlay(reading, () => { process.env.OS_5387_PROBE = 'rewritten'; }); + // dotenv-flow's own `unload()` test: delete only what still holds the value + // that was written. Blind deletion would silently undo a caller's write. + expect(process.env.OS_5387_PROBE).toBe('rewritten'); + }); +}); + +describe('environmentSourcesCheck — the report line that makes the merge non-silent', () => { + it('names the files it loaded, the mode, and where each declared input came from', () => { + write('.env', 'OS_MULTI_ORG_ENABLED=true\n'); + write('.env.production', 'OS_TENANCY_POSTURE=isolated\n'); + const reading = readDotenvFiles(dir, 'production'); + + const check = environmentSourcesCheck(reading, {} as NodeJS.ProcessEnv); + expect(check.status).toBe('ok'); + const text = plain(`${check.message}\n${check.fix ?? ''}`); + + expect(text).toContain('.env, .env.production'); + expect(text).toContain('node_env=production'); + expect(text).toContain('OS_TENANCY_POSTURE from .env.production'); + expect(text).toContain('OS_MULTI_ORG_ENABLED from .env'); + }); + + it('reports the SOURCE and not the VALUE — so a future input may carry a secret', () => { + write('.env', 'OS_TENANCY_POSTURE=isolated\n'); + const reading = readDotenvFiles(dir, 'production'); + + const check = environmentSourcesCheck(reading, {} as NodeJS.ProcessEnv); + const text = plain(`${check.message}\n${check.fix ?? ''}`); + + // `DOCTOR_ENV_INPUTS` is meant to grow as doctor gains env-derived checks, + // and the next variable added may well be a credential. Printing the source + // and never the contents is what makes growing that list safe by + // construction rather than by everyone remembering. + expect(text).toContain('OS_TENANCY_POSTURE from .env'); + expect(text).not.toContain('isolated'); + }); + + it('says so plainly when there is nothing to load', () => { + const check = environmentSourcesCheck(readDotenvFiles(dir, 'production'), {} as NodeJS.ProcessEnv); + expect(check.status).toBe('ok'); + expect(plain(check.message)).toContain('No .env* files here'); + expect(plain(check.message)).toContain('no environment input set'); + }); + + it('warns — without failing the run — when a `.env*` file cannot be read', () => { + fs.mkdirSync(path.join(dir, '.env')); + const check = environmentSourcesCheck(readDotenvFiles(dir, 'production'), {} as NodeJS.ProcessEnv); + + // Warning, not error: `os serve` boots without those values (it loads with + // `silent: true`), so the environment does start. Doctor's contribution is + // saying out loud what serve swallows. + expect(check.status).toBe('warning'); + expect(plain(check.message)).toContain('could not be read'); + }); + + it('reports every declared input, including the ones nobody set', () => { + const check = environmentSourcesCheck(readDotenvFiles(dir, 'production'), {} as NodeJS.ProcessEnv); + for (const name of DOCTOR_ENV_INPUTS) { + expect(plain(check.fix ?? '')).toContain(`${name} not set`); + } + }); + + it("describes the three sources in the operator's vocabulary", () => { + const reading = readDotenvFiles(dir, 'production'); + expect(describeProvenance(reading, { name: 'X', source: 'shell' as const })) + .toContain("this process's environment"); + expect(describeProvenance(reading, { name: 'X', source: 'unset' as const })).toContain('not set'); + expect(describeProvenance(reading, { name: 'X', source: 'file' as const, file: path.join(dir, '.env') })) + .toBe('X from .env'); + }); +}); + +describe('DOCTOR_ENV_INPUTS — the declaration a new env-derived check must join', () => { + it('declares every `OS_*` variable named anywhere in doctor.ts (drift guard)', () => { + const source = fs.readFileSync(path.join(HERE, 'doctor.ts'), 'utf-8'); + const named = new Set(source.match(/OS_[A-Z0-9_]+/g) ?? []); + + // The failure mode this guards is specific: someone adds an env-derived + // check, reads the variable, and the value now silently arrives from a + // `.env` with nothing in the report saying so — #5387 reintroduced one + // variable at a time. If a name below is only PROSE and doctor reads + // 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(); + + for (const name of named) { + if (PROSE_ONLY.has(name)) continue; + expect(DOCTOR_ENV_INPUTS as readonly string[]).toContain(name); + } + // Guard the guard: a regex that matched nothing would pass vacuously. + expect(named.size).toBeGreaterThan(0); + }); +}); + +describe('the posture finding reads .env and says which file it came from', () => { + it('finds an invalid posture that lives ONLY in a committed .env — the #5387 repro', () => { + write('.env', 'OS_TENANCY_POSTURE=isolatd\n'); + const reading = readDotenvFiles(dir, 'production'); + + const verdict = resolveTenancyPostureOrFinding(reading); + if (verdict.ok) throw new Error('expected a finding: the .env value is not a posture'); + + const text = plain(`${verdict.result.message}\n${verdict.result.fix ?? ''}`); + // Before #5387 this was `{ ok: true, posture: 'single' }` — doctor never + // read the file, so it reported a healthy environment `os serve` refuses to + // boot. + expect(verdict.result.status).toBe('error'); + expect(text).toContain('OS_TENANCY_POSTURE="isolatd"'); + // …and the attribution, which is the half a silent `dotenvFlow.config()` + // would not have given: WHICH of the files it read set this. + expect(text).toContain('Read from .env'); + expect(text).toContain('node_env=production'); + + // Reading it must not leave it behind. + expect(Object.prototype.hasOwnProperty.call(process.env, 'OS_TENANCY_POSTURE')).toBe(false); + }); + + it('attributes to the highest-priority file when several set it', () => { + write('.env', 'OS_TENANCY_POSTURE=isolated\n'); + write('.env.production', 'OS_TENANCY_POSTURE=isolatd\n'); + + const verdict = resolveTenancyPostureOrFinding(readDotenvFiles(dir, 'production')); + if (verdict.ok) throw new Error('expected a finding'); + + const text = plain(`${verdict.result.message}\n${verdict.result.fix ?? ''}`); + expect(text).toContain('Read from .env.production'); + // Naming `.env` here would send the operator to edit a file whose value + // never reaches the server. + expect(text).not.toContain('Read from .env —'); + }); + + it('lets the shell override a broken .env, because that is what `os serve` does', () => { + write('.env', 'OS_TENANCY_POSTURE=isolatd\n'); + process.env.OS_TENANCY_POSTURE = 'group'; + + // Not a leniency: `dotenvFlow.config()` does not overwrite a variable the + // shell defined, so this environment really does boot as `group`. Doctor + // reporting the `.env` typo as fatal here would be a false alarm about a + // value nothing reads. + expect(resolveTenancyPostureOrFinding(readDotenvFiles(dir, 'production'))) + .toEqual({ ok: true, posture: 'group' }); + }); + + it('derives from OS_MULTI_ORG_ENABLED in a .env when no posture is set anywhere', () => { + write('.env', 'OS_MULTI_ORG_ENABLED=true\n'); + // The legacy derivation is env-derived too, so it moved with the posture: + // before #5387 this read `single` while `os serve` read `isolated`. + expect(resolveTenancyPostureOrFinding(readDotenvFiles(dir, 'production'))) + .toEqual({ ok: true, posture: 'isolated' }); + }); +}); + +describe('os doctor, end to end, against a posture that only exists in .env', () => { + /** + * The differential that would have caught #5387, run against the real command + * in-process: one temp cwd, one variable, changed only inside `.env`. + * + * `node_modules/` exists in the temp cwd on purpose — without it doctor's + * `Dependencies` check is itself an `error` and exits 1 on its own, which + * would make the interesting assertion pass for a reason having nothing to do + * with this change (the same trap PR #5390 wrote down). + */ + let tmp: string; + let cwdSpy: ReturnType; + + beforeEach(() => { + tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5387-e2e-')); + fs.mkdirSync(path.join(tmp, 'node_modules')); + cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmp); + }); + + afterEach(() => { + cwdSpy.mockRestore(); + fs.rmSync(tmp, { recursive: true, force: true }); + }); + + async function runDoctor(): Promise<{ out: string; exitCode: number | undefined }> { + const logs: string[] = []; + const logSpy = vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { + logs.push(a.join(' ')); + }); + let exitCode: number | undefined; + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + exitCode = code; + throw new Error(`__PROCESS_EXIT__:${code}`); + }) as never); + + try { + await Doctor.run([], { root: CLI_ROOT }); + } catch (err) { + if (!(err instanceof Error) || !err.message.startsWith('__PROCESS_EXIT__')) throw err; + } finally { + logSpy.mockRestore(); + exitSpy.mockRestore(); + } + return { out: plain(logs.join('\n')), exitCode }; + } + + it('names the file, refuses to call the environment functional, and exits 1', async () => { + // ── Control: the same .env, a posture that parses ───────────────────── + fs.writeFileSync(path.join(tmp, '.env'), 'OS_TENANCY_POSTURE=isolated\n'); + const healthy = await runDoctor(); + + expect(healthy.exitCode).toBeUndefined(); + expect(healthy.out).toContain('Environment is functional'); + expect(healthy.out).not.toContain('Tenancy posture'); + // The report says what it read even when everything is fine — that is the + // "not a silent merge" half, and it is only observable on a healthy run. + expect(healthy.out).toContain('Environment files'); + expect(healthy.out).toContain('OS_TENANCY_POSTURE from .env'); + + // ── The case: one character changed, inside the file ────────────────── + fs.writeFileSync(path.join(tmp, '.env'), 'OS_TENANCY_POSTURE=isolatd\n'); + const broken = await runDoctor(); + + // Every one of these was the other way round before #5387: the value lived + // in a file doctor never opened, so the run said nothing about it and + // exited 0 while `os serve` refused to boot the same directory. + expect(broken.out).toContain('OS_TENANCY_POSTURE="isolatd"'); + expect(broken.out).toContain('is not a recognized tenancy posture'); + expect(broken.out).toContain('Read from .env'); + expect(broken.out).not.toContain('Environment is functional'); + expect(broken.exitCode).toBe(1); + + // And the retired sentence is gone from the REAL output, not just from the + // unit under test: doctor no longer tells the operator it skipped `.env*`. + expect(broken.out).not.toContain('does not\n load `.env*` files'); + + // The run must not have left the file's value in this process — the next + // command in the same process would otherwise inherit it. + expect(Object.prototype.hasOwnProperty.call(process.env, 'OS_TENANCY_POSTURE')).toBe(false); + // 60s, not the 5s default: this case runs the REAL doctor command + // in-process twice, and each run shells out to `pnpm -v`, `tsc -v` and + // `git --version`. A loaded merge-queue shard blew the 5s default for PR + // #5381's equivalent case (queue run 30971902650) and took that PR out of + // the queue. + }, 60_000); +}); diff --git a/packages/cli/src/commands/doctor-tenancy-posture-report.test.ts b/packages/cli/src/commands/doctor-tenancy-posture-report.test.ts index 5c9b55ed23..348bcef7ab 100644 --- a/packages/cli/src/commands/doctor-tenancy-posture-report.test.ts +++ b/packages/cli/src/commands/doctor-tenancy-posture-report.test.ts @@ -33,16 +33,37 @@ * Nothing, for doctor. `git grep -n OS_TENANCY_POSTURE packages/cli/src` matched * serve's prose, `verify`'s back-compat tests, and PR #5381's serve gate test — * no assertion of any kind on `doctor`. + * + * ── Amended by #5387 ───────────────────────────────────────────────────── + * + * `resolveTenancyPostureOrFinding()` now takes the `.env*` reading its verdict + * is resolved against: doctor reads the cascade `os serve` reads instead of this + * shell alone. Two things in this file legitimately changed premise and are + * updated rather than deleted: + * + * • every call site passes a reading — built by the REAL `readDotenvFiles()` + * over a real (empty) directory, so these cases keep testing shell-sourced + * values, which is what they were always about; + * • the case that pinned "doctor does not load `.env*`" pinned a sentence + * that is now false. It is replaced by its opposite — the finding must name + * what doctor DID read — with the anti-overclaim assertion kept, pointing at + * the retired sentence so it cannot come back. + * + * The `.env`-sourced half of the behaviour lives in `doctor-env-provenance.test.ts`. */ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, beforeAll, afterEach, afterAll, vi } from 'vitest'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { TENANCY_POSTURES } from '@objectstack/spec/security'; -import Doctor, { resolveTenancyPostureOrFinding } from './doctor.js'; +import Doctor, { + resolveTenancyPostureOrFinding, + readDotenvFiles, + type DotenvReading, +} from './doctor.js'; /** `packages/cli` — the oclif root the command is loaded against below. */ const CLI_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); @@ -60,6 +81,26 @@ const plain = (s: string) => s.replace(SGR, ''); const TOUCHED = ['OS_TENANCY_POSTURE', 'OS_MULTI_ORG_ENABLED'] as const; let saved: Record = {}; +/** + * A real reading of a real directory that contains no `.env*` file (#5387). + * + * Not a hand-built literal: `readDotenvFiles()` is the code under test in the + * sibling file, and a fake reading here would let these cases keep passing if + * the real one started reporting files that do not exist. + */ +let emptyDir: string; +let shellOnly: DotenvReading; + +beforeAll(() => { + emptyDir = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5387-noenv-')); + shellOnly = readDotenvFiles(emptyDir, 'production'); + expect(shellOnly.files).toEqual([]); +}); + +afterAll(() => { + fs.rmSync(emptyDir, { recursive: true, force: true }); +}); + beforeEach(() => { saved = Object.fromEntries(TOUCHED.map((k) => [k, process.env[k]])); for (const k of TOUCHED) delete process.env[k]; @@ -76,25 +117,25 @@ describe('resolveTenancyPostureOrFinding — accepted values', () => { it('passes every posture the spec vocabulary declares', () => { for (const posture of TENANCY_POSTURES) { process.env.OS_TENANCY_POSTURE = posture; - expect(resolveTenancyPostureOrFinding()).toEqual({ ok: true, posture }); + expect(resolveTenancyPostureOrFinding(shellOnly)).toEqual({ ok: true, posture }); } }); it("keeps the legacy 'multi' spelling normalizing to isolated", () => { process.env.OS_TENANCY_POSTURE = 'multi'; - expect(resolveTenancyPostureOrFinding()).toEqual({ ok: true, posture: 'isolated' }); + expect(resolveTenancyPostureOrFinding(shellOnly)).toEqual({ ok: true, posture: 'isolated' }); }); it('unset falls back to the OS_MULTI_ORG_ENABLED derivation, not to a finding', () => { - expect(resolveTenancyPostureOrFinding()).toEqual({ ok: true, posture: 'single' }); + expect(resolveTenancyPostureOrFinding(shellOnly)).toEqual({ ok: true, posture: 'single' }); process.env.OS_MULTI_ORG_ENABLED = 'true'; - expect(resolveTenancyPostureOrFinding()).toEqual({ ok: true, posture: 'isolated' }); + expect(resolveTenancyPostureOrFinding(shellOnly)).toEqual({ ok: true, posture: 'isolated' }); }); it('treats a blank value as unset — reporting it would flag `OS_TENANCY_POSTURE=` in a .env', () => { process.env.OS_TENANCY_POSTURE = ' '; - expect(resolveTenancyPostureOrFinding()).toEqual({ ok: true, posture: 'single' }); + expect(resolveTenancyPostureOrFinding(shellOnly)).toEqual({ ok: true, posture: 'single' }); }); }); @@ -107,15 +148,15 @@ describe('resolveTenancyPostureOrFinding — the finding', () => { // throw was caught by a `catch` that knows nothing about env vars and // downgraded "cannot start" to "config checks skipped". A verdict cannot be // caught by an unrelated catch. - expect(() => resolveTenancyPostureOrFinding()).not.toThrow(); + expect(() => resolveTenancyPostureOrFinding(shellOnly)).not.toThrow(); - const reading = resolveTenancyPostureOrFinding(); + const reading = resolveTenancyPostureOrFinding(shellOnly); expect(reading.ok).toBe(false); }); it('is an ERROR health check — the severity that makes doctor exit non-zero', () => { process.env.OS_TENANCY_POSTURE = 'bogus'; - const reading = resolveTenancyPostureOrFinding(); + const reading = resolveTenancyPostureOrFinding(shellOnly); if (reading.ok) throw new Error('expected a finding'); // `status: 'error'` is load-bearing, not cosmetic: doctor's display loop @@ -127,7 +168,7 @@ describe('resolveTenancyPostureOrFinding — the finding', () => { it('names the fact: the variable and the value the operator actually typed', () => { process.env.OS_TENANCY_POSTURE = 'islolated'; // a real transposition typo - const reading = resolveTenancyPostureOrFinding(); + const reading = resolveTenancyPostureOrFinding(shellOnly); if (reading.ok) throw new Error('expected a finding'); const text = plain(`${reading.result.message}\n${reading.result.fix ?? ''}`); @@ -142,7 +183,7 @@ describe('resolveTenancyPostureOrFinding — the finding', () => { it('prescribes a way out for EVERY posture the vocabulary declares (drift guard)', () => { process.env.OS_TENANCY_POSTURE = 'bogus'; - const reading = resolveTenancyPostureOrFinding(); + const reading = resolveTenancyPostureOrFinding(shellOnly); if (reading.ok) throw new Error('expected a finding'); const fix = plain(reading.result.fix ?? ''); @@ -158,7 +199,7 @@ describe('resolveTenancyPostureOrFinding — the finding', () => { it("carries the resolver's own sentence as `cause` rather than paraphrasing it", () => { process.env.OS_TENANCY_POSTURE = 'bogus'; - const reading = resolveTenancyPostureOrFinding(); + const reading = resolveTenancyPostureOrFinding(shellOnly); if (reading.ok) throw new Error('expected a finding'); // `@objectstack/types` owns the vocabulary and its wording; doctor must not @@ -166,18 +207,27 @@ describe('resolveTenancyPostureOrFinding — the finding', () => { expect(plain(reading.result.fix ?? '')).toContain('cause: Invalid OS_TENANCY_POSTURE="bogus"'); }); - it('says what it did NOT read — doctor loads no .env, so a green report is not a serve guarantee', () => { + it('says WHERE it read the value — and no longer claims it skipped `.env*` (#5387)', () => { process.env.OS_TENANCY_POSTURE = 'bogus'; - const reading = resolveTenancyPostureOrFinding(); + const reading = resolveTenancyPostureOrFinding(shellOnly); if (reading.ok) throw new Error('expected a finding'); + const fix = plain(reading.result.fix ?? ''); - // Deliberately the OPPOSITE of serve's gate text, which says it checked - // "every .env file dotenv-flow loaded". serve calls `dotenvFlow.config()`; - // doctor does not load `.env*` at all, so claiming the same coverage here - // would be false. Overclaiming by one sentence is how a diagnostic stops - // being trustworthy — the same reason PR #5381 refused to write "no port - // has been bound". - expect(plain(reading.result.fix ?? '')).toContain('does not\n load `.env*` files'); + // The premise of this case changed, it was not softened. Until #5387 doctor + // read no `.env*` at all, and this text said so — the honest sentence for + // the code as it stood, and the reason #5387 was filed. Doctor now reads + // serve's cascade, so the same slot must carry the opposite fact: this + // value came from THIS PROCESS's environment, and doctor looked in the + // files too (here: none exist in the temp dir the reading was taken from). + expect(fix).toContain("Read from this process's environment"); + expect(fix).toContain(`no \`.env*\` file exists in ${shellOnly.cwd}`); + expect(fix).toContain('node_env=production'); + + // Anti-overclaim, kept and pointed at the retired sentence: a diagnostic + // that says it did not look somewhere it now looks is as untrustworthy as + // one claiming coverage it never had. Both directions are failures. + expect(fix).not.toContain('does not\n load `.env*` files'); + expect(fix).not.toContain("Read from this process's environment only"); }); }); diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index 509cdf26b0..f326fc9ab5 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -3,6 +3,7 @@ import { Command, Flags } from '@oclif/core'; import chalk from 'chalk'; import { execSync } from 'child_process'; +import dotenvFlow from 'dotenv-flow'; import fs from 'fs'; import path from 'path'; import { normalizeStackInput } from '@objectstack/spec'; @@ -31,6 +32,314 @@ interface HealthCheckResult { fix?: string; } +// ─── Environment sources (#5387) ──────────────────────────────────── +// +// `serve` / `dev` / `start` all load `.env*` through dotenv-flow before they +// read a single `OS_*` variable (`serve.ts:520`, `dev.ts`, `start.ts`); doctor +// read none. So a value living in a committed `.env` — the most common home for +// exactly these variables, and the reason PR #5381 put serve's posture gate +// AFTER the dotenv load — reached the server and never reached the diagnostic: +// doctor green, `os serve` refusing to boot the same directory. +// +// Doctor now reads the same cascade, with two deliberate constraints: +// +// • It does NOT merge the values into `process.env` for the run. The overlay +// is applied around the individual read that needs it and taken back off in +// a `finally` (`withDotenvOverlay`), so nothing downstream — the config +// bundle, a spawned tool — silently inherits a different environment than +// the one it inherited yesterday. +// • Every env-derived value is REPORTED WITH ITS SOURCE (shell vs which +// file). A silent merge would trade "doctor cannot see your `.env`" for +// "doctor cannot tell you which of your four `.env*` files it believed", +// which is the same class of defect one layer along. + +/** + * The environment variables doctor's own checks derive from. + * + * Every entry's PROVENANCE is reported by {@link environmentSourcesCheck} — the + * variable's name and where its value came from, **never the value itself**. + * Reporting the source rather than the contents is what keeps this list safe to + * grow: a future env-derived check whose variable carries a credential can be + * declared here without doctor printing the credential. (The one value doctor + * does print is an *unrecognized* `OS_TENANCY_POSTURE`, quoted back by the + * posture finding so the operator can see their typo — a posture is a + * vocabulary word, not a secret.) + * + * A new env-derived check MUST add its variable here: `doctor-env-provenance.test.ts` + * fails when `doctor.ts` names an `OS_*` variable this list does not declare. + * Reading `.env` into a check that reports no attribution is precisely the + * defect #5387 closed, and it would otherwise creep back one variable at a time. + */ +export const DOCTOR_ENV_INPUTS = ['OS_TENANCY_POSTURE', 'OS_MULTI_ORG_ENABLED'] as const; + +/** Where one environment value actually came from. */ +export interface EnvValueProvenance { + name: string; + /** `'shell'` — this process's environment; `'file'` — a `.env*` file; `'unset'` — neither. */ + source: 'shell' | 'file' | 'unset'; + /** Absolute path of the winning `.env*` file. Only when `source === 'file'`. */ + file?: string; +} + +/** What doctor found when it read the `.env*` cascade `os serve` reads. */ +export interface DotenvReading { + /** The `node_env` the cascade was resolved for. */ + nodeEnv: string; + /** The directory the cascade was read from (doctor's cwd). */ + cwd: string; + /** Existing `.env*` files in dotenv-flow's ASCENDING priority order. */ + files: string[]; + /** varname → merged value across `files` (later file wins). */ + fileValues: Map; + /** varname → the highest-priority file that defines it. */ + fileOrigin: Map; + /** A file dotenv-flow could not read. `os serve` (silent: true) ignores this. */ + error?: { file: string; message: string }; +} + +/** + * The `node_env` doctor resolves the `.env*` cascade for. + * + * Identical to `serve.ts:517-519` with its `--dev` flag out of the picture — + * doctor has no such flag, and the PM ruling on #5387 explicitly declined to + * invent one for this first version. serve's remaining expression is + * `NODE_ENV === 'test' ? 'test' : (NODE_ENV || 'production')`, whose first + * branch is a no-op once `flags.dev` is false, so this really is the same + * derivation and not a lookalike. + * + * Read from `process.env` only, never from a `.env*` file — a `NODE_ENV` set + * inside a `.env` cannot change which files are loaded under serve either + * (serve computes the mode before the load), and mirroring that is the point. + */ +export function doctorNodeEnv(env: NodeJS.ProcessEnv = process.env): string { + return env.NODE_ENV || 'production'; +} + +/** + * Read — without loading — the `.env*` files `os serve` would load from `cwd`. + * + * `dotenvFlow.listFiles()` is the same function `dotenvFlow.config()` uses to + * pick its files, so the list is serve's list by construction rather than by a + * reimplemented naming convention. Files are parsed one at a time (instead of + * `parse(files)`, which merges them) purely so each variable keeps the name of + * the file it won in: that per-key attribution is the whole deliverable. + */ +export function readDotenvFiles(cwd: string, nodeEnv: string): DotenvReading { + const reading: DotenvReading = { + nodeEnv, + cwd, + files: [], + fileValues: new Map(), + fileOrigin: new Map(), + }; + + try { + reading.files = dotenvFlow.listFiles({ node_env: nodeEnv, path: cwd }); + } catch (err) { + reading.error = { file: cwd, message: err instanceof Error ? err.message : String(err) }; + return reading; + } + + // Ascending priority: a later file overwrites an earlier one, which is + // exactly how `dotenvFlow.parse(list)` merges them. + for (const file of reading.files) { + let parsed: Record; + try { + parsed = dotenvFlow.parse(file); + } catch (err) { + reading.error = { file, message: err instanceof Error ? err.message : String(err) }; + continue; + } + for (const [name, value] of Object.entries(parsed)) { + reading.fileValues.set(name, value); + reading.fileOrigin.set(name, file); + } + } + + return reading; +} + +/** + * Where `name`'s effective value comes from, under serve's precedence. + * + * dotenv-flow's `load()` skips any variable `process.env` already **has** — + * `hasOwnProperty`, not truthiness — so an explicitly empty shell value beats a + * populated `.env`. The same test is used here on purpose: a provenance report + * that disagreed with the loader on `FOO=` would be a second convention. + */ +export function provenanceOf( + reading: DotenvReading, + name: string, + env: NodeJS.ProcessEnv = process.env, +): EnvValueProvenance { + if (Object.prototype.hasOwnProperty.call(env, name)) return { name, source: 'shell' }; + const file = reading.fileOrigin.get(name); + if (file) return { name, source: 'file', file }; + return { name, source: 'unset' }; +} + +/** + * The value a reader would see for `name` under serve's precedence. + * + * Deliberately separate from {@link provenanceOf}: provenance is what doctor + * PRINTS, and keeping the value out of that structure is what makes the report + * safe for a future secret-bearing input. The value is fetched only where it is + * genuinely needed — quoting an unrecognized posture back at the operator. + */ +export function effectiveEnvValue( + reading: DotenvReading, + name: string, + env: NodeJS.ProcessEnv = process.env, +): string | undefined { + if (Object.prototype.hasOwnProperty.call(env, name)) return env[name]; + return reading.fileValues.get(name); +} + +/** A `.env*` path as the operator typed it — relative to cwd when it lives there. */ +function displayEnvFile(reading: DotenvReading, file: string): string { + const rel = path.relative(reading.cwd, file); + return rel && !rel.startsWith('..') && !path.isAbsolute(rel) ? rel : file; +} + +/** One line of provenance, in the operator's vocabulary. Names the source, not the value. */ +export function describeProvenance(reading: DotenvReading, provenance: EnvValueProvenance): string { + switch (provenance.source) { + case 'shell': + return `${provenance.name} from this process's environment`; + case 'file': + return `${provenance.name} from ${displayEnvFile(reading, provenance.file!)}`; + default: + return `${provenance.name} not set`; + } +} + +/** + * The attribution sentence a finding about a single variable carries: where the + * value came from, and which files were consulted to decide that. + * + * Written for the `fix` block's 6-space continuation indent. `source: 'unset'` + * cannot reach here from a finding — a variable nobody set produces no + * complaint — and is folded into the process-environment wording rather than + * given a fourth sentence nothing can print. + */ +function envSourceSentence(reading: DotenvReading, provenance: EnvValueProvenance): string { + const loaded = reading.files.map((file) => displayEnvFile(reading, file)).join(', '); + + if (provenance.source === 'file') { + return `Read from ${displayEnvFile(reading, provenance.file!)} — \`os doctor\` loaded the same \`.env*\` cascade\n` + + ` \`os serve\` does (node_env=${reading.nodeEnv}: ${loaded}).`; + } + if (reading.files.length > 0) { + return `Read from this process's environment, which overrides the \`.env*\` files doctor also\n` + + ` read (node_env=${reading.nodeEnv}: ${loaded}) — the precedence \`os serve\` resolves.`; + } + return `Read from this process's environment; no \`.env*\` file exists in ${reading.cwd}\n` + + ` (node_env=${reading.nodeEnv}), so \`os serve\` would find none here either.`; +} + +/** + * The variables `dotenvFlow.config()` would ADD to this process — i.e. the ones + * the shell has not already defined. + */ +function dotenvOverlay( + reading: DotenvReading, + env: NodeJS.ProcessEnv = process.env, +): Record { + const overlay: Record = {}; + for (const [name, value] of reading.fileValues) { + if (!Object.prototype.hasOwnProperty.call(env, name)) overlay[name] = value; + } + return overlay; +} + +/** + * Run `fn` with the `.env*` overlay in `process.env`, then take it back off. + * + * Needed because the readers doctor must agree with — `resolveTenancyPosture()` + * in `@objectstack/types` — read `process.env` themselves. Reimplementing their + * resolution over a plain map instead would give doctor a *second* copy of a + * vocabulary `@objectstack/types` owns, free to drift from the one `os serve` + * enforces; that is the failure #5382 was careful to avoid when it quoted the + * resolver's own sentence rather than paraphrasing it. + * + * So the mutation is real but bounded: only variables the shell does NOT define + * (dotenv-flow's own precedence), only for the duration of a synchronous call, + * and removed in a `finally` — using dotenv-flow's own `unload()` test, which + * deletes a variable only when it still holds the value that was written, so a + * value `fn` deliberately changed is left alone. + */ +export function withDotenvOverlay(reading: DotenvReading, fn: () => T): T { + const overlay = dotenvOverlay(reading); + const names = Object.keys(overlay); + for (const name of names) process.env[name] = overlay[name]; + try { + return fn(); + } finally { + for (const name of names) { + if (process.env[name] === overlay[name]) delete process.env[name]; + } + } +} + +/** + * The report line that says what doctor read, and where each declared + * environment input came from (#5387). + * + * Printed on every run, including a clean one. That is the point: after this + * change doctor's env-derived checks answer "what will `os serve` see", and a + * report that consults four files without naming them has only moved the + * blind spot from "doctor never read my `.env`" to "which `.env` did doctor + * believe?". + */ +export function environmentSourcesCheck( + reading: DotenvReading, + env: NodeJS.ProcessEnv = process.env, +): HealthCheckResult { + const loaded = reading.files.map((file) => displayEnvFile(reading, file)); + const provenances = DOCTOR_ENV_INPUTS.map((name) => provenanceOf(reading, name, env)); + const set = provenances.filter((p) => p.source !== 'unset'); + + const where = loaded.length > 0 + ? `${loaded.join(', ')} (node_env=${reading.nodeEnv}), the cascade \`os serve\` loads` + : `No .env* files here (node_env=${reading.nodeEnv}) — environment read from this process only`; + const attribution = set.length > 0 + ? ` — ${set.map((p) => describeProvenance(reading, p)).join(', ')}` + : ' — no environment input set'; + + const detail = + 'Where each environment input doctor reads comes from:\n' + + provenances + .map((p) => ` • ${describeProvenance(reading, p)}`) + .join('\n') + + '\n A variable set in this process wins over every `.env*` file, and a later file in\n' + + ' the cascade wins over an earlier one — the same precedence `os serve` resolves.\n' + + ' Sources only: doctor reports where a value came from, never what it is.'; + + if (reading.error) { + return { + name: 'Environment files', + status: 'warning', + message: `${displayEnvFile(reading, reading.error.file)} could not be read — its values are missing from this report`, + // serve loads with `silent: true`, so it says nothing at all about an + // unreadable file and simply boots without those values. Doctor's job is + // to say so out loud; the verdict stays a warning because the environment + // still starts. + fix: + `${reading.error.message}\n` + + ' `os serve` ignores an unreadable `.env*` file silently and boots without those\n' + + ' values, so this is a real difference between what you wrote and what runs.\n' + + ` ${detail}`, + }; + } + + return { + name: 'Environment files', + status: 'ok', + message: `${where}${attribution}`, + fix: detail, + }; +} + // ─── Tenancy Posture ──────────────────────────────────────────────── /** @@ -82,16 +391,25 @@ export type TenancyPostureReading = * The counterpart in `serve.ts` (`resolveTenancyPostureOrRefusal`, #5359) has * the same shape but a different verdict, and deliberately so: serve REFUSES * (FATAL + `process.exit(1)` before any boot work), doctor REPORTS (an `error` - * health check that flows through doctor's own error summary). The wording - * differs for the same reason — see the `.env` note below, which is true of - * doctor and false of serve. + * health check that flows through doctor's own error summary). + * + * #5387 — the INPUT is now the `.env*` cascade serve reads, not the shell + * alone. `resolveTenancyPosture()` reads `process.env` itself, so the overlay is + * applied around that one call and removed again (see {@link withDotenvOverlay}); + * the finding then names the file (or the shell) the value actually came from, + * because "OS_TENANCY_POSTURE is wrong" is only half an answer when four files + * could have set it. */ -export function resolveTenancyPostureOrFinding(): TenancyPostureReading { +export function resolveTenancyPostureOrFinding(reading: DotenvReading): TenancyPostureReading { try { - return { ok: true, posture: resolveTenancyPosture() }; + return { ok: true, posture: withDotenvOverlay(reading, () => resolveTenancyPosture()) }; } catch (err) { - const raw = (globalThis as { process?: { env?: Record } }) - .process?.env?.OS_TENANCY_POSTURE; + // Read back through the reading, not `process.env`: the overlay is already + // off by the time this runs (the `finally` above), so `process.env` would + // report the shell's value — `undefined` — for a posture that came from a + // `.env` file, quoting the operator a value they never typed. + const raw = effectiveEnvValue(reading, 'OS_TENANCY_POSTURE'); + const provenance = provenanceOf(reading, 'OS_TENANCY_POSTURE'); const cause = err instanceof Error ? err.message : String(err); const fixes = TENANCY_POSTURES.map((posture) => { const hint = TENANCY_POSTURE_FIX_HINTS[posture]; @@ -110,13 +428,13 @@ export function resolveTenancyPostureOrFinding(): TenancyPostureReading { + `${fixes}\n` + ' • or unset OS_TENANCY_POSTURE entirely — the posture then derives from\n' + ' OS_MULTI_ORG_ENABLED (true ⇒ isolated, anything else ⇒ single)\n' - // Said out loud because it is a real limit of THIS report, and the - // opposite of serve's gate, which runs after `dotenv-flow` has loaded. - // `os doctor` loads no `.env*`, so a posture that lives in a committed - // `.env` reaches the server and never reaches this check — a green - // doctor is not proof that serve will accept the posture. - + ' Read from this process\'s environment only: unlike `os serve`, `os doctor` does not\n' - + ' load `.env*` files, so a value set in one is not visible here.\n' + // Attribution, not a disclaimer. This sentence used to read "unlike + // `os serve`, `os doctor` does not load `.env*` files, so a value set + // in one is not visible here" — honest at the time (#5382) and the + // reason #5387 was filed. Doctor now reads the same cascade, so the + // sentence states what it READ: the file (or the shell) this value + // came from, and the files it consulted to decide that. + + ` ${envSourceSentence(reading, provenance)}\n` // The resolver owns the vocabulary and its wording; quoting rather // than paraphrasing keeps doctor from maintaining a second copy that // can disagree with it. @@ -598,7 +916,13 @@ export default class Doctor extends Command { // Note this REPORTS rather than refuses — no `process.exit(1)` here. The // finding is an ordinary `error` health check, so the rest of the report // still runs and doctor's own summary owns the non-zero exit. - const postureReading = resolveTenancyPostureOrFinding(); + // + // #5387 — the posture is resolved against the `.env*` cascade `os serve` + // reads, not this shell alone. Read here, before any check, for the same + // reason serve loads dotenv before its gate: a posture committed to `.env` + // is the common case, not the exotic one. + const dotenvReading = readDotenvFiles(process.cwd(), doctorNodeEnv()); + const postureReading = resolveTenancyPostureOrFinding(dotenvReading); // Check Node.js version try { @@ -716,6 +1040,12 @@ export default class Doctor extends Command { }); } + // #5387 — what doctor read the environment FROM, reported before anything + // derived from it. Unconditional: an env-derived verdict whose inputs are + // not attributed is a verdict the operator cannot check, and after this + // change every such verdict has four possible sources. + results.push(environmentSourcesCheck(dotenvReading)); + // #5382 — the posture verdict resolved at the top of `run()`, reported here // among the other environment facts. Only an unrecognized value produces a // row: a valid posture is not a finding, and doctor's output for every