From ae8658affa8505c3360e206395f46f8cb6bfcb8d Mon Sep 17 00:00:00 2001 From: Frank Song Date: Thu, 14 May 2026 22:28:20 +0800 Subject: [PATCH] Fix blank skill detail views --- CHANGELOG.md | 2 + api/routes.py | 142 ++++++++++++++---- tests/test_issue1880_profile_scoped_skills.py | 28 ++++ 3 files changed, 139 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6df1a29..1f91d498 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ### Fixed +- Skills panel detail views now render the selected `SKILL.md` content again. The WebUI no longer passes profile-resolved absolute skill paths into Hermes Agent `skill_view()`, which rejects non-relative patterns on newer Agent versions and previously left the detail pane blank. + - **PR #2234** by @Jordan-SkyLF (refines #2207) — Three update-banner improvements: (1) Update summaries no longer repeat the same bullet under both "What you'll notice" and "Worth knowing" — visible notice items keep priority, and the secondary section is omitted when there is no distinct detail to show. (2) Update summaries now cap large commit lists (24 + probe item) before sending them to the summarizer and disclose when the summary uses only the latest commit subjects, while keeping the full comparison link available — bounds summarizer cost on large update ranges while remaining honest about coverage. (3) Update banners now wrap generated-summary links and long update text on narrow/mobile screens inside the banner instead of pushing controls off-screen. 108-line regression coverage for short-target dedup, repeated Agent-summary bullets, large-range capped input, and responsive wrapping. - **PR #2236** by @jasonjcwu — Silent failure detection in `api/streaming.py` now scans only NEW messages, not the full conversation history. Pre-fix, the `_assistant_added` check at `_run_agent_streaming` scanned all messages in `result["messages"]` (including pre-turn history); if any prior turn contained an assistant response, `_assistant_added` was `True` and the apperror SSE event was silently skipped, leaving the user staring at a blank response after a provider 401/429/rate-limit error. Fix extracts a `_has_new_assistant_reply(all_messages, prev_count)` helper that only inspects messages beyond the pre-turn history offset (`_previous_context_messages`); applied to both the main detection path and the self-heal/retry `_heal_ok` check. 15-test regression suite covering empty/short/long-history scenarios, the heal path, and the `len < prev_count` edge-case fallback. Also includes a small alignment fix to `test_issue1857_usage_overwrite.py` so the FakeAgent message shape matches what the real agent produces. diff --git a/api/routes.py b/api/routes.py index dadc17f4..012124ac 100644 --- a/api/routes.py +++ b/api/routes.py @@ -269,13 +269,13 @@ def _skills_list_from_dir(skills_dir: Path, category: str | None = None) -> dict return result -def _find_skill_in_dir(name: str, skills_dir: Path) -> tuple[Path | None, Path | None]: - """Resolve a WebUI skill name inside an explicit skills directory.""" +def _find_skill_in_dirs(name: str, skills_dirs: list[Path]) -> tuple[Path | None, Path | None]: + """Resolve a WebUI skill name inside explicit skills directories.""" from agent.skill_utils import iter_skill_index_files from tools.skills_tool import _EXCLUDED_SKILL_DIRS, _parse_frontmatter raw_name = str(name or "").strip().strip("/") - if not raw_name or not skills_dir.exists(): + if not raw_name: return None, None candidate_names = [raw_name] @@ -284,37 +284,45 @@ def _find_skill_in_dir(name: str, skills_dir: Path) -> tuple[Path | None, Path | if namespace and bare: candidate_names.append(f"{namespace}/{bare}") - for candidate_name in candidate_names: - direct_path = skills_dir / candidate_name - if not _skill_path_within(skills_dir, direct_path): + for skills_dir in skills_dirs: + if not skills_dir.exists(): continue - if direct_path.is_dir() and (direct_path / "SKILL.md").exists(): - return direct_path, direct_path / "SKILL.md" - legacy_md = direct_path.with_suffix(".md") - if legacy_md.exists() and _skill_path_within(skills_dir, legacy_md): - return legacy_md.parent, legacy_md + for candidate_name in candidate_names: + direct_path = skills_dir / candidate_name + if not _skill_path_within(skills_dir, direct_path): + continue + if direct_path.is_dir() and (direct_path / "SKILL.md").exists(): + return direct_path, direct_path / "SKILL.md" + legacy_md = direct_path.with_suffix(".md") + if legacy_md.exists() and _skill_path_within(skills_dir, legacy_md): + return legacy_md.parent, legacy_md - for skill_md in iter_skill_index_files(skills_dir, "SKILL.md"): - if any(part in _EXCLUDED_SKILL_DIRS for part in skill_md.parts): - continue - skill_dir = skill_md.parent - if skill_dir.name == raw_name: - return skill_dir, skill_md - try: - frontmatter, _ = _parse_frontmatter(skill_md.read_text(encoding="utf-8")[:4000]) - if frontmatter.get("name") == raw_name: + for skill_md in iter_skill_index_files(skills_dir, "SKILL.md"): + if any(part in _EXCLUDED_SKILL_DIRS for part in skill_md.parts): + continue + skill_dir = skill_md.parent + if skill_dir.name == raw_name: return skill_dir, skill_md - except Exception: - continue + try: + frontmatter, _ = _parse_frontmatter(skill_md.read_text(encoding="utf-8")[:4000]) + if frontmatter.get("name") == raw_name: + return skill_dir, skill_md + except Exception: + continue - for legacy_md in skills_dir.rglob("*.md"): - if legacy_md.name == "SKILL.md": - continue - if legacy_md.stem == raw_name and _skill_path_within(skills_dir, legacy_md): - return legacy_md.parent, legacy_md + for legacy_md in skills_dir.rglob("*.md"): + if legacy_md.name == "SKILL.md": + continue + if legacy_md.stem == raw_name and _skill_path_within(skills_dir, legacy_md): + return legacy_md.parent, legacy_md return None, None +def _find_skill_in_dir(name: str, skills_dir: Path) -> tuple[Path | None, Path | None]: + """Resolve a WebUI skill name inside an explicit skills directory.""" + return _find_skill_in_dirs(name, [skills_dir]) + + 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]] return { @@ -325,11 +333,80 @@ def _skill_not_found_payload(name: str, skills_dir: Path) -> dict: } +def _linked_files_for_skill(skill_dir: Path | None) -> dict: + if not skill_dir or not (skill_dir / "SKILL.md").exists(): + return {} + linked_files: dict[str, list[str]] = {} + + references_dir = skill_dir / "references" + if references_dir.exists(): + refs = [str(f.relative_to(skill_dir)) for f in references_dir.glob("*.md")] + if refs: + linked_files["references"] = sorted(refs) + + templates_dir = skill_dir / "templates" + if templates_dir.exists(): + templates = [] + for ext in ["*.md", "*.py", "*.yaml", "*.yml", "*.json", "*.tex", "*.sh"]: + templates.extend(str(f.relative_to(skill_dir)) for f in templates_dir.rglob(ext)) + if templates: + linked_files["templates"] = sorted(set(templates)) + + assets_dir = skill_dir / "assets" + if assets_dir.exists(): + assets = [str(f.relative_to(skill_dir)) for f in assets_dir.rglob("*") if f.is_file()] + if assets: + linked_files["assets"] = sorted(assets) + + scripts_dir = skill_dir / "scripts" + if scripts_dir.exists(): + scripts = [] + for ext in ["*.py", "*.sh", "*.bash", "*.js", "*.ts", "*.rb"]: + scripts.extend(str(f.relative_to(skill_dir)) for f in scripts_dir.glob(ext)) + if scripts: + linked_files["scripts"] = sorted(set(scripts)) + + return linked_files + + +def _skill_view_from_file(skill_dir: Path | None, skill_md: Path) -> dict: + from tools.skills_tool import _parse_frontmatter, _parse_tags, skill_matches_platform + + content = skill_md.read_text(encoding="utf-8") + frontmatter, _body = _parse_frontmatter(content) + if not skill_matches_platform(frontmatter): + return {"success": False, "error": "Skill is not available on this platform."} + + metadata = frontmatter.get("metadata") + hermes_meta = metadata.get("hermes", {}) if isinstance(metadata, dict) else {} + tags = _parse_tags(hermes_meta.get("tags") or frontmatter.get("tags", "")) + related_skills = _parse_tags( + hermes_meta.get("related_skills") or frontmatter.get("related_skills", "") + ) + try: + path = str(skill_md.relative_to((skill_dir or skill_md.parent).parent)) + except ValueError: + path = str(skill_md) + + return { + "success": True, + "name": frontmatter.get("name", skill_md.stem if not skill_dir else skill_dir.name), + "description": frontmatter.get("description", ""), + "tags": tags, + "related_skills": related_skills, + "content": content, + "path": path, + "skill_dir": str(skill_dir) if skill_dir else None, + "linked_files": _linked_files_for_skill(skill_dir), + } + + def _skill_view_from_active_dir(name: str) -> dict: from tools.skills_tool import skill_view as _skill_view skills_dir = _active_skills_dir() - skill_dir, skill_md = _find_skill_in_dir(name, skills_dir) + search_dirs = _active_skill_search_dirs(skills_dir) + skill_dir, skill_md = _find_skill_in_dirs(name, search_dirs) if not skill_md: # Preserve plugin-qualified skill viewing without falling back to the # startup/root profile's local skills tree for ordinary missing skills. @@ -348,10 +425,7 @@ def _skill_view_from_active_dir(name: str) -> dict: except Exception: pass return _skill_not_found_payload(name, skills_dir) - target_name = str(skill_dir) if skill_dir and (skill_dir / "SKILL.md") == skill_md else str(skill_md) - raw = _skill_view(target_name) - data = json.loads(raw) if isinstance(raw, str) else raw - return data + return _skill_view_from_file(skill_dir, skill_md) # ── SSE app-level heartbeat (#1623) ──────────────────────────────────────── # @@ -3890,7 +3964,9 @@ def handle_get(handler, parsed) -> bool: if _re.search(r"[*?\[\]]", name): return bad(handler, "Invalid skill name", 400) skills_dir = _active_skills_dir() - skill_dir, _skill_md = _find_skill_in_dir(name, skills_dir) + skill_dir, _skill_md = _find_skill_in_dirs( + name, _active_skill_search_dirs(skills_dir) + ) if not skill_dir: return bad(handler, "Skill not found", 404) target = (skill_dir / file_path).resolve() diff --git a/tests/test_issue1880_profile_scoped_skills.py b/tests/test_issue1880_profile_scoped_skills.py index b368f892..be46af95 100644 --- a/tests/test_issue1880_profile_scoped_skills.py +++ b/tests/test_issue1880_profile_scoped_skills.py @@ -138,6 +138,34 @@ def test_api_skills_list_and_content_respect_profile_cookie(): assert linked.get("content") == "linked file for profile-only-skill-1880\n" +def test_skill_detail_reads_resolved_file_without_skill_view_absolute_path(monkeypatch): + profile = "skills1880direct" + with _IsolatedSkillsDirs(profile) as dirs: + _write_skill( + dirs.profile_skills, + "profile-direct-skill-1880", + "Direct profile skill", + "This skill should be read from the resolved SKILL.md file.", + ) + + from api import routes + import tools.skills_tool as skills_tool + + monkeypatch.setattr(routes, "_active_skills_dir", lambda: dirs.profile_skills) + + def fail_if_skill_view_called(*_args, **_kwargs): + raise AssertionError("WebUI local skill details must not call skill_view()") + + monkeypatch.setattr(skills_tool, "skill_view", fail_if_skill_view_called) + + detail = routes._skill_view_from_active_dir("profile-direct-skill-1880") + + assert detail["success"] is True + assert detail["name"] == "profile-direct-skill-1880" + assert "resolved SKILL.md file" in detail["content"] + assert isinstance(detail["linked_files"], dict) + + def test_skill_save_and_delete_respect_profile_cookie(): profile = "skills1880save" with _IsolatedSkillsDirs(profile) as dirs: