-
Notifications
You must be signed in to change notification settings - Fork 10.9k
A2a admin setting overrides #17565
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.
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
base: main
Are you sure you want to change the base?
A2a admin setting overrides #17565
Changes from all commits
f447120
ffe503e
53ec0ab
7f2114e
51cb51b
ea96970
27885e8
992b469
28950e4
25be924
c4d69cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,22 @@ export async function loadConfig( | |
| } | ||
| } | ||
|
|
||
| // Admin settings should be able to override existing mcpEnablement settings | ||
| // and extensionEnabled settings. | ||
| // By default, these should be undefined if not set by an admin so as to not | ||
| // override any existing defaults. | ||
| const getAdminBooleanOverride = (envVar: string): boolean | undefined => { | ||
| const value = process.env[envVar]; | ||
| return value !== undefined ? value === 'true' : undefined; | ||
| }; | ||
|
|
||
| const mcpEnabledAdminOverride = getAdminBooleanOverride('MCP_ADMIN_ENABLED'); | ||
| const extensionsEnabledAdminOverride = getAdminBooleanOverride( | ||
| 'EXTENSIONS_ADMIN_ENABLED', | ||
| ); | ||
| const secureModeAdminEnabled = | ||
| getAdminBooleanOverride('SECURE_MODE_ADMIN_ENABLED') === true; | ||
|
|
||
| const configParams: ConfigParameters = { | ||
| sessionId: taskId, | ||
| model: settings.general?.previewFeatures | ||
|
|
@@ -70,9 +86,11 @@ export async function loadConfig( | |
| excludeTools: settings.excludeTools || undefined, | ||
| showMemoryUsage: settings.showMemoryUsage || false, | ||
| approvalMode: | ||
| process.env['GEMINI_YOLO_MODE'] === 'true' | ||
| process.env['GEMINI_YOLO_MODE'] === 'true' && !secureModeAdminEnabled | ||
| ? ApprovalMode.YOLO | ||
| : ApprovalMode.DEFAULT, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there an explicit disableYoloMode setting which we should also set
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its a bit confusing in the current state but both disableYoloMode and secureModeEnabled end up resulting in the same behavior- secure mode enabled corresponds to the admin setting (I wanted to nest all admin settings under the same obj so I didn't reuse disableYoloMode) |
||
| mcpEnabled: mcpEnabledAdminOverride, | ||
| extensionsEnabled: extensionsEnabledAdminOverride, | ||
| mcpServers: settings.mcpServers, | ||
| cwd: workspaceDir, | ||
| telemetry: { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Gemini CLI if YOLO mode is disabled but this is passed anyway we throw a FatalConfigErrro.
Can we just do a logger.warn() here. WE shouldn't fail but we should log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising this point, @adamfweidman. I understand your concern about the
FatalConfigErrorin the Gemini CLI when YOLO mode is unexpectedly disabled. While thea2a-serveritself doesn't throw an error in this scenario, it's indeed a good practice to inform the user if theirGEMINI_YOLO_MODEsetting is being overridden bysecureModeAdminEnabled.To improve clarity and prevent potential confusion for users, I suggest adding a
logger.warn()message whenGEMINI_YOLO_MODEis set totruebut the effectiveapprovalModebecomesDEFAULTdue tosecureModeAdminEnabled. This would provide valuable feedback without causing a fatal error.Here's how the relevant section could be updated:
This change would ensure that users are aware of the override, aligning with the principle of providing clear feedback.
Severity:
medium