fix(ui): apply PR #869 review findings (turnstone + copilot)

- list_cache: null-prototype _byKey — a row keyed "__proto__" swapped the
  map's prototype via the inherited setter, and getByKey of inherited
  members ("toString", "constructor") resolved them as rows; + guard test
- list_cache: document why _pending clears BEFORE the trailing refresh
  (a .finally clear would coalesce a late force onto a stale fetch —
  declines the reviewer's .finally suggestion with the ruling in-code)
- list_cache: extra() accessor doc reflects the conditional reset;
  resetExtraOnError @param notes it is moot without extraDefaults
  (declines per-module knobs in personas/skills, which have no extra)
- ui: extract _paintFromCache — sync-mirrors-freshOnOpen /
  async-always-fresh:false now encoded once for the model/skill/persona
  wrappers and asserted at the chokepoint
- ui: replaceChildren() for the model/judge/skill picker clears
  (consistency with the persona/project populates)
- console: reword the skills fail-open comment to unambiguous past tense;
  drop the orphaned _resolveModelLabel docstring
- tests: fork-gate asserts require each paint to open its own
  `if (!_forkFromWsId)` block (the rfind+50 window false-passed a closed
  gate; the model first-gate check was vacuous; the persona gate was
  unasserted); drop one redundant `0 <=` (kept where it guards find()==-1)

tests/test_app_js.py

@@ -2517,31 +2517,47 @@ _PERSONAS_JS = Path(__file__).resolve().parent.parent / "turnstone/shared_static
 
 
 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)."""
+    """The _paintFromCache chokepoint (PR #869 review: the three wrapper twins
+    repeated the same wiring, so the discipline now lives ONCE) paints from the
+    warm cache SYNCHRONOUSLY, then refreshes-and-repaints.  CRITICAL: the sync
+    paint mirrors the caller's freshOnOpen but the ASYNC repaint MUST pass
+    fresh:false — leaking freshOnOpen into the async paint would clobber a
+    mid-window pick (for the project picker that reconciles to "" -> a
+    require_project 400).  Each wrapper must pair its OWN bridge refresh with
+    its OWN populate helper and thread `fresh` through untouched."""
     body = _APP_JS.read_text(encoding="utf-8")
-    for wrapper, populate, refresh in (
-        ("function _paintModelSelects(", "_populateModelSelect(", "refreshModels().then"),
-        ("function _paintSkillSelect(", "_populateSkillSelect(", "refreshSkills().then"),
+    helper = _slice_top_level_fn(body, "function _paintFromCache(")
+    sync = helper.find("repaint(!!(opts && opts.freshOnOpen))")
+    async_ = helper.find("refresh().then")
+    assert 0 <= sync < async_, (
+        "_paintFromCache must sync-paint (repaint mirroring freshOnOpen) BEFORE "
+        "the async refresh repaint"
+    )
+    assert "repaint(false)" in helper[async_:], (
+        "_paintFromCache's async repaint must pass fresh:false (never leak "
+        "freshOnOpen, or it clobbers a mid-window pick)"
+    )
+    for wrapper, bridge_refresh, populate in (
+        (
+            "function _paintModelSelects(",
+            "window.TurnstoneModels && window.TurnstoneModels.refreshModels",
+            "_populateModelSelect(modelSel, judgeSel, { fresh: fresh })",
+        ),
+        (
+            "function _paintSkillSelect(",
+            "window.TurnstoneSkills && window.TurnstoneSkills.refreshSkills",
+            "_populateSkillSelect(sel, { fresh: fresh })",
+        ),
         (
             "function _paintPersonaSelect(",
-            "_populatePersonaSelect(",
-            "refreshPersonas().then",
+            "window.TurnstonePersonas && window.TurnstonePersonas.refreshPersonas",
+            "_populatePersonaSelect(sel, { fresh: fresh })",
         ),
     ):
         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)"
-        )
+        assert "_paintFromCache(" in fn, f"{wrapper} must route through _paintFromCache"
+        assert bridge_refresh in fn, f"{wrapper} must pass its own bridge refresh"
+        assert populate in fn, f"{wrapper} must thread fresh through to its own populate helper"
 
 
 def test_new_ws_modal_renders_all_selects_fresh_on_open() -> None:
@@ -2564,10 +2580,16 @@ def test_new_ws_modal_renders_all_selects_fresh_on_open() -> None:
     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"
+    # Fork-gates: the model + persona paints must each be the FIRST statement
+    # inside their own `if (!_forkFromWsId)` block.  (A first-gate ordering
+    # check is vacuous here — the first gate in showNewWsModal IS the model
+    # gate, so it would pass even with the paint hoisted out below it.)
+    assert re.search(r"if \(!_forkFromWsId\) \{\s*_paintModelSelects\(modelSelect", fn), (
+        "the modal model paint must be skipped for a fork"
+    )
+    assert re.search(r"if \(!_forkFromWsId\) \{\s*_paintPersonaSelect\(personaSelect", fn), (
+        "the modal persona paint must be skipped for a fork"
+    )
 
 
 def test_dashboard_paints_model_and_skill_via_wrappers_preserving() -> None:
@@ -2603,12 +2625,13 @@ def test_new_ws_modal_fork_inherits_model_and_judge() -> None:
     )
     # [4] fix: the skill paint is fork-gated too (skill is hidden for a fork).
     # Tie the gate to the skill paint SPECIFICALLY — a first-gate check would be
-    # satisfied by the model gate at line ~352 even if the skill paint were left
-    # unconditional, so require the nearest preceding gate to be right above it.
+    # satisfied by the model gate even if the skill paint were left
+    # unconditional, so require the paint to be the FIRST statement inside its
+    # own gate block.  (A nearest-preceding-gate proximity window false-passes
+    # a gate block that CLOSES before the paint.)
     skill_paint = modal.find("_paintSkillSelect(tplSelect")
     assert skill_paint >= 0, "the modal must paint the skill picker"
-    gate = modal.rfind("if (!_forkFromWsId) {", 0, skill_paint)
-    assert gate >= 0 and (skill_paint - gate) < 50, (
+    assert re.search(r"if \(!_forkFromWsId\) \{\s*_paintSkillSelect\(tplSelect", modal), (
         "the modal skill paint must be directly wrapped in `if (!_forkFromWsId)` "
         "(skip the wasted fetch + hidden-select rebuild on a fork)"
     )
@@ -2630,7 +2653,7 @@ def test_console_launcher_paints_model_and_skill_from_cache_synchronously() -> N
     sync_model = model_fn.find("_populateHomeModelDropdowns()")
     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, (
+    assert sync_model < async_model, (
         "the synchronous launcher model paint must precede the async refresh repaint"
     )
     skill_fn = _slice_top_level_fn(body, "function _refreshAndPopulateSkills(")
@@ -2717,6 +2740,11 @@ def test_list_cache_core_is_failopen_coalesced_and_gated() -> None:
     schedules a trailing refetch that converges to the latest (finding [2])."""
     src = _LIST_CACHE_JS.read_text(encoding="utf-8")
     assert "return _inflight;" in src, "non-force callers must coalesce onto the in-flight refresh"
+    assert src.count("_byKey = Object.create(null)") == 2, (
+        "both _byKey sites (declaration + _setCache rebuild) must be null-prototype "
+        "(a row keyed __proto__ must not swap the map's prototype; inherited members "
+        "must not resolve as rows)"
+    )
     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)"
     )

turnstone/console/static/app.js

@@ -1757,8 +1757,9 @@ function _populateHomeSkillDropdown() {
   if (!TS) return;
   // Reads the shared skills cache, which is FAIL-OPEN by design: a transient
   // non-OK refresh keeps the last-known rows rather than blanking to the
-  // placeholder (main blanked via `r.ok ? json : {skills:[]}`; this matches the
-  // projects/personas policy).  A since-removed skill is rejected server-side on
+  // placeholder (the pre-cache inline fetch blanked on any non-OK via
+  // `r.ok ? json : {skills:[]}`; this cache matches the projects/personas
+  // policy instead).  A since-removed skill is rejected server-side on
   // launch — a narrow stale-selection window traded for not blanking on a blip.
   const previous = _homeCoordComposer.getOptionValue("skill");
   const choices = TS.getSkills().map(function (t) {
@@ -1776,10 +1777,6 @@ function _populateHomeSkillDropdown() {
   if (stillValid) _homeCoordComposer.setOptionValue("skill", previous);
 }
 
-// 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 can fall back
-// to a neutral placeholder.
 // 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

turnstone/shared_static/list_cache.js

@@ -45,6 +45,8 @@ import { authFetch } from "./auth.js";
  *                                      false for a merely COSMETIC extra that
  *                                      should keep its last-known value through a
  *                                      transient failure rather than blank.
+ *                                      Moot unless extraDefaults is set — the
+ *                                      reset branch is gated on both.
  * `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
@@ -66,7 +68,12 @@ export function makeListCache(opts) {
   const resetExtraOnError = opts.resetExtraOnError !== false;
 
   let _cache = []; // last-fetched rows (caller-visible)
-  let _byKey = {}; // keyField value -> row, for O(1) lookup (when keyField set)
+  // keyField value -> row, for O(1) lookup (when keyField set).  Null-prototype:
+  // a row keyed "__proto__" must not swap this map's prototype via the inherited
+  // setter, and an absent-key lookup ("toString", "constructor") must miss (->
+  // undefined -> getByKey's null) instead of resolving an inherited
+  // Object.prototype member as a "row".
+  let _byKey = Object.create(null);
   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
@@ -87,7 +94,7 @@ export function makeListCache(opts) {
   function _setCache(rows) {
     _cache = Array.isArray(rows) ? rows : [];
     if (keyField) {
-      _byKey = {};
+      _byKey = Object.create(null);
       for (const r of _cache) if (r && r[keyField]) _byKey[r[keyField]] = r;
     }
     const firstLoad = !_loaded;
@@ -122,7 +129,13 @@ export function makeListCache(opts) {
         // 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.
+        // callers wedge on a stuck promise.  The _pending = null clear is
+        // deliberately BEFORE the recursive refresh() (NOT in a .finally after
+        // it): a force landing DURING the trailing refetch must find _pending
+        // null and chain a NEW trailing fetch, because its invalidation
+        // postdates this one's start.  Clearing after the trailing refresh
+        // resolves would coalesce that late force onto a fetch that predates
+        // its change — the exact race this path exists to close.
         _pending =
           _pending ||
           _inflight.then(function () {
@@ -197,7 +210,8 @@ export function makeListCache(opts) {
       return _lastError;
     },
     /** The captured extra top-level fields (fail-open defaults until a
-     *  successful refresh; reset to those defaults on any failure). */
+     *  successful refresh; reset to those defaults on failure ONLY when
+     *  resetExtraOnError — a cosmetic extra keeps its last-known value). */
     extra: function () {
       return _extra;
     },

turnstone/ui/static/app.js

@@ -545,46 +545,59 @@ function _paintProjectPicker(sel, hint, opts) {
   });
 }
 
-// Paint the model + judge pickers from the warm cache then refresh-and-repaint,
-// collapsing the identical sync-then-refresh dance the modal and dashboard both
-// need (mirrors _paintProjectPicker).  freshOnOpen renders the actual defaults on
-// a reused-dialog open; the async repaint ALWAYS preserves (fresh:false) so a
-// mid-window pick survives.  The populate helper no-ops when the bridge/select is
-// absent (cold cache -> the async refresh fills it, never worse than before).
-function _paintModelSelects(modelSel, judgeSel, opts) {
-  const freshOnOpen = !!(opts && opts.freshOnOpen);
-  _populateModelSelect(modelSel, judgeSel, { fresh: freshOnOpen });
-  if (window.TurnstoneModels) {
-    window.TurnstoneModels.refreshModels().then(function () {
-      _populateModelSelect(modelSel, judgeSel, { fresh: false });
+// One chokepoint for the composer paint discipline, shared by the three
+// cache-backed wrappers below (mirrors _paintProjectPicker, which keeps its own
+// tail — fork/hint logic): sync-paint NOW from the warm cache, then
+// refresh-and-repaint.  The sync paint mirrors the caller's freshOnOpen (a
+// reused-dialog open renders the actual defaults); the async repaint is ALWAYS
+// fresh:false — it must preserve a pick made during the fetch window, never
+// re-blank.  `refresh` may be absent (bridge module still loading): the
+// populate helpers no-op without their bridge and the refresh is skipped —
+// never worse than the pre-cache behavior.
+function _paintFromCache(refresh, repaint, opts) {
+  repaint(!!(opts && opts.freshOnOpen));
+  if (refresh) {
+    refresh().then(function () {
+      repaint(false);
     });
   }
 }
 
+// Paint the model + judge pickers from the warm cache then refresh-and-repaint,
+// collapsing the identical sync-then-refresh dance the modal and dashboard both
+// need.
+function _paintModelSelects(modelSel, judgeSel, opts) {
+  _paintFromCache(
+    window.TurnstoneModels && window.TurnstoneModels.refreshModels,
+    function (fresh) {
+      _populateModelSelect(modelSel, judgeSel, { fresh: fresh });
+    },
+    opts,
+  );
+}
+
 // Skill twin of _paintModelSelects.
 function _paintSkillSelect(sel, opts) {
-  const freshOnOpen = !!(opts && opts.freshOnOpen);
-  _populateSkillSelect(sel, { fresh: freshOnOpen });
-  if (window.TurnstoneSkills) {
-    window.TurnstoneSkills.refreshSkills().then(function () {
-      _populateSkillSelect(sel, { fresh: false });
-    });
-  }
+  _paintFromCache(
+    window.TurnstoneSkills && window.TurnstoneSkills.refreshSkills,
+    function (fresh) {
+      _populateSkillSelect(sel, { fresh: fresh });
+    },
+    opts,
+  );
 }
 
-// Persona twin of _paintModelSelects (fourth composer picker on the same shape,
-// so the modal + dashboard don't maintain the persona sync-then-refresh dance
-// two different ways).  _populatePersonaSelect keeps the kind default when
-// nothing valid is selected, so the async fresh:false repaint can't clobber a
-// mid-window pick.
+// Persona twin of _paintModelSelects.  _populatePersonaSelect keeps the kind
+// default when nothing valid is selected, so the fresh:false repaint can't
+// clobber a mid-window pick.
 function _paintPersonaSelect(sel, opts) {
-  const freshOnOpen = !!(opts && opts.freshOnOpen);
-  _populatePersonaSelect(sel, { fresh: freshOnOpen });
-  if (window.TurnstonePersonas) {
-    window.TurnstonePersonas.refreshPersonas().then(function () {
-      _populatePersonaSelect(sel, { fresh: false });
-    });
-  }
+  _paintFromCache(
+    window.TurnstonePersonas && window.TurnstonePersonas.refreshPersonas,
+    function (fresh) {
+      _populatePersonaSelect(sel, { fresh: fresh });
+    },
+    opts,
+  );
 }
 
 // Fill the model + judge-model <select>s from the shared models cache.  One list
@@ -604,7 +617,7 @@ function _populateModelSelect(modelSel, judgeSel, opts) {
   const prevJudge = fresh || !judgeSel ? "" : judgeSel.value;
   const modelDefault = M.modelLabel(defaults.default_alias || "");
   const judgeDefault = M.modelLabel(defaults.judge_default_alias || "");
-  modelSel.textContent = "";
+  modelSel.replaceChildren();
   _appendOption(
     modelSel,
     "",
@@ -612,7 +625,7 @@ function _populateModelSelect(modelSel, judgeSel, opts) {
     false,
   );
   if (judgeSel) {
-    judgeSel.textContent = "";
+    judgeSel.replaceChildren();
     _appendOption(
       judgeSel,
       "",
@@ -643,7 +656,7 @@ function _populateSkillSelect(sel, opts) {
   if (!sel || !window.TurnstoneSkills) return;
   const fresh = !!(opts && opts.fresh);
   const previous = fresh ? "" : sel.value;
-  sel.textContent = "";
+  sel.replaceChildren();
   _appendOption(sel, "", "Use defaults", false);
   window.TurnstoneSkills.getSkills().forEach(function (t) {
     let label = t.name;