fix(skills): report the total when skill-not-found truncates available_skills (#7426)
The not-found reply capped available_skills at 20 with no total and no truncation marker, so a caller with more skills than the cap was handed a partial list that reads as the complete set. A skill sorting past the cut looked uninstalled. Keep the bound; stop it lying. The payload now carries total_skills and available_skills_truncated, and the hint names how many of how many were shown, matching the (list, truncated) shape already used for capped commit subject lists in api/updates.py.
This commit is contained in:
1 parent
e168b67e42
commit
d7ed506d21
2 files changed
+87
-2
No files matched your search
+17
-2
@@ -1007,13 +1007,28 @@ def _find_skill_in_dir(name: str, skills_dir: Path) -> tuple[Path | None, Path |
|
||||
return _find_skill_in_dirs(name, [skills_dir])
|
||||
|
||||
|
||||
# Cap on the courtesy list of names carried by a skill-not-found reply. The
|
||||
# bound stays; what it must never do is present a partial list as the whole
|
||||
# set, because a caller that cannot find its skill in `available_skills` will
|
||||
# conclude the skill is not installed.
|
||||
_SKILL_NOT_FOUND_LIST_LIMIT = 20
|
||||
|
||||
|
||||
def _skill_not_found_payload(name: str, skills_dir: Path) -> dict:
|
||||
available = [s["name"] for s in _skills_list_from_dir(skills_dir).get("skills", [])[:20]]
|
||||
all_names = [s["name"] for s in _skills_list_from_dir(skills_dir).get("skills", [])]
|
||||
total = len(all_names)
|
||||
available = all_names[:_SKILL_NOT_FOUND_LIST_LIMIT]
|
||||
truncated = total > len(available)
|
||||
hint = "Use skills_list to see all available skills"
|
||||
if truncated:
|
||||
hint = f"Showing {len(available)} of {total} skills. {hint}"
|
||||
return {
|
||||
"success": False,
|
||||
"error": f"Skill '{name}' not found.",
|
||||
"available_skills": available,
|
||||
"hint": "Use skills_list to see all available skills",
|
||||
"available_skills_truncated": truncated,
|
||||
"total_skills": total,
|
||||
"hint": hint,
|
||||
}
|
||||
|
||||
|
||||
|
||||
@@ -0,0 +1,70 @@
|
||||
"""#7426: the skill-not-found reply must not present a capped list as the whole set.
|
||||
|
||||
`_skill_not_found_payload` caps `available_skills` so a large skills tree cannot
|
||||
bloat an error payload. The bug was that the cap was silent: with 74 skills
|
||||
installed, a caller asking for one that sorts past the 20th got "not found" plus
|
||||
a 20-name list that did not contain it and did not say anything was omitted, so
|
||||
the only reasonable reading was that the skill is not installed.
|
||||
|
||||
These tests pin the honesty of the payload, not the size of the cap.
|
||||
"""
|
||||
|
||||
import pathlib
|
||||
|
||||
import api.routes as routes
|
||||
|
||||
|
||||
def _stub_listing(monkeypatch, count):
|
||||
names = [f"skill-{i:03d}" for i in range(count)]
|
||||
monkeypatch.setattr(
|
||||
routes,
|
||||
"_skills_list_from_dir",
|
||||
lambda *args, **kwargs: {
|
||||
"success": True,
|
||||
"skills": [{"name": n} for n in names],
|
||||
"count": len(names),
|
||||
},
|
||||
)
|
||||
return names
|
||||
|
||||
|
||||
def test_truncated_listing_reports_the_total_and_says_it_is_truncated(monkeypatch):
|
||||
names = _stub_listing(monkeypatch, 74)
|
||||
payload = routes._skill_not_found_payload("skill-073", pathlib.Path("/nonexistent"))
|
||||
|
||||
assert payload["success"] is False
|
||||
# The cap still applies: the payload does not grow without bound.
|
||||
assert len(payload["available_skills"]) < len(names)
|
||||
# But a caller can now tell a partial list from a complete one.
|
||||
assert payload["available_skills_truncated"] is True
|
||||
assert payload["total_skills"] == 74
|
||||
assert "74" in payload["hint"]
|
||||
assert str(len(payload["available_skills"])) in payload["hint"]
|
||||
|
||||
|
||||
def test_untruncated_listing_is_not_flagged_as_truncated(monkeypatch):
|
||||
names = _stub_listing(monkeypatch, 3)
|
||||
payload = routes._skill_not_found_payload("nope", pathlib.Path("/nonexistent"))
|
||||
|
||||
assert payload["available_skills"] == names
|
||||
assert payload["available_skills_truncated"] is False
|
||||
assert payload["total_skills"] == 3
|
||||
assert payload["hint"] == "Use skills_list to see all available skills"
|
||||
|
||||
|
||||
def test_empty_skills_tree_reports_zero_and_no_truncation(monkeypatch):
|
||||
_stub_listing(monkeypatch, 0)
|
||||
payload = routes._skill_not_found_payload("nope", pathlib.Path("/nonexistent"))
|
||||
|
||||
assert payload["available_skills"] == []
|
||||
assert payload["available_skills_truncated"] is False
|
||||
assert payload["total_skills"] == 0
|
||||
|
||||
|
||||
def test_exactly_at_the_cap_is_not_flagged_as_truncated(monkeypatch):
|
||||
names = _stub_listing(monkeypatch, routes._SKILL_NOT_FOUND_LIST_LIMIT)
|
||||
payload = routes._skill_not_found_payload("nope", pathlib.Path("/nonexistent"))
|
||||
|
||||
assert payload["available_skills"] == names
|
||||
assert payload["available_skills_truncated"] is False
|
||||
assert payload["total_skills"] == len(names)
|
||||
Reference in new issue
Block a user