diff --git a/CHANGELOG.md b/CHANGELOG.md index c61bece8..671f4e9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,11 @@ ## [Unreleased] +### Fixed + +- **`/skills pending` and the other write-approval subcommands reach the agent again.** The WebUI's local `/skills` handler swallowed every argument into its own skill-name search, so the agent-owned write-approval subcommands never reached their handler — `/skills pending`, `/skills approve 3`, `/skills diff` and friends silently ran a name search instead of doing anything. Those subcommands now fall through to the normal send path using the existing opt-out contract that `/reasoning` already uses. Covers every alias the agent handler accepts, including `apply`, `deny` and `drop`, which a first pass missed. Thanks @totalitarian. (#7623) +- **Disabled skills are no longer offered by the slash-command picker.** Skills in `skills.disabled` are already excluded from the backend skill-command map, but the WebUI listed every `/api/skills` entry regardless — so a disabled skill stayed selectable in both `/use ` sub-args and the `/`-prefix skill suggestions. Both surfaces now share one filter. Switching profiles is also fenced: an `/api/skills` reply still in flight when the switch lands can no longer commit, so a payload generated for the outgoing profile cannot keep hiding a skill that is enabled in the new one. Separately, a transient `/api/skills` failure no longer wedges the picker — readiness was previously published even when the request rejected, which left an empty cache marked authoritative and stopped the composer from ever retrying until a page reload. Thanks @webtecnica. (#7511, #7509) + ### Documentation - **`AGENTS.md` now routes contributors to the references that match their change.** The old "read first" list asked for four files up front regardless of what was being changed, and carried a compressed copy of the ten change guidelines that `docs/GUIDELINES.md` owns. It now maps each reference to the kind of work it applies to and states explicit completion/verification criteria instead. No information is lost — the ten rules remain in `docs/GUIDELINES.md`, which the new version still points to. Thanks @steveafrost. (#7593) diff --git a/static/commands.js b/static/commands.js index e322c1d8..685cbf5c 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; @@ -351,31 +358,47 @@ async function _loadSlashPersonalitySubArgs(force=false){ return _slashPersonalityCachePromise; } +// Disabled skills are excluded from the backend skill-command map, so the slash +// picker surfaces must agree. Single gate for every place that turns /api/skills +// entries into selectable completions. +function _isSkillDisabled(skill){ + return !!(skill&&skill.disabled); +} + 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'); const values=[]; for(const skill of (data&&data.skills)||[]){ + if(_isSkillDisabled(skill)) continue; const name=_normalizeSlashSubArg(skill&&skill.name); 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=[]; @@ -1101,47 +1124,59 @@ async function cmdTheme(args){ showToast(t('theme_usage')+themes.join('|')+' | '+skins.join('|')+' | legacy:'+legacyThemes.join('|')); } -async function cmdSkills(args){ - try{ - const data = await api('/api/skills'); - let skills = data.skills || []; - if(args){ - const q = args.toLowerCase(); - skills = skills.filter(s => - (s.name||'').toLowerCase().includes(q) || - (s.description||'').toLowerCase().includes(q) || - (s.category||'').toLowerCase().includes(q) - ); - } - if(!skills.length){ - const msg = {role:'assistant', content: args ? `No skills matching "${args}".` : 'No skills found.'}; - S.messages.push(msg); renderMessages(); return; - } - // Group by category - const byCategory = {}; - skills.forEach(s => { - const cat = s.category || 'General'; - if(!byCategory[cat]) byCategory[cat] = []; - byCategory[cat].push(s); - }); - const lines = []; - for(const [cat, items] of Object.entries(byCategory).sort()){ - lines.push(`**${cat}**`); - items.forEach(s => { - const desc = s.description ? ` — ${s.description.slice(0,80)}${s.description.length>80?'...':''}` : ''; - lines.push(` \`${s.name}\`${desc}`); +// Subcommands owned by the agent's own /skills write-approval handler +// (hermes_cli/write_approval_commands.py via gateway/slash_commands.py) — these must +// fall through to the normal send path rather than be swallowed by the local search below. +// Includes every alias that handler accepts: approve/apply, reject/deny/drop, approval/mode. +// Keep in sync with handle_pending_subcommand() — a missing alias is silently swallowed here. +const SKILLS_AGENT_SUBCOMMANDS=['pending','approve','apply','reject','deny','drop','diff','approval','mode']; + +function cmdSkills(args){ + const sub=(args||'').trim().split(/\s+/)[0].toLowerCase(); + if(SKILLS_AGENT_SUBCOMMANDS.includes(sub)) return false; + (async()=>{ + try{ + const data = await api('/api/skills'); + let skills = data.skills || []; + if(args){ + const q = args.toLowerCase(); + skills = skills.filter(s => + (s.name||'').toLowerCase().includes(q) || + (s.description||'').toLowerCase().includes(q) || + (s.category||'').toLowerCase().includes(q) + ); + } + if(!skills.length){ + const msg = {role:'assistant', content: args ? `No skills matching "${args}".` : 'No skills found.'}; + S.messages.push(msg); renderMessages(); return; + } + // Group by category + const byCategory = {}; + skills.forEach(s => { + const cat = s.category || 'General'; + if(!byCategory[cat]) byCategory[cat] = []; + byCategory[cat].push(s); }); - lines.push(''); + const lines = []; + for(const [cat, items] of Object.entries(byCategory).sort()){ + lines.push(`**${cat}**`); + items.forEach(s => { + const desc = s.description ? ` — ${s.description.slice(0,80)}${s.description.length>80?'...':''}` : ''; + lines.push(` \`${s.name}\`${desc}`); + }); + lines.push(''); + } + const header = args + ? `Skills matching "${args}" (${skills.length}):\n\n` + : `Available skills (${skills.length}):\n\n`; + S.messages.push({role:'assistant', content: header + lines.join('\n')}); + renderMessages(); + showToast(t('type_slash')); + }catch(e){ + showToast('Failed to load skills: '+e.message); } - const header = args - ? `Skills matching "${args}" (${skills.length}):\n\n` - : `Available skills (${skills.length}):\n\n`; - S.messages.push({role:'assistant', content: header + lines.join('\n')}); - renderMessages(); - showToast(t('type_slash')); - }catch(e){ - showToast('Failed to load skills: '+e.message); - } + })(); + return true; } async function cmdUse(args){ @@ -2096,6 +2131,7 @@ function _getReservedSlashCommandSlugs(){ return reserved; } function _buildSkillCommandEntry(skill){ + if(_isSkillDisabled(skill))return null; const skillName=String(skill&&skill.name||'').trim(); const slug=_skillCommandSlug(skillName); if(!slug)return null; @@ -2117,14 +2153,32 @@ function _buildBundleCommandEntry(bundle){ 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'); 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;} + _committed=true; + }catch(_){ + if(gen===_slashSkillCacheGen)_skillCommandCache=[]; + } + finally{ + // 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; })(); 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 new file mode 100644 index 00000000..a101789e --- /dev/null +++ b/tests/test_issue7509_disabled_skills_slash_picker.py @@ -0,0 +1,371 @@ +"""Issue #7509: disabled skills must not be offered by the slash-command picker. + +``skills.disabled`` entries are already excluded from the backend skill-command +map (``scan_skill_commands`` in the hermes-agent runtime), so the WebUI picker is +the surface that leaks them. This suite runs the real ``static/commands.js`` +inside a ``vm`` context with a mocked ``api()`` and asserts on +``getSlashAutocompleteMatches()`` -- the entry point the composer calls -- for +both surfaces reported in the issue: + +* skill suggestions (``/partial``) +* ``/use`` sub-args (``/use partial``) + +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 +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +COMMANDS_JS = (ROOT / "static" / "commands.js").read_text(encoding="utf-8") + +# `window` is intentionally absent, so commands.js keeps its module scope local. +_PRELUDE = """ +const skillsOffered = (matches) => matches.filter((m) => m && m.source === 'skill').map((m) => m.name).sort(); +const subArgsOffered = (matches) => matches.map((m) => String(m && m.value || '')).sort(); +const loadPicker = async () => { + await loadSkillCommands(true); + await loadBundleCommands(true); +}; +""" + + +def _run_commands_js(script_body: str, skills: list) -> dict: + """Run `script_body` against static/commands.js with /api/skills mocked.""" + script = textwrap.dedent( + 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; + let failSkillsOnce = false; + const heldSkills = []; + const ctx = {{ + console, + localStorage: {{ getItem(){{return null;}}, setItem(){{}}, removeItem(){{}} }}, + t: (key) => key, + api: async (path) => {{ + 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); }}); + }} + // 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: [] }}; + 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; }}, + __holdSkills: (on) => {{ holdSkills = !!on; }}, + __releaseHeldSkills: () => {{ heldSkills.splice(0).forEach((resolve) => resolve()); }}, + __failSkillsOnce: () => {{ failSkillsOnce = true; }}, + __skillRequests: () => skillRequestCount + }}; + vm.createContext(ctx); + vm.runInContext({json.dumps(COMMANDS_JS)}, ctx); + (async () => {{ + const result = await vm.runInContext(`(async () => {{ {_PRELUDE} {script_body} }})()`, ctx); + process.stdout.write(JSON.stringify(result)); + }})().catch(err => {{ + console.error(err && err.stack || err); + process.exit(1); + }}); + """ + ) + with tempfile.NamedTemporaryFile("w", suffix=".js", encoding="utf-8", delete=False) as handle: + handle.write(script) + script_path = Path(handle.name) + try: + proc = subprocess.run(["node", str(script_path)], capture_output=True, text=True) + finally: + script_path.unlink(missing_ok=True) + if proc.returncode != 0: + raise RuntimeError(f"node harness failed (exit {proc.returncode}):\n{proc.stderr}") + return json.loads(proc.stdout) + + +MIXED_SKILLS = [ + {"name": "gamma-live", "description": "Enabled skill", "disabled": False}, + {"name": "beta-gone", "description": "Disabled skill", "disabled": True}, + {"name": "delta-gone", "description": "Disabled skill", "disabled": True}, + {"name": "alpha-legacy", "description": "Entry without a disabled flag"}, +] + +ALL_DISABLED_SKILLS = [ + {"name": "solo-one", "description": "Disabled skill", "disabled": True}, + {"name": "solo-two", "description": "Disabled skill", "disabled": True}, +] + + +def test_disabled_skills_are_not_offered_as_skill_commands(): + result = _run_commands_js( + """ + await loadPicker(); + return { + enabled: skillsOffered(await getSlashAutocompleteMatches('/gam')), + disabled_beta: skillsOffered(await getSlashAutocompleteMatches('/bet')), + disabled_delta: skillsOffered(await getSlashAutocompleteMatches('/del')), + flag_absent: skillsOffered(await getSlashAutocompleteMatches('/alp')), + }; + """, + MIXED_SKILLS, + ) + + assert result["enabled"] == ["gamma-live"], result + assert result["disabled_beta"] == [], result + assert result["disabled_delta"] == [], result + assert result["flag_absent"] == ["alpha-legacy"], result + + +def test_disabled_skills_are_not_offered_as_use_sub_args(): + result = _run_commands_js( + """ + await loadPicker(); + return { + all: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + disabled_prefix: subArgsOffered(await getSlashAutocompleteMatches('/use bet')), + enabled_prefix: subArgsOffered(await getSlashAutocompleteMatches('/use gam')), + }; + """, + MIXED_SKILLS, + ) + + assert result["all"] == ["alpha-legacy", "gamma-live"], result + assert result["disabled_prefix"] == [], result + assert result["enabled_prefix"] == ["gamma-live"], result + + +def test_picker_stays_empty_when_every_skill_is_disabled(): + result = _run_commands_js( + """ + await loadPicker(); + return { + commands: skillsOffered(await getSlashAutocompleteMatches('/so')), + sub_args: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + }; + """, + ALL_DISABLED_SKILLS, + ) + + assert result == {"commands": [], "sub_args": []}, result + + +def test_picker_handles_an_empty_skill_list(): + result = _run_commands_js( + """ + await loadPicker(); + return { + commands: skillsOffered(await getSlashAutocompleteMatches('/an')), + sub_args: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + }; + """, + [], + ) + + assert result == {"commands": [], "sub_args": []}, result + + +def test_newly_disabled_skill_disappears_after_cache_refresh(): + result = _run_commands_js( + """ + await loadPicker(); + const before = { + commands: skillsOffered(await getSlashAutocompleteMatches('/ze')), + sub_args: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + }; + __setSkills([{ name: 'zeta-live', description: 'Disabled later', disabled: true }]); + invalidateSlashSkillCaches(); + await loadPicker(); + const after = { + commands: skillsOffered(await getSlashAutocompleteMatches('/ze')), + sub_args: subArgsOffered(await getSlashAutocompleteMatches('/use ')), + }; + return { before, after }; + """, + [{"name": "zeta-live", "description": "Enabled skill"}], + ) + + 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" + + +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"