From abc48445086a9b65357cb9341be06c1de4a85c0b Mon Sep 17 00:00:00 2001 From: Gabriel Malinosqui Date: Wed, 18 Feb 2026 15:44:45 -0300 Subject: [PATCH 1/4] fix: harden auth flow and standardize CLI language --- .../__tests__/auth.login-logout.test.ts | 156 ++++++++++++++++ src/commands/__tests__/auth.team-key.test.ts | 166 ++++++++++++++++++ src/commands/auth/index.ts | 3 +- src/commands/auth/login.ts | 9 +- src/commands/auth/logout.ts | 3 +- src/commands/auth/team-key.ts | 7 + src/services/__tests__/auth.service.test.ts | 44 ++++- src/services/api/__tests__/api.real.test.ts | 124 +++++++++++++ src/services/api/api.real.ts | 86 ++++++++- src/services/auth.service.ts | 72 +++++--- src/services/git.service.ts | 31 ++-- src/utils/__tests__/config.test.ts | 96 ++++++++++ src/utils/__tests__/credentials.test.ts | 106 +++++++++++ src/utils/config.ts | 24 ++- src/utils/credentials.ts | 25 ++- 15 files changed, 887 insertions(+), 65 deletions(-) create mode 100644 src/commands/__tests__/auth.login-logout.test.ts create mode 100644 src/commands/__tests__/auth.team-key.test.ts create mode 100644 src/services/api/__tests__/api.real.test.ts create mode 100644 src/utils/__tests__/config.test.ts create mode 100644 src/utils/__tests__/credentials.test.ts diff --git a/src/commands/__tests__/auth.login-logout.test.ts b/src/commands/__tests__/auth.login-logout.test.ts new file mode 100644 index 0000000..659f4af --- /dev/null +++ b/src/commands/__tests__/auth.login-logout.test.ts @@ -0,0 +1,156 @@ +import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'; + +const spinnerInstances: Array<{ start: ReturnType; succeed: ReturnType; fail: ReturnType; text: string }> = []; + +vi.mock('ora', () => ({ + default: vi.fn(() => { + const spinner = { + start: vi.fn(), + succeed: vi.fn(), + fail: vi.fn(), + text: '', + }; + spinnerInstances.push(spinner); + return spinner; + }), +})); + +vi.mock('inquirer', () => ({ + default: { + prompt: vi.fn(), + }, +})); + +vi.mock('../../services/auth.service.js', () => ({ + authService: { + isAuthenticated: vi.fn(), + getCredentials: vi.fn(), + login: vi.fn(), + logout: vi.fn(), + }, +})); + +import inquirer from 'inquirer'; +import { authService } from '../../services/auth.service.js'; +import { loginAction } from '../auth/login.js'; +import { logoutAction } from '../auth/logout.js'; + +const mockPrompt = vi.mocked(inquirer.prompt); +const mockAuthService = vi.mocked(authService); + +function mockProcessExit(): ReturnType { + return vi.spyOn(process, 'exit').mockImplementation((code?: string | number | null) => { + throw new Error(`process.exit:${code ?? 0}`); + }) as any; +} + +describe('auth login command', () => { + beforeEach(() => { + vi.clearAllMocks(); + spinnerInstances.length = 0; + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('logs in with provided email/password without prompts', async () => { + mockAuthService.isAuthenticated.mockResolvedValue(false); + mockAuthService.login.mockResolvedValue(undefined); + + await loginAction({ email: 'test@example.com', password: 'secret123' }); + + expect(mockPrompt).not.toHaveBeenCalled(); + expect(mockAuthService.login).toHaveBeenCalledWith('test@example.com', 'secret123'); + expect(spinnerInstances[0]?.start).toHaveBeenCalled(); + expect(spinnerInstances[0]?.succeed).toHaveBeenCalled(); + }); + + it('does not re-login when user cancels account switch', async () => { + mockAuthService.isAuthenticated.mockResolvedValue(true); + mockAuthService.getCredentials.mockResolvedValue({ + accessToken: 'token', + refreshToken: 'refresh', + expiresAt: Date.now() + 1000 * 60 * 60, + user: { id: 'u1', email: 'old@example.com', orgs: [] }, + } as any); + mockPrompt.mockResolvedValue({ confirm: false } as any); + + await loginAction({}); + + expect(mockAuthService.login).not.toHaveBeenCalled(); + expect(mockPrompt).toHaveBeenCalledTimes(1); + }); + + it('shows team-key switch prompt message and logs in when confirmed', async () => { + mockAuthService.isAuthenticated.mockResolvedValue(true); + mockAuthService.getCredentials.mockResolvedValue(null); + mockAuthService.login.mockResolvedValue(undefined); + mockPrompt + .mockResolvedValueOnce({ confirm: true } as any) + .mockResolvedValueOnce({ email: 'new@example.com', password: 'secret123' } as any); + + await loginAction({}); + + expect(mockPrompt).toHaveBeenCalledTimes(2); + const firstPrompt = mockPrompt.mock.calls[0][0] as Array<{ message?: string }>; + expect(firstPrompt[0]?.message).toBe('Do you want to login with an account instead?'); + expect(mockAuthService.login).toHaveBeenCalledWith('new@example.com', 'secret123'); + }); + + it('exits with code 1 when login fails', async () => { + const exitSpy = mockProcessExit(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + mockAuthService.isAuthenticated.mockResolvedValue(false); + mockAuthService.login.mockRejectedValue(new Error('bad credentials')); + + await expect(loginAction({ email: 'test@example.com', password: 'wrong' })).rejects.toThrow('process.exit:1'); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalled(); + }); +}); + +describe('auth logout command', () => { + beforeEach(() => { + vi.clearAllMocks(); + spinnerInstances.length = 0; + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('prints not authenticated when there is no session', async () => { + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + mockAuthService.isAuthenticated.mockResolvedValue(false); + + await logoutAction(); + + expect(mockAuthService.logout).not.toHaveBeenCalled(); + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining('Not authenticated.')); + }); + + it('logs out when authenticated', async () => { + mockAuthService.isAuthenticated.mockResolvedValue(true); + mockAuthService.logout.mockResolvedValue(undefined); + + await logoutAction(); + + expect(mockAuthService.logout).toHaveBeenCalledTimes(1); + expect(spinnerInstances[0]?.start).toHaveBeenCalled(); + expect(spinnerInstances[0]?.succeed).toHaveBeenCalled(); + }); + + it('exits with code 1 when logout fails', async () => { + const exitSpy = mockProcessExit(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + mockAuthService.isAuthenticated.mockResolvedValue(true); + mockAuthService.logout.mockRejectedValue(new Error('network')); + + await expect(logoutAction()).rejects.toThrow('process.exit:1'); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalled(); + }); +}); diff --git a/src/commands/__tests__/auth.team-key.test.ts b/src/commands/__tests__/auth.team-key.test.ts new file mode 100644 index 0000000..f3ade2e --- /dev/null +++ b/src/commands/__tests__/auth.team-key.test.ts @@ -0,0 +1,166 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../../utils/config.js', () => ({ + loadConfig: vi.fn(), + saveConfig: vi.fn(), +})); + +vi.mock('../../utils/credentials.js', () => ({ + clearCredentials: vi.fn(), +})); + +import { loadConfig, saveConfig } from '../../utils/config.js'; +import { clearCredentials } from '../../utils/credentials.js'; +import { teamKeyAction, teamStatusAction } from '../auth/team-key.js'; + +const mockLoadConfig = vi.mocked(loadConfig); +const mockSaveConfig = vi.mocked(saveConfig); +const mockClearCredentials = vi.mocked(clearCredentials); + +function mockProcessExit(): ReturnType { + return vi.spyOn(process, 'exit').mockImplementation((code?: string | number | null) => { + throw new Error(`process.exit:${code ?? 0}`); + }) as any; +} + +describe('auth team-key command', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.stubGlobal('fetch', vi.fn()); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it('exits when key is missing', async () => { + const exitSpy = mockProcessExit(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + await expect(teamKeyAction({})).rejects.toThrow('process.exit:1'); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalled(); + }); + + it('exits when key format is invalid', async () => { + const exitSpy = mockProcessExit(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + await expect(teamKeyAction({ key: 'invalid' })).rejects.toThrow('process.exit:1'); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalled(); + }); + + it('saves config and clears credentials when key is valid', async () => { + const fetchMock = vi.mocked(fetch); + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + data: { + team: { name: 'Platform Team' }, + organization: { name: 'Kodus' }, + }, + }), + { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }, + ), + ); + mockClearCredentials.mockResolvedValue(undefined); + + await teamKeyAction({ key: 'kodus_abc123' }); + + expect(mockSaveConfig).toHaveBeenCalledWith({ + teamKey: 'kodus_abc123', + teamName: 'Platform Team', + organizationName: 'Kodus', + }); + expect(mockClearCredentials).toHaveBeenCalled(); + expect(fetchMock).toHaveBeenCalledWith( + expect.stringContaining('/cli/validate-key'), + expect.objectContaining({ + headers: { 'X-Team-Key': 'kodus_abc123' }, + }), + ); + }); + + it('does not fail when clearing old credentials throws', async () => { + const fetchMock = vi.mocked(fetch); + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + data: { + teamName: 'Backend Team', + organizationName: 'Kodus', + }, + }), + { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }, + ), + ); + mockClearCredentials.mockRejectedValue(new Error('fs error')); + + await expect(teamKeyAction({ key: 'kodus_abc123' })).resolves.toBeUndefined(); + expect(mockSaveConfig).toHaveBeenCalled(); + }); + + it('exits when API returns invalid key', async () => { + const fetchMock = vi.mocked(fetch); + const exitSpy = mockProcessExit(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ message: 'Invalid team key' }), + { + status: 401, + headers: { 'Content-Type': 'application/json' }, + }, + ), + ); + + await expect(teamKeyAction({ key: 'kodus_abc123' })).rejects.toThrow('process.exit:1'); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalled(); + }); +}); + +describe('auth team-status command', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('shows not-authenticated message when no team config exists', async () => { + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + mockLoadConfig.mockResolvedValue(null); + + await teamStatusAction(); + + const output = logSpy.mock.calls.map((c) => c.join(' ')).join('\n'); + expect(output).toContain('Not authenticated with team key'); + }); + + it('shows team details when team config exists', async () => { + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + mockLoadConfig.mockResolvedValue({ + teamKey: 'kodus_abc123', + teamName: 'Platform Team', + organizationName: 'Kodus', + } as any); + + await teamStatusAction(); + + const output = logSpy.mock.calls.map((c) => c.join(' ')).join('\n'); + expect(output).toContain('Authenticated'); + expect(output).toContain('Kodus'); + expect(output).toContain('Platform Team'); + }); +}); diff --git a/src/commands/auth/index.ts b/src/commands/auth/index.ts index 1f499ab..a346a57 100644 --- a/src/commands/auth/index.ts +++ b/src/commands/auth/index.ts @@ -17,7 +17,7 @@ authCommand authCommand .command('logout') - .description('Remove local credentials') + .description('Remove local authentication (login and team key)') .action(logoutAction); authCommand @@ -40,4 +40,3 @@ authCommand .command('team-status') .description('Show team authentication status') .action(teamStatusAction); - diff --git a/src/commands/auth/login.ts b/src/commands/auth/login.ts index 4a91c51..663ecfa 100644 --- a/src/commands/auth/login.ts +++ b/src/commands/auth/login.ts @@ -16,13 +16,18 @@ export async function loginAction(options: LoginOptions): Promise { if (isAuthenticated && !options.email) { const credentials = await authService.getCredentials(); - console.log(chalk.yellow(`\nAlready logged in as ${credentials?.user.email}`)); + const email = credentials?.user?.email; + console.log( + chalk.yellow(email ? `\nAlready logged in as ${email}` : '\nAlready authenticated with team key'), + ); const { confirm } = await inquirer.prompt([ { type: 'confirm', name: 'confirm', - message: 'Do you want to login with a different account?', + message: email + ? 'Do you want to login with a different account?' + : 'Do you want to login with an account instead?', default: false, }, ]); diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index 4dcdb6b..867c258 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -9,7 +9,7 @@ export async function logoutAction(): Promise { const isAuthenticated = await authService.isAuthenticated(); if (!isAuthenticated) { - console.log(chalk.yellow('\nNot logged in.')); + console.log(chalk.yellow('\nNot authenticated.')); return; } @@ -27,4 +27,3 @@ export async function logoutAction(): Promise { process.exit(1); } } - diff --git a/src/commands/auth/team-key.ts b/src/commands/auth/team-key.ts index 1432a77..56c0dce 100644 --- a/src/commands/auth/team-key.ts +++ b/src/commands/auth/team-key.ts @@ -1,5 +1,6 @@ import chalk from 'chalk'; import { loadConfig, saveConfig } from '../../utils/config.js'; +import { clearCredentials } from '../../utils/credentials.js'; import { API_URL } from '../../constants.js'; export async function teamKeyAction(options: { key?: string }): Promise { @@ -39,6 +40,12 @@ export async function teamKeyAction(options: { key?: string }): Promise { teamName, organizationName, }); + // Team-key auth should not compete with a previously stored user session. + try { + await clearCredentials(); + } catch { + // Best effort cleanup. + } console.log(chalk.green('✓ Authenticated successfully!')); console.log(chalk.cyan(` Organization: ${organizationName}`)); diff --git a/src/services/__tests__/auth.service.test.ts b/src/services/__tests__/auth.service.test.ts index 389f2ad..014e9b2 100644 --- a/src/services/__tests__/auth.service.test.ts +++ b/src/services/__tests__/auth.service.test.ts @@ -22,11 +22,12 @@ vi.mock('../../utils/credentials.js', () => ({ vi.mock('../../utils/config.js', () => ({ loadConfig: vi.fn(), + clearConfig: vi.fn(), })); import { api } from '../api/index.js'; import { loadCredentials, saveCredentials, clearCredentials } from '../../utils/credentials.js'; -import { loadConfig } from '../../utils/config.js'; +import { loadConfig, clearConfig } from '../../utils/config.js'; import { AuthService } from '../auth.service.js'; const mockApi = vi.mocked(api); @@ -34,6 +35,7 @@ const mockLoadCredentials = vi.mocked(loadCredentials); const mockSaveCredentials = vi.mocked(saveCredentials); const mockClearCredentials = vi.mocked(clearCredentials); const mockLoadConfig = vi.mocked(loadConfig); +const mockClearConfig = vi.mocked(clearConfig); function makeCredentials(overrides: Partial = {}): StoredCredentials { return { @@ -77,6 +79,7 @@ describe('AuthService', () => { refreshToken: 'new-refresh-token', }) ); + expect(mockClearConfig).toHaveBeenCalled(); }); }); @@ -90,6 +93,7 @@ describe('AuthService', () => { expect(mockApi.auth.logout).toHaveBeenCalledWith('access-token'); expect(mockClearCredentials).toHaveBeenCalled(); + expect(mockClearConfig).toHaveBeenCalled(); }); it('ignores API errors during logout silently', async () => { @@ -100,6 +104,7 @@ describe('AuthService', () => { await authService.logout(); expect(mockClearCredentials).toHaveBeenCalled(); + expect(mockClearConfig).toHaveBeenCalled(); }); }); @@ -134,12 +139,23 @@ describe('AuthService', () => { describe('getValidToken', () => { it('returns teamKey when it exists', async () => { mockLoadConfig.mockResolvedValue({ teamKey: 'kodus_team_key' } as any); + mockLoadCredentials.mockResolvedValue(null); const token = await authService.getValidToken(); expect(token).toBe('kodus_team_key'); }); + it('prefers accessToken when credentials and teamKey both exist', async () => { + mockLoadConfig.mockResolvedValue({ teamKey: 'kodus_team_key' } as any); + const creds = makeCredentials({ expiresAt: Date.now() + 60 * 60 * 1000 }); + mockLoadCredentials.mockResolvedValue(creds); + + const token = await authService.getValidToken(); + + expect(token).toBe('access-token'); + }); + it('returns accessToken when not expired', async () => { mockLoadConfig.mockResolvedValue(null); const creds = makeCredentials({ expiresAt: Date.now() + 60 * 60 * 1000 }); @@ -178,6 +194,32 @@ describe('AuthService', () => { expect(mockClearCredentials).toHaveBeenCalled(); }); + it('falls back to teamKey when refresh fails and teamKey exists', async () => { + mockLoadConfig.mockResolvedValue({ teamKey: 'kodus_team_key' } as any); + const expiredCreds = makeCredentials({ expiresAt: Date.now() - 1000 }); + mockLoadCredentials.mockResolvedValue(expiredCreds); + mockApi.auth.refresh = vi.fn().mockRejectedValue(new Error('refresh failed')); + + const token = await authService.getValidToken(); + + expect(token).toBe('kodus_team_key'); + expect(mockClearCredentials).toHaveBeenCalled(); + }); + + it('deduplicates concurrent refresh requests', async () => { + mockLoadConfig.mockResolvedValue(null); + const expiredCreds = makeCredentials({ expiresAt: Date.now() - 1000 }); + mockLoadCredentials.mockResolvedValue(expiredCreds); + const refreshed = makeAuthResponse({ accessToken: 'single-refresh-token' }); + mockApi.auth.refresh = vi.fn().mockResolvedValue(refreshed); + + const [a, b] = await Promise.all([authService.getValidToken(), authService.getValidToken()]); + + expect(a).toBe('single-refresh-token'); + expect(b).toBe('single-refresh-token'); + expect(mockApi.auth.refresh).toHaveBeenCalledTimes(1); + }); + it('throws AuthError when no credentials exist', async () => { mockLoadConfig.mockResolvedValue(null); mockLoadCredentials.mockResolvedValue(null); diff --git a/src/services/api/__tests__/api.real.test.ts b/src/services/api/__tests__/api.real.test.ts new file mode 100644 index 0000000..b601bce --- /dev/null +++ b/src/services/api/__tests__/api.real.test.ts @@ -0,0 +1,124 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { RealApi } from '../api.real.js'; +import { ApiError } from '../../../types/index.js'; + +describe('RealApi review.getPullRequestSuggestions', () => { + let fetchMock: ReturnType; + + beforeEach(() => { + fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock as any); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it('sends X-Team-Key header when using team key', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + data: { summary: 'ok', issues: [], filesAnalyzed: 0, duration: 0 }, + }), + { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }, + ), + ); + + const api = new RealApi(); + await api.review.getPullRequestSuggestions('kodus_team_key', { prUrl: 'https://github.com/acme/repo/pull/1' }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const [, options] = fetchMock.mock.calls[0]; + expect(options.headers['X-Team-Key']).toBe('kodus_team_key'); + expect(options.headers.Authorization).toBeUndefined(); + }); + + it('sends Authorization header when using bearer token', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + data: { summary: 'ok', issues: [], filesAnalyzed: 0, duration: 0 }, + }), + { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }, + ), + ); + + const api = new RealApi(); + await api.review.getPullRequestSuggestions('eyJ.test.token', { prUrl: 'https://github.com/acme/repo/pull/1' }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const [, options] = fetchMock.mock.calls[0]; + expect(options.headers.Authorization).toBe('Bearer eyJ.test.token'); + expect(options.headers['X-Team-Key']).toBeUndefined(); + }); + + it('normalizes API auth errors to default CLI English message', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + message: 'Team key required by backend', + }), + { + status: 401, + headers: { 'Content-Type': 'application/json' }, + }, + ), + ); + + const api = new RealApi(); + + await expect( + api.review.getPullRequestSuggestions('eyJ.test.token', { prUrl: 'https://github.com/acme/repo/pull/1' }), + ).rejects.toEqual( + expect.objectContaining({ + name: 'ApiError', + statusCode: 401, + message: 'Authentication failed while fetching pull request suggestions. Run: kodus auth login or configure a valid team key.', + } satisfies Partial), + ); + }); +}); + +describe('RealApi review.analyze auth mode', () => { + let fetchMock: ReturnType; + + beforeEach(() => { + fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock as any); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it('sends Authorization header for user login token', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + data: { summary: 'ok', issues: [], filesAnalyzed: 0, duration: 0 }, + }), + { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }, + ), + ); + + const api = new RealApi(); + await api.review.analyze('diff --git a/file b/file', 'eyJ.test.token'); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const [url, options] = fetchMock.mock.calls[0]; + expect(url).toContain('/cli/review'); + expect(options.headers.Authorization).toBe('Bearer eyJ.test.token'); + expect(options.headers['X-Team-Key']).toBeUndefined(); + }); +}); diff --git a/src/services/api/api.real.ts b/src/services/api/api.real.ts index f3de0e5..d442625 100644 --- a/src/services/api/api.real.ts +++ b/src/services/api/api.real.ts @@ -51,6 +51,67 @@ function getApiBaseUrl(): string { const API_BASE_URL = getApiBaseUrl(); const REQUEST_TIMEOUT_MS = 20 * 60 * 1000; // 20 minutes +function getDefaultApiErrorMessage(statusCode: number, endpoint: string): string { + const endpointPath = endpoint.split('?')[0] || endpoint; + + if (statusCode === 400) { + return `Invalid request sent to Kodus API (${endpointPath}).`; + } + + if (statusCode === 401) { + if (endpointPath === '/pull-requests/suggestions') { + return 'Authentication failed while fetching pull request suggestions. Run: kodus auth login or configure a valid team key.'; + } + return 'Authentication failed. Run: kodus auth login or configure a valid team key.'; + } + + if (statusCode === 403) { + return `Access denied for Kodus API endpoint (${endpointPath}).`; + } + + if (statusCode === 404) { + return `Kodus API endpoint not found (${endpointPath}).`; + } + + if (statusCode === 422) { + return `Kodus API could not process the request (${endpointPath}).`; + } + + if (statusCode === 429) { + return 'Rate limit exceeded. Please try again later.'; + } + + if (statusCode >= 500) { + return 'Kodus API is currently unavailable. Please try again.'; + } + + return `Request failed with status ${statusCode}`; +} + +function normalizeApiErrorMessage(statusCode: number, endpoint: string, apiMessage?: string): string { + const fallbackMessage = getDefaultApiErrorMessage(statusCode, endpoint); + if (!apiMessage || typeof apiMessage !== 'string') { + return fallbackMessage; + } + + // Keep auth/permission/server errors deterministic and always in CLI English. + if (statusCode === 401 || statusCode === 403 || statusCode === 404 || statusCode === 429 || statusCode >= 500) { + return fallbackMessage; + } + + const trimmed = apiMessage.trim(); + if (!trimmed) { + return fallbackMessage; + } + + const hasNonAscii = /[^\x00-\x7F]/.test(trimmed); + if (hasNonAscii) { + return fallbackMessage; + } + + return trimmed; +} + async function request( endpoint: string, options: RequestInit = {} @@ -91,10 +152,16 @@ async function request( const errorData = isJson ? await response.json().catch(() => ({ message: 'Request failed' })) as { message?: string } : { message: `Request failed with status ${response.status}` }; - const errorMessage = errorData.message || `Request failed with status ${response.status}`; + const errorMessage = normalizeApiErrorMessage(response.status, endpoint, errorData.message); if (process.env.KODUS_VERBOSE) { - console.error('[API] Error:', { status: response.status, url, contentType, errorData }); + console.error('[API] Error:', { + status: response.status, + url, + contentType, + errorData, + normalizedMessage: errorMessage, + }); } throw new ApiError(response.status, errorMessage); @@ -132,8 +199,8 @@ async function request( } } - // API retorna { data: {...}, statusCode, type } - // Extrair apenas o .data se existir + // API usually returns { data: {...}, statusCode, type } + // Return only .data when present. if (json && typeof json === 'object' && 'data' in json) { return json.data as T; } @@ -184,13 +251,13 @@ class RealAuthApi implements IAuthApi { body: JSON.stringify({ email, password }), }); - // Mapear resposta da API para formato esperado pelo CLI + // Map API response into the CLI auth shape. return { accessToken: response.accessToken, refreshToken: response.refreshToken, - expiresIn: 3600, // Default: 1 hora + expiresIn: 3600, // Default: 1 hour user: { - id: 'unknown', // API não retorna user info no login + id: 'unknown', // Login response does not include user profile fields. email, orgs: [], }, @@ -365,10 +432,13 @@ class RealReviewApi implements IReviewApi { const queryString = query.toString(); const endpoint = `/pull-requests/suggestions${queryString ? `?${queryString}` : ''}`; + const isTeamKey = accessToken.startsWith('kodus_'); return requestWithRetry(endpoint, { headers: { - Authorization: `Bearer ${accessToken}`, + ...(isTeamKey + ? { 'X-Team-Key': accessToken } + : { Authorization: `Bearer ${accessToken}` }), }, }); } diff --git a/src/services/auth.service.ts b/src/services/auth.service.ts index a1833f0..2038384 100644 --- a/src/services/auth.service.ts +++ b/src/services/auth.service.ts @@ -4,7 +4,7 @@ import { saveCredentials, clearCredentials, } from '../utils/credentials.js'; -import { loadConfig } from '../utils/config.js'; +import { loadConfig, clearConfig } from '../utils/config.js'; import type { StoredCredentials, AuthResponse } from '../types/index.js'; import { AuthError } from '../types/index.js'; @@ -12,10 +12,17 @@ const TOKEN_REFRESH_BUFFER_MS = 5 * 60 * 1000; class AuthService { private cachedCredentials: StoredCredentials | null = null; + private refreshInFlight: Promise | null = null; async login(email: string, password: string): Promise { const response = await api.auth.login(email, password); await this.storeAuthResponse(response); + // Successful login switches auth mode to user credentials. + try { + await clearConfig(); + } catch { + // Best effort cleanup. + } } async logout(): Promise { @@ -29,7 +36,7 @@ class AuthService { } } - await clearCredentials(); + await Promise.all([clearCredentials(), clearConfig()]); this.cachedCredentials = null; } @@ -51,32 +58,24 @@ class AuthService { } async getValidToken(): Promise { - const config = await loadConfig(); - if (config?.teamKey) { - return config.teamKey; - } - const credentials = await this.getCredentials(); - if (!credentials) { - throw new AuthError('Not authenticated. Run: kodus auth login or kodus auth team-key --key '); - } - - const isExpired = Date.now() > credentials.expiresAt - TOKEN_REFRESH_BUFFER_MS; + if (credentials) { + const isExpired = Date.now() > credentials.expiresAt - TOKEN_REFRESH_BUFFER_MS; - if (isExpired) { - try { - const response = await api.auth.refresh(credentials.refreshToken); - await this.storeAuthResponse(response); - return response.accessToken; - } catch (error) { - await clearCredentials(); - this.cachedCredentials = null; - throw new AuthError('Session expired. Run: kodus auth login'); + if (isExpired) { + return this.refreshTokenOrFallback(credentials.refreshToken); } + + return credentials.accessToken; } - return credentials.accessToken; + const config = await loadConfig(); + if (config?.teamKey) { + return config.teamKey; + } + + throw new AuthError('Not authenticated. Run: kodus auth login or kodus auth team-key --key '); } async generateCIToken(): Promise { @@ -105,8 +104,35 @@ class AuthService { await saveCredentials(credentials); this.cachedCredentials = credentials; } + + private async refreshTokenOrFallback(refreshToken: string): Promise { + if (!this.refreshInFlight) { + this.refreshInFlight = (async () => { + try { + const response = await api.auth.refresh(refreshToken); + await this.storeAuthResponse(response); + return response.accessToken; + } catch (error) { + await clearCredentials(); + this.cachedCredentials = null; + + const config = await loadConfig(); + if (config?.teamKey) { + return config.teamKey; + } + + throw new AuthError('Session expired. Run: kodus auth login'); + } + })(); + } + + try { + return await this.refreshInFlight; + } finally { + this.refreshInFlight = null; + } + } } export { AuthService }; export const authService = new AuthService(); - diff --git a/src/services/git.service.ts b/src/services/git.service.ts index 63d0120..6f5f865 100644 --- a/src/services/git.service.ts +++ b/src/services/git.service.ts @@ -204,14 +204,14 @@ class GitService { branch?: string; }): Promise { await this.ensureRepo(); - // 1. Identificar arquivos a processar + // 1. Identify files to process let filesToRead: string[]; // Map to track file statuses (A=added, M=modified, D=deleted, R=renamed) const fileStatusMap = new Map(); if (explicitFiles && explicitFiles.length > 0) { - // Arquivos explícitos fornecidos + // Explicit files provided by the caller filesToRead = explicitFiles; } else if (options?.branch) { // Branch comparison: get files changed between branch and HEAD with status @@ -250,44 +250,44 @@ class GitService { } } - // 2. Para cada arquivo, ler conteúdo E diff + // 2. For each file, read content and diff const fileContents: FileContent[] = []; for (const filePath of filesToRead) { try { - // Determinar status do arquivo + // Resolve file status const status = fileStatusMap.get(filePath) || 'modified'; - // Pular arquivos deletados (não tem conteúdo) + // Skip deleted files (no content to read) if (status === 'deleted') continue; - // Pegar diff específico desse arquivo + // Read diff for this specific file let fileDiff: string; if (options?.branch) { - // Diff entre branch e HEAD + // Diff between target branch and HEAD fileDiff = await this.git.diff([`${options.branch}...HEAD`, '--', filePath]); } else if (options?.commit) { - // Diff do commit específico + // Diff for the selected commit fileDiff = await this.git.diff([`${options.commit}^`, options.commit, '--', filePath]); } else if (options?.staged) { - // Diff staged apenas + // Staged-only diff fileDiff = await this.git.diff(['--cached', '--', filePath]); } else { - // Diff completo (staged + unstaged) + // Full working-tree diff (staged + unstaged) const stagedDiff = await this.git.diff(['--cached', '--', filePath]); const unstagedDiff = await this.git.diff(['--', filePath]); fileDiff = `${stagedDiff}\n${unstagedDiff}`.trim(); } - // Ler conteúdo do arquivo + // Read file content let content: string; if (options?.commit) { - // Ler do commit específico + // Read file content from the selected commit content = await this.git.show([`${options.commit}:${filePath}`]); } else { - // Ler do working tree (fs.readFile) - // Para branch comparison, lê a versão atual (HEAD) + // Read from working tree (fs.readFile) + // For branch comparison this reads the current local version. const fullPath = path.resolve(filePath); content = await fs.readFile(fullPath, 'utf-8'); } @@ -299,7 +299,7 @@ class GitService { diff: fileDiff, }); } catch (error) { - // Arquivo pode ser binário ou inacessível - pular silenciosamente + // File may be binary or inaccessible; skip silently. continue; } } @@ -386,4 +386,3 @@ class GitService { } export const gitService = new GitService(); - diff --git a/src/utils/__tests__/config.test.ts b/src/utils/__tests__/config.test.ts new file mode 100644 index 0000000..5e30219 --- /dev/null +++ b/src/utils/__tests__/config.test.ts @@ -0,0 +1,96 @@ +import fs from 'fs/promises'; +import os from 'os'; +import path from 'path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +async function importConfigModule(homeDir: string): Promise { + vi.resetModules(); + vi.doMock('os', async () => { + const actual = await vi.importActual('os'); + return { + ...actual, + homedir: () => homeDir, + default: { + ...actual, + homedir: () => homeDir, + }, + }; + }); + + return import('../config.js'); +} + +describe('config utils', () => { + const tempDirs: string[] = []; + + afterEach(async () => { + vi.doUnmock('os'); + vi.restoreAllMocks(); + vi.resetModules(); + + while (tempDirs.length > 0) { + const dir = tempDirs.pop()!; + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + it('returns null when config file does not exist', async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-config-test-')); + tempDirs.push(home); + const { loadConfig } = await importConfigModule(home); + + await expect(loadConfig()).resolves.toBeNull(); + }); + + it('saves and loads config successfully', async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-config-test-')); + tempDirs.push(home); + const { saveConfig, loadConfig } = await importConfigModule(home); + + const input = { + teamKey: 'kodus_abc123', + teamName: 'Platform Team', + organizationName: 'Kodus', + }; + + await saveConfig(input); + const loaded = await loadConfig(); + + expect(loaded).toEqual(input); + }); + + it('writes config atomically without leaving temp files', async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-config-test-')); + tempDirs.push(home); + const { saveConfig } = await importConfigModule(home); + + await saveConfig({ + teamKey: 'kodus_abc123', + teamName: 'Platform Team', + organizationName: 'Kodus', + }); + + const configDir = path.join(home, '.kodus'); + const files = await fs.readdir(configDir); + expect(files.some((f) => f.includes('.tmp'))).toBe(false); + expect(files).toContain('config.json'); + }); + + it('self-heals malformed JSON by isolating corrupted config', async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-config-test-')); + tempDirs.push(home); + const { loadConfig } = await importConfigModule(home); + + const configDir = path.join(home, '.kodus'); + const configFile = path.join(configDir, 'config.json'); + await fs.mkdir(configDir, { recursive: true }); + await fs.writeFile(configFile, '{ malformed-json ', 'utf-8'); + + const loaded = await loadConfig(); + expect(loaded).toBeNull(); + + const files = await fs.readdir(configDir); + expect(files).not.toContain('config.json'); + expect(files.some((f) => f.startsWith('config.json.corrupted.'))).toBe(true); + }); +}); diff --git a/src/utils/__tests__/credentials.test.ts b/src/utils/__tests__/credentials.test.ts new file mode 100644 index 0000000..6a9ebcf --- /dev/null +++ b/src/utils/__tests__/credentials.test.ts @@ -0,0 +1,106 @@ +import fs from 'fs/promises'; +import os from 'os'; +import path from 'path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +async function importCredentialsModule(homeDir: string): Promise { + vi.resetModules(); + vi.doMock('os', async () => { + const actual = await vi.importActual('os'); + return { + ...actual, + homedir: () => homeDir, + default: { + ...actual, + homedir: () => homeDir, + }, + }; + }); + + return import('../credentials.js'); +} + +describe('credentials utils', () => { + const tempDirs: string[] = []; + + afterEach(async () => { + vi.doUnmock('os'); + vi.restoreAllMocks(); + vi.resetModules(); + + while (tempDirs.length > 0) { + const dir = tempDirs.pop()!; + await fs.rm(dir, { recursive: true, force: true }); + } + }); + + it('returns null when credentials file does not exist', async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-credentials-test-')); + tempDirs.push(home); + const { loadCredentials } = await importCredentialsModule(home); + + await expect(loadCredentials()).resolves.toBeNull(); + }); + + it('saves and loads credentials successfully', async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-credentials-test-')); + tempDirs.push(home); + const { saveCredentials, loadCredentials } = await importCredentialsModule(home); + + const input = { + accessToken: 'access', + refreshToken: 'refresh', + expiresAt: Date.now() + 60 * 60 * 1000, + user: { + id: 'u1', + email: 'dev@kodus.io', + orgs: ['Kodus'], + }, + }; + + await saveCredentials(input); + const loaded = await loadCredentials(); + + expect(loaded).toEqual(input); + }); + + it('writes credentials atomically without leaving temp files', async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-credentials-test-')); + tempDirs.push(home); + const { saveCredentials } = await importCredentialsModule(home); + + await saveCredentials({ + accessToken: 'access', + refreshToken: 'refresh', + expiresAt: Date.now() + 60 * 60 * 1000, + user: { + id: 'u1', + email: 'dev@kodus.io', + orgs: ['Kodus'], + }, + }); + + const configDir = path.join(home, '.kodus'); + const files = await fs.readdir(configDir); + expect(files.some((f) => f.includes('.tmp'))).toBe(false); + expect(files).toContain('credentials.json'); + }); + + it('self-heals malformed JSON by isolating corrupted credentials', async () => { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'kodus-credentials-test-')); + tempDirs.push(home); + const { loadCredentials } = await importCredentialsModule(home); + + const configDir = path.join(home, '.kodus'); + const credentialsFile = path.join(configDir, 'credentials.json'); + await fs.mkdir(configDir, { recursive: true }); + await fs.writeFile(credentialsFile, '{ malformed-json ', 'utf-8'); + + const loaded = await loadCredentials(); + expect(loaded).toBeNull(); + + const files = await fs.readdir(configDir); + expect(files).not.toContain('credentials.json'); + expect(files.some((f) => f.startsWith('credentials.json.corrupted.'))).toBe(true); + }); +}); diff --git a/src/utils/config.ts b/src/utils/config.ts index dc84007..63dd9b5 100644 --- a/src/utils/config.ts +++ b/src/utils/config.ts @@ -11,6 +11,10 @@ export interface CliConfig { organizationName: string; } +function isJsonParseError(error: unknown): boolean { + return error instanceof SyntaxError; +} + async function ensureConfigDir(): Promise { try { await fs.mkdir(CONFIG_DIR, { recursive: true, mode: 0o700 }); @@ -23,10 +27,11 @@ async function ensureConfigDir(): Promise { export async function saveConfig(config: CliConfig): Promise { await ensureConfigDir(); - await fs.writeFile(CONFIG_FILE, JSON.stringify(config, null, 2), { - encoding: 'utf-8', - mode: 0o600, - }); + const tmpFile = `${CONFIG_FILE}.${process.pid}.${Date.now()}.tmp`; + const content = JSON.stringify(config, null, 2); + + await fs.writeFile(tmpFile, content, { encoding: 'utf-8', mode: 0o600 }); + await fs.rename(tmpFile, CONFIG_FILE); } export async function loadConfig(): Promise { @@ -34,9 +39,18 @@ export async function loadConfig(): Promise { const content = await fs.readFile(CONFIG_FILE, 'utf-8'); return JSON.parse(content) as CliConfig; } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ENOENT') { return null; } + + // Self-heal malformed JSON by isolating the broken file and treating as no config. + if (isJsonParseError(error)) { + const brokenFile = `${CONFIG_FILE}.corrupted.${Date.now()}`; + await fs.rename(CONFIG_FILE, brokenFile).catch(() => {}); + return null; + } + throw error; } } diff --git a/src/utils/credentials.ts b/src/utils/credentials.ts index d7d5193..67fc4df 100644 --- a/src/utils/credentials.ts +++ b/src/utils/credentials.ts @@ -6,6 +6,10 @@ import type { StoredCredentials, GitInfo, PlatformType } from '../types/index.js const KODUS_DIR = path.join(os.homedir(), '.kodus'); const CREDENTIALS_FILE = path.join(KODUS_DIR, 'credentials.json'); +function isJsonParseError(error: unknown): boolean { + return error instanceof SyntaxError; +} + async function ensureKodusDir(): Promise { try { await fs.mkdir(KODUS_DIR, { recursive: true, mode: 0o700 }); @@ -21,19 +25,29 @@ export async function loadCredentials(): Promise { const content = await fs.readFile(CREDENTIALS_FILE, 'utf-8'); return JSON.parse(content) as StoredCredentials; } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ENOENT') { + return null; + } + + // Self-heal malformed JSON by isolating the broken file and treating as no credentials. + if (isJsonParseError(error)) { + const brokenFile = `${CREDENTIALS_FILE}.corrupted.${Date.now()}`; + await fs.rename(CREDENTIALS_FILE, brokenFile).catch(() => {}); return null; } + throw error; } } export async function saveCredentials(credentials: StoredCredentials): Promise { await ensureKodusDir(); - await fs.writeFile(CREDENTIALS_FILE, JSON.stringify(credentials, null, 2), { - encoding: 'utf-8', - mode: 0o600, - }); + const tmpFile = `${CREDENTIALS_FILE}.${process.pid}.${Date.now()}.tmp`; + const content = JSON.stringify(credentials, null, 2); + + await fs.writeFile(tmpFile, content, { encoding: 'utf-8', mode: 0o600 }); + await fs.rename(tmpFile, CREDENTIALS_FILE); } export async function clearCredentials(): Promise { @@ -54,4 +68,3 @@ export async function credentialsExist(): Promise { return false; } } - From 7a9d9e0a8984b6948cbe5ea9a8a018b38e950d34 Mon Sep 17 00:00:00 2001 From: Gabriel Malinosqui Date: Wed, 18 Feb 2026 17:14:16 -0300 Subject: [PATCH 2/4] fix(auth): fail fast and rollback when team-key switch cannot clear credentials --- src/commands/__tests__/auth.team-key.test.ts | 13 ++++++++++--- src/commands/auth/team-key.ts | 5 +++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/commands/__tests__/auth.team-key.test.ts b/src/commands/__tests__/auth.team-key.test.ts index f3ade2e..35f680b 100644 --- a/src/commands/__tests__/auth.team-key.test.ts +++ b/src/commands/__tests__/auth.team-key.test.ts @@ -3,18 +3,20 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; vi.mock('../../utils/config.js', () => ({ loadConfig: vi.fn(), saveConfig: vi.fn(), + clearConfig: vi.fn(), })); vi.mock('../../utils/credentials.js', () => ({ clearCredentials: vi.fn(), })); -import { loadConfig, saveConfig } from '../../utils/config.js'; +import { clearConfig, loadConfig, saveConfig } from '../../utils/config.js'; import { clearCredentials } from '../../utils/credentials.js'; import { teamKeyAction, teamStatusAction } from '../auth/team-key.js'; const mockLoadConfig = vi.mocked(loadConfig); const mockSaveConfig = vi.mocked(saveConfig); +const mockClearConfig = vi.mocked(clearConfig); const mockClearCredentials = vi.mocked(clearCredentials); function mockProcessExit(): ReturnType { @@ -86,8 +88,10 @@ describe('auth team-key command', () => { ); }); - it('does not fail when clearing old credentials throws', async () => { + it('fails and rolls back team config when clearing old credentials throws', async () => { const fetchMock = vi.mocked(fetch); + const exitSpy = mockProcessExit(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); fetchMock.mockResolvedValue( new Response( JSON.stringify({ @@ -104,7 +108,10 @@ describe('auth team-key command', () => { ); mockClearCredentials.mockRejectedValue(new Error('fs error')); - await expect(teamKeyAction({ key: 'kodus_abc123' })).resolves.toBeUndefined(); + await expect(teamKeyAction({ key: 'kodus_abc123' })).rejects.toThrow('process.exit:1'); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalled(); + expect(mockClearConfig).toHaveBeenCalledTimes(1); expect(mockSaveConfig).toHaveBeenCalled(); }); diff --git a/src/commands/auth/team-key.ts b/src/commands/auth/team-key.ts index 56c0dce..dec17c3 100644 --- a/src/commands/auth/team-key.ts +++ b/src/commands/auth/team-key.ts @@ -1,5 +1,5 @@ import chalk from 'chalk'; -import { loadConfig, saveConfig } from '../../utils/config.js'; +import { clearConfig, loadConfig, saveConfig } from '../../utils/config.js'; import { clearCredentials } from '../../utils/credentials.js'; import { API_URL } from '../../constants.js'; @@ -44,7 +44,8 @@ export async function teamKeyAction(options: { key?: string }): Promise { try { await clearCredentials(); } catch { - // Best effort cleanup. + await clearConfig().catch(() => {}); + throw new Error('Failed to switch to team-key auth because personal credentials could not be cleared.'); } console.log(chalk.green('✓ Authenticated successfully!')); From 35d5e4a37dc7ebec35782f993950be2904b82b05 Mon Sep 17 00:00:00 2001 From: Gabriel Malinosqui Date: Wed, 18 Feb 2026 17:32:24 -0300 Subject: [PATCH 3/4] fix: fallback PR suggestions auth and align branch file content --- .../__tests__/review.suggestions-auth.test.ts | 92 +++++++++++++++++++ src/services/git.service.ts | 4 +- src/services/review.service.ts | 20 +++- 3 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 src/services/__tests__/review.suggestions-auth.test.ts diff --git a/src/services/__tests__/review.suggestions-auth.test.ts b/src/services/__tests__/review.suggestions-auth.test.ts new file mode 100644 index 0000000..5e06468 --- /dev/null +++ b/src/services/__tests__/review.suggestions-auth.test.ts @@ -0,0 +1,92 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ApiError } from '../../types/index.js'; + +vi.mock('../api/index.js', () => ({ + api: { + review: { + getPullRequestSuggestions: vi.fn(), + }, + }, +})); + +vi.mock('../auth.service.js', () => ({ + authService: { + getValidToken: vi.fn(), + }, +})); + +vi.mock('../../utils/config.js', () => ({ + loadConfig: vi.fn(), +})); + +import { api } from '../api/index.js'; +import { authService } from '../auth.service.js'; +import { loadConfig } from '../../utils/config.js'; +import { reviewService } from '../review.service.js'; + +const mockApi = vi.mocked(api); +const mockAuthService = vi.mocked(authService); +const mockLoadConfig = vi.mocked(loadConfig); + +describe('ReviewService getPullRequestSuggestions auth fallback', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('falls back to team key when bearer auth returns 401', async () => { + mockAuthService.getValidToken.mockResolvedValue('eyJ.user.token'); + mockLoadConfig.mockResolvedValue({ + teamKey: 'kodus_team_key', + teamName: 'Team', + organizationName: 'Org', + } as any); + + mockApi.review.getPullRequestSuggestions = vi + .fn() + .mockRejectedValueOnce(new ApiError(401, 'Unauthorized')) + .mockResolvedValueOnce({ + summary: 'Suggestions', + issues: [], + filesAnalyzed: 0, + duration: 0, + } as any); + + const result = await reviewService.getPullRequestSuggestions({ + prUrl: 'https://github.com/kodustech/cli/pull/6', + }); + + expect(mockApi.review.getPullRequestSuggestions).toHaveBeenCalledTimes(2); + expect(mockApi.review.getPullRequestSuggestions).toHaveBeenNthCalledWith( + 1, + 'eyJ.user.token', + expect.objectContaining({ prUrl: 'https://github.com/kodustech/cli/pull/6' }), + ); + expect(mockApi.review.getPullRequestSuggestions).toHaveBeenNthCalledWith( + 2, + 'kodus_team_key', + expect.objectContaining({ prUrl: 'https://github.com/kodustech/cli/pull/6' }), + ); + expect(result.result.summary).toBe('Suggestions'); + }); + + it('does not fallback when token is already a team key', async () => { + mockAuthService.getValidToken.mockResolvedValue('kodus_team_key'); + mockApi.review.getPullRequestSuggestions = vi.fn().mockRejectedValue(new ApiError(401, 'Unauthorized')); + + await expect( + reviewService.getPullRequestSuggestions({ prUrl: 'https://github.com/kodustech/cli/pull/6' }), + ).rejects.toThrow(ApiError); + + expect(mockLoadConfig).not.toHaveBeenCalled(); + }); + + it('rethrows when bearer auth fails with 401 and no team key is configured', async () => { + mockAuthService.getValidToken.mockResolvedValue('eyJ.user.token'); + mockLoadConfig.mockResolvedValue(null); + mockApi.review.getPullRequestSuggestions = vi.fn().mockRejectedValue(new ApiError(401, 'Unauthorized')); + + await expect( + reviewService.getPullRequestSuggestions({ prUrl: 'https://github.com/kodustech/cli/pull/6' }), + ).rejects.toThrow(ApiError); + }); +}); diff --git a/src/services/git.service.ts b/src/services/git.service.ts index 6f5f865..9bcfa47 100644 --- a/src/services/git.service.ts +++ b/src/services/git.service.ts @@ -285,9 +285,11 @@ class GitService { if (options?.commit) { // Read file content from the selected commit content = await this.git.show([`${options.commit}:${filePath}`]); + } else if (options?.branch) { + // Read file content from HEAD to match the branch comparison diff target. + content = await this.git.show([`HEAD:${filePath}`]); } else { // Read from working tree (fs.readFile) - // For branch comparison this reads the current local version. const fullPath = path.resolve(filePath); content = await fs.readFile(fullPath, 'utf-8'); } diff --git a/src/services/review.service.ts b/src/services/review.service.ts index 79c3103..18ce39b 100644 --- a/src/services/review.service.ts +++ b/src/services/review.service.ts @@ -5,6 +5,7 @@ import { getTrialIdentifier } from '../utils/rate-limit.js'; import { loadConfig } from '../utils/config.js'; import { CLI_VERSION } from '../constants.js'; import chalk from 'chalk'; +import { ApiError } from '../types/index.js'; import type { ReviewConfig, ReviewResult, TrialReviewResult, PullRequestSuggestionsResponse, ReviewIssue, ApiFileSuggestion, ApiPrLevelSuggestion, ApiSuggestionsObject, Severity, FileContent } from '../types/index.js'; const MAX_FILES = 100; @@ -127,7 +128,24 @@ class ReviewService { } const token = await authService.getValidToken(); - const response = await api.review.getPullRequestSuggestions(token, params); + + let response: PullRequestSuggestionsResponse; + try { + response = await api.review.getPullRequestSuggestions(token, params); + } catch (error) { + const canFallbackToTeamKey = error instanceof ApiError && error.statusCode === 401 && !token.startsWith('kodus_'); + if (!canFallbackToTeamKey) { + throw error; + } + + const config = await loadConfig(); + if (!config?.teamKey) { + throw error; + } + + response = await api.review.getPullRequestSuggestions(config.teamKey, params); + } + return { result: this.normalizeSuggestionsResponse(response), markdown: response.markdown, From 65f00f535a2e3b584b3723fa5152624781796f34 Mon Sep 17 00:00:00 2001 From: Gabriel Malinosqui Date: Wed, 18 Feb 2026 17:41:45 -0300 Subject: [PATCH 4/4] fix(review): preserve primary auth error when fallback also fails --- .../__tests__/review.suggestions-auth.test.ts | 20 +++++++++++++++++++ src/services/review.service.ts | 7 ++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/services/__tests__/review.suggestions-auth.test.ts b/src/services/__tests__/review.suggestions-auth.test.ts index 5e06468..9c63cd8 100644 --- a/src/services/__tests__/review.suggestions-auth.test.ts +++ b/src/services/__tests__/review.suggestions-auth.test.ts @@ -89,4 +89,24 @@ describe('ReviewService getPullRequestSuggestions auth fallback', () => { reviewService.getPullRequestSuggestions({ prUrl: 'https://github.com/kodustech/cli/pull/6' }), ).rejects.toThrow(ApiError); }); + + it('rethrows original bearer error when fallback with team key also fails', async () => { + const originalError = new ApiError(401, 'Bearer unauthorized'); + const fallbackError = new ApiError(401, 'Team key unauthorized'); + + mockAuthService.getValidToken.mockResolvedValue('eyJ.user.token'); + mockLoadConfig.mockResolvedValue({ + teamKey: 'kodus_team_key', + teamName: 'Team', + organizationName: 'Org', + } as any); + mockApi.review.getPullRequestSuggestions = vi + .fn() + .mockRejectedValueOnce(originalError) + .mockRejectedValueOnce(fallbackError); + + await expect( + reviewService.getPullRequestSuggestions({ prUrl: 'https://github.com/kodustech/cli/pull/6' }), + ).rejects.toBe(originalError); + }); }); diff --git a/src/services/review.service.ts b/src/services/review.service.ts index 18ce39b..5b8578d 100644 --- a/src/services/review.service.ts +++ b/src/services/review.service.ts @@ -143,7 +143,12 @@ class ReviewService { throw error; } - response = await api.review.getPullRequestSuggestions(config.teamKey, params); + try { + response = await api.review.getPullRequestSuggestions(config.teamKey, params); + } catch { + // Preserve the primary auth failure from the original token attempt. + throw error; + } } return {