diff --git a/api/routes.py b/api/routes.py index 920a3663..40686135 100644 --- a/api/routes.py +++ b/api/routes.py @@ -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, } diff --git a/tests/test_issue7426_skill_not_found_listing_truncation.py b/tests/test_issue7426_skill_not_found_listing_truncation.py new file mode 100644 index 00000000..507bc993 --- /dev/null +++ b/tests/test_issue7426_skill_not_found_listing_truncation.py @@ -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)