mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(orchestrator): briefing renders ToolSearch directive + current verb names
Smoke-8 follow-up. Two issues in _write_agent_briefing: 1. _build_tool_load_block was scraping role prompts for a "## Load on spawn" section that doesn't exist in any role file. Returned "" for every role → no ToolSearch directive in the briefing. Combined with weak models skipping the system-prompt-layer directive (#144), the agent's first action was Edit → "not enabled in this context." Fix: per-role tool list lives in the orchestrator (mirrors factories._base.py). Pre-renders the directive directly. developer and documenter get Edit + Write; QA/PMs/board get the common read-only set. 7 tests pin the contract. 2. The briefing's "Terminal tools (how to exit cleanly)" section still listed pre-gateway verb names: roboco_agent_idle, roboco_task_substitute, roboco_task_escalate, roboco_task_submit_qa, _qa_pass/fail, _docs_complete, _complete. Same rename pattern as #145's _TERMINAL_TOOLS set. Updated to: i_am_idle, i_am_blocked, unclaim, i_am_done, pass, fail, i_documented, complete, submit_up, escalate_up, escalate_to_ceo. The agent now reads the same directive in two places (system prompt + session briefing) — the second touch point catches weak models that skip the first.
This commit is contained in:
@@ -2175,23 +2175,61 @@ class AgentOrchestrator:
|
|||||||
|
|
||||||
_TOOL_LOAD_CACHE: ClassVar[dict[str, str]] = {}
|
_TOOL_LOAD_CACHE: ClassVar[dict[str, str]] = {}
|
||||||
|
|
||||||
|
# Per-role built-in tools that must be activated via ToolSearch before
|
||||||
|
# use. Mirrors the system-prompt layer's _ROLE_BUILTIN_TOOLS in
|
||||||
|
# roboco/agents/factories/_base.py — kept in sync because the briefing
|
||||||
|
# and the system prompt are independent code paths. Smoke-8 evidence:
|
||||||
|
# weak models skip the system-prompt directive; the briefing block is
|
||||||
|
# the second touch point that gets them to actually run ToolSearch.
|
||||||
|
_COMMON_BUILTIN_TOOLS: ClassVar[tuple[str, ...]] = (
|
||||||
|
"Read",
|
||||||
|
"Bash",
|
||||||
|
"Grep",
|
||||||
|
"Glob",
|
||||||
|
"Task",
|
||||||
|
"TodoWrite",
|
||||||
|
)
|
||||||
|
_ROLE_BUILTIN_TOOLS: ClassVar[dict[str, tuple[str, ...]]] = {
|
||||||
|
"developer": (*_COMMON_BUILTIN_TOOLS, "Edit", "Write"),
|
||||||
|
"documenter": (*_COMMON_BUILTIN_TOOLS, "Edit", "Write"),
|
||||||
|
"qa": _COMMON_BUILTIN_TOOLS,
|
||||||
|
"main_pm": _COMMON_BUILTIN_TOOLS,
|
||||||
|
"cell_pm": _COMMON_BUILTIN_TOOLS,
|
||||||
|
"product_owner": _COMMON_BUILTIN_TOOLS,
|
||||||
|
"head_marketing": _COMMON_BUILTIN_TOOLS,
|
||||||
|
"auditor": _COMMON_BUILTIN_TOOLS,
|
||||||
|
}
|
||||||
|
|
||||||
def _build_tool_load_block(self, role: str) -> str:
|
def _build_tool_load_block(self, role: str) -> str:
|
||||||
"""Build the mandatory first-action ToolSearch directive.
|
"""Build the mandatory first-action ToolSearch directive.
|
||||||
|
|
||||||
Weak models consistently skip the role prompt's `Load on spawn
|
Weak models consistently skip the system-prompt's directive (added
|
||||||
(one ToolSearch select: call)` line — then trip on "Edit exists
|
in #144) and call Edit/Write directly, hitting "Edit exists but is
|
||||||
but is not enabled in this context" when they try to edit a
|
not enabled in this context." Hoisting the same directive into the
|
||||||
file, or "No such tool available: mcp__roboco-flow__…" when
|
briefing (which is the first user-visible message after spawn) is
|
||||||
they try to act. Hoisting the directive into the briefing's
|
the second touch point. Pre-rendered from the per-role tool list
|
||||||
first block (with the exact query string inline) pulls the
|
— no role-file scrape required.
|
||||||
bootstrap into the prompt-most-salient position.
|
|
||||||
|
|
||||||
Returns empty string if we can't locate the role file — the
|
|
||||||
briefing still works without it.
|
|
||||||
"""
|
"""
|
||||||
if role in self._TOOL_LOAD_CACHE:
|
if role in self._TOOL_LOAD_CACHE:
|
||||||
return self._TOOL_LOAD_CACHE[role]
|
return self._TOOL_LOAD_CACHE[role]
|
||||||
block = self._read_tool_load_from_role_prompt(role)
|
tools = self._ROLE_BUILTIN_TOOLS.get(role)
|
||||||
|
if not tools:
|
||||||
|
block = ""
|
||||||
|
else:
|
||||||
|
tool_list = ",".join(tools)
|
||||||
|
block = (
|
||||||
|
"## First action required\n"
|
||||||
|
"\n"
|
||||||
|
"Before any other tool call you MUST run this ToolSearch.\n"
|
||||||
|
"Built-in tools (Edit, Write, Read, etc.) are deferred until\n"
|
||||||
|
"you activate them. Skipping this step makes Edit calls fail\n"
|
||||||
|
'with "Edit exists but is not enabled in this context" — '
|
||||||
|
"wasting your budget and stalling the task.\n"
|
||||||
|
"\n"
|
||||||
|
"Copy this verbatim as your first action:\n"
|
||||||
|
"\n"
|
||||||
|
f'```\nToolSearch(query="select:{tool_list}")\n```\n"\n"\n'
|
||||||
|
)
|
||||||
self._TOOL_LOAD_CACHE[role] = block
|
self._TOOL_LOAD_CACHE[role] = block
|
||||||
return block
|
return block
|
||||||
|
|
||||||
@@ -2311,15 +2349,19 @@ class AgentOrchestrator:
|
|||||||
f"- **Workspace:** `{workspace_path}`\n"
|
f"- **Workspace:** `{workspace_path}`\n"
|
||||||
f"{task_block}"
|
f"{task_block}"
|
||||||
"\n## Terminal tools (how to exit cleanly)\n"
|
"\n## Terminal tools (how to exit cleanly)\n"
|
||||||
"- `roboco_agent_idle()` — no work remaining\n"
|
"- `i_am_idle()` — no work remaining (every role)\n"
|
||||||
"- `roboco_task_substitute(reason=...)` — release the task\n"
|
"- `i_am_blocked(task_id, reason, ...)` — stuck (developer)\n"
|
||||||
"- `roboco_task_escalate(reason=...)` — escalate up the chain\n"
|
"- `unclaim(task_id)` — release a claim back to the pool\n"
|
||||||
"- `roboco_task_pause(checkpoint=...)` — save progress, resume later\n"
|
"- Role handoffs:\n"
|
||||||
"- Role handoffs: `roboco_task_submit_qa()`, `_qa_pass/fail()`, "
|
" - developer → `i_am_done(task_id, notes)` (submit for QA)\n"
|
||||||
"`_docs_complete()`, `_complete()`\n"
|
" - qa → `pass(task_id, notes)` / `fail(task_id, issues)`\n"
|
||||||
|
" - documenter → `i_documented(task_id, notes, files)`\n"
|
||||||
|
" - cell_pm → `complete(task_id, notes)` / `submit_up(...)`"
|
||||||
|
" / `escalate_up(...)`\n"
|
||||||
|
" - main_pm → `complete(...)` / `escalate_to_ceo(...)`\n"
|
||||||
"\n"
|
"\n"
|
||||||
"A Stop without a terminal tool will be rejected; a second Stop\n"
|
"A Stop without a terminal tool will be rejected; a second Stop\n"
|
||||||
"auto-substitutes with `reason='stopped_without_transition'`.\n"
|
"auto-substitutes the task so it can be picked up elsewhere.\n"
|
||||||
"\n"
|
"\n"
|
||||||
"## Budget\n"
|
"## Budget\n"
|
||||||
f"Soft-warn at {settings.agent_tool_call_warn} tool calls, "
|
f"Soft-warn at {settings.agent_tool_call_warn} tool calls, "
|
||||||
|
|||||||
@@ -0,0 +1,83 @@
|
|||||||
|
"""Smoke-8: briefing's _build_tool_load_block emits the ToolSearch directive
|
||||||
|
without depending on a "## Load on spawn" section in the role file.
|
||||||
|
|
||||||
|
The previous implementation scraped role prompts for that marker; since
|
||||||
|
no role file had it, the function returned "" and the briefing showed
|
||||||
|
agents no tool-load instruction. Combined with weak models skipping the
|
||||||
|
system-prompt directive, the agent went straight to Edit and hit "not
|
||||||
|
enabled in this context."
|
||||||
|
|
||||||
|
Fix: per-role tool list lives in the orchestrator (mirrors the
|
||||||
|
factories layer). Pre-renders the directive directly — no file scrape.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import re
|
||||||
|
from unittest.mock import patch
|
||||||
|
|
||||||
|
from roboco.runtime.orchestrator import AgentOrchestrator
|
||||||
|
|
||||||
|
|
||||||
|
def _orch() -> AgentOrchestrator:
|
||||||
|
with patch.object(AgentOrchestrator, "__init__", return_value=None):
|
||||||
|
orch = AgentOrchestrator.__new__(AgentOrchestrator)
|
||||||
|
orch._TOOL_LOAD_CACHE = {}
|
||||||
|
return orch
|
||||||
|
|
||||||
|
|
||||||
|
def test_developer_directive_includes_edit_and_write() -> None:
|
||||||
|
block = _orch()._build_tool_load_block("developer")
|
||||||
|
assert "First action required" in block
|
||||||
|
assert "ToolSearch" in block
|
||||||
|
assert "Edit" in block
|
||||||
|
assert "Write" in block
|
||||||
|
|
||||||
|
|
||||||
|
def test_documenter_directive_includes_edit_and_write() -> None:
|
||||||
|
block = _orch()._build_tool_load_block("documenter")
|
||||||
|
assert "Edit" in block
|
||||||
|
assert "Write" in block
|
||||||
|
|
||||||
|
|
||||||
|
def test_qa_directive_excludes_edit_and_write() -> None:
|
||||||
|
block = _orch()._build_tool_load_block("qa")
|
||||||
|
assert "First action required" in block
|
||||||
|
# The ToolSearch line itself: pull the comma list to be precise about
|
||||||
|
# which tools are listed (substring match would catch "TodoWrite").
|
||||||
|
match = re.search(r'select:([^"]+)"', block)
|
||||||
|
assert match is not None
|
||||||
|
tools = match.group(1).split(",")
|
||||||
|
assert "Edit" not in tools
|
||||||
|
assert "Write" not in tools
|
||||||
|
assert "Read" in tools
|
||||||
|
assert "Bash" in tools
|
||||||
|
|
||||||
|
|
||||||
|
def test_pm_directives_exclude_edit_and_write() -> None:
|
||||||
|
for role in ("main_pm", "cell_pm", "product_owner", "head_marketing", "auditor"):
|
||||||
|
block = _orch()._build_tool_load_block(role)
|
||||||
|
match = re.search(r'select:([^"]+)"', block)
|
||||||
|
assert match is not None
|
||||||
|
tools = match.group(1).split(",")
|
||||||
|
assert "Edit" not in tools, f"{role} must not list Edit"
|
||||||
|
assert "Write" not in tools, f"{role} must not list Write"
|
||||||
|
|
||||||
|
|
||||||
|
def test_unknown_role_returns_empty() -> None:
|
||||||
|
"""No directive for unknown roles (defensive)."""
|
||||||
|
block = _orch()._build_tool_load_block("nonexistent")
|
||||||
|
assert block == ""
|
||||||
|
|
||||||
|
|
||||||
|
def test_directive_warns_about_failure_mode() -> None:
|
||||||
|
block = _orch()._build_tool_load_block("developer")
|
||||||
|
assert "Edit exists but is not enabled" in block
|
||||||
|
|
||||||
|
|
||||||
|
def test_role_cache_works() -> None:
|
||||||
|
orch = _orch()
|
||||||
|
first = orch._build_tool_load_block("developer")
|
||||||
|
# Second call hits the cache (same return).
|
||||||
|
second = orch._build_tool_load_block("developer")
|
||||||
|
assert first is second # same string object
|
||||||
Reference in New Issue
Block a user