Feat/refresh account #401
Feat/refresh account #401
Conversation
…refine account merging logic to prioritize email for deduplication.
…d update package dependencies.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the
WalkthroughThis pull request introduces error notification functionality with Telegram integration and account synchronization features. Changes include a new notification service ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryThis PR adds account refresh and error notification features, including Telegram integration for remote monitoring. The changes refactor notification logic from a separate module into Key changes:
Issues identified:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Plugin as plugin.ts
participant AccountMgr as AccountManager
participant Request as request.ts
participant API as Antigravity API
participant Telegram as Telegram Bot
Client->>Plugin: fetch(request)
Plugin->>AccountMgr: getCurrentOrNextForFamily()
AccountMgr-->>Plugin: account
alt Account Available
Plugin->>Request: prepareAntigravityRequest(token, account.fingerprint)
Request-->>Plugin: prepared request
Plugin->>API: fetch(prepared)
alt Success (200)
API-->>Plugin: response
Plugin->>Request: transformAntigravityResponse(response, account)
Request-->>Plugin: transformed response
Plugin-->>Client: response
else Rate Limited (429)
API-->>Plugin: 429 response
Plugin->>AccountMgr: markRateLimited(account)
alt Multiple Accounts
Plugin->>AccountMgr: switchToNextAccount()
AccountMgr-->>Plugin: next account
Note over Plugin: Retry with new account
else Single Account
Plugin->>Request: transformAntigravityResponse(lastFailure)
Note over Plugin,Request: Missing account parameter
Request-->>Plugin: response with "Account: Unknown"
Plugin-->>Client: error response
end
else Auth Error (401/403)
API-->>Plugin: 401/403 response
Plugin->>AccountMgr: disableAccount(account)
opt Telegram Configured
Plugin->>Telegram: sendNotification(account.email, error)
Telegram-->>Plugin: notification sent
end
Plugin->>AccountMgr: getCurrentOrNextForFamily()
Note over Plugin: Switch to next account
end
else All Accounts Unavailable
Plugin-->>Client: Error: All accounts rate-limited
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| } | ||
| shouldSwitchAccount = true; | ||
| break; | ||
| } | ||
| } | ||
| } // end headerStyleLoop | ||
|
|
||
| if (shouldSwitchAccount) { | ||
| // Avoid tight retry loops when there's only one account. | ||
| if (accountCount <= 1) { | ||
| if (lastFailure) { | ||
| return transformAntigravityResponse( | ||
| lastFailure.response, | ||
| lastFailure.streaming, | ||
| lastFailure.debugContext, | ||
| lastFailure.requestedModel, | ||
| lastFailure.projectId, | ||
| lastFailure.endpoint, | ||
| lastFailure.effectiveModel, | ||
| lastFailure.sessionId, | ||
| lastFailure.toolDebugMissing, | ||
| lastFailure.toolDebugSummary, | ||
| lastFailure.toolDebugPayload, | ||
| debugLines, | ||
| ); | ||
| } // end headerStyleLoop | ||
|
|
||
| if (shouldSwitchAccount) { | ||
| // Avoid tight retry loops when there's only one account. | ||
| if (accountCount <= 1) { | ||
| if (lastFailure) { | ||
| return transformAntigravityResponse( | ||
| lastFailure.response, | ||
| lastFailure.streaming, | ||
| lastFailure.debugContext, | ||
| lastFailure.requestedModel, | ||
| lastFailure.projectId, | ||
| lastFailure.endpoint, |
There was a problem hiding this comment.
Missing account in debug
In the single-account failure path, transformAntigravityResponse(...) is called without the new account argument, so error debug output will show Account: Unknown even though the account is known. Pass the account identifier (e.g. account.email) through lastFailure and into this call to keep debug info consistent.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 1819:1833
Comment:
**Missing account in debug**
In the single-account failure path, `transformAntigravityResponse(...)` is called without the new `account` argument, so error debug output will show `Account: Unknown` even though the account is known. Pass the account identifier (e.g. `account.email`) through `lastFailure` and into this call to keep debug info consistent.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| continue; | ||
| } | ||
| // If we get here without returning, something went wrong | ||
| if (lastFailure) { | ||
| return transformAntigravityResponse( | ||
| lastFailure.response, | ||
| lastFailure.streaming, | ||
| lastFailure.debugContext, | ||
| lastFailure.requestedModel, | ||
| lastFailure.projectId, |
There was a problem hiding this comment.
Missing account in debug
Same issue as above: the fallback transformAntigravityResponse(...) call after the loop omits the new account parameter, so debug output loses which account produced the error. This will always happen whenever lastFailure is returned from this branch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 1847:1856
Comment:
**Missing account in debug**
Same issue as above: the fallback `transformAntigravityResponse(...)` call after the loop omits the new `account` parameter, so debug output loses which account produced the error. This will always happen whenever `lastFailure` is returned from this branch.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugin/accounts.ts`:
- Around line 774-791: syncEnabledStatusFromDisk() is ineffective because
saveToDisk() never persists the enabled flag; update saveToDisk() to include
each account's enabled state in the serialized AccountStorageV3 (so
loadAccounts() can read it) and then change syncEnabledStatusFromDisk() to only
update account.enabled for in-memory accounts whose refreshToken exists on disk
(i.e., set account.enabled = storedEnabledSet.has(account.parts.refreshToken)
only when that token is present) to avoid disabling newly-added in-memory
accounts that haven't been flushed yet; reference the saveToDisk(),
loadAccounts(), syncEnabledStatusFromDisk(), and this.accounts symbols when
making the change.
🧹 Nitpick comments (2)
src/plugin/request.ts (1)
1486-1500: 13 positional parameters is unwieldy — consider an options object.This function now has 13 positional parameters, most of which are optional. This makes call sites fragile (easy to swap arguments) and hard to read. Consider consolidating trailing optional parameters into a single options/config object.
This is a pre-existing concern amplified by the new
accountparameter, so it can be deferred.src/plugin/accounts.ts (1)
788-790: Add debug logging to the empty catch block to align with project patterns.The fallback to in-memory state is intentional and reasonable, but the codebase widely uses structured logging across plugins (9 files already use
createLogger). Adding debug-level logging here would improve debuggability without compromising the graceful fallback behavior.Suggested improvement
+import { createLogger } from "./logger"; +const log = createLogger("accounts"); + } catch (error) { - // Ignore errors reading from disk, fallback to in-memory state + // Fallback to in-memory state on disk read errors + log.debug("syncEnabledStatusFromDisk failed, using in-memory state", { error }); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/plugin.tssrc/plugin/accounts.tssrc/plugin/request.ts
🔇 Additional comments (1)
src/plugin/request.ts (1)
1571-1573: LGTM —accountfield added to debug info and ordering adjusted.The new
Accountline in the debug block and the reordering ofinjectedDebugbeforedebugInfolook correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async syncEnabledStatusFromDisk(): Promise<void> { | ||
| try { | ||
| const stored = await loadAccounts(); | ||
| if (!stored) return; | ||
|
|
||
| const enabledRefreshTokens = new Set( | ||
| stored.accounts | ||
| .filter((a) => a.enabled !== false) | ||
| .map((a) => a.refreshToken) | ||
| ); | ||
|
|
||
| for (const account of this.accounts) { | ||
| account.enabled = enabledRefreshTokens.has(account.parts.refreshToken); | ||
| } | ||
| } catch (error) { | ||
| // Ignore errors reading from disk, fallback to in-memory state | ||
| } | ||
| } |
There was a problem hiding this comment.
saveToDisk() never persists enabled, so this sync is effectively a no-op.
syncEnabledStatusFromDisk() reads a.enabled !== false from stored accounts, but saveToDisk() (lines 799–812) does not include enabled in the serialized object. Since the constructor defaults enabled to true when the field is absent (line 298: enabled: acc.enabled !== false), every account loaded from disk will appear enabled, so the enabledRefreshTokens set will always contain all disk tokens.
For this method to work as intended, saveToDisk must persist the enabled flag:
Proposed fix
accounts: this.accounts.map((a) => ({
email: a.email,
refreshToken: a.parts.refreshToken,
projectId: a.parts.projectId,
managedProjectId: a.parts.managedProjectId,
addedAt: a.addedAt,
lastUsed: a.lastUsed,
+ enabled: a.enabled,
lastSwitchReason: a.lastSwitchReason,
rateLimitResetTimes: Object.keys(a.rateLimitResetTimes).length > 0 ? a.rateLimitResetTimes : undefined,Additionally, accounts present in memory but absent from disk will be silently disabled by this method (line 786 yields false for tokens not in the set). If an account was recently added in-memory but not yet flushed, this sync would disable it. Consider guarding against that, e.g. only updating accounts whose refresh token exists on disk.
#!/bin/bash
# Verify that 'enabled' is part of the AccountStorageV3 type definition
# and check if it's persisted anywhere else
rg -n "enabled" --type=ts -C3 -g '**/storage*'🤖 Prompt for AI Agents
In `@src/plugin/accounts.ts` around lines 774 - 791, syncEnabledStatusFromDisk()
is ineffective because saveToDisk() never persists the enabled flag; update
saveToDisk() to include each account's enabled state in the serialized
AccountStorageV3 (so loadAccounts() can read it) and then change
syncEnabledStatusFromDisk() to only update account.enabled for in-memory
accounts whose refreshToken exists on disk (i.e., set account.enabled =
storedEnabledSet.has(account.parts.refreshToken) only when that token is
present) to avoid disabling newly-added in-memory accounts that haven't been
flushed yet; reference the saveToDisk(), loadAccounts(),
syncEnabledStatusFromDisk(), and this.accounts symbols when making the change.
- Merged all updates from main branch - Re-applied syncEnabledStatusFromDisk() feature for rate limit handling - Resolved conflicts by accepting main's structure
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugin/request.ts`:
- Around line 1580-1583: The debug block that builds debugInfo uses the
`account` parameter which is never populated at call sites and currently logs
email (PII); update call sites that invoke the function which builds `debugInfo`
to pass the selected account's index (e.g. use `account.index` from the selected
account in the callers around the earlier calls) and change the debugInfo
construction to reference that index (or, if account context is not required,
remove the unused `account` parameter from the function and the `${account ||
"Unknown"}` fragment). Specifically, ensure callers that call the
request/error-handling function supply the account index instead of an email and
the `debugInfo` string uses that index to avoid logging PII.
🧹 Nitpick comments (1)
src/plugin/request.ts (1)
1496-1510: Consider consolidating into an options object.
transformAntigravityResponsenow has 13 positional parameters, most of them optional. This makes call sites fragile and hard to read (easy to swap twostring | undefinedargs). Consider grouping the non-essential params into a single options/context object in a follow-up.♻️ Sketch of a potential options object
+export interface TransformResponseOptions { + debugContext?: AntigravityDebugContext | null; + requestedModel?: string; + projectId?: string; + endpoint?: string; + effectiveModel?: string; + sessionId?: string; + toolDebugMissing?: number; + toolDebugSummary?: string; + toolDebugPayload?: string; + debugLines?: string[]; + account?: string; +} + export async function transformAntigravityResponse( response: Response, streaming: boolean, - debugContext?: AntigravityDebugContext | null, - requestedModel?: string, - ... - account?: string, + options?: TransformResponseOptions, ): Promise<Response> {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/plugin/accounts.tssrc/plugin/request.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugin/accounts.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugin/request.ts (1)
scripts/check-quota.mjs (1)
projectId(175-175)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (errorBody?.error) { | ||
| const debugInfo = `\n\n[Debug Info]\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; | ||
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; | ||
| const injectedDebug = debugText ? `\n\n${debugText}` : ""; | ||
| errorBody.error.message = (errorBody.error.message || "Unknown error") + debugInfo + injectedDebug; | ||
| errorBody.error.message = (errorBody.error.message || "Unknown error") + injectedDebug + debugInfo; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts -C5 'transformAntigravityResponse' -g '!*test*' -g '!*spec*'Repository: NoeFabris/opencode-antigravity-auth
Length of output: 4015
🏁 Script executed:
sed -n '1496,1520p' src/plugin/request.tsRepository: NoeFabris/opencode-antigravity-auth
Length of output: 1066
🏁 Script executed:
# Check call site at line 1389
sed -n '1389,1410p' src/plugin.ts
echo "---"
# Check call site at line 1914
sed -n '1914,1935p' src/plugin.ts
echo "---"
# Check call site at line 2004
sed -n '2004,2025p' src/plugin.ts
echo "---"
# Check call site at line 2028
sed -n '2028,2049p' src/plugin.tsRepository: NoeFabris/opencode-antigravity-auth
Length of output: 3428
🏁 Script executed:
# Search for account-related variables/context in the main plugin file
rg -n 'account' src/plugin.ts | head -30Repository: NoeFabris/opencode-antigravity-auth
Length of output: 1904
🏁 Script executed:
# Search for where accountIndex is used in request flow
rg -n 'accountIndex' src/plugin.ts | grep -E '(loop|for|while|try|fetch)' -B3 -A3Repository: NoeFabris/opencode-antigravity-auth
Length of output: 501
🏁 Script executed:
# Search for where accounts are iterated in the main request flow
rg -n 'for.*account|accountIndex.*=' src/plugin.ts | head -20Repository: NoeFabris/opencode-antigravity-auth
Length of output: 1720
🏁 Script executed:
# Look at the main request handler around line 1900-1950 to see account context
sed -n '1850,1950p' src/plugin.tsRepository: NoeFabris/opencode-antigravity-auth
Length of output: 5350
🏁 Script executed:
# Look at the broader loop structure containing the transformAntigravityResponse call
sed -n '1700,1850p' src/plugin.tsRepository: NoeFabris/opencode-antigravity-auth
Length of output: 7580
🏁 Script executed:
# Look for where the account variable is declared in the main request handler
sed -n '1100,1350p' src/plugin.ts | grep -n -B5 -A5 'account\s*='Repository: NoeFabris/opencode-antigravity-auth
Length of output: 533
🏁 Script executed:
# Get more context around line 1118 (1100 + 18) to see the account variable and its scope
sed -n '1100,1250p' src/plugin.tsRepository: NoeFabris/opencode-antigravity-auth
Length of output: 7295
🏁 Script executed:
# Check what account data is available and whether account.email could contain PII
# Look at the account object structure
rg -n 'type.*Account|interface.*Account' src/plugin/accounts.ts | head -20Repository: NoeFabris/opencode-antigravity-auth
Length of output: 548
🏁 Script executed:
# Get the ManagedAccount interface definition
sed -n '142,200p' src/plugin/accounts.tsRepository: NoeFabris/opencode-antigravity-auth
Length of output: 2066
Pass account.index instead of account.email to maintain privacy.
The account parameter in the function signature is never passed at any call site (lines 1389, 1914, 2004, 2028), so the debug output always shows "Unknown". If the intent is to include account information for debugging, pass account.index (available in scope as the selected account object) rather than the email address, which would be PII. Alternatively, if account context isn't needed, remove the unused parameter.
🤖 Prompt for AI Agents
In `@src/plugin/request.ts` around lines 1580 - 1583, The debug block that builds
debugInfo uses the `account` parameter which is never populated at call sites
and currently logs email (PII); update call sites that invoke the function which
builds `debugInfo` to pass the selected account's index (e.g. use
`account.index` from the selected account in the callers around the earlier
calls) and change the debugInfo construction to reference that index (or, if
account context is not required, remove the unused `account` parameter from the
function and the `${account || "Unknown"}` fragment). Specifically, ensure
callers that call the request/error-handling function supply the account index
instead of an email and the `debugInfo` string uses that index to avoid logging
PII.
| toolDebugSummary?: string, | ||
| toolDebugPayload?: string, | ||
| debugLines?: string[], | ||
| account?: string, |
There was a problem hiding this comment.
The new account parameter is not being passed by any of the callers in src/plugin.ts (lines 1914, 2004, 2028). This means the debug output will always show "Account: Unknown" even though the account information is available at all call sites via account.email || \Account ${account.index + 1}``.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/request.ts
Line: 1509:1509
Comment:
The new `account` parameter is not being passed by any of the callers in `src/plugin.ts` (lines 1914, 2004, 2028). This means the debug output will always show "Account: Unknown" even though the account information is available at all call sites via `account.email || \`Account ${account.index + 1}\``.
How can I resolve this? If you propose a fix, please make it concise.| async syncEnabledStatusFromDisk(): Promise<void> { | ||
| try { | ||
| const stored = await loadAccounts(); | ||
| if (!stored) return; | ||
|
|
||
| const enabledRefreshTokens = new Set( | ||
| stored.accounts | ||
| .filter((a) => a.enabled !== false) | ||
| .map((a) => a.refreshToken) | ||
| ); | ||
|
|
||
| for (const account of this.accounts) { | ||
| account.enabled = enabledRefreshTokens.has(account.parts.refreshToken); | ||
| } | ||
| } catch (error) { | ||
| // Ignore errors reading from disk, fallback to in-memory state | ||
| } | ||
| } |
There was a problem hiding this comment.
The syncEnabledStatusFromDisk() method is added but never called anywhere in the codebase. If this is intended for future use, consider adding a TODO comment. If it should be called as part of this feature, the call site is missing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/accounts.ts
Line: 899:916
Comment:
The `syncEnabledStatusFromDisk()` method is added but never called anywhere in the codebase. If this is intended for future use, consider adding a TODO comment. If it should be called as part of this feature, the call site is missing.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugin/request.ts`:
- Line 1581: The debugInfo template duplicates the Account field by appending
`\nAccount: ${account || "Unknown"}` inside the `toolDebugPayload` conditional;
remove that trailing Account snippet so the `toolDebugPayload` branch only
includes the payload itself. Update the string construction where `const
debugInfo = ...` references `toolDebugPayload` (and ensure `toolDebugMissing`,
`toolDebugSummary` branches remain unchanged) so `Account` appears only once at
the top of the `[Debug Info]` block.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/plugin/request.ts
🔇 Additional comments (1)
src/plugin/request.ts (1)
1654-1664: No-op whitespace changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Inject Debug Info | ||
| if (errorBody?.error) { | ||
| const debugInfo = `\n\n[Debug Info]\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; | ||
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`; |
There was a problem hiding this comment.
Bug: Account is duplicated in the debug info string.
Account: ${account || "Unknown"} appears twice — once at the top of the [Debug Info] block and again at the end inside the toolDebugPayload conditional. The second occurrence looks like a copy-paste mistake.
Proposed fix
- const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`;
+ const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`; | |
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; |
🤖 Prompt for AI Agents
In `@src/plugin/request.ts` at line 1581, The debugInfo template duplicates the
Account field by appending `\nAccount: ${account || "Unknown"}` inside the
`toolDebugPayload` conditional; remove that trailing Account snippet so the
`toolDebugPayload` branch only includes the payload itself. Update the string
construction where `const debugInfo = ...` references `toolDebugPayload` (and
ensure `toolDebugMissing`, `toolDebugSummary` branches remain unchanged) so
`Account` appears only once at the top of the `[Debug Info]` block.
| // Inject Debug Info | ||
| if (errorBody?.error) { | ||
| const debugInfo = `\n\n[Debug Info]\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; | ||
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`; |
There was a problem hiding this comment.
Duplicate Account: field in debug output. Account is already shown at the start of debugInfo, but here it's being added again after Tool Debug Payload. Remove the duplicate \nAccount: ${account || "Unknown"} from the end of the toolDebugPayload ternary.
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`; | |
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/request.ts
Line: 1581:1581
Comment:
Duplicate `Account:` field in debug output. Account is already shown at the start of `debugInfo`, but here it's being added again after `Tool Debug Payload`. Remove the duplicate `\nAccount: ${account || "Unknown"}` from the end of the `toolDebugPayload` ternary.
```suggestion
const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`;
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/CONFIGURATION.md`:
- Around line 252-262: The fenced code block example is missing a language
identifier (triggering markdownlint MD040); update the triple-backtick fence for
that notification example to include the text language specifier (change ``` to
```text) so the block is treated as plain text and the linter warning is
resolved.
In `@src/plugin/notification.ts`:
- Around line 110-136: The Telegram send is using parse_mode: "Markdown" while
formatNotificationMessage produces unescaped dynamic text (errorType,
errorMessage, payload); remove the parse_mode option (or set it to plain text)
where the message is sent so messages from formatNotificationMessage are
delivered as plain text, and if you prefer MarkdownV2 instead, escape all
interpolated values (errorType, errorMessage, payload) before sending; locate
the send call that passes parse_mode (referenced alongside
formatNotificationMessage) and remove that parse_mode property or replace it
with plain text sending.
🧹 Nitpick comments (3)
src/plugin/config/schema.ts (1)
300-306: Inconsistent indentation in the JSDoc comment.The comment body on line 301 uses 5-space indentation while the rest of the file uses consistent 1- or 3-space indentation within JSDoc blocks. This appears to be a rebase/merge artifact.
Proposed fix
/** - * Switch to another account immediately on first rate limit (after 1s delay). - * When disabled, retries same account first, then switches on second rate limit. + * Switch to another account immediately on first rate limit (after 1s delay). + * When disabled, retries same account first, then switches on second rate limit. * - * `@default` true + * `@default` true */ switch_on_first_rate_limit: z.boolean().default(true),src/plugin/notification.ts (2)
159-171: No timeout on the Telegramfetchcall.If the Telegram API is unreachable or slow, this
fetchwill hang indefinitely. SincesendTelegramMessageis called fire-and-forget fromnotifyAccountError(line 259), a hanging promise won't block the caller, but it will leak resources. Consider adding anAbortSignal.timeout().Proposed fix
const response = await fetch(url, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ chat_id: config.chatId, text: message, - parse_mode: "Markdown", disable_notification: false, }), + signal: AbortSignal.timeout(10_000), });
230-235: Account email logged atinfolevel.
accountEmailis PII. While this is the plugin owner's own account and logging goes to their local TUI/console, consider whetherdebuglevel would be more appropriate to avoid surfacing email addresses in routine log output.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
docs/CONFIGURATION.mdsrc/plugin.tssrc/plugin/config/schema.tssrc/plugin/notification.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugin/notification.ts (1)
src/plugin/logger.ts (1)
createLogger(64-106)
🪛 markdownlint-cli2 (0.20.0)
docs/CONFIGURATION.md
[warning] 252-252: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (9)
docs/CONFIGURATION.md (2)
188-189: LGTM!New environment variable overrides for Telegram are documented clearly and consistently with the existing pattern.
194-264: Documentation for Error Notifications section looks good.The section is well-structured with clear setup steps, configuration table, and an example payload. It aligns with the new schema fields and the
notification.tsimplementation.src/plugin/config/schema.ts (2)
430-466: LGTM! Notification config fields are well-defined.Defaults, constraints, and JSDoc comments are consistent with the documentation and the notification implementation. The
min(0)on cooldown correctly allows disabling it.
506-507: LGTM!DEFAULT_CONFIG values match the schema defaults for the new notification fields.
src/plugin/notification.ts (5)
1-10: LGTM!Clean module header and logger setup.
12-62: LGTM!Interfaces are well-defined with clear documentation. The
errorTypeunion covers the expected failure modes, and theNotificationConfigcleanly separates concerns (enabled flag, Telegram config, cooldown, quiet mode).
64-105: LGTM — cooldown and cleanup logic is sound.Cleanup only runs when the map exceeds
MAX_COOLDOWN_ENTRIES, and iterating+deleting aMapis safe in JS. The early return forcooldownMs <= 0correctly bypasses the entire cooldown path.
215-265: Overall notification orchestration is well-structured.Toast is awaited for UX ordering, Telegram is fire-and-forget with a defensive
.catch()— good pattern. Theclientnull-check andquietModeguard are clean.
267-279: LGTM!Returning a shallow copy from
getNotificationCooldownStatecorrectly prevents external mutation of internal state.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ``` | ||
| ⚠️ Account Error | ||
| ━━━━━━━━━━━━━━━━━━━━━━━━ | ||
| 📧 Account: user@gmail.com | ||
| ❌ Error: invalid_grant | ||
| 💬 Message: Token revoked - run `opencode auth login` | ||
| 📊 Status: 401 | ||
| 🤖 Model: claude-sonnet-4-20250514 | ||
| 📋 Remaining: 2 account(s) | ||
| 🕐 Time: 2025-02-08T10:30:00.000Z | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block.
The static analysis tool (markdownlint MD040) flags this code block for missing a language specifier. Since this is a plain-text notification example, use text as the language.
Proposed fix
-```
+```text
⚠️ Account Error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| ⚠️ Account Error | |
| ━━━━━━━━━━━━━━━━━━━━━━━━ | |
| 📧 Account: user@gmail.com | |
| ❌ Error: invalid_grant | |
| 💬 Message: Token revoked - run `opencode auth login` | |
| 📊 Status: 401 | |
| 🤖 Model: claude-sonnet-4-20250514 | |
| 📋 Remaining: 2 account(s) | |
| 🕐 Time: 2025-02-08T10:30:00.000Z | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 252-252: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/CONFIGURATION.md` around lines 252 - 262, The fenced code block example
is missing a language identifier (triggering markdownlint MD040); update the
triple-backtick fence for that notification example to include the text language
specifier (change ``` to ```text) so the block is treated as plain text and the
linter warning is resolved.
…and notification delivery, fix notification module import, and add `prepare` build script.
…aching, quotas, and notification settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@assets/antigravity.schema.json`:
- Around line 319-328: The schema documents an environment override
OPENCODE_ANTIGRAVITY_TELEGRAM_BOT_TOKEN for the secret field telegram_bot_token
but applyEnvOverrides() in src/plugin/config/loader.ts never reads that env var;
update applyEnvOverrides() to check
process.env.OPENCODE_ANTIGRAVITY_TELEGRAM_BOT_TOKEN and, if present, set
config.telegram_bot_token to that value (mirroring how other flags like
quiet_mode/debug/log_dir are handled), ensuring the override takes precedence
over file values; alternatively, remove the env-var mention from the schema if
you prefer not to support runtime overrides.
🧹 Nitpick comments (2)
package.json (1)
44-47: Redundant build on publish:prepareandprepublishOnlyboth runnpm run build.
preparefires beforenpm publish(and onnpm installfrom git), whileprepublishOnlyfires only before publish. Duringnpm publish, the build will execute twice. Consider removingprepublishOnlynow thatpreparecovers both scenarios.Proposed fix
- "prepublishOnly": "npm run build", "test:e2e:models": "npx tsx script/test-models.ts", "test:e2e:regression": "npx tsx script/test-regression.ts", "prepare": "npm run build"assets/antigravity.schema.json (1)
13-20: Missingdescriptionfields on new properties.Most existing properties include a
descriptionfor documentation and IDE autocompletion hints. The newly added properties (toast_scope,scheduling_mode,max_cache_first_wait_seconds,failure_ttl_seconds,request_jitter_max_ms,soft_quota_threshold_percent,quota_refresh_interval_minutes,soft_quota_cache_ttl_minutes) all lack descriptions, which is inconsistent with the rest of the schema.Also applies to: 163-183, 196-227
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
assets/antigravity.schema.jsonpackage.jsonsrc/plugin.ts
🔇 Additional comments (2)
assets/antigravity.schema.json (2)
214-227:soft_quota_cache_ttl_minutesZod schema correctly matches JSON Schema.The Zod schema in
src/plugin/config/schema.ts(lines 391–394) properly implements the union type pattern:z.union([z.literal("auto"), z.number().min(1).max(120)])with a default of"auto". This correctly mirrors the JSON Schema'sanyOfstructure. No changes needed.
319-322:notify_on_account_errordefaults totrue— this is intentional and properly handled.The notification system correctly implements graceful degradation. When
notify_on_account_erroris enabled:
- Toast notifications are always sent (when a client is available and not in quiet mode)
- Telegram notifications are only sent if both
telegram_bot_tokenandtelegram_chat_idare configured- Missing Telegram credentials do not cause errors — the code uses optional chaining and conditional checks to safely skip Telegram if not configured
- Telegram failures are caught asynchronously and logged without blocking the notification flow
The default of
trueis a sound design choice that provides notifications out of the box while gracefully handling missing optional Telegram configuration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "notify_on_account_error": { | ||
| "default": true, | ||
| "type": "boolean" | ||
| }, | ||
| "telegram_bot_token": { | ||
| "type": "string" | ||
| }, | ||
| "telegram_chat_id": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'telegram_bot_token|TELEGRAM_BOT_TOKEN' --type=ts -C3Repository: NoeFabris/opencode-antigravity-auth
Length of output: 3107
🏁 Script executed:
# Look at the config schema to understand how env-vars are handled
fd -e ts config | head -20Repository: NoeFabris/opencode-antigravity-auth
Length of output: 95
🏁 Script executed:
# Search for how env-vars are processed in config loading
rg -n 'OPENCODE_ANTIGRAVITY|process.env' --type=ts src/plugin/config -A2 -B2Repository: NoeFabris/opencode-antigravity-auth
Length of output: 8458
🏁 Script executed:
sed -n '116,250p' src/plugin/config/loader.tsRepository: NoeFabris/opencode-antigravity-auth
Length of output: 3913
Environment variable override for telegram_bot_token is documented but not implemented.
telegram_bot_token is a secret credential. The schema (line 446 in src/plugin/config/schema.ts) documents an env-var override OPENCODE_ANTIGRAVITY_TELEGRAM_BOT_TOKEN, but the applyEnvOverrides() function in src/plugin/config/loader.ts does not actually implement this override. The token is currently loaded only from config files, forcing users to store it in plaintext on disk.
Implement the env-var override in applyEnvOverrides() to allow users to set the token via environment variable (consistent with other config fields like quiet_mode, debug, log_dir, etc.), or remove the false documentation from the schema.
🤖 Prompt for AI Agents
In `@assets/antigravity.schema.json` around lines 319 - 328, The schema documents
an environment override OPENCODE_ANTIGRAVITY_TELEGRAM_BOT_TOKEN for the secret
field telegram_bot_token but applyEnvOverrides() in src/plugin/config/loader.ts
never reads that env var; update applyEnvOverrides() to check
process.env.OPENCODE_ANTIGRAVITY_TELEGRAM_BOT_TOKEN and, if present, set
config.telegram_bot_token to that value (mirroring how other flags like
quiet_mode/debug/log_dir are handled), ensuring the override takes precedence
over file values; alternatively, remove the env-var mention from the schema if
you prefer not to support runtime overrides.
| // Inject Debug Info | ||
| if (errorBody?.error) { | ||
| const debugInfo = `\n\n[Debug Info]\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; | ||
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`; |
There was a problem hiding this comment.
Duplicate Account: field - appears both at start and inside toolDebugPayload ternary
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`; | |
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/request.ts
Line: 1581:1581
Comment:
Duplicate `Account:` field - appears both at start and inside `toolDebugPayload` ternary
```suggestion
const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`;
```
How can I resolve this? If you propose a fix, please make it concise.| return transformAntigravityResponse( | ||
| lastFailure.response, | ||
| lastFailure.streaming, | ||
| lastFailure.debugContext, | ||
| lastFailure.requestedModel, | ||
| lastFailure.projectId, | ||
| lastFailure.endpoint, | ||
| lastFailure.effectiveModel, | ||
| lastFailure.sessionId, | ||
| lastFailure.toolDebugMissing, | ||
| lastFailure.toolDebugSummary, | ||
| lastFailure.toolDebugPayload, | ||
| debugLines, | ||
| ); |
There was a problem hiding this comment.
Missing account parameter - debug output will show "Account: Unknown"
The transformAntigravityResponse signature includes an optional account parameter, but it's not being passed here. Add the account identifier (available in loop scope) to provide proper debug context.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 2082:2095
Comment:
Missing `account` parameter - debug output will show "Account: Unknown"
The `transformAntigravityResponse` signature includes an optional `account` parameter, but it's not being passed here. Add the account identifier (available in loop scope) to provide proper debug context.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| return transformAntigravityResponse( | ||
| lastFailure.response, | ||
| lastFailure.streaming, | ||
| lastFailure.debugContext, | ||
| lastFailure.requestedModel, | ||
| lastFailure.projectId, | ||
| lastFailure.endpoint, | ||
| lastFailure.effectiveModel, | ||
| lastFailure.sessionId, | ||
| lastFailure.toolDebugMissing, | ||
| lastFailure.toolDebugSummary, | ||
| lastFailure.toolDebugPayload, | ||
| debugLines, | ||
| ); |
There was a problem hiding this comment.
Missing account parameter - debug output will show "Account: Unknown"
Same as above: the account parameter should be passed to preserve which account produced the error for debugging.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 2106:2119
Comment:
Missing `account` parameter - debug output will show "Account: Unknown"
Same as above: the `account` parameter should be passed to preserve which account produced the error for debugging.
How can I resolve this? If you propose a fix, please make it concise.| type FailureContext = { | ||
| response: Response; | ||
| streaming: boolean; | ||
| debugContext: ReturnType<typeof startAntigravityDebugRequest>; | ||
| requestedModel?: string; | ||
| projectId?: string; | ||
| endpoint?: string; | ||
| effectiveModel?: string; | ||
| sessionId?: string; | ||
| toolDebugMissing?: number; | ||
| toolDebugSummary?: string; | ||
| toolDebugPayload?: string; | ||
| }; |
There was a problem hiding this comment.
Consider adding account field to capture which account produced the failure
Adding an account?: string field to FailureContext would allow passing the account identifier through to error handlers, avoiding "Account: Unknown" in debug output when lastFailure is used.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin.ts
Line: 1043:1055
Comment:
Consider adding `account` field to capture which account produced the failure
Adding an `account?: string` field to `FailureContext` would allow passing the account identifier through to error handlers, avoiding "Account: Unknown" in debug output when `lastFailure` is used.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| async syncEnabledStatusFromDisk(): Promise<void> { | ||
| try { | ||
| const stored = await loadAccounts(); | ||
| if (!stored) return; | ||
|
|
||
| const enabledRefreshTokens = new Set( | ||
| stored.accounts | ||
| .filter((a) => a.enabled !== false) | ||
| .map((a) => a.refreshToken) | ||
| ); | ||
|
|
||
| for (const account of this.accounts) { | ||
| account.enabled = enabledRefreshTokens.has(account.parts.refreshToken); | ||
| } | ||
| } catch (error) { | ||
| // Ignore errors reading from disk, fallback to in-memory state | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused method - never called in codebase
syncEnabledStatusFromDisk() is defined but has no call sites. If this is for future use, add a TODO comment. If it should be used now, add the call site.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/accounts.ts
Line: 899:916
Comment:
Unused method - never called in codebase
`syncEnabledStatusFromDisk()` is defined but has no call sites. If this is for future use, add a TODO comment. If it should be used now, add the call site.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/notifications.ts`:
- Around line 104-121: shouldNotify currently records the cooldown
(notificationCooldowns.set) before any sends, which causes failed deliveries to
still consume the cooldown; update the flow so shouldNotify only reads cooldowns
and does NOT set them, and move the notificationCooldowns.set(key, now) call
into notifyAccountError (or into the success path of the actual send functions)
so the cooldown is stamped only after at least one channel succeeds;
alternatively, if pre-stamping was intended, add a clear comment in shouldNotify
explaining that failed sends should still suppress retries for the cooldown
window; reference the functions/vars: shouldNotify, notifyAccountError,
notificationCooldowns, cleanupCooldowns and the key
`${notification.errorType}:${notification.accountIndex}` when making the change.
- Around line 230-235: The log call publishing PII uses log.info with
notification.accountEmail; change this to avoid logging raw emails in production
by either removing accountEmail from the payload passed to
log.info("account-error-notification") or moving the email into a separate
debug-level call (e.g., log.debug("account-error-notification-detail", {
accountIndex, accountEmail })) and keep the info-level entry limited to non-PII
fields (accountIndex, errorType, remainingAccounts) — update the call site that
constructs the object (referenced by log.info("account-error-notification") and
notification.accountEmail) to implement redaction or debug-level logging
accordingly.
🧹 Nitpick comments (2)
src/notifications.ts (2)
169-172:NonNullabledoesn't make the inner fields required —botTokenandchatIdremain optional.
NonNullable<NotificationConfig["telegram"]>only stripsundefinedfrom the outer union, yielding{ botToken?: string; chatId?: string; }. A caller could invokesendTelegramMessage({ botToken: undefined, chatId: undefined }, ...)and the URL would becomehttps://api.telegram.org/botundefined/sendMessage. The call site innotifyAccountErrorguards this at runtime (line 257), but the exported function's type doesn't enforce it.🔧 Require both fields in the function signature
export async function sendTelegramMessage( - config: NonNullable<NotificationConfig["telegram"]>, + config: { botToken: string; chatId: string }, notification: AccountErrorNotification ): Promise<boolean> {
215-216:clientis typed asany— consider a minimal interface.Typing
clientasanydisables all compile-time checks on theclient.tui.showToast(...)call path (line 241). A narrow interface would catch shape mismatches early and document the contract.🛠️ Example minimal interface
+interface NotificationClient { + tui: { + showToast(opts: { + body: { title: string; message: string; variant: string; durationMs: number }; + }): Promise<void>; + }; +} + export async function notifyAccountError( - client: any, + client: NotificationClient | null, config: NotificationConfig, notification: AccountErrorNotification ): Promise<void> {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/notifications.tssrc/plugin.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/notifications.ts (1)
src/plugin/logger.ts (1)
createLogger(64-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (4)
src/notifications.ts (4)
1-9: LGTM!Clean module setup with scoped logger.
11-56: LGTM!Well-documented interface with appropriate optional fields and truncation-friendly string types.
83-99: LGTM!Bounded cooldown map with safe cleanup during iteration. The
cooldownMs * 2stale threshold is reasonable.
267-279: LGTM!Returning a defensive copy from
getNotificationCooldownStateis the right call.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/notifications.ts
Outdated
| function shouldNotify(notification: AccountErrorNotification, cooldownMs: number): boolean { | ||
| if (cooldownMs <= 0) return true; | ||
|
|
||
| cleanupCooldowns(cooldownMs); | ||
|
|
||
| // Create unique key: error type + account index | ||
| const key = `${notification.errorType}:${notification.accountIndex}`; | ||
| const lastNotified = notificationCooldowns.get(key) ?? 0; | ||
| const now = Date.now(); | ||
|
|
||
| if (now - lastNotified < cooldownMs) { | ||
| log.debug("notification-cooldown", { key, remainingMs: cooldownMs - (now - lastNotified) }); | ||
| return false; | ||
| } | ||
|
|
||
| notificationCooldowns.set(key, now); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Cooldown is recorded before notifications are dispatched — failed sends consume the cooldown window.
shouldNotify stamps the cooldown at line 119, but the actual toast/Telegram sends happen afterward in notifyAccountError. If both delivery channels fail, the error is silently suppressed for the entire cooldown period. If this is intentional (to avoid spam on persistent failures), a brief comment would clarify the design choice. Otherwise, consider stamping after at least one channel succeeds.
🤖 Prompt for AI Agents
In `@src/notifications.ts` around lines 104 - 121, shouldNotify currently records
the cooldown (notificationCooldowns.set) before any sends, which causes failed
deliveries to still consume the cooldown; update the flow so shouldNotify only
reads cooldowns and does NOT set them, and move the
notificationCooldowns.set(key, now) call into notifyAccountError (or into the
success path of the actual send functions) so the cooldown is stamped only after
at least one channel succeeds; alternatively, if pre-stamping was intended, add
a clear comment in shouldNotify explaining that failed sends should still
suppress retries for the cooldown window; reference the functions/vars:
shouldNotify, notifyAccountError, notificationCooldowns, cleanupCooldowns and
the key `${notification.errorType}:${notification.accountIndex}` when making the
change.
| // Inject Debug Info | ||
| if (errorBody?.error) { | ||
| const debugInfo = `\n\n[Debug Info]\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; | ||
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`; |
There was a problem hiding this comment.
duplicate Account: field - already shown at start of debugInfo, remove from end of toolDebugPayload ternary
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}\nAccount: ${account || "Unknown"}` : ""}`; | |
| const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/request.ts
Line: 1581:1581
Comment:
duplicate `Account:` field - already shown at start of debugInfo, remove from end of toolDebugPayload ternary
```suggestion
const debugInfo = `\n\n[Debug Info]\nAccount: ${account || "Unknown"}\nRequested Model: ${requestedModel || "Unknown"}\nEffective Model: ${effectiveModel || "Unknown"}\nProject: ${projectId || "Unknown"}\nEndpoint: ${endpoint || "Unknown"}\nStatus: ${response.status}\nRequest ID: ${headers.get("x-request-id") || "N/A"}${toolDebugMissing !== undefined ? `\nTool Debug Missing: ${toolDebugMissing}` : ""}${toolDebugSummary ? `\nTool Debug Summary: ${toolDebugSummary}` : ""}${toolDebugPayload ? `\nTool Debug Payload: ${toolDebugPayload}` : ""}`;
```
How can I resolve this? If you propose a fix, please make it concise.…ts into plugin.ts.
| @@ -1577,9 +1578,9 @@ export async function transformAntigravityResponse( | |||
|
|
|||
| // Inject Debug Info | |||
| if (errorBody?.error) { | |||
There was a problem hiding this comment.
duplicate Account: field in debug output - already shown at start of debugInfo, remove second occurrence at end of toolDebugPayload ternary
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin/request.ts
Line: 1580:1580
Comment:
duplicate `Account:` field in debug output - already shown at start of debugInfo, remove second occurrence at end of toolDebugPayload ternary
How can I resolve this? If you propose a fix, please make it concise.
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
|
Not in scope, add complexity and doesn't provide any benefit to the user. And please next time please open a PR properly, most of the code changes are just formatting and there is not frikin description... |
2 participants
No description provided.