Merge pull request #34 from o3LL/test/macos-failures-and-stub-leak-guard

test: fix environment-dependent failures and guard module-stub leaks

tests/conftest.py

@@ -159,3 +159,43 @@ def _serve_test_static():
             os.environ["ODYSSEUS_TEST_STATIC_ORIGIN"] = previous_origin
         server.shutdown()
         server.server_close()
+
+
+@pytest.fixture(autouse=True)
+def _no_leaked_module_stubs():
+    """Fail the test that leaves a bare ``src.*``/``core.*`` stub behind.
+
+    Several test modules install empty stand-in modules so an import-heavy
+    production module can be loaded under the mocks above. When one of those
+    writes is not undone, the stub stays in ``sys.modules`` for the rest of the
+    session and every later test that imports the real module silently gets an
+    empty one instead. The suite still passes as a whole, because the victims
+    usually run before the leak; it only breaks under a different collection
+    order, which is why this class of bug reaches CI green.
+
+    This fixture is declared in the root conftest, so it is set up before any
+    test-module fixture and torn down after all of them — a stub that a test's
+    own teardown removes is not reported. The leaked entries are dropped here
+    as well as reported, so the failure stays attributed to the test that
+    introduced it instead of cascading into the rest of the run.
+
+    Bare stubs present before the test starts are ignored: this guards against
+    new leaks, it does not police import state the session began with.
+    """
+    from tests.helpers.import_state import bare_module_stubs, clear_module
+
+    before = bare_module_stubs()
+    yield
+    leaked = sorted(bare_module_stubs() - before)
+    if not leaked:
+        return
+    for name in leaked:
+        clear_module(name)
+    pytest.fail(
+        "test left bare module stub(s) in sys.modules: "
+        + ", ".join(leaked)
+        + ". Register the stub through monkeypatch.setitem(sys.modules, ...) "
+        "or tests.helpers.import_state.preserve_import_state so it is undone "
+        "at teardown.",
+        pytrace=False,
+    )

tests/helpers/import_state.py

@@ -31,6 +31,7 @@ safe for callers that pass both a parent package and a child module.
 """
 
 import sys
+import types
 from contextlib import contextmanager
 
 _ABSENT = object()
@@ -167,3 +168,33 @@ def preserve_import_state(*module_names):
         # Phase 2: restore all parent-package attributes.
         for name, (_, saved_attr) in saved.items():
             _restore_parent_attr(name, saved_attr)
+
+
+# Names under these prefixes are the ones a leaked stub actually breaks: a
+# later test doing ``import src.x`` or ``import core.x`` silently gets the
+# empty stub instead of the real module.
+_GUARDED_PREFIXES = ("src.", "core.")
+
+
+def bare_module_stubs():
+    """Return the ``src.*``/``core.*`` names currently bound to a bare stub.
+
+    A bare stub is a plain :class:`types.ModuleType` with no on-disk
+    ``__file__`` — the object ``types.ModuleType(name)`` produces. That is the
+    same "is this a fake?" test the ``clear_fake_*`` helpers above use, so a
+    module imported from disk is never reported.
+
+    ``MagicMock`` stand-ins are deliberately out of scope: they answer every
+    attribute, so they fail loudly at use rather than silently, and several
+    test modules install them on purpose.
+    """
+    found = set()
+    for name, mod in list(sys.modules.items()):
+        if not name.startswith(_GUARDED_PREFIXES):
+            continue
+        if type(mod) is not types.ModuleType:
+            continue
+        if getattr(mod, "__file__", None):
+            continue
+        found.add(name)
+    return found

tests/test_auth_regressions.py

@@ -21,7 +21,7 @@ from unittest.mock import MagicMock
 # (Same trick as test_null_owner_gates.py — the real modules instantiate
 # SQLAlchemy declarative classes at import-time which blow up under the
 # conftest's `sqlalchemy.*` MagicMock stubs.)
-def _ensure_stub(name: str, **attrs):
+def _ensure_stub(monkeypatch, name: str, **attrs):
     """Create or augment a stub module with the given attributes.
     Augments existing entries because earlier-run tests may have already
     stubbed the same module with a different attribute set.
@@ -48,7 +48,7 @@ def _ensure_stub(name: str, **attrs):
                 *parent_name.split("."),
             )
             parent.__path__ = [real_path] if os.path.isdir(real_path) else []
-            sys.modules[parent_name] = parent
+            monkeypatch.setitem(sys.modules, parent_name, parent)
         else:
             parent = sys.modules[parent_name]
     else:
@@ -58,17 +58,17 @@ def _ensure_stub(name: str, **attrs):
     mod = sys.modules.get(name)
     if mod is None:
         mod = types.ModuleType(name)
-        sys.modules[name] = mod
+        monkeypatch.setitem(sys.modules, name, mod)
     for k, v in attrs.items():
         if not hasattr(mod, k):
-            setattr(mod, k, v)
+            monkeypatch.setattr(mod, k, v, raising=False)
     if parent is not None and not hasattr(parent, child_name):
-        setattr(parent, child_name, mod)
+        monkeypatch.setattr(parent, child_name, mod, raising=False)
     return mod
 
 @pytest.fixture(autouse=True)
 def _auth_regressions_stubs(monkeypatch):
-    db = _ensure_stub("core.database",
+    db = _ensure_stub(monkeypatch, "core.database",
         SessionLocal=MagicMock(), ScheduledTask=MagicMock(), TaskRun=MagicMock(),
         ModelEndpoint=MagicMock(), Session=MagicMock(), ChatMessage=MagicMock(),
         CalendarCal=MagicMock(), CalendarEvent=MagicMock(),
@@ -76,17 +76,18 @@ def _auth_regressions_stubs(monkeypatch):
         GalleryImage=MagicMock(), GalleryAlbum=MagicMock(), Note=MagicMock(),
         McpServer=MagicMock(),
     )
-    auth = _ensure_stub("core.auth", AuthManager=MagicMock())
-    ep = _ensure_stub("src.endpoint_resolver",
+    auth = _ensure_stub(monkeypatch, "core.auth", AuthManager=MagicMock())
+    ep = _ensure_stub(monkeypatch, "src.endpoint_resolver",
         resolve_endpoint=MagicMock(return_value=("", "", {})),
         normalize_base=MagicMock(),
         build_chat_url=MagicMock(),
         build_models_url=MagicMock(),
         build_headers=MagicMock(),
     )
-    monkeypatch.setitem(sys.modules, "core.database", db)
-    monkeypatch.setitem(sys.modules, "core.auth", auth)
-    monkeypatch.setitem(sys.modules, "src.endpoint_resolver", ep)
+    # _ensure_stub now registers each stub through monkeypatch itself, so the
+    # whole set is undone at teardown. Re-setting them here would capture the
+    # stub as the restore target and leave it behind for the rest of the run.
+    assert db and auth and ep
 
 from fastapi import HTTPException
 
@@ -293,7 +294,7 @@ def test_research_spinoff_rejects_wrong_owner():
 # pop_notifications owner filter
 # ---------------------------------------------------------------------------
 
-def test_pop_notifications_owner_filtered():
+def test_pop_notifications_owner_filtered(monkeypatch):
     """pop_notifications(owner='alice') must return only alice's items.
     bob's and legacy ownerless items stay behind in the queue."""
     # Build a minimal scheduler instance that we can hit directly.
@@ -302,11 +303,15 @@ def test_pop_notifications_owner_filtered():
     import sys, types
     from unittest.mock import MagicMock as _MM
     # `task_scheduler` pulls in lots of helpers — stub the ones it uses.
+    # monkeypatch.setitem, not a bare assignment: a plain write leaves these
+    # empty stubs in sys.modules for the rest of the session, and every later
+    # test that imports a real name from one of them fails with
+    # "cannot import name ... (unknown location)". The stubs above in this file
+    # already use monkeypatch for the same reason.
     for s in ["src.builtin_actions", "src.ai_interaction", "src.endpoint_resolver",
               "src.agent_loop", "src.session_manager"]:
         if s not in sys.modules:
-            mod = types.ModuleType(s)
-            sys.modules[s] = mod
+            monkeypatch.setitem(sys.modules, s, types.ModuleType(s))
     from src.task_scheduler import TaskScheduler
     sch = TaskScheduler.__new__(TaskScheduler)  # bypass __init__ network etc.
     sch._pending_notifications = []

tests/test_document_rich_color_reset_and_contrast.py

@@ -20,6 +20,12 @@ def test_color_controls_have_theme_reset_and_split_palettes():
 
 
 def test_rich_colors_follow_theme_and_undo_as_one_edit():
+    # Two input conventions in here are platform-sensitive and must stay that
+    # way. Palette entries are opened with a plain click: on macOS a
+    # Control+click is delivered as `contextmenu`, so the menu item's `click`
+    # handler never runs and nothing is applied. Undo uses Playwright's
+    # `ControlOrMeta` alias because the editor's undo accelerator is Cmd+Z on
+    # macOS and Ctrl+Z everywhere else.
     script = r"""
       import { chromium } from 'playwright';
       const browser = await chromium.launch({ headless: true });
@@ -58,24 +64,24 @@ def test_rich_colors_follow_theme_and_undo_as_one_edit():
         labels: [...document.querySelectorAll('.rich-color-palette-label')].map(item => item.textContent),
         reset: document.querySelector('.rich-color-reset')?.textContent.trim(),
       }));
-      await page.locator('#doc-md-dd-menu .doc-overflow-item').filter({ hasText: 'Lemon' }).click({ modifiers: ['Control'] });
+      await page.locator('#doc-md-dd-menu .doc-overflow-item').filter({ hasText: 'Lemon' }).click();
       const highlighted = await page.locator('#doc-email-richbody p').nth(0).locator('span').evaluate(span => ({
         color: getComputedStyle(span).color,
         background: getComputedStyle(span).backgroundColor,
       }));
-      await page.locator('#doc-email-richbody').press('Control+z');
+      await page.locator('#doc-email-richbody').press('ControlOrMeta+z');
       const highlightUndone = await page.locator('#doc-email-richbody p').nth(0).innerHTML();
 
       await selectParagraph(1);
       await openMenu('color');
-      await page.locator('.rich-color-reset').click({ modifiers: ['Control'] });
+      await page.locator('.rich-color-reset').click();
       const defaultColor = await page.locator('#doc-email-richbody p').nth(1).locator('span').evaluate(span => ({
         style: span.getAttribute('style'),
         color: getComputedStyle(span).color,
       }));
       await page.evaluate(() => document.documentElement.style.setProperty('--fg', '#88cc44'));
       const changedThemeColor = await page.locator('#doc-email-richbody p').nth(1).locator('span').evaluate(span => getComputedStyle(span).color);
-      await page.locator('#doc-email-richbody').press('Control+z');
+      await page.locator('#doc-email-richbody').press('ControlOrMeta+z');
       const colorUndone = await page.locator('#doc-email-richbody p').nth(1).innerHTML();
       console.log(JSON.stringify({ palette, highlighted, highlightUndone, defaultColor, changedThemeColor, colorUndone }));
       await browser.close();

tests/test_inspect_media_tool.py

@@ -1,5 +1,6 @@
 import asyncio
 import base64
+import functools
 import io
 import json
 from pathlib import Path
@@ -20,6 +21,28 @@ from src.tool_execution import _active_workspace
 from src.tool_schemas import FUNCTION_TOOL_SCHEMAS
 
 
+@functools.lru_cache(maxsize=None)
+def _ffmpeg_has_encoder(name: str) -> bool:
+    """Whether the ffmpeg on PATH was built with the named encoder.
+
+    Codec support is a build option, not something the project requires. The
+    Homebrew ffmpeg on macOS ships without libwebp, for instance, so a test
+    that asserts a successful `.webp` export there fails on the build rather
+    than on the tool.
+    """
+    if not shutil.which("ffmpeg"):
+        return False
+    listed = subprocess.run(
+        ["ffmpeg", "-hide_banner", "-loglevel", "error", "-encoders"],
+        check=False, capture_output=True, text=True,
+    )
+    return any(
+        line.split()[1:2] == [name] or f"(codec {name})" in line
+        for line in listed.stdout.splitlines()
+        if line.strip()
+    )
+
+
 def test_media_timestamp_parser_accepts_units_and_four_field_timecodes():
     assert _parse_seconds("0m", default=-1) == 0
     assert _parse_seconds("30m", default=-1) == 1800
@@ -491,13 +514,16 @@ def test_inspect_media_exports_final_decodable_frame_at_exact_duration(tmp_path:
         result = asyncio.run(InspectMediaTool().execute(json.dumps({
             "path": "/workspace/video.mp4",
             "timestamp": "end",
-            "output_path": "/workspace/final.webp",
+            # PNG, not WebP: this asserts that the *final* frame is decodable at
+            # the exact duration, so it must not also depend on an optional
+            # ffmpeg encoder. WebP export is covered separately below.
+            "output_path": "/workspace/final.png",
         }), {}))
     finally:
         _active_workspace.reset(token)
 
     assert result["exit_code"] == 0, result
-    assert (tmp_path / "final.webp").stat().st_size > 0
+    assert (tmp_path / "final.png").stat().st_size > 0
 
     token = _active_workspace.set(str(tmp_path))
     try:
@@ -514,6 +540,67 @@ def test_inspect_media_exports_final_decodable_frame_at_exact_duration(tmp_path:
     assert Image.open(io.BytesIO(base64.b64decode(high_detail["images"][0]["data"]))).size == (768, 432)
 
 
+@pytest.mark.skipif(not shutil.which("ffmpeg") or not shutil.which("ffprobe"), reason="ffmpeg required")
+@pytest.mark.skipif(not _ffmpeg_has_encoder("webp"), reason="ffmpeg built without a webp encoder")
+def test_inspect_media_exports_a_webp_still(tmp_path: Path):
+    """A `.webp` output_path is passed straight through to ffmpeg.
+
+    Guarded on the encoder rather than asserted unconditionally: WebP is a
+    build option (Homebrew's macOS ffmpeg omits it) and the project does not
+    require it. When the encoder is missing the tool reports ffmpeg's failure
+    with `exit_code` 1, which is covered by
+    `test_inspect_media_reports_a_missing_encoder_instead_of_crashing`.
+    """
+    video = tmp_path / "video.mp4"
+    subprocess.run([
+        "ffmpeg", "-hide_banner", "-loglevel", "error", "-f", "lavfi",
+        "-i", "testsrc2=size=320x180:rate=4:duration=2", "-pix_fmt", "yuv420p",
+        "-y", str(video),
+    ], check=True)
+    token = _active_workspace.set(str(tmp_path))
+    try:
+        result = asyncio.run(InspectMediaTool().execute(json.dumps({
+            "path": "/workspace/video.mp4",
+            "timestamp": "end",
+            "output_path": "/workspace/final.webp",
+        }), {}))
+    finally:
+        _active_workspace.reset(token)
+
+    assert result["exit_code"] == 0, result
+    assert Image.open(tmp_path / "final.webp").format == "WEBP"
+
+
+@pytest.mark.skipif(not shutil.which("ffmpeg") or not shutil.which("ffprobe"), reason="ffmpeg required")
+@pytest.mark.skipif(_ffmpeg_has_encoder("webp"), reason="needs an ffmpeg built without webp")
+def test_inspect_media_reports_a_missing_encoder_instead_of_crashing(tmp_path: Path):
+    """An export in a format this ffmpeg cannot encode fails as a tool error.
+
+    The tool does not probe the encoder list, so the only contract it can keep
+    is to surface ffmpeg's own failure rather than raise or write a truncated
+    file. Asserted only on builds that actually lack the encoder.
+    """
+    video = tmp_path / "video.mp4"
+    subprocess.run([
+        "ffmpeg", "-hide_banner", "-loglevel", "error", "-f", "lavfi",
+        "-i", "testsrc2=size=320x180:rate=4:duration=2", "-pix_fmt", "yuv420p",
+        "-y", str(video),
+    ], check=True)
+    token = _active_workspace.set(str(tmp_path))
+    try:
+        result = asyncio.run(InspectMediaTool().execute(json.dumps({
+            "path": "/workspace/video.mp4",
+            "timestamp": "end",
+            "output_path": "/workspace/final.webp",
+        }), {}))
+    finally:
+        _active_workspace.reset(token)
+
+    assert result["exit_code"] == 1
+    assert "ffmpeg still extraction failed" in result["error"]
+    assert not (tmp_path / "final.webp").exists()
+
+
 @pytest.mark.skipif(not shutil.which("ffmpeg") or not shutil.which("ffprobe"), reason="ffmpeg required")
 def test_inspect_media_rejects_ambiguous_multi_frame_single_image_export(tmp_path: Path):
     video = tmp_path / "video.mp4"

tests/test_scheduler_restart_doublefire.py

@@ -21,12 +21,21 @@ def _test_utcnow():
     return datetime.now(timezone.utc).replace(tzinfo=None)
 
 
-def _stub_heavy():
+def _stub_heavy(monkeypatch):
+    """Stub the heavy modules ``task_scheduler`` imports, for this test only.
+
+    Registered through ``monkeypatch.setitem`` so every entry is removed at
+    teardown. A bare ``sys.modules[name] = ...`` leaves an empty module behind
+    for the rest of the session, and any later test that imports the real one
+    silently gets the stub instead - a failure that only shows up under a
+    different collection order.
+    """
     for name in [
         "src.builtin_actions", "src.ai_interaction", "src.endpoint_resolver",
         "src.agent_loop", "src.session_manager",
     ]:
-        sys.modules.setdefault(name, types.ModuleType(name))
+        if name not in sys.modules:
+            monkeypatch.setitem(sys.modules, name, types.ModuleType(name))
 
 
 def _setup_isolated_db():
@@ -74,7 +83,7 @@ def test_scheduler_utcnow_preserves_naive_utc_contract():
 
 def _drive_scheduler(monkeypatch, pre_start_setup=None):
     """Build a TaskScheduler bypassing __init__ and run start() + two polls."""
-    _stub_heavy()
+    _stub_heavy(monkeypatch)
     cd, ScheduledTask, TaskRun = _setup_isolated_db()
 
     from src.task_scheduler import TaskScheduler