fix(skills): stage updates in a temp dir and swap in-place (cli#13449)

Co-authored-by: sammorrowdrums <sammorrowdrums@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: SamMorrowDrums

acceptance/testdata/skills/skills-update-inplace.txtar

@@ -0,0 +1,31 @@
+# Updating a namespaced skill via --dir must write back to its original
+# location and must NOT delete the original directory (issue #13370).
+
+# Dry-run update should detect the namespaced skill and report an update
+exec gh skill update --dry-run --all --dir $WORK/skills-dir
+stdout 'git-commit'
+
+# Force update should re-download in-place
+exec gh skill update --force --all --dir $WORK/skills-dir
+stdout 'Updated'
+
+# Verify the SKILL.md was rewritten at the ORIGINAL namespaced location
+grep 'github-repo' $WORK/skills-dir/anthropics-skills/git-commit/SKILL.md
+! grep 'Test skill content' $WORK/skills-dir/anthropics-skills/git-commit/SKILL.md
+
+# The namespace directory must still exist (not deleted)
+exists $WORK/skills-dir/anthropics-skills/git-commit/SKILL.md
+
+# The skill must NOT have been relocated to a flat path
+! exists $WORK/skills-dir/git-commit/SKILL.md
+
+-- skills-dir/anthropics-skills/git-commit/SKILL.md --
+---
+name: git-commit
+description: Git commit helper
+metadata:
+  github-repo: https://github.com/github/awesome-copilot.git
+  github-tree-sha: 0000000000000000000000000000000000000000
+  github-path: skills/git-commit
+---
+Test skill content

pkg/cmd/skills/update/update.go

@@ -385,54 +385,11 @@ func updateRun(opts *UpdateOptions) error {
 
 	var failed bool
 	for _, u := range updates {
-		installOpts := &installer.Options{
-			Host:      u.local.repoHost,
-			Owner:     u.local.owner,
-			Repo:      u.local.repo,
-			Ref:       u.resolved.Ref,
-			SHA:       u.resolved.SHA,
-			Skills:    []discovery.Skill{u.skill},
-			AgentHost: u.local.host,
-			Scope:     u.local.scope,
-			GitRoot:   gitRoot,
-			HomeDir:   homeDir,
-			Client:    apiClient,
-		}
-		// When updating skills from a custom --dir, host is nil.
-		// Use the skill's install root as the target. For namespaced
-		// skills (name contains "/"), the dir is two levels below the
-		// root instead of one.
-		if u.local.host == nil {
-			base := filepath.Dir(u.local.dir)
-			if strings.Contains(u.local.name, "/") {
-				base = filepath.Dir(base)
-			}
-			installOpts.Dir = base
-		}
-		_, installErr := installer.Install(installOpts)
-		if installErr != nil {
-			fmt.Fprintf(opts.IO.ErrOut, "%s Failed to update %s: %v\n", cs.FailureIcon(), u.local.name, installErr)
+		if err := updateSkillInPlace(opts, u, apiClient, gitRoot, homeDir); err != nil {
+			fmt.Fprintf(opts.IO.ErrOut, "%s Failed to update %s: %v\n", cs.FailureIcon(), u.local.name, err)
 			failed = true
 			continue
 		}
-
-		// When the install location has changed (e.g. migrating from a
-		// namespaced layout to flat), remove the old directory so that the
-		// stale copy does not shadow the freshly installed one.
-		newDir := filepath.Join(installOpts.Dir, u.skill.Name)
-		if installOpts.Dir == "" && u.local.host != nil {
-			if d, err := u.local.host.InstallDir(u.local.scope, gitRoot, homeDir); err == nil {
-				newDir = filepath.Join(d, u.skill.Name)
-			}
-		}
-		if newDir != "" && u.local.dir != "" && filepath.Clean(newDir) != filepath.Clean(u.local.dir) {
-			_ = os.RemoveAll(u.local.dir)
-			// Remove the parent if it is now empty (leftover namespace directory).
-			parent := filepath.Dir(u.local.dir)
-			if entries, readErr := os.ReadDir(parent); readErr == nil && len(entries) == 0 {
-				_ = os.Remove(parent)
-			}
-		}
 		if opts.IO.IsStdoutTTY() {
 			fmt.Fprintf(opts.IO.Out, "%s Updated %s\n", cs.SuccessIcon(), u.local.name)
 		} else {
@@ -447,6 +404,121 @@ func updateRun(opts *UpdateOptions) error {
 	return nil
 }
 
+// updateSkillInPlace installs the resolved update into a staging directory
+// alongside the existing skill directory and, on success, atomically swaps
+// the staged contents into place via same-filesystem renames. This
+// guarantees:
+//
+//   - The skill directory's own inode is preserved, so symlinks, mounts, and
+//     external references that point at it stay valid.
+//   - Stale files from the previous version are removed.
+//   - A failure at any point (install, read, rename) leaves the existing
+//     skill completely untouched: existing files are first moved aside into
+//     a backup directory and restored if any subsequent step fails.
+func updateSkillInPlace(opts *UpdateOptions, u pendingUpdate, apiClient *api.Client, gitRoot, homeDir string) error {
+	if u.local.dir == "" {
+		return fmt.Errorf("cannot update %s: no install location recorded", u.local.name)
+	}
+
+	parent := filepath.Dir(u.local.dir)
+	if err := os.MkdirAll(parent, 0o755); err != nil {
+		return fmt.Errorf("could not ensure parent directory %s: %w", parent, err)
+	}
+
+	// Stage as a sibling of the existing skill directory so the swap stays
+	// on the same filesystem and every rename is atomic.
+	staging, err := os.MkdirTemp(parent, "."+u.skill.Name+".gh-skill-update-")
+	if err != nil {
+		return fmt.Errorf("could not create staging directory: %w", err)
+	}
+	defer os.RemoveAll(staging)
+
+	installOpts := &installer.Options{
+		Host:    u.local.repoHost,
+		Owner:   u.local.owner,
+		Repo:    u.local.repo,
+		Ref:     u.resolved.Ref,
+		SHA:     u.resolved.SHA,
+		Skills:  []discovery.Skill{u.skill},
+		Dir:     staging,
+		GitRoot: gitRoot,
+		HomeDir: homeDir,
+		Client:  apiClient,
+	}
+	if _, err := installer.Install(installOpts); err != nil {
+		return err
+	}
+
+	stagedSkillDir := filepath.Join(staging, u.skill.Name)
+	if _, err := os.Stat(stagedSkillDir); err != nil {
+		return fmt.Errorf("installer did not produce %s: %w", stagedSkillDir, err)
+	}
+
+	if err := os.MkdirAll(u.local.dir, 0o755); err != nil {
+		return fmt.Errorf("could not ensure skill directory %s: %w", u.local.dir, err)
+	}
+
+	return swapDirectoryContents(u.local.dir, stagedSkillDir)
+}
+
+// swapDirectoryContents replaces the entries inside dest with the entries
+// inside src, preserving dest's inode. It first moves every existing entry
+// into a sibling backup directory, then moves the staged entries into dest.
+// If any step fails, the original contents are restored from the backup.
+//
+// src and dest must live on the same filesystem so renames are atomic.
+func swapDirectoryContents(dest, src string) error {
+	backup, err := os.MkdirTemp(filepath.Dir(dest), "."+filepath.Base(dest)+".gh-skill-backup-")
+	if err != nil {
+		return fmt.Errorf("could not create backup directory: %w", err)
+	}
+
+	existing, err := os.ReadDir(dest)
+	if err != nil {
+		_ = os.RemoveAll(backup)
+		return fmt.Errorf("could not read skill directory %s: %w", dest, err)
+	}
+	var movedOut []string
+	for _, entry := range existing {
+		if err := os.Rename(filepath.Join(dest, entry.Name()), filepath.Join(backup, entry.Name())); err != nil {
+			restoreBackup(dest, backup, movedOut, nil)
+			return fmt.Errorf("could not move %s aside: %w", entry.Name(), err)
+		}
+		movedOut = append(movedOut, entry.Name())
+	}
+
+	staged, err := os.ReadDir(src)
+	if err != nil {
+		restoreBackup(dest, backup, movedOut, nil)
+		return fmt.Errorf("could not read staged skill directory %s: %w", src, err)
+	}
+	var movedIn []string
+	for _, entry := range staged {
+		from := filepath.Join(src, entry.Name())
+		to := filepath.Join(dest, entry.Name())
+		if err := os.Rename(from, to); err != nil {
+			restoreBackup(dest, backup, movedOut, movedIn)
+			return fmt.Errorf("could not move %s into place: %w", entry.Name(), err)
+		}
+		movedIn = append(movedIn, entry.Name())
+	}
+
+	_ = os.RemoveAll(backup)
+	return nil
+}
+
+// restoreBackup undoes a partial swap by removing any freshly installed
+// entries and moving the original entries back from backup into dest.
+func restoreBackup(dest, backup string, movedOut, movedIn []string) {
+	for _, name := range movedIn {
+		_ = os.RemoveAll(filepath.Join(dest, name))
+	}
+	for _, name := range movedOut {
+		_ = os.Rename(filepath.Join(backup, name), filepath.Join(dest, name))
+	}
+	_ = os.RemoveAll(backup)
+}
+
 // scanAllAgents walks every registered agent's skill directory (project + user scope) and
 // collects installed skills. Shared install roots are scanned only once.
 func scanAllAgents(gitRoot, homeDir string) []installedSkill {

pkg/cmd/skills/update/update_test.go

@@ -709,7 +709,7 @@ func TestUpdateRun(t *testing.T) {
 			wantStdout: "Updated code-review",
 		},
 		{
-			name: "namespaced skill with --dir resolves install base correctly",
+			name: "namespaced skill with --dir updates in-place",
 			setup: func(t *testing.T, dir string) {
 				t.Helper()
 				homeDir := t.TempDir()
@@ -727,6 +727,8 @@ func TestUpdateRun(t *testing.T) {
 					---
 					Old namespaced content
 				`)), 0o644))
+				// Plant a stale file that should be cleaned during update.
+				require.NoError(t, os.WriteFile(filepath.Join(skillDir, "STALE.txt"), []byte("leftover"), 0o644))
 			},
 			stubs: func(reg *httpmock.Registry) {
 				reg.Register(
@@ -762,14 +764,20 @@ func TestUpdateRun(t *testing.T) {
 			},
 			verify: func(t *testing.T, dir string) {
 				t.Helper()
-				// After update, skill should be installed flat (not namespaced).
-				content, err := os.ReadFile(filepath.Join(dir, "code-review", "SKILL.md"))
+				// Skill must stay in its original namespaced directory.
+				content, err := os.ReadFile(filepath.Join(dir, "monalisa", "code-review", "SKILL.md"))
 				require.NoError(t, err)
 				assert.Contains(t, string(content), "github-repo: https://github.com/monalisa/octocat-skills")
 				assert.NotContains(t, string(content), "Old namespaced content")
-				// Old namespaced directory should be cleaned up.
+				// Skill must NOT have been relocated to a flat path.
+				_, err = os.Stat(filepath.Join(dir, "code-review", "SKILL.md"))
+				assert.True(t, os.IsNotExist(err), "skill should not be relocated to flat path")
+				// Namespace directory must still exist.
 				_, err = os.Stat(filepath.Join(dir, "monalisa", "code-review"))
-				assert.True(t, os.IsNotExist(err), "old namespaced directory should be removed")
+				assert.False(t, os.IsNotExist(err), "namespaced directory must not be deleted")
+				// Stale file should have been cleaned during update.
+				_, err = os.Stat(filepath.Join(dir, "monalisa", "code-review", "STALE.txt"))
+				assert.True(t, os.IsNotExist(err), "stale file should be removed during update")
 			},
 			wantStdout: "Updated monalisa/code-review",
 		},
@@ -1219,9 +1227,9 @@ func TestUpdateRun(t *testing.T) {
 
 			if tt.wantErr != "" {
 				assert.EqualError(t, err, tt.wantErr)
-				return
+			} else {
+				require.NoError(t, err)
 			}
-			require.NoError(t, err)
 			if tt.wantStderr != "" {
 				assert.Contains(t, stderr.String(), tt.wantStderr)
 			}
@@ -1234,3 +1242,82 @@ func TestUpdateRun(t *testing.T) {
 		})
 	}
 }
+
+// If the staged contents cannot be installed after the existing entries
+// have already been moved aside, the original skill directory must be
+// restored byte-for-byte and its inode must be preserved.
+func TestSwapDirectoryContents_RollsBackOnFailure(t *testing.T) {
+	parent := t.TempDir()
+	dest := filepath.Join(parent, "code-review")
+	require.NoError(t, os.MkdirAll(dest, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dest, "SKILL.md"), []byte("original"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(dest, "extra.txt"), []byte("keep me"), 0o644))
+	subdir := filepath.Join(dest, "examples")
+	require.NoError(t, os.MkdirAll(subdir, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(subdir, "demo.txt"), []byte("demo"), 0o644))
+
+	destBefore, err := os.Stat(dest)
+	require.NoError(t, err)
+
+	// Point src at a path that does not exist so the staged ReadDir fails
+	// after the existing entries have already been moved aside. This is the
+	// only deterministic, portable way to exercise the rollback branch from
+	// outside the swap.
+	src := filepath.Join(parent, "does-not-exist")
+
+	err = swapDirectoryContents(dest, src)
+	require.Error(t, err, "swap should fail when staged dir cannot be read")
+
+	destAfter, err := os.Stat(dest)
+	require.NoError(t, err)
+	assert.True(t, os.SameFile(destBefore, destAfter), "dest directory identity must be preserved across rollback")
+
+	content, readErr := os.ReadFile(filepath.Join(dest, "SKILL.md"))
+	require.NoError(t, readErr)
+	assert.Equal(t, "original", string(content), "original SKILL.md must be restored")
+	extra, readErr := os.ReadFile(filepath.Join(dest, "extra.txt"))
+	require.NoError(t, readErr)
+	assert.Equal(t, "keep me", string(extra), "original extra.txt must be restored")
+	demo, readErr := os.ReadFile(filepath.Join(subdir, "demo.txt"))
+	require.NoError(t, readErr)
+	assert.Equal(t, "demo", string(demo), "original nested subdir must be restored intact")
+
+	entries, err := os.ReadDir(parent)
+	require.NoError(t, err)
+	var leftovers []string
+	for _, e := range entries {
+		if e.Name() != "code-review" {
+			leftovers = append(leftovers, e.Name())
+		}
+	}
+	assert.Empty(t, leftovers, "no staging or backup directories should remain after rollback")
+}
+
+// The skill directory's own inode must survive an update so symlinks,
+// bind mounts, and other external references pointing at it remain
+// valid. Per-entry rename swaps satisfy this; replacing the directory
+// itself would not.
+func TestSwapDirectoryContents_PreservesDestInode(t *testing.T) {
+	parent := t.TempDir()
+	dest := filepath.Join(parent, "code-review")
+	require.NoError(t, os.MkdirAll(dest, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(dest, "old.txt"), []byte("old"), 0o644))
+
+	src := filepath.Join(parent, "staged")
+	require.NoError(t, os.MkdirAll(src, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(src, "new.txt"), []byte("new"), 0o644))
+
+	destBefore, err := os.Stat(dest)
+	require.NoError(t, err)
+
+	require.NoError(t, swapDirectoryContents(dest, src))
+
+	destAfter, err := os.Stat(dest)
+	require.NoError(t, err)
+	assert.True(t, os.SameFile(destBefore, destAfter), "dest directory identity must be preserved")
+
+	assert.NoFileExists(t, filepath.Join(dest, "old.txt"), "stale files must be removed")
+	content, err := os.ReadFile(filepath.Join(dest, "new.txt"))
+	require.NoError(t, err)
+	assert.Equal(t, "new", string(content), "staged content must be installed")
+}