Merge pull request #7627 from nesquena/release/stage-318
Release exp-v0.52.318: slash-command correctness (#7623, #7511)
This commit is contained in:
5 files changed
+486
-43
No files matched your search
@@ -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 <skill>` 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)
|
||||
|
||||
+97
-43
@@ -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;
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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"
|
||||
Reference in new issue
Block a user