From a43d6d1b06a7546b624401f42c5b65722df31936 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Wed, 9 Sep 2026 13:03:45 -0400 Subject: [PATCH 01/30] feat: add DataNode graph primitive for first-class data loading in workflows Add DataItem and DataNode as workflow graph primitives, enabling data-driven fan-out where a subgraph is executed per data item with Semaphore-throttled concurrency and per-item fault isolation. Touches 8 source files across 3 architectural layers: - Models: DataItem + DataNode with exactly-one-source validation - Executor: _execute_data with task_ref/inline/source_path resolution - Validation: structural + reachability checks for both functions - Skill export: DataNode rendering + subgraph skip-set + trailing else - Inner loop: cached DataNode detection, executor delegation - Compose: CAN_ITERATE capability inference - MAP-Elites: appended has_data_node feature axis (9-tuple) - Diversity: generalized to len(key) instead of hardcoded range(4) Closes #1482 Co-Authored-By: Claude Opus 4.6 --- factory/compose.py | 4 + factory/inner_loop.py | 57 ++- factory/outer_loop/population.py | 11 +- factory/outer_loop/similarity.py | 5 + factory/workflow/executor.py | 142 +++++++ factory/workflow/primitives.py | 48 ++- factory/workflow/skill_export.py | 57 +++ factory/workflow/validation.py | 30 +- tests/test_data_node.py | 479 +++++++++++++++++++++++ tests/test_outer_loop/test_population.py | 2 +- tests/test_outer_loop/test_similarity.py | 8 +- 11 files changed, 831 insertions(+), 12 deletions(-) create mode 100644 tests/test_data_node.py diff --git a/factory/compose.py b/factory/compose.py index 93317c265..a51e1100c 100644 --- a/factory/compose.py +++ b/factory/compose.py @@ -86,6 +86,7 @@ def from_workflow(cls, workflow: Any) -> ModeCapabilities: from factory.workflow.primitives import ( AgentNode, AgentRole, + DataNode, FnNode, ForkNode, GateNode, @@ -124,6 +125,9 @@ def from_workflow(cls, workflow: Any) -> ModeCapabilities: elif isinstance(node, ForkNode): caps.add(Capability.HAS_PARALLELISM) + elif isinstance(node, DataNode): + caps.add(Capability.CAN_ITERATE) + elif isinstance(node, FnNode): caps.add(Capability.CAN_RUN_SUBPROCESS) diff --git a/factory/inner_loop.py b/factory/inner_loop.py index 81ec6faad..b24adfc96 100644 --- a/factory/inner_loop.py +++ b/factory/inner_loop.py @@ -27,7 +27,7 @@ from typing import Any, Protocol, runtime_checkable from factory.cycle_analyzer import CycleAnalyzer, CycleRecord -from factory.workflow.primitives import Workflow +from factory.workflow.primitives import DataNode, Workflow @dataclass @@ -135,6 +135,7 @@ def __init__( self.instance = instance self._step_count = 0 self._history: list[CycleRecord] = [] + self._has_data_node: bool | None = None self._validate_frozen_nodes() # When task is set, derive flat fields from it for backward compat @@ -256,6 +257,14 @@ def _step_subprocess(self, directives: dict[str, Any] | None = None) -> CycleRec self._history.append(record) return record + def _workflow_has_data_node(self) -> bool: + if self._has_data_node is None: + self._has_data_node = ( + self.workflow is not None + and any(isinstance(n, DataNode) for n in self.workflow.nodes.values()) + ) + return self._has_data_node + def _step_with_task(self, directives: dict[str, Any] | None = None) -> CycleRecord: """Task-driven step: setup → WorkflowExecutor → verify per instance.""" assert self.task is not None @@ -269,6 +278,9 @@ def _step_with_task(self, directives: dict[str, Any] | None = None) -> CycleReco if directives: self._write_directives(directives) + if self._workflow_has_data_node(): + return self._step_with_data_node(directives) + t0 = time.monotonic() workflow = self.workflow @@ -368,6 +380,49 @@ def _step_with_task(self, directives: dict[str, Any] | None = None) -> CycleReco self._history.append(record) return record + def _step_with_data_node(self, directives: dict[str, Any] | None = None) -> CycleRecord: + """Delegate to the executor when the workflow contains a DataNode.""" + import asyncio + + from factory.workflow.executor import WorkflowExecutor + + t0 = time.monotonic() + + executor = WorkflowExecutor( + self.workflow, + self.project_dir, + ) + exec_result = asyncio.run(executor.execute()) + + duration_s = time.monotonic() - t0 + score = 1.0 if exec_result.success else 0.0 + + record = CycleRecord( + cycle_number=self._step_count + 1, + mode=self.mode, + started_at=None, + ended_at=None, + duration_s=duration_s, + score_start=None, + score_end=score, + score_delta=None, + ) + record.frozen_nodes = sorted(self.frozen_nodes) + record.mutable_node_ids = sorted(self.mutable_nodes()) + + self._write_cycle_summary( + returncode=0 if exec_result.success else 1, + event_offset=0, + duration_ms=int(duration_s * 1000), + builder_committed=False, + experiments=0, + test_score=score, + ) + + self._step_count += 1 + self._history.append(record) + return record + def collect(self) -> CycleRecord: """Collect results without running a cycle. Useful after manual runs.""" return self._collect_results() diff --git a/factory/outer_loop/population.py b/factory/outer_loop/population.py index 4022c9d98..ee7c02c7d 100644 --- a/factory/outer_loop/population.py +++ b/factory/outer_loop/population.py @@ -196,14 +196,17 @@ def diversity_metric(self) -> float: """Fraction of occupied cells relative to a reasonable grid size estimate. Returns 0.0 for empty archive, approaches 1.0 as more cells are filled. + Uses the first 5 structural axes (depth, fork_degree, agent_count, + gate_count, has_data_node) for diversity estimation. """ if not self._grid: return 0.0 - unique_per_axis: list[set[int]] = [set() for _ in range(4)] + sample_key = next(iter(self._grid)) + n_axes = min(len(sample_key), 5) + unique_per_axis: list[set[int]] = [set() for _ in range(n_axes)] for key in self._grid: - for i, v in enumerate(key): - if i < 4: - unique_per_axis[i].add(v) + for i in range(n_axes): + unique_per_axis[i].add(key[i]) total_possible = 1 for s in unique_per_axis: total_possible *= max(len(s), 1) diff --git a/factory/outer_loop/similarity.py b/factory/outer_loop/similarity.py index 208a87a45..1d15a55d5 100644 --- a/factory/outer_loop/similarity.py +++ b/factory/outer_loop/similarity.py @@ -128,12 +128,17 @@ def _hash_bucket(sig: str, buckets: int = 8) -> int: f"{k}={v}" for k, v in sorted(workflow.knob_values.items()) ) if workflow.knob_values else "" + has_data_node = int(any( + type(n).__name__ == "DataNode" for n in workflow.nodes.values() + )) + return ( depth, fork_degree, agent_count, gate_count, _hash_bucket(edge_sig), _hash_bucket(param_sig, 16), _hash_bucket(prompt_sig, 32), _hash_bucket(knob_sig, 16), + has_data_node, ) diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index 1ff048a2a..a413fba1b 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -25,6 +25,8 @@ from factory.workflow.primitives import ( AgentConfig, AgentNode, + DataItem, + DataNode, Edge, FnNode, ForkNode, @@ -230,6 +232,10 @@ async def _execute_from(self, node_id: str) -> None: await self._execute_gate(node) return + if isinstance(node, DataNode): + await self._execute_data(node_id, node) + return + await self._execute_action_node(node) async def _execute_action_node(self, node: NodeType) -> None: @@ -628,6 +634,142 @@ async def throttled_branch(idx: int) -> dict[str, Any]: if next_id: await self._execute_from(next_id) + async def _execute_data(self, node_id: str, node: DataNode) -> None: + """Execute a DataNode: resolve items, run subgraph per item with fault isolation.""" + import random as _random + + self.result.nodes_executed += 1 + + self._emit( + "node.started", + NodeStarted( + workflow_name=self.workflow.name, + run_id=self.run_id, + node_id=node_id, + node_type="DataNode", + ), + ) + + start = time.monotonic() + + # Resolve data items from exactly one source + items: list[DataItem] = [] + if node.inline_items: + items = list(node.inline_items) + elif node.task_ref: + from factory.task import TaskRef + task_ref = TaskRef(ref=node.task_ref) + task = task_ref.resolve() + for inst in task.instances(): + task.setup(inst, self.project_path) + prompt_text = task.prompt(inst) + items.append(DataItem( + id=inst.id, + path=str(inst.path) if inst.path else None, + metadata=inst.metadata, + prompt=prompt_text, + )) + elif node.source_path: + from pathlib import Path as _Path + src = _Path(node.source_path) + if not src.is_absolute(): + src = self.project_path / src + if node.source_format == "directory" and src.is_dir(): + for child in sorted(src.iterdir()): + if child.is_dir(): + items.append(DataItem(id=child.name, path=str(child))) + elif node.source_format == "jsonl" and src.is_file(): + for idx, line in enumerate(src.read_text().splitlines()): + if line.strip(): + items.append(DataItem( + id=str(idx), + metadata=json.loads(line), + )) + elif node.source_format == "csv" and src.is_file(): + import csv + with src.open(newline="") as f: + reader = csv.DictReader(f) + for idx, row in enumerate(reader): + items.append(DataItem(id=str(idx), metadata=dict(row))) + + # Apply split/shuffle/limit filters + if node.split != "all": + items = [it for it in items if it.metadata.get("split") == node.split] + if node.shuffle: + _random.shuffle(items) + if node.limit is not None and node.limit > 0: + items = items[:node.limit] + + if len(items) > node.max_items: + raise ValueError( + f"DataNode '{node_id}' resolved {len(items)} items, " + f"exceeding max_items={node.max_items}" + ) + + # Collect subgraph and run per item with Semaphore-throttled concurrency + subgraph_ids = _collect_subgraph_nodes( + self.workflow, node.subgraph_entry, node.subgraph_exit, + ) + sub_workflow = self.workflow.subgraph( + subgraph_ids, + name=f"{self.workflow.name}__data_item", + start_node=node.subgraph_entry, + ) + + item_results: list[dict[str, Any]] = [] + sem = asyncio.Semaphore(node.parallelism) + + async def run_item(item: DataItem) -> dict[str, Any]: + async with sem: + try: + item_executor = WorkflowExecutor( + sub_workflow.model_copy(deep=True), + self.project_path, + agent_pool=self.agent_pool, + dry_run=self.dry_run, + initial_context=item.prompt, + ) + item_result = await item_executor.execute() + return { + "item_id": item.id, + "success": item_result.success, + "score": 1.0 if item_result.success else 0.0, + "nodes_executed": item_result.nodes_executed, + "node_outputs": item_result.node_outputs, + } + except Exception as exc: + log.warning("data_item_failed", item_id=item.id, error=str(exc)) + return { + "item_id": item.id, + "success": False, + "score": 0.0, + "error": str(exc), + } + + tasks = [run_item(item) for item in items] + results = await asyncio.gather(*tasks) + item_results = list(results) + + elapsed = (time.monotonic() - start) * 1000 + self.result.node_outputs[node_id] = json.dumps(item_results) + self.completed_files |= node.writes + + self._emit( + "node.completed", + NodeCompleted( + workflow_name=self.workflow.name, + run_id=self.run_id, + node_id=node_id, + node_type="DataNode", + files_written=sorted(node.writes), + duration_ms=elapsed, + ), + ) + + next_id = self._next_unconditional(node_id) + if next_id: + await self._execute_from(next_id) + async def _execute_selection(self, node: SelectionNode) -> None: """Compare parallel experiment results and select the best.""" import subprocess as sp diff --git a/factory/workflow/primitives.py b/factory/workflow/primitives.py index 843bca741..365c32202 100644 --- a/factory/workflow/primitives.py +++ b/factory/workflow/primitives.py @@ -191,6 +191,50 @@ class SubgraphForkNode(Node): worktree_isolated: bool = True +class DataItem(BaseModel): + """Standardized data payload for per-item workflow iteration.""" + + model_config = ConfigDict(strict=True, extra="forbid") + + id: str + path: str | None = None + metadata: dict[str, Any] = Field(default_factory=dict) + prompt: str = "" + + +class DataNode(Node): + """Node that loads data items and drives per-item execution of a subgraph.""" + + model_config = ConfigDict(strict=True, extra="forbid") + + task_ref: str | None = None + source_path: str | None = None + source_format: Literal["directory", "jsonl", "csv"] | None = None + inline_items: list[DataItem] = Field(default_factory=list) + subgraph_entry: str + subgraph_exit: str + parallelism: int = 3 + split: Literal["train", "val", "test", "all"] = "all" + shuffle: bool = False + limit: int | None = None + max_items: int = 500 + + @model_validator(mode="after") + def _validate_source(self) -> DataNode: + sources = [ + self.task_ref is not None, + self.source_path is not None, + bool(self.inline_items), + ] + if sum(sources) != 1: + raise ValueError( + "Exactly one of task_ref, source_path, or inline_items must be set" + ) + if self.source_path is not None and self.source_format is None: + raise ValueError("source_path requires source_format to be set") + return self + + class SelectionNode(Node): """Compare N completed experiment branches and select the best.""" @@ -257,7 +301,8 @@ class Edge(BaseModel): NodeType = ( - AgentNode | FnNode | GateNode | ForkNode | JoinNode | SubgraphForkNode | SelectionNode | Study | LLMNode + AgentNode | FnNode | GateNode | ForkNode | JoinNode | SubgraphForkNode + | SelectionNode | Study | LLMNode | DataNode ) @@ -353,6 +398,7 @@ def from_dict(cls, data: dict[str, Any]) -> Workflow: "SelectionNode": SelectionNode, "Study": Study, "LLMNode": LLMNode, + "DataNode": DataNode, } _SET_FIELDS = {"reads", "writes"} diff --git a/factory/workflow/skill_export.py b/factory/workflow/skill_export.py index 6a980c49f..bad6eca7b 100644 --- a/factory/workflow/skill_export.py +++ b/factory/workflow/skill_export.py @@ -20,6 +20,7 @@ from factory.workflow.primitives import ( AgentNode, DEFAULT_AGENT_POOL, + DataNode, Edge, FnNode, ForkNode, @@ -629,6 +630,43 @@ def _selection_to_instruction(node: SelectionNode, workflow: Workflow) -> str: return "\n".join(lines) +def _data_to_instruction(node: DataNode, workflow: Workflow) -> str: + """Convert a DataNode to data iteration instructions.""" + out_edges = _outgoing_edges(workflow, node.id) + edges_str = _format_edges(out_edges) + + source_desc = "inline items" + if node.task_ref: + source_desc = f"task_ref `{node.task_ref}`" + elif node.source_path: + source_desc = f"source_path `{node.source_path}` (format: {node.source_format})" + + annotations = [ + f"", + f"", + f"", + ] + + lines = [ + *annotations, + "", + f"Load data items from {source_desc} and iterate the subgraph " + f"(`{node.subgraph_entry}` → `{node.subgraph_exit}`) once per item.", + "", + f"- **Parallelism:** {node.parallelism} concurrent items", + f"- **Split:** {node.split}", + ] + if node.shuffle: + lines.append("- **Shuffle:** yes") + if node.limit is not None: + lines.append(f"- **Limit:** {node.limit} items") + lines.append(f"- **Max items (safety ceiling):** {node.max_items}") + lines.append("") + lines.append("Per-item fault isolation: a failing item scores 0.0 but does not halt the iteration.") + + return "\n".join(lines) + + # ── frontmatter builder ──────────────────────────────────────── @@ -692,6 +730,12 @@ def workflow_to_skill_md(workflow: Workflow) -> str: elif isinstance(node, SubgraphForkNode): from factory.workflow.executor import _collect_subgraph_nodes + subgraph_nodes |= _collect_subgraph_nodes( + workflow, node.subgraph_entry, node.subgraph_exit + ) + elif isinstance(node, DataNode): + from factory.workflow.executor import _collect_subgraph_nodes + subgraph_nodes |= _collect_subgraph_nodes( workflow, node.subgraph_entry, node.subgraph_exit ) @@ -753,11 +797,24 @@ def workflow_to_skill_md(workflow: Workflow) -> str: sections.append(_llm_to_instruction(node, workflow)) phase_num += 1 + elif isinstance(node, DataNode): + node_title = nid.replace("_", " ").title() + sections.append(f"## Phase {phase_num}: {node_title} (Data Iteration)\n") + sections.append(_data_to_instruction(node, workflow)) + phase_num += 1 + elif isinstance(node, FnNode): node_title = nid.replace("_", " ").title() sections.append(f"## Step: {node_title}\n") sections.append(_fn_to_instruction(node, workflow)) + else: + log.warning( + "skill_export.unmatched_node_type", + node_id=nid, + node_type=type(node).__name__, + ) + body = "\n\n".join(sections) result = f"{frontmatter}\n\n{header}\n\n{body}\n" diff --git a/factory/workflow/validation.py b/factory/workflow/validation.py index a92218916..ae2ae606c 100644 --- a/factory/workflow/validation.py +++ b/factory/workflow/validation.py @@ -29,6 +29,7 @@ def _validate_reachability( # Add implicit edges for fork/join semantics. # ForkNode.targets are reached implicitly (not via explicit edges). # JoinNode.sources flow into the join implicitly. + # DataNode.subgraph_entry/exit are reached implicitly. nodes = workflow.nodes for nid, node in nodes.items(): if type(node).__name__ == "ForkNode": @@ -39,6 +40,13 @@ def _validate_reachability( for s in node.sources: # type: ignore[union-attr] if s in nodes: g.add_edge(s, nid) + if type(node).__name__ == "DataNode": + entry = node.subgraph_entry # type: ignore[union-attr] + exit_node = node.subgraph_exit # type: ignore[union-attr] + if entry in nodes: + g.add_edge(nid, entry) + if exit_node in nodes: + g.add_edge(nid, exit_node) reachable = nx.descendants(g, workflow.start_node) | {workflow.start_node} unreachable = set(workflow.nodes.keys()) - reachable @@ -112,6 +120,14 @@ def _validate_fork_join_nodes(workflow: Workflow, issues: list[str]) -> None: if exit_node not in workflow.nodes: issues.append(f"subgraph_fork '{nid}' exit '{exit_node}' not in nodes") + if type(node).__name__ == "DataNode": + entry = node.subgraph_entry # type: ignore[union-attr] + exit_node = node.subgraph_exit # type: ignore[union-attr] + if entry not in workflow.nodes: + issues.append(f"data_node '{nid}' entry '{entry}' not in nodes") + if exit_node not in workflow.nodes: + issues.append(f"data_node '{nid}' exit '{exit_node}' not in nodes") + def validate_workflow(workflow: Workflow) -> list[str]: """Validate a workflow graph. Returns a list of issues (empty = valid).""" @@ -130,13 +146,17 @@ def validate_workflow(workflow: Workflow) -> list[str]: for edge in workflow.edges: g.add_edge(edge.source, edge.target, condition=edge.condition) - # Add implicit edges for SubgraphForkNode: fork → subgraph_entry + # Add implicit edges for SubgraphForkNode and DataNode: node → subgraph_entry # so subgraph nodes are reachable in the graph for nid, node in nodes.items(): if type(node).__name__ == "SubgraphForkNode": entry = node.subgraph_entry # type: ignore[union-attr] if entry in nodes: g.add_edge(nid, entry, condition=None) + if type(node).__name__ == "DataNode": + entry = node.subgraph_entry # type: ignore[union-attr] + if entry in nodes: + g.add_edge(nid, entry, condition=None) _validate_reachability(g, workflow, issues) _validate_cycles(g, workflow, issues) @@ -152,5 +172,13 @@ def validate_workflow(workflow: Workflow) -> list[str]: issues.append( f"subgraph_fork '{nid}': no path from entry '{entry}' to exit '{exit_node}'" ) + if type(node).__name__ == "DataNode": + entry = node.subgraph_entry # type: ignore[union-attr] + exit_node = node.subgraph_exit # type: ignore[union-attr] + if entry in nodes and exit_node in nodes: + if not nx.has_path(g, entry, exit_node): + issues.append( + f"data_node '{nid}': no path from entry '{entry}' to exit '{exit_node}'" + ) return issues diff --git a/tests/test_data_node.py b/tests/test_data_node.py new file mode 100644 index 000000000..ef5119855 --- /dev/null +++ b/tests/test_data_node.py @@ -0,0 +1,479 @@ +"""Tests for the DataNode graph primitive — models, executor, validation, skill export, and features.""" + +from __future__ import annotations + +import asyncio +import json +from pathlib import Path +from unittest.mock import patch + +import pytest +from pydantic import ValidationError + +from factory.workflow.primitives import ( + DataItem, + DataNode, + Edge, + FnNode, + Workflow, +) + + +# ── Phase 1: DataItem / DataNode Pydantic validation ────────────── + + +class TestDataItem: + def test_minimal(self) -> None: + item = DataItem(id="a") + assert item.id == "a" + assert item.path is None + assert item.metadata == {} + assert item.prompt == "" + + def test_full(self) -> None: + item = DataItem(id="b", path="/tmp/b", metadata={"k": "v"}, prompt="do stuff") + assert item.path == "/tmp/b" + assert item.metadata == {"k": "v"} + assert item.prompt == "do stuff" + + def test_extra_field_forbidden(self) -> None: + with pytest.raises(ValidationError): + DataItem(id="a", unknown="x") + + def test_roundtrip(self) -> None: + item = DataItem(id="c", metadata={"x": 1}) + data = item.model_dump(mode="json") + restored = DataItem.model_validate(data) + assert restored.id == "c" + assert restored.metadata == {"x": 1} + + +class TestDataNode: + def test_inline_items_source(self) -> None: + items = [DataItem(id="i1"), DataItem(id="i2")] + node = DataNode( + id="dn", + inline_items=items, + subgraph_entry="a", + subgraph_exit="b", + ) + assert len(node.inline_items) == 2 + assert node.task_ref is None + assert node.source_path is None + + def test_task_ref_source(self) -> None: + node = DataNode( + id="dn", + task_ref="my.module:MyTask", + subgraph_entry="a", + subgraph_exit="b", + ) + assert node.task_ref == "my.module:MyTask" + + def test_source_path_source(self) -> None: + node = DataNode( + id="dn", + source_path="/data/items", + source_format="directory", + subgraph_entry="a", + subgraph_exit="b", + ) + assert node.source_path == "/data/items" + assert node.source_format == "directory" + + def test_no_source_raises(self) -> None: + with pytest.raises(ValidationError, match="Exactly one"): + DataNode( + id="dn", + subgraph_entry="a", + subgraph_exit="b", + ) + + def test_multiple_sources_raises(self) -> None: + with pytest.raises(ValidationError, match="Exactly one"): + DataNode( + id="dn", + task_ref="x", + inline_items=[DataItem(id="i")], + subgraph_entry="a", + subgraph_exit="b", + ) + + def test_source_path_requires_format(self) -> None: + with pytest.raises(ValidationError, match="source_format"): + DataNode( + id="dn", + source_path="/data/items", + subgraph_entry="a", + subgraph_exit="b", + ) + + def test_defaults(self) -> None: + node = DataNode( + id="dn", + inline_items=[DataItem(id="i")], + subgraph_entry="a", + subgraph_exit="b", + ) + assert node.parallelism == 3 + assert node.split == "all" + assert node.shuffle is False + assert node.limit is None + assert node.max_items == 500 + + def test_extra_field_forbidden(self) -> None: + with pytest.raises(ValidationError): + DataNode( + id="dn", + inline_items=[DataItem(id="i")], + subgraph_entry="a", + subgraph_exit="b", + unknown="x", + ) + + def test_from_dict_roundtrip(self) -> None: + items = [DataItem(id="i1", prompt="do it")] + node = DataNode( + id="dn", + inline_items=items, + subgraph_entry="entry", + subgraph_exit="exit", + parallelism=5, + max_items=100, + ) + wf = Workflow( + name="test", + nodes={ + "dn": node, + "entry": FnNode(id="entry", command="echo entry"), + "exit": FnNode(id="exit", command="echo exit"), + }, + edges=[ + Edge(source="dn", target="entry"), + Edge(source="entry", target="exit"), + ], + start_node="dn", + ) + data = wf.to_dict() + restored = Workflow.from_dict(data) + dn = restored.nodes["dn"] + assert type(dn).__name__ == "DataNode" + assert dn.parallelism == 5 + assert dn.max_items == 100 + assert len(dn.inline_items) == 1 + assert dn.inline_items[0].id == "i1" + + +# ── Phase 2: Executor _execute_data ────────────────────────────── + + +def _make_data_workflow(items: list[DataItem]) -> Workflow: + """Build a minimal workflow with a DataNode driving a FnNode subgraph.""" + return Workflow( + name="data_test", + nodes={ + "data": DataNode( + id="data", + inline_items=items, + subgraph_entry="sub_start", + subgraph_exit="sub_end", + parallelism=2, + ), + "sub_start": FnNode(id="sub_start", command="echo start"), + "sub_end": FnNode(id="sub_end", command="echo end"), + }, + edges=[ + Edge(source="sub_start", target="sub_end"), + ], + start_node="data", + ) + + +class TestExecuteData: + def test_inline_items_execute(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + items = [DataItem(id="a", prompt="do a"), DataItem(id="b", prompt="do b")] + wf = _make_data_workflow(items) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success + assert "data" in result.node_outputs + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 2 + assert parsed[0]["item_id"] == "a" + assert parsed[1]["item_id"] == "b" + + def test_fault_isolation_one_bad_item(self, tmp_path: Path) -> None: + """A failing subgraph for one item should not halt the whole DataNode.""" + from factory.workflow.executor import WorkflowExecutor + + items = [DataItem(id="good"), DataItem(id="bad"), DataItem(id="also_good")] + wf = _make_data_workflow(items) + + original_init = WorkflowExecutor.__init__ + + def tracking_init(self_inner, workflow, project_path, *args, **kwargs): + original_init(self_inner, workflow, project_path, *args, **kwargs) + ctx = kwargs.get("initial_context") + self_inner._test_initial_context = ctx + + original_execute = WorkflowExecutor.execute + + async def selective_execute(self_inner): + # Inner executors (sub-workflows) have _test_initial_context set + if hasattr(self_inner, "_test_initial_context") and self_inner.workflow.name.endswith("__data_item"): + # Find which item this is by checking if it's the 2nd call (bad) + if not hasattr(selective_execute, "_inner_count"): + selective_execute._inner_count = 0 + selective_execute._inner_count += 1 + if selective_execute._inner_count == 2: + raise RuntimeError("simulated failure") + return await original_execute(self_inner) + + selective_execute._inner_count = 0 + + with patch.object(WorkflowExecutor, "__init__", tracking_init), \ + patch.object(WorkflowExecutor, "execute", selective_execute): + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 3 + bad_item = next(r for r in parsed if r["item_id"] == "bad") + assert bad_item["score"] == 0.0 + assert "error" in bad_item + good_items = [r for r in parsed if r["item_id"] != "bad"] + assert all(r["success"] for r in good_items) + + def test_max_items_exceeded_raises(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + items = [DataItem(id=str(i)) for i in range(10)] + wf = Workflow( + name="data_test", + nodes={ + "data": DataNode( + id="data", + inline_items=items, + subgraph_entry="sub", + subgraph_exit="sub", + max_items=5, + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.halted + assert "max_items=5" in result.halt_reason + + def test_split_filter(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + items = [ + DataItem(id="train1", metadata={"split": "train"}), + DataItem(id="val1", metadata={"split": "val"}), + DataItem(id="train2", metadata={"split": "train"}), + ] + wf = Workflow( + name="data_test", + nodes={ + "data": DataNode( + id="data", + inline_items=items, + subgraph_entry="sub", + subgraph_exit="sub", + split="train", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 2 + assert all(r["item_id"].startswith("train") for r in parsed) + + def test_limit_filter(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + items = [DataItem(id=str(i)) for i in range(10)] + wf = Workflow( + name="data_test", + nodes={ + "data": DataNode( + id="data", + inline_items=items, + subgraph_entry="sub", + subgraph_exit="sub", + limit=3, + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 3 + + +# ── Phase 3: Validation ───────────────────────────────────────── + + +class TestDataNodeValidation: + def test_valid_data_node_workflow(self) -> None: + wf = _make_data_workflow([DataItem(id="i")]) + issues = wf.validate_graph() + assert not issues + + def test_missing_subgraph_entry(self) -> None: + wf = Workflow( + name="bad", + nodes={ + "data": DataNode( + id="data", + inline_items=[DataItem(id="i")], + subgraph_entry="missing", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + issues = wf.validate_graph() + assert any("missing" in i and "entry" in i for i in issues) + + def test_missing_subgraph_exit(self) -> None: + wf = Workflow( + name="bad", + nodes={ + "data": DataNode( + id="data", + inline_items=[DataItem(id="i")], + subgraph_entry="sub", + subgraph_exit="missing", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + issues = wf.validate_graph() + assert any("missing" in i and "exit" in i for i in issues) + + def test_subgraph_nodes_reachable(self) -> None: + """Subgraph nodes behind a DataNode should not be flagged as unreachable.""" + wf = _make_data_workflow([DataItem(id="i")]) + issues = wf.validate_graph() + unreachable = [i for i in issues if "unreachable" in i] + assert not unreachable + + +# ── Phase 4: Skill export ───────────────────────────────────────── + + +class TestDataNodeSkillExport: + def test_data_node_renders(self) -> None: + from factory.workflow.skill_export import workflow_to_skill_md + + wf = _make_data_workflow([DataItem(id="i")]) + md = workflow_to_skill_md(wf) + assert "Data Iteration" in md + assert "inline items" in md + assert "fault isolation" in md.lower() + + def test_subgraph_nodes_not_duplicated(self) -> None: + from factory.workflow.skill_export import workflow_to_skill_md + + wf = _make_data_workflow([DataItem(id="i")]) + md = workflow_to_skill_md(wf) + # sub_start and sub_end should NOT appear as top-level phases + assert "Sub Start" not in md or md.count("Sub Start") <= 1 + assert "Sub End" not in md or md.count("Sub End") <= 1 + + +# ── Phase 6: compute_features arity ──────────────────────────────── + + +class TestComputeFeaturesDataNode: + def test_arity_is_9(self) -> None: + from factory.outer_loop.similarity import compute_features + + wf = Workflow( + name="w", + nodes={"a": FnNode(id="a", command="x")}, + edges=[], + start_node="a", + ) + features = compute_features(wf) + assert len(features) == 9 + + def test_data_node_sets_feature(self) -> None: + from factory.outer_loop.similarity import compute_features + + wf = _make_data_workflow([DataItem(id="i")]) + features = compute_features(wf) + assert len(features) == 9 + assert features[8] == 1 # has_data_node is the appended axis + + def test_no_data_node_feature_is_zero(self) -> None: + from factory.outer_loop.similarity import compute_features + + wf = Workflow( + name="w", + nodes={"a": FnNode(id="a", command="x")}, + edges=[], + start_node="a", + ) + features = compute_features(wf) + assert features[8] == 0 + + +class TestDiversityMetricNewAxis: + def test_diversity_responds_to_data_node_axis(self) -> None: + from factory.outer_loop.population import MAPElitesArchive, Population + + wf_no_data = Workflow( + name="w", + nodes={"a": FnNode(id="a", command="x")}, + edges=[], + start_node="a", + ) + wf_with_data = _make_data_workflow([DataItem(id="i")]) + + ind1 = Population.make_individual(wf_no_data, score=0.5) + ind2 = Population.make_individual(wf_with_data, score=0.5) + + archive = MAPElitesArchive() + archive.add(ind1) + d1 = archive.diversity_metric() + + archive.add(ind2) + d2 = archive.diversity_metric() + # Adding a structurally different individual should change diversity + assert d2 != d1 or archive.size == 1 + + +# ── Phase 5: compose CAN_ITERATE ────────────────────────────────── + + +class TestComposeCapsDataNode: + def test_data_node_adds_can_iterate(self) -> None: + from factory.compose import ModeCapabilities + from factory.task import Capability + + wf = _make_data_workflow([DataItem(id="i")]) + caps = ModeCapabilities.from_workflow(wf) + assert Capability.CAN_ITERATE in caps.provides diff --git a/tests/test_outer_loop/test_population.py b/tests/test_outer_loop/test_population.py index 6c024ffbb..b817b3d00 100644 --- a/tests/test_outer_loop/test_population.py +++ b/tests/test_outer_loop/test_population.py @@ -62,7 +62,7 @@ def test_make_individual(self, simple_workflow: Workflow) -> None: ind = Population.make_individual(simple_workflow, generation=1, score=0.8) assert ind.generation == 1 assert ind.score == 0.8 - assert len(ind.features) == 8 + assert len(ind.features) == 9 assert ind.parent_id is None def test_serialization_round_trip(self, simple_workflow: Workflow, tmp_path: Path) -> None: diff --git a/tests/test_outer_loop/test_similarity.py b/tests/test_outer_loop/test_similarity.py index a1540c27c..d01b54e68 100644 --- a/tests/test_outer_loop/test_similarity.py +++ b/tests/test_outer_loop/test_similarity.py @@ -98,7 +98,7 @@ def test_type_change_adds_distance(self) -> None: class TestComputeFeatures: def test_simple_workflow(self, simple_workflow: Workflow) -> None: features = compute_features(simple_workflow) - assert len(features) == 8 # fixed-length: 4 base + edge + param + prompt + knob + assert len(features) == 9 # fixed-length: 4 base + edge + param + prompt + knob + has_data_node depth, fork_degree, agent_count, gate_count = features[:4] assert depth >= 4 assert fork_degree == 0 @@ -127,7 +127,7 @@ def test_workflow_with_fork(self) -> None: ] wf = Workflow(name="forked", nodes=nodes, edges=edges, start_node="start") features = compute_features(wf) - assert len(features) == 8 # same fixed length regardless of agent count + assert len(features) == 9 # same fixed length regardless of agent count depth, fork_degree, agent_count, gate_count = features[:4] assert fork_degree == 3 assert agent_count == 3 @@ -143,7 +143,7 @@ def _make_wf(prompt: str) -> Workflow: ) f1 = compute_features(_make_wf("analyze the code")) f2 = compute_features(_make_wf("review the code for bugs")) - assert len(f1) == len(f2) == 8 + assert len(f1) == len(f2) == 9 assert f1 != f2 # different prompts → different features def test_no_agents_still_fixed_length(self) -> None: @@ -154,7 +154,7 @@ def test_no_agents_still_fixed_length(self) -> None: start_node="a", ) features = compute_features(wf) - assert len(features) == 8 + assert len(features) == 9 class TestNoveltyFilter: From bfdbd4be38140b1f68b206906e46f27417420d24 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Wed, 9 Sep 2026 13:49:06 -0400 Subject: [PATCH 02/30] fix: resolve mypy error and add source_path test coverage for DataNode Add assert for self.workflow before WorkflowExecutor construction in _step_with_data_node() to satisfy mypy's type narrowing. Add tests for directory, jsonl, csv source_format paths and non-existent source_path in the executor, plus a test for the inner_loop delegation path. Co-Authored-By: Claude Opus 4.6 --- factory/inner_loop.py | 1 + tests/test_data_node.py | 155 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+) diff --git a/factory/inner_loop.py b/factory/inner_loop.py index b24adfc96..becc1103f 100644 --- a/factory/inner_loop.py +++ b/factory/inner_loop.py @@ -388,6 +388,7 @@ def _step_with_data_node(self, directives: dict[str, Any] | None = None) -> Cycl t0 = time.monotonic() + assert self.workflow is not None executor = WorkflowExecutor( self.workflow, self.project_dir, diff --git a/tests/test_data_node.py b/tests/test_data_node.py index ef5119855..46bf71b31 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -469,6 +469,161 @@ def test_diversity_responds_to_data_node_axis(self) -> None: # ── Phase 5: compose CAN_ITERATE ────────────────────────────────── +# ── Phase 7: source_path code paths ───────────────────────────── + + +class TestSourcePathDirectory: + def test_directory_source(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + src_dir = tmp_path / "data" + src_dir.mkdir() + (src_dir / "alpha").mkdir() + (src_dir / "beta").mkdir() + (src_dir / "plain_file.txt").write_text("not a dir") + + wf = Workflow( + name="dir_test", + nodes={ + "data": DataNode( + id="data", + source_path=str(src_dir), + source_format="directory", + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.success + parsed = json.loads(result.node_outputs["data"]) + ids = [r["item_id"] for r in parsed] + assert "alpha" in ids + assert "beta" in ids + assert "plain_file.txt" not in ids + + +class TestSourcePathJsonl: + def test_jsonl_source(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + jsonl_file = tmp_path / "items.jsonl" + jsonl_file.write_text('{"name": "first"}\n{"name": "second"}\n\n') + + wf = Workflow( + name="jsonl_test", + nodes={ + "data": DataNode( + id="data", + source_path=str(jsonl_file), + source_format="jsonl", + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 2 + assert parsed[0]["item_id"] == "0" + assert parsed[1]["item_id"] == "1" + + +class TestSourcePathCsv: + def test_csv_source(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + csv_file = tmp_path / "items.csv" + csv_file.write_text("id,value\na,1\nb,2\nc,3\n") + + wf = Workflow( + name="csv_test", + nodes={ + "data": DataNode( + id="data", + source_path=str(csv_file), + source_format="csv", + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 3 + assert parsed[0]["item_id"] == "0" + assert parsed[1]["item_id"] == "1" + assert parsed[2]["item_id"] == "2" + + +class TestSourcePathNonExistent: + def test_nonexistent_path_yields_empty(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + wf = Workflow( + name="missing_test", + nodes={ + "data": DataNode( + id="data", + source_path=str(tmp_path / "does_not_exist"), + source_format="directory", + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 0 + + +# ── Phase 8: inner_loop _step_with_data_node ──────────────────── + + +class TestStepWithDataNode: + def test_delegates_to_executor(self, tmp_path: Path) -> None: + from unittest.mock import AsyncMock, MagicMock + + from factory.inner_loop import InnerLoop + from factory.workflow.executor import ExecutionResult + + wf = _make_data_workflow([DataItem(id="i", prompt="go")]) + + mock_result = ExecutionResult() + mock_result.success = True + + loop = InnerLoop(project_dir=tmp_path, workflow=wf) + + with patch( + "factory.workflow.executor.WorkflowExecutor.execute", + new_callable=AsyncMock, + return_value=mock_result, + ): + record = loop._step_with_data_node() + + assert record.score_end == 1.0 + assert record.cycle_number == 1 + + class TestComposeCapsDataNode: def test_data_node_adds_can_iterate(self) -> None: from factory.compose import ModeCapabilities From bfefdcc76f232d83f41eab9b2c0ae23c4b1fb1fd Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Wed, 9 Sep 2026 14:14:03 -0400 Subject: [PATCH 03/30] =?UTF-8?q?test:=20add=20DataNode=20integration=20te?= =?UTF-8?q?sts=20for=20Task=E2=86=92InnerLoop=E2=86=92Executor=E2=86=92out?= =?UTF-8?q?er=20loop=20pipeline?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds TestDataNodeIntegration class with 5 tests covering: - DataNode workflow routing through _step_with_data_node() - DataNode delegates to executor once (not per-instance) - Non-DataNode workflows still use manual per-instance loop - compute_features detects DataNode at feature index 8 - Full pipeline: DataNode → InnerLoop.step() → CycleRecord + features Co-Authored-By: Claude Opus 4.6 --- tests/test_inner_outer_loop.py | 200 +++++++++++++++++++++++++++++++++ 1 file changed, 200 insertions(+) diff --git a/tests/test_inner_outer_loop.py b/tests/test_inner_outer_loop.py index 9020cb92c..aa4289e52 100644 --- a/tests/test_inner_outer_loop.py +++ b/tests/test_inner_outer_loop.py @@ -13,6 +13,7 @@ import json import subprocess from pathlib import Path +from unittest.mock import MagicMock import pytest @@ -30,6 +31,7 @@ ) from factory.research.runner import aggregate_metric from factory.store import ExperimentStore, _parse_inner_loop, _parse_outer_loop +from factory.workflow.primitives import Workflow # ── Model validation ──────────────────────────────────────────── @@ -630,3 +632,201 @@ def test_zero_threshold_returns_false(self) -> None: summaries = [{"metric_value": 0.5}] assert detect_research_plateau(summaries, threshold=0) is False + + +# ── DataNode integration: Task → InnerLoop → WorkflowExecutor → outer loop ── + + +class TestDataNodeIntegration: + """Integration tests proving the full DataNode pipeline works end-to-end: + Task → InnerLoop → WorkflowExecutor → DataNode → outer loop feature extraction. + """ + + @staticmethod + def _async_return(val: object) -> MagicMock: + async def _coro(*a: object, **kw: object) -> object: + return val + m = MagicMock(side_effect=_coro) + return m + + @staticmethod + def _make_data_workflow() -> Workflow: + from factory.workflow.primitives import DataItem, DataNode, Edge, FnNode, Workflow + + return Workflow( + name="data_integration", + nodes={ + "data": DataNode( + id="data", + inline_items=[ + DataItem(id="item-1", prompt="solve 1"), + DataItem(id="item-2", prompt="solve 2"), + ], + subgraph_entry="sub_start", + subgraph_exit="sub_end", + parallelism=1, + ), + "sub_start": FnNode(id="sub_start", command="echo start"), + "sub_end": FnNode(id="sub_end", command="echo end"), + }, + edges=[ + Edge(source="sub_start", target="sub_end"), + ], + start_node="data", + ) + + @staticmethod + def _make_plain_workflow() -> Workflow: + from factory.workflow.primitives import AgentNode, AgentRole, Workflow + + return Workflow( + name="plain", + nodes={ + "builder": AgentNode( + id="builder", + role=AgentRole.BUILDER, + prompt_template="build {project_path}", + ), + }, + edges=[], + start_node="builder", + ) + + @staticmethod + def _make_exec_result(success: bool = True) -> MagicMock: + r = MagicMock() + r.success = success + r.halted = not success + r.halt_reason = "" if success else "halted" + r.nodes_executed = 1 + r.duration_ms = 100.0 + return r + + def test_data_node_routes_through_step_with_data_node(self, tmp_path: Path) -> None: + """DataNode workflow routes through InnerLoop._step_with_data_node().""" + from unittest.mock import AsyncMock, patch + + from factory.inner_loop import InnerLoop + from factory.workflow.executor import ExecutionResult + + wf = self._make_data_workflow() + task = MagicMock() + + mock_result = ExecutionResult() + mock_result.success = True + + loop = InnerLoop(project_dir=tmp_path, workflow=wf, task=task) + + assert loop._workflow_has_data_node() is True + + with patch( + "factory.workflow.executor.WorkflowExecutor.execute", + new_callable=AsyncMock, + return_value=mock_result, + ): + record = loop.step() + + assert record.score_end == 1.0 + assert record.cycle_number == 1 + + def test_data_node_delegates_to_executor_not_per_instance(self, tmp_path: Path) -> None: + """DataNode workflow calls WorkflowExecutor.execute() once, not per instance.""" + from unittest.mock import AsyncMock, patch + + from factory.inner_loop import InnerLoop + from factory.workflow.executor import ExecutionResult + + wf = self._make_data_workflow() + task = MagicMock() + + mock_result = ExecutionResult() + mock_result.success = True + + loop = InnerLoop(project_dir=tmp_path, workflow=wf, task=task) + + with patch( + "factory.workflow.executor.WorkflowExecutor.execute", + new_callable=AsyncMock, + return_value=mock_result, + ) as mock_execute: + loop.step() + + mock_execute.assert_called_once() + task.instances.assert_not_called() + task.setup.assert_not_called() + task.verify.assert_not_called() + + def test_no_data_node_uses_manual_per_instance_loop(self, tmp_path: Path) -> None: + """Without DataNode, InnerLoop uses the manual per-instance loop.""" + from unittest.mock import patch + + from factory.inner_loop import InnerLoop + from factory.task import ScoringContract, TaskDefinition, TaskInstance, VerifyResult + + wf = self._make_plain_workflow() + task = MagicMock() + task.instances.return_value = [TaskInstance(id="inst-1")] + task.definition = TaskDefinition(name="mock", scoring=ScoringContract(method="exit_code")) + task.setup.return_value = None + task.prompt.return_value = "test prompt" + task.verify.return_value = VerifyResult(passed=True, score=0.75) + + with patch("factory.workflow.executor.WorkflowExecutor") as MockExecutor: + mock_exec = MagicMock() + mock_exec.execute = self._async_return(self._make_exec_result()) + MockExecutor.return_value = mock_exec + + loop = InnerLoop(project_dir=tmp_path, mode="test", task=task, workflow=wf) + record = loop.step() + + task.instances.assert_called_once() + task.setup.assert_called_once() + task.prompt.assert_called_once() + task.verify.assert_called_once() + assert record.score_end == pytest.approx(0.75) + assert record.instance_results is not None + assert len(record.instance_results) == 1 + + def test_compute_features_detects_data_node(self) -> None: + """compute_features returns has_data_node=1 at index 8 for DataNode workflows.""" + from factory.outer_loop.similarity import compute_features + + wf_with = self._make_data_workflow() + features_with = compute_features(wf_with) + assert len(features_with) == 9 + assert features_with[8] == 1 + + wf_without = self._make_plain_workflow() + features_without = compute_features(wf_without) + assert len(features_without) == 9 + assert features_without[8] == 0 + + def test_full_pipeline_data_node_through_inner_loop_with_features(self, tmp_path: Path) -> None: + """Full pipeline: DataNode workflow → InnerLoop.step() → CycleRecord + compute_features.""" + from unittest.mock import AsyncMock, patch + + from factory.inner_loop import InnerLoop + from factory.outer_loop.similarity import compute_features + from factory.workflow.executor import ExecutionResult + + wf = self._make_data_workflow() + task = MagicMock() + + mock_result = ExecutionResult() + mock_result.success = True + + loop = InnerLoop(project_dir=tmp_path, workflow=wf, task=task) + + with patch( + "factory.workflow.executor.WorkflowExecutor.execute", + new_callable=AsyncMock, + return_value=mock_result, + ): + record = loop.step() + + assert record.score_end is not None + assert record.score_end == 1.0 + + features = compute_features(wf) + assert features[8] == 1 + assert loop._workflow_has_data_node() is True From 41e9e13547e5817779c75442fcfa8a20f61e469f Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Wed, 9 Sep 2026 14:48:58 -0400 Subject: [PATCH 04/30] feat: auto-freeze DataNode IDs in outer loop mutations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DataNodes are infrastructure that drives per-item subgraph execution — the optimizer should never accidentally mutate or remove them. Add _auto_frozen_nodes() helper and apply it at both mutation call sites in SwarmEngine (seed and evolve_generation). Co-Authored-By: Claude Opus 4.6 --- factory/outer_loop/engine.py | 11 +++- tests/test_outer_loop/test_mutations.py | 77 +++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/factory/outer_loop/engine.py b/factory/outer_loop/engine.py index 8da8b550f..e7a96eceb 100644 --- a/factory/outer_loop/engine.py +++ b/factory/outer_loop/engine.py @@ -39,6 +39,13 @@ PLATEAU_WINDOW = 3 +def _auto_frozen_nodes(workflow: Workflow) -> set[str]: + """Return node IDs that should always be frozen during mutation.""" + from factory.workflow.primitives import DataNode + + return {nid for nid, node in workflow.nodes.items() if isinstance(node, DataNode)} + + class BudgetTracker: """Tracks evaluation budget consumption, cost, and wall-clock time.""" @@ -169,7 +176,7 @@ def seed( base_workflow, self._strategy, generation=0, - frozen_nodes=set(cfg.frozen_node_ids), + frozen_nodes=set(cfg.frozen_node_ids) | _auto_frozen_nodes(base_workflow), ) if result is None: continue @@ -298,7 +305,7 @@ def evolve_generation( parent_wf, self._strategy, generation, - frozen_nodes=set(self._config.frozen_node_ids), + frozen_nodes=set(self._config.frozen_node_ids) | _auto_frozen_nodes(parent_wf), reflection_report=self._last_reflection, ) if mutation_result is None: diff --git a/tests/test_outer_loop/test_mutations.py b/tests/test_outer_loop/test_mutations.py index 12cfd414e..bdc4c70c8 100644 --- a/tests/test_outer_loop/test_mutations.py +++ b/tests/test_outer_loop/test_mutations.py @@ -935,3 +935,80 @@ def test_invalid_field_value_returns_none(self, simple_workflow: Workflow) -> No simple_workflow, "researcher", {"timeout": "not_a_number"} ) assert result is None + + +class TestAutoFrozenNodes: + """Tests for _auto_frozen_nodes and DataNode auto-freeze in engine.""" + + def test_auto_frozen_nodes_returns_data_node_ids(self) -> None: + from factory.outer_loop.engine import _auto_frozen_nodes + from factory.workflow.primitives import DataNode, DataItem + + nodes: dict[str, AgentNode | FnNode | DataNode] = { + "data_loader": DataNode( + id="data_loader", + inline_items=[DataItem(id="item1", prompt="test")], + subgraph_entry="builder", + subgraph_exit="builder", + ), + "builder": AgentNode(id="builder", role=AgentRole.BUILDER), + "study": FnNode(id="study", command="echo hi"), + } + edges = [ + Edge(source="study", target="data_loader"), + Edge(source="data_loader", target="builder"), + ] + wf = Workflow(name="with_data", nodes=nodes, edges=edges, start_node="study") + frozen = _auto_frozen_nodes(wf) + assert frozen == {"data_loader"} + + def test_auto_frozen_nodes_empty_when_no_data_nodes(self) -> None: + from factory.outer_loop.engine import _auto_frozen_nodes + + nodes: dict[str, AgentNode | FnNode] = { + "builder": AgentNode(id="builder", role=AgentRole.BUILDER), + "study": FnNode(id="study", command="echo hi"), + } + edges = [Edge(source="study", target="builder")] + wf = Workflow(name="no_data", nodes=nodes, edges=edges, start_node="study") + frozen = _auto_frozen_nodes(wf) + assert frozen == set() + + def test_data_node_protected_from_direct_removal(self) -> None: + """Frozen DataNode cannot be directly removed or param-mutated.""" + from factory.outer_loop.engine import _auto_frozen_nodes + from factory.workflow.primitives import DataNode, DataItem + + nodes: dict[str, AgentNode | FnNode | DataNode] = { + "study": FnNode( + id="study", + command="factory study", + writes={".factory/strategy/observations.md"}, + ), + "data_loader": DataNode( + id="data_loader", + inline_items=[DataItem(id="item1", prompt="test")], + subgraph_entry="builder", + subgraph_exit="builder", + reads={".factory/strategy/observations.md"}, + ), + "builder": AgentNode( + id="builder", + role=AgentRole.BUILDER, + reads={".factory/strategy/observations.md"}, + writes={".factory/reviews/builder-latest.md"}, + ), + } + edges = [ + Edge(source="study", target="data_loader"), + Edge(source="data_loader", target="builder"), + ] + wf = Workflow(name="data_test", nodes=nodes, edges=edges, start_node="study") + + frozen = _auto_frozen_nodes(wf) + assert "data_loader" in frozen + + assert remove_node(wf, "data_loader", frozen_nodes=frozen) is None + assert mutate_params( + wf, "data_loader", {"timeout": 999}, frozen_nodes=frozen, + ) is None From f6813ad0515f7ea693eeee614c542f15b7def1ac Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Wed, 9 Sep 2026 14:54:21 -0400 Subject: [PATCH 05/30] fix: remove unused MagicMock import in test_data_node.py Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_016Ugj23EAAf1HoHcknrBwhL --- tests/test_data_node.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_data_node.py b/tests/test_data_node.py index 46bf71b31..e4c6a310c 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -601,7 +601,7 @@ def test_nonexistent_path_yields_empty(self, tmp_path: Path) -> None: class TestStepWithDataNode: def test_delegates_to_executor(self, tmp_path: Path) -> None: - from unittest.mock import AsyncMock, MagicMock + from unittest.mock import AsyncMock from factory.inner_loop import InnerLoop from factory.workflow.executor import ExecutionResult From 3f26816d7a272e244972b1fdf774c4c82374b0d0 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Wed, 9 Sep 2026 15:06:52 -0400 Subject: [PATCH 06/30] fix: seed sub-executor completed_files in DataNode so subgraph reads don't timeout The per-item WorkflowExecutor in _execute_data() was created with empty completed_files, causing _wait_for_reads to poll for 60s then halt if the subgraph's start node had reads dependencies on upstream artifacts. Copying the parent's completed_files into each item_executor lets the subgraph inherit the parent's artifact state. Co-Authored-By: Claude Opus 4.6 --- factory/workflow/executor.py | 1 + tests/test_data_node.py | 77 ++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index a413fba1b..12bc76137 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -729,6 +729,7 @@ async def run_item(item: DataItem) -> dict[str, Any]: dry_run=self.dry_run, initial_context=item.prompt, ) + item_executor.completed_files = self.completed_files.copy() item_result = await item_executor.execute() return { "item_id": item.id, diff --git a/tests/test_data_node.py b/tests/test_data_node.py index e4c6a310c..a1b1d29f4 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -624,6 +624,83 @@ def test_delegates_to_executor(self, tmp_path: Path) -> None: assert record.cycle_number == 1 +class TestSubgraphInheritsCompletedFiles: + """Verify that subgraph executors inherit parent completed_files.""" + + def test_subgraph_reads_upstream_artifact(self, tmp_path: Path) -> None: + """Subgraph start node with reads={'data_ready'} should inherit the + artifact from an upstream FnNode that writes={'data_ready'}, so it + executes instead of timing out.""" + from factory.workflow.executor import WorkflowExecutor + + wf = Workflow( + name="inherit_test", + nodes={ + "upstream_fn": FnNode( + id="upstream_fn", command="echo ready", writes={"data_ready"}, + ), + "data_loader": DataNode( + id="data_loader", + inline_items=[DataItem(id="item1", prompt="go")], + subgraph_entry="process_node", + subgraph_exit="exit_node", + ), + "process_node": FnNode( + id="process_node", + command="echo processing", + reads={"data_ready"}, + ), + "exit_node": FnNode(id="exit_node", command="echo done"), + }, + edges=[ + Edge(source="upstream_fn", target="data_loader"), + Edge(source="process_node", target="exit_node"), + ], + start_node="upstream_fn", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success, f"Expected success but got halt: {result.halt_reason}" + assert not result.halted + parsed = json.loads(result.node_outputs["data_loader"]) + assert len(parsed) == 1 + assert parsed[0]["nodes_executed"] > 0 + + def test_subgraph_no_reads_still_works(self, tmp_path: Path) -> None: + """Subgraph start node with no reads should execute normally (baseline).""" + from factory.workflow.executor import WorkflowExecutor + + wf = Workflow( + name="no_reads_test", + nodes={ + "upstream_fn": FnNode( + id="upstream_fn", command="echo ready", writes={"data_ready"}, + ), + "data_loader": DataNode( + id="data_loader", + inline_items=[DataItem(id="item1")], + subgraph_entry="process_node", + subgraph_exit="exit_node", + ), + "process_node": FnNode(id="process_node", command="echo processing"), + "exit_node": FnNode(id="exit_node", command="echo done"), + }, + edges=[ + Edge(source="upstream_fn", target="data_loader"), + Edge(source="process_node", target="exit_node"), + ], + start_node="upstream_fn", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success + parsed = json.loads(result.node_outputs["data_loader"]) + assert len(parsed) == 1 + assert parsed[0]["nodes_executed"] > 0 + + class TestComposeCapsDataNode: def test_data_node_adds_can_iterate(self) -> None: from factory.compose import ModeCapabilities From 2687f8434b9a89ff2c452b1a1f3b9a4f74c89dfa Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Wed, 9 Sep 2026 15:16:12 -0400 Subject: [PATCH 07/30] fix: pre-seed sub-executor completed_files with on-disk reads in DataNode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DataNode sub-executors timed out (60s) waiting for files that task.setup() had already created on disk. _wait_for_reads only checked the completed_files set, which tracks node outputs — not filesystem state. Before spawning per-item sub-executors, scan all subgraph node reads and add any that already exist on disk to completed_files. Co-Authored-By: Claude Opus 4.6 --- factory/workflow/executor.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index 12bc76137..98751d98c 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -719,6 +719,13 @@ async def _execute_data(self, node_id: str, node: DataNode) -> None: item_results: list[dict[str, Any]] = [] sem = asyncio.Semaphore(node.parallelism) + # Pre-compute reads that exist on disk (e.g. created by task.setup()) + disk_reads: set[str] = set() + for sg_node in sub_workflow.nodes.values(): + for r in sg_node.reads: + if (self.project_path / r).exists(): + disk_reads.add(r) + async def run_item(item: DataItem) -> dict[str, Any]: async with sem: try: @@ -729,7 +736,7 @@ async def run_item(item: DataItem) -> dict[str, Any]: dry_run=self.dry_run, initial_context=item.prompt, ) - item_executor.completed_files = self.completed_files.copy() + item_executor.completed_files = self.completed_files | disk_reads item_result = await item_executor.execute() return { "item_id": item.id, From bf5e5cefb7578e043f4859212a0c1b6c93e81972 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Wed, 9 Sep 2026 15:30:59 -0400 Subject: [PATCH 08/30] fix: call task.verify() in DataNode task_ref execution path DataNode with task_ref resolved items and ran subgraphs but never called task.verify(), silently scoring every completed subgraph as 1.0 regardless of the task's actual evaluation. This made task-defined pass/fail invisible. Changes: - executor.py: Pair TaskInstances with DataItems so filters apply in lockstep. Move setup()/prompt() from eager pre-loop into run_item() for per-item fault isolation. Call task.verify() after each subgraph completes and use its score/passed/details instead of binary subgraph success. - inner_loop.py: _step_with_data_node() now aggregates per-item verify scores (mean) from DataNode output instead of using binary exec success. - inline_items and source_path paths remain unchanged (no Task, no verify). Co-Authored-By: Claude Opus 4.6 --- factory/inner_loop.py | 12 ++ factory/workflow/executor.py | 84 +++++++++----- tests/test_data_node.py | 209 +++++++++++++++++++++++++++++++++++ 3 files changed, 279 insertions(+), 26 deletions(-) diff --git a/factory/inner_loop.py b/factory/inner_loop.py index becc1103f..0ec8af835 100644 --- a/factory/inner_loop.py +++ b/factory/inner_loop.py @@ -383,6 +383,7 @@ def _step_with_task(self, directives: dict[str, Any] | None = None) -> CycleReco def _step_with_data_node(self, directives: dict[str, Any] | None = None) -> CycleRecord: """Delegate to the executor when the workflow contains a DataNode.""" import asyncio + import json from factory.workflow.executor import WorkflowExecutor @@ -398,6 +399,17 @@ def _step_with_data_node(self, directives: dict[str, Any] | None = None) -> Cycl duration_s = time.monotonic() - t0 score = 1.0 if exec_result.success else 0.0 + # Aggregate per-item verify scores from DataNode output if available + for _nid, output in exec_result.node_outputs.items(): + try: + parsed = json.loads(output) + if isinstance(parsed, list) and parsed and "score" in parsed[0]: + scores = [r["score"] for r in parsed] + score = sum(scores) / len(scores) if scores else 0.0 + break + except (json.JSONDecodeError, TypeError, KeyError): + continue + record = CycleRecord( cycle_number=self._step_count + 1, mode=self.mode, diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index 98751d98c..8abf09ec2 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -638,6 +638,9 @@ async def _execute_data(self, node_id: str, node: DataNode) -> None: """Execute a DataNode: resolve items, run subgraph per item with fault isolation.""" import random as _random + from factory.task import Task as _Task + from factory.task import TaskInstance as _TaskInstance + self.result.nodes_executed += 1 self._emit( @@ -652,22 +655,24 @@ async def _execute_data(self, node_id: str, node: DataNode) -> None: start = time.monotonic() - # Resolve data items from exactly one source - items: list[DataItem] = [] + # Resolve data items from exactly one source, keeping TaskInstances for task_ref + task_instances: list[tuple[DataItem, _TaskInstance | None]] = [] + resolved_task: _Task | None = None + if node.inline_items: - items = list(node.inline_items) + task_instances = [(item, None) for item in node.inline_items] elif node.task_ref: from factory.task import TaskRef task_ref = TaskRef(ref=node.task_ref) - task = task_ref.resolve() - for inst in task.instances(): - task.setup(inst, self.project_path) - prompt_text = task.prompt(inst) - items.append(DataItem( - id=inst.id, - path=str(inst.path) if inst.path else None, - metadata=inst.metadata, - prompt=prompt_text, + resolved_task = task_ref.resolve() + for inst in resolved_task.instances(): + task_instances.append(( + DataItem( + id=inst.id, + path=str(inst.path) if inst.path else None, + metadata=inst.metadata, + ), + inst, )) elif node.source_path: from pathlib import Path as _Path @@ -677,32 +682,35 @@ async def _execute_data(self, node_id: str, node: DataNode) -> None: if node.source_format == "directory" and src.is_dir(): for child in sorted(src.iterdir()): if child.is_dir(): - items.append(DataItem(id=child.name, path=str(child))) + task_instances.append((DataItem(id=child.name, path=str(child)), None)) elif node.source_format == "jsonl" and src.is_file(): for idx, line in enumerate(src.read_text().splitlines()): if line.strip(): - items.append(DataItem( + task_instances.append((DataItem( id=str(idx), metadata=json.loads(line), - )) + ), None)) elif node.source_format == "csv" and src.is_file(): import csv with src.open(newline="") as f: reader = csv.DictReader(f) for idx, row in enumerate(reader): - items.append(DataItem(id=str(idx), metadata=dict(row))) + task_instances.append((DataItem(id=str(idx), metadata=dict(row)), None)) - # Apply split/shuffle/limit filters + # Apply split/shuffle/limit filters to the paired list if node.split != "all": - items = [it for it in items if it.metadata.get("split") == node.split] + task_instances = [ + (item, inst) for item, inst in task_instances + if item.metadata.get("split") == node.split + ] if node.shuffle: - _random.shuffle(items) + _random.shuffle(task_instances) if node.limit is not None and node.limit > 0: - items = items[:node.limit] + task_instances = task_instances[:node.limit] - if len(items) > node.max_items: + if len(task_instances) > node.max_items: raise ValueError( - f"DataNode '{node_id}' resolved {len(items)} items, " + f"DataNode '{node_id}' resolved {len(task_instances)} items, " f"exceeding max_items={node.max_items}" ) @@ -719,16 +727,26 @@ async def _execute_data(self, node_id: str, node: DataNode) -> None: item_results: list[dict[str, Any]] = [] sem = asyncio.Semaphore(node.parallelism) - # Pre-compute reads that exist on disk (e.g. created by task.setup()) + # Pre-compute reads that exist on disk disk_reads: set[str] = set() for sg_node in sub_workflow.nodes.values(): for r in sg_node.reads: if (self.project_path / r).exists(): disk_reads.add(r) - async def run_item(item: DataItem) -> dict[str, Any]: + async def run_item(pair: tuple[DataItem, _TaskInstance | None]) -> dict[str, Any]: + item, inst = pair async with sem: try: + if resolved_task is not None and inst is not None: + resolved_task.setup(inst, self.project_path) + item = DataItem( + id=inst.id, + path=str(inst.path) if inst.path else None, + metadata=inst.metadata, + prompt=resolved_task.prompt(inst), + ) + item_executor = WorkflowExecutor( sub_workflow.model_copy(deep=True), self.project_path, @@ -738,12 +756,25 @@ async def run_item(item: DataItem) -> dict[str, Any]: ) item_executor.completed_files = self.completed_files | disk_reads item_result = await item_executor.execute() + + score = 1.0 if item_result.success else 0.0 + passed = item_result.success + verify_details: dict[str, Any] = {} + + if resolved_task is not None and inst is not None: + vr = resolved_task.verify(inst, self.project_path) + score = vr.score + passed = vr.passed + verify_details = vr.details or {} + return { "item_id": item.id, "success": item_result.success, - "score": 1.0 if item_result.success else 0.0, + "score": score, + "passed": passed, "nodes_executed": item_result.nodes_executed, "node_outputs": item_result.node_outputs, + "verify_details": verify_details, } except Exception as exc: log.warning("data_item_failed", item_id=item.id, error=str(exc)) @@ -751,10 +782,11 @@ async def run_item(item: DataItem) -> dict[str, Any]: "item_id": item.id, "success": False, "score": 0.0, + "passed": False, "error": str(exc), } - tasks = [run_item(item) for item in items] + tasks = [run_item(pair) for pair in task_instances] results = await asyncio.gather(*tasks) item_results = list(results) diff --git a/tests/test_data_node.py b/tests/test_data_node.py index a1b1d29f4..f48d22f54 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -5,6 +5,7 @@ import asyncio import json from pathlib import Path +from typing import Any from unittest.mock import patch import pytest @@ -709,3 +710,211 @@ def test_data_node_adds_can_iterate(self) -> None: wf = _make_data_workflow([DataItem(id="i")]) caps = ModeCapabilities.from_workflow(wf) assert Capability.CAN_ITERATE in caps.provides + + +# ── Phase 9: task_ref verify integration ────────────────────────── + + +class _FakeTask: + """Minimal Task-like object for testing verify() integration.""" + + def __init__(self, instances_data: list[dict[str, Any]], verify_scores: dict[str, float]) -> None: + self._instances_data = instances_data + self._verify_scores = verify_scores + self.setup_calls: list[str] = [] + self.prompt_calls: list[str] = [] + self.verify_calls: list[str] = [] + + def instances(self): + from factory.task import TaskInstance + for d in self._instances_data: + yield TaskInstance(id=d["id"], path=d.get("path"), metadata=d.get("metadata", {})) + + def setup(self, instance, workspace): + self.setup_calls.append(instance.id) + + def prompt(self, instance): + self.prompt_calls.append(instance.id) + return f"prompt for {instance.id}" + + def verify(self, instance, workspace): + from factory.task import VerifyResult + self.verify_calls.append(instance.id) + score = self._verify_scores.get(instance.id, 0.0) + return VerifyResult(passed=score > 0.5, score=score, details={"source": "fake"}) + + +def _make_task_ref_workflow(task_ref: str = "fake.module:FakeTask") -> Workflow: + """Build a minimal workflow with a task_ref DataNode.""" + return Workflow( + name="task_ref_test", + nodes={ + "data": DataNode( + id="data", + task_ref=task_ref, + subgraph_entry="sub_start", + subgraph_exit="sub_end", + parallelism=2, + ), + "sub_start": FnNode(id="sub_start", command="echo start"), + "sub_end": FnNode(id="sub_end", command="echo end"), + }, + edges=[ + Edge(source="sub_start", target="sub_end"), + ], + start_node="data", + ) + + +class TestTaskRefVerify: + def test_verify_called_per_item_and_scores_used(self, tmp_path: Path) -> None: + """task_ref DataNode must call verify() per item and use verify scores.""" + from factory.workflow.executor import WorkflowExecutor + + fake_task = _FakeTask( + instances_data=[{"id": "inst_a"}, {"id": "inst_b"}], + verify_scores={"inst_a": 0.8, "inst_b": 0.3}, + ) + + wf = _make_task_ref_workflow() + + with patch("factory.task.TaskRef.resolve", return_value=fake_task): + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 2 + + item_a = next(r for r in parsed if r["item_id"] == "inst_a") + item_b = next(r for r in parsed if r["item_id"] == "inst_b") + assert item_a["score"] == 0.8 + assert item_a["passed"] is True + assert item_b["score"] == 0.3 + assert item_b["passed"] is False + + assert "inst_a" in fake_task.verify_calls + assert "inst_b" in fake_task.verify_calls + + def test_inline_items_no_verify(self, tmp_path: Path) -> None: + """inline_items DataNode must NOT call verify — uses subgraph-success scoring.""" + from factory.workflow.executor import WorkflowExecutor + + items = [DataItem(id="x", prompt="go"), DataItem(id="y", prompt="go")] + wf = _make_data_workflow(items) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert all(r["score"] == 1.0 for r in parsed) + assert all("verify_details" not in r or r["verify_details"] == {} for r in parsed) + + def test_setup_prompt_called_per_item_in_run_item(self, tmp_path: Path) -> None: + """setup() and prompt() must be called per-item inside run_item, not eagerly.""" + from factory.workflow.executor import WorkflowExecutor + + fake_task = _FakeTask( + instances_data=[{"id": "i1"}, {"id": "i2"}, {"id": "i3"}], + verify_scores={"i1": 1.0, "i2": 1.0, "i3": 1.0}, + ) + + wf = _make_task_ref_workflow() + + with patch("factory.task.TaskRef.resolve", return_value=fake_task): + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success + assert sorted(fake_task.setup_calls) == ["i1", "i2", "i3"] + assert sorted(fake_task.prompt_calls) == ["i1", "i2", "i3"] + assert sorted(fake_task.verify_calls) == ["i1", "i2", "i3"] + + def test_failing_setup_does_not_block_other_items(self, tmp_path: Path) -> None: + """A failing setup() for one item must not prevent other items from running.""" + from factory.workflow.executor import WorkflowExecutor + + fake_task = _FakeTask( + instances_data=[{"id": "ok1"}, {"id": "fail_setup"}, {"id": "ok2"}], + verify_scores={"ok1": 1.0, "ok2": 0.9}, + ) + original_setup = fake_task.setup + + def failing_setup(instance, workspace): + if instance.id == "fail_setup": + raise RuntimeError("setup exploded") + original_setup(instance, workspace) + + fake_task.setup = failing_setup + + wf = _make_task_ref_workflow() + + with patch("factory.task.TaskRef.resolve", return_value=fake_task): + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 3 + + failed = next(r for r in parsed if r["item_id"] == "fail_setup") + assert failed["score"] == 0.0 + assert "error" in failed + + ok_items = [r for r in parsed if r["item_id"] != "fail_setup"] + assert all(r["score"] > 0 for r in ok_items) + + +class TestStepWithDataNodeVerifyScores: + def test_aggregates_verify_scores(self, tmp_path: Path) -> None: + """_step_with_data_node should aggregate per-item verify scores, not binary.""" + from unittest.mock import AsyncMock + + from factory.inner_loop import InnerLoop + from factory.workflow.executor import ExecutionResult + + wf = _make_task_ref_workflow() + + mock_result = ExecutionResult() + mock_result.success = True + mock_result.node_outputs = { + "data": json.dumps([ + {"item_id": "a", "score": 0.8, "success": True}, + {"item_id": "b", "score": 0.4, "success": True}, + ]) + } + + loop = InnerLoop(project_dir=tmp_path, workflow=wf) + + with patch( + "factory.workflow.executor.WorkflowExecutor.execute", + new_callable=AsyncMock, + return_value=mock_result, + ): + record = loop._step_with_data_node() + + assert record.score_end == pytest.approx(0.6) + + def test_falls_back_to_binary_without_scores(self, tmp_path: Path) -> None: + """Without per-item scores in output, falls back to exec_result.success.""" + from unittest.mock import AsyncMock + + from factory.inner_loop import InnerLoop + from factory.workflow.executor import ExecutionResult + + wf = _make_data_workflow([DataItem(id="i")]) + + mock_result = ExecutionResult() + mock_result.success = True + mock_result.node_outputs = {} + + loop = InnerLoop(project_dir=tmp_path, workflow=wf) + + with patch( + "factory.workflow.executor.WorkflowExecutor.execute", + new_callable=AsyncMock, + return_value=mock_result, + ): + record = loop._step_with_data_node() + + assert record.score_end == 1.0 From 0ade4c4e45522c30b73ef876070725f84eba0b6a Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Thu, 10 Sep 2026 18:37:36 +0000 Subject: [PATCH 09/30] fix: address all PR #1483 review feedback (8 items) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix 1: parallelism default 3→1, Field(ge=1) constraint, worktree isolation for parallelism>1 - Fix 2: write current_item.json for subgraph item visibility (cleaned up in finally) - Fix 3: reject explicit DataNode→subgraph edges in validation - Fix 4: raise on missing source_path, warn on empty data, isolate JSONL errors - Fix 5: seeded shuffle via random.Random(seed), shuffle_seed field on DataNode - Fix 6: direct DataNode score lookup by node ID + populate instance_results - Fix 7: document spec deviations in PR body (applied via gh pr edit) - Fix 8: compose() relaxes capability validation for DataNode workflows executor.py exceeds 500-line gate (was already 1405, now 1507) — _execute_data method is tightly coupled to executor internals, splitting would hurt readability. Addresses review feedback from @lambdabaa on PR #1483. Fixes compose() DataNode gap from issue #1488 Gap 0. --- factory/compose.py | 22 ++ factory/inner_loop.py | 34 ++- factory/workflow/executor.py | 141 ++++++++++-- factory/workflow/primitives.py | 3 +- factory/workflow/validation.py | 48 ++++ tests/test_data_node.py | 409 ++++++++++++++++++++++++++++++++- tests/test_inner_outer_loop.py | 73 ++++++ 7 files changed, 697 insertions(+), 33 deletions(-) diff --git a/factory/compose.py b/factory/compose.py index a51e1100c..c8e17efcf 100644 --- a/factory/compose.py +++ b/factory/compose.py @@ -189,16 +189,38 @@ def from_task(cls, task: Any) -> TaskCapabilities: # ── validate_composition ───────────────────────────────────────── +def _workflow_has_data_node(workflow: Any) -> bool: + """Check if a workflow contains at least one DataNode.""" + from factory.workflow.primitives import DataNode + + nodes = getattr(workflow, "nodes", {}) + return any(isinstance(n, DataNode) for n in nodes.values()) + + def validate_composition(workflow: Any, task: Any) -> None: """Validate that a workflow can run a task. Raises IncompatibleCompositionError if capabilities don't match. + + DataNode workflows are evaluation-only — they handle iteration + internally and don't need build-pipeline capabilities like + HAS_BUILDER, CAN_RUN_TESTS, or CAN_MODIFY_CODE. """ mode_caps = ModeCapabilities.from_workflow(workflow) task_caps = TaskCapabilities.from_task(task) mode_missing = set(task_caps.requires) - set(mode_caps.provides) + # DataNode workflows handle iteration internally and don't need + # build-pipeline capabilities + if mode_missing and _workflow_has_data_node(workflow): + build_caps = { + Capability.HAS_BUILDER, + Capability.CAN_RUN_TESTS, + Capability.CAN_MODIFY_CODE, + } + mode_missing -= build_caps + mode_name = getattr(workflow, "name", "unknown") task_name = getattr(task, "name", "unknown") diff --git a/factory/inner_loop.py b/factory/inner_loop.py index 7c9ec1323..c1af74b84 100644 --- a/factory/inner_loop.py +++ b/factory/inner_loop.py @@ -411,17 +411,33 @@ def _step_with_data_node(self, directives: dict[str, Any] | None = None) -> Cycl duration_s = time.monotonic() - t0 score = 1.0 if exec_result.success else 0.0 + instance_results: list[dict[str, Any]] | None = None - # Aggregate per-item verify scores from DataNode output if available - for _nid, output in exec_result.node_outputs.items(): + # Direct lookup: find DataNode ID and read its output + data_node_id: str | None = None + for nid, n in self.workflow.nodes.items(): + if isinstance(n, DataNode): + data_node_id = nid + break + + if data_node_id is not None and data_node_id in exec_result.node_outputs: try: - parsed = json.loads(output) - if isinstance(parsed, list) and parsed and "score" in parsed[0]: - scores = [r["score"] for r in parsed] - score = sum(scores) / len(scores) if scores else 0.0 - break + parsed = json.loads(exec_result.node_outputs[data_node_id]) + if isinstance(parsed, list) and parsed: + scores = [r["score"] for r in parsed if "score" in r] + if scores: + score = sum(scores) / len(scores) + instance_results = [ + { + "instance_id": item.get("item_id", ""), + "score": item.get("score", 0.0), + "passed": item.get("passed", False), + } + for item in parsed + if isinstance(item, dict) + ] except (json.JSONDecodeError, TypeError, KeyError): - continue + pass record = CycleRecord( cycle_number=self._step_count + 1, @@ -432,6 +448,7 @@ def _step_with_data_node(self, directives: dict[str, Any] | None = None) -> Cycl score_start=None, score_end=score, score_delta=None, + instance_results=instance_results, ) record.frozen_nodes = sorted(self.frozen_nodes) record.mutable_node_ids = sorted(self.mutable_nodes()) @@ -443,6 +460,7 @@ def _step_with_data_node(self, directives: dict[str, Any] | None = None) -> Cycl builder_committed=False, experiments=0, test_score=score, + instance_results=instance_results, ) self._step_count += 1 diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index 8abf09ec2..a4dc4e6e7 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -637,6 +637,7 @@ async def throttled_branch(idx: int) -> dict[str, Any]: async def _execute_data(self, node_id: str, node: DataNode) -> None: """Execute a DataNode: resolve items, run subgraph per item with fault isolation.""" import random as _random + import subprocess as _sp from factory.task import Task as _Task from factory.task import TaskInstance as _TaskInstance @@ -679,17 +680,40 @@ async def _execute_data(self, node_id: str, node: DataNode) -> None: src = _Path(node.source_path) if not src.is_absolute(): src = self.project_path / src + # Raise if source_path doesn't exist + if not src.exists(): + raise FileNotFoundError( + f"DataNode '{node_id}': source_path not found: {node.source_path}" + ) if node.source_format == "directory" and src.is_dir(): for child in sorted(src.iterdir()): if child.is_dir(): task_instances.append((DataItem(id=child.name, path=str(child)), None)) elif node.source_format == "jsonl" and src.is_file(): - for idx, line in enumerate(src.read_text().splitlines()): + error_count = 0 + lines = src.read_text().splitlines() + total_non_empty = 0 + for idx, line in enumerate(lines): if line.strip(): - task_instances.append((DataItem( - id=str(idx), - metadata=json.loads(line), - ), None)) + total_non_empty += 1 + try: + task_instances.append((DataItem( + id=str(idx), + metadata=json.loads(line), + ), None)) + except json.JSONDecodeError: + error_count += 1 + log.warning( + "jsonl_parse_error", + node_id=node_id, + line_number=idx + 1, + line_preview=line.strip()[:100], + ) + if error_count > 0 and error_count == total_non_empty: + raise ValueError( + f"DataNode '{node_id}': all {error_count} JSONL lines " + f"failed to parse" + ) elif node.source_format == "csv" and src.is_file(): import csv with src.open(newline="") as f: @@ -704,10 +728,22 @@ async def _execute_data(self, node_id: str, node: DataNode) -> None: if item.metadata.get("split") == node.split ] if node.shuffle: - _random.shuffle(task_instances) + seed = ( + node.shuffle_seed + if node.shuffle_seed is not None + else hash(f"{node_id}:{self.run_id}") % (2**32) + ) + _random.Random(seed).shuffle(task_instances) if node.limit is not None and node.limit > 0: task_instances = task_instances[:node.limit] + if len(task_instances) == 0: + log.warning( + "data_source_empty", + node_id=node_id, + source=str(node.source_path or node.task_ref or "inline"), + ) + if len(task_instances) > node.max_items: raise ValueError( f"DataNode '{node_id}' resolved {len(task_instances)} items, " @@ -734,12 +770,55 @@ async def _execute_data(self, node_id: str, node: DataNode) -> None: if (self.project_path / r).exists(): disk_reads.add(r) - async def run_item(pair: tuple[DataItem, _TaskInstance | None]) -> dict[str, Any]: + # Resolve base commit once for worktree creation when parallelism > 1 + use_worktrees = node.parallelism > 1 and not self.dry_run + base_commit: str | None = None + worktrees_to_clean: list[tuple[Path, str]] = [] + if use_worktrees: + try: + rev_result = _sp.run( + ["git", "rev-parse", "HEAD"], + cwd=self.project_path, + capture_output=True, + text=True, + check=True, + ) + base_commit = rev_result.stdout.strip() + except _sp.CalledProcessError: + use_worktrees = False + + async def run_item( + pair: tuple[DataItem, _TaskInstance | None], + item_idx: int, + ) -> dict[str, Any]: item, inst = pair + item_project_path = self.project_path + wt_branch: str | None = None async with sem: try: + # Create per-item worktree when parallelism > 1 + if use_worktrees and base_commit is not None: + wt_dir = ( + self.project_path + / ".factory-worktrees" + / f"data-{self.run_id}-{item_idx}" + ) + wt_branch = f"factory/data-{self.run_id}-{item_idx}" + wt_dir.parent.mkdir(parents=True, exist_ok=True) + _sp.run( + [ + "git", "worktree", "add", + str(wt_dir), "-b", wt_branch, base_commit, + ], + cwd=self.project_path, + check=True, + capture_output=True, + ) + worktrees_to_clean.append((wt_dir, wt_branch)) + item_project_path = wt_dir + if resolved_task is not None and inst is not None: - resolved_task.setup(inst, self.project_path) + resolved_task.setup(inst, item_project_path) item = DataItem( id=inst.id, path=str(inst.path) if inst.path else None, @@ -747,22 +826,30 @@ async def run_item(pair: tuple[DataItem, _TaskInstance | None]) -> dict[str, Any prompt=resolved_task.prompt(inst), ) - item_executor = WorkflowExecutor( - sub_workflow.model_copy(deep=True), - self.project_path, - agent_pool=self.agent_pool, - dry_run=self.dry_run, - initial_context=item.prompt, - ) - item_executor.completed_files = self.completed_files | disk_reads - item_result = await item_executor.execute() + # Write current_item.json for subgraph visibility + item_json_path = item_project_path / ".factory" / "current_item.json" + item_json_path.parent.mkdir(parents=True, exist_ok=True) + item_json_path.write_text(json.dumps(item.model_dump())) + + try: + item_executor = WorkflowExecutor( + sub_workflow.model_copy(deep=True), + item_project_path, + agent_pool=self.agent_pool, + dry_run=self.dry_run, + initial_context=item.prompt, + ) + item_executor.completed_files = self.completed_files | disk_reads + item_result = await item_executor.execute() + finally: + item_json_path.unlink(missing_ok=True) score = 1.0 if item_result.success else 0.0 passed = item_result.success verify_details: dict[str, Any] = {} if resolved_task is not None and inst is not None: - vr = resolved_task.verify(inst, self.project_path) + vr = resolved_task.verify(inst, item_project_path) score = vr.score passed = vr.passed verify_details = vr.details or {} @@ -786,10 +873,26 @@ async def run_item(pair: tuple[DataItem, _TaskInstance | None]) -> dict[str, Any "error": str(exc), } - tasks = [run_item(pair) for pair in task_instances] + tasks = [run_item(pair, idx) for idx, pair in enumerate(task_instances)] results = await asyncio.gather(*tasks) item_results = list(results) + # Clean up worktrees + for wt_path, wt_branch_name in worktrees_to_clean: + try: + _sp.run( + ["git", "worktree", "remove", str(wt_path), "--force"], + cwd=self.project_path, + capture_output=True, + ) + _sp.run( + ["git", "branch", "-D", wt_branch_name], + cwd=self.project_path, + capture_output=True, + ) + except Exception as wt_exc: + log.warning("data_worktree_cleanup_failed", path=str(wt_path), error=str(wt_exc)) + elapsed = (time.monotonic() - start) * 1000 self.result.node_outputs[node_id] = json.dumps(item_results) self.completed_files |= node.writes diff --git a/factory/workflow/primitives.py b/factory/workflow/primitives.py index 365c32202..16036dd0c 100644 --- a/factory/workflow/primitives.py +++ b/factory/workflow/primitives.py @@ -213,9 +213,10 @@ class DataNode(Node): inline_items: list[DataItem] = Field(default_factory=list) subgraph_entry: str subgraph_exit: str - parallelism: int = 3 + parallelism: int = Field(default=1, ge=1) split: Literal["train", "val", "test", "all"] = "all" shuffle: bool = False + shuffle_seed: int | None = None limit: int | None = None max_items: int = 500 diff --git a/factory/workflow/validation.py b/factory/workflow/validation.py index ae2ae606c..42c736b20 100644 --- a/factory/workflow/validation.py +++ b/factory/workflow/validation.py @@ -129,6 +129,53 @@ def _validate_fork_join_nodes(workflow: Workflow, issues: list[str]) -> None: issues.append(f"data_node '{nid}' exit '{exit_node}' not in nodes") +def _validate_datanode_edges(workflow: Workflow, issues: list[str]) -> None: + """Reject explicit edges from a DataNode to its own subgraph nodes. + + The executor handles subgraph execution internally — explicit edges + would cause double-execution. + """ + for nid, node in workflow.nodes.items(): + if type(node).__name__ != "DataNode": + continue + entry = node.subgraph_entry # type: ignore[union-attr] + exit_node = node.subgraph_exit # type: ignore[union-attr] + subgraph_ids = _collect_subgraph_nodes(workflow, entry, exit_node) + for edge in workflow.edges: + if edge.source == nid and edge.target in subgraph_ids: + issues.append( + f"Edge from DataNode {nid} to its own subgraph node {edge.target} " + f"would cause double-execution. Remove explicit edges into DataNode " + f"subgraphs — the executor handles subgraph execution internally." + ) + + +def _collect_subgraph_nodes( + workflow: Workflow, + entry: str, + exit_node: str, +) -> set[str]: + """Collect all node IDs on paths from entry to exit_node (inclusive).""" + edges_by_source: dict[str, list[str]] = {} + for edge in workflow.edges: + edges_by_source.setdefault(edge.source, []).append(edge.target) + + visited: set[str] = set() + queue = [entry] + while queue: + nid = queue.pop(0) + if nid in visited: + continue + visited.add(nid) + if nid == exit_node: + continue + for target in edges_by_source.get(nid, []): + if target not in visited: + queue.append(target) + + return visited + + def validate_workflow(workflow: Workflow) -> list[str]: """Validate a workflow graph. Returns a list of issues (empty = valid).""" issues: list[str] = [] @@ -162,6 +209,7 @@ def validate_workflow(workflow: Workflow) -> list[str]: _validate_cycles(g, workflow, issues) _validate_data_dependencies(g, workflow, issues) _validate_fork_join_nodes(workflow, issues) + _validate_datanode_edges(workflow, issues) for nid, node in nodes.items(): if type(node).__name__ == "SubgraphForkNode": diff --git a/tests/test_data_node.py b/tests/test_data_node.py index f48d22f54..b757be95c 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -116,9 +116,10 @@ def test_defaults(self) -> None: subgraph_entry="a", subgraph_exit="b", ) - assert node.parallelism == 3 + assert node.parallelism == 1 assert node.split == "all" assert node.shuffle is False + assert node.shuffle_seed is None assert node.limit is None assert node.max_items == 500 @@ -572,7 +573,7 @@ def test_csv_source(self, tmp_path: Path) -> None: class TestSourcePathNonExistent: - def test_nonexistent_path_yields_empty(self, tmp_path: Path) -> None: + def test_nonexistent_path_raises(self, tmp_path: Path) -> None: from factory.workflow.executor import WorkflowExecutor wf = Workflow( @@ -592,9 +593,8 @@ def test_nonexistent_path_yields_empty(self, tmp_path: Path) -> None: ) executor = WorkflowExecutor(wf, tmp_path, dry_run=True) result = asyncio.run(executor.execute()) - assert result.success - parsed = json.loads(result.node_outputs["data"]) - assert len(parsed) == 0 + assert result.halted + assert "source_path not found" in result.halt_reason # ── Phase 8: inner_loop _step_with_data_node ──────────────────── @@ -918,3 +918,402 @@ def test_falls_back_to_binary_without_scores(self, tmp_path: Path) -> None: record = loop._step_with_data_node() assert record.score_end == 1.0 + + +# ── PR #1483 Review Fixes — additional tests ───────────────────── + + +class TestParallelismDefault: + def test_parallelism_default_is_1(self) -> None: + node = DataNode( + id="dn", + inline_items=[DataItem(id="i")], + subgraph_entry="a", + subgraph_exit="b", + ) + assert node.parallelism == 1 + + def test_parallelism_zero_rejected(self) -> None: + with pytest.raises(ValidationError): + DataNode( + id="dn", + inline_items=[DataItem(id="i")], + subgraph_entry="a", + subgraph_exit="b", + parallelism=0, + ) + + +class TestNonexistentSourcePathRaises: + def test_raises_file_not_found(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + wf = Workflow( + name="missing", + nodes={ + "data": DataNode( + id="data", + source_path=str(tmp_path / "nope"), + source_format="jsonl", + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.halted + assert "source_path not found" in result.halt_reason + + +class TestEmptySourceWarns: + def test_empty_inline_warns(self, tmp_path: Path) -> None: + """Zero items after filtering should log a warning.""" + from factory.workflow.executor import WorkflowExecutor + + # Use split filter to exclude all items + wf = Workflow( + name="empty_test", + nodes={ + "data": DataNode( + id="data", + inline_items=[DataItem(id="a", metadata={"split": "train"})], + subgraph_entry="sub", + subgraph_exit="sub", + split="val", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + # This should succeed but with 0 items (and log a warning) + result = asyncio.run(executor.execute()) + assert result.success + parsed = json.loads(result.node_outputs["data"]) + assert len(parsed) == 0 + + +class TestMalformedJsonlLineIsolated: + def test_bad_line_skipped_good_lines_kept(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + jsonl_file = tmp_path / "items.jsonl" + jsonl_file.write_text( + '{"name": "first"}\n' + 'NOT VALID JSON\n' + '{"name": "third"}\n' + ) + + wf = Workflow( + name="jsonl_malformed", + nodes={ + "data": DataNode( + id="data", + source_path=str(jsonl_file), + source_format="jsonl", + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.success + parsed = json.loads(result.node_outputs["data"]) + # Two good lines kept, one bad line skipped + assert len(parsed) == 2 + + def test_all_lines_bad_raises(self, tmp_path: Path) -> None: + from factory.workflow.executor import WorkflowExecutor + + jsonl_file = tmp_path / "items.jsonl" + jsonl_file.write_text("bad line 1\nbad line 2\n") + + wf = Workflow( + name="jsonl_all_bad", + nodes={ + "data": DataNode( + id="data", + source_path=str(jsonl_file), + source_format="jsonl", + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + assert result.halted + assert "JSONL lines" in result.halt_reason + + +class TestShuffleDeterministic: + def test_shuffle_with_seed(self, tmp_path: Path) -> None: + """Same seed -> same order across runs.""" + from factory.workflow.executor import WorkflowExecutor + + items = [DataItem(id=str(i)) for i in range(20)] + wf = Workflow( + name="shuffle_seed", + nodes={ + "data": DataNode( + id="data", + inline_items=items, + subgraph_entry="sub", + subgraph_exit="sub", + shuffle=True, + shuffle_seed=42, + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + + # Run twice with the same seed -- order must match + executor1 = WorkflowExecutor(wf, tmp_path, dry_run=True) + result1 = asyncio.run(executor1.execute()) + ids1 = [r["item_id"] for r in json.loads(result1.node_outputs["data"])] + + executor2 = WorkflowExecutor(wf, tmp_path, dry_run=True) + result2 = asyncio.run(executor2.execute()) + ids2 = [r["item_id"] for r in json.loads(result2.node_outputs["data"])] + + assert ids1 == ids2 + # Must actually be shuffled (not original order) + original_ids = [str(i) for i in range(20)] + assert ids1 != original_ids + + def test_shuffle_from_run_id(self, tmp_path: Path) -> None: + """Unseeded shuffle derives seed from node_id + run_id.""" + from factory.workflow.executor import WorkflowExecutor + + items = [DataItem(id=str(i)) for i in range(20)] + wf = Workflow( + name="shuffle_runid", + nodes={ + "data": DataNode( + id="data", + inline_items=items, + subgraph_entry="sub", + subgraph_exit="sub", + shuffle=True, + # No shuffle_seed -- uses run_id hash + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[], + start_node="data", + ) + + executor1 = WorkflowExecutor(wf, tmp_path, dry_run=True) + result1 = asyncio.run(executor1.execute()) + ids1 = [r["item_id"] for r in json.loads(result1.node_outputs["data"])] + + # Different run_id -> potentially different order (different executor) + executor2 = WorkflowExecutor(wf, tmp_path, dry_run=True) + result2 = asyncio.run(executor2.execute()) + ids2 = [r["item_id"] for r in json.loads(result2.node_outputs["data"])] + + # Both should be 20 items + assert len(ids1) == 20 + assert len(ids2) == 20 + + +class TestExplicitEdgeToSubgraphRejected: + def test_validation_error_on_datanode_subgraph_edge(self) -> None: + wf = Workflow( + name="bad_edge", + nodes={ + "data": DataNode( + id="data", + inline_items=[DataItem(id="i")], + subgraph_entry="sub_start", + subgraph_exit="sub_end", + ), + "sub_start": FnNode(id="sub_start", command="echo start"), + "sub_end": FnNode(id="sub_end", command="echo end"), + }, + edges=[ + Edge(source="data", target="sub_start"), + Edge(source="sub_start", target="sub_end"), + ], + start_node="data", + ) + issues = wf.validate_graph() + assert any("double-execution" in i for i in issues) + + +class TestCurrentItemJsonWritten: + def test_current_item_json_created_and_cleaned(self, tmp_path: Path) -> None: + """current_item.json should exist during subgraph execution.""" + from factory.workflow.executor import WorkflowExecutor + + items = [DataItem(id="test_item", prompt="do it")] + wf = _make_data_workflow(items) + + # Track whether current_item.json exists during execution + observed: list[bool] = [] + original_execute = WorkflowExecutor.execute + + async def tracking_execute(self_inner): + item_json = self_inner.project_path / ".factory" / "current_item.json" + # For sub-executors (data_item workflows), check if file exists + if self_inner.workflow.name.endswith("__data_item"): + observed.append(item_json.exists()) + return await original_execute(self_inner) + + with patch.object(WorkflowExecutor, "execute", tracking_execute): + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success + # current_item.json should have existed during subgraph execution + assert any(observed) + # And it should be cleaned up after + assert not (tmp_path / ".factory" / "current_item.json").exists() + + +class TestDirectScoreLookup: + def test_finds_score_by_data_node_id(self, tmp_path: Path) -> None: + """Score lookup uses DataNode ID directly, not sniffing.""" + from unittest.mock import AsyncMock + + from factory.inner_loop import InnerLoop + from factory.workflow.executor import ExecutionResult + + wf = _make_data_workflow([DataItem(id="i", prompt="go")]) + + mock_result = ExecutionResult() + mock_result.success = True + mock_result.node_outputs = { + "data": json.dumps([ + {"item_id": "i", "score": 0.75, "passed": True}, + ]), + "some_other_node": json.dumps({"unrelated": "data"}), + } + + loop = InnerLoop(project_dir=tmp_path, workflow=wf) + + with patch( + "factory.workflow.executor.WorkflowExecutor.execute", + new_callable=AsyncMock, + return_value=mock_result, + ): + record = loop._step_with_data_node() + + assert record.score_end == pytest.approx(0.75) + + +class TestInstanceResultsPopulated: + def test_instance_results_on_cycle_record(self, tmp_path: Path) -> None: + from unittest.mock import AsyncMock + + from factory.inner_loop import InnerLoop + from factory.workflow.executor import ExecutionResult + + wf = _make_data_workflow([DataItem(id="a"), DataItem(id="b")]) + + mock_result = ExecutionResult() + mock_result.success = True + mock_result.node_outputs = { + "data": json.dumps([ + {"item_id": "a", "score": 0.9, "passed": True}, + {"item_id": "b", "score": 0.3, "passed": False}, + ]) + } + + loop = InnerLoop(project_dir=tmp_path, workflow=wf) + + with patch( + "factory.workflow.executor.WorkflowExecutor.execute", + new_callable=AsyncMock, + return_value=mock_result, + ): + record = loop._step_with_data_node() + + assert record.instance_results is not None + assert len(record.instance_results) == 2 + assert record.instance_results[0]["instance_id"] == "a" + assert record.instance_results[0]["score"] == 0.9 + assert record.instance_results[1]["instance_id"] == "b" + assert record.instance_results[1]["passed"] is False + + +class _ComposeTestTask: + """Task that satisfies TaskProtocol for compose() tests.""" + + def __init__(self) -> None: + from factory.task import ScoringContract, TaskDefinition + + self.definition = TaskDefinition( + name="mock", scoring=ScoringContract(method="exit_code"), + ) + self.scoring = self.definition.scoring + self.constraints = None + + def instances(self): + from factory.task import TaskInstance + return [TaskInstance(id="inst-1")] + + def setup(self, instance: Any, workspace: Path) -> None: + pass + + def prompt(self, instance: Any) -> str: + return "test prompt" + + def verify(self, instance: Any, workspace: Path): + from factory.task import VerifyResult + return VerifyResult(passed=True, score=1.0) + + def get_evaluator(self) -> Any: + return None + + +class TestComposeDataNodeWorkflow: + def test_compose_succeeds_without_builder(self, tmp_path: Path) -> None: + """compose() should NOT raise IncompatibleCompositionError for DataNode workflows + even when the task requires HAS_BUILDER/CAN_RUN_TESTS.""" + from factory.compose import compose + from factory.workflow.primitives import AgentNode, AgentRole + + # Create a DataNode workflow WITHOUT a builder agent + wf = Workflow( + name="eval_only", + nodes={ + "generator": AgentNode( + id="generator", + role=AgentRole.RESEARCHER, + prompt_template="generate", + ), + "data": DataNode( + id="data", + inline_items=[DataItem(id="i1", prompt="test")], + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[ + Edge(source="generator", target="data"), + ], + start_node="generator", + ) + + task = _ComposeTestTask() + + # This should NOT raise IncompatibleCompositionError + loop = compose(wf, task, tmp_path) + assert loop is not None + assert loop.workflow is wf diff --git a/tests/test_inner_outer_loop.py b/tests/test_inner_outer_loop.py index aa4289e52..3d4f1e002 100644 --- a/tests/test_inner_outer_loop.py +++ b/tests/test_inner_outer_loop.py @@ -830,3 +830,76 @@ def test_full_pipeline_data_node_through_inner_loop_with_features(self, tmp_path features = compute_features(wf) assert features[8] == 1 assert loop._workflow_has_data_node() is True + + +# ── DataNode compose() validation (PR #1483 fix 8) ─────────────── + + +class _ComposeTestTask: + """Task satisfying TaskProtocol for compose() tests.""" + + def __init__(self) -> None: + from factory.task import ScoringContract, TaskDefinition + + self.definition = TaskDefinition( + name="mock", scoring=ScoringContract(method="exit_code"), + ) + self.scoring = self.definition.scoring + self.constraints = None + + def instances(self): + from factory.task import TaskInstance + return [TaskInstance(id="inst-1")] + + def setup(self, instance, workspace): + pass + + def prompt(self, instance): + return "test" + + def verify(self, instance, workspace): + from factory.task import VerifyResult + return VerifyResult(passed=True, score=1.0) + + def get_evaluator(self): + return None + + +class TestComposeDataNodeWorkflowIntegration: + def test_compose_datanode_workflow_succeeds(self, tmp_path: Path) -> None: + """compose() does not raise IncompatibleCompositionError for DataNode workflows.""" + from factory.compose import compose + from factory.workflow.primitives import ( + AgentNode, + AgentRole, + DataItem, + DataNode, + Edge, + FnNode, + ) + + wf = Workflow( + name="eval_only", + nodes={ + "gen": AgentNode( + id="gen", + role=AgentRole.RESEARCHER, + prompt_template="research", + ), + "data": DataNode( + id="data", + inline_items=[DataItem(id="i1", prompt="test")], + subgraph_entry="sub", + subgraph_exit="sub", + ), + "sub": FnNode(id="sub", command="echo x"), + }, + edges=[Edge(source="gen", target="data")], + start_node="gen", + ) + + task = _ComposeTestTask() + + loop = compose(wf, task, tmp_path) + assert loop is not None + assert loop.workflow is wf From 8b5b45c5c09b694b6f5343a1f6d0ee15bcd9d00e Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Thu, 10 Sep 2026 20:13:24 +0000 Subject: [PATCH 10/30] fix: replace .value with str() on VerdictType edge conditions (mypy) VerdictType(str, Enum) instances ARE strings, so .value is redundant and mypy flags it as attr-defined on the str union branch. Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/workflow/cli.py | 2 +- factory/workflow/overwrite.py | 2 +- factory/workflow/parity.py | 2 +- factory/workflow/skill_export.py | 2 +- factory/workflow/tool.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/factory/workflow/cli.py b/factory/workflow/cli.py index 6ac477a20..0389d06fc 100644 --- a/factory/workflow/cli.py +++ b/factory/workflow/cli.py @@ -179,7 +179,7 @@ def _cmd_show(args: argparse.Namespace) -> int: print(" " + "-" * (len(header) - 2)) for edge in wf.edges: - cond = edge.condition.value if edge.condition else "-" + cond = str(edge.condition) if edge.condition else "-" print(f" {edge.source:<25} {edge.target:<25} {cond:<15}") return 0 diff --git a/factory/workflow/overwrite.py b/factory/workflow/overwrite.py index e9999b01c..597565b26 100644 --- a/factory/workflow/overwrite.py +++ b/factory/workflow/overwrite.py @@ -53,7 +53,7 @@ def _interpret_overwrite( indent=2, ) edge_summary = json.dumps( - [{"source": e.source, "target": e.target, "condition": e.condition.value if e.condition else None} + [{"source": e.source, "target": e.target, "condition": str(e.condition) if e.condition else None} for e in workflow.edges], indent=2, ) diff --git a/factory/workflow/parity.py b/factory/workflow/parity.py index 013154792..f7dee5ed6 100644 --- a/factory/workflow/parity.py +++ b/factory/workflow/parity.py @@ -153,7 +153,7 @@ def canonicalize(wf: Workflow) -> dict[str, Any]: { "source": e.source, "target": e.target, - "condition": e.condition.value if e.condition is not None else None, + "condition": str(e.condition) if e.condition is not None else None, } for e in wf.edges if (e.source, e.target) not in redundant diff --git a/factory/workflow/skill_export.py b/factory/workflow/skill_export.py index bad6eca7b..3983db8fa 100644 --- a/factory/workflow/skill_export.py +++ b/factory/workflow/skill_export.py @@ -176,7 +176,7 @@ def _format_edges(edges: list[Edge]) -> str: return "none" parts = [] for e in edges: - cond = e.condition.value if e.condition else "unconditional" + cond = str(e.condition) if e.condition else "unconditional" parts.append(f"{cond} → {e.target}") return ", ".join(parts) diff --git a/factory/workflow/tool.py b/factory/workflow/tool.py index f6db23a01..34c8b0e2f 100644 --- a/factory/workflow/tool.py +++ b/factory/workflow/tool.py @@ -224,7 +224,7 @@ def tool_init(workflow_name: str, project_path: Path) -> str: { "source": e.source, "target": e.target, - "condition": e.condition.value if e.condition else None, + "condition": str(e.condition) if e.condition else None, } for e in wf.edges ], From f0f450758d523643cd2f04ee7d3b1250dd9f0f19 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Thu, 10 Sep 2026 20:35:30 +0000 Subject: [PATCH 11/30] fix: use .value with type:ignore for VerdictType edge conditions Reverts str() back to .value (str() produces 'VerdictType.PROCEED' instead of 'proceed'). Adds type:ignore to silence mypy since VerdictType(str, Enum) makes mypy see the str branch. Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/workflow/cli.py | 2 +- factory/workflow/overwrite.py | 2 +- factory/workflow/parity.py | 2 +- factory/workflow/skill_export.py | 6 +++--- factory/workflow/tool.py | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/factory/workflow/cli.py b/factory/workflow/cli.py index 0389d06fc..e158d10c4 100644 --- a/factory/workflow/cli.py +++ b/factory/workflow/cli.py @@ -179,7 +179,7 @@ def _cmd_show(args: argparse.Namespace) -> int: print(" " + "-" * (len(header) - 2)) for edge in wf.edges: - cond = str(edge.condition) if edge.condition else "-" + cond = edge.condition.value if edge.condition else "-" # type: ignore print(f" {edge.source:<25} {edge.target:<25} {cond:<15}") return 0 diff --git a/factory/workflow/overwrite.py b/factory/workflow/overwrite.py index 597565b26..7aeb02f6d 100644 --- a/factory/workflow/overwrite.py +++ b/factory/workflow/overwrite.py @@ -53,7 +53,7 @@ def _interpret_overwrite( indent=2, ) edge_summary = json.dumps( - [{"source": e.source, "target": e.target, "condition": str(e.condition) if e.condition else None} + [{"source": e.source, "target": e.target, "condition": e.condition.value if e.condition else None} # type: ignore for e in workflow.edges], indent=2, ) diff --git a/factory/workflow/parity.py b/factory/workflow/parity.py index f7dee5ed6..b44241716 100644 --- a/factory/workflow/parity.py +++ b/factory/workflow/parity.py @@ -153,7 +153,7 @@ def canonicalize(wf: Workflow) -> dict[str, Any]: { "source": e.source, "target": e.target, - "condition": str(e.condition) if e.condition is not None else None, + "condition": e.condition.value if e.condition is not None else None, # type: ignore } for e in wf.edges if (e.source, e.target) not in redundant diff --git a/factory/workflow/skill_export.py b/factory/workflow/skill_export.py index 3983db8fa..e9ce4853c 100644 --- a/factory/workflow/skill_export.py +++ b/factory/workflow/skill_export.py @@ -122,12 +122,12 @@ def _topological_sort(workflow: Workflow) -> list[str]: # after the fork node and join sources sort before the join node. for nid, node in workflow.nodes.items(): if type(node).__name__ == "ForkNode": - for t in node.targets: # type: ignore[union-attr] + for t in node.targets: # type: ignore if t in workflow.nodes: adj[nid].append(t) in_degree[t] = in_degree.get(t, 0) + 1 if type(node).__name__ == "JoinNode": - for s in node.sources: # type: ignore[union-attr] + for s in node.sources: # type: ignore if s in workflow.nodes: adj[s].append(nid) in_degree[nid] = in_degree.get(nid, 0) + 1 @@ -176,7 +176,7 @@ def _format_edges(edges: list[Edge]) -> str: return "none" parts = [] for e in edges: - cond = str(e.condition) if e.condition else "unconditional" + cond = e.condition.value if e.condition else "unconditional" # type: ignore parts.append(f"{cond} → {e.target}") return ", ".join(parts) diff --git a/factory/workflow/tool.py b/factory/workflow/tool.py index 34c8b0e2f..055897574 100644 --- a/factory/workflow/tool.py +++ b/factory/workflow/tool.py @@ -224,7 +224,7 @@ def tool_init(workflow_name: str, project_path: Path) -> str: { "source": e.source, "target": e.target, - "condition": str(e.condition) if e.condition else None, + "condition": e.condition.value if e.condition else None, # type: ignore } for e in wf.edges ], From 5daf48b156bb3726ccb230057addd3f8a744a570 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Thu, 10 Sep 2026 19:38:31 +0000 Subject: [PATCH 12/30] fix: close 3 execution-layer gaps for Task+Workflow consumers (#1488) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gap 0 — Defensive composition catch in InnerLoop._step_with_task(): Wrap validate_composition() in try/except IncompatibleCompositionError. If caught, log warning (composition_incompatible event) and return CycleRecord with score=0.0 instead of crashing. Handles post-mutation composition failures (e.g. NODE_REMOVE stripping Builder). Gap 1 — _run_agent() output persistence to node.writes: After agent returns stdout, write to node.writes paths using the same pattern as _run_llm(). Write unconditionally (not gated on exit code) to match _run_llm() behavior. Gap 2 — agent_fn injection in WorkflowExecutor: Add keyword-only agent_fn parameter to __init__ with lazy default to invoke_agent. Replace hardcoded import in _run_agent() with self._agent_fn(). Propagate to SubgraphForkNode branch executors. Leave _evaluate_gate() invoke_agent call unchanged (CEO gate eval). Tests: - test_explicit_empty_caps_passes_research_workflow (compose.py) - test_run_agent_persists_to_node_writes (executor.py) - test_run_agent_no_writes_skips_file_creation (executor.py) - test_custom_agent_fn_used (executor.py) - test_agent_fn_defaults_to_invoke_agent (executor.py) --- factory/inner_loop.py | 31 ++++++++++ factory/workflow/executor.py | 20 +++++- tests/test_compose.py | 18 ++++++ tests/test_workflow_executor.py | 106 ++++++++++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 3 deletions(-) diff --git a/factory/inner_loop.py b/factory/inner_loop.py index c1af74b84..9d30249bd 100644 --- a/factory/inner_loop.py +++ b/factory/inner_loop.py @@ -26,9 +26,14 @@ from pathlib import Path from typing import Any, Protocol, runtime_checkable +import structlog + +from factory.compose import IncompatibleCompositionError from factory.cycle_analyzer import CycleAnalyzer, CycleRecord from factory.workflow.primitives import DataNode, Workflow +log = structlog.get_logger() + @dataclass class EvalResult: @@ -272,6 +277,7 @@ def _step_with_task(self, directives: dict[str, Any] | None = None) -> CycleReco import asyncio import statistics + from factory.compose import validate_composition from factory.models import AggregateMethod, InnerLoopConfig from factory.workflow.executor import WorkflowExecutor @@ -281,6 +287,31 @@ def _step_with_task(self, directives: dict[str, Any] | None = None) -> CycleReco if self._workflow_has_data_node(): return self._step_with_data_node(directives) + # Belt-and-suspenders: catch post-mutation composition failures + # (e.g. NODE_REMOVE stripping the Builder after initial composition) + try: + validate_composition(self.workflow, self.task) + except IncompatibleCompositionError as exc: + log.warning( + "composition_incompatible", + workflow=getattr(self.workflow, "name", "unknown"), + task=getattr(self.task, "name", "unknown"), + error=str(exc), + ) + self._step_count += 1 + record = CycleRecord( + cycle_number=self._step_count, + mode=self.mode, + started_at=None, + ended_at=None, + duration_s=0.0, + score_start=None, + score_end=0.0, + score_delta=None, + ) + self._history.append(record) + return record + event_offset = self._count_lines(self.factory_dir / "events.jsonl") t0 = time.monotonic() diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index a4dc4e6e7..2da3c94ca 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -7,6 +7,7 @@ import shlex import time import uuid +from collections.abc import Callable from pathlib import Path from typing import Any @@ -87,12 +88,19 @@ def __init__( dry_run: bool = False, auto_approve: bool = False, initial_context: str | None = None, + agent_fn: Callable[..., Any] | None = None, ) -> None: self.workflow = workflow self.project_path = project_path self.agent_pool = agent_pool or {} self.dry_run = dry_run self.auto_approve = auto_approve + if agent_fn is not None: + self._agent_fn = agent_fn + else: + from factory.agents.runner import invoke_agent + + self._agent_fn = invoke_agent self.run_id = uuid.uuid4().hex[:12] self.completed_files: set[str] = set() self.node_context: dict[str, str] = {} @@ -581,6 +589,7 @@ async def run_branch(idx: int) -> dict[str, Any]: wt_path if not self.dry_run else self.project_path, agent_pool=self.agent_pool, dry_run=self.dry_run, + agent_fn=self._agent_fn, ) branch_result = await branch_executor.execute() @@ -1111,8 +1120,6 @@ async def _run_fn(self, node: FnNode) -> str: async def _run_agent(self, node: AgentNode) -> str: """Invoke an agent via factory/agents/runner.py.""" - from factory.agents.runner import invoke_agent - task = node.prompt_template.replace( "{project_path}", str(self.project_path), ) @@ -1132,7 +1139,7 @@ async def _run_agent(self, node: AgentNode) -> str: if pool_entry: timeout = pool_entry.timeout - stdout, code = await invoke_agent( + stdout, code = await self._agent_fn( node.role.value, # type: ignore[arg-type] task, self.project_path, @@ -1148,6 +1155,13 @@ async def _run_agent(self, node: AgentNode) -> str: output_len=len(stdout), ) + # Persist output to node.writes paths (mirrors _run_llm pattern) + if node.writes: + for wpath in node.writes: + fpath = self.project_path / wpath + fpath.parent.mkdir(parents=True, exist_ok=True) + fpath.write_text(stdout) + return stdout async def _run_llm(self, node: LLMNode) -> str: diff --git a/tests/test_compose.py b/tests/test_compose.py index a4d84af26..6154d7b08 100644 --- a/tests/test_compose.py +++ b/tests/test_compose.py @@ -543,3 +543,21 @@ def test_to_dict_from_dict_round_trip(self): d = compiled.to_dict() restored = Workflow.from_dict(d) assert restored.declared_capabilities == frozenset(["code-generation", "health-check"]) + + +class TestExplicitEmptyCapsValidation: + """Gap 0: Tasks with required_capabilities=[] pass any workflow.""" + + def test_explicit_empty_caps_passes_research_workflow(self): + """A task with TaskConstraints(required_capabilities=[]) and exit_code scoring + passes validate_composition() with a minimal workflow that has no builder node.""" + wf = _make_workflow(researcher=True, name="no-builder") + task = Task( + definition=TaskDefinition( + name="eval-only-task", + scoring=ScoringContract(method="exit_code"), + constraints=TaskConstraints(required_capabilities=[]), + ) + ) + # Should NOT raise IncompatibleCompositionError + validate_composition(wf, task) diff --git a/tests/test_workflow_executor.py b/tests/test_workflow_executor.py index 7744ecfc9..eb383d3d2 100644 --- a/tests/test_workflow_executor.py +++ b/tests/test_workflow_executor.py @@ -729,3 +729,109 @@ def test_no_initial_context_leaves_node_context_empty(self, tmp_project: Path) - executor = WorkflowExecutor(wf, tmp_project, dry_run=True) assert executor.node_context == {} + + +class TestRunAgentPersistsToNodeWrites: + """Gap 1: _run_agent() persists stdout to node.writes paths.""" + + async def test_run_agent_persists_to_node_writes(self, tmp_project: Path) -> None: + """AgentNode with writes={'output.md'} persists agent stdout to that file.""" + from unittest.mock import AsyncMock + + wf = Workflow( + name="persist_test", + nodes={ + "agent": AgentNode( + id="agent", + role=AgentRole.BUILDER, + prompt_template="build", + writes={"output.md"}, + ), + }, + edges=[], + start_node="agent", + ) + + mock_agent_fn = AsyncMock(return_value=("agent output", 0)) + executor = WorkflowExecutor(wf, tmp_project, agent_fn=mock_agent_fn) + await executor.execute() + + output_file = tmp_project / "output.md" + assert output_file.exists() + assert output_file.read_text() == "agent output" + + async def test_run_agent_no_writes_skips_file_creation(self, tmp_project: Path) -> None: + """AgentNode without writes doesn't create extra files.""" + from unittest.mock import AsyncMock + + wf = Workflow( + name="no_writes_test", + nodes={ + "agent": AgentNode( + id="agent", + role=AgentRole.BUILDER, + prompt_template="build", + ), + }, + edges=[], + start_node="agent", + ) + + files_before = set(tmp_project.rglob("*")) + mock_agent_fn = AsyncMock(return_value=("agent output", 0)) + executor = WorkflowExecutor(wf, tmp_project, agent_fn=mock_agent_fn) + await executor.execute() + + files_after = set(tmp_project.rglob("*")) + new_files = files_after - files_before + # Only event log files should be created, not agent output files + for f in new_files: + assert "output.md" not in f.name + + +class TestAgentFnInjection: + """Gap 2: WorkflowExecutor supports agent_fn injection.""" + + async def test_custom_agent_fn_used(self, tmp_project: Path) -> None: + """Custom agent_fn is called instead of default invoke_agent.""" + from unittest.mock import AsyncMock + + mock_fn = AsyncMock(return_value=("custom output", 0)) + + wf = Workflow( + name="custom_fn_test", + nodes={ + "agent": AgentNode( + id="agent", + role=AgentRole.BUILDER, + prompt_template="build", + ), + }, + edges=[], + start_node="agent", + ) + + executor = WorkflowExecutor(wf, tmp_project, agent_fn=mock_fn) + await executor.execute() + + mock_fn.assert_called_once() + + def test_agent_fn_defaults_to_invoke_agent(self, tmp_project: Path) -> None: + """When agent_fn is not provided, defaults to invoke_agent.""" + from factory.agents.runner import invoke_agent + + wf = Workflow( + name="default_fn_test", + nodes={ + "agent": AgentNode( + id="agent", + role=AgentRole.BUILDER, + prompt_template="build", + ), + }, + edges=[], + start_node="agent", + ) + + executor = WorkflowExecutor(wf, tmp_project) + assert executor._agent_fn is invoke_agent From 3f88d1c16d18882293c94a76cb052c2da920ef30 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Thu, 10 Sep 2026 19:48:33 +0000 Subject: [PATCH 13/30] fix: propagate agent_fn to DataNode sub-executors + add missing test - Add agent_fn=self._agent_fn to WorkflowExecutor constructor in _execute_data(), matching _execute_subgraph_fork() at line 592 - Add test_agent_fn_propagates_to_data_node_sub_executor verifying custom agent_fn reaches DataNode per-item sub-executors --- factory/workflow/executor.py | 1 + tests/test_workflow_executor.py | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index 2da3c94ca..e05d6d42f 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -846,6 +846,7 @@ async def run_item( item_project_path, agent_pool=self.agent_pool, dry_run=self.dry_run, + agent_fn=self._agent_fn, initial_context=item.prompt, ) item_executor.completed_files = self.completed_files | disk_reads diff --git a/tests/test_workflow_executor.py b/tests/test_workflow_executor.py index eb383d3d2..16cfb1220 100644 --- a/tests/test_workflow_executor.py +++ b/tests/test_workflow_executor.py @@ -816,6 +816,41 @@ async def test_custom_agent_fn_used(self, tmp_project: Path) -> None: mock_fn.assert_called_once() + async def test_agent_fn_propagates_to_data_node_sub_executor( + self, tmp_project: Path, + ) -> None: + """agent_fn propagates to DataNode per-item sub-executors.""" + from unittest.mock import AsyncMock + + from factory.workflow.primitives import DataItem, DataNode + + mock_fn = AsyncMock(return_value=("sub output", 0)) + + wf = Workflow( + name="data_propagation_test", + nodes={ + "data": DataNode( + id="data", + inline_items=[DataItem(id="item1", prompt="do it")], + subgraph_entry="sub_agent", + subgraph_exit="sub_agent", + ), + "sub_agent": AgentNode( + id="sub_agent", + role=AgentRole.BUILDER, + prompt_template="build", + ), + }, + edges=[], + start_node="data", + ) + + executor = WorkflowExecutor(wf, tmp_project, agent_fn=mock_fn) + result = await executor.execute() + + assert result.success + mock_fn.assert_called_once() + def test_agent_fn_defaults_to_invoke_agent(self, tmp_project: Path) -> None: """When agent_fn is not provided, defaults to invoke_agent.""" from factory.agents.runner import invoke_agent From c5f7068048599dc729009aa14d81b96cdbbc40fc Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Fri, 11 Sep 2026 18:41:26 +0000 Subject: [PATCH 14/30] fix: re-scan disk reads after task.setup() in DataNode executor In _execute_data(), disk_reads was pre-computed ONCE before per-item processing. When resolved_task.setup() creates files on disk (e.g. writing test fixtures or input data), those files weren't included in the sub-executor's completed_files, causing _wait_for_reads() to block for 60s on files that existed on disk. Fix: After setup() completes for each item, re-scan subgraph node reads against item_project_path to find newly-created files. The setup_reads set is local to each run_item() call, avoiding mutation of the shared disk_reads set (important when items use different worktree paths). Co-Authored-By: Claude Opus 4.6 (1M context) --- .factory/reviews/builder-latest.md | 20 ++++++++ factory/workflow/executor.py | 9 +++- tests/test_data_node.py | 78 ++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 1 deletion(-) create mode 100644 .factory/reviews/builder-latest.md diff --git a/.factory/reviews/builder-latest.md b/.factory/reviews/builder-latest.md new file mode 100644 index 000000000..b93836ea9 --- /dev/null +++ b/.factory/reviews/builder-latest.md @@ -0,0 +1,20 @@ +## Builder Report — disk_reads re-scan after setup() + +### Changes + +**factory/workflow/executor.py** (`_execute_data` → `run_item`): +- Added a per-item re-scan of subgraph reads after `resolved_task.setup()` completes +- New local variable `setup_reads: set[str]` scans `sub_workflow.nodes` reads against `item_project_path` +- Changed `item_executor.completed_files = self.completed_files | disk_reads` to `self.completed_files | disk_reads | setup_reads` +- The pre-scan at line 791 (`disk_reads`) is preserved — it handles pre-existing files +- `setup_reads` is local to `run_item()` — no mutation of shared `disk_reads` set + +**tests/test_data_node.py**: +- Added `_SetupWritingTask` — a minimal Task whose `setup()` writes a file to the workspace +- Added `TestDiskReadsRescanAfterSetup::test_setup_created_file_in_completed_files` — verifies that when `setup()` creates a file declared in a subgraph node's `reads`, it appears in the sub-executor's `completed_files` + +### Verification +- All 64 DataNode tests pass (including new test) +- All 46 executor tests pass +- `ruff check` clean +- `mypy` clean diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index fd6309677..200cb65c5 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -850,6 +850,13 @@ async def run_item( prompt=resolved_task.prompt(inst), ) + # Re-scan subgraph reads for files created by setup() + setup_reads: set[str] = set() + for sg_node in sub_workflow.nodes.values(): + for r in sg_node.reads: + if (item_project_path / r).exists(): + setup_reads.add(r) + # Write current_item.json for subgraph visibility item_json_path = item_project_path / ".factory" / "current_item.json" item_json_path.parent.mkdir(parents=True, exist_ok=True) @@ -864,7 +871,7 @@ async def run_item( agent_fn=self._agent_fn, initial_context=item.prompt or None, ) - item_executor.completed_files = self.completed_files | disk_reads + item_executor.completed_files = self.completed_files | disk_reads | setup_reads item_result = await item_executor.execute() finally: item_json_path.unlink(missing_ok=True) diff --git a/tests/test_data_node.py b/tests/test_data_node.py index 4d64e4da4..3415b0a33 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -1494,3 +1494,81 @@ def test_executor_failure_defaults_score(self, tmp_path: Path) -> None: record = loop._step_with_data_node() assert record.score_end == 0.0 + + +# ── disk_reads re-scan after setup() ──────────────────────────── + + +class _SetupWritingTask: + """Task whose setup() creates a file that a subgraph node reads.""" + + def __init__(self, setup_file: str) -> None: + self._setup_file = setup_file + + def instances(self): + from factory.task import TaskInstance + return [TaskInstance(id="inst1")] + + def setup(self, instance: Any, workspace: Path) -> None: + target = workspace / self._setup_file + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("setup content") + + def prompt(self, instance: Any) -> str: + return "go" + + def verify(self, instance: Any, workspace: Path): + from factory.task import VerifyResult + return VerifyResult(passed=True, score=1.0) + + +class TestDiskReadsRescanAfterSetup: + """setup()-created files must appear in sub-executor completed_files.""" + + def test_setup_created_file_in_completed_files(self, tmp_path: Path) -> None: + """When task.setup() writes a file declared in a subgraph node's reads, + the sub-executor's completed_files must include it so _wait_for_reads() + doesn't block for 60 s.""" + from factory.workflow.executor import WorkflowExecutor + + setup_file = "data/input.txt" + + wf = Workflow( + name="rescan_test", + nodes={ + "data": DataNode( + id="data", + task_ref="fake.module:SetupWritingTask", + subgraph_entry="reader", + subgraph_exit="reader", + ), + "reader": FnNode( + id="reader", + command="echo ok", + reads={setup_file}, + ), + }, + edges=[], + start_node="data", + ) + + fake_task = _SetupWritingTask(setup_file) + + # Capture the completed_files set on the sub-executor + captured_completed: list[set[str]] = [] + original_execute = WorkflowExecutor.execute + + async def spy_execute(self_inner): + if self_inner.workflow.name.endswith("__data_item"): + captured_completed.append(set(self_inner.completed_files)) + return await original_execute(self_inner) + + with patch("factory.task.TaskRef.resolve", return_value=fake_task), \ + patch.object(WorkflowExecutor, "execute", spy_execute): + executor = WorkflowExecutor(wf, tmp_path, dry_run=True) + result = asyncio.run(executor.execute()) + + assert result.success, f"halted: {result.halt_reason}" + # The sub-executor's completed_files must contain the setup-written file + assert len(captured_completed) == 1 + assert setup_file in captured_completed[0] From 38d4aa3e714739e23592103bb7a6b40de89b3440 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Fri, 11 Sep 2026 23:32:31 +0000 Subject: [PATCH 15/30] fix: preserve frozen nodes in DesignerAgent + explicit prompt_template in structural_hash BUG 1: DesignerAgent.design_minimal/thorough/custom now accept seed_workflow and frozen_node_ids params. Frozen nodes from the seed are injected into every designer variant, preserving the immutability contract. On ID collision, frozen node takes precedence (with warning). Engine._add_designer_variants() updated to pass these through. BUG 2: structural_hash() now explicitly includes a SHA-256 of each AgentNode prompt_template in the canonical form. While model_dump already includes it implicitly, this makes prompt hashing robust against future Pydantic config changes. Fixes #1488 Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/outer_loop/designer.py | 54 +++++++++++++- factory/outer_loop/engine.py | 18 ++++- factory/outer_loop/similarity.py | 4 + tests/test_outer_loop/test_designer.py | 95 ++++++++++++++++++++++++ tests/test_outer_loop/test_similarity.py | 46 ++++++++++++ 5 files changed, 211 insertions(+), 6 deletions(-) diff --git a/factory/outer_loop/designer.py b/factory/outer_loop/designer.py index c31af5794..d4f30690b 100644 --- a/factory/outer_loop/designer.py +++ b/factory/outer_loop/designer.py @@ -31,7 +31,12 @@ class DesignerAgent: Mutation mode proposes targeted mutations from failure telemetry. """ - def design_minimal(self, benchmark_spec: str) -> Workflow: + def design_minimal( + self, + benchmark_spec: str, + seed_workflow: Workflow | None = None, + frozen_node_ids: set[str] | None = None, + ) -> Workflow: """Create a 3-4 node workflow optimized for speed. Structure: researcher → builder → gate @@ -57,6 +62,9 @@ def design_minimal(self, benchmark_spec: str) -> Workflow: reads={".factory/reviews/builder-latest.md"}, ), } + + _inject_frozen_nodes(nodes, seed_workflow, frozen_node_ids) + edges = [ Edge(source="researcher", target="builder"), Edge(source="builder", target="gate_qa"), @@ -70,7 +78,12 @@ def design_minimal(self, benchmark_spec: str) -> Workflow: log.info("designed_minimal", nodes=len(wf.nodes), benchmark=benchmark_spec[:40]) return wf - def design_thorough(self, benchmark_spec: str) -> Workflow: + def design_thorough( + self, + benchmark_spec: str, + seed_workflow: Workflow | None = None, + frozen_node_ids: set[str] | None = None, + ) -> Workflow: """Create an 8-10 node workflow optimized for thoroughness. Structure: study → researcher → strategist → fork(builder_a, builder_b) @@ -142,6 +155,9 @@ def design_thorough(self, benchmark_spec: str) -> Workflow: reads={".factory/reviews/adversarial-qa.md"}, ), } + + _inject_frozen_nodes(nodes, seed_workflow, frozen_node_ids) + edges = [ Edge(source="study", target="researcher"), Edge(source="researcher", target="strategist"), @@ -163,7 +179,13 @@ def design_thorough(self, benchmark_spec: str) -> Workflow: log.info("designed_thorough", nodes=len(wf.nodes), benchmark=benchmark_spec[:40]) return wf - def design_custom(self, benchmark_spec: str, constraints: dict[str, object]) -> Workflow: + def design_custom( + self, + benchmark_spec: str, + constraints: dict[str, object], + seed_workflow: Workflow | None = None, + frozen_node_ids: set[str] | None = None, + ) -> Workflow: """Create a custom from-scratch workflow with optional constraints. Constraints can specify: @@ -216,6 +238,8 @@ def design_custom(self, benchmark_spec: str, constraints: dict[str, object]) -> ) edges.append(Edge(source=prev_id, target=gate_id)) + _inject_frozen_nodes(nodes, seed_workflow, frozen_node_ids) + start = core_roles[0][0] if core_roles else "gate_qa" wf = Workflow( name=f"custom_{_slug(benchmark_spec)}", @@ -305,6 +329,30 @@ def propose( return proposals[:3] +def _inject_frozen_nodes( + nodes: dict[str, object], + seed_workflow: Workflow | None, + frozen_node_ids: set[str] | None, +) -> None: + """Inject frozen nodes from a seed workflow into a template nodes dict. + + Frozen nodes take precedence over template nodes on ID collision. + """ + if not seed_workflow or not frozen_node_ids: + return + for frozen_id in frozen_node_ids: + if frozen_id in seed_workflow.nodes: + if frozen_id in nodes: + log.warning( + "frozen_node_collision", + node_id=frozen_id, + action="preferring_frozen_over_template", + ) + nodes[frozen_id] = seed_workflow.nodes[frozen_id] + else: + log.warning("frozen_node_missing_in_seed", node_id=frozen_id) + + def extract_telemetry(eval_result: EvalResult) -> dict[str, object]: """Extract structured diagnostics from an EvalResult. diff --git a/factory/outer_loop/engine.py b/factory/outer_loop/engine.py index e7a96eceb..7c4d20352 100644 --- a/factory/outer_loop/engine.py +++ b/factory/outer_loop/engine.py @@ -195,7 +195,7 @@ def seed( self._mode_registry.register(ind.id, 0, mutated_wf) if designer_count > 0: - self._add_designer_variants(pop, cfg, designer_count) + self._add_designer_variants(pop, cfg, designer_count, base_workflow) log.info( "population_seeded", @@ -210,21 +210,31 @@ def _add_designer_variants( pop: Population, cfg: SwarmConfig, designer_count: int, + seed_workflow: Workflow | None = None, ) -> None: """Add from-scratch designed workflows to the population.""" benchmark_spec = cfg.benchmark designs: list[Workflow] = [] + frozen = set(cfg.frozen_node_ids) if cfg.frozen_node_ids else None if designer_count >= 1: try: - minimal = self._designer.design_minimal(benchmark_spec) + minimal = self._designer.design_minimal( + benchmark_spec, + seed_workflow=seed_workflow, + frozen_node_ids=frozen, + ) designs.append(minimal) except Exception: log.warning("designer_minimal_failed", exc_info=True) if designer_count >= 2: try: - thorough = self._designer.design_thorough(benchmark_spec) + thorough = self._designer.design_thorough( + benchmark_spec, + seed_workflow=seed_workflow, + frozen_node_ids=frozen, + ) designs.append(thorough) except Exception: log.warning("designer_thorough_failed", exc_info=True) @@ -234,6 +244,8 @@ def _add_designer_variants( custom = self._designer.design_custom( benchmark_spec, {"max_nodes": 4 + i, "parallel": i % 2 == 0}, + seed_workflow=seed_workflow, + frozen_node_ids=frozen, ) designs.append(custom) except Exception: diff --git a/factory/outer_loop/similarity.py b/factory/outer_loop/similarity.py index 1d15a55d5..3ae107740 100644 --- a/factory/outer_loop/similarity.py +++ b/factory/outer_loop/similarity.py @@ -23,6 +23,10 @@ def structural_hash(workflow: Workflow) -> str: node = workflow.nodes[nid] d = node.model_dump(mode="json") d["_type"] = type(node).__name__ + if hasattr(node, "prompt_template"): + d["_prompt_hash"] = hashlib.sha256( + (getattr(node, "prompt_template", "") or "").encode() + ).hexdigest() nodes_canonical.append(d) edges_canonical = sorted( diff --git a/tests/test_outer_loop/test_designer.py b/tests/test_outer_loop/test_designer.py index ef0e8a97a..b87702a82 100644 --- a/tests/test_outer_loop/test_designer.py +++ b/tests/test_outer_loop/test_designer.py @@ -4,6 +4,13 @@ from factory.outer_loop.designer import DesignerAgent from factory.outer_loop.models import MutationType +from factory.workflow.primitives import ( + AgentNode, + AgentRole, + Edge, + FnNode, + Workflow, +) class TestDesignMinimal: @@ -221,3 +228,91 @@ def test_max_3_proposals(self, simple_workflow) -> None: # type: ignore[no-unty benchmark_spec="test", ) assert len(proposals) <= 3 + + +class TestFrozenNodePreservation: + """Tests for frozen node injection in designer methods.""" + + @staticmethod + def _seed_with_positions() -> Workflow: + """Create a seed workflow containing a FnNode with id='positions'.""" + return Workflow( + name="seed", + nodes={ + "positions": FnNode( + id="positions", + command="load_positions", + writes={".factory/positions.json"}, + ), + "researcher": AgentNode( + id="researcher", + role=AgentRole.RESEARCHER, + ), + }, + edges=[Edge(source="positions", target="researcher")], + start_node="positions", + ) + + def test_design_minimal_preserves_frozen_nodes(self) -> None: + designer = DesignerAgent() + seed = self._seed_with_positions() + result = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + assert "positions" in result.nodes + assert result.nodes["positions"].command == "load_positions" # type: ignore[union-attr] + + def test_design_thorough_preserves_frozen_nodes(self) -> None: + designer = DesignerAgent() + seed = self._seed_with_positions() + result = designer.design_thorough( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + assert "positions" in result.nodes + assert result.nodes["positions"].command == "load_positions" # type: ignore[union-attr] + + def test_design_custom_preserves_frozen_nodes(self) -> None: + designer = DesignerAgent() + seed = self._seed_with_positions() + result = designer.design_custom( + "bench", + {"max_nodes": 6}, + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + assert "positions" in result.nodes + assert result.nodes["positions"].command == "load_positions" # type: ignore[union-attr] + + def test_frozen_node_overwrites_template_on_collision(self) -> None: + """When a frozen node ID collides with a template node, frozen wins.""" + seed = Workflow( + name="seed", + nodes={ + "researcher": AgentNode( + id="researcher", + role=AgentRole.RESEARCHER, + timeout=999, + ), + }, + edges=[], + start_node="researcher", + ) + designer = DesignerAgent() + result = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"researcher"}, + ) + assert result.nodes["researcher"].timeout == 999 # type: ignore[union-attr] + + def test_design_without_frozen_nodes_unchanged(self) -> None: + """Calling design_minimal() without seed/frozen params works as before.""" + designer = DesignerAgent() + wf = designer.design_minimal("test benchmark") + assert 3 <= len(wf.nodes) <= 4 + issues = wf.validate_graph() + assert issues == [], f"Validation issues: {issues}" diff --git a/tests/test_outer_loop/test_similarity.py b/tests/test_outer_loop/test_similarity.py index d01b54e68..0a62426f5 100644 --- a/tests/test_outer_loop/test_similarity.py +++ b/tests/test_outer_loop/test_similarity.py @@ -35,6 +35,52 @@ def test_different_workflows_different_hash(self, simple_workflow: Workflow) -> ) assert structural_hash(simple_workflow) != structural_hash(other) + def test_prompt_change_affects_structural_hash(self) -> None: + """Two workflows identical except prompt_template must hash differently.""" + wf1 = Workflow( + name="w", + nodes={ + "a": AgentNode( + id="a", role=AgentRole.RESEARCHER, prompt_template="analyze code" + ), + }, + edges=[], + start_node="a", + ) + wf2 = Workflow( + name="w", + nodes={ + "a": AgentNode( + id="a", role=AgentRole.RESEARCHER, prompt_template="review code" + ), + }, + edges=[], + start_node="a", + ) + assert structural_hash(wf1) != structural_hash(wf2) + + def test_empty_prompt_stable_hash(self) -> None: + """Workflow with prompt_template='' and default (also '') hash the same.""" + wf1 = Workflow( + name="w", + nodes={ + "a": AgentNode( + id="a", role=AgentRole.RESEARCHER, prompt_template="" + ), + }, + edges=[], + start_node="a", + ) + wf2 = Workflow( + name="w", + nodes={ + "a": AgentNode(id="a", role=AgentRole.RESEARCHER), + }, + edges=[], + start_node="a", + ) + assert structural_hash(wf1) == structural_hash(wf2) + def test_same_structure_same_hash(self) -> None: nodes1 = { "a": FnNode(id="a", command="echo a"), From 3e18a159638b01961f19dc09dcac2df730ecd055 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 00:01:30 +0000 Subject: [PATCH 16/30] fix: NoveltyFilter.is_novel() skip edit-distance when hash is novel structural_hash now includes prompt content (_prompt_hash), so a different hash IS proof of novelty. Previously, prompt-only mutations were rejected because graph_edit_distance returned 0 (same topology) which is below min_edit_distance (5). Now is_novel returns True immediately when the hash is not in seen_hashes. Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/outer_loop/similarity.py | 14 +++++------ tests/test_outer_loop/test_similarity.py | 30 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/factory/outer_loop/similarity.py b/factory/outer_loop/similarity.py index 3ae107740..e68762aaf 100644 --- a/factory/outer_loop/similarity.py +++ b/factory/outer_loop/similarity.py @@ -158,18 +158,16 @@ def __init__(self, min_edit_distance: int = 5, max_archive_size: int = 1000) -> def is_novel(self, workflow: Workflow, threshold: int | None = None) -> bool: """Check if a workflow is novel (not seen before). - Returns False if the structural hash was seen before OR if the - graph edit distance to any archived workflow is below threshold. + Returns False if the structural hash was seen before. + Returns True otherwise, since the content-aware structural hash + (which includes prompt content) is sufficient to prove novelty. """ h = structural_hash(workflow) if h in self.seen_hashes: return False - - t = threshold if threshold is not None else self.min_edit_distance - for archived in self._archived_workflows: - if graph_edit_distance(workflow, archived) < t: - return False - + # Hash not in seen_hashes — content-aware hash proves novelty. + # Edit-distance check was rejecting prompt-only mutations + # (GED=0 < min_edit_distance) despite genuine content differences. return True def add(self, workflow: Workflow) -> None: diff --git a/tests/test_outer_loop/test_similarity.py b/tests/test_outer_loop/test_similarity.py index 0a62426f5..214f4f2ea 100644 --- a/tests/test_outer_loop/test_similarity.py +++ b/tests/test_outer_loop/test_similarity.py @@ -244,6 +244,36 @@ def test_very_different_workflow_is_novel(self, simple_workflow: Workflow) -> No ) assert nf.is_novel(other) is True + def test_prompt_only_mutation_passes_is_novel(self) -> None: + """Prompt-only mutation should be considered novel.""" + wf1 = Workflow( + name="w", + nodes={ + "a": AgentNode( + id="a", + role=AgentRole.RESEARCHER, + prompt_template="analyze code", + ), + }, + edges=[], + start_node="a", + ) + wf2 = Workflow( + name="w", + nodes={ + "a": AgentNode( + id="a", + role=AgentRole.RESEARCHER, + prompt_template="review bugs", + ), + }, + edges=[], + start_node="a", + ) + nf = NoveltyFilter(min_edit_distance=5) + nf.add(wf1) + assert nf.is_novel(wf2) is True + def test_custom_threshold(self, simple_workflow: Workflow) -> None: nf = NoveltyFilter(min_edit_distance=100) nf.add(simple_workflow) From 94b4baa4b69de58874ccac55bce64269e21758a7 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 00:25:37 +0000 Subject: [PATCH 17/30] fix: include auto-frozen DataNodes in designer variant frozen set _add_designer_variants() was only using cfg.frozen_node_ids to build the frozen set, missing DataNodes detected by _auto_frozen_nodes(). The mutation path (line 179) correctly merges both, but the designer path did not. This caused designer variants to omit DataNode "positions", producing 0-score workflows every generation. Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/outer_loop/engine.py | 5 ++- tests/test_outer_loop/test_designer.py | 57 ++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/factory/outer_loop/engine.py b/factory/outer_loop/engine.py index 7c4d20352..65bb68140 100644 --- a/factory/outer_loop/engine.py +++ b/factory/outer_loop/engine.py @@ -215,7 +215,10 @@ def _add_designer_variants( """Add from-scratch designed workflows to the population.""" benchmark_spec = cfg.benchmark designs: list[Workflow] = [] - frozen = set(cfg.frozen_node_ids) if cfg.frozen_node_ids else None + frozen_ids = set(cfg.frozen_node_ids) if cfg.frozen_node_ids else set() + if seed_workflow is not None: + frozen_ids |= _auto_frozen_nodes(seed_workflow) + frozen = frozen_ids if frozen_ids else None if designer_count >= 1: try: diff --git a/tests/test_outer_loop/test_designer.py b/tests/test_outer_loop/test_designer.py index b87702a82..65ecd0a00 100644 --- a/tests/test_outer_loop/test_designer.py +++ b/tests/test_outer_loop/test_designer.py @@ -316,3 +316,60 @@ def test_design_without_frozen_nodes_unchanged(self) -> None: assert 3 <= len(wf.nodes) <= 4 issues = wf.validate_graph() assert issues == [], f"Validation issues: {issues}" + + def test_design_minimal_preserves_frozen_data_node(self) -> None: + """DataNode auto-frozen via _auto_frozen_nodes should be preserved.""" + from factory.workflow.primitives import DataItem, DataNode + + designer = DesignerAgent() + seed = Workflow( + name="seed", + nodes={ + "positions": DataNode( + id="positions", + inline_items=[DataItem(id="pos1", prompt="test")], + subgraph_entry="solver", + subgraph_exit="solver", + ), + "solver": AgentNode( + id="solver", + role=AgentRole.BUILDER, + ), + }, + edges=[Edge(source="positions", target="solver")], + start_node="positions", + ) + result = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + assert "positions" in result.nodes + assert type(result.nodes["positions"]).__name__ == "DataNode" + + def test_engine_designer_includes_auto_frozen_data_nodes(self) -> None: + """_add_designer_variants should include auto-frozen DataNodes.""" + from factory.outer_loop.engine import _auto_frozen_nodes + from factory.workflow.primitives import DataItem, DataNode + + seed = Workflow( + name="seed", + nodes={ + "positions": DataNode( + id="positions", + inline_items=[DataItem(id="pos1", prompt="test")], + subgraph_entry="solver", + subgraph_exit="solver", + ), + "solver": AgentNode( + id="solver", + role=AgentRole.BUILDER, + ), + }, + edges=[Edge(source="positions", target="solver")], + start_node="positions", + ) + + # Verify _auto_frozen_nodes detects the DataNode + auto_frozen = _auto_frozen_nodes(seed) + assert "positions" in auto_frozen From d4eefbcf7461d4137405921f0241e226d387c448 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 01:09:00 +0000 Subject: [PATCH 18/30] fix: rewire frozen DataNode subgraph refs and start_node in designer variants After _inject_frozen_nodes() adds a DataNode to a designer template, the DataNode was orphaned: no edges pointed to it, start_node was not updated, and subgraph_entry/exit still referenced seed nodes that do not exist in the template. Add _rewire_data_nodes() helper that: 1. Updates subgraph_entry to the template original start_node 2. Updates subgraph_exit to the template terminal node 3. Adds an edge from the DataNode to subgraph_entry 4. Returns the DataNode ID as the new start_node Call _rewire_data_nodes() in design_minimal, design_thorough, and design_custom after _inject_frozen_nodes(). Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/outer_loop/designer.py | 80 ++++++++++++++++- tests/test_outer_loop/test_designer.py | 119 +++++++++++++++++++++++++ 2 files changed, 197 insertions(+), 2 deletions(-) diff --git a/factory/outer_loop/designer.py b/factory/outer_loop/designer.py index d4f30690b..3a68fe98b 100644 --- a/factory/outer_loop/designer.py +++ b/factory/outer_loop/designer.py @@ -15,6 +15,7 @@ from factory.workflow.primitives import ( AgentNode, AgentRole, + DataNode, Edge, FnNode, GateNode, @@ -69,11 +70,19 @@ def design_minimal( Edge(source="researcher", target="builder"), Edge(source="builder", target="gate_qa"), ] + + start_node = "researcher" + new_start = _rewire_data_nodes( + nodes, edges, start_node, seed_workflow, frozen_node_ids + ) + if new_start is not None: + start_node = new_start + wf = Workflow( name=f"minimal_{_slug(benchmark_spec)}", nodes=nodes, # type: ignore[arg-type] edges=edges, - start_node="researcher", + start_node=start_node, ) log.info("designed_minimal", nodes=len(wf.nodes), benchmark=benchmark_spec[:40]) return wf @@ -170,11 +179,19 @@ def design_thorough( Edge(source="code_reviewer", target="adversarial_tester"), Edge(source="adversarial_tester", target="gate_qa"), ] + + start_node = "study" + new_start = _rewire_data_nodes( + nodes, edges, start_node, seed_workflow, frozen_node_ids + ) + if new_start is not None: + start_node = new_start + wf = Workflow( name=f"thorough_{_slug(benchmark_spec)}", nodes=nodes, # type: ignore[arg-type] edges=edges, - start_node="study", + start_node=start_node, ) log.info("designed_thorough", nodes=len(wf.nodes), benchmark=benchmark_spec[:40]) return wf @@ -241,6 +258,12 @@ def design_custom( _inject_frozen_nodes(nodes, seed_workflow, frozen_node_ids) start = core_roles[0][0] if core_roles else "gate_qa" + new_start = _rewire_data_nodes( + nodes, edges, start, seed_workflow, frozen_node_ids + ) + if new_start is not None: + start = new_start + wf = Workflow( name=f"custom_{_slug(benchmark_spec)}", nodes=nodes, # type: ignore[arg-type] @@ -353,6 +376,59 @@ def _inject_frozen_nodes( log.warning("frozen_node_missing_in_seed", node_id=frozen_id) +def _rewire_data_nodes( + nodes: dict[str, object], + edges: list[Edge], + original_start: str, + seed_workflow: Workflow | None, + frozen_node_ids: set[str] | None, +) -> str | None: + """Rewire injected frozen DataNodes so they integrate into the template. + + For each frozen DataNode: + 1. Update subgraph_entry → template's original start_node + 2. Update subgraph_exit → template's terminal node (no outgoing edges) + 3. Add edge from DataNode → subgraph_entry + + Returns the DataNode ID (new start_node) or None if no DataNode was injected. + """ + if not seed_workflow or not frozen_node_ids: + return None + + # Find terminal node: the node with no outgoing edges (among template edges) + sources = {e.source for e in edges} + all_node_ids = set(nodes.keys()) + terminal_candidates = all_node_ids - sources + # Exclude the frozen DataNodes themselves from terminal candidates + frozen_data_ids: set[str] = set() + + for fid in frozen_node_ids: + node = nodes.get(fid) + if isinstance(node, DataNode): + frozen_data_ids.add(fid) + + if not frozen_data_ids: + return None + + terminal_candidates -= frozen_data_ids + terminal_node = next(iter(terminal_candidates)) if terminal_candidates else original_start + + new_start: str | None = None + for data_id in frozen_data_ids: + data_node = nodes[data_id] + assert isinstance(data_node, DataNode) + # Replace with updated subgraph_entry/exit pointing to template nodes + updated = data_node.model_copy( + update={"subgraph_entry": original_start, "subgraph_exit": terminal_node} + ) + nodes[data_id] = updated + # Add edge from DataNode → subgraph_entry + edges.insert(0, Edge(source=data_id, target=original_start)) + new_start = data_id + + return new_start + + def extract_telemetry(eval_result: EvalResult) -> dict[str, object]: """Extract structured diagnostics from an EvalResult. diff --git a/tests/test_outer_loop/test_designer.py b/tests/test_outer_loop/test_designer.py index 65ecd0a00..97e4f7aa3 100644 --- a/tests/test_outer_loop/test_designer.py +++ b/tests/test_outer_loop/test_designer.py @@ -7,6 +7,8 @@ from factory.workflow.primitives import ( AgentNode, AgentRole, + DataItem, + DataNode, Edge, FnNode, Workflow, @@ -373,3 +375,120 @@ def test_engine_designer_includes_auto_frozen_data_nodes(self) -> None: # Verify _auto_frozen_nodes detects the DataNode auto_frozen = _auto_frozen_nodes(seed) assert "positions" in auto_frozen + + +class TestDataNodeRewiring: + """Tests that frozen DataNodes are properly wired into designer templates.""" + + @staticmethod + def _seed_with_data_node() -> Workflow: + """Seed workflow containing a DataNode with subgraph refs to 'solver'.""" + return Workflow( + name="seed", + nodes={ + "positions": DataNode( + id="positions", + inline_items=[DataItem(id="pos1", prompt="test")], + subgraph_entry="solver", + subgraph_exit="solver", + ), + "solver": AgentNode( + id="solver", + role=AgentRole.BUILDER, + ), + }, + edges=[Edge(source="positions", target="solver")], + start_node="positions", + ) + + def test_minimal_start_node_is_data_node(self) -> None: + """Designer variant with DataNode has start_node == DataNode ID.""" + designer = DesignerAgent() + seed = self._seed_with_data_node() + wf = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + assert wf.start_node == "positions" + + def test_minimal_subgraph_entry_points_to_template_start(self) -> None: + """DataNode.subgraph_entry points to the template's original start.""" + designer = DesignerAgent() + seed = self._seed_with_data_node() + wf = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + data_node = wf.nodes["positions"] + assert isinstance(data_node, DataNode) + assert data_node.subgraph_entry == "researcher" + + def test_minimal_subgraph_exit_points_to_terminal(self) -> None: + """DataNode.subgraph_exit points to template's terminal node.""" + designer = DesignerAgent() + seed = self._seed_with_data_node() + wf = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + data_node = wf.nodes["positions"] + assert isinstance(data_node, DataNode) + assert data_node.subgraph_exit == "gate_qa" + + def test_minimal_has_edge_from_data_node_to_entry(self) -> None: + """Designer variant has edge from DataNode → subgraph_entry.""" + designer = DesignerAgent() + seed = self._seed_with_data_node() + wf = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + edge_pairs = [(e.source, e.target) for e in wf.edges] + assert ("positions", "researcher") in edge_pairs + + def test_minimal_without_data_node_unchanged(self) -> None: + """Designer without frozen DataNode retains original start_node.""" + designer = DesignerAgent() + wf = designer.design_minimal("bench") + assert wf.start_node == "researcher" + edge_sources = {e.source for e in wf.edges} + assert "positions" not in edge_sources + + def test_thorough_start_node_is_data_node(self) -> None: + """design_thorough variant with DataNode has start_node == DataNode ID.""" + designer = DesignerAgent() + seed = self._seed_with_data_node() + wf = designer.design_thorough( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + assert wf.start_node == "positions" + data_node = wf.nodes["positions"] + assert isinstance(data_node, DataNode) + assert data_node.subgraph_entry == "study" + assert data_node.subgraph_exit == "gate_qa" + edge_pairs = [(e.source, e.target) for e in wf.edges] + assert ("positions", "study") in edge_pairs + + def test_custom_start_node_is_data_node(self) -> None: + """design_custom variant with DataNode has start_node == DataNode ID.""" + designer = DesignerAgent() + seed = self._seed_with_data_node() + wf = designer.design_custom( + "bench", + {"max_nodes": 6}, + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + assert wf.start_node == "positions" + data_node = wf.nodes["positions"] + assert isinstance(data_node, DataNode) + assert data_node.subgraph_entry == "researcher" + assert data_node.subgraph_exit == "gate_qa" + edge_pairs = [(e.source, e.target) for e in wf.edges] + assert ("positions", "researcher") in edge_pairs From 5f5cdc5e51338a2952917e8dbb55b631dfa2175c Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 01:24:38 +0000 Subject: [PATCH 19/30] fix: remove invalid DataNode edge and guard subgraph_entry self-reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 1: _rewire_data_nodes() was inserting an explicit edge from the DataNode to its subgraph_entry. The workflow validator (_validate_datanode_edges) rejects such edges because the executor handles subgraph execution internally via DataNode.subgraph_entry — explicit edges cause double-execution. Removed the edges.insert() call. The executor reads subgraph_entry directly from the DataNode object, not from the edge list. Bug 2: When a frozen DataNode ID collides with the template's original_start (e.g., both named 'researcher'), subgraph_entry was set to itself, creating a self-referential cycle. Now follows edges from original_start to find the actual first template node, and removes the now-stale edges from original_start that would become invalid DataNode-to-subgraph edges. Tests updated: - Existing edge assertions flipped to assert edges do NOT exist - test_rewired_workflow_validates_graph: validates rewired workflow passes - test_data_node_id_collision_with_start: verifies no self-reference Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/outer_loop/designer.py | 24 ++++++++-- tests/test_outer_loop/test_designer.py | 64 ++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 9 deletions(-) diff --git a/factory/outer_loop/designer.py b/factory/outer_loop/designer.py index 3a68fe98b..46837f5c7 100644 --- a/factory/outer_loop/designer.py +++ b/factory/outer_loop/designer.py @@ -388,7 +388,11 @@ def _rewire_data_nodes( For each frozen DataNode: 1. Update subgraph_entry → template's original start_node 2. Update subgraph_exit → template's terminal node (no outgoing edges) - 3. Add edge from DataNode → subgraph_entry + + No explicit edge is added from the DataNode to subgraph_entry — the + executor reads subgraph_entry directly from the DataNode object. + Adding an explicit edge would fail validation (_validate_datanode_edges + rejects edges from a DataNode to its own subgraph nodes). Returns the DataNode ID (new start_node) or None if no DataNode was injected. """ @@ -417,13 +421,25 @@ def _rewire_data_nodes( for data_id in frozen_data_ids: data_node = nodes[data_id] assert isinstance(data_node, DataNode) + + # Determine subgraph entry: if the DataNode ID collides with the + # template's original_start, follow edges to find the actual first + # template node (otherwise subgraph_entry would point to itself). + # Also remove the now-stale edges from original_start — they would + # become invalid edges from the DataNode to its own subgraph. + entry = original_start + if data_id == original_start: + for edge in edges: + if edge.source == original_start: + entry = edge.target + break + edges[:] = [e for e in edges if e.source != original_start] + # Replace with updated subgraph_entry/exit pointing to template nodes updated = data_node.model_copy( - update={"subgraph_entry": original_start, "subgraph_exit": terminal_node} + update={"subgraph_entry": entry, "subgraph_exit": terminal_node} ) nodes[data_id] = updated - # Add edge from DataNode → subgraph_entry - edges.insert(0, Edge(source=data_id, target=original_start)) new_start = data_id return new_start diff --git a/tests/test_outer_loop/test_designer.py b/tests/test_outer_loop/test_designer.py index 97e4f7aa3..e8ff9d152 100644 --- a/tests/test_outer_loop/test_designer.py +++ b/tests/test_outer_loop/test_designer.py @@ -438,8 +438,8 @@ def test_minimal_subgraph_exit_points_to_terminal(self) -> None: assert isinstance(data_node, DataNode) assert data_node.subgraph_exit == "gate_qa" - def test_minimal_has_edge_from_data_node_to_entry(self) -> None: - """Designer variant has edge from DataNode → subgraph_entry.""" + def test_minimal_no_explicit_edge_from_data_node_to_entry(self) -> None: + """No explicit edge from DataNode to subgraph_entry (executor uses subgraph_entry directly).""" designer = DesignerAgent() seed = self._seed_with_data_node() wf = designer.design_minimal( @@ -448,7 +448,7 @@ def test_minimal_has_edge_from_data_node_to_entry(self) -> None: frozen_node_ids={"positions"}, ) edge_pairs = [(e.source, e.target) for e in wf.edges] - assert ("positions", "researcher") in edge_pairs + assert ("positions", "researcher") not in edge_pairs def test_minimal_without_data_node_unchanged(self) -> None: """Designer without frozen DataNode retains original start_node.""" @@ -472,8 +472,9 @@ def test_thorough_start_node_is_data_node(self) -> None: assert isinstance(data_node, DataNode) assert data_node.subgraph_entry == "study" assert data_node.subgraph_exit == "gate_qa" + # No explicit edge from DataNode to subgraph_entry edge_pairs = [(e.source, e.target) for e in wf.edges] - assert ("positions", "study") in edge_pairs + assert ("positions", "study") not in edge_pairs def test_custom_start_node_is_data_node(self) -> None: """design_custom variant with DataNode has start_node == DataNode ID.""" @@ -490,5 +491,58 @@ def test_custom_start_node_is_data_node(self) -> None: assert isinstance(data_node, DataNode) assert data_node.subgraph_entry == "researcher" assert data_node.subgraph_exit == "gate_qa" + # No explicit edge from DataNode to subgraph_entry edge_pairs = [(e.source, e.target) for e in wf.edges] - assert ("positions", "researcher") in edge_pairs + assert ("positions", "researcher") not in edge_pairs + + def test_rewired_workflow_validates_graph(self) -> None: + """Rewired workflow with DataNode passes validate_graph() without issues.""" + designer = DesignerAgent() + seed = self._seed_with_data_node() + wf = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"positions"}, + ) + issues = wf.validate_graph() + assert issues == [], f"Validation issues: {issues}" + + def test_data_node_id_collision_with_start(self) -> None: + """DataNode ID == template start_node must not create self-referential subgraph_entry.""" + designer = DesignerAgent() + # Create a seed where the DataNode ID is 'researcher' — same as + # the minimal template's start_node. + seed = Workflow( + name="seed", + nodes={ + "researcher": DataNode( + id="researcher", + inline_items=[DataItem(id="pos1", prompt="test")], + subgraph_entry="solver", + subgraph_exit="solver", + ), + "solver": AgentNode( + id="solver", + role=AgentRole.BUILDER, + ), + }, + edges=[Edge(source="researcher", target="solver")], + start_node="researcher", + ) + wf = designer.design_minimal( + "bench", + seed_workflow=seed, + frozen_node_ids={"researcher"}, + ) + data_node = wf.nodes["researcher"] + assert isinstance(data_node, DataNode) + # subgraph_entry must NOT be 'researcher' (self-reference) + assert data_node.subgraph_entry != "researcher" + # It should point to the first node reachable from original start via edges + assert data_node.subgraph_entry == "builder" + # No structural issues (cycle, double-execution edge, unreachable). + # Data dependency warnings are expected since the DataNode replaced + # the researcher that would normally write the file. + issues = wf.validate_graph() + structural = [i for i in issues if "no predecessor writes" not in i] + assert structural == [], f"Structural issues: {structural}" From 920c6c9b3cccf8610a4d62ed8e93ae3206f009fd Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 01:24:58 +0000 Subject: [PATCH 20/30] docs: update builder-latest.md with fix report Co-Authored-By: Claude Opus 4.6 (1M context) --- .factory/reviews/builder-latest.md | 31 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/.factory/reviews/builder-latest.md b/.factory/reviews/builder-latest.md index b93836ea9..41a70f28c 100644 --- a/.factory/reviews/builder-latest.md +++ b/.factory/reviews/builder-latest.md @@ -1,20 +1,21 @@ -## Builder Report — disk_reads re-scan after setup() +## Builder Report + +- **Branch:** factory/run-e3ddbac6 +- **Commit:** b364c09f +- **Status:** ✅ COMPLETE — 38/38 tests pass, lint clean ### Changes -**factory/workflow/executor.py** (`_execute_data` → `run_item`): -- Added a per-item re-scan of subgraph reads after `resolved_task.setup()` completes -- New local variable `setup_reads: set[str]` scans `sub_workflow.nodes` reads against `item_project_path` -- Changed `item_executor.completed_files = self.completed_files | disk_reads` to `self.completed_files | disk_reads | setup_reads` -- The pre-scan at line 791 (`disk_reads`) is preserved — it handles pre-existing files -- `setup_reads` is local to `run_item()` — no mutation of shared `disk_reads` set +**factory/outer_loop/designer.py** — `_rewire_data_nodes()`: +- **Bug 1 (Critical):** Removed `edges.insert(0, Edge(source=data_id, target=original_start))`. The workflow validator (`_validate_datanode_edges`) rejects explicit edges from a DataNode to its subgraph nodes — the executor handles subgraph execution internally via `DataNode.subgraph_entry`, so explicit edges cause double-execution. +- **Bug 2 (Medium):** Added guard for `data_id == original_start` collision. When the DataNode ID matches the template's start node, follows edges from `original_start` to find the actual first template node (avoiding self-referential `subgraph_entry`). Also removes stale template edges from `original_start` that would become invalid DataNode-to-subgraph edges. -**tests/test_data_node.py**: -- Added `_SetupWritingTask` — a minimal Task whose `setup()` writes a file to the workspace -- Added `TestDiskReadsRescanAfterSetup::test_setup_created_file_in_completed_files` — verifies that when `setup()` creates a file declared in a subgraph node's `reads`, it appears in the sub-executor's `completed_files` +**tests/test_outer_loop/test_designer.py**: +- Updated 3 existing tests to assert edges do NOT exist (previously asserted the buggy behavior) +- Added `test_rewired_workflow_validates_graph` — calls `wf.validate_graph()` on a rewired workflow, asserts no issues +- Added `test_data_node_id_collision_with_start` — creates a seed where DataNode ID == 'researcher' (same as minimal template start), verifies no self-referential cycle and no structural validation issues -### Verification -- All 64 DataNode tests pass (including new test) -- All 46 executor tests pass -- `ruff check` clean -- `mypy` clean +### Test Results +- 38/38 tests pass (36 existing + 2 new) +- Lint: clean +- Mypy: 6 pre-existing errors (dict invariance), no new errors From acce1c3703f5ed5fe91d2c07011a8418279b8822 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 03:20:23 +0000 Subject: [PATCH 21/30] fix: validate DataNode+Loop subgraph_exit and add structured gate logging - Add _validate_datanode_exit() in validation.py: warns when DataNode's subgraph_exit points to a GateNode (drops PROCEED edge, silently breaks loop termination) - Add gate_proceed_edge_missing structured warning in executor.py: logs when a gate evaluates PROCEED but no PROCEED edge exists - Add 3 integration tests in test_data_node.py: - test_data_node_with_loop_subgraph: end-to-end Loop body runs 3x - test_data_node_loop_wrong_exit_warns: wrong subgraph_exit caught - test_loop_package_compiled_preserves_edges: all 3 loop edges preserved Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/workflow/executor.py | 7 + factory/workflow/validation.py | 21 +++ tests/test_data_node.py | 225 +++++++++++++++++++++++++++++++++ 3 files changed, 253 insertions(+) diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index 200cb65c5..da2303f5e 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -437,6 +437,13 @@ async def _execute_gate(self, node: GateNode) -> None: if target_id is None: target_id = self._next_unconditional(node_id) + if target_id is None: + log.warning( + "gate_proceed_edge_missing", + gate_id=node_id, + workflow=self.workflow.name, + ) + if target_id: await self._execute_from(target_id) diff --git a/factory/workflow/validation.py b/factory/workflow/validation.py index 42c736b20..69f97325d 100644 --- a/factory/workflow/validation.py +++ b/factory/workflow/validation.py @@ -150,6 +150,26 @@ def _validate_datanode_edges(workflow: Workflow, issues: list[str]) -> None: ) +def _validate_datanode_exit(workflow: Workflow, issues: list[str]) -> None: + """Warn when a DataNode's subgraph_exit points to a GateNode. + + When subgraph_exit is a GateNode, _collect_subgraph_nodes stops BFS + at the gate, excluding the PROCEED edge target (the real exit node). + Workflow.subgraph() then drops the PROCEED edge, causing execution + to silently halt after one loop iteration. + """ + for nid, node in workflow.nodes.items(): + if type(node).__name__ != "DataNode": + continue + exit_id = node.subgraph_exit # type: ignore[union-attr] + exit_node = workflow.nodes.get(exit_id) + if exit_node is not None and type(exit_node).__name__ == "GateNode": + issues.append( + f"DataNode '{nid}' has subgraph_exit pointing to GateNode '{exit_id}'. " + f"This drops the PROCEED edge. Use the Loop's exit_node instead." + ) + + def _collect_subgraph_nodes( workflow: Workflow, entry: str, @@ -210,6 +230,7 @@ def validate_workflow(workflow: Workflow) -> list[str]: _validate_data_dependencies(g, workflow, issues) _validate_fork_join_nodes(workflow, issues) _validate_datanode_edges(workflow, issues) + _validate_datanode_exit(workflow, issues) for nid, node in nodes.items(): if type(node).__name__ == "SubgraphForkNode": diff --git a/tests/test_data_node.py b/tests/test_data_node.py index 3415b0a33..48b20ca33 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -1522,6 +1522,231 @@ def verify(self, instance: Any, workspace: Path): return VerifyResult(passed=True, score=1.0) +class TestDataNodeLoopSubgraph: + """DataNode + Loop/Gate subgraph integration tests.""" + + @pytest.mark.asyncio + async def test_data_node_with_loop_subgraph(self, tmp_path: Path) -> None: + """DataNode whose subgraph is a Loop should execute body 3 times via fn gate.""" + from factory.workflow.executor import WorkflowExecutor + from factory.workflow.package import Loop, Package + from factory.workflow.primitives import GateNode, VerdictType + + project_path = tmp_path + (project_path / ".factory").mkdir(parents=True, exist_ok=True) + counter_file = project_path / "counter.txt" + + pp = str(project_path) + + body_node = FnNode( + id="loop_body", + command=f"python3 -c \"open('{pp}/counter.txt','a').write('x\\n')\"", + reads=set(), + writes={"counter.txt"}, + ) + body_pkg = Package( + name="body", + graph=Workflow( + name="body_graph", + nodes={"loop_body": body_node}, + edges=[], + start_node="loop_body", + ), + entry_node="loop_body", + exit_node="loop_body", + ) + + gate = GateNode( + id="loop_gate", + evaluator_type="fn", + evaluator_command=( + f"python3 -c \"" + f"import pathlib; " + f"p=pathlib.Path('{pp}/counter.txt'); " + f"c=len(p.read_text().splitlines()) if p.exists() else 0; " + f"print('PROCEED' if c >= 3 else 'RELOOP: try again')" + f"\"" + ), + reads=set(), + ) + + loop_pkg = Loop(body_pkg, gate, max_iterations=10, name="test_loop") + + # Build DataNode workflow with correct subgraph_exit = loop exit_node + data_node = DataNode( + id="data_driver", + inline_items=[DataItem(id="game1", prompt="play")], + subgraph_entry=loop_pkg.entry_node, + subgraph_exit=loop_pkg.exit_node, + ) + + all_nodes: dict[str, Any] = {"data_driver": data_node} + for nid, node in loop_pkg.graph.nodes.items(): + all_nodes[nid] = node + + wf = Workflow( + name="loop_data_test", + nodes=all_nodes, + edges=list(loop_pkg.graph.edges), + start_node="data_driver", + ) + + # Validate graph — should have no issues + issues = wf.validate_graph() + assert not issues, f"Unexpected validation issues: {issues}" + + executor = WorkflowExecutor(wf, project_path, dry_run=False) + result = await executor.execute() + + assert result.success, f"Execution failed: {result.halt_reason}" + assert counter_file.exists(), "counter.txt should exist" + lines = counter_file.read_text().splitlines() + assert len(lines) == 3, f"Expected 3 lines, got {len(lines)}" + # Check inner executor nodes: 3 body + 3 gate + 1 exit = 7 + parsed = json.loads(result.node_outputs["data_driver"]) + assert len(parsed) == 1 + inner_nodes = parsed[0]["nodes_executed"] + assert inner_nodes >= 7, f"Expected >= 7 inner nodes, got {inner_nodes}" + + def test_data_node_loop_wrong_exit_warns(self) -> None: + """DataNode with subgraph_exit pointing to GateNode should produce a validation warning.""" + from factory.workflow.package import Loop, Package + from factory.workflow.primitives import GateNode, VerdictType + + body_node = FnNode( + id="loop_body", + command="echo body", + reads=set(), + writes={"counter.txt"}, + ) + body_pkg = Package( + name="body", + graph=Workflow( + name="body_graph", + nodes={"loop_body": body_node}, + edges=[], + start_node="loop_body", + ), + entry_node="loop_body", + exit_node="loop_body", + ) + + gate = GateNode( + id="loop_gate", + evaluator_type="fn", + evaluator_command="echo PROCEED", + reads=set(), + ) + + loop_pkg = Loop(body_pkg, gate, max_iterations=5, name="test_loop") + + # INCORRECT: subgraph_exit points to gate instead of exit_node + data_node = DataNode( + id="data_driver", + inline_items=[DataItem(id="game1", prompt="play")], + subgraph_entry=loop_pkg.entry_node, + subgraph_exit=gate.id, # WRONG — should be loop_pkg.exit_node + ) + + all_nodes: dict[str, Any] = {"data_driver": data_node} + for nid, node in loop_pkg.graph.nodes.items(): + all_nodes[nid] = node + + wf = Workflow( + name="wrong_exit_test", + nodes=all_nodes, + edges=list(loop_pkg.graph.edges), + start_node="data_driver", + ) + + issues = wf.validate_graph() + gate_warnings = [ + i for i in issues + if "GateNode" in i and "subgraph_exit" in i + ] + assert len(gate_warnings) >= 1, f"Expected GateNode warning, got: {issues}" + + def test_loop_package_compiled_preserves_edges(self) -> None: + """Loop Package compiled into a Workflow preserves all 3 loop edges in subgraph.""" + from factory.workflow.executor import _collect_subgraph_nodes + from factory.workflow.package import Loop, Package + from factory.workflow.primitives import GateNode, VerdictType + + body_node = FnNode( + id="loop_body", + command="echo body", + reads=set(), + ) + body_pkg = Package( + name="body", + graph=Workflow( + name="body_graph", + nodes={"loop_body": body_node}, + edges=[], + start_node="loop_body", + ), + entry_node="loop_body", + exit_node="loop_body", + ) + + gate = GateNode( + id="loop_gate", + evaluator_type="fn", + evaluator_command="echo PROCEED", + reads=set(), + ) + + loop_pkg = Loop(body_pkg, gate, max_iterations=5, name="test_loop") + + # Build DataNode with CORRECT exit_node + data_node = DataNode( + id="data_driver", + inline_items=[DataItem(id="game1", prompt="play")], + subgraph_entry=loop_pkg.entry_node, + subgraph_exit=loop_pkg.exit_node, + ) + + all_nodes: dict[str, Any] = {"data_driver": data_node} + for nid, node in loop_pkg.graph.nodes.items(): + all_nodes[nid] = node + + wf = Workflow( + name="edge_preservation_test", + nodes=all_nodes, + edges=list(loop_pkg.graph.edges), + start_node="data_driver", + ) + + # Collect subgraph nodes + subgraph_ids = _collect_subgraph_nodes( + wf, loop_pkg.entry_node, loop_pkg.exit_node, + ) + + # All 3 loop nodes + exit must be in subgraph + assert "loop_body" in subgraph_ids + assert "loop_gate" in subgraph_ids + assert loop_pkg.exit_node in subgraph_ids + + # Extract subgraph and check edges + sub_wf = wf.subgraph(subgraph_ids, name="sub", start_node=loop_pkg.entry_node) + + # Check all 3 loop edges are preserved + edge_tuples = [(e.source, e.target, e.condition) for e in sub_wf.edges] + + # body → gate (unconditional) + assert ("loop_body", "loop_gate", None) in edge_tuples, ( + f"Missing body→gate edge. Edges: {edge_tuples}" + ) + # gate → body (RELOOP) + assert ("loop_gate", "loop_body", VerdictType.RELOOP) in edge_tuples, ( + f"Missing gate→body RELOOP edge. Edges: {edge_tuples}" + ) + # gate → exit (PROCEED) + assert ("loop_gate", loop_pkg.exit_node, VerdictType.PROCEED) in edge_tuples, ( + f"Missing gate→exit PROCEED edge. Edges: {edge_tuples}" + ) + + class TestDiskReadsRescanAfterSetup: """setup()-created files must appear in sub-executor completed_files.""" From 32bf05e1a516f61a2e82c415b57572873020e2dd Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 03:22:14 +0000 Subject: [PATCH 22/30] fix: refine DataNode exit validation to only warn for Loop GateNodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The _validate_datanode_exit function was producing false positives by warning whenever subgraph_exit pointed to ANY GateNode. Terminal GateNodes (no RELOOP edges) are valid as subgraph_exit — only GateNodes participating in a Loop (with outgoing RELOOP edges) cause the PROCEED edge drop issue. Now checks for RELOOP edges before emitting the warning. Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/workflow/validation.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/factory/workflow/validation.py b/factory/workflow/validation.py index 69f97325d..1454a3363 100644 --- a/factory/workflow/validation.py +++ b/factory/workflow/validation.py @@ -151,19 +151,31 @@ def _validate_datanode_edges(workflow: Workflow, issues: list[str]) -> None: def _validate_datanode_exit(workflow: Workflow, issues: list[str]) -> None: - """Warn when a DataNode's subgraph_exit points to a GateNode. + """Warn when a DataNode's subgraph_exit points to a Loop GateNode. - When subgraph_exit is a GateNode, _collect_subgraph_nodes stops BFS - at the gate, excluding the PROCEED edge target (the real exit node). + When subgraph_exit is a GateNode that participates in a Loop (has + outgoing RELOOP edges), _collect_subgraph_nodes stops BFS at the gate, + excluding the PROCEED edge target (the real exit node). Workflow.subgraph() then drops the PROCEED edge, causing execution to silently halt after one loop iteration. + + Terminal GateNodes (no RELOOP edges) are fine as subgraph_exit — they + don't have a PROCEED edge that would be dropped. """ + from factory.workflow.primitives import VerdictType + for nid, node in workflow.nodes.items(): if type(node).__name__ != "DataNode": continue exit_id = node.subgraph_exit # type: ignore[union-attr] exit_node = workflow.nodes.get(exit_id) if exit_node is not None and type(exit_node).__name__ == "GateNode": + has_reloop = any( + e.source == exit_id and e.condition == VerdictType.RELOOP + for e in workflow.edges + ) + if not has_reloop: + continue issues.append( f"DataNode '{nid}' has subgraph_exit pointing to GateNode '{exit_id}'. " f"This drops the PROCEED edge. Use the Loop's exit_node instead." From e9d5b70fa3856caf83f216f5eb01f7f09c4fb916 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 03:22:28 +0000 Subject: [PATCH 23/30] docs: update builder-latest.md with fix report Co-Authored-By: Claude Opus 4.6 (1M context) --- .factory/reviews/builder-latest.md | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/.factory/reviews/builder-latest.md b/.factory/reviews/builder-latest.md index 41a70f28c..cf7e347f3 100644 --- a/.factory/reviews/builder-latest.md +++ b/.factory/reviews/builder-latest.md @@ -1,21 +1,15 @@ -## Builder Report +# Builder Report -- **Branch:** factory/run-e3ddbac6 -- **Commit:** b364c09f -- **Status:** ✅ COMPLETE — 38/38 tests pass, lint clean +## Issue +Fix false positive in `_validate_datanode_exit` — terminal GateNodes (no RELOOP edges) are valid as `subgraph_exit`. -### Changes +## Changes +- **factory/workflow/validation.py**: Updated `_validate_datanode_exit` to check for outgoing RELOOP edges before warning about a GateNode used as `subgraph_exit`. Terminal GateNodes without RELOOP edges are now allowed. -**factory/outer_loop/designer.py** — `_rewire_data_nodes()`: -- **Bug 1 (Critical):** Removed `edges.insert(0, Edge(source=data_id, target=original_start))`. The workflow validator (`_validate_datanode_edges`) rejects explicit edges from a DataNode to its subgraph nodes — the executor handles subgraph execution internally via `DataNode.subgraph_entry`, so explicit edges cause double-execution. -- **Bug 2 (Medium):** Added guard for `data_id == original_start` collision. When the DataNode ID matches the template's start node, follows edges from `original_start` to find the actual first template node (avoiding self-referential `subgraph_entry`). Also removes stale template edges from `original_start` that would become invalid DataNode-to-subgraph edges. +## Verification +- `tests/test_outer_loop/test_designer.py`: 38 passed +- `tests/test_data_node.py`: 67 passed +- Total: 105 passed, 0 failed -**tests/test_outer_loop/test_designer.py**: -- Updated 3 existing tests to assert edges do NOT exist (previously asserted the buggy behavior) -- Added `test_rewired_workflow_validates_graph` — calls `wf.validate_graph()` on a rewired workflow, asserts no issues -- Added `test_data_node_id_collision_with_start` — creates a seed where DataNode ID == 'researcher' (same as minimal template start), verifies no self-referential cycle and no structural validation issues - -### Test Results -- 38/38 tests pass (36 existing + 2 new) -- Lint: clean -- Mypy: 6 pre-existing errors (dict invariance), no new errors +## Commit +`fix: refine DataNode exit validation to only warn for Loop GateNodes` From 5fdc2146d3da188c6f9e9607418059aa6ba0df35 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Sat, 12 Sep 2026 04:28:21 +0000 Subject: [PATCH 24/30] feat: add diagnostic logging and AgentNode loop test for DataNode executor - Add data_item_setup_complete log after setup() to list workspace files - Add setup_read_path_mismatch warning when declared reads differ from actual file paths - Add wait_for_reads_timeout_diagnostic warning with completed_files snapshot - Add integration test: DataNode + Loop(AgentNode body, fn GateNode) with 3-iteration RELOOP - Add test: setup_read_path_mismatch detects file at wrong relative path Co-Authored-By: Claude Opus 4.6 (1M context) --- .factory/reviews/builder-latest.md | 24 ++--- factory/workflow/executor.py | 38 ++++++++ tests/test_data_node.py | 152 +++++++++++++++++++++++++++++ 3 files changed, 203 insertions(+), 11 deletions(-) diff --git a/.factory/reviews/builder-latest.md b/.factory/reviews/builder-latest.md index cf7e347f3..a95832480 100644 --- a/.factory/reviews/builder-latest.md +++ b/.factory/reviews/builder-latest.md @@ -1,15 +1,17 @@ -# Builder Report +# Builder Report — Diagnostic Logging & AgentNode Loop Test -## Issue -Fix false positive in `_validate_datanode_exit` — terminal GateNodes (no RELOOP edges) are valid as `subgraph_exit`. +## Changes Made -## Changes -- **factory/workflow/validation.py**: Updated `_validate_datanode_exit` to check for outgoing RELOOP edges before warning about a GateNode used as `subgraph_exit`. Terminal GateNodes without RELOOP edges are now allowed. +### factory/workflow/executor.py +- **data_item_setup_complete log**: After `setup()` call, logs the files created in the workspace (item_id, workspace path, file count, first 20 files) +- **setup_read_path_mismatch warning**: During setup_reads rescan, detects when a declared read path doesn't match the actual file location by searching for the basename recursively +- **wait_for_reads_timeout_diagnostic warning**: When `_wait_for_reads` times out, logs what files DO exist in completed_files vs what's missing -## Verification -- `tests/test_outer_loop/test_designer.py`: 38 passed -- `tests/test_data_node.py`: 67 passed -- Total: 105 passed, 0 failed +### tests/test_data_node.py +- **test_data_node_loop_with_agent_body**: Integration test for DataNode + Loop(AgentNode body, fn GateNode). Uses a mock agent_fn that appends to counter.txt, gate checks line count, verifies 3 iterations via RELOOP→PROCEED cycle +- **test_setup_read_path_mismatch_logs_warning**: Tests that a mismatched read path (file at `.factory/memory.md` but node reads `memory.md`) causes the inner executor to halt, with a fast timeout patch to avoid 60s CI delay -## Commit -`fix: refine DataNode exit validation to only warn for Loop GateNodes` +## Test Results +- All 69 tests in test_data_node.py pass +- All 747 workflow-related tests pass (8 skipped) +- No lint errors in modified files diff --git a/factory/workflow/executor.py b/factory/workflow/executor.py index da2303f5e..6acbf0aa7 100644 --- a/factory/workflow/executor.py +++ b/factory/workflow/executor.py @@ -857,12 +857,41 @@ async def run_item( prompt=resolved_task.prompt(inst), ) + # Diagnostic: list files created by setup() + _setup_files = [ + str(f.relative_to(item_project_path)) + for f in item_project_path.rglob('*') if f.is_file() + ] + log.info( + 'data_item_setup_complete', + item_id=item.id, + workspace=str(item_project_path), + files_found=len(_setup_files), + files=_setup_files[:20], + ) + # Re-scan subgraph reads for files created by setup() setup_reads: set[str] = set() for sg_node in sub_workflow.nodes.values(): for r in sg_node.reads: if (item_project_path / r).exists(): setup_reads.add(r) + else: + # Check if file exists under a different relative path + basename = Path(r).name + matches = [ + str(f.relative_to(item_project_path)) + for f in item_project_path.rglob(basename) + if f.is_file() + ] + if matches: + log.warning( + 'setup_read_path_mismatch', + node_id=sg_node.id, + declared_read=r, + found_at=matches, + hint='node.reads path does not match setup() output location', + ) # Write current_item.json for subgraph visibility item_json_path = item_project_path / ".factory" / "current_item.json" @@ -1456,6 +1485,15 @@ async def _wait_for_reads(self, node: NodeType) -> None: if not missing: return if waited >= max_wait: + # Diagnostic: show what files exist vs what's expected + existing = sorted(self.completed_files) + log.warning( + 'wait_for_reads_timeout_diagnostic', + node_id=node.id, + missing_reads=sorted(missing), + completed_files_count=len(self.completed_files), + sample_completed=existing[:10], + ) self.result.halted = True self.result.halt_reason = ( f"node '{node.id}' timed out waiting for reads: {sorted(missing)}" diff --git a/tests/test_data_node.py b/tests/test_data_node.py index 48b20ca33..dc6e8c88c 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -1797,3 +1797,155 @@ async def spy_execute(self_inner): # The sub-executor's completed_files must contain the setup-written file assert len(captured_completed) == 1 assert setup_file in captured_completed[0] + + @pytest.mark.asyncio + async def test_data_node_loop_with_agent_body(self, tmp_path: Path) -> None: + """DataNode + Loop where body is an AgentNode — loop should iterate via RELOOP.""" + from factory.workflow.executor import WorkflowExecutor + from factory.workflow.package import Loop, Package + from factory.workflow.primitives import AgentNode, AgentRole, GateNode + + project_path = tmp_path + (project_path / '.factory').mkdir(parents=True, exist_ok=True) + counter_file = project_path / 'counter.txt' + pp = str(project_path) + + # Agent body: mock agent_fn that appends to counter file + call_count = 0 + + async def mock_agent_fn(role, task, proj_path, **kwargs): + nonlocal call_count + call_count += 1 + cf = Path(proj_path) / 'counter.txt' + cf.parent.mkdir(parents=True, exist_ok=True) + with open(cf, 'a') as f: + f.write(f'move {call_count}\n') + return (f'Generated move {call_count}', 0) + + generator = AgentNode( + id='generator', + role=AgentRole.BUILDER, + prompt_template='Generate the next move', + reads=set(), + writes=set(), # mock_agent_fn handles file I/O directly + timeout=30, + ) + body_pkg = Package( + name='gen_body', + graph=Workflow( + name='gen_graph', + nodes={'generator': generator}, + edges=[], + start_node='generator', + ), + entry_node='generator', + exit_node='generator', + ) + + gate = GateNode( + id='game_gate', + evaluator_type='fn', + evaluator_command=( + f"python3 -c \"" + f"import pathlib; " + f"p=pathlib.Path('{pp}/counter.txt'); " + f"c=len(p.read_text().splitlines()) if p.exists() else 0; " + f"print('PROCEED' if c >= 3 else 'RELOOP: keep playing')" + f"\"" + ), + reads=set(), + ) + + loop_pkg = Loop(body_pkg, gate, max_iterations=10, name='game_loop') + + data_node = DataNode( + id='game_data', + inline_items=[DataItem(id='game1', prompt='Play chess')], + subgraph_entry=loop_pkg.entry_node, + subgraph_exit=loop_pkg.exit_node, + ) + + all_nodes: dict[str, Any] = {'game_data': data_node} + for nid, node in loop_pkg.graph.nodes.items(): + all_nodes[nid] = node + + wf = Workflow( + name='agent_loop_test', + nodes=all_nodes, + edges=list(loop_pkg.graph.edges), + start_node='game_data', + ) + + issues = wf.validate_graph() + assert not issues, f'Validation issues: {issues}' + + executor = WorkflowExecutor(wf, project_path, agent_fn=mock_agent_fn) + result = await executor.execute() + + assert result.success, f'Execution failed: {result.halt_reason}' + assert counter_file.exists(), 'counter.txt should exist' + lines = counter_file.read_text().strip().splitlines() + assert len(lines) == 3, f'Expected 3 lines (3 iterations), got {len(lines)}: {lines}' + assert call_count == 3, f'Expected agent called 3 times, got {call_count}' + + @pytest.mark.asyncio + async def test_setup_read_path_mismatch_logs_warning(self, tmp_path: Path) -> None: + """When setup() creates a file at a different path than node.reads expects, + the reader should timeout waiting for the mismatched read path.""" + from factory.workflow.executor import WorkflowExecutor + + project_path = tmp_path + (project_path / '.factory').mkdir(parents=True, exist_ok=True) + + # Create a file at .factory/memory.md but declare reads as 'memory.md' + (project_path / '.factory' / 'memory.md').write_text('game state') + + wf = Workflow( + name='mismatch_test', + nodes={ + 'data': DataNode( + id='data', + inline_items=[DataItem(id='item1', prompt='test')], + subgraph_entry='reader', + subgraph_exit='reader', + ), + 'reader': FnNode( + id='reader', + command='echo ok', + reads={'memory.md'}, # WRONG — file is at .factory/memory.md + ), + }, + edges=[], + start_node='data', + ) + + # Patch max_wait to avoid 60s timeout in CI + async def fast_wait(self_inner, node): + # Reduced max_wait for test speed + poll_interval = 0.1 + waited = 0.0 + while True: + missing = node.reads - self_inner.completed_files + if not missing: + return + if waited >= 0.3: + self_inner.result.halted = True + self_inner.result.halt_reason = ( + f"node '{node.id}' timed out waiting for reads: {sorted(missing)}" + ) + return + await asyncio.sleep(poll_interval) + waited += poll_interval + + with patch.object(WorkflowExecutor, '_wait_for_reads', fast_wait): + executor = WorkflowExecutor(wf, project_path, dry_run=False) + result = await executor.execute() + + # DataNode fault-isolates: outer succeeds but inner item fails due to read timeout + assert result.success + parsed = json.loads(result.node_outputs['data']) + assert len(parsed) == 1 + item_result = parsed[0] + # Inner executor halted waiting for 'memory.md' that doesn't exist at that path + assert not item_result['success'] + assert item_result['nodes_executed'] == 0 # reader never ran From c8d391a273d96d4d8ca369cbc725e08dce455e2e Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Mon, 14 Sep 2026 17:49:40 +0000 Subject: [PATCH 25/30] fix: auto-freeze DataNode subgraph nodes when DataNode is frozen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend _auto_frozen_nodes() to include all nodes in a DataNode's subgraph (entry→exit) by calling _collect_subgraph_nodes() from the executor module. This prevents outer-loop mutations from breaking DataNode execution contracts by removing or redirecting subgraph nodes. Changes: - factory/outer_loop/engine.py: _auto_frozen_nodes() now collects subgraph node IDs for each DataNode and adds debug logging - tests/test_outer_loop/test_mutations.py: Updated existing assertion, added multi-node subgraph test and removal protection test Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/outer_loop/engine.py | 23 +++++++- tests/test_outer_loop/test_mutations.py | 77 ++++++++++++++++++++++++- 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/factory/outer_loop/engine.py b/factory/outer_loop/engine.py index 65bb68140..b34c22700 100644 --- a/factory/outer_loop/engine.py +++ b/factory/outer_loop/engine.py @@ -40,10 +40,27 @@ def _auto_frozen_nodes(workflow: Workflow) -> set[str]: - """Return node IDs that should always be frozen during mutation.""" - from factory.workflow.primitives import DataNode + """Return node IDs that should always be frozen during mutation. - return {nid for nid, node in workflow.nodes.items() if isinstance(node, DataNode)} + Includes DataNode IDs and all nodes in their subgraphs (entry→exit). + """ + from factory.workflow.primitives import DataNode + from factory.workflow.executor import _collect_subgraph_nodes + + frozen: set[str] = set() + for nid, node in workflow.nodes.items(): + if isinstance(node, DataNode): + frozen.add(nid) + subgraph_ids = _collect_subgraph_nodes( + workflow, node.subgraph_entry, node.subgraph_exit, + ) + frozen.update(subgraph_ids) + log.debug( + "data_node_subgraph_frozen", + data_node=nid, + subgraph_ids=list(subgraph_ids), + ) + return frozen class BudgetTracker: diff --git a/tests/test_outer_loop/test_mutations.py b/tests/test_outer_loop/test_mutations.py index bdc4c70c8..757a67970 100644 --- a/tests/test_outer_loop/test_mutations.py +++ b/tests/test_outer_loop/test_mutations.py @@ -960,7 +960,7 @@ def test_auto_frozen_nodes_returns_data_node_ids(self) -> None: ] wf = Workflow(name="with_data", nodes=nodes, edges=edges, start_node="study") frozen = _auto_frozen_nodes(wf) - assert frozen == {"data_loader"} + assert frozen == {"data_loader", "builder"} def test_auto_frozen_nodes_empty_when_no_data_nodes(self) -> None: from factory.outer_loop.engine import _auto_frozen_nodes @@ -1012,3 +1012,78 @@ def test_data_node_protected_from_direct_removal(self) -> None: assert mutate_params( wf, "data_loader", {"timeout": 999}, frozen_nodes=frozen, ) is None + + def test_auto_frozen_nodes_includes_multi_node_subgraph(self) -> None: + """Subgraph spanning multiple nodes is fully frozen.""" + from factory.outer_loop.engine import _auto_frozen_nodes + from factory.workflow.primitives import DataNode, DataItem + + nodes: dict[str, AgentNode | FnNode | DataNode] = { + "data_loader": DataNode( + id="data_loader", + inline_items=[DataItem(id="item1", prompt="test")], + subgraph_entry="sub_entry", + subgraph_exit="sub_exit", + ), + "sub_entry": AgentNode(id="sub_entry", role=AgentRole.BUILDER), + "sub_mid": FnNode(id="sub_mid", command="echo mid"), + "sub_exit": AgentNode(id="sub_exit", role=AgentRole.CODE_REVIEWER), + "external": FnNode(id="external", command="echo external"), + } + edges = [ + Edge(source="external", target="data_loader"), + Edge(source="data_loader", target="sub_entry"), + Edge(source="sub_entry", target="sub_mid"), + Edge(source="sub_mid", target="sub_exit"), + ] + wf = Workflow( + name="multi_subgraph", nodes=nodes, edges=edges, start_node="external", + ) + frozen = _auto_frozen_nodes(wf) + # DataNode + all subgraph nodes frozen + assert frozen == {"data_loader", "sub_entry", "sub_mid", "sub_exit"} + # External node is NOT frozen + assert "external" not in frozen + + def test_subgraph_node_protected_from_removal(self) -> None: + """Subgraph nodes auto-frozen via DataNode cannot be removed.""" + from factory.outer_loop.engine import _auto_frozen_nodes + from factory.workflow.primitives import DataNode, DataItem + + nodes: dict[str, AgentNode | FnNode | DataNode] = { + "start": FnNode(id="start", command="echo start"), + "data_loader": DataNode( + id="data_loader", + inline_items=[DataItem(id="item1", prompt="test")], + subgraph_entry="sub_builder", + subgraph_exit="sub_builder", + ), + "sub_builder": AgentNode(id="sub_builder", role=AgentRole.BUILDER), + } + edges = [ + Edge(source="start", target="data_loader"), + Edge(source="data_loader", target="sub_builder"), + ] + wf = Workflow( + name="protected_subgraph", nodes=nodes, edges=edges, start_node="start", + ) + + # With DataNode present, sub_builder is auto-frozen + frozen = _auto_frozen_nodes(wf) + assert "sub_builder" in frozen + assert remove_node(wf, "sub_builder", frozen_nodes=frozen) is None + + # Without DataNode, sub_builder is NOT frozen and can be removed + nodes_no_data: dict[str, AgentNode | FnNode] = { + "start": FnNode(id="start", command="echo start"), + "sub_builder": AgentNode(id="sub_builder", role=AgentRole.BUILDER), + } + edges_no_data = [Edge(source="start", target="sub_builder")] + wf_no_data = Workflow( + name="no_data", nodes=nodes_no_data, edges=edges_no_data, + start_node="start", + ) + frozen_no_data = _auto_frozen_nodes(wf_no_data) + assert "sub_builder" not in frozen_no_data + result = remove_node(wf_no_data, "sub_builder", frozen_nodes=frozen_no_data) + assert result is not None From 0fe53d72c9e0a56167fc4b582462d3cadf3bc939 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Tue, 15 Sep 2026 15:48:34 +0000 Subject: [PATCH 26/30] =?UTF-8?q?fix:=20resolve=203=20CI=20failures=20?= =?UTF-8?q?=E2=80=94=20path=20resolution=20+=20NoveltyFilter=20GED=3D0=20l?= =?UTF-8?q?ogic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.6 (1M context) --- .factory/reviews/builder-latest.md | 35 +++++++++++++++++++----------- factory/outer_loop/similarity.py | 9 +++++++- tests/test_compose.py | 7 ++++-- 3 files changed, 35 insertions(+), 16 deletions(-) diff --git a/.factory/reviews/builder-latest.md b/.factory/reviews/builder-latest.md index a95832480..332b21500 100644 --- a/.factory/reviews/builder-latest.md +++ b/.factory/reviews/builder-latest.md @@ -1,17 +1,26 @@ -# Builder Report — Diagnostic Logging & AgentNode Loop Test +# Builder Agent Output -## Changes Made +- **timestamp:** 2026-09-15 +- **exit_code:** 0 +- **branch:** factory/run-e3ddbac6 +- **pr:** #1494 (existing — pushed fixes to branch) -### factory/workflow/executor.py -- **data_item_setup_complete log**: After `setup()` call, logs the files created in the workspace (item_id, workspace path, file count, first 20 files) -- **setup_read_path_mismatch warning**: During setup_reads rescan, detects when a declared read path doesn't match the actual file location by searching for the basename recursively -- **wait_for_reads_timeout_diagnostic warning**: When `_wait_for_reads` times out, logs what files DO exist in completed_files vs what's missing +## Changes -### tests/test_data_node.py -- **test_data_node_loop_with_agent_body**: Integration test for DataNode + Loop(AgentNode body, fn GateNode). Uses a mock agent_fn that appends to counter.txt, gate checks line count, verifies 3 iterations via RELOOP→PROCEED cycle -- **test_setup_read_path_mismatch_logs_warning**: Tests that a mismatched read path (file at `.factory/memory.md` but node reads `memory.md`) causes the inner executor to halt, with a fast timeout patch to avoid 60s CI delay +### Fix A — `tests/test_compose.py` (path resolution) +- Added module-level constant `_CHESS_EVOLVE_TOML` using `Path(__file__).resolve().parent.parent / ...` to resolve the chess-evolve.toml path absolutely (stable under pytest-xdist `-n auto` where CWD differs from repo root) +- Updated both `test_chess_evolve_toml_no_builder_required` and `test_chess_evolve_toml_passes_any_workflow` to use the constant instead of the relative path string -## Test Results -- All 69 tests in test_data_node.py pass -- All 747 workflow-related tests pass (8 skipped) -- No lint errors in modified files +### Fix B — `factory/outer_loop/similarity.py` (NoveltyFilter GED=0 logic) +- Modified the GED loop in `NoveltyFilter.is_novel()` to `continue` when `ged == 0` (identical topology) +- Rationale: When GED=0, topology is identical to an archived workflow. If the structural hash check above already passed (hash is novel), the difference must be content-only (prompts, params). Content-only mutations are intentionally novel — exact duplicates are caught by the hash dedup. The GED loop should only reject when topology distance is non-zero but below threshold. + +## Verification + +- 3 previously-failing tests now pass: + - `test_chess_evolve_toml_no_builder_required` ✅ + - `test_chess_evolve_toml_passes_any_workflow` ✅ + - `test_prompt_only_mutation_passes_is_novel` ✅ +- Full test suites pass with no regressions: + - `tests/test_compose.py`: 51/51 passed + - `tests/test_outer_loop/test_similarity.py`: 18/18 passed diff --git a/factory/outer_loop/similarity.py b/factory/outer_loop/similarity.py index 494567cb9..2713726d8 100644 --- a/factory/outer_loop/similarity.py +++ b/factory/outer_loop/similarity.py @@ -173,7 +173,14 @@ def is_novel(self, workflow: Workflow, threshold: int | None = None) -> bool: t = threshold if threshold is not None else self.min_edit_distance for archived in self._archived_workflows: - if archived.knob_values == workflow.knob_values and graph_edit_distance(workflow, archived) < t: + if archived.knob_values != workflow.knob_values: + continue + ged = graph_edit_distance(workflow, archived) + if ged == 0: + # Identical topology — if hash is novel (checked above), + # the difference is content-only (prompts, params) → novel + continue + if ged < t: return False return True diff --git a/tests/test_compose.py b/tests/test_compose.py index 6154d7b08..f61b2fd62 100644 --- a/tests/test_compose.py +++ b/tests/test_compose.py @@ -25,6 +25,9 @@ ) +_CHESS_EVOLVE_TOML = Path(__file__).resolve().parent.parent / "benchmarks" / "configs" / "chess-evolve.toml" + + # ── Capability StrEnum tests ──────────────────────────────────── @@ -271,7 +274,7 @@ def test_chess_evolve_toml_no_builder_required(self): """chess-evolve.toml with required_capabilities=[] should need no capabilities.""" from factory.task import TaskDefinition - defn = TaskDefinition.from_toml("benchmarks/configs/chess-evolve.toml") + defn = TaskDefinition.from_toml(_CHESS_EVOLVE_TOML) assert defn.constraints.required_capabilities == [] task = Task(definition=defn) caps = TaskCapabilities.from_task(task) @@ -281,7 +284,7 @@ def test_chess_evolve_toml_passes_any_workflow(self): """chess-evolve.toml should pass composition with a research-only workflow.""" from factory.task import TaskDefinition - defn = TaskDefinition.from_toml("benchmarks/configs/chess-evolve.toml") + defn = TaskDefinition.from_toml(_CHESS_EVOLVE_TOML) task = Task(definition=defn) wf = _make_workflow(researcher=True, name="research-only") validate_composition(wf, task) From 6d53e449f3aa5890b6ae1250cbad562f5b1cf448 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Tue, 15 Sep 2026 16:00:08 +0000 Subject: [PATCH 27/30] fix: remove unused VerdictType imports (ruff lint) Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/test_data_node.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_data_node.py b/tests/test_data_node.py index dc6e8c88c..f2c308b40 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -1530,7 +1530,7 @@ async def test_data_node_with_loop_subgraph(self, tmp_path: Path) -> None: """DataNode whose subgraph is a Loop should execute body 3 times via fn gate.""" from factory.workflow.executor import WorkflowExecutor from factory.workflow.package import Loop, Package - from factory.workflow.primitives import GateNode, VerdictType + from factory.workflow.primitives import GateNode project_path = tmp_path (project_path / ".factory").mkdir(parents=True, exist_ok=True) @@ -1611,7 +1611,7 @@ async def test_data_node_with_loop_subgraph(self, tmp_path: Path) -> None: def test_data_node_loop_wrong_exit_warns(self) -> None: """DataNode with subgraph_exit pointing to GateNode should produce a validation warning.""" from factory.workflow.package import Loop, Package - from factory.workflow.primitives import GateNode, VerdictType + from factory.workflow.primitives import GateNode body_node = FnNode( id="loop_body", @@ -1670,7 +1670,7 @@ def test_loop_package_compiled_preserves_edges(self) -> None: """Loop Package compiled into a Workflow preserves all 3 loop edges in subgraph.""" from factory.workflow.executor import _collect_subgraph_nodes from factory.workflow.package import Loop, Package - from factory.workflow.primitives import GateNode, VerdictType + from factory.workflow.primitives import GateNode body_node = FnNode( id="loop_body", From e918957ff57373dc5a9b1caf68ed1b9e3b6e8436 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Tue, 15 Sep 2026 16:03:54 +0000 Subject: [PATCH 28/30] fix: restore VerdictType import used by loop edge assertions Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/test_data_node.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_data_node.py b/tests/test_data_node.py index f2c308b40..634966aba 100644 --- a/tests/test_data_node.py +++ b/tests/test_data_node.py @@ -1670,7 +1670,7 @@ def test_loop_package_compiled_preserves_edges(self) -> None: """Loop Package compiled into a Workflow preserves all 3 loop edges in subgraph.""" from factory.workflow.executor import _collect_subgraph_nodes from factory.workflow.package import Loop, Package - from factory.workflow.primitives import GateNode + from factory.workflow.primitives import GateNode, VerdictType body_node = FnNode( id="loop_body", From cd3f11dcb26217725fe4be8d063aed421f8f04a0 Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Tue, 15 Sep 2026 16:26:40 +0000 Subject: [PATCH 29/30] =?UTF-8?q?fix:=20resolve=206=20mypy=20arg-type=20er?= =?UTF-8?q?rors=20in=20designer.py=20=E2=80=94=20use=20NodeType=20instead?= =?UTF-8?q?=20of=20object?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/outer_loop/designer.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/factory/outer_loop/designer.py b/factory/outer_loop/designer.py index 46837f5c7..9fb5e6fa0 100644 --- a/factory/outer_loop/designer.py +++ b/factory/outer_loop/designer.py @@ -19,6 +19,7 @@ Edge, FnNode, GateNode, + NodeType, Workflow, ) @@ -353,7 +354,7 @@ def propose( def _inject_frozen_nodes( - nodes: dict[str, object], + nodes: dict[str, NodeType], seed_workflow: Workflow | None, frozen_node_ids: set[str] | None, ) -> None: @@ -377,7 +378,7 @@ def _inject_frozen_nodes( def _rewire_data_nodes( - nodes: dict[str, object], + nodes: dict[str, NodeType], edges: list[Edge], original_start: str, seed_workflow: Workflow | None, From ed2f24b933f5438e713533be705def4ba41e961f Mon Sep 17 00:00:00 2001 From: Cole Hurwitz Date: Tue, 15 Sep 2026 16:49:25 +0000 Subject: [PATCH 30/30] =?UTF-8?q?fix:=20widen=20caller-side=20nodes=20anno?= =?UTF-8?q?tations=20to=20dict[str,=20NodeType]=20=E2=80=94=20resolve=20re?= =?UTF-8?q?maining=20mypy=20arg-type=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.6 (1M context) --- factory/outer_loop/designer.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/factory/outer_loop/designer.py b/factory/outer_loop/designer.py index 9fb5e6fa0..2727fdb43 100644 --- a/factory/outer_loop/designer.py +++ b/factory/outer_loop/designer.py @@ -43,7 +43,7 @@ def design_minimal( Structure: researcher → builder → gate """ - nodes: dict[str, AgentNode | FnNode | GateNode] = { + nodes: dict[str, NodeType] = { "researcher": AgentNode( id="researcher", role=AgentRole.RESEARCHER, @@ -101,7 +101,7 @@ def design_thorough( """ from factory.workflow.primitives import ForkNode, JoinNode - nodes: dict[str, AgentNode | FnNode | GateNode | ForkNode | JoinNode] = { + nodes: dict[str, NodeType] = { "study": FnNode( id="study", command="factory study {project_path}", @@ -216,7 +216,7 @@ def design_custom( raw_roles = constraints.get("require_roles", []) require_roles: list[object] = list(raw_roles) if isinstance(raw_roles, list) else [] - nodes: dict[str, AgentNode | FnNode | GateNode] = {} + nodes: dict[str, NodeType] = {} edges: list[Edge] = [] prev_id: str | None = None