From 75f1dc6f753547b4bd19e46486662f64430b8480 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 13:30:13 -0400 Subject: [PATCH] fix(ws1/ws5): bootstrap orchestrator root in in-process invokers (G1); thread handbook into clarifier (G4) Gap-audit findings: - G1 (blocks-feature): make_cross_reviewer_invoker / make_fast_coder_invoker did 'from models import' without putting the orchestrator root on sys.path. The run-team serve daemon only bootstraps agent-team/, so on the live box every GPT-4.1 plan review hit ModuleNotFoundError -> review_plan's blanket except silently fail-closed to REQUEST_CHANGES (GPT-4.1 never actually ran). Both in-process invokers now call invoker_multi._ensure_orchestrator_on_path() before the deferred import. WS1 introduced this when it swapped the review default from the subprocess invoker to in-process. - G4 (degrades): the handbook context_provider was wired into the planner only; default_clarify_node_factory now accepts + forwards it, and run-team wires it into build_clarify_node too, so clarifying questions are handbook-aware. Tests: +2 regression tests (path-bootstrap, clarifier threading); _FakeCoordinator gains build_clarify_node. 1142 passed, ruff clean. --- agent-team/agent_team/coordinator.py | 12 +++- agent-team/agent_team/nodes/builders_llm.py | 7 +++ .../agent_team/nodes/review_loop_llm.py | 8 +++ agent-team/run-team.py | 15 +++-- agent-team/tests/test_run_team.py | 2 + agent-team/tests/test_ws_activation_wiring.py | 60 +++++++++++++++++++ 6 files changed, 97 insertions(+), 7 deletions(-) diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py index a8336c6..75cd7d9 100644 --- a/agent-team/agent_team/coordinator.py +++ b/agent-team/agent_team/coordinator.py @@ -187,7 +187,9 @@ def default_slack_listener_factory( ) -def default_clarify_node_factory() -> Callable[[PipelineState], PipelineState]: +def default_clarify_node_factory( + context_provider: "Callable[[], str] | None" = None, +) -> Callable[[PipelineState], PipelineState]: """Build the live Claude-backed clarifier node (§3.3, §7.1 P1). Composes the two committed leaves: the Claude clarifier callables @@ -206,7 +208,13 @@ def default_clarify_node_factory() -> Callable[[PipelineState], PipelineState]: from agent_team.nodes.clarifier import make_clarifier_node from agent_team.nodes.clarifier_llm import build_claude_clarifier_callables - assess_confidence, generate_questions = build_claude_clarifier_callables() + # WS5: thread the handbook/memory context_provider into the clarifier too + # (not just the planner) so clarifying questions are handbook-aware. Only + # forwarded when non-None to preserve the byte-identical default behavior. + kwargs: dict[str, Any] = {} + if context_provider is not None: + kwargs["context_provider"] = context_provider + assess_confidence, generate_questions = build_claude_clarifier_callables(**kwargs) return make_clarifier_node( assess_confidence=assess_confidence, generate_questions=generate_questions, diff --git a/agent-team/agent_team/nodes/builders_llm.py b/agent-team/agent_team/nodes/builders_llm.py index 7f3ad78..812ddfd 100644 --- a/agent-team/agent_team/nodes/builders_llm.py +++ b/agent-team/agent_team/nodes/builders_llm.py @@ -167,6 +167,13 @@ def make_fast_coder_invoker() -> "BuildCallable": """ def _invoke(instruction: str) -> str: + # Bootstrap the orchestrator root onto sys.path (models.py lives there; + # the run-team serve daemon does not add it) before the deferred import, + # mirroring make_cross_reviewer_invoker. Without it `from models import` + # raises ModuleNotFoundError in the daemon. + from agent_team.invoker_multi import _ensure_orchestrator_on_path # noqa: PLC0415 + + _ensure_orchestrator_on_path() from models import get_fast_coder # noqa: PLC0415 - intentional deferred import coder = get_fast_coder() diff --git a/agent-team/agent_team/nodes/review_loop_llm.py b/agent-team/agent_team/nodes/review_loop_llm.py index 3d8de87..50c4d62 100644 --- a/agent-team/agent_team/nodes/review_loop_llm.py +++ b/agent-team/agent_team/nodes/review_loop_llm.py @@ -210,6 +210,14 @@ def make_cross_reviewer_invoker() -> PlanReviewer: """ def _invoke(prompt: str, *, config: Any = None, **_kw: Any) -> str: + # The orchestrator root (where models.py lives) is NOT on sys.path in + # the run-team serve daemon (it only bootstraps agent-team/). Without + # this, `from models import` raises ModuleNotFoundError → review_plan's + # blanket except silently fails-closed to REQUEST_CHANGES, so GPT-4.1 + # never actually runs. Bootstrap the root before the deferred import. + from agent_team.invoker_multi import _ensure_orchestrator_on_path # noqa: PLC0415 + + _ensure_orchestrator_on_path() # Deferred import: keep the orchestrator package out of module import. from models import get_cross_reviewer # noqa: PLC0415 diff --git a/agent-team/run-team.py b/agent-team/run-team.py index 33d914f..9248ccd 100644 --- a/agent-team/run-team.py +++ b/agent-team/run-team.py @@ -529,16 +529,18 @@ def _build_coordinator(args: argparse.Namespace) -> Any: """ from agent_team.coordinator import ( Coordinator, + default_clarify_node_factory, default_plan_node_factory, default_review_wiring, ) transport = _build_transport(args) - # WS5 (D10): inject the Sea Haven handbook conventions into the planner prompt - # via the context_provider seam. The provider is the zero-arg handbook loader, - # which is itself fail-safe (returns "" when the handbook dir is absent), and - # planner.plan_node additionally swallows provider errors — so this never - # affects a run where the handbook is unavailable. + # WS5 (D10): inject the Sea Haven handbook conventions into BOTH the clarifier + # and the planner prompts via the context_provider seam. The provider is the + # zero-arg handbook loader, which is itself fail-safe (returns "" when the + # handbook dir is absent), and the clarifier/planner additionally swallow + # provider errors — so this never affects a run where the handbook is + # unavailable. context_provider = _build_context_provider() # Production runs the full P2 graph: the wrapped real planner + the bound @@ -547,6 +549,9 @@ def _build_coordinator(args: argparse.Namespace) -> Any: coordinator = Coordinator( db_path=args.db, transport=transport, + build_clarify_node=lambda: default_clarify_node_factory( + context_provider=context_provider + ), build_plan_node=lambda: default_plan_node_factory( context_provider=context_provider ), diff --git a/agent-team/tests/test_run_team.py b/agent-team/tests/test_run_team.py index 7ca0ce7..9d180ae 100644 --- a/agent-team/tests/test_run_team.py +++ b/agent-team/tests/test_run_team.py @@ -644,6 +644,7 @@ class _FakeCoordinator: *, db_path: Any, transport: Any, + build_clarify_node: Any = None, build_plan_node: Any = None, review_wiring: Any = None, ) -> None: @@ -651,6 +652,7 @@ class _FakeCoordinator: self.transport = transport # The production CLI opts the coordinator into the P2 graph by injecting # these factories; record them so the wiring is asserted, not ignored. + self.build_clarify_node = build_clarify_node self.build_plan_node = build_plan_node self.review_wiring = review_wiring self.setup_called = False diff --git a/agent-team/tests/test_ws_activation_wiring.py b/agent-team/tests/test_ws_activation_wiring.py index b0d1bd7..9988d9c 100644 --- a/agent-team/tests/test_ws_activation_wiring.py +++ b/agent-team/tests/test_ws_activation_wiring.py @@ -158,3 +158,63 @@ def test_slack_listener_factory_forwards_new_task_callback( new_task_callback=sentinel, ) assert captured["new_task_callback"] is sentinel + + +# --------------------------------------------------------------------------- # +# WS5 (G4): context_provider is threaded into the CLARIFIER too, not just plan +# --------------------------------------------------------------------------- # + + +def test_build_coordinator_threads_context_provider_into_clarify_node( + tmp_path: Path, monkeypatch: Any +) -> None: + cli = _load_run_team() + from agent_team import coordinator as coord_mod + + captured: dict[str, Any] = {} + + def _spy_clarify_factory(context_provider: Any = None): + captured["context_provider"] = context_provider + return lambda state: state + + monkeypatch.setattr(coord_mod, "default_clarify_node_factory", _spy_clarify_factory) + + coordinator = cli._build_coordinator(_dry_args(tmp_path)) + coordinator._build_clarify_node() + assert "context_provider" in captured + assert callable(captured["context_provider"]) + + +# --------------------------------------------------------------------------- # +# WS1 (G1): the in-process cross-reviewer bootstraps the orchestrator root onto +# sys.path before importing `models` (else the daemon silently REQUEST_CHANGES). +# --------------------------------------------------------------------------- # + + +def test_cross_reviewer_invoker_bootstraps_orchestrator_path(monkeypatch: Any) -> None: + import types + + from agent_team.invoker_multi import _ensure_orchestrator_on_path # noqa: F401 + from agent_team.nodes.review_loop_llm import make_cross_reviewer_invoker + + # The orchestrator root is parents[2] of invoker_multi.py. + import agent_team.invoker_multi as im + + root = str(Path(im.__file__).resolve().parents[2]) + + # Simulate the daemon: root NOT on sys.path. Inject a fake `models` so the + # deferred import resolves without real provider keys — the point is to + # prove the bootstrap runs (root re-added) BEFORE the import. + monkeypatch.setattr(sys, "path", [p for p in sys.path if p != root]) + fake_models = types.ModuleType("models") + fake_reviewer = MagicMock() + fake_reviewer.invoke.return_value = MagicMock(content="APPROVE") + fake_models.get_cross_reviewer = lambda: fake_reviewer # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, "models", fake_models) + + invoker = make_cross_reviewer_invoker() + out = invoker("review this plan") + assert out == "APPROVE" + assert root in sys.path, ( + "invoker must bootstrap the orchestrator root onto sys.path" + )