Merge pull request #1214 from ouroboros-agent/codex/settings-followup-20260922

Fix account model refresh across Settings load races

docs/architecture/03-web-ui-pages-and-buttons.md

@@ -279,6 +279,8 @@ The synchronous lock-owning apply executor publishes process-local stage observa
 
 Settings has Accounts, Secrets, Models, Agents, Behavior, Appearance, Advanced and About tabs — a sequence from connections to runtime detail. Accounts: managed subscriptions and their shared service banner, API providers, custom compatible endpoints, local runtime entry points, and the optional non-loopback network gate. Secrets: known provider/integration secrets, skill-requested keys and owner-defined custom keys, without returning stored values. Models: compact source/model/account role rows, ordered fallbacks, context assertions and effort lanes. Agents: task actors and review lanes, with delegation permissions, per-root and depth limits and subagent path roots; their accounts are managed in Accounts. Behavior: context, safety-supervisor coverage, task acceptance, self-evolution, prompt-cache posture. Appearance: the client-local theme choice described above, never a runtime-settings value. Advanced: process, timeout, local-model, integration, source-control and cleanup controls (worker count is process capacity, so it lives here). About reports application/runtime identity. Keys, defaults and per-key semantics are the §7 Default settings table, not this chapter. Models, Available subagents and Review lanes share ONE grouped source select owned by `web/modules/route_editor_primitives.js` (`routeChoiceGroups`, `configuredApiProviders`): the owner picks a source and the editor composes the stored id, so the provider prefixes (`provider::model`, `claudexor::source=model`, `harness=model`) are serialization only, never owner input.
 
+`settings_catalog.js` owns model-catalog reads and the Accounts subscription: a changed confirmed account or recovery from a failed read refreshes discovery; identical settled status stays quiet, and `settings.js` arms/disposes the subscription with the page. Catalog updates preserve the owner's current model draft.
+
 The Settings client validates the whole current draft before Save; a local error keeps every value available for correction and sends no partial save (`settings_controls.js` keeps custom-key collection pure and dirty reads passive). Ordinary refresh and failed writes preserve current edits, leaving or explicitly reloading a dirty draft asks first, the write response distinguishes saved, unsaved and unknown outcomes, and there is no durable cross-page draft store or secret persistence.
 
 Advanced also hosts the settings sections live extensions register, hydrated before they are shown: the client reads each declarative `form`/`action` component's own `route` with `GET`, and a 2xx JSON object of saved values pre-fills exactly the fields it names through `renderSafeField` (passwords are never hydrated), so Save rewrites current values instead of posting first options over them. A `404`/`405` means the skill serves no current values and the form renders from its declared defaults with Save enabled; a transport or server failure renders it with Save disabled and a note pointing at Reload Settings, because a blank form must never overwrite values the client could not read.

ouroboros/size_ratchet_manifest.py

@@ -225,6 +225,7 @@ BAND_PATHS = {
     "web/app.js": "Sidebar activity adds in-place marker and accessible-name updates to the existing navigation owner and connects the shared snapshot sequencer to transport failures. Pure census/status interpretation stays in project_activity.js; moving these DOM bindings would split navigation ownership.",
     "web/modules/chat_activity.js": "Existing task activity renderer consumes the shared quota/auth wait state; no parallel task card or lifecycle.",
     "web/modules/harness_accounts.js": None,
+    "web/modules/settings.js": "Settings retains draft, validation and save ownership; account discovery subscription and refresh state live together in the existing settings_catalog.js owner.",
     "web/modules/settings_ui.js": "The existing Settings markup owner includes the client-local Appearance panel alongside the other tab declarations; theme behavior remains in theme.js, without a new one-panel wrapper.",
     "web/modules/skills.js": "One installed-skill page controller owns independently settling primary/optional reads and current-generation menu, identity and badge updates; domain lifecycle, cards, hub truth and shared interactions remain separate owners.",
     "web/tests/chat_instance_dom.test.js": "Entered the band from 1000 lines with the alias-free subagent cost pin (stage-2 fix wave): that regression reproduces only through the real createChatInstance card path, and this file owns the DOM harness that drives it; split when the next createChatInstance face lands.",

tests/test_native_attention.py

@@ -8,7 +8,7 @@ def test_macos_attention_uses_system_sound(monkeypatch):
 
     calls = []
     monkeypatch.setattr(platform_layer, "IS_MACOS", True)
-    monkeypatch.setattr(platform_layer.pathlib.Path, "is_file", lambda self: str(self) == "/System/Library/Sounds/Glass.aiff")
+    monkeypatch.setattr(platform_layer.pathlib.Path, "is_file", lambda self: self.as_posix() == "/System/Library/Sounds/Glass.aiff")
     monkeypatch.setattr(platform_layer.subprocess, "run", lambda *args, **kwargs: (calls.append((args, kwargs)) or type("Result", (), {"returncode": 0})()))
     shown = []
     result = platform_layer.request_native_attention(lambda: shown.append(True))

tests/test_settings_account_refresh_browser.py

@@ -0,0 +1,152 @@
+"""Settings account discovery and legacy cadence through the actual browser UI."""
+from __future__ import annotations
+
+import copy
+import json
+import re
+
+import pytest
+
+from tests import test_subscription_setup_browser as setup_browser
+
+subscription_ui = setup_browser.subscription_ui
+capture = setup_browser.capture
+
+pytestmark = [pytest.mark.ui_browser, pytest.mark.serial]
+
+
+@pytest.mark.parametrize("transition", ["login", "reconnect"])
+@pytest.mark.parametrize("settle_first", ["settings", "catalog"])
+def test_confirmed_accounts_refresh_models_without_replacing_draft(subscription_ui, transition, settle_first):
+    from playwright.sync_api import expect
+
+    ui, page = subscription_ui, subscription_ui["page"]
+    connected = copy.deepcopy(ui["fixture"]["status"])
+    catalog = copy.deepcopy(ui["fixture"]["catalog"])
+    ui["settings"].update(OUROBOROS_MODEL="openai::saved-model", OPENAI_API_KEY="configured...")
+    ui["fixture"]["catalog"] = {"items": [], "model_sources": [], "errors": []}
+    if transition == "login":
+        ui["fixture"]["status"]["profiles"]["profiles"] = []
+    held_settings, held_catalog = [], []
+    hold_catalog = True
+
+    def hold_settings_read(route):
+        if len(held_settings) == 2:
+            route.fallback()
+            return
+        ui["reads"].append(route.request.url)
+        held_settings.append(route)
+        page.evaluate("count => window.heldSettingsReads = count", len(held_settings))
+
+    def hold_catalog_read(route):
+        if not hold_catalog:
+            route.fallback()
+            return
+        ui["reads"].append(route.request.url)
+        held_catalog.append(route)
+        page.evaluate("window.heldCatalogRead = true")
+
+    page.route("**/api/settings", hold_settings_read)
+    page.route("**/api/model-catalog", hold_catalog_read)
+    page.goto(ui["url"] + "/#settings")
+    page.wait_for_function("window.heldSettingsReads === 2")
+    # Page-show supersedes the initial GET before either document is applied.
+    held_settings[0].fulfill(json=ui["settings"])
+    page.wait_for_function("window.heldCatalogRead === true")
+    if settle_first == "settings":
+        held_settings[1].fulfill(json=ui["settings"])
+        expect(page.locator("#btn-save-settings")).to_be_enabled()
+    hold_catalog = False
+    held_catalog[0].fulfill(json=ui["fixture"]["catalog"])
+    expect(page.locator("#settings-model-catalog-status")).to_contain_text("No provider catalogs")
+    if settle_first == "catalog":
+        expect(page.locator("#btn-save-settings")).to_be_disabled()
+        held_settings[1].fulfill(json=ui["settings"])
+    expect(page.locator("#settings-status")).to_have_text("Settings refreshed")
+    page.locator('[data-settings-tab="models"]').click()
+    row = page.locator('[data-model-role="main"]')
+    source, model = row.locator('[data-model-role-source]'), row.locator('[data-model-role-model]')
+    expect(source.locator('option[value="subscription:codex"]')).to_have_count(0)
+    model.fill("owner-unsaved-model")
+    model.evaluate("e => { window.ownerDraftNode = e; e.setSelectionRange(4, 9); }")
+    settings_reads = sum(url.endswith("/api/settings") for url in ui["reads"])
+
+    def refresh_accounts():
+        # The same store refresh used when the Accounts login card settles.
+        page.evaluate("""async () => {
+            const {claudexorStatus} = await import('/static/modules/claudexor_status_store.js');
+            await claudexorStatus.refresh();
+        }""")
+
+    if transition == "reconnect":
+        ui["fixture"]["status"]["reads"]["accounts"] = "failed"
+        refresh_accounts()
+    ui["fixture"]["status"] = connected
+    ui["fixture"]["catalog"] = catalog
+    with page.expect_response(lambda response: response.url.endswith("/api/model-catalog"), timeout=5000):
+        refresh_accounts()
+    expect(source.locator('option[value="subscription:codex"]')).to_have_text("Codex (model)")
+    expect(model).to_have_value("owner-unsaved-model")
+    assert model.evaluate("e => e === window.ownerDraftNode && document.activeElement === e")
+    assert model.evaluate("e => [e.selectionStart, e.selectionEnd]") == [4, 9]
+    assert sum(url.endswith("/api/settings") for url in ui["reads"]) == settings_reads
+    reads = sum(url.endswith("/api/model-catalog") for url in ui["reads"])
+    refresh_accounts()
+    assert sum(url.endswith("/api/model-catalog") for url in ui["reads"]) == reads
+    assert not ui["posts"], "discovery cannot save or replace the owner's draft"
+    capture(page, f"settings-account-{transition}-{settle_first}-first")
+
+
+def test_legacy_30_second_cadence_loads_as_valid_settings_form(subscription_ui, tmp_path, monkeypatch):
+    from playwright.sync_api import expect
+    from starlette.applications import Starlette
+    from starlette.routing import Route
+    from starlette.testclient import TestClient
+    from ouroboros import config
+    from ouroboros.gateway import settings
+
+    ui, page = subscription_ui, subscription_ui["page"]
+    legacy = {**ui["settings"], "OUROBOROS_BG_WAKEUP_MIN": 30, "OUROBOROS_BG_WAKEUP_MAX": 7200}
+    legacy["OUROBOROS_SUBAGENTS"] = json.dumps(legacy["OUROBOROS_SUBAGENTS"])
+    legacy.pop("_meta")
+    legacy["OUROBOROS_CONTEXT_MODE_AUTO_LOW"] = "false"
+    path = tmp_path / "settings.json"
+    path.write_text(json.dumps(legacy), encoding="utf-8")
+    before = path.read_bytes()
+    monkeypatch.setattr(config, "DATA_DIR", tmp_path)
+    monkeypatch.setattr(config, "SETTINGS_PATH", path)
+    monkeypatch.setattr(settings, "apply_runtime_provider_defaults", lambda value: (value, False, []))
+    app = Starlette(routes=[Route("/api/settings", settings.api_settings_get)])
+    app.state.drive_root = app.state.repo_dir = tmp_path
+    with TestClient(app) as client:
+        response = client.get("/api/settings")
+    assert response.status_code == 200
+    ui["settings"].clear()
+    ui["settings"].update(response.json())
+    assert path.read_bytes() == before, "GET must not rewrite the legacy file"
+    def save_response(route):
+        if route.request.method != "POST":
+            route.fallback()
+            return
+        payload = route.request.post_data_json
+        ui["posts"].append(("/api/settings", payload))
+        ui["settings"].update(payload)
+        route.fulfill(json={"status": "saved", "saved": True})
+
+    page.route("**/api/settings", save_response)
+    page.goto(ui["url"] + "/#settings")
+    expect(page.locator("#settings-status")).to_have_text(re.compile("Settings (loaded|refreshed)"))
+    expect(page.locator("#s-bg-wakeup-min")).to_have_value("60")
+    expect(page.locator("#s-bg-wakeup-max")).to_have_value("7200")
+    assert page.locator("#s-bg-wakeup-min").evaluate("e => e.checkValidity()")
+    page.locator('[data-settings-tab="advanced"]').click()
+    page.locator("#s-gh-repo").fill("owner/unrelated-edit")
+    with page.expect_request(lambda request: request.url.endswith("/api/settings") and request.method == "POST", timeout=5000):
+        page.locator("#btn-save-settings").click()
+    saved = [body for url, body in ui["posts"] if url == "/api/settings"][-1]
+    assert saved["OUROBOROS_BG_WAKEUP_MIN"] == 60
+    assert saved["GITHUB_REPO"] == "owner/unrelated-edit"
+    expect(page.locator("#settings-status")).to_have_text("Settings saved")
+    page.locator('[data-settings-tab="behavior"]').click()
+    page.locator("#s-bg-wakeup-min").scroll_into_view_if_needed()
+    capture(page, "settings-legacy-cadence")

web/modules/settings.js

@@ -1,4 +1,5 @@
-import { refreshModelCatalog } from './settings_catalog.js';
+import { refreshModelCatalog, watchAccountModelCatalog } from './settings_catalog.js';
+export { accountCatalogRefreshKey } from './settings_catalog.js';
 import { getNotifier } from './notifications.js';
 import { bindEffortSegments, syncEffortSegments, readCustomSecretDraft, collectCustomSecretDraft, paintSettingsFieldErrors, settingsWriteFailure } from './settings_controls.js';
 import { bindLocalModelControls } from './settings_local_model.js';
@@ -22,7 +23,7 @@ import { PROVIDER_TEST_INPUTS, SECRET_KEYS, bindSecretInputs, bindSettingsTabs,
 import { showToast } from './toast.js';
 import { escapeHtmlAttr as escapeHtml, formatDualVersion } from './utils.js';
 import { apiClient, apiFetch, cleanExtensionRoute, extensionRoutePath } from './api_client.js';
-import { accountRows, claudexorStatus, READ_OK } from './claudexor_status_store.js';
+import { claudexorStatus } from './claudexor_status_store.js';
 import { createModelRolesEditor, modelRoleMap } from './model_roles.js';
 import { PROCESSING_PREFERENCE_KEY, MODEL_PROCESSING_PREFERENCES_KEY } from './route_editor_primitives.js';
 import { collectSafeFieldValues, normalizeTone, renderSafeField, setInlineStatus, revealNewRow } from './ui_helpers.js';
@@ -102,26 +103,6 @@ function isTruthySetting(value) {
     return value === true || ['true', '1', 'yes', 'on'].includes(normalized);
 }
 
-// Account login/status is the authority for subscription model discovery. Keep
-// one small signature of the confirmed account facts so a newly settled login
-// (or a changed account) can refresh the existing catalog without polling or
-// replacing the owner's in-memory model draft.
-export function accountCatalogRefreshKey(view) {
-    if (view?.reads?.accounts !== READ_OK) return null;
-    return JSON.stringify(accountRows(view.snapshot || {}).map((row) => ({
-        harness: row.harness,
-        profile_id: row.profile_id,
-        display_name: row.display_name,
-        enabled: row.enabled,
-        identity: {
-            email: row.identity?.email || '',
-            plan: row.identity?.plan || '',
-        },
-        verification: row.status?.verification || '',
-        availability: row.status?.availability || '',
-    })));
-}
-
 // A loading, validation or editor owner may update its own status. A later
 // message from anyone else drops that ownership, protecting the newer result.
 function setStatus(text, tone = 'ok', owner = '', subject = '') {
@@ -503,9 +484,6 @@ export function initSettings({ state, setBeforePageLeave, ws } = {}) {
     let settingsSaving = false;
     let saveOutcomeUnknown = false;
     let validationAttempted = false;
-    let modelCatalogStatusReady = false;
-    let lastAccountCatalogKey = null;
-    let disposeModelCatalogStatus = null;
     const providerTestGenerations = new Map();
     const providerTestsInFlight = new Set();
     const modelRoles = createModelRolesEditor({ hostId: 'settings-model-roles',
@@ -871,13 +849,10 @@ export function initSettings({ state, setBeforePageLeave, ws } = {}) {
                     'warn'
                 );
             }
-            if (applied) {
-                // The initial catalog read is already the page-load refresh. Arm
-                // account-change detection only after it so the same settled
-                // status snapshot cannot issue a duplicate request.
-                lastAccountCatalogKey = accountCatalogRefreshKey(claudexorStatus);
-                modelCatalogStatusReady = true;
-            }
+            // Catalog readiness is independent of Settings GET completion:
+            // a superseding page-show load may still be waiting for its document.
+            // Arm after discovery so its settled Accounts snapshot stays quiet.
+            accountModelCatalog.arm();
         } catch (error) {
             if (reloadSequence !== loadSequence) return;
             settingsLoaded = false;
@@ -907,25 +882,6 @@ export function initSettings({ state, setBeforePageLeave, ws } = {}) {
         }
     }
 
-    function noteAccountCatalogStatus(view) {
-        const next = accountCatalogRefreshKey(view);
-        if (!modelCatalogStatusReady) {
-            lastAccountCatalogKey = next;
-            return;
-        }
-        // A failed/unread Accounts facet clears the confirmation. The next
-        // successful answer therefore refreshes even when the account rows look
-        // unchanged, which is the reconnect path; a settled identical poll is
-        // quiet and cannot become a timer-driven catalog refresh.
-        if (next === null) {
-            lastAccountCatalogKey = null;
-            return;
-        }
-        if (next === lastAccountCatalogKey) return;
-        lastAccountCatalogKey = next;
-        void refreshModelCatalog();
-    }
-
     function collectBody() {
         const fieldValue = (id) => byId(id)?.value || '';
         const mutativeInput = byId('s-allow-mutative-subagents');
@@ -1207,7 +1163,7 @@ export function initSettings({ state, setBeforePageLeave, ws } = {}) {
     // A confirmed Accounts facet is the existing status-store seam for login
     // completion. It refreshes the catalog only on a changed/rehydrated account
     // answer, while modelRoles.adoptCatalog keeps unsaved assignments intact.
-    disposeModelCatalogStatus = claudexorStatus.subscribe(noteAccountCatalogStatus);
+    const accountModelCatalog = watchAccountModelCatalog();
     const disposeRestartReconnect = ws?.on?.('open', () => {
         refreshRestartState();
         if (settingsLoaded) void refreshModelCatalog();
@@ -1232,8 +1188,7 @@ export function initSettings({ state, setBeforePageLeave, ws } = {}) {
         window.removeEventListener('beforeunload', beforeUnload);
         disposeLocalModel();
         disposeRestartReconnect?.();
-        disposeModelCatalogStatus?.();
-        disposeModelCatalogStatus = null;
+        accountModelCatalog.dispose();
         restartReadSequence += 1;
         baselineSettleDisposer?.();
         modelRoles.destroy();

web/modules/settings_catalog.js

@@ -1,8 +1,54 @@
+import { accountRows, claudexorStatus, READ_OK } from './claudexor_status_store.js';
 import { apiFetch } from './api_client.js';
 import { setInlineStatus } from './ui_helpers.js';
 export const MODEL_CATALOG_TIMEOUT_MS = 25000;
 let catalogRefreshSeq = 0;
 
+// Account login/status is the authority for subscription model discovery. Keep
+// one small signature of the confirmed account facts so a newly settled login
+// (or a changed account) can refresh the existing catalog without polling or
+// replacing the owner's in-memory model draft.
+export function accountCatalogRefreshKey(view) {
+    if (view?.reads?.accounts !== READ_OK) return null;
+    return JSON.stringify(accountRows(view.snapshot || {}).map((row) => ({
+        harness: row.harness,
+        profile_id: row.profile_id,
+        display_name: row.display_name,
+        enabled: row.enabled,
+        identity: {
+            email: row.identity?.email || '',
+            plan: row.identity?.plan || '',
+        },
+        verification: row.status?.verification || '',
+        availability: row.status?.availability || '',
+    })));
+}
+
+/** Own the Accounts subscription beside catalog refresh; never reload Settings. */
+export function watchAccountModelCatalog() {
+    let ready = false;
+    let lastKey = null;
+    const dispose = claudexorStatus.subscribe((view) => {
+        const next = accountCatalogRefreshKey(view);
+        if (!ready || next === null) {
+            // An unread/failed facet resets confirmation, so reconnect refreshes
+            // even unchanged accounts; an identical settled poll stays quiet.
+            lastKey = next;
+            return;
+        }
+        if (next === lastKey) return;
+        lastKey = next;
+        void refreshModelCatalog();
+    });
+    return {
+        arm() {
+            lastKey = accountCatalogRefreshKey(claudexorStatus);
+            ready = true;
+        },
+        dispose,
+    };
+}
+
 /**
  * Read provenance belongs to discovery, never to the owner's saved assignment.
  * One unreachable source among several is `partial`: the catalogs that did