diff --git a/CHANGELOG.md b/CHANGELOG.md index 23180fe44..519ed19ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -224,6 +224,17 @@ This section accumulates work toward the **2.1.0** milestone `use_previous_response_id` chaining cannot resume the original server-side history. Controlled by `read_before_write.elide_blocked_payloads` (default on) and `read_before_write.elide_min_chars` (default 2000). +- **agents:** `ToolOutputBudgetMiddleware` now also elides the `content` of a + successful `write_file` call from model-bound requests once the same path + was read or modified again later in the conversation. After a successful + write the file on disk is the source of truth, and the read-before-write + gate forces a `read_file` before the next modification, so the historical + copy was redundant with that read and long report-writing runs carried every + section twice. The newest `tool_output.keep_recent_writes` successful writes + (default 1) always stay visible, `str_replace` payloads are never touched, + and stored history, receipts, and the run journal keep the original + arguments. Controlled by `tool_output.elide_superseded_writes` (default on) + and `tool_output.superseded_write_min_chars` (default 2000). #### Memory diff --git a/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md b/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md index d74a8a51f..2a886a1a8 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md +++ b/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md @@ -44,7 +44,7 @@ ordinary compaction or a model call. **Shared runtime base** (`build_lead_runtime_middlewares`; subagents reuse most of this via `build_subagent_runtime_middlewares`): 1. **InputSanitizationMiddleware** - First, so it is the outermost `wrap_model_call` wrapper; every inner middleware (including LLM retries) sees sanitized messages. `additional_kwargs.original_user_content` is server-owned provenance: Gateway strips caller-supplied values for non-internal run requests, trusted IM calls may carry the string they captured before adding transport/file context, and the middleware replaces any non-string value before wrapping. Uploads and sanitization retain first-writer-wins only for validated strings. -2. **ToolOutputBudgetMiddleware** - Caps tool output size (per app config) before it re-enters the model context. Oversized results are externalized to `tool_output.storage_subdir` (default `.tool-results`, shared constant `TOOL_RESULTS_DIRNAME`) under the thread outputs dir with a typed synopsis + `read_file` reference left in context; those files are process feedback, so the workspace-changes scanner excludes that directory and run delivery verification never counts them as produced artifacts +2. **ToolOutputBudgetMiddleware** - Caps model-bound tool output per app config. Externalizes oversized results to `tool_output.storage_subdir` (default `.tool-results`, constant `TOOL_RESULTS_DIRNAME`) under thread outputs, leaving a typed synopsis + `read_file` reference. These process-feedback files are excluded from workspace-change scans and delivery verification. `wrap_model_call` elides successful `write_file` content only in model-bound requests (#5328) after a later successful same-path `read_file`/`write_file`/`str_replace`; the on-disk file becomes the reference. Preserves the newest `keep_recent_writes` writes. Pairs call occurrences via `tool_call_args.pair_tool_call_results` and rewrites through shared `tool_call_args` helpers; controls: `elide_superseded_writes`, `superseded_write_min_chars`. 3. **ToolResultSanitizationMiddleware** - Neutralizes framework/injection tags (e.g. ``) and boundary markers in *remote-content* tool results (`web_fetch`/`web_search`/`image_search`/`web_capture`) so attacker-controlled fetched pages cannot forge trusted framework context. Mirrors `InputSanitizationMiddleware`'s user-input guardrail for the other untrusted-content entry point; sits inner of `ToolOutputBudgetMiddleware` (neutralizes the raw output, then the budget truncates). Local tool output (bash/read_file) is left untouched. Scope is a name-based allowlist for the first-party web tools, plus every MCP-sourced tool via its `deerflow_mcp` metadata tag, so an MCP server naming its fetcher `fetch_url` is still covered Result-rewriting middlewares between the raw callable boundary and the diff --git a/backend/packages/harness/deerflow/agents/middlewares/read_before_write_middleware.py b/backend/packages/harness/deerflow/agents/middlewares/read_before_write_middleware.py index e6b0afdf5..48dcabac3 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/read_before_write_middleware.py +++ b/backend/packages/harness/deerflow/agents/middlewares/read_before_write_middleware.py @@ -40,7 +40,6 @@ import logging import posixpath import threading import weakref -from collections import defaultdict, deque from collections.abc import Awaitable, Callable from typing import Any, override @@ -50,7 +49,7 @@ from langchain_core.messages import AIMessage, ToolMessage from langgraph.prebuilt.tool_node import ToolCallRequest from langgraph.types import Command -from deerflow.agents.middlewares.tool_call_args import rewrite_messages_tool_call_args +from deerflow.agents.middlewares.tool_call_args import pair_tool_call_results, rewrite_messages_tool_call_args from deerflow.agents.middlewares.tool_result_meta import normalize_tool_result, stamp_exception_meta from deerflow.config.read_before_write_config import ReadBeforeWriteConfig from deerflow.sandbox.exceptions import SandboxAuthorizationError @@ -402,30 +401,12 @@ def elide_blocked_write_payloads(messages: list[Any], *, min_chars: int) -> list def _blocked_call_occurrences(messages: list[Any]) -> set[tuple[int, str]]: """Return ``(id(ai_message), call_id)`` for every call occurrence answered by a gate-blocked result. - Tool-call ids may repeat across assistant turns, so a history-wide id set - would also hit an earlier (or later) *successful* call with the same id and - mislabel it as blocked. Results are paired with call occurrences the way - ``DanglingToolCallMiddleware`` does: ToolMessages queue per id in history - order and each AIMessage call consumes the next one for its id. + Pairing is per occurrence (``tool_call_args.pair_tool_call_results``): + tool-call ids may repeat across assistant turns, so a history-wide id set + would also hit an earlier (or later) *successful* call with the same id + and mislabel it as blocked. """ - results_by_id: dict[str, deque[ToolMessage]] = defaultdict(deque) - for message in messages: - if isinstance(message, ToolMessage) and isinstance(message.tool_call_id, str) and message.tool_call_id: - results_by_id[message.tool_call_id].append(message) - - blocked: set[tuple[int, str]] = set() - for message in messages: - if not isinstance(message, AIMessage): - continue - for tool_call in message.tool_calls or (): - call_id = tool_call.get("id") if isinstance(tool_call, dict) else None - if not isinstance(call_id, str) or not call_id: - continue - queue = results_by_id.get(call_id) - result = queue.popleft() if queue else None - if result is not None and isinstance((result.additional_kwargs or {}).get(WRITE_BLOCK_KEY), dict): - blocked.add((id(message), call_id)) - return blocked + return {(id(occurrence.message), occurrence.call_id) for occurrence in pair_tool_call_results(messages) if occurrence.result is not None and isinstance((occurrence.result.additional_kwargs or {}).get(WRITE_BLOCK_KEY), dict)} def _elide_args(args: dict[str, Any], tool_name: str, min_chars: int) -> dict[str, Any] | None: diff --git a/backend/packages/harness/deerflow/agents/middlewares/tool_call_args.py b/backend/packages/harness/deerflow/agents/middlewares/tool_call_args.py index 52a3360a4..c22463a3f 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/tool_call_args.py +++ b/backend/packages/harness/deerflow/agents/middlewares/tool_call_args.py @@ -26,7 +26,11 @@ that touched ``tool_calls`` alone would still send the original payload. ``model_copy`` (or the same object when nothing matched), so callers never mutate state and the result is identical across model calls. Policy — which calls, and what replaces their arguments — stays with the caller; see -``read_before_write_middleware.elide_blocked_write_payloads`` for one. +``read_before_write_middleware.elide_blocked_write_payloads`` and +``tool_output_budget_middleware.elide_superseded_write_payloads``. Both decide +per call *occurrence*, pairing each AIMessage call with the ToolMessage that +answered it through :func:`pair_tool_call_results`, because tool-call ids may +repeat across assistant turns. A rewrite also invalidates server-side continuation. With ``use_previous_response_id`` the OpenAI adapter sends only the messages after @@ -44,10 +48,12 @@ billed either way, so replay costs no more). from __future__ import annotations import json +from collections import Counter, defaultdict, deque from collections.abc import Callable, Mapping, Sequence +from dataclasses import dataclass, replace from typing import Any -from langchain_core.messages import AIMessage +from langchain_core.messages import AIMessage, ToolMessage #: Replacement args keyed by tool-call id. ArgsReplacements = Mapping[str, dict[str, Any]] @@ -62,8 +68,13 @@ def rewrite_messages_tool_call_args(messages: list[Any], replacement_for: Replac was replaced. Untouched messages pass through by identity, except that once anything was rewritten every AIMessage loses its ``resp_`` response id (see the module docstring: the server-side history behind that id still holds - the original arguments). Only calls with a non-empty string id are offered - to the selector, since nothing else can be matched across surfaces. + the original arguments). Only calls with a non-empty string id that is + unique within its message are offered to the selector: every surface is + addressed by id, so nothing else can be matched across surfaces, and an id + a malformed provider payload repeats inside one AIMessage could only be + rewritten for *all* of its occurrences at once — a failed sibling would + take on the successful call's arguments (review on #5374). Such calls are + conservatively left alone. """ updated: list[Any] = [] changed = False @@ -71,11 +82,12 @@ def rewrite_messages_tool_call_args(messages: list[Any], replacement_for: Replac patched = message if isinstance(message, AIMessage) and message.tool_calls: replacements: dict[str, dict[str, Any]] = {} + duplicated = _duplicated_call_ids(message.tool_calls) for tool_call in message.tool_calls: if not isinstance(tool_call, dict): continue call_id = tool_call.get("id") - if not isinstance(call_id, str) or not call_id: + if not isinstance(call_id, str) or not call_id or call_id in duplicated: continue new_args = replacement_for(message, tool_call) if new_args is not None: @@ -90,6 +102,78 @@ def rewrite_messages_tool_call_args(messages: list[Any], replacement_for: Replac return [_without_response_chain_id(message) for message in updated] +def _duplicated_call_ids(tool_calls: Sequence[Any]) -> set[str]: + """Ids that occur more than once in one message's structured tool-call list (the list every surface mirrors). + + Only non-empty string ids are counted: a list or dict id from a malformed + provider payload is unhashable and must be skipped, never hashed, or the + whole model call would fail (review on #5374). + """ + counts = Counter(call_id for tool_call in tool_calls if isinstance(tool_call, dict) and isinstance(call_id := tool_call.get("id"), str) and call_id) + return {call_id for call_id, count in counts.items() if count > 1} + + +@dataclass(frozen=True, slots=True) +class ToolCallOccurrence: + """One tool call on one AIMessage, paired with the ToolMessage that answered it (``None`` if unanswered).""" + + #: Position of ``message`` in the history it was paired from. + index: int + message: AIMessage + tool_call: dict[str, Any] + result: ToolMessage | None + + @property + def call_id(self) -> str: + return self.tool_call["id"] + + @property + def name(self) -> str: + name = self.tool_call.get("name") + return name if isinstance(name, str) else "" + + @property + def args(self) -> dict[str, Any]: + args = self.tool_call.get("args") + return args if isinstance(args, dict) else {} + + +def pair_tool_call_results(messages: Sequence[Any]) -> list[ToolCallOccurrence]: + """Pair every AIMessage tool call carrying a non-empty string id with the ToolMessage that answered it. + + Walks ``messages`` in document order. Each AIMessage opens its own calls, + and a ToolMessage answers the still-open call with its id from the *most + recent preceding* AIMessage only — the rule ``DanglingToolCallMiddleware`` + applies: a result never answers a call from an earlier turn. So ids that + repeat across turns pair per occurrence, an interrupted call whose id a + later turn reused stays unanswered instead of inheriting that turn's result + (review on #5374), and stray or duplicate results are ignored. ``index`` is + the AIMessage's position in ``messages``, so callers can order events + across turns; the calls of one AIMessage share an index because they ran + concurrently, in no fixed order. + """ + occurrences: list[ToolCallOccurrence] = [] + # Unanswered calls of the most recent AIMessage: id -> positions in ``occurrences``. + open_calls: dict[str, deque[int]] = defaultdict(deque) + for index, message in enumerate(messages): + if isinstance(message, AIMessage): + open_calls = defaultdict(deque) + for tool_call in message.tool_calls or (): + if not isinstance(tool_call, dict): + continue + call_id = tool_call.get("id") + if not isinstance(call_id, str) or not call_id: + continue + open_calls[call_id].append(len(occurrences)) + occurrences.append(ToolCallOccurrence(index, message, tool_call, None)) + elif isinstance(message, ToolMessage): + queue = open_calls.get(message.tool_call_id) if isinstance(message.tool_call_id, str) else None + if queue: + position = queue.popleft() + occurrences[position] = replace(occurrences[position], result=message) + return occurrences + + def _without_response_chain_id(message: Any) -> Any: """Drop an OpenAI ``resp_`` response id so the adapter replays history instead of chaining to it.""" if not isinstance(message, AIMessage): diff --git a/backend/packages/harness/deerflow/agents/middlewares/tool_output_budget_middleware.py b/backend/packages/harness/deerflow/agents/middlewares/tool_output_budget_middleware.py index a29d20597..05e44225c 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/tool_output_budget_middleware.py +++ b/backend/packages/harness/deerflow/agents/middlewares/tool_output_budget_middleware.py @@ -4,6 +4,21 @@ Oversized tool results are persisted to disk and replaced with a compact typed synopsis containing a file reference. When disk persistence is unavailable the middleware falls back to head+tail truncation so the model context is never blown by a single large tool return. + +The model-call hooks also budget the other bulky side of a tool call: the +``content`` argument of a successful ``write_file`` call (issue #5328, step +2). After a successful write the file on disk is the source of truth, and the +read-before-write gate forces a ``read_file`` before the next modification of +that path, so once a *later* successful read or write of the same path exists +the historical copy is redundant with it. Such superseded content is replaced +by a short deterministic placeholder in the *model-bound request only* +(``request.override``): ``state["messages"]``, checkpoints, tool receipts, +loop detection, and the run journal keep the original arguments, and nothing +is externalized to disk (the file itself is the reference). The newest +``keep_recent_writes`` successful writes always stay visible so the model can +still say what it just wrote without a read. Gate-blocked calls are the +read-before-write middleware's own policy; both rewrite through the shared +``tool_call_args`` helper so every provider surface changes together. """ from __future__ import annotations @@ -11,20 +26,23 @@ from __future__ import annotations import asyncio import logging import os +import posixpath import shlex import uuid -from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Mapping from dataclasses import replace as dc_replace from typing import TYPE_CHECKING, Any, override from langchain.agents import AgentState from langchain.agents.middleware import AgentMiddleware from langchain.agents.middleware.types import ModelCallResult, ModelRequest, ModelResponse -from langchain_core.messages import ToolMessage +from langchain_core.messages import AIMessage, ToolMessage from langgraph.prebuilt.tool_node import ToolCallRequest from langgraph.types import Command +from deerflow.agents.middlewares.tool_call_args import ToolCallOccurrence, pair_tool_call_results, rewrite_messages_tool_call_args from deerflow.agents.middlewares.tool_output_synopsis import render_tool_output_preview +from deerflow.agents.middlewares.tool_result_meta import TOOL_META_KEY from deerflow.agents.middlewares.tool_transform_meta import append_tool_transform from deerflow.config.tool_output_config import ToolOutputConfig from deerflow.sandbox.sandbox_provider import get_sandbox_provider @@ -578,6 +596,115 @@ def _patch_model_messages(messages: list[Any], config: ToolOutputConfig) -> list return updated if changed else None +# --------------------------------------------------------------------------- +# Superseded write payload elision (issue #5328, step 2) +# --------------------------------------------------------------------------- + +_WRITE_TOOL = "write_file" +# A successful call of these tools changes the file, so every earlier write's +# content is stale afterwards. ``str_replace`` payloads are never elided +# themselves: they are usually small, and the issue scopes step 2 to +# ``write_file.content``. +_FILE_MODIFYING_TOOLS = frozenset({"write_file", "str_replace"}) +# A non-error read (full, ranged, or head-truncated — ``partial_success``) +# showed the model the on-disk file, which is what the placeholder points at. +_FILE_READING_TOOLS = frozenset({"read_file"}) +_SUPERSEDING_READ_STATUSES = frozenset({"success", "partial_success"}) +# Deterministic for a given payload so repeated model calls keep the same +# request prefix (prompt caching) instead of drifting. Framework-owned static +# text plus a character count; no model-supplied value is interpolated (the +# path stays visible in the call's own ``path`` argument). +_ELIDED_WRITE_CONTENT_TEMPLATE = "[content elided: {chars} chars; this write_file call succeeded and the file was read or modified again afterwards, so the on-disk file is the current version; call read_file on its path to see it]" + + +def elide_superseded_write_payloads(messages: list[Any], *, min_chars: int, keep_recent: int) -> list[Any] | None: + """Return ``messages`` with superseded ``write_file`` content replaced by placeholders, or ``None`` if unchanged. + + Only the policy lives here. A call qualifies when its paired result is + stamped ``deerflow_tool_meta.status == "success"``, its ``content`` is a + string of at least ``min_chars`` characters, a *later* message holds a + successful ``read_file`` / ``write_file`` / ``str_replace`` of the same + normalized path, and it is not among the ``keep_recent`` newest successful + writes. Calls are paired with results per occurrence + (``tool_call_args.pair_tool_call_results``), and "later" means a later + message index: the calls of one AIMessage ran concurrently, so a same-turn + read may predate the write and never supersedes it. The surface-by-surface + rewrite is ``rewrite_messages_tool_call_args``, which never mutates the + input and passes untouched messages through by identity, so the stored + history keeps the original arguments and the output is identical across + model calls. The policy is monotonic: once a write is elided, more history + never brings its content back. + """ + if not _has_elidable_write(messages, min_chars): + return None + + latest_touch: dict[str, int] = {} + successful_writes: list[tuple[ToolCallOccurrence, str]] = [] + for occurrence in pair_tool_call_results(messages): + path = _normalized_path_arg(occurrence.args) + if path is None: + continue + name = occurrence.name + if name in _FILE_MODIFYING_TOOLS: + if _result_status(occurrence.result) != "success": + continue + if name == _WRITE_TOOL: + successful_writes.append((occurrence, path)) + elif name in _FILE_READING_TOOLS: + if _result_status(occurrence.result) not in _SUPERSEDING_READ_STATUSES: + continue + else: + continue + latest_touch[path] = max(latest_touch.get(path, -1), occurrence.index) + + replacements: dict[tuple[int, str], dict[str, Any]] = {} + cutoff = max(0, len(successful_writes) - keep_recent) + for occurrence, path in successful_writes[:cutoff]: + content = occurrence.args.get("content") + if not isinstance(content, str) or not content or len(content) < min_chars: + continue + if latest_touch.get(path, -1) <= occurrence.index: + continue + replacements[(id(occurrence.message), occurrence.call_id)] = {**occurrence.args, "content": _ELIDED_WRITE_CONTENT_TEMPLATE.format(chars=len(content))} + if not replacements: + return None + + def replacement_for(message: AIMessage, tool_call: dict[str, Any]) -> dict[str, Any] | None: + return replacements.get((id(message), tool_call["id"])) + + return rewrite_messages_tool_call_args(messages, replacement_for) + + +def _has_elidable_write(messages: list[Any], min_chars: int) -> bool: + """Cheap pre-scan so a history without a sizeable ``write_file`` call is never paired or rebuilt.""" + for message in messages: + if not isinstance(message, AIMessage): + continue + for tool_call in message.tool_calls or (): + if not isinstance(tool_call, dict) or tool_call.get("name") != _WRITE_TOOL: + continue + args = tool_call.get("args") + content = args.get("content") if isinstance(args, dict) else None + if isinstance(content, str) and content and len(content) >= min_chars: + return True + return False + + +def _result_status(result: ToolMessage | None) -> str | None: + """``deerflow_tool_meta.status`` of a paired result; ``None`` when unanswered or unstamped (never treated as success).""" + if result is None: + return None + meta = (result.additional_kwargs or {}).get(TOOL_META_KEY) + status = meta.get("status") if isinstance(meta, dict) else None + return status if isinstance(status, str) else None + + +def _normalized_path_arg(args: Mapping[str, Any]) -> str | None: + """The call's ``path`` argument normalized the way the read-before-write gate keys its marks.""" + path = args.get("path") + return posixpath.normpath(path) if isinstance(path, str) and path else None + + # --------------------------------------------------------------------------- # Middleware class # --------------------------------------------------------------------------- @@ -636,7 +763,7 @@ class ToolOutputBudgetMiddleware(AgentMiddleware[AgentState]): sandbox = _resolve_sandbox(request) return await asyncio.to_thread(_patch_result, result, self._config, outputs_path, sandbox) - # -- model call hooks (historical message truncation) ------------------ + # -- model call hooks (historical context budgeting) ------------------- @override def wrap_model_call( @@ -644,13 +771,7 @@ class ToolOutputBudgetMiddleware(AgentMiddleware[AgentState]): request: ModelRequest, handler: Callable[[ModelRequest], ModelResponse], ) -> ModelCallResult: - if self._config.enabled: - messages = getattr(request, "messages", None) - if isinstance(messages, list): - patched = _patch_model_messages(messages, self._config) - if patched is not None: - request = request.override(messages=patched) - return handler(request) + return handler(self._budget_model_request(request)) @override async def awrap_model_call( @@ -658,10 +779,28 @@ class ToolOutputBudgetMiddleware(AgentMiddleware[AgentState]): request: ModelRequest, handler: Callable[[ModelRequest], Awaitable[ModelResponse]], ) -> ModelCallResult: - if self._config.enabled: - messages = getattr(request, "messages", None) - if isinstance(messages, list): - patched = _patch_model_messages(messages, self._config) - if patched is not None: - request = request.override(messages=patched) - return await handler(request) + # Pure in-memory rewrite: no sandbox or file I/O, so it stays on the loop. + return await handler(self._budget_model_request(request)) + + def _budget_model_request(self, request: ModelRequest) -> ModelRequest: + """Truncate oversized historical tool output and elide superseded write payloads in the request copy only.""" + if not self._config.enabled: + return request + original = getattr(request, "messages", None) + if not isinstance(original, list): + return request + messages = original + patched = _patch_model_messages(messages, self._config) + if patched is not None: + messages = patched + if self._config.elide_superseded_writes: + elided = elide_superseded_write_payloads( + messages, + min_chars=self._config.superseded_write_min_chars, + keep_recent=self._config.keep_recent_writes, + ) + if elided is not None: + messages = elided + if messages is original: + return request + return request.override(messages=messages) diff --git a/backend/packages/harness/deerflow/config/tool_output_config.py b/backend/packages/harness/deerflow/config/tool_output_config.py index bfd5638f0..a3e4d3bf4 100644 --- a/backend/packages/harness/deerflow/config/tool_output_config.py +++ b/backend/packages/harness/deerflow/config/tool_output_config.py @@ -16,6 +16,12 @@ class ToolOutputConfig(BaseModel): the full output is persisted to disk and replaced with a compact preview + file reference. If disk persistence is unavailable the output falls back to head+tail truncation. + + The same middleware also budgets the other bulky side of a tool call in + model-bound requests: the ``content`` argument of a successful + ``write_file`` call, once a later read or write of the same path has + made the historical copy redundant with the file on disk + (``elide_superseded_writes``; issue #5328). """ enabled: bool = Field( @@ -86,3 +92,28 @@ class ToolOutputConfig(BaseModel): default_factory=dict, description="Per-tool externalize_min_chars overrides. Keys are tool names, values are char thresholds. Use 0 to disable externalization for a specific tool.", ) + elide_superseded_writes: bool = Field( + default=True, + description=( + "Replace the content argument of a successful write_file call with a short placeholder in model-bound " + "requests once the same path was read or modified again later in the conversation. After a successful " + "write the file on disk is the source of truth, and the read-before-write gate forces a read_file before " + "the next modification, so the historical copy is redundant with that read. Only the request copy " + "changes: stored message history, receipts, and the run journal keep the original arguments." + ), + ) + superseded_write_min_chars: int = Field( + default=2000, + ge=0, + description=( + "Elide only write_file content at least this many characters long; shorter content stays visible. " + "0 elides every non-empty content. This is a Python character count, not a token count: the same value " + "spans roughly 3-4x in real context cost between ASCII and CJK text, and the placeholder's elided-size " + "figure is the same character count." + ), + ) + keep_recent_writes: int = Field( + default=1, + ge=0, + description="Never elide the content of the newest N successful write_file calls (counted across all paths), even when superseded, so the model can still say what it just wrote without a read. 0 keeps none.", + ) diff --git a/backend/tests/test_read_before_write_middleware.py b/backend/tests/test_read_before_write_middleware.py index 4859c1855..08228dc1a 100644 --- a/backend/tests/test_read_before_write_middleware.py +++ b/backend/tests/test_read_before_write_middleware.py @@ -792,3 +792,53 @@ class TestBlockedPayloadElision: assert len(calls) == 1 assert json.loads(calls[0]["arguments"])["content"].startswith("[payload elided: 5000 chars") assert payload not in json.dumps(sent, ensure_ascii=False) + + def test_unanswered_write_with_a_reused_id_is_not_labeled_blocked(self): + """Review on #5374: an interrupted write must not inherit the blocked result of a later call that reused its id.""" + mw = self._middleware() + draft = "d" * 5000 + interrupted = AIMessage(content="", tool_calls=[{"name": "write_file", "id": "reused", "args": {"description": "d", "path": "/mnt/user-data/outputs/other.md", "content": draft}}]) + blocked_ai, blocked = self._blocked_turn(mw, "write_file", {"description": "d", "path": self.PATH, "content": "b" * 5000}, tool_call_id="reused") + request = self._model_request([HumanMessage(content="go"), interrupted, blocked_ai, blocked]) + handler = MagicMock(return_value=AIMessage(content="ok")) + + mw.wrap_model_call(request, handler) + + captured = self._captured(handler).messages + assert captured[1] is interrupted + assert captured[2].tool_calls[0]["args"]["content"].startswith("[payload elided: 5000 chars") + + def test_duplicate_ids_in_one_turn_are_never_rewritten(self): + """Review on #5374: a successful sibling sharing the id of a blocked write must not be rewritten into it.""" + from deerflow.agents.middlewares.read_before_write_middleware import WRITE_BLOCK_KEY + + mw = self._middleware() + turn = AIMessage( + content="", + tool_calls=[ + {"name": "write_file", "id": "dup", "args": {"description": "d", "path": self.PATH, "content": "a" * 5000}}, + {"name": "write_file", "id": "dup", "args": {"description": "d", "path": "/mnt/user-data/outputs/other.md", "content": "b" * 5000}}, + ], + ) + blocked = ToolMessage(content="Error: blocked", tool_call_id="dup", name="write_file", status="error", additional_kwargs={WRITE_BLOCK_KEY: {"path": self.PATH, "tool": "write_file"}}) + ok = ToolMessage(content="OK", tool_call_id="dup", name="write_file") + request = self._model_request([HumanMessage(content="go"), turn, blocked, ok]) + handler = MagicMock(return_value=AIMessage(content="ok")) + + mw.wrap_model_call(request, handler) + + assert self._captured(handler) is request + + def test_unhashable_sibling_id_does_not_crash_the_model_call(self): + """Review on #5374 (round 3): a malformed sibling id next to a blocked call must be skipped, not hashed.""" + mw = self._middleware() + ai, blocked = self._blocked_turn(mw, "write_file", {"description": "d", "path": self.PATH, "content": "x" * 5000}) + ai.tool_calls.append({"name": "bash", "id": ["not", "a", "string"], "args": {"command": "ls"}}) + request = self._model_request([HumanMessage(content="go"), ai, blocked]) + handler = MagicMock(return_value=AIMessage(content="ok")) + + mw.wrap_model_call(request, handler) + + rewritten = self._captured(handler).messages[1] + assert rewritten.tool_calls[0]["args"]["content"].startswith("[payload elided: 5000 chars") + assert rewritten.tool_calls[1] == ai.tool_calls[1] diff --git a/backend/tests/test_tool_call_args.py b/backend/tests/test_tool_call_args.py index da4fd47de..a39c0b509 100644 --- a/backend/tests/test_tool_call_args.py +++ b/backend/tests/test_tool_call_args.py @@ -4,7 +4,7 @@ import json from langchain_core.messages import AIMessage, AIMessageChunk, HumanMessage, ToolMessage -from deerflow.agents.middlewares.tool_call_args import rewrite_messages_tool_call_args, rewrite_tool_call_args +from deerflow.agents.middlewares.tool_call_args import pair_tool_call_results, rewrite_messages_tool_call_args, rewrite_tool_call_args ARGS = {"path": "/mnt/user-data/outputs/report.md", "content": "x" * 50} NEW_ARGS = {"path": "/mnt/user-data/outputs/report.md", "content": "[elided]"} @@ -385,3 +385,173 @@ class TestResponseChainInvalidation: assert calls[0]["id"] == "fc_1" assert any(item.get("type") == "function_call_output" for item in payload["input"]) assert self.PAYLOAD not in json.dumps(payload, ensure_ascii=False) + + +class TestPairToolCallResults: + """Per-occurrence pairing of AIMessage tool calls with the ToolMessage that answered them.""" + + @staticmethod + def _call(call_id, name="bash", args=None): + return {"name": name, "id": call_id, "args": {"command": "ls"} if args is None else args} + + def test_pairs_each_call_with_the_result_that_answered_it(self): + ai = AIMessage(content="", tool_calls=[self._call("call-1"), self._call("call-2")]) + first = ToolMessage(content="1", tool_call_id="call-1") + second = ToolMessage(content="2", tool_call_id="call-2") + + occurrences = pair_tool_call_results([HumanMessage(content="go"), ai, second, first]) + + assert [(o.index, o.message is ai, o.call_id, o.result) for o in occurrences] == [(1, True, "call-1", first), (1, True, "call-2", second)] + assert occurrences[0].name == "bash" + assert occurrences[0].args == {"command": "ls"} + + def test_unanswered_call_gets_no_result(self): + ai = AIMessage(content="", tool_calls=[self._call("call-1")]) + + occurrences = pair_tool_call_results([ai]) + + assert len(occurrences) == 1 + assert occurrences[0].result is None + + def test_reused_ids_pair_per_occurrence_in_history_order(self): + first_ai = AIMessage(content="", tool_calls=[self._call("call-1")]) + first_result = ToolMessage(content="first", tool_call_id="call-1") + second_ai = AIMessage(content="", tool_calls=[self._call("call-1")]) + second_result = ToolMessage(content="second", tool_call_id="call-1") + + occurrences = pair_tool_call_results([first_ai, first_result, second_ai, second_result]) + + assert [(o.index, o.result) for o in occurrences] == [(0, first_result), (2, second_result)] + + def test_calls_without_a_string_id_are_skipped(self): + ai = AIMessage(content="", tool_calls=[self._call("call-1"), {"name": "bash", "id": None, "args": {}}, {"name": "bash", "id": "", "args": {}}]) + ai.tool_calls.append({"name": "bash", "id": ["not", "a", "string"], "args": {}}) + + occurrences = pair_tool_call_results([ai, ToolMessage(content="1", tool_call_id="call-1")]) + + assert [o.call_id for o in occurrences] == ["call-1"] + + def test_non_ai_messages_and_non_dict_calls_are_ignored(self): + ai = AIMessage(content="", tool_calls=[self._call("call-1")]) + ai.tool_calls.append("not-a-dict") # malformed provider payload + + occurrences = pair_tool_call_results([HumanMessage(content="go"), ToolMessage(content="stray", tool_call_id="call-9"), ai]) + + assert [o.call_id for o in occurrences] == ["call-1"] + + def test_accessors_tolerate_malformed_calls(self): + ai = AIMessage(content="", tool_calls=[self._call("call-1")]) + # Malformed provider payloads can only get here past construction-time validation. + ai.tool_calls[0]["args"] = "not-a-dict" + del ai.tool_calls[0]["name"] + + (occurrence,) = pair_tool_call_results([ai]) + + assert occurrence.name == "" + assert occurrence.args == {} + assert occurrence.call_id == "call-1" + + def test_empty_history_pairs_nothing(self): + assert pair_tool_call_results([]) == [] + + def test_unanswered_call_never_consumes_a_later_turns_result_for_a_reused_id(self): + """Review on #5374: an interrupted call must not inherit the result of a later call that reused its id.""" + interrupted = AIMessage(content="", tool_calls=[self._call("reused", args={"path": "report.md"})]) + read = AIMessage(content="", tool_calls=[self._call("r1", name="read_file")]) + read_result = ToolMessage(content="text", tool_call_id="r1") + later = AIMessage(content="", tool_calls=[self._call("reused", args={"path": "notes.md"})]) + later_result = ToolMessage(content="OK", tool_call_id="reused") + + occurrences = pair_tool_call_results([interrupted, read, read_result, later, later_result]) + + assert [(o.index, o.call_id, o.result) for o in occurrences] == [(0, "reused", None), (1, "r1", read_result), (3, "reused", later_result)] + + def test_result_answers_only_the_most_recent_preceding_turn(self): + """A result never answers a call from an earlier turn (same rule as DanglingToolCallMiddleware).""" + first = AIMessage(content="", tool_calls=[self._call("call-x")]) + second = AIMessage(content="", tool_calls=[self._call("call-y")]) + stale = ToolMessage(content="late", tool_call_id="call-x") + fresh = ToolMessage(content="ok", tool_call_id="call-y") + + occurrences = pair_tool_call_results([first, second, stale, fresh]) + + assert [(o.call_id, o.result) for o in occurrences] == [("call-x", None), ("call-y", fresh)] + + def test_stray_results_before_any_call_are_ignored(self): + ai = AIMessage(content="", tool_calls=[self._call("call-1")]) + stray = ToolMessage(content="stray", tool_call_id="call-1") + real = ToolMessage(content="real", tool_call_id="call-1") + + occurrences = pair_tool_call_results([stray, ai, real]) + + assert [(o.call_id, o.result) for o in occurrences] == [("call-1", real)] + + def test_second_result_for_an_answered_call_is_ignored(self): + ai = AIMessage(content="", tool_calls=[self._call("call-1")]) + first = ToolMessage(content="first", tool_call_id="call-1") + duplicate = ToolMessage(content="duplicate", tool_call_id="call-1") + + occurrences = pair_tool_call_results([ai, first, duplicate]) + + assert [(o.call_id, o.result) for o in occurrences] == [("call-1", first)] + + def test_non_ai_messages_between_call_and_result_do_not_break_pairing(self): + ai = AIMessage(content="", tool_calls=[self._call("call-1"), self._call("call-2")]) + first = ToolMessage(content="1", tool_call_id="call-1") + second = ToolMessage(content="2", tool_call_id="call-2") + + occurrences = pair_tool_call_results([ai, first, HumanMessage(content="reminder"), second]) + + assert [(o.call_id, o.result) for o in occurrences] == [("call-1", first), ("call-2", second)] + + +class TestDuplicateIdsWithinOneMessage: + """Review on #5374: surfaces are addressed by id, so an id that repeats inside one AIMessage can never be rewritten for just one occurrence.""" + + @staticmethod + def _message(): + calls = [ + {"name": "write_file", "id": "dup", "args": {"path": "a.md", "content": "a" * 50}}, + {"name": "write_file", "id": "dup", "args": {"path": "b.md", "content": "b" * 50}}, + {"name": "write_file", "id": "solo", "args": {"path": "c.md", "content": "c" * 50}}, + ] + return AIMessage( + content=[{"type": "tool_use", "id": call["id"], "name": call["name"], "input": dict(call["args"])} for call in calls], + tool_calls=[dict(call, args=dict(call["args"])) for call in calls], + additional_kwargs={"tool_calls": [{"id": call["id"], "type": "function", "function": {"name": call["name"], "arguments": json.dumps(call["args"])}} for call in calls]}, + ) + + def test_duplicated_ids_are_never_offered_or_rewritten_on_any_surface(self): + message = self._message() + offered: list[str] = [] + + def replacement_for(_message, tool_call): + offered.append(tool_call["id"]) + return {**tool_call["args"], "content": "[elided]"} + + (rewritten,) = rewrite_messages_tool_call_args([message], replacement_for) + + assert offered == ["solo"] + assert [call["args"]["content"][:1] for call in rewritten.tool_calls] == ["a", "b", "["] + assert [block["input"]["content"][:1] for block in rewritten.content] == ["a", "b", "["] + raw = [json.loads(entry["function"]["arguments"])["content"][:1] for entry in rewritten.additional_kwargs["tool_calls"]] + assert raw == ["a", "b", "["] + assert [call["args"]["path"] for call in rewritten.tool_calls] == ["a.md", "b.md", "c.md"] + + def test_message_with_only_duplicated_ids_passes_through_by_identity(self): + message = self._message() + message.tool_calls.pop() # leave the two ``dup`` calls only + + assert rewrite_messages_tool_call_args([message], lambda _m, tool_call: {"content": "[elided]"}) is None + assert rewrite_tool_call_args(message, {"dup": {"content": "[elided]"}}) is not message # the low-level rewriter itself stays id-keyed + + def test_unhashable_sibling_id_neither_crashes_nor_blocks_the_rewrite(self): + """Review on #5374 (round 3): a list/dict id from a malformed payload must be skipped, not hashed.""" + message = AIMessage(content="", tool_calls=[{"name": "write_file", "id": "call-1", "args": dict(ARGS)}]) + message.tool_calls.append({"name": "bash", "id": ["not", "a", "string"], "args": {"command": "ls"}}) + message.tool_calls.append({"name": "bash", "id": {"nested": "dict"}, "args": {"command": "ls"}}) + + (rewritten,) = rewrite_messages_tool_call_args([message], lambda _m, tool_call: NEW_ARGS if tool_call["id"] == "call-1" else None) + + assert rewritten.tool_calls[0]["args"] == NEW_ARGS + assert rewritten.tool_calls[1:] == message.tool_calls[1:] diff --git a/backend/tests/test_tool_output_budget_middleware.py b/backend/tests/test_tool_output_budget_middleware.py index 0615c493c..b68291188 100644 --- a/backend/tests/test_tool_output_budget_middleware.py +++ b/backend/tests/test_tool_output_budget_middleware.py @@ -1,9 +1,9 @@ """Comprehensive tests for ToolOutputBudgetMiddleware. Covers: pass-through, disk externalization, fallback truncation, UTF-8 -boundaries, Command results, model-request history patching, config -variations, exempt tools, per-tool overrides, edge cases, and both -sync/async code paths. +boundaries, Command results, model-request history patching, superseded +write_file payload elision (issue #5328), config variations, exempt tools, +per-tool overrides, edge cases, and both sync/async code paths. """ from __future__ import annotations @@ -1593,3 +1593,544 @@ class TestBudgetContentNoSandboxNoProviderCall: assert result[1] == "externalized" assert called["n"] == 0 assert (tmp_path / ".tool-results").is_dir() + + +# =========================================================================== +# Superseded write payload elision (issue #5328, step 2) +# =========================================================================== + + +def _meta_result(name: str, tool_call_id: str, content: str = "OK", *, status: str | None = "success") -> ToolMessage: + """A ToolMessage stamped the way ToolErrorHandlingMiddleware stamps it; ``status=None`` leaves it unstamped.""" + msg = ToolMessage(content=content, name=name, tool_call_id=tool_call_id, status="error" if status == "error" else "success") + if status is not None: + msg.additional_kwargs["deerflow_tool_meta"] = { + "status": status, + "error_type": None, + "recoverable_by_model": True, + "recommended_next_action": "continue", + "source": "content_analysis", + } + return msg + + +def _write(tool_call_id: str, path: str, content: str, *, append: bool = False, status: str | None = "success") -> tuple[AIMessage, ToolMessage]: + args = {"description": "d", "path": path, "content": content, "append": append} + ai = AIMessage(content="", tool_calls=[{"name": "write_file", "id": tool_call_id, "args": args}]) + return ai, _meta_result("write_file", tool_call_id, "Error: boom" if status == "error" else "OK", status=status) + + +def _read(tool_call_id: str, path: str, *, status: str | None = "success") -> tuple[AIMessage, ToolMessage]: + ai = AIMessage(content="", tool_calls=[{"name": "read_file", "id": tool_call_id, "args": {"path": path}}]) + return ai, _meta_result("read_file", tool_call_id, "Error: File not found" if status == "error" else "file text", status=status) + + +def _str_replace(tool_call_id: str, path: str, *, new_str: str = "n", status: str | None = "success") -> tuple[AIMessage, ToolMessage]: + ai = AIMessage(content="", tool_calls=[{"name": "str_replace", "id": tool_call_id, "args": {"path": path, "old_str": "o", "new_str": new_str}}]) + return ai, _meta_result("str_replace", tool_call_id, "Error: boom" if status == "error" else "OK", status=status) + + +class TestSupersededWriteElision: + """Model-bound requests drop the content of successful write_file calls superseded by a later read or write of the same path.""" + + PATH = "/mnt/user-data/outputs/report.md" + OTHER = "/mnt/user-data/outputs/other.md" + + @staticmethod + def _middleware(**overrides) -> ToolOutputBudgetMiddleware: + return ToolOutputBudgetMiddleware(config=ToolOutputConfig(**overrides)) + + @staticmethod + def _model_request(messages) -> ModelRequest: + return ModelRequest(model=None, messages=list(messages), tools=[], state={"messages": list(messages)}) + + def _forward(self, mw: ToolOutputBudgetMiddleware, messages) -> tuple[ModelRequest, ModelRequest]: + """Run ``wrap_model_call`` and return ``(original request, request the handler received)``.""" + captured: dict[str, ModelRequest] = {} + + def handler(req): + captured["request"] = req + return AIMessage(content="ok") + + request = self._model_request(messages) + mw.wrap_model_call(request, handler) + return request, captured["request"] + + @staticmethod + def _content(forwarded: ModelRequest, index: int) -> str: + return forwarded.messages[index].tool_calls[0]["args"]["content"] + + # -- policy ------------------------------------------------------------ + + def test_superseded_write_is_elided_and_newest_write_is_kept(self): + mw = self._middleware() + payload = "x" * 5000 + human = HumanMessage(content="go") + w1, r1 = _write("call-1", self.PATH, payload) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.PATH, "y" * 5000, append=True) + + request, forwarded = self._forward(mw, [human, w1, r1, rd, rr, w2, r2]) + + assert forwarded is not request + elided = forwarded.messages[1].tool_calls[0]["args"] + assert elided["content"].startswith("[content elided: 5000 chars") + assert "read_file" in elided["content"] + assert payload not in elided["content"] + assert elided["path"] == self.PATH + assert elided["description"] == "d" + assert elided["append"] is False + # The newest successful write stays visible (keep_recent_writes=1). + assert forwarded.messages[5] is w2 + # Untouched neighbours pass through by identity; state and stored history keep the original. + assert forwarded.messages[0] is human + assert forwarded.messages[2] is r1 + assert forwarded.messages[3] is rd + assert request.messages[1] is w1 + assert request.state["messages"][1] is w1 + assert w1.tool_calls[0]["args"]["content"] == payload + + def test_write_without_a_later_touch_of_the_path_is_kept(self): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + w2, r2 = _write("call-2", self.OTHER, "y" * 5000) + w3, r3 = _write("call-3", "/mnt/user-data/outputs/third.md", "z" * 5000) + + request, forwarded = self._forward(mw, [w1, r1, w2, r2, w3, r3]) + + assert forwarded is request + + def test_newest_write_is_kept_even_when_superseded(self): + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + + request, forwarded = self._forward(self._middleware(), [w1, r1, rd, rr]) + assert forwarded is request + + _request, forwarded = self._forward(self._middleware(keep_recent_writes=0), [w1, r1, rd, rr]) + assert self._content(forwarded, 0).startswith("[content elided: 5000 chars") + + def test_keep_recent_counts_successful_writes_across_paths(self): + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + w3, r3 = _write("call-4", "/mnt/user-data/outputs/third.md", "tiny") + history = [w1, r1, rd, rr, w2, r2, w3, r3] + + _request, forwarded = self._forward(self._middleware(keep_recent_writes=2), history) + assert self._content(forwarded, 0).startswith("[content elided") + + request, forwarded = self._forward(self._middleware(keep_recent_writes=3), history) + assert forwarded is request + + def test_later_successful_write_of_the_same_path_supersedes(self): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + w2, r2 = _write("call-2", self.PATH, "y" * 5000) + w3, r3 = _write("call-3", self.OTHER, "z" * 5000) + + _request, forwarded = self._forward(mw, [w1, r1, w2, r2, w3, r3]) + + assert self._content(forwarded, 0).startswith("[content elided: 5000 chars") + assert forwarded.messages[2] is w2 # not superseded, and older than the newest write + assert forwarded.messages[4] is w3 + + def test_later_successful_str_replace_supersedes(self): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + sr, srr = _str_replace("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + _request, forwarded = self._forward(mw, [w1, r1, sr, srr, w2, r2]) + + assert self._content(forwarded, 0).startswith("[content elided") + assert forwarded.messages[2] is sr + + @pytest.mark.parametrize("status", ["error", "partial_success", None], ids=["error", "partial", "unstamped"]) + def test_later_write_that_did_not_succeed_does_not_supersede(self, status): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + w2, r2 = _write("call-2", self.PATH, "y" * 5000, status=status) + w3, r3 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, r1, w2, r2, w3, r3]) + + assert forwarded is request + + def test_gate_blocked_later_write_does_not_supersede(self): + from deerflow.agents.middlewares.read_before_write_middleware import WRITE_BLOCK_KEY + + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + w2, r2 = _write("call-2", self.PATH, "y" * 5000, status="error") + r2.additional_kwargs[WRITE_BLOCK_KEY] = {"path": self.PATH, "tool": "write_file"} + w3, r3 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, r1, w2, r2, w3, r3]) + + assert forwarded is request + + @pytest.mark.parametrize("status", ["error", None], ids=["error", "unstamped"]) + def test_read_that_did_not_succeed_does_not_supersede(self, status): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH, status=status) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert forwarded is request + + def test_partial_read_still_supersedes(self): + """A truncated or ranged read still showed the model the on-disk file.""" + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH, status="partial_success") + w2, r2 = _write("call-3", self.OTHER, "tiny") + + _request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert self._content(forwarded, 0).startswith("[content elided") + + @pytest.mark.parametrize("status", ["error", "partial_success", None], ids=["error", "partial", "unstamped"]) + def test_write_that_did_not_succeed_is_never_a_candidate(self, status): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000, status=status) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert forwarded is request + + def test_unanswered_write_is_never_a_candidate(self): + mw = self._middleware() + w1, _unused = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, rd, rr, w2, r2]) + + assert forwarded is request + + def test_same_turn_read_does_not_supersede(self): + """Parallel calls in one AIMessage run in no fixed order, so the read may predate the write.""" + mw = self._middleware() + payload = "x" * 5000 + ai = AIMessage( + content="", + tool_calls=[ + {"name": "write_file", "id": "call-1", "args": {"description": "d", "path": self.PATH, "content": payload}}, + {"name": "read_file", "id": "call-2", "args": {"path": self.PATH}}, + ], + ) + results = [_meta_result("write_file", "call-1"), _meta_result("read_file", "call-2", "file text")] + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [ai, *results, w2, r2]) + + assert forwarded is request + + def test_paths_are_normalized_before_matching(self): + mw = self._middleware() + w1, r1 = _write("call-1", "/mnt/user-data/outputs/./report.md", "x" * 5000) + rd, rr = _read("call-2", "/mnt/user-data/outputs/sub/../report.md") + w2, r2 = _write("call-3", self.OTHER, "tiny") + + _request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert self._content(forwarded, 0).startswith("[content elided") + + def test_different_path_does_not_supersede(self): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.OTHER) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert forwarded is request + + def test_str_replace_payloads_are_never_elided(self): + mw = self._middleware() + sr, srr = _str_replace("call-1", self.PATH, new_str="n" * 5000) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [sr, srr, rd, rr, w2, r2]) + + assert forwarded is request + + # -- thresholds and config --------------------------------------------- + + def test_content_below_min_chars_stays_visible(self): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "short " * 20) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert forwarded is request + + def test_min_chars_zero_elides_any_non_empty_content(self): + mw = self._middleware(superseded_write_min_chars=0) + w1, r1 = _write("call-1", self.PATH, "v1") + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "") + rd2, rr2 = _read("call-4", self.OTHER) + w3, r3 = _write("call-5", "/mnt/user-data/outputs/third.md", "tiny") + + _request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2, rd2, rr2, w3, r3]) + + assert self._content(forwarded, 0).startswith("[content elided: 2 chars") + assert forwarded.messages[4] is w2 + + def test_non_string_content_is_left_alone(self): + mw = self._middleware(superseded_write_min_chars=0) + ai = AIMessage(content="", tool_calls=[{"name": "write_file", "id": "call-1", "args": {"path": self.PATH, "content": ["not", "a", "string"]}}]) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [ai, _meta_result("write_file", "call-1"), rd, rr, w2, r2]) + + assert forwarded is request + + def test_disabled_by_config_passes_request_through(self): + mw = self._middleware(elide_superseded_writes=False) + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert forwarded is request + + def test_middleware_disabled_passes_request_through(self): + mw = self._middleware(enabled=False) + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert forwarded is request + + def test_config_defaults(self): + config = ToolOutputConfig() + assert config.elide_superseded_writes is True + assert config.superseded_write_min_chars == 2000 + assert config.keep_recent_writes == 1 + + def test_release_policy_declares_config(self): + params = self._middleware(keep_recent_writes=3, superseded_write_min_chars=123).release_policy_parameters() + assert params["config"]["elide_superseded_writes"] is True + assert params["config"]["superseded_write_min_chars"] == 123 + assert params["config"]["keep_recent_writes"] == 3 + + def test_from_app_config_passes_the_keys(self): + config = AppConfig(sandbox=SandboxConfig(use="test"), tool_output={"superseded_write_min_chars": 10, "keep_recent_writes": 0}) + mw = ToolOutputBudgetMiddleware.from_app_config(config) + assert mw._config.superseded_write_min_chars == 10 + assert mw._config.keep_recent_writes == 0 + + def test_config_example_documents_the_keys(self): + import yaml + + example_path = os.path.join(os.path.dirname(__file__), "..", "..", "config.example.yaml") + with open(example_path, encoding="utf-8") as f: + data = yaml.safe_load(f) + tool_output = data["tool_output"] + assert tool_output["elide_superseded_writes"] is True + assert tool_output["superseded_write_min_chars"] == 2000 + assert tool_output["keep_recent_writes"] == 1 + # New user-settable keys are a schema change: the outdated-config warning must fire. + assert data["config_version"] >= 42 + + # -- surfaces, pairing, determinism, composition ------------------------ + + def test_rewrites_every_provider_surface_together(self): + mw = self._middleware() + payload = "y" * 5000 + args = {"description": "d", "path": self.PATH, "content": payload} + ai = AIMessage( + content=[ + {"type": "text", "text": "writing"}, + {"type": "tool_use", "id": "call-1", "name": "write_file", "input": dict(args), "partial_json": json.dumps(args)}, + ], + tool_calls=[{"name": "write_file", "id": "call-1", "args": dict(args)}], + additional_kwargs={"tool_calls": [{"id": "call-1", "type": "function", "function": {"name": "write_file", "arguments": json.dumps(args)}}]}, + ) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + _request, forwarded = self._forward(mw, [ai, _meta_result("write_file", "call-1"), rd, rr, w2, r2]) + + rewritten = forwarded.messages[0] + structured = rewritten.tool_calls[0]["args"] + assert structured["content"].startswith("[content elided: 5000 chars") + raw = json.loads(rewritten.additional_kwargs["tool_calls"][0]["function"]["arguments"]) + assert raw == structured + block = rewritten.content[1] + assert block["input"] == structured + assert "partial_json" not in block + assert rewritten.content[0] == {"type": "text", "text": "writing"} + assert payload not in json.dumps(rewritten.model_dump(), ensure_ascii=False) + # Original objects are untouched. + assert ai.content[1]["input"]["content"] == payload + assert payload in ai.additional_kwargs["tool_calls"][0]["function"]["arguments"] + + def test_reused_call_ids_pair_per_occurrence(self): + """A failed write and a later successful one may share a tool-call id; the failed one must not inherit success.""" + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000, status="error") + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-1", self.OTHER, "y" * 5000) + w3, r3 = _write("call-3", "/mnt/user-data/outputs/third.md", "tiny") + + request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2, w3, r3]) + + assert forwarded is request + + def test_elision_is_deterministic_across_model_calls(self): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + history = [w1, r1, rd, rr, w2, r2] + + _request, first = self._forward(mw, history) + _request, second = self._forward(mw, history) + + assert first.messages[0].tool_calls == second.messages[0].tool_calls + + def test_elision_is_monotonic_as_history_grows(self): + """Once a write is elided, appending more history never brings its content back.""" + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + history = [w1, r1, rd, rr, w2, r2] + _request, before = self._forward(mw, history) + assert self._content(before, 0).startswith("[content elided") + + w3, r3 = _write("call-4", "/mnt/user-data/outputs/third.md", "z" * 5000) + _request, after = self._forward(mw, [*history, HumanMessage(content="more"), w3, r3]) + + assert after.messages[0].tool_calls == before.messages[0].tool_calls + + def test_rewritten_history_drops_openai_response_chain_ids(self): + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + w1.response_metadata = {"id": "resp_write", "output_version": "responses/v1"} + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + w2.response_metadata = {"id": "resp_latest"} + + _request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + assert "id" not in forwarded.messages[0].response_metadata + assert "id" not in forwarded.messages[4].response_metadata + + def test_applies_alongside_historical_output_truncation(self): + mw = self._middleware(fallback_max_chars=500, fallback_head_chars=100, fallback_tail_chars=50) + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + oversized = _tm("q" * 1000, name="tool", tool_call_id="tc-q") + w2, r2 = _write("call-3", self.OTHER, "tiny") + + _request, forwarded = self._forward(mw, [w1, r1, rd, rr, oversized, w2, r2]) + + assert self._content(forwarded, 0).startswith("[content elided") + assert "omitted" in forwarded.messages[4].content + assert forwarded.messages[5] is w2 + + def test_async_model_call_elides(self): + import asyncio + + mw = self._middleware() + w1, r1 = _write("call-1", self.PATH, "x" * 5000) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + request = self._model_request([w1, r1, rd, rr, w2, r2]) + seen: dict[str, ModelRequest] = {} + + async def handler(req): + seen["request"] = req + return AIMessage(content="ok") + + asyncio.run(mw.awrap_model_call(request, handler)) + + assert seen["request"] is not request + assert self._content(seen["request"], 0).startswith("[content elided") + + def test_no_write_calls_in_history_is_a_cheap_no_op(self): + mw = self._middleware() + history = [HumanMessage(content="go"), AIMessage(content="", tool_calls=[{"name": "bash", "id": "call-1", "args": {"command": "ls"}}]), _meta_result("bash", "call-1", "files")] + + request, forwarded = self._forward(mw, history) + + assert forwarded is request + + def test_chat_completions_payload_uses_the_placeholder(self): + """End to end against the OpenAI chat-completions message converter.""" + from langchain_openai.chat_models.base import _convert_message_to_dict + + mw = self._middleware() + payload = "x" * 5000 + w1, r1 = _write("call-1", self.PATH, payload) + rd, rr = _read("call-2", self.PATH) + w2, r2 = _write("call-3", self.OTHER, "tiny") + + _request, forwarded = self._forward(mw, [w1, r1, rd, rr, w2, r2]) + + wire = json.loads(_convert_message_to_dict(forwarded.messages[0])["tool_calls"][0]["function"]["arguments"]) + assert wire["content"].startswith("[content elided: 5000 chars") + assert payload not in json.dumps(wire) + + def test_unanswered_write_with_a_reused_id_is_never_treated_as_successful(self): + """Review on #5374: an interrupted write must not inherit the success of a later call that reused its id.""" + mw = self._middleware() + draft = "d" * 5000 + interrupted, _never_delivered = _write("reused", self.PATH, draft) + rd, rr = _read("call-2", self.PATH) + later, later_ok = _write("reused", self.OTHER, "n" * 5000) + last, last_ok = _write("call-4", "/mnt/user-data/outputs/last.md", "l" * 5000) + + request, forwarded = self._forward(mw, [interrupted, rd, rr, later, later_ok, last, last_ok]) + + assert forwarded is request + assert interrupted.tool_calls[0]["args"]["content"] == draft + + def test_duplicate_ids_in_one_turn_are_never_rewritten(self): + """Review on #5374: a failed sibling sharing the id of a superseded successful write must not be rewritten into it.""" + mw = self._middleware() + turn = AIMessage( + content="", + tool_calls=[ + {"name": "write_file", "id": "dup", "args": {"path": self.PATH, "content": "a" * 5000}}, + {"name": "write_file", "id": "dup", "args": {"path": self.OTHER, "content": "b" * 5000}}, + ], + ) + results = [_meta_result("write_file", "dup"), _meta_result("write_file", "dup", "Error: boom", status="error")] + rd, rr = _read("call-2", self.PATH) + last, last_ok = _write("call-3", "/mnt/user-data/outputs/last.md", "l" * 5000) + + request, forwarded = self._forward(mw, [turn, *results, rd, rr, last, last_ok]) + + assert forwarded is request + assert [call["args"]["path"] for call in turn.tool_calls] == [self.PATH, self.OTHER] + assert turn.tool_calls[1]["args"]["content"] == "b" * 5000 + + def test_unhashable_sibling_id_does_not_crash_the_model_call(self): + """Review on #5374 (round 3): a malformed sibling id next to an elision candidate must be skipped, not hashed.""" + mw = self._middleware(keep_recent_writes=0) + payload = "x" * 5000 + turn, ok = _write("call-1", self.PATH, payload) + turn.tool_calls.append({"name": "bash", "id": ["not", "a", "string"], "args": {"command": "ls"}}) + rd, rr = _read("call-2", self.PATH) + + _request, forwarded = self._forward(mw, [turn, ok, rd, rr]) + + assert self._content(forwarded, 0).startswith("[content elided: 5000 chars") + assert forwarded.messages[0].tool_calls[1] == turn.tool_calls[1] diff --git a/config.example.yaml b/config.example.yaml index 719a27bf2..9059321bc 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -1197,6 +1197,13 @@ tool_search: # # `exempt_tools` prevents persist→read→persist infinite loops for read tools. # `tool_overrides` allows per-tool threshold customization. +# +# The same middleware also elides the `content` argument of successful +# write_file calls from model-bound requests once the same path was read or +# modified again later (issue #5328): the file on disk is then the source of +# truth, and the read-before-write gate forces a read_file before the next +# modification anyway, so the historical copy only costs context. Stored +# history, receipts, and the run journal keep the original arguments. tool_output: enabled: true @@ -1215,6 +1222,18 @@ tool_output: # tool_overrides: # web_search: 8000 # bash: 20000 + # Replace a successful write_file call's `content` with a short placeholder + # pointing at read_file once a later successful read_file / write_file / + # str_replace of the same path exists. Only the model-bound request changes. + elide_superseded_writes: true + # Only elide content at least this many characters long (a character count, + # not tokens: CJK text costs ~3-4x more per character than ASCII). 0 elides + # every non-empty content. + superseded_write_min_chars: 2000 + # The newest N successful write_file calls (across all paths) always stay + # visible, even when superseded, so the model can still say what it just + # wrote without a read. 0 keeps none. + keep_recent_writes: 1 # ============================================================================ # Suggestions Configuration