Preserve merged Warp bonus lane - #3166
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
@clawsweeper re-review This is the current-main follow-up to the sole compatibility finding from merged #3155. It includes resolver-state coverage, a merged controller-level pixel regression, a clean repository gate, and a full 928-selection test run. — Akshay / Codex |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
![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: a96c9d7664
ℹ️ 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".
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs real behavior proof before merge. Reviewed August 23, 2026, 11:40 PM ET / August 24, 2026, 03:40 UTC. ClawSweeper reviewWhat this changesThis PR removes the merged-style exception from Warp quota resolution so an unused bonus window keeps an empty secondary icon lane while an exhausted window retains Warp’s missing-secondary presentation. Regression provenancePossible regression — probable (reviewed change; known regression link). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 3 items remain The patch correctly addresses the merged-Warp semantic mismatch and resolves both prior findings, but the supplied image is generated by synthetic XCTest fixtures rather than a freshly built app run; external-PR proof policy still requires real after-fix behavior evidence before merge. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCodexBar converts provider usage snapshots into menu-bar icon meter values. Warp’s provider presentation policy feeds the merged status-item controller and renderer, whose signatures and cache keys decide whether the secondary lane is refreshed and drawn. flowchart LR
A[Warp usage snapshot] --> B[Provider percentage resolver]
B --> C[Warp presentation policy]
C --> D[Merged status-item controller]
D --> E[Renderer cache and lane layout]
E --> F[Menu-bar icon]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the provider-owned sentinel and cache regression coverage, then add a redacted after-fix app-bundle screenshot or recording of merged Warp with Show used enabled before merging. Do we have a high-confidence way to reproduce the issue? Yes—source provides a deterministic path: select Warp in merged mode with Show used enabled, then transition its secondary window from 100% used to 0% used; current main’s combined-style guard suppresses the required sentinel. Is this the best way to solve the issue? Yes—the provider-owned sentinel is the narrowest solution because layout policy is already passed independently to the renderer, and the 0.1 value survives both relevant cache normalizations while still rendering zero filled pixels. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1835ccd218b6. 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 (2 earlier review cycles) |
a96c9d7 to
991f76c
Compare
|
@clawsweeper re-review the corrected signed head |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
the
merge-risk: 🚨 security-boundary
991f76c to
ba59dab
Compare
|
@clawsweeper re-review final signed head |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
removed
merge-risk: 🚨 compatibility
2 participants
Summary
.combinedrenderingrenderingStyleargument from quota resolution now that provider layout policy is passed separatelyFixes #3165
Follow-up to #3155.
Root cause
The merged status-item path resolved Warp's provider windows but passed
.combinedas a second semantic switch. In show-used mode that bypassed Warp's sentinel conversion: a real unused bonus window stayed at zero and was later treated as absent, while an exhausted window stayed at 100 and was treated as present.Provider presentation now travels independently from visual icon style, so presence/exhaustion semantics must follow the selected provider in both merged and unmerged rendering.
Compatibility
0.1%subpixel sentinel: it keeps an empty lane, rounds tozero filled pixels, and remains distinct from exhausted zero in both controller and renderer cache keys.
treatsExhaustedSecondaryIconWindowAsMissing.Privacy and proof
The regression uses fixed synthetic percentages only. It constructs the production merged controller path,
transitions exhausted to unused, and verifies both the cache signature and the resulting bitmap/lane. It does
not launch the app or access live accounts, Keychain, browser state, workspaces, usernames, emails, paths, or
the desktop. Its controller receives
AccountInfo(email: nil, plan: nil)directly; no ambient account loader iscalled.
The checked-in proof is generated by the production resolver and renderer from fixed synthetic values. It was
visually inspected before upload and contains no PII:
Reproduce it with:
CODEXBAR_WARP_ICON_PROOF_DIR=.github/pr-proof swift test \ --filter IconRendererScreenshotRenderTests/test_renderSyntheticMergedWarpTransitionThe anonymized before/after proof for the original single-quota change remains in #3155; this PR is limited to its Warp compatibility follow-up.
Validation
make check— passed; SwiftFormat clean and 0 SwiftLint violationsmake test— 928 selections in 78/78 groups passed; 0 failures, retries, or timeoutsgit diff --check— passedSigned commit:
ba59dab171918738e54163fa5ec697eead5ec0f8— Akshay / Codex