From b5ca58cec2d575c096d7d2aabf0b252c5cd88420 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 05:18:02 +0000 Subject: [PATCH] fix(automation): resolve same-named flow definitions deterministically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A runtime-authored flow reusing a packaged flow's name silently and non-deterministically replaced it. The registry coexists both by design (ADR-0048 §3.4) and listItems returns both with no precedence, while the engine keys flows by bare name — so the boot pull registered both under one key and Map iteration order decided the survivor. Apply the ADR-0005 overlay precedence ADR-0048 §3.4 routes this case to (runtime overlay wins over the packaged artifact), warn per colliding name, and leave an admin-visible receipt for the shadowed definition. Fixes #11997 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 --- .changeset/wild-pears-remain.md | 34 +++ packages/objectql/src/index.ts | 9 + packages/objectql/src/registry.ts | 2 +- .../services/service-automation/src/engine.ts | 80 +++++++ .../src/flow-name-shadowing.test.ts | 221 ++++++++++++++++++ .../service-automation/src/flow-precedence.ts | 173 ++++++++++++++ .../services/service-automation/src/index.ts | 13 ++ .../services/service-automation/src/plugin.ts | 50 +++- 8 files changed, 576 insertions(+), 6 deletions(-) create mode 100644 .changeset/wild-pears-remain.md create mode 100644 packages/services/service-automation/src/flow-name-shadowing.test.ts create mode 100644 packages/services/service-automation/src/flow-precedence.ts diff --git a/.changeset/wild-pears-remain.md b/.changeset/wild-pears-remain.md new file mode 100644 index 0000000000..dc16c2aae3 --- /dev/null +++ b/.changeset/wild-pears-remain.md @@ -0,0 +1,34 @@ +--- +"@objectstack/service-automation": patch +"@objectstack/objectql": patch +--- + +Arm a deterministic flow when a runtime-authored flow reuses a packaged flow's name + +A runtime-authored flow that reused a packaged flow's name silently replaced it, +and which of the two ended up armed depended on registration order. The metadata +registry keys items `packageId:name` and deliberately coexists both (ADR-0048 +§3.4), `listItems('flow')` returns both with no precedence, and the automation +engine keys flows by bare name — so the boot pull registered both under one key +and Map iteration order picked the survivor. Measured: registering the package +first armed the runtime flow, registering the runtime row first armed the +packaged flow, with no warning and no way to tell which had won. + +The boot pull now collapses same-named definitions before anything is armed, +applying the ADR-0005 overlay precedence ADR-0048 §3.4 routes this case to: the +runtime/DB overlay wins over the packaged artifact, which is the sanctioned +override path. Two packages shipping one bare name resolve by package id, so +boot order no longer decides anything. + +Collisions are no longer silent. The pull warns once per colliding name — naming +the name, every contender, and which one is armed — and repeats it at bootstrap +beside the other automation audits. `getShadowedFlows()` is a new receipt listing +each contested name with its armed and shadowed definitions, and +`getFlowRuntimeStates()` rows now carry `armedFrom`/`shadowed` for contested +names; previously the displaced definition was invisible by construction, since +the flow map holds one entry per name. The `Pulled N flow(s)` line now counts +distinct names rather than registrations. + +`isCodeArtifactBody` is exported from `@objectstack/objectql` so consumers that +collapse same-named metadata answer "does a code package ship this?" with the +registry's own test instead of re-deriving it from `_packageId`. diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 7e8e0569db..d2dfaf10f0 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -26,6 +26,15 @@ export { DEFAULT_EXTENDER_PRIORITY, } from './registry.js'; export type { ObjectContributor, SchemaRegistryOptions } from './registry.js'; +// [#11997] The canonical "does a code package ship this body?" test (ADR-0029 +// D9.6). Exported because the ADR-0005 overlay precedence is not the registry's +// alone to apply: any consumer that collapses two same-named contenders into one +// slot — the automation engine's flow map is the first — has to answer the SAME +// question, and its whole reason for existing is that callers must not drift +// into a second answer. Consumers ask this; they do not re-derive it from +// `_packageId`, which cannot tell a tenant overlay from a code artifact on its +// own (see the function's own doc, and `isTenantAuthored` above it). +export { isCodeArtifactBody } from './registry.js'; // [#7865] Injected-column provenance — the machine-readable marker for anchors // the registry registers without provisioning storage (external objects, // ADR-0015). Canonical home: `@objectstack/metadata-core`, beside the diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index 705ff36fbf..dc3be3c729 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -1132,7 +1132,7 @@ function isTenantAuthored(item: unknown): boolean { * Truthy `_packageId`, not the `'sys_metadata'` rehydration sentinel, and not * tenant provenance. */ -function isCodeArtifactBody(item: unknown): boolean { +export function isCodeArtifactBody(item: unknown): boolean { const it = item as { _packageId?: unknown } | null | undefined; if (!it || !it._packageId || it._packageId === 'sys_metadata') return false; return !isTenantAuthored(it); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index eba76b924f..d7fc50d3cf 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -1240,6 +1240,34 @@ function graftConditionEnvelopes(converted: unknown, parsed: unknown): unknown { return converted; } +/** + * [#11997] Where one flow body came from, as the boot pull could tell. + * + * `'package'` — a code-shipped artifact (`isCodeArtifactBody`, ADR-0029 D9.6). + * `'runtime'` — a `sys_metadata`/runtime-authored row with no real package + * provenance: the ADR-0005 overlay, and the sanctioned override path per + * ADR-0048 §1.5. + */ +export interface FlowContender { + source: 'package' | 'runtime'; + /** The owning package id, when a code package ships this body. */ + packageId?: string; +} + +/** + * [#11997] What the ADR-0005 overlay precedence decided for one bare flow name. + * + * Emitted only when a name had more than one contender at pull time. `armed` is + * the body that is actually in the engine's flow map and will dispatch; + * `shadowed` are the ones that lost, in the order the registry listed them. + */ +export interface FlowShadowingRecord { + /** The bare name every contender claimed. */ + name: string; + armed: FlowContender; + shadowed: FlowContender[]; +} + export class AutomationEngine implements IAutomationService { /** * ADR-0044: maximum times a single node may be (re-)entered at the top @@ -1250,6 +1278,22 @@ export class AutomationEngine implements IAutomationService { static readonly MAX_NODE_REENTRIES = 100; private flows = new Map(); + /** + * [#11997] Shadowing receipts, keyed by the bare flow name. + * + * `flows` is keyed by BARE name and stays that way — making it + * package-aware is a much larger change than this defect warrants, and + * ADR-0048 does not ask for it. The consequence is that when a packaged + * flow and a runtime-authored flow claim one name, the loser leaves no + * trace in `flows`: `listFlows`/`getFlowRuntimeStates` enumerate + * `flows.keys()`, so the shadowed contender is invisible BY CONSTRUCTION, + * and an admin cannot tell which of the two is armed. + * + * This side map is the receipt. It never affects dispatch — it records + * what the ADR-0005 precedence decided, so {@link getShadowedFlows} and + * {@link getFlowRuntimeStates} can show it. + */ + private flowShadowing = new Map(); private flowEnabled = new Map(); /** * Re-entrancy guard for record-triggered flows (complements the intra-run @@ -2596,9 +2640,15 @@ export class AutomationEngine implements IAutomationService { status?: string; triggerType?: string; object?: string; + armedFrom?: FlowContender; + shadowed?: FlowContender[]; }> { return [...this.flows.keys()].map((name) => { const resolved = this.resolveTriggerBinding(name); + // [#11997] Attach the shadowing receipt to the row an admin already + // reads. This map holds ONE entry per bare name, so without these + // two fields a displaced contender leaves no trace on this surface. + const shadowing = this.flowShadowing.get(name); return { name, enabled: this.flowEnabled.get(name) !== false, @@ -2606,6 +2656,9 @@ export class AutomationEngine implements IAutomationService { status: (this.flows.get(name) as { status?: string } | undefined)?.status, triggerType: resolved?.triggerType, object: resolved?.binding.object, + ...(shadowing + ? { armedFrom: shadowing.armed, shadowed: shadowing.shadowed } + : {}), }; }); } @@ -2634,6 +2687,33 @@ export class AutomationEngine implements IAutomationService { return audit; } + /** + * [#11997] Record what the ADR-0005 overlay precedence decided for one bare + * name. Called by the boot pull when a name had more than one contender. + * + * Purely a receipt: it does not arm, disarm, or reorder anything. The pull + * has already registered the winner through the normal + * {@link registerFlow} path by the time this is called. + */ + recordFlowShadowing(record: FlowShadowingRecord): void { + if (!record.shadowed.length) return; + this.flowShadowing.set(record.name, record); + } + + /** + * [#11997] Admin-visible receipt: every bare flow name that had more than + * one contender at boot, which body is armed, and which were shadowed. + * + * Empty when no name collided — the normal case. Without this the shadowed + * flow is unobservable: {@link listFlows} and {@link getFlowRuntimeStates} + * both enumerate a map that holds one entry per name by construction, so a + * packaged flow displaced by a same-named runtime overlay simply is not + * there to be listed. + */ + getShadowedFlows(): FlowShadowingRecord[] { + return [...this.flowShadowing.values()]; + } + async listFlows(): Promise { return [...this.flows.keys()]; } diff --git a/packages/services/service-automation/src/flow-name-shadowing.test.ts b/packages/services/service-automation/src/flow-name-shadowing.test.ts new file mode 100644 index 0000000000..f4d868f779 --- /dev/null +++ b/packages/services/service-automation/src/flow-name-shadowing.test.ts @@ -0,0 +1,221 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#11997] A runtime-authored flow reusing a packaged flow's name must not +// silently and non-deterministically replace it. +// +// MEASURED BEFORE THE FIX (this is what these tests were written against): +// with a real `SchemaRegistry` and a real `AutomationEngine`, registering the +// package first armed the RUNTIME body, and registering the runtime row first +// armed the PACKAGED body. Same two definitions, opposite outcomes, decided by +// nothing but Map iteration order — and `listFlows()` returned exactly one +// name in BOTH cases, so nothing observable distinguished them. +// +// Worse, in the ONE ordering where `Registry.registerItem` does emit its +// `[Registry] Collision` warning (a runtime row exists, then a package ships +// the name), the warning promises "the runtime row will shadow the package +// value" while the engine armed the PACKAGED body — the warning was actively +// contradicted by the thing it warned about. +// +// The direction asserted here is NOT chosen by this test. ADR-0048 §1.5 lists +// the runtime/DB overlay as "the sanctioned override path" and §3.4 routes it +// to "the ADR-0005 overlay precedence"; ADR-0005 states that precedence as +// `sys_metadata … ← overlay (wins)` over `SchemaRegistry … ← artifact default`. +// Runtime wins. If that direction is ever revisited, this file is one of the +// places the decision has to be re-argued — do not flip it to match code. + +import { describe, it, expect, vi } from 'vitest'; +import { SchemaRegistry } from '@objectstack/objectql'; +import { AutomationEngine } from './engine.js'; +import { resolveFlowPrecedence, describeFlowContender } from './flow-precedence.js'; + +const FLOW = 'opportunity_approval'; +const silentLogger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } as any; + +function flowBody(name: string, marker: string) { + return { + name, + label: marker, + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: {} }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }; +} + +const packagedBody = () => ({ ...flowBody(FLOW, 'PACKAGED'), _packageId: 'crm' }); +const runtimeBody = () => ({ ...flowBody(FLOW, 'RUNTIME') }); + +/** A registry holding both contenders, registered in the given order. */ +function registryWithBoth(order: 'package-first' | 'runtime-first') { + const registry: any = new SchemaRegistry(); + if (order === 'package-first') { + registry.registerItem('flow', packagedBody(), 'name', 'crm'); + registry.registerItem('flow', runtimeBody(), 'name'); + } else { + registry.registerItem('flow', runtimeBody(), 'name'); + registry.registerItem('flow', packagedBody(), 'name', 'crm'); + } + return registry; +} + +/** + * The boot pull, reduced to the two steps under test: resolve precedence, then + * register the winners. Mirrors `plugin.ts`'s flow pull — see the comment there. + */ +function bootPull(registry: any, logger: { warn(m: string, meta?: unknown): void } = silentLogger) { + const engine = new AutomationEngine(silentLogger); + const listed = registry.listItems('flow') as unknown[]; + const resolved = resolveFlowPrecedence(listed, logger); + for (const entry of resolved) { + engine.registerFlow(entry.name, entry.definition as never); + if (entry.shadowing) engine.recordFlowShadowing(entry.shadowing); + } + return { engine, listed, resolved }; +} + +describe('#11997 — packaged flow shadowed by a same-named runtime flow', () => { + it('the registry still returns BOTH contenders (ADR-0048 §3.4 coexistence is untouched)', () => { + // The fix must NOT dedup in the registry: two entries under one bare + // name is deliberate there, and package-scoped getItem depends on it. + for (const order of ['package-first', 'runtime-first'] as const) { + const listed = registryWithBoth(order).listItems('flow') as any[]; + expect(listed).toHaveLength(2); + expect(listed.map((i) => i.label).sort()).toEqual(['PACKAGED', 'RUNTIME']); + } + }); + + it('arms the SAME definition regardless of registration order (Map order no longer decides)', async () => { + const first = bootPull(registryWithBoth('package-first')); + const second = bootPull(registryWithBoth('runtime-first')); + + const armedFirst = (await first.engine.getFlow(FLOW)) as any; + const armedSecond = (await second.engine.getFlow(FLOW)) as any; + + // Pre-fix these were 'RUNTIME' and 'PACKAGED' respectively. + expect(armedFirst?.label).toBe('RUNTIME'); + expect(armedSecond?.label).toBe('RUNTIME'); + expect(armedFirst?.label).toBe(armedSecond?.label); + }); + + it('arms the runtime overlay over the packaged artifact (ADR-0005 direction)', async () => { + const { engine } = bootPull(registryWithBoth('package-first')); + const armed = (await engine.getFlow(FLOW)) as any; + expect(armed?.label).toBe('RUNTIME'); + expect(armed?._packageId).toBeUndefined(); + }); + + it('registers ONE flow per bare name, not one per definition', async () => { + const { engine, listed, resolved } = bootPull(registryWithBoth('package-first')); + expect(listed).toHaveLength(2); // two candidates in + expect(resolved).toHaveLength(1); // one armed out + expect(await engine.listFlows()).toEqual([FLOW]); + }); + + it('warns loudly, naming the bare name, BOTH contenders, and which is armed', () => { + const warn = vi.fn(); + bootPull(registryWithBoth('package-first'), { warn }); + + expect(warn).toHaveBeenCalledTimes(1); + const [message, meta] = warn.mock.calls[0] as [string, any]; + + expect(message).toContain(FLOW); // the bare name + expect(message).toContain('package "crm"'); // contender A + expect(message).toContain('runtime-authored row'); // contender B + expect(message).toContain('arming a runtime-authored row'); // which one wins + expect(message).toContain('ADR-0005'); + // A single line: the boot diagnostic buffer keeps only the line + // carrying the level prefix (#5048). + expect(message).not.toContain('\n'); + + expect(meta.flow).toBe(FLOW); + expect(meta.armed).toEqual({ source: 'runtime' }); + expect(meta.shadowed).toEqual([{ source: 'package', packageId: 'crm' }]); + }); + + it('leaves an admin-visible receipt for the shadowed definition', () => { + const { engine } = bootPull(registryWithBoth('package-first')); + + // The dedicated audit, beside getTriggerBindingAudit. + expect(engine.getShadowedFlows()).toEqual([ + { + name: FLOW, + armed: { source: 'runtime' }, + shadowed: [{ source: 'package', packageId: 'crm' }], + }, + ]); + + // …and on the row an admin already reads. Without these two fields the + // displaced definition is invisible: this map holds one entry per name. + const states = engine.getFlowRuntimeStates(); + expect(states).toHaveLength(1); + expect(states[0].name).toBe(FLOW); + expect(states[0].armedFrom).toEqual({ source: 'runtime' }); + expect(states[0].shadowed).toEqual([{ source: 'package', packageId: 'crm' }]); + }); + + it('says nothing and attaches nothing when a name has only one definition', () => { + const warn = vi.fn(); + const registry: any = new SchemaRegistry(); + registry.registerItem('flow', packagedBody(), 'name', 'crm'); + const { engine, resolved } = bootPull(registry, { warn }); + + expect(warn).not.toHaveBeenCalled(); + expect(resolved).toHaveLength(1); + expect(engine.getShadowedFlows()).toEqual([]); + const [state] = engine.getFlowRuntimeStates(); + expect(state.armedFrom).toBeUndefined(); + expect(state.shadowed).toBeUndefined(); + }); +}); + +describe('#11997 — precedence is a total order, not an iteration order', () => { + it('two packages shipping one bare name resolve by packageId, either way round', () => { + const a = { ...flowBody(FLOW, 'FROM_ALPHA'), _packageId: 'alpha' }; + const b = { ...flowBody(FLOW, 'FROM_BETA'), _packageId: 'beta' }; + + const forward = resolveFlowPrecedence([a, b], silentLogger); + const backward = resolveFlowPrecedence([b, a], silentLogger); + + expect((forward[0].definition as any).label).toBe('FROM_ALPHA'); + expect((backward[0].definition as any).label).toBe('FROM_ALPHA'); + expect(forward[0].shadowing?.shadowed).toEqual([{ source: 'package', packageId: 'beta' }]); + }); + + it('preserves first-seen order for the non-colliding names around a collision', () => { + const before = { ...flowBody('aaa_first', 'A') }; + const after = { ...flowBody('zzz_last', 'Z') }; + const resolved = resolveFlowPrecedence( + [before, packagedBody(), after, runtimeBody()], + silentLogger, + ); + expect(resolved.map((r) => r.name)).toEqual(['aaa_first', FLOW, 'zzz_last']); + }); + + it('classifies the sys_metadata rehydration sentinel as runtime, not as a package', () => { + // `loadMetaFromDb` rehydrates overlay rows with a synthetic + // `_packageId = 'sys_metadata'` (ADR-0005 §Provenance edge case). A + // bare `_packageId` truthiness test would misread it as packaged. + expect(describeFlowContender({ name: FLOW, _packageId: 'sys_metadata' })).toEqual({ + source: 'runtime', + packageId: 'sys_metadata', + }); + // And a tenant-authored overlay bound to a REAL package id is runtime + // too — provenance is the axis, not the id (cloud#970). + expect( + describeFlowContender({ name: FLOW, _packageId: 'app.built_by_studio', _provenance: 'org' }), + ).toEqual({ source: 'runtime', packageId: 'app.built_by_studio' }); + expect(describeFlowContender({ name: FLOW, _packageId: 'crm' })).toEqual({ + source: 'package', + packageId: 'crm', + }); + }); + + it('a tenant overlay beats the package even when both carry a real package id', () => { + const packaged = { ...flowBody(FLOW, 'PACKAGED'), _packageId: 'crm' }; + const tenant = { ...flowBody(FLOW, 'TENANT'), _packageId: 'crm', _provenance: 'org' }; + const resolved = resolveFlowPrecedence([packaged, tenant], silentLogger); + expect((resolved[0].definition as any).label).toBe('TENANT'); + }); +}); diff --git a/packages/services/service-automation/src/flow-precedence.ts b/packages/services/service-automation/src/flow-precedence.ts new file mode 100644 index 0000000000..07ed8e6e53 --- /dev/null +++ b/packages/services/service-automation/src/flow-precedence.ts @@ -0,0 +1,173 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#11997] ADR-0005 overlay precedence for the automation boot flow pull. +// +// THE DEFECT THIS EXISTS FOR +// +// The SchemaRegistry keys metadata `:` and deliberately +// coexists a packaged item and a same-named runtime overlay (ADR-0048 §3.4). +// `listItems('flow')` returns BOTH, with no dedup and no precedence. The +// automation engine, however, keys flows by BARE name — so the boot pull used +// to register both under one key and whichever came last in Map iteration +// order won. The armed flow was decided by registry insertion order, i.e. boot +// load order, and nothing else. Measured before the fix: registering the +// package first arms the RUNTIME body; registering the runtime row first arms +// the PACKAGED body. Same inputs, different automation, no diagnostic. +// +// WHY RUNTIME WINS (this is not a choice made here) +// +// ADR-0048 §1.5 lists "Runtime / DB overlay (ADR-0005) — a `sys_metadata` row +// overlaying a packaged artifact" under what is NOT a collision: it is "the +// sanctioned override path". §3.4 then routes this exact case — "a write with +// no real package provenance" — to "the ADR-0005 overlay precedence +// (artifact-vs-DB warning, unchanged)". +// +// ADR-0005 states the direction twice: +// +// RUNTIME READ getMetaItem(type, name) +// 1. sys_metadata WHERE (type, name, project_id, state='active') ← overlay (wins) +// 2. SchemaRegistry / MetadataService ← artifact default +// +// and, in §"Collision warning": "the runtime overlay layer silently shadows +// the artifact value (correct ADR-0005 behavior)". `Registry.registerItem`'s +// own `[Registry] Collision` warning says the same in its message — "The +// runtime row will shadow the package value (ADR-0005 overlay precedence)". +// +// So: the runtime/DB overlay wins, the packaged artifact is the default it +// overlays. This module makes the engine agree with that, deterministically, +// instead of agreeing with whatever Map order happened to produce. Note that +// pre-fix the engine could actively CONTRADICT the registry warning: in the +// one order where `registerItem` warns (runtime row present, then the package +// ships the name), the engine armed the PACKAGED body — the opposite of what +// the warning had just promised. +// +// ⛔ NOT DONE HERE: making the engine's flow map package-aware. That is a much +// larger change, and ADR-0048 does not ask for it — the ADR's answer for this +// case is a precedence plus a warning, both of which are here. + +import { isCodeArtifactBody } from '@objectstack/objectql'; +import type { FlowContender, FlowShadowingRecord } from './engine.js'; + +/** One flow name's resolved winner, plus the receipt when it displaced others. */ +export interface FlowPrecedenceWinner { + name: string; + /** The body to register — the winner under ADR-0005 precedence. */ + definition: unknown; + /** Present only when this name had more than one contender. */ + shadowing?: FlowShadowingRecord; +} + +/** + * Classify one registry body's provenance. + * + * Delegates to `isCodeArtifactBody` — the canonical ADR-0029 D9.6 test, which + * exists precisely so callers cannot drift into a second answer to "does a code + * package ship this name?". ⛔ Do not re-derive this from `_packageId`: that + * sentinel test cannot tell a tenant-authored overlay from a code artifact, + * because a tenant overlay bound to a package carries a real package id too + * (see `isTenantAuthored` in objectql's registry.ts, and cloud#970). + */ +export function describeFlowContender(item: unknown): FlowContender { + const packageId = (item as { _packageId?: unknown } | null | undefined)?._packageId; + if (isCodeArtifactBody(item)) { + return { source: 'package', packageId: String(packageId) }; + } + return { + source: 'runtime', + ...(typeof packageId === 'string' && packageId ? { packageId } : {}), + }; +} + +/** + * Rank one contender for a bare name. LOWER wins. + * + * `runtime` (0) beats `package` (1) — the ADR-0005 direction quoted at the top + * of this file. + */ +function precedenceRank(contender: FlowContender): number { + return contender.source === 'runtime' ? 0 : 1; +} + +/** + * Collapse the registry's flow list to one body per bare name, deterministically. + * + * The returned order is the first-seen order of the names, so a registry with no + * collisions at all pulls in exactly the order it always did. Only names with + * more than one contender are reordered — and those by a TOTAL order that does + * not read registry iteration order at all: + * + * 1. `runtime` before `package` (ADR-0005 overlay precedence); + * 2. within `package`, lexicographic `packageId`. + * + * Rule 2 covers the ADR-0048 §3.4 case of two packages legitimately shipping one + * bare name. Package-scoped resolution disambiguates them properly for callers + * that can express it; the engine's bare-name flow map cannot, so it needs SOME + * deterministic answer, and a sorted package id is one that does not change when + * boot order does. That case is warned about too — it is exactly as invisible as + * the artifact-vs-DB one. + * + * @param items whatever `registry.listItems('flow')` returned + * @param logger warned once per colliding name, naming both contenders + */ +export function resolveFlowPrecedence( + items: readonly unknown[], + logger?: { warn(message: string, meta?: unknown): void }, +): FlowPrecedenceWinner[] { + // Group by bare name, remembering arrival order for a stable tie-break. + const groups = new Map>(); + const order: string[] = []; + items.forEach((item, index) => { + const name = (item as { name?: unknown } | null | undefined)?.name; + if (typeof name !== 'string' || !name) return; + let group = groups.get(name); + if (!group) { + group = []; + groups.set(name, group); + order.push(name); + } + group.push({ definition: item, contender: describeFlowContender(item), index }); + }); + + const winners: FlowPrecedenceWinner[] = []; + for (const name of order) { + const group = groups.get(name)!; + if (group.length === 1) { + winners.push({ name, definition: group[0].definition }); + continue; + } + + const ranked = [...group].sort((a, b) => { + const byRank = precedenceRank(a.contender) - precedenceRank(b.contender); + if (byRank !== 0) return byRank; + const byPackage = (a.contender.packageId ?? '').localeCompare(b.contender.packageId ?? ''); + if (byPackage !== 0) return byPackage; + // Fully-tied bodies: keep arrival order so the result is still total. + return a.index - b.index; + }); + + const armed = ranked[0]; + const shadowed = ranked.slice(1).map((entry) => entry.contender); + const describe = (c: FlowContender) => + c.source === 'package' ? `package "${c.packageId}"` : 'a runtime-authored row (sys_metadata)'; + + logger?.warn( + `[Automation] Flow name collision: '${name}' is claimed by ${group.length} definitions ` + + `(${ranked.map((entry) => describe(entry.contender)).join(', ')}); ` + + `arming ${describe(armed.contender)} per ADR-0005 overlay precedence and shadowing ` + + `${shadowed.length} other definition(s). Only the armed definition dispatches. ` + + `Rename one, or remove the sys_metadata row if the package value should win.`, + { + flow: name, + armed: armed.contender, + shadowed, + }, + ); + + winners.push({ + name, + definition: armed.definition, + shadowing: { name, armed: armed.contender, shadowed }, + }); + } + return winners; +} diff --git a/packages/services/service-automation/src/index.ts b/packages/services/service-automation/src/index.ts index 43d4a20bc9..ce24bcf923 100644 --- a/packages/services/service-automation/src/index.ts +++ b/packages/services/service-automation/src/index.ts @@ -22,8 +22,21 @@ export type { RunRecord, StepLogEntry, UnknownNodeTypeAuditEntry, + // [#11997] The shadowing receipt: which body is armed for a bare flow name, + // and which same-named definitions it displaced. Exported so a host building + // an admin surface reads the platform's own answer rather than re-deriving + // one it cannot derive — the shadowed definition is not in the flow map. + FlowContender, + FlowShadowingRecord, } from './engine.js'; +// [#11997] ADR-0005 overlay precedence for same-named flow definitions. The boot +// pull applies this; exported so a host that assembles its own flow list (or a +// test) collapses contenders the same deterministic way instead of inventing a +// second precedence. +export { resolveFlowPrecedence, describeFlowContender } from './flow-precedence.js'; +export type { FlowPrecedenceWinner } from './flow-precedence.js'; + // Per-run summary (#4354): the fold that turns a run's step log into // "selected N, acted M, skipped K by ". Exported so a host building its // own observability surface (or a test asserting a sweep actually wrote diff --git a/packages/services/service-automation/src/plugin.ts b/packages/services/service-automation/src/plugin.ts index 516d07002a..321331bef7 100644 --- a/packages/services/service-automation/src/plugin.ts +++ b/packages/services/service-automation/src/plugin.ts @@ -15,6 +15,7 @@ import { stripReadDecorations } from '@objectstack/spec/kernel'; import { AutomationEngine } from './engine.js'; import type { RunSummaryLogLevel } from './engine.js'; import { describeThrownForLog, thrownMessageText } from './thrown-cause-diagnostics.js'; +import { resolveFlowPrecedence } from './flow-precedence.js'; import { installBuiltinNodes, rearmSuspendedWaitTimers } from './builtin/index.js'; import { resolveRunDataContext } from './runtime-identity.js'; import { SysAutomationRun } from './sys-automation-run.object.js'; @@ -869,13 +870,25 @@ export class AutomationServicePlugin implements Plugin { } const flows = ql?.registry?.listItems?.('flow') ?? []; ctx.logger.debug(`[Automation] flow pull: registry returned ${flows.length} flow(s)`); + // [#11997] Collapse same-named contenders BEFORE anything is armed. + // `listItems` returns a packaged flow and a same-named runtime + // overlay as two entries (ADR-0048 §3.4 coexistence, deliberate); + // the engine's flow map is keyed by bare name, so registering both + // used to let Map iteration order decide which one dispatches. + // resolveFlowPrecedence applies the ADR-0005 direction — runtime + // overlay wins over the packaged artifact — and warns per colliding + // name, which is the artifact-vs-DB warning ADR-0048 §3.4 routes to. + const resolved = resolveFlowPrecedence(flows, ctx.logger); + const shadowedNames = resolved.filter((entry) => entry.shadowing).length; let registered = 0; - for (const f of flows) { - const def = f as { name?: string }; + for (const entry of resolved) { + const def = entry.definition as { name?: string }; if (!def?.name) continue; try { - this.engine.registerFlow(def.name, def as never); - this.syncedFlowNames.add(def.name); + this.engine.registerFlow(entry.name, def as never); + this.syncedFlowNames.add(entry.name); + // The receipt, so the shadowed contender is observable at all. + if (entry.shadowing) this.engine.recordFlowShadowing(entry.shadowing); registered++; } catch (e) { // #5048 — the facts go in `meta`, never interpolated into the @@ -890,7 +903,17 @@ export class AutomationServicePlugin implements Plugin { } } if (registered > 0) { - ctx.logger.info(`[Automation] Pulled ${registered} flow(s) from ObjectQL registry`); + // [#11997] `registered` is now a count of DISTINCT names, because + // same-named contenders were collapsed above. Before the fix it + // counted registrations, so two definitions of one name read as + // "2 flows" while only one was ever armed. Name the shadowing + // here too, so the count and the collision agree in one place. + ctx.logger.info( + `[Automation] Pulled ${registered} flow(s) from ObjectQL registry` + + (shadowedNames > 0 + ? ` (${flows.length} candidate(s); ${shadowedNames} name(s) had shadowed definitions — see the collision warnings above)` + : ''), + ); } } catch (err) { ctx.logger.warn('[Automation] flow pull from ObjectQL registry failed', describeThrownForLog(err)); @@ -1021,6 +1044,23 @@ export class AutomationServicePlugin implements Plugin { `[Automation] flow '${entry.flowName}' declares a '${entry.triggerType}' trigger but is NOT bound — it will never auto-launch. ${entry.reason}`, ); } + // [#11997] Name every flow whose bare name had more than one + // definition. The pull already warned at the moment it resolved + // precedence; this repeats it at bootstrap, where the other + // silent-miss audits are read, because a shadowed flow leaves NO + // trace on any other surface — `flows` is keyed by bare name, so + // the loser is not in `listFlows()` or in `states` below. + for (const record of this.engine.getShadowedFlows()) { + const describe = (c: { source: string; packageId?: string }) => + c.source === 'package' ? `package '${c.packageId}'` : 'a runtime-authored row (sys_metadata)'; + ctx.logger.warn( + `[Automation] flow '${record.name}' is claimed by ${record.shadowed.length + 1} definitions — ` + + `${describe(record.armed)} is ARMED and ${record.shadowed.map(describe).join(', ')} ` + + `${record.shadowed.length === 1 ? 'is' : 'are'} shadowed (ADR-0005 overlay precedence). ` + + `Only the armed definition dispatches.`, + ); + } + const states = this.engine.getFlowRuntimeStates(); const drafts = states.filter((s) => s.enabled && (s.status ?? 'draft') === 'draft'); if (drafts.length > 0) {