Skip to content

Commit e75ad8f

Browse files
committed
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
1 parent 597b078 commit e75ad8f

5 files changed

Lines changed: 346 additions & 19 deletions

File tree

codeframe/core/conductor.py

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,9 @@ class BatchRun:
577577
``__config_reloads__`` key, which leaked into the batch API and the
578578
CLI as a bogus "task" (#957).
579579
config_reloads: ISO timestamps of config reloads observed during the run
580+
cloud_timeout_minutes: Sandbox timeout for the cloud engine (1-60).
581+
Persisted so `resume` restores the user's value instead of silently
582+
falling back to the callee default of 30 (#959).
580583
"""
581584

582585
id: str
@@ -597,6 +600,7 @@ class BatchRun:
597600
llm_provider: Optional[str] = None
598601
llm_model: Optional[str] = None
599602
config_reloads: list[str] = field(default_factory=list)
603+
cloud_timeout_minutes: int = 30
600604

601605

602606
def create_batch(
@@ -677,6 +681,9 @@ def create_batch(
677681
isolation=isolation,
678682
llm_provider=llm_provider,
679683
llm_model=llm_model,
684+
# Persisted (#959): this used to be accepted and dropped, so the
685+
# callee default of 30 always won and --cloud-timeout was a no-op.
686+
cloud_timeout_minutes=cloud_timeout_minutes,
680687
)
681688

682689
# Save to database
@@ -896,7 +903,7 @@ def get_batch(workspace: Workspace, batch_id: str) -> Optional[BatchRun]:
896903
SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
897904
on_failure, started_at, completed_at, results, engine,
898905
isolation, stall_timeout_s, stall_action, concurrency_by_status,
899-
llm_provider, llm_model, config_reloads
906+
llm_provider, llm_model, config_reloads, cloud_timeout_minutes
900907
FROM batch_runs
901908
WHERE workspace_id = ? AND id = ?
902909
""",
@@ -937,7 +944,7 @@ def list_batches(
937944
SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
938945
on_failure, started_at, completed_at, results, engine,
939946
isolation, stall_timeout_s, stall_action, concurrency_by_status,
940-
llm_provider, llm_model, config_reloads
947+
llm_provider, llm_model, config_reloads, cloud_timeout_minutes
941948
FROM batch_runs
942949
WHERE workspace_id = ? AND status = ?
943950
ORDER BY started_at DESC
@@ -951,7 +958,7 @@ def list_batches(
951958
SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
952959
on_failure, started_at, completed_at, results, engine,
953960
isolation, stall_timeout_s, stall_action, concurrency_by_status,
954-
llm_provider, llm_model, config_reloads
961+
llm_provider, llm_model, config_reloads, cloud_timeout_minutes
955962
FROM batch_runs
956963
WHERE workspace_id = ?
957964
ORDER BY started_at DESC
@@ -1029,7 +1036,7 @@ def find_batch_by_prefix(workspace: Workspace, prefix: str) -> list[BatchRun]:
10291036
SELECT id, workspace_id, task_ids, status, strategy, max_parallel,
10301037
on_failure, started_at, completed_at, results, engine,
10311038
isolation, stall_timeout_s, stall_action, concurrency_by_status,
1032-
llm_provider, llm_model, config_reloads
1039+
llm_provider, llm_model, config_reloads, cloud_timeout_minutes
10331040
FROM batch_runs
10341041
WHERE workspace_id = ? AND id LIKE ? ESCAPE '\\'
10351042
ORDER BY started_at DESC
@@ -1357,6 +1364,7 @@ def _run_serial_resume(
13571364
stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action,
13581365
worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
13591366
llm_provider=batch.llm_provider, llm_model=batch.llm_model,
1367+
cloud_timeout_minutes=batch.cloud_timeout_minutes,
13601368
)
13611369
finally:
13621370
exec_ctx.cleanup()
@@ -1555,6 +1563,7 @@ def _run_retries(
15551563
stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action,
15561564
worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
15571565
llm_provider=batch.llm_provider, llm_model=batch.llm_model,
1566+
cloud_timeout_minutes=batch.cloud_timeout_minutes,
15581567
)
15591568
finally:
15601569
exec_ctx.cleanup()
@@ -1801,6 +1810,7 @@ def _execute_serial(
18011810
stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action,
18021811
worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
18031812
llm_provider=batch.llm_provider, llm_model=batch.llm_model,
1813+
cloud_timeout_minutes=batch.cloud_timeout_minutes,
18041814
)
18051815

18061816
# If task is BLOCKED, try supervisor resolution
@@ -1814,6 +1824,7 @@ def _execute_serial(
18141824
stall_timeout_s=batch.stall_timeout_s, stall_action=batch.stall_action,
18151825
worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
18161826
llm_provider=batch.llm_provider, llm_model=batch.llm_model,
1827+
cloud_timeout_minutes=batch.cloud_timeout_minutes,
18171828
)
18181829
finally:
18191830
exec_ctx.cleanup()
@@ -2341,6 +2352,7 @@ def _execute_single_task(
23412352
stall_action=batch.stall_action,
23422353
worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
23432354
llm_provider=batch.llm_provider, llm_model=batch.llm_model,
2355+
cloud_timeout_minutes=batch.cloud_timeout_minutes,
23442356
)
23452357

23462358
# If task is BLOCKED, try supervisor resolution
@@ -2356,6 +2368,7 @@ def _execute_single_task(
23562368
stall_action=batch.stall_action,
23572369
worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
23582370
llm_provider=batch.llm_provider, llm_model=batch.llm_model,
2371+
cloud_timeout_minutes=batch.cloud_timeout_minutes,
23592372
)
23602373
finally:
23612374
exec_ctx.cleanup()
@@ -2471,6 +2484,7 @@ def execute_task(task_id: str) -> tuple[str, str]:
24712484
stall_action=batch.stall_action,
24722485
worktree_path=exec_ctx.workspace_path if exec_ctx.workspace_path != workspace.repo_path else None,
24732486
llm_provider=batch.llm_provider, llm_model=batch.llm_model,
2487+
cloud_timeout_minutes=batch.cloud_timeout_minutes,
24742488
)
24752489
finally:
24762490
exec_ctx.cleanup()
@@ -2705,8 +2719,8 @@ def _save_batch(
27052719
(id, workspace_id, task_ids, status, strategy, max_parallel, on_failure,
27062720
started_at, completed_at, results, engine, isolation,
27072721
stall_timeout_s, stall_action, concurrency_by_status,
2708-
llm_provider, llm_model, config_reloads)
2709-
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
2722+
llm_provider, llm_model, config_reloads, cloud_timeout_minutes)
2723+
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
27102724
""",
27112725
(
27122726
batch.id,
@@ -2727,6 +2741,7 @@ def _save_batch(
27272741
batch.llm_provider,
27282742
batch.llm_model,
27292743
config_reloads_json,
2744+
batch.cloud_timeout_minutes,
27302745
),
27312746
)
27322747
conn.commit()
@@ -2774,4 +2789,7 @@ def _row_to_batch(row: tuple) -> BatchRun:
27742789
llm_provider=row[15] if len(row) > 15 else None,
27752790
llm_model=row[16] if len(row) > 16 else None,
27762791
config_reloads=config_reloads,
2792+
cloud_timeout_minutes=(
2793+
row[18] if len(row) > 18 and row[18] is not None else 30
2794+
),
27772795
)

codeframe/core/dependency_analyzer.py

Lines changed: 50 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
"""
1212

1313
import json
14+
import logging
1415
import re
1516
from typing import Optional
1617

@@ -19,6 +20,8 @@
1920
from codeframe.core.tasks import Task
2021
from codeframe.adapters.llm.base import Purpose
2122

23+
logger = logging.getLogger(__name__)
24+
2225

2326
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.
2427
@@ -198,19 +201,57 @@ def apply_inferred_dependencies(
198201
workspace: Workspace,
199202
dependencies: dict[str, list[str]],
200203
) -> None:
201-
"""Apply inferred dependencies to tasks in the workspace.
204+
"""Merge inferred dependencies into each task's existing ``depends_on``.
205+
206+
Inferred edges are **added to** the task's current dependencies, never
207+
substituted for them. ``update_depends_on`` replaces the list wholesale, so
208+
passing the inferred list straight through silently discarded a
209+
hand-curated dependency — and persisted the loss for every future run
210+
(#959). Existing edges keep their order and come first.
202211
203-
Updates each task's depends_on field with the inferred dependencies.
204-
Only updates tasks that have non-empty dependency lists to preserve
205-
any existing manual/explicit dependencies.
212+
An inferred edge that would close a cycle is dropped with a warning: the
213+
merge can create one that neither set had alone (manual ``A -> B`` plus
214+
inferred ``B -> A``). The manual edge is explicit user intent, so the
215+
inferred one loses.
206216
207217
Args:
208218
workspace: Workspace containing the tasks
209219
dependencies: Dict mapping task_id -> list of dependency task_ids
210220
"""
211-
for task_id, deps in dependencies.items():
212-
# Only update when there are inferred dependencies to apply
213-
# Empty list means no dependencies found - don't clear existing ones
214-
if deps:
215-
task_module.update_depends_on(workspace, task_id, deps)
221+
from codeframe.core.dependency_graph import detect_cycle
222+
223+
# Working copy of the whole workspace graph, so a cycle check sees edges
224+
# added earlier in this same call.
225+
graph: dict[str, list[str]] = {
226+
t.id: list(t.depends_on or []) for t in task_module.list_tasks(workspace, limit=None)
227+
}
228+
229+
for task_id, inferred in dependencies.items():
230+
if not inferred:
231+
# No inference for this task — leave whatever is already there.
232+
continue
233+
existing = graph.get(task_id)
234+
if existing is None:
235+
continue # Task vanished between analysis and apply.
236+
237+
merged = list(existing)
238+
for dep in inferred:
239+
if dep == task_id or dep in merged or dep not in graph:
240+
continue
241+
merged.append(dep)
242+
graph[task_id] = merged
243+
cycle = detect_cycle(graph)
244+
if cycle:
245+
merged.pop()
246+
graph[task_id] = merged
247+
logger.warning(
248+
"Dropping inferred dependency %s -> %s: it would create a "
249+
"cycle (%s). Existing dependencies are kept.",
250+
task_id,
251+
dep,
252+
" -> ".join(cycle),
253+
)
254+
255+
if merged != existing:
256+
task_module.update_depends_on(workspace, task_id, merged)
216257

codeframe/core/workspace.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@ def _utc_now() -> datetime:
3535
# table/column/index so existing workspaces re-enter the (idempotent)
3636
# migration path exactly once. Gates #733: steady-state loads skip all DDL.
3737
# 3: batch_runs.config_reloads (#957).
38-
SCHEMA_VERSION = 3
38+
# 4: batch_runs.cloud_timeout_minutes (#959).
39+
SCHEMA_VERSION = 4
3940

4041
# Per-workspace config file written by the Settings page (issue #556).
4142
# 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:
316317
llm_provider TEXT,
317318
llm_model TEXT,
318319
config_reloads TEXT,
320+
cloud_timeout_minutes INTEGER NOT NULL DEFAULT 30,
319321
FOREIGN KEY (workspace_id) REFERENCES workspace(id),
320322
CHECK (status IN ('PENDING', 'RUNNING', 'COMPLETED', 'PARTIAL', 'FAILED', 'CANCELLED'))
321323
)
@@ -580,6 +582,7 @@ def _ensure_schema_upgrades(db_path: Path) -> None:
580582
llm_provider TEXT,
581583
llm_model TEXT,
582584
config_reloads TEXT,
585+
cloud_timeout_minutes INTEGER NOT NULL DEFAULT 30,
583586
FOREIGN KEY (workspace_id) REFERENCES workspace(id),
584587
CHECK (status IN ('PENDING', 'RUNNING', 'COMPLETED', 'PARTIAL', 'FAILED', 'CANCELLED'))
585588
)
@@ -605,6 +608,8 @@ def _ensure_schema_upgrades(db_path: Path) -> None:
605608
("llm_model", "ALTER TABLE batch_runs ADD COLUMN llm_model TEXT"),
606609
# #957: config-reload bookkeeping moved off the results JSON blob.
607610
("config_reloads", "ALTER TABLE batch_runs ADD COLUMN config_reloads TEXT"),
611+
# #959: the user's --cloud-timeout must survive a resume.
612+
("cloud_timeout_minutes", "ALTER TABLE batch_runs ADD COLUMN cloud_timeout_minutes INTEGER NOT NULL DEFAULT 30"),
608613
)
609614
for column, ddl in batch_migrations:
610615
if column not in batch_columns:

0 commit comments

Comments
 (0)