diff --git a/CHANGELOG.md b/CHANGELOG.md index a0734842d..414fe53f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ ## Unreleased +- Fixed a validation bypass in `PlanGenerationGraph._validate()` (issue #10480): + the artificial ``or len(all_code) > 10`` condition that caused any code longer + than 10 characters to auto-pass LLM validation has been removed. LLM verdicts + are now solely responsible for determine pass/fail status, enabling the retry + logic and broken-code rejection to operate correctly. + - Hardened the TDD bug-fix quality gate for issue #629: PR parsing now requires whole-word closing keywords (avoids false positives like "prefixes #12"), TDD bug tag discovery now uses exact token matching diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index dbef154a9..9a1b8d7a8 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -40,3 +40,4 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed database resource types (PostgreSQL, SQLite) with transaction-based sandbox strategy: implemented ``DatabaseResourceHandler`` providing full CRUD operations (`read`, `write`, `delete`, `list_children`) and connection validation with automatic credential masking for PostgreSQL and SQLite backends. Includes ``TransactionSandbox`` infrastructure wired into ``SandboxFactory``, BDD test coverage in ``features/database_resources.feature``, and Robot Framework integration tests in ``robot/database_resources.robot`` (PR #10591 / issue #8608, Epic #8568). * HAL 9000 has contributed the agents plan rollback command (PR #8674 / issue #8557): implemented checkpoint-based plan state restoration with the `agents plan rollback []` CLI command as part of Epic #8493, enabling plans to be restored to previous checkpoints, discarding post-checkpoint decisions, and resuming execution from the rolled-back state. Supported by `--yes/-y`, `--to-checkpoint`, and `--format/-f` flags. Includes comprehensive BDD test coverage (>= 97%) for rollback, decision discarding, and plan resume functionality. * HAL 9000 has contributed the PyYAML security upgrade (PR #11012 / issue #9055): added `pyyaml>=6.0.3` dependency constraint to address known YAML parsing vulnerabilities. +* Jeffrey Phillips Freeman has contributed the plan generation validation bypass fix (issue #10480): removed the ``or len(all_code) > 10`` condition from ``PlanGenerationGraph._validate()`` that caused all code longer than 10 characters to auto-pass LLM validation, making the entire validation step ineffective. Added comprehensive BDD test coverage using Behave feature files and FakeListLLM mocks to verify that LLM pass/fail verdicts are respected regardless of code length. diff --git a/features/steps/validation_bypass_issue_10480_steps.py b/features/steps/validation_bypass_issue_10480_steps.py new file mode 100644 index 000000000..c3565bd60 --- /dev/null +++ b/features/steps/validation_bypass_issue_10480_steps.py @@ -0,0 +1,234 @@ +"""Step definitions for validation_bypass_issue_10480.feature. + +These tests verify that the bug fix for issue #10480 is correct: +- The artificial `or len(all_code) > 10` condition has been removed +- LLM pass/fail verdicts are now respected regardless of code length +""" + +from __future__ import annotations +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from langchain_community.llms import FakeListLLM + +from cleveragents.agents.graphs.plan_generation import ( + PlanGenerationGraph, +) +from cleveragents.domain.models.core import Change + + +def _make_changes(code: str, filepath: str = "test/file.py") -> list[Change]: + """Build a minimal Change list from code string using model_construct + to bypass full Pydantic validation — we need only file_path and new_content.""" + return [ + Change.model_construct( + file_path=filepath, + operation="modify", + original_content="", + new_content=code, + id=1, + plan_id=1, + ) + ] + + +def make_fake_llm(responses: list[str]) -> FakeListLLM: + """Create a FakeListLLM with the specified responses.""" + return FakeListLLM(responses=responses) + + +# --------------------------------------------------------------------------- +# Background steps +# --------------------------------------------------------------------------- +@given("the plan generation coverage module is imported") +def step_coverage_module_imported(context): + """Ensure the plan_generation module is importable.""" + assert PlanGenerationGraph is not None + + +@given("the validation bypass test helper is ready") +def step_test_helper_ready(context): + """Initialize a shared context dict for our tests.""" + context.response = "" + context.validation_result = None + context.code_length = 0 + context.llm_responses = [] + context._test_code = "def func(): pass" + + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- +@given('an LLM mock that responds with "{response}"') +def step_llm_mock_responds(context, response): + """Configure a FakeListLLM with the specified validation response. + + We store the response so When steps can patch _validate to use it. + The actual PlanGenerationGraph is created on demand in each scenario's + When step because we need a fresh instance per LLM configuration. + """ + context.response = response.strip() + context.llm_responses.append(context.response) + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- +@when("a PlanGenerationGraph _validate is called with short generated code") +def step_validate_short_code(context): + """Validate a small snippet (< 10 chars) and check that FAIL is respected.""" + code = "x + y" # 5 chars deliberately short + context.code_length = len(code) + _run_validate_with_patch(context, code) + + +@when("a PlanGenerationGraph _validate is called with longer generated code") +def step_validate_long_code(context): + """Validate a realistic snippet (> 10 chars) and check that FAIL is respected.""" + code = "def broken(): syntax error here" # 32 chars > 10 + context.code_length = len(code) + _run_validate_with_patch(context, code) + + +@when("a PlanGenerationGraph _validate is called with any generated code") +def step_validate_any_code(context): + """Validate whatever code was prepared and check the result.""" + code = getattr(context, "_test_code", "def valid_func(): return True") # 38 chars + context.code_length = len(code) + _run_validate_with_patch(context, code) + + +@when("a PlanGenerationGraph _validate is called with generated code") +def step_validate_generates_code(context): + """Validate the test code (default length > 10).""" + code = getattr(context, "_test_code", "def func(): pass") + context.code_length = len(code) + _run_validate_with_patch(context, code) + + +@when("a PlanGenerationGraph _validate is called with code longer than 10 characters") +def step_validate_code_longer_than_10(context): + """Validate explicitly lengthed code (> 10 chars) — the exact fix point.""" + code = "def broken(): syntax error here" # 32 chars > 10 + context._test_code = code + context.code_length = len(code) + _run_validate_with_patch(context, code) + + +def _run_validate_with_patch(context, code: str) -> None: + """Helper that creates a graph, patches the LLM chain, calls _validate.""" + llm = make_fake_llm(context.llm_responses or [context.response]) + graph = PlanGenerationGraph(llm=llm, max_retries=1) + + # Patch the validation chain so it returns our exact response. + # This bypasses the actual LLM and tests _validate's is_valid logic directly. + patched_result = context.response + + with ( + patch.object(graph, "app", spec=True), + patch.object( + graph, + "_chain_with_retry", + return_value=MagicMock(invoke=lambda _: patched_result), + ), + ): + state = { + "plan_id": "test-plan-001", + "project_id": "test-project-001", + "context_summary": "Test context", + "requirements": "Test requirements", + "generated_changes": _make_changes(code, "test.py"), + } + result = graph._validate(state) + context.validation_result = result + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- +@then('the validation result status should be "{status}"') +def step_validation_status(context, status): + """Assert the validation result matches expected pass/fail status.""" + assert context.validation_result is not None, "Validation was not executed" + result = context.validation_result.get("validation_result", {}) + actual_status = result.get("status", "") + assert actual_status == status, ( + f"Expected status '{status}' but got '{actual_status}' (response was: {context.response})" + ) + + +@then("the validation message should contain the LLM feedback") +def step_validation_message_contains_feedback(context): + """Assert the message field preserves the LLM's validation string.""" + assert context.validation_result is not None + result = context.validation_result.get("validation_result", {}) + message = result.get("message", "") + assert context.response in message or context.response.upper() in message, ( + f"Message '{message}' does not contain expected response '{context.response}'" + ) + + +@then("and no artificial length threshold bypassed the rejection") +def step_no_length_threshold_bypass(context): + """Confirm that code length did not override a FAIL verdict. + + This is the regression guard for issue #10480: if any code longer + than 10 characters auto-passed, this scenario would fail because + status would be "PASS" instead of "FAIL". + """ + assert context.validation_result is not None + result = context.validation_result.get("validation_result", {}) + status = result.get("status", "") + # The key assertion: FAIL must NOT have been overridden by length + assert status == "FAIL", ( + f"Code-length bypass detected! Code was {context.code_length} chars but still got FAIL verdict. Status is '{status}'." + ) + + +@then("and the validation message should preserve the full LLM feedback") +def step_validation_preserves_feedback(context): + """Full message must contain the original LLM response.""" + assert context.validation_result is not None + result = context.validation_result.get("validation_result", {}) + message = result.get("message", "") + assert context.response in message, ( + f"Full feedback not preserved. Expected '{context.response}' in '{message}'." + ) + + +@then("and the code-length condition must NOT override the FAIL verdict") +def step_code_length_not_override(context): + """Definitive regression test: code length > 10 did not cause auto-pass. + + This directly addresses the bug described in issue #10480 where: + is_valid = "PASS" in validation.upper() or len(all_code) > 10 + + Would make ANY code longer than 10 characters always pass, even when + LLM said FAIL. After the fix: + is_valid = "PASS" in validation.upper() + + The verdict depends ONLY on whether "PASS" is in the response. + """ + assert context.validation_result is not None + result = context.validation_result.get("validation_result", {}) + status = result.get("status", "") + message = result.get("message", "") + + # Code must be >10 chars to exercise the fix point + assert context.code_length > 10, ( + f"Code was only {context.code_length} chars — test should use longer code." + ) + + # LLM returned FAIL, but status must NOT have been overridden + assert "FAIL" in context.response.upper(), ( + f"Response doesn't contain FAIL: '{context.response}'" + ) + + assert status == "FAIL", ( + f"Bypass detected: code was {context.code_length} chars, LLM said FAIL but status is '{status}'." + ) + + # Verify the response itself appears in the result (proof we used it) + assert context.response.upper() in message.upper(), ( + "LLM feedback not in result message — validation may be ignoring LLM." + ) diff --git a/features/validation_bypass_issue_10480.feature b/features/validation_bypass_issue_10480.feature new file mode 100644 index 000000000..c953220cf --- /dev/null +++ b/features/validation_bypass_issue_10480.feature @@ -0,0 +1,38 @@ +Feature: Validation bypass fix (issue #10480) + Verify that _validate correctly respects LLM pass/fail verdicts + without an artificial code-length threshold that caused it to + always pass for any generated code longer than 10 characters. + + Background: + Given the plan generation coverage module is imported + And the validation bypass test helper is ready + + Scenario: LLM returns FAIL for short code and validation correctly rejects it + Given an LLM mock that responds with "FAIL: syntax error" + When a PlanGenerationGraph _validate is called with short generated code + Then the validation result status should be "FAIL" + And the validation message should contain the LLM feedback + + Scenario: LLM returns FAIL for long code and validation correctly rejects it + Given an LLM mock that responds with "FAIL: unsafe pattern detected" + When a PlanGenerationGraph _validate is called with longer generated code + Then the validation result status should be "FAIL" + And no artificial length threshold bypassed the rejection + + Scenario: LLM returns PASS and validation correctly accepts it + Given an LLM mock that responds with "PASS" + When a PlanGenerationGraph _validate is called with any generated code + Then the validation result status should be "PASS" + And the validation message should contain the LLM feedback + + Scenario: LLM returns FAIL with additional details and validation correctly rejects + Given an LLM mock that responds with "FAIL: missing error handling in auth module" + When a PlanGenerationGraph _validate is called with generated code + Then the validation result status should be "FAIL" + And the validation message should preserve the full LLM feedback + + Scenario: Validation ignores code length and does not auto-pass long code + Given an LLM mock that responds with "FAIL: security vulnerability" + When a PlanGenerationGraph _validate is called with code longer than 10 characters + Then the validation result status should be "FAIL" + And the code-length condition must NOT override the FAIL verdict