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 SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 18, 2026, 1:19 PM ET / 17:19 UTC (Revision 7). ClawSweeper reviewWhat this changesPreserves Antigravity quota history without an advertised reset duration and adds local API-equivalent cost estimates with explicit unpriced-usage disclosures. Merge readiness⛔ Blocked before merge - 2 items remain Keep open: the contribution remains useful, prior code findings are resolved, and no new actionable defect was found. Real native-app proof remains the merge blocker. Priority: P2 Review scores
Verification
How this fits togetherCodexBar records Antigravity quota snapshots for menu charts and reads local token-history files for cost displays. This change affects history persistence, chart presentation, and CLI cost output. flowchart LR
A[Quota snapshots] --> B[Cadence classification]
B --> C[Account history]
C --> D[Menu charts]
E[Local token records] --> F[Dated price estimates]
F --> G[Costs and unpriced coverage]
G --> D
G --> H[CLI output]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Retain account-scoped observations and transparent local estimates, with native menu evidence confirming the persisted data and disclosures reach users correctly. Do we have a high-confidence way to reproduce the issue? Yes, source inspection establishes the durationless-history gap and absent local pricing on main; no live current-main reproduction was executed. Is this the best way to solve the issue? Yes, the patch extends existing history and reader paths, preserves account storage, and reuses Claude pricing while explicitly retaining unpriced usage. The open alternative pricing PR is not an already-supported replacement. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 1d8cb13d3fba. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate review findings require fixes before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Antigravity quota-history observations and local API-rate token-cost estimates, with updated pace eligibility, UI/CLI disclosures, documentation, and tests.
Changes:
- Preserves durationless legacy quota observations without synthetic cadence.
- Adds dated Claude/Gemini estimates and unpriced-request tracking.
- Updates presentation, documentation, changelog, and test coverage.
File summaries
| File | Change / final review note |
|---|---|
Tests/CodexBarTests/ProviderPresentationPolicyCharacterizationTests.swift |
Updated pace-policy characterization tests. |
Tests/CodexBarTests/ProviderArchitectureGatekeeperTests.swift |
Updated source-location assertions. |
Tests/CodexBarTests/MenuCardAntigravityTests.swift |
Tests explicit session-duration behavior. |
Tests/CodexBarTests/AntigravityQuotaHistoryTests.swift |
Tests observation persistence and chart behavior. |
Tests/CodexBarTests/AntigravityLocalPricingTests.swift |
Tests pricing and unpriced coverage. Nit (1 vote): improve test-name wording. |
Tests/CodexBarTests/AntigravityCLICostTests.swift |
Tests CLI estimate output. |
Sources/CodexBarCore/Providers/Antigravity/AntigravityProviderDescriptor.swift |
Updates pace and estimate presentation. Moderate (1 vote): show coverage messaging even when no priced total exists. |
Sources/CodexBarCore/Providers/Antigravity/AntigravityLocalReader.swift |
Aggregates local costs and coverage metadata. |
Sources/CodexBarCore/Providers/Antigravity/AntigravityLocalPricing.swift |
Adds dated model pricing. Moderate (2 votes): canonicalize retired Flash aliases. Moderate (2 votes): add Claude-thinking pricing coverage tests. |
Sources/CodexBarCore/CostUsageFetcher.swift |
Publishes cost snapshots. Moderate (1 vote): add fixture coverage for estimated-cost and provenance mapping. |
Sources/CodexBarCLI/CLIHelp.swift |
Documents local cost estimates. |
Sources/CodexBarCLI/CLICostCommand.swift |
Displays estimates and coverage. Moderate (3 votes): report unpriced coverage when all requests are unpriced. |
Sources/CodexBar/UsageStore+PlanUtilization.swift |
Persists Antigravity history samples. |
Sources/CodexBar/PlanUtilizationHistoryStore.swift |
Supports cadence-free observation history. |
Sources/CodexBar/PlanUtilizationHistoryChartMenuView.swift |
Renders observation timestamps. Moderate (1 vote): define fallback behavior when observations and structured histories coexist without a snapshot. |
Sources/CodexBar/AntigravityQuotaHistory.swift |
Defines legacy pool observations. |
docs/antigravity.md |
Documents quota-history and pricing behavior. |
CHANGELOG.md |
Records the Antigravity changes. |
Review details
Suppressed comments (4)
Sources/CodexBar/PlanUtilizationHistoryChartMenuView.swift:198
- If
snapshotis nil while an account has both legacy observation histories and structured session/weekly histories, this fallback setsusesObservationsto true merely because any observation exists and hides every structured series. The chart can therefore show stale pool data or no data during an unavailable/initial-refresh state; without a snapshot, the code needs an explicit policy that does not arbitrarily discard one retained history set.
let usesObservations = snapshot.map { !UsageStore.hasAntigravityQuotaSummary($0) }
?? histories.contains { $0.name.isQuotaObservation }
guard history.name.isQuotaObservation == usesObservations else { continue }
Sources/CodexBarCore/CostUsageFetcher.swift:1261
- The new fetcher-to-snapshot mapping is not exercised by the added tests: pricing tests stop at
DailyReportResult, while the CLI tests constructCostUsageTokenSnapshotvalues manually and the existing fixture uses an unpriced model. A regression in the selected-day cost aggregation or.listPriceEstimateprovenance here could therefore pass all tests. Add a fixture-backed snapshot assertion covering estimated cost and coverage propagation.
return Self.tokenSnapshot(
from: daily,
now: now,
historyDays: historyDays,
useCurrentLocalDayForSession: true,
calendar: cal,
historyCoverageIsEstablished: reportResult.isComplete,
costProvenance: filtered.contains { $0.costUSD != nil } ? .listPriceEstimate : .unknown)
Sources/CodexBarCore/Providers/Antigravity/AntigravityProviderDescriptor.swift:58
- This new chart disclaimer is only rendered inside
CostHistoryChartMenuViewwhentotalCostUSDis non-nil. If all local requests are unpriced, the daily token rows still appear but the total is nil, so the menu omits both the estimate disclaimer and any explanation for the missing dollar values; make the coverage message render independently of a priced total (including the unpriced count) so the UI does not silently present incomplete history.
chartEstimateDisclaimer: .literal("API estimate · not billed · unpriced usage excluded")),
Tests/CodexBarTests/AntigravityLocalPricingTests.swift:7
- The test name has subject-verb agreement and wording issues: use “estimates each disjoint token bucket and honors the published cutoff.”
func `Gemini estimates price each disjoint token bucket and honor the published cutoff`(seconds: UInt64) throws {
- Files reviewed: 18/18 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| hasEstimates && unpriced > 0 | ||
| ? "Partial API-rate estimate: \(unpriced) requests have no supported price." : nil, |
There was a problem hiding this comment.
Fixed in 25d74f5: unpriced coverage is now shown even when no request has an estimate. The chart disclosure is also independent of the dollar total. Added fixture-backed partial/all-unpriced cases through the local reader, fetcher and CLI, including an out-of-range priced event that must not affect provenance. Formatting and strict lint pass; execution of the new tests is delegated to CI.
| if model.hasPrefix("claude-") { | ||
| let apiModel = model.hasSuffix("-thinking") ? String(model.dropLast("-thinking".count)) : model | ||
| return CostUsagePricing.claudeCostUSD( | ||
| model: apiModel, | ||
| inputTokens: event.input, | ||
| cacheReadInputTokens: usage.cacheRead, | ||
| cacheCreationInputTokens: event.cacheWrite, | ||
| outputTokens: output, | ||
| pricingDate: date, | ||
| modelsDevCatalog: ModelsDevCatalog(providers: [:])) |
There was a problem hiding this comment.
Added a claude-opus-4-6-thinking fixture in 25d74f5 asserting the numeric estimate, reasoning-as-output, raw model identity and estimated/unpriced counts. Also added fetcher-to-CLI coverage. The PR body now includes actual synthetic SQLite-to-CLI output from the existing 53d35b5 binary; the newly added tests await CI.
| guard ["gemini-3.6-flash", "gemini-3.7-flash", "gemini-3.8-flash"].contains(model), | ||
| event.cacheWrite == 0 |
There was a problem hiding this comment.
I checked canonicalModelID and am deliberately keeping these historical IDs unpriced. Its retiredFlashTiers table redirects retired picker generations to their successor (for example, 3.5 to 3.7); this is a routing compatibility rule, not evidence that an old request used the successor model or its historical price. Applying it to recorded usage would silently reprice old generations. In 25d74f5 I documented this boundary and added regression tests for both examples. A verified historical price or same-model alias can be added separately; the request count now remains visible even when all usage is unpriced.
|
@clawsweeper re-review Review fixes are in 25d74f5; current head 26b48da also merges upstream main at 6e9e73f and resolves the conflicts that were preventing ordinary CI from starting. The body now includes actual synthetic SQLite-to-CLI text/JSON evidence, populated version-1 upgrade coverage through both refresh formats and persistence/reload, fully unpriced CLI/chart disclosure, and freshest-format fallback when a snapshot is unavailable. The retired-picker alias suggestion is answered inline: successor routing does not establish historical prices. Formatting and strict lint pass again after the merge; new regression execution and current-head CI remain pending. No fresh app screenshot is claimed; chart validation remains at the persistence/model seam. Please assess the supplied evidence and remaining gap on that basis. |
|
🦞👀 Re-review progress:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
![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: 26b48da90c
ℹ️ 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".
| ] { | ||
| let chart = PlanUtilizationHistoryChartMenuView._modelSnapshotForTesting( | ||
| histories: histories, provider: .antigravity, snapshot: snapshot, referenceDate: now) | ||
| #expect(Set(chart.visibleSeries) == Set(names)) |
There was a problem hiding this comment.
When this loop reaches structured, standardPlanUtilizationSeries returns only .weekly because the snapshot contains a 10,080-minute window; visibleSeries then applies that set as allowedNames, filtering out session:300. The actual set is therefore only ["weekly:10080"], so this new expectation deterministically fails unless Antigravity's structured visibility policy is changed to admit the session series or the expected list is reduced.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Corrected in 7e20e47. The existing standardPlanUtilizationSeries policy intentionally exposes only weekly when a weekly window is available; the test now expects weekly:10080. Session and weekly preservation assertions remain unchanged. The macOS CI log confirms this was the only recorded issue in the upgrade test; the persistence/reload assertions recorded none. I kept the product policy rather than changing it to satisfy an incorrect expectation.
| inputTokens: event.input, | ||
| cacheReadInputTokens: usage.cacheRead, | ||
| cacheCreationInputTokens: event.cacheWrite, | ||
| outputTokens: output, |
There was a problem hiding this comment.
When an accepted Antigravity JSONL Claude event has a nonzero cacheWrite, the source format supplies no 5-minute-versus-1-hour cache duration, but this call passes every write as cacheCreationInputTokens and leaves cacheCreationInputTokens1h at its zero default. claudeCostUSD consequently prices every such write at the 5-minute rate even though 1-hour writes use a different rate, producing an incorrect API-equivalent estimate; preserve the duration if available or treat duration-less Claude cache writes as unpriced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7e20e47: cache-write events without a duration remain unpriced for both Claude and Gemini, while all token counts and unpriced coverage are retained. Added four JSONL fixture cases (both model families, with and without writes) and documented the duration boundary. Formatting and strict lint passed; current-head test execution is pending in CI.
|
@clawsweeper re-review Pushed 7e20e47. Non-nil offline/empty snapshots now use the persisted-format fallback unless they contain usable quota data; added ten cases covering offline, empty, placeholder, non-finite and unknown-summary responses against newer pool/structured histories. Also addressed the Codex cache-write review: duration-less writes retain tokens and remain unpriced, with four JSONL cases. The macOS CI log at 26b48da executed the populated upgrade scenario. Its only recorded issue was my incorrect session-plus-weekly chart expectation; that is now corrected to the existing weekly-only policy. Account preservation and save/reload assertions were retained. The PR body links this evidence and distinguishes the failed previous run from pending current-head validation. Formatting and strict lint pass. CLI tests now emit synthetic partial/all-unpriced output for CI evidence. No native-app screenshot is claimed; no local App build was performed. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
removed
the
merge-risk: 🚨 compatibility
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaabc0d1e7
ℹ️ 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".
| snapshot: snapshot, | ||
| capturedAt: now, | ||
| forSessionEquivalents: true) | ||
| + Self.antigravityQuotaObservationSamples(snapshot: snapshot, capturedAt: now) |
There was a problem hiding this comment.
When a cadence-less pool's usage decreases within the same hour without a meaningfully different resetsAt value (for example, a reset or replenishment when the source omits reset metadata), routing these samples through the generic history updater keeps the earlier hourly peak and its earlier capturedAt. The chart therefore continues to show the stale higher value, and antigravityHistoryUsesObservations can later misidentify the newest response format because it compares those retained timestamps. Quota-observation series should retain the latest capture/freshness independently rather than using peak-only compaction.
Useful? React with 👍 / 👎.
| if model.hasPrefix("claude-") { | ||
| let apiModel = model.hasSuffix("-thinking") ? String(model.dropLast("-thinking".count)) : model |
There was a problem hiding this comment.
When a local event uses Antigravity's dotted Claude spelling, such as claude-opus-4.6-thinking (the same model ID form already exercised in AntigravityCLIHTTPSFetchStrategyTests.swift), removing -thinking passes claude-opus-4.6 to claudeCostUSD. Its normalization does not convert the internal dot, while the built-in pricing key is claude-opus-4-6, so these otherwise supported Claude events are silently counted as unpriced. Normalize Antigravity's dotted version component before invoking the Claude pricing resolver.
Useful? React with 👍 / 👎.
| if snapshot.extraRateWindows?.contains(where: { | ||
| $0.id.hasPrefix("antigravity-quota-summary-") && $0.usageKnown | ||
| && !$0.window.isSyntheticPlaceholder && $0.window.usedPercent.isFinite | ||
| }) == true { return false } |
There was a problem hiding this comment.
When a quota-summary bucket has a valid remaining fraction but an unrecognized or omitted cadence, AntigravityStatusSnapshot can emit a usageKnown summary window whose windowMinutes is nil. This check nevertheless forces structured-history mode, while antigravityQuotaObservationSamples rejects every snapshot containing a summary row and the structured recorder cannot form its required 300/10,080-minute pair. Consequently the usable balance is not recorded and the chart hides newer observation history in favor of stale structured rows. Only select structured mode when the known summary exposes a supported cadence; otherwise preserve it as a cadence-less observation.
Useful? React with 👍 / 👎.
|
@clawsweeper re-review Summary of updates since the last review:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
the
proof: 📸 screenshot
Preserve the latest account-scoped pool observation per hour, including replenishment and delayed captures, without inventing reset windows or five-hour pace forecasts. Keep recognized structured summaries on their existing peak-history path. Retain version-1 persistence and account adoption while consolidating duplicate history decoding, ordering, and empty-chart construction. Add regression coverage and synthetic before/after chart proof. Refs #3619 Co-authored-by: hhh2210 <hzy2210@gmail.com> (cherry picked from commit da627ae)
Preserve the latest account-scoped pool observation per hour, including replenishment and delayed captures, without inventing reset windows or five-hour pace forecasts. Keep recognized structured summaries on their existing peak-history path. Retain version-1 persistence and account adoption while consolidating duplicate history decoding, ordering, and empty-chart construction. Add regression coverage and synthetic before/after chart proof. Refs #3619 Co-authored-by: hhh2210 <hzy2210@gmail.com> (cherry picked from commit da627ae)
|
Thanks @hhh2210. We adopted the quota-history portion in #3856 ( |
Antigravity can return usable pool balances without a reset duration. This preserves those account-scoped observations, avoids assuming a five-hour pace, and adds explicitly labeled API-equivalent estimates to the existing local token-history reader.
Observation history keeps the latest value and actual capture time per hour, including replenishment without reset metadata and account-history adoption. Structured windows retain their existing peak semantics. Valid summary balances with an unknown/omitted cadence use observations; unavailable responses choose the freshest stored format, with structured windows winning timestamp ties. Existing history and account boundaries remain intact.
Supported Claude and Gemini events receive dated API-rate estimates. Dotted Claude version spellings normalize for pricing while the recorded model ID stays unchanged. Separately recorded reasoning is priced as output where the source establishes disjoint buckets. Unknown models and cache writes without an established duration remain unpriced, retain their tokens, and expose unpriced request counts even when no dollar total exists. Retired picker aliases are not repriced as successor generations. These estimates are not Antigravity subscription charges or credit deductions.
Evidence and remaining proof gap
The September 18 review of
6b191ababconfirmed that all three prior findings are resolved and found no new actionable code defect. It accepted the images below as populated production-chart renders, but real native app setup proof remains outstanding. These synthetic offscreen captures do not establish the integrated menu workflow in a real app setup. That proof still requires a suitable test environment or maintainer acceptance of the current evidence.Production chart rendering
The opt-in
AntigravityHistoryNativeProofTestsrenders the actualPlanUtilizationHistoryChartMenuViewandCostHistoryChartMenuViewfrom synthetic fixtures. The quota fixture records throughUsageStore; the Cost fixture reads two unknown-model SQLite records throughAntigravityLocalReader. It does not replace the charts or disclosures with mock text.Render receipt · Reproduction command
The observation image shows populated bars and the latest 20% balance after 82%. The separate Cost image shows token usage and the explicit
2 unpriced requestsdisclosure without a fabricated dollar total. The receipt records the selected observation series, values, and disclaimer from production models.These are native SwiftUI
NSHostingViewbitmap captures, not a full menu-interaction end-to-end test. No app launch, visible windows, real accounts, credentials, provider requests, or production history files were used. The earlier empty-track screenshots are superseded by these populated renders.Review fixes and validation
toUsageSnapshot→UsageStore→ chart selection, preserving 75%/50% observations despite stale weekly history.claude-opus-4-6-thinking,claude-opus-4.6-thinking, andclaude-opus-4.6assert numeric estimates, raw model identity, reasoning accounting, and estimated/unpriced counts.At
6b191abab(September 18):make checkpassed: SwiftFormat, strict SwiftLint (2,355 files, zero violations), generated-file, locale, JavaScript/TypeScript, package, shell, and test-runner checks.swift test --jobs 2 --filter 'Antigravity|CostHistoryChartMenuView|MenuCardAntigravity|ProviderArchitectureGatekeeper|ProviderPresentationPolicyCharacterization|PlanUtilizationHistory|UsageStorePlanUtilization'passed: 757 tests in 50 suites plus 18 tests in three cross-platform suites, including the native render fixture and disclosure-cache regression.git diff --checkand repository artifact-size checks passed.make testcompleted: 1,191 selections in 110 groups. One group needed a retry:TTYCommandRunnerEnvTests.fast process exit drains buffered PTY output; the full-group retry recovered, with no timed-out groups. This is the September 18 result, not a test run of the new integration head.6b191abab, including both macOS test shards (CI run). ClawSweeper found no actionable code findings on that head; the real-app proof gap above remains.September 19 main integration
12ab05d91is an ordinary merge ofmain@1d8cb13d3(0.61.0 release notes). The only hand-resolved conflict wasCHANGELOG.md: this PR's two entries now remain under Unreleased, while the published 0.61.0 notes are retained. The previous history, pricing, and disclosure-cache fixes are unchanged.make checkpassed at this head (including strict SwiftLint across 2,420 files, zero violations), the PR diff against current main passesgit diff --check, and a static source check confirms all 397 pinned architecture-gatekeeper anchors still match. The new remote CI run is pending.No new local Swift tests, AppKit/status-bar tests, chart rendering, or app launch were performed for this merge. The tests and images above remain labeled with their original source head. No rebase or force-push.
Prior SQLite-to-CLI evidence remains available: all-unpriced text, all-unpriced JSON. These September 16 transcripts were captured at
eaabc0d1e, before the current repair; they are historical CLI evidence, not current-head validation.Pricing references: Gemini API pricing, Claude API pricing.