feat(gemini): add profile management and advanced settings UI#74
feat(gemini): add profile management and advanced settings UI#74Itstommy10 wants to merge 2 commits into
Conversation
- implement saved profiles list and deletion in provider settings modal - add profiles directory, quota display, and pace mode configuration - wire profiles directory override from database to runtime manager - align Gemini provider features with Codex for consistency
prakersh
left a comment
prakersh
left a comment
There was a problem hiding this comment.
Hey, thanks for putting this together! Multi-account Gemini support is something we've wanted (issue #73), and the overall architecture here - profile-based management with a manager pattern mirroring how Codex multi-account works - is the right direction. There are some issues to sort out before this can merge though. I've grouped them by severity to make it easier to work through.
Build Failures (CI broken)
The PR references types and functions that don't exist in the current codebase. It looks like there are API-layer changes that were meant to be part of this PR but weren't included in the diff:
api.GeminiCredentials.UserID- this field doesn't exist on the struct (seeinternal/api/gemini_credentials.go:13-19)api.ParseGeminiIDTokenUserID()- function doesn't exist anywhere in the repoapi.GeminiCredentials.CompositeExternalID()- method doesn't existagent.NewGeminiAgentWithAccount()- onlyNewGeminiAgentexists currentlyagent.GeminiAgent.SetTokenSave()- not defined on the agent
There are also two call-site signature changes that don't match the existing functions:
store.QueryGeminiRange(0, start, end)- current signature is(start, end time.Time, limit ...int), no account ID parametertracker.UsageSummary(0, modelID)- current signature is(familyID string), no account ID parameter
Both CI jobs (Test and Menubar macOS) fail with 9+ compilation errors from these missing pieces. If you have these changes locally, they just need to be included in the PR.
Security Issues
A few things here that should be addressed before merge:
Path traversal in profile names (High):
Profile names are used directly in file path construction without any sanitization:
profilePath := filepath.Join(profilesDir, name+".json")A name like ../../.ssh/authorized_keys would resolve outside the profiles directory. This affects save, delete, refresh, SaveProfile, and DeleteProfile. A straightforward fix would be validating names against something like [a-zA-Z0-9_-], or using filepath.Base() and checking the result.
Credential file written world-readable (High):
In gemini_agent_manager.go, SaveProfile writes files with 0644:
return os.WriteFile(profilePath, data, 0644)Since these files contain OAuth tokens, they'd be readable by any user on the system. The rest of the codebase consistently uses 0o600 for credential files - this should match.
Plaintext credential storage (Medium):
Profiles store access tokens, refresh tokens, and ID tokens as plaintext JSON. The project uses macOS Keychain for Anthropic tokens (see internal/api/anthropic_token_unix.go). Worth considering whether Gemini tokens should follow the same pattern rather than introducing a separate, less secure storage path. Happy to discuss the tradeoffs here if helpful.
Missing Functionality
No API route handlers registered:
The frontend JS calls ${API_BASE}/api/gemini/profiles (both GET and DELETE), but no route handler for that path is registered anywhere in handlers.go. The PR adds SetGeminiAgentManager but the actual endpoint wiring is missing - so all profile operations from the UI will return 404.
Hardcoded placeholder in production code (gemini_profiles.go:315):
// TODO: Sostituire con ricerca corretta per nome
dbAccount, err := st.GetProviderAccountByID(1) // Segnaposto temporaneo per sbloccare la buildThis will return wrong account data for every profile except the one that happens to have DB ID 1.
Code Structure
Root-level files:
gemini_profiles.go (373 lines) and gemini_profiles_test.go are in the repo root as package main. The project's convention is to put this kind of logic under internal/ - for reference, the Codex equivalent lives at internal/agent/codex_agent_manager.go. Moving these would keep the codebase consistent.
Duplicate function definitions:
geminiCompositeExternalID and geminiProfileCompositeExternalID are defined identically in both gemini_profiles.go and gemini_agent_manager.go. These should live in one place.
Dead code in main.go:
geminiClient = api.NewGeminiClient(token, logger)
_ = geminiClient // Marcatore per evitare declared and not usedSince the PR replaces the single-agent approach with the manager, geminiClient is no longer needed and can be removed.
Test Issues
The tests have a few problems that would prevent them from passing even if the build issues were fixed:
gemini_profiles_test.go: Creates profiles at~/.gemini/profiles/butgeminiProfilesDir()returns~/.onwatch/data/gemini-profiles/- the test will never find its own test datagemini_agent_manager_test.go: Callsf.manager.Run()with no arguments, butRun(ctx context.Context)requires a context- Several tests construct
api.GeminiCredentials{UserID: ...}which won't compile sinceUserIDisn't a field on that struct
Minor
- Comments in Italian ("Segnaposto temporaneo", "Sostituire con ricerca corretta", "Marcatore per evitare") - should be in English for consistency
- The template parsing refactor (
template.New("").ParseFStotemplate.ParseFS+ manualLookup) is an unrelated change that alters template naming semantics - would be cleaner as a separate PR if intentional subtle.ConstantTimeCompareis used for comparing locally-stored tokens where there's no timing attack vector - a plain!=would be clearer and more appropriate here- Verbose template debug logging (iterating and logging all template names) added to production code paths
I know that's a lot of items, but the core design is solid. Once the missing API-layer changes are included and these issues are addressed, this should come together well. Happy to help with any of these if you have questions.
|
Hi! I've pushed a new commit (f6ad685) to PR #74 addressing all the points raised in the Gemini review. Here's a summary of what was done: API & account handling
Security
Code quality
Validation (Full 12 files changed, +317 / -179. Let me know if there's anything else you'd like me to fix or improve — happy to keep iterating! |
prakersh
left a comment
prakersh
left a comment
There was a problem hiding this comment.
Good progress on this second commit - the API types, path traversal fix, file permissions, route registration, and code restructuring from the first round are all addressed well. I did a thorough audit of every changed file and found some remaining issues. Grouped by severity:
CI Blocker
main.go:1030: undefined: geminiProfilesDir
geminiProfilesDir() is an unexported function in package agent (internal/agent/gemini_profiles_cli.go:25), but main.go calls it as a bare unqualified function. Either export it as agent.GeminiProfilesDir() or provide a default through the manager.
Critical: Security
1. API endpoint leaks OAuth tokens to the browser
geminiProfilesList (handlers.go:382-388) calls ListProfiles() which returns []GeminiProfile - including Tokens.AccessToken, Tokens.RefreshToken, and Tokens.IDToken (all with explicit json: tags). respondJSON does raw json.Encode, so all three tokens are serialized to the HTTP response. Any browser tab hitting GET /api/gemini/profiles receives full OAuth credentials.
The existing codexProfilesList avoids this by building a safe response map with only id, name, createdAt. The Gemini handler should do the same - project only name, project_id, saved_at.
2. XSS: profile.project_id unescaped in settings modal
app.js line ~9252:
html += `<div ...>${profile.project_id || 'Auto-detected'}</div>`;profile.name is correctly escaped with escapeHtml() on the adjacent line, but project_id is interpolated raw. Should be escapeHtml(profile.project_id) || 'Auto-detected'.
Critical: Functionality
3. Profile switching is entirely non-functional
switchGeminiProfile stores the selected profile in State.geminiAccount and localStorage, then reloads the page. But State.geminiAccount is never sent in any API request. There is no geminiAccountParam() function, and providerParam() (lines 75-85) has explicit branches for codex and minimax but none for gemini. The same gap exists in the multi-account ternary chains at lines ~6049, ~7200, ~7480.
The result: profile switching appears to work visually (the dropdown updates) but all data fetches still use the default account. The feature is a no-op.
4. FetchTier(nil) panics in CLI commands
gemini_profiles_cli.go lines 155 and 222 call client.FetchTier(nil). FetchTier calls context.WithTimeout(ctx, 30*time.Second) which panics on a nil context. The manager's SaveProfile correctly uses context.Background() - the CLI should do the same.
5. GeminiTracker state maps cause cross-account data corruption
gemini_tracker.go - lastFractions and lastResetTimes maps (lines 16-17) are keyed by modelID only, not (accountID, modelID). When the tracker processes snapshots from multiple accounts, account A's state for "gemini-flash" is overwritten by account B's data, causing false reset detections and incorrect delta calculations. The fix is to key by fmt.Sprintf("%d:%s", accountID, modelID).
6. QueryGeminiUsageSeries missing account_id filter
gemini_store.go lines 470-499 - This function was not updated in the PR. It joins gemini_quota_values and gemini_snapshots with no account_id filter, so it aggregates usage data from all accounts into one series. It needs an accountID parameter and WHERE s.account_id = ? clause, like every other query function in the file.
Important
7. getDatabasePath() diverges between CLI and daemon
main.go:77:filepath.Join(defaultPIDDir(), "onwatch.db")gemini_profiles_cli.go:16-22:filepath.Join(home, ".onwatch", "onwatch.db")
On macOS these happen to match since defaultPIDDir() returns $HOME/.onwatch. On Windows, defaultPIDDir() uses %LOCALAPPDATA%\onwatch while the CLI version uses %USERPROFILE%\.onwatch. CLI profile commands will open a different (empty) database than the running daemon, making gemini profile status show all profiles as "untracked/never" even when the daemon has data.
8. lastScanProfiles data race
gemini_agent_manager.go - lastScanProfiles is a map[string]time.Time accessed from:
loadAndStartProfiles(line 162) - no lockscanForProfileChanges(lines 547, 560, 578) - no lockSetTokenSaveclosure (line 443) - underm.mu.Lock()
The scanner goroutine and poll goroutines can access this map concurrently. CLAUDE.md requires -race before commit. Protect all accesses under m.mu, or use a dedicated mutex.
9. gemini_reset_cycles DDL missing account_id
store.go line 585 - CREATE TABLE IF NOT EXISTS gemini_reset_cycles does not include account_id (it's only added via ALTER TABLE migration at line 992). But gemini_snapshots DDL at line 565 does include it. This asymmetry means fresh databases rely on the migration running to add the column, and any test that creates only the schema without running migrations will fail with "no such column: account_id" on cycle queries.
10. Backfill migration predicate too broad
store.go lines 1019/1025 - WHERE account_id = 0 OR account_id = 1 will reassign any row with account_id = 1 to the new default ID, even if 1 was already a valid, correctly-assigned provider_accounts.id. The MiniMax equivalent (line 920) only uses WHERE account_id = 0. Should match that pattern.
11. DeleteProfile doesn't stop the running agent
gemini_agent_manager.go lines 808-818 - DeleteProfile removes the file but doesn't call m.stopAgent(name) or clean up lastScanProfiles. The agent goroutine continues polling with stale credentials until the scanner notices the file is gone (up to 30 seconds later). Should stop the agent immediately.
12. Empty composite ID false matches
gemini_profiles_cli.go findGeminiProfileByAccount - when both projectID and userID are empty, geminiCompositeExternalID("", "") returns "", and any profile with similarly empty fields also returns "", causing "" == "" to match. This blocks saves with spurious "account is already saved as profile 'X'" errors. Guard against empty composites.
13. listGeminiProfiles (CLI) vs ListProfiles (manager) diverge
The manager's ListProfiles backfills p.Name from the filename when it's empty in JSON (line 718). The CLI's listGeminiProfiles does not. Profiles saved without a Name field will show blank names in CLI output. Consolidate into one function or add the same backfill.
Minor
14. getGeminiPaceMode() is dead code - defined in handlers.go but never called anywhere.
15. geminiProfileSave POST body unbounded - no http.MaxBytesReader wrapper unlike other POST handlers (e.g., MenubarPreferences uses MaxBytesReader(w, r.Body, 64*1024)).
16. No outside-click handler for Gemini dropdown - Codex profile dropdown has document click + Escape key handlers to close it; the Gemini equivalent doesn't.
17. store.New errors silently swallowed in CLI - geminiProfileSave and geminiProfileRefresh use st, _ := store.New(...). Compare geminiProfileStatus which correctly checks the error.
I know this is a long list, but the architecture is solid and most of the infrastructure (store layer, agent manager, profile scanner, migration) is well done. The critical items are: the token leak in the API response, the profile switching not actually working, and the tracker state maps not being per-account. Once those are addressed, the rest are straightforward fixes. Happy to help work through any of these.
2 participants
This PR enhances the Gemini provider by adding profile management and advanced configuration options to the UI, matching the functionality available for the Codex provider.
Key Changes:
Why:
Previously, Gemini settings only allowed toggling telemetry. This update provides parity with Codex, allowing users with multiple Google Cloud projects or specific quota needs to manage them more effectively through the web interface.