From 961a7d8acae3c93d70e881b53e4db298c880dc85 Mon Sep 17 00:00:00 2001 From: Christophe Vila Date: Sat, 25 Jul 2026 20:04:18 +0200 Subject: [PATCH] docs: add design spec for dropping MCP in favor of a direct Garmin wrapper Replaces internal/garmin's MCP-based mcp-garmin integration with a custom newline-delimited-JSON subprocess protocol around python-garminconnect directly, folded into this repo. MCP's dynamic tool-discovery value is unused here (fixed call sites, no LLM choosing tools), and both projects are owned by the same person, so the extra protocol layer and two SDK dependencies (mcp-go, mcp[cli]) were pure overhead -- especially given plans to containerize the backend. Co-Authored-By: Claude Sonnet 5 --- ...2026-07-25-garmin-direct-wrapper-design.md | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-25-garmin-direct-wrapper-design.md diff --git a/docs/superpowers/specs/2026-07-25-garmin-direct-wrapper-design.md b/docs/superpowers/specs/2026-07-25-garmin-direct-wrapper-design.md new file mode 100644 index 0000000..1fbf5da --- /dev/null +++ b/docs/superpowers/specs/2026-07-25-garmin-direct-wrapper-design.md @@ -0,0 +1,102 @@ +# Drop MCP for Garmin integration — direct Python wrapper design + +Date: 2026-07-25 + +## Overview + +`internal/garmin` currently talks to `mcp-garmin` (a separate repo) as an MCP client over stdio, spawning `server.py` as a subprocess and driving it through JSON-RPC tool calls (`authenticate`, `complete_mfa`, `get_activities`, `get_activity_splits`, `get_activity_details`, `get_workout_by_id`). MCP's actual value — letting an LLM dynamically discover and choose among tools — is unused here: geniusrun's Go backend calls a fixed, hardcoded set of operations in fixed code paths. The MCP protocol layer (session init, capability negotiation, JSON-RPC envelope, the `mcp-go` client dependency in Go, the `mcp` SDK dependency in Python) buys nothing for this use case. + +Both `mcp-garmin` and geniusrun are projects the same person owns, so there's no third-party maintenance benefit to keeping `mcp-garmin` as a separate abstraction either. Given a plan to package the backend in a container, fewer runtime dependencies is a concrete win: dropping the MCP layer means one fewer Python package (`mcp[cli]`) and one fewer Go dependency (`github.com/mark3labs/mcp-go`), and folding the wrapper script into the geniusrun repo itself means the container image doesn't need to vendor/clone a second repo. + +This replaces the MCP transport with a small custom Python subprocess wrapper around `garminconnect` directly, speaking a minimal newline-delimited JSON protocol over stdin/stdout. `mcp-garmin` is retired once this ships. + +## Goals + +- Remove the MCP protocol layer and its two SDK dependencies (`mcp-go` in Go, `mcp[cli]` in Python) entirely. +- Fold the Python wrapper into the geniusrun repo (`backend/garmin-wrapper/wrapper.py`) so the container image only needs a Python runtime + `pip install garminconnect` + this one file — no separate repo to fetch at build time. +- Preserve `internal/garmin.Client`'s existing interface and behavior exactly (lazy subprocess spawn, per-user token store, `UpdateCredentials` tear-down/respawn, stderr draining) so `internal/sync` and `internal/api` require zero changes. +- Replace today's fragile string pattern-matching of auth replies (`parseAuthResult`, documented in CLAUDE.md as a "learned the hard way" wart) with structured JSON status, now that both sides of the protocol are under our control. +- Make every method `python-garminconnect`'s `Garmin` class exposes reachable from Go without per-method Python boilerplate, anticipating future AI-driven features (recovery/HRV context for training-plan recommendations) that may need data beyond what's synced today. + +## Non-goals + +- No change to what data geniusrun actually syncs/stores/classifies today — only the transport between Go and Python changes. `internal/sync`, `internal/classify`, `internal/store`, and the frontend are untouched. +- No security sandboxing of the generic dispatcher (see below) — the subprocess is spoken to only by geniusrun's own Go process over a private pipe it owns, not exposed to any other caller, so an open dispatch surface is an accepted risk, not a gap. +- No auto-restart-on-crash logic for the subprocess. Matches today's behavior: a dead subprocess surfaces as a Go error on next use; only `UpdateCredentials` explicitly tears down and respawns. +- No change to the per-user token store layout (`{GarminTokenStoreRoot}/{userID}`) or the multi-tenant `garminFor` caching in `api.Server`. + +## Wire protocol + +Newline-delimited JSON, one object per line each direction, over the subprocess's stdin/stdout: + +``` +Go -> Python: {"id": 1, "cmd": "authenticate"} +Python -> Go: {"id": 1, "result": {"status": "success", "message": "Authenticated successfully."}} + +Go -> Python: {"id": 2, "cmd": "complete_mfa", "params": {"code": "123456"}} +Python -> Go: {"id": 2, "result": {"status": "mfa_required", "message": "..."}} + +Go -> Python: {"id": 3, "cmd": "call", "params": {"method": "get_activities_by_date", "args": {"start_date": "2026-07-01", "end_date": "2026-07-25"}}} +Python -> Go: {"id": 3, "result": [...]} + +Go -> Python: {"id": 4, "cmd": "call", "params": {"method": "get_activity_splits", "args": {"activity_id": "123"}}} +Python -> Go: {"id": 4, "error": "Error fetching activity splits: "} +``` + +Three command kinds: + +- **`authenticate`** / **`complete_mfa`** — special-cased in Python: same background-thread login + MFA-prompt race + 10-second timeout detection logic `mcp-garmin`'s `server.py` already has (this is genuine custom control flow, not a passthrough to a single `garminconnect` method), but now return a **structured** `{"status": "success" | "mfa_required" | "failed", "message": "..."}` object instead of a string Go has to pattern-match. +- **`call`** — a fully generic dispatcher: `getattr(garmin_client, params["method"])(**params["args"])`, JSON-serializing whatever it returns. This is what makes every `garminconnect` method reachable without new Python code per method — including future needs (sleep, HRV, stress, body battery) and, by the same generic mechanism, mutating/destructive methods (`upload_activity`, `delete_activity`, etc.). No allowlist is enforced in Python; correctness of what Go chooses to call is Go's responsibility, same as calling any other library method directly would be. + +`id` lets Go assert a response matches its in-flight request — calls are already serialized by the existing `sync.Mutex` in `mcpClient`/its replacement, so this is a desync-detection assertion, not a concurrency mechanism. + +Go's line reader uses a raised buffer size (well above the default 64KB `bufio.Scanner` limit) since `get_activity_details` responses (per-second telemetry for a full run) can be several MB. + +## Components + +### Python wrapper — `backend/garmin-wrapper/wrapper.py` + +- Single file. Same shape as today's `server.py` minus all MCP scaffolding (`FastMCP`, `@mcp.tool()` decorators, `mcp.run()`). +- Reads `GARMIN_EMAIL` / `GARMIN_PASSWORD` / `GARMIN_TOKENSTORE` from the environment at process start — unchanged from today. +- Holds one module-level `Garmin` client instance and an auth-state variable (`unauthenticated` / `mfa_pending` / `authenticated`). +- Main loop: `for line in sys.stdin`, parse `{"id", "cmd", "params"}`, dispatch to the `authenticate` / `complete_mfa` / `call` handler, write exactly one JSON line to stdout per response, flush immediately. Debug/diagnostic output continues to go to stderr only (stdout is reserved for the response protocol) — Go already drains stderr today and keeps doing so unchanged. +- Any exception raised by a `call` dispatch (bad method name via `AttributeError`, `GarminConnectAuthenticationError`, network errors, anything else) is caught at the top-level dispatch loop and turned into `{"id": N, "error": ""}` — it must never let a raw Python traceback hit stdout, since that would corrupt the line-based framing. +- Dependency: `garminconnect` only. Drop `mcp[cli]`. + +### Go client — `internal/garmin/client.go` + +- Same `mu sync.Mutex` + lazy `ensureStarted`/`started` pattern as today. The mcp-go stdio transport is replaced with `os/exec.Cmd` plus a `bufio.Scanner` (raised buffer) over `Stdout` and a `json.Encoder` over `Stdin`. +- `UpdateCredentials` still tears down (kill subprocess) and lets the next call respawn fresh, unchanged. +- `Close` still terminates the subprocess. +- Each `Client` interface method builds the appropriate request (`authenticate`/`complete_mfa` directly, or a `call` envelope naming the right `garminconnect` method for `GetActivities` → `get_activities_by_date`, `GetActivitySplits` → `get_activity_splits`, `GetActivityDetails` → `get_activity_details`, `GetWorkoutByID` → `get_workout_by_id`), writes+flushes it, reads one response line, and unmarshals `result` into the existing typed structs (`Activity`, `ActivitySplits`, `ActivityDetails`, `Workout` — unchanged; the wrapper still forwards `garminconnect`'s own JSON-compatible dicts/lists as-is). +- `AuthResult`/`AuthStatus` types are unchanged, but `parseAuthResult`'s string-matching (`client.go:189-198`) is deleted entirely — the wrapper's `status` field maps directly to `AuthSuccess` / `AuthMFARequired` / `AuthFailed`. +- The interface itself gains no new methods yet (YAGNI) — the generic `call` command exists at the wire-protocol level so a future typed method (e.g. for sleep/HRV data, if the AI training-plan features end up needing it) is a small, self-contained addition when actually needed, not a protocol change. + +## Error handling + +- Python: every `call` dispatch exception is caught and returned as `{"error": ""}`. `authenticate`/`complete_mfa` never raise past their own handlers (matches today). +- Go: a non-nil `error` field in a response becomes `fmt.Errorf("call %s: %s", method, resp.Error)` (or equivalent for `authenticate`/`complete_mfa`), the same shape `internal/sync` already receives and wraps further today (`fmt.Errorf("get_activities(...): %w", err)`, etc.) — confirmed no code downstream branches on specific error text (e.g. no "Authentication error" string-matching in `internal/sync`/`internal/api`), so no structured error-type field is needed beyond the plain message. +- Process-level failures (subprocess exited, pipe closed, malformed JSON line) surface as a Go error from the read/write call. `started` is not auto-reset on such failures — matching today's behavior where a crashed subprocess just errors on next use until `UpdateCredentials` forces a respawn. + +## Process lifecycle & testing + +- Lifecycle unchanged: lazy-start on first call, one persistent subprocess per user (kept warm across calls to preserve the Garmin session/tokenstore), torn down and respawned only by `UpdateCredentials`. +- `internal/garmin/mock.Client` is untouched — it fakes the `Client` interface, not the transport, so `internal/sync`/`internal/api` tests need no changes. +- New/updated tests in `internal/garmin`: + - Framing round-trip: a request encodes correctly, a response line decodes correctly, and a large (multi-MB) `get_activity_details`-shaped payload doesn't overflow the scanner buffer. + - Structured auth status mapping (replacing today's `parseAuthResult` string-matching tests). + - Error propagation: a `{"error": ...}` response becomes a Go error with the expected message. + - These can run against a stub script (not the real `wrapper.py`/`garminconnect`) that speaks the protocol, mirroring how `client_test.go` already avoids hitting real Garmin. + +## Migration & container + +- Full replacement, no compatibility shim, consistent with this project's "no migration history" stance elsewhere: `mcp-go` import and all MCP-specific code in `client.go` are deleted in the same change, not kept behind a flag. +- `backend/start.sh`'s `MCP_GARMIN_PYTHON` / `MCP_GARMIN_SERVER` env vars are renamed (e.g. `GARMIN_WRAPPER_PYTHON` / `GARMIN_WRAPPER_SCRIPT`) and default to paths inside the geniusrun repo (`backend/garmin-wrapper/`) instead of a sibling `mcp-garmin` checkout. +- Container image only needs: a Python runtime, `pip install garminconnect`, and `backend/garmin-wrapper/wrapper.py` copied in — no second repo to clone/vendor, no `mcp` SDK on either side. +- The standalone `mcp-garmin` repo is retired (archived) once this ships; its test suite (`tests/test_server.py`) either gets ported to test `wrapper.py` directly inside geniusrun, or is dropped in favor of the new `internal/garmin` tests above, whichever ends up covering the same ground with less duplication — a call to make during implementation, not part of this spec. + +## Rollout notes + +- No data migration involved — this only changes how the Go backend talks to Garmin, not what's stored. +- Existing per-user token stores under `GarminTokenStoreRoot` are unaffected (same `GARMIN_TOKENSTORE` env var name passed to the subprocess, same directory layout) — no re-login required for existing users after cutover. +- Deploy order: ship the new wrapper + Go client together as one change (interface unchanged, so this is an internal swap, not a rolling/compat-sensitive migration).