docs(files-mcp): design spec for sandboxed file CRUD MCP service

Adds docs/superpowers/specs/2026-06-30-files-mcp-design.md describing a sibling FastAPI sub-app (mount: /files-mcp) that exposes 9 file operations (create/read/update/delete/list_dir/stat/search/move/copy) over fastapi-mcp, sandboxed to FILES_MCP_ROOT with utf-8 text only and atomic writes. Branch: feat/files-mcp Mount path: /files-mcp New package: files_mcp/ Backward compat: 0 existing tools / tests / dependencies changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-06-30 17:24:19 +08:00
parent 9305ac750b
commit 254cbe9d7a
1 changed files with 384 additions and 0 deletions
@@ -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`.