test(CodexBarCore): isolate Claude keychain state under unit tests - #2441
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5b618b722
ℹ️ 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".
ss251
changed the title
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: test(CodexBarCore): isolate Claude keychain state under unit tests This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
|
Review follow-up at
Focused follow-up validation passed with no remaining review findings. GitHub CI is rerunning on the scoped head; if macOS shard 0 reproduces the known SpendDashboard date/defaults failure before #2390 lands, that is an external base failure rather than a Claude isolation regression. |
|
Final scoped-head CI result for
No Claude OAuth/keychain test failed. I intentionally removed the temporary duplicate SpendDashboard patch from this PR, so the scoped diff and the resolved P2 review fix remain clean. Once #2390 lands and this branch is rebased by the normal repository flow, the external shard failure should clear. |
|
Merged after a fresh hostile review of exact head Verification:
Thanks @ss251 for the focused test-isolation layer and for preserving the production behavior boundary. |
2 participants
Intent
Submit a focused upstream safety patch discovered while validating the recurring Claude Code-credentials prompt fix. CodexBar's standard test suite must never inherit the developer's persisted Claude prompt preference, resolve the real Claude credentials file, share pending cache-clear state across tests, or lose the captured prompt policy inside detached delegated-refresh work. Preserve release behavior and explicit live-test opt-in; live proof must fail closed unless CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS=1 exposes real user state. Add deterministic regression coverage, address the P2 review comment, and keep unrelated SpendDashboard stabilization out because PR #2390 already owns it. This complements, but does not compete with, the production ownership-boundary fix in PR #2380. Do not edit CHANGELOG.md. Existing unrelated SpendDashboard CI failures must be reported honestly rather than hidden. Skip the CI monitor on this follow-up because GitHub will rerun the known external #2390-dependent shard, and the pipeline must not auto-add duplicate SpendDashboard fixes again.
What Changed
CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS=1, with regression coverage for isolation and opt-in gates.Risk Assessment
✅ Low: DEBUG/test-only isolation is well-bounded, release paths are unchanged, the prior process-shared pending sink was replaced with per-scope TaskLocal stores plus ephemeral sinks, and deterministic regression coverage matches the required safety invariants.
Testing
Exercised the isolation acceptance criteria with focused Swift Testing suites (not the full suite): prompt-preference isolation, credentials-file isolation, pending cache-clear isolation, detached delegated-refresh prompt-policy capture, and live-keychain fail-closed gating; confirmed CHANGELOG/SpendDashboard stay out of the patch. All isolation tests passed, and LIVE=1 without ALLOW intentionally recorded that real user state requires CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS=1. No UI surface changed, so evidence is CLI transcripts rather than screenshots.
Evidence: Live proof fails closed without CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS=1
✘ Test "live background Auto skips the opaque Claude Keychain boundary" recorded an issue at ClaudeKeychainLiveProofTests.swift:25:25: Issue recorded ↳ Live proof requires CODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESS=1 to read the real prompt policyEvidence: Safety-named isolation tests (prompt pref, credentials file, pending cache clear)
Evidence: ClaudeOAuthCredentialsStoreTests including three new safety isolation tests
Evidence: Prompt policy, live-proof gate, delegated refresh, KeychainNoUI suites
Evidence: Temporary keychain cache + isolated security CLI suites
Evidence: Live-proof unit gate (empty env disallows real keychain access)
Evidence: Acceptance criteria transcript
Evidence: Diff scope (no CHANGELOG / no SpendDashboard)
Source: Diff scope (no CHANGELOG / no SpendDashboard) (local file:
/var/folders/nc/j5wdfhws2q56jzdcj_jkdvww0000gn/T/no-mistakes-evidence/01KYA7N2C9PSSPVHYBQTQJTGHQ/intent-scope.log)/var/folders/nc/j5wdfhws2q56jzdcj_jkdvww0000gn/T/no-mistakes-evidence/01KYA7N2C9PSSPVHYBQTQJTGHQ/key-proof-lines.txt)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
Sources/CodexBarCore/Providers/Claude/ClaudeOAuth/ClaudeOAuthCredentials.swift:134- Intent requires the standard suite must never "share pending cache-clear state across tests", but the auto-isolation sink is a single process-staticPendingCacheClearMemoryStore(isolatedTestPendingCacheClearStore, returned fromcurrentPendingCodexBarOAuthKeychainCacheClearStorewhen no TaskLocal override is set). That still lets never-modeinvalidateCache/markPendingand_resetCredentialsFileTrackingForTesting(which now clears pending whenever isolation is active) couple concurrent/non-overridden tests. App-suite writes are correctly avoided; confirm whether process-wide memory sharing is acceptable, or whether pending should be TaskLocal/per-test by default under isolation.🔧 Fix: Isolate pending cache-clear state per test scope
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
swift test --filter 'safety '(prompt preference, credentials-file, pending-cache-clear, and related safety gates)swift test --filter ClaudeOAuthCredentialsStoreTestsswift test --filter ClaudeOAuthCredentialsStorePromptPolicyTests --filter ClaudeKeychainLiveProofTests --filter ClaudeOAuthDelegatedRefreshCoordinatorTests --filter KeychainNoUIQueryTestsswift test --filter ClaudeOAuthCredentialsStoreTemporaryKeychainCacheTests --filter ClaudeOAuthCredentialsStoreIsolatedSecurityCLITestsLIVE_CLAUDE_KEYCHAIN_PROOF=1withCODEXBAR_ALLOW_TEST_KEYCHAIN_ACCESSunset →swift test --filter 'live background Auto skips'(fail-closed live proof)swift test --filter 'live proof requires explicit access'git diff --name-onlybase..target for CHANGELOG.md / SpendDashboard scope check✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.