Reviewing files that changed from the base of the PR and between 64f1884 and ea33af4.

• internal/core/spec/builder.go
• internal/core/spec/builder_test.go
• internal/core/spec/dag_test.go
• internal/core/spec/errors.go
• internal/core/spec/loader_test.go
• internal/core/spec/step.go
• internal/core/spec/step_test.go
• internal/core/step.go
• internal/integration/multi_command_test.go
• internal/integration/shell_test.go
• internal/runtime/agent/agent_test.go
• internal/runtime/agent/reporter.go
• internal/runtime/agent/reporter_test.go
• internal/runtime/builtin/archive/archive.go
• internal/runtime/builtin/archive/archive_test.go
• internal/runtime/builtin/command/command.go
• internal/runtime/builtin/command/command_script.go
• internal/runtime/builtin/command/command_test.go
• internal/runtime/builtin/command/multi_command.go
• internal/runtime/builtin/docker/client.go
• internal/runtime/builtin/docker/executor.go
• internal/runtime/builtin/gha/executor.go
• internal/runtime/builtin/http/http.go
• internal/runtime/builtin/http/http_test.go
• internal/runtime/builtin/jq/jq.go
• internal/runtime/builtin/jq/jq_test.go
• internal/runtime/builtin/ssh/ssh.go
• internal/runtime/data.go
• internal/runtime/node.go
• internal/runtime/node_test.go
• internal/runtime/runner_helper_test.go
• internal/service/frontend/api/v1/dags.go
• internal/service/frontend/api/v2/transformer.go

• internal/integration/shell_test.go

• GitHub Check: Test on ubuntu-latest

internal/runtime/builtin/gha/executor.go (2)

210-217: Command extraction looks safe.

The bounds checking before accessing Commands[0] is appropriate, and the error message clearly guides users on the expected format.

---

219-219: Good practice trimming the command string.

Using the extracted command variable with strings.TrimSpace ensures clean input for action resolution.

internal/runtime/builtin/docker/client.go (2)

471-495: LGTM! Well-implemented background startup method.

The StartBackground method correctly delegates to CreateContainerKeepAlive, which properly handles all startup modes (keepalive, command, entrypoint), readiness waiting, and log pattern matching. The documentation is clear, error handling is appropriate, and logging follows structured logging patterns.

---

593-596: Good defensive programming for startup command fallback.

This fallback condition makes startNewContainer more self-contained by allowing it to use StartCmd when no explicit cmd parameter is provided in "command" startup mode. The validation check len(c.cfg.StartCmd) > 0 ensures the fallback only applies when StartCmd is configured, maintaining safe behavior.

While current callers already provide the cmd parameter, this change improves flexibility for future use cases without breaking existing behavior.

internal/runtime/builtin/command/command_test.go (1)

452-453: Test migration to Commands-based representation looks good.

The tests have been correctly migrated to use the new Commands: []core.CommandEntry{{Command: "...", Args: [...]}} format. The changes are consistent across all test cases and properly reflect the new data model where each step contains a slice of command entries rather than single Command/Args fields.

Also applies to: 461-462, 470-471, 565-566, 574-575, 583-584, 591-592, 811-814, 1114-1114, 1128-1129, 1381-1382, 2240-2241

internal/core/spec/builder.go (1)

199-199: Command detection correctly updated to use Commands slice.

The change from step.Command != "" to len(step.Commands) > 0 properly adapts the step prefix logic to the new multi-command data model.

internal/core/spec/errors.go (1)

34-34: Error constant added for multi-command validation.

The new error ErrExecutorDoesNotSupportMultipleCmd provides clear error messaging for executors that don't support multiple commands.

internal/service/frontend/api/v1/dags.go (1)

828-835: API transformation correctly adapted to Commands-based model.

The toStep function properly extracts command information from Commands[0] with appropriate guards for empty Commands. The fallback to empty values is appropriate for the API presentation layer.

internal/runtime/data.go (1)

146-154: Remove dead method SetArgs() or prevent invalid CommandEntry state.

Data.SetArgs() creates a CommandEntry with empty Command field, which contradicts all other code patterns where Command is always populated. Additionally, this method appears to be unused—no callers found in the codebase. Either remove it entirely or add validation to prevent creating invalid state. If keeping the method for future API compatibility, at least document why an empty Command is acceptable.

Likely an incorrect or invalid review comment.

internal/service/frontend/api/v2/transformer.go (1)

49-64: LGTM! Safe extraction from Commands slice with proper guard.

The logic correctly checks len(obj.Commands) > 0 before accessing Commands[0], ensuring no index-out-of-bounds panic. This maintains backward compatibility by exposing only the first command's data through the existing API fields.

Note: For steps with multiple commands, only the first command's details will be visible through Command, Args, and CmdWithArgs API fields. If multi-command visibility is needed in the API response in the future, consider adding a Commands field to api.Step.

internal/runtime/builtin/archive/archive.go (2)

39-47: LGTM! Proper migration to Commands-based access.

The validation correctly checks for commands in the new Commands slice and maintains the existing error for missing commands.

---

72-80: LGTM! Consistent command extraction in newExecutor.

The executor creation logic mirrors the validation pattern with proper bounds checking before accessing Commands[0].Command.

internal/runtime/agent/reporter.go (2)

259-266: LGTM! Correct use of CmdWithArgs for HTML rendering.

Properly extracts the full command string from Commands[0].CmdWithArgs with appropriate HTML escaping.

---

586-593: LGTM! Consistent with the other HTML renderer.

Same correct pattern as renderHTML, using CmdWithArgs for the command display.

internal/runtime/builtin/command/command_script.go (1)

88-106: LGTM! Clean validation logic migrated to Commands-based model.

The validation correctly handles all valid step configurations:

• Commands with Script
• Commands only
• Script only
• SubDAG

The hasCommands variable improves readability and the switch structure is clear.

internal/runtime/builtin/http/http_test.go (1)

28-38: LGTM! Tests properly migrated to Commands-based step construction.

Test cases correctly use the new Commands: []core.CommandEntry{{Command: "GET", Args: []string{...}}} pattern, consistent with the updated HTTP executor implementation.

internal/runtime/builtin/command/multi_command.go (2)

86-118: LGTM! Sequential execution with proper error handling and cancellation checks.

The implementation correctly:

• Executes commands in order
• Captures exit code after each command
• Stops on first failure with descriptive error
• Checks context cancellation between commands
• Uses mutex to protect shared state

---

30-55: The context handling in this code is correct. The cfg.Ctx stored in the config does not control execution—commandExecutor.Run(ctx) receives and uses the context parameter passed from multiCommandExecutor.Run(), which is the expected behavior. The concern about stale context is unfounded.

internal/runtime/agent/agent_test.go (2)

243-245: LGTM! Test correctly migrated to Commands-based step modification.

The retry test properly updates steps using the new Commands slice structure, maintaining both Command and CmdWithArgs for execution and display purposes.

---

316-318: LGTM! Consistent with the other retry test.

Same correct pattern applied to the StepRetry test case.

internal/runtime/builtin/archive/archive_test.go (3)

28-39: LGTM! Test correctly uses Commands-based step construction.

The extract test properly constructs the step with Commands: []core.CommandEntry{{Command: opExtract}}, aligning with the updated archive executor implementation.

---

87-98: LGTM! Consistent pattern for create operation test.

Same correct structure used for the tar.gz creation test.

---

135-144: LGTM! Consistent pattern for list operation test.

All three archive operation tests (extract, create, list) now correctly use the Commands-based step structure.

internal/runtime/runner_helper_test.go (1)

86-91: LGTM!

The test helper correctly migrates to the new Commands slice structure. Using cmdutil.SplitCommand ensures consistent parsing, and preserving CmdWithArgs maintains display fidelity.

internal/runtime/builtin/command/command.go (2)

293-314: LGTM!

The config derivation correctly handles both populated and empty Commands slices. When Commands is empty, command and args remain zero values, which the downstream logic handles appropriately.

---

273-286: Multi-command support is properly implemented with correct sequential execution and edge case handling.

The branching logic correctly delegates to newMultiCommandExecutor when len(step.Commands) > 1. The implementation iterates through all commands sequentially, stops on first failure, and handles context cancellation between commands. Edge cases are safe: empty Commands slice and single-command path both route to NewCommandConfig, which safely checks len(step.Commands) > 0 before accessing Commands[0]. All Go conventions are followed (tabs, error handling, thread-safe mutex, proper naming).

internal/runtime/node.go (2)

428-468: Well-structured multi-command evaluation logic.

The implementation correctly:

1. Deep-clones the Commands slice to prevent race conditions
2. Evaluates each command entry's Args independently
3. Sets ShellCmdArgs only for the first command when using shell executor

One observation: The CmdWithArgs field is preserved from the original entry without re-evaluation (line 446), which is intentional for maintaining the original command string for display purposes.

---

470-562: Legacy field migration paths look correct.

All legacy formats (CmdArgsSys, CmdWithArgs, Command with/without Args) are properly converted to the new Commands structure. This ensures backward compatibility with existing JSON serialization.

internal/core/spec/loader_test.go (5)

116-117: LGTM!

Tests correctly updated to validate the new Commands structure. The pattern of checking Len(Commands, 1) before accessing Commands[0].Command is safe and consistent.

---

258-259: LGTM!

Consistent with the updated data model.

---

280-281: LGTM!

Test assertions properly adapted.

---

348-349: LGTM!

Multi-DAG test correctly validates Commands structure.

---

358-359: LGTM!

Consistent pattern maintained across test cases.

internal/core/spec/step_test.go (6)

841-895: LGTM!

Test structure correctly updated to validate Commands slice and Script fields. The assertion that legacy fields (Command, Args, CmdWithArgs) remain empty ensures clean separation from the new data model.

---

897-1010: Comprehensive multi-command test coverage.

Excellent coverage of edge cases including:

• Single command in array
• Multiple commands with various argument patterns
• Quoted arguments and pipes
• Empty arrays and mixed empty/valid entries
• Non-string element conversion

The test at lines 973-980 validates that non-string types (int, bool, float) are converted to string commands, which is a useful flexibility for YAML parsing.

---

1012-1090: LGTM!

buildSingleCommand tests properly validate both command parsing and script fallback behavior for multiline commands.

---

1092-1147: LGTM!

buildMultipleCommands tests correctly validate array processing and empty-value filtering.

---

1149-1195: LGTM!

HasMultipleCommands tests cover all boundary conditions (0, 1, 2+ commands).

---

1859-2044: Thorough executor validation testing.

The test suite comprehensively validates which executor types support multiple commands:

• Supported: shell, command, docker, container, ssh (empty string executor type)
• Unsupported: jq, http, archive, github_action, mail, dag, parallel

This aligns with the semantic expectations for each executor type.

internal/runtime/node_test.go (2)

15-15: LGTM!

Import added for cmdutil to support command splitting in test helpers.

---

1042-1057: LGTM!

Test helpers correctly updated to populate Commands slice. The withNodeCmdArgs helper properly uses cmdutil.SplitCommand for consistent parsing, while withNodeCommand creates a minimal entry for simple command-only tests.

internal/runtime/builtin/ssh/ssh.go (1)

84-122: Multi-command SSH execution implemented correctly with one caveat.

The implementation properly handles SSH's constraint that each session can only call Run once by creating a new session per command. Context cancellation checks between commands and indexed error messages are good additions.

Potential issue with Kill() during execution: The e.session field is overwritten on each iteration (line 104), so calling Kill() during multi-command execution will only close the current session. However, since SSH commands run synchronously and Kill() would typically be called from a signal handler, this should work correctly in practice—the current running session is the one that needs to be killed.

internal/runtime/builtin/jq/jq_test.go (6)

169-170: LGTM!

Test correctly updated to use Commands slice with CmdWithArgs for the JQ query. The Script field remains at the Step level as it represents input data, not a command.

---

246-247: LGTM!

Consistent pattern maintained.

---

277-278: LGTM!

---

302-303: LGTM!

---

323-324: LGTM!

---

348-349: LGTM!

internal/runtime/agent/reporter_test.go (2)

56-58: LGTM! Test data correctly migrated to Commands structure.

The test data now uses the new Commands: []core.CommandEntry structure with proper field population (Command, Args). This aligns with the PR's multi-command support while maintaining backward compatibility for single-command tests.

---

231-232: Verify Commands slice is non-empty before accessing index 0.

The test asserts Len(Commands, 1) on line 231 before accessing Commands[0].Args[0] on line 232, which is correct defensive programming. However, consider consolidating these assertions for clarity.

internal/integration/multi_command_test.go (2)

17-43: LGTM! Comprehensive multi-command shell test coverage.

The test suite properly exercises multiple commands with output concatenation, environment variable expansion, failure scenarios, and sequential execution. The Windows skip and parallel execution are appropriately configured.

---

382-404: The test is well-written and error messages are sufficiently descriptive for production use. The validation already provides context including the executor type ("executor type "jq" only supports a single command"), field name, and command values. Temporary file cleanup via t.TempDir() is handled automatically. No changes needed.

internal/core/spec/dag_test.go (2)

1639-1654: LGTM! Handler tests correctly migrated to Commands structure.

All handler fields (Init, Exit, Success, Failure, Cancel) are now properly accessed via Commands[0].CmdWithArgs. The assertions first verify the Commands slice length before accessing elements, which is defensive and correct.

---

1698-1708: Error path tests validate empty command handling.

The test correctly validates that an empty command (after trim) triggers an error. This ensures the new Commands-based validation maintains the same strictness as the legacy code.

internal/runtime/builtin/docker/executor.go (2)

186-236: LGTM! Existing container execution properly handles multiple commands.

The method correctly:

• Handles zero commands case with nil command execution
• Iterates Commands sequentially with proper error reporting
• Checks context cancellation between commands (lines 228-232)
• Tracks exit codes with mutex protection
• Augments errors with stderr tail when available

---

281-294: Update the comment to include all three startup modes.

The comment at lines 286-288 mentions only "startup:command" and "keepalive" modes, but the implementation supports a third mode: "entrypoint". Update the comment to reflect that StartBackground delegates to CreateContainerKeepAlive, which properly handles all three configured startup modes: "keepalive" (default), "command" (uses configured StartCmd), and "entrypoint" (respects image entrypoint/cmd).

internal/core/spec/builder_test.go (3)

495-511: Breaking change: Array commands now create multiple Commands entries.

The test now expects Len(Commands, 2) where each array element becomes a separate command. Previously, command: [echo, 1] was treated as a single command with args. This is a breaking behavioral change.

Verify this breaking change is documented in migration notes. The new behavior treats:

command: [echo, 1]  # Creates 2 commands: "echo" and "1"

differently from:

command: "echo 1"   # Creates 1 command: "echo" with arg "1"

Users might expect the array form to split command and args, not create multiple commands.

---

531-554: LGTM! Excellent test coverage for sequential multi-command execution.

This new test validates the core multi-command feature with proper assertions on Commands length and individual command parsing (Command, Args). The test data npm install, npm run build, npm test represents a realistic use case.

---

610-628: SubDAG migration correctly uses SubDAG.Name and SubDAG.Params.

The tests properly validate that both call and legacy run fields populate SubDAG.Name and SubDAG.Params instead of the deprecated Command/Args/CmdWithArgs fields. The deprecation warning check on line 628 is appropriate.

internal/core/spec/step.go (3)

582-602: LGTM! Clean type switch for command parsing.

The refactored buildStepCommand properly delegates to specialized helpers based on command type (string vs []any). Error handling returns a descriptive validation error for unsupported types.

---

604-634: Single command parsing correctly handles scripts and splits command/args.

The logic:

1. Trims whitespace
2. Detects multi-line as script (line 612)
3. Splits command using cmdutil.SplitCommand (line 618)
4. Populates Commands[0] with parsed data

This is correct and maintains backward compatibility with legacy single-command syntax.

---

636-681: Multiple command parsing handles array elements sequentially.

The implementation:

• Iterates array elements, converting non-strings via fmt.Sprintf (lines 645-650)
• Skips empty commands after trim (lines 653-655)
• Parses each command string into Command/Args/CmdWithArgs (lines 657-671)
• Provides per-command error context with command[%d] format (line 661)

This is well-designed for user-friendly error reporting.

Reviewing files that changed from the base of the PR and between ea33af4 and 7746156.

• internal/cmd/exec_spec.go
• internal/core/spec/step.go

• GitHub Check: Test on ubuntu-latest

internal/core/spec/step.go (4)

208-210: LGTM!

The validation is correctly placed after buildStepCommand and buildStepExecutor, ensuring that both result.Commands and result.ExecutorConfig.Type are populated before validation.

---

590-602: LGTM!

Clean type switch pattern that delegates to appropriate handler functions. The error case properly validates against unexpected types.

---

604-634: LGTM!

The function correctly handles edge cases: empty strings are rejected, multi-line content is treated as a script, and single-line commands are properly parsed. The CmdWithArgs field preserves the original trimmed command string for display/logging purposes.

---

698-718: LGTM!

The validation logic correctly handles edge cases:

• Early return for single or no commands avoids unnecessary checks.
• Empty executor type (default behavior) correctly bypasses validation since shell-like executors support multiple commands.
• Error message is informative with the executor type included.

internal/cmd/exec_spec.go (2)

40-42: LGTM!

The type change from []string to string for the Command field aligns with the new approach of storing a single shell-quoted command string. The YAML tag is appropriate.

---

72-72: LGTM!

The use of commandStr is consistent with the type change and properly utilizes the shell-quoted command string.

Reviewing files that changed from the base of the PR and between 7746156 and 518f81a.

• internal/integration/gha_test.go

• GitHub Check: Test on ubuntu-latest

internal/integration/gha_test.go (2)

4-4: LGTM: Import updated correctly.

The import change from os to os/exec is appropriate for the new git initialization logic.

---

17-29: The node:25-bookworm Docker image exists on Docker Hub and is available for use.

Likely an incorrect or invalid review comment.

Reviewing files that changed from the base of the PR and between 518f81a and 144a7bd.

• api/v2/api.gen.go
• api/v2/api.yaml
• internal/core/step.go
• internal/integration/container_test.go
• internal/runtime/builtin/command/command.go
• internal/runtime/builtin/command/command_test.go
• internal/runtime/env_test.go
• internal/runtime/executor/dag_runner_test.go
• internal/runtime/node.go
• internal/runtime/node_test.go
• internal/runtime/output_test.go
• internal/runtime/plan_test.go
• internal/runtime/runner_helper_test.go
• internal/runtime/runner_test.go
• internal/runtime/transform/status_test.go
• internal/service/frontend/api/v2/transformer.go
• ui/src/api/v2/schema.ts
• ui/src/components/ui/command-display.tsx
• ui/src/features/dags/components/dag-details/DAGStepTableRow.tsx
• ui/src/features/dags/components/dag-details/NodeStatusTableRow.tsx

• api/v2/api.yaml

• internal/service/frontend/api/v2/transformer.go

• GitHub Check: Test on ubuntu-latest
• GitHub Check: Build

internal/runtime/executor/dag_runner_test.go (1)

29-31: Step fixture migration to Commands looks correct

Using Commands: []core.CommandEntry{{Command: "echo", Args: []string{"hello"}}} aligns this test with the new multi-command model without changing behavior; no issues spotted.

internal/runtime/transform/status_test.go (1)

24-31: Status/Node fixtures now aligned with Step.Commands

All updated step fixtures correctly use Commands: []core.CommandEntry{...} while keeping existing assertions focused on names/status/preconditions, so behavior stays the same and exercises the new shape.

Also applies to: 60-62, 305-315

ui/src/features/dags/components/dag-details/DAGStepTableRow.tsx (1)

81-91: Command rendering correctly switched to multi-command API

Conditioning on step.commands?.length > 0 and passing commands={step.commands} matches the updated schema and the new CommandDisplay API; no additional handling needed here.

internal/runtime/plan_test.go (1)

59-62: Plan dependency tests now exercise Commands without altering semantics

Adding trivial Commands entries to the “basic” test steps keeps dependency expectations intact while validating the new command representation; looks good.

internal/runtime/node.go (1)

413-467: Multi-command evaluation logic looks sound and preserves executor semantics

The new evaluateCommandArgs correctly:

• Guards with cmdEvaluated.
• Applies cmdutil.WithoutExpandEnv() only for command executors with a shell.
• Evaluates each arg and (when set) CmdWithArgs per entry, then writes back a fresh []core.CommandEntry via SetStep.

This keeps behavior consistent while supporting multiple commands; no obvious edge-case regressions found.

ui/src/api/v2/schema.ts (1)

1219-1230: API schema correctly models commands and CommandEntry

Exposing Step.commands?: CommandEntry[] plus the new CommandEntry { command: string; args?: string[] } type aligns the UI schema with the backend’s multi-command model. Given this file is generated, the changes look consistent and sufficient for the new UI consumers.

Also applies to: 1328-1334

internal/runtime/output_test.go (1)

55-62: Output-focused node tests correctly migrated to Commands

All affected tests now construct steps via Commands with sh -c <python script> while preserving Output and assertions about size/behavior. This keeps the intent of the tests intact and ensures the new multi-command path is exercised.

Also applies to: 123-132, 174-183, 209-217, 247-257

internal/runtime/node_test.go (9)

15-15: LGTM!

The cmdutil import is correctly added and used by the withNodeCmdArgs helper function on line 1050.

---

1059-1066: LGTM!

The helper correctly sets both Command and CmdWithArgs to the same value. For simple commands like "true" or "sleep 3", the executor will use CmdWithArgs via the shell when Args is empty, which is the expected behavior.

---

676-681: LGTM!

The test data correctly uses the new Commands []core.CommandEntry structure with properly populated Command and Args fields.

---

736-741: LGTM!

Consistent with the new multi-command data model.

---

884-890: LGTM!

The Output field is correctly placed at the step level (not inside CommandEntry), which aligns with the core.Step structure where output capture is a step-level concern.

---

1171-1177: LGTM!

Test data correctly structured for output redirection testing with the new Commands model.

---

1213-1219: LGTM!

Consistent test data construction for relative path testing.

---

1260-1266: LGTM!

Shell command with stderr redirection correctly structured.

---

1308-1314: LGTM!

Nested path test case correctly uses the new Commands structure.

internal/runtime/builtin/command/command.go (2)

296-319: No action needed—backward compatibility is properly handled via UnmarshalJSON migration.

The Step struct includes an UnmarshalJSON implementation (internal/core/step.go:136-156) that automatically migrates legacy Command/Args fields to the Commands slice during JSON unmarshalling. When old-format configurations are deserialized, the migration happens before the code at lines 296-319 executes, so extracting from step.Commands[0] and falling back to step.ShellCmdArgs is correct and sufficient. No additional fallback to deprecated fields is needed in NewCommandConfig.

---

273-286: LGTM. The branching logic correctly routes to multiCommandExecutor when multiple commands exist. The newMultiCommandExecutor implementation properly iterates over step.Commands and constructs configs for each command. The single-command path handles both len(step.Commands) == 0 and len(step.Commands) == 1 cases via NewCommandConfig, with proper fallback to step.ShellCmdArgs. Code follows Go conventions with proper tab indentation, error wrapping, and naming patterns.

api/v2/api.gen.go (2)

869-871: Step.Commands shape looks consistent with multi‑command support

Using Commands *[]CommandEntry with omitempty matches how other optional lists are modeled in this file and cleanly represents sequential commands for a step; no issues from an API-shape perspective.

---

8476-8670: Swagger blob appears properly regenerated; just keep it source‑of‑truth driven

The updated swaggerSpec base64/gzip blob is expected after changing the OpenAPI for Step.commands/CommandEntry. As long as this file is entirely generated from the updated spec (e.g., via your normal oapi-codegen/make generate flow) and not hand‑edited, this block looks fine.

internal/runtime/builtin/command/command_test.go (1)

450-476: Single-command tests correctly migrated to Commands API

All updated tests now use core.Step{ Name: "…", Commands: []core.CommandEntry{{Command: "...", Args: ...}} }, which aligns with NewCommand’s len(step.Commands) > 1 switch and keeps single-command behavior unchanged. No further changes needed here.

Also applies to: 562-595, 810-815, 1112-1132, 1380-1383, 2239-2246

internal/runtime/runner_test.go (2)

2260-2306: Step retry DAG updated correctly to use Commands

The DAG in TestRunner_StepRetryExecution now uses Commands: []core.CommandEntry{{Command: "echo", Args: []string{"A"}}} etc., which matches the new multi-command representation and keeps the retry-plan semantics intact. The node construction from dag.Steps[i] lines up with this; no issues spotted.

---

2408-2606: Handler ID-access tests correctly exercise newHandlerStep and new handler wiring

The event-handler tests now:

• Build handlers via newHandlerStep(t, name, id, command) so each handler has a proper Commands entry and optional ID.
• Use withOnSuccess, withOnFailure, and withOnExit that now correctly assign to runtime.Config.OnSuccess / OnFailure / OnExit.
• Verify that handlers can reference main steps via ${id.stdout} / ${id.exit_code}, while cross-handler references remain unresolved.

This cleanly validates the new handler construction path and the environment semantics around step IDs. No further changes needed.

Reviewing files that changed from the base of the PR and between ed9a723 and d75c716.

• internal/cmd/exec_spec.go
• internal/core/spec/step.go
• internal/core/spec/step_test.go
• internal/core/step.go
• internal/core/step_test.go
• internal/integration/container_test.go
• internal/integration/gha_test.go
• internal/runtime/agent/reporter.go
• internal/runtime/builtin/command/command_test.go
• internal/runtime/env_test.go
• internal/runtime/node_test.go
• internal/runtime/runner_helper_test.go

• internal/runtime/agent/reporter.go

• GitHub Check: Build
• GitHub Check: Test on ubuntu-latest

internal/core/step_test.go (1)

280-318: LGTM! Well-structured table-driven test for CommandEntry.String().

The test covers the key scenarios including empty entry, command-only, command with args, and the priority behavior where CmdWithArgs takes precedence. This aligns with the coding guidelines for table-driven tests.

internal/cmd/exec_spec.go (2)

58-59: LGTM! Command string building using cmdutil.

The use of BuildCommandEscapedString properly handles argument escaping and quoting, which is necessary when converting from []string args to a single command string for YAML serialization.

---

39-43: Aligned with the new command model.

The execStep struct now uses a single Command string field which will be parsed by the spec loader into the Commands []CommandEntry structure during DAG loading.

internal/core/step.go (3)

118-141: Well-designed CommandEntry type with proper String() method.

The CommandEntry struct cleanly encapsulates command data, and the String() method correctly prioritizes CmdWithArgs for display purposes, falling back to building from Command and Args.

---

148-178: Migration logic now properly guards against dummy entries.

The UnmarshalJSON implementation correctly applies the fix from the past review: it only populates Commands when at least one legacy field (Command, Args, or CmdWithArgs) is actually set. This prevents creating dummy CommandEntry entries for non-command steps like SubDAG-only or script-only steps.

---

143-146: Simple and effective helper method.

HasMultipleCommands() provides a clean way to check for multi-command scenarios, which is used by NewCommand to decide between single and multi-command executors.

internal/integration/gha_test.go (1)

32-38: Good defensive checks for test prerequisites.

The git availability check and repository initialization are properly implemented. This setup ensures the test environment is correctly configured before running.

internal/runtime/builtin/command/command_test.go (2)

452-476: Test cases properly migrated to Commands-based model.

The exit code tests correctly use Commands: []core.CommandEntry{{...}} to construct steps, aligning with the new data model.

---

2296-2408: Comprehensive multi-command executor test suite.

Excellent coverage of the multi-command functionality:

• Sequential execution: Verifies commands run in order with file-based output verification
• Stop on first failure: Confirms execution halts and subsequent commands don't run
• Executor type selection: Validates single command uses commandExecutor, multiple uses multiCommandExecutor
• Exit code propagation: Ensures the failing command's exit code is properly captured

The tests follow table-driven patterns and use require/assert from testify as per coding guidelines.

internal/runtime/runner_helper_test.go (2)

80-91: Clean helper for parsing command strings into CommandEntry.

The parseCommand helper correctly uses cmdutil.SplitCommand to parse command strings and constructs a CommandEntry with all fields populated. Using panic is appropriate here since a parse failure indicates a test authoring bug that should fail fast.

---

93-97: withCommand updated to use new Commands model.

The withCommand step option now correctly builds a Commands slice with a single entry from the parsed command.

internal/runtime/env_test.go (1)

283-289: Test properly migrated to Commands-based step construction.

The step construction now correctly uses Commands: []core.CommandEntry{{...}} with all relevant fields populated, aligning with the new multi-command data model.

internal/integration/container_test.go (1)

35-36: Parallelization moved to outer test level.

Adding t.Parallel() at the test function level (rather than inside each subtest) is a valid approach when subtests may share state or when sequential execution within a test function is desired. This allows the test functions themselves to run in parallel with other tests while subtests execute sequentially.

Also applies to: 95-96, 523-524

internal/runtime/node_test.go (3)

1048-1061: Well-structured test helper for the new multi-command API.

The withNodeCmdArgs helper correctly uses cmdutil.SplitCommand to parse command strings and populates the new Commands slice with a CommandEntry. The use of t.Fatalf for parse errors is appropriate for test setup code.

---

1063-1070: LGTM!

The updated withNodeCommand helper correctly populates the Commands slice for simple command strings that don't require argument parsing.

---

109-192: Output tests properly adapted to use the new command parsing helper.

The output-related tests correctly use withNodeCmdArgs for command strings that contain quotes, escapes, and special characters, ensuring proper parsing through cmdutil.SplitCommand.

internal/core/spec/step_test.go (4)

837-895: Excellent test coverage for the refactored buildStepCommand function.

The test properly verifies:

• Single commands populate the Commands slice
• Multiline commands become scripts
• Legacy fields (Command, Args, CmdWithArgs) remain empty

---

939-945: Verify pipe handling behavior is intentional.

The test shows that cat file.txt | grep pattern is parsed with |, grep, and pattern as separate args. This is expected behavior from cmdutil.SplitCommand but means pipes won't work as shell pipes during direct command execution. Ensure this is the intended behavior, as users may expect shell-like pipe support.

---

1012-1090: Comprehensive unit tests for buildSingleCommand.

Good coverage of command parsing scenarios including multiline scripts, quoted arguments, and error cases for empty/whitespace-only commands.

---

1169-1215: LGTM!

Clear and concise tests for the HasMultipleCommands() helper method on core.Step.

internal/core/spec/step.go (5)

208-210: LGTM!

The validation is correctly placed after buildStepCommand to validate the parsed commands against the executor type.

---

582-601: Clean implementation of command type dispatching.

The function correctly handles the three cases: nil (no-op), string (single command), and array (multiple commands), with appropriate error handling for invalid types.

---

604-634: Well-implemented single command parsing.

The function correctly:

• Trims whitespace and validates non-empty
• Treats multiline strings as scripts
• Uses cmdutil.SplitCommand for proper shell-like parsing
• Populates only the new Commands field, leaving legacy fields empty

---

636-689: Robust multiple command parsing with good edge case handling.

The implementation properly:

• Converts primitive types to strings
• Rejects complex types (maps, arrays) with clear error messages
• Skips empty commands silently
• Validates that at least one valid command exists

Minor consideration: The type switch handles int, int64, uint64, float64, bool but not uint or int32. YAML parsers typically unmarshal to int or int64, so this should be sufficient.

---

706-726: LGTM!

The validation function is correctly implemented with clear logic and informative error messages. The early return for single/no commands is efficient.

Reviewing files that changed from the base of the PR and between d75c716 and b0d8588.

• internal/core/capabilities.go
• internal/core/spec/step.go
• internal/core/spec/step_test.go
• internal/runtime/builtin/archive/archive.go
• internal/runtime/builtin/command/command.go
• internal/runtime/builtin/dag/dag.go
• internal/runtime/builtin/dag/parallel.go
• internal/runtime/builtin/docker/executor.go
• internal/runtime/builtin/gha/executor.go
• internal/runtime/builtin/http/http.go
• internal/runtime/builtin/jq/jq.go
• internal/runtime/builtin/mail/mail.go
• internal/runtime/builtin/ssh/ssh.go
• internal/runtime/builtin/ssh/ssh_test.go
• internal/runtime/executor/executor.go

• internal/runtime/builtin/ssh/ssh_test.go

• internal/runtime/builtin/archive/archive.go
• internal/runtime/builtin/http/http.go

• GitHub Check: Test on ubuntu-latest
• GitHub Check: Build

internal/runtime/builtin/dag/parallel.go (1)

315-315: LGTM!

The parallel executor correctly registers with SubDAG: true capability, which accurately reflects its purpose of executing sub-DAGs in parallel. This is consistent with the dag executor registration pattern.

internal/runtime/executor/executor.go (1)

39-46: LGTM!

The updated RegisterExecutor function properly integrates the capabilities system. The unconditional call to core.RegisterExecutorCapabilities is correct since the zero-value of ExecutorCapabilities (all fields false) is a valid state representing an executor with no special capabilities.

internal/runtime/builtin/gha/executor.go (3)

210-219: LGTM!

The command extraction properly migrates from the legacy step.Command field to the new step.Commands slice. The nil-check before array access (len(step.Commands) > 0) prevents potential panics.

---

531-538: LGTM!

Consistent command extraction pattern with generateWorkflowYAML. Both functions use the same guard and access pattern for the Commands slice.

---

547-551: LGTM!

The GHA executor correctly registers with Command: true only (not MultipleCommands), which aligns with its design to execute a single GitHub Action per step.

internal/core/spec/step_test.go (6)

16-51: LGTM!

Excellent test setup using TestMain to register executor capabilities before tests run. This ensures tests are isolated from runtime init() functions and explicitly defines the capabilities needed for validation tests. As per coding guidelines, this follows good Go testing practices.

---

875-933: LGTM!

The test correctly validates that:

1. Single-line commands populate Commands slice (not legacy fields)
2. Multi-line commands become Script instead
3. Legacy fields (Command, Args, CmdWithArgs) remain empty

This ensures backward-compatible parsing while migrating to the new data model.

---

935-1048: LGTM!

Comprehensive table-driven tests for multiple command parsing covering:

• Array of commands with various argument forms
• Quoted arguments preservation
• Empty/whitespace handling
• Non-string type conversion

Good coverage of edge cases. As per coding guidelines, the table-driven approach is preferred for Go tests.

---

976-983: Consider shell handling for pipe characters.

The test expects cat file.txt | grep pattern to parse as separate args ["file.txt", "|", "grep", "pattern"]. This is technically correct for the current tokenization, but pipe handling typically requires shell execution. If users expect shell semantics, they should use the script field or ensure the shell executor is invoked.

This test documents the current behavior, which is acceptable.

---

1917-2102: LGTM!

Thorough validation tests for validateMultipleCommands covering:

• Single command always passes
• Multiple commands pass for multi-command executors (shell, docker, ssh)
• Multiple commands fail for single-command executors (jq, http, archive, gha, mail, dag)
• Empty commands always pass

Test cases align with the capabilities registered in TestMain.

---

2104-2198: LGTM!

Validation tests for script support cover all executor types appropriately, including the case where SSH does not support script (as per registered capabilities).

internal/core/capabilities.go (2)

1-17: LGTM!

Clean capability definition struct with well-documented fields. The boolean flags provide a clear contract for what each executor supports.

---

43-76: LGTM!

Public registration function and convenience accessors provide a clean API for the capabilities system. The one-liner accessors reduce boilerplate across the codebase.

internal/runtime/builtin/jq/jq.go (2)

43-55: LGTM!

The jq executor correctly extracts the query from Commands[0].CmdWithArgs, preserving the original query string which may contain spaces and special characters. Using CmdWithArgs instead of Command is the right choice here.

---

147-148: LGTM!

Registration with Command: true, Script: true correctly reflects the jq executor's usage pattern: the command field holds the jq query, and the script field holds the JSON input to process.

internal/runtime/builtin/mail/mail.go (1)

124-125: LGTM!

The mail executor correctly registers with an empty ExecutorCapabilities{}, indicating it doesn't support command, script, shell, or other step-level features. All mail configuration comes from the executor config block, which is the appropriate pattern for this executor type.

internal/runtime/builtin/dag/dag.go (1)

149-153: LGTM!

Clean registration of both subworkflow and dag executor types with shared SubDAG: true capability. Reusing the caps variable is efficient and ensures consistency between the two aliases.

internal/runtime/builtin/command/command.go (3)

328-336: LGTM!

The executor capabilities registration correctly advertises support for Command, MultipleCommands, Script, and Shell, enabling the multi-command execution path.

---

296-305: Potential index out of bounds when accessing step.Commands[0].

The code accesses step.Commands[0] without verifying that step.Commands is non-empty. While upstream validation may ensure at least one command exists, defensive programming would prevent a potential panic.

🔎 Suggested fix

 func NewCommandConfig(ctx context.Context, step core.Step) (*commandConfig, error) {
 	env := runtime.GetEnv(ctx)
 
 	var command string
 	var args []string
 	var shellCmdArgs string
 
 	// Extract command and args from the first command entry
-	if len(step.Commands) > 0 {
+	if len(step.Commands) > 0 {
 		command = step.Commands[0].Command
 		args = step.Commands[0].Args
 		shellCmdArgs = step.Commands[0].CmdWithArgs
 	}

Actually, the current code already has the length check on line 301. Let me reconsider...

Likely an incorrect or invalid review comment.

---

307-310: The backward compatibility for step.ShellCmdArgs is already properly implemented.

The field is defined in the Step struct with standard JSON tags and will be automatically unmarshalled from DAG files. The fallback logic at lines 308–310 is intentional and tested; it ensures that legacy DAGs using ShellCmdArgs instead of the Commands array continue to work. This pattern mirrors the deprecated Command field, which is explicitly documented as kept "for JSON backward compatibility."

internal/runtime/builtin/ssh/ssh.go (2)

84-88: LGTM!

Early return for empty commands is a clean guard clause.

---

125-130: Validation correctly moved to spec layer with capability-based checks.

The SSH executor registration with nil validator is appropriate. The spec layer (internal/core/spec/step.go) validates executor capabilities using core.SupportsCommand(), core.SupportsMultipleCommands(), and core.SupportsShell() checks, which query the capabilities registered via RegisterExecutor. No SSH-specific validation logic was lost—validation has been generalized to a capability-based approach applied uniformly across all executors.

internal/runtime/builtin/docker/executor.go (3)

186-200: LGTM!

The empty commands case correctly uses nil to invoke the image's default entrypoint, with proper error handling and stderr tail augmentation.

---

255-279: LGTM!

Single command path uses the straightforward Run approach with proper error handling and stderr tail reporting.

---

486-494: LGTM!

Executor capabilities registration correctly advertises support for Command, MultipleCommands, Script, Shell, and Container.

internal/core/spec/step.go (4)

597-617: LGTM!

Clean routing logic for single vs. multiple commands with proper type validation.

---

619-649: LGTM!

The single command builder correctly handles multi-line strings as scripts and properly splits command strings into Command/Args.

---

706-795: LGTM!

The validation functions follow a consistent pattern and properly leverage the capability system to enforce executor constraints. Clean separation of concerns between parsing and validation.

---

208-225: Validation sequence is well-structured.

The new validation calls (validateCommand, validateMultipleCommands, validateScript, validateShell, validateContainer, validateSubDAG) provide clear capability checking after command building. This enforces executor constraints early in the build process.

Reviewing files that changed from the base of the PR and between b0d8588 and 923f304.

• internal/core/capabilities.go
• internal/core/spec/errors.go
• internal/core/spec/step.go
• internal/core/spec/step_test.go
• internal/runtime/builtin/dag/dag.go
• internal/runtime/builtin/dag/parallel.go

• internal/runtime/builtin/dag/parallel.go
• internal/core/spec/errors.go
• internal/runtime/builtin/dag/dag.go

• GitHub Check: Test on ubuntu-latest
• GitHub Check: Build

internal/core/capabilities.go (3)

3-19: Well-designed capability model.

The ExecutorCapabilities struct clearly defines executor features with well-documented boolean fields. The field names are intuitive and the structure supports the multi-command and executor-specific validation requirements.

---

45-83: Clean and consistent public API.

The public API functions (RegisterExecutorCapabilities and the Supports* accessors) provide a clear, consistent interface for capability checks. The naming is intuitive and the implementation is straightforward.

---

21-28: Thread safety is properly handled by init()-time registration.

All RegisterExecutorCapabilities calls occur exclusively in init() functions across the builtin executors (command, docker, ssh, jq, http, mail, archive, gha, dag, parallel). Since Go serializes all init() functions before main() executes, all registrations complete before any concurrent access. After initialization, the map is only read via the public Get() and Supports*() functions, never written to. This pattern is safe and idiomatic Go.

internal/core/spec/step_test.go (5)

877-935: LGTM - Correct validation of new command model.

The test correctly validates that:

1. Commands slice is populated with the new CommandEntry structure
2. Legacy fields (Command, Args, CmdWithArgs) remain empty after build
3. Multiline commands are treated as scripts

This aligns with the design where Commands is the source of truth and legacy fields are kept only for JSON backward compatibility.

---

937-1050: Excellent comprehensive coverage for multiple commands.

This test suite thoroughly covers:

• Multiple command parsing with various argument forms
• Pipe handling in commands
• Empty command filtering
• Type coercion (primitives to strings)
• Edge cases and error conditions

The test data is well-structured and validates both the Commands slice and the expected emptiness of legacy fields.

---

1870-2577: LGTM - Comprehensive executor capability validation tests.

The validation test suite systematically covers all executor capabilities:

• Multiple commands support (shell, docker, ssh support; jq, http, archive don't)
• Script support (command, shell, docker, jq, http support; ssh, mail, archive don't)
• Shell configuration support (command, docker, ssh support; jq, http, mail don't)
• Container support (docker/container support; others don't)
• Sub-DAG support (dag, parallel support; others don't)
• Command support (most support; dag, subworkflow, parallel, mail don't)
• Worker selector support (dag, subworkflow, parallel support; others don't)

The test pattern is consistent and thorough, checking both supported and unsupported cases plus edge cases.

---

2579-2653: LGTM - Thorough conflict validation tests.

The test correctly validates mutual exclusivity constraints:

• Sub-DAG fields (call/run/parallel) cannot coexist with executor, command, script, or container
• Container cannot coexist with executor or script

This ensures users get clear validation errors when specifying incompatible configurations.

---

16-53: TestMain capability registrations correctly mirror production. All executor types in TestMain match their production counterparts in internal/runtime/builtin, confirming the test environment accurately reflects production behavior.

internal/core/spec/step.go (6)

195-241: LGTM - Well-structured validation pipeline.

The validation pipeline is properly ordered:

1. Early conflict validation (line 196) - Checks mutual exclusivity before building
2. Action-defining transformations (lines 201-218) - Builds container, parallel, subDAG, executor, command, params
3. Final capability validators (lines 221-240) - Validates executor capabilities after executor type is determined

This ordering ensures executor type is known before validating capability-dependent fields.

---

604-624: LGTM - Clean command parsing dispatch.

The function cleanly handles the three cases:

1. Nil command - no-op
2. String command - delegates to buildSingleCommand
3. Array command - delegates to buildMultipleCommands

Clear error message for invalid types.

---

627-656: LGTM - Correct single command parsing.

The function correctly:

1. Validates non-empty input
2. Treats multiline strings as scripts
3. Parses single-line commands into command and args
4. Populates the Commands slice (not legacy fields)

The use of cmdutil.SplitCommand ensures consistent parsing including pipe handling.

---

660-711: LGTM - Flexible multiple command parsing.

The function provides good flexibility:

1. Accepts arrays of strings and primitives (converted to strings)
2. Filters out empty entries automatically
3. Provides clear error messages with array indices
4. Validates final result is non-empty

The type coercion (lines 672-673) is useful for YAML where numbers might not be quoted.

---

713-817: LGTM - Consistent capability-based validation.

All validation functions follow a consistent pattern:

1. Early return if field is empty/nil
2. Check capability via core.Supports*() functions
3. Return clear validation error if unsupported

This leverages the executor capability registry introduced in the PR, providing centralized and maintainable validation logic.

---

819-850: LGTM - Clear conflict validation logic.

The function effectively prevents incompatible field combinations:

Sub-DAG conflicts (lines 827-840):

• Sub-DAG fields (call/run/parallel) cannot combine with executor, command, script, or container
• Ensures sub-DAG execution mode is not mixed with other execution modes

Container conflicts (lines 842-847):

• Container cannot combine with executor (container itself implies executor type)
• Container cannot combine with script (enforces use of command field instead)

The use of predefined error constants (e.g., ErrSubDAGAndExecutorConflict) ensures consistent error messages across the codebase.

Reviewing files that changed from the base of the PR and between 7445908 and 622fef5.

• internal/common/cmdutil/shell.go
• internal/common/cmdutil/shell_test.go
• internal/core/capabilities.go
• internal/core/capabilities_test.go
• internal/core/spec/step_test.go
• internal/runtime/builtin/ssh/ssh.go
• internal/runtime/builtin/ssh/ssh_test.go