Skip to content

Commit b94857a

Browse files
committed
Improve subagent and planexit tool.
1 parent 1bfbe66 commit b94857a

8 files changed

Lines changed: 274 additions & 10 deletions

File tree

python_agent_harness/agent.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,13 @@ def compact(self) -> bool:
162162
# tool execution
163163
# ------------------------------------------------------------------
164164
def _execute_tool_call(self, call: ToolCall) -> str:
165+
if not self.top_level and call.name in config.SUBAGENT_EXCLUDED_TOOLS:
166+
# defense in depth: a hallucinated call must never reach the
167+
# registry — the spec was filtered, so refuse it here too
168+
return (
169+
f"Error: {call.name} is not available to sub-agents — "
170+
"one-shot/interactive tools are parent-only"
171+
)
165172
args = call.arguments
166173
if isinstance(args, str):
167174
try:
@@ -256,9 +263,16 @@ def safe_delta(text: str) -> None:
256263
session.on_delta(text)
257264

258265
try:
266+
# sub-agents are one-shot tasks: they must not see (or
267+
# call) parent-only tools — Agent (no nesting), Question
268+
# and PlanExit (interactive/handoff) — filtered from the
269+
# specs before sending
270+
tools = session.tool_specs(
271+
exclude=config.SUBAGENT_EXCLUDED_TOOLS if not self.top_level else ()
272+
)
259273
assistant, usage = session.client.chat(
260274
self.messages,
261-
tools=session.tool_specs() if session.tools_enabled else None,
275+
tools=tools if session.tools_enabled else None,
262276
system=self.system,
263277
temperature=session.temperature,
264278
max_tokens=session.max_tokens,

python_agent_harness/agent_session.py

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,13 @@ def ask_questions(self, questions: list[dict]) -> str:
164164
# ------------------------------------------------------------------
165165
# tools
166166
# ------------------------------------------------------------------
167-
def tool_specs(self) -> list:
168-
return self.registry.specs()
167+
def tool_specs(self, exclude: tuple[str, ...] = ()) -> list:
168+
"""Tool specs exposed to the model; ``exclude`` drops tools by
169+
name (e.g. one-shot/interactive tools for sub-agent runs)."""
170+
return [
171+
spec for spec in self.registry.specs()
172+
if spec.name not in exclude
173+
]
169174

170175
def execute_tool(
171176
self, name: str, args: dict[str, Any], call_id: str | None = None
@@ -401,7 +406,13 @@ def run_subagent(self, subagent_type: str, description: str, prompt: str) -> str
401406
self.pop_todo_scope()
402407

403408
def plan_exit(self) -> str:
404-
"""PlanExit tool implementation."""
409+
"""PlanExit tool implementation.
410+
411+
Asks the user to approve the plan→build switch through a y/n
412+
confirmation UI (rendered like the Question tool's choice list,
413+
but keyed with y/n instead of numbers). The TUI hook decides
414+
the exact look; the session only interprets the boolean answer.
415+
"""
405416
if not self.plan_mode.is_plan:
406417
return "Not in plan mode; PlanExit has no effect. Continue as normal."
407418
approved = self.confirm(

python_agent_harness/config.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,13 @@
135135
"The plan at %s has been approved, you can now edit files. Execute the plan"
136136
)
137137

138+
# PlanExit asks the user with the same choice UI as the Question tool:
139+
# option[0] approves the switch to build mode, anything else rejects it.
140+
PLAN_EXIT_OPTIONS = (
141+
"Yes, switch to build agent",
142+
"No, keep refining the plan",
143+
)
144+
138145
# ---- tools -------------------------------------------------------------------
139146
DEFAULT_TOOLS: list[str] = [
140147
"Agent", "TodoWrite", "Glob", "Grep", "Read", "Insert", "Edit",
@@ -149,6 +156,11 @@
149156

150157
# ---- sub-agents ---------------------------------------------------------------
151158
SUBAGENT_MAX_ROUNDS = 60
159+
# Tools a sub-agent must NOT see or call: it runs autonomously as a
160+
# one-shot task inside the parent's tool round, so it cannot spawn
161+
# further sub-agents (Agent), ask the user questions (Question), nor
162+
# end in a plan/build handoff (PlanExit).
163+
SUBAGENT_EXCLUDED_TOOLS = ("Agent", "Question", "PlanExit")
152164

153165
# ---- TUI preview limits -------------------------------------------------------
154166
TOOL_RESULT_PREVIEW_LINES = 5 # max lines of a tool result shown in the TUI

python_agent_harness/prompts/subagent.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ You are an autonomous subagent. Your role is to independently complete well-defi
1515
- If you lack information needed to proceed, make reasonable assumptions based on context
1616

1717
# Tool usage policy
18+
- You do NOT have access to the `Agent`, `Question`, and `PlanExit`
19+
tools — they are parent-only (one-shot/interactive). Run autonomously
20+
to completion; never delegate work to further sub-agents.
1821
- You have the capability to call multiple tools in a single response. When multiple independent pieces of information are requested, batch your tool calls together for optimal performance. When making multiple bash tool calls, you MUST send a single message with multiple tools calls to run the calls in parallel. For example, if you need to run "git status" and "git diff", send a single message with two tool calls to run the calls in parallel.
1922

2023
You MUST answer concisely with fewer than 4 lines of text (not including tool use or code generation), unless user asks for detail.

python_agent_harness/tui.py

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -234,15 +234,38 @@ def get_completions(self, document: Any, complete_event: Any):
234234
class UiQuestion:
235235
def __init__(self, prompt: str, multiple: bool = False,
236236
options: list[str] | None = None,
237-
custom: bool = True) -> None:
237+
custom: bool = True,
238+
keys: list[str] | None = None) -> None:
238239
self.prompt = prompt
239240
self.multiple = multiple
240241
self.options = options or []
241242
self.custom = custom
243+
# keyed choices (e.g. ["y", "n"] for a confirm): render the
244+
# options as a keyed list and resolve typed keys to labels,
245+
# instead of the numbered-list style of the Question tool
246+
self.keys = keys or []
242247
self.answer: str | None = None
243248
self.event = threading.Event()
244249

245250

251+
def _resolve_keyed_choice(answer: str, options: list[str], keys: list[str]) -> str:
252+
"""Map bare keys in ANSWER to the matching option label.
253+
254+
Comma-separated keys pick several options (multiple select);
255+
non-key tokens pass through unchanged as free-text answers.
256+
"""
257+
if not options or not keys or not answer.strip():
258+
return answer
259+
resolved: list[str] = []
260+
for part in answer.split(","):
261+
part = part.strip()
262+
if part.lower() in keys:
263+
resolved.append(options[keys.index(part.lower())])
264+
continue
265+
resolved.append(part)
266+
return ", ".join(resolved)
267+
268+
246269
def _resolve_numbered_choice(answer: str, options: list[str]) -> str:
247270
"""Map bare numbers in ANSWER (1-based) to the matching option label.
248271
@@ -329,8 +352,20 @@ def _on_log(self, msg: str) -> None:
329352
self.status = f" {msg[:60]}"
330353

331354
def _ui_confirm(self, prompt: str) -> bool:
332-
q = UiQuestion(prompt)
333-
return self._ask_sync(q) in ("y", "yes", "true", "1", "a")
355+
"""PlanExit confirmation: same look as the Question tool, but a
356+
y/n keyed choice list instead of numbers (two choices only)."""
357+
q = UiQuestion(
358+
prompt,
359+
options=list(config.PLAN_EXIT_OPTIONS),
360+
keys=["y", "n"],
361+
custom=False,
362+
)
363+
answer = self._ask_sync(q).strip().lower()
364+
# resolved answers arrive as the option label; legacy free-text
365+
# (y/yes/a/1/true) keeps working for muscle memory
366+
return answer == config.PLAN_EXIT_OPTIONS[0].lower() or answer in (
367+
"y", "yes", "a", "true", "1",
368+
)
334369

335370
def _ui_ask(self, questions: list[dict]) -> str:
336371
lines = []
@@ -608,7 +643,24 @@ def _ask_question_blocking(self) -> None:
608643
self.console.print()
609644
self._flush()
610645
options = q.options or []
611-
if options and any(len(o) > 1 for o in options):
646+
keys = q.keys or []
647+
if keys and options and len(keys) == len(options):
648+
# keyed choices (e.g. y/n confirm): type the key to pick —
649+
# same list look as the Question tool, keys instead of numbers
650+
self.console.print(Text(q.prompt))
651+
for key, opt in zip(keys, options):
652+
line = Text(f" {key}) ", style="cyan")
653+
line.append(opt)
654+
self.console.print(line)
655+
if q.multiple:
656+
hint = "Enter keys, comma-separated"
657+
else:
658+
hint = "Enter a key"
659+
if q.custom:
660+
hint += ", or type your own answer"
661+
self.console.print(f"[dim]{hint}[/dim]")
662+
prompt = "> "
663+
elif options and any(len(o) > 1 for o in options):
612664
# long option labels get a numbered list: type the number to pick
613665
self.console.print(Text(q.prompt))
614666
for i, opt in enumerate(options, 1):
@@ -632,7 +684,10 @@ def _ask_question_blocking(self) -> None:
632684
answer = self.prompt_session.prompt(prompt, multiline=False)
633685
except (EOFError, KeyboardInterrupt):
634686
answer = ""
635-
q.answer = _resolve_numbered_choice(answer, options)
687+
if keys:
688+
q.answer = _resolve_keyed_choice(answer, options, keys)
689+
else:
690+
q.answer = _resolve_numbered_choice(answer, options)
636691
q.event.set()
637692
self.question = None
638693
self._data_event.set() # re-render promptly after the answer

tests/test_planmode.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,5 +65,63 @@ def test_plan_reminder(self):
6565
self.assertIn(pm.plan_file, reminder)
6666

6767

68+
class TestPlanExitConfirm(unittest.TestCase):
69+
"""plan_exit goes through the session confirm hook (a y/n choice UI
70+
in the TUI) — not through the Question tool's ask_questions."""
71+
72+
class FakeClient:
73+
def chat(self, *a, **k):
74+
return None
75+
76+
def chat_sync(self, *a, **k):
77+
return None
78+
79+
def close(self):
80+
pass
81+
82+
def make_session(self):
83+
from python_agent_harness.agent_session import AgentSession
84+
from python_agent_harness.tools import default_registry
85+
86+
s = AgentSession(
87+
project_dir="/tmp/proj", client=self.FakeClient(), model="m",
88+
registry=default_registry(),
89+
)
90+
s.switch_to_plan()
91+
return s
92+
93+
def test_plan_exit_uses_confirm_hook_not_ask_questions(self):
94+
s = self.make_session()
95+
seen = {}
96+
s.confirm_fn = lambda prompt: seen.setdefault("prompt", prompt) or True
97+
asked = []
98+
s.ask_fn = lambda questions: asked.append(questions) or "Unanswered"
99+
result = s.plan_exit()
100+
self.assertIn("approved", result)
101+
self.assertFalse(s.plan_mode.is_plan)
102+
self.assertIn("Plan at", seen["prompt"])
103+
self.assertIn("Switch to build agent", seen["prompt"])
104+
# the Question path must NOT be used for the plan approval
105+
self.assertEqual(asked, [])
106+
s.close()
107+
108+
def test_plan_exit_rejected_stays_in_plan(self):
109+
s = self.make_session()
110+
s.confirm_fn = lambda prompt: False
111+
result = s.plan_exit()
112+
self.assertIn("rejected", result)
113+
self.assertTrue(s.plan_mode.is_plan)
114+
s.close()
115+
116+
def test_plan_exit_noop_outside_plan(self):
117+
s = self.make_session()
118+
s.switch_to_build()
119+
s.confirm_fn = lambda prompt: True
120+
result = s.plan_exit()
121+
self.assertIn("Not in plan mode", result)
122+
self.assertFalse(s.plan_mode.is_plan)
123+
s.close()
124+
125+
68126
if __name__ == "__main__":
69127
unittest.main()

tests/test_subagent_isolation.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,13 @@ def __init__(self, script):
2020
self.n = 0
2121
self.streamed = [] # turn indices that streamed
2222
self.sent = []
23+
self.sent_tools = [] # tool names sent per chat call
2324

2425
def chat(self, messages, tools=None, system=None, temperature=None,
2526
max_tokens=None, reasoning_effort=None, on_delta=None):
2627
self.n += 1
2728
self.sent.append([m.to_api() for m in messages])
29+
self.sent_tools.append([t.name for t in tools] if tools else None)
2830
if on_delta:
2931
on_delta(f"stream-{self.n} ")
3032
self.streamed.append(self.n)
@@ -164,6 +166,62 @@ def test_subagent_does_not_touch_shared_context_accounting(self):
164166
self.assertEqual(s.calibrator.factor, 1.0)
165167
self.assertIsNone(s.calibrator.last_raw_estimate)
166168

169+
def test_subagent_does_not_get_parent_only_specs(self):
170+
"""Parent-only tools (Agent, Question, PlanExit) are excluded
171+
from the sub-agent's request specs, while the parent keeps them."""
172+
from python_agent_harness.tools import PlanExit
173+
174+
client = RecClient([
175+
("", [ToolCall(id="p1", name="Agent", arguments=AGENT_CALL)]),
176+
"sub done",
177+
"parent done",
178+
])
179+
s = make_session(client)
180+
s.registry.register(PlanExit()) # plan mode registers it too
181+
run_agent_loop(
182+
s, messages=[Message(role="user", content="delegate")], top_level=True
183+
)
184+
# turn 1 = parent's Agent call, turn 2 = the sub-agent, turn 3 = parent
185+
parent_tools, sub_tools = client.sent_tools[0], client.sent_tools[1]
186+
self.assertIn("Agent", parent_tools)
187+
self.assertIn("Question", parent_tools)
188+
self.assertIn("PlanExit", parent_tools)
189+
self.assertNotIn("Agent", sub_tools)
190+
self.assertNotIn("Question", sub_tools)
191+
self.assertNotIn("PlanExit", sub_tools)
192+
# the sub-agent keeps its working tools
193+
self.assertIn("Read", sub_tools)
194+
self.assertIn("Bash", sub_tools)
195+
196+
def test_subagent_parent_only_call_refused_at_execution(self):
197+
"""Defense in depth: even a hallucinated parent-only call (Agent,
198+
Question, PlanExit) from a sub-agent must be refused at execution
199+
time, not silently run."""
200+
from python_agent_harness.agent import AgentLoop
201+
from python_agent_harness.tools import PlanExit
202+
203+
client = RecClient([
204+
("", [ToolCall(id="s1", name="Agent", arguments=AGENT_CALL)]),
205+
"sub done",
206+
])
207+
s = make_session(client)
208+
s.registry.register(PlanExit())
209+
loop = AgentLoop(
210+
s,
211+
messages=[Message(role="user", content="find stuff")],
212+
top_level=False,
213+
system="SUB",
214+
)
215+
loop.run()
216+
tool_rows = [m for m in loop.messages if m.role == "tool"]
217+
self.assertTrue(tool_rows)
218+
self.assertIn("not available to sub-agents", tool_rows[0].text())
219+
# the refused call never executed the Agent tool, so no nested
220+
# sub-agent loop ran: exactly 2 chat calls (refused call turn +
221+
# the terminal "sub done" reply) — a real delegation would have
222+
# spawned an additional nested loop's chat call
223+
self.assertEqual(len(client.sent), 2)
224+
167225

168226
if __name__ == "__main__":
169227
unittest.main()

0 commit comments

Comments
 (0)