diff --git a/lib/dev/deps/gem_skill_linker.rb b/lib/dev/deps/gem_skill_linker.rb index 2df5767..ae43c48 100644 --- a/lib/dev/deps/gem_skill_linker.rb +++ b/lib/dev/deps/gem_skill_linker.rb @@ -48,33 +48,24 @@ class GemSkillLinker def initialize(project_root:, skills_dir: nil, tmpdir: Dir.tmpdir) @project_root = Pathname(project_root) @skills_dir = Pathname(skills_dir || @project_root.join(*AGENT_SKILLS_SUBDIRS)) - @skill_installer = SkillInstaller.new(skills_dir: @skills_dir) - @tmpdir_roots = tmpdir_roots(Pathname(tmpdir)) + @skill_installer = SkillInstaller.new(skills_dir: @skills_dir, tmpdir: tmpdir) end # Scan the locked gem set for shipped skills and refresh the project's # links: install one per skill found, prune gem links whose gem left the - # lock. A skill that resolves under the temp dir is never linked (warned - # and skipped — a persistent link to purgeable state silently dangles - # later), but its gem still counts as present for pruning, so an - # ephemeral resolution cannot delete a durable link minted earlier. - # Never raises — skill links are hygiene riding a dependency install, - # and hygiene must not block correctness (failures are reported on - # stderr). + # lock. A skill that resolves under the temp dir is never linked — + # SkillInstaller refuses ephemeral sources at the shared seam — but its + # gem still counts as present for pruning, so an ephemeral resolution + # cannot delete a durable link minted earlier. Never raises — skill + # links are hygiene riding a dependency install, and hygiene must not + # block correctness (failures are reported on stderr). # # @return [void] def link_all return unless gemfile_path.exist? expected = expected_links - expected.each do |name, skill_dir| - if ephemeral?(skill_dir) - $stderr.puts "dev: warning: not linking #{name} — #{skill_dir} is under the temp dir " \ - "and would dangle once it is purged." - else - @skill_installer.install(name, skill_dir) - end - end + expected.each { |name, skill_dir| @skill_installer.install(name, skill_dir) } prune_stale_links(expected.keys) rescue StandardError => e $stderr.puts "dev: warning: could not refresh gem skill links (#{e.message})." @@ -162,30 +153,6 @@ def locked_gem_names names.uniq end - # The temp root in both its raw and fully-resolved forms — on macOS - # Dir.tmpdir is under /var/... while bundler reports the - # /private/var/... realpath, so containment must check both spellings. - # - # @param tmpdir [Pathname] - # @return [Array] - def tmpdir_roots(tmpdir) - expanded = tmpdir.expand_path - roots = [expanded] - roots << expanded.realpath if expanded.exist? - roots.uniq - end - - # Whether a resolved skill directory lives under the temp dir — the - # signature of a harness resolving the bundle into its own purgeable - # cache, whatever produced it. - # - # @param path [Pathname] - # @return [Boolean] - def ephemeral?(path) - resolved = path.exist? ? path.realpath : path.expand_path - @tmpdir_roots.any? { |root| resolved.to_s.start_with?("#{root}#{File::SEPARATOR}") } - end - # Remove gem links that no current gem accounts for (the gem left the # lock; its tree may still exist on disk, so broken-link pruning alone # would miss it). Only `gem-`-prefixed symlinks are candidates — diff --git a/lib/dev/skill_installer.rb b/lib/dev/skill_installer.rb index a3dfe93..ff3dd6f 100644 --- a/lib/dev/skill_installer.rb +++ b/lib/dev/skill_installer.rb @@ -28,11 +28,19 @@ class SkillInstaller # @param skills_dir [Pathname, String] target dir the symlinks live in; # defaults to the user-global ~/.cursor/skills - def initialize(skills_dir: Pathname(Dir.home) / ".cursor" / "skills") + # @param tmpdir [Pathname, String] ephemeral temp root that links must + # never target; defaults to Dir.tmpdir (override for tests, whose + # fixture skill trees themselves live under the real temp dir) + def initialize(skills_dir: Pathname(Dir.home) / ".cursor" / "skills", tmpdir: Dir.tmpdir) @skills_dir = Pathname(skills_dir) + @tmpdir_roots = tmpdir_roots(Pathname(tmpdir)) end - # Install or refresh one skill symlink. Never raises: a broken skill + # Install or refresh one skill symlink. A source that resolves under the + # temp dir is never linked (warned and skipped): a durable link to + # purgeable state silently dangles later, whatever produced it — e.g. a + # dev running from a temp clone would re-point the machine-global links + # at itself through SHIPPED_SKILLS_DIR. Never raises: a broken skill # install must not block the command it rides (the failure is reported # on stderr). # @@ -43,6 +51,12 @@ def install(name, source_dir) source = Pathname(source_dir) return unless source.directory? + if ephemeral?(source) + $stderr.puts "dev: warning: not linking #{name} — #{source} is under the temp dir " \ + "and would dangle once it is purged." + return + end + link = @skills_dir / name return if link.symlink? && link.readlink == source @@ -92,6 +106,29 @@ def remove(name) private + # The temp root in both its raw and fully-resolved forms — on macOS + # Dir.tmpdir is under /var/... while realpath resolution reports the + # /private/var/... spelling, so containment must check both. + # + # @param tmpdir [Pathname] + # @return [Array] + def tmpdir_roots(tmpdir) + expanded = tmpdir.expand_path + roots = [expanded] + roots << expanded.realpath if expanded.exist? + roots.uniq + end + + # Whether a path resolves under the temp dir — a durable link to it + # would dangle once the temp dir is purged, whatever produced it. + # + # @param path [Pathname] + # @return [Boolean] + def ephemeral?(path) + resolved = path.exist? ? path.realpath : path.expand_path + @tmpdir_roots.any? { |root| resolved.to_s.start_with?("#{root}#{File::SEPARATOR}") } + end + # Prune symlinks that point under source_root but whose target skill no # longer exists (e.g. a skill removed from the knowledge repo). Links # pointing elsewhere are never touched. diff --git a/test/dev/learnings/accessor_test.rb b/test/dev/learnings/accessor_test.rb index 54b43b1..d39a5b3 100644 --- a/test/dev/learnings/accessor_test.rb +++ b/test/dev/learnings/accessor_test.rb @@ -36,7 +36,9 @@ def build_env(dir, project_root: :default) File.write(config, "knowledge_repo: #{source}\n") settings = Dev::Settings.new(config_path: config) cache = Dev::Learnings::Cache.new(repo: source, dir: File.join(dir, "cache")) - installer = Dev::SkillInstaller.new(skills_dir: File.join(dir, "user-skills")) + # The fixture cache lives under the real temp dir; the tmpdir override + # keeps the installer's ephemeral-source guard out of these tests' way. + installer = Dev::SkillInstaller.new(skills_dir: File.join(dir, "user-skills"), tmpdir: File.join(dir, "tmp")) synchronizer = Dev::Learnings::Synchronizer.new(settings: settings, cache: cache, skill_installer: installer) project = project_root == :default ? Pathname(dir) / "repo" : project_root FileUtils.mkdir_p(project) if project diff --git a/test/dev/learnings/synchronizer_test.rb b/test/dev/learnings/synchronizer_test.rb index 6dec271..4cea113 100644 --- a/test/dev/learnings/synchronizer_test.rb +++ b/test/dev/learnings/synchronizer_test.rb @@ -20,7 +20,9 @@ def build_env(dir, refresh_floor: 0) File.write(config, "knowledge_repo: #{source}\n") settings = Dev::Settings.new(config_path: config) cache = Dev::Learnings::Cache.new(repo: source, dir: File.join(dir, "cache"), refresh_floor: refresh_floor) - installer = Dev::SkillInstaller.new(skills_dir: File.join(dir, "user-skills")) + # The fixture cache lives under the real temp dir; the tmpdir override + # keeps the installer's ephemeral-source guard out of these tests' way. + installer = Dev::SkillInstaller.new(skills_dir: File.join(dir, "user-skills"), tmpdir: File.join(dir, "tmp")) project = Pathname(dir) / "repo" FileUtils.mkdir_p(project) synchronizer = Dev::Learnings::Synchronizer.new(settings: settings, cache: cache, skill_installer: installer) @@ -142,7 +144,7 @@ def commit_all(source, message) dir = Dir.mktmpdir("dev-learnings-sync-test-") saved_env = ENV.delete("DEV_KNOWLEDGE_REPO") settings = Dev::Settings.new(config_path: File.join(dir, "config.yml")) - installer = Dev::SkillInstaller.new(skills_dir: File.join(dir, "user-skills")) + installer = Dev::SkillInstaller.new(skills_dir: File.join(dir, "user-skills"), tmpdir: File.join(dir, "tmp")) synchronizer = Dev::Learnings::Synchronizer.for(settings: settings, skill_installer: installer) project = Pathname(dir) / "repo" FileUtils.mkdir_p(project) diff --git a/test/dev/skill_installer_test.rb b/test/dev/skill_installer_test.rb index c9a670c..8b0dce1 100644 --- a/test/dev/skill_installer_test.rb +++ b/test/dev/skill_installer_test.rb @@ -15,12 +15,20 @@ def build_skill(dir, *path_parts) source end + # Installer under test. The real Dir.tmpdir contains these tests' own + # fixture trees, so every installer gets a tmpdir override pointing inside + # the fixture dir — sources built by build_skill read as durable, and a + # test opts into ephemerality by building under `/tmp`. + def build_installer(dir, skills_dir:) + Dev::SkillInstaller.new(skills_dir: skills_dir, tmpdir: File.join(dir, "tmp")) + end + test "install creates the symlink on first run" do Given "a skill source and an empty skills dir" dir = Dir.mktmpdir("dev-skill-test-") source = build_skill(dir, "share", "cursor-skills", "ai-flow") skills_dir = File.join(dir, "skills") - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) When "installing the skill" installer.install("ai-flow", source) @@ -38,7 +46,7 @@ def build_skill(dir, *path_parts) dir = Dir.mktmpdir("dev-skill-test-") source = build_skill(dir, "share", "cursor-skills", "ai-flow") skills_dir = File.join(dir, "skills") - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) installer.install("ai-flow", source) When "installing again" @@ -58,7 +66,7 @@ def build_skill(dir, *path_parts) skills_dir = File.join(dir, "skills") FileUtils.mkdir_p(skills_dir) File.symlink(File.join(dir, "old-location"), File.join(skills_dir, "ai-flow")) - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) When "installing the skill" installer.install("ai-flow", source) @@ -78,7 +86,7 @@ def build_skill(dir, *path_parts) user_dir = File.join(skills_dir, "ai-flow") FileUtils.mkdir_p(user_dir) File.write(File.join(user_dir, "SKILL.md"), "user's own\n") - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) old_stderr = $stderr $stderr = StringIO.new @@ -99,7 +107,7 @@ def build_skill(dir, *path_parts) Given "an installer and a nonexistent source" dir = Dir.mktmpdir("dev-skill-test-") skills_dir = File.join(dir, "skills") - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) When "installing from the missing source" installer.install("ai-flow", File.join(dir, "missing")) @@ -111,6 +119,71 @@ def build_skill(dir, *path_parts) FileUtils.rm_rf(dir) end + test "install refuses a source under the temp dir and warns" do + Given "a skill source living under the ephemeral temp dir (e.g. a dev checkout in a build workspace)" + dir = Dir.mktmpdir("dev-skill-test-") + source = build_skill(dir, "tmp", "clone", "share", "cursor-skills", "ai-flow") + skills_dir = File.join(dir, "skills") + installer = build_installer(dir, skills_dir: skills_dir) + old_stderr = $stderr + $stderr = StringIO.new + + When "installing the skill" + installer.install("ai-flow", source) + + Then "no link is minted and the skip is warned" + !File.exist?(File.join(skills_dir, "ai-flow")) + $stderr.string.include?("not linking ai-flow") + + Cleanup + $stderr = old_stderr + FileUtils.rm_rf(dir) + end + + test "install refuses an ephemeral source even when it would replace a dangling link" do + Given "a link already dangling, and a refresh source under the temp dir" + dir = Dir.mktmpdir("dev-skill-test-") + source = build_skill(dir, "tmp", "clone", "share", "cursor-skills", "ai-flow") + skills_dir = File.join(dir, "skills") + FileUtils.mkdir_p(skills_dir) + File.symlink(File.join(dir, "gone"), File.join(skills_dir, "ai-flow")) + installer = build_installer(dir, skills_dir: skills_dir) + old_stderr = $stderr + $stderr = StringIO.new + + When "installing the skill" + installer.install("ai-flow", source) + + Then "the existing link is left alone rather than re-pointed at purgeable state" + File.readlink(File.join(skills_dir, "ai-flow")) == File.join(dir, "gone") + + Cleanup + $stderr = old_stderr + FileUtils.rm_rf(dir) + end + + test "temp dir containment sees through symlinked temp roots (macOS /var vs /private/var)" do + Given "a tmpdir override that is a symlink to the dir the source lives under" + dir = Dir.mktmpdir("dev-skill-test-") + source = build_skill(dir, "tmp", "share", "cursor-skills", "ai-flow") + tmp_alias = File.join(dir, "tmp-alias") + File.symlink(File.join(dir, "tmp"), tmp_alias) + skills_dir = File.join(dir, "skills") + installer = Dev::SkillInstaller.new(skills_dir: skills_dir, tmpdir: tmp_alias) + old_stderr = $stderr + $stderr = StringIO.new + + When "installing the skill" + installer.install("ai-flow", source) + + Then "the path is recognized as ephemeral and never links" + !File.exist?(File.join(skills_dir, "ai-flow")) + + Cleanup + $stderr = old_stderr + FileUtils.rm_rf(dir) + end + test "install warns instead of raising when the skills dir cannot be created" do Given "a skills dir under a read-only parent" dir = Dir.mktmpdir("dev-skill-test-") @@ -118,7 +191,7 @@ def build_skill(dir, *path_parts) read_only_parent = File.join(dir, "read-only") FileUtils.mkdir_p(read_only_parent) FileUtils.chmod(0o555, read_only_parent) - installer = Dev::SkillInstaller.new(skills_dir: File.join(read_only_parent, "skills")) + installer = build_installer(dir, skills_dir: File.join(read_only_parent, "skills")) old_stderr = $stderr $stderr = StringIO.new @@ -141,7 +214,7 @@ def build_skill(dir, *path_parts) build_skill(dir, "source", "typed-errors") FileUtils.mkdir_p(File.join(dir, "source", "not-a-skill")) skills_dir = File.join(dir, "skills") - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) When "installing all skills" installer.install_all(File.join(dir, "source")) @@ -160,7 +233,7 @@ def build_skill(dir, *path_parts) dir = Dir.mktmpdir("dev-skill-test-") build_skill(dir, "gems", "rspock-1.2.0", "skills", "rspock") skills_dir = File.join(dir, "skills") - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) When "installing all skills with a prefix" installer.install_all(File.join(dir, "gems", "rspock-1.2.0", "skills"), prefix: "gem-rspock--") @@ -179,7 +252,7 @@ def build_skill(dir, *path_parts) build_skill(dir, "source", "kept") removed = build_skill(dir, "source", "removed") skills_dir = File.join(dir, "skills") - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) installer.install_all(source_root) FileUtils.rm_rf(removed) foreign_target = build_skill(dir, "elsewhere", "mine") @@ -206,7 +279,7 @@ def build_skill(dir, *path_parts) skills_dir = File.join(dir, "skills") FileUtils.mkdir_p(skills_dir) FileUtils.chmod(0o000, skills_dir) - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) old_stderr = $stderr $stderr = StringIO.new @@ -227,7 +300,7 @@ def build_skill(dir, *path_parts) dir = Dir.mktmpdir("dev-skill-test-") source = build_skill(dir, "source", "linked") skills_dir = File.join(dir, "skills") - installer = Dev::SkillInstaller.new(skills_dir: skills_dir) + installer = build_installer(dir, skills_dir: skills_dir) installer.install("linked", source) user_dir = File.join(skills_dir, "user-owned") FileUtils.mkdir_p(user_dir)