diff --git a/.changeset/cli-lint-strict-warnings-fail.md b/.changeset/cli-lint-strict-warnings-fail.md new file mode 100644 index 0000000000..62a098d4ae --- /dev/null +++ b/.changeset/cli-lint-strict-warnings-fail.md @@ -0,0 +1,25 @@ +--- +"@objectstack/cli": minor +--- + +`os lint --strict` makes warning-severity findings fail the run, so an app can rely on the platform's warning-level rules as its gate instead of re-implementing them locally (#15935) + +Only an `error` failed `os lint` before. `packages/lint` ships ≈250 authoring rules, 119 of them at `warning`, and a run with any number of warnings and no errors exited 0 — so an app that wanted one of those rules to gate its CI had to re-implement it locally at error level, or bolt a script onto the JSON output to promote a family by hand. + +New public flag: **`os lint --strict`**. With it, a run with one or more `warning`-severity findings exits 1 exactly as an `error` does, and the console says why, naming the count and the flag: + +``` +✗ 1 warning(s) fail this run under --strict (a warning is advisory without the flag) +``` + +`suggestion`s stay advisory under both. ⛔ The default is unchanged: without the flag the same stack still exits 0, and no existing `os lint` expectation moves. + +The `--json` face carries the verdict so a gate can read it without re-deriving it from the counts. Two keys, unconditionally present on every project-lint payload, flag or no flag: + +```json +{ "passed": false, "errors": 0, "warnings": 1, "suggestions": 0, "strict": true, "failing": 1 } +``` + +`strict` says whether the flag was in effect; `failing` is the count the exit code was read from — `errors`, or `errors + warnings` under `--strict`; and `passed` is `failing === 0`, the same statement the exit code makes — so `--strict --json` on a warning-only stack reads `passed: false` beside exit 1, never `passed: true` next to a failing exit. + +Not in this change: per-rule severity configuration, any change to a rule's severity, and `--eval` mode, which keeps its own pass bar (`--eval-min`). diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index 4b0e7b025c..661f5a78ec 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -1351,6 +1351,7 @@ data-model conventions, translation coverage, with a 0-100 quality score. os lint # Author-time rules + style / convention checks os lint --score # Append a 0-100 metadata quality score (letter-graded) os lint --fix # Show what would be fixed (dry-run) +os lint --strict # Warnings fail the run too (exit 1); suggestions stay advisory os lint --json # JSON output for CI ``` @@ -1362,6 +1363,18 @@ before #4409: `os lint` ran one gating rule neither other command ran and missed six that both of them ran, so it disagreed with the build in **both** directions. +**What fails the run.** By default only an `error`-severity finding fails +`os lint` (exit 1); warnings and suggestions are printed and the exit code stays +0. `--strict` makes a run with one or more `warning`-severity findings exit 1 +exactly as an error does, and says why — `N warning(s) fail this run under +--strict` — so an app can rely on the platform's warning-level rules as its gate +instead of re-implementing them locally; `suggestion`s stay advisory either way, +and the default is unchanged by the flag's existence. On the `--json` face the +verdict is readable without re-deriving it from the counts: `strict` (whether +the flag was in effect), `failing` (the count the exit code was read from — +`errors`, or `errors + warnings` under `--strict`) and `passed` (`failing` is +`0` — the same statement the exit code makes). + #### `os test` Runs Quality Protocol test scenarios (JSON-based BDD) against a running ObjectStack server. diff --git a/packages/cli/src/commands/lint.ts b/packages/cli/src/commands/lint.ts index 43ace89d8e..a0bb8e2046 100644 --- a/packages/cli/src/commands/lint.ts +++ b/packages/cli/src/commands/lint.ts @@ -478,6 +478,10 @@ export default class Lint extends Command { static override flags = { json: Flags.boolean({ description: 'Output as JSON' }), fix: Flags.boolean({ description: 'Show what would be fixed (dry-run)' }), + strict: Flags.boolean({ + description: + 'Fail the run (exit 1) on warning-severity findings too, exactly as an error does; suggestions stay advisory. Without it only errors fail', + }), score: Flags.boolean({ description: 'Print a 0–100 metadata-quality score (the lint rubric) for this project', }), @@ -612,17 +616,38 @@ export default class Lint extends Command { // Metadata-quality score (the lint rubric expressed as 0–100). const score = flags.score ? scoreMetadata(normalized) : null; + // ── Verdict ── + // Only an `error` fails a run by default. `--strict` (#15935) makes a + // `warning` fail it too — so an app can rely on the warning-level rules + // this registry ships as its gate instead of re-implementing them + // locally at error level — while a `suggestion` stays advisory under + // both. `failing` is the ONE count the exit code is read from, computed + // here, above the two faces, so `--json` and the console cannot disagree + // about it. ⛔ The default is deliberately unchanged: promoting warnings + // for every app is a separate decision, not this flag's. + const strict = flags.strict ?? false; + const errors = issues.filter((i) => i.severity === 'error'); + const warnings = issues.filter((i) => i.severity === 'warning'); + const suggestions = issues.filter((i) => i.severity === 'suggestion'); + const failing = errors.length + (strict ? warnings.length : 0); + // ── JSON output ── if (flags.json) { - const errors = issues.filter((i) => i.severity === 'error'); - const warnings = issues.filter((i) => i.severity === 'warning'); - const suggestions = issues.filter((i) => i.severity === 'suggestion'); await emitJson({ - passed: errors.length === 0, + passed: failing === 0, total: issues.length, errors: errors.length, warnings: warnings.length, suggestions: suggestions.length, + // [#15935] The verdict, readable without re-deriving it from the + // counts: `strict` says whether the flag was in effect, `failing` + // is the count the exit code was read from — `errors`, or + // `errors + warnings` under `--strict` — and `passed` is + // `failing === 0`, the same statement the exit code makes. Both + // keys are unconditionally present so a gate keying off them never + // has to distinguish "not strict" from "this build does not say". + strict, + failing, ...(hiddenPlatform > 0 ? { hiddenPlatform } : {}), ...(score ? { score: score.score, grade: score.grade } : {}), issues, @@ -635,7 +660,7 @@ export default class Lint extends Command { // distinguish "did not convert" from "this command does not tell me". conversions: conversionNotices, duration: timer.elapsed(), - }, errors.length > 0 ? 1 : 0); + }, failing > 0 ? 1 : 0); return; } @@ -659,11 +684,6 @@ export default class Lint extends Command { return; } - // Group by severity - const errors = issues.filter((i) => i.severity === 'error'); - const warnings = issues.filter((i) => i.severity === 'warning'); - const suggestions = issues.filter((i) => i.severity === 'suggestion'); - const printIssue = (issue: LintIssue) => { const color = issue.severity === 'error' ? chalk.red : @@ -714,9 +734,20 @@ export default class Lint extends Command { printInfo('Dry-run mode: no files were modified.'); } + // A run that fails ONLY because of `--strict` says so, naming the count + // and the flag: the summary line above reads identically with and + // without the flag, and exit 1 under a heading that says "Warnings" is + // otherwise a verdict with no stated reason. + if (strict && warnings.length > 0) { + console.log(''); + printError( + `${warnings.length} warning(s) fail this run under --strict (a warning is advisory without the flag)`, + ); + } + console.log(''); - if (errors.length > 0) process.exit(1); + if (failing > 0) process.exit(1); } catch (error: any) { if (isExitSignal(error)) throw error; diff --git a/packages/cli/test/lint-strict-warnings.e2e.test.ts b/packages/cli/test/lint-strict-warnings.e2e.test.ts new file mode 100644 index 0000000000..60ff03b4c3 --- /dev/null +++ b/packages/cli/test/lint-strict-warnings.e2e.test.ts @@ -0,0 +1,295 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #15935 — `os lint --strict`: a warning-severity finding fails the run. + * + * ## The contract, and why it is pinned as a PAIR + * + * Only an `error` failed `os lint`. `packages/lint` ships ≈250 authoring rules, + * 119 of them at `warning`, and a run with any number of warnings and no + * errors exited 0 — so an app that wanted one of those rules to gate its CI + * re-implemented it locally at error level. `--strict` promotes `warning` to + * failing; `suggestion` stays advisory; ⛔ the default is unchanged. + * + * Every positive assertion here has a negative twin on the SAME stack, and the + * twin is the whole contract: "exit 1 under `--strict`" is worthless on its own + * because a regression that makes the flag a no-op reads exactly like the + * default — exit 0 — and a regression that promotes warnings by default reads + * exactly like the flag. Only the pair, on one fixture, tells the two apart. + * + * ## ⭐ The fixture must REALLY warn, and the assertion must SAY SO first + * + * `--strict` over a stack with zero warnings exits 0 in every implementation, + * including a broken one — the exit-1 assertion would then pass over an empty + * set. So the warning count is read off the `--json` face of the run under + * test and asserted `> 0` (and `errors === 0`, or an error would carry the + * exit and hide a no-op flag) BEFORE either half of the pair is read. The + * count is TWO, not one, so `failing === warnings` distinguishes "the number + * of warnings" from "1 if any". + * + * What each fixture establishes: + * + * fixture | errors | warnings | suggestions | pins + * ----------+--------+----------+-------------+------------------------------ + * warns | 0 | 2 | 0 | the pair; the console reason + * suggests | 0 | 0 | 2 | suggestions stay advisory + * errs | 1 | 1 | 0 | errors still fail; `failing` sums + * clean | 0 | 0 | 0 | `--strict` on a clean stack is 0 + * + * Which rule produces each finding is incidental and deliberately NOT pinned — + * a rule's severity is `packages/lint`'s decision. Should one move, the + * precondition assertion goes red with a message naming the count, never + * silently green over an emptier set. + * + * ## The `--json` verdict + * + * Two keys, unconditionally present: `strict` (was the flag in effect) and + * `failing` (the count the exit code was read from — `errors`, or + * `errors + warnings` under `--strict`). `passed` is `failing === 0`, the same + * statement the exit code makes, so `--strict --json` on a warning-only stack + * reads `passed: false` beside exit 1 rather than `passed: true` next to a + * failing exit. The keys are asserted on the serialized BYTES as well as the + * parsed object: `JSON.stringify` drops an `undefined` value silently. + * + * ## Why no `dist/` sits on the measured path + * + * These run the CLI through `bin/run-dev.js`, the SOURCE entry point — same + * CLI, run from `src/` through tsx — so `lint.ts` is loaded from source by the + * child and an ablation of it is measured without a rebuild. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFile } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync, mkdirSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { childEnv } from './helpers/serve-process.js'; + +const HERE = resolve(fileURLToPath(import.meta.url), '..'); +const CLI = resolve(HERE, '../bin/run-dev.js'); +const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); + +/** The console sentence a strict-only failure must print — count, then flag. */ +const STRICT_REASON = /(\d+) warning\(s\) fail this run under --strict/; + +interface Run { + code: number; + stdout: string; + stderr: string; +} + +function runCli(args: string[], cwd: string): Promise { + return new Promise((resolvePromise) => { + execFile( + TSX, + [CLI, ...args], + { cwd, maxBuffer: 16 * 1024 * 1024, env: childEnv({ NO_COLOR: '1' }) }, + (err, stdout, stderr) => { + resolvePromise({ + code: err ? (typeof (err as { code?: unknown }).code === 'number' ? (err as unknown as { code: number }).code : 1) : 0, + stdout: String(stdout), + stderr: String(stderr), + }); + }, + ); + }); +} + +interface Verdict { + passed: boolean; + total: number; + errors: number; + warnings: number; + suggestions: number; + strict: boolean; + failing: number; +} + +function payloadOf(run: Run, label: string): Verdict & Record { + try { + return JSON.parse(run.stdout) as Verdict & Record; + } catch { + throw new Error(`${label}: stdout was not one JSON document (exit ${run.code})\n${run.stdout}\n${run.stderr}`); + } +} + +/** The two verdict keys must survive serialization, not just exist on the object. */ +function expectVerdictKeys(run: Run, payload: Record, label: string): void { + expect(Object.keys(payload), `${label}: the verdict keys must be published`).toEqual( + expect.arrayContaining(['passed', 'strict', 'failing']), + ); + expect(run.stdout, `${label}: \`strict\` must survive serialization`).toContain('"strict"'); + expect(run.stdout, `${label}: \`failing\` must survive serialization`).toContain('"failing"'); +} + +/** + * One shape, four knobs. `engines` absent is one warning (`protocol/missing- + * engines-range`); a master_detail without `required` is a second; omitting + * its `deleteBehavior` / `inlineEdit` is two suggestions; an unknown list-view + * column is an error. Each fixture below turns the knobs it needs and no other. + */ +function stack( + ns: string, + opts: { engines?: boolean; requireMaster?: boolean; adviseMaster?: boolean; unknownColumn?: boolean } = {}, +): string { + const { engines = true, requireMaster = true, adviseMaster = true, unknownColumn = false } = opts; + const enginesLine = engines ? `engines: { protocol: '>=1' },` : ''; + const masterAdvice = adviseMaster ? `deleteBehavior: 'cascade', inlineEdit: true,` : ''; + const masterRequired = requireMaster ? `required: true,` : ''; + const columns = unknownColumn ? `'title', 'nope_missing'` : `'title'`; + return ` +export default { + manifest: { id: 'com.example.${ns}', name: '${ns}', version: '1.0.0', type: 'app', namespace: '${ns}', ${enginesLine} }, + objects: [ + { + name: '${ns}_invoice', + label: 'Invoice', + sharingModel: 'private', + fields: { title: { type: 'text', label: 'Title' } }, + listViews: { all: { label: 'All', columns: [${columns}] } }, + }, + { + name: '${ns}_line', + label: 'Line', + sharingModel: 'private', + fields: { + title: { type: 'text', label: 'Title' }, + invoice: { type: 'master_detail', reference: '${ns}_invoice', label: 'Invoice', ${masterRequired} ${masterAdvice} }, + }, + }, + ], +}; +`; +} + +const dirs: Record = {}; +let root = ''; + +beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'os-lint-strict-')); + const make = (name: string, config: string): void => { + const dir = join(root, name); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, 'objectstack.config.ts'), config); + dirs[name] = dir; + }; + + // Two warnings, nothing else — the pair's fixture. + make('warns', stack('wrn', { engines: false, requireMaster: false })); + // Two suggestions, nothing else — the "stays advisory" control. + make('suggests', stack('sug', { adviseMaster: false })); + // One error AND one warning — errors keep failing; `failing` sums under the flag. + make('errs', stack('err', { engines: false, unknownColumn: true })); + // Nothing at all — `--strict` on a clean stack must not invent a failure. + make('clean', stack('cln')); +}); + +afterAll(() => { + if (root) rmSync(root, { recursive: true, force: true }); +}); + +describe('#15935 — `os lint --strict` fails the run on warning-severity findings', () => { + it('⭐ the fixture REALLY warns — ≥1 warning and 0 errors on the run under test', async () => { + // The precondition every other assertion on `warns` stands on. Without it, + // "exit 1 under --strict" passes over an empty set, in any implementation. + const payload = payloadOf(await runCli(['lint', '--json'], dirs.warns), 'warns'); + expect(payload.errors, 'an error would carry the exit and hide a no-op flag').toBe(0); + expect(payload.warnings, 'the pair is vacuous over zero warnings').toBeGreaterThan(0); + expect(payload.warnings, 'two, so `failing === warnings` is a count and not a boolean').toBe(2); + }, 120_000); + + it('without --strict the same stack exits 0 — the default is unchanged', async () => { + // The negative half of the pair. A regression that promoted warnings by + // default would pass the strict half below and fail here. + const run = await runCli(['lint', '--json'], dirs.warns); + const payload = payloadOf(run, 'warns/default'); + expect(run.code, 'warnings are advisory without the flag').toBe(0); + expect(payload.passed).toBe(true); + expectVerdictKeys(run, payload, 'warns/default'); + expect(payload.strict, 'the flag was not in effect, and the payload says so').toBe(false); + expect(payload.failing, 'nothing decided a failing exit').toBe(0); + }, 120_000); + + it('⭐ with --strict the same stack exits 1 — passed:false, strict:true, failing === warnings', async () => { + // The positive half. A regression that made `--strict` a no-op reads + // exactly like the default — exit 0, `passed: true` — and fails here. + const run = await runCli(['lint', '--strict', '--json'], dirs.warns); + const payload = payloadOf(run, 'warns/strict'); + expect(run.code, 'a warning fails the run under --strict, as an error does').toBe(1); + expect(payload.passed, '`passed` makes the same statement the exit code does').toBe(false); + expectVerdictKeys(run, payload, 'warns/strict'); + expect(payload.strict).toBe(true); + expect(payload.failing, 'the count the exit was read from — every warning, not "1 if any"').toBe(payload.warnings); + expect(payload.errors, 'no error was involved: the flag alone decided the exit').toBe(0); + }, 120_000); + + it('⭐ the console face says why — the count and the flag, and the count is the measured one', async () => { + const run = await runCli(['lint', '--strict'], dirs.warns); + expect(run.code).toBe(1); + const said = run.stdout.match(STRICT_REASON); + expect(said, `the exit must state its reason, naming the flag:\n${run.stdout}`).not.toBeNull(); + // The number in the sentence is the number the run found, not a literal. + const counted = payloadOf(await runCli(['lint', '--strict', '--json'], dirs.warns), 'warns/strict').warnings; + expect(Number(said![1]), 'the sentence names the run\'s own warning count').toBe(counted); + }, 180_000); + + it('the console face without the flag exits 0 and claims no strict failure', async () => { + // The console twin: a reason printed unconditionally would satisfy the + // test above and fail this one. + const run = await runCli(['lint'], dirs.warns); + expect(run.code).toBe(0); + expect(run.stdout, 'the warnings are still printed').toMatch(/2 warning\(s\)/); + expect(run.stdout, 'no strict verdict was made, so none may be claimed').not.toMatch(STRICT_REASON); + }, 120_000); + + it('suggestions stay advisory under --strict', async () => { + // The severity boundary: the flag promotes `warning` and nothing below it. + // A regression that promoted "anything but clean" reads exit 1 here. + const run = await runCli(['lint', '--strict', '--json'], dirs.suggests); + const payload = payloadOf(run, 'suggests/strict'); + expect(payload.suggestions, 'the control is vacuous over zero suggestions').toBeGreaterThan(0); + expect(payload.warnings, 'a warning here would be the flag firing for the right reason').toBe(0); + expect(payload.errors).toBe(0); + expect(run.code, 'a suggestion never fails the run, flag or no flag').toBe(0); + expect(payload.passed).toBe(true); + expect(payload.strict).toBe(true); + expect(payload.failing).toBe(0); + }, 120_000); + + it('an error still fails with and without the flag, and --strict counts the warnings on top', async () => { + const plain = await runCli(['lint', '--json'], dirs.errs); + const plainPayload = payloadOf(plain, 'errs/default'); + expect(plainPayload.errors, 'the control is vacuous over zero errors').toBeGreaterThan(0); + expect(plainPayload.warnings, 'and needs a warning to show the sum').toBeGreaterThan(0); + expect(plain.code, 'an error fails the run, as it always did').toBe(1); + expect(plainPayload.passed).toBe(false); + expect(plainPayload.strict).toBe(false); + expect(plainPayload.failing, 'without the flag only the errors decided the exit').toBe(plainPayload.errors); + + const strict = await runCli(['lint', '--strict', '--json'], dirs.errs); + const strictPayload = payloadOf(strict, 'errs/strict'); + expect(strict.code).toBe(1); + expect(strictPayload.passed).toBe(false); + expect(strictPayload.strict).toBe(true); + expect(strictPayload.failing, 'under the flag every error and every warning decided it').toBe( + strictPayload.errors + strictPayload.warnings, + ); + }, 180_000); + + it('a clean stack exits 0 under --strict, in both faces', async () => { + // Guards the other degenerate regression: a flag that fails unconditionally. + const json = await runCli(['lint', '--strict', '--json'], dirs.clean); + const payload = payloadOf(json, 'clean/strict'); + expect(payload.total, 'the control is vacuous unless the stack is really clean').toBe(0); + expect(json.code).toBe(0); + expect(payload.passed).toBe(true); + expect(payload.strict).toBe(true); + expect(payload.failing).toBe(0); + + const text = await runCli(['lint', '--strict'], dirs.clean); + expect(text.code).toBe(0); + expect(text.stdout).toContain('All checks passed'); + expect(text.stdout).not.toMatch(STRICT_REASON); + }, 180_000); +});