diff --git a/ROADMAP.md b/ROADMAP.md index 994b3042f..02a6baefd 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1177,3 +1177,4 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [118-auto-rescan-on-change](./specs/118-auto-rescan-on-change/) | — | — | | [118-pin-visibility-boundary](./specs/118-pin-visibility-boundary/) | — | — | | [119-scan-default-trust](./specs/119-scan-default-trust/) | — | — | +| [120-mcp-router-import](./specs/120-mcp-router-import/) | `drafted` | 0/66 (0%) | diff --git a/specs/120-mcp-router-import/checklists/requirements.md b/specs/120-mcp-router-import/checklists/requirements.md new file mode 100644 index 000000000..181ece923 --- /dev/null +++ b/specs/120-mcp-router-import/checklists/requirements.md @@ -0,0 +1,34 @@ +# Specification Quality Checklist: MCP Router Importer + +**Purpose**: Validate specification completeness and quality before planning +**Created**: 2026-10-20 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) beyond the existing CLI/REST surfaces the feature must reuse +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders (the mapping table and `research.md` carry the technical detail) +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain (decisions self-answered, see Clarifications) +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- The one real technical decision (a read-only SQLite reader, since `go.mod` has none) is deliberately left to plan.md; the spec only requires "read a live database without modifying it" (FR-004, FR-005). diff --git a/specs/120-mcp-router-import/contracts/cli.md b/specs/120-mcp-router-import/contracts/cli.md new file mode 100644 index 000000000..ef228417f --- /dev/null +++ b/specs/120-mcp-router-import/contracts/cli.md @@ -0,0 +1,60 @@ +# CLI contract: `mcpproxy import` + +```text +mcpproxy import --from [--path ] [--dry-run] [--workspace ] + [--yes] [--secrets-inline] [-o table|json|yaml] +``` + +| Flag | Meaning | +|------|---------| +| `--from` | required; `mcp-router` (unknown values: error listing supported sources) | +| `--path` | data directory, `mcprouter.db`, or an exported `mcp-servers-*.json`; default: auto-detect per OS | +| `--dry-run` | plan only: no add, no keyring write, no telemetry | +| `--workspace` | limit to one local workspace | +| `--yes` | skip the single confirmation prompt (never implies `--secrets-inline`) | +| `--secrets-inline` | allow secret-like values and inline credentials to stay in the config file when the keyring is unavailable or cannot hold them | +| `--help-json` | machine-readable help (existing mechanism) | + +No `--no-quarantine`. + +## Default locations checked (and printed when none exist) + +| OS | Path | +|----|------| +| macOS | `~/Library/Application Support/MCP Router` | +| Windows | `%APPDATA%\MCP Router` | +| Linux | `~/.config/MCP Router` (best effort) | + +## Output (`-o json`; the table shows the same content) + +```json +{ + "source": "mcp-router", + "input": {"kind": "data_dir", "path": "...", "workspaces": ["Local"]}, + "dry_run": true, + "summary": {"total": 12, "to_import": 8, "needs_configuration": 1, "already_present": 1, + "conflict": 0, "collision": 0, "unmappable": 2, "skipped": 0, "added": 0, "failed": 0}, + "servers": [ + {"name": "fs", "source_name": "fs", "workspace": "Local", "class": "to_import", + "kind": "stdio", "summary": "npx -y @modelcontextprotocol/server-filesystem /tmp", + "enabled": true, "hidden_tools": ["write_file"], + "secrets": [{"field": "env:API_KEY", "secret_like": true, "handling": "keyring"}], + "not_carried_over": ["auto_start", "project"], "warnings": []} + ], + "not_imported": {"workflows": true, "hooks": false, "skills": true, "logs": true, "cloud_workspaces": 0}, + "reconnect": [{"client": "cursor", "config_path": "~/.cursor/mcp.json", "command": "mcpproxy connect cursor"}, + {"client": "cline", "note": "MCPProxy has no Cline connector; re-point Cline's MCP settings manually"}], + "warnings": [], + "exit_class": "success" +} +``` + +Never present: any env/header/token/arg value, any `MCPR_TOKEN`. `servers[].summary` is the importer-owned summary (plan D4): fixed `‹redacted›` masks for sensitive argument values and for every URL query value and fragment; no tail bytes, no lengths. + +## Exit codes + +0 success · 20 partial · 21 nothing added · 22 source not found · 23 source unreadable/inconsistent · 24 refused by secret rules. (Plan D8.) + +## Interaction + +Interactive run: print the plan, then `Add N servers (X secrets to keyring, Y inline)? [y/N]`. Non-interactive without `--yes`: exit 24 with the message. After a real run: print the reconnect list and `mcpproxy upstream list`/review hints. diff --git a/specs/120-mcp-router-import/contracts/rest.md b/specs/120-mcp-router-import/contracts/rest.md new file mode 100644 index 000000000..6e4ac4d9f --- /dev/null +++ b/specs/120-mcp-router-import/contracts/rest.md @@ -0,0 +1,20 @@ +# REST contract additions + +## `POST /api/v1/servers/import/report` (new) + +Auth: API key + `requireServerOp(auth.ServerOpAdd, ...)`. + +Request: `{"source": "mcp-router"}` — `source` must be in the closed enum (`mcp-router`; extensible). No count is sent. +Responses: `204` for a valid request whether or not an event was recorded (the response never reveals opt-out state); `400` for an unknown `source`. +Effect: `Service.RecordImportRun(source)` evaluates event-time telemetry eligibility under the service's own synchronisation (config plus env overrides) and, if eligible, increments the run counter for that source. + +## `POST /api/v1/servers` (extended) + +`AddServerRequest.disabled_tools []string` (create only), applied to the new server's `DisabledTools`. `enabled_tools` is not accepted on this surface. Documented in `docs/api/rest-api.md` and `oas/swagger.yaml`; no change for callers that omit it. + +## Web UI phase (P5, non-gating) + +New endpoints, so no existing contract changes: + +- `GET /api/v1/servers/import/mcp-router` → `{found, input_kind, path, workspaces:[{id,name}], server_count}` (stat plus a snapshot count under the same rules). +- `POST /api/v1/servers/import/mcp-router` body `{path?, workspace?, apply:false|true, secrets:"keyring"|"inline", inline_credentials_ok:false|true, servers?:[names]}` → plan view (same `ItemView` DTO as the CLI JSON) when `apply:false`, outcomes when `apply:true`. Same authorization as other server-adding operations. Records telemetry in-process through `Service.RecordImportRun`. Never bypasses quarantine; placeholder rejection (FR-006b) applies before any destination reference is inserted. diff --git a/specs/120-mcp-router-import/data-model.md b/specs/120-mcp-router-import/data-model.md new file mode 100644 index 000000000..c111db3b4 --- /dev/null +++ b/specs/120-mcp-router-import/data-model.md @@ -0,0 +1,58 @@ +# Data Model: MCP Router Importer + +No new persisted state. All types are in-memory, in `internal/mcprouter`. + +## Input + +| Type | Fields | Notes | +|------|--------|-------| +| `Input` | `Kind` (`data_dir`\|`database`\|`export_file`), `Root`, `Files []string` | Output of `Locate`. `Files` are already containment-checked (plan D3). | +| `Workspace` | `ID`, `Name`, `Type` (`local`\|`remote`), `DBPath`, `Order` | `remote` workspaces have no rows; counted as "not imported". Default workspace (`local-default`) sorts first. | + +## Source (immutable, one per `servers` row or export entry) + +| Field | Source column | Missing-column default | +|-------|---------------|------------------------| +| `Workspace` | workspace id | `local-default` | +| `Name` | `name` | required, else row failed | +| `Kind` | `server_type` | `local` | +| `Command`, `Args` | `command`, `args` (JSON) | empty | +| `Env` | `env` (JSON object, plaintext) | `{}` | +| `RemoteURL` | `remote_url` | empty | +| `Bearer` | `bearer_token` | none; `BearerState` = `none`\|`plain`\|`encrypted` (prefix `mcpr-secret:v1:`) | +| `Disabled` | `disabled` | false | +| `AutoStart`, `ProjectID`, `Description`, `Version`, `Verification` | respective | unset (reported as not carried over when set) | +| `Params` | `input_params` (JSON) | none | +| `ToolOff []string` | keys of `tool_permissions` with `false` | none | +| `FromExport bool` | — | export entries lack Disabled/ToolOff/Bearer/Kind info | + +## Plan (output of `BuildPlan`, pure, no side effects) + +| Type | Fields | +|------|--------| +| `Plan` | `Input`, `Items []Item`, `NotImported` (workflows, hooks, skills, logs, cloud workspaces: booleans/counts from table presence only), `Clients []ClientAdvice`, `Warnings []string` | +| `Item` | `Source`, `Class`, `Reason`, `Server *config.ServerConfig` (nil unless addable), `Secrets []SecretPlan`, `NotCarried []string` (field names), `Differs []string` (conflict, field names only) | +| `Class` | `to_import`, `needs_configuration`, `already_present`, `conflict`, `collision`, `unmappable`, `skipped_self`, `filtered_out` | +| `SecretPlan` | `Field` (`env:NAME`\|`header:NAME`\|`arg:INDEX`), `Handling` (`keyring`\|`inline`\|`missing`\|`blocked_inline_credential`), `SecretLike bool` — never a value | +| `ClientAdvice` | `Client` (MCP Router id), `ConfigPath`, `Command` (`mcpproxy connect ` or empty), `Note` | + +### State transitions of an Item + +```text +read ─▶ resolve placeholders ─▶ validate (name, URL, command, `${`) ─▶ classify + unmappable ◀──────────── any failure + already_present / conflict / collision ◀── compare against existing servers (FR-016a) + needs_configuration ◀── encrypted bearer + to_import ◀── otherwise +apply: to_import|needs_configuration ─▶ added | failed(reason) +``` + +## Outcome (output of `Apply`) + +`ServerOutcome{Name, Status(added|failed|uncertain), Reason, Enabled, Quarantined (read back, or `unknown`), OrphanRefs []string}`. `Classify(plan, outcomes)` gives the exit class; only `added` outcomes make a run telemetry-eligible. + +`ItemView` is the only type that is ever rendered or serialised: it has no value-bearing fields; `Plan`/`Item` (which hold `*config.ServerConfig` with raw values) are never marshalled. + +## Telemetry + +`CounterRegistry.importCounts map[ImportSource]int64` (new), payload `import_source_counts: {"mcp-router": N}` — integer counts of runs, fixed key set, omitted when empty. diff --git a/specs/120-mcp-router-import/plan.md b/specs/120-mcp-router-import/plan.md new file mode 100644 index 000000000..843f82449 --- /dev/null +++ b/specs/120-mcp-router-import/plan.md @@ -0,0 +1,197 @@ +# Implementation Plan: MCP Router Importer + +**Branch**: `claude/spec-mcp-router-import-7ae3fb` (spec dir `120-mcp-router-import`) | **Date**: 2026-10-20 | **Spec**: [spec.md](spec.md) +**Input**: spec.md (clarified, two Sol review rounds applied), research.md (verified MCP Router format) + +## Summary + +Add `mcpproxy import --from mcp-router [--path] [--dry-run] [--workspace] [--yes] [--secrets-inline]`. A new package `internal/mcprouter` locates MCP Router's data, takes a verified private snapshot of its SQLite database (or reads an export file), maps each server onto `config.ServerConfig` using the existing `internal/configimport` name sanitiser, validator and secret classifier, and produces a **Plan** (to-import / skipped / conflict / unmappable / needs-configuration, per-field handling, secret handling, clients to reconnect). The CLI renders the plan, asks one confirmation, stores secret-like values in the OS keyring, adds servers one at a time through the daemon (or the config file when no daemon runs), and reports the true per-server outcome. The daemon records one closed-enum telemetry count (`import_source`) when told that a run committed at least one addition. A Web UI entry reuses the same package through the existing import endpoints and is the last, droppable phase. + +`upstream import` is **not** changed. + +## Technical Context + +**Language/Version**: Go 1.26 (existing). Vue 3.5 / TypeScript 5.9 for the optional Web UI phase. +**Primary Dependencies**: Existing: `internal/configimport`, `internal/secret`, `internal/cliclient`, `internal/telemetry`, `internal/config`, cobra. **One new dependency: `modernc.org/sqlite` (pure Go, no CGO)**, imported only by `internal/mcprouter`. Justified in D1. +**Storage**: Reads MCP Router's SQLite (read-only, via private snapshot). Writes MCPProxy config through the existing add path. Keyring through `internal/secret`. +**Testing**: `go test -race` unit tests with generated SQLite fixtures (built in-test with the same driver, so no committed binary files), table-driven mapping tests, CLI tests with the existing `cmd/mcpproxy` harness, httpapi handler tests, telemetry payload/privacy tests, one E2E script step. +**Target Platform**: macOS, Windows (MCP Router's real platforms), Linux (best effort). All three are existing release targets; `modernc.org/sqlite` supports them and needs no CGO, so cross-compilation is unchanged. +**Project Type**: single Go module plus embedded frontend. +**Performance Goals**: SC-002, 50 servers in < 5 s each for dry-run and apply (dominated by the snapshot copy and keyring writes). +**Constraints**: never write to MCP Router's files; never print/log/telemeter a secret; quarantine always set; no behavior change to `upstream import`; no new global state. +**Scale/Scope**: tens of servers per user; up to hundreds must stay readable. + +## Constitution Check + +| Principle | Status | +|-----------|--------| +| I Performance at scale | Pass. Offline one-shot command; no hot-path impact. Binary-size cost of D1 is measured and gated. | +| II Actor-based concurrency | Pass. Sequential command; no new shared mutable state. Telemetry uses the existing registry. | +| III Configuration-driven | Pass. Servers land in `mcp_config.json` via the existing add path; no new config keys. | +| IV Security by default | Pass, and the core of the design: quarantine always set (FR-012), secrets to keyring (FR-011), untrusted-source handling (FR-004a, FR-006b, FR-014), no decryption of foreign credentials (A-4). stdio isolation (Docker) is applied by the normal runtime path to the imported servers, unchanged. | +| V TDD | Pass. Each task pair is test-first; fixtures built in test. Lint with both CI invocations (bare and `--build-tags server`). | +| VI Documentation hygiene | Tasks include docs page, CLI docs, telemetry doc, REST docs/swagger, CLAUDE.md Recent Changes line. | + +Post-design re-check: the only deviation is the new dependency (D1), documented with measurement and an exit hatch. + +## Decisions + +### D1. SQLite reader: `modernc.org/sqlite`, isolated, with a measured gate + +Options: (a) `modernc.org/sqlite` (pure Go); (b) shell out to `sqlite3` (absent on Windows, so unusable for half of MCP Router's users); (c) a hand-written reader of the SQLite file format including WAL replay; (d) export-file only. + +Chosen: **(a)**. Measured on 2026-10-20 (`go1.26`, darwin/arm64, unstripped): a hello-world binary grows from 2.5 MB to 9.4 MB with the driver, +6.9 MB, against the current 46.9 MB `mcpproxy` build, +15% unstripped; the release build is stripped, so the real delta is smaller. Rejected (c): a format reader with WAL replay is ~600 lines of subtle code whose failure mode is silently wrong data. Rejected (d) as the primary path: the export drops remote servers (research §3), which is the main value. + +Gate (task T003): measure the **stripped** release-binary delta on linux/amd64, darwin/arm64 and windows/amd64. If any exceeds 10 MB, stop and escalate; the fallback is shipping (d) alone (Story 6 and the export path need no driver) and keeping the direct-DB reader behind a build tag. The dependency is imported by exactly one package so that fallback is a build-tag change. + +### D2. Snapshot protocol (FR-005) + +All source files are opened through an `os.Root` (Go 1.24+) rooted at the validated data directory (D3), so a path can neither leave the root nor be swapped for a symlink that does. Copies are made **from the open descriptors**. + +1. For each attempt: open `mcprouter.db` (or the named workspace db), its `-wal` (if present) and `-shm` (if present) via the root; `Fstat` each and require a regular file; record identity (`os.SameFile` handle plus size, mtime) and which sidecars existed. +2. Enforce limits before copying: total bytes of db+wal ≤ **1 GiB** (otherwise `ErrSourceTooLarge`, advice: use the export file; MCP Router also stores request logs in these databases). +3. Copy the descriptors into a `0700` directory from `os.MkdirTemp`; then re-`Fstat`/re-stat the *names* through the root and compare to step 1. Any difference (size, mtime, different file identity, a sidecar appeared, vanished or was replaced, i.e. checkpoint/truncate/recreate) discards the copy and retries. +4. Open the copy read-only with a context carrying a 30 s deadline, run `PRAGMA quick_check`; failure counts as a failed attempt. +5. Up to 3 attempts, then `ErrNoConsistentSnapshot` (exit class (e)) with advice to close MCP Router or use the export file. +6. The temp directory is removed on every exit path including SIGINT (the command's signal-aware context). + +`quick_check` establishes structure, not generation consistency; the pre/post identity comparison plus the retry bound is the consistency argument, and "a torn snapshot is never read as consistent" is enforced by tests that rewrite, checkpoint, truncate and replace the WAL during the copy. SQLite's online backup or `VACUUM INTO` would be stronger but opens the live database (it may create `-shm`), which FR-005 forbids. + +### D3. Path containment and hostile input (FR-004a, FR-007, FR-014) + +`Locate(path)` returns an `Input{Kind, Root, Files}`. Kinds: `data_dir`, `database`, `export_file`. Root handling: for `data_dir` the root is the directory itself (canonicalised once with `EvalSymlinks` at the start, which is the user's own `--path` choice, not source data); for `database` the root is its parent directory and the only candidates are that file plus sidecars; for `export_file` no root, the single file is read once with a 4 MiB cap. + +Everything after that goes through `os.OpenRoot(root)`: containment is component-aware and symlink-escape-resistant by construction, with no string-prefix test and no check-then-open gap. Workspace databases: the workspace list comes from the `workspaces` table of the snapshot of `mcprouter.db`, but `localConfig.databasePath` is untrusted data; a candidate is accepted only if it is exactly `mcprouter.db` or `workspaces//database.db` with `` matching `[A-Za-z0-9._-]{1,64}` and not `.`/`..`. Others produce warning `workspace_path_rejected` (id only) and nothing is opened. If the `workspaces` table is absent (very old databases), fall back to one workspace `local-default` backed by the file given. + +**Bounds** (all before materialising data, all cancellable via context): ≤ 5,000 servers; name ≤ 256 bytes; every other string field ≤ 64 KiB; `args` ≤ 1,024 entries; JSON columns decoded with a size cap and depth ≤ 8; SQL queries read with a 30 s deadline. Over-limit rows are failed individually (`row_too_large`). Remote URLs must parse as `http`/`https` with a host (other schemes → unmappable, a check added by this importer because `ValidateServerForLoad` does not do it). Names and any other source-controlled text rendered to a terminal are escaped (`strconv.Quote` style) so control characters and ANSI sequences cannot reach the screen. `command`/`args` paths are never opened. + +### D4. Secret handling (FR-006a, FR-009 to FR-011, FR-017) + +- **Provenance from every branch.** `ResolveArgs` returns, for each argument slot, where the value came from: literal, parameter default (with the parameter's `sensitive` flag and name classification), or env (with that env name's classification). A slot is `sensitive` when its source was a `sensitive` parameter, a secret-like-named parameter, **or a secret-like-named env var**, so `args=["${API_KEY}"]` with `env.API_KEY="ordinary-looking-secret"` and no schema is still protected, masked and compared as sensitive. +- **Classification** reuses the `configimport` name-based classifier through new exported wrappers (`ClassifyEnv`, `ClassifyHeaders`; `Reclassify` is already exported, `describeFields` is not). Plain-text bearer tokens are always secret-like. Parameters marked `sensitive`, or whose name classifies secret-like, carry **provenance**: each argument slot produced from such a parameter is marked `sensitive` in the plan. +- **Output never serialises the internal plan or `configimport.ImportedServer`** (its `Server` field holds raw values). The CLI/JSON/Web output is built from a separate `ItemView` DTO that has no value fields. Its `summary` is generated by the importer and does **not** use `configimport`'s summary (which relies on `oauth.LiveRedaction`, whose masks keep trailing bytes and length): command basename, then each argument rendered as-is *unless* it sits in a `sensitive` slot or the existing redactor would alter it, in which case the **whole argument value** is replaced with the fixed mask `‹redacted›`; for remote servers the URL is shown as scheme, host and path with userinfo dropped and **every query value and the fragment** replaced by `‹redacted›` (fixed mask, no tail bytes, no length). This also applies to entries blocked as inline credentials. Canary tests use innocuously named parameters and positional values to prove it (SC-004). +- **Reference names** are `mcpr-import--` where `` is the first 16 hex chars of `sha256(destinationServerName 0x00 kind 0x00 key)`; `kind` is `env`/`header`/`arg`. Fixed length (no 64-character truncation problem), unique per destination server and field (so no allocation collision inside a run), deterministic (re-run reuses them, FR-017), namespaced by the `mcpr-import-` prefix. This deliberately does not use `secret.RefName`, whose truncation and collision-suffixing are wrong for this purpose. +- **Ownership and overwrite.** Before writing a reference, `Resolve` any existing value and remember it. A reference is only ever overwritten if it carries our prefix. Rollback restores the remembered prior value, or deletes the entry when there was none, and touches nothing else. +- **Per-server protocol, serialised.** The whole apply phase runs under one cross-process **import lock** (`import.lock` in the config directory, `O_CREATE|O_EXCL`, stale-lock detection by pid and age, released on every exit path), in both sink modes, so two importers can never interleave their keyring writes, adds and compensations. Under the lock, for each server: re-check that the name is still absent (a concurrent import may have added it: then it is `already_present`, and **no reference is touched**); mark each reference "possibly written" *before* `Store` (a timed-out store may still complete, and a store can succeed before its registry update fails); `Store` all; add the server. + - **Definite rejection** (an HTTP response carrying an error status, or the file save returning an error before anything was renamed into place): compensate this server's references only. + - **Anything else** (transport error, timeout, unreadable or undecodable response): the daemon's create transaction does not observe the request context, so absence on a later read does **not** prove the add will not still commit. The outcome is `uncertain`: references are kept, never deleted; a later read that finds the server present upgrades it to `added`; one that finds it absent leaves it `uncertain`, with the reference names listed and the advice to re-run (a re-run is idempotent: it finds the server or adds it and reuses the same deterministic references). + - Compensation errors and timeouts are reported as indeterminate cleanup (names only). +- **Keyring unavailable** (`KeyringAvailability()`), non-dry run, secret-like values present, no `--secrets-inline`: stop before any write, exit class (f). +- **macOS gate.** Outside the tray, `internal/secret` refuses keyring writes on macOS unless `MCPPROXY_KEYRING_WRITE=1` (a maintainer safety gate against the "Keychain Not Found" modal; `KeyringAvailability()` returns false with that reason). MCP Router's users are mostly on macOS with API keys in env, so a plain-terminal first run would hit exit 24. The importer does **not** override the gate (assumption O1). The plan view and the refusal both print the exact remedy (`MCPPROXY_KEYRING_WRITE=1 mcpproxy import ...` or `--secrets-inline`) and the unavailable reason verbatim. If this proves the main drop-off in the experiment, a follow-up can ask the running daemon (which the tray starts opted in) to store the secrets over REST. Same behavior as `upstream add --secret-env`. +- **Inline credentials** (a token in a URL query, `--api-key=...` in an argument; detected with the same classifier the import preview uses) are listed as `blocked_inline_credential` and block the server unless `--secrets-inline`. +- **Source `${...}`** after placeholder resolution makes the entry unmappable (FR-006b). The importer's own `${keyring:...}` references are inserted afterwards, by construction, never by re-scanning source text. + +### D5. Placeholder resolution (FR-006) + +`ResolveArgs(args, env, params)` is a direct port of `substituteArgsParameters` as documented in research §2 (whole-argument match only; `${user_config.x}` → param `x` default, then non-empty env `user_config.x`, then param `user_config.x` default; `${x}` → non-empty env `x`, then param `x` default). Table tests cover both placeholder forms, conflicting values, empty env values and partial-string placeholders (left literal, then caught by FR-006b). + +### D6. Apply contract, sinks, daemon discovery (FR-012a, FR-016a) + +- `internal/mcprouter.Apply(ctx, plan, Sink, secrets)` returns `[]ServerOutcome{Name, Status(added|failed|uncertain), Reason, Enabled, Quarantined, OrphanRefs}`. +- **Sink choice is frozen for the run** and decided by a tri-state probe in `import_cmd.go`: *reachable* (daemon answers `GET /api/v1/status` with valid credentials, via the configured endpoint or socket) → `daemonSink`; *definitively absent* (no socket file, connection refused on the default local endpoint) → `fileSink`; *inaccessible* (auth failure, timeout, explicit endpoint that fails) → stop with an error, never fall back to file mode, never switch sinks after an add error. +- `daemonSink` calls `cliclient.AddServer` with `Enabled`, `Quarantined=true` and the new `DisabledTools`, then **reads the state back through the existing `GetServers` projection** (no new REST route; it carries `enabled` and `quarantined`) to fill what is actually stored (FR-012 read-back); the same read serves reconciliation of `uncertain` outcomes. A read-back failure keeps the add result and marks the state `unknown`. +- `fileSink` (daemon definitively absent): runs under the same import lock; per server it **reloads** the config from disk, re-checks the name, appends, saves with `config.SaveConfig`. A failed candidate is discarded, never carried into a later save. Residual limitation, documented: a daemon started *during* the run reloads the file as a replacement. +- `AddServerRequest` in `internal/cliclient` and `internal/httpapi` gains `disabled_tools []string` (create only), applied to `ServerConfig.DisabledTools`. Create-time `enabled_tools` is **not** added on this surface; the documented behavior is simply that only `disabled_tools` is accepted. +- **Plan-time comparison for idempotency (FR-015/016/016a).** Items compare on: transport, command, URL, args, env/header **names**. For argument slots marked `sensitive`, the canonical comparison token is the constant ``; an existing server's argument at that position that is **any** `${keyring:}` reference (importer-generated or operator-renamed) compares equal to it, without resolving values or comparing reference names (FR-016a). An existing literal or any other reference form at that position is a conflict naming `args`. Existing servers whose args differ elsewhere are conflicts naming `args`. +- **Exit classification** is a pure function of plan classes plus outcomes: `Classify(plan, outcomes)`; it does not depend on apply outcomes alone. Telemetry eligibility depends only on outcomes with status `added`. +- `applyImportedServers*` (used by `upstream import`) are untouched; their dropping of `Enabled` and swallowing of add errors is a recorded follow-up issue, out of scope. The new sinks do not reuse them. + +### D7. Telemetry (FR-022 to FR-024) + +- `telemetry.ImportSource` closed enum `{mcp-router, other}` (extensible); `Service.RecordImportRun(src)` is a **service-owned** operation that evaluates event-time eligibility (`EffectiveTelemetryEnabled` plus env overrides) under the service's own synchronisation, the way the heartbeat path does (see the race documented near `telemetry.go:771`), and only then increments `CounterRegistry.importCounts`. The registry counter participates in `Drain()`/`Snapshot()`/`Reset()`/`HasPendingCounters()`. The payload field is `import_source_counts` (object, omitted when empty, keys from the fixed enum, non-negative integer values). `ScanForPII` gets the structural rule `import_source_field_invalid`. `SchemaVersion` takes the next value (16 on main when written, so 17; re-check at implementation time). +- **Schema bump hygiene.** The existing guard test only hunts obsolete v12 spellings. The bump task enumerates the current-version (v16 at the time of writing) pins by `rg` over code, tests, fixtures (`internal/telemetry/testdata`) and docs, updates them, extends the guard to the current to next transition, and leaves historical activation/cohort thresholds alone. Its own commit. +- **Endpoint** `POST /api/v1/servers/import/report` with body `{"source":"mcp-router"}` only (the count of servers is *not* sent: unnecessary, and the daemon client can target a non-loopback endpoint). Auth `requireServerOp(auth.ServerOpAdd, handler)`; unknown `source` returns 400; otherwise 204, whether or not the event was recorded (the response never reveals opt-out state). The handler calls `Service.RecordImportRun`. +- The CLI calls it once, only when the daemon sink was used and at least one outcome is `added`. A daemonless run is not counted (clarified). The Web UI path calls `Service.RecordImportRun` in-process from its own handler. +- Race test under `-race`: opt-out toggled concurrently with recording; disabled → import → enabled emits nothing. + +### D8. Exit codes (FR-021) + +A new band 20-24, assigned centrally in `classifyError` (`cmd/mcpproxy/main.go`) through a typed error `importOutcomeError` (same mechanism as `preflightVerdictError`), documented in `docs/cli-management-commands.md`: + +| Code | Meaning (FR-021) | +|------|------------------| +| 0 | (a) success: all added, nothing to import, already-present only, dry-run planned | +| 20 | (b) partial: at least one added and at least one failed/conflict/unmappable | +| 21 | (c) nothing added although expected | +| 22 | (d) source not found | +| 23 | (e) source unreadable, malformed or no consistent snapshot (also `--dry-run`) | +| 24 | (f) refused by the secret rules | + +### D9. Client reconnect detection (FR-008, FR-019) + +`ClientsUsingMCPRouter()` opens only the six known client config paths, read-only and size-capped at 1 MiB, parses JSON (`mcpServers`/`servers`) or TOML (Codex `[mcp_servers.*]`), and flags an entry as using MCP Router if either (a) it is the bridge: command `npx`, args containing `connect` and an element containing `@mcp_router/cli`; or (b) it is a remote entry whose URL host is `localhost`/`127.0.0.1`/`[::1]` with port **3282** (MCP Router's fixed aggregator port, `apps/electron/src/main.ts`, default URL `http://localhost:3282/mcp` in `apps/cli/src/commands/connect.ts`). The same predicate drives the FR-008 self-skip in server import. It extracts **only** the boolean: entries are decoded into structs that have no `env`/`headers` fields, so `MCPR_TOKEN` and bearer headers are never in memory. Output maps client to `mcpproxy connect ` (`internal/connect` ids); Cline gets the one-line note. + +## Project Structure + +### Documentation (this feature) + +```text +specs/120-mcp-router-import/ +├── spec.md research.md plan.md data-model.md quickstart.md tasks.md +├── checklists/requirements.md +└── contracts/ + ├── cli.md # flags, output JSON, exit codes + └── rest.md # POST /servers/import/report, Web UI additions +``` + +### Source Code + +```text +internal/mcprouter/ # NEW, the only importer of modernc.org/sqlite +├── locate.go # OS default paths, input kind, containment (D3) +├── snapshot.go # stat-copy-verify (D2) +├── reader.go # servers/workspaces/projects tables, tolerant of missing columns +├── export.go # export JSON parser (+ empty-command remote detection) +├── resolve.go # placeholder resolution (D5) +├── plan.go # map → configimport.ImportedServer + classes; conflict compare (FR-016a) +├── clients.go # reconnect detection (D9) +├── apply.go # secrets + Sink + outcomes (D4, D6) +└── *_test.go + testdata/ (JSON only; DB fixtures are built in-test) +internal/configimport/ # small exports: ClassifyEnv/Headers, Sanitize already exported +internal/telemetry/ # ImportSource, RecordImportOn, payload field, scan rule, schema bump +internal/cliclient/client.go # AddServerRequest.DisabledTools, ReportImport() +internal/httpapi/server.go # AddServerRequest.DisabledTools; POST /servers/import/report (requireServerOp) +cmd/mcpproxy/import_cmd.go # NEW cobra command, rendering, prompt, exit mapping +cmd/mcpproxy/exit_codes.go # importOutcomeError + band 20-24 +cmd/mcpproxy/main.go # classifyError learns importOutcomeError; AddCommand(importCmd) +docs/features/mcp-router-import.md # user page; docs sidebar entry +docs/cli/import-command.md docs/features/telemetry.md docs/api/rest-api.md oas/swagger.yaml +frontend/src/components/ImportServersPanel.vue # P3, last +scripts/test-api-e2e.sh # +1 import step +``` + +## Phases + +1. **P0 Gate and skeleton**: dependency measurement (T003), package skeleton, fixture builder. +2. **P1 Read and plan** (no writes): locate, snapshot, reader, export parser, resolver, plan/mapping/conflicts, clients. Milestone is package-test level (the command arrives in P2). +3. **P2 Apply and CLI**: secrets, sinks, `disabled_tools` plumbing, command, prompts, exit codes, JSON output. +4. **P3 Telemetry**: registry, payload, scan rule, schema bump, endpoint, doc. +5. **P4 Docs, E2E and acceptance**: user page, CLI docs, E2E, recorded acceptance checks (SC-001 timed single-confirmation run, SC-002 50-server timing, SC-009 one manual reconnect check per supported client: Claude Desktop, Cursor, Windsurf, VS Code, Codex), QA pass against a real MCP Router 0.6.4 database in a scratch home, and the hand-off to the website session (final command text, flags, docs URL) so the migration page links to and matches the shipped behavior (FR-026). +6. **P5 Web UI (non-gating, droppable)**: its own request shape (contracts/rest.md), detection row, panel entry. Does not alter the existing endpoints' contracts. + +## Test strategy + +- **Fixture builder** (`testdata` helper): creates a real SQLite file with MCP Router's `CREATE TABLE` statements and rows via the driver, including an old schema without later columns, WAL mode with an unflushed `-wal`, an encrypted-looking token (`mcpr-secret:v1:...`), hostile values, and workspace rows pointing outside the directory. +- **Added from review**: symlink-swap tests (database, parent directory, sidecar) via `os.Root`; sibling-directory prefix test (`MCP Router-evil`); WAL checkpoint/truncate/replace during copy; commit-succeeds-response-lost reconcile test; partial `Store` failure and timeout; reference collision and restore-prior-value; concurrent `fileSink` imports; stale lock file; legacy servers-only database; oversized/deeply-nested/control-character/non-http-URL fixtures; mixed-class exit classification table; rerun after sensitive-arg keyring replacement and after operator approval; global quarantine disabled read-back; MCP Router endpoint-style client entry. +- **Properties**: no byte of any canary secret in any output channel; dry-run leaves config, keyring and registry unchanged; re-run is a no-op (config file byte-identical); every plan class is exercised; snapshot retry under a concurrently writing goroutine; temp directory always removed. +- **Mutation checks** (per repo practice): disable the containment check, the `${` check, the quarantine flag and the keyring-first order one at a time and confirm a test fails. +- **Real-data QA**: one run against a database produced by the real MCP Router 0.6.4 app in a scratch home, recorded in the PR, not committed. + +## Risks + +| Risk | Mitigation | +|------|------------| +| Driver size/portability | D1 gate with a build-tag fallback | +| Snapshot of a busy WAL DB | D2 bounded verify loop, fail safe, export-file alternative | +| Keyring behaviors differ per OS; macOS terminal runs are gated off by default | reuse `internal/secret` unchanged; print the exact remedy; tests with a fake provider for available / unavailable / macOS-gated; manual check on macOS and Windows; follow-up path via the daemon if the gate is the main drop-off | +| MCP Router users with 0 servers or only cloud workspaces | explicit "nothing to import" path and message | +| Telemetry schema bump touches many pinned sites | rely on the existing guard test; do the bump as its own commit | +| Scope creep into `upstream import` | out of scope; follow-up issue for its daemon-mode Enabled/error handling | + +## Complexity Tracking + +| Item | Why needed | Simpler alternative rejected because | +|------|-----------|--------------------------------------| +| New dependency `modernc.org/sqlite` | Reading remote servers/disabled flags/tool toggles needs the database | Export-only loses the main value; shelling out fails on Windows | +| New REST report endpoint | Daemon must own the telemetry event and gate on its own opt-out state | CLI-side recording would be lost with the process and ignore the daemon's opt-out | +| Exit-code band 20-24 | FR-021 needs six distinguishable outcomes | Reusing 1-6 would collide with startup meanings | diff --git a/specs/120-mcp-router-import/quickstart.md b/specs/120-mcp-router-import/quickstart.md new file mode 100644 index 000000000..f751382c8 --- /dev/null +++ b/specs/120-mcp-router-import/quickstart.md @@ -0,0 +1,29 @@ +# Quickstart (verification script for reviewers) + +```bash +# Every command targets the scratch instance explicitly +D=$(mktemp -d); C="--data-dir $D --config $D/mcp_config.json" +go test ./internal/mcprouter -run TestFixtureWriteDir -count=1 -args -out $D/mcpr-fixture +./mcpproxy serve $C --listen 127.0.0.1:18921 & # wait for: ./mcpproxy status $C + +# 1. Preview: nothing changes +./mcpproxy import $C --from mcp-router --path $D/mcpr-fixture --dry-run +./mcpproxy import $C --from mcp-router --path $D/mcpr-fixture --dry-run -o json | jq '.summary' + +# 2. Apply (macOS terminal: prefix MCPPROXY_KEYRING_WRITE=1), then re-run: second run adds nothing +./mcpproxy import $C --from mcp-router --path $D/mcpr-fixture --yes +./mcpproxy import $C --from mcp-router --path $D/mcpr-fixture --yes +./mcpproxy upstream list $C # imported servers quarantined; disabled ones disabled + +# 3. Secret rules on a FRESH destination with the keyring unavailable (unit tests use a fake provider; +# manually: run in an environment where KeyringAvailability() is false, e.g. macOS terminal without +# MCPPROXY_KEYRING_WRITE): refuse with exit 24, then opt in +D2=$(mktemp -d); C2="--data-dir $D2 --config $D2/mcp_config.json" +./mcpproxy import $C2 --from mcp-router --path $D/mcpr-fixture --yes; echo $? # 24, nothing written +./mcpproxy import $C2 --from mcp-router --path $D/mcpr-fixture --yes --secrets-inline + +# 4. Export file path +./mcpproxy import $C --from mcp-router --path $D/mcpr-fixture/mcp-servers-2026-09-18.json --dry-run +``` + +Expected: step 1 lists classes and redacted summaries and exits 0; step 2's second run reports `already_present` for every server (config file byte-identical); step 3 exits 24 and writes nothing, then succeeds with the opt-in; canary secrets never appear in any output. diff --git a/specs/120-mcp-router-import/research.md b/specs/120-mcp-router-import/research.md new file mode 100644 index 000000000..933e421a3 --- /dev/null +++ b/specs/120-mcp-router-import/research.md @@ -0,0 +1,169 @@ +# Research: MCP Router configuration format (E4, phase 1) + +**Date**: 2026-10-20 +**Source**: `github.com/mcp-router/mcp-router`, cloned read-only, `main` @ `7d6cd4e5e7e73ef54b938c29d14e0cab64850fae` (2026-09-18, "Remove remaining legacy purchase prompts from final release (#213)"). Last release tag: `v0.6.4` (final). Repo is archived; its README says new adoption is not recommended. + +All paths below are relative to the MCP Router repo root unless prefixed `mcpproxy-go:`. + +## Verdict + +**Importable.** Server definitions live in a plain SQLite file with a small, stable, fully readable schema, and the app ships an export button. Three caveats shape the design: + +1. The one-click **export is lossy**. It drops remote servers' URL, the disabled flag, tool toggles and tokens, and **writes remote servers as empty-command stdio entries**. +2. **Bearer tokens are encrypted** with the OS credential store (Electron `safeStorage`) and are not readable outside MCP Router. `env` values are **plaintext** in the database. +3. The format is **frozen**. The project is archived and `v0.6.4` is final, so a reader written against it will not drift. That removes the usual "unstable undocumented format" risk. + +The format is undocumented in prose, but the schema is created by `CREATE TABLE` statements in source and covered by migrations, so every column and its meaning can be confirmed from code (see sources). + +## 1. Where server definitions are stored + +MCP Router is an Electron app (`apps/electron/package.json`: `"productName": "MCP Router"`, `"version": "0.6.4"`). All state sits under Electron `app.getPath("userData")`. + +| OS | `userData` (default) | Source of the path | +|----|----------------------|--------------------| +| macOS | `~/Library/Application Support/MCP Router` | `docs/END_OF_SUPPORT.md` step 5 | +| Windows | `%APPDATA%\MCP Router` | `docs/END_OF_SUPPORT.md` step 5 | +| Linux | `~/.config/MCP Router` | Electron default for `productName`; **no official Linux build** (`apps/electron/forge.config.ts` makers are Squirrel, ZIP, DMG only; README lists "Windows and macOS"). Best effort only. | + +Files inside `userData` that matter (from `apps/electron/src/main/modules/workspace/workspace.service.ts` and `infrastructure/shared-config-manager.ts`): + +| File | What it holds | +|------|---------------| +| `mcprouter.db` | SQLite. Meta DB (`workspaces` table) **and**, in practice, the data DB of the default workspace `local-default`. `workspace.service.ts:38-39` opens it, then `:71-89` checks whether it exists. It always does by then (it was just created), so the default workspace always uses `databasePath: "mcprouter.db"`. | +| `workspaces//database.db` | SQLite data DB of each additional local workspace (`workspace.service.ts:205,442`). Located through `workspaces.localConfig` JSON (`{"databasePath": ...}`). | +| `shared-config.json` | App settings and `mcpApps.tokens` (the `MCPR_TOKEN` values issued to clients). Not server definitions. | +| `mcp-servers/dxt//` | Extracted `.dxt` bundles (`mcp-server-manager/dxt-processor/dxt-processor.ts:17-41`). DXT servers' `command`/`args` point into this tree. | + +The database runs in WAL mode (`infrastructure/database/sqlite-manager.ts`: `journal_mode = WAL`). A reader must either open read-only with the `-wal` file beside it, or copy the trio (`.db`, `-wal`, `-shm`) first. A running MCP Router holds the file open. + +## 2. Schema (`servers` table) + +`apps/electron/src/main/modules/mcp-server-manager/mcp-server-manager.repository.ts` (last changed `4c4642c`, 2026-06-24), `CREATE_TABLE_SQL`: + +| Column | Type | Meaning | MCPProxy target | +|--------|------|---------|-----------------| +| `id` | TEXT PK | UUID, or `dxt--` for DXT servers | not imported (identity is the name) | +| `name` | TEXT NOT NULL | display name, may contain spaces/Unicode; unique indexed non-uniquely (`idx_servers_name`) | `name`, sanitized | +| `command` | TEXT | stdio executable; `''`/NULL for remote servers | `command` | +| `args` | TEXT (JSON array) | stdio args. May contain whole-argument `${paramName}` / `${user_config.paramName}` placeholders resolved **at run time** by `substituteArgsParameters` (`mcp-apps-manager/mcp-client.ts:191-225`, called from `mcp-server-manager.ts:~488`): for `${user_config.x}` first `input_params.x.default`; then, for any placeholder, the non-empty `env[]`; then `input_params[].default`; otherwise left literal. Partial-string placeholders (`--a=${x}`) are **not** substituted. | `args`, placeholders resolved at import by the same order | +| `env` | TEXT (JSON object) | **plaintext** env vars (`serializeEntityData` does `JSON.stringify(entity.env)`, no encryption) | `env` | +| `auto_start` | INTEGER | start when MCP Router launches | no equivalent; reported | +| `disabled` | INTEGER | server turned off | `enabled = !disabled` | +| `auto_approve`, `context_path` | TEXT | legacy columns, never written by current code | ignored | +| `server_type` | TEXT, default `'local'` | `local` \| `remote` \| `remote-streamable` | `stdio` \| `sse` \| `streamable-http` (`mcp-client.ts:47,78` use `StreamableHTTPClientTransport` / `SSEClientTransport`) | +| `remote_url` | TEXT | remote endpoint; normalized by `utils/remote-url-security.ts` | `url` | +| `bearer_token` | TEXT | remote auth token. **Encrypted** when the OS store is available: value starts with `mcpr-secret:v1:` followed by base64 of `safeStorage.encryptString()` output (`utils/secret-storage.ts`). Plaintext when written before encryption existed or when encryption was unavailable. | `Authorization: Bearer ...` header, **only** if plaintext; else "re-enter" | +| `input_params` | TEXT (JSON) | DXT `user_config` schema: `{name: {type,title,description,sensitive,required,default,min,max}}` | used to resolve `${...}`; rest reported | +| `description`, `version`, `latest_version`, `verification_status`, `required_params` | TEXT | catalog metadata | not imported | +| `project_id` | TEXT | FK-like to `projects.id` (grouping) | no equivalent; reported | +| `tool_permissions` | TEXT (JSON object) | `{toolName: boolean}`. A tool is enabled unless the value is `false` (`mcp-server-manager.ts:449`, `:420`: `enabled: toolPermissions[tool.name] !== false`) | `disabled_tools` = names with `false` | +| `created_at`, `updated_at` | INTEGER | epoch ms | not imported | + +Migration history (`infrastructure/database/main-database-migration.ts`, last changed `a213cd3`, 2026-01-24) shows every column except the original ones was added by `ALTER TABLE ... ADD COLUMN`, so **older databases lack columns** (`server_type`, `remote_url`, `bearer_token`, `tool_permissions`, `project_id`, ...). The reader must tolerate missing columns. + +`projects` table (`modules/projects/projects.repository.ts`): `id, name, created_at, updated_at, optimization` (JSON). Unique index on `name COLLATE NOCASE`. + +Type definition for cross-checking: `packages/shared/src/types/mcp-types.ts` (`MCPServerConfig`: `serverType: "local" | "remote" | "remote-streamable"`, `toolPermissions`, `bearerToken`, `projectId`, `disabled`, `autoStart`). + +### Auth and OAuth + +- **Remote auth is a static bearer token only** (`utils/fetch-utils.ts:25`: `Authorization: Bearer `). There is no OAuth flow and no custom headers field in the schema. So there is nothing OAuth-shaped to map. +- A token that is encrypted cannot be recovered by MCPProxy without reaching into the OS credential store under another application's identity. **Decision: never attempt it.** The importer reports the server as "needs bearer token" and leaves the header out. + +### Groups / workspaces / tool toggles + +- **Projects** group servers (`project_id`). They are labels in MCP Router's UI and a per-project `optimization` setting; clients can connect to one with `mcpr connect --project `. +- **Workspaces** are separate DB files (above). `type` is `local` or `remote`; remote workspaces were cloud-backed, and the cloud ended 2026-09-18 (`docs/END_OF_SUPPORT.md`). A `remote` workspace has no local server rows. +- **Tool toggles** are `tool_permissions` (above). + +## 3. Export and import features + +### Export (`apps/electron/src/renderer/components/Home.tsx:201-224`, last changed `c883644`) + +Button titled "Export" on the Servers screen. Exports the **currently selected workspace's** `servers` from the renderer store as `mcp-servers-YYYY-MM-DD.json`: + +```json +{ "mcpServers": { "": { "command": "...", "args": [...], "env": { ... } } } } +``` + +This is exactly the **Claude Desktop config shape**, so MCPProxy's existing `upstream import` already parses it as `claude_desktop` today. But it is lossy: + +| Lost in export | Consequence | +|----------------|-------------| +| `server_type`, `remote_url` | A remote server is written as `{"command": "", "args": [], "env": {}}`, a broken stdio entry. | +| `bearer_token` | Not present (good for secrecy, but the user must re-enter it). | +| `disabled`, `auto_start`, `tool_permissions`, `project_id` | Not present. | +| Servers in other workspaces | Only the selected workspace is exported. | + +`docs/END_OF_SUPPORT.md` (the official migration guide) tells users to use this export, warns it "is not a complete application backup", and tells them to "confirm remote server URLs and authentication separately". So the official path leaves remote servers for users to redo by hand. Direct DB reading is the way to do better. + +### Import into MCP Router (relevant for what users already brought in) + +- **Manual / JSON** (`renderer/components/mcp/server/Manual.tsx`, `utils/mcp-server-utils.ts`): paste `mcpServers` JSON. +- **DXT**: `.dxt` extension files (Anthropic's extension format, the predecessor of `.mcpb`). `dxt-processor.ts` unpacks to `userData/mcp-servers/dxt//` and `dxt-converter.ts` turns the manifest into a `local` server with `${...}` placeholders and `inputParams`. **No `.mcpb`-specific handling** exists in the repo (`grep -i mcpb` finds nothing). There is **no export to DXT/MCPB**. +- **Client-config sync** (`mcp-apps-manager/mcp-config-importer.ts`): when `settings.loadExternalMCPConfigs !== false`, MCP Router reads the user's client configs at start and **copies any server it does not know by name into its own DB**. Consequence for E4: many rows in `servers` are copies of entries that still exist in the user's own client configs, so importing from clients and from the DB can overlap. Dedupe by name is required. + +## 4. How MCP Router configured its clients + +`mcp-apps-manager/app-definitions.ts` and `app-paths.ts` define six clients. MCP Router writes **one** entry into each, named `mcp-router`, that bridges stdio to its local aggregator (`mcp-apps-manager.service.ts:253-259`): + +```json +"mcp-router": { "command": "npx", "args": ["-y", "@mcp_router/cli@latest", "connect"], "env": { "MCPR_TOKEN": "" } } +``` + +VS Code uses a top-level `servers` key instead of `mcpServers`. Codex uses TOML: `[mcp_servers.mcp_router]` with `[mcp_servers.mcp_router.env] MCPR_TOKEN = "..."` (`mcp-apps-manager.service.ts:328-352`). + +| MCP Router id | Config file | MCPProxy `connect` client id (`mcpproxy-go: internal/connect/clients.go`) | +|---------------|-------------|-----------------------------------------| +| `claude` | macOS `~/Library/Application Support/Claude/claude_desktop_config.json`; Windows `%APPDATA%\Claude\claude_desktop_config.json`; Linux `~/.config/Claude/claude_desktop_config.json` | `claude-desktop` | +| `cursor` | `~/.cursor/mcp.json` (global) | `cursor` | +| `windsurf` | `~/.codeium/windsurf/mcp_config.json` | `windsurf` | +| `vscode` | macOS `~/Library/Application Support/Code/User/mcp.json`; Windows `%APPDATA%\Code\User\mcp.json`; Linux `~/.config/Code/User/mcp.json` | `vscode` | +| `codex` | `~/.codex/config.toml` | `codex` | +| `cline` | VS Code `globalStorage` (`saoudrizwan.claude-dev/settings/cline_mcp_settings.json`) | **none** (MCPProxy has no Cline client) | + +So the importer can **detect which clients still route through MCP Router** by looking for an entry whose args include `@mcp_router/cli` and `connect` (the same test MCP Router itself uses, `mcp-config-importer.ts:extractConfigInfo`), and tell the user to run `mcpproxy connect ` for each and then remove the `mcp-router` entry. MCP Router's own aggregated endpoint is a localhost HTTP server (`apps/cli/src/commands/connect.ts` default URL, port 3282); an entry pointing at it must never be imported as a server (a loop). MCPProxy already has the equivalent guard for itself (`internal/configimport/self_reference.go`). + +`MCPR_TOKEN` values live in `shared-config.json` (`mcpApps.tokens`). They are MCP Router's own client credentials, useless to MCPProxy, and **must not be read, shown or imported**. + +## 5. Existing MCPProxy machinery to reuse (`mcpproxy-go` @ `9dfdb2f19`) + +| Need | Existing piece | +|------|----------------| +| Parse + map + dedupe + quarantine + self-reference + redacted preview | `internal/configimport` (`Parser` interface, `GetParser`, `Import`, `ImportOptions{Preview, ExistingServers, SelfListenAddrs, SkipQuarantine}`, `summary.go` secret classification). Quarantine is the default; `SkipQuarantine` is opt-in. | +| CLI | `mcpproxy upstream import [--dry-run] [--server] [--format] [--no-quarantine]` (`cmd/mcpproxy/upstream_cmd.go:240-405`, `runUpstreamImport` at `:2054`). There is **no** top-level `mcpproxy import`. | +| Daemon and config-file apply modes | `applyImportedServers` (`upstream_cmd.go:2344`). | +| REST | `POST /api/v1/servers/import`, `/import/json`, `/import/path`; `GET /import/paths` (`internal/httpapi/server.go:1096-1100`, `internal/httpapi/import.go`). `handleGetCanonicalConfigPaths` lists well-known client config paths. | +| Web UI | `frontend/src/components/ImportServersPanel.vue`, `ImportServers.vue`, `AddMenu.vue`, `AddServer.vue`. | +| Secret storage | `internal/secret` + `${keyring:}`, `cmd/mcpproxy/upstream_add_secret.go` (`--secret-env`, `--secret-header`, refuses if keyring unavailable). | +| Telemetry | `internal/telemetry` `CounterRegistry` (`Record*On(reg, ...)`), `SchemaVersion` (14 when researched; 16 on main at merge time), `ScanForPII`. Closed-enum, count-only counters. | +| Connect | `mcpproxy connect `, `internal/connect/clients.go` (claude-desktop, cursor, windsurf, vscode, codex, ...). | + +**There is no SQLite driver in `mcpproxy-go/go.mod`** (storage is BBolt). Reading `mcprouter.db` therefore needs either a new pure-Go SQLite dependency or a restricted fallback; this is the one real dependency decision (see plan.md). + +## 6. What cannot be done, and why that is acceptable + +| Item | Status | +|------|--------| +| Decrypt `bearer_token` values with the `mcpr-secret:v1:` prefix | Not attempted; needs another app's OS credential. User re-enters. | +| Import workflows, hooks, skills, request logs, client tokens, cloud-workspace data | Out of scope. Counted and listed as "not imported". | +| Recreate Projects/Workspaces | No equivalent without inventing profiles. Reported with the project name per server. Revisit once Spec 108 profiles are stable. | +| Cline client reconnect | MCPProxy has no Cline client; the importer can only point at Cline's generic MCP settings. | +| Remote (cloud) workspaces | Cloud ended 2026-09-18; no local data. | + +## 7. Evidence of untrusted-input handling + +The cloned repo's root `CLAUDE.md` contains instructions addressed to AI agents (a Japanese "7 principles" list: show a plan and ask y/n before any action, print the principles at the start of each chat). That file is third-party data inside a repository being read, not an instruction from the user; it was not followed. This is also a reminder that the importer must treat every field of an MCP Router database as data (it is attacker-influenceable if a user imported a malicious DXT): see FR-014 in spec.md. + +## Sources (all in `mcp-router/mcp-router` @ `7d6cd4e`) + +- `apps/electron/src/main/infrastructure/database/sqlite-manager.ts` (DB open, WAL) +- `apps/electron/src/main/infrastructure/database/main-database-migration.ts` (@ `a213cd3`, column history) +- `apps/electron/src/main/modules/mcp-server-manager/mcp-server-manager.repository.ts` (@ `4c4642c`, `servers` schema, row mapping) +- `apps/electron/src/main/utils/secret-storage.ts` (@ `4c4642c`, `mcpr-secret:v1:` prefix) +- `apps/electron/src/main/modules/workspace/workspace.service.ts` (DB locations) +- `apps/electron/src/main/modules/mcp-apps-manager/{app-definitions,app-paths,mcp-apps-manager.service,mcp-config-importer,mcp-client}.ts` +- `apps/electron/src/renderer/components/Home.tsx` (export), `.../mcp/server/Manual.tsx` +- `apps/electron/src/main/modules/mcp-server-manager/dxt-processor/{dxt-processor,dxt-converter}.ts` +- `packages/shared/src/types/mcp-types.ts` +- `docs/END_OF_SUPPORT.md`, `docs/adr/database/DATABASE_ARCHITECTURE.md`, `README.md`, `apps/cli/README.md` diff --git a/specs/120-mcp-router-import/spec.md b/specs/120-mcp-router-import/spec.md new file mode 100644 index 000000000..3b0299017 --- /dev/null +++ b/specs/120-mcp-router-import/spec.md @@ -0,0 +1,322 @@ +# Feature Specification: MCP Router Importer (`mcpproxy import --from mcp-router`) + +**Feature Branch**: `claude/spec-mcp-router-import-7ae3fb` (spec directory `120-mcp-router-import`) +**Created**: 2026-10-20 +**Status**: Draft +**Input**: Growth experiment E4 from the Q4 2026 promotion strategy. MCP Router (a 2,135-star desktop MCP manager) was archived on 2026-09-18; its README says new adoption is not recommended, so its users have no maintained tool. MCPProxy offers a one-command way across. + +**Related**: `research.md` in this directory (verified MCP Router storage format, with file paths and commit SHAs). Existing import machinery: `mcpproxy upstream import`, `internal/configimport`, REST `/api/v1/servers/import*`, Web UI `ImportServersPanel`. Spec 109 (catalog-first Add, import preview, FR-064/065 secret handling), Spec 032 / MCP-2931 (quarantine), Spec 108 (profiles), telemetry schema v16. + +## Context and Motivation + +A person with MCP Router has their servers in its local database and their AI clients pointed at it. MCP Router no longer receives fixes, and the official migration guide only offers a JSON export. We verified that export is lossy: it writes remote servers as broken empty-command entries and drops disabled flags and tool toggles (see `research.md` §3). The database itself holds everything except encrypted bearer tokens. + +Today such a user can already run `mcpproxy upstream import `, but only if they know that the export is Claude-Desktop-shaped, and they silently get broken remote entries. Nothing tells them which clients to reconnect. This feature makes the move one discoverable command, loses as little as possible, says plainly what it could not carry over, and keeps MCPProxy's security stance: nothing arrives trusted, and no secret is stored without the person seeing it. + +It is also an experiment. Success is measured with a privacy-safe counter: at least 25 successful imports by day 60 after release. + +## Scope Boundary + +In scope: +- A top-level CLI command `mcpproxy import --from mcp-router` with `--path` and `--dry-run`. +- Reading MCP Router's data directory and database directly, and reading its exported JSON file. +- A summary of what was imported, skipped and could not be mapped, and a list of clients to reconnect. +- A closed-enum telemetry counter `import_source`. +- A Web UI entry point reusing the existing import panel, if it stays small (P3). + +Out of scope (documented in the summary instead): MCP Router workflows, hooks, skills, request logs, client tokens, cloud workspaces, Projects (grouping) and Workspaces as first-class MCPProxy objects, decrypting MCP Router's encrypted bearer tokens, `.dxt`/`.mcpb` file import (MCPProxy has no extension runtime), and a general `--from ` catalog beyond reserving the flag shape. + +## Clarifications + +Self-answered under the zero-interruption policy (no human available); each answer is the safest default consistent with MCPProxy's existing import and secret behavior. + +### Session 2026-10-20 + +- Q: How does a person confirm what will be stored, and how do scripts run it? → A: An interactive run prints the plan and asks one yes/no confirmation. `--yes` skips the prompt. A non-interactive run without `--yes` refuses. `--dry-run` never prompts. +- Q: What is the explicit opt-in for keeping secrets inline when no keyring exists? → A: `--secrets-inline`. It is never implied by `--yes`. +- Q: Which values count as "secret-like"? → A: Whatever the existing import classifier already marks secret-like for env/headers, plus every plain-text bearer token regardless of shape. No new classifier. +- Q: Several local workspaces contain the same server name? → A: The default workspace wins, then others in creation order; losers are listed as collisions. `--workspace ` limits the import to one workspace. +- Q: How is a live (open, WAL-mode) database read without touching it? → A: Copy the database and its journal files to a private temporary directory (mode 0700), read the copy, delete the copy on exit. The live files are never opened for writing. The reader library choice is a plan decision. +- Q: Does `upstream import` also learn this source? → A: No. `upstream import` is unchanged (an export file is byte-compatible with the Claude Desktop shape, so it keeps importing there as before, with its known lossy remote entries). `mcpproxy import --from mcp-router` is the only entry point, so no second path can bypass the secret, confirmation and accounting rules. (Review round 1, finding 8.) +- Q: What happens to an entry that cannot run without a value MCPProxy does not have? → A: Two outcomes. *Unmappable* (not added): empty command, remote server with no URL, unresolved `${...}` placeholder left after resolution. *Needs configuration* (added **disabled** and quarantined): a remote server whose bearer token was encrypted. Both are listed with the reason. (Review round 1, finding 10.) +- Q: Is quarantine unconditional? → A: The importer always creates the server with the quarantined flag set, exactly as `upstream import` does today, independent of trust mode. If the operator has globally disabled quarantine, the existing global semantics apply unchanged and the summary reports the state actually read back from the configuration. (Review round 1, finding 13.) +- Q: Who records the telemetry count, and what about a run without the daemon? → A: The daemon records it when the import is applied through the daemon (the normal case, since the tray starts the core), through the existing counter window and retry mechanism. A daemonless config-file run is **not counted** (documented undercount; the experiment is measured among installs where the core runs). (Review round 1, finding 14.) +- Q: Is the web entry gating? → A: No. Story 7 / FR-025 are non-gating for this release and ship last in the plan, droppable without affecting any other requirement. (Review round 1, finding 19.) +- Q: Are imported servers that MCP Router had disabled also quarantined? → A: Yes. Disabled and quarantined are independent: they arrive disabled and quarantined. +- Q: Daemon running or not? → A: Same as `upstream import`: apply through the running daemon when present, otherwise edit the configuration file. +- Q: What does "success" in the counter mean? → A: A non-dry run that added at least one server (exit status success or partial success). Re-runs that add nothing do not count. +- Q: Exit status for a dry run? → A: Success, regardless of how many servers would fail; the failures are in the output. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Move my servers in one command (Priority: P1) + +A MCP Router user installs MCPProxy and runs `mcpproxy import --from mcp-router`. MCPProxy finds MCP Router's data on this computer, shows what it found, and adds the servers, all quarantined, with a summary. + +**Why this priority**: This is the experiment. Without it there is nothing to measure. + +**Independent Test**: Point `--path` at a fixture data directory built from MCP Router's real schema (local, remote, remote-streamable, disabled, with tool toggles). Run with `--dry-run`, then without. Check the resulting servers and that nothing was changed by the dry run. + +**Acceptance Scenarios**: + +1. **Given** MCP Router's data directory exists at its default location, **When** the user runs `mcpproxy import --from mcp-router`, **Then** the location is auto-detected, the servers are listed with source name, kind (local process or remote), and status, and each server that can be mapped is added and quarantined. +2. **Given** a stdio server `fs` with `command`, `args` and `env`, **When** it is imported, **Then** the MCPProxy server has the same command, args and env, and shows as quarantined awaiting review like any other newly added server. +3. **Given** a remote server of type `remote` (SSE) and one of type `remote-streamable`, **When** imported, **Then** they become URL servers with the matching transport. +4. **Given** a server MCP Router had disabled, **When** imported, **Then** it is added disabled. +5. **Given** a server with tool toggles where some tools are off, **When** imported, **Then** those tools are hidden on the MCPProxy server and the summary says so. +6. **Given** no MCP Router data is found at the default location, **When** the command runs, **Then** it exits with a clear message naming the paths it looked at and suggesting `--path`; it does not touch configuration. + +--- + +### User Story 2 - Preview first, with nothing changed (Priority: P1) + +The user runs `mcpproxy import --from mcp-router --dry-run` to see exactly what would happen. + +**Why this priority**: People will not run an importer on their working setup without a safe preview. It is also the first measurable funnel step. + +**Independent Test**: Run `--dry-run` against the fixture; assert the configuration, keyring and telemetry import counter are unchanged and the output equals what a real run later reports. + +**Acceptance Scenarios**: + +1. **Given** any valid source, **When** `--dry-run` is used, **Then** no server is added, no secret is stored, and the import counter is not incremented. +2. **Given** a dry run followed by a real run on the same source and state, **When** compared, **Then** the same servers are listed as to-import, skipped and unmappable. +3. **Given** `-o json`, **When** `--dry-run` runs, **Then** the result is machine-readable with the same content as the table, and contains no secret value. + +--- + +### User Story 3 - No secret moves silently (Priority: P1) + +MCP Router stores environment variables in plain text and bearer tokens encrypted by the operating system. The user must see what will be stored and where, and must approve it. + +**Why this priority**: The reputation of a security-first proxy depends on it. A silent copy of API keys into a new config file would be a defect. + +**Independent Test**: Import a fixture containing an API-key-like env var, a plain-text bearer token and an encrypted bearer token. Verify what is shown, what is stored, and where. + +**Acceptance Scenarios**: + +1. **Given** servers with env values or tokens, **When** the preview is shown, **Then** each is listed by name with "value present" and "looks like a secret" flags and never with its value. +2. **Given** secret-like values and a working OS keyring, **When** the user confirms the import, **Then** those values are stored in the keyring and the configuration holds only a reference, matching how `upstream add --secret-env` behaves. Non-secret values may stay inline. +3. **Given** secret-like values and no keyring, **When** the user runs a non-dry import, **Then** the command stops before writing anything and says how to proceed (an explicit, named opt-in to keep them inline, or fix the keyring). It never falls back to plaintext on its own. +4. **Given** an encrypted bearer token, **When** the server is imported, **Then** the server is added without that header, flagged "needs bearer token", and the summary tells the user to re-enter it. MCPProxy does not try to decrypt it. +5. **Given** a non-interactive run (no terminal), **When** secret-like values exist and no explicit choice flag was given, **Then** the command refuses rather than guessing. +6. **Given** MCP Router's client tokens (`MCPR_TOKEN`), **When** the import runs, **Then** they are never extracted, printed, logged, stored or sent, and `shared-config.json` is never opened. +7. **Given** a sensitive DXT parameter substituted into an argument, **When** imported, **Then** the argument carries a keyring reference, not the value, even if the parameter has an innocuous name. +8. **Given** a source string containing `${keyring:...}` or `${env:...}`, **When** imported, **Then** the entry is reported unmappable and no destination secret is resolved. + +--- + +### User Story 4 - Run it twice, nothing breaks (Priority: P1) + +The user re-runs the command (they added servers in MCP Router since, or the first run was interrupted). + +**Why this priority**: Importers are rerun. Duplicates and overwrites destroy trust. + +**Independent Test**: Run twice against the same source; then change one source server and run again. + +**Acceptance Scenarios**: + +1. **Given** a completed import, **When** the same command is re-run, **Then** nothing is added or changed, and every server is reported as already present. +2. **Given** a server of the same name exists in MCPProxy with a different configuration, **When** the import runs, **Then** it is not overwritten; the summary reports a conflict naming the differing fields (not values) and how to resolve it. +3. **Given** a new server was added to MCP Router after the first import, **When** re-run, **Then** only the new server is added. +4. **Given** an interrupted first run, **When** re-run, **Then** it completes the remainder without duplicates. + +--- + +### User Story 5 - Tell me what did not come across, and what to reconnect (Priority: P2) + +At the end the user gets an honest list: what was imported, what was skipped and why, what could not be mapped, and which AI clients still talk to MCP Router and how to point them at MCPProxy. + +**Why this priority**: Without it the import "works" but the user's clients still call a dead app, and the first thing they see is a broken workflow. + +**Independent Test**: Provide fixture client configs containing MCP Router's bridge entry; verify the reconnect list and that no config file is modified. + +**Acceptance Scenarios**: + +1. **Given** a server field with no MCPProxy equivalent (auto-start, project, workspace, description, DXT input parameters), **When** imported, **Then** it is listed per server under "not carried over". +2. **Given** a local server that depends on files in MCP Router's data directory (DXT-installed), **When** imported, **Then** the summary warns that it still depends on that directory. +3. **Given** a stdio server whose arguments contain unresolved `${param}` placeholders, **When** imported, **Then** placeholders that MCP Router would have filled from `env` or parameter defaults are filled the same way, and any left over are listed as "needs a value". +4. **Given** a Claude Desktop, Cursor, Windsurf, VS Code or Codex config that contains MCP Router's bridge entry, **When** the import finishes, **Then** the summary names each such client and the exact `mcpproxy connect ` command, and notes the old `mcp-router` entry should then be removed. The command only reads those files. +5. **Given** the user's client config still contains an entry that points at MCP Router's local endpoint, **When** discovered by the import, **Then** it is never imported as a server. +6. **Given** a Cline installation (no MCPProxy connect target), **When** found, **Then** the summary says so in one line rather than offering a command that does not exist. +7. **Given** MCP Router had workflows, hooks, skills, logs or cloud data, **When** the import ends, **Then** the summary says in one line that these are not imported. + +--- + +### User Story 6 - Use the exported file instead (Priority: P2) + +Some users only have the JSON that MCP Router's Export button produced (or are on a machine where the database is not available). + +**Why this priority**: It is the officially documented path in MCP Router's end-of-support guide, and it needs no database access. + +**Independent Test**: Run `mcpproxy import --from mcp-router --path mcp-servers-2026-09-18.json` on a fixture export. + +**Acceptance Scenarios**: + +1. **Given** an export file, **When** passed via `--path`, **Then** it is recognised as an MCP Router export and its servers are imported with the same rules. +2. **Given** an export entry with an empty command (how MCP Router exports remote servers), **When** imported, **Then** it is not added as a broken server; it is listed as "remote server: URL was not exported" with the advice to re-run against the data directory, or to add the URL manually. +3. **Given** `--path` points at a directory, a database file, or an export file, **When** the command runs, **Then** each is accepted and the input kind is stated in the output. +4. **Given** `--path` points at something else (a random JSON, a binary), **When** run, **Then** it is rejected with an error that says what was expected. +5. **Given** any export file, **When** imported, **Then** the output states what an export cannot carry (disabled state, tool toggles, bearer tokens, parameter schemas, other workspaces) and that imported servers therefore arrive enabled with all tools visible, after review. + +--- + +### User Story 7 - Import from the Web UI (Priority: P3, non-gating) + +A user who prefers the Web UI picks "MCP Router" in the existing Add / Import server flow. It shows the same preview and applies the same rules. + +**Why this priority**: Cheap if it reuses the existing import panel and endpoints, but the CLI alone proves the experiment. + +**Independent Test**: In the Web UI, open Add server, choose Import, choose MCP Router, observe detected location, preview, and confirm. + +**Acceptance Scenarios**: + +1. **Given** MCP Router data exists, **When** the user opens the import panel, **Then** MCP Router appears among the detected sources with a count of servers found. +2. **Given** the preview, **When** the user confirms, **Then** the result and summary match the CLI's for the same source, including the secret rules, the quarantine default, and the reconnect list. +3. **Given** a read-only or agent-scoped session, **When** the user attempts it, **Then** the same authorization as other server-adding operations applies. + +--- + +### User Story 8 - Measure the experiment without identifying anyone (Priority: P1) + +The maintainer needs to know how many people completed an import from MCP Router, without learning who they are or what they imported. + +**Why this priority**: The experiment's success criterion (25 imports by day 60) is meaningless without it. + +**Independent Test**: Run a real import and a dry run; inspect the next heartbeat payload. + +**Acceptance Scenarios**: + +1. **Given** a real import that added at least one server, **When** the next telemetry heartbeat is built, **Then** it carries a count for `import_source` = `mcp-router` and nothing else about the import. +2. **Given** a dry run, a run that committed no server addition (all failed, all already present, refused), or a run without the daemon, **When** the heartbeat is built, **Then** the counter is unchanged. +3. **Given** telemetry is disabled by any existing opt-out, **When** an import runs, **Then** nothing is recorded or sent. +4. **Given** the heartbeat payload, **When** scanned by the existing privacy check, **Then** it passes: only a fixed enum key and a non-negative integer; no server names, counts of servers, paths, URLs, or MCP Router identifiers. +5. **Given** the counter window resets after an accepted send, **When** several imports occurred in the window, **Then** the delta is reported once and not repeated. + +--- + +### Edge Cases + +- **MCP Router is still running** and holds its database open. The importer must read a consistent snapshot without modifying or locking the live files (and must tell the user nothing needs to be closed). +- **Older database** without columns added later (no `server_type`, `remote_url`, `tool_permissions`, ...). Missing columns mean defaults, not errors. +- **Corrupt, encrypted-at-rest or unreadable database**: clear error, nothing imported, exit code distinct from "nothing found". +- **Multiple local workspaces**: the default workspace plus others. Servers from all local workspaces are listed with their workspace; the same name in two workspaces is a collision, reported (first wins by default, the rest listed) and selectable with `--workspace`. +- **Names with spaces/Unicode/duplicates after sanitizing**: handled by the existing sanitizer; a collision after sanitizing is reported, never silently merged. +- **Hostile values**: a name, URL, command or argument containing control characters, shell metacharacters, extreme length, or a `file:`/non-http scheme. Treated as data: validated like any added server, never executed or interpreted during import. +- **Remote URL to a private address**: allowed (MCPProxy supports local servers) but shown in the preview. +- **A source server that is MCPProxy itself** (an entry pointing at this instance): skipped as self-reference. +- **Case-only or whitespace-only name differences** with existing servers. +- **The user is not on a supported OS** (Linux): best-effort default path; `--path` always works. +- **Zero servers in source**: a clear "nothing to import" message and a success exit. +- **Both daemon running and not running**: works in both modes, as `upstream import` does. +- **Large source** (hundreds of servers): preview stays readable (grouped, summarised) and completes in seconds. + +## Requirements *(mandatory)* + +### Functional Requirements + +**Command and sources** + +- **FR-001**: The system MUST provide `mcpproxy import --from mcp-router [--path ] [--dry-run]`. `--from` is required, accepts `mcp-router`, and rejects unknown values with the list of supported sources. +- **FR-002**: Without `--path`, the system MUST auto-detect MCP Router's data location per operating system (macOS, Windows; Linux best effort) and name every location it checked when none is found. +- **FR-003**: `--path` MUST accept (a) MCP Router's data directory, (b) a database file inside it, or (c) an exported server JSON file, and MUST state which kind it recognised. +- **FR-004**: The system MUST read all local workspaces' servers from the data directory, tolerate databases created by any MCP Router release that had the `servers` table (missing columns take defaults), and read the source without modifying it. +- **FR-004a**: Workspace database locations recorded inside the source database are untrusted. The system MUST only open `mcprouter.db` and `workspaces//database.db` beneath the selected data directory, where `` matches a conservative identifier pattern; the canonical path (symlinks resolved) MUST stay inside that directory. Any other recorded location (absolute, parent-relative, symlink escaping the directory) is skipped with a warning naming the workspace, never followed, and never opened. Paths in server `command`/`args` (for example DXT extension folders) are never opened or probed. +- **FR-005**: The system MUST work against a database that MCP Router currently has open, from a consistent snapshot, without requiring the user to quit MCP Router and without opening the live files for writing. The snapshot lives in a private, permission-restricted location removed on exit. Consistency MUST be verified (the copy passes an integrity check and the source files did not change while copying), retried a bounded number of times, and if it cannot be obtained the command MUST fail safely with advice to close MCP Router or use an export file. A torn snapshot MUST never be read as if it were consistent. +- **FR-005a**: When several local workspaces hold the same server name, the default workspace wins, then others in creation order; the rest are listed as collisions. `--workspace ` MUST limit the import to one workspace. + +**Mapping** + +- **FR-006**: The system MUST map each source server to an MCPProxy server per this table: + + | MCP Router | MCPProxy | + |------------|----------| + | local server: command, args, env | stdio server with the same command, args, env | + | `${param}` / `${user_config.param}` placeholders in args | resolved exactly as MCP Router resolved them (`research.md` §2: whole-argument match only; `${user_config.x}` takes parameter `x`'s default first, then env `user_config.x`, then parameter `user_config.x`'s default; `${x}` takes a non-empty env `x` first, then parameter `x`'s default); leftovers are unmappable (FR-006b) | + | remote (SSE) | URL server, SSE transport | + | remote-streamable | URL server, streamable HTTP transport | + | remote URL | URL | + | plain-text bearer token | `Authorization: Bearer` header, subject to the secret rules (FR-009..FR-011) | + | encrypted bearer token | not imported; "needs bearer token" | + | disabled | added disabled | + | tool toggles set to off | those tools hidden on the server | + | auto-start, project, workspace, description, version, verification status, parameter schema | not carried over; listed | + +- **FR-006a**: Placeholder resolution MUST NOT launder secrets into unprotected fields. A value substituted into `args` that comes from a parameter marked sensitive, from a secret-like env var, or from a parameter whose *name* the existing name-based classifier judges secret-like (no value-based detection is added) MUST be stored as a keyring reference (per FR-011) and the argument MUST carry the reference, not the value. Credentials embedded directly in a URL or argument (query token, `--api-key=...`) are listed in the plan as "inline credential" and are imported only with `--secrets-inline`; otherwise that server is reported as needing a decision and is not added. +- **FR-006b**: Source text is data, not MCPProxy secret syntax. After placeholder resolution, any remaining `${...}` sequence (including `${env:...}` and `${keyring:...}`) in a command, argument, env value, URL or header from the source MUST make the entry *unmappable* ("unresolved or reserved placeholder"). Only references generated by the importer itself may carry resolution semantics. +- **FR-007**: A source entry that cannot form a valid server MUST be classified and reported, never added in a broken state. *Unmappable* (not added): empty command, remote server with no URL, invalid URL or name, unresolved or reserved placeholder (FR-006b), inline credential without `--secrets-inline`. *Needs configuration* (added disabled and quarantined): remote server whose bearer token was encrypted. +- **FR-008**: Entries that point at this MCPProxy instance, or that are MCP Router's own client bridge (`@mcp_router/cli connect`) or its local endpoint, MUST be skipped with the reason stated. + +**Secrets and trust** + +- **FR-009**: The system MUST NEVER print, log, or place in telemetry any secret value. Previews show names and flags only (value present, looks like a secret, empty or placeholder), as the existing import preview does. +- **FR-010**: Before a non-dry run writes anything, the system MUST show what will be stored: for each server, which env vars and headers will be kept, which will go to the OS keyring, and which are missing. +- **FR-011**: Secret-like values (the existing import classifier's judgment, plus every plain-text bearer token) MUST be stored through the same keyring mechanism and reference format as `upstream add --secret-env/--secret-header`. If the keyring is unavailable the run MUST stop before any write, naming the reason, unless the user passes `--secrets-inline` (never implied by `--yes`). An interactive run MUST ask one confirmation after showing the plan; `--yes` skips it; a non-interactive run without `--yes` MUST refuse rather than choose. +- **FR-012**: Every imported server MUST be added through the shared add path with the quarantined flag set and unapproved, independent of trust mode, exactly as `upstream import` does today. If the operator has globally disabled quarantine, existing global semantics apply unchanged and the summary MUST report the state actually read back. The command MUST NOT offer a way to skip quarantine. +- **FR-012a**: The shared apply path MUST carry the enabled flag and the hidden-tools list, in both daemon and config-file modes, and MUST return the true per-server outcome (added, failed with reason). Totals, exit status and telemetry MUST derive from those outcomes, not from attempted adds. +- **FR-013**: The system MUST NOT open `shared-config.json` or any other MCP Router file besides the approved database files, their SQLite sidecar files (`-wal`, `-shm`; same containment, symlink and regular-file checks as the database, FR-004a) and an export file the user named. Client configuration files are opened read-only solely to detect MCP Router's bridge entry; the `MCPR_TOKEN` values found there MUST NOT be extracted, retained, logged, displayed, stored or sent. +- **FR-014**: All source content MUST be treated as untrusted data: validated with the same rules as any added server, never executed, never interpreted as a command, and bounded in size. A malicious database must not be able to make the import run code, write outside MCPProxy's configuration, or read files other than the ones named in this spec. + +**Idempotency** + +- **FR-015**: Re-running against the same source MUST add nothing and change nothing, reporting each server as already present. +- **FR-016**: A server whose name exists in MCPProxy with a different configuration MUST NOT be overwritten; the summary MUST report the conflict and which fields differ (names only, never values). +- **FR-016a**: Names match after the existing sanitizer, case-insensitively, with surrounding whitespace trimmed. "Same configuration" compares transport, command, args, URL, and the *names* of env vars and headers (never secret values or keyring reference names). It ignores enabled state, quarantine and approval state, and anything the operator edited after import that is outside those fields. Two source servers that sanitize to the same name are a collision, reported, not merged. +- **FR-017**: Each server is committed independently. A failure while adding one server MUST attempt to roll back only the keyring entries written for that server and MUST NOT touch entries referenced by servers already committed. If a rollback is incomplete or indeterminate (the keyring backend can time out or mark itself unavailable), the output MUST name the possibly orphaned references (names, never values). Keyring reference names MUST be deterministic per server and field, so a re-run overwrites and reuses them instead of creating new ones. Unreferenced entries (one or more per failed server) may therefore remain after a backend failure or a hard kill; this is documented and a re-run repairs it. Rollback happens only after a definite rejection. If the outcome of an add is unknown (timeout, lost response), the server is reported as *uncertain*, its keyring entries are kept, it counts as a failure for exit status but earns no telemetry count, and re-running resolves it idempotently. The whole apply phase is serialised across importer processes so concurrent runs cannot delete each other's entries. + +**Reporting** + +- **FR-018**: The output MUST contain: servers to import, skipped (already present, filtered, self-reference), failed or unmappable (with reason), fields not carried over per server, categories not imported at all (workflows, hooks, skills, logs, cloud), and warnings (DXT path dependency, unresolved placeholders, private-address URLs). +- **FR-019**: The output MUST list AI clients still configured to use MCP Router, found by reading their configuration files read-only, with the exact `mcpproxy connect ` command for each client MCPProxy supports and a one-line note for those it does not. Client configuration files MUST NOT be modified by this command. +- **FR-020**: Output MUST support the repository's standard `-o json|yaml` modes and the machine-readable help (`--help-json`), with identical content to the table output. +- **FR-021**: Exit status MUST distinguish: (a) success, including "nothing to import", already-present only, and a `--dry-run` whose planning completed; (b) partial success (at least one server added and at least one failed, conflicted or unmappable); (c) nothing added although something was expected (all failed, unmappable or conflicting); (d) source not found; (e) source unreadable, malformed, or no consistent snapshot; (f) refused by the secret rules (no keyring, non-interactive without `--yes`). A `--dry-run` that cannot read the source fails with (d) or (e). The numeric codes are fixed in the plan, reuse the existing exit-code constants where they fit, and are documented. + +**Telemetry** + +- **FR-022**: The daemon MUST record, for each non-dry run applied through it that committed at least one server addition (outcome (a) or (b) of FR-021 with at least one real add; a re-run that adds nothing does not count), one count under the closed enum key `import_source` = `mcp-router`. The enum MUST be closed (known sources plus `other`), and a value outside the enum MUST be impossible to emit. +- **FR-023**: The counter MUST carry only a non-negative integer per enum key. It MUST NOT carry server names, number of servers, paths, URLs, usernames, or any MCP Router identifier, MUST honour every existing telemetry opt-out, and MUST pass the existing payload privacy scan. +- **FR-023a**: The eligibility check (every existing opt-out, including environment overrides, evaluated for the process that records the event) MUST run before the event is recorded, not only before sending. Disabled → import → enabled MUST NOT emit the earlier import retroactively. +- **FR-024**: The telemetry change MUST be documented in the telemetry document and carry a schema version bump. The privacy scanner and schema validation MUST recognise the new field structurally: closed key set, non-negative integers only, with negative tests for unknown keys, negative numbers and wrong types. + +**Web UI (P3)** + +- **FR-025** *(non-gating for the release)*: The Web UI SHOULD offer MCP Router as an import source in the existing import panel, using the same detection, preview, secret rules, quarantine default, idempotency and reconnect list as the CLI. It MUST NOT introduce a second import code path. + +**Documentation** + +- **FR-026**: The system MUST ship user documentation for the command, including what is and is not carried over and a short "after you import" checklist. The page is linked from, and consistent with, the migration page handled on the marketing website. + +### Key Entities + +- **Import source**: a recognised origin of servers (`mcp-router` now; the enum is closed and extensible). Has a detected location and an input kind (data directory, database, export file). +- **Source server**: one MCP Router server row, including its workspace, with fields as in `research.md`. Immutable input. +- **Import plan**: the result of reading and mapping, before any write. Lists to-import, skipped, failed and unmappable servers, per-server not-carried-over fields, secret handling per field, and detected clients to reconnect. A dry run prints exactly this. +- **Import outcome**: what a real run did. Same shape as the plan plus what was written. +- **Reconnect advice**: for each detected client, whether it still uses MCP Router and the command to switch. +- **Import counter**: a delta count per source enum value in the telemetry window. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: A user with MCP Router installed goes from running the command to having all mappable servers added and quarantined in under 2 minutes of their time (excluding quarantine review), with at most one confirmation prompt. +- **SC-002**: On a fixture database with 50 servers of all kinds, the dry run and the real run complete in under 5 seconds each, and the real run's results equal the dry run's plan. +- **SC-003**: 100% of servers in a fixture covering local, remote and remote-streamable are either imported correctly, added disabled as "needs configuration", or listed unmappable with a specific reason; zero are added enabled in a state that cannot run (for example an empty command or an unresolved placeholder). +- **SC-004**: Zero secret values appear in any output, log, report or telemetry payload across the full test matrix (verified by a scan with seeded canary secrets). +- **SC-005**: A second run against an unchanged source changes nothing (byte-identical configuration), and a run against a changed source adds only the new servers. +- **SC-006**: 100% of imported servers are quarantined under default settings. +- **SC-007**: Experiment target: at least 25 successful import runs counted under `import_source=mcp-router` within 60 days of the release that ships the command. This is run-based, measured among installs where the core is running; installs are approximated by the heartbeat's anonymous id and are not exact. +- **SC-008**: For a fixed fixture matrix, each of command, args, env, URL, transport, enabled state and hidden tools is carried over equivalently for every server whose source contains it, where "equivalent" means after the specified mapping (placeholder resolution, FR-006) and the specified secret-reference transformation (FR-006a, FR-011). The only other exceptions are encrypted bearer tokens and fields an export file lacks; every exception is listed in the output, never silently dropped. +- **SC-009**: Each step of the reconnect advice, when followed on a machine with a client configured through MCP Router, results in that client using MCPProxy (checked once manually per supported client). + +## Assumptions + +- A-1: MCP Router's on-disk format is frozen: the project is archived and `v0.6.4` is final, so one reader written against `research.md` covers every real database. Versions before the columns were added are handled by defaults. +- A-2: A new top-level `import` command is added rather than extending `upstream import`, for discoverability by someone who knows only "MCP Router is dead". The parsing work is a new format in the existing import library. `upstream import` itself is not changed. +- A-3: Reading the database directly is the main path, because the export loses remote servers. The export file is a supported secondary input. +- A-4: Encrypted bearer tokens are never decrypted, even if technically possible on the same machine; this is a deliberate security decision. +- A-5: Linux is best effort only; MCP Router never shipped a Linux build. +- A-6: Projects and Workspaces are not recreated as MCPProxy profiles in this release; the original grouping is reported so a later feature or the user can recreate it. +- A-7: The Web UI entry is P3 and non-gating. It ships only if it reuses the current import panel and REST endpoints with no new import code path; the plan places it last so it can be dropped alone. +- A-8: The `import_source` counter is a count of daemon-applied successful runs, not distinct installs. Installs can be approximated on the dashboard by `anonymous_id` (not `heartbeat_id`, which rotates every accepted window); that approximation is acknowledged as imprecise. +- A-9: Servers copied by MCP Router from the user's own client configs may duplicate servers MCPProxy already has; name-based skip handles it and the summary says so. +- A-10: `--no-quarantine` is intentionally absent from this command. A user who wants it can approve in the review flow like for any server. diff --git a/specs/120-mcp-router-import/tasks.md b/specs/120-mcp-router-import/tasks.md new file mode 100644 index 000000000..98eaf67a3 --- /dev/null +++ b/specs/120-mcp-router-import/tasks.md @@ -0,0 +1,171 @@ +# Tasks: MCP Router Importer (`mcpproxy import --from mcp-router`) + +**Input**: `specs/120-mcp-router-import/` (spec.md, plan.md, research.md, data-model.md, contracts/cli.md, contracts/rest.md, quickstart.md) +**Tests**: REQUIRED (constitution V: test first; each implementation task follows its test task and the test must be seen failing first). +**Routing note** (global model routing): T-tasks marked **[opus-review]** touch security-sensitive code (containment, secrets, apply, telemetry gate) and get an independent Sonnet/Opus subagent review in addition to the Sol review; the rest is Sonnet-implementable. + +## Format: `- [ ] T### [P?] [US#?] Description with file path` + +`[P]` = parallelisable (different files, no unfinished dependency). Paths are repo-relative. `internal/mcprouter` is the new package; it is the **only** importer of `modernc.org/sqlite` (plan D1). + +## Path conventions + +Go: `internal/mcprouter/`, `internal/configimport/`, `internal/telemetry/`, `internal/cliclient/`, `internal/httpapi/`, `cmd/mcpproxy/`. Docs: `docs/`. Frontend (P5 only): `frontend/src/`. + +--- + +## Phase 1: Setup + +- [ ] T001 Create package skeleton `internal/mcprouter/doc.go` (package comment: purpose, "only importer of modernc.org/sqlite", untrusted-input rules) and empty `locate.go`, `snapshot.go`, `reader.go`, `export.go`, `resolve.go`, `plan.go`, `clients.go`, `apply.go`, `view.go` so later tasks only fill them. +- [ ] T002 Add `modernc.org/sqlite` to `go.mod`/`go.sum` (`go get modernc.org/sqlite@v1.60.1`, run `go mod tidy`); import it only from `internal/mcprouter/reader.go`. Add a CI guard `scripts/check-sqlite-import-scope.sh` (fails if any other package imports it) and call it from the same workflow that runs `scripts/check-upstream-write-sites.sh`. +- [ ] T003 **Dependency gate (plan D1).** Build the release binary with the release flags (`-ldflags "-s -w"`) with and without the driver for linux/amd64, darwin/arm64, windows/amd64 (`GOOS=... GOARCH=... go build`), record the stripped-size deltas in `specs/120-mcp-router-import/research.md` under a new section "Dependency measurement". If any delta exceeds 10 MB: STOP, put the direct-DB reader behind a build tag `mcprouter_db` (default off) and ship the export-file path only; note it in the PR. Otherwise proceed. +- [ ] T004 [P] Write the in-test fixture builder `internal/mcprouter/fixture_test.go` (+ `internal/mcprouter/fixturegen/` helper exposed as `TestFixtureWriteDir` with `-args -out ` for quickstart): creates a real SQLite data directory with MCP Router's `CREATE TABLE` statements (servers, projects, workspaces), WAL mode with an unflushed `-wal`, rows for: local stdio, remote (SSE), remote-streamable, disabled, tool_permissions with `false` entries, plain bearer token, `mcpr-secret:v1:` encrypted token, `${param}`/`${user_config.x}` args with `input_params`, hostile values (control chars, `${keyring:x}`, `file:` URL, 70 KiB string), a second local workspace under `workspaces//database.db`, a workspace row whose `databasePath` points outside the directory, and a legacy variant without later columns and without a `workspaces` table. Also writes `mcp-servers-.json` in MCP Router's export shape, including an empty-command remote entry. Canary secret strings are constants shared by tests. +- [ ] T005 [P] Add the canary scanner helper `internal/mcprouter/canary_test.go`: `AssertNoCanary(t, ...)` checks stdout, stderr, JSON/YAML output, log buffers, the config file and a telemetry payload for any canary substring. Used by every later story. + +## Phase 2: Foundational (blocks all stories): read, resolve, plan (no writes) + +**Goal**: produce a correct `Plan` from any supported input. No CLI, no writes. + +- [ ] T006 [P] Test `internal/mcprouter/locate_test.go`: default per-OS paths (macOS, Windows, Linux via an injectable `Env{GOOS, Home, AppData}`), input kind detection (`data_dir`, `database`, `export_file`, unrecognised file → error naming what was expected), symlink-swap of data dir contents, sibling-directory prefix (`MCP Router-evil`), absolute and `..` workspace `databasePath` rejected with warning `workspace_path_rejected`, workspace id pattern, regular-file requirement. **[opus-review]** +- [ ] T007 Implement `internal/mcprouter/locate.go` per plan D3: `Locate`, `DefaultLocations`, all file access through `os.OpenRoot`. **[opus-review]** +- [ ] T008 [P] Test `internal/mcprouter/snapshot_test.go`: stable copy; WAL present/absent; concurrent writer goroutine; WAL checkpoint/truncate/replace/disappear during copy forces retry; file-replacement with identical size+mtime detected via identity; 3 failed attempts → `ErrNoConsistentSnapshot`; size cap 1 GiB → `ErrSourceTooLarge`; temp dir mode 0700 and always removed (also on context cancel). **[opus-review]** +- [ ] T009 Implement `internal/mcprouter/snapshot.go` per plan D2 (descriptor copy, pre/post identity, `quick_check`, 30 s deadline, bounded retries). +- [ ] T010 [P] Test `internal/mcprouter/reader_test.go`: reads all fixture rows; missing columns take defaults; missing `workspaces` table → single `local-default`; limits (5,000 servers, 256-byte name, 64 KiB fields, 1,024 args, JSON depth 8) fail the row with `row_too_large`/`bad_json` instead of the run; `tool_permissions` false → `ToolOff`; bearer state `none|plain|encrypted`; remote workspaces counted not read; presence of workflow/hook/skill/log tables → `NotImported` flags (table existence and row counts only, never contents). +- [ ] T011 Implement `internal/mcprouter/reader.go` (queries by column-existence probe; context deadlines; the only file importing the driver). +- [ ] T012 [P] Test `internal/mcprouter/export_test.go`: export shape parsed; empty-command entry flagged `RemoteURLNotExported`; unknown shape rejected; 4 MiB cap; `FromExport` set (disabled/tool toggles/token unknown). +- [ ] T013 [P] Implement `internal/mcprouter/export.go`. +- [ ] T014 [P] Test `internal/mcprouter/resolve_test.go`: table tests for plan D5 and its provenance (`${x}` env-first, `${user_config.x}` default-first, empty env treated absent, partial-string `--a=${x}` left literal, conflicting values); provenance marks a slot sensitive for sensitive param, secret-named param **and secret-like env name** (`args=["${API_KEY}"]` with ordinary-looking value); leftover or source-originated `${...}` (incl. `${keyring:x}`, `${env:x}`) → `unresolved_placeholder`. +- [ ] T015 Implement `internal/mcprouter/resolve.go` (`ResolveArgs` returning per-slot provenance). +- [ ] T016 Export `ClassifyEnv` and `ClassifyHeaders` wrappers in `internal/configimport/summary.go` (thin, with a test in `internal/configimport/summary_classify_test.go`); no behavior change to existing callers. +- [ ] T017 [P] Test `internal/mcprouter/plan_test.go`: mapping table (FR-006) for every kind; `enabled = !disabled`; `DisabledTools`; protocol `stdio|sse|streamable-http`; bearer plain → `Authorization: Bearer` header classified secret; bearer encrypted → class `needs_configuration`, server disabled, no header; unmappable reasons (empty command, no URL, non-http(s) URL, invalid name, `${...}` leftovers, inline credential without `--secrets-inline`); self/MCP Router-endpoint skip (`skipped_self`, port 3282); name sanitising, collisions (`collision`, default workspace wins, `--workspace` filter); comparison against existing servers per FR-016a (already_present vs conflict naming fields only; sensitive slot token accepts any `${keyring:*}`; operator-edited enabled/quarantine ignored); `NotCarried` lists; every item `Quarantined=true`. +- [ ] T018 Implement `internal/mcprouter/plan.go` and `internal/mcprouter/view.go` (the `ItemView` DTO with no value fields, importer-owned summary with fixed `‹redacted›` masks for sensitive argument values and for URL userinfo/query/fragment; names escaped for terminals). **[opus-review]** +- [ ] T019 [P] Test `internal/mcprouter/view_test.go`: canaries in env, headers, args (innocuous parameter names, positional values), URL query, userinfo never appear in `ItemView` JSON/YAML/table output; no tail bytes or lengths in masks; `Plan`/`Item` are never marshalled (compile-time assertion that they have no `json` tags + a reflection test). +- [ ] T020 [P] Test `internal/mcprouter/clients_test.go`: six client config fixtures (JSON, VS Code `servers`, Codex TOML); bridge entry detected; endpoint entry (`localhost:3282`) detected; `MCPR_TOKEN` and headers never decoded (structs lack the fields; canary test); 1 MiB cap; Cline note; missing files ignored; files only opened read-only. +- [ ] T021 [P] Implement `internal/mcprouter/clients.go`, exposing the predicate reused by T018 for the FR-008 skip. + +**Checkpoint**: `BuildPlan(ctx, Input, ExistingServers, Options)` works for data dir, database file and export file; all package tests pass with `-race`. + +## Phase 3: US2 — Preview first, nothing changed (P1, MVP: read-only command) + +**Goal**: `mcpproxy import --from mcp-router [--path] --dry-run [--workspace] [-o json]` prints the plan and changes nothing. +**Independent test**: run against the T004 fixture; config, keyring and telemetry registry byte-identical before/after; JSON equals table content; exit 0. + +- [ ] T022 [P] [US2] Test `cmd/mcpproxy/import_cmd_test.go` (dry-run): flag validation (`--from` required, unknown source lists supported ones), auto-detect message naming checked locations when none found (exit 22), dry-run success exit 0 incl. all-unmappable plan, read failure in dry-run → 23/22, `-o json|yaml` structure equals `contracts/cli.md`, `--help-json` includes the command, canaries absent, no config/keyring/telemetry mutation (hash config file; fake keyring counts writes; registry snapshot). +- [ ] T023 [US2] Implement `cmd/mcpproxy/import_cmd.go`: cobra command, flags (`--from --path --dry-run --workspace --yes --secrets-inline`), `BuildPlan`, table and JSON/YAML rendering from `ItemView`, reconnect section; register in `cmd/mcpproxy/main.go` (`rootCmd.AddCommand(importCmd)`). +- [ ] T024 [P] [US2] Add `importOutcomeError` and exit band 20-24 to `cmd/mcpproxy/exit_codes.go`, teach `classifyError` in `cmd/mcpproxy/main.go`, and add `cmd/mcpproxy/exit_codes_import_test.go` table-testing `Classify(plan, outcomes)` for: mixed added+conflict, added+unmappable, all-unmappable, empty source, already-present-only, dry-run, source not found, unreadable, refused. **[opus-review]** +- [ ] T025 [US2] Wire `Classify(plan, outcomes)` into the command (`internal/mcprouter/classify.go`) and make T022/T024 pass. + +**Checkpoint**: a read-only importer is shippable on its own (user-visible value: they see exactly what would happen). + +## Phase 4: US1 — Move my servers in one command (P1) + +**Goal**: apply the plan: servers added quarantined, disabled/hidden-tool state preserved, per-server true outcome. +**Independent test**: quickstart steps 2 on a scratch instance in daemon mode and in file mode. + +- [ ] T026 [P] [US1] Test REST/client `disabled_tools`: `internal/httpapi/server_add_disabled_tools_test.go` (create with `disabled_tools` stores it; omitted → unchanged behavior; `enabled_tools` not accepted/ignored as documented) and `internal/cliclient/client_add_test.go` (request marshalling). +- [ ] T027 [US1] Add `DisabledTools []string \`json:"disabled_tools,omitempty"\`` to `AddServerRequest` in `internal/cliclient/client.go` and `internal/httpapi/server.go`; apply to `ServerConfig.DisabledTools` in the add handler; update `docs/api/rest-api.md` and `oas/swagger.yaml` (run the repo's swagger generation/verify command). +- [ ] T028 [P] [US1] Test `internal/mcprouter/apply_test.go` (non-secret paths): fake `Sink`; added servers carry `Quarantined=true`, `Enabled`, `DisabledTools`; definite rejection → `failed`; per-server outcomes returned; read-back fills actual `Enabled/Quarantined` through the `GetServers` projection and `unknown` when the read fails; global quarantine disabled still reports the stored state. +- [ ] T029 [US1] Implement `internal/mcprouter/apply.go` (`Apply`, `Sink`, `ServerOutcome`) and `daemonSink` (uses `cliclient.AddServer` + `GetServers` read-back). **[opus-review]** +- [ ] T030 [P] [US1] Test `internal/mcprouter/filesink_test.go`: import lock (`import.lock`, `O_EXCL`, stale detection by pid/age, released on all exits incl. panic/SIGINT), per-server reload-recheck-append-save, a name that appeared concurrently → `already_present`, failed candidate never persisted by a later save, two concurrent importers serialised, residual-daemon-started-mid-run documented. **[opus-review]** +- [ ] T031 [US1] Implement `fileSink` and the lock in `internal/mcprouter/filesink.go`; share the lock with `daemonSink` (apply phase is serialised in both modes, plan D4). +- [ ] T032 [P] [US1] Test `cmd/mcpproxy/import_daemon_probe_test.go`: tri-state probe (reachable / definitively absent / inaccessible); inaccessible and explicit-endpoint failures stop with an error and never fall back; sink frozen after the first add; no file-mode fallback after an add error. +- [ ] T033 [US1] Implement the probe and sink selection in `cmd/mcpproxy/import_cmd.go` (reuse `daemonEndpoint`/`newDaemonClient` internals without their boolean collapse). +- [ ] T034 [P] [US1] Test `cmd/mcpproxy/import_apply_test.go`: interactive confirmation (`[y/N]` once), `--yes`, non-TTY without `--yes` → refuse (24), servers listed `uncertain` for transport errors, exit statuses (0, 20, 21), reconnect advice printed after a real run, hidden-tool and disabled state present via `upstream list -o json` against an in-process daemon. +- [ ] T035 [US1] Implement apply flow in `cmd/mcpproxy/import_cmd.go`: plan view → confirmation prompt (reuse the TTY check pattern of `confirmation.go`) → `Apply` → summary → exit class. +- [ ] T036 [US1] E2E step: add an import scenario to `scripts/test-api-e2e.sh` (scratch data dir, high port, fixture from `TestFixtureWriteDir`; asserts quarantined, disabled, hidden tools, second run no-op). + +**Checkpoint**: MVP complete for non-secret servers. + +## Phase 5: US3 — No secret moves silently (P1) + +**Independent test**: fixture with API-key env, plain token, encrypted token; verify what is shown, stored and where; canary scan. + +- [ ] T037 [P] [US3] Test `internal/mcprouter/secrets_test.go`: reference names `mcpr-import--` deterministic, fixed length, unique per server+field; existing value remembered and restored on rollback; entry without our prefix never overwritten; mark-before-`Store`; partial `Store` failure (value written, registry update fails) and timeout handled as indeterminate with names listed; keyring unavailable + secrets + no `--secrets-inline` → refuse before any write (exit class f) incl. macOS-gate reason text with the exact `MCPPROXY_KEYRING_WRITE=1` remedy; `--secrets-inline` keeps values inline; inline-credential servers blocked without it; `${keyring:...}` inserted after the source `${` check; sensitive slots carry the reference; encrypted token → server added disabled without header, "needs bearer token"; `MCPR_TOKEN` never read. **[opus-review]** +- [ ] T038 [US3] Implement `internal/mcprouter/secrets.go` (reference naming, backup/restore, compensation, provenance-driven keyring writes) and hook it into `Apply` (store → add → on definite rejection compensate; on indeterminate result keep references and mark `uncertain`; later `GetServers` read upgrades to `added`). **[opus-review]** +- [ ] T039 [P] [US3] Test `internal/mcprouter/apply_uncertain_test.go`: commit-succeeds-response-lost keeps references; timeout then absent stays `uncertain`; timeout then present → `added`; definite 4xx → compensation of that server only and committed servers' references untouched; two importers concurrently on the same server (lock) never delete each other's references. +- [ ] T040 [US3] Plan view and refusal text in `cmd/mcpproxy/import_cmd.go`: per-server secret handling lines (`keyring`/`inline`/`missing`/`blocked_inline_credential`) with names and flags only; confirmation line "Add N servers (X secrets to keyring, Y inline)?"; the exact remedy for the macOS gate. +- [ ] T041 [US3] Canary sweep test `cmd/mcpproxy/import_canary_test.go`: dry run, real run, refused run, failed run, JSON and YAML modes, `--log-level=debug`; assert zero canary bytes in stdout, stderr, log files, config file (only references), telemetry payload. + +## Phase 6: US4 — Run it twice, nothing breaks (P1) + +- [ ] T042 [P] [US4] Test `internal/mcprouter/idempotency_test.go`: re-run is a no-op and config file byte-identical; new source server added alone; interrupted run completes without duplicates and reuses deterministic references; conflict reports field names only; operator-renamed keyring reference at a sensitive slot still `already_present`; operator-changed enabled/approval state ignored; collision and case/whitespace name rules (FR-016a). +- [ ] T043 [US4] Make T042 pass (comparison in `plan.go`, reuse in `apply.go`); fix any gaps found. + +## Phase 7: US8 — Measure the experiment anonymously (P1) + +- [ ] T044 [P] [US8] Test `internal/telemetry/import_counter_test.go`: `Service.RecordImportRun` records once per call when enabled; nothing when config opt-out or any env override (use `env_overrides` fixtures); opt-out toggled concurrently under `-race`; disabled → import → enabled emits nothing retroactively; `Drain/Snapshot/Reset/HasPendingCounters` include the counter; frozen-window retry keeps and dedups the delta; payload `import_source_counts` omitted when empty, keys only from the closed enum, integers ≥ 0. **[opus-review]** +- [ ] T045 [US8] Implement `ImportSource` enum, `CounterRegistry.importCounts`, `Service.RecordImportRun` in `internal/telemetry/registry.go` and `internal/telemetry/telemetry.go`; payload field `import_source_counts`. +- [ ] T046 [P] [US8] Test privacy scan: extend `internal/telemetry/payload_privacy_test.go` with positive and negative cases (unknown key, negative number, string value, nested object, key containing a path or server name) for rule `import_source_field_invalid`. +- [ ] T047 [US8] Implement the structural rule in `internal/telemetry/anonymity.go`. +- [ ] T048 [US8] Schema bump to the next version as its own commit: `rg` all current-version (v16 at the time of writing) pins in code, tests, `internal/telemetry/testdata`, docs; update them; extend `internal/telemetry/schema_version_guard_test.go` to the current to next transition; keep historical v14 activation/cohort thresholds; add the "Schema v17 (or next free)" section to `docs/features/telemetry.md` (what is counted: runs, closed enum, never names/paths/counts/servers; opt-outs; undercount when the core is not running). +- [ ] T049 [P] [US8] Test `internal/httpapi/import_report_test.go`: `POST /api/v1/servers/import/report` with `{"source":"mcp-router"}` → 204 and counter incremented when enabled; 204 and no count when opted out (response identical); unknown source → 400; missing auth → 401/403 like other server-add routes (`requireServerOp(auth.ServerOpAdd, ...)`). +- [ ] T050 [US8] Implement the handler and route in `internal/httpapi/server.go`; add `Client.ReportImport(ctx, source)` in `internal/cliclient/client.go`; document in `docs/api/rest-api.md`/`oas/swagger.yaml`. +- [ ] T051 [US8] CLI: after `Apply`, if the daemon sink was used and ≥1 outcome is `added`, call `ReportImport` once (never for dry run, file mode, refused, all-failed, `uncertain`-only). Test in `cmd/mcpproxy/import_telemetry_test.go` with a recording fake daemon. + +## Phase 8: US5 — What did not come across, what to reconnect (P2) + +- [ ] T052 [P] [US5] Test `cmd/mcpproxy/import_report_test.go`: not-carried-over lines per server; DXT-path dependency warning (commands under `mcp-servers/dxt/`); unresolved placeholders listed; private-address URL warning; categories not imported; reconnect section with `mcpproxy connect ` per detected client, Cline one-liner, entry removal note; no client config file modified (hash before/after); JSON `reconnect[]` shape. +- [ ] T053 [US5] Implement report sections in `cmd/mcpproxy/import_cmd.go` using `Plan.NotImported`, `NotCarried`, `Clients`; add the next-step hints (`mcpproxy upstream list`, review queue). + +## Phase 9: US6 — Use the exported file (P2) + +- [ ] T054 [P] [US6] Test `cmd/mcpproxy/import_export_file_test.go`: `--path` to export, to directory and to database each accepted with input kind stated; random JSON/binary rejected with expectation; empty-command entry "URL was not exported"; export-loss warning (disabled state, tool toggles, tokens, parameter schemas, other workspaces; servers arrive enabled with all tools visible); same secret/quarantine/idempotency rules. +- [ ] T055 [US6] Make T054 pass (mostly wiring; fix gaps in `export.go`/`plan.go`/`import_cmd.go`). + +## Phase 10: Docs and acceptance + +- [ ] T056 [P] Write `docs/features/mcp-router-import.md` (what is carried over, what is not, secret handling, macOS keyring note with the exact environment variable, after-import checklist per client, undercount note) and add it to the docs allowlist and `website/sidebars.js` per the docs-site pipeline; link check against the docs build. +- [ ] T057 [P] Write `docs/cli/import-command.md` (flags, exit codes 0/20-24, JSON schema from `contracts/cli.md`); add the exit-code band to `docs/cli-management-commands.md`. +- [ ] T058 [P] Add a "Recent Changes" line for spec 120 to `CLAUDE.md` (lean, one paragraph) and the command to the CLI list. +- [ ] T059 Hand-off for FR-026: write `specs/120-mcp-router-import/handoff-website.md` with the final command text, flags, exit behavior, docs URL, and the caveats the migration page must repeat (macOS keyring variable, remote tokens re-entry); send it to the website session via the shared notes folder. The marketing page must link to the docs page and not contradict it. +- [ ] T060 Acceptance run, results recorded in the PR (not committed): SC-001 timed run with one confirmation; SC-002 50-server fixture dry-run vs apply times < 5 s each and plan equality; SC-004 canary matrix green; SC-005 byte-identical config on re-run; SC-006 all quarantined; SC-009 one manual reconnect check per client (Claude Desktop, Cursor, Windsurf, VS Code, Codex). +- [ ] T061 Real-data QA: install MCP Router 0.6.4 (final release) in a scratch home, create servers incl. a remote one with a bearer token and tool toggles, run the importer against the real data directory while the app is running and after quitting; record findings; update `research.md` if the real schema differs. +- [ ] T062 Lint and test gate: `golangci-lint run --config .github/.golangci.yml ./...` and with `--build-tags server`; `go test -race ./internal/mcprouter/... ./internal/telemetry/... ./internal/httpapi/... ./cmd/mcpproxy/...`; the CI `-skip` regex run for `internal/server`/`httpapi`/`storage` under `-tags server`; `./scripts/test-api-e2e.sh`; mutation spot-checks (disable the `os.Root` containment, the `${` check, the quarantine flag, keyring-before-add order, the telemetry gate, one at a time: a test must fail each time). +- [ ] T063 Cross-model review of the implementation PR(s) with opencode `github-copilot/gpt-6.1-sol` (read-only, chunked by package, empty output = UNREVIEWED), round cap 10; independent Sonnet/Opus subagent review for the **[opus-review]** items. + +## Phase 11: US7 — Web UI entry (P3, non-gating, droppable; ship last) + +Skip this phase entirely if time or review capacity is short; nothing above depends on it. + +- [ ] T064 [P] [US7] Test `internal/httpapi/import_mcp_router_test.go`: `GET /api/v1/servers/import/mcp-router` (found/not found, workspaces, count); `POST` preview returns the `ItemView` plan with no values; `POST apply` honours `secrets:"keyring"|"inline"`, `inline_credentials_ok`, per-server selection; quarantine always set; placeholder rejection before any destination reference; same authorization as other server-adding routes; telemetry recorded in-process via `Service.RecordImportRun`; existing `/servers/import*` endpoints unchanged. +- [ ] T065 [US7] Implement the endpoints in `internal/httpapi/import_mcp_router.go` reusing `internal/mcprouter` (daemon-side sink = in-process add path; same lock, outcomes, read-back); update `oas/swagger.yaml` and `docs/api/rest-api.md`. +- [ ] T066 [US7] Frontend: add "MCP Router" as a detected source in `frontend/src/components/ImportServersPanel.vue` (and its entry in `AddMenu.vue` if the detected-sources list lives there), preview table from the plan DTO, secret-handling choices, confirmation, outcome and reconnect list; unit test under `frontend/tests/unit/*.spec.ts`; run the Playwright sweep per `docs/development/web-ui-verification.md`. + +--- + +## Dependencies and order + +```text +Setup (T001-T005; T003 gate decides the reader's build mode) + └─▶ Foundational (T006-T021) + └─▶ US2 dry-run (T022-T025) ← shippable read-only MVP + ├─▶ US1 apply (T026-T036) + │ ├─▶ US3 secrets (T037-T041) + │ └─▶ US4 idempotency (T042-T043) [needs US1 apply for the re-run] + │ └─▶ US8 telemetry (T044-T051) [needs US1 apply; independent files] + ├─▶ US5 report (T052-T053) [after US2; reconnect data from T021] + └─▶ US6 export file (T054-T055) [after US2] +Docs/acceptance (T056-T063) after US1+US3+US4+US8 +US7 web (T064-T066) last, droppable +``` + +US3 must land before the first real (non-dry) release of US1 to users: until secrets are handled, the command is dry-run only (guard with `--dry-run` forced when T037-T041 are not merged; do not release apply without them). + +## Parallel examples + +- After T005: T006, T008, T010, T012, T014, T016, T017, T019, T020 can be written in parallel (different test files), then their implementations T007, T009, T011, T013, T015, T018, T021. +- After US2: T026/T028/T030/T032 tests in parallel; US5 (T052) and US6 (T054) tests in parallel with US1 implementation. +- Docs T056-T058 in parallel with US8. + +## Implementation strategy + +1. **Release-able slices.** PR-A: Setup+Foundational+US2 (read-only command, no writes, no secrets, no telemetry). PR-B: US1+US3+US4 (apply, hard-dependent trio). PR-C: US8 telemetry + the next schema version (v17 when written) (its own commit). PR-D: US5+US6+docs. PR-E (optional): US7 web UI. Merge each before starting the next (no stacking). +2. **Gate first.** T003 decides the shape of everything else: if the dependency gate fails, PR-A ships with the export-file reader only. +3. **Security-sensitive tasks** (T006-T009, T018, T024, T029-T031, T037-T039, T044) get the independent review and the mutation checks in T062. +4. **Experiment readout.** Success = ≥ 25 runs counted under `import_source=mcp-router` within 60 days of the release carrying PR-C; known undercounts: daemonless runs, opted-out installs, runs where macOS keyring gating stopped the user (track via support feedback). + +## Follow-ups (not in this feature) + +- `upstream import` daemon mode drops `Enabled` and `disabled_tools` and swallows add errors (`cmd/mcpproxy/upstream_cmd.go:applyImportedServersDaemonMode`): separate issue. +- If the macOS keyring gate is the main drop-off, let the importer ask the running daemon (opted in by the tray) to store secrets over REST. +- Recreate MCP Router Projects/Workspaces as Spec 108 profiles once those settle.