fix(core): plumb cloud_timeout_minutes and stop wiping manual dependencies (#959) (#1092)

* fix(core): plumb cloud_timeout_minutes and stop wiping manual dependencies (#959)

1. --cloud-timeout was silently ignored. create_batch accepted
   cloud_timeout_minutes and dropped it: it was never stored on BatchRun, and
   execute_batch only receives the batch, so the value could not reach
   _execute_task_subprocess and its default of 30 always won —
   `cf work batch run --engine cloud --cloud-timeout 60` did nothing.

   Now a persisted BatchRun field with its own column (DDL + idempotent
   migration, SCHEMA_VERSION 3 -> 4 so already-stamped workspaces actually
   receive it), forwarded at all seven _execute_task_subprocess call sites
   (serial, parallel, both supervisor retries, resume, and the two auto-strategy
   paths). Persisting it is what makes resume restore the user's value instead
   of falling back to 30.

2. apply_inferred_dependencies wiped hand-curated dependencies. Its docstring
   promised it preserved them, but it passed the inferred list straight to
   update_depends_on, which replaces depends_on wholesale — so under
   --strategy auto a manual edge was silently replaced and the loss persisted
   for every future run.

   It now merges: existing edges keep their order and come first, inferred ones
   are appended, duplicates collapse. An inferred edge that would close a cycle
   is dropped with a warning — merging can create one that neither set had
   alone (manual A->B plus inferred B->A), and the manual edge is explicit user
   intent so the inferred one loses. The cycle check runs against a working
   copy of the whole graph, so it also catches cycles formed across several
   edges applied in the same call.

Note: tests/core/test_dependency_analyzer.py::test_only_updates_tasks_with_
dependencies asserted the OLD behaviour — it deliberately built a cycle and
expected it persisted. Its own comment called the cycle incidental ("we're just
testing apply logic"), and create_execution_plan refuses to schedule a cyclic
graph, so the assertion was pinning a bug. Updated with the reasoning inline
rather than deleted.

Closes #959

* fix(core): blame the edge being added, not any cycle in the graph (#959)

Caught by PR review. The guard ran detect_cycle over the whole workspace graph
after tentatively adding an edge — but update_depends_on performs no cycle
validation, so a workspace can already hold one. With a pre-existing A<->B
cycle, every later inferred edge (C->D, entirely unrelated) came back "cyclic",
was dropped, and the warning named A->B->A as the cause.

Replaced with a targeted reachability check: adding target -> start closes a
cycle exactly when target is already reachable from start. That is immune to
unrelated pre-existing cycles, reports the actual offending path, and is
cheaper than a full detector per candidate edge. The visited set means a
pre-existing cycle elsewhere terminates the walk rather than hanging it.

Verified the new test fails against the whole-graph version before keeping it.

Author: frankbria

codeframe/core/conductor.py

@@ -577,6 +577,9 @@ class BatchRun:
             ``__config_reloads__`` key, which leaked into the batch API and the
             CLI as a bogus "task" (#957).
         config_reloads: ISO timestamps of config reloads observed during the run
+        cloud_timeout_minutes: Sandbox timeout for the cloud engine (1-60).
+            Persisted so `resume` restores the user's value instead of silently
+            falling back to the callee default of 30 (#959).
     """
 
     id: str
@@ -597,6 +600,7 @@ class BatchRun:
     llm_provider: Optional[str] = None
     llm_model: Optional[str] = None
     config_reloads: list[str] = field(default_factory=list)
+    cloud_timeout_minutes: int = 30
 
 
 def create_batch(
@@ -677,6 +681,9 @@ def create_batch(
         isolation=isolation,
         llm_provider=llm_provider,
         llm_model=llm_model,
+        # Persisted (#959): this used to be accepted and dropped, so the
+        # callee default of 30 always won and --cloud-timeout was a no-op.
+        cloud_timeout_minutes=cloud_timeout_minutes,
     )
 
     # Save to database
@@ -896,7 +903,7 @@ def get_batch(workspace: Workspace, batch_id: str) -> Optional[BatchRun]:
             SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
                    on_failure, started_at, completed_at, results, engine,
                    isolation, stall_timeout_s, stall_action, concurrency_by_status,
-                   llm_provider, llm_model, config_reloads
+                   llm_provider, llm_model, config_reloads, cloud_timeout_minutes
             FROM batch_runs
             WHERE workspace_id = ? AND id = ?
             """,
@@ -937,7 +944,7 @@ def list_batches(
                 SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
                        on_failure, started_at, completed_at, results, engine,
                        isolation, stall_timeout_s, stall_action, concurrency_by_status,
-                       llm_provider, llm_model, config_reloads
+                       llm_provider, llm_model, config_reloads, cloud_timeout_minutes
                 FROM batch_runs
                 WHERE workspace_id = ? AND status = ?
                 ORDER BY started_at DESC
@@ -951,7 +958,7 @@ def list_batches(
                 SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
                        on_failure, started_at, completed_at, results, engine,
                        isolation, stall_timeout_s, stall_action, concurrency_by_status,
-                       llm_provider, llm_model, config_reloads
+                       llm_provider, llm_model, config_reloads, cloud_timeout_minutes
                 FROM batch_runs
                 WHERE workspace_id = ?
                 ORDER BY started_at DESC
@@ -1029,7 +1036,7 @@ def find_batch_by_prefix(workspace: Workspace, prefix: str) -> list[BatchRun]:
             SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
                    on_failure, started_at, completed_at, results, engine,
                    isolation, stall_timeout_s, stall_action, concurrency_by_status,
-                   llm_provider, llm_model, config_reloads
+                   llm_provider, llm_model, config_reloads, cloud_timeout_minutes
             FROM batch_runs
             WHERE workspace_id = ? AND id LIKE ? ESCAPE '\\'
             ORDER BY started_at DESC
@@ -1357,6 +1364,7 @@ def _run_serial_resume(
                 stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action,
                 worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
                 llm_provider=batch.llm_provider, llm_model=batch.llm_model,
+                cloud_timeout_minutes=batch.cloud_timeout_minutes,
             )
         finally:
             exec_ctx.cleanup()
@@ -1555,6 +1563,7 @@ def _run_retries(
                     stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action,
                     worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
                     llm_provider=batch.llm_provider, llm_model=batch.llm_model,
+                    cloud_timeout_minutes=batch.cloud_timeout_minutes,
                 )
             finally:
                 exec_ctx.cleanup()
@@ -1801,6 +1810,7 @@ def _execute_serial(
                     stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action,
                     worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
                     llm_provider=batch.llm_provider, llm_model=batch.llm_model,
+                    cloud_timeout_minutes=batch.cloud_timeout_minutes,
                 )
 
                 # If task is BLOCKED, try supervisor resolution
@@ -1814,6 +1824,7 @@ def _execute_serial(
                             stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action,
                             worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
                             llm_provider=batch.llm_provider, llm_model=batch.llm_model,
+                            cloud_timeout_minutes=batch.cloud_timeout_minutes,
                         )
             finally:
                 exec_ctx.cleanup()
@@ -2341,6 +2352,7 @@ def _execute_single_task(
             stall_action=batch.stall_action,
             worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
             llm_provider=batch.llm_provider, llm_model=batch.llm_model,
+            cloud_timeout_minutes=batch.cloud_timeout_minutes,
         )
 
         # If task is BLOCKED, try supervisor resolution
@@ -2356,6 +2368,7 @@ def _execute_single_task(
                     stall_action=batch.stall_action,
                     worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
                     llm_provider=batch.llm_provider, llm_model=batch.llm_model,
+                    cloud_timeout_minutes=batch.cloud_timeout_minutes,
                 )
     finally:
         exec_ctx.cleanup()
@@ -2471,6 +2484,7 @@ def execute_task(task_id: str) -> tuple[str, str]:
                 stall_action=batch.stall_action,
                 worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
                 llm_provider=batch.llm_provider, llm_model=batch.llm_model,
+                cloud_timeout_minutes=batch.cloud_timeout_minutes,
             )
         finally:
             exec_ctx.cleanup()
@@ -2705,8 +2719,8 @@ def _save_batch(
                 (id, workspace_id, task_ids, status, strategy, max_parallel, on_failure,
                  started_at, completed_at, results, engine, isolation,
                  stall_timeout_s, stall_action, concurrency_by_status,
-                 llm_provider, llm_model, config_reloads)
-                VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
+                 llm_provider, llm_model, config_reloads, cloud_timeout_minutes)
+                VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
                 """,
                 (
                     batch.id,
@@ -2727,6 +2741,7 @@ def _save_batch(
                     batch.llm_provider,
                     batch.llm_model,
                     config_reloads_json,
+                    batch.cloud_timeout_minutes,
                 ),
             )
             conn.commit()
@@ -2774,4 +2789,7 @@ def _row_to_batch(row: tuple) -> BatchRun:
         llm_provider=row[15] if len(row) > 15 else None,
         llm_model=row[16] if len(row) > 16 else None,
         config_reloads=config_reloads,
+        cloud_timeout_minutes=(
+            row[18] if len(row) > 18 and row[18] is not None else 30
+        ),
     )

codeframe/core/dependency_analyzer.py

@@ -11,6 +11,7 @@
 """
 
 import json
+import logging
 import re
 from typing import Optional
 
@@ -19,6 +20,8 @@
 from codeframe.core.tasks import Task
 from codeframe.adapters.llm.base import Purpose
 
+logger = logging.getLogger(__name__)
+
 
 DEPENDENCY_ANALYSIS_SYSTEM_PROMPT = """You are a software project task analyzer. Your job is to analyze a list of development tasks and identify dependencies between them.
 
@@ -198,19 +201,86 @@ def apply_inferred_dependencies(
     workspace: Workspace,
     dependencies: dict[str, list[str]],
 ) -> None:
-    """Apply inferred dependencies to tasks in the workspace.
+    """Merge inferred dependencies into each task's existing ``depends_on``.
+
+    Inferred edges are **added to** the task's current dependencies, never
+    substituted for them. ``update_depends_on`` replaces the list wholesale, so
+    passing the inferred list straight through silently discarded a
+    hand-curated dependency — and persisted the loss for every future run
+    (#959). Existing edges keep their order and come first.
 
-    Updates each task's depends_on field with the inferred dependencies.
-    Only updates tasks that have non-empty dependency lists to preserve
-    any existing manual/explicit dependencies.
+    An inferred edge that would close a cycle is dropped with a warning: the
+    merge can create one that neither set had alone (manual ``A -> B`` plus
+    inferred ``B -> A``). The manual edge is explicit user intent, so the
+    inferred one loses.
 
     Args:
         workspace: Workspace containing the tasks
         dependencies: Dict mapping task_id -> list of dependency task_ids
     """
-    for task_id, deps in dependencies.items():
-        # Only update when there are inferred dependencies to apply
-        # Empty list means no dependencies found - don't clear existing ones
-        if deps:
-            task_module.update_depends_on(workspace, task_id, deps)
+    # Working copy of the whole workspace graph, so a cycle check sees edges
+    # added earlier in this same call.
+    graph: dict[str, list[str]] = {
+        t.id: list(t.depends_on or []) for t in task_module.list_tasks(workspace, limit=None)
+    }
+
+    for task_id, inferred in dependencies.items():
+        if not inferred:
+            # No inference for this task — leave whatever is already there.
+            continue
+        existing = graph.get(task_id)
+        if existing is None:
+            continue  # Task vanished between analysis and apply.
+
+        merged = list(existing)
+        for dep in inferred:
+            if dep == task_id or dep in merged or dep not in graph:
+                continue
+            path = _path_to(graph, dep, task_id)
+            if path is not None:
+                logger.warning(
+                    "Dropping inferred dependency %s -> %s: it would create a "
+                    "cycle (%s). Existing dependencies are kept.",
+                    task_id,
+                    dep,
+                    " -> ".join([task_id, *path]),
+                )
+                continue
+            merged.append(dep)
+            graph[task_id] = merged
+
+        if merged != existing:
+            task_module.update_depends_on(workspace, task_id, merged)
+
+
+def _path_to(
+    graph: dict[str, list[str]], start: str, target: str
+) -> Optional[list[str]]:
+    """Return a dependency path from ``start`` to ``target``, or ``None``.
+
+    Adding the edge ``target -> start`` closes a cycle exactly when ``target``
+    is already reachable from ``start``. Asking that directly — rather than
+    adding the edge and running a whole-graph cycle detector — keeps the check
+    honest when the graph *already* contains an unrelated cycle: a global
+    detector would return that pre-existing cycle, so every inferred edge would
+    be dropped and the warning would blame the wrong pair. ``update_depends_on``
+    performs no cycle validation, so a pre-existing cycle is reachable.
+
+    The visited set also means a pre-existing cycle anywhere in the graph
+    terminates this walk instead of hanging it.
+    """
+    if start == target:
+        return [start]
+    stack: list[tuple[str, list[str]]] = [(start, [start])]
+    seen = {start}
+    while stack:
+        node, path = stack.pop()
+        for nxt in graph.get(node, []):
+            if nxt == target:
+                return path + [nxt]
+            if nxt in seen:
+                continue
+            seen.add(nxt)
+            stack.append((nxt, path + [nxt]))
+    return None
 

codeframe/core/workspace.py

@@ -35,7 +35,8 @@ def _utc_now() -> datetime:
 # table/column/index so existing workspaces re-enter the (idempotent)
 # migration path exactly once. Gates #733: steady-state loads skip all DDL.
 # 3: batch_runs.config_reloads (#957).
-SCHEMA_VERSION = 3
+# 4: batch_runs.cloud_timeout_minutes (#959).
+SCHEMA_VERSION = 4
 
 # Per-workspace config file written by the Settings page (issue #556).
 # Owned by the UI layer today; kept here so a future core consumer can
@@ -316,6 +317,7 @@ def _init_database(db_path: Path) -> None:
             llm_provider TEXT,
             llm_model TEXT,
             config_reloads TEXT,
+            cloud_timeout_minutes INTEGER NOT NULL DEFAULT 30,
             FOREIGN KEY (workspace_id) REFERENCES workspace(id),
             CHECK (status IN ('PENDING', 'RUNNING', 'COMPLETED', 'PARTIAL', 'FAILED', 'CANCELLED'))
         )
@@ -580,6 +582,7 @@ def _ensure_schema_upgrades(db_path: Path) -> None:
                 llm_provider TEXT,
                 llm_model TEXT,
                 config_reloads TEXT,
+                cloud_timeout_minutes INTEGER NOT NULL DEFAULT 30,
                 FOREIGN KEY (workspace_id) REFERENCES workspace(id),
                 CHECK (status IN ('PENDING', 'RUNNING', 'COMPLETED', 'PARTIAL', 'FAILED', 'CANCELLED'))
             )
@@ -605,6 +608,8 @@ def _ensure_schema_upgrades(db_path: Path) -> None:
             ("llm_model", "ALTER TABLE batch_runs ADD COLUMN llm_model TEXT"),
             # #957: config-reload bookkeeping moved off the results JSON blob.
             ("config_reloads", "ALTER TABLE batch_runs ADD COLUMN config_reloads TEXT"),
+            # #959: the user's --cloud-timeout must survive a resume.
+            ("cloud_timeout_minutes", "ALTER TABLE batch_runs ADD COLUMN cloud_timeout_minutes INTEGER NOT NULL DEFAULT 30"),
         )
         for column, ddl in batch_migrations:
             if column not in batch_columns: