From 7003f9dc5bdf34c111beffb39db12a5211f07148 Mon Sep 17 00:00:00 2001 From: hermes Date: Thu, 17 Sep 2026 19:28:30 +0100 Subject: [PATCH 1/6] fix(webui): let /skills write-approval subcommands fall through to the agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmdSkills intercepted every /skills argument as a local search filter, so /skills pending|approve|reject|diff|approval|mode never reached the agent's own write-approval handler (hermes_cli/write_approval_commands.py via gateway/slash_commands.py) — pending memory/skill writes staged by skills.write_approval could not be reviewed or approved from the WebUI at all, only via CLI/gateway. cmdSkills now checks the first argument against the known agent subcommand names and returns false to opt out of local handling (matching the existing /reasoning fall-through convention), letting those subcommands reach the agent like any other unrecognized text. Co-Authored-By: Claude Sonnet 5 --- static/commands.js | 88 ++++++++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 39 deletions(-) diff --git a/static/commands.js b/static/commands.js index e322c1d8..3daf787d 100644 --- a/static/commands.js +++ b/static/commands.js @@ -1101,47 +1101,57 @@ 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. +const SKILLS_AGENT_SUBCOMMANDS=['pending','approve','reject','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){ From a865c7f68802ccc950bd6eb7e66f8663c8ffe209 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Thu, 17 Sep 2026 22:56:36 +0000 Subject: [PATCH 2/6] fix(webui): also fall through the approve/reject/approval aliases The agent's handle_pending_subcommand() accepts aliases the initial list missed: approve|apply, reject|deny|drop. Those three were still swallowed by the local /skills search, so '/skills apply 3' and '/skills deny 3' silently ran a skill-name search instead of reaching the write-approval handler. Completes the class rather than the reported instance. Co-authored-by: totalitarian --- static/commands.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/static/commands.js b/static/commands.js index 3daf787d..6df02358 100644 --- a/static/commands.js +++ b/static/commands.js @@ -1104,7 +1104,9 @@ async function cmdTheme(args){ // 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. -const SKILLS_AGENT_SUBCOMMANDS=['pending','approve','reject','diff','approval','mode']; +// 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(); From 9cb0235f5e6ad980a6204f527d2de52b9a5570cd Mon Sep 17 00:00:00 2001 From: webtecnica Date: Thu, 10 Sep 2026 15:23:28 -0300 Subject: [PATCH 3/6] fix(commands): keep disabled skills out of the slash-command picker `skills.disabled` entries are already excluded from the backend skill-command map, but the composer still offered them as selectable slash commands and as `/use` sub-args: both picker surfaces read `/api/skills` and ignored the `disabled` flag the endpoint reports per skill. Add one `_isSkillDisabled()` gate in static/commands.js and apply it at the two places that turn `/api/skills` rows into completions: * `_buildSkillCommandEntry()` returns null for a disabled skill, which keeps `_skillCommandCache` (and every consumer of it, including the dropdown) clean; * `_loadSlashSkillSubArgs()` skips disabled skills when building `/use` sub-args. Both caches still rebuild from `/api/skills`, so a skill toggled off in the skills panel disappears from the picker on the next load. Adds tests/test_issue7509_disabled_skills_slash_picker.py, which runs the real static/commands.js in a vm context with a mocked `api()` and asserts on `getSlashAutocompleteMatches()` for both surfaces, over 0 / 1 / many skills, enabled / disabled / flag-absent entries and a disable-then-refresh cycle. Fixes #7509 --- static/commands.js | 9 + ..._issue7509_disabled_skills_slash_picker.py | 184 ++++++++++++++++++ 2 files changed, 193 insertions(+) create mode 100644 tests/test_issue7509_disabled_skills_slash_picker.py diff --git a/static/commands.js b/static/commands.js index e322c1d8..f8513b53 100644 --- a/static/commands.js +++ b/static/commands.js @@ -351,6 +351,13 @@ 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; @@ -359,6 +366,7 @@ async function _loadSlashSkillSubArgs(force=false){ 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); } @@ -2096,6 +2104,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; 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..f5048e0c --- /dev/null +++ b/tests/test_issue7509_disabled_skills_slash_picker.py @@ -0,0 +1,184 @@ +"""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. +""" + +from __future__ import annotations + +import json +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)}; + 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/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; }} + }}; + 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 From 65e968c447623c60391159c290e2721ce630a762 Mon Sep 17 00:00:00 2001 From: webtecnica Date: Fri, 11 Sep 2026 17:24:11 -0300 Subject: [PATCH 4/6] 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" From a89808886f67c210b89b9d3a2580c695e30b03fd Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 18 Sep 2026 02:39:35 +0000 Subject: [PATCH 5/6] 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" From 262236147eb7c99d2f3e52797c93147e0bf3b74b Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 18 Sep 2026 03:00:14 +0000 Subject: [PATCH 6/6] changelog: /skills write-approval fall-through (#7623) + disabled-skill picker filter (#7511) --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) 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)