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

[mrveiss]

Closes #13699

Thinking Path

The reported symptom was a bad patch target: TestOrchestratorPerformance.setup_method
patches orchestrator.config_manager, which the module does not define. It imports
get_config_manager, and Orchestrator.__init__ takes an injectable config_manager
(#13162) — so the fix is constructor injection, matching dependency_injection_test.py.

That made both nodes run. But the issue's real concern was fake coverage, so I checked
whether the bodies now measure anything. They did not, for two further reasons:

1. GemmaClassificationAgent() raises AgentConfigurationError unless a 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 timed an early return, and timed a different path
   depending on the environment.
2. assert isinstance(steps, list) passes on the empty list. Asserting a non-empty plan
   instead turned the test red and exposed a live production bug:
   plan_workflow_steps returned [] on every call. WorkflowStep has aliased
   autobot_shared.workflow.WorkflowTask since refactor: unify 5 duplicate WorkflowStep/Task/Plan definitions + add first-class prompt/tools fields #6951 Phase 2A (task_id /
   estimated_duration_seconds), but the method still passed the retired id /
   expected_duration_ms, so each invocation raised TypeError into the broad handler.
   workflow_templates/types.py:29-31 documents the invariant that "every
   WorkflowStep(task_id=..., ...) call site constructs a real WorkflowTask" — this was
   the one site never migrated.

So the issue was two layers deep: the tests could not run, and the method they cover
produced nothing.

What Changed

autobot-backend/orchestrator.py

• plan_workflow_steps: three construction sites repointed from id /
  expected_duration_ms to the canonical task_id / estimated_duration_seconds.
• The except guard now logs exc_info=True, so a future constructor mismatch is
  diagnosable rather than a one-line mystery that silently empties every plan.

autobot-backend/performance_benchmarks_test.py

• setup_method: injects the config double through the constructor instead of patching an
  absent module attribute.
• Binds a classification double so the measured path is identical in every environment and
  never reaches a provider.
• Asserts a real TaskComplexity and a non-empty plan; the concurrent test asserts step
  counts are > 0 rather than merely integer-shaped.
• Corrects the stale #13162 comment claiming these entry points route through
  llm_service — they do not; the double stays only as a provider guard.

Verification

Before — both nodes error at setup, so neither body ever ran:

$ python3 -m pytest performance_benchmarks_test.py::TestOrchestratorPerformance -q -p no:randomly
performance_benchmarks_test.py EE                                        [100%]
E   AttributeError: <module 'orchestrator' ...> does not have the attribute 'config_manager'
============================== 2 errors in 2.58s ===============================

After:

$ python3 -m pytest performance_benchmarks_test.py::TestOrchestratorPerformance -q -p no:randomly
============================== 2 passed in 1.23s ===============================

AC 2 — assertions execute, proven by deliberate failure, not by a green run. Each of
the four assertions was inverted in turn and each fired:

E   AssertionError: DELIBERATE: complexity assert reached
E   AssertionError: DELIBERATE: planning budget assert reached
E   AssertionError: DELIBERATE: results assert reached
E   AssertionError: DELIBERATE: concurrent budget assert reached

The strongest evidence is unsynthetic: the new assert steps failed for real against the
pre-fix orchestrator, which is how the empty-plan bug was found:

E   AssertionError: Planning produced no steps for: List files in current directory...
E   assert []
ERROR orchestrator: Failed to plan workflow steps: WorkflowTask.__init__() got an
      unexpected keyword argument 'id'

AC 3 — no non-existent patch target. The only remaining patch in the class is
patch.object(self.orchestrator, "llm_service"), an attribute set in _init_core_components.

Whole module under the marker CI selects, plus a regression sweep over every area the
orchestrator change touches:

$ python3 -m pytest -m performance performance_benchmarks_test.py -q
============================== 10 passed in 3.78s ==============================

$ python3 -m pytest orchestration/ services/workflow_automation/ services/advanced_workflow/ \
    workflow_templates/ performance_benchmarks_test.py dependency_injection_test.py -q -p no:randomly
======================= 364 passed, 4 warnings in 14.22s =======================

Note: local Python is 3.10 and 19/69 backend deps are absent, so this is not a CI proxy —
CI is the authority on the full suite.

Discovered — out of scope, filed as #13730

Fixing the method exposed that every remaining caller of it is still broken, two ways:
all four production call sites invoke the async planning API without await (the benchmark
repaired here is the only awaited caller in the tree), and they read step.id /
step.user_approval_required, which WorkflowTask does not define. Also
orchestration/plan_steps_e2e_test.py has no assertions and swallows every exception — it
passed throughout the entire period the method returned nothing.

That is a caller-side migration across four modules with real behavioural blast radius, so
it is not bundled here. Chat-request workflow planning is dead end-to-end until #13730
lands; this PR fixes the planner itself and the tests that cover it.

Model Used

Claude Opus 5 (1M context)

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Review

Self-review — GitHub refuses an approving review from the PR author, and
Dev_new_gui has required_pull_request_reviews: null, so this is a review record
rather than an approval gate. Stated plainly so it is not mistaken for independent sign-off.

Verified

Unit conversion is correct and matches the canonical convention. The four rewrites
convert ms → s exactly (2000→2.0, 3000→3.0, 10000→10.0, 2000→2.0), and
estimated_duration_seconds is used in seconds everywhere else it appears
(workflow_templates/sysadmin.py uses 10.0/15.0/20.0). The one place the retired ms name
survives is workflow_templates/manager.py:119, which deliberately re-emits
int(step.estimated_duration_seconds * 1000) for the frontend contract — consistent, not drifted.

No caller can regress from the behaviour change. plan_workflow_steps went from
always-[] to returning real tasks. No call site branches on an empty plan — all four
immediately iterate or len() it, and all four bind the coroutine un-awaited, so they
raise before reading anything either way (that is #13730). The change is behaviourally
inert for every existing caller.

The strengthened assertions hold on both branches. assert isinstance(complexity, TaskComplexity) passes whether classification runs through the injected double or
short-circuits on CLASSIFICATION_AVAILABLE = False, and TaskComplexity is imported from
autobot_types — the same class object orchestrator.py:111 uses, so there is no
two-enums identity trap.

No stale patch targets remain. The only surviving patch in the class is
patch.object(self.orchestrator, "llm_service"), and llm_service is set unconditionally
in _init_core_components.

Mock(spec=ConfigManager) is honest. A spec'd mock raises on any attribute
ConfigManager does not define, so the double cannot silently absorb a renamed config
method the way an unspec'd MagicMock would.

One risk, accepted deliberately

assert steps couples the benchmark to WORKFLOW_TYPES_AVAILABLE being true — if
workflow_templates ever fails to import, this test now fails instead of passing on the
empty list. That is the correct signal (a benchmark that cannot build a plan is measuring
nothing, which is this issue's whole complaint), and it is contained: the module already
imports autobot_types and orchestrator at top level, so a broken import graph produces
a collection error long before this assertion. All 12 python-suite shards are green,
which exercises it under full CI dependencies.

Not addressed here, by design

The caller-side migration (#13730). Bundling it would mean four modules, missing awaits,
and stale field reads with real behavioural blast radius — a different risk class than a
test repair plus a constructor rename.

CI

59 checks, 0 failures. All 10 required contexts green or correctly skipped
(migration-matrix / api-wiring — no migrations, no API surface). All 12 python-suite
shards SUCCESS, so this is not the pytest-split empty-shard false green.

Verdict: merge.