Skip to content

fix: cool down Claude keychain pre-alerts - #2021

Merged
steipete merged 3 commits into
steipete:mainfrom
harjothkhara:codex/fix-1990-keychain-prealert-cooldown
Jul 11, 2026
Merged

fix: cool down Claude keychain pre-alerts#2021
steipete merged 3 commits into
steipete:mainfrom
harjothkhara:codex/fix-1990-keychain-prealert-cooldown

Conversation

@harjothkhara

@harjothkhara harjothkhara commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Suppress repeated CodexBar Claude OAuth explanatory alerts for six hours after the alert is shown and acknowledged.
  • Reserve an in-flight presentation so concurrent credential reads cannot display duplicates, and do not consume the cooldown when no prompt handler is installed.
  • Add gate and prompt-policy regressions, plus document the behavior.

Product and security boundary

This PR chooses a persisted six-hour acknowledgement cooldown for maintainer review. The cooldown applies only to the CodexBar explanation. The subsequent Security.framework read is unchanged, macOS may still show its authorization prompt, and Claude Never prompt plus global Disable Keychain access remain authoritative.

No live Keychain UI validation was run, per AGENTS.md.

Validation

  • swift build --target CodexBarCore: passed
  • swift build -c release --target CodexBarCore: passed
  • SwiftFormat on touched files: passed
  • Portable repository checks: passed
  • Full Swift tests and SwiftLint are unavailable in this Command Line Tools environment because Apple macro and sourcekitd frameworks are missing; CI is the authoritative full run
  • PR Fix Claude never-prompt Keychain cache bypass (#1991) #2011 addresses a separate periodic Keychain prompt loop and explicitly leaves 每次手动刷新都会有提示框,这是为什么? #1990’s manual-refresh pre-alert UX unchanged, so it does not supersede this PR.
  • A synthetic merge of Fix Claude never-prompt Keychain cache bypass (#1991) #2011 and this PR completed without conflicts, and swift build --target CodexBarCore passed on the combined tree.
  • This uses a separate acknowledgement gate because ClaudeOAuthKeychainAccessGate controls actual Keychain access, while this cooldown suppresses only CodexBar’s explanatory pre-alert.
  • Six hours prevents repeated interruptions during an active work session while allowing the explanation to reappear on a later retry.
  • Codex assisted with implementation, tests, and compatibility analysis

Fixes #1990

@steipete
steipete force-pushed the codex/fix-1990-keychain-prealert-cooldown branch from dc9fa87 to 01dfb69 Compare July 11, 2026 09:18
@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. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 11, 2026
@clawsweeper

clawsweeper Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 11, 2026, 5:25 AM ET / 09:25 UTC.

Summary
The PR adds a persisted six-hour cooldown and in-flight deduplication for Claude OAuth explanatory Keychain alerts, plus focused tests and documentation.

Reproducibility: yes. Source inspection gives a high-confidence path: keep the first synchronous modal open, start a second qualifying load, and the gate returns false while the second caller continues into the Keychain read.

Review metrics: 3 noteworthy metrics.

  • Changed surface: 8 files, 317 additions, 24 deletions. The patch adds persisted state and changes the central Claude OAuth Keychain notification path.
  • Focused coverage: 5 gate cases, 1 repeated-load policy case. The tests cover cooldown state but do not assert that a duplicate concurrent credential read is prevented from reaching the Keychain.
  • Owner follow-up: 2 hardening commits. The repository owner refined persistence and concurrency code and preserved the existing prompt-handler API on this branch.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #1990
Summary: This PR is the candidate fix for the repeated manual-refresh explanation issue; the merged never-prompt cache PR is adjacent and explicitly does not supersede this work.

Members:

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

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦪 silver shellfish
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] Prevent concurrent loads from reaching the Keychain before the active explanation completes and add a regression test for that ordering.
  • Post redacted proof from a freshly built bundle showing cooldown behavior and unchanged macOS authorization, Never prompt, and Disable Keychain access behavior.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR supplies builds and isolated test-store coverage but no after-fix real packaged-app evidence; add a redacted recording or runtime transcript, then update the PR body to trigger a fresh review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] A concurrent Claude OAuth read can reach macOS Keychain authorization while the first CodexBar explanation is still open, bypassing the documented explanation-before-read ordering.
  • [P1] The six-hour timestamp persists across launches, changing upgrade behavior for existing users even though actual authorization and opt-out settings remain intact.
  • [P1] No after-fix packaged-app evidence demonstrates the modal, cooldown, concurrent-read behavior, and preserved opt-outs in a real setup.

Maintainer options:

  1. Block concurrent reads until acknowledgement (recommended)
    Change the gate/caller contract so an in-flight duplicate cannot continue into the Keychain read, and cover that ordering with a focused concurrent test before merge.
  2. Accept concurrent authorization risk
    Merge while explicitly accepting that another credential load may trigger macOS authorization before the first explanation is dismissed.
  3. Pause the cooldown implementation
    Close or defer this PR if preserving strict explanation-before-read ordering would require a broader prompt-coordination design than desired.

Next step before merge

  • [P1] Ask the contributor to fix the concurrent-read ordering defect and supply after-fix native proof; the security-sensitive synchronization choice and real-setup validation should remain human-reviewed.

Security
Needs attention: The cooldown does not broaden credential access, but its in-flight path can let a concurrent Keychain read cross the explanation boundary before acknowledgement.

Review findings

  • [P1] Prevent duplicate callers from reading before acknowledgement — Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthKeychainPreAlertGate.swift:112-114
Review details

Best possible solution:

Make concurrent callers defer, fail without reading, or reuse the first presentation outcome so no potentially interactive Keychain read proceeds until the explanatory modal completes; add a focused concurrent integration test and redacted proof from a freshly built app.

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

Yes. Source inspection gives a high-confidence path: keep the first synchronous modal open, start a second qualifying load, and the gate returns false while the second caller continues into the Keychain read.

Is this the best way to solve the issue?

No. The cooldown and persisted acknowledgement are reasonable, but in-flight deduplication must also prevent or defer the duplicate credential read rather than suppressing only its explanation.

Full review comments:

  • [P1] Prevent duplicate callers from reading before acknowledgement — Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthKeychainPreAlertGate.swift:112-114
    When another credential load arrives while the first explanatory modal is open, this branch returns false from the gate and the caller immediately continues into loadFromClaudeKeychain. That can surface macOS authorization before the user dismisses the first explanation, contrary to the documented pre-read boundary. Defer or reject the duplicate read until the active presentation completes, and add an integration test that asserts the second Keychain read does not run early.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: The PR addresses bounded Claude prompt friction, but its concurrency defect affects a sensitive authorization explanation rather than core availability.
  • add merge-risk: 🚨 auth-provider: Merging changes persisted behavior and concurrency handling in the Claude OAuth credential-read path.
  • add merge-risk: 🚨 security-boundary: A duplicate caller can bypass the intended explanation-before-interactive-Keychain-read ordering while the first modal is still active.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • 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 supplies builds and isolated test-store coverage but no after-fix real packaged-app evidence; add a redacted recording or runtime transcript, then update the PR body to trigger a fresh review or ask a maintainer to comment @clawsweeper re-review.

Label justifications:

  • P2: The PR addresses bounded Claude prompt friction, but its concurrency defect affects a sensitive authorization explanation rather than core availability.
  • merge-risk: 🚨 auth-provider: Merging changes persisted behavior and concurrency handling in the Claude OAuth credential-read path.
  • merge-risk: 🚨 security-boundary: A duplicate caller can bypass the intended explanation-before-interactive-Keychain-read ordering while the first modal is still active.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR supplies builds and isolated test-store coverage but no after-fix real packaged-app evidence; add a redacted recording or runtime transcript, then update the PR body to trigger a fresh review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

Security concerns:

  • [medium] Concurrent read bypasses explanatory ordering — Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthKeychainPreAlertGate.swift:112
    An in-flight duplicate suppresses only the CodexBar alert; it does not stop the duplicate caller from performing the potentially interactive Claude Keychain read while the first explanation remains open.
    Confidence: 0.96

What I checked:

Likely related people:

  • steipete: Introduced the current explanatory pre-alert behavior and authored the two latest commits refining this PR's gate and API compatibility. (role: feature introducer and branch hardener; confidence: high; commits: 24fe798f3769, f04a0eca4171, 01dfb6903025; files: Sources/CodexBar/KeychainPromptCoordinator.swift, Sources/CodexBarCore/KeychainAccessPreflight.swift, Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthCredentials.swift)
  • Yuxin-Qiao: Authored the recently merged Claude never-prompt cache fix on the same OAuth credential and Keychain-policy boundary while explicitly leaving this manual-refresh behavior distinct. (role: adjacent Keychain behavior contributor; confidence: medium; commits: 2d58d098335a; files: Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthCredentials.swift, Tests/CodexBarTests/ClaudeOAuthCredentialsStoreNeverPromptCacheTests.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.

@steipete

Copy link
Copy Markdown
Owner

Maintainer pass complete on exact head 01dfb6903025748889efa1256ae437a40b7ae4bc.

Improvements beyond the original patch:

  • Replaced the exposed begin/finish pairing with one scoped presentation operation, preventing lifecycle mismatches and duplicate in-flight alerts.
  • Anchored the six-hour cooldown after the synchronous explanation closes, so reading time does not shorten the cooldown.
  • Made the DEBUG state store lock-protected for concurrent tests.
  • Preserved the released public KeychainPromptHandler.notify Void signature and kept handler-delivery reporting internal.
  • Added the 0.42.1 changelog entry and credits @harjothkhara.

Proof:

  • ClaudeOAuthKeychainPreAlertGateTests: 5 passed.
  • ClaudeOAuthCredentialsStorePromptPolicyTests: 12 passed, including the public function-signature regression.
  • make test: 603 selections across 51 groups, all passed first attempt; zero retries, timeouts, or failures.
  • make check: passed; zero SwiftFormat/SwiftLint findings.
  • Autoreview: clean; patch classified correct with 0.96 confidence after the API-compatibility repair.
  • Behavior contract reviewed without real provider or Keychain access. Prompt-producing clauses remain intentionally unexecuted under repository safety policy; parser/stub tests verify the cooldown, persistence, completion timing, missing-handler behavior, and opt-out routing without opening a macOS authorization prompt.
  • Exact-head CI: https://github.com/steipete/CodexBar/actions/runs/29147503728

@steipete
steipete merged commit 1dc05e6 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: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. 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