feat(skills): geospatial skill, multi-skill install, and auto-update#68
feat(skills): geospatial skill, multi-skill install, and auto-update#68eddietejeda merged 6 commits intomainfrom
Conversation
Ships a separate hotdata-geospatial skill and installs it alongside the base hotdata skill so agents can load geospatial guidance only when relevant.
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}
| if !all_exist { | ||
| row("Installed", &"Partial".yellow().to_string()); | ||
| for skill_name in SKILL_NAMES { | ||
| let ok = skill_store_path(skill_name).exists(); | ||
| let status = if ok { | ||
| "Yes".green().to_string() | ||
| } else { | ||
| "No".red().to_string() | ||
| }; | ||
| row(&format!("{skill_name}"), &status); | ||
| } | ||
| println!("\nRun 'hotdata skills install' to install."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit: the early return after printing the partial-install breakdown means agent symlink status is never shown for the skills that are installed. A user with hotdata present but hotdata-geospatial missing can't tell whether their .claude/skills/hotdata link is intact without running install again. Consider falling through to the agent-skills loop rather than returning, or at least showing the symlink rows for installed skills. (not blocking)
| for skill_name in SKILL_NAMES { | ||
| println!( | ||
| "{:<20}{}", | ||
| format!("{skill_name}:"), | ||
| skill_store_path(skill_name).display().to_string().cyan() | ||
| ); | ||
| } |
There was a problem hiding this comment.
nit: this correctly prints all skill store paths, but install() decides whether to re-download using only the primary skill's version (read_installed_version()). If hotdata-geospatial is absent from the store while hotdata is up-to-date, the version check returns "already up to date", ensure_symlinks() runs, and produces a dangling symlink for the geospatial skill. Consider also checking that all skill store paths exist before skipping the download:
Some(ref v) if *v >= current && SKILL_NAMES.iter().all(|n| skill_store_path(n).exists()) => {
// already up to date
}(not blocking)
There was a problem hiding this comment.
Review
Blocking Issues
Cargo.toml — missing pre-release-replacements entry for skills/hotdata-geospatial/SKILL.md
The pre-release-replacements table only bumps skills/hotdata/SKILL.md. skills/hotdata-geospatial/SKILL.md has no entry, so its version: field will stay pinned at 0.1.14 on every future release. The tarball for v0.1.15+ will ship geospatial skill metadata that claims the wrong version — agents that read the SKILL.md front-matter for version awareness will see a stale value.
Fix: add the missing replacement alongside the existing one in Cargo.toml:
{ file = "skills/hotdata-geospatial/SKILL.md", search = "^version: .+", replace = "version: {{version}}", exactly = 1 },Action Required
Add the hotdata-geospatial/SKILL.md entry to pre-release-replacements in Cargo.toml before merge.
Re-download when the primary skill version is current but a skill store directory is missing. Show agent skill symlink rows after partial install, and omit agent rows for skills not present in the store.
| Some(ref v) if *v >= current => { | ||
| Some(ref v) if *v >= current && all_skill_stores_present() => { | ||
| println!("Managed by skills agent — already up to date (v{v})."); | ||
| false |
There was a problem hiding this comment.
nit: same issue from the prior review — read_installed_version() only checks hotdata's SKILL.md. If hotdata-geospatial is absent from the store while hotdata is current, needs_download is false, the download is skipped, ensure_symlinks() runs, and the geospatial skill gets a dangling symlink.
| false | |
| Some(ref v) if *v >= current && SKILL_NAMES.iter().all(|n| skill_store_path(n).exists()) => { |
Same fix applies to the identical guard in install_project() (line 221). (not blocking)
When ~/.hotdata/skills/hotdata exists but the bundle is older than the CLI or incomplete, refresh from the matching release tarball before other commands. Skip if skills were never installed, if ~/.agents/.skill-lock.json manages installs, or when HOTDATA_SKILLS_AUTO_UPDATE is disabled. Route skill download progress to stderr so stdout stays clean for JSON/piped output.
| row("Installed", &"Yes".green().to_string()); | ||
|
|
||
| match &installed_version { | ||
| Some(v) if *v < current => { |
There was a problem hiding this comment.
nit: when neither skill is installed (both paths absent), all_exist is false and the output reads Installed: Partial — which implies a previous interrupted install rather than "never installed". The old code showed Installed: No cleanly for this case.
Consider distinguishing zero-installs from partial-installs:
| Some(v) if *v < current => { | |
| let any_exist = SKILL_NAMES | |
| .iter() | |
| .any(|name| skill_store_path(name).exists()); | |
| let all_exist = any_exist | |
| && SKILL_NAMES | |
| .iter() | |
| .all(|name| skill_store_path(name).exists()); | |
| if !any_exist { | |
| row("Installed", &"No".red().to_string()); | |
| println!("\nRun 'hotdata skills install' to install."); | |
| return; | |
| } | |
| if !all_exist { |
(not blocking)
| .iter() | ||
| .all(|name| skill_store_path(name).exists()); | ||
|
|
||
| if !all_exist { |
There was a problem hiding this comment.
nit: still shows Partial when neither skill has ever been installed. all_exist is false whether zero or one skill is missing, so a fresh machine prints Installed: Partial rather than Installed: No. Consider distinguishing the two cases:
let any_exist = SKILL_NAMES.iter().any(|name| skill_store_path(name).exists());
let all_exist = any_exist && SKILL_NAMES.iter().all(|name| skill_store_path(name).exists());
if !any_exist {
row("Installed", &"No".red().to_string());
println!("\nRun 'hotdata skills install' to install.");
return;
}
if !all_exist {(not blocking)
eddietejeda
changed the title
1 participant
Summary
hotdata-geospatialagent skill (skills/hotdata-geospatial/SKILL.md) with PostGIS-style / geospatial SQL guidance so agents can load it only when work is geospatial.hotdata skills install(global and--project) installs bothhotdataandhotdata-geospatialunder~/.hotdata/skills/and symlinks/copies into~/.agentsand optional agent roots (.claude,.pi).Cargo.toml:pre-release-replacementsbumpsversion:in both skillSKILL.mdfiles on release (same as basehotdataskill).hotdata skills statusshows agent symlink rows after partial install (missing store shows—).~/.hotdata/skills/hotdataalready exists and the bundle is older than this CLI or incomplete, refresh from the matching release tarball before other commands (skips barehotdata,hotdata skills …, and installs managed via~/.agents/.skill-lock.json). Skill download/progress goes to stderr so JSON/piped stdout stays clean. Failures warn on stderr and do not abort the command.Test plan
cargo testhotdata skills install/hotdata skills statuswith both skills present and with one skill dir removed (partial).SKILL.mdversion under~/.hotdata/skills/hotdataand run e.g.hotdata workspaces list— expect skill refresh on stderr when eligible.