fix(copilot): defer keychain auth lookup - #2739
Conversation
PR governanceThis PR does not yet satisfy the required template fields:
Please update the PR body, or move the PR back to draft while it is still in progress. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
61fdc20 to
18b7de9
Compare
|
@rNoz is attempting to deploy a commit to the Headroom Labs Team on Vercel. A member of the Team first needs to authorize it. |
JerrettDavis
left a comment
There was a problem hiding this comment.
Reviewed the latest merged head adversarially. The two-pass discovery preserves existing candidate precedence, defers only prompt-capable macOS/Linux stores until non-platform credentials fail, avoids duplicate token exchange, and retains off-platform fallbacks. After merging current main, 158 focused auth/platform/wrapper tests and targeted Ruff checks pass locally. The prior dependency-audit failure was from the older cryptography 48.0.1 lock; this head carries main's audited 50.0.0 lock. I found no correctness or security blocker.
JerrettDavis
left a comment
There was a problem hiding this comment.
Re-reviewed the latest main merge. No feature source follows the previously approved two-pass credential discovery: saved/non-platform credentials retain precedence, prompt-capable macOS/Linux stores remain deferred, and duplicate token exchange is avoided. The branch is clean and mergeable with the current audited dependency baseline. No blocking correctness or security finding.
ce281f7 to
bed0bc6
Compare
|
@chopratejas ping for merge when you have a moment. Approved by @JerrettDavis, rebased onto current main today, CI fully green, mergeable: clean. One data point consistent with this PR's premise: on a machine with multiple Copilot-capable keychain entries, a wrap launch without --subscription can end up bound to the copilot-language-server integrator, whose model allowlist 400s third-party models (kimi-*) that work fine once the correct token is resolved. Deferring the keychain lookup, as this PR does, avoids that path. #2352 and #2723 are also approved, green, and clean if you are doing a sweep. |
bed0bc6 to
899c2ef
Compare
Description
Defers macOS Keychain and Linux Secret Service OAuth discovery until non-platform Copilot credentials fail during
headroom wrap copilot --subscription. This prevents needless macOS Keychain prompts when saved Headroom OAuth authentication already resolves. Closes #2738.Type of Change
Changes Made
headroom/copilot_auth.py: split subscription credential resolution into a non-platform pass and a platform-secret-store fallback pass.tests/test_copilot_auth.py: cover successful saved-token resolution without Keychain access, Keychain fallback, and duplicate-token avoidance.tests/test_copilot_subscription_smoke.py: preserve the generic-token-to-Keychain fallback smoke test for the new two-pass candidate-discovery API.ec306470.Testing
Test Output
Real Behavior Proof
rnoz/fix-copilot-keychain-fallback, GitHub Copilot CLI 1.0.77, saved Headroom OAuth credential present.OK, launched againsthttp://127.0.0.1:8795/p/fix-copilot-keychain-fallback/v1, and no macOS Keychain authorization dialog appeared.Review Readiness
Checklist
CHANGELOG.mdScreenshots (if applicable)
Not applicable.
Additional Notes
The full
pytest -qrun was attempted after installing the missinglitellmtest dependency. It reached execution but failed in five unrelated existing tests: four Kompress readiness tests depended on an already-initialized PyTorch backend, and one Windows-only selector-loop test failed under Python 3.14 on macOS. The changed Copilot suites and the live wrapped-Copilot proof passed.