auto-claude: 151-fix-pr-review-agent-token-refresh-on-account-swap (#1456)

* auto-claude: subtask-1-1 - Add GITHUB_AUTH_CHANGED IPC channel constant

* auto-claude: subtask-1-2 - Add async getGitHubTokenForSubprocess() helper to utils.ts

Add a new exported async function getGitHubTokenForSubprocess() that calls
getTokenFromGhCli() to retrieve fresh GitHub tokens for subprocess use.
This provides a clean interface for runner-env.ts to get tokens without
caching, ensuring account changes are reflected immediately.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* auto-claude: subtask-1-3 - Update getRunnerEnv() to include GITHUB_TOKEN

* auto-claude: subtask-2-1 - Add auth change detection and event emission to oauth-handlers.ts

- Add GitHubAuthChangedPayload interface for auth change events
- Add sendAuthChangedToRenderer() to broadcast auth changes to all windows
- Add getCurrentGitHubUsername() helper to get current GitHub user
- Modify registerStartGhAuth() to:
  - Capture username before auth starts
  - Get username after successful auth
  - Emit GITHUB_AUTH_CHANGED event if account changed

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* auto-claude: subtask-3-1 - Add onGitHubAuthChanged listener to GitHubAPI interface

- Add onGitHubAuthChanged to GitHubAPI interface in github-api.ts
- Add implementation using createIpcListener with IPC_CHANNELS.GITHUB_AUTH_CHANGED
- Add mock implementation in browser-mock.ts for testing

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* auto-claude: subtask-4-1 - Add GitHub auth change listener to pr-review-store

* fix: Address PR review findings for GitHub auth handlers

- Convert getCurrentGitHubUsername() to async using promisified execFile
  to avoid blocking Electron main thread during auth flow (finding #1)
- Make getGitHubTokenForSubprocess() truly async by introducing async
  getTokenFromGhCliAsync() - the sync version is preserved for
  getGitHubConfig (finding #2)
- Add warning log when username fetch fails after successful auth,
  handling the edge case where auth succeeds but account change
  detection fails (finding #3)
- Remove unused timestamp field from GitHubAuthChangedPayload interface
  since the renderer callback only uses oldUsername/newUsername (finding #4)
- Document the intentional extraEnv override behavior in getRunnerEnv()
  JSDoc comment (finding #5)

All 5 findings from PR review were real issues. These fixes improve code
quality by avoiding main thread blocking and clarifying edge cases.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* test: Fix oauth-handlers tests for async getCurrentGitHubUsername

Update test mocks and add waitForAsyncSetup helper to handle the async
changes in getCurrentGitHubUsername(). The function now uses promisified
execFile instead of execFileSync to avoid blocking the main thread.

Key changes:
- Add mockExecFile mock for the promisified execFile function
- Add waitForAsyncSetup helper to wait for async setup before emitting
  process events
- Update all affected tests to use waitForAsyncSetup before emitting
  mock process events

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andy
2026-01-25 11:30:51 +01:00
committed by GitHub
parent 4937d57453
commit d081af0422
8 changed files with 222 additions and 11 deletions
@@ -9,6 +9,7 @@ import { EventEmitter } from 'events';
const mockSpawn = vi.fn();
const mockExecSync = vi.fn();
const mockExecFileSync = vi.fn();
const mockExecFile = vi.fn();
vi.mock('child_process', async (importOriginal) => {
const actual = await importOriginal<typeof import('child_process')>();
@@ -16,7 +17,8 @@ vi.mock('child_process', async (importOriginal) => {
...actual,
spawn: (...args: unknown[]) => mockSpawn(...args),
execSync: (...args: unknown[]) => mockExecSync(...args),
execFileSync: (...args: unknown[]) => mockExecFileSync(...args)
execFileSync: (...args: unknown[]) => mockExecFileSync(...args),
execFile: (...args: unknown[]) => mockExecFile(...args)
};
});
@@ -110,6 +112,10 @@ function createMockProcess(): EventEmitter & {
return proc;
}
// Helper to wait for async setup (getCurrentGitHubUsername) to complete
// This is needed because the handler now awaits async operations before spawning
const waitForAsyncSetup = () => new Promise(resolve => setTimeout(resolve, 20));
describe('GitHub OAuth Handlers', () => {
let ipcMain: EventEmitter & {
handlers: Map<string, Function>;
@@ -125,6 +131,24 @@ describe('GitHub OAuth Handlers', () => {
mockGetAugmentedEnv.mockReturnValue(process.env as Record<string, string>);
mockFindExecutable.mockReturnValue(null); // Default: executable not found
// Set up default execFile mock for getCurrentGitHubUsername (async)
// This returns null by default (not authenticated)
mockExecFile.mockImplementation(
(
_cmd: string,
_args: string[],
_options: unknown,
callback?: (error: Error | null, stdout: string, stderr: string) => void
) => {
// If callback provided, call it with error to simulate not authenticated
if (callback) {
callback(new Error('not authenticated'), '', '');
}
// Return a mock ChildProcess-like object
return { on: vi.fn(), stdout: null, stderr: null };
}
);
// Get mocked ipcMain
const electron = await import('electron');
ipcMain = electron.ipcMain as unknown as typeof ipcMain;
@@ -146,6 +170,9 @@ describe('GitHub OAuth Handlers', () => {
// Start the handler
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup (getCurrentGitHubUsername) to complete
await waitForAsyncSetup();
// Simulate gh CLI output with device code
mockProcess.stderr?.emit('data', '! First copy your one-time code: ABCD-1234\n');
mockProcess.stderr?.emit('data', '- Press Enter to open github.com in your browser...\n');
@@ -171,6 +198,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
// Alternate format: "code: XXXX-XXXX" without "one-time"
mockProcess.stderr?.emit('data', 'Enter the code: EFGH-5678\n');
mockProcess.emit('close', 0);
@@ -192,6 +222,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
// Device code in stdout instead of stderr
mockProcess.stdout?.emit('data', '! First copy your one-time code: IJKL-9012\n');
mockProcess.emit('close', 0);
@@ -212,6 +245,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
// Output without device code
mockProcess.stderr?.emit('data', 'Some other message\n');
mockProcess.emit('close', 0);
@@ -233,6 +269,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
mockProcess.stderr?.emit('data', '! First copy your one-time code: MNOP-3456\n');
mockProcess.stderr?.emit('data', 'Then visit https://github.com/login/device to authenticate\n');
mockProcess.emit('close', 0);
@@ -256,9 +295,12 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
mockProcess.stderr?.emit('data', '! First copy your one-time code: QRST-7890\n');
// Wait for next tick to allow async browser opening
// Wait for async browser opening
await new Promise(resolve => setTimeout(resolve, 10));
mockProcess.emit('close', 0);
@@ -277,6 +319,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
mockProcess.stderr?.emit('data', '! First copy your one-time code: UVWX-1234\n');
// Wait for async browser opening
@@ -300,6 +345,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
mockProcess.stderr?.emit('data', '! First copy your one-time code: YZAB-5678\n');
// Wait for async browser opening to fail
@@ -323,6 +371,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
mockProcess.stderr?.emit('data', '! First copy your one-time code: CDEF-9012\n');
// Wait for async browser opening to fail
@@ -346,6 +397,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
mockProcess.stderr?.emit('data', '! First copy your one-time code: GHIJ-3456\n');
// Wait for async browser opening
@@ -370,6 +424,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
// Emit error event
mockProcess.emit('error', new Error('spawn gh ENOENT'));
@@ -390,6 +447,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
mockProcess.stderr?.emit('data', 'error: some authentication error\n');
mockProcess.emit('close', 1);
@@ -410,6 +470,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
// Device code output followed by failure
mockProcess.stderr?.emit('data', '! First copy your one-time code: KLMN-7890\n');
@@ -436,6 +499,9 @@ describe('GitHub OAuth Handlers', () => {
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for async setup
await waitForAsyncSetup();
mockProcess.emit('error', new Error('spawn gh ENOENT'));
const result = await resultPromise;
@@ -532,7 +598,11 @@ describe('GitHub OAuth Handlers', () => {
const { registerStartGhAuth } = await import('../oauth-handlers');
registerStartGhAuth();
ipcMain.invokeHandler('github:startAuth', {});
// Start the handler (this is async due to getCurrentGitHubUsername)
const resultPromise = ipcMain.invokeHandler('github:startAuth', {});
// Wait for the async getCurrentGitHubUsername to complete and spawn to be called
await new Promise(resolve => setTimeout(resolve, 10));
expect(mockSpawn).toHaveBeenCalledWith(
'gh',
@@ -541,6 +611,10 @@ describe('GitHub OAuth Handlers', () => {
stdio: ['pipe', 'pipe', 'pipe']
})
);
// Complete the process to avoid hanging promise
mockProcess.emit('close', 0);
await resultPromise;
});
});
@@ -4,12 +4,15 @@
*/
import { ipcMain, shell, BrowserWindow } from 'electron';
import { execSync, execFileSync, spawn } from 'child_process';
import { execSync, execFileSync, execFile, spawn } from 'child_process';
import { promisify } from 'util';
import { IPC_CHANNELS } from '../../../shared/constants';
import type { IPCResult } from '../../../shared/types';
import { getAugmentedEnv, findExecutable } from '../../env-utils';
import { getToolPath } from '../../cli-tool-manager';
const execFileAsync = promisify(execFile);
/**
* Send device code info to all renderer windows immediately when extracted
* This allows the UI to display the code while the auth process is still running
@@ -26,6 +29,48 @@ function sendDeviceCodeToRenderer(deviceCode: string, authUrl: string, browserOp
}
}
/**
* Payload for GitHub auth change event
*/
interface GitHubAuthChangedPayload {
oldUsername: string | null;
newUsername: string;
}
/**
* Send auth change notification to all renderer windows
* This notifies the UI that GitHub authentication has changed (e.g., account swap)
*/
function sendAuthChangedToRenderer(oldUsername: string | null, newUsername: string): void {
debugLog('Sending auth changed event to renderer windows', { oldUsername, newUsername });
const windows = BrowserWindow.getAllWindows();
const payload: GitHubAuthChangedPayload = {
oldUsername,
newUsername
};
for (const win of windows) {
win.webContents.send(IPC_CHANNELS.GITHUB_AUTH_CHANGED, payload);
}
}
/**
* Get current GitHub username from gh CLI (async to avoid blocking main thread)
* Returns null if not authenticated or on error
*/
async function getCurrentGitHubUsername(): Promise<string | null> {
try {
const { stdout } = await execFileAsync(getToolPath('gh'), ['api', 'user', '--jq', '.login'], {
encoding: 'utf-8',
env: getAugmentedEnv()
});
const username = stdout.trim();
return username || null;
} catch {
// Not authenticated or gh CLI error
return null;
}
}
// Debug logging helper
const DEBUG = process.env.DEBUG === 'true' || process.env.NODE_ENV === 'development';
@@ -230,12 +275,20 @@ interface GitHubAuthStartResult {
* Start GitHub OAuth flow using gh CLI
* This will extract the device code from gh CLI output and open the browser
* using Electron's shell.openExternal (bypasses macOS child process restrictions)
*
* Detects account changes and emits GITHUB_AUTH_CHANGED event when the authenticated
* account differs from the previous one (or when going from unauthenticated to authenticated).
*/
export function registerStartGhAuth(): void {
ipcMain.handle(
IPC_CHANNELS.GITHUB_START_AUTH,
async (): Promise<IPCResult<GitHubAuthStartResult>> => {
debugLog('startGitHubAuth handler called');
// Capture current username before auth to detect account changes (async to avoid blocking main thread)
const usernameBeforeAuth = await getCurrentGitHubUsername();
debugLog('Username before auth:', usernameBeforeAuth || '(not authenticated)');
return new Promise((resolve) => {
try {
// Use gh auth login with web flow and repo scope
@@ -313,12 +366,29 @@ export function registerStartGhAuth(): void {
void tryExtractAndOpenBrowser();
});
ghProcess.on('close', (code) => {
ghProcess.on('close', async (code) => {
debugLog('gh process exited with code:', code);
debugLog('Full stdout:', output);
debugLog('Full stderr:', errorOutput);
if (code === 0) {
// Check for auth change after successful authentication (async to avoid blocking main thread)
const usernameAfterAuth = await getCurrentGitHubUsername();
debugLog('Username after auth:', usernameAfterAuth || '(unknown)');
// Emit auth changed event if account changed (or went from unauthenticated to authenticated)
if (usernameAfterAuth && usernameAfterAuth !== usernameBeforeAuth) {
debugLog('GitHub account changed detected', {
from: usernameBeforeAuth || '(none)',
to: usernameAfterAuth
});
sendAuthChangedToRenderer(usernameBeforeAuth, usernameAfterAuth);
} else if (!usernameAfterAuth) {
// Auth succeeded (exit code 0) but username fetch failed - log warning
// This edge case means we can't detect account changes, but auth is still valid
debugLog('WARNING: Auth succeeded but could not fetch username to detect account change');
}
// Success case - include fallbackUrl if browser failed to open
// so the user can manually navigate if needed
resolve({
@@ -3,7 +3,8 @@
*/
import { existsSync, readFileSync } from 'fs';
import { execFileSync } from 'child_process';
import { execFileSync, execFile } from 'child_process';
import { promisify } from 'util';
import path from 'path';
import type { Project } from '../../../shared/types';
import { parseEnvFile } from '../utils';
@@ -11,11 +12,30 @@ import type { GitHubConfig } from './types';
import { getAugmentedEnv } from '../../env-utils';
import { getToolPath } from '../../cli-tool-manager';
const execFileAsync = promisify(execFile);
/**
* Get GitHub token from gh CLI if available
* Get GitHub token from gh CLI if available (async to avoid blocking main thread)
* Uses augmented PATH to find gh CLI in common locations (e.g., Homebrew on macOS)
*/
function getTokenFromGhCli(): string | null {
async function getTokenFromGhCliAsync(): Promise<string | null> {
try {
const { stdout } = await execFileAsync(getToolPath('gh'), ['auth', 'token'], {
encoding: 'utf-8',
env: getAugmentedEnv()
});
const token = stdout.trim();
return token || null;
} catch {
return null;
}
}
/**
* Get GitHub token from gh CLI if available (sync version for getGitHubConfig)
* Uses augmented PATH to find gh CLI in common locations (e.g., Homebrew on macOS)
*/
function getTokenFromGhCliSync(): string | null {
try {
const token = execFileSync(getToolPath('gh'), ['auth', 'token'], {
encoding: 'utf-8',
@@ -28,6 +48,15 @@ function getTokenFromGhCli(): string | null {
}
}
/**
* Get a fresh GitHub token for subprocess use (async to avoid blocking main thread)
* Always fetches fresh from gh CLI - no caching to ensure account changes are reflected
* @returns The current GitHub token or null if not authenticated
*/
export async function getGitHubTokenForSubprocess(): Promise<string | null> {
return getTokenFromGhCliAsync();
}
/**
* Get GitHub configuration from project environment file
* Falls back to gh CLI token if GITHUB_TOKEN not in .env
@@ -43,9 +72,9 @@ export function getGitHubConfig(project: Project): GitHubConfig | null {
let token: string | undefined = vars['GITHUB_TOKEN'];
const repo = vars['GITHUB_REPO'];
// If no token in .env, try to get it from gh CLI
// If no token in .env, try to get it from gh CLI (sync version for sync function)
if (!token) {
const ghToken = getTokenFromGhCli();
const ghToken = getTokenFromGhCliSync();
if (ghToken) {
token = ghToken;
}
@@ -2,6 +2,7 @@ import { getOAuthModeClearVars } from '../../../agent/env-utils';
import { getAPIProfileEnv } from '../../../services/profile';
import { getProfileEnv } from '../../../rate-limit-detector';
import { pythonEnvManager } from '../../../python-env-manager';
import { getGitHubTokenForSubprocess } from '../utils';
/**
* Get environment variables for Python runner subprocesses.
@@ -11,7 +12,11 @@ import { pythonEnvManager } from '../../../python-env-manager';
* 2. apiProfileEnv - Custom Anthropic-compatible API profile (ANTHROPIC_BASE_URL, ANTHROPIC_AUTH_TOKEN)
* 3. oauthModeClearVars - Clears stale ANTHROPIC_* vars when in OAuth mode
* 4. profileEnv - Claude OAuth token from profile manager (CLAUDE_CODE_OAUTH_TOKEN)
* 5. extraEnv - Caller-specific vars (e.g., USE_CLAUDE_MD)
* 5. githubEnv - Fresh GitHub token from gh CLI (GITHUB_TOKEN) - fetched on each call to reflect account changes
* 6. extraEnv - Caller-specific vars (e.g., USE_CLAUDE_MD)
*
* NOTE: extraEnv can intentionally override any of the above, including GITHUB_TOKEN.
* This allows callers to provide their own token for testing or special cases.
*
* The pythonEnv is critical for packaged apps (#139) - without PYTHONPATH, Python
* cannot find bundled dependencies like dotenv, claude_agent_sdk, etc.
@@ -19,6 +24,9 @@ import { pythonEnvManager } from '../../../python-env-manager';
* The profileEnv is critical for OAuth authentication (#563) - it retrieves the
* decrypted OAuth token from the profile manager's encrypted storage (macOS Keychain
* via Electron's safeStorage API).
*
* The githubEnv is critical for GitHub operations (#151) - it fetches a fresh token
* from the gh CLI on each call to ensure account changes are reflected immediately.
*/
export async function getRunnerEnv(
extraEnv?: Record<string, string>
@@ -28,11 +36,16 @@ export async function getRunnerEnv(
const oauthModeClearVars = getOAuthModeClearVars(apiProfileEnv);
const profileEnv = getProfileEnv();
// Fetch fresh GitHub token from gh CLI (no caching to reflect account changes)
const githubToken = await getGitHubTokenForSubprocess();
const githubEnv: Record<string, string> = githubToken ? { GITHUB_TOKEN: githubToken } : {};
return {
...pythonEnv, // Python environment including PYTHONPATH (fixes #139)
...apiProfileEnv,
...oauthModeClearVars,
...profileEnv, // OAuth token from profile manager (fixes #563)
...githubEnv, // Fresh GitHub token from gh CLI (fixes #151)
...extraEnv,
};
}
@@ -189,6 +189,11 @@ export interface GitHubAPI {
callback: (data: { deviceCode: string; authUrl: string; browserOpened: boolean }) => void
) => IpcListenerCleanup;
// OAuth event listener - notifies when GitHub account changes (via gh auth login)
onGitHubAuthChanged: (
callback: (data: { oldUsername: string | null; newUsername: string }) => void
) => IpcListenerCleanup;
// Repository detection and management
detectGitHubRepo: (projectPath: string) => Promise<IPCResult<string>>;
getGitHubBranches: (repo: string, token: string) => Promise<IPCResult<string[]>>;
@@ -532,6 +537,12 @@ export const createGitHubAPI = (): GitHubAPI => ({
): IpcListenerCleanup =>
createIpcListener(IPC_CHANNELS.GITHUB_AUTH_DEVICE_CODE, callback),
// OAuth event listener - notifies when GitHub account changes (via gh auth login)
onGitHubAuthChanged: (
callback: (data: { oldUsername: string | null; newUsername: string }) => void
): IpcListenerCleanup =>
createIpcListener(IPC_CHANNELS.GITHUB_AUTH_CHANGED, callback),
// Repository detection and management
detectGitHubRepo: (projectPath: string): Promise<IPCResult<string>> =>
invokeIpc(IPC_CHANNELS.GITHUB_DETECT_REPO, projectPath),
@@ -189,6 +189,7 @@ const browserMockAPI: ElectronAPI = {
addGitRemote: async () => ({ success: true, data: { remoteUrl: '' } }),
listGitHubOrgs: async () => ({ success: true, data: { orgs: [] } }),
onGitHubAuthDeviceCode: () => () => {},
onGitHubAuthChanged: () => () => {},
onGitHubInvestigationProgress: () => () => {},
onGitHubInvestigationComplete: () => () => {},
onGitHubInvestigationError: () => () => {},
@@ -252,6 +252,18 @@ export function initializePRReviewListeners(): void {
}
);
// Listen for GitHub auth changes - clear all PR review state when account changes
window.electronAPI.github.onGitHubAuthChanged(
(data: { oldUsername: string | null; newUsername: string }) => {
console.warn(
`[PRReviewStore] GitHub auth changed from "${data.oldUsername ?? 'none'}" to "${data.newUsername}". ` +
`Clearing all PR review state.`
);
// Clear all PR review state since the token has changed
usePRReviewStore.setState({ prReviews: {} });
}
);
prReviewListenersInitialized = true;
}
@@ -254,6 +254,7 @@ export const IPC_CHANNELS = {
// GitHub OAuth events (main -> renderer) - for streaming device code during auth
GITHUB_AUTH_DEVICE_CODE: 'github:authDeviceCode',
GITHUB_AUTH_CHANGED: 'github:authChanged', // Event: GitHub auth state changed (account swap)
// GitHub events (main -> renderer)
GITHUB_INVESTIGATION_PROGRESS: 'github:investigationProgress',