From 868c1ff807ad6a12793821bbbcedf430c8c78d16 Mon Sep 17 00:00:00 2001 From: haotianqi666 Date: Sat, 24 Oct 2026 08:06:38 +0800 Subject: [PATCH] fix(agents): treat read_file JSON errors as document data (#7369) Co-authored-by: haotianqi666 <250384991+haotianqi666@users.noreply.github.com> --- README.md | 6 + .../deerflow/agents/middlewares/AGENTS.md | 6 + .../agents/middlewares/tool_result_meta.py | 9 +- .../tests/test_read_file_json_result_meta.py | 126 ++++++++++++++++++ 4 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 backend/tests/test_read_file_json_result_meta.py diff --git a/README.md b/README.md index bbc98a6d9c..c25ede3f09 100644 --- a/README.md +++ b/README.md @@ -954,6 +954,12 @@ BoxLite shutdown rejects late VM registration and keeps its SDK loop open while in-flight acquisitions finish. If they cannot drain within five seconds, shutdown fails with resources still owned and can be retried. +Successful `read_file` calls treat a JSON document's `error` field as file data, +not a failure of the read tool. This keeps receipts accurate when inspecting +saved API error responses and avoids blocking later reads when the optional +`tool_progress` guard is enabled. Actual read failures and output truncation +still retain their error or partial-result status. + #### MCP Server In the chat UI, enable **Token Usage → Debug** to inspect generic/MCP tool calls. diff --git a/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md b/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md index a6df8f6ffd..4cc9557d98 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md +++ b/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md @@ -114,6 +114,12 @@ No match: skip inspection, log ID. Tests: `test_read_mark_tool_call_correlation. 13. **ReadBeforeWriteMiddleware** - *(`read_before_write.enabled`, default on; #3857)* Existing-file `write_file` (including append) and `str_replace` require the newest matching `read_file` content hash. Marks live on ToolMessages: dropping a read loses its mark; writes never refresh it. Sync/async read/check/execution/mark steps share a per-(scope, path) gate. Uninspectable content fails open. It owns sandbox authorization; `SandboxAuthorizationError` becomes an error ToolMessage. Outside ToolProgress/ToolErrorHandling, blocks consume no slot and stamp `deerflow_tool_meta` plus `deerflow_write_block` (`{path, tool}`). `wrap_model_call` elides blocked payloads with deterministic placeholders (`elide_blocked_payloads`, `elide_min_chars`), preserving state, receipts and journal. All model-bound argument rewrites use `tool_call_args`; policy stays in this gate. 14. **ToolProgressMiddleware** - *(optional, if `tool_progress.enabled`)* State-machine-based stagnation guard (RFC #3177). Outer wrapper around ToolErrorHandlingMiddleware so its tool wrapper receives results already stamped with `deerflow_tool_meta`. Recoverable problems stay WARNED with hints; retryable non-recoverable problems escalate to BLOCKED; stop-category failures block immediately. It is independent of LoopDetection's turn-level call-pattern guard. Effective phase changes emit `middleware:tool_progress`, including `reset` when a later agent invocation (for example, goal continuation) clears WARNED/BLOCKED state. The state machine supplies the action and count threshold (`null` for category rules, recovery, and reset); recorder calls happen after releasing the state lock. Events include the actual sync/async hook plus bounded status, error, and recovery fields. Server-owned recorder keys determine subagent attribution. Arguments, content, prompts, and hashes are never persisted; recorder failures are fail-open. See [event semantics](../../../../../docs/RUN_EVENT_STREAM.md). 15. **ToolReceiptMiddleware + ToolErrorHandlingMiddleware** - `ToolReceiptMiddleware` is *(optional, if `verification.receipts_enabled`, default on)*. It is the **outermost `wrap_tool_call` layer** — registered ahead of authorization, audit, write and progress gates — because Guardrail/SandboxAudit/ReadBeforeWrite/ToolProgress can short-circuit a call with their own ToolMessage (and SandboxAudit annotates medium-risk results); an inner receipt layer would silently gap the ledger on those results (ordering constraints in `deerflow.extensions.ordering`). Normal results still carry the `deerflow_tool_meta` status ToolErrorHandlingMiddleware stamps on the inner return path; short-circuit messages self-stamp meta or fall back to `message.status`. It stamps deterministic provenance (tool name, status, args/output hashes, byte count, timestamp) onto direct `ToolMessage` results and every matching `ToolMessage` carried in `Command.update.messages`, including delegated `task`, `present_file`, `view_image`, and `tool_search` results; before model calls it derives a hidden receipt ledger (display ids r1..rN) from message state, and when the 2,000-character budget is exceeded the newest receipts are retained in chronological order with their original ids plus an older-receipts omission marker. Rendering returns both the text and its retained receipt subset; every response that received a ledger carries only that exact server-owned subset, never omitted receipts. Snapshot validation accepts a strictly consecutive positive original-id range (for example `r24`–`r30`) rather than requiring `r1`, so subagent terminal citation verification resolves ids against evidence present in the citing turn even when later summarization drops and renumbers tool messages. Model-generated citation IDs are digit-bounded before integer conversion; oversized IDs are ignored as malformed input rather than raising through task write-back. Citation parsing deduplicates exact `(id, anchor)` pairs, not IDs alone, so repeated identical references stay compact while every distinct anchor claim is verified. Gateway strips delegated receipts/verdicts from external messages. `ToolErrorHandlingMiddleware` receives `AppConfig`, converts tool exceptions into error `ToolMessage`s so the run can continue instead of aborting, stamps every result with `deerflow_tool_meta` (status / error_type / recoverable_by_model / recommended_next_action / source) via `tool_result_meta.normalize_tool_result`, stamps structured metadata for task exception wrappers, and stamps skill-read metadata for downstream durable-context capture. Task tool result text is generated from the same status/result/error inputs as the structured metadata so callers do not hand-write a second protocol string. + JSON error-envelope inference excludes successful native `read_file` results: + their `error` fields belong to the document. Explicit error status, existing + metadata and legacy `Error:` failures remain authoritative; other tools keep + JSON error inference. Real truncation markers still produce partial results. + `tests/test_read_file_json_result_meta.py` exercises native sync/async reads, + receipts and follow-up calls with the progress guard on and off. 16. **ArtifactCaptureMiddleware** - See [tool artifacts](TOOL_ARTIFACTS.md). Authorization identity is independent of enforcement. Gateway strips client identity overrides: only the server auth source sets `is_internal`, and only authenticated IM `body.context` supplies `channel_user_id` (never `body.config`). `build_principal_from_context` applies role defaults, strict provenance, and copied attributes; RBAC rejects unknown defaults. Delegation and `GuardrailMiddleware` share this identity. Layer 1 precedes deferred assembly across agent paths and its provider is reused for Layer 2; framework skill/memory ordering stays stable. Trusted `DeerFlowClient.stream()` accepts identity overrides. Its graph key always includes effective storage `user_id` and, when enforced, the full Principal; nested attributes are copied so mutation cannot hide stale cache state. diff --git a/backend/packages/harness/deerflow/agents/middlewares/tool_result_meta.py b/backend/packages/harness/deerflow/agents/middlewares/tool_result_meta.py index dec2dbd996..7dc34c1362 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/tool_result_meta.py +++ b/backend/packages/harness/deerflow/agents/middlewares/tool_result_meta.py @@ -313,8 +313,13 @@ def normalize_tool_message(msg: ToolMessage) -> ToolMessage: attrs = _classify_error_text(content[len(_ERROR_PREFIX) :]) meta = _make_meta(status="error", source="tool_return", **attrs) elif (json_error := _extract_json_error_text(content)) is not None: - attrs = _classify_error_text(json_error) - meta = _make_meta(status="error", source="tool_return", **attrs) + if msg.name == "read_file": + # Native reads return document text, not a tool error envelope. + # Explicit failures above still take precedence over file content. + meta = _make_meta(status="success", source="content_analysis") + else: + attrs = _classify_error_text(json_error) + meta = _make_meta(status="error", source="tool_return", **attrs) elif failed_bash: # A nonzero shell exit does not raise a Python exception. Keep it # recoverable: grep/diff and diagnostic probes can legitimately use 1. diff --git a/backend/tests/test_read_file_json_result_meta.py b/backend/tests/test_read_file_json_result_meta.py new file mode 100644 index 0000000000..8e0d705522 --- /dev/null +++ b/backend/tests/test_read_file_json_result_meta.py @@ -0,0 +1,126 @@ +"""File contents are not a JSON error envelope from the read_file tool.""" + +from __future__ import annotations + +import json +from unittest.mock import AsyncMock + +import pytest +from langchain.agents import create_agent +from langchain_core.language_models.fake_chat_models import GenericFakeChatModel +from langchain_core.messages import AIMessage, HumanMessage, ToolMessage + +from deerflow.agents.middlewares.read_before_write_middleware import ReadBeforeWriteMiddleware +from deerflow.agents.middlewares.tool_error_handling_middleware import ToolErrorHandlingMiddleware +from deerflow.agents.middlewares.tool_progress_middleware import ToolProgressMiddleware +from deerflow.agents.middlewares.tool_receipt import TOOL_RECEIPT_KEY +from deerflow.agents.middlewares.tool_receipt_middleware import ToolReceiptMiddleware +from deerflow.agents.middlewares.tool_result_meta import TOOL_META_KEY, normalize_tool_message +from deerflow.agents.thread_state import ThreadState +from deerflow.config.tool_progress_config import ToolProgressConfig + + +@pytest.mark.parametrize("error", ["Unauthorized", {"code": 401, "message": "Unauthorized"}, ["missing config"], 500, True, "no results found"]) +def test_read_file_json_error_field_is_document_data(error): + content = json.dumps({"error": error}) + message = ToolMessage(content=content, name="read_file", tool_call_id="read") + + result = normalize_tool_message(message) + + assert result.content == content + assert result.additional_kwargs[TOOL_META_KEY] == { + "status": "success", + "error_type": None, + "recoverable_by_model": True, + "recommended_next_action": "continue", + "source": "content_analysis", + } + + +@pytest.mark.parametrize("name,status", [("read_file", "error"), ("web_search", "success"), ("custom_tool", "success")]) +def test_actual_tool_json_errors_are_still_classified(name, status): + message = ToolMessage(content='{"error": {"code": 401, "message": "Unauthorized"}}', name=name, tool_call_id="call", status=status) + + meta = normalize_tool_message(message).additional_kwargs[TOOL_META_KEY] + + assert meta["status"] == "error" + assert meta["error_type"] == "auth" + assert meta["recoverable_by_model"] is False + assert meta["recommended_next_action"] == "stop" + + +@pytest.mark.parametrize("source", ["exception", "tool_return", "custom"]) +def test_read_file_preserves_explicit_failure_metadata(source): + metadata = {"status": "error", "source": source, "error_type": "auth"} + message = ToolMessage(content='{"error":"Unauthorized"}', name="read_file", tool_call_id="read", additional_kwargs={TOOL_META_KEY: metadata}) + + assert normalize_tool_message(message).additional_kwargs[TOOL_META_KEY] is metadata + + +class _ScriptedModel(GenericFakeChatModel): + def bind_tools(self, tools, **kwargs): + return self + + +@pytest.mark.parametrize("mode", ["sync", "async"]) +@pytest.mark.parametrize("progress_enabled", [False, True]) +@pytest.mark.parametrize("first_kind", ["json_error", "success", "missing", "truncated"]) +@pytest.mark.asyncio +async def test_native_read_file_keeps_followup_reads_available(tmp_path, monkeypatch, mode, progress_enabled, first_kind): + from deerflow.sandbox import tools as sandbox_tools + from deerflow.sandbox.local.local_sandbox import LocalSandbox + + outputs = tmp_path / "outputs" + outputs.mkdir() + first_content = '{"error": {"code": 401, "message": "Unauthorized"}}\n' + if first_kind == "success": + first_content = '{"result": "OK"}\n' + elif first_kind == "truncated": + first_content = json.dumps({"error": "Unauthorized", "details": "sample line\n" * 10000}) + if first_kind != "missing": + (outputs / "response.json").write_text(first_content, encoding="utf-8") + notes = "The saved response belongs to an earlier request.\n" + (outputs / "notes.txt").write_text(notes, encoding="utf-8") + + sandbox = LocalSandbox("json-result-test") + reads = [] + original_read = sandbox.read_file + + def trace_read(path, *args, **kwargs): + reads.append(path) + return original_read(path, *args, **kwargs) + + monkeypatch.setattr(sandbox, "read_file", trace_read) + monkeypatch.setattr(sandbox_tools, "ensure_sandbox_initialized", lambda *_args, **_kwargs: sandbox) + monkeypatch.setattr(sandbox_tools, "ensure_sandbox_initialized_async", AsyncMock(return_value=sandbox)) + responses = [AIMessage(content="", tool_calls=[{"name": "read_file", "args": {"path": f"/mnt/user-data/outputs/{filename}"}, "id": f"read-{index}"}]) for index, filename in enumerate(("response.json", "notes.txt"))] + responses.append(AIMessage(content="Done.")) + middleware = [ToolReceiptMiddleware(render_mode="delegation_only"), ReadBeforeWriteMiddleware()] + if progress_enabled: + middleware.append(ToolProgressMiddleware.from_config(ToolProgressConfig(enabled=True))) + middleware.append(ToolErrorHandlingMiddleware()) + graph = create_agent(_ScriptedModel(messages=iter(responses)), tools=[sandbox_tools.read_file_tool], middleware=middleware, state_schema=ThreadState) + state = { + "messages": [HumanMessage(content="Read the saved API response and then the notes explaining it.")], + "sandbox": {"sandbox_id": "local:json-result-test"}, + "thread_data": {"outputs_path": str(outputs)}, + "thread_directories_created": True, + } + context = {"thread_id": "json-result-test", "user_id": "default"} + result = graph.invoke(state, context=context) if mode == "sync" else await graph.ainvoke(state, context=context) + first, second = [message for message in result["messages"] if isinstance(message, ToolMessage)] + + assert second.content == notes + assert str(outputs / "notes.txt") in reads + assert second.additional_kwargs[TOOL_RECEIPT_KEY]["status"] == "success" + expected_status = {"missing": "error", "truncated": "partial_success"}.get(first_kind, "success") + assert first.additional_kwargs[TOOL_META_KEY]["status"] == expected_status + assert first.additional_kwargs[TOOL_RECEIPT_KEY]["status"] == expected_status + if first_kind == "missing": + assert first.content.startswith("Error: File not found:") + assert first.additional_kwargs[TOOL_META_KEY]["error_type"] == "not_found" + elif first_kind == "truncated": + assert len(first.content) < len(first_content) + assert "truncated" in first.content.lower() + else: + assert first.content == first_content