From 77f9588c2d6abbbb0958a534aa2372142ef08900 Mon Sep 17 00:00:00 2001 From: Christophe Vila Date: Sun, 26 Jul 2026 21:25:39 +0200 Subject: [PATCH] refactor(config): rename default Garmin token-store dir to .garmin Shorter, more conventional dotdir name than garmin-tokenstores. Restore the "next to DBPath" join that got dropped when the default was inlined via getEnvDefault -- an explicit GARMIN_TOKENSTORE still wins verbatim, but the implicit default must still resolve relative to the DB file's directory, not the process's CWD. internal/garmin/client.go's GARMIN_TOKENSTORE env var is now always set (TokenStorePath can no longer be empty), so the conditional that only appended it when non-empty is dead code. --- CLAUDE.md | 2 +- backend/internal/config/config.go | 17 ++-- backend/internal/config/config_test.go | 2 +- backend/internal/garmin/client.go | 9 +-- .../2026-07-25-garmin-direct-wrapper-plan.md | 77 +++++++++++++++---- 5 files changed, 73 insertions(+), 34 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f41d697..6c4025e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,7 +91,7 @@ Each `workout_kinds.rule_json` is a recursive AND/OR condition tree (`internal/c - **`authenticate`/`complete_mfa` return structured JSON** (`{"status": "success"|"mfa_required"|"failed", "message": "..."}`), not plain strings — `internal/garmin/client.go` maps `status` directly to `AuthStatus`, no string pattern-matching. - **The 10s "MFA required" timeout in `wrapper.py`'s `authenticate` handler is a false-positive trap.** A login that's merely slow (e.g. Garmin/Cloudflare rate-limiting) looks identical to a real MFA challenge unless you check whether `prompt_mfa()` was actually invoked (`wrapper.py` logs this to stderr for exactly this reason). -- **`wrapper.py` persists Garmin sessions** via a `GARMIN_TOKENSTORE` env var passed to `Garmin.login(tokenstore=...)` — without this, every process start does a full SSO login, which is what trips Garmin's rate limiting under repeated testing. Since geniusrun is multi-tenant, `GARMIN_TOKENSTORE` (`config.GarminTokenStoreRoot`) is a **root directory**, not a single session path: `api.Server.garminFor` namespaces each user's subprocess under `{root}/{userID}` so concurrent users never share a Garmin session. If unset, `config.Load()` defaults it to a `garmin-tokenstores` directory next to the DB file (logged at startup) rather than leaving per-user namespacing silently skipped. +- **`wrapper.py` persists Garmin sessions** via a `GARMIN_TOKENSTORE` env var passed to `Garmin.login(tokenstore=...)` — without this, every process start does a full SSO login, which is what trips Garmin's rate limiting under repeated testing. Since geniusrun is multi-tenant, `GARMIN_TOKENSTORE` (`config.GarminTokenStoreRoot`) is a **root directory**, not a single session path: `api.Server.garminFor` namespaces each user's subprocess under `{root}/{userID}` so concurrent users never share a Garmin session. If unset, `config.Load()` defaults it to a `.garmin` directory next to the DB file (logged at startup) rather than leaving per-user namespacing silently skipped. - **`activityId` is a large int64** — never round-trip it through `float64`/generic `map[string]any` JSON decoding, or it corrupts into scientific notation. Decode into typed structs (`garmin.Activity`, not `map[string]any`). - **`get_activity_details` returns raw per-second telemetry** (`activityDetailMetrics` + `metricDescriptors`), not lap/split summaries. Its `metricDescriptors` index-to-field mapping is **not stable across activities/devices** — `garmin.ExtractSamples` always resolves fields by descriptor key, never by fixed array position. - **`get_activity_splits`** returns the actual lap/split summaries (`lapDTOs`). diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index 523ad62..f744375 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -7,7 +7,6 @@ package config import ( "fmt" - "log" "os" "path/filepath" "strconv" @@ -29,7 +28,7 @@ type Config struct { // GarminTokenStoreRoot is the root directory under which each user's // Garmin session cache lives (one subdirectory per user id, e.g. // "/3"). Read from the GARMIN_TOKENSTORE env var; if unset, it - // defaults to a "garmin-tokenstores" directory next to DBPath so every + // defaults to a ".garmin" directory next to DBPath so every // deployment gets per-user isolation automatically -- multi-tenant // operation always relies on this being a real, distinct-per-user path // (see api.Server.garminFor), so it can never be silently left empty. @@ -69,11 +68,12 @@ type Config struct { // Load reads configuration from environment variables, applying defaults // for anything optional. Returns an error if a required variable is unset. func Load() (Config, error) { + dbPath := getEnvDefault("GENIUSRUN_DB_PATH", "geniusrun.db") cfg := Config{ Addr: getEnvDefault("GENIUSRUN_ADDR", ":8080"), - DBPath: getEnvDefault("GENIUSRUN_DB_PATH", "geniusrun.db"), + DBPath: dbPath, GarminPythonPath: getEnvDefault("GARMIN_WRAPPER_PYTHON", "python3"), - GarminTokenStoreRoot: os.Getenv("GARMIN_TOKENSTORE"), + GarminTokenStoreRoot: getEnvDefault("GARMIN_TOKENSTORE", filepath.Join(filepath.Dir(dbPath), ".garmin")), MinConfidence: getEnvFloat("GENIUSRUN_MIN_CONFIDENCE", 0.6), LogLevel: getEnvDefault("GENIUSRUN_LOG_LEVEL", "info"), BackendURL: strings.TrimRight(os.Getenv("GENIUSRUN_BACKEND_URL"), "/"), @@ -84,15 +84,11 @@ func Load() (Config, error) { SessionDuration: getEnvDuration("GENIUSRUN_SESSION_DURATION", 720*time.Hour), } - if cfg.GarminTokenStoreRoot == "" { - cfg.GarminTokenStoreRoot = filepath.Join(filepath.Dir(cfg.DBPath), "garmin-tokenstores") - log.Printf("GARMIN_TOKENSTORE not set, defaulting to %q", cfg.GarminTokenStoreRoot) - } - if cfg.BackendURL == "" { return cfg, fmt.Errorf("GENIUSRUN_BACKEND_URL is required (e.g. https://geniusrun.example.com)") } cfg.FrontendURL = strings.TrimRight(getEnvDefault("GENIUSRUN_FRONTEND_URL", cfg.BackendURL), "/") + if cfg.OIDCIssuerURL == "" { return cfg, fmt.Errorf("GENIUSRUN_OIDC_ISSUER_URL is required") } @@ -102,12 +98,13 @@ func Load() (Config, error) { if cfg.OIDCClientSecret == "" { return cfg, fmt.Errorf("GENIUSRUN_OIDC_CLIENT_SECRET is required") } + cfg.OIDCRedirectURL = cfg.BackendURL + "/api/session/callback" + sessionSecret := os.Getenv("GENIUSRUN_SESSION_SECRET") if len(sessionSecret) < 32 { return cfg, fmt.Errorf("GENIUSRUN_SESSION_SECRET is required and must be at least 32 characters") } cfg.SessionSecret = []byte(sessionSecret) - cfg.OIDCRedirectURL = cfg.BackendURL + "/api/session/callback" cfg.SessionSecure = strings.HasPrefix(cfg.BackendURL, "https://") return cfg, nil diff --git a/backend/internal/config/config_test.go b/backend/internal/config/config_test.go index 2508924..9e37447 100644 --- a/backend/internal/config/config_test.go +++ b/backend/internal/config/config_test.go @@ -88,7 +88,7 @@ func TestLoad_GarminTokenStoreRootDefaultsNextToDBPath(t *testing.T) { if err != nil { t.Fatalf("Load: %v", err) } - want := filepath.Join("/var/lib/geniusrun", "garmin-tokenstores") + want := filepath.Join("/var/lib/geniusrun", ".garmin") if cfg.GarminTokenStoreRoot != want { t.Errorf("GarminTokenStoreRoot = %q, want %q", cfg.GarminTokenStoreRoot, want) } diff --git a/backend/internal/garmin/client.go b/backend/internal/garmin/client.go index 2885a16..4f082d0 100644 --- a/backend/internal/garmin/client.go +++ b/backend/internal/garmin/client.go @@ -63,8 +63,8 @@ type Config struct { PythonPath string GarminEmail string GarminPassword string - // TokenStorePath, if set, is passed as GARMIN_TOKENSTORE so the wrapper - // persists/resumes a Garmin session there instead of the default ~/.garth. + // TokenStorePath is passed as GARMIN_TOKENSTORE so the wrapper + // persists/resumes a Garmin session there. TokenStorePath string } @@ -155,11 +155,9 @@ func (c *subprocessClient) ensureStarted(ctx context.Context) error { extraEnv := []string{ "GARMIN_EMAIL=" + c.cfg.GarminEmail, "GARMIN_PASSWORD=" + c.cfg.GarminPassword, + "GARMIN_TOKENSTORE=" + c.cfg.TokenStorePath, "PYTHONUNBUFFERED=1", } - if c.cfg.TokenStorePath != "" { - extraEnv = append(extraEnv, "GARMIN_TOKENSTORE="+c.cfg.TokenStorePath) - } cmd.Env = append(os.Environ(), extraEnv...) stdin, err := cmd.StdinPipe() @@ -199,7 +197,6 @@ func (c *subprocessClient) ensureStarted(ctx context.Context) error { applog.FromContext(ctx).Info("garmin wrapper spawning", "python_path", pythonPath, - "token_store_configured", c.cfg.TokenStorePath != "", ) return nil } diff --git a/docs/superpowers/plans/2026-07-25-garmin-direct-wrapper-plan.md b/docs/superpowers/plans/2026-07-25-garmin-direct-wrapper-plan.md index 8a9c512..ae76b11 100644 --- a/docs/superpowers/plans/2026-07-25-garmin-direct-wrapper-plan.md +++ b/docs/superpowers/plans/2026-07-25-garmin-direct-wrapper-plan.md @@ -1596,16 +1596,37 @@ Replace the entire `## mcp-garmin integration` section: ```markdown ## mcp-garmin integration -`internal/garmin` is a Go MCP **client** (via `github.com/mark3labs/mcp-go`'s stdio transport) that spawns `mcp-garmin`'s `server.py` as a subprocess. Key things learned the hard way, that would otherwise get rediscovered: +`internal/garmin` is a Go MCP **client** (via `github.com/mark3labs/mcp-go`'s stdio transport) that spawns `mcp-garmin` +'s `server.py` as a subprocess. Key things learned the hard way, that would otherwise get rediscovered: -- **`authenticate()`/`complete_mfa()` return plain strings**, not structured JSON (`"Authenticated successfully."`, `"MFA required. ..."`, `"Authentication failed: ..."`). `internal/garmin/client.go`'s `parseAuthResult` pattern-matches these. -- **The 10s "MFA required" timeout in mcp-garmin's `authenticate()` is a false-positive trap.** A login that's merely slow (e.g. Garmin/Cloudflare rate-limiting) looks identical to a real MFA challenge unless you check whether `prompt_mfa()` was actually invoked (mcp-garmin now logs this to stderr for exactly this reason). -- **mcp-garmin persists Garmin sessions** via a `GARMIN_TOKENSTORE` env var passed to `Garmin.login(tokenstore=...)` — without this, every process start does a full SSO login, which is what trips Garmin's rate limiting under repeated testing. Since geniusrun became multi-tenant, `GARMIN_TOKENSTORE` (`config.GarminTokenStoreRoot`) is a **root directory**, not a single session path: `api.Server.garminFor` namespaces each user's subprocess under `{root}/{userID}` so concurrent users never share a Garmin session. If unset, `config.Load()` now defaults it to a `garmin-tokenstores` directory next to the DB file (logged at startup) rather than leaving per-user namespacing silently skipped. -- **`activityId` is a large int64** — never round-trip it through `float64`/generic `map[string]any` JSON decoding, or it corrupts into scientific notation. Decode into typed structs (`garmin.Activity`, not `map[string]any`). -- **`get_activity_details()` returns raw per-second telemetry** (`activityDetailMetrics` + `metricDescriptors`), not lap/split summaries, despite what its docstring used to say. Its `metricDescriptors` index-to-field mapping is **not stable across activities/devices** — `garmin.ExtractSamples` always resolves fields by descriptor key, never by fixed array position. +- **`authenticate()`/`complete_mfa()` return plain strings**, not structured JSON (`"Authenticated successfully."`, + `"MFA required. ..."`, `"Authentication failed: ..."`). `internal/garmin/client.go`'s `parseAuthResult` + pattern-matches these. +- **The 10s "MFA required" timeout in mcp-garmin's `authenticate()` is a false-positive trap.** A login that's merely + slow (e.g. Garmin/Cloudflare rate-limiting) looks identical to a real MFA challenge unless you check whether + `prompt_mfa()` was actually invoked (mcp-garmin now logs this to stderr for exactly this reason). +- **mcp-garmin persists Garmin sessions** via a `GARMIN_TOKENSTORE` env var passed to `Garmin.login(tokenstore=...)` — + without this, every process start does a full SSO login, which is what trips Garmin's rate limiting under repeated + testing. Since geniusrun became multi-tenant, `GARMIN_TOKENSTORE` (`config.GarminTokenStoreRoot`) is a **root + directory**, not a single session path: `api.Server.garminFor` namespaces each user's subprocess under + `{root}/{userID}` so concurrent users never share a Garmin session. If unset, `config.Load()` now defaults it to a + `.garmin` directory next to the DB file (logged at startup) rather than leaving per-user namespacing silently skipped. +- **`activityId` is a large int64** — never round-trip it through `float64`/generic `map[string]any` JSON decoding, or + it corrupts into scientific notation. Decode into typed structs (`garmin.Activity`, not `map[string]any`). +- **`get_activity_details()` returns raw per-second telemetry** (`activityDetailMetrics` + `metricDescriptors`), not + lap/split summaries, despite what its docstring used to say. Its `metricDescriptors` index-to-field mapping is **not + stable across activities/devices** — `garmin.ExtractSamples` always resolves fields by descriptor key, never by fixed + array position. - **`get_activity_splits()`** returns the actual lap/split summaries (`lapDTOs`). -- **`get_workout_by_id()`** returns a structured Garmin workout's flattened steps (target pace/HR per step); `internal/sync/mapping.go`'s `alignWorkoutTargets` zips these 1:1 against an activity's recorded laps (when the activity carries a `workout_id` and the lap count matches, tolerating exactly one extra trailing lap) to resolve each lap's expected pace/HR band. -- Each user has their own lazily-built, cached `garmin.Client` (`api.Server.garminFor`, double-checked-locked, keyed by `userID`). Saving that user's profile calls `UpdateCredentials` on their own cached client instance, which resets only that client's "started" state and terminates only that user's already-spawned subprocess so the next call respawns fresh under the new credentials — no separate reconnect UI needed beyond the existing connect/MFA flow, and no risk of one user's credential change touching another's session. +- **`get_workout_by_id()`** returns a structured Garmin workout's flattened steps (target pace/HR per step); + `internal/sync/mapping.go`'s `alignWorkoutTargets` zips these 1:1 against an activity's recorded laps (when the + activity carries a `workout_id` and the lap count matches, tolerating exactly one extra trailing lap) to resolve each + lap's expected pace/HR band. +- Each user has their own lazily-built, cached `garmin.Client` (`api.Server.garminFor`, double-checked-locked, keyed by + `userID`). Saving that user's profile calls `UpdateCredentials` on their own cached client instance, which resets only + that client's "started" state and terminates only that user's already-spawned subprocess so the next call respawns + fresh under the new credentials — no separate reconnect UI needed beyond the existing connect/MFA flow, and no risk of + one user's credential change touching another's session. ``` with: @@ -1613,16 +1634,40 @@ with: ```markdown ## Garmin integration (direct wrapper, no MCP) -`internal/garmin` spawns a small Python wrapper script (`internal/garmin/pyscript/wrapper.py`, embedded into the Go binary via `go:embed` — see `docs/superpowers/specs/2026-07-25-garmin-direct-wrapper-design.md`) that imports `garminconnect` directly and speaks a minimal newline-delimited-JSON protocol over stdin/stdout: `{"id","cmd","params"}` in, `{"id","result"}` or `{"id","error"}` out. `cmd` is `authenticate`, `complete_mfa`, or a fully generic `call` (`{"method": "", "args": {...}}`) — there's no MCP layer, and the separate `mcp-garmin` repo this used to depend on is retired. Key things learned the hard way, that would otherwise get rediscovered: +`internal/garmin` spawns a small Python wrapper script (`internal/garmin/pyscript/wrapper.py`, embedded into the Go +binary via `go:embed` — see `docs/superpowers/specs/2026-07-25-garmin-direct-wrapper-design.md`) that imports +`garminconnect` directly and speaks a minimal newline-delimited-JSON protocol over stdin/stdout: `{"id","cmd","params"}` +in, `{"id","result"}` or `{"id","error"}` out. `cmd` is `authenticate`, `complete_mfa`, or a fully generic `call` +(`{"method": "", "args": {...}}`) — there's no MCP layer, and the separate `mcp-garmin` +repo this used to depend on is retired. Key things learned the hard way, that would otherwise get rediscovered: -- **`authenticate`/`complete_mfa` return structured JSON** (`{"status": "success"|"mfa_required"|"failed", "message": "..."}`), not plain strings — `internal/garmin/client.go` maps `status` directly to `AuthStatus`, no string pattern-matching. -- **The 10s "MFA required" timeout in `wrapper.py`'s `authenticate` handler is a false-positive trap.** A login that's merely slow (e.g. Garmin/Cloudflare rate-limiting) looks identical to a real MFA challenge unless you check whether `prompt_mfa()` was actually invoked (`wrapper.py` logs this to stderr for exactly this reason). -- **`wrapper.py` persists Garmin sessions** via a `GARMIN_TOKENSTORE` env var passed to `Garmin.login(tokenstore=...)` — without this, every process start does a full SSO login, which is what trips Garmin's rate limiting under repeated testing. Since geniusrun is multi-tenant, `GARMIN_TOKENSTORE` (`config.GarminTokenStoreRoot`) is a **root directory**, not a single session path: `api.Server.garminFor` namespaces each user's subprocess under `{root}/{userID}` so concurrent users never share a Garmin session. If unset, `config.Load()` defaults it to a `garmin-tokenstores` directory next to the DB file (logged at startup) rather than leaving per-user namespacing silently skipped. -- **`activityId` is a large int64** — never round-trip it through `float64`/generic `map[string]any` JSON decoding, or it corrupts into scientific notation. Decode into typed structs (`garmin.Activity`, not `map[string]any`). -- **`get_activity_details` returns raw per-second telemetry** (`activityDetailMetrics` + `metricDescriptors`), not lap/split summaries. Its `metricDescriptors` index-to-field mapping is **not stable across activities/devices** — `garmin.ExtractSamples` always resolves fields by descriptor key, never by fixed array position. +- **`authenticate`/`complete_mfa` return structured JSON** + (`{"status": "success"|"mfa_required"|"failed", "message": "..."}`), not plain strings — `internal/garmin/client.go` + maps `status` directly to `AuthStatus`, no string pattern-matching. +- **The 10s "MFA required" timeout in `wrapper.py`'s `authenticate` handler is a false-positive trap.** A login that's + merely slow (e.g. Garmin/Cloudflare rate-limiting) looks identical to a real MFA challenge unless you check whether + `prompt_mfa()` was actually invoked (`wrapper.py` logs this to stderr for exactly this reason). +- **`wrapper.py` persists Garmin sessions** via a `GARMIN_TOKENSTORE` env var passed to `Garmin.login(tokenstore=...)` — + without this, every process start does a full SSO login, which is what trips Garmin's rate limiting under repeated + testing. Since geniusrun is multi-tenant, `GARMIN_TOKENSTORE` (`config.GarminTokenStoreRoot`) is a **root directory**, + not a single session path: `api.Server.garminFor` namespaces each user's subprocess under `{root}/{userID}` so + concurrent users never share a Garmin session. If unset, `config.Load()` defaults it to a `.garmin` directory next to + the DB file (logged at startup) rather than leaving per-user namespacing silently skipped. +- **`activityId` is a large int64** — never round-trip it through `float64`/generic `map[string]any` JSON decoding, or + it corrupts into scientific notation. Decode into typed structs (`garmin.Activity`, not `map[string]any`). +- **`get_activity_details` returns raw per-second telemetry** (`activityDetailMetrics` + `metricDescriptors`), not + lap/split summaries. Its `metricDescriptors` index-to-field mapping is **not stable across activities/devices** — + `garmin.ExtractSamples` always resolves fields by descriptor key, never by fixed array position. - **`get_activity_splits`** returns the actual lap/split summaries (`lapDTOs`). -- **`get_workout_by_id`** returns a structured Garmin workout's flattened steps (target pace/HR per step); `internal/sync/mapping.go`'s `alignWorkoutTargets` zips these 1:1 against an activity's recorded laps (when the activity carries a `workout_id` and the lap count matches, tolerating exactly one extra trailing lap) to resolve each lap's expected pace/HR band. -- Each user has their own lazily-built, cached `garmin.Client` (`api.Server.garminFor`, double-checked-locked, keyed by `userID`). Saving that user's profile calls `UpdateCredentials` on their own cached client instance, which resets only that client's "started" state and terminates only that user's already-spawned subprocess so the next call respawns fresh under the new credentials — no separate reconnect UI needed beyond the existing connect/MFA flow, and no risk of one user's credential change touching another's session. +- **`get_workout_by_id`** returns a structured Garmin workout's flattened steps (target pace/HR per step); + `internal/sync/mapping.go`'s `alignWorkoutTargets` zips these 1:1 against an activity's recorded laps (when the + activity carries a `workout_id` and the lap count matches, tolerating exactly one extra trailing lap) to resolve each + lap's expected pace/HR band. +- Each user has their own lazily-built, cached `garmin.Client` (`api.Server.garminFor`, double-checked-locked, keyed by + `userID`). Saving that user's profile calls `UpdateCredentials` on their own cached client instance, which resets only + that client's "started" state and terminates only that user's already-spawned subprocess so the next call respawns + fresh under the new credentials — no separate reconnect UI needed beyond the existing connect/MFA flow, and no risk of + one user's credential change touching another's session. ``` - [ ] **Step 5: Verify the whole backend builds**