fix(orchestrator): repair TestOrchestratorPerformance patch target and the empty plan it was hiding (#13699)

.session/HANDOFF-issue-13699.md

@@ -0,0 +1,33 @@
+# Handoff: issue-13699
+status: complete
+pr: #13731
+base_at_push: 319c00127
+gates: tests=PASS (2/2 target nodes; 10/10 module under -m performance; 364 passed across orchestration/, services/workflow_automation/, services/advanced_workflow/, workflow_templates/, performance_benchmarks_test.py, dependency_injection_test.py)
+needs_rebase_before_merge: no
+remaining:
+blocked_on:
+worktree: .worktrees/issue-13699  (safe to remove after merge)
+
+## What landed
+
+Two commits:
+- `fix(orchestrator)`: `plan_workflow_steps` repointed from the retired `id` /
+  `expected_duration_ms` kwargs to the canonical `WorkflowTask` fields (`task_id` /
+  `estimated_duration_seconds`). Every prior call raised TypeError into the broad
+  handler and returned `[]`. Guard now logs `exc_info=True`.
+- `test(orchestrator)`: `TestOrchestratorPerformance.setup_method` injects the config
+  double via the constructor instead of patching the absent
+  `orchestrator.config_manager`; binds a classification double so the measured path is
+  environment-independent; asserts a real `TaskComplexity` and a non-empty plan.
+
+## Carried forward — NOT done
+
+**#13730** — every remaining caller of `plan_workflow_steps` is still broken:
+- un-awaited async calls at `orchestration/workflow_planner.py:113,219`,
+  `services/workflow_automation/manager.py:75-76`,
+  `services/advanced_workflow/step_generator.py:63`
+- reads of `step.id` / `step.user_approval_required`, absent from `WorkflowTask`
+- `orchestration/plan_steps_e2e_test.py` has no assertions and swallows exceptions
+
+Chat-request workflow planning is dead end-to-end until #13730 lands. Do not close
+#13730 on the strength of this PR — it fixes the planner, not its callers.

autobot-backend/orchestrator.py

@@ -557,50 +557,57 @@ async def plan_workflow_steps(self, user_request: str, complexity: TaskComplexit
             if complexity == TaskComplexity.SIMPLE:
                 return [
                     WorkflowStep(
-                        id="step_1",
+                        task_id="step_1",
                         agent_type="llm",
                         action="generate_response",
                         description="Generate direct response to user query",
                         requires_approval=False,
                         dependencies=[],
                         inputs={"query": user_request},
-                        expected_duration_ms=2000,
+                        estimated_duration_seconds=2.0,
                     )
                 ]
             return [
                 WorkflowStep(
-                    id="step_1",
+                    task_id="step_1",
                     agent_type="analyzer",
                     action="analyze_request",
                     description="Analyze user request",
                     requires_approval=False,
                     dependencies=[],
                     inputs={"query": user_request},
-                    expected_duration_ms=3000,
+                    estimated_duration_seconds=3.0,
                 ),
                 WorkflowStep(
-                    id="step_2",
+                    task_id="step_2",
                     agent_type="executor",
                     action="execute_plan",
                     description="Execute the planned actions",
                     requires_approval=True,
                     dependencies=["step_1"],
                     inputs={"query": user_request},
-                    expected_duration_ms=10000,
+                    estimated_duration_seconds=10.0,
                 ),
                 WorkflowStep(
-                    id="step_3",
+                    task_id="step_3",
                     agent_type="synthesizer",
                     action="synthesize_results",
                     description="Synthesize results",
                     requires_approval=False,
                     dependencies=["step_2"],
                     inputs={"query": user_request},
-                    expected_duration_ms=2000,
+                    estimated_duration_seconds=2.0,
                 ),
             ]
         except Exception as e:
-            logger.error("Failed to plan workflow steps: %s", e)
+            # #13699: this handler turned a constructor mismatch into a silent
+            # empty plan for every caller — `WorkflowStep` has aliased the
+            # canonical `WorkflowTask` since #6951 Phase 2A (`task_id` /
+            # `estimated_duration_seconds`), but the calls above still passed
+            # the retired `id` / `expected_duration_ms`, so every invocation
+            # raised TypeError and was logged as a one-line mystery. Keep the
+            # guard, but make the next such breakage diagnosable.
+            logger.error("Failed to plan workflow steps: %s", e, exc_info=True)
             return []
 
     # ------------------------------------------ workflow planning (canonical path)

autobot-backend/performance_benchmarks_test.py

@@ -15,11 +15,13 @@
 import threading
 import time
 from typing import Dict
-from unittest.mock import AsyncMock, MagicMock, patch
+from unittest.mock import AsyncMock, MagicMock, Mock, patch
 
 import psutil
 import pytest
 
+from autobot_types import TaskComplexity
+from config import ConfigManager
 from knowledge import KnowledgeBase
 from memory import MemoryManager
 from memory.enums import MemoryCategory
@@ -312,9 +314,28 @@ def setup_method(self):
         """Set up test environment"""
         self.benchmark = PerformanceBenchmark()
 
-        with patch("orchestrator.config_manager") as mock_config:
-            mock_config.get_llm_config.return_value = {"orchestrator_llm": "llama3.2:1b"}
-            self.orchestrator = Orchestrator()
+        # #13699: the `orchestrator` module has no `config_manager` attribute to
+        # patch — it imports `get_config_manager` and `Orchestrator.__init__`
+        # takes an injectable `config_manager` (#13162). Patching the absent
+        # name raised AttributeError in setup, so both benchmarks below have
+        # been erroring at collection rather than running. Inject the config
+        # double instead, the same way dependency_injection_test.py does.
+        mock_config = Mock(spec=ConfigManager)
+        mock_config.get_llm_config.return_value = {"orchestrator_llm": "llama3.2:1b"}
+        mock_config.get.side_effect = lambda key, default=None: default
+        self.orchestrator = Orchestrator(config_manager=mock_config)
+
+        # #13699: `GemmaClassificationAgent()` raises AgentConfigurationError
+        # unless AUTOBOT_CLASSIFICATION_PROVIDER is configured, and
+        # `_init_classification_agent` swallows that into
+        # `classification_agent = None`. `classify_request_complexity` then
+        # short-circuits on the None branch, so the benchmark would time an
+        # early return rather than the classification seam — and would time a
+        # different path depending on the environment. Bind a double so the
+        # measured path is the same everywhere and never reaches a provider.
+        self.orchestrator.classification_agent = MagicMock(
+            classify_user_request=AsyncMock(return_value=MagicMock(complexity=TaskComplexity.COMPLEX))
+        )
 
     async def test_orchestrator_task_planning_performance(self):
         """Benchmark orchestrator task planning operations"""
@@ -324,9 +345,12 @@ async def test_orchestrator_task_planning_performance(self):
             "Research network security tools, evaluate them, and provide installation recommendations with approval workflow",
         ]
 
+        # #13699: neither benchmarked entry point routes through
+        # `llm_service` today — classification goes via `classification_agent`
+        # and `plan_workflow_steps` is pure step construction. The double stays
+        # as a guard so no iteration can reach a real provider if that changes,
+        # and `chat` is an AsyncMock because it is awaited when it is called.
         with patch.object(self.orchestrator, "llm_service") as mock_llm:
-            # #13162: llm_service.chat is awaited by the orchestrator, so the
-            # double must be an AsyncMock.
             mock_llm.chat = AsyncMock(return_value=MagicMock(content="Mock orchestrator response", error=None))
 
             for request in test_requests:
@@ -343,9 +367,14 @@ async def test_orchestrator_task_planning_performance(self):
 
                     self.benchmark.end_measurement()
 
-                    # Verify planning results
-                    assert complexity is not None
+                    # Verify planning results. #13699: `isinstance(steps, list)`
+                    # alone passes on the empty list every unavailable-types
+                    # branch returns, which is how this body could have looked
+                    # meaningful while measuring nothing. Assert the plan is
+                    # real and that classification produced a TaskComplexity.
+                    assert isinstance(complexity, TaskComplexity)
                     assert isinstance(steps, list)
+                    assert steps, f"Planning produced no steps for: {request[:50]}..."
 
                 stats = self.benchmark.get_statistics()
 
@@ -380,9 +409,11 @@ async def run_workflow():
             results = await asyncio.gather(*tasks)
             total_time = time.perf_counter() - start_time
 
-            # Verify all workflows completed
+            # Verify all workflows completed. #13699: a step count of 0 is not a
+            # completed workflow, so assert the plans were non-empty rather than
+            # merely integer-shaped.
             assert len(results) == count
-            assert all(isinstance(r, int) for r in results)
+            assert all(isinstance(r, int) and r > 0 for r in results)
 
             self.benchmark.get_statistics()