From f4ec939248411ca3bcb757254deb3019113c23f5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 16:46:05 +0000 Subject: [PATCH 1/2] fix(cli): move the CLI's resolved artifact off OS_ARTIFACT_PATH onto an internal channel (#8985) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- ...rtifact-path-child-env-internal-channel.md | 55 ++++ .../commands/artifact-child-env.pin.test.ts | 241 ++++++++++++++++++ packages/cli/src/commands/dev.ts | 13 +- packages/cli/src/commands/serve.ts | 37 ++- packages/cli/src/commands/start.ts | 86 +++++-- .../src/utils/internal-artifact-channel.ts | 134 ++++++++++ 6 files changed, 540 insertions(+), 26 deletions(-) create mode 100644 .changeset/artifact-path-child-env-internal-channel.md create mode 100644 packages/cli/src/commands/artifact-child-env.pin.test.ts create mode 100644 packages/cli/src/utils/internal-artifact-channel.ts diff --git a/.changeset/artifact-path-child-env-internal-channel.md b/.changeset/artifact-path-child-env-internal-channel.md new file mode 100644 index 0000000000..b7a401f921 --- /dev/null +++ b/.changeset/artifact-path-child-env-internal-channel.md @@ -0,0 +1,55 @@ +--- +"@objectstack/cli": minor +--- + +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) + +`os start` and `os dev` are supervisors: each resolves an artifact, then spawns +`os serve` to boot it. Both handed the resolved path down by writing +**`OS_ARTIFACT_PATH`** into the child environment — the same variable an operator +sets to name an artifact. `dev` wrote it unconditionally; `start` wrote it +whenever it had resolved anything and no `OS_ARTIFACT_URL` was in play. Both +writes happen **before** the downstream `objectstack.config.ts` is evaluated. + +So inside any config, that variable was set on **every** boot, including boots +where no operator had ever mentioned it — measured from the shipped EE image +with its own `ENV` deliberately deleted: `os start` still printed +`Artifact: dist/objectstack.json` and handed that path down. A config could not +answer *"did a human ask for this, or did the CLI put it here?"* + +The resolved path now travels on **`OS_INTERNAL_ARTIFACT_PATH`**, a channel the +CLI owns both ends of (`packages/cli/src/utils/internal-artifact-channel.ts`), +and the property downstream consumers need is restored: + +> **the presence of `OS_ARTIFACT_PATH` in a config's environment means an +> operator set it.** + +**Nothing about resolution changed.** Each command's ladder resolves in the +parent exactly as before, and `serve` reads the new channel strictly between the +reference and the operator knob: + +``` +--artifact > OS_ARTIFACT_URL > OS_INTERNAL_ARTIFACT_PATH > OS_ARTIFACT_PATH > /dist/objectstack.json +``` + +That position is what preserves today's answers in both directions. It beats +`OS_ARTIFACT_PATH` because `os start --artifact X` run with an operator's +`OS_ARTIFACT_PATH=Y` exported boots **X** today — the parent used to overwrite +the variable on the way down, and now inherits it untouched. It loses to +`OS_ARTIFACT_URL` because `os dev` writes the channel unconditionally, as it +wrote the old variable unconditionally, and the reference has always outranked +the path. + +Two further behaviours are unchanged and now pinned rather than incidental: +`start` still refuses to set `OS_BOOT_EMPTY` when a reference is driving the +boot (an unreachable artifact host stays a loud refusal instead of a silently +empty platform), and a resolved-but-missing artifact is still "named" to +`resolveDefaultArtifactPath`, so it fails loudly rather than booting empty. + +**If you depended on the old side effect** — a config reading +`process.env.OS_ARTIFACT_PATH` and expecting the CLI to have populated it — set +the variable yourself, or read the artifact from the config's own inputs. +`OS_ARTIFACT_PATH` remains a fully supported operator knob on the exact rung it +has always occupied; the CLI simply no longer manufactures it on your behalf. +`OS_INTERNAL_ARTIFACT_PATH` is not a supported knob and is deliberately absent +from the environment-variable reference. diff --git a/packages/cli/src/commands/artifact-child-env.pin.test.ts b/packages/cli/src/commands/artifact-child-env.pin.test.ts new file mode 100644 index 0000000000..34d14f0709 --- /dev/null +++ b/packages/cli/src/commands/artifact-child-env.pin.test.ts @@ -0,0 +1,241 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Pin: **the presence of `OS_ARTIFACT_PATH` in a config's environment means an + * operator set it.** + * + * `os start` and `os dev` spawn `os serve`, and the downstream + * `objectstack.config.ts` is evaluated inside that child. While the supervisors + * wrote their own resolved artifact path into the child's `OS_ARTIFACT_PATH`, + * the variable was set on **every** boot — so a config could not tell an + * operator's instruction from the CLI's own plumbing, and a consumer wanting to + * refuse the retired knob could only do so by inspecting its *value*. + * + * The plumbing now travels on `OS_INTERNAL_ARTIFACT_PATH` + * (`utils/internal-artifact-channel.ts`). This file pins both halves of the + * property, plus the two behaviours that had to survive the move: the + * resolution ladder, and `start`'s deliberate refusal to declare an empty boot + * acceptable when a reference is driving the boot. + * + * Two kinds of assertion here, and both are needed: + * + * - **Behavioural** — over `childEnvWithResolvedArtifact`, which is the whole + * of what each command contributes to its child's artifact environment. + * - **Structural** — a source assertion that neither command writes + * `OS_ARTIFACT_PATH` into an env object at all. The behavioural pins describe + * the helper; only this one refuses a future edit that re-adds the write + * beside it. Both files compose their child env as + * `{ ...childEnvWithResolvedArtifact(process.env, …), …other keys }`, so + * "the helper is correct" plus "nothing else writes the key" is what makes + * the composed env correct. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'path'; +import { + INTERNAL_ARTIFACT_PATH_ENV, + childEnvWithResolvedArtifact, + readInternalArtifactPath, +} from '../utils/internal-artifact-channel.js'; +import { resolveArtifactSource } from './start.js'; + +const ARTIFACT = '/srv/app/objectstack.json'; + +describe('the child `serve` env — OS_ARTIFACT_PATH means an operator set it', () => { + it('carries NO OS_ARTIFACT_PATH when the operator did not set one', () => { + const parentEnv = { PATH: '/usr/bin', NODE_ENV: 'production' }; + + for (const decision of [ + { kind: 'resolved', path: ARTIFACT }, + { kind: 'reference' }, + { kind: 'empty' }, + ] as const) { + const childEnv = childEnvWithResolvedArtifact(parentEnv, decision); + expect( + Object.prototype.hasOwnProperty.call(childEnv, 'OS_ARTIFACT_PATH'), + `decision ${decision.kind} must not introduce OS_ARTIFACT_PATH`, + ).toBe(false); + expect(childEnv.OS_ARTIFACT_PATH).toBeUndefined(); + } + }); + + it('still carries OS_ARTIFACT_PATH — verbatim — when the operator DID set one', () => { + const parentEnv = { OS_ARTIFACT_PATH: './dist/from-operator.json' }; + + for (const decision of [ + { kind: 'resolved', path: '/abs/dist/from-operator.json' }, + { kind: 'reference' }, + { kind: 'empty' }, + ] as const) { + const childEnv = childEnvWithResolvedArtifact(parentEnv, decision); + // Inherited untouched: the child sees exactly what the operator wrote, + // not an absolutised rewrite of it. + expect(childEnv.OS_ARTIFACT_PATH).toBe('./dist/from-operator.json'); + } + }); + + it('hands the resolved artifact down on the internal channel instead', () => { + const childEnv = childEnvWithResolvedArtifact({}, { kind: 'resolved', path: ARTIFACT }); + expect(childEnv[INTERNAL_ARTIFACT_PATH_ENV]).toBe(ARTIFACT); + expect(readInternalArtifactPath(childEnv)).toBe(ARTIFACT); + }); + + it('lets the parent OWN the internal channel — an inherited value never speaks for it', () => { + const parentEnv = { [INTERNAL_ARTIFACT_PATH_ENV]: '/stale/inherited.json' }; + + expect(childEnvWithResolvedArtifact(parentEnv, { kind: 'resolved', path: ARTIFACT })) + .toMatchObject({ [INTERNAL_ARTIFACT_PATH_ENV]: ARTIFACT }); + + for (const decision of [{ kind: 'reference' }, { kind: 'empty' }] as const) { + const childEnv = childEnvWithResolvedArtifact(parentEnv, decision); + expect( + readInternalArtifactPath(childEnv), + `decision ${decision.kind} resolved nothing, so the channel must be empty`, + ).toBeUndefined(); + expect(Object.prototype.hasOwnProperty.call(childEnv, INTERNAL_ARTIFACT_PATH_ENV)).toBe(false); + } + }); + + it('reads a blank channel value as no decision at all', () => { + expect(readInternalArtifactPath({})).toBeUndefined(); + expect(readInternalArtifactPath({ [INTERNAL_ARTIFACT_PATH_ENV]: '' })).toBeUndefined(); + expect(readInternalArtifactPath({ [INTERNAL_ARTIFACT_PATH_ENV]: ' ' })).toBeUndefined(); + }); +}); + +describe('OS_BOOT_EMPTY — the artifact-reference refusal survives the move', () => { + it('is NOT set when a reference (OS_ARTIFACT_URL) is driving the boot', () => { + // Load-bearing: setting it here would tell `serve` that booting an app-less + // kernel is an acceptable outcome, turning an unreachable artifact host + // into a silently empty platform instead of a loud refusal. + const childEnv = childEnvWithResolvedArtifact({}, { kind: 'reference' }); + expect(childEnv.OS_BOOT_EMPTY).toBeUndefined(); + expect(readInternalArtifactPath(childEnv)).toBeUndefined(); + }); + + it('is NOT set when an artifact was resolved', () => { + expect(childEnvWithResolvedArtifact({}, { kind: 'resolved', path: ARTIFACT }).OS_BOOT_EMPTY) + .toBeUndefined(); + }); + + it('is set only when nothing resolved and an empty boot IS the intent', () => { + expect(childEnvWithResolvedArtifact({}, { kind: 'empty' }).OS_BOOT_EMPTY).toBe('1'); + }); + + it('never CLEARS an operator-exported OS_BOOT_EMPTY (add-only, as before)', () => { + const parentEnv = { OS_BOOT_EMPTY: '1' }; + for (const decision of [ + { kind: 'resolved', path: ARTIFACT }, + { kind: 'reference' }, + { kind: 'empty' }, + ] as const) { + expect( + childEnvWithResolvedArtifact(parentEnv, decision).OS_BOOT_EMPTY, + `decision ${decision.kind} must not start clearing an inherited OS_BOOT_EMPTY`, + ).toBe('1'); + } + }); +}); + +describe('resolveArtifactSource — the resolution ladder is unchanged', () => { + let cwd: string; + let home: string; + + const write = (dir: string, rel: string) => { + const abs = path.join(dir, rel); + mkdirSync(path.dirname(abs), { recursive: true }); + writeFileSync(abs, '{}'); + return abs; + }; + + beforeEach(() => { + cwd = mkdtempSync(path.join(tmpdir(), 'os-artifact-cwd-')); + home = mkdtempSync(path.join(tmpdir(), 'os-artifact-home-')); + }); + afterEach(() => { + for (const d of [cwd, home]) { + try { rmSync(d, { recursive: true, force: true }); } catch { /* noop */ } + } + }); + + it('rung 1: --artifact wins over everything, including an operator OS_ARTIFACT_PATH', () => { + const flagFile = write(cwd, 'build/pinned.json'); + write(cwd, 'dist/objectstack.json'); + write(home, 'dist/objectstack.json'); + + const r = resolveArtifactSource('build/pinned.json', home, { + cwd, + env: { OS_ARTIFACT_PATH: '/from/env.json' }, + }); + expect(r?.path).toBe(flagFile); + }); + + it('rung 1: --artifact passes an http(s) URL through untouched', () => { + const url = 'https://cdn.example.com/app.json'; + expect(resolveArtifactSource(url, home, { cwd, env: {} })?.path).toBe(url); + }); + + it('rung 2: $OS_ARTIFACT_PATH wins over both auto-detected locations', () => { + write(cwd, 'dist/objectstack.json'); + write(home, 'dist/objectstack.json'); + + const r = resolveArtifactSource(undefined, home, { + cwd, + env: { OS_ARTIFACT_PATH: 'custom/app.json' }, + }); + // Anchored on the cwd, exactly as before — the ladder resolves it; the + // variable itself is inherited by the child untouched. + expect(r?.path).toBe(path.join(cwd, 'custom/app.json')); + }); + + it('rung 2: $OS_ARTIFACT_PATH may itself be an http(s) URL', () => { + const url = 'https://cdn.example.com/env.json'; + expect(resolveArtifactSource(undefined, home, { cwd, env: { OS_ARTIFACT_PATH: url } })?.path) + .toBe(url); + }); + + it('rung 3: /dist/objectstack.json wins over /dist', () => { + const cwdArtifact = write(cwd, 'dist/objectstack.json'); + write(home, 'dist/objectstack.json'); + expect(resolveArtifactSource(undefined, home, { cwd, env: {} })?.path).toBe(cwdArtifact); + }); + + it('rung 4: /dist/objectstack.json is the last resort', () => { + const homeArtifact = write(home, 'dist/objectstack.json'); + expect(resolveArtifactSource(undefined, home, { cwd, env: {} })?.path).toBe(homeArtifact); + }); + + it('rung 5: nothing reachable resolves to undefined', () => { + expect(resolveArtifactSource(undefined, home, { cwd, env: {} })).toBeUndefined(); + }); +}); + +describe('structural: the supervisors never write the operator knob', () => { + // Strip comments first — both files discuss OS_ARTIFACT_PATH at length, and + // the prose is exactly what this assertion must NOT read. + const codeOf = (file: string): string => { + const src = readFileSync(new URL(`./${file}`, import.meta.url), 'utf8'); + return src + .replace(/\/\*[\s\S]*?\*\//g, '') + .split('\n') + .map((line) => (line.trim().startsWith('//') ? '' : line.replace(/\/\/.*$/, ''))) + .join('\n'); + }; + + for (const file of ['start.ts', 'dev.ts']) { + it(`${file} contains no OS_ARTIFACT_PATH assignment`, () => { + const offenders = codeOf(file) + .split('\n') + .filter((line) => /OS_ARTIFACT_PATH\s*[:=]/.test(line)); + expect( + offenders, + `${file} must not write OS_ARTIFACT_PATH into a child environment — the CLI's own ` + + `resolved artifact travels on ${INTERNAL_ARTIFACT_PATH_ENV}, so that a downstream ` + + `objectstack.config.ts seeing OS_ARTIFACT_PATH knows an operator set it. ` + + `Reading process.env.OS_ARTIFACT_PATH (the operator's value) stays correct.`, + ).toEqual([]); + }); + } +}); diff --git a/packages/cli/src/commands/dev.ts b/packages/cli/src/commands/dev.ts index 07f7167b75..7f783bcb56 100644 --- a/packages/cli/src/commands/dev.ts +++ b/packages/cli/src/commands/dev.ts @@ -16,6 +16,7 @@ import { assessArtifactStaleness, formatMtimeGap, } from '../utils/dev-restart.js'; +import { childEnvWithResolvedArtifact } from '../utils/internal-artifact-channel.js'; import { readEnvWithDeprecation, isMcpServerEnabled } from '@objectstack/types'; import type { ResolvedProjectDatabaseUrl } from '@objectstack/runtime'; @@ -327,10 +328,18 @@ export default class Dev extends Command { fs.mkdirSync(path.dirname(resolvedDb.url.replace(/^file:/, '')), { recursive: true }); } const effectiveDb = resolvedDb.url; + // `dev` always has a resolved artifact by this point (it compiled one, or + // was handed one with `--artifact`, or is pointing at the canonical + // `/dist/objectstack.json`), so the decision is unconditionally + // `resolved` — exactly as unconditional as the `OS_ARTIFACT_PATH` write + // it replaces. What changed is the channel: the resolved path travels on + // the CLI's own `OS_INTERNAL_ARTIFACT_PATH`, so an `OS_ARTIFACT_PATH` + // seen by a downstream `objectstack.config.ts` means an operator set it. + // The operator's own value is inherited verbatim, and `dev`'s ladder + // above still honours it on the rung it has always occupied. const localEnv: NodeJS.ProcessEnv = { - ...process.env, + ...childEnvWithResolvedArtifact(process.env, { kind: 'resolved', path: artifactPath }), OS_ENVIRONMENT_ID: environmentId, - OS_ARTIFACT_PATH: artifactPath, OS_SEED_ADMIN: seedAdmin ? '1' : '0', ...(seedAdmin && flags['admin-email'] ? { OS_SEED_ADMIN_EMAIL: flags['admin-email'] } : {}), ...(seedAdmin && flags['admin-password'] ? { OS_SEED_ADMIN_PASSWORD: flags['admin-password'] } : {}), diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 24b262f548..608e93666b 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -9,6 +9,7 @@ import { bundleRequire } from 'bundle-require'; import { loadConfig, BUNDLE_REQUIRE_EXTERNALS } from '../utils/config.js'; import { mergeBootConfig } from '../utils/merge-boot-config.js'; import { isHostConfig, shouldBootWithLibrary } from '../utils/plugin-detection.js'; +import { readInternalArtifactPath } from '../utils/internal-artifact-channel.js'; import { resolveDriverType, resolveStorageDefinition, @@ -956,7 +957,19 @@ export default class Serve extends Command { // fragment. It is resolved here, before anything else looks for an // artifact, and it wins over every local lookup: // - // --artifact > OS_ARTIFACT_URL > OS_ARTIFACT_PATH > /dist/… + // --artifact > OS_ARTIFACT_URL > OS_INTERNAL_ARTIFACT_PATH + // > OS_ARTIFACT_PATH > /dist/… + // + // `OS_INTERNAL_ARTIFACT_PATH` is the CLI's private parent-to-child channel: + // an `os start` / `os dev` supervisor resolved an artifact through its own + // ladder and is handing the answer down. It sits BELOW the reference (a + // supervisor that saw OS_ARTIFACT_URL resolves nothing and sends nothing, + // and `os dev` sends its answer unconditionally, so the reference has to + // keep outranking it) and ABOVE the operator's OS_ARTIFACT_PATH (which the + // supervisor no longer overwrites on the way down, so only a higher rung + // keeps `--artifact` beating an exported OS_ARTIFACT_PATH the way it does + // today). See `utils/internal-artifact-channel.ts` for why the CLI stopped + // writing the operator's knob at all. // // Beating OS_ARTIFACT_PATH is not a nicety, it is the acceptance // criterion: the official runtime image sets @@ -1018,7 +1031,11 @@ export default class Serve extends Command { if (configMissing && !pinnedArtifact) { const { resolveDefaultArtifactPath } = await import('@objectstack/runtime'); - const artifactSource = resolveDefaultArtifactPath(); + // A supervising `os start` / `os dev` passes its already-resolved answer + // as the explicit override — the same position `OS_ARTIFACT_PATH` used to + // occupy when the supervisor wrote it, so a named-but-missing artifact is + // still a loud refusal rather than a silent empty boot. + const artifactSource = resolveDefaultArtifactPath(readInternalArtifactPath()); if (!artifactSource) { // Quick-start mode: `objectstack start` lets the user boot an // empty kernel with no config and no artifact, then install apps @@ -1246,7 +1263,15 @@ export default class Serve extends Command { // what stops the loader from fetching the URL a second time — a pin // that verifies one response while a different response boots would // verify nothing. - ...(pinnedArtifact ? { artifactPath: pinnedArtifact.localPath } : {}), + // Same reasoning one rung down: when no reference is in play, a + // supervisor's resolved answer is handed over explicitly instead of + // being re-derived from the environment. + ...(pinnedArtifact + ? { artifactPath: pinnedArtifact.localPath } + : (() => { + const internal = readInternalArtifactPath(); + return internal ? { artifactPath: internal } : {}; + })()), }); // [#4002] `api` merges per key — see mergeBootConfig. A shallow spread // let the boot builder's two scoping keys wipe the author's whole `api` @@ -1765,7 +1790,11 @@ export default class Serve extends Command { if (!hasMetadataPlugin) { try { const { resolveDefaultArtifactPath } = await import('@objectstack/runtime'); - const hmrArtifactPath = resolveDefaultArtifactPath(); + // `os dev` is the only caller that reaches here, and it is a + // supervisor: read its channel, or the artifact this HMR watcher + // polls would silently drift to `/dist/objectstack.json` + // whenever `os dev --artifact ` was used. + const hmrArtifactPath = resolveDefaultArtifactPath(readInternalArtifactPath()); if (hmrArtifactPath && !/^https?:\/\//i.test(hmrArtifactPath)) { const { MetadataPlugin } = await import('@objectstack/metadata'); // Mirror the standalone stack's dev config exactly diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index b6e6488c4e..ac91009016 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -12,6 +12,7 @@ import { printHeader, printKV, printStep, printError } from '../utils/format.js' import { redirectStdoutToStderr } from '../utils/json-stdout.js'; import { redactConnectionUrl } from '../utils/connection-display.js'; import { databaseDriverFlag } from '../utils/database-driver-flag.js'; +import { childEnvWithResolvedArtifact } from '../utils/internal-artifact-channel.js'; import { readEnvWithDeprecation } from '@objectstack/types'; import type { ResolvedProjectDatabaseUrl } from '@objectstack/runtime'; @@ -182,6 +183,18 @@ export default class Start extends Command { // Priority: --artifact > $OS_ARTIFACT_PATH > ./dist/objectstack.json // > /dist/objectstack.json > none // + // This ladder resolves in the PARENT and is unchanged. What changed is how + // the answer reaches the child: it travels on the CLI's own + // `OS_INTERNAL_ARTIFACT_PATH` channel, never on `OS_ARTIFACT_PATH`, so an + // `OS_ARTIFACT_PATH` visible to a downstream `objectstack.config.ts` means + // an operator set it. See `utils/internal-artifact-channel.ts`. + // + // Note every read of `process.env.OS_ARTIFACT_PATH` in this command — the + // ladder's second rung below, and the auto-compile guard — is a read of the + // PARENT's environment, i.e. of the operator's own value. This command + // never mutates `process.env`; it composes a separate child env. So those + // guards see exactly what they saw before. + // // In project mode (objectstack.config.ts present) we additionally // auto-compile the config to ./dist/objectstack.json when no // artifact has been built yet, so `os start` works on a fresh @@ -191,8 +204,14 @@ export default class Start extends Command { // NOT resolve it — `serve` does, once, and owns the fetch, the `#sha256=` // verification, the protocol handshake and the migration gate. All `start` // does is get out of the way: no local lookup, no auto-compile, and no - // OS_ARTIFACT_PATH / OS_BOOT_EMPTY in the child env that would contradict - // the reference. The variable itself is inherited by the child. + // resolved-artifact channel or OS_BOOT_EMPTY in the child env that would + // contradict the reference. The variable itself is inherited by the child. + // + // That "nothing in the child env that contradicts the reference" intent is + // now general rather than special-cased: the child env carries a resolved + // artifact only when this command actually resolved one, on a channel of + // the CLI's own — so the operator-facing knob says one thing and one thing + // only. // // An explicit `--artifact` still wins (flags over env, as everywhere in // this command), and it wins by REMOVING the variable from the child env — @@ -293,8 +312,25 @@ export default class Start extends Command { // ── Child env ─────────────────────────────────────────────────── // Flags win over inherited env. When no artifact was located, signal // serve.ts to boot an empty kernel via OS_BOOT_EMPTY=1. + // #8368: with OS_ARTIFACT_URL in play, neither the resolved-artifact + // channel nor OS_BOOT_EMPTY is set — the child resolves the reference + // itself. OS_BOOT_EMPTY in particular must NOT be set there: it would tell + // `serve` that booting an app-less kernel is an acceptable outcome, turning + // an unreachable artifact host into a silently empty platform instead of + // the loud refusal acceptance #2 asks for. + // + // The resolved path travels on `OS_INTERNAL_ARTIFACT_PATH`, so an + // `OS_ARTIFACT_PATH` the child sees is the operator's own, inherited + // verbatim and never written by this command. const localEnv: NodeJS.ProcessEnv = { - ...process.env, + ...childEnvWithResolvedArtifact( + process.env, + artifactUrl + ? { kind: 'reference' } + : artifactSource + ? { kind: 'resolved', path: artifactSource.path } + : { kind: 'empty' }, + ), OS_HOME: homeDir, OS_ENVIRONMENT_ID: environmentId, OS_DATABASE_URL: databaseUrl, @@ -302,17 +338,6 @@ export default class Start extends Command { ...(flags['database-driver'] ? { OS_DATABASE_DRIVER: flags['database-driver'] } : {}), ...(flags['database-auth-token'] ? { OS_DATABASE_AUTH_TOKEN: flags['database-auth-token'] } : {}), AUTH_SECRET: authSecret, - // #8368: with OS_ARTIFACT_URL in play, neither knob is set — the child - // resolves the reference itself. OS_BOOT_EMPTY in particular must NOT be - // set here: it would tell `serve` that booting an app-less kernel is an - // acceptable outcome, turning an unreachable artifact host into a - // silently empty platform instead of the loud refusal acceptance #2 asks - // for. - ...(artifactUrl - ? {} - : artifactSource - ? { OS_ARTIFACT_PATH: artifactSource.path } - : { OS_BOOT_EMPTY: '1' }), }; // Flags over env: an explicit --artifact removes the reference rather than // racing it (see the resolution note above). @@ -401,15 +426,34 @@ function resolveHome( return path.resolve(os.homedir(), '.objectstack'); } -interface ResolvedArtifact { - /** Absolute path or URL passed to OS_ARTIFACT_PATH. */ +export interface ResolvedArtifact { + /** + * Absolute path or URL handed to the child on the CLI's internal channel + * (`OS_INTERNAL_ARTIFACT_PATH`) — never on the operator's `OS_ARTIFACT_PATH`. + */ path: string; /** Human-friendly form for the banner. */ display: string; } -function resolveArtifactSource(flagValue: string | undefined, homeDir: string): ResolvedArtifact | undefined { - const cwd = process.cwd(); +/** + * `start`'s artifact resolution ladder, in one place: + * + * `--artifact` > `$OS_ARTIFACT_PATH` > `/dist/objectstack.json` + * > `/dist/objectstack.json` > none + * + * Exported (with `cwd` / `env` injectable) so the ladder itself is pinned + * rather than inferred: moving the CLI's plumbing off `OS_ARTIFACT_PATH` must + * not shift a single rung, and the operator's `$OS_ARTIFACT_PATH` in + * particular must keep being honoured exactly where it is honoured today. + */ +export function resolveArtifactSource( + flagValue: string | undefined, + homeDir: string, + opts: { cwd?: string; env?: NodeJS.ProcessEnv } = {}, +): ResolvedArtifact | undefined { + const cwd = opts.cwd ?? process.cwd(); + const env = opts.env ?? process.env; // Explicit flag wins, including URLs. if (flagValue) { @@ -423,8 +467,10 @@ function resolveArtifactSource(flagValue: string | undefined, homeDir: string): return { path: abs, display: path.relative(cwd, abs) }; } - // Explicit env var wins next. - const envPath = process.env.OS_ARTIFACT_PATH; + // Explicit env var wins next — the OPERATOR's value, read from the parent + // environment. It is resolved here and passed down on the internal channel; + // the variable itself is inherited by the child untouched. + const envPath = env.OS_ARTIFACT_PATH; if (envPath) { if (/^https?:\/\//i.test(envPath)) return { path: envPath, display: envPath }; const abs = path.resolve(cwd, envPath); diff --git a/packages/cli/src/utils/internal-artifact-channel.ts b/packages/cli/src/utils/internal-artifact-channel.ts new file mode 100644 index 0000000000..8a4c9de733 --- /dev/null +++ b/packages/cli/src/utils/internal-artifact-channel.ts @@ -0,0 +1,134 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The CLI's INTERNAL parent-to-child artifact channel. + * + * `os start` and `os dev` are supervisors: each resolves an artifact, then + * spawns `os serve` to boot it. They used to hand the resolved path down by + * writing `OS_ARTIFACT_PATH` into the child environment — the same variable an + * operator sets to name an artifact. That made the two indistinguishable + * downstream: every `objectstack.config.ts` evaluated inside the child saw + * `OS_ARTIFACT_PATH` set on **every** boot, including boots where no operator + * had ever mentioned it, so a config could not answer "did a human ask for + * this, or did the CLI put it here?". + * + * This module moves the CLI's own plumbing onto a variable the CLI owns, and + * restores the property downstream consumers need: + * + * **the presence of `OS_ARTIFACT_PATH` in a config's environment means an + * operator set it.** + * + * ## Why this name + * + * `OS_INTERNAL_ARTIFACT_PATH` keeps the mandatory `OS_` prefix and the + * `OS_{DOMAIN}_{FEATURE}_{QUALIFIER}` shape (AGENTS.md Prime Directive #9), + * with `INTERNAL` in the domain slot. There is no `INTERNAL` subsystem, and + * that is the point: like the deliberately ungrouped, deliberately + * scary-looking `OS_ALLOW_*` escape hatches, the name groups with nothing and + * reads at a glance as "not a knob you set". It is deliberately **not** listed + * in `content/docs/deployment/environment-variables.mdx` — a documented name is + * a supported name, and this one is a private call between two processes the + * CLI owns both ends of. + * + * ## Precedence is unchanged + * + * The channel is read by `serve` strictly between `OS_ARTIFACT_URL` and + * `OS_ARTIFACT_PATH`: + * + * `--artifact` > `OS_ARTIFACT_URL` > `OS_INTERNAL_ARTIFACT_PATH` > `OS_ARTIFACT_PATH` > `/dist/objectstack.json` + * + * That position is what preserves today's answers exactly, in both directions: + * + * - It must beat `OS_ARTIFACT_PATH`, because `os start --artifact X` run with + * an operator's `OS_ARTIFACT_PATH=Y` in the environment boots **X** today + * (the parent overwrote the variable on the way down). The operator's `Y` is + * now inherited by the child untouched, so only a higher-precedence channel + * keeps X winning. + * - It must lose to `OS_ARTIFACT_URL`, because `os dev` writes the channel + * unconditionally — as it wrote `OS_ARTIFACT_PATH` unconditionally — and + * `OS_ARTIFACT_URL` outranks `OS_ARTIFACT_PATH` in `serve` today. + * + * The parent's own resolution ladder is untouched, and so is the value: the + * child is handed exactly the path the parent resolved, "named" in the sense + * `resolveDefaultArtifactPath` means it — a named artifact that is missing is + * still a loud refusal, never a silent empty boot. + */ + +/** + * The internal channel's variable name. Not an operator-facing knob — see the + * module docblock for why it is spelled this way. + */ +export const INTERNAL_ARTIFACT_PATH_ENV = 'OS_INTERNAL_ARTIFACT_PATH'; + +/** + * What a supervisor command decided about the artifact, as handed to the child. + * + * - `resolved` — a local path or `http(s)://` URL the parent resolved. It is + * passed down verbatim. + * - `reference` — `OS_ARTIFACT_URL` is driving this boot. The parent resolves + * nothing and says nothing: the child owns the fetch, the `#sha256=` + * verification and the refusal. + * - `empty` — nothing resolved, and booting an app-less kernel is the intended + * outcome (`os start`'s quick-start mode). + */ +export type ArtifactChannelDecision = + | { kind: 'resolved'; path: string } + | { kind: 'reference' } + | { kind: 'empty' }; + +/** + * Build the child environment for a `serve` child: the parent environment plus + * this command's artifact decision. + * + * Two deliberate asymmetries, both load-bearing: + * + * 1. **The parent OWNS `OS_INTERNAL_ARTIFACT_PATH` in the child env** — it is + * set on a `resolved` decision and *deleted* otherwise, so the value the + * child reads is a pure function of what the parent decided. An inherited + * copy can never speak for a decision the parent did not make. + * + * 2. **`OS_BOOT_EMPTY` is only ever ADDED, never removed.** An operator who + * exported it keeps whatever it means for them today; this function does not + * quietly start clearing it. What matters for the artifact-reference refusal + * is that the CLI does not *add* it on a `reference` boot — setting it there + * would tell `serve` that an app-less kernel is an acceptable outcome and + * turn an unreachable artifact host into a silently empty platform instead + * of the loud refusal the reference boot promises. + * + * `OS_ARTIFACT_PATH` is never written here, and never read here. Whatever the + * parent inherited is passed through untouched — including its exact spelling, + * so a config downstream sees the operator's own value rather than an + * absolutised rewrite of it. + */ +export function childEnvWithResolvedArtifact( + parentEnv: NodeJS.ProcessEnv, + decision: ArtifactChannelDecision, +): NodeJS.ProcessEnv { + const childEnv: NodeJS.ProcessEnv = { ...parentEnv }; + + if (decision.kind === 'resolved') { + childEnv[INTERNAL_ARTIFACT_PATH_ENV] = decision.path; + } else { + delete childEnv[INTERNAL_ARTIFACT_PATH_ENV]; + } + + if (decision.kind === 'empty') { + childEnv.OS_BOOT_EMPTY = '1'; + } + + return childEnv; +} + +/** + * Reader side, for `serve`: the artifact path a supervising `os start` / + * `os dev` already resolved for this process, if any. + * + * Returns `undefined` for an unset or blank value so an exported-but-empty + * variable cannot be mistaken for a decision. + */ +export function readInternalArtifactPath( + env: NodeJS.ProcessEnv = process.env, +): string | undefined { + const raw = env[INTERNAL_ARTIFACT_PATH_ENV]; + return raw && raw.trim() !== '' ? raw : undefined; +} From 8705d57007a90eb7ccb27124a0e340fd2ed016a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 16:49:59 +0000 Subject: [PATCH 2/2] test(cli): read the OS_ARTIFACT_PATH write pin off the AST, not a text scan (#8985) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../commands/artifact-child-env.pin.test.ts | 98 ++++++++++++++++--- 1 file changed, 85 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/commands/artifact-child-env.pin.test.ts b/packages/cli/src/commands/artifact-child-env.pin.test.ts index 34d14f0709..a657edabce 100644 --- a/packages/cli/src/commands/artifact-child-env.pin.test.ts +++ b/packages/cli/src/commands/artifact-child-env.pin.test.ts @@ -34,6 +34,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { mkdtempSync, mkdirSync, rmSync, writeFileSync, readFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'path'; +import ts from 'typescript'; import { INTERNAL_ARTIFACT_PATH_ENV, childEnvWithResolvedArtifact, @@ -213,24 +214,63 @@ describe('resolveArtifactSource — the resolution ladder is unchanged', () => { }); describe('structural: the supervisors never write the operator knob', () => { - // Strip comments first — both files discuss OS_ARTIFACT_PATH at length, and - // the prose is exactly what this assertion must NOT read. - const codeOf = (file: string): string => { + /** + * Read WRITES of `OS_ARTIFACT_PATH` off the TypeScript AST. + * + * Deliberately not a text scan. The first version of this pin stripped + * comments with a regex and reported `start.ts` clean while the file really + * did carry the write: the `--auth-secret` flag description contains the + * literal `/api/v1/auth/*`, whose `/*` opened a phantom block comment that + * swallowed 250 lines of real code, the injection among them. A detector that + * under-reports silently is worse than none — so the parser decides what is + * code and what is prose, and strings and comments cannot lie to it. + * + * Only writes are collected. Reading `process.env.OS_ARTIFACT_PATH` — the + * operator's own value, which both commands' ladders still honour — is + * correct and must stay possible. + */ + const artifactPathWrites = (file: string): string[] => { const src = readFileSync(new URL(`./${file}`, import.meta.url), 'utf8'); - return src - .replace(/\/\*[\s\S]*?\*\//g, '') - .split('\n') - .map((line) => (line.trim().startsWith('//') ? '' : line.replace(/\/\/.*$/, ''))) - .join('\n'); + const sourceFile = ts.createSourceFile(file, src, ts.ScriptTarget.Latest, true); + const hits: string[] = []; + + const at = (node: ts.Node) => + `${file}:${sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)).line + 1}`; + const staticName = (node: ts.Node): string | undefined => + ts.isIdentifier(node) || ts.isStringLiteral(node) ? node.text : undefined; + + const visit = (node: ts.Node): void => { + // `{ OS_ARTIFACT_PATH: value }` and `{ OS_ARTIFACT_PATH }` + if ( + (ts.isPropertyAssignment(node) || ts.isShorthandPropertyAssignment(node)) + && staticName(node.name) === 'OS_ARTIFACT_PATH' + ) { + hits.push(`${at(node)} object property`); + } + // `env.OS_ARTIFACT_PATH = value` / `env['OS_ARTIFACT_PATH'] = value` + if (ts.isBinaryExpression(node) && node.operatorToken.kind === ts.SyntaxKind.EqualsToken) { + const lhs = node.left; + if (ts.isPropertyAccessExpression(lhs) && lhs.name.text === 'OS_ARTIFACT_PATH') { + hits.push(`${at(node)} property assignment`); + } + if ( + ts.isElementAccessExpression(lhs) + && staticName(lhs.argumentExpression) === 'OS_ARTIFACT_PATH' + ) { + hits.push(`${at(node)} indexed assignment`); + } + } + ts.forEachChild(node, visit); + }; + + visit(sourceFile); + return hits; }; for (const file of ['start.ts', 'dev.ts']) { - it(`${file} contains no OS_ARTIFACT_PATH assignment`, () => { - const offenders = codeOf(file) - .split('\n') - .filter((line) => /OS_ARTIFACT_PATH\s*[:=]/.test(line)); + it(`${file} writes OS_ARTIFACT_PATH nowhere`, () => { expect( - offenders, + artifactPathWrites(file), `${file} must not write OS_ARTIFACT_PATH into a child environment — the CLI's own ` + `resolved artifact travels on ${INTERNAL_ARTIFACT_PATH_ENV}, so that a downstream ` + `objectstack.config.ts seeing OS_ARTIFACT_PATH knows an operator set it. ` @@ -238,4 +278,36 @@ describe('structural: the supervisors never write the operator knob', () => { ).toEqual([]); }); } + + it('the detector itself sees a write that a comment-stripping text scan missed', () => { + // The specimen is `start.ts`'s own shape, with the `/*`-bearing string that + // defeated the text scan sitting above it. Without this, the pin above + // could go permanently green by failing to look. + // The trailing docblock matters: the `/*` inside the string only swallows + // code up to the next `*/`, and in the real file that closer is an ordinary + // docblock a few hundred lines further down. + const specimen = [ + "const flag = { description: 'mount /api/v1/auth/* (overrides $AUTH_SECRET)' };", + 'const childEnv = {', + ' ...process.env,', + ' OS_ARTIFACT_PATH: resolved.path,', + '};', + '/** An ordinary docblock, whose closer ends the phantom comment. */', + 'export const done = true;', + ].join('\n'); + + const sourceFile = ts.createSourceFile('specimen.ts', specimen, ts.ScriptTarget.Latest, true); + let found = 0; + const visit = (node: ts.Node): void => { + if (ts.isPropertyAssignment(node) && ts.isIdentifier(node.name) + && node.name.text === 'OS_ARTIFACT_PATH') found += 1; + ts.forEachChild(node, visit); + }; + visit(sourceFile); + expect(found).toBe(1); + + // ...and the text scan this replaced reports the same specimen clean. + const textScanned = specimen.replace(/\/\*[\s\S]*?\*\//g, ''); + expect(/OS_ARTIFACT_PATH\s*:/.test(textScanned)).toBe(false); + }); });