Skip to content

Commit 566985c

Browse files
os-zhuangclaude
andauthored
fix(cli): os start / os dev stop injecting OS_ARTIFACT_PATH into the child serve environment (#9365)
* fix(cli): move the CLI's resolved artifact off OS_ARTIFACT_PATH onto an internal channel (#8985) `os start` and `os dev` wrote their own resolved artifact path into the child `serve` environment as `OS_ARTIFACT_PATH` — the same variable an operator sets. The write happens before the downstream `objectstack.config.ts` is evaluated, so a config saw the variable set on every boot and could not tell an operator's instruction from the CLI's plumbing. The resolved path now travels on `OS_INTERNAL_ARTIFACT_PATH`, owned by `packages/cli/src/utils/internal-artifact-channel.ts`. `serve` reads it strictly between `OS_ARTIFACT_URL` and `OS_ARTIFACT_PATH`, which preserves every answer the resolution ladder gives today: it must beat the operator knob so `--artifact X` still wins over an exported `OS_ARTIFACT_PATH=Y`, and it must lose to the reference because `dev` sends its answer unconditionally. Unchanged and now pinned: the ladder itself, `start`'s refusal to set `OS_BOOT_EMPTY` on a reference boot, and the named-vs-conventional distinction that keeps a missing resolved artifact a loud refusal. Co-Authored-By: Claude <noreply@anthropic.com> * test(cli): read the OS_ARTIFACT_PATH write pin off the AST, not a text scan (#8985) Reverse verification caught the pin under-reporting: with the old injection restored in both commands, only `dev.ts` went red. `start.ts`'s `--auth-secret` flag description contains the literal `/api/v1/auth/*`, and that `/*` opened a phantom block comment which the regex comment-stripper closed against a docblock 250 lines later — swallowing the reinstated write along with it. The pin now parses the file and walks for real `OS_ARTIFACT_PATH` writes (object property, property assignment, indexed assignment), so strings and comments cannot lie to it, and carries a self-test over the exact specimen that defeated the text scan. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2416dd5 commit 566985c

6 files changed

Lines changed: 612 additions & 26 deletions

File tree

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
---
2+
"@objectstack/cli": minor
3+
---
4+
5+
fix(cli): `os start` / `os dev` stop writing `OS_ARTIFACT_PATH` into the child `serve` environment — the CLI's own plumbing moves to an internal channel (#8985)
6+
7+
`os start` and `os dev` are supervisors: each resolves an artifact, then spawns
8+
`os serve` to boot it. Both handed the resolved path down by writing
9+
**`OS_ARTIFACT_PATH`** into the child environment — the same variable an operator
10+
sets to name an artifact. `dev` wrote it unconditionally; `start` wrote it
11+
whenever it had resolved anything and no `OS_ARTIFACT_URL` was in play. Both
12+
writes happen **before** the downstream `objectstack.config.ts` is evaluated.
13+
14+
So inside any config, that variable was set on **every** boot, including boots
15+
where no operator had ever mentioned it — measured from the shipped EE image
16+
with its own `ENV` deliberately deleted: `os start` still printed
17+
`Artifact: dist/objectstack.json` and handed that path down. A config could not
18+
answer *"did a human ask for this, or did the CLI put it here?"*
19+
20+
The resolved path now travels on **`OS_INTERNAL_ARTIFACT_PATH`**, a channel the
21+
CLI owns both ends of (`packages/cli/src/utils/internal-artifact-channel.ts`),
22+
and the property downstream consumers need is restored:
23+
24+
> **the presence of `OS_ARTIFACT_PATH` in a config's environment means an
25+
> operator set it.**
26+
27+
**Nothing about resolution changed.** Each command's ladder resolves in the
28+
parent exactly as before, and `serve` reads the new channel strictly between the
29+
reference and the operator knob:
30+
31+
```
32+
--artifact > OS_ARTIFACT_URL > OS_INTERNAL_ARTIFACT_PATH > OS_ARTIFACT_PATH > <cwd>/dist/objectstack.json
33+
```
34+
35+
That position is what preserves today's answers in both directions. It beats
36+
`OS_ARTIFACT_PATH` because `os start --artifact X` run with an operator's
37+
`OS_ARTIFACT_PATH=Y` exported boots **X** today — the parent used to overwrite
38+
the variable on the way down, and now inherits it untouched. It loses to
39+
`OS_ARTIFACT_URL` because `os dev` writes the channel unconditionally, as it
40+
wrote the old variable unconditionally, and the reference has always outranked
41+
the path.
42+
43+
Two further behaviours are unchanged and now pinned rather than incidental:
44+
`start` still refuses to set `OS_BOOT_EMPTY` when a reference is driving the
45+
boot (an unreachable artifact host stays a loud refusal instead of a silently
46+
empty platform), and a resolved-but-missing artifact is still "named" to
47+
`resolveDefaultArtifactPath`, so it fails loudly rather than booting empty.
48+
49+
**If you depended on the old side effect** — a config reading
50+
`process.env.OS_ARTIFACT_PATH` and expecting the CLI to have populated it — set
51+
the variable yourself, or read the artifact from the config's own inputs.
52+
`OS_ARTIFACT_PATH` remains a fully supported operator knob on the exact rung it
53+
has always occupied; the CLI simply no longer manufactures it on your behalf.
54+
`OS_INTERNAL_ARTIFACT_PATH` is not a supported knob and is deliberately absent
55+
from the environment-variable reference.
Lines changed: 313 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,313 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Pin: **the presence of `OS_ARTIFACT_PATH` in a config's environment means an
5+
* operator set it.**
6+
*
7+
* `os start` and `os dev` spawn `os serve`, and the downstream
8+
* `objectstack.config.ts` is evaluated inside that child. While the supervisors
9+
* wrote their own resolved artifact path into the child's `OS_ARTIFACT_PATH`,
10+
* the variable was set on **every** boot — so a config could not tell an
11+
* operator's instruction from the CLI's own plumbing, and a consumer wanting to
12+
* refuse the retired knob could only do so by inspecting its *value*.
13+
*
14+
* The plumbing now travels on `OS_INTERNAL_ARTIFACT_PATH`
15+
* (`utils/internal-artifact-channel.ts`). This file pins both halves of the
16+
* property, plus the two behaviours that had to survive the move: the
17+
* resolution ladder, and `start`'s deliberate refusal to declare an empty boot
18+
* acceptable when a reference is driving the boot.
19+
*
20+
* Two kinds of assertion here, and both are needed:
21+
*
22+
* - **Behavioural** — over `childEnvWithResolvedArtifact`, which is the whole
23+
* of what each command contributes to its child's artifact environment.
24+
* - **Structural** — a source assertion that neither command writes
25+
* `OS_ARTIFACT_PATH` into an env object at all. The behavioural pins describe
26+
* the helper; only this one refuses a future edit that re-adds the write
27+
* beside it. Both files compose their child env as
28+
* `{ ...childEnvWithResolvedArtifact(process.env, …), …other keys }`, so
29+
* "the helper is correct" plus "nothing else writes the key" is what makes
30+
* the composed env correct.
31+
*/
32+
33+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
34+
import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readFileSync } from 'node:fs';
35+
import { tmpdir } from 'node:os';
36+
import path from 'path';
37+
import ts from 'typescript';
38+
import {
39+
INTERNAL_ARTIFACT_PATH_ENV,
40+
childEnvWithResolvedArtifact,
41+
readInternalArtifactPath,
42+
} from '../utils/internal-artifact-channel.js';
43+
import { resolveArtifactSource } from './start.js';
44+
45+
const ARTIFACT = '/srv/app/objectstack.json';
46+
47+
describe('the child `serve` env — OS_ARTIFACT_PATH means an operator set it', () => {
48+
it('carries NO OS_ARTIFACT_PATH when the operator did not set one', () => {
49+
const parentEnv = { PATH: '/usr/bin', NODE_ENV: 'production' };
50+
51+
for (const decision of [
52+
{ kind: 'resolved', path: ARTIFACT },
53+
{ kind: 'reference' },
54+
{ kind: 'empty' },
55+
] as const) {
56+
const childEnv = childEnvWithResolvedArtifact(parentEnv, decision);
57+
expect(
58+
Object.prototype.hasOwnProperty.call(childEnv, 'OS_ARTIFACT_PATH'),
59+
`decision ${decision.kind} must not introduce OS_ARTIFACT_PATH`,
60+
).toBe(false);
61+
expect(childEnv.OS_ARTIFACT_PATH).toBeUndefined();
62+
}
63+
});
64+
65+
it('still carries OS_ARTIFACT_PATH — verbatim — when the operator DID set one', () => {
66+
const parentEnv = { OS_ARTIFACT_PATH: './dist/from-operator.json' };
67+
68+
for (const decision of [
69+
{ kind: 'resolved', path: '/abs/dist/from-operator.json' },
70+
{ kind: 'reference' },
71+
{ kind: 'empty' },
72+
] as const) {
73+
const childEnv = childEnvWithResolvedArtifact(parentEnv, decision);
74+
// Inherited untouched: the child sees exactly what the operator wrote,
75+
// not an absolutised rewrite of it.
76+
expect(childEnv.OS_ARTIFACT_PATH).toBe('./dist/from-operator.json');
77+
}
78+
});
79+
80+
it('hands the resolved artifact down on the internal channel instead', () => {
81+
const childEnv = childEnvWithResolvedArtifact({}, { kind: 'resolved', path: ARTIFACT });
82+
expect(childEnv[INTERNAL_ARTIFACT_PATH_ENV]).toBe(ARTIFACT);
83+
expect(readInternalArtifactPath(childEnv)).toBe(ARTIFACT);
84+
});
85+
86+
it('lets the parent OWN the internal channel — an inherited value never speaks for it', () => {
87+
const parentEnv = { [INTERNAL_ARTIFACT_PATH_ENV]: '/stale/inherited.json' };
88+
89+
expect(childEnvWithResolvedArtifact(parentEnv, { kind: 'resolved', path: ARTIFACT }))
90+
.toMatchObject({ [INTERNAL_ARTIFACT_PATH_ENV]: ARTIFACT });
91+
92+
for (const decision of [{ kind: 'reference' }, { kind: 'empty' }] as const) {
93+
const childEnv = childEnvWithResolvedArtifact(parentEnv, decision);
94+
expect(
95+
readInternalArtifactPath(childEnv),
96+
`decision ${decision.kind} resolved nothing, so the channel must be empty`,
97+
).toBeUndefined();
98+
expect(Object.prototype.hasOwnProperty.call(childEnv, INTERNAL_ARTIFACT_PATH_ENV)).toBe(false);
99+
}
100+
});
101+
102+
it('reads a blank channel value as no decision at all', () => {
103+
expect(readInternalArtifactPath({})).toBeUndefined();
104+
expect(readInternalArtifactPath({ [INTERNAL_ARTIFACT_PATH_ENV]: '' })).toBeUndefined();
105+
expect(readInternalArtifactPath({ [INTERNAL_ARTIFACT_PATH_ENV]: ' ' })).toBeUndefined();
106+
});
107+
});
108+
109+
describe('OS_BOOT_EMPTY — the artifact-reference refusal survives the move', () => {
110+
it('is NOT set when a reference (OS_ARTIFACT_URL) is driving the boot', () => {
111+
// Load-bearing: setting it here would tell `serve` that booting an app-less
112+
// kernel is an acceptable outcome, turning an unreachable artifact host
113+
// into a silently empty platform instead of a loud refusal.
114+
const childEnv = childEnvWithResolvedArtifact({}, { kind: 'reference' });
115+
expect(childEnv.OS_BOOT_EMPTY).toBeUndefined();
116+
expect(readInternalArtifactPath(childEnv)).toBeUndefined();
117+
});
118+
119+
it('is NOT set when an artifact was resolved', () => {
120+
expect(childEnvWithResolvedArtifact({}, { kind: 'resolved', path: ARTIFACT }).OS_BOOT_EMPTY)
121+
.toBeUndefined();
122+
});
123+
124+
it('is set only when nothing resolved and an empty boot IS the intent', () => {
125+
expect(childEnvWithResolvedArtifact({}, { kind: 'empty' }).OS_BOOT_EMPTY).toBe('1');
126+
});
127+
128+
it('never CLEARS an operator-exported OS_BOOT_EMPTY (add-only, as before)', () => {
129+
const parentEnv = { OS_BOOT_EMPTY: '1' };
130+
for (const decision of [
131+
{ kind: 'resolved', path: ARTIFACT },
132+
{ kind: 'reference' },
133+
{ kind: 'empty' },
134+
] as const) {
135+
expect(
136+
childEnvWithResolvedArtifact(parentEnv, decision).OS_BOOT_EMPTY,
137+
`decision ${decision.kind} must not start clearing an inherited OS_BOOT_EMPTY`,
138+
).toBe('1');
139+
}
140+
});
141+
});
142+
143+
describe('resolveArtifactSource — the resolution ladder is unchanged', () => {
144+
let cwd: string;
145+
let home: string;
146+
147+
const write = (dir: string, rel: string) => {
148+
const abs = path.join(dir, rel);
149+
mkdirSync(path.dirname(abs), { recursive: true });
150+
writeFileSync(abs, '{}');
151+
return abs;
152+
};
153+
154+
beforeEach(() => {
155+
cwd = mkdtempSync(path.join(tmpdir(), 'os-artifact-cwd-'));
156+
home = mkdtempSync(path.join(tmpdir(), 'os-artifact-home-'));
157+
});
158+
afterEach(() => {
159+
for (const d of [cwd, home]) {
160+
try { rmSync(d, { recursive: true, force: true }); } catch { /* noop */ }
161+
}
162+
});
163+
164+
it('rung 1: --artifact wins over everything, including an operator OS_ARTIFACT_PATH', () => {
165+
const flagFile = write(cwd, 'build/pinned.json');
166+
write(cwd, 'dist/objectstack.json');
167+
write(home, 'dist/objectstack.json');
168+
169+
const r = resolveArtifactSource('build/pinned.json', home, {
170+
cwd,
171+
env: { OS_ARTIFACT_PATH: '/from/env.json' },
172+
});
173+
expect(r?.path).toBe(flagFile);
174+
});
175+
176+
it('rung 1: --artifact passes an http(s) URL through untouched', () => {
177+
const url = 'https://cdn.example.com/app.json';
178+
expect(resolveArtifactSource(url, home, { cwd, env: {} })?.path).toBe(url);
179+
});
180+
181+
it('rung 2: $OS_ARTIFACT_PATH wins over both auto-detected locations', () => {
182+
write(cwd, 'dist/objectstack.json');
183+
write(home, 'dist/objectstack.json');
184+
185+
const r = resolveArtifactSource(undefined, home, {
186+
cwd,
187+
env: { OS_ARTIFACT_PATH: 'custom/app.json' },
188+
});
189+
// Anchored on the cwd, exactly as before — the ladder resolves it; the
190+
// variable itself is inherited by the child untouched.
191+
expect(r?.path).toBe(path.join(cwd, 'custom/app.json'));
192+
});
193+
194+
it('rung 2: $OS_ARTIFACT_PATH may itself be an http(s) URL', () => {
195+
const url = 'https://cdn.example.com/env.json';
196+
expect(resolveArtifactSource(undefined, home, { cwd, env: { OS_ARTIFACT_PATH: url } })?.path)
197+
.toBe(url);
198+
});
199+
200+
it('rung 3: <cwd>/dist/objectstack.json wins over <home>/dist', () => {
201+
const cwdArtifact = write(cwd, 'dist/objectstack.json');
202+
write(home, 'dist/objectstack.json');
203+
expect(resolveArtifactSource(undefined, home, { cwd, env: {} })?.path).toBe(cwdArtifact);
204+
});
205+
206+
it('rung 4: <home>/dist/objectstack.json is the last resort', () => {
207+
const homeArtifact = write(home, 'dist/objectstack.json');
208+
expect(resolveArtifactSource(undefined, home, { cwd, env: {} })?.path).toBe(homeArtifact);
209+
});
210+
211+
it('rung 5: nothing reachable resolves to undefined', () => {
212+
expect(resolveArtifactSource(undefined, home, { cwd, env: {} })).toBeUndefined();
213+
});
214+
});
215+
216+
describe('structural: the supervisors never write the operator knob', () => {
217+
/**
218+
* Read WRITES of `OS_ARTIFACT_PATH` off the TypeScript AST.
219+
*
220+
* Deliberately not a text scan. The first version of this pin stripped
221+
* comments with a regex and reported `start.ts` clean while the file really
222+
* did carry the write: the `--auth-secret` flag description contains the
223+
* literal `/api/v1/auth/*`, whose `/*` opened a phantom block comment that
224+
* swallowed 250 lines of real code, the injection among them. A detector that
225+
* under-reports silently is worse than none — so the parser decides what is
226+
* code and what is prose, and strings and comments cannot lie to it.
227+
*
228+
* Only writes are collected. Reading `process.env.OS_ARTIFACT_PATH` — the
229+
* operator's own value, which both commands' ladders still honour — is
230+
* correct and must stay possible.
231+
*/
232+
const artifactPathWrites = (file: string): string[] => {
233+
const src = readFileSync(new URL(`./${file}`, import.meta.url), 'utf8');
234+
const sourceFile = ts.createSourceFile(file, src, ts.ScriptTarget.Latest, true);
235+
const hits: string[] = [];
236+
237+
const at = (node: ts.Node) =>
238+
`${file}:${sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)).line + 1}`;
239+
const staticName = (node: ts.Node): string | undefined =>
240+
ts.isIdentifier(node) || ts.isStringLiteral(node) ? node.text : undefined;
241+
242+
const visit = (node: ts.Node): void => {
243+
// `{ OS_ARTIFACT_PATH: value }` and `{ OS_ARTIFACT_PATH }`
244+
if (
245+
(ts.isPropertyAssignment(node) || ts.isShorthandPropertyAssignment(node))
246+
&& staticName(node.name) === 'OS_ARTIFACT_PATH'
247+
) {
248+
hits.push(`${at(node)} object property`);
249+
}
250+
// `env.OS_ARTIFACT_PATH = value` / `env['OS_ARTIFACT_PATH'] = value`
251+
if (ts.isBinaryExpression(node) && node.operatorToken.kind === ts.SyntaxKind.EqualsToken) {
252+
const lhs = node.left;
253+
if (ts.isPropertyAccessExpression(lhs) && lhs.name.text === 'OS_ARTIFACT_PATH') {
254+
hits.push(`${at(node)} property assignment`);
255+
}
256+
if (
257+
ts.isElementAccessExpression(lhs)
258+
&& staticName(lhs.argumentExpression) === 'OS_ARTIFACT_PATH'
259+
) {
260+
hits.push(`${at(node)} indexed assignment`);
261+
}
262+
}
263+
ts.forEachChild(node, visit);
264+
};
265+
266+
visit(sourceFile);
267+
return hits;
268+
};
269+
270+
for (const file of ['start.ts', 'dev.ts']) {
271+
it(`${file} writes OS_ARTIFACT_PATH nowhere`, () => {
272+
expect(
273+
artifactPathWrites(file),
274+
`${file} must not write OS_ARTIFACT_PATH into a child environment — the CLI's own `
275+
+ `resolved artifact travels on ${INTERNAL_ARTIFACT_PATH_ENV}, so that a downstream `
276+
+ `objectstack.config.ts seeing OS_ARTIFACT_PATH knows an operator set it. `
277+
+ `Reading process.env.OS_ARTIFACT_PATH (the operator's value) stays correct.`,
278+
).toEqual([]);
279+
});
280+
}
281+
282+
it('the detector itself sees a write that a comment-stripping text scan missed', () => {
283+
// The specimen is `start.ts`'s own shape, with the `/*`-bearing string that
284+
// defeated the text scan sitting above it. Without this, the pin above
285+
// could go permanently green by failing to look.
286+
// The trailing docblock matters: the `/*` inside the string only swallows
287+
// code up to the next `*/`, and in the real file that closer is an ordinary
288+
// docblock a few hundred lines further down.
289+
const specimen = [
290+
"const flag = { description: 'mount /api/v1/auth/* (overrides $AUTH_SECRET)' };",
291+
'const childEnv = {',
292+
' ...process.env,',
293+
' OS_ARTIFACT_PATH: resolved.path,',
294+
'};',
295+
'/** An ordinary docblock, whose closer ends the phantom comment. */',
296+
'export const done = true;',
297+
].join('\n');
298+
299+
const sourceFile = ts.createSourceFile('specimen.ts', specimen, ts.ScriptTarget.Latest, true);
300+
let found = 0;
301+
const visit = (node: ts.Node): void => {
302+
if (ts.isPropertyAssignment(node) && ts.isIdentifier(node.name)
303+
&& node.name.text === 'OS_ARTIFACT_PATH') found += 1;
304+
ts.forEachChild(node, visit);
305+
};
306+
visit(sourceFile);
307+
expect(found).toBe(1);
308+
309+
// ...and the text scan this replaced reports the same specimen clean.
310+
const textScanned = specimen.replace(/\/\*[\s\S]*?\*\//g, '');
311+
expect(/OS_ARTIFACT_PATH\s*:/.test(textScanned)).toBe(false);
312+
});
313+
});

‎packages/cli/src/commands/dev.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
assessArtifactStaleness,
1717
formatMtimeGap,
1818
} from '../utils/dev-restart.js';
19+
import { childEnvWithResolvedArtifact } from '../utils/internal-artifact-channel.js';
1920
import { readEnvWithDeprecation, isMcpServerEnabled } from '@objectstack/types';
2021
import type { ResolvedProjectDatabaseUrl } from '@objectstack/runtime';
2122

@@ -327,10 +328,18 @@ export default class Dev extends Command {
327328
fs.mkdirSync(path.dirname(resolvedDb.url.replace(/^file:/, '')), { recursive: true });
328329
}
329330
const effectiveDb = resolvedDb.url;
331+
// `dev` always has a resolved artifact by this point (it compiled one, or
332+
// was handed one with `--artifact`, or is pointing at the canonical
333+
// `<cwd>/dist/objectstack.json`), so the decision is unconditionally
334+
// `resolved` — exactly as unconditional as the `OS_ARTIFACT_PATH` write
335+
// it replaces. What changed is the channel: the resolved path travels on
336+
// the CLI's own `OS_INTERNAL_ARTIFACT_PATH`, so an `OS_ARTIFACT_PATH`
337+
// seen by a downstream `objectstack.config.ts` means an operator set it.
338+
// The operator's own value is inherited verbatim, and `dev`'s ladder
339+
// above still honours it on the rung it has always occupied.
330340
const localEnv: NodeJS.ProcessEnv = {
331-
...process.env,
341+
...childEnvWithResolvedArtifact(process.env, { kind: 'resolved', path: artifactPath }),
332342
OS_ENVIRONMENT_ID: environmentId,
333-
OS_ARTIFACT_PATH: artifactPath,
334343
OS_SEED_ADMIN: seedAdmin ? '1' : '0',
335344
...(seedAdmin && flags['admin-email'] ? { OS_SEED_ADMIN_EMAIL: flags['admin-email'] } : {}),
336345
...(seedAdmin && flags['admin-password'] ? { OS_SEED_ADMIN_PASSWORD: flags['admin-password'] } : {}),

0 commit comments

Comments
 (0)