refactor(tools): group H pass 3 — dispatch table for V4A apply, docstring/comment compaction keeping every WHY

tools/osv_check.py

@@ -20,12 +20,11 @@ logger = logging.getLogger(__name__)
 _OSV_ENDPOINT = os.getenv("OSV_ENDPOINT", "https://api.osv.dev/v1/query")
 _TIMEOUT = 10  # seconds
 
-# Result cache: (ecosystem, package, version) -> (expiry_monotonic, result).
-# MCP reconnect ladders and parked-server self-probes re-run the preflight for
-# the SAME package on every spawn; uncached, a flapping server becomes a
-# sustained OSV/DNS query stream. Advisories don't flip on second timescales,
-# so clean AND blocked verdicts are reusable. Network failures are NOT cached:
-# fail-open covers them and caching one could mask a real advisory later.
+# Result cache: (ecosystem, package, version) -> (expiry_monotonic, result). Reconnect
+# ladders and parked-server self-probes re-run the preflight for the SAME package on every
+# spawn; uncached, a flapping server becomes a sustained OSV/DNS query stream. Clean AND
+# blocked verdicts are reusable; network failures are NOT cached (fail-open covers them and
+# caching one could mask a real advisory later).
 _CACHE_TTL_S = float(os.getenv("OSV_CHECK_CACHE_TTL", "3600"))
 _CACHE_MAX_ENTRIES = 256
 _cache: dict = {}
@@ -55,10 +54,7 @@ def _cache_put(key, result: Optional[str]) -> None:
 
 def check_package_for_malware(command: str, args: list) -> Optional[str]:
     """Check an MCP server package (inferred from ``command``/``args``) for MAL-* advisories.
-
-    Returns a BLOCKED message when malware is found, else None — including on network
-    errors and unrecognized commands (fail-open).
-    """
+    Returns a BLOCKED message, else None — also on network errors/unknown commands (fail-open)."""
     ecosystem = _infer_ecosystem(command)
     if not ecosystem:
         return None  # not npx/uvx — skip
@@ -99,9 +95,8 @@ def _infer_ecosystem(command: str) -> Optional[str]:
 
 def _parse_package_from_args(args: list, ecosystem: str) -> Tuple[Optional[str], Optional[str]]:
     """Extract (package_name, version) from command args, or (None, None) if not parseable."""
-    # Skip flags to find the package token. Honor npx's explicit install target
-    # (--package=NAME / --package NAME / -p NAME), which names a package distinct
-    # from the executed binary; otherwise the first bare positional is used.
+    # Skip flags to find the package token. npx's explicit install target (--package=NAME /
+    # --package NAME / -p NAME) names a package distinct from the executed binary.
     package_token = None
     take_next = False
     for arg in args or ():

tools/patch_parser.py

@@ -1,17 +1,9 @@
 #!/usr/bin/env python3
 """V4A patch format parser and applier (format used by codex, cline, etc.).
 
-    *** Begin Patch
-    *** Update File: path/to/file.py
-    @@ optional context hint @@
-     context line (space prefix)
-    -removed line
-    +added line
-    *** Add File: path/to/new.py
-    +new file content
-    *** Delete File: path/to/old.py
-    *** Move File: old/path.py -> new/path.py
-    *** End Patch
+    *** Begin Patch / *** End Patch wrap the operations:
+    *** Update File: p.py   then hunks: ``@@ hint @@``, `` ctx``, ``-old``, ``+new``
+    *** Add File: n.py      then ``+`` lines;  *** Delete File: o.py;  *** Move File: a -> b
 
     operations, error = parse_v4a_patch(patch_content)
     result = apply_v4a_operations(operations, file_ops)
@@ -175,10 +167,10 @@ def _validate_operations(operations: List[PatchOperation], file_ops: Any) -> Lis
     errors: List[str] = []
     real_change_count = 0
 
-    # Virtual overlay so inter-op state validates (e.g. a MOVE creating the
-    # destination a later UPDATE targets). UPDATE/MOVE reads consult it first.
-    pending_content: dict = {}   # path -> content produced by an earlier op
-    removed_paths: set = set()   # paths a MOVE/DELETE has taken away
+    # Virtual overlay so inter-op state validates (e.g. a MOVE creating the destination
+    # a later UPDATE targets): path -> content from an earlier op; paths MOVE/DELETE removed.
+    pending_content: dict = {}
+    removed_paths: set = set()
 
     def _read(path: str):
         if path in removed_paths and path not in pending_content:
@@ -283,9 +275,7 @@ def apply_v4a_operations(operations: List[PatchOperation], file_ops: Any) -> 'Pa
             error="Patch validation failed (no files were modified):\n"
                   + "\n".join(f"  • {e}" for e in validation_errors))
 
-    files_modified: List[str] = []
-    files_created: List[str] = []
-    files_deleted: List[str] = []
+    files: Dict[str, List[str]] = {"created": [], "deleted": [], "modified": []}
     all_diffs: List[str] = []
     # V4A bypasses the WriteResult/PatchResult plumbing that write_file uses,
     # so LSP diagnostics and lint must be propagated explicitly per file.
@@ -293,15 +283,9 @@ def apply_v4a_operations(operations: List[PatchOperation], file_ops: Any) -> 'Pa
     errors: List[str] = []
     lint_results: Dict[str, dict] = {}
 
-    dispatch: Dict[OperationType, Tuple[Callable[[PatchOperation, Any], ApplyResult], List[str], str]] = {
-        OperationType.ADD: (_apply_add, files_created, "add"),
-        OperationType.DELETE: (_apply_delete, files_deleted, "delete"),
-        OperationType.MOVE: (_apply_move, files_modified, "move"),
-        OperationType.UPDATE: (_apply_update, files_modified, "update")}
-
     for op in operations:
         try:
-            handler, bucket, verb = dispatch[op.operation]
+            handler, verb, bucket = _APPLY_DISPATCH[op.operation]
             ok, payload, lsp, lint = handler(op, file_ops)
             if not ok:
                 errors.append(f"Failed to {verb} {op.file_path}: {payload}")
@@ -309,7 +293,7 @@ def apply_v4a_operations(operations: List[PatchOperation], file_ops: Any) -> 'Pa
             label = op.file_path
             if op.operation is OperationType.MOVE:
                 label = f"{op.file_path} -> {op.new_path}"
-            bucket.append(label)
+            files[bucket].append(label)
             all_diffs.append(payload)
             if lsp:
                 lsp_blocks.append(lsp)
@@ -322,9 +306,7 @@ def apply_v4a_operations(operations: List[PatchOperation], file_ops: Any) -> 'Pa
     # concatenation keeps per-file attribution.
     result_kwargs = dict(
         diff='\n'.join(all_diffs),
-        files_modified=files_modified,
-        files_created=files_created,
-        files_deleted=files_deleted,
+        files_modified=files["modified"], files_created=files["created"], files_deleted=files["deleted"],
         lint=lint_results if lint_results else None,
         lsp_diagnostics="\n\n".join(lsp_blocks) if lsp_blocks else None)
     if errors:
@@ -454,3 +436,12 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> ApplyResult:
         current_content.splitlines(keepends=True), new_content.splitlines(keepends=True),
         fromfile=f"a/{op.file_path}", tofile=f"b/{op.file_path}"))
     return True, diff, getattr(write_result, "lsp_diagnostics", None), getattr(write_result, "lint", None)
+
+
+# operation -> (handler, verb for error text, files_* bucket)
+_APPLY_DISPATCH: Dict[OperationType, Tuple[Callable[[PatchOperation, Any], ApplyResult], str, str]] = {
+    OperationType.ADD: (_apply_add, "add", "created"),
+    OperationType.DELETE: (_apply_delete, "delete", "deleted"),
+    OperationType.MOVE: (_apply_move, "move", "modified"),
+    OperationType.UPDATE: (_apply_update, "update", "modified"),
+}

tools/read_extract.py

@@ -25,20 +25,16 @@ from pathlib import Path
 from typing import Any, Callable, Iterator, Optional
 from xml.etree import ElementTree as ET
 
-__all__ = [
-    "EXTRACTABLE_EXTENSIONS",
-    "ExtractionError",
-    "extract_document_bytes",
-    "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.
 ANYDOC_EXTENSIONS = frozenset({
     ".doc", ".docm", ".ppt", ".pps", ".pot", ".pptx", ".pptm", ".ppsx", ".ppsm",
     ".xls", ".xlsm", ".xlsb", ".odt", ".ods", ".odp", ".rtf", ".epub", ".pdf"})
-# anydoc loads the whole file through its Rust core with no streaming, and the
-# read_file char budget only applies after conversion — cap the input size.
+# anydoc loads whole files with no streaming and the read_file char budget only applies
+# after conversion — cap the input size.
 MAX_ANYDOC_BYTES = 50 * 1024 * 1024
 MAX_DOCUMENT_BYTES = 50 * 1024 * 1024
 _MAX_XLSX_ROWS_PER_SHEET = 5000
@@ -56,9 +52,8 @@ class ExtractionError(Exception):
 
 def _extension(path: str) -> str:
     ext = Path(path).suffix.lower()
-    if ext in EXTRACTABLE_EXTENSIONS or (ext in ANYDOC_EXTENSIONS and _anydoc() is not None):
-        return ext
-    return ""
+    known = ext in EXTRACTABLE_EXTENSIONS or (ext in ANYDOC_EXTENSIONS and _anydoc() is not None)
+    return ext if known else ""
 
 
 _ANYDOC_UNSET = object()
@@ -143,8 +138,8 @@ def extract_document_bytes(data: bytes, path: str) -> str:
 
 
 def _anydoc_missing_error(path: str) -> str:
-    """Teaching error for anydoc-gated formats; the schema deliberately omits this caveat
-    so only sessions that hit one pay for the explanation (and the fix)."""
+    """Teaching error for anydoc-gated formats (deliberately absent from the schema so only
+    sessions that hit one pay for it)."""
     return (
         f"Cannot convert {path!r}: this format needs the optional anydoc "
         "converter, which is not installed (install blocked or first "
@@ -155,28 +150,23 @@ def _anydoc_missing_error(path: str) -> str:
 
 
 def _hosted_ocr_config() -> tuple:
-    """Resolve hosted-OCR settings: (enabled, api_key, api_url). Never raises.
-
-    Maintainer decision: the ONLY route is a direct ``FIRECRAWL_API_KEY`` (anydoc defaults
-    api_url to https://api.firecrawl.dev); the Nous managed gateway is NOT used — its Parse
-    proxy live-probed broken (revisit when it grows Parse support). ``file_tools.hosted_ocr:
-    false`` disables even with a key; true/unset → enabled iff the key is present. Env probe
-    only, no network at schema-build time.
-    """
+    """Resolve hosted-OCR settings: (enabled, api_key, api_url). Never raises; no network.
+    Maintainer decision: the ONLY route is a direct ``FIRECRAWL_API_KEY`` (anydoc defaults the
+    api_url); the Nous gateway is NOT used — its Parse proxy live-probed broken (revisit when
+    it grows Parse support). ``file_tools.hosted_ocr: false`` disables even with a key."""
     api_key = os.environ.get("FIRECRAWL_API_KEY") or None
     enabled = api_key is not None
     with contextlib.suppress(Exception):
         from hermes_cli.config import load_config_readonly
-        cfg = load_config_readonly()
-        section = cfg.get("file_tools") if isinstance(cfg, dict) else None
+        section = load_config_readonly().get("file_tools")
         if isinstance(section, dict) and section.get("hosted_ocr") is False:
             enabled = False
     return enabled, api_key, None
 
 
 def hosted_ocr_available() -> bool:
-    """Public probe for read_file's schema line (same gate as :func:`_hosted_ocr_config`);
-    a key that fails at conversion time lands in the NEEDS-OCR warning instead."""
+    """Public probe for read_file's schema line; a key failing at conversion time lands in
+    the NEEDS-OCR warning instead."""
     return _hosted_ocr_config()[0]
 
 
@@ -202,16 +192,12 @@ def _finalize_anydoc_text(text: Any, path: str, pdf_note: Callable[[], str]) ->
     paginates: a footer may never be fetched). Covers PARTIAL gaps without NeedsOcrError."""
     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_note()
-        if note:
-            text = note + text
-    return text
+    note = pdf_note() if Path(path).suffix.lower() == ".pdf" else ""
+    return (note or "") + text.rstrip("\n") + "\n"
 
 
 def _ocr_scanned_pdf(mod: Any, path: str, exc: BaseException) -> str:
-    """Typed scanned-pages signal (anydoc >= 0.2): try hosted OCR when a Firecrawl route exists, else teach recovery."""
+    """anydoc >= 0.2 scanned-pages signal: hosted OCR when a route exists, else teach recovery."""
     pages = list(getattr(exc, "pages", []) or [])
     enabled, api_key, api_url = _hosted_ocr_config()
     hosted_error = ""
@@ -286,9 +272,8 @@ def _pdf_page_texts(path: str) -> Optional[list[str]]:
             ["pdftotext", path, "-"], capture_output=True, timeout=PDF_PAGE_SCAN_TIMEOUT)
     except (OSError, subprocess.SubprocessError):
         return None
-    if proc.returncode != 0:
-        return None
-    pages = proc.stdout.decode("utf-8", errors="replace").split("\f")
+    out = proc.stdout.decode("utf-8", errors="replace") if proc.returncode == 0 else ""
+    pages = out.split("\f") if out else []
     if pages and not pages[-1].strip():
         pages.pop()  # trailing form-feed artifact
     return pages or None
@@ -351,8 +336,8 @@ def _pdf_coverage_note(path: str, display_path: Optional[str] = None) -> str:
 
 
 def _pdf_coverage_note_from_bytes(data: bytes, display_path: str) -> str:
-    """Coverage note for backend-transferred PDF bytes: pdftotext is path-oriented, so scan a
-    host temp copy; the recovery command still names ``display_path`` (visible to the agent)."""
+    """Coverage note for backend-transferred PDF bytes via a host temp copy (pdftotext is
+    path-oriented); the recovery command still names ``display_path``."""
     try:
         with _temp_copy(data, ".pdf") as temp_path:
             return _pdf_coverage_note(temp_path, display_path=display_path)
@@ -380,8 +365,7 @@ def _base64_bytes(payload: str) -> int:
 
 
 def _clean_stream_text(text: str) -> str:
-    """Strip ANSI escapes and collapse ``\\r`` progress-bar rewrites: Jupyter renders only
-    the final frame of a ``\\r``-redrawn line (tqdm), so keep the text after the last ``\\r``."""
+    """Strip ANSI escapes; keep only the final ``\\r`` frame of each line (tqdm redraws)."""
     from tools.ansi_strip import strip_ansi
     lines = []
     for line in strip_ansi(text).replace("\r\n", "\n").split("\n"):
@@ -390,10 +374,8 @@ def _clean_stream_text(text: str) -> str:
     return "\n".join(lines)
 
 
-# Notebook outputs longer than this are tail-truncated per output block so a
-# single runaway training log cannot flood the extracted text.
+# Per-output-block truncation so one runaway training log cannot flood the extraction.
 _MAX_OUTPUT_CHARS = 20_000
-
 # nbformat v3 stores mime data flat on the output dict under these keys.
 _V3_MIME_KEYS = (("png", "image/png"), ("jpeg", "image/jpeg"), ("svg", "image/svg+xml"), ("html", "text/html"))
 
@@ -507,7 +489,7 @@ def _extract_notebook(path: str) -> str:
 
 @contextlib.contextmanager
 def _open_zip(path: str, kind: str) -> Iterator[zipfile.ZipFile]:
-    """Open an OOXML package; bad-zip/OS failures (also from the body) become ExtractionError."""
+    """Open an OOXML package; bad-zip/OS failures (body included) become ExtractionError."""
     try:
         with zipfile.ZipFile(path) as zf:
             yield zf

tools/read_terminal_tool.py

@@ -14,11 +14,9 @@ from tools.registry import registry, tool_error
 
 
 def read_pane(callback: Optional[Callable], window, errors: tuple) -> str:
-    """Shared body of the read_terminal / read_preview bridges.
-
-    ``window`` is ``((key, value, floor), ...)``; None values are omitted, others are
-    int-coerced and floored. ``errors`` = (not_desktop, not_integers, fail_prefix, empty).
-    """
+    """Shared body of the read_terminal / read_preview / read_window bridges. ``window`` is
+    ``((key, value, floor), ...)`` (None omitted, else int-coerced and floored); ``errors`` =
+    (not_desktop, not_integers, fail_prefix, empty)."""
     if callback is None:
         return tool_error(errors[0])
     try:

tools/shell_heredoc.py

@@ -1,18 +1,11 @@
-"""Conservative heredoc masking for shell-command scanners.
-
-Guards that scan raw command text (the background-'&' guard in ``tools/terminal_tool.py``,
-blocked-command checks, ``cron/lifecycle_guard``) false-positive on heredoc *bodies*, which
-are usually inline data. Naively stripping every body is unsafe the other way (fake ``<<``
-in quotes can swallow a real operator; unquoted bodies expand; ``bash <<'EOF'`` executes).
-
-A body is masked ONLY when: every delimiter on the opener is quoted (no expansion); every
-heredoc is terminated by an exact delimiter line; the opener is a single command (no
-``;``/``|``/``&`` and no ``$(...)``, backtick or process substitution); and the consumer is
-an allowlisted non-shell interpreter (``_INERT_HEREDOC_CONSUMER_RE``). Otherwise the command
-is returned untouched: a false positive is acceptable, hiding real shell syntax from a guard
-is not. Masked bodies become an equal number of newlines so ``re.MULTILINE`` scanning keeps
-its line structure. Adapted from Wolfram Ravenwolf's security-hardened rework of PR #63788.
-"""
+"""Conservative heredoc masking for shell-command scanners (terminal '&' guard, blocked-command
+checks, cron lifecycle_guard) that false-positive on heredoc *bodies*. Stripping every body is
+unsafe the other way (a fake ``<<`` in quotes can swallow a real operator; unquoted bodies
+expand; ``bash <<'EOF'`` executes), so a body is masked ONLY when every delimiter is quoted,
+every heredoc has an exact terminator line, the opener is a single command (no ``;|&``,
+``$(...)``, backticks or process substitution) and the consumer is an allowlisted non-shell
+interpreter. Otherwise the command is returned untouched: a false positive is acceptable,
+hiding shell syntax from a guard is not. Masked bodies keep their newline count (re.MULTILINE)."""
 
 from __future__ import annotations
 
@@ -22,10 +15,7 @@ import re
 # THAT interpreter. Optional VAR=... assignments, ``env`` and a path prefix are
 # allowed. Deliberately narrow: anything unmatched keeps its body visible.
 _INERT_HEREDOC_CONSUMER_RE = re.compile(
-    r"^\s*"
-    r"(?:[A-Z_][A-Z0-9_]*=\S+\s+)*"
-    r"(?:env\s+)?"
-    r"(?:[A-Za-z0-9_./-]+/)?"
+    r"^\s*(?:[A-Z_][A-Z0-9_]*=\S+\s+)*(?:env\s+)?(?:[A-Za-z0-9_./-]+/)?"
     r"(?:python(?:3(?:\.\d+)*)?|osascript|cat)(?=\s|$)",
     re.IGNORECASE)
 
@@ -133,11 +123,8 @@ def _parse_heredoc_operator(command: str, index: int):
 
 
 def _scan_heredoc_command_unit(command: str, start: int):
-    """Scan one logical command -> ``(end, specs, unknown_operator, has_list_operator)``.
-
-    ``unknown_operator``: an unparseable ``<<`` (caller must fail closed).
-    ``has_list_operator``: unquoted ``;``/``|``/``&`` on the opener.
-    """
+    """Scan one logical command -> ``(end, specs, unknown_operator, has_list_operator)``:
+    an unparseable ``<<`` (caller must fail closed) / unquoted ``;|&`` on the opener."""
     cursor = start
     quote = None
     comment = False