Skip to content

[MCP] Add EEGPrep agent server - #233

Closed
suraj-ranganath wants to merge 1 commit into
developfrom
feature/eegprep-mcp-agent-server
Closed

[MCP] Add EEGPrep agent server#233
suraj-ranganath wants to merge 1 commit into
developfrom
feature/eegprep-mcp-agent-server

Conversation

@suraj-ranganath

Copy link
Copy Markdown
Member

Add an optional eegprep-mcp entrypoint backed by reusable MCP tool helpers for dataset inspection, validation, CLI schema discovery, bundled agent guidance, and guarded JSON command execution. The server reuses the existing CLI contract, requires explicit write/overwrite approval for mutating commands, and documents the workflow for researchers using AI agents.

@claude

claude Bot commented Jun 20, 2026

Copy link
Copy Markdown

Claude finished @suraj-ranganath's task in 3m 30s —— View job


Code review

  • Overall assessment: Safe to merge. Well-structured, conservative safety model, good test coverage.
  • Highest-risk area: The write/overwrite gating logic in mcp/tools.py (_command_writes / WRITE_FLAGS) — this is the security boundary. I traced every writing command against it and it holds up.
  • Merge recommendation: Safe to merge.

Blocking

None.

Important

None.

Nits

  • server._call catches Exception, not BaseException (src/eegprep/mcp/server.py:167). tools.run_cli_command invokes cli_main, and an argparse code path that raises SystemExit would bypass the structured-error wrapper. In practice this is very unlikely — the allowlist requires an explicit subcommand, --json is enforced, and both the root and subcommand parsers override error() to raise EEGPrepCLIError — so I'm flagging this only as a low-priority robustness note, not a real defect.

Test gaps

  • No happy-path test that allow_write=True actually lets a write command through the gate. test_run_cli_command_requires_json_and_write_confirmation (tests/test_mcp_tools.py:106) confirms the gate rejects writes without approval, but nothing confirms that approval permits execution. A regression that made allow_write=True still raise WRITE_CONFIRMATION_REQUIRED would go uncaught. Concrete suggestion — add a test that runs a real write to tmp_path and asserts it executes and produces output:
    def test_run_cli_command_executes_write_with_approval(tmp_path):
        out = tmp_path / "resampled.set"
        payload = tools.run_cli_command(
            ["resample", str(SAMPLE_DATASET_PATH), "--freq", "64", "--output", str(out), "--json"],
            allow_write=True,
        )
        assert payload["status"] == "ok"
        assert payload["exit_code"] == 0
        assert out.exists()

EEGLAB parity notes

None — this PR is an agent/CLI integration layer with no EEG data-structure or numerical behavior. It correctly reuses the existing CLI contract (discovery, dataset, core) rather than introducing a parallel one, and the bundled skill/docs stay standalone (no MATLAB or vendored EEGLAB runtime dependency), consistent with AGENTS.md.


What I checked and liked

  • Write-safety model is conservative and correct. I verified each mutating path is gated: transform commands (resample/rereference/filter/clean/epoch/ica/report) are unconditionally in WRITE_COMMANDS; pipeline run and batch are gated even when output comes from YAML (no flag present) via the special cases in _command_writes; config-driven writers and migrate convert-script --output / qc report --html are caught through WRITE_FLAGS. --dry-run and pipeline plan/validate correctly classify as read-only.
  • ALLOWED_COMMANDS matches the real CLI subparsers (capabilities, schema, examples, skills, inspect, validate, etc. all exist in cli/main.py).
  • Tooling/contract reuse is good — no second agent contract; _call produces eegprep.error.v1-shaped errors; in-process execution avoids shelling out.
  • Bounded inspection (_bounded_records) preserves the original count while truncating, so the truncation signaling (returned/truncated) is accurate.
  • .notes/implementation-notes.html is intentionally whitelisted in .gitignore, so its inclusion is fine.
    · feature/eegprep-mcp-agent-server

@suraj-ranganath

Copy link
Copy Markdown
Member Author

🤖 Specification

Problem: EEGPrep already has a strong CLI for agents, but MCP clients had no first-class, version-matched way to inspect EEG datasets, discover EEGPrep command contracts, retrieve agent guidance, or run safe JSON commands without scraping terminal help or shelling out manually.

Approach: Added an optional eegprep[mcp] extra and eegprep-mcp entrypoint. The reusable tool layer in src/eegprep/mcp/tools.py exposes capabilities, the MCP agent guide, bounded sample-data inspection, validation, CLI schemas/examples, command planning, and guarded CLI execution. The FastMCP adapter in src/eegprep/mcp/server.py registers those as MCP tools, resources, and a preprocessing planning prompt while keeping the MCP SDK optional.

Key code: run_cli_command() builds an allowlisted command plan, requires --json, blocks file-writing commands until allow_write=True, blocks --overwrite until allow_overwrite=True, then calls eegprep.cli.main.main(arguments) in-process under captured stdout/stderr. No shell execution is used.

Tests: Added tests/test_mcp_tools.py covering capabilities, bundled MCP skill discovery, sample-data summary/events/channels/ICA inspection, dataset validation, command schema/example delegation, read/write/overwrite planning, guarded execution, non-EEGPrep command rejection, FastMCP registration, structured error conversion, and module help/version output. Also ran CLI adjacency tests and docs build.

Copy link
Copy Markdown
Member Author

🤖 Re-audited at e88f5c0c. Looks good. The MCP layer reuses the existing CLI and dataset contracts, keeps the SDK optional, avoids shell execution, and conservatively gates the current file-writing and overwrite paths. Documentation, lint, and the supported-platform test matrix are green.

@arnodelorme

Copy link
Copy Markdown
Collaborator

🤖 Closing without merging for now.

Since this PR was opened, Claude Desktop's Code tab (local environment) and Cowork can run shell commands, install packages, and read/write connected local files directly, with built-in per-command permission prompts. For Claude users that covers what this server offers: the existing agent CLI (eegprep inspect --json, schema, examples, skills) is already reachable there without an MCP layer, and the approval gating is provided by the host.

The remaining audience for an MCP server is the Chat tab and third-party MCP clients. If that demand materializes, this branch is a good starting point since the tools are thin wrappers over the CLI. Before reopening, please address:

  • eegprep_run_cli_command does not catch SystemExit, so --help escapes the tool call (argparse calls parser.exit()).
  • server._call re-implements the error payload that EEGPrepCLIError.to_response() already builds.
  • mcp is added to the dev dependency group (about 24 transitive packages, +700 lines in uv.lock); keep it as the optional [mcp] extra only.
  • The .notes/implementation-notes.html edit conflicts with Keep local notes out of the repo #234, which removed that file.

Review notes from this round: tool functions test cleanly against real sample data, no shell/eval/exec usage, write paths are gated as described, and importing eegprep does not import the mcp SDK.

@arnodelorme arnodelorme closed this Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants