feat: add Moonshot/Kimi and DeepSeek providers (balance-based)#68
feat: add Moonshot/Kimi and DeepSeek providers (balance-based)#68papajade55-debug wants to merge 1 commit intoonllm-dev:mainfrom
Conversation
Mirrors the OpenRouter pattern to track two pay-as-you-go providers whose quotas are exposed as a remaining balance (not cumulative usage). Endpoints: - Moonshot: GET https://api.moonshot.ai/v1/users/me/balance (CNY) - DeepSeek: GET https://api.deepseek.com/user/balance (USD/CNY) Semantic inversion vs OpenRouter: - OpenRouter tracks cumulative usage (grows, resets monthly) -> reset on 50% drop - Moonshot/DeepSeek track balance (decreases on spend, grows on recharge) -> reset on >=50% growth (recharge); TotalDelta cumulates balance drops Files added (mirroring openrouter pattern): - internal/api/{moonshot,deepseek}_{client,types}.go (+tests) - internal/agent/{moonshot,deepseek}_agent.go - internal/store/{moonshot,deepseek}_store.go (+tests, with currency col on deepseek) - internal/tracker/{moonshot,deepseek}_tracker.go (+tests) - internal/web/{moonshot,deepseek}_handlers.go Files modified: - internal/config/config.go: MOONSHOT_API_KEY, DEEPSEEK_API_KEY env vars - internal/store/store.go: schema migrations - internal/web/{handlers,static/app.js,templates/dashboard.html}: dashboard tabs - internal/metrics/metrics.go: onwatch_credits_balance{unit="cny_*"|"usd_*"} gauges - main.go: agent registration Tested live on Linux Mint 22.3 / Cinnamon 6.6.7 / Go 1.25.7: - Moonshot balance: ¥19.47 detected - DeepSeek balance: $34.69 detected - 1224 unit tests pass (api/store/tracker packages) Note: Moonshot endpoint set to api.moonshot.ai (international). Users on the Chinese moonshot.cn endpoint may need to patch the baseURL or expose it via env (suggested follow-up). Co-Authored-By: Jules <noreply@jules.google> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
prakersh
left a comment
There was a problem hiding this comment.
Review summary
Thanks for the substantial work - structural mirroring of the OpenRouter pattern is mostly faithful. That said, I went through every layer carefully and have a number of issues I'd want addressed before this lands. Grouped by severity with file:line references throughout.
Blockers
B1. Reset log emits stale field that always equals new balance - internal/tracker/moonshot_tracker.go:111-116, internal/tracker/deepseek_tracker.go:114-120. t.lastBalance = currentBalance runs before the Info log, so the "lastBalance" log field always equals newBalance. Capture into a local before reassignment.
B2. Notifier wired but never invoked - internal/agent/moonshot_agent.go, internal/agent/deepseek_agent.go. Both agents declare notifier *notify.NotificationEngine and expose SetNotifier(...), but neither poll() ever calls a.notifier.Check(...). Compare internal/agent/openrouter_agent.go:114-125. Configuring notification thresholds for these providers is currently a silent no-op. Either dispatch a notify.QuotaStatus (note: balance providers need the threshold inverted, e.g. low-balance triggers alert) or remove the dead SetNotifier/notifier plumbing.
B3. Custom errorsIs test helper bypasses errors.Is and has a latent panic - internal/api/moonshot_client_test.go:106-111 (also referenced from internal/api/deepseek_client_test.go). Three problems: (1) every other *_client_test.go in internal/api/ uses stdlib errors.Is; both new clients wrap with fmt.Errorf("%w: ...", ErrX, ...) so it works directly; (2) err.Error()[:len(target.Error())] panics if len(err.Error()) < len(target.Error()) - the empty-string guard does not protect against this; (3) prefix matching can produce false positives across unrelated wrapped errors. Fix: delete errorsIs, use errors.Is(err, tt.wantErr) in both files.
High
H4. Reset detection has no absolute-delta floor - internal/tracker/moonshot_tracker.go:91-95, internal/tracker/deepseek_tracker.go:94-98. The currentBalance >= t.lastBalance*1.5 condition fires on 0.05 -> 0.10, closing the cycle and discarding TotalDelta. Voucher fragments, refunds, or rounding near zero will flip cycles. Same risk on the cold-zero branch. Add an absolute floor (e.g. currentBalance - t.lastBalance >= minRechargeAbs) combined with the ratio check.
H5. PeakUsage stores peak balance but is rendered as "peak requests" in the dashboard - tracker writes cycle.PeakUsage = currentBalance (i.e. highest balance seen, essentially post-recharge starting balance) at moonshot_tracker.go:130-133 / deepseek_tracker.go:134-136. Handlers ship that value as "peakRequests" (moonshot_handlers.go:162, deepseek_handlers.go:173) and app.js:7158 renders it in the per-cycle modal table. UsageSummary already correctly uses cycle.TotalDelta for summary.PeakCycle (moonshot_tracker.go:177-179), so the persisted PeakUsage is dead in the summary path while being live and misleading in the cycle table. Either rename to StartingBalance end-to-end, or write running max of cycle.TotalDelta to PeakUsage (matching OpenRouter semantic).
H6. DeepSeek is_available=false is silently dropped, "exhausted state in cycle metadata" is not implemented - internal/agent/deepseek_agent.go:91-95 returns before InsertDeepSeekSnapshot, so the is_available column on deepseek_snapshots (store.go:535) only ever stores 1. The dashboard cannot distinguish "down" from "no recent polls". The PR description's claim that exhaustion is "marked as exhausted state in cycle metadata" is unsupported - deepseek_reset_cycles (store.go:541-549) has no status/is_exhausted column; deepseek_handlers.go:45 derives status="exhausted" purely at read-time from latest.TotalBalance == 0. Either insert the snapshot with is_available=0 and let downstream filter, or drop the column. PR description should be amended to match what was actually built.
H7. UsageSummary("USD") returns incoherent payload when latest snapshot is CNY - internal/tracker/deepseek_tracker.go:191-209. activeCycle and history are filtered by currency (lines 159, 164), but QueryLatestDeepSeek() (deepseek_store.go:53) ignores currency. When user requests USD but last poll was CNY, function returns CurrentBalance=0, CurrentRate=0 alongside non-zero CompletedCycles/TotalTracked. Add QueryLatestDeepSeekByCurrency(currency), or fail closed and return an empty summary when no per-currency snapshot exists.
H8. scrapeDeepSeek keeps emitting last good balance forever when service is down - internal/metrics/metrics.go:610-642. No guard on snap.IsAvailable. Combined with H6 (agent skips inserting on unavailable), QueryLatestDeepSeek returns the previous available snapshot indefinitely, so Prometheus keeps publishing the last known good creditsBalance for an account that may have been suspended hours/days ago. Add if !snap.IsAvailable { return }, or have the agent insert with IsAvailable=false and have the scraper short-circuit. Optionally expose a provider_unavailable{provider="deepseek"} gauge.
H9. Moonshot client never validates API code field - internal/api/moonshot_types.go:16-19, internal/api/moonshot_client.go:78-151. Code is unmarshalled but never checked. Compare internal/api/zai_client.go:127-134 which explicitly handles "HTTP 200 with error code in body". If Moonshot returns {"code": 401, "data": {zero balances}} with HTTP 200 (a common Chinese-cloud envelope pattern), ToSnapshot blindly persists a zero-balance snapshot and triggers spurious threshold alerts. After ParseMoonshotResponse, return an error when Code != 0.
H10. New SQLite tables ship without indexes - internal/store/store.go:517-549. The four new tables have zero CREATE INDEX statements. Every other provider has matching ones (e.g. idx_anthropic_snapshots_captured at line 369, idx_openrouter_snapshots_captured at line 659, idx_antigravity_cycles_model_active_unique at line 557). QueryLatestDeepSeek/QueryLatestMoonshot (ORDER BY captured_at DESC LIMIT 1) become full-table scans on every metrics scrape and dashboard render; CloseDeepSeekCycle/QueryActiveDeepSeekCycle scan the cycles table on every poll. Please add: idx_moonshot_snapshots_captured(captured_at), idx_moonshot_cycles_type_active(quota_type) WHERE cycle_end IS NULL, idx_deepseek_snapshots_captured(captured_at), idx_deepseek_cycles_type_currency_active(quota_type, currency) WHERE cycle_end IS NULL.
H11. Currency label in metrics has unbounded cardinality on format drift - internal/metrics/metrics.go:621-626. The fallback unitPrefix = snap.Currency uses the upstream string verbatim with no normalization. Today DeepSeek returns only CNY/USD, so this is defensive rather than an active bug, but "CNY " (trailing space), "cny", or a new currency would each spawn 3 distinct Prometheus series (_total, _granted, _topped_up), violating bounded-cardinality discipline. Suggest strings.ToLower(strings.TrimSpace(snap.Currency)) plus a {cny, usd} allowlist that falls back to "unknown".
H12. DeepSeek dangling open cycle on currency switch - internal/store/deepseek_store.go:148. CloseDeepSeekCycle filters by (quota_type, currency). If upstream switches a user's currency between cycles (CNY -> USD), the close pass for the new cycle never matches the old open (balance, CNY) row, leaving a dangling open cycle. Either close the prior currency's cycle explicitly when currency changes, or add CREATE UNIQUE INDEX ... ON deepseek_reset_cycles(quota_type) WHERE cycle_end IS NULL to force closure-before-open across currencies.
H13. ParseFloat errors silently zero balance fields - internal/api/deepseek_types.go:55-63. Empty/null/malformed strings cause balance fields to keep their zero value, indistinguishable from a real zero. The tracker then sees lastBalance -> 0, adds the entire prior balance to TotalDelta, and on the next valid poll triggers a phantom reset (via the lastBalance == 0 branch). Suggest logging at warn and either returning an error from ToSnapshot or skipping the snapshot insert in the agent (mirroring the IsAvailable=false skip).
Medium
M14. Tab labels for moonshot/deepseek fall through to lowercase rendering - internal/web/templates/dashboard.html:15. The provider tab template enumerates labels for nine existing providers but not openrouter, moonshot, deepseek - they all hit the {{else}}{{.}}{{end}} fallback. Pre-existing for openrouter, perpetuated by this PR. Add {{else if eq . "moonshot"}}Moonshot{{else if eq . "deepseek"}}DeepSeek{{else if eq . "openrouter"}}OpenRouter.
M15. No tests for new web handlers - no moonshot_handlers_test.go or deepseek_handlers_test.go despite established precedent (gemini_handlers_test.go, cursor_handlers_test.go, minimax_handlers_test.go, api_integrations_handlers_test.go). CLAUDE.md mandates "TDD-first". Please cover at least current, history (range/downsample), summary, cycleOverview.
M16. No tests for new metrics scrape paths - internal/metrics/metrics_test.go has zero assertions for scrapeMoonshot/scrapeDeepSeek or the new cny_*/usd_* units. A smoke test asserting metric labels and stale-threshold behaviour would catch regressions of H8/H11.
M17. Tracker tests cover only one happy path - moonshot_tracker_test.go, deepseek_tracker_test.go each exercise exactly one trajectory (100 -> 80 -> 200). Reset detection has at least four branches; please add table-driven cases for sub-50% growth (no-reset), cold-zero with tiny positive balance (intent unclear given H4), and successive recharges within the same logical period.
M18. Query{Moonshot,DeepSeek}CycleHistory are unbounded by default - internal/store/moonshot_store.go:187, internal/store/deepseek_store.go:201, internal/tracker/moonshot_tracker.go:160, internal/tracker/deepseek_tracker.go:164. Variadic limit ...int is omitted at tracker call sites, so UsageSummary reads the entire history table on every call. CLAUDE.md guardrail: "cycles ≤ 200". Pass 200 explicitly or change to required limit int.
M19. First poll after restart silently skips delta accumulation - internal/tracker/moonshot_tracker.go:137-145 (and DeepSeek analog). When the agent restarts with an active cycle, processBalance enters the else { /* First snapshot after restart */ } branch and skips delta accumulation with no log. Fundamental compromise (no in-memory baseline), not a bug, but invisible to operators - one Info log line ("first poll after restart, gap not measurable") would make it observable.
M20. Major handler duplication (793 LOC across two near-identical files) - internal/web/moonshot_handlers.go (374) + internal/web/deepseek_handlers.go (419). Eight mirror methods each, plus 12+ duplicate if h.config.HasProvider(...) blocks in handlers.go. Not a blocker - but a future balance-style provider means another ~400 LOC of mirror. A small generic balance-handler helper would pay back quickly.
M21. DeepSeek summary accepts arbitrary currency query param without validation - internal/web/deepseek_handlers.go:185-191, 245-252, 321-332. Forwarded to store queries (parameterized, so no injection) and reflected in JSON response. Unknown values silently produce blank currencySymbol and dataless responses. Whitelist to {CNY, USD} with CNY fallback - matches the symbol pick logic at lines 277-282.
Notes / nits
- N1.
internal/store/store.go:519, 527-528, 534, 545-546useDATETIMEwhile every other provider table usesTEXT NOT NULL. Functionally inert (RFC3339Nano strings stored unchanged) but worth aligning. - N2.
redactMoonshotAPIKey/redactDeepSeekAPIKeyare byte-for-byte duplicates ofredactOpenRouterAPIKey. Six near-identical redaction functions ininternal/api/- candidate for a shared helper. - N3. Hard-coded
User-Agent: onwatch/1.0propagated from OpenRouter; repo is at v2.11.43 - likely worth reading fromVERSIONin a future cleanup. - N4.
internal/web/{moonshot,deepseek}_handlers.go:42-44use exact float equality (== 0) to flag exhausted state. In practice servers return discrete cents, but a small epsilon would be more defensive.
Summary
Solid structural work overall. I'd love to see the three blockers addressed (especially B2: dead-notifier wiring is the kind of silent issue that bites in production), the H-tier correctness items (H4, H5, H6, H7, H8, H13 in particular) tightened up, and the missing indexes (H10) added before this lands. Mediums are mostly polish but the test coverage gaps (M15, M16, M17) and the tab-label fallthrough (M14) would round it out nicely.
Happy to elaborate on any of these or pair on specific fixes.
Summary
Adds two pay-as-you-go providers to onWatch by mirroring the existing OpenRouter pattern:
GET https://api.moonshot.ai/v1/users/me/balance(CNY)GET https://api.deepseek.com/user/balance(USD/CNY)Both expose a remaining-balance (not cumulative usage) → semantic inversion of the reset detection: a recharge (≥50% balance growth) is treated as cycle reset;
TotalDeltacumulates balance drops (money spent).What's new
internal/api/moonshot_{client,types}.go+deepseek_{client,types}.go(+tests)internal/agent/moonshot_agent.go,deepseek_agent.gointernal/store/moonshot_store.go,deepseek_store.go(withcurrencycolumn for DeepSeek; +tests)internal/tracker/moonshot_tracker.go,deepseek_tracker.go(+tests)internal/web/moonshot_handlers.go,deepseek_handlers.goWhat's modified
internal/config/config.go—MOONSHOT_API_KEY,DEEPSEEK_API_KEYenv wiring + extendsAvailableProviders/HasProviderinternal/store/store.go— table migrationsinternal/web/{handlers.go, static/app.js, templates/dashboard.html}— Material Design 3 dashboard tabsinternal/metrics/metrics.go— Prometheus gaugesonwatch_credits_balance{unit="cny_available"|"cny_cash"|"usd_total"|"usd_topped_up"|"usd_granted"}main.go— agent factory registration + boot sequenceEdge cases handled
is_available=false→ log warning, skip snapshotparseFloatTests
1224 unit tests pass across
internal/api/...,internal/store/...,internal/tracker/.... Build green on Go 1.25.7.Live verification
Tested on Linux Mint 22.3, Cinnamon 6.6.7, kernel 6.17:
Note on Moonshot baseURL
Default endpoint set to
api.moonshot.ai(international). Users on the Chinesemoonshot.cnAPI endpoint may need to patch the baseURL constant or expose it via an env var (MOONSHOT_BASE_URL) — happy to add that as a follow-up if you'd like it in this PR.Test plan
go build ./...succeedsgo test ./...greenMOONSHOT_API_KEY=...populatesmoonshottabDEEPSEEK_API_KEY=...populatesdeepseektab🤖 Generated with Claude Code — code by Jules