From 9ec2b22394019e38c49c97e4e285d50404d8f519 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 12:45:33 +0000 Subject: [PATCH] fix(cli): pass unrecognised `functions` entries through the lowering (#7318) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The map branch of the top-level `functions` lowering REBUILT the map rather than editing it: `out` admitted an entry only in the three shapes it knew — a bare callable, `{ handler: callable }`, or a plain string ref — and deleted everything else. No error, no warning, no key. Two failures came out of that. 1. Lowering stopped being IDEMPOTENT. `{ handler: 'syncBilling', effect: 'writes' }` — the shape this step itself emits for a declared writer, and the one `FlowFunctionLoweredDeclarationSchema` was added to accept in #4976 — matched none of the recognised shapes, so a second pass dropped the key and silently un-declared the writer the first pass had kept. Measured on `examples/app-showcase/dist/objectstack.json`: re-lowering it returned `{ summarizeCompletedTask: 'summarizeCompletedTask' }`, `sweepProjectHealth` gone, and the result still parsed green. 2. A MALFORMED entry was destroyed instead of reported. The headless husk `{ effect: 'writes' }` that a plain `JSON.stringify(stack)` leaves where a declaration was (#6293) left the lowering as `functions: {}` and the stack parsed green, so the build wrote an artifact missing the function rather than refusing — the evidence deleted before the parse could name it. Unrecognised entries now ride through under their own key, untouched, and `FlowFunctionEntrySchema` decides. The husk is refused where the build checks: `invalid_union` on `functions`, with `functions.sweep` in the branch tree that `formatZodErrors` (#5341) prints. The dedicated loop that re-added string entries is folded into the same pass. Bare callables, declared callables, pre-existing refs and the array form lower exactly as before. Tests pin both halves against the real pipeline: a second lowering of a lowered stack leaves the key set and the declarations unchanged (map and array), and the husk reaches the parse and is refused by key. `packages/qa/dogfood/test/build-shaped-artifact.ts` keeps its key-for-key reconciliation — it is now unreachable on this path but is still the only check that compares input `functions` keys against output ones, so it stays as the backstop against a producer that starts dropping again. Its measured claims and the showcase fixture's expectation are updated to the gate that now speaks. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RiF5oDWaCLK57mPuBsyB4t --- .../lower-callables-functions-passthrough.md | 38 ++++++ .../cli/src/utils/lower-callables.test.ts | 117 ++++++++++++++++++ packages/cli/src/utils/lower-callables.ts | 30 ++++- .../qa/dogfood/test/build-shaped-artifact.ts | 37 +++--- ...case-declarative-endpoints.dogfood.test.ts | 18 +-- 5 files changed, 215 insertions(+), 25 deletions(-) create mode 100644 .changeset/lower-callables-functions-passthrough.md diff --git a/.changeset/lower-callables-functions-passthrough.md b/.changeset/lower-callables-functions-passthrough.md new file mode 100644 index 0000000000..d39636b235 --- /dev/null +++ b/.changeset/lower-callables-functions-passthrough.md @@ -0,0 +1,38 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): stop `lowerCallables` deleting the `functions` entries it does not recognise (#7318) + +The map branch of the top-level `functions` lowering REBUILT the map instead of +editing it: `out` admitted an entry only in the three shapes it knew — a bare +callable, `{ handler: callable }`, or a plain string ref — and everything else +was dropped. No error, no warning, no key. Two distinct failures came out of +that one line. + +**A built artifact could not be lowered again.** The already-lowered declaration +`{ handler: 'syncBilling', effect: 'writes' }` — the shape this very step emits +for a declared writer, and the one `FlowFunctionLoweredDeclarationSchema` was +added to accept in #4976 — matched none of the recognised shapes. A second pass +therefore deleted the key outright, silently un-declaring the writer the first +pass had gone out of its way to keep. Lowering is now idempotent: lower a +lowered stack and the `functions` key set and the declared entries are +unchanged, in both the map and the array spelling. + +**A malformed entry was destroyed rather than reported.** The headless husk +`{ effect: 'writes' }` — a declaration for a function that is not there, which +is exactly what a plain `JSON.stringify(stack)` leaves where a declared writer +was (#6293) — reached the lowering and left it as `functions: {}`. The stack +then parsed GREEN, so `objectstack build` wrote an artifact missing the function +instead of refusing, and the evidence had been deleted before the parse could +name it. + +Unrecognised entries now ride through under their own key, untouched, and +`FlowFunctionEntrySchema` decides. The husk is refused where the build actually +checks — `invalid_union` on `functions`, with the offending key nameable in the +branch tree, which `formatZodErrors` (#5341) prints in the terminal. + +Nothing changes for a stack that was building correctly: bare callables, declared +callables, pre-existing string refs and the array form all lower exactly as +before. A stack that was silently shipping a `functions` map missing an entry now +fails its build, naming `functions` — which is the point. diff --git a/packages/cli/src/utils/lower-callables.test.ts b/packages/cli/src/utils/lower-callables.test.ts index 448bd78679..98aa25bca1 100644 --- a/packages/cli/src/utils/lower-callables.test.ts +++ b/packages/cli/src/utils/lower-callables.test.ts @@ -1,6 +1,7 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; +import type { z } from 'zod'; import { defineStack, normalizeStackInput, ObjectStackDefinitionSchema } from '@objectstack/spec'; import { FlowFunctionEntrySchema } from '@objectstack/spec/automation'; import { lowerCallables } from './lower-callables.js'; @@ -254,3 +255,119 @@ describe('lowerCallables → the spec parses what it emits (#4976, #6238)', () = .toEqual([{ name: 'syncBilling', handler: 'syncBilling', effect: 'writes' }]); }); }); + +// ── #7318: the `functions` map branch is a lowering, not a filter ─────────── +// +// The map branch REBUILT the map from the shapes it recognised, so anything +// else was deleted before the parse could see it. Two failures came out of that +// one line, and both are pinned here: +// +// 1. Lowering stopped being IDEMPOTENT. The already-lowered declaration +// `{ handler: 'syncBilling', effect: 'writes' }` — which #4976 taught +// `FlowFunctionEntrySchema` to accept, and which is exactly what the first +// pass emits — matched none of the recognised shapes, so a second pass +// dropped the key entirely and silently un-declared the writer the first +// pass had gone out of its way to keep. +// 2. A MALFORMED entry was destroyed rather than reported. The headless husk +// `{ effect: 'writes' }` (what a plain `JSON.stringify(stack)` leaves +// where a declaration was, #6293) left the lowering as `functions: {}` and +// the stack then parsed GREEN — the build writing an artifact missing the +// function instead of refusing. +describe('lowerCallables — unrecognised `functions` entries reach the parse (#7318)', () => { + const base = { + manifest: { id: 'com.example.demo', name: 'demo', version: '1.0.0', type: 'app' as const }, + }; + + /** `objectstack compile`'s first three steps, then `JSON.stringify` — the artifact. */ + const buildArtifact = (functions: unknown) => { + const stack = defineStack({ ...base, functions } as never); + const { lowered } = lowerCallables(normalizeStackInput(stack as Record)); + return JSON.parse(JSON.stringify(lowered)) as Record; + }; + + const functionsOf = (stack: Record) => + stack.functions as Record; + + /** Every `path` in a Zod error, including the branches folded inside a union. */ + const allIssuePaths = (issues: readonly z.core.$ZodIssue[], prefix: PropertyKey[] = []): string[] => + issues.flatMap((issue) => { + const path = [...prefix, ...issue.path]; + const nested = 'errors' in issue && Array.isArray(issue.errors) + ? (issue.errors as z.core.$ZodIssue[][]).flatMap((branch) => allIssuePaths(branch, path)) + : []; + return [path.join('.'), ...nested]; + }); + + it('lowering a lowered stack changes nothing — same keys, same declarations', () => { + // The artifact carries BOTH lowered shapes: a bare ref and a lowered + // declaration. Neither may move, and no key may go missing. + const once = buildArtifact({ + scoreLead: () => ({ score: 1 }), + syncBilling: { handler: () => ({ ok: true }), effect: 'writes' }, + }); + expect(functionsOf(once)).toEqual({ + scoreLead: 'scoreLead', + syncBilling: { handler: 'syncBilling', effect: 'writes' }, + }); + + const second = lowerCallables(once); + + expect( + Object.keys(functionsOf(second.lowered)).sort(), + 'the key set of an already-lowered `functions` map must survive a second pass', + ).toEqual(Object.keys(functionsOf(once)).sort()); + expect(functionsOf(second.lowered)).toEqual(functionsOf(once)); + // Nothing was left to lower, so nothing was registered — a lowered artifact + // carries its callables in the sibling module, not here. + expect(second.count).toBe(0); + expect(ObjectStackDefinitionSchema.safeParse(second.lowered).success).toBe(true); + }); + + it('is idempotent for the ARRAY form too', () => { + const once = buildArtifact([{ name: 'syncBilling', handler: () => ({ ok: true }), effect: 'writes' }]); + const second = lowerCallables(once); + expect(second.lowered.functions).toEqual(once.functions); + expect(second.count).toBe(0); + }); + + it('keeps a pre-existing bare string ref under its own key (legacy bundles)', () => { + const { lowered } = lowerCallables({ functions: { legacy: 'legacy' } }); + expect(lowered.functions).toEqual({ legacy: 'legacy' }); + }); + + it('passes the headless husk through, so the parse refuses it by key', () => { + // The card's measured case. `{ sweep: { effect: 'writes' } }` is what + // `JSON.stringify` leaves of a declared writer — a declaration for a + // function that is not there. + const husk = { sweep: { effect: 'writes' } }; + const { lowered, count } = lowerCallables({ ...base, functions: husk }); + + expect( + lowered.functions, + 'the husk must reach the artifact intact — deleting it here is what made the bad build green', + ).toEqual(husk); + expect(count).toBe(0); + + // Refused at the entry… + const entry = FlowFunctionEntrySchema.safeParse(husk.sweep); + expect(entry.success).toBe(false); + expect(entry.success ? [] : entry.error.issues.map((i) => i.code)).toContain('invalid_union'); + + // …and refused by the whole-stack parse the build actually runs, with the + // offending key nameable in the tree rather than an `invalid_union` that + // stops at `functions`. + const result = ObjectStackDefinitionSchema.safeParse(lowered); + expect(result.success, 'a stack whose `functions` map holds a husk must NOT parse green').toBe(false); + const paths = result.success ? [] : allIssuePaths(result.error.issues); + expect(paths).toContain('functions'); + expect(paths, 'the rejection must name the key it is about').toContain('functions.sweep'); + }); + + it('refuses a declaration whose `handler` is neither callable nor a ref', () => { + // Same rule, the other way a declaration goes wrong: the key is kept and + // the schema gets to name it. + const { lowered } = lowerCallables({ ...base, functions: { sweep: { handler: 42, effect: 'writes' } } }); + expect(lowered.functions).toEqual({ sweep: { handler: 42, effect: 'writes' } }); + expect(ObjectStackDefinitionSchema.safeParse(lowered).success).toBe(false); + }); +}); diff --git a/packages/cli/src/utils/lower-callables.ts b/packages/cli/src/utils/lower-callables.ts index b9b8a91f1d..8d96f8e043 100644 --- a/packages/cli/src/utils/lower-callables.ts +++ b/packages/cli/src/utils/lower-callables.ts @@ -165,12 +165,34 @@ export function lowerCallables(input: Record): LoweringResult { taken.add(ref); functions[ref] = value.handler as AnyFn; out[ref] = { ...value, handler: ref }; + } else { + // NOTHING ELSE IS THIS STEP'S TO JUDGE (#7318). Everything that is not + // a callable to lower rides through under its own key, untouched, and + // `FlowFunctionEntrySchema` decides whether it is legal. + // + // Two kinds of value arrive here, and passing both through is the same + // decision, not a compromise between two: + // + // ALREADY LOWERED — a bare ref (`'scoreLead'`, #4343) or a lowered + // declaration (`{ handler: 'scoreLead', effect: 'writes' }`, #4976). + // Both are shapes the schema accepts, so lowering a lowered stack + // must be a no-op: same key set, same declarations. Rebuilding the + // map around a fixed list of recognised shapes made that false — the + // lowered declaration matched none of them and was deleted, so a + // second pass (a re-lowered artifact, a fixture that lowers what it + // read back) silently un-declared the writer the FIRST pass had + // carefully kept. + // + // MALFORMED — the headless husk `{ effect: 'writes' }` that a plain + // `JSON.stringify(stack)` leaves where a declaration was (#6293). + // Deleting it here erased the evidence BEFORE the parse: the artifact + // came out `functions: {}` and validated green, so the build shipped + // an app missing the function instead of refusing. Handed on, it + // reaches `FlowFunctionEntrySchema`, which names it — `invalid_union` + // on this key — and `objectstack build` fails where it should. + out[key] = value; } } - // Preserve any pre-existing string entries (legacy bundles). - for (const [key, value] of Object.entries(fnsField)) { - if (typeof value === 'string') out[key] = value; - } (lowered as Record).functions = out; } diff --git a/packages/qa/dogfood/test/build-shaped-artifact.ts b/packages/qa/dogfood/test/build-shaped-artifact.ts index 671c4188ad..ea08c44ab1 100644 --- a/packages/qa/dogfood/test/build-shaped-artifact.ts +++ b/packages/qa/dogfood/test/build-shaped-artifact.ts @@ -20,12 +20,13 @@ // The declared entry made a noise ONCE, on the path where the residue is fed // straight to the parse: `FlowFunctionEntrySchema` refuses an entry declaring an // effect for a function it does not carry, and that red CI job is the only -// reason anybody learned about this (#4976). Measured here, it is not a general -// guarantee — put the same husk back through the lowering and it never reaches -// the schema at all (see the key-for-key check below). The BARE entry never made -// a noise on any path: it vanishes key and all, the artifact holds -// `functions: {}`, and the fixture parses green carrying zero of what it -// advertises. +// reason anybody learned about this (#4976). Putting the same husk back through +// the lowering used to silence even that — the map branch deleted the entry +// before the schema saw it — until #7318 taught the lowering to hand an +// unrecognised entry on unchanged, so the refusal now happens on both paths. +// The BARE entry still makes no noise anywhere: it vanishes key and all before +// this module is ever called, the artifact holds `functions: {}`, and the +// fixture parses green carrying zero of what it advertises. // `showcase-declarative-endpoints.dogfood.test.ts` shipped exactly that for its // whole existence. AGENTS.md, "Absence must be loud": a verifier that silently // degrades is worse than no verifier. @@ -174,14 +175,22 @@ export function buildShapedArtifact(stack: Record): BuildShaped ); } - // Key-for-key on the `functions` MAP, which the lowering rebuilds rather than - // edits: its `out` object admits an entry only in the three shapes it knows - // (a callable, `{ handler: callable }`, a string ref), and anything else is - // dropped — no error, no warning, no key. Measured on this exact stack: hand - // the lowering the `{ effect: 'writes' }` husk `JSON.stringify` leaves behind - // and the artifact comes out with `functions: {}`, parsing green, which is the - // #6293 failure wearing a different hat. The parse below cannot see it: by the - // time it runs, the evidence has been deleted. + // Key-for-key on the `functions` MAP. This was the live gate until #7318: the + // lowering rebuilt the map and admitted an entry only in the three shapes it + // knew (a callable, `{ handler: callable }`, a string ref), dropping anything + // else — no error, no warning, no key. Measured on this exact stack then: + // hand the lowering the `{ effect: 'writes' }` husk `JSON.stringify` leaves + // behind and the artifact came out `functions: {}`, parsing green, which is + // the #6293 failure wearing a different hat; the parse below could not see it, + // because by the time it ran the evidence had been deleted. + // + // The producer was fixed at the source — an entry `lowerCallables` does not + // recognise now rides through under its own key and the parse below refuses + // it by name — so this check no longer has anything to catch on that path. + // It is KEPT as the backstop it always was: it is the only assertion that + // reconciles the input's `functions` keys against the output's, so a future + // lowering that starts deleting again fails here, named, instead of shrinking + // this stand-in in silence. const inputFns = normalized.functions; if (isPlainObject(inputFns)) { const kept = new Set(Object.keys((lowering.lowered.functions ?? {}) as Record)); diff --git a/packages/qa/dogfood/test/showcase-declarative-endpoints.dogfood.test.ts b/packages/qa/dogfood/test/showcase-declarative-endpoints.dogfood.test.ts index 1a051c6f66..7b0c108957 100644 --- a/packages/qa/dogfood/test/showcase-declarative-endpoints.dogfood.test.ts +++ b/packages/qa/dogfood/test/showcase-declarative-endpoints.dogfood.test.ts @@ -194,14 +194,18 @@ describe('[#6293] the stand-in artifact carries what a built one carries', () => it('REFUSES to build an artifact out of that residue instead of quietly shrinking', () => { // The reverse verification, kept in the suite rather than done once by hand: // feed the helper the very thing this fixture used to write and it must - // fail, loudly, naming what went missing. Direction predicted before it was - // run — and the mechanism is NOT the one #4976 documented. The schema never - // sees the husk: `lowerCallables` rebuilds the `functions` map from the three - // shapes it recognises and deletes everything else, so the residue would have - // reached the parse as `functions: {}` and passed. The gate that speaks here - // is the helper's own key-for-key reconciliation. + // fail, loudly. Which gate speaks CHANGED in #7318, and the new one is the + // mechanism #4976 documented: `lowerCallables` used to rebuild the + // `functions` map from the shapes it recognised and delete everything else, + // so the residue reached the parse as `functions: {}` and passed — only the + // helper's own key-for-key reconciliation caught it. The lowering now hands + // an unrecognised entry ON, so the husk reaches `FlowFunctionEntrySchema` + // and the SPEC refuses it, here and in `objectstack build` alike. The + // reconciliation stays as the backstop for a producer that starts dropping + // again. const residue = JSON.parse(JSON.stringify(showcaseStack)) as Record; - expect(() => buildShapedArtifact(residue)).toThrowError(/dropped 1 `functions` entr.*sweepProjectHealth/s); + expect(() => buildShapedArtifact(residue)) + .toThrowError(/does not satisfy ObjectStackDefinitionSchema[\s\S]*functions/); }); });