Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions .claude/skills/auth-token-safety/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
15 changes: 8 additions & 7 deletions PolyPilot/Services/CopilotService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1331,16 +1331,17 @@ internal async Task<bool> 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)
{
Expand Down