Every log record now carries type ('http' for the request middleware
line, 'wrapper' for garmin execute() calls and forwarded wrapper.py
stderr, 'app' for everything else), class (Go type with package, or the
package/module for free functions), and method (the emitting Go/Python
function -- wrapper.py stamps it automatically, so its msg prefixes are
gone). msg is optional and omitted when empty; the redundant messages
('HTTP request', 'wrapper call', 'garmin wrapper stderr') are dropped,
the HTTP verb moves to http_method, source=garmin-wrapper is replaced by
type=wrapper, and the request_id middleware plumbing (applog
WithLogger/FromContext) is removed. applog.App(class, method) is the
tagging helper for app records.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The background login thread now flips _auth_state to authenticated
itself when a slow tokenstore resume succeeds after the 10s wait --
previously its result landed in an abandoned private queue and the
docstring's promised late recovery never happened, leaving every
subsequent call failing until an explicit authenticate. Guarded to only
ever transition from unauthenticated, so an explicit authenticate/MFA
flow that took over meanwhile is never clobbered; the timeout branch no
longer re-writes the state (a redundant write that could race a success
at the boundary). Structured-log messages reworked alongside.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All registry keys must exist as rows in the config table: main seeds
missing keys with their defaults at startup, LoadApp fails fast on a
missing key, and the code-side fallback const/helper for the onboarding
setup timeout is gone -- the value rides in SessionConfig.SetupTimeout.
The key is renamed session.idle_timeout -> session.setup_timeout, and
the /config page's 'overridden' now means 'differs from the default'.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
session.idle_timeout (minutes, default 15) joins the app-config registry
and drives the onboarding Garmin session eviction, distinct from
session.duration (the login cookie lifetime in hours). The raw Keycloak
ID token no longer rides in auth.Claims through every request context:
it's minted into the session cookie separately and read back only by the
logout handler via IDTokenFromSessionCookie. OnboardingWizard uses the
type-imported FormEvent<HTMLFormElement> instead of the React.FormEvent
namespace alias.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Error responses from wrapper.py now carry error_type and traceback in
the protocol itself, logged as attrs on the Go side's per-call record.
wrapper.py's free-text stderr debug prints become JSON log lines
({level,msg,...attrs}); client.go parses that stream and re-emits it
through the application logger (level-mapped, source=garmin-wrapper),
wrapping any non-JSON line instead of passing it through raw. The
wrapper also survives malformed request lines and unserializable
results with JSON responses instead of crashing with a raw traceback.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every log.Printf/Fatalf becomes a structured slog call through
internal/log: request handlers use the request-scoped logger
(applog.FromContext, carrying request_id), startup failures exit via a
fatal() helper that still emits JSON, and the seedsample/dumpschema CLIs
bootstrap the same JSON logger. Stale client_test expectations aligned
with the refactored wrapper-call logging (message casing, result attr,
ERROR level).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
users.display_name becomes users.name and is now the only human-facing
name -- profiles.name is dropped (it duplicated the account name; the
Profile page's Name field now edits users.name through PUT /api/profile,
which carries Name alongside the profiles columns). The profile table
becomes profiles and laps becomes activity_laps, homogeneous with
activity_samples. Onboarding pre-fills the display name from the OIDC
claim's name.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Garmin auth/sync routes move under /api/garmin/*; sync.Service becomes
garmin.Sync with garmin.SyncConfig/ClientConfig; applog becomes
internal/log; the test mock moves into the garmin package as MockClient
(breaking the test-only import cycle the merge created); stale test
URLs and type names updated to match.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Removes the unused GET /api/activities list/detail endpoints, moves the
review-queue list and resolve/unlock/unassign actions under
/api/activities, renames resolve to assign end-to-end, and drops the
now-unused store.ReviewQueue helper.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SetActivityWorkoutNotFound is a per-user-scoped store method but never got
the adversarial cross-user coverage CLAUDE.md's testing conventions require
for such features. Extend the existing isolation test with a mismatched-user
call (must no-op) and a correct-user call (must exclude the activity from
that user's own ActivitiesMissingWorkout/count without touching the other
user's pending activity).
fillPendingWorkouts previously treated every get_workout_by_id
failure identically -- log it, leave workout_raw_json null, retry
next sync -- which is correct for a transient failure but means a
definitive 404 (the workout deleted on Garmin's side after being
linked to an activity) got silently retried forever, with the only
symptom being an unexplained, permanent "1 more workout pending"
nudge. Now checks errors.Is(err, garmin.ErrNotFound) and, only for
that case, calls SetActivityWorkoutNotFound instead of retrying.
A confirmed-404 workout must stop being retried forever, but
workout_raw_json should never be fabricated -- it stays null exactly
as it does for "not yet fetched." workout_not_found_at is the
separate marker ActivitiesMissingWorkout/CountActivitiesMissingWorkout
now check for, mirroring the existing pattern of the other _fetched_at
columns. UpsertActivity's ON CONFLICT clause already never touches
these columns, so the marker persists across every later re-sync.
roundTrip now wraps the returned error with the new ErrNotFound
sentinel whenever the wrapper's response set not_found (Task 1),
letting any Client method's caller distinguish a definitive 404 from
a transient failure via errors.Is, regardless of which garminconnect
method was called.
garminconnect's own GarminConnectNotFoundError already exists
specifically for this (its docstring: "so callers can now catch a
missing resource specifically, e.g. deleting an already-deleted
workout"), raised by connectapi() for any real HTTP 404. dispatch()
now surfaces that distinction as an extra not_found: true field
alongside the existing error string, so internal/garmin/client.go
(next commit) can tell a definitive 404 apart from a transient
failure.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Corrupting the true last character of a JWT's base64url-encoded
HMAC-SHA256 signature is unreliable: that character encodes only 4
real bits plus 2 unused padding bits, and Go's encoding/base64
ignores those padding bits by default -- about 1 in 4 replacement
characters decode to byte-identical signature bytes, so the
"tampered" cookie still verifies and the test spuriously passes.
Corrupting the second-to-last character instead is deterministic,
since HMAC-SHA256's fixed 32-byte digest length means that position
is always fully significant.
Confirmed via 20 repeated runs (previously ~1/3 failure rate).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestFillPendingDetails_ReportsPhaseAwareProgressThroughActivitiesAndWorkoutsPhases
only ever called FillPendingDetails directly, never exercising
FullSync's own setProgress(PhaseDiscovering, ...) / deferred
setProgress(PhaseIdle, ...) wiring around backfillCore/
incrementalSyncCore -- what handleSyncRun actually runs in production
and what SyncModal's "Discovering activities..." spinner depends on.
Add a mock.Client.Delay field (slept, ctx-cancellable, at the start of
GetActivities) so a test can give the discovering phase real
wall-clock duration, then add
TestFullSync_ReportsPhaseTransitionsThroughDiscoveringToIdle, which
runs FullSync in a goroutine and polls Progress() to confirm it
observes PhaseDiscovering mid-flight and PhaseIdle after completion.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every run recorded today is SyncKindFull (the single-pass "Sync now"
action) -- "backfill"/"incremental" only ever appear in historical
rows predating that consolidation.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow this file's existing pattern (two real provisioned users,
identical-shaped rows, assert on empty-result/unaffected-by rather
than just two independently-created rows not colliding) for the two
methods touched by the details_fetched_at gating fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fillPendingDetails and fillPendingWorkouts are each independently
LIMIT-bounded over different candidate sets, so on a large first
backfill a structured-workout activity could fall inside the workouts
pass's window while still outside the details pass's window.
fillActivityWorkout would then align workout targets against zero laps
(details/laps never written yet) and unconditionally mark
workout_raw_json non-NULL, permanently losing that activity's target
pace/HR bands once its real laps arrived later -- silently, with no
error.
Add details_fetched_at IS NOT NULL to ActivitiesMissingWorkout's and
CountActivitiesMissingWorkout's WHERE clause: an activity is only
eligible for a workout fetch once its laps actually exist to align
against. This still covers the intended retry case (an activity whose
workout fetch previously failed always has details_fetched_at already
set) while excluding never-yet-processed activities.
Rename/rewrite the store test to assert the corrected exclusion
(count=1, not 2) as an explicit regression test, and fix the
TestSyncStatus_ReportsPhaseAwareProgressAndWorkoutsPending API fixture,
which exercised the same buggy shape.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces detail_fill_progress ({Done, Total}) with progress ({Phase,
Done, Total}), and adds workouts_pending (mirroring
activities_pending_details) from the new CountActivitiesMissingWorkout.
Frontend types updated to match -- GarminConnection.tsx is intentionally
left broken by this commit alone; it's fixed in the next commit that
adds SyncModal.
Progress becomes phase-aware ({Phase, Done, Total} instead of a flat
{Done, Total}), with FullSync reporting an indeterminate "discovering"
phase during backfill/incremental sync (there's no meaningful total
before those calls have already happened), then FillPendingDetails
reporting a real Done/Total for its activities pass, then its new,
independent workouts pass.
alignWorkoutTargets now takes a lap count instead of a []garmin.Lap
slice, since it never used lap content, only length -- this lets the
new workouts pass call it against laps read back from the DB rather
than needing the original Garmin lap data again.
Splitting the passes also fixes a latent bug: an activity whose
details were fetched successfully but whose workout fetch failed in
that same run previously had no way to ever retry the workout fetch,
since ActivitiesMissingDetails stops returning it once
details_fetched_at/splits_fetched_at are set. ActivitiesMissingWorkout
queries workout_id/workout_raw_json independently, so it keeps
surfacing that activity until its workout is actually fetched.
Mirrors ActivitiesMissingDetails/CountActivitiesMissingDetails, but
queries workout_id IS NOT NULL AND workout_raw_json IS NULL --
independent of whether details/splits were ever fetched, since
workout_id is known at initial upsert time. This also fixes a latent
gap: an activity whose details were fetched successfully but whose
workout fetch failed in the same run previously had no way to ever be
retried, since ActivitiesMissingDetails stops returning it the moment
details_fetched_at/splits_fetched_at are set.
Both were only reachable via the periodic background sync loop removed
in 4d2cbe4 -- nothing in production calls them anymore, only tests did.
FullSync already calls backfillCore/incrementalSyncCore directly.
Rewrite the affected tests to call the *Core functions (still exported
within the package) instead, dropping the now-redundant standalone
SyncRun-recording assertion covered by TestFullSync_RecordsOneCombinedSyncRun.
There's no reason the Garmin token-store root and the SQLite database
file need to live near each other -- they're independent env vars
(GARMIN_TOKENSTORE, GENIUSRUN_DB_PATH) and should stay independently
configurable, including in their defaults. Revert the "next to DBPath"
default introduced in 77f9588 back to a plain ".garmin" relative to
the working directory, so pointing GENIUSRUN_DB_PATH elsewhere never
silently drags the token-store default along with it.
_startup_login() used to run unconditionally at process boot, before
main()'s stdin dispatch loop started. Whenever the very first real
command turned out to be an explicit authenticate, this was actively
counterproductive: on success it duplicated a login authenticate was
about to redo anyway, and on failure it was a wasted, unauthenticated
hit against Garmin's servers moments before the real attempt --
exactly the kind of extra load that worsens rate-limiting risk. It
also never handled MFA, so it couldn't stand in for authenticate
regardless.
Make it lazy instead: only _handle_call falls back to it, at most once
per subprocess lifetime, and only if authenticate was never explicitly
attempted first. This is what it was actually for -- silently resuming
a cached tokenstore session for a data call that never goes through
the explicit authenticate command (e.g. "Sync now" reaching an
already-connected user's client right after a backend restart cleared
the in-memory client cache).
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.
handleSetupComplete reused the onboarding client object as-is in the
permanent per-user cache, but its garmin.Config.TokenStorePath was fixed
at construction to the ephemeral setup/{hash} directory and never
corrected after that directory was renamed to the permanent {userID}
path. The next respawn of that same client (e.g. any Profile-page save,
which unconditionally calls UpdateCredentials) wrote a fresh token file
back under setup/{hash}, forcing a real re-login/MFA on the next Garmin
connect even though a valid session already existed under {userID}.
Close the ephemeral client instead and let the next garminFor call build
a fresh one against the correct, already-renamed directory.
Garmin's rate-limiting means every unattended sync attempt risks a
ban; syncing should only ever happen when explicitly triggered via
"Sync now" (POST /api/sync/run), never on an unattended timer.
Removes the periodic background loop (main.go's runIncrementalSyncLoop,
api.Server.RunIncrementalSyncForAllUsers), its GENIUSRUN_INCREMENTAL_SYNC_EVERY
config, and store.DB.ListUsers (which existed solely to feed it). The
manual "Sync now" flow (Backfill/IncrementalSync/FillPendingDetails via
FullSync) is untouched.
Carries the raw ID token in the session cookie so logout can hand it back
to Keycloak as id_token_hint, letting it skip its own logout-confirmation
prompt -- otherwise a user could cancel out of it and land back in the app
with a Keycloak SSO session but no geniusrun profile (e.g. right after
deleting their account).
Same class of bug as the OIDC callback fix: handleSessionLogout redirected
Keycloak's end-session flow back to BackendURL+"/", which 404s in a
split-origin deployment (the backend serves no "/" route). Flagged as a
known-deferred question in the callback-redirect design spec; fixing it
now that it's been hit in practice.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Now that GENIUSRUN_FRONTEND_URL exists as a separate config value, keeping
the backend's own origin named "PublicBaseURL" invited exactly the kind of
mixup that caused the OIDC callback 404 in the first place. Renamed
consistently: env var, Config.BackendURL, api.SessionConfig.BackendURL.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
handleSessionCallback's redirects (success and all 4 failure branches)
were relative paths, which resolve against the backend's own origin --
broken in this project's own supported split-origin local dev setup,
since the Go backend serves no "/" route at all. Now uses the new
config.Config.FrontendURL (defaults to PublicBaseURL, so no change for
single-origin production deployments).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Lets the OIDC callback redirect to the frontend's real origin instead of
a relative path resolved against the backend's own origin -- needed for
this project's own supported split-origin local dev setup (frontend on
Vite, backend on geniusrund, bridged by CORS).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_startup_login's background thread and _handle_authenticate's shared the
module-level _login_result_queue with no correlation. Since _startup_login
can now time out at 10s while its thread keeps running (from the previous
fix in this wave), a slow-cold-starting subprocess's first explicit
"Connect to Garmin" call could accidentally dequeue the startup thread's
stale result instead of its own fresh one, orphaning the loser's result to
corrupt a later authenticate/complete_mfa call.
_handle_authenticate and _handle_complete_mfa still correctly share
_login_result_queue -- they're two halves of one explicit, MFA-capable
login flow. _startup_login is a background tokenstore resume with no MFA
involved, so it now uses its own private, function-local queue.Queue()
instead, making cross-contamination structurally impossible.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_startup_login() ran garminconnect's login synchronously with no timeout
before main() ever started reading stdin, so a slow/rate-limited Garmin
login could wedge a user's whole subprocess before it became responsive.
Give it the same background-thread + bounded-10s-timeout shape
_handle_authenticate already uses, with tests for both the fast-success
and timeout paths.
Also refreshes .claude/skills/geniusrun-dev/SKILL.md (still describing
the retired mcp-garmin MCP architecture) and three stale doc comments
(garmin.AuthStatus, config.GarminTokenStoreRoot, mock package doc) left
over from the direct-wrapper migration.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Removes the last references to garmin.Config.ServerPath and the
MCP_GARMIN_* env vars now that the wrapper script is embedded in the
binary; updates CLAUDE.md's mcp-garmin section to describe the direct
wrapper instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The wrapper script is embedded in the binary now (internal/garmin), so
there's no script path left to configure. The interpreter path becomes
optional, defaulting to python3 on PATH, matching how other optional
plumbing (e.g. GENIUSRUN_OIDC_REQUIRED_ROLE) is already handled.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GetActivities/GetActivitySplits/GetActivityDetails/GetWorkoutByID now send
{"cmd":"call","params":{"method":...,"args":...}} instead of named MCP
tools. subprocessClient fully implements Client.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
