Conversation
Made-with: Cursor # Conflicts: # internal/config/config.go
There was a problem hiding this comment.
Review: Add Cursor Provider Support
Solid provider implementation - follows the established pattern well and has good test coverage across all layers. A few things to address before merge:
Blocking
ParseIntString overflows on 32-bit (cursor_types.go)
Returns int (32-bit on 32-bit systems). Billing timestamps like "1768399334000" exceed int32 max. Fix: return int64 or use strconv.ParseInt.
Duplicate account type detection (cursor_client.go + cursor_types.go)
FetchQuotas and ToCursorSnapshot both detect account type independently. FetchQuotas then overwrites what ToCursorSnapshot set, but has extra logic (request-based -> enterprise promotion) the other doesn't. Pick one place to own this.
cursorTestMode data race (cursor_token.go)
Package-level bool read by production code and written by tests without synchronization. Will fail under -race. Use atomic.Bool.
Should fix
-
Platform file duplication -
cursor_token_unix.goandcursor_token_windows.goduplicatereadCursorStateValue,buildCursorAuthStatenearly verbatim. Extract shared logic tocursor_token.go. -
macOS path hardcoded in
!windowsbuild -getCursorStateDBPathreturns~/Library/Application Support/Cursor/...on Linux too. Should checkruntime.GOOS == "darwin"and use~/.config/Cursor/...on Linux. -
Unbounded
io.ReadAllinRefreshCursorToken- every other HTTP read in this file usesio.LimitReader. Be consistent. -
No menubar integration - other providers are wired into
menubar.go. Cursor quotas won't appear in the macOS status bar. -
insightStatstruct change - addingKey/Metric/Severity/Descto the shared struct works (fields areomitempty), but consider whether Cursor-specific enrichment deserves its own type.
Nits
- Missing newline at EOF in
CURSOR_SETUP.md - Whitespace-only changes in unrelated code (
codexProfileSave,stripProviderSecrets) - addsgit blamenoise cursorAuthToCredentialsappears unused outside tests - possible dead codegoto processSnapshotmatches existing pattern inanthropic_agent.gobut remains hard to follow
What's good
- Clean pattern compliance across all 6 layers
- Thorough tests at every level (types, client, token detection, store, tracker, agent, handlers)
- Token redaction, parameterized SQL,
LimitReaderon most reads - Three account types (Individual/Team/Enterprise) with appropriate quota formats
- Auto-detection from SQLite + Keychain/keyring with refresh token rotation
- Full dashboard wiring: charts, burn rate forecasts, cycle tracking, logging history
Nice work overall - the blocking issues are small fixes.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Review: Add Cursor Provider Support
Solid provider implementation - follows the established pattern well and has good test coverage across all layers. A few things to address before merge:
Blocking
ParseIntString overflows on 32-bit (cursor_types.go)
Returns int (32-bit on 32-bit systems). Billing timestamps like "1768399334000" exceed int32 max. Fix: return int64 or use strconv.ParseInt.
Duplicate account type detection (cursor_client.go + cursor_types.go)
FetchQuotas and ToCursorSnapshot both detect account type independently. FetchQuotas then overwrites what ToCursorSnapshot set, but has extra logic (request-based -> enterprise promotion) the other doesn't. Pick one place to own this.
cursorTestMode data race (cursor_token.go)
Package-level bool read by production code and written by tests without synchronization. Will fail under -race. Use atomic.Bool.
Should fix
-
Platform file duplication -
cursor_token_unix.goandcursor_token_windows.goduplicatereadCursorStateValue,buildCursorAuthStatenearly verbatim. Extract shared logic tocursor_token.go. -
macOS path hardcoded in
!windowsbuild -getCursorStateDBPathreturns~/Library/Application Support/Cursor/...on Linux too. Should checkruntime.GOOS == "darwin"and use~/.config/Cursor/...on Linux. -
Unbounded
io.ReadAllinRefreshCursorToken- every other HTTP read in this file usesio.LimitReader. Be consistent. -
No menubar integration - other providers are wired into
menubar.go. Cursor quotas won't appear in the macOS status bar. -
insightStatstruct change - addingKey/Metric/Severity/Descto the shared struct works (fields areomitempty), but consider whether Cursor-specific enrichment deserves its own type.
Nits
- Missing newline at EOF in
CURSOR_SETUP.md - Whitespace-only changes in unrelated code (
codexProfileSave,stripProviderSecrets) - addsgit blamenoise cursorAuthToCredentialsappears unused outside tests - possible dead codegoto processSnapshotmatches existing pattern inanthropic_agent.gobut remains hard to follow
What's good
- Clean pattern compliance across all 6 layers
- Thorough tests at every level (types, client, token detection, store, tracker, agent, handlers)
- Token redaction, parameterized SQL,
LimitReaderon most reads - Three account types (Individual/Team/Enterprise) with appropriate quota formats
- Auto-detection from SQLite + Keychain/keyring with refresh token rotation
- Full dashboard wiring: charts, burn rate forecasts, cycle tracking, logging history
Nice work overall - the blocking issues are small fixes.
…esponding test case - Updated `applyRefreshedCredentials` method to return false on save failure. - Added a new test `TestCursorAgent_ApplyRefreshedCredentials_FailsWhenSaveFails` to verify behavior when token saving fails. - Refactored `FetchQuotas` to streamline account type determination and removed unused code. - Enhanced `ToCursorSnapshot` to include request-based usage logic. - Updated various tests to accommodate changes in function signatures and behavior.
|
Hi @LeslieLeung - thanks for the quick turnaround on
Looks good to me - happy to approve. One thing before we merge: the branch is currently behind |
prakersh
left a comment
There was a problem hiding this comment.
Confirmed all blocking/should-fix items addressed. Security review clean - no new vulnerabilities introduced. LGTM.
No description provided.