[napetrov]

Summary

discover_skills walked every directory under the catalog root, including .git, node_modules, site-packages and virtualenvs. A dependency's copy of a skill there loaded as a skill of the catalog. If the copy differed by even one byte from the real skill, the mirror rule did not drop it, so the skill-catalog: arm offered two rival skills with the same name.

 dokimi/skills/loader.py   + VENDORED_DIR_NAMES; os.walk prunes them (not walked, no note)

Found while working on #211 (trigger-set formats).

Evidence

• Before: a root with skills/linux-perf and node_modules/pkg/skills/linux-perf (a different version) gave two linux-perf skills.
  After: it gives one. The new cases are in tests/test_skill_loader.py: node_modules, .git/modules, and .venv/.../site-packages are skipped, and a root that itself sits inside node_modules is still walked.
• The full suite passes locally (7786 passed), along with ruff and mypy.

Merge Danger

Door: two-way

Blast Radius: small. A catalog that keeps real skills under a directory named venv, .tox, site-packages or one of the others no longer loads them. These are directory names that would be odd for a skills tree.

🤖 Generated with Claude Code

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.75%. Comparing base (b226dba) to head (37efd72).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
- Coverage   92.76%   92.75%   -0.01%     
==========================================
  Files         256      256              
  Lines       32414    32423       +9     
  Branches     5421     5423       +2     
==========================================
+ Hits        30068    30075       +7     
- Misses       1561     1562       +1     
- Partials      785      786       +1     

Files with missing lines	Coverage Δ
dokimi/dokimi/commands/novelty.py	96.28% <ø> (ø)
dokimi/dokimi/external/catalog.py	97.18% <100.00%> (ø)
dokimi/dokimi/skills/formats.py	93.19% <100.00%> (ø)
dokimi/dokimi/skills/loader.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[antonwolfy]

The fix is right: os.walk with in-place pruning matches the old rglob for everything except the pruned names, and both the skill-catalog: arm and the bare catalog scan go through it. One thing to fix before merge, the tests do not pin most of the names; three nits.

What I ran, on a Linux review host at 3f67aea:

• make ci-local: 29 passed, 0 failed, 7 skipped; changed-line coverage passes.
• Old rglob against new walk_skill_files on a tree with symlinked directories, a symlink loop, a symlinked SKILL.md, hidden directories and relative roots: same results and same order, except that a directory named SKILL.md is no longer returned.
• Mutation run on VENDORED_DIR_NAMES against test_skill_loader.py and test_external_catalog.py: see the inline comment.

Nit: the docs that describe discovery do not mention the new skip. dokimi/docs/README.md:40, dokimi/docs/evaluating-treatments.md:33 and :290-295, dokimi/docs/external-content.md:52-54 and the --catalog help in dokimi/dokimi/commands/novelty.py:639-643 list the template and mirror exclusions only. Because the skip leaves no note, a user cannot find out from the output either. Suggestion: add "vendored and VCS directories (.git, node_modules, site-packages, virtualenvs) are not walked" next to the template rule in those places.

Checked, no issues

• Ordering and symlinks: sorted Path order is kept; symlinked directories are not followed, as before.
• Callers: walk_skill_files is the only recursive SKILL.md lookup; the other glob calls are non-recursive.
• Root inside a vendored directory: still walked, and tested.
• Changelog format: one ### Fixed entry with a bold lead-in, as changelog.d/README.md asks.

[antonwolfy]

I pushed a commit per nit, up to 6248970, which fix the two nits from my review:

• 0a96697: dokimi/changelog.d/20261024_130000_napetrov_discover-skills-skip-vendored.md:3: the entry now names the skill-catalog: arm and the catalog commands, not the skill: arm.
• 6248970: dokimi/docs/README.md:40, evaluating-treatments.md:33 and :292, external-content.md:53 and the --catalog help in commands/novelty.py: each now says vendored and VCS directories are not walked.

Left as is:

• dokimi/dokimi/skills/loader.py:27: both suggested fixes change discovery behaviour (a new note, or a smaller pruned set), so the choice stays with the author.

make ci-local at 6248970: 29 passed, 0 failed, 7 skipped.

Not approving yet. Still open: the risk from my review.

[napetrov]

Thanks for the review. Open items, mapped to commits on the branch:

• Test pinning of VENDORED_DIR_NAMES: 56366a0, test_discovery_skips_each_vendored_name (all ten names).
• catalog formats also listed a vendored copy as unloadable: 56366a0, test_a_vendored_skill_is_not_listed_as_a_package.
• Docs and changelog nits: 6248970 and 0a96697 (your commits), both on the branch.

The branch is two commits behind intel/main (one conflict in skills/loader.py with #241); the merge is not in yet. Ready for another look.

[antonwolfy]

Review at 56366a0. The test gap from my first review is closed: the ten names are pinned one by one, and catalog formats has its own case. One doc nit is new, and one earlier nit is still open.

What I ran, on a Linux review host at 56366a0:

• make ci-local: 29 passed, 0 failed, 7 skipped; changed-line coverage passes.
• Mutation runs against the three touched test files: pruning only at the root fails 10 tests; dropping site-packages and venv from the set fails 3; formats.py back to its old skip set fails 1; external/catalog.py back to rglob fails 2.

Nit: three docs still describe --catalog without the vendored-directory rule. The PR rewrote the --catalog help in novelty.py and four docs, but these pages carry the old wording "a template/ copy and a byte-identical mirror ... are left out" (all outside this diff):

• README.md:168-169
• dokimi/docs/skill-novelty-walkthrough.md:44
• dokimi/docs/skill-novelty.md:604

A reader of these pages assumes a node_modules or .venv copy is still a competitor, while the help text says it is not. scripts/check_docs_review_triggers.py warns that novelty.py changed and the last two pages were not updated. Suggestion: add "vendored and VCS directories (.git, node_modules, site-packages, virtualenvs) are not walked" to the same sentence in each.

Still open from my earlier review:

• dokimi/dokimi/skills/loader.py:27: a real skill under a directory named venv, .venv, .tox, .nox or site-packages is still dropped with no note. With skills/venv/SKILL.md, skills/site-packages/SKILL.md and skills/.nox/SKILL.md, discover_skills returns neither, and notes stays empty. Both suggested fixes still stand: a note per pruned directory that holds a SKILL.md, or a smaller set.

Checked, no issues

• Ordering and symlinks: walk_skill_files sorts the Path list like the old rglob; symlinked directories are not followed, and relative roots give the same paths.
• Callers: no other recursive SKILL.md lookup remains; catalog.py, formats.py and the loader all use the shared set.
• Root inside a vendored directory: still walked, and tested.
• Changelog: one ### Fixed entry with a bold lead-in.

[antonwolfy]

I pushed a commit per nit, up to 201bb90, which fix the nit from my review:

• 201bb90: README.md:168, dokimi/docs/skill-novelty-walkthrough.md:44 and dokimi/docs/skill-novelty.md:604: the --catalog sentence now says vendored and VCS directories (.git, node_modules, site-packages, virtualenvs) are not walked.

Left as is:

• dokimi/dokimi/skills/loader.py:27: both suggested fixes change what discover_skills returns or reports, so the choice between a note per pruned directory and a smaller set is left to the author.

make ci-local at 201bb90: 29 passed, 0 failed, 7 skipped.

GitHub CI has not run on these commits: the branch conflicts with main, and Actions runs no workflow on a conflicting PR. CI will check them once main is merged in.

Not approving until CI passes at this head.

[napetrov]

Merged intel/main in 085c7e3. That covers the merge my last comment said was still missing. The skills/loader.py conflict with #241 is resolved, and its is_file() guard now sits in walk_skill_files, with a test for a dangling symlink. The last open nit, no note for a pruned venv directory (loader.py:27), is declined, with the reasons in its thread. Local make ci-local: every test passes apart from two failures this host causes. One is test_probe_reports_a_writable_docker_socket, where the AF_UNIX path is too long under the long scratch path. The other is bandit, which scans the local .venv.

[xaleryb]

• Ran: full suite, ruff, mypy and bandit on the head merged with current main; the new tests against the pre-fix source (all red); mutants over the new pruning code; and an old-vs-new comparison of discover_skills, catalog scan and catalog formats on vendored, symlinked and nested trees.
• Only the announced changes appear: copies under vendored or VCS directories are dropped, and skill-catalog: no longer fails with duplicate tool names when a skill also ships inside node_modules.
• The open thread about real skills under venv/.venv/.tox/.nox was answered by design, and the docstring and docs state that no note is emitted. That is the author's call.
• Merged with fix(cli): tools preference and catalog exit with a message on bad input or output #239, catalog scan stops reporting a SKILL.md that is a directory or a dangling symlink: walk_skill_files keeps only regular files, so fix(cli): tools preference and catalog exit with a message on bad input or output #239's new test_catalog_scan_reports_a_malformed_skill_as_an_error_row[directory] gets no error row and exit 0. Commit we will push: a987465 returns every SKILL.md entry from the walker and keeps the is_file guard in skill_files, so catalog scan reports the broken skill again while discover_skills still leaves it out.

[antonwolfy]

LGTM. The walker keeps the old rglob results and order apart from the pruned names, and a987465 puts the is_file guard in skill_files so catalog scan reports a broken SKILL.md again while discover_skills leaves it out.

What I ran, on Linux at a987465:

• make ci-local: 26 passed, 7 skipped, 2 failed. The two failures are the gitleaks download (the proxy returned 504) and the commit scan that needs it, not this PR. Tests: 8,999 passed; changed-line coverage passes.
• Old rglob against the new walker on trees with symlinked files, dangling links, directories named SKILL.md and nested vendored directories: same results and order, except for the pruned names.
• 20 mutants over the four touched test files (directory entry dropped, is_file guard dropped, root-only pruning, in-place prune removed, unsorted walk, rglob back in catalog.py and formats.py, each of the ten names removed): 19 killed. The survivor removes the template/ skip in _scan_skill_files, which no test covers; that predates the PR.

Checked, no issues

• Callers: walk_skill_files is used only by skill_files (guarded) and _scan_skill_files; formats.py now shares VENDORED_DIR_NAMES.
• Docs and help: the ten names in evaluating-treatments.md and the changelog match the code; the --catalog help matches.