refactor(skills): bundle search/analytics/geospatial under hotdata + fix CLI drift - #244
refactor(skills): bundle search/analytics/geospatial under hotdata + fix CLI drift#244eddietejeda wants to merge 3 commits into
Conversation
…fix CLI drift Only the top-level `hotdata` skill should appear in the agent skill list (autocomplete). The specialized guides — search, analytics, geospatial — are now bundled inside it under `skills/hotdata/subskills/<name>/` and loaded on demand via progressive disclosure, instead of installing as three separate top-level skills. Nested SKILL.md files are not discovered by Claude Code (discovery is first-level only), so only `hotdata` registers while the three sub-skills ride along inside the `hotdata` directory in the release tarball. - skill.rs: `SKILL_NAMES = ["hotdata"]`; add `RETIRED_SKILL_NAMES` + cleanup so upgraders' stale top-level `hotdata-search` / `-analytics` / `-geospatial` entries are removed from the store, `~/.agents`, and each agent root on install/update (global and project paths). - hotdata/SKILL.md: replace "Bundled sub-skills / install all" with a "Sub-skills (loaded on demand)" section that Reads the nested files by path with trigger keywords. - Cargo.toml: point pre-release version bumps at the nested sub-skill paths. Also fix accumulated CLI drift in the skill docs (verified against the v0.21.0 CLI): - `connections` is not a command — sources are `ingest` datasources; replace `hotdata connections list/create/refresh` with `ingest list-datasources` / `new-datasource` / `show-datasource`. - `tables list` has no `--connection-id`; `indexes list` has no `-c` / `--connection-id` — use `--schema` / `--table` filters. - add `managed_load` to `jobs --job-type`; add `--result-id` to `databases load` / `databases tables load`; document `query status` exit codes (0/1/2/3). - repoint all cross-skill relative links to the new nested locations.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// `hotdata/subskills/` (see `RETIRED_SKILL_NAMES`). Cleans the store, the | ||
| /// `~/.agents` layer, and every detected home agent root. Best-effort: a | ||
| /// retired skill that isn't present is simply skipped. | ||
| fn remove_retired_skills_global() { |
There was a problem hiding this comment.
remove_retired_skills_global() is called from install() (line 572) and install_for_version() (line 287), but not from maybe_auto_update_after_cli_upgrade() — which is the auto-refresh path for users who upgrade the binary out-of-band (Homebrew, curl installer, package manager) and then just run any hotdata command.
Failure scenario: a user on v0.21.0 has ~/.hotdata/skills/hotdata-search plus ~/.claude/skills/hotdata-search from the old layout. They brew upgrade hotdata to the version carrying this change, then run hotdata tables list. maybe_auto_update_after_cli_upgrade() sees installed < current, downloads the new tarball, calls ensure_symlinks() (line 170) — and returns. The retired top-level dirs are never removed. On every subsequent run needs_refresh is now false (line 149-152), so the function early-returns at line 154 and the cleanup never gets a second chance. The stale hotdata-search / -analytics / -geospatial entries stay in the agent skill list permanently unless the user happens to run hotdata skills install by hand — which defeats the stated purpose of this PR.
Fix: call remove_retired_skills_global() right after ensure_symlinks() at line 170.
| **Prerequisites:** Authenticate, workspace, and catalog discovery via the **`hotdata`** skill (`connections`, `tables`, `databases`). | ||
| **Prerequisites:** Authenticate, workspace, and catalog discovery via the **`hotdata`** skill (`ingest` datasources, `tables`, `databases`). | ||
|
|
||
| **Related skills:** **`hotdata-search`** (BM25, vector, retrieval indexes), **`hotdata-geospatial`** (spatial SQL). |
There was a problem hiding this comment.
nit: cross-references between sub-skills are still bare names (hotdata-search, hotdata-geospatial) with no path. The name→path mapping only exists in the parent skills/hotdata/SKILL.md:27, and an agent that Reads this file on demand may not have that line in context — it's told to use a skill that is no longer registered anywhere. Same pattern in subskills/search/SKILL.md:13,65,66, subskills/geospatial/SKILL.md:11, and subskills/analytics/references/WORKFLOWS.md:5,27.
Suggest making them relative paths the way references/WORKFLOWS.md:15-17 already does, e.g. **`hotdata-search`** (`../search/SKILL.md`). (not blocking)
|
|
||
| /// Project-scoped counterpart to `remove_retired_skills_global`: clears stale | ||
| /// retired skills from `./.agents/skills` and each detected project agent root. | ||
| fn remove_retired_skills_project(cwd: &std::path::Path, project_skills_root: &std::path::Path) { |
There was a problem hiding this comment.
nit: remove_retired_skills_project is already parameterized on cwd and project_skills_root (unlike its global sibling, which reads $HOME), so it's directly unit-testable with a tempdir — but no test was added. A test that lays down .agents/skills/hotdata-search + .claude/skills/hotdata-search (as a symlink, to exercise the is_symlink() branch in remove_skill_path) and asserts both are gone while hotdata survives would pin the behavior this PR exists to deliver. (not blocking)
There was a problem hiding this comment.
Review
Blocking Issues
src/commands/skill.rs:140-192—maybe_auto_update_after_cli_upgrade()downloads the new tarball and callsensure_symlinks()but never callsremove_retired_skills_global().install()(line 572) andinstall_for_version()(line 287) both do. Users who upgrade the binary out-of-band (Homebrew, curl installer, package manager) hit only the auto-update path, so their stale top-levelhotdata-search/hotdata-analytics/hotdata-geospatialentries are left in~/.hotdata/skills,~/.agents/skills, and~/.claude/skills. Worse, once the auto-update succeeds the version check at line 149-152 makesneeds_refreshfalse forever, so the cleanup never runs on a later invocation either — the retired skills stay in the autocomplete list permanently unless the user manually runshotdata skills install. That is the exact outcome this PR is meant to prevent.
Action Required
Call remove_retired_skills_global() after ensure_symlinks() in maybe_auto_update_after_cli_upgrade() (line 170).
Everything else checks out — I verified the CLI drift claims against the source (ingest list-datasources/new-datasource/show-datasource exist; tables list and indexes list have no --connection-id, with indexes.rs:1368 asserting rejection; --result-id on both load paths; managed_load in the --job-type value parser; EXIT_INCOMPLETE_RESULT = 3), confirmed every relative markdown link across the moved files resolves, and confirmed dist-workspace.toml:27 tars skills/ recursively so the nested sub-skills still ship.
Address review on #244. - `maybe_auto_update_after_cli_upgrade()` (the auto-refresh path hit by Homebrew/curl/package-manager upgraders) refreshed symlinks but never called `remove_retired_skills_global()`, and its version gate means it never runs again — so stale top-level `hotdata-search`/`-analytics`/`-geospatial` entries would persist permanently. Call cleanup right after `ensure_symlinks()`. - Add a unit test for `remove_retired_skills_project` (real dir + symlink → both removed, `hotdata` survives). - Give sub-skill cross-references relative paths (e.g. `../search/SKILL.md`) so a sub-skill loaded on demand can reach its siblings without the parent's name→path mapping in context.
|
Addressed in 975b5aa:
|
| return; | ||
| } | ||
| let _ = if path.is_symlink() { | ||
| fs::remove_file(path) |
There was a problem hiding this comment.
nit: on Windows, fs::remove_file fails for directory symlinks (DeleteFileW rejects directories) — fs::remove_dir is the one that removes a dir symlink. ensure_symlink_or_copy creates these with symlink_dir (line 330), so on a Windows box with Developer Mode/admin enabled the ~/.agents/skills/hotdata-search and ~/.claude/skills/hotdata-search links are directory symlinks and this removal silently no-ops (the error is swallowed by let _ =). The store copy is a real dir so it goes away, but the retired entries stay in the agent skill list — the exact failure this PR is fixing. Windows users without symlink privilege get copies, which remove_dir_all handles fine, so it only bites the symlink-capable case.
| fs::remove_file(path) | |
| let _ = if path.is_symlink() { | |
| fs::remove_file(path).or_else(|_| fs::remove_dir(path)) | |
| } else { |
(ensure_symlink_or_copy at line 316 has the same pattern, but there the error surfaces to the caller rather than being dropped.) (not blocking)
1 participant
What
Make only
hotdataappear in the Claude Code / agent skill list (autocomplete). The three specialized guides — search, analytics, geospatial — are kept in full but bundled inside thehotdataskill underskills/hotdata/subskills/<name>/and loaded on demand (progressive disclosure) rather than installed as three separate top-level skills.Why
Claude Code discovers skills at the first level only (
<skills-root>/<name>/SKILL.md); nestedSKILL.mdfiles are not registered. Bundling the three underhotdata/subskills/means they still ship in the release tarball and areReadon demand, but they no longer clutter the skill autocomplete —hotdatais the single entry point that dispatches to them.Changes
Structure / install
hotdata-{search,analytics,geospatial}/→hotdata/subskills/{search,analytics,geospatial}/(content preserved).skill.rs:SKILL_NAMES = ["hotdata"]; addRETIRED_SKILL_NAMES+ best-effort cleanup so an upgrader's stale top-levelhotdata-search/-analytics/-geospatialentries are removed from the store,~/.agents, and each agent root (global and--projectinstall paths).hotdata/SKILL.md: "Sub-skills (loaded on demand)" section that Reads the nested files by path with their trigger keywords.Cargo.toml: pre-release version bumps point at the nested sub-skill paths.tar -czf skills.tar.gz skills/) already recurses, so the nested files ship; extraction yields exactly one top-levelhotdatadir (verified).CLI drift fixes (verified against the v0.21.0 CLI)
connectionsis not a command — sources areingestdatasources. Replacehotdata connections list/create/refreshwithingest list-datasources/new-datasource/show-datasource.tables listhas no--connection-id;indexes listhas no-c/--connection-id— use--schema/--tablefilters.managed_loadtojobs --job-type; add--result-idtodatabases load/databases tables load; documentquery statusexit codes (0/1/2/3).Verification
cargo build+cargo testpass.hotdatabecomes a top-level skill dir; sub-skills nested underhotdata/subskills/.connections/--connection-id/indexes list -cdrift: none remain.