lelii
/
humainflow-web
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Read the real provider catalog instead of hardcoding InternalOllama 

Two things in the copilot both used to hinge on the same wrong check,
isInternalProvider(provider) - a plain string comparison - instead of
what the backend's /llm/providers catalog already publishes:

  - providerNeedsCredential decided whether to show the credential picker
    by name, so a credential-free provider under any name other than
    "InternalOllama" - a remote Ollama with no key, say - was told it
    needed a credential anyway. It now reads requiresCredential from the
    catalog.
  - The "Add credential" dialog had no endpoint field at all. It now
    shows one, built the same way llm-descriptor-settings.ts already
    rebuilds its model field when the provider changes: onValuesChange
    recomputes the field list from the current draft, so choosing a
    provider that requiresEndpoint adds the field, and choosing one back
    away removes it, without closing the dialog.

Both read the same LlmProviderService the execution viewer already uses,
loaded once and cached in llmProviderCapabilities. It has to be loaded
eagerly now, not only when the dialog opens: providerNeedsCredential is a
computed() signal that gates whether the configuration is valid to save,
so it cannot wait for a promise nothing has awaited yet. Bootstrap now
kicks that load off in the background; the dialog's own lazy load stays
too, in case bootstrap has not resolved by the time it opens.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Lucio Lelii 2026-09-17 12:30:28 +02:00
parent a0fdfcda35
commit e1708f95b8
2 changed files with 196 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

113
src/app/shared/flow-assistant/flow-assistant.spec.ts
View File

@ -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<string, string> | 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', () => {

101
src/app/shared/flow-assistant/flow-assistant.ts
View File

@ -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<AssistantChatMessage[]>([]);
readonly models = signal<string[]>([]);
readonly providers = signal<string[]>([]);
/** Loaded lazily, once, the first time the credential dialog is opened. */
private readonly llmProviderCapabilities = signal<LlmProviderCapability[]>([]);
readonly providersLoading = signal(false);
readonly providersError = signal<string | null>(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<void> {
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();
}