diff --git a/.changeset/functions-array-lowered-handler.md b/.changeset/functions-array-lowered-handler.md new file mode 100644 index 0000000000..62d6dac22d --- /dev/null +++ b/.changeset/functions-array-lowered-handler.md @@ -0,0 +1,46 @@ +--- +"@objectstack/spec": minor +"@objectstack/runtime": patch +--- + +fix(spec,runtime): `functions: [{ name, handler }]` survives `objectstack build` (#6238) + +The array form of the top-level `functions` collection could not pass its own +build. `lowerCallables` has lowered the array branch the whole time — it rewrites +both `handler` and `name` to the emitted ref — but the array member of the +`functions` union in `stack.zod.ts` still demanded `handler: z.function()`. So +`objectstack build` produced +`[{ name: 'syncBilling', handler: 'syncBilling', effect: 'writes' }]` and then +rejected it, with `invalid_union: Invalid input` and a path stopping at +`functions`: no entry named, no key named, no reason given. + +This is the third time the same seam has parted, and the first two fixes are why +this one only looks small. #4343 taught the union the bare lowered ref; #4976 +taught it the lowered *declaration*. Both only ever touched the **map** member — +the array member is a separate inline record (an array entry names itself, so it +carries `name` and an optional `packageId` and cannot be `FlowFunctionEntrySchema` +in a list), and widening one never widened the other. + +**The fix.** The array member's `handler` now accepts the lowered string ref +beside the authored callable. One widening covers both array spellings at once, +unlike the map form's two separate members: `effect` is already optional on an +array entry, so the bare and the declared entry differ only in whether that key +is present. All four cells of map/array × bare/declared now round-trip. + +**The load seam, which the fix made reachable.** `mergeRuntimeModule` re-attaches +each callable from the sibling ESM module to the declaration the JSON carried. +Its array branch fell through to a map rebuild — `existing` was `{}` whenever +`bundle.functions` was an array — so the merged bundle came back as a bare +`{ name: callable }` map with `effect: 'writes'` dropped on the floor. The +function still registered and still ran, and its writes were counted as none: +#4396's silent un-declaring arriving by the other door, and exactly the state +that keeps #4354's broken-sweep alert quiet on the one run that needed it. Since +the parse rejected the array form until now, no built artifact had ever reached +that branch; it is fixed in the same change rather than shipped as a live trap. +The array shape is preserved, callables are attached per entry `name`, and a +module function the artifact declared no entry for still registers — the map +branch keeps those, and the array branch must not ship fewer functions than the +bundle was built with. + +Authoring is unchanged and nothing narrows: this widens what the artifact form +accepts. The map form is still the preferred spelling. diff --git a/packages/cli/src/utils/lower-callables.test.ts b/packages/cli/src/utils/lower-callables.test.ts index 2c8c3f7d10..448bd78679 100644 --- a/packages/cli/src/utils/lower-callables.test.ts +++ b/packages/cli/src/utils/lower-callables.test.ts @@ -142,49 +142,70 @@ describe('lowerCallables — declared `functions` entries (#4396)', () => { // hand-written sample is a third copy of the truth and drifts exactly the way // the two halves already did. // -// SCOPE: the map form. The ARRAY form (`functions: [{ name, handler }]`) does -// not round-trip either — in both its bare and declared spellings, since #4343 -// and #4976 each only ever touched the map — and its member lives in -// `stack.zod.ts` rather than in `FlowFunctionEntrySchema`. Filed as #6238; -// extend the parametrisation below when it lands. -describe('lowerCallables → the spec parses what it emits (#4976)', () => { +// SCOPE: all four cells of map/array × bare/declared. The ARRAY form +// (`functions: [{ name, handler }]`) was the last one still broken — in BOTH +// its spellings, because #4343 and #4976 each only ever touched the map, while +// `lowerCallables` has lowered the array branch the whole time. Its member +// lives inline in `stack.zod.ts` rather than in `FlowFunctionEntrySchema` +// (an array entry names itself, so it is a different record, not the same +// schema in a list), which is why widening one did not widen the other. #6238 +// widened it; the parametrisation below now covers the array form too. +describe('lowerCallables → the spec parses what it emits (#4976, #6238)', () => { const base = { manifest: { id: 'com.example.demo', name: 'demo', version: '1.0.0', type: 'app' as const }, }; /** Exactly what `objectstack compile` does, in the order it does it. */ - const buildPipeline = (functions: Record) => { + const buildPipeline = (functions: unknown) => { const stack = defineStack({ ...base, functions } as never); const normalized = normalizeStackInput(stack as Record); return lowerCallables(normalized); }; - const cases: Array<[label: string, functions: Record]> = [ - ['a bare handler', { scoreLead: () => ({ score: 1 }) }], - ['a declared writer', { syncBilling: { handler: () => ({ ok: true }), effect: 'writes' } }], - ['a declaration that states the pure default', { scoreLead: { handler: () => ({ score: 1 }), effect: 'pure' } }], - ['a declaration that states nothing', { scoreLead: { handler: () => ({ score: 1 }) } }], - ['both spellings side by side', { + /** + * `form` drives the entry-level assertion, which can only run on the map: + * `FlowFunctionEntrySchema` is the map entry's schema and is exported, while + * the array member is inline in `ObjectStackDefinitionSchema`. The array + * cells are pinned by the whole-stack parse below — the assertion the build + * actually makes. + */ + const cases: Array<[label: string, form: 'map' | 'array', functions: unknown]> = [ + ['a bare handler', 'map', { scoreLead: () => ({ score: 1 }) }], + ['a declared writer', 'map', { syncBilling: { handler: () => ({ ok: true }), effect: 'writes' } }], + ['a declaration that states the pure default', 'map', { scoreLead: { handler: () => ({ score: 1 }), effect: 'pure' } }], + ['a declaration that states nothing', 'map', { scoreLead: { handler: () => ({ score: 1 }) } }], + ['both spellings side by side', 'map', { scoreLead: () => ({ score: 1 }), syncBilling: { handler: () => ({ ok: true }), effect: 'writes' }, }], + // ── the array form (#6238) ── + ['an array entry with a bare handler', 'array', [{ name: 'scoreLead', handler: () => ({ score: 1 }) }]], + ['an array entry declaring a writer', 'array', [{ name: 'syncBilling', handler: () => ({ ok: true }), effect: 'writes' }]], + ['an array entry declaring the pure default', 'array', [{ name: 'scoreLead', handler: () => ({ score: 1 }), effect: 'pure' }]], + ['an array entry carrying a packageId', 'array', [{ name: 'scoreLead', handler: () => ({ score: 1 }), packageId: 'com.example.pkg' }]], + ['several array entries side by side', 'array', [ + { name: 'scoreLead', handler: () => ({ score: 1 }) }, + { name: 'syncBilling', handler: () => ({ ok: true }), effect: 'writes' }, + ]], ]; - for (const [label, functions] of cases) { - it(`parses every entry it emits for ${label}`, () => { - const emitted = (buildPipeline(functions).lowered as { - functions: Record; - }).functions; - - for (const [name, entry] of Object.entries(emitted)) { - const result = FlowFunctionEntrySchema.safeParse(entry); - expect( - result.success, - `emitted entry '${name}' (${JSON.stringify(entry)}) is not a shape FlowFunctionEntrySchema accepts: ` - + JSON.stringify(result.success ? [] : result.error.issues), - ).toBe(true); - } - }); + for (const [label, form, functions] of cases) { + if (form === 'map') { + it(`parses every entry it emits for ${label}`, () => { + const emitted = (buildPipeline(functions).lowered as { + functions: Record; + }).functions; + + for (const [name, entry] of Object.entries(emitted)) { + const result = FlowFunctionEntrySchema.safeParse(entry); + expect( + result.success, + `emitted entry '${name}' (${JSON.stringify(entry)}) is not a shape FlowFunctionEntrySchema accepts: ` + + JSON.stringify(result.success ? [] : result.error.issues), + ).toBe(true); + } + }); + } it(`parses the whole lowered stack for ${label}`, () => { // The assertion the build itself makes (`compile.ts` step 3). Parsing the @@ -217,4 +238,19 @@ describe('lowerCallables → the spec parses what it emits (#4976)', () => { expect(JSON.parse(JSON.stringify(lowered)).functions.syncBilling) .toEqual({ handler: 'syncBilling', effect: 'writes' }); }); + + it('carries an ARRAY entry\'s declaration into the artifact too (#6238)', () => { + // Same guarantee, other spelling. The array branch of `lowerCallables` + // rewrites `name` to the ref as well as `handler`, so both keys must come + // back as the ref and `effect` must survive beside them. + const { lowered } = buildPipeline([ + { name: 'syncBilling', handler: () => ({ ok: true }), effect: 'writes' }, + ]); + const parsed = ObjectStackDefinitionSchema.parse(lowered) as { + functions: Array<{ name: string; handler: string; effect: string }>; + }; + expect(parsed.functions).toEqual([{ name: 'syncBilling', handler: 'syncBilling', effect: 'writes' }]); + expect(JSON.parse(JSON.stringify(lowered)).functions) + .toEqual([{ name: 'syncBilling', handler: 'syncBilling', effect: 'writes' }]); + }); }); diff --git a/packages/runtime/src/artifact-function-declarations.test.ts b/packages/runtime/src/artifact-function-declarations.test.ts index 141e369c70..5b8c423ed9 100644 --- a/packages/runtime/src/artifact-function-declarations.test.ts +++ b/packages/runtime/src/artifact-function-declarations.test.ts @@ -62,6 +62,52 @@ describe('mergeRuntimeModule — declared functions', () => { expect((entries.syncBilling.handler as () => unknown)()).toEqual({ ok: true }); }); + it('re-attaches into the ARRAY form without dropping what it declared (#6238)', async () => { + // The array spelling reaches this seam for the first time now that + // #6238 lets it past the parse. Rebuilding it as a map would attach the + // callable and drop `effect` beside it — the same silent un-declaring + // #4396 fixed for the map form, arriving by the other door. + const bundle: any = { + runtimeModule: './objectstack-runtime.mjs', + // What `lowerCallables`'s array branch emits: `name` and `handler` + // both rewritten to the ref, the declaration kept beside them. + functions: [ + { name: 'scoreLead', handler: 'scoreLead' }, + { name: 'syncBilling', handler: 'syncBilling', effect: 'writes' }, + ], + }; + + await mergeRuntimeModule(bundle, artifactPath); + + expect(Array.isArray(bundle.functions)).toBe(true); + const [scoreLead, syncBilling] = bundle.functions; + expect(typeof scoreLead.handler).toBe('function'); + expect(typeof syncBilling.handler).toBe('function'); + expect(syncBilling.effect).toBe('writes'); + expect(syncBilling.name).toBe('syncBilling'); + + const entries = collectBundleFunctionEntries(bundle); + expect(entries.scoreLead.effect).toBe('pure'); + expect(entries.syncBilling.effect).toBe('writes'); + expect((entries.syncBilling.handler as () => unknown)()).toEqual({ ok: true }); + }); + + it('registers a module function the array form declared no entry for (#6238)', async () => { + // The map branch keeps these; the array branch must not ship fewer + // functions than the bundle was built with. + const bundle: any = { + runtimeModule: './objectstack-runtime.mjs', + functions: [{ name: 'syncBilling', handler: 'syncBilling', effect: 'writes' }], + }; + + await mergeRuntimeModule(bundle, artifactPath); + + const entries = collectBundleFunctionEntries(bundle); + expect(Object.keys(entries).sort()).toEqual(['scoreLead', 'syncBilling']); + expect(entries.syncBilling.effect).toBe('writes'); + expect((entries.scoreLead.handler as () => unknown)()).toEqual({ score: 1 }); + }); + it('leaves a bundle with no runtimeModule alone', async () => { const handler = () => ({ ok: true }); const bundle: any = { functions: { syncBilling: { handler, effect: 'writes' } } }; diff --git a/packages/runtime/src/load-artifact-bundle.ts b/packages/runtime/src/load-artifact-bundle.ts index eb318b5ed4..af71b4773b 100644 --- a/packages/runtime/src/load-artifact-bundle.ts +++ b/packages/runtime/src/load-artifact-bundle.ts @@ -142,7 +142,39 @@ export async function mergeRuntimeModule(bundle: any, artifactAbsPath: string, t console.warn(`${tag} runtime module '${moduleAbsPath}' exported no \`functions\` map`); return; } - const existing = (bundle.functions && typeof bundle.functions === 'object' && !Array.isArray(bundle.functions)) + // The ARRAY form (`[{ name, handler: '', effect }]`) carries its + // declaration exactly like the map form, but names itself by an entry's + // `name` instead of by a map key. Rebuilding it as a map below would + // attach the callable and drop everything standing beside it — + // `effect: 'writes'` included — which is #4396's silent un-declaring in + // the other spelling: the function still registers, still runs, and its + // writes are still counted as none, so #4354's broken-sweep alert stays + // quiet on the one run that needed it. Unreachable until #6238 let the + // array form past the parse; reachable now, so it is handled here. + if (Array.isArray(bundle.functions)) { + const moduleFns = fns as Record; + const attached = new Set(); + const mergedEntries = (bundle.functions as unknown[]).map((entry) => { + if (!entry || typeof entry !== 'object' || Array.isArray(entry)) return entry; + const record = entry as Record; + const name = typeof record.name === 'string' ? record.name : undefined; + if (name === undefined) return entry; + const fn = moduleFns[name]; + if (typeof fn !== 'function') return entry; + attached.add(name); + return { ...record, handler: fn }; + }); + // A module function the artifact declared no entry for still has to + // register — the map branch keeps those, and dropping them here + // would make the array form quietly ship fewer functions than it + // was built with. + for (const [name, fn] of Object.entries(moduleFns)) { + if (typeof fn === 'function' && !attached.has(name)) mergedEntries.push({ name, handler: fn }); + } + bundle.functions = mergedEntries; + return; + } + const existing = (bundle.functions && typeof bundle.functions === 'object') ? bundle.functions as Record : {}; // The module supplies the CALLABLE; the JSON supplies what the function diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index bc6880d8fb..485687e663 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -441,12 +441,36 @@ export const ObjectStackDefinitionSchema = lazySchema(() => z.object({ * (what it declared rides along in the artifact and is re-attached on load). * The `AppPlugin` registers them on the engine before binding hooks so * `string` handlers resolve at startup. + * + * BOTH shapes therefore reach this schema twice: once as authored, once + * lowered. All four combinations (map/array × bare/declared) are accepted — + * the map's two lowered forms since #4343 and #4976, the array's since #6238. + * `packages/cli`'s `lower-callables.test.ts` pins every cell against what the + * lowering actually emits, rather than against a belief about it. */ functions: z.union([ z.record(z.string(), FlowFunctionEntrySchema), + // The array member is NOT `FlowFunctionEntrySchema` in a list: an array + // entry carries its own `name` (and an optional `packageId`) because it has + // no map key to be named by, so the two shapes are genuinely different + // records rather than one reused schema. + // + // `handler` accepts the lowered string ref for the same reason the map form + // does (#4343, #4976), and this member was the last place it did not: + // `lowerCallables` lowers the array branch too (`next.handler = ref`, + // `next.name = ref`), so `objectstack build` emitted + // `[{ name: 'syncBilling', handler: 'syncBilling', effect: 'writes' }]` into + // a schema that still demanded a callable — `invalid_union: Invalid input`, + // path stopping at `functions`, naming neither the entry nor the key. One + // widening covers BOTH array spellings at once, unlike the map form's two + // separate members: `effect` is already optional here, so the bare and the + // declared entry differ only in whether that key is present. z.array(z.object({ name: z.string(), - handler: z.function(), + handler: z.union([ + z.function(), + z.string().min(1).describe('The lowered handler ref (built artifacts) — the callable rides in the sibling ESM module'), + ]).describe('The function invoked by name — the authored callable, or the ref `objectstack build` lowered it to'), packageId: z.string().optional(), effect: FlowFunctionEffectSchema.optional(), })),