From a89808886f67c210b89b9d3a2580c695e30b03fd Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 18 Sep 2026 02:39:35 +0000 Subject: [PATCH] fix(commands): do not latch an empty skill-command cache after a failed load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadSkillCommands() published readiness in a finally block that ran even when /api/skills rejected, so one transient failure left _skillCommandCacheReady=true with an empty cache. Because ensureSkillCommandsLoadedForAutocomplete() only re-loads while !_skillCommandCacheReady && !_skillCommandLoadPromise, no later picker pass ever retried: skill commands stayed missing until a page reload even after the API recovered. Readiness is now set only after a successful current-generation commit; the promise is cleared either way so the next pass can retry. The stale-generation fence is unchanged (verified a reply from the outgoing profile still refuses to commit). This wedge is PRE-EXISTING on master (its finally sets ready unconditionally), not introduced by this PR — fixed here because this change already reworks the surrounding generation handling. Adds a reject-once/recover regression plus a __failSkillsOnce() harness knob; reverting the fix turns it red. Co-authored-by: webtecnica --- static/commands.js | 12 ++++- ..._issue7509_disabled_skills_slash_picker.py | 50 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/static/commands.js b/static/commands.js index c857e896..d431a37c 100644 --- a/static/commands.js +++ b/static/commands.js @@ -2142,6 +2142,7 @@ async function loadSkillCommands(force=false){ if(_skillCommandCacheReady&&!force)return _skillCommandCache; if(_skillCommandLoadPromise&&!force)return _skillCommandLoadPromise; const gen=_slashSkillCacheGen; + let _committed=false; _skillCommandLoadPromise=(async()=>{ try{ const data=await api('/api/skills'); @@ -2151,11 +2152,20 @@ async function loadSkillCommands(force=false){ // (and not "ready") so the composer's next pass loads the new profile (#7509). if(gen!==_slashSkillCacheGen) return _skillCommandCache; _skillCommandCache=Array.from(deduped.values()).sort((a,b)=>a.name.localeCompare(b.name)); + _committed=true; }catch(_){ if(gen===_slashSkillCacheGen)_skillCommandCache=[]; } finally{ - if(gen===_slashSkillCacheGen){_skillCommandCacheReady=true;_skillCommandLoadPromise=null;} + // Only publish the cache as "ready" after a SUCCESSFUL current-generation + // commit. Marking it ready in the failure path (what master does + // unconditionally) permanently wedges the picker: a single transient + // /api/skills rejection leaves ready=true + cache=[], and + // ensureSkillCommandsLoadedForAutocomplete() only retries when + // !ready && !promise — so skill commands stay missing until a page + // reload even after the API recovers. Clear the promise either way so the + // next picker pass can retry. + if(gen===_slashSkillCacheGen){_skillCommandCacheReady=_committed;_skillCommandLoadPromise=null;} } return _skillCommandCache; })(); diff --git a/tests/test_issue7509_disabled_skills_slash_picker.py b/tests/test_issue7509_disabled_skills_slash_picker.py index 99f033f0..a101789e 100644 --- a/tests/test_issue7509_disabled_skills_slash_picker.py +++ b/tests/test_issue7509_disabled_skills_slash_picker.py @@ -52,6 +52,7 @@ def _run_commands_js(script_body: str, skills: list) -> dict: // __releaseHeldSkills(), so a test can land a profile switch mid-request. let skillRequestCount = 0; let holdSkills = false; + let failSkillsOnce = false; const heldSkills = []; const ctx = {{ console, @@ -66,6 +67,10 @@ def _run_commands_js(script_body: str, skills: list) -> dict: if (holdSkills) {{ await new Promise((resolve) => {{ heldSkills.push(resolve); }}); }} + // Transient-failure injection: __failSkillsOnce() makes exactly the + // next /api/skills reject, so a test can prove the loader recovers + // instead of latching an empty cache as "ready". + if (failSkillsOnce) {{ failSkillsOnce = false; throw new Error('simulated /api/skills failure'); }} return {{ skills: snapshot }}; }} if (path === '/api/commands') return {{ commands: [] }}; @@ -77,6 +82,7 @@ def _run_commands_js(script_body: str, skills: list) -> dict: __setSkills: (next) => {{ skillsPayload = next; }}, __holdSkills: (on) => {{ holdSkills = !!on; }}, __releaseHeldSkills: () => {{ heldSkills.splice(0).forEach((resolve) => resolve()); }}, + __failSkillsOnce: () => {{ failSkillsOnce = true; }}, __skillRequests: () => skillRequestCount }}; vm.createContext(ctx); @@ -319,3 +325,47 @@ def test_both_profile_switch_paths_drop_the_slash_skill_caches(): "previous profile's /api/skills payload keeps hiding the new profile's skills" ) assert invalidate_call > switch_call, f"{label}: caches dropped before the switch request" + + +def test_transient_skills_failure_does_not_wedge_the_picker(): + """A failed /api/skills must not latch an empty cache as authoritative. + + ``loadSkillCommands()`` publishes its result by setting + ``_skillCommandCacheReady``, and ``ensureSkillCommandsLoadedForAutocomplete()`` + only re-loads while ``!_skillCommandCacheReady && !_skillCommandLoadPromise``. + So marking the cache "ready" on the failure path permanently wedges the + picker: one transient rejection leaves ready=true with an empty cache, and no + later picker pass ever retries -- skill commands stay missing until a page + reload even after the API recovers. + + Readiness must therefore be set only after a successful current-generation + commit. This regression pins the recovery; reverting the fix (marking ready + unconditionally in ``finally``) turns the second assertion red. + """ + result = _run_commands_js( + """ + // Bundle commands are a separate cache that getSlashAutocompleteMatches() + // also awaits; load it up front so this test isolates the skill path. + await loadBundleCommands(true); + __failSkillsOnce(); + // First pass rejects. Model the composer's own entry point: it only + // re-loads while the cache is neither ready nor in flight. + await loadSkillCommands(); + const afterFailure = skillsOffered(await getSlashAutocompleteMatches('/gam')); + // API is healthy again. A NON-forced load is what the picker actually + // issues, so the retry has to happen without force=true. + await loadSkillCommands(); + const afterRecovery = skillsOffered(await getSlashAutocompleteMatches('/gam')); + return {afterFailure, afterRecovery, requests: __skillRequests()}; + """, + MIXED_SKILLS, + ) + assert result["afterFailure"] == [], ( + "a rejected /api/skills should leave no skill commands offered" + ) + assert result["afterRecovery"] == ["gamma-live"], ( + "the picker must recover once /api/skills succeeds again; got " + f"{result['afterRecovery']!r} -- an empty list means the failure path " + "latched _skillCommandCacheReady and no retry ever happens" + ) + assert result["requests"] >= 2, "the loader must actually re-issue /api/skills"