Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 15 additions & 2 deletions crates/goose/src/providers/gemini_oauth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ struct SetupData {
}

#[derive(Debug, Clone)]
struct TokenCache {
pub(crate) struct TokenCache {
cache_path: PathBuf,
}

Expand All @@ -153,7 +153,7 @@ fn get_cache_path() -> PathBuf {
}

impl TokenCache {
fn new() -> Self {
pub(crate) fn new() -> Self {
let cache_path = get_cache_path();
if let Some(parent) = cache_path.parent() {
let _ = std::fs::create_dir_all(parent);
Expand All @@ -167,6 +167,10 @@ impl TokenCache {
.and_then(|contents| serde_json::from_str(&contents).ok())
}

pub(crate) fn has_token(&self) -> bool {
self.load().is_some()
}

fn save(&self, data: &SetupData) -> Result<()> {
if let Some(parent) = self.cache_path.parent() {
std::fs::create_dir_all(parent)?;
Expand Down Expand Up @@ -1096,8 +1100,15 @@ mod tests {
}

#[test]
#[serial_test::serial]
fn test_token_cache_roundtrip() {
let root = tempfile::tempdir().unwrap();
let root_path = root.path().to_string_lossy().to_string();
let _guard = env_lock::lock_env([("GOOSE_PATH_ROOT", Some(root_path.as_str()))]);

let cache = TokenCache::new();
cache.clear();
assert!(!cache.has_token());
let data = SetupData {
project_id: "test-project".to_string(),
token: TokenData {
Expand All @@ -1111,7 +1122,9 @@ mod tests {
assert_eq!(loaded.project_id, "test-project");
assert_eq!(loaded.token.access_token, "test-access");
assert_eq!(loaded.token.refresh_token, "test-refresh");
assert!(cache.has_token());
cache.clear();
assert!(cache.load().is_none());
assert!(!cache.has_token());
}
}
5 changes: 4 additions & 1 deletion crates/goose/src/providers/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,10 @@ async fn init_registry() -> RwLock<ProviderRegistry> {
);
registry.register::<GcpVertexAIProvider>(false);
registry.register::<GeminiCliProvider>(false);
registry.register::<GeminiOAuthProvider>(true);
registry.register_with_inventory::<GeminiOAuthProvider>(
true,
Some(registrations::gemini_oauth_inventory()),
);
registry.register::<GithubCopilotProvider>(false);
registry.register_with_inventory::<GoogleProviderDef>(
true,
Expand Down
50 changes: 50 additions & 0 deletions crates/goose/src/providers/inventory/registrations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::providers::claude_acp::{CLAUDE_ACP_BINARY, CLAUDE_ACP_PROVIDER_NAME};
use crate::providers::codex_acp::CODEX_ACP_PROVIDER_NAME;
use crate::providers::copilot_acp::{COPILOT_ACP_BINARY, COPILOT_ACP_PROVIDER_NAME};
use crate::providers::formats::anthropic::ANTHROPIC_PROVIDER_NAME;
use crate::providers::gemini_oauth::TokenCache as GeminiOAuthTokenCache;
use crate::providers::google::{GOOGLE_API_HOST, GOOGLE_PROVIDER_NAME};
use crate::providers::huggingface::HuggingFaceProvider;
use crate::providers::huggingface_auth;
Expand Down Expand Up @@ -155,6 +156,15 @@ pub fn chatgpt_codex_inventory() -> InventoryRegistration {
.with_configured(|| ChatGptCodexTokenCache::new().has_token())
}

pub fn gemini_oauth_inventory() -> InventoryRegistration {
InventoryRegistration {
supports_refresh: false,
identity: default_inventory_identity_resolver(),
configured: None,
}
.with_configured(|| GeminiOAuthTokenCache::new().has_token())
}

pub fn xai_oauth_inventory() -> InventoryRegistration {
InventoryRegistration {
supports_refresh: false,
Expand Down Expand Up @@ -196,3 +206,43 @@ pub fn copilot_acp_inventory() -> InventoryRegistration {
pub fn pi_acp_inventory() -> InventoryRegistration {
acp_inventory(PI_ACP_PROVIDER_NAME, PI_ACP_BINARY, false)
}

#[cfg(test)]
mod tests {
use super::*;
use crate::config::paths::Paths;
use chrono::Utc;

#[test]
#[serial_test::serial]
fn gemini_oauth_inventory_configured_uses_token_cache() {
let root = tempfile::tempdir().unwrap();
let root_path = root.path().to_string_lossy().to_string();
let _guard = env_lock::lock_env([("GOOSE_PATH_ROOT", Some(root_path.as_str()))]);

let registration = gemini_oauth_inventory();
let configured = registration
.configured
.expect("Gemini OAuth should define configured resolver");

assert!(!configured());

let cache_path = Paths::in_config_dir("gemini_oauth/tokens.json");
std::fs::create_dir_all(cache_path.parent().unwrap()).unwrap();
std::fs::write(
cache_path,
serde_json::to_string(&serde_json::json!({
"project_id": "test-project",
"token": {
"access_token": "access",
"refresh_token": "refresh",
"expires_at": (Utc::now() + chrono::Duration::hours(1)).to_rfc3339(),
},
}))
.unwrap(),
)
.unwrap();

assert!(configured());
}
}
51 changes: 41 additions & 10 deletions ui/desktop/src/components/onboarding/LocalModelPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
} from '../../acp/local-inference';
import { trackOnboardingSetupFailed } from '../../utils/analytics';
import { defineMessages, useIntl } from '../../i18n';
import { errorMessage as formatErrorMessage } from '../../utils/conversionUtils';

const i18n = defineMessages({
checkingModels: {
Expand Down Expand Up @@ -61,7 +62,8 @@ const i18n = defineMessages({
},
localModelsNote: {
id: 'localModelPicker.localModelsNote',
defaultMessage: 'Local models keep everything on your machine for full privacy. Performance and context window size may vary compared to cloud providers depending on your hardware and model size.',
defaultMessage:
'Local models keep everything on your machine for full privacy. Performance and context window size may vary compared to cloud providers depending on your hardware and model size.',
},
failedToLoad: {
id: 'localModelPicker.failedToLoad',
Expand All @@ -82,7 +84,7 @@ const i18n = defineMessages({
});

interface LocalModelPickerProps {
onConfigured: (providerName: string, modelId: string) => void;
onConfigured: (providerName: string, modelId: string) => void | Promise<void>;
}

const formatBytes = (bytes: number): string => {
Expand Down Expand Up @@ -146,8 +148,15 @@ export default function LocalModelPicker({ onConfigured }: LocalModelPickerProps
load();
}, [intl]);

const finishSetup = (modelId: string) => {
onConfigured(LOCAL_PROVIDER, modelId);
const finishSetup = async (modelId: string) => {
try {
await onConfigured(LOCAL_PROVIDER, modelId);
} catch (error) {
console.error('Failed to finish local model setup:', error);
setErrorMessage(formatErrorMessage(error));
trackOnboardingSetupFailed(LOCAL_PROVIDER, 'save_defaults_failed');
setPhase('error');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep downloaded models selectable after setup errors

When onConfigured rejects after a newly downloaded model completes, this error path leaves models with the pre-download status. The Try Again button only switches back to select, so handlePrimaryAction still sees the selected model as not downloaded and starts another download instead of retrying the defaults save; this affects transient save failures or races where the backend has not registered the just-downloaded local model yet.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
};

const startDownload = async (modelId: string) => {
Expand Down Expand Up @@ -186,7 +195,23 @@ export default function LocalModelPicker({ onConfigured }: LocalModelPickerProps
setDownloadProgress(progress);
if (progress.status === 'completed') {
cleanup();
finishSetup(modelId);
setModels((previousModels) =>
previousModels.map((model) =>
model.id === modelId
? {
...model,
status: {
...model.status,
state: 'Downloaded',
progressPercent: 100,
bytesDownloaded: model.sizeBytes,
totalBytes: model.sizeBytes,
},
}
: model
)
);
await finishSetup(modelId);
} else if (progress.status === 'failed') {
cleanup();
setErrorMessage(progress.error || 'Download failed.');
Expand Down Expand Up @@ -223,7 +248,7 @@ export default function LocalModelPicker({ onConfigured }: LocalModelPickerProps
const model = models.find((m) => m.id === selectedModelId);
if (!model) return;
if (model.status.state === 'Downloaded') {
finishSetup(model.id);
await finishSetup(model.id);
} else {
await startDownload(model.id);
}
Expand Down Expand Up @@ -310,7 +335,9 @@ export default function LocalModelPicker({ onConfigured }: LocalModelPickerProps
onClick={() => setShowAllModels(!showAllModels)}
className="text-sm text-blue-500 hover:text-blue-400 transition-colors flex items-center gap-1"
>
{showAllModels ? intl.formatMessage(i18n.hideOtherSizes) : intl.formatMessage(i18n.showOtherSizes, { count: otherModels.length })}
{showAllModels
? intl.formatMessage(i18n.hideOtherSizes)
: intl.formatMessage(i18n.showOtherSizes, { count: otherModels.length })}
<svg
className={`w-3.5 h-3.5 transition-transform ${showAllModels ? 'rotate-180' : ''}`}
fill="none"
Expand Down Expand Up @@ -376,10 +403,12 @@ export default function LocalModelPicker({ onConfigured }: LocalModelPickerProps
{selectedModel?.status.state === 'Downloaded'
? intl.formatMessage(i18n.useModel, { modelId: selectedModel.id })
: selectedModel
? intl.formatMessage(i18n.downloadModel, { modelId: selectedModel.id, size: formatSize(selectedModel.sizeBytes) })
? intl.formatMessage(i18n.downloadModel, {
modelId: selectedModel.id,
size: formatSize(selectedModel.sizeBytes),
})
: intl.formatMessage(i18n.selectModel)}
</button>

</div>
)}

Expand Down Expand Up @@ -427,7 +456,9 @@ export default function LocalModelPicker({ onConfigured }: LocalModelPickerProps
) : (
<div className="flex items-center gap-3">
<div className="animate-spin rounded-full h-4 w-4 border-t-2 border-b-2 border-text-muted"></div>
<span className="text-sm text-text-muted">{intl.formatMessage(i18n.startingDownload)}</span>
<span className="text-sm text-text-muted">
{intl.formatMessage(i18n.startingDownload)}
</span>
</div>
)}
</div>
Expand Down
23 changes: 10 additions & 13 deletions ui/desktop/src/components/onboarding/ProviderConfigForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import { SecureStorageNotice } from '../settings/providers/modal/subcomponents/S
import { Button } from '../ui/button';
import { LogIn, ChevronRight } from 'lucide-react';
import { defineMessages, useIntl } from '../../i18n';
import { errorMessage } from '../../utils/conversionUtils';

type OnConfigured = (name: string) => void | Promise<void>;

const i18n = defineMessages({
browserWindowOpen: {
Expand Down Expand Up @@ -69,7 +72,7 @@ function OAuthForm({
onError,
}: {
provider: ProviderDetails;
onConfigured: (name: string) => void;
onConfigured: OnConfigured;
onError: (msg: string) => void;
}) {
const intl = useIntl();
Expand All @@ -79,9 +82,9 @@ function OAuthForm({
setIsLoading(true);
try {
await acpAuthenticateProvider(provider.name);
onConfigured(provider.name);
await onConfigured(provider.name);
} catch (err) {
onError(`Sign-in failed: ${err instanceof Error ? err.message : String(err)}`);
onError(`Setup failed: ${errorMessage(err)}`);
} finally {
setIsLoading(false);
}
Expand Down Expand Up @@ -117,7 +120,7 @@ function ApiKeyForm({
onError,
}: {
provider: ProviderDetails;
onConfigured: (name: string) => void;
onConfigured: OnConfigured;
onError: (msg: string) => void;
}) {
const intl = useIntl();
Expand Down Expand Up @@ -157,15 +160,9 @@ function ApiKeyForm({
setIsSubmitting(true);
try {
await providerConfigSubmitHandler(provider, toSubmit);
onConfigured(provider.name);
await onConfigured(provider.name);
} catch (err) {
const msg =
err instanceof Error
? err.message
: typeof err === 'object' && err !== null && 'message' in err
? String((err as Record<string, unknown>).message)
: JSON.stringify(err);
onError(msg);
onError(errorMessage(err));
} finally {
setIsSubmitting(false);
}
Expand Down Expand Up @@ -214,7 +211,7 @@ function ApiKeyForm({

interface ProviderConfigFormProps {
provider: ProviderDetails;
onConfigured: (providerName: string) => void;
onConfigured: OnConfigured;
}

export default function ProviderConfigForm({ provider, onConfigured }: ProviderConfigFormProps) {
Expand Down
4 changes: 2 additions & 2 deletions ui/desktop/src/components/onboarding/ProviderSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ interface ProviderOption {
}

interface ProviderSelectorProps {
onConfigured: (providerName: string, modelId?: string) => void;
onConfigured: (providerName: string, modelId?: string) => void | Promise<void>;
onFirstSelection?: () => void;
}

Expand Down Expand Up @@ -123,7 +123,7 @@ export default function ProviderSelector({
const result = await acpCreateCustomProviderFromRequest(data);
setShowCustomModal(false);
if (result.provider_name) {
onConfigured(result.provider_name);
await onConfigured(result.provider_name);
}
};

Expand Down
Loading