From 3962d0d970ed3a6443f6632f722ed879af0154c9 Mon Sep 17 00:00:00 2001 From: silverwind Date: Wed, 23 Sep 2026 15:13:49 +0000 Subject: [PATCH] fix: composite pre/post conditions read the job status (#1242) A composite action's child run context starts with empty step results, so a nested action's `post-if: success()` saw the composite's own result and still ran after the job had failed, which let `actions/cache` save caches from failed jobs. The `job` context now always comes from the job's run context. Only `success()` and `failure()` in a composite's own main steps read the composite's result, matching GitHub Actions. Assisted-by: Claude Code:claude-fable-5-1 Reviewed-on: https://gitea.com/gitea/runner/pulls/1242 Reviewed-by: bircni Co-authored-by: silverwind --- act/runner/action_composite_test.go | 48 +++++++++++++++++++++++++++++ act/runner/expression.go | 8 ++--- act/runner/run_context.go | 27 +++++++++------- act/runner/step.go | 7 ++++- 4 files changed, 74 insertions(+), 16 deletions(-) diff --git a/act/runner/action_composite_test.go b/act/runner/action_composite_test.go index e71d30bb..a88bc0fb 100644 --- a/act/runner/action_composite_test.go +++ b/act/runner/action_composite_test.go @@ -117,3 +117,51 @@ func TestAppendUniqueMasksNoExponentialGrowth(t *testing.T) { assert.Equal(t, []string{"secret"}, parentMasks) } + +func TestCompositeStepIfReadsJobStatusExceptInItsOwnMainSteps(t *testing.T) { + failStep := func(rc *RunContext) { + rc.StepResults["failed"] = &model.StepResult{Conclusion: model.StepStatusFailure} + } + tests := []struct { + name string + setup func(job, composite *RunContext) + nested bool + stage stepStage + want map[string]bool + }{ + {"post-if reads the failed job", func(job, _ *RunContext) { failStep(job) }, false, stepStagePost, map[string]bool{"success()": false, "failure()": true, "job.status == 'failure'": true}}, + {"main if ignores the failed job", func(job, _ *RunContext) { failStep(job) }, false, stepStageMain, map[string]bool{"success()": true, "failure()": false}}, + {"main if reads the composite's own failure", func(_, composite *RunContext) { failStep(composite) }, false, stepStageMain, map[string]bool{"success()": false, "failure()": true}}, + {"nested main if ignores the outer composite's failure", func(_, composite *RunContext) { failStep(composite) }, true, stepStageMain, map[string]bool{"success()": true}}, + {"main if sees the cancelled job", func(job, _ *RunContext) { job.markCancelled() }, false, stepStageMain, map[string]bool{"cancelled()": true, "success()": false}}, + {"post-if sees the cancelled job", func(job, _ *RunContext) { job.markCancelled() }, false, stepStagePost, map[string]bool{"cancelled()": true, "success()": false}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + job := makeTestRC(t, "runs-on: ubuntu-latest") + composite := newTestCompositeRunContext(t, job) + rc := composite + if tt.nested { + rc = newTestCompositeRunContext(t, composite) + } + tt.setup(job, composite) + step := &stepRun{RunContext: rc} + for expr, want := range tt.want { + enabled, err := isStepEnabled(t.Context(), expr, step, tt.stage) + require.NoError(t, err) + assert.Equal(t, want, enabled, expr) + } + }) + } +} + +func newTestCompositeRunContext(t *testing.T, parent *RunContext) *RunContext { + composite, err := newCompositeRunContext(t.Context(), parent, &stepActionRemote{ + Step: &model.Step{}, + RunContext: parent, + action: &model.Action{}, + env: map[string]string{}, + }, "/action") + require.NoError(t, err) + return composite +} diff --git a/act/runner/expression.go b/act/runner/expression.go index d2106235..573211e5 100644 --- a/act/runner/expression.go +++ b/act/runner/expression.go @@ -127,15 +127,15 @@ var hashfiles string // NewStepExpressionEvaluator creates a new evaluator with the `inputs` of the enclosing workflow or composite action func (rc *RunContext) NewStepExpressionEvaluator(ctx context.Context, step step) *ExpressionEvaluator { - return rc.newStepExpressionEvaluator(ctx, step, rc.actionInputs) + return rc.newStepExpressionEvaluator(ctx, step, rc.actionInputs, rc.getJobContext()) } // NewActionInputsExpressionEvaluator creates a new evaluator with the step's own with: values as `inputs` func (rc *RunContext) NewActionInputsExpressionEvaluator(ctx context.Context, step step) *ExpressionEvaluator { - return rc.newStepExpressionEvaluator(ctx, step, inputsFromEnv(*step.getEnv())) + return rc.newStepExpressionEvaluator(ctx, step, inputsFromEnv(*step.getEnv()), rc.getJobContext()) } -func (rc *RunContext) newStepExpressionEvaluator(ctx context.Context, step step, stepInputs map[string]any) *ExpressionEvaluator { +func (rc *RunContext) newStepExpressionEvaluator(ctx context.Context, step step, stepInputs map[string]any, jobContext *model.JobContext) *ExpressionEvaluator { // todo: cleanup EvaluationEnvironment creation job := rc.Run.Job() strategy := make(map[string]any) @@ -157,7 +157,7 @@ func (rc *RunContext) newStepExpressionEvaluator(ctx context.Context, step step, ee := &exprparser.EvaluationEnvironment{ Github: step.getGithubContext(ctx), Env: *step.getEnv(), - Job: rc.getJobContext(), + Job: jobContext, Steps: rc.getStepsContext(), Secrets: getWorkflowSecrets(rc), Vars: getWorkflowVars(ctx, rc), diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 3e7e0d73..f293ced2 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -1339,21 +1339,26 @@ func trimToLen(s string, l int) string { return s } -func (rc *RunContext) getJobContext() *model.JobContext { - jobStatus := "success" +// ownStatus is the result of this context's own steps, which for a composite action is its action status +func (rc *RunContext) ownStatus() string { if rc.jobFailed { - jobStatus = "failure" + return "failure" } for _, stepStatus := range rc.StepResults { if stepStatus.Conclusion == model.StepStatusFailure { - jobStatus = "failure" - break + return "failure" } } + return "success" +} + +func (rc *RunContext) getJobContext() *model.JobContext { + job := rc.topLevelRunContext() + jobStatus := job.ownStatus() // A cancelled run takes precedence over success/failure so cancelled() is true and // success()/failure() are false, matching GitHub Actions: on cancellation only // always() and cancelled() steps run. - if rc.jobCancelled { + if job.jobCancelled { jobStatus = "cancelled" } @@ -1361,17 +1366,17 @@ func (rc *RunContext) getJobContext() *model.JobContext { Status: jobStatus, Services: map[string]model.JobService{}, // an empty map, never null } - if rc.jobContainerID != "" { - jobContext.Container.ID = rc.jobContainerID - jobContext.Container.Network = rc.jobNetworkName + if job.jobContainerID != "" { + jobContext.Container.ID = job.jobContainerID + jobContext.Container.Network = job.jobNetworkName } - for _, svc := range rc.serviceContainers { + for _, svc := range job.serviceContainers { if svc.info == nil { continue } jobContext.Services[svc.name] = model.JobService{ ID: svc.info.ID, - Network: rc.jobNetworkName, + Network: job.jobNetworkName, Ports: svc.info.Ports, } } diff --git a/act/runner/step.go b/act/runner/step.go index 9daf0d87..e42fd4c2 100644 --- a/act/runner/step.go +++ b/act/runner/step.go @@ -299,7 +299,12 @@ func isStepEnabled(ctx context.Context, expr string, step step, stage stepStage) defaultStatusCheck = exprparser.DefaultStatusCheckSuccess } - runStep, err := EvalBool(ctx, rc.NewStepExpressionEvaluator(ctx, step), expr, defaultStatusCheck) + // success() and failure() in a composite's own main steps read the composite's result, other stages the job's + jobContext := rc.getJobContext() + if rc.Parent != nil && stage == stepStageMain && jobContext.Status != "cancelled" { + jobContext.Status = rc.ownStatus() + } + runStep, err := EvalBool(ctx, rc.newStepExpressionEvaluator(ctx, step, rc.actionInputs, jobContext), expr, defaultStatusCheck) if err != nil { return false, fmt.Errorf("if-expression %q evaluation failed: %s", expr, err) }