Skip to content

allow saving when a provider has no config #3396

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.

Sign up for GitHub

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

Closed
michaelneale wants to merge 1 commit into from
Closed

allow saving when a provider has no config #3396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 110 additions & 0 deletions crates/goose-server/src/routes/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ pub fn check_provider_configured(metadata: &ProviderMetadata) -> bool {
.filter(|key| key.required)
.collect();

// If there are no required keys at all, the provider is considered configured
if required_keys.is_empty() {
return true;
}

// Special case: If a provider has exactly one required key and that key
// has a default value, check if it's explicitly set
if required_keys.len() == 1 && required_keys[0].default.is_some() {
Expand Down Expand Up @@ -154,3 +159,108 @@ pub fn check_provider_configured(metadata: &ProviderMetadata) -> bool {
is_set_in_env || is_set_in_config
})
}

#[cfg(test)]
mod tests {
use super::*;
use goose::providers::base::{ConfigKey, Provider, ProviderMetadata};

#[test]
fn test_check_provider_configured_no_required_keys() {
// Test provider with no required keys (like gemini-cli)
let metadata = ProviderMetadata::new(
"test-provider",
"Test Provider",
"A test provider with no required keys",
"default",
vec!["default"],
"https://example.com",
vec![], // No config keys at all
);

assert!(check_provider_configured(&metadata));
}

#[test]
fn test_check_provider_configured_optional_key_with_default() {
// Test provider with one optional key that has a default (like claude-code)
let metadata = ProviderMetadata::new(
"test-provider",
"Test Provider",
"A test provider with optional key with default",
"default",
vec!["default"],
"https://example.com",
vec![ConfigKey::new(
"TEST_COMMAND",
false,
false,
Some("default_cmd"),
)],
);

// Should be considered configured because it has no required keys
assert!(check_provider_configured(&metadata));
}

#[test]
fn test_check_provider_configured_required_key_with_default_not_set() {
// Test provider with one required key that has a default but is not set
let metadata = ProviderMetadata::new(
"test-provider",
"Test Provider",
"A test provider with required key with default",
"default",
vec!["default"],
"https://example.com",
vec![ConfigKey::new(
"TEST_COMMAND",
true,
false,
Some("default_cmd"),
)], // required=true, secret=false
);

// Should NOT be considered configured because the required key is not explicitly set
// even though it has a default value
assert!(!check_provider_configured(&metadata));
}

#[test]
fn test_check_provider_configured_required_key_without_default() {
// Test provider with required key without default
let metadata = ProviderMetadata::new(
"test-provider",
"Test Provider",
"A test provider with required key without default",
"default",
vec!["default"],
"https://example.com",
vec![ConfigKey::new("TEST_API_KEY", true, true, None)], // required=true, secret=true
);

// Should NOT be considered configured
assert!(!check_provider_configured(&metadata));
}

#[test]
fn test_check_provider_configured_real_gemini_cli() {
// Test the actual gemini-cli provider metadata
use goose::providers::gemini_cli::GeminiCliProvider;
let metadata = GeminiCliProvider::metadata();

// Should be considered configured because it has no required keys
assert!(check_provider_configured(&metadata));
}

#[test]
fn test_check_provider_configured_real_claude_code() {
// Test the actual claude-code provider metadata
use goose::providers::claude_code::ClaudeCodeProvider;
let metadata = ClaudeCodeProvider::metadata();

// Should be considered configured because it has no required keys
// (CLAUDE_CODE_COMMAND is optional with a default)
assert!(check_provider_configured(&metadata));
}
}
7 changes: 5 additions & 2 deletions ui/desktop/src/components/settings/providers/ProviderGrid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,13 @@ const ProviderCards = memo(function ProviderCards({
refreshProviders();
}
},
formProps: {},
formProps: {
isOnboarding,
onProviderLaunch,
},
});
},
[openModal, refreshProviders]
[openModal, refreshProviders, isOnboarding, onProviderLaunch]
);

const deleteProviderConfigViaModal = useCallback(
Expand Down
11 changes: 11 additions & 0 deletions ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,17 @@ export default function ProviderConfigurationModal() {
// Wait for the submission to complete
await SubmitHandler(upsert, currentProvider, configValues);

// Check if this provider has no required configuration and we're in onboarding mode
const hasNoRequiredConfig = parameters.filter(p => p.required).length === 0;
const isOnboardingMode = modalProps.formProps?.isOnboarding;
const onProviderLaunch = modalProps.formProps?.onProviderLaunch;

// If it's onboarding mode, has no required config, and we have a launch function, launch the provider
if (isOnboardingMode && hasNoRequiredConfig && onProviderLaunch) {
console.log('Launching provider after configuration:', currentProvider.name);
await onProviderLaunch(currentProvider);
}

// Close the modal before triggering refreshes to avoid UI issues
closeModal();

Expand Down
4 changes: 4 additions & 0 deletions .../src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@ interface DefaultProviderSetupFormProps {
setConfigValues: React.Dispatch<React.SetStateAction<Record<string, string>>>;
provider: ProviderDetails;
validationErrors: ValidationErrors;
isOnboarding?: boolean;
onProviderLaunch?: (provider: ProviderDetails) => void;
}

export default function DefaultProviderSetupForm({
configValues,
setConfigValues,
provider,
validationErrors = {},
isOnboarding,
onProviderLaunch,
}: DefaultProviderSetupFormProps) {
const parameters = useMemo(
() => provider.metadata.config_keys || [],
Expand Down
12 changes: 12 additions & 0 deletions ...p/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,18 @@ export const DefaultSubmitHandler = async (
}
);

// For providers with no required configuration, save optional parameters with defaults
// This ensures something gets saved to mark the provider as configured
if (parameters.length > 0 && parameters.every(p => !p.required)) {
parameters.forEach((parameter) => {
if (parameter.default !== undefined && parameter.default !== null) {
const configKey = `${parameter.name}`;
const isSecret = parameter.secret === true;
upsertPromises.push(upsertFn(configKey, parameter.default, isSecret));
}
});
}

// Wait for all upsert operations to complete
return Promise.all(upsertPromises);
};
Loading