From 5f4b0f6a4255faad486685c8d9a610e356883491 Mon Sep 17 00:00:00 2001 From: Grady Zhuo Date: Tue, 7 Jul 2026 21:39:13 +0800 Subject: [PATCH 1/3] [SPEC] keychain test isolation design --- ...26-07-07-keychain-test-isolation-design.md | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-07-keychain-test-isolation-design.md 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..dd0c33e --- /dev/null +++ b/docs/superpowers/specs/2026-07-07-keychain-test-isolation-design.md @@ -0,0 +1,74 @@ +# Keychain test isolation — Design + +**Goal:** Stop the test suite from writing to the developer's real macOS login +Keychain. A full `swift test` currently adds ~12 stray `Claude Code-orrery-*` +items per run (≈2900 had accumulated). Route the two production keychain **write** +paths through the injectable `KeychainAccess` seam so the polluting tests inject a +fake and never touch the real Keychain. + +**Status:** proposed. Test-isolation refactor; no user-facing behavior change +(production keeps `.live`). Branch `feat/keychain-test-isolation`. + +--- + +## Root cause + +The macOS login Keychain is global — `ORRERY_HOME`/`ORRERY_USER_HOME` do not scope +it, and setting `$HOME` breaks Keychain resolution. Two production write primitives +run against the real default service `Claude Code-credentials` when tests exercise +them: + +- `ClaudeKeychain.copyKeychainItem` — via `AccountLoginFlow.importFrom` (macOS + claude). Hit by `AccountLoginFlowTests` (macOS claude test) and the + `AccountAddFinalize` "v3.1 layout" test (`AccountCommandsTests`). +- `ClaudeKeychain.storePassword` — via `AccountMigration.copyCredentialIntoPool` + ← `AccountMigration.migrateAccount(tool:…)` (the v3.0.4→pool credential path). + +`ClaudeKeychainTests` only tests `service(for:)` (pure string derivation) — no I/O, +does not pollute. Read paths (`password(forService:)`, `keychainItemExists`) don't +pollute (a miss is harmless), so they stay direct — only writes are seamed (YAGNI). + +## Design + +Extend the existing `KeychainAccess` seam (added in PR #22 — currently `itemExists` ++ `copyItem`) with the second write primitive: + +```swift +public var storePassword: @Sendable (_ password: String, _ accountID: String) -> Bool +``` + +`.live.storePassword = ClaudeKeychain.storePassword(_:forOrreryAccount:)`. + +Thread a `keychain: KeychainAccess = .live` parameter through the two write paths and +their callers; production omits it (`.live`), tests pass a fake: + +| Production symbol | change | +|---|---| +| `AccountLoginFlow.importFrom(stagingDir:into:)` | add `keychain: KeychainAccess = .live`; macOS-claude branch uses `keychain.copyItem` instead of `ClaudeKeychain.copyKeychainItem` | +| `AccountMigration.migrateAccount(tool:…)` + `copyCredentialIntoPool` | add `keychain: KeychainAccess = .live`; use `keychain.storePassword` instead of `ClaudeKeychain.storePassword` | +| `AccountAddFinalizeCommand` | thread `.live` through to `importFrom` (+ `migrateAccount` if it calls the tool-level one); a test can inject via an internal seam if needed | +| `OriginAccountSeeder` | already holds a `KeychainAccess` — pass it to `importFrom` (was calling the no-arg form) | + +## Tests to convert (inject a fake, assert no real-Keychain touch) + +- `AccountLoginFlowTests` macOS-claude test → inject a recording fake for `copyItem`. +- `AccountCommandsTests` `AccountAddFinalize` "v3.1 layout" test → inject a fake so + `importFrom` (and any `migrateAccount`) never write the real Keychain. +- Any test calling `AccountMigration.migrateAccount(tool:…)` → inject a fake. +- New guard test: run the seeder/import/finalize with a fake and assert the fake's + recorded calls (not the real Keychain). + +**Isolation regression check:** the manual step from PR #22 (count +`Claude Code-orrery-*` before/after a full `swift test`) must show **no growth**. +Codex/gemini paths are already file-based and unaffected. + +## Out of scope +- Read-path seaming (harmless; YAGNI). +- `ClaudeKeychainTests` (no I/O). +- Any production behavior change (`.live` everywhere in prod). + +## Open question +- Whether `AccountAddFinalizeCommand` needs an injectable entry point for its test, + or whether the test can drive `AccountLoginFlow.importFrom(…, keychain:)` + + `migrateAccount(…, keychain:)` directly. Prefer the latter (no command-level + plumbing) if the finalize test can be reframed around the two seam-aware calls. From ff34e980371a958cf5d2d7fc164513d79aa4258a Mon Sep 17 00:00:00 2001 From: Grady Zhuo Date: Tue, 7 Jul 2026 21:57:23 +0800 Subject: [PATCH 2/3] [TEST] sweep stray claude Keychain items in isolated-home teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macOS login Keychain is global — ORRERY_HOME does not isolate it — so tests that run claude keychain writes (notably AccountMigrationTests via AccountMigration.runIfNeeded -> migrateOrigin(claude), which reads the real Claude Code-credentials service) leave stray Claude Code-orrery-* items in the developer's real keychain (~2900 had accumulated; the suite added ~7/run). Add sweepClaudeKeychain(home:) and call it from withIsolatedHome and AccountMigrationTests.makeTempHome teardown — deletes each isolated claude account's per-account service (by name) after the test. Test-only; no production change. Reduces per-run growth from ~7 to a residual ~1 from an as-yet-unpinned source (needs runtime instrumentation; tracked as follow-up). --- Tests/OrreryTests/AccountMigrationTests.swift | 4 +++ Tests/OrreryTests/TestHelpers.swift | 30 +++++++++++++++++++ 2 files changed, 34 insertions(+) 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 { From 5db33bd71eec612b8988e0df7061a5bb2d98dabd Mon Sep 17 00:00:00 2001 From: Grady Zhuo Date: Wed, 8 Jul 2026 01:28:18 +0800 Subject: [PATCH 3/3] [SPEC] rewrite keychain isolation to the test-side-cleanup approach (no production seam) --- ...26-07-07-keychain-test-isolation-design.md | 106 +++++++++--------- 1 file changed, 56 insertions(+), 50 deletions(-) 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 index dd0c33e..593fe0c 100644 --- a/docs/superpowers/specs/2026-07-07-keychain-test-isolation-design.md +++ b/docs/superpowers/specs/2026-07-07-keychain-test-isolation-design.md @@ -1,74 +1,80 @@ # Keychain test isolation — Design -**Goal:** Stop the test suite from writing to the developer's real macOS login -Keychain. A full `swift test` currently adds ~12 stray `Claude Code-orrery-*` -items per run (≈2900 had accumulated). Route the two production keychain **write** -paths through the injectable `KeychainAccess` seam so the polluting tests inject a -fake and never touch the real Keychain. +**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. -**Status:** proposed. Test-isolation refactor; no user-facing behavior change -(production keeps `.live`). Branch `feat/keychain-test-isolation`. +**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. Two production write primitives -run against the real default service `Claude Code-credentials` when tests exercise -them: +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. -- `ClaudeKeychain.copyKeychainItem` — via `AccountLoginFlow.importFrom` (macOS - claude). Hit by `AccountLoginFlowTests` (macOS claude test) and the - `AccountAddFinalize` "v3.1 layout" test (`AccountCommandsTests`). -- `ClaudeKeychain.storePassword` — via `AccountMigration.copyCredentialIntoPool` - ← `AccountMigration.migrateAccount(tool:…)` (the v3.0.4→pool credential path). +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 tests `service(for:)` (pure string derivation) — no I/O, -does not pollute. Read paths (`password(forService:)`, `keychainItemExists`) don't -pollute (a miss is harmless), so they stay direct — only writes are seamed (YAGNI). +`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`. -## Design +## Fix (test-only) -Extend the existing `KeychainAccess` seam (added in PR #22 — currently `itemExists` -+ `copyItem`) with the second write primitive: +`Tests/OrreryTests/TestHelpers.swift`: add ```swift -public var storePassword: @Sendable (_ password: String, _ accountID: String) -> Bool +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 +} ``` -`.live.storePassword = ClaudeKeychain.storePassword(_:forOrreryAccount:)`. +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. -Thread a `keychain: KeychainAccess = .live` parameter through the two write paths and -their callers; production omits it (`.live`), tests pass a fake: +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). -| Production symbol | change | -|---|---| -| `AccountLoginFlow.importFrom(stagingDir:into:)` | add `keychain: KeychainAccess = .live`; macOS-claude branch uses `keychain.copyItem` instead of `ClaudeKeychain.copyKeychainItem` | -| `AccountMigration.migrateAccount(tool:…)` + `copyCredentialIntoPool` | add `keychain: KeychainAccess = .live`; use `keychain.storePassword` instead of `ClaudeKeychain.storePassword` | -| `AccountAddFinalizeCommand` | thread `.live` through to `importFrom` (+ `migrateAccount` if it calls the tool-level one); a test can inject via an internal seam if needed | -| `OriginAccountSeeder` | already holds a `KeychainAccess` — pass it to `importFrom` (was calling the no-arg form) | +## Verification -## Tests to convert (inject a fake, assert no real-Keychain touch) +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). -- `AccountLoginFlowTests` macOS-claude test → inject a recording fake for `copyItem`. -- `AccountCommandsTests` `AccountAddFinalize` "v3.1 layout" test → inject a fake so - `importFrom` (and any `migrateAccount`) never write the real Keychain. -- Any test calling `AccountMigration.migrateAccount(tool:…)` → inject a fake. -- New guard test: run the seeder/import/finalize with a fake and assert the fake's - recorded calls (not the real Keychain). +## Residual (open) -**Isolation regression check:** the manual step from PR #22 (count -`Claude Code-orrery-*` before/after a full `swift test`) must show **no growth**. -Codex/gemini paths are already file-based and unaffected. +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 -- Read-path seaming (harmless; YAGNI). +- Any production change (rejected — test-only concern). +- Read-path handling (a keychain read miss is harmless). - `ClaudeKeychainTests` (no I/O). -- Any production behavior change (`.live` everywhere in prod). - -## Open question -- Whether `AccountAddFinalizeCommand` needs an injectable entry point for its test, - or whether the test can drive `AccountLoginFlow.importFrom(…, keychain:)` + - `migrateAccount(…, keychain:)` directly. Prefer the latter (no command-level - plumbing) if the finalize test can be reframed around the two seam-aware calls.