feat(pi): add Antigravity multi-account routing parity - #15
Open
ComputerByte wants to merge 7 commits into
Open
Conversation
added 7 commits
September 9, 2026 16:27
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
21 issues found across 31 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/pi/src/settings.ts">
<violation number="1" location="packages/pi/src/settings.ts:30">
P2: When the settings file is `[]` or either field is explicitly `null`, `readSettings` silently accepts it and enables the defaults. Reject arrays and apply defaults only when properties are absent so invalid settings fail closed as documented.</violation>
</file>
<file name="packages/core/src/rate-limit-response.ts">
<violation number="1" location="packages/core/src/rate-limit-response.ts:2">
P3: This rate-limit/cooldown parsing logic moved into the shared core package is now on the critical path for both the OpenCode and Pi harnesses, but none of the three functions (retryAfterMsFromResponse, extractRateLimitBodyInfo, parseDurationToMs) has a unit test. The `parseDurationToMs` compound-duration regex and the multi-section error-details scanning in `extractRateLimitBodyInfo` are the kind of parsing logic most prone to regressions. Add a small test file for rate-limit-response.ts covering header precedence, RetryInfo vs metadata quotaResetDelay vs message-based fallbacks, and compound-duration parsing before this shared logic is relied on further.</violation>
<violation number="2" location="packages/core/src/rate-limit-response.ts:15">
P2: When a server sends the valid HTTP-date form of `Retry-After`, `retryAfterMsFromResponse` treats it as invalid and uses the default cooldown. Parse HTTP-date values after delay-seconds so routing honors the server's reset time.</violation>
<violation number="3" location="packages/core/src/rate-limit-response.ts:36">
P2: When an upstream error has a non-string `error.message`, `extractRateLimitBodyInfo` calls `.match()` on that value and aborts routing instead of failing over. Validate `message` with `typeof message === 'string'` before using it.</violation>
</file>
<file name="packages/core/src/account-storage.ts">
<violation number="1" location="packages/core/src/account-storage.ts:214">
P2: When normalization removes an account before the persisted active index, loading the pool can switch the active account because the loader never remaps `activeIndex` or `activeIndexByFamily`. Remap those indices through deduplication, or retain the survivor at the removed account’s slot.</violation>
<violation number="2" location="packages/core/src/account-storage.ts:214">
P2: When case/whitespace-variant email records carry different stable `accountId` values, this normalization silently drops one account instead of preserving or rejecting the identity conflict. Detect conflicting account IDs before deduplication and fail closed, matching the existing reconciliation behavior.</violation>
<violation number="3" location="packages/core/src/account-storage.ts:214">
P2: When a persisted v4 record contains a truthy non-string `email`, `.trim()` throws instead of returning the typed unreadable-storage error. Validate optional `email` as a string before normalization, so malformed files follow the existing backup and fail-closed path.</violation>
</file>
<file name="packages/pi/src/runtime.ts">
<violation number="1" location="packages/pi/src/runtime.ts:108">
P2: When an OAuth refresh hangs during quota refresh, the quota manager cannot cancel it because this call drops its timeout signal. Pass `signal` to `credentialFor` so quota refreshes honor the manager's timeout and dispose cancellation.</violation>
<violation number="2" location="packages/pi/src/runtime.ts:553">
P2: `dispatch` awaits `refreshQuota()` on every request before routing. In `fetchAccountQuota` each stale account does `credentialFor` (which runs `refreshToken` inside a fenced `mutateAccountStorage` lock) plus a network quota fetch plus a `patch`, so once an account's cache passes the 30-minute `QUOTA_REFRESH_MS` window the next interactive request serially refreshes every stale account's quota before the actual generation request is sent. This adds wall-clock latency (and lock serialization with peer processes) to the request critical path. Consider refreshing quota off the request path or in parallel.</violation>
<violation number="3" location="packages/pi/src/runtime.ts:617">
P2: `markAccountUsed` is called before the provider request is sent, but core's `AccountManager.markAccountUsed` documents it "Should be called AFTER request completion, not during account selection." Calling it here updates `account.lastUsed` to dispatch time, and the following `patch` persists `current.lastUsed = max(...)` even when the request then fails (transport error) or fails over via 429/503/529/500. A failing account is thus scored as freshly used, biasing the freshness/LRU scoring against it on the very next request.</violation>
</file>
<file name="packages/core/src/persist-account-pool.ts">
<violation number="1" location="packages/core/src/persist-account-pool.ts:175">
P2: When a pool write follows different Claude and Gemini cursors, this resets both family routes to `activeIndex` and changes subsequent account selection. Preserve and independently clamp `current.activeIndexByFamily` in the non-replace branch.</violation>
<violation number="2" location="packages/core/src/persist-account-pool.ts:213">
P2: When identity reconciliation collapses an enabled survivor with an operator-disabled duplicate, this merge loses the disable flag and makes the account routable again. Keep `enabled` false when either proven duplicate is disabled.</violation>
<violation number="3" location="packages/core/src/persist-account-pool.ts:296">
P1: When identity resolution has an email but no `accountId`, matching records with different stored account IDs are merged instead of failing closed. Compare each candidate ID with the current survivor ID as well as the optional resolved ID before removing the candidate.</violation>
</file>
<file name="packages/core/src/account-manager.ts">
<violation number="1" location="packages/core/src/account-manager.ts:572">
P2: When a Google account's email changes while its stable `accountId` remains unchanged, `reconcileStorage()` treats it as a different account, especially after refresh-token rotation. Match by the stable `accountId` when available, with normalized email as a fallback, so health, token balance, session pins, and routing state stay with the account.</violation>
</file>
<file name="packages/pi/src/stream.ts">
<violation number="1" location="packages/pi/src/stream.ts:711">
P2: When `dispatch` returns a non-retryable HTTP error such as 400, this catch marks the account unhealthy even though the failure can be request-wide. Only record stream failures after an OK response; let `dispatch` own HTTP status handling.</violation>
</file>
<file name="packages/core/src/quota-manager.ts">
<violation number="1" location="packages/core/src/quota-manager.ts:120">
P2: defaultKeyOf places the new stable accountId fallback after the email check, so accountId is only consulted when no non-empty email exists. Since deduplicateAccountsByEmail also keys by email only, two records for the same physical account that differ in email can't be collapsed and will receive separate quota caches/backoffs. To deliver the stated 'stable account identity reconciliation' goal, prefer accountId over email (or reconcile by accountId when it is present) so the collision case the PR calls out is actually handled.</violation>
</file>
<file name="packages/pi/src/commands.ts">
<violation number="1" location="packages/pi/src/commands.ts:62">
P3: When a user types an invalid argument (e.g. `/agy-quota foobar`), the thrown `Error('Use /agy-quota [refresh]')` is swallowed by the `register` wrapper and replaced with the generic 'Check the account/ settings file, account number, or re-authenticate' notification. The usage hint is never shown, and the generic message points at the wrong problem. Surface the specific error message (e.g. rethrow it or only apply the generic credential-talk), or let `register` include `error.message` for non-Ambiguity errors.</violation>
</file>
<file name="packages/core/src/rotation.ts">
<violation number="1" location="packages/core/src/rotation.ts:601">
P2: When two AccountManager instances manage different account pools, the process-global single-slot `trackerLayouts` makes the second manager read the first manager's identity layout as its `previous`, then remaps its own global health/token state through the foreign layout. The `ownsExpectedLayout` guard does not prevent this — it only switches to the fallback identity-matching path, which still iterates the other manager's identities. Key the stored layout by something pool-scoped, or only trust `trackerLayouts` when the caller's `previousIdentities` match (which the no-op check already computes) rather than the last caller.</violation>
</file>
<file name="packages/pi/src/provider.test.ts">
<violation number="1" location="packages/pi/src/provider.test.ts:397">
P3: This assertion does not prove that the partial stream is emitted only once. A replay regression could append two `partial` text blocks and still pass. Assert the complete text content or count matching blocks exactly once.</violation>
<violation number="2" location="packages/pi/src/provider.test.ts:398">
P3: This test does not verify that failover selects another account. Assert that the two transport calls use `Bearer access-1` and `Bearer access-2` so a same-account retry cannot satisfy the test.</violation>
</file>
<file name="packages/core/src/account-manager.test.ts">
<violation number="1" location="packages/core/src/account-manager.test.ts:155">
P2: In this test `health.recordFailure(0)` is called twice on account 'a', so its score drops to 30 (70-20-20), yet the expected values list index 1 as `[50, 43]`, which only holds if a single failure was applied. After the two reconciliations account 'a' lands at tracker index 1 preserving its (30, 43) state, so the expected value appears to be `[30, 43]`. Verify whether the double failure should apply and correct the assertion, otherwise this remapping case is not asserting the state it intends.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| if ( | ||
| emailMatch && | ||
| candidate.accountId && | ||
| accountId && |
There was a problem hiding this comment.
P1: When identity resolution has an email but no accountId, matching records with different stored account IDs are merged instead of failing closed. Compare each candidate ID with the current survivor ID as well as the optional resolved ID before removing the candidate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/persist-account-pool.ts, line 296:
<comment>When identity resolution has an email but no `accountId`, matching records with different stored account IDs are merged instead of failing closed. Compare each candidate ID with the current survivor ID as well as the optional resolved ID before removing the candidate.</comment>
<file context>
@@ -0,0 +1,368 @@
+ if (
+ emailMatch &&
+ candidate.accountId &&
+ accountId &&
+ candidate.accountId !== accountId
+ ) {
</file context>
| 'Cannot read Pi Antigravity routing settings; repair the settings file', | ||
| ) | ||
| } | ||
| if (!value || typeof value !== 'object') |
There was a problem hiding this comment.
P2: When the settings file is [] or either field is explicitly null, readSettings silently accepts it and enables the defaults. Reject arrays and apply defaults only when properties are absent so invalid settings fail closed as documented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi/src/settings.ts, line 30:
<comment>When the settings file is `[]` or either field is explicitly `null`, `readSettings` silently accepts it and enables the defaults. Reject arrays and apply defaults only when properties are absent so invalid settings fail closed as documented.</comment>
<file context>
@@ -0,0 +1,62 @@
+ 'Cannot read Pi Antigravity routing settings; repair the settings file',
+ )
+ }
+ if (!value || typeof value !== 'object')
+ throw new Error('Invalid Pi Antigravity settings')
+ const config = value as Record<string, unknown>
</file context>
Comment on lines
+15
to
+20
| if (retryAfterHeader) { | ||
| const parsed = Number.parseInt(retryAfterHeader, 10) | ||
| if (!Number.isNaN(parsed) && parsed > 0) { | ||
| return parsed * 1000 | ||
| } | ||
| } |
There was a problem hiding this comment.
P2: When a server sends the valid HTTP-date form of Retry-After, retryAfterMsFromResponse treats it as invalid and uses the default cooldown. Parse HTTP-date values after delay-seconds so routing honors the server's reset time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/rate-limit-response.ts, line 15:
<comment>When a server sends the valid HTTP-date form of `Retry-After`, `retryAfterMsFromResponse` treats it as invalid and uses the default cooldown. Parse HTTP-date values after delay-seconds so routing honors the server's reset time.</comment>
<file context>
@@ -0,0 +1,157 @@
+ }
+
+ const retryAfterHeader = response.headers.get('retry-after')
+ if (retryAfterHeader) {
+ const parsed = Number.parseInt(retryAfterHeader, 10)
+ if (!Number.isNaN(parsed) && parsed > 0) {
</file context>
Suggested change
| if (retryAfterHeader) { | |
| const parsed = Number.parseInt(retryAfterHeader, 10) | |
| if (!Number.isNaN(parsed) && parsed > 0) { | |
| return parsed * 1000 | |
| } | |
| } | |
| if (retryAfterHeader) { | |
| if (/^\d+$/.test(retryAfterHeader)) { | |
| const parsed = Number(retryAfterHeader) | |
| if (Number.isFinite(parsed) && parsed >= 0) { | |
| return parsed * 1000 | |
| } | |
| } | |
| const retryAt = Date.parse(retryAfterHeader) | |
| if (!Number.isNaN(retryAt)) { | |
| return Math.max(0, retryAt - Date.now()) | |
| } | |
| } |
Comment on lines
+36
to
+39
| const message = | ||
| error && typeof error === 'object' | ||
| ? (error as { message?: string }).message | ||
| : undefined |
There was a problem hiding this comment.
P2: When an upstream error has a non-string error.message, extractRateLimitBodyInfo calls .match() on that value and aborts routing instead of failing over. Validate message with typeof message === 'string' before using it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/rate-limit-response.ts, line 36:
<comment>When an upstream error has a non-string `error.message`, `extractRateLimitBodyInfo` calls `.match()` on that value and aborts routing instead of failing over. Validate `message` with `typeof message === 'string'` before using it.</comment>
<file context>
@@ -0,0 +1,157 @@
+ if (!body || typeof body !== 'object') return { retryDelayMs: null }
+
+ const error = (body as { error?: unknown }).error
+ const message =
+ error && typeof error === 'object'
+ ? (error as { message?: string }).message
</file context>
Suggested change
| const message = | |
| error && typeof error === 'object' | |
| ? (error as { message?: string }).message | |
| : undefined | |
| const message = | |
| error && | |
| typeof error === 'object' && | |
| typeof (error as { message?: unknown }).message === 'string' | |
| ? (error as { message: string }).message | |
| : undefined |
| } | ||
|
|
||
| const existingIndex = emailToNewestIndex.get(acc.email) | ||
| const email = acc.email.trim().toLowerCase() |
There was a problem hiding this comment.
P2: When normalization removes an account before the persisted active index, loading the pool can switch the active account because the loader never remaps activeIndex or activeIndexByFamily. Remap those indices through deduplication, or retain the survivor at the removed account’s slot.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/account-storage.ts, line 214:
<comment>When normalization removes an account before the persisted active index, loading the pool can switch the active account because the loader never remaps `activeIndex` or `activeIndexByFamily`. Remap those indices through deduplication, or retain the survivor at the removed account’s slot.</comment>
<file context>
@@ -211,15 +211,20 @@ export function deduplicateAccountsByEmail<
}
- const existingIndex = emailToNewestIndex.get(acc.email)
+ const email = acc.email.trim().toLowerCase()
+ if (!email) {
+ indicesToKeep.add(i)
</file context>
| manager.reconcileStorage(next) | ||
| expect(values()).toEqual([ | ||
| [70, 50], | ||
| [50, 43], |
There was a problem hiding this comment.
P2: In this test health.recordFailure(0) is called twice on account 'a', so its score drops to 30 (70-20-20), yet the expected values list index 1 as [50, 43], which only holds if a single failure was applied. After the two reconciliations account 'a' lands at tracker index 1 preserving its (30, 43) state, so the expected value appears to be [30, 43]. Verify whether the double failure should apply and correct the assertion, otherwise this remapping case is not asserting the state it intends.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/account-manager.test.ts, line 155:
<comment>In this test `health.recordFailure(0)` is called twice on account 'a', so its score drops to 30 (70-20-20), yet the expected values list index 1 as `[50, 43]`, which only holds if a single failure was applied. After the two reconciliations account 'a' lands at tracker index 1 preserving its (30, 43) state, so the expected value appears to be `[30, 43]`. Verify whether the double failure should apply and correct the assertion, otherwise this remapping case is not asserting the state it intends.</comment>
<file context>
@@ -42,6 +43,455 @@ const stored: AccountStorageV4 = {
+ manager.reconcileStorage(next)
+ expect(values()).toEqual([
+ [70, 50],
+ [50, 43],
+ [70, 50],
+ ])
</file context>
| @@ -0,0 +1,157 @@ | |||
| /** Reads `retry-after-ms` / `retry-after` headers, in that order. */ | |||
There was a problem hiding this comment.
P3: This rate-limit/cooldown parsing logic moved into the shared core package is now on the critical path for both the OpenCode and Pi harnesses, but none of the three functions (retryAfterMsFromResponse, extractRateLimitBodyInfo, parseDurationToMs) has a unit test. The parseDurationToMs compound-duration regex and the multi-section error-details scanning in extractRateLimitBodyInfo are the kind of parsing logic most prone to regressions. Add a small test file for rate-limit-response.ts covering header precedence, RetryInfo vs metadata quotaResetDelay vs message-based fallbacks, and compound-duration parsing before this shared logic is relied on further.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/rate-limit-response.ts, line 2:
<comment>This rate-limit/cooldown parsing logic moved into the shared core package is now on the critical path for both the OpenCode and Pi harnesses, but none of the three functions (retryAfterMsFromResponse, extractRateLimitBodyInfo, parseDurationToMs) has a unit test. The `parseDurationToMs` compound-duration regex and the multi-section error-details scanning in `extractRateLimitBodyInfo` are the kind of parsing logic most prone to regressions. Add a small test file for rate-limit-response.ts covering header precedence, RetryInfo vs metadata quotaResetDelay vs message-based fallbacks, and compound-duration parsing before this shared logic is relied on further.</comment>
<file context>
@@ -0,0 +1,157 @@
+/** Reads `retry-after-ms` / `retry-after` headers, in that order. */
+export function retryAfterMsFromResponse(
+ response: Response,
+ defaultRetryMs: number = 60_000,
</file context>
| `Quota refresh failed for ${failures} account(s); cached values retained.`, | ||
| 'warning', | ||
| ) | ||
| } else if (args.trim()) throw new Error('Use /agy-quota [refresh]') |
There was a problem hiding this comment.
P3: When a user types an invalid argument (e.g. /agy-quota foobar), the thrown Error('Use /agy-quota [refresh]') is swallowed by the register wrapper and replaced with the generic 'Check the account/ settings file, account number, or re-authenticate' notification. The usage hint is never shown, and the generic message points at the wrong problem. Surface the specific error message (e.g. rethrow it or only apply the generic credential-talk), or let register include error.message for non-Ambiguity errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi/src/commands.ts, line 62:
<comment>When a user types an invalid argument (e.g. `/agy-quota foobar`), the thrown `Error('Use /agy-quota [refresh]')` is swallowed by the `register` wrapper and replaced with the generic 'Check the account/ settings file, account number, or re-authenticate' notification. The usage hint is never shown, and the generic message points at the wrong problem. Surface the specific error message (e.g. rethrow it or only apply the generic credential-talk), or let `register` include `error.message` for non-Ambiguity errors.</comment>
<file context>
@@ -0,0 +1,140 @@
+ `Quota refresh failed for ${failures} account(s); cached values retained.`,
+ 'warning',
+ )
+ } else if (args.trim()) throw new Error('Use /agy-quota [refresh]')
+ ctx.ui.notify(await runtime.describe(), 'info')
+ },
</file context>
| const result = await stream.result() | ||
| expect(result.stopReason).toBe('error') | ||
| expect(result.content).toContainEqual({ type: 'text', text: 'partial' }) | ||
| expect(transport).toHaveBeenCalledTimes(2) |
There was a problem hiding this comment.
P3: This test does not verify that failover selects another account. Assert that the two transport calls use Bearer access-1 and Bearer access-2 so a same-account retry cannot satisfy the test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi/src/provider.test.ts, line 398:
<comment>This test does not verify that failover selects another account. Assert that the two transport calls use `Bearer access-1` and `Bearer access-2` so a same-account retry cannot satisfy the test.</comment>
<file context>
@@ -0,0 +1,404 @@
+ const result = await stream.result()
+ expect(result.stopReason).toBe('error')
+ expect(result.content).toContainEqual({ type: 'text', text: 'partial' })
+ expect(transport).toHaveBeenCalledTimes(2)
+ const bodies = transport.mock.calls.map((call) =>
+ JSON.parse((call[1] as RequestInit).body as string),
</file context>
Suggested change
| expect(transport).toHaveBeenCalledTimes(2) | |
| expect( | |
| transport.mock.calls.map( | |
| (call) => | |
| ((call[1] as RequestInit).headers as Record<string, string>) | |
| .Authorization, | |
| ), | |
| ).toEqual(['Bearer access-1', 'Bearer access-2']) |
| ) | ||
| const result = await stream.result() | ||
| expect(result.stopReason).toBe('error') | ||
| expect(result.content).toContainEqual({ type: 'text', text: 'partial' }) |
There was a problem hiding this comment.
P3: This assertion does not prove that the partial stream is emitted only once. A replay regression could append two partial text blocks and still pass. Assert the complete text content or count matching blocks exactly once.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi/src/provider.test.ts, line 397:
<comment>This assertion does not prove that the partial stream is emitted only once. A replay regression could append two `partial` text blocks and still pass. Assert the complete text content or count matching blocks exactly once.</comment>
<file context>
@@ -0,0 +1,404 @@
+ )
+ const result = await stream.result()
+ expect(result.stopReason).toBe('error')
+ expect(result.content).toContainEqual({ type: 'text', text: 'partial' })
+ expect(transport).toHaveBeenCalledTimes(2)
+ const bodies = transport.mock.calls.map((call) =>
</file context>
Suggested change
| expect(result.content).toContainEqual({ type: 'text', text: 'partial' }) | |
| expect(result.content).toEqual([{ type: 'text', text: 'partial' }]) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Pi multi-account parity for Antigravity, including durable account pooling, quota-aware routing, failover, operator controls, Pi 0.85.1 compatibility, stable account identity reconciliation, and live route diagnostics.
Verification
48460e8Previously identified issues verified fixed
/loginidentity loss / duplicate-account reconciliationLive validation
Real Pi 0.85.1 multi-account routing was exercised successfully.
With two eligible non-Gemini accounts and round-robin enabled: