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) {