Skip to content

fix(codex): suppress false session restore when resetsAt is unchanged - #2060

Merged
steipete merged 9 commits into
steipete:mainfrom
Yuxin-Qiao:fix/codex-session-false-restore
Jul 11, 2026
Merged

fix(codex): suppress false session restore when resetsAt is unchanged#2060
steipete merged 9 commits into
steipete:mainfrom
Yuxin-Qiao:fix/codex-session-false-restore

Conversation

@Yuxin-Qiao

@Yuxin-Qiao Yuxin-Qiao commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Reuse the canonical limit-reset boundary comparison for Codex session OS notifications.
  • Prevent transient Codex OAuth samples (usedPercent = 0 while resetsAt is unchanged) from posting a false "session restored" notification.
  • Preserve the depleted notification baseline through suppressed restores so API flicker does not loop depleted → restored → depleted.
  • Allow a fallback sample without resetsAt to restore only after the previously known boundary has elapsed.
  • Keep reset-boundary evidence monotonic across missing, stale, or regressed metadata, including while depletion continues.
  • Clear the remembered boundary anywhere account/provider notification state is cleared.

Related to #2054, but scoped separately from weekly confetti work in #2056.

Test plan

Made with Cursor

Yuxin Qiao and others added 2 commits July 11, 2026 20:07
Apply the same session reset-boundary guard used by confetti detection to
OS session quota notifications, so transient Codex usedPercent=0 samples
no longer post a restored notification before the window actually resets.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

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

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

currentResetBoundary: Date?) -> Bool
{
guard let previousResetBoundary else { return true }
guard let currentResetBoundary else { return false }

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 Allow restores after timestamped windows become nil

When Codex auto falls back from OAuth data that had a future reset timestamp to CLI/RPC data that omits resetsAt after that cached reset has passed, this nil current boundary now rejects the real depleted -> available transition. Since handleSessionQuotaTransition preserves the depleted baseline on a rejected restore, subsequent positive samples with no boundary are rejected the same way, so the user never gets the restored notification even though quota is usable again.

Useful? React with 👍 / 👎.

@clawsweeper

clawsweeper Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 11, 2026, 3:02 PM ET / 19:02 UTC.

Summary
The PR gates Codex session-restored notifications on reset-boundary evidence, records that boundary monotonically, centralizes transition-state cleanup, and adds focused regression tests.

Reproducibility: yes. at source level: record a later future boundary for depleted account A, activate account B with an earlier valid boundary, then restore B before A’s boundary elapses. The provider-scoped monotonic guard rejects B’s boundary as regressed.

Review metrics: 3 noteworthy metrics.

  • Patch surface: 8 files, +460/-18. The patch changes notification-state lifecycle across several cleanup paths and adds a substantial regression suite.
  • Test additions: 337 lines added. Focused logic and state-model tests are extensive, but none cover cached account activation.
  • Relevant head delta: 0 affected files changed since prior finding. The current head leaves the previously reported account-switch path unchanged.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Clear or account-scope session transition state during cached token-account activation and add the earlier-boundary switch regression test.
  • [P1] Add redacted after-fix proof from a real setup, preferably a short recording or runtime logs showing false restores suppressed and a legitimate restore delivered.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR reports focused and full-suite tests, including a source-blind contract run, but no after-fix recording, runtime log, terminal output, or linked artifact from a real Codex setup; add redacted proof and update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Switching Codex token accounts can retain the previous account’s later reset boundary and suppress the newly selected account’s legitimate restored notification until that stale boundary passes.
  • [P1] The supplied validation does not establish how the revised notification state behaves with real Codex account switching and live OS notification delivery.

Maintainer options:

  1. Reset state on account activation (recommended)
    Clear the complete session quota transition state before a newly selected cached Codex account can drive notification evaluation, then add focused account-switch coverage.
  2. Scope transition state per account
    Key remaining, source, and reset-boundary baselines by stable account identity if switching back should preserve independent account continuity.

Next step before merge

  • [P1] The contributor needs to repair the account-switch lifecycle defect and supply real-setup proof; ClawSweeper automation cannot provide evidence from the contributor’s Codex accounts.

Security
Cleared: The diff changes local notification state and tests without altering dependencies, permissions, credential handling, downloaded artifacts, or build and release execution paths.

Review findings

  • [P2] Clear the session boundary when switching token accounts — Sources/CodexBar/UsageStore.swift:321
Review details

Best possible solution:

Session notification state should remain monotonic within one account while being cleared or keyed by stable account identity across account changes, with focused coverage and redacted real-setup proof of both suppressed false restores and delivered legitimate restores.

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

Yes at source level: record a later future boundary for depleted account A, activate account B with an earlier valid boundary, then restore B before A’s boundary elapses. The provider-scoped monotonic guard rejects B’s boundary as regressed.

Is this the best way to solve the issue?

No, not yet. Reset-boundary gating is the narrow correct mechanism for transient samples, but its lifecycle must be made account-safe before merge.

Full review comments:

  • [P2] Clear the session boundary when switching token accounts — Sources/CodexBar/UsageStore.swift:321
    The new reset-boundary baseline is keyed only by provider, while cached token-account activation replaces the selected account snapshot without clearing that baseline. An earlier valid boundary for the new account is therefore treated as regressed, which can suppress its restored notification until the old account’s boundary passes. Clear the full transition state on activation or key it by stable account identity, and add the account-switch regression test.
    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 0bb874930d6a.

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • 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 focused and full-suite tests, including a source-blind contract run, but no after-fix recording, runtime log, terminal output, or linked artifact from a real Codex setup; add redacted proof and update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.
  • remove proof: sufficient: Current real behavior proof status is mock_only, not sufficient.
  • remove status: ⏳ waiting on author: Current PR status label is status: 📣 needs proof.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: The PR addresses a bounded provider-specific notification bug, but a concrete cross-account correctness defect remains.
  • merge-risk: 🚨 message-delivery: The new provider-scoped boundary state can suppress a legitimate session-restored OS notification after an account switch.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • 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 focused and full-suite tests, including a source-blind contract run, but no after-fix recording, runtime log, terminal output, or linked artifact from a real Codex setup; add redacted proof and update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: Introduced both session quota notifications and token-account multi-account support, and authored the recent branch commits refining reset-boundary handling. (role: feature owner; confidence: high; commits: 4fc3132c525e, 4373855c6a1a, c6cf35770a81; files: Sources/CodexBar/UsageStore.swift, Sources/CodexBar/SessionQuotaNotifications.swift, Sources/CodexBar/UsageStore+TokenAccounts.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.
Review history (5 earlier review cycles)
  • reviewed 2026-07-11T12:22:05.364Z sha 3a1ee2f :: needs changes before merge. :: [P2] Allow restores after timestamped windows become nil
  • reviewed 2026-07-11T12:32:06.031Z sha 4a46157 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-11T18:26:32.688Z sha 3d20558 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-11T18:33:43.474Z sha 3d20558 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-11T18:56:40.892Z sha a8acfa9 :: needs changes before merge. :: [P2] Clear the session boundary when switching token accounts

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 11, 2026
…tsAt

When Codex falls back to CLI/RPC samples that omit resetsAt after a
previously known session boundary has passed, allow the restored
notification instead of preserving the depleted baseline forever.
Add regression coverage for the repeated fallback sequence.

Co-authored-by: Cursor <cursoragent@cursor.com>
@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. labels Jul 11, 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: 3d20558056

ℹ️ 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 on lines +239 to +240
guard previousBoundary == nil ||
UsageStore.limitResetBoundaryAdvanced(previous: previousBoundary, current: resetBoundary)

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 Reset session boundary on account changes

When the active token account changes, a valid reset time for the new account can be earlier than the previous account's boundary; because this provider-scoped guard treats that as a regression and keeps the old boundary, subsequent depleted -> available samples for the new account can be suppressed until the old account's reset time. The token-account switch path (setActiveTokenAccountIndexactivateCachedTokenAccountSnapshot) does not clear this new boundary state, so the boundary needs to be account-scoped or reset on account activation before rejecting earlier values.

Useful? React with 👍 / 👎.

@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 11, 2026
@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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed 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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 11, 2026
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 11, 2026
@steipete
steipete merged commit 72acff1 into steipete:main Jul 11, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. 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.

2 participants