mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-09 21:49:37 +00:00
fix(clarification): drop sibling tool calls before interrupt (#4908)
* fix(clarification): drop sibling tool calls before interrupt - Rewrite the AIMessage in ClarificationMiddleware.after_model so a parallel bash/write_file cannot run before the user answers - langchain return_direct only inspects the last ToolMessage; siblings both execute and can keep the agent loop alive - Skip the rewrite when disable_clarification is set - Prompt and tool docs: do not call other tools in the same turn Fixes #4906 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(clarification): enhance sibling tool call handling in ClarificationMiddleware - Update ClarificationMiddleware to ensure sibling tool calls are dropped when `ask_clarification` is invoked, preventing unintended execution before user input. - Modify documentation to clarify that the `return_direct` router now inspects all client-side tool calls of the last AIMessage, ensuring proper routing behavior. - Introduce a new integration test to validate that sibling tools do not execute when `ask_clarification` is present in the same turn. This change addresses potential issues with tool execution order and improves the overall reliability of the middleware. Fixes #4906 * fix(clarification): enhance tool call filtering in ClarificationMiddleware - Update _filter_content_tool_use to handle Gemini-style function_call blocks by matching on name when no id is present, ensuring proper filtering of tool calls. - Modify ClarificationMiddleware to maintain sibling tool call integrity by dropping unnecessary blocks, improving the clarity of the AIMessage content. - Add a new test to validate the correct stripping of idless function call content blocks, ensuring that sibling tool calls do not execute prematurely. This change improves the robustness of the middleware and addresses potential execution order issues. Fixes #4906 * fix(clarification): drop siblings when ask_clarification is malformed LangChain parks invalid args on invalid_tool_calls independently, so a valid sibling would otherwise still execute before the user answers. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
simon
GitHub
parent
8d15b87d8b
commit
ea9b70148e
@ -548,6 +548,7 @@ data — do NOT reveal it.
|
||||
- ❌ DO NOT skip clarification for "efficiency" - accuracy matters more than speed
|
||||
- ❌ DO NOT make assumptions when information is missing - ALWAYS ask
|
||||
- ❌ DO NOT proceed with guesses - STOP and call ask_clarification first
|
||||
- ❌ DO NOT call any other tool in the same turn as ask_clarification — sibling calls are dropped
|
||||
- ✅ Analyze the request in thinking → Identify unclear aspects → Ask BEFORE any action
|
||||
- ✅ If you identify the need for clarification in your thinking, you MUST call the tool IMMEDIATELY
|
||||
- ✅ After calling ask_clarification, execution will be interrupted automatically
|
||||
|
||||
@ -92,4 +92,4 @@ Before changing a later authorization phase, read the [authorization RFC](../../
|
||||
32. **TerminalResponseMiddleware** - When a provider returns an empty terminal `AIMessage` after tool execution, injects a hidden recovery prompt and retries the model once; a second empty response is replaced in checkpoint state by a visible error fallback marked for the run worker, so the run finishes as an error instead of a silent success
|
||||
33. **ModelLengthFinishReasonMiddleware** - Records `stop_reason=model_length_capped` when provider-specific length detectors match a terminal `AIMessage` without tool-call intent (`finish_reason=length` / `MAX_TOKENS`, or `stop_reason=max_tokens`), preserving the original assistant content and never reparsing textual tool-call-like envelopes
|
||||
34. **SafetyFinishReasonMiddleware** - *(optional, if `safety_finish_reason.enabled`)* Suppresses tool execution when the provider safety-terminated the response (e.g. `finish_reason=content_filter`); registered after terminal-response/custom/configured middlewares so LangChain's reverse-order `after_model` dispatch runs it first
|
||||
35. **ClarificationMiddleware** - Intercepts `ask_clarification` tool calls, writes a readable `ToolMessage.content` fallback plus structured `ToolMessage.artifact.human_input` request payload, and interrupts via `Command(goto=END)` (must be last). Payloads are versioned: legacy modes (`free_text` / `choice_with_other`) keep `version: 1` unchanged, while the v2 `form` mode (from `fields`) carries `version: 2` so older frontends reject the payload and degrade to the plain-text fallback. Field normalization is deterministic and lives in the middleware, not the tool schema — the middleware short-circuits before tool execution, so tool-arg typing alone provides no runtime validation. Validation is atomic: any structurally broken entry (non-dict, bad/duplicate name, a name colliding with a JS `Object.prototype` member like `__proto__`/`constructor`, exceeding the caps of 16 fields / 24 options per field / 200 chars per text, or the whole normalized definition exceeding `MAX_FORM_SERIALIZED_BYTES` = 16KB UTF-8 — the per-item caps alone admit forms whose IM text fallback would blow channel delivery limits and truncate away trailing fields) degrades the whole form to the legacy option/free-text modes, so a card can never render "complete" while silently missing a business field; benign issues keep local degradation (unknown types — including unhashable JSON like `type: []`, which must never raise from the membership probe — and option-less selects become `text`), and options are trimmed/deduped with blanks dropped (both form-level and top-level) because the frontend parser rejects blank option labels. Model-produced XML-to-dict option payloads are recursively flattened from dict/list containers in source order, scalar string/number leaves are retained, and residual XML tags are removed before the same trimming and deduplication. Checkbox fields are booleans that default to an explicit "no"; `required` on a checkbox means must-agree/consent semantics. The response protocol is deliberately unchanged (v1 `text`/`option` only): form cards submit a readable text summary as `response_kind: "text"`, so journal persistence and answered-card recovery need no new allowlist entries. Because this middleware can short-circuit tool execution before LangChain emits `on_tool_end`, `RunJournal` performs a root-run final reconciliation for allowlisted clarification `ToolMessage`s whose `tool_call_id` was produced by the current run, so human-input request cards remain recoverable from `run_events` after checkpoint compaction. Human Input Card replies are submitted as `hide_from_ui` `HumanMessage`s with `additional_kwargs.human_input_response`; `RunJournal` persists only allowlisted hidden response sources (currently `ask_clarification`) as `llm.human.input`, which preserves answered-card state after compaction without exposing generic internal hidden context.
|
||||
35. **ClarificationMiddleware** - Intercepts `ask_clarification` tool calls, writes a readable `ToolMessage.content` fallback plus structured `ToolMessage.artifact.human_input` request payload, and interrupts via `Command(goto=END)` (must be last). In `after_model` it drops sibling tool calls from the same turn so they cannot execute before the user answers; a malformed `ask_clarification` that LangChain parked on `invalid_tool_calls` is the same stop signal (the valid sibling would otherwise still run). `disable_clarification` runs keep the siblings. Payloads are versioned: legacy modes (`free_text` / `choice_with_other`) keep `version: 1` unchanged, while the v2 `form` mode (from `fields`) carries `version: 2` so older frontends reject the payload and degrade to the plain-text fallback. Field normalization is deterministic and lives in the middleware, not the tool schema — the middleware short-circuits before tool execution, so tool-arg typing alone provides no runtime validation. Validation is atomic: any structurally broken entry (non-dict, bad/duplicate name, a name colliding with a JS `Object.prototype` member like `__proto__`/`constructor`, exceeding the caps of 16 fields / 24 options per field / 200 chars per text, or the whole normalized definition exceeding `MAX_FORM_SERIALIZED_BYTES` = 16KB UTF-8 — the per-item caps alone admit forms whose IM text fallback would blow channel delivery limits and truncate away trailing fields) degrades the whole form to the legacy option/free-text modes, so a card can never render "complete" while silently missing a business field; benign issues keep local degradation (unknown types — including unhashable JSON like `type: []`, which must never raise from the membership probe — and option-less selects become `text`), and options are trimmed/deduped with blanks dropped (both form-level and top-level) because the frontend parser rejects blank option labels. Model-produced XML-to-dict option payloads are recursively flattened from dict/list containers in source order, scalar string/number leaves are retained, and residual XML tags are removed before the same trimming and deduplication. Checkbox fields are booleans that default to an explicit "no"; `required` on a checkbox means must-agree/consent semantics. The response protocol is deliberately unchanged (v1 `text`/`option` only): form cards submit a readable text summary as `response_kind: "text"`, so journal persistence and answered-card recovery need no new allowlist entries. Because this middleware can short-circuit tool execution before LangChain emits `on_tool_end`, `RunJournal` performs a root-run final reconciliation for allowlisted clarification `ToolMessage`s whose `tool_call_id` was produced by the current run, so human-input request cards remain recoverable from `run_events` after checkpoint compaction. Human Input Card replies are submitted as `hide_from_ui` `HumanMessage`s with `additional_kwargs.human_input_response`; `RunJournal` persists only allowlisted hidden response sources (currently `ask_clarification`) as `llm.human.input`, which preserves answered-card state after compaction without exposing generic internal hidden context.
|
||||
|
||||
@ -9,13 +9,18 @@ from typing import Any, override
|
||||
|
||||
from langchain.agents import AgentState
|
||||
from langchain.agents.middleware import AgentMiddleware
|
||||
from langchain_core.messages import ToolMessage
|
||||
from langchain_core.messages import AIMessage, ToolMessage
|
||||
from langgraph.graph import END
|
||||
from langgraph.prebuilt.tool_node import ToolCallRequest
|
||||
from langgraph.runtime import Runtime
|
||||
from langgraph.types import Command
|
||||
|
||||
from deerflow.agents.middlewares.tool_call_metadata import clone_ai_message_with_tool_calls
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
ASK_CLARIFICATION_TOOL_NAME = "ask_clarification"
|
||||
|
||||
# Whitelisted form field types; anything else degrades to "text" so a bad
|
||||
# model-provided type can never produce an unrenderable card.
|
||||
FORM_FIELD_TYPES = frozenset({"text", "textarea", "number", "select", "multi_select", "checkbox", "date"})
|
||||
@ -66,15 +71,48 @@ class ClarificationMiddlewareState(AgentState):
|
||||
pass
|
||||
|
||||
|
||||
def _filter_content_tool_use(content: Any, kept_ids: set[str], kept_names: set[str]) -> Any:
|
||||
"""Drop provider tool-use blocks that were stripped from ``tool_calls``.
|
||||
|
||||
Anthropic ``tool_use`` blocks carry an ``id`` that matches ``tool_calls``.
|
||||
Gemini-style ``function_call`` blocks often have no ``id`` (langchain
|
||||
synthesizes ids onto ``tool_calls`` only), so those are matched by ``name``.
|
||||
"""
|
||||
if not isinstance(content, list):
|
||||
return content
|
||||
filtered: list[Any] = []
|
||||
for block in content:
|
||||
if isinstance(block, dict) and block.get("type") in {"tool_use", "function_call"}:
|
||||
block_id = block.get("id")
|
||||
if isinstance(block_id, str) and block_id:
|
||||
if block_id not in kept_ids:
|
||||
continue
|
||||
elif block.get("type") == "function_call":
|
||||
name = block.get("name")
|
||||
if not isinstance(name, str) or name not in kept_names:
|
||||
continue
|
||||
filtered.append(block)
|
||||
return filtered
|
||||
|
||||
|
||||
class ClarificationMiddleware(AgentMiddleware[ClarificationMiddlewareState]):
|
||||
"""Intercepts clarification tool calls and interrupts execution to present questions to the user.
|
||||
|
||||
When the model calls the `ask_clarification` tool, this middleware:
|
||||
1. Intercepts the tool call before execution
|
||||
2. Extracts the clarification question and metadata
|
||||
3. Formats a user-friendly message
|
||||
4. Returns a Command that interrupts execution and presents the question
|
||||
5. Waits for user response before continuing
|
||||
1. Drops any sibling tool calls from the same AIMessage (``after_model``)
|
||||
so they cannot execute before the user answers. langchain's
|
||||
``return_direct`` router inspects all client-side tool calls of the
|
||||
last AIMessage and routes to END only when every one is
|
||||
``return_direct``. A mixed ``[ask_clarification, bash]`` batch would
|
||||
both run the siblings *and* loop back to the model. Malformed
|
||||
``ask_clarification`` arguments land in ``invalid_tool_calls`` while a
|
||||
valid sibling stays in ``tool_calls``; that still counts as a stop
|
||||
signal and the siblings are dropped.
|
||||
2. Intercepts the remaining ``ask_clarification`` call before execution
|
||||
3. Extracts the clarification question and metadata
|
||||
4. Formats a user-friendly message
|
||||
5. Returns a Command that interrupts execution and presents the question
|
||||
6. Waits for user response before continuing
|
||||
|
||||
This replaces the tool-based approach where clarification continued the conversation flow.
|
||||
"""
|
||||
@ -360,22 +398,83 @@ class ClarificationMiddleware(AgentMiddleware[ClarificationMiddlewareState]):
|
||||
|
||||
return "\n".join(message_parts)
|
||||
|
||||
def _is_disabled(self, request: ToolCallRequest) -> bool:
|
||||
def _clarification_disabled(self, runtime: Any) -> bool:
|
||||
"""Whether clarifications are suppressed for this run.
|
||||
|
||||
Non-interactive channels (e.g. GitHub webhooks) set
|
||||
``disable_clarification`` in the run context because a clarification
|
||||
would dead-end the run — the human only "replies" via a later
|
||||
webhook delivery, by which point the agent's turn is long over.
|
||||
When set, we don't interrupt; we return a ToolMessage nudging the
|
||||
agent to proceed with its best judgment instead.
|
||||
"""
|
||||
runtime = getattr(request, "runtime", None)
|
||||
context = getattr(runtime, "context", None)
|
||||
if not context:
|
||||
return False
|
||||
return bool(context.get("disable_clarification"))
|
||||
|
||||
def _is_disabled(self, request: ToolCallRequest) -> bool:
|
||||
"""Whether clarifications are suppressed for this tool-call request."""
|
||||
return self._clarification_disabled(getattr(request, "runtime", None))
|
||||
|
||||
def _drop_parallel_non_clarification_tools(self, state: AgentState, runtime: Runtime) -> dict | None:
|
||||
"""Keep only ``ask_clarification`` when it was emitted alongside other tools.
|
||||
|
||||
Providers routinely batch tool calls. If ``ask_clarification`` shares a
|
||||
turn with ``bash`` / ``write_file`` / ..., those siblings execute before
|
||||
the user answers, and langchain's ``return_direct`` check (every
|
||||
client-side tool call of the last AIMessage must be ``return_direct``)
|
||||
routes back to the model. Rewrite the AIMessage so the tools node
|
||||
never sees the siblings.
|
||||
|
||||
LangChain parses each provider call independently, so a malformed
|
||||
``ask_clarification`` is stored on ``invalid_tool_calls`` while a
|
||||
valid sibling remains executable on ``tool_calls``. Treat that
|
||||
malformed call as the same stop signal: drop the siblings so the
|
||||
tools node cannot run them. With no remaining ``tool_calls``,
|
||||
``create_agent`` routes to END.
|
||||
|
||||
``disable_clarification`` skips this rewrite: those runs must keep the
|
||||
sibling actions, because the clarification itself is turned into a
|
||||
"proceed" ToolMessage instead of an interrupt.
|
||||
"""
|
||||
if self._clarification_disabled(runtime):
|
||||
return None
|
||||
|
||||
messages = state.get("messages", [])
|
||||
if not messages:
|
||||
return None
|
||||
last = messages[-1]
|
||||
if not isinstance(last, AIMessage):
|
||||
return None
|
||||
|
||||
tool_calls = list(last.tool_calls or [])
|
||||
invalid_tool_calls = [tc for tc in (getattr(last, "invalid_tool_calls", None) or []) if isinstance(tc, dict)]
|
||||
clarification_calls = [tc for tc in tool_calls if tc.get("name") == ASK_CLARIFICATION_TOOL_NAME]
|
||||
invalid_clarification_calls = [tc for tc in invalid_tool_calls if tc.get("name") == ASK_CLARIFICATION_TOOL_NAME]
|
||||
if not clarification_calls and not invalid_clarification_calls:
|
||||
return None
|
||||
|
||||
sibling_calls = [tc for tc in tool_calls if tc.get("name") != ASK_CLARIFICATION_TOOL_NAME]
|
||||
if not sibling_calls:
|
||||
return None
|
||||
|
||||
dropped_names = [str(tc.get("name") or "unknown") for tc in sibling_calls]
|
||||
logger.warning(
|
||||
"ask_clarification was emitted with %d sibling tool call(s); dropping %s so the turn can interrupt",
|
||||
len(dropped_names),
|
||||
dropped_names,
|
||||
)
|
||||
|
||||
kept_for_content = clarification_calls + invalid_clarification_calls
|
||||
kept_ids = {tc["id"] for tc in kept_for_content if isinstance(tc.get("id"), str) and tc["id"]}
|
||||
kept_names = {str(tc["name"]) for tc in kept_for_content if isinstance(tc.get("name"), str) and tc["name"]}
|
||||
new_content = _filter_content_tool_use(last.content, kept_ids, kept_names)
|
||||
patched = clone_ai_message_with_tool_calls(
|
||||
last,
|
||||
clarification_calls,
|
||||
content=new_content if new_content is not last.content else None,
|
||||
)
|
||||
return {"messages": [patched]}
|
||||
|
||||
def _handle_disabled_clarification(self, request: ToolCallRequest) -> ToolMessage:
|
||||
"""Suppress a clarification and tell the agent to proceed.
|
||||
|
||||
@ -395,7 +494,7 @@ class ClarificationMiddleware(AgentMiddleware[ClarificationMiddlewareState]):
|
||||
"you made in your final response."
|
||||
),
|
||||
tool_call_id=tool_call_id,
|
||||
name="ask_clarification",
|
||||
name=ASK_CLARIFICATION_TOOL_NAME,
|
||||
)
|
||||
|
||||
def _handle_clarification(self, request: ToolCallRequest) -> Command:
|
||||
@ -433,7 +532,7 @@ class ClarificationMiddleware(AgentMiddleware[ClarificationMiddlewareState]):
|
||||
id=request_id,
|
||||
content=formatted_message,
|
||||
tool_call_id=tool_call_id,
|
||||
name="ask_clarification",
|
||||
name=ASK_CLARIFICATION_TOOL_NAME,
|
||||
artifact={"human_input": human_input_payload},
|
||||
)
|
||||
|
||||
@ -463,7 +562,7 @@ class ClarificationMiddleware(AgentMiddleware[ClarificationMiddlewareState]):
|
||||
Command that interrupts execution with the formatted clarification message
|
||||
"""
|
||||
# Check if this is an ask_clarification tool call
|
||||
if request.tool_call.get("name") != "ask_clarification":
|
||||
if request.tool_call.get("name") != ASK_CLARIFICATION_TOOL_NAME:
|
||||
# Not a clarification call, execute normally
|
||||
return handler(request)
|
||||
|
||||
@ -488,7 +587,7 @@ class ClarificationMiddleware(AgentMiddleware[ClarificationMiddlewareState]):
|
||||
Command that interrupts execution with the formatted clarification message
|
||||
"""
|
||||
# Check if this is an ask_clarification tool call
|
||||
if request.tool_call.get("name") != "ask_clarification":
|
||||
if request.tool_call.get("name") != ASK_CLARIFICATION_TOOL_NAME:
|
||||
# Not a clarification call, execute normally
|
||||
return await handler(request)
|
||||
|
||||
@ -496,3 +595,11 @@ class ClarificationMiddleware(AgentMiddleware[ClarificationMiddlewareState]):
|
||||
return self._handle_disabled_clarification(request)
|
||||
|
||||
return self._handle_clarification(request)
|
||||
|
||||
@override
|
||||
def after_model(self, state: AgentState, runtime: Runtime) -> dict | None:
|
||||
return self._drop_parallel_non_clarification_tools(state, runtime)
|
||||
|
||||
@override
|
||||
async def aafter_model(self, state: AgentState, runtime: Runtime) -> dict | None:
|
||||
return self._drop_parallel_non_clarification_tools(state, runtime)
|
||||
|
||||
@ -70,6 +70,8 @@ def ask_clarification_tool(
|
||||
- If a skill provides a predefined field template, pass it through `fields`
|
||||
unchanged instead of redesigning it
|
||||
- After calling this tool, execution will be interrupted automatically
|
||||
- Do not call any other tool in the same turn as this one; sibling tool
|
||||
calls are dropped so they cannot run before the user answers
|
||||
|
||||
Args:
|
||||
question: The clarification question to ask the user. Be specific and clear.
|
||||
|
||||
@ -0,0 +1,186 @@
|
||||
"""Factory-level wiring test for ClarificationMiddleware sibling-tool dropping.
|
||||
|
||||
Unit tests in ``test_clarification_middleware.py`` call ``after_model``
|
||||
directly. This file builds a real ``langchain.agents.create_agent`` graph
|
||||
so a langchain hook-dispatch regression or a same-id ``add_messages``
|
||||
replacement failure would reintroduce #4906 instead of staying green.
|
||||
A second graph path covers malformed ``ask_clarification`` parked on
|
||||
``invalid_tool_calls`` beside a valid sibling.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from langchain.agents import create_agent
|
||||
from langchain_core.language_models import BaseChatModel
|
||||
from langchain_core.messages import AIMessage, HumanMessage, ToolMessage
|
||||
from langchain_core.outputs import ChatGeneration, ChatResult
|
||||
from langchain_core.tools import tool
|
||||
|
||||
from deerflow.agents.middlewares.clarification_middleware import ClarificationMiddleware
|
||||
from deerflow.tools.builtins.clarification_tool import ask_clarification_tool
|
||||
|
||||
_BASH_INVOCATIONS: list[str] = []
|
||||
_MIXED_MESSAGE_ID = "ai-clarification-with-sibling"
|
||||
_INVALID_MIXED_MESSAGE_ID = "ai-invalid-clarification-with-sibling"
|
||||
|
||||
|
||||
@tool
|
||||
def bash(command: str) -> str:
|
||||
"""Pretend to run a shell command. Records the call for assertion."""
|
||||
_BASH_INVOCATIONS.append(command)
|
||||
return f"ran: {command}"
|
||||
|
||||
|
||||
class _MixedBatchModel(BaseChatModel):
|
||||
"""First call emits ``ask_clarification`` plus ``bash``; a second call is a wiring failure."""
|
||||
|
||||
call_count: int = 0
|
||||
|
||||
@property
|
||||
def _llm_type(self) -> str:
|
||||
return "fake-clarification-mixed-batch"
|
||||
|
||||
def bind_tools(self, tools, **kwargs):
|
||||
return self
|
||||
|
||||
def _generate(self, messages, stop=None, run_manager=None, **kwargs):
|
||||
self.call_count += 1
|
||||
if self.call_count == 1:
|
||||
message = AIMessage(
|
||||
id=_MIXED_MESSAGE_ID,
|
||||
content="",
|
||||
tool_calls=[
|
||||
{
|
||||
"id": "call_clarify_1",
|
||||
"name": "ask_clarification",
|
||||
"args": {
|
||||
"question": "Which directory should I use?",
|
||||
"clarification_type": "missing_info",
|
||||
},
|
||||
},
|
||||
{
|
||||
"id": "call_bash_1",
|
||||
"name": "bash",
|
||||
"args": {"command": "rm -rf /tmp/foo"},
|
||||
},
|
||||
],
|
||||
)
|
||||
else:
|
||||
message = AIMessage(content="should-not-happen")
|
||||
return ChatResult(generations=[ChatGeneration(message=message)])
|
||||
|
||||
async def _agenerate(self, messages, stop=None, run_manager=None, **kwargs):
|
||||
return self._generate(messages, stop=stop, run_manager=run_manager, **kwargs)
|
||||
|
||||
|
||||
class _InvalidClarificationMixedBatchModel(BaseChatModel):
|
||||
"""First call emits a malformed ask_clarification plus a valid bash sibling."""
|
||||
|
||||
call_count: int = 0
|
||||
|
||||
@property
|
||||
def _llm_type(self) -> str:
|
||||
return "fake-invalid-clarification-mixed-batch"
|
||||
|
||||
def bind_tools(self, tools, **kwargs):
|
||||
return self
|
||||
|
||||
def _generate(self, messages, stop=None, run_manager=None, **kwargs):
|
||||
self.call_count += 1
|
||||
if self.call_count == 1:
|
||||
message = AIMessage(
|
||||
id=_INVALID_MIXED_MESSAGE_ID,
|
||||
content="",
|
||||
tool_calls=[
|
||||
{
|
||||
"id": "call_bash_1",
|
||||
"name": "bash",
|
||||
"args": {"command": "rm -rf /tmp/foo"},
|
||||
},
|
||||
],
|
||||
invalid_tool_calls=[
|
||||
{
|
||||
"id": "call_clarify_1",
|
||||
"name": "ask_clarification",
|
||||
"args": "{",
|
||||
"error": "Failed to parse tool arguments",
|
||||
"type": "invalid_tool_call",
|
||||
},
|
||||
],
|
||||
)
|
||||
else:
|
||||
message = AIMessage(content="should-not-happen")
|
||||
return ChatResult(generations=[ChatGeneration(message=message)])
|
||||
|
||||
async def _agenerate(self, messages, stop=None, run_manager=None, **kwargs):
|
||||
return self._generate(messages, stop=stop, run_manager=run_manager, **kwargs)
|
||||
|
||||
|
||||
def test_mixed_clarification_batch_does_not_execute_siblings_or_loop():
|
||||
"""after_model must be dispatched and replace the AIMessage in place.
|
||||
|
||||
(a) the bash handler never runs
|
||||
(b) the run ends without a second model call
|
||||
"""
|
||||
_BASH_INVOCATIONS.clear()
|
||||
model = _MixedBatchModel()
|
||||
agent = create_agent(
|
||||
model=model,
|
||||
tools=[ask_clarification_tool, bash],
|
||||
middleware=[ClarificationMiddleware()],
|
||||
)
|
||||
|
||||
result = agent.invoke(
|
||||
{"messages": [HumanMessage(content="clean up the temp dir")]},
|
||||
config={"recursion_limit": 8},
|
||||
)
|
||||
|
||||
assert _BASH_INVOCATIONS == [], f"bash ran before the user answered: {_BASH_INVOCATIONS}"
|
||||
assert model.call_count == 1
|
||||
|
||||
ai_messages = [message for message in result["messages"] if isinstance(message, AIMessage)]
|
||||
assert len(ai_messages) == 1
|
||||
patched = ai_messages[0]
|
||||
assert patched.id == _MIXED_MESSAGE_ID
|
||||
assert [tc["name"] for tc in patched.tool_calls] == ["ask_clarification"]
|
||||
|
||||
tool_messages = [message for message in result["messages"] if isinstance(message, ToolMessage)]
|
||||
assert [message.name for message in tool_messages] == ["ask_clarification"]
|
||||
assert tool_messages[0].tool_call_id == "call_clarify_1"
|
||||
|
||||
|
||||
def test_mixed_invalid_clarification_batch_does_not_execute_siblings_or_loop():
|
||||
"""Malformed ask_clarification is still a stop signal for executable siblings.
|
||||
|
||||
LangChain parks the broken call on ``invalid_tool_calls`` while the valid
|
||||
bash sibling stays on ``tool_calls``. after_model must still rewrite the
|
||||
AIMessage so:
|
||||
(a) the bash handler never runs
|
||||
(b) the run ends without a second model call
|
||||
"""
|
||||
_BASH_INVOCATIONS.clear()
|
||||
model = _InvalidClarificationMixedBatchModel()
|
||||
agent = create_agent(
|
||||
model=model,
|
||||
tools=[ask_clarification_tool, bash],
|
||||
middleware=[ClarificationMiddleware()],
|
||||
)
|
||||
|
||||
result = agent.invoke(
|
||||
{"messages": [HumanMessage(content="clean up the temp dir")]},
|
||||
config={"recursion_limit": 8},
|
||||
)
|
||||
|
||||
assert _BASH_INVOCATIONS == [], f"bash ran before the user answered: {_BASH_INVOCATIONS}"
|
||||
assert model.call_count == 1
|
||||
|
||||
ai_messages = [message for message in result["messages"] if isinstance(message, AIMessage)]
|
||||
assert len(ai_messages) == 1
|
||||
patched = ai_messages[0]
|
||||
assert patched.id == _INVALID_MIXED_MESSAGE_ID
|
||||
assert patched.tool_calls == []
|
||||
assert [tc["name"] for tc in patched.invalid_tool_calls] == ["ask_clarification"]
|
||||
assert patched.invalid_tool_calls[0]["id"] == "call_clarify_1"
|
||||
|
||||
tool_messages = [message for message in result["messages"] if isinstance(message, ToolMessage)]
|
||||
assert tool_messages == []
|
||||
@ -800,3 +800,253 @@ class TestClarificationDisabled:
|
||||
merged = add_messages(add_messages([], [first_message]), [second_message])
|
||||
|
||||
assert len(merged) == 1
|
||||
|
||||
|
||||
class TestDropParallelSiblingTools:
|
||||
"""after_model must drop sibling tools when ask_clarification is in the same turn.
|
||||
|
||||
langchain's return_direct router inspects all client-side tool calls of the
|
||||
last AIMessage and routes to END only when every one is return_direct, so a
|
||||
parallel bash/write_file would both execute and keep the agent loop alive.
|
||||
"""
|
||||
|
||||
def _runtime(self, **context):
|
||||
return SimpleNamespace(context=context)
|
||||
|
||||
def _ai(self, tool_calls, content="", invalid_tool_calls=None):
|
||||
from langchain_core.messages import AIMessage
|
||||
|
||||
kwargs = {"content": content, "tool_calls": tool_calls}
|
||||
if invalid_tool_calls is not None:
|
||||
kwargs["invalid_tool_calls"] = invalid_tool_calls
|
||||
return AIMessage(**kwargs)
|
||||
|
||||
def test_drops_siblings_when_clarification_is_first(self, middleware):
|
||||
msg = self._ai(
|
||||
[
|
||||
{"id": "c1", "name": "ask_clarification", "args": {"question": "Which dir?"}},
|
||||
{"id": "b1", "name": "bash", "args": {"command": "rm -rf /tmp/foo"}},
|
||||
]
|
||||
)
|
||||
update = middleware.after_model({"messages": [msg]}, self._runtime())
|
||||
assert update is not None
|
||||
patched = update["messages"][0]
|
||||
assert [tc["name"] for tc in patched.tool_calls] == ["ask_clarification"]
|
||||
assert patched.id == msg.id
|
||||
|
||||
def test_drops_siblings_when_clarification_is_last(self, middleware):
|
||||
msg = self._ai(
|
||||
[
|
||||
{"id": "b1", "name": "bash", "args": {"command": "echo hi"}},
|
||||
{"id": "c1", "name": "ask_clarification", "args": {"question": "q?"}},
|
||||
]
|
||||
)
|
||||
update = middleware.after_model({"messages": [msg]}, self._runtime())
|
||||
assert update is not None
|
||||
assert [tc["name"] for tc in update["messages"][0].tool_calls] == ["ask_clarification"]
|
||||
|
||||
def test_leaves_solo_clarification_unchanged(self, middleware):
|
||||
msg = self._ai([{"id": "c1", "name": "ask_clarification", "args": {"question": "q?"}}])
|
||||
assert middleware.after_model({"messages": [msg]}, self._runtime()) is None
|
||||
|
||||
def test_leaves_non_clarification_turn_unchanged(self, middleware):
|
||||
msg = self._ai(
|
||||
[
|
||||
{"id": "b1", "name": "bash", "args": {"command": "ls"}},
|
||||
{"id": "w1", "name": "write_file", "args": {"path": "a.txt"}},
|
||||
]
|
||||
)
|
||||
assert middleware.after_model({"messages": [msg]}, self._runtime()) is None
|
||||
|
||||
def test_disable_clarification_keeps_sibling_tools(self, middleware):
|
||||
msg = self._ai(
|
||||
[
|
||||
{"id": "c1", "name": "ask_clarification", "args": {"question": "q?"}},
|
||||
{"id": "b1", "name": "bash", "args": {"command": "echo hi"}},
|
||||
]
|
||||
)
|
||||
assert middleware.after_model({"messages": [msg]}, self._runtime(disable_clarification=True)) is None
|
||||
|
||||
def test_drops_siblings_when_clarification_args_fail_json_parse(self, middleware):
|
||||
"""default_tool_parser splits raw OpenAI payloads per call."""
|
||||
from langchain_core.messages import AIMessage
|
||||
|
||||
msg = AIMessage(
|
||||
content="",
|
||||
additional_kwargs={
|
||||
"tool_calls": [
|
||||
{
|
||||
"id": "c1",
|
||||
"type": "function",
|
||||
"function": {"name": "ask_clarification", "arguments": "{not-json"},
|
||||
},
|
||||
{
|
||||
"id": "b1",
|
||||
"type": "function",
|
||||
"function": {"name": "bash", "arguments": '{"command": "echo hi"}'},
|
||||
},
|
||||
]
|
||||
},
|
||||
)
|
||||
assert [tc["name"] for tc in msg.tool_calls] == ["bash"]
|
||||
assert [tc["name"] for tc in msg.invalid_tool_calls] == ["ask_clarification"]
|
||||
|
||||
update = middleware.after_model({"messages": [msg]}, self._runtime())
|
||||
assert update is not None
|
||||
patched = update["messages"][0]
|
||||
assert patched.tool_calls == []
|
||||
assert [tc["name"] for tc in patched.invalid_tool_calls] == ["ask_clarification"]
|
||||
|
||||
def test_drops_siblings_when_clarification_is_invalid(self, middleware):
|
||||
"""LangChain parks malformed ask_clarification on invalid_tool_calls."""
|
||||
invalid = [
|
||||
{
|
||||
"id": "c1",
|
||||
"name": "ask_clarification",
|
||||
"args": "{",
|
||||
"error": "Failed to parse tool arguments",
|
||||
"type": "invalid_tool_call",
|
||||
}
|
||||
]
|
||||
msg = self._ai(
|
||||
[{"id": "b1", "name": "bash", "args": {"command": "rm -rf /tmp/foo"}}],
|
||||
invalid_tool_calls=invalid,
|
||||
)
|
||||
update = middleware.after_model({"messages": [msg]}, self._runtime())
|
||||
assert update is not None
|
||||
patched = update["messages"][0]
|
||||
assert patched.tool_calls == []
|
||||
assert [tc["name"] for tc in patched.invalid_tool_calls] == ["ask_clarification"]
|
||||
assert patched.invalid_tool_calls[0]["id"] == "c1"
|
||||
assert patched.id == msg.id
|
||||
|
||||
def test_leaves_solo_invalid_clarification_unchanged(self, middleware):
|
||||
msg = self._ai(
|
||||
[],
|
||||
invalid_tool_calls=[
|
||||
{
|
||||
"id": "c1",
|
||||
"name": "ask_clarification",
|
||||
"args": "{",
|
||||
"error": "Failed to parse tool arguments",
|
||||
"type": "invalid_tool_call",
|
||||
}
|
||||
],
|
||||
)
|
||||
assert middleware.after_model({"messages": [msg]}, self._runtime()) is None
|
||||
|
||||
def test_disable_clarification_keeps_siblings_when_clarification_is_invalid(self, middleware):
|
||||
msg = self._ai(
|
||||
[{"id": "b1", "name": "bash", "args": {"command": "echo hi"}}],
|
||||
invalid_tool_calls=[
|
||||
{
|
||||
"id": "c1",
|
||||
"name": "ask_clarification",
|
||||
"args": "{",
|
||||
"error": "Failed to parse tool arguments",
|
||||
"type": "invalid_tool_call",
|
||||
}
|
||||
],
|
||||
)
|
||||
assert middleware.after_model({"messages": [msg]}, self._runtime(disable_clarification=True)) is None
|
||||
|
||||
def test_keeps_valid_clarification_when_mixed_with_invalid_and_sibling(self, middleware):
|
||||
msg = self._ai(
|
||||
[
|
||||
{"id": "c1", "name": "ask_clarification", "args": {"question": "Which dir?"}},
|
||||
{"id": "b1", "name": "bash", "args": {"command": "echo hi"}},
|
||||
],
|
||||
invalid_tool_calls=[
|
||||
{
|
||||
"id": "c2",
|
||||
"name": "ask_clarification",
|
||||
"args": "{",
|
||||
"error": "Failed to parse tool arguments",
|
||||
"type": "invalid_tool_call",
|
||||
}
|
||||
],
|
||||
)
|
||||
patched = middleware.after_model({"messages": [msg]}, self._runtime())["messages"][0]
|
||||
assert [tc["name"] for tc in patched.tool_calls] == ["ask_clarification"]
|
||||
assert patched.tool_calls[0]["id"] == "c1"
|
||||
assert [tc["id"] for tc in patched.invalid_tool_calls] == ["c2"]
|
||||
|
||||
def test_strips_sibling_content_blocks_when_clarification_is_invalid(self, middleware):
|
||||
content = [
|
||||
{"type": "text", "text": "asking"},
|
||||
{"type": "tool_use", "id": "c1", "name": "ask_clarification", "input": "{"},
|
||||
{"type": "tool_use", "id": "b1", "name": "bash", "input": {"command": "rm -rf /"}},
|
||||
]
|
||||
msg = self._ai(
|
||||
[{"id": "b1", "name": "bash", "args": {"command": "rm -rf /"}}],
|
||||
content=content,
|
||||
invalid_tool_calls=[
|
||||
{
|
||||
"id": "c1",
|
||||
"name": "ask_clarification",
|
||||
"args": "{",
|
||||
"error": "Failed to parse tool arguments",
|
||||
"type": "invalid_tool_call",
|
||||
}
|
||||
],
|
||||
)
|
||||
patched = middleware.after_model({"messages": [msg]}, self._runtime())["messages"][0]
|
||||
assert patched.content == [
|
||||
{"type": "text", "text": "asking"},
|
||||
{"type": "tool_use", "id": "c1", "name": "ask_clarification", "input": "{"},
|
||||
]
|
||||
assert patched.tool_calls == []
|
||||
|
||||
def test_strips_matching_tool_use_content_blocks(self, middleware):
|
||||
content = [
|
||||
{"type": "text", "text": "asking"},
|
||||
{"type": "tool_use", "id": "c1", "name": "ask_clarification", "input": {"question": "q?"}},
|
||||
{"type": "tool_use", "id": "b1", "name": "bash", "input": {"command": "rm -rf /"}},
|
||||
]
|
||||
msg = self._ai(
|
||||
[
|
||||
{"id": "c1", "name": "ask_clarification", "args": {"question": "q?"}},
|
||||
{"id": "b1", "name": "bash", "args": {"command": "rm -rf /"}},
|
||||
],
|
||||
content=content,
|
||||
)
|
||||
patched = middleware.after_model({"messages": [msg]}, self._runtime())["messages"][0]
|
||||
assert patched.content == [
|
||||
{"type": "text", "text": "asking"},
|
||||
{"type": "tool_use", "id": "c1", "name": "ask_clarification", "input": {"question": "q?"}},
|
||||
]
|
||||
|
||||
def test_strips_idless_gemini_function_call_content_blocks(self, middleware):
|
||||
# Gemini-style function_call blocks have no id; langchain synthesizes
|
||||
# ids onto tool_calls only. Matching by id would leave the dropped
|
||||
# sibling's content block in the transcript.
|
||||
content = [
|
||||
{"type": "text", "text": "asking"},
|
||||
{"type": "function_call", "name": "ask_clarification", "args": {"question": "q?"}},
|
||||
{"type": "function_call", "name": "bash", "args": {"command": "rm -rf /"}},
|
||||
]
|
||||
msg = self._ai(
|
||||
[
|
||||
{"id": "c1", "name": "ask_clarification", "args": {"question": "q?"}},
|
||||
{"id": "b1", "name": "bash", "args": {"command": "rm -rf /"}},
|
||||
],
|
||||
content=content,
|
||||
)
|
||||
patched = middleware.after_model({"messages": [msg]}, self._runtime())["messages"][0]
|
||||
assert patched.content == [
|
||||
{"type": "text", "text": "asking"},
|
||||
{"type": "function_call", "name": "ask_clarification", "args": {"question": "q?"}},
|
||||
]
|
||||
assert [tc["name"] for tc in patched.tool_calls] == ["ask_clarification"]
|
||||
|
||||
def test_aafter_model_matches_sync(self, middleware):
|
||||
import asyncio
|
||||
|
||||
msg = self._ai(
|
||||
[
|
||||
{"id": "c1", "name": "ask_clarification", "args": {"question": "q?"}},
|
||||
{"id": "b1", "name": "bash", "args": {"command": "echo hi"}},
|
||||
]
|
||||
)
|
||||
update = asyncio.run(middleware.aafter_model({"messages": [msg]}, self._runtime()))
|
||||
assert [tc["name"] for tc in update["messages"][0].tool_calls] == ["ask_clarification"]
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user