diff --git a/core/src/main/java/com/google/adk/workflow/Graph.java b/core/src/main/java/com/google/adk/workflow/Graph.java index d2d1738fb..852456983 100644 --- a/core/src/main/java/com/google/adk/workflow/Graph.java +++ b/core/src/main/java/com/google/adk/workflow/Graph.java @@ -55,7 +55,10 @@ private Graph( this.terminalNodeNames = terminalNodeNames; } - /** Builds the graph that {@code edges} describe. */ + /** + * Builds the graph that {@code edges} describe, throwing {@link IllegalArgumentException} if a + * node's name is empty or contains {@code /}, {@code @}, or {@code .}. + */ static Graph of(List edges) { ImmutableList edgeList = ImmutableList.copyOf(edges); // Edges reuse a node by passing the same instance, so deduplicate by identity. @@ -69,6 +72,10 @@ static Graph of(List edges) { } } ImmutableList nodes = nodeList.build(); + // The scheduler starts graph nodes without a name check, so check every one here. + for (Node node : nodes) { + NodeNames.validate(node.name()); + } ImmutableMap nodesByName = nodes.stream().collect(toImmutableMap(Node::name, node -> node, (first, last) -> last)); ImmutableSet sourceNames = diff --git a/core/src/main/java/com/google/adk/workflow/JoinNode.java b/core/src/main/java/com/google/adk/workflow/JoinNode.java index 21d1a6ba1..5e739a05e 100644 --- a/core/src/main/java/com/google/adk/workflow/JoinNode.java +++ b/core/src/main/java/com/google/adk/workflow/JoinNode.java @@ -44,6 +44,7 @@ private JoinNode(Builder builder) { builder.inputSchema, builder.outputSchema, /* stateSchema= */ null); + NodeNames.validate(name()); } public static Builder builder() { @@ -95,6 +96,12 @@ public Builder outputSchema(@Nullable Schema outputSchema) { return this; } + /** + * Builds the join node. + * + * @throws IllegalArgumentException if the name is empty or contains {@code /}, {@code @}, or + * {@code .} + */ public JoinNode build() { checkState(name != null, "JoinNode.Builder requires name to be set."); return new JoinNode(this); diff --git a/core/src/main/java/com/google/adk/workflow/NodeNames.java b/core/src/main/java/com/google/adk/workflow/NodeNames.java new file mode 100644 index 000000000..383518f18 --- /dev/null +++ b/core/src/main/java/com/google/adk/workflow/NodeNames.java @@ -0,0 +1,45 @@ +/* + * Copyright 2026 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.adk.workflow; + +import static com.google.common.base.Preconditions.checkArgument; + +import com.google.adk.annotations.Experimental; +import com.google.common.base.CharMatcher; + +/** The rules a node name follows so that the node paths and branches built from it stay intact. */ +@Experimental +final class NodeNames { + + // '/' and '@' form node-path segments and '.' separates branch segments, so disallow all three. + static final CharMatcher PATH_SEPARATORS = CharMatcher.anyOf("/@."); + + private NodeNames() {} + + /** + * Throws {@link IllegalArgumentException} if {@code name} is empty or contains {@code /}, + * {@code @}, or {@code .}. + */ + static void validate(String name) { + checkArgument(!name.isEmpty(), "A node name must not be empty."); + checkArgument( + PATH_SEPARATORS.matchesNoneOf(name), + "A node name must not contain '/', '@', or '.', which separate node paths and branches," + + " but '%s' does.", + name); + } +} diff --git a/core/src/main/java/com/google/adk/workflow/NodeRunner.java b/core/src/main/java/com/google/adk/workflow/NodeRunner.java index e7b0258f8..ec2d5a228 100644 --- a/core/src/main/java/com/google/adk/workflow/NodeRunner.java +++ b/core/src/main/java/com/google/adk/workflow/NodeRunner.java @@ -16,7 +16,6 @@ package com.google.adk.workflow; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Strings.isNullOrEmpty; @@ -30,7 +29,6 @@ import com.google.adk.events.EventActions; import com.google.adk.telemetry.Tracing; import com.google.adk.workflow.ResumeScan.Interception; -import com.google.common.base.CharMatcher; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.errorprone.annotations.CanIgnoreReturnValue; @@ -66,9 +64,6 @@ @Experimental public final class NodeRunner { - // '/' and '@' form node-path segments and '.' separates branch segments, so disallow all three. - static final CharMatcher PATH_SEPARATORS = CharMatcher.anyOf("/@."); - /** Sink that receives the events of a workflow run. */ @Experimental interface EventSink { @@ -139,6 +134,8 @@ public static Flowable runRoot( checkNotNull(processEvent); return Flowable.defer( () -> { + // App checks a root node's name only against the agent-name rule, which allows '.'. + NodeNames.validate(node.name()); FlowableProcessor queue = UnicastProcessor.create().toSerialized(); Queue unprocessedFailures = new ConcurrentLinkedQueue<>(); @@ -216,12 +213,7 @@ private static Maybe startChild( + " the workflow reruns the caller on resume to collect the child's output.", parentState.node().name()); // No graph validates a child that a node runs itself, so check that it keeps paths intact. - checkArgument(!node.name().isEmpty(), "A node name must not be empty."); - checkArgument( - PATH_SEPARATORS.matchesNoneOf(node.name()), - "A node name must not contain '/', '@', or '.', but '%s' does. Agent names allow these" - + " characters, so an agent run as a node needs a name that a node path can carry.", - node.name()); + NodeNames.validate(node.name()); // A workflow takes its output from its terminal node, so it never delegates it to a child. boolean delegatesOutput = config.useAsOutput() && !(parentState.node() instanceof Workflow); if (delegatesOutput) { diff --git a/core/src/main/java/com/google/adk/workflow/RunNodeConfig.java b/core/src/main/java/com/google/adk/workflow/RunNodeConfig.java index 3720532b0..cb19b86c5 100644 --- a/core/src/main/java/com/google/adk/workflow/RunNodeConfig.java +++ b/core/src/main/java/com/google/adk/workflow/RunNodeConfig.java @@ -114,7 +114,7 @@ public final RunNodeConfig build() { "runId \"%s\" must contain a non-digit character.", runId); checkArgument( - NodeRunner.PATH_SEPARATORS.matchesNoneOf(runId), + NodeNames.PATH_SEPARATORS.matchesNoneOf(runId), "runId \"%s\" must not contain '/', '@', or '.'.", runId); }); diff --git a/core/src/main/java/com/google/adk/workflow/Workflow.java b/core/src/main/java/com/google/adk/workflow/Workflow.java index a14a8382c..c405595c4 100644 --- a/core/src/main/java/com/google/adk/workflow/Workflow.java +++ b/core/src/main/java/com/google/adk/workflow/Workflow.java @@ -53,6 +53,7 @@ private Workflow(Builder builder) { builder.inputSchema, builder.outputSchema, builder.stateSchema); + NodeNames.validate(name()); this.edges = builder.edges; this.maxConcurrency = builder.maxConcurrency; this.graph = edges.isEmpty() ? null : Graph.of(edges); @@ -201,6 +202,12 @@ public Builder stateSchema(@Nullable Schema stateSchema) { return this; } + /** + * Builds the workflow. + * + * @throws IllegalArgumentException if the workflow's name or the name of a node in its graph is + * empty or contains {@code /}, {@code @}, or {@code .} + */ public Workflow build() { checkState(name != null, "Workflow.Builder requires name to be set."); return new Workflow(this); diff --git a/core/src/test/java/com/google/adk/runner/RunnerRootNodeTest.java b/core/src/test/java/com/google/adk/runner/RunnerRootNodeTest.java index 6c922f2b3..6252eabfd 100644 --- a/core/src/test/java/com/google/adk/runner/RunnerRootNodeTest.java +++ b/core/src/test/java/com/google/adk/runner/RunnerRootNodeTest.java @@ -131,6 +131,28 @@ public void runAsync_nodeRoot_runsTheNodeOnTheUserMessage() { assertThat(inputs).containsExactly(USER_MESSAGE); } + @Test + public void runAsync_nodeRootWithANameThatANodePathCannotCarry_failsWithoutRunningIt() { + // App accepts the name, since an agent name may contain '.', but a node path cannot carry it. + AtomicInteger runs = new AtomicInteger(); + Node node = + new TestNode( + "a.b", + (context, input) -> { + runs.incrementAndGet(); + return Flowable.empty(); + }); + Runner runner = runner(node); + Session session = newSession(runner); + Flowable events = runner.runAsync(USER_ID, session.id(), USER_MESSAGE); + + IllegalArgumentException error = + assertThrows(IllegalArgumentException.class, events::blockingSubscribe); + + assertThat(error).hasMessageThat().contains("but 'a.b' does"); + assertThat(runs.get()).isEqualTo(0); + } + @Test public void runAsync_nodeRoot_persistsItsStampedEventAndState() { Node node = diff --git a/core/src/test/java/com/google/adk/workflow/GraphTest.java b/core/src/test/java/com/google/adk/workflow/GraphTest.java index 7a930c4b6..ad70c84b6 100644 --- a/core/src/test/java/com/google/adk/workflow/GraphTest.java +++ b/core/src/test/java/com/google/adk/workflow/GraphTest.java @@ -78,6 +78,51 @@ public void node_rejectsAnUnknownName() { assertThat(e).hasMessageThat().contains("missing"); } + @Test + public void of_rejectsANodeNameContainingASlash() { + // '/' separates the segments of a node path. + ImmutableList edges = ImmutableList.of(new Edge(Start.INSTANCE, TestNode.of("a/b"))); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> Graph.of(edges)); + + assertThat(e).hasMessageThat().contains("but 'a/b' does"); + } + + @Test + public void of_rejectsANodeNameContainingAnAtSign() { + // '@' separates a node's name from its run ID in a node path. + ImmutableList edges = ImmutableList.of(new Edge(Start.INSTANCE, TestNode.of("a@1"))); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> Graph.of(edges)); + + assertThat(e).hasMessageThat().contains("but 'a@1' does"); + } + + @Test + public void of_rejectsANodeNameContainingADot() { + // '.' separates branch segments; an agent name may contain one, a node name may not. + Node a = TestNode.of("a"); + ImmutableList edges = + ImmutableList.of(new Edge(Start.INSTANCE, a), new Edge(a, TestNode.of("a.b"))); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> Graph.of(edges)); + + assertThat(e).hasMessageThat().contains("but 'a.b' does"); + } + + @Test + public void of_rejectsAnEmptyNodeName() { + ImmutableList edges = ImmutableList.of(new Edge(Start.INSTANCE, TestNode.of(""))); + + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> Graph.of(edges)); + + assertThat(e).hasMessageThat().isEqualTo("A node name must not be empty."); + } + @Test public void nodesTriggeredBy_followsOnlyTheEdgeWithTheEmittedRoute() { Node router = TestNode.of("router"); diff --git a/core/src/test/java/com/google/adk/workflow/JoinNodeTest.java b/core/src/test/java/com/google/adk/workflow/JoinNodeTest.java index cc0bcef29..81d7c3801 100644 --- a/core/src/test/java/com/google/adk/workflow/JoinNodeTest.java +++ b/core/src/test/java/com/google/adk/workflow/JoinNodeTest.java @@ -73,6 +73,24 @@ public void build_rejectsAMissingName() { assertThat(e).hasMessageThat().contains("name"); } + @Test + public void build_rejectsANameThatANodePathCannotCarry() { + JoinNode.Builder builder = JoinNode.builder().name("gather@1"); + + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, builder::build); + + assertThat(e).hasMessageThat().contains("but 'gather@1' does"); + } + + @Test + public void build_rejectsAnEmptyName() { + JoinNode.Builder builder = JoinNode.builder().name(""); + + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, builder::build); + + assertThat(e).hasMessageThat().isEqualTo("A node name must not be empty."); + } + @Test public void name_rejectsNull() { JoinNode.Builder builder = JoinNode.builder(); diff --git a/core/src/test/java/com/google/adk/workflow/WorkflowTest.java b/core/src/test/java/com/google/adk/workflow/WorkflowTest.java index e4ea1318b..20e1fb104 100644 --- a/core/src/test/java/com/google/adk/workflow/WorkflowTest.java +++ b/core/src/test/java/com/google/adk/workflow/WorkflowTest.java @@ -187,6 +187,25 @@ public void build_rejectsAMissingName() { assertThat(e).hasMessageThat().contains("name"); } + @Test + public void build_rejectsANameThatANodePathCannotCarry() { + Workflow.Builder builder = Workflow.builder().name("support.flow"); + + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, builder::build); + + assertThat(e).hasMessageThat().contains("but 'support.flow' does"); + } + + @Test + public void build_rejectsANodeNameThatANodePathCannotCarry() { + Workflow.Builder builder = + Workflow.builder().name("flow").edges(new Edge(Start.INSTANCE, TestNode.of("a/b"))); + + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, builder::build); + + assertThat(e).hasMessageThat().contains("but 'a/b' does"); + } + @Test public void aWorkflowCanBeANodeOfAnotherWorkflow() { Workflow inner =