From 65bc079142fc84a9aaa1882b708fb4e03e696df6 Mon Sep 17 00:00:00 2001 From: sswrk Date: Thu, 23 Jul 2026 15:24:17 +0200 Subject: [PATCH 1/5] [steps] Support local composite functions in hook steps --- packages/steps/src/BuildWorkflow.ts | 8 +- .../steps/src/CompositeFunctionExpander.ts | 4 + packages/steps/src/StepsConfigParser.ts | 45 +--- .../src/__tests__/BuildWorkflow-hooks-test.ts | 228 ++++++++++++++++++ .../__tests__/StepsConfigParser-hooks-test.ts | 143 ++++++++++- packages/steps/src/hooks.ts | 48 ++-- 6 files changed, 419 insertions(+), 57 deletions(-) diff --git a/packages/steps/src/BuildWorkflow.ts b/packages/steps/src/BuildWorkflow.ts index f7713bd8d7..0b15e4f5d6 100644 --- a/packages/steps/src/BuildWorkflow.ts +++ b/packages/steps/src/BuildWorkflow.ts @@ -272,8 +272,8 @@ export async function executeHookStepsAsync( return { failedLocally, firstError }; } -// The one wording for an `if:` that could not be evaluated — anchor steps, -// hook steps, and hook group entries all log through here. +// Shared wording for unevaluable `if:` gates. `ifCondition` is omitted when the +// failure is a composite call-site `if:` evaluated via the step's scope. function logConditionEvaluationError( logger: bunyan, err: unknown, @@ -282,6 +282,8 @@ function logConditionEvaluationError( ): void { logger.error({ err }); logger.error( - `Runner failed to evaluate if it should execute ${subject}, using its if condition "${ifCondition}". This can be caused by trying to access non-existing object property. If you think this is a bug report it here: https://github.com/expo/eas-cli/issues.` + `Runner failed to evaluate if it should execute ${subject}${ + ifCondition ? `, using its if condition "${ifCondition}"` : '' + }. This can be caused by trying to access non-existing object property. If you think this is a bug report it here: https://github.com/expo/eas-cli/issues.` ); } diff --git a/packages/steps/src/CompositeFunctionExpander.ts b/packages/steps/src/CompositeFunctionExpander.ts index 1fa2134572..43091072c0 100644 --- a/packages/steps/src/CompositeFunctionExpander.ts +++ b/packages/steps/src/CompositeFunctionExpander.ts @@ -45,6 +45,10 @@ export type FunctionMaps = { buildFunctionGroupById: BuildFunctionGroupById; }; +export type FunctionMapsWithExpander = FunctionMaps & { + compositeFunctionExpander: CompositeFunctionExpander; +}; + type CompositeFunctionCall = { compositeFunctionPath: string; /** Caller-assigned id used as prefix for all inner step ids. */ diff --git a/packages/steps/src/StepsConfigParser.ts b/packages/steps/src/StepsConfigParser.ts index 617a3cbd3a..314ca6a38b 100644 --- a/packages/steps/src/StepsConfigParser.ts +++ b/packages/steps/src/StepsConfigParser.ts @@ -13,7 +13,7 @@ import { import assert from 'node:assert'; import { AbstractConfigParser } from './AbstractConfigParser'; -import { CompositeFunctionExpander, FunctionMaps } from './CompositeFunctionExpander'; +import { CompositeFunctionExpander, FunctionMapsWithExpander } from './CompositeFunctionExpander'; import { BuildFunction, BuildFunctionById, createBuildFunctionByIdMapping } from './BuildFunction'; import { BuildFunctionGroup, @@ -102,6 +102,7 @@ export class StepsConfigParser extends AbstractConfigParser { // step ids identical across the splicing→engine rollout. const buildSteps: BuildStep[] = []; const hooksByAnchorStep = new Map(); + const maps = { buildFunctionById, buildFunctionGroupById, compositeFunctionExpander }; for (const stepConfig of validatedSteps) { const maybeFunctionGroup = @@ -122,10 +123,7 @@ export class StepsConfigParser extends AbstractConfigParser { if (anchorId === undefined) { continue; } - const anchorHooks = this.constructAnchorHooks(anchorId, validatedHooks, { - buildFunctionById, - buildFunctionGroupById, - }); + const anchorHooks = this.constructAnchorHooks(anchorId, validatedHooks, maps); if (anchorHooks !== undefined) { hooksByAnchorStep.set(expandedStep, anchorHooks); } @@ -133,16 +131,9 @@ export class StepsConfigParser extends AbstractConfigParser { continue; } - const maps = { buildFunctionById, buildFunctionGroupById }; const anchorId = StepsConfigParser.resolveStepAnchor(stepConfig, buildFunctionById); if (anchorId === undefined) { - buildSteps.push( - ...this.createBuildStepsFromNonGroupStepConfig( - stepConfig, - maps, - compositeFunctionExpander - ) - ); + buildSteps.push(...this.createBuildStepsFromNonGroupStepConfig(stepConfig, maps)); continue; } // Rejected regardless of expansion size: the anchor would land on an @@ -153,11 +144,7 @@ export class StepsConfigParser extends AbstractConfigParser { ); } const before = this.constructHookSideEntries(anchorId, 'before', validatedHooks, maps); - const createdSteps = this.createBuildStepsFromNonGroupStepConfig( - stepConfig, - maps, - compositeFunctionExpander - ); + const createdSteps = this.createBuildStepsFromNonGroupStepConfig(stepConfig, maps); assert( createdSteps.length === 1, 'a non-composite step config must create exactly one build step' @@ -227,10 +214,7 @@ export class StepsConfigParser extends AbstractConfigParser { private constructAnchorHooks( anchorId: HookAnchorId, validatedHooks: Record, - maps: { - buildFunctionById: BuildFunctionById; - buildFunctionGroupById: BuildFunctionGroupById; - } + maps: FunctionMapsWithExpander ): AnchorHooks | undefined { const before = this.constructHookSideEntries(anchorId, 'before', validatedHooks, maps); const after = this.constructHookSideEntries(anchorId, 'after', validatedHooks, maps); @@ -244,10 +228,7 @@ export class StepsConfigParser extends AbstractConfigParser { anchorId: HookAnchorId, side: 'before' | 'after', validatedHooks: Record, - maps: { - buildFunctionById: BuildFunctionById; - buildFunctionGroupById: BuildFunctionGroupById; - } + maps: FunctionMapsWithExpander ): HookEntry[] { const hookSteps = validatedHooks[`${side}_${anchorId}`]; if (hookSteps === undefined) { @@ -258,18 +239,13 @@ export class StepsConfigParser extends AbstractConfigParser { private createBuildStepsFromNonGroupStepConfig( stepConfig: Step, - maps: FunctionMaps, - compositeFunctionExpander: CompositeFunctionExpander + maps: FunctionMapsWithExpander ): BuildStep[] { if (isStepShellStep(stepConfig)) { return [createBuildStepFromShellStep(this.ctx, stepConfig)]; } if (isStepFunctionStep(stepConfig)) { - return this.createBuildStepsFromFunctionStepConfig( - stepConfig, - maps, - compositeFunctionExpander - ); + return this.createBuildStepsFromFunctionStepConfig(stepConfig, maps); } throw new BuildConfigError( 'Invalid job step configuration detected. Step must be shell or function step' @@ -278,8 +254,7 @@ export class StepsConfigParser extends AbstractConfigParser { private createBuildStepsFromFunctionStepConfig( step: FunctionStep, - { buildFunctionById }: FunctionMaps, - compositeFunctionExpander: CompositeFunctionExpander + { buildFunctionById, compositeFunctionExpander }: FunctionMapsWithExpander ): BuildStep[] { if (isLocalCompositeFunctionPath(step.uses)) { return compositeFunctionExpander diff --git a/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts b/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts index d700aa668e..6165861907 100644 --- a/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts +++ b/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts @@ -2,11 +2,14 @@ import fs from 'fs/promises'; import { Hooks, Step } from '@expo/eas-build-job'; +import { makeCatalog } from './StepsConfigParser-composite-functions-test-utils'; import { createGlobalContextMock } from './utils/context'; import { BuildFunction } from '../BuildFunction'; import { BuildFunctionGroup } from '../BuildFunctionGroup'; import { BuildStepStatus } from '../BuildStep'; import { BuildStepGlobalContext } from '../BuildStepContext'; +import { BuildStepInput, BuildStepInputValueTypeName } from '../BuildStepInput'; +import { BuildStepOutput } from '../BuildStepOutput'; import { BuildWorkflow } from '../BuildWorkflow'; import { StepsConfigParser } from '../StepsConfigParser'; import { WorkflowHookMetric } from '../StepMetrics'; @@ -70,22 +73,57 @@ describe('BuildWorkflow hook execution', () => { }); } + function versionFunction(id: string, version: string): BuildFunction { + return new BuildFunction({ + namespace: 'test', + id, + fn: (_ctx, { outputs }) => { + executionLog.push(id); + outputs.version.set(version); + }, + outputProviders: [BuildStepOutput.createProvider({ id: 'version', required: true })], + }); + } + + function captureFunction(id: string, sink: (value: unknown) => void): BuildFunction { + return new BuildFunction({ + namespace: 'test', + id, + fn: (_ctx, { inputs }) => { + executionLog.push(id); + sink(inputs.value.value); + }, + inputProviders: [ + BuildStepInput.createProvider({ + id: 'value', + required: false, + allowedValueTypeName: BuildStepInputValueTypeName.STRING, + }), + ], + }); + } + async function parseAsync({ steps, hooks, externalFunctions, externalFunctionGroups, + compositeFunctionCatalog, }: { steps: Step[]; hooks: Hooks | undefined; externalFunctions: BuildFunction[]; externalFunctionGroups?: BuildFunctionGroup[]; + compositeFunctionCatalog?: Record; }): Promise { const parser = new StepsConfigParser(ctx, { steps, hooks, externalFunctions, externalFunctionGroups, + compositeFunctionCatalog: compositeFunctionCatalog + ? makeCatalog(compositeFunctionCatalog) + : undefined, }); return await parser.parseAsync(); } @@ -520,6 +558,196 @@ describe('BuildWorkflow hook execution', () => { }); }); + describe('composite function hook entries', () => { + it('runs composite children in order in a before hook and collects the declared outputs', async () => { + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { before_install_node_modules: [{ uses: './.eas/functions/setup', id: 'setup' }] }, + externalFunctions: [ + anchorFunction(), + versionFunction('read-version', '1.2.3'), + recordingFunction('second-child'), + ], + compositeFunctionCatalog: { + './.eas/functions/setup': { + outputs: { version: { value: '${{ steps.read.outputs.version }}' } }, + runs: { + steps: [ + { id: 'read', uses: 'test/read-version' }, + { id: 'second', uses: 'test/second-child' }, + ], + }, + }, + }, + }); + await workflow.executeAsync(); + expect(executionLog).toEqual(['read-version', 'second-child', 'anchor']); + const entry = [...workflow.hooksByAnchorStep.values()][0].before[0]; + const outputsNode = entry.steps[entry.steps.length - 1]; + expect(outputsNode.id).toBe('setup'); + expect(outputsNode.getOutputValueByName('version')).toBe('1.2.3'); + }); + + it('skips every step of a composite call whose if condition is false and reports no metric', async () => { + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + before_install_node_modules: [ + { uses: './.eas/functions/setup', id: 'setup', if: '${{ false }}' }, + ], + }, + externalFunctions: [anchorFunction(), versionFunction('read-version', '1.2.3')], + compositeFunctionCatalog: { + './.eas/functions/setup': { + outputs: { version: { value: '${{ steps.read.outputs.version }}' } }, + runs: { steps: [{ id: 'read', uses: 'test/read-version' }] }, + }, + }, + }); + await workflow.executeAsync(); + expect(executionLog).toEqual(['anchor']); + const entry = [...workflow.hooksByAnchorStep.values()][0].before[0]; + for (const step of entry.steps) { + expect(step.status).toBe(BuildStepStatus.SKIPPED); + } + expect(metrics).toEqual([]); + }); + + it('after a failed anchor, runs composite children unless they require success() and still collects outputs', async () => { + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + after_install_node_modules: [{ uses: './.eas/functions/cleanup', id: 'cleanup' }], + }, + externalFunctions: [ + failingAnchorFunction(), + recordingFunction('no-if-child'), + recordingFunction('on-success'), + versionFunction('always-version', '9.9.9'), + ], + compositeFunctionCatalog: { + './.eas/functions/cleanup': { + outputs: { last: { value: '${{ steps.always.outputs.version }}' } }, + runs: { + steps: [ + { id: 'first', uses: 'test/no-if-child' }, + { id: 'skipped', uses: 'test/on-success', if: '${{ success() }}' }, + { id: 'always', uses: 'test/always-version', if: '${{ always() }}' }, + ], + }, + }, + }, + }); + await expect(workflow.executeAsync()).rejects.toThrow('anchor failed'); + expect(executionLog).toEqual(['anchor', 'no-if-child', 'always-version']); + expect(hookStepStatuses(workflow)['cleanup__skipped']).toBe(BuildStepStatus.SKIPPED); + const entry = [...workflow.hooksByAnchorStep.values()][0].after[0]; + const outputsNode = entry.steps[entry.steps.length - 1]; + expect(outputsNode.getOutputValueByName('last')).toBe('9.9.9'); + }); + + it('when a composite child fails, skips later children unless they have always() and still collects outputs', async () => { + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { before_install_node_modules: [{ uses: './.eas/functions/setup', id: 'setup' }] }, + externalFunctions: [ + anchorFunction(), + recordingFunction('boom-child', { failWith: new Error('child failed') }), + recordingFunction('after-boom'), + recordingFunction('always-child'), + ], + compositeFunctionCatalog: { + './.eas/functions/setup': { + outputs: { note: { value: 'done' } }, + runs: { + steps: [ + { id: 'boom', uses: 'test/boom-child' }, + { id: 'skipped', uses: 'test/after-boom' }, + { id: 'always', uses: 'test/always-child', if: '${{ always() }}' }, + ], + }, + }, + }, + }); + await expect(workflow.executeAsync()).rejects.toThrow('child failed'); + expect(executionLog).toEqual(['boom-child', 'always-child']); + const statuses = hookStepStatuses(workflow); + expect(statuses['setup__skipped']).toBe(BuildStepStatus.SKIPPED); + expect(statuses['setup']).toBe(BuildStepStatus.SUCCESS); + const entry = [...workflow.hooksByAnchorStep.values()][0].before[0]; + expect(entry.steps[entry.steps.length - 1].getOutputValueByName('note')).toBe('done'); + expect(workflow.buildSteps[0].status).toBe(BuildStepStatus.SKIPPED); + }); + + it('skips the whole composite entry, including its always() children, when an earlier hook entry fails', async () => { + // Memoized call gate: unlike an inline always() hook step, always() children + // inside the composite also skip once the call is inactive. + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + before_install_node_modules: [ + { uses: 'test/first-boom', id: 'first-boom' }, + { uses: './.eas/functions/setup', id: 'setup' }, + ], + }, + externalFunctions: [ + anchorFunction(), + recordingFunction('first-boom', { failWith: new Error('first failed') }), + recordingFunction('plain-child'), + recordingFunction('always-child'), + ], + compositeFunctionCatalog: { + './.eas/functions/setup': { + outputs: { note: { value: 'done' } }, + runs: { + steps: [ + { id: 'plain', uses: 'test/plain-child' }, + { id: 'always', uses: 'test/always-child', if: '${{ always() }}' }, + ], + }, + }, + }, + }); + await expect(workflow.executeAsync()).rejects.toThrow('first failed'); + expect(executionLog).toEqual(['first-boom']); + const statuses = hookStepStatuses(workflow); + expect(statuses['setup__plain']).toBe(BuildStepStatus.SKIPPED); + expect(statuses['setup__always']).toBe(BuildStepStatus.SKIPPED); + expect(statuses['setup']).toBe(BuildStepStatus.SKIPPED); + }); + + it('a later hook step consumes the composite call outputs via steps.', async () => { + const captured: unknown[] = []; + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + before_install_node_modules: [ + { uses: './.eas/functions/setup', id: 'setup' }, + { + id: 'consume', + uses: 'test/consume', + with: { value: '${{ steps.setup.outputs.version }}' }, + }, + ], + }, + externalFunctions: [ + anchorFunction(), + versionFunction('read-version', '1.2.3'), + captureFunction('consume', value => captured.push(value)), + ], + compositeFunctionCatalog: { + './.eas/functions/setup': { + outputs: { version: { value: '${{ steps.read.outputs.version }}' } }, + runs: { steps: [{ id: 'read', uses: 'test/read-version' }] }, + }, + }, + }); + await workflow.executeAsync(); + expect(executionLog).toEqual(['read-version', 'consume', 'anchor']); + expect(captured).toEqual(['1.2.3']); + }); + }); + describe('the eas.workflow.hook metric callback', () => { it('reports one event per executed hook side with anchor, timing, and result', async () => { const workflow = await parseAsync({ diff --git a/packages/steps/src/__tests__/StepsConfigParser-hooks-test.ts b/packages/steps/src/__tests__/StepsConfigParser-hooks-test.ts index 499a94d4e2..29399070a3 100644 --- a/packages/steps/src/__tests__/StepsConfigParser-hooks-test.ts +++ b/packages/steps/src/__tests__/StepsConfigParser-hooks-test.ts @@ -265,7 +265,7 @@ describe('StepsConfigParser hook construction', () => { expect(error.message).toMatch(/nonexistent_function/); }); - it('rejects a hook step using a local composite function with BuildConfigError, not an assertion crash', async () => { + it('rejects a hook step using a composite function missing from the catalog with BuildConfigError', async () => { const error = await getErrorAsync(async () => { await parseWorkflowAsync({ ctx, @@ -274,7 +274,7 @@ describe('StepsConfigParser hook construction', () => { }); }); expect(error).toBeInstanceOf(BuildConfigError); - expect(error.message).toMatch(/not supported in hooks/); + expect(error.message).toMatch(/"\.\/\.eas\/functions\/setup" does not exist/); }); }); @@ -606,6 +606,145 @@ describe('StepsConfigParser hooks with composite functions', () => { expect(workflow.buildSteps.map(step => step.displayName)).toEqual(['Install node modules']); expect(workflow.hooksByAnchorStep.size).toBe(0); }); + + it('parses a composite hook step into a single entry with namespaced children and the outputs step last', async () => { + const workflow = await parseWorkflowAsync({ + ctx, + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + before_install_node_modules: [{ uses: './.eas/functions/setup', id: 'setup' }], + }, + compositeFunctionCatalog: makeCatalog({ + './.eas/functions/setup': { + outputs: { version: { value: '${{ steps.read.outputs.version }}' } }, + runs: { + steps: [{ id: 'read', run: 'set-output version "1.0.0"' }, { run: 'echo hi' }], + }, + }, + }), + }); + const anchorHooks = [...workflow.hooksByAnchorStep.values()][0]; + expect(anchorHooks.before).toHaveLength(1); + expect(anchorHooks.before[0].steps.map(step => step.id)).toEqual([ + 'setup__read', + 'setup__composite_function_step_1', + 'setup', + ]); + expect(workflow.buildSteps.map(step => step.displayName)).toEqual(['Install node modules']); + }); + + it('does not copy the if condition of a composite hook step onto the entry', async () => { + // The authored if: is applied inside the expansion scope, not on the entry. + const workflow = await parseWorkflowAsync({ + ctx, + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + before_install_node_modules: [ + { uses: './.eas/functions/setup', id: 'setup', if: '${{ failure() }}' }, + ], + }, + compositeFunctionCatalog: makeCatalog({ + './.eas/functions/setup': { + runs: { steps: [{ run: 'echo hi' }] }, + }, + }), + }); + const anchorHooks = [...workflow.hooksByAnchorStep.values()][0]; + expect(anchorHooks.before[0].ifCondition).toBeUndefined(); + }); + + it('rejects working_directory on a composite hook step', async () => { + // The call expands away during parsing, so there is no single step to apply it to. + const error = await getErrorAsync(async () => { + await parseWorkflowAsync({ + ctx, + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + before_install_node_modules: [ + { uses: './.eas/functions/setup', id: 'setup', working_directory: 'app' }, + ], + }, + compositeFunctionCatalog: makeCatalog({ + './.eas/functions/setup': { + runs: { steps: [{ run: 'echo hi' }] }, + }, + }), + }); + }); + expect(error).toBeInstanceOf(BuildConfigError); + expect(error.message).toMatch(/"working_directory" is not supported/); + }); + + it('flattens nested composite calls into a single hook entry', async () => { + const workflow = await parseWorkflowAsync({ + ctx, + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + before_install_node_modules: [{ uses: './.eas/functions/outer', id: 'top' }], + }, + compositeFunctionCatalog: makeCatalog({ + './.eas/functions/outer': { + runs: { + steps: [{ uses: './.eas/functions/inner', id: 'mid' }, { run: 'echo done' }], + }, + }, + './.eas/functions/inner': { + runs: { steps: [{ run: 'echo inner' }] }, + }, + }), + }); + const anchorHooks = [...workflow.hooksByAnchorStep.values()][0]; + expect(anchorHooks.before).toHaveLength(1); + // No declared outputs, so no outputs nodes appear in the expansion. + expect(anchorHooks.before[0].steps.map(step => step.id)).toEqual([ + 'top__mid__composite_function_step_1', + 'top__composite_function_step_1', + ]); + }); + + it('orders composite hook expansions as before hook, anchor, then after hook', async () => { + const workflow = await parseWorkflowAsync({ + ctx, + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + before_install_node_modules: [{ uses: './.eas/functions/setup', id: 'setup' }], + after_install_node_modules: [{ uses: './.eas/functions/teardown', id: 'teardown' }], + }, + compositeFunctionCatalog: makeCatalog({ + './.eas/functions/setup': { + runs: { steps: [{ id: 'prepare', run: 'echo prepare' }] }, + }, + './.eas/functions/teardown': { + runs: { steps: [{ id: 'clean', run: 'echo clean' }] }, + }, + }), + }); + expect(workflow.getExecutionOrderedSteps().map(step => step.id)).toEqual([ + 'setup__prepare', + expect.stringMatching(/^step-\d{3,}$/), + 'teardown__clean', + ]); + }); + + it('fails parseAsync when a composite hook call id collides with a job step id', async () => { + const error = await getErrorAsync(async () => { + await parseWorkflowAsync({ + ctx, + steps: [{ uses: 'eas/install_node_modules' }, { run: 'echo job', id: 'setup' }], + hooks: { + before_install_node_modules: [{ uses: './.eas/functions/setup', id: 'setup' }], + }, + compositeFunctionCatalog: makeCatalog({ + './.eas/functions/setup': { + // Outputs node reuses the call id, forcing the collision with the job step. + outputs: { version: { value: '${{ steps.read.outputs.version }}' } }, + runs: { steps: [{ id: 'read', run: 'set-output version "1.0.0"' }] }, + }, + }), + }); + }); + expect(error).toBeInstanceOf(BuildWorkflowError); + }); }); describe('StepsConfigParser hook validation view', () => { diff --git a/packages/steps/src/hooks.ts b/packages/steps/src/hooks.ts index 84cf80473d..b3c1c5209f 100644 --- a/packages/steps/src/hooks.ts +++ b/packages/steps/src/hooks.ts @@ -1,4 +1,5 @@ import { + CompositeFunctionCatalog, HookAnchorId, ShellStep, Step, @@ -7,17 +8,17 @@ import { } from '@expo/eas-build-job'; import assert from 'node:assert'; -import { BuildFunction, BuildFunctionById, createBuildFunctionByIdMapping } from './BuildFunction'; -import { - BuildFunctionGroup, - BuildFunctionGroupById, - createBuildFunctionGroupByIdMapping, -} from './BuildFunctionGroup'; +import { BuildFunction, createBuildFunctionByIdMapping } from './BuildFunction'; +import { BuildFunctionGroup, createBuildFunctionGroupByIdMapping } from './BuildFunctionGroup'; import { BuildStep } from './BuildStep'; import { BuildStepGlobalContext } from './BuildStepContext'; import { collectAggregateStepErrors } from './BuildWorkflowValidator'; +import { CompositeFunctionExpander, FunctionMapsWithExpander } from './CompositeFunctionExpander'; import { BuildConfigError, BuildWorkflowError } from './errors'; -import { isLocalCompositeFunctionPath } from './utils/localCompositeFunctions'; +import { + isLocalCompositeFunctionPath, + parseLocalCompositeFunctionPath, +} from './utils/localCompositeFunctions'; import { createBuildStepOutputsFromDefinition, getShellStepDisplayName } from './utils/step'; /** @@ -30,6 +31,11 @@ import { createBuildStepOutputsFromDefinition, getShellStepDisplayName } from '. * (2) An entry whose explicit `if:` passed behaves like a single step whose * `if:` passed: its no-`if:` steps run past earlier entries' failures, while * within-entry failures still skip later siblings. + * + * Composite calls are also one entry, but their call-site `if:` lives on the + * expansion scope (not `ifCondition`), matching main-workflow composites. A + * passing call `if:` therefore does not grant the group-entry `!entryFailed` + * shield; no-`if:` children follow the plain hook default. */ export interface HookEntry { steps: BuildStep[]; @@ -64,9 +70,12 @@ export async function constructHookEntriesAsync( { externalFunctions, externalFunctionGroups, + compositeFunctionCatalog, }: { externalFunctions?: BuildFunction[]; externalFunctionGroups?: BuildFunctionGroup[]; + /** When omitted, composite `uses:` fail as missing from an empty catalog. */ + compositeFunctionCatalog?: CompositeFunctionCatalog; } ): Promise { // An empty array is a valid no-op (e.g. opting out of a default hook); @@ -86,6 +95,10 @@ export async function constructHookEntriesAsync( return constructHookEntriesFromValidatedSteps(ctx, validatedSteps, { buildFunctionById, buildFunctionGroupById, + compositeFunctionExpander: new CompositeFunctionExpander(ctx, compositeFunctionCatalog ?? {}, { + buildFunctionById, + buildFunctionGroupById, + }), }); } @@ -110,13 +123,7 @@ export async function validateHookStepsAsync( export function constructHookEntriesFromValidatedSteps( ctx: BuildStepGlobalContext, validatedSteps: Step[], - { - buildFunctionById, - buildFunctionGroupById, - }: { - buildFunctionById: BuildFunctionById; - buildFunctionGroupById: BuildFunctionGroupById; - } + { buildFunctionById, buildFunctionGroupById, compositeFunctionExpander }: FunctionMapsWithExpander ): HookEntry[] { const entries: HookEntry[] = []; for (const step of validatedSteps) { @@ -127,9 +134,16 @@ export function constructHookEntriesFromValidatedSteps( continue; } if (isLocalCompositeFunctionPath(step.uses)) { - throw new BuildConfigError( - `Local composite function steps ("uses: ${step.uses}") are not supported in hooks.` - ); + entries.push({ + steps: compositeFunctionExpander + .expandCompositeFunctionStep( + step, + parseLocalCompositeFunctionPath(step.uses), + BuildStep.getNewId(step.id) + ) + .getFlattenedSteps(), + }); + continue; } const maybeFunctionGroup = buildFunctionGroupById[step.uses]; if (maybeFunctionGroup !== undefined) { From 61547b150d14311d4e0ba42e65482d1bfd56cf20 Mon Sep 17 00:00:00 2001 From: sswrk Date: Tue, 28 Jul 2026 14:14:59 +0200 Subject: [PATCH 2/5] remove FunctionMapsWithExpander in favor of expander-only call sites --- .../steps/src/CompositeFunctionExpander.ts | 12 ++-- packages/steps/src/StepsConfigParser.ts | 60 ++++++++++++++----- packages/steps/src/hooks.ts | 20 +++---- 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/packages/steps/src/CompositeFunctionExpander.ts b/packages/steps/src/CompositeFunctionExpander.ts index 43091072c0..87f91ca42d 100644 --- a/packages/steps/src/CompositeFunctionExpander.ts +++ b/packages/steps/src/CompositeFunctionExpander.ts @@ -45,10 +45,6 @@ export type FunctionMaps = { buildFunctionGroupById: BuildFunctionGroupById; }; -export type FunctionMapsWithExpander = FunctionMaps & { - compositeFunctionExpander: CompositeFunctionExpander; -}; - type CompositeFunctionCall = { compositeFunctionPath: string; /** Caller-assigned id used as prefix for all inner step ids. */ @@ -74,6 +70,14 @@ export class CompositeFunctionExpander { private readonly functionMaps: FunctionMaps ) {} + public get buildFunctionById(): BuildFunctionById { + return this.functionMaps.buildFunctionById; + } + + public get buildFunctionGroupById(): BuildFunctionGroupById { + return this.functionMaps.buildFunctionGroupById; + } + public expandCompositeFunctionStep( step: FunctionStep, compositeFunctionPath: string, diff --git a/packages/steps/src/StepsConfigParser.ts b/packages/steps/src/StepsConfigParser.ts index 314ca6a38b..ae6bccd2a3 100644 --- a/packages/steps/src/StepsConfigParser.ts +++ b/packages/steps/src/StepsConfigParser.ts @@ -13,7 +13,7 @@ import { import assert from 'node:assert'; import { AbstractConfigParser } from './AbstractConfigParser'; -import { CompositeFunctionExpander, FunctionMapsWithExpander } from './CompositeFunctionExpander'; +import { CompositeFunctionExpander } from './CompositeFunctionExpander'; import { BuildFunction, BuildFunctionById, createBuildFunctionByIdMapping } from './BuildFunction'; import { BuildFunctionGroup, @@ -102,7 +102,6 @@ export class StepsConfigParser extends AbstractConfigParser { // step ids identical across the splicing→engine rollout. const buildSteps: BuildStep[] = []; const hooksByAnchorStep = new Map(); - const maps = { buildFunctionById, buildFunctionGroupById, compositeFunctionExpander }; for (const stepConfig of validatedSteps) { const maybeFunctionGroup = @@ -123,7 +122,11 @@ export class StepsConfigParser extends AbstractConfigParser { if (anchorId === undefined) { continue; } - const anchorHooks = this.constructAnchorHooks(anchorId, validatedHooks, maps); + const anchorHooks = this.constructAnchorHooks( + anchorId, + validatedHooks, + compositeFunctionExpander + ); if (anchorHooks !== undefined) { hooksByAnchorStep.set(expandedStep, anchorHooks); } @@ -133,7 +136,9 @@ export class StepsConfigParser extends AbstractConfigParser { const anchorId = StepsConfigParser.resolveStepAnchor(stepConfig, buildFunctionById); if (anchorId === undefined) { - buildSteps.push(...this.createBuildStepsFromNonGroupStepConfig(stepConfig, maps)); + buildSteps.push( + ...this.createBuildStepsFromNonGroupStepConfig(stepConfig, compositeFunctionExpander) + ); continue; } // Rejected regardless of expansion size: the anchor would land on an @@ -143,15 +148,28 @@ export class StepsConfigParser extends AbstractConfigParser { 'Hook anchors are not supported on local composite function steps.' ); } - const before = this.constructHookSideEntries(anchorId, 'before', validatedHooks, maps); - const createdSteps = this.createBuildStepsFromNonGroupStepConfig(stepConfig, maps); + const before = this.constructHookSideEntries( + anchorId, + 'before', + validatedHooks, + compositeFunctionExpander + ); + const createdSteps = this.createBuildStepsFromNonGroupStepConfig( + stepConfig, + compositeFunctionExpander + ); assert( createdSteps.length === 1, 'a non-composite step config must create exactly one build step' ); const anchorStep = createdSteps[0]; buildSteps.push(anchorStep); - const after = this.constructHookSideEntries(anchorId, 'after', validatedHooks, maps); + const after = this.constructHookSideEntries( + anchorId, + 'after', + validatedHooks, + compositeFunctionExpander + ); if (before.length > 0 || after.length > 0) { hooksByAnchorStep.set(anchorStep, { anchor: anchorId, before, after }); } @@ -214,10 +232,20 @@ export class StepsConfigParser extends AbstractConfigParser { private constructAnchorHooks( anchorId: HookAnchorId, validatedHooks: Record, - maps: FunctionMapsWithExpander + compositeFunctionExpander: CompositeFunctionExpander ): AnchorHooks | undefined { - const before = this.constructHookSideEntries(anchorId, 'before', validatedHooks, maps); - const after = this.constructHookSideEntries(anchorId, 'after', validatedHooks, maps); + const before = this.constructHookSideEntries( + anchorId, + 'before', + validatedHooks, + compositeFunctionExpander + ); + const after = this.constructHookSideEntries( + anchorId, + 'after', + validatedHooks, + compositeFunctionExpander + ); if (before.length === 0 && after.length === 0) { return undefined; } @@ -228,24 +256,24 @@ export class StepsConfigParser extends AbstractConfigParser { anchorId: HookAnchorId, side: 'before' | 'after', validatedHooks: Record, - maps: FunctionMapsWithExpander + compositeFunctionExpander: CompositeFunctionExpander ): HookEntry[] { const hookSteps = validatedHooks[`${side}_${anchorId}`]; if (hookSteps === undefined) { return []; } - return constructHookEntriesFromValidatedSteps(this.ctx, hookSteps, maps); + return constructHookEntriesFromValidatedSteps(this.ctx, hookSteps, compositeFunctionExpander); } private createBuildStepsFromNonGroupStepConfig( stepConfig: Step, - maps: FunctionMapsWithExpander + compositeFunctionExpander: CompositeFunctionExpander ): BuildStep[] { if (isStepShellStep(stepConfig)) { return [createBuildStepFromShellStep(this.ctx, stepConfig)]; } if (isStepFunctionStep(stepConfig)) { - return this.createBuildStepsFromFunctionStepConfig(stepConfig, maps); + return this.createBuildStepsFromFunctionStepConfig(stepConfig, compositeFunctionExpander); } throw new BuildConfigError( 'Invalid job step configuration detected. Step must be shell or function step' @@ -254,7 +282,7 @@ export class StepsConfigParser extends AbstractConfigParser { private createBuildStepsFromFunctionStepConfig( step: FunctionStep, - { buildFunctionById, compositeFunctionExpander }: FunctionMapsWithExpander + compositeFunctionExpander: CompositeFunctionExpander ): BuildStep[] { if (isLocalCompositeFunctionPath(step.uses)) { return compositeFunctionExpander @@ -266,7 +294,7 @@ export class StepsConfigParser extends AbstractConfigParser { .getFlattenedSteps(); } - const buildFunction = buildFunctionById[step.uses]; + const buildFunction = compositeFunctionExpander.buildFunctionById[step.uses]; assert(buildFunction, 'function ID must be ID of function or function group'); return [ diff --git a/packages/steps/src/hooks.ts b/packages/steps/src/hooks.ts index b3c1c5209f..648a3aa165 100644 --- a/packages/steps/src/hooks.ts +++ b/packages/steps/src/hooks.ts @@ -13,7 +13,7 @@ import { BuildFunctionGroup, createBuildFunctionGroupByIdMapping } from './Build import { BuildStep } from './BuildStep'; import { BuildStepGlobalContext } from './BuildStepContext'; import { collectAggregateStepErrors } from './BuildWorkflowValidator'; -import { CompositeFunctionExpander, FunctionMapsWithExpander } from './CompositeFunctionExpander'; +import { CompositeFunctionExpander } from './CompositeFunctionExpander'; import { BuildConfigError, BuildWorkflowError } from './errors'; import { isLocalCompositeFunctionPath, @@ -92,14 +92,14 @@ export async function constructHookEntriesAsync( externalFunctionIds: Object.keys(buildFunctionById), externalFunctionGroupIds: Object.keys(buildFunctionGroupById), }); - return constructHookEntriesFromValidatedSteps(ctx, validatedSteps, { - buildFunctionById, - buildFunctionGroupById, - compositeFunctionExpander: new CompositeFunctionExpander(ctx, compositeFunctionCatalog ?? {}, { + return constructHookEntriesFromValidatedSteps( + ctx, + validatedSteps, + new CompositeFunctionExpander(ctx, compositeFunctionCatalog ?? {}, { buildFunctionById, buildFunctionGroupById, - }), - }); + }) + ); } /** @@ -123,7 +123,7 @@ export async function validateHookStepsAsync( export function constructHookEntriesFromValidatedSteps( ctx: BuildStepGlobalContext, validatedSteps: Step[], - { buildFunctionById, buildFunctionGroupById, compositeFunctionExpander }: FunctionMapsWithExpander + compositeFunctionExpander: CompositeFunctionExpander ): HookEntry[] { const entries: HookEntry[] = []; for (const step of validatedSteps) { @@ -145,7 +145,7 @@ export function constructHookEntriesFromValidatedSteps( }); continue; } - const maybeFunctionGroup = buildFunctionGroupById[step.uses]; + const maybeFunctionGroup = compositeFunctionExpander.buildFunctionGroupById[step.uses]; if (maybeFunctionGroup !== undefined) { entries.push({ steps: maybeFunctionGroup.createBuildStepsFromFunctionGroupCall(ctx, { @@ -155,7 +155,7 @@ export function constructHookEntriesFromValidatedSteps( }); continue; } - const buildFunction = buildFunctionById[step.uses]; + const buildFunction = compositeFunctionExpander.buildFunctionById[step.uses]; assert(buildFunction, 'function ID must be ID of function or function group'); entries.push({ steps: [ From 5ec7702b1e9d863b3857b6e47ebdf0d36a09add7 Mon Sep 17 00:00:00 2001 From: sswrk Date: Tue, 28 Jul 2026 14:45:38 +0200 Subject: [PATCH 3/5] composite functions and function groups: fast-fail within them in after hooks, but continue after hook execution after entry failure --- packages/steps/src/BuildWorkflow.ts | 14 ++- .../src/__tests__/BuildWorkflow-hooks-test.ts | 103 +++++++++++++++++- packages/steps/src/hooks.ts | 9 +- 3 files changed, 112 insertions(+), 14 deletions(-) diff --git a/packages/steps/src/BuildWorkflow.ts b/packages/steps/src/BuildWorkflow.ts index 0b15e4f5d6..10e3432883 100644 --- a/packages/steps/src/BuildWorkflow.ts +++ b/packages/steps/src/BuildWorkflow.ts @@ -165,8 +165,9 @@ export class BuildWorkflow { * Default-run rules (a step or entry with no `if:`): * - `before`: runs unless a failure occurred within THIS hook sequence; * failures predating the call are ignored — "runs iff the anchor runs". - * - `after`: runs unconditionally — past the anchor's own failure AND past an - * earlier after-entry's failure. + * - `after`: each entry starts fresh, past the anchor's own failure AND past + * an earlier after-entry's failure, but a failure within the entry skips + * its later no-`if:` steps. * Passed as `runByDefault` so composite scopes share the same missing-`if:` rule. * A user `if:` is always evaluated against the real global context, so * `failure()` / `success()` keep their global meaning on both sides. @@ -224,10 +225,11 @@ export async function executeHookStepsAsync( let entryFailed = false; for (const step of entry.steps) { - // before skips on in-sequence failure; an entry with a passed if: only skips - // on within-entry failure; after always runs. - const runByDefault = - options.timing === 'after' || (entryHasExplicitCondition ? !entryFailed : !failedLocally); + // after and an entry with a passed if: skip on within-entry failure; + // before without an entry if: skips on any in-sequence failure. + const scopedFailure = + options.timing === 'after' || entryHasExplicitCondition ? entryFailed : failedLocally; + const runByDefault = !scopedFailure; let shouldExecuteStep = false; try { shouldExecuteStep = step.shouldExecuteStep({ runByDefault }); diff --git a/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts b/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts index 6165861907..ae2d05dd1c 100644 --- a/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts +++ b/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts @@ -493,7 +493,7 @@ describe('BuildWorkflow hook execution', () => { ]); }); - it('within a single after entry, a no-if step following a failed sibling still runs', async () => { + it('within a single after entry, a failed sibling skips later no-if siblings', async () => { const failingChild = recordingFunction('failing-child', { failWith: new Error('child failed'), }); @@ -513,7 +513,66 @@ describe('BuildWorkflow hook execution', () => { externalFunctionGroups: [group], }); await expect(workflow.executeAsync()).rejects.toThrow('child failed'); - expect(executionLog).toEqual(['anchor', 'failing-child', 'ok-child']); + expect(executionLog).toEqual(['anchor', 'failing-child']); + expect(hookStepStatuses(workflow)['ok-child']).toBe(BuildStepStatus.SKIPPED); + }); + + it('a failed after group entry does not stop a later no-if after entry', async () => { + const failingChild = recordingFunction('failing-child', { + failWith: new Error('child failed'), + }); + const okChild = recordingFunction('ok-child'); + const group = new BuildFunctionGroup({ + namespace: 'test', + id: 'group', + createBuildStepsFromFunctionGroupCall: globalCtx => [ + failingChild.createBuildStepFromFunctionCall(globalCtx, { id: 'failing-child' }), + okChild.createBuildStepFromFunctionCall(globalCtx, { id: 'ok-child' }), + ], + }); + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + after_install_node_modules: [ + { uses: 'test/group' }, + { uses: 'test/next-entry', id: 'next-entry' }, + ], + }, + externalFunctions: [ + anchorFunction(), + failingChild, + okChild, + recordingFunction('next-entry'), + ], + externalFunctionGroups: [group], + }); + await expect(workflow.executeAsync()).rejects.toThrow('child failed'); + expect(executionLog).toEqual(['anchor', 'failing-child', 'next-entry']); + expect(hookStepStatuses(workflow)['ok-child']).toBe(BuildStepStatus.SKIPPED); + }); + + it('within an after entry whose if: passed, a failed sibling skips later no-if siblings', async () => { + const failingChild = recordingFunction('failing-child', { + failWith: new Error('child failed'), + }); + const okChild = recordingFunction('ok-child'); + const group = new BuildFunctionGroup({ + namespace: 'test', + id: 'group', + createBuildStepsFromFunctionGroupCall: globalCtx => [ + failingChild.createBuildStepFromFunctionCall(globalCtx, { id: 'failing-child' }), + okChild.createBuildStepFromFunctionCall(globalCtx, { id: 'ok-child' }), + ], + }); + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { after_install_node_modules: [{ uses: 'test/group', if: '${{ always() }}' }] }, + externalFunctions: [anchorFunction(), failingChild, okChild], + externalFunctionGroups: [group], + }); + await expect(workflow.executeAsync()).rejects.toThrow('child failed'); + expect(executionLog).toEqual(['anchor', 'failing-child']); + expect(hookStepStatuses(workflow)['ok-child']).toBe(BuildStepStatus.SKIPPED); }); it('within a before entry whose if: passed, a failed sibling skips later no-if siblings', async () => { @@ -679,6 +738,40 @@ describe('BuildWorkflow hook execution', () => { expect(workflow.buildSteps[0].status).toBe(BuildStepStatus.SKIPPED); }); + it('when an after composite child fails, skips later children unless they have always() and still collects outputs', async () => { + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { + after_install_node_modules: [{ uses: './.eas/functions/publish', id: 'publish' }], + }, + externalFunctions: [ + anchorFunction(), + recordingFunction('login', { failWith: new Error('login failed') }), + recordingFunction('push'), + recordingFunction('always-child'), + ], + compositeFunctionCatalog: { + './.eas/functions/publish': { + outputs: { note: { value: 'done' } }, + runs: { + steps: [ + { id: 'login', uses: 'test/login' }, + { id: 'push', uses: 'test/push' }, + { id: 'always', uses: 'test/always-child', if: '${{ always() }}' }, + ], + }, + }, + }, + }); + await expect(workflow.executeAsync()).rejects.toThrow('login failed'); + expect(executionLog).toEqual(['anchor', 'login', 'always-child']); + const statuses = hookStepStatuses(workflow); + expect(statuses['publish__push']).toBe(BuildStepStatus.SKIPPED); + expect(statuses['publish']).toBe(BuildStepStatus.SUCCESS); + const entry = [...workflow.hooksByAnchorStep.values()][0].after[0]; + expect(entry.steps[entry.steps.length - 1].getOutputValueByName('note')).toBe('done'); + }); + it('skips the whole composite entry, including its always() children, when an earlier hook entry fails', async () => { // Memoized call gate: unlike an inline always() hook step, always() children // inside the composite also skip once the call is inactive. @@ -842,8 +935,8 @@ describe('BuildWorkflow hook execution', () => { const group = new BuildFunctionGroup({ namespace: 'test', id: 'group', - // The failing child comes FIRST: after-side no-`if:` steps continue - // past sibling failures, so ok-child must still execute. + // The failing child comes FIRST: within-entry fail-fast skips + // ok-child, and the single event still reports the aggregated failure. createBuildStepsFromFunctionGroupCall: globalCtx => [ failingChild.createBuildStepFromFunctionCall(globalCtx), okChild.createBuildStepFromFunctionCall(globalCtx), @@ -856,7 +949,7 @@ describe('BuildWorkflow hook execution', () => { externalFunctionGroups: [group], }); await expect(workflow.executeAsync()).rejects.toThrow('child failed'); - expect(executionLog).toEqual(['anchor', 'failing-child', 'ok-child']); + expect(executionLog).toEqual(['anchor', 'failing-child']); expect(metrics).toEqual([ { anchor: 'install_node_modules', diff --git a/packages/steps/src/hooks.ts b/packages/steps/src/hooks.ts index 648a3aa165..f527b885fd 100644 --- a/packages/steps/src/hooks.ts +++ b/packages/steps/src/hooks.ts @@ -30,12 +30,15 @@ import { createBuildStepOutputsFromDefinition, getShellStepDisplayName } from '. * context only; a group call's `with:` inputs are not visible to them. * (2) An entry whose explicit `if:` passed behaves like a single step whose * `if:` passed: its no-`if:` steps run past earlier entries' failures, while - * within-entry failures still skip later siblings. + * within-entry failures still skip later siblings. `after` entries get the + * same within-entry gate even without an explicit `if:`. * * Composite calls are also one entry, but their call-site `if:` lives on the * expansion scope (not `ifCondition`), matching main-workflow composites. A - * passing call `if:` therefore does not grant the group-entry `!entryFailed` - * shield; no-`if:` children follow the plain hook default. + * passing call `if:` therefore does not grant the before-side `!entryFailed` + * shield; there, no-`if:` children follow the plain in-sequence default. On + * the after side every entry is gated by `!entryFailed`, so composite + * children fail fast within their entry like everything else. */ export interface HookEntry { steps: BuildStep[]; From 6e56f6188de5040d72bfdac271953f21e21d9a76 Mon Sep 17 00:00:00 2001 From: sswrk Date: Wed, 29 Jul 2026 14:59:09 +0200 Subject: [PATCH 4/5] Correct comment on unevaluable if: log helper --- packages/steps/src/BuildWorkflow.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/steps/src/BuildWorkflow.ts b/packages/steps/src/BuildWorkflow.ts index 10e3432883..3df1334d2d 100644 --- a/packages/steps/src/BuildWorkflow.ts +++ b/packages/steps/src/BuildWorkflow.ts @@ -274,8 +274,9 @@ export async function executeHookStepsAsync( return { failedLocally, firstError }; } -// Shared wording for unevaluable `if:` gates. `ifCondition` is omitted when the -// failure is a composite call-site `if:` evaluated via the step's scope. +// Shared wording for unevaluable `if:` gates. Callers pass the step's own +// `if:`, but the throw may come from a composite call-site `if:` evaluated via +// the step's scope. In that case the quoted condition is not the one that failed. function logConditionEvaluationError( logger: bunyan, err: unknown, From e260a74979883ebbda294d04ed14d0541f911fb3 Mon Sep 17 00:00:00 2001 From: sswrk Date: Wed, 29 Jul 2026 15:34:41 +0200 Subject: [PATCH 5/5] Don't let the synthetic composite outputs node alone trigger the hook metric --- packages/steps/src/BuildWorkflow.ts | 11 +++++--- .../src/__tests__/BuildWorkflow-hooks-test.ts | 26 +++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/packages/steps/src/BuildWorkflow.ts b/packages/steps/src/BuildWorkflow.ts index 3df1334d2d..c759c617e5 100644 --- a/packages/steps/src/BuildWorkflow.ts +++ b/packages/steps/src/BuildWorkflow.ts @@ -5,6 +5,7 @@ import { BuildFunctionById } from './BuildFunction'; import { BuildRuntimePlatform } from './BuildRuntimePlatform'; import { BuildStep } from './BuildStep'; import { BuildStepGlobalContext } from './BuildStepContext'; +import { CompositeBuildStep } from './CompositeBuildStep'; import { StepMetricResult } from './StepMetrics'; import { AnchorHooks, HookEntry } from './hooks'; import { evaluateIfCondition } from './utils/jsepEval'; @@ -192,7 +193,7 @@ export async function executeHookStepsAsync( ctx.markAsFailed(); }; - let anyStepExecuted = false; + let anyAuthoredStepExecuted = false; for (const entry of entries) { // Truthiness, not presence: an empty `if:` means "no condition", the same // as BuildStep.shouldExecuteStep treats it. @@ -247,7 +248,11 @@ export async function executeHookStepsAsync( step.skip(); continue; } - anyStepExecuted = true; + // The synthetic composite outputs node runs under always(), so it alone + // must not make a fully-skipped hook side report a metric. + if (!(step instanceof CompositeBuildStep)) { + anyAuthoredStepExecuted = true; + } const startTime = performance.now(); let stepResult: StepMetricResult = 'success'; try { @@ -262,7 +267,7 @@ export async function executeHookStepsAsync( } } - if (anyStepExecuted) { + if (anyAuthoredStepExecuted) { ctx.reportWorkflowHookMetric({ anchor: options.anchor, timing: options.timing, diff --git a/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts b/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts index ae2d05dd1c..364c0b6b61 100644 --- a/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts +++ b/packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts @@ -645,6 +645,9 @@ describe('BuildWorkflow hook execution', () => { const outputsNode = entry.steps[entry.steps.length - 1]; expect(outputsNode.id).toBe('setup'); expect(outputsNode.getOutputValueByName('version')).toBe('1.2.3'); + expect(metrics).toEqual([ + { anchor: 'install_node_modules', timing: 'before', result: 'success' }, + ]); }); it('skips every step of a composite call whose if condition is false and reports no metric', async () => { @@ -672,6 +675,29 @@ describe('BuildWorkflow hook execution', () => { expect(metrics).toEqual([]); }); + it('reports no metric when every authored child of an outputs-declaring composite is skipped', async () => { + const workflow = await parseAsync({ + steps: [{ uses: 'eas/install_node_modules' }], + hooks: { before_install_node_modules: [{ uses: './.eas/functions/setup', id: 'setup' }] }, + externalFunctions: [anchorFunction(), versionFunction('read-version', '1.2.3')], + compositeFunctionCatalog: { + './.eas/functions/setup': { + outputs: { version: { value: '${{ steps.read.outputs.version }}' } }, + runs: { steps: [{ id: 'read', uses: 'test/read-version', if: '${{ false }}' }] }, + }, + }, + }); + await workflow.executeAsync(); + expect(executionLog).toEqual(['anchor']); + expect(hookStepStatuses(workflow)['setup__read']).toBe(BuildStepStatus.SKIPPED); + // The synthetic outputs node still runs under always(); only the metric + // must not depend on it. + const entry = [...workflow.hooksByAnchorStep.values()][0].before[0]; + const outputsNode = entry.steps[entry.steps.length - 1]; + expect(outputsNode.status).toBe(BuildStepStatus.SUCCESS); + expect(metrics).toEqual([]); + }); + it('after a failed anchor, runs composite children unless they require success() and still collects outputs', async () => { const workflow = await parseAsync({ steps: [{ uses: 'eas/install_node_modules' }],