fix(admin): fix a few bugs related to admin controls (#17590)

This commit is contained in:
Shreya Keshive
2026-01-27 12:14:11 -05:00
committed by GitHub
parent a63277c1d0
commit 7904f973a0
8 changed files with 191 additions and 22 deletions
+63 -7
View File
@@ -2215,8 +2215,8 @@ describe('Settings Loading and Merging', () => {
// and missing properties revert to schema defaults.
loadedSettings.setRemoteAdminSettings({ secureModeEnabled: false });
expect(loadedSettings.merged.admin?.secureModeEnabled).toBe(false);
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(true); // Reverts to default: true
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(true); // Reverts to default: true
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(false); // Defaulting to false if missing
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(false); // Defaulting to false if missing
});
it('should correctly handle undefined remote admin settings', () => {
@@ -2276,10 +2276,10 @@ describe('Settings Loading and Merging', () => {
secureModeEnabled: true,
});
// Verify secureModeEnabled is updated, others remain defaults
// Verify secureModeEnabled is updated, others default to false
expect(loadedSettings.merged.admin?.secureModeEnabled).toBe(true);
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(true);
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(true);
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(false);
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(false);
// Set remote settings with only mcpSetting.mcpEnabled
loadedSettings.setRemoteAdminSettings({
@@ -2289,7 +2289,7 @@ describe('Settings Loading and Merging', () => {
// Verify mcpEnabled is updated, others remain defaults (secureModeEnabled reverts to default:false)
expect(loadedSettings.merged.admin?.secureModeEnabled).toBe(false);
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(false);
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(true);
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(false);
// Set remote settings with only cliFeatureSetting.extensionsSetting.extensionsEnabled
loadedSettings.setRemoteAdminSettings({
@@ -2298,7 +2298,7 @@ describe('Settings Loading and Merging', () => {
// Verify extensionsEnabled is updated, others remain defaults
expect(loadedSettings.merged.admin?.secureModeEnabled).toBe(false);
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(true);
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(false);
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(false);
});
@@ -2318,6 +2318,62 @@ describe('Settings Loading and Merging', () => {
});
expect(loadedSettings.merged.admin.skills?.enabled).toBe(false);
});
it('should default mcp.enabled to false if mcpSetting is present but mcpEnabled is undefined', () => {
const loadedSettings = loadSettings(MOCK_WORKSPACE_DIR);
loadedSettings.setRemoteAdminSettings({
mcpSetting: {},
});
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(false);
});
it('should default extensions.enabled to false if extensionsSetting is present but extensionsEnabled is undefined', () => {
const loadedSettings = loadSettings(MOCK_WORKSPACE_DIR);
loadedSettings.setRemoteAdminSettings({
cliFeatureSetting: {
extensionsSetting: {},
},
});
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(false);
});
it('should force secureModeEnabled to false if undefined, overriding schema defaults', () => {
// Mock schema to have secureModeEnabled default to true to verify the override
const originalSchema = getSettingsSchema();
const modifiedSchema = JSON.parse(JSON.stringify(originalSchema));
if (modifiedSchema.admin?.properties?.secureModeEnabled) {
modifiedSchema.admin.properties.secureModeEnabled.default = true;
}
vi.mocked(getSettingsSchema).mockReturnValue(modifiedSchema);
try {
(mockFsExistsSync as Mock).mockReturnValue(true);
(fs.readFileSync as Mock).mockImplementation(() => '{}');
const loadedSettings = loadSettings(MOCK_WORKSPACE_DIR);
// Pass a non-empty object that doesn't have secureModeEnabled
loadedSettings.setRemoteAdminSettings({
mcpSetting: {},
});
// It should be forced to false by the logic, overriding the mock default of true
expect(loadedSettings.merged.admin?.secureModeEnabled).toBe(false);
} finally {
vi.mocked(getSettingsSchema).mockReturnValue(originalSchema);
}
});
it('should handle completely empty remote admin settings response', () => {
const loadedSettings = loadSettings(MOCK_WORKSPACE_DIR);
loadedSettings.setRemoteAdminSettings({});
// Should default to schema defaults (standard defaults)
expect(loadedSettings.merged.admin?.secureModeEnabled).toBe(false);
expect(loadedSettings.merged.admin?.mcp?.enabled).toBe(true);
expect(loadedSettings.merged.admin?.extensions?.enabled).toBe(true);
});
});
describe('getDefaultsFromSchema', () => {
+9 -10
View File
@@ -350,18 +350,17 @@ export class LoadedSettings {
const admin: Settings['admin'] = {};
const { secureModeEnabled, mcpSetting, cliFeatureSetting } = remoteSettings;
if (secureModeEnabled !== undefined) {
admin.secureModeEnabled = secureModeEnabled;
if (Object.keys(remoteSettings).length === 0) {
this._remoteAdminSettings = { admin };
this._merged = this.computeMergedSettings();
return;
}
if (mcpSetting?.mcpEnabled !== undefined) {
admin.mcp = { enabled: mcpSetting.mcpEnabled };
}
const extensionsSetting = cliFeatureSetting?.extensionsSetting;
if (extensionsSetting?.extensionsEnabled !== undefined) {
admin.extensions = { enabled: extensionsSetting.extensionsEnabled };
}
admin.secureModeEnabled = secureModeEnabled ?? false;
admin.mcp = { enabled: mcpSetting?.mcpEnabled ?? false };
admin.extensions = {
enabled: cliFeatureSetting?.extensionsSetting?.extensionsEnabled ?? false,
};
if (cliFeatureSetting?.advancedFeaturesEnabled !== undefined) {
admin.skills = { enabled: cliFeatureSetting.advancedFeaturesEnabled };