Skip to content

refactor: derive or justify every remaining provider special case - #2691

Merged
steipete merged 9 commits into
mainfrom
steipete/special-case-triage
Aug 6, 2026
Merged

refactor: derive or justify every remaining provider special case#2691
steipete merged 9 commits into
mainfrom
steipete/special-case-triage

Conversation

@steipete

@steipete steipete commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Summary

This completes the provider special-case triage with 6 derived / 38 justified / 0 remaining cross-provider case clusters. Six dispatch decisions now come from provider descriptors, capability records, semantic-window metadata, or provider implementations; the remaining 38 clusters carry an adjacent Provider-specific by design: explanation for the ownership or behavior that makes them intentionally provider-specific.

Decision-table highlights:

  • Settings presentation derives token-account organization/team controls and primary actions from TokenAccountSupport plus the provider implementation.
  • CLI cost support, token-cost surfaces, estimate wording, menu hint lines, and provider usage presentation come from descriptor-owned capabilities.
  • Weekly/session pace selection derives from semantic-window metadata instead of provider-name branches.
  • Irreducible ownership and state-machine cases remain local and documented, including managed Codex accounts, Claude/Codex combined usage, OpenAI web state, provider payload projections, and provider-specific icon behavior.

The new special-case gatekeeper scans cross-provider case clusters outside provider-owned implementation directories. Audited clusters require a justification marker within the cluster window, generic dispatch files must be explicitly allowlisted, and failures report the actionable file/line plus whether to derive, inventory, or justify the case.

The generated Codex parser hash was refreshed after the vendored scanner source changed; Scripts/regenerate-codex-parser-hash.sh --check reproduces 37aedd661c4272a8.

🤖 Generated with Claude Code

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 5, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a2a077286

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +208 to +210
let paceWindow = snapshot.flatMap {
ProviderDescriptorRegistry.descriptor(for: target).presentation.semanticWindows(snapshot: $0).weekly
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve Abacus's primary-lane pace window

For Abacus cards, this descriptor-based lookup now returns nil: AbacusUsageSnapshot.toUsageSnapshot() only populates primary, and that primary balance/billing window is monthly, while the default standardSemanticWindows only treats exactly 7-day windows as .weekly. The previous code explicitly used snapshot?.primary for Abacus, so Abacus users lose the pace/forecast in the menu card (and the same refactor affects the settings pane) whenever they have a normal Abacus usage snapshot; add an Abacus semantic-window resolver or keep the primary fallback.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 6, 2026
@clawsweeper

clawsweeper Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed August 5, 2026, 8:02 PM ET / August 6, 2026, 00:02 UTC.

ClawSweeper review

What this changes

This PR centralizes selected provider capabilities and presentation rules in descriptors, documents intentional provider-local cases, and adds a gatekeeper test for unexplained cross-provider branches.

Regression provenance

Possible regression — probable (reproduction; reviewed change). No predecessor PR is attributed.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep open: the descriptor refactor regresses Abacus’s existing primary-window pace display. Likely related people: steipete (recent provider-descriptor work, high confidence) and ratulsarna (original Abacus provider work, high confidence).

Priority: P2
Reviewed head: 9a2a077286c1cb5a629a6b4c8ac3fed9fcbb533c

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The refactor has useful structural coverage, but a concrete user-visible compatibility regression blocks merge.
Proof confidence 🌊 off-meta tidepool Not applicable: This owner-authored PR is outside the external-contributor proof gate; focused regression coverage remains necessary for the changed provider presentation.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This owner-authored PR is outside the external-contributor proof gate; focused regression coverage remains necessary for the changed provider presentation.
Evidence reviewed 4 items PR regression: The new semantic-window lookup returns only a weekly window, but the default resolver recognizes weekly only when its duration is exactly seven days.
Abacus source behavior: Abacus emits only a primary billing-cycle window, calculated as one calendar month, with no secondary window.
Current-main behavior: The PR base selected Abacus primary explicitly for pace, so this branch removes established behavior rather than completing an already-landed change.
Findings 1 actionable finding [P2] Preserve Abacus’s primary-lane pace window
Security None None.

How this fits together

CodexBar converts provider usage data and stored settings into menu, preference, widget, and CLI presentation. Shared provider descriptors bridge provider-specific data semantics to those common surfaces.

flowchart LR
A[Provider usage data] --> B[Provider descriptors]
B --> C[Shared presentation logic]
C --> D[Menu bar card]
C --> E[Provider settings]
C --> F[CLI output]
G[Architecture gatekeeper] --> B
Loading

Before merge

  • Preserve Abacus’s primary-lane pace window (P2) - AbacusUsageSnapshot produces only a month-long primary window, while the default semantic resolver accepts a weekly window only at exactly seven days. This makes paceWindow nil where current main explicitly selected primary, removing Abacus pace from both the menu card and settings pane. Add an Abacus semantic resolver or preserve the narrow primary fallback.
  • Resolve merge risk (P1) - Until repaired, existing Abacus users lose the established pace calculation on their primary monthly credit window in both menu-card and settings presentation.
  • Complete next step (P2) - A narrow descriptor-level repair and regression test can be prepared before merge without product-direction decisions.

Findings

  • [P2] Preserve Abacus’s primary-lane pace window — Sources/CodexBar/StatusItemController+MenuCardModel.swift:208-210
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Files affected 62 files affected; +575, -108 lines This broad provider-policy refactor changes shared descriptor metadata and multiple presentation consumers, making provider compatibility coverage important.

Merge-risk options

Maintainer options:

  1. Restore Abacus pace resolution (recommended)
    Add an Abacus semantic-window resolver or a narrow primary fallback so its monthly primary window continues to drive pace presentation.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Add focused coverage for an Abacus primary-only monthly snapshot and preserve its pace output.

Technical review

Best possible solution:

Encode Abacus’s established primary billing-cycle pace semantics in its descriptor, then cover that primary-only snapshot shape with a focused regression test.

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

Yes: an Abacus usage snapshot has only a month-long primary window, which the new generic semantic lookup does not classify as weekly while current main explicitly uses that primary window.

Is this the best way to solve the issue?

No: the refactor needs an explicit Abacus semantic-window mapping, analogous to Kimi’s descriptor mapping, before it can replace the prior primary-window selection.

Full review comments:

  • [P2] Preserve Abacus’s primary-lane pace window — Sources/CodexBar/StatusItemController+MenuCardModel.swift:208-210
    AbacusUsageSnapshot produces only a month-long primary window, while the default semantic resolver accepts a weekly window only at exactly seven days. This makes paceWindow nil where current main explicitly selected primary, removing Abacus pace from both the menu card and settings pane. Add an Abacus semantic resolver or preserve the narrow primary fallback.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: The branch removes an existing pace display for Abacus users, but the regression has a focused provider-specific repair.
  • add merge-risk: 🚨 compatibility: Shared descriptor routing can silently replace established provider-specific presentation behavior during upgrade.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This owner-authored PR is outside the external-contributor proof gate; focused regression coverage remains necessary for the changed provider presentation.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P2: The branch removes an existing pace display for Abacus users, but the regression has a focused provider-specific repair.
  • merge-risk: 🚨 compatibility: Shared descriptor routing can silently replace established provider-specific presentation behavior during upgrade.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This owner-authored PR is outside the external-contributor proof gate; focused regression coverage remains necessary for the changed provider presentation.

Evidence

Acceptance criteria:

  • [P1] swift test --filter AbacusDescriptorTests.
  • [P1] make test.
  • [P1] make check.

What I checked:

Likely related people:

  • steipete: Authored the current-main provider-presentation descriptor refactor and this PR’s related descriptor work. (role: recent area contributor; confidence: high; commits: fb66e63e8719, 8022b63ea453; files: Sources/CodexBar/StatusItemController+MenuCardModel.swift, Sources/CodexBarCore/Providers/ProviderUsagePresentation.swift)
  • ratulsarna: Introduced the Abacus provider and its documented monthly-primary pace semantics. (role: feature introducer; confidence: high; commits: c56ee9b13ff4; files: Sources/CodexBarCore/Providers/Abacus/AbacusUsageSnapshot.swift, Sources/CodexBarCore/Providers/Abacus/AbacusProviderDescriptor.swift)

Rank-up moves

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

  • Restore Abacus primary-window pace selection and add focused regression coverage.

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.

@steipete
steipete merged commit 61fbe9f into main Aug 6, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant