Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/ai_company/engine/coordination/models.py (1)

146-190: ⚠️ Potential issue | 🟠 Major

Reject nested results from a different parent task.

CoordinationResult only validates topology today, so a decomposition_result, routing_result, or status_rollup from another parent task is accepted and then reported under this aggregate. That can corrupt rollups and parent updates for the wrong task. The new acceptance test in tests/unit/engine/test_coordination_models.py should flip once this validator exists.

Suggested fix

     `@model_validator`(mode="after")
     def _validate_topology_resolved(self) -> Self:
         """Ensure topology is resolved (not AUTO) in final result."""
         if self.topology == CoordinationTopology.AUTO:
             msg = "CoordinationResult topology must be resolved, not AUTO"
             raise ValueError(msg)
         return self
+
+    `@model_validator`(mode="after")
+    def _validate_parent_task_ids(self) -> Self:
+        """Ensure nested results belong to the same parent task."""
+        if (
+            self.decomposition_result is not None
+            and self.decomposition_result.plan.parent_task_id != self.parent_task_id
+        ):
+            msg = "decomposition_result parent_task_id must match parent_task_id"
+            raise ValueError(msg)
+        if (
+            self.routing_result is not None
+            and self.routing_result.parent_task_id != self.parent_task_id
+        ):
+            msg = "routing_result parent_task_id must match parent_task_id"
+            raise ValueError(msg)
+        if (
+            self.status_rollup is not None
+            and self.status_rollup.parent_task_id != self.parent_task_id
+        ):
+            msg = "status_rollup parent_task_id must match parent_task_id"
+            raise ValueError(msg)
+        return self

As per coding guidelines, "Validate: at system boundaries (user input, external APIs, config files)".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ai_company/engine/coordination/models.py` around lines 146 - 190, Add
validation to CoordinationResult to reject nested results that reference a
different parent task: in the existing model_validator (or a new after-mode
validator) for CoordinationResult (method _validate_topology_resolved), check
any present decomposition_result.parent_task_id, routing_result.parent_task_id,
and status_rollup.parent_task_id (and workspace_merge.parent_task_id if
applicable) and raise ValueError if any of them do not equal
self.parent_task_id; keep the existing topology AUTO check and return self when
all validations pass.

tests/unit/engine/test_coordination_dispatchers.py (1)

129-134: ⚠️ Potential issue | 🟡 Minor

Pass the dispatcher topology into make_routing().

These SAS/decentralized/context-dependent cases still rely on make_routing()'s default CENTRALIZED metadata. That weakens coverage and can hide mismatches between the selected dispatcher and the routing result. Apply the same fix to the other non-centralized call sites in this file.

Representative fix

         routing = make_routing(
             [
                 ("sub-a", "alice"),
                 ("sub-b", "alice"),
-            ]
+            ],
+            topology=CoordinationTopology.SAS,
         )

Also applies to: 394-399, 489-494

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/engine/test_coordination_dispatchers.py` around lines 129 - 134,
The tests call make_routing(...) without passing the dispatcher topology, so
they default to CENTRALIZED and miss coverage for non-centralized cases; update
each call (e.g., the routing assignment in test_coordination_dispatchers.py) to
pass the appropriate topology argument (use the dispatcher topology constant
instead of default CENTRALIZED) so make_routing(subscriptions,
topology=YOUR_TOPOLOGY) matches the dispatcher under test; apply the same change
to the other non-centralized call sites in this file (the other make_routing
invocations that should not use CENTRALIZED).

tests/unit/engine/test_coordination_service.py (1)

250-296: ⚠️ Potential issue | 🟠 Major

Don't execute a dependent wave after its prerequisite fails.

sub_b depends on sub-a, but this test still expects wave 1 to run after wave 0 failed. That turns dependency edges into ordering-only hints and can produce invalid downstream work. If the goal is to cover non-fail_fast continuation, make the subtasks independent; otherwise assert the dependent wave is skipped/blocked.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/engine/test_coordination_service.py` around lines 250 - 296, The
test currently defines sub_b with a dependency on sub_a (make_subtask("sub-b",
dependencies=("sub-a",))) but then expects wave 1 to run after wave 0 fails;
either make the tasks independent or change assertions. Fix by making sub_b
independent (remove the dependencies argument) and update the decomposition to
use an independent structure (e.g., TaskStructure.PARALLEL in the
make_decomposition call) so the test validates non-fail_fast continuation; keep
the rest of the setup (routing, exec_results,
CoordinationConfig(fail_fast=False), and assertions) unchanged.

src/ai_company/engine/coordination/group_builder.py (1)

96-102: ⚠️ Potential issue | 🟠 Major

Log the routing/decomposition mismatch before raising.

This still raises CoordinationError without a WARNING, so stale routing output is invisible in coordination telemetry. Log wave_index and subtask_id before raising.

Suggested fix

 from ai_company.observability.events.coordination import (
+    COORDINATION_PHASE_FAILED,
     COORDINATION_WAVE_BUILT,
 )
@@
             if task is None:
                 msg = (
                     f"Subtask {subtask_id!r} has a routing decision "
                     "but no corresponding created task in decomposition"
                 )
+                logger.warning(
+                    COORDINATION_PHASE_FAILED,
+                    phase="build_execution_waves",
+                    wave_index=wave_idx,
+                    subtask_id=subtask_id,
+                    error=msg,
+                )
                 raise CoordinationError(msg)

As per coding guidelines, "All error paths must log at WARNING or ERROR with context before raising" and "Validate: at system boundaries (user input, external APIs, config files)".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ai_company/engine/coordination/group_builder.py` around lines 96 - 102,
The code in group_builder.py raises CoordinationError when a routing decision
references a missing task but does not log context; before raising in the block
that checks task_lookup.get(subtask_id) == None, add a warning log (e.g., using
the module logger or the existing coordination logger) that includes wave_index
and subtask_id and any other relevant context (like the routing entry or
decomposition id) so stale routing output is visible in telemetry, then raise
CoordinationError as before; update the logging call adjacent to the check in
the function/method that builds groups (the block that constructs msg and raises
CoordinationError) to emit logger.warning(...) with wave_index and subtask_id.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/unit/engine/test_coordination_dispatchers.py (1)

709-742: ⚠️ Potential issue | 🟠 Major

Don't lock in downstream execution after a prerequisite wave failed.

sub_b explicitly depends on sub-a, but this test asserts that wave 1 still runs after wave 0 raised. That turns dependency edges into ordering-only hints and will mask the real fix if blocked descendants are enforced later. If the goal is to cover non-fail_fast continuation, make the second subtask independent; otherwise assert that the dependent wave is skipped/blocked.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/engine/test_coordination_dispatchers.py` around lines 709 - 742,
The test test_exception_with_fail_fast_off_continues currently makes sub_b
depend on sub_a (via make_subtask("sub-b", dependencies=("sub-a",))) but then
asserts wave 1 still ran after wave 0 failed, which is inconsistent with real
dependency enforcement; either make the second subtask independent (remove
dependencies from make_subtask for "sub-b") to test non-fail_fast continuation,
or keep the dependency and change the assertions to expect the dependent wave to
be skipped/blocked (e.g., result.waves[1].execution_result is None and that
execute_group was not invoked for blocked wave); update the setup using
make_subtask/make_decomposition/make_routing accordingly and adjust assertions
for SasDispatcher dispatch with CoordinationConfig(fail_fast=False).

docs/design/engine.md (1)

802-804: ⚠️ Potential issue | 🟡 Minor

Add validate to the pipeline diagram.

The numbered list and implementation both have a dedicated validation step, but the diagram jumps straight from topology resolution to dispatch.

Suggested diff

-decompose → route → resolve topology → dispatch → rollup → update parent
+decompose → route → resolve topology → validate → dispatch → rollup → update parent

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/design/engine.md` around lines 802 - 804, The pipeline diagram string
"decompose → route → resolve topology → dispatch → rollup → update parent" is
missing the dedicated validation step; update that diagram to insert "validate"
after "resolve topology" so it reads "decompose → route → resolve topology →
validate → dispatch → rollup → update parent" to match the numbered list and
implementation (look for the pipeline text in the docs/design/engine.md
content).

tests/unit/engine/test_coordination_models.py (1)

375-406: ⚠️ Potential issue | 🟠 Major

Don't codify foreign parent IDs as valid.

This test blesses attaching a status_rollup from "other-task" to CoordinationResult(parent_task_id="task-1"). That lets another task's nested coordination data be associated with the wrong aggregate result. Please invert this into a ValidationError case once CoordinationResult validates nested parent_task_ids. As per coding guidelines, "Validate: at system boundaries (user input, external APIs, config files)".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/engine/test_coordination_models.py` around lines 375 - 406, The
test test_mismatched_parent_task_id_accepted currently asserts that a
CoordinationResult can accept a SubtaskStatusRollup with a different
parent_task_id; change it to assert that providing a rollup whose parent_task_id
doesn't match CoordinationResult.parent_task_id raises a ValidationError once
CoordinationResult enforces cross-validation. Update the test to construct the
same SubtaskStatusRollup and CoordinationResult inputs but wrap the
creation/validation of CoordinationResult in a pytest.raises(ValidationError)
(or the project's equivalent validation exception) and assert the exception is
raised; reference CoordinationResult and SubtaskStatusRollup to locate the code
under test.

src/ai_company/engine/coordination/service.py (3)

139-148: ⚠️ Potential issue | 🟠 Major

Wrap dispatch failures as a real coordination phase error.

select_dispatcher() and dispatcher.dispatch() can still raise here — for example, decentralized precondition failures currently bubble as raw exceptions. That means callers lose a dispatch phase entry and partial_phases on the pipeline's most failure-prone step. Please funnel this block through a dedicated dispatch-phase helper that appends a failed CoordinationPhaseResult before re-raising CoordinationPhaseError.

Also applies to: 176-184

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ai_company/engine/coordination/service.py` around lines 139 - 148, The
dispatch block should be wrapped in a helper that runs select_dispatcher(...)
and await dispatcher.dispatch(...), catching any Exception, creating and
appending a failed CoordinationPhaseResult for the "dispatch" phase (including
partial_phases/state from the attempted dispatch when available) to the phases
list, and then re-raising a CoordinationPhaseError that carries that failed
phase result; implement this by extracting the current dispatch calls into a
helper (e.g., _run_dispatch_phase or similar) that returns a dispatch result on
success and on exception appends a failed CoordinationPhaseResult and raises
CoordinationPhaseError. Apply the same pattern to the other dispatch site (the
similar block around lines 176-184) so both select_dispatcher and
dispatcher.dispatch failures produce a proper dispatch-phase entry and raise
CoordinationPhaseError.

---

372-389: ⚠️ Potential issue | 🔴 Critical

Roll up against the full planned subtask set.

This only counts statuses from dispatch_result.waves. build_execution_waves() drops unrouted/empty waves, and fail-fast or setup failures can skip later waves entirely, so missing subtasks disappear from the rollup and the parent can be marked complete with unfinished work. Seed the rollup from the full decomposition plan and synthesize BLOCKED/FAILED for any subtask with no recorded outcome.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ai_company/engine/coordination/service.py` around lines 372 - 389, The
current rollup uses only statuses collected from dispatch_result.waves, which
omits subtasks that were never routed; instead fetch the full decomposition plan
(via the decomposition service used to build_execution_waves) for
context.task.id, iterate the plan's ordered subtask ids, build a map of outcomes
from dispatch_result.waves (by subtask id), and for each planned subtask append
TaskStatus.COMPLETED if outcome.is_success, TaskStatus.FAILED if outcome exists
but not success, or TaskStatus.BLOCKED if there is no recorded outcome; pass
that complete tuple to
self._decomposition_service.rollup_status(context.task.id, tuple(statuses)) so
missing subtasks are treated as BLOCKED/FAILED rather than disappearing from the
rollup.

---

211-230: ⚠️ Potential issue | 🟠 Major

Don't normalize catastrophic exceptions into ordinary phase failures.

These generic except Exception handlers will wrap or swallow MemoryError, which lets the coordinator keep running after a process-fatal failure. At minimum, catastrophic exceptions should bypass the phase-failure path and propagate unchanged; that also means the new test_memory_error_propagated expectation needs to flip to MemoryError. Based on learnings, "Errors: handle explicitly, never silently swallow".

Also applies to: 265-284, 390-405, 471-485

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ai_company/engine/coordination/service.py` around lines 211 - 230, The
generic except Exception blocks (the handler that logs
COORDINATION_PHASE_FAILED, creates CoordinationPhaseResult, appends to phases
and raises CoordinationPhaseError) must not normalize catastrophic
exceptions—explicitly detect and re-raise MemoryError (and optionally other
fatal exceptions like SystemExit/KeyboardInterrupt) before converting to a phase
failure; locate the try/except that uses phase_name, start, logger.warning(...,
phase=phase_name, error=str(exc)), creates CoordinationPhaseResult and raises
CoordinationPhaseError and add a guard such as if isinstance(exc, MemoryError):
raise (and likewise for SystemExit/KeyboardInterrupt) so those exceptions
propagate unchanged; apply the same change to the other identical handlers
referenced (the blocks around the other ranges) so test_memory_error_propagated
will see a raw MemoryError.

src/ai_company/engine/coordination/group_builder.py (1)

84-94: ⚠️ Potential issue | 🔴 Critical

Don't let an unroutable prerequisite disappear from the DAG.

When decision is None, this code just skips the subtask. DependencyGraph.parallel_groups() only gives you topological order, so any routed descendants can still show up in later waves and run without their prerequisite ever being assigned. Please either fail coordination here or mark downstream dependents blocked before building subsequent waves. As per coding guidelines, "Validate: at system boundaries (user input, external APIs, config files)".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ai_company/engine/coordination/group_builder.py` around lines 84 - 94,
The loop in group_builder.py that iterates over subtask_ids currently skips
subtasks when routing_lookup.get(subtask_id) returns None, which lets downstream
nodes run without their prerequisite; instead, when decision is None you must
fail coordination or explicitly mark downstream dependents blocked: modify the
loop (the branch checking "if decision is None") to either raise a clear
exception (e.g., CoordinationError or RuntimeError) with context (subtask_id,
wave_idx) so coordination aborts, or call the dependency-graph API to mark the
entire descendant subtree of subtask_id as blocked before continuing (use the
module/class that provides dependency traversal from
DependencyGraph.parallel_groups(), e.g.,
dependency_graph.get_descendants(subtask_id) /
dependency_graph.block_subtree(subtask_id)), and replace the current
logger.debug-only behavior (COORDINATION_WAVE_BUILT, wave_idx, subtask_id,
skipped=True) with the chosen failure/blocking action so unroutable
prerequisites cannot disappear from the DAG.