fix(tools): preserve document extraction boundaries
This commit is contained in:
parent
fb4664f79d
commit
8de3ddb9ef
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 <first> -l <last> '{path}' /tmp/page` "
|
||||
f"`pdftoppm -jpeg -r 150 -f <first> -l <last> '{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
|
||||
|
|
|
|||
Loading…
Reference in New Issue