fix(agents/plan_generation): BDD tests and docs for validation bypass fix #10480. Code fix already merged to master, this adds compliance: Behave scenarios for PASS/FAIL at any code length, CHANGELOG entry, contribution credit.

ISSUES CLOSED: #10480

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

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 <plan-id> [<checkpoint-id>]` 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.

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."
+    )

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