diff --git a/.changeset/screen-provenance-record-leg-by-value.md b/.changeset/screen-provenance-record-leg-by-value.md new file mode 100644 index 0000000000..4b96f248e0 --- /dev/null +++ b/.changeset/screen-provenance-record-leg-by-value.md @@ -0,0 +1,15 @@ +--- +"@objectstack/service-automation": patch +--- + +A wizard screen is no longer skipped after a durable pause because a record column happens to be an array. + +`judgeHeadlessScreen` decides a screen was already answered by proving the negative: a field is **not** caller-supplied when the subject record carries that key and `params` holds the same value — necessary because the params bag a flow action arrives with is `{ ...record, recordId, Id, ...params }`, so every column of the launched row is in there whether the caller named it or not. + +That comparison was reference identity (`Object.is`), which is real in memory and does not survive persistence. A suspended run stores its context as JSON and resumes from the parsed copy — and the store is preferred over the in-process cache whenever one is wired, so no restart is needed. After that round trip an **array or object** column is equal but no longer identical: the record leg could not disprove it, the field read as caller-supplied, and a later screen with no required fields of its own was **skipped on a run that had supplied nothing**. An interactive user pressed a button and never saw a form they should have been shown; the run completed carrying the row's own value as if they had typed it. Reproduced end to end against a wired store, not inferred. + +The record leg now compares by value (`isDeepStrictEqual`), which survives serialisation. That predicate compares primitives with `Object.is` itself, so this is a strict widening of the "not caller-supplied" set — every pair the old check called equal it still calls equal, plus the structurally identical non-primitives. More screens render, never fewer, which is the direction this module resolves every ambiguity in. + +**Accepted cost, precisely.** A caller that genuinely re-sends a value structurally identical to the row's column is no longer distinguishable from the dispatcher's seed, so it now gets the screen rendered instead of skipped — a lost skip on a headless call, never a lost run, and the same trade the module's other legs already make. Scalar columns behave exactly as before, on both sides of a pause. The row-id leg keeps identity comparison deliberately: a row id is a scalar by construction, so serialisation cannot defeat it and there is nothing there to widen. Measured overhead is a deep compare per declared screen field at screen entry: ~1.5 µs added for a deliberately maximal screen that declares a field for every one of a ten-column row, which is about 38% of one `JSON.stringify` of the run context — a cost the durable store already pays on every suspend. + +This closes the gap the same release's screen-flow headless-satisfaction note records as known. diff --git a/packages/services/service-automation/src/builtin/screen-headless-provenance-durable-resume.test.ts b/packages/services/service-automation/src/builtin/screen-headless-provenance-durable-resume.test.ts new file mode 100644 index 0000000000..9a07f7c20f --- /dev/null +++ b/packages/services/service-automation/src/builtin/screen-headless-provenance-durable-resume.test.ts @@ -0,0 +1,238 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The caller-provenance record leg survives a DURABLE resume (#15812). + * + * `judgeHeadlessScreen` (#15705) lets a screen continue when the CALLER already + * answered it. It cannot read that off `context.params`, because the params bag + * a flow action reaches the engine with is not the caller's bag — + * `seedFlowActionParams` spreads the whole subject row in first. So it proves + * the NEGATIVE instead: a key is not caller-supplied when the record carries it + * and `params` holds the same value. + * + * That leg was written as `Object.is`, i.e. as reference identity, and the + * record spread does copy the record's own value by reference — so in memory a + * run that supplied nothing IS identity-equal there. **Persistence destroys + * that.** A suspended run persists its `context` as JSON + * (`suspended-run-store.ts`: `context_json: JSON.stringify(...)` on save, + * `parseJson` on load) and `resumeInternal` continues the run with the parsed + * value — and `loadSuspendedRunStrict` prefers the STORE over the hot cache + * whenever one is wired, so it does not take a process restart. After that + * round trip `params.tags` and `record.tags` are equal but no longer identical: + * the record leg could not disprove them, the field read as caller-supplied, + * and a later all-optional screen was SKIPPED on a run that had supplied + * nothing. + * + * The direction is the one #15705 exists to prevent: an INTERACTIVE run + * skipping a screen it should have rendered. Hence the remedy here — compare by + * VALUE, which survives serialisation. + * + * ## The rig, and why it is shaped like this + * + * The card lists the conjunction that has to hold together, and every clause is + * load-bearing, so the rig satisfies all of them at once rather than stubbing + * any: the **actions door** (so the row is spread into `params`), a **wired + * durable store**, **a later screen in the same run** entered after the resume, + * a **non-primitive** colliding column, and **no other required field** on that + * screen to force the pause anyway. Remove any one and the run pauses for a + * different reason, which is what the controls below are for — they are not + * decoration, they are the evidence that the pause the fixed code produces + * comes from this leg and not from one of the others. + * + * ⚠️ Primitive columns were never affected: `Object.is('x','x')` is true across + * a round trip. `records the primitive column` below is the control that keeps + * that boundary honest — it passes before AND after the fix, so it cannot be + * mistaken for evidence of the fix. + * + * REVERT-PROOF: put `Object.is` back on the record leg of `callerSupplied` and + * `THE BUG` fails (the run completes instead of pausing) while every control + * here stays green. + */ + +import { describe, it, expect } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import { registerScreenNodes } from './screen-nodes.js'; +import { InMemorySuspendedRunStore } from '../suspended-run-store.js'; +import type { SuspendedRunStore } from '../engine.js'; +import type { AutomationContext } from '@objectstack/spec/contracts'; + +function silentLogger() { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; +} + +/** + * Two screens, deliberately. The FIRST one pauses (a required field nobody + * supplied) — that is what puts the context through the store. The SECOND is + * the one under test: a single OPTIONAL field, so nothing but the provenance + * verdict can decide whether it renders. + */ +function twoScreenFlow(secondField: string) { + return { + name: 'lead_review', + label: 'Lead review', + type: 'screen', + status: 'active', + version: 1, + variables: [ + { name: 'full_name', type: 'text', isInput: true, isOutput: true }, + { name: secondField, type: 'text', isInput: true, isOutput: true }, + ], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'collect', type: 'screen', label: 'Your details', + config: { title: 'Your details', fields: [{ name: 'full_name', label: 'Full name', type: 'text', required: true }] }, + }, + { + id: 'review', type: 'screen', label: 'Review', + // All-optional and single-field: no `required` can force this + // pause, so "it paused" means exactly "nothing was judged + // caller-supplied". + config: { title: 'Review', fields: [{ name: secondField, label: 'Review', type: 'text' }] }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'collect', type: 'default' }, + { id: 'e2', source: 'collect', target: 'review', type: 'default' }, + { id: 'e3', source: 'review', target: 'end', type: 'default' }, + ], + } as any; +} + +/** A fresh engine over `store` (or none) — one per simulated process lifetime. */ +function buildEngine(secondField: string, store?: SuspendedRunStore) { + const e = new AutomationEngine(silentLogger(), store); + registerScreenNodes(e, { logger: silentLogger() } as any); + e.registerFlow('lead_review', twoScreenFlow(secondField)); + return e; +} + +/** + * The bag a flow ACTION actually reaches the engine with — the subject row + * first, the caller's own params last, exactly as `seedFlowActionParams` + * (`@objectstack/runtime`) composes it. Reproduced rather than imported so this + * package's pins do not depend on the other package's build, matching + * `screen-headless-satisfaction.test.ts`. + */ +function actionContext( + record: Record, + params: Record = {}, +): AutomationContext { + return { + record, + object: 'crm_lead', + params: { ...record, recordId: record.id, crmLeadId: record.id, ...params }, + } as AutomationContext; +} + +/** A row whose `tags` column is an ARRAY — the shape identity cannot survive. */ +function lead() { + return { id: 'lead_1', tags: ['a', 'b'], company: 'Acme Inc' }; +} + +/** + * Drive the whole conjunction: launch through the actions door, pause on the + * first screen, resume, and report what the SECOND screen did. + */ +async function driveThroughResume( + secondField: string, + store: SuspendedRunStore | undefined, + context: AutomationContext, +) { + const engine = buildEngine(secondField, store); + const paused = await engine.execute('lead_review', context); + // Precondition, asserted rather than assumed: if the first screen did not + // park, nothing below went through the store and the case is vacuous. + expect(paused.status).toBe('paused'); + expect(paused.screen?.nodeId).toBe('collect'); + return engine.resume(paused.runId!, { variables: { full_name: 'Ada' } }); +} + +describe('caller-provenance survives a durable resume (#15812)', () => { + /** + * THE BUG. Every clause of the card's conjunction holds: actions door, + * wired store, a later screen after the resume, a non-primitive colliding + * column, and no other required field. + * + * The caller supplied NOTHING — `params.tags` is there only because the + * dispatcher spread the row in. So the review screen must render. + */ + it('THE BUG — an array column does not answer a later screen after a durable resume', async () => { + const resumed = await driveThroughResume('tags', new InMemorySuspendedRunStore(), actionContext(lead())); + expect(resumed.status).toBe('paused'); + expect(resumed.screen?.nodeId).toBe('review'); + // Not merely "it stopped": it stopped WITHOUT having answered itself + // from the row. + expect((resumed.output as Record | undefined)?.tags).toBeUndefined(); + }); + + /** + * The same run with NO store: the engine resumes from its hot cache, where + * `params.tags` is still the very array `record.tags` is, so identity holds + * and the leg worked even before the fix. Green on both sides — its job is + * to localise the defect to the serialisation boundary, not to the screen + * logic. + */ + it('CONTROL — with no store wired the same run pauses too (identity never left memory)', async () => { + const resumed = await driveThroughResume('tags', undefined, actionContext(lead())); + expect(resumed.status).toBe('paused'); + expect(resumed.screen?.nodeId).toBe('review'); + }); + + /** + * The card's own ⚠️: a PRIMITIVE column is unaffected, because + * `Object.is('Acme Inc', 'Acme Inc')` is true across a round trip. Green + * before and after — ⛔ never read this one as evidence of the fix. + */ + it('CONTROL — a primitive colliding column was already refused across the round trip', async () => { + const resumed = await driveThroughResume('company', new InMemorySuspendedRunStore(), actionContext(lead())); + expect(resumed.status).toBe('paused'); + expect(resumed.screen?.nodeId).toBe('review'); + }); + + /** + * The row-id leg reads scalars, so serialisation cannot defeat it either. + * Pinned because the fix deliberately leaves that leg on `Object.is` — this + * is the case that says the decision was measured, not overlooked. + */ + it('CONTROL — the row-id seed leg still refuses across a durable resume', async () => { + const resumed = await driveThroughResume('recordId', new InMemorySuspendedRunStore(), actionContext(lead())); + expect(resumed.status).toBe('paused'); + expect(resumed.screen?.nodeId).toBe('review'); + }); + + /** + * #15705's own purpose, preserved: a caller who genuinely drove the screen + * still continues past it after a durable resume. Without this the fix + * could "pass" by making everything pause. + */ + it('a caller who genuinely supplied a DIFFERENT value still continues after the resume', async () => { + const resumed = await driveThroughResume( + 'tags', new InMemorySuspendedRunStore(), actionContext(lead(), { tags: ['urgent'] }), + ); + expect(resumed.status).not.toBe('paused'); + expect(resumed.success).toBe(true); + expect(resumed.output).toMatchObject({ tags: ['urgent'] }); + }); + + /** + * THE WIDENING, pinned as behaviour rather than left as prose. + * + * Value equality enlarges the not-caller-supplied set: a caller that + * re-sends a value structurally identical to the row's now reads as + * indistinguishable from the record seed and the screen renders. Under + * `Object.is` this run CONTINUED (two distinct arrays), so this case is a + * deliberate behaviour change and it changes in this module's standing + * direction — every ambiguity resolves to pausing, which costs a headless + * run a skip and costs an interactive run nothing. + * + * No store here: the widening is a property of the comparison, not of + * persistence. + */ + it('WIDENING — a caller re-sending a value equal to the row is now indistinguishable, so it pauses', async () => { + const resumed = await driveThroughResume('tags', undefined, actionContext(lead(), { tags: ['a', 'b'] })); + expect(resumed.status).toBe('paused'); + expect(resumed.screen?.nodeId).toBe('review'); + }); +}); diff --git a/packages/services/service-automation/src/builtin/screen-headless-satisfaction.test.ts b/packages/services/service-automation/src/builtin/screen-headless-satisfaction.test.ts index a050accb5b..97a8b208a1 100644 --- a/packages/services/service-automation/src/builtin/screen-headless-satisfaction.test.ts +++ b/packages/services/service-automation/src/builtin/screen-headless-satisfaction.test.ts @@ -396,7 +396,7 @@ describe('screen headless satisfaction (#15705)', () => { * The record-change trigger's shape, pinned for the MECHANISM as well as * the outcome: it sets `params` to the SAME object it sets as `record` * (`record-change-trigger.ts`), so `params` is emphatically NOT empty — it - * pauses because every key is identity-equal to the record's own value, not + * pauses because every key still holds the record's own value, not * because there was nothing to read. */ it('CONTROL — record-change trigger shape: params IS the record, and it still pauses', async () => { diff --git a/packages/services/service-automation/src/screen-input-contract.ts b/packages/services/service-automation/src/screen-input-contract.ts index 34585dec87..c2f87e1370 100644 --- a/packages/services/service-automation/src/screen-input-contract.ts +++ b/packages/services/service-automation/src/screen-input-contract.ts @@ -21,6 +21,8 @@ * caller. */ +import { isDeepStrictEqual } from 'node:util'; + import type { ScreenFieldSpec, ScreenSpec } from '@objectstack/spec/contracts'; import type { FieldErrorCode } from '@objectstack/spec/api'; @@ -195,23 +197,42 @@ const NOTHING_SUPPLIED: HeadlessScreenVerdict = { satisfied: false, supplied: [] * * - the record has no such key at all ⇒ the record leg cannot be the source; * - the record HAS the key but `params` holds a different value ⇒ the - * caller's bag overwrote it. The record spread copies the record's own value - * by reference/primitive, so a run that supplied nothing is `Object.is`-equal - * here. Equality is therefore "indistinguishable", not "caller-set". + * caller's bag overwrote it. The record spread copies the record's own + * value, so a run that supplied nothing carries the row's value here. + * Equality is therefore "indistinguishable", not "caller-set". * * The ambiguous case (same key, same value) resolves to NOT caller-supplied, * which costs a headless run a pause it might have been allowed to skip and * costs an interactive run nothing. That asymmetry is deliberate: every * uncertainty in this module must land on today's behaviour. * - * ⚠️ **The identity leg is weaker across a durable resume.** A suspended run - * persists its `context` as JSON (`suspended-run-store.ts`), so a run continued - * from the store judges against a `JSON.parse`d copy: a NON-primitive column - * value (an array, an object) is no longer `Object.is`-equal to the one in - * `params`, and a later wizard screen colliding with such a column can read as - * caller-supplied. Primitive columns are unaffected. Stated, not fixed here — - * the remedy is value comparison rather than identity, which is a different - * change and has its own card. + * **The record leg compares by VALUE, and had to (#15812).** Written as + * `Object.is` it asked about reference identity, which is real in memory — the + * record spread copies by reference — and is destroyed by persistence. A + * suspended run persists its `context` as JSON (`suspended-run-store.ts`: + * `JSON.stringify` on save, `parseJson` on load) and `resumeInternal` continues + * the run with the parsed value; `loadSuspendedRunStrict` prefers the store + * over the hot cache whenever one is wired, so this needs no process restart. + * After that round trip a NON-primitive column value (an array, an object) is + * equal but no longer identical, the record leg could not disprove it, and a + * later all-optional screen was SKIPPED on a run that had supplied nothing — + * measured end to end through a wired store, which is the failure #15705 exists + * to prevent. Primitive columns were never affected (`Object.is('x','x')` is + * true across a round trip), which is exactly why the hole was invisible to + * every in-memory unit test. + * + * `isDeepStrictEqual` compares primitives with `Object.is` itself, so this is a + * strict WIDENING of the old predicate: every pair the identity check called + * equal it still calls equal, plus the structurally-identical non-primitives. + * The widened set is "not caller-supplied", i.e. more pauses, so the change can + * only move runs toward this module's standing direction. The price is that a + * caller who genuinely re-sends a value identical to the row's is no longer + * distinguishable from the seed and gets the screen rendered — a lost skip, not + * a lost run, and the same trade every other leg here already makes. + * + * ⛔ The row-id leg above deliberately keeps `Object.is`: a row id is a scalar + * by construction (`params.recordId` is seeded as one, `record.id` is one), so + * serialisation cannot defeat it and there is nothing there to widen. */ function callerSupplied( name: string, @@ -251,7 +272,10 @@ function callerSupplied( if (seededRowIds.some((id) => id !== undefined && Object.is(value, id))) return false; if (!record || !Object.prototype.hasOwnProperty.call(record, name)) return true; - return !Object.is(value, record[name]); + // BY VALUE, not by identity (#15812) — see the note above. `Object.is` here + // read as "the caller overwrote the column" for any non-primitive value that + // had been through the durable store's JSON round trip. + return !isDeepStrictEqual(value, record[name]); } /**