fix: improve blocked-account verification UX and recovery flow#395
fix: improve blocked-account verification UX and recovery flow#395ndycode wants to merge 21 commits intoNoeFabris:devfrom
Conversation
WalkthroughAdds persistent blocked-account storage (load/save/clear) with locking and atomic writes plus BlockedAccount types and path helpers. Enhances accounts/storage save to support merging, added/removed refresh tokens, preservation of deletions, and fingerprintHistory. Extends AccountManager with quarantine, fingerprint history methods, and reloadFromDisk. Reworks quota checking to a concurrent worker pool with timeouts and progress callbacks; introduces QuotaGroup/QuotaGroupSummary. Changes request handling to strip Claude thinking blocks, disable Claude thinking warmups/caching, and emit single Antigravity-style synthetic SSE error payloads. UI updates: verify flow, ANSI-aware windowed select, and revised auth menu. package.json exports and .gitignore entries updated; many tests adjusted. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration 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)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting 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 introduces a comprehensive blocked-account verification flow to help users recover from Google account verification blocks. The changes successfully address multiple issues from previous review threads. Key improvementsNew verification flow
Fixed previous review issues
Quarantine system
URL normalization
Minor issues
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as opencode auth login
participant Menu as Auth Menu
participant Scanner as Account Scanner
participant Storage as Blocked Storage
participant Google as Google APIs
User->>CLI: opencode auth login
CLI->>Menu: Show menu with "Verify blocked accounts"
User->>Menu: Select "Verify blocked accounts"
Menu->>Storage: loadBlockedAccounts()
Storage-->>Menu: quarantined accounts
Menu->>Scanner: scanAllAccountsForVerification(active)
loop For each active account
Scanner->>Google: POST streamGenerateContent (ping)
alt 200 OK
Google-->>Scanner: success
Scanner->>Scanner: mark as "ok"
else 403 VALIDATION_REQUIRED
Google-->>Scanner: error + validation_url
Scanner->>Scanner: extract validation URL
Scanner->>Scanner: mark as "blocked"
else 403 license error
Google-->>Scanner: error #3501
Scanner->>Scanner: mark as "license"
else 429 rate-limited
Google-->>Scanner: 429
Scanner->>Scanner: mark as "error"
end
end
Menu->>Scanner: scanAllAccountsForVerification(quarantined)
Scanner-->>Menu: scan results
alt Any quarantined accounts now OK
Menu->>Storage: restore to active accounts
Menu->>Storage: saveAccounts() + saveBlockedAccounts()
end
Menu->>User: Display results summary
alt Has blocked accounts
User->>Menu: Select blocked account
Menu->>Menu: Show action menu
User->>Menu: Choose action (open/copy URL)
alt Open sign-in page
Menu->>Google: buildGoogleSigninFirstUrl()
Menu->>Menu: openUrlInDefaultBrowser()
else Copy verification link
Menu->>Menu: copyTextToClipboard()
end
User->>Google: Complete verification (phone/QR)
Google-->>User: Auth success
User->>Menu: Run scan again
Menu->>Scanner: scanAllAccountsForVerification()
Scanner->>Google: Test account
Google-->>Scanner: 200 OK
Menu->>Storage: Auto-restore to active rotation
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/storage.ts
Line: 560:590
Comment:
**Deletion preservation disabled**
`saveAccounts()` documents `preserveDeletions` as defaulting to true, but the implementation only enables it when `options.preserveDeletions === true`. Any call sites relying on the default will *not* prune stale incoming tokens, so a concurrent/stale writer can resurrect deleted accounts despite the new merge logic. Make this actually default to true (e.g. `options.preserveDeletions !== false`) or update all call sites to pass `preserveDeletions: true` explicitly.
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/plugin/quota.ts (1)
244-295:⚠️ Potential issue | 🟡 MinorStatus field and disabled flag are inconsistent; disabled accounts unnecessarily fetch quotas.
The type definition includes
status: "disabled", but the code returnsstatus: "ok"withdisabled: true. While downstream consumers (e.g.,badgeOf,summaryCounts) correctly check thedisabledflag, disabled accounts still execute full token refresh and quota fetches. Either short-circuit disabled accounts early or returnstatus: "disabled"to align with the type definition and avoid unnecessary network calls.src/plugin/accounts.ts (1)
345-369:⚠️ Potential issue | 🟠 MajorAuth-fallback merge block is unreachable.
Because the stored-accounts branch returns before this block, the fallback token is never appended. This defeats the intended “always include current auth” behavior.🛠️ Suggested fix
- return; - } - - // If we have stored accounts, check if we need to add the current auth - if (authFallback && this.accounts.length > 0) { - const authParts = parseRefreshParts(authFallback.refresh); - const hasMatching = this.accounts.some(acc => acc.parts.refreshToken === authParts.refreshToken); - if (!hasMatching && authParts.refreshToken) { - const now = nowMs(); - const newAccount: ManagedAccount = { - index: this.accounts.length, - email: undefined, - addedAt: now, - lastUsed: 0, - parts: authParts, - access: authFallback.access, - expires: authFallback.expires, - enabled: true, - rateLimitResetTimes: {}, - touchedForQuota: {}, - }; - this.accounts.push(newAccount); - this.pendingAddedRefreshTokens.add(authParts.refreshToken); - this.requestSaveToDisk(); - // Update indices to include the new account - this.currentAccountIndexByFamily.claude = Math.min(this.currentAccountIndexByFamily.claude, this.accounts.length - 1); - this.currentAccountIndexByFamily.gemini = Math.min(this.currentAccountIndexByFamily.gemini, this.accounts.length - 1); - } - } + // If we have stored accounts, check if we need to add the current auth + if (authFallback && this.accounts.length > 0) { + const authParts = parseRefreshParts(authFallback.refresh); + const hasMatching = this.accounts.some(acc => acc.parts.refreshToken === authParts.refreshToken); + if (!hasMatching && authParts.refreshToken) { + const now = nowMs(); + const newAccount: ManagedAccount = { + index: this.accounts.length, + email: undefined, + addedAt: now, + lastUsed: 0, + parts: authParts, + access: authFallback.access, + expires: authFallback.expires, + enabled: true, + rateLimitResetTimes: {}, + touchedForQuota: {}, + }; + this.accounts.push(newAccount); + this.pendingAddedRefreshTokens.add(authParts.refreshToken); + this.requestSaveToDisk(); + // Update indices to include the new account + this.currentAccountIndexByFamily.claude = Math.min(this.currentAccountIndexByFamily.claude, this.accounts.length - 1); + this.currentAccountIndexByFamily.gemini = Math.min(this.currentAccountIndexByFamily.gemini, this.accounts.length - 1); + } + } + return; + }
🤖 Fix all issues with AI agents
In `@src/plugin.ts`:
- Around line 613-621: The helper openUrlInDefaultBrowser currently runs a
Windows-only exec and is effectively a no-op on other platforms while the UI
always reports success; update openUrlInDefaultBrowser to delegate to the
existing cross-platform openBrowser(url) for non-Windows platforms (or always
use openBrowser if it already handles Windows) and return or surface
success/failure so the caller can conditionally print "Opened…". Specifically,
modify the function openUrlInDefaultBrowser to call openBrowser(url) and
propagate its result/throw on error, or at minimum detect platform
(process.platform === "win32") and only run the cmd exec on Windows while
calling openBrowser(url) on other platforms, and ensure the catch path returns a
falsy/error outcome so the UI prints success only when the operation actually
succeeded.
In `@src/plugin/storage.ts`:
- Around line 574-606: The preserveDeletions behavior in saveAccounts is
inverted: change how preserveDeletions is computed so its default is true
(matching the doc) by using options.preserveDeletions !== false instead of
options.preserveDeletions === true; update the local variable in the
saveAccounts function (where preserveDeletions is declared) to this expression
so callers that omit the option will preserve deletions by default.
In `@src/plugin/ui/select.ts`:
- Around line 28-52: Replace the regex literals that contain the control char
\x1b with RegExp constructions: create ANSI_REGEX via new
RegExp('\\x1b\\[[0-9;]*m','g') (or equivalent escaped string) and replace the
inline /^\x1b\[[0-9;]*m/ usage inside truncateAnsi with a RegExp constructed
from the escaped string (or create a separate ANSI_SEQ_REGEX = new
RegExp('^\\x1b\\[[0-9;]*m') and use it in truncateAnsi). Update references in
stripAnsi and the match inside truncateAnsi so both use the RegExp versions
instead of regex literals (preserving flags and behavior).
📜 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 ignored due to path filters (8)
docs/pr-369/pr1.pngis excluded by!**/*.pngdocs/pr-369/pr2.pngis excluded by!**/*.pngdocs/pr-369/pr3.pngis excluded by!**/*.pngdocs/pr-369/pr4.pngis excluded by!**/*.pngdocs/pr-369/pr5.pngis excluded by!**/*.pngdocs/pr-369/pr6.pngis excluded by!**/*.pngdocs/pr-369/pr7.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.gitignorepackage.jsonsrc/plugin.tssrc/plugin/accounts.tssrc/plugin/cli.tssrc/plugin/quota.tssrc/plugin/request-helpers.test.tssrc/plugin/request-helpers.tssrc/plugin/request.test.tssrc/plugin/request.tssrc/plugin/storage.test.tssrc/plugin/storage.tssrc/plugin/ui/auth-menu.tssrc/plugin/ui/select.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/plugin/ui/select.ts (1)
src/plugin/ui/ansi.ts (1)
ANSI(6-25)
src/plugin/quota.ts (4)
src/plugin/storage.ts (1)
AccountMetadataV3(180-196)src/plugin/auth.ts (2)
accessTokenExpired(33-38)parseRefreshParts(12-19)src/plugin/token.ts (1)
refreshAccessToken(85-169)src/constants.ts (1)
ANTIGRAVITY_DEFAULT_PROJECT_ID(71-71)
src/plugin/ui/auth-menu.ts (3)
scripts/check-quota.mjs (2)
accounts(155-155)label(169-169)src/plugin/ui/ansi.ts (1)
ANSI(6-25)src/plugin/ui/select.ts (1)
select(76-294)
src/plugin/cli.ts (1)
src/plugin/ui/auth-menu.ts (1)
showAccountDetails(101-135)
src/plugin/request-helpers.test.ts (1)
src/plugin/request-helpers.ts (1)
createSyntheticErrorResponse(2785-2826)
src/plugin/request.ts (5)
src/constants.ts (1)
SKIP_THOUGHT_SIGNATURE(209-209)src/plugin/request-helpers.ts (3)
deepFilterThinkingBlocks(1290-1339)transformThinkingParts(1450-1502)stripThinkingFromResponse(1508-1555)src/plugin/transform/claude.ts (1)
isClaudeModel(27-29)src/plugin/debug.ts (1)
isDebugEnabled(149-151)src/plugin/config/loader.ts (1)
getKeepThinking(227-229)
src/plugin/accounts.ts (3)
src/plugin/types.ts (1)
OAuthAuthDetails(4-9)src/plugin/storage.ts (6)
loadAccounts(487-572)AccountStorageV3(198-206)BlockedAccountMetadataV1(231-236)loadBlockedAccounts(713-744)saveBlockedAccounts(746-768)saveAccounts(594-673)src/plugin/auth.ts (1)
parseRefreshParts(12-19)
src/plugin/request-helpers.ts (2)
src/plugin/transform/index.ts (1)
isClaudeModel(35-35)src/plugin/transform/claude.ts (1)
isClaudeModel(27-29)
🪛 Biome (2.3.13)
src/plugin/ui/select.ts
[error] 28-28: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 50-50: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (34)
.gitignore (1)
8-16: LGTM — generated artifacts are ignored.
The new ignore rules cover build outputs and generated typings without impacting source files.package.json (1)
7-13: Confirm downstream deep-import compatibility with the new exports map.
Adding"exports"restricts consumers to the root entry; any deep imports (e.g.,dist/*orpackage.json) will now fail unless explicitly exported. Please confirm downstream usage or add subpath exports if needed.src/plugin/request-helpers.test.ts (1)
1565-1618: LGTM — SSE tests align with the new Antigravity payload.
Assertions now match the singledata:response shape and keep coverage relevant.src/plugin/request-helpers.ts (3)
1105-1110: LGTM — Claude requests now avoid replaying thinking blocks.
The unconditional strip for Claude aligns with non‑replayable signatures.
1504-1555: Nice addition: response‑side thinking strip helper.
Keeps Claude responses clean and avoids leaking reasoning.
2785-2824: LGTM — synthetic error SSE payload is OpenCode‑compatible.
Single payload plus error‑type header matches the updated response expectations.src/plugin/request.ts (8)
105-112: LGTM — signature caching limited to Gemini 3.
Avoids caching Claude signatures that cannot be replayed.
359-396: LGTM — sentinel signatures consistently enforced.
Overwriting existing thinking signatures prevents replay errors across turns.
418-555: Good safeguard around tool‑use thinking blocks.
Stripping or sentinel‑injecting on cache misses keeps tool flows resilient without replaying invalid signatures.
715-743: LGTM — Claude wrapped payloads are sanitized early.
Deep filtering and disabling warmup here aligns with non‑replayable thinking signatures.
1159-1163: LGTM — warmup disabled for Claude in unwrapped requests.
Consistent with the wrapped‑body path.
1468-1510: Nice: Claude thinking stripped in streaming transform.
The streaming transformer now cleans reasoning parts while keeping other content intact.
1528-1568: LGTM — raw error classification before debug injection.
Using the upstream message for recovery detection avoids false matches.
1662-1665: LGTM — non‑streaming responses also strip Claude thinking.
Keeps behavior consistent between streaming and non‑streaming paths.src/plugin/request.test.ts (1)
231-284: LGTM — tests cover sentinel signature behavior.
The updated assertions align with the new sentinel‑based thinking signatures.src/plugin/storage.ts (4)
21-29: Good: blocked-accounts artifacts are gitignored.
Keeps new sensitive storage out of repos.
192-195: Nice: fingerprint history is now persisted.
This aligns storage with the new fingerprint management APIs.
227-311: Blocked-accounts storage plumbing looks solid.
Separate path + lock + atomic writes should keep quarantine data consistent.Also applies to: 713-780
608-656: Merge + pruning + index clamping is robust.
Good to see stale token filtering and index normalization before persist.src/plugin/storage.test.ts (1)
468-485: Tests updated to cover blocked-account gitignore entries.
Nice alignment with the new storage artifacts.Also applies to: 533-551
src/plugin/accounts.ts (5)
1-13: Storage/blocked-account wiring is clear.
Imports + pending token sets make the multi-process save flow explicit.Also applies to: 252-270, 278-278
372-392: Good: fallback-only account is persisted immediately.
The pending-added token bookkeeping is consistent with the new merge flow.
320-324: Fingerprint history is carried through load/reload/save.
Nice end‑to‑end persistence and explicitenabledserialization.Also applies to: 848-853, 932-948
696-700: Pending add/remove tokens are tracked and flushed cleanly.
This should prevent cross‑process resurrection.Also applies to: 956-971
800-926: Reload + quarantine flows are thoughtfully wired.
Resets session-only state and keeps quarantine persistence best-effort.src/plugin/cli.ts (1)
32-126: Verify mode wiring looks consistent.
Fallback and TTY flows both expose and handle the new option.src/plugin/ui/auth-menu.ts (1)
17-121: Menu restructuring reads great.
Headings, separators, and status badges make navigation clearer.src/plugin/ui/select.ts (1)
9-24: Heading support + windowed rendering is a nice UX lift.
The new selection model and truncation safeguards look solid.Also applies to: 88-193, 231-237
src/plugin.ts (6)
1-66: Imports updated cleanly for the new verification flow.
Keeps the dependency surface explicit.
87-612: Verification URL normalization + scan helpers are well-structured.
Good separation of parsing, normalization, and scan orchestration.
1458-1607: Good resilience around stale storage + zero-account reload.
Clearing blocked storage on invalid auth and reloading when empty reduces stuck states.
1970-2357: Rate-limit, license, and validation handling is well thought out.
Avoiding endpoint retries on 403 and quarantining validation-required accounts is a solid UX improvement.
2564-3262: Verify/quota flows are comprehensive and user-friendly.
Action grouping and scan/rescan controls should make recovery much smoother.
3270-3299: Toggles and delete‑all now persist blocked storage correctly.
Nice consistency across active and quarantined records.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/plugin.ts`:
- Around line 2683-2708: The quota-save path can drop a rotated refresh token
because saveAccounts is called with preserveDeletions which prunes tokens not
marked as added; modify runCheck so when iterating results it compares
existingStorage.accounts[res.index].refreshToken to
res.updatedAccount?.refreshToken and, if different, record that token change
(e.g., push into addedTokens/removedTokens lists or a tokenChange map keyed by
account index), update existingStorage.accounts[res.index] = res.updatedAccount,
set needsSave = true, and then call saveAccounts(existingStorage, { merge: true,
preserveDeletions: true, tokenChanges: { added: addedTokens, removed:
removedTokens } }) or the equivalent API your saveAccounts supports so the new
refresh token is treated as added and not pruned; reference runCheck,
checkAccountsQuota, existingStorage.accounts, res.updatedAccount.refreshToken,
and saveAccounts when making the change.
In `@src/plugin/accounts.ts`:
- Around line 803-812: reloadFromDisk currently resets sessionOffsetApplied and
toast state but does not clear in-memory "pending add/remove" token diffs,
causing stale deltas to be applied on subsequent saves; inside reloadFromDisk
(after loading stored = await loadAccounts() and after resetting
sessionOffsetApplied/lastToast*), explicitly clear the pending token diff state
(e.g. set this.pendingTokenAdds = empty, this.pendingTokenRemoves = empty, and
this.pendingTokenDiffs = empty/null) so any previous in-memory add/remove deltas
are dropped after a disk reload.
📜 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 (5)
src/plugin.tssrc/plugin/accounts.tssrc/plugin/quota.tssrc/plugin/storage.tssrc/plugin/ui/select.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugin/accounts.ts (4)
src/plugin/types.ts (1)
OAuthAuthDetails(4-9)src/plugin/storage.ts (6)
loadAccounts(487-572)AccountStorageV3(198-206)BlockedAccountMetadataV1(231-236)loadBlockedAccounts(713-744)saveBlockedAccounts(746-768)saveAccounts(594-673)src/plugin/refresh-queue.ts (1)
refreshToken(209-224)src/plugin/auth.ts (1)
parseRefreshParts(12-19)
src/plugin/ui/select.ts (1)
src/plugin/ui/ansi.ts (1)
ANSI(6-25)
🪛 ast-grep (0.40.5)
src/plugin/ui/select.ts
[warning] 28-28: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(ANSI_ESCAPE_PATTERN, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 29-29: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${ANSI_ESCAPE_PATTERN})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (23)
src/plugin/quota.ts (3)
41-45: Clean options surface for quota checks.
The new options shape (concurrency, timeout, progress callback) is well-scoped and intuitive.
156-165: Timeout wrapper is solid.
The guard correctly handles invalid values and ensures timers are cleared.
230-335: Worker-pool refactor looks good.
Order-preserving results + progress callbacks are implemented cleanly.src/plugin/ui/select.ts (5)
9-23: MenuItem/SelectOptions extensions are well integrated.
Headings, help text, and clearScreen options fit naturally with existing API usage.
28-66: ANSI-safe truncation utilities are solid.
Nice balance of visibility math while preserving escape sequences.
90-104: Selectable filtering + cursor init is correct.
Heading/separator handling is consistent with the new item kinds.
108-195: Windowed rendering logic looks robust.
The viewport calculations and truncation should significantly reduce wrap jitter.
233-240: Navigation correctly skips headings now.
Good alignment with the new “non-selectable” semantics.src/plugin.ts (8)
87-117: Validation/license summarizers are crisp.
The message trimming and link-stripping keep user-facing errors concise.
121-247: Google URL normalization looks thorough.
Good handling of accountchooser/signin flows and malformed authuser keys.
286-401: Validation URL extraction is comprehensive.
SSE parsing + multi-source URL collection is a solid approach.
403-536: Scan request path is well designed.
Using streaming generateContent for reliable VALIDATION_REQUIRED detection makes sense.
554-611: Concurrent scan pipeline looks good.
The timeout + progress tracking logic is clean and easy to follow.
613-632: Browser/clipboard helpers fit the flow.
Returning success flags gives the caller clear control over UX messaging.
1594-1601: Reload-on-empty is a good resilience tweak.
This should reduce “restart required” friction after external fixes.
2853-3267: Blocked-account verification flow is well structured.
The scanning, ranking, and per-account actions feel consistent and actionable.src/plugin/accounts.ts (3)
330-354: Auth fallback merge is a nice safety net.
Good resilience for cross-process OAuth changes.
880-929: Quarantine persistence path looks solid.
Best-effort persistence is appropriate for this request path.
959-974: Added/removed refresh token tracking is a good guardrail.
This should reduce cross-process resurrection issues.src/plugin/storage.ts (4)
25-27: Gitignore updates are appropriate.
The blocked-accounts files are correctly treated as sensitive state.
227-311: Blocked-accounts storage wiring looks good.
Path, file init, and lock usage mirror the existing accounts flow well.
594-672: saveAccounts merge semantics are consistent.
The default preserve-deletions behavior is now safe and predictable.
713-780: Blocked accounts load/save/clear flow is clean.
Good defensive parsing and atomic write handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin.ts
Line: 743:751
Comment:
**Shell injection via exec**
`openBrowser()` builds a shell command string with an untrusted `url` interpolated and passes it to `exec()` (e.g. ``exec(`open "${url}"`)``) (`src/plugin.ts:743-751`). If `url` contains quotes/backticks/`$()` etc (even accidentally via upstream parsing), this becomes command injection. Use `execFile`/`spawn` with args (no shell) or properly escape/sanitize the URL before execution.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/plugin/request.ts (1)
1607-1624:⚠️ Potential issue | 🟡 MinorMove retry-header extraction before the early return to make it reachable.
The
if (errorBody?.error)block returns at line 1600, preventing control from ever reaching theif (errorBody?.error?.details)block at line 1607. SinceerrorBody?.error?.detailsrequireserrorBody.errorto exist, and any truthyerrorBody.errortriggers the earlier return, the retry-info parsing is dead code. Move the retry-header extraction inside the first if-block before the return statement.Proposed fix
if (errorBody?.error) { // ... rawMessage extraction, detectErrorType, debug info ... + // Extract Retry-After from error details before returning + if (errorBody.error.details && Array.isArray(errorBody.error.details)) { + const retryInfo = errorBody.error.details.find( + (detail: any) => detail['@type'] === 'type.googleapis.com/google.rpc.RetryInfo' + ); + if (retryInfo?.retryDelay) { + const match = retryInfo.retryDelay.match(/^([\d.]+)s$/); + if (match && match[1]) { + const retrySeconds = parseFloat(match[1]); + if (!isNaN(retrySeconds) && retrySeconds > 0) { + const retryAfterSec = Math.ceil(retrySeconds).toString(); + const retryAfterMs = Math.ceil(retrySeconds * 1000).toString(); + headers.set('Retry-After', retryAfterSec); + headers.set('retry-after-ms', retryAfterMs); + } + } + } + } + return new Response(JSON.stringify(errorBody), { status: response.status, statusText: response.statusText, headers }); } - - if (errorBody?.error?.details && Array.isArray(errorBody.error.details)) { - const retryInfo = errorBody.error.details.find( - (detail: any) => detail['@type'] === 'type.googleapis.com/google.rpc.RetryInfo' - ); - // ... rest of retry-info extraction ... - }src/plugin/ui/auth-menu.ts (1)
104-121:⚠️ Potential issue | 🟡 MinorAvoid duplicated “Account N” in the details header.
Line 120 prefixesAccount {index+1}:even whenheaderalready contains the fallback label (Account {index+1}), so accounts without email render like “Account 3: Account 3 …”. Consider using the header directly when no email is present.🔧 Suggested fix
const label = account.email || `Account ${account.index + 1}`; const badge = getStatusBadge(account.status); const disabledBadge = account.enabled === false ? ` ${ANSI.red}[disabled]${ANSI.reset}` : ''; const header = `${label}${badge ? ' ' + badge : ''}${disabledBadge}`; + const messageHeader = account.email + ? `Account ${account.index + 1}: ${header}` + : header; const subtitleParts = [ `Added: ${formatDate(account.addedAt)}`, `Last used: ${formatRelativeTime(account.lastUsed)}`, ]; @@ - message: `Account ${account.index + 1}: ${header}`, + message: messageHeader,
🤖 Fix all issues with AI agents
In `@src/plugin/accounts.ts`:
- Around line 508-516: The current sticky-account selection block returns the
account from getCurrentAccountForFamily(family) without verifying it's enabled,
so disabled accounts can be used; update the conditional to require
current.enabled (or !current.enabled check to skip) before calling
clearExpiredRateLimits/isRateLimitedForHeaderStyle/isAccountCoolingDown/markTouchedForQuota
and before returning current, i.e., ensure the code only proceeds and returns
when current exists AND current.enabled and passes the existing
rate-limit/cooldown checks.
🧹 Nitpick comments (3)
src/plugin/request.ts (2)
716-721: Defense-in-depth stripping is intentional but redundant.
deepFilterThinkingBlockson Line 720 mutateswrappedBodyin place, which includes the nestedrequestObjects. The subsequent call on Line 733 re-filters the same nested objects. This is harmless and justified as a safety net, but worth noting that the second pass will be a no-op after the first succeeds.
355-458: Inconsistent use ofSENTINEL_SIGNATUREvsSKIP_THOUGHT_SIGNATUREfor the same value.
SENTINEL_SIGNATURE(local, Line 358) andSKIP_THOUGHT_SIGNATURE(imported from../constants, Line 9) both resolve to"skip_thought_signature_validator". The code mixes them:
ensureThoughtSignature/ensureMessageThinkingSignature/hasSignedThinkingPart→SENTINEL_SIGNATUREensureThinkingBeforeToolUseInContents(Line 438) /ensureThinkingBeforeToolUseInMessages(Line 547, 555) /hasSignedThinkingInMessages(Line 501) →SKIP_THOUGHT_SIGNATUREIf either constant is ever updated independently, behavior silently diverges. Consider removing the local
SENTINEL_SIGNATUREand using the canonicalSKIP_THOUGHT_SIGNATUREeverywhere (or vice versa).Proposed: unify on the imported constant
-// Sentinel value used when signature recovery fails - allows Claude to handle gracefully -// by redacting the thinking block instead of rejecting the request entirely. -// Reference: LLM-API-Key-Proxy uses this pattern for Gemini 3 tool calls. -const SENTINEL_SIGNATURE = "skip_thought_signature_validator"; - function ensureThoughtSignature(part: any, _sessionId: string): any { ... - if (part.thoughtSignature === SENTINEL_SIGNATURE) { + if (part.thoughtSignature === SKIP_THOUGHT_SIGNATURE) { return part; } - return { ...part, thoughtSignature: SENTINEL_SIGNATURE }; + return { ...part, thoughtSignature: SKIP_THOUGHT_SIGNATURE }; ... }Apply the same replacement in
hasSignedThinkingPartandensureMessageThinkingSignature.src/plugin.ts (1)
3204-3211: Disable copy actions when clipboard isn’t supported.
Line 3207/3209 uses the Windows-only clipboard helper, so on macOS/Linux the “Copy …” actions always fail. Consider gating these menu items (and similar copy actions elsewhere) by platform to avoid offering options that cannot succeed.🔧 Suggested fix (gate by platform)
- const action = await select<BlockedAction>( + const canCopy = process.platform === "win32"; + const action = await select<BlockedAction>( [ { label: "Open sign-in page (prefill email)", value: { type: "openSignin" }, color: "cyan" }, - { label: "Copy sign-in page link", value: { type: "copySignin" }, color: "cyan" }, + { label: "Copy sign-in page link", value: { type: "copySignin" }, color: "cyan", disabled: !canCopy }, { label: "Open direct verification link", value: { type: "openDirect" } }, - { label: "Copy direct verification link", value: { type: "copyDirect" } }, + { label: "Copy direct verification link", value: { type: "copyDirect" }, disabled: !canCopy }, { label: "Back", value: { type: "back" } }, ],
📜 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 (7)
package.jsonsrc/plugin.tssrc/plugin/accounts.tssrc/plugin/cli.tssrc/plugin/request.tssrc/plugin/ui/auth-menu.tssrc/plugin/ui/select.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/plugin/cli.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/plugin/ui/auth-menu.ts (3)
scripts/check-quota.mjs (2)
accounts(155-155)label(169-169)src/plugin/ui/ansi.ts (1)
ANSI(6-25)src/plugin/ui/select.ts (1)
select(77-295)
src/plugin/request.ts (7)
src/constants.ts (1)
SKIP_THOUGHT_SIGNATURE(209-209)src/plugin/request-helpers.ts (3)
deepFilterThinkingBlocks(1290-1339)transformThinkingParts(1450-1502)stripThinkingFromResponse(1508-1555)src/plugin/cache.ts (1)
getCachedSignature(177-213)src/plugin/transform/claude.ts (1)
isClaudeModel(27-29)src/plugin/debug.ts (2)
isDebugEnabled(149-151)DEBUG_MESSAGE_PREFIX(11-11)src/plugin/config/loader.ts (1)
getKeepThinking(231-233)src/plugin/recovery.ts (1)
detectErrorType(86-111)
src/plugin.ts (8)
src/plugin/types.ts (2)
PluginClient(42-42)OAuthAuthDetails(4-9)src/plugin/auth.ts (4)
formatRefreshParts(24-28)accessTokenExpired(33-38)parseRefreshParts(12-19)isOAuthAuth(5-7)src/constants.ts (3)
ANTIGRAVITY_DEFAULT_PROJECT_ID(71-71)ANTIGRAVITY_ENDPOINT_PROD(34-34)ANTIGRAVITY_HEADERS(94-98)src/plugin/storage.ts (3)
loadAccounts(487-572)loadBlockedAccounts(713-744)saveAccounts(594-673)src/plugin/request.ts (1)
transformAntigravityResponse(1460-1702)src/plugin/ui/select.ts (2)
MenuItem(3-12)select(77-295)src/plugin/transform/model-resolver.ts (1)
resolveModelWithTier(162-268)src/plugin/transform/index.ts (1)
resolveModelWithTier(22-22)
src/plugin/accounts.ts (1)
src/plugin/storage.ts (6)
loadAccounts(487-572)AccountStorageV3(198-206)BlockedAccountMetadataV1(231-236)loadBlockedAccounts(713-744)saveBlockedAccounts(746-768)saveAccounts(594-673)
⏰ 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 (20)
src/plugin/request.ts (11)
106-113: LGTM — Claude signature caching correctly disabled.The restriction to Gemini-3 only is well-motivated by the comment explaining that Claude signatures are per-response and non-replayable.
360-382: LGTM — Sentinel-only signature assignment is clean.Always using the sentinel avoids stale/invalid signature reuse. The early return when already sentinel prevents unnecessary object spread.
384-400: LGTM — Sentinel recognized as valid signature.
445-459: Consistent sentinel pattern — LGTM.
740-744: LGTM — warmup correctly disabled for Claude.
1160-1164: Consistent with the wrapped path — LGTM.
1478-1490: LGTM — Synthetic placeholder correctly skipped for Claude.Claude's signed thinking blocks cannot accept synthetic placeholders, so gating on
!isClaudeis correct.
1516-1519: LGTM — Claude thinking stripped in streaming path.
1538-1539: Good fix — cloning preserves the original body for the catch fallback.Previously, if
response.text()consumed the body and a later step threw, the catch block'sreturn responsewould return a body-less response. Cloning before reading avoids this.
1550-1581: Good fix — recovery detection now uses raw upstream message.Extracting
rawMessagebefore debug injection prevents false-positive recovery triggers. Gating debug info behindisDebugEnabled()keeps user-facing errors clean.
1683-1686: Consistent Claude thinking stripping in non-streaming path — LGTM.Mirrors the streaming transform at Lines 1516–1519.
src/plugin/ui/auth-menu.ts (1)
52-90: Clear action/account grouping and badges.
The headings, separators, and status/current/disabled badges make the menu easier to scan, andclearScreenkeeps nested flows tidy.src/plugin/ui/select.ts (2)
3-24: Heading rows are correctly treated as non-selectable.
kind: 'heading'is consistently enforced in selection, cursor init, and navigation.Also applies to: 89-103, 156-160, 232-239
28-195: ANSI-safe truncation + windowed rendering look solid.
The new truncation helpers and viewport windowing should prevent wrapping and scrollback artifacts.src/plugin.ts (4)
87-401: URL normalization and validation parsing look robust.
Nice handling of malformedauthuserparams, account chooser flows, and SSE/JSON validation URL extraction.
403-611: Verification scan pipeline is well-contained.
Timeouts, concurrency caps, and status mapping keep the scan resilient and easy to reason about.
1472-1481: Good guard against stale storage when OAuth is missing.
Clearing accounts and blocked-account storage keeps state consistent with auth.
2971-2999: Auto-restore of verified quarantined accounts is a solid UX win.
Persisting restores and reporting counts gives a clean recovery path.src/plugin/accounts.ts (2)
803-814: Nice cleanup of pending token diffs on reload.
Resetting per-session state makes reload semantics predictable.
882-983: Quarantine + persistence changes look solid.
Recording blocked accounts and saving with added/removed token diffs improves multi-process consistency.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Additional Comments (3)
If the env var is still supported elsewhere, Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/storage.ts
Line: 33:44
Comment:
**Config dir regression**
`getConfigDir()` no longer honors `OPENCODE_CONFIG_DIR` (and the legacy migration was removed), so users relying on a custom config dir will silently start reading/writing accounts under the default path. This is a behavior-breaking change that can strand existing credentials.
If the env var is still supported elsewhere, `getConfigDir()` should keep the `OPENCODE_CONFIG_DIR` precedence (and optionally keep the Windows migration logic).
How can I resolve this? If you propose a fix, please make it concise.
Consider wrapping Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/quota.ts
Line: 340:356
Comment:
**Per-account timeout missing**
`checkOne()` wraps `refreshAccessToken()` with `withTimeout(...)`, but `fetchAvailableModels()` is not wrapped, so a hung network call can still block a worker indefinitely and stall `checkAccountsQuota()` (especially with small `concurrency`). The `perAccountTimeoutMs` option suggests the whole account check should be bounded.
Consider wrapping `fetchAvailableModels(...)` with `withTimeout(..., perAccountTimeoutMs, ...)` as well (or fold it into `fetchWithTimeout` consistently).
How can I resolve this? If you propose a fix, please make it concise.
Either restore the Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin.ts
Line: 1470:1486
Comment:
**toast_scope behavior lost**
This removes `toast_scope` filtering by deleting child-session detection and always showing toasts (e.g. recovery toasts and rate-limit toasts). If `toast_scope: "root_only"` is a supported config (it was referenced here before), this is a behavior regression and can reintroduce toast spam in subagents/background sessions.
Either restore the `toast_scope` gating, or remove/ignore `toast_scope` from config everywhere so behavior is consistent.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin/request.ts (1)
490-504:⚠️ Potential issue | 🟠 MajorInconsistency:
hasSignedThinkingInMessageschecksSKIP_THOUGHT_SIGNATUREbutensureMessageThinkingSignaturesetsSENTINEL_SIGNATURE.
ensureMessageThinkingSignature(Line 458) stamps blocks withSENTINEL_SIGNATURE, buthasSignedThinkingInMessages(Line 501) only recognizesSKIP_THOUGHT_SIGNATURE. This means blocks signed byensureMessageThinkingSignaturewill fail the "has signed thinking" check, leading to unnecessary re-injection via the fallback path at Lines 537–549.This is a downstream symptom of the dual-sentinel issue flagged above.
🤖 Fix all issues with AI agents
In `@src/plugin/accounts.ts`:
- Around line 1019-1068: The quarantining flow in
quarantineAccountForVerification removes the account (removeAccount) before
persisting it to blocked storage (saveBlockedAccounts), so if
saveBlockedAccounts throws the account can be lost; update the
saveBlockedAccounts try/catch to capture the thrown error, log a clear error
(using the instance logger if available, e.g. this.logger.error(...) or
console.error as a fallback) including the error and account identity
(email/refreshToken), and on failure attempt to restore the account back into
the active pool by calling the inverse operation (e.g. this.addAccount(account))
so the account isn’t lost before saveToDisk runs; keep the outer “best-effort”
behavior but ensure failures are logged and the account is re-added when
blocked-save fails.
🧹 Nitpick comments (4)
src/plugin/request.ts (1)
716-737: Redundant double-filter on nested request objects — acceptable as defense-in-depth.
deepFilterThinkingBlocksis called onwrappedBody(Line 720), which traverses intowrappedBody.request. Then it's called again on eachrequestObject(Line 733), which are the same nested objects. The second pass is a no-op for thinking blocks already stripped. Not harmful, but if you want to tighten this up, you could skip the inner call for Claude when the outer call already ran.src/plugin.ts (2)
617-632:copyTextToClipboardonly works on Windows.This helper uses
cmd /c clipwhich is a no-op (returnsfalse) on macOS and Linux. The callers handle thefalsereturn by printing to console, so it's not broken — but users on non-Windows won't get clipboard functionality. Consider addingpbcopy(macOS) andxclip/xsel(Linux) support, similar to howopenBrowserhandles cross-platform differences.♻️ Proposed cross-platform clipboard support
function copyTextToClipboard(text: string): boolean { - // Best-effort: copy text to Windows clipboard using built-in tools. - // Use stdin piping (spawnSync input) to avoid shell escaping issues. try { const input = text.endsWith("\n") ? text : `${text}\n`; - const result = spawnSync("cmd", ["/c", "clip"], { - input, - encoding: "utf8", - windowsHide: true, - }); + let result; + if (process.platform === "win32") { + result = spawnSync("cmd", ["/c", "clip"], { + input, + encoding: "utf8", + windowsHide: true, + }); + } else if (process.platform === "darwin") { + result = spawnSync("pbcopy", [], { + input, + encoding: "utf8", + }); + } else { + // Try xclip first, then xsel + result = spawnSync("xclip", ["-selection", "clipboard"], { + input, + encoding: "utf8", + }); + if (result.status !== 0) { + result = spawnSync("xsel", ["--clipboard", "--input"], { + input, + encoding: "utf8", + }); + } + } return result.status === 0; } catch (err) { log.debug("Failed to copy URL to clipboard", { err }); return false; } }
2922-2938: Progress counter across two sequential scans is correct but slightly fragile.The outer
donevariable is incremented across bothscanAllAccountsForVerificationcalls (active then quarantined), relying on them being sequential. This works since both calls areawaited. A brief comment noting this dependency would help future readers.src/plugin/accounts.ts (1)
326-448: Constructor andreloadFromDiskshare near-identical account mapping logic.The mapping from
AccountMetadataV3toManagedAccount(lines 338-375 vs 961-997) is duplicated. This is a minor DRY concern — if a new field is added toManagedAccount, it needs updating in both places.
📜 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 ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.gitignoresrc/plugin.tssrc/plugin/accounts.tssrc/plugin/request.tssrc/plugin/ui/auth-menu.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugin/ui/auth-menu.ts (3)
scripts/check-quota.mjs (2)
accounts(155-155)label(169-169)src/plugin/ui/ansi.ts (1)
ANSI(6-25)src/plugin/ui/select.ts (1)
select(77-295)
src/plugin/accounts.ts (5)
src/plugin/quota.ts (2)
QuotaGroup(15-15)QuotaGroupSummary(17-21)src/plugin/storage.ts (3)
AccountStorageV3(198-206)ModelFamily(128-128)BlockedAccountMetadataV1(231-236)src/plugin/transform/types.ts (1)
ModelFamily(3-3)src/plugin/transform/index.ts (1)
ModelFamily(9-9)src/plugin/auth.ts (1)
parseRefreshParts(12-19)
🔇 Additional comments (20)
src/plugin/request.ts (8)
106-113: LGTM! Narrowing signature caching to Gemini-3 only makes sense given that Claude signatures are per-response and non-replayable. The comment clearly documents the rationale.
1478-1490: LGTM! Correctly skipping synthetic thinking placeholder injection for Claude models, since Claude's signed thinking blocks would reject synthetic content.
1516-1519: LGTM! Consistently strips Claude thinking from streaming responses, matching the non-streaming path at Lines 1685-1688.
1538-1539: Good fix — preserves the original response body for the catch fallback at Line 1702.Using
response.clone().text()ensures the originalresponsereturned in thecatchblock still has a consumable body.
1549-1578: Good fix — prevents debug text from poisoning error-type detection and recovery logic.Running
detectErrorTypeonrawMessagebefore any debug injection (Line 1558) and gating verbose debug info behindisDebugEnabled()(Line 1568) are both solid improvements. This avoids false-positive recovery triggers and keeps user-facing errors clean.
1600-1619: RetryInfo extraction looks correct; minor regex note.The regex
^([\d.]+)s$at Line 1608 would accept malformed values like"1.2.3s", butparseFloatwould still yield1.2which passes the> 0guard, so this is safe in practice. TheRetry-Afterandretry-after-msheader extraction is a useful addition for upstream rate-limit honoring.
1685-1688: LGTM! Consistent with the streaming path — Claude thinking blocks are stripped from non-streaming responses as well.
358-382: No action needed. BothSENTINEL_SIGNATURE(local) andSKIP_THOUGHT_SIGNATURE(imported from../constants) resolve to the identical string value"skip_thought_signature_validator". There is no inconsistency between the two constants despite their separate definitions.src/plugin/ui/auth-menu.ts (2)
52-83: LGTM — well-structured menu layout with headings and badges.The use of
kind: 'heading'for non-selectable section titles and separators is clean. The numbered account labels with status/current/disabled badges are a nice UX improvement.
103-137: LGTM — showAccountDetails refactored cleanly.The computed
headerandsubtitlePartsapproach is more maintainable than direct console output, and theclearScreenoption improves the TUI experience.src/plugin.ts (6)
571-610: Race-free concurrent scan pattern — nicely done.The worker pool pattern with
nextIndex++is safe in single-threaded JS. Pre-allocated results array with index-based assignment avoids ordering issues. Thefilter(Boolean)at the end handles any sparse slots. Clean implementation.
87-117: URL and message sanitization helpers look solid.
summarizeUpstreamValidationMessagestrips debug sections, URLs, and truncates — good defense against leaking noisy upstream details to the user.isGeminiCodeAssistLicenseErrorchecks multiple indicators for robustness.
141-175: Good defensive sanitization of malformed Google auth parameters.The handling of double-encoded
authuser%2522%2Ckeys and the normalization logic is thorough. The iteration-then-delete pattern avoids modifying the collection during iteration.
286-401:extractValidationUrlAndMessage— robust multi-format extraction.Handles JSON, SSE
data:lines, regex URL scanning, double-escaped unicode sequences, and URL scoring withplt=preference. The layered extraction approach is well-suited for dealing with inconsistent upstream responses.
2312-2369: 403 VALIDATION_REQUIRED quarantine and synthetic error response — good UX.Quarantining the account removes it from rotation so the retry loop doesn't keep hitting it. When all accounts are quarantined, the synthetic SSE error gives the user a clear recovery path. The message format is actionable.
2964-3019: Auto-restore of verified quarantined accounts is a nice touch.The scan detects accounts that are now "ok" and moves them back into the active pool, saving the user from manual intervention. The
addedRefreshTokensparameter ensures cross-process consistency.src/plugin/accounts.ts (4)
940-1017:reloadFromDiskcorrectly resets pending token diffs and session state.This addresses the previously flagged concern. The implementation mirrors the constructor's mapping logic while properly resetting ephemeral state (
sessionOffsetApplied, toast state, pending token sets).
559-573: Disabled account check in sticky mode — previous concern addressed.Line 560 now checks
current.enabled !== falsebefore returning the sticky account, preventing disabled accounts from being selected.
584-631: New soft-quota and Antigravity-availability helpers look correct.
updateQuotaCachemerges partial updates,getMinWaitTimeForSoftQuotafails open (returns0ornullappropriately), andhasOtherAccountWithAntigravityAvailablecorrectly skips the current account, disabled accounts, and cooling-down accounts before checking rate limits.
1070-1119: Cross-process merge semantics forsaveToDiskare well-implemented.Pending token diffs are accumulated between saves and flushed atomically. The
merge: true+preserveDeletions: true+ explicit added/removed token lists ensure concurrent processes don't resurrect deleted accounts or drop rotated tokens.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin/storage.ts (1)
773-797:⚠️ Potential issue | 🟡 Minor
loadAccountsUnsafedoesn't validate version for the default branch.When
parsed.versionis neither 1 nor 2, the function returns the raw parsed data asAccountStorageV3without checking thatversion === 3or thataccountsis an array. A corrupted or future-version file could produce unexpected merge behavior insaveAccounts.🛡️ Suggested fix
if (parsed.version === 2) { return migrateV2ToV3(parsed); } + if (parsed.version !== 3 || !Array.isArray(parsed.accounts)) { + return null; + } + return { ...parsed, accounts: deduplicateAccountsByEmail(parsed.accounts), };
🧹 Nitpick comments (6)
src/plugin/storage.ts (3)
364-409:withBlockedFileLockis a near-exact duplicate ofwithFileLock.The only difference is calling
ensureBlockedAccountsFileExistsvsensureFileExists. Consider a shared helper parameterized by the ensure function to eliminate this duplication.♻️ Suggested refactor
-async function withFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { - await ensureFileExists(path); +async function withLock<T>(path: string, ensureFn: (p: string) => Promise<void>, fn: () => Promise<T>): Promise<T> { + await ensureFn(path); let release: (() => Promise<void>) | null = null; try { release = await lockfile.lock(path, LOCK_OPTIONS); return await fn(); } finally { if (release) { try { await release(); } catch (unlockError) { log.warn("Failed to release lock", { error: String(unlockError) }); } } } } -async function withBlockedFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { - await ensureBlockedAccountsFileExists(path); - ... -} +function withFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { + return withLock(path, ensureFileExists, fn); +} + +function withBlockedFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { + return withLock(path, ensureBlockedAccountsFileExists, fn); +}
821-828: Excessiveas anycasts obscure type safety.The parsing loop casts every field through
as any. Consider defining a minimal shape type for the raw parsed account and using a validation/narrowing approach instead:♻️ Suggested approach
- const map = new Map<string, BlockedAccountMetadataV1>(); - for (const acc of parsed.accounts) { - if (!acc || typeof acc !== "object") continue; - const refreshToken = (acc as any).refreshToken; - if (typeof refreshToken !== "string" || !refreshToken) continue; - const blockedAt = typeof (acc as any).blockedAt === "number" ? (acc as any).blockedAt : 0; - map.set(refreshToken, { ...(acc as any), blockedAt }); + const map = new Map<string, BlockedAccountMetadataV1>(); + for (const raw of parsed.accounts) { + if (!raw || typeof raw !== "object") continue; + const acc = raw as Record<string, unknown>; + const refreshToken = acc.refreshToken; + if (typeof refreshToken !== "string" || !refreshToken) continue; + const blockedAt = typeof acc.blockedAt === "number" ? acc.blockedAt : 0; + map.set(refreshToken, { ...(raw as BlockedAccountMetadataV1), blockedAt }); }
844-866:saveBlockedAccountsdoesn't merge with existing on-disk state.Unlike
saveAccounts, this function overwrites the file with whateverstorageis passed in. The caller inquarantineAccountForVerificationdoes load+merge manually before calling this, butclearBlockedAccountselsewhere could race with a concurrent quarantine. Since blocked-account writes appear infrequent, this is low risk, but worth noting.src/plugin/accounts.ts (3)
943-1020: Account-mapping logic inreloadFromDiskduplicates the constructor.The mapping from
AccountMetadataV3→ManagedAccount(lines 964-1000) is nearly identical to the constructor (lines 341-378). If the mapping logic changes, both locations must be updated in sync. Consider extracting a sharedmapStoredToManaged(stored, authFallback)helper.
1255-1289: Restored fingerprint entry remains in history — consider removing it.After restoring from
historyIndex, the old entry remains in the array (shifted by +1 due to the unshift of the current fingerprint). This means the same fingerprint appears both as the active fingerprint and in the history. If this is intentional (history as an audit log), that's fine. If not, consider splicing out the restored entry to avoid confusion.
157-163: Consolidate duplicateQuotaGroupandQuotaGroupSummarydefinitions.These types are identically defined in both
accounts.ts(lines 157–163) andquota.ts(lines 15–22). Sincequota.tsis the specialized quota module, it should be the canonical source. Move these type definitions toquota.tsonly, then haveaccounts.tsimport and re-export them if needed for internal API compatibility.
📜 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/storage.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugin/accounts.ts (5)
src/plugin/quota.ts (2)
QuotaGroup(15-15)QuotaGroupSummary(17-21)src/plugin/storage.ts (3)
AccountStorageV3(202-210)ModelFamily(132-132)BlockedAccountMetadataV1(329-334)src/plugin/transform/types.ts (1)
ModelFamily(3-3)src/plugin/refresh-queue.ts (1)
refreshToken(213-228)src/plugin/auth.ts (1)
parseRefreshParts(12-19)
⏰ 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 (8)
src/plugin/storage.ts (3)
29-30: LGTM! Blocked accounts files are correctly added to the gitignore entries.
672-754: LGTM —saveAccountsmerge and preserve-deletions logic looks correct.The
preserveDeletions !== falsedefault is correct (matching the doc), merge behavior is sound, and index clamping after removals prevents writing invalid indices. The previous review concern about inverted defaults has been properly addressed.
325-339: LGTM!getBlockedAccountsPath,BlockedAccountMetadataV1, andBlockedAccountStorageV1are well-structured. The type properly extendsAccountMetadataV3with quarantine-specific fields.src/plugin/accounts.ts (5)
381-407: LGTM — Cross-process account merge in constructor is well-handled.The check for missing fallback accounts, creation of a new
ManagedAccount, and tracking viapendingAddedRefreshTokensproperly handles the case where another process performed an OAuth login.
1022-1120: Quarantine rollback logic is thorough and well-implemented.The state capture before removal, the rollback of in-memory state (account list, cursor, family indices, pending token sets), and the logging on failure all address the previously raised concerns. Good work.
1122-1172: LGTM —saveToDiskcorrectly handles merge semantics and pending token cleanup.Pending token sets are only cleared after successful persistence, preserving them for retry on failure. Index clamping prevents invalid state on disk.
833-843: LGTM —removeAccountproperly tracks removed tokens.The pending token tracking correctly adds to
pendingRemovedRefreshTokensand removes frompendingAddedRefreshTokensto maintain consistency.
562-577: Previous concern about disabled accounts in sticky mode is addressed.Line 563 now checks
current.enabled !== falsebefore proceeding, and line 573 additionally checks soft quota threshold.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/cli.ts
Line: 108:114
Comment:
**Unreachable delete flow**
`promptLoginMode()` returns `{ mode: "manage", deleteAccountIndex: ... }` when `showAccountDetails()` returns `"delete"`, but this PR removed the `manage` action from the auth menu flow (it now emits `{ type: "verify" }` instead). If the caller doesn’t handle `mode: "manage"`, selecting “Delete this account” will become a dead-end/unhandled branch. Fix by returning a mode that still exists in the current state machine (or reintroduce the `manage` mode handling end-to-end).
How can I resolve this? If you propose a fix, please make it concise.
In the interactive TTY flow, selecting an account and choosing Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/cli.ts
Line: 117:127
Comment:
**Returns removed "manage" mode**
In the interactive TTY flow, selecting an account and choosing `Delete this account` or `Enable/disable account` returns `{ mode: "manage" }` (`src/plugin/cli.ts:119-127`), but the menu action for manage was removed (auth menu now has `verify` instead), and `src/plugin.ts` appears to only branch on `mode === "check"` / `deleteAccountIndex` / etc. This makes the returned `mode` inconsistent with the current state machine and can strand users in an unhandled mode. Return a mode that is still handled by the caller, or update the caller to handle `manage` consistently end-to-end.
How can I resolve this? If you propose a fix, please make it concise. |
Additional Comments (1)
In this PR, Either re-add the quota cache fields to Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/storage.ts
Line: 193:198
Comment:
**V3 schema mismatch**
In this PR, `AccountMetadataV3` replaces `cachedQuota`/`cachedQuotaUpdatedAt` with `fingerprintHistory` (`src/plugin/storage.ts:193-198`), but `AccountManager.saveToDisk()` still serializes `cachedQuota` and `cachedQuotaUpdatedAt` (`src/plugin/accounts.ts:953-955`). As-is, the persisted shape and the declared V3 schema disagree, and quota cache data can be silently lost/ignored on reload.
Either re-add the quota cache fields to `AccountMetadataV3`, or stop persisting them so the on-disk format matches the schema.
How can I resolve this? If you propose a fix, please make it concise. |
|
Checked the latest bot comments and patched the real issues
Also verified the latest V3 schema mismatch note is stale on current branch Commit pushed to PR branch |
|
Thanks for flagging this @SK-DEV-AI
So this specific issue is resolved on the current PR state |
|
Are there other ways to find blocked accounts instead of creating a generation request to 3-flash? I noticed that blocked accounts do not have access to the gemini cli quota check. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/plugin/storage.ts`:
- Around line 904-906: The temp file written by saveBlockedAccounts uses
fs.writeFile(tempPath, content, "utf-8") and must be created with strict
permissions like the main accounts file; change the write call in
saveBlockedAccounts to include an options object { encoding: "utf-8", mode:
0o600 } so the temp file is created with 0o600 permissions before the subsequent
fs.rename(path) (match the pattern used where the main accounts path is written
with { encoding: "utf-8", mode: 0o600 }).
- Around line 376-387: The blocked-accounts file created by
ensureBlockedAccountsFileExists is written without restrictive permissions;
update ensureBlockedAccountsFileExists to mirror ensureFileExists by supplying a
mode of 0o600 when writing the file (e.g., call fs.writeFile with encoding
"utf-8" and mode 0o600 or equivalent) so the JSON file containing refresh tokens
is created with correct permissions; keep the existing mkdir call and JSON
content but add the mode option to the write step in
ensureBlockedAccountsFileExists.
🧹 Nitpick comments (4)
src/plugin/request.ts (1)
1615-1634: Consider adding atypeofguard onretryDelayfor defensive programming.Per Google's ProtoJSON specification,
google.protobuf.Durationis always encoded as a string (e.g.,"1.5s"), soretryDelaywill be a string if it exists in the JSON response. However, adding atypeofcheck before calling.match()is reasonable defensive programming:- if (retryInfo?.retryDelay) { - const match = retryInfo.retryDelay.match(/^([\d.]+)s$/); + if (retryInfo?.retryDelay && typeof retryInfo.retryDelay === 'string') { + const match = retryInfo.retryDelay.match(/^([\d.]+)s$/);This guards against unexpected schema deviations and makes the intent explicit.
src/plugin/storage.ts (2)
406-421:withBlockedFileLockduplicateswithFileLock— consider parameterising.The two functions differ only in the
ensure*call. A single generic helper accepting anensureFnparameter would eliminate the duplication.♻️ Example consolidation
-async function withFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { - await ensureFileExists(path); +async function withLock<T>( + path: string, + ensureFn: (p: string) => Promise<void>, + fn: () => Promise<T>, +): Promise<T> { + await ensureFn(path); let release: (() => Promise<void>) | null = null; try { release = await lockfile.lock(path, LOCK_OPTIONS); @@ ... } + +async function withFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { + return withLock(path, ensureFileExists, fn); +} + +async function withBlockedFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { + return withLock(path, ensureBlockedAccountsFileExists, fn); +}
762-768: Unnecessaryas anycasts onactiveIndexByFamily.The type of
activeIndexByFamilyalready declaresclaude?: numberandgemini?: number, so the casts are superfluous and reduce type safety.♻️ Proposed fix
if (merged.activeIndexByFamily && typeof merged.activeIndexByFamily === "object") { merged.activeIndexByFamily = { ...merged.activeIndexByFamily, - claude: clampIndex((merged.activeIndexByFamily as any).claude), - gemini: clampIndex((merged.activeIndexByFamily as any).gemini), + claude: clampIndex(merged.activeIndexByFamily.claude), + gemini: clampIndex(merged.activeIndexByFamily.gemini), }; }src/plugin/accounts.ts (1)
157-163: ConsolidateQuotaGroupandQuotaGroupSummarytype definitions to avoid duplication.These types are identically defined in both
src/plugin/quota.ts(lines 15-20) andsrc/plugin/accounts.ts(lines 157-163). Move the definitions toquota.ts(the more quota-focused module) and haveaccounts.tsimport and re-export them to prevent them from silently diverging.
📜 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 (6)
package.jsonsrc/plugin.tssrc/plugin/accounts.tssrc/plugin/request-helpers.tssrc/plugin/request.tssrc/plugin/storage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugin/request-helpers.ts (2)
src/plugin/transform/index.ts (1)
isClaudeModel(35-35)src/plugin/transform/claude.ts (1)
isClaudeModel(27-29)
src/plugin/accounts.ts (5)
src/plugin/quota.ts (2)
QuotaGroup(15-15)QuotaGroupSummary(17-21)src/plugin/storage.ts (4)
AccountStorageV3(202-210)ModelFamily(132-132)HeaderStyle(133-133)BlockedAccountMetadataV1(329-334)src/plugin/transform/types.ts (1)
ModelFamily(3-3)src/constants.ts (1)
HeaderStyle(156-156)src/plugin/auth.ts (1)
parseRefreshParts(12-19)
⏰ 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). (4)
- GitHub Check: Greptile Review
- GitHub Check: Greptile Review
- GitHub Check: Greptile Review
- GitHub Check: Greptile Review
🔇 Additional comments (15)
src/plugin/request-helpers.ts (3)
1104-1109: LGTM — unconditional Claude thinking-block stripping is a sound defense-in-depth measure.The comment clearly explains the rationale: Claude signatures are per-response and cannot be replayed, so stripping all thinking blocks avoids hard-to-recover 400 errors. Tool blocks are correctly preserved via
stripAllThinkingBlocks.
1502-1553:stripThinkingFromResponselooks correct but silently dropsreasoning_contentfrom non-candidate responses.The function handles both
candidates[].content.partsand top-levelcontent[]arrays, plus thereasoning_contentaggregate — all correct.One minor observation: if a response has
reasoning_contentbut no thinking parts incandidatesorcontent(e.g., a pre-aggregated response from another provider), the reasoning text is deleted without any trace. This is intentional per the doc comment ("users generally don't want"), but worth confirming this never applies to Gemini models since the call site gates onisClaude.
2786-2822: Synthetic error response now emits Antigravity-shaped SSE — good fix.The switch from multi-event Claude SSE to a single
data: {"response": {...}}payload aligns with what OpenCode's Google provider expects. TheerrorTypeparameter and correspondingX-Antigravity-Error-Typeheader provide good downstream signal.Minor note:
outputTokensis computed asMath.ceil(errorMessage.length / 4)which is a rough heuristic. Since this is a synthetic response for error surfacing only, the imprecision is acceptable and won't affect billing or real usage tracking.src/plugin/request.ts (9)
48-48: LGTM — import addition forstripThinkingFromResponse.Correctly added to the import list from
./request-helpers, aligning with its usage in the response transformation paths.
109-113: LGTM — restricting signature caching to Gemini 3 only.Good change. Claude signatures are per-response and non-replayable, so caching them would only lead to invalid-signature errors on subsequent turns.
732-737: LGTM — defense-in-depth thinking block stripping on the wrapped payload.Note that
deepFilterThinkingBlocksis called again on individualrequestObjectsat line 749. Since it mutates in-place, the second call is effectively a no-op for overlapping parts, so there's no functional issue — just belt-and-suspenders.
757-759: LGTM — disabling warmup for Claude models.Since all Claude thinking blocks are stripped from outgoing requests, a warmup request to obtain a thinking signature serves no purpose.
1176-1180: LGTM — consistent with the wrapped-payload path.Both the wrapped and non-wrapped Claude paths now skip thinking injection/warmup, maintaining a single invariant: Claude thinking blocks are always stripped.
1494-1506: LGTM — Claude-aware debug text and response handling.Correctly avoids injecting the synthetic
SYNTHETIC_THINKING_PLACEHOLDERfor Claude (since Claude thinking blocks are signed and non-replayable), while still allowing full debug logs when debug mode is enabled.
1532-1535: LGTM — streaming path strips Claude thinking blocks.Consistent with the non-streaming path at lines 1700-1703:
transformThinkingPartsnormalizes the format, thenstripThinkingFromResponseremoves thinking blocks for Claude models.
1565-1596: Good fix — extractingerrorTypefrom the raw upstream message before debug injection.This prevents debug text (which can contain words like "thinking" or "tool_use") from falsely triggering recovery patterns. The change to only append verbose debug info under
isDebugEnabled()also keeps user-facing error messages clean by default.
1700-1703: LGTM — non-streaming response path strips Claude thinking blocks.Consistent with the streaming path, ensuring Claude thinking blocks never reach OpenCode regardless of response mode.
src/plugin/accounts.ts (3)
977-988: Pending token diffs correctly cleared on reload — looks good.Lines 986–987 reset
pendingAddedRefreshTokensandpendingRemovedRefreshTokens, preventing stale deltas from corrupting a subsequent save after a disk reload.
1056-1154: Quarantine with rollback is well-structured.The rollback captures all mutable state (cursor, family indices, pending token sets) before the removal, and restores on failure. The log messages include enough context without leaking full tokens. Good defensive coding.
1190-1205: Merge-safe persistence with token tracking — looks good.
saveToDiskpasses the accumulatedpendingAddedRefreshTokensandpendingRemovedRefreshTokenstosaveAccountsand clears them after a successful write, correctly supporting concurrent multi-process access.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugin/storage.ts`:
- Around line 762-768: The current spread+clamp pattern on
merged.activeIndexByFamily indiscriminately assigns claude and gemini to
clampIndex(...) which converts undefined to 0; change this to only clamp and
assign each family key if it already exists (e.g., check hasOwnProperty or
typeof merged.activeIndexByFamily.claude !== "undefined" before setting
merged.activeIndexByFamily.claude = clampIndex(...), and likewise for gemini) so
undefined keys remain undefined and other existing keys are preserved.
🧹 Nitpick comments (2)
src/plugin/storage.ts (2)
861-878: Unvalidated spread of parsed JSON into typed result.Line 877 does
{ ...(acc as any), blockedAt }, which blindly copies every property from the parsed JSON entry into aBlockedAccountMetadataV1. A corrupted or hand-edited file could introduce unexpected fields that persist through save round-trips. This is low-risk for a local config file, but consider picking only known keys (similar to howmigrateV1ToV2explicitly maps fields) for defense-in-depth.
406-421:withBlockedFileLockduplicateswithFileLock— consider extracting a shared helper.The only difference is the
ensure*call. A parameterized helper likewithLock(path, ensureFn, fn)would eliminate the duplication.
📜 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/storage.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugin/storage.ts (1)
src/plugin/refresh-queue.ts (1)
refreshToken(213-228)
🔇 Additional comments (4)
src/plugin/storage.ts (4)
29-30: LGTM — new gitignore entries for blocked accounts files.Correctly mirrors the existing patterns for the main accounts file.
376-387: Previous permission issue addressed.
ensureBlockedAccountsFileExistsnow uses{ encoding: "utf-8", mode: 0o600 }, matchingensureFileExists.
717-752: Merge + preserve-deletions logic looks correct.The three-phase approach (merge → remove explicitly deleted → prune stale resurrections) is sound. The
preserveDeletionsdefault is now correctly!== false. The earlier concern about the inverted default has been addressed.
894-916: Atomic write with proper permissions — LGTM.Temp-file + rename under lock with
mode: 0o600. The earlier permission concern has been addressed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Right now we still need the 3 flash probe because it is the only reliable way to detect blocked accounts and get the verify link. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin/accounts.ts (1)
1178-1237:⚠️ Potential issue | 🟡 Minor
activeIndexis always derived from the Claude family index — this affects load-time defaults for Gemini-only deployments with migrated storage.Line 1185 derives
activeIndexexclusively fromthis.currentAccountIndexByFamily.claude. SinceactiveIndexByFamilyis optional inAccountStorageV3(and is not created during migration from V1/V2), deployments loading old storage will have both Claude and Gemini default to the sameactiveIndex—which reflects Claude's historical state, not Gemini's. This could cause incorrect account selection in Gemini-only deployments until the firstsaveToDisk()call, after which both family indices are persisted and the issue is mitigated.Consider initializing
activeIndexByFamilyduring migration (V1→V2→V3) to provide family-specific defaults rather than reusing Claude's index.
🤖 Fix all issues with AI agents
In `@src/plugin/accounts.ts`:
- Around line 157-163: Remove the duplicate local type definitions QuotaGroup
and QuotaGroupSummary in accounts.ts and replace them with an import of those
types from the existing quota module that already exports them; specifically
delete the local "export type QuotaGroup" and "export interface
QuotaGroupSummary" declarations and add an import for QuotaGroup and
QuotaGroupSummary, then update any local references to use the imported types.
🧹 Nitpick comments (3)
src/plugin/storage.ts (2)
406-421: Consider extracting a shared lock helper to reduce duplication withwithFileLock.
withBlockedFileLockis nearly identical towithFileLock(lines 389–404) — the only difference is theensure*call. A small generic helper would eliminate the duplication:♻️ Example
-async function withFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { - await ensureFileExists(path); - ... -} - -async function withBlockedFileLock<T>(path: string, fn: () => Promise<T>): Promise<T> { - await ensureBlockedAccountsFileExists(path); - ... -} +async function withLock<T>( + path: string, + ensureFn: (p: string) => Promise<void>, + fn: () => Promise<T>, +): Promise<T> { + await ensureFn(path); + let release: (() => Promise<void>) | null = null; + try { + release = await lockfile.lock(path, LOCK_OPTIONS); + return await fn(); + } finally { + if (release) { + try { + await release(); + } catch (unlockError) { + log.warn("Failed to release lock", { error: String(unlockError) }); + } + } + } +}
868-911:loadBlockedAccountsholds a write-lock for a read-only operation.The function only reads and normalizes data but acquires an exclusive lock via
withBlockedFileLock. This serialises all reads behind any concurrent write, which is safe but may add unnecessary contention if the scan UI is polling or multiple processes read simultaneously. A shared/read lock or a lock-free read with retry would reduce contention, though this is unlikely to be a bottleneck in practice with the current usage patterns.src/plugin/accounts.ts (1)
1320-1354: Restoring a fingerprint leaves a copy of the restored entry in history.After
unshiftat line 1340 shifts all indices, the original entry athistoryIndex(now athistoryIndex + 1) remains in the array alongside the newly pushed current-fingerprint entry. Over repeated restore operations this can fill history with duplicates faster than expected, reducing the effective history depth. This is acceptable if it's the intended audit-trail behavior, but worth a note.
📜 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)
src/plugin/accounts.test.tssrc/plugin/accounts.tssrc/plugin/storage.test.tssrc/plugin/storage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugin/storage.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugin/accounts.test.ts (2)
src/plugin/storage.ts (3)
AccountStorageV3(202-210)ModelFamily(132-132)saveAccounts(707-793)src/plugin/transform/types.ts (1)
ModelFamily(3-3)
src/plugin/accounts.ts (7)
src/plugin/quota.ts (2)
QuotaGroup(15-15)QuotaGroupSummary(17-21)src/plugin/types.ts (1)
OAuthAuthDetails(4-9)src/plugin/storage.ts (4)
AccountStorageV3(202-210)ModelFamily(132-132)HeaderStyle(133-133)BlockedAccountMetadataV1(329-334)src/plugin/transform/types.ts (1)
ModelFamily(3-3)src/plugin/transform/index.ts (1)
ModelFamily(9-9)src/constants.ts (1)
HeaderStyle(156-156)src/plugin/auth.ts (1)
parseRefreshParts(12-19)
⏰ 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 (8)
src/plugin/storage.ts (3)
29-30: LGTM — blocked-accounts file creation and gitignore entries.Permissions are now set to
0o600(addressing the prior review), and the gitignore entries cover both the JSON file and its temp-file glob.Also applies to: 376-387
687-775: Merge, prune, and clamp logic looks correct and addresses prior review feedback.
preserveDeletionsnow defaults totrue(!== false),clampFamilyIndexconditionally applies only to defined keys, and the stale-incoming-token pruning correctly uses bothdiskTokensandaddedTokenssets. The overall flow (merge → remove → prune stale → clamp → atomic write) is sound.
913-935: LGTM —saveBlockedAccountsuses atomic temp-file + rename under lock with0o600permissions.src/plugin/accounts.ts (3)
397-423: LGTM — auto-merge of fallback account into persisted pool.Correctly checks for a matching
refreshToken, creates a newManagedAccount, marks it as a pending addition, and triggers a debounced save. This handles the cross-process "fresh OAuth login" scenario well.
997-1008: LGTM —reloadFromDisknow clears pending token diffs.Lines 1006–1007 clear both
pendingAddedRefreshTokensandpendingRemovedRefreshTokens, addressing the prior review concern about stale deltas surviving a reload.
1078-1176: LGTM — quarantine rollback is thorough.The capture-before-mutate pattern (lines 1088–1092), the full state restoration on blocked-storage failure (lines 1135–1152), and the best-effort
saveToDiskafterward form a solid safety net. Logging on both failure paths (blocked-save and restore) addresses the prior review feedback.src/plugin/accounts.test.ts (2)
4-15: LGTM — mock setup correctly isolates storage I/O.
saveAccountsandsaveAccountsReplaceare mocked while preserving the rest of the module, allowing tests to inspect persisted data without hitting the filesystem.
545-586: LGTM — sentinel tests verify the full round-trip of-1through load and save.The first test confirms
getCurrentAccountForFamilyreturnsnullwhen the stored index is-1, and the second confirmssaveToDiskwrites-1back throughclampFamilyIndexwhile clamping the legacyactiveIndexto0. Good coverage of the sentinel semantics.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugin/accounts.ts`:
- Around line 1173-1191: The saveToDisk method currently computes activeIndex
from this.currentAccountIndexByFamily.claude, which persists the Claude family
index instead of the real round-robin counter; change saveToDisk to derive
activeIndex from this.cursor (the global round-robin counter) and then pass that
activeIndex into clampFamilyIndex for claude and gemini so the persisted
activeIndex matches runtime rotation state (refer to saveToDisk,
currentAccountIndexByFamily, cursor, clampFamilyIndex, and the claude/gemini
index variables).
📜 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/accounts.ts
🔇 Additional comments (13)
src/plugin/accounts.ts (13)
19-27: Past duplicate-type concern is resolved.
QuotaGroupandQuotaGroupSummaryare now imported from./quotaand re-exported, eliminating the DRY violation.
198-212: LGTM.clampFamilyIndexcorrectly handles the -1 sentinel, invalid inputs, and boundary clamping.
392-418: Clean cross-process account merge logic.The constructor properly deduplicates by refresh token before adding a new account from auth fallback, and correctly tracks it for merge-aware persistence.
519-603: Soft quota threshold is consistently integrated across sticky, round-robin, and hybrid strategies. TheDEFAULT_SOFT_QUOTA_CACHE_TTL_MSconstant is used uniformly.
619-653: LGTM. The fail-open semantics are well-considered: stale/missing cache data yields0(proceed), and invalidresetTimevalues are safely filtered viaNumber.isFinite.
659-675: LGTM. Correctly checks other accounts for Antigravity availability, skipping disabled and cooling-down accounts.
882-921: Proper cross-process token tracking in removeAccount.The bidirectional update of
pendingRemovedRefreshTokens/pendingAddedRefreshTokensensures the merge-aware save won't resurrect a removed account.
992-1071: Past concern about stale pending token diffs is resolved —pendingAddedRefreshTokensandpendingRemovedRefreshTokensare now cleared on lines 1001–1002.The account hydration logic (lines 1012–1049) closely mirrors the constructor. Consider extracting a shared
hydrateAccounthelper to avoid the duplication, though this is low priority given the code is stable.[approve_code_changes, suggest_optional_refactor]
1073-1171: Solid rollback-safe quarantine implementation. The state snapshot beforeremoveAccountand the comprehensive restore path (including pending token sets) address the atomicity concern from the prior review. The structured logging on failure is a good addition.
1216-1232: Merge-aware save with token deltas looks correct. Pending sets are captured before the async write and cleared after, ensuring cross-process consistency.
1315-1349: LGTM. The fingerprint is correctly captured before history mutation, preventing the shifted-index issue. ThecreatedAtreset on restore is a reasonable semantic choice.
606-611: LGTM. Quota cache update with spread-merge correctly layers new data on top of existing cache.
66-68: LGTM. The constant is well-documented with its test assumption and fail-open semantics.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/cli.ts
Line: 120:120
Comment:
Check that `plugin.ts` processes delete via the `deleteAccountIndex` field regardless of mode value
How can I resolve this? If you propose a fix, please make it concise. |
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/plugin/ui/select.ts
Line: 1:3
Comment:
**Regression: no redraw without clear**
`render()` no longer rewrites previous frames unless `options.clearScreen === true` (the old `ANSI.up(totalLines)` path was removed). Any existing callers that don’t pass `clearScreen` will now print a fresh copy of the menu on every keypress, spamming scrollback and breaking the interactive UX. Either restore cursor-up repaint when `clearScreen` is false, or make `clearScreen` effectively default-on for interactive menus (and update docs accordingly).
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/ui/select.ts`:
- Around line 126-131: The subtitle block is counted as 4 lines but actually
renders 3, so update the calculation to use const subtitleLines = subtitle ? 3 :
0 (and update the inline comment) so fixedLines (1 + subtitleLines + 2)
correctly reflects header + subtitle + help/bottom; then keep maxVisibleItems as
Math.max(1, Math.min(items.length, rows - fixedLines - 1)) so the viewport uses
the freed row.
🧹 Nitpick comments (2)
src/plugin/ui/select.ts (2)
28-65: ANSI utilities look correct; minor style-leak risk on truncation.
truncateAnsipreserves ANSI escape sequences while truncating visible characters — nice. However, when truncation kicks in, any open ANSI style (e.g.\x1b[31m) is never closed: thesuffix("...") inherits the style, and it can bleed into subsequent terminal output.Appending
ANSI.resetafter the suffix would make this bullet-proof:Proposed fix
- return out + suffix; + return out + suffix + ANSI.reset;This is especially relevant at Line 165 where
ANSI.dim + ANSI.boldare baked into the string before truncation — if the label is long, the trailingANSI.resetgets clipped.
249-257:findNextSelectableis safe given preconditions but has no cycle guard.The
do…whilerelies on the validation at lines 90-93 guaranteeing at least one selectable item. If that invariant were ever violated (e.g. items mutated externally), this would spin forever. A low-priority defensive addition:Optional: add a cycle guard
let next = from; + let steps = 0; do { next = (next + direction + items.length) % items.length; - } while (items[next]?.disabled || items[next]?.separator || items[next]?.kind === 'heading'); + if (++steps > items.length) return from; // safety: no selectable item found + } while (items[next]?.disabled || items[next]?.separator || items[next]?.kind === 'heading');
📜 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.tssrc/plugin/ui/select.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugin/ui/select.ts (1)
src/plugin/ui/ansi.ts (1)
ANSI(6-25)
⏰ 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/plugin/ui/select.ts (4)
3-12: LGTM on the newMenuItem.kindandSelectOptionsextensions.Clean additions with clear JSDoc. The
headingkind andclearScreen/helpoptions integrate well with the rendering and navigation logic below.Also applies to: 14-24
89-103:isSelectableand cursor initialisation are consistent — looks good.The predicate correctly excludes
disabled,separator, andheadingitems, and is reused for both filtering and initial cursor placement.
154-194: Windowed item rendering and heading support look correct.The loop properly maps
itemIndexback to the fullitemsarray for cursor comparison (Line 170), headings are rendered as non-selectable styled rows, andtruncateAnsiprevents line wrapping. Clean implementation.
196-212: Stale-line cleanup and help text rendering are well handled.The leftover-line clearing logic (lines 204-209) correctly accounts for the difference between previous and current render heights, and the dynamic window hint gives users orientation within long lists.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
4 participants
What this PR fixes
People got stuck when a Google account was blocked.
The old flow was confusing.
This PR makes the recovery flow clear inside
opencode auth login.Simple user flow now
Open
opencode auth loginChoose Verify blocked accounts
Run scan
Pick blocked account
Open or copy verification link
Finish Google verification
Run command again
Changes in simple terms
Added Verify blocked accounts in the auth menu.
Added account scan with clear statuses:
blocked
ok
error
no-license
skipped
Blocked accounts are shown first.
Added blocked-account actions:
Open sign-in page (prefill email)
Copy sign-in page link
Open direct verification link
Copy direct verification link
Fixed bad Google URLs that could open broken 400 pages.
Improved sign-in link building so selected email is prefilled.
Runtime blocked message now points users to
opencode auth login.Screenshot walkthrough (same order as uploaded files)
Shows Verify blocked accounts in the main auth menu.
Shows scan output where some accounts are marked BLOCKED.
Shows options for a blocked account, including open/copy sign-in link.
Shows the page where the user completes verification (QR code / phone).
Shows the success page after authentication/authorization is complete.
Shows another scan output example with blocked/ok/error statuses.
Shows details for an error account (rate-limited 429), not a blocked account.
Validation
npm run build
npm run typecheck
npm test
All passed locally.