Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 17, 2026, 8:02 AM ET / 12:02 UTC (Revision 7). ClawSweeper reviewWhat this changesAdds automatic Antigravity quota recovery through agy when OAuth returns no quotas, using locally discovered email metadata to associate the report with the selected account, with tests and updated guidance. Merge readiness⛔ Blocked before merge - 15 items remain Keep open under the repository’s conservative review policy. The latest test-fixture repairs address two earlier findings, but all four previously reported production blockers remain. The selected-account recovery is distinct from the shipped explicit CLI option. Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s Antigravity provider combines selected Google-account credentials and local application or CLI usage sources. Its source-selection rules determine which account’s quotas appear in the menu bar. flowchart TD
A[Selected Google account] --> B[OAuth quota request]
B --> C{Quotas available?}
C -->|Yes| F[Account usage display]
C -->|No| D[Local email lookup]
D --> E[agy usage report]
E --> F
Decision needed
Why: Shipped behavior intentionally rejects identity-free reports, and the proposed metadata heuristic cannot establish ownership; choosing a supported integration contract requires owner input. Before merge
Findings
Agent review detailsSecurityNeeds attention: Account attribution and Keychain access cross existing safety boundaries without adequate authorization checks. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve explicit local-source recovery until a supported, credential-bound ownership check can safely associate each agy report with the selected account. Do we have a high-confidence way to reproduce the issue? Yes for the introduced defects: source establishes the Linux import failure, unknown-email acceptance, and direct Keychain access; supplied mocked output corroborates unknown-email acceptance. The linked reporter’s exact runtime failure remains unverified, and this review executed no tests. Is this the best way to solve the issue? No. Automatic recovery is useful, but arbitrary email metadata cannot safely authenticate an identity-free report; the existing explicit local-source option is the supported alternative. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against b6e65a83dc47. LabelsLabel justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
968af4b to
f99c5db
Compare
…fork-approval gate: zero jobs, conclusion action_required, identical for all 5 runs across 3 head commits of this cross-repository PR from Chener/CodexBar. A maintainer must approve the workflow; no code can change that. However, running the CI steps locally exposed a real code-caused break hidden behind the gate: the CodexBarTests target did not compile because Tests/CodexBarTests/AntigravityIdentityFallbackTests.swift used a non-concurrency-safe static handler, conformed a mock to the UsageFetcher struct as if it were a protocol, referenced a nonexistent CLICommand type, omitted all ClaudeUsageFetching requirements, and called nonexistent BrowserDetection.standard. Both macOS test shards would have failed once approved. Fix: rewrote only that test file, keeping the same two scenarios; it now builds the context like sibling suites, stubs the real cloudcode-pa.googleapis.com endpoints (loadCodeAssist 200, both quota endpoints 403), drives a fake agy honoring --version and -p /usage with a real report fixture, and asserts observable outputs (sourceLabel, primary usedPercent, identity, whether agy was launched). Regression property confirmed: with the base descriptor the matching-login test fails on all assertions; with the fallback it passes. Verified: swift build --build-tests passes; AntigravityIdentityFallbackTests 3 cases pass; ProviderSettingsDescriptorTests 54 pass; swiftformat --lint clean; swiftlint --strict 0 violations. Notes: this overlaps the declined review finding about these tests, but was required for a compiling test target and is confined to the test file. Separately, AntigravityCLIIdentityResolver.swift imports Security unconditionally (all other Core files guard with #if os(macOS)); the always-on build-linux-cli job will fail once approved. That was a declined finding and was left untouched; could not prove locally (no Linux SDK installed)
|
Thanks @Chener for identifying the selected-account OAuth-to-CLI fallback gap. The fetched head has changed the older concrete-fetcher mock issue. The implementation still cannot establish the required account match: it accepts the first email anywhere in settings or Keychain metadata, and an unknown selected email passes the helper. It also imports Security unconditionally and bypasses no-UI Keychain handling. I recommend closing this implementation while keeping #3662 open. The fallback design should use a verified active CLI identity field and retain the OAuth result when identity is missing or conflicting. |
|
Thank you for the careful review. I agree that this implementation does not establish the required account identity boundary and should remain closed. Let us keep #3662 open for a design based on verified active CLI identity, preserving the OAuth result when identity is missing or conflicting. |
What Changed
AntigravityOAuthFetchStrategynow re-runsagy -p /usagein Auto mode when the account-scoped OAuth fetch identifies a selected Google account but returns no model quotas (for example,retrieveUserQuotaanswering403), and only when the resolved localagylogin email matches that account. The report is attributed to the selected account with theclisource label; a missing email, a mismatch, a missing binary, or a failed report keeps the OAuth identity-only result (Limits not available).AntigravityCLIIdentityResolver, which resolves the localagylogin email from thegemini/antigravitygeneric-password Keychain item, else from the first email found in~/.gemini/antigravity-cli/settings.json,auth.json, orjetski_state.pbtxt.docs/antigravity.md, andCHANGELOG.mdto describe the identity-match rule, and addedAntigravityIdentityFallbackTestscovering the matched-identity fallback and the unmatched-identityLimits not availablepath with a stubbed OAuth transport and a fakeagybinary.Risk Assessment
🚨 High: No new findings, but the code is unchanged since the previous round and the declined error-level defects remain in place (CodexBarTests target cannot compile because MockUsageFetcher conforms to a concrete struct and MockClaudeFetcher omits three protocol requirements; the unconditional
import Securitybreaks the always-on Linux CLI build; the CLI fallback attributes an identity-freeagyreport to the selected account based on a guessed local file), so merging still requires the owner's explicit sign-off.Testing
Baseline:
swift build --build-testson the unmodified branch fails with eight compile errors in the branch's new Tests/CodexBarTests/AntigravityIdentityFallbackTests.swift (non-protocol conformance to the UsageFetcher struct, missing ClaudeUsageFetching requirements, unknown CLICommand and BrowserDetection.standard, non-Sendable static handler), so no test in the package can run on macOS and the branch's own two tests never execute; this was already declined in review and is reported only as observed state. To obtain behavioral signal I temporarily moved that file aside, added a transient Swift Testing harness that drives the real AntigravityOAuthFetchStrategy with URLSession.shared stubbed for cloudcode-pa.googleapis.com (loadCodeAssist 200, fetchAvailableModels 403, retrieveUserQuota 403) and a fakeagyselected via ANTIGRAVITY_CLI_PATH (real agy never executed), ran it with a focusedswift test --filter, then removed the harness and restored the branch file; the worktree is clean. All eight harness cases passed in-process: matching identity yields source "cli" with primary/secondary/four extra windows and the selected email; unprovable identity, foreign identity, failing or too-old agy, explicit oauth mode, auto mode without a selected account, and remote-quotas-present all keep the oauth result and never invoke agy. These harness runs are not live product validation and no scenario is marked pass on their basis. Two observations were recorded without findings because they map to declined review items: a selected account with no known email accepts any CLI identity and returns CLI quotas with a nil email (OBS-A), and on this machine's real agy install (settings.json and jetski_state.pbtxt present, no auth.json, no gemini/antigravity Keychain item) the resolver finds no email, so the fallback cannot fire here (OBS-B). No live end-to-end run or UI screenshot was possible: the remote host cannot be redirected in the real binary, and exercising it would need a real Google account whose quota endpoints return 403 plus a real agy login, which CLAUDE.md forbids without explicit request.codexbar usage --provider antigravitywith a real selected Google account that receives 403 from the quota endpoints and a logged-in agy shows CLI quotas instead of 'L…agylogin for the same account, and a Keychain-backed token account; CLAUDE.md forbids live…Evidence: Per-scenario output of the real OAuth strategy under the in-process harness (not live)
Evidence: swift test transcript for the harness run
Evidence: Compile failure of the branch's own test file (blocks macOS test bundle)
Tests/CodexBarTests/AntigravityIdentityFallbackTests.swift:226:8: error: inheritance from non-protocol type 'UsageFetcher' Tests/CodexBarTests/AntigravityIdentityFallbackTests.swift:231:8: error: type 'MockClaudeFetcher' does not conform to protocol 'ClaudeUsageFetching' Tests/CodexBarTests/AntigravityIdentityFallbackTests.swift:228:28: error: cannot find type 'CLICommand' in scope Tests/CodexBarTests/AntigravityIdentityFallbackTests.swift:130:48: error: type 'BrowserDetection' has no member 'standard' Tests/CodexBarTests/AntigravityIdentityFallbackTests.swift:7:16: error: static property 'handler' is not concurrency-safe because it is nonisolated global shared mutable state error: emit-module command failed with exit code 1Evidence: Transient harness source (not committed; kept for reproducibility)
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🚨 **Review** - high risk
✅ No issues found.
codexbar usage --provider antigravitywith a real selected Google account that receives 403 from the quota endpoints and a logged-in agy shows CLI quotas instead of 'Limits not available'codexbar usage --provider antigravitywith a real selected Google account that receives 403 from the quota endpoints and a logged-in agy shows CLI quotas instead of 'L…agylogin for the same account, and a Keychain-backed token account; CLAUDE.md forbids live…swift build --build-testson the unmodified branch (fails in AntigravityIdentityFallbackTests.swift; evidence codexbartests-build-failure.txt)NM_EVIDENCE_LOG=... CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test --filter AntigravityOAuthCLIQuotaFallbackProofTestswith the branch's broken test file temporarily set aside (8 in-process harness tests passed; harness source preserved in evidence dir, removed from worktree; not live product validation)Harness S1 (in-process, remote stubbed, fake agy): remote 403 on fetchAvailableModels and retrieveUserQuota + settings.json email == selected email + fake agy report → source=cli, quotas populated, identity.email=selected, agy invoked with--versionthen-p /usage --output-format json --print-timeout 90sHarness S2 (in-process): remote 403 + no CLI config email → source=oauth, primary=nil, agy never invokedHarness S3 (in-process): remote 403 + settings.json email of a different account → source=oauth, primary=nil, agy never invokedHarness S4 (in-process; failsVersion, oldVersion 1.0.0): matching identity but agy cannot report → source=oauth, primary=nil, only--versionattemptedHarness S5 (in-process; sourceMode=oauth with selected account; sourceMode=auto without selected account) → source=oauth, agy never invokedHarness S6 (in-process): fetchAvailableModels 200 with quotas → source=oauth with remote quotas, agy never invokedObservation OBS-A: selected credentials without email + foreign CLI email → fallback runs, identity.email=nil, loginMethod=cli (matches declined review item)Observation OBS-B: AntigravityCLIIdentityResolver against this machine's real HOME and against an empty HOME (Keychain only) → no email found; settings.json and jetski_state.pbtxt present, auth.json absentRead-only check that/opt/homebrew/bin/agyexists and that real ~/.gemini/antigravity-cli/settings.json and jetski_state.pbtxt contain no email pattern (counts only, no values read into evidence)git status --shortandgit diff --statafter cleanup: cleanSources/CodexBar/Providers/Antigravity/AntigravityProviderImplementation.swift:53- The Settings picker subtitle beside Usage source still says "Auto skips agy reports without account identity for selected or injected Google accounts." After this change, Auto with a selected token account does consult theagy -p /usagereport once the account-scoped OAuth fetch returns no model quotas and the localagylogin email matches the selected account. docs/antigravity.md explicitly points users to this subtitle for the explanation, so the UI copy now understates the behavior. It is user-facing Swift string content rather than a documentation file, so the documentation phase did not edit it. Suggested wording: "Auto uses agy reports for a selected or injected Google account only when the local agy login matches it; otherwise it skips identity-free reports." Keep the second sentence about Local API / agy CLI unchanged.🔧 Fix applied.
1 info still open:
Tests/CodexBarTests/ProviderSettingsDescriptorTests.swift:332- Judgment call for awareness: applying the user-selected Settings subtitle fix required updating the verbatim string expectation inantigravity usage source picker clarifies local ide and agy, because that test asserts the exact subtitle copy. Only the expected literal changed; no test logic or behavior was altered. No further action needed unless the outer executor prefers the copy change and test mirror to land as separate commits.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.