[Fix #1604] Shell refactor

[fjtirado]

Fix #1604
Fix #1599

[fjtirado]

This appears here as difference because I copied right away the schema from open-workflow-specification/specification#1182. The format (just whitespaces) of this section was slightly different because of manual modifications I did with previous Jiras (to finally obtain the same schema but with different white spaces). This change is aligning that so both schemas (spec and sdk) will be identical when open-workflow-specification/specification#1182 is merged (which will be hopefully soon)

[fjtirado]

with this approach we are unavoidily losing the possibility of specifying arguments from environment. I think thats for good, rather than set the enviroment to specify arguments, specify then directly either harcdoded or throoug expressions
The use case that we are not covering is when the user knows that there is an enviroments variable in his SO (a preexisting one, not one we are setting) that should be used as parameter. For covering this case it might be interesting, either manual replacing of the args string after applying the jq expression (replace $PEPE with System.getEnv("PEPE") in the evaluated string) or add the possibility to access the env from jq expressions (which is interesting by itself)
@ricardozanini @mcruzdev In any case, I think is better to adresss that with a different issue

[ricardozanini]

Why can't we have environment? Not sure if I understood, since environment is part of the definition.

[fjtirado]

We are still populating the environment of the process with the data provided by the definition, but since theere is not longer a shell invocation, if we use enviroment data in the arguments of the invocation, it does not get replaced by the environment value as before.

Now

        command: echo 
        arguments: 
            - Hello $FIRST_NAME $LAST_NAME from env!
        environment:
            FIRST_NAME: John
            LAST_NAME: ${.lastName}

Will print in console
"Hello $FIRST_NAME $LAST_NAME from env!"

while deleted test was printing:

"Hello John Doe from env!"

(there was a lastName Doe in the jq input)

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[ricardozanini]

Why can't we have environment? Not sure if I understood, since environment is part of the definition.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

impl/test/src/test/resources/workflows-samples/run-shell/echo-jq.yaml:10

• The command scalar has a trailing space (echo ). In YAML plain scalars, trailing whitespace can be preserved depending on parser settings, which would make the allowlist check fail unexpectedly ("echo " != "echo").

          command: echo 

impl/test/src/test/java/io/serverlessworkflow/impl/test/RunShellExecutorTest.java:43

• With sh -c removed, the environment-variable behavior is still implemented via addEnviromment(...), but coverage for env passing was dropped when the env-based sample workflow was removed. Add a test that asserts environment variables are set (e.g., run printenv FIRST_NAME or env and check output), and include that command in the allowlist for the test application.

  @BeforeAll
  static void init() {
    appl = WorkflowApplication.builder().withAllowedCommands(List.of("ls", "echo")).build();
  }

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowApplication.java:152

• Collections.unmodifiableSet(builder.allowedCommands) wraps the Builder's mutable set without copying. If the same Builder instance is reused or mutated after build(), previously built WorkflowApplication instances will observe changes to allowedCommands, defeating immutability/thread-safety expectations.

    this.allowedCommands = Collections.unmodifiableSet(builder.allowedCommands);

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

impl/core/src/main/java/io/serverlessworkflow/impl/executors/RunShellExecutor.java:60

• command is passed through as-is for allowlist matching and execution; trailing/leading whitespace from YAML or expressions (e.g., command: echo ) will cause a false allowlist miss. Normalize the resolved command (e.g., strip()) before checking allowedCommands and building the ProcessBuilder argv. This also makes the error message clearer by quoting the command.

    String command = shellCommand.apply(workflowContext, taskContext, model);
    if (!workflowContext.definition().application().allowedCommands().contains(command)) {
      return CompletableFuture.failedFuture(
          new SecurityException(
              "Command "

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowApplication.java:280

• Use the diamond operator for consistency with the rest of the builder field initializations in this class (e.g., new ArrayList<>()).

    private Set<String> allowedCommands = new HashSet<String>();

impl/test/src/test/java/io/serverlessworkflow/impl/test/RunShellExecutorTest.java:43

• After the refactor away from sh -c, there is no longer any test that validates shell.environment is actually applied to the spawned process. Consider adding a Linux-only sample/test that runs a stable binary like env and asserts the provided variables are present in stdout (and include env in the allowlist for tests).

  @BeforeAll
  static void init() {
    appl = WorkflowApplication.builder().withAllowedCommands(List.of("ls", "echo")).build();
  }

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (3)

impl/core/src/main/java/io/serverlessworkflow/impl/executors/RunShellExecutor.java:55

• shellCommand/shellArguments are resolved at runtime and may legitimately evaluate to null (e.g., a jq expression returning null). ProcessBuilder will throw a NullPointerException if any argv element is null, and a null/blank command currently becomes a misleading "not allowed" SecurityException. Validate the resolved command/args and fail with a clear exception before constructing the ProcessBuilder.

    String command = shellCommand.apply(workflowContext, taskContext, model);
    if (!workflowContext.definition().application().allowedCommands().contains(command)) {
      return CompletableFuture.failedFuture(

impl/test/src/test/java/io/serverlessworkflow/impl/test/RunShellExecutorTest.java:54

• RunShell environment handling is still supported, but there is no longer a positive test that executes a shell command with shell.environment set (the remaining workflow that includes environment fails early due to missing command). Adding a passing env test would help prevent regressions in ScriptUtils.addEnviromment(...) wiring.

  @Test
  void testEcho() throws IOException {
    Workflow workflow =
        WorkflowReader.readWorkflowFromClasspath("workflows-samples/run-shell/echo.yaml");
    WorkflowModel model = appl.workflowDefinition(workflow).instance(Map.of()).start().join();

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowApplication.java:411

• The new allowlist API is specifically for RunShell execution, but the public names allowedCommands / withAllowedCommands are generic and could be misread as applying to other "command" concepts (e.g., container tasks). Consider renaming to allowedShellCommands / withAllowedShellCommands (matching the linked issue’s proposal) or otherwise clarifying scope in the API surface.

    public Builder withAllowedCommand(String command) {
      this.allowedCommands.add(command);
      return this;
    }

    public Builder withAllowedCommands(Collection<String> commands) {
      this.allowedCommands.addAll(commands);
      return this;
    }