fix(service-automation): seed a retry attempt's variable environment through the same chokepoint as attempt 1 (#9888)

* fix(service-automation): seed a retry attempt's variable environment through the same chokepoint as attempt 1

`executeWithoutRetry()` — the method `retryExecution` re-runs the flow
through on every retry attempt — seeded only the flow's declared variables
and `$record`, while `execute()` also binds `record` plus the trigger
record's flattened fields, `previous`, `$runId`, `$flowName` and
`$flowLabel`. A retry attempt therefore ran in a strictly smaller variable
environment than the first.

Under strict CEL an unbound name ABORTS the predicate rather than yielding
false (#4697), so a start condition or edge predicate reading `previous`
(#3427) or a bare record field failed on the retry for a reason attempt 1
never hit, and a pausing node on a retry attempt had no `$runId` to map its
external state back to this run with (ADR-0019).

Both methods now seed through one private `seedRunVariables` helper, the
`buildRunTrigger` chokepoint pattern. First-attempt behaviour is unchanged:
the helper is `execute()`'s own block verbatim, and the run id is now minted
before the seeding (order-safe — `nextRunId()` is a stateless random id).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx

* chore(changeset): patch for the retry-attempt variable-environment repair

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-warren

.changeset/retry-attempt-variable-environment.md

@@ -0,0 +1,25 @@
+---
+'@objectstack/service-automation': patch
+---
+
+fix(service-automation): a retry attempt now runs with the same variable environment as the first
+
+`executeWithoutRetry()` — the method the retry loop re-runs a flow through on every
+attempt — seeded only the flow's declared variables and `$record`, while the first
+attempt also binds `record` plus the triggering record's flattened fields, `previous`,
+`$runId`, `$flowName` and `$flowLabel`. Every retry attempt therefore ran in a strictly
+smaller environment than attempt 1.
+
+Because conditions are strict CEL, where reading an unbound name aborts the predicate
+rather than yielding `false`, this was user-visible exactly where retry is most used —
+`errorHandling.strategy: 'retry'` on a record-change flow:
+
+- a start condition or edge predicate reading `previous` (the create-vs-update
+  discriminator) aborted on the retry, so the retry failed for a reason the first attempt
+  never hit — reading as a flaky flow rather than a defect;
+- a bare reference to a triggering-record field (`status`, `budget`) aborted for the same
+  reason;
+- a pausing node (e.g. Approval) reached on a retry attempt saw no `$runId`, so the
+  external state it minted could not be mapped back to the run for resume (ADR-0019).
+
+Both methods now seed through one shared chokepoint. First-attempt behaviour is unchanged.

packages/services/service-automation/src/engine.ts

@@ -3077,38 +3077,19 @@ export class AutomationEngine implements IAutomationService {
         // runaway it exists to stop.
         let reentryHeld = false;
 
-        // Initialize variable context
-        const variables = this.seedDeclaredVariables(flow, context);
-        // Inject trigger record. `$record` is the canonical handle; `record` is a
-        // friendlier alias so templates/conditions can write `{record.title}` and
-        // `record.status`. We also flatten the record's own fields to top-level
-        // variables (so bare references like `status`/`budget` resolve in start
-        // conditions and edge predicates) WITHOUT clobbering flow inputs already
-        // seeded above. `previous` exposes the pre-update row for transition gates.
-        if (context?.record) {
-            variables.set('$record', context.record);
-            variables.set('record', context.record);
-            for (const [k, v] of Object.entries(context.record)) {
-                if (!variables.has(k)) variables.set(k, v);
-            }
-        }
-        // Always bind `previous` — to `null` on the create/insert leg (there is no
-        // prior row) — so a start condition can DISCRIMINATE create vs update on a
-        // `record-after-write` flow: `previous == null` is the create leg (#3427).
-        // Binding only-when-truthy left `previous` an unknown CEL variable on
-        // insert, so ANY reference to it (even `previous == null`) threw
-        // "Unknown variable: previous" and failed the whole condition.
-        variables.set('previous', context?.previous ?? null);
-
+        // Initialize the run's variable environment. Every binding a run is
+        // entitled to — the flow's declared variables, the trigger record and
+        // its flattened fields, `previous`, and the engine-owned `$runId` /
+        // `$flowName` / `$flowLabel` — is seeded by `seedRunVariables`, the one
+        // chokepoint `executeWithoutRetry` shares (#9704). See that helper for
+        // why the seeding is not written out here any more.
+        //
+        // The run id is minted BEFORE the seeding because `$runId` is part of
+        // that environment. Order-safe: `nextRunId()` is a stateless random id
+        // (no counter to advance), and nothing between the old call site and
+        // this one reads or mints one.
         const runId = this.nextRunId();
-        // Expose the run id to executors (ADR-0019): a pausing node (e.g. Approval)
-        // reads `$runId` to map its external state back to this run for resume.
-        variables.set('$runId', runId);
-        // Expose flow identity to executors so externalized state (e.g. an
-        // approval request row) can carry a human-readable origin. Captured in
-        // the variable snapshot, so still present after a suspend/resume.
-        variables.set('$flowName', flowName);
-        variables.set('$flowLabel', flow.label ?? flowName);
+        const variables = this.seedRunVariables(flow, flowName, context, runId);
         const startedAt = new Date().toISOString();
         const steps: StepLogEntry[] = [];
 
@@ -6315,6 +6296,78 @@ export class AutomationEngine implements IAutomationService {
         return variables;
     }
 
+    /**
+     * Seed a run's COMPLETE variable environment — the one chokepoint every
+     * attempt of every run goes through (#9704).
+     *
+     * `seedDeclaredVariables` above turns the flow's own DECLARATIONS into
+     * bindings; this adds everything the ENGINE owns on top of them, in the
+     * order the precedence rules require:
+     *
+     *  1. declared variables (params, then `defaultValue`) — seeded first, so
+     *     the record flattening below cannot shadow a flow input;
+     *  2. the trigger record: `$record` is the canonical handle, `record` a
+     *     friendlier alias so templates and conditions can write
+     *     `{record.title}` / `record.status`, plus the record's own fields
+     *     flattened to top-level names so bare references (`status`, `budget`)
+     *     resolve in start conditions and edge predicates — WITHOUT clobbering
+     *     anything already bound;
+     *  3. `previous`, bound ALWAYS — to `null` on the create/insert leg, since
+     *     there is no prior row — so a start condition can DISCRIMINATE create
+     *     from update on a `record-after-write` flow: `previous == null` is the
+     *     create leg (#3427). Binding it only when truthy left `previous` an
+     *     unknown CEL variable on insert, so ANY reference to it (even
+     *     `previous == null`) threw "Unknown variable: previous" and failed the
+     *     whole condition;
+     *  4. `$runId`, so a pausing node (e.g. Approval) can map its external
+     *     state back to this run for resume (ADR-0019), and `$flowName` /
+     *     `$flowLabel`, so externalized state carries a human-readable origin.
+     *     All three are captured in the variable snapshot, so they survive a
+     *     suspend/resume.
+     *
+     * ⚠️ It is ONE method because the two callers drifting apart is the defect
+     * it repairs, not a tidiness preference. `execute()` seeded all of the
+     * above and `executeWithoutRetry()` — the method `retryExecution` re-runs
+     * the flow through on EVERY retry attempt — seeded only (1) and `$record`,
+     * so a retry attempt ran in a strictly smaller environment than the first
+     * one: conditions are strict CEL, where reading an unbound name ABORTS the
+     * predicate instead of yielding `false` (#4697), so the retry failed for a
+     * reason attempt 1 never hit, which reads as a flaky flow rather than a
+     * defect. The two methods had already drifted once per card on four
+     * separate exits (#9378, #9415, #9414, #9510) before this one, always in
+     * the same direction — the copy that is not `execute()` is the one a repair
+     * forgets. `buildRunTrigger` is the same chokepoint pattern, for the same
+     * reason. ⛔ So a change here belongs here: re-inlining either caller's copy
+     * re-opens the drift, and `retry-attempt-pause.test.ts` pins the two
+     * snapshots against EACH OTHER precisely so it cannot happen silently.
+     *
+     * The caller mints `runId` and passes it in rather than this helper minting
+     * one, because the run id is the caller's own bookkeeping: it keys the log
+     * row, the continuation and the returned envelope, and a helper that
+     * produced a second one would put a `$runId` in the snapshot that names no
+     * run anybody can resume.
+     */
+    private seedRunVariables(
+        flow: FlowParsed,
+        flowName: string,
+        context: AutomationContext | undefined,
+        runId: string,
+    ): Map<string, unknown> {
+        const variables = this.seedDeclaredVariables(flow, context);
+        if (context?.record) {
+            variables.set('$record', context.record);
+            variables.set('record', context.record);
+            for (const [k, v] of Object.entries(context.record)) {
+                if (!variables.has(k)) variables.set(k, v);
+            }
+        }
+        variables.set('previous', context?.previous ?? null);
+        variables.set('$runId', runId);
+        variables.set('$flowName', flowName);
+        variables.set('$flowLabel', flow.label ?? flowName);
+        return variables;
+    }
+
     /**
      * Execute a flow without triggering retry logic (used by retryExecution to prevent recursion).
      *
@@ -6351,12 +6404,19 @@ export class AutomationEngine implements IAutomationService {
             return { success: false, code: 'FLOW_DISABLED', error: `Flow '${flowName}' is disabled` };
         }
 
-        const variables = this.seedDeclaredVariables(flow, context);
-        if (context?.record) {
-            variables.set('$record', context.record);
-        }
-
+        // [#9704] The SAME environment attempt 1 runs in — seeded through the
+        // same chokepoint `execute()` uses. This method used to seed only the
+        // declared variables and `$record`, so every retry attempt ran in a
+        // strictly smaller environment than the first: `record` and its
+        // flattened fields, `previous`, `$runId`, `$flowName` and `$flowLabel`
+        // were all absent. Under strict CEL an unbound name ABORTS the
+        // predicate rather than yielding false (#4697), so a start condition or
+        // edge predicate reading `previous` (#3427) or a bare record field
+        // failed on the retry for a reason attempt 1 never hit — a flaky flow,
+        // to its author — and a pausing node on a retry attempt had no `$runId`
+        // to map its external state back to this run with (ADR-0019).
         const runId = this.nextRunId();
+        const variables = this.seedRunVariables(flow, flowName, context, runId);
         const startedAt = new Date().toISOString();
         const steps: StepLogEntry[] = [];
 

packages/services/service-automation/src/retry-attempt-pause.test.ts

@@ -273,26 +273,53 @@ describe('#9510 — a pause on a RETRY attempt is durable, not a burned attempt'
         expect(storedViaRetry?.nodeType).toBe(storedViaExecute?.nodeType);
         expect(storedViaRetry?.correlation).toBe(storedViaExecute?.correlation);
 
-        // ⚠️ The one thing that does NOT match, asserted rather than skirted.
-        // `executeWithoutRetry` seeds none of the engine-owned variables
+        // ⭐ [#9704] The variable ENVIRONMENT matches too — and this block is
+        // where the divergence used to be pinned as measured behaviour.
+        // `executeWithoutRetry` seeded none of the engine-owned variables
         // `execute()` binds (`$runId`, `$flowName`, `$flowLabel`, `record` and
-        // its flattened fields, `previous`), so a retry attempt has always run
-        // in a smaller environment than the first — filed as #9704, a divergent
-        // run environment rather than a lost pause, and out of scope here: it
-        // afflicts every retry attempt, pausing or not, and predates this card.
+        // its flattened fields, `previous`), so a retry attempt ran in a
+        // strictly SMALLER environment than the first: under strict CEL an
+        // unbound name ABORTS a predicate instead of yielding false (#4697), so
+        // a start condition or edge predicate reading `previous` (#3427) or a
+        // bare record field failed on the retry for a reason attempt 1 never
+        // hit, and a pausing node on a retry attempt saw no `$runId` to map its
+        // external state back with (ADR-0019). Both methods now seed through
+        // one helper (`seedRunVariables`), so the two snapshots are compared to
+        // EACH OTHER — the same discipline the result envelope above uses.
         //
-        // It is pinned as TODAY's measured behaviour, deliberately, so #9704
-        // cannot be repaired silently. When it is repaired these three
-        // assertions are the ones that go red, and the correct edit is to
-        // delete them and add `variables` to the parity block above.
-        const engineOwned = ['$runId', '$flowName', '$flowLabel', 'previous', 'record'];
-        for (const name of engineOwned) {
-            expect(Object.keys(storedViaExecute?.variables ?? {})).toContain(name);
-            expect(Object.keys(storedViaRetry?.variables ?? {})).not.toContain(name);
-        }
-        // What the two DO share: the run's own work. Both snapshots carry the
-        // pausing node's inputs, so the continuation is a real continuation on
-        // either route — the half this card is about.
+        // `$runId` is per-run by nature, so it is asserted against the run id
+        // each route actually returned and then normalized for the comparison.
+        // Asserting it by VALUE is the point: a snapshot merely *carrying* a
+        // `$runId` that names a different run is the ADR-0019 mapping hole this
+        // card is about, and a presence-only check cannot see it.
+        expect(storedViaRetry?.variables?.$runId).toBe(viaRetry.runId);
+        expect(storedViaExecute?.variables?.$runId).toBe(viaExecute.runId);
+        const vars = (s: { variables: Record<string, unknown> } | null) => ({
+            ...(s?.variables ?? {}),
+            $runId: '<runId>',
+        });
+        expect(vars(storedViaRetry)).toEqual(vars(storedViaExecute));
+
+        // …and the engine-owned bindings pinned by VALUE on the RETRY route,
+        // not merely by parity: the comparison above is equally satisfied if
+        // BOTH routes lose them, which is the shape a later "simplification" of
+        // the shared helper would take.
+        expect(storedViaRetry?.variables?.$flowName).toBe('flaky_approval');
+        expect(storedViaRetry?.variables?.$flowLabel).toBe('flaky_approval');
+        // `previous` is bound ALWAYS — to `null` on the create leg, since that
+        // is what lets a start condition discriminate create vs update (#3427).
+        // `toHaveProperty` rather than a `?.previous` read: the defect was the
+        // key being ABSENT, and absent and `null` both read as `null`.
+        expect(storedViaRetry?.variables).toHaveProperty('previous', null);
+        expect(storedViaRetry?.variables?.record).toEqual({ id: 'ord_9510', amount: 500 });
+        // The trigger record's own fields flattened to top-level names — what
+        // makes a bare `amount` reference resolve on a retry attempt.
+        expect(storedViaRetry?.variables?.amount).toBe(500);
+        expect(storedViaRetry?.variables?.id).toBe('ord_9510');
+
+        // What the two shared even BEFORE the repair: the run's own work. Both
+        // snapshots carry the pausing node's inputs, so the continuation is a
+        // real continuation on either route — the half #9510 was about.
         expect(storedViaRetry?.variables?.['flaky.ok']).toEqual(storedViaExecute?.variables?.['flaky.ok']);
         expect(storedViaRetry?.variables?.$record).toEqual(storedViaExecute?.variables?.$record);