diff --git a/.changeset/score-metadata-lint-crash-visible.md b/.changeset/score-metadata-lint-crash-visible.md new file mode 100644 index 0000000000..1b2de23d85 --- /dev/null +++ b/.changeset/score-metadata-lint-crash-visible.md @@ -0,0 +1,17 @@ +--- +"@objectstack/cli": minor +--- + +`scoreMetadata` no longer scores a stack whose linter crashed as a perfect one. + +The metadata rubric is two halves: a schema parse and the lint sweep. When `lintConfig` threw, the scorer caught the throw and continued with `issues = []` — so the penalty was 0 and a stack half of whose rubric never ran came back as **100 / grade `A` / `valid: true`, every count zero, `issues: []`** — byte-for-byte the verdict a genuinely clean stack gets. "The linter found nothing" and "the linter never ran" collapsed into the better-looking one. + +The crash is reachable on a schema-valid stack: a localized `label` (`{ en: 'Todos', 'zh-CN': '待办' }`) on an app, or on a view's `list`, parses clean and makes the label-case rule throw a `TypeError`. That rule's crash is a separate defect, filed on its own; what changes here is that the scorer stops publishing a clean verdict it did not earn. + +A crashed lint run is now recorded in every carrier a consumer might read, because reading any one of them has to be enough: + +- **`lintError`** — a new optional string on `MetadataScore`, carrying the thrown message. Set only when the linter could not run; absent when it ran and reported errors, which is a lint verdict rather than a missing one. It reaches the CLI's published payload through `os lint --eval --json`, on `results[].score`. +- **A synthetic `error` issue** (`rule: 'rubric/lint-crashed'`, exported as `LINT_CRASHED_RULE`) — so `issues`, `counts.errors` and `valid` carry the failure too. This is what makes the eval harness fail the case: its `passed` reads `counts.errors`, and would never have seen a new field. It still fails at `--eval-min 0`, where the score alone stops discriminating. +- **`score: 0` / grade `F`** — the only channel `os lint --score --json` publishes, and the same refusal `unscorableScore()` already gives an eval case there was nothing to judge. + +The schema half is untouched and still reported: `schemaErrors` and `counts.schemaErrors` say exactly what the parse found, which was the defensible half of the original intent. diff --git a/packages/cli/src/lint/score.ts b/packages/cli/src/lint/score.ts index 8a42073c8a..eef1cc1bc4 100644 --- a/packages/cli/src/lint/score.ts +++ b/packages/cli/src/lint/score.ts @@ -27,6 +27,14 @@ export const SCORE_WEIGHTS = { suggestion: 1, } as const; +/** + * The `rule` id on the synthetic issue raised when the linter throws. + * + * Exported so a consumer can tell "the linter reported a problem" from "the + * linter never ran" by matching an id rather than prose. + */ +export const LINT_CRASHED_RULE = 'rubric/lint-crashed'; + export interface MetadataScore { /** 0–100 quality score (higher is better). */ score: number; @@ -44,6 +52,18 @@ export interface MetadataScore { schemaErrors: string[]; /** Lint issues (naming, labels, structure, data-model conventions). */ issues: LintIssue[]; + /** + * Set only when the lint half of the rubric could NOT run: `lintConfig` threw + * and this carries the thrown message. Absent on every run where the linter + * completed -- including one where it reported errors, which is a lint + * verdict, not a missing one. + * + * Optional on purpose. It is the machine-readable half of the refusal below, + * not its enforcement: a consumer that never reads it still cannot mistake a + * crashed run for a clean one, because the same event is carried by `issues`, + * `counts.errors`, `valid` and `score`. + */ + lintError?: string; } function gradeFor(score: number): MetadataScore['grade'] { @@ -72,12 +92,47 @@ export function scoreMetadata(stack: unknown): MetadataScore { : parsed.error.issues.map((i) => `${i.path.join('.') || '(root)'}: ${i.message}`); // 2) Lint (naming/labels/structure + data-model conventions). + // + // A linter crash must not mask the schema verdict — the original intent, and + // still right. What was not right is the number that came out of it: with + // `issues = []` the penalty was 0, so a stack whose linter threw scored + // 100 / A / `valid: true` with every count at zero — byte-for-byte the + // verdict a genuinely clean stack gets, on a rubric half of which never ran. + // "The linter found nothing" and "the linter never ran" are different facts. + // + // Reachable, and not exotically: a localized `label` (`{ en: …, 'zh-CN': … }`) + // on an app, or on a view's `list`, is schema-valid and makes the label-case + // rule throw. That crash is its own defect, filed separately; this function's + // job is to never publish a clean verdict it did not earn. + // + // So the failure is recorded in every carrier a consumer might read, because + // reading any ONE of them must be enough: + // · `lintError` — the fact itself, typed, for a machine consumer; + // · a synthetic `error` issue — so `issues`, `counts.errors` and `valid` + // carry it too, which is what makes the eval harness fail the case: its + // `passed` reads `counts.errors` and would never see a new field; + // · `score` 0 / grade `F` — the only channel `os lint --score --json` + // publishes, and the same refusal `unscorableScore()` (metadata-eval.ts) + // already gives a case there was nothing to judge, for the same reason: + // a clean number nothing earned is the worst possible output. + // The schema verdict survives all of it — `schemaErrors` and + // `counts.schemaErrors` still report exactly what the parse found. let issues: LintIssue[] = []; + let lintError: string | undefined; try { issues = lintConfig(normalized) as LintIssue[]; - } catch { - // A linter crash shouldn't mask the schema verdict — treat as no lint data. - issues = []; + } catch (err) { + lintError = err instanceof Error ? err.message : String(err); + issues = [ + { + severity: 'error', + rule: LINT_CRASHED_RULE, + message: + `The lint rubric did not run: ${lintError}. This verdict covers the schema ` + + `parse only — no lint verdict was produced, so it must not be read as a clean one.`, + path: '(lint)', + }, + ]; } const errors = bySeverity(issues, 'error'); @@ -90,7 +145,10 @@ export function scoreMetadata(stack: unknown): MetadataScore { warnings.length * SCORE_WEIGHTS.warning + suggestions.length * SCORE_WEIGHTS.suggestion; - const score = Math.max(0, Math.min(100, 100 - penalty)); + // A rubric that did not run has no score to report. 0 / `F` is not a penalty + // dressed up as a measurement — it is the refusal, and it is the shape the + // eval harness already uses for "there was nothing to judge". + const score = lintError !== undefined ? 0 : Math.max(0, Math.min(100, 100 - penalty)); return { score: Math.round(score), @@ -104,5 +162,6 @@ export function scoreMetadata(stack: unknown): MetadataScore { }, schemaErrors, issues, + ...(lintError !== undefined ? { lintError } : {}), }; } diff --git a/packages/cli/test/score-lint-crash.test.ts b/packages/cli/test/score-lint-crash.test.ts new file mode 100644 index 0000000000..559cbb3995 --- /dev/null +++ b/packages/cli/test/score-lint-crash.test.ts @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `scoreMetadata` must never publish a clean verdict for a rubric that did not + * run. + * + * ## Why the linter is mocked here rather than driven + * + * The crash IS reachable on a schema-valid stack — a localized `label` + * (`{ en: …, 'zh-CN': … }`) on an app, or on a view's `list`, parses clean and + * makes the label-case rule throw a `TypeError`. That is a defect in the rule, + * filed on its own; pinning it here would make this suite depend on a bug + * staying unfixed, and the day someone repairs the rule these assertions would + * go green for the wrong reason — or be deleted to make them pass. + * + * What this file pins is the SCORER's contract, which holds for any throw from + * any rule: a crash is recorded, never swallowed into `issues: []`. So the + * linter is replaced by one that throws on demand, and the control below runs + * the same harness with a linter that returns cleanly — a mock that always + * failed would satisfy every assertion here for no reason at all. + */ + +import { describe, expect, it, vi } from 'vitest'; + +const lint = vi.hoisted(() => ({ + /** When set, the stand-in `lintConfig` throws this instead of returning. */ + throws: null as unknown, +})); + +vi.mock('../src/commands/lint.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + lintConfig: () => { + if (lint.throws !== null) throw lint.throws; + return []; + }, + }; +}); + +const { scoreMetadata, LINT_CRASHED_RULE, SCORE_WEIGHTS } = await import('../src/lint/score.js'); +const { runMetadataEval } = await import('../src/lint/metadata-eval.js'); + +/** Schema-valid and, to the stand-in linter, clean. */ +const STACK = { + objects: [ + { + name: 'invoice', + label: 'Invoice', + sharingModel: 'private', + fields: { name: { type: 'text', label: 'Invoice Number', required: true } }, + }, + ], +}; + +/** Schema-INVALID (`namespace` fails its pattern), so the parse half has a verdict. */ +const SCHEMA_INVALID_STACK = { + manifest: { id: 'bad', namespace: 'X', version: '1.0.0', name: 'Bad', type: 'app' as const }, +}; + +function withLinterThrowing(thrown: unknown, fn: () => T): T { + lint.throws = thrown; + try { + return fn(); + } finally { + lint.throws = null; + } +} + +/** + * The async twin. ⚠️ Not a stylistic variant of the above: the sync helper + * restores `lint.throws` when `fn` RETURNS, which for an async `fn` is the + * moment it hands back a pending promise — before a single line of the work + * being measured has run. Awaiting inside the `try` is what keeps the stand-in + * throwing for the whole run. + */ +async function withLinterThrowingAsync(thrown: unknown, fn: () => Promise): Promise { + lint.throws = thrown; + try { + return await fn(); + } finally { + lint.throws = null; + } +} + +describe('scoreMetadata — when the linter crashes', () => { + it('CONTROL: the same harness with a linter that returns cleanly still scores 100 / A', () => { + const r = scoreMetadata(STACK); + expect(r.lintError).toBeUndefined(); + expect(r.score).toBe(100); + expect(r.grade).toBe('A'); + expect(r.valid).toBe(true); + expect(r.counts.errors).toBe(0); + expect(r.issues).toEqual([]); + }); + + it('refuses the verdict instead of scoring 100 / A / valid', () => { + const r = withLinterThrowing(new TypeError('boom'), () => scoreMetadata(STACK)); + + // The headline: nothing about this may read like a clean stack. + expect(r.score).toBe(0); + expect(r.grade).toBe('F'); + expect(r.valid).toBe(false); + }); + + it('records the crash in every carrier a consumer might read', () => { + const r = withLinterThrowing(new TypeError('boom'), () => scoreMetadata(STACK)); + + expect(r.lintError).toBe('boom'); + expect(r.counts.errors).toBe(1); + expect(r.issues).toHaveLength(1); + expect(r.issues[0]).toMatchObject({ severity: 'error', rule: LINT_CRASHED_RULE }); + // The message must say the rubric did not RUN — "no issues found" is the + // exact reading this whole change exists to prevent. + expect(r.issues[0].message).toContain('did not run'); + expect(r.issues[0].message).toContain('boom'); + }); + + it('keeps the schema verdict, which the crash must not mask', () => { + const r = withLinterThrowing(new TypeError('boom'), () => scoreMetadata(SCHEMA_INVALID_STACK)); + + expect(r.counts.schemaErrors).toBeGreaterThan(0); + expect(r.schemaErrors.some((m) => m.includes('namespace'))).toBe(true); + expect(r.lintError).toBe('boom'); + }); + + it('stringifies a non-Error throw rather than reporting "undefined"', () => { + const r = withLinterThrowing('plain string failure', () => scoreMetadata(STACK)); + + expect(r.lintError).toBe('plain string failure'); + expect(r.issues[0].message).toContain('plain string failure'); + }); + + it('is not a lint error in disguise: a real lint error scores by the rubric and sets no lintError', () => { + // One `error`-severity issue costs exactly its weight — the crash path is a + // different claim from "the linter found one error", and the two must not + // land on the same output. + const r = scoreMetadata({ + objects: [{ name: 'BadName', label: 'Bad', fields: { name: { type: 'text', label: 'Name' } } }], + }); + expect(r.lintError).toBeUndefined(); + expect(r.score).toBeGreaterThan(100 - SCORE_WEIGHTS.error * 2); + }); +}); + +describe('the eval harness reads the refusal without a new field', () => { + const CORPUS = [{ id: 'crashing_case', prompt: 'anything', fixture: STACK }]; + + it('CONTROL: the same case passes when the linter returns cleanly', async () => { + const report = await runMetadataEval(CORPUS, { minScore: 75 }); + expect(report.results[0].passed).toBe(true); + expect(report.ok).toBe(true); + }); + + it('fails the case whose linter crashed, and contributes 0 to the mean', async () => { + const report = await withLinterThrowingAsync(new TypeError('boom'), () => + runMetadataEval(CORPUS, { minScore: 75 }), + ); + + expect(report.results[0].passed).toBe(false); + expect(report.results[0].score.lintError).toBe('boom'); + expect(report.meanScore).toBe(0); + expect(report.ok).toBe(false); + }); + + it('still fails it when the score bar is lowered to 0 — the synthetic error is what holds', async () => { + // `passed` reads `score >= minScore && counts.errors === 0 && + // counts.schemaErrors === 0`. At `--eval-min 0` the score half stops + // discriminating, so the crash has to be an `error` in `counts` or the case + // passes again. This is why the refusal is not carried by the number alone. + const report = await withLinterThrowingAsync(new TypeError('boom'), () => + runMetadataEval(CORPUS, { minScore: 0 }), + ); + + expect(report.results[0].score.score).toBeGreaterThanOrEqual(0); + expect(report.results[0].score.counts.errors).toBe(1); + expect(report.results[0].passed).toBe(false); + expect(report.ok).toBe(false); + }); +});