From db36e7856317fd934a877151042bafc5a664771a Mon Sep 17 00:00:00 2001 From: Trecek Date: Wed, 10 Jun 2026 10:26:14 -0700 Subject: [PATCH 1/3] refactor: decouple cli/app.py from claude_code_project_dir() via SessionLocator Protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the vestigial try/except OSError guard around direct claude_code_project_dir() call with Protocol-dispatched ctx.backend.session_locator().project_log_dir(). The Protocol dispatch is backend-agnostic — ClaudeSessionLocator delegates to the same function, but other backends (e.g. Codex) can now return their own project log dir without cli/app.py needing to know. Also remove the now-unused claude_code_project_dir import and add a smoke test in tests/cli/test_app_main.py proving the activity_check lambda receives the backend-derived marker_dir. Test file size marker upgraded from small to medium (uses asyncio.run to exercise the async closure path). Co-Authored-By: Claude Fable 5 --- src/autoskillit/cli/app.py | 11 +++---- tests/cli/test_app_main.py | 64 +++++++++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/src/autoskillit/cli/app.py b/src/autoskillit/cli/app.py index cc831fd37e..ed126bc03b 100644 --- a/src/autoskillit/cli/app.py +++ b/src/autoskillit/cli/app.py @@ -31,7 +31,6 @@ from autoskillit.core import ( RecipeSource, atomic_write, - claude_code_project_dir, ) from autoskillit.execution import _has_active_execution_marker @@ -152,11 +151,11 @@ def serve(*, verbose: Annotated[bool, Parameter(name=["--verbose", "-v"])] = Fal try: backend_options = {"use_uvloop": sys.platform != "win32"} - _marker_dir: Path | None = None - try: - _marker_dir = claude_code_project_dir(str(ctx.project_dir)) - except OSError: - pass + _marker_dir: Path | None = ( + ctx.backend.session_locator().project_log_dir(str(ctx.project_dir)) + if ctx.backend is not None + else None + ) async def _guarded_serve() -> None: await serve_with_signal_guard( diff --git a/tests/cli/test_app_main.py b/tests/cli/test_app_main.py index 6f3e0ed1e7..c963d7c65b 100644 --- a/tests/cli/test_app_main.py +++ b/tests/cli/test_app_main.py @@ -4,10 +4,11 @@ import importlib import sys +from pathlib import Path import pytest -pytestmark = [pytest.mark.layer("cli"), pytest.mark.small] +pytestmark = [pytest.mark.layer("cli"), pytest.mark.medium] def test_main_does_not_call_app_after_update_restart(monkeypatch: pytest.MonkeyPatch) -> None: @@ -36,3 +37,64 @@ def fake_run_update_checks(**kwargs: object) -> None: pass assert not app_called, "app() must not be called when update path exits the process" + + +def test_serve_activity_check_uses_backend_derived_marker_dir( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + """serve() must derive _marker_dir via backend.session_locator().project_log_dir().""" + import asyncio + from unittest.mock import MagicMock + + monkeypatch.chdir(tmp_path) + + expected_dir = tmp_path / "backend-marker" + + mock_backend = MagicMock() + mock_backend.session_locator.return_value.project_log_dir.return_value = expected_dir + + mock_ctx = MagicMock() + mock_ctx.project_dir = tmp_path + mock_ctx.backend = mock_backend + mock_ctx.fleet_lock = None + + mock_cfg = MagicMock() + mock_cfg.logging.level = "INFO" + mock_cfg.logging.json_output = None + mock_cfg.safety.protected_branches = [] + + monkeypatch.setattr("autoskillit.config.load_config", lambda _: mock_cfg) + monkeypatch.setattr("autoskillit.core.configure_logging", lambda **kw: None) + monkeypatch.setattr("autoskillit.server.make_context", lambda *a, **kw: mock_ctx) + monkeypatch.setattr("autoskillit.server._initialize", lambda ctx: None) + + captured_activity_check: list = [] + + async def fake_serve_guard(mcp_instance, *, activity_check=None, **kw): + captured_activity_check.append(activity_check) + + monkeypatch.setattr("autoskillit.cli.app.serve_with_signal_guard", fake_serve_guard) + monkeypatch.setattr("anyio.run", lambda fn, **kw: asyncio.run(fn())) + + from autoskillit.cli.app import serve + + serve() + + mock_backend.session_locator.return_value.project_log_dir.assert_called_once_with( + str(tmp_path) + ) + + assert captured_activity_check, "serve_with_signal_guard was not called" + + seen_marker_dirs: list = [] + monkeypatch.setattr( + "autoskillit.cli.app.is_server_active", + lambda md, fl: (seen_marker_dirs.append(md), False)[-1], + ) + + captured_activity_check[0]() + assert seen_marker_dirs == [expected_dir], ( + f"activity_check should pass backend-derived marker_dir ({expected_dir}), " + f"got {seen_marker_dirs}" + ) From 27ca9ca6cfe364cd85c0c38b8a7bf6e53166e458 Mon Sep 17 00:00:00 2001 From: Trecek Date: Wed, 10 Jun 2026 10:30:42 -0700 Subject: [PATCH 2/3] fix: use importlib.import_module to patch app module attributes autoskillit.cli.__init__ exports `app` (the App instance), so monkeypatch.setattr("autoskillit.cli.app.*") traverses to the App object rather than the module. Use importlib.import_module to obtain the module directly, matching the pattern already used in the sibling test. --- tests/cli/test_app_main.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/cli/test_app_main.py b/tests/cli/test_app_main.py index c963d7c65b..198f128ca8 100644 --- a/tests/cli/test_app_main.py +++ b/tests/cli/test_app_main.py @@ -45,8 +45,11 @@ def test_serve_activity_check_uses_backend_derived_marker_dir( ) -> None: """serve() must derive _marker_dir via backend.session_locator().project_log_dir().""" import asyncio + import importlib from unittest.mock import MagicMock + app_module = importlib.import_module("autoskillit.cli.app") + monkeypatch.chdir(tmp_path) expected_dir = tmp_path / "backend-marker" @@ -74,7 +77,7 @@ def test_serve_activity_check_uses_backend_derived_marker_dir( async def fake_serve_guard(mcp_instance, *, activity_check=None, **kw): captured_activity_check.append(activity_check) - monkeypatch.setattr("autoskillit.cli.app.serve_with_signal_guard", fake_serve_guard) + monkeypatch.setattr(app_module, "serve_with_signal_guard", fake_serve_guard) monkeypatch.setattr("anyio.run", lambda fn, **kw: asyncio.run(fn())) from autoskillit.cli.app import serve @@ -89,7 +92,8 @@ async def fake_serve_guard(mcp_instance, *, activity_check=None, **kw): seen_marker_dirs: list = [] monkeypatch.setattr( - "autoskillit.cli.app.is_server_active", + app_module, + "is_server_active", lambda md, fl: (seen_marker_dirs.append(md), False)[-1], ) From 2484989e8742dfc55661411f3255158379234ebe Mon Sep 17 00:00:00 2001 From: Trecek Date: Wed, 10 Jun 2026 10:52:02 -0700 Subject: [PATCH 3/3] fix(review): move deferred imports to module level and strengthen guard assertion Per reviewer findings L47 and L91: - Move asyncio and MagicMock imports from inside test function body to module level; remove redundant local `import importlib` (already at module level) - Strengthen `assert captured_activity_check` to `assert len(...) == 1` so the test catches regressions where serve_with_signal_guard is called multiple times Co-Authored-By: Claude Sonnet 4.6 --- tests/cli/test_app_main.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/cli/test_app_main.py b/tests/cli/test_app_main.py index 198f128ca8..8b9b950314 100644 --- a/tests/cli/test_app_main.py +++ b/tests/cli/test_app_main.py @@ -2,9 +2,11 @@ from __future__ import annotations +import asyncio import importlib import sys from pathlib import Path +from unittest.mock import MagicMock import pytest @@ -44,10 +46,6 @@ def test_serve_activity_check_uses_backend_derived_marker_dir( tmp_path: Path, ) -> None: """serve() must derive _marker_dir via backend.session_locator().project_log_dir().""" - import asyncio - import importlib - from unittest.mock import MagicMock - app_module = importlib.import_module("autoskillit.cli.app") monkeypatch.chdir(tmp_path) @@ -88,7 +86,7 @@ async def fake_serve_guard(mcp_instance, *, activity_check=None, **kw): str(tmp_path) ) - assert captured_activity_check, "serve_with_signal_guard was not called" + assert len(captured_activity_check) == 1, "serve_with_signal_guard was not called exactly once" seen_marker_dirs: list = [] monkeypatch.setattr(