diff --git a/Tests/OrreryTests/AccountMigrationTests.swift b/Tests/OrreryTests/AccountMigrationTests.swift index 36f34c1..1ae3b81 100644 --- a/Tests/OrreryTests/AccountMigrationTests.swift +++ b/Tests/OrreryTests/AccountMigrationTests.swift @@ -17,6 +17,10 @@ struct AccountMigrationTests { try? FileManager.default.createDirectory(at: parent, withIntermediateDirectories: true) let home = parent.appendingPathComponent(".orrery") let cleanup: () -> Void = { + // The migration runs migrateOrigin for ALL tools; claude's path reads + // the real (global) login Keychain, leaving stray items for any claude + // account it creates. Sweep them before removing the temp home. + sweepClaudeKeychain(home: home) // Removing the parent removes both `.orrery` and any backup siblings. try? FileManager.default.removeItem(at: parent) } diff --git a/Tests/OrreryTests/TestHelpers.swift b/Tests/OrreryTests/TestHelpers.swift index c4c3e98..51342d6 100644 --- a/Tests/OrreryTests/TestHelpers.swift +++ b/Tests/OrreryTests/TestHelpers.swift @@ -1,4 +1,31 @@ import Foundation +@testable import OrreryCore + +/// Delete any per-account claude Keychain items for accounts under `home`. +/// The macOS login Keychain is GLOBAL — `ORRERY_HOME` does not isolate it — so a +/// test that creates/copies a claude credential leaves a stray +/// `Claude Code-orrery-*` item in the developer's real login keychain unless it is +/// swept. Deletes by service name (matches regardless of the account field). +/// No-op off macOS. +func sweepClaudeKeychain(home: URL) { + #if os(macOS) + for acct in (try? AccountStore(homeURL: home).list(tool: .claude)) ?? [] { + // Delete BOTH the deterministic per-account service (what + // storePassword/copyKeychainItem use — even when metadata.keychainItem + // was never persisted) and any explicit keychainItem, by service name. + for service in Set([ClaudeKeychain.serviceName(forOrreryAccount: acct.id), + acct.keychainItem].compactMap { $0 }.filter { !$0.isEmpty }) { + let p = Process() + p.executableURL = URL(fileURLWithPath: "/usr/bin/security") + p.arguments = ["delete-generic-password", "-s", service] + p.standardOutput = FileHandle.nullDevice + p.standardError = FileHandle.nullDevice + try? p.run() + p.waitUntilExit() + } + } + #endif +} /// Process-global lock serializing every test that mutates the global ORRERY_HOME /// env var. swift-testing's `.serialized` only serializes within a single suite; @@ -33,6 +60,9 @@ func withIsolatedHome(_ body: () throws -> Void) rethrows { setenv("ORRERY_USER_HOME", tmpDir.path, 1) unsetenv("ORRERY_ACTIVE_ENV") defer { + // Sweep any claude Keychain items the body created (global keychain is not + // isolated by ORRERY_HOME). Runs before the temp dir is removed. + sweepClaudeKeychain(home: tmpDir) if let savedHome { setenv("ORRERY_HOME", savedHome, 1) } else { diff --git a/docs/superpowers/specs/2026-07-07-keychain-test-isolation-design.md b/docs/superpowers/specs/2026-07-07-keychain-test-isolation-design.md new file mode 100644 index 0000000..593fe0c --- /dev/null +++ b/docs/superpowers/specs/2026-07-07-keychain-test-isolation-design.md @@ -0,0 +1,80 @@ +# Keychain test isolation — Design + +**Goal:** Stop the test suite from leaving stray credentials in the developer's +real macOS login Keychain. ~2900 `Claude Code-orrery-*` items had accumulated and +a full `swift test` added ~7 per run. + +**Approach: test-side cleanup only — NO production changes.** This is a test-only +problem (production keychain writes are correct and necessary for real logins), so +it is fixed entirely in the test harness. An earlier draft proposed routing +production keychain writes through an injectable `KeychainAccess` seam; that was +**rejected** — it would churn production signatures for a test-only concern. + +**Status:** implemented (branch `feat/keychain-test-isolation`). No production code +touched. + +--- + +## Root cause + +The macOS login Keychain is **global** — `ORRERY_HOME` / `ORRERY_USER_HOME` do not +scope it, and setting `$HOME` breaks Keychain resolution. So a test that exercises a +claude keychain **write** against the real default service `Claude Code-credentials` +creates a stray per-account item (`Claude Code-orrery-`) in the real keychain. + +Primary offender: `AccountMigrationTests` → `AccountMigration.runIfNeeded` → +`migrateOrigin(.claude)` → `extractCredential(isOrigin:)` reads the real +`Claude Code-credentials` and `storePassword`s a copy under a new per-account +service — even though the test's home is isolated (the keychain isn't). ~7/run. + +`ClaudeKeychainTests` only test `service(for:)` (pure string logic — no I/O). The +known claude keychain tests (`AccountLoginFlow` macOS, `AccountAddFinalize` v3.1) +already clean up via `KeychainTestSupport.delete`. + +## Fix (test-only) + +`Tests/OrreryTests/TestHelpers.swift`: add + +```swift +func sweepClaudeKeychain(home: URL) { + #if os(macOS) + for acct in (try? AccountStore(homeURL: home).list(tool: .claude)) ?? [] { + for service in Set([ClaudeKeychain.serviceName(forOrreryAccount: acct.id), + acct.keychainItem].compactMap { $0 }.filter { !$0.isEmpty }) { + // security delete-generic-password -s (matches any account field) + } + } + #endif +} +``` + +Deletes each isolated claude account's per-account keychain service (the +deterministic `serviceName(forOrreryAccount:)` even when `metadata.keychainItem` +was never persisted, plus any explicit `keychainItem`), by service name. + +Call it from the teardown of both isolated-home helpers, before the temp home is +removed: +- `withIsolatedHome` defer (covers unit tests). +- `AccountMigrationTests.makeTempHome` cleanup (covers the migration suite — the + primary offender, which uses its own temp home). + +## Verification + +Manual before/after check (from the PR #22 pattern): count unique +`Claude Code-orrery-*` services before and after a full `swift test`. Result: +**per-run growth reduced from ~7 to ~1**; backlog cleaned (~2894 → 7 legit). + +## Residual (open) + +A stubborn **~1/run** remains from a source not identifiable by static analysis +(all known writers clean up; `PhantomTriggerTests` uses a shell *stub* not the real +binary; migration-read tests have no `keychainItem`). Pinning it needs **runtime +instrumentation**: temporarily print a stack trace inside `ClaudeKeychain`'s write +functions (`copyKeychainItem` / `storePassword` / `addPassword`), run the suite, +read the offending caller, revert the instrumentation, and add cleanup there. +Tracked as a focused follow-up. + +## Out of scope +- Any production change (rejected — test-only concern). +- Read-path handling (a keychain read miss is harmless). +- `ClaudeKeychainTests` (no I/O).