From 8e4aa0523e12783de26a4a9123dadd841b229608 Mon Sep 17 00:00:00 2001 From: Julian lechner Date: Wed, 30 Sep 2026 17:07:33 +0200 Subject: [PATCH] fix(install): keep skill backups undiscoverable --- scripts/lib/manifest.ps1 | 46 +++++++++++++++++++++++--- scripts/lib/manifest.sh | 38 +++++++++++++++++++-- scripts/tests/test-managed-install.ps1 | 18 ++++++++++ scripts/tests/test-managed-install.sh | 15 +++++++++ 4 files changed, 110 insertions(+), 7 deletions(-) diff --git a/scripts/lib/manifest.ps1 b/scripts/lib/manifest.ps1 index a3be11b..f042edd 100644 --- a/scripts/lib/manifest.ps1 +++ b/scripts/lib/manifest.ps1 @@ -3,6 +3,14 @@ function Get-ManagedManifestPath { return (Join-Path $Destination '.ai-config-manifest.tsv') } +function Get-ManagedBackupRoot { + param([Parameter(Mandatory=$true)][string]$Destination) + if ((Split-Path $Destination -Leaf) -eq 'skills' -and (Split-Path (Split-Path $Destination -Parent) -Leaf) -eq '.agents') { + return (Resolve-ManagedPath -Destination (Split-Path $Destination -Parent) -Relative '.ai-config-skill-backups') + } + return (Resolve-ManagedPath -Destination $Destination -Relative 'backups') +} + function Resolve-ManagedPath { param([Parameter(Mandatory=$true)][string]$Destination, [Parameter(Mandatory=$true)][string]$Relative) $parts = $Relative -split '/', 0, 'SimpleMatch' @@ -35,6 +43,23 @@ function Get-Sha256HashOfBytes { finally { $sha256.Dispose() } } +function Write-ManagedSkillFile { + param([Parameter(Mandatory=$true)][string]$Path,[Parameter(Mandatory=$true)][byte[]]$Bytes) + $temporary = Join-Path (Split-Path $Path -Parent) ('.ai-config-skill-' + [Guid]::NewGuid().ToString('N') + '.tmp') + $backup = "$temporary.bak" + try { + [System.IO.File]::WriteAllBytes($temporary, $Bytes) + if ([System.IO.File]::Exists($Path)) { + [System.IO.File]::Replace($temporary, $Path, $backup) + } else { + [System.IO.File]::Move($temporary, $Path) + } + } finally { + if ([System.IO.File]::Exists($temporary)) { [System.IO.File]::Delete($temporary) } + if ([System.IO.File]::Exists($backup)) { [System.IO.File]::Delete($backup) } + } +} + function Read-ManagedManifest { param([Parameter(Mandatory=$true)][string]$ManifestPath) $entries = @{} @@ -88,7 +113,16 @@ function Sync-ManagedDestination { $null = Resolve-ManagedPath -Destination $Destination -Relative '.ai-config-manifest.tsv' $oldManifest = Read-ManagedManifest $manifestPath $newManifest = @{} - $backupRoot = Join-Path $Destination "backups/$Stamp" + $backupRoot = Get-ManagedBackupRoot $Destination + $oldBackupRoot = Resolve-ManagedPath -Destination $Destination -Relative 'backups' + if ($backupRoot -ne $oldBackupRoot -and (Test-Path -LiteralPath $oldBackupRoot)) { + if ($DryRun) { + if (-not $Summary) { Write-Output "DRYRUN MOVE $oldBackupRoot -> $backupRoot" } + } else { + New-Item -ItemType Directory -Path $backupRoot -Force | Out-Null + Move-Item -LiteralPath $oldBackupRoot -Destination (Join-Path $backupRoot ('legacy-' + [Guid]::NewGuid().ToString('N'))) + } + } $tally = @{ Created = 0; Updated = 0; Unchanged = 0; Removed = 0; Warned = 0 } Get-ChildItem $Source -File -Recurse | ForEach-Object { @@ -132,14 +166,18 @@ function Sync-ManagedDestination { if ($DryRun) { if (-not $Summary) { Write-Output "DRYRUN $action $target" }; if ($action -eq 'CREATE') { $tally.Created++ } else { $tally.Updated++ }; return } if ($exists) { $wasManaged = $oldManifest.ContainsKey($relative) - $backup = Resolve-ManagedPath -Destination $Destination -Relative "backups/$Stamp/$relative" + $backup = Resolve-ManagedPath -Destination $backupRoot -Relative "$Stamp/$relative" New-Item (Split-Path $backup -Parent) -ItemType Directory -Force | Out-Null Copy-Item -LiteralPath $target $backup -Force if (-not $Summary) { Write-Output "BACKUP $target -> $backup" } if (-not $wasManaged) { Write-Output "WARN $target existed before this installation but was not tracked by a previous run; it was backed up before being overwritten."; $tally.Warned++ } } New-Item (Split-Path $target -Parent) -ItemType Directory -Force | Out-Null - [System.IO.File]::WriteAllBytes($target, $newBytes) + if ($relative -eq 'SKILL.md' -or $relative.EndsWith('/SKILL.md', [StringComparison]::Ordinal)) { + Write-ManagedSkillFile -Path $target -Bytes $newBytes + } else { + [System.IO.File]::WriteAllBytes($target, $newBytes) + } if (-not $Summary) { Write-Output "$action $target" } if ($action -eq 'CREATE') { $tally.Created++ } else { $tally.Updated++ } } @@ -148,7 +186,7 @@ function Sync-ManagedDestination { $target = Resolve-ManagedPath -Destination $Destination -Relative $relative if (-not (Test-Path -LiteralPath $target)) { continue } if ($DryRun) { if (-not $Summary) { Write-Output "DRYRUN REMOVE $target" }; $tally.Removed++; continue } - $backup = Resolve-ManagedPath -Destination $Destination -Relative "backups/$Stamp/$relative" + $backup = Resolve-ManagedPath -Destination $backupRoot -Relative "$Stamp/$relative" New-Item (Split-Path $backup -Parent) -ItemType Directory -Force | Out-Null Copy-Item -LiteralPath $target $backup -Force if (-not $Summary) { Write-Output "BACKUP $target -> $backup" } diff --git a/scripts/lib/manifest.sh b/scripts/lib/manifest.sh index 08917e8..f88bf4f 100755 --- a/scripts/lib/manifest.sh +++ b/scripts/lib/manifest.sh @@ -16,6 +16,15 @@ manifest_path() { printf '%s/.ai-config-manifest.tsv' "$1" } +managed_backup_root() { + local destination=$1 + if [ "${destination##*/}" = skills ] && [ "$(basename -- "$(dirname -- "$destination")")" = .agents ]; then + managed_target "$(dirname -- "$destination")" '.ai-config-skill-backups' + else + managed_target "$destination" backups + fi +} + managed_target() { local destination=$1 relative=$2 component target probe target=$destination @@ -82,6 +91,20 @@ sync_managed_destination() { read_managed_manifest "$manifest" old_manifest || return 1 declare -A new_manifest local created=0 updated=0 unchanged=0 removed=0 warned=0 + local backup_root old_backup_root + backup_root=$(managed_backup_root "$destination") || return 1 + old_backup_root=$(managed_target "$destination" backups) || return 1 + if [ "$backup_root" != "$old_backup_root" ] && [ -e "$old_backup_root" ]; then + if [ "$dry_run" = true ]; then + [ "$summary" = true ] || printf 'DRYRUN MOVE %s -> %s\n' "$old_backup_root" "$backup_root" + else + mkdir -p -- "$backup_root" + local legacy_target + legacy_target="$backup_root/legacy-$RANDOM-$RANDOM" + while [ -e "$legacy_target" ]; do legacy_target="$backup_root/legacy-$RANDOM-$RANDOM"; done + mv -- "$old_backup_root" "$legacy_target" || return 1 + fi + fi while IFS= read -r -d '' source_file; do local relative target content needs_sub new_hash exists current_hash action backup @@ -143,7 +166,7 @@ sync_managed_destination() { fi if [ "$exists" = true ]; then - backup=$(managed_target "$destination" "backups/$stamp/$relative") || return 1 + backup=$(managed_target "$backup_root" "$stamp/$relative") || return 1 mkdir -p "$(dirname -- "$backup")" cp -- "$target" "$backup" [ "$summary" = true ] || printf 'BACKUP %s -> %s\n' "$target" "$backup" @@ -153,7 +176,16 @@ sync_managed_destination() { fi fi mkdir -p "$(dirname -- "$target")" - if [ "$needs_sub" = true ]; then + if [[ "$relative" = SKILL.md || "$relative" = */SKILL.md ]]; then + local temporary + temporary=$(mktemp "$(dirname -- "$target")/.ai-config-skill.XXXXXX") || return 1 + if [ "$needs_sub" = true ]; then + printf '%s' "$content" > "$temporary" || { rm -f -- "$temporary"; return 1; } + else + cp -- "$source_file" "$temporary" || { rm -f -- "$temporary"; return 1; } + fi + mv -f -- "$temporary" "$target" || { rm -f -- "$temporary"; return 1; } + elif [ "$needs_sub" = true ]; then printf '%s' "$content" > "$target" else cp -- "$source_file" "$target" @@ -172,7 +204,7 @@ sync_managed_destination() { removed=$((removed + 1)) continue fi - backup=$(managed_target "$destination" "backups/$stamp/$relative") || return 1 + backup=$(managed_target "$backup_root" "$stamp/$relative") || return 1 mkdir -p "$(dirname -- "$backup")" cp -- "$target" "$backup" [ "$summary" = true ] || printf 'BACKUP %s -> %s\n' "$target" "$backup" diff --git a/scripts/tests/test-managed-install.ps1 b/scripts/tests/test-managed-install.ps1 index 65be785..acef5b7 100644 --- a/scripts/tests/test-managed-install.ps1 +++ b/scripts/tests/test-managed-install.ps1 @@ -89,8 +89,16 @@ try { } $targets = @(Get-InstallTargets -Generated $generated -HomePath $testHome -Shell PowerShell -Client Codex) if ($targets[0].Destination -ne (Join-Path $testHome '.agents/skills')) { throw 'Canonical skills must be installed before legacy copies are removed.' } + $oldSkillBackup = Join-Path $testHome '.agents/skills/backups/old/example' + New-Item $oldSkillBackup -ItemType Directory -Force | Out-Null + Set-Content (Join-Path $oldSkillBackup 'SKILL.md') 'old backup' -NoNewline + $null = Sync-ManagedDestination -Source $targets[0].Source -Destination $targets[0].Destination -Stamp 'migrate-dry' -DryRun + if (-not (Test-Path (Join-Path $oldSkillBackup 'SKILL.md'))) { throw 'Dry run moved an old skill backup.' } foreach ($item in $targets) { $null = Sync-ManagedDestination -Source $item.Source -Destination $item.Destination -Stamp 'migrate' } if (-not (Test-Path (Join-Path $testHome '.agents/skills/example/SKILL.md'))) { throw 'Canonical skill is missing.' } + if (Test-Path (Join-Path $testHome '.agents/skills/backups')) { throw 'Old skill backups must leave the discoverable skill tree.' } + $skillBackupRoot = Join-Path $testHome '.agents/.ai-config-skill-backups' + if (@(Get-ChildItem -LiteralPath $skillBackupRoot -Filter SKILL.md -Recurse -File | Where-Object { (Get-Content -LiteralPath $_.FullName -Raw) -eq 'old backup' }).Count -ne 1) { throw 'Old skill backup was not preserved outside the skill tree.' } if (Test-Path (Join-Path $legacy 'skills/example/SKILL.md')) { throw 'Unmodified managed skill must not remain duplicated.' } if (-not (Test-Path (Join-Path $legacy 'backups/migrate/skills/example/SKILL.md'))) { throw 'Removed legacy skill must have a backup.' } foreach ($name in @('custom.md', 'foreign.md')) { @@ -98,6 +106,16 @@ try { } foreach ($item in $targets) { $null = Sync-ManagedDestination -Source $item.Source -Destination $item.Destination -Stamp 'migrate-repeat' } if (Test-Path (Join-Path $legacy 'backups/migrate-repeat')) { throw 'Skill migration must be idempotent.' } + Set-Content (Join-Path $package 'skills/example/SKILL.md') 'updated managed skill' -NoNewline + foreach ($item in $targets) { $null = Sync-ManagedDestination -Source $item.Source -Destination $item.Destination -Stamp 'skill-update' } + $installedSkill = Join-Path $testHome '.agents/skills/example/SKILL.md' + if ((Get-Content -LiteralPath $installedSkill -Raw) -ne 'updated managed skill') { throw 'Managed skill update did not replace the file.' } + if ((Get-Content -LiteralPath (Join-Path $skillBackupRoot 'skill-update/example/SKILL.md') -Raw) -ne 'managed skill') { throw 'New skill backup must be outside the skill tree.' } + if (@(Get-ChildItem -LiteralPath (Split-Path $installedSkill -Parent) -Filter '.ai-config-skill-*' -File).Count) { throw 'Managed skill update left a temporary file.' } + Remove-Item -LiteralPath (Join-Path $package 'skills/example/SKILL.md') -Force + $null = Sync-ManagedDestination -Source $targets[0].Source -Destination $targets[0].Destination -Stamp 'skill-remove' + if (Test-Path -LiteralPath $installedSkill) { throw 'Deselected managed skill must be removed.' } + if ((Get-Content -LiteralPath (Join-Path $skillBackupRoot 'skill-remove/example/SKILL.md') -Raw) -ne 'updated managed skill') { throw 'Removed skill backup must be outside the skill tree.' } Set-Content (Join-Path $source 'concurrency.txt') '__CLAUDE_CODE_MAX_TOOL_USE_CONCURRENCY__' -NoNewline $null = Sync-ManagedDestination -Source $source -Destination $destination -Stamp 'concurrency' -ClaudeConcurrency '7' if ((Get-Content (Join-Path $destination 'concurrency.txt') -Raw) -ne '7') { throw 'Standalone concurrency placeholder must be resolved.' } diff --git a/scripts/tests/test-managed-install.sh b/scripts/tests/test-managed-install.sh index 67ea61b..b93ddb4 100644 --- a/scripts/tests/test-managed-install.sh +++ b/scripts/tests/test-managed-install.sh @@ -108,15 +108,30 @@ legacy_manifest[skills/example/custom.md]=$(printf '0%.0s' {1..64}) write_managed_manifest "$(manifest_path "$legacy")" legacy_manifest mapfile -t targets < <(get_install_targets "$generated" "$test_home" bash codex) [[ "${targets[0]}" = *"|$test_home/.agents/skills" ]] || { printf 'Canonical skills must be installed first.\n' >&2; exit 1; } +mkdir -p "$test_home/.agents/skills/backups/old/example" +printf 'old backup' > "$test_home/.agents/skills/backups/old/example/SKILL.md" +sync_managed_destination "${targets[0]%%|*}" "${targets[0]#*|}" migrate-dry "" "" "" true > /dev/null +[ -f "$test_home/.agents/skills/backups/old/example/SKILL.md" ] || { printf 'Dry run moved an old skill backup.\n' >&2; exit 1; } for target_pair in "${targets[@]}"; do sync_managed_destination "${target_pair%%|*}" "${target_pair#*|}" migrate "" "" "" false > /dev/null done [ -f "$test_home/.agents/skills/example/SKILL.md" ] && [ ! -f "$legacy/skills/example/SKILL.md" ] && [ -f "$legacy/backups/migrate/skills/example/SKILL.md" ] || { printf 'Skill migration must remove and back up the managed duplicate.\n' >&2; exit 1; } +[ ! -e "$test_home/.agents/skills/backups" ] && [ "$(find "$test_home/.agents/.ai-config-skill-backups" -name SKILL.md -type f -exec grep -l '^old backup$' {} + | wc -l)" -eq 1 ] || { printf 'Old skill backup must be preserved outside the skill tree.\n' >&2; exit 1; } [ -f "$legacy/skills/example/custom.md" ] && [ -f "$legacy/skills/example/foreign.md" ] || { printf 'Skill migration must preserve modified and foreign files.\n' >&2; exit 1; } for target_pair in "${targets[@]}"; do sync_managed_destination "${target_pair%%|*}" "${target_pair#*|}" migrate-repeat "" "" "" false > /dev/null done [ ! -d "$legacy/backups/migrate-repeat" ] || { printf 'Skill migration must be idempotent.\n' >&2; exit 1; } +printf 'updated managed skill' > "$package/skills/example/SKILL.md" +for target_pair in "${targets[@]}"; do + sync_managed_destination "${target_pair%%|*}" "${target_pair#*|}" skill-update "" "" "" false > /dev/null +done +[ "$(cat "$test_home/.agents/skills/example/SKILL.md")" = 'updated managed skill' ] || { printf 'Managed skill update did not replace the file.\n' >&2; exit 1; } +[ "$(cat "$test_home/.agents/.ai-config-skill-backups/skill-update/example/SKILL.md")" = 'managed skill' ] || { printf 'New skill backup must be outside the skill tree.\n' >&2; exit 1; } +[ -z "$(find "$test_home/.agents/skills/example" -maxdepth 1 -name '.ai-config-skill.*' -print -quit)" ] || { printf 'Managed skill update left a temporary file.\n' >&2; exit 1; } +rm -f -- "$package/skills/example/SKILL.md" +sync_managed_destination "${targets[0]%%|*}" "${targets[0]#*|}" skill-remove "" "" "" false > /dev/null +[ ! -f "$test_home/.agents/skills/example/SKILL.md" ] && [ "$(cat "$test_home/.agents/.ai-config-skill-backups/skill-remove/example/SKILL.md")" = 'updated managed skill' ] || { printf 'Deselected skill must be removed and backed up outside the skill tree.\n' >&2; exit 1; } printf '__CLAUDE_CODE_MAX_TOOL_USE_CONCURRENCY__' > "$source_dir/concurrency.txt" sync_managed_destination "$source_dir" "$destination" concurrency "" "" "" false false true 7 > /dev/null [ "$(cat "$destination/concurrency.txt")" = 7 ] || { printf 'Standalone concurrency placeholder must be resolved.\n' >&2; exit 1; }