test(R03/R04): cover the three code paths that were changed but never executed
Verification gap closed. The suite proved the new services correct in isolation, but three paths I had modified had no test actually running them: tests/integration/test_task_executor_flow.py (7 tests) The Schedule Task path after R04-T05. Pins that History is still re-saved from the LIVE message list mid-run (the reason begin_turn() exists - the pre-turn copy would have frozen progress at the first user message), that update_plan tracking still reports an unfinished checklist, and that a failed run still raises so execute_task writes error.txt. tests/integration/test_routing_surfaces.py (11 tests) Real offscreen CoworkTab/Co4ETab/FolderTab calling the shared routing service: correct surface key per screen, Auto switches, Off does not consult the engine, Manual switches only on approval, a pinned Admin agent still wins, and AI-Edit still pins TaskType.CODING. Also pins the field contract ui/routing_toggle.py reads off RoutingDecision (from_model/to_model as provider/model keys) - a rename there would only fail inside a modal dialog. Also updates docs/refactor/Refactoring_Checklist.md: the 16 completed R01/R03/R04 tasks, the Team Duy daily rows, and a status block recording the measured numbers, the scope correction (team owns R01/R02/R04/R10), and what is still outstanding. Suite: 243 passed, 2 pre-existing failures (EPIC R02). Fast suite (unit + contracts + characterization + routing): 218 passed in 1.16s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,256 @@
|
||||
"""The three chat surfaces really route through the shared service (R03-T04/T05).
|
||||
|
||||
The unit suite proves ``RoutingApplicationService`` decides correctly against a
|
||||
fake router. This file proves the three widgets that used to own a private copy
|
||||
of that algorithm now call it, on real (offscreen) widgets:
|
||||
|
||||
* ``ui/chat_panel.py::_apply_routing`` (Cowork)
|
||||
* ``ui/co4e_tab.py::_apply_co4e_routing`` (Co4E)
|
||||
* ``ui/folder_tab.py::_ai_apply_routing`` (AI-Edit)
|
||||
|
||||
It also pins the Manual-mode handshake, including the field contract the
|
||||
existing confirm dialog reads off the decision - the one place where the new
|
||||
``RoutingDecision`` has to look like the legacy ``SwitchDecision`` it replaced.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
from pathlib import Path
|
||||
from typing import Any, List, Optional, Tuple
|
||||
|
||||
import pytest
|
||||
|
||||
os.environ.setdefault("QT_QPA_PLATFORM", "offscreen")
|
||||
|
||||
from cowork_local.application.model_routing import ( # noqa: E402
|
||||
RoutingApplicationService,
|
||||
RoutingDecision,
|
||||
RoutingMode,
|
||||
)
|
||||
from cowork_local.config import AppConfig # noqa: E402
|
||||
from cowork_local.state import AppContext # noqa: E402
|
||||
|
||||
pytest.importorskip("PySide6", reason="Qt is required for the integration suite")
|
||||
|
||||
|
||||
@pytest.fixture(scope="module")
|
||||
def qt_app():
|
||||
from PySide6.QtWidgets import QApplication
|
||||
|
||||
return QApplication.instance() or QApplication([])
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def ctx(qt_app, tmp_path: Path) -> AppContext:
|
||||
return AppContext(AppConfig.load(tmp_path / "config.json"))
|
||||
|
||||
|
||||
class _FakeRouteResult:
|
||||
"""Shaped like ``core.routing.service.RouteResult``."""
|
||||
|
||||
def __init__(self, provider: str, model: str, gain: float = 0.4,
|
||||
task: str = "coding") -> None:
|
||||
self.should_switch = True
|
||||
self._target = (provider, model)
|
||||
self.task_type = type("_T", (), {"value": task})()
|
||||
self.decision = type("_D", (), {"score_gain": gain, "reason": "better fit"})()
|
||||
|
||||
def target(self) -> Optional[Tuple[str, str]]:
|
||||
return self._target
|
||||
|
||||
|
||||
class _FakeRouter:
|
||||
"""Minimal RoutingPort: always proposes the same switch, records the surface."""
|
||||
|
||||
def __init__(self, provider="anthropic", model="claude-sonnet-4-6") -> None:
|
||||
self.result = _FakeRouteResult(provider, model)
|
||||
self.surfaces: List[str] = []
|
||||
|
||||
def route(self, surface, prompt, current_provider, current_model, **kwargs):
|
||||
self.surfaces.append(surface)
|
||||
return self.result
|
||||
|
||||
|
||||
def _install(ctx: AppContext, mode: str) -> _FakeRouter:
|
||||
"""Wire a fake router into the context and force ``mode`` on every surface."""
|
||||
router = _FakeRouter()
|
||||
service = RoutingApplicationService(router, mode_reader=lambda _surface: mode)
|
||||
ctx._routing_application = service # already-built instance; accessor returns it
|
||||
return router
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Cowork chat
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_cowork_applies_an_auto_switch_to_the_next_turn(ctx):
|
||||
from cowork_local.ui.cowork_tab import CoworkTab
|
||||
|
||||
router = _install(ctx, "auto")
|
||||
tab = CoworkTab(ctx)
|
||||
turn: dict = {"bubbles": []}
|
||||
|
||||
tab._apply_routing("write a function", turn)
|
||||
|
||||
assert router.surfaces == [tab.kind]
|
||||
# build_provider() honours these for THIS turn only.
|
||||
assert (tab._routed_provider, tab._routed_model) == ("anthropic", "claude-sonnet-4-6")
|
||||
assert turn["bubbles"], "the user must be told the model was switched"
|
||||
|
||||
|
||||
def test_cowork_leaves_the_model_alone_when_routing_is_off(ctx):
|
||||
from cowork_local.ui.cowork_tab import CoworkTab
|
||||
|
||||
router = _install(ctx, "off")
|
||||
tab = CoworkTab(ctx)
|
||||
turn: dict = {"bubbles": []}
|
||||
|
||||
tab._apply_routing("write a function", turn)
|
||||
|
||||
assert router.surfaces == []
|
||||
assert (tab._routed_provider, tab._routed_model) == (None, None)
|
||||
assert turn["bubbles"] == []
|
||||
|
||||
|
||||
def test_cowork_manual_mode_switches_only_after_the_dialog_approves(ctx, monkeypatch):
|
||||
from cowork_local.ui import chat_panel as chat_panel_module
|
||||
from cowork_local.ui.cowork_tab import CoworkTab
|
||||
|
||||
_install(ctx, "manual")
|
||||
tab = CoworkTab(ctx)
|
||||
asked: List[Any] = []
|
||||
monkeypatch.setattr(tab, "_confirm_routing_switch",
|
||||
lambda decision: asked.append(decision) or True)
|
||||
turn: dict = {"bubbles": []}
|
||||
|
||||
tab._apply_routing("write a function", turn)
|
||||
|
||||
assert len(asked) == 1
|
||||
assert (tab._routed_provider, tab._routed_model) == ("anthropic", "claude-sonnet-4-6")
|
||||
|
||||
|
||||
def test_cowork_manual_mode_keeps_the_model_when_the_dialog_is_declined(ctx, monkeypatch):
|
||||
from cowork_local.ui.cowork_tab import CoworkTab
|
||||
|
||||
_install(ctx, "manual")
|
||||
tab = CoworkTab(ctx)
|
||||
monkeypatch.setattr(tab, "_confirm_routing_switch", lambda _decision: False)
|
||||
turn: dict = {"bubbles": []}
|
||||
|
||||
tab._apply_routing("write a function", turn)
|
||||
|
||||
assert (tab._routed_provider, tab._routed_model) == (None, None)
|
||||
assert turn["bubbles"] == []
|
||||
|
||||
|
||||
def test_a_pinned_admin_agent_still_wins_over_routing(ctx):
|
||||
"""An explicitly chosen Admin agent pins its own provider/model; routing must
|
||||
not override a deliberate user choice."""
|
||||
from cowork_local.ui.cowork_tab import CoworkTab
|
||||
|
||||
router = _install(ctx, "auto")
|
||||
tab = CoworkTab(ctx)
|
||||
tab._admin_agent = object()
|
||||
turn: dict = {"bubbles": []}
|
||||
|
||||
tab._apply_routing("write a function", turn)
|
||||
|
||||
assert router.surfaces == []
|
||||
assert (tab._routed_provider, tab._routed_model) == (None, None)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Co4E
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_co4e_routes_on_its_own_surface_key_and_returns_the_model(ctx):
|
||||
from cowork_local.ui.co4e_tab import Co4ETab
|
||||
|
||||
router = _install(ctx, "auto")
|
||||
tab = Co4ETab(ctx)
|
||||
|
||||
model = tab._apply_co4e_routing("build me a flow")
|
||||
|
||||
assert router.surfaces == ["co4e"]
|
||||
assert model == "claude-sonnet-4-6"
|
||||
assert tab._co4e_routed_provider == "anthropic"
|
||||
|
||||
|
||||
def test_co4e_returns_an_empty_model_when_routing_is_off(ctx):
|
||||
"""'' means "use the provider default" - the contract _run_chat_turn expects."""
|
||||
from cowork_local.ui.co4e_tab import Co4ETab
|
||||
|
||||
_install(ctx, "off")
|
||||
tab = Co4ETab(ctx)
|
||||
|
||||
assert tab._apply_co4e_routing("build me a flow") == ""
|
||||
assert tab._co4e_routed_provider is None
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# AI-Edit
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_ai_edit_routes_on_its_own_surface_key(ctx):
|
||||
from cowork_local.ui.folder_tab import FolderTab
|
||||
|
||||
router = _install(ctx, "auto")
|
||||
tab = FolderTab(ctx)
|
||||
|
||||
tab._ai_apply_routing("rename this variable")
|
||||
|
||||
assert router.surfaces == ["ai_edit"]
|
||||
assert (tab._ai_routed_provider, tab._ai_routed_model) == (
|
||||
"anthropic", "claude-sonnet-4-6")
|
||||
|
||||
|
||||
def test_ai_edit_pins_the_coding_task_type(ctx):
|
||||
"""An edit instruction is never a QA question, so AI-Edit skips
|
||||
classification entirely - the constraint has to survive the move into the
|
||||
shared service or it is silently dropped."""
|
||||
from cowork_local.core.routing.models import TaskType
|
||||
from cowork_local.ui.folder_tab import FolderTab
|
||||
|
||||
seen: List[Any] = []
|
||||
|
||||
class _Recorder(_FakeRouter):
|
||||
def route(self, surface, prompt, current_provider, current_model, **kwargs):
|
||||
seen.append(kwargs.get("task_type"))
|
||||
return super().route(surface, prompt, current_provider, current_model, **kwargs)
|
||||
|
||||
ctx._routing_application = RoutingApplicationService(
|
||||
_Recorder(), mode_reader=lambda _s: "auto")
|
||||
tab = FolderTab(ctx)
|
||||
|
||||
tab._ai_apply_routing("rename this variable")
|
||||
|
||||
assert seen == [TaskType.CODING]
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# The confirm dialog's field contract
|
||||
# --------------------------------------------------------------------------- #
|
||||
def test_the_decision_exposes_exactly_what_the_confirm_dialog_reads():
|
||||
"""``ui/routing_toggle.py::confirm_switch`` is not migrated until EPIC R08,
|
||||
so it still reads ``from_model``/``to_model`` as ``provider/model`` candidate
|
||||
keys and splits them. A rename here would blow up inside a modal dialog -
|
||||
the one place a failure is hardest to see in a test run."""
|
||||
from cowork_local.core.routing.models import split_key
|
||||
|
||||
decision = RoutingDecision(
|
||||
mode=RoutingMode.MANUAL, provider="anthropic", model="claude-sonnet-4-6",
|
||||
switched=True, task_type="coding", score_gain=0.31, reason="better fit",
|
||||
previous_provider="openai_compat", previous_model="gpt-4o-mini",
|
||||
)
|
||||
|
||||
assert split_key(decision.from_model)[1] == "gpt-4o-mini"
|
||||
assert split_key(decision.to_model)[1] == "claude-sonnet-4-6"
|
||||
assert decision.task_type == "coding"
|
||||
assert f"{decision.score_gain:.2f}" == "0.31"
|
||||
assert decision.reason == "better fit"
|
||||
|
||||
|
||||
def test_a_first_turn_with_no_current_model_yields_an_empty_from_model():
|
||||
"""split_key() is only called when from_model is truthy, so an unset current
|
||||
model must produce "" rather than a bare "provider/"."""
|
||||
decision = RoutingDecision(mode=RoutingMode.AUTO, provider="anthropic",
|
||||
model="claude", switched=True)
|
||||
|
||||
assert decision.from_model == ""
|
||||
@@ -0,0 +1,178 @@
|
||||
"""End-to-end check of the Schedule Task path after R04-T05.
|
||||
|
||||
``core/task_executors.py::_run_agent`` used to assemble its own ``run_cowork``
|
||||
call, in parallel with ``ui/cowork_tab.py`` doing the same thing slightly
|
||||
differently. It now goes through ``ConversationApplicationService``, and the
|
||||
things most at risk from that change are exactly what this file pins:
|
||||
|
||||
* the unattended run still returns the answer text the scheduler writes to output.md
|
||||
* History is still re-saved from the LIVE message list after every assistant
|
||||
message, so a long run shows progress when reopened mid-flight
|
||||
* ``update_plan`` tracking still works, so a task whose checklist is unfinished
|
||||
is not reported as done
|
||||
* a failed run still raises, because ``execute_task`` writes error.txt from it
|
||||
|
||||
No Qt and no network: the provider is scripted and History is redirected into a
|
||||
tmp folder.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from typing import Any, Dict, List
|
||||
|
||||
import pytest
|
||||
|
||||
from cowork_local.config import AppConfig
|
||||
from cowork_local.core import audit_log, chat_agent, task_executors
|
||||
from cowork_local.state import AppContext
|
||||
from tests.fakes import FakeProvider, ScriptedTurn
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def task_ctx(tmp_path: Path, monkeypatch):
|
||||
"""An AppContext whose History and audit log live in a tmp folder."""
|
||||
monkeypatch.setattr(chat_agent, "active_skills_text", lambda: "")
|
||||
monkeypatch.setattr(chat_agent, "load_rules", lambda: "")
|
||||
monkeypatch.setattr(audit_log, "AUDIT_DIR", tmp_path / "audit")
|
||||
|
||||
ctx = AppContext(AppConfig.load(tmp_path / "config.json"))
|
||||
# Same reason as the Cowork integration suite: the security pre-flight costs
|
||||
# an extra provider call that has nothing to do with what is being tested.
|
||||
ctx.config.agent_security["enabled"] = False
|
||||
monkeypatch.setattr(ctx.config, "history_dir", lambda: tmp_path / "history")
|
||||
return ctx
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def history_saves(monkeypatch) -> List[List[Dict[str, Any]]]:
|
||||
"""Capture a SNAPSHOT of the messages at each History save.
|
||||
|
||||
Snapshotting matters: the engine keeps appending to the same list, so
|
||||
storing the list itself would make every recorded save look identical to the
|
||||
final state and the "live progress" assertion would prove nothing.
|
||||
"""
|
||||
saves: List[List[Dict[str, Any]]] = []
|
||||
|
||||
def fake_save(_dir, _kind, _session_id, messages, **_kwargs):
|
||||
saves.append([dict(m) for m in messages])
|
||||
|
||||
from cowork_local.core import history
|
||||
|
||||
monkeypatch.setattr(history, "save_conversation", fake_save)
|
||||
return saves
|
||||
|
||||
|
||||
def _run(ctx, provider, prompt="do the thing", out_dir: Path = None, **kwargs):
|
||||
"""Run one unattended cowork task with ``provider`` pinned."""
|
||||
ctx.build_active_provider = lambda: provider
|
||||
events: List[Dict[str, Any]] = []
|
||||
result = task_executors._run_agent(
|
||||
ctx, "cowork", prompt, out_dir, events.append, lambda: False,
|
||||
title=kwargs.pop("title", "T1"), **kwargs)
|
||||
return result, events
|
||||
|
||||
|
||||
def test_an_unattended_cowork_run_returns_the_answer(task_ctx, tmp_path, history_saves):
|
||||
provider = FakeProvider([ScriptedTurn(text="task answer")])
|
||||
|
||||
(answer, timed_out, incomplete), events = _run(task_ctx, provider,
|
||||
out_dir=tmp_path / "out")
|
||||
|
||||
assert answer == "task answer"
|
||||
assert timed_out is False
|
||||
assert incomplete == ""
|
||||
assert provider.call_count == 1
|
||||
|
||||
|
||||
def test_the_scheduler_still_gets_history_ready_before_the_turn_events(
|
||||
task_ctx, tmp_path, history_saves):
|
||||
"""The scheduler refreshes the History panel on this event, so a running
|
||||
task's conversation shows up while it runs."""
|
||||
provider = FakeProvider([ScriptedTurn(text="ok")])
|
||||
|
||||
_, events = _run(task_ctx, provider, out_dir=tmp_path / "out")
|
||||
|
||||
assert [e["type"] for e in events] == [
|
||||
"history_ready", "text", "assistant_done", "turn_completed"]
|
||||
|
||||
|
||||
def test_history_is_resaved_from_the_live_conversation_during_the_run(
|
||||
task_ctx, tmp_path, history_saves):
|
||||
"""The reason ``begin_turn()`` exists: the service builds its own message
|
||||
list, and the scheduler needs THAT list - not the pre-turn copy - or the
|
||||
mid-run saves would only ever contain the original user message.
|
||||
"""
|
||||
provider = FakeProvider([
|
||||
ScriptedTurn(tool_calls=[("save_file", {"filename": "a.md", "content": "x"})]),
|
||||
ScriptedTurn(text="Saved."),
|
||||
])
|
||||
|
||||
_run(task_ctx, provider, out_dir=tmp_path / "out")
|
||||
|
||||
# At least one save DURING the run already carried an assistant message,
|
||||
# and the final save carries the whole conversation.
|
||||
assert len(history_saves) >= 3 # initial + per assistant_done + final
|
||||
assert any(any(m["role"] == "assistant" for m in save)
|
||||
for save in history_saves[1:-1])
|
||||
assert [m["role"] for m in history_saves[-1]] == [
|
||||
"system", "user", "assistant", "tool", "assistant"]
|
||||
|
||||
|
||||
def test_an_unfinished_plan_is_reported_so_the_task_is_not_marked_done(
|
||||
task_ctx, tmp_path, history_saves):
|
||||
"""plan_set tracking runs through the same emit path; losing it would let a
|
||||
task whose own checklist says "not finished" be reported as successful."""
|
||||
provider = FakeProvider([
|
||||
ScriptedTurn(tool_calls=[("update_plan", {"steps": [
|
||||
{"title": "step one", "status": "running"}]})]),
|
||||
ScriptedTurn(text="stopping here"),
|
||||
])
|
||||
|
||||
(_answer, _timed_out, incomplete), _events = _run(task_ctx, provider,
|
||||
out_dir=tmp_path / "out")
|
||||
|
||||
assert incomplete != ""
|
||||
|
||||
|
||||
def test_a_completed_plan_reports_no_incompleteness(task_ctx, tmp_path, history_saves):
|
||||
provider = FakeProvider([
|
||||
ScriptedTurn(tool_calls=[("update_plan", {"steps": [
|
||||
{"title": "step one", "status": "done"}]})]),
|
||||
ScriptedTurn(text="all done"),
|
||||
])
|
||||
|
||||
(_answer, _timed_out, incomplete), _events = _run(task_ctx, provider,
|
||||
out_dir=tmp_path / "out")
|
||||
|
||||
assert incomplete == ""
|
||||
|
||||
|
||||
def test_a_failed_run_still_raises_so_execute_task_writes_error_txt(
|
||||
task_ctx, tmp_path, history_saves):
|
||||
provider = FakeProvider([ScriptedTurn(error="provider down"),
|
||||
ScriptedTurn(error="provider down")])
|
||||
|
||||
with pytest.raises(Exception) as excinfo:
|
||||
_run(task_ctx, provider, out_dir=tmp_path / "out")
|
||||
|
||||
assert "provider down" in str(excinfo.value)
|
||||
# The partial conversation is still saved - it is exactly what the user
|
||||
# needs to see after a failure.
|
||||
assert history_saves
|
||||
|
||||
|
||||
def test_a_per_task_provider_override_is_honoured(task_ctx, tmp_path, history_saves):
|
||||
"""A task can pin its own provider/model; the service must use that one, not
|
||||
the machine's Settings default."""
|
||||
default_provider = FakeProvider([], strict=True)
|
||||
task_provider = FakeProvider([ScriptedTurn(text="from the pinned model")])
|
||||
task_ctx.build_active_provider = lambda: default_provider
|
||||
task_ctx.build_provider_for = lambda _name, _model: task_provider
|
||||
|
||||
(answer, _timed_out, _incomplete) = task_executors._run_agent(
|
||||
task_ctx, "cowork", "go", tmp_path / "out", lambda _e: None, lambda: False,
|
||||
title="T", provider_name="anthropic", model="claude")[0:3]
|
||||
|
||||
assert answer == "from the pinned model"
|
||||
assert default_provider.call_count == 0
|
||||
assert task_provider.call_count == 1
|
||||
Reference in New Issue
Block a user