From ff7d49990de515825ddbe4099f3a39b944b61370 Mon Sep 17 00:00:00 2001 From: Agent_J <107950076+Varshith-JV-1410@users.noreply.github.com> Date: Mon, 6 Apr 2026 16:48:58 +0530 Subject: [PATCH] feat: GitHub provider lifecycle and onboarding hardening (#351) * 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> --- src/commands/onboard-github/index.ts | 1 + .../onboard-github/onboard-github.test.ts | 148 +++++++++ .../onboard-github/onboard-github.tsx | 149 +++++++-- src/commands/provider/provider.test.tsx | 15 + src/commands/provider/provider.tsx | 17 + src/components/ProviderManager.tsx | 299 ++++++++++++++++-- src/services/github/deviceFlow.test.ts | 76 +++++ src/services/github/deviceFlow.ts | 91 ++++-- src/state/onChangeAppState.ts | 7 + .../githubModelsCredentials.hydrate.test.ts | 5 + src/utils/githubModelsCredentials.ts | 14 +- src/utils/model/model.ts | 8 +- src/utils/model/modelOptions.github.test.ts | 46 +++ src/utils/model/modelOptions.ts | 16 + src/utils/model/modelStrings.github.test.ts | 54 ++++ src/utils/model/modelStrings.ts | 2 +- src/utils/providerProfile.test.ts | 20 ++ src/utils/providerProfile.ts | 5 + src/utils/providerProfiles.test.ts | 246 +++++++++----- src/utils/providerProfiles.ts | 97 +++++- 20 files changed, 1150 insertions(+), 166 deletions(-) create mode 100644 src/commands/onboard-github/onboard-github.test.ts create mode 100644 src/utils/model/modelOptions.github.test.ts create mode 100644 src/utils/model/modelStrings.github.test.ts diff --git a/src/commands/onboard-github/index.ts b/src/commands/onboard-github/index.ts index 91d67247..f8bc66aa 100644 --- a/src/commands/onboard-github/index.ts +++ b/src/commands/onboard-github/index.ts @@ -2,6 +2,7 @@ import type { Command } from '../../commands.js' const onboardGithub: Command = { name: 'onboard-github', + aliases: ['onboarding-github', 'onboardgithub', 'onboardinggithub'], description: 'Interactive setup for GitHub Models: device login or PAT, saved to secure storage', type: 'local-jsx', diff --git a/src/commands/onboard-github/onboard-github.test.ts b/src/commands/onboard-github/onboard-github.test.ts new file mode 100644 index 00000000..febd906f --- /dev/null +++ b/src/commands/onboard-github/onboard-github.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, test } from 'bun:test' + +import { + activateGithubOnboardingMode, + applyGithubOnboardingProcessEnv, + buildGithubOnboardingSettingsEnv, + hasExistingGithubModelsLoginToken, + shouldForceGithubRelogin, +} from './onboard-github.js' + +describe('shouldForceGithubRelogin', () => { + test.each(['force', '--force', 'relogin', '--relogin', 'reauth', '--reauth'])( + 'treats %s as force re-login', + arg => { + expect(shouldForceGithubRelogin(arg)).toBe(true) + }, + ) + + test('returns false for empty or unknown args', () => { + expect(shouldForceGithubRelogin('')).toBe(false) + expect(shouldForceGithubRelogin(undefined)).toBe(false) + expect(shouldForceGithubRelogin('something-else')).toBe(false) + }) + + test('treats force flags as present in multi-word args', () => { + expect(shouldForceGithubRelogin('--force extra')).toBe(true) + expect(shouldForceGithubRelogin('foo --relogin bar')).toBe(true) + expect(shouldForceGithubRelogin('abc reauth xyz')).toBe(true) + }) +}) + +describe('hasExistingGithubModelsLoginToken', () => { + test('returns true when GITHUB_TOKEN is present', () => { + expect( + hasExistingGithubModelsLoginToken({ GITHUB_TOKEN: 'token' }, ''), + ).toBe(true) + }) + + test('returns true when GH_TOKEN is present', () => { + expect( + hasExistingGithubModelsLoginToken({ GH_TOKEN: 'token' }, ''), + ).toBe(true) + }) + + test('returns true when stored token exists', () => { + expect(hasExistingGithubModelsLoginToken({}, 'stored-token')).toBe(true) + }) + + test('returns false when both env and stored token are missing', () => { + expect(hasExistingGithubModelsLoginToken({}, '')).toBe(false) + }) +}) + +describe('onboarding auth precedence cleanup', () => { + test('clears preexisting OpenAI auth when switching to GitHub', () => { + const env: NodeJS.ProcessEnv = { + CLAUDE_CODE_USE_OPENAI: '1', + OPENAI_MODEL: 'gpt-4o', + OPENAI_API_KEY: 'sk-stale-openai-key', + OPENAI_ORG: 'org-old', + OPENAI_PROJECT: 'project-old', + OPENAI_ORGANIZATION: 'org-legacy', + OPENAI_BASE_URL: 'https://api.openai.com/v1', + OPENAI_API_BASE: 'https://api.openai.com/v1', + CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED: '1', + CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED_ID: 'profile_old', + } + + applyGithubOnboardingProcessEnv('github:copilot', env) + + expect(env.CLAUDE_CODE_USE_GITHUB).toBe('1') + expect(env.OPENAI_MODEL).toBe('github:copilot') + + expect(env.OPENAI_API_KEY).toBeUndefined() + expect(env.OPENAI_ORG).toBeUndefined() + expect(env.OPENAI_PROJECT).toBeUndefined() + expect(env.OPENAI_ORGANIZATION).toBeUndefined() + expect(env.OPENAI_BASE_URL).toBeUndefined() + expect(env.OPENAI_API_BASE).toBeUndefined() + + expect(env.CLAUDE_CODE_USE_OPENAI).toBeUndefined() + expect(env.CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED).toBeUndefined() + expect(env.CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED_ID).toBeUndefined() + + const settingsEnv = buildGithubOnboardingSettingsEnv('github:copilot') + expect(settingsEnv.CLAUDE_CODE_USE_GITHUB).toBe('1') + expect(settingsEnv.OPENAI_MODEL).toBe('github:copilot') + expect(settingsEnv.OPENAI_API_KEY).toBeUndefined() + expect(settingsEnv.OPENAI_ORG).toBeUndefined() + expect(settingsEnv.OPENAI_PROJECT).toBeUndefined() + expect(settingsEnv.OPENAI_ORGANIZATION).toBeUndefined() + }) +}) + +describe('activateGithubOnboardingMode', () => { + test('activates settings/env/hydration in order when merge succeeds', () => { + const calls: string[] = [] + + const result = activateGithubOnboardingMode(' github:copilot ', { + mergeSettingsEnv: model => { + calls.push(`merge:${model}`) + return { ok: true } + }, + applyProcessEnv: model => { + calls.push(`apply:${model}`) + }, + hydrateToken: () => { + calls.push('hydrate') + }, + onChangeAPIKey: () => { + calls.push('onChangeAPIKey') + }, + }) + + expect(result).toEqual({ ok: true }) + expect(calls).toEqual([ + 'merge:github:copilot', + 'apply:github:copilot', + 'hydrate', + 'onChangeAPIKey', + ]) + }) + + test('stops activation when settings merge fails', () => { + const calls: string[] = [] + + const result = activateGithubOnboardingMode(DEFAULT_MODEL_FOR_TESTS, { + mergeSettingsEnv: () => { + calls.push('merge') + return { ok: false, detail: 'settings write failed' } + }, + applyProcessEnv: () => { + calls.push('apply') + }, + hydrateToken: () => { + calls.push('hydrate') + }, + onChangeAPIKey: () => { + calls.push('onChangeAPIKey') + }, + }) + + expect(result).toEqual({ ok: false, detail: 'settings write failed' }) + expect(calls).toEqual(['merge']) + }) +}) + +const DEFAULT_MODEL_FOR_TESTS = 'github:copilot' diff --git a/src/commands/onboard-github/onboard-github.tsx b/src/commands/onboard-github/onboard-github.tsx index 66326957..1b968895 100644 --- a/src/commands/onboard-github/onboard-github.tsx +++ b/src/commands/onboard-github/onboard-github.tsx @@ -12,11 +12,20 @@ import { import type { LocalJSXCommandCall } from '../../types/command.js' import { hydrateGithubModelsTokenFromSecureStorage, + readGithubModelsToken, saveGithubModelsToken, } from '../../utils/githubModelsCredentials.js' import { updateSettingsForSource } from '../../utils/settings/settings.js' const DEFAULT_MODEL = 'github:copilot' +const FORCE_RELOGIN_ARGS = new Set([ + 'force', + '--force', + 'relogin', + '--relogin', + 'reauth', + '--reauth', +]) type Step = | 'menu' @@ -24,17 +33,72 @@ type Step = | 'pat' | 'error' +export function shouldForceGithubRelogin(args?: string): boolean { + const normalized = (args ?? '').trim().toLowerCase() + if (!normalized) { + return false + } + return normalized.split(/\s+/).some(arg => FORCE_RELOGIN_ARGS.has(arg)) +} + +export function hasExistingGithubModelsLoginToken( + env: NodeJS.ProcessEnv = process.env, + storedToken?: string, +): boolean { + const envToken = env.GITHUB_TOKEN?.trim() || env.GH_TOKEN?.trim() + if (envToken) { + return true + } + const persisted = (storedToken ?? readGithubModelsToken())?.trim() + return Boolean(persisted) +} + +export function buildGithubOnboardingSettingsEnv( + model: string, +): Record { + return { + CLAUDE_CODE_USE_GITHUB: '1', + OPENAI_MODEL: model, + OPENAI_API_KEY: undefined, + OPENAI_ORG: undefined, + OPENAI_PROJECT: undefined, + OPENAI_ORGANIZATION: undefined, + OPENAI_BASE_URL: undefined, + OPENAI_API_BASE: undefined, + CLAUDE_CODE_USE_OPENAI: undefined, + CLAUDE_CODE_USE_GEMINI: undefined, + CLAUDE_CODE_USE_BEDROCK: undefined, + CLAUDE_CODE_USE_VERTEX: undefined, + CLAUDE_CODE_USE_FOUNDRY: undefined, + } +} + +export function applyGithubOnboardingProcessEnv( + model: string, + env: NodeJS.ProcessEnv = process.env, +): void { + env.CLAUDE_CODE_USE_GITHUB = '1' + env.OPENAI_MODEL = model + + delete env.OPENAI_API_KEY + delete env.OPENAI_ORG + delete env.OPENAI_PROJECT + delete env.OPENAI_ORGANIZATION + delete env.OPENAI_BASE_URL + delete env.OPENAI_API_BASE + + delete env.CLAUDE_CODE_USE_OPENAI + delete env.CLAUDE_CODE_USE_GEMINI + delete env.CLAUDE_CODE_USE_BEDROCK + delete env.CLAUDE_CODE_USE_VERTEX + delete env.CLAUDE_CODE_USE_FOUNDRY + delete env.CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED + delete env.CLAUDE_CODE_PROVIDER_PROFILE_ENV_APPLIED_ID +} + 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, - }, + env: buildGithubOnboardingSettingsEnv(model) as any, }) if (error) { return { ok: false, detail: error.message } @@ -42,6 +106,32 @@ function mergeUserSettingsEnv(model: string): { ok: boolean; detail?: string } { return { ok: true } } +export function activateGithubOnboardingMode( + model: string = DEFAULT_MODEL, + options?: { + mergeSettingsEnv?: (model: string) => { ok: boolean; detail?: string } + applyProcessEnv?: (model: string) => void + hydrateToken?: () => void + onChangeAPIKey?: () => void + }, +): { ok: boolean; detail?: string } { + const normalizedModel = model.trim() || DEFAULT_MODEL + const mergeSettingsEnv = options?.mergeSettingsEnv ?? mergeUserSettingsEnv + const applyProcessEnv = options?.applyProcessEnv ?? applyGithubOnboardingProcessEnv + const hydrateToken = + options?.hydrateToken ?? hydrateGithubModelsTokenFromSecureStorage + + const merged = mergeSettingsEnv(normalizedModel) + if (!merged.ok) { + return merged + } + + applyProcessEnv(normalizedModel) + hydrateToken() + options?.onChangeAPIKey?.() + return { ok: true } +} + function OnboardGithub(props: { onDone: Parameters[0] onChangeAPIKey: () => void @@ -64,19 +154,17 @@ function OnboardGithub(props: { setStep('error') return } - const merged = mergeUserSettingsEnv(model.trim() || DEFAULT_MODEL) - if (!merged.ok) { + const activated = activateGithubOnboardingMode(model, { + onChangeAPIKey, + }) + if (!activated.ok) { setErrorMsg( - `Token saved, but settings were not updated: ${merged.detail ?? 'unknown error'}. ` + + `Token saved, but settings were not updated: ${activated.detail ?? 'unknown error'}. ` + `Add env CLAUDE_CODE_USE_GITHUB=1 and OPENAI_MODEL to ~/.claude/settings.json manually.`, ) setStep('error') return } - process.env.CLAUDE_CODE_USE_GITHUB = '1' - process.env.OPENAI_MODEL = model.trim() || DEFAULT_MODEL - hydrateGithubModelsTokenFromSecureStorage() - onChangeAPIKey() onDone( 'GitHub Models onboard complete. Token stored in secure storage; user settings updated. Restart if the model does not switch.', { display: 'user' }, @@ -147,11 +235,11 @@ function OnboardGithub(props: { {deviceHint.verification_uri} - A browser window may have opened. Waiting for authorization… + A browser window may have opened. Waiting for authorization... ) : ( - Requesting device code from GitHub… + Requesting device code from GitHub... )} @@ -206,7 +294,7 @@ function OnboardGithub(props: { GitHub Models setup Stores your token in the OS credential store (macOS Keychain when available) - and enables CLAUDE_CODE_USE_GITHUB in your user settings — no export + and enables CLAUDE_CODE_USE_GITHUB in your user settings - no export GITHUB_TOKEN needed for future runs. 0) { + if (hasSelectableProviders) { setScreen('select-active') } break @@ -484,7 +679,7 @@ export function ProviderManager({ mode, onDone }: Props): React.ReactNode { } break case 'delete': - if (profiles.length > 0) { + if (hasSelectableProviders) { setScreen('select-delete') } break @@ -504,8 +699,29 @@ export function ProviderManager({ mode, onDone }: Props): React.ReactNode { title: string, emptyMessage: string, onSelect: (profileId: string) => void, + options?: { includeGithub?: boolean }, ): React.ReactNode { - if (profiles.length === 0) { + const includeGithub = options?.includeGithub ?? false + const selectOptions = profiles.map(profile => ({ + value: profile.id, + label: + profile.id === activeProfileId + ? `${profile.name} (active)` + : profile.name, + description: `${profile.provider === 'anthropic' ? 'anthropic' : 'openai-compatible'} · ${profile.baseUrl} · ${profile.model}`, + })) + + if (includeGithub && githubProviderAvailable) { + selectOptions.push({ + value: GITHUB_PROVIDER_ID, + label: isGithubActive + ? `${GITHUB_PROVIDER_LABEL} (active)` + : GITHUB_PROVIDER_LABEL, + description: `github-models · ${GITHUB_PROVIDER_DEFAULT_BASE_URL} · ${getGithubProviderModel()}`, + }) + } + + if (selectOptions.length === 0) { return ( @@ -528,25 +744,16 @@ export function ProviderManager({ mode, onDone }: Props): React.ReactNode { ) } - const options = profiles.map(profile => ({ - value: profile.id, - label: - profile.id === activeProfileId - ? `${profile.name} (active)` - : profile.name, - description: `${profile.provider === 'anthropic' ? 'anthropic' : 'openai-compatible'} · ${profile.baseUrl} · ${profile.model}`, - })) - return ( {title}