Skip to content

Commit ee85dad

Browse files
committed
fix(cli): os doctor reports WHY the config could not be loaded (#5403)
The config-analysis `catch` took no binding — `catch {` — so the error object was discarded at the point it was caught. A genuinely broken config produced `Could not load config for analysis (config checks skipped)` and nothing else, under any flag: the sentence came from a bare `printWarning`, not from a `HealthCheckResult`, so `--verbose` had no `fix` to expand. `os serve`, in the same directory, prints the error in full — the diagnostic command returned strictly less than the command it exists to diagnose. #5382 / #5387 / #5397 fixed this sentence's ATTRIBUTION. This fixes what it SAYS now that it is finally saying it about the right thing. - `configLoadFailureCheck(err)` builds an ordinary `HealthCheckResult`, quoting the thrower verbatim (#5390's `cause:` posture) rather than paraphrasing: the failure can come from user code, esbuild, Node's resolver, or `loadConfig()` itself. - `renderHealthCheckResult()` extracts the environment block's rendering so the finding travels the same path and the same `--verbose` rule as `Environment files` / `Tenancy posture`, deleting the bare-printWarning bypass that had no `fix` channel at all. - The row carries a whitespace-folded, bounded quote (esbuild's first line is its least informative one); `--verbose` carries the untruncated original. - Gauge unchanged: warning, rest of the report runs, exit 0. The recognizable sentence survives verbatim as the head of the row — sibling tests assert its absence to mean "the config loaded". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh
1 parent 4b61cf3 commit ee85dad

3 files changed

Lines changed: 519 additions & 15 deletions

File tree

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
fix(cli): `os doctor` 说出配置载入失败的**原因**,不再只说一句「载入不了」(#5403)
6+
7+
config 分析那个很宽的 `catch` **不带绑定**(`catch {`),error 对象在被捕获的那一刻当场丢弃。
8+
于是配置真坏掉时,报告里没有任何线索:
9+
10+
```
11+
$ cat objectstack.config.ts
12+
throw new Error('this config is genuinely broken');
13+
14+
$ os doctor
15+
→ Loading configuration for analysis...
16+
⚠ Could not load config for analysis (config checks skipped)
17+
18+
⚠️ Environment is functional but has some warnings.
19+
```
20+
21+
`this config is genuinely broken` 一个字都不出现,`--verbose` 也没有 —— 这句话是裸
22+
`printWarning` 直接打的,不是 `HealthCheckResult`,所以根本没有 `fix` 可供展开,没有任何
23+
旗标能让操作者看到更多。而 `os serve` 在同一个目录会把这个错误**完整**打出来。诊断命令在
24+
它最该出力的一刻(配置坏了),给出的信息**严格少于**直接跑 `os serve`
25+
26+
这与前三单是同一句话的不同侧面:#5382 / #5387 / #5397 修的是这句话的**归因**(先把
27+
posture 的抛错挪出这个 catch,再让 env 派生检查读到 serve 的环境,最后让配置载入也在
28+
serve 的环境下进行)。三单之后这句话触发时配置**确实**坏了。本单修的是它归因正确之后
29+
**说了什么**
30+
31+
**现在的行为。** 这条路径不再是裸 `printWarning`,而是一条常规 `HealthCheckResult`,与
32+
`Environment files` / `Tenancy posture`**同一个渲染器**、同一条 `--verbose` 展开规则:
33+
34+
```
35+
$ os doctor
36+
→ Loading configuration for analysis...
37+
⚠ Config load Could not load config for analysis (config checks skipped) — this config is genuinely broken
38+
39+
$ os doctor --verbose
40+
⚠ Config load Could not load config for analysis (config checks skipped) — this config is genuinely broken
41+
→ `os serve` loads this same file the same way — bundle-require, under the `.env*`
42+
cascade named above (#5397) — and prints this error in full, so a config that
43+
lands here is one the server cannot boot either.
44+
The config-aware checks were SKIPPED, not passed: spec version, circular
45+
dependencies, unused objects, orphan views, dashboard integrity.
46+
cause: this config is genuinely broken
47+
```
48+
49+
四条刻意的取舍:
50+
51+
- **原话照引,不改写。**#5390`resolveTenancyPosture()` 原话同一体例:抛错方拥有措辞。
52+
配置载入的失败可能来自四个不同的权威(用户自己的 `throw`、esbuild 的打包诊断、Node 的
53+
模块解析、`loadConfig()` 自己的 "no default export"),doctor 没有立场把它们总结得比它们
54+
自己更好。
55+
- **cause 进 `message`,而不只进 `fix`** `Environment files` 把 cause 只放在 `fix` 里是对的
56+
—— 那一行本身已经把结论说完整了,cause 是脚注。这里 cause **就是**结论:没有它,这一行
57+
只说了「出了点没被指名的问题」。所以平铺一行放收敛后的引文(多行折叠成一行、超长截断),
58+
`--verbose` 给未截断的原文。折叠而不是取首行,是因为 esbuild 的首行恰好是它最没信息量的
59+
一句(`Build failed with 1 error:`,文件与原因在下一行)。
60+
- **档位不变。** 仍是 warning,doctor 仍然跑完其余检查,仍然 exit 0。本单让这句话说得更多,
61+
不是说得更响。
62+
- **那句话本身原样保留**,作为该行的开头。它被两份 changeset 引用、被本文件四处注释引用,
63+
也是操作者会去 grep 的字符串;更要紧的是,兄弟测试用它的**缺席**来表示「配置载入成功」,
64+
改写它会让那些断言在「没有任何东西能匹配」的空理由下继续变绿。
65+
66+
顺带消掉的是一条旁路:渲染规则此前只存在于环境检查那个 `forEach` 的循环体里,任何在它之后
67+
产生的结论都只能自己再手打一遍格式 —— 而手打的那份没有 `fix` 通道,`--verbose` 对它无效。
68+
渲染规则现在是一个具名函数,两处共用。
Lines changed: 307 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,307 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `os doctor` says WHY the config could not be loaded (#5403).
5+
*
6+
* ── The defect ───────────────────────────────────────────────────────────
7+
*
8+
* The config-analysis `catch` took no binding — `catch {` — so the error object
9+
* was discarded at the point it was caught:
10+
*
11+
* $ cat objectstack.config.ts
12+
* throw new Error('this config is genuinely broken');
13+
*
14+
* $ os doctor
15+
* → Loading configuration for analysis...
16+
* ⚠ Could not load config for analysis (config checks skipped)
17+
*
18+
* ⚠️ Environment is functional but has some warnings.
19+
*
20+
* `this config is genuinely broken` appeared nowhere, and no flag could reveal
21+
* it: the sentence came from a bare `printWarning`, not from a
22+
* `HealthCheckResult`, so `--verbose` had no `fix` to expand. Meanwhile
23+
* `os serve`, in that same directory, prints the error in full. The diagnostic
24+
* command returned STRICTLY LESS than the command it exists to diagnose, at the
25+
* one moment it is most needed.
26+
*
27+
* ── Why this is a separate issue from the three before it ────────────────
28+
*
29+
* #5382 → #5387 → #5397 fixed this sentence's ATTRIBUTION: first by lifting the
30+
* tenancy-posture throw out of this `catch`, then by letting doctor's
31+
* env-derived checks read serve's `.env*` cascade, then by loading the config
32+
* itself under that cascade. After all three, a run that reaches this `catch`
33+
* has a genuinely broken config — `os serve` cannot load it either. #5403 is
34+
* about what the sentence SAYS once it is finally saying it about the right
35+
* thing.
36+
*
37+
* ── What is pinned here ──────────────────────────────────────────────────
38+
*
39+
* • the cause reaches the terminal, quoted from the thrower, not paraphrased;
40+
* • the recognizable sentence SURVIVES verbatim as the head of the row —
41+
* sibling tests assert its absence to mean "the config loaded", and two
42+
* changesets quote it;
43+
* • the verdict stays a `warning`, exit 0, rest of the report still runs
44+
* (#5397's "not a silencer" constraint, from the other direction);
45+
* • the row goes through the ONE `HealthCheckResult` renderer, so `--verbose`
46+
* expands it exactly like `Environment files` / `Tenancy posture`;
47+
* • #5397's `.env*` overlay is not regressed: a config that throws only
48+
* without its `.env` value must still never reach this path.
49+
*/
50+
51+
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
52+
import fs from 'node:fs';
53+
import os from 'node:os';
54+
import path from 'node:path';
55+
import { fileURLToPath } from 'node:url';
56+
57+
import Doctor, { configLoadFailureCheck } from './doctor.js';
58+
59+
const HERE = path.dirname(fileURLToPath(import.meta.url));
60+
/** `packages/cli` — the oclif root the real command is loaded against below. */
61+
const CLI_ROOT = path.resolve(HERE, '..', '..');
62+
63+
/**
64+
* The escape is written as `\x1b`, never as the byte itself: one raw control
65+
* character makes `grep` treat the whole file as binary, and a test file no
66+
* `git grep` can find stops being maintained (#4890 / #5157).
67+
*/
68+
const SGR = /\x1b\[[0-9;]*m/g;
69+
const plain = (s: string) => s.replace(SGR, '');
70+
71+
/** The sentence three issues spent their effort making true. It must survive. */
72+
const HEADLINE = 'Could not load config for analysis (config checks skipped)';
73+
74+
/** The issue's own repro anchor. */
75+
const BROKEN = 'this config is genuinely broken';
76+
77+
describe('configLoadFailureCheck — the finding the discarded error used to be', () => {
78+
it('quotes the thrown message, in the row AND in the verbose detail', () => {
79+
const check = configLoadFailureCheck(new Error(BROKEN));
80+
81+
// Before #5403 this string existed only inside a `catch` that named
82+
// nothing. Both channels carry it now: the row so a plain `os doctor` is
83+
// actionable, the `fix` so `--verbose` has the untruncated original.
84+
expect(check.message).toContain(BROKEN);
85+
expect(check.fix).toContain(BROKEN);
86+
});
87+
88+
it('keeps the recognizable sentence intact at the head of the row', () => {
89+
const check = configLoadFailureCheck(new Error(BROKEN));
90+
91+
// Load-bearing, not cosmetic: `doctor-config-env-overlay.test.ts` and
92+
// `doctor-tenancy-posture-report.test.ts` assert this string's ABSENCE to
93+
// mean "the config loaded fine". Rewording it would leave those assertions
94+
// passing for the empty reason that nothing matches them any more.
95+
expect(check.message.startsWith(`${HEADLINE} — `)).toBe(true);
96+
});
97+
98+
it('stays a warning, and stays a named row like every other check', () => {
99+
const check = configLoadFailureCheck(new Error(BROKEN));
100+
101+
// #5397's constraint, restated from the other side: the point of #5403 is
102+
// that the warning says more, never that it says it louder. A broken config
103+
// does not stop doctor from finishing, and does not fail the run.
104+
expect(check.status).toBe('warning');
105+
// A `name` is what makes it renderable by the shared renderer at all — the
106+
// bare `printWarning` it replaces had no such column.
107+
expect(check.name).toBe('Config load');
108+
});
109+
110+
it('points at `os serve` and says the skipped checks were skipped, not passed', () => {
111+
const fix = configLoadFailureCheck(new Error(BROKEN)).fix ?? '';
112+
113+
expect(fix).toContain('os serve');
114+
// The second half of the harm the issue describes: a reader who sees the
115+
// config-aware sections simply missing from the report can otherwise read
116+
// their silence as a clean bill of health.
117+
expect(fix).toContain('SKIPPED, not passed');
118+
});
119+
120+
it('folds a multi-line cause onto the row WITHOUT losing the informative line', () => {
121+
// esbuild's shape, and the reason the row is not simply `cause.split("\n")[0]`:
122+
// the first line is the least informative thing the failure has to say.
123+
const esbuild = new Error(
124+
'Build failed with 1 error:\nobjectstack.config.ts:3:0: ERROR: Expected ";" but found "}"',
125+
);
126+
const check = configLoadFailureCheck(esbuild);
127+
128+
expect(check.message).toContain('Build failed with 1 error:');
129+
expect(check.message).toContain('objectstack.config.ts:3:0');
130+
expect(check.message).toContain('Expected ";" but found "}"');
131+
// One row is one line.
132+
expect(check.message).not.toContain('\n');
133+
// The verbose channel keeps the original's own line breaks.
134+
expect(check.fix).toContain('objectstack.config.ts:3:0: ERROR: Expected ";" but found "}"');
135+
});
136+
137+
it('clamps an overlong cause on the row and keeps it whole in the detail', () => {
138+
const long = `HEAD ${'x'.repeat(4000)} TAIL`;
139+
const check = configLoadFailureCheck(new Error(long));
140+
141+
expect(check.message).toContain('HEAD ');
142+
expect(check.message).toContain('…');
143+
expect(check.message.length).toBeLessThan(260);
144+
// Truncation is a property of the ROW, never of the record: `--verbose`
145+
// hands over every character the thrower wrote.
146+
expect(check.fix).toContain('TAIL');
147+
expect(check.fix).toContain('x'.repeat(4000));
148+
});
149+
150+
it('never trails off into nothing when the Error carries no message', () => {
151+
// `throw new Error()` and `throw new TypeError()` are rare but real, and a
152+
// headline ending in a bare dash is the same "no information" defect this
153+
// issue is about, reintroduced at the edge.
154+
const check = configLoadFailureCheck(new TypeError());
155+
156+
expect(check.message.endsWith('— ')).toBe(false);
157+
expect(check.message).toContain('TypeError');
158+
});
159+
160+
it('reports a thrown non-Error rather than swallowing it', () => {
161+
// A config is arbitrary user code; `throw 'boom'` is legal.
162+
expect(configLoadFailureCheck('boom').message).toContain('boom');
163+
expect(configLoadFailureCheck(42).message).toContain('42');
164+
});
165+
});
166+
167+
describe('os doctor, end to end, against a config that cannot be loaded', () => {
168+
/**
169+
* `node_modules/` exists in the temp cwd on purpose — without it doctor's
170+
* `Dependencies` check is itself an `error` and exits 1 on its own, which
171+
* would make an assertion pass for a reason having nothing to do with this
172+
* change (the trap PR #5390 wrote down and #5398 / #5402 inherited).
173+
*/
174+
let tmp: string;
175+
let cwdSpy: ReturnType<typeof vi.spyOn>;
176+
177+
beforeEach(() => {
178+
tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'os-doctor-5403-e2e-'));
179+
fs.mkdirSync(path.join(tmp, 'node_modules'));
180+
cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmp);
181+
});
182+
183+
afterEach(() => {
184+
cwdSpy.mockRestore();
185+
fs.rmSync(tmp, { recursive: true, force: true });
186+
});
187+
188+
const writeFile = (name: string, body: string) => fs.writeFileSync(path.join(tmp, name), body);
189+
190+
async function runDoctor(argv: string[] = []): Promise<{ out: string; exitCode: number | undefined }> {
191+
const logs: string[] = [];
192+
const logSpy = vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => {
193+
logs.push(a.join(' '));
194+
});
195+
let exitCode: number | undefined;
196+
const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => {
197+
exitCode = code;
198+
throw new Error(`__PROCESS_EXIT__:${code}`);
199+
}) as never);
200+
201+
try {
202+
await Doctor.run(argv, { root: CLI_ROOT });
203+
} catch (err) {
204+
if (!(err instanceof Error) || !err.message.startsWith('__PROCESS_EXIT__')) throw err;
205+
} finally {
206+
logSpy.mockRestore();
207+
exitSpy.mockRestore();
208+
}
209+
return { out: plain(logs.join('\n')), exitCode };
210+
}
211+
212+
it('prints the cause on a plain run — the issue’s own repro', async () => {
213+
writeFile('objectstack.config.ts', `throw new Error('${BROKEN}');\n`);
214+
215+
const run = await runDoctor();
216+
217+
// THE assertion of this issue. Before #5403 this string appeared nowhere in
218+
// any doctor output, under any flag, while `os serve` printed it in full.
219+
expect(run.out).toContain(BROKEN);
220+
// …without losing the sentence that says what was skipped.
221+
expect(run.out).toContain(HEADLINE);
222+
// Rendered through the shared renderer, so it carries a name column like
223+
// every other health check rather than being a naked one-liner.
224+
expect(run.out).toContain('Config load');
225+
// Gauge unchanged: warning, report finishes, exit 0.
226+
expect(run.out).toContain('Environment is functional but has some warnings');
227+
expect(run.exitCode).toBeUndefined();
228+
}, 60_000);
229+
230+
it('expands the full detail under --verbose, and only under --verbose', async () => {
231+
writeFile('objectstack.config.ts', `throw new Error('${BROKEN}');\n`);
232+
233+
const plainRun = await runDoctor();
234+
const verboseRun = await runDoctor(['--verbose']);
235+
236+
// The `fix` channel follows the ONE rule every other warning follows:
237+
// shown when asked for, or when the finding is an error. This row is a
238+
// warning, so a default run stops at the (bounded) headline.
239+
expect(plainRun.out).not.toContain('cause:');
240+
expect(verboseRun.out).toContain('cause:');
241+
expect(verboseRun.out).toContain(BROKEN);
242+
// The verbose block is what tells the reader the skipped checks are not
243+
// silent passes.
244+
expect(verboseRun.out).toContain('SKIPPED, not passed');
245+
expect(verboseRun.exitCode).toBeUndefined();
246+
}, 60_000);
247+
248+
it('surfaces a SYNTAX error’s file and reason, not just “build failed”', async () => {
249+
// The bundle never runs the file here, so the failure comes from esbuild
250+
// rather than from user code — a different authority, quoted the same way.
251+
writeFile('objectstack.config.ts', 'export default { manifest: { name: "broken",\n');
252+
253+
const run = await runDoctor();
254+
255+
expect(run.out).toContain(HEADLINE);
256+
expect(run.out).toContain('objectstack.config.ts');
257+
expect(run.exitCode).toBeUndefined();
258+
}, 60_000);
259+
260+
it('does NOT fire for a config that only needed its .env — #5397 is not regressed', async () => {
261+
// The load-bearing overlay: `loadConfig()` runs inside
262+
// `withDotenvOverlayAsync`, and the async variant is what survives the
263+
// dynamic `import()` the bundle performs. If this change broke that, the
264+
// config below would throw and this row would appear.
265+
writeFile('.env', 'OS_5403_DB_URL=postgres://from-dotenv/db\n');
266+
writeFile(
267+
'objectstack.config.ts',
268+
[
269+
'const url = process.env.OS_5403_DB_URL;',
270+
"if (!url) throw new Error('OS_5403_DB_URL is required');",
271+
'export default {',
272+
" manifest: { name: 'os5403', label: 'Config Load Cause', version: '1.0.0' },",
273+
" objects: [{ name: 'account', label: 'Account', fields: [{ name: 'name', type: 'text', label: 'Name' }] }],",
274+
'};',
275+
'',
276+
].join('\n'),
277+
);
278+
279+
const run = await runDoctor(['--verbose']);
280+
281+
expect(run.out).not.toContain(HEADLINE);
282+
expect(run.out).not.toContain('Config load ');
283+
expect(run.out).not.toContain('OS_5403_DB_URL is required');
284+
// The checks ran rather than merely not failing.
285+
expect(run.out).toContain('No circular references detected');
286+
// The overlay left no residue behind it.
287+
expect(Object.prototype.hasOwnProperty.call(process.env, 'OS_5403_DB_URL')).toBe(false);
288+
}, 60_000);
289+
290+
it('says nothing at all when the config loads — the row is a finding, not a status line', async () => {
291+
writeFile(
292+
'objectstack.config.ts',
293+
[
294+
'export default {',
295+
" manifest: { name: 'os5403ok', label: 'Healthy', version: '1.0.0' },",
296+
" objects: [{ name: 'account', label: 'Account', fields: [{ name: 'name', type: 'text', label: 'Name' }] }],",
297+
'};',
298+
'',
299+
].join('\n'),
300+
);
301+
302+
const run = await runDoctor(['--verbose']);
303+
304+
expect(run.out).not.toContain('Config load');
305+
expect(run.out).not.toContain(HEADLINE);
306+
}, 60_000);
307+
});

0 commit comments

Comments
 (0)