-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: exclude undefined max token fields from settings export #7946
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 |
|---|---|---|
|
|
@@ -471,7 +471,21 @@ export class ProviderSettingsManager { | |
| const configs = profiles.apiConfigs | ||
| for (const name in configs) { | ||
| // Avoid leaking properties from other providers. | ||
| configs[name] = discriminatedProviderSettingsWithIdSchema.parse(configs[name]) | ||
| let config = discriminatedProviderSettingsWithIdSchema.parse(configs[name]) | ||
|
|
||
| // Remove max token fields if they are undefined | ||
| // These fields should only be included for models that support reasoning budgets | ||
| // and when the user has explicitly set values | ||
| // Use type assertion to access these optional fields | ||
| const configAny = config as any | ||
| if (configAny.modelMaxTokens === undefined) { | ||
| delete configAny.modelMaxTokens | ||
| } | ||
| if (configAny.modelMaxThinkingTokens === undefined) { | ||
| delete configAny.modelMaxThinkingTokens | ||
|
Contributor
Author
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. Could we extract this logic into a helper function to avoid duplication and improve maintainability? For example: function removeUndefinedTokenFields(config: any): void {
if (config.modelMaxTokens === undefined) {
delete config.modelMaxTokens
}
if (config.modelMaxThinkingTokens === undefined) {
delete config.modelMaxThinkingTokens
}
}Then just call |
||
| } | ||
|
|
||
| configs[name] = config | ||
| } | ||
| return profiles | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1608,5 +1608,76 @@ describe("importExport", () => { | |
| "https://custom-api.example.com/v1", | ||
| ) | ||
| }) | ||
|
|
||
| it("should exclude undefined modelMaxTokens and modelMaxThinkingTokens from export", async () => { | ||
| // This test verifies that undefined max token fields are not included in the export | ||
| // to prevent them from appearing for models that don't support reasoning budgets | ||
|
|
||
| ;(vscode.window.showSaveDialog as Mock).mockResolvedValue({ | ||
| fsPath: "/mock/path/roo-code-settings.json", | ||
| }) | ||
|
|
||
| const mockProviderProfiles = { | ||
| currentApiConfigName: "test-provider", | ||
| apiConfigs: { | ||
| "test-provider": { | ||
| apiProvider: "openai" as ProviderName, | ||
| id: "test-id", | ||
| // modelMaxTokens and modelMaxThinkingTokens are undefined | ||
| apiKey: "test-key", | ||
| openAiBaseUrl: "https://api.openai.com/v1", | ||
| }, | ||
| "anthropic-provider": { | ||
| apiProvider: "anthropic" as ProviderName, | ||
| id: "anthropic-id", | ||
| apiKey: "anthropic-key", | ||
| modelMaxTokens: 4096, // This one has a value set | ||
| // modelMaxThinkingTokens is undefined | ||
| }, | ||
| "reasoning-provider": { | ||
| apiProvider: "openrouter" as ProviderName, | ||
| id: "reasoning-id", | ||
| openRouterApiKey: "reasoning-key", | ||
| modelMaxTokens: 8192, | ||
| modelMaxThinkingTokens: 4096, // Both values set for reasoning model | ||
| }, | ||
| }, | ||
| modeApiConfigs: {}, | ||
| } | ||
|
|
||
| const mockGlobalSettings = { | ||
| mode: "code", | ||
| autoApprovalEnabled: true, | ||
| } | ||
|
|
||
| mockProviderSettingsManager.export.mockResolvedValue(mockProviderProfiles) | ||
| mockContextProxy.export.mockResolvedValue(mockGlobalSettings) | ||
| ;(fs.mkdir as Mock).mockResolvedValue(undefined) | ||
|
|
||
| await exportSettings({ | ||
| providerSettingsManager: mockProviderSettingsManager, | ||
| contextProxy: mockContextProxy, | ||
| }) | ||
|
|
||
| // Get the exported data | ||
| const exportedData = (safeWriteJson as Mock).mock.calls[0][1] | ||
|
|
||
| // Verify that undefined fields are not included in the export | ||
| const testProvider = exportedData.providerProfiles.apiConfigs["test-provider"] | ||
| expect(testProvider.apiKey).toBe("test-key") | ||
| expect(testProvider.openAiBaseUrl).toBe("https://api.openai.com/v1") | ||
| expect("modelMaxTokens" in testProvider).toBe(false) // Should not be present | ||
| expect("modelMaxThinkingTokens" in testProvider).toBe(false) // Should not be present | ||
|
|
||
| // Verify that defined modelMaxTokens is included but undefined modelMaxThinkingTokens is not | ||
| const anthropicProvider = exportedData.providerProfiles.apiConfigs["anthropic-provider"] | ||
| expect(anthropicProvider.modelMaxTokens).toBe(4096) // Should be present with value | ||
| expect("modelMaxThinkingTokens" in anthropicProvider).toBe(false) // Should not be present | ||
|
|
||
| // Verify that both fields are included when they have values | ||
| const reasoningProvider = exportedData.providerProfiles.apiConfigs["reasoning-provider"] | ||
| expect(reasoningProvider.modelMaxTokens).toBe(8192) // Should be present with value | ||
| expect(reasoningProvider.modelMaxThinkingTokens).toBe(4096) // Should be present with value | ||
| }) | ||
|
Contributor
Author
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. Excellent test coverage! The test thoroughly covers all scenarios: completely undefined fields, partially defined fields, and fully defined fields. This gives confidence that the fix works correctly. |
||
| }) | ||
| }) | ||
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.
The use of
as anyhere works, but have you considered a more type-safe approach? We could check if these properties exist in the discriminated union type or use a type guard. Something like: