diff --git a/.changeset/lint-label-case-localized-guard.md b/.changeset/lint-label-case-localized-guard.md new file mode 100644 index 0000000000..25fd2527f0 --- /dev/null +++ b/.changeset/lint-label-case-localized-guard.md @@ -0,0 +1,13 @@ +--- +"@objectstack/cli": minor +--- + +`os lint` no longer crashes on a localized label. + +`convention/label-case` indexed its argument (`label[0].toUpperCase()`) on a parameter annotated `string`, while every call site reaches it through `any`-typed config walking and the spec does not require a label to be a string: `I18nLabelSchema` is `z.union([z.string(), InlineLocaleMapSchema])`. On the map form `label[0]` is `undefined`, the rule threw a `TypeError`, and the throw escaped `lintConfig` into the command's catch-all — so an author who localized an app label or a list-view label could not lint the project at all. Every face exited 1 with `Cannot read properties of undefined (reading 'toUpperCase')`, naming no rule, no path and no remedy, on input `ObjectStackDefinitionSchema` parses clean. + +The rule now checks `typeof label === 'string'` first. Two of the four carriers it walks accept the inline locale map — `apps[].label` (`AppSchema`) and a view's `list` / `listViews.*` labels (`ListViewShapeSchema`); the other two are `z.string()` and reject the map at the schema door (`objects[].label`, `objects[].fields.*.label`). + +**Nothing about a plain string label moves.** Same warning, same message, same `fix`, same path, on all four carriers — that is pinned per carrier rather than asserted. + +**The rule deliberately says nothing about a localized label**, rather than resolving the map and case-checking one of its entries. Case is a property of a literal, and deciding which locale entry a case verdict is taken against is a product call, not a lint call. Widening the rule that way is a separate change. diff --git a/packages/cli/src/commands/lint.ts b/packages/cli/src/commands/lint.ts index d6c14bb15f..611e6613c3 100644 --- a/packages/cli/src/commands/lint.ts +++ b/packages/cli/src/commands/lint.ts @@ -98,7 +98,31 @@ function checkLabelExists(item: any, path: string, kind: string): LintIssue | nu return null; } -function checkLabelCase(label: string, path: string): LintIssue | null { +// A label is not required to be a string. `I18nLabelSchema` (spec +// `ui/i18n.zod`) is `z.union([z.string(), InlineLocaleMapSchema])`, and it is +// the label primitive the whole `ui/` tree imports — so of the four carriers +// this rule is called on, two accept the inline locale map: +// `views[].list.label` / `views[].listViews.*.label` (`ListViewShapeSchema`) +// and `apps[].label` (`AppSchema`). The other two are `z.string()` and reject +// the map at the schema door (`objects[].label`, `objects[].fields.*.label`). +// +// Every call site reaches this function through `any`-typed config walking, so +// the annotation below used to say `string` and be wrong: on a map, +// `label[0]` is `undefined` and `undefined.toUpperCase()` threw. The throw +// escaped `lintConfig` into the command's catch-all, so an author who +// localized an app or list-view label could not lint the project at all — +// every face exited 1 naming no rule, no path and no remedy, on input +// `ObjectStackDefinitionSchema` parses clean. +// +// ⛔ The guard deliberately says NOTHING about a localized label rather than +// resolving the map and case-checking an entry. Case is a property of a +// literal; picking WHICH locale entry a case verdict is taken against is a +// product decision (`resolveI18nLabel` exists, but which entry is +// authoritative for a lint verdict is not this rule's to answer). Widening +// the rule to localized labels is an extension, filed separately; this guard +// is the floor, and it leaves the string branch below byte-identical. +function checkLabelCase(label: unknown, path: string): LintIssue | null { + if (typeof label !== 'string') return null; if (label && label[0] !== label[0].toUpperCase()) { return { severity: 'warning', @@ -111,7 +135,11 @@ function checkLabelCase(label: string, path: string): LintIssue | null { return null; } -function getViewLabel(view: any, viewPath: string): { label?: string; path: string } { +// ⚠️ `label` is `unknown`, not `string`: it is read straight off `any`-typed +// config and `ListViewShapeSchema.label` is `I18nLabelSchema`, so the value +// can legitimately be an inline locale map. Annotating it `string` here is +// what let the map reach `checkLabelCase`'s indexing unchecked. +function getViewLabel(view: any, viewPath: string): { label?: unknown; path: string } { if (view?.list?.label) { return { label: view.list.label, path: `${viewPath}.list.label` }; } diff --git a/packages/cli/test/lint-label-case-localized.test.ts b/packages/cli/test/lint-label-case-localized.test.ts new file mode 100644 index 0000000000..08b547b126 --- /dev/null +++ b/packages/cli/test/lint-label-case-localized.test.ts @@ -0,0 +1,235 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `convention/label-case` must not crash `os lint` on a localized label. + * + * ## What this file pins, and how each half can fail + * + * The rule indexed its argument (`label[0].toUpperCase()`) on a parameter + * annotated `string` that every call site reaches through `any`-typed config + * walking. `I18nLabelSchema` is `z.union([z.string(), InlineLocaleMapSchema])`, + * so on the map form `label[0]` is `undefined` and the rule threw — out of + * `lintConfig`, into the command's catch-all, exit 1 on every face with a + * message naming no rule, no path and no remedy. + * + * Two independent properties, and neither one covers the other: + * + * 1. **NO MOVE** — the check on a plain string label is unchanged. This is + * the property that makes a guard the uncontroversial floor, so it is + * pinned per carrier rather than asserted in prose. Falsified by any guard + * that also swallows strings (a `typeof` typo, an early return placed + * above the string branch, or the guard inverted to + * `typeof label === 'string'`): every lowercase row below stops + * reporting. + * 2. **NO CRASH** — a localized label is walked without throwing, on every + * carrier whose schema accepts the map. Falsified by removing the guard: + * each of those rows throws a `TypeError` instead of returning issues. + * + * ## Why the carriers are enumerated rather than sampled + * + * The filed mutation sweep hit exactly two paths (`apps.0.label`, + * `views.0.list.label`) because the fixture it swept had exactly those two. + * The class is the set of call sites, not the set of sweep hits: + * `lintConfig` calls this rule from four places, and the schema decides which + * of them can carry a map — + * + * | call site | governing schema | map? | + * | `objects[].label` | `ObjectSchema.label` — `z.string()` | no | + * | `objects[].fields.*.label` | field base — `z.string()` | no | + * | `views[].list{,Views.*}.label` | `ListViewShapeSchema` — `I18nLabelSchema` | yes | + * | `apps[].label` | `AppSchema.label` — `I18nLabelSchema` | yes | + * + * — which is why `views[].listViews.*.label` is pinned below even though no + * sweep ever reached it: it is the same primitive behind a second path, and + * `getViewLabel` only falls through to it when `list.label` is absent. + * + * ## What the rule now SAYS about a localized label: nothing + * + * Deliberately. Case is a property of a literal; deciding which locale entry a + * case verdict is taken against is a product call this card does not make. So + * the localized rows assert the ABSENCE of a `convention/label-case` issue — + * if someone later widens the rule to resolve the map, these are the + * assertions that must be rewritten on purpose rather than silently satisfied. + */ + +import { describe, expect, it } from 'vitest'; +import { ObjectStackDefinitionSchema, normalizeStackInput } from '@objectstack/spec'; +import { lintConfig } from '../src/commands/lint'; +import { scoreMetadata } from '../src/lint/score'; + +const MANIFEST = { + id: 'todo', + namespace: 'todo', + version: '1.0.0', + name: 'Todo', + type: 'app' as const, +}; + +/** The card's own fixture value, plus the sweep's literal hit value. */ +const LOCALIZED = { en: 'Todos', 'zh-CN': '待办' }; +const EMPTY_MAP = {}; + +const caseIssues = (issues: { rule: string }[]) => + issues.filter((i) => i.rule === 'convention/label-case'); + +/** `objects[]` needs fields to avoid drowning the label rows in structure issues. */ +const objectWith = (label: unknown) => ({ + name: 'invoice', + label, + fields: { name: { type: 'text', label: 'Invoice Number' } }, +}); + +const objectWithFieldLabel = (label: unknown) => ({ + name: 'invoice', + label: 'Invoice', + fields: { name: { type: 'text', label } }, +}); + +const stackWithApp = (label: unknown) => ({ + manifest: MANIFEST, + apps: [{ name: 'todo_app', label }], +}); + +const stackWithListLabel = (label: unknown) => ({ + manifest: MANIFEST, + views: [{ name: 'invoice_views', object: 'invoice', list: { label, type: 'grid', columns: ['name'] } }], +}); + +const stackWithNamedListLabel = (label: unknown) => ({ + manifest: MANIFEST, + views: [{ + name: 'invoice_views', + object: 'invoice', + listViews: { all: { label, type: 'grid', columns: ['name'] } }, + }], +}); + +describe('convention/label-case — the plain-string check does not move', () => { + // Falsification for every row: a guard that swallows strings as well as + // maps makes the lowercase rows report nothing and this whole block red. + const rows: [string, unknown, string][] = [ + ['objects[].label', { objects: [objectWith('invoice')] }, 'objects[0].label'], + ['objects[].fields.*.label', { objects: [objectWithFieldLabel('invoice number')] }, 'objects[0].fields.name.label'], + ['views[].list.label', stackWithListLabel('accounts'), 'views[0].list.label'], + ['views[].listViews.*.label', stackWithNamedListLabel('all accounts'), 'views[0].listViews.all.label'], + ['apps[].label', stackWithApp('todos'), 'apps[0].label'], + ]; + + for (const [carrier, config, path] of rows) { + it(`still warns on a lowercase string at ${carrier}`, () => { + const issues = caseIssues(lintConfig(config as any)); + expect(issues).toHaveLength(1); + expect(issues[0]).toEqual({ + severity: 'warning', + rule: 'convention/label-case', + message: expect.stringContaining('should start with an uppercase letter'), + path, + fix: expect.any(String), + }); + }); + } + + it('carries the label verbatim in the message and the capitalized value in `fix`', () => { + // The message/fix wording is what an author reads, so it is pinned whole + // on one row rather than left to `stringContaining` everywhere. + const [issue] = caseIssues(lintConfig(stackWithApp('todos') as any)); + expect(issue).toMatchObject({ + message: 'Label "todos" should start with an uppercase letter', + fix: 'Todos', + }); + }); + + it('stays silent on an already-uppercase string', () => { + expect(caseIssues(lintConfig(stackWithApp('Todos') as any))).toEqual([]); + }); + + it('stays silent on a label whose first character has no case (unchanged)', () => { + // `'1st quarter'[0].toUpperCase()` === `'1'`, so the rule never fired here + // and must still not. A guard written as `label.length && ...` would keep + // this green; one written as "warn unless the first char is uppercase" + // would flip it. That is the distinction this row protects. + expect(caseIssues(lintConfig(stackWithApp('1st quarter') as any))).toEqual([]); + }); +}); + +describe('convention/label-case — a localized label is walked, not indexed', () => { + // Falsification for every row: drop the `typeof label !== 'string'` guard + // and each of these throws `TypeError: Cannot read properties of undefined + // (reading 'toUpperCase')` instead of returning. + const rows: [string, (label: unknown) => unknown][] = [ + ['apps[].label', stackWithApp], + ['views[].list.label', stackWithListLabel], + ['views[].listViews.*.label', stackWithNamedListLabel], + ]; + + for (const [carrier, build] of rows) { + it(`does not throw on an inline locale map at ${carrier}`, () => { + expect(() => lintConfig(build(LOCALIZED) as any)).not.toThrow(); + }); + + it(`does not throw on an EMPTY locale map at ${carrier}`, () => { + // `{}` is the value the filed sweep actually mutated in, and it is a + // valid `InlineLocaleMapSchema` (a `z.record` with no entries). + expect(() => lintConfig(build(EMPTY_MAP) as any)).not.toThrow(); + }); + + it(`reports no case verdict for the localized label at ${carrier}`, () => { + expect(caseIssues(lintConfig(build(LOCALIZED) as any))).toEqual([]); + }); + + it(`does not report the localized label as MISSING at ${carrier}`, () => { + // The other failure mode a careless guard produces: treat a non-string + // as absent and emit `required/label`, which would be a NEW error on a + // schema-valid config — the opposite of leaving behaviour where it was. + const issues = lintConfig(build(LOCALIZED) as any) as { rule: string }[]; + expect(issues.filter((i) => i.rule === 'required/label')).toEqual([]); + }); + } + + it('does not throw on a non-string, non-map label either', () => { + // Schema-INVALID input (no label carrier accepts a number), so this is + // not the defect's class — but the guard is written on the type, not on + // the map shape, and a linter that dies on bad input still cannot report + // the bad input. Falsified by a guard spelled `if (isLocaleMap(label))`. + expect(() => lintConfig(stackWithApp(42) as any)).not.toThrow(); + expect(caseIssues(lintConfig(stackWithApp(42) as any))).toEqual([]); + }); +}); + +describe('the localized fixtures are schema-VALID — this is not bad input', () => { + // Falsification: if any of these stopped parsing, the crash rows above + // would be pinning a diagnostic degrading on bad input (acceptable) rather + // than a tool that cannot walk a supported authoring shape (the defect). + const stacks: [string, unknown][] = [ + ['apps[].label', stackWithApp(LOCALIZED)], + ['views[].list.label', stackWithListLabel(LOCALIZED)], + ['views[].listViews.*.label', stackWithNamedListLabel(LOCALIZED)], + ['apps[].label, empty map', stackWithApp(EMPTY_MAP)], + ]; + + for (const [carrier, stack] of stacks) { + it(`${carrier} parses clean`, () => { + const parsed = ObjectStackDefinitionSchema.safeParse(normalizeStackInput(stack as any)); + expect(parsed.success).toBe(true); + }); + } + + it('CONTROL: a number label does NOT parse, so the parse check discriminates', () => { + const parsed = ObjectStackDefinitionSchema.safeParse(normalizeStackInput(stackWithApp(42) as any)); + expect(parsed.success).toBe(false); + }); +}); + +describe('the scorer reaches a verdict on a localized stack', () => { + // The join with the swallowed-crash repair one module over: that repair + // makes `scoreMetadata` REFUSE when a rule throws. With the guard there is + // no throw, so the refusal must not fire here. Falsification: drop the + // guard and `lintError` is set, `valid` is false, `grade` is 'F'. + it('scores it without a lint crash', () => { + const r = scoreMetadata(stackWithApp(LOCALIZED)); + expect(r.lintError).toBeUndefined(); + expect(r.valid).toBe(true); + expect(r.counts.schemaErrors).toBe(0); + expect(r.grade).not.toBe('F'); + }); +}); diff --git a/packages/cli/test/score-lint-crash.test.ts b/packages/cli/test/score-lint-crash.test.ts index 559cbb3995..ef90e28958 100644 --- a/packages/cli/test/score-lint-crash.test.ts +++ b/packages/cli/test/score-lint-crash.test.ts @@ -6,12 +6,15 @@ * * ## Why the linter is mocked here rather than driven * - * The crash IS reachable on a schema-valid stack — a localized `label` + * The crash WAS 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. + * made the label-case rule throw a `TypeError`. That was a defect in the rule, + * filed and fixed on its own (`convention/label-case` now guards on + * `typeof label === 'string'`; the pins live in + * `lint-label-case-localized.test.ts`). Pinning it here would have made this + * suite depend on a bug staying unfixed — and that day has since come: the + * repair landed, and had these assertions been driven through the real rule + * they would now be green for the wrong reason, or 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