fix: added on_other field for approval nodes so users can specify an alternative free-text target when none of the options match what they want
This commit is contained in:
@@ -291,6 +291,7 @@ nodes:
|
|||||||
routes:
|
routes:
|
||||||
"Yes": deploy
|
"Yes": deploy
|
||||||
"No": cancel
|
"No": cancel
|
||||||
|
on_other: cancel
|
||||||
deploy:
|
deploy:
|
||||||
type: end
|
type: end
|
||||||
cancel:
|
cancel:
|
||||||
|
|||||||
@@ -174,6 +174,13 @@ pub struct ApprovalNode {
|
|||||||
|
|
||||||
pub routes: HashMap<String, String>,
|
pub routes: HashMap<String, String>,
|
||||||
|
|
||||||
|
/// REQUIRED. The user_ask tool always permits a free-form "type your
|
||||||
|
/// own answer" response in addition to the listed `options`. When the
|
||||||
|
/// user supplies any answer that does NOT match a key in `routes`,
|
||||||
|
/// execution routes to this node. The free-form text is available to
|
||||||
|
/// downstream nodes via `state_updates` (e.g. `clarification: "{{choice}}"`).
|
||||||
|
pub on_other: String,
|
||||||
|
|
||||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
pub state_updates: Option<HashMap<String, String>>,
|
pub state_updates: Option<HashMap<String, String>>,
|
||||||
|
|
||||||
@@ -427,6 +434,7 @@ nodes:
|
|||||||
routes:
|
routes:
|
||||||
yes: i
|
yes: i
|
||||||
no: e
|
no: e
|
||||||
|
on_other: e
|
||||||
i:
|
i:
|
||||||
id: i
|
id: i
|
||||||
type: input
|
type: input
|
||||||
@@ -559,6 +567,7 @@ routes:
|
|||||||
approve: apply
|
approve: apply
|
||||||
reject: end_reject
|
reject: end_reject
|
||||||
edit: edit_loop
|
edit: edit_loop
|
||||||
|
on_other: edit_loop
|
||||||
"#;
|
"#;
|
||||||
let node: Node = serde_yaml::from_str(yaml).unwrap();
|
let node: Node = serde_yaml::from_str(yaml).unwrap();
|
||||||
let approval = match node.node_type {
|
let approval = match node.node_type {
|
||||||
|
|||||||
@@ -139,14 +139,10 @@ fn build_input_question(node: &InputNode, state_manager: &StateManager) -> Resul
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn resolve_approval_route(node: &ApprovalNode, choice: &str) -> Result<String> {
|
fn resolve_approval_route(node: &ApprovalNode, choice: &str) -> Result<String> {
|
||||||
node.routes.get(choice).cloned().ok_or_else(|| {
|
if let Some(target) = node.routes.get(choice) {
|
||||||
let mut available: Vec<&str> = node.routes.keys().map(String::as_str).collect();
|
return Ok(target.clone());
|
||||||
available.sort();
|
}
|
||||||
anyhow!(
|
Ok(node.on_other.clone())
|
||||||
"No route defined for choice '{choice}'. Available routes: {}",
|
|
||||||
available.join(", ")
|
|
||||||
)
|
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn apply_state_updates_with_var(
|
fn apply_state_updates_with_var(
|
||||||
@@ -234,7 +230,7 @@ mod tests {
|
|||||||
StateManager::new(map)
|
StateManager::new(map)
|
||||||
}
|
}
|
||||||
|
|
||||||
fn approval(options: &[&str], routes: &[(&str, &str)]) -> ApprovalNode {
|
fn approval(options: &[&str], routes: &[(&str, &str)], on_other: &str) -> ApprovalNode {
|
||||||
let mut r = HashMap::new();
|
let mut r = HashMap::new();
|
||||||
for (k, v) in routes {
|
for (k, v) in routes {
|
||||||
r.insert((*k).into(), (*v).into());
|
r.insert((*k).into(), (*v).into());
|
||||||
@@ -243,6 +239,7 @@ mod tests {
|
|||||||
question: "?".into(),
|
question: "?".into(),
|
||||||
options: options.iter().map(|s| (*s).into()).collect(),
|
options: options.iter().map(|s| (*s).into()).collect(),
|
||||||
routes: r,
|
routes: r,
|
||||||
|
on_other: on_other.into(),
|
||||||
state_updates: None,
|
state_updates: None,
|
||||||
timeout: None,
|
timeout: None,
|
||||||
on_timeout: None,
|
on_timeout: None,
|
||||||
@@ -289,19 +286,27 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn approval_route_lookup_returns_target_on_match() {
|
fn approval_route_lookup_returns_target_on_match() {
|
||||||
let node = approval(&["yes", "no"], &[("yes", "deploy"), ("no", "cancel")]);
|
let node = approval(
|
||||||
|
&["yes", "no"],
|
||||||
|
&[("yes", "deploy"), ("no", "cancel")],
|
||||||
|
"clarify",
|
||||||
|
);
|
||||||
assert_eq!(resolve_approval_route(&node, "yes").unwrap(), "deploy");
|
assert_eq!(resolve_approval_route(&node, "yes").unwrap(), "deploy");
|
||||||
assert_eq!(resolve_approval_route(&node, "no").unwrap(), "cancel");
|
assert_eq!(resolve_approval_route(&node, "no").unwrap(), "cancel");
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn approval_route_lookup_errors_on_unknown_choice() {
|
fn approval_route_lookup_falls_back_to_on_other_for_unknown_choice() {
|
||||||
let node = approval(&["yes", "no"], &[("yes", "deploy"), ("no", "cancel")]);
|
let node = approval(
|
||||||
let err = resolve_approval_route(&node, "maybe")
|
&["yes", "no"],
|
||||||
.unwrap_err()
|
&[("yes", "deploy"), ("no", "cancel")],
|
||||||
.to_string();
|
"clarify",
|
||||||
assert!(err.contains("'maybe'"), "got: {err}");
|
);
|
||||||
assert!(err.contains("yes") && err.contains("no"), "got: {err}");
|
assert_eq!(resolve_approval_route(&node, "maybe").unwrap(), "clarify");
|
||||||
|
assert_eq!(
|
||||||
|
resolve_approval_route(&node, "free-form text").unwrap(),
|
||||||
|
"clarify"
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
+26
-4
@@ -248,6 +248,7 @@ fn declared_targets(node: &Node) -> Vec<(String, &'static str)> {
|
|||||||
for v in a.routes.values() {
|
for v in a.routes.values() {
|
||||||
out.push((v.clone(), "approval 'routes'"));
|
out.push((v.clone(), "approval 'routes'"));
|
||||||
}
|
}
|
||||||
|
out.push((a.on_other.clone(), "approval 'on_other'"));
|
||||||
if let Some(t) = &a.on_timeout {
|
if let Some(t) = &a.on_timeout {
|
||||||
out.push((t.clone(), "'on_timeout'"));
|
out.push((t.clone(), "'on_timeout'"));
|
||||||
}
|
}
|
||||||
@@ -369,7 +370,7 @@ mod tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn approval_node(id: &str, options: &[&str], routes: &[(&str, &str)]) -> Node {
|
fn approval_node(id: &str, options: &[&str], routes: &[(&str, &str)], on_other: &str) -> Node {
|
||||||
let mut r: HashMap<String, String> = HashMap::new();
|
let mut r: HashMap<String, String> = HashMap::new();
|
||||||
for (k, v) in routes {
|
for (k, v) in routes {
|
||||||
r.insert((*k).into(), (*v).into());
|
r.insert((*k).into(), (*v).into());
|
||||||
@@ -381,6 +382,7 @@ mod tests {
|
|||||||
question: "?".into(),
|
question: "?".into(),
|
||||||
options: options.iter().map(|s| (*s).into()).collect(),
|
options: options.iter().map(|s| (*s).into()).collect(),
|
||||||
routes: r,
|
routes: r,
|
||||||
|
on_other: on_other.into(),
|
||||||
state_updates: None,
|
state_updates: None,
|
||||||
timeout: None,
|
timeout: None,
|
||||||
on_timeout: None,
|
on_timeout: None,
|
||||||
@@ -449,7 +451,12 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn flags_missing_approval_route_target() {
|
fn flags_missing_approval_route_target() {
|
||||||
let approval = approval_node("ap", &["yes", "no"], &[("yes", "end"), ("no", "missing")]);
|
let approval = approval_node(
|
||||||
|
"ap",
|
||||||
|
&["yes", "no"],
|
||||||
|
&[("yes", "end"), ("no", "missing")],
|
||||||
|
"end",
|
||||||
|
);
|
||||||
let graph = graph_with(vec![("ap", approval), ("end", end_node("end"))], "ap");
|
let graph = graph_with(vec![("ap", approval), ("end", end_node("end"))], "ap");
|
||||||
let result = validator().validate(&graph);
|
let result = validator().validate(&graph);
|
||||||
assert!(!result.is_valid());
|
assert!(!result.is_valid());
|
||||||
@@ -461,6 +468,21 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn flags_missing_approval_on_other_target() {
|
||||||
|
let approval = approval_node("ap", &["yes"], &[("yes", "end")], "missing");
|
||||||
|
let graph = graph_with(vec![("ap", approval), ("end", end_node("end"))], "ap");
|
||||||
|
let result = validator().validate(&graph);
|
||||||
|
assert!(!result.is_valid());
|
||||||
|
assert!(
|
||||||
|
result
|
||||||
|
.errors
|
||||||
|
.iter()
|
||||||
|
.any(|e| e.message.contains("non-existent node 'missing'")
|
||||||
|
&& e.message.contains("on_other"))
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn flags_missing_script_fallback_target() {
|
fn flags_missing_script_fallback_target() {
|
||||||
let scr = script_node("s", "does-not-exist.py", Some("nowhere"));
|
let scr = script_node("s", "does-not-exist.py", Some("nowhere"));
|
||||||
@@ -597,7 +619,7 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn errors_when_approval_option_has_no_route() {
|
fn errors_when_approval_option_has_no_route() {
|
||||||
let approval = approval_node("ap", &["yes", "no"], &[("yes", "end")]);
|
let approval = approval_node("ap", &["yes", "no"], &[("yes", "end")], "end");
|
||||||
let graph = graph_with(vec![("ap", approval), ("end", end_node("end"))], "ap");
|
let graph = graph_with(vec![("ap", approval), ("end", end_node("end"))], "ap");
|
||||||
let result = validator().validate(&graph);
|
let result = validator().validate(&graph);
|
||||||
assert!(
|
assert!(
|
||||||
@@ -610,7 +632,7 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn warns_when_approval_has_extra_route() {
|
fn warns_when_approval_has_extra_route() {
|
||||||
let approval = approval_node("ap", &["yes"], &[("yes", "end"), ("maybe", "end")]);
|
let approval = approval_node("ap", &["yes"], &[("yes", "end"), ("maybe", "end")], "end");
|
||||||
let graph = graph_with(vec![("ap", approval), ("end", end_node("end"))], "ap");
|
let graph = graph_with(vec![("ap", approval), ("end", end_node("end"))], "ap");
|
||||||
let result = validator().validate(&graph);
|
let result = validator().validate(&graph);
|
||||||
assert!(result.warnings.iter().any(|w| {
|
assert!(result.warnings.iter().any(|w| {
|
||||||
|
|||||||
Reference in New Issue
Block a user