diff --git a/packages/tapps-core/src/tapps_core/agent_identity.py b/packages/tapps-core/src/tapps_core/agent_identity.py index 8e5ae02e..84ff5cdb 100644 --- a/packages/tapps-core/src/tapps_core/agent_identity.py +++ b/packages/tapps-core/src/tapps_core/agent_identity.py @@ -35,6 +35,25 @@ _SLUG_INVALID_RE = re.compile(r"[^a-zA-Z0-9_-]+") +def is_real_writable_root(project_root: object) -> bool: + """Return True only when *project_root* is a real, absolute filesystem path. + + TAP-4573 guard. Production callers always resolve ``project_root`` to an + absolute path (explicit arg, ``TAPPS_MCP_PROJECT_ROOT`` env, or ``cwd``). + A bare ``MagicMock()`` coerces via ``os.fspath`` to the *relative* string + ``MagicMock/mock.project_root/`` (verified empirically), so ~70 test + call sites that pass unspec'd mocks were causing real ``mkdir`` trees under + the pytest CWD (the repo root). Rejecting non-absolute / non-coercible + roots blocks that leak at the single production write path without touching + the tests, and is a no-op for every real deployment (roots are absolute). + """ + try: + raw = os.fspath(project_root) # type: ignore[arg-type] + except (TypeError, ValueError): + return False + return Path(raw).is_absolute() + + def _slugify(value: str) -> str: """Reduce a string to a safe agent-id prefix (alnum/dash/underscore).""" cleaned = _SLUG_INVALID_RE.sub("-", value).strip("-_") @@ -86,7 +105,16 @@ def get_stable_agent_id(settings: TappsMCPSettings) -> str: if override: return override - project_root = Path(getattr(settings, "project_root", Path.cwd())) + raw_root = getattr(settings, "project_root", Path.cwd()) + + # TAP-4573: never mkdir/write under a non-real (mock- or relative-coerced) + # project_root. A bare MagicMock() coerces to a relative "MagicMock/..." + # path, so writing it would create a real tree in the pytest CWD. Skip the + # persistence attempt but still return a valid in-memory agent id. + if not is_real_writable_root(raw_root): + return f"{_project_slug(settings)}-{uuid.uuid4().hex[:_UUID_SHORT_LEN]}" + + project_root = Path(raw_root) id_path = project_root / _AGENT_ID_RELATIVE_PATH uuid_hex = _read_uuid(id_path) @@ -113,4 +141,4 @@ def get_stable_agent_id(settings: TappsMCPSettings) -> str: return f"{_project_slug(settings)}-{short}" -__all__ = ["get_stable_agent_id"] +__all__ = ["get_stable_agent_id", "is_real_writable_root"] diff --git a/packages/tapps-core/tests/unit/test_agent_identity.py b/packages/tapps-core/tests/unit/test_agent_identity.py index 8331c304..8785b416 100644 --- a/packages/tapps-core/tests/unit/test_agent_identity.py +++ b/packages/tapps-core/tests/unit/test_agent_identity.py @@ -5,8 +5,12 @@ import re from pathlib import Path from typing import TYPE_CHECKING +from unittest.mock import MagicMock -from tapps_core.agent_identity import get_stable_agent_id +from tapps_core.agent_identity import ( + get_stable_agent_id, + is_real_writable_root, +) from tapps_core.config.settings import TappsMCPSettings if TYPE_CHECKING: @@ -115,3 +119,43 @@ def test_agent_id_slugifies_unsafe_project_id( assert agent_id.startswith("my-project-v2-") assert _AGENT_ID_RE.match(agent_id) + + +# --------------------------------------------------------------------------- # +# TAP-4573: mock/relative-root write guard +# --------------------------------------------------------------------------- # + + +def test_is_real_writable_root_rejects_mock_and_relative() -> None: + """The guard accepts only absolute filesystem paths.""" + assert is_real_writable_root(Path("/abs/real/path")) is True + assert is_real_writable_root("/abs/real/path") is True + # A bare MagicMock coerces (via os.fspath) to a *relative* "MagicMock/..." + # path — the exact leak vector from TAP-4573. + assert is_real_writable_root(MagicMock().project_root) is False + assert is_real_writable_root(Path("relative/dir")) is False + assert is_real_writable_root(object()) is False + + +def test_mock_settings_creates_no_files( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A bare MagicMock() settings must not mkdir/write under the CWD. + + Regression guard for TAP-4573: ~70 call sites passed unspec'd mocks whose + ``project_root`` coerced to a relative ``MagicMock/...`` path, leaking real + directory trees into the repo root on every full-suite run. + """ + monkeypatch.delenv("CLAUDE_AGENT_ID", raising=False) + monkeypatch.chdir(tmp_path) + + settings = MagicMock() + settings.memory.project_id = "" + + agent_id = get_stable_agent_id(settings) + + # Still returns a well-formed id (in-memory UUID, no persistence). + assert _AGENT_ID_RE.match(agent_id), f"unexpected format: {agent_id!r}" + # And critically: no MagicMock/ tree was created under the CWD. + assert not (tmp_path / "MagicMock").exists() + assert list(tmp_path.iterdir()) == [] diff --git a/packages/tapps-mcp/src/tapps_mcp/server_helpers.py b/packages/tapps-mcp/src/tapps_mcp/server_helpers.py index d9ef361a..976733a8 100644 --- a/packages/tapps-mcp/src/tapps_mcp/server_helpers.py +++ b/packages/tapps-mcp/src/tapps_mcp/server_helpers.py @@ -654,6 +654,13 @@ def write_session_start_marker(project_root: Path | str) -> None: user prompt yields a reminder. """ try: + # TAP-4573: refuse to mkdir/write under a non-real project_root. A bare + # MagicMock() coerces to a relative "MagicMock/..." path, which would + # otherwise create a real tree in the pytest CWD (the repo root). + from tapps_core.agent_identity import is_real_writable_root + + if not is_real_writable_root(project_root): + return root = project_root if isinstance(project_root, Path) else Path(project_root) sidecar_dir = root / ".tapps-mcp" sidecar_dir.mkdir(parents=True, exist_ok=True) diff --git a/packages/tapps-mcp/tests/unit/test_user_prompt_submit_hook.py b/packages/tapps-mcp/tests/unit/test_user_prompt_submit_hook.py index 3066c6ba..56a47ce8 100644 --- a/packages/tapps-mcp/tests/unit/test_user_prompt_submit_hook.py +++ b/packages/tapps-mcp/tests/unit/test_user_prompt_submit_hook.py @@ -59,6 +59,17 @@ def test_session_start_marker_swallows_errors(self) -> None: # Writer must not raise. write_session_start_marker("/dev/null/cannot-mkdir-here") + def test_session_start_marker_ignores_mock_root( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # TAP-4573: a bare MagicMock().project_root coerces to a relative + # "MagicMock/..." path. The writer must refuse it rather than create a + # real tree under the pytest CWD. + monkeypatch.chdir(tmp_path) + write_session_start_marker(MagicMock().project_root) + assert not (tmp_path / "MagicMock").exists() + assert list(tmp_path.iterdir()) == [] + def test_checklist_state_marker_emits_brain_event(self, tmp_path: Path) -> None: mock_bridge = MagicMock() mock_bridge.record_kg_event = AsyncMock(return_value={"recorded": True})