feat(spend): cache-first and 5m TTL for dashboard - #3107
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: 1f4bbb2cc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !force, | ||
| let lastAt = self.lastSpendDashboardTokenFetchAt[provider.instanceID], | ||
| let lastScope = self.lastSpendDashboardTokenFetchScope[provider.instanceID], | ||
| lastScope == costScopeSignature, | ||
| self.spendDashboardTokenSnapshotPublicationForCurrentConfig(for: provider) != nil, |
There was a problem hiding this comment.
This TTL cannot fire through production code: the sole caller in SpendDashboardSource.makeRequest always passes force: true, while .refreshMissing invokes that caller only when no current publication exists—even though this condition requires one. Consequently, the new five-minute guard cannot suppress any dashboard token scan; the caller needs to preserve the build mode's forced/non-forced semantics or perform the TTL decision before the missing-publication predicate.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed August 25, 2026, 2:08 PM ET / 18:08 UTC. ClawSweeper reviewWhat this changesThis PR adds cache-first Spend dashboard hydration, a five-minute refresh TTL, aggregate SQLite reads for Codex usage, and windowed OpenCodex cache loading. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 9 items remain Keep open: the cache-first path still has four concrete P2 correctness defects, and the supplied evidence is test-only rather than an after-fix dashboard run. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherThe Spend dashboard combines provider snapshots and local Codex/OpenCodex usage into a published cost view. This change sits between persisted usage caches and dashboard publication, affecting refresh frequency, historical-window loading, and displayed cost accuracy. flowchart LR
A[Dashboard pane opens] --> B[Request builder]
B --> C[Five-minute freshness check]
C --> D[Provider snapshots]
C --> E[Local usage caches]
D --> F[Cost reconciliation]
E --> F
F --> G[Published Spend dashboard]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve exact pricing-era information in aggregate storage, carry the requested report cutoff through every reload fallback, migrate compatible v1 cache data before read-only fallback, and start freshness at successful fetch completion. Do we have a high-confidence way to reproduce the issue? Yes—source-reproducible. The four failures follow deterministically from the TTL timestamp, reload fallback calls, read-only schema guard, and single-timestamp aggregate reconstruction paths. Is this the best way to solve the issue? No. The cache-first implementation needs to preserve its window, offline fallback, TTL, and historical-pricing invariants before it is a safe dashboard optimization. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0a1aa53598c9. LabelsLabel changes:
Label 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 (24 earlier review cycles; latest 8 shown)
|
|
CI failure is a real compile error:
|
4582171 to
d3596a6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3596a6a84
ℹ️ 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 shouldPrimeCachedCodex: Bool = self.cachedLoader != nil | ||
| && !Set(Self.codexOwnershipByID(configuration.codexAccountIdentities).keys) | ||
| .isSubset(of: Set(self.loadedInputs.map(\.id))) | ||
| && (phase == .ordinary || self.loadedInputs.isEmpty) |
There was a problem hiding this comment.
LoadPhase does not conform to Equatable, and its reconciling(ForcedOutcome) payload prevents synthesized conformance, so phase == .ordinary produces a “binary operator '==' cannot be applied” compiler error and blocks every build. Preserve the prior if case .ordinary = phase pattern or introduce an explicit case-checking property; the repository-required test run would have caught this before handoff.
AGENTS.md reference: AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac856d5c36
ℹ️ 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 identity = "\(logURL.path)|\(size)|\(mtime)" | ||
|
|
||
| if let cached = self.readCachedEntries(identity: identity), !cached.isEmpty { | ||
| if let cached = self.readCachedEntries(identity: identity, since: since) { |
There was a problem hiding this comment.
When usage.jsonl has changed since the previous read—as it normally does whenever a new request is appended—the identity check here misses and loadEntries reparses, deduplicates, sorts, and returns the entire lifetime log without applying since. Consequently, the new window only limits SQLite cache-hit reads, while active users continue hitting the unbounded latency and memory path this change is meant to avoid; apply the cutoff in the parser/miss path as well, while retaining the full cache if needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73bc2a1a34
ℹ️ 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".
| for baseline in providerBaselines where mode.shouldRefresh( | ||
| hasPublication: baseline.publication != nil, | ||
| isDashboardTokenStale: store.spendDashboardTokenFetchIsStale(for: baseline.provider)) |
There was a problem hiding this comment.
When the pane is reopened after five minutes without a configuration change, SpendDashboardPane.onAppear calls update(configuration:), but the equality guard in SpendDashboardController.update returns before building a request, so the stale predicate added here is never evaluated. Fresh evidence in the current tree is that the patch now correctly passes the non-forced mode to the loader, while the unchanged controller guard still prevents that path from running on an ordinary reopen. The cached snapshot can therefore remain beyond the advertised TTL until a configuration/date change or manual refresh triggers another load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Apply the cutoff before parsing cache misses
When usage.jsonl changes, this cache-miss path still parses the entire lifetime file, materializes every entry in unique, sorts the full collection, and rewrites all entries before the since filter runs. Thus active logs remain subject to unbounded peak memory and refresh latency even though the returned array is windowed. Fresh evidence in the current tree is that the follow-up only filters deduped after these full-history operations; pass the cutoff into parsing or otherwise avoid materializing old entries on the report path.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b577455ac9
ℹ️ 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 rowsByPath = mode == .scanReady | ||
| ? Dictionary(grouping: snapshot.usageRows, by: \.path) | ||
| : [:] |
There was a problem hiding this comment.
When cached Codex usage contains reasoning tokens or usage preceding a historical pricing cutoff, .aggregateReport discards the persisted rows and reconstructs them from day/model aggregates. Those synthesized rows omit both reasoning and timestampUnixMs, while buildCodexReportFromCache derives displayed reasoning totals and date-sensitive pricing from those fields. Consequently, the cache-first snapshot can report zero reasoning and price historical usage at current rates; retain the required metadata in the aggregate representation or decode the rows needed for an equivalent report.
Useful? React with 👍 / 👎.
| guard self.tokenSnapshotPublicationForCurrentProviderConfig(for: provider) != nil, | ||
| let legacyLast = self.lastTokenFetchAt[provider.instanceID] | ||
| else { return true } | ||
| return Date().timeIntervalSince(legacyLast) >= 5 * 60 |
There was a problem hiding this comment.
When the regular token pipeline has just published its default 30-day snapshot before the first dashboard open, this branch treats it as fresh solely from its timestamp even though the dashboard requests scanDays == 365. capturedTokenPublication then falls back to that legacy snapshot and skips the independent dashboard fetch, so the dashboard's 365-day/All view is populated with only 30 days of history until another refresh is triggered. Reuse legacy freshness only when its history coverage and scope satisfy the dashboard request.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
b577455 to
d0c3479
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0c347973b
ℹ️ 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.metadata.timeZoneIdentifier == calendar.timeZone.identifier | ||
| else { return CostUsageCache() } | ||
| return Self.cache(from: snapshot) | ||
| return Self.cache(from: snapshot, mode: mode) |
There was a problem hiding this comment.
When the Codex cache approaches its 25,000-row or 256 MB budget, .aggregateReport still calls readSnapshot(), whose implementation reads and copies every usage_rows.payload blob and every token snapshot before cache(from:mode:) discards those arrays. As a result, the new dashboard path avoids JSON decoding but still incurs full-cache SQLite I/O and memory allocation on first paint; use a mode-specific snapshot reader that does not query these tables.
Useful? React with 👍 / 👎.
d0c3479 to
d8c5ae9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8c5ae9269
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let cached = self.readCachedEntries(identity: identity, since: since) { | ||
| return cached |
There was a problem hiding this comment.
When the matching SQLite cache cannot be read completely—for example, sqlite3_step returns SQLITE_BUSY/SQLITE_ERROR, or every cached payload fails decoding—readCachedEntries currently returns an empty array, and this newly unconditional cache-hit branch accepts it instead of reparsing the valid JSONL source. The OpenCodex source is then published as confirmed empty and remains so until the log identity changes; distinguish a successfully completed zero-row query from read/decoding failures and return nil for the latter.
Useful? React with 👍 / 👎.
77b21f3 to
9cb04e2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cb04e241b
ℹ️ 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".
| WHERE timestamp >= ? | ||
| ORDER BY timestamp, request_id |
There was a problem hiding this comment.
When an existing OpenCodex cache contains a large lifetime history, this query still performs a full scan of entries and sorts the matches because the schema only indexes the request_id primary key, not timestamp. Thus every dashboard cache hit remains proportional to the entire cache rather than the 365-day window this change is intended to load; add a (timestamp, request_id) index and ensure existing schema-version-1 databases receive it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 328a4360d6
ℹ️ 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 snapshot = self.readSnapshot(skipRowTables: mode == .aggregateReport) | ||
| guard snapshot.metadata.timeZoneIdentifier == nil | ||
| || snapshot.metadata.timeZoneIdentifier == calendar.timeZone.identifier | ||
| else { return CostUsageCache() } | ||
| return Self.cache(from: snapshot) | ||
| return Self.cache(from: snapshot, mode: mode) |
There was a problem hiding this comment.
When cache-first hydration must rebuild a report from aggregates rather than use an exact previous report, the synthesized token-bearing rows have no eventIndex, so codexRowCostBreakdown marks them unstable and rowCostIsTrusted is always false. Standard usage then falls back to aggregate pricing that excludes the newly separated reasoning tokens, while priority evidence disables that fallback entirely and produces a nil cost. Fresh evidence is that the current patch now synthesizes reasoning/timestamp metadata, but this aggregate-mode path still discards its row-level pricing, so cached spend remains lower or unavailable until the live scan finishes.
Useful? React with 👍 / 👎.
| Self.bindAggregateValues(aggregate, to: insert, startingAt: 4) | ||
| Self.bind(aggregate.earliestTimestampUnixMs, to: insert, at: 20) |
There was a problem hiding this comment.
For every persisted file aggregate, this statement has only 18 parameters: the 14 values bound starting at index 4 end at 17, making earliest_timestamp_ms parameter 18 rather than 20. SQLite therefore returns SQLITE_RANGE for this ignored bind and inserts NULL for the timestamp; the global aggregate insert repeats the same off-by-two error at index 19 instead of 17. Fresh evidence is that the patch added timestamp persistence to preserve date-sensitive pricing, but cached historical rows still lose that metadata and resolve without their original pricing date.
Useful? React with 👍 / 👎.
328a436 to
e4466e0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63e8df340e
ℹ️ 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".
| $0 + ($1.inputTokens ?? 0) + ($1.cacheReadTokens ?? 0) | ||
| + max(0, ($1.outputTokens ?? 0) - ($1.reasoningTokens ?? 0)) |
There was a problem hiding this comment.
For this fixture, stored.data already exposes output exclusive of reasoning because the changed scanner stores deltaOutput - deltaReasoningTokens; expectedUnits was likewise updated to count exclusive output. Subtracting reasoningTokens here a second time therefore undercounts every reasoning-bearing entry and makes the final storedUnits == expectedUnits assertion fail. The repository-required full test run would expose this regression.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| if self.spendDashboardTokenSnapshotPublicationForCurrentConfig(for: provider) != nil { | ||
| return false |
There was a problem hiding this comment.
After the first successful or confirmed-empty dashboard scan, this branch treats the current publication as fresh whenever lastSpendDashboardTokenFetchAt is absent, but this change also removes the only production assignment to that dictionary and no success path replaces it. With an unchanged provider scope, the five-minute comparison is consequently never reached and ordinary pane reopens reuse the publication indefinitely; record the completion time when publishing a successful or empty result while leaving failures timestamp-free.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4381ec51ae
ℹ️ 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 rowsByPath = mode == .scanReady | ||
| ? Dictionary(grouping: snapshot.usageRows, by: \.path) | ||
| : [:] |
There was a problem hiding this comment.
When a cached day/model contains multiple standard requests, this aggregate-only path discards their individual rows and later synthesizes one row containing the entire daily input. Long-context pricing is request-scoped, so two 150k-token requests below a 200k threshold become one 300k-token row; because that synthetic row has no eventIndex, makeCodexBilledDayEntry rejects its row cost and codexAggregateCostUSD returns nil once the aggregate exceeds the threshold. The cache-first dashboard therefore loses an otherwise computable cost until the live scan finishes; retain per-request threshold evidence or a trustworthy persisted computed cost.
Useful? React with 👍 / 👎.
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fdf6d7d05
ℹ️ 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".
| // back at the output rate (see `codexResolvedCostUSD`), keeping USD unchanged while | ||
| // making token buckets additive. | ||
| let deltaReasoningTokens = deltaReasoning ?? 0 | ||
| let outputExcludingReasoning = max(0, deltaOutput - deltaReasoningTokens) |
There was a problem hiding this comment.
When reasoning exceeds the non-reasoning output remainder, this representation change truncates reasoning in project/model analytics. For example, an event with 100 output and 60 reasoning now stores (output: 40, reasoning: 60); CodexLocalProjectUsageIndexer forwards those values to CodexModelsUsageFragment, whose initializer in CodexModelsAnalyticsModels.swift clamps reasoning to outputTokens, reducing it to 40. Update that downstream invariant for the new exclusive-output representation so analytics retain the actual reasoning count.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
the
rating: 🦪 silver shellfish
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff7fc29012
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let cached = self.readCachedState(), Self.canReuseCursor(cached.parseCursor, identity: identity) { | ||
| if identity.size == cached.parseCursor.parsedOffset { | ||
| return cached.entries | ||
| return Self.filteredEntries(cached.entries, since: since) |
There was a problem hiding this comment.
When the cache contains a long OpenCodex history, readCachedState() has already queried and decoded every row before this in-memory since filter runs; readEntries still uses an unbounded SELECT ... FROM entries ORDER BY timestamp, request_id. Consequently, cache-hit I/O and memory remain proportional to the lifetime log rather than the requested 365-day window. Fresh evidence in the current tree is that the timestamp index was added, but the read query never binds a timestamp predicate, so pass since into the cache reader and filter in SQL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f545a6070
ℹ️ 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".
| cached: aggregate.priorityCachedTokens, | ||
| output: aggregate.priorityOutputTokens, | ||
| mode: "priority", | ||
| reasoning: hasStandardTokens ? 0 : syntheticReasoning) |
There was a problem hiding this comment.
When a day/model aggregate contains both standard and priority usage and its reasoning total exceeds the standard output, this assigns zero reasoning to the priority row while append clamps the entire total to the standard output. For example, 10 standard output tokens plus 100 priority output tokens and 80 reasoning tokens hydrate as only 10 reasoning tokens, so the cache-first dashboard underreports reasoning until the live scan finishes; allocate the remainder to the priority row while preserving the aggregate total.
Useful? React with 👍 / 👎.
|
Final-head proof for PR #3107:
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
|
Fix for the current-head P2 review finding:
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3399e04e1b
ℹ️ 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".
| standardTokens: 0, | ||
| priorityTokens: 0) | ||
| priorityTokens: 0, | ||
| earliestTimestampUnixMs: rows.compactMap(\.timestampUnixMs).min()) |
There was a problem hiding this comment.
When one file/day/model contains usage on both sides of a date-sensitive pricing cutoff—for example, a non-UTC dashboard day that straddles the UTC cutoff—persisting only the earliest timestamp causes every synthetic aggregate row to be priced using the earlier rate. The scan-ready report prices each original row with its own timestamp, so cache-first spend diverges until the live scan finishes; preserve separate aggregates per pricing era or a trustworthy computed cost instead of one minimum timestamp.
Useful? React with 👍 / 👎.
| if version == 1, Self.databaseHasEntriesTable(db) { | ||
| if self.ensureTimestampIndex(db) { | ||
| Self.setUserVersion(db, Self.schemaVersion) | ||
| return |
There was a problem hiding this comment.
When a v1 payload has been copied into opencodex-usage-v2.sqlite and usage.jsonl is temporarily unreadable, this migration never runs: readCachedState() opens read-only and rejects any version other than 2, while ensureSchema() is invoked only after a writable open reached through the full-reload path. The valid cached entries therefore cannot provide the promised non-destructive fallback; perform the v1 migration before rejecting the cached read.
Useful? React with 👍 / 👎.
|
Current-head repair and proof for PR #3107:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49147463fd
ℹ️ 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".
| // A current successful fetch owns freshness from here, including the | ||
| // confirmed-empty path below; without this stamp the published snapshot would | ||
| // bypass the five-minute TTL forever. | ||
| self.lastSpendDashboardTokenFetchAt[provider.instanceID] = now |
There was a problem hiding this comment.
When a dashboard token fetch takes a substantial portion of the five-minute interval, this stores now, which was captured before loadTokenUsageSnapshot was awaited, so a successful result can already be stale—or immediately expire—when it is published and reopening the pane triggers another expensive fetch. Fresh evidence beyond the earlier missing-timestamp finding is that the timestamp is now assigned, but it still records the attempt start rather than the successful completion time.
Useful? React with 👍 / 👎.
| logURL: URL, | ||
| identity: LogIdentity, | ||
| fileManager: FileManager, | ||
| since: Date? = nil, |
There was a problem hiding this comment.
When an incremental read races an append and parseLog throws ChangedUnderReadError, incrementalReload invokes this newly parameterized fullReload without forwarding since; a second file-identity race also returns parsed.entries directly. Those paths therefore return the entire lifetime log to the dashboard instead of the requested window, restoring the unbounded sorting and aggregation cost under active writes. Fresh evidence beyond the earlier cache-miss cutoff finding is that ordinary reload exits now filter correctly, while these race exits still take the new default nil cutoff.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Current-head repair and proof for PR #3107:
@clawsweeper please re-review this exact head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1559fb8cf1
ℹ️ 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".
| !self.isRefreshing, | ||
| !self.phase.manualRefreshOutstanding | ||
| else { return } | ||
| self.startLoad(configuration: configuration, phase: .ordinary) |
There was a problem hiding this comment.
On every pane reopen after the initial load, this unconditionally starts an ordinary load even when all dashboard data is still inside the five-minute TTL. The request builder suppresses only the independent provider refreshes; it still includes every Codex request, and handleBuiltRequest still invokes SpendDashboardSource.load, which scans each Codex account and reloads OpenCodex. Consequently, repeatedly closing and reopening the pane within five minutes still shows a refresh and performs the expensive work the TTL is intended to avoid; check staleness before calling startLoad, or let the request signal that no loader pass is needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bee53b4c6e
ℹ️ 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 key = CodexPricingSegmentKey( | ||
| day: row.day, | ||
| model: row.model, | ||
| timestampUnixMs: eraTimestamp, | ||
| pricingMode: row.pricingMode, | ||
| hasKnownCost: row.knownCostNanos != nil) |
There was a problem hiding this comment.
When a priority turn's displayed model is an alias but its completed response supplies the billable model, codexRowsWithPricingMetadata preserves that resolved value in row.pricingModel; this segment key and accumulator retain only row.model, and hydration later also sets pricingModel to that display model. For cases such as codex-auto-review resolved to gpt-5.4, the cache-first report therefore cannot reproduce the live priority price and may show no cost until the full scan completes. Include the resolved pricing model in the persisted segment and its grouping key.
Useful? React with 👍 / 👎.
|
Current-head repair and proof for PR #3107:
@clawsweeper please re-review this exact head. |
|
@clawsweeper re-review Please review the current pushed head |
|
🦞👀 Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 632219a64d
ℹ️ 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".
| else { return } | ||
| if self.lastSuccessfulConfiguration != nil, | ||
| self.failedSourceCount == 0, | ||
| self.nowProvider().timeIntervalSince(self.loadedAt) < Self.dashboardSnapshotTTL |
There was a problem hiding this comment.
When the pane receives a same-day didBecomeActive notification, refreshDateWindow() advances loadedAt without fetching any data (lines 1571/1576). Comparing the TTL against that same field means app activations can make an old snapshot appear fresh; repeated activations within five minutes can postpone dashboard refresh indefinitely. Track the last successful load completion separately and use that timestamp for this TTL check.
Useful? React with 👍 / 👎.
|
Current-head update for PR #3107:
@clawsweeper please re-review this exact head. |
|
@clawsweeper re-review Current head is |
|
🦞👀 Re-review progress:
|
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: feat(spend): cache-first and 5m TTL for dashboard This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
2 participants
Summary
Rebased onto current main (2db6a57, includes #3120 parity scanner) and reconciled the cache-first spend dashboard + 5m TTL feature on top of it.
Reconciliation with landed #3120 semantics
Correctness repairs from review
SpendDashboardSource.loador rescan Codex accounts. Snapshot freshness has its own completion clock, separate from the reporting-windowloadedAtclock, so same-day activations cannot postpone expiry.earliest_timestamp_msat the actual placeholder indices, preserving pricing dates.scannerUnitsdefinition.entriesquery (timestamp >= ?) so long-lived caches do not decode lifetime history before filtering.codex-auto-review->gpt-5.4) in both persistence and grouping keys.Current-head proof
Current pushed head:
e468f364cdc8962cd4e74f385fcac41bc6c699a9The deterministic proof uses isolated temporary stores and no provider credentials:
Current-head CI: https://github.com/steipete/CodexBar/actions/runs/32889384161
The current-head exact review has been requested; no live provider credentials are used.