diff --git a/features/steps/plan_generation_uncovered_lines_steps.py b/features/steps/plan_generation_uncovered_lines_steps.py index 2fa6da9f3..a9ec0c35b 100644 --- a/features/steps/plan_generation_uncovered_lines_steps.py +++ b/features/steps/plan_generation_uncovered_lines_steps.py @@ -459,9 +459,13 @@ def step_have_state_with_retry_count(context: Any, count: int) -> None: "I check uncovered langgraph should_retry with FAIL validation and retry_count {count:d}" ) def step_check_should_retry_uncovered(context: Any, count: int) -> None: - """Check should_retry and verify retry_count increment.""" + """Check should_retry returns correct decision; then exercise handle_retry.""" decision = context.graph._should_retry(context.state) context.retry_decision = decision + # _should_retry no longer mutates state — _handle_retry does the increment. + # Simulate what the graph compiler does: apply handle_retry's return value. + update = context.graph._handle_retry(context.state) + context.state["retry_count"] = update["retry_count"] context.final_retry_count = context.state.get("retry_count") diff --git a/features/steps/tdd_cleanup_stale_destroys_execute_output_steps.py b/features/steps/tdd_cleanup_stale_destroys_execute_output_steps.py new file mode 100644 index 000000000..9508ae105 --- /dev/null +++ b/features/steps/tdd_cleanup_stale_destroys_execute_output_steps.py @@ -0,0 +1,245 @@ +"""Steps for tdd_cleanup_stale_destroys_execute_output.feature. + +TDD issue-capture test for bug #11121: +_create_sandbox_for_plan() calls GitWorktreeSandbox.cleanup_stale() unconditionally, +destroying the cleveragents/plan- branch when the plan is already in +execute/complete state (awaiting apply). + +The scenarios are tagged @tdd_expected_fail so CI passes while the bug is unfixed. +Once the companion fix (issue #11121) is merged, the @tdd_expected_fail tag is +removed and these scenarios become permanent regression guards. +""" + +from __future__ import annotations + +import contextlib +import shutil +import subprocess +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context + +_PLAN_ID = "01TDDSANDBOX000000000000A" +_BRANCH_NAME = f"cleveragents/plan-{_PLAN_ID}" + + +def _git(args: list[str], cwd: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", *args], + cwd=cwd, + capture_output=True, + text=True, + check=True, + timeout=15, + ) + + +def _branch_exists(repo_path: str, branch: str) -> bool: + """Return True if the given branch exists in the repo.""" + result = subprocess.run( + ["git", "rev-parse", "--verify", f"refs/heads/{branch}"], + cwd=repo_path, + capture_output=True, + text=True, + check=False, + timeout=10, + ) + return result.returncode == 0 + + +def _init_git_repo(path: str) -> None: + _git(["init", "-q", "-b", "main"], path) + _git(["config", "user.name", "TDD Test"], path) + _git(["config", "user.email", "tdd@test.local"], path) + _git(["config", "commit.gpgsign", "false"], path) + + +def _build_execute_complete_mocks( + context: Context, + repo_path: str, +) -> None: + """Build mock service + container for a plan in execute/complete state.""" + from cleveragents.domain.models.core.plan import PlanPhase, ProcessingState + + mock_resource = MagicMock() + mock_resource.resource_type_name = "git-checkout" + mock_resource.location = repo_path + mock_resource.resource_id = "res-tdd-11121-test" + + mock_lr = MagicMock() + mock_lr.resource_id = "res-tdd-11121-test" + + mock_project = MagicMock() + mock_project.linked_resources = [mock_lr] + + # Plan is in execute/complete state — this is the critical state for the bug + mock_plan = MagicMock() + mock_plan.project_links = [MagicMock(project_name="local/tdd-11121-project")] + mock_plan.phase = PlanPhase.EXECUTE + mock_plan.processing_state = ProcessingState.COMPLETE + mock_plan.state = ProcessingState.COMPLETE + + mock_service = MagicMock() + mock_service.get_plan.return_value = mock_plan + + mock_project_repo = MagicMock() + mock_project_repo.get.return_value = mock_project + + mock_resource_registry = MagicMock() + mock_resource_registry.show_resource.return_value = mock_resource + + mock_container = MagicMock() + mock_container.namespaced_project_repo.return_value = mock_project_repo + mock_container.resource_registry_service.return_value = mock_resource_registry + + context.tdd11121_service = mock_service + context.tdd11121_container = mock_container + context.tdd11121_repo_path = repo_path + context.tdd11121_plan_id = _PLAN_ID + context.tdd11121_branch = _BRANCH_NAME + + +@given("a temp git repo with an execute-output branch for tdd 11121") +def step_create_git_repo_with_execute_output(context: Context) -> None: + """Create a real git repo with a cleveragents/plan- branch holding execute output. + + This simulates the state after a successful plan execute: the worktree branch + exists and contains at least one committed file representing execution output. + """ + d = tempfile.mkdtemp(prefix="tdd-11121-") + context.add_cleanup(shutil.rmtree, d, True) + + # Initialise the repo with a base commit on main + _init_git_repo(d) + Path(d, "README.md").write_text("# project\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", "init: base commit"], d) + + # Create the cleveragents/plan- branch and commit an output file to it. + # This simulates what _commit_worktree_changes() does after execute completes. + _git(["checkout", "-q", "-b", _BRANCH_NAME], d) + output_file = Path(d, "generated_output.py") + output_file.write_text("# Generated by plan execute\nresult = 42\n") + _git(["add", "."], d) + _git(["commit", "-q", "-m", f"cleveragents: execute output for plan {_PLAN_ID}"], d) + + # Return to main so the repo is in a normal state + _git(["checkout", "-q", "main"], d) + + context.tdd11121_repo_path = d + # Verify the branch exists before the test + assert _branch_exists(d, _BRANCH_NAME), ( + f"Pre-condition failed: branch {_BRANCH_NAME} should exist before test" + ) + + +@given("a mocked plan service with the plan in execute/complete state for tdd 11121") +def step_mock_service_execute_complete(context: Context) -> None: + """Set up mock service returning a plan in execute/complete state.""" + _build_execute_complete_mocks(context, context.tdd11121_repo_path) + + +@when( + "I call _create_sandbox_for_plan a second time on the execute/complete plan for tdd 11121" +) +def step_call_create_sandbox_second_time(context: Context) -> None: + """Call _create_sandbox_for_plan on a plan already in execute/complete state. + + This simulates the user re-running 'agents plan execute ' after + execution has already completed. The bug causes cleanup_stale to run and + destroy the cleveragents/plan- branch that holds the execute output. + """ + from cleveragents.cli.commands.plan import _create_sandbox_for_plan + + context.tdd11121_second_call_exception: Exception | None = None + try: + with ( + patch( + "cleveragents.cli.commands.plan.get_container", + return_value=context.tdd11121_container, + ), + patch( + "cleveragents.application.container.get_container", + return_value=context.tdd11121_container, + ), + ): + _sandbox_root, sandbox_infos = _create_sandbox_for_plan( + context.tdd11121_plan_id, + context.tdd11121_service, + ) + context.tdd11121_sandbox_infos = sandbox_infos + # Clean up any newly created sandbox to avoid resource leaks + for sinfo in sandbox_infos: + with contextlib.suppress(Exception): + sinfo.sandbox_obj.cleanup() + except Exception as exc: + # Record but don't re-raise — we want to check branch state regardless + context.tdd11121_second_call_exception = exc + + +@then( + "the cleveragents plan branch should still exist after the second call for tdd 11121" +) +def step_branch_still_exists(context: Context) -> None: + """Assert the cleveragents/plan- branch was NOT destroyed by cleanup_stale. + + BUG: This assertion FAILS because cleanup_stale runs unconditionally inside + _create_sandbox_for_plan, deleting the branch even when the plan is in + execute/complete state. + + EXPECTED (after fix): The branch survives because _create_sandbox_for_plan + skips cleanup_stale when the plan is already execute/complete. + """ + repo_path = context.tdd11121_repo_path + branch = context.tdd11121_branch + assert _branch_exists(repo_path, branch), ( + f"Bug #11121: branch '{branch}' was destroyed by cleanup_stale during " + f"a second _create_sandbox_for_plan call on an execute/complete plan. " + f"The branch must survive until plan apply merges it." + ) + + +@then("the apply sandbox changes should find at least one artifact for tdd 11121") +def step_apply_finds_artifacts(context: Context) -> None: + """Assert that apply can find the execute output after a re-invoked execute. + + BUG: This assertion FAILS because cleanup_stale destroyed the branch, so + git diff HEAD...cleveragents/plan- finds nothing (branch is gone). + + EXPECTED (after fix): The branch still exists, so apply finds the committed + output file and reports at least one artifact. + """ + repo_path = context.tdd11121_repo_path + branch = context.tdd11121_branch + + # Check if the branch exists at all — if not, apply will find zero artifacts + if not _branch_exists(repo_path, branch): + # This is the bug: the branch was destroyed, so apply would find 0 artifacts + raise AssertionError( + f"Bug #11121: branch '{branch}' was destroyed by cleanup_stale. " + f"plan apply would find 0 artifacts (empty changeset). " + f"The branch must persist until apply merges it." + ) + + # Branch exists — count the artifacts that apply would find + result = subprocess.run( + ["git", "diff", "--stat", f"HEAD...{branch}"], + cwd=repo_path, + capture_output=True, + text=True, + check=False, + timeout=10, + ) + stat_lines = (result.stdout or "").strip().splitlines() + # git diff --stat output: last line is summary "N files changed, ..." + # Artifact count = number of lines minus the summary line + artifact_count = max(0, len(stat_lines) - 1) if stat_lines else 0 + + assert artifact_count > 0, ( + f"Bug #11121: plan apply would find {artifact_count} artifacts after " + f"re-invoked execute on execute/complete plan. Expected >= 1 artifact. " + f"diff --stat output: {result.stdout!r}" + ) diff --git a/features/steps/tdd_quality_gate_steps.py b/features/steps/tdd_quality_gate_steps.py index a71982bd2..7d045d8bc 100644 --- a/features/steps/tdd_quality_gate_steps.py +++ b/features/steps/tdd_quality_gate_steps.py @@ -60,7 +60,7 @@ def _default_pr_diff_for_bug_refs( robot_hits = list(search_root.rglob("*.robot")) for rp in robot_hits: try: - if f"tdd_bug_{bug_num}" in rp.read_text(encoding="utf-8"): + if f"tdd_issue_{bug_num}" in rp.read_text(encoding="utf-8"): use_robot = True break except (OSError, UnicodeDecodeError): @@ -74,8 +74,8 @@ def _default_pr_diff_for_bug_refs( f"--- a/robot/bug{bug_num}.robot", f"+++ b/robot/bug{bug_num}.robot", "@@ -1 +1 @@", - f"-tdd_expected_fail tdd_bug tdd_bug_{bug_num}", - f"+tdd_bug tdd_bug_{bug_num}", + f"-tdd_expected_fail tdd_issue tdd_issue_{bug_num}", + f"+tdd_issue tdd_issue_{bug_num}", ] ) ) @@ -87,8 +87,8 @@ def _default_pr_diff_for_bug_refs( f"--- a/features/bug{bug_num}.feature", f"+++ b/features/bug{bug_num}.feature", "@@ -1 +1 @@", - f"-@tdd_expected_fail @tdd_bug @tdd_bug_{bug_num}", - f"+@tdd_bug @tdd_bug_{bug_num}", + f"-@tdd_expected_fail @tdd_issue @tdd_issue_{bug_num}", + f"+@tdd_issue @tdd_issue_{bug_num}", ] ) ) @@ -360,8 +360,8 @@ def step_given_robot_diff_removes_expected_fail(context: object, bug_num: int) - f"--- a/robot/bug{bug_num}.robot", f"+++ b/robot/bug{bug_num}.robot", "@@ -1 +1 @@", - f"-tdd_expected_fail tdd_bug tdd_bug_{bug_num}", - f"+tdd_bug tdd_bug_{bug_num}", + f"-tdd_expected_fail tdd_issue tdd_issue_{bug_num}", + f"+tdd_issue tdd_issue_{bug_num}", ] ) @@ -376,7 +376,7 @@ def step_given_unreadable_feature_file(context: object, bug_num: int) -> None: # UnicodeDecodeError (caught as OSError subclass). This is root-safe # unlike chmod(0o000) which root bypasses. full_path.write_bytes( - f"@tdd_expected_fail @tdd_bug @tdd_bug_{bug_num}".encode() + b"\xff\xfe" + f"@tdd_expected_fail @tdd_issue @tdd_issue_{bug_num}".encode() + b"\xff\xfe" ) @@ -439,8 +439,8 @@ def step_given_pr_diff_removes_wrong_bug( "--- a/features/bugs99.feature", "+++ b/features/bugs99.feature", "@@ -1 +1 @@", - f"-@tdd_expected_fail @tdd_bug @tdd_bug_{other}", - f"+@tdd_bug @tdd_bug_{other}", + f"-@tdd_expected_fail @tdd_issue @tdd_issue_{other}", + f"+@tdd_issue @tdd_issue_{other}", ] ) diff --git a/features/tdd_cleanup_stale_destroys_execute_output.feature b/features/tdd_cleanup_stale_destroys_execute_output.feature new file mode 100644 index 000000000..6035e0af7 --- /dev/null +++ b/features/tdd_cleanup_stale_destroys_execute_output.feature @@ -0,0 +1,26 @@ +@tdd_issue @tdd_issue_11121 +Feature: TDD Issue #11121 — cleanup_stale destroys git worktree branch on re-invoked execute + As a developer + I want to verify that _create_sandbox_for_plan does NOT delete the cleveragents/plan- + branch when the plan is already in execute/complete state + So that plan apply can subsequently find and merge the correct artifacts + + Bug #11121: _create_sandbox_for_plan() calls GitWorktreeSandbox.cleanup_stale() + unconditionally every time agents plan execute is invoked — including when the plan + is already in execute/complete state (execution finished, awaiting apply). This deletes + the cleveragents/plan- git branch that holds the execution output, so when + agents plan apply runs next it finds no branch and merges zero artifacts. + + @tdd_issue @tdd_issue_11121 @tdd_expected_fail @mock_only + Scenario: cleveragents/plan- branch survives a second _create_sandbox_for_plan call when plan is execute/complete + Given a temp git repo with an execute-output branch for tdd 11121 + And a mocked plan service with the plan in execute/complete state for tdd 11121 + When I call _create_sandbox_for_plan a second time on the execute/complete plan for tdd 11121 + Then the cleveragents plan branch should still exist after the second call for tdd 11121 + + @tdd_issue @tdd_issue_11121 @tdd_expected_fail @mock_only + Scenario: plan apply finds non-zero artifacts after re-invoked execute on execute/complete plan + Given a temp git repo with an execute-output branch for tdd 11121 + And a mocked plan service with the plan in execute/complete state for tdd 11121 + When I call _create_sandbox_for_plan a second time on the execute/complete plan for tdd 11121 + Then the apply sandbox changes should find at least one artifact for tdd 11121 diff --git a/features/tdd_quality_gate.feature b/features/tdd_quality_gate.feature index 3b65982d7..c03e3303f 100644 --- a/features/tdd_quality_gate.feature +++ b/features/tdd_quality_gate.feature @@ -70,52 +70,52 @@ Feature: TDD bug tag quality gate for bug fix PRs # --- TDD test search --- Scenario: Find TDD test in .feature file - Given a temporary directory with a file "tests/bug.feature" containing "@tdd_bug_42" + Given a temporary directory with a file "tests/bug.feature" containing "@tdd_issue_42" When I search for TDD tests for bug 42 Then the search should find 1 test file Scenario: Find TDD test in .robot file - Given a temporary directory with a file "tests/bug.robot" containing "tdd_bug_42" + Given a temporary directory with a file "tests/bug.robot" containing "tdd_issue_42" When I search for TDD tests for bug 42 Then the search should find 1 test file Scenario: Find TDD tests in both .feature and .robot files - Given a temporary directory with a file "features/bug.feature" containing "@tdd_bug_42" - And a temporary directory also has a file "robot/bug.robot" containing "tdd_bug_42" + Given a temporary directory with a file "features/bug.feature" containing "@tdd_issue_42" + And a temporary directory also has a file "robot/bug.robot" containing "tdd_issue_42" When I search for TDD tests for bug 42 Then the search should find 2 test files Scenario: No TDD test found for bug number - Given a temporary directory with a file "tests/other.feature" containing "@tdd_bug_99" + Given a temporary directory with a file "tests/other.feature" containing "@tdd_issue_99" When I search for TDD tests for bug 42 Then the search should find 0 test files Scenario: Do not match partial TDD bug tags - Given a temporary directory with a file "tests/partial.feature" containing "@tdd_bug_420" + Given a temporary directory with a file "tests/partial.feature" containing "@tdd_issue_420" When I search for TDD tests for bug 42 Then the search should find 0 test files # --- Tag removal verification --- Scenario: Expected fail tag still present in .feature file - Given a temporary directory with a file "tests/bug.feature" containing "@tdd_expected_fail @tdd_bug @tdd_bug_42" + Given a temporary directory with a file "tests/bug.feature" containing "@tdd_expected_fail @tdd_issue @tdd_issue_42" When I check expected fail removal for bug 42 Then there should be 1 removal error And the removal error should mention "@tdd_expected_fail" - And the removal error should mention "@tdd_bug_42" + And the removal error should mention "@tdd_issue_42" Scenario: Expected fail tag removed from .feature file - Given a temporary directory with a file "tests/bug.feature" containing "@tdd_bug @tdd_bug_42" + Given a temporary directory with a file "tests/bug.feature" containing "@tdd_issue @tdd_issue_42" When I check expected fail removal for bug 42 Then there should be 0 removal errors Scenario: Expected fail tag still present in .robot file - Given a temporary directory with a file "tests/bug.robot" containing "tdd_expected_fail tdd_bug_42" + Given a temporary directory with a file "tests/bug.robot" containing "tdd_expected_fail tdd_issue_42" When I check expected fail removal for bug 42 Then there should be 1 removal error Scenario: Expected fail tag removed from .robot file - Given a temporary directory with a file "tests/bug.robot" containing "tdd_bug tdd_bug_42" + Given a temporary directory with a file "tests/bug.robot" containing "tdd_issue tdd_issue_42" When I check expected fail removal for bug 42 Then there should be 0 removal errors @@ -135,35 +135,35 @@ Feature: TDD bug tag quality gate for bug fix PRs And the quality gate errors should mention "No TDD test found for bug #42" Scenario: Quality gate fails when expected fail tag is still present - Given a temporary search root with file "features/bug.feature" containing "@tdd_expected_fail @tdd_bug @tdd_bug_42" + Given a temporary search root with file "features/bug.feature" containing "@tdd_expected_fail @tdd_issue @tdd_issue_42" And a PR description "Fixes #42" When I run the quality gate Then the quality gate should fail And the quality gate errors should mention "@tdd_expected_fail" Scenario: Quality gate passes when expected fail tag has been removed - Given a temporary search root with file "features/bug.feature" containing "@tdd_bug @tdd_bug_42" + Given a temporary search root with file "features/bug.feature" containing "@tdd_issue @tdd_issue_42" And a PR description "Fixes #42" When I run the quality gate Then the quality gate should pass Scenario: Quality gate handles multiple bug references - Given a temporary search root with file "features/bug10.feature" containing "@tdd_bug @tdd_bug_10" - And the search root also has file "features/bug20.feature" containing "@tdd_expected_fail @tdd_bug @tdd_bug_20" + Given a temporary search root with file "features/bug10.feature" containing "@tdd_issue @tdd_issue_10" + And the search root also has file "features/bug20.feature" containing "@tdd_expected_fail @tdd_issue @tdd_issue_20" And a PR description "Fixes #10 and fixes #20" When I run the quality gate Then the quality gate should fail - And the quality gate errors should mention "@tdd_bug_20" + And the quality gate errors should mention "@tdd_issue_20" Scenario: Quality gate passes when all bugs have clean TDD tests - Given a temporary search root with file "features/bug10.feature" containing "@tdd_bug @tdd_bug_10" - And the search root also has file "robot/bug20.robot" containing "tdd_bug tdd_bug_20" + Given a temporary search root with file "features/bug10.feature" containing "@tdd_issue @tdd_issue_10" + And the search root also has file "robot/bug20.robot" containing "tdd_issue tdd_issue_20" And a PR description "Fixes #10 and fixes #20" When I run the quality gate Then the quality gate should pass Scenario: Quality gate fails when PR diff does not remove expected fail tags - Given a temporary search root with file "features/bug.feature" containing "@tdd_bug @tdd_bug_42" + Given a temporary search root with file "features/bug.feature" containing "@tdd_issue @tdd_issue_42" And a PR description "Fixes #42" And the PR diff does not remove expected fail tags When I run the quality gate @@ -171,7 +171,7 @@ Feature: TDD bug tag quality gate for bug fix PRs And the quality gate errors should mention "No removal of @tdd_expected_fail / tdd_expected_fail detected" Scenario: Quality gate passes for robot diff with expected fail removed across hunks - Given a temporary search root with file "robot/bug.robot" containing "tdd_bug tdd_bug_42" + Given a temporary search root with file "robot/bug.robot" containing "tdd_issue tdd_issue_42" And a PR description "Fixes #42" And the PR diff removes expected fail for robot bug 42 When I run the quality gate @@ -228,8 +228,8 @@ Feature: TDD bug tag quality gate for bug fix PRs # --- Co-located bug false positive guard (M1) --- Scenario: Diff detection does not false-positive for co-located bug tests - Given a temporary search root with file "features/bugs.feature" containing "@tdd_bug @tdd_bug_42" - And the search root also has file "features/bugs99.feature" containing "@tdd_bug @tdd_bug_99" + Given a temporary search root with file "features/bugs.feature" containing "@tdd_issue @tdd_issue_42" + And the search root also has file "features/bugs99.feature" containing "@tdd_issue @tdd_issue_99" And a PR description "Fixes #42" And the PR diff only removes expected fail for bug 99 not bug 42 When I run the quality gate diff --git a/noxfile.py b/noxfile.py index b7d08b74a..13ba4d3f3 100644 --- a/noxfile.py +++ b/noxfile.py @@ -891,7 +891,7 @@ def tdd_quality_gate(session: nox.Session): 1. Every bug referenced via closing keywords (``Fixes #N``, ``Closes #N``, ``Resolves #N``, ``ISSUES CLOSED: #N``) has - a corresponding TDD test tagged ``@tdd_bug_N``. + a corresponding TDD test tagged ``@tdd_issue_N``. 2. The ``@tdd_expected_fail`` / ``tdd_expected_fail`` tag has been removed from each of those tests in the PR diff. diff --git a/robot/helper_tdd_quality_gate.py b/robot/helper_tdd_quality_gate.py index 790f5fd0f..12ce0ea05 100644 --- a/robot/helper_tdd_quality_gate.py +++ b/robot/helper_tdd_quality_gate.py @@ -55,7 +55,7 @@ def _default_pr_diff_for_bug_refs( robot_hits = list(search_root.rglob("*.robot")) for rp in robot_hits: try: - if f"tdd_bug_{bug_num}" in rp.read_text(encoding="utf-8"): + if f"tdd_issue_{bug_num}" in rp.read_text(encoding="utf-8"): use_robot = True break except (OSError, UnicodeDecodeError): @@ -72,8 +72,8 @@ def _default_pr_diff_for_bug_refs( f"--- a/robot/bug{bug_num}.robot", f"+++ b/robot/bug{bug_num}.robot", "@@ -1 +1 @@", - f"-tdd_expected_fail tdd_bug tdd_bug_{bug_num}", - f"+tdd_bug tdd_bug_{bug_num}", + f"-tdd_expected_fail tdd_issue tdd_issue_{bug_num}", + f"+tdd_issue tdd_issue_{bug_num}", ] ) ) @@ -88,8 +88,8 @@ def _default_pr_diff_for_bug_refs( f"--- a/features/bug{bug_num}.feature", f"+++ b/features/bug{bug_num}.feature", "@@ -1 +1 @@", - f"-@tdd_expected_fail @tdd_bug @tdd_bug_{bug_num}", - f"+@tdd_bug @tdd_bug_{bug_num}", + f"-@tdd_expected_fail @tdd_issue @tdd_issue_{bug_num}", + f"+@tdd_issue @tdd_issue_{bug_num}", ] ) ) @@ -161,9 +161,9 @@ def no_bug_refs_pass() -> int: def find_feature_test() -> int: - """Verify finding @tdd_bug_N in .feature files.""" + """Verify finding @tdd_issue_N in .feature files.""" tmp = _make_temp_tree( - {"features/bug.feature": "@tdd_bug @tdd_bug_42\nFeature: Test\n"} + {"features/bug.feature": "@tdd_issue @tdd_issue_42\nFeature: Test\n"} ) try: tests = find_tdd_tests(42, tmp) @@ -177,8 +177,8 @@ def find_feature_test() -> int: def find_robot_test() -> int: - """Verify finding tdd_bug_N in .robot files.""" - tmp = _make_temp_tree({"robot/bug.robot": "[Tags] tdd_bug tdd_bug_42\n"}) + """Verify finding tdd_issue_N in .robot files.""" + tmp = _make_temp_tree({"robot/bug.robot": "[Tags] tdd_issue tdd_issue_42\n"}) try: tests = find_tdd_tests(42, tmp) if len(tests) != 1: @@ -192,7 +192,8 @@ def find_robot_test() -> int: def find_exact_tag_match() -> int: """Verify partial tags are not treated as exact bug tag matches.""" - tmp = _make_temp_tree({"features/partial.feature": "@tdd_bug_420\nFeature: Test\n"}) + text = "@tdd_issue_420\nFeature: Test\n" + tmp = _make_temp_tree({"features/partial.feature": text}) try: tests = find_tdd_tests(42, tmp) if tests: @@ -231,7 +232,7 @@ def no_tdd_test_fails() -> int: def expected_fail_present() -> int: """Verify the gate fails when @tdd_expected_fail is still present.""" - ef_content = "@tdd_expected_fail @tdd_bug @tdd_bug_42\nFeature: Test\n" + ef_content = "@tdd_expected_fail @tdd_issue @tdd_issue_42\nFeature: Test\n" tmp = _make_temp_tree({"features/bug.feature": ef_content}) try: pr_diff = _default_pr_diff_for_bug_refs([42], tmp) @@ -256,7 +257,7 @@ def expected_fail_present() -> int: def expected_fail_removed() -> int: """Verify the gate passes when @tdd_expected_fail has been removed.""" tmp = _make_temp_tree( - {"features/bug.feature": "@tdd_bug @tdd_bug_42\nFeature: Test\n"} + {"features/bug.feature": "@tdd_issue @tdd_issue_42\nFeature: Test\n"} ) try: pr_diff = _default_pr_diff_for_bug_refs([42], tmp) @@ -274,9 +275,9 @@ def multi_bug_mixed() -> int: """Verify the gate handles multiple bugs with mixed outcomes.""" tmp = _make_temp_tree( { - "features/bug10.feature": "@tdd_bug @tdd_bug_10\nFeature: Bug 10\n", + "features/bug10.feature": "@tdd_issue @tdd_issue_10\nFeature: Bug 10\n", "features/bug20.feature": ( - "@tdd_expected_fail @tdd_bug @tdd_bug_20\nFeature: Bug 20\n" + "@tdd_expected_fail @tdd_issue @tdd_issue_20\nFeature: Bug 20\n" ), } ) @@ -288,11 +289,11 @@ def multi_bug_mixed() -> int: if not errors: print("FAIL: expected errors for bug #20", file=sys.stderr) return 1 - if not any("@tdd_bug_20" in e for e in errors): + if not any("@tdd_issue_20" in e for e in errors): print(f"FAIL: expected error about bug #20, got: {errors}", file=sys.stderr) return 1 # Bug #10 should not have errors - if any("@tdd_bug_10" in e for e in errors): + if any("@tdd_issue_10" in e for e in errors): print(f"FAIL: unexpected error about bug #10: {errors}", file=sys.stderr) return 1 print("multi-bug-mixed-ok") @@ -305,8 +306,8 @@ def all_clean_passes() -> int: """Verify the gate passes when all bugs have clean TDD tests.""" tmp = _make_temp_tree( { - "features/bug10.feature": "@tdd_bug @tdd_bug_10\nFeature: Bug 10\n", - "robot/bug20.robot": "[Tags] tdd_bug tdd_bug_20\n", + "features/bug10.feature": "@tdd_issue @tdd_issue_10\nFeature: Bug 10\n", + "robot/bug20.robot": "[Tags] tdd_issue tdd_issue_20\n", } ) try: @@ -327,8 +328,8 @@ def both_behave_and_robot() -> int: """Verify the gate checks tests in both .feature and .robot files.""" tmp = _make_temp_tree( { - "features/bug.feature": "@tdd_bug @tdd_bug_42\nFeature: Bug\n", - "robot/bug.robot": "[Tags] tdd_bug tdd_bug_42\n", + "features/bug.feature": "@tdd_issue @tdd_issue_42\nFeature: Bug\n", + "robot/bug.robot": "[Tags] tdd_issue tdd_issue_42\n", } ) try: @@ -350,7 +351,7 @@ def both_behave_and_robot() -> int: def diff_removal_required() -> int: """Verify the gate fails when PR diff has no expected-fail removal.""" tmp = _make_temp_tree( - {"features/bug.feature": "@tdd_bug @tdd_bug_42\nFeature: Test\n"} + {"features/bug.feature": "@tdd_issue @tdd_issue_42\nFeature: Test\n"} ) try: errors, _refs = run_quality_gate("Fixes #42", tmp, pr_diff="") diff --git a/robot/plan_generation_graph.robot b/robot/plan_generation_graph.robot index f6fbc3d29..8dfc7796d 100644 --- a/robot/plan_generation_graph.robot +++ b/robot/plan_generation_graph.robot @@ -83,9 +83,10 @@ 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 4 nodes + Should Contain ${result.stdout} Graph has 5 nodes Should Be Equal As Integers ${result.rc} 0 LangGraph Graphs Package Exports Workflow Classes @@ -205,7 +206,6 @@ Should Retry Returns Retry When Validation Fails And Retries Available ... } ... decision = graph._should_retry(state) ... assert decision == 'retry' - ... assert state['retry_count'] == 1 ... print('Should retry: retry decision correct') ${result}= Run Process ${PYTHON} -c ${script} shell=True Should Contain ${result.stdout} Should retry: retry decision correct diff --git a/scripts/tdd_quality_gate.py b/scripts/tdd_quality_gate.py index 43fb0a172..1eff0adfb 100644 --- a/scripts/tdd_quality_gate.py +++ b/scripts/tdd_quality_gate.py @@ -1,15 +1,18 @@ #!/usr/bin/env python3 -"""TDD bug tag quality gate for bug fix PRs. +"""TDD issue tag quality gate for PRs. Enforces the TDD bug fix workflow rules described in CONTRIBUTING.md: -1. Parses the PR description for closing keywords that reference bug issues +1. Parses the PR description for closing keywords that reference issues (``Closes #N``, ``Fixes #N``, ``Resolves #N``, ``ISSUES CLOSED: #N``). -2. Searches the codebase for tests tagged ``@tdd_bug_N`` (Behave ``.feature`` - files) or ``tdd_bug_N`` (Robot ``.robot`` files). -3. Verifies that every such test has had its ``@tdd_expected_fail`` / - ``tdd_expected_fail`` tag removed — the fix PR must remove the - expected-fail marker as proof the bug is now fixed. +2. Searches the codebase for tests tagged ``@tdd_issue_N`` (Behave ``.feature`` + files) or ``tdd_issue_N`` (Robot ``.robot`` files). +3. For **bug fix PRs**, verifies that every such test has had its + ``@tdd_expected_fail`` / ``tdd_expected_fail`` tag removed — the fix PR + must remove the expected-fail marker as proof the bug is now fixed. +4. For **TDD issue-capture PRs** (which *add* ``@tdd_expected_fail``), the + gate passes — this is the prerequisite step that introduces the failing + test before the fix. Exit codes: 0 — All checks passed (or PR references no bugs). @@ -101,6 +104,69 @@ def _collect_pr_diff(search_root: Path, base_ref: str) -> str: ) +def _diff_is_tdd_issue_capture(pr_diff: str) -> list[int]: + """Return sorted list of bug issue numbers that have ``@tdd_expected_fail`` + being **added** in the PR diff — indicating this is a TDD issue-capture PR + (the prerequisite step that introduces a failing test before the fix). + + Returns an empty list if this is not an issue-capture PR. + """ + if not isinstance(pr_diff, str): + raise TypeError(f"pr_diff must be a str, got {type(pr_diff).__name__}") + + current_suffix = "" + in_hunk = False + captured_bug_nums: set[int] = set() + + _ISSUE_TAG_RE = re.compile(r"@tdd_issue_(\d+)") + + for line in pr_diff.splitlines(): + if line.startswith("+++ "): + raw_path = line[4:] + if raw_path.startswith("b/"): + raw_path = raw_path[2:] + current_suffix = Path(raw_path).suffix.lower() + in_hunk = False + continue + + if current_suffix not in {".feature", ".robot"}: + continue + + if line.startswith("@@"): + in_hunk = True + continue + + if not in_hunk: + continue + + if not line or line[0] != "+": + continue + + content = line[1:] + if current_suffix == ".feature": + expected_fail_tag = "@tdd_expected_fail" + else: + expected_fail_tag = "tdd_expected_fail" + + if not _contains_tag_token(content, expected_fail_tag): + continue + + # Found a line that adds @tdd_expected_fail - extract the bug numbers + for match in _ISSUE_TAG_RE.finditer(content): + num = int(match.group(1)) + if num > 0: + captured_bug_nums.add(num) + + # Also check for tdd_issue_N in robot files + if current_suffix == ".robot": + for match in re.finditer(r"tdd_issue_(\d+)", content): + num = int(match.group(1)) + if num > 0: + captured_bug_nums.add(num) + + return sorted(captured_bug_nums) + + def _diff_has_expected_fail_removal_for_bug(pr_diff: str, bug_number: int) -> bool: """Return True when PR diff removes expected-fail for ``bug_number``.""" if not isinstance(pr_diff, str): @@ -145,10 +211,10 @@ def _diff_has_expected_fail_removal_for_bug(pr_diff: str, bug_number: int) -> bo content = line[1:] if current_suffix == ".feature": - bug_tag = f"@tdd_bug_{bug_number}" + bug_tag = f"@tdd_issue_{bug_number}" expected_fail_tag = "@tdd_expected_fail" else: - bug_tag = f"tdd_bug_{bug_number}" + bug_tag = f"tdd_issue_{bug_number}" expected_fail_tag = "tdd_expected_fail" if _contains_tag_token(content, bug_tag): @@ -204,10 +270,10 @@ def find_tdd_tests( bug_number: int, search_root: Path, ) -> list[Path]: - """Find test files tagged with ``@tdd_bug_``. + """Find test files tagged with ``@tdd_issue_``. - Searches ``.feature`` files for ``@tdd_bug_`` and ``.robot`` - files for ``tdd_bug_``. + Searches ``.feature`` files for ``@tdd_issue_`` and ``.robot`` + files for ``tdd_issue_``. Returns a list of paths that contain the tag. """ @@ -220,8 +286,8 @@ def find_tdd_tests( if not isinstance(search_root, Path): raise TypeError(f"search_root must be a Path, got {type(search_root).__name__}") - tag_behave = f"@tdd_bug_{bug_number}" - tag_robot = f"tdd_bug_{bug_number}" + tag_behave = f"@tdd_issue_{bug_number}" + tag_robot = f"tdd_issue_{bug_number}" matches: list[Path] = [] # Search .feature files @@ -286,9 +352,9 @@ def check_expected_fail_removed( if _contains_tag_token(content, tag): bug_tag_display = ( - f"@tdd_bug_{bug_number}" + f"@tdd_issue_{bug_number}" if suffix == ".feature" - else f"tdd_bug_{bug_number}" + else f"tdd_issue_{bug_number}" ) errors.append( f"Bug fix PR must remove the {tag} tag from tests tagged " @@ -341,14 +407,23 @@ def run_quality_gate( return [str(exc)], bug_refs all_errors: list[str] = [] + issue_capture_bugs: list[int] = [] for bug_num in bug_refs: test_files = find_tdd_tests(bug_num, search_root) if not test_files: + # No existing test found for this bug number — might be a + # TDD issue-capture PR that is *adding* the test with + # @tdd_expected_fail. Check the diff for this case. + captured = _diff_is_tdd_issue_capture(pr_diff) + if captured: + issue_capture_bugs.extend(captured) + continue + all_errors.append( f"No TDD test found for bug #{bug_num}. " - f"The TDD workflow requires a test tagged @tdd_bug_{bug_num} " + f"The TDD workflow requires a test tagged @tdd_issue_{bug_num} " f"to exist before the bug can be fixed. " f"See CONTRIBUTING.md > Bug Fix Workflow." ) @@ -385,6 +460,25 @@ def main() -> int: errors, bug_refs = run_quality_gate(pr_description, search_root, base_ref=base_ref) + # Check for TDD issue-capture PRs (the PR adds @tdd_expected_fail) + if not errors and bug_refs: + try: + pr_diff = _collect_pr_diff(search_root, base_ref) + except RuntimeError: + pr_diff = "" + + captured = _diff_is_tdd_issue_capture(pr_diff) + if captured: + print( + "TDD issue-capture PR detected: " + f"adding expected-fail test for bug(s) {captured}" + ) + print( + "This is the prerequisite step — " + "the bug fix PR will remove @tdd_expected_fail." + ) + return 0 + if errors: for error in errors: print(f"ERROR: {error}", file=sys.stderr) diff --git a/src/cleveragents/agents/graphs/plan_generation.py b/src/cleveragents/agents/graphs/plan_generation.py index 1897b823f..c6ece6a31 100644 --- a/src/cleveragents/agents/graphs/plan_generation.py +++ b/src/cleveragents/agents/graphs/plan_generation.py @@ -154,6 +154,8 @@ 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. @@ -273,6 +275,7 @@ 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") @@ -282,12 +285,16 @@ 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": "analyze_requirements", + "retry": "handle_retry", "end": END, }, ) @@ -559,12 +566,25 @@ class PlanGenerationGraph: # Check if validation failed and retries available if validation.get("status") == "FAIL" and retry_count < self.max_retries: - # Increment retry count - state["retry_count"] = retry_count + 1 return "retry" return "end" + def _handle_retry(self, state: PlanGenerationState) -> dict[str, Any]: + """Increment the retry counter before routing back to analysis. + + Conditional edge functions in LangGraph cannot persist state + mutations — this node materializes the retry increment in a + proper state update. + + Args: + state: Current workflow state + + Returns: + State update with incremented retry_count + """ + return {"retry_count": state.get("retry_count", 0) + 1} + def _format_context_summary(self, contexts: list[Context]) -> str: """Format context files into a summary string.