Skip to content

Commit e958468

Browse files
baozhoutaoclaude
andauthored
fix(lint): report a hook write-set finding on a lowered hook at hooks[i].handler, not the un-authored body.source (#16546) (#17438)
lowerCallables now records which hooks[*].handler refs had their body minted from the inline handler it replaced; validateReadonlyHookWrites and validateHookBodyWrites redirect path (+ a message suffix) there via one shared helper, so os build and os lint keep agreeing word for word. Author-written bodies are unaffected; no verdict moved. Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU Co-authored-by: Claude <noreply@anthropic.com>
1 parent ab56ea3 commit e958468

14 files changed

Lines changed: 350 additions & 35 deletions
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
"@objectstack/lint": patch
3+
"@objectstack/cli": patch
4+
---
5+
6+
fix(lint): a hook write-set finding on a handler-authored hook reports `path: hooks[i].handler` — a key the author actually wrote — instead of the lowered `hooks[i].body.source` (#16546)
7+
8+
`hook-api-update-readonly-field` / `hook-api-update-readonly-when-field`
9+
(`validate-readonly-hook-writes.ts`) and `hook-body-write-unknown-field` /
10+
`hook-body-write-unprovisioned-anchor` / `hook-body-source-unparseable`
11+
(`validate-hook-body-writes.ts`) all report their `path` against `hook.body`,
12+
because that is the shape they parse. For a hook authored as an inline
13+
`handler: async (ctx) => { … }` (39 of 39 hooks in the reference app),
14+
`hooks[i].body` is not something the author wrote at all — `lowerCallables`
15+
mints it from the handler before `os build` / `os lint` hand the stack to
16+
these rules (#16095). The reported `path` therefore named a key that does not
17+
exist in the author's own source file; grepping for `body.source` there finds
18+
nothing.
19+
20+
**What changed.** `lowerCallables` now records, per `lowerCallables()` call,
21+
which `hooks[*].handler` ref strings got their `body` minted this way (as
22+
opposed to a `body` the author wrote directly). The CLI's four lowering doors
23+
(`os build`, `os lint`, `os validate`, `os init`/`dev`'s scaffold validation)
24+
pass that set through `runAuthoringRules`'s `ctx.loweredHookRefs`, and the two
25+
hook write-set rules use it to redirect a finding on a lowered hook to
26+
`path: hooks[i].handler` — the key that replaced the function the author
27+
wrote — with a message suffix ("judged on the metadata body lowered from the
28+
inline handler") explaining why. A hook whose `body` the author wrote directly
29+
is unaffected: `path` stays `hooks[i].body.source`, unchanged.
30+
31+
**No verdict changed.** Which hooks are flagged, at what severity, and why is
32+
untouched — #13653 and #4271 are unmoved by a word. Only the location a
33+
finding points at, and the wording explaining it, are different. `os build`
34+
and `os lint` continue to report the identical `path` and message for the
35+
same hook (#16095's "one implementation, both commands agree" — now including
36+
this).
37+
38+
No `--json` field was added or removed: `path` and `message` keep their
39+
existing shape (string), and this is a within-type value correction for the
40+
one subclass whose old value could never be resolved against the author's
41+
source in the first place.

‎packages/cli/src/commands/compile.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,10 @@ export default class Compile extends Command {
416416
normalized: authoringRuleUnionStack(normalized as Record<string, unknown>),
417417
parsed: authoringRuleUnionStack(result.data as Record<string, unknown>),
418418
sduiManifest: resolveSduiManifest(),
419+
// [#16546] Ref strings, not hook indices — survive the union fold and
420+
// the per-package re-slice below unchanged (see `LoweringResult.
421+
// loweredHookRefs`'s header for why an index would not).
422+
loweredHookRefs: lowering.loweredHookRefs,
419423
});
420424
const { errors: ruleErrors, advisories } = splitBySeverity(findings);
421425
ruleAdvisories = advisories;
@@ -489,6 +493,7 @@ export default class Compile extends Command {
489493
normalized: asStack,
490494
parsed: asStack,
491495
sduiManifest: resolveSduiManifest(),
496+
loweredHookRefs: lowering.loweredHookRefs,
492497
}).filter((f) => !alreadyReported.has(findingKey(f)));
493498
for (const f of pkgFindings) alreadyReported.add(findingKey(f));
494499
const split = splitBySeverity(pkgFindings);

‎packages/cli/src/commands/lint.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -511,11 +511,15 @@ export function lintConfig(config: any, opts: LintConfigOptions = {}): LintIssue
511511
// the family stays silent on it and `checkHookBodyLowering` is what
512512
// reports it, so no verdict is ever given about a body that was not
513513
// produced. Nothing here touches what `os build` accepts (#13838).
514-
const { lowered } = lowerCallables(config as Record<string, unknown>);
514+
const { lowered, loweredHookRefs } = lowerCallables(config as Record<string, unknown>);
515515
for (const f of runAuthoringRules('lint', {
516516
normalized: config,
517517
parsed: lowered,
518518
sduiManifest: opts.sduiManifest,
519+
// [#16546] Same ref set `os build` computes from the same normalized
520+
// input — what lets `validateReadonlyHookWrites` / `validateHookBodyWrites`
521+
// report `hooks[i].handler` here byte-identically to `os build`.
522+
loweredHookRefs,
519523
})) {
520524
issues.push({
521525
severity: f.severity === 'info' ? 'suggestion' : f.severity,

‎packages/cli/src/commands/validate.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,8 +260,8 @@ export default class Validate extends Command {
260260
// own contract decision (`compile.ts` records why the key is
261261
// build's alone). No step line is printed either: the text face is
262262
// byte-for-byte what it was, and the docs transcripts stay true.
263-
const { lowered } = lowerCallables(normalized as Record<string, unknown>);
264-
const result = ObjectStackDefinitionSchema.safeParse(lowered);
263+
const lowering = lowerCallables(normalized as Record<string, unknown>);
264+
const result = ObjectStackDefinitionSchema.safeParse(lowering.lowered);
265265

266266
if (!result.success) {
267267
if (flags.json) {
@@ -304,6 +304,9 @@ export default class Validate extends Command {
304304
normalized: normalized as Record<string, unknown>,
305305
parsed: result.data as Record<string, unknown>,
306306
sduiManifest: resolveSduiManifest(),
307+
// [#16546] Same ref set `os build` / `os lint` compute — keeps this
308+
// door's hook write-set findings at the same `path` as the other two.
309+
loweredHookRefs: lowering.loweredHookRefs,
307310
});
308311
const { errors: ruleErrors, advisories } = splitBySeverity(findings);
309312
ruleAdvisories = advisories;

‎packages/cli/src/utils/lower-callables.ts‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,29 @@ export interface LoweringResult {
5959
bodyExtracted: number;
6060
/** Per-extraction failures (still emit handler ref + bundle, but warn). */
6161
bodyExtractionWarnings: BodyExtractionWarning[];
62+
/**
63+
* [#16546] The `hooks[*].handler` ref strings whose `body` was minted HERE
64+
* — extracted from the author's inline `handler` function — as opposed to a
65+
* `body` the author wrote directly. A ref lands here if and only if this
66+
* call set `hook.body`, so the set is empty whenever every hook already
67+
* carried its own `body`.
68+
*
69+
* A ref, not a hook index: `register`'s `taken`/`refByFn` maps are shared
70+
* across the whole walk (top level AND every ADR-0130 D4 package body, see
71+
* `lowerBody`'s header), so a ref is unique for this whole call and
72+
* survives unchanged through a Zod parse (`handler` is an ordinary
73+
* string field) — an index would not: `authoringRuleUnionStack` folds
74+
* package hooks into a new top-level array with its OWN numbering, and a
75+
* hook index recorded against this call's `hooks[i]` would silently
76+
* address a different hook once folded.
77+
*
78+
* Consumed by `validateReadonlyHookWrites` / `validateHookBodyWrites`
79+
* (`@objectstack/lint`) via `AuthoringRuleContext.loweredHookRefs`, to
80+
* report `path: hooks[i].handler` — a key that exists in the author's
81+
* source — instead of `hooks[i].body.source`, a key this function minted
82+
* and the author never wrote.
83+
*/
84+
loweredHookRefs: ReadonlySet<string>;
6285
}
6386

6487
type AnyFn = (...args: unknown[]) => unknown;
@@ -83,6 +106,7 @@ export function lowerCallables(input: Record<string, unknown>): LoweringResult {
83106
const functions: Record<string, AnyFn> = {};
84107
const taken = new Set<string>();
85108
const warnings: BodyExtractionWarning[] = [];
109+
const loweredHookRefs = new Set<string>();
86110
let bodyExtracted = 0;
87111

88112
// Try to extract a metadata-only body from a callable. Returns null if the
@@ -170,7 +194,12 @@ export function lowerCallables(input: Record<string, unknown>): LoweringResult {
170194
// Extract metadata body unless the user already provided one.
171195
if (!hook.body) {
172196
const body = tryExtractBody(hook.handler as AnyFn, `hook '${name}'`);
173-
if (body) hook.body = body;
197+
if (body) {
198+
hook.body = body;
199+
// [#16546] This ref's `body` was minted here, not authored — the
200+
// marker the two hook write-set rules redirect their `path` on.
201+
loweredHookRefs.add(ref);
202+
}
174203
}
175204
hook.handler = ref;
176205
}
@@ -296,6 +325,7 @@ export function lowerCallables(input: Record<string, unknown>): LoweringResult {
296325
count: Object.keys(functions).length,
297326
bodyExtracted,
298327
bodyExtractionWarnings: warnings,
328+
loweredHookRefs,
299329
};
300330
}
301331

‎packages/cli/src/utils/scaffold-validate.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,9 @@ export function runScaffoldAuthoringRules(config: unknown): ScaffoldRuleReport {
8888
normalized: normalized as Record<string, unknown>,
8989
parsed: result.data as Record<string, unknown>,
9090
sduiManifest: resolveSduiManifest(),
91+
// [#16546] Same ref set the other three doors compute — keeps this
92+
// door's hook write-set findings at the same `path` as they are.
93+
loweredHookRefs: lowering.loweredHookRefs,
9194
});
9295
const { errors, advisories } = splitBySeverity(findings);
9396

‎packages/cli/test/lint-hook-rules-reach-handler-hooks.test.ts‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,25 @@ describe('#16095 — INTAKE: a handler-authored hook reaches the readonly family
7878
expect(findings).toHaveLength(1);
7979
expect(findings[0].severity).toBe('error');
8080
expect(findings[0].message).toContain('is_escalated');
81-
// The finding is reported on the LOWERED body — the same path `os build`
82-
// reports it on for the same hook, so the two commands agree word for word.
83-
expect(findings[0].path).toBe('hooks[0].body.source');
81+
// [#16546] `body.source` is a key this author never wrote — the body was
82+
// MINTED here from their inline `handler`, so the finding points at the
83+
// key they actually wrote instead, with a suffix saying why. `os build`
84+
// reports the identical `path` and message suffix for the same hook (see
85+
// the e2e sibling's parity assertion), so the two commands still agree
86+
// word for word — just no longer on the wrong word.
87+
expect(findings[0].path).toBe('hooks[0].handler');
88+
expect(findings[0].message).toContain('judged on the metadata body lowered from the inline handler');
8489
});
8590

86-
it('CONTROL — the identical statement as an explicit `body` fires the same finding', () => {
91+
it('CONTROL — the identical statement as an explicit `body` fires the same finding, path UNCHANGED', () => {
92+
// [#16546] Non-regression control: this hook's `body` is author-written,
93+
// not lowered from a `handler` — `hooks[0].body.source` names a key that
94+
// is really there, and the fix must never move it.
8795
const findings = rulesOf(bodyHook('escalate', WRITE_READONLY_SOURCE), READONLY_RULE);
8896
expect(findings).toHaveLength(1);
8997
expect(findings[0].severity).toBe('error');
9098
expect(findings[0].path).toBe('hooks[0].body.source');
99+
expect(findings[0].message).not.toContain('judged on the metadata body lowered from the inline handler');
91100
});
92101

93102
it('RED — `hook-api-update-readonly-when-field` (warning) fires on an inline handler too', () => {
@@ -99,6 +108,8 @@ describe('#16095 — INTAKE: a handler-authored hook reaches the readonly family
99108
);
100109
expect(findings).toHaveLength(1);
101110
expect(findings[0].severity).toBe('warning');
111+
// [#16546] Same fix, this rule's other finding id.
112+
expect(findings[0].path).toBe('hooks[0].handler');
102113
});
103114

104115
it('RED — `hook-body-write-unknown-field` fires on an inline handler writing an undeclared field', () => {
@@ -111,6 +122,9 @@ describe('#16095 — INTAKE: a handler-authored hook reaches the readonly family
111122
expect(findings).toHaveLength(1);
112123
expect(findings[0].severity).toBe('warning');
113124
expect(findings[0].message).toContain('is_escalatd');
125+
// [#16546] The sibling validator (`validate-hook-body-writes.ts`) gets the
126+
// same fix — same `ctx.loweredHookRefs`, same shared `hookBodyFindingLocation`.
127+
expect(findings[0].path).toBe('hooks[0].handler');
114128
});
115129
});
116130

@@ -227,16 +241,21 @@ describe('#16095 — door: `runScaffoldAuthoringRules` (reached, and not one of
227241
};
228242

229243
it('INTAKE — the handler-authored hook IS judged here (it lowers before it parses)', () => {
230-
expect(
231-
familyOf(
232-
handlerHook('escalate', async (ctx: any) => {
233-
await ctx.api.object('crm_case').update({ id: ctx.input.id, is_escalated: true });
234-
}),
235-
),
236-
).toHaveLength(1);
244+
const findings = familyOf(
245+
handlerHook('escalate', async (ctx: any) => {
246+
await ctx.api.object('crm_case').update({ id: ctx.input.id, is_escalated: true });
247+
}),
248+
);
249+
expect(findings).toHaveLength(1);
250+
// [#16546] This door also gets `ctx.loweredHookRefs` (`scaffold-validate.ts`
251+
// hands it through the same as `os build` / `os lint` do), so it must not
252+
// be the one door still pointing at the un-authored `body.source` key.
253+
expect(findings[0].path).toBe('hooks[0].handler');
237254
});
238255

239-
it('CONTROL — the identical statement as an explicit `body` fires the same finding', () => {
240-
expect(familyOf(bodyHook('escalate_body', WRITE_READONLY_SOURCE))).toHaveLength(1);
256+
it('CONTROL — the identical statement as an explicit `body` fires the same finding, path UNCHANGED', () => {
257+
const findings = familyOf(bodyHook('escalate_body', WRITE_READONLY_SOURCE));
258+
expect(findings).toHaveLength(1);
259+
expect(findings[0].path).toBe('hooks[0].body.source');
241260
});
242261
});

‎packages/cli/test/lower-callables.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,47 @@ describe('lowerCallables', () => {
101101
});
102102
expect(out.bodyExtracted).toBe(1);
103103
expect((out.lowered.hooks as any[])[0].body).toBeDefined();
104+
// [#16546] The ref this hook was minted under (its own name here, no
105+
// collision) is recorded as body-from-handler — what the two hook
106+
// write-set rules read to redirect their finding's `path`.
107+
expect((out.lowered.hooks as any[])[0].handler).toBe('contained_hook');
108+
expect(out.loweredHookRefs.has('contained_hook')).toBe(true);
109+
expect(out.loweredHookRefs.size).toBe(1);
110+
});
111+
112+
it('[#16546] does NOT mark the ref when body extraction fails — no body was minted to misreport', () => {
113+
const handler = (ctx: any) => {
114+
ctx.record.slug = moduleHelper(ctx.record.name);
115+
};
116+
const out = lowerCallables({
117+
hooks: [{ name: 'slugify_hook_2', handler, events: ['beforeInsert'], object: 'doc' }],
118+
});
119+
expect(out.bodyExtracted).toBe(0);
120+
expect(out.loweredHookRefs.size).toBe(0);
121+
});
122+
123+
it('[#16546] does NOT mark the ref when the hook already carries an author-written `body`', () => {
124+
// `lowerCallables` never extracts over an existing `body` (`if (!hook.body)`)
125+
// — the `handler` is still lowered to a ref for bundling, but the body
126+
// judged by the write-set rules is the one the author wrote, so its `path`
127+
// must stay `body.source`.
128+
const handler = (ctx: any) => {
129+
ctx.record.id = Math.round(Number(ctx.record.raw));
130+
};
131+
const out = lowerCallables({
132+
hooks: [
133+
{
134+
name: 'authored_body_hook',
135+
handler,
136+
events: ['beforeInsert'],
137+
object: 'doc',
138+
body: { language: 'js', source: 'ctx.record.id = 1;' },
139+
},
140+
],
141+
});
142+
expect((out.lowered.hooks as any[])[0].handler).toBe('authored_body_hook');
143+
expect((out.lowered.hooks as any[])[0].body).toEqual({ language: 'js', source: 'ctx.record.id = 1;' });
144+
expect(out.loweredHookRefs.size).toBe(0);
104145
});
105146

106147
it('produces a JSON-serializable lowered shape', () => {

‎packages/lint/src/authoring-rules.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,22 @@ export interface AuthoringRuleContext {
299299
* argument.
300300
*/
301301
runtimeWriteType?: string;
302+
/**
303+
* [#16546] `hooks[*].handler` ref strings whose metadata `body` was minted
304+
* by `lowerCallables` from the author's inline `handler` function, rather
305+
* than authored directly. Set by the CLI commands that lower before they
306+
* run this registry (`os build`, `os lint`, `os validate`, scaffold
307+
* validate — `lowerCallables`'s own return value, unchanged); ABSENT on the
308+
* runtime publish gate, which never receives a `handler` function to lower.
309+
*
310+
* Read by `validateReadonlyHookWrites` / `validateHookBodyWrites`
311+
* (`@objectstack/lint`) through the reference-integrity suite, to report
312+
* `path: hooks[i].handler` — the key the author actually wrote — instead of
313+
* `hooks[i].body.source`, a key this stack's `body` did not come from the
314+
* author for. Absent or not naming a hook's ref means "this hook's `body`
315+
* is author-written", and both validators keep reporting `body.source`.
316+
*/
317+
loweredHookRefs?: ReadonlySet<string>;
302318
}
303319

304320
export interface AuthoringRule {
@@ -1623,7 +1639,7 @@ export function authoringRulesFor(command: AuthoringCommand): readonly Authoring
16231639
*/
16241640
export function runAuthoringRules(command: AuthoringCommand, run: AuthoringRuleRun): AuthoringFinding[] {
16251641
const findings: AuthoringFinding[] = [];
1626-
const ctx: AuthoringRuleContext = { sduiManifest: run.sduiManifest };
1642+
const ctx: AuthoringRuleContext = { sduiManifest: run.sduiManifest, loweredHookRefs: run.loweredHookRefs };
16271643
for (const rule of authoringRulesFor(command)) {
16281644
const stack = rule.input === 'normalized' ? run.normalized : (run.parsed ?? run.normalized);
16291645
findings.push(...rule.run(stack, ctx));

0 commit comments

Comments
 (0)