CHANGELOG.md

@@ -119,6 +119,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.
+
 - **`LLMTraceRepository.save()` premature commit breaks UnitOfWork transactions** (#7505):
   Replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a
   dual-path implementation that respects the UnitOfWork (UoW) pattern. When an external

CONTRIBUTORS.md

@@ -24,10 +24,11 @@ Below are some of the specific details of various contributions.
 * This project was made possible thanks to considerable donation of time, money, and resources by CleverThis, Inc.
 * HAL 9000 has contributed automated bug fixes, CLI output formatting improvements, and ongoing maintenance as part of the CleverAgents automation system.
 * HAL 9000 has contributed the file edit encoding parameter fix (PR #8258 / issue #7559).
-<<* 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 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 mandatory PR compliance checklist to `implementation-supervisor.md` (#9824): added an 8-item checklist to the worker prompt body with concrete items covering CHANGELOG.md, CONTRIBUTORS.md, commit footer, CI verification, BDD tests, Epic reference, labels, and milestone assignment to eliminate systemic PR merge blockers.
+* 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.
 * HAL 9000 has contributed the LLMTraceRepository data-integrity fix (PR #8185 / issue #7505): replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a dual-path implementation that respects the UnitOfWork pattern — flushing only when an external session is provided, and flushing + committing + closing when operating standalone. This eliminates premature transaction commits, loss of rollback capability, and a docstring/implementation mismatch.
 * HAL 9000 has contributed the ACMS Index Data Model and File Traversal Engine (PR #9664 / issue #9579): foundational data structures for indexed context entries with hot/warm/cold/archive storage tier classification, tag system, and a timeout-safe chunked file traversal engine for large projects with 10,000+ files.

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 = <previous_value>` 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

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

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

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

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

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