diff --git a/build/npm/install.js b/build/npm/install.js index ac74bb8d..cb53ea56 100755 --- a/build/npm/install.js +++ b/build/npm/install.js @@ -619,11 +619,11 @@ function backupAndRemoveSkillDir(homeDir, dir, backups = null, options = {}) { // leave an unmarked (never-prunable) stamp behind. writeSkillBackupMarker(targetRoot); } catch (err) { - // Never leave an empty unowned stamp root behind. The removal is - // deliberately non-recursive so a pre-existing non-empty root (foreign - // data) is never destroyed. + // Never leave an empty unowned stamp root behind. rmdirSync removes an + // empty root but deliberately fails on a non-empty one, so a pre-existing + // root holding foreign data is never destroyed. try { - fs.rmSync(targetRoot, { force: true }); + fs.rmdirSync(targetRoot); } catch { // Non-empty pre-existing root: foreign data stays. } @@ -1041,8 +1041,14 @@ function rollbackPublishedSkillPath(publication, options = {}) { } } catch (err) { if (err && err.code === "ENOENT") { - cleanupRoot(); - return; + // ENOENT from the fingerprint walk means the destination itself is + // gone only when it no longer exists lexically; a concurrently removed + // CHILD produces the same error and must fall through to the refusal + // below instead of skipping rollback. + if (!pathExistsLexicallySync(destination)) { + cleanupRoot(); + return; + } } if (String(err.message || "").startsWith("refusing to delete non-transaction Skill") || String(err.message || "").startsWith("failed to clean Skill rollback quarantine")) { diff --git a/internal/upgrade/paths.go b/internal/upgrade/paths.go index 88df2324..57233a7f 100644 --- a/internal/upgrade/paths.go +++ b/internal/upgrade/paths.go @@ -233,11 +233,30 @@ func backupAndRemoveSkillDir(homeDir, dir string) (string, error) { } } rememberSkillBackupRoot(backupRoot) + // The stamp only has second precision and the collision loop dedupes on + // the payload target, not on the root, so a second skill dir backed up in + // the same second shares this root. Record whether this call created the + // root: marker-failure cleanup must never delete a root that may already + // hold a sibling's moved payload — its original is gone, so that loss + // would be unrecoverable. + fresh := false + if _, statErr := upgradeLstat(backupRoot); statErr != nil && os.IsNotExist(statErr) { + fresh = true + } if err := upgradeMkdirAll(backupRoot, dirPermShared); err != nil { return "", fmt.Errorf("创建备份目录失败 %s: %w", backupRoot, err) } if err := writeSkillBackupMarker(backupRoot); err != nil { - _ = upgradeRemoveAll(backupRoot) + // Clean up only a root this call created (empty by construction) and + // only non-recursively: os.Remove refuses a non-empty root, which is + // the intended guard against a concurrent writer or a sibling payload + // already moved into a shared root. A pre-existing root is foreign + // from this call's perspective and is left untouched. The victim was + // never moved, so it stays in place. + if fresh { + _ = os.Remove(filepath.Join(backupRoot, skillBackupMarkerFile)) + _ = os.Remove(backupRoot) + } return "", fmt.Errorf("写入备份所有权标记失败 %s: %w", backupRoot, err) } if err := moveSkillPathRecoverably(dir, target); err != nil { diff --git a/internal/upgrade/paths_multi_test.go b/internal/upgrade/paths_multi_test.go index 2790bdf3..58f2e660 100644 --- a/internal/upgrade/paths_multi_test.go +++ b/internal/upgrade/paths_multi_test.go @@ -964,6 +964,54 @@ func TestCrossPlatformCoverageSkillBackupMarkerOwnership(t *testing.T) { t.Fatalf("victim must stay in place: %v", statErr) } }) + + t.Run("marker write failure preserves a sibling backup in the same stamp", func(t *testing.T) { + home := t.TempDir() + // Pin one stamp: a second skill dir backed up in the same second + // shares the root with the first (collision dedupe is per payload). + testseam.Swap(t, &upgradeBackupStamp, func() string { return "20260818-010101" }) + newVictim := func(name, body string) string { + victim := filepath.Join(home, ".claude", "skills", name) + if err := os.MkdirAll(victim, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(victim, "SKILL.md"), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return victim + } + victimA := newVictim("dingtalk-a", "payload-a") + victimB := newVictim("dingtalk-b", "payload-b") + backupA, err := backupAndRemoveSkillDir(home, victimA) + if err != nil { + t.Fatal(err) + } + root := filepath.Dir(backupA) + + testseam.Swap(t, &upgradeWriteFile, func(string, []byte, os.FileMode) error { return errors.New("write denied") }) + backupB, err := backupAndRemoveSkillDir(home, victimB) + testseam.Swap(t, &upgradeWriteFile, os.WriteFile) + if err == nil || !strings.Contains(err.Error(), "备份所有权标记") { + t.Fatalf("marker write failure must fail the backup, got %v", err) + } + if backupB != "" { + t.Fatalf("failed backup must not return a path: %q", backupB) + } + // The root existed before B's call, so its contents — including A's + // already-moved payload, the only remaining copy — must be untouched. + if body, readErr := os.ReadFile(filepath.Join(backupA, "SKILL.md")); readErr != nil || string(body) != "payload-a" { + t.Fatalf("sibling backup must survive, body = %q, err = %v", body, readErr) + } + if _, statErr := os.Stat(root); statErr != nil { + t.Fatalf("stamp root must survive: %v", statErr) + } + if body, readErr := os.ReadFile(filepath.Join(root, skillBackupMarkerFile)); readErr != nil || string(body) != skillBackupMarkerBody { + t.Fatalf("ownership marker must survive, body = %q, err = %v", body, readErr) + } + if body, readErr := os.ReadFile(filepath.Join(victimB, "SKILL.md")); readErr != nil || string(body) != "payload-b" { + t.Fatalf("victim B must stay in place, body = %q, err = %v", body, readErr) + } + }) } func TestCrossPlatformCoveragePruneSkillBackupsEdges(t *testing.T) { diff --git a/scripts/install-devapp.sh b/scripts/install-devapp.sh index d5716fd5..4859dcf7 100755 --- a/scripts/install-devapp.sh +++ b/scripts/install-devapp.sh @@ -72,7 +72,13 @@ copy_tree() { fi for _ct_child in "$dest"/* "$dest"/.[!.]* "$dest"/..?*; do [ -e "$_ct_child" ] || [ -L "$_ct_child" ] || continue - mv "$_ct_child" "$stage/" || true + # Bounded failure like install.sh's publish rollback: a child that + # cannot move back leaves dest occupied; stop instead of swallowing + # the error so the restore below reports the failed rollback. + if ! mv "$_ct_child" "$stage/"; then + _ct_failed=1 + break + fi done rmdir "$dest" 2>/dev/null || true fi @@ -148,12 +154,24 @@ backup_skill_dir() { fi done record_current_run_backup_stamp "$backup_root" + # Freshness must be sampled before mkdir: the collision loop tests the + # payload path, so a second backup in the same stamp second reuses this + # root while a sibling payload from this run already lives in it. + fresh=1 + [ ! -d "$backup_root" ] || fresh=0 mkdir -p "$backup_root" || return 1 # Ownership proof, the exact bytes Go's writeSkillBackupMarker stamps: a # stamp-shaped name alone is not evidence, so pruning only ever removes # roots carrying this marker — it must exist before the payload moves in. printf '%s\n' 'dws skill backup v1' > "$backup_root/.dws-skill-backup" || { - rm -rf "$backup_root" + # Non-recursive cleanup, sibling protection: only a root this call + # created may be dropped, and only the marker plus an empty root (rmdir + # refuses a non-empty directory) — rm -rf here would destroy a completed + # same-second sibling backup whose original is already moved away. + if [ "$fresh" -eq 1 ]; then + rm -f "$backup_root/.dws-skill-backup" + rmdir "$backup_root" 2>/dev/null || true + fi return 1 } mv "$victim" "$backup" || return 1 diff --git a/scripts/install-event.sh b/scripts/install-event.sh index 0665461d..c367edbb 100755 --- a/scripts/install-event.sh +++ b/scripts/install-event.sh @@ -137,7 +137,13 @@ copy_tree() { fi for _ct_child in "$dest"/* "$dest"/.[!.]* "$dest"/..?*; do [ -e "$_ct_child" ] || [ -L "$_ct_child" ] || continue - mv "$_ct_child" "$stage/" || true + # Bounded failure like install.sh's publish rollback: a child that + # cannot move back leaves dest occupied; stop instead of swallowing + # the error so the restore below reports the failed rollback. + if ! mv "$_ct_child" "$stage/"; then + _ct_failed=1 + break + fi done rmdir "$dest" 2>/dev/null || true fi @@ -217,12 +223,24 @@ backup_skill_dir() { fi done record_current_run_backup_stamp "$backup_root" + # Freshness must be sampled before mkdir: the collision loop tests the + # payload path, so a second backup in the same stamp second reuses this + # root while a sibling payload from this run already lives in it. + fresh=1 + [ ! -d "$backup_root" ] || fresh=0 mkdir -p "$backup_root" || return 1 # Ownership proof, the exact bytes Go's writeSkillBackupMarker stamps: a # stamp-shaped name alone is not evidence, so pruning only ever removes # roots carrying this marker — it must exist before the payload moves in. printf '%s\n' 'dws skill backup v1' > "$backup_root/.dws-skill-backup" || { - rm -rf "$backup_root" + # Non-recursive cleanup, sibling protection: only a root this call + # created may be dropped, and only the marker plus an empty root (rmdir + # refuses a non-empty directory) — rm -rf here would destroy a completed + # same-second sibling backup whose original is already moved away. + if [ "$fresh" -eq 1 ]; then + rm -f "$backup_root/.dws-skill-backup" + rmdir "$backup_root" 2>/dev/null || true + fi return 1 } mv "$victim" "$backup" || return 1 diff --git a/scripts/install-skills.sh b/scripts/install-skills.sh index 820e5880..8a95c930 100755 --- a/scripts/install-skills.sh +++ b/scripts/install-skills.sh @@ -69,6 +69,11 @@ backup_and_remove_skill_dir() { done _bed_stamp_root="$(dirname "$_bed_target")" record_current_run_backup_stamp "$_bed_stamp_root" + # Freshness must be sampled before mkdir: the collision loop tests the + # payload path, so a second backup in the same stamp second reuses this + # root while a sibling payload from this run already lives in it. + _bed_fresh=1 + [ ! -d "$_bed_stamp_root" ] || _bed_fresh=0 mkdir -p "$_bed_stamp_root" 2>/dev/null || { printf ' ⚠️ 无法创建备份目录，保留原目录 %s\n' "$_bed_dir" return 1 @@ -77,7 +82,14 @@ backup_and_remove_skill_dir() { # stamp-shaped name alone is not evidence, so pruning only ever removes # roots carrying this marker — it must exist before any payload moves in. printf '%s\n' 'dws skill backup v1' > "$_bed_stamp_root/.dws-skill-backup" 2>/dev/null || { - rm -rf "$_bed_stamp_root" + # Non-recursive cleanup, sibling protection: only a root this call + # created may be dropped, and only the marker plus an empty root (rmdir + # refuses a non-empty directory) — rm -rf here would destroy a completed + # same-second sibling backup whose original is already moved away. + if [ "$_bed_fresh" -eq 1 ]; then + rm -f "$_bed_stamp_root/.dws-skill-backup" + rmdir "$_bed_stamp_root" 2>/dev/null || true + fi printf ' ⚠️ 无法写入备份所有权标记，保留原目录 %s\n' "$_bed_dir" return 1 } @@ -205,7 +217,10 @@ sha256_stdin() { } # sha256_file : content digest used by skill_dir_content_digest. Never -# emits on failure so the identity fails instead of parsing a wrong token. +# emits on failure. Callers that capture its status directly detect that +# failure; skill_dir_content_digest cannot — see the pipeline caveat there: +# POSIX sh returns only the right-hand pipeline status, so a subshell exit on +# its left side is discarded and digest-failure detection stays best-effort. sha256_file() { if command -v sha256sum >/dev/null 2>&1; then sha256sum "$1" 2>/dev/null | awk '{print $1}' @@ -484,8 +499,11 @@ skill_dir_identity() { # Recursive content digest over a published tree, mirroring the Go # fingerprint: sorted relative paths emit type+permission bits plus either a # content sha256 or the symlink target, and the whole line stream feeds -# sha256_stdin. Digest failures surface as a non-zero identity instead of a -# wrong-but-parseable token. +# sha256_stdin. Failure detection is asymmetric and best-effort: with no hash +# tool installed sha256_stdin itself returns 1 and the identity genuinely +# fails, but an unreadable file only exits the left-hand subshell — /bin/sh +# has no pipefail, so the pipeline returns sha256_stdin's status and the +# digest is computed over a truncated stream instead of failing. skill_dir_content_digest() { ( cd "$1" 2>/dev/null || exit 1 diff --git a/scripts/install.sh b/scripts/install.sh index 300d5e95..542a1530 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -87,6 +87,11 @@ backup_and_remove_skill_dir() { done _bed_stamp_root="$(dirname "$_bed_target")" record_current_run_backup_stamp "$_bed_stamp_root" + # Freshness must be sampled before mkdir: the collision loop tests the + # payload path, so a second backup in the same stamp second reuses this + # root while a sibling payload from this run already lives in it. + _bed_fresh=1 + [ ! -d "$_bed_stamp_root" ] || _bed_fresh=0 mkdir -p "$_bed_stamp_root" 2>/dev/null || { say " ⚠️ 无法创建备份目录，保留原目录 $_bed_dir" return 1 @@ -95,7 +100,14 @@ backup_and_remove_skill_dir() { # stamp-shaped name alone is not evidence, so pruning only ever removes # roots carrying this marker — it must exist before any payload moves in. printf '%s\n' 'dws skill backup v1' > "$_bed_stamp_root/.dws-skill-backup" 2>/dev/null || { - rm -rf "$_bed_stamp_root" + # Non-recursive cleanup, sibling protection: only a root this call + # created may be dropped, and only the marker plus an empty root (rmdir + # refuses a non-empty directory) — rm -rf here would destroy a completed + # same-second sibling backup whose original is already moved away. + if [ "$_bed_fresh" -eq 1 ]; then + rm -f "$_bed_stamp_root/.dws-skill-backup" + rmdir "$_bed_stamp_root" 2>/dev/null || true + fi say " ⚠️ 无法写入备份所有权标记，保留原目录 $_bed_dir" return 1 } @@ -223,8 +235,10 @@ sha256_stdin() { } # sha256_file : content digest used by skill_dir_content_digest. Never -# emits on failure (the caller's pipeline fails the whole identity instead of -# producing a wrong-but-parseable token). +# emits on failure. Callers that capture its status directly detect that +# failure; skill_dir_content_digest cannot — see the pipeline caveat there: +# POSIX sh returns only the right-hand pipeline status, so a subshell exit on +# its left side is discarded and digest-failure detection stays best-effort. sha256_file() { if need_cmd sha256sum; then sha256sum "$1" 2>/dev/null | awk '{print $1}' @@ -507,8 +521,11 @@ skill_dir_identity() { # the sha256 of the file content or the symlink target. Sorted-path lines are # piped through sha256_stdin, so one opaque token covers renames, mode flips, # content edits and link retargets — the same in-place-edit guarantee the Go -# fingerprint provides. Digest failures (missing hash tool, unreadable file) -# surface as a non-zero identity instead of a wrong-but-parseable token. +# fingerprint provides. Failure detection is asymmetric and best-effort: with +# no hash tool installed sha256_stdin itself returns 1 and the identity +# genuinely fails, but an unreadable file only exits the left-hand subshell — +# /bin/sh has no pipefail, so the pipeline returns sha256_stdin's status and +# the digest is computed over a truncated stream instead of failing. skill_dir_content_digest() { ( cd "$1" 2>/dev/null || exit 1 diff --git a/test/scripts/install_script_test.go b/test/scripts/install_script_test.go index 04d967b0..285694cb 100644 --- a/test/scripts/install_script_test.go +++ b/test/scripts/install_script_test.go @@ -294,6 +294,39 @@ func TestInstallScriptsRemoveJunctionStagesLexically(t *testing.T) { t.Errorf("install-devapp.ps1 missing junction-safe removal contract %q", want) } } + + // The backup prune must also be reparse-safe: a stamp tree can hold + // junction/symlink entries at any depth (victims are collected before + // the physical-equality filter), and Windows PowerShell 5.1 follows + // reparse points during Remove-Item -Recurse. + for scriptName, pruneFn := range map[string]string{ + "install.ps1": "Remove-OldSkillBackups", + "install-devapp.ps1": "Remove-OldDevSkillBackups", + } { + scriptData, err := os.ReadFile(filepath.Join("..", "..", "scripts", scriptName)) + if err != nil { + t.Fatalf("ReadFile(%s) error = %v", scriptName, err) + } + body := extractPowerShellFunction(t, string(scriptData), pruneFn) + if strings.Contains(body, "-Recurse") { + t.Errorf("%s %s must delegate to child-first lexical removal, not Remove-Item -Recurse:\n%s", scriptName, pruneFn, body) + } + } +} + +// extractPowerShellFunction returns a `function Name { ... }` body from a +// PowerShell script, terminated by the next top-level function declaration. +func extractPowerShellFunction(t *testing.T, text, name string) string { + t.Helper() + begin := strings.Index(text, "function "+name) + if begin < 0 { + t.Fatalf("function %s not found", name) + } + end := strings.Index(text[begin+1:], "\nfunction ") + if end < 0 { + end = len(text) - begin - 1 + } + return text[begin : begin+1+end] } func TestInstallEventScriptDegradesFailedAgentLoudly(t *testing.T) { @@ -778,6 +811,364 @@ exit 0 assertSkillBackupOwnershipResult(t, home) } +// lookPowerShellForScripts prefers Windows PowerShell 5.1 (the advertised +// irm|iex surface) on Windows and falls back to pwsh everywhere. +func lookPowerShellForScripts(t *testing.T) string { + t.Helper() + powerShellNames := []string{"pwsh"} + if runtime.GOOS == "windows" { + powerShellNames = []string{"powershell", "pwsh"} + } + for _, name := range powerShellNames { + if candidate, err := exec.LookPath(name); err == nil { + return candidate + } + } + t.Skip("PowerShell is not available") + return "" +} + +// TestInstallPowerShellPruneRemovesLinkChildrenWithoutFollowingTargets pins +// the Blocker 2 fix: pruning a stamp root that contains a junction/symlink +// child must delete the link itself while the link target's contents survive. +// Windows PowerShell 5.1 follows reparse points during Remove-Item -Recurse, +// so the prune removes children lexically, recursing real directories only. +func TestInstallPowerShellPruneRemovesLinkChildrenWithoutFollowingTargets(t *testing.T) { + pwsh := lookPowerShellForScripts(t) + + scriptPath, err := filepath.Abs(filepath.Join("..", "..", "scripts", "install.ps1")) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(scriptPath) + if err != nil { + t.Fatal(err) + } + cut := strings.LastIndex(string(data), "# ── Main") + if cut < 0 { + t.Fatal("install.ps1 main section not found") + } + prefix := strings.ReplaceAll(string(data[:cut]), "$HOME", "$env:DWS_TEST_HOME") + // Junctions need no privilege on Windows but silently no-op on non-Windows + // pwsh builds; symlinks need no privilege on Unix. Exit 4 marks "no link + // flavor testable on this host". + harness := prefix + ` +$backupRoot = Join-Path $env:DWS_TEST_HOME '.dws\skill-backups' +$linkType = if ([System.Environment]::OSVersion.Platform -eq [System.PlatformID]::Win32NT) { 'Junction' } else { 'SymbolicLink' } +$precious = Join-Path $env:DWS_TEST_HOME 'precious-store' +New-Item -ItemType Directory -Path $precious -Force | Out-Null +[System.IO.File]::WriteAllText((Join-Path $precious 'target-data.txt'), 'must survive') +# The oldest of six verified stamps is prunable and holds a link child. +$oldest = Join-Path $backupRoot '20200101-000001' +New-Item -ItemType Directory -Path (Join-Path $oldest 'payload') -Force | Out-Null +[System.IO.File]::WriteAllText((Join-Path $oldest 'payload\SKILL.md'), 'old') +Write-SkillBackupMarker -Root $oldest +$link = Join-Path $oldest 'linked-skill' +New-Item -ItemType $linkType -Path $link -Target $precious -ErrorAction SilentlyContinue | Out-Null +if (!(Test-Path -LiteralPath $link)) { exit 4 } +foreach ($stamp in @('20200101-000002','20200101-000003','20200101-000004','20200101-000005','20200101-000006')) { + $stampRoot = Join-Path $backupRoot $stamp + New-Item -ItemType Directory -Path (Join-Path $stampRoot 'payload') -Force | Out-Null + Write-SkillBackupMarker -Root $stampRoot +} +Remove-OldSkillBackups +if (Test-Path -LiteralPath $oldest) { exit 5 } +if (!(Test-Path -LiteralPath (Join-Path $precious 'target-data.txt') -PathType Leaf)) { exit 6 } +foreach ($stamp in @('20200101-000002','20200101-000003','20200101-000004','20200101-000005','20200101-000006')) { + if (!(Test-Path -LiteralPath (Join-Path $backupRoot $stamp) -PathType Container)) { exit 7 } +} +exit 0 +` + home := filepath.Join(t.TempDir(), "home") + harnessPath := filepath.Join(t.TempDir(), "skill-backup-link-prune.ps1") + mustWriteFile(t, harnessPath, []byte(harness), 0o644) + cmd := exec.Command(pwsh, "-NoProfile", "-NonInteractive", "-File", harnessPath) + cmd.Env = append(os.Environ(), "DWS_TEST_HOME="+home) + output, err := cmd.CombinedOutput() + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 4 { + t.Skipf("no junction/symlink flavor can be created on this host:\n%s", string(output)) + } + t.Fatalf("PowerShell link-aware prune harness failed: %v\n%s", err, string(output)) + } + if got, err := os.ReadFile(filepath.Join(home, "precious-store", "target-data.txt")); err != nil || string(got) != "must survive" { + t.Fatalf("link target content = %q, %v; want it to survive pruning", got, err) + } + entries, err := os.ReadDir(filepath.Join(home, ".dws", "skill-backups")) + if err != nil { + t.Fatal(err) + } + var surviving []string + for _, entry := range entries { + surviving = append(surviving, entry.Name()) + } + want := []string{"20200101-000002", "20200101-000003", "20200101-000004", "20200101-000005", "20200101-000006"} + if len(surviving) != len(want) { + t.Fatalf("after pruning the link-bearing stamp, surviving stamps = %v, want %v", surviving, want) + } + for _, stamp := range want { + if _, err := os.Stat(filepath.Join(home, ".dws", "skill-backups", stamp)); err != nil { + t.Fatalf("stamp %s must survive pruning: %v", stamp, err) + } + } +} + +// TestInstallDevappPowerShellPruneRemovesLinkChildrenWithoutFollowingTargets +// is the install-devapp.ps1 twin of the link-aware prune contract above. +func TestInstallDevappPowerShellPruneRemovesLinkChildrenWithoutFollowingTargets(t *testing.T) { + pwsh := lookPowerShellForScripts(t) + + scriptPath, err := filepath.Abs(filepath.Join("..", "..", "scripts", "install-devapp.ps1")) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(scriptPath) + if err != nil { + t.Fatal(err) + } + cut := strings.Index(string(data), "# Read the releases list") + if cut < 0 { + t.Fatal("install-devapp.ps1 release section not found") + } + prefix := strings.ReplaceAll(string(data[:cut]), "$HOME", "$env:DWS_TEST_HOME") + harness := prefix + ` +$backupRoot = Join-Path $env:DWS_TEST_HOME '.dws\skill-backups' +$linkType = if ([System.Environment]::OSVersion.Platform -eq [System.PlatformID]::Win32NT) { 'Junction' } else { 'SymbolicLink' } +$precious = Join-Path $env:DWS_TEST_HOME 'precious-store' +New-Item -ItemType Directory -Path $precious -Force | Out-Null +[System.IO.File]::WriteAllText((Join-Path $precious 'target-data.txt'), 'must survive') +$oldest = Join-Path $backupRoot '20200101-000001' +New-Item -ItemType Directory -Path (Join-Path $oldest 'payload') -Force | Out-Null +[System.IO.File]::WriteAllText((Join-Path $oldest 'payload\SKILL.md'), 'old') +Write-DevSkillBackupMarker $oldest +$link = Join-Path $oldest 'linked-skill' +New-Item -ItemType $linkType -Path $link -Target $precious -ErrorAction SilentlyContinue | Out-Null +if (!(Test-PathLexically $link)) { exit 4 } +foreach ($stamp in @('20200101-000002','20200101-000003','20200101-000004','20200101-000005','20200101-000006')) { + $stampRoot = Join-Path $backupRoot $stamp + New-Item -ItemType Directory -Path (Join-Path $stampRoot 'payload') -Force | Out-Null + Write-DevSkillBackupMarker $stampRoot +} +Remove-OldDevSkillBackups +if (Test-PathLexically $oldest) { exit 5 } +if (!(Test-PathLexically (Join-Path $precious 'target-data.txt'))) { exit 6 } +foreach ($stamp in @('20200101-000002','20200101-000003','20200101-000004','20200101-000005','20200101-000006')) { + if (!(Test-PathLexically (Join-Path $backupRoot $stamp))) { exit 7 } +} +exit 0 +` + home := filepath.Join(t.TempDir(), "home") + harnessPath := filepath.Join(t.TempDir(), "devapp-skill-backup-link-prune.ps1") + mustWriteFile(t, harnessPath, []byte(harness), 0o644) + cmd := exec.Command(pwsh, "-NoProfile", "-NonInteractive", "-File", harnessPath) + cmd.Env = append(os.Environ(), "DWS_TEST_HOME="+home) + output, err := cmd.CombinedOutput() + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 4 { + t.Skipf("no junction/symlink flavor can be created on this host:\n%s", string(output)) + } + t.Fatalf("PowerShell devapp link-aware prune harness failed: %v\n%s", err, string(output)) + } + if got, err := os.ReadFile(filepath.Join(home, "precious-store", "target-data.txt")); err != nil || string(got) != "must survive" { + t.Fatalf("link target content = %q, %v; want it to survive pruning", got, err) + } + for _, stamp := range []string{"20200101-000002", "20200101-000003", "20200101-000004", "20200101-000005", "20200101-000006"} { + if _, err := os.Stat(filepath.Join(home, ".dws", "skill-backups", stamp)); err != nil { + t.Fatalf("stamp %s must survive pruning: %v", stamp, err) + } + } +} + +// TestInstallPowerShellBackupNeverClaimsUnverifiedStampRoot pins the +// collision-loop fix: a stamp root that exists without the verified +// ownership marker is foreign data, so Backup-SkillDir bumps to a -N root +// instead of writing its marker (and later prune eligibility) into it. +// Foreign roots are seeded for the current second and the next nine so the +// stamp Backup-SkillDir picks is always a pre-existing unverified root. +func TestInstallPowerShellBackupNeverClaimsUnverifiedStampRoot(t *testing.T) { + pwsh := lookPowerShellForScripts(t) + + scriptPath, err := filepath.Abs(filepath.Join("..", "..", "scripts", "install.ps1")) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(scriptPath) + if err != nil { + t.Fatal(err) + } + cut := strings.LastIndex(string(data), "# ── Main") + if cut < 0 { + t.Fatal("install.ps1 main section not found") + } + prefix := strings.ReplaceAll(string(data[:cut]), "$HOME", "$env:DWS_TEST_HOME") + harness := prefix + ` +$backupRoot = Join-Path $env:DWS_TEST_HOME '.dws\skill-backups' +New-Item -ItemType Directory -Path $backupRoot -Force | Out-Null +$now = [DateTime]::UtcNow +$seeded = @() +for ($i = 0; $i -lt 10; $i++) { + $stamp = $now.AddSeconds($i).ToString('yyyyMMdd-HHmmss') + $seeded += $stamp + $foreign = Join-Path $backupRoot $stamp + New-Item -ItemType Directory -Path $foreign -Force | Out-Null + [System.IO.File]::WriteAllText((Join-Path $foreign 'user-data.txt'), 'foreign') +} +$victim = Join-Path $env:DWS_TEST_HOME '.zcode\skills\dingtalk-chat' +New-Item -ItemType Directory -Path $victim -Force | Out-Null +[System.IO.File]::WriteAllText((Join-Path $victim 'SKILL.md'), 'old copy') +$backupPath = '' +if (!(Backup-SkillDir -Dir $victim -BackupPath ([ref]$backupPath))) { exit 2 } +if (!$backupPath) { exit 3 } +foreach ($stamp in $seeded) { + $foreign = Join-Path $backupRoot $stamp + if (Test-Path -LiteralPath (Join-Path $foreign '.dws-skill-backup')) { exit 4 } + $children = @(Get-ChildItem -LiteralPath $foreign -Force | ForEach-Object { $_.Name }) + if ($children.Count -ne 1 -or $children[0] -ne 'user-data.txt') { exit 5 } +} +# Every plain stamp root was foreign, so the payload must sit in a -N root. +if ($backupPath -notmatch '-[0-9]+[\\/]\.zcode-skills-dingtalk-chat$') { exit 6 } +if (!(Test-Path -LiteralPath (Join-Path $backupPath 'SKILL.md') -PathType Leaf)) { exit 7 } +exit 0 +` + home := filepath.Join(t.TempDir(), "home") + harnessPath := filepath.Join(t.TempDir(), "skill-backup-collision.ps1") + mustWriteFile(t, harnessPath, []byte(harness), 0o644) + cmd := exec.Command(pwsh, "-NoProfile", "-NonInteractive", "-File", harnessPath) + cmd.Env = append(os.Environ(), "DWS_TEST_HOME="+home) + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("PowerShell backup collision harness failed: %v\n%s", err, string(output)) + } + // The foreign same-second roots stay untouched: no marker, no payload. + entries, err := os.ReadDir(filepath.Join(home, ".dws", "skill-backups")) + if err != nil { + t.Fatal(err) + } + foreignIntact, bumped := 0, "" + for _, entry := range entries { + children, err := os.ReadDir(filepath.Join(home, ".dws", "skill-backups", entry.Name())) + if err != nil { + t.Fatal(err) + } + hasPayload, hasMarker := false, false + for _, child := range children { + if child.Name() == ".dws-skill-backup" { + hasMarker = true + } + if child.Name() == ".zcode-skills-dingtalk-chat" { + hasPayload = true + } + } + if hasPayload { + bumped = entry.Name() + if !hasMarker { + t.Fatalf("backup stamp root %s must carry the ownership marker", bumped) + } + continue + } + if hasMarker { + t.Fatalf("foreign stamp root %s must never be marked DWS-owned", entry.Name()) + } + if len(children) != 1 || children[0].Name() != "user-data.txt" { + t.Fatalf("foreign stamp root %s must stay untouched, children = %v", entry.Name(), children) + } + foreignIntact++ + } + if foreignIntact != 10 || bumped == "" || !strings.Contains(bumped, "-") { + t.Fatalf("want 10 untouched foreign roots plus one -N backup root, got %d intact and backup root %q", foreignIntact, bumped) + } +} + +// TestInstallDevappPowerShellCrossDeviceRetractVerifiesPublication pins the +// path-blind retract fix: after a cross-device publish, a post-occupy +// verification failure must retract the destination only through +// Remove-VerifiedDevSkillPublication, so a concurrently replaced destination +// is restored instead of deleted. +func TestInstallDevappPowerShellCrossDeviceRetractVerifiesPublication(t *testing.T) { + pwsh, err := exec.LookPath("pwsh") + if err != nil { + if runtime.GOOS == "windows" { + pwsh, err = exec.LookPath("powershell") + } + if err != nil { + t.Skip("PowerShell is not available") + } + } + scriptPath, err := filepath.Abs(filepath.Join("..", "..", "scripts", "install-devapp.ps1")) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(scriptPath) + if err != nil { + t.Fatal(err) + } + cut := strings.Index(string(data), "# Read the releases list") + if cut < 0 { + t.Fatal("install-devapp.ps1 release section not found") + } + root := t.TempDir() + home := filepath.Join(root, "home") + source := filepath.Join(root, "bundle", "dingtalk-misc") + destination := filepath.Join(root, "external", "dingtalk-misc") + if err := os.MkdirAll(home, 0o755); err != nil { + t.Fatal(err) + } + mustWriteFile(t, filepath.Join(source, "SKILL.md"), []byte("new dev skill\n"), 0o640) + + prefix := strings.ReplaceAll(string(data[:cut]), "$HOME", "$env:DWS_TEST_HOME") + prefix += ` +# The first move throws the cross-device error, so the recoverable path +# stages and publishes a copy; right after that publish another process +# replaces the destination, and post-occupy verification fails. +$script:ReplacedPublished = $false +function Move-DevSkillPath([string]$Source, [string]$Destination) { + if ($Source -eq $env:DWS_TEST_SOURCE -and $Destination -eq $env:DWS_TEST_DESTINATION) { + throw [System.IO.IOException]::new("injected cross-device rename", -2147024879) + } + Microsoft.PowerShell.Management\Move-Item -LiteralPath $Source -Destination $Destination -ErrorAction Stop + if (!$script:ReplacedPublished -and $Destination -eq $env:DWS_TEST_DESTINATION) { + $script:ReplacedPublished = $true + Microsoft.PowerShell.Management\Move-Item -LiteralPath $Destination (Join-Path $env:DWS_TEST_HOME 'replaced-published') -ErrorAction Stop + New-Item -ItemType Directory -Path $Destination -Force -ErrorAction Stop | Out-Null + Set-Content -LiteralPath (Join-Path $Destination 'concurrent-user-data.txt') -Value 'keep' + } +} +try { + Move-DevSkillPathRecoverably $env:DWS_TEST_SOURCE $env:DWS_TEST_DESTINATION + exit 2 +} catch { + if ($_.Exception.Message -notmatch 'state uncertain') { Write-Error $_; exit 3 } +} +if (!(Test-PathLexically (Join-Path $env:DWS_TEST_DESTINATION 'concurrent-user-data.txt'))) { exit 4 } +if (!(Test-PathLexically (Join-Path $env:DWS_TEST_SOURCE 'SKILL.md'))) { exit 5 } +exit 0 +` + harnessPath := filepath.Join(t.TempDir(), "install-devapp-retract-verify-harness.ps1") + mustWriteFile(t, harnessPath, []byte(prefix), 0o644) + cmd := exec.Command(pwsh, "-NoProfile", "-NonInteractive", "-File", harnessPath) + cmd.Env = append(os.Environ(), + "DWS_TEST_HOME="+home, + "DWS_TEST_SOURCE="+source, + "DWS_TEST_DESTINATION="+destination, + ) + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("PowerShell devapp verified retract must retain the concurrent replacement: %v\n%s", err, string(output)) + } + if got, err := os.ReadFile(filepath.Join(destination, "concurrent-user-data.txt")); err != nil || strings.TrimSpace(string(got)) != "keep" { + t.Fatalf("concurrent replacement data = %q, %v; want it retained", got, err) + } + if got, err := os.ReadFile(filepath.Join(source, "SKILL.md")); err != nil || string(got) != "new dev skill\n" { + t.Fatalf("source must be retained after the failed cross-device move, got %q, %v", got, err) + } + if _, err := os.Stat(filepath.Join(home, "replaced-published")); err != nil { + t.Fatalf("the replaced original copy must stay inspectable, not be deleted: %v", err) + } + for _, pattern := range []string{".dws-dev-rollback-*", ".dingtalk-misc.cross-device-*"} { + if matches, err := filepath.Glob(filepath.Join(root, "external", pattern)); err != nil || len(matches) != 0 { + t.Fatalf("devapp rollback leftovers %s = %v, %v", pattern, matches, err) + } + } +} + func TestInstallScriptLinkPublicationRacePreservesConcurrentObjects(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("POSIX shell semantics are unavailable") @@ -2641,6 +3032,167 @@ fi } } +// TestInstallerShellBackupMarkerWriteFailurePreservesSameStampSibling pins +// the review blocker on the shared stamp root: the backup collision loop +// tests the payload path, so two backups in the same stamp second land in +// ONE stamp root. When the second call fails to write the ownership marker, +// the old `rm -rf` cleanup destroyed the first, already completed sibling +// payload whose original directory had already been moved away. The cleanup +// must be non-recursive and may only drop a stamp root the failing call +// itself created (marker file plus empty root; rmdir refuses non-empty). +func TestInstallerShellBackupMarkerWriteFailurePreservesSameStampSibling(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX shell semantics are unavailable") + } + // Both scenarios rely on permission-bit enforcement (unwritable marker + // file, unwritable fresh root), which root bypasses entirely. + if os.Geteuid() == 0 { + t.Skip("running as root ignores permission bits, the marker write would succeed") + } + for _, scriptName := range []string{"install.sh", "install-skills.sh", "install-event.sh", "install-devapp.sh"} { + scriptName := scriptName + t.Run(scriptName, func(t *testing.T) { + t.Parallel() + backupFn := "backup_and_remove_skill_dir" + emitsMarkerWarning := true + if scriptName == "install-event.sh" || scriptName == "install-devapp.sh" { + backupFn = "backup_skill_dir" + emitsMarkerWarning = false + } + scriptPath, err := filepath.Abs(filepath.Join("..", "..", "scripts", scriptName)) + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(scriptPath) + if err != nil { + t.Fatal(err) + } + cut := strings.LastIndex(string(data), "\nmain\n") + if cut < 0 { + t.Fatalf("%s final main invocation not found", scriptName) + } + library := filepath.Join(t.TempDir(), scriptName+"-lib.sh") + mustWriteFile(t, library, data[:cut], 0o755) + + // Pin the stamp: a stub `date` always prints the same UTC second, + // so consecutive backups deterministically share one stamp root. + const stamp = "20260818-120000" + stubDir := filepath.Join(t.TempDir(), "bin") + mustWriteFile(t, filepath.Join(stubDir, "date"), []byte("#!/bin/sh\nprintf '"+stamp+"\\n'\n"), 0o755) + + t.Run("same-stamp sibling payload survives marker write failure", func(t *testing.T) { + home := t.TempDir() + root := filepath.Join(home, ".dws", "skill-backups", stamp) + victimA := filepath.Join(home, ".agents", "skills", "skill-a") + victimB := filepath.Join(home, ".agents", "skills", "skill-b") + mustWriteFile(t, filepath.Join(victimA, "SKILL.md"), []byte("payload A\n"), 0o644) + mustWriteFile(t, filepath.Join(victimB, "SKILL.md"), []byte("victim B\n"), 0o644) + + // The first backup succeeds and moves victim A into the stamp + // root; the marker is then made unwritable so the second, + // same-second backup fails exactly at the marker write. + harness := `. "$DWS_TEST_LIBRARY" +if ` + backupFn + ` "$DWS_TEST_VICTIM_A"; then :; else exit 10; fi +chmod 000 "$DWS_TEST_ROOT/` + skillBackupMarkerFile + `" +if ` + backupFn + ` "$DWS_TEST_VICTIM_B"; then + exit 11 +fi +[ -d "$DWS_TEST_ROOT" ] || exit 12 +[ -f "$DWS_TEST_VICTIM_B/SKILL.md" ] || exit 13 +` + cmd := exec.Command("sh", "-c", harness) + cmd.Env = append(os.Environ(), + "HOME="+home, + "PATH="+stubDir+":"+os.Getenv("PATH"), + "DWS_TEST_LIBRARY="+library, + "DWS_TEST_ROOT="+root, + "DWS_TEST_VICTIM_A="+victimA, + "DWS_TEST_VICTIM_B="+victimB, + ) + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("%s sibling marker-failure harness failed: %v\n%s", scriptName, err, output) + } + if emitsMarkerWarning && !strings.Contains(string(output), "无法写入备份所有权标记") { + t.Fatalf("%s did not report the marker write failure:\n%s", scriptName, output) + } + if _, err := os.Lstat(victimA); !os.IsNotExist(err) { + t.Fatalf("%s first victim must have been moved into the stamp root: %v", scriptName, err) + } + entries, err := os.ReadDir(root) + if err != nil { + t.Fatalf("%s stamp root must survive the failed marker write: %v", scriptName, err) + } + var payloadDirs []string + for _, entry := range entries { + if entry.Name() == skillBackupMarkerFile { + continue + } + if !entry.IsDir() { + t.Fatalf("%s stamp root carries an unexpected non-marker entry %s", scriptName, entry.Name()) + } + payloadDirs = append(payloadDirs, entry.Name()) + } + if len(payloadDirs) != 1 { + t.Fatalf("%s stamp root must retain exactly the first payload, got %v", scriptName, payloadDirs) + } + got, err := os.ReadFile(filepath.Join(root, payloadDirs[0], "SKILL.md")) + if err != nil || string(got) != "payload A\n" { + t.Fatalf("%s completed sibling payload was destroyed: %q, %v", scriptName, got, err) + } + if got, err := os.ReadFile(filepath.Join(victimB, "SKILL.md")); err != nil || string(got) != "victim B\n" { + t.Fatalf("%s second victim must stay at its original path: %q, %v", scriptName, got, err) + } + }) + + t.Run("fresh stamp root is dropped non-recursively", func(t *testing.T) { + home := t.TempDir() + root := filepath.Join(home, ".dws", "skill-backups", stamp) + victim := filepath.Join(home, ".agents", "skills", "skill-fresh") + mustWriteFile(t, filepath.Join(victim, "SKILL.md"), []byte("fresh victim\n"), 0o644) + + // The failing call itself created the root, so the cleanup + // must drop it again: mkdir is intercepted to hand back a + // non-writable root, failing the marker write while the root + // stays removable through its writable parent directory. + harness := `. "$DWS_TEST_LIBRARY" +mkdir() { + command mkdir "$@" + _mk_rc=$? + if [ "${1:-}" = "-p" ] && [ "${2:-}" = "$DWS_TEST_ROOT" ]; then + chmod 0555 "$DWS_TEST_ROOT" + fi + return "$_mk_rc" +} +if ` + backupFn + ` "$DWS_TEST_VICTIM"; then + exit 11 +fi +[ ! -e "$DWS_TEST_ROOT" ] || exit 12 +[ -f "$DWS_TEST_VICTIM/SKILL.md" ] || exit 13 +` + cmd := exec.Command("sh", "-c", harness) + cmd.Env = append(os.Environ(), + "HOME="+home, + "PATH="+stubDir+":"+os.Getenv("PATH"), + "DWS_TEST_LIBRARY="+library, + "DWS_TEST_ROOT="+root, + "DWS_TEST_VICTIM="+victim, + ) + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("%s fresh marker-failure harness failed: %v\n%s", scriptName, err, output) + } + if _, err := os.Lstat(root); !os.IsNotExist(err) { + t.Fatalf("%s fresh stamp root must be removed again: %v", scriptName, err) + } + if got, err := os.ReadFile(filepath.Join(victim, "SKILL.md")); err != nil || string(got) != "fresh victim\n" { + t.Fatalf("%s victim must stay untouched at its original path: %q, %v", scriptName, got, err) + } + }) + }) + } +} + func TestInstallerShellUsesUpstreamXDGAndCustomAgentRoots(t *testing.T) { for _, scriptName := range []string{"install.sh", "install-skills.sh"} { t.Run(scriptName, func(t *testing.T) {