From d0881fd6762658993f1dad8d18aada82d82c79a6 Mon Sep 17 00:00:00 2001 From: dovvnloading Date: Sun, 26 Jul 2026 09:34:58 -0400 Subject: [PATCH] R7.4a second pass: fix 3 defects the first fix pass introduced, plus 5 it missed A user-requested re-audit aimed specifically at the R7.4a FIX PASS (rather than the original feature) found that the fix pass itself introduced new bugs and shipped two regression tests that could not fail. Root cause: the first pass fixed "coerce untyped wire input" for exactly one argument in one handler instead of applying it as a rule, so the two new code paths that same pass added each re-opened the identical hole. Introduced by the first fix pass, now fixed: - _redact() was handed the RAW wire api_key, so a non-string key raised TypeError inside an except block - where its own try cannot catch it - escaping the handler and pinning that provider's catalog at "loading" for the process lifetime, permanently disabling the Load button in every tab. Coerced at the top of load_api_models + isinstance guard in _redact. - api_catalog_state[provider] used the raw provider as a dict key, so an unhashable JSON value crashed the handler where the pre-fix flat cells could not. - That dict was written on the unsupported-provider reject path before returning, growing without bound on a client-controlled key (200 junk providers left 200 entries; now 0). Tests that could not fail, now mutation-verified: - The F1 concurrency test injected its "concurrent" write inside the mocked initialize_api, which sits BEFORE the racy read, so it passed against the very ordering it was meant to catch. Rewritten to inject inside _apply - the actual read/write window. - The F2 stale-provider guard had zero coverage; deleting it left the suite green. Now covered. Missed by the first review entirely: - The write-only-key design left an already-configured user with a blank key field, and load_api_models had no server-side fallback, so Load ALWAYS failed for them with "API key not configured" while the same payload reported apiKeyConfigured=true. Now falls back to the stored key (still never sent to the client), with a clean message when genuinely unconfigured - which also closes a hole where a blank-key Load could succeed off an ambient env var and silently repoint the live provider. - The OpenAI "Base URL must not be empty" guard both legacy predecessors had was dropped on load and save. On save a persisted "" is read back verbatim (SettingsManager's default only covers a missing key), silently re-pointing the saved key at api.openai.com. Restored on both paths. - Reset API Settings wiped every provider's key on a single click, dropping legacy's "This cannot be undone" gate. Restored as the same two-step confirm the legacy island and ChatLibraryDialog already use. Verification: 2438 backend pytest (was 2430), 1066 frontend vitest, compileall/tsc/eslint/build clean, burn-down pin unchanged at 152/84/68, both rewritten tests mutation-verified, and a second live WS drive against a real backend covering all five new guard paths (isolated scratch settings file; the real ~/.graphlink/session.dat was never touched). Co-Authored-By: Claude Opus 5 --- backend/settings.py | 116 ++++++++- backend/tests/test_settings.py | 231 ++++++++++++++++-- web_ui/src/app/chrome/SettingsDialog.test.tsx | 32 ++- web_ui/src/app/chrome/SettingsDialog.tsx | 59 +++-- 4 files changed, 396 insertions(+), 42 deletions(-) diff --git a/backend/settings.py b/backend/settings.py index 5ab0cd9..687eeed 100644 --- a/backend/settings.py +++ b/backend/settings.py @@ -66,6 +66,15 @@ config.TASK_WEB_SUMMARIZE, ) +# The three providers the page can actually display. Used to bound the +# per-provider catalog-state dict: without it, that dict is keyed by a +# raw client-supplied string and grows without limit. +_KNOWN_API_PROVIDERS = ( + config.API_PROVIDER_OPENAI, + config.API_PROVIDER_ANTHROPIC, + config.API_PROVIDER_GEMINI, +) + # Every persistence-touching mutation runs in a worker thread # (asyncio.to_thread) so SettingsManager._save_state's json-dump + fsync + # atomic-replace never stalls the event loop (and with it, every session's @@ -82,7 +91,7 @@ def _apply(mutation: Callable[..., None], *args: Any) -> None: mutation(*args) -def _redact(text: str, secret: str) -> str: +def _redact(text: str, secret: Any) -> str: # Post-review fix: some HTTP client libraries embed request parameters # (including a rejected API key) directly in exception text - the # legacy bridge this page replaces redacted for exactly this reason @@ -90,7 +99,15 @@ def _redact(text: str, secret: str) -> str: # Applied before any exception text reaches a WS-broadcast message, so # the write-only-key guarantee holds on the failure path too, not just # the happy path. - if not secret: + # + # Second-pass fix: the isinstance check is load-bearing, not defensive + # noise. Intent arguments arrive as raw JSON off the wire with no + # coercion anywhere in the dispatch path, and both call sites are + # inside `except` blocks - where a TypeError from str.replace would + # NOT be caught by its own `try` and would escape the handler, leaving + # the catalog pinned at "loading" (which disables the Load button) for + # the life of the process. + if not isinstance(secret, str) or not secret: return text return text.replace(secret, "***") @@ -232,12 +249,60 @@ async def set_viewing_api_provider(provider: str): await bus.publish("app-settings") async def load_api_models(provider: str, api_key: str, base_url: str = ""): + # Second-pass fix: coerce every wire argument BEFORE it is used for + # anything - `provider` in particular is used as a dict key below, + # and an unhashable JSON value (array/object) would raise TypeError + # straight out of the handler. Intent args reach here as raw JSON + # with no validation anywhere in the dispatch path. + provider = str(provider) + base_url = str(base_url).strip() + # Gemini has no live catalog endpoint wired (legacy never showed the # Load button for it either) - a stale/misbehaving client asking # anyway gets a clean error rather than a confusing initialize_api # call with the wrong semantics. if provider not in (config.API_PROVIDER_OPENAI, config.API_PROVIDER_ANTHROPIC): - api_catalog_state[provider] = {"status": "error", "message": f"{provider} does not support catalog refresh."} + # Only a provider the UI can actually VIEW gets a stored status + # slot: build_payload surfaces the slot for the viewed provider + # only, so a slot for an unknown string could never be read - + # it would just grow this dict without bound on a key the client + # controls. + if provider in _KNOWN_API_PROVIDERS: + api_catalog_state[provider] = { + "status": "error", + "message": f"{provider} does not support catalog refresh.", + } + await bus.publish("app-settings") + return + + if provider == config.API_PROVIDER_OPENAI and not base_url: + # Restores a guard both legacy predecessors had. Without it an + # empty Base URL silently falls through to api_provider's own + # "https://api.openai.com/v1" default, sending a key meant for a + # self-hosted proxy to OpenAI's public API instead. + api_catalog_state[provider] = { + "status": "error", + "message": "Please enter the Base URL for the OpenAI-compatible provider.", + } + await bus.publish("app-settings") + return + + # This page deliberately never sends the saved key to the client + # (see the module docstring), so a user who already has a key saved + # opens the dialog with a blank field. Legacy pre-filled the field + # and therefore always had a key to test with; the write-only port + # has to make up for that here instead, or Load is simply unusable + # for every already-configured user. Falling back server-side keeps + # the plaintext key off the wire entirely. + key = str(api_key).strip() + if not key: + key = ( + manager.get_openai_key() + if provider == config.API_PROVIDER_OPENAI + else manager.get_anthropic_key() + ) + if not key: + api_catalog_state[provider] = {"status": "error", "message": "Please enter the API Key."} await bus.publish("app-settings") return @@ -254,8 +319,8 @@ async def load_api_models(provider: str, api_key: str, base_url: str = ""): await asyncio.to_thread( api_provider.initialize_api, provider, - str(api_key), - str(base_url) if provider == config.API_PROVIDER_OPENAI else None, + key, + base_url if provider == config.API_PROVIDER_OPENAI else None, ) descriptors = await asyncio.to_thread(api_provider.get_available_model_descriptors) # Post-review fix: api_provider's provider globals are process- @@ -291,7 +356,11 @@ async def load_api_models(provider: str, api_key: str, base_url: str = ""): for descriptor in descriptors ] except Exception as exc: # noqa: BLE001 - redacted, then surfaced, matching legacy's error label - message = _redact(str(exc), api_key) + # Redact the EFFECTIVE key (which may be the stored one picked + # up by the fallback above), not the submitted argument - the + # effective key is what actually reached the provider SDK and + # so is what can come back embedded in its exception text. + message = _redact(str(exc), key) api_catalog_state[provider] = { "status": "error", "message": f"Catalog refresh failed: {message}\nSaved/custom model IDs remain usable if the endpoint supports them.", @@ -319,6 +388,18 @@ async def save_api_configuration(provider: str, base_url: str, api_key: str, mod if not isinstance(models_by_task, dict): models_by_task = {} + if provider == config.API_PROVIDER_OPENAI and not base_url: + # Same guard as load_api_models, and the more important of the + # two: an empty base_url persisted here is read straight back + # by SettingsManager.get_api_base_url (whose own default only + # applies to a MISSING key, not a stored ""), so every later + # provider bootstrap silently re-points the saved key at + # api.openai.com. + if notifications is not None: + notifications.show("Please enter the Base URL for the OpenAI-compatible provider.", "warning") + await bus.publish("notification") + return + if not api_key: if notifications is not None: notifications.show("Please enter your API Key.", "warning") @@ -384,7 +465,28 @@ def _persist() -> None: await bus.publish("app-settings") async def reset_api_settings(): - await asyncio.to_thread(_apply, manager.reset_api_settings) + def _reset() -> None: + manager.reset_api_settings() + # SettingsManager.reset_api_settings intentionally leaves + # api_model_catalog_by_provider alone (it is shared with the + # legacy Qt bridge), but "All API settings have been cleared" + # has to mean it - otherwise the page keeps offering the old + # provider's model catalog after the reset. Clearing through + # the existing public setter rather than editing the shared + # legacy module. + for known in _KNOWN_API_PROVIDERS: + manager.set_api_model_catalog([], known) + + await asyncio.to_thread(_apply, _reset) + # Transient per-provider status/message is UI state, not manager + # state, so it has to be cleared here too - otherwise the previous + # "Catalog refreshed - N model(s)" success banner survives a reset + # that just deleted that very catalog. + api_catalog_state.clear() + # Legacy's reset_settings snapped the provider dropdown back to + # OpenAI-Compatible; reset_api_settings restores exactly that as + # the persisted provider, so the viewed provider follows it. + viewing_api_provider["value"] = manager.get_api_provider() if notifications is not None: notifications.show("All API settings have been cleared.", "success") await bus.publish("notification") diff --git a/backend/tests/test_settings.py b/backend/tests/test_settings.py index 09c4066..2fa3831 100644 --- a/backend/tests/test_settings.py +++ b/backend/tests/test_settings.py @@ -7,6 +7,7 @@ import api_provider import graphlink_task_config as config +import backend.settings as settings_module from backend.events import SessionBus from backend.notifications import NotificationState from backend.settings import register_settings, settings_payload @@ -454,6 +455,149 @@ def _boom(*a, **k): assert "***" in payload["apiCatalogMessage"] +def test_load_api_models_falls_back_to_the_saved_key_when_the_field_is_blank(manager, monkeypatch): + # This page never sends the saved key to the client, so an already- + # configured user opens the dialog with a blank key field. Without a + # server-side fallback, Load is simply unusable for them - it fails + # with "API key not configured" while the same payload reports + # apiKeyConfigured=true. Legacy avoided this by pre-filling the field. + seen = [] + monkeypatch.setattr(api_provider, "initialize_api", lambda provider, key, base_url=None: seen.append(key)) + monkeypatch.setattr(api_provider, "API_PROVIDER_TYPE", config.API_PROVIDER_OPENAI) + monkeypatch.setattr(api_provider, "get_available_model_descriptors", lambda: []) + manager.set_api_settings(config.API_PROVIDER_OPENAI, "https://x/v1", "sk-saved-openai", "", "") + bus = SessionBus("settings-load-key-fallback-test") + register_settings(bus, manager) + + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_OPENAI, "", "https://x/v1"])) + + assert seen == ["sk-saved-openai"] + + +def test_load_api_models_reports_a_clean_error_when_no_key_is_available_anywhere(manager, monkeypatch): + called = [] + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: called.append(a)) + bus = SessionBus("settings-load-no-key-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_OPENAI, "", "https://x/v1"])) + + assert called == [] # never contacts a provider with an empty key + payload = recorder.messages[-1]["payload"] + assert payload["apiCatalogStatus"] == "error" + assert payload["apiCatalogMessage"] == "Please enter the API Key." + + +def test_load_api_models_requires_a_base_url_for_the_openai_compatible_provider(manager, monkeypatch): + # Without this an empty Base URL falls through to api_provider's own + # api.openai.com default, sending a self-hosted-proxy key to OpenAI. + called = [] + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: called.append(a)) + bus = SessionBus("settings-load-no-base-url-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_OPENAI, "sk-test", " "])) + + assert called == [] + assert "Base URL" in recorder.messages[-1]["payload"]["apiCatalogMessage"] + + +def test_save_api_configuration_requires_a_base_url_for_the_openai_compatible_provider(manager): + # Worse on the save path than the load path: an empty base_url is + # PERSISTED, and SettingsManager.get_api_base_url's default only covers + # a missing key, not a stored "", so every later bootstrap silently + # re-points the saved key at api.openai.com. + notifications = NotificationState() + bus = SessionBus("settings-save-no-base-url-test") + bus.register_topic("notification", notifications.payload) + register_settings(bus, manager, notifications) + + asyncio.run( + bus.dispatch_intent( + "app-settings", "saveApiConfiguration", [config.API_PROVIDER_OPENAI, " ", "sk-test", _six_task_models()] + ) + ) + + assert manager.get_openai_key() == "" + assert notifications.msg_type == "warning" + assert "Base URL" in notifications.message + + +def test_load_api_models_survives_non_string_wire_arguments(manager, monkeypatch): + # Intent args arrive as raw JSON with no validation anywhere in the + # dispatch path. Two of these used to raise straight out of the + # handler: a non-str key hit str.replace inside the except block (where + # its own try cannot catch it), and an unhashable provider was used + # directly as a dict key. + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("nope"))) + bus = SessionBus("settings-load-wire-types-test") + register_settings(bus, manager) + + for bad_args in ( + [config.API_PROVIDER_OPENAI, 12345, "https://x/v1"], + [config.API_PROVIDER_OPENAI, ["a"], "https://x/v1"], + [config.API_PROVIDER_OPENAI, {"k": "v"}, "https://x/v1"], + [["unhashable"], "sk-test", "https://x/v1"], + [{"also": "unhashable"}, "sk-test", "https://x/v1"], + [config.API_PROVIDER_OPENAI, "sk-test", 99], + ): + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", bad_args)) # must not raise + + +def test_load_api_models_does_not_grow_catalog_state_for_unknown_providers(manager): + # api_catalog_state is keyed by a client-supplied string; only the + # three providers the UI can actually display may occupy a slot, or a + # misbehaving client grows this dict without bound for the life of the + # session. + bus = SessionBus("settings-load-unbounded-growth-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + for i in range(50): + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [f"junk-provider-{i}", "sk", "u"])) + + # Nothing was published at all for an unknown provider, and the viewed + # provider's own state is untouched. + asyncio.run(bus.publish("app-settings")) + assert recorder.messages[-1]["payload"]["apiCatalogStatus"] == "idle" + + +def test_reset_api_settings_also_clears_the_catalog_and_snaps_the_viewed_provider_back(manager, monkeypatch): + from graphlink_model_catalog import ModelDescriptor + + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: None) + monkeypatch.setattr(api_provider, "API_PROVIDER_TYPE", config.API_PROVIDER_ANTHROPIC) + monkeypatch.setattr( + api_provider, + "get_available_model_descriptors", + lambda: [ModelDescriptor(model_id="claude-sonnet", provider=config.API_PROVIDER_ANTHROPIC)], + ) + bus = SessionBus("settings-reset-clears-catalog-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + asyncio.run(bus.dispatch_intent("app-settings", "setViewingApiProvider", [config.API_PROVIDER_ANTHROPIC])) + asyncio.run(bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_ANTHROPIC, "sk-ant", ""])) + assert recorder.messages[-1]["payload"]["apiCatalogStatus"] == "success" + + asyncio.run(bus.dispatch_intent("app-settings", "resetApiSettings", [])) + + payload = recorder.messages[-1]["payload"] + # "All API settings have been cleared" has to actually mean it: no + # stale success banner, no stale catalog, and the viewed provider + # follows the reset back to the default (as legacy's reset did). + assert payload["apiCatalogStatus"] == "idle" + assert payload["apiModelCatalog"] == [] + assert payload["viewingApiProvider"] == config.API_PROVIDER_OPENAI + assert manager.get_api_model_catalog(config.API_PROVIDER_ANTHROPIC) == [] + + def test_save_api_configuration_rejects_a_non_dict_models_argument_without_crashing(manager): # Regression test: a non-dict 4th argument (e.g. a WS client sending # null) used to raise an uncaught AttributeError from models_by_task.get @@ -502,20 +646,36 @@ def test_save_api_configuration_preserves_other_providers_keys(manager, monkeypa def test_save_api_configuration_reads_other_providers_keys_atomically_with_its_own_write(manager, monkeypatch): # Regression test for a post-review R7.4a race: the original code read # manager.get_anthropic_key()/get_openai_key()/get_gemini_key() for the - # "keep as-is" providers on the event loop, BEFORE awaiting - # initialize_api and persisting - so a second, concurrent - # saveApiConfiguration for a different provider (e.g. two browser tabs - # on the same session) landing in between got its own freshly-committed - # key silently reverted by this call's stale pre-read. The fix moved - # that read inside _persist(), which only ever runs under _manager_lock - # - this test simulates the interleaving by having initialize_api's own - # mock perform the "concurrent" write, proving the later read (inside - # _persist) picks up the fresh value rather than a stale one captured - # earlier. - def _simulate_concurrent_anthropic_save_during_our_own_initialize_api(*_args, **_kwargs): - manager.set_api_settings(config.API_PROVIDER_ANTHROPIC, "", "", "sk-concurrent-ant", "") - - monkeypatch.setattr(api_provider, "initialize_api", _simulate_concurrent_anthropic_save_during_our_own_initialize_api) + # "keep as-is" providers on the event loop, OUTSIDE _manager_lock, then + # wrote them back much later inside the locked persist - so a second, + # concurrent saveApiConfiguration for a different provider (two browser + # tabs share one session) committing in that window got its own + # freshly-saved key silently reverted by this call's stale pre-read. + # + # The injection point matters and is the whole point of this test. An + # earlier version of it injected the "concurrent" write inside the + # mocked initialize_api, which sits BEFORE the racy read - so it also + # passed against the still-racy ordering it was meant to catch (a + # second-pass audit caught that). The write has to land AFTER the read + # the buggy version performed and BEFORE the persist, which is exactly + # the window _apply() opens: patching _apply to run the concurrent + # commit first reproduces "another connection committed while this + # save was between its read and its write". + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: None) + + real_apply = settings_module._apply + injected = {"done": False} + + def _apply_with_a_concurrent_commit_in_the_window(mutation, *args): + if not injected["done"]: + injected["done"] = True + # Another connection's save lands and commits, in full, right + # here - after our read (if the code reads too early) and + # before our own write. + manager.set_api_settings(config.API_PROVIDER_ANTHROPIC, "", "", "sk-concurrent-ant", "") + return real_apply(mutation, *args) + + monkeypatch.setattr(settings_module, "_apply", _apply_with_a_concurrent_commit_in_the_window) bus = SessionBus("settings-save-race-regression-test") register_settings(bus, manager) @@ -525,13 +685,52 @@ def _simulate_concurrent_anthropic_save_during_our_own_initialize_api(*_args, ** ) ) + assert injected["done"], "the concurrent commit never ran - the test no longer exercises the window" assert manager.get_openai_key() == "sk-openai" # Must NOT be reverted to "" - our own save's "keep Anthropic as-is" - # logic must see the concurrently-committed value, not a stale read - # taken before the concurrent write happened. + # read has to happen inside the same locked section as its write, so it + # sees the concurrently-committed value rather than a stale earlier one. assert manager.get_anthropic_key() == "sk-concurrent-ant" +def test_load_api_models_aborts_when_another_request_repointed_the_provider_globals(manager, monkeypatch): + # Regression test for the F2 stale-provider guard, which shipped with + # no coverage of its own (a second-pass audit found that deleting the + # guard left the whole suite green). api_provider's provider state is + # process-global and initialize_api/get_available_model_descriptors are + # two separate to_thread hops - a concurrent call for a different + # provider landing between them makes the descriptors describe someone + # else's provider. The guard must refuse to persist that under this + # call's provider name. + from graphlink_model_catalog import ModelDescriptor + + monkeypatch.setattr(api_provider, "initialize_api", lambda *a, **k: None) + monkeypatch.setattr(api_provider, "API_PROVIDER_TYPE", config.API_PROVIDER_OPENAI) + + def _descriptors_after_someone_else_switched_provider(): + # Simulates another connection's initialize_api completing in the + # await gap and repointing the process-global provider state. + monkeypatch.setattr(api_provider, "API_PROVIDER_TYPE", config.API_PROVIDER_ANTHROPIC) + return [ModelDescriptor(model_id="claude-sonnet", provider=config.API_PROVIDER_ANTHROPIC)] + + monkeypatch.setattr(api_provider, "get_available_model_descriptors", _descriptors_after_someone_else_switched_provider) + bus = SessionBus("settings-stale-provider-guard-test") + register_settings(bus, manager) + recorder = Recorder() + bus.attach(recorder) + + asyncio.run( + bus.dispatch_intent("app-settings", "loadApiModels", [config.API_PROVIDER_OPENAI, "sk-test", "https://x/v1"]) + ) + + payload = recorder.messages[-1]["payload"] + assert payload["apiCatalogStatus"] == "error" + assert "another request" in payload["apiCatalogMessage"] + # The decisive assertion: Anthropic's models must NOT have been filed + # under OpenAI-Compatible. + assert manager.get_api_model_catalog(config.API_PROVIDER_OPENAI) == [] + + def test_catalog_state_is_isolated_per_provider(manager, monkeypatch): # Regression test: apiCatalogStatus/apiCatalogMessage used to be one # flat pair of session-local cells shared by every provider, so a load diff --git a/web_ui/src/app/chrome/SettingsDialog.test.tsx b/web_ui/src/app/chrome/SettingsDialog.test.tsx index 63bcedb..6c8d33f 100644 --- a/web_ui/src/app/chrome/SettingsDialog.test.tsx +++ b/web_ui/src/app/chrome/SettingsDialog.test.tsx @@ -212,16 +212,43 @@ describe("SettingsDialog", () => { expect(screen.getByPlaceholderText("Enter your API key...")).toHaveValue(""); }); - it("Reset API Settings fires resetApiSettings", async () => { + it("Reset API Settings does nothing on the first click - it only arms the confirmation", async () => { + // Reset irreversibly wipes every provider's saved key, so it must not + // be a single-click action. Legacy gated it behind a "This cannot be + // undone" Yes/No prompt. const { user, push, intents } = setup(); await goToApiEndpoint(user, push); await user.click(screen.getByText("Reset API Settings")); + expect(intents.some(([, intent]) => intent === "resetApiSettings")).toBe(false); + expect(screen.getByText(/cannot be undone/)).toBeInTheDocument(); + expect(screen.getByText("Confirm Reset")).toBeInTheDocument(); + }); + + it("Cancel backs out of an armed Reset without firing anything", async () => { + const { user, push, intents } = setup(); + await goToApiEndpoint(user, push); + + await user.click(screen.getByText("Reset API Settings")); + await user.click(screen.getByText("Cancel")); + + expect(intents.some(([, intent]) => intent === "resetApiSettings")).toBe(false); + expect(screen.queryByText("Confirm Reset")).toBeNull(); + expect(screen.getByText("Reset API Settings")).toBeInTheDocument(); + }); + + it("Confirm Reset fires resetApiSettings", async () => { + const { user, push, intents } = setup(); + await goToApiEndpoint(user, push); + + await user.click(screen.getByText("Reset API Settings")); + await user.click(screen.getByText("Confirm Reset")); + expect(intents).toContainEqual(["app-settings", "resetApiSettings", []]); }); - it("Reset API Settings clears the local Base URL and model drafts too, not just the key", async () => { + it("Confirm Reset clears the local Base URL and model drafts too, not just the key", async () => { // Regression test: Reset originally only cleared draftApiKey, leaving // draftBaseUrl/draftModels showing their pre-reset values (the resync // effect only fires on a genuine provider switch, which Reset doesn't @@ -235,6 +262,7 @@ describe("SettingsDialog", () => { await user.type(screen.getByLabelText("Chat, Explain, Takeaways (main model)"), "gpt-4o"); await user.click(screen.getByText("Reset API Settings")); + await user.click(screen.getByText("Confirm Reset")); expect(screen.getByPlaceholderText("https://api.openai.com/v1")).toHaveValue("https://api.openai.com/v1"); expect(screen.getByLabelText("Chat, Explain, Takeaways (main model)")).toHaveValue(""); diff --git a/web_ui/src/app/chrome/SettingsDialog.tsx b/web_ui/src/app/chrome/SettingsDialog.tsx index 44a7445..88d99c2 100644 --- a/web_ui/src/app/chrome/SettingsDialog.tsx +++ b/web_ui/src/app/chrome/SettingsDialog.tsx @@ -234,6 +234,7 @@ function ApiProviderPage({ const [draftApiKey, setDraftApiKey] = useState(""); const [draftBaseUrl, setDraftBaseUrl] = useState(state.apiBaseUrl); const [draftModels, setDraftModels] = useState>(state.apiModels); + const [confirmingReset, setConfirmingReset] = useState(false); const [lastViewingProvider, setLastViewingProvider] = useState(viewingProvider); if (viewingProvider !== lastViewingProvider) { @@ -241,6 +242,8 @@ function ApiProviderPage({ setDraftApiKey(""); setDraftBaseUrl(state.apiBaseUrl); setDraftModels(state.apiModels); + // Don't leave a Reset armed across a provider switch. + setConfirmingReset(false); } const catalogModelIds = state.apiModelCatalog.map((entry) => entry.modelId); @@ -349,24 +352,46 @@ function ApiProviderPage({ ))} + {confirmingReset && ( +

+ This clears all saved API keys and model configurations and cannot be undone. +

+ )} +
- + {/* Reset wipes every provider's key irreversibly. Legacy gated it + behind a "This cannot be undone" Yes/No prompt; this is the same + two-step confirm the legacy settings island and ChatLibraryDialog + already use, rather than a one-click destructive button. */} + {confirmingReset ? ( + <> + + + + ) : ( + + )}