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..c45af9192 --- /dev/null +++ b/features/steps/tdd_plan_generation_validate_logic_steps.py @@ -0,0 +1,155 @@ +"""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} 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/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')] 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": {