# 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`.