fix: Require injection of user interaction functions for LLM nodes to be explicit
This commit is contained in:
3 files changed
+410
-13
No files matched your search
+74
-9
@@ -38,7 +38,15 @@ impl LlmNodeExecutor {
|
||||
parent_ctx: &mut RequestContext,
|
||||
abort: &AbortSignal,
|
||||
) -> Result<LlmExecutionOutcome> {
|
||||
let result = run(node_id, node, state_manager, parent_ctx, abort).await;
|
||||
let result = run(
|
||||
node_id,
|
||||
node,
|
||||
state_manager,
|
||||
parent_ctx,
|
||||
abort,
|
||||
&mut default_completion_runner(),
|
||||
)
|
||||
.await;
|
||||
finish(node, state_manager, parent_ctx, result, abort).await
|
||||
}
|
||||
}
|
||||
@@ -94,12 +102,15 @@ fn outcome_from(
|
||||
}
|
||||
}
|
||||
|
||||
/// The runner is a parameter for the same reason `finish` is split out:
|
||||
/// tests drive the node scope without a model.
|
||||
async fn run(
|
||||
node_id: &str,
|
||||
node: &LlmNode,
|
||||
state_manager: &mut StateManager,
|
||||
parent_ctx: &mut RequestContext,
|
||||
abort: &AbortSignal,
|
||||
runner: &mut CompletionRunner<'_>,
|
||||
) -> Result<String> {
|
||||
let mut instructions: Option<String> = match &node.instructions {
|
||||
Some(s) => Some(
|
||||
@@ -202,14 +213,11 @@ async fn run(
|
||||
node.mcp_tools.clone().map(|map| (node_id.to_string(), map)),
|
||||
);
|
||||
parent_ctx.refresh_mcp_tool_filters();
|
||||
let result = bounded_run(
|
||||
node,
|
||||
&prompt,
|
||||
parent_ctx,
|
||||
abort,
|
||||
&mut default_completion_runner(),
|
||||
)
|
||||
.await;
|
||||
// Inside the node, user__/todo__ builtins require explicit opt-in via the
|
||||
// node's `tools:` list; mem::replace so nested execution restores cleanly.
|
||||
let saved_in_node = std::mem::replace(&mut parent_ctx.in_graph_llm_node, true);
|
||||
let result = bounded_run(node, &prompt, parent_ctx, abort, runner).await;
|
||||
parent_ctx.in_graph_llm_node = saved_in_node;
|
||||
parent_ctx.role = saved_role;
|
||||
let node_jobs =
|
||||
std::mem::replace(&mut parent_ctx.node_job_scope, saved_job_scope).unwrap_or_default();
|
||||
@@ -1205,4 +1213,61 @@ mod tests {
|
||||
"{captured}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn node_scope_flag_is_set_during_the_run_and_restored_on_ok() {
|
||||
let node = node_with(None);
|
||||
let mut state = manager_with(&[]);
|
||||
let mut ctx = plain_ctx();
|
||||
ctx.agent = Some(Agent::test_new(AgentConfig::default()));
|
||||
let abort = create_abort_signal();
|
||||
|
||||
let out = run(
|
||||
"n",
|
||||
&node,
|
||||
&mut state,
|
||||
&mut ctx,
|
||||
&abort,
|
||||
&mut completion_runner(|_input, ctx, _abort| {
|
||||
assert!(
|
||||
ctx.in_graph_llm_node,
|
||||
"flag must be set while the node runs"
|
||||
);
|
||||
boxed_completion(async { Ok(("done".to_string(), vec![])) })
|
||||
}),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(out, "done");
|
||||
assert!(!ctx.in_graph_llm_node, "flag must not outlive the node");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn node_scope_flag_restores_prior_value_when_the_run_fails() {
|
||||
let node = node_with(None);
|
||||
let mut state = manager_with(&[]);
|
||||
let mut ctx = plain_ctx();
|
||||
ctx.agent = Some(Agent::test_new(AgentConfig::default()));
|
||||
ctx.in_graph_llm_node = true;
|
||||
let abort = create_abort_signal();
|
||||
|
||||
run(
|
||||
"n",
|
||||
&node,
|
||||
&mut state,
|
||||
&mut ctx,
|
||||
&abort,
|
||||
&mut completion_runner(|_input, _ctx, _abort| {
|
||||
boxed_completion(async { Err(anyhow!("boom")) })
|
||||
}),
|
||||
)
|
||||
.await
|
||||
.expect_err("the runner failure must surface");
|
||||
|
||||
assert!(
|
||||
ctx.in_graph_llm_node,
|
||||
"a nested run must restore the outer scope's flag"
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -3,6 +3,7 @@ use super::types::{ConcurrencyCap, Graph, NextTargets, Node, NodeType};
|
||||
use crate::client::{Model, ModelType};
|
||||
use crate::config;
|
||||
use crate::config::{Agent, AppConfig, paths};
|
||||
use crate::function::user_interaction::USER_FUNCTION_PREFIX;
|
||||
use crate::rag::{GraphRagConfig, RagData};
|
||||
use anyhow::{Result, bail};
|
||||
use std::collections::{BTreeMap, HashMap, HashSet, VecDeque};
|
||||
@@ -179,6 +180,7 @@ impl GraphValidator {
|
||||
self.validate_max_attempts(graph, &mut result);
|
||||
self.validate_fallback_capture(graph, &mut result);
|
||||
self.validate_output_schema_properties(graph, &mut result);
|
||||
self.validate_structured_output_user_tools(graph, &mut result);
|
||||
self.validate_max_concurrency(graph, &mut result);
|
||||
self.validate_max_concurrency_template(graph, &mut result);
|
||||
self.validate_orchestration_limits(graph, &mut result);
|
||||
@@ -458,6 +460,29 @@ impl GraphValidator {
|
||||
}
|
||||
}
|
||||
|
||||
fn validate_structured_output_user_tools(&self, graph: &Graph, result: &mut ValidationResult) {
|
||||
for (node_id, node) in &graph.nodes {
|
||||
let NodeType::Llm(llm) = &node.node_type else {
|
||||
continue;
|
||||
};
|
||||
if llm.output_schema.is_none() {
|
||||
continue;
|
||||
}
|
||||
let exposes_user_tools = llm.tools.as_ref().is_some_and(|tools| {
|
||||
tools
|
||||
.iter()
|
||||
.any(|t| t.trim().starts_with(USER_FUNCTION_PREFIX))
|
||||
});
|
||||
if exposes_user_tools {
|
||||
result.warning(ValidationError::with_node(
|
||||
node_id,
|
||||
"structured-output llm node exposes interactive user tools (`user__*`); \
|
||||
in headless runs a user__ call blocks the node as an escalation",
|
||||
));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn validate_node_references(&self, graph: &Graph, result: &mut ValidationResult) {
|
||||
for (node_id, node) in &graph.nodes {
|
||||
for (target, label) in declared_targets(node) {
|
||||
@@ -4685,6 +4710,89 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn structured_output_node_with_user_tools_warns() {
|
||||
let mut node = llm_with_output_schema("l", &["verdict"], Some("end"));
|
||||
if let NodeType::Llm(ref mut n) = node.node_type {
|
||||
n.tools = Some(vec!["user__select".into()]);
|
||||
}
|
||||
let graph = graph_with(vec![("l", node), ("end", end_node("end"))], "l");
|
||||
|
||||
let result = validator().validate(&graph);
|
||||
|
||||
assert!(
|
||||
result
|
||||
.warnings
|
||||
.iter()
|
||||
.any(|e| e.node_id.as_deref() == Some("l")
|
||||
&& e.message.contains("interactive user tools")),
|
||||
"expected user-tool warning: {:?}",
|
||||
result.warnings
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn structured_output_node_with_all_tools_does_not_warn() {
|
||||
// `all` is not a valid llm-node tools entry (validate_tools_subset
|
||||
// rejects it at run()), so this advisory stays silent about it.
|
||||
let mut node = llm_with_output_schema("l", &["verdict"], Some("end"));
|
||||
if let NodeType::Llm(ref mut n) = node.node_type {
|
||||
n.tools = Some(vec!["all".into()]);
|
||||
}
|
||||
let graph = graph_with(vec![("l", node), ("end", end_node("end"))], "l");
|
||||
|
||||
let result = validator().validate(&graph);
|
||||
|
||||
assert!(
|
||||
!result
|
||||
.warnings
|
||||
.iter()
|
||||
.any(|e| e.message.contains("interactive user tools")),
|
||||
"unexpected user-tool warning: {:?}",
|
||||
result.warnings
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn structured_output_node_without_user_tools_does_not_warn() {
|
||||
let mut node = llm_with_output_schema("l", &["verdict"], Some("end"));
|
||||
if let NodeType::Llm(ref mut n) = node.node_type {
|
||||
n.tools = Some(vec![]);
|
||||
}
|
||||
let graph = graph_with(vec![("l", node), ("end", end_node("end"))], "l");
|
||||
|
||||
let result = validator().validate(&graph);
|
||||
|
||||
assert!(
|
||||
!result
|
||||
.warnings
|
||||
.iter()
|
||||
.any(|e| e.message.contains("interactive user tools")),
|
||||
"unexpected user-tool warning: {:?}",
|
||||
result.warnings
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn non_structured_node_with_user_tools_does_not_warn() {
|
||||
let mut node = llm_node("l", None, Some("end"));
|
||||
if let NodeType::Llm(ref mut n) = node.node_type {
|
||||
n.tools = Some(vec!["user__select".into()]);
|
||||
}
|
||||
let graph = graph_with(vec![("l", node), ("end", end_node("end"))], "l");
|
||||
|
||||
let result = validator().validate(&graph);
|
||||
|
||||
assert!(
|
||||
!result
|
||||
.warnings
|
||||
.iter()
|
||||
.any(|e| e.message.contains("interactive user tools")),
|
||||
"unexpected user-tool warning: {:?}",
|
||||
result.warnings
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fallback_with_output_schema_and_empty_state_updates_warns() {
|
||||
let mut node = llm_with_output_schema("l", &["summary"], Some("end"));
|
||||
|
||||
Reference in new issue
Block a user