Skip to content

perf: skip loadCliConfig + refreshAuth in parent for non-sandbox startup #24804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bzqzheng wants to merge 2 commits into from
+67 −67
Closed

perf: skip loadCliConfig + refreshAuth in parent for non-sandbox startup #24804

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0370de7
perf: skip loadCliConfig + refreshAuth in parent for non-sandbox startup
bzqzheng Apr 6, 2026
51865fd
refactor: extract performEarlyInit to deduplicate config+auth logic
bzqzheng Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 67 additions & 67 deletions packages/cli/src/gemini.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,80 +348,82 @@ export async function main() {
}
}

const partialConfig = await loadCliConfig(settings.merged, sessionId, argv, {
projectHooks: settings.workspace.settings.hooks,
});
adminControlsListner.setConfig(partialConfig);

// Refresh auth to fetch remote admin settings from CCPA and before entering
// the sandbox because the sandbox will interfere with the Oauth2 web
// redirect.
let initialAuthFailed = false;
if (!settings.merged.security.auth.useExternal && !argv.isCommand) {
try {
if (
partialConfig.isInteractive() &&
settings.merged.security.auth.selectedType
) {
const err = validateAuthMethod(
settings.merged.security.auth.selectedType,
);
if (err) {
throw new Error(err);
// Determine memory args and sandbox config early — these are needed
// regardless of whether we take the fast (relaunch) or full (sandbox) path.
const memoryArgs =
!process.env['SANDBOX'] &&
!argv.isCommand &&
settings.merged.advanced.autoConfigureMemory
? getNodeMemoryArgs(isDebugMode)
: [];

async function performEarlyInit() {
const partialConfig = await loadCliConfig(
settings.merged,
sessionId,
argv,
{
projectHooks: settings.workspace.settings.hooks,
},
);
adminControlsListner.setConfig(partialConfig);

let authFailed = false;
if (!settings.merged.security.auth.useExternal && !argv.isCommand) {
try {
if (
partialConfig.isInteractive() &&
settings.merged.security.auth.selectedType
) {
const err = validateAuthMethod(
settings.merged.security.auth.selectedType,
);
if (err) {
throw new Error(err);
}

await partialConfig.refreshAuth(
settings.merged.security.auth.selectedType,
);
} else if (!partialConfig.isInteractive()) {
const authType = await validateNonInteractiveAuth(
settings.merged.security.auth.selectedType,
settings.merged.security.auth.useExternal,
partialConfig,
settings,
);
await partialConfig.refreshAuth(authType);
}
} catch (err) {
if (err instanceof ValidationCancelledError) {
await runExitCleanup();
process.exit(ExitCodes.SUCCESS);
}

await partialConfig.refreshAuth(
settings.merged.security.auth.selectedType,
);
} else if (!partialConfig.isInteractive()) {
const authType = await validateNonInteractiveAuth(
settings.merged.security.auth.selectedType,
settings.merged.security.auth.useExternal,
partialConfig,
settings,
);
await partialConfig.refreshAuth(authType);
}
} catch (err) {
if (err instanceof ValidationCancelledError) {
// User cancelled verification, exit immediately.
await runExitCleanup();
process.exit(ExitCodes.SUCCESS);
if (!(err instanceof ValidationRequiredError)) {
debugLogger.error('Error authenticating:', err);
authFailed = true;
}
}
}

// If validation is required, we don't treat it as a fatal failure.
// We allow the app to start, and the React-based ValidationDialog
// will handle it.
if (!(err instanceof ValidationRequiredError)) {
debugLogger.error('Error authenticating:', err);
initialAuthFailed = true;
}
const remoteAdminSettings = partialConfig.getRemoteAdminSettings();
if (remoteAdminSettings) {
settings.setRemoteAdminSettings(remoteAdminSettings);
}
}

const remoteAdminSettings = partialConfig.getRemoteAdminSettings();
// Set remote admin settings if returned from CCPA.
if (remoteAdminSettings) {
settings.setRemoteAdminSettings(remoteAdminSettings);
}
await runDeferredCommand(settings.merged);
Comment on lines +361 to +415
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The logic for loading configuration, refreshing authentication, and handling remote admin settings is now duplicated between the if (sandboxConfig) block (lines 345-400) and this else block. This duplication increases maintenance overhead and has already introduced inconsistencies, such as the missing initialAuthFailed check in this second block. Please refactor this shared initialization logic into a reusable function or restructure the control flow to avoid duplication.

Copy link
Copy Markdown
Author

@bzqzheng bzqzheng Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the duplication — addressing in a follow-up commit by extracting a performEarlyInit() function. Re: the missing initialAuthFailed: this is intentional, not a bug. The sandbox path needs a hard exit (FATAL_AUTHENTICATION_ERROR) before entering the sandbox because it blocks OAuth redirects. The else path falls through to the full init at line ~507 which handles auth independently, so an early fatal exit would be wrong.


// Run deferred command now that we have admin settings.
await runDeferredCommand(settings.merged);
return { partialConfig, authFailed };
}

// hop into sandbox if we are outside and sandboxing is enabled
if (!process.env['SANDBOX'] && !argv.isCommand) {
const memoryArgs = settings.merged.advanced.autoConfigureMemory
? getNodeMemoryArgs(isDebugMode)
: [];
const sandboxConfig = await loadSandboxConfig(settings.merged, argv);
// We intentionally omit the list of extensions here because extensions
// should not impact auth or setting up the sandbox.
// TODO(jacobr): refactor loadCliConfig so there is a minimal version
// that only initializes enough config to enable refreshAuth or find
// another way to decouple refreshAuth from requiring a config.

if (sandboxConfig) {
if (initialAuthFailed) {
const { partialConfig, authFailed } = await performEarlyInit();

if (authFailed) {
await runExitCleanup();
process.exit(ExitCodes.FATAL_AUTHENTICATION_ERROR);
}
Expand All @@ -442,11 +444,9 @@ export async function main() {
(arg) => arg === '--prompt' || arg === '-p',
);
if (promptIndex > -1 && finalArgs.length > promptIndex + 1) {
// If there's a prompt argument, prepend stdin to it
finalArgs[promptIndex + 1] =
`${stdinData}\n\n${finalArgs[promptIndex + 1]}`;
} else {
// If there's no prompt argument, add stdin as the prompt
finalArgs.push('--prompt', stdinData);
}
}
Expand All @@ -461,10 +461,10 @@ export async function main() {
await runExitCleanup();
process.exit(ExitCodes.SUCCESS);
} else {
// Relaunch app so we always have a child process that can be internally
// restarted if needed.
await relaunchAppInChildProcess(memoryArgs, [], remoteAdminSettings);
await relaunchAppInChildProcess(memoryArgs, []);
}
} else {
await performEarlyInit();
}

// We are now past the logic handling potentially launching a child process
Expand Down