Address CI log grouping review feedback

Author: osterman

cmd/cmd_utils.go

@@ -879,10 +879,16 @@ func executeCustomCommand(
 			log.Debug("Prepared environment with identity for custom command step", customCommandKeyIdentity, commandIdentity, customCommandKeyCommand, commandConfig.Name, "step", i)
 		}
 
-		// Whenever grouping is enabled (any mode), mark the subprocess environment
-		// so a nested `atmos` invocation skips its own grouping — CI providers do
-		// not support nested groups.
-		if ci.GroupingEnabled(&atmosConfig) {
+		// Determine step type - default to shell if not specified.
+		stepType := strings.TrimSpace(step.Type)
+		if stepType == "" {
+			stepType = "shell"
+		}
+
+		// If this step will be enclosed in a CI log group, mark the subprocess
+		// environment so a nested `atmos` invocation skips unsupported nested
+		// grouping.
+		if stepType != schema.TaskTypeExec && ci.ShouldPropagateLogGroupSentinel(&atmosConfig, ci.DimensionStep) {
 			env = append(env, ci.LogGroupSentinelEnv())
 		}
 
@@ -891,12 +897,6 @@ func executeCustomCommand(
 		commandToRun, err := e.ProcessTmpl(&atmosConfig, fmt.Sprintf("step-%d", i), step.Command, data, false)
 		errUtils.CheckErrorPrintAndExit(err, "", "")
 
-		// Determine step type - default to shell if not specified.
-		stepType := strings.TrimSpace(step.Type)
-		if stepType == "" {
-			stepType = "shell"
-		}
-
 		// Execute the step based on type.
 		//
 		// shell/exec/atmos use the legacy, cross-platform, child-reaping paths
@@ -907,9 +907,10 @@ func executeCustomCommand(
 		// processes on Linux (no process-group cleanup), so they stay on the
 		// legacy paths.
 		//
-		// The whole dispatch is wrapped in a collapsible CI log group (no-op
-		// outside CI / when disabled), labeled with the step name or command.
-		err = stepPkg.RunGrouped(&atmosConfig, step.Name, commandToRun, func() error {
+		// The dispatch is wrapped in a collapsible CI log group (no-op outside
+		// CI / when disabled), labeled with the step name or command. Exec steps
+		// run bare because a successful Unix exec never returns to close a group.
+		err = stepPkg.RunGroupedForType(&atmosConfig, step.Name, commandToRun, stepType, func() error {
 			switch stepType {
 			case "shell":
 				// Execute shell command (backward compatible).

internal/exec/workflow_utils.go

@@ -436,15 +436,6 @@ func ExecuteWorkflow(
 			workDir = "."
 		}
 
-		// Whenever grouping is enabled (any mode), mark the subprocess environment
-		// so a nested `atmos` invocation skips its own grouping — CI providers do
-		// not support nested groups. This applies even in invocation mode (where
-		// this step itself does not open a group): the parent's invocation group
-		// must stay flat.
-		if ci.GroupingEnabled(&atmosConfig) {
-			stepEnv = append(stepEnv, ci.LogGroupSentinelEnv())
-		}
-
 		// Clear progress line and re-render as permanent record before step execution.
 		// This ensures progress line appears as header, then step output below it.
 		if progressRenderer.IsEnabled() {
@@ -457,15 +448,26 @@ func ExecuteWorkflow(
 		if commandType != "shell" && commandType != schema.TaskTypeExec && commandType != "atmos" && !stepPkg.IsExtendedStepType(commandType) {
 			return errUtils.Build(errUtils.ErrInvalidWorkflowStepType).
 				WithTitle(WorkflowErrTitle).
+				WithExplanationf("Workflow `%s` step `%s` uses unsupported type `%s`.", workflow, step.Name, commandType).
+				WithContext("workflow", workflow).
+				WithContext("step", step.Name).
 				WithHintf("Step type '%s' is not supported", commandType).
 				WithHint("Each step must specify a valid type: 'atmos', 'shell', 'exec', or an interactive type like 'input', 'confirm', 'choose'").
 				WithExitCode(1).
 				Err()
 		}
 
+		// If this step will be enclosed in a CI log group, mark the subprocess
+		// environment so a nested `atmos` invocation skips unsupported nested
+		// grouping.
+		if commandType != schema.TaskTypeExec && ci.ShouldPropagateLogGroupSentinel(&atmosConfig, ci.DimensionStep) {
+			stepEnv = append(stepEnv, ci.LogGroupSentinelEnv())
+		}
+
 		// Wrap each step's output in a collapsible CI log group when grouping is
-		// active. RunGrouped is a no-op outside CI / when disabled.
-		err = stepPkg.RunGrouped(&atmosConfig, step.Name, command, func() error {
+		// active. Exec steps run bare because a successful Unix exec never returns
+		// to close a deferred group.
+		err = stepPkg.RunGroupedForType(&atmosConfig, step.Name, command, commandType, func() error {
 			switch commandType {
 			case "shell":
 				// Render command before execution if show.command is enabled.

pkg/ci/loggroup.go

@@ -68,9 +68,8 @@ var logGroupOut io.Writer = os.Stdout
 var logGroupDepth int32
 
 // LogGroupSentinelEnv returns the "KEY=VALUE" environment entry that
-// orchestrators append to a step/command subprocess's environment while
-// grouping is enabled, so nested `atmos` invocations skip re-grouping. Callers
-// append it only when GroupingEnabled reports true.
+// orchestrators append to a step/command subprocess's environment while that
+// subprocess is inside, or is about to be inside, a CI log group.
 func LogGroupSentinelEnv() string {
 	defer perf.Track(nil, "ci.LogGroupSentinelEnv")()
 
@@ -132,21 +131,32 @@ func grouper(atmosConfig *schema.AtmosConfiguration) (provider.LogGrouper, bool)
 	return lg, true
 }
 
-// GroupingEnabled reports whether CI log grouping is active for this run in any
-// dimension: a grouping mode other than "off" is configured, a grouping-capable
-// provider is detected, and no parent Atmos process already has grouping open.
-//
-// Orchestrators use it to decide whether to append LogGroupSentinelEnv to a
-// child subprocess's environment — which must happen whenever grouping is
-// enabled, regardless of which dimension the current command emits, so that a
-// nested `atmos` invocation never emits its own (nested) groups.
+// GroupingEnabled reports whether CI log grouping is available for this run in
+// any dimension: a grouping mode other than "off" is configured, a
+// grouping-capable provider is detected, and no parent Atmos process already has
+// grouping open.
 func GroupingEnabled(atmosConfig *schema.AtmosConfiguration) bool {
 	defer perf.Track(nil, "ci.GroupingEnabled")()
 
 	_, ok := grouper(atmosConfig)
 	return ok
 }
 
+// ShouldPropagateLogGroupSentinel reports whether a subprocess launched at dim
+// should inherit LogGroupSentinelEnv. It is true when a group is already open in
+// this Atmos process, or when the configured mode/provider would open a group at
+// dim. This keeps child Atmos invocations from suppressing their own grouping
+// unless a parent group actually exists or will be opened for this boundary.
+func ShouldPropagateLogGroupSentinel(atmosConfig *schema.AtmosConfiguration, dim Dimension) bool {
+	defer perf.Track(nil, "ci.ShouldPropagateLogGroupSentinel")()
+
+	if atomic.LoadInt32(&logGroupDepth) > 0 {
+		return true
+	}
+	_, ok := grouper(atmosConfig)
+	return ok && dimensionActive(resolveGroupMode(atmosConfig), dim)
+}
+
 // Group runs fn wrapped in the detected CI provider's log-group markers, named
 // `name`, when the configured mode selects the given dimension; otherwise it
 // simply calls fn. The group-end marker is always emitted (even when fn returns

pkg/ci/loggroup_test.go

@@ -211,6 +211,42 @@ func TestGroupingEnabled(t *testing.T) {
 	})
 }
 
+func TestShouldPropagateLogGroupSentinel(t *testing.T) {
+	tests := []struct {
+		name string
+		mode string
+		dim  Dimension
+		want bool
+	}{
+		{name: "auto step boundary will group", mode: GroupModeAuto, dim: DimensionStep, want: true},
+		{name: "auto phase boundary will group", mode: GroupModeAuto, dim: DimensionPhase, want: true},
+		{name: "auto invocation boundary will not group", mode: GroupModeAuto, dim: DimensionInvocation, want: false},
+		{name: "invocation step boundary will not group by itself", mode: GroupModeInvocation, dim: DimensionStep, want: false},
+		{name: "invocation boundary will group", mode: GroupModeInvocation, dim: DimensionInvocation, want: true},
+		{name: "off boundary will not group", mode: GroupModeOff, dim: DimensionStep, want: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			registerGrouping(t)
+			assert.Equal(t, tt.want, ShouldPropagateLogGroupSentinel(modeConfig(tt.mode), tt.dim))
+		})
+	}
+}
+
+func TestShouldPropagateLogGroupSentinel_WhenGroupAlreadyOpen(t *testing.T) {
+	registerGrouping(t)
+	var buf bytes.Buffer
+	setLogGroupOut(t, &buf)
+
+	err := Group(modeConfig(GroupModeInvocation), DimensionInvocation, "atmos workflow deploy", func() error {
+		assert.True(t, ShouldPropagateLogGroupSentinel(modeConfig(GroupModeInvocation), DimensionStep))
+		return nil
+	})
+
+	require.NoError(t, err)
+}
+
 func TestLogGroupSentinelEnv(t *testing.T) {
 	assert.Equal(t, "ATMOS_CI_LOG_GROUP_ACTIVE=1", LogGroupSentinelEnv())
 }

pkg/runner/step/loggroup.go

@@ -22,6 +22,18 @@ func RunGrouped(atmosConfig *schema.AtmosConfiguration, name, command string, fn
 	return ci.Group(atmosConfig, ci.DimensionStep, groupLabel(name, command), fn)
 }
 
+// RunGroupedForType is RunGrouped for ordinary step types, but deliberately
+// skips grouping for exec steps. Exec steps may replace the Atmos process on
+// Unix, so a deferred group close would never run after a successful handoff.
+func RunGroupedForType(atmosConfig *schema.AtmosConfiguration, name, command, stepType string, fn func() error) error {
+	defer perf.Track(nil, "step.RunGroupedForType")()
+
+	if strings.TrimSpace(stepType) == schema.TaskTypeExec {
+		return fn()
+	}
+	return RunGrouped(atmosConfig, name, command, fn)
+}
+
 // groupLabel picks the human-facing group label: the step name when present,
 // otherwise the resolved command.
 func groupLabel(name, command string) string {

pkg/runner/step/loggroup_test.go

@@ -44,3 +44,12 @@ func TestRunGrouped_PassthroughWhenInactive(t *testing.T) {
 	}))
 	assert.True(t, called)
 }
+
+func TestRunGroupedForType_ExecRunsBare(t *testing.T) {
+	called := false
+	require.NoError(t, RunGroupedForType(&schema.AtmosConfiguration{}, "name", "command", schema.TaskTypeExec, func() error {
+		called = true
+		return nil
+	}))
+	assert.True(t, called)
+}

pkg/workflow/executor.go

@@ -199,10 +199,10 @@ func (e *Executor) runSteps(params *WorkflowParams, steps []schema.WorkflowStep,
 		}
 
 		// Wrap each step's output in a collapsible CI log group when grouping is
-		// active. RunGrouped is a no-op outside CI / when disabled, so executeStep
-		// runs unchanged in the common case.
+		// active. Exec steps run bare because a successful Unix exec never returns
+		// to close a deferred group.
 		var stepResult stepResultInternal
-		_ = stepPkg.RunGrouped(params.AtmosConfig, step.Name, step.Command, func() error {
+		_ = stepPkg.RunGroupedForType(params.AtmosConfig, step.Name, step.Command, step.Type, func() error {
 			stepResult = e.executeStep(params, step, stepIdx)
 			return nil
 		})
@@ -337,8 +337,8 @@ func (e *Executor) executeStep(params *WorkflowParams, step *schema.WorkflowStep
 
 	// When this step's output is wrapped in a CI log group (see runSteps), mark
 	// the subprocess environment so a nested `atmos workflow`/custom-command
-	// invocation skips re-grouping (CI providers do not support nested groups).
-	if ci.GroupingEnabled(params.AtmosConfig) {
+	// invocation skips unsupported nested grouping.
+	if commandType != schema.TaskTypeExec && ci.ShouldPropagateLogGroupSentinel(params.AtmosConfig, ci.DimensionStep) {
 		stepEnv = append(stepEnv, ci.LogGroupSentinelEnv())
 	}