From 4fd746a72eb536f749396000363239ec98afc7f0 Mon Sep 17 00:00:00 2001 From: Roman Lutz Date: Sun, 1 Nov 2026 11:53:32 -0800 Subject: [PATCH] FIX: Guard preset saves during scenario selection Keep the selected scenario separate from resolved metadata and reject superseded requests, including programmatic submits before selection state commits. Preserve failed selections, loaded preset values, and already accepted saves. Fixes #3755 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- doc/gui/0_gui.md | 3 + .../ScenarioPresetEditor.test.tsx | 362 +++++++++++++++++- .../ScenarioPresets/ScenarioPresetEditor.tsx | 54 ++- 3 files changed, 390 insertions(+), 29 deletions(-) diff --git a/doc/gui/0_gui.md b/doc/gui/0_gui.md index b653464ac1..efebe3b313 100644 --- a/doc/gui/0_gui.md +++ b/doc/gui/0_gui.md @@ -82,6 +82,9 @@ on this page. Target defaults apply to new chats and scans. **Scenario presets** save a scenario configuration for reuse with a target chosen at launch. **Suites** maintain ordered collections of preset definitions. They are stored separately from live component instances. +In the preset editor, **Save preset** is unavailable until the selected scenario's +configuration loads. A failed load keeps your selection and shows an error rather +than saving the previous configuration. Targets, converters, and techniques added only to the live registry must be recreated after a backend restart or reinitialization. Use diff --git a/frontend/src/components/ScenarioPresets/ScenarioPresetEditor.test.tsx b/frontend/src/components/ScenarioPresets/ScenarioPresetEditor.test.tsx index 7ed1573f4d..4f2756849f 100644 --- a/frontend/src/components/ScenarioPresets/ScenarioPresetEditor.test.tsx +++ b/frontend/src/components/ScenarioPresets/ScenarioPresetEditor.test.tsx @@ -1,11 +1,21 @@ -import { act, render, screen, waitFor, within } from '@testing-library/react' +import type { ReactNode } from 'react' + +import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { FluentProvider, webLightTheme } from '@fluentui/react-components' -import { MemoryRouter, Route, Routes } from 'react-router' +import { MemoryRouter, Route, Routes, useNavigate } from 'react-router' +import * as scenarioConfigForm from '@/components/Scenarios/scenarioConfigForm' import { scenarioPresetsApi, scenariosApi } from '@/services/api' +import { deferred } from '@/test-utils/deferred' import { makeScenario } from '@/test-utils/scenarioFixtures' -import type { ScenarioDatasetSettings, ScenarioPreset, ScenarioRunSizeEstimateResponse } from '@/types' +import type { + RegisteredScenario, + ScenarioDatasetSettings, + ScenarioPreset, + ScenarioPresetResponse, + ScenarioRunSizeEstimateResponse, +} from '@/types' import ScenarioPresetEditor from './ScenarioPresetEditor' @@ -42,9 +52,10 @@ const mockNavigate = jest.fn() jest.mock('react-router', () => ({ ...jest.requireActual('react-router'), - useNavigate: () => mockNavigate, + useNavigate: jest.fn(() => mockNavigate), })) +const mockUseNavigate = jest.mocked(useNavigate) const SCENARIO = makeScenario() const STORED_PRESET: ScenarioPreset = { @@ -62,6 +73,21 @@ function apiError(status: number, detail: string): unknown { } } +function TestWrapper({ children }: { children: ReactNode }) { + return {children} +} + +async function selectScenario(user: ReturnType, scenarioName: string): Promise { + await user.click(screen.getByRole('combobox', { name: 'Scenario' })) + await user.click(await screen.findByRole('option', { name: scenarioName, exact: true })) +} + +function submitPresetForm(): void { + const form = screen.getByRole('button', { name: /^(Save preset|Saving\.\.\.)$/ }).closest('form') + if (!form) throw new Error('Save preset must submit the preset form.') + fireEvent.submit(form) +} + /** Drives the shared dataset table's limit popover the same way its own suite does. */ async function editLimit(user: ReturnType, label: string, value: string): Promise { await user.click(await screen.findByRole('button', { name: `Edit ${label}` })) @@ -74,33 +100,36 @@ async function editLimit(user: ReturnType, label: string function renderCreate() { return render( - - - - } /> - - - , + + + } /> + Preset library} /> + + , + { wrapper: TestWrapper }, ) } function renderEdit(name = 'nightly_probe') { return render( - - - - } - /> - - - , + + + } + /> + + , + { wrapper: TestWrapper }, ) } beforeEach(() => { jest.clearAllMocks() + for (const mock of [mockGet, mockCreate, mockUpdate, mockListCatalog, mockGetScenario, mockEstimateRun]) { + mock.mockReset() + } + mockUseNavigate.mockReturnValue(mockNavigate) mockListCatalog.mockResolvedValue({ items: [SCENARIO], pagination: { has_more: false, next_cursor: null }, @@ -260,6 +289,266 @@ describe('ScenarioPresetEditor create mode', () => { }) }) +describe('ScenarioPresetEditor scenario selection ownership', () => { + const REPLACEMENT_SCENARIO = makeScenario({ + scenario_name: 'airt.multilingual', + all_techniques: ['multilingual'], + default_techniques: ['multilingual'], + supported_parameters: [ + { name: 'languages', type_name: 'list[str]', is_list: true, required: false, default: ['Spanish'] }, + ], + }) + const LATEST_SCENARIO = makeScenario({ + scenario_name: 'garak.prompt_inject', + all_techniques: ['prompt_inject'], + default_techniques: ['prompt_inject'], + }) + + beforeEach(() => { + mockListCatalog.mockResolvedValue({ + items: [SCENARIO, REPLACEMENT_SCENARIO, LATEST_SCENARIO], + pagination: { has_more: false, next_cursor: null }, + }) + }) + + async function renderSelectedCreate( + user: ReturnType, + ): Promise> { + const view = renderCreate() + await user.type(await screen.findByRole('textbox', { name: 'Name' }), 'nightly_probe') + await selectScenario(user, SCENARIO.scenario_name) + await waitFor(() => expect(screen.getByRole('button', { name: 'Save preset' })).toBeEnabled()) + return view + } + + it('disables Save during a pending replacement and saves only the replacement configuration', async () => { + const user = userEvent.setup() + await renderSelectedCreate(user) + await user.click(screen.getByTestId('technique-crescendo')) + const replacement = deferred() + mockGetScenario.mockReturnValueOnce(replacement.promise) + + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + + expect(screen.getByText('Loading scenario...')).toBeInTheDocument() + expect(screen.getByRole('combobox', { name: 'Scenario' })).toHaveValue(REPLACEMENT_SCENARIO.scenario_name) + const save = screen.getByRole('button', { name: 'Save preset' }) + expect(save).toBeDisabled() + await user.click(save) + expect(mockCreate).not.toHaveBeenCalled() + expect(mockNavigate).not.toHaveBeenCalled() + + await act(async () => { replacement.resolve(REPLACEMENT_SCENARIO) }) + expect(await screen.findByRole('textbox', { name: 'languages' })).toHaveValue('Spanish') + expect(save).toBeEnabled() + await user.click(save) + expect(mockCreate).toHaveBeenCalledWith({ + name: 'nightly_probe', + scenario_name: REPLACEMENT_SCENARIO.scenario_name, + }) + expect(mockNavigate).toHaveBeenCalledWith('/registry/scenario-presets') + }) + + it('guards a programmatic submit during pending replacement metadata', async () => { + const user = userEvent.setup() + await renderSelectedCreate(user) + const replacement = deferred() + mockGetScenario.mockReturnValueOnce(replacement.promise) + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + + submitPresetForm() + + expect(mockCreate).not.toHaveBeenCalled() + expect(mockUpdate).not.toHaveBeenCalled() + expect(mockNavigate).not.toHaveBeenCalled() + expect(screen.getByRole('alert')).toHaveTextContent('Wait for the selected scenario to finish loading.') + await act(async () => { replacement.resolve(REPLACEMENT_SCENARIO) }) + }) + + it('guards a programmatic submit before the selection event finishes rendering', async () => { + const user = userEvent.setup() + await renderSelectedCreate(user) + const replacement = deferred() + mockGetScenario.mockReturnValueOnce(replacement.promise) + await user.click(screen.getByRole('combobox', { name: 'Scenario' })) + const option = await screen.findByRole('option', { name: REPLACEMENT_SCENARIO.scenario_name, exact: true }) + let selectionStarted = false + const submitDuringSelection = (): void => { + selectionStarted = mockGetScenario.mock.calls.length === 2 + submitPresetForm() + } + document.addEventListener('click', submitDuringSelection, { once: true }) + try { + await user.click(option) + + expect(selectionStarted).toBe(true) + expect(mockCreate).not.toHaveBeenCalled() + expect(mockNavigate).not.toHaveBeenCalled() + } finally { + document.removeEventListener('click', submitDuringSelection) + await act(async () => { replacement.resolve(REPLACEMENT_SCENARIO) }) + } + }) + + it('rejects resolved metadata that does not match the selected identity', async () => { + const user = userEvent.setup() + await renderSelectedCreate(user) + mockGetScenario.mockResolvedValueOnce(SCENARIO) + + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + + expect(await screen.findByText(/metadata does not match the selected scenario/i)).toBeInTheDocument() + expect(screen.getByRole('combobox', { name: 'Scenario' })).toHaveValue(REPLACEMENT_SCENARIO.scenario_name) + expect(screen.getByRole('button', { name: 'Save preset' })).toBeDisabled() + submitPresetForm() + expect(mockCreate).not.toHaveBeenCalled() + expect(mockNavigate).not.toHaveBeenCalled() + }) + + it('keeps the latest selection and its edits when responses arrive in reverse order', async () => { + const user = userEvent.setup() + await renderSelectedCreate(user) + const older = deferred() + const latest = deferred() + mockGetScenario.mockReturnValueOnce(older.promise).mockReturnValueOnce(latest.promise) + + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + expect(screen.getByRole('combobox', { name: 'Scenario' })).toBeEnabled() + await selectScenario(user, LATEST_SCENARIO.scenario_name) + await act(async () => { latest.resolve(LATEST_SCENARIO) }) + await user.click(screen.getByRole('checkbox', { name: /^baseline$/i })) + await act(async () => { older.resolve(REPLACEMENT_SCENARIO) }) + + expect(screen.getByRole('combobox', { name: 'Scenario' })).toHaveValue(LATEST_SCENARIO.scenario_name) + expect(screen.getByRole('checkbox', { name: /^baseline$/i })).not.toBeChecked() + expect(screen.queryByRole('textbox', { name: 'languages' })).not.toBeInTheDocument() + const save = screen.getByRole('button', { name: 'Save preset' }) + expect(save).toBeEnabled() + await user.click(save) + expect(mockCreate).toHaveBeenCalledWith({ + name: 'nightly_probe', + scenario_name: LATEST_SCENARIO.scenario_name, + include_baseline: false, + }) + }) + + it.each(['resolve', 'reject'] as const)( + 'ignores an older request that %ss while the latest selection is pending', + async (outcome: 'resolve' | 'reject') => { + const user = userEvent.setup() + await renderSelectedCreate(user) + const older = deferred() + const latest = deferred() + mockGetScenario.mockReturnValueOnce(older.promise).mockReturnValueOnce(latest.promise) + + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + expect(screen.getByRole('combobox', { name: 'Scenario' })).toBeEnabled() + await selectScenario(user, LATEST_SCENARIO.scenario_name) + await act(async () => { + if (outcome === 'resolve') older.resolve(REPLACEMENT_SCENARIO) + else older.reject(apiError(503, 'Older scenario request failed.')) + }) + + expect(screen.getByText('Loading scenario...')).toBeInTheDocument() + expect(screen.getByRole('combobox', { name: 'Scenario' })).toHaveValue(LATEST_SCENARIO.scenario_name) + expect(screen.getByRole('button', { name: 'Save preset' })).toBeDisabled() + expect(screen.queryByText('Older scenario request failed.')).not.toBeInTheDocument() + submitPresetForm() + expect(mockCreate).not.toHaveBeenCalled() + await act(async () => { latest.resolve(LATEST_SCENARIO) }) + expect(screen.getByRole('button', { name: 'Save preset' })).toBeEnabled() + }, + ) + + it('keeps a failed selection and its error without saving the old configuration, then permits retry', async () => { + const user = userEvent.setup() + await renderSelectedCreate(user) + await user.type(screen.getByRole('textbox', { name: 'Description' }), 'Keep this draft.') + mockGetScenario.mockRejectedValueOnce(apiError(503, 'Replacement scenario is unavailable.')) + + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + + expect(await screen.findByText('Replacement scenario is unavailable.')).toBeInTheDocument() + expect(screen.getByRole('combobox', { name: 'Scenario' })).toHaveValue(REPLACEMENT_SCENARIO.scenario_name) + expect(screen.queryByText('Loading scenario...')).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Save preset' })).toBeDisabled() + submitPresetForm() + expect(screen.getByText('Replacement scenario is unavailable.')).toBeInTheDocument() + expect(mockCreate).not.toHaveBeenCalled() + expect(mockNavigate).not.toHaveBeenCalled() + + mockGetScenario.mockResolvedValueOnce(REPLACEMENT_SCENARIO) + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + expect(await screen.findByRole('textbox', { name: 'languages' })).toHaveValue('Spanish') + expect(screen.queryByText('Replacement scenario is unavailable.')).not.toBeInTheDocument() + await user.click(screen.getByRole('button', { name: 'Save preset' })) + expect(mockCreate).toHaveBeenCalledWith({ + name: 'nightly_probe', + scenario_name: REPLACEMENT_SCENARIO.scenario_name, + description: 'Keep this draft.', + }) + }) + + it('leaves pending replacement metadata behind when Cancel navigates to the library', async () => { + const user = userEvent.setup() + mockUseNavigate.mockImplementation( + jest.requireActual('react-router').useNavigate, + ) + await renderSelectedCreate(user) + const replacement = deferred() + mockGetScenario.mockReturnValueOnce(replacement.promise) + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + + await user.click(screen.getByRole('button', { name: 'Cancel' })) + expect(await screen.findByRole('heading', { name: 'Preset library' })).toBeInTheDocument() + await act(async () => { replacement.resolve(REPLACEMENT_SCENARIO) }) + + expect(screen.getByRole('heading', { name: 'Preset library' })).toBeInTheDocument() + expect(screen.queryByText('Loading scenario...')).not.toBeInTheDocument() + expect(mockCreate).not.toHaveBeenCalled() + expect(mockUpdate).not.toHaveBeenCalled() + }) + + it('does not apply replacement metadata after unmount', async () => { + const user = userEvent.setup() + const view = await renderSelectedCreate(user) + const replacement = deferred() + mockGetScenario.mockReturnValueOnce(replacement.promise) + const initializeConfig = jest.spyOn(scenarioConfigForm, 'initialScenarioConfigState') + try { + await selectScenario(user, REPLACEMENT_SCENARIO.scenario_name) + view.unmount() + await act(async () => { replacement.resolve(REPLACEMENT_SCENARIO) }) + + expect(initializeConfig).not.toHaveBeenCalled() + expect(mockCreate).not.toHaveBeenCalled() + expect(mockUpdate).not.toHaveBeenCalled() + expect(mockNavigate).not.toHaveBeenCalled() + } finally { + initializeConfig.mockRestore() + } + }) + + it('does not cancel an accepted save on unmount or accept a duplicate programmatic submit', async () => { + const user = userEvent.setup() + const view = await renderSelectedCreate(user) + const saving = deferred() + mockCreate.mockReturnValueOnce(saving.promise) + + await user.click(screen.getByRole('button', { name: 'Save preset' })) + expect(mockCreate).toHaveBeenCalledTimes(1) + expect(screen.getByRole('button', { name: 'Saving...' })).toBeDisabled() + expect(screen.getByRole('combobox', { name: 'Scenario' })).toBeDisabled() + submitPresetForm() + view.unmount() + await act(async () => { saving.resolve({ preset: STORED_PRESET, version: 'v1', issues: [] }) }) + + expect(mockCreate).toHaveBeenCalledTimes(1) + expect(mockNavigate).toHaveBeenCalledTimes(1) + expect(mockNavigate).toHaveBeenCalledWith('/registry/scenario-presets') + }) +}) + describe('ScenarioPresetEditor edit mode', () => { it('loads the stored preset and pins its name and scenario', async () => { renderEdit() @@ -285,6 +574,37 @@ describe('ScenarioPresetEditor edit mode', () => { expect(mockNavigate).toHaveBeenCalledWith('/registry/scenario-presets') }) + it('preserves stored list values when edit metadata finishes loading', async () => { + const user = userEvent.setup() + const metadata = deferred() + const languages = ['French', 'German'] + mockGet.mockResolvedValue({ + preset: { ...STORED_PRESET, scenario_params: { languages } }, + version: 'v1', + issues: [], + }) + mockGetScenario.mockReturnValueOnce(metadata.promise) + renderEdit() + + expect(await screen.findByRole('button', { name: 'Save preset' })).toBeDisabled() + await act(async () => { + metadata.resolve(makeScenario({ + supported_parameters: [ + { name: 'languages', type_name: 'list[str]', is_list: true, required: false, default: ['English'] }, + ], + })) + }) + expect(await screen.findByRole('textbox', { name: 'languages' })).toHaveValue('French, German') + await user.type(screen.getByRole('textbox', { name: 'Description' }), ' Updated.') + await user.click(screen.getByRole('button', { name: 'Save preset' })) + + expect(mockUpdate).toHaveBeenCalledWith( + STORED_PRESET.name, + expect.objectContaining({ techniques: STORED_PRESET.techniques, scenario_params: { languages } }), + 'v1', + ) + }) + it('reports a version conflict instead of navigating away', async () => { const user = userEvent.setup() mockUpdate.mockRejectedValue(apiError(409, 'The preset changed since it was loaded.')) diff --git a/frontend/src/components/ScenarioPresets/ScenarioPresetEditor.tsx b/frontend/src/components/ScenarioPresets/ScenarioPresetEditor.tsx index 184ef99bed..5a9e3f2956 100644 --- a/frontend/src/components/ScenarioPresets/ScenarioPresetEditor.tsx +++ b/frontend/src/components/ScenarioPresets/ScenarioPresetEditor.tsx @@ -1,4 +1,4 @@ -import { type FormEvent, useCallback, useEffect, useMemo, useState } from 'react' +import { type FormEvent, useCallback, useEffect, useMemo, useRef, useState } from 'react' import { Button, @@ -87,8 +87,11 @@ function ScenarioPresetEditorContent({ mode, presetName }: ScenarioPresetEditorC const [name, setName] = useState(mode === 'edit' ? decodedName : '') const [description, setDescription] = useState('') const [showInScanner, setShowInScanner] = useState(true) + const [selectedScenarioName, setSelectedScenarioName] = useState('') const [scenario, setScenario] = useState(null) const [scenarioLoading, setScenarioLoading] = useState(false) + const scenarioRequestSequence = useRef(0) + const [resolvedScenarioRequestSequence, setResolvedScenarioRequestSequence] = useState(null) const [config, setConfig] = useState(null) const [scenarioUnavailable, setScenarioUnavailable] = useState(null) const [datasetError, setDatasetError] = useState(null) @@ -123,6 +126,8 @@ function ScenarioPresetEditorContent({ mode, presetName }: ScenarioPresetEditorC setName(response.preset.name) setDescription(response.preset.description ?? '') setShowInScanner(response.preset.show_in_scanner !== false) + setSelectedScenarioName(response.preset.scenario_name) + setScenarioLoading(true) setStatus('ready') // The list endpoint omits run-size estimates, so the preset's scenario is @@ -135,9 +140,15 @@ function ScenarioPresetEditorContent({ mode, presetName }: ScenarioPresetEditorC if (cancelled) return setScenario(full) setConfig(presetToConfigState(full, response.preset)) + setResolvedScenarioRequestSequence(scenarioRequestSequence.current) } catch (scenarioErr) { if (cancelled) return setScenarioUnavailable(toApiError(scenarioErr).detail) + } finally { + if (!cancelled) { + setScenarioLoading(false) + setValidationError(null) + } } } catch (err) { if (cancelled) return @@ -150,26 +161,44 @@ function ScenarioPresetEditorContent({ mode, presetName }: ScenarioPresetEditorC void load() return () => { cancelled = true + scenarioRequestSequence.current += 1 } }, [decodedName, mode]) const handleScenarioChange = useCallback(async (nextScenarioName: string): Promise => { + const requestSequence = scenarioRequestSequence.current + 1 + scenarioRequestSequence.current = requestSequence + setSelectedScenarioName(nextScenarioName) + setScenario(null) + setConfig(null) setScenarioLoading(true) setSaveError(null) setValidationError(null) + setDatasetError(null) setDatasetProjection(null) setDatasetProjectionResult(null) try { const full = await scenariosApi.getScenario(nextScenarioName) + if (requestSequence !== scenarioRequestSequence.current) return + if (full.scenario_name !== nextScenarioName) { + setSaveError('Loaded metadata does not match the selected scenario. Select it again to retry.') + return + } setScenario(full) setConfig(initialScenarioConfigState(full)) + setResolvedScenarioRequestSequence(requestSequence) } catch (err) { + if (requestSequence !== scenarioRequestSequence.current) return setSaveError(toApiError(err).detail) } finally { - setScenarioLoading(false) + if (requestSequence === scenarioRequestSequence.current) { + setScenarioLoading(false) + setValidationError(null) + } } }, []) + const scenarioIsCurrent = scenario !== null && scenario.scenario_name === selectedScenarioName const datasetMetadata = useScenarioDatasets() const datasetBaseline = useMemo( () => (scenario @@ -205,7 +234,7 @@ function ScenarioPresetEditorContent({ mode, presetName }: ScenarioPresetEditorC [config, dynamicParameters], ) const scenarioName = scenario?.scenario_name - const projectionRequest = !scenarioLoading && builtConfig?.ok ? builtConfig.config : null + const projectionRequest = !scenarioLoading && scenarioIsCurrent && builtConfig?.ok ? builtConfig.config : null const projectionIsCurrent = datasetProjectionResult?.request === projectionRequest && datasetProjectionResult?.scenarioName === scenarioName const datasetProjectionLoading = Boolean(scenarioName && projectionRequest && !projectionIsCurrent) @@ -251,6 +280,15 @@ function ScenarioPresetEditorContent({ mode, presetName }: ScenarioPresetEditorC if (saving) { return } + // A programmatic submit can run before the selection's state updates commit. + const selectionPending = scenarioLoading + || (scenarioIsCurrent && resolvedScenarioRequestSequence !== scenarioRequestSequence.current) + if (selectionPending || !scenarioIsCurrent) { + setValidationError(selectionPending + ? 'Wait for the selected scenario to finish loading.' + : 'Select a scenario.') + return + } setSaveError(null) const trimmedName = name.trim() @@ -406,9 +444,9 @@ function ScenarioPresetEditorContent({ mode, presetName }: ScenarioPresetEditorC { if (data.optionValue) { @@ -440,7 +478,7 @@ function ScenarioPresetEditorContent({ mode, presetName }: ScenarioPresetEditorC {scenarioLoading && } - {scenario && config && ( + {scenarioIsCurrent && scenario && config && ( <> {saving ? 'Saving...' : 'Save preset'}