From 61f8bd22774317ffeab19017c80811c789f2b5e4 Mon Sep 17 00:00:00 2001 From: Michael Kuehl Date: Mon, 20 Apr 2026 09:16:26 +0200 Subject: [PATCH] perf(engine): prioritize pre-review scheduling for critical-path specs (fixes #476) Two changes to surface blocking issues earlier: 1. Exempt reviewer:pre-review nodes from cross-spec dependency propagation in the graph builder. Pre-reviews validate spec content, not upstream implementation, so they can run before upstream specs complete. 2. Add two-tier priority scheduling in _interleave_by_spec: auto_pre nodes (group 0) run before coder nodes, sorted by spec fan-out descending so high-impact specs surface blockers first. --- agent_fox/engine/graph_sync.py | 132 +++++++--- agent_fox/graph/builder.py | 7 + .../engine/test_spec_fair_scheduling_props.py | 94 ++++--- .../unit/engine/test_spec_fair_scheduling.py | 246 ++++++++++++++++++ tests/unit/graph/test_builder.py | 47 ++-- 5 files changed, 438 insertions(+), 88 deletions(-) diff --git a/agent_fox/engine/graph_sync.py b/agent_fox/engine/graph_sync.py index 4281b337..78a79e42 100644 --- a/agent_fox/engine/graph_sync.py +++ b/agent_fox/engine/graph_sync.py @@ -34,53 +34,60 @@ def _spec_number(spec_name: str) -> tuple[int, str]: return (float("inf"), spec_name) # type: ignore[return-value] -def _interleave_by_spec( - ready: list[str], - duration_hints: dict[str, int] | None = None, -) -> list[str]: - """Order ready tasks with spec-fair round-robin interleaving. +def _is_auto_pre(node_id: str) -> bool: + """Check if a node is an auto_pre archetype (group 0). - 1. Group tasks by spec name (everything before first ':' in node ID). - 2. Sort spec groups by spec number ascending (numeric prefix). - 3. Within each group, sort by duration descending (if hints), else - alphabetically. Hinted tasks come before unhinted tasks. - 4. Interleave across groups: take one from each spec per round. + Group 0 is reserved for auto_pre archetype nodes (pre-review, + drift-review, skeptic, etc.). Coder groups start at 1. - Args: - ready: List of ready node IDs. - duration_hints: Optional mapping of node_id -> predicted duration ms. + Requirements: 69-REQ-1.1 + """ + parts = node_id.split(":") + return len(parts) >= 2 and parts[1] == "0" - Returns: - Spec-fair-ordered list of node IDs. - Requirements: 69-REQ-1.1, 69-REQ-1.3, 69-REQ-2.1, 69-REQ-2.2, 69-REQ-2.3 +def _spec_round_robin( + tasks: list[str], + duration_hints: dict[str, int] | None = None, + fan_out_weights: dict[str, int] | None = None, +) -> list[str]: + """Group by spec, sort within groups, and round-robin interleave. + + Args: + tasks: List of node IDs to interleave. + duration_hints: Optional per-node duration hints. + fan_out_weights: Optional per-spec fan-out weights. When + provided, specs are sorted by fan-out descending (highest + impact first) with ties broken by spec number ascending. """ - if not ready: + if not tasks: return [] - # Group tasks by spec name groups: dict[str, list[str]] = {} - for node_id in ready: + for node_id in tasks: spec = _spec_name(node_id) groups.setdefault(spec, []).append(node_id) - # Sort spec groups by spec number ascending - sorted_specs = sorted(groups.keys(), key=_spec_number) + if fan_out_weights: + sorted_specs = sorted( + groups.keys(), + key=lambda s: (-fan_out_weights.get(s, 0), *_spec_number(s)), + ) + else: + sorted_specs = sorted(groups.keys(), key=_spec_number) - # Sort within each group sorted_groups: list[list[str]] = [] for spec in sorted_specs: - tasks = groups[spec] + spec_tasks = groups[spec] if duration_hints: - hinted = [(t, duration_hints[t]) for t in tasks if t in duration_hints] - unhinted = [t for t in tasks if t not in duration_hints] + hinted = [(t, duration_hints[t]) for t in spec_tasks if t in duration_hints] + unhinted = [t for t in spec_tasks if t not in duration_hints] hinted.sort(key=lambda x: x[1], reverse=True) unhinted.sort() sorted_groups.append([t for t, _ in hinted] + unhinted) else: - sorted_groups.append(sorted(tasks)) + sorted_groups.append(sorted(spec_tasks)) - # Round-robin interleave across groups result: list[str] = [] queues = [list(g) for g in sorted_groups] while any(queues): @@ -91,6 +98,49 @@ def _interleave_by_spec( return result +def _interleave_by_spec( + ready: list[str], + duration_hints: dict[str, int] | None = None, + fan_out_weights: dict[str, int] | None = None, +) -> list[str]: + """Order ready tasks with pre-review priority and spec-fair interleaving. + + Partitions ready tasks into two tiers: + + 1. **Pre-review tier** (auto_pre nodes at group 0): sorted by spec + fan-out descending so critical-path specs surface blockers first. + 2. **Regular tier** (coder and post-review nodes): sorted by spec + number ascending with spec-fair round-robin interleaving. + + Within each tier, tasks are interleaved round-robin across spec + groups. + + Args: + ready: List of ready node IDs. + duration_hints: Optional mapping of node_id -> predicted duration ms. + fan_out_weights: Optional mapping of spec_name -> fan-out weight + (count of distinct downstream specs). + + Returns: + Pre-review-prioritized, spec-fair-ordered list of node IDs. + + Requirements: 69-REQ-1.1, 69-REQ-1.3, 69-REQ-2.1, 69-REQ-2.2, 69-REQ-2.3 + """ + if not ready: + return [] + + pre = [n for n in ready if _is_auto_pre(n)] + regular = [n for n in ready if not _is_auto_pre(n)] + + result: list[str] = [] + if pre: + result.extend(_spec_round_robin(pre, duration_hints, fan_out_weights)) + if regular: + result.extend(_spec_round_robin(regular, duration_hints)) + + return result + + class GraphSync: """Graph state propagation: ready detection, cascade blocking. @@ -137,6 +187,10 @@ def ready_tasks( - Its status is ``pending`` - All of its dependencies have status ``completed`` + Pre-review nodes (auto_pre at group 0) are prioritized ahead of + coder nodes, with high-fan-out specs ordered first so that + critical-path blockers surface early. + Args: duration_hints: Optional mapping of node_id to predicted duration in milliseconds. When provided, ready tasks are @@ -145,10 +199,8 @@ def ready_tasks( of duration hints. Returns: - List of ready node_ids in spec-fair round-robin order. - Spec groups are ordered by numeric prefix ascending; within - each spec group tasks are sorted alphabetically or by - duration descending when hints are provided. + List of ready node_ids in pre-review-prioritized, + spec-fair round-robin order. Requirements: 39-REQ-1.1, 39-REQ-1.3, 69-REQ-1.1, 69-REQ-2.2 """ @@ -160,7 +212,23 @@ def ready_tasks( if all(self.node_states.get(d) == "completed" for d in deps): ready.append(node_id) - return _interleave_by_spec(ready, duration_hints) + fan_out = self._compute_spec_fan_out() + return _interleave_by_spec(ready, duration_hints, fan_out) + + def _compute_spec_fan_out(self) -> dict[str, int]: + """Count distinct cross-spec dependent specs. + + For each spec, count how many OTHER specs have at least one + node that depends on a node in this spec. + """ + spec_dependents: dict[str, set[str]] = {} + for node_id, dependents in self._dependents.items(): + src_spec = _spec_name(node_id) + for dep_id in dependents: + dep_spec = _spec_name(dep_id) + if dep_spec != src_spec: + spec_dependents.setdefault(src_spec, set()).add(dep_spec) + return {spec: len(deps) for spec, deps in spec_dependents.items()} def predecessors(self, node_id: str) -> list[str]: """Return predecessor node IDs for *node_id*.""" diff --git a/agent_fox/graph/builder.py b/agent_fox/graph/builder.py index de54f62a..69441624 100644 --- a/agent_fox/graph/builder.py +++ b/agent_fox/graph/builder.py @@ -235,9 +235,16 @@ def _add_cross_spec_edges( # Propagate to direct intra-spec predecessors that are review nodes. # This covers auto_pre nodes (skeptic/oracle) that gate the target. + # + # Exception: reviewer:pre-review nodes are exempt — they validate + # spec content (requirements, design), not upstream implementation, + # so they can run before upstream specs complete. Running them + # early surfaces blockers before coder work begins (fixes #476). for pred_id in intra_preds.get(target_id, []): pred_node = nodes.get(pred_id) if pred_node is not None and pred_node.archetype != "coder": + if pred_node.archetype == "reviewer" and pred_node.mode == "pre-review": + continue edges.append(Edge(source=source_id, target=pred_id, kind="cross_spec")) return edges diff --git a/tests/property/engine/test_spec_fair_scheduling_props.py b/tests/property/engine/test_spec_fair_scheduling_props.py index 5efa425e..ca0e80a7 100644 --- a/tests/property/engine/test_spec_fair_scheduling_props.py +++ b/tests/property/engine/test_spec_fair_scheduling_props.py @@ -11,7 +11,7 @@ from hypothesis import given, settings from hypothesis import strategies as st -from agent_fox.engine.graph_sync import _interleave_by_spec, _spec_name +from agent_fox.engine.graph_sync import _interleave_by_spec, _is_auto_pre, _spec_name # --------------------------------------------------------------------------- # Hypothesis strategies @@ -156,7 +156,10 @@ def _spec_number(spec_name: str) -> tuple[int | float, str]: class TestFairnessGuarantee: - """Property test: every spec's first task appears in the first N positions. + """Property test: within each tier, every spec's first task appears in the first N positions. + + Pre-review nodes (group 0) form a separate priority tier. Fairness + is guaranteed within each tier independently. Test Spec: TS-69-P1 Properties: Property 1 from design.md @@ -166,18 +169,23 @@ class TestFairnessGuarantee: @given(ready=multi_spec_list(min_specs=2, max_specs=10)) @settings(max_examples=200) def test_fairness_guarantee(self, ready: list[str]) -> None: - """Every spec's first task must appear within the first N positions.""" + """Within each tier, every spec's first task appears in the first N positions.""" result = _interleave_by_spec(ready) - specs = list({_spec_name(nid) for nid in ready}) - n = len(specs) - - for spec in specs: - first_index = next(i for i, nid in enumerate(result) if _spec_name(nid) == spec) - assert first_index < n, ( - f"Spec '{spec}' first appears at index {first_index}, " - f"but should appear within first {n} positions. " - f"Result: {result}" - ) + + for tier_filter, label in [(_is_auto_pre, "pre"), (lambda n: not _is_auto_pre(n), "regular")]: + tier_tasks = [nid for nid in result if tier_filter(nid)] + specs = list({_spec_name(nid) for nid in tier_tasks}) + n = len(specs) + if n == 0: + continue + + for spec in specs: + first_index = next(i for i, nid in enumerate(tier_tasks) if _spec_name(nid) == spec) + assert first_index < n, ( + f"[{label}] Spec '{spec}' first appears at index {first_index}, " + f"but should appear within first {n} positions. " + f"Tier tasks: {tier_tasks}" + ) # --------------------------------------------------------------------------- @@ -207,7 +215,7 @@ def test_single_spec_identity(self, ready: list[str]) -> None: class TestDurationPreservesWithinSpecOrder: - """Property test: within each spec, tasks are in duration-descending order. + """Property test: within each spec and tier, tasks are in duration-descending order. Test Spec: TS-69-P3 Properties: Property 3 from design.md @@ -217,34 +225,35 @@ class TestDurationPreservesWithinSpecOrder: @given(spec_list_with_hints()) @settings(max_examples=200) def test_duration_preserves_within_spec_order(self, args: tuple[list[str], dict[str, int]]) -> None: - """Within each spec, tasks are ordered by duration descending in result.""" + """Within each spec and tier, tasks are ordered by duration descending.""" ready, hints = args if not ready: return result = _interleave_by_spec(ready, duration_hints=hints if hints else None) - specs = list({_spec_name(nid) for nid in ready}) - - for spec in specs: - spec_tasks_in_result = [nid for nid in result if _spec_name(nid) == spec] - # Hinted tasks should come before unhinted, hinted in descending order - hinted = [(nid, hints[nid]) for nid in spec_tasks_in_result if nid in hints] - durations = [d for _, d in hinted] - assert durations == sorted(durations, reverse=True), ( - f"Spec '{spec}' hinted tasks not in descending duration order: " - f"{hinted} in result {spec_tasks_in_result}" - ) - # All hinted tasks should appear before unhinted tasks - unhinted_indices = [i for i, nid in enumerate(spec_tasks_in_result) if nid not in hints] - hinted_indices = [i for i, nid in enumerate(spec_tasks_in_result) if nid in hints] - if hinted_indices and unhinted_indices: - assert max(hinted_indices) < min(unhinted_indices), ( - f"Spec '{spec}': hinted tasks don't all precede unhinted tasks. " - f"Hinted indices: {hinted_indices}, " - f"unhinted indices: {unhinted_indices}" + for tier_filter in [_is_auto_pre, lambda n: not _is_auto_pre(n)]: + tier_tasks = [nid for nid in result if tier_filter(nid)] + specs = list({_spec_name(nid) for nid in tier_tasks}) + + for spec in specs: + spec_tasks_in_result = [nid for nid in tier_tasks if _spec_name(nid) == spec] + hinted = [(nid, hints[nid]) for nid in spec_tasks_in_result if nid in hints] + durations = [d for _, d in hinted] + assert durations == sorted(durations, reverse=True), ( + f"Spec '{spec}' hinted tasks not in descending duration order: " + f"{hinted} in result {spec_tasks_in_result}" ) + unhinted_indices = [i for i, nid in enumerate(spec_tasks_in_result) if nid not in hints] + hinted_indices = [i for i, nid in enumerate(spec_tasks_in_result) if nid in hints] + if hinted_indices and unhinted_indices: + assert max(hinted_indices) < min(unhinted_indices), ( + f"Spec '{spec}': hinted tasks don't all precede unhinted tasks. " + f"Hinted indices: {hinted_indices}, " + f"unhinted indices: {unhinted_indices}" + ) + # --------------------------------------------------------------------------- # TS-69-P4: Completeness @@ -285,7 +294,11 @@ def test_completeness(self, ready: list[str]) -> None: class TestSpecOrderConsistency: - """Property test: lower-numbered specs appear before higher-numbered specs. + """Property test: within the regular tier, lower-numbered specs appear first. + + Pre-review nodes (group 0) form a separate priority tier and are + exempt from spec-number ordering (they use fan-out ordering instead). + The spec-number consistency property applies to the regular tier. Test Spec: TS-69-P5 Properties: Property 5 from design.md @@ -295,14 +308,17 @@ class TestSpecOrderConsistency: @given(ready=multi_spec_list(min_specs=2, max_specs=10)) @settings(max_examples=200) def test_spec_order_consistency(self, ready: list[str]) -> None: - """For specs A < B by number, A's first task appears before B's first task.""" + """Within the regular tier, A < B by number ⇒ A's first task before B's.""" result = _interleave_by_spec(ready) - specs = list({_spec_name(nid) for nid in ready}) + regular_result = [nid for nid in result if not _is_auto_pre(nid)] + specs = list({_spec_name(nid) for nid in regular_result}) + if len(specs) < 2: + return specs_sorted = sorted(specs, key=_spec_number) first_indices: dict[str, int] = {} for spec in specs_sorted: - first_indices[spec] = next(i for i, nid in enumerate(result) if _spec_name(nid) == spec) + first_indices[spec] = next(i for i, nid in enumerate(regular_result) if _spec_name(nid) == spec) for i in range(len(specs_sorted)): for j in range(i + 1, len(specs_sorted)): @@ -312,7 +328,7 @@ def test_spec_order_consistency(self, ready: list[str]) -> None: f"Spec '{spec_a}' (number {_spec_number(spec_a)}) first appears " f"at index {first_indices[spec_a]}, but spec '{spec_b}' " f"(number {_spec_number(spec_b)}) first appears at " - f"index {first_indices[spec_b]}. Result: {result}" + f"index {first_indices[spec_b]}. Regular result: {regular_result}" ) diff --git a/tests/unit/engine/test_spec_fair_scheduling.py b/tests/unit/engine/test_spec_fair_scheduling.py index da7d1adb..52537d9d 100644 --- a/tests/unit/engine/test_spec_fair_scheduling.py +++ b/tests/unit/engine/test_spec_fair_scheduling.py @@ -156,6 +156,252 @@ def test_ready_tasks_multi_spec_graph(self) -> None: # --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- +# Pre-review priority scheduling (fixes #476) +# --------------------------------------------------------------------------- + + +class TestPreReviewPriority: + """Tests for pre-review prioritization in _interleave_by_spec. + + Pre-review nodes (auto_pre at group 0) are placed before coder nodes + in the ready queue so that blockers surface early. + """ + + def test_pre_reviews_before_coder_nodes(self) -> None: + """Auto_pre review nodes appear before coder nodes.""" + from agent_fox.engine.graph_sync import _interleave_by_spec + + ready = [ + "65_foo:1", + "65_foo:0:reviewer:pre-review", + "67_bar:1", + "67_bar:0:reviewer:pre-review", + ] + result = _interleave_by_spec(ready) + pre_reviews = [n for n in result if ":0:" in n] + coders = [n for n in result if ":0:" not in n] + assert result == pre_reviews + coders + + def test_pre_review_fan_out_ordering(self) -> None: + """High-fan-out specs' pre-reviews come first.""" + from agent_fox.engine.graph_sync import _interleave_by_spec + + ready = [ + "01_setup:0:reviewer:pre-review", + "02_broker:0:reviewer:pre-review", + "03_api:0:reviewer:pre-review", + ] + fan_out = {"02_broker": 4, "01_setup": 1, "03_api": 2} + result = _interleave_by_spec(ready, fan_out_weights=fan_out) + assert result[0] == "02_broker:0:reviewer:pre-review" + assert result[1] == "03_api:0:reviewer:pre-review" + assert result[2] == "01_setup:0:reviewer:pre-review" + + def test_fan_out_tie_breaks_by_spec_number(self) -> None: + """Equal fan-out ties are broken by spec number ascending.""" + from agent_fox.engine.graph_sync import _interleave_by_spec + + ready = [ + "03_api:0:reviewer:pre-review", + "01_setup:0:reviewer:pre-review", + ] + fan_out = {"01_setup": 2, "03_api": 2} + result = _interleave_by_spec(ready, fan_out_weights=fan_out) + assert result == [ + "01_setup:0:reviewer:pre-review", + "03_api:0:reviewer:pre-review", + ] + + def test_mixed_pre_review_and_coder_ordering(self) -> None: + """Pre-reviews from all specs come before any coder node.""" + from agent_fox.engine.graph_sync import _interleave_by_spec + + ready = [ + "01_setup:1", + "02_broker:0:reviewer:pre-review", + "03_api:0:reviewer:pre-review", + "01_setup:2", + ] + result = _interleave_by_spec(ready) + # Pre-reviews first (round-robin: 02, 03) + assert result[:2] == [ + "02_broker:0:reviewer:pre-review", + "03_api:0:reviewer:pre-review", + ] + # Then coder nodes (round-robin: 01:1, 01:2) + assert result[2:] == ["01_setup:1", "01_setup:2"] + + def test_drift_review_also_prioritized(self) -> None: + """Drift-review (also auto_pre at group 0) is prioritized with pre-reviews.""" + from agent_fox.engine.graph_sync import _interleave_by_spec + + ready = [ + "01_setup:1", + "02_broker:0:reviewer:drift-review", + "02_broker:0:reviewer:pre-review", + ] + result = _interleave_by_spec(ready) + assert result[0].startswith("02_broker:0:") + assert result[1].startswith("02_broker:0:") + assert result[2] == "01_setup:1" + + def test_single_auto_pre_node_id_format(self) -> None: + """Single auto_pre with 'spec:0' format is also prioritized.""" + from agent_fox.engine.graph_sync import _interleave_by_spec + + ready = ["01_setup:1", "02_broker:0", "03_api:1"] + result = _interleave_by_spec(ready) + assert result[0] == "02_broker:0" + + +class TestIsAutoPre: + """Tests for the _is_auto_pre() helper.""" + + def test_suffixed_pre_review(self) -> None: + from agent_fox.engine.graph_sync import _is_auto_pre + + assert _is_auto_pre("02_broker:0:reviewer:pre-review") is True + + def test_suffixed_drift_review(self) -> None: + from agent_fox.engine.graph_sync import _is_auto_pre + + assert _is_auto_pre("02_broker:0:reviewer:drift-review") is True + + def test_single_auto_pre(self) -> None: + from agent_fox.engine.graph_sync import _is_auto_pre + + assert _is_auto_pre("02_broker:0") is True + + def test_coder_group(self) -> None: + from agent_fox.engine.graph_sync import _is_auto_pre + + assert _is_auto_pre("02_broker:1") is False + + def test_audit_review(self) -> None: + from agent_fox.engine.graph_sync import _is_auto_pre + + assert _is_auto_pre("02_broker:3:reviewer:audit-review") is False + + def test_no_colon(self) -> None: + from agent_fox.engine.graph_sync import _is_auto_pre + + assert _is_auto_pre("orphan_node") is False + + +class TestSpecFanOut: + """Tests for GraphSync._compute_spec_fan_out().""" + + def test_fan_out_with_cross_spec_edges(self) -> None: + """Specs with cross-spec dependents have non-zero fan-out.""" + from agent_fox.engine.graph_sync import GraphSync + + states = { + "01_setup:1": "pending", + "01_setup:2": "pending", + "02_broker:1": "pending", + "03_api:1": "pending", + } + edges = { + "01_setup:1": [], + "01_setup:2": ["01_setup:1"], + "02_broker:1": ["01_setup:2"], + "03_api:1": ["01_setup:2"], + } + gs = GraphSync(states, edges) + fan_out = gs._compute_spec_fan_out() + assert fan_out["01_setup"] == 2 # 02_broker and 03_api depend on it + + def test_fan_out_no_cross_spec(self) -> None: + """No cross-spec edges means empty fan-out.""" + from agent_fox.engine.graph_sync import GraphSync + + states = {"01_a:1": "pending", "01_a:2": "pending"} + edges = {"01_a:1": [], "01_a:2": ["01_a:1"]} + gs = GraphSync(states, edges) + fan_out = gs._compute_spec_fan_out() + assert fan_out == {} + + def test_fan_out_counts_distinct_specs(self) -> None: + """Multiple edges to same spec count as 1.""" + from agent_fox.engine.graph_sync import GraphSync + + states = { + "01_a:1": "pending", + "01_a:2": "pending", + "02_b:1": "pending", + "02_b:2": "pending", + } + edges = { + "01_a:1": [], + "01_a:2": ["01_a:1"], + "02_b:1": ["01_a:1"], + "02_b:2": ["01_a:2"], + } + gs = GraphSync(states, edges) + fan_out = gs._compute_spec_fan_out() + assert fan_out["01_a"] == 1 # only 02_b, counted once + + +class TestReadyTasksPreReviewIntegration: + """Integration tests for ready_tasks() with pre-review priority.""" + + def test_pre_reviews_ordered_first(self) -> None: + """Pre-review nodes appear before coder nodes in ready_tasks().""" + from agent_fox.engine.graph_sync import GraphSync + + states = { + "01_setup:0:reviewer:pre-review": "pending", + "01_setup:1": "pending", + "02_broker:0:reviewer:pre-review": "pending", + "02_broker:1": "pending", + } + edges = { + "01_setup:0:reviewer:pre-review": [], + "01_setup:1": ["01_setup:0:reviewer:pre-review"], + "02_broker:0:reviewer:pre-review": [], + "02_broker:1": ["02_broker:0:reviewer:pre-review"], + } + gs = GraphSync(states, edges) + result = gs.ready_tasks() + # Only pre-reviews are ready (coders depend on them) + assert result == [ + "01_setup:0:reviewer:pre-review", + "02_broker:0:reviewer:pre-review", + ] + + def test_fan_out_affects_pre_review_order(self) -> None: + """High-fan-out specs' pre-reviews ordered first in ready_tasks().""" + from agent_fox.engine.graph_sync import GraphSync + + states = { + "01_setup:0:reviewer:pre-review": "pending", + "01_setup:1": "pending", + "02_broker:0:reviewer:pre-review": "pending", + "02_broker:1": "pending", + "03_api:0:reviewer:pre-review": "pending", + "03_api:1": "pending", + } + edges = { + "01_setup:0:reviewer:pre-review": [], + "01_setup:1": ["01_setup:0:reviewer:pre-review"], + "02_broker:0:reviewer:pre-review": [], + "02_broker:1": ["02_broker:0:reviewer:pre-review", "01_setup:1"], + "03_api:0:reviewer:pre-review": [], + "03_api:1": ["03_api:0:reviewer:pre-review", "01_setup:1"], + } + gs = GraphSync(states, edges) + result = gs.ready_tasks() + # 01_setup has fan-out 2 (02_broker and 03_api depend on it) + # Pre-reviews: 01 first (highest fan-out), then 02, then 03 + assert result[0] == "01_setup:0:reviewer:pre-review" + + +# --------------------------------------------------------------------------- +# TS-69-E1 through TS-69-E4: Edge cases +# --------------------------------------------------------------------------- + + class TestEdgeCases: """Edge case tests for spec-fair scheduling. diff --git a/tests/unit/graph/test_builder.py b/tests/unit/graph/test_builder.py index d3df1794..c1e0b966 100644 --- a/tests/unit/graph/test_builder.py +++ b/tests/unit/graph/test_builder.py @@ -290,16 +290,21 @@ def test_group_level_enables_earlier_scheduling(self) -> None: class TestCrossSpecEdgePropagationToReviewNodes: """Cross-spec edges propagate to auto_pre review predecessors of the target. - When a cross-spec dependency targets coder group N, the auto_pre review - node (reviewer:pre-review / reviewer:drift-review) that gates group N - should also receive a cross-spec edge from the same source. This prevents - the review node from running before the dependency is met. + When a cross-spec dependency targets coder group N, auto_pre review + nodes (e.g. drift-review, skeptic) that gate group N receive a + cross-spec edge from the same source — preventing them from running + before the dependency is met. + + Exception: reviewer:pre-review nodes are exempt from propagation. + Pre-reviews validate spec content (requirements, design), not + upstream implementation, so they run early to surface blockers. Fixes: https://github.com/agent-fox-dev/agent-fox/issues/337 + Updates: https://github.com/agent-fox-dev/agent-fox/issues/476 """ - def test_pre_review_receives_cross_spec_edge(self) -> None: - """Reviewer:pre-review (group 0) gets a cross-spec edge when group 1 has one.""" + def test_pre_review_exempt_from_cross_spec_propagation(self) -> None: + """Reviewer:pre-review does NOT get cross-spec edge (fixes #476).""" from agent_fox.core.config import ArchetypesConfig specs = [ @@ -322,7 +327,6 @@ def test_pre_review_receives_cross_spec_edge(self) -> None: graph = build_graph(specs, groups, cross_deps, archetypes_config=config) - # Find the pre-review node for beta spec pre_review_nodes = [ n for n in graph.nodes.values() @@ -331,7 +335,7 @@ def test_pre_review_receives_cross_spec_edge(self) -> None: assert len(pre_review_nodes) >= 1 pre_node = pre_review_nodes[0] pre_preds = graph.predecessors(pre_node.id) - assert "01_alpha:2" in pre_preds + assert "01_alpha:2" not in pre_preds def test_original_cross_spec_edge_preserved(self) -> None: """The declared cross-spec edge to the coder group is still present.""" @@ -385,8 +389,8 @@ def test_no_propagation_without_archetypes(self) -> None: reviewer_nodes = [n for n in graph.nodes.values() if n.archetype == "reviewer"] assert len(reviewer_nodes) == 0 - def test_propagation_with_suffixed_auto_pre_ids(self) -> None: - """Propagation works with suffixed IDs (e.g. spec:0:reviewer:pre-review).""" + def test_drift_review_receives_cross_spec_edge(self) -> None: + """Drift-review still gets cross-spec edge (non-exempt review archetype).""" from agent_fox.core.config import ArchetypesConfig specs = [ @@ -409,15 +413,25 @@ def test_propagation_with_suffixed_auto_pre_ids(self) -> None: graph = build_graph(specs, groups, cross_deps, archetypes_config=config) - # Find all auto_pre reviewer nodes for beta + # Drift-review should still get the cross-spec edge + drift_nodes = [ + n + for n in graph.nodes.values() + if n.spec_name == "02_beta" and n.archetype == "reviewer" and n.mode == "drift-review" + ] + for drift_node in drift_nodes: + preds = graph.predecessors(drift_node.id) + assert "01_alpha:2" in preds + + # Pre-review should NOT get the cross-spec edge pre_nodes = [ n for n in graph.nodes.values() - if n.spec_name == "02_beta" and n.archetype == "reviewer" and n.mode in ("pre-review", "drift-review") + if n.spec_name == "02_beta" and n.archetype == "reviewer" and n.mode == "pre-review" ] for pre_node in pre_nodes: preds = graph.predecessors(pre_node.id) - assert "01_alpha:2" in preds + assert "01_alpha:2" not in preds def test_no_propagation_to_non_predecessor_groups(self) -> None: """Cross-spec edges don't propagate to groups that aren't predecessors.""" @@ -461,8 +475,8 @@ def test_no_propagation_to_non_predecessor_groups(self) -> None: preds = graph.predecessors(pre_node.id) assert "01_alpha:3" not in preds - def test_propagation_for_dep_on_first_group(self) -> None: - """Cross-spec dep on group 1 propagates to the reviewer:pre-review at group 0.""" + def test_pre_review_exempt_for_dep_on_first_group(self) -> None: + """Cross-spec dep on group 1 does NOT propagate to pre-review (fixes #476).""" from agent_fox.core.config import ArchetypesConfig specs = [ @@ -485,7 +499,6 @@ def test_propagation_for_dep_on_first_group(self) -> None: graph = build_graph(specs, groups, cross_deps, archetypes_config=config) - # Find pre-review node for beta pre_review_nodes = [ n for n in graph.nodes.values() @@ -494,7 +507,7 @@ def test_propagation_for_dep_on_first_group(self) -> None: assert len(pre_review_nodes) >= 1 pre_node = pre_review_nodes[0] pre_preds = graph.predecessors(pre_node.id) - assert "01_alpha:2" in pre_preds + assert "01_alpha:2" not in pre_preds class TestDanglingCrossSpecRef: