Skip to content

fix(skills): ignore internal asset markdown during skill lookup - #37164

Closed
westkite1201 wants to merge 1 commit into
NousResearch:mainfrom
westkite1201:fix/skill-view-ignore-internal-assets
Closed

fix(skills): ignore internal asset markdown during skill lookup#37164
westkite1201 wants to merge 1 commit into
NousResearch:mainfrom
westkite1201:fix/skill-view-ignore-internal-assets

Conversation

@westkite1201

Copy link
Copy Markdown
Contributor

Summary

Testing

  • uv run --with pytest-timeout pytest tests/tools/test_skills_tool.py -q
  • uv run ruff check tools/skills_tool.py tests/tools/test_skills_tool.py
  • python3 -m py_compile tools/skills_tool.py tests/tools/test_skills_tool.py && git diff --check
  • python3 scripts/check-windows-footguns.py --diff origin/main

Notes

  • Full repository test suite not run locally.

Legacy flat skill lookup scanned every matching markdown file under the skills root, including templates and references owned by another skill. That made valid skills such as productivity/notion look ambiguous when another skill shipped an internal notion.md asset. This keeps legacy flat lookup but skips markdown files whose ancestors already define a skill.

Constraint: Preserve collision detection for real same-name skills across local and external skill dirs

Constraint: Preserve legacy top-level <name>.md skill loading

Rejected: Remove recursive legacy flat lookup entirely | broader compatibility break for old-style skills

Confidence: high

Scope-risk: narrow

Tested: uv run --with pytest-timeout pytest tests/tools/test_skills_tool.py -q

Tested: uv run ruff check tools/skills_tool.py tests/tools/test_skills_tool.py

Tested: python3 -m py_compile tools/skills_tool.py tests/tools/test_skills_tool.py && git diff --check

Tested: python3 scripts/check-windows-footguns.py --diff origin/main

Not-tested: Full repository test suite
@liuhao1024

Copy link
Copy Markdown
Contributor

Verified this fix is correct.

The bug: Strategy 3 in _find_skill_markdown (skills_tool.py:963) uses search_dir.rglob(f"{name}.md") to find legacy flat skill files. When a skill like popular-web-designs contains an asset file templates/notion.md, the rglob matches it as a "notion" skill candidate, causing a collision with the real notion skill's SKILL.md.

Fix validation: The new _is_inside_skill_dir helper correctly walks up from the found .md file toward the search root, checking for SKILL.md at each ancestor. If a SKILL.md is found before reaching the root, the file is inside a skill directory and is skipped. The ancestor == root check terminates the walk correctly — top-level .md files directly under the search root are not rejected.

Edge case: The function returns False when the walk reaches root without finding SKILL.md, which correctly preserves the legacy flat-file behavior for files at the top level of a skills directory.

@alt-glitch alt-glitch added type/bug Something isn't working tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels Jun 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Same root cause as #35768 and #36895 — all three fix #35743 (skill lookup treating nested markdown assets inside other skill dirs as standalone legacy skills, causing false collisions). Consolidate to one.

@westkite1201

Copy link
Copy Markdown
Contributor Author

Thanks for pointing this out. I missed #35768/#36895 during the duplicate check. Since #35768 predates this PR and covers the same root cause, I'll close this one to avoid splitting review. Appreciate the pointer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: skill_view falsely reports collision when another skill has an asset file with the same name

3 participants