diff --git a/src/app/services/assistant/assistant-call.base.ts b/src/app/services/assistant/assistant-call.base.ts index f8f1044..29ce6ae 100644 --- a/src/app/services/assistant/assistant-call.base.ts +++ b/src/app/services/assistant/assistant-call.base.ts @@ -15,6 +15,12 @@ export abstract class AssistantCallServiceBase { abstract listModels(retrieverUrlTemplate: string, provider: string): Observable; + /** + * Whether the provider's model list is incomplete, so the model has to be typed. True for the + * hosted providers, whose catalogues cannot be enumerated without a credential. + */ + abstract areModelsOpen(retrieverUrlTemplate: string, provider: string): Observable; + abstract createSession(request: AssistantSessionRequest): Observable; abstract getSession(sessionId: string): Observable; diff --git a/src/app/services/assistant/assistant-call.fake.ts b/src/app/services/assistant/assistant-call.fake.ts index 9caab9c..3aa6ea1 100644 --- a/src/app/services/assistant/assistant-call.fake.ts +++ b/src/app/services/assistant/assistant-call.fake.ts @@ -16,6 +16,11 @@ import { AssistantCallServiceBase } from './assistant-call.base'; export class AssistantCallServiceFake extends AssistantCallServiceBase { private readonly models = ['llama3.1:8b', 'qwen2.5:7b', 'mistral:7b']; private readonly providers = ['InternalOllama', 'OpenAI']; + /** + * The hosted providers list nothing, exactly as the real ones do: their catalogues cannot be + * enumerated without a credential, so the model has to be typed. + */ + private readonly openModelProviders = new Set(['OpenAI', 'Anthropic', 'Gemini']); private readonly sessions = new Map(); private readonly calls = new Map(); @@ -34,7 +39,11 @@ export class AssistantCallServiceFake extends AssistantCallServiceBase { } override listModels(_retrieverUrlTemplate: string, _provider: string): Observable { - return of(this.models); + return of(this.openModelProviders.has(_provider) ? [] : this.models); + } + + override areModelsOpen(_retrieverUrlTemplate: string, provider: string): Observable { + return of(this.openModelProviders.has(provider)); } override createSession(request: AssistantSessionRequest): Observable { diff --git a/src/app/services/assistant/assistant-call.spec.ts b/src/app/services/assistant/assistant-call.spec.ts index 381c90f..a21f2f0 100644 --- a/src/app/services/assistant/assistant-call.spec.ts +++ b/src/app/services/assistant/assistant-call.spec.ts @@ -105,6 +105,22 @@ describe('AssistantCallService', () => { await expect(models).resolves.toEqual(['gpt-oss:20b']); }); + it('asks whether a provider can be enumerated on the sibling endpoint, keeping the query', async () => { + // The suffix goes on the path, not the end of the URL: the provider has to survive it, or the + // answer would be about no provider at all. + const open = firstValueFrom(service.areModelsOpen('/retriever/LLM/models?provider={provider}', 'Gemini')); + httpMock.expectOne(`${environment.apiUrl}/retriever/LLM/models/open?provider=Gemini`).flush(true); + await expect(open).resolves.toBe(true); + }); + + it('treats anything but a plain true as a closed list', async () => { + // Closed is the safe answer: a select the user can see is empty beats a text box that silently + // accepts a model the provider does not have. + const open = firstValueFrom(service.areModelsOpen('/retriever/LLM/models?provider={provider}', 'InternalOllama')); + httpMock.expectOne(`${environment.apiUrl}/retriever/LLM/models/open?provider=InternalOllama`).flush('yes'); + await expect(open).resolves.toBe(false); + }); + it('sends llmSelection only when supplied when creating a session', async () => { const defaultSession = firstValueFrom(service.createSession({})); const defaultSessionRequest = httpMock.expectOne(`${environment.apiUrl}/assistant/sessions`); diff --git a/src/app/services/assistant/assistant-call.ts b/src/app/services/assistant/assistant-call.ts index 76eff0f..25a0994 100644 --- a/src/app/services/assistant/assistant-call.ts +++ b/src/app/services/assistant/assistant-call.ts @@ -13,7 +13,8 @@ import { AssistantValidationIssue } from '@models/assistant'; import { environment } from '@environment'; -import { map, Observable } from 'rxjs'; +import { map, Observable, of } from 'rxjs'; +import { appendRetrieverQuestion } from '@services/retriever/retriever-url'; import { AssistantCallServiceBase } from './assistant-call.base'; export class AssistantCallService extends AssistantCallServiceBase { @@ -39,6 +40,15 @@ export class AssistantCallService extends AssistantCallServiceBase { .pipe(map((raw) => mapModelList(raw))); } + override areModelsOpen(retrieverUrlTemplate: string, provider: string): Observable { + const retrieverUrl = retrieverUrlTemplate.replace('{provider}', encodeURIComponent(provider)); + const openUrl = appendRetrieverQuestion(retrieverUrl, 'open'); + if (!openUrl) return of(false); + return this.http + .get(resolveAssistantUrl(openUrl)) + .pipe(map((raw) => raw === true)); + } + override createSession(request: AssistantSessionRequest): Observable { return this.http .post(`${environment.apiUrl}/assistant/sessions`, request) diff --git a/src/app/services/assistant/assistant.ts b/src/app/services/assistant/assistant.ts index cbc3ff3..8b038d2 100644 --- a/src/app/services/assistant/assistant.ts +++ b/src/app/services/assistant/assistant.ts @@ -24,6 +24,10 @@ export class AssistantService { return this.assistantCall.listModels(retrieverUrlTemplate, provider); } + areModelsOpen(retrieverUrlTemplate: string, provider: string) { + return this.assistantCall.areModelsOpen(retrieverUrlTemplate, provider); + } + createSession(request: AssistantSessionRequest) { return this.assistantCall.createSession(request); } diff --git a/src/app/services/retriever/field-retriever-call.ts b/src/app/services/retriever/field-retriever-call.ts index 33688a0..f211d75 100644 --- a/src/app/services/retriever/field-retriever-call.ts +++ b/src/app/services/retriever/field-retriever-call.ts @@ -3,9 +3,7 @@ import { inject } from "@angular/core"; import { environment } from "@environment"; import { map, Observable, of } from "rxjs"; import { FieldRetrieverCallServiceBase, RetrieverStructuredItem } from "./field-retriever-call.base"; - -/** The boolean questions a retriever answers about a field, each on its own sibling endpoint. */ -type RetrieverQuestion = 'required' | 'open'; +import { appendRetrieverQuestion, RetrieverQuestion } from "./retriever-url"; export class FieldRetrieverCallService extends FieldRetrieverCallServiceBase { private readonly http = inject(HttpClient); @@ -40,7 +38,7 @@ export class FieldRetrieverCallService extends FieldRetrieverCallServiceBase { context?: Record, retrieverUrl?: string | null ): Observable { - const requiredRetrieverUrl = this.appendSuffix(retrieverUrl, 'required'); + const requiredRetrieverUrl = appendRetrieverQuestion(retrieverUrl, 'required'); const { url, params } = this.resolveRequest(blockType, key, context, requiredRetrieverUrl, 'required'); return this.http.get(url, { params }); } @@ -51,7 +49,7 @@ export class FieldRetrieverCallService extends FieldRetrieverCallServiceBase { context?: Record, retrieverUrl?: string | null ): Observable { - const openRetrieverUrl = this.appendSuffix(retrieverUrl, 'open'); + const openRetrieverUrl = appendRetrieverQuestion(retrieverUrl, 'open'); const { url, params } = this.resolveRequest(blockType, key, context, openRetrieverUrl, 'open'); return this.http.get(url, { params }); } @@ -123,17 +121,6 @@ export class FieldRetrieverCallService extends FieldRetrieverCallServiceBase { return { url, params }; } - /** - * The yes/no endpoints sit beside the values one, so their URL is the configured retriever URL - * with a suffix - no second URL to declare on the field. - */ - private appendSuffix(rawUrl: string | null | undefined, suffix: RetrieverQuestion): string | null { - if (typeof rawUrl !== 'string' || rawUrl.trim().length === 0) return null; - const [path, queryString] = rawUrl.split('?', 2); - const normalizedPath = path.endsWith(`/${suffix}`) ? path : `${path}/${suffix}`; - return queryString ? `${normalizedPath}?${queryString}` : normalizedPath; - } - private normalizeStringList(raw: unknown): string[] { if (Array.isArray(raw)) { return this.normalizeStringArray(raw); diff --git a/src/app/services/retriever/retriever-url.spec.ts b/src/app/services/retriever/retriever-url.spec.ts new file mode 100644 index 0000000..69ddd05 --- /dev/null +++ b/src/app/services/retriever/retriever-url.spec.ts @@ -0,0 +1,25 @@ +import { appendRetrieverQuestion } from './retriever-url'; + +/** + * Two callers derive these URLs - the schema-driven fields and the assistant's own model picker - + * so the rule lives in one place. The query string is the part that breaks if it does not. + */ +describe('appendRetrieverQuestion', () => { + it('suffixes the path and keeps the query, which carries the provider', () => { + expect(appendRetrieverQuestion('/retriever/LLM/models?provider=Gemini', 'open')) + .toBe('/retriever/LLM/models/open?provider=Gemini'); + expect(appendRetrieverQuestion('/retriever/LLM/models', 'required')) + .toBe('/retriever/LLM/models/required'); + }); + + it('does not suffix twice, so a derived URL survives a second pass', () => { + expect(appendRetrieverQuestion('/retriever/LLM/models/open?provider=Gemini', 'open')) + .toBe('/retriever/LLM/models/open?provider=Gemini'); + }); + + it('has no answer for a field that declares no retriever URL', () => { + expect(appendRetrieverQuestion(null, 'open')).toBeNull(); + expect(appendRetrieverQuestion(undefined, 'open')).toBeNull(); + expect(appendRetrieverQuestion(' ', 'open')).toBeNull(); + }); +}); diff --git a/src/app/services/retriever/retriever-url.ts b/src/app/services/retriever/retriever-url.ts new file mode 100644 index 0000000..2c3b2c1 --- /dev/null +++ b/src/app/services/retriever/retriever-url.ts @@ -0,0 +1,19 @@ +/** The boolean questions a retriever answers about a field, each on its own sibling endpoint. */ +export type RetrieverQuestion = 'required' | 'open'; + +/** + * The endpoint that answers one of those questions, derived from the values URL by suffixing its + * path - so a field declares one URL and nothing has to stay in sync. + * + *

Shared rather than reimplemented per caller: the query string has to survive the suffix, and + * a second copy of that detail is where the two would drift apart. + */ +export function appendRetrieverQuestion( + rawUrl: string | null | undefined, + question: RetrieverQuestion +): string | null { + if (typeof rawUrl !== 'string' || rawUrl.trim().length === 0) return null; + const [path, queryString] = rawUrl.split('?', 2); + const normalizedPath = path.endsWith(`/${question}`) ? path : `${path}/${question}`; + return queryString ? `${normalizedPath}?${queryString}` : normalizedPath; +} diff --git a/src/app/shared/flow-assistant/flow-assistant.html b/src/app/shared/flow-assistant/flow-assistant.html index 0958cf8..fe814ec 100644 --- a/src/app/shared/flow-assistant/flow-assistant.html +++ b/src/app/shared/flow-assistant/flow-assistant.html @@ -76,6 +76,15 @@ Model + @if (modelsOpen()) { + + } @else { {{ model }} } - @if (modelsLoading()) { Loading models... } + } + @if (modelsLoading() && !modelsOpen()) { Loading models... } @if (modelsError()) { {{ modelsError() }} } @@ -120,24 +130,54 @@

Planning + @if (modelsOpen()) { + + } @else { Main model @for (model of models(); track model) { {{ model }} } + } JSON + @if (modelsOpen()) { + + } @else { Main model @for (model of models(); track model) { {{ model }} } + } Repair + @if (modelsOpen()) { + + } @else { Main model @for (model of models(); track model) { {{ model }} } + }
} diff --git a/src/app/shared/flow-assistant/flow-assistant.spec.ts b/src/app/shared/flow-assistant/flow-assistant.spec.ts new file mode 100644 index 0000000..20701b3 --- /dev/null +++ b/src/app/shared/flow-assistant/flow-assistant.spec.ts @@ -0,0 +1,111 @@ +import { TestBed } from '@angular/core/testing'; +import { AssistantService } from '@services/assistant/assistant'; +import { Authorization } from '@services/authorization/authorization'; +import { VaultService } from '@services/vault/vault'; +import { AssistantSessionStore } from '@stores/assistant-session-store'; +import { EditorStateHolder } from '@stores/flow-editor'; +import { of } from 'rxjs'; +import { vi } from 'vitest'; + +import { FlowAssistant } from './flow-assistant'; + +/** + * The copilot picks a provider and a model through its own controls, not through the schema-driven + * field machinery, so the rule that a hosted catalogue has to be typed rather than picked has to + * be honoured here too - and its "no models available" message must not fire on a provider whose + * models were never listable in the first place. + */ +describe('FlowAssistant model selection', () => { + const MODELS_URL = '/retriever/LLM/models?provider={provider}'; + + function build(options: { models: string[]; open: boolean }) { + const assistant = { + getConfig: vi.fn(() => of({ + defaultProvider: '', + defaultModel: '', + availableProvidersRetrieverUrl: '/retriever/LLM/providers', + availableModelsRetrieverUrl: MODELS_URL, + defaultPhaseModels: {}, + providerCatalogUrl: '/llm/providers' + })), + listProviders: vi.fn(() => of(['InternalOllama', 'Gemini'])), + listModels: vi.fn(() => of(options.models)), + areModelsOpen: vi.fn(() => of(options.open)), + listProviderCatalog: vi.fn(() => of([])) + }; + + TestBed.configureTestingModule({ + providers: [ + { provide: AssistantService, useValue: assistant }, + { provide: VaultService, useValue: { listSecrets: vi.fn(() => of([])) } }, + { + provide: EditorStateHolder, + useValue: { currentFlow: vi.fn(() => null), activeFlowData: vi.fn(() => null) } + }, + { provide: Authorization, useValue: { currentUser: vi.fn(() => null) } }, + { + provide: AssistantSessionStore, + useValue: { + flowKey: vi.fn(() => 'flow-1'), + getSnapshot: vi.fn(() => null), + setSnapshot: vi.fn(), + clearSnapshot: vi.fn(), + cloneSnapshot: vi.fn((snapshot: unknown) => snapshot) + } + } + ] + }); + + const component = TestBed.createComponent(FlowAssistant).componentInstance as any; + component.assistantConfig.set({ availableModelsRetrieverUrl: MODELS_URL }); + return { component, assistant }; + } + + afterEach(() => TestBed.resetTestingModule()); + + it('takes a typed model when the provider cannot be enumerated, and reports no error', () => { + // Gemini lists nothing because its catalogue needs a credential. Calling that "no models are + // available" would be wrong twice: the models exist, and the message hides the text field. + const { component, assistant } = build({ models: [], open: true }); + + component.selectProvider('Gemini'); + + expect(assistant.areModelsOpen).toHaveBeenCalledWith(MODELS_URL, 'Gemini'); + expect(component.modelsOpen()).toBe(true); + expect(component.modelsError()).toBeNull(); + }); + + it('still reports an empty list from a provider that was supposed to have one', () => { + // Our own Ollama is asked what it has, so nothing back means it is unreachable - a text field + // there would hide a broken provider. + const { component } = build({ models: [], open: false }); + + component.selectProvider('InternalOllama'); + + expect(component.modelsOpen()).toBe(false); + expect(component.modelsError()).toBe('No models are available for the selected provider.'); + }); + + it('keeps the select when the provider lists its models', () => { + const { component } = build({ models: ['llama3.2:3b'], open: false }); + + component.selectProvider('InternalOllama'); + + expect(component.modelsOpen()).toBe(false); + expect(component.models()).toEqual(['llama3.2:3b']); + expect(component.modelsError()).toBeNull(); + }); + + it('forgets that a provider was open when another one is chosen', () => { + const { component } = build({ models: [], open: true }); + component.selectProvider('Gemini'); + expect(component.modelsOpen()).toBe(true); + + // The next provider decides for itself; carrying the flag over would offer a text field + // against a closed list. + component.assistantConfig.set(null); + component.selectProvider('InternalOllama'); + + expect(component.modelsOpen()).toBe(false); + }); +}); diff --git a/src/app/shared/flow-assistant/flow-assistant.ts b/src/app/shared/flow-assistant/flow-assistant.ts index f5691c9..45c07f1 100644 --- a/src/app/shared/flow-assistant/flow-assistant.ts +++ b/src/app/shared/flow-assistant/flow-assistant.ts @@ -62,6 +62,11 @@ export class FlowAssistant implements OnInit, OnDestroy { readonly providersError = signal(null); readonly modelsLoading = signal(false); readonly modelsError = signal(null); + /** + * The provider cannot be asked what it offers, so the model is typed rather than picked. An + * empty list is not enough to tell: for our own Ollama it means the provider is unreachable. + */ + readonly modelsOpen = signal(false); readonly sessionLoading = signal(false); readonly requestPending = signal(false); readonly prompt = signal(''); @@ -293,6 +298,7 @@ export class FlowAssistant implements OnInit, OnDestroy { this.selectedCredentialId.set(''); this.models.set([]); this.modelsError.set(null); + this.modelsOpen.set(false); if (provider) void this.loadModels(provider); if (provider) void this.loadCredentials(); this.persistSnapshot(); @@ -607,13 +613,27 @@ export class FlowAssistant implements OnInit, OnDestroy { if (!config?.availableModelsRetrieverUrl) return; this.modelsLoading.set(true); this.modelsError.set(null); + + // Asked alongside the list and not derived from it: an open provider is exactly the one whose + // list comes back empty, so "nothing to show" and "nothing to offer" must not be the same + // answer. Closed on failure, which keeps a select the user can see is broken. + this.assistant.areModelsOpen(config.availableModelsRetrieverUrl, provider).pipe(take(1)).subscribe({ + next: (open) => { + this.modelsOpen.set(open); + if (open) this.modelsError.set(null); + }, + error: () => this.modelsOpen.set(false) + }); + this.assistant.listModels(config.availableModelsRetrieverUrl, provider).pipe( take(1), finalize(() => this.modelsLoading.set(false)) ).subscribe({ next: (models) => { this.models.set(models); - if (!models.length) this.modelsError.set('No models are available for the selected provider.'); + if (!models.length && !this.modelsOpen()) { + this.modelsError.set('No models are available for the selected provider.'); + } }, error: (err) => this.modelsError.set(this.backendErrorMessage(err)) });