diff --git a/Dockerfile b/Dockerfile index 220c39c6..645e8de1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -4,7 +4,9 @@ WORKDIR /app # Install dependencies first for better layer caching COPY pyproject.toml . -RUN pip install --no-cache-dir . +# .[pdf] — analyze_pdf needs PyMuPDF; without it the catalog gate hides the +# tool, so an official image would ship without a capability it advertises. +RUN pip install --no-cache-dir ".[pdf]" # Copy application source COPY src/ src/ diff --git a/packaging/postinstall.sh b/packaging/postinstall.sh index 205cef46..18c04ff4 100755 --- a/packaging/postinstall.sh +++ b/packaging/postinstall.sh @@ -85,8 +85,11 @@ fi # Install Python dependencies from pyproject.toml echo " Installing Python dependencies (this can take a few minutes)..." if [ -f "$APP_DIR/pyproject.toml" ]; then - "$APP_DIR/.venv/bin/pip" install --quiet "$APP_DIR" 2>/dev/null || \ - echo " Warning: pip install failed — run '$APP_DIR/.venv/bin/pip install $APP_DIR' manually" + # [pdf] installs PyMuPDF so the advertised analyze_pdf tool actually works. + # Without it the tool is hidden by the catalog gate, so an official package + # would silently ship without a capability it documents. + "$APP_DIR/.venv/bin/pip" install --quiet "$APP_DIR[pdf]" 2>/dev/null || \ + echo " Warning: pip install failed — run '$APP_DIR/.venv/bin/pip install $APP_DIR[pdf]' manually" fi # Install Playwright browsers for native browser support (optional feature) diff --git a/scripts/incus-deploy.sh b/scripts/incus-deploy.sh index 999c3b7b..17a355bb 100755 --- a/scripts/incus-deploy.sh +++ b/scripts/incus-deploy.sh @@ -107,7 +107,8 @@ done # Install Python dependencies echo "Installing Python dependencies..." incus exec "$INSTANCE" -- bash -c " - cd /app && pip install --no-cache-dir --break-system-packages . > /dev/null 2>&1 + # .[pdf] — see packaging/postinstall.sh: analyze_pdf needs PyMuPDF. + cd /app && pip install --no-cache-dir --break-system-packages '.[pdf]' > /dev/null 2>&1 " # Set ownership diff --git a/src/discord/tool_catalog.py b/src/discord/tool_catalog.py index 01e23315..e4aa85fb 100644 --- a/src/discord/tool_catalog.py +++ b/src/discord/tool_catalog.py @@ -12,10 +12,14 @@ from __future__ import annotations +import importlib.util from collections.abc import Callable +from ..odin_log import get_logger from ..tools import get_tool_definitions +log = get_logger("tools") + class ToolCatalog: def __init__(self, *, get_config: Callable, skill_manager) -> None: @@ -54,6 +58,20 @@ def merged_definitions(self) -> list[dict]: if not image_tool_available(config): builtin = [t for t in builtin if t["name"] != "generate_image"] + # analyze_pdf: PyMuPDF lives in the optional `pdf` extra, and no + # install path used to install extras — so the tool was advertised on + # every install while its dependency was present on none of them, and + # calls died with "No module named 'fitz'". Structural availability + # only; the handler still converts a load failure into a clean result, + # because find_spec proves the module is importable, not that the + # native library loads. + if importlib.util.find_spec("fitz") is None: + builtin = [t for t in builtin if t["name"] != "analyze_pdf"] + log.info( + "analyze_pdf hidden from the tool catalog: PyMuPDF is not " + "installed. Install the 'pdf' extra to enable it " + "(pip install '.[pdf]')." + ) # Per-spawn agent model/effort catalogue: expose each axis's field + # clause on spawn_agent/spawn_loop_agents only when that agent config # axis is "auto" (operates on clones — never mutates the shared defs). diff --git a/src/tools/handlers/files_docs.py b/src/tools/handlers/files_docs.py index 29cc435d..ce12e922 100644 --- a/src/tools/handlers/files_docs.py +++ b/src/tools/handlers/files_docs.py @@ -123,7 +123,18 @@ async def _handle_analyze_pdf(self, inp: dict) -> str: if is_url_blocked(url): return "Error: blocked URL (localhost / private IP / cloud-metadata address)." - import fitz + # Structural gating hides this tool when PyMuPDF is missing, but the + # handler must still degrade cleanly: find_spec proves the module is + # importable, not that its native library loads, and a direct call can + # reach here on an install whose catalog was built elsewhere. + try: + import fitz + except Exception as exc: + return ( + "PDF support unavailable: PyMuPDF could not be loaded " + f"({type(exc).__name__}: {exc}). Install the 'pdf' extra " + "(pip install '.[pdf]') and restart Odin." + ) pdf_bytes: bytes | None = None diff --git a/src/tools/http_probe_ops.py b/src/tools/http_probe_ops.py index d2594f02..e2ddd829 100644 --- a/src/tools/http_probe_ops.py +++ b/src/tools/http_probe_ops.py @@ -108,17 +108,37 @@ def build_http_probe_command(params: dict) -> str: f"Invalid HTTP method: {method}. Allowed: {', '.join(sorted(ALLOWED_METHODS))}" ) + # HEAD needs curl's native no-body mode, not a method override. `-X HEAD` + # sends the HEAD token but leaves libcurl expecting a response body, so it + # blocks until the timeout and exits 18 ("transfer closed with N bytes + # remaining"): measured 5.1s/exit-18 versus 0.065s/exit-0 for `-I` against + # a healthy server. HEAD is the ONLY affected method — POST/PUT/PATCH/ + # DELETE/OPTIONS may legitimately return zero-length bodies and curl frames + # those normally (verified: `-X OPTIONS` exits 0 in 0.067s). + is_head = method == "HEAD" + + # A request body on HEAD is rejected rather than silently dropped: data + # flags combined with -I make curl's method selection ambiguous, and HEAD + # request-body semantics are not worth preserving here. + if is_head and params.get("body"): + raise ValueError("HTTP method HEAD does not accept a request body") + parts = ["curl", "-sS"] # Timing output format parts.append(f"-w {_sq(_TIMING_FORMAT)}") - # Include response headers in output - parts.append("-i") - - # HTTP method - if method != "GET": - parts.append(f"-X {method}") + if is_head: + # -I already routes response headers to output; adding -i as well is + # redundant and makes the output contract depend on how a given curl + # version coalesces the two. + parts.append("-I") + else: + # Include response headers in output + parts.append("-i") + # HTTP method + if method != "GET": + parts.append(f"-X {method}") # Timeout timeout = _clamp_int(params.get("timeout"), DEFAULT_TIMEOUT, 1, MAX_TIMEOUT) diff --git a/tests/characterization/test_tool_parity.py b/tests/characterization/test_tool_parity.py index 9a579620..901d0bcb 100644 --- a/tests/characterization/test_tool_parity.py +++ b/tests/characterization/test_tool_parity.py @@ -182,6 +182,20 @@ class TestBackendGatedVisibility: GATED = {"claude_code", "email_send", "email_search", "email_read", "email_list_recent", "issue_tracker", "generate_image"} + @staticmethod + def _dependency_gated() -> set[str]: + """Tools gated by an installed DEPENDENCY rather than by config. + + analyze_pdf needs PyMuPDF, which lives in the optional `pdf` extra, so + whether it is visible depends on the environment rather than the Config + under test. Computing this instead of hardcoding keeps the arithmetic + below true on a machine with the extra AND on one without — a fixed + constant would pass locally and fail in CI, or vice versa. + """ + import importlib.util + + return set() if importlib.util.find_spec("fitz") else {"analyze_pdf"} + def _catalog_names(self, **config_kwargs) -> set[str]: from src.config.schema import Config from src.discord.client import OdinBot @@ -196,8 +210,25 @@ def test_unconfigured_backends_are_invisible(self): names = self._catalog_names() leaked = self.GATED & names assert not leaked, f"backend-gated tools visible without config: {sorted(leaked)}" - # exact arithmetic: full registry minus the six gated tools - assert len(names) == len(EXPECTED_TOOL_ORDER) - len(self.GATED) + dependency_gated = self._dependency_gated() + assert not (dependency_gated & names), ( + f"tools with a missing dependency are advertised: {sorted(dependency_gated & names)}" + ) + # exact arithmetic: full registry minus config-gated minus + # dependency-gated tools + assert len(names) == len(EXPECTED_TOOL_ORDER) - len(self.GATED) - len(dependency_gated) + + def test_analyze_pdf_follows_its_dependency(self): + """analyze_pdf must be advertised exactly when PyMuPDF can be imported. + + It was previously advertised unconditionally while no install path + installed the `pdf` extra, so every call failed with + "No module named 'fitz'" (found in the v3.65.0 smoke test). + """ + import importlib.util + + visible = "analyze_pdf" in self._catalog_names() + assert visible is (importlib.util.find_spec("fitz") is not None) def test_configured_claude_code_is_visible(self): names = self._catalog_names( diff --git a/tests/test_handlers_files_docs.py b/tests/test_handlers_files_docs.py index a4f9c387..1559333b 100644 --- a/tests/test_handlers_files_docs.py +++ b/tests/test_handlers_files_docs.py @@ -224,3 +224,26 @@ async def test_truncation(self): out = await _tools(exec_ret=(0, b64))._handle_analyze_pdf( {"host": "s", "path": "/p"}) assert "truncated" in out + + +async def test_analyze_pdf_degrades_cleanly_without_pymupdf(monkeypatch): + """find_spec proves the module is importable, not that its native library + loads — and a direct call can reach the handler on an install whose catalog + was built elsewhere. Either way the caller gets a clean, actionable result + rather than a raw ImportError (v3.65.0 smoke test: "No module named 'fitz'"). + """ + import builtins + + real_import = builtins.__import__ + + def _no_fitz(name, *args, **kwargs): + if name == "fitz": + raise ImportError("No module named 'fitz'") + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _no_fitz) + + tools = _tools() + result = await tools._handle_analyze_pdf({"host": "localhost", "path": "/tmp/x.pdf"}) + assert "PDF support unavailable" in result + assert "pdf" in result and "install" in result.lower(), "must name the remedy" diff --git a/tests/test_http_probe_ops.py b/tests/test_http_probe_ops.py index 6990427c..8a1e1cc6 100644 --- a/tests/test_http_probe_ops.py +++ b/tests/test_http_probe_ops.py @@ -222,9 +222,85 @@ def test_patch(self): cmd = build_http_probe_command({"url": "https://example.com", "method": "PATCH"}) assert "-X PATCH" in cmd - def test_head(self): + def test_head_uses_native_no_body_mode(self): + """HEAD must use curl's -I, never -X HEAD. + + `-X HEAD` sends the HEAD token but leaves libcurl expecting a response + body, so it blocks until the timeout and exits 18 ("transfer closed + with N bytes remaining"). Measured against a healthy server: 5.1s and + exit 18 with -X HEAD, versus 0.065s and exit 0 with -I. This test + previously asserted "-X HEAD" in cmd — it pinned the bug. + """ cmd = build_http_probe_command({"url": "https://example.com", "method": "HEAD"}) - assert "-X HEAD" in cmd + assert "-I" in cmd.split(), cmd + assert "-X HEAD" not in cmd, "the -X override reintroduces the hang" + + def test_head_suppresses_the_header_include_flag(self): + """-I already routes response headers to output; adding -i as well is + redundant and makes the output contract depend on how a given curl + version coalesces the two.""" + cmd = build_http_probe_command({"url": "https://example.com", "method": "HEAD"}) + assert "-i" not in cmd.split(), cmd + + def test_non_head_methods_keep_the_include_flag_and_override(self): + """The fix must stay HEAD-specific: other methods may legitimately + return zero-length bodies and curl frames those normally (verified: + -X OPTIONS exits 0 in 0.067s).""" + for method in ("POST", "PUT", "PATCH", "DELETE", "OPTIONS"): + cmd = build_http_probe_command({"url": "https://example.com", "method": method}) + assert f"-X {method}" in cmd, method + assert "-i" in cmd.split(), method + assert "-I" not in cmd.split(), method + + def test_get_is_unchanged(self): + cmd = build_http_probe_command({"url": "https://example.com", "method": "GET"}) + assert "-i" in cmd.split() + assert "-I" not in cmd.split() + assert "-X" not in cmd.split() + + def test_head_keeps_follow_redirects(self): + """curl -I -L performs HEAD across the redirect chain, matching the + requested method and the existing follow-redirects contract; dropping + -L only for HEAD would make its behaviour inconsistent.""" + cmd = build_http_probe_command({"url": "https://example.com", "method": "HEAD"}) + assert "-L" in cmd.split() + + def test_head_without_follow_redirects_omits_follow_flag(self): + cmd = build_http_probe_command({ + "url": "https://example.com", "method": "HEAD", "follow_redirects": False, + }) + assert "-L" not in cmd.split() + assert "-I" in cmd.split() + + def test_head_keeps_the_timing_trailer_and_its_sentinel(self): + """The timing block is parsed from a distinct sentinel rather than from + body termination, so it survives HEAD's empty body.""" + cmd = build_http_probe_command({"url": "https://example.com", "method": "HEAD"}) + assert "---PROBE-RESULTS---" in cmd + assert "%{http_code}" in cmd + assert "%{time_total}" in cmd + + def test_head_honours_the_timeout(self): + cmd = build_http_probe_command({ + "url": "https://example.com", "method": "HEAD", "timeout": 7, + }) + assert "--max-time 7" in cmd + + def test_head_rejects_a_request_body(self): + """Rejected, not silently dropped: data flags combined with -I make + curl's method selection ambiguous, and HEAD request-body semantics are + not worth preserving.""" + with pytest.raises(ValueError, match="HEAD"): + build_http_probe_command({ + "url": "https://example.com", "method": "HEAD", "body": "x=1", + }) + + def test_head_with_empty_body_is_accepted(self): + """Only a NONEMPTY body is a conflict.""" + cmd = build_http_probe_command({ + "url": "https://example.com", "method": "HEAD", "body": "", + }) + assert "-I" in cmd.split() def test_options(self): cmd = build_http_probe_command({"url": "https://example.com", "method": "OPTIONS"}) @@ -327,13 +403,21 @@ def test_delete_body_ignored(self): }) assert "-d" not in cmd - def test_head_body_ignored(self): - cmd = build_http_probe_command({ - "url": "https://example.com", - "method": "HEAD", - "body": "should not appear", - }) - assert "-d" not in cmd + def test_head_body_rejected(self): + """CONTRACT CHANGE: a body on HEAD used to be silently dropped; it is + now rejected. + + Silently discarding it hid a caller mistake, and data flags combined + with curl's -I make method selection ambiguous. Rejecting is the + deliberate choice (Odin's design call on this fix); the previous + assertion here was that the body simply never reached the command. + """ + with pytest.raises(ValueError, match="HEAD"): + build_http_probe_command({ + "url": "https://example.com", + "method": "HEAD", + "body": "should not appear", + }) def test_empty_body_not_added(self): cmd = build_http_probe_command({