Fix/self healing#127
Conversation
- Fireworks: paginate /inference/v1/models (limit=200, cursor-based), 30s timeout, structured error logging, accept all accounts/fireworks/models/ prefixed models, minimal 2-model fallback - Anthropic: paginate /v1/models (limit=1000, after_id cursor), 30s timeout, structured error logging, minimal 2-model fallback - OpenAI: request /v1/models with limit=500, structured error handling, minimal 2-model fallback (previously returned bare error) - All providers: graceful degradation chain (API → CLI → hardcoded fallback with tracing::warn), no more silent failures - Update default Fireworks model from llama-v3p1-8b-instruct to deepseek-v3p1 across fireworks.rs, fallback.rs, mod.rs, process.rs - Add 3 new unit tests for fallback lists (Anthropic, OpenAI, Fireworks)
Implements a structural separation between LLM reasoning and decisions using the tool calling API, eliminating the crash class where unparseable JSON responses terminated the entire orchestration flow. Primary path (Decision Tool): - Inject a `submit_decision` tool alongside MCP tools so the LLM calls it structurally when ready to decide. Reasoning stays in text, the decision is a tool call with parsed JSON args. Works across all providers (Anthropic, OpenAI, Gemini, Fireworks). - System prompt explicitly instructs: "Reason in text, decide via submit_decision tool call." Fallback path (enhanced text extraction): - 7 parsing strategies: clean JSON, markdown blocks (```json and ```), last JSON object, truncated JSON repair, line-by-line, loose key extraction, pattern matching. - Retry with clarified prompt up to DECISION_RETRY_LIMIT=2 times. - Safe default: returns `no_work` AgentDecision with [SELF-HEAL] prefix instead of crashing the flow. Flow-level error recovery: - Flow::run() catches node errors and retries up to max_retries=3. - Routes to `node_error` or `awaiting_human` handlers before giving up. - Emits `node_error_recovery` events to SharedStore for observability. NexusNode self-healing: - awaiting_human escalation: LLM can return action "awaiting_human" when stuck. Marks tickets as AwaitingHuman, suspends workers, stores reason in SharedStore for TUI visibility. - completed_without_pr recovery: tickets marked completed with outcome "pr_opened" but no matching PR are reset to Open for re-assignment. - Auto-resolve unrecognized STATUS.json statuses. Configurable flow wiring: - nexus → node_error → nexus (retry on LLM/api failures) - forge_pair → node_error → nexus (retry on build failures) - vessel → node_error → nexus (retry on merge failures) - All nodes → awaiting_human → nexus (pause for human input)
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| match flow_node.node.run(store).await { | ||
| Ok(action) => { | ||
| // Successful execution — clear retry tracker | ||
| if let Some(ref tracker) = &retry_tracker { | ||
| if tracker.node_name == current { | ||
| info!( | ||
| node = %current, | ||
| retries = tracker.count, | ||
| "Node recovered after retries — clearing retry tracker" | ||
| ); | ||
| } | ||
| retry_tracker = None; | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: Retry counter reset on unrelated node success defeats max_retries
In Flow::run, the retry tracker is cleared on any successful node execution, not just when the node that was retrying succeeds.
At flow.rs:131-140, retry_tracker = None; is placed inside the outer if let Some(ref tracker) block but outside the inner if tracker.node_name == current check. So whenever a recovery/intermediate node succeeds, the failure count for the actually-failing node is wiped.
Consider the wired routes in agentflow.rs: forge_pair and vessel route NODE_ERROR -> nexus. When forge_pair fails, the tracker becomes {forge_pair, 1} and control routes to nexus. nexus runs successfully and resets the tracker to None. nexus then routes back to forge_pair, which fails again — but retry_count restarts at 1. As a result retry_count can never exceed max_retries for any node whose error route points to a different node that succeeds. The intended max_retries safety cap (default 3) is bypassed, and the flow can livelock — making expensive agent/LLM/subprocess calls — until the far larger max_steps cap (10_000) is finally hit and bails. The added test test_flow_retry_on_node_error uses a self-routing node (empty routes -> self-loop), so it does not exercise the cross-node recovery path and misses this bug.
Fix: only clear the tracker when the recovering node itself succeeds (move the reset inside the node_name == current branch).
Only reset the retry tracker when the node that was being retried is the one that succeeded, so consecutive cross-node failures still accumulate toward max_retries.:
if let Some(ref tracker) = &retry_tracker {
if tracker.node_name == current {
info!(
node = %current,
retries = tracker.count,
"Node recovered after retries — clearing retry tracker"
);
retry_tracker = None;
}
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| // | ||
| // Error Recovery: When a node's exec() phase fails, the system no longer | ||
| // crashes. Instead, it emits an error event and returns a fallback action | ||
| // that routes back to the calling node for retry or safe degradation. | ||
| // This enables self-healing behavior — the flow continues even when an | ||
| // individual agent encounteres a transient failure. |
There was a problem hiding this comment.
💡 Quality: node.rs docs claim error swallowing that code doesn't do; NODE_ERROR action path is dead
Two related inconsistencies introduced by this PR:
-
The new doc comments on
Node::run(node.rs:6-10, 26-29, 46-49) state that on error in any phase,run()"emits an error event and returns a fallback Action rather than crashing/propagating the error." The actual implementation (node.rs:56-77) still propagates every phase error with?. The recovery is performed by the caller (Flow::run), not byrun()itself. The comments are misleading for maintainers and contradict the code. -
NODE_ERROR("node_error") is defined and theFlow::runOk-branch handles a node returning this action (flow.rs:149-174), but noNode::postimplementation in the codebase ever returnsNODE_ERROR. That branch is currently dead code. Consider either having nodes actually returnNODE_ERRORfor recoverable internal failures, or removing/annotating the unused branch to avoid confusion.
Neither is a runtime bug, but both will mislead future readers about how self-healing actually works.
Was this helpful? React with 👍 / 👎
CI failed: The build failed due to formatting inconsistencies, spelling errors in documentation, and Clippy lint violations in the agent-client and agent-nexus modules.OverviewThree distinct CI jobs failed (Format, Spelling, and Clippy) due to style discrepancies, documentation typos, and non-idiomatic Rust code introduced in the recent 'self-healing' changes. All failures are direct results of the current PR and require cleanup. FailuresFormatting Inconsistencies (confidence: high)
Spelling Errors (confidence: high)
Clippy Violations (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact |
|
|
Important
Your trial ends in 2 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.
Was this helpful? React with 👍 / 👎 | Gitar
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // max_turns exceeded — recover gracefully instead of crashing | ||
| warn!( | ||
| agent = persona.id, |
There was a problem hiding this comment.
[logic] (confidence: 85%)
The recovery loop destructures LlmResponse::ToolCall with { name, args, .. }, discarding the LLM-provided id field. It then constructs assistant_tool_use and tool_result messages using a synthetic ID ("recovery-tool-{attempt}"). The main loop correctly uses the LLM's id (LlmResponse::ToolCall { id, name, args }). Using a synthetic ID that doesn't match the LLM's original tool call ID can cause API errors with providers like Anthropic that validate tool_use/tool_result ID consistency in conversation history.
Impact: When the LLM calls a non-decision tool during recovery, the conversation history will contain mismatched tool call IDs. LLM APIs that validate tool_use/tool_result ID pairing may reject the subsequent request, causing the recovery path to fail and fall back to a safe default decision unnecessarily.
Evidence:
Ok(LlmResponse::ToolCall { name, args, .. }) => {
// ...
let tool_call_id = format!("recovery-tool-{}", attempt);
messages.push(Message::assistant_tool_use(
tool_call_id.clone(),
name,
args,
));
messages.push(Message::tool_result(tool_call_id, tool_result));
Suggestion: Capture the id field from LlmResponse::ToolCall and use it instead of generating a synthetic one, matching the pattern used in the main loop: LlmResponse::ToolCall { id, name, args } then Message::assistant_tool_use(id.clone(), name, args) and Message::tool_result(id, tool_result).
| let issue_url = extract_json_string_value(text, "issue_url"); | ||
|
|
||
| warn!( | ||
| action = %action, |
There was a problem hiding this comment.
[logic] (confidence: 75%)
Both extract_json_string_value and extract_quoted_value use find (first occurrence) to locate key patterns, while the main extraction logic in extract_decision (step 3) uses rfind (last occurrence). The step 3 comment explicitly states: 'we prefer the last one because the LLM may produce multiple JSON-like fragments during reasoning, and the final one is most likely the actual decision.' The fallback functions violate this principle by preferring the first occurrence.
Impact: When the LLM produces multiple JSON-like fragments (e.g., reasoning followed by a final decision), the fallback extraction may extract stale values from an earlier fragment instead of the final decision, causing the agent to take the wrong action.
Evidence:
fn extract_json_string_value(text: &str, key: &str) -> Option<String> {
// ...
for pattern in [&pattern_standard as &str, pattern_no_quotes_key] {
if let Some(pos) = text.find(pattern) {
// ...
fn extract_quoted_value(text: &str, key: &str) -> Option<String> {
// ...
for pattern in &patterns {
if let Some(pos) = text.find(pattern.as_str()) {
Suggestion: Change text.find(pattern) to text.rfind(pattern) in both extract_json_string_value and extract_quoted_value to be consistent with the main extraction logic's preference for the last occurrence.
Snif ReviewThis change implements self-healing and crash recovery for the agent orchestration system. It adds a Found 2 issues. See inline comments for details. Review detailsChanged files:
Context analyzed: 100 related files (34 via semantic similarity, 99 via keyword matching). Stats: 2133 lines | |
1 participant
Summary by Gitar
submit_decisiontool for structural LLM responses and added automated retry/recovery logic to theAgentRunner.NODE_ERRORrouting inpocketflow-coreto allow the flow to retry failed nodes or escalate to human intervention.NexusNodeto recover stuck tickets by resetting status.discover_modelsfor Anthropic, OpenAI, and Fireworks providers to use paginated API requests and fallback mechanisms.deepseek-v3p1).v1.1.7.This will update automatically on new commits.