Skip to content

Keep retained models on the shared usage card - #3747

Closed
Chipagosfinest wants to merge 6 commits into
steipete:mainfrom
Chipagosfinest:fix/share-card-top-models-partial
Closed

Chipagosfinest wants to merge 6 commits into
steipete:mainfrom
Chipagosfinest:fix/share-card-top-models-partial

Conversation

@Chipagosfinest

@Chipagosfinest Chipagosfinest commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Settings Usage & Spend already lists retained top-model rows when another provider in the same currency is unpriced or incomplete. The 1200×630 share card did not: ShareStatsBuilder required the whole USD group to be complete, so one unpriced source (Antigravity in the live case) blanked Codex/Claude families and rendered No model-level history.

This keeps those retained rows on the shared image and copied text, skips incomplete-request lines, and labels the ranking PARTIAL so it cannot be mistaken for a closed ranking.

Before / after

Same three USD subscriptions. Antigravity is unpriced, so the group is incomplete. Synthetic totals.

Before (0.61.0 / main) After (this PR)
before after

Header goes BY USAGEPARTIAL. GPT / Claude / Gemini appear. Method: docs/research/share-card-top-models.md.

What was wrong

Currency groups are USD-wide, not per provider. modelHistoryCompleteness becomes .incomplete when any sibling in that group is unpriced. The builder then dropped every model:

model.groups.filter {
    $0.modelHistoryCompleteness == .complete && $0.incompleteRequestCount == 0
}

Settings uses the same group.models with a partial presentation. The OG card now follows that seam.

Tested against 0.61.0

Installed Homebrew codexbar 0.60.5 → 0.61.0 (current published release). This branch is rebased on upstream/main (0.61.1-dev, includes #3741).

Live 0.61.0 CLI:

  • OpenRouter Activity now returns last-30-day token/request/model counts. Deprecated -1 requests / 10s is gone (Stop surfacing OpenRouter's deprecated key rate limit as "-1 requests / 10s" #3720).
  • codexbar cost still skips OpenRouter/Grok/xAI — that command is local JSONL only (Antigravity, Claude, Codex, Cursor). Not this PR.
  • Codex and Claude still have complete priced model rows; Antigravity remains unpriced. That is the emptiness this PR fixes, and it still reproduces on 0.61.0 until this lands.

Related

Test plan

  • retained models still enter shared rankings when sibling coverage is incomplete
  • incomplete request rows stay out of shared rankings
  • Synthetic 1200×630 before/after renders (docs/research/assets/share-card-top-models/)
  • Live 0.61.0 codexbar usage --provider openrouter / codexbar cost / codexbar dashboard
  • macOS CI ShareStatsTests (local swift test blocked by actool / ibtoold plugin init)

@clawsweeper re-review

The OG card dropped every top-model family when any USD provider was unpriced or incomplete. Settings already listed those retained rows as partial. Share the same rows, skip incomplete-request lines, and label the ranking partial.
@clawsweeper

clawsweeper Bot commented Sep 18, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 18, 2026
@clawsweeper

clawsweeper Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 18, 2026, 5:22 PM ET / 21:22 UTC (Revision 3).

ClawSweeper review

What this changes

Retain eligible model families in shared usage images and copied text when currency-group coverage is incomplete, marking the list partial and removing numeric ranks.

Merge readiness

Blocked before merge - 4 items remain

The prior code findings are resolved, and the contribution remains distinct from current main. Keep open for approval of the changed sharing policy and after-fix production-path proof; repository policy also disables automatic closure.

Priority: P2
Reviewed head: 6c1fa156da24c0df0314bb8ef7d6b0066f5a6a16
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused patch resolves both prior findings, but the supplied visual evidence does not yet establish the changed production sharing behavior.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: The inspected images prove the copied view’s partial layout, but bypass ShareStatsBuilder and the production exporter. The new renderer test is useful supplemental coverage; an observed after-fix image and copied text from the actual sharing path remain absent. 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 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The inspected images prove the copied view’s partial layout, but bypass ShareStatsBuilder and the production exporter. The new renderer test is useful supplemental coverage; an observed after-fix image and copied text from the actual sharing path remain absent. 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 8 items Current main still excludes partial groups: The fetched main builder requires complete model history and zero incomplete requests; the proposed partial-list behavior is absent.
Latest release has the same exclusion: The v0.61.0 builder also excludes incomplete currency groups. Related gateway-family and OpenRouter changes do not implement this policy change.
Explicit existing product policy: GitHub confirms owner-authored #2308 merged with an explicit decision to retain partial rows in Settings while excluding them from shared rankings. This warrants maintainer approval before changing that distinction.
Findings None None.
Security None None.

How this fits together

CodexBar builds shared usage summaries from the Usage & Spend dashboard’s currency groups. The sharing builder sanitizes model families and feeds both the native image renderer and copied-text formatter.

flowchart LR
 A[Provider usage history] --> B[Dashboard currency groups]
 B --> C[Window and completeness checks]
 C --> D[Sanitized shared summary]
 D --> E[Native card image]
 D --> F[Copied statistics]
Loading

Decision needed

Question Recommendation
Should shared cards admit retained model families from incomplete currency groups when labeled PARTIAL and shown without numeric ranks? Approve qualified partial lists: Accept this bounded presentation change while retaining selected-day and incomplete-request exclusions.

Why: The existing exclusion was an explicit owner-authored product decision; matching Settings behavior does not establish approval to change shared output.

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The inspected images prove the copied view’s partial layout, but bypass ShareStatsBuilder and the production exporter. The new renderer test is useful supplemental coverage; an observed after-fix image and copied text from the actual sharing path remain absent. 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.
  • Resolve merge risk (P1) - Acceptance of partial shared model lists remains unresolved against the explicit existing sharing policy.
  • Complete next step (P2) - Obtain approval for partial sharing and add redacted after-fix image/copied-text evidence from the actual sharing path. Screenshots or recordings are preferred when useful; logs or live output also count. Update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +24/−7; tests +176/−2 Production growth supports the partial-list state and presentation, with focused regression and renderer coverage.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    With maintainer approval, share clearly qualified retained families while preserving full-window exclusions, provider attribution, and model sanitization.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

With maintainer approval, share clearly qualified retained families while preserving full-window exclusions, provider attribution, and model sanitization.

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

Not applicable as an established-behavior bug: source confirms the reported exclusion, but existing tests and the merged policy deliberately require it.

Is this the best way to solve the issue?

The implementation is a narrow, coherent approach if partial sharing is approved; the prior selected-day and architecture-anchor defects are resolved.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 5065a72b3832.

Labels

Label justifications:

  • P2: This is a bounded sharing improvement; dashboard model rows and provider totals remain available.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The inspected images prove the copied view’s partial layout, but bypass ShareStatsBuilder and the production exporter. The new renderer test is useful supplemental coverage; an observed after-fix image and copied text from the actual sharing path remain absent. 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.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The inspected images prove the copied view’s partial layout, but bypass ShareStatsBuilder and the production exporter. The new renderer test is useful supplemental coverage; an observed after-fix image and copied text from the actual sharing path remain absent.

Evidence

What I checked:

  • Current main still excludes partial groups: The fetched main builder requires complete model history and zero incomplete requests; the proposed partial-list behavior is absent. (Sources/CodexBar/ShareStatsPayload.swift:279, 5065a72b3832)
  • Latest release has the same exclusion: The v0.61.0 builder also excludes incomplete currency groups. Related gateway-family and OpenRouter changes do not implement this policy change. (Sources/CodexBar/ShareStatsPayload.swift:279, 60a677e6b22e)
  • Explicit existing product policy: GitHub confirms owner-authored Fix partial model history aggregation #2308 merged with an explicit decision to retain partial rows in Settings while excluding them from shared rankings. This warrants maintainer approval before changing that distinction. (bddbc9aff14e)
  • Previous findings addressed: The exact head’s parent-relative GitHub patch adds the selected-day exclusion and moves the architecture anchor to line 213. Both corrections match the inspected checkout; selected-day regression coverage is present. (Sources/CodexBar/ShareStatsPayload.swift:290, 6c1fa156da24)
  • Inspected visual evidence and its boundary: Both downloaded images match the checked-in PNGs byte-for-byte. The after-image visibly shows PARTIAL and dot markers for GPT, Claude, and Gemini. The documented method uses a copied view with synthetic payloads and ImageRenderer, bypassing the changed builder and production NSHostingView exporter. (docs/research/share-card-top-models.md:5, 6c1fa156da24)
  • Production-renderer coverage is supplemental: The added test calls the actual builder, formatter, and PNG renderer, but the captured discussion supplies no observed after-fix output from that path. The documented live CLI checks ran released v0.61.0 and exercise other behavior. Builds and tests were not executed during this read-only review. (Tests/CodexBarTests/ShareStatsTests.swift:423, 6c1fa156da24)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Chipagosfinest: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Provide an after-fix image and copied text from the actual sharing path, covering partial and complete snapshots; redact private usage, identities, credentials, and endpoints.
  • Record maintainer acceptance of qualified partial sharing.

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.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-18T20:34:56.766Z sha 03451ac :: needs real behavior proof before merge. :: [P2] Preserve the reporting window when admitting selected-day rows
  • reviewed 2026-09-18T20:45:31.776Z sha 9a2ce17 :: needs real behavior proof before merge. :: [P2] Keep selected-day model totals out of full-period exports | [P2] Update the architecture allowlist after shifting the sanitizer

Synthetic 1200x630 renders of the 0.61.0 empty ranking versus this branch's partial GPT/Claude/Gemini column. Live 0.61.0 notes recorded alongside.
@Chipagosfinest

Copy link
Copy Markdown
Contributor Author

Proof and 0.61.0 live check are now on the branch.

  • Before/after OG renders: docs/research/assets/share-card-top-models/ (synthetic payload, ImageRenderer of ShareStatsCardView at 1200×630). Method in docs/research/share-card-top-models.md.
  • Homebrew cask upgraded locally 0.60.5 → 0.61.0. OpenRouter Activity totals now show (tokens / requests / model count); the deprecated -1 requests / 10s rate limit is gone (#3720). codexbar cost still skips OpenRouter because that command is local JSONL only — not this ranking fix.
  • This PR is on current main (0.61.1-dev). It unblocks the models column independently of the spend-first layout in #3704 and the sparkline in #3722. Gateway families from #3713 still apply once OpenRouter Activity rows reach the builder.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 18, 2026

Copy link
Copy Markdown

🦞👀
Exact review queued.

Re-review progress:

A selected complete day inside an incomplete range still has zero incomplete requests on the retained rows. Restore the group-level incomplete-request guard so that day cannot export as the advertised period ranking, and omit numeric ranks when the list is partial.
@Chipagosfinest

Copy link
Copy Markdown
Contributor Author

Addressed the ClawSweeper P2 on selected-day reporting.

  • Restored the group-level incompleteRequestCount == 0 guard. Unpriced siblings (Antigravity) still share retained families; a complete day selected inside an incomplete window does not export as the advertised period ranking. Existing test: selecting a complete day does not share it as a complete window ranking.
  • Partial lists no longer show 01/02/03 ranks (dot marker instead), matching the earlier nonnumeric partial presentation.
  • After-fix render updated: docs/research/assets/share-card-top-models/after-partial.png.

Copied text for the synthetic payload:

Top models (partial):
GPT (Codex): 8.4M tokens · ~$42.50 est
Claude (Claude): 2.1M tokens · ~$18.25 est
Gemini (Antigravity): 900K tokens

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 18, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 18, 2026
Unpriced groups with a selected day still advertise the last-N-days window. Skip day-scoped model rows there, move the Mistral sanitizer architecture anchor with the new payload fields, and render partial cards through ShareStatsBuilder plus the production NSHostingView exporter.
@Chipagosfinest

Copy link
Copy Markdown
Contributor Author

Revision 2 findings addressed on 6c1fa156d:

  1. Selected-day vs advertised window. Unpriced groups with selectedDay set no longer export those day-scoped model rows under a last-N-days heading. Regression: selected day in an unpriced group does not export as the advertised window ranking.
  2. Architecture allowlist. Mistral family sanitizer anchor moved 208 → 213 with the new payload fields.
  3. Production-path proof in tests. builder payload renders a partial model card through the production exporter runs ShareStatsBuilder.makeShareStatsRenderer.pngData (NSHostingView) and asserts copied text. Set CODEXBAR_SHARE_STATS_SCREENSHOT_DIR to persist PNG + .txt.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 18, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@Chipagosfinest

Copy link
Copy Markdown
Contributor Author

Usage vs spend follow-up (not this ranking patch): #3748 projects live plugin costUsage (OpenRouter Activity) onto dashboard cost rows so usage totals and spend are the same catalog. This PR still owns the share-card completeness filter.

@steipete

Copy link
Copy Markdown
Owner

Superseded by #3817, which retains the PARTIAL presentation and contributor credit while gating incomplete history per provider. This also covers the incomplete-request case that still excluded the entire currency group. The replacement passes 69 focused tests and make check and includes synthetic before/after images. Thanks @Chipagosfinest!

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

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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.

2 participants