Skip to content

Commit ada3834

Browse files
os-steveclaude
andauthored
feat(cli): make the silent hook-body downgrade loud — os lint refuses an accidental scope leak (ask 1) (#13834)
* feat(cli): make the silent hook-body downgrade a lint verdict An L2 hook handler that reaches out of the sandbox's scope is refused by `extractHookBody`; `lowerCallables` caught the refusal, recorded it, and bundled the closure anyway at exit 0. The deployment shape changed from metadata to bundle with nothing red. The refusal now carries the classification the refusing rule already had (`HookBodyExtractionError` / `HookBodyRefusalKind`), and `lowerCallables` carries it plus the free-identifier list on each warning. `os lint` reads the kind and splits the accidental class (an `error`, so a gate can fail on it) from the structural one (a `warning`, because bundling is its designed answer). The catch stays: what `os build` accepts is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UngCYXF98BVpYA9hfz6NYk * fix(cli): type the two test fixtures' index maps so tsc reads them The package's `tsconfig.json` says `include: ["src"]`, so these two new test files ARE in the package's own tsc program — and they carried 3 TS7053 (indexing a typed object with an `any` key). Measured, and fully attributed: removing exactly these two files returned TEST_DEBT['@objectstack/cli'] to its recorded 144 with no other entry moving, so the +3 was entirely theirs. Fixed at source. ⛔ The ledger entry is not raised — it is shrink-only and maintainer-only by the gate's own text. `Record<string, …>` changes nothing about the fixtures being module-scope free identifiers, which is what they are in the tests for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UngCYXF98BVpYA9hfz6NYk * fix(cli): keep instrument failures distinct from author verdicts in os lint (#13834 review) Contract-review items 1 and 2 on PR #13834: - judge() no longer folds `unparseable`/`unknown` into the hook-body/bundled-fallback arm, whose prose asserts the author chose a bundled closure. An instrument limit (unparseable) and an instrument failure (unknown) now report under their own rules — hook-body/unparseable and hook-body/extraction-failed — as warnings whose prose names the instrument, not the author. Severities are unchanged (warning), so the lint exit contract does not move. - Parity pins now cover all four kinds: the rule->kind map gains rows for unparseable and unknown, the parity fixture population includes both, and three unit pins assert the instrument kinds never borrow the designed-fallback prose. - Changeset: the false 'new exports' sentence is replaced by the real published carriers — os lint's exit contract (0 -> 1) and the kind / freeIdentifiers fields on os build --json bodyExtractionWarnings. Grade stays minor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UngCYXF98BVpYA9hfz6NYk --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 59c0891 commit ada3834

7 files changed

Lines changed: 848 additions & 8 deletions

File tree

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
---
2+
'@objectstack/cli': minor
3+
---
4+
5+
feat(cli): `os lint` refuses a hook body that silently stopped being metadata (#13651)
6+
7+
An L2 hook handler is lowered to a metadata-only `body.source` and evaluated in
8+
QuickJS with no module scope. When the handler reaches out of that scope,
9+
`extractHookBody` refuses — and `lowerCallables` caught the refusal, recorded it,
10+
and shipped the callable through the back-compat `.mjs` bundle instead. `os build`
11+
exited 0. The hook kept working locally. What changed silently was the
12+
**deployment shape**: the app had stopped being shippable as pure metadata.
13+
14+
The refusal was never the missing piece — `extractHookBody` had already computed
15+
the exact free-identifier list. What was missing is that nothing said no to the
16+
recorded array.
17+
18+
**The refusal now carries its classification — and what that publishes rides
19+
on two CLI surfaces, not on new API exports.** Internally,
20+
`HookBodyExtractionError` (with `HookBodyRefusalKind`) names which rule
21+
refused — `free-identifiers` / `forbidden-token` / `unparseable` — and
22+
`lowerCallables` records it beside the identifier list on each
23+
`bodyExtractionWarnings` entry. Those types are module-internal: the package
24+
`exports` map exposes only `.` and `./console`, and `src/index.ts` re-exports
25+
none of them. What this release actually publishes is:
26+
27+
- **`os lint`'s exit contract** — the new `hook-body/not-lowerable` rule is an
28+
`error`, so `os lint` can now exit 1 where it previously exited 0.
29+
- **`os build --json`** — each `bodyExtractionWarnings` entry now carries
30+
`kind` and `freeIdentifiers` fields beside the unchanged `origin`/`reason`.
31+
32+
**`os lint` is the first consumer, and it tells the classes apart.** They
33+
used to share one catch, so they shared one fate:
34+
35+
- **accidental** (`free-identifiers`) — the handler *is* expressible as a
36+
metadata body; it merely names a module-scope const, helper or import. Now a
37+
lint **`error`**, so a gate can fail on it. `os lint` exits 1.
38+
- **structural** (`forbidden-token`) — `fetch`/`require`/`process`/… are
39+
capabilities the sandbox does not have, so writing one *is* choosing a bundled
40+
closure and the bundle is the designed answer. Reported as a **`warning`**,
41+
never fatal.
42+
- **instrument** (`unparseable`, or a failure of the extractor itself) — the
43+
tool could not judge the body at all. Reported as a **`warning`** under its
44+
own rule, with prose that names the instrument — never as if the author chose
45+
a bundle, because an instrument failure is not a verdict about the author.
46+
47+
An author who deliberately wants a bundled closure keeps two channels that
48+
already existed and are still silent: give the hook an explicit `body`, or move
49+
the function into the top-level `functions:` map and reference it by name.
50+
51+
**What did NOT change: what `os build` accepts.** The catch in `lowerCallables`
52+
stays, both classes still fall back to bundling, and the build still exits 0 —
53+
verified over a real spawned `os build`. Flipping that default is a separate
54+
contract decision. The new rule runs the *same* `extractHookBody` the build
55+
runs, so the lint verdict cannot drift from what the build would do to the same
56+
handler.

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { lintDataModel, runAuthoringRules } from '@objectstack/lint';
1111
import { resolveSduiManifest } from '../utils/sdui-manifest.js';
1212
import { collectAndLintDocs } from '../utils/collect-docs.js';
1313
import { scoreMetadata } from '../lint/score.js';
14+
import { checkHookBodyLowering } from '../lint/hook-body-lowering.js';
1415
import { runMetadataEval } from '../lint/metadata-eval.js';
1516
import { DEFAULT_METADATA_EVAL_CORPUS } from '../lint/corpus.js';
1617
import {
@@ -400,6 +401,19 @@ export function lintConfig(config: any, opts: LintConfigOptions = {}): LintIssue
400401
}
401402
}
402403

404+
// ── Hook/action bodies that cannot be lowered to metadata (#13651) ──
405+
// `os build` catches every extraction refusal, warns, and bundles the closure
406+
// at exit 0 — so an app can stop being shippable as pure metadata with nothing
407+
// red anywhere. This rule is the "no" to that recorded array. It runs the SAME
408+
// `extractHookBody` the build runs, so the two cannot disagree, and it splits
409+
// the accidental class (an `error`, which a gate can fail on) from the
410+
// structural one (a `warning`, because bundling is its designed answer). It
411+
// does NOT move what `os build` accepts; see the rule module's header.
412+
//
413+
// Reads FUNCTION values, so it must run on the normalized input before any
414+
// Zod parse — which is where `lintConfig` already sits.
415+
issues.push(...checkHookBodyLowering(config as Record<string, unknown>));
416+
403417
// ── Data-model best practices (relationships / master-detail / roll-ups) ──
404418
// Cross-object rules that encode the conventions in ADR-0035 and the
405419
// objectstack-data/-ui skills. These double as the eval rubric (see score.ts).
Lines changed: 292 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,292 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #13651 — the silent downgrade becomes a lint verdict.
5+
*
6+
* The pins here are about the DISTINCTION, not just the noise: an accidental
7+
* scope leak must be an `error` (so a gate can fail on it) while the structural
8+
* refusal must stay a `warning` (so the legitimate fallback-to-bundling path is
9+
* not punished). A test that only asserted "something was reported" would pass
10+
* on the change this card explicitly forbids — deleting the catch.
11+
*/
12+
13+
import { describe, it, expect } from 'vitest';
14+
import {
15+
checkHookBodyLowering,
16+
NOT_LOWERABLE_RULE,
17+
BUNDLED_FALLBACK_RULE,
18+
UNPARSEABLE_BODY_RULE,
19+
EXTRACTION_FAILED_RULE,
20+
} from './hook-body-lowering.js';
21+
import { lowerCallables } from '../utils/lower-callables.js';
22+
23+
// Module scope — exactly what a lowered body cannot reach.
24+
const SLA_MATRIX: Record<string, number> = { high: 4, low: 48 };
25+
26+
const freeIdentifierHook = {
27+
name: 'case_sla',
28+
object: 'case',
29+
events: ['beforeInsert'],
30+
handler: (ctx: any) => {
31+
ctx.input.sla_hours = SLA_MATRIX[ctx.input.priority];
32+
},
33+
};
34+
35+
const forbiddenTokenHook = {
36+
name: 'enrich_lead',
37+
object: 'lead',
38+
events: ['beforeInsert'],
39+
handler: async (ctx: any) => {
40+
const res = await fetch('https://example.invalid/enrich');
41+
ctx.input.score = res.status;
42+
},
43+
};
44+
45+
const selfContainedHook = {
46+
name: 'normalize_name',
47+
object: 'account',
48+
events: ['beforeInsert'],
49+
handler: (ctx: any) => {
50+
ctx.input.name = String(ctx.input.name).trim();
51+
},
52+
};
53+
54+
// `unparseable`: `String(fn)` yields something `peelToBlockBody` cannot peel —
55+
// an instrument LIMIT, not anything the author chose.
56+
const unparseableHook = (() => {
57+
const fn = (ctx: any) => {
58+
ctx.input.x = 1;
59+
};
60+
Object.defineProperty(fn, 'toString', { value: () => '???' });
61+
return { name: 'opaque', object: 'o', events: ['beforeInsert'], handler: fn };
62+
})();
63+
64+
// `unknown`: the extractor itself throws a bare Error (not a
65+
// `HookBodyExtractionError`) — an instrument FAILURE. Same fixture shape the
66+
// build-side pin in `hook-body-refusal-kind.test.ts` uses.
67+
const explodingHook = (() => {
68+
const fn = () => undefined;
69+
Object.defineProperty(fn, 'toString', {
70+
value: () => {
71+
throw new Error('cannot stringify');
72+
},
73+
});
74+
return { name: 'exploding', object: 'o', events: ['beforeInsert'], handler: fn };
75+
})();
76+
77+
describe('checkHookBodyLowering', () => {
78+
it('reports an accidental scope leak as an ERROR a gate can fail on', () => {
79+
const issues = checkHookBodyLowering({ hooks: [freeIdentifierHook] });
80+
81+
expect(issues).toHaveLength(1);
82+
expect(issues[0].severity).toBe('error');
83+
expect(issues[0].rule).toBe(NOT_LOWERABLE_RULE);
84+
expect(issues[0].path).toBe('hooks[0].handler');
85+
// Names the callable and the identifier that caused it — the diagnostic
86+
// `extractHookBody` had already computed and the build dropped into a log.
87+
expect(issues[0].message).toContain("hook 'case_sla'");
88+
expect(issues[0].message).toContain('SLA_MATRIX');
89+
// Says what actually changed. "no behavior change" is true of behaviour and
90+
// false of deployment shape, which is the whole defect.
91+
expect(issues[0].message).toContain('deployment shape');
92+
});
93+
94+
it('keeps the STRUCTURAL refusal a warning — the bundle is its designed answer', () => {
95+
const issues = checkHookBodyLowering({ hooks: [forbiddenTokenHook] });
96+
97+
expect(issues).toHaveLength(1);
98+
expect(issues[0].severity).toBe('warning');
99+
expect(issues[0].rule).toBe(BUNDLED_FALLBACK_RULE);
100+
expect(issues[0].message).toContain("hook 'enrich_lead'");
101+
// Reported, but never fatal: `os lint` exits 1 on `error` only, so a
102+
// legitimate `fetch()` handler still lints clean-enough to ship.
103+
expect(issues.some((i) => i.severity === 'error')).toBe(false);
104+
});
105+
106+
it('says nothing about a handler that really does ship as metadata', () => {
107+
expect(checkHookBodyLowering({ hooks: [selfContainedHook] })).toEqual([]);
108+
});
109+
110+
it('truncates the multi-line offending-source dump to one line', () => {
111+
const [issue] = checkHookBodyLowering({ hooks: [forbiddenTokenHook] });
112+
expect(issue.message).not.toContain('--- offending body source ---');
113+
expect(issue.message.split('\n')).toHaveLength(1);
114+
});
115+
116+
describe('an instrument failure is NOT an author verdict', () => {
117+
// The whole PR exists because a silent downgrade was misattributed. Telling
118+
// an author "you chose a bundled closure" when in fact the TOOL failed is
119+
// the same wrong verdict wearing the fix's clothes — so `unparseable` and
120+
// `unknown` must never land in the `bundled-fallback` arm, whose prose
121+
// asserts "the body uses something the sandbox cannot provide".
122+
123+
it('reports an unparseable body under its own rule, naming the instrument', () => {
124+
const issues = checkHookBodyLowering({ hooks: [{ ...unparseableHook }] });
125+
126+
expect(issues).toHaveLength(1);
127+
expect(issues[0].rule).toBe(UNPARSEABLE_BODY_RULE);
128+
// Never fatal: an instrument limit must not move the exit contract.
129+
expect(issues[0].severity).toBe('warning');
130+
expect(issues[0].message).toContain('not a verdict about the handler');
131+
// And never the author-verdict prose of the deliberate-bundle arm.
132+
expect(issues[0].rule).not.toBe(BUNDLED_FALLBACK_RULE);
133+
expect(issues[0].message).not.toContain('the body uses something the sandbox cannot provide');
134+
});
135+
136+
it('reports an extractor throw (kind `unknown`) under its own rule, not as a chosen bundle', () => {
137+
const issues = checkHookBodyLowering({ hooks: [{ ...explodingHook }] });
138+
139+
expect(issues).toHaveLength(1);
140+
expect(issues[0].rule).toBe(EXTRACTION_FAILED_RULE);
141+
expect(issues[0].severity).toBe('warning');
142+
expect(issues[0].message).toContain('the extraction instrument itself failed');
143+
expect(issues[0].message).toContain('not a verdict about the handler');
144+
expect(issues[0].rule).not.toBe(BUNDLED_FALLBACK_RULE);
145+
expect(issues[0].message).not.toContain('the body uses something the sandbox cannot provide');
146+
});
147+
148+
it('keeps the two instrument kinds distinct from each other, not just from the verdict arms', () => {
149+
// `unknown` is deliberately not folded into `unparseable` (#13651): a
150+
// broken instrument and a limited instrument are different events.
151+
const [unparseable] = checkHookBodyLowering({ hooks: [{ ...unparseableHook }] });
152+
const [unknown] = checkHookBodyLowering({ hooks: [{ ...explodingHook }] });
153+
expect(unparseable.rule).not.toBe(unknown.rule);
154+
});
155+
});
156+
157+
describe('the two ways an author declares "bundle this deliberately"', () => {
158+
it('says nothing about a string handler (already a bundle reference)', () => {
159+
const issues = checkHookBodyLowering({
160+
hooks: [{ name: 'h', object: 'o', events: ['beforeInsert'], handler: 'some_bundled_fn' }],
161+
});
162+
expect(issues).toEqual([]);
163+
});
164+
165+
it('says nothing when the author supplied an explicit `body`', () => {
166+
// `lowerCallables` only extracts `if (!hook.body)`; this rule mirrors that
167+
// skip, so the opt-out is the same one the build already honours.
168+
const issues = checkHookBodyLowering({
169+
hooks: [{ ...freeIdentifierHook, body: { language: 'js', source: 'return 1;' } }],
170+
});
171+
expect(issues).toEqual([]);
172+
});
173+
174+
it('never judges a top-level `functions:` entry — that path is never lowered', () => {
175+
const issues = checkHookBodyLowering({
176+
functions: { my_fn: (ctx: any) => ctx.input.x = SLA_MATRIX.high },
177+
});
178+
expect(issues).toEqual([]);
179+
});
180+
});
181+
182+
describe('actions', () => {
183+
it('judges an object action `target` and names its path', () => {
184+
const issues = checkHookBodyLowering({
185+
objects: [
186+
{
187+
name: 'case',
188+
actions: [
189+
{ name: 'escalate', target: (ctx: any) => { ctx.out = SLA_MATRIX.high; } },
190+
],
191+
},
192+
],
193+
});
194+
expect(issues).toHaveLength(1);
195+
expect(issues[0].severity).toBe('error');
196+
expect(issues[0].path).toBe('objects[0].actions[0].target');
197+
expect(issues[0].message).toContain("action 'case_escalate'");
198+
});
199+
200+
it('judges a top-level action `target`', () => {
201+
const issues = checkHookBodyLowering({
202+
actions: [{ name: 'sweep', target: (ctx: any) => { ctx.out = SLA_MATRIX.low; } }],
203+
});
204+
expect(issues).toHaveLength(1);
205+
expect(issues[0].path).toBe('actions[0].target');
206+
expect(issues[0].message).toContain("action 'global_sweep'");
207+
});
208+
});
209+
210+
/**
211+
* The #3782 class: two surfaces disagreeing about what an author is told.
212+
* This rule and the build both call `extractHookBody` on the same normalized
213+
* input, so the agreement is by construction — this pins that it stays so,
214+
* and would fail the moment someone re-implements the analysis here.
215+
*/
216+
it('judges exactly the callables the build records as extraction warnings', () => {
217+
const config = {
218+
hooks: [
219+
freeIdentifierHook,
220+
forbiddenTokenHook,
221+
selfContainedHook,
222+
unparseableHook,
223+
explodingHook,
224+
{ name: 'string_ref', object: 'o', events: ['beforeInsert'], handler: 'bundled' },
225+
{ ...selfContainedHook, name: 'with_body', body: { language: 'js', source: 'return 1;' } },
226+
],
227+
objects: [
228+
{
229+
name: 'case',
230+
actions: [{ name: 'escalate', target: (ctx: any) => { ctx.out = SLA_MATRIX.high; } }],
231+
},
232+
],
233+
};
234+
235+
// What the BUILD records (and then bundles anyway, at exit 0).
236+
const lowering = lowerCallables(structuredClone_(config));
237+
const buildSaw = lowering.bodyExtractionWarnings
238+
.map((w) => `${w.origin}|${w.kind}`)
239+
.sort();
240+
241+
// What LINT reports, mapped back through the rule -> kind correspondence.
242+
// Total over all four kinds on purpose: a kind with no row here is a kind
243+
// whose divergent labeling this pin could never catch — which is exactly
244+
// how the `unknown`-folded-into-"designed fallback" defect survived to
245+
// review the first time.
246+
const kindOfRule: Record<string, string> = {
247+
[NOT_LOWERABLE_RULE]: 'free-identifiers',
248+
[BUNDLED_FALLBACK_RULE]: 'forbidden-token',
249+
[UNPARSEABLE_BODY_RULE]: 'unparseable',
250+
[EXTRACTION_FAILED_RULE]: 'unknown',
251+
};
252+
const lintSaw = checkHookBodyLowering(structuredClone_(config))
253+
.map((i) => {
254+
const origin = /^((?:hook|action) '[^']+')/.exec(i.message)?.[1];
255+
return `${origin}|${kindOfRule[i.rule]}`;
256+
})
257+
.sort();
258+
259+
expect(lintSaw).toEqual(buildSaw);
260+
// And the population is the real one, not an empty set agreeing with itself
261+
// — all four kinds present, the two instrument kinds included.
262+
expect(buildSaw).toEqual([
263+
"action 'case_escalate'|free-identifiers",
264+
"hook 'case_sla'|free-identifiers",
265+
"hook 'enrich_lead'|forbidden-token",
266+
"hook 'exploding'|unknown",
267+
"hook 'opaque'|unparseable",
268+
]);
269+
});
270+
});
271+
272+
/**
273+
* `structuredClone` cannot carry functions, and `lowerCallables` mutates only
274+
* shallow clones of what it is handed — so the two passes above must each get a
275+
* fresh object graph without losing the callables. A shallow-enough hand clone
276+
* is exactly that.
277+
*/
278+
function structuredClone_<T extends Record<string, any>>(v: T): T {
279+
return {
280+
...v,
281+
...(Array.isArray(v.hooks) ? { hooks: v.hooks.map((h: any) => ({ ...h })) } : {}),
282+
...(Array.isArray(v.objects)
283+
? {
284+
objects: v.objects.map((o: any) => ({
285+
...o,
286+
...(Array.isArray(o.actions) ? { actions: o.actions.map((a: any) => ({ ...a })) } : {}),
287+
})),
288+
}
289+
: {}),
290+
...(Array.isArray(v.actions) ? { actions: v.actions.map((a: any) => ({ ...a })) } : {}),
291+
};
292+
}

0 commit comments

Comments
 (0)