From 84c553ca06cb3784d0b481f00ba4674245bb255c Mon Sep 17 00:00:00 2001 From: CleverThis Date: Tue, 5 May 2026 09:23:22 +0000 Subject: [PATCH] fix(repositories): derive PlanResult.success from result_success column instead of error_message --- CHANGELOG.md | 9 + CONTRIBUTORS.md | 1 + docs/specification.md | 2 + .../tdd_plan_result_success_7501_steps.py | 211 ++++++++++++++++++ features/tdd_plan_result_success_7501.feature | 43 ++++ .../m9_003_plan_result_success_column.py | 51 +++++ .../infrastructure/database/models.py | 1 + .../infrastructure/database/repositories.py | 16 +- 8 files changed, 333 insertions(+), 1 deletion(-) create mode 100644 features/steps/tdd_plan_result_success_7501_steps.py create mode 100644 features/tdd_plan_result_success_7501.feature create mode 100644 src/cleveragents/infrastructure/database/migrations/versions/m9_003_plan_result_success_column.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a81003e6..45466330a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,15 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **fix(repositories): derive PlanResult.success from result_success column instead of error_message** (#7501): + Fixed a critical bug in `PlanRepository._to_domain` where `PlanResult.success` was incorrectly + derived from `error_message is None`. Because `error_message` is shared between the build phase + and the result phase, a plan with a historical build error would be marked as failed even after + successfully completing and being applied. The fix introduces a dedicated `result_success` boolean + column in the `plans` table (migration `m9_003_plan_result_success_column`) and updates the + repository read path to use it. For backward compatibility, when `result_success` is NULL + (pre-migration records), the legacy `error_message is None` heuristic is preserved. + - **git_tools._get_base_env() TOCTOU Race Condition** (#7619): Fixed a Time-Of-Check-To-Time-Of-Use race condition in `git_tools._get_base_env()` where two concurrent threads could both observe `_BASE_ENV is None`, both diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 1ce843171..ac0616fc1 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -28,4 +28,5 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the architecture-pool-supervisor milestone assignment feature (PR #8188 / issue #7521): added `forgejo_update_pull_request` permission and documented the PR workflow for major spec changes, enabling automatic milestone assignment for specification PRs. * HAL 9000 has contributed the git worktree TOCTOU race condition fix (PR #8178 / issue #7507): replaced the unsafe mkdtemp() + rmdir() pattern with a parent-directory approach to eliminate the race window in concurrent git worktree operations. * HAL 9000 has contributed the git_tools TOCTOU race condition fix (PR #8255 / issue #7619): eliminated the Time-Of-Check-To-Time-Of-Use race in `_get_base_env()` by adding double-checked locking with a module-level `threading.Lock`, preventing concurrent threads from writing conflicting environment snapshots. +* HAL 9000 has contributed the PlanResult.success derivation fix (PR #8214 / issue #7501): replaced the incorrect `error_message is None` heuristic with a dedicated `result_success` column in the plans table, ensuring plans with historical build errors are not incorrectly marked as failed after a successful apply. * HAL 9000 has contributed comprehensive milestone documentation for v3.6.0 (Advanced Concepts & Deferred Features) and v3.7.0 (TUI Implementation) (PR #9903): split into sub-documents covering context strategies, LLM backends, resource types, A2A rename, container tool execution, scope chain resolution, cost/safety budgets, E2E workflow tests, code review examples, plugin architecture, TUI layout, persona system, reference/command input, session management, configuration, and TuiMaterializer integration. diff --git a/docs/specification.md b/docs/specification.md index e8c02523f..0926d8df8 100644 --- a/docs/specification.md +++ b/docs/specification.md @@ -45871,6 +45871,8 @@ The relational database follows a normalized design with foreign key constraints 4. **Optimistic concurrency control**: The `updated_at` timestamp column on mutable entities serves as a version check. Update operations include `WHERE updated_at = ` to detect concurrent modifications. On conflict, the operation fails with an explicit error rather than silently overwriting. +6. **PlanResult Domain Model — `result_success` column** (migration `m9_003_plan_result_success_column`): The legacy `plans` table includes a dedicated `result_success BOOLEAN NULLABLE` column that explicitly records the success status of the result (apply) phase. `PlanRepository._to_domain` derives `PlanResult.success` from this column rather than from `error_message is None`. The `error_message` column is shared between the build phase and the result phase; using it to infer result success would incorrectly mark plans as failed when a build-phase error was recorded but the apply phase later succeeded. When `result_success` is `NULL` (pre-migration records), the repository falls back to the legacy `error_message is None` heuristic for backward compatibility. + 5. **Datetime handling contract**: All timestamps stored in the database are UTC ISO 8601 strings (format: `YYYY-MM-DDTHH:MM:SS.ffffff`). All Python datetime comparisons involving stored timestamps MUST parse the stored string back to a timezone-aware `datetime` object before comparing — **never** compare ISO timestamp strings lexicographically. String comparison is incorrect because different timezone offset formats (`+00:00` vs `Z` vs `+05:30`) produce strings that are not lexicographically comparable even when representing the same moment. The canonical pattern is: ```python diff --git a/features/steps/tdd_plan_result_success_7501_steps.py b/features/steps/tdd_plan_result_success_7501_steps.py new file mode 100644 index 000000000..933530183 --- /dev/null +++ b/features/steps/tdd_plan_result_success_7501_steps.py @@ -0,0 +1,211 @@ +"""Step definitions for TDD issue #7501 — PlanResult.success derivation. + +Validates that PlanRepository._to_domain derives PlanResult.success from +the dedicated result_success column rather than from error_message is None. + +Targets ``PlanRepository`` in +``src/cleveragents/infrastructure/database/repositories.py``. +""" + +from __future__ import annotations + +import warnings +from datetime import datetime + +from behave import given, then, when +from behave.runner import Context +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +from cleveragents.infrastructure.database.models import Base, PlanModel +from cleveragents.infrastructure.database.repositories import PlanRepository + + +def _make_project(session: object) -> int: + """Insert a minimal project row and return its id.""" + from cleveragents.infrastructure.database.models import ProjectModel + + project = ProjectModel( + name="test-project-7501", + path="/tmp/test-project-7501", + settings={}, + ) + session.add(project) # type: ignore[attr-defined] + session.flush() # type: ignore[attr-defined] + return int(project.id) # type: ignore[attr-defined] + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("a clean in-memory database for plan result success tests") +def step_clean_db_result_success(context: Context) -> None: + """Create a fresh in-memory SQLite database with all tables.""" + engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all(engine) + context.rs_db_engine = engine + session = sessionmaker(bind=engine)() + context.rs_db_session = session + + +@given("a legacy plan repository backed by the database") +def step_legacy_plan_repo(context: Context) -> None: + """Instantiate a PlanRepository using the in-memory session.""" + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + context.rs_plan_repo = PlanRepository(session=context.rs_db_session) + context.rs_retrieved_plan = None + context.rs_project_id = _make_project(context.rs_db_session) + context.rs_db_session.commit() + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _insert_plan_model( + session: object, + project_id: int, + applied_at: datetime | None = None, + error_message: str | None = None, + result_success: bool | None = None, +) -> int: + """Insert a PlanModel row directly and return its id.""" + db_plan = PlanModel( + project_id=project_id, + name="test-plan-7501", + prompt="Test prompt for issue 7501", + status="pending", + current=False, + applied_at=applied_at, + error_message=error_message, + result_success=result_success, + ) + session.add(db_plan) # type: ignore[attr-defined] + session.flush() # type: ignore[attr-defined] + session.commit() # type: ignore[attr-defined] + return int(db_plan.id) # type: ignore[attr-defined] + + +# --------------------------------------------------------------------------- +# Given steps +# --------------------------------------------------------------------------- + + +@given("a legacy plan with applied_at set and result_success column True") +def step_plan_result_success_true(context: Context) -> None: + """Insert a plan row with result_success=True.""" + context.rs_plan_id = _insert_plan_model( + session=context.rs_db_session, + project_id=context.rs_project_id, + applied_at=datetime.now(), + error_message=None, + result_success=True, + ) + + +@given("a legacy plan with applied_at set and result_success column False") +def step_plan_result_success_false(context: Context) -> None: + """Insert a plan row with result_success=False.""" + context.rs_plan_id = _insert_plan_model( + session=context.rs_db_session, + project_id=context.rs_project_id, + applied_at=datetime.now(), + error_message=None, + result_success=False, + ) + + +@given("a legacy plan with a build error_message and result_success column True") +def step_plan_build_error_result_success_true(context: Context) -> None: + """Insert a plan row with a build error but result_success=True. + + This is the core bug scenario: a plan that had a build error but later + succeeded in the apply phase. Without the fix, success would be derived + as False (because error_message is not None). With the fix, success is + correctly derived as True from result_success. + """ + context.rs_plan_id = _insert_plan_model( + session=context.rs_db_session, + project_id=context.rs_project_id, + applied_at=datetime.now(), + error_message="Build phase error: compilation failed", + result_success=True, + ) + + +@given( + "a legacy plan with applied_at set and result_success column NULL and no error_message" +) +def step_plan_null_result_success_no_error(context: Context) -> None: + """Insert a plan row with result_success=NULL and no error_message. + + Simulates a pre-migration record. The legacy heuristic should mark it + as success=True because error_message is None. + """ + context.rs_plan_id = _insert_plan_model( + session=context.rs_db_session, + project_id=context.rs_project_id, + applied_at=datetime.now(), + error_message=None, + result_success=None, + ) + + +@given( + "a legacy plan with applied_at set and result_success column NULL and an error_message" +) +def step_plan_null_result_success_with_error(context: Context) -> None: + """Insert a plan row with result_success=NULL and an error_message. + + Simulates a pre-migration record that failed. The legacy heuristic + should mark it as success=False because error_message is not None. + """ + context.rs_plan_id = _insert_plan_model( + session=context.rs_db_session, + project_id=context.rs_project_id, + applied_at=datetime.now(), + error_message="Apply phase error: deployment failed", + result_success=None, + ) + + +# --------------------------------------------------------------------------- +# When steps +# --------------------------------------------------------------------------- + + +@when("the legacy plan is retrieved from the repository") +def step_retrieve_legacy_plan(context: Context) -> None: + """Retrieve the plan via PlanRepository.get_by_id.""" + context.rs_retrieved_plan = context.rs_plan_repo.get_by_id(context.rs_plan_id) + + +# --------------------------------------------------------------------------- +# Then steps +# --------------------------------------------------------------------------- + + +@then("the retrieved plan result success should be True") +def step_check_result_success_true(context: Context) -> None: + """Assert that the retrieved plan's result.success is True.""" + plan = context.rs_retrieved_plan + assert plan is not None, "Expected a plan but got None" + assert plan.result is not None, "Expected plan.result to be set but it was None" + assert plan.result.success is True, ( + f"Expected plan.result.success=True but got {plan.result.success!r}" + ) + + +@then("the retrieved plan result success should be False") +def step_check_result_success_false(context: Context) -> None: + """Assert that the retrieved plan's result.success is False.""" + plan = context.rs_retrieved_plan + assert plan is not None, "Expected a plan but got None" + assert plan.result is not None, "Expected plan.result to be set but it was None" + assert plan.result.success is False, ( + f"Expected plan.result.success=False but got {plan.result.success!r}" + ) diff --git a/features/tdd_plan_result_success_7501.feature b/features/tdd_plan_result_success_7501.feature new file mode 100644 index 000000000..a40f54f12 --- /dev/null +++ b/features/tdd_plan_result_success_7501.feature @@ -0,0 +1,43 @@ +@tdd_issue @tdd_issue_7501 +Feature: TDD Issue #7501 — PlanResult.success derived from result_success column + As a system operator managing plan lifecycle + I want PlanResult.success to be derived from the dedicated result_success column + So that plans with historical build errors are not incorrectly marked as failed + + The root cause is that PlanRepository._to_domain derived PlanResult.success + from `error_message is None`. Because error_message is shared between the + build phase and the result phase, a plan that had a build error but later + succeeded in the apply phase would be incorrectly reconstructed as failed. + + The fix adds a dedicated result_success column to the plans table and updates + _to_domain to use it. For backward compatibility, when result_success is NULL + (pre-migration records), the legacy heuristic (error_message is None) is used. + + Background: + Given a clean in-memory database for plan result success tests + And a legacy plan repository backed by the database + + Scenario: Plan with result_success=True is reconstructed as success=True + Given a legacy plan with applied_at set and result_success column True + When the legacy plan is retrieved from the repository + Then the retrieved plan result success should be True + + Scenario: Plan with result_success=False is reconstructed as success=False + Given a legacy plan with applied_at set and result_success column False + When the legacy plan is retrieved from the repository + Then the retrieved plan result success should be False + + Scenario: Plan with build error but result_success=True is reconstructed as success=True + Given a legacy plan with a build error_message and result_success column True + When the legacy plan is retrieved from the repository + Then the retrieved plan result success should be True + + Scenario: Plan with NULL result_success falls back to error_message heuristic when no error + Given a legacy plan with applied_at set and result_success column NULL and no error_message + When the legacy plan is retrieved from the repository + Then the retrieved plan result success should be True + + Scenario: Plan with NULL result_success falls back to error_message heuristic when error present + Given a legacy plan with applied_at set and result_success column NULL and an error_message + When the legacy plan is retrieved from the repository + Then the retrieved plan result success should be False diff --git a/src/cleveragents/infrastructure/database/migrations/versions/m9_003_plan_result_success_column.py b/src/cleveragents/infrastructure/database/migrations/versions/m9_003_plan_result_success_column.py new file mode 100644 index 000000000..e1e8e3376 --- /dev/null +++ b/src/cleveragents/infrastructure/database/migrations/versions/m9_003_plan_result_success_column.py @@ -0,0 +1,51 @@ +"""Add result_success column to plans table. + +This migration adds a dedicated ``result_success`` boolean column to the +``plans`` table to accurately track the final result status of a plan's +apply phase. + +Previously, ``PlanRepository._to_domain`` derived ``PlanResult.success`` +from ``error_message is None``. Because ``error_message`` is shared +between the build phase and the result phase, a plan that encountered a +build error but later succeeded in the apply phase would be incorrectly +reconstructed as failed. + +The new ``result_success`` column is nullable to preserve backward +compatibility with existing database records. When ``result_success`` +is NULL (pre-migration records), the repository falls back to the legacy +``error_message is None`` heuristic. + +Revision ID: m9_003_plan_result_success_column +Revises: m10_001_virtual_builtin_actors +Create Date: 2026-05-05 00:00:00 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "m9_003_plan_result_success_column" +down_revision: str | Sequence[str] | None = "m10_001_virtual_builtin_actors" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """Add ``result_success`` column to the ``plans`` table. + + The column is nullable so that existing rows (which have no explicit + result-phase success signal) are not affected. New rows written by + the updated repository will always populate this column. + """ + op.add_column( + "plans", + sa.Column("result_success", sa.Boolean(), nullable=True), + ) + + +def downgrade() -> None: + """Remove ``result_success`` column from the ``plans`` table.""" + op.drop_column("plans", "result_success") diff --git a/src/cleveragents/infrastructure/database/models.py b/src/cleveragents/infrastructure/database/models.py index 2a91de8ed..febf20bf5 100644 --- a/src/cleveragents/infrastructure/database/models.py +++ b/src/cleveragents/infrastructure/database/models.py @@ -142,6 +142,7 @@ class PlanModel(Base): files_created = Column(Integer, nullable=True, default=0) files_modified = Column(Integer, nullable=True, default=0) files_deleted = Column(Integer, nullable=True, default=0) + result_success = Column(Boolean, nullable=True) # Relationships project = relationship("ProjectModel", back_populates="plans") diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 818af488e..0aa0036f6 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -294,6 +294,7 @@ class PlanRepository: files_modified=plan.files_modified, files_deleted=plan.files_deleted, error_message=plan.build.error_message if plan.build else None, + result_success=plan.result.success if plan.result else None, ) self.session.add(db_plan) @@ -372,6 +373,7 @@ class PlanRepository: db_plan.files_created = plan.result.files_created # type: ignore db_plan.files_modified = plan.result.files_modified # type: ignore db_plan.files_deleted = plan.result.files_deleted # type: ignore + db_plan.result_success = plan.result.success # type: ignore self.session.flush() @@ -420,8 +422,20 @@ class PlanRepository: result = None if db_plan.applied_at: # type: ignore + # Derive success from the dedicated result_success column when + # available. For legacy records where result_success is NULL + # (written before migration m9_003), fall back to the old + # heuristic of checking whether error_message is None. + result_success_col = getattr(db_plan, "result_success", None) + if result_success_col is True: + plan_success = True + elif result_success_col is False: + plan_success = False + else: + # NULL — pre-migration record; use legacy heuristic + plan_success = db_plan.error_message is None # type: ignore result = PlanResult( - success=db_plan.error_message is None, # type: ignore + success=plan_success, files_created=db_plan.files_created or 0, # type: ignore files_modified=db_plan.files_modified or 0, # type: ignore files_deleted=db_plan.files_deleted or 0, # type: ignore