From cab2646543083e81e6ebf709785abf671cfd2aa8 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 19 Apr 2026 07:19:06 +0000 Subject: [PATCH 1/2] fix(agents/graphs/plan_generation): _validate always passes for code longer than 10 characters Fix PlanGenerationGraph._validate logic by removing the erroneous or len(all_code) > 10 condition that caused validation to always pass for any generated code longer than 10 characters, completely bypassing the LLM validation response. The fix changes the is_valid calculation from: is_valid = 'PASS' in validation.upper() or len(all_code) > 10 to: is_valid = 'PASS' in validation.upper() and 'FAIL' not in validation.upper() This ensures the LLM's judgment is respected regardless of code length, restoring the retry logic and preventing invalid/broken code from passing validation. Also adds TDD tests (feature file and steps) for issue #10477 as the TDD counterpart, with @tdd_issue and @tdd_issue_10477 tags. Closes #10480 --- ...dd_plan_generation_validate_logic_steps.py | 156 ++++++++++++++++++ ...tdd_plan_generation_validate_logic.feature | 43 +++++ .../agents/graphs/plan_generation.py | 8 +- 3 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 features/steps/tdd_plan_generation_validate_logic_steps.py create mode 100644 features/tdd_plan_generation_validate_logic.feature diff --git a/features/steps/tdd_plan_generation_validate_logic_steps.py b/features/steps/tdd_plan_generation_validate_logic_steps.py new file mode 100644 index 000000000..2a3ed63ac --- /dev/null +++ b/features/steps/tdd_plan_generation_validate_logic_steps.py @@ -0,0 +1,156 @@ +"""Step definitions for tdd_plan_generation_validate_logic.feature. + +This test captures bug #10480 (TDD counterpart #10477): +PlanGenerationGraph._validate() always passes for code longer than 10 +characters, making LLM validation ineffective. + +The bug was: + is_valid = "PASS" in validation.upper() or len(all_code) > 10 # BUG + +The fix: + is_valid = "PASS" in validation.upper() and "FAIL" not in validation.upper() + +These tests verify that _validate() correctly respects the LLM's response +regardless of the length of the generated code. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +from behave import given, then, when +from langchain_community.llms import FakeListLLM + +from cleveragents.domain.models.core import Change, OperationType, Plan, Project + + +def _make_change_with_long_code() -> Change: + """Create a Change with code longer than 10 characters.""" + return Change( + id=None, + plan_id=1, + file_path="test.py", + operation=OperationType.CREATE, + original_content=None, + new_content="def broken_function():\n syntax error here\n return None", + applied=False, + applied_at=None, + new_path=None, + ) + + +def _make_state_with_changes(changes: list[Change]) -> dict[str, Any]: + """Create a minimal PlanGenerationState dict with the given changes.""" + return { + "project": Project(id=1, name="test", path=Path("/tmp/test")), + "plan": Plan(id=1, project_id=1, name="plan", prompt="test"), + "contexts": [], + "context_summary": "", + "context_dependencies": {}, + "context_relevance": {}, + "context_analysis_error": None, + "actor_name": None, + "actor_options": {}, + "actor_graph_descriptor": None, + "actor_initial_context": {}, + "prompt": "test", + "analyzed_requirements": {}, + "generated_changes": changes, + "validation_result": {}, + "retry_count": 0, + "error": None, + } + + +@given("a PlanGenerationGraph with an LLM that always returns FAIL") +def step_graph_with_fail_llm(context: Any) -> None: + """Create a PlanGenerationGraph whose LLM always returns FAIL.""" + from cleveragents.agents.graphs.plan_generation import PlanGenerationGraph + + context.graph = PlanGenerationGraph( + llm=FakeListLLM(responses=["FAIL: this code is completely broken"]) + ) + + +@given("a PlanGenerationGraph with an LLM that always returns PASS") +def step_graph_with_pass_llm(context: Any) -> None: + """Create a PlanGenerationGraph whose LLM always returns PASS.""" + from cleveragents.agents.graphs.plan_generation import PlanGenerationGraph + + context.graph = PlanGenerationGraph( + llm=FakeListLLM(responses=["PASS: code looks good"]) + ) + + +@given('a PlanGenerationGraph with an LLM that returns "FAIL: syntax error on line 5"') +def step_graph_with_specific_fail_llm(context: Any) -> None: + """Create a PlanGenerationGraph whose LLM returns a specific FAIL message.""" + from cleveragents.agents.graphs.plan_generation import PlanGenerationGraph + + context.llm_response = "FAIL: syntax error on line 5" + context.graph = PlanGenerationGraph( + llm=FakeListLLM(responses=[context.llm_response]) + ) + + +@given("a state with generated changes containing code longer than 10 characters") +def step_state_with_long_code(context: Any) -> None: + """Create a state with a Change whose code is longer than 10 characters.""" + change = _make_change_with_long_code() + # Verify the code is indeed longer than 10 characters (sanity check) + assert len(change.new_content or "") > 10, ( + f"Test setup error: new_content must be > 10 chars, " + f"got {len(change.new_content or '')} chars" + ) + context.state = _make_state_with_changes([change]) + + +@when("I call _validate on the state") +def step_call_validate(context: Any) -> None: + """Call _validate() on the graph with the prepared state.""" + context.result = context.graph._validate(context.state) + + +@then("the validation result status should be FAIL") +def step_validation_status_fail(context: Any) -> None: + """Assert that the validation result status is FAIL. + + This is the core assertion for bug #10480: when the LLM returns FAIL, + the validation must fail regardless of the code length. + + Before the fix, ``or len(all_code) > 10`` caused is_valid to always be + True for any real code, so this assertion would fail (status would be + PASS instead of FAIL). + """ + validation = context.result.get("validation_result", {}) + status = validation.get("status") + assert status == "FAIL", ( + f"Expected validation status FAIL but got {status!r}. " + f"Bug #10480: The `or len(all_code) > 10` condition incorrectly " + f"overrides the LLM's FAIL response when code is longer than 10 chars. " + f"Full validation result: {validation}" + ) + + +@then("the validation result status should be PASS") +def step_validation_status_pass(context: Any) -> None: + """Assert that the validation result status is PASS.""" + validation = context.result.get("validation_result", {}) + status = validation.get("status") + assert status == "PASS", ( + f"Expected validation status PASS but got {status!r}. " + f"Full validation result: {validation}" + ) + + +@then("the validation message should contain the LLM response") +def step_validation_message_contains_llm_response(context: Any) -> None: + """Assert that the validation message contains the LLM's response text.""" + validation = context.result.get("validation_result", {}) + message = validation.get("message", "") + llm_response = getattr(context, "llm_response", "FAIL") + assert llm_response in message, ( + f"Expected validation message to contain {llm_response!r} " + f"but got: {message!r}" + ) diff --git a/features/tdd_plan_generation_validate_logic.feature b/features/tdd_plan_generation_validate_logic.feature new file mode 100644 index 000000000..147a023d2 --- /dev/null +++ b/features/tdd_plan_generation_validate_logic.feature @@ -0,0 +1,43 @@ +@tdd_issue @tdd_issue_10477 +Feature: TDD Issue #10477 — PlanGenerationGraph._validate() always passes for code longer than 10 characters + As a developer relying on LLM-based code validation + I want _validate() to respect the LLM's FAIL response + So that invalid, broken, or insecure generated code is correctly rejected + + This test captures bug #10480 (and its TDD counterpart #10477). + The _validate() method had a logic error: + + is_valid = "PASS" in validation.upper() or len(all_code) > 10 # BUG + + The ``or len(all_code) > 10`` condition made validation always pass for + any generated code longer than 10 characters, completely bypassing the + LLM's validation response. + + The fix removes the ``or len(all_code) > 10`` condition so that + validation correctly reflects the LLM's response: + + is_valid = "PASS" in validation.upper() and "FAIL" not in validation.upper() + + See CONTRIBUTING.md > Bug Fix Workflow > TDD Issue Test Tags. + + @tdd_issue @tdd_issue_10477 + Scenario: _validate() fails when LLM returns FAIL regardless of code length + Given a PlanGenerationGraph with an LLM that always returns FAIL + And a state with generated changes containing code longer than 10 characters + When I call _validate on the state + Then the validation result status should be FAIL + + @tdd_issue @tdd_issue_10477 + Scenario: _validate() passes when LLM returns PASS + Given a PlanGenerationGraph with an LLM that always returns PASS + And a state with generated changes containing code longer than 10 characters + When I call _validate on the state + Then the validation result status should be PASS + + @tdd_issue @tdd_issue_10477 + Scenario: _validate() fails when LLM returns FAIL with detailed message + Given a PlanGenerationGraph with an LLM that returns "FAIL: syntax error on line 5" + And a state with generated changes containing code longer than 10 characters + When I call _validate on the state + Then the validation result status should be FAIL + And the validation message should contain the LLM response diff --git a/src/cleveragents/agents/graphs/plan_generation.py b/src/cleveragents/agents/graphs/plan_generation.py index a2226ed53..241293b11 100644 --- a/src/cleveragents/agents/graphs/plan_generation.py +++ b/src/cleveragents/agents/graphs/plan_generation.py @@ -527,8 +527,12 @@ class PlanGenerationGraph: ) validation = str(result) - # Simple validation check (in real implementation, parse the LLM response) - is_valid = "PASS" in validation.upper() or len(all_code) > 10 + # Parse the LLM response: PASS only when the response contains PASS + # and does not contain FAIL. This ensures the LLM's judgment is + # respected regardless of the generated code length. + # Bug fix for #10480: removed the erroneous `or len(all_code) > 10` + # condition that caused validation to always pass for any real code. + is_valid = "PASS" in validation.upper() and "FAIL" not in validation.upper() return { "validation_result": { -- 2.52.0 From d89a1c7098e5fb5b3f20421eab6b3faf528a0a9f Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 19 Apr 2026 12:26:29 +0000 Subject: [PATCH 2/2] fix(agents/graphs/plan_generation): _validate always passes for code longer than 10 characters Applied a Ruff format fix in features/steps/tdd_plan_generation_validate_logic_steps.py by merging two f-string lines into one. Updated two integration tests in robot/plan_generation_graph.robot to use FakeListLLM(responses=['PASS: code looks good']*10) instead of FakeListLLM(responses=['test']*3). This is necessary because the validation logic fix makes 'test' responses fail validation (they don't contain 'PASS'), causing the workflow to retry indefinitely. The tests now provide passing responses to reflect the new behavior. --- features/steps/tdd_plan_generation_validate_logic_steps.py | 3 +-- robot/plan_generation_graph.robot | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/features/steps/tdd_plan_generation_validate_logic_steps.py b/features/steps/tdd_plan_generation_validate_logic_steps.py index 2a3ed63ac..c45af9192 100644 --- a/features/steps/tdd_plan_generation_validate_logic_steps.py +++ b/features/steps/tdd_plan_generation_validate_logic_steps.py @@ -151,6 +151,5 @@ def step_validation_message_contains_llm_response(context: Any) -> None: message = validation.get("message", "") llm_response = getattr(context, "llm_response", "FAIL") assert llm_response in message, ( - f"Expected validation message to contain {llm_response!r} " - f"but got: {message!r}" + f"Expected validation message to contain {llm_response!r} but got: {message!r}" ) diff --git a/robot/plan_generation_graph.robot b/robot/plan_generation_graph.robot index 10bcb3282..42ec62a7c 100644 --- a/robot/plan_generation_graph.robot +++ b/robot/plan_generation_graph.robot @@ -375,7 +375,7 @@ Workflow Invoke Method Returns Complete State ... from cleveragents.agents.plan_generation import PlanGenerationGraph ... from langchain_community.llms import FakeListLLM ... from cleveragents.domain.models.core import Project, Plan, Context - ... graph = PlanGenerationGraph(llm=FakeListLLM(responses=['test']*3)) + ... graph = PlanGenerationGraph(llm=FakeListLLM(responses=['PASS: code looks good']*10)) ... project = Project(id=1, name='test_project', path=Path('/tmp/test_project')) ... plan = Plan(id=1, project_id=1, name='Logging Plan', prompt='Add logging') ... contexts = [Context(plan_id=plan.id, path='app.py', content='def main(): pass')] @@ -405,7 +405,7 @@ Workflow Stream Method Yields Events ... from cleveragents.agents.plan_generation import PlanGenerationGraph ... from langchain_community.llms import FakeListLLM ... from cleveragents.domain.models.core import Project, Plan, Context - ... graph = PlanGenerationGraph(llm=FakeListLLM(responses=['test']*3)) + ... graph = PlanGenerationGraph(llm=FakeListLLM(responses=['PASS: code looks good']*10)) ... project = Project(id=1, name='test_project', path=Path('/tmp/test_project')) ... plan = Plan(id=1, project_id=1, name='Feature Plan', prompt='Add feature') ... contexts = [Context(plan_id=plan.id, path='app.py', content='# app')] -- 2.52.0