fix: set sentinel on auth success to prevent lazy Keychain reads

[PureWeen]

Problem

After PR #462 merged, users still get macOS "allow copilot-cli" password dialogs every ~1-2 hours. Process monitoring confirmed the parent PID is the PolyPilot .NET process itself (dotnet exec), not external copilot binaries.

Root Cause

PR #462 prevented TryRecoverPersistentServerAsync from clearing _resolvedGitHubToken. But the token was never set in the first place — on startup, ResolveGitHubTokenFromEnv() returns null (no env var), and when CheckAuthStatusAsync finds the server authenticated, it returns true without setting the field. So _resolvedGitHubToken stays null.

Later, any transient auth failure (connection blip, client recreation, server token rotation) calls CheckAuthStatusAsync again. This time auth fails, the guard _resolvedGitHubToken == null is satisfied, and the lazy path fires:

1. ResolveGitHubTokenForServer() → TryReadCopilotKeychainToken()
2. Tries 3 service names: copilot-cli, github-copilot, GitHub Copilot
3. Each spawns /usr/bin/security find-generic-password -s <name> -w (3s timeout)
4. 3 macOS password dialogs (PolyPilot not in the Keychain ACL for this entry)

Evidence

Process monitor output confirmed PolyPilot as the source:

security PID=66918, Parent PID=66646
  66646 66630 /usr/local/share/dotnet/dotnet exec --runtimeconfig ...
security PID=67068, Parent PID=66646  (3s later)
security PID=67146, Parent PID=66646  (3s later)

Fix

Set _resolvedGitHubToken ??= string.Empty when CheckAuthStatusAsync finds the server authenticated. The empty string sentinel means "server can self-authenticate, no Keychain read needed." The lazy path guard _resolvedGitHubToken == null is no longer satisfied.

ServerManager.StartServerAsync uses !string.IsNullOrEmpty(githubToken) so the empty sentinel is never forwarded as a COPILOT_GITHUB_TOKEN env var.

Behavior Matrix

Scenario	Before	After
Startup, server self-auths	_resolvedGitHubToken stays null	Set to "" (sentinel)
Later transient auth failure	Lazy path fires → 3 password dialogs	Lazy path skipped → auth banner shown
Server genuinely cant auth	Lazy path fires once (correct)	Same — "" not set because auth failed
ReauthenticateAsync (user)	Fresh Keychain read	Same — clears to null first
ReconnectAsync (settings)	Clears to null	Same

Tests

3064/3064 pass ✅

Related

• Fixes remaining issue after fix: stop recurring Keychain password prompts on server recovery #462

[PureWeen]

Multi-Model Code Review: PR #463 (R1)

PR: fix: set sentinel on auth success to prevent lazy Keychain reads
Models: claude-opus-4.6 ✅, claude-sonnet-4.6 ✅, gpt-5.3-codex ✅
CI: ⚠️ No checks reported on the branch
Prior reviews: None (0 comments, 0 reviews)

---

Core Fix Verification — ✅ Correct (3/3 models agree)

All three models independently verified:

• ??= correctly preserves existing real tokens (no-op when non-null) ✅
• "" sentinel is never forwarded as COPILOT_GITHUB_TOKEN — ServerManager.cs:97 uses !string.IsNullOrEmpty(githubToken) ✅
• ReconnectAsync (clears to null) and ReauthenticateAsync (overwrites with fresh token) are unaffected ✅
• All 7+ StartServerAsync call sites that pass _resolvedGitHubToken are safe with "" ✅
• Token expiration recovery path is correct: auth banner shown → user Re-authenticates → fresh Keychain read ✅

---

Consensus Findings

🟢 MINOR — Non-atomic ??= race window (3/3 models flagged)

PolyPilot/Services/CopilotService.Utilities.cs:872

_resolvedGitHubToken ??= string.Empty compiles to a non-atomic read-check-write. Theoretical interleave:

1. Thread A (CheckAuthStatusAsync) reads _resolvedGitHubToken == null ✓
2. Thread B (ReauthenticateAsync) sets _resolvedGitHubToken = "realToken"
3. Thread A writes string.Empty → clobbers the real token

All 3 models agree this is not a blocking issue because:

• Window is a few CPU cycles between read and write
• ReauthenticateAsync calls StopAuthPolling() first, so a poll must already be in-flight to race
• All other writes to _resolvedGitHubToken in the codebase are equally unprotected — this is a pre-existing pattern, not introduced by this PR
• Even if it fires, the server already demonstrated self-authentication (Thread A confirmed IsAuthenticated)

🟢 MINOR — No test coverage for sentinel behavior (3/3 models flagged)

PolyPilot.Tests/ServerRecoveryTests.cs

No test asserts that CheckAuthStatusAsync sets _resolvedGitHubToken to "" on auth success, or that ??= preserves existing tokens. The regression this PR fixes could silently reappear. StubServerManager.LastGitHubToken already exists and could pin this. Non-blocking for a targeted bugfix PR, but recommended as follow-up.

---

Summary

Check	Result
Regressions ("" at all StartServerAsync sites)	✅ Safe
??= semantics (preserves real tokens)	✅ Correct
ReauthenticateAsync / ReconnectAsync interaction	✅ Correct
Security	✅ No risk
Thread safety	🟢 Theoretical race, pre-existing pattern
Data loss / sessions	✅ None
Test coverage	🟢 No direct tests — acceptable for bugfix
SKILL.md documentation	✅ Accurate

Recommended action: ✅ Approve