From 549611d48297e4c6443b6cf704469d9c94298071 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Tue, 31 Mar 2026 23:27:28 -0500 Subject: [PATCH] fix: stop recurring Keychain password prompts on server recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TryRecoverPersistentServerAsync was clearing _resolvedGitHubToken = null on every recovery cycle (watchdog timeout, wake/sleep, auth polling). This caused CheckAuthStatusAsync to re-enter the lazy Keychain resolution path, spawning `security find-generic-password` which triggers a macOS password dialog each time. Re-reading the Keychain is pointless after token expiration — the stored token is static and only changes when `copilot login` is run. The same expired token comes back, triggering another dialog on the next cycle. Fix: preserve the cached token through recovery. Only ReauthenticateAsync (explicit user action) and ReconnectAsync (settings change) clear it. When the forwarded token expires, the auth banner appears and the user can click Re-authenticate for a fresh Keychain read. Updates INV-A3 in auth-token-safety skill to reflect the new invariant. Tests: 3064/3064 pass ✅ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .claude/skills/auth-token-safety/SKILL.md | 22 +++++++++++++--------- PolyPilot/Services/CopilotService.cs | 15 ++++++++------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/.claude/skills/auth-token-safety/SKILL.md b/.claude/skills/auth-token-safety/SKILL.md index dd284d3f55..4732b64073 100644 --- a/.claude/skills/auth-token-safety/SKILL.md +++ b/.claude/skills/auth-token-safety/SKILL.md @@ -74,17 +74,21 @@ Do NOT re-read from Keychain on every recovery cycle. **Why:** Each Keychain read = another password dialog. The polling loop runs every 10s. If it re-reads Keychain, users get prompted every 10 seconds. -### INV-A3: Token expiration causes cascading failures +### INV-A3: Never clear `_resolvedGitHubToken` on automatic recovery -A static `COPILOT_GITHUB_TOKEN` will expire. When it does: -1. All sessions fail simultaneously (server-wide auth loss) -2. Watchdog fires after `WatchdogServerRecoveryThreshold` (2) consecutive timeouts -3. `TryRecoverPersistentServerAsync` restarts server with same stale token -4. New server also fails → more timeouts → recovery loop +`TryRecoverPersistentServerAsync` must NOT clear `_resolvedGitHubToken`. Re-reading the +Keychain returns the same (possibly expired) token — the Keychain is a static store that +only changes when `copilot login` is run. Clearing the cache causes the lazy path in +`CheckAuthStatusAsync` to re-read Keychain, triggering another macOS password dialog. -**The fix:** On recovery, try starting the server WITHOUT a token first (clear -`_resolvedGitHubToken = null`). Let the server attempt its own Keychain access via -keytar.node. Only if that also fails, fall back to the cached token or show the banner. +- ❌ `_resolvedGitHubToken = null` in `TryRecoverPersistentServerAsync` +- ✅ Preserve the cached token through recovery — forward it to the new server +- ✅ Only `ReconnectAsync` (settings change) and `ReauthenticateAsync` (user action) clear it + +**Token expiration scenario:** When the forwarded token expires, the server reports +unauthenticated. `CheckAuthStatusAsync` sees `_resolvedGitHubToken != null` → skips +lazy path → shows the auth banner. User runs `copilot login` + clicks Re-authenticate +→ `ReauthenticateAsync` does a fresh Keychain read (correct — explicit user action). ### INV-A4: All AuthNotice writes must be inside InvokeOnUI diff --git a/PolyPilot/Services/CopilotService.cs b/PolyPilot/Services/CopilotService.cs index a5cce0c083..c112e38ff7 100644 --- a/PolyPilot/Services/CopilotService.cs +++ b/PolyPilot/Services/CopilotService.cs @@ -1331,16 +1331,17 @@ internal async Task TryRecoverPersistentServerAsync() await Task.Delay(250); } - // Save whatever token was resolved (may be null for watchdog callers, or a + // Forward whatever token was resolved (may be null for watchdog callers, or a // freshly-resolved token from ReauthenticateAsync/CheckAuthStatusAsync). - // Then clear the field so future auth failures trigger lazy Keychain resolution. - // See .claude/skills/auth-token-safety/SKILL.md (INV-A3). + // Do NOT clear _resolvedGitHubToken — re-reading Keychain produces the same + // (possibly expired) token, so clearing the cache only triggers redundant macOS + // password prompts on the next CheckAuthStatusAsync lazy-resolution cycle. + // Only ReauthenticateAsync (explicit user action) and ReconnectAsync (settings + // change) should clear the cache. See .claude/skills/auth-token-safety/SKILL.md. var tokenToForward = _resolvedGitHubToken; - _resolvedGitHubToken = null; - // Start a fresh server — forwards the saved token (if any) or null to let - // the server try native Keychain auth. If native auth fails and tokenToForward - // was null, CheckAuthStatusAsync will lazily resolve a fresh token. + // Start a fresh server — forwards the cached token (if any) or null to let + // the server try native Keychain auth. var started = await _serverManager.StartServerAsync(settings.Port, tokenToForward); if (!started) {