fix(core): close the already-DONE TOCTOU in complete_run (#957)

Both PR reviewers independently flagged that the previous fix was check-then-
act. `tasks.update_status` does its own fresh read + compare-and-set, so a
reconciliation DONE (#1032 runs in a daemon thread) landing between our read
and that CAS still raised InvalidTransitionError — and execute_agent's handler
still persisted a successful run as FAILED. Narrower window, same bug.

Replaced the pre-check with a catch: re-read in the handler and treat "the row
is now DONE" as success. That covers the static case and the race in one path.
A genuinely illegal transition (the row moved somewhere that is not DONE) still
raises, leaving the run RUNNING.

Two tests: one flips the row to DONE inside the call, exactly as a losing CAS
would see it (fails on the pre-check version); one flips it to BLOCKED and
asserts the error still propagates.

Author: frankbria

codeframe/core/runtime.py

@@ -13,7 +13,11 @@
 from typing import TYPE_CHECKING, Optional
 
 from codeframe.core import engine_stats, events, tasks
-from codeframe.core.state_machine import TaskStatus, can_transition
+from codeframe.core.state_machine import (
+    InvalidTransitionError,
+    TaskStatus,
+    can_transition,
+)
 from codeframe.core.workspace import Workspace, get_db_connection
 
 logger = logging.getLogger(__name__)
@@ -362,17 +366,25 @@ def complete_run(
     # transition raise here leaves the run RUNNING, which is recoverable.
     #
     # A task already DONE is the goal state, not a failure: reconciliation
-    # (#1032) and manual completion both move a task to DONE while its run is
-    # still active, and DONE -> DONE is a rejected transition. Raising there
-    # would send execute_agent's handler into fail_run and persist a
-    # successful run as FAILED.
-    task = tasks.get(workspace, run.task_id)
-    if task is None:
-        raise ValueError(f"Task not found: {run.task_id}")
-    if task.status != TaskStatus.DONE:
+    # (#1032, a daemon thread) and manual completion both move a task to DONE
+    # while its run is still active, and DONE -> DONE is a rejected transition.
+    # Raising there would send execute_agent's handler into fail_run and
+    # persist a successful run as FAILED.
+    #
+    # Handled by catching rather than pre-checking: a read-then-call guard is
+    # check-then-act, and update_status does its own fresh read + compare-and-
+    # set, so a DONE landing in between would still raise. Re-reading in the
+    # handler is the only way to tell "someone else already finished it"
+    # (success) from a genuinely illegal transition (still BACKLOG — raise, and
+    # the run stays RUNNING).
+    try:
         tasks.update_status(
             workspace, run.task_id, TaskStatus.DONE, github_autoclose=github_autoclose
         )
+    except InvalidTransitionError:
+        task = tasks.get(workspace, run.task_id)
+        if task is None or task.status != TaskStatus.DONE:
+            raise
 
     now = _utc_now().isoformat()
 

tests/core/test_orchestration_defects_957.py

@@ -348,6 +348,63 @@ def test_already_done_task_completes_the_run(self, tmp_path):
         assert runtime.get_run(ws, run.id).status == RunStatus.COMPLETED
         assert tasks.get(ws, task.id).status == TaskStatus.DONE
 
+    def test_task_flipping_to_done_mid_call_still_completes_the_run(self, tmp_path):
+        """The race, not just the static case (#957 review round 2).
+
+        A read-then-call guard is check-then-act: `update_status` does its own
+        fresh read + compare-and-set, so a reconciliation DONE landing between
+        our read and that CAS still raises. Simulated here by flipping the row
+        to DONE *inside* the call, exactly as the losing CAS would see it.
+        """
+        from codeframe.core import runtime, tasks
+        from codeframe.core.state_machine import InvalidTransitionError
+        from codeframe.core.runtime import RunStatus
+        from codeframe.core.tasks import TaskStatus
+        from codeframe.core.workspace import create_or_load_workspace
+
+        ws = create_or_load_workspace(tmp_path)
+        task = tasks.create(ws, title="T", description="d")
+        run = runtime.start_task_run(ws, task.id)
+
+        real_update = tasks.update_status
+
+        def racing_update(workspace, task_id, new_status, **kwargs):
+            # Someone else wins the race: the row is DONE before our CAS runs.
+            real_update(workspace, task_id, TaskStatus.DONE)
+            raise InvalidTransitionError(TaskStatus.DONE, TaskStatus.DONE)
+
+        with patch.object(tasks, "update_status", side_effect=racing_update):
+            result = runtime.complete_run(ws, run.id)
+
+        assert result.status == RunStatus.COMPLETED
+        assert runtime.get_run(ws, run.id).status == RunStatus.COMPLETED
+        assert tasks.get(ws, task.id).status == TaskStatus.DONE
+
+    def test_genuinely_illegal_transition_still_raises(self, tmp_path):
+        """Only DONE is forgiven — a task stuck elsewhere must not pass."""
+        from codeframe.core import runtime, tasks
+        from codeframe.core.state_machine import InvalidTransitionError
+        from codeframe.core.runtime import RunStatus
+        from codeframe.core.tasks import TaskStatus
+        from codeframe.core.workspace import create_or_load_workspace
+
+        ws = create_or_load_workspace(tmp_path)
+        task = tasks.create(ws, title="T", description="d")
+        run = runtime.start_task_run(ws, task.id)
+
+        real_update = tasks.update_status
+
+        def racing_backwards(workspace, task_id, new_status, **kwargs):
+            # The row moved somewhere that is NOT the goal state.
+            real_update(workspace, task_id, TaskStatus.BLOCKED)
+            raise InvalidTransitionError(TaskStatus.BLOCKED, TaskStatus.DONE)
+
+        with patch.object(tasks, "update_status", side_effect=racing_backwards):
+            with pytest.raises(InvalidTransitionError):
+                runtime.complete_run(ws, run.id)
+
+        assert runtime.get_run(ws, run.id).status == RunStatus.RUNNING
+
     def test_happy_path_still_completes_both(self, tmp_path):
         from codeframe.core import runtime, tasks
         from codeframe.core.runtime import RunStatus