Skip to content

Commit 7d80695

Browse files
os-zhuangclaude
andauthored
fix(lint): an object declaring no fields is unjudgeable, not "has no such field" (#4383) (#4385)
`hook-body-write-unknown-field` and `action-body-write-unknown-field` reported EVERY field write to an object that declares no `fields` — an external object, or a datasource-introspected schema whose columns are resolved at runtime. `indexObjectFields` returns an empty Set for such an object rather than `undefined`, and both rules only asked "is this object in the stack?" (`targetSets.every((s) => s !== undefined)`, `if (!known) continue`). An empty Set is neither undefined nor falsy, so it became the answer to `has(field)` — and that answer is always false. The field map is not empty, it is UNKNOWN. Two other rules in the same family already drew that distinction with their reasons written down (validate-searchable-fields skip #2, and validate-flow-node-writes, which added the guard in #4369 because it gates). Two of four had it: the drift shape #3583 and #4330 exist to remove. Fixed once, not twice. The guard now lives in a shared `judgeableFieldsOf(index, objectName)` returning the declared names only when they are a sound basis for a "resolves to nothing" judgement, and `undefined` for both unjudgeable cases. All three write-set rules route through it, so a fourth cannot repeat the omission. Internal to the family — not re-exported from the package barrel, same as `indexObjectFields` and `IMPLICIT_FIELDS`. One semantic call worth naming: a multi-target hook where only SOME targets are judgeable is now skipped entirely. The `ctx.input` finding fires only when a field is missing from every target, and an unjudgeable target is one the field might well exist on — judging the remainder would assert "missing everywhere" on evidence that does not cover everywhere. Consistent with the rule's stated asymmetry: prefer a missed finding to a false one. No behaviour change for objects that declare fields; a test sits next to each new skip so the guard cannot swallow the real finding. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 10575f3 commit 7d80695

6 files changed

Lines changed: 181 additions & 15 deletions
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
fix(lint): an object declaring no fields is unjudgeable, not "has no such field" (#4383)
6+
7+
`hook-body-write-unknown-field` and `action-body-write-unknown-field` reported
8+
**every** field write to an object that declares no `fields` — an external
9+
object, or a datasource-introspected schema whose columns are resolved at
10+
runtime. Measured before the fix:
11+
12+
```
13+
hook : ["hook-body-write-unknown-field / warning"] ← false
14+
action: ["action-body-write-unknown-field / warning"] ← false
15+
flow : [] ← correct
16+
```
17+
18+
`indexObjectFields` returns an **empty Set** for such an object rather than
19+
`undefined`, and both rules only asked "is this object in the stack?" —
20+
`targetSets.every((s) => s !== undefined)` and `if (!known) continue`. An empty
21+
Set is neither undefined nor falsy, so it became the answer to `has(field)`,
22+
and the answer is always `false`.
23+
24+
That field map is not empty, it is **unknown**. The distinction already existed
25+
in two other rules of the same family, each with its reason written down —
26+
`validate-searchable-fields` skip #2 and `validate-flow-node-writes` (#4369,
27+
which added the guard because it gates). Two of four had it; the drift shape
28+
#3583 and #4330 exist to remove.
29+
30+
**Fixed once, not twice.** The guard now lives in a shared
31+
`judgeableFieldsOf(index, objectName)` that returns the declared names only when
32+
they are a sound basis for a "resolves to nothing" judgement, and `undefined`
33+
for both unjudgeable cases — cross-package objects and fields-less ones. All
34+
three write-set rules route their lookups through it, so a fourth cannot repeat
35+
the omission. It is internal to the family (not re-exported from the package
36+
barrel), same as `indexObjectFields` and `IMPLICIT_FIELDS`.
37+
38+
One semantic call worth naming: a **multi-target** hook where only *some*
39+
targets are judgeable is now skipped entirely. The `ctx.input` finding fires
40+
only when a field is missing from EVERY target, and an unjudgeable target is one
41+
the field might well exist on — so judging the remainder would assert "missing
42+
everywhere" on evidence that does not cover everywhere. Consistent with the
43+
rule's stated asymmetry: prefer a missed finding to a false one.
44+
45+
No behaviour change for objects that declare fields: an unknown field on a
46+
normal object still warns exactly as before, pinned by a test placed next to
47+
each new skip so the guard cannot swallow the real finding.

packages/lint/src/validate-action-body-writes.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,24 @@ describe('validateActionBodyWrites — ctx.api writes', () => {
199199
expect(findings).toEqual([]);
200200
});
201201

202+
// #4383 — an object declaring NO fields (external / datasource-introspected,
203+
// columns resolved at runtime) is unjudgeable, not "has no such field". The
204+
// empty Set used to answer `has()` false for every write to it.
205+
it('stays silent on an object that declares no fields', () => {
206+
const stack = {
207+
objects: [dealObject, { name: 'legacy_deal', label: 'Legacy Deal', external: true }],
208+
actions: [
209+
{
210+
name: 'sync_legacy',
211+
label: 'Sync Legacy',
212+
objectName: 'crm_deal',
213+
body: { language: 'js', source: "await ctx.api.object('legacy_deal').update({ stage: 'won' });" },
214+
},
215+
],
216+
};
217+
expect(validateActionBodyWrites(stack)).toEqual([]);
218+
});
219+
202220
it('reports each unknown field once per action, even when written repeatedly', () => {
203221
const findings = validateActionBodyWrites(
204222
stackWith(

packages/lint/src/validate-action-body-writes.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared';
8080
import {
8181
extractHookBodyWriteSet,
8282
indexObjectFields,
83+
judgeableFieldsOf,
8384
IMPLICIT_FIELDS,
8485
HOOK_BODY_WRITE_PATTERNS,
8586
type BodyWritePatternExclusion,
@@ -322,8 +323,8 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[
322323
const dedupeKey = `${w.object}\u0000${w.field}`;
323324
if (reported.has(dedupeKey)) continue;
324325

325-
const known = objectFields.get(w.object);
326-
if (!known) continue; // object declared by another package — cannot judge
326+
const known = judgeableFieldsOf(objectFields, w.object);
327+
if (!known) continue; // cross-package, or no declared fields — cannot judge
327328
if (IMPLICIT_FIELDS.has(w.field) || known.has(w.field)) continue;
328329

329330
reported.add(dedupeKey);

packages/lint/src/validate-flow-node-writes.ts

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@
8484

8585
import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared';
8686

87-
import { indexObjectFields, IMPLICIT_FIELDS } from './validate-hook-body-writes.js';
87+
import { indexObjectFields, judgeableFieldsOf, IMPLICIT_FIELDS } from './validate-hook-body-writes.js';
8888

8989
export type FlowNodeWriteSeverity = 'error';
9090

@@ -210,13 +210,12 @@ export function validateFlowNodeWrites(stack: AnyRec): FlowNodeWriteFinding[] {
210210
if (!objectName) return; // templated / dynamic object — resolved at run time
211211

212212
objectFields ??= indexObjectFields(stack);
213-
const known = objectFields.get(objectName);
214-
if (!known) return; // object declared by another package — cannot judge its fields
215-
// An object that declares NO fields is an external / datasource-introspected
216-
// schema whose columns are resolved at runtime (the same skip
217-
// validate-searchable-fields takes). Every write to it would otherwise be
218-
// flagged, and this rule gates.
219-
if (known.size === 0) return;
213+
// Cross-package objects and objects declaring no fields at all (external /
214+
// datasource-introspected schemas) are both unjudgeable, and this rule
215+
// gates — see {@link judgeableFieldsOf}, which is where that guard now
216+
// lives for the whole family rather than once per rule (#4383).
217+
const known = judgeableFieldsOf(objectFields, objectName);
218+
if (!known) return;
220219

221220
const nodeName =
222221
typeof node.label === 'string' && node.label

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,52 @@ describe('validateHookBodyWrites — ctx.input writes', () => {
183183
);
184184
expect(findings).toEqual([]);
185185
});
186+
187+
// #4383 — an object that declares NO fields is an external object or a
188+
// datasource-introspected schema whose columns are resolved at runtime. Its
189+
// field map is not empty, it is UNKNOWN; treating the empty Set as an answer
190+
// reported every write to such an object as a missing field.
191+
it('a target declaring no fields at all is unjudgeable — silent, not "no such field"', () => {
192+
const stack = {
193+
objects: [{ name: 'legacy_deal', label: 'Legacy Deal', external: true }],
194+
hooks: [
195+
{
196+
name: 'h',
197+
object: 'legacy_deal',
198+
events: ['beforeInsert'],
199+
body: { language: 'js', source: "ctx.input.stage = 'won';" },
200+
},
201+
],
202+
};
203+
expect(validateHookBodyWrites(stack)).toEqual([]);
204+
});
205+
206+
it('one fields-less target makes a multi-target everywhere-miss unsound — silent', () => {
207+
// The finding fires only when a field is missing from EVERY target, and the
208+
// opaque target is one it might well exist on. Judging the rest would state
209+
// "missing everywhere" on evidence that does not cover everywhere.
210+
const stack = {
211+
objects: [dealObject, { name: 'legacy_deal', external: true }],
212+
hooks: [
213+
{
214+
name: 'h',
215+
object: ['crm_deal', 'legacy_deal'],
216+
events: ['beforeInsert'],
217+
body: { language: 'js', source: "ctx.input.nowhere_field = 'x';" },
218+
},
219+
],
220+
};
221+
expect(validateHookBodyWrites(stack)).toEqual([]);
222+
});
223+
224+
it('still warns when every target is judgeable', () => {
225+
// The guard above must not swallow the real finding: same shape, but both
226+
// targets declare fields.
227+
const findings = validateHookBodyWrites(
228+
stackWith("ctx.input.nowhere_field = 'x';", { object: ['crm_deal', 'crm_contact'] }),
229+
);
230+
expect(findings).toHaveLength(1);
231+
});
186232
});
187233

188234
describe('validateHookBodyWrites — ctx.api writes', () => {
@@ -219,6 +265,22 @@ describe('validateHookBodyWrites — ctx.api writes', () => {
219265
);
220266
expect(findings).toEqual([]);
221267
});
268+
269+
// #4383 — same unjudgeable class as a cross-package object, on the ctx.api side.
270+
it('stays silent on an object that declares no fields', () => {
271+
const stack = {
272+
objects: [dealObject, { name: 'legacy_deal', external: true }],
273+
hooks: [
274+
{
275+
name: 'h',
276+
object: 'crm_deal',
277+
events: ['afterInsert'],
278+
body: { language: 'js', source: "await ctx.api.object('legacy_deal').update({ stage: 'won' });" },
279+
},
280+
],
281+
};
282+
expect(validateHookBodyWrites(stack)).toEqual([]);
283+
});
222284
});
223285

224286
describe('validateHookBodyWrites — scope and shape tolerance', () => {

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

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,39 @@ export function indexObjectFields(stack: AnyRec): Map<string, Set<string>> {
303303
return out;
304304
}
305305

306+
/**
307+
* The declared field names of `objectName` — but ONLY when they are a sound
308+
* basis for judging "this name resolves to nothing". Otherwise `undefined`.
309+
*
310+
* Two different unknowns collapse to one answer on purpose, because every
311+
* caller in this family owes them the same silence:
312+
*
313+
* • the object is not in this stack — another package declares it, and a
314+
* field map we cannot see cannot be judged;
315+
* • the object is here but declares NO fields at all — an external object or
316+
* a datasource-introspected schema whose columns are resolved at runtime.
317+
* Its field map is not empty, it is *unknown*, and an empty Set answers
318+
* `has(anything) === false`, which reads as "no such field" for EVERY write
319+
* to it. That is a false-positive generator, and a false positive kills an
320+
* advisory lint (#4383).
321+
*
322+
* The distinction is unused today — no rule in the family wants to act on one
323+
* and not the other — so collapsing it here is what stops the guard from being
324+
* hand-copied per call site and forgotten at one of them, which is exactly how
325+
* it went missing from the hook and action rules while
326+
* `validate-searchable-fields` (skip #2) and `validate-flow-node-writes` both
327+
* had it. A future caller that genuinely needs to tell them apart should read
328+
* the index directly and say why.
329+
*/
330+
export function judgeableFieldsOf(
331+
index: ReadonlyMap<string, Set<string>>,
332+
objectName: string,
333+
): Set<string> | undefined {
334+
const declared = index.get(objectName);
335+
if (!declared || declared.size === 0) return undefined;
336+
return declared;
337+
}
338+
306339
/** One statically-extracted field write found in an L2 body. */
307340
export interface ExtractedHookBodyWrite {
308341
/** Which {@link HOOK_BODY_WRITE_PATTERNS} entry matched. */
@@ -563,12 +596,18 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] {
563596
const hookName = typeof hook.name === 'string' && hook.name ? hook.name : `#${hookIndex}`;
564597

565598
// The hook's own target set, for `ctx.input` writes. A wildcard target has
566-
// no single object to check against; an unknown (cross-package) target
567-
// cannot be judged — either way `ctx.input` writes are skipped, not guessed.
599+
// no single object to check against; a target whose fields cannot be judged
600+
// ({@link judgeableFieldsOf} — cross-package, or declaring no fields at all)
601+
// gives nothing to resolve against — either way `ctx.input` writes are
602+
// skipped, not guessed.
568603
const targets = (Array.isArray(hook.object) ? hook.object : [hook.object]).filter(
569604
(o): o is string => typeof o === 'string' && o.trim() !== '',
570605
);
571-
const targetSets = targets.map((t) => objectFields!.get(t));
606+
const targetSets = targets.map((t) => judgeableFieldsOf(objectFields!, t));
607+
// ALL targets must be judgeable, not just one: the finding below fires only
608+
// when a field is missing from EVERY target, and an unjudgeable target is
609+
// one the field might well exist on. One opaque target therefore makes the
610+
// whole "missing everywhere" claim unsound, not merely narrower (#4383).
572611
const inputJudgeable =
573612
targets.length > 0 && !targets.includes('*') && targetSets.every((s) => s !== undefined);
574613

@@ -608,8 +647,8 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] {
608647
});
609648
} else {
610649
// ctx.api write → the named object.
611-
const known = objectFields!.get(w.object);
612-
if (!known) continue; // object declared by another package — cannot judge
650+
const known = judgeableFieldsOf(objectFields!, w.object);
651+
if (!known) continue; // cross-package, or no declared fields — cannot judge
613652
if (IMPLICIT_FIELDS.has(w.field) || known.has(w.field)) continue;
614653

615654
reported.add(dedupeKey);

0 commit comments

Comments
 (0)