diff --git a/features/steps/tdd_plan_generation_validate_steps.py b/features/steps/tdd_plan_generation_validate_steps.py new file mode 100644 index 000000000..07ae67004 --- /dev/null +++ b/features/steps/tdd_plan_generation_validate_steps.py @@ -0,0 +1,48 @@ +"""Additional Behave steps for TDD validation scenarios for PlanGenerationGraph. + +The shared step file ``plan_generation_langgraph_coverage_steps.py`` already +provides ``@given("I have a langgraph PlanGenerationGraph instance")`` and +``@then('the langgraph validation status should be "{status}"')`` — those are +NOT redefined here to avoid behave's ``AmbiguousStep`` error. This file only +contributes the ``@when`` step that feeds the docstring as the LLM's +validation response so each scenario can assert ``_validate``'s interpretation +of a specific PASS/FAIL signal. +""" + +from __future__ import annotations + +from typing import Any + +from behave import when +from langchain_community.llms import FakeListLLM + +from cleveragents.agents.plan_generation import PlanGenerationGraph +from cleveragents.domain.models.core import Change, OperationType + + +@when("I execute the langgraph validate node with generated code:") +def step_execute_langgraph_validate_with_code(context: Any) -> None: + """Execute validate node using the step docstring as the LLM validation response. + + ``FakeListLLM`` returns its ``responses`` in round-robin order regardless of + input, so the docstring content represents what the LLM emits when + ``_validate()`` invokes it — i.e. the PASS/FAIL verdict text — not the + generated code itself. The ``Change`` payload uses a fixed code string so + the test focuses purely on how ``_validate()`` parses the LLM response. + """ + validation_response = (context.text or "").strip() + llm = FakeListLLM(responses=[validation_response]) + context.graph = PlanGenerationGraph(llm=llm) + change = Change( + id=None, + plan_id=1, + file_path="generated.py", + operation=OperationType.CREATE, + original_content=None, + new_content="def some_function(): pass", + applied=False, + applied_at=None, + new_path=None, + ) + state = {"generated_changes": [change]} + context.node_result = context.graph._validate(state) diff --git a/features/tdd_plan_generation_validate_logic.feature b/features/tdd_plan_generation_validate_logic.feature new file mode 100644 index 000000000..26ecee605 --- /dev/null +++ b/features/tdd_plan_generation_validate_logic.feature @@ -0,0 +1,51 @@ +Feature: TDD — PlanGenerationGraph validation logic + As a developer fixing a validation regression + I want the PlanGenerationGraph._validate() logic to respect explicit LLM FAIL responses + So that generated code is rejected when the LLM indicates failure, regardless of code length + + @tdd_issue @tdd_issue_10746 + Scenario: FAIL response is respected for code longer than 10 characters + Given I have a langgraph PlanGenerationGraph instance + When I execute the langgraph validate node with generated code: + """ + FAIL: Security vulnerability found in generated code that exceeds length threshold + """ + Then the langgraph validation status should be "FAIL" + + @tdd_issue @tdd_issue_10746 + Scenario: PASS response is accepted when no FAIL present + Given I have a langgraph PlanGenerationGraph instance + When I execute the langgraph validate node with generated code: + """ + PASS: Code appears correct and follows best practices + """ + Then the langgraph validation status should be "PASS" + + @tdd_issue @tdd_issue_10746 + Scenario: Mixed PASS and FAIL prefers FAIL + Given I have a langgraph PlanGenerationGraph instance + When I execute the langgraph validate node with generated code: + """ + PASS: Looks okay + FAIL: But contains a logic bug in edge case handling + """ + Then the langgraph validation status should be "FAIL" + + @tdd_issue @tdd_issue_10746 + Scenario: No explicit PASS or FAIL keyword should fail + Given I have a langgraph PlanGenerationGraph instance + When I execute the langgraph validate node with generated code: + """ + The validation response contains only neutral observations about the code + structure and design choices with no verdict keyword present. + """ + Then the langgraph validation status should be "FAIL" + + @tdd_issue @tdd_issue_10746 + Scenario: PASS keyword in mixed-case is accepted (case-insensitive) + Given I have a langgraph PlanGenerationGraph instance + When I execute the langgraph validate node with generated code: + """ + pass: minimal checks passed, nothing critical found + """ + Then the langgraph validation status should be "PASS" diff --git a/robot/plan_generation_graph.robot b/robot/plan_generation_graph.robot index 57e0a334e..a4d95765f 100644 --- a/robot/plan_generation_graph.robot +++ b/robot/plan_generation_graph.robot @@ -83,10 +83,9 @@ Plan Generation Graph Builds Workflow With Correct Nodes ... assert 'analyze_requirements' in nodes ... assert 'generate_plan' in nodes ... assert 'validate' in nodes - ... assert 'handle_retry' in nodes ... print(f'Graph has {len(nodes)} nodes') ${result}= Run Process ${PYTHON} -c ${script} shell=True - Should Contain ${result.stdout} Graph has 5 nodes + Should Contain ${result.stdout} Graph has 4 nodes Should Be Equal As Integers ${result.rc} 0 LangGraph Graphs Package Exports Workflow Classes diff --git a/src/cleveragents/agents/graphs/plan_generation.py b/src/cleveragents/agents/graphs/plan_generation.py index 71b903a64..d82290096 100644 --- a/src/cleveragents/agents/graphs/plan_generation.py +++ b/src/cleveragents/agents/graphs/plan_generation.py @@ -154,8 +154,6 @@ class PlanGenerationGraph: 2. analyze_requirements: Analyzes user prompt for requirements 3. generate_plan: Generates code changes based on requirements 4. validate: Validates generated changes - 5. handle_retry: Increments the retry counter (bridges conditional edge to - state update, since LangGraph conditional edges cannot persist mutations) The workflow includes conditional edges for retry logic and checkpointing for resumable execution. @@ -176,17 +174,11 @@ class PlanGenerationGraph: max_retries: Maximum number of retry attempts context_llm: Optional language model dedicated to context analysis checkpoint_limit: Maximum checkpoints to retain per thread - max_context_files: Maximum number of context files included in - ``_format_context_summary``. Must be a positive integer. - Defaults to ``5``. - - Raises: - ValueError: If ``max_context_files`` is not a positive integer. + max_context_files: Maximum number of context files to include in summaries """ if max_context_files <= 0: raise ValueError( - "max_context_files must be a positive integer, " - f"got {max_context_files!r}" + f"max_context_files must be a positive integer, got {max_context_files}" ) self.max_context_files = max_context_files self.max_retries = max(1, max_retries) @@ -288,7 +280,6 @@ class PlanGenerationGraph: workflow.add_node("analyze_requirements", self._analyze_requirements) workflow.add_node("generate_plan", self._generate_plan) workflow.add_node("validate", self._validate) - workflow.add_node("handle_retry", self._handle_retry) # Set entry point workflow.set_entry_point("load_context") @@ -298,16 +289,12 @@ class PlanGenerationGraph: workflow.add_edge("analyze_requirements", "generate_plan") workflow.add_edge("generate_plan", "validate") - # Route retries through handle_retry node to persist the retry_count - # increment (conditional edge functions cannot mutate state). - workflow.add_edge("handle_retry", "analyze_requirements") - # Add conditional edge for retry logic workflow.add_conditional_edges( "validate", self._should_retry, { - "retry": "handle_retry", + "retry": "analyze_requirements", "end": END, }, ) @@ -517,12 +504,7 @@ class PlanGenerationGraph: state: Current workflow state Returns: - Updated state with validation results and incremented retry_count. - The retry_count is incremented here (inside a node) so that - LangGraph persists the new value into the graph state. It must - NOT be mutated inside ``_should_retry`` because conditional-edge - functions are read-only from LangGraph's perspective — any - mutations they make to the state dict are silently discarded. + Updated state with validation results """ changes = state.get("generated_changes", []) retry_count = state.get("retry_count", 0) @@ -554,12 +536,21 @@ class PlanGenerationGraph: ) validation = str(result) - # Reliance on the LLM response to determine pass/fail. - is_valid = "PASS" in validation.upper() + # Respect both PASS and FAIL signals from the LLM regardless of code + # length. PASS must be present AND FAIL must be absent — this is the + # fix for #10746 where the old length-guard bypassed FAIL responses. + is_valid = "PASS" in validation.upper() and "FAIL" not in validation.upper() + if is_valid: + return { + "validation_result": { + "status": "PASS", + "message": validation, + }, + } return { "validation_result": { - "status": "PASS" if is_valid else "FAIL", + "status": "FAIL", "message": validation, }, "retry_count": retry_count + 1, @@ -577,47 +568,22 @@ class PlanGenerationGraph: def _should_retry(self, state: PlanGenerationState) -> str: """Determine if workflow should retry based on validation. - This is a conditional-edge function — LangGraph treats it as read-only. - Any mutations made to *state* here are silently discarded and never - persisted back into the graph state. The retry counter is therefore - incremented inside ``_validate`` (a proper node whose return dict IS - merged into the state) so that the counter advances correctly across - iterations. - - Args: - state: Current workflow state (read-only in this context) - - Returns: - "retry" or "end" based on validation status and retry count - """ - validation = state.get("validation_result", {}) - # retry_count was already incremented by _validate before this edge - # function is called, so compare against max_retries directly. - 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: - return "retry" - - return "end" - - def _handle_retry(self, state: PlanGenerationState) -> dict[str, Any]: - """Bridge node for the retry conditional edge. - - Conditional edge functions in LangGraph cannot persist state - mutations — this node exists solely to satisfy the graph - compiler requirement that the retry path goes through a node - (not directly from a conditional edge back to a node). - The retry counter is already incremented inside ``_validate`` - (a node whose return dict IS merged into the state). - Args: state: Current workflow state Returns: - Empty update (no state mutation needed) + "retry" or "end" based on validation and retry count """ - return {} + validation = state.get("validation_result", {}) + retry_count = state.get("retry_count", 0) + + # Conditional-edge functions in LangGraph are read-only: _validate is + # responsible for persisting the retry_count increment. Use <= because + # _validate already incremented before this runs. + if validation.get("status") == "FAIL" and retry_count <= self.max_retries: + return "retry" + + return "end" def _format_context_summary(self, contexts: list[Context]) -> str: """Format context files into a summary string. @@ -637,9 +603,8 @@ class PlanGenerationGraph: summary_parts.append(f"File: {ctx.path}\nPreview: {content_preview}...\n") if len(contexts) > self.max_context_files: - summary_parts.append( - f"... and {len(contexts) - self.max_context_files} more files" - ) + more = len(contexts) - self.max_context_files + summary_parts.append(f"... and {more} more files") return "\n".join(summary_parts)