Conversation
Hexagon nodes with an `auto_policy` attribute (e.g. `auto_policy="findings_empty"`) now route to the new `auto.gate` handler instead of `wait.human`. This enables automated review loops that cycle based on findings count and respect `max_iterations` caps, without requiring human interaction at every gate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the AutoGateHandler, a new node handler for automatic gate nodes in the graph. It supports iteration limits and policy-based routing (e.g., findings_empty). The feedback identifies several critical issues: the parsing of max_iterations is fragile and doesn't handle string values from DOT files; the logic for falling back when specific edges (like 'pass' or 'findings') are missing results in empty suggested_next_ids, which would prematurely terminate the graph execution; and the edge label matching is overly permissive, potentially leading to incorrect routing if labels contain substrings like 'bypass'.
| let max_iterations = node | ||
| .attrs | ||
| .get("max_iterations") | ||
| .and_then(|v| v.as_i64()) | ||
| .unwrap_or(5) as usize; |
There was a problem hiding this comment.
The current parsing of max_iterations only handles integer attributes. If the attribute is provided as a string in the DOT file (e.g., max_iterations="10"), as_i64() will return None, causing it to default to 5. Additionally, using as usize on a potentially negative i64 can lead to unexpected behavior. It's safer to use to_string_value().parse() which handles both numeric and string representations robustly.
| let max_iterations = node | |
| .attrs | |
| .get("max_iterations") | |
| .and_then(|v| v.as_i64()) | |
| .unwrap_or(5) as usize; | |
| let max_iterations = node | |
| .attrs | |
| .get("max_iterations") | |
| .map(|v| v.to_string_value().parse::<usize>().unwrap_or(5)) | |
| .unwrap_or(5); |
| return Ok(NodeOutcome { | ||
| status: NodeStatus::Success, | ||
| context_updates: updates, | ||
| notes: Some(format!("{reason} (no pass edge found)")), | ||
| ..Default::default() | ||
| }); |
There was a problem hiding this comment.
The comment indicates a fallback to the first edge if no specific 'pass' edge is found, but the implementation returns a NodeOutcome with an empty suggested_next_ids list. This will cause the graph execution to stop unexpectedly at this node. The fallback should explicitly select the first available edge to maintain graph flow.
let edge = edges.first().ok_or_else(|| AttractorError::Runtime("auto gate: no outgoing edges".to_string()))?;
return Ok(NodeOutcome {
status: NodeStatus::Success,
preferred_label: Some(edge.attrs.get_str("label").unwrap_or("").to_string()),
suggested_next_ids: vec![edge.to.clone()],
context_updates: updates,
notes: Some(format!("{reason} (no pass edge found, falling back to first edge)")),
..Default::default()
});| Ok(NodeOutcome { | ||
| status: NodeStatus::Success, | ||
| context_updates: updates, | ||
| notes: Some("auto gate: no findings edge, defaulting to pass".to_string()), | ||
| ..Default::default() | ||
| }) |
There was a problem hiding this comment.
When no findings edge is found, the handler should still provide a next node ID to continue execution. Currently, it returns an empty suggested_next_ids list, which effectively terminates the pipeline. It should fall back to the pass_edge or the first available edge.
let edge = pass_edge.or_else(|| edges.first()).ok_or_else(|| AttractorError::Runtime("auto gate: no outgoing edges".to_string()))?;
Ok(NodeOutcome {
status: NodeStatus::Success,
preferred_label: Some(edge.attrs.get_str("label").unwrap_or("").to_string()),
suggested_next_ids: vec![edge.to.clone()],
context_updates: updates,
notes: Some("auto gate: no findings edge, defaulting to pass".to_string()),
..Default::default()
})| fn label_is_pass(label: &str) -> bool { | ||
| let lower = label.to_ascii_lowercase(); | ||
| lower.contains("[p]") || lower.contains("pass") | ||
| } | ||
|
|
||
| fn label_is_findings(label: &str) -> bool { | ||
| let lower = label.to_ascii_lowercase(); | ||
| lower.contains("[f]") || lower.contains("findings") || lower.contains("finding") | ||
| } |
There was a problem hiding this comment.
Using contains for label matching is too permissive and can lead to incorrect routing. For example, label_is_pass would match a label like "bypass", and label_is_findings would match "no findings". It is better to check for the specific [P] / [F] prefix or perform exact word matching to ensure the correct edge is selected.
| fn label_is_pass(label: &str) -> bool { | |
| let lower = label.to_ascii_lowercase(); | |
| lower.contains("[p]") || lower.contains("pass") | |
| } | |
| fn label_is_findings(label: &str) -> bool { | |
| let lower = label.to_ascii_lowercase(); | |
| lower.contains("[f]") || lower.contains("findings") || lower.contains("finding") | |
| } | |
| fn label_is_pass(label: &str) -> bool { | |
| let lower = label.to_ascii_lowercase(); | |
| lower.contains("[p]") || lower == "pass" || lower.starts_with("pass ") | |
| } | |
| fn label_is_findings(label: &str) -> bool { | |
| let lower = label.to_ascii_lowercase(); | |
| lower.contains("[f]") || lower.starts_with("finding") | |
| } |
|
@claude address pr review comments |
Summary
AutoGateHandlerfor hexagon nodes withauto_policyattribute, enabling automated review loops without human interactionauto_policytoauto.gatehandler instead ofwait.humanfindings_emptypolicy (pass whenfindings_countis 0) andmax_iterationscap (force pass when exceeded)Test plan
🤖 Generated with Claude Code