From ba3a391b1ec8875775cfc8f436c5e8ac8d05e126 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 18:26:18 +0000 Subject: [PATCH] fix(service-automation): one renderer for the contested-flow phrase (#12563) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One event — a flow name claimed by more than one definition — was described to an operator in three places, each with its own private `const describe` beside the log call. Nothing held them equal, and two axes had already drifted: `flow-precedence.ts` rendered `package "crm"` while the other two rendered `package 'crm'`, and the two engine copies interpolated a bare `undefined` where the CLI copy rendered a real fallback. `describeFlowContender` sat exactly where a shared renderer would live and read like one, but it is a CLASSIFIER with no prose in it — so each caller wrote its own sentence while an export that looked reusable sat beside them. That is the durable part of the defect: a missing renderer is a gap someone fills, a misleading one is a gap that keeps getting re-filled privately. The two copies in this package now call one exported `renderFlowContender`, and `describeFlowContender`'s own docs say what it is not. Both spellings were decided on measurement: - Single quotes, measured against this package rather than voted across the three copies: 203 single-quoted interpolations in operator prose under `service-automation/src` against 3 double-quoted, one of which was this phrase. The sentence already single-quotes the flow name beside it. - A named fallback instead of `package 'undefined'`. This package's callers cannot reach that branch today (`isCodeArtifactBody` is false on a falsy `_packageId`) — but that is a property of today's callers, not of an exported function. The third copy, in `@objectstack/cli`'s startup banner, deliberately keeps its own structural rendering: that package takes no static value import of `@objectstack/service-automation`, and its engine reads are feature-detected so a host on an older automation package still boots its banner. It is held equal by a test-only agreement pin instead, which is red in both directions. Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5 --- .changeset/flow-contender-one-renderer.md | 53 +++++++ .../format.flow-contender-agreement.test.ts | 143 ++++++++++++++++++ .../src/flow-name-shadowing.test.ts | 56 ++++++- .../service-automation/src/flow-precedence.ts | 52 ++++++- .../services/service-automation/src/index.ts | 2 +- .../services/service-automation/src/plugin.ts | 7 +- 6 files changed, 301 insertions(+), 12 deletions(-) create mode 100644 .changeset/flow-contender-one-renderer.md create mode 100644 packages/cli/src/utils/format.flow-contender-agreement.test.ts diff --git a/.changeset/flow-contender-one-renderer.md b/.changeset/flow-contender-one-renderer.md new file mode 100644 index 0000000000..bdf53d09df --- /dev/null +++ b/.changeset/flow-contender-one-renderer.md @@ -0,0 +1,53 @@ +--- +"@objectstack/service-automation": minor +--- + +fix(service-automation): one renderer for the contested-flow phrase, and the two spellings it had drifted into (#12563) + +`minor`, not `patch`, and not empty: this adds a new export +(`renderFlowContender`) to a published package's public API, and it changes +**shipped operator-facing log text**. Both are real changes a consumer can +observe. + +## What changed + +One event — a flow name claimed by more than one definition — was described to +an operator in three places, each with its own private `const describe` beside +the log call: `flow-precedence.ts`'s precedence warning, `plugin.ts`'s bootstrap +audit, and (in `@objectstack/cli`) the startup banner. Nothing held them equal, +and two axes had already drifted: + +- **Quoting.** `flow-precedence.ts` rendered `package "crm"`; the other two + rendered `package 'crm'`. +- **Absent package id.** The two engine copies interpolated a bare `undefined` + into the sentence; the CLI copy rendered a real fallback. + +The two copies in this package are now one exported renderer. The choice on +each axis was measured, not voted: + +- **Single quotes**, measured against this package rather than across the three + copies: of the interpolated identifiers in operator prose under + `service-automation/src`, 203 are single-quoted and 3 double-quoted — one of + those 3 being this phrase. The sentence already single-quotes the flow name + beside it. +- **A named fallback** (`a code-shipped package (id unknown)`) instead of + `package 'undefined'`. This package's own callers cannot reach that branch + today, because `isCodeArtifactBody` is false on a falsy `_packageId` — but + that is a property of today's callers, not of an exported function. + +## Log text a consumer may be matching on + +`[Automation] Flow name collision: …` (the precedence warning) now renders a +packaged contender as `package 'crm'` rather than `package "crm"`. +`plugin.ts`'s bootstrap `[Automation] flow '' is claimed by …` warning is +byte-identical to before for every input its callers can produce; only its +unreachable absent-id branch changed. + +## Why the CLI still renders its own + +`@objectstack/cli` deliberately keeps its own spelling and takes no value +import of this package for the banner: its engine reads are structural and +feature-detected so a host on an older automation package still boots. The +third copy is held equal by a test-only agreement pin +(`packages/cli/src/utils/format.flow-contender-agreement.test.ts`) that asserts +the banner line through this renderer, so it goes red in both directions. diff --git a/packages/cli/src/utils/format.flow-contender-agreement.test.ts b/packages/cli/src/utils/format.flow-contender-agreement.test.ts new file mode 100644 index 0000000000..0083107573 --- /dev/null +++ b/packages/cli/src/utils/format.flow-contender-agreement.test.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#12563] THE CLI's COPY OF THE CONTESTED-FLOW PHRASE, HELD EQUAL TO THE +// ENGINE's — by agreement, not by a shared runtime import. +// +// ## What was wrong +// +// One event — a flow name claimed by more than one definition — was described +// to an operator in three places, each with its own private `const describe`: +// twice inside `@objectstack/service-automation` and once here, in the startup +// banner. Nothing held them equal, and two axes had ALREADY drifted before +// anyone noticed: the engine's precedence warning double-quoted the package id +// while the other two single-quoted it, and the two engine copies interpolated +// a bare `undefined` where this one renders a real fallback. +// +// The two engine copies are now one exported renderer, `renderFlowContender`. +// This file is what holds the THIRD copy — the one in this package — to it. +// +// ## ⛔ Why this is a TEST-only import and `format.ts` still renders its own +// +// The obvious fix is for `format.ts` to import the renderer and call it. It is +// the wrong fix here, and the reason is measured rather than stylistic: +// +// - `packages/cli` takes ZERO static value imports of +// `@objectstack/service-automation` today. Its only value import of that +// package is a DYNAMIC `await import()` in `utils/data-migration-plugins.ts`, +// deferred behind `opts.automation === true`. +// - `collectAutomationSummary` reads the engine STRUCTURALLY and feature- +// detects every probe (#12028/#12562) precisely so a host running an OLDER +// automation package still boots its banner. A value import states a +// guarantee that runtime deliberately does not make. +// - `utils/format.ts` is imported by ~56 command modules in this package. A +// static import here would pull the whole automation package into the +// module graph of `os whoami`, `os init`, `os login` — every command. +// +// A TEST is not the shipped runtime path, and `@objectstack/service-automation` +// is already a workspace dependency, so importing the renderer HERE costs none +// of that. The banner keeps its own spelling; this file is what makes the two +// spellings a fact rather than a coincidence. +// +// ⚠️ This specifier resolves through the package's `exports` to its **dist**, +// not its source — it is already registered for this package in +// `KNOWN_UNALIASED_TEST_IMPORTS['@objectstack/cli']` (that shrink-only ledger is +// NOT widened by this file; the specifier was already reachable via the dynamic +// import named above). `turbo.json` declares `@objectstack/cli#test` +// `dependsOn: ["build"]`, so CI builds it first. Locally: build +// `@objectstack/service-automation` before running this, or the named import +// below fails to link and says so. +// +// ⛔ Deliberately NOT aliased to source in `vitest.config.ts`. Aliasing a dep to +// source imports that dep's ENTIRE import surface into this package's +// resolution domain (see `scripts/check-test-source-alias.mjs` and this +// package's vitest config header) — a config change reaching all ~185 test +// files here, to buy staleness-resistance on one three-branch pure function. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { renderFlowContender } from '@objectstack/service-automation'; +import { printServerReady, type ServerReadyOptions, type AutomationReadySummary } from './format.js'; + +type Contender = { source: 'package' | 'runtime'; packageId?: string }; + +const base: ServerReadyOptions = { + externalBaseOrigin: 'http://localhost:3000', + configFile: 'objectstack.config.ts', + isDev: true, + pluginCount: 1, +}; + +/** + * The banner summary shaped by hand rather than through + * `collectAutomationSummary`: this file is about the RENDERING of one contender, + * so the collection step is deliberately not in the loop. The end-to-end path + * from a fake engine through the collector is covered by + * `commands/serve-automation-shadowing.test.ts`. + */ +const summaryWith = (armed: Contender, shadowedCount = 1): AutomationReadySummary => ({ + enabled: true, + declaredFlowCount: 1, + flowCount: 1, + boundCount: 1, + triggerTypes: ['record_change'], + unbound: [], + unknownObject: [], + draftCount: 0, + shadowed: [{ flowName: 'send-welcome', armed, shadowedCount }], +}); + +describe('#12563 — the banner phrase for a contested flow agrees with the engine renderer', () => { + let lines: string[]; + let spy: ReturnType; + + beforeEach(() => { + lines = []; + // stderr, not stdout (#7915) — the whole banner is a diagnostic. + spy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + lines.push(args.join(' ')); + }); + }); + afterEach(() => spy.mockRestore()); + + const shadowLine = (armed: Contender) => { + printServerReady({ ...base, automation: summaryWith(armed) }); + const shown = lines.filter((l) => l.includes('is claimed by')); + expect(shown).toHaveLength(1); + return shown[0]; + }; + + // One row per branch of the renderer, so a drift on ONE axis fails the row + // for that axis instead of collapsing the file. The assertion is + // `toContain(renderFlowContender(c))`, which is red in BOTH directions: it + // fails if this package's `describeFlowBody` changes, and it fails if the + // engine's renderer changes underneath it. + it('spells a packaged contender exactly as the engine spells it', () => { + const armed: Contender = { source: 'package', packageId: 'crm' }; + expect(shadowLine(armed)).toContain(renderFlowContender(armed)); + }); + + it('spells a runtime overlay row exactly as the engine spells it', () => { + const armed: Contender = { source: 'runtime' }; + expect(shadowLine(armed)).toContain(renderFlowContender(armed)); + }); + + it('spells an absent package id exactly as the engine spells it — and never as `undefined`', () => { + const armed: Contender = { source: 'package' }; + const line = shadowLine(armed); + expect(line).toContain(renderFlowContender(armed)); + // The axis this pin exists for, stated independently of the renderer: if + // BOTH sides regressed to interpolation at once the agreement above would + // still hold, and this row would not. + expect(line).not.toContain('undefined'); + }); + + // ── The instrument can say no ───────────────────────────────────────────── + it('would notice a disagreement — the comparison is not vacuous', () => { + const armed: Contender = { source: 'package', packageId: 'crm' }; + const line = shadowLine(armed); + // The pre-#12563 engine spelling of the SAME contender. If `toContain` + // could not tell the two apart, every row above would pass no matter what + // either side rendered. + expect(line).not.toContain(`package "${armed.packageId}"`); + expect(renderFlowContender(armed)).not.toBe(`package "${armed.packageId}"`); + }); +}); diff --git a/packages/services/service-automation/src/flow-name-shadowing.test.ts b/packages/services/service-automation/src/flow-name-shadowing.test.ts index f4d868f779..dac0ac9570 100644 --- a/packages/services/service-automation/src/flow-name-shadowing.test.ts +++ b/packages/services/service-automation/src/flow-name-shadowing.test.ts @@ -26,7 +26,7 @@ import { describe, it, expect, vi } from 'vitest'; import { SchemaRegistry } from '@objectstack/objectql'; import { AutomationEngine } from './engine.js'; -import { resolveFlowPrecedence, describeFlowContender } from './flow-precedence.js'; +import { resolveFlowPrecedence, describeFlowContender, renderFlowContender } from './flow-precedence.js'; const FLOW = 'opportunity_approval'; const silentLogger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() } as any; @@ -121,7 +121,14 @@ describe('#11997 — packaged flow shadowed by a same-named runtime flow', () => const [message, meta] = warn.mock.calls[0] as [string, any]; expect(message).toContain(FLOW); // the bare name - expect(message).toContain('package "crm"'); // contender A + // ⚠️ Contender A is asserted THROUGH the renderer, not as a literal + // (#12563). This warning used to spell the phrase itself, in a private + // `const describe` two lines above the call — with double quotes, while + // the two other copies of the same sentence used single. Deriving the + // expectation here means re-introducing a private spelling at this call + // site fails THIS row; the renderer's own literal output is pinned + // separately below, so the two cannot drift together and stay green. + expect(message).toContain(renderFlowContender({ source: 'package', packageId: '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'); @@ -219,3 +226,48 @@ describe('#11997 — precedence is a total order, not an iteration order', () => expect((resolved[0].definition as any).label).toBe('TENANT'); }); }); + +/** + * [#12563] The phrase itself, pinned as literals. + * + * The call sites above and the CLI banner assert THROUGH `renderFlowContender`, + * so a caller that re-invents the phrase privately goes red. That alone is not + * enough: if the renderer's own spelling changed, every derived assertion would + * move with it and stay green. These three rows are the anchor that cannot + * move quietly — one per decision the renderer makes. + */ +describe('#12563 — renderFlowContender: one spelling for the contested-flow phrase', () => { + it('quotes a package id the way this package quotes identifiers — single, not double', () => { + // ⛔ Not a majority vote of the three copies that used to exist. Measured + // over `service-automation/src`: 203 single-quoted interpolations in + // operator prose against 3 double-quoted, one of which WAS this phrase. + // The sentence already single-quotes the flow name beside this. + expect(renderFlowContender({ source: 'package', packageId: 'crm' })).toBe("package 'crm'"); + expect(renderFlowContender({ source: 'package', packageId: 'com.objectstack.platform-objects' })).toBe( + "package 'com.objectstack.platform-objects'", + ); + }); + + it('names a runtime overlay by the table an admin would go edit', () => { + expect(renderFlowContender({ source: 'runtime' })).toBe('a runtime-authored row (sys_metadata)'); + // A runtime row bound to a real package id is STILL a runtime row — the + // id is not part of this branch's phrase, and leaking it here would + // read as "a package shipped this", the opposite of what happened. + expect(renderFlowContender({ source: 'runtime', packageId: 'crm' })).toBe( + 'a runtime-authored row (sys_metadata)', + ); + }); + + it('never interpolates an absent package id into the sentence', () => { + // ⚠️ Unreachable from THIS package's callers today — `isCodeArtifactBody` + // is false on a falsy `_packageId`, so `describeFlowContender` never + // emits a package contender without one. That is a property of today's + // callers, not of the renderer, and this row is what keeps the renderer + // safe for the next one. It is a real branch of an exported function. + expect(renderFlowContender({ source: 'package' })).toBe('a code-shipped package (id unknown)'); + expect(renderFlowContender({ source: 'package' })).not.toContain('undefined'); + expect(renderFlowContender({ source: 'package', packageId: '' })).toBe( + 'a code-shipped package (id unknown)', + ); + }); +}); diff --git a/packages/services/service-automation/src/flow-precedence.ts b/packages/services/service-automation/src/flow-precedence.ts index 07ed8e6e53..723cb35cdb 100644 --- a/packages/services/service-automation/src/flow-precedence.ts +++ b/packages/services/service-automation/src/flow-precedence.ts @@ -60,6 +60,14 @@ export interface FlowPrecedenceWinner { /** * Classify one registry body's provenance. * + * ⚠️ This is a CLASSIFIER, not a renderer. It answers "where did this body come + * from?" with a {@link FlowContender} and carries no prose at all — yet it is + * exported, and it sits exactly where a shared renderer would live. It has been + * mistaken for one: three separate callers each wrote their own private + * sentence about a contested flow name, because the export that looked + * reusable had nothing in it to reuse. For the operator-facing phrase, use + * {@link renderFlowContender} below. + * * 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 @@ -78,6 +86,43 @@ export function describeFlowContender(item: unknown): FlowContender { }; } +/** + * Render one contender as the phrase an operator reads. + * + * ⛔ The ONLY place this phrase is spelled. Before it existed the same sentence + * was written three times from scratch — twice in this package (the pull + * warning below, and the plugin's bootstrap audit) and once in + * `@objectstack/cli`'s startup banner — and the copies had already drifted on + * TWO axes. A private `const describe = …` beside a log call is how each copy + * arrived; reach for this instead, and a fourth caller costs nothing. + * + * ## Both spellings are decisions, so they are recorded here + * + * **Single quotes** — measured against this package, ⛔ not voted across the + * copies. Of the interpolated identifiers in operator prose under + * `service-automation/src`, 203 are single-quoted and 3 double-quoted, and one + * of those 3 was this phrase. The sentence this phrase lands in already + * single-quotes the flow NAME, which is the more free-form of the two values, + * so single quotes here add no ambiguity the line does not already carry. + * `packageId` is an unconstrained `z.string()` in `packages/spec`, so neither + * spelling is provably safe against an adversarial id — this one is at least + * the house convention rather than a coin flip. + * + * **A named fallback, never an interpolated `undefined`.** `packageId` is + * optional on {@link FlowContender}, and `package 'undefined'` is the one + * rendering an operator cannot act on. This package's own callers cannot reach + * that branch today — `isCodeArtifactBody` is false on a falsy `_packageId`, + * so a `source: 'package'` contender always carries one — but that is a + * property of today's CALLERS, not of this function. A renderer that is safe + * only because of who happens to call it stops being safe at the next caller. + */ +export function renderFlowContender(contender: FlowContender): string { + if (contender.source !== 'package') return 'a runtime-authored row (sys_metadata)'; + return contender.packageId + ? `package '${contender.packageId}'` + : 'a code-shipped package (id unknown)'; +} + /** * Rank one contender for a bare name. LOWER wins. * @@ -147,13 +192,10 @@ export function resolveFlowPrecedence( 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 ` + + `(${ranked.map((entry) => renderFlowContender(entry.contender)).join(', ')}); ` + + `arming ${renderFlowContender(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.`, { diff --git a/packages/services/service-automation/src/index.ts b/packages/services/service-automation/src/index.ts index 81d4cba54a..259152fd2f 100644 --- a/packages/services/service-automation/src/index.ts +++ b/packages/services/service-automation/src/index.ts @@ -40,7 +40,7 @@ export type { // 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 { resolveFlowPrecedence, describeFlowContender, renderFlowContender } 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 diff --git a/packages/services/service-automation/src/plugin.ts b/packages/services/service-automation/src/plugin.ts index 3ea3130eab..08c5723e99 100644 --- a/packages/services/service-automation/src/plugin.ts +++ b/packages/services/service-automation/src/plugin.ts @@ -15,7 +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 { resolveFlowPrecedence, renderFlowContender } 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'; @@ -1135,11 +1135,10 @@ export class AutomationServicePlugin implements Plugin { // 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(', ')} ` + + `${renderFlowContender(record.armed)} is ARMED and ` + + `${record.shadowed.map(renderFlowContender).join(', ')} ` + `${record.shadowed.length === 1 ? 'is' : 'are'} shadowed (ADR-0005 overlay precedence). ` + `Only the armed definition dispatches.`, );