-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: add organization default provider settings support #6221
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
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| import { describe, it, expect } from "vitest" | ||
| import { organizationSettingsSchema } from "../cloud.js" | ||
|
|
||
| describe("organizationSettingsSchema", () => { | ||
| it("should accept valid organization settings with defaultProviderSettings", () => { | ||
| const validSettings = { | ||
| version: 1, | ||
| defaultSettings: {}, | ||
| allowList: { | ||
| allowAll: false, | ||
| providers: { | ||
| anthropic: { | ||
| allowAll: true, | ||
| models: [], | ||
| }, | ||
| }, | ||
| }, | ||
| defaultProviderSettings: { | ||
| anthropic: { | ||
| apiProvider: "anthropic" as const, | ||
| apiKey: "test-key", | ||
| apiModelId: "claude-3-5-sonnet-20241022", | ||
| }, | ||
| openai: { | ||
| apiProvider: "openai" as const, | ||
| openAiApiKey: "test-key", | ||
| openAiModelId: "gpt-4", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| const result = organizationSettingsSchema.safeParse(validSettings) | ||
| expect(result.success).toBe(true) | ||
| if (result.success) { | ||
| expect(result.data.defaultProviderSettings).toEqual(validSettings.defaultProviderSettings) | ||
| } | ||
| }) | ||
|
|
||
| it("should accept organization settings without defaultProviderSettings", () => { | ||
| const validSettings = { | ||
| version: 1, | ||
| defaultSettings: {}, | ||
| allowList: { | ||
| allowAll: true, | ||
| providers: {}, | ||
| }, | ||
| } | ||
|
|
||
| const result = organizationSettingsSchema.safeParse(validSettings) | ||
| expect(result.success).toBe(true) | ||
| if (result.success) { | ||
| expect(result.data.defaultProviderSettings).toBeUndefined() | ||
| } | ||
| }) | ||
|
|
||
| it("should reject invalid provider names in defaultProviderSettings", () => { | ||
| const invalidSettings = { | ||
| version: 1, | ||
| defaultSettings: {}, | ||
| allowList: { | ||
| allowAll: true, | ||
| providers: {}, | ||
| }, | ||
| defaultProviderSettings: { | ||
| "invalid-provider": { | ||
| apiProvider: "invalid-provider", | ||
| apiKey: "test-key", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| const result = organizationSettingsSchema.safeParse(invalidSettings) | ||
| expect(result.success).toBe(false) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest" | ||
| import * as vscode from "vscode" | ||
| import { CloudService } from "@roo-code/cloud" | ||
| import { webviewMessageHandler } from "../webviewMessageHandler" | ||
| import { ClineProvider } from "../ClineProvider" | ||
| import { ProviderSettings } from "@roo-code/types" | ||
|
|
||
| // Mock CloudService | ||
| vi.mock("@roo-code/cloud", () => ({ | ||
| CloudService: { | ||
| instance: { | ||
| getOrganizationSettings: vi.fn(), | ||
| }, | ||
| }, | ||
| })) | ||
|
|
||
| describe("webviewMessageHandler - Organization Defaults", () => { | ||
| let mockProvider: any | ||
| let mockMarketplaceManager: any | ||
|
|
||
| beforeEach(() => { | ||
| // Reset mocks | ||
| vi.clearAllMocks() | ||
|
|
||
| // Create mock provider | ||
| mockProvider = { | ||
| log: vi.fn(), | ||
| upsertProviderProfile: vi.fn(), | ||
| postMessageToWebview: vi.fn(), | ||
| getState: vi.fn().mockResolvedValue({ | ||
| apiConfiguration: {}, | ||
| currentApiConfigName: "test-config", | ||
| }), | ||
| } | ||
|
|
||
| // Create mock marketplace manager | ||
| mockMarketplaceManager = {} | ||
| }) | ||
|
|
||
| it("should apply organization default settings when creating a new profile", async () => { | ||
| // Mock organization settings with defaults | ||
| const orgDefaults = { | ||
| anthropic: { | ||
| apiProvider: "anthropic" as const, | ||
| anthropicApiKey: "org-default-key", | ||
| apiModelId: "claude-3-opus-20240229", | ||
| temperature: 0.7, | ||
| }, | ||
| } | ||
|
|
||
| vi.mocked(CloudService.instance.getOrganizationSettings).mockResolvedValue({ | ||
| version: 1, | ||
| defaultSettings: {}, | ||
| allowList: { allowAll: true, providers: {} }, | ||
| defaultProviderSettings: orgDefaults, | ||
| }) | ||
|
|
||
| // Send upsertApiConfiguration message | ||
| const message = { | ||
| type: "upsertApiConfiguration" as const, | ||
| text: "new-profile", | ||
| apiConfiguration: { | ||
| apiProvider: "anthropic", | ||
| anthropicApiKey: "user-key", // User-provided key should take precedence | ||
| // temperature is not provided, so org default should be used | ||
| } as ProviderSettings, | ||
| } | ||
|
|
||
| await webviewMessageHandler(mockProvider, message, mockMarketplaceManager) | ||
|
|
||
| // Verify that upsertProviderProfile was called with merged settings | ||
| expect(mockProvider.upsertProviderProfile).toHaveBeenCalledWith("new-profile", { | ||
| apiProvider: "anthropic", | ||
| anthropicApiKey: "user-key", // User value takes precedence | ||
| apiModelId: "claude-3-opus-20240229", // From org defaults | ||
| temperature: 0.7, // From org defaults | ||
| }) | ||
| }) | ||
|
|
||
| it("should handle missing organization settings gracefully", async () => { | ||
| // Mock CloudService to throw an error | ||
| vi.mocked(CloudService.instance.getOrganizationSettings).mockRejectedValue(new Error("Not authenticated")) | ||
|
|
||
| // Send upsertApiConfiguration message | ||
| const message = { | ||
| type: "upsertApiConfiguration" as const, | ||
| text: "new-profile", | ||
| apiConfiguration: { | ||
| apiProvider: "anthropic", | ||
| anthropicApiKey: "user-key", | ||
| } as ProviderSettings, | ||
| } | ||
|
|
||
| await webviewMessageHandler(mockProvider, message, mockMarketplaceManager) | ||
|
|
||
| // Verify that error was logged | ||
| expect(mockProvider.log).toHaveBeenCalledWith(expect.stringContaining("Failed to get organization defaults")) | ||
|
|
||
| // Verify that upsertProviderProfile was still called with original settings | ||
| expect(mockProvider.upsertProviderProfile).toHaveBeenCalledWith("new-profile", { | ||
| apiProvider: "anthropic", | ||
| anthropicApiKey: "user-key", | ||
| }) | ||
| }) | ||
|
|
||
| it("should not apply defaults for a different provider", async () => { | ||
| // Mock organization settings with defaults for anthropic | ||
| const orgDefaults = { | ||
| anthropic: { | ||
| apiProvider: "anthropic" as const, | ||
| anthropicApiKey: "org-default-key", | ||
| apiModelId: "claude-3-opus-20240229", | ||
| }, | ||
| } | ||
|
|
||
| vi.mocked(CloudService.instance.getOrganizationSettings).mockResolvedValue({ | ||
| version: 1, | ||
| defaultSettings: {}, | ||
| allowList: { allowAll: true, providers: {} }, | ||
| defaultProviderSettings: orgDefaults, | ||
| }) | ||
|
|
||
| // Send upsertApiConfiguration message for openai provider | ||
| const message = { | ||
| type: "upsertApiConfiguration" as const, | ||
| text: "new-profile", | ||
| apiConfiguration: { | ||
| apiProvider: "openai", | ||
| openAiApiKey: "user-key", | ||
| } as ProviderSettings, | ||
| } | ||
|
|
||
| await webviewMessageHandler(mockProvider, message, mockMarketplaceManager) | ||
|
|
||
| // Verify that only the user-provided settings were used | ||
| expect(mockProvider.upsertProviderProfile).toHaveBeenCalledWith("new-profile", { | ||
| apiProvider: "openai", | ||
| openAiApiKey: "user-key", | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -106,7 +106,7 @@ const ApiOptions = ({ | |||||
| setErrorMessage, | ||||||
| }: ApiOptionsProps) => { | ||||||
| const { t } = useAppTranslation() | ||||||
| const { organizationAllowList } = useExtensionState() | ||||||
| const { organizationAllowList, organizationDefaultProviderSettings } = useExtensionState() | ||||||
|
|
||||||
| const [customHeaders, setCustomHeaders] = useState<[string, string][]>(() => { | ||||||
| const headers = apiConfiguration?.openAiHeaders || {} | ||||||
|
|
@@ -246,6 +246,23 @@ const ApiOptions = ({ | |||||
| (value: ProviderName) => { | ||||||
| setApiConfigurationField("apiProvider", value) | ||||||
|
|
||||||
| // Apply organization default settings if available | ||||||
| const orgDefaults = organizationDefaultProviderSettings?.[value] | ||||||
|
|
||||||
| if (orgDefaults) { | ||||||
| // Apply each default setting from the organization | ||||||
| Object.entries(orgDefaults).forEach(([key, defaultValue]) => { | ||||||
|
Member
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. There's a potential race condition here. When the provider changes, defaults are applied immediately in the same function, but the provider change might trigger other async operations (like fetching available models). Consider using a useEffect(() => {
const orgDefaults = organizationDefaultProviderSettings?.[apiConfiguration.apiProvider];
if (orgDefaults && isNewProfile) { // You'd need to track if this is a new profile
// Apply defaults here
}
}, [apiConfiguration.apiProvider, organizationDefaultProviderSettings]);This would ensure defaults are applied at the right time in the component lifecycle. |
||||||
| // Skip apiProvider as we've already set it | ||||||
| if (key === "apiProvider") return | ||||||
|
|
||||||
| // Only apply defaults if the current value is undefined or empty | ||||||
| const currentValue = apiConfiguration[key as keyof ProviderSettings] | ||||||
| if (!currentValue || (typeof currentValue === "string" && currentValue.trim() === "")) { | ||||||
|
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. Avoid using '!currentValue' to check if a field is empty. This check will override valid falsy values (like 0 or false). Instead, explicitly check for undefined or null (e.g.,
Suggested change
Member
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. This falsy value check will incorrectly override valid falsy values. For example, if an organization sets Consider using a more explicit check: if (currentValue === undefined || currentValue === null || (typeof currentValue === "string" && currentValue.trim() === "")) {This issue was already raised by ellipsis-dev[bot] but hasn't been addressed yet. |
||||||
| setApiConfigurationField(key as keyof ProviderSettings, defaultValue) | ||||||
| } | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| // It would be much easier to have a single attribute that stores | ||||||
| // the modelId, but we have a separate attribute for each of | ||||||
| // OpenRouter, Glama, Unbound, and Requesty. | ||||||
|
|
@@ -311,7 +328,7 @@ const ApiOptions = ({ | |||||
| ) | ||||||
| } | ||||||
| }, | ||||||
| [setApiConfigurationField, apiConfiguration], | ||||||
| [setApiConfigurationField, apiConfiguration, organizationDefaultProviderSettings], | ||||||
| ) | ||||||
|
|
||||||
| const modelValidationError = useMemo(() => { | ||||||
|
|
||||||
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.
When
getOrganizationSettings()fails, the error is logged butorganizationDefaultProviderSettingsremains an empty object. This silent failure might be confusing for users who expect defaults to be applied.Consider:
This would help users understand why their expected defaults aren't being applied.