Skip to content

Commit be75493

Browse files
os-litantclaude
andauthored
fix(cli): refuse os lint --generator without --eval instead of silently ignoring it (#16115)
* fix(cli): refuse `os lint --generator` without `--eval` instead of ignoring it The flag's description ends "Requires --eval." and nothing checked it. `flags.generator` is read at exactly three sites, all inside `runEval`, which `run()` reaches only when `flags.eval` is set — so outside eval mode the flag reached no code at all. Driven before the change, from a lint-clean project, with a generator that writes a marker at top-level evaluation: os lint --generator ./gen-marker.mjs exit 0 · All checks passed · marker ABSENT os lint --generator ./does-not-exist.mjs exit 0 · All checks passed os lint --json --generator ./does-not-exist.mjs exit 0 · {"passed":true,…} Accepted, never loaded, and not named once on either face. The operator got a successful-looking run whose generator was never called. The repair refuses rather than deleting the sentence: nothing outside eval mode reads the flag, so dropping the claim would document a no-op instead of removing one. The refusal keeps the envelope this command already answers with — the human message on `error`, exit 1, both faces — and mints no ADR-0112 code, because it has no producer error to pass one through. Not oclif's `dependsOn: ['eval']`, measured rather than assumed: it refuses in the parser with exit 2, a stack trace on stderr, and empty stdout under `--json`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * chore(changeset): declare the `os lint --generator` accept-set narrowing `minor` with a `**BREAKING**` banner, on the `os create` project-name precedent (#15893): while the workspace versions in lockstep the bump level carries no breaking-ness, so the banner and the ADR-0087 disposition are the carriers. A scripted invocation that passed `--generator` outside eval mode exited 0 and now exits 1. Disposition `not-required (no-migration-prescription)`: the change narrows what a CLI flag combination accepts at invocation time, touching no metadata surface, stored row or spec declaration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * docs(cli): record the blast-radius reason for the guard, not a false one Contract review drove `dependsOn` rather than reading the report, and the recorded reason was partly false. The choice is unchanged; only the reason is. Two corrections, both comment-only: * `dependsOn` CAN be brought inside this command's envelope — a `catch()` override on the parse answers exit 1 with `{error}` on the `--json` face and an empty stderr. "The framework spelling cannot be enveloped" was the sentence the guard leaned on, and it is not true. The real objection is scope: that override re-shapes EVERY parse error on this command — every unknown flag and bad value moving from exit 2/stderr to exit 1/stdout, carrying oclif's prose and its `--help` hint inside the JSON `error` string — where the guard moves one invocation class and leaves the rest. * The stack trace beside oclif's refusal is a DEV-ENTRY artefact of `settings.debug` on `bin/run-dev.js`; the shipped `bin/run.js` prints the pretty message with no stack. The old comment generalised the dev entry. The test header carried the same false claim and said these pins "fail that implementation" — a `catch()` override passes them. It now says so, and says a green here is not a verdict on the choice. No assertion is touched: the `expect(` count is 18 before and after, and the diff is comment lines only. Behaviour, pins and changeset are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 81b426f commit be75493

3 files changed

Lines changed: 304 additions & 0 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/cli": minor
3+
---
4+
5+
**BREAKING** `os lint --generator` now refuses to run without `--eval`, instead of accepting the flag and ignoring it.
6+
7+
The flag's own description has always ended "Requires --eval.", and nothing checked it. `--generator` is read only by eval mode, so outside `--eval` the flag reached no code at all: `os lint --generator ./gen.mjs` linted the current project, exited 0 with "All checks passed", never loaded the module, and named the flag nowhere on either the human face or `--json`. A path that did not exist was accepted just as readily. Someone who meant to score a live generator got a successful-looking run whose generator was never called, with nothing said.
8+
9+
The refusal is this command's own, not the argument parser's, so it keeps the shape the command's other failures already have: the reason on `error`, exit 1, and on `--json` a single JSON document with stdout still reserved for the machine. No error code is invented for it.
10+
11+
A scripted invocation that passed `--generator` outside eval mode now exits 1 with the reason, where it previously exited 0 having silently skipped the generator. Eval mode itself is untouched: `--eval --generator` still loads the module and scores live output, and `--eval` alone still scores the bundled corpus offline.
12+
13+
<!-- adr-0087: not-required (no-migration-prescription) The change narrows what a CLI flag combination accepts at invocation time. No metadata surface, stored row or spec declaration is touched, so `objectstack migrate meta` has nothing to carry and the ledger has nothing to record. -->

packages/cli/src/commands/lint.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -514,6 +514,75 @@ export default class Lint extends Command {
514514
const configPath = args.config;
515515
const timer = createTimer();
516516

517+
// ── `--generator` means nothing without `--eval` — refuse, don't ignore ──
518+
//
519+
// [#15550] The flag's own description ends "Requires --eval." and nothing
520+
// checked it. Driven on this entry before this change, from a lint-clean
521+
// project, with a generator that writes a marker file at TOP-LEVEL
522+
// evaluation so "was it loaded?" is answered by the filesystem rather than
523+
// by reading the control flow:
524+
//
525+
// os lint --generator ./gen-marker.mjs exit 0 · All checks passed · marker ABSENT
526+
// os lint --generator ./does-not-exist.mjs exit 0 · All checks passed
527+
// os lint --json --generator ./does-not-exist.mjs exit 0 · {"passed":true,…}
528+
//
529+
// ⇒ accepted by the parser, never loaded, and not named once on either
530+
// face — a path that does not exist passes too. `flags.generator` is read
531+
// at exactly three sites, all inside `runEval`, which `run()` reaches only
532+
// when `flags.eval` is set, so outside eval mode the flag reaches no code
533+
// at all.
534+
//
535+
// That is Prime Directive #10's declared-≠-enforced shape landing on the
536+
// person least able to diagnose it: a successful-looking run whose
537+
// generator was never called, saying nothing. The direction is #12 — refuse
538+
// the off-contract invocation loudly at the boundary. The alternative
539+
// repair, deleting "Requires --eval." from the description, was rejected
540+
// for the reason that sentence exists: nothing outside eval mode reads this
541+
// flag, so dropping the claim documents a no-op flag instead of removing
542+
// one, and blesses the silent acceptance rather than ending it.
543+
//
544+
// ⛔ NOT oclif's `dependsOn: ['eval']` — and the reason is BLAST RADIUS,
545+
// not an inability to answer inside this command's envelope.
546+
//
547+
// Bare `dependsOn` refuses in the PARSER, before the command runs, so its
548+
// refusal is oclif's: exit 2, and under `--json` an EMPTY STDOUT. (The
549+
// stack trace that accompanies it on `bin/run-dev.js` is a DEV-ENTRY
550+
// artefact of `settings.debug`; the shipped `bin/run.js` prints oclif's
551+
// pretty message with no stack. Don't generalise the dev entry's output.)
552+
//
553+
// ⚠️ That much CAN be brought inside the envelope: a `catch()` override on
554+
// the parse was measured answering exit 1 with `{error}` on the `--json`
555+
// face and an empty stderr. So "the framework spelling cannot be
556+
// enveloped" is FALSE, and ⛔ nobody should re-derive this choice from it.
557+
//
558+
// The real objection is scope. That override re-shapes EVERY parse error on
559+
// this command, not the one precondition this card is about: every unknown
560+
// flag and every bad value would move from exit 2 / stderr to exit 1 /
561+
// stdout, and would carry oclif's own prose plus its `--help` hint inside
562+
// the JSON `error` string — a wide, uncommissioned change to the very
563+
// `--json` envelope #15549/#16044 had just repaired one exit over. A guard
564+
// here moves ONE invocation class and leaves every other parse error
565+
// exactly as it was, while keeping the envelope this command already
566+
// answers with: the human message on `error`, exit 1, both faces.
567+
//
568+
// ⛔ Nor the raw-argv guard `os migrate meta` uses for its stored-only
569+
// flags. That one exists because oclif reads a `default: false` boolean and
570+
// an `env`-backed string as "provided"; `--generator` has neither a default
571+
// nor an `env`, so `!== undefined` already means the operator typed it.
572+
//
573+
// ⛔ Nothing is minted: no `code` is attached. This refusal has no producer
574+
// error to pass one through, and ADR-0112's ledger is the authority on who
575+
// may mint one — the same restraint the generator-load exit below keeps.
576+
if (flags.generator !== undefined && !flags.eval) {
577+
const message =
578+
'--generator only applies to `os lint --eval` (the metadata-generation eval). '
579+
+ 'Without --eval this command lints the current project and never loads the generator. '
580+
+ 'Re-run as `os lint --eval --generator <module>`.';
581+
if (flags.json) await emitJson({ error: message }, 0, { compact: true });
582+
else printError(message);
583+
process.exit(1);
584+
}
585+
517586
// ── Eval mode — score generated metadata against the convention rubric ──
518587
// Short-circuits the project lint: this evaluates a generation corpus, not
519588
// the current config.
Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `os lint --generator` claimed a precondition nothing checked.
5+
*
6+
* ## The measured before-shape
7+
*
8+
* The flag's description ends "Requires --eval." and `run()` enforced nothing.
9+
* Driven on this entry before the fix, from a lint-clean project, with a
10+
* generator that writes a marker file at TOP-LEVEL evaluation so "was it
11+
* loaded?" is answered by the filesystem instead of by reading control flow:
12+
*
13+
* os lint --generator ./gen-marker.mjs exit 0 · All checks passed · marker ABSENT
14+
* os lint --generator ./does-not-exist.mjs exit 0 · All checks passed
15+
* os lint --json --generator ./does-not-exist.mjs exit 0 · {"passed":true,…}
16+
*
17+
* ⇒ accepted by the parser, never loaded, and not named once on either face —
18+
* a path that does not exist passed too. The operator got a successful-looking
19+
* run whose generator was never called, with nothing said. Silent, not loud.
20+
*
21+
* ## What is pinned, and why the negatives are not decoration
22+
*
23+
* The fix REFUSES rather than amending the description, so the accept set
24+
* narrows and the pins have to hold both directions:
25+
*
26+
* - positive — the refusal happens, on both faces, with the same envelope
27+
* this command already answers with (#16044): the human message on
28+
* `error`, exit 1, `--json` stdout still one JSON document.
29+
* - ⛔ negative — `--eval --generator` still LOADS the generator (marker
30+
* present, live mode). A "fix" that refused too broadly, or that refused
31+
* after loading the module, fails these directly. So does one that moved
32+
* plain `os lint` or offline `--eval`.
33+
*
34+
* ⛔ `nothing is minted` pins the ADR-0112 restraint from this side: the
35+
* refusal has no producer error to pass a `code` through, so the payload's key
36+
* set is exactly `error`. A later edit that invents a code for it goes red here
37+
* rather than handing consumers a vocabulary no ledger declares.
38+
*
39+
* ⛔ The refusal is deliberately NOT oclif's `dependsOn: ['eval']` — and these
40+
* pins are NOT the argument for that. Bare `dependsOn` does fail them (exit 2,
41+
* empty stdout under `--json`, measured), but a `catch()` override brings it
42+
* inside the envelope and PASSES them. So a green here is not a verdict on the
43+
* choice, and ⛔ must not be read as one. The reason the guard lives in the
44+
* command is BLAST RADIUS — that override re-shapes every parse error on this
45+
* command rather than this one precondition — and it is recorded where the
46+
* decision is, at the guard in `src/commands/lint.ts`.
47+
*
48+
* ## Why no `dist/` sits on the measured path
49+
*
50+
* These run the CLI through `bin/run-dev.js`, the SOURCE entry — same CLI, run
51+
* from `src/` through tsx — so `commands/lint.ts` is loaded from source by the
52+
* child and this change is measured without a rebuild.
53+
*/
54+
55+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
56+
import { execFile } from 'node:child_process';
57+
import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
58+
import { tmpdir } from 'node:os';
59+
import { join, resolve } from 'node:path';
60+
import { fileURLToPath } from 'node:url';
61+
import { childEnv } from './helpers/serve-process.js';
62+
63+
const HERE = resolve(fileURLToPath(import.meta.url), '..');
64+
const CLI = resolve(HERE, '../bin/run-dev.js');
65+
const TSX = resolve(HERE, '../../../node_modules/.bin/tsx');
66+
67+
interface Run {
68+
code: number;
69+
stdout: string;
70+
stderr: string;
71+
}
72+
73+
let dir: string;
74+
75+
/** A project this command lints CLEAN, so any non-zero exit is the refusal. */
76+
const CONFIG = `export default {
77+
name: 'refusal_probe',
78+
objects: [
79+
{
80+
name: 'probe_item',
81+
label: 'Probe Item',
82+
sharingModel: 'private',
83+
fields: { name: { type: 'text', label: 'Name' } },
84+
},
85+
],
86+
};
87+
`;
88+
89+
/**
90+
* The marker path the generator writes at import. Absent ⇒ the module was
91+
* never evaluated, which is the fact "was the generator loaded?" needs.
92+
*/
93+
const MARKER = 'GENERATOR_WAS_LOADED.marker';
94+
95+
const GENERATOR = `import { writeFileSync } from 'node:fs';
96+
writeFileSync(new URL('./${MARKER}', import.meta.url), 'loaded\\n');
97+
export default function generate() {
98+
return { name: 'from_generator', objects: [] };
99+
}
100+
`;
101+
102+
function markerPresent(): boolean {
103+
return existsSync(join(dir, MARKER));
104+
}
105+
106+
function clearMarker(): void {
107+
rmSync(join(dir, MARKER), { force: true });
108+
}
109+
110+
function runLint(args: string[]): Promise<Run> {
111+
return new Promise((resolvePromise) => {
112+
execFile(
113+
TSX,
114+
[CLI, 'lint', ...args],
115+
{ cwd: dir, maxBuffer: 16 * 1024 * 1024, env: childEnv({ NO_COLOR: '1' }) },
116+
(err, stdout, stderr) => {
117+
resolvePromise({
118+
code: err
119+
? typeof (err as { code?: unknown }).code === 'number'
120+
? (err as unknown as { code: number }).code
121+
: 1
122+
: 0,
123+
stdout: String(stdout),
124+
stderr: String(stderr),
125+
});
126+
},
127+
);
128+
});
129+
}
130+
131+
/** stdout as ONE JSON document, or a failure that quotes what was there instead. */
132+
function payloadOf(run: Run, label: string): Record<string, unknown> {
133+
try {
134+
return JSON.parse(run.stdout) as Record<string, unknown>;
135+
} catch {
136+
throw new Error(
137+
`${label}: stdout was not one JSON document (exit ${run.code}, ${run.stdout.length} stdout bytes)\n` +
138+
`stdout: ${JSON.stringify(run.stdout)}\nstderr: ${JSON.stringify(run.stderr)}`,
139+
);
140+
}
141+
}
142+
143+
beforeAll(() => {
144+
dir = mkdtempSync(join(tmpdir(), 'os-lint-generator-requires-eval-'));
145+
writeFileSync(join(dir, 'objectstack.config.mjs'), CONFIG, 'utf8');
146+
writeFileSync(join(dir, 'gen-marker.mjs'), GENERATOR, 'utf8');
147+
});
148+
149+
afterAll(() => {
150+
rmSync(dir, { recursive: true, force: true });
151+
});
152+
153+
describe('os lint --generator without --eval is refused', () => {
154+
it('refuses on the human face, naming the flag it requires', async () => {
155+
clearMarker();
156+
const run = await runLint(['--generator', './gen-marker.mjs']);
157+
158+
expect(run.code).toBe(1);
159+
expect(run.stdout).toContain('--generator');
160+
expect(run.stdout).toContain('--eval');
161+
// ⛔ The sharpest pin: the refusal happens INSTEAD of the run, not after
162+
// loading the module. Before the fix this marker was absent for the
163+
// opposite reason — nothing read the flag at all — so it is asserted
164+
// together with the exit code, which was 0 then.
165+
expect(markerPresent()).toBe(false);
166+
}, 120_000);
167+
168+
it('the --json face stays a machine face — one JSON document, nothing on stderr', async () => {
169+
clearMarker();
170+
const run = await runLint(['--json', '--generator', './gen-marker.mjs']);
171+
const payload = payloadOf(run, 'json refusal');
172+
173+
expect(run.code).toBe(1);
174+
expect(String(payload.error)).toContain('--eval');
175+
expect(run.stderr).toBe('');
176+
expect(markerPresent()).toBe(false);
177+
}, 120_000);
178+
179+
it('nothing is minted — the payload key set is exactly `error`', async () => {
180+
// ADR-0112: this refusal has no producer error to pass a code through, and
181+
// the ledger is the authority on who may mint one.
182+
const run = await runLint(['--json', '--generator', './gen-marker.mjs']);
183+
const payload = payloadOf(run, 'key set');
184+
185+
expect(Object.keys(payload)).toEqual(['error']);
186+
}, 120_000);
187+
188+
it('is judged on the flag being TYPED, not on the path resolving', async () => {
189+
// Before the fix this exited 0 with "All checks passed" — a generator path
190+
// that does not exist was accepted as readily as one that does.
191+
const run = await runLint(['--generator', './does-not-exist.mjs']);
192+
193+
expect(run.code).toBe(1);
194+
expect(run.stdout).toContain('--eval');
195+
// The refusal is this command's, not esbuild's: the module is never reached.
196+
expect(run.stdout).not.toContain('Failed to load generator');
197+
}, 120_000);
198+
});
199+
200+
describe('os lint — what the refusal must NOT move', () => {
201+
it('`--eval --generator` still loads the generator and runs live', async () => {
202+
clearMarker();
203+
const run = await runLint(['--eval', '--generator', './gen-marker.mjs']);
204+
205+
expect(markerPresent()).toBe(true);
206+
expect(run.stdout).toContain('Mode: live');
207+
}, 120_000);
208+
209+
it('offline `--eval` with no generator is untouched', async () => {
210+
const run = await runLint(['--eval']);
211+
212+
expect(run.code).toBe(0);
213+
expect(run.stdout).toContain('Mode: offline');
214+
}, 120_000);
215+
216+
it('a plain project lint is untouched', async () => {
217+
const run = await runLint([]);
218+
219+
expect(run.code).toBe(0);
220+
expect(run.stdout).toContain('All checks passed');
221+
}, 120_000);
222+
});

0 commit comments

Comments
 (0)