From 65e968c447623c60391159c290e2721ce630a762 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 11 Sep 2026 17:24:11 -0300 Subject: [PATCH] fix(commands): drop slash-skill caches on profile switch The slash-skill caches (_slashSkillCache, _slashSkillCachePromise, _skillCommandCache) outlived a profile switch, so the outgoing profile's /api/skills payload kept hiding skills that the incoming profile enables (#7509). - call invalidateSlashSkillCaches() from both switch handlers (static/panels.js switchToProfile, static/sessions.js _switchProfileForSessionLoad), after the /api/profile/switch request - bump a generation counter in invalidateSlashSkillCaches() and capture it in both loaders: a reply issued for the outgoing profile can no longer commit once the switch has landed - tests: in-flight race and cache-refresh regression on both picker surfaces, plus a check that both switch paths call the invalidation --- static/commands.js | 31 +++- static/panels.js | 9 ++ static/sessions.js | 4 + ..._issue7509_disabled_skills_slash_picker.py | 141 +++++++++++++++++- 4 files changed, 179 insertions(+), 6 deletions(-) diff --git a/static/commands.js b/static/commands.js index f8513b53..c857e896 100644 --- a/static/commands.js +++ b/static/commands.js @@ -250,8 +250,15 @@ let _slashPersonalityCachePromise=null; let _bundleCommandCache=[]; let _bundleCommandLoadPromise=null; let _bundleCommandCacheReady=false; +// Bumped by invalidateSlashSkillCaches(). The two skill-cache loaders below capture +// it when they issue /api/skills and refuse to commit when it moved while their +// response was in flight: a reply generated for the previous profile would otherwise +// land after a profile switch and repopulate the caches with that profile's +// disabled-filtered payload, keeping a skill that is enabled in the new profile +// hidden (#7509). let _slashSkillCache=null; let _slashSkillCachePromise=null; +let _slashSkillCacheGen=0; let _agentCommandCache=null; let _agentCommandCachePromise=null; @@ -361,6 +368,7 @@ function _isSkillDisabled(skill){ async function _loadSlashSkillSubArgs(force=false){ if(_slashSkillCache&&!force) return _slashSkillCache; if(_slashSkillCachePromise&&!force) return _slashSkillCachePromise; + const gen=_slashSkillCacheGen; _slashSkillCachePromise=(async()=>{ try{ const data=await api('/api/skills'); @@ -371,19 +379,26 @@ async function _loadSlashSkillSubArgs(force=false){ if(name) values.push(name); } const deduped=Array.from(new Set(values)).sort((a,b)=>a.localeCompare(b)); + // A profile switch during the request bumped the generation: this payload + // belongs to the previous profile, so leave the cache empty for the fresh + // read instead of publishing stale names (#7509). + if(gen!==_slashSkillCacheGen) return _slashSkillCache||[]; _slashSkillCache=deduped; return deduped; }catch(_){ - _slashSkillCache=null; + if(gen===_slashSkillCacheGen) _slashSkillCache=null; return []; }finally{ - _slashSkillCachePromise=null; + if(gen===_slashSkillCacheGen) _slashSkillCachePromise=null; } })(); return _slashSkillCachePromise; } function invalidateSlashSkillCaches(){ + // Bump before dropping the caches: an /api/skills response still in flight was + // generated for the previous profile and must not commit once it lands (#7509). + _slashSkillCacheGen++; _slashSkillCache=null; _slashSkillCachePromise=null; _skillCommandCache=[]; @@ -2126,14 +2141,22 @@ function _buildBundleCommandEntry(bundle){ async function loadSkillCommands(force=false){ if(_skillCommandCacheReady&&!force)return _skillCommandCache; if(_skillCommandLoadPromise&&!force)return _skillCommandLoadPromise; + const gen=_slashSkillCacheGen; _skillCommandLoadPromise=(async()=>{ try{ const data=await api('/api/skills'); const deduped=new Map(); for(const skill of (data&&data.skills)||[]){const entry=_buildSkillCommandEntry(skill);if(entry&&!deduped.has(entry.name))deduped.set(entry.name,entry);} + // Bumped by a profile switch while we were awaiting: keep the cache empty + // (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)); - }catch(_){_skillCommandCache=[];} - finally{_skillCommandCacheReady=true;_skillCommandLoadPromise=null;} + }catch(_){ + if(gen===_slashSkillCacheGen)_skillCommandCache=[]; + } + finally{ + if(gen===_slashSkillCacheGen){_skillCommandCacheReady=true;_skillCommandLoadPromise=null;} + } return _skillCommandCache; })(); return _skillCommandLoadPromise; diff --git a/static/panels.js b/static/panels.js index b53a89c6..248a7853 100644 --- a/static/panels.js +++ b/static/panels.js @@ -7070,6 +7070,15 @@ async function switchToProfile(name) { if (typeof _resetCronUnreadForProfileSwitch === 'function') { _resetCronUnreadForProfileSwitch(); } + // #7509: the slash-skill caches hold the previous profile's disabled-filtered + // /api/skills payload, so drop them once the switch has actually succeeded — + // otherwise a skill that is enabled in the new profile stays hidden behind the + // old payload. Invalidating here (rather than before the POST) also kills any + // response still in flight, so the previous profile's reply can't repopulate + // the caches after this point (see invalidateSlashSkillCaches in commands.js). + if (typeof window !== 'undefined' && typeof window.invalidateSlashSkillCaches === 'function') { + window.invalidateSlashSkillCaches(); + } const targetActiveProfile = S.activeProfile || 'default'; let sessionProfileMatchesTarget = true; if (!sessionInProgress && S.session) { diff --git a/static/sessions.js b/static/sessions.js index 7be28623..c6a302a4 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -1660,6 +1660,10 @@ async function _switchProfileForSessionLoad(profile){ if(typeof _resetCronUnreadForProfileSwitch==='function'){ _resetCronUnreadForProfileSwitch(); } + // #7509: mirror the canonical switch in panels.js — the slash-skill caches still + // hold the previous profile's /api/skills payload, so drop them (and any reply + // still in flight) once the switch has succeeded. + if(typeof window!=='undefined'&&typeof window.invalidateSlashSkillCaches==='function') window.invalidateSlashSkillCaches(); if(typeof _clearPersistedModelState==='function') _clearPersistedModelState(); else localStorage.removeItem('hermes-webui-model'); if(data.default_model) window._defaultModel=data.default_model; diff --git a/tests/test_issue7509_disabled_skills_slash_picker.py b/tests/test_issue7509_disabled_skills_slash_picker.py index f5048e0c..99f033f0 100644 --- a/tests/test_issue7509_disabled_skills_slash_picker.py +++ b/tests/test_issue7509_disabled_skills_slash_picker.py @@ -12,11 +12,17 @@ both surfaces reported in the issue: State space covered: 0 / 1 / many skills, enabled / disabled / flag-absent entries, both surfaces, and a cache-refresh cycle after a skill is disabled. + +Profile-switch coverage (#7509 follow-up): a skill that is disabled in the outgoing +profile and enabled in the incoming one must become visible on the next picker pass, +including when the outgoing profile's ``/api/skills`` reply is still in flight while +the switch lands -- that stale reply must not repopulate the caches. """ from __future__ import annotations import json +import re import subprocess import tempfile import textwrap @@ -42,19 +48,36 @@ def _run_commands_js(script_body: str, skills: list) -> dict: f""" const vm = require('vm'); let skillsPayload = {json.dumps(skills)}; + // In-flight control: __holdSkills(true) parks every /api/skills reply until + // __releaseHeldSkills(), so a test can land a profile switch mid-request. + let skillRequestCount = 0; + let holdSkills = false; + const heldSkills = []; const ctx = {{ console, localStorage: {{ getItem(){{return null;}}, setItem(){{}}, removeItem(){{}} }}, t: (key) => key, api: async (path) => {{ - if (path === '/api/skills') return {{ skills: skillsPayload }}; + if (path === '/api/skills') {{ + skillRequestCount++; + // Snapshot at request time: a reply produced for the outgoing profile + // must not be able to read a payload that only exists after the switch. + const snapshot = skillsPayload; + if (holdSkills) {{ + await new Promise((resolve) => {{ heldSkills.push(resolve); }}); + }} + return {{ skills: snapshot }}; + }} if (path === '/api/commands') return {{ commands: [] }}; if (path === '/api/commands/bundles') return {{ bundles: [] }}; throw new Error('unexpected api path: ' + path); }}, // Test-only knob, same idiom as the other commands.js harnesses: lets the // in-context script swap the mocked /api/skills payload mid-run. - __setSkills: (next) => {{ skillsPayload = next; }} + __setSkills: (next) => {{ skillsPayload = next; }}, + __holdSkills: (on) => {{ holdSkills = !!on; }}, + __releaseHeldSkills: () => {{ heldSkills.splice(0).forEach((resolve) => resolve()); }}, + __skillRequests: () => skillRequestCount }}; vm.createContext(ctx); vm.runInContext({json.dumps(COMMANDS_JS)}, ctx); @@ -182,3 +205,117 @@ def test_newly_disabled_skill_disappears_after_cache_refresh(): assert result["before"] == {"commands": ["zeta-live"], "sub_args": ["zeta-live"]}, result assert result["after"] == {"commands": [], "sub_args": []}, result + +# A payload shaped like the one the outgoing profile served: the skill the incoming +# profile enables is present here, flagged disabled, so a stale cache entry (or a +# stale in-flight reply) keeps it hidden. +_DISABLED_SHARED_SKILL = [ + {"name": "shared-skill", "description": "Disabled in the outgoing profile", "disabled": True}, +] +_ENABLED_SHARED_SKILL = [ + {"name": "shared-skill", "description": "Enabled in the incoming profile"}, +] + + +def test_skill_enabled_in_new_profile_appears_in_both_surfaces_after_switch(): + result = _run_commands_js( + """ + await loadPicker(); + const before = { + commands: skillsOffered(await getSlashAutocompleteMatches('/sh')), + sub_args: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + }; + // The profile switch drops the caches (invalidateSlashSkillCaches) and + // nothing forces a reload afterwards, so the picker's own non-forced load + // is what has to serve the incoming profile's payload. + __setSkills([{ name: 'shared-skill', description: 'Enabled in the incoming profile' }]); + invalidateSlashSkillCaches(); + await loadSkillCommands(); + const after = { + commands: skillsOffered(await getSlashAutocompleteMatches('/sh')), + sub_args: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + }; + return { before, after }; + """, + _DISABLED_SHARED_SKILL, + ) + + assert result["before"] == {"commands": [], "sub_args": []}, result + assert result["after"] == {"commands": ["shared-skill"], "sub_args": ["shared-skill"]}, result + + +def test_inflight_skills_reply_cannot_repopulate_caches_after_switch(): + result = _run_commands_js( + """ + await loadPicker(); + const before = { + commands: skillsOffered(await getSlashAutocompleteMatches('/sh')), + sub_args: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + }; + // The caches are empty (as they are after the picker's own refresh), so the + // next picker pass issues one /api/skills request per surface for the + // outgoing profile. Park both of them, then land the switch. + invalidateSlashSkillCaches(); + __holdSkills(true); + const inflightCommands = loadSkillCommands(); + const inflightSubArgs = getSlashAutocompleteMatches('/use '); + __setSkills([{ name: 'shared-skill', description: 'Enabled in the incoming profile' }]); + invalidateSlashSkillCaches(); + __holdSkills(false); + __releaseHeldSkills(); + await inflightCommands; + await inflightSubArgs; + await loadSkillCommands(); + const after = { + commands: skillsOffered(await getSlashAutocompleteMatches('/sh')), + sub_args: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + requests: __skillRequests(), + }; + return { before, after }; + """, + _DISABLED_SHARED_SKILL, + ) + + assert result["before"] == {"commands": [], "sub_args": []}, result + assert result["after"]["commands"] == ["shared-skill"], result + assert result["after"]["sub_args"] == ["shared-skill"], result + # 1 from loadPicker(), 1 held reply that must be discarded, 1 fresh fetch: a + # cache that simply stayed empty would be wrong, the picker has to reload. + assert result["after"]["requests"] >= 3, result + + +PANELS_JS = (ROOT / "static" / "panels.js").read_text(encoding="utf-8") +SESSIONS_JS = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8") + + +def _top_level_function_body(source: str, name: str) -> str: + """Return the source of a column-0 `function name(...)` declaration.""" + match = re.search(r"^(?:async\s+)?function\s+" + re.escape(name) + r"\s*\(", source, re.MULTILINE) + assert match, f"{name} not found" + end = source.find("\n}\n", match.start()) + assert end != -1, f"{name}: closing brace not found" + return source[match.start():end + 3] + + +def test_both_profile_switch_paths_drop_the_slash_skill_caches(): + """The switch handlers must call the invalidation once the POST has succeeded. + + The handlers cannot be executed in isolation here (they touch most of the app + shell), so this is a structural check: the invalidation call has to live inside + the switch function, after the /api/profile/switch request -- dropping the + caches before the request would let a reply from the outgoing profile commit + once the switch is done. + """ + for label, source, function_name in ( + ("panels.switchToProfile", PANELS_JS, "switchToProfile"), + ("sessions._switchProfileForSessionLoad", SESSIONS_JS, "_switchProfileForSessionLoad"), + ): + body = _top_level_function_body(source, function_name) + switch_call = body.find("/api/profile/switch") + invalidate_call = body.find("invalidateSlashSkillCaches()") + assert switch_call != -1, f"{label}: no /api/profile/switch call in {function_name}" + assert invalidate_call != -1, ( + f"{label}: {function_name} does not drop the slash-skill caches, so the " + "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"