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
5f40418dd7
commit
85553ac917
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).
|
"""Build the live Claude-backed clarifier node (§3.3, §7.1 P1).
|
||||||
|
|
||||||
Composes the two committed leaves: the Claude clarifier callables
|
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 import make_clarifier_node
|
||||||
from agent_team.nodes.clarifier_llm import build_claude_clarifier_callables
|
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(
|
return make_clarifier_node(
|
||||||
assess_confidence=assess_confidence,
|
assess_confidence=assess_confidence,
|
||||||
generate_questions=generate_questions,
|
generate_questions=generate_questions,
|
||||||
|
|
|
||||||
|
|
@ -167,6 +167,13 @@ def make_fast_coder_invoker() -> "BuildCallable":
|
||||||
"""
|
"""
|
||||||
|
|
||||||
def _invoke(instruction: str) -> str:
|
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
|
from models import get_fast_coder # noqa: PLC0415 - intentional deferred import
|
||||||
|
|
||||||
coder = get_fast_coder()
|
coder = get_fast_coder()
|
||||||
|
|
|
||||||
|
|
@ -210,6 +210,14 @@ def make_cross_reviewer_invoker() -> PlanReviewer:
|
||||||
"""
|
"""
|
||||||
|
|
||||||
def _invoke(prompt: str, *, config: Any = None, **_kw: Any) -> str:
|
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.
|
# Deferred import: keep the orchestrator package out of module import.
|
||||||
from models import get_cross_reviewer # noqa: PLC0415
|
from models import get_cross_reviewer # noqa: PLC0415
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -529,16 +529,18 @@ def _build_coordinator(args: argparse.Namespace) -> Any:
|
||||||
"""
|
"""
|
||||||
from agent_team.coordinator import (
|
from agent_team.coordinator import (
|
||||||
Coordinator,
|
Coordinator,
|
||||||
|
default_clarify_node_factory,
|
||||||
default_plan_node_factory,
|
default_plan_node_factory,
|
||||||
default_review_wiring,
|
default_review_wiring,
|
||||||
)
|
)
|
||||||
|
|
||||||
transport = _build_transport(args)
|
transport = _build_transport(args)
|
||||||
# WS5 (D10): inject the Sea Haven handbook conventions into the planner prompt
|
# WS5 (D10): inject the Sea Haven handbook conventions into BOTH the clarifier
|
||||||
# via the context_provider seam. The provider is the zero-arg handbook loader,
|
# and the planner prompts via the context_provider seam. The provider is the
|
||||||
# which is itself fail-safe (returns "" when the handbook dir is absent), and
|
# zero-arg handbook loader, which is itself fail-safe (returns "" when the
|
||||||
# planner.plan_node additionally swallows provider errors — so this never
|
# handbook dir is absent), and the clarifier/planner additionally swallow
|
||||||
# affects a run where the handbook is unavailable.
|
# provider errors — so this never affects a run where the handbook is
|
||||||
|
# unavailable.
|
||||||
context_provider = _build_context_provider()
|
context_provider = _build_context_provider()
|
||||||
|
|
||||||
# Production runs the full P2 graph: the wrapped real planner + the bound
|
# 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(
|
coordinator = Coordinator(
|
||||||
db_path=args.db,
|
db_path=args.db,
|
||||||
transport=transport,
|
transport=transport,
|
||||||
|
build_clarify_node=lambda: default_clarify_node_factory(
|
||||||
|
context_provider=context_provider
|
||||||
|
),
|
||||||
build_plan_node=lambda: default_plan_node_factory(
|
build_plan_node=lambda: default_plan_node_factory(
|
||||||
context_provider=context_provider
|
context_provider=context_provider
|
||||||
),
|
),
|
||||||
|
|
|
||||||
|
|
@ -644,6 +644,7 @@ class _FakeCoordinator:
|
||||||
*,
|
*,
|
||||||
db_path: Any,
|
db_path: Any,
|
||||||
transport: Any,
|
transport: Any,
|
||||||
|
build_clarify_node: Any = None,
|
||||||
build_plan_node: Any = None,
|
build_plan_node: Any = None,
|
||||||
review_wiring: Any = None,
|
review_wiring: Any = None,
|
||||||
) -> None:
|
) -> None:
|
||||||
|
|
@ -651,6 +652,7 @@ class _FakeCoordinator:
|
||||||
self.transport = transport
|
self.transport = transport
|
||||||
# The production CLI opts the coordinator into the P2 graph by injecting
|
# The production CLI opts the coordinator into the P2 graph by injecting
|
||||||
# these factories; record them so the wiring is asserted, not ignored.
|
# 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.build_plan_node = build_plan_node
|
||||||
self.review_wiring = review_wiring
|
self.review_wiring = review_wiring
|
||||||
self.setup_called = False
|
self.setup_called = False
|
||||||
|
|
|
||||||
|
|
@ -158,3 +158,63 @@ def test_slack_listener_factory_forwards_new_task_callback(
|
||||||
new_task_callback=sentinel,
|
new_task_callback=sentinel,
|
||||||
)
|
)
|
||||||
assert captured["new_task_callback"] is 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