fix(skills): stage updates in a temp dir and swap in-place by SamMorrowDrums · Pull Request #13449 · cli/cli

SamMorrowDrums · 2026-05-18T13:09:08Z

Problem

gh skill update could relocate skills and delete the original install directory:

With --dir, namespaced layouts ({dir}/{namespace}/{name}/SKILL.md) had their install base computed by walking filepath.Dir twice, putting the new install one level too shallow. The post-install migration step then RemoveAlled the original namespaced directory, taking the user's files with it.
Without --dir, the same migration cleanup quietly relocated namespaced skills found under an agent install directory to a flat layout, even though users expect updates to land where the skill currently lives.

Either path breaks symlinks, mounts, and any external references that point at the skill directory.

Fix

Each update is staged into a sibling directory of the existing skill dir (same filesystem, so renames are atomic). On success the contents are swapped in via per-entry rename:

Move every existing entry from the skill dir into a sibling backup dir.
Move every staged entry into the skill dir.
Remove the backup.

If any step fails, the rollback path removes any freshly installed entries and moves the originals back from the backup.

This guarantees:

All updates happen in-place, regardless of layout (flat vs. namespaced) and regardless of whether the skill was found via an agent host or via --dir.
The skill directory's inode is preserved, so symlinks, mounts, and external references continue to resolve.
Stale files from the previous version are removed.
A failure at any point (install, read, rename) leaves the existing skill completely untouched.

Tests

New acceptance test acceptance/testdata/skills/skills-update-inplace.txtar plants a namespaced skill, runs gh skill update --dir, and asserts the skill remains at the original namespaced path with no flat-path duplicate.
New unit test TestSwapDirectoryContents_PreservesDestInode asserts the skill directory's identity is preserved across a successful swap (via os.SameFile).
New unit test TestSwapDirectoryContents_RollsBackOnFailure exercises the rollback path and asserts the original content (including nested subdirs) is fully restored.
The "namespaced skill with --dir" unit test is updated to assert in-place behavior and stale-file cleanup.
The existing install_failure_during_update_reports_error_and_continues unit test had a harness shortcut that silently skipped its verify block whenever wantErr was set. That shortcut is removed so the "preserve original on install failure" assertion is now actually enforced.

Previously, `gh skill update --dir` walked two directory levels up for namespaced skill layouts when computing the install base, causing the skill to be relocated and the original directory to be removed by the post-install migration step. The same migration also relocated namespaced skills found under agent install directories during a normal update, even though users expect updates to land in the same place the skill was discovered. Update now stages each install into a private temp directory and, on success, atomically swaps the contents into the existing skill directory. This: - Always updates in-place, regardless of whether the skill lives under an agent host directory or a custom `--dir`, and regardless of whether the layout is flat or namespaced. - Preserves the skill directory's inode so symlinks, mounts, and external references continue to resolve. - Removes stale files left over from the previous version. - Leaves the existing skill completely untouched if the install fails partway through. The previous test harness silently skipped `verify` whenever `wantErr` was set, which masked the existing failure-preservation assertion. That harness shortcut is removed so the assertion is enforced. Adds an acceptance test that plants a namespaced skill, runs an update with `--dir`, and asserts the skill remains at its original namespaced path with no flat-path duplicate. Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Fixes gh skill update so updates are applied in-place (preserving the existing skill directory) by staging installs in a temporary directory and then replacing the contents of the installed skill directory, preventing accidental relocation/deletion of namespaced installs (closes #13370).

Changes:

Reworks update flow to stage an updated skill in a temp directory and then copy/move files into the existing install directory (no directory relocation/removal).
Updates unit tests to assert in-place behavior (including stale-file cleanup) and ensures error-path verify blocks actually run.
Adds an acceptance test covering in-place updates for namespaced skills when using --dir.

Show a summary per file

File	Description
pkg/cmd/skills/update/update.go	Replaces “install then possibly delete/migrate” logic with a staging + in-place content replacement approach.
pkg/cmd/skills/update/update_test.go	Updates expectations for in-place behavior; fixes test harness so `verify` runs even when `wantErr` is set.
acceptance/testdata/skills/skills-update-inplace.txtar	New acceptance test ensuring namespaced `--dir` updates don’t relocate or delete the original directory.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (3)

pkg/cmd/skills/update/update.go:469

The implementation deletes the existing contents of u.local.dir before moving/copying staged files in. If RemoveAll or any subsequent moveOrCopy/copyPath call fails partway through (permissions, disk full, interrupted copy, etc.), the skill directory can be left partially updated or empty, which contradicts the function contract/comments (“failed install leaves the existing skill completely untouched”) and the PR description. To make this failure-safe, consider a transactional approach (e.g., rename existing entries into a temporary backup dir first, populate from staging, and restore from backup on error; ideally stage on the same filesystem so renames can be atomic).

	if err := os.MkdirAll(u.local.dir, 0o755); err != nil {
		return fmt.Errorf("could not ensure skill directory %s: %w", u.local.dir, err)
	}
	existing, err := os.ReadDir(u.local.dir)
	if err != nil {
		return fmt.Errorf("could not read skill directory %s: %w", u.local.dir, err)
	}
	for _, entry := range existing {
		if err := os.RemoveAll(filepath.Join(u.local.dir, entry.Name())); err != nil {
			return fmt.Errorf("could not clean skill directory %s: %w", u.local.dir, err)
		}
	}

	staged, err := os.ReadDir(stagedSkillDir)
	if err != nil {
		return fmt.Errorf("could not read staged skill directory %s: %w", stagedSkillDir, err)
	}
	for _, entry := range staged {
		src := filepath.Join(stagedSkillDir, entry.Name())
		dst := filepath.Join(u.local.dir, entry.Name())
		if err := moveOrCopy(src, dst); err != nil {
			return fmt.Errorf("could not move %s into place: %w", entry.Name(), err)
		}
	}

pkg/cmd/skills/update/update.go:481

moveOrCopy falls back to copyPath for any os.Rename error. That can mask real failures (e.g., permission denied, destination already exists, invalid path) and may produce partial copies while still discarding the original error context. It’s safer to only fall back for cross-device rename failures (EXDEV) and otherwise return the rename error.

// moveOrCopy renames src to dst, falling back to a recursive copy when the
// rename crosses filesystem boundaries (e.g. when TMPDIR lives on a separate
// volume from the skill directory).
func moveOrCopy(src, dst string) error {
	if err := os.Rename(src, dst); err == nil {
		return nil
	}
	return copyPath(src, dst)
}

pkg/cmd/skills/update/update.go:515

copyPath reads entire files into memory via os.ReadFile before writing. If a skill includes large files, this can cause unnecessary memory spikes. Consider switching to streaming copy (open src/dst and io.Copy) while still applying the desired permissions.

	default:
		data, err := os.ReadFile(src)
		if err != nil {
			return err
		}
		return os.WriteFile(dst, data, info.Mode().Perm())
	}

Files reviewed: 3/3 changed files
Comments generated: 1

Address review feedback on the in-place update implementation: - Stage into a sibling of the existing skill directory so every rename during the swap is intra-filesystem and atomic. This eliminates the cross-device copy fallback and the associated memory-spike concern (no more os.ReadFile/os.WriteFile of arbitrary skill files). - Replace the "clear-then-fill" sequence with a backup-and-swap: move existing entries into a sibling backup dir, then move staged entries into place. If any step fails, restore from backup. This makes the "failed install leaves the existing skill untouched" guarantee hold even when the failure occurs mid-swap rather than only during install. - Drop the moveOrCopy/copyPath helpers entirely. Adds two unit tests: TestSwapDirectoryContents_PreservesDestInode asserts the directory's identity is preserved across a successful swap, and TestSwapDirectoryContents_RollsBackOnFailure exercises the rollback path by pointing the swap at a non-existent staged dir, verifying original content (including nested subdirs) is fully restored. Co-authored-by: Copilot <[email protected]>

Match prior-art style: leading docstring on helper tests (see TestDeduplicateByName_Namespaced) and use assert.Empty for clearer failure output instead of a bare t.Errorf in a loop. Co-authored-by: Copilot <[email protected]>

tommaso-moro

lgtm

BagToad

LGTM

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.94.0` → `v2.96.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.96.0`](https://github.com/cli/cli/releases/tag/v2.96.0): GitHub CLI 2.96.0 [Compare Source](cli/cli@v2.95.0...v2.96.0) #### Security A security vulnerability has been identified, and fixed, that could allow command execution on a user's computer when connecting to a malicious Codespace via `gh codespace jupyter`. Users of `gh codespace jupyter` are advised to update gh to version v2.96.0 as soon as possible. For more information see: <GHSA-8cg3-r6g9-fpg2> #### Download release assets without authentication `gh release download` now works against public repositories without authentication, matching `gh extension install`. A token is still used when one is present: ```shell # Download assets from a public repository, no login required gh release download v2.96.0 --repo cli/cli ``` #### What's Changed ##### ✨ Features - Allow `gh release download` without authentication on public repositories by [@BagToad](https://github.com/BagToad) in [#13723](cli/cli#13723) - Detect additional third-party coding agents by [@BagToad](https://github.com/BagToad) in [#13722](cli/cli#13722) - Support `antigravity-cli` and `antigravity2.0` in `gh skill` by [@BagToad](https://github.com/BagToad) in [#13784](cli/cli#13784) ##### 🐛 Fixes - fix: show checks summary when all checks were cancelled by [@s3onghyun](https://github.com/s3onghyun) in [#13679](cli/cli#13679) - fix(skills): install universal agent to `~/.agents/skills` by [@toller892](https://github.com/toller892) in [#13681](cli/cli#13681) - fix(skills): honor `--dir` without agent prompt by [@happysnaker](https://github.com/happysnaker) in [#13766](cli/cli#13766) - Fix concurrent map writes in codespace port forwarding by [@williammartin](https://github.com/williammartin) in [#13313](cli/cli#13313) - Use `int64` for GitHub database IDs by [@williammartin](https://github.com/williammartin) in [#13403](cli/cli#13403) ##### 📚 Docs & Chores - Pin reusable triage workflows to a commit SHA by [@BagToad](https://github.com/BagToad) in [#13705](cli/cli#13705) - Add security disclosure guidance to `AGENTS.md` by [@BagToad](https://github.com/BagToad) in [#13720](cli/cli#13720) - Clarify `--clone` boolean flag behaviour in `gh repo fork` help by [@BagToad](https://github.com/BagToad) in [#13786](cli/cli#13786) - Fix flaky `TestHuhPrompterMultiSelectWithSearchPersistence` on slow architectures by [@pdostal](https://github.com/pdostal) in [#13675](cli/cli#13675) - docs(search): add examples for multiple qualifiers by [@happysnaker](https://github.com/happysnaker) in [#13756](cli/cli#13756) - docs: fix broken anchor link in release-process-deep-dive by [@patrickwehbe](https://github.com/patrickwehbe) in [#13688](cli/cli#13688) - docs: fix broken install command and link/grammar errors by [@patrickwehbe](https://github.com/patrickwehbe) in [#13690](cli/cli#13690) - docs: fix duplicated word in primer README by [@s3onghyun](https://github.com/s3onghyun) in [#13677](cli/cli#13677) #####

Dependencies - chore(deps): bump github.com/microsoft/dev-tunnels from 0.1.19 to 0.1.27 by [@dependabot](https://github.com/dependabot) in [#13708](cli/cli#13708) - chore(deps): bump actions/checkout from 6.0.3 to 7.0.0 by [@dependabot](https://github.com/dependabot) in [#13703](cli/cli#13703) - chore(deps): bump github.com/google/go-containerregistry from 0.21.6 to 0.21.7 by [@dependabot](https://github.com/dependabot) in [#13702](cli/cli#13702) - chore(deps): bump actions/setup-go from 6.4.0 to 6.5.0 by [@dependabot](https://github.com/dependabot) in [#13740](cli/cli#13740) - chore(deps): bump actions/attest from 4.1.0 to 4.1.1 by [@dependabot](https://github.com/dependabot) in [#13754](cli/cli#13754) - chore(deps): bump goreleaser/goreleaser-action from 7.2.2 to 7.2.3 by [@dependabot](https://github.com/dependabot) in [#13759](cli/cli#13759) - chore(deps): bump golangci/golangci-lint-action from 9.2.1 to 9.3.0 by [@dependabot](https://github.com/dependabot) in [#13779](cli/cli#13779) #### New Contributors - [@patrickwehbe](https://github.com/patrickwehbe) made their first contribution in [#13688](cli/cli#13688) - [@s3onghyun](https://github.com/s3onghyun) made their first contribution in [#13679](cli/cli#13679) - [@toller892](https://github.com/toller892) made their first contribution in [#13681](cli/cli#13681) - [@happysnaker](https://github.com/happysnaker) made their first contribution in [#13756](cli/cli#13756) **Full Changelog**: <cli/cli@v2.95.0...v2.96.0> ### [`v2.95.0`](https://github.com/cli/cli/releases/tag/v2.95.0): GitHub CLI 2.95.0 [Compare Source](cli/cli@v2.94.0...v2.95.0) #### Read repository files and directories with `gh repo read-file` and `gh repo read-dir` Two new preview commands read repository contents without cloning: ```shell # Read a single file to stdout gh repo read-file README.md --repo cli/cli # Read from a specific branch, tag, or commit gh repo read-file go.mod --ref v2.94.0 --repo cli/cli # Write a file to disk (use --clobber to overwrite) gh repo read-file README.md --output ./README.md --repo cli/cli # List the entries in a directory gh repo read-dir script --repo cli/cli ``` Both commands default to the repository's default branch, accept `--ref` to target any branch, tag, or commit, and support `--json`, `--jq`, and `--template` for scripting. This makes it easy for agents and automation to inspect a repo without a full checkout. > \[!NOTE] > `gh repo read-file` and `gh repo read-dir` are in preview and subject to change without notice. #### What's Changed ##### ✨ Features - feat: add `repo read-file` and `repo read-dir` by [@babakks](https://github.com/babakks) in [#13580](cli/cli#13580) - feat(skills): list available skills when install runs non-interactively by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13548](cli/cli#13548) - Support custom CLAUDE\_CONFIG\_DIR in install by [@tommaso-moro](https://github.com/tommaso-moro) in [#13523](cli/cli#13523) ##### 🐛 Fixes - fix(skills): stage updates in a temp dir and swap in-place by [@SamMorrowDrums](https://github.com/SamMorrowDrums) in [#13449](cli/cli#13449) ##### 📚 Docs & Chores - Make filtering by bot authors more discoverable by [@BagToad](https://github.com/BagToad) in [#13642](cli/cli#13642) - docs(discussion): polish help docs by [@babakks](https://github.com/babakks) in [#13632](cli/cli#13632) - Bump Go in devcontainer by [@spenserblack](https://github.com/spenserblack) in [#13674](cli/cli#13674) #####

Dependencies - chore(deps): bump golang.org/x/text from 0.37.0 to 0.38.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13640](cli/cli#13640) - chore(deps): bump charm.land/lipgloss/v2 from 2.0.3 to 2.0.4 by [@dependabot](https://github.com/dependabot)\[bot] in [#13663](cli/cli#13663) - chore(deps): bump golang.org/x/term from 0.43.0 to 0.44.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13661](cli/cli#13661) - chore(deps): bump github/codeql-action from 4.36.1 to 4.36.2 by [@dependabot](https://github.com/dependabot)\[bot] in [#13619](cli/cli#13619) - chore(deps): bump github.com/sigstore/sigstore-go from 1.1.4 to 1.2.1 by [@dependabot](https://github.com/dependabot)\[bot] in [#13662](cli/cli#13662) - chore(deps): bump golang.org/x/crypto from 0.52.0 to 0.53.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#13641](cli/cli#13641) **Full Changelog**: <cli/cli@v2.94.0...v2.95.0> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate).

SamMorrowDrums marked this pull request as ready for review May 18, 2026 13:09

SamMorrowDrums requested a review from a team as a code owner May 18, 2026 13:09

Copilot AI review requested due to automatic review settings May 18, 2026 13:09

SamMorrowDrums requested a review from a team as a code owner May 18, 2026 13:09

SamMorrowDrums requested a review from BagToad May 18, 2026 13:09

Copilot started reviewing on behalf of SamMorrowDrums May 18, 2026 13:10 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread pkg/cmd/skills/update/update.go

Copilot started work on behalf of SamMorrowDrums May 18, 2026 13:32 View session

Copilot finished work on behalf of SamMorrowDrums May 18, 2026 13:33

test: add why-comments and tighten leftover assertion

e71b11f

Match prior-art style: leading docstring on helper tests (see TestDeduplicateByName_Namespaced) and use assert.Empty for clearer failure output instead of a bare t.Errorf in a loop. Co-authored-by: Copilot <[email protected]>

tommaso-moro approved these changes May 18, 2026

View reviewed changes

SamMorrowDrums added 3 commits May 26, 2026 14:28

Merge branch 'trunk' into sammorrowdrums/fix-skill-update-in-place

6af3a03

Merge branch 'trunk' into sammorrowdrums/fix-skill-update-in-place

b4e79e7

Merge branch 'trunk' into sammorrowdrums/fix-skill-update-in-place

cbbdb95

BagToad approved these changes Jun 17, 2026

View reviewed changes

BagToad merged commit 01bcd47 into trunk Jun 17, 2026
11 checks passed

BagToad deleted the sammorrowdrums/fix-skill-update-in-place branch June 17, 2026 17:53

BrewTestBot mentioned this pull request Jun 17, 2026

gh 2.95.0 Homebrew/homebrew-core#288534

Merged

harder mentioned this pull request Jun 18, 2026

Adopt gh 2.95.0 skill changes; require gh ≥ 2.95.0 harder/gh-skillview#4

Merged

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(skills): stage updates in a temp dir and swap in-place#13449
BagToad merged 6 commits into
trunkfrom
sammorrowdrums/fix-skill-update-in-place

SamMorrowDrums commented May 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

tommaso-moro left a comment

Uh oh!

BagToad left a comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

SamMorrowDrums commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

tommaso-moro left a comment

Choose a reason for hiding this comment

Uh oh!

BagToad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SamMorrowDrums commented May 18, 2026 •

edited

Loading