Skip to content

fix(sakana): parse weekly/5-hour reset time as UTC, not device-local#1827

Merged
steipete merged 7 commits into
steipete:mainfrom
ss251:fix/sakana-weekly-reset-timezone
Jul 2, 2026
Merged

fix(sakana): parse weekly/5-hour reset time as UTC, not device-local#1827
steipete merged 7 commits into
steipete:mainfrom
ss251:fix/sakana-weekly-reset-timezone

Conversation

@ss251

@ss251 ss251 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #1826.

Summary

console.sakana.ai/billing always server-renders "Resets on " in UTC — confirmed by the raw HTML's embedded "timeZone":"UTC" hydration marker and Sakana's own "Weekly usage resets every Monday at 00:00 UTC" copy elsewhere on the page. The browser only corrects that string to the viewer's local timezone client-side, after JS hydration, which SakanaUsageFetcher (an HTML-only fetcher) never runs.

parseResetDate parsed the raw string using TimeZone.current instead of UTC, so every non-UTC user got a reset countdown off by exactly their UTC offset.

  • Removes the timeZone parameter from parseBillingHTML/parseWindow/parseResetDate entirely — there's no legitimate caller-supplied value here, the source text is always UTC — and hardcodes UTC in the formatter.
  • Updates the existing fixture tests, which previously asserted the buggy local-timezone interpretation (via a shanghaiTimeZone test parameter) as if it were correct.
  • Adds a regression test pinned to a literal Unix timestamp, independent of the date-building test helper, so a future refactor of the helper itself can't silently reintroduce the bug.

Verification

I built this branch (and, for comparison, unmodified main) into the codexbar CLI and ran both against a real Sakana account (Standard plan, active weekly window) with a live cookie, comparing to the same account's billing page rendered in a real browser at the same moment:

Browser (ground truth) main (before) This branch (after)
Weekly reset July 6, 2026, 5:30 AM IST (2026-07-06T00:00:00Z) 2026-07-05T18:30:00Z (5.5h early) 2026-07-06T00:00:00Z

Plan name, 5-hour %, and weekly % were already correct in both and are untouched by this change.

Test plan

  • swift build --target CodexBarCore / swift build --target CodexBar — clean
  • ./Scripts/lint.sh format — 0 files reformatted
  • ./Scripts/lint.sh lint (swiftlint --strict) — 0 violations
  • swift test --filter SakanaUsageFetcherTests — 12/12 pass
  • Verified against a live Sakana account via the built codexbar CLI before and after the fix (table above)

console.sakana.ai/billing always server-renders "Resets on <date>" in
UTC (confirmed via the raw HTML's embedded "timeZone":"UTC" hydration
payload marker, and Sakana's own "Weekly usage resets every Monday at
00:00 UTC" copy elsewhere on the page). The browser only corrects that
text to the viewer's local timezone client-side, after JS hydration --
which this HTML-only fetcher never runs.

SakanaUsageFetcher.parseResetDate parsed the raw string using
TimeZone.current instead, so every non-UTC user got a reset time off
by exactly their UTC offset -- 5.5h early on an IST machine, verified
by building main into the codexbar CLI and running it against a real
account: the tool's own JSON output landed 5.5h before the true reset
instant shown in a browser on the same page at the same moment.

Removes the timeZone parameter entirely (it no longer has a legitimate
caller-supplied value; the source data is always UTC) and hardcodes
UTC in parseResetDate. Updates the existing fixture-based tests, which
previously asserted the buggy local-timezone interpretation as if it
were correct, and adds a regression test pinned to a literal Unix
timestamp independent of the date-building helper.

Fixes steipete#1826.
@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 2, 2026, 7:20 AM ET / 11:20 UTC.

Summary
The PR changes Sakana billing reset parsing from device-local time to UTC and updates parser tests, Sakana docs, and the unreleased changelog.

Reproducibility: yes. source-level: current main passes TimeZone.current into Sakana reset parsing, and the linked report provides live non-UTC Sakana billing evidence. I did not run live provider probes because repository policy warns against real account checks unless explicitly requested.

Review metrics: 2 noteworthy metrics.

  • Diff size: 4 files changed, 48 added, 27 removed. The patch is scoped to one provider parser with matching tests/docs rather than broad provider or settings churn.
  • Global timezone mutation: 1 test suite serialized. The new regression test mutates process-global timezone state, so serialization is relevant to stable parallel test runs.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #1826
Summary: This PR is the candidate fix for the linked Sakana reset-timezone bug report.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Risk before merge

  • [P1] Repository-standard make check and full make test are not both reported in the PR body; several CI build/test jobs were still pending when checked, so maintainers should let standard validation finish before merge.

Maintainer options:

  1. Decide the mitigation before merge
    Merge the focused UTC parsing fix after maintainer-standard validation is satisfied, preserving the serialized timezone regression test and the Sakana docs update.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No automated repair remains; the next action is ordinary maintainer merge validation for the linked Sakana bug fix.

Security
Cleared: No security or supply-chain concern found; the diff changes parser logic, tests, docs, and changelog text without broadening credential handling or dependency execution.

Review details

Best possible solution:

Merge the focused UTC parsing fix after maintainer-standard validation is satisfied, preserving the serialized timezone regression test and the Sakana docs update.

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

Yes, source-level: current main passes TimeZone.current into Sakana reset parsing, and the linked report provides live non-UTC Sakana billing evidence. I did not run live provider probes because repository policy warns against real account checks unless explicitly requested.

Is this the best way to solve the issue?

Yes. Parsing the raw server-rendered Sakana reset string as UTC is the narrowest maintainable fix and avoids adding unnecessary configuration surface.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This fixes a normal-priority Sakana provider accuracy bug affecting reset countdowns for users outside UTC, with limited blast radius.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix live CLI verification against a real Sakana account, compared with browser-rendered ground truth and main-before output.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix live CLI verification against a real Sakana account, compared with browser-rendered ground truth and main-before output.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: Git blame and the merged Sakana provider PR tie the current parser and tests to the provider merge, and later Sakana docs alignment also appears under this name. (role: feature introducer and recent Sakana area contributor; confidence: high; commits: 87635bcc755b, f39879f0c4a7; files: Sources/CodexBarCore/Providers/Sakana/SakanaUsageFetcher.swift, Tests/CodexBarTests/SakanaUsageFetcherTests.swift, docs/sakana.md)
  • LeoLin990405: The merged Sakana provider PR credits original implementation work from this contributor, and that provider merge introduced the affected parser and fixtures. (role: original provider implementation contributor; confidence: medium; commits: 87635bcc755b; files: Sources/CodexBarCore/Providers/Sakana/SakanaUsageFetcher.swift, Tests/CodexBarTests/SakanaUsageFetcherTests.swift)
  • kiranmagic7: Recent Sakana documentation commits added and revised the guide section that described reset timezone behavior before this PR updated it. (role: adjacent docs contributor; confidence: medium; commits: 5e305d3df995, 700cf1bf65ae; files: docs/sakana.md)
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. P2 Normal priority bug or improvement with limited blast radius. labels Jul 1, 2026
- The UTC regression test didn't actually pin the fix: on a UTC CI
  runner, the pre-fix TimeZone.current code would coincidentally
  still produce the correct instant, so the test would pass either
  way. Force NSTimeZone.default to UTC+14 for the test's duration
  (same pattern as BedrockUsageStatsTests) so it only passes when the
  parser genuinely ignores the device timezone. Verified by
  temporarily reverting the fix locally and confirming the test now
  fails with the expected 5.5h-off result before restoring it.
- docs/sakana.md still said reset dates use the device's local time
  zone (TimeZone.current) -- exactly the behavior this PR removes.
  Updated to describe the actual UTC parsing and why, linking steipete#1826.
@ss251

ss251 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both P3 findings from clawsweeper's review:

  • Regression test now actually pins the fix. The original test would have passed on a UTC CI runner even against the pre-fix TimeZone.current code (since TimeZone.current == UTC there). Now forces NSTimeZone.default to UTC+14 for the test's duration (same pattern as BedrockUsageStatsTests). Verified locally by temporarily reverting the fix and confirming the test fails with the expected 5.5h-off result, then restored it.
  • docs/sakana.md updated — it still said reset dates are parsed using the device's local time zone (TimeZone.current), exactly the behavior this PR removes. Now describes the actual UTC parsing and links Sakana AI: weekly/5-hour quota reset time is off by the viewer's UTC offset #1826.

12/12 Sakana tests pass, lint clean.

@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. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. and removed 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. labels Jul 1, 2026
@ss251

ss251 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

clawsweeper's re-review flagged that the new NSTimeZone.default
override test lives in a non-serialized suite, unlike the existing
BedrockUsageStatsTests that uses the same pattern -- unrelated
parallel Swift Testing cases could observe UTC+14 and fail
nondeterministically. Mark @suite(.serialized), matching Bedrock's
precedent exactly.
@ss251

ss251 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the P2 finding from the re-review: marked SakanaUsageFetcherTests @Suite(.serialized), matching the existing BedrockUsageStatsTests precedent for the same NSTimeZone.default mutation pattern. Confirmed serialized execution locally (tests now run strictly one-at-a-time instead of all starting concurrently). 12/12 pass, lint clean.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added 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. and removed 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. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jul 1, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 2, 2026
@steipete steipete merged commit 8fb8c74 into steipete:main Jul 2, 2026
10 checks passed
@steipete

steipete commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Landed as 8fb8c74.

Proof:

  • Exact contributor head 0834fa5382f7f315bcfd5644e3ac35a3e1b609ea passed all 10 required checks.
  • Focused Sakana suite passed 12/12 tests, including a raw UTC fixture verified under a non-UTC device timezone.
  • make check passed with 0 SwiftFormat/SwiftLint violations; make test completed all 45 shards.
  • Reviewed the parser boundary: server-rendered reset timestamps are now interpreted as UTC before conversion for display.

Thanks @ss251 for the strong reproduction and focused fix!

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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sakana AI: weekly/5-hour quota reset time is off by the viewer's UTC offset

2 participants