fix: close the action header group before the action runs (#1345)

`uses:` steps and job hooks kept their "Run …" group open until they finished. Gitea doesn't nest groups, so the action's own first `::group::` closed the header and everything before it stayed hidden there, e.g. all of goreleaser's output in https://gitea.com/gitea/cli/actions/runs/1121069/jobs/1519275. The header group now closes right after its details, as in actions/runner.

The action header also matches actions/runner's otherwise now: it is titled by the `uses:` reference, lists the resolved inputs including `action.yml` defaults without empty ones, is shown for pre stages too, and post stages start with `Post job cleanup.`.

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: silverwind <me@silverwind.io>
Reviewed-on: https://gitea.com/gitea/runner/pulls/1345
Reviewed-by: silverwind <2021+silverwind@noreply.gitea.com>
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: bircni <bircni@icloud.com>

act/runner/action.go

@@ -186,6 +186,7 @@ func runActionImpl(step actionStep, actionDir string, remoteAction *remoteAction
 			return err
 		}
 		warnUnexpectedInputs(ctx, action, stepModel.With)
+		printRunActionHeader(ctx, stepModel, action, *step.getEnv(), rc)
 
 		actionName, containerActionDir := getContainerActionPaths(step)
 
@@ -504,10 +505,14 @@ func populateEnvsFromSavedState(env *map[string]string, step actionStep, rc *Run
 	}
 }
 
+func inputEnvKey(inputID string) string {
+	return "INPUT_" + inputEnvPattern.ReplaceAllString(strings.ToUpper(inputID), "_")
+}
+
 func populateEnvsFromInput(ctx context.Context, env *map[string]string, action *model.Action, rc *RunContext) error {
 	eval := rc.NewExpressionEvaluator(ctx)
 	for inputID, input := range action.Inputs {
-		envKey := "INPUT_" + inputEnvPattern.ReplaceAllString(strings.ToUpper(inputID), "_")
+		envKey := inputEnvKey(inputID)
 		if _, ok := (*env)[envKey]; !ok {
 			var err error
 			if (*env)[envKey], err = eval.Interpolate(ctx, input.Default); err != nil {
@@ -651,6 +656,7 @@ func runPreStep(step actionStep) common.Executor {
 				return err
 			}
 		}
+		printRunActionHeader(ctx, step.getStepModel(), action, *step.getEnv(), rc)
 		switch {
 		case x.IsNode():
 			if err := maybeCopyToActionDir(ctx, step, actionDir, actionPath, containerActionDir); err != nil {
@@ -749,6 +755,9 @@ func runPostStep(step actionStep) common.Executor {
 		actionDir, actionPath, _, containerActionDir := actionStagePaths(step)
 
 		x := action.Runs.Using
+		if !x.IsComposite() { // a composite's children print their own
+			logger.With(rawOutputField, true).Info("Post job cleanup.")
+		}
 		switch {
 		case x.IsNode():
 

act/runner/action_composite.go

@@ -31,7 +31,7 @@ func evaluateCompositeInputAndEnv(ctx context.Context, parent *RunContext, step
 	ee := parent.NewStepExpressionEvaluator(ctx, step)
 
 	for inputID, input := range step.getActionModel().Inputs {
-		envKey := "INPUT_" + inputEnvPattern.ReplaceAllString(strings.ToUpper(inputID), "_")
+		envKey := inputEnvKey(inputID)
 
 		// lookup if key is defined in the step but the already
 		// evaluated value from the environment

act/runner/action_composite_test.go

@@ -9,6 +9,7 @@ import (
 
 	"gitea.com/gitea/runner/act/common"
 	"gitea.com/gitea/runner/act/common/git"
+	"gitea.com/gitea/runner/act/common/logging"
 	"gitea.com/gitea/runner/act/common/logging/logtest"
 
 	"gitea.dev/actionslib/pkg/model"
@@ -19,8 +20,8 @@ import (
 
 func TestCompositeActionParity(t *testing.T) {
 	t.Run("inherits contexts without leaking inputs", func(t *testing.T) {
-		logger, hook := logtest.NewLogger()
-		ctx := common.WithLogger(t.Context(), logger)
+		hook := logtest.NewWithLevel(logging.LevelWarn)
+		ctx := common.WithLogger(t.Context(), logging.New(hook))
 		strategy := &model.Strategy{MaxParallelString: "3", RawMatrix: yaml.Node{Kind: yaml.MappingNode}}
 		parent := &RunContext{
 			Config:        &Config{PresetGitHubContext: &model.GithubContext{}},

act/runner/action_test.go

@@ -20,6 +20,7 @@ import (
 
 	"gitea.com/gitea/runner/act/common"
 	"gitea.com/gitea/runner/act/common/git"
+	"gitea.com/gitea/runner/act/common/logging"
 	"gitea.com/gitea/runner/act/common/logging/logtest"
 	"gitea.com/gitea/runner/act/container"
 
@@ -197,8 +198,8 @@ func TestActionRunner(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			logger, hook := logtest.NewLogger()
-			ctx := common.WithLogger(t.Context(), logger)
+			hook := logtest.NewWithLevel(logging.LevelWarn)
+			ctx := common.WithLogger(t.Context(), logging.New(hook))
 			cm := &containerMock{}
 			containerActionDir := "/var/run/act/actions/" + model.UsesHash("/org/repo/path@ref") + "/"
 			cm.On("CopyDir", containerActionDir, "dir/", false, true).Return(func(_ctx context.Context) error { return nil })
@@ -664,8 +665,8 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			logger, hook := logtest.NewLogger()
-			ctx := common.WithLogger(t.Context(), logger)
+			hook := logtest.NewWithLevel(logging.LevelWarn)
+			ctx := common.WithLogger(t.Context(), logging.New(hook))
 			tc.runs.Using, tc.runs.Image = "docker", "docker://node:14"
 			cm := &containerMock{}
 			var input *container.NewContainerInput

act/runner/job_hooks.go

@@ -46,12 +46,12 @@ func (rc *RunContext) runJobHook(ctx context.Context, hookPath, name string) err
 
 	cmd, shell := hookCommand(hookPath)
 	rawLogger := common.Logger(ctx).With(rawOutputField, true)
-	defer rawLogger.Infof("::endgroup::")
-	rawLogger.Infof("::group::Run '%s'", EscapeCommandData(hookPath))
 	rawLogger.Infof("A %s hook has been configured by the runner administrator", name)
+	rawLogger.Infof("::group::Run '%s'", EscapeCommandData(hookPath))
 	if shell != "" {
 		rawLogger.Infof("shell: %s", shell)
 	}
+	rawLogger.Infof("::endgroup::")
 
 	env := mergeMaps(rc.containerSpec.Env, rc.GetEnv())
 	rc.withGithubEnv(ctx, rc.getGithubContext(ctx), env)

act/runner/job_hooks_test.go

@@ -91,15 +91,29 @@ func newHookRunContext(jobContainer *hookContainer, config *Config) (*RunContext
 }
 
 func TestRunJobHook(t *testing.T) {
-	t.Run("runs the hook with the job environment", func(t *testing.T) {
-		jobContainer := &hookContainer{}
+	t.Run("runs the hook with the job environment after its closed header group", func(t *testing.T) {
+		jobContainer := &hookContainer{output: "from the hook\n"}
 		rc, ctx := newHookRunContext(jobContainer, &Config{
 			JobStartedHook: "/hooks/started.sh",
 			Env:            map[string]string{"A_VAR": "value", jobStartedHookEnv: "/from/env.sh"},
 		})
+		records := logtest.New()
+		ctx = common.WithLogger(ctx, logging.New(records))
 
 		require.NoError(t, rc.runJobStartedHook(ctx))
 
+		var lines []string
+		for _, record := range records.AllRecords() {
+			lines = append(lines, record.Message)
+		}
+		assert.Equal(t, []string{
+			"A job started hook has been configured by the runner administrator",
+			"::group::Run '/hooks/started.sh'",
+			"shell: bash -e {0}",
+			"::endgroup::",
+			"from the hook\n",
+		}, lines)
+
 		// The setting wins over the environment variable.
 		assert.Equal(t, []string{"bash", "-e", "/hooks/started.sh"}, jobContainer.cmd)
 		assert.Equal(t, "value", jobContainer.env["A_VAR"])

act/runner/step_action_builtin.go

@@ -33,9 +33,7 @@ func (sab *stepActionBuiltin) main() common.Executor {
 			return nil
 		}
 
-		printRunActionHeader(ctx, sab.Step, sab.env, sab.RunContext)
-		rawLogger := common.Logger(ctx).With(rawOutputField, true)
-		defer rawLogger.Infof("::endgroup::")
+		printRunActionHeader(ctx, sab.Step, nil, sab.env, sab.RunContext)
 
 		rc := sab.RunContext
 		rc.ApplyExtraPath(ctx, &sab.env)

act/runner/step_action_local.go

@@ -42,10 +42,6 @@ func (sal *stepActionLocal) main() common.Executor {
 			return nil
 		}
 
-		printRunActionHeader(ctx, sal.Step, sal.env, sal.getRunContext())
-		rawLogger := common.Logger(ctx).With(rawOutputField, true)
-		defer rawLogger.Infof("::endgroup::")
-
 		actionDir := filepath.Join(sal.getRunContext().Config.Workdir, sal.Step.Uses)
 		_, containerActionPath := getContainerActionPaths(sal)
 

act/runner/step_action_remote.go

@@ -153,12 +153,9 @@ func (sar *stepActionRemote) main() common.Executor {
 	return common.NewPipelineExecutor(
 		sar.prepareActionExecutor(),
 		runStepExecutor(sar, stepStageMain, func(ctx context.Context) error {
-			printRunActionHeader(ctx, sar.Step, sar.env, sar.RunContext)
-			rawLogger := common.Logger(ctx).With(rawOutputField, true)
-			defer rawLogger.Infof("::endgroup::")
-
 			github := sar.getGithubContext(ctx)
 			if sar.skipsLocalCheckout(sar.remoteAction, github) {
+				printRunActionHeader(ctx, sar.Step, nil, sar.env, sar.RunContext)
 				if sar.RunContext.Config.BindWorkdir {
 					common.Logger(ctx).Debugf("Skipping local actions/checkout because you bound your workspace")
 					return nil

act/runner/step_run.go

@@ -87,24 +87,34 @@ func (sr *stepRun) printRunScriptActionDetails(ctx context.Context) {
 }
 
 // printRunActionHeader mirrors actions/runner's "Run <action>" header for `uses:` steps,
-// including the with: inputs and the step-level env: block. The caller is responsible
-// for emitting ::endgroup:: after the action finishes.
-func printRunActionHeader(ctx context.Context, step *model.Step, env map[string]string, rc *RunContext) {
+// including the with: inputs and the step-level env: block.
+func printRunActionHeader(ctx context.Context, step *model.Step, action *model.Action, env map[string]string, rc *RunContext) {
 	if step == nil {
 		return
 	}
 	rawLogger := common.Logger(ctx).With(rawOutputField, true)
 
-	rawLogger.Infof("::group::Run %s", EscapeCommandData(cmp.Or(step.Name, step.Uses)))
+	rawLogger.Infof("::group::Run %s", EscapeCommandData(step.Uses))
 
-	if len(step.With) > 0 {
+	inputs := map[string]string{}
+	maps.Copy(inputs, step.With)
+	if action != nil {
+		for name := range action.Inputs {
+			if _, provided := lookupFold(step.With, name, true); !provided {
+				inputs[name] = env[inputEnvKey(name)]
+			}
+		}
+	}
+	maps.DeleteFunc(inputs, func(_name, value string) bool { return value == "" })
+	if len(inputs) > 0 {
 		rawLogger.Infof("with:")
-		for _, k := range slices.Sorted(maps.Keys(step.With)) {
-			logKeyedValue(rawLogger, k, step.With[k])
+		for _, k := range slices.Sorted(maps.Keys(inputs)) {
+			logKeyedValue(rawLogger, k, inputs[k])
 		}
 	}
 
 	printStepEnvBlock(ctx, step, env, rc)
+	rawLogger.Infof("::endgroup::")
 }
 
 // printStepEnvBlock emits the declared-env block (YAML order, internal vars filtered)

act/runner/step_run_print_test.go

@@ -118,10 +118,12 @@ env:
 
 func TestPrintRunActionHeaderGolden(t *testing.T) {
 	raw := `id: s1
+name: Checkout
 uses: actions/checkout@v4
 with:
   fetch-depth: "0"
-  token: secret
+  Token: secret
+  clean: ""
 env:
   CUSTOM: value
   GITHUB_TOKEN: tok
@@ -134,15 +136,19 @@ env:
 	entry := logger.With("job", "j1")
 	ctx := common.WithLogger(t.Context(), entry)
 
-	printRunActionHeader(ctx, &step, map[string]string{"CUSTOM": "value", "GITHUB_TOKEN": "tok"}, &RunContext{})
+	action := &model.Action{Inputs: map[string]model.Input{"token": {}, "clean": {}, "path": {}, "lfs": {}}}
+	env := map[string]string{"CUSTOM": "value", "GITHUB_TOKEN": "tok", "INPUT_TOKEN": "secret", "INPUT_CLEAN": "", "INPUT_PATH": "sub"}
+	printRunActionHeader(ctx, &step, action, env, &RunContext{})
 
 	want := strings.Join([]string{
 		"[j1]   | ::group::Run actions/checkout@v4",
 		"[j1]   | with:",
+		"[j1]   |   Token: secret",
 		"[j1]   |   fetch-depth: 0",
-		"[j1]   |   token: secret",
+		"[j1]   |   path: sub",
 		"[j1]   | env:",
 		"[j1]   |   CUSTOM: value",
+		"[j1]   | ::endgroup::",
 		"",
 	}, "\n")
 	assert.Equal(t, want, buf.String())

docs/job-hooks.md

@@ -47,7 +47,7 @@ A hook path that does not exist inside the job environment fails the job with `N
 
 A hook sees the job's environment: the workflow, job and `container:` `env:`, the runner's `envs`, and the `GITHUB_*` context variables, with the same masking applied to its output as to a step's. The step-specific ones (`GITHUB_ACTION`, `GITHUB_OUTPUT`, `GITHUB_STATE`) are not set — a hook is not a step, so `::save-state::` and `::set-output::` have nowhere to go.
 
-Its stdout is part of the job log, inside a collapsible group, and is scanned for workflow commands. `::add-mask::` registers a value to be masked for the rest of the job. `::set-env::` and `::add-path::` apply to the steps that follow, but as in a step they need `ACTIONS_ALLOW_UNSECURE_COMMANDS: true`, and a refused one fails the hook.
+Its stdout is part of the job log, after a collapsible group naming the hook and its shell, and is scanned for workflow commands. `::add-mask::` registers a value to be masked for the rest of the job. `::set-env::` and `::add-path::` apply to the steps that follow, but as in a step they need `ACTIONS_ALLOW_UNSECURE_COMMANDS: true`, and a refused one fails the hook.
 
 `$GITHUB_ENV` and `$GITHUB_PATH` point at files that are read back after the hook exits, so the file-command form works too: