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

171-175: Consider adding validation for MaxToolIterations.

The MaxToolIterations field accepts any integer value via the pointer, but negative or zero values would likely cause issues in the tool execution loop. Consider adding validation in buildStepLLM similar to how MaxTokens is validated.

Example validation to add in buildStepLLM

 	// Validate maxTokens if specified
 	if cfg.MaxTokens != nil {
 		if *cfg.MaxTokens < 1 {
 			return core.NewValidationError("llm.maxTokens", *cfg.MaxTokens,
 				fmt.Errorf("maxTokens must be at least 1"))
 		}
 	}

+	// Validate maxToolIterations if specified
+	if cfg.MaxToolIterations != nil {
+		if *cfg.MaxToolIterations < 1 {
+			return core.NewValidationError("llm.maxToolIterations", *cfg.MaxToolIterations,
+				fmt.Errorf("maxToolIterations must be at least 1"))
+		}
+	}

ui/src/features/dags/components/chat-history/StepMessagesTable.tsx (1)

169-186: Add ARIA state for the tool definitions toggle.

This improves accessibility and screen-reader context for the collapsible section.

♿ Proposed tweak

-          <button
+          <button
             onClick={() => setShowTools(!showTools)}
             className="w-full flex items-center gap-2 px-2 py-1 hover:bg-accent/50 text-left"
             type="button"
+            aria-expanded={showTools}
+            aria-controls="tool-definitions"
           >
...
-          {showTools && (
-            <div className="px-4 pb-2 space-y-2">
+          {showTools && (
+            <div id="tool-definitions" className="px-4 pb-2 space-y-2">

internal/runtime/builtin/chat/tool_executor_test.go (1)

6-9: Prefer require for test assertions.

Most assertions here should fail fast to keep diagnostics clean and align with test guidelines.

As per coding guidelines, consider switching to require.* for these assertions.

ui/src/features/dags/components/chat-history/ToolDefinitionCard.tsx (1)

27-43: Wrap long tool text to prevent overflow.

Tool names, descriptions, and default values can be long; add wrapping to keep the card compact and readable.
Based on learnings, add whitespace-normal break-words.

♻️ Proposed tweak

-    <div className="text-xs border-l-2 border-l-purple-500 pl-2 py-0.5">
-      <div className="font-mono font-medium text-purple-500">{tool.name}</div>
+    <div className="text-xs border-l-2 border-l-purple-500 pl-2 py-0.5 whitespace-normal break-words">
+      <div className="font-mono font-medium text-purple-500 break-words">
+        {tool.name}
+      </div>
       {tool.description && (
-        <div className="text-muted-foreground">{tool.description}</div>
+        <div className="text-muted-foreground break-words">{tool.description}</div>
       )}

internal/runtime/data.go (1)

515-526: Copy tool definitions in/out to avoid shared mutation.

Returning internal slices allows external mutation and can break thread-safety guarantees. Mirror the defensive copy pattern used elsewhere.

♻️ Proposed fix

 func (d *Data) SetToolDefinitions(tools []exec.ToolDefinition) {
 	d.mu.Lock()
 	defer d.mu.Unlock()
-	d.inner.State.ToolDefinitions = tools
+	if tools == nil {
+		d.inner.State.ToolDefinitions = nil
+		return
+	}
+	copied := make([]exec.ToolDefinition, len(tools))
+	copy(copied, tools)
+	d.inner.State.ToolDefinitions = copied
 }

 func (d *Data) GetToolDefinitions() []exec.ToolDefinition {
 	d.mu.RLock()
 	defer d.mu.RUnlock()
-	return d.inner.State.ToolDefinitions
+	if d.inner.State.ToolDefinitions == nil {
+		return nil
+	}
+	copied := make([]exec.ToolDefinition, len(d.inner.State.ToolDefinitions))
+	copy(copied, d.inner.State.ToolDefinitions)
+	return copied
 }

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

1257-1259: Consider using DAGName alias for DagName fields.

Keeps type consistency with the rest of the API models; update api/v2/api.yaml and regenerate if desired.

Also applies to: 1269-1271

internal/runtime/builtin/chat/tool_executor.go (2)

182-198: Consider aggregating all errors instead of returning only the first.

The Kill method collects errors but only returns the first one. This could hide failures when multiple DAGs fail to terminate.

♻️ Option: Return aggregated errors

+import "errors"
+
 func (e *ToolExecutor) Kill(sig os.Signal) error {
 	e.mu.Lock()
 	defer e.mu.Unlock()
 
 	var errs []error
 	for runID, subDAG := range e.runningDAGs {
 		if err := subDAG.Kill(sig); err != nil {
 			errs = append(errs, fmt.Errorf("failed to kill tool DAG %s: %w", runID, err))
 		}
 	}
 
-	if len(errs) > 0 {
-		return errs[0]
-	}
-	return nil
+	return errors.Join(errs...)
 }

---

266-278: Outputs iteration order is non-deterministic.

In formatToolResult, iterating over result.Outputs map produces non-deterministic output order in the fallback path (lines 271-275). While this is a fallback for JSON marshal errors, consider sorting keys for consistent output.

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

477-502: Tool calling uses non-streaming mode - document the trade-off.

The comment on line 498 notes streaming is disabled for tool calling because it "complicates tool call detection." This is a reasonable design choice, but users expecting streaming with tools should understand this limitation.

Consider adding a debug log or noting in documentation that streaming is automatically disabled when tools are configured.

internal/runtime/builtin/chat/tools.go (3)

44-47: Consider returning empty registry instead of nil for empty dagNames.

Returning nil for empty dagNames means callers must always nil-check. An empty registry with HasTools() == false would be safer.

♻️ Return empty registry instead of nil

 func NewToolRegistry(ctx context.Context, dagNames []string) (*ToolRegistry, error) {
 	if len(dagNames) == 0 {
-		return nil, nil
+		return &ToolRegistry{
+			tools:    make(map[string]*toolInfo),
+			dagNames: make(map[string]string),
+		}, nil
 	}

---

152-198: Regex could silently skip malformed parameters.

When toolParamRegex.FindStringSubmatch(part) returns nil, the parameter is silently skipped (line 173-175). Consider logging a warning for malformed parameters to aid debugging.

 		match := toolParamRegex.FindStringSubmatch(part)
 		if match == nil {
+			// Log warning for debugging malformed params
 			continue
 		}

---

111-123: Tool ordering is non-deterministic.

Iterating over r.tools map produces tools in non-deterministic order. This could lead to inconsistent tool ordering in LLM requests, which might affect reproducibility.

♻️ Sort tools by name for consistent ordering

+import "sort"
+
 func (r *ToolRegistry) ToLLMTools() []llmpkg.Tool {
 	if r == nil {
 		return nil
 	}
 
+	// Collect and sort tool names for deterministic ordering
+	names := make([]string, 0, len(r.tools))
+	for name := range r.tools {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+
 	tools := make([]llmpkg.Tool, 0, len(r.tools))
-	for _, info := range r.tools {
+	for _, name := range names {
+		info := r.tools[name]
 		tools = append(tools, llmpkg.Tool{
 			Type: "function",
 			Function: llmpkg.ToolFunction{
 				Name:        info.Name,
 				Description: info.Description,
 				Parameters:  buildJSONSchema(info.Params),
 			},
 		})
 	}
 
 	return tools
 }