feat: GitHub provider lifecycle and onboarding hardening#351
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves GitHub Models onboarding and provider lifecycle handling by integrating GitHub as a “virtual provider” alongside saved provider profiles, tightening model-option behavior while GitHub is active, and adding regression tests around provider/profile env syncing.
Changes:
- Add GitHub Models lifecycle + onboarding safeguards (device-flow scope fallback, skip onboarding when already logged in unless forced).
- Improve provider-profile env management (track applied profile ID, safer auto-heal behavior, persist active profile model on
/modelchanges). - Enforce GitHub-specific model options/strings behavior and add targeted regression tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/providerProfiles.ts | Adds profile-applied ID tracking, safer re-apply logic, and persists active profile model updates. |
| src/utils/providerProfiles.test.ts | Adds coverage for env drift auto-heal and persisted model syncing behavior. |
| src/utils/providerProfile.ts | Treats profile-managed env as an explicit provider selection for startup env behavior. |
| src/utils/providerProfile.test.ts | Tests that profile-managed env is preserved during startup env build. |
| src/utils/model/modelStrings.ts | Treats github like openai for builtin model string mapping. |
| src/utils/model/modelStrings.github.test.ts | New tests ensuring GitHub model strings are concrete and parseable. |
| src/utils/model/modelOptions.ts | Restricts model options when GitHub provider is active. |
| src/utils/model/modelOptions.github.test.ts | New tests verifying GitHub-only model options behavior. |
| src/utils/model/model.ts | Ensures GitHub provider reads/uses OPENAI_MODEL for user/default model resolution. |
| src/state/onChangeAppState.ts | Syncs /model changes back into the active provider profile when profile-managed. |
| src/services/github/deviceFlow.ts | Changes default scope to OAuth-safe and retries with fallback scope on invalid_scope. |
| src/services/github/deviceFlow.test.ts | Adds tests for the OAuth-safe default scope and retry behavior. |
| src/components/ProviderManager.tsx | Adds GitHub virtual provider activation/deletion flows and updates provider selection UI. |
| src/commands/provider/provider.tsx | Extends provider summary to recognize GitHub Models mode. |
| src/commands/provider/provider.test.tsx | Adds regression test for GitHub Models provider summary. |
| src/commands/onboard-github/onboard-github.tsx | Adds “already logged in” guard + --force relogin handling; minor copy tweaks. |
| src/commands/onboard-github/onboard-github.test.ts | New tests for relogin arg parsing and token-existence detection. |
| src/commands/onboard-github/index.ts | Adds aliases for the onboard-github command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
please address copilots comment |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/commands/onboard-github/onboard-github.tsx:64
- Onboarding enables GitHub mode but does not clear OPENAI_API_KEY from user settings or process.env. If an OpenAI key is present, OpenAI shim GitHub normalization won’t override it (OPENAI_API_KEY ??= …), leading to GitHub requests using the wrong credential. Clear OPENAI_API_KEY when setting CLAUDE_CODE_USE_GITHUB=1 so hydrated GITHUB_TOKEN is actually used.
function mergeUserSettingsEnv(model: string): { ok: boolean; detail?: string } {
const { error } = updateSettingsForSource('userSettings', {
env: {
CLAUDE_CODE_USE_GITHUB: '1',
OPENAI_MODEL: model,
CLAUDE_CODE_USE_OPENAI: undefined as any,
CLAUDE_CODE_USE_GEMINI: undefined as any,
CLAUDE_CODE_USE_BEDROCK: undefined as any,
CLAUDE_CODE_USE_VERTEX: undefined as any,
CLAUDE_CODE_USE_FOUNDRY: undefined as any,
},
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kevincodex1 Done. Please review and let me know for any changes |
|
fix conflicts |
ce203ee to
e907593
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Vasanthdev2004 Fixed conflicts. |
gnanam1990
left a comment
There was a problem hiding this comment.
This improves a lot of the GitHub provider lifecycle, but I don't think the onboarding flow is fully hardened yet.
The main gap I still see is onboarding when the user already has OpenAI credentials configured. The finalize path appears able to leave existing OpenAI auth in place, and the shim only hydrates the GitHub token with ??=, so requests can still go out with the wrong key.
Please fix the auth precedence/cleanup in the onboarding finalize path and add a focused test for onboarding with preexisting OpenAI credentials.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const call: LocalJSXCommandCall = async (onDone, context, args) => { | ||
| const forceRelogin = shouldForceGithubRelogin(args) | ||
| if (hasExistingGithubModelsLoginToken() && !forceRelogin) { | ||
| onDone( | ||
| 'Already logged in to GitHub Models. Use /onboard-github --force to re-authenticate.', | ||
| { display: 'system' }, | ||
| ) | ||
| return null | ||
| } |
There was a problem hiding this comment.
The early return in call short-circuits onboarding whenever any GitHub token exists (env or secure storage). This skips the side effects that /onboard-github currently performs (updating user settings + applying CLAUDE_CODE_USE_GITHUB / OPENAI_MODEL in-process), so users with an existing token can’t use the command to (re)activate GitHub Models mode without --force. Suggest: when a token already exists and --force is not set, still apply the settings/env activation steps and exit with a success message, rather than returning before the component runs.
|
|
||
| export function shouldForceGithubRelogin(args?: string): boolean { | ||
| const normalized = (args ?? '').trim().toLowerCase() | ||
| return FORCE_RELOGIN_ARGS.has(normalized) |
There was a problem hiding this comment.
shouldForceGithubRelogin only returns true when the entire args string exactly matches a single token (e.g. "--force"). Command args are the full string after the command name and may contain multiple words, so /onboard-github --force something would not be treated as force re-login. Consider tokenizing args (whitespace split) and checking whether any arg is in FORCE_RELOGIN_ARGS.
| return FORCE_RELOGIN_ARGS.has(normalized) | |
| if (!normalized) { | |
| return false | |
| } | |
| return normalized.split(/\s+/).some(arg => FORCE_RELOGIN_ARGS.has(arg)) |
| const hydratedTokenInSession = process.env.GITHUB_TOKEN?.trim() | ||
| if ( | ||
| process.env[GITHUB_MODELS_HYDRATED_ENV_MARKER] === '1' && | ||
| hydratedTokenInSession && | ||
| (!storedTokenBeforeClear || hydratedTokenInSession === storedTokenBeforeClear) | ||
| ) { | ||
| delete process.env.GITHUB_TOKEN | ||
| } | ||
|
|
||
| delete process.env.CLAUDE_CODE_USE_GITHUB | ||
| delete process.env[GITHUB_MODELS_HYDRATED_ENV_MARKER] | ||
| delete process.env.OPENAI_MODEL | ||
| delete process.env.OPENAI_BASE_URL | ||
| delete process.env.OPENAI_API_BASE | ||
|
|
||
| // Restore active provider profile immediately when one exists. | ||
| applyActiveProviderProfileFromConfig() | ||
|
|
There was a problem hiding this comment.
deleteGithubProvider() clears GITHUB_TOKEN (when hydrated) and some OpenAI env keys, but it doesn’t clear OPENAI_API_KEY. Since createOpenAIShimClient sets OPENAI_API_KEY from the GitHub token in GitHub mode, deleting GitHub can leave OPENAI_API_KEY pointing at the GitHub token when there is no active provider profile to immediately re-apply. Consider clearing OPENAI_API_KEY (and related OpenAI auth env like org/project if present) as part of GitHub provider deletion when GitHub mode is being turned off.
9ad57bb to
a318975
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function hydrateGithubModelsTokenFromSecureStorage(): void { | ||
| if (!isEnvTruthy(process.env.CLAUDE_CODE_USE_GITHUB)) { | ||
| delete process.env[GITHUB_MODELS_HYDRATED_ENV_MARKER] | ||
| return | ||
| } | ||
| if (process.env.GITHUB_TOKEN?.trim() || process.env.GH_TOKEN?.trim()) { | ||
| if (process.env.GH_TOKEN?.trim()) { | ||
| delete process.env[GITHUB_MODELS_HYDRATED_ENV_MARKER] | ||
| return | ||
| } | ||
| if (process.env.GITHUB_TOKEN?.trim()) { | ||
| delete process.env[GITHUB_MODELS_HYDRATED_ENV_MARKER] | ||
| return |
There was a problem hiding this comment.
hydrateGithubModelsTokenFromSecureStorage() always deletes CLAUDE_CODE_GITHUB_TOKEN_HYDRATED when GITHUB_TOKEN is already set. Since createOpenAIShimClient() calls this hydrator on every client creation, a token that was hydrated from secure storage will typically lose its marker on the next call, making it indistinguishable from a user-provided token. This breaks later cleanup logic that relies on the marker (e.g., ProviderManager’s GitHub delete flow) and can leave hydrated tokens in process.env after deleting the GitHub provider. Consider preserving the marker when it is already set (or re-deriving it by comparing against secure storage) so it remains stable for the session, or adjust the delete logic to not depend on a marker that can be cleared by subsequent hydration calls.
| // Keep active provider profiles in sync with /model choices so restarts | ||
| // keep using the last selected model instead of the profile's old default. | ||
| if (process.env.CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED === '1') { | ||
| persistActiveProviderProfileModel(newState.mainLoopModel) | ||
| } |
There was a problem hiding this comment.
The profile-model sync on /model changes only checks CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED === '1' before persisting into the active provider profile. Since this PR introduced CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED_ID to disambiguate which profile applied the env, it’s possible to persist the model into the wrong profile if the applied flag is stale or the active profile changed. Consider also validating CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED_ID against the current active profile (or moving that validation into persistActiveProviderProfileModel() before mutating config).
| process.env[GITHUB_MODELS_HYDRATED_ENV_MARKER] === '1' && | ||
| hydratedTokenInSession && | ||
| (!storedTokenBeforeClear || hydratedTokenInSession === storedTokenBeforeClear) |
There was a problem hiding this comment.
deleteGithubProvider() only clears process.env.GITHUB_TOKEN when CLAUDE_CODE_GITHUB_TOKEN_HYDRATED === '1'. With the current hydrator behavior, this marker is often cleared on subsequent hydrator calls once GITHUB_TOKEN exists, so GitHub provider deletion can leave the hydrated token in the running process env. Consider basing this cleanup on a more stable signal (e.g., comparing the current env token to the stored token captured before clearing, and/or keeping the hydrated marker stable for the session).
| process.env[GITHUB_MODELS_HYDRATED_ENV_MARKER] === '1' && | |
| hydratedTokenInSession && | |
| (!storedTokenBeforeClear || hydratedTokenInSession === storedTokenBeforeClear) | |
| storedTokenBeforeClear && | |
| hydratedTokenInSession && | |
| hydratedTokenInSession === storedTokenBeforeClear |
|
I have been involved in the latest fixes on this branch, so I should not be the final approver here. The remaining step should be an independent review from another maintainer/collaborator before merge. |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the latest head 446e112bc4c6476960ea831ac4c472690520eecb against current origin/main.
The earlier auth-precedence blocker is fixed on this revision, and I did not find a remaining branch-specific correctness blocker.
What I verified on this head:
- onboarding activation now clears preexisting OpenAI auth state before enabling GitHub mode
- direct helper repro via
buildGithubOnboardingSettingsEnv()/applyGithubOnboardingProcessEnv()clears:OPENAI_API_KEYOPENAI_ORGOPENAI_PROJECTOPENAI_ORGANIZATIONOPENAI_BASE_URLOPENAI_API_BASE
- and sets:
CLAUDE_CODE_USE_GITHUB=1OPENAI_MODEL=github:copilot
- direct helper repro via
- existing-token onboarding path now activates GitHub mode instead of just bailing out
- force relogin parsing now handles multi-word args (
foo --force bar,abc reauth xyz, etc.) - focused lifecycle coverage passes:
bun test src/commands/onboard-github/onboard-github.test.ts src/commands/provider/provider.test.tsx src/utils/providerProfile.test.ts src/utils/providerProfiles.test.ts src/utils/model/modelOptions.github.test.ts src/utils/model/modelStrings.github.test.ts src/services/github/deviceFlow.test.ts src/utils/githubModelsCredentials.hydrate.test.ts-> 85 pass
bun run build-> successbun run smoke-> success
Residual risk is mostly ergonomics rather than a demonstrated bug: the GitHub virtual provider still relies on some synchronous secure-storage reads for availability/summary state in ProviderManager, and there still is no full component-level test of the virtual provider UI flow itself. But the current head does address the GitHub lifecycle/auth issues that were blocking the earlier review.
kevincodex1
left a comment
There was a problem hiding this comment.
Alright this is good
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
* feat: improve GitHub provider onboarding and lifecycle * fix: address copilot review in provider manager * fix: address follow-up copilot review comments * test: resolve rebase conflict in provider profiles suite * fix: clear stale github hydrated marker * fix: harden github onboarding auth precedence * fix: remove merge markers from provider tests * fix: resolve latest copilot onboarding comments --------- Co-authored-by: KRATOS <84986124+gnanam1990@users.noreply.github.com>
Summary
Impact
Testing
Notes