diff --git a/docs/superpowers/specs/2026-06-30-files-mcp-design.md b/docs/superpowers/specs/2026-06-30-files-mcp-design.md new file mode 100644 index 0000000..fc05c05 --- /dev/null +++ b/docs/superpowers/specs/2026-06-30-files-mcp-design.md @@ -0,0 +1,384 @@ +# Files MCP — Design Spec + +**Date:** 2026-06-30 +**Branch:** `feat/files-mcp` +**Mount path:** `/files-mcp` +**New package:** `files_mcp/` +**Status:** Design approved; awaiting implementation plan + +## Goal + +Add a sibling MCP service that exposes file-system CRUD (plus a handful of +introspection and rearrangement operations) over the same `fastapi-mcp` transport +the existing `spark-executor-mcp` already uses. The service is sandboxed to a +configurable root directory and only handles utf-8 text content. + +## Background & Motivation + +`spark-executor-mcp` ships 12 tools for the full Spark-on-YARN lifecycle +(submit, status, logs, kill, connections, plus Stage-2 PySpark file helpers). +The `MCP_SERVICES` registry in `main.py` was designed for additional sibling +services — the comment block above the list literally shows the intended +pattern: `McpService(name="metrics", app=metrics_app, mount_path="/metrics-mcp")`. + +This spec fills in one of those slots: a generic, sandboxed file-CRUD service. +Use cases the LLM can now express through the same MCP transport: + +- Read/write configuration snippets (YAML, JSON, `.env`) on the host. +- Stage input datasets in a known working directory before submitting a Spark job. +- Inspect prior job logs and outputs. +- Move/rename artifacts after a run completes. + +## Non-Goals + +- **Binary files.** MCP is JSON; binary would force base64 round-trips and + inflates the LLM context. We only do utf-8 text. (Mirrors the existing + `job_writer` "reject code that is not valid UTF-8" rule.) +- **Streaming / partial reads.** Read returns the whole file, capped at 1 MB. + YARN log tails are a separate tool (`get_job_logs`) on the other service. +- **Multi-tenant isolation / per-user roots.** Stage 1 ships a single shared + root from one env var. Per-user roots are a Stage 3 concern (per the existing + plan `docs/superpowers/plans/2026-06-24-spark-executor-mcp.md`). +- **File locking / concurrent writer arbitration.** Each write is atomic + (`tempfile + os.replace`) but we do not serialize writers. This is fine for + the actual usage (one LLM agent at a time per process); revisit if needed. + +## Architecture + +``` +main.py # append one McpService entry + └── FastApiMCP(files_app, mount_path="/files-mcp") + +files_mcp/ # new sibling package + __init__.py # re-exports app; triggers root initialization + server.py # FastAPI("Files MCP") + 9 routes + exception handlers + models.py # FileEntry, OperationResult + core/ + path_guard.py # resolve_and_check(path) -> Path; sandbox check + fs_ops.py # pure file operations on already-resolved Path + tools/ + files.py # business wrappers: log + delegate to fs_ops + requests.py # Pydantic body models (fastapi-mcp requires) + +tests/ + unit/test_path_guard.py + unit/test_fs_ops.py + unit/test_files_tool.py + integration/test_files_mcp_routes.py +``` + +### Why a sibling package (not a sub-module of `spark_executor`)? + +The existing `MCP_SERVICES` registry is the extension point the codebase was +designed around. Adding a sibling: + +- Uses the reserved extension point without bypassing it. +- Keeps the 12 Spark tools and 9 file tools on separate mount paths, so + `tools/list` for `/spark-executor-mcp` is not bloated. +- Aligns with the project's stated principle of additive change: 0 existing + files get modified beyond the one-line append in `main.py`. + +## Components + +### `core/path_guard.py` — the sandbox + +```python +_ROOT: Path # set once at import time from FILES_MCP_ROOT + +def _init_root() -> Path: + root = os.environ.get("FILES_MCP_ROOT") + if not root: + raise RuntimeError("FILES_MCP_ROOT env var is required") + p = Path(root).expanduser().resolve() + if not p.is_dir(): + raise RuntimeError(f"FILES_MCP_ROOT={root!r} is not an existing directory") + return p + +def resolve_and_check(path: str) -> Path: + """Resolve an arbitrary input path and verify it lies inside the sandbox. + + Raises: + ValueError: empty, contains NUL byte, or escapes the root. + """ + if not path: + raise ValueError("path is empty") + if "\x00" in path: + raise ValueError("path contains NUL byte") + p = Path(path).expanduser().resolve() + try: + p.relative_to(_ROOT) + except ValueError: + raise ValueError(f"path escapes sandbox root {_ROOT}: got {p}") from None + return p + +def set_root_for_testing(p: Path) -> None: + """Test-only escape hatch; never call from production code.""" + global _ROOT + _ROOT = p +``` + +Key properties: + +- One `.resolve()` call handles `~`, `.`, `..`, symlinks, double slashes. +- `relative_to` is the only sandbox check — no whitelist to keep in sync. +- A symlink inside the root pointing outside is rejected because + `resolve()` follows the link before we check. +- `set_root_for_testing` is the only mutation point, used by the + `conftest.py` autouse fixture so each test gets a fresh `tmp_path` root. + +### `core/fs_ops.py` — pure file operations + +All functions take an **already-resolved** `Path` (the caller is responsible for +running `path_guard.resolve_and_check`). This keeps `fs_ops` trivially +testable — no global state, no side effects beyond the operation it performs. + +```python +MAX_BYTES = 1_048_576 # 1 MB cap, mirrors read_job_file / update_job_file + +def create(path: Path, content: str, overwrite: bool) -> OperationResult: ... +def read(path: Path) -> dict: ... +def update(path: Path, content: str) -> OperationResult: ... +def delete(path: Path, recursive: bool) -> OperationResult: ... +def list_dir(path: Path, recursive: bool, max_depth: int) -> list[FileEntry]: ... +def stat(path: Path) -> FileEntry: ... +def search(path: Path, pattern: str, recursive: bool) -> list[str]: ... +def move(src: Path, dst: Path, overwrite: bool) -> OperationResult: ... +def copy(src: Path, dst: Path, overwrite: bool) -> OperationResult: ... +``` + +Atomic write pattern (used by `create` and `update`): + +```python +fd, tmp_path = tempfile.mkstemp(dir=path.parent, prefix=".tmp_", suffix=path.suffix) +try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(content) + os.replace(tmp_path, path) # atomic on same fs +except BaseException: + Path(tmp_path).unlink(missing_ok=True) + raise +``` + +### `tools/files.py` — business wrappers + +One function per tool, each: + +1. Logs DEBUG on entry. +2. Calls `path_guard.resolve_and_check` on every user-supplied path. +3. Delegates to `fs_ops`. +4. Returns a Pydantic-serialized dict. + +No business logic of its own — same shape as +`spark_executor/tools/connections.py`. + +### `tools/requests.py` — Pydantic body models + +One model per route. fastapi-mcp passes tool args as a JSON body, so dict-typed +params (none here, but the pattern matters) need a model, not query params. +Mirrors the pattern in `spark_executor/tools/requests.py`. + +### `server.py` — FastAPI app + routes + +9 routes, all `POST`, each with a `summary` and a `description`. The same +exception handlers as `spark_executor/server.py`: + +- `KeyError` → 404 +- `ValueError` → 400 +- `FileExistsError` → 409 (new in this service — needed for create / move / copy) + +The `/health` route is intentionally **not** added: the root app already has +`/health`. (Linus: don't add a duplicate.) + +## Data Model + +```python +# files_mcp/models.py +from typing import Literal +from pydantic import BaseModel + +class FileEntry(BaseModel): + path: str # absolute, resolved, inside sandbox + name: str + type: Literal["file", "directory", "symlink", "other"] + size: int # bytes; 0 for directories + mtime: float # unix seconds + mode: int # st_mode (octal, e.g. 0o644) + +class OperationResult(BaseModel): + path: str + status: str # "CREATED" | "UPDATED" | "DELETED" | "MOVED" | "COPIED" + size: int | None # None for delete/move +``` + +## API Surface + +| Tool | Inputs | Returns | Errors | +|---|---|---|---| +| `create_file` | `path`, `content`, `overwrite=false` | `OperationResult` | 409 if exists & !overwrite; 400 if >1MB or outside root | +| `read_file` | `path` | `{path, content, size, encoding:"utf-8"}` | 404 if not found; 400 if dir / >1MB / non-utf-8 / outside root | +| `update_file` | `path`, `content` | `OperationResult` | 404 if not found; 400 if >1MB / outside root | +| `delete_file` | `path`, `recursive=false` | `OperationResult` | 404 if not found; 400 if dir & !recursive | +| `list_dir` | `path`, `recursive=false`, `max_depth=1` | `{path, entries: [FileEntry]}` | 404 if not found; 400 if not dir | +| `stat` | `path` | `FileEntry` | 404 if not found; 400 if outside root | +| `search` | `path`, `pattern`, `recursive=true` | `{pattern, matches: [str]}` | 404 if root not found; 400 if outside root | +| `move` | `src`, `dst`, `overwrite=false` | `OperationResult` | 404 if src missing; 409 if dst exists & !overwrite | +| `copy` | `src`, `dst`, `overwrite=false` | `OperationResult` | Same as move | + +## Error Handling + +| Exception | HTTP | When | +|---|---|---| +| `KeyError("path 'X' not found")` | 404 | read/update/delete/stat on missing path | +| `ValueError("path 'X' is not a directory")` | 400 | list_dir with a file path | +| `ValueError("path escapes sandbox")` | 400 | path_guard rejects | +| `ValueError("file 'X' already exists")` | 409 | create / move / copy with dst existing and `overwrite=False` | +| `ValueError("content too large")` | 400 | write > 1 MB | +| `ValueError("not valid utf-8")` | 400 | read encounters non-utf-8 bytes | +| `FileExistsError` | 409 | (extra handler) atomic rename target exists | +| `IsADirectoryError` / `NotADirectoryError` | 400 | operation type mismatches path type | +| Other `OSError` | 500 | real I/O failure (EACCES, ENOSPC, etc.) | + +The `FileExistsError` handler is **new** (spark_executor has only the first +two). It is registered on `files_mcp.server.app`, not on the root app, so +existing `KeyError` / `ValueError` handlers in `spark_executor.server` are +unaffected. + +## Configuration + +- **`FILES_MCP_ROOT`** (required env var): absolute path to the sandbox root. + Must exist and be a directory at process start, otherwise `RuntimeError` + halts startup with a clear message. (Mirrors the way `SPARK_EXECUTOR_DATA_DIR` + is documented in the project plan.) +- **No new dependencies.** All operations use stdlib (`pathlib`, `shutil`, + `tempfile`, `os`, `os.replace`). +- **No config file.** A single env var is enough; multi-tenant per-user roots + is explicitly out of scope. + +## Integration with `main.py` + +One import, one new entry: + +```python +# main.py +from files_mcp import app as files_app + +MCP_SERVICES: list[McpService] = [ + McpService(name="spark_executor", app=spark_executor_app, mount_path="/spark-executor-mcp"), + McpService(name="files", app=files_app, mount_path="/files-mcp"), +] +``` + +The existing `lifespan` already iterates `MCP_SERVICES` and calls +`init_mcp_server(svc.app).mount_http(...)` for each. No edit to the lifespan +function. + +## Backward Compatibility + +- **0 existing tools changed.** +- **0 existing tests changed** (only `conftest.py` gains a new autouse fixture + for the files_mcp tests; spark_executor tests are unaffected). +- **0 new dependencies.** +- **API change is purely additive**: the new `/files-mcp` endpoint exists; + `/spark-executor-mcp` is byte-for-byte identical. +- The existing `GUNICORN_WORKERS=1` warning now covers both mounts, because + both share the root app process. No edit to `gunicorn.conf.py` is needed — + the comment block there already says the warning is process-wide. + +## Testing + +### `tests/unit/test_path_guard.py` — 7 cases + +- `tmp/a.txt` → accept, resolved equals expected. +- `../etc/passwd` → reject. +- `/etc/passwd` (root not `/`) → reject. +- `tmp/../etc/passwd` → reject. +- `~/outside` (root not under home) → reject. +- symlink `tmp/link → /etc/passwd` → reject (resolve follows link). +- empty / NUL → reject. + +### `tests/unit/test_fs_ops.py` — ~10 cases + +- create: success / overwrite=True / overwrite=False + existing → `FileExistsError`. +- read: success / directory path → `ValueError` / >1MB → `ValueError` / non-utf-8 bytes → `ValueError`. +- update: success / missing → `KeyError`. +- delete: success / missing → `KeyError` / dir without recursive → `ValueError`. +- list_dir: single / recursive / depth cap. +- search: simple glob / recursive / zero matches. +- move/copy: same-fs rename / cross-fs (non-atomic but allowed) / dst exists → `FileExistsError`. + +### `tests/unit/test_files_tool.py` — ~6 cases + +- Business function calls `path_guard`; escape attempt → `ValueError`. +- DEBUG log line emitted. +- Returns Pydantic-serialized dict. + +### `tests/integration/test_files_mcp_routes.py` — ~12 cases + +`TestClient(files_app)`, one happy-path POST per tool (9), plus error +coverage: 404 (read missing), 400 (escape), 400 (>1MB), 409 (create hit +existing). Mirrors `tests/integration/test_mcp_routes.py` style for +`spark_executor`. + +### `tests/conftest.py` addition + +```python +@pytest.fixture(autouse=True) +def _isolate_files_root(monkeypatch, tmp_path): + """Point files_mcp at a fresh tmp_path for every test.""" + monkeypatch.setenv("FILES_MCP_ROOT", str(tmp_path)) + from files_mcp.core import path_guard + path_guard.set_root_for_testing(tmp_path.resolve()) + yield +``` + +Total expected: existing 242 tests + ~36 new, full suite under 5 s. + +## Risks + +1. **Symlink-based escape.** Mitigated by `.resolve()` following the link + before `relative_to` runs. Verified by the symlink test case. +2. **Write race between agents.** Two writers calling `create_file` for the + same path in the same process can still both succeed (each holds a different + `tempfile`); the loser wins the `os.replace`. This is acceptable for the + stated usage (single agent per process) and explicitly called out in + Non-Goals. +3. **`FILES_MCP_ROOT` misconfiguration.** If the env var points at `/`, + any path is "inside" the sandbox. The startup-time check that it is a + directory is the only guard. Document in the README that operators must + choose a narrow root. +4. **Pydantic model drift.** Mirrors existing patterns, low risk. +5. **MCP session affinity.** New endpoint shares the existing constraint + (`GUNICORN_WORKERS=1`). The existing `gunicorn.conf.py` warning already + fires for any workers > 1 regardless of which mount the request targets. + +## Open Questions + +None — all five clarifying questions resolved before this spec was written +(scope=sandbox; ops=CRUD+list+stat+search+move+copy; content=utf-8 only; +write=atomic+strict create/update; naming=`feat/files-mcp` + `/files-mcp` + +`files_mcp/`). + +## Implementation Order (preview) + +This is a preview only — the full plan comes from the `writing-plans` skill. + +1. `files_mcp/__init__.py` + `core/path_guard.py` + `tests/unit/test_path_guard.py` +2. `files_mcp/core/fs_ops.py` + `tests/unit/test_fs_ops.py` +3. `files_mcp/models.py` + `tools/requests.py` + `tools/files.py` + + `tests/unit/test_files_tool.py` +4. `files_mcp/server.py` + `tests/integration/test_files_mcp_routes.py` +5. `main.py` one-line append +6. `tests/conftest.py` autouse fixture +7. Run full suite; commit. + +## Acceptance Criteria + +- [ ] `uv run pytest` passes (existing 242 + new ~36). +- [ ] `uv run main.py` starts cleanly with `FILES_MCP_ROOT` set; fails with a + clear `RuntimeError` if it is unset or points at a non-directory. +- [ ] `tools/list` against `/files-mcp` returns 9 tools with their descriptions. +- [ ] End-to-end smoke: a `TestClient` POST to `create_file` followed by + `read_file` returns the same content. +- [ ] `git diff main` shows changes only in: new `files_mcp/` package, new + tests, the one-line append in `main.py`, and the conftest fixture addition. +- [ ] No new top-level dependencies in `pyproject.toml`.