From 0d8be572c71e1af19ebf4df467aa47efd8664314 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sun, 19 Jul 2026 01:18:18 -0700 Subject: [PATCH] fix(ui): apply PR #869 review findings (turnstone + copilot) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 | 84 ++++++++++++++++++--------- turnstone/console/static/app.js | 9 +-- turnstone/shared_static/list_cache.js | 22 +++++-- turnstone/ui/static/app.js | 81 +++++++++++++++----------- 4 files changed, 124 insertions(+), 72 deletions(-) diff --git a/tests/test_app_js.py b/tests/test_app_js.py index 7c6635e7..d666f487 100644 --- a/tests/test_app_js.py +++ b/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)" ) diff --git a/turnstone/console/static/app.js b/turnstone/console/static/app.js index 0946e610..22a00d13 100644 --- a/turnstone/console/static/app.js +++ b/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 diff --git a/turnstone/shared_static/list_cache.js b/turnstone/shared_static/list_cache.js index 8f71ef41..ebe498ad 100644 --- a/turnstone/shared_static/list_cache.js +++ b/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; }, diff --git a/turnstone/ui/static/app.js b/turnstone/ui/static/app.js index 705f74ee..3421f6b1 100644 --- a/turnstone/ui/static/app.js +++ b/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