harden(#4057): pass local skills dir to _skill_category_from_path explicitly (Opus SHOULD-FIX)
_active_skill_search_dirs filters to existing dirs, so on a host with no local skills dir but configured external dirs the local root is dropped from the list and the position-based skills_dirs[0]==local assumption misclassified the first external root as local (its flat skills silently lost their category label). Pass the local dir explicitly (backward-compatible optional param defaulting to the old skills_dirs[0] behavior) + regression test for the absent-local-dir case.
This commit is contained in:
@@ -226,13 +226,24 @@ def _skill_path_within(base_dir: Path, candidate: Path) -> bool:
|
||||
return False
|
||||
|
||||
|
||||
def _skill_category_from_path(skill_md: Path, skills_dirs: list[Path]) -> str | None:
|
||||
def _skill_category_from_path(
|
||||
skill_md: Path,
|
||||
skills_dirs: list[Path],
|
||||
local_skills_dir: Path | None = None,
|
||||
) -> str | None:
|
||||
"""Return the UI category for a discovered skill path.
|
||||
|
||||
``skills_dirs[0]`` must be the active local skills root so flat local skills
|
||||
stay uncategorized while flat external roots can use their directory name.
|
||||
Flat skills directly under the active *local* skills root stay uncategorized,
|
||||
while flat skills under an *external* root use that root's directory name as
|
||||
their category. ``local_skills_dir`` identifies the local root explicitly; if
|
||||
omitted it falls back to ``skills_dirs[0]`` for backward compatibility, but
|
||||
callers should pass it directly because the local root can be filtered out of
|
||||
``skills_dirs`` (e.g. when it does not exist yet on a host with only external
|
||||
skills configured), which would otherwise misclassify the first external root
|
||||
as local.
|
||||
"""
|
||||
local_skills_dir = skills_dirs[0] if skills_dirs else None
|
||||
if local_skills_dir is None:
|
||||
local_skills_dir = skills_dirs[0] if skills_dirs else None
|
||||
for skills_dir in skills_dirs:
|
||||
try:
|
||||
rel_path = skill_md.relative_to(skills_dir)
|
||||
@@ -399,7 +410,9 @@ def _skills_list_from_dir(skills_dir: Path, category: str | None = None) -> dict
|
||||
{
|
||||
"name": name,
|
||||
"description": description,
|
||||
"category": _skill_category_from_path(skill_md, search_dirs),
|
||||
"category": _skill_category_from_path(
|
||||
skill_md, search_dirs, local_skills_dir=skills_dir
|
||||
),
|
||||
"disabled": name in disabled,
|
||||
}
|
||||
)
|
||||
|
||||
@@ -39,3 +39,29 @@ def test_external_skill_categories_keep_local_flat_and_label_external_roots(tmp_
|
||||
== external_flat_root.name
|
||||
)
|
||||
assert routes._skill_category_from_path(nested_external_skill, search_dirs) == "ops"
|
||||
|
||||
|
||||
def test_external_skill_category_when_local_dir_absent_from_search_dirs(tmp_path):
|
||||
"""When the local skills dir does not exist it is filtered out of the search
|
||||
list (``_active_skill_search_dirs`` keeps only existing dirs). Passing the
|
||||
local dir explicitly must keep its flat skills uncategorized AND still let a
|
||||
flat external root use its directory name — instead of misidentifying the
|
||||
first surviving (external) root as local."""
|
||||
from api import routes
|
||||
|
||||
local_root = tmp_path / "skills" # intentionally never created
|
||||
external_flat_root = tmp_path / "partner-skills"
|
||||
flat_external_skill = _write_skill(external_flat_root, "external-flat-skill")
|
||||
|
||||
# _active_skill_search_dirs would drop the missing local_root; simulate that.
|
||||
search_dirs = [external_flat_root]
|
||||
|
||||
# Without local_skills_dir, the old position-based logic would treat the
|
||||
# external root as local and return None. With it passed explicitly, the
|
||||
# external root is correctly labeled.
|
||||
assert (
|
||||
routes._skill_category_from_path(
|
||||
flat_external_skill, search_dirs, local_skills_dir=local_root
|
||||
)
|
||||
== external_flat_root.name
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user