Repository navigation
Conversation
ded582d to
31b18a4
Compare
Greptile SummaryThis PR replaces the previous Antigravity quota implementation (which mirrored the Gemini snapshot) with a fully independent credential authority that reads from the official
Confidence Score: 4/5The Antigravity quota path is well-isolated; a failure degrades gracefully to an unavailable status bar slot without affecting other providers. The core auth, single-flight refresh, and quota parsing logic is solid and well-tested. Three quality issues exist: a dead catch branch in the legacy fallback path, a potential refresh loop for credentials that store expiry as a Unix-second integer, and an orphaned .tmp credential file if the atomic rename step fails. None affect the primary happy path against a real agy account. Files Needing Attention: src/main/rate-limits/antigravity-token-refresh.ts (temp-file cleanup), src/main/rate-limits/antigravity-auth.ts (numeric expiry unit), src/main/rate-limits/antigravity-usage-fetcher.ts (dead catch branch)
|
| Filename | Overview |
|---|---|
| src/main/rate-limits/antigravity-token-refresh.ts | New file: single-flight OAuth token refresh with per-source AbortController fanout. Temp file cleanup gap on rename failure is a minor resource leak. |
| src/main/rate-limits/antigravity-auth.ts | New file: credential loading with keychain/token-file fallback. parseExpiry numeric branch could mis-treat Unix-second timestamps as milliseconds. |
| src/main/rate-limits/antigravity-usage-fetcher.ts | New file: quota fetcher via loadCodeAssist → retrieveUserQuotaSummary with 404/405/501 fallback. Dead catch branch in loadQuota legacy path cannot be reached. |
| src/main/rate-limits/antigravity-keychain.ts | New file: cross-platform keychain adapters. Abort/timeout handling is solid. |
| src/main/rate-limits/service.ts | Replaces mirrored Gemini snapshot with an independent fetchAntigravityRateLimits call in the parallel allSettled batch. |
| src/renderer/src/components/status-bar/tooltip.tsx | Adds deduplication logic to prevent a weekly bucket appearing twice. Uses value-equality rather than reference equality. |
| src/renderer/src/lib/stop-dashboard-agent.ts | New file: tears down one agent pane from the main renderer. Correctly orders drop-before-close to prevent retention resurrection. |
| src/main/ipc/dashboard-popout.ts | Adds dashboardPopout:stopAgent IPC handler with origin and payload validation. |
Sequence Diagram
sequenceDiagram
participant SVC as RateLimitService
participant FET as antigravity-usage-fetcher
participant AUTH as antigravity-auth
participant KC as antigravity-keychain
participant REF as antigravity-token-refresh
participant API as cloudcode-pa.googleapis.com
participant GOA as oauth2.googleapis.com
SVC->>FET: fetchAntigravityRateLimits
FET->>AUTH: getAntigravityAccessToken
AUTH->>KC: readAntigravityKeyring
KC-->>AUTH: found value
AUTH-->>FET: AntigravityAccessToken
FET->>API: POST loadCodeAssist
API-->>FET: cloudaicompanionProject
FET->>API: POST retrieveUserQuotaSummary
alt buckets returned
API-->>FET: groups with buckets
else 404/405/501
FET->>API: POST retrieveUserQuota legacy
API-->>FET: buckets
end
FET-->>SVC: ProviderRateLimits antigravity
Note over AUTH,REF: On 401
AUTH->>REF: refreshAntigravitySingleFlight
REF->>GOA: POST token refresh
GOA-->>REF: new tokens
REF->>KC: writeAntigravityKeyring
REF-->>AUTH: RefreshedCredentials
Comments Outside Diff (3)
-
src/main/rate-limits/antigravity-usage-fetcher.ts, line 1787-1793 (link)Dead catch branch for
usage-unavailableThe condition
error.failureKind === 'usage-unavailable'can never be true here.postJsononly throwsAntigravityApiErrorwith kinds derived fromfailureKindForStatus(stale-token,missing-scope,rate-limited,server,unknown) or'network'/'parse'—'usage-unavailable'is not in that set.parseAntigravityQuotaBucketsnever throws (it returns[]on empty/unrecognised data). The branch was likely intended to catch the case where the caller ofloadQuota(i.e.quotaResult) throws, butquotaResultis invoked outsideloadQuota. The dead branch could mislead future maintainers into thinking empty-legacy responses are handled here. -
src/main/rate-limits/antigravity-token-refresh.ts, line 1334-1345 (link)renamefailswriteFilesucceeds and creates<tokenPath>.<pid>.tmp, thenrenamefails (e.g. cross-device or permissions). The outercatch {}block swallows the error and rethrows asAntigravityAuthError, but the.tmpfile is never removed. On the next successful refresh the new temp file will overwrite it (same process, same.pid), but a crash between the two calls leaves an orphaned credential fragment on disk. Consider adding afinallycleanup:await unlink(temporaryPath).catch(() => {})after therenamethrows. -
src/renderer/src/components/status-bar/tooltip.tsx, line 1007-1029 (link)weeklyIsAlreadyBucketuses value equality, not identityThe deduplication check compares
windowMinutes,usedPercent, andresetsAtby value. Two distinct quota windows that happen to share the same numeric values (e.g. two providers both at 25 % with the same reset time by coincidence) would cause one to be silently dropped from the tooltip. A reference-based check (comparing the bucket object itself againstp.weekly) would be unambiguous, sincequotaResultderivesp.weeklydirectly from the corresponding bucket viawithoutName. This is a cosmetic risk rather than a correctness one, but worth noting for future providers that might expose coincidental overlaps.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "fix(dashboard): remove unused validation..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function parseExpiry(value: string | number | undefined): number | null { | ||
| if (typeof value === 'number') { | ||
| return Number.isFinite(value) ? value : null | ||
| } |
There was a problem hiding this comment.
expiry treated as milliseconds but Unix timestamps are in seconds
parseExpiry returns a numeric expiry value unchanged, and isAccessTokenFresh compares it directly with Date.now() (milliseconds since epoch, ~1.75 × 10¹²). A credential file that stores expiry as a Unix second timestamp (e.g. 1780000000) would always satisfy expiresAtMs - Date.now() < TOKEN_SKEW_MS, causing the token to be treated as perpetually expired and triggering a refresh on every call. The real agy CLI appears to use ISO-8601 strings, so this only affects edge cases — but the type allows number and a comment clarifying the expected unit (or a conversion guard) would prevent a future regression.
| function parseExpiry(value: string | number | undefined): number | null { | |
| if (typeof value === 'number') { | |
| return Number.isFinite(value) ? value : null | |
| } | |
| function parseExpiry(value: string | number | undefined): number | null { | |
| if (typeof value === 'number') { | |
| // Why: treat numbers as milliseconds if they look like ms-epoch (> year 2001), | |
| // otherwise assume Unix seconds and convert. The official agy CLI always | |
| // writes ISO-8601 strings; the number branch is a defensive fallback. | |
| if (!Number.isFinite(value)) { | |
| return null | |
| } | |
| return value > 1_000_000_000_000 ? value : value * 1000 | |
| } |
📝 WalkthroughWalkthroughThe changes add dashboard agent-stop support across shared types, preload IPC, renderer controls, confirmation dialogs, terminal teardown, tests, and localization. They also add Antigravity authentication, cross-platform keyring access, OAuth refresh, quota parsing, usage fetching, provider-specific service state, status-bar visibility updates, and related test coverage. Merge Risk: 🟠 High · up to The PR adds Antigravity credential rotation and quota access, but the current implementation can expose OAuth tokens to local processes on macOS and can store rotated credentials under the wrong Windows item, potentially causing authentication loss. These security and availability risks should be fixed or explicitly accepted before merge. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/main/rate-limits/antigravity-token-refresh.ts (1)
169-180: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove the temporary file when the atomic write fails.
If
writeFilesucceeds andrenamefails,${tokenPath}.${process.pid}.tmpstays in~/.gemini/antigravity-cli/. The file holds a valid refresh token at mode0o600. Repeated failures accumulate credential files that no code ever reads or deletes.Unlink the temporary path in the failure path.
♻️ Proposed refactor
const temporaryPath = `${credentials.tokenPath}.${process.pid}.tmp` - await writeFile(temporaryPath, JSON.stringify(envelope, null, 2), { - encoding: 'utf8', - mode: 0o600 - }) - await rename(temporaryPath, credentials.tokenPath) + try { + await writeFile(temporaryPath, JSON.stringify(envelope, null, 2), { + encoding: 'utf8', + mode: 0o600 + }) + await rename(temporaryPath, credentials.tokenPath) + } catch (error) { + await rm(temporaryPath, { force: true }).catch(() => undefined) + throw error + }Add the import:
import { rename, rm, writeFile } from 'node:fs/promises'src/main/rate-limits/antigravity-auth.ts (1)
195-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared cancellation helpers.
createAbortErroris duplicated insrc/main/rate-limits/antigravity-token-refresh.tsat lines 199-201.isAbortErrorappears in this cohort inantigravity-keychain.ts,antigravity-token-refresh.ts, andantigravity-usage-fetcher.ts.Move both into a single module, for example
src/main/rate-limits/antigravity-request-cancellation.ts, so the abort contract stays consistent across the auth, keyring, and fetch paths.src/main/rate-limits/antigravity-keychain.test.ts (1)
75-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Linux
secret-toolcoverage.The suite covers macOS and Windows but not Linux.
readLinuxSecretServiceuses an error mapping that is inverted relative toreadMacKeychain, as noted atsrc/main/rate-limits/antigravity-keychain.tslines 110-118. A Linux test would pin the intended mapping.Add two cases with
setPlatform('linux'): an absent item returnsmissing, and a Secret Service failure returnsunavailable.src/main/rate-limits/antigravity-usage-fetcher.ts (1)
134-168: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnreachable branch in the legacy quota fallback.
The check at Line 160 (
error.failureKind === 'usage-unavailable') inside the legacy-endpoint catch can never be true.postJsononly ever throws with failureKind'network','parse', or a value fromfailureKindForStatus('stale-token' | 'missing-scope' | 'rate-limited' | 'server' | 'unknown'). Nothing sets'usage-unavailable'before this catch runs, becauseparseAntigravityQuotaBucketsnever throws — an empty legacy result returns[]successfully and skips the catch entirely.If the legacy endpoint succeeds but returns zero recognized buckets,
loadQuotareturns[], andquotaResult([])later throws a different, generic'usage-unavailable'error without thesummaryError?.statuscontext this branch was written to preserve. Check the parsed array length instead of relying on an error path that never fires.🐛 Proposed fix
try { const legacy = await postJson(LEGACY_QUOTA_URL, { project: projectId }, accessToken, signal) - return parseAntigravityQuotaBuckets(legacy) + const legacyBuckets = parseAntigravityQuotaBuckets(legacy) + if (legacyBuckets.length === 0) { + throw new AntigravityApiError( + 'Antigravity quota endpoint returned no recognized quota buckets', + { status: summaryError?.status ?? null, failureKind: 'usage-unavailable' } + ) + } + return legacyBuckets } catch (error) { - if (error instanceof AntigravityApiError && error.failureKind === 'usage-unavailable') { - throw new AntigravityApiError( - 'Antigravity quota endpoint returned no recognized quota buckets', - { status: summaryError?.status ?? null, failureKind: 'usage-unavailable' } - ) - } throw error }Add a test that mocks the summary endpoint as unsupported (404) and the legacy endpoint as returning zero recognized buckets, to lock in the intended behavior. Do you want me to draft that test?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8e3af19-9c01-4551-b7a3-5415122a630e
📥 Commits
Reviewing files that changed from the base of the PR and between 0ae9174 and 31b18a46072cde6b24243d9b6bfec99bba0ed670.
📒 Files selected for processing (38)
src/main/ipc/dashboard-payload-validation.tssrc/main/ipc/dashboard-popout.test.tssrc/main/ipc/dashboard-popout.tssrc/main/rate-limits/antigravity-auth-types.tssrc/main/rate-limits/antigravity-auth.test.tssrc/main/rate-limits/antigravity-auth.tssrc/main/rate-limits/antigravity-keychain.test.tssrc/main/rate-limits/antigravity-keychain.tssrc/main/rate-limits/antigravity-quota-parser.tssrc/main/rate-limits/antigravity-token-refresh.tssrc/main/rate-limits/antigravity-usage-fetcher.test.tssrc/main/rate-limits/antigravity-usage-fetcher.tssrc/main/rate-limits/service.test.tssrc/main/rate-limits/service.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanCard.test.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanCard.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanCardStopControl.tsxsrc/renderer/src/components/dashboard-popout/AgentStopConfirmDialog.tsxsrc/renderer/src/components/dashboard/useDashboardPopoutBridge.tssrc/renderer/src/components/status-bar/StatusBar.tsxsrc/renderer/src/components/status-bar/UsageRosterPanel.tsxsrc/renderer/src/components/status-bar/status-bar-provider-visibility.test.tssrc/renderer/src/components/status-bar/status-bar-provider-visibility.tssrc/renderer/src/components/status-bar/tooltip.test.tssrc/renderer/src/components/status-bar/tooltip.tsxsrc/renderer/src/components/status-bar/usage-provider-settings-target.test.tssrc/renderer/src/components/status-bar/usage-provider-settings-target.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/lib/stop-dashboard-agent.test.tssrc/renderer/src/lib/stop-dashboard-agent.tssrc/shared/dashboard-snapshot.ts
|
Thanks for the thorough credential-authority and refresh work here. I opened #14571 as a related alternative and linked this PR in its comparison section. The main architectural difference is quota authority:
Cross-linking the alternatives so maintainers can choose between credential-owned and runtime-owned discovery with the tradeoffs visible. Thanks for incorporating the earlier maintainer feedback in this implementation. |
…ktree Clearing a finished agent off the kanban board required opening its worktree, finding the tab, and closing it there — the card only disappeared once the pane died. For idle agents, which is most of what accumulates on the board, that is three navigations to dismiss one card. Add a per-card stop control in the top-right corner, where the state dot sits (the dot fades on hover so they never collide). Idle and done cards confirm inline: the X becomes a red "Stop agent?" pill that commits on a second click and reverts on mouse-leave, blur, Escape, or a 3s timeout. Anything still running routes through a modal reusing CloseTerminalDialog's strings verbatim, so the board and Cmd+W say the same thing. No bulk "clear idle" action: idle means the agent is not working right now, not that its work is finished, so each dismissal stays deliberate. The row must be dropped BEFORE the pane is torn down. dropAgentStatus only plants its one-shot retention suppressor when a live entry still exists; reversed, the retention sync re-adds the agent as a retained "done" row and the card never leaves the board. Two teardown cases the naive "kill the pty" would get wrong: - A split tab hosts panes the user did not ask to close, so kill only this agent's pty and leave the tab standing. Single-pane tabs go through closeTerminalTab for full Cmd+W semantics. - A pinned tab's close confirmation would open a modal in the main window, which is behind the board and may be unattended. Stop the agent anyway via the onCancel path and leave the pinned tab alone — a button that silently does nothing reads as broken. The IPC deliberately does not raise the main window: the point is clearing an agent without leaving the board.
…d stop-agent flow - Clean up temporary credential file if atomic rename fails - Support numeric Unix-second expiry timestamps in credential parsing - Fix unreachable dead catch in legacy quota fallback and retry on 401/403 - Add Linux Secret Service test coverage and classification fixes - Dynamically re-resolve pending stop agent against live card snapshot - Clean up stop dialog if card disappears before confirmation Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
31b18a4 to
337e724
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/rate-limits/antigravity-usage-fetcher.ts (1)
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider importing the quota parser directly instead of re-exporting it.
Lines 20-21 re-export
parseAntigravityQuotaBucketsandAntigravityQuotaBucketfrom the fetcher. The only consumer isantigravity-usage-fetcher.test.ts. Service tests mock this module withfetchAntigravityRateLimitsalone, so the re-exported parser becomesundefinedin those suites. Import the parser from./antigravity-quota-parserin the test and drop the re-export to keep the module boundary narrow.src/main/rate-limits/antigravity-usage-fetcher.test.ts (1)
211-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting the 401 and 403 cases into separate tests.
This test covers two independent scenarios and resets mocks in the middle. A failure in the 403 half reports against a case name that mentions both codes. Use
it.each([401, 403])so each status fails independently.src/main/rate-limits/antigravity-token-refresh.ts (1)
176-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the original persistence failure for diagnostics.
The bare
catchdiscards the underlying error. A keychain rejection, anEACCESon the token file, and a missingtokenPathall surface as the samekeychain-unavailablemessage. Attach the original error ascauseso the failure can be diagnosed from logs.♻️ Proposed change
- } catch { + } catch (error) { throw new AntigravityAuthError( 'Antigravity rotated credentials could not be saved', - 'keychain-unavailable' + 'keychain-unavailable', + undefined, + { cause: error } ) }
AntigravityAuthErrormust accept and store the cause; adjust its constructor inantigravity-auth-types.tsif it does not.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cc3c405-76c9-4400-8c22-ddefd1b92e06
📒 Files selected for processing (46)
src/main/ipc/dashboard-payload-validation.tssrc/main/ipc/dashboard-popout.test.tssrc/main/ipc/dashboard-popout.tssrc/main/rate-limits/antigravity-auth-types.tssrc/main/rate-limits/antigravity-auth.test.tssrc/main/rate-limits/antigravity-auth.tssrc/main/rate-limits/antigravity-keychain.test.tssrc/main/rate-limits/antigravity-keychain.tssrc/main/rate-limits/antigravity-quota-parser.tssrc/main/rate-limits/antigravity-token-refresh.tssrc/main/rate-limits/antigravity-usage-fetcher.test.tssrc/main/rate-limits/antigravity-usage-fetcher.tssrc/main/rate-limits/rate-limit-service-test-harness.tssrc/main/rate-limits/service-account-target-selection.test.tssrc/main/rate-limits/service-inactive-account-previews.test.tssrc/main/rate-limits/service-live-claude-usage.test.tssrc/main/rate-limits/service-minimax-usage.test.tssrc/main/rate-limits/service-refresh-orchestration.test.tssrc/main/rate-limits/service-window-activation.test.tssrc/main/rate-limits/service.tssrc/preload/api/dashboard-api.tssrc/preload/index.tssrc/renderer/src/components/dashboard-popout/AgentKanbanBoard.test.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanCard.test.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanCard.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanCardStopControl.tsxsrc/renderer/src/components/dashboard-popout/AgentKanbanColumn.tsxsrc/renderer/src/components/dashboard-popout/AgentStopConfirmDialog.tsxsrc/renderer/src/components/dashboard/useDashboardPopoutBridge.tssrc/renderer/src/components/status-bar/StatusBar.tsxsrc/renderer/src/components/status-bar/UsageRosterPanel.tsxsrc/renderer/src/components/status-bar/status-bar-provider-visibility.test.tssrc/renderer/src/components/status-bar/status-bar-provider-visibility.tssrc/renderer/src/components/status-bar/tooltip.test.tssrc/renderer/src/components/status-bar/tooltip.tsxsrc/renderer/src/components/status-bar/usage-provider-settings-target.test.tssrc/renderer/src/components/status-bar/usage-provider-settings-target.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/lib/stop-dashboard-agent.test.tssrc/renderer/src/lib/stop-dashboard-agent.tssrc/shared/dashboard-snapshot.ts
🚧 Files skipped from review as they are similar to previous changes (29)
- src/renderer/src/i18n/locales/es.json
- src/renderer/src/components/status-bar/usage-provider-settings-target.ts
- src/renderer/src/components/status-bar/usage-provider-settings-target.test.ts
- src/main/ipc/dashboard-payload-validation.ts
- src/main/ipc/dashboard-popout.test.ts
- src/renderer/src/components/status-bar/UsageRosterPanel.tsx
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/components/status-bar/status-bar-provider-visibility.ts
- src/renderer/src/components/status-bar/tooltip.tsx
- src/main/rate-limits/antigravity-quota-parser.ts
- src/renderer/src/components/status-bar/tooltip.test.ts
- src/renderer/src/lib/stop-dashboard-agent.test.ts
- src/renderer/src/components/dashboard/useDashboardPopoutBridge.ts
- src/main/ipc/dashboard-popout.ts
- src/renderer/src/components/status-bar/StatusBar.tsx
- src/renderer/src/i18n/locales/ko.json
- src/renderer/src/lib/stop-dashboard-agent.ts
- src/renderer/src/components/dashboard-popout/AgentKanbanCard.tsx
- src/preload/index.ts
- src/renderer/src/components/status-bar/status-bar-provider-visibility.test.ts
- src/main/rate-limits/antigravity-auth.ts
- src/renderer/src/i18n/locales/zh.json
- src/renderer/src/i18n/locales/ja.json
- src/renderer/src/components/dashboard-popout/AgentKanbanBoard.tsx
- src/main/rate-limits/antigravity-auth-types.ts
- src/renderer/src/components/dashboard-popout/AgentKanbanCardStopControl.tsx
- src/shared/dashboard-snapshot.ts
- src/renderer/src/components/dashboard-popout/AgentKanbanCard.test.tsx
- src/main/rate-limits/service.ts
| export async function writeAntigravityKeyring(value: string, signal?: AbortSignal): Promise<void> { | ||
| if (process.platform === 'darwin') { | ||
| await runCommand( | ||
| 'security', | ||
| ['add-generic-password', '-U', '-s', KEYCHAIN_SERVICE, '-a', KEYCHAIN_ACCOUNT, '-w', value], | ||
| { signal } | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
macOS security add-generic-password read password from stdin -X argv exposure
💡 Result:
The macOS command-line utility security add-generic-password does not natively support reading a password from standard input (stdin) via the -w flag [1]. Consequently, passing a password as an argument to -w or -p exposes the secret in the process list (e.g., via ps or proc_pidinfo), where it is visible to other processes running under the same user account [2][1][3]. While modern macOS restricts non-root users from viewing the full argument lists of processes belonging to other users, the secret remains observable to any process running as the same user [1]. To avoid this command-line argument exposure, consider the following alternatives: 1. Interactive Prompting (Recommended for manual tasks): By omitting the password value after the -w flag, the security utility will interactively prompt the user to enter the password securely, preventing it from appearing in the shell history or process list [4][2][5]. 2. Keychain Access GUI: Create or manage keychain items directly through the Keychain Access application to bypass command-line exposure entirely [4][5]. 3. Direct API Access: For programmatic use, avoid the security CLI subprocess. Instead, use native language bindings (e.g., Swift/Objective-C/C) to call the Keychain Services API (specifically SecItemAdd or SecKeychainAddGenericPassword) directly within your process [1][6][3]. This ensures the secret is never exposed in the process argument list [3]. Note that some documented attempts to "pipe" stdin to the security utility via the -w flag are often mistaken or ineffective because the tool is designed to open an interactive retype prompt on /dev/tty when it detects no existing entry, which can block non-interactive processes or daemons [7].
Citations:
- 1: Security: Datadog API/App keys exposed via
securityargv during install mxnyawi/datadog-assistant#10 - 2: https://alsaheem.hashnode.dev/how-to-store-secrets-in-the-mac-keychain-and-use-them-like-environment-variables
- 3: raine/claude-code-proxy@a022041
- 4: https://apple.stackexchange.com/questions/313494/add-password-to-keychain-from-command-line-without-showing-them
- 5: https://www.netmeister.org/blog/keychain-passwords.html
- 6: Password exposed in command line on Mac zalando/go-keyring#24
- 7: lsm/HyperNeo@9d55185
🏁 Script executed:
#!/bin/bash
set -eu
file=$(git ls-files | rg '(^|/)antigravity-keychain\.ts$' | head -n 1)
printf '%s\n' "FILE=$file"
cat -n "$file"
printf '\n-- runCommand definitions and call sites --\n'
rg -n -A45 -B10 'function runCommand|const runCommand|export .*runCommand|runCommand\(' src
printf '\n-- platform-specific credential input --\n'
rg -n -A20 -B8 'writeAntigravityKeyring|stdin|add-generic-password|secret-tool|cmdkey|credential' srcRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
file=$(git ls-files | rg '(^|/)antigravity-keychain\.ts$' | head -n 1)
printf '%s\n' "FILE=$file"
cat -n "$file"
printf '\n-- imports and local command helper references --\n'
rg -n '^(import|export)|runCommand|spawn|execFile|stdin|add-generic-password|secret-tool|cmdkey' "$file"
printf '\n-- helper definitions only --\n'
rg -l 'function runCommand|const runCommand|export .*runCommand' src/main src/shared | head -n 30Repository: stablyai/orca
Length of output: 13200
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
p = Path("src/main/rate-limits/antigravity-keychain.ts")
s = p.read_text()
mac = re.search(
r"runCommand\(\s*'security',\s*\[(.*?)\],\s*\{ signal \}\s*\)",
s,
re.S,
)
linux = re.search(
r"runCommand\(\s*'secret-tool',\s*\[(.*?)\],\s*\{ input: value, signal \}\s*\)",
s,
re.S,
)
assert mac and linux
print("macOS call contains value in argv:", "value" in mac.group(1))
print("macOS call passes input:", "input:" in mac.group(0))
print("Linux call contains value in argv:", "value" in linux.group(1))
print("Linux call passes input:", "input: value" in linux.group(0))
print("macOS argv expression:", " ".join(mac.group(1).split()))
PYRepository: stablyai/orca
Length of output: 406
🌐 Web query:
site:keith.github.io/xcode-man-pages security.1 add-generic-password -w stdin
💡 Result:
The security command's add-generic-password function does not natively support reading a password directly from stdin through a flag [1]. According to the official security(1) manual, the -w option is used to specify the password. If you provide -w without an argument, or place it at the end of the command, the utility will interactively prompt you to enter the password [1]. This design is intended to prevent passwords from being exposed in the command history or process list [1][2]. If you need to automate adding a password from a script (e.g., using a pipe or redirection), the standard approach is to use the interactive prompt capability by not providing the password on the command line [1], or to use tools designed for keychain interaction if available. Note that some other macOS utilities, such as networksetup(8) or hdiutil(1), explicitly support reading from stdin (often via a hyphen -) [3][2], but security add-generic-password does not follow this pattern [1].
Citations:
- 1: https://keith.github.io/xcode-man-pages/security.1.html
- 2: https://keith.github.io/xcode-man-pages/hdiutil.1.html
- 3: https://keith.github.io/xcode-man-pages/networksetup.8.html
Avoid passing the token through security -w. The macOS execFile call exposes the token in the child process argument list. Use a native Keychain API instead; security add-generic-password has no non-interactive stdin mode.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
| const WINDOWS_CREDENTIAL_READ_SCRIPT = `${WINDOWS_CREDENTIAL_LIBRARY} | ||
| $target = 'gemini:antigravity' | ||
| $value = [OrcaAntigravityCredential]::Read($target) | ||
| if ($null -eq $value) { $value = [OrcaAntigravityCredential]::Read('gemini/antigravity') } | ||
| if ($null -ne $value) { | ||
| [Console]::Out.Write($value) | ||
| } | ||
| ` | ||
|
|
||
| const WINDOWS_CREDENTIAL_WRITE_SCRIPT = `${WINDOWS_CREDENTIAL_LIBRARY} | ||
| $inputValue = [Console]::In.ReadToEnd() | ||
| if ($inputValue.Length -eq 0 -or -not [OrcaAntigravityCredential]::Write('gemini:antigravity', $inputValue)) { | ||
| exit 1 | ||
| } | ||
| ` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The Windows read and write scripts do not target the same credential item.
WINDOWS_CREDENTIAL_READ_SCRIPT reads gemini:antigravity and falls back to gemini/antigravity. WINDOWS_CREDENTIAL_WRITE_SCRIPT always writes gemini:antigravity. If the official Antigravity CLI stored the record under gemini/antigravity, a rotation creates a second item. From then on Orca reads its own copy, and the CLI keeps the stale refresh token. Google invalidates the old refresh token after rotation, so the official CLI can lose access.
Write to the target that the read resolved, or update both items.
🛠️ Proposed fix
const WINDOWS_CREDENTIAL_WRITE_SCRIPT = `${WINDOWS_CREDENTIAL_LIBRARY}
$inputValue = [Console]::In.ReadToEnd()
-if ($inputValue.Length -eq 0 -or -not [OrcaAntigravityCredential]::Write('gemini:antigravity', $inputValue)) {
+if ($inputValue.Length -eq 0) { exit 1 }
+$target = 'gemini:antigravity'
+if ($null -eq [OrcaAntigravityCredential]::Read($target) -and
+ $null -ne [OrcaAntigravityCredential]::Read('gemini/antigravity')) {
+ $target = 'gemini/antigravity'
+}
+if (-not [OrcaAntigravityCredential]::Write($target, $inputValue)) {
exit 1
}
`📝 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 WINDOWS_CREDENTIAL_READ_SCRIPT = `${WINDOWS_CREDENTIAL_LIBRARY} | |
| $target = 'gemini:antigravity' | |
| $value = [OrcaAntigravityCredential]::Read($target) | |
| if ($null -eq $value) { $value = [OrcaAntigravityCredential]::Read('gemini/antigravity') } | |
| if ($null -ne $value) { | |
| [Console]::Out.Write($value) | |
| } | |
| ` | |
| const WINDOWS_CREDENTIAL_WRITE_SCRIPT = `${WINDOWS_CREDENTIAL_LIBRARY} | |
| $inputValue = [Console]::In.ReadToEnd() | |
| if ($inputValue.Length -eq 0 -or -not [OrcaAntigravityCredential]::Write('gemini:antigravity', $inputValue)) { | |
| exit 1 | |
| } | |
| ` | |
| const WINDOWS_CREDENTIAL_READ_SCRIPT = `${WINDOWS_CREDENTIAL_LIBRARY} | |
| $target = 'gemini:antigravity' | |
| $value = [OrcaAntigravityCredential]::Read($target) | |
| if ($null -eq $value) { $value = [OrcaAntigravityCredential]::Read('gemini/antigravity') } | |
| if ($null -ne $value) { | |
| [Console]::Out.Write($value) | |
| } | |
| ` | |
| const WINDOWS_CREDENTIAL_WRITE_SCRIPT = `${WINDOWS_CREDENTIAL_LIBRARY} | |
| $inputValue = [Console]::In.ReadToEnd() | |
| if ($inputValue.Length -eq 0) { exit 1 } | |
| $target = 'gemini:antigravity' | |
| if ($null -eq [OrcaAntigravityCredential]::Read($target) -and | |
| $null -ne [OrcaAntigravityCredential]::Read('gemini/antigravity')) { | |
| $target = 'gemini/antigravity' | |
| } | |
| if (-not [OrcaAntigravityCredential]::Write($target, $inputValue)) { | |
| exit 1 | |
| } | |
| ` |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
|
The dashboard stop-agent work in this branch is unrelated to the Antigravity quota change and is now split out as #16788, rebased onto current |
|
Closing this PR. I am not going to keep iterating contribution PRs here; if a report is useful I will file an issue instead. |
5 participants
Summary
This is a rework of the closed #9757, incorporating the maintainer feedback from issuecomment-5151463530.
loadCodeAssistandretrieveUserQuotaSummary.gemini-5h→Antigravity 5hgemini-weekly→Antigravity weekly3p-5h→3-party 5h3p-weekly→3-party weeklyDesign note: OAuth client pair
The current implementation uses the native-app OAuth client pair used by the official
agyCLI. These values identify the application OAuth client; they are not user account data, email, access tokens, refresh tokens, Keychain contents, or project IDs.GitHub Push Protection detected the client values as secret-like strings. The repository unblock approval was completed for this branch so the current official CLI-compatible design can be reviewed explicitly.
End-to-end validation
Verified locally against a real official
agyaccount:macOS Keychain (gemini/antigravity) → OAuth credential → loadCodeAssist → retrieveUserQuotaSummary → repository quota parserThe live response returned all four recognized quota identities with real usage percentages and reset timestamps. The values were passed through the repository parser; no mock quota values were used and no token values were logged.
Targeted validation:
Review request
Please review: