fix(packaging): ship bundled skills in the wheel via setup.py data_files

pyproject.toml

@@ -301,28 +301,22 @@ hermes-acp = "acp_adapter.entry:main"
 [tool.setuptools]
 py-modules = ["run_agent", "model_tools", "toolsets", "batch_runner", "trajectory_compressor", "toolset_distributions", "cli", "hermes_bootstrap", "hermes_constants", "hermes_state", "hermes_time", "hermes_logging", "utils", "mcp_serve"]
 
-[tool.setuptools.data-files]
-# i18n catalogs. locales/ is a bare data directory (no __init__.py), so it is
-# neither a package (packages.find) nor package-data (which attaches to a
-# package). data-files ships it in the wheel; MANIFEST.in `graft locales`
-# ships it in the sdist. Without this, sealed installs (pip wheel, Nix store
-# venv) drop the catalogs and gateway/CLI commands surface raw i18n keys like
-# `gateway.reset.header_default` (#27632, #35374, #23943).
-locales = ["locales/*.yaml"]
-# Shipped MCP catalog (optional-mcps/<name>/manifest.yaml). Same bare-data-dir
-# case as locales: data-files ships it in the wheel, `graft optional-mcps` in
-# MANIFEST.in ships it in the sdist. Without this, `hermes mcp catalog` and the
-# dashboard catalog screen come up empty on packaged installs even though the
-# manifests exist in the repo (hermes_cli/mcp_catalog.py:_catalog_root resolves
-# the packaged dir; list_catalog() returns [] when it's missing).
+# NOTE: there is intentionally NO [tool.setuptools.data-files] table here.
 #
-# data-files flattens every glob match into its single target dir, so each
-# catalog entry needs its OWN target to preserve the per-entry directory the
-# catalog iterates over (a shared `optional-mcps/*/*` glob would collapse all
-# manifests into one colliding optional-mcps/manifest.yaml). One target per
-# entry; tests/test_packaging_metadata.py enforces an entry per optional-mcps/<name>.
-"optional-mcps/linear" = ["optional-mcps/linear/manifest.yaml"]
-"optional-mcps/n8n" = ["optional-mcps/n8n/manifest.yaml"]
+# Bundled non-package data dirs (skills/, optional-skills/, locales/,
+# optional-mcps/) are bare data directories — no __init__.py, so they are
+# neither packages (packages.find) nor package-data (which attaches to a
+# package). They ship in the wheel via setuptools `data_files`, generated in
+# setup.py:bundled_data_files(), and in the sdist via MANIFEST.in `graft`.
+#
+# data_files can be declared in EITHER this table OR setup.py, never merged:
+# whichever pyproject declares wins and silently drops setup.py's list. The
+# skills/ and optional-skills/ trees are too deep and change too often to
+# enumerate statically here (data_files also flattens each glob into one target
+# dir, so a nested tree needs one target per directory), so ALL four dirs are
+# generated dynamically in setup.py. Reintroducing this table would shadow
+# setup.py and the wheel would ship zero skills again — see
+# tests/test_packaging_metadata.py and CONTRIBUTING.md (#27632, #35374, #23943).
 
 [tool.setuptools.package-data]
 hermes_cli = ["web_dist/**/*", "tui_dist/**/*", "scripts/install.sh", "scripts/install.ps1"]

setup.py

@@ -12,6 +12,37 @@
 REPO_ROOT = Path(__file__).parent.resolve()
 
 
+# Directory names that never carry shippable bundled-data content: VCS,
+# virtualenv / dependency trees, byte-code and test caches, and the skills
+# ``index-cache`` (a runtime download cache, also filtered by
+# nix/hermes-agent.nix). Mirrors agent.skill_utils.EXCLUDED_SKILL_DIRS — kept
+# as a literal so setup.py stays import-free at build time — plus
+# ``index-cache``. NOTE: skill *support* dirs (references/templates/assets/
+# scripts) are deliberately NOT excluded; they are real skill payload that
+# tools/skills_sync.py copies into ~/.hermes/skills/ on seed.
+_EXCLUDED_DATA_DIRS = frozenset(
+    {
+        ".git",
+        ".github",
+        ".hub",
+        ".archive",
+        ".venv",
+        "venv",
+        "node_modules",
+        "site-packages",
+        "__pycache__",
+        ".tox",
+        ".nox",
+        ".pytest_cache",
+        ".mypy_cache",
+        ".ruff_cache",
+        "index-cache",
+    }
+)
+_EXCLUDED_DATA_SUFFIXES = frozenset({".pyc", ".pyo"})
+_EXCLUDED_DATA_NAMES = frozenset({".DS_Store", "Thumbs.db"})
+
+
 def _source_tree_is_writable() -> bool:
     probe = REPO_ROOT / ".setuptools-write-probe"
     try:
@@ -64,24 +95,67 @@ def finalize_options(self) -> None:
         super().finalize_options()
 
 
+def _is_shippable(rel_path: Path) -> bool:
+    """True when *rel_path* (relative to REPO_ROOT) is real bundled-data content."""
+    if _EXCLUDED_DATA_DIRS.intersection(rel_path.parts):
+        return False
+    if rel_path.suffix.lower() in _EXCLUDED_DATA_SUFFIXES:
+        return False
+    if rel_path.name in _EXCLUDED_DATA_NAMES:
+        return False
+    return True
+
+
 def _data_file_tree(root_name: str) -> list[tuple[str, list[str]]]:
+    """Map every file under *root_name* to a setuptools ``data_files`` entry.
+
+    Returns ``(target_dir, [source_files])`` tuples with the on-disk directory
+    layout preserved one-directory-per-target. setuptools ``data_files``
+    FLATTENS every glob match into its single target dir, so a nested tree must
+    be enumerated per directory or the structure collapses — skills are seeded
+    by category/skill path (tools/skills_sync.py), so a flattened
+    ``skills/*`` glob would corrupt the install. Paths are emitted as POSIX
+    (forward slash) so the wheel is correct even when built on Windows, where
+    ``str(Path(...))`` would otherwise bake in backslash targets.
+    """
     root = REPO_ROOT / root_name
+    if not root.is_dir():
+        return []
     grouped: defaultdict[str, list[str]] = defaultdict(list)
     for path in sorted(root.rglob("*")):
         if not path.is_file():
             continue
         rel_path = path.relative_to(REPO_ROOT)
-        grouped[str(rel_path.parent)].append(str(rel_path))
+        if not _is_shippable(rel_path):
+            continue
+        grouped[rel_path.parent.as_posix()].append(rel_path.as_posix())
     return sorted(grouped.items())
 
 
+def bundled_data_files() -> list[tuple[str, list[str]]]:
+    """All bundled non-package data shipped in the wheel's data scheme.
+
+    setuptools resolves ``data_files`` from EITHER pyproject.toml's
+    ``[tool.setuptools.data-files]`` table OR setup.py — never a merge of both.
+    Whichever the pyproject table declares wins and silently drops setup.py's
+    list (this clobbered the skills shipping for two releases). The skills and
+    optional-skills trees are too deep and change too often to enumerate
+    statically in TOML, so ALL bundled-data dirs are generated here in one
+    place. Do NOT reintroduce ``[tool.setuptools.data-files]`` in pyproject.toml
+    or it will shadow this and the wheel will ship zero skills again.
+    """
+    return [
+        *_data_file_tree("skills"),
+        *_data_file_tree("optional-skills"),
+        *_data_file_tree("locales"),
+        *_data_file_tree("optional-mcps"),
+    ]
+
+
 setup(
     cmdclass={
         "build": ReadOnlySourceBuild,
         "egg_info": ReadOnlySourceEggInfo,
     },
-    data_files=[
-        *_data_file_tree("skills"),
-        *_data_file_tree("optional-skills"),
-    ]
+    data_files=bundled_data_files(),
 )

tests/test_packaging_metadata.py

@@ -1,6 +1,9 @@
 from pathlib import Path
+import posixpath
 import re
+import runpy
 import tomllib
+import unittest.mock
 
 import pytest
 
@@ -15,6 +18,32 @@
 REPO_ROOT = Path(__file__).resolve().parents[1]
 
 
+def _setup_data_files() -> list[tuple[str, list[str]]]:
+    """Return the ``data_files`` list setup.py passes to ``setuptools.setup()``.
+
+    Runs setup.py via runpy with ``setuptools.setup`` patched to a capture shim
+    (the same technique as tests/test_docker_webui_install_surface.py), so the
+    exact wheel data-files the build backend receives are read without running a
+    real build. setup.py generates these dynamically — see
+    ``setup.py:bundled_data_files()``.
+    """
+    import setuptools
+
+    captured: dict = {}
+
+    def _capture(**kwargs):
+        captured.update(kwargs)
+
+    with unittest.mock.patch.object(setuptools, "setup", _capture):
+        runpy.run_path(str(REPO_ROOT / "setup.py"))
+    return captured.get("data_files", [])
+
+
+def _shipped_data_sources() -> set[str]:
+    """All POSIX source paths setup.py ships as wheel data-files."""
+    return {src for _, sources in _setup_data_files() for src in sources}
+
+
 def _distribution_name(requirement: str) -> str:
     """Extract the PEP 508 distribution name from a requirement string.
 
@@ -243,25 +272,122 @@ def test_locked_starlette_is_not_vulnerable_to_cve_2026_48710():
 def test_locale_catalogs_ship_in_both_wheel_and_sdist():
     """Regression test for #27632 / #35374 / #23943.
 
-    locales/ is a bare data directory (no __init__.py), so it is invisible to
-    packages.find and to package-data (which attaches to a package). It must be
-    declared as setuptools data-files (wheel) AND grafted in MANIFEST.in
-    (sdist). Without both, sealed installs drop the catalogs and gateway/CLI
-    commands surface raw i18n keys like `gateway.reset.header_default`.
+    locales/ is a bare data directory (no __init__.py), so it reaches installs
+    only as setuptools data-files (wheel) AND a MANIFEST.in graft (sdist). The
+    wheel data-files are generated in ``setup.py:bundled_data_files()`` — NOT a
+    ``[tool.setuptools.data-files]`` table in pyproject.toml, which would shadow
+    setup.py and drop the skills tree (see
+    ``test_pyproject_does_not_shadow_setup_py_data_files``). Without both
+    channels, sealed installs drop the catalogs and gateway/CLI commands surface
+    raw i18n keys like ``gateway.reset.header_default``.
     """
-    data = tomllib.loads((REPO_ROOT / "pyproject.toml").read_text(encoding="utf-8"))
-    data_files = data["tool"]["setuptools"].get("data-files", {})
-    assert data_files.get("locales") == ["locales/*.yaml"], (
-        "pyproject [tool.setuptools.data-files] must declare "
-        'locales = ["locales/*.yaml"] so the wheel ships i18n catalogs'
+    shipped = _shipped_data_sources()
+    on_disk = {
+        p.relative_to(REPO_ROOT).as_posix()
+        for p in (REPO_ROOT / "locales").glob("*.yaml")
+    }
+    assert on_disk, "expected locales/*.yaml catalogs on disk"
+    missing = sorted(on_disk - shipped)
+    assert not missing, (
+        f"locale catalogs dropped from wheel data-files: {missing} — "
+        "setup.py:bundled_data_files() must ship every locales/*.yaml"
     )
 
     manifest = (REPO_ROOT / "MANIFEST.in").read_text(encoding="utf-8")
     assert "graft locales" in manifest, (
         "MANIFEST.in must `graft locales` so the sdist ships i18n catalogs"
     )
 
-    # Every on-disk catalog has the .yaml extension the globs above match.
-    on_disk = list((REPO_ROOT / "locales").glob("*.yaml"))
-    assert on_disk, "expected locales/*.yaml catalogs on disk"
+
+def test_bundled_skills_ship_in_wheel_data_files():
+    """Regression test: the wheel must ship bundled skills/ and optional-skills/.
+
+    Both are bare data dirs (no __init__.py), so they reach the wheel only via
+    setuptools ``data_files``. setup.py generates that list dynamically, but a
+    ``[tool.setuptools.data-files]`` table in pyproject.toml silently OVERRIDES
+    setup.py's ``data_files`` — which shipped a wheel with ZERO bundled skills
+    for two releases. On a sealed/wheel install ``_get_bundled_dir()`` then
+    resolves an absent dir and ``sync_skills()`` returns ``total_bundled: 0``,
+    so nothing is seeded into ~/.hermes/skills/.
+
+    This drives setup.py's own generator and asserts:
+      - every on-disk SKILL.md is shipped (completeness),
+      - the nested category/skill layout is preserved (data_files flattens each
+        glob into one target dir, so a collapsed tree would corrupt seeding),
+      - runtime cruft (index-cache, __pycache__, *.pyc) is not shipped.
+    """
+    entries = _setup_data_files()
+
+    shipped: set[str] = set()
+    for target, sources in entries:
+        for src in sources:
+            assert posixpath.dirname(src) == target, (
+                f"data_files flatten hazard: {src!r} is not directly under its "
+                f"declared target {target!r}; setuptools would collapse the tree"
+            )
+            shipped.add(src)
+
+    for root_name in ("skills", "optional-skills"):
+        on_disk = {
+            p.relative_to(REPO_ROOT).as_posix()
+            for p in (REPO_ROOT / root_name).rglob("SKILL.md")
+        }
+        assert on_disk, f"expected SKILL.md files under {root_name}/ on disk"
+        missing = sorted(on_disk - shipped)
+        assert not missing, (
+            f"{root_name}/ skills dropped from wheel data-files: {missing}"
+        )
+
+    cruft = sorted(
+        s
+        for s in shipped
+        if "index-cache" in s.split("/")
+        or "__pycache__" in s.split("/")
+        or s.endswith((".pyc", ".pyo"))
+    )
+    assert not cruft, f"runtime cruft leaked into wheel data-files: {cruft}"
+
+
+def test_optional_mcps_manifests_ship_in_wheel():
+    """Every optional-mcps/<name>/manifest.yaml must ship in the wheel and sdist.
+
+    The catalog reader (hermes_cli/mcp_catalog.py) lists whatever manifests are
+    present in the packaged optional-mcps dir, so a dropped entry silently
+    removes a server from ``hermes mcp catalog``. Generating data-files from the
+    tree (vs a hand-maintained pyproject list) keeps this from drifting — the
+    earlier static list shipped linear + n8n but missed unreal-engine.
+    """
+    shipped = _shipped_data_sources()
+    on_disk = {
+        p.relative_to(REPO_ROOT).as_posix()
+        for p in (REPO_ROOT / "optional-mcps").rglob("manifest.yaml")
+    }
+    assert on_disk, "expected optional-mcps/<name>/manifest.yaml on disk"
+    missing = sorted(on_disk - shipped)
+    assert not missing, (
+        f"optional-mcps manifests dropped from wheel data-files: {missing}"
+    )
+
+    manifest = (REPO_ROOT / "MANIFEST.in").read_text(encoding="utf-8")
+    assert "graft optional-mcps" in manifest, (
+        "MANIFEST.in must `graft optional-mcps` so the sdist ships the catalog"
+    )
+
+
+def test_pyproject_does_not_shadow_setup_py_data_files():
+    """pyproject.toml must NOT declare ``[tool.setuptools.data-files]``.
+
+    Bundled data dirs are generated dynamically in
+    ``setup.py:bundled_data_files()`` because skills/ and optional-skills/ are
+    too deep to enumerate statically and data_files flattens nested globs.
+    setuptools resolves ``data_files`` from EITHER pyproject OR setup.py, never
+    both — a pyproject table wins and silently drops setup.py's list, which
+    shipped a wheel with zero bundled skills. Keep the table out of pyproject so
+    setup.py stays the single source of truth.
+    """
+    data = tomllib.loads((REPO_ROOT / "pyproject.toml").read_text(encoding="utf-8"))
+    assert "data-files" not in data["tool"]["setuptools"], (
+        "Remove [tool.setuptools.data-files] from pyproject.toml — it shadows "
+        "setup.py:bundled_data_files() and drops skills/ from the wheel."
+    )