diff --git a/tests/test_app_js.py b/tests/test_app_js.py index c1bb1ac8..df0bcab4 100644 --- a/tests/test_app_js.py +++ b/tests/test_app_js.py @@ -2481,7 +2481,9 @@ def test_paint_project_picker_syncs_before_refresh() -> None: the two can't drift and silently re-introduce the FOUC) seeds the required/ optional hint + paints the picker via _populateProjectSelect SYNCHRONOUSLY, then refreshes-and-repaints. It skips a fork, and both paints route through - the same _populateProjectSelect (preserving the #867 strict-picker invariant).""" + the same _populateProjectSelect (preserving the #867 strict-picker invariant). + The sync paint passes freshOnOpen; the async repaint MUST pass fresh=false, or + it clobbers a project the user picked mid-refresh (require_project -> 400).""" body = _APP_JS.read_text(encoding="utf-8") fn = _slice_top_level_fn(body, "function _paintProjectPicker(") assert "_populateProjectSelect(" in fn, ( @@ -2489,12 +2491,15 @@ def test_paint_project_picker_syncs_before_refresh() -> None: ) assert "hint.textContent" in fn, "_paintProjectPicker must seed the required/optional hint" assert "opts.fork" in fn, "_paintProjectPicker must skip for a fork" - sync_call = fn.find("paint();") + sync_call = fn.find("paint(freshOnOpen)") async_refresh = fn.find("refreshProjects().then") assert async_refresh >= 0, "_paintProjectPicker must keep refreshProjects().then" assert 0 <= sync_call < async_refresh, ( - "_paintProjectPicker must paint synchronously (paint()) BEFORE the async " - "refreshProjects().then repaint" + "_paintProjectPicker must paint synchronously (paint(freshOnOpen)) BEFORE the " + "async refreshProjects().then repaint" + ) + assert "paint(false)" in fn[async_refresh:], ( + "_paintProjectPicker's async repaint must pass fresh=false (preserve a mid-window pick)" ) @@ -2516,57 +2521,103 @@ _PROJECTS_JS = Path(__file__).resolve().parent.parent / "turnstone/shared_static _PERSONAS_JS = Path(__file__).resolve().parent.parent / "turnstone/shared_static/personas.js" -def test_new_ws_modal_paints_model_and_skill_from_cache_synchronously() -> None: - """The modal's model + skill pickers paint from the warm cache SYNCHRONOUSLY - on open, BEFORE the async refresh-and-repaint (no empty-dropdown flash).""" +def test_paint_model_and_skill_wrappers_sync_before_refresh() -> None: + """The shared _paintModelSelects / _paintSkillSelect wrappers (used by BOTH the + modal and dashboard — collapsing the 4x paint-then-refresh dup) paint from the + warm cache SYNCHRONOUSLY, then refresh-and-repaint. CRITICAL: the sync paint + passes fresh=freshOnOpen but the ASYNC repaint MUST pass fresh:false — a wrapper + leaking freshOnOpen into the async paint would clobber a mid-window pick (for the + project picker that reconciles to "" -> a require_project 400).""" + body = _APP_JS.read_text(encoding="utf-8") + for wrapper, populate, refresh in ( + ("function _paintModelSelects(", "_populateModelSelect(", "refreshModels().then"), + ("function _paintSkillSelect(", "_populateSkillSelect(", "refreshSkills().then"), + ): + fn = _slice_top_level_fn(body, wrapper) + sync = fn.find(populate) + async_ = fn.find(refresh) + assert 0 <= sync < async_, f"{wrapper} must paint synchronously before the async refresh" + assert "{ fresh: freshOnOpen }" in fn, f"{wrapper} sync paint must pass fresh=freshOnOpen" + assert "{ fresh: false }" in fn, ( + f"{wrapper} async repaint must pass fresh:false (never leak freshOnOpen, " + "or it clobbers a mid-window pick)" + ) + + +def test_new_ws_modal_renders_all_selects_fresh_on_open() -> None: + """Finding [0] fix — every composer select in the reused new-ws renders + its ACTUAL value fresh on open (no stale carryover from the last open): model + + skill via the wrappers with freshOnOpen:true, persona via {fresh:true}, project + via _paintProjectPicker freshOnOpen:true. The model paint is fork-gated.""" body = _APP_JS.read_text(encoding="utf-8") fn = _slice_top_level_fn(body, "function showNewWsModal(") - sync_model = fn.find("_populateModelSelect(modelSelect") - async_model = fn.find("refreshModels().then") - assert sync_model >= 0, "the modal must paint the model picker from cache synchronously" - assert async_model >= 0, "the modal must keep refreshModels().then" - assert sync_model < async_model, ( - "the synchronous model paint must precede the async refreshModels().then repaint" + assert "_paintModelSelects(modelSelect, judgeSelect, { freshOnOpen: true })" in fn, ( + "the modal must paint model+judge fresh-on-open via the shared wrapper" ) - sync_skill = fn.find("_populateSkillSelect(tplSelect)") - async_skill = fn.find("refreshSkills().then") - assert sync_skill >= 0, "the modal must paint the skill picker from cache synchronously" - assert async_skill >= 0, "the modal must keep refreshSkills().then" - assert sync_skill < async_skill, ( - "the synchronous skill paint must precede the async refreshSkills().then repaint" + assert "_paintSkillSelect(tplSelect, { freshOnOpen: true })" in fn, ( + "the modal must paint skill fresh-on-open" ) + assert "_populatePersonaSelect(personaSelect, { fresh: true })" in fn, ( + "the modal must paint persona fresh-on-open (kind default, no stale carryover)" + ) + proj = fn[fn.find("_paintProjectPicker(projSelect") :] + assert "freshOnOpen: true" in proj[:120], ( + "the modal must paint the project picker fresh-on-open" + ) + # the model wrapper call is fork-gated (a fork inherits + hides model/judge) + guard = fn.find("if (!_forkFromWsId) {") + model_paint = fn.find("_paintModelSelects(modelSelect") + assert 0 <= guard < model_paint, "the modal model paint must be skipped for a fork" -def test_dashboard_paints_model_and_skill_from_cache_synchronously() -> None: - """Dashboard twin — model + skill paint synchronously before the refresh, and - the old fetch-once ``options.length <= 1`` guard is GONE (upgraded to - refresh-on-open via the coalesced cache, matching project/persona).""" +def test_dashboard_paints_model_and_skill_via_wrappers_preserving() -> None: + """Dashboard twin — model + skill paint via the SAME wrappers but freshOnOpen:false + (a persistent panel preserves a pick across a repaint), and the old fetch-once + ``options.length <= 1`` guard is gone.""" body = _APP_JS.read_text(encoding="utf-8") fn = _slice_top_level_fn(body, "function _loadDashboardOptionsLists(") - sync_model = fn.find("_populateModelSelect(modelSel") - async_model = fn.find("refreshModels().then") - assert sync_model >= 0, "the dashboard must paint the model picker from cache synchronously" - assert 0 <= sync_model < async_model, ( - "the synchronous dashboard model paint must precede the async refresh repaint" + assert "_paintModelSelects(modelSel, judgeSel, { freshOnOpen: false })" in fn, ( + "the dashboard must paint model+judge via the wrapper, preserving (freshOnOpen:false)" ) - sync_skill = fn.find("_populateSkillSelect(skillSel)") - async_skill = fn.find("refreshSkills().then") - assert sync_skill >= 0, "the dashboard must paint the skill picker from cache synchronously" - assert 0 <= sync_skill < async_skill, ( - "the synchronous dashboard skill paint must precede the async refresh repaint" + assert "_paintSkillSelect(skillSel, { freshOnOpen: false })" in fn, ( + "the dashboard must paint skill via the wrapper, preserving" ) assert "options.length <= 1" not in fn, ( "the dashboard model/skill fetch-once guard must be removed (refresh-on-open now)" ) +def test_new_ws_modal_fork_inherits_model_and_judge() -> None: + """Q2 — a fork INHERITS its source's model + judge: the modal hides both selects + for a fork (like skill/persona/project), and submitNewWs gates body.model AND + body.judge_model on !_forkFromWsId. Asserts the model line SPECIFICALLY — its + guard `model && !_forkFromWsId` is a substring of the judge line, so a + model-unguarded regression would otherwise false-pass.""" + body = _APP_JS.read_text(encoding="utf-8") + modal = _slice_top_level_fn(body, "function showNewWsModal(") + assert "modelSelect.hidden = !!_forkFromWsId" in modal, ( + "modal must hide the model select for a fork" + ) + assert "judgeSelect.hidden = !!_forkFromWsId" in modal, ( + "modal must hide the judge select for a fork" + ) + submit = _slice_top_level_fn(body, "function submitNewWs(") + assert "if (model && !_forkFromWsId) body.model = model;" in submit, ( + "submitNewWs must fork-gate body.model (distinct from the judge line)" + ) + assert "if (judge_model && !_forkFromWsId) body.judge_model = judge_model;" in submit, ( + "submitNewWs must fork-gate body.judge_model" + ) + + def test_console_launcher_paints_model_and_skill_from_cache_synchronously() -> None: """Console launcher — the model + skill refresh wrappers paint synchronously - from the warm cache before the async refresh-and-repaint.""" + from the warm cache before the async refresh-and-repaint. The model wrapper + threads callOpts so models_changed can force a trailing refresh.""" body = _CONSOLE_APP_JS.read_text(encoding="utf-8") model_fn = _slice_top_level_fn(body, "function _refreshAndPopulateModels(") sync_model = model_fn.find("_populateHomeModelDropdowns()") - async_model = model_fn.find("refreshModels().then") + async_model = model_fn.find("refreshModels(callOpts).then") assert sync_model >= 0, "the launcher must paint the model pickers from cache synchronously" assert 0 <= sync_model < async_model, ( "the synchronous launcher model paint must precede the async refresh repaint" @@ -2574,7 +2625,6 @@ def test_console_launcher_paints_model_and_skill_from_cache_synchronously() -> N skill_fn = _slice_top_level_fn(body, "function _refreshAndPopulateSkills(") sync_skill = skill_fn.find("_populateHomeSkillDropdown()") async_skill = skill_fn.find("refreshSkills().then") - assert sync_skill >= 0, "the launcher must paint the skill picker from cache synchronously" assert 0 <= sync_skill < async_skill, ( "the synchronous launcher skill paint must precede the async refresh repaint" ) @@ -2591,18 +2641,38 @@ def test_console_relogin_rewarms_model_and_skill() -> None: assert "_refreshAndPopulateModels()" in login, "onLoginSuccess must re-warm models" -def test_models_changed_single_repaint_path() -> None: - """The console ``models_changed`` SSE handler repaints the launcher via the - refresh wrapper (which reads the warm cache), and the launcher does NOT ALSO - subscribe onModelsChange — one repaint path, no double rebuild (graph R7).""" +def test_models_changed_forces_trailing_single_repaint_path() -> None: + """The console ``models_changed`` handler repaints via the refresh wrapper with + {force:true} — so a burst of model-config changes converges to the latest server + state (trailing refresh) — and the launcher does NOT ALSO subscribe onModelsChange + (one repaint path, no double rebuild).""" body = _CONSOLE_APP_JS.read_text(encoding="utf-8") mc = body.index('data.type === "models_changed"') - handler = body[mc : mc + 600] - assert "_refreshAndPopulateModels()" in handler, ( - "models_changed must repaint the launcher model dropdowns via the refresh wrapper" + handler = body[mc : mc + 700] + assert "_refreshAndPopulateModels({ force: true })" in handler, ( + "models_changed must force a trailing refresh so it converges to the latest" ) assert "onModelsChange" not in body, ( - "the console must not ALSO subscribe onModelsChange (single repaint path — R7)" + "the console must not ALSO subscribe onModelsChange (single repaint path)" + ) + + +def test_models_label_centralized_on_bridge() -> None: + """Finding [7] — the "alias (model)" label lives ONCE in models.js: modelLabel is + exported AND registered on the window.TurnstoneModels bridge (the classic app.js + bundles reach it only via the bridge — an ES-only export throws at runtime), and + neither app keeps a local _resolveModelLabel copy.""" + models_src = _MODELS_JS.read_text(encoding="utf-8") + assert "export function modelLabel(" in models_src, "models.js must export modelLabel" + assert "modelLabel: modelLabel" in models_src, ( + "models.js must register modelLabel on the window.TurnstoneModels bridge " + "(classic bundles call it via the bridge)" + ) + assert "_resolveModelLabel" not in _APP_JS.read_text(encoding="utf-8"), ( + "the ui app must not keep a local _resolveModelLabel (use TurnstoneModels.modelLabel)" + ) + assert "_resolveModelLabel" not in _CONSOLE_APP_JS.read_text(encoding="utf-8"), ( + "the console app must not keep a local _resolveModelLabel" ) @@ -2616,24 +2686,47 @@ def test_models_skills_module_tagged_in_both_apps() -> None: assert "/shared/skills.js" in html, f"skills.js must be module-tagged in {idx.name}" -def test_list_cache_core_is_failopen_and_coalesced() -> None: - """The extracted list_cache.js core coalesces concurrent refreshes, fails open - (keeps the prior cache + records the error on a non-OK/exception, never - rejects — only the SUCCESS branch calls _setCache), and fires subscribers only - on first load or a changed fingerprint.""" +def test_list_cache_core_is_failopen_coalesced_and_gated() -> None: + """The extracted list_cache.js core: non-force callers coalesce onto the in-flight + refresh; it fails open (keeps the prior cache + records the error, never rejects — + only the SUCCESS branch calls _setCache); the extra-reset is GATED on + resetExtraOnError across BOTH error branches (finding [1]); and a force caller + schedules a trailing refetch that converges to the latest (finding [2]).""" src = _LIST_CACHE_JS.read_text(encoding="utf-8") - assert "if (_inflight) return _inflight" in src, "refresh must coalesce concurrent callers" + assert "return _inflight;" in src, "non-force callers must coalesce onto the in-flight refresh" assert "_lastError = r.status" in src and "_lastError = 0" in src, ( "a non-OK status and a network/parse error must both be recorded (fail-open)" ) assert src.count("_setCache(data[dataKey]") == 1, ( "only the success branch may repopulate the cache (non-OK/catch keep the prior cache)" ) + # finding [1]: the extra-reset must be gated on resetExtraOnError on BOTH the + # non-OK branch AND the network/parse .catch branch (a cosmetic extra must + # survive a network drop too, not just a non-OK status). + assert src.count("resetExtraOnError && extraDefaults") == 2, ( + "resetExtraOnError must gate the extra-reset on BOTH error branches" + ) + # finding [2]: a force caller gets a trailing refetch (converges to latest). + assert "callOpts.force" in src and "_pending" in src, ( + "a force caller must schedule a trailing refetch so it converges to the latest state" + ) assert "firstLoad || fp !== _fingerprint" in src, ( "subscribers must fire on first load or a changed fingerprint only" ) +def test_reset_extra_policy_per_cache() -> None: + """require_project (an advisory that GATES the picker) must fail open on error; + the models default-aliases (a COSMETIC annotation) must keep their last-known + value. So projects opts INTO the reset, models opts OUT (finding [1]).""" + assert "resetExtraOnError: true" in _PROJECTS_JS.read_text(encoding="utf-8"), ( + "projects.js must reset the require_project advisory on error (fail-open)" + ) + assert "resetExtraOnError: false" in _MODELS_JS.read_text(encoding="utf-8"), ( + "models.js must keep last-known default aliases on error (cosmetic, not a gate)" + ) + + def test_models_cache_exposes_both_server_schemas() -> None: """models.js must carry ALL default-alias fields — the node server sends default_alias, the console sends coordinator_default_alias, both send diff --git a/turnstone/console/static/app.js b/turnstone/console/static/app.js index d9016cfc..e42c6642 100644 --- a/turnstone/console/static/app.js +++ b/turnstone/console/static/app.js @@ -429,9 +429,11 @@ function handleClusterEvent(data) { // Server emits this when a model definition or a role-assignment // setting (model.default_alias, judge.model, coordinator.model_alias, // coordinator.reasoning_effort) changes. Refresh anything that - // renders model aliases so labels stay accurate without a reload. + // renders model aliases so labels stay accurate without a reload. Pass + // {force:true} so a burst of changes converges to the latest (the coalesced + // open/startup path stays plain — see _refreshAndPopulateModels). if (typeof _refreshAndPopulateModels === "function") { - _refreshAndPopulateModels(); + _refreshAndPopulateModels({ force: true }); } if (typeof _sklcInvalidateModelsCache === "function") { _sklcInvalidateModelsCache(); @@ -1764,27 +1766,18 @@ function _populateHomeSkillDropdown() { // dropdown rows ("alias (model)", or just "alias" when they coincide). // Returns "" when alias is empty or unknown so callers can fall back // to a neutral placeholder. -function _resolveModelLabel(alias, models) { - if (!alias) return ""; - for (let i = 0; i < (models || []).length; i++) { - const m = models[i]; - if (m.alias === alias) { - return m.alias === m.model ? m.alias : m.alias + " (" + m.model + ")"; - } - } - return ""; -} - // Refresh the shared models cache then repaint the launcher's Model + Judge // Model pickers. Sync paint from the warm cache first (no empty flash for the // round-trip), then refresh-and-repaint. This is ALSO the single repaint path -// for the `models_changed` SSE event: it refreshes the cache and repaints, so -// there is no second (subscription) path that would double-repaint. -function _refreshAndPopulateModels() { +// for the `models_changed` SSE event, which passes {force:true} so the refresh +// converges to the latest server state (and its .then repaints from THAT) even +// when a burst of model-config changes lands mid-fetch — no second +// (subscription) path that would double-repaint. +function _refreshAndPopulateModels(callOpts) { const TM = window.TurnstoneModels; if (!TM) return; _populateHomeModelDropdowns(); - TM.refreshModels().then(_populateHomeModelDropdowns); + TM.refreshModels(callOpts).then(_populateHomeModelDropdowns); } // Populate the launcher's Model + Judge Model pickers from the shared cache — @@ -1799,8 +1792,11 @@ function _populateHomeModelDropdowns() { const TM = window.TurnstoneModels; if (!TM) return; const choices = TM.modelChoices(); - const models = TM.getModels(); const defaults = TM.modelDefaults(); + // The launcher composer is a PERSISTENT panel (it never reopens like the new-ws + // modal), so preserving the prior pick across a repaint is INTENDED — not the + // modal's fresh-on-open reset. A background models_changed must not clobber a + // mid-window model/judge choice. const prevModel = _homeCoordComposer.getOptionValue("model"); const prevJudge = _homeCoordComposer.getOptionValue("judge_model"); _homeCoordComposer.setOptionChoices("model", choices); @@ -1811,14 +1807,8 @@ function _populateHomeModelDropdowns() { // "Default model" line above it. Em-dash separator (rather than nested // parens) keeps the alias's "(model)" suffix legible and matches the // ``(default — alias (model))`` pattern used in the admin Roles tab. - const coordDefault = _resolveModelLabel( - defaults.coordinator_default_alias || "", - models, - ); - const judgeDefault = _resolveModelLabel( - defaults.judge_default_alias || "", - models, - ); + const coordDefault = TM.modelLabel(defaults.coordinator_default_alias || ""); + const judgeDefault = TM.modelLabel(defaults.judge_default_alias || ""); _homeCoordComposer.setOptionPlaceholder( "model", coordDefault ? "Default — " + coordDefault : "Default model", diff --git a/turnstone/shared_static/list_cache.js b/turnstone/shared_static/list_cache.js index 91d3c791..9a95f70a 100644 --- a/turnstone/shared_static/list_cache.js +++ b/turnstone/shared_static/list_cache.js @@ -41,9 +41,20 @@ import { authFetch } from "./auth.js"; * successful refresh (returns the new * `extra` object, replacing the prior one) * @param {object} [opts.extraDefaults] the fail-open `extra` value — installed - * at init AND restored on every refresh - * failure, so a stale-true advisory can't - * survive a transient error + * at init AND (when resetExtraOnError) + * restored on a refresh failure, so a + * stale-true advisory can't survive a + * transient error + * @param {boolean} [opts.resetExtraOnError=true] whether a failed refresh resets + * `extra` to extraDefaults. True for an + * ADVISORY that gates UI (must fail open); + * false for a merely COSMETIC extra that + * should keep its last-known value through a + * transient failure rather than blank. + * `refresh(opts)` accepts `{force:true}` — an INVALIDATION (e.g. a *_changed SSE + * event) that must converge to the latest server state: if a refresh is already + * in flight it chains ONE trailing refetch after it and awaits that, instead of + * coalescing onto the in-flight response that may predate the change. * @returns {{refresh:Function, get:Function, getByKey:Function, * loaded:Function, error:Function, extra:Function, onChange:Function}} */ @@ -56,12 +67,17 @@ export function makeListCache(opts) { const fpExtra = opts.fpExtra || null; const captureExtra = opts.captureExtra || null; const extraDefaults = opts.extraDefaults || null; + // Default true: an advisory that gates UI (projects' require_project) must fail + // open, so a stale-true value can't survive an error. A cosmetic extra + // (models' resolved default aliases) passes false to keep the last-known value. + const resetExtraOnError = opts.resetExtraOnError !== false; let _cache = []; // last-fetched rows (caller-visible) let _byKey = {}; // keyField value -> row, for O(1) lookup (when keyField set) let _loaded = false; // has the first refresh attempt completed (ok or failed)? let _lastError = null; // last failure: HTTP status, 0 for network/parse, null when ok let _inflight = null; // shared pending refresh so concurrent callers coalesce + let _pending = null; // trailing refresh chained after _inflight for a force() call let _fingerprint = null; // last fired signature, for change-detection let _extra = extraDefaults ? Object.assign({}, extraDefaults) : {}; const _subs = []; // () => void, fired after each CHANGED refresh @@ -102,20 +118,43 @@ export function makeListCache(opts) { } } - function refresh() { + function refresh(callOpts) { // Coalesce concurrent callers (startup warm + a picker open can both fire // this) onto one in-flight request — they share the promise and _setCache // runs once. - if (_inflight) return _inflight; + if (_inflight) { + if (callOpts && callOpts.force) { + // A force caller is an INVALIDATION ("the data definitely changed" — a + // *_changed SSE event): it must converge to the latest server state, so + // chain ONE trailing refetch after the in-flight one and await THAT, + // rather than coalescing onto a response that may predate the change. + // Reused if several force calls land during the same fetch (a burst + // collapses to one trailing). INVARIANT: refresh() never rejects (the + // chain below always resolves via .catch), so this .then always runs; if + // a reject path is ever added, clear _pending in a .catch or force + // callers wedge on a stuck promise. + _pending = + _pending || + _inflight.then(function () { + _pending = null; + return refresh(); + }); + return _pending; + } + return _inflight; + } _inflight = authFetch(url) .then(function (r) { if (r.ok) return r.json(); // A non-OK status (403 when a grant is missing, a 5xx, ...) is NOT "you - // have zero rows": keep the prior cache rather than blanking it, record - // the status so the failure is visible, and reset the advisory extra to - // its fail-open default (a stale-true value must not survive an error). + // have zero rows": keep the prior cache rather than blanking it and + // record the status. Reset the extra to its fail-open default ONLY when + // resetExtraOnError (an advisory that gates UI must not survive stale); a + // cosmetic extra keeps its last-known value. _lastError = r.status; - if (extraDefaults) _extra = Object.assign({}, extraDefaults); + if (resetExtraOnError && extraDefaults) { + _extra = Object.assign({}, extraDefaults); + } console.warn(name + ": GET " + url + " -> " + r.status); return null; }) @@ -130,9 +169,11 @@ export function makeListCache(opts) { .catch(function (e) { // Network drop or a non-JSON body — same policy as a non-OK status: // preserve the last-known cache, never reject (callers chain a bare - // .then), surface the failure, fail-open the extra. + // .then), surface the failure, fail-open the extra (when configured). _lastError = 0; - if (extraDefaults) _extra = Object.assign({}, extraDefaults); + if (resetExtraOnError && extraDefaults) { + _extra = Object.assign({}, extraDefaults); + } console.warn(name + ": refresh failed", e); return _cache; }) diff --git a/turnstone/shared_static/models.js b/turnstone/shared_static/models.js index 33108724..49222be8 100644 --- a/turnstone/shared_static/models.js +++ b/turnstone/shared_static/models.js @@ -57,16 +57,23 @@ const _core = makeListCache({ judge_default_alias: "", coordinator_default_alias: "", }, + // The default aliases are a COSMETIC placeholder annotation ("Default — gpt-5"), + // not a UI-gating advisory, so keep the last-known value through a transient + // refresh failure instead of blanking it back to an un-annotated "Default + // model". A first-load failure still shows all-"" (the seed above). + resetExtraOnError: false, }); /** * Fetch /v1/api/models into the cache. Resolves to the row list and NEVER * rejects — a failed/forbidden fetch keeps the prior cache (a picker never - * blanks) and resets the default aliases to "". Recorded (see - * {@link modelsError}) and warned rather than masqueraded as "no models". + * blanks) AND keeps the last-known default aliases; a first-load failure still + * shows all-"". Recorded (see {@link modelsError}) rather than masqueraded as + * "no models". Pass `{force:true}` to force a fresh fetch that converges to the + * latest server state even mid-flight (the `models_changed` SSE path uses this). */ -export function refreshModels() { - return _core.refresh(); +export function refreshModels(callOpts) { + return _core.refresh(callOpts); } /** Cached model rows (empty until the first refresh resolves). Raw rows so @@ -87,24 +94,42 @@ export function modelsError() { return _core.error(); } -/** `{value, text}` choices for a model — the label is identical across + * all three creation surfaces, so it is centralized here. Callers seed their + * own static "Default …" placeholder as option 0. */ export function modelChoices() { return _core.get().map(function (m) { - return { - value: m.alias, - text: m.alias === m.model ? m.alias : m.alias + " (" + m.model + ")", - }; + return { value: m.alias, text: _fmtLabel(m) }; }); } +/** The "alias (model)" label for a model alias, or "" when the alias is empty or + * unknown — lets a composer annotate its "Default — " placeholder + * without re-implementing the format. The models cache has no key index, so + * this scans the (small) row list. */ +export function modelLabel(alias) { + if (!alias) return ""; + const rows = _core.get(); + for (let i = 0; i < rows.length; i++) { + if (rows[i].alias === alias) return _fmtLabel(rows[i]); + } + return ""; +} + /** The resolved default aliases the local server reported: * `{default_alias, judge_default_alias, coordinator_default_alias}` (each "" * when not sent). The ui dashboard reads default_alias, the console launcher - * reads coordinator_default_alias; both read judge_default_alias. Fails open - * to all-"" on a refresh error. */ + * reads coordinator_default_alias; both read judge_default_alias. Keeps the + * last-known values on a refresh error (a cosmetic annotation, not a UI gate); + * all-"" only before the first successful load. */ export function modelDefaults() { return _core.extra(); } @@ -123,6 +148,7 @@ window.TurnstoneModels = { modelsLoaded: modelsLoaded, modelsError: modelsError, modelChoices: modelChoices, + modelLabel: modelLabel, modelDefaults: modelDefaults, onModelsChange: onModelsChange, }; diff --git a/turnstone/shared_static/projects.js b/turnstone/shared_static/projects.js index ce0b32f2..0a5cdb79 100644 --- a/turnstone/shared_static/projects.js +++ b/turnstone/shared_static/projects.js @@ -39,6 +39,11 @@ const _core = makeListCache({ return { requireProject: !!data.require_project }; }, extraDefaults: { requireProject: false }, + // require_project GATES the picker (strict mode hides the projectless option), + // so it must fail OPEN: a failed refresh resets it to false rather than letting + // a stale-true value hide options. (Default, but explicit for the contrast + // with models.js, whose cosmetic extra opts out of the reset.) + resetExtraOnError: true, }); /** diff --git a/turnstone/ui/static/app.js b/turnstone/ui/static/app.js index 9f7516d5..5d574b63 100644 --- a/turnstone/ui/static/app.js +++ b/turnstone/ui/static/app.js @@ -336,27 +336,26 @@ function showNewWsModal(forkFromWsId) { if (skillLabel) skillLabel.hidden = !!_forkFromWsId; if (skillSelect) skillSelect.hidden = !!_forkFromWsId; - // Model + judge pickers — paint from the warm models cache first, then - // refresh-and-repaint. The modal keeps the plain static "Default …" - // placeholders (annotate:false); the dashboard annotates with the resolved - // default. On a cold cache the sync paint is a no-op the refresh fills. + // Model + judge pickers — HIDDEN for a fork: a fork inherits its source's + // model + judge (like skill/persona/project), and submitNewWs gates both on + // !_forkFromWsId. For a fresh create, paint from the warm cache with the + // resolved-default annotation ("Default — gpt-5"), FRESH on open (the reused + // dialog has no prior pick to carry over), then refresh-and-repaint. + const modelLabel = document.querySelector('label[for="new-ws-model"]'); + const judgeLabel = document.querySelector('label[for="new-ws-judge-model"]'); const modelSelect = document.getElementById("new-ws-model"); const judgeSelect = document.getElementById("new-ws-judge-model"); - _populateModelSelect(modelSelect, judgeSelect, { annotate: false }); - if (window.TurnstoneModels) { - window.TurnstoneModels.refreshModels().then(function () { - _populateModelSelect(modelSelect, judgeSelect, { annotate: false }); - }); + if (modelLabel) modelLabel.hidden = !!_forkFromWsId; + if (judgeLabel) judgeLabel.hidden = !!_forkFromWsId; + if (modelSelect) modelSelect.hidden = !!_forkFromWsId; + if (judgeSelect) judgeSelect.hidden = !!_forkFromWsId; + if (!_forkFromWsId) { + _paintModelSelects(modelSelect, judgeSelect, { freshOnOpen: true }); } - // Skill picker — same paint-from-cache-then-refresh. + // Skill picker — paint fresh-on-open from the warm cache, then refresh. const tplSelect = document.getElementById("new-ws-skill"); - _populateSkillSelect(tplSelect); - if (window.TurnstoneSkills) { - window.TurnstoneSkills.refreshSkills().then(function () { - _populateSkillSelect(tplSelect); - }); - } + _paintSkillSelect(tplSelect, { freshOnOpen: true }); // Project picker — populated from the shared projects cache, refreshed on // open. Fresh creates SHOW it; forks HIDE it — a fork's project is its @@ -378,7 +377,10 @@ function showNewWsModal(forkFromWsId) { // refresh-and-repaint — shared with the dashboard via _paintProjectPicker so // the two can't drift; skips for a fork (its picker is hidden above, // inheritance is server-enforced). - _paintProjectPicker(projSelect, projHint, { fork: !!_forkFromWsId }); + _paintProjectPicker(projSelect, projHint, { + fork: !!_forkFromWsId, + freshOnOpen: true, + }); // Persona picker — hidden when forking (a fork resumes the source's // stamped persona; the create handler skips resolution on resume_ws). @@ -387,13 +389,13 @@ function showNewWsModal(forkFromWsId) { if (personaLabel) personaLabel.hidden = !!_forkFromWsId; if (personaSelect) personaSelect.hidden = !!_forkFromWsId; if (personaSelect && !_forkFromWsId && window.TurnstonePersonas) { - // Sync paint from the warm cache first, then refresh-and-repaint to catch a - // persona created elsewhere. _populatePersonaSelect preserves a mid-window - // pick and only applies the kind default when nothing valid is selected, so - // the second (async) paint can't clobber the user's choice. - _populatePersonaSelect(personaSelect); + // Fresh-on-open sync paint (renders the kind default; the reused dialog has + // no prior pick to carry over), then refresh-and-repaint. The async paint + // passes fresh:false so it preserves a mid-window pick and only re-applies + // the kind default when nothing valid is selected. + _populatePersonaSelect(personaSelect, { fresh: true }); window.TurnstonePersonas.refreshPersonas().then(function () { - _populatePersonaSelect(personaSelect); + _populatePersonaSelect(personaSelect, { fresh: false }); }); } @@ -471,9 +473,12 @@ function _ensureStandaloneProjectCreator(sel) { // — this dialog only creates interactive workstreams), preselecting the kind // default so a zero-touch create behaves exactly like today. No-op when the // personas bridge is absent (module still loading / pre-seed database). -function _populatePersonaSelect(sel) { +function _populatePersonaSelect(sel, opts) { if (!sel || !window.TurnstonePersonas) return; - const previous = sel.value; + // A fresh modal open has no prior selection to preserve — render the kind + // default; a repaint (fresh=false) keeps a mid-window pick. + const fresh = !!(opts && opts.fresh); + const previous = fresh ? "" : sel.value; const placeholder = sel.options.length ? sel.options[0] : null; sel.replaceChildren(); if (placeholder) sel.appendChild(placeholder); @@ -518,37 +523,66 @@ function _optionExists(sel, val) { // #867 strict-picker invariant (never auto-select a real project) holds on each. function _paintProjectPicker(sel, hint, opts) { if ((opts && opts.fork) || !sel || !window.TurnstoneProjects) return; - const paint = function () { + // paint(fresh): a fresh open renders the actual default (no prior selection); + // the async repaint MUST pass fresh=false, or it clobbers a project the user + // picked during the refresh round-trip — under require_project that reconciles + // the select back to "" and submit then sends no project -> a 400. + const paint = function (fresh) { const strict = !!window.TurnstoneProjects.requireProject(); if (hint) hint.textContent = strict ? "required" : "optional"; - _populateProjectSelect(sel, { requireProject: strict }); + _populateProjectSelect(sel, { requireProject: strict, fresh: fresh }); }; - paint(); // sync from the warm cache (no-op when cold; the async fills it) - window.TurnstoneProjects.refreshProjects().then(paint); + const freshOnOpen = !!(opts && opts.freshOnOpen); + paint(freshOnOpen); // sync from the warm cache (no-op when cold; the async fills it) + window.TurnstoneProjects.refreshProjects().then(function () { + paint(false); + }); } -// Fill the model + judge-model s from the shared models cache. One list +// feeds BOTH selects with the same "alias (model)" labels; each placeholder is +// annotated with the server-resolved default alias ("Default — gpt-5"), resolved +// once via the cache's modelLabel. A fresh open renders the default (no prior +// selection); a repaint preserves each select's mid-window pick independently. +// No-op when the models bridge is absent (still loading) — the async refresh then +// fills it, exactly as before this cache existed. function _populateModelSelect(modelSel, judgeSel, opts) { if (!modelSel || !window.TurnstoneModels) return; - const annotate = !!(opts && opts.annotate); + const fresh = !!(opts && opts.fresh); const M = window.TurnstoneModels; const choices = M.modelChoices(); const defaults = M.modelDefaults(); - const models = M.getModels(); - const prevModel = modelSel.value; - const prevJudge = judgeSel ? judgeSel.value : ""; - const modelDefault = annotate - ? _resolveModelLabel(defaults.default_alias || "", models) - : ""; - const judgeDefault = annotate - ? _resolveModelLabel(defaults.judge_default_alias || "", models) - : ""; + const prevModel = fresh ? "" : modelSel.value; + const prevJudge = fresh || !judgeSel ? "" : judgeSel.value; + const modelDefault = M.modelLabel(defaults.default_alias || ""); + const judgeDefault = M.modelLabel(defaults.judge_default_alias || ""); modelSel.textContent = ""; _appendOption( modelSel, @@ -569,10 +603,11 @@ function _populateModelSelect(modelSel, judgeSel, opts) { _appendOption(modelSel, c.value, c.text, false); if (judgeSel) _appendOption(judgeSel, c.value, c.text, false); }); - // Preserve a mid-window pick on EACH select independently so the async - // repaint can't clobber a fast user's choice. - if (prevModel && _optionExists(modelSel, prevModel)) + // Preserve a mid-window pick on EACH select independently so the async repaint + // can't clobber a fast user's choice (a fresh open zeroed both above). + if (prevModel && _optionExists(modelSel, prevModel)) { modelSel.value = prevModel; + } if (judgeSel && prevJudge && _optionExists(judgeSel, prevJudge)) { judgeSel.value = prevJudge; } @@ -583,9 +618,10 @@ function _populateModelSelect(modelSel, judgeSel, opts) { // ui label appends " [MCP]" for MCP-origin skills (the console launcher does // not — which is why the cache returns raw rows). No-op when the bridge is // absent. -function _populateSkillSelect(sel) { +function _populateSkillSelect(sel, opts) { if (!sel || !window.TurnstoneSkills) return; - const previous = sel.value; + const fresh = !!(opts && opts.fresh); + const previous = fresh ? "" : sel.value; sel.textContent = ""; _appendOption(sel, "", "Use defaults", false); window.TurnstoneSkills.getSkills().forEach(function (t) { @@ -615,10 +651,15 @@ function _populateSkillSelect(sel) { function _populateProjectSelect(sel, opts) { if (!sel || !window.TurnstoneProjects) return; const mode = opts || sel._projMode || {}; - sel._projMode = mode; + // Stamp ONLY the picker mode onto the element (the inline "+ New project…" + // creator's no-opts repaint reuses it); `fresh` is read from the LIVE opts + // per-call and deliberately NOT stamped, so a fresh:true modal open can't + // persist into a later preserve-repaint. + sel._projMode = { requireProject: !!mode.requireProject }; const strict = !!mode.requireProject; + const fresh = !!(opts && opts.fresh); _ensureStandaloneProjectCreator(sel); - const previous = sel.value; + const previous = fresh ? "" : sel.value; const choices = window.TurnstoneProjects.projectChoices(); sel.replaceChildren(); if (!strict) { @@ -687,8 +728,10 @@ function submitNewWs() { const initEl = document.getElementById("new-ws-initial-message"); const initial_message = initEl ? initEl.value.trim() : ""; if (name) body.name = name; - if (model) body.model = model; - if (judge_model) body.judge_model = judge_model; + // Forks inherit their source's model + judge (the selects are hidden for a + // fork), so never override them — matches the skill/persona/project fork guards. + if (model && !_forkFromWsId) body.model = model; + if (judge_model && !_forkFromWsId) body.judge_model = judge_model; if (skill && !_forkFromWsId) body.skill = skill; // Only a FRESH create sends a project_id; a fork's project is its source's, // enforced server-side (the picker is hidden for forks, and any explicit pid is @@ -1450,46 +1493,18 @@ function _refreshDashboardSubmitLabel() { btn.textContent = hasText || hasFiles ? "Send" : "Create"; } -// Format a resolved alias with its model suffix the same way as the -// dropdown rows ("alias (model)", or just "alias" when they coincide). -// Returns "" when alias is empty or unknown so callers fall back to a -// neutral placeholder. -function _resolveModelLabel(alias, models) { - if (!alias) return ""; - for (let i = 0; i < (models || []).length; i++) { - const m = models[i]; - if (m.alias === alias) { - return m.alias === m.model ? m.alias : m.alias + " (" + m.model + ")"; - } - } - return ""; -} - function _loadDashboardOptionsLists() { - // Models — paint from the warm cache (placeholders annotated with the - // server-resolved default alias) then refresh-and-repaint. Previously - // fetch-once (guarded on options.length); now refresh-on-open via the - // coalesced models cache, matching the project/persona pickers. + // Models + skills — paint from the warm cache (model placeholders annotated + // with the server-resolved default) then refresh-and-repaint, via the shared + // wrappers (same paths the modal uses). The dashboard composer is persistent + // (not a reused dialog), so freshOnOpen:false — it preserves a pick across a + // repaint. Previously fetch-once (guarded on options.length); now + // refresh-on-open via the coalesced caches, matching the project/persona pickers. const modelSel = document.getElementById("dashboard-model"); const judgeSel = document.getElementById("dashboard-judge-model"); - if (modelSel) { - _populateModelSelect(modelSel, judgeSel, { annotate: true }); - if (window.TurnstoneModels) { - window.TurnstoneModels.refreshModels().then(function () { - _populateModelSelect(modelSel, judgeSel, { annotate: true }); - }); - } - } - // Skills — same paint-from-cache-then-refresh. + _paintModelSelects(modelSel, judgeSel, { freshOnOpen: false }); const skillSel = document.getElementById("dashboard-skill"); - if (skillSel) { - _populateSkillSelect(skillSel); - if (window.TurnstoneSkills) { - window.TurnstoneSkills.refreshSkills().then(function () { - _populateSkillSelect(skillSel); - }); - } - } + _paintSkillSelect(skillSel, { freshOnOpen: false }); // Project picker — paint from the warm cache then refresh-and-repaint (also // feeds the rail's group-by-project). Dashboard quick-create is ALWAYS a fresh