[steps] Support local composite functions in hook steps - #4062
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## szymonswierk/eng-22387-steps-unify-no-if-gating-policy #4062 +/- ##
==========================================================================================
+ Coverage 62.22% 62.23% +0.01%
==========================================================================================
Files 995 995
Lines 44754 44764 +10
Branches 9412 9415 +3
==========================================================================================
+ Hits 27845 27855 +10
Misses 15462 15462
Partials 1447 1447 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ccd27cf to
88b25ed
Compare
e118ae0 to
5a88848
Compare
88b25ed to
afe2ad9
Compare
afe2ad9 to
de031b5
Compare
642f105 to
40b60f7
Compare
73ece96 to
be7ec02
Compare
fd5922a to
44fec11
Compare
0236cea to
f14d4f6
Compare
44fec11 to
56907d3
Compare
f14d4f6 to
fb5d85f
Compare
56907d3 to
a107305
Compare
fb5d85f to
0a629b7
Compare
a107305 to
95e91de
Compare
0a629b7 to
32f79bd
Compare
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
79cbcd4 to
ccd5926
Compare
9358a14 to
073ad5d
Compare
073ad5d to
98ef96f
Compare
hSATAC
left a comment
There was a problem hiding this comment.
A few non-blocking nits. Most of them are about composite behaviour that predates this PR rather than anything it introduces, so they are all fine to split out into a separate PR if you would rather keep this one focused.
| buildFunctionGroupById: BuildFunctionGroupById; | ||
| }; | ||
|
|
||
| export type FunctionMapsWithExpander = FunctionMaps & { |
There was a problem hiding this comment.
nit, not blocking.
CompositeFunctionExpander already holds its own functionMaps, so this type carries the maps twice and nothing stops a caller from assembling a bag whose expander was built from a different pair of maps. Passing just the expander, or exposing the maps off it, would make that impossible to get wrong. Happy to leave it if you prefer keeping the destructuring at the call sites as it is.
| * 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. |
There was a problem hiding this comment.
nit, not blocking, and I think this predates the PR rather than being caused by it, so it is fine as a separate PR.
The paragraph is accurate, but the consequence is sharper than it reads. When an earlier entry has already failed and the call if: passes, the no-if: children skip as described, while the synthetic outputs node runs anyway, since it is constructed with ifCondition: '${{ always() }}'. So the call reports SUCCESS and publishes its declared outputs resolved against skipped children, which come out as empty strings. A later step reading ${{ steps.<call-id>.outputs.x }} silently gets ''.
I checked and the same thing happens in the main workflow, so this PR does not introduce it. Raising it here because hooks are where people write if: always(), so it will start showing up.
There was a problem hiding this comment.
Valid point. Deferring it to a follow-up PR as this seems like a bigger change.
| // 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. |
There was a problem hiding this comment.
nit, not blocking, and fine to split into its own PR.
The omission is conditional on the child having an if:, not on where the failure came from. When a composite call-site if: throws and the child does have its own if:, this quotes the child's condition as the culprit even though that condition is valid and did not fail. And because isActive memoizes with ??=, a throw is never cached, so every remaining child re-evaluates the same broken expression and logs the same pair of lines. A 10-step composite produces 10 copies naming 10 different steps, none of which is the one the user actually wrote the condition on.
The same misattribution already exists in the main-workflow loop further up, so it is not new. Flagging it here because this PR is the one touching the message.
There was a problem hiding this comment.
Yeah, not specific to this PR but a valid point. Changed the message a bit and deferring the fix to a follow-up PR.
| ); | ||
| entries.push({ | ||
| steps: compositeFunctionExpander | ||
| .expandCompositeFunctionStep( |
There was a problem hiding this comment.
nit, not blocking, pre-existing rather than yours, and fine as a separate PR.
expandCompositeFunctionStep forwards name, with, if and env from the call step and rejects working_directory with a good message, but it never reads step.shell, so shell: on a composite call is silently dropped. Same class of thing as working_directory, just unguarded. Worth either rejecting it the same way or forwarding it to the inner steps.
There was a problem hiding this comment.
I think we're fine because https://github.com/expo/eas-cli/blob/main/packages/eas-build-job/src/step.ts#L97
We can follow-up with further improving the handling of this like in working_directory.
| parseLocalCompositeFunctionPath(step.uses), | ||
| BuildStep.getNewId(step.id) | ||
| ) | ||
| .getFlattenedSteps(), |
There was a problem hiding this comment.
nit, not blocking, and arguably my area rather than yours. Also fine as a separate PR.
getFlattenedSteps() appends the synthetic outputs node whenever the composite declares outputs:, and that node runs under always(). Since anyStepExecuted is the only gate on reportWorkflowHookMetric, a hook side where every authored child was skipped still reports one eas.workflow.hook event with result: 'success' when the composite declares outputs, and reports nothing when it does not. Same hook, same outcome, different signal depending on whether an outputs: block happens to be there. I would rather the metric did not depend on that, but it is not urgent.
hSATAC
left a comment
There was a problem hiding this comment.
One thing I noticed while reviewing this: I don’t think this PR is doing anything wrong, but it does make an existing behavior difference more likely to surface for users.
Today, when a single hook entry expands into multiple steps, the later steps are gated differently depending on which side the hook runs on:
- In a
before_*hook, if an earlier step fails, later steps without anif:are skipped. - In an
after_*hook, later steps still run.
This PR preserves that existing behavior. The difference is that, until now, multi-step hook entries could only come from function groups that we own. We don’t have many function groups, and users are also unlikely to use them inside hooks, so this difference probably hasn’t been very visible in practice.
With composite functions, users can now define and reuse the same multi-step unit themselves:
# .eas/functions/publish/function.yml
runs:
steps:
- run: ./login.sh
- run: ./push.shWhen used in steps:, ./push.sh is skipped if ./login.sh fails. When the same composite is used in after_build, ./push.sh still runs. I verified this behavior on the branch.
I think this could be surprising because the author of a composite may not know where it will eventually be used, so the behavior isn’t something they can fully account for inside the composite itself.
I don’t think this needs to block the PR, but I wanted to flag it so we can decide whether we want to document the difference, align the default behavior for after_* hooks, or keep it as-is.
One related detail if we document it: success() and failure() inside a composite reflect the overall job status, not the result of the previous step. That means if: ${{ success() }} is not a reliable fail-fast guard inside an after_* hook, since the hook may already be running after the job has failed.
To express “only run if the previous step succeeded,” users would need to pass that state explicitly, for example:
- id: login
run: |
./login.sh
set-output ok true
outputs: [ok]
- run: ./push.sh
if: ${{ steps.login.outputs.ok == 'true' }}The same behavior applies to function groups, so whatever we decide here could cover both.
336986e to
bfe042d
Compare
98ef96f to
ff9cb50
Compare
|
@hSATAC Thank you for noting that, and yes, this was done like that for the sake of consistency. But taking a step back, as a user I'd expect composite functions/function groups to fast-fail within them even in the Corrected the behavior for both composite functions and function groups. |
ff9cb50 to
4a56ee2
Compare
bfe042d to
c692851
Compare
4a56ee2 to
70ce854
Compare
c692851 to
f9f51cc
Compare
70ce854 to
3702906
Compare
f9f51cc to
e693624
Compare
…er hooks, but continue after hook execution after entry failure
3702906 to
7136a56
Compare
|
⏩ The changelog entry check has been skipped since the "no changelog" label is present. |

Why
Hook steps currently reject
uses: ./...local composite functions. Users should be able to reuse the same composites in hooks as in the main workflow.How
A composite function used in a hook is expanded into its inner steps at parse time, the same way it is in the main workflow, and the whole expansion becomes a single hook entry. The step's
if:gates the expansion as a whole.Hook construction now takes a catalog of composite functions. Without it, referencing a composite fails with a missing-function error instead of the previous "not supported in hooks" error.
Wiring reading the composite functions files will be added in follow-up PRs in the stack.
Test Plan
Added unit tests.