Merge pull request #26 from o3LL/fix/tmp-realpath-test-bugs

fix(tests): resolve temp paths consistently on macOS

tests/KNOWN_FAILURES.md

@@ -0,0 +1,133 @@
+# Known full-suite failures
+
+`python -m pytest -q` does not come back clean on every machine, and it never
+has. Without a list of which failures are expected, a first local run is
+uninterpretable: you cannot tell "you broke something" from "you are on a Mac",
+so the usual result is either chasing a non-bug or ignoring a real one.
+
+This is that list. It is a record of observation, not a permission slip: a test
+here is still a test that does not pass, and the three that remain below are
+all still worth someone's time.
+
+Last measured: `lab @ c499c01b` plus the fixes in this change, macOS 15 on
+Apple Silicon, Python 3.11, with the **default** `$TMPDIR` — see the socket
+entry below for why that qualifier is load-bearing.
+
+```
+3 failed, 10658 passed, 6 skipped
+```
+
+## Get the prerequisites right first
+
+Most "surprise" failures are a missing dependency rather than anything in this
+file. A clean run needs all of:
+
+```bash
+python3.11 -m venv venv
+./venv/bin/python -m pip install -r requirements.txt
+npm ci                              # the browser tests shell out to node
+npx playwright install chromium     # ~30 tests drive a real browser
+mkdir -p data                       # SQLite lives at ./data/app.db
+```
+
+plus `ffmpeg` on `PATH` for the media tests.
+
+If you already have a ChromaDB running, point `CHROMADB_PORT` at a closed port
+for the run. The client reaches Chroma over HTTP regardless of the data
+directory, so a test run will otherwise attach to whatever store is listening,
+including one holding real data.
+
+Miss `npm ci` and roughly 36 browser tests fail on `Cannot find package
+'playwright'`. That is not a regression, it is the missing install.
+
+## The three that remain, and the seven that no longer do
+
+### Test bugs: comparing an unresolved path against a resolved one
+
+Three failures compared an unresolved `/tmp` path against a resolved
+`/private/tmp` one, and are fixed rather than listed:
+
+- `tests/test_code_nav_tools.py` (two tests) built a fixture under
+  `tempfile.mkdtemp(dir="/tmp")` and compared it against the path the code
+  reports, which it resolves.
+- `tests/test_workspace_confine.py::test_glob_confined_e2e` mixed
+  `os.path.realpath(ws)` with an unresolved secret directory, so `relpath`
+  produced `../../../../tmp/<absolute path>` and the assertion that the
+  absolute path was absent matched it as a substring.
+
+Both now resolve consistently. They are recorded here because the shape recurs:
+on macOS, mixing a resolved and an unresolved temp path is a test bug that
+looks like a platform failure.
+
+### Test bugs: a temp path too long to bind a socket to
+
+Four more, same family, invisible unless `$TMPDIR` is long enough:
+
+- `tests/test_shell_routes.py::TestHostDockerAccess` (three tests)
+- `tests/test_cookbook_docker_access.py::test_container_opt_in_with_unix_socket_is_allowed`
+
+```
+OSError: AF_UNIX path too long
+```
+
+Each bound an `AF_UNIX` socket at `tmp_path / "docker.sock"`. macOS gives
+`sun_path` 104 bytes including the terminator, and pytest's `tmp_path` is
+rooted at `$TMPDIR`, which on a stock Mac is a 49-character
+`/var/folders/<2>/<30>/T/`. Add `pytest-of-<user>/pytest-<n>/` and the test's
+own name and the bind path is 115 bytes before the filename.
+
+This is why the counts above depend on where you run from: under a shortened
+`$TMPDIR` the path lands at 103 and the tests pass, and it tips over the moment
+pytest's run counter reaches two digits. Linux allows 108 bytes and roots
+`$TMPDIR` at `/tmp`, so it never bites there and CI stays green.
+
+They now bind through `tests/helpers/unix_sockets.bound_unix_socket`, which
+puts the socket under a short directory. **Measure with the default `$TMPDIR`**
+— `env -u TMPDIR` or an explicit `/var/folders/...` — or this whole file
+records a run nobody else has.
+
+### Optional dependency: ffmpeg without a WebP encoder
+
+- `tests/test_inspect_media_tool.py::test_inspect_media_exports_final_decodable_frame_at_exact_duration`
+
+```
+ffmpeg still extraction failed: Automatic encoder selection failed ...
+Error opening output files: Encoder not found
+```
+
+The test asks ffmpeg for a `.webp` still and asserts `exit_code == 0`. WebP
+encoding is a build option, and Homebrew's ffmpeg does not always carry it. CI
+installs a build that does, which is why this is green there.
+
+**Needs a decision**: skip when the encoder is absent, or fall back to PNG. The
+current shape asserts success from a codec that is not guaranteed present.
+
+### Environmental: real sockets
+
+- `tests/test_integration_api_call_ssrf.py::test_real_socket_falls_back_from_dead_first_to_live_second`
+
+```
+httpcore.ConnectTimeout / httpx.ConnectTimeout
+```
+
+Opens real sockets and depends on a connection to a dead address being refused
+quickly rather than hanging. Sandboxed and restricted-network machines time out
+instead. Genuinely environmental.
+
+### Unexplained: rich-text colour contrast
+
+- `tests/test_document_rich_color_reset_and_contrast.py::test_rich_colors_follow_theme_and_undo_as_one_edit`
+
+A Playwright run times out waiting for `#doc-email-richbody p` to contain a
+`span` after a colour is applied.
+
+**This one is not flaky.** Three consecutive runs failed identically, each at
+about 31 seconds. It was previously written off as timing noise and that was
+wrong. The cause is not established, and until it is, treat it as a possible
+real defect in the rich-text colour path rather than a platform artifact.
+
+## Keeping this current
+
+Re-measure on a clean checkout of `lab` with the prerequisites above, and
+update the header revision, the counts and any entry that changed. A failure
+that appears and is not listed here is a regression until shown otherwise.

tests/README.md

@@ -33,6 +33,8 @@ the sub-area. The `area_*` names are registered in `pyproject.toml`; the dynamic
 `sub_*` names are registered before collection by `pytest_configure` in
 `tests/conftest.py`, so unknown-mark warnings still flag genuine typos.
 
+The full suite does not come back clean on every machine. [KNOWN_FAILURES.md](KNOWN_FAILURES.md) lists which failures are expected, which are test bugs worth fixing, and the prerequisites a clean run needs; anything not on that list is a regression until shown otherwise.
+
 For common focused runs, use `tests/run_focus.py`. It validates area and
 sub-area names, accepts sub-areas with or without the `sub_` prefix, and passes
 extra pytest arguments after `--`:

tests/helpers/unix_sockets.py

@@ -0,0 +1,46 @@
+"""Bind an AF_UNIX socket at a path the kernel will actually accept.
+
+``sun_path`` is 104 bytes on macOS, terminator included, so a bind path longer
+than 103 characters fails with ``OSError: AF_UNIX path too long``. pytest's
+``tmp_path`` is rooted at ``$TMPDIR``, which on stock macOS is a 49-character
+``/var/folders/<2>/<30>/T/`` path; adding ``pytest-of-<user>/pytest-<n>/`` and
+the test's own (truncated) name spends the rest of the budget before the
+filename is appended.
+
+That is why this reads as flaky rather than broken. Linux allows 108 bytes and
+roots ``$TMPDIR`` at ``/tmp``, so it never bites there; on macOS whether it
+bites depends on the length of ``$TMPDIR``, the test's name, and how many
+digits pytest's run counter is currently using. A run under a shortened
+``$TMPDIR`` passes, the same checkout under the default one does not.
+
+The path is resolved before it is handed back, for the same reason the rest of
+this change resolves temp paths: on macOS ``/tmp`` is a symlink to
+``/private/tmp``, and a test that binds one spelling and asserts on the other
+is comparing two names for the same socket.
+"""
+
+import os
+import shutil
+import socket
+import tempfile
+from contextlib import contextmanager
+
+# Short enough to leave room for the socket's own name under every platform's
+# sun_path budget. A relative root would depend on the working directory.
+_SHORT_ROOT = os.path.realpath(tempfile.gettempdir() if os.name == "nt" else "/tmp")
+
+
+@contextmanager
+def bound_unix_socket(name="docker.sock"):
+    """Yield the path of a listening AF_UNIX socket, cleaned up on exit."""
+    directory = os.path.realpath(tempfile.mkdtemp(prefix="odysseus-sock-", dir=_SHORT_ROOT))
+    path = os.path.join(directory, name)
+    if len(path) > 103:  # pragma: no cover - guards the guard
+        raise AssertionError(f"socket path is {len(path)} bytes, over the limit: {path}")
+    sock = socket.socket(socket.AF_UNIX)
+    try:
+        sock.bind(path)
+        yield path
+    finally:
+        sock.close()
+        shutil.rmtree(directory, ignore_errors=True)

tests/test_code_nav_tools.py

@@ -17,7 +17,10 @@ def _run(tool, content):
 @pytest.fixture
 def repo():
     # Built under /tmp, which is on the default tool-path allowlist.
-    root = tempfile.mkdtemp(dir="/tmp", prefix="codenav_")
+    # realpath because the code under test resolves the path it reports, and on
+    # macOS /tmp is a symlink to /private/tmp: comparing the unresolved path
+    # against the resolved one fails on a file both sides found correctly.
+    root = os.path.realpath(tempfile.mkdtemp(dir="/tmp", prefix="codenav_"))
     try:
         with open(os.path.join(root, "a.py"), "w") as f:
             f.write("import os\n# needle here\nprint('x')\n")

tests/test_cookbook_docker_access.py

@@ -1,4 +1,3 @@
-import socket
 from unittest.mock import AsyncMock
 
 import pytest
@@ -9,6 +8,7 @@ from starlette.requests import Request
 import routes.cookbook_routes as cookbook_routes
 from routes.cookbook_helpers import ServeRequest, _validate_serve_cmd
 from src.host_docker_access import HOST_DOCKER_ACCESS_HINT
+from tests.helpers.unix_sockets import bound_unix_socket
 
 
 def _model_serve_endpoint():
@@ -57,19 +57,18 @@ async def test_container_cli_only_is_rejected(monkeypatch, tmp_path):
 
 
 @pytest.mark.asyncio
-async def test_container_opt_in_with_unix_socket_is_allowed(monkeypatch, tmp_path):
+async def test_container_opt_in_with_unix_socket_is_allowed(monkeypatch):
     monkeypatch.setattr(cookbook_routes.shutil, "which", lambda binary: "/usr/bin/docker")
-    socket_path = tmp_path / "docker.sock"
 
-    with socket.socket(socket.AF_UNIX) as unix_socket:
+    # Not tmp_path: binding under $TMPDIR overruns sun_path on macOS.
-        unix_socket.bind(str(socket_path))
+    with bound_unix_socket() as socket_path:
         available = await cookbook_routes._binary_available(
             "docker",
             None,
             None,
             in_container=True,
             environ={"ODYSSEUS_ENABLE_HOST_DOCKER": "true"},
-            socket_path=str(socket_path),
+            socket_path=socket_path,
         )
 
     assert available is True

tests/test_shell_routes.py

@@ -5,7 +5,6 @@ import importlib
 import importlib.util
 import json
 import os
-import socket
 import sys
 from pathlib import Path
 from types import SimpleNamespace
@@ -28,6 +27,7 @@ from routes.shell_routes import (
     _venv_activate_prefix,
     DOCKER_IN_CONTAINER_HINT,
 )
+from tests.helpers.unix_sockets import bound_unix_socket
 
 
 def test_shell_routes_import_without_posix_pty_modules(monkeypatch):
@@ -294,30 +294,25 @@ class TestHostDockerAccess:
     def test_socket_without_explicit_opt_in_is_disabled(
         self,
         monkeypatch,
-        tmp_path,
         flag,
     ):
-        socket_path = tmp_path / "docker.sock"
+        # Not tmp_path: binding under $TMPDIR overruns sun_path on macOS.
-        with socket.socket(socket.AF_UNIX) as unix_socket:
+        with bound_unix_socket() as socket_path:
-            unix_socket.bind(str(socket_path))
             if flag is None:
                 monkeypatch.delenv("ODYSSEUS_ENABLE_HOST_DOCKER", raising=False)
             else:
                 monkeypatch.setenv("ODYSSEUS_ENABLE_HOST_DOCKER", flag)
 
-            assert _host_docker_access_enabled(str(socket_path)) is False
+            assert _host_docker_access_enabled(socket_path) is False
 
     def test_explicit_opt_in_with_unix_socket_is_enabled(
         self,
         monkeypatch,
-        tmp_path,
     ):
-        socket_path = tmp_path / "docker.sock"
+        with bound_unix_socket() as socket_path:
-        with socket.socket(socket.AF_UNIX) as unix_socket:
-            unix_socket.bind(str(socket_path))
             monkeypatch.setenv("ODYSSEUS_ENABLE_HOST_DOCKER", "true")
 
-            assert _host_docker_access_enabled(str(socket_path)) is True
+            assert _host_docker_access_enabled(socket_path) is True
 
 
 class TestPackageProbeStatus:

tests/test_workspace_confine.py

@@ -225,8 +225,13 @@ async def test_glob_confined_e2e(ws, admin):
     assert ws not in r["output"]
     assert "/workspace/found.py" in r["output"]
 
-    # a secret outside the workspace must not be discoverable via glob
+    # a secret outside the workspace must not be discoverable via glob.
-    outside = tempfile.mkdtemp()
+    # realpath so this directory and os.path.realpath(ws) below sit in the same
+    # resolved tree. On macOS /tmp is a symlink to /private/tmp, and mixing a
+    # resolved workspace with an unresolved secret makes relpath emit
+    # "../../../../tmp/<abs path>", which trivially contains the absolute path
+    # the assertion is checking for.
+    outside = os.path.realpath(tempfile.mkdtemp())
     secret = os.path.join(outside, "secret.txt")
     with open(secret, "w") as f:
         f.write("nope")