From 8de3ddb9efe9164496b2209be93fe689269d9415 Mon Sep 17 00:00:00 2001 From: fangliquanflq Date: Thu, 6 Aug 2026 20:08:42 +0800 Subject: [PATCH] fix(tools): preserve document extraction boundaries --- tests/tools/test_lazy_deps_durable_target.py | 19 ++++ tests/tools/test_read_extract.py | 58 +++++++++++ tools/file_operations.py | 37 +++++++ tools/file_tools.py | 26 ++++- tools/lazy_deps.py | 12 ++- tools/read_extract.py | 101 ++++++++++++++++++- 6 files changed, 242 insertions(+), 11 deletions(-) diff --git a/tests/tools/test_lazy_deps_durable_target.py b/tests/tools/test_lazy_deps_durable_target.py index 7795dabe9ef66..cb9b517c51a67 100644 --- a/tests/tools/test_lazy_deps_durable_target.py +++ b/tests/tools/test_lazy_deps_durable_target.py @@ -197,6 +197,25 @@ class TestInstallArgConstruction: assert "--target" not in captured["cmd"] assert "--constraint" not in captured["cmd"] + def test_uv_resolution_failure_does_not_fall_through_to_pip(self, monkeypatch): + monkeypatch.delenv(ld._LAZY_TARGET_ENV, raising=False) + monkeypatch.setattr("hermes_cli.managed_uv.resolve_uv", lambda: "uv") + calls = [] + + def fake_run(cmd, *args, **kwargs): + calls.append(cmd) + if cmd[:3] == ["uv", "pip", "install"]: + return subprocess.CompletedProcess( + cmd, 1, "", "release excluded by exclude-newer" + ) + pytest.fail(f"unexpected pip fallback: {cmd}") + + monkeypatch.setattr(ld.subprocess, "run", fake_run) + result = ld._venv_pip_install(("fresh-package==1.0.0",)) + assert not result.success + assert "exclude-newer" in result.stderr + assert len(calls) == 1 + @pytest.mark.skipif( os.environ.get("HERMES_RUN_NETWORK_TESTS") != "1", diff --git a/tests/tools/test_read_extract.py b/tests/tools/test_read_extract.py index 1c4b4fe3f405d..6186d03e2934a 100644 --- a/tests/tools/test_read_extract.py +++ b/tests/tools/test_read_extract.py @@ -10,6 +10,7 @@ omission. Run with: python -m pytest tests/tools/test_read_extract.py -v """ +import base64 import json import os import tempfile @@ -237,8 +238,11 @@ class TestAnydocInitLifecycle(unittest.TestCase): self._saved_retry = read_extract.ANYDOC_RETRY_SECONDS read_extract._anydoc_module = read_extract._ANYDOC_UNSET read_extract._anydoc_failed_at = None + self._ensure = mock.patch("tools.lazy_deps.ensure", return_value=None) + self._ensure.start() def tearDown(self): + self._ensure.stop() self.rex._anydoc_module = self._saved_module self.rex._anydoc_failed_at = self._saved_failed_at self.rex.ANYDOC_RETRY_SECONDS = self._saved_retry @@ -256,6 +260,13 @@ class TestAnydocInitLifecycle(unittest.TestCase): self.assertIs(self.rex._anydoc(), fake) self.assertEqual(calls, ["anydoc"]) + def test_failed_reconciliation_does_not_import_unverified_binding(self): + with mock.patch( + "tools.lazy_deps.ensure", side_effect=RuntimeError("wrong version") + ), mock.patch("importlib.import_module") as import_module: + self.assertIsNone(self.rex._anydoc()) + import_module.assert_not_called() + def test_failed_load_is_retried_after_cooldown(self): fake = object() calls = [] @@ -484,6 +495,53 @@ class TestReadFileToolIntegration(unittest.TestCase): self.assertTrue(res.get("extracted_document")) self.assertIn("Report body", res["content"]) + def test_backend_only_anydoc_path_uses_transferred_bytes(self): + from tools import file_tools, read_extract + from tools.file_operations import ReadResult + + payload = br"{\rtf1\ansi Remote body\par}" + + class FakeAnydoc: + def to_markdown_bytes(self, data): + self.seen = data + return "Remote body\n" + + class FakeFileOps: + def read_file_bytes(self, path, max_bytes=None): + self.path = path + return ReadResult( + base64_content=base64.b64encode(payload).decode("ascii"), + file_size=len(payload), + is_binary=True, + ) + + @staticmethod + def _add_line_numbers(content, start_line=1): + return "\n".join( + f"{number}|{line}" + for number, line in enumerate(content.split("\n"), start_line) + ) + + fake_anydoc = FakeAnydoc() + fake_ops = FakeFileOps() + saved_module = read_extract._anydoc_module + read_extract._anydoc_module = fake_anydoc + try: + with mock.patch.object(file_tools, "_get_file_ops", return_value=fake_ops), \ + mock.patch.object( + file_tools, + "_resolve_path_for_task", + return_value=file_tools.PurePosixPath("/workspace/remote.rtf"), + ), mock.patch("os.path.getsize", side_effect=AssertionError("host read")): + res = json.loads(read_file_tool("/workspace/remote.rtf", task_id="remote")) + finally: + read_extract._anydoc_module = saved_module + + self.assertTrue(res.get("extracted_document")) + self.assertIn("Remote body", res["content"]) + self.assertEqual(fake_anydoc.seen, payload) + self.assertEqual(fake_ops.path, "/workspace/remote.rtf") + # --------------------------------------------------------------------------- # Scanned-PDF coverage warning diff --git a/tools/file_operations.py b/tools/file_operations.py index 2c1d1a62c9675..6e3ede2ee945d 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -25,6 +25,7 @@ Usage: result = file_ops.search("TODO", path=".", file_glob="*.py") """ +import base64 import os import re import difflib @@ -464,6 +465,10 @@ class FileOperations(ABC): """ ... + def read_file_bytes(self, path: str, max_bytes: Optional[int] = None) -> ReadResult: + """Read complete binary content as base64 across the backend boundary.""" + return ReadResult(error="Binary reads are not implemented for this backend") + @abstractmethod def write_file(self, path: str, content: str, pre_content: Optional[str] = None) -> WriteResult: @@ -1328,6 +1333,38 @@ class ShellFileOperations(FileOperations): file_size=file_size, ) + def read_file_bytes(self, path: str, max_bytes: Optional[int] = None) -> ReadResult: + """Read binary-safe bytes from any shell-backed environment.""" + path = self._expand_path(path) + stat_result = self._exec( + f"wc -c < {self._escape_shell_arg(path)} 2>/dev/null" + ) + if stat_result.exit_code != 0: + return ReadResult(error=f"File not found: {path}") + try: + file_size = int(_strip_terminal_fence_leaks(stat_result.stdout).strip()) + except ValueError: + return ReadResult(error=f"Could not determine file size: {path}") + if max_bytes is not None and file_size > max_bytes: + return ReadResult( + file_size=file_size, + error=f"File is too large ({file_size:,} bytes, limit is {max_bytes:,})", + ) + + encoded = self._exec(f"base64 < {self._escape_shell_arg(path)}") + if encoded.exit_code != 0: + return ReadResult(error=f"Failed to read binary file: {encoded.stdout}") + compact = "".join(_strip_terminal_fence_leaks(encoded.stdout).split()) + try: + base64.b64decode(compact, validate=True) + except (ValueError, base64.binascii.Error): + return ReadResult(error=f"Backend returned invalid binary data for: {path}") + return ReadResult( + base64_content=compact, + file_size=file_size, + is_binary=True, + ) + def delete_file(self, path: str) -> WriteResult: """Delete a single file. diff --git a/tools/file_tools.py b/tools/file_tools.py index 65a5c57990b44..146a2626f92a3 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 """File Tools Module - LLM agent file manipulation tools.""" +import base64 import errno import json import logging @@ -1533,15 +1534,30 @@ def read_file_tool(path: str, offset: int = 1, limit: int = 2000, task_id: str = # ── Structured-document extraction ──────────────────────────── # Try before the binary-extension guard so .docx/.xlsx can render as text. # Malformed documents fall through to the normal path/binary guard. - from tools.read_extract import ExtractionError, extract_document_text, is_extractable_document + from tools.read_extract import ( + MAX_DOCUMENT_BYTES, + ExtractionError, + extract_document_bytes, + is_extractable_document, + ) if is_extractable_document(str(_resolved)): + file_ops = _get_file_ops(task_id) try: - extracted_text = extract_document_text(str(_resolved)) - except ExtractionError: + binary = file_ops.read_file_bytes( + str(_resolved), max_bytes=MAX_DOCUMENT_BYTES + ) + if binary.error or binary.base64_content is None: + raise ExtractionError(binary.error or "Document bytes unavailable") + document_bytes = base64.b64decode( + binary.base64_content, validate=True + ) + extracted_text = extract_document_bytes( + document_bytes, str(_resolved) + ) + except (ExtractionError, ValueError, base64.binascii.Error): logger.debug("document extraction failed for %s", path, exc_info=True) else: - file_ops = _get_file_ops(task_id) lines = extracted_text.splitlines() total_lines = len(lines) end_line = offset + limit - 1 @@ -1549,7 +1565,7 @@ def read_file_tool(path: str, offset: int = 1, limit: int = 2000, task_id: str = result_dict = { "content": file_ops._add_line_numbers(page_text, offset) if page_text else "", "total_lines": total_lines, - "file_size": os.path.getsize(_resolved), + "file_size": binary.file_size, "truncated": total_lines > end_line, "extracted_document": True, } diff --git a/tools/lazy_deps.py b/tools/lazy_deps.py index a7f20b748e274..7ccfc01d22393 100644 --- a/tools/lazy_deps.py +++ b/tools/lazy_deps.py @@ -766,7 +766,17 @@ def _venv_pip_install(specs: tuple[str, ...], *, timeout: int = 300) -> _Install _activate_target_on_syspath(target) return _InstallResult(True, r.stdout or "", r.stderr or "") logger.debug("uv pip install failed: %s", r.stderr) - except (subprocess.TimeoutExpired, FileNotFoundError) as e: + # A resolver failure is authoritative. Falling through to pip + # here would silently discard uv policy such as exclude-newer + # and could install a release that the project quarantined. + return _InstallResult(False, r.stdout or "", r.stderr or "") + except subprocess.TimeoutExpired as e: + logger.debug("uv invocation failed: %s", e) + return _InstallResult(False, "", f"uv pip install timed out: {e}") + except FileNotFoundError as e: + # The resolved uv path disappeared between lookup and spawn. + # In that narrow availability failure, the pip tier remains a + # valid fallback because uv never evaluated the requirements. logger.debug("uv invocation failed: %s", e) # Tier 2: python -m pip (with ensurepip bootstrap if needed) diff --git a/tools/read_extract.py b/tools/read_extract.py index 8f55528905233..6c2f299379822 100644 --- a/tools/read_extract.py +++ b/tools/read_extract.py @@ -18,6 +18,7 @@ import os import posixpath import shutil import subprocess +import tempfile import threading import time import zipfile @@ -25,7 +26,13 @@ from pathlib import Path from typing import Any, Optional from xml.etree import ElementTree as ET -__all__ = ["EXTRACTABLE_EXTENSIONS", "ExtractionError", "extract_document_text", "is_extractable_document"] +__all__ = [ + "EXTRACTABLE_EXTENSIONS", + "ExtractionError", + "extract_document_bytes", + "extract_document_text", + "is_extractable_document", +] EXTRACTABLE_EXTENSIONS = frozenset({".ipynb", ".docx", ".xlsx"}) # Formats handled only when the optional anydoc converter is installed. @@ -41,6 +48,7 @@ MAX_XLSX_BYTES = 50 * 1024 * 1024 # Rust core with no streaming, and the read_file char budget only applies # after conversion, so an unbounded input can pin a tool turn and spike RAM. MAX_ANYDOC_BYTES = 50 * 1024 * 1024 +MAX_DOCUMENT_BYTES = 50 * 1024 * 1024 _MAX_XLSX_ROWS_PER_SHEET = 5000 _MAX_XLSX_COLS = 256 @@ -97,12 +105,14 @@ def _anydoc() -> Optional[Any]: # prompt=False: read_file must never block on an install prompt. _lazy_ensure("tool.doc_extract", prompt=False) except Exception: - pass # lazy install unavailable — fall through to a plain import + _anydoc_failed_at = time.monotonic() + return None try: _anydoc_module = importlib.import_module("anydoc") except Exception: # ImportError or a broken native binding _anydoc_failed_at = time.monotonic() return None + _anydoc_failed_at = None return _anydoc_module # type: ignore[return-value] @@ -123,6 +133,34 @@ def extract_document_text(path: str) -> str: raise ExtractionError(f"Unsupported document type: {path!r}") +def extract_document_bytes(data: bytes, path: str) -> str: + """Extract a document already fetched across a file backend boundary.""" + if len(data) > MAX_DOCUMENT_BYTES: + raise ExtractionError( + f"Document too large to convert ({len(data):,} bytes, limit is {MAX_DOCUMENT_BYTES:,})" + ) + ext = _extension(path) + if ext in ANYDOC_EXTENSIONS: + return _extract_anydoc_bytes(data, path) + if ext not in EXTRACTABLE_EXTENSIONS: + raise ExtractionError(f"Unsupported document type: {path!r}") + + # The stdlib extractors are path-oriented. Materialize backend bytes in a + # private host temp file, then remove it even when parsing fails. + temp_path = "" + try: + with tempfile.NamedTemporaryFile(suffix=ext, delete=False) as fh: + fh.write(data) + temp_path = fh.name + return extract_document_text(temp_path) + finally: + if temp_path: + try: + os.unlink(temp_path) + except OSError: + pass + + def _extract_anydoc(path: str) -> str: mod = _anydoc() if mod is None: @@ -212,8 +250,13 @@ def _page_ranges(pages: list[int]) -> str: return ", ".join(parts) -def _pdf_coverage_note(path: str) -> str: - """A warning footer when many PDF pages produced no text, else ''.""" +def _pdf_coverage_note(path: str, display_path: Optional[str] = None) -> str: + """A warning header when many PDF pages produced no text, else ''. + + ``path`` is the file scanned with pdftotext (may be a host temp file + for backend-transferred bytes); ``display_path`` is the path shown in + the recovery command — the one the agent's terminal can actually see. + """ counts = _pdf_page_char_counts(path) if not counts or len(counts) < 2: return "" @@ -226,6 +269,7 @@ def _pdf_coverage_note(path: str) -> str: and len(empty) < PDF_COVERAGE_ABSOLUTE_EMPTY ): return "" + shown = display_path or path return ( "[EXTRACTION COVERAGE WARNING: " f"{len(empty)} of {total} pages in this PDF yielded no text " @@ -233,12 +277,59 @@ def _pdf_coverage_note(path: str) -> str: "images (or blank) — their content is MISSING from the extracted " "text below, even where section headers appear with empty bodies. " "To read them: render pages to images with " - f"`pdftoppm -jpeg -r 150 -f -l '{path}' /tmp/page` " + f"`pdftoppm -jpeg -r 150 -f -l '{shown}' /tmp/page` " "and inspect each image with the vision_analyze tool, or use the " "ocr-and-documents skill (marker-pdf) for bulk OCR.]\n" ) +def _extract_anydoc_bytes(data: bytes, path: str) -> str: + mod = _anydoc() + if mod is None: + raise ExtractionError(f"Unsupported document type: {path!r}") + if len(data) > MAX_ANYDOC_BYTES: + raise ExtractionError( + f"Document too large to convert ({len(data):,} bytes, limit is {MAX_ANYDOC_BYTES:,})" + ) + try: + text = mod.to_markdown_bytes(data) + except Exception as exc: + raise ExtractionError(f"{type(exc).__name__}: {exc}") from exc + if not isinstance(text, str) or not text.strip(): + raise ExtractionError("Document contains no extractable text") + text = text.rstrip("\n") + "\n" + if Path(path).suffix.lower() == ".pdf": + note = _pdf_coverage_note_from_bytes(data, path) + if note: + # Prepend: read_file paginates the extraction, so a footer on a + # long document would sit on a page the model may never fetch. + text = note + text + return text + + +def _pdf_coverage_note_from_bytes(data: bytes, display_path: str) -> str: + """Coverage note for backend-transferred PDF bytes. + + pdftotext is path-oriented, so materialize the bytes in a private host + temp file for the scan; the recovery command still names + ``display_path`` — the path the agent's terminal backend can see. + """ + temp_path = "" + try: + with tempfile.NamedTemporaryFile(suffix=".pdf", delete=False) as fh: + fh.write(data) + temp_path = fh.name + return _pdf_coverage_note(temp_path, display_path=display_path) + except OSError: + return "" + finally: + if temp_path: + try: + os.unlink(temp_path) + except OSError: + pass + + def _source_text(source) -> str: if isinstance(source, str): return source