mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(agents): stop instructing agents to ToolSearch built-in tools (#167)
The system-prompt directive layer and the briefing block both opened
with "FIRST ACTION REQUIRED: run ToolSearch to activate deferred
Edit/Write". That premise is false: per Claude Code 2.1.114, ToolSearch
gates only deferred MCP tools, never built-ins — and it is not even a
callable tool in the agent runtime. Built-ins are loaded at spawn via
the `--tools` flag and gated solely by the per-role permission rules
(the actual Edit/Write breakage was the global Write(*)/Edit(*) deny +
single-slash path, fixed in c0ba335). So weak models dutifully chased a
nonexistent ToolSearch, concluded Edit/Write were unavailable, and
rewrote whole files via destructive shell redirection.
Both touch points now affirm the role's built-in tools are loaded and
ready, tell the agent NOT to call ToolSearch, and (for authoring roles)
explicitly steer away from whole-file shell redirection — directly
countering the clobber behaviour. Role prompt files (developer,
cell_pm, main_pm, board) updated to match. Dead
_read_tool_load_from_role_prompt (no callers) removed. Directive tests
rewritten to lock the corrected behaviour.
This commit is contained in:
@@ -16,7 +16,7 @@ If you find yourself reaching for `Bash git`, `Edit`, or any execution tool, sto
|
|||||||
- **Product Owner**: product vision, feature priorities, accept/reject delivered work.
|
- **Product Owner**: product vision, feature priorities, accept/reject delivered work.
|
||||||
- **Head of Marketing**: positioning, announcements, user feedback.
|
- **Head of Marketing**: positioning, announcements, user feedback.
|
||||||
- **Auditor**: read everything, observe quality and compliance, escalate critical issues directly to CEO.
|
- **Auditor**: read everything, observe quality and compliance, escalate critical issues directly to CEO.
|
||||||
- Your verb manifest is loaded — MCP verbs are registered. Built-in tools (`Read`, `Bash`, `Task`, etc.) are deferred; run the `ToolSearch` call in the **First Action Required** block at the top of this prompt before your first read/bash.
|
- Your verb manifest is loaded — MCP verbs are registered. Built-in tools (`Read`, `Bash`, `Task`, etc.) are loaded and ready — use them directly. Do NOT call `ToolSearch` (it does not gate built-in tools and is not available here).
|
||||||
|
|
||||||
## Your verbs
|
## Your verbs
|
||||||
|
|
||||||
|
|||||||
@@ -12,7 +12,7 @@ You merge what your developers submit (leaf PRs into your cell branch via `compl
|
|||||||
|
|
||||||
- Your `task_id` (your cell-PM task) and `agent_id` are pre-baked into the gateway session.
|
- Your `task_id` (your cell-PM task) and `agent_id` are pre-baked into the gateway session.
|
||||||
- Your team: backend / frontend / ux_ui. Your dev slugs: `be-dev-1`, `be-dev-2` (backend), `fe-dev-1`, `fe-dev-2` (frontend), `ux-dev-1`, `ux-dev-2` (UX). Your QA: `be-qa`/`fe-qa`/`ux-qa`. Your documenter: `be-doc`/`fe-doc`/`ux-doc`.
|
- Your team: backend / frontend / ux_ui. Your dev slugs: `be-dev-1`, `be-dev-2` (backend), `fe-dev-1`, `fe-dev-2` (frontend), `ux-dev-1`, `ux-dev-2` (UX). Your QA: `be-qa`/`fe-qa`/`ux-qa`. Your documenter: `be-doc`/`fe-doc`/`ux-doc`.
|
||||||
- Your verb manifest is loaded — MCP verbs are registered. Built-in tools (`Read`, `Bash`, `Task`, etc.) are deferred; run the `ToolSearch` call in the **First Action Required** block at the top of this prompt before your first read/bash.
|
- Your verb manifest is loaded — MCP verbs are registered. Built-in tools (`Read`, `Bash`, `Task`, etc.) are loaded and ready — use them directly. Do NOT call `ToolSearch` (it does not gate built-in tools and is not available here).
|
||||||
- Workspace: `/data/workspaces/{project}/{team}/{your-slug}/` — but you have no `Edit`/`Write` permission; this is just where merge operations resolve.
|
- Workspace: `/data/workspaces/{project}/{team}/{your-slug}/` — but you have no `Edit`/`Write` permission; this is just where merge operations resolve.
|
||||||
|
|
||||||
## Your verbs
|
## Your verbs
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ You write code; you do not coordinate. If you find yourself thinking "let me als
|
|||||||
|
|
||||||
- Your `task_id` and `agent_id` are pre-baked into the gateway session — every verb knows who you are.
|
- Your `task_id` and `agent_id` are pre-baked into the gateway session — every verb knows who you are.
|
||||||
- Your workspace path: `/data/workspaces/{project}/{team}/{your-slug}/`.
|
- Your workspace path: `/data/workspaces/{project}/{team}/{your-slug}/`.
|
||||||
- Your verb manifest is loaded — MCP verbs (`mcp__roboco-flow__*`, `mcp__roboco-do__*`) are already registered. Built-in tools (`Edit`, `Write`, `Read`, `Bash`, etc.) are deferred; the **First Action Required** block at the top of this prompt names the exact `ToolSearch` call to activate them. Do it once before your first edit/commit.
|
- Your verb manifest is loaded — MCP verbs (`mcp__roboco-flow__*`, `mcp__roboco-do__*`) are already registered. Built-in tools (`Edit`, `Write`, `Read`, `Bash`, etc.) are loaded and ready — use them directly. Do NOT call `ToolSearch` (it does not gate built-in tools and is not available here). Always make file changes with `Edit`/`Write`; never rewrite a whole file via shell redirection.
|
||||||
- Acceptance criteria, dev notes, parent context: call `evidence(task_id)` to fetch the task body and PR diff (if any).
|
- Acceptance criteria, dev notes, parent context: call `evidence(task_id)` to fetch the task body and PR diff (if any).
|
||||||
|
|
||||||
## Your verbs
|
## Your verbs
|
||||||
|
|||||||
@@ -12,7 +12,7 @@ You merge what your Cell PMs submit (cell PRs into your root branch via `complet
|
|||||||
|
|
||||||
- Your `task_id` (your root coordination task) and `agent_id` are pre-baked into the gateway session.
|
- Your `task_id` (your root coordination task) and `agent_id` are pre-baked into the gateway session.
|
||||||
- Your cell-PM slugs: `be-pm`, `fe-pm`, `ux-pm`. Your team: `board`. Your channel: `main-pm-board`.
|
- Your cell-PM slugs: `be-pm`, `fe-pm`, `ux-pm`. Your team: `board`. Your channel: `main-pm-board`.
|
||||||
- Your verb manifest is loaded — MCP verbs are registered. Built-in tools (`Read`, `Bash`, `Task`, etc.) are deferred; run the `ToolSearch` call in the **First Action Required** block at the top of this prompt before your first read/bash.
|
- Your verb manifest is loaded — MCP verbs are registered. Built-in tools (`Read`, `Bash`, `Task`, etc.) are loaded and ready — use them directly. Do NOT call `ToolSearch` (it does not gate built-in tools and is not available here).
|
||||||
- Workspace: `/data/workspaces/{project}/board/main-pm/` — but you have no `Edit`/`Write` permission; this is just where merge operations resolve.
|
- Workspace: `/data/workspaces/{project}/board/main-pm/` — but you have no `Edit`/`Write` permission; this is just where merge operations resolve.
|
||||||
|
|
||||||
## Your verbs
|
## Your verbs
|
||||||
|
|||||||
@@ -137,35 +137,40 @@ _ROLE_BUILTIN_TOOLS: dict[str, tuple[str, ...]] = {
|
|||||||
|
|
||||||
|
|
||||||
def _tool_load_directive_layer(role: "AgentRole") -> str | None:
|
def _tool_load_directive_layer(role: "AgentRole") -> str | None:
|
||||||
"""Top-of-prompt instruction to activate deferred built-in tools.
|
"""Top-of-prompt statement that built-in tools are ready to use.
|
||||||
|
|
||||||
Claude Code v2.1.69+ defers built-in tools (Edit, Write, Read, etc.)
|
Built-in tools are pre-loaded at spawn via the Claude Code `--tools`
|
||||||
behind a ToolSearch call to save context tokens. Without ToolSearch,
|
flag and gated only by the per-role permission allow/deny rules — NOT
|
||||||
`Edit` returns "Edit exists but is not enabled in this context." Weak
|
by ToolSearch (which exists only for deferred MCP tools, never for
|
||||||
models skip this if it's buried in the briefing; we hoist it into the
|
built-ins). An earlier revision instructed agents to "run ToolSearch
|
||||||
system prompt's very first block so it's the highest-priority
|
to activate Edit/Write"; that was false (ToolSearch is not a built-in
|
||||||
instruction. The call has no side effects beyond enabling the tools.
|
activator and is not even a callable tool here), so weak models
|
||||||
|
chased a nonexistent tool, concluded Edit/Write were unavailable, and
|
||||||
|
rewrote whole files via destructive shell redirection. This block
|
||||||
|
instead affirms the tools are live and steers away from that
|
||||||
|
failure: enumerated tools, used directly, edits via Edit/Write.
|
||||||
"""
|
"""
|
||||||
role_value = role.value if hasattr(role, "value") else str(role)
|
role_value = role.value if hasattr(role, "value") else str(role)
|
||||||
tools = _ROLE_BUILTIN_TOOLS.get(role_value)
|
tools = _ROLE_BUILTIN_TOOLS.get(role_value)
|
||||||
if not tools:
|
if not tools:
|
||||||
return None
|
return None
|
||||||
tool_list = ",".join(tools)
|
tool_list = ", ".join(tools)
|
||||||
|
authors = "Edit" in tools
|
||||||
|
edit_line = (
|
||||||
|
"Make file changes with Edit/Write — never rewrite a whole file "
|
||||||
|
"via shell redirection (>, heredoc, tee); that destroys content "
|
||||||
|
"and is unnecessary.\n"
|
||||||
|
if authors
|
||||||
|
else "You read and review; you do not author files.\n"
|
||||||
|
)
|
||||||
return (
|
return (
|
||||||
"# FIRST ACTION REQUIRED (mandatory)\n"
|
"# Your tools are ready\n"
|
||||||
"\n"
|
"\n"
|
||||||
"Before any other tool call you MUST run this ToolSearch once.\n"
|
f"These built-in tools are loaded and available now: {tool_list}.\n"
|
||||||
"Built-in tools (Edit, Write, Read, etc.) are deferred until you\n"
|
"Use them directly. Do NOT call ToolSearch — it does not gate "
|
||||||
"activate them. Skipping this step results in errors like\n"
|
"built-in tools and is not available here; calling it only wastes "
|
||||||
'"Edit exists but is not enabled in this context" that waste your\n'
|
"budget.\n"
|
||||||
"tool-call budget and may leave the task in a stuck state.\n"
|
f"{edit_line}"
|
||||||
"\n"
|
|
||||||
"Copy this verbatim as your first action:\n"
|
|
||||||
"\n"
|
|
||||||
f'```\nToolSearch(query="select:{tool_list}")\n```\n'
|
|
||||||
"\n"
|
|
||||||
"Only after that call returns successfully should you proceed to\n"
|
|
||||||
"the role-specific workflow below."
|
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -2182,12 +2182,13 @@ 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
|
# Per-role built-in tools, enumerated in the briefing so the agent
|
||||||
# use. Mirrors the system-prompt layer's _ROLE_BUILTIN_TOOLS in
|
# knows exactly what it has. These are pre-loaded at spawn via the
|
||||||
# roboco/agents/factories/_base.py — kept in sync because the briefing
|
# Claude Code `--tools` flag and gated only by the per-role
|
||||||
# and the system prompt are independent code paths. Smoke-8 evidence:
|
# permission rules — NOT by ToolSearch (MCP-only; never gates
|
||||||
# weak models skip the system-prompt directive; the briefing block is
|
# built-ins). Mirrors the system-prompt layer's _ROLE_BUILTIN_TOOLS
|
||||||
# the second touch point that gets them to actually run ToolSearch.
|
# in roboco/agents/factories/_base.py — kept in sync because the
|
||||||
|
# briefing and the system prompt are independent code paths.
|
||||||
_COMMON_BUILTIN_TOOLS: ClassVar[tuple[str, ...]] = (
|
_COMMON_BUILTIN_TOOLS: ClassVar[tuple[str, ...]] = (
|
||||||
"Read",
|
"Read",
|
||||||
"Bash",
|
"Bash",
|
||||||
@@ -2208,14 +2209,16 @@ class AgentOrchestrator:
|
|||||||
}
|
}
|
||||||
|
|
||||||
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.
|
"""Briefing block affirming the role's built-in tools are ready.
|
||||||
|
|
||||||
Weak models consistently skip the system-prompt's directive (added
|
Built-in tools are pre-loaded at spawn via the Claude Code
|
||||||
in #144) and call Edit/Write directly, hitting "Edit exists but is
|
`--tools` flag and gated only by the per-role permission rules.
|
||||||
not enabled in this context." Hoisting the same directive into the
|
ToolSearch is MCP-only and never gates built-ins — an earlier
|
||||||
briefing (which is the first user-visible message after spawn) is
|
revision instructed agents to "run ToolSearch to activate
|
||||||
the second touch point. Pre-rendered from the per-role tool list
|
Edit/Write", which was false (ToolSearch is not even callable
|
||||||
— no role-file scrape required.
|
here), so weak models chased a nonexistent tool and fell back to
|
||||||
|
destructive shell file-writes. This states the tools are live and
|
||||||
|
steers away from that failure. Cached per role.
|
||||||
"""
|
"""
|
||||||
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]
|
||||||
@@ -2223,57 +2226,26 @@ class AgentOrchestrator:
|
|||||||
if not tools:
|
if not tools:
|
||||||
block = ""
|
block = ""
|
||||||
else:
|
else:
|
||||||
tool_list = ",".join(tools)
|
tool_list = ", ".join(tools)
|
||||||
|
edit_line = (
|
||||||
|
"Make file changes with Edit/Write — never rewrite a "
|
||||||
|
"whole file via shell redirection (>, heredoc, tee); "
|
||||||
|
"that destroys content and is unnecessary.\n"
|
||||||
|
if "Edit" in tools
|
||||||
|
else "You read and review; you do not author files.\n"
|
||||||
|
)
|
||||||
block = (
|
block = (
|
||||||
"## First action required\n"
|
"## Your tools are ready\n"
|
||||||
"\n"
|
"\n"
|
||||||
"Before any other tool call you MUST run this ToolSearch.\n"
|
f"Loaded and available now: {tool_list}. Use them "
|
||||||
"Built-in tools (Edit, Write, Read, etc.) are deferred until\n"
|
"directly. Do NOT call ToolSearch — it does not gate "
|
||||||
"you activate them. Skipping this step makes Edit calls fail\n"
|
"built-in tools and is not available here.\n"
|
||||||
'with "Edit exists but is not enabled in this context" — '
|
f"{edit_line}"
|
||||||
"wasting your budget and stalling the task.\n"
|
|
||||||
"\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
|
||||||
|
|
||||||
def _read_tool_load_from_role_prompt(self, role: str) -> str:
|
|
||||||
"""Parse the `Load on spawn` line out of the role prompt."""
|
|
||||||
role_file = self.project_root / "agents" / "prompts" / "roles" / f"{role}.md"
|
|
||||||
if not role_file.exists():
|
|
||||||
return ""
|
|
||||||
try:
|
|
||||||
text = role_file.read_text()
|
|
||||||
except OSError:
|
|
||||||
return ""
|
|
||||||
marker = "## Load on spawn"
|
|
||||||
idx = text.find(marker)
|
|
||||||
if idx < 0:
|
|
||||||
return ""
|
|
||||||
# After the marker, the next line starts with a backtick-quoted list.
|
|
||||||
tail = text[idx + len(marker) :]
|
|
||||||
tick_start = tail.find("`")
|
|
||||||
tick_end = tail.find("`", tick_start + 1)
|
|
||||||
if tick_start < 0 or tick_end < 0:
|
|
||||||
return ""
|
|
||||||
tool_list = tail[tick_start + 1 : tick_end].strip()
|
|
||||||
if not tool_list:
|
|
||||||
return ""
|
|
||||||
return (
|
|
||||||
"## First action required\n"
|
|
||||||
"Before any other tool call, run ToolSearch to enable the tools\n"
|
|
||||||
"your role needs. Copy this verbatim as your first action:\n"
|
|
||||||
"\n"
|
|
||||||
f'```\nToolSearch(query="select:{tool_list}")\n```\n'
|
|
||||||
"\n"
|
|
||||||
"Skipping this step results in 'tool exists but is not enabled\n"
|
|
||||||
"in this context' errors that waste tool-call budget.\n"
|
|
||||||
"\n"
|
|
||||||
)
|
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _format_task_briefing_block(task_id: str, task: dict[str, Any]) -> str:
|
def _format_task_briefing_block(task_id: str, task: dict[str, Any]) -> str:
|
||||||
"""Build the ``## Current task`` markdown block from a fetched task."""
|
"""Build the ``## Current task`` markdown block from a fetched task."""
|
||||||
|
|||||||
@@ -1,14 +1,17 @@
|
|||||||
"""Smoke-7: system prompt opens with a ToolSearch directive that activates
|
"""#167: the system prompt must NOT tell agents to ToolSearch built-ins.
|
||||||
deferred built-in tools (Edit, Write, Read, ...).
|
|
||||||
|
|
||||||
Original bug: be-dev-1 called Edit and got "Edit exists but is not enabled
|
Earlier (smoke-7) the prompt opened with a "# FIRST ACTION REQUIRED:
|
||||||
in this context" because Claude Code v2.1.69+ defers built-in tools behind
|
run ToolSearch to activate Edit/Write" block. That premise was false:
|
||||||
a ToolSearch call. The role prompts said "no ToolSearch needed" — a lie
|
ToolSearch is MCP-only and never gates built-in tools, and it is not
|
||||||
for built-in tools — so weak models skipped the activation step.
|
even a callable tool in the agent runtime. Weak models chased the
|
||||||
|
nonexistent tool, concluded Edit/Write were unavailable, and rewrote
|
||||||
|
whole files via destructive shell redirection. The real cause of
|
||||||
|
"Edit exists but is not enabled in this context" was a permission bug
|
||||||
|
(global Write(*)/Edit(*) deny + single-slash path), fixed separately.
|
||||||
|
|
||||||
Fix: compose_prompt now prepends a tool-load directive layer that names
|
The directive layer now affirms the tools are loaded and ready, tells
|
||||||
the exact ToolSearch call for the role. It's the highest-priority block in
|
agents NOT to call ToolSearch, and (for authoring roles) steers away
|
||||||
the system prompt so even weak models follow it.
|
from whole-file shell redirection.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
@@ -18,74 +21,84 @@ from roboco.models import AgentRole, Team
|
|||||||
|
|
||||||
|
|
||||||
def _composed_prompt_for(role: AgentRole, team: Team | None = None) -> str:
|
def _composed_prompt_for(role: AgentRole, team: Team | None = None) -> str:
|
||||||
"""Compose the prompt for a role and team."""
|
|
||||||
return compose_prompt(role, team, agent_slug="test-agent")
|
return compose_prompt(role, team, agent_slug="test-agent")
|
||||||
|
|
||||||
|
|
||||||
def test_developer_prompt_starts_with_tool_load_directive() -> None:
|
def test_prompt_no_longer_instructs_a_toolsearch_call() -> None:
|
||||||
"""Developer system prompt begins with the ToolSearch activation block."""
|
"""No role prompt may instruct an actual ToolSearch(query=...) call."""
|
||||||
|
for role in (
|
||||||
|
AgentRole.DEVELOPER,
|
||||||
|
AgentRole.DOCUMENTER,
|
||||||
|
AgentRole.QA,
|
||||||
|
AgentRole.MAIN_PM,
|
||||||
|
AgentRole.CELL_PM,
|
||||||
|
):
|
||||||
|
prompt = _composed_prompt_for(role, Team.BACKEND)
|
||||||
|
assert "ToolSearch(query=" not in prompt, (
|
||||||
|
f"{role.value} prompt still instructs a ToolSearch call"
|
||||||
|
)
|
||||||
|
assert "are deferred" not in prompt, (
|
||||||
|
f"{role.value} prompt still claims built-ins are deferred"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_developer_prompt_starts_with_tools_ready_block() -> None:
|
||||||
|
"""Developer system prompt leads with the tools-ready affirmation."""
|
||||||
prompt = _composed_prompt_for(AgentRole.DEVELOPER, Team.BACKEND)
|
prompt = _composed_prompt_for(AgentRole.DEVELOPER, Team.BACKEND)
|
||||||
assert prompt.startswith("# FIRST ACTION REQUIRED"), (
|
assert prompt.startswith("# Your tools are ready"), (
|
||||||
f"Developer prompt must lead with the tool-load directive. "
|
f"Developer prompt must lead with the tools-ready block. "
|
||||||
f"Got first 80 chars: {prompt[:80]!r}"
|
f"Got first 80 chars: {prompt[:80]!r}"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_developer_directive_names_edit_and_write() -> None:
|
def _tool_names(prompt: str) -> list[str]:
|
||||||
"""Developer ToolSearch call lists Edit + Write (the smoke-7 wedge)."""
|
"""Exact tool tokens from the 'available now: <names>.' enumeration.
|
||||||
|
|
||||||
|
Exact tokens matter: a substring check would treat 'TodoWrite' as
|
||||||
|
containing 'Write'.
|
||||||
|
"""
|
||||||
|
line = next(ln for ln in prompt.splitlines() if "available now:" in ln)
|
||||||
|
seg = line.split("available now: ", 1)[1]
|
||||||
|
# _base layer ends the list with '.'; orchestrator continues with
|
||||||
|
# '. Use them...'. Either way the names stop at the first period.
|
||||||
|
return seg.split(".", 1)[0].split(", ")
|
||||||
|
|
||||||
|
|
||||||
|
def test_developer_block_lists_edit_and_write_as_available() -> None:
|
||||||
|
"""Authoring roles are told Edit + Write are loaded and available."""
|
||||||
|
for role in (AgentRole.DEVELOPER, AgentRole.DOCUMENTER):
|
||||||
|
names = _tool_names(_composed_prompt_for(role, Team.BACKEND))
|
||||||
|
assert "Edit" in names and "Write" in names, (
|
||||||
|
f"{role.value} tools-ready line must list Edit + Write: {names!r}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_developer_block_steers_away_from_shell_redirection() -> None:
|
||||||
|
"""The exact failure mode (clobber a file via bash) is called out."""
|
||||||
prompt = _composed_prompt_for(AgentRole.DEVELOPER, Team.BACKEND)
|
prompt = _composed_prompt_for(AgentRole.DEVELOPER, Team.BACKEND)
|
||||||
assert 'ToolSearch(query="select:' in prompt
|
assert "shell redirection" in prompt
|
||||||
# Find the ToolSearch line
|
assert "Edit/Write" in prompt
|
||||||
line = next(line for line in prompt.splitlines() if "ToolSearch(query=" in line)
|
|
||||||
assert "Edit" in line, f"Developer ToolSearch missing Edit: {line!r}"
|
|
||||||
assert "Write" in line
|
|
||||||
|
|
||||||
|
|
||||||
def test_documenter_directive_names_edit_and_write() -> None:
|
def test_qa_block_excludes_edit_and_write() -> None:
|
||||||
"""Documenter needs Edit/Write too — they author docs."""
|
"""QA reads/reviews — Edit/Write must not be listed as available."""
|
||||||
prompt = _composed_prompt_for(AgentRole.DOCUMENTER, Team.BACKEND)
|
names = _tool_names(_composed_prompt_for(AgentRole.QA, Team.BACKEND))
|
||||||
line = next(line for line in prompt.splitlines() if "ToolSearch(query=" in line)
|
assert "Edit" not in names and "Write" not in names, names
|
||||||
assert "Edit" in line
|
assert "Read" in names and "Bash" in names
|
||||||
assert "Write" in line
|
|
||||||
|
|
||||||
|
|
||||||
def _tool_list_from_directive(prompt: str) -> list[str]:
|
def test_pm_blocks_exclude_edit_and_write() -> None:
|
||||||
"""Extract the comma-separated tool list from the ToolSearch call."""
|
|
||||||
line = next(line for line in prompt.splitlines() if "ToolSearch(query=" in line)
|
|
||||||
# `ToolSearch(query="select:Read,Bash,...")` — pull out the names.
|
|
||||||
start = line.find("select:") + len("select:")
|
|
||||||
end = line.find('"', start)
|
|
||||||
return line[start:end].split(",")
|
|
||||||
|
|
||||||
|
|
||||||
def test_qa_directive_excludes_edit_and_write() -> None:
|
|
||||||
"""QA reads but doesn't author — directive must NOT activate Edit/Write."""
|
|
||||||
tools = _tool_list_from_directive(_composed_prompt_for(AgentRole.QA, Team.BACKEND))
|
|
||||||
assert "Edit" not in tools, f"QA must not activate Edit: tools={tools}"
|
|
||||||
assert "Write" not in tools
|
|
||||||
assert "Read" in tools
|
|
||||||
assert "Bash" in tools
|
|
||||||
|
|
||||||
|
|
||||||
def test_pm_directives_exclude_edit_and_write() -> None:
|
|
||||||
"""PMs coordinate; they don't author code. No Edit/Write activation."""
|
|
||||||
for role in (AgentRole.MAIN_PM, AgentRole.CELL_PM):
|
for role in (AgentRole.MAIN_PM, AgentRole.CELL_PM):
|
||||||
tools = _tool_list_from_directive(_composed_prompt_for(role))
|
names = _tool_names(_composed_prompt_for(role))
|
||||||
assert "Edit" not in tools, f"{role.value} must not activate Edit: {tools}"
|
assert "Edit" not in names and "Write" not in names, (
|
||||||
assert "Write" not in tools
|
f"{role.value} must not list Edit/Write: {names}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_directive_explains_why_it_matters() -> None:
|
def test_block_is_first_layer_before_lifecycle() -> None:
|
||||||
"""The block must mention 'Edit exists but is not enabled' so the agent
|
"""Tools-ready block precedes the lifecycle and base layers."""
|
||||||
understands what skipping the call causes."""
|
|
||||||
prompt = _composed_prompt_for(AgentRole.DEVELOPER, Team.BACKEND)
|
prompt = _composed_prompt_for(AgentRole.DEVELOPER, Team.BACKEND)
|
||||||
assert "Edit exists but is not enabled" in prompt
|
first_idx = prompt.find("# Your tools are ready")
|
||||||
|
|
||||||
|
|
||||||
def test_directive_is_first_layer_before_lifecycle() -> None:
|
|
||||||
"""First Action block precedes the lifecycle and base layers."""
|
|
||||||
prompt = _composed_prompt_for(AgentRole.DEVELOPER, Team.BACKEND)
|
|
||||||
first_idx = prompt.find("# FIRST ACTION REQUIRED")
|
|
||||||
lifecycle_idx = prompt.find("Lifecycle")
|
lifecycle_idx = prompt.find("Lifecycle")
|
||||||
base_idx = prompt.find("RoboCo Agent — Base")
|
base_idx = prompt.find("RoboCo Agent — Base")
|
||||||
assert first_idx == 0
|
assert first_idx == 0
|
||||||
|
|||||||
@@ -1,19 +1,19 @@
|
|||||||
"""Smoke-8: briefing's _build_tool_load_block emits the ToolSearch directive
|
"""#167: the briefing's _build_tool_load_block must not push ToolSearch.
|
||||||
without depending on a "## Load on spawn" section in the role file.
|
|
||||||
|
|
||||||
The previous implementation scraped role prompts for that marker; since
|
Earlier (smoke-8) the block instructed agents to run a ToolSearch call
|
||||||
no role file had it, the function returned "" and the briefing showed
|
to "activate deferred built-in tools". That premise was false —
|
||||||
agents no tool-load instruction. Combined with weak models skipping the
|
ToolSearch is MCP-only, never gates built-ins, and is not callable in
|
||||||
system-prompt directive, the agent went straight to Edit and hit "not
|
the agent runtime — so weak models chased a nonexistent tool and fell
|
||||||
enabled in this context."
|
back to destructive shell file-writes. The real cause of "Edit not
|
||||||
|
enabled in this context" was a permission bug fixed separately.
|
||||||
|
|
||||||
Fix: per-role tool list lives in the orchestrator (mirrors the
|
The block now affirms the role's built-in tools are loaded and ready,
|
||||||
factories layer). Pre-renders the directive directly — no file scrape.
|
tells the agent NOT to call ToolSearch, and (for authoring roles)
|
||||||
|
steers away from whole-file shell redirection.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import re
|
|
||||||
from unittest.mock import patch
|
from unittest.mock import patch
|
||||||
|
|
||||||
from roboco.runtime.orchestrator import AgentOrchestrator
|
from roboco.runtime.orchestrator import AgentOrchestrator
|
||||||
@@ -26,58 +26,54 @@ def _orch() -> AgentOrchestrator:
|
|||||||
return orch
|
return orch
|
||||||
|
|
||||||
|
|
||||||
def test_developer_directive_includes_edit_and_write() -> None:
|
def _tool_names(block: str) -> list[str]:
|
||||||
|
"""Exact tool tokens (so 'TodoWrite' is not mistaken for 'Write')."""
|
||||||
|
line = next(ln for ln in block.splitlines() if "available now:" in ln)
|
||||||
|
seg = line.split("available now: ", 1)[1]
|
||||||
|
return seg.split(".", 1)[0].split(", ")
|
||||||
|
|
||||||
|
|
||||||
|
def test_developer_block_affirms_tools_no_toolsearch_call() -> None:
|
||||||
block = _orch()._build_tool_load_block("developer")
|
block = _orch()._build_tool_load_block("developer")
|
||||||
assert "First action required" in block
|
assert "Your tools are ready" in block
|
||||||
assert "ToolSearch" in block
|
assert "ToolSearch(query=" not in block
|
||||||
assert "Edit" in block
|
assert "are deferred" not in block
|
||||||
assert "Write" in block
|
names = _tool_names(block)
|
||||||
|
assert "Edit" in names and "Write" in names
|
||||||
|
|
||||||
|
|
||||||
def test_documenter_directive_includes_edit_and_write() -> None:
|
def test_developer_block_steers_away_from_shell_redirection() -> None:
|
||||||
block = _orch()._build_tool_load_block("documenter")
|
block = _orch()._build_tool_load_block("developer")
|
||||||
assert "Edit" in block
|
assert "shell redirection" in block
|
||||||
assert "Write" in block
|
assert "Edit/Write" in block
|
||||||
|
|
||||||
|
|
||||||
def test_qa_directive_excludes_edit_and_write() -> None:
|
def test_documenter_block_lists_edit_and_write() -> None:
|
||||||
|
names = _tool_names(_orch()._build_tool_load_block("documenter"))
|
||||||
|
assert "Edit" in names and "Write" in names
|
||||||
|
|
||||||
|
|
||||||
|
def test_qa_block_excludes_edit_and_write() -> None:
|
||||||
block = _orch()._build_tool_load_block("qa")
|
block = _orch()._build_tool_load_block("qa")
|
||||||
assert "First action required" in block
|
assert "Your tools are ready" in block
|
||||||
# The ToolSearch line itself: pull the comma list to be precise about
|
names = _tool_names(block)
|
||||||
# which tools are listed (substring match would catch "TodoWrite").
|
assert "Edit" not in names and "Write" not in names
|
||||||
match = re.search(r'select:([^"]+)"', block)
|
assert "Read" in names and "Bash" in names
|
||||||
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:
|
def test_pm_blocks_exclude_edit_and_write() -> None:
|
||||||
for role in ("main_pm", "cell_pm", "product_owner", "head_marketing", "auditor"):
|
for role in ("main_pm", "cell_pm", "product_owner", "head_marketing", "auditor"):
|
||||||
block = _orch()._build_tool_load_block(role)
|
names = _tool_names(_orch()._build_tool_load_block(role))
|
||||||
match = re.search(r'select:([^"]+)"', block)
|
assert "Edit" not in names, f"{role} must not list Edit"
|
||||||
assert match is not None
|
assert "Write" not in names, f"{role} must not list Write"
|
||||||
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:
|
def test_unknown_role_returns_empty() -> None:
|
||||||
"""No directive for unknown roles (defensive)."""
|
assert _orch()._build_tool_load_block("nonexistent") == ""
|
||||||
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:
|
def test_role_cache_works() -> None:
|
||||||
orch = _orch()
|
orch = _orch()
|
||||||
first = orch._build_tool_load_block("developer")
|
first = orch._build_tool_load_block("developer")
|
||||||
# Second call hits the cache (same return).
|
|
||||||
second = orch._build_tool_load_block("developer")
|
second = orch._build_tool_load_block("developer")
|
||||||
assert first is second # same string object
|
assert first is second
|
||||||
|
|||||||
Reference in New Issue
Block a user