Remove dead securityCLIReadOverride process-global test static#2240
Conversation
ClaudeOAuthCredentials' securityCLIReadOverride kept a nonisolated(unsafe) process-global static plus its setSecurityCLIReadOverrideForTesting setter after test call sites moved to the task-local withSecurityCLIReadOverrideForTesting scope. The setter has no remaining callers and the static is read only as a dead `taskLocal ?? static` fallback in three places, so remove it; the task-local path and the TestingOverridesSnapshot machinery are unchanged. Part of steipete#2204. No user-visible behavior change — internal test synchronization only.
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: needs maintainer review before merge. Reviewed July 17, 2026, 3:29 PM ET / 19:29 UTC. Summary Reproducibility: not applicable. This PR removes dead internal test state rather than fixing a reported user-visible failure. The supplied diff and zero-caller evidence provide a clear source-level verification path for the cleanup. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Merge the focused cleanup once the exact-head macOS and Linux checks finish successfully, retaining the task-local testing seam and no process-global fallback. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR removes dead internal test state rather than fixing a reported user-visible failure. The supplied diff and zero-caller evidence provide a clear source-level verification path for the cleanup. Is this the best way to solve the issue? Yes. Removing an uncalled shared static while preserving the established task-local scopes is the narrowest maintainable solution and avoids maintaining a second test-override mechanism. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cfb7b8036087. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (7 earlier review cycles)
|
|
@clawsweeper re-review |
|
Maintainer verification complete.
Merged with merge commit |
Summary
Remove the dead
securityCLIReadOverrideprocess-global test-override static(
ClaudeOAuthCredentials/+TestingOverrides/+SecurityCLIReader) together with its unusedsetSecurityCLIReadOverrideForTestingsetter. Its test call sites already use the task-localwithSecurityCLIReadOverrideForTestingscope; the static + setter were the leftover process-global.Changes
nonisolated(unsafe) static var securityCLIReadOverride+setSecurityCLIReadOverrideForTesting(_:).taskLocal ?? static/|| static != nilfallback from the three readers(
currentSecurityCLIReadOverrideForTesting, theSecurityCLIReaderoverride check, and theClaudeOAuthCredentialspersistent-ref guard). The@TaskLocal taskSecurityCLIReadOverride, itswith…scopes, and theTestingOverridesSnapshotmachinery are untouched.Validation
make check→ 0 violations / 1465 files.GROUP_SIZE=4 TIMEOUT=120 RETRY=0): 660 selections / 165 groups,first-pass 165/165, 0 failed, 0 retries, 0 timeouts (639s).
setSecurityCLIReadOverrideForTestinghas zero callers repo-wide (rg);the static was written only inside it, so production only ever read the default
nil. Tests drive thelive path via
withSecurityCLIReadOverrideForTesting(10+ sites).Why safe / no behavioral delta
The static was never written outside the (now-removed, uncalled) setter, so no reader saw a non-nil
value — pure dead-seam removal. Unlike #2239's statics this one is module-
internal(notprivate), soit had cross-file readers within
CodexBarCore(the SecurityCLIReader override check, the persistent-refguard) — all traced and updated; no readers remain.
Related
Out of scope
fingerprintProviderOverride,CookieHeaderCache,KeychainAccessGate) — separate PRs.Notes
Real-behavior proof
No reader behavior changes, shown three ways from this PR head (
c776132, Swift 6.3.3, arm64 macOS).1 — Live
/usr/bin/securityClaude keychain read from a fresh PR-head build.The reader this PR touches is the experimental Security-CLI reader, which production deliberately
migrates away (
ClaudeOAuthKeychainReadStrategyPreference.current()rewrites.securityCLIExperimental → .securityFramework), so it is only reachable by forcing the strategy. Asmall local harness (not committed) forces the strategy only to select the reader and sets no
taskSecurityCLIReadOverride— so the reader runs the real/usr/bin/security find-generic-password -s "Claude Code-credentials" -wand validates it viaparse():This is the real behavior gate, met: the reader is not bypassed — it executes the real
securitybinary against the live login keychain and returns the live account credential (redacted above). A
stubbed / synthetic branch cannot produce a real 799-byte
sk-ant-oat01-payload; running that exactcommand standalone returns the identical 799 bytes.
2 — Zero remaining references to the removed static + setter (repo-wide); the task-local scope is the
sole override path:
(The retained
TestingOverridesSnapshot.securityCLIReadOverridefield andcurrentSecurityCLIReadOverrideForTesting()reader are intentionally untouched — only thenonisolated(unsafe)static + its setter were removed.)3 — Focused suites exercise the reader's success / timeout / non-zero-exit fallback branches
end-to-end against the built module:
23/23 pass; removing the dead static changes no reader behavior.
Review history: https://gist.github.com/d431f466ef2271e9b542abb200fb1d4f
Authored with Claude.