Skip to content

fix(spend): widen ownership and revision coverage - #3104

Closed
Yuxin-Qiao wants to merge 1 commit into
steipete:mainfrom
Yuxin-Qiao:fix/spend-ownership-revision
Closed

fix(spend): widen ownership and revision coverage#3104
Yuxin-Qiao wants to merge 1 commit into
steipete:mainfrom
Yuxin-Qiao:fix/spend-ownership-revision

Conversation

@Yuxin-Qiao

Copy link
Copy Markdown
Contributor

Two coalesce/revision gaps kept the dashboard stale:

1. sameSourceOwnership:1536 too narrow
Only compared costUsageEnabled/providerIDs/codexAccountIdentities/sourceOwnershipFingerprints. Changing bucketTimeZoneIdentifier / openCodexUsageLogsEnabled / hideNativeCodexCostWhenOpenCodexPresent / hiddenSourceIDs / preferredCurrencyCode during an in-flight forceRefresh (#3041 path) was treated as same-owner, adopted request.configuration without a new load, leaving daily/hourly/chartDomain stale. Now compare the 5 display-affecting fields.

2. snapshotRevision:659 ignored hourly/projects/sessions
Only hashed daily[]. An OpenCodex per-request-hour update or project/session addition didn't bump sourceRevisions, so ForcedOutcome.incorporating:922 saw !hasNewerSourceRevision and discarded the confirmedNonempty input. Now hash hourly (hour+cost), projects (name/path/tokens/cost/dailyCount), sessions (id/tokens/cost).

Both fixes keep #3041 coalesce behavior for true same-owner churn while ensuring display changes and hourly-only OpenCodex updates invalidate.

Verified: swiftformat + swiftlint --strict clean. Gatekeeper anchors: no new provider clusters, only widened predicate + revision.

sameSourceOwnership only compared 4 fields, so switching
bucketTimeZoneIdentifier / openCodexUsageLogsEnabled /
hideNativeCodexCostWhenOpenCodexPresent / hiddenSourceIDs /
preferredCurrencyCode during an in-flight forceRefresh was
coalesced as same-owner and left daily/hourly/chartDomain stale.
Compare the display-affecting fields explicitly.

snapshotRevision only hashed daily[]. An OpenCodex hourly-only change or
project/session update did not bump sourceRevisions, so
ForcedOutcome.incorporating treated it as not newer and discarded the
confirmedNonempty input. Hash hourly / projects / sessions as well.
@clawsweeper

clawsweeper Bot commented Aug 20, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 20, 2026
@clawsweeper

clawsweeper Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 20, 2026, 6:17 AM ET / 10:17 UTC.

ClawSweeper review

What this changes

The PR expands spend-dashboard refresh ownership checks and snapshot revision inputs so configuration changes and hourly, project, and session updates trigger a fresh displayed result.

Merge readiness

Blocked until real behavior proof is added - 9 items remain

Keep open: the patch is targeted but does not actually publish hourly-only OpenCodex data and still omits rendered project/session fields from its revision hash.

Priority: P2
Reviewed head: de882960add408837d27750cf17587148c3fb687

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The patch has concrete refresh-state gaps and lacks after-fix real behavior proof.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR reports formatting and lint checks but supplies no after-fix refresh-path output, recording, or redacted runtime evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) 3 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR reports formatting and lint checks but supplies no after-fix refresh-path output, recording, or redacted runtime evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 4 items Hourly-only OpenCodex data is still discarded: The added hourly hash does not make an hourly-only OpenCodex snapshot visible: the OpenCodex merge gate publishes only snapshots with daily or session entries, and the configuration revision has only a settings revision for OpenCodex.
Project rows use unencoded daily contents: Project rows are calculated from every project daily entry, but the new revision input records only each project's daily-entry count and aggregate fields.
Session rows use unencoded activity and model data: The dashboard sorts sessions by last activity and derives their displayed model from model breakdowns, neither of which the new session hash records.
Findings 3 actionable findings [P2] Publish hourly-only OpenCodex snapshots
[P2] Hash project daily contents, not only their count
[P2] Include displayed session activity and model fields
Security None None.

Live Verification

Command: swift test --filter SpendDashboardControllerRevisionTests

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.22.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.22.0.tgz

Assertions:

  • FAIL expect_output: SpendDashboardControllerRevisionTests

How this fits together

CodexBar’s spend dashboard combines provider usage snapshots with display configuration during refresh. It coalesces compatible in-flight refreshes, then publishes charts, totals, project rows, and session rows.

flowchart LR
A[Provider usage snapshots] --> B[Spend refresh]
C[Dashboard display settings] --> D[Ownership comparison]
D --> B
B --> E[Snapshot revision]
E --> F[Coalesce or reload]
F --> G[Dashboard charts and totals]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR reports formatting and lint checks but supplies no after-fix refresh-path output, recording, or redacted runtime evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Publish hourly-only OpenCodex snapshots (P2) - These new hash inputs cannot fix the stated hourly-only OpenCodex case: OpenCodex snapshots are discarded unless daily or sessions is nonempty, and sourceRevisions tracks only its settings revision. Include hourly data in the publish condition and connect the relevant OpenCodex state to reconciliation, with a regression test.
  • Hash project daily contents, not only their count (P2) - projectRows derives visible totals from each project's daily entries, but this fingerprint records only daily.count. A correction that preserves count and stored aggregate fields can therefore leave the project panel stale; encode the daily fields used by the row calculation.
  • Include displayed session activity and model fields (P2) - The dashboard sorts sessions by lastActivity and derives the displayed model from modelBreakdowns, neither of which is fingerprinted here. An update with the same ID, token total, and cost can leave the sessions panel stale; encode those rendered fields too.
  • Resolve merge risk (P1) - Merging leaves the claimed hourly-only OpenCodex refresh case stale and can retain stale project totals, session order, or session model labels after reconciliation.
  • Complete next step (P2) - The author needs to resolve the review findings and provide real behavior proof; the OpenCodex reconciliation boundary requires an explicit implementation choice before automated repair.
  • Improve patch quality - Repair the OpenCodex publication/revision flow and complete rendered project/session hashing.
  • Improve patch quality - Add focused regression coverage for hourly-only OpenCodex, same-count project daily changes, and session activity/model changes.
  • Improve patch quality - Add redacted after-fix runtime evidence or a focused terminal test transcript, then update the PR body for re-review.

Findings

  • [P2] Publish hourly-only OpenCodex snapshots — Sources/CodexBar/SpendDashboardController.swift:689-694
  • [P2] Hash project daily contents, not only their count — Sources/CodexBar/SpendDashboardController.swift:697-702
  • [P2] Include displayed session activity and model fields — Sources/CodexBar/SpendDashboardController.swift:704-707
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production/test delta production +26, tests +0 The revised refresh fingerprint has no focused regression coverage despite repository guidance to extend XCTest coverage for new logic.

Merge-risk options

Maintainer options:

  1. Complete the refresh-state coverage (recommended)
    Publish hourly-only OpenCodex snapshots and encode all project/session fields that affect rendered rows, with focused regressions.
  2. Pause the partial fix
    Hold this PR if the OpenCodex revision boundary needs a broader refresh-flow design before it can be safely completed.

Technical review

Best possible solution:

Define an OpenCodex refresh revision that reaches reconciliation, publish hourly-only snapshots, and hash every project/session field used by rendered dashboard rows.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: an OpenCodex snapshot containing only hourly entries is rejected, while same-count project-daily or same-ID session activity/model changes leave the added hash unchanged.

Is this the best way to solve the issue?

No; widening the hash is the right direction, but the OpenCodex data never enters that revision path and the hash must cover all rendered fields.

Full review comments:

  • [P2] Publish hourly-only OpenCodex snapshots — Sources/CodexBar/SpendDashboardController.swift:689-694
    These new hash inputs cannot fix the stated hourly-only OpenCodex case: OpenCodex snapshots are discarded unless daily or sessions is nonempty, and sourceRevisions tracks only its settings revision. Include hourly data in the publish condition and connect the relevant OpenCodex state to reconciliation, with a regression test.
    Confidence: 0.99
  • [P2] Hash project daily contents, not only their count — Sources/CodexBar/SpendDashboardController.swift:697-702
    projectRows derives visible totals from each project's daily entries, but this fingerprint records only daily.count. A correction that preserves count and stored aggregate fields can therefore leave the project panel stale; encode the daily fields used by the row calculation.
    Confidence: 0.96
  • [P2] Include displayed session activity and model fields — Sources/CodexBar/SpendDashboardController.swift:704-707
    The dashboard sorts sessions by lastActivity and derives the displayed model from modelBreakdowns, neither of which is fingerprinted here. An update with the same ID, token total, and cost can leave the sessions panel stale; encode those rendered fields too.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a3457dafbe63.

Labels

Label changes:

  • add P2: The patch affects user-visible spend dashboard freshness with a bounded blast radius.
  • add merge-risk: 🚨 other: The added partial invalidation logic can preserve stale dashboard data after merge.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports formatting and lint checks but supplies no after-fix refresh-path output, recording, or redacted runtime evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: The patch affects user-visible spend dashboard freshness with a bounded blast radius.
  • merge-risk: 🚨 other: The added partial invalidation logic can preserve stale dashboard data after merge.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports formatting and lint checks but supplies no after-fix refresh-path output, recording, or redacted runtime evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Likely related people:

  • Yuxin-Qiao: Merged refresh-coalescing and adjacent OpenCodex spend work establish a direct history connection to this path. (role: recent area contributor; confidence: high; commits: 85873f52eaae, 07cd5a2a4e30; files: Sources/CodexBar/SpendDashboardController.swift)
  • Alec Gutman, Chip: The most recent current-main change to the controller shared spend sources between Overview and the dashboard. (role: recent area contributor; confidence: high; commits: 40297051639b; files: Sources/CodexBar/SpendDashboardController.swift)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@Yuxin-Qiao

Copy link
Copy Markdown
Contributor Author

Superseded by larger invalidation PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant