From 261788cf911c5ea1dd67b1e4084c40b11063a185 Mon Sep 17 00:00:00 2001 From: Shreya Keshive Date: Wed, 18 Feb 2026 17:54:07 -0500 Subject: [PATCH] feat(admin): Admin settings should only apply if adminControlsApplicable = true and fetch errors should be fatal (#19453) --- packages/cli/src/ui/AppContainer.tsx | 1 + .../code_assist/admin/admin_controls.test.ts | 94 +++++++++++-------- .../src/code_assist/admin/admin_controls.ts | 53 +++++------ packages/core/src/code_assist/types.ts | 1 + packages/core/src/config/config.ts | 2 +- 5 files changed, 79 insertions(+), 72 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index af05cb41a8..a3b460555b 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -699,6 +699,7 @@ export const AppContainer = (props: AppContainerProps) => { settings.setValue(scope, 'security.auth.selectedType', authType); try { + config.setRemoteAdminSettings(undefined); await config.refreshAuth(authType); setAuthState(AuthState.Authenticated); } catch (e) { diff --git a/packages/core/src/code_assist/admin/admin_controls.test.ts b/packages/core/src/code_assist/admin/admin_controls.test.ts index 0606d7f255..d676a59a92 100644 --- a/packages/core/src/code_assist/admin/admin_controls.test.ts +++ b/packages/core/src/code_assist/admin/admin_controls.test.ts @@ -345,6 +345,7 @@ describe('Admin Controls', () => { // Should still start polling (mockServer.fetchAdminControls as Mock).mockResolvedValue({ strictModeDisabled: true, + adminControlsApplicable: true, }); await vi.advanceTimersByTimeAsync(5 * 60 * 1000); @@ -363,7 +364,10 @@ describe('Admin Controls', () => { }); it('should fetch from server if no cachedSettings provided', async () => { - const serverResponse = { strictModeDisabled: false }; + const serverResponse = { + strictModeDisabled: false, + adminControlsApplicable: true, + }; (mockServer.fetchAdminControls as Mock).mockResolvedValue(serverResponse); const result = await fetchAdminControls( @@ -386,31 +390,24 @@ describe('Admin Controls', () => { expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); }); - it('should return empty object on fetch error and still start polling', async () => { - (mockServer.fetchAdminControls as Mock).mockRejectedValue( - new Error('Network error'), - ); - const result = await fetchAdminControls( - mockServer, - undefined, - true, - mockOnSettingsChanged, - ); + it('should throw error on fetch error and NOT start polling', async () => { + const error = new Error('Network error'); + (mockServer.fetchAdminControls as Mock).mockRejectedValue(error); - expect(result).toEqual({}); + await expect( + fetchAdminControls(mockServer, undefined, true, mockOnSettingsChanged), + ).rejects.toThrow(error); - // Polling should have been started and should retry - (mockServer.fetchAdminControls as Mock).mockResolvedValue({ - strictModeDisabled: false, - }); + // Polling should NOT have been started + // Advance timers just to be absolutely sure await vi.advanceTimersByTimeAsync(5 * 60 * 1000); - expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(2); // Initial + poll + expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); // Only initial fetch }); - it('should return empty object on 403 fetch error and STOP polling', async () => { - const error403 = new Error('Forbidden'); - Object.assign(error403, { status: 403 }); - (mockServer.fetchAdminControls as Mock).mockRejectedValue(error403); + it('should return empty object on adminControlsApplicable false and STOP polling', async () => { + (mockServer.fetchAdminControls as Mock).mockResolvedValue({ + adminControlsApplicable: false, + }); const result = await fetchAdminControls( mockServer, @@ -421,7 +418,7 @@ describe('Admin Controls', () => { expect(result).toEqual({}); - // Advance time - should NOT poll because of 403 + // Advance time - should NOT poll because of adminControlsApplicable: false await vi.advanceTimersByTimeAsync(5 * 60 * 1000); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); // Only the initial call }); @@ -430,6 +427,7 @@ describe('Admin Controls', () => { (mockServer.fetchAdminControls as Mock).mockResolvedValue({ strictModeDisabled: false, unknownField: 'bad', + adminControlsApplicable: true, }); const result = await fetchAdminControls( @@ -455,7 +453,9 @@ describe('Admin Controls', () => { }); it('should reset polling interval if called again', async () => { - (mockServer.fetchAdminControls as Mock).mockResolvedValue({}); + (mockServer.fetchAdminControls as Mock).mockResolvedValue({ + adminControlsApplicable: true, + }); // First call await fetchAdminControls( @@ -514,6 +514,7 @@ describe('Admin Controls', () => { const serverResponse = { strictModeDisabled: true, unknownField: 'should be removed', + adminControlsApplicable: true, }; (mockServer.fetchAdminControls as Mock).mockResolvedValue(serverResponse); @@ -532,22 +533,22 @@ describe('Admin Controls', () => { expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); }); - it('should return empty object on 403 fetch error', async () => { - const error403 = new Error('Forbidden'); - Object.assign(error403, { status: 403 }); - (mockServer.fetchAdminControls as Mock).mockRejectedValue(error403); + it('should return empty object on adminControlsApplicable false', async () => { + (mockServer.fetchAdminControls as Mock).mockResolvedValue({ + adminControlsApplicable: false, + }); const result = await fetchAdminControlsOnce(mockServer, true); expect(result).toEqual({}); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); }); - it('should return empty object on any other fetch error', async () => { - (mockServer.fetchAdminControls as Mock).mockRejectedValue( - new Error('Network error'), + it('should throw error on any other fetch error', async () => { + const error = new Error('Network error'); + (mockServer.fetchAdminControls as Mock).mockRejectedValue(error); + await expect(fetchAdminControlsOnce(mockServer, true)).rejects.toThrow( + error, ); - const result = await fetchAdminControlsOnce(mockServer, true); - expect(result).toEqual({}); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); }); @@ -555,7 +556,9 @@ describe('Admin Controls', () => { const setIntervalSpy = vi.spyOn(global, 'setInterval'); const clearIntervalSpy = vi.spyOn(global, 'clearInterval'); - (mockServer.fetchAdminControls as Mock).mockResolvedValue({}); + (mockServer.fetchAdminControls as Mock).mockResolvedValue({ + adminControlsApplicable: true, + }); await fetchAdminControlsOnce(mockServer, true); expect(setIntervalSpy).not.toHaveBeenCalled(); @@ -568,6 +571,7 @@ describe('Admin Controls', () => { // Initial fetch (mockServer.fetchAdminControls as Mock).mockResolvedValue({ strictModeDisabled: true, + adminControlsApplicable: true, }); await fetchAdminControls( mockServer, @@ -579,6 +583,7 @@ describe('Admin Controls', () => { // Update for next poll (mockServer.fetchAdminControls as Mock).mockResolvedValue({ strictModeDisabled: false, + adminControlsApplicable: true, }); // Fast forward @@ -598,7 +603,10 @@ describe('Admin Controls', () => { }); it('should NOT emit if settings are deeply equal but not the same instance', async () => { - const settings = { strictModeDisabled: false }; + const settings = { + strictModeDisabled: false, + adminControlsApplicable: true, + }; (mockServer.fetchAdminControls as Mock).mockResolvedValue(settings); await fetchAdminControls( @@ -613,6 +621,7 @@ describe('Admin Controls', () => { // Next poll returns a different object with the same values (mockServer.fetchAdminControls as Mock).mockResolvedValue({ strictModeDisabled: false, + adminControlsApplicable: true, }); await vi.advanceTimersByTimeAsync(5 * 60 * 1000); @@ -623,6 +632,7 @@ describe('Admin Controls', () => { // Initial fetch is successful (mockServer.fetchAdminControls as Mock).mockResolvedValue({ strictModeDisabled: true, + adminControlsApplicable: true, }); await fetchAdminControls( mockServer, @@ -643,6 +653,7 @@ describe('Admin Controls', () => { // Subsequent poll succeeds with new data (mockServer.fetchAdminControls as Mock).mockResolvedValue({ strictModeDisabled: false, + adminControlsApplicable: true, }); await vi.advanceTimersByTimeAsync(5 * 60 * 1000); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(3); @@ -659,10 +670,11 @@ describe('Admin Controls', () => { }); }); - it('should STOP polling if server returns 403', async () => { + it('should STOP polling if server returns adminControlsApplicable false', async () => { // Initial fetch is successful (mockServer.fetchAdminControls as Mock).mockResolvedValue({ strictModeDisabled: true, + adminControlsApplicable: true, }); await fetchAdminControls( mockServer, @@ -672,10 +684,10 @@ describe('Admin Controls', () => { ); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(1); - // Next poll returns 403 - const error403 = new Error('Forbidden'); - Object.assign(error403, { status: 403 }); - (mockServer.fetchAdminControls as Mock).mockRejectedValue(error403); + // Next poll returns adminControlsApplicable: false + (mockServer.fetchAdminControls as Mock).mockResolvedValue({ + adminControlsApplicable: false, + }); await vi.advanceTimersByTimeAsync(5 * 60 * 1000); expect(mockServer.fetchAdminControls).toHaveBeenCalledTimes(2); @@ -688,7 +700,9 @@ describe('Admin Controls', () => { describe('stopAdminControlsPolling', () => { it('should stop polling after it has started', async () => { - (mockServer.fetchAdminControls as Mock).mockResolvedValue({}); + (mockServer.fetchAdminControls as Mock).mockResolvedValue({ + adminControlsApplicable: true, + }); // Start polling await fetchAdminControls( diff --git a/packages/core/src/code_assist/admin/admin_controls.ts b/packages/core/src/code_assist/admin/admin_controls.ts index 43816215a1..d4117c2107 100644 --- a/packages/core/src/code_assist/admin/admin_controls.ts +++ b/packages/core/src/code_assist/admin/admin_controls.ts @@ -80,15 +80,6 @@ export function sanitizeAdminSettings( }; } -function isGaxiosError(error: unknown): error is { status: number } { - return ( - typeof error === 'object' && - error !== null && - 'status' in error && - typeof (error as { status: unknown }).status === 'number' - ); -} - /** * Fetches the admin controls from the server if enabled by experiment flag. * Safely handles polling start/stop based on the flag and server availability. @@ -113,7 +104,7 @@ export async function fetchAdminControls( // If we already have settings (e.g. from IPC during relaunch), use them // to avoid blocking startup with another fetch. We'll still start polling. - if (cachedSettings) { + if (cachedSettings && Object.keys(cachedSettings).length !== 0) { currentSettings = cachedSettings; startAdminControlsPolling(server, server.projectId, onSettingsChanged); return cachedSettings; @@ -123,22 +114,20 @@ export async function fetchAdminControls( const rawSettings = await server.fetchAdminControls({ project: server.projectId, }); + + if (rawSettings.adminControlsApplicable !== true) { + stopAdminControlsPolling(); + currentSettings = undefined; + return {}; + } + const sanitizedSettings = sanitizeAdminSettings(rawSettings); currentSettings = sanitizedSettings; startAdminControlsPolling(server, server.projectId, onSettingsChanged); return sanitizedSettings; } catch (e) { - // Non-enterprise users don't have access to fetch settings. - if (isGaxiosError(e) && e.status === 403) { - stopAdminControlsPolling(); - currentSettings = undefined; - return {}; - } debugLogger.error('Failed to fetch admin controls: ', e); - // If initial fetch fails, start polling to retry. - currentSettings = {}; - startAdminControlsPolling(server, server.projectId, onSettingsChanged); - return {}; + throw e; } } @@ -162,17 +151,18 @@ export async function fetchAdminControlsOnce( const rawSettings = await server.fetchAdminControls({ project: server.projectId, }); - return sanitizeAdminSettings(rawSettings); - } catch (e) { - // Non-enterprise users don't have access to fetch settings. - if (isGaxiosError(e) && e.status === 403) { + + if (rawSettings.adminControlsApplicable !== true) { return {}; } + + return sanitizeAdminSettings(rawSettings); + } catch (e) { debugLogger.error( 'Failed to fetch admin controls: ', e instanceof Error ? e.message : e, ); - return {}; + throw e; } } @@ -192,6 +182,13 @@ function startAdminControlsPolling( const rawSettings = await server.fetchAdminControls({ project, }); + + if (rawSettings.adminControlsApplicable !== true) { + stopAdminControlsPolling(); + currentSettings = undefined; + return; + } + const newSettings = sanitizeAdminSettings(rawSettings); if (!isDeepStrictEqual(newSettings, currentSettings)) { @@ -199,12 +196,6 @@ function startAdminControlsPolling( onSettingsChanged(newSettings); } } catch (e) { - // Non-enterprise users don't have access to fetch settings. - if (isGaxiosError(e) && e.status === 403) { - stopAdminControlsPolling(); - currentSettings = undefined; - return; - } debugLogger.error('Failed to poll admin controls: ', e); } }, diff --git a/packages/core/src/code_assist/types.ts b/packages/core/src/code_assist/types.ts index 3f9bd9fa7e..7845ceee89 100644 --- a/packages/core/src/code_assist/types.ts +++ b/packages/core/src/code_assist/types.ts @@ -355,4 +355,5 @@ export const FetchAdminControlsResponseSchema = z.object({ strictModeDisabled: z.boolean().optional(), mcpSetting: McpSettingSchema.optional(), cliFeatureSetting: CliFeatureSettingSchema.optional(), + adminControlsApplicable: z.boolean().optional(), }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 525e22743d..ad2b0a1a1b 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1171,7 +1171,7 @@ export class Config { return this.remoteAdminSettings; } - setRemoteAdminSettings(settings: AdminControlsSettings): void { + setRemoteAdminSettings(settings: AdminControlsSettings | undefined): void { this.remoteAdminSettings = settings; }