fix: consolidate all community fixes + RE-based improvements#581
fix: consolidate all community fixes + RE-based improvements#581amartidandqdq wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a post-build ESM specifier fixer for dist/, a rate-limit parser/classifier (parseRateLimitResetMs and parseRateLimitReason), a small Antigravity request header tweak (remove x-goog-api-key), an in-memory hourly usage tracker and debug logging helpers, and a substantial quota subsystem refactor (bounded concurrency, per-account timeout, endpoint fallbacks, subscription-tier detection) with comprehensive Vitest coverage for checkAccountsQuota. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 SummaryThis PR consolidates six community fixes and several reverse-engineered improvements into a single release, covering Claude API-key sentinel, ESM import specifier fixing, TUI pause, Gemini 3.5 Flash support, subscription-tier detection, and a new
Confidence Score: 5/5Safe to merge; all changes are additive features or hardening improvements and 1048 tests pass cleanly. The core account-selection, retry, and persistence logic is well-tested and the overall changes are correct. There are two minor usability gaps — the fallbackAttempts counter can cut off quota-wait retries sooner than intended in single-account setups, and Claude thinking-tier variants may silently miss a configured model affinity entry — but neither causes incorrect behaviour for the common case and both are straightforward to address in a follow-up. src/plugin.ts (fallbackAttempts counter semantics) and src/plugin/transform/model-resolver.ts (affinity key normalisation for Claude thinking-tier variants) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request
model + family] --> B{model_account_affinity
configured for model?}
B -- Yes --> C[resolveModelAccountAffinityEmail
normalizeModelAccountAffinityKey]
B -- No --> G[Normal account selection
strategy: sticky / round-robin / hybrid]
C --> D[getAffinityAccount]
D --> E{Account available?
enabled, not rate-limited,
not cooling-down, not verif-required}
E -- Yes --> F[Use pinned account]
E -- No, strict=false --> G
E -- No, strict=true --> H[Throw AccountAffinityError
fail-fast]
G --> I[getCurrentOrNextForFamily]
I --> J{Account found?}
J -- Yes --> K[Make request
with jittered retry + fallbackAttempts cap]
J -- No: all verif-required --> L[Circuit-breaker
show toast + throw]
J -- No: all over quota --> M{waitMs <= MAX_WAIT?}
M -- Yes --> N[sleep + continue outer loop
fallbackAttempts++]
M -- No --> O[Throw: wait too long]
N --> K
K --> P{Response OK?}
P -- Yes --> Q[Return response]
P -- 429 --> R{attempt 1?}
R -- Yes --> S[sleep jitter 1s
continue]
R -- No --> T[switch account or
header-pool fallback]
S --> K
T --> I
Reviews (7): Last reviewed commit: "fix(plugin): harden fallback and quota h..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0af63b86d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isGemini35FlashModel(modelWithoutQuota)) { | ||
| antigravityModel = `${baseName}-low`; |
There was a problem hiding this comment.
When users select the newly exposed antigravity-gemini-3.5-flash-low model, extractThinkingTierFromModel() does not strip -low because Gemini 3.5 is excluded from thinking-tier support, so baseName is still gemini-3.5-flash-low; this branch then resolves it to gemini-3.5-flash-low-low, causing requests for that configured model to target a non-existent API model.
Useful? React with 👍 / 👎.
| limit: { context: 1048576, output: 65536 }, | ||
| modalities: DEFAULT_MODALITIES, | ||
| }, | ||
| "gemini-3.1-pro-preview": { |
There was a problem hiding this comment.
Adding the new Gemini CLI model IDs with dotted versions means their retrieveUserQuota buckets will be dropped by the quota UI: aggregateGeminiCliQuota() in src/plugin/quota.ts still only keeps modelId.startsWith("gemini-3-") or gemini-2.5-pro, so buckets for gemini-3.1-pro-preview (and the new gemini-3.5-flash) are filtered out and users see missing/empty CLI quota for these configured models.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/plugin.ts (2)
824-837:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh-token updates leave the in-memory email index stale.
After updating an existing account,
indexByEmailis not updated when email changes (or is newly set). In the sameresultsbatch, this can create duplicates for the same email.✅ Localized fix
const existing = accounts[existingIndex]; if (!existing) { continue; } + const previousEmail = existing.email; const oldToken = existing.refreshToken; accounts[existingIndex] = { ...existing, email: result.email ?? existing.email, refreshToken: parts.refreshToken, projectId: parts.projectId ?? existing.projectId, managedProjectId: parts.managedProjectId ?? existing.managedProjectId, lastUsed: now, }; + const updatedEmail = accounts[existingIndex]?.email; + if (previousEmail && previousEmail !== updatedEmail) { + indexByEmail.delete(previousEmail); + } + if (updatedEmail) { + indexByEmail.set(updatedEmail, existingIndex); + } if (oldToken !== parts.refreshToken) { indexByRefreshToken.delete(oldToken); indexByRefreshToken.set(parts.refreshToken, existingIndex); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin.ts` around lines 824 - 837, The accounts update block updates refresh-token index but misses updating the in-memory email index, causing stale/duplicate email mappings; after computing the new account object (accounts[existingIndex] = { ...existing, email: result.email ?? existing.email, ... }), compare the previous email (existing.email) to the new email (result.email ?? existing.email) and, if different, remove the old mapping from indexByEmail (if present) and set indexByEmail.set(newEmail, existingIndex) when newEmail is non-empty; ensure this logic mirrors the refresh-token handling around indexByRefreshToken so the email index stays consistent within the same results batch.
768-770:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUnreadable storage is treated as empty, which can silently overwrite existing accounts.
When
replaceAllisfalse, anullfromloadAccounts()is treated as “no accounts” and then persisted immediately. Ifnullcame from a read/parse/permission failure, this path can replace existing on-disk data with a new single-account set.💡 Suggested direction
- const stored = replaceAll ? null : await loadAccounts(); - const accounts = stored?.accounts ? [...stored.accounts] : []; + // Distinguish "missing file" from "unreadable/corrupt file" before merging. + // Example: use a load API that returns { kind: "ok" | "missing" | "unreadable", storage? }. + const storedResult = replaceAll ? { kind: "missing" as const } : await loadAccountsDetailed(); + if (!replaceAll && storedResult.kind === "unreadable") { + throw new Error("Account storage exists but could not be read. Aborting merge to prevent overwrite."); + } + const accounts = storedResult.kind === "ok" ? [...storedResult.storage.accounts] : [];Also applies to: 849-857
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin.ts` around lines 768 - 770, The current logic treats a null return from loadAccounts() as “no accounts” and proceeds to overwrite storage; instead detect and propagate a load error: change the block using replaceAll, loadAccounts, stored and accounts so that if loadAccounts() returns null (or signals an unreadable state) and replaceAll is false you do not treat it as an empty account list—either abort the save, surface/log the error and keep existing on-disk data, or require explicit replaceAll=true to overwrite. Concretely, check loadAccounts()'s return for an error/unreadable flag before setting accounts (do not default null to []), and only proceed to persist when stored is valid or when replaceAll is true.
🧹 Nitpick comments (3)
src/plugin/auth.test.ts (1)
26-138: 💤 Low valueConsider adding a test for JSON with empty-string field values.
The current tests cover all major parsing scenarios well. One minor edge case not explicitly tested: JSON input with empty string values like
{"r":"token","p":"","m":""}. This would validate the|| undefinednormalization inparseRefreshParts. The implementation handles it correctly (empty strings becomeundefined), but an explicit test would document this behavior.💡 Suggested additional test
it("parses JSON format with empty string p/m as undefined", () => { const result = parseRefreshParts('{"r":"token","p":"","m":""}'); expect(result).toEqual({ refreshToken: "token", projectId: undefined, managedProjectId: undefined, }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin/auth.test.ts` around lines 26 - 138, Add a test case to the parseRefreshParts suite that verifies JSON input with empty-string fields normalizes those fields to undefined; specifically, in the describe("parseRefreshParts") block add a new it that calls parseRefreshParts('{"r":"token","p":"","m":""}') and expects { refreshToken: "token", projectId: undefined, managedProjectId: undefined } to document and lock in the existing || undefined normalization behavior in the parseRefreshParts function.src/plugin/rate-limit-parser.ts (2)
55-58: 💤 Low valueResponse body is never read; body parsing silently skipped.
When a
Responseobject is passed,bodyTextis set to""(line 57-58), so the body parsing phase (lines 60-117) is always skipped. If the caller has already consumed the response body, they must pass the extracted text directly as a string for body-based patterns to work.Consider either:
- Documenting this behavior clearly in the JSDoc, or
- Accepting
{response: Response, bodyText?: string}to support both header and body parsing from a Response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin/rate-limit-parser.ts` around lines 55 - 58, The code sets bodyText = "" when responseOrBody is a Response, so body parsing never runs; update the input handling in the rate-limit parser (the code that uses responseOrBody and computes bodyText) to support reading or accepting the response body: change the function signature to accept either a string, a Response, or an object like { response: Response, bodyText?: string }, then compute bodyText by using the provided bodyText if present or by calling await response.text() when response is a Response (so you don't silently skip parsing if body is available), and keep existing header parsing paths unchanged; also update the JSDoc to document the new accepted shapes and the behavior when bodyText is pre-supplied to avoid re-consuming an already-read Response.
131-138: Remove the signature-mismatch concern; consolidate duplicated rate-limit types/parsers.
src/plugin.tscallsparseRateLimitReason(bodyText, response.status)fromsrc/plugin/rate-limit-parser.ts, which matches that function’s 2-arg signature.RateLimitReasonis defined in bothsrc/plugin/rate-limit-parser.tsandsrc/plugin/accounts.ts, andsrc/plugin/accounts.tsalso has a separate 3-argparseRateLimitReason(reason, message, status?).parseRateLimitReasonfromsrc/plugin/accounts.tsappears only insrc/plugin/accounts.test.ts, so the runtime path currently usesrate-limit-parser.ts.Consider making
RateLimitReasonandparseRateLimitReasonoriginate from a single canonical module (or have one module re-export/wrap the other) to prevent drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin/rate-limit-parser.ts` around lines 131 - 138, There are duplicate RateLimitReason and parseRateLimitReason implementations across rate-limit-parser.ts and accounts.ts which can drift; consolidate them by making a single canonical export (keep the implementation in parseRateLimitReason and RateLimitReason in src/plugin/rate-limit-parser.ts) and update src/plugin/accounts.ts to import and re-export (or call) that parseRateLimitReason/RateLimitReason instead of defining its own 3-arg variant; remove the duplicate type and function in accounts.ts (or replace usages in accounts.test.ts to call the canonical parseRateLimitReason(bodyText, status?)) so all runtime and tests use the same symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugin/quota.ts`:
- Line 204: The statement assigning quotaUserAgent from
getAntigravityHeaders()["User-Agent"] is missing a trailing semicolon which can
cause syntax/style failures; update the declaration of quotaUserAgent in
function/module where it appears (the line using quotaUserAgent and
getAntigravityHeaders()) to add a semicolon at the end of the statement so it
reads as a properly terminated const declaration.
In `@src/plugin/rate-limit-parser.ts`:
- Around line 144-162: The parseRateLimitReason implementation checks quota
patterns before capacity ones and treats "resource_exhausted" as quota; reorder
and align its pattern checks to match accounts.ts by checking capacity-related
patterns (e.g., "model_capacity_exhausted", "capacity_exhausted", "model is
currently overloaded", "service temporarily unavailable", and the phrase
"resource exhausted" with a space) before quota patterns, and ensure both
underscore/space variants of "resource exhausted" are classified as
MODEL_CAPACITY_EXHAUSTED so the retry/backoff behavior is consistent with the
parseRateLimitReason in accounts.ts.
In `@src/plugin/transform/model-resolver.ts`:
- Around line 236-239: When forcing Gemini 3.5 Flash to the "-low" tier (inside
the isGemini35FlashModel branch), defensively strip any existing tier suffix
from the incoming model name before appending "-low" so you don't end up with
double suffixes; use the existing extractThinkingTierFromModel/TIER_REGEX logic
(or run TIER_REGEX against modelWithoutQuota/baseName) to remove any recognized
or unrecognized tier suffix and then set antigravityModel =
`${strippedBase}-low`. Ensure this change is applied in the isGemini35FlashModel
branch where antigravityModel is assigned so manually specified tiers like
"-high" are removed first.
In `@src/plugin/usage-tracker.ts`:
- Around line 38-39: The in-memory UsageHistory variable (history: UsageHistory)
is unbounded and never pruned—so modify the usage-tracker to enforce a retention
cap (e.g., keep only the last N hourly entries) whenever history is updated
and/or when resetSessionUsage() is called; implement a helper (e.g.,
pruneHistoryToRetention) that trims older hourly keys from history after
merges/updates in functions that update totalStats or history (reference
history, resetSessionUsage, and any merge/update routines) and/or add an
explicit clearHistory/resetHistory API to allow manual resets.
- Around line 114-120: getUsageHistory() currently returns a shallow copy that
exposes internal mutable objects (history) to callers; change it to return a
deep copy so callers cannot mutate tracker internals. In getUsageHistory(),
iterate the sorted keys and for each history[key] (the per-hour / per-family
bucket object) create and assign a new object clone (copying primitive fields
and nested maps/objects) rather than assigning the original reference; ensure
the returned UsageHistory contains independent copies of all nested bucket
objects so future reads/writes to the tracker are not affected by external
mutations.
- Around line 94-99: The token-accumulation block in usage-tracker.ts currently
uses truthy checks that allow NaN/negative values to corrupt totals; for each
field (e.g., usage.inputTokens, usage.outputTokens, usage.thinkingOutputTokens,
usage.responseOutputTokens, usage.cacheReadTokens, usage.cacheWriteTokens)
validate that the value is a finite number and >= 0 (e.g.,
Number.isFinite(value) && value >= 0) before mutating the corresponding
totalStats property (totalStats.inputTokens, totalStats.outputTokens, etc.);
apply this guard to each add operation to reject invalid numeric values prior to
updating totals.
---
Outside diff comments:
In `@src/plugin.ts`:
- Around line 824-837: The accounts update block updates refresh-token index but
misses updating the in-memory email index, causing stale/duplicate email
mappings; after computing the new account object (accounts[existingIndex] = {
...existing, email: result.email ?? existing.email, ... }), compare the previous
email (existing.email) to the new email (result.email ?? existing.email) and, if
different, remove the old mapping from indexByEmail (if present) and set
indexByEmail.set(newEmail, existingIndex) when newEmail is non-empty; ensure
this logic mirrors the refresh-token handling around indexByRefreshToken so the
email index stays consistent within the same results batch.
- Around line 768-770: The current logic treats a null return from
loadAccounts() as “no accounts” and proceeds to overwrite storage; instead
detect and propagate a load error: change the block using replaceAll,
loadAccounts, stored and accounts so that if loadAccounts() returns null (or
signals an unreadable state) and replaceAll is false you do not treat it as an
empty account list—either abort the save, surface/log the error and keep
existing on-disk data, or require explicit replaceAll=true to overwrite.
Concretely, check loadAccounts()'s return for an error/unreadable flag before
setting accounts (do not default null to []), and only proceed to persist when
stored is valid or when replaceAll is true.
---
Nitpick comments:
In `@src/plugin/auth.test.ts`:
- Around line 26-138: Add a test case to the parseRefreshParts suite that
verifies JSON input with empty-string fields normalizes those fields to
undefined; specifically, in the describe("parseRefreshParts") block add a new it
that calls parseRefreshParts('{"r":"token","p":"","m":""}') and expects {
refreshToken: "token", projectId: undefined, managedProjectId: undefined } to
document and lock in the existing || undefined normalization behavior in the
parseRefreshParts function.
In `@src/plugin/rate-limit-parser.ts`:
- Around line 55-58: The code sets bodyText = "" when responseOrBody is a
Response, so body parsing never runs; update the input handling in the
rate-limit parser (the code that uses responseOrBody and computes bodyText) to
support reading or accepting the response body: change the function signature to
accept either a string, a Response, or an object like { response: Response,
bodyText?: string }, then compute bodyText by using the provided bodyText if
present or by calling await response.text() when response is a Response (so you
don't silently skip parsing if body is available), and keep existing header
parsing paths unchanged; also update the JSDoc to document the new accepted
shapes and the behavior when bodyText is pre-supplied to avoid re-consuming an
already-read Response.
- Around line 131-138: There are duplicate RateLimitReason and
parseRateLimitReason implementations across rate-limit-parser.ts and accounts.ts
which can drift; consolidate them by making a single canonical export (keep the
implementation in parseRateLimitReason and RateLimitReason in
src/plugin/rate-limit-parser.ts) and update src/plugin/accounts.ts to import and
re-export (or call) that parseRateLimitReason/RateLimitReason instead of
defining its own 3-arg variant; remove the duplicate type and function in
accounts.ts (or replace usages in accounts.test.ts to call the canonical
parseRateLimitReason(bodyText, status?)) so all runtime and tests use the same
symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 11aedbea-b81e-4b37-8b78-cb2f8063f067
📒 Files selected for processing (22)
docs/ANTIGRAVITY_API_SPEC.mdpackage.jsonscript/fix-esm-imports.mjssrc/constants.tssrc/plugin.tssrc/plugin/auth.test.tssrc/plugin/auth.tssrc/plugin/cli.tssrc/plugin/config/models.test.tssrc/plugin/config/models.tssrc/plugin/config/updater.test.tssrc/plugin/debug.tssrc/plugin/persist-account-pool.test.tssrc/plugin/quota-fallback.test.tssrc/plugin/quota.test.tssrc/plugin/quota.tssrc/plugin/rate-limit-parser.tssrc/plugin/request.test.tssrc/plugin/request.tssrc/plugin/transform/model-resolver.test.tssrc/plugin/transform/model-resolver.tssrc/plugin/usage-tracker.ts
📜 Review details
⏰ 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 (55)
script/fix-esm-imports.mjs (1)
1-107: LGTM!package.json (1)
37-37: LGTM!src/plugin/quota-fallback.test.ts (1)
32-32: LGTM!src/plugin/auth.ts (4)
5-7: LGTM!
13-37: LGTM!
43-48: LGTM!
50-72: LGTM!src/plugin/auth.test.ts (3)
6-24: LGTM!
140-194: LGTM!
196-263: LGTM!src/plugin.ts (1)
2-2: LGTM!Also applies to: 14-14, 1453-1453, 2695-2696, 2709-2710, 2719-2720, 2799-2800, 2810-2811, 2817-2818, 2838-2839, 2877-2878, 2881-2882
src/plugin/cli.ts (1)
22-30: LGTM!Also applies to: 162-163
src/plugin/persist-account-pool.test.ts (1)
15-16: LGTM!Also applies to: 38-74, 95-220, 245-447, 449-513, 515-582, 583-668
src/plugin/rate-limit-parser.ts (3)
164-184: LGTM!
14-53: LGTM!
60-125: LGTM!src/constants.ts (3)
14-21: LGTM!
29-54: LGTM!
56-59: LGTM!src/plugin/config/models.ts (2)
61-65: LGTM!
90-99: LGTM!docs/ANTIGRAVITY_API_SPEC.md (1)
79-86: LGTM!src/plugin/quota.ts (9)
18-27: LGTM!
123-136: LGTM!
238-272: LGTM!
331-341: LGTM!
343-354: LGTM!
356-409: LGTM!
411-458: LGTM!
460-483: LGTM!
485-573: LGTM!src/plugin/debug.ts (2)
526-540: LGTM!
542-554: LGTM!src/plugin/quota.test.ts (3)
1-100: LGTM!
103-189: LGTM!
191-298: LGTM!src/plugin/config/models.test.ts (2)
22-22: LGTM!Also applies to: 28-28
53-58: LGTM!src/plugin/config/updater.test.ts (2)
69-71: LGTM!
239-239: LGTM!src/plugin/transform/model-resolver.ts (7)
64-68: LGTM!
80-95: LGTM!
105-109: LGTM!Also applies to: 143-150
160-180: LGTM!
202-218: LGTM!
261-262: LGTM!Also applies to: 291-299
361-366: LGTM!Also applies to: 370-402
src/plugin/request.ts (1)
1550-1556: LGTM!src/plugin/request.test.ts (2)
1039-1049: LGTM!
1087-1097: LGTM!src/plugin/transform/model-resolver.test.ts (5)
30-36: LGTM!
63-104: LGTM!
137-149: LGTM!
229-233: LGTM!Also applies to: 247-254, 266-271
313-317: LGTM!Also applies to: 339-343
| }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
| const quotaUserAgent = getAntigravityHeaders()["User-Agent"] || "antigravity/windows/amd64" |
There was a problem hiding this comment.
Missing semicolon causes syntax error.
Line 204 is missing a semicolon at the end of the statement. While JavaScript's ASI (Automatic Semicolon Insertion) may handle this in some cases, it's a syntax error in strict contexts and violates the project's code style.
🐛 Proposed fix
- const quotaUserAgent = getAntigravityHeaders()["User-Agent"] || "antigravity/windows/amd64"
+ const quotaUserAgent = getAntigravityHeaders()["User-Agent"] || "antigravity/windows/amd64";📝 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 quotaUserAgent = getAntigravityHeaders()["User-Agent"] || "antigravity/windows/amd64" | |
| const quotaUserAgent = getAntigravityHeaders()["User-Agent"] || "antigravity/windows/amd64"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugin/quota.ts` at line 204, The statement assigning quotaUserAgent from
getAntigravityHeaders()["User-Agent"] is missing a trailing semicolon which can
cause syntax/style failures; update the declaration of quotaUserAgent in
function/module where it appears (the line using quotaUserAgent and
getAntigravityHeaders()) to add a semicolon at the end of the statement so it
reads as a properly terminated const declaration.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugin.ts`:
- Around line 2042-2062: parseRateLimitResetMs currently receives only bodyText,
which bypasses header/date-based parsing and can under-estimate retry delays;
update the logic in the block around bodyText, parseRateLimitResetMs,
retryAfterMsFromResponse, and extractRetryInfoFromBody so header-derived retry
values are preserved — either call parseRateLimitResetMs with both the response
(or its headers) and bodyText, or first compute headerRetryMs via
retryAfterMsFromResponse(response) and only override it when
parseRateLimitResetMs(bodyText) returns a non-null value; ensure serverRetryMs
selection uses the header-derived value as the fallback rather than dropping
header parsing entirely.
In `@src/plugin/request.ts`:
- Around line 1817-1832: The streaming path currently returns early at
response.body.pipeThrough(streamingTransformer) and never calls trackUsage(...)
or logUsageSummary(...), so SSE success cases under-report; update the streaming
completion path (the code that constructs/uses streamingTransformer and the
response.body.pipeThrough(...) flow) to invoke the same
trackUsage(effectiveModel, {...}) and logUsageSummary(effectiveModel, {...})
call with the same fields (use usage.promptTokenCount,
usage.candidatesTokenCount, usage.thoughtsTokenCount,
usage.cachedContentTokenCount with ?? 0 defaults) when the stream successfully
finishes (and also ensure the error path still logs on failure), effectively
mirroring the buffered fallback block so both streamingTransformer completion
and buffered fallback invoke the same telemetry functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80e45436-c20c-4dd4-a778-73d14145e548
📒 Files selected for processing (7)
src/plugin.tssrc/plugin/cli.tssrc/plugin/quota.test.tssrc/plugin/quota.tssrc/plugin/request.tssrc/plugin/transform/model-resolver.tssrc/plugin/ui/auth-menu.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugin/cli.ts
- src/plugin/transform/model-resolver.ts
- src/plugin/quota.ts
- src/plugin/quota.test.ts
📜 Review details
🔇 Additional comments (3)
src/plugin.ts (1)
43-50: LGTM!Also applies to: 1057-1057, 2593-2597, 2735-2748
src/plugin/request.ts (1)
28-28: LGTM!src/plugin/ui/auth-menu.ts (1)
17-26: LGTM!Also applies to: 55-64
| // Get body text once for enhanced rate-limit parsing | ||
| let bodyText = "" | ||
| try { | ||
| bodyText = await response.clone().text() | ||
| } catch { | ||
| // Ignore body read failures | ||
| } | ||
|
|
||
| // [Enhanced Parsing] Use server's exact reset time from error body | ||
| const parsedResetMs = parseRateLimitResetMs(bodyText) | ||
|
|
||
| const defaultRetryMs = (config.default_retry_after_seconds ?? 60) * 1000; | ||
| const maxBackoffMs = (config.max_backoff_seconds ?? 60) * 1000; | ||
| const headerRetryMs = retryAfterMsFromResponse(response, defaultRetryMs); | ||
| const bodyInfo = await extractRetryInfoFromBody(response); | ||
| const serverRetryMs = bodyInfo.retryDelayMs ?? headerRetryMs; | ||
|
|
||
| // [Enhanced Parsing] Pass status to handling logic | ||
| const rateLimitReason = parseRateLimitReason(bodyInfo.reason, bodyInfo.message, response.status); | ||
| // Use parsed reset time if available, with MIN_BACKOFF_MS floor, else fallback to header/body | ||
| const rawServerRetryMs = bodyInfo.retryDelayMs ?? headerRetryMs | ||
| const serverRetryMs = parsedResetMs !== null | ||
| ? Math.max(parsedResetMs, MIN_BACKOFF_MS) | ||
| : rawServerRetryMs |
There was a problem hiding this comment.
Preserve header-based reset parsing when deriving retry delay.
parseRateLimitResetMs is called with bodyText, which skips its header parsing phase. That drops support for reset headers/date-form retry-after, and can undercompute wait times under throttle pressure.
Suggested patch
- const parsedResetMs = parseRateLimitResetMs(bodyText)
+ const parsedResetMs =
+ parseRateLimitResetMs(response) ??
+ parseRateLimitResetMs(bodyText)📝 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.
| // Get body text once for enhanced rate-limit parsing | |
| let bodyText = "" | |
| try { | |
| bodyText = await response.clone().text() | |
| } catch { | |
| // Ignore body read failures | |
| } | |
| // [Enhanced Parsing] Use server's exact reset time from error body | |
| const parsedResetMs = parseRateLimitResetMs(bodyText) | |
| const defaultRetryMs = (config.default_retry_after_seconds ?? 60) * 1000; | |
| const maxBackoffMs = (config.max_backoff_seconds ?? 60) * 1000; | |
| const headerRetryMs = retryAfterMsFromResponse(response, defaultRetryMs); | |
| const bodyInfo = await extractRetryInfoFromBody(response); | |
| const serverRetryMs = bodyInfo.retryDelayMs ?? headerRetryMs; | |
| // [Enhanced Parsing] Pass status to handling logic | |
| const rateLimitReason = parseRateLimitReason(bodyInfo.reason, bodyInfo.message, response.status); | |
| // Use parsed reset time if available, with MIN_BACKOFF_MS floor, else fallback to header/body | |
| const rawServerRetryMs = bodyInfo.retryDelayMs ?? headerRetryMs | |
| const serverRetryMs = parsedResetMs !== null | |
| ? Math.max(parsedResetMs, MIN_BACKOFF_MS) | |
| : rawServerRetryMs | |
| // Get body text once for enhanced rate-limit parsing | |
| let bodyText = "" | |
| try { | |
| bodyText = await response.clone().text() | |
| } catch { | |
| // Ignore body read failures | |
| } | |
| // [Enhanced Parsing] Use server's exact reset time from error body | |
| const parsedResetMs = | |
| parseRateLimitResetMs(response) ?? | |
| parseRateLimitResetMs(bodyText) | |
| const defaultRetryMs = (config.default_retry_after_seconds ?? 60) * 1000; | |
| const maxBackoffMs = (config.max_backoff_seconds ?? 60) * 1000; | |
| const headerRetryMs = retryAfterMsFromResponse(response, defaultRetryMs); | |
| const bodyInfo = await extractRetryInfoFromBody(response); | |
| // Use parsed reset time if available, with MIN_BACKOFF_MS floor, else fallback to header/body | |
| const rawServerRetryMs = bodyInfo.retryDelayMs ?? headerRetryMs | |
| const serverRetryMs = parsedResetMs !== null | |
| ? Math.max(parsedResetMs, MIN_BACKOFF_MS) | |
| : rawServerRetryMs |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugin.ts` around lines 2042 - 2062, parseRateLimitResetMs currently
receives only bodyText, which bypasses header/date-based parsing and can
under-estimate retry delays; update the logic in the block around bodyText,
parseRateLimitResetMs, retryAfterMsFromResponse, and extractRetryInfoFromBody so
header-derived retry values are preserved — either call parseRateLimitResetMs
with both the response (or its headers) and bodyText, or first compute
headerRetryMs via retryAfterMsFromResponse(response) and only override it when
parseRateLimitResetMs(bodyText) returns a non-null value; ensure serverRetryMs
selection uses the header-derived value as the fallback rather than dropping
header parsing entirely.
| if (usage && effectiveModel) { | ||
| trackUsage(effectiveModel, { | ||
| inputTokens: usage.promptTokenCount ?? 0, | ||
| outputTokens: usage.candidatesTokenCount ?? 0, | ||
| thinkingOutputTokens: usage.thoughtsTokenCount ?? 0, | ||
| responseOutputTokens: 0, | ||
| cacheReadTokens: usage.cachedContentTokenCount ?? 0, | ||
| cacheWriteTokens: 0, | ||
| }) | ||
| logUsageSummary(effectiveModel, { | ||
| inputTokens: usage.promptTokenCount ?? 0, | ||
| outputTokens: usage.candidatesTokenCount ?? 0, | ||
| thinkingOutputTokens: usage.thoughtsTokenCount ?? 0, | ||
| cacheReadTokens: usage.cachedContentTokenCount ?? 0, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate streaming transformer implementation"
fd -i 'streaming' src/plugin
echo "2) Find createStreamingTransformer definition and call sites"
rg -n --type=ts 'createStreamingTransformer\s*\(' src/plugin -C 3
echo "3) Check where usage tracking is performed"
rg -n --type=ts '\btrackUsage\s*\(|\blogUsageSummary\s*\(|extractUsageFromSsePayload\s*\(' src/plugin -C 2Repository: NoeFabris/opencode-antigravity-auth
Length of output: 8085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "A) Read control-flow around the alleged early return in src/plugin/request.ts"
sed -n '1660,1720p' src/plugin/request.ts
echo
echo "B) Read the snippet region + nearby context in src/plugin/request.ts"
sed -n '1760,1850p' src/plugin/request.ts
echo
echo "C) Find all trackUsage/logUsageSummary usages in the repo"
rg -n --type=ts '\btrackUsage\s*\(' src/plugin
rg -n --type=ts '\blogUsageSummary\s*\(' src/plugin
echo
echo "D) Check whether streaming transformer invokes usage tracking"
rg -n --type=ts 'trackUsage\s*\(|logUsageSummary\s*\(|extractUsageFromSsePayload\s*\(' src/plugin/core/streamingRepository: NoeFabris/opencode-antigravity-auth
Length of output: 6543
Usage telemetry is skipped for successful SSE streaming
src/plugin/request.ts returns early for the successful SSE streaming path (response.body.pipeThrough(streamingTransformer)), so the trackUsage(...)/logUsageSummary(...) block (lines 1817-1832) in the buffered fallback path never runs. There are no other trackUsage(...)/logUsageSummary(...) call sites in the streaming layer (repo-wide, they only appear in src/plugin/request.ts), so successful SSE traffic will under-report session totals unless the stream completion path tracks usage elsewhere.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugin/request.ts` around lines 1817 - 1832, The streaming path currently
returns early at response.body.pipeThrough(streamingTransformer) and never calls
trackUsage(...) or logUsageSummary(...), so SSE success cases under-report;
update the streaming completion path (the code that constructs/uses
streamingTransformer and the response.body.pipeThrough(...) flow) to invoke the
same trackUsage(effectiveModel, {...}) and logUsageSummary(effectiveModel,
{...}) call with the same fields (use usage.promptTokenCount,
usage.candidatesTokenCount, usage.thoughtsTokenCount,
usage.cachedContentTokenCount with ?? 0 defaults) when the stream successfully
finishes (and also ensure the error path still logs on failure), effectively
mirroring the buffered fallback block so both streamingTransformer completion
and buffered fallback invoke the same telemetry functions.
…, recover ESM fixer, x-goog-api-key stripping, usage-tracker guards - rate-limit-parser.ts: capacity checks before quota, word-boundary regex for status codes - ESM fixer: recovered from judgment-day fixes, removed redundant patterns - request.ts: strip x-goog-api-key to prevent sentinel leak - usage-tracker.ts: history pruning (7d), deep clone, NaN guard - model-resolver.ts: strip all tier suffixes before appending -low for 3.5 Flash
amartidandqdq
force-pushed
the
fix/consolidate-all-fixes
branch
from
e1f14e4 to
f66022f
Compare
…t, JSON auth mocks - quota.ts: recovered full parallel version from 3596d16 with withTimeout, Promise.all, getSubscriptionTier - debug.ts: added logCreditCost and logSubscriptionTier exports - quota.test.ts: updated auth mocks to JSON format, re-enabled all 11 tests - All review issues resolved: rate-limit ordering, SERVER_ERROR boundaries, ESM fixer, usage-tracker guards, x-goog-api-key stripping, model-resolver tier strip
|
Follow-up pushed for bot review:
Validation on Windows:pm run typecheckpm test → 36 files, 1020 passed, 25 todopm run build → runs ESM fixer and rewrites 40 dist files |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/plugin/transform/model-resolver.ts (1)
277-292: 💤 Low valueConsider validating for duplicate normalized affinity keys at config load time.
resolveModelAccountAffinityEmailiteratesObject.entries(affinity)and returns the first email whose normalized key matches. If multiple entries inconfig.model_account_affinitynormalize to the same key (e.g.,"claude-sonnet-4-6"and"antigravity-claude-sonnet-4-6"), the first match wins, which may surprise users. A config-time check inschema.tsor duringloadConfigthat detects and warns about collisions would improve the user experience.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin/transform/model-resolver.ts` around lines 277 - 292, resolveModelAccountAffinityEmail can yield surprising results when multiple config.model_account_affinity keys normalize to the same value; add a config-time validation that detects duplicate normalized affinity keys and surfaces a clear error or warning. In the config loading path (e.g., loadConfig or schema.ts) iterate Object.entries(config.model_account_affinity), apply normalizeModelAccountAffinityKey to each configuredModel, track seen normalized keys, and if a collision is found report the conflict including the original configuredModel strings and their associated emails (or throw/exit to fail fast) so users must resolve duplicates before runtime; use the same normalizeModelAccountAffinityKey function to ensure consistency with resolveModelAccountAffinityEmail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/plugin/transform/model-resolver.ts`:
- Around line 277-292: resolveModelAccountAffinityEmail can yield surprising
results when multiple config.model_account_affinity keys normalize to the same
value; add a config-time validation that detects duplicate normalized affinity
keys and surfaces a clear error or warning. In the config loading path (e.g.,
loadConfig or schema.ts) iterate Object.entries(config.model_account_affinity),
apply normalizeModelAccountAffinityKey to each configuredModel, track seen
normalized keys, and if a collision is found report the conflict including the
original configuredModel strings and their associated emails (or throw/exit to
fail fast) so users must resolve duplicates before runtime; use the same
normalizeModelAccountAffinityKey function to ensure consistency with
resolveModelAccountAffinityEmail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a1f5ce09-8bd2-4fbe-9ff4-50e513c06359
📒 Files selected for processing (13)
assets/antigravity.schema.jsondocs/MULTI-ACCOUNT.mdpackage.jsonscript/fix-esm-imports.mjssrc/plugin.tssrc/plugin/accounts.test.tssrc/plugin/accounts.tssrc/plugin/config/models.test.tssrc/plugin/config/models.tssrc/plugin/config/schema.test.tssrc/plugin/config/schema.tssrc/plugin/transform/model-resolver.test.tssrc/plugin/transform/model-resolver.ts
✅ Files skipped from review due to trivial changes (1)
- docs/MULTI-ACCOUNT.md
🚧 Files skipped from review as they are similar to previous changes (1)
- script/fix-esm-imports.mjs
📜 Review details
⏰ 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 (15)
package.json (2)
37-37: LGTM!
67-67: LGTM!assets/antigravity.schema.json (1)
167-182: LGTM!src/plugin.ts (1)
1564-1595: LGTM!src/plugin/transform/model-resolver.test.ts (1)
2-26: LGTM!src/plugin/config/models.test.ts (1)
23-23: LGTM!Also applies to: 51-56
src/plugin/config/schema.ts (2)
296-314: LGTM!
489-490: LGTM!src/plugin/config/models.ts (1)
70-80: LGTM!src/plugin/accounts.ts (4)
158-169: LGTM!
229-231: LGTM!
531-547: LGTM!
624-671: LGTM!src/plugin/accounts.test.ts (1)
647-781: LGTM!src/plugin/config/schema.test.ts (1)
50-78: LGTM!
|
Update pushed in \26a81a8\. Verification:
I also removed the unverified \�ntigravity-gemini-3.5-flash\ provider entry because both bare \gemini-3.5-flash\ and \gemini-3.5-flash-medium\ returned CloudCode 404 in local runtime tests. Better to avoid exposing a broken Antigravity model until the real CloudCode ID is confirmed. |
|
Latest verification after 1459e04: typecheck passed; npm test passed with 37 files, 1022 tests passed, 25 todo; npm run build passed and cleaned dist before tsc, ESM fixer updated 39 dist files; npm pack dry-run is clean with 239 files and no usage-tracker/rate-limit-parser entries; npm pack produced a 239-file tarball; runtime smoke passed locally for google/antigravity-gemini-3-flash => READY and google/antigravity-gemini-3.1-pro => READY; opencode models google exposes verified Antigravity models only, with no google/antigravity-gemini-3.5-flash entry. CodeRabbit is passing; Greptile Review is still pending/in progress. |
Avoid duplicate account file writes while preserving refreshed-token durability.
1 participant
Summary
Consolidates all community fixes + reverse-engineered improvements for opencode-antigravity-auth. 1048 tests pass, typecheck clean. Tested on Windows with OpenCode Desktop.
Credits — Community PRs integrated
Fixes included
1. Claude API Key Sentinel (fixes #579)
apiKey: ""→apiKey: "antigravity-oauth"— passes OpenCode's non-empty validation. Custom fetch handles real auth.2. ESM Import Specifiers (fixes #564, #568)
Post-build script
script/fix-esm-imports.mjsrewrites bare relative ESM specifiers to explicit.js//index.jspaths. Handles re-exports, type imports, bare imports,.mjs. Windows-safe with POSIX path resolution.3. TUI Output Overwrite (fixes #537)
promptContinue()waits for Enter after quota checks, verification, account toggles — prevents menu from overwriting output.4. Legacy Model Aliases
LEGACY_MODEL_ALIASES+normalizeLegacyModel()transparently remap stale model IDs (Gemini 3 Pro→3.1 Pro, Claude 4.5→4.6). Tier suffixes preserved.5. Gemini 3.5 Flash + openid (fixes #578, #572, #573)
Added
antigravity-gemini-3.5-flash-lowmodel definition +isGemini35FlashModel()resolution. AddedopenidOAuth scope.6. Endpoint Fallback Order
daily-cloudcode-pa.googleapis.com(non-sandbox) first,cloudcode-pa.googleapis.com(prod) second. Matches official CLI behavior.7. Subscription Tier Detection
getSubscriptionTier()via loadCodeAssist API — detects free/pro/ultra. Displayed in quota check output.8. Rate-Limit Parser (NEW:
rate-limit-parser.ts)Parses
quotaResetDelay/quotaResetTimeStampfrom 429 responses for exact server reset times. Wired into retry handler.9. Token Usage Tracker (NEW:
usage-tracker.ts)Local
ModelUsageStatstracking per model (input/output/thinking/cache tokens). "View my usage" in TUI menu + live tracking via SSE stream.10. Credit Cost Logging
logCreditCost()logs AI credit cost per call from ChatModelMetadata.Verification
Files Changed
20 source files, 3 new modules, 7 test files updated. Full diff: ~2500 insertions, ~300 deletions.
Related Issues
Closes #579, Closes #564, Closes #568, Closes #537
Addresses #578, #572, #573 (Gemini 3.5 Flash)
Based on #580, #565, #562, #569, #575