Skip to content

Commit d1dbe70

Browse files
fix(lint)!: four authoring rules now fire on the sibling spelling of a defect they already caught (#22281)
Part of #22212 Clause-②: no (narrowing: each fix makes an existing rule fire on a sibling spelling) Four of the card's six members are closed here: items 1, 3, 4 and 6. Items 2 and 5 are left out because each needs a new rule id. Details are in "Left out" below. No rule id is added, and no file outside `packages/lint/src` or `.changeset/` is touched. There is one commit per item, item 1 first, each carrying its own pins, plus one changeset commit. Every pin keeps the card's firing control beside the probe that now fires. ## What changes ### Item 1: `hook-api-update-readonly-field` / `hook-body-write-unknown-field` and an aliased `ctx.api` - **Mechanism (H1 confirmed).** Both rules read writes from `extractHookBodyWriteSet` in `validate-hook-body-writes.ts`. `validate-readonly-hook-writes.ts` imports it and has no extractor of its own. The `api-crud-literal` matcher required `isCtxDot(receiver, 'api')`, so only the literal `ctx.api.object(...)` matched. - **Fix.** The receiver can now be `ctx.api` or a local bound to it. A new `collectCtxApiAliases` pre-pass finds these locals. Type-only and non-null wrappers are seen through: `as T`, a type assertion, `satisfies T`, `x!` and parentheses. - **Followed:** `const|let|var api = ctx.api` (including the mandated `as HookApi | undefined` spelling), `const { api } = ctx`, `const { api: db } = ctx`, and an optional call `api?.object(...)`. - **Left opaque, deliberately.** The card left open whether a reassigned or shadowed `api` must stay opaque. It does, and so do these: - a name declared more than once anywhere in the body, such as a nested parameter or a second `const`; - any assignment to the name; - a reference outside the declaring block, or outside the declaring function for `var`; - a non-alias initializer such as `ctx.api ?? x`; - an alias of an alias; - a destructure with a default; - `const api = ctx.api.sudo()`. The elevated channel stays invisible, as the readonly rule's header requires. - **Ledger.** `HOOK_BODY_WRITE_PATTERNS` `api-crud-literal` now states the receiver in its syntax line, and its reconciliation example includes the aliased spelling. The action-body write rules share the extractor and consume `api-crud-literal`, so they read the same receivers. The action ledger's end-to-end test pins that through the updated example. ### Item 3: `visibility-bare-identifier` and an unbound namespace root - **Mechanism.** `firstBareIdentifier` declared every receiver-position name (`namespaceRoots`) before the strict check. `foo.duplicate_of_type` therefore always passed. The module note justified this with "the set of legal roots is not yet trustworthy enough to gate on (#6146)". - **H3, measured.** That doubt concerns *members* of the root set, and the rule still judges none of them. What it judges now is the *complement* of a set that is generous by contract. The checker already declares `@objectstack/formula` `SCOPE_ROOTS`, whose published contract is that a missing root is a false build error. `VIEW_PAGE_EXTRA_ROOTS` (`current_user`, `page`) is added to that. - **What the renderers bind** (objectui `main` `f3a0488`, read at `buildExpressionScope`, `SchemaRenderer`, the form renderer and the metadata-admin `predicate.ts`): - form field: `record`, `previous`, `current_user`, `user`, `ctx`, `os`, `features`; - page component: those plus `page` and the adapter `data`; - metadata form: `data` plus the identity roots. Every one of these is in the union. - **Fix.** The verdict is now the checker's alone. The receiver walk only classifies how the name was written, so an unbound namespace gets its own sentence and hint under the same id. - **One stand-down.** On a metadata-editing form, a dotted chain on the right of `==` / `!=` stays `predicate-rhs-path-shaped`'s, which is #7696's single-voice rule one step further (`rhsChainRoots`). - **Three pins changed.** They encoded the old exclusion: - `my_record.x` on the metadata layer: still no mis-layer advisory, and now an unbound-root finding; - "an UNKNOWN root is left to the wrong-root rules": rewritten to its surviving half, which is that a wrong-but-bound root stays the ADR-0089 rules'; - "an unknown root does not mask a bare identifier": both are now defects, the first is reported, and fixing it reveals the next. ### Item 4: `security-master-detail-ungranted` and a lookup-bound child - **H4 measured.** `SecurityPlugin.resolveCbpRelation` (`plugin-security/src/security-plugin.ts`) resolves a `controlled_by_parent` object's master in this order: required `master_detail`, then any `master_detail`, then a **required `lookup`**. Lint's `resolveCbpRelation` / `CBP_TIERS` mirror it point for point. On the card's shape (a cbp object with a lookup and no `master_detail`), the runtime's parent is that lookup. An optional lookup resolves nothing, and `security-controlled-by-parent-no-relation` already reports that. - **Fix.** The new `derivedAccessParent` answers a cbp object through `resolveCbpRelation`. Every other object keeps the `firstMasterDetailField` reading. A child with two `master_detail` fields is now reported against the master the runtime picks. The message names the relation type. ### Item 6: `component-props-*` and a nested component with no `properties` - **H6 confirmed.** `validateComponentProps` ran `if (!props) continue` on `isRec(component.properties)`. - **Fix.** An absent bag is judged as `{}`, which is the value `PageComponentSchema`'s default gives a top-level node. A present non-object bag is still skipped. - **Fixture triage.** The #5775 container fixture used `{ type: 'element:text' }` as filler children. The fix updated the filler (it now carries the one required prop). The test's subject, the container keys, is unchanged. ## Left out, and why (`Part of`) - **Item 2 (an action body referencing an undeclared identifier).** No existing rule judges it. `action-body-source-unparseable` answers a body that does not parse, and this body parses. The `os lint` `hook-body/*` rules answer a *handler* refused at lowering, not an authored `body.source`. - H2: the sandbox's globals DO have one declared, measured source. That is `SANDBOX_GLOBALS` in `packages/cli/src/utils/detect-free-identifiers.ts`, pinned by `sandbox-globals-probe.test.ts`. - That source lives in `@objectstack/cli`, which depends on `@objectstack/lint`, so the lint package cannot read it without moving it. - Item 2 therefore needs a new rule id and an edit outside the surface. Both are stop conditions. - **Item 5 (an empty `record:details` section, `fields: []`).** H5 holds. `page-section-group-unknown` is a reference rule: it resolves a section's `group` key against the object's field groups. An empty `fields` array carries no reference to resolve. Firing that id on it would change what the id means, so item 5 needs a new rule id. ## Measurements **Unit-door probe** (`validate*` from the built `@objectstack/lint` dist). - Before: at the base `9f0de32a`, every probe is silent and every control fires. - After: at `4ab0ff40`, every probe fires beside its control: ```text item 1 control ctx.api.object(..).update readonly-field(error) + unknown-field(warning) probe const api = ctx.api before: silent after: same two findings probe ... as HookApi | undefined before: silent after: same two findings probe api?.object(..) before: silent after: same two findings probe const api = ctx.api! before: silent after: same two findings opaque reassigned / shadowed silent before and after item 3 control bare duplicate_of_type == ... visibility-bare-identifier(error) probe foo.duplicate_of_type before: silent after: visibility-bare-identifier(error) item 4 control master_detail child ungranted security-master-detail-ungranted(warning) probe required-lookup cbp child before: silent after: security-master-detail-ungranted(warning) item 6 control top-level, properties {} component-props-invalid(warning) probe nested, properties absent before: silent after: component-props-invalid(warning) ``` **Public door, `os lint --strict` at `4ab0ff40`.** These are scratch configs inside `examples/app-todo`, removed afterwards (`git status` clean). - The control config fires all four rules: `hook-api-update-readonly-field` at `hooks[0].handler`, `visibility-bare-identifier`, `security-master-detail-ungranted` at `objects[2].fields.campaign`, and `component-props-invalid`. - The probe config, with each item's sibling spelling, fires the same four rules, with the nested path `...components[0].properties.children[0].properties.relationshipField`. - Both exit 1. **Fixture sweep.** - `os lint --json --skip-i18n` at `4ab0ff40` on `examples/app-crm`, `app-todo`, `app-showcase` and `app-multi-package` reports **0** findings from any affected rule id. The registry runs on that door: `security-private-no-readscope` and `approval-approvers-may-resolve-empty` both appear. - No change can remove a finding: each one only widens what its rule reads. So zero at head also means no newly firing example site. - The lint fixture corpus is the package's own suite: one fixture newly fired (the #5775 filler above), and it is triaged. **Tests and typecheck.** - `pnpm --filter @objectstack/lint exec vitest run --maxWorkers=2`: 126 files and 5793 tests passed. - `pnpm --filter @objectstack/lint typecheck` (tsc plus `check:test-typecheck`): exit 0. **ESLint, narrowed and proven.** - The command was `eslint --no-inline-config --format json` on the 10 changed `.ts` files. The JSON output lists 10 files linted, with 0 errors and 0 warnings. None was ignored. - `eslint.config.mjs` never enables type-aware linting (no `parserOptions.project`; its own note at the `QUERY_OPTIONS_TEST_GLOBS` block says so). So this diff cannot move any untouched file's verdict. **Gates.** The derived gate list (`dispatch-gates --commands`, 62 families at `4ab0ff40`) was run, and `--ran` was reconciled. The results are in the report comment on the card. ## Acceptance notes - **Message wording.** The readonly and unknown-field messages still quote the receiver as `ctx.api.object('X')...` when the author wrote `api.object('X')...`. That is true of the call, because `api` is `ctx.api`, but an author grepping for it will not find it. The location path points at the body or handler. Not filed. - **Over-approximate stand-down.** `rhsChainRoots` stands down a root that appears anywhere on the right of a metadata form's `==` / `!=`, including inside a macro body. That direction can only remove a finding. - **Changeset level: `minor`, not `patch`.** The PR declares `Clause-②: no (narrowing)`. `check-changeset-no-major` enforces at least `minor` on a package whose `src` this PR grows when the arm is `narrowing`, and AGENTS.md reads that arm as BREAKING. The changeset therefore carries the `**BREAKING**` banner and an ADR-0087 `not-required (no-migration-prescription)` disposition. That gate passes locally. --- _Generated by [Claude Code](https://claude.ai/code/session_01DhTqaEHqPVSVnAkjG3jywn)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5870716 commit d1dbe70

11 files changed

Lines changed: 692 additions & 63 deletions
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/lint': minor
3+
---
4+
5+
fix(lint)!: four authoring rules now fire on the sibling spelling of a defect they already caught
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, export or stored shape is removed, renamed or re-shaped, and no stored row is read or rewritten, so there is nothing for `objectstack migrate meta` to rewrite. What narrows is the set of stacks four existing @objectstack/lint rules pass: each now reports, under its existing id and severity, a spelling of the defect it already reported in another spelling. The fix for every new finding is the one the rule's own hint already gives. The other categories are closed on facts: @objectstack/lint publishes (not unpublished); no ADR-0087 id covers these paths and this diff adds none (not registered / already-registered); and no exported TypeScript declaration changes (not runtime-interface-only / type-surface-only). -->
10+
11+
**BREAKING**: an accept-set narrowing of the `os lint`, `os validate` and `os build` verdicts, shipped as `minor` under the launch-window convention for accept-set narrowings. No rule id is added. Each rule below now reports a spelling of a defect it already reported, at the severity it already had.
12+
13+
- **`hook-api-update-readonly-field`** (error) and **`hook-body-write-unknown-field`** (warning) read a hook body's writes from one extractor. That extractor accepted only the literal `ctx.api.object(...)` receiver. A body that binds the handle first, such as `const api = ctx.api as HookApi | undefined; await api.object('crm_case').update({...})`, was invisible to both rules. The byte-identical `ctx.api.object(...)` statement fired both. The receiver can now also be a local bound to `ctx.api`, with or without type-only and non-null wrappers. That covers `const api = ctx.api`, `const { api } = ctx`, `const { api: db } = ctx` and an optional call `api?.object(...)`. The local must be declared once and never reassigned, and the write must sit in the block that declares it. A reassigned, shadowed, defaulted or out-of-scope local is still not judged, and neither is a local bound to `ctx.api.sudo()`. The action-body write rules share the extractor, so they read the same receivers.
14+
- **`visibility-bare-identifier`** (error) used to accept any name written as a namespace. `has(record.x) && foo.x == 'crm_lead'` published clean, and the console's engine then answered `Unknown variable: foo` and rendered the element unconditionally. A namespace root that no layer binds is now reported, with its own message and fix. Every root a renderer binds still passes: `record`, `previous`, `parent`, `current_user`, `user`, `ctx`, `os`, `features`, `page` and `data`, plus everything in `@objectstack/formula`'s `SCOPE_ROOTS`. On a metadata-editing form, a dotted chain on the right of `==` / `!=` is still left to `predicate-rhs-path-shaped`.
15+
- **`security-master-detail-ungranted`** (warning) used to treat an object as a detail only when it had a `master_detail` field. The runtime also resolves a `controlled_by_parent` object's master through a required lookup. That child, granted in no permission set, gets the same never-derived 403 on object-level CRUD. It is now reported too. A child with two `master_detail` fields is reported against the master the runtime picks.
16+
- **`component-props-invalid`** / **`component-props-unknown-key`** (warning) skipped a component whose `properties` bag was absent. A top-level component never arrives without one, because `PageComponentSchema` defaults it to `{}`. A nested component (in `children`, `items[].children` or `footer`) is never parsed by that schema, so every required prop it omitted went unreported. An absent bag is now judged as `{}`.
17+
18+
**What an author sees.** A stack that passed before can now draw one of these findings. Each one names a real defect: a write the runtime drops or refuses, a predicate that can never evaluate, a role that is denied the object, or a component missing a prop its contract requires. The fix is in the finding's hint. The readonly-write and visibility findings are errors, so they fail `os build`. The others are warnings, which fail only `os lint --strict`.

‎packages/lint/src/validate-component-props.test.ts‎

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,51 @@ describe('validateComponentProps — value verdicts', () => {
223223
);
224224
});
225225

226+
/**
227+
* [#22212] A NESTED component with NO `properties` bag at all. The top-level
228+
* firing control parses through `PageComponentSchema`, whose default makes the
229+
* absent bag `{}`; a nested node sits in an untyped slot and never gets that
230+
* default, so it reached this rule as `undefined` and was skipped — measured
231+
* on nested `record:related_list`, `page:accordion` and `element:text` in the
232+
* reporting app. The control and the probe are the same node, one level apart.
233+
*/
234+
describe('a nested component whose `properties` are absent (#22212)', () => {
235+
const required = (f: ReturnType<typeof validateComponentProps>) => invalid(f).map((x) => x.path);
236+
237+
it('the firing control: a top-level node with an empty bag reports its required props', () => {
238+
expect(required(validateComponentProps(stackWith([{ type: 'record:related_list', properties: {} }]))))
239+
.toContain('pages[0].regions[0].components[0].properties.relationshipField');
240+
});
241+
242+
it('the parsed door: the top-level node with NO bag reports the same (the schema default)', () => {
243+
const parsed = normalizeStackInput(stackWith([{ type: 'record:related_list' }])) as AnyRec;
244+
expect(required(validateComponentProps(parsed)))
245+
.toContain('pages[0].regions[0].components[0].properties.relationshipField');
246+
});
247+
248+
it.each([
249+
['a container\'s `children`', { type: 'page:section', properties: { children: [{ type: 'record:related_list' }] } },
250+
'pages[0].regions[0].components[0].properties.children[0].properties.relationshipField'],
251+
['a tab panel\'s `items[].children`',
252+
{ type: 'page:tabs', properties: { items: [{ label: 'T', value: 't', children: [{ type: 'record:related_list' }] }] } },
253+
'pages[0].regions[0].components[0].properties.items[0].children[0].properties.relationshipField'],
254+
])('the probe: a nested node with NO bag in %s reports the same required prop', (_label, node, path) => {
255+
expect(required(validateComponentProps(stackWith([node])))).toContain(path);
256+
});
257+
258+
it('a present bag that is not an object is still left to the parse that owns its shape', () => {
259+
expect(validateComponentProps(
260+
stackWith([{ type: 'page:section', properties: { children: [{ type: 'record:related_list', properties: 'x' }] } }]),
261+
)).toEqual([]);
262+
});
263+
264+
it('a nested node whose required props are all optional stays silent with no bag', () => {
265+
expect(validateComponentProps(
266+
stackWith([{ type: 'page:section', properties: { children: [{ type: 'page:header' }] } }]),
267+
)).toEqual([]);
268+
});
269+
});
270+
226271
/**
227272
* #7702 — `PageHeaderProps.title` used to be required while the platform's
228273
* own synthesizer (objectui `buildDefaultHeader`) emits every seeded
@@ -415,12 +460,16 @@ describe('validateComponentProps — value verdicts', () => {
415460
* tab items' `value`/`count` are the same shape one level down.
416461
*/
417462
it('reports nothing on the container/child keys the renderers honour (#5775)', () => {
463+
// The child is filler: the subject is the CONTAINER keys. Since #22212 a
464+
// nested node is judged even with no `properties` bag, so the filler carries
465+
// the one prop `element:text` requires instead of omitting it.
466+
const TEXT_CHILD = { type: 'element:text', properties: { content: 'Hi' } };
418467
const findings = validateComponentProps(
419468
stackWith([
420-
{ type: 'page:card', properties: { title: 'Shortcuts', children: [{ type: 'element:text' }] } },
421-
{ type: 'page:section', properties: { children: [{ type: 'element:text' }] } },
422-
{ type: 'page:footer', properties: { children: [{ type: 'element:text' }] } },
423-
{ type: 'page:sidebar', properties: { children: [{ type: 'element:text' }] } },
469+
{ type: 'page:card', properties: { title: 'Shortcuts', children: [TEXT_CHILD] } },
470+
{ type: 'page:section', properties: { children: [TEXT_CHILD] } },
471+
{ type: 'page:footer', properties: { children: [TEXT_CHILD] } },
472+
{ type: 'page:sidebar', properties: { children: [TEXT_CHILD] } },
424473
{
425474
type: 'page:tabs',
426475
properties: { items: [{ label: 'Tasks', value: 'related:task', count: 3, children: [] }] },

‎packages/lint/src/validate-component-props.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,20 @@ export function validateComponentProps(stack: AnyRec): ComponentPropsFinding[] {
243243
// map does not carry.
244244
const schema = PROPS_SCHEMAS[type];
245245
if (!schema) continue;
246-
const props = isRec(component.properties) ? component.properties : undefined;
246+
// [#22212] An ABSENT bag is judged as `{}` — what `PageComponentSchema`'s
247+
// `properties` default makes it. That default only ever reaches the
248+
// components the schema parses, which are the top-level ones: a NESTED
249+
// component (in another's `children` / `items[].children` / `footer`) sits
250+
// in a `z.array(z.unknown())` slot and keeps its `undefined`. Skipping
251+
// that hid every required prop such a component omits, while the same
252+
// node one level up drew `component-props-invalid`. A PRESENT bag that is
253+
// not an object is still left to the parse that owns its shape.
254+
const props =
255+
component.properties === undefined
256+
? {}
257+
: isRec(component.properties)
258+
? component.properties
259+
: undefined;
247260
if (!props) continue;
248261

249262
const where = `page "${pageName}" · ${type}`;

‎packages/lint/src/validate-hook-body-writes.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,25 @@ describe('validateHookBodyWrites — ctx.api writes', () => {
312312
expect(finding.message).not.toMatch(/write-path validator skips/);
313313
});
314314

315+
// [#22212] The firing control and the aliased spelling, side by side: the
316+
// reporting app's conventions mandate the alias, so before this every one of
317+
// its writes reached neither hook write-set rule.
318+
it('reads a local bound to ctx.api as the same receiver (#22212)', () => {
319+
const WRITE = "object('crm_deal').update({ id, stag: 'won' });";
320+
const control = validateHookBodyWrites(stackWith(`await ctx.api.${WRITE}`));
321+
expect(control).toHaveLength(1);
322+
expect(control[0].rule).toBe(HOOK_BODY_WRITE_UNKNOWN_FIELD);
323+
for (const source of [
324+
`const api = ctx.api as HookApi | undefined;\nif (!api) return;\nawait api.${WRITE}`,
325+
`const api = ctx.api!;\nawait api?.${WRITE}`,
326+
`const { api } = ctx;\nawait api.${WRITE}`,
327+
]) {
328+
expect(validateHookBodyWrites(stackWith(source)), source).toEqual(control);
329+
}
330+
// ...and an alias the extractor cannot prove is ctx.api stays unjudged.
331+
expect(validateHookBodyWrites(stackWith(`let api = ctx.api;\napi = other;\nawait api.${WRITE}`))).toEqual([]);
332+
});
333+
315334
it('checks updateById payloads at argument 1, not 0', () => {
316335
const findings = validateHookBodyWrites(
317336
stackWith("await ctx.api.object('crm_deal').updateById(ctx.input.id, { stag: 'won' });"),

0 commit comments

Comments
 (0)