SED-3310 Local execution of AP should spawn and execute keywords on actual agents by david-stephan · Pull Request #690 · exense/step

david-stephan · 2026-08-18T09:03:56Z

No description provided.

…ctual agents

…should-spawn-and-execute-keywords-on-actual-agents

gemini-code-assist

Code Review

This pull request introduces a new module, step-agent-provisioning-local, enabling local execution of automation packages by running real Java, Node.js, and .NET agents as separate processes on the developer's machine. Key additions include an embedded execution grid, local agent providers, script engine library extraction, and CLI integration. The code review identifies several critical improvements: resolving a potential NullPointerException in LocalTokenSelectionCriteriaFilter, addressing concurrency and resource leak race conditions in LocalProcessAgentProvisioningDriver during agent provisioning and teardown, refactoring the blocking I/O output pump in LocalAgentProcess from the common ForkJoinPool to a dedicated daemon thread, and adding a fallback for non-atomic moves in ScriptEngineLibraries to support diverse file systems.

cl-exense

Approving with a few comments. In general, that's very solid and well-documented code, so most comments are more about understandability etc. than actual bugs (IIRC), and only very few of them are actual requests to change something.

cl-exense · 2026-08-19T08:55:14Z

+        }
+        logger.debug("Making {} executable", file);
+        if (!file.toFile().setExecutable(true)) {
+            logger.warn("Unable to make {} executable. Run 'chmod +x' on it if the agent fails to start.", file);


You'll want to log file.toAbsolutePath() just to be sure.

cl-exense · 2026-08-19T09:22:56Z

+        // The directory is named after the version, which does not change between two builds of the same one. An
+        // extracted agent is therefore only reused when it is also the same size as the embedded one, otherwise
+        // every CLI rebuilt during development would go on running the agent of the first build, for ever.
+        long embeddedSize = embeddedAgentSize();


This is not guaranteed to never produce "false negatives". While it's unlikely that the size remains the same over different builds, it's not impossible. I suggest to check the size AND the checksum to minimize the risk; the overhead will be extremely low (compared to everything else we're doing). Here's some simple code:

public long checksum(InputStream is) throws Exception { try (CheckedInputStream checkedStream = new CheckedInputStream(is, new CRC32C())) { checkedStream.transferTo(OutputStream.nullOutputStream()); return checkedStream.getChecksum().getValue(); } }

refactored to use the checksum available in the JAR (embedded) and storing the current one (extracted) on disk. Note that this only required for local dev, so we should not over engineer and limit the impact on the production code

cl-exense · 2026-08-19T09:27:48Z

+        try {
+            return Files.size(file);
+        } catch (IOException e) {
+            logger.debug("Unable to determine the size of {}", file, e);


Use .toAbsolutePath() to be sure.

cl-exense · 2026-08-19T09:40:01Z

+    private List<String> vmArgs() {
+        return Optional.ofNullable(configuration.getJavaAgentVmArgs())
+            .filter(args -> !args.isBlank())
+            .map(args -> List.of(args.trim().split("\\s+")))


This is reasonable, but still dangerous. Are we sure that individual arguments inside the args String can never contain spaces? (Contrived example for a shell: java -cp "C:/Program Files/crash boom bang/*"

changed to a support a list of VM arg options to avoid the list of splits all together

cl-exense · 2026-08-19T09:52:23Z

+        if (requiredAgentTypes.isEmpty()) {
+            // Either the keywords are unknown at this point, or none of them declares an agent type this machine can
+            // serve. Starting everything available is the only answer left which lets the execution run at all.
+            logger.debug("No agent type could be derived from the keywords. Starting one agent of each available type.");


Is that a meaningful tactic, i.e. does it actually solve the problem? IOW: is "we don't know the keywords that will run" a meaningful and expected situation?

I would expect to

clearly know what wants to run (e.g. ["java", ".net"]). It that's empty, then nothing needs to be spawned.

clearly know what from that list can be run (i.e. if no .net is installed, then it's simply impossible). If that's empty, there's still no need to spawn anything, because it won't help solve the problem anyway.

This path was incorrect, updated

cl-exense · 2026-08-20T10:15:13Z

+    /**
+     * The conversion words of logback's exception converters, {@code %ex} and {@code %throwable} with their variants
+     */
+    private static final List<String> EXCEPTION_CONVERSION_WORDS = List.of("%ex", "%exception", "%throwable",


The entire logger handling looks a bit like voodoo. I'm fine with keeping it, but suggest to move it into a separate utility class focused on that functionality, if possible.

cl-exense · 2026-08-20T10:16:54Z

            private String includeCategories;
            private String excludeCategories;
            private Map<String, String> executionParameters;
+            public step.agents.provisioning.local.LocalAgentProvisioningConfiguration localAgentConfiguration;


needlessly fully-qualified, I suppose?

Hehehe, thanks for the acknowledgement. Then why keep it? ;-)

Well I did not just all occurrences :-), updated now

cl-exense · 2026-08-20T10:33:59Z

@@ -24,15 +24,39 @@ public enum OperationMode {



General note on this class - both the constants and method names are rather confusing, even with the comments. It all sounds like it defines exceptions on top of exceptions on top of exceptions :-)

I can't really think of much better names off the top of my head, but we can have a short discussion/brainstorming if needed.

cl-exense

Two small nitpicks remaining.

cl-exense · 2026-09-01T07:58:28Z


        // all parameters
-        int res = runMain(histories, "ap", "execute", "-p=src/test/resources/samples/step-automation-packages-sample1.jar", "--local", "--includePlans=p1,p2", "--excludePlans=p3,p4", "--includeCategories=CatA,CatB", "--excludeCategories=CatC,CatD", "-ep=key1=value1|key2=value2", "-ep=key3=value3");
+        int res = runMain(histories, "ap", "execute", "-p=src/test/resources/samples/step-automation-packages-sample1.jar", "--local", "--includePlans=p1,p2", "--excludePlans=p3,p4", "--includeCategories=CatA,CatB", "--excludeCategories=CatC,CatD", "-ep=key1=value1|key2=value2", "-ep=key3=value3", "--localAgentVmArgs=-Xmx4g", "--localAgentVmArgs=-XX:HeapDumpPath=C:\\Program Files\\dumps");


This is a bad idea and is not platform-agnostic. It uses paths that will not be present on most systems (including windows, I don't know how it would behave there), and it would create a leftover directory (literally named "C:\Program Files\dumps") on non-windows machines. I assume that under normal circumstances we don't expect this directory to be used because we don't expect heap dumps, but still...

If this is absolutely required under some circumstances for debugging or so, I suggest to keep the relevant definitions as comments so they can easily be enabled when needed - but not as a default definition that will be used on normal test runs.

This works anywhere: 1. This test does not cover actual execution which is mocked, 2. the VM arg is HeapDumpPath which does nothing unless a heap dump is triggered. This test covers among other things that such arguments (including backslahes and spaces) are properly handled.

In that case, fine with me.

cl-exense · 2026-09-01T07:58:48Z

        Assert.assertEquals("CatC,CatD", usedParams.excludeCategories);
        Assert.assertEquals(Map.of("key1", "value1", "key2", "value2", "key3", "value3"), usedParams.executionParameters);
        Assert.assertEquals("step-automation-packages-sample1.jar", usedParams.apFile.getName());
+        Assert.assertEquals(List.of("-Xmx4g", "-XX:HeapDumpPath=C:\\Program Files\\dumps"),


Related to above comment.

If the purpose of the argument is actually purely for tests, then simply use a relative directory name like "./threaddumps" that's not OS-specific.

see previous comment

cl-exense · 2026-09-01T08:02:14Z

            private String includeCategories;
            private String excludeCategories;
            private Map<String, String> executionParameters;
+            public step.agents.provisioning.local.LocalAgentProvisioningConfiguration localAgentConfiguration;


Hehehe, thanks for the acknowledgement. Then why keep it? ;-)

…n-and-execute-keywords-on-actual-agents

…gent" This reverts commit dd28136.

cl-exense

Loked over the latest changes; while I trust you to better understand the full details, I don't see any glaring problems.

…n-and-execute-keywords-on-actual-agents

david-stephan added 14 commits August 11, 2026 10:33

SED-3310 Local execution of AP should spawn and execute keywords on a…

300b31b

…ctual agents

SED-3310 wiring and adding cli test with node.js keyword execution

b731428

SED-3310 agent shutdown fixes, and script engine support

4f96db5

SED-3310 fixing agent configuration, log support, debug mode...

41cc384

SED-3310 bumping grid

5a6aa5d

SED-3310 supporting global node.js agent install

a1dc335

SED-3310 supporting routing and plan agent configurations

28b4771

SED-3310 dotnet agent supports

d6e8485

SED-3310 improving logging

ce2cb05

SED-3310 maven module refactoring

b3da5c9

SED-3310 improving wrong dotnet path error handling

f7e0c6b

Merge branch 'refs/heads/master' into SED-3310-local-execution-of-ap-…

80a996e

…should-spawn-and-execute-keywords-on-actual-agents

SED-3310 adapting to spring boot packaging and other fixes

6f29242

SED-3310 improving debug logging mode

b728f89

gemini-code-assist Bot reviewed Aug 18, 2026

View reviewed changes

david-stephan added 2 commits August 18, 2026 15:12

SED-3310 Improving node.js process mgmt and PR feedbacks

ccc96aa

SED-3310 PR feedbacks

9d92b5d

david-stephan requested a review from cl-exense August 18, 2026 13:15

cl-exense requested changes Aug 20, 2026

View reviewed changes

david-stephan added 3 commits August 31, 2026 10:42

SED-3310 PR feedbacks part1

ebf39d3

SED-3310 PR feedbacks part2

d80bd5c

SED-4841 OperationMode refactoring

3074e24

cl-exense requested changes Sep 1, 2026

View reviewed changes

SED-3310 removing unrequired full qualified name

8aa8f29

cl-exense previously approved these changes Sep 1, 2026

View reviewed changes

SED-3310 bug fixing

85f9d31

david-stephan dismissed cl-exense’s stale review via 85f9d31 September 1, 2026 12:14

david-stephan and others added 3 commits September 2, 2026 09:28

Merge branch 'master' into SED-3310-local-execution-of-ap-should-spaw…

5eb61c2

…n-and-execute-keywords-on-actual-agents

SED-3310 skipping junit test requiring to install a node.js agent

dd28136

Revert "SED-3310 skipping junit test requiring to install a node.js a…

38bc97a

…gent" This reverts commit dd28136.

david-stephan added 2 commits September 3, 2026 14:50

SED-3310 align node agent wit java agent (embedded and simplification)

f9bba75

SED-3310 adding package-lock.json to git as best practice

6429c52

cl-exense previously approved these changes Sep 4, 2026

View reviewed changes

david-stephan added 2 commits September 8, 2026 10:56

Merge branch 'master' into SED-3310-local-execution-of-ap-should-spaw…

12a202b

…n-and-execute-keywords-on-actual-agents

SED-3310 resolve merge issues from master

b243a9e

david-stephan dismissed cl-exense’s stale review via b243a9e September 8, 2026 10:26

david-stephan added 3 commits September 8, 2026 13:01

SED-3310 unrelated logging fix

a267ca9

SED-3310 unrelated logging fix

4311dab

SED-3310 error handling for local execution with mvn coordinate

5e09e8a

Uh oh!

Conversation

david-stephan commented Aug 18, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cl-exense left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cl-exense left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl-exense left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!