agents/graphs/plan_generation: `_should_retry` mutates `state["retry_count"]` in a LangGraph conditional edge function, bypassing state management #10400

New issue

Open

opened 2026-04-18 09:29:26 +00:00 by HAL9000 · 0 comments

HAL9000 commented

2026-04-18 09:29:26 +00:00

Owner

Metadata

Commit: fix(agents/graphs/plan_generation): move retry_count increment out of conditional edge function
Branch: fix/plan-generation-should-retry-state-mutation

Background and Context

PlanGenerationGraph._should_retry() is registered as a LangGraph conditional edge function but incorrectly mutates state["retry_count"] directly. LangGraph conditional edge functions are pure routing functions — state mutations inside them are not tracked by LangGraph's state management system, leading to incorrect retry count tracking and potential infinite retry loops.

Expected Behavior

_should_retry() should be a pure routing function that only inspects state and returns a routing key ("retry" or "end"). The retry_count increment should be performed in a dedicated node function that returns a state update dict, which LangGraph processes correctly through its state management system.

Acceptance Criteria

_should_retry() does not mutate state["retry_count"] or any other state key
A dedicated node (e.g., _increment_retry) handles the retry_count increment and returns {"retry_count": ...}
The graph wiring routes through the increment node before re-entering analyze_requirements
Retry limit enforcement still works correctly (no infinite loops)
Checkpoint/resume scenarios correctly reflect the retry count
TDD test test_should_retry_does_not_mutate_state (from #10397) passes
All existing tests pass with coverage >= 97%

Subtasks

Remove state["retry_count"] = retry_count + 1 from _should_retry()
Make _should_retry() a pure routing function
Add _increment_retry() node method returning {"retry_count": state.get("retry_count", 0) + 1}
Add "increment_retry" node to the workflow graph
Update conditional edges: "retry" → "increment_retry" → "analyze_requirements"
Verify retry limit still enforced correctly
Run full test suite and confirm coverage >= 97%

Definition of Done

This issue is closed when _should_retry() is a pure routing function, the retry_count increment is handled by a proper node, the graph wiring is updated accordingly, the TDD test from #10397 passes, and all existing tests continue to pass with coverage >= 97%.

Bug Report

Summary

Affected File

src/cleveragents/agents/graphs/plan_generation.py

Code Evidence

def _should_retry(self, state: PlanGenerationState) -> str:
    """Determine if workflow should retry based on validation."""
    validation = state.get("validation_result", {})
    retry_count = state.get("retry_count", 0)

    # Check if validation failed and retries available
    if validation.get("status") == "FAIL" and retry_count < self.max_retries:
        # Increment retry count
        state["retry_count"] = retry_count + 1  # BUG: direct state mutation in conditional edge!
        return "retry"

    return "end"

This function is registered as a conditional edge:

workflow.add_conditional_edges(
    "validate",
    self._should_retry,  # registered as conditional edge
    {"retry": "analyze_requirements", "end": END},
)

Impact

State management bypass: LangGraph tracks state changes through node return values. Mutations in conditional edge functions are not part of LangGraph's state update mechanism. The retry_count increment may not be properly persisted in checkpoints.
Incorrect retry counting: When LangGraph resumes from a checkpoint (e.g., after a failure), the retry_count in the checkpoint may not reflect the mutation made in _should_retry, causing the retry limit to be exceeded or ignored.
Potential infinite retries: If the checkpoint restores retry_count to a pre-mutation value, the retry limit check retry_count < self.max_retries may never terminate.
LangGraph contract violation: The LangGraph documentation explicitly states that conditional edge functions must be pure routing functions that only return a routing key.

Fix

Move the retry_count increment to a proper node function:

def _should_retry(self, state: PlanGenerationState) -> str:
    """Pure routing function - only returns routing key, no state mutation."""
    validation = state.get("validation_result", {})
    retry_count = state.get("retry_count", 0)
    if validation.get("status") == "FAIL" and retry_count < self.max_retries:
        return "retry"
    return "end"

def _increment_retry(self, state: PlanGenerationState) -> dict[str, Any]:
    """Node: increment retry counter before re-running analysis."""
    return {"retry_count": state.get("retry_count", 0) + 1}

And add the increment node to the graph:

workflow.add_node("increment_retry", self._increment_retry)
workflow.add_conditional_edges(
    "validate",
    self._should_retry,
    {"retry": "increment_retry", "end": END},
)
workflow.add_edge("increment_retry", "analyze_requirements")

Validation Gate

Code evidence: state["retry_count"] = retry_count + 1 inside _should_retry() in plan_generation.py
Environment verification: Reproducible with any failing validation that triggers retry
Actionability: Move increment to a dedicated node function
Codebase freshness: Verified in current HEAD
Severity match: Critical - incorrect retry counting, potential infinite loops, checkpoint inconsistency

Blocked By

Depends on TDD issue #10397.

Automated by CleverAgents Bot
Supervisor: Bug Hunt Pool | Agent: bug-hunt-pool-supervisor

## Metadata - **Commit:** `fix(agents/graphs/plan_generation): move retry_count increment out of conditional edge function` - **Branch:** `fix/plan-generation-should-retry-state-mutation` ## Background and Context `PlanGenerationGraph._should_retry()` is registered as a LangGraph conditional edge function but incorrectly mutates `state["retry_count"]` directly. LangGraph conditional edge functions are pure routing functions — state mutations inside them are not tracked by LangGraph's state management system, leading to incorrect retry count tracking and potential infinite retry loops. ## Expected Behavior `_should_retry()` should be a pure routing function that only inspects state and returns a routing key (`"retry"` or `"end"`). The `retry_count` increment should be performed in a dedicated node function that returns a state update dict, which LangGraph processes correctly through its state management system. ## Acceptance Criteria - [ ] `_should_retry()` does not mutate `state["retry_count"]` or any other state key - [ ] A dedicated node (e.g., `_increment_retry`) handles the `retry_count` increment and returns `{"retry_count": ...}` - [ ] The graph wiring routes through the increment node before re-entering `analyze_requirements` - [ ] Retry limit enforcement still works correctly (no infinite loops) - [ ] Checkpoint/resume scenarios correctly reflect the retry count - [ ] TDD test `test_should_retry_does_not_mutate_state` (from #10397) passes - [ ] All existing tests pass with coverage >= 97% ## Subtasks - [ ] Remove `state["retry_count"] = retry_count + 1` from `_should_retry()` - [ ] Make `_should_retry()` a pure routing function - [ ] Add `_increment_retry()` node method returning `{"retry_count": state.get("retry_count", 0) + 1}` - [ ] Add `"increment_retry"` node to the workflow graph - [ ] Update conditional edges: `"retry"` → `"increment_retry"` → `"analyze_requirements"` - [ ] Verify retry limit still enforced correctly - [ ] Run full test suite and confirm coverage >= 97% ## Definition of Done This issue is closed when `_should_retry()` is a pure routing function, the `retry_count` increment is handled by a proper node, the graph wiring is updated accordingly, the TDD test from #10397 passes, and all existing tests continue to pass with coverage >= 97%. --- ## Bug Report ### Summary `PlanGenerationGraph._should_retry()` is registered as a LangGraph conditional edge function but incorrectly mutates `state["retry_count"]` directly. LangGraph conditional edge functions are pure routing functions — state mutations inside them are not tracked by LangGraph's state management system, leading to incorrect retry count tracking and potential infinite retry loops. ### Affected File `src/cleveragents/agents/graphs/plan_generation.py` ### Code Evidence ```python def _should_retry(self, state: PlanGenerationState) -> str: """Determine if workflow should retry based on validation.""" validation = state.get("validation_result", {}) retry_count = state.get("retry_count", 0) # Check if validation failed and retries available if validation.get("status") == "FAIL" and retry_count < self.max_retries: # Increment retry count state["retry_count"] = retry_count + 1 # BUG: direct state mutation in conditional edge! return "retry" return "end" ``` This function is registered as a conditional edge: ```python workflow.add_conditional_edges( "validate", self._should_retry, # registered as conditional edge {"retry": "analyze_requirements", "end": END}, ) ``` ### Impact 1. **State management bypass**: LangGraph tracks state changes through node return values. Mutations in conditional edge functions are not part of LangGraph's state update mechanism. The `retry_count` increment may not be properly persisted in checkpoints. 2. **Incorrect retry counting**: When LangGraph resumes from a checkpoint (e.g., after a failure), the `retry_count` in the checkpoint may not reflect the mutation made in `_should_retry`, causing the retry limit to be exceeded or ignored. 3. **Potential infinite retries**: If the checkpoint restores `retry_count` to a pre-mutation value, the retry limit check `retry_count < self.max_retries` may never terminate. 4. **LangGraph contract violation**: The LangGraph documentation explicitly states that conditional edge functions must be pure routing functions that only return a routing key. ### Fix Move the `retry_count` increment to a proper node function: ```python def _should_retry(self, state: PlanGenerationState) -> str: """Pure routing function - only returns routing key, no state mutation.""" validation = state.get("validation_result", {}) retry_count = state.get("retry_count", 0) if validation.get("status") == "FAIL" and retry_count < self.max_retries: return "retry" return "end" def _increment_retry(self, state: PlanGenerationState) -> dict[str, Any]: """Node: increment retry counter before re-running analysis.""" return {"retry_count": state.get("retry_count", 0) + 1} ``` And add the increment node to the graph: ```python workflow.add_node("increment_retry", self._increment_retry) workflow.add_conditional_edges( "validate", self._should_retry, {"retry": "increment_retry", "end": END}, ) workflow.add_edge("increment_retry", "analyze_requirements") ``` ### Validation Gate - [x] Code evidence: `state["retry_count"] = retry_count + 1` inside `_should_retry()` in `plan_generation.py` - [x] Environment verification: Reproducible with any failing validation that triggers retry - [x] Actionability: Move increment to a dedicated node function - [x] Codebase freshness: Verified in current HEAD - [x] Severity match: Critical - incorrect retry counting, potential infinite loops, checkpoint inconsistency ### Blocked By Depends on TDD issue #10397. --- **Automated by CleverAgents Bot** Supervisor: Bug Hunt Pool | Agent: bug-hunt-pool-supervisor

HAL9000 added the

labels

2026-04-18 09:33:12 +00:00