diff --git a/.changeset/28893-flow-region-shape-at-validate-build.md b/.changeset/28893-flow-region-shape-at-validate-build.md new file mode 100644 index 00000000000..75a008f3a1e --- /dev/null +++ b/.changeset/28893-flow-region-shape-at-validate-build.md @@ -0,0 +1,14 @@ +--- +'@objectstack/cli': patch +--- + +fix(cli): `os validate` and `os build` refuse a flow whose structured region the automation engine refuses to register (ADR-0031, #28893) + +Clause-②: no (narrowing) +Metadata-protocol: none + +**BREAKING** accept-set narrowing at `os validate` and `os build` (`@objectstack/cli`): a stack carrying a flow whose `loop` body, `parallel` branches or `try_catch` region is not single-entry/single-exit or is cyclic, at any depth, or whose `parallel` has fewer than two branches, now fails validation and the build instead of passing and emitting an artifact. + +Both commands now call `findFlowRegionRefusals`, which hands each flow (each `packages[]` entry's flows, or the stack's own) to the automation engine's own registration verdict: `FlowSchema`, then the spec's `validateControlFlow`. A refused flow prints `Flow '' would be refused at registration: : ` (`VALIDATION_ERROR`, exit 1), and the build writes nothing to `dist/`. Before, both passed it, and the engine refused it at boot with an `[Automation] failed to register flow` warning while the server ran without the flow. What the engine registers is unchanged. The fix is the engine's own sentence, e.g. add one node every exit of the region leads to. + + diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 8f239dd841d..9a450afe6b2 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -83,6 +83,9 @@ import { findOverrideDeclarationRefusals } from '../utils/override-declarations. // `os validate` step 3a-iv runs, over the save and install doors' own judge // (`judgeExtensionFieldSuffix`). import { findExtensionFieldSuffixRefusals } from '../utils/extension-field-suffix.js'; +// [#28893] The ADR-0031 region-shape check — the walk `os validate` step 3a-vi +// runs, over the automation engine's own registration verdict. +import { findFlowRegionRefusals } from '../utils/flow-region-shape.js'; export default class Compile extends Command { static override description = 'Compile ObjectStack configuration to JSON artifact'; @@ -837,6 +840,29 @@ export default class Compile extends Command { this.exit(1); } + // 3b-iii-ter. [#28893] Flow region shape (ADR-0031), at build time — the + // SAME call `os validate` makes at its step 3a-vi: the engine's + // registration verdict (`FlowSchema`, then the spec's + // `validateControlFlow`). Before this the build emitted a flow the + // engine then refused at boot, logging a warning and running without + // it; now the door that ships refuses it and writes no artifact. Same + // two faces as the gates above. + const regionRefusals = findFlowRegionRefusals(result.data as Record); + if (regionRefusals.length > 0) { + if (flags.json) { + await emitJson({ success: false, errors: regionRefusals, warnings: warningsSoFar(), conversions: conversionNotices }, 0, { compact: true }); + this.exit(1); + } + const n = regionRefusals.length; + console.log(''); + printError(`The automation engine would refuse to register ${n} flow${n > 1 ? 's' : ''} for a malformed region (ADR-0031)`); + printBulletList( + regionRefusals.map((r) => `${r.path} [${r.code}] ${r.message}`), + { noun: 'flow region refusal(s)', remedy: JSON_FULL_LIST_REMEDY }, + ); + this.exit(1); + } + // 3b-quater. ADR-0141 D2, author side — every `manifest.dependencies` // range of the packages one install act would deliver, judged by the // install's own predicate (`utils/dependency-range-check.ts`). diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index 5d72a190a21..aa7682a7b0e 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -81,6 +81,9 @@ import { findExtensionFieldSuffixRefusals } from '../utils/extension-field-suffi // [ADR-0146 D10, second rule] The field-patch target check's walk; the verdict // is the marketplace install door's own judge. import { findFieldPatchTargetRefusals } from '../utils/field-patch-targets.js'; +// [#28893] The ADR-0031 region-shape check: the automation engine's own +// registration verdict (`FlowSchema` then the spec's `validateControlFlow`). +import { findFlowRegionRefusals } from '../utils/flow-region-shape.js'; // A field `picklist` that names no picklist the stack declares — refused, or // reported when the declaring package depends on packages outside the stack. // Walked the way the load path registers, like the refusal above. @@ -751,6 +754,36 @@ export default class Validate extends Command { this.exit(1); } + // 3a-vi. [#28893] Flow region shape (ADR-0031): every `loop` body, + // `parallel` branch and `try_catch` region, at every depth, must be + // single-entry/single-exit and acyclic, and a `parallel` needs two + // branches. The verdict is the engine's registration call + // (`FlowSchema`, then the spec's `validateControlFlow`), so a flow + // green here is one boot registers instead of logging + // `[Automation] failed to register flow` and running without it; see + // `utils/flow-region-shape.ts`. + const regionRefusals = findFlowRegionRefusals(result.data as Record); + if (regionRefusals.length > 0) { + if (flags.json) { + await emitJson({ + valid: false, + errors: regionRefusals, + warnings: warningsSoFar(), + conversions: conversionNotices, + duration: timer.elapsed(), + }); + this.exit(1); + } + const n = regionRefusals.length; + console.log(''); + printError(`The automation engine would refuse to register ${n} flow${n > 1 ? 's' : ''} for a malformed region (ADR-0031)`); + printBulletList( + regionRefusals.map((r) => `${r.path} [${r.code}] ${r.message}`), + { noun: 'flow region refusal(s)', remedy: JSON_FULL_LIST_REMEDY }, + ); + this.exit(1); + } + // 3b. [#3366] Installable-provider preflight — the shift-left of the // `serve`-time capability check. `os validate` previously only checked // the `requires` tokens against the vocabulary (ADR-0066), never diff --git a/packages/cli/src/utils/flow-region-shape.ts b/packages/cli/src/utils/flow-region-shape.ts new file mode 100644 index 00000000000..955c9e0b5e6 --- /dev/null +++ b/packages/cli/src/utils/flow-region-shape.ts @@ -0,0 +1,72 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #28893 — the ADR-0031 region-shape check at `os validate` and `os build`. + * + * The automation engine refuses to register a flow whose structured regions + * (`loop` bodies, `parallel` branches, `try_catch` try/catch regions, at every + * depth) are not single-entry/single-exit and acyclic, or whose `parallel` has + * fewer than two branches: `registerFlow` parses the body with `FlowSchema` + * and calls the spec's `validateControlFlow`. Before this module validate and + * build passed such a flow, and the first sign of it was a boot warning + * (`[Automation] failed to register flow`) with the flow silently absent. + * + * The verdict here is that same call — `FlowSchema` then `validateControlFlow` + * from `@objectstack/spec/automation` — so there is no second copy of the + * region rules, and its message (`: `) is the engine's, + * verbatim, behind the flow's name. Like the engine it stops at a flow's first + * malformed region; each flow is judged. + * + * Walked the way `findExtensionFieldSuffixRefusals` (`./extension-field-suffix.ts`) + * walks: each `packages[]` entry's flows, or the stack's own flows when it + * declares no packages. A flow the schema refuses is left to the schema step, + * which has already reported it. + * + * Not judged here: a cycle in the flow's TOP-LEVEL graph. The engine refuses it + * with `detectCycles`, which lives in `@objectstack/service-automation` and is + * not exported; region bodies ARE cycle-checked, by `validateControlFlow`. + */ + +import { FlowSchema, validateControlFlow } from '@objectstack/spec/automation'; + +import { artifactPackages } from './artifact-packages.js'; + +/** One refused flow, located. */ +export interface FlowRegionRefusalRow { + /** Where the flow sits in the parsed stack, e.g. `flows[0]` or `packages[1].manifest.flows[0]`. */ + path: string; + /** The code the CLI's other author-time refusals carry. */ + code: 'VALIDATION_ERROR'; + /** The flow's name. */ + flow: string; + /** `Flow '' …: ` followed by the engine's refusal, verbatim. */ + message: string; +} + +/** Every flow of the parsed stack the engine would refuse to register for its region shape. */ +export function findFlowRegionRefusals(parsed: Record): FlowRegionRefusalRow[] { + const entries = artifactPackages(parsed); + const subjects = entries.length > 0 + ? entries.map(({ index, body }) => ({ flows: body.flows, at: `packages[${index}].manifest.` })) + : [{ flows: parsed.flows, at: '' }]; + const rows: FlowRegionRefusalRow[] = []; + for (const { flows, at } of subjects) { + if (!Array.isArray(flows)) continue; + flows.forEach((raw, i) => { + const result = FlowSchema.safeParse(raw); + if (!result.success) return; + try { + validateControlFlow(result.data); + } catch (err) { + const name = result.data.name; + rows.push({ + path: `${at}flows[${i}]`, + code: 'VALIDATION_ERROR', + flow: name, + message: `Flow '${name}' would be refused at registration: ${err instanceof Error ? err.message : String(err)}`, + }); + } + }); + } + return rows; +} diff --git a/packages/cli/test/authoring-rule-command-parity.test.ts b/packages/cli/test/authoring-rule-command-parity.test.ts index 02cd06334b0..795a6b1c020 100644 --- a/packages/cli/test/authoring-rule-command-parity.test.ts +++ b/packages/cli/test/authoring-rule-command-parity.test.ts @@ -142,6 +142,53 @@ const CASES: ReadonlyArray<{ rule: string; blindTo: readonly AuthoringCommand[]; }, ]; +/** + * Checks `os validate` and `os build` gate on OUTSIDE the rule table, so + * `os lint`, which runs only the table, is blind to them. `blindTo` is the same + * documentation as above; the pin below asserts the blindness, so closing it + * (registering the check as an authoring rule) reds here and the case moves + * into `CASES`. + * + * - flow region shape (#28893): a `try_catch` region that is not single-exit. + * validate and build refuse it with the automation engine's own registration + * verdict (`utils/flow-region-shape.ts`, pinned by + * `validate-flow-region-shape.test.ts`); `os lint` passes it. + */ +const OUTSIDE_THE_TABLE: ReadonlyArray<{ check: string; marker: string; blindTo: readonly AuthoringCommand[]; stack: Record }> = [ + { + check: 'flow region shape (#28893)', + marker: 'single-exit', + blindTo: ['lint'], + stack: withBaseline({ + objects: [{ name: 'parity_task', label: 'Task', sharingModel: 'private', fields: { name: { type: 'text', label: 'Name' } } }], + flows: [{ + name: 'parity_region', label: 'Region', type: 'record_change', runAs: 'system', status: 'active', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { objectName: 'parity_task', triggerType: 'record-after-create' } }, + { + id: 'guard', type: 'try_catch', label: 'Guard', + config: { + try: { nodes: [{ id: 'work', type: 'assignment', label: 'Work' }], edges: [] }, + catch: { + nodes: ['d', 'x', 'y'].map((id) => ({ id, type: 'assignment', label: id })), + edges: [ + { id: 'dx', source: 'd', target: 'x', type: 'default' }, + { id: 'dy', source: 'd', target: 'y', type: 'default' }, + ], + }, + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'guard', type: 'default' }, + { id: 'e2', source: 'guard', target: 'end', type: 'default' }, + ], + }], + }), + }, +]; + describe('every authoring command reaches the same verdict (#4409)', () => { it.each(CASES.map((c) => [c.rule, c] as const))('%s gates on all three commands', (_rule, testCase) => { for (const command of AUTHORING_COMMANDS) { @@ -160,6 +207,16 @@ describe('every authoring command reaches the same verdict (#4409)', () => { } }); + it.each(OUTSIDE_THE_TABLE.map((c) => [c.check, c] as const))('%s: a known gap, os lint is still blind to it', (_check, testCase) => { + for (const command of testCase.blindTo) { + const findings = runAuthoringRules(command, { + normalized: testCase.stack, + ...(command === 'lint' ? {} : { parsed: testCase.stack }), + }); + expect(findings.filter((f) => f.message.includes(testCase.marker))).toEqual([]); + } + }); + it('the case table still covers every command as a blind spot', () => { // Guard the guard: if the table drifted to cover only one direction, the // suite above would keep passing while testing half the problem. The issue diff --git a/packages/cli/test/validate-build-gate-parity.test.ts b/packages/cli/test/validate-build-gate-parity.test.ts index 632d6647001..5587cb769c8 100644 --- a/packages/cli/test/validate-build-gate-parity.test.ts +++ b/packages/cli/test/validate-build-gate-parity.test.ts @@ -213,6 +213,15 @@ const SHARED_NON_REGISTRY_GATES: readonly string[] = [ // (#28727), so the row moved here — deleted there rather than reworded, for // the reason the `runPerPackageAuthoringRules` row above gives. 'findExtensionFieldSuffixRefusals', + // [#28893] Flow region shape (ADR-0031): every `loop` body, `parallel` + // branch and `try_catch` region single-entry/single-exit and acyclic, and a + // `parallel` with two branches, judged by the automation engine's own + // registration call (`FlowSchema`, then the spec's `validateControlFlow`). + // Wired into both commands in one card. Not a registry rule today, so + // `os lint` is blind to it: the parity test pins that gap + // (`authoring-rule-command-parity.test.ts`, `OUTSIDE_THE_TABLE`), and moving + // it into `authoring-rules.ts` is the reported follow-up. + 'findFlowRegionRefusals', ]; /** diff --git a/packages/cli/test/validate-flow-region-shape.test.ts b/packages/cli/test/validate-flow-region-shape.test.ts new file mode 100644 index 00000000000..848e29bf9b3 --- /dev/null +++ b/packages/cli/test/validate-flow-region-shape.test.ts @@ -0,0 +1,170 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#28893] `os validate` and `os build` refuse a flow the automation engine + * refuses at registration for its ADR-0031 region shape (`validateControlFlow`: + * a region that is not single-entry / single-exit or is cyclic, or a parallel + * with fewer than 2 branches), at every region depth, with the engine's own + * message and the flow's name. + * + * ## The defect, measured on `origin/main` (`ff881050`) before this file + * + * The duly shape — a `try_catch` whose catch region is a decision fanning out + * to three nodes with no join — exited 0 from `os validate --json` and built. + * The engine then refused it at `registerFlow` ("region must be single-exit"), + * logged at WARN, and the boot went on without the flow, so its trigger never + * fired. Both commands run in-process (`Command.run`), not as spawned CLIs. + */ + +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import Compile from '../src/commands/compile'; +import Validate from '../src/commands/validate'; +import { defineStackSource, linkSpec } from './helpers/define-stack-fixture.js'; + +const CLI_ROOT = resolve(fileURLToPath(import.meta.url), '..', '..'); + +interface Run { code: number; stdout: string } + +async function run(command: typeof Compile | typeof Validate, argv: string[]): Promise { + const chunks: string[] = []; + const spy = vi.spyOn(process.stdout, 'write').mockImplementation(((chunk: unknown, ...rest: unknown[]) => { + chunks.push(String(chunk)); + const done = rest.find((a) => typeof a === 'function') as ((e?: Error | null) => void) | undefined; + done?.(null); + return true; + }) as never); + const exitCodeBefore = process.exitCode; + let code = 0; + try { + await command.run(argv, { root: CLI_ROOT }); + } catch (error) { + const exit = (error as { oclif?: { exit?: unknown } }).oclif?.exit; + if (typeof exit !== 'number') throw error; + code = exit; + } finally { + spy.mockRestore(); + process.exitCode = exitCodeBefore; + } + // eslint-disable-next-line no-control-regex + return { code, stdout: chunks.join('').replace(/\u001b\[[0-9;]*m/g, '') }; +} + +const json = (r: Run, label: string): Record => { + try { + return JSON.parse(r.stdout) as Record; + } catch { + throw new Error(`${label}: stdout was not one JSON document (exit ${r.code})\n${r.stdout}`); + } +}; + +const OBJECT = 'rgn_visa'; +const n = (id: string, type = 'assignment', extra: Record = {}) => ({ id, type, label: id, ...extra }); +const e = (id: string, source: string, target: string) => ({ id, source, target, type: 'default' }); + +/** d → x, y, z: one entry, three exits — the duly catch region. With `join`, x, y, z → j closes it. */ +const fanOut = (join: boolean) => ({ + nodes: [n('d'), n('x'), n('y'), n('z'), ...(join ? [n('j')] : [])], + edges: [ + e('r1', 'd', 'x'), e('r2', 'd', 'y'), e('r3', 'd', 'z'), + ...(join ? [e('r4', 'x', 'j'), e('r5', 'y', 'j'), e('r6', 'z', 'j')] : []), + ], +}); + +/** A record-change flow: start → → end. */ +const flow = (name: string, middle: Record) => ({ + name, label: name, type: 'record_change', runAs: 'system', status: 'active', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { objectName: OBJECT, triggerType: 'record-after-create' } }, + middle, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [e('e1', 'start', String(middle.id)), e('e2', String(middle.id), 'end')], +}); + +const guard = (catchRegion: Record) => + n('guard', 'try_catch', { config: { try: { nodes: [n('work')], edges: [] }, catch: catchRegion } }); + +const stackWith = (flows: unknown[]) => ({ + manifest: { id: 'com.example.rgn', namespace: 'rgn', version: '1.0.0', name: 'Rgn', type: 'app', engines: { protocol: '^18' } }, + objects: [{ name: OBJECT, label: 'Visa', sharingModel: 'private', fields: { title: { type: 'text', label: 'Title' } } }], + flows, +}); + +const CASES: Record; refusal?: string }> = { + duly: { + stack: stackWith([flow('rgn_lead_time', guard(fanOut(false)))]), + refusal: "try_catch 'guard' catch: region must be single-exit but has 3: x, y, z", + }, + nested: { + stack: stackWith([flow('rgn_nested', n('outer', 'try_catch', { + config: { + try: { + nodes: [n('sweep', 'loop', { config: { collection: { dialect: 'cel', source: 'record.items' }, iteratorVariable: 'item', body: fanOut(false) } })], + edges: [], + }, + }, + }))]), + refusal: "try_catch 'outer' try → loop 'sweep' body: region must be single-exit but has 3: x, y, z", + }, + singleBranch: { + stack: stackWith([flow('rgn_fan', n('fan', 'parallel', { config: { branches: [{ nodes: [n('only')], edges: [] }] } }))]), + refusal: "parallel 'fan': a parallel block needs at least 2 branches", + }, + joined: { stack: stackWith([flow('rgn_lead_time', guard(fanOut(true)))]) }, +}; + +let root = ''; +const fixtures: Record = {}; + +beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'os-validate-flow-region-')); + for (const [label, c] of Object.entries(CASES)) { + const dir = join(root, label); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, 'objectstack.config.ts'), defineStackSource(c.stack, { strict: false })); + linkSpec(dir); + fixtures[label] = { config: join(dir, 'objectstack.config.ts'), dist: join(dir, 'dist'), artifact: join(dir, 'dist', 'objectstack.json') }; + } +}); + +afterAll(() => { + if (root) rmSync(root, { recursive: true, force: true }); +}); + +const validate = (label: string, ...flags: string[]) => run(Validate, [fixtures[label].config, ...flags]); +const build = (label: string, ...flags: string[]) => + run(Compile, [fixtures[label].config, '--output', fixtures[label].artifact, ...flags]); + +describe('[#28893] validate and build refuse a flow region the engine refuses, with its message', () => { + for (const label of ['duly', 'nested', 'singleBranch'] as const) { + it(`${label}: validate exits 1 naming the flow and the engine's refusal; build refuses the same and writes nothing`, async () => { + const { refusal } = CASES[label]; + const flowName = (CASES[label].stack.flows as Array<{ name: string }>)[0]!.name; + const validated = await validate(label, '--json'); + expect(validated.code, validated.stdout).toBe(1); + const errors = json(validated, `validate ${label}`).errors as Array>; + const row = errors.find((r) => String(r.message).includes(refusal!)); + expect(row, JSON.stringify(errors, null, 2)).toBeDefined(); + expect(String(row!.message)).toContain(flowName); + + const built = await build(label, '--json'); + expect(built.code).toBe(1); + const payload = json(built, `build ${label}`); + expect(payload.success).toBe(false); + expect((payload.errors as Array>).some((r) => String(r.message).includes(refusal!))).toBe(true); + expect(existsSync(fixtures[label].dist)).toBe(false); + }, 60_000); + } + + it('CONTROL: the same flow with the join restored validates and builds', async () => { + const validated = await validate('joined', '--json'); + expect(validated.code, validated.stdout).toBe(0); + const built = await build('joined', '--json'); + expect(json(built, 'build joined').success, built.stdout).toBe(true); + expect(built.code).toBe(0); + }, 60_000); +});