Skip to content

Trim menu formatting work#1572

Merged
steipete merged 4 commits into
steipete:mainfrom
ProspectOre:fix/trim-menu-formatting-work
Jun 18, 2026
Merged

Trim menu formatting work#1572
steipete merged 4 commits into
steipete:mainfrom
ProspectOre:fix/trim-menu-formatting-work

Conversation

@ProspectOre

@ProspectOre ProspectOre commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Avoid building a formatter for token counts below 1,000, where grouping never changes the output.
  • Replace repeated Bedrock billing-day date parsing with validated yyyy-MM-dd day-key comparison and direct month/day rendering.
  • Add focused coverage for small token counts and Bedrock latest valid billing-day selection.

Before / After

Temporary benchmark harness, not committed. Same checksums before/after.

Baseline: main at ac01d736
Branch: 5ab41dc1
Command: /usr/bin/time -l swift test --skip-build --filter FormatterHotPathBenchmarkTests

Probe Before After Change
tokenCountString small values, 2,000,000 calls 19.073202s 0.870158s 21.9x faster
Bedrock menu model, 5,000 builds over 30 daily entries 13.940430s 1.064601s 13.1x faster
Focused run wall time 38.14s 2.94s 13.0x faster
Max resident set size 299.7 MiB 108.9 MiB 63.7% lower
Peak memory footprint 27.4 MiB 27.2 MiB effectively flat

Checksums:

  • tokenCountStringSmallValues: 5780000 before and after
  • bedrockMenuModel: 175000 before and after

Verification

  • swift test --filter UsageFormatterTests passed: 34 tests
  • swift test --filter BedrockMenuCardTests passed: 2 tests
  • swift build --build-tests passed
  • swiftlint --strict passed: 0 violations in 1138 files
  • git diff --check passed

Notes

  • No new logging, persistence, network behavior, or secrets handling.
  • The fresh main benchmark worktree needed the local Sparkle framework symlink workaround under .build/out/Products/Debug/PackageFrameworks before running the focused test binary.

@ProspectOre
ProspectOre force-pushed the fix/trim-menu-formatting-work branch from 5ab41dc to f9b2906 Compare June 16, 2026 07:30
@clawsweeper

clawsweeper Bot commented Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 16, 2026, 3:55 AM ET / 07:55 UTC.

Summary
This PR optimizes token-count and Bedrock billing-day formatting paths and adds focused XCTest coverage for the changed behavior.

Reproducibility: not applicable. this is a cleanup/performance PR rather than a bug report. The PR body provides benchmark and checksum evidence instead of a failing user reproduction path.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 4 files, +132/-27. The diff is limited to two production formatter/menu-model files and two focused test files.
  • Reported validation: 5 verification commands plus benchmark output. The PR body reports focused tests, build-tests, lint, diff check, and benchmark checksums for the changed behavior.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Let the pending lint-build-test/full repository check finish before merge.

Risk before merge

  • [P1] The PR was still draft and the main lint-build-test check was still in progress at inspection; final merge should wait for completed CI or an explicit repository check result.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused optimization after final CI/check confirmation, preserving the current user-visible strings and Bedrock day-selection behavior covered by tests.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] No repair lane is needed because there are no actionable review findings; this needs normal draft completion, CI/check completion, and maintainer review.

Security
Cleared: The diff only changes local Swift formatter/menu-model code and tests; no security, secrets, network, dependency, CI, or supply-chain path is touched.

Review details

Best possible solution:

Land the focused optimization after final CI/check confirmation, preserving the current user-visible strings and Bedrock day-selection behavior covered by tests.

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

Not applicable; this is a cleanup/performance PR rather than a bug report. The PR body provides benchmark and checksum evidence instead of a failing user reproduction path.

Is this the best way to solve the issue?

Yes, the proposed approach is narrow: it removes hot-path formatter allocation while keeping the same visible token/date strings and adding focused coverage. The remaining merge condition is validation completion, not a different design.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P3: This is a low-risk internal performance cleanup with focused tests and no reported user-facing regression.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied terminal-style benchmark results with before/after timings and matching checksums, which is sufficient real behavior proof for this internal performance cleanup.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes copied terminal-style benchmark results with before/after timings and matching checksums, which is sufficient real behavior proof for this internal performance cleanup.

Label justifications:

  • P3: This is a low-risk internal performance cleanup with focused tests and no reported user-facing regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes copied terminal-style benchmark results with before/after timings and matching checksums, which is sufficient real behavior proof for this internal performance cleanup.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied terminal-style benchmark results with before/after timings and matching checksums, which is sufficient real behavior proof for this internal performance cleanup.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its guidance favors focused XCTest/model-seam validation for menu/parser behavior and asks for make check after code changes. (AGENTS.md:1, ac01d73625ee)
  • Focused formatter/menu diff: The PR changes the Bedrock latest-billing-day path from repeated DateFormatter parsing to validated day-key comparison, and changes sub-1000 token formatting to return the integer string directly. (Sources/CodexBar/MenuCardView+Costs.swift:133, f9b2906bd308)
  • Current main source behavior: Current main still constructs DateFormatter instances in the Bedrock date helpers and a NumberFormatter for token values below 1000, so the central optimization is not already implemented on main. (Sources/CodexBar/MenuCardView+Costs.swift:150, ac01d73625ee)
  • Focused tests added: The PR adds Bedrock menu-model coverage for selecting the latest valid billing day and UsageFormatter coverage for small token counts. (Tests/CodexBarTests/BedrockMenuCardTests.swift:50, f9b2906bd308)
  • Real behavior proof: The PR body reports a benchmark command with before/after timings and matching checksums, plus focused test/build/lint/diff-check commands for the changed formatter paths. (f9b2906bd308)
  • Validation state: GitHub reported the PR as mergeable but unstable, with the lint-build-test check still in progress while other checks had completed successfully at inspection time. (f9b2906bd308)

Likely related people:

  • steipete: git blame shows Peter Steinberger authored the current Bedrock billing-day helpers and tokenCountString implementation, and PR metadata shows recent adjacent MenuCardView+Costs work in merged PR 1546. (role: introduced behavior and adjacent owner; confidence: high; commits: 06fea2c897cc, bd243504ca6d; files: Sources/CodexBar/MenuCardView+Costs.swift, Sources/CodexBarCore/UsageFormatter.swift)
  • kiranmagic7: Merged PR 1498 recently changed Sources/CodexBar/MenuCardView+Costs.swift in the provider cost section adjacent to the Bedrock menu-model path. (role: recent area contributor; confidence: medium; commits: 6b0a0e504b39; files: Sources/CodexBar/MenuCardView+Costs.swift)
  • tukuyomil032: Merged PR 1568 recently changed UsageFormatter.swift and UsageFormatterTests.swift in another formatting path, making them a useful adjacent routing candidate. (role: recent adjacent contributor; confidence: medium; commits: ac01d73625ee; files: Sources/CodexBarCore/UsageFormatter.swift, Tests/CodexBarTests/UsageFormatterTests.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jun 16, 2026
@ProspectOre
ProspectOre marked this pull request as ready for review June 18, 2026 03:38
@steipete
steipete force-pushed the fix/trim-menu-formatting-work branch from f9b2906 to 24452ba Compare June 18, 2026 07:48
@steipete

steipete commented Jun 18, 2026

Copy link
Copy Markdown
Owner

Maintainer repair pushed at 24452bad3526338eeded022a44fbeef4b87cc42f.

Changes:

  • Preserve the contributor's menu-formatting hot-path optimization.
  • Reject impossible Bedrock billing dates before selecting the latest day.
  • Reject invalid daily cost rows before they can supply session cost or token values.
  • Align the menu fixture with the selected valid daily row.
  • Add changelog credit for @ProspectOre.

Proof:

  • swift test --filter 'UsageFormatterTests|BedrockMenuCardTests|CostUsageDecodingTests' - 53 tests passed.
  • make check - format, locale, and strict lint gates passed with zero violations.
  • Full branch Codex autoreview - no accepted/actionable findings (0.86 confidence).
  • Rebased onto 421bf576; diff check clean.

The invalid-date regressions exercise June 31 and prove it cannot outrank a valid billing day or populate usage totals. That malformed upstream shape cannot be produced reliably by a live AWS account, so deterministic exact-head parser/menu coverage is the meaningful end-to-end proof for this repair.

@steipete
steipete merged commit acd0302 into steipete:main Jun 18, 2026
4 checks passed
This was referenced Jun 18, 2026
Merged
Merged
@ProspectOre
ProspectOre deleted the fix/trim-menu-formatting-work branch June 18, 2026 08:30
@steipete steipete mentioned this pull request Jun 18, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ProspectOre @steipete