diff --git a/.changeset/rapid-lemurs-click.md b/.changeset/rapid-lemurs-click.md new file mode 100644 index 000000000..6493f4cf8 --- /dev/null +++ b/.changeset/rapid-lemurs-click.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3917 +--- +**A `scripts/`-side tool that fails unexpectedly under `--json-errors` now emits the documented `{ok:false, reason, message}` envelope** — it previously printed a raw stack trace, because the exit module under `scripts/` was a second hand-written copy that never gained the structured-error branch its `src/` twin has. The copy is now generated from one source and byte-compared in CI, so the two cannot drift again. (#3904) diff --git a/docs/json-errors.md b/docs/json-errors.md index eecc376f2..fdd34d59a 100644 --- a/docs/json-errors.md +++ b/docs/json-errors.md @@ -58,6 +58,15 @@ assert on the exit code and (if needed) the plain-text message. The "parse stderr as JSON" guidance below applies only to the structured-envelope branch (non-`ExitError` failures). +> **Which tools honor this.** Both surfaces that run `runMain` do: the compiled +> `gsd-core/bin/lib/cli-exit.cjs` and the `scripts/lib/cli-exit.cjs` that the +> repo's own `scripts/**` tooling requires. Before [#3904](https://github.com/open-gsd/gsd-core/issues/3904) +> the latter was a separate hand-written copy that never gained the +> structured-envelope branch, so a `scripts/`-side tool failing unexpectedly +> printed a raw stack trace even under `--json-errors`. It is now generated from +> the same source and byte-compared by `npm run lint:generated-sync`, so the two +> cannot answer differently again. + ## Degraded results vs faults — read this before writing a caller `gsd-tools` has **two** ways of telling you something went wrong, and they use **different exit diff --git a/eslint.config.mjs b/eslint.config.mjs index 79727f41f..41f7eb845 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -338,6 +338,12 @@ export default tseslint.config( // src/pattern.cts — module resolution for a .cts source is relative to // src/, not the output dir). Same verbatim-third-party exemption. 'src/vendor/**', + // #3904 (ADR-3889 Phase 0): tsc-generated runtime artifact — generated + // by scripts/gen-scripts-cli-exit.cjs from a fresh compile of + // src/cli-exit.cts, and byte-guarded by `npm run lint:generated-sync` + // (stricter than lint: it forbids ANY hand edit, not just bad ones). + // Lint the src/cli-exit.cts source, not this emitted copy. + 'scripts/lib/cli-exit.cjs', ], }, diff --git a/package.json b/package.json index 1b6fa578e..e193981a8 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ "gen:registry": "node scripts/gen-registry.cjs --write", "gen:install-tree": "node scripts/gen-install-tree-fixtures.cjs", "gen:section-manifest": "node scripts/gen-section-manifest.cjs --write", - "regen:derived": "npm run build && npm run gen:registry && node scripts/gen-adr-index.cjs --write && node scripts/gen-features.cjs --write && node scripts/gen-capability-matrix.cjs --write && node scripts/gen-inventory-manifest.cjs --write && node scripts/gen-context-index.cjs --write && node scripts/gen-state-md-docs.cjs --write && npm run gen:section-manifest && node scripts/sync-manifest-versions.cjs && npm run gen:install-tree", + "regen:derived": "npm run build && npm run gen:registry && node scripts/gen-adr-index.cjs --write && node scripts/gen-features.cjs --write && node scripts/gen-capability-matrix.cjs --write && node scripts/gen-inventory-manifest.cjs --write && node scripts/gen-context-index.cjs --write && node scripts/gen-state-md-docs.cjs --write && npm run gen:section-manifest && node scripts/sync-manifest-versions.cjs && npm run gen:install-tree && node scripts/gen-scripts-cli-exit.cjs --write", "validate:registry": "node scripts/validate-registry.cjs", "prepack": "npm run build:lib", "prepare": "npm run build:lib", @@ -128,7 +128,7 @@ "lint:test-file-count": "node scripts/lint-test-file-count.cjs", "lint:pr-checks": "node scripts/lint-pr-check-project-dir.cjs", "lint:changeset": "node scripts/changeset/lint.cjs", - "lint:generated-sync": "node scripts/gen-capability-registry.cjs --check && node scripts/gen-loop-host-contract.cjs --check && node scripts/gen-capability-matrix.cjs --check && node scripts/sync-manifest-versions.cjs --check && node scripts/gen-inventory-manifest.cjs --check && node scripts/generate-package-identity.cjs --check && node scripts/gen-plugin-skills.cjs --check && node scripts/gen-registry.cjs --check && node scripts/gen-adr-index.cjs --check && node scripts/gen-features.cjs --check && node scripts/check-glossary-refs.cjs --check && node scripts/lint-compiled-artifact-sync.cjs --check && node scripts/gen-context-index.cjs --check && node scripts/gen-section-manifest.cjs --check && node scripts/gen-health-docs.cjs --check && node scripts/gen-state-md-docs.cjs --check", + "lint:generated-sync": "node scripts/gen-capability-registry.cjs --check && node scripts/gen-loop-host-contract.cjs --check && node scripts/gen-capability-matrix.cjs --check && node scripts/sync-manifest-versions.cjs --check && node scripts/gen-inventory-manifest.cjs --check && node scripts/generate-package-identity.cjs --check && node scripts/gen-plugin-skills.cjs --check && node scripts/gen-registry.cjs --check && node scripts/gen-adr-index.cjs --check && node scripts/gen-features.cjs --check && node scripts/check-glossary-refs.cjs --check && node scripts/lint-compiled-artifact-sync.cjs --check && node scripts/gen-context-index.cjs --check && node scripts/gen-section-manifest.cjs --check && node scripts/gen-health-docs.cjs --check && node scripts/gen-state-md-docs.cjs --check && node scripts/gen-scripts-cli-exit.cjs --check", "lint:docs": "node scripts/lint-docs-required.cjs", "lint:qa-smells": "node scripts/qa-smell-ratchet.cjs", "lint:legacy-name": "node scripts/lint-legacy-dir-name.cjs", diff --git a/scripts/gen-scripts-cli-exit.cjs b/scripts/gen-scripts-cli-exit.cjs new file mode 100644 index 000000000..288a87a6e --- /dev/null +++ b/scripts/gen-scripts-cli-exit.cjs @@ -0,0 +1,185 @@ +#!/usr/bin/env node +/** + * gen-scripts-cli-exit.cjs — generates scripts/lib/cli-exit.cjs from a fresh + * compile of src/cli-exit.cts. + * + * ADR-3889 Phase 0 (#3904): scripts/lib/cli-exit.cjs used to be a hand-written + * fork of gsd-core/bin/lib/cli-exit.cjs (the compiled artifact of + * src/cli-exit.cts). The two drifted — only the .cts copy routed a + * non-ExitError throw through getJsonErrorMode() to emit a structured + * { ok:false, reason, message } envelope. This script makes scripts/lib/cli-exit.cjs + * a generated artifact of the SAME source, so the two surfaces cannot diverge + * again. + * + * scripts/ runs straight from the repo checkout and must work on an unbuilt + * clone (scripts/check-env.cjs requires this file before any build runs), so + * the generated file is compiled to a THROWAWAY outDir rather than read from + * gsd-core/bin/lib/ — reading the tracked build output would let a stale build + * produce a false green. + * + * Usage: + * node scripts/gen-scripts-cli-exit.cjs # same as --write + * node scripts/gen-scripts-cli-exit.cjs --write # write scripts/lib/cli-exit.cjs + * node scripts/gen-scripts-cli-exit.cjs --check # exit 1 if committed file is stale + */ + +'use strict'; + +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const REPO_ROOT = path.resolve(__dirname, '..'); +const OUTPUT_PATH = path.join(REPO_ROOT, 'scripts', 'lib', 'cli-exit.cjs'); +const COMPILE_TIMEOUT_MS = 60_000; + +/** Frozen reason codes so tests assert on structure, not prose. */ +const REASON = Object.freeze({ + OK: 'ok_generated_sync', + DRIFTED: 'fail_generated_drifted', + BUILD_FAILED: 'fail_build_failed', + MISSING_EMIT: 'fail_missing_emit', + USAGE: 'fail_usage', +}); + +const USAGE_MESSAGE = [ + 'Usage: node scripts/gen-scripts-cli-exit.cjs [--write|--check]', + ' (no flag) same as --write', + ' --write write scripts/lib/cli-exit.cjs', + ' --check exit 1 if the committed file is stale', +].join('\n'); + +const BANNER = [ + '// GENERATED FILE — DO NOT EDIT BY HAND.', + '// Source of truth: src/cli-exit.cts. Regenerate with:', + '// node scripts/gen-scripts-cli-exit.cjs --write', + '// Byte-compared by `npm run lint:generated-sync` (#3904, ADR-3889 Phase 0).', + '//', + '// Why this copy exists: scripts/ runs straight from the repo checkout and must', + '// work on an unbuilt clone — 64+ scripts require this file, including', + '// check-env.cjs, which runs before any build. gsd-core/bin/lib/cli-exit.cjs is', + '// gitignored tsc output and doubles as the build sentinel, so it cannot be', + '// required from here. Hence one source, two emitted locations.', + '', + '', +].join('\n'); + +/** Compile the whole project to a throwaway outDir so the work tree is untouched. */ +function compileToTemp() { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-scripts-cli-exit-')); + try { + execFileSync( + process.execPath, + [ + path.join(REPO_ROOT, 'node_modules', 'typescript', 'bin', 'tsc'), + '-p', path.join(REPO_ROOT, 'tsconfig.build.json'), + '--outDir', tmp, + // A throwaway outDir must not reuse the in-tree incremental state, or + // tsc skips emit for files it believes are already current. + '--incremental', 'false', + '--tsBuildInfoFile', 'null', + ], + { cwd: REPO_ROOT, encoding: 'utf8', stdio: 'pipe', timeout: COMPILE_TIMEOUT_MS }, + ); + return { ok: true, dir: tmp }; + } catch (err) { + fs.rmSync(tmp, { recursive: true, force: true }); + const detail = [err.stdout, err.stderr].filter(Boolean).join('\n').trim(); + return { ok: false, detail }; + } +} + +/** + * Compile src/cli-exit.cts to a throwaway outDir and return the expected + * generated content (banner + compiled bytes), or a failure descriptor. + * + * @returns {{ ok: true, content: string } | { ok: false, reason: string, detail?: string }} + */ +function buildExpectedContent() { + const build = compileToTemp(); + if (!build.ok) { + return { ok: false, reason: REASON.BUILD_FAILED, detail: build.detail }; + } + try { + const emitted = path.join(build.dir, 'cli-exit.cjs'); + if (!fs.existsSync(emitted)) { + return { ok: false, reason: REASON.MISSING_EMIT, detail: `no emit produced at ${emitted} from src/cli-exit.cts` }; + } + const compiled = fs.readFileSync(emitted, 'utf8'); + return { ok: true, content: BANNER + compiled }; + } finally { + fs.rmSync(build.dir, { recursive: true, force: true }); + } +} + +function doWrite() { + const result = buildExpectedContent(); + if (!result.ok) { + console.error(`FAIL gen-scripts-cli-exit: ${result.reason}`); + if (result.detail) console.error(result.detail); + return 1; + } + fs.mkdirSync(path.dirname(OUTPUT_PATH), { recursive: true }); + fs.writeFileSync(OUTPUT_PATH, result.content, 'utf8'); + console.log(`ok gen-scripts-cli-exit: wrote ${path.relative(REPO_ROOT, OUTPUT_PATH)}`); + return 0; +} + +function doCheck() { + const result = buildExpectedContent(); + if (!result.ok) { + console.error(`FAIL gen-scripts-cli-exit: ${result.reason}`); + if (result.detail) console.error(result.detail); + return 1; + } + + if (!fs.existsSync(OUTPUT_PATH)) { + console.error(`FAIL gen-scripts-cli-exit: ${REASON.MISSING_EMIT}`); + console.error(` ${path.relative(REPO_ROOT, OUTPUT_PATH)} does not exist. Run:`); + console.error(' node scripts/gen-scripts-cli-exit.cjs --write'); + return 1; + } + + const committed = fs.readFileSync(OUTPUT_PATH, 'utf8'); + if (committed !== result.content) { + console.error(`FAIL gen-scripts-cli-exit: ${REASON.DRIFTED}`); + console.error( + ` ${path.relative(REPO_ROOT, OUTPUT_PATH)} (${committed.length} bytes) != ` + + `compile of src/cli-exit.cts (${result.content.length} bytes)`, + ); + console.error(''); + console.error('Regenerate with:'); + console.error(' node scripts/gen-scripts-cli-exit.cjs --write'); + return 1; + } + + console.log(`ok gen-scripts-cli-exit: ${path.relative(REPO_ROOT, OUTPUT_PATH)} matches src/cli-exit.cts`); + return 0; +} + +function main() { + const flag = process.argv[2]; + const extra = process.argv[3]; + + if (flag !== undefined && flag !== '--write' && flag !== '--check') { + console.error(`FAIL gen-scripts-cli-exit: ${REASON.USAGE}`); + console.error(` unrecognized argument: ${flag}`); + console.error(USAGE_MESSAGE); + return 1; + } + + if (extra !== undefined) { + console.error(`FAIL gen-scripts-cli-exit: ${REASON.USAGE}`); + console.error(` unexpected extra argument: ${extra}`); + console.error(USAGE_MESSAGE); + return 1; + } + + if (flag === '--check') return doCheck(); + return doWrite(); +} + +if (require.main === module) process.exitCode = main(); + +module.exports = { REASON, buildExpectedContent, OUTPUT_PATH, BANNER }; diff --git a/scripts/lib/cli-exit.cjs b/scripts/lib/cli-exit.cjs index 709e8dc23..321756016 100644 --- a/scripts/lib/cli-exit.cjs +++ b/scripts/lib/cli-exit.cjs @@ -1,56 +1,105 @@ -'use strict'; +// GENERATED FILE — DO NOT EDIT BY HAND. +// Source of truth: src/cli-exit.cts. Regenerate with: +// node scripts/gen-scripts-cli-exit.cjs --write +// Byte-compared by `npm run lint:generated-sync` (#3904, ADR-3889 Phase 0). +// +// Why this copy exists: scripts/ runs straight from the repo checkout and must +// work on an unbuilt clone — 64+ scripts require this file, including +// check-env.cjs, which runs before any build. gsd-core/bin/lib/cli-exit.cjs is +// gitignored tsc output and doubles as the build sentinel, so it cannot be +// required from here. Hence one source, two emitted locations. +"use strict"; +var __importDefault = (this && this.__importDefault) || function (mod) { + return (mod && mod.__esModule) ? mod : { "default": mod }; +}; /** - * Error that carries a process exit code. CLI logic throws this instead of - * calling process.exit() (banned by n/no-process-exit); runMain() translates it - * into process.exitCode at the entrypoint. + * Process-exit primitives (ExitError, runMain) plus the json-error-mode cell. + * Must import nothing but `node:fs` — this source is emitted to TWO locations, + * gsd-core/bin/lib/cli-exit.cjs (tsc build output) and scripts/lib/cli-exit.cjs + * (a generated, committed artifact regenerated by scripts/gen-scripts-cli-exit.cjs), + * and the latter must load on an unbuilt clone before anything under ./lib exists. + */ +const node_fs_1 = __importDefault(require("node:fs")); +/** + * The wire value `runMain` stamps into its structured envelope. Declared HERE, + * not in io.cts, because this module must not import anything (see the module + * header): io.cts builds ERROR_REASON.SDK_FAIL_FAST from this constant, so the + * two surfaces share ONE definition rather than two literals kept in step by a + * parity test. + */ +const EXIT_ENVELOPE_REASON = 'sdk_fail_fast'; +/** + * Process-level flag: when true, error paths emit structured JSON to stderr + * instead of plain text. Set by gsd-tools.cjs when the CLI is invoked with + * `--json-errors`; re-exported by io.cts, which is where most callers reach it. * - * @param {number} code exit code (default 1) - * @param {string} [message] optional human message; when set and code != 0 it is - * written to stderr by runMain before the process exits. + * Held in a Symbol-keyed cell on globalThis rather than in module scope, and + * that is load-bearing: this module is emitted to TWO locations + * (gsd-core/bin/lib/cli-exit.cjs and the generated scripts/lib/cli-exit.cjs), + * so a process that loads both would get two independent module instances. A + * module-level `let` would give them two independent flags — one copy could + * think json mode is on while the other thought it was off, which is exactly + * the divergence class ADR-3889 exists to remove. One cell, keyed by a + * registry Symbol, makes that unrepresentable. + */ +const JSON_ERROR_MODE_KEY = Symbol.for('gsd.exit.jsonErrorMode'); +function setJsonErrorMode(v) { + globalThis[JSON_ERROR_MODE_KEY] = !!v; +} +function getJsonErrorMode() { + return globalThis[JSON_ERROR_MODE_KEY] === true; +} +/** + * Error carrying a process exit code. CLI logic throws this instead of calling + * process.exit() (banned by n/no-process-exit); runMain() translates it into + * process.exitCode at the entrypoint. */ class ExitError extends Error { - constructor(code = 1, message) { - super(message === undefined ? `process exit ${code}` : message); - this.name = 'ExitError'; - this.code = code; - // Whether runMain should print this.message to stderr (only when a real - // message was provided, not the synthetic default). - this.hasUserMessage = message !== undefined; - } + code; + hasUserMessage; + constructor(code = 1, message) { + super(message === undefined ? `process exit ${code}` : message); + this.name = 'ExitError'; + this.code = code; + this.hasUserMessage = message !== undefined; + } } - /** - * Run a CLI main function and translate its outcome into process.exitCode - * (never process.exit(), so n/no-process-exit stays satisfied). Supports sync or - * async main. - * - main returns a number -> process.exitCode = that number - * - main throws/rejects ExitError -> process.exitCode = err.code, and if - * err.hasUserMessage && err.code !== 0, err.message is written to stderr - * - main throws/rejects anything else -> the stack is written to stderr and - * process.exitCode = 1 - * Letting the event loop drain (vs process.exit) means buffered stdout/stderr is - * flushed and process.on('exit') cleanup handlers still fire. - * - * @param {() => (number|void|Promise)} main + * Run a CLI main and translate its outcome into process.exitCode (never + * process.exit, so n/no-process-exit stays satisfied; output flushes and + * process.on('exit') cleanup still fires). main may be sync or async: + * number return -> process.exitCode = it + * thrown ExitError -> process.exitCode = err.code (+ stderr err.message if hasUserMessage && code!=0) + * other throw -> when json-error mode is active, emits structured { ok:false, reason, message } + * to stderr; otherwise writes raw stack trace. exit code = 1 in either case. */ function runMain(main) { - Promise.resolve() - .then(() => main()) - .then((code) => { - if (typeof code === 'number') process.exitCode = code; - }) - .catch((err) => { - if (err instanceof ExitError) { - if (err.hasUserMessage && err.code !== 0) { - process.stderr.write(`${err.message}\n`); + Promise.resolve() + .then(() => main()) + .then((code) => { if (typeof code === 'number') + process.exitCode = code; }) + .catch((err) => { + if (err instanceof ExitError) { + if (err.hasUserMessage && err.code !== 0) + process.stderr.write(`${err.message}\n`); + process.exitCode = err.code; + return; } - process.exitCode = err.code; - return; - } - process.stderr.write(`${err && err.stack ? err.stack : String(err)}\n`); - process.exitCode = 1; + if (getJsonErrorMode()) { + const e = err; + const payload = JSON.stringify({ + ok: false, + reason: EXIT_ENVELOPE_REASON, + message: (e && e.message) ? e.message : String(err), + }) + '\n'; + node_fs_1.default.writeSync(2, payload); + } + else { + const e = err; + process.stderr.write(`${e && e.stack ? e.stack : String(err)}\n`); + } + process.exitCode = 1; }); } - -module.exports = { ExitError, runMain }; +module.exports = { ExitError, runMain, setJsonErrorMode, getJsonErrorMode, EXIT_ENVELOPE_REASON }; diff --git a/src/cli-exit.cts b/src/cli-exit.cts index ff7f93aa3..8ccf2a51f 100644 --- a/src/cli-exit.cts +++ b/src/cli-exit.cts @@ -1,7 +1,44 @@ +/** + * Process-exit primitives (ExitError, runMain) plus the json-error-mode cell. + * Must import nothing but `node:fs` — this source is emitted to TWO locations, + * gsd-core/bin/lib/cli-exit.cjs (tsc build output) and scripts/lib/cli-exit.cjs + * (a generated, committed artifact regenerated by scripts/gen-scripts-cli-exit.cjs), + * and the latter must load on an unbuilt clone before anything under ./lib exists. + */ import fs from 'node:fs'; -// eslint-disable-next-line @typescript-eslint/no-require-imports -import ioModule = require('./io.cjs'); -const { getJsonErrorMode, ERROR_REASON } = ioModule; + +/** + * The wire value `runMain` stamps into its structured envelope. Declared HERE, + * not in io.cts, because this module must not import anything (see the module + * header): io.cts builds ERROR_REASON.SDK_FAIL_FAST from this constant, so the + * two surfaces share ONE definition rather than two literals kept in step by a + * parity test. + */ +const EXIT_ENVELOPE_REASON = 'sdk_fail_fast' as const; + +/** + * Process-level flag: when true, error paths emit structured JSON to stderr + * instead of plain text. Set by gsd-tools.cjs when the CLI is invoked with + * `--json-errors`; re-exported by io.cts, which is where most callers reach it. + * + * Held in a Symbol-keyed cell on globalThis rather than in module scope, and + * that is load-bearing: this module is emitted to TWO locations + * (gsd-core/bin/lib/cli-exit.cjs and the generated scripts/lib/cli-exit.cjs), + * so a process that loads both would get two independent module instances. A + * module-level `let` would give them two independent flags — one copy could + * think json mode is on while the other thought it was off, which is exactly + * the divergence class ADR-3889 exists to remove. One cell, keyed by a + * registry Symbol, makes that unrepresentable. + */ +const JSON_ERROR_MODE_KEY = Symbol.for('gsd.exit.jsonErrorMode'); + +function setJsonErrorMode(v: unknown): void { + (globalThis as unknown as Record)[JSON_ERROR_MODE_KEY] = !!v; +} + +function getJsonErrorMode(): boolean { + return (globalThis as unknown as Record)[JSON_ERROR_MODE_KEY] === true; +} /** * Error carrying a process exit code. CLI logic throws this instead of calling @@ -42,7 +79,7 @@ function runMain(main: () => number | void | Promise): void { const e = err as Error; const payload = JSON.stringify({ ok: false, - reason: ERROR_REASON.SDK_FAIL_FAST, + reason: EXIT_ENVELOPE_REASON, message: (e && e.message) ? e.message : String(err), }) + '\n'; fs.writeSync(2, payload); @@ -54,4 +91,4 @@ function runMain(main: () => number | void | Promise): void { }); } -export = { ExitError, runMain }; +export = { ExitError, runMain, setJsonErrorMode, getJsonErrorMode, EXIT_ENVELOPE_REASON }; diff --git a/src/io.cts b/src/io.cts index bcc6c4227..a5a64aecf 100644 --- a/src/io.cts +++ b/src/io.cts @@ -12,6 +12,9 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { platformWriteSync, platformEnsureDir } from './shell-command-projection.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import cliExitModule = require('./cli-exit.cjs'); +const { setJsonErrorMode, getJsonErrorMode, EXIT_ENVELOPE_REASON } = cliExitModule; // ─── Temp-file helpers (needed by output()) ────────────────────────────────── @@ -184,7 +187,7 @@ const ERROR_REASON = Object.freeze({ CONFIG_PARSE_FAILED: 'config_parse_failed', CONFIG_INVALID_KEY: 'config_invalid_key', // SDK / gsd-tools dispatch - SDK_FAIL_FAST: 'sdk_fail_fast', + SDK_FAIL_FAST: EXIT_ENVELOPE_REASON, SDK_UNKNOWN_COMMAND: 'sdk_unknown_command', SDK_MISSING_ARG: 'sdk_missing_arg', // workflow / phase @@ -220,18 +223,8 @@ const ERROR_REASON = Object.freeze({ type ErrorReasonValue = typeof ERROR_REASON[keyof typeof ERROR_REASON]; -/** - * Process-level flag: when true, error() emits structured JSON to stderr - * instead of plain "Error: " text. Set by gsd-tools.cjs when the - * CLI is invoked with `--json-errors`. Tests opt in to typed-IR error - * assertions by passing that flag and parsing the JSON. - * - * Default off so existing callers and human operators keep their plain-text - * diagnostics. The structured form is opt-in for tooling and tests (#2974). - */ -let _jsonErrorMode = false; -function setJsonErrorMode(v: unknown): void { _jsonErrorMode = !!v; } -function getJsonErrorMode(): boolean { return _jsonErrorMode; } +// setJsonErrorMode / getJsonErrorMode now live in cli-exit.cts (imported above) +// and are re-exported here for the callers that already import them from io. /** * Emit an error and exit. When the second argument is provided it must be @@ -248,7 +241,7 @@ function getJsonErrorMode(): boolean { return _jsonErrorMode; } * message is the only thing an operator sees there. */ function error(message: string, reason: ErrorReasonValue = ERROR_REASON.UNKNOWN, extra?: Record): never { - if (_jsonErrorMode) { + if (getJsonErrorMode()) { const payload = JSON.stringify({ ok: false, reason, message, ...(extra || {}) }) + '\n'; writeAllSync(2, payload); } else { diff --git a/tests/cli-exit.test.cjs b/tests/cli-exit.test.cjs index bb2cef742..ace786fd3 100644 --- a/tests/cli-exit.test.cjs +++ b/tests/cli-exit.test.cjs @@ -3,16 +3,19 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const path = require('node:path'); +const fs = require('node:fs'); const { ExitError, runMain } = require('../scripts/lib/cli-exit.cjs'); const { runNode } = require('./helpers/process-seam.cjs'); const { toLegacyResult } = require('./helpers/git-fixture.cjs'); const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); // Paths to the compiled product seam (src/cli-exit.cts → gsd-core/bin/lib/cli-exit.cjs) // used for json-error mode regression tests which require io.cjs integration. const BUILT_CLI_EXIT_PATH = path.resolve(__dirname, '../gsd-core/bin/lib/cli-exit.cjs'); const IO_PATH = path.resolve(__dirname, '../gsd-core/bin/lib/io.cjs'); +const SCRIPTS_CLI_EXIT_PATH = path.resolve(__dirname, '../scripts/lib/cli-exit.cjs'); /** Settle the runMain promise chain before asserting. */ async function settle() { @@ -285,4 +288,344 @@ describe('regressions', () => { assert.ok(envParsed.message, 'envelope must carry a message'); }); }); + + /** + * #3904 (epic #3889, ADR-3889 P0) — scripts/lib/cli-exit.cjs was a SECOND + * hand-written implementation of this seam, and it had no json-error arm at + * all: an unexpected throw printed a raw stack trace where the documented + * contract promises { ok:false, reason, message }. 64+ files under scripts/ + * require that copy. + * + * Fix: scripts/lib/cli-exit.cjs is now GENERATED from src/cli-exit.cts's + * compiled output and byte-compared by scripts/gen-scripts-cli-exit.cjs + * --check, so the two cannot diverge again. + * + * These run against the SCRIPTS copy specifically — the sibling bug-965 block + * above deliberately targets the built copy, which is exactly how the drift + * stayed invisible. + */ + describe('bug-3904: the scripts copy is the same artifact as the built one', () => { + /** Build a one-shot driver script for whichever copy is under test. */ + function driver(modulePath, { jsonMode, throwExpr }) { + return [ + `const cliExit = require(${JSON.stringify(modulePath)});`, + `const { runMain, ExitError } = cliExit;`, + `void ExitError;`, + `cliExit.setJsonErrorMode(${jsonMode});`, + `runMain(() => { throw ${throwExpr}; });`, + `setImmediate(() => {});`, + ].join('\n'); + } + + /** + * Drive the SCRIPTS copy. json-error mode is set through the scripts copy's + * own accessor, because a scripts/ consumer on an unbuilt clone has no + * io.cjs to reach for — that independence is part of what is under test. + */ + function spawnScriptsRun(opts) { + return toLegacyResult( + runNode(['-e', driver(SCRIPTS_CLI_EXIT_PATH, opts)], { timeoutMs: PROBE_TIMEOUT_MS }), + ); + } + + /** The same driver, pointed at the BUILT copy, for the parity row. */ + function spawnBuiltRun(opts) { + return toLegacyResult( + runNode(['-e', driver(BUILT_CLI_EXIT_PATH, opts)], { timeoutMs: PROBE_TIMEOUT_MS }), + ); + } + + /** Run a snippet that prints JSON on stdout, and return the parsed value. */ + function readJsonFromChild(lines) { + const r = toLegacyResult(runNode(['-e', lines.join('\n')], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.strictEqual(r.status, 0, `child exited ${r.status}; stderr: ${r.stderr}`); + return JSON.parse(r.stdout); + } + + /** Parse stderr as a single JSON object, failing with the raw text if it is not one. */ + function parseEnvelope(result) { + const trimmed = result.stderr.trim(); + try { + return JSON.parse(trimmed); + } catch (e) { + return assert.fail( + `stderr is NOT a single JSON object (raw stack leaked through):\n${trimmed}\nparse error: ${e.message}`, + ); + } + } + + // ── Matrix rows 1-3: the reported defect, at the consumer's output ──────── + test('scripts copy emits the structured envelope on an unexpected throw under json mode', () => { + const result = spawnScriptsRun({ jsonMode: true, throwExpr: `new TypeError('unexpected boom')` }); + assert.strictEqual(result.status, 1, `expected exit 1; stderr: ${result.stderr}`); + const parsed = parseEnvelope(result); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.reason, 'sdk_fail_fast'); + assert.ok( + String(parsed.message).includes('unexpected boom'), + `expected the thrown text in message, got: ${JSON.stringify(parsed.message)}`, + ); + }); + + test('scripts copy envelope covers RangeError as well as TypeError', () => { + const result = spawnScriptsRun({ jsonMode: true, throwExpr: `new RangeError('out of bounds')` }); + assert.strictEqual(result.status, 1); + const parsed = parseEnvelope(result); + assert.strictEqual(parsed.reason, 'sdk_fail_fast'); + assert.ok(String(parsed.message).includes('out of bounds')); + }); + + test('scripts copy writes the envelope to stderr and leaves stdout empty', () => { + const result = spawnScriptsRun({ jsonMode: true, throwExpr: `new TypeError('boom')` }); + assert.strictEqual(result.stdout, '', `expected empty stdout, got: ${result.stdout}`); + }); + + // ── Matrix rows 4-5: negative space — what must NOT become an envelope ──── + test('scripts copy preserves the raw stack trace when json mode is off', () => { + const result = spawnScriptsRun({ jsonMode: false, throwExpr: `new TypeError('unexpected boom')` }); + assert.strictEqual(result.status, 1); + const trimmed = result.stderr.trim(); + let parsed = null; + try { parsed = JSON.parse(trimmed); } catch { /* expected — not JSON */ } + assert.strictEqual(parsed, null, `expected a raw stack in plain mode, got JSON: ${trimmed.slice(0, 200)}`); + assert.ok(trimmed.includes('unexpected boom'), `expected the thrown text; got: ${trimmed.slice(0, 200)}`); + }); + + test('scripts copy keeps ExitError plain-text under json mode', () => { + const result = spawnScriptsRun({ + jsonMode: true, + throwExpr: `new ExitError(1, 'Usage: gsd-tools [args]')`, + }); + assert.strictEqual(result.status, 1, 'ExitError exits with its own code'); + const trimmed = result.stderr.trim(); + let parsed = null; + try { parsed = JSON.parse(trimmed); } catch { /* expected — plain text */ } + assert.strictEqual(parsed, null, `ExitError must stay plain text; got JSON: ${trimmed.slice(0, 200)}`); + assert.ok(trimmed.includes('Usage'), `plain-text message must reach stderr; got: ${trimmed.slice(0, 200)}`); + }); + + // ── Matrix rows 6-10: non-Error throws reach String(err) ───────────────── + for (const [label, throwExpr, expectedMessage] of [ + ['a thrown string', `'a bare string'`, 'a bare string'], + ['a thrown null', `null`, 'null'], + ['a thrown undefined', `undefined`, 'undefined'], + ['an Error with an empty message', `new Error('')`, 'Error'], + ]) { + test(`scripts copy envelope handles ${label}`, () => { + const result = spawnScriptsRun({ jsonMode: true, throwExpr }); + assert.strictEqual(result.status, 1, `expected exit 1; stderr: ${result.stderr}`); + const parsed = parseEnvelope(result); + assert.strictEqual(parsed.ok, false); + assert.strictEqual(parsed.reason, 'sdk_fail_fast'); + assert.ok( + String(parsed.message).includes(expectedMessage), + `expected ${JSON.stringify(expectedMessage)} in message, got ${JSON.stringify(parsed.message)}`, + ); + }); + } + + test('scripts copy envelope stays parseable when the message contains quotes and newlines', () => { + // Proves JSON.stringify is doing the encoding rather than string concatenation: + // an unescaped quote or newline would split stderr into something JSON.parse rejects. + const hostile = 'he said "hi"\nthen \\left\ttab'; + const result = spawnScriptsRun({ jsonMode: true, throwExpr: `new Error(${JSON.stringify(hostile)})` }); + assert.strictEqual(result.status, 1); + const parsed = parseEnvelope(result); + assert.strictEqual(parsed.message, hostile, 'the message must round-trip byte-for-byte'); + }); + + // ── Matrix row 11: the two copies are one artifact ─────────────────────── + // Stack-trace bytes are NOT the contract here: on the json=false path, stderr + // is a raw stack trace, and the generated scripts/ copy carries an 11-line + // provenance banner that the built copy does not, so every frame line number + // is offset by exactly that banner length, and the two files necessarily sit + // at different absolute paths. The two copies share one compiled BODY — + // the banner is the only difference — so what actually must match is the + // VERDICT: same exit code, and (json mode) the same structured envelope, or + // (plain-text mode) the same unqualified error header line with no path or + // line number in it. + test('the built copy and the scripts copy produce identical verdicts for every throw class', () => { + const cases = [ + { jsonMode: true, throwExpr: `new TypeError('same boom')`, compare: 'json' }, + { jsonMode: false, throwExpr: `new TypeError('same boom')`, compare: 'firstLine' }, + { jsonMode: true, throwExpr: `new ExitError(3, 'same usage')`, compare: 'exact' }, + ]; + for (const c of cases) { + const fromScripts = spawnScriptsRun(c); + const fromBuilt = spawnBuiltRun(c); + assert.strictEqual( + fromScripts.status, fromBuilt.status, + `exit status must match for ${c.throwExpr} (json=${c.jsonMode})`, + ); + const label = `${c.throwExpr} (json=${c.jsonMode})`; + if (c.compare === 'json') { + // Structured output: parse both and compare the resulting objects. + assert.deepStrictEqual( + parseEnvelope(fromScripts), parseEnvelope(fromBuilt), + `parsed envelopes must match for ${label}`, + ); + } else if (c.compare === 'firstLine') { + // Plain-text stack trace: only the header line (e.g. "TypeError: same + // boom") is path/line-number-free and therefore comparable; the frame + // lines below it are expected to diverge per the banner offset above. + for (const r of [fromScripts, fromBuilt]) { + assert.throws(() => JSON.parse(r.stderr.trim()), `stderr for ${label} must NOT be JSON`); + } + const firstLine = (s) => s.trim().split('\n')[0]; + assert.strictEqual( + firstLine(fromScripts.stderr), firstLine(fromBuilt.stderr), + `stderr first line must match for ${label}`, + ); + } else { + // ExitError: plain prose with no stack trace, so it is byte-identical. + assert.strictEqual( + fromScripts.stderr.trim(), fromBuilt.stderr.trim(), + `stderr must match for ${label}`, + ); + } + } + }); + + // ── Matrix rows 13-15: ONE json-error-mode cell, not two ───────────────── + // This is the hazard the fix INTRODUCES and must therefore be tested rather + // than reasoned about: after generation there are two module instances of + // the same artifact, and a module-level `let` would give them two flags. + test('the mode set through io is visible through the scripts copy', () => { + assert.deepStrictEqual( + readJsonFromChild([ + `const io = require(${JSON.stringify(IO_PATH)});`, + `const cliExit = require(${JSON.stringify(SCRIPTS_CLI_EXIT_PATH)});`, + `io.setJsonErrorMode(true);`, + `process.stdout.write(JSON.stringify({ viaCliExit: cliExit.getJsonErrorMode() }));`, + ]), + { viaCliExit: true }, + ); + }); + + test('the mode set through the scripts copy is visible through io', () => { + assert.deepStrictEqual( + readJsonFromChild([ + `const io = require(${JSON.stringify(IO_PATH)});`, + `const cliExit = require(${JSON.stringify(SCRIPTS_CLI_EXIT_PATH)});`, + `cliExit.setJsonErrorMode(true);`, + `process.stdout.write(JSON.stringify({ viaIo: io.getJsonErrorMode() }));`, + ]), + { viaIo: true }, + ); + }); + + test('both copies of the exit module share one json-error-mode cell', () => { + assert.deepStrictEqual( + readJsonFromChild([ + `const io = require(${JSON.stringify(IO_PATH)});`, + `const built = require(${JSON.stringify(BUILT_CLI_EXIT_PATH)});`, + `const scripts = require(${JSON.stringify(SCRIPTS_CLI_EXIT_PATH)});`, + // Two distinct module instances of the same artifact. + `if (built === scripts) throw new Error('expected two distinct module instances');`, + `io.setJsonErrorMode(true);`, + `process.stdout.write(JSON.stringify({`, + ` built: built.getJsonErrorMode(),`, + ` scripts: scripts.getJsonErrorMode(),`, + ` io: io.getJsonErrorMode(),`, + `}));`, + ]), + { built: true, scripts: true, io: true }, + 'all three views must read one cell — two module-level flags would diverge here', + ); + }); + + // ── Matrix rows 16-17: coercion and default, preserved exactly ─────────── + test('setJsonErrorMode keeps its truthiness coercion', () => { + const seen = readJsonFromChild([ + `const c = require(${JSON.stringify(SCRIPTS_CLI_EXIT_PATH)});`, + `const seen = [];`, + `for (const v of [0, '', 'false', null, undefined, 1, 'x']) {`, + ` c.setJsonErrorMode(v); seen.push(c.getJsonErrorMode());`, + `}`, + `process.stdout.write(JSON.stringify(seen));`, + ]); + // `!!v` — note 'false' is a NON-EMPTY string and is therefore true. + assert.deepStrictEqual(seen, [false, false, true, false, false, true, true]); + }); + + test('json-error mode defaults to false when never set', () => { + assert.deepStrictEqual( + readJsonFromChild([ + `const c = require(${JSON.stringify(SCRIPTS_CLI_EXIT_PATH)});`, + `const v = c.getJsonErrorMode();`, + `process.stdout.write(JSON.stringify({ v, type: typeof v }));`, + ]), + { v: false, type: 'boolean' }, + 'an unset cell must read as boolean false, never undefined', + ); + }); + + // ── Matrix rows 18-21: io's export surface must not move (Hyrum) ───────── + test('io still exports both json-error-mode accessors and an unchanged ERROR_REASON', () => { + const seen = readJsonFromChild([ + `const io = require(${JSON.stringify(IO_PATH)});`, + `process.stdout.write(JSON.stringify({`, + ` setter: typeof io.setJsonErrorMode,`, + ` getter: typeof io.getJsonErrorMode,`, + ` failFast: io.ERROR_REASON.SDK_FAIL_FAST,`, + ` frozen: Object.isFrozen(io.ERROR_REASON),`, + ` reasonCount: Object.keys(io.ERROR_REASON).length,`, + ` keys: Object.keys(io.ERROR_REASON).sort(),`, + `}));`, + ]); + assert.strictEqual(seen.setter, 'function'); + assert.strictEqual(seen.getter, 'function'); + assert.strictEqual(seen.failFast, 'sdk_fail_fast', 'the literal must survive moving to cli-exit'); + assert.strictEqual(seen.frozen, true); + assert.strictEqual(seen.reasonCount, 23, 'ERROR_REASON must keep all 23 members'); + assert.ok( + seen.keys.includes('SDK_FAIL_FAST'), + `ERROR_REASON must still include SDK_FAIL_FAST, got: ${JSON.stringify(seen.keys)}`, + ); + }); + + // ── Matrix rows 22-23: the unbuilt-clone constraint ────────────────────── + test('the scripts copy loads with no gsd-core tree in scope at all', (t) => { + // The generated file is COMMITTED and 64+ scripts/ consumers require it, + // including scripts/check-env.cjs which runs before any build. It must + // therefore not reach into gsd-core/bin/lib/, which is gitignored tsc + // output absent on a fresh clone. + // + // Proven by copying the file into an isolated temp directory that has no + // gsd-core sibling and no node_modules — a require of the built tree is + // MODULE_NOT_FOUND there. Deliberately NOT done by renaming the real + // gsd-core/bin/lib: test files run in parallel, so mutating a shared + // production directory would break every sibling suite mid-run. + // + // This is the sole guard of the "depends on node: builtins only" + // constraint: it proves the property by real module resolution in an + // isolated directory, rather than by inspecting require() specifiers. + const dir = createTempDir('gsd-3904-standalone-'); + t.after(() => cleanup(dir)); + const copied = path.join(dir, 'cli-exit.cjs'); + fs.copyFileSync(SCRIPTS_CLI_EXIT_PATH, copied); + + const r = toLegacyResult(runNode(['-e', [ + `const c = require(${JSON.stringify(copied)});`, + `c.setJsonErrorMode(true);`, + `c.runMain(() => { throw new TypeError('still works'); });`, + `setImmediate(() => {});`, + ].join('\n')], { cwd: dir, timeoutMs: PROBE_TIMEOUT_MS })); + + assert.ok( + !r.stderr.includes('MODULE_NOT_FOUND'), + `the scripts copy must not require anything outside node: builtins; got: ${r.stderr.slice(0, 400)}`, + ); + assert.strictEqual(r.status, 1, `expected exit 1; stderr: ${r.stderr}`); + assert.strictEqual(JSON.parse(r.stderr.trim()).reason, 'sdk_fail_fast'); + }); + + test('the build sentinel is still emitted', () => { + // gsd-core/bin/ensure-runtime-build.cjs keys isBuilt() on this exact filename. + assert.ok( + fs.statSync(BUILT_CLI_EXIT_PATH).isFile(), + 'gsd-core/bin/lib/cli-exit.cjs must remain tsc output — it is the build sentinel', + ); + }); + }); });