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.
This commit is contained in:
parent
4a48f9459c
commit
75f1dc6f75
6 changed files with 97 additions and 7 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
),
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
)
|
||||
|
|
|
|||
Reference in a new issue