diff --git a/src/app/shared/flow-assistant/flow-assistant.spec.ts b/src/app/shared/flow-assistant/flow-assistant.spec.ts index 43f901b..28992da 100644 --- a/src/app/shared/flow-assistant/flow-assistant.spec.ts +++ b/src/app/shared/flow-assistant/flow-assistant.spec.ts @@ -6,6 +6,7 @@ import { TestBed } from '@angular/core/testing'; import { AssistantService } from '@services/assistant/assistant'; import { Authorization } from '@services/authorization/authorization'; import { NodeSettingsDialogService } from '@services/dialogs/node-settings-dialog'; +import { LlmProviderService } from '@services/llm-provider/llm-provider'; import { VaultService } from '@services/vault/vault'; import { AssistantSessionStore } from '@stores/assistant-session-store'; import { EditorStateHolder } from '@stores/flow-editor'; @@ -134,6 +135,7 @@ describe('FlowAssistant credentials', () => { function build(options: { dialogResult?: Record | null; credentials?: unknown[]; + llmProviderCapabilities?: unknown[]; } = {}) { const assistant = { getConfig: vi.fn(() => of({ @@ -154,6 +156,12 @@ describe('FlowAssistant credentials', () => { createSecret: vi.fn(() => of({ id: 'cred-1', label: 'Key', provider: 'Gemini', active: true })), updateSecret: vi.fn(() => of({ id: 'cred-1', label: 'Key', provider: 'Gemini', active: true })) }; + const llmProviderService = { + listCapabilities: vi.fn(() => of(options.llmProviderCapabilities ?? [ + { name: 'InternalOllama', requiresCredential: false, requiresEndpoint: false }, + { name: 'Gemini', requiresCredential: true, requiresEndpoint: false } + ])) + }; const settingsDialog = { open: vi.fn((_input: unknown) => Promise.resolve(options.dialogResult ?? null)) }; TestBed.configureTestingModule({ @@ -161,6 +169,7 @@ describe('FlowAssistant credentials', () => { { provide: AssistantService, useValue: assistant }, { provide: NodeSettingsDialogService, useValue: settingsDialog }, { provide: VaultService, useValue: vault }, + { provide: LlmProviderService, useValue: llmProviderService }, { provide: EditorStateHolder, useValue: { currentFlow: vi.fn(() => null), activeFlowData: vi.fn(() => null) } @@ -181,7 +190,7 @@ describe('FlowAssistant credentials', () => { const component = TestBed.createComponent(FlowAssistant).componentInstance as any; component.providers.set(['InternalOllama', 'Gemini']); - return { component, vault, settingsDialog }; + return { component, vault, settingsDialog, llmProviderService }; } afterEach(() => TestBed.resetTestingModule()); @@ -262,6 +271,108 @@ describe('FlowAssistant credentials', () => { .find((field: any) => field.key === 'provider'); expect(provider.options).toEqual([{ label: 'Gemini', value: 'Gemini' }]); }); + + it('does not show an endpoint field for a provider that does not need one', async () => { + const { component, settingsDialog } = build(); + + await component.openCredentialForm('Gemini'); + + const dialog = settingsDialog.open.mock.calls[0][0] as any; + expect(dialog.fields.map((field: any) => field.key)).toEqual(['label', 'provider', 'description', 'value']); + }); + + it('does not require a credential for a provider the catalog says needs none, whatever it is named', async () => { + // The bug this replaces: only "InternalOllama" by name was exempt, so a credential-free + // provider under any other name - a remote Ollama with no key, say - was told it needed one. + const { component } = build({ + llmProviderCapabilities: [ + { name: 'RemoteOllamaOnMyLan', requiresCredential: false, requiresEndpoint: false } + ] + }); + component.providers.set(['RemoteOllamaOnMyLan']); + component.selectProvider('RemoteOllamaOnMyLan'); + await component.ensureLlmProviderCapabilitiesLoaded(); + + expect(component.providerNeedsCredential()).toBe(false); + }); + + it('requires a credential for a provider the catalog says needs one', async () => { + const { component } = build(); + component.selectProvider('Gemini'); + await component.ensureLlmProviderCapabilitiesLoaded(); + + expect(component.providerNeedsCredential()).toBe(true); + }); + + it('defaults to not requiring a credential while the catalog has not loaded yet', () => { + // Consistent with providerRequiresEndpoint's own default: an unresolved capability answers no, + // not yes - a provider is not asked for a credential it may never have needed. + const { component } = build(); + component.selectProvider('Gemini'); + + expect(component.providerNeedsCredential()).toBe(false); + }); + + it('shows a required endpoint field for a provider whose credential must carry one', async () => { + const { component, settingsDialog } = build({ + llmProviderCapabilities: [ + { name: 'Gemini', requiresCredential: true, requiresEndpoint: false }, + { name: 'OpenAICompatible', requiresCredential: true, requiresEndpoint: true } + ] + }); + component.providers.set(['Gemini', 'OpenAICompatible']); + + await component.openCredentialForm('OpenAICompatible'); + + const dialog = settingsDialog.open.mock.calls[0][0] as any; + expect(dialog.fields.map((field: any) => field.key)) + .toEqual(['label', 'provider', 'description', 'endpoint', 'value']); + expect(dialog.fields.find((field: any) => field.key === 'endpoint').required).toBe(true); + }); + + it('adds or removes the endpoint field as the provider changes inside the open dialog', async () => { + const { component, settingsDialog } = build({ + llmProviderCapabilities: [ + { name: 'Gemini', requiresCredential: true, requiresEndpoint: false }, + { name: 'OpenAICompatible', requiresCredential: true, requiresEndpoint: true } + ] + }); + component.providers.set(['Gemini', 'OpenAICompatible']); + + await component.openCredentialForm('Gemini'); + + const dialog = settingsDialog.open.mock.calls[0][0] as any; + const toEndpointProvider = await dialog.onValuesChange({ provider: 'OpenAICompatible' }); + expect(toEndpointProvider.fields.map((field: any) => field.key)).toContain('endpoint'); + + const backToGemini = await dialog.onValuesChange({ provider: 'Gemini' }); + expect(backToGemini.fields.map((field: any) => field.key)).not.toContain('endpoint'); + }); + + it('sends the endpoint the dialog returned along with the rest of the credential', async () => { + const { component, vault } = build({ + llmProviderCapabilities: [ + { name: 'OpenAICompatible', requiresCredential: true, requiresEndpoint: true } + ], + dialogResult: { + label: 'Gateway key', + provider: 'OpenAICompatible', + description: '', + endpoint: 'https://api.example.com/v1', + value: 'sk-1' + } + }); + + await component.openCredentialForm('OpenAICompatible'); + + expect(vault.createSecret).toHaveBeenCalledWith({ + label: 'Gateway key', + provider: 'OpenAICompatible', + description: undefined, + value: 'sk-1', + endpoint: 'https://api.example.com/v1' + }); + }); }); describe('FlowAssistant quick prompts', () => { diff --git a/src/app/shared/flow-assistant/flow-assistant.ts b/src/app/shared/flow-assistant/flow-assistant.ts index 3081674..a24edc9 100644 --- a/src/app/shared/flow-assistant/flow-assistant.ts +++ b/src/app/shared/flow-assistant/flow-assistant.ts @@ -23,10 +23,12 @@ import { VaultSecret } from '@models/assistant'; import { Flow } from '@models/flow'; +import { LlmProviderCapability } from '@models/llm-provider'; import { AssistantService } from '@services/assistant/assistant'; import { CREDENTIAL_ERROR_MESSAGES, VaultService } from '@services/vault/vault'; import { Authorization } from '@services/authorization/authorization'; import { NodeSettingField, NodeSettingsDialogService } from '@services/dialogs/node-settings-dialog'; +import { LlmProviderService } from '@services/llm-provider/llm-provider'; import { AssistantSessionStore } from '@stores/assistant-session-store'; import { EditorStateHolder } from '@stores/flow-editor'; import { finalize, firstValueFrom, interval, Subscription, switchMap, take } from 'rxjs'; @@ -48,6 +50,7 @@ export class FlowAssistant implements OnInit, OnDestroy { private readonly authorization = inject(Authorization); private readonly sessionStore = inject(AssistantSessionStore); private readonly settingsDialog = inject(NodeSettingsDialogService); + private readonly llmProviderService = inject(LlmProviderService); private pollSubscription: Subscription | null = null; private initialized = false; private activeFlowKey: string | null = null; @@ -64,6 +67,8 @@ export class FlowAssistant implements OnInit, OnDestroy { readonly localMessages = signal([]); readonly models = signal([]); readonly providers = signal([]); + /** Loaded lazily, once, the first time the credential dialog is opened. */ + private readonly llmProviderCapabilities = signal([]); readonly providersLoading = signal(false); readonly providersError = signal(null); readonly modelsLoading = signal(false); @@ -242,7 +247,7 @@ export class FlowAssistant implements OnInit, OnDestroy { }); readonly providerNeedsCredential = computed(() => - !!this.selectedProvider().trim() && !this.isInternalProvider(this.selectedProvider()) + !!this.selectedProvider().trim() && this.providerRequiresCredential(this.selectedProvider()) ); readonly compatibleCredentials = computed(() => { const provider = this.selectedProvider().trim().toLowerCase(); @@ -348,29 +353,52 @@ export class FlowAssistant implements OnInit, OnDestroy { async openCredentialForm(provider = this.selectedProvider()) { if (this.configurationLocked()) return; this.credentialsError.set(null); + await this.ensureLlmProviderCapabilitiesLoaded(); // The current provider is offered even when the list has not loaded: it is the one the user // is adding a key for, and a select with nothing in it would be a dead end. const providerOptions = [...new Set([...this.providers(), provider].filter((name) => !!name))] .map((name) => ({ label: name, value: name })); - const fields: NodeSettingField[] = [ - { key: 'label', label: 'Label', type: 'text', required: true, autofocus: true }, - { key: 'provider', label: 'Provider', type: 'select', required: true, options: providerOptions }, - { key: 'description', label: 'Description (optional)', type: 'text' }, - { + // Provider is a select the user can still change inside this same dialog, unlike the fixed + // provider on an execution's credential form - so which fields apply is recomputed from the + // draft on every change, through onValuesChange below, rather than decided once up front. + const buildFields = (currentProvider: string): NodeSettingField[] => { + const fields: NodeSettingField[] = [ + { key: 'label', label: 'Label', type: 'text', required: true, autofocus: true }, + { key: 'provider', label: 'Provider', type: 'select', required: true, options: providerOptions }, + { key: 'description', label: 'Description (optional)', type: 'text' } + ]; + if (this.providerRequiresEndpoint(currentProvider)) { + fields.push({ + key: 'endpoint', + label: 'Endpoint URL', + type: 'text', + required: true, + tip: 'The base URL to call, e.g. https://api.example.com/v1' + }); + } + fields.push({ key: 'value', label: 'API key', type: 'password', required: true, tip: 'The value will not be shown again after saving.' - } - ]; + }); + return fields; + }; const result = await this.settingsDialog.open({ title: 'Add credential', - fields, - initial: { label: '', provider, description: '', value: '' } + fields: buildFields(provider), + initial: { label: '', provider, description: '', endpoint: '', value: '' }, + onValuesChange: (draft) => ({ + fields: buildFields(String(draft['provider'] ?? '').trim()), + // Every other field's current value survives this refresh regardless of what is put + // here - the dialog only falls back to this for a field the draft does not have yet, + // which is exactly the endpoint field the first time it appears. + initial: { endpoint: '' } + }) }); if (!result) return; @@ -378,16 +406,55 @@ export class FlowAssistant implements OnInit, OnDestroy { String(result['label'] ?? '').trim(), String(result['provider'] ?? '').trim(), String(result['description'] ?? '').trim(), - String(result['value'] ?? '') + String(result['value'] ?? ''), + String(result['endpoint'] ?? '').trim() ); } - private saveCredential(label: string, provider: string, description: string, value: string) { + /** + * Called both eagerly at bootstrap - so providerNeedsCredential and providerRequiresEndpoint + * are answered correctly as soon as a provider is picked - and again, cheaply idempotent, right + * before the credential dialog opens, in case bootstrap has not resolved yet. + */ + private async ensureLlmProviderCapabilitiesLoaded(): Promise { + if (this.llmProviderCapabilities().length > 0) return; + try { + const capabilities = await firstValueFrom(this.llmProviderService.listCapabilities()); + this.llmProviderCapabilities.set(capabilities); + } catch { + // Every capability question defaults to false while the catalog cannot be reached - no + // loading/error state of its own, matching the conservative default used everywhere else + // for an unresolved capability. + } + } + + private providerRequiresEndpoint(provider: string): boolean { + return this.findProviderCapability(provider)?.requiresEndpoint ?? false; + } + + /** + * Reads the same catalog `/llm/providers` already publishes, instead of comparing the provider's + * name to a hardcoded string. That old check named only `"InternalOllama"` as needing no + * credential, so a credential-free provider under any other name - a remote Ollama with no key, + * say - was told it needed one anyway. Defaults to false while the catalog has not loaded yet, + * the same conservative default `providerRequiresEndpoint` uses for an unresolved capability. + */ + private providerRequiresCredential(provider: string): boolean { + return this.findProviderCapability(provider)?.requiresCredential ?? false; + } + + private findProviderCapability(provider: string): LlmProviderCapability | undefined { + const wanted = provider.trim().toLowerCase(); + return this.llmProviderCapabilities().find((capability) => capability.name.trim().toLowerCase() === wanted); + } + + private saveCredential(label: string, provider: string, description: string, value: string, endpoint: string) { if (!label || !provider || !value.trim() || this.credentialSaving()) return; this.credentialSaving.set(true); this.credentialsError.set(null); - this.vault.createSecret({ label, provider, description: description || undefined, value }) + this.vault.createSecret({ label, provider, description: description || undefined, value, + endpoint: endpoint || undefined }) .pipe(finalize(() => this.credentialSaving.set(false))) .subscribe({ next: (credential) => { @@ -588,6 +655,10 @@ export class FlowAssistant implements OnInit, OnDestroy { this.assistantConfig.set(config); this.initializeConfiguration(config); void this.loadCredentials(); + // Loaded eagerly, not only when the credential dialog opens: providerNeedsCredential is a + // computed signal that gates save-validity as soon as a provider is picked, so it cannot + // wait for a promise nobody has awaited yet. + void this.ensureLlmProviderCapabilitiesLoaded(); }, error: (err) => { console.error('Assistant config loading failed', err); @@ -744,10 +815,6 @@ export class FlowAssistant implements OnInit, OnDestroy { return FlowAssistant.STANDARD_ASSISTANT_ERROR; } - private isInternalProvider(provider: string): boolean { - return provider.trim().toLowerCase() === 'internalollama'; - } - private sameProvider(left: string, right: string): boolean { return left.trim().toLowerCase() === right.trim().toLowerCase(); }