Skip to content

Use descriptor pace rules in CLI - #2489

Open
kiranmagic7 wants to merge 1 commit into
steipete:mainfrom
kiranmagic7:kiran/descriptor-driven-cli-pace-20260729
Open

Use descriptor pace rules in CLI#2489
kiranmagic7 wants to merge 1 commit into
steipete:mainfrom
kiranmagic7:kiran/descriptor-driven-cli-pace-20260729

Conversation

@kiranmagic7

@kiranmagic7 kiranmagic7 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Addresses the CLI follow-up discussed in #2431.

Summary

  • use provider descriptor reset-window pace rules when producing CLI text and JSON
  • share calendar-month duration resolution between menu and CLI rendering
  • include eligible tertiary windows in CLI text and the additive pace.tertiary JSON field
  • add Grok weekly, Amp calendar-month, and Alibaba/OpenCode Go tertiary regression coverage

Root cause

CLIRenderer.PaceKind.supports still used provider allowlists from before the descriptor capability move in #2364. Descriptor-supported windows such as Grok weekly and Amp monthly were therefore omitted from CLI pace output even though the menu could pace them. The same computation also stopped at primary and secondary slots, so monthly quotas stored in snapshot.tertiary remained absent from CLI text and JSON.

Verification

  • final head: f2ada4a0, based on e92a6fa4
  • regression proof on the original base: the initial Grok and Amp CLI tests failed because both provider pace payloads were nil
  • swift test --filter 'CLISnapshotTests|ProviderPaceCapabilityTests|ZoomMateCreditsHistoryFetcherTests' — 70 tests passed on the final head, including Alibaba and OpenCode Go tertiary text/JSON coverage
  • make check — passed; SwiftFormat 0/1642 files, SwiftLint 0 violations in 1641 files
  • git diff --check upstream/main...HEAD — passed
  • make test — stopped after its automatic retry on two unchanged AdaptiveRefreshTimerTests CancellationError timeouts; both failures reproduced on the untouched original base, and this PR does not modify that test or timer code

Compatibility

Risk is low. Existing primary/secondary Codex, Claude, Ollama, OpenCode, and Kimi pace behavior is preserved. Tertiary pace is additive and only appears when the existing standard rules or the provider descriptor can compute it. The JSON change is additive: pace.tertiary is omitted when no tertiary pace applies.

Review focus: descriptor-to-CLI window/kind resolution, tertiary text/JSON parity, and the shared calendar-month duration calculation.

@kiranmagic7
kiranmagic7 marked this pull request as ready for review July 29, 2026 05:54

@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: f725708fb2

ℹ️ 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".

Comment thread Sources/CodexBarCLI/CLIRenderer.swift
@kiranmagic7
kiranmagic7 force-pushed the kiran/descriptor-driven-cli-pace-20260729 branch from cf393b0 to f2ada4a Compare July 29, 2026 07:44
@clawsweeper clawsweeper Bot added 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. P2 Normal priority bug or improvement with limited blast radius. labels Jul 29, 2026
@clawsweeper

clawsweeper Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 29, 2026, 8:47 AM ET / 12:47 UTC.

ClawSweeper review

What this changes

The PR makes CLI pace calculations use provider descriptor rules, shares calendar-month reset-window resolution with the menu, and adds tertiary pace output to CLI text and JSON.

Merge readiness

Blocked until real behavior proof from a real setup is added - 3 items remain

Keep this PR open. The focused patch appears to complete the CLI follow-up to the descriptor-based pace refactor and addresses the earlier tertiary-window review concern, but the external PR has only test and CI claims rather than captured after-fix CLI behavior proof; obtain redacted live text/JSON output before merge.

Priority: P2
Reviewed head: f2ada4a04abed86312bdaa4219507741a6cef01f

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The implementation is focused and well-covered by stated regression tests, but merge readiness is capped because no after-fix real CLI behavior evidence is attached.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The PR reports tests and CI, but it needs a redacted terminal capture or copied live CLI text/JSON output showing the corrected pace behavior; updating the PR body with that proof should trigger a fresh review, or a maintainer can request @clawsweeper re-review. 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 real behavior proof before merge: The PR reports tests and CI, but it needs a redacted terminal capture or copied live CLI text/JSON output showing the corrected pace behavior; updating the PR body with that proof should trigger a fresh review, or a maintainer can request @clawsweeper re-review. 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 5 items Current CLI gap and proposed repair: The branch changes CLI pace construction to use descriptor-aware reset-window rules and adds tertiary pace to the payload, covering the prior primary/secondary-only path.
Shared monthly-duration behavior: The branch moves calendar-month duration resolution into ProviderPaceCapability so the menu and CLI consume the same provider-defined reset-window calculation.
Prior review concern addressed: The earlier review identified missing tertiary descriptor pace in CLI text and JSON; the follow-up discussion states it was repaired and regression coverage was added for Alibaba and OpenCode Go tertiary output.
Findings None None.
Security None None.

How this fits together

CodexBar collects quota windows from provider probes and presents their pace in both the macOS menu and the CLI. Provider descriptors define which reset windows have enough known cadence to calculate pace, and the renderers turn that into human-readable and JSON output.

flowchart LR
  A[Provider quota snapshot] --> B[Provider descriptor]
  B --> C[Pace window resolution]
  C --> D[Menu pace metrics]
  C --> E[CLI text and JSON]
  D --> F[Menu bar usage view]
  E --> G[CLI consumers]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR reports tests and CI, but it needs a redacted terminal capture or copied live CLI text/JSON output showing the corrected pace behavior; updating the PR body with that proof should trigger a fresh review, or a maintainer can request @clawsweeper re-review. 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) - No redacted live CLI capture yet demonstrates that descriptor-derived primary, monthly, and tertiary pace appear correctly in both text and JSON output; tests and CI are supplemental rather than real-behavior proof.
  • Complete next step (P2) - No code repair is indicated from the reviewed diff; the remaining merge blocker is contributor-supplied real behavior proof followed by ordinary maintainer review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 6 files affected; 240 added, 54 removed The change spans shared pace resolution, both CLI output forms, and focused regression coverage, so output parity is the key review target.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Keep provider descriptors as the single source of pace eligibility and add a redacted live CLI text and JSON capture for a descriptor-only window plus a tertiary window before merging.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep provider descriptors as the single source of pace eligibility and add a redacted live CLI text and JSON capture for a descriptor-only window plus a tertiary window before merging.

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

Yes, source-reproducible: descriptor-supported windows with missing or monthly-sentinel durations follow the old CLI allowlist/primary-secondary path, while the proposed focused tests exercise the resulting absent pace payloads. I did not execute that path in this read-only review.

Is this the best way to solve the issue?

Yes. Reusing ProviderPaceCapability for CLI rendering is the narrowest maintainable fix because it keeps menu and CLI eligibility and calendar-month duration semantics aligned instead of restoring a second provider allowlist.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This is a bounded correctness repair for provider pace visibility in CLI output without evidence of an outage or data loss.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • 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 tests and CI, but it needs a redacted terminal capture or copied live CLI text/JSON output showing the corrected pace behavior; updating the PR body with that proof should trigger a fresh review, or a maintainer can request @clawsweeper re-review. 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: This is a bounded correctness repair for provider pace visibility in CLI output without evidence of an outage or data loss.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • 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 tests and CI, but it needs a redacted terminal capture or copied live CLI text/JSON output showing the corrected pace behavior; updating the PR body with that proof should trigger a fresh review, or a maintainer can request @clawsweeper re-review. 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:

  • Current CLI gap and proposed repair: The branch changes CLI pace construction to use descriptor-aware reset-window rules and adds tertiary pace to the payload, covering the prior primary/secondary-only path. (Sources/CodexBarCLI/CLIRenderer.swift:574, f2ada4a04abe)
  • Shared monthly-duration behavior: The branch moves calendar-month duration resolution into ProviderPaceCapability so the menu and CLI consume the same provider-defined reset-window calculation. (Sources/CodexBarCore/Providers/ProviderDescriptor.swift:75, f2ada4a04abe)
  • Prior review concern addressed: The earlier review identified missing tertiary descriptor pace in CLI text and JSON; the follow-up discussion states it was repaired and regression coverage was added for Alibaba and OpenCode Go tertiary output. (Tests/CodexBarTests/CLISnapshotTests.swift:653, f2ada4a04abe)
  • Feature provenance: The merged descriptor refactor established provider-declared pace eligibility before this CLI follow-up, making this PR a parity repair rather than a parallel capability design. (Sources/CodexBarCore/Providers/ProviderDescriptor.swift:75, cc8da27cec92)
  • Validation evidence remains test-only: The PR body reports focused Swift tests, formatting/lint checks, and CI success, but it does not include a terminal capture, copied live output, recording, or redacted runtime log that shows corrected CLI text or JSON after the fix. (f2ada4a04abe)

Likely related people:

  • steipete: Authored and merged the descriptor pace-capability refactor that introduced the shared provider-rule surface this CLI follow-up consumes. (role: feature author; confidence: high; commits: cc8da27cec92, 65a2f86d3691; files: Sources/CodexBarCore/Providers/ProviderDescriptor.swift, Sources/CodexBarCLI/CLIRenderer.swift)

Rank-up moves

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

  • Add redacted terminal output from a real CLI run showing descriptor-derived pace in text and JSON, including a tertiary window where practical.

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.

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. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant