[steps] Unify the default gating of steps with no if: across steps, composite scopes, and hooks

packages/steps/src/BuildStep.ts

@@ -333,15 +333,15 @@ export class BuildStep extends BuildStepOutputAccessor {
     );
   }
 
-  public shouldExecuteStep(): boolean {
+  public shouldExecuteStep({ runByDefault }: { runByDefault: boolean }): boolean {
     if (
       this.compositeFunctionScope &&
-      !this.compositeFunctionScope.isActive(evaluateIfConditionExpression)
+      !this.compositeFunctionScope.isActive(evaluateIfConditionExpression, runByDefault)
     ) {
       return false;
     }
     if (!this.ifCondition) {
-      return !this.ctx.global.hasAnyPreviousStepFailed;
+      return runByDefault;
     }
     return this.evaluateIfCondition(this.ifCondition, {
       scope: this.compositeFunctionScope,

packages/steps/src/BuildStepCompositeFunctionScope.ts

@@ -73,18 +73,18 @@ export class BuildStepCompositeFunctionScope {
    * Memoized: global status can change mid-expansion, and re-evaluating would flip a passed
    * success() gate and skip remaining always()/failure() inner steps.
    */
-  public isActive(evaluate: EvaluateIfExpression): boolean {
-    if (this.parent && !this.parent.isActive(evaluate)) {
+  public isActive(evaluate: EvaluateIfExpression, runByDefault: boolean): boolean {
+    if (this.parent && !this.parent.isActive(evaluate, runByDefault)) {
       return false;
     }
-    this.cachedIsActive ??= this.evaluateCallIfCondition(evaluate);
+    this.cachedIsActive ??= this.evaluateCallIfCondition(evaluate, runByDefault);
     return this.cachedIsActive;
   }
 
   // Call-site if uses caller env/inputs/steps, not expanded inner steps.
-  private evaluateCallIfCondition(evaluate: EvaluateIfExpression): boolean {
+  private evaluateCallIfCondition(evaluate: EvaluateIfExpression, runByDefault: boolean): boolean {
     if (!this.ifCondition) {
-      return !this.ctx.hasAnyPreviousStepFailed;
+      return runByDefault;
     }
     const callerBase: JobInterpolationContext = {
       ...this.ctx.getInterpolationContext(),

packages/steps/src/BuildWorkflow.ts

@@ -76,7 +76,9 @@ export class BuildWorkflow {
       // occurrence; an anchor's `if:` cannot see its before-hooks' outputs.
       let shouldExecuteStep = false;
       try {
-        shouldExecuteStep = step.shouldExecuteStep();
+        shouldExecuteStep = step.shouldExecuteStep({
+          runByDefault: !this.ctx.hasAnyPreviousStepFailed,
+        });
       } catch (err: any) {
         logConditionEvaluationError(
           step.ctx.logger,
@@ -165,6 +167,7 @@ export class BuildWorkflow {
  *   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.
+ * 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.
  */
@@ -221,28 +224,22 @@ 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);
       let shouldExecuteStep = false;
-      if (step.ifCondition) {
-        try {
-          shouldExecuteStep = step.shouldExecuteStep();
-        } catch (err) {
-          logConditionEvaluationError(
-            step.ctx.logger,
-            err,
-            `step "${step.displayName}"`,
-            step.ifCondition
-          );
-          recordFailure(err);
-          entryFailed = true;
-        }
-      } else {
-        // Before-side: a no-`if:` step runs unless this hook sequence has
-        // already failed. An entry whose explicit condition evaluated true
-        // behaves like a single step whose if: passed — the entry's no-`if:`
-        // steps ignore failures from EARLIER entries (only within-entry
-        // failures skip them).
-        shouldExecuteStep =
-          options.timing === 'after' || (entryHasExplicitCondition ? !entryFailed : !failedLocally);
+      try {
+        shouldExecuteStep = step.shouldExecuteStep({ runByDefault });
+      } catch (err) {
+        logConditionEvaluationError(
+          step.ctx.logger,
+          err,
+          `step "${step.displayName}"`,
+          step.ifCondition
+        );
+        recordFailure(err);
+        entryFailed = true;
       }
       if (!shouldExecuteStep) {
         step.skip();

packages/steps/src/__tests__/BuildStep-test.ts

@@ -1179,7 +1179,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       ],
     });
 
-    expect(step.shouldExecuteStep()).toBe(false);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(false);
   });
 
   it('returns true when if condition is always and previous steps failed', () => {
@@ -1191,7 +1191,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       command: 'echo 123',
       ifCondition: '${ always() }',
     });
-    expect(step.shouldExecuteStep()).toBe(true);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
   });
 
   it('returns true when if condition is always and previous steps have not failed', () => {
@@ -1202,7 +1202,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       command: 'echo 123',
       ifCondition: '${ always() }',
     });
-    expect(step.shouldExecuteStep()).toBe(true);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
   });
 
   it('returns false when if condition is success and previous steps failed', () => {
@@ -1214,7 +1214,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       command: 'echo 123',
       ifCondition: '${ success() }',
     });
-    expect(step.shouldExecuteStep()).toBe(false);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(false);
   });
 
   it('returns true when a dynamic expression matches', () => {
@@ -1231,7 +1231,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       },
       ifCondition: '${ env.NODE_ENV === "production" && env.LOCAL_ENV === "true" }',
     });
-    expect(step.shouldExecuteStep()).toBe(true);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
   });
 
   it('can use the general interpolation context', () => {
@@ -1245,7 +1245,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       command: 'echo 123',
       ifCondition: 'fromJSON(env.CONFIG_JSON).foo == "bar"',
     });
-    expect(step.shouldExecuteStep()).toBe(true);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
   });
 
   it('returns true when a simplified dynamic expression matches', () => {
@@ -1259,7 +1259,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       },
       ifCondition: "env.NODE_ENV === 'production'",
     });
-    expect(step.shouldExecuteStep()).toBe(true);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
   });
 
   it('returns true when an input matches', () => {
@@ -1282,7 +1282,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       ],
       ifCondition: 'inputs.foo1 === "bar"',
     });
-    expect(step.shouldExecuteStep()).toBe(true);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
   });
 
   it('returns true when an eas value matches', () => {
@@ -1293,7 +1293,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       command: 'echo 123',
       ifCondition: 'eas.runtimePlatform === "linux"',
     });
-    expect(step.shouldExecuteStep()).toBe(true);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
   });
 
   it('returns true when if condition is success and previous steps have not failed', () => {
@@ -1304,7 +1304,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
       command: 'echo 123',
       ifCondition: '${ success() }',
     });
-    expect(step.shouldExecuteStep()).toBe(true);
+    expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
   });
 
   it('returns true when if condition is failure and previous steps failed', () => {
@@ -1317,7 +1317,7 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
         command: 'echo 123',
         ifCondition,
       });
-      expect(step.shouldExecuteStep()).toBe(true);
+      expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(true);
     }
   });
 
@@ -1330,7 +1330,31 @@ describe(BuildStep.prototype.shouldExecuteStep, () => {
         command: 'echo 123',
         ifCondition,
       });
-      expect(step.shouldExecuteStep()).toBe(false);
+      expect(step.shouldExecuteStep({ runByDefault: !ctx.hasAnyPreviousStepFailed })).toBe(false);
     }
   });
+
+  it('resolves a missing if condition with the provided run-by-default policy', () => {
+    const ctx = createGlobalContextMock();
+    const step = new BuildStep(ctx, {
+      id: 'test1',
+      displayName: 'Test 1',
+      command: 'echo 123',
+    });
+    expect(step.shouldExecuteStep({ runByDefault: false })).toBe(false);
+    ctx.markAsFailed();
+    expect(step.shouldExecuteStep({ runByDefault: true })).toBe(true);
+  });
+
+  it('ignores the run-by-default value when an if condition is present', () => {
+    const ctx = createGlobalContextMock();
+    ctx.markAsFailed();
+    const step = new BuildStep(ctx, {
+      id: 'test1',
+      displayName: 'Test 1',
+      command: 'echo 123',
+      ifCondition: '${ success() }',
+    });
+    expect(step.shouldExecuteStep({ runByDefault: true })).toBe(false);
+  });
 });

packages/steps/src/__tests__/BuildStepCompositeFunctionScope-test.ts

@@ -53,4 +53,21 @@ describe(BuildStepCompositeFunctionScope, () => {
       interpolateJobContext({ target: '${{ steps.checkout.outputs.sha }}', context })
     ).toBeUndefined();
   });
+
+  describe('isActive', () => {
+    const neverEvaluate = (): boolean => {
+      throw new Error('a call without an if: must not evaluate an expression');
+    };
+
+    it('resolves a call without an if: with the provided run-by-default value', () => {
+      expect(makeScope().isActive(neverEvaluate, false)).toBe(false);
+      expect(makeScope().isActive(neverEvaluate, true)).toBe(true);
+    });
+
+    it('memoizes the first evaluation; a later policy flip cannot re-gate the call', () => {
+      const scope = makeScope();
+      expect(scope.isActive(neverEvaluate, true)).toBe(true);
+      expect(scope.isActive(neverEvaluate, false)).toBe(true);
+    });
+  });
 });

packages/steps/src/__tests__/BuildWorkflow-hooks-test.ts

@@ -455,6 +455,53 @@ describe('BuildWorkflow hook execution', () => {
       ]);
     });
 
+    it('within a single after entry, a no-if step following a failed sibling still runs', 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' }] },
+        externalFunctions: [anchorFunction(), failingChild, okChild],
+        externalFunctionGroups: [group],
+      });
+      await expect(workflow.executeAsync()).rejects.toThrow('child failed');
+      expect(executionLog).toEqual(['anchor', 'failing-child', 'ok-child']);
+    });
+
+    it('within a before 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: { before_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(['failing-child']);
+      expect(hookStepStatuses(workflow)['ok-child']).toBe(BuildStepStatus.SKIPPED);
+    });
+
     it('an entry-level condition evaluation error fails the hook (and the job), not silently ignored', async () => {
       const { group, functions } = createGroup('group', ['child-one']);
       const workflow = await parseAsync({