Skip to content

feat: use configured work days for weekly pace#1451

Draft
steipete wants to merge 9 commits into
mainfrom
feat/workday-aware-pace-maintainer
Draft

feat: use configured work days for weekly pace#1451
steipete wants to merge 9 commits into
mainfrom
feat/workday-aware-pace-maintainer

Conversation

@steipete

Copy link
Copy Markdown
Owner

Summary

  • use configured work days for standard weekly pace calculations
  • keep historical Codex pacing unchanged when historical data is available
  • compute ETA in work time and map it back to wall-clock time
  • avoid declaring zero usage safe before the first configured workday has elapsed

Supersedes #1357 because repository CI would not start for the fork head. Preserves @pstanton237's commits and authorship.

Fixes #1356.

Validation

  • swift test --filter UsagePace
  • make check
  • autoreview clean after one accepted edge-case fix

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed June 13, 2026, 5:51 AM ET / 09:51 UTC.

Summary
The PR threads configured work days into standard weekly pace and ETA calculations, updates localized setting copy and release notes, and adds focused UsagePace regression tests.

Reproducibility: not applicable. as a bug reproduction. Source inspection and the linked issue show current main uses seven-day linear weekly pace while this PR deliberately changes the product semantics for configured work days.

Review metrics: 3 noteworthy metrics.

  • Changed surface: 16 files, 433 additions, 24 deletions. The runtime change is small, but localization and tests make the PR broad enough to review as a user-visible semantic change.
  • Pace regression coverage: 283 test lines added. The new tests cover the core workday calculations and edge cases that determine whether the product behavior is credible.
  • Localized setting copy: 10 localized strings changed. The PR intentionally advertises the broadened setting meaning across supported localizations, which reinforces that this is a product semantics decision.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🐚 platinum hermit
Patch quality: 🦐 gold shrimp
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:

  • Resolve whether existing marker-only stored values may alter pace after upgrade.
  • Wait for the latest head's CI to finish before any merge decision.

Risk before merge

  • [P1] Existing users who selected four or five work days for marker layout will get different pace labels and depletion ETAs immediately after upgrade because the same persisted value now affects quota-consumption pace.
  • [P1] The owner has explicitly left the draft open for a product decision on whether that persisted setting should become both marker and pace policy.

Maintainer options:

  1. Preserve upgrade semantics
    Keep existing stored work-day values marker-only and add a distinct opt-in pace setting or migration that requires clear user intent.
  2. Approve the broadened meaning
    Explicitly accept in the PR context that existing four- and five-day marker preferences will also change pace labels and ETA after upgrade.
  3. Leave the draft paused
    Keep the draft open or close it without merge if neither compatibility contract is acceptable for current users.

Next step before merge

  • [P2] Human review is required because the remaining blocker is the persisted-setting compatibility/product contract, not a narrow mechanical repair.

Security
Cleared: The diff changes local pace logic, tests, localized copy, and release notes without touching credentials, permissions, dependencies, workflows, downloads, or other supply-chain execution paths.

Review findings

  • [P1] Preserve marker-only upgrades or document the migration — Sources/CodexBar/UsageStore+HistoricalPace.swift:12
Review details

Best possible solution:

Choose and document the persisted-setting contract: either explicitly approve unified marker-and-pace semantics for existing values, or preserve marker-only upgrade behavior and add a separate opt-in or migration with fresh-install and upgrade coverage.

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

Not applicable as a bug reproduction. Source inspection and the linked issue show current main uses seven-day linear weekly pace while this PR deliberately changes the product semantics for configured work days.

Is this the best way to solve the issue?

Unclear pending product approval. The algorithm is focused and well covered, but silently reusing an existing stored marker preference is not the safest upgrade path unless the owner explicitly accepts that contract.

Full review comments:

  • [P1] Preserve marker-only upgrades or document the migration — Sources/CodexBar/UsageStore+HistoricalPace.swift:12
    This directly reuses weeklyProgressWorkDays, which current main stores for visual weekly bar markers. Users who already selected 4 or 5 days for marker layout will receive different pace labels and ETAs after upgrade, so preserve marker-only semantics, add an explicit opt-in/migration, or record maintainer acceptance of that compatibility break before merge.
    Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P3: This is a low-urgency pacing ergonomics feature awaiting product semantics review, not an urgent broken provider workflow.
  • merge-risk: 🚨 compatibility: Merging expands the runtime meaning of an existing persisted display preference and can change upgraded users' pace and ETA without a separate opt-in.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external-contributor proof gate does not apply to this owner-authored draft; prior linked contribution includes visible before/after app screenshots and this PR includes exact-head validation comments.
Evidence reviewed

What I checked:

Likely related people:

  • Yuxin-Qiao: Introduced the persisted weekly workday display setting, marker renderer, and related tests that this PR now expands beyond display-only semantics. (role: introduced behavior; confidence: high; commits: e9b7e25f851b; files: Sources/CodexBar/MenuCardQuotaWarningMarkers.swift, Sources/CodexBar/PreferencesDisplayPane.swift, Sources/CodexBar/SettingsStore+Defaults.swift)
  • Remedy92: GitHub path history points to the original weekly pace indicator commit that introduced the linear UsagePace.weekly model being changed here. (role: introduced weekly pace model; confidence: high; commits: a679311227fc; files: Sources/CodexBarCore/UsagePace.swift, Sources/CodexBar/UsagePaceText.swift, Tests/CodexBarTests/UsagePaceTextTests.swift)
  • ViperThanks: Recent history on UsageStore+HistoricalPace generalized quota pace display and touched the fallback/generic pace path this PR now threads work days through. (role: adjacent pace contributor; confidence: medium; commits: 0d60c9067352; files: Sources/CodexBar/UsageStore+HistoricalPace.swift, Tests/CodexBarTests/HistoricalUsagePaceBackfillAuthorityTests.swift)
  • steipete: The repository owner carried the contributor branch forward, authored the latest edge-case refinements, co-authored the earlier workday-marker merge, and explicitly left this draft for persisted-setting semantics review. (role: product decision owner and recent area contributor; confidence: high; commits: e9b7e25f851b, 19a120a90aee; files: Sources/CodexBarCore/UsagePace.swift, Tests/CodexBarTests/UsagePaceTests.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 the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 11, 2026
@steipete steipete force-pushed the feat/workday-aware-pace-maintainer branch 2 times, most recently from 795bfac to 22903e3 Compare June 12, 2026 09:08
@steipete

Copy link
Copy Markdown
Owner Author

Rebased onto current main (8e50e885) and resolved the stale switcher changelog conflict. Exact head: 22903e3f.

Autoreview caught and this head fixes one edge case: a fully depleted quota after the final configured workday now remains exhausted instead of reporting that it lasts through the weekend.

Proof:

  • pace/history focused suite: 58 tests passed
  • added exhausted-weekend regression
  • make check: passed
  • exact-head autoreview: clean, confidence 0.84

Fresh CI is running. Keeping this draft/unmerged for product review of the workday pacing semantics.

@steipete steipete force-pushed the feat/workday-aware-pace-maintainer branch from 22903e3 to c994ca7 Compare June 12, 2026 10:11
@steipete

Copy link
Copy Markdown
Owner Author

Rebased onto current main (3fdb549a), which includes the deterministic StatusMenuPersistentRefreshTests fix that was the only failure on the previous head.

Exact head: c994ca7d.

Proof:

  • swift test --filter UsagePace: 70 tests passed.
  • make check: clean.
  • exact-head autoreview: clean, 0.83 confidence.

Fresh CI is running. Keeping this draft and unmerged for the existing product decision on changing the semantics of the persisted work-day setting.

@steipete steipete force-pushed the feat/workday-aware-pace-maintainer branch 2 times, most recently from b695eb5 to 999b083 Compare June 12, 2026 14:00
@clawsweeper clawsweeper Bot added 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. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 12, 2026
@steipete

Copy link
Copy Markdown
Owner Author

Prepared on exact head 999b083de9ba722196b3b1cf4860bcd702b1edb5.

Proof:

  • Rebased onto current main at preparation time.
  • Focused pace suites: 68 tests across 4 suites passed.
  • make check
  • Codex autoreview: no accepted/actionable findings.
  • GitHub CI: both macOS runs, both Linux x64 runs, both Linux ARM64 runs, and GitGuardian passed. The first Linux x64 attempt hit an unrelated Text file busy PTY-test flake; rerun passed.

Not merging autonomously: interpreting configured work days as quota-consumption pace changes user-facing product semantics.

pstanton237 and others added 9 commits June 13, 2026 10:41
The existing weeklyProgressWorkDays setting only affected visual markers
on the progress bar. This change threads the work-days value into
UsagePace.weekly() so that the expected usage curve distributes 100%
across configured work days only. Non-work days contribute zero expected
usage, producing a flat curve on weekends.

Users who work Mon-Fri and consume 100% by Friday now see "on pace"
instead of a misleading ~29% deficit from the 7-day linear model.

Fixes #1356
Address Codex bot review feedback: when the weekly reset time is not at
midnight, the previous 24-hour-slice approach could misclassify hours
near day boundaries. Now uses calendar.startOfDay to split intervals at
local midnight, ensuring each slice is classified by its actual calendar
weekday regardless of reset time offset.
Align the valid workDays range (>= 2) with the UI picker (Off/4/5/7)
and marker path (2...7) to prevent inconsistent behavior from stale
or manual values.
@steipete steipete force-pushed the feat/workday-aware-pace-maintainer branch from 999b083 to 19a120a Compare June 13, 2026 09:47
@steipete

Copy link
Copy Markdown
Owner Author

Prepared on exact head 19a120a90aeee52b37a82d4ad46cf270a711effd.

Proof:

  • focused pace tests passed, including the pre-first-workday and exhausted-weekend regressions
  • make check passed
  • Codex autoreview reported no accepted/actionable findings
  • GitHub CI is fully green: macOS, Linux x64, Linux ARM64, and GitGuardian

Keeping this draft and unmerged. The remaining question is product/upgrade semantics: whether the existing persisted work-day display preference should also control quota pace and ETA.

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. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. 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.

feat: factor weeklyProgressWorkDays into pace calculation

2 participants