From a47740ca1c061c9fc24870e048708e49f98c919b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Fri, 14 Aug 2026 19:23:59 +0800 Subject: [PATCH] fix(skills): make publication rollback race-safe --- .changes/996-canonical-agent-skills.md | 2 +- build/npm/install.js | 155 ++++++++- internal/app/skill_setup.go | 20 +- .../skill_setup_canonical_failures_test.go | 5 +- .../app/skill_setup_cleanup_coverage_test.go | 25 +- internal/app/skill_setup_plan_test.go | 12 +- .../app/skill_setup_publication_race_test.go | 85 +++++ internal/upgrade/canonical_skills_test.go | 4 +- internal/upgrade/paths.go | 60 ++-- internal/upgrade/paths_multi_test.go | 22 +- internal/upgrade/skill_path_move.go | 39 +-- internal/upgrade/skill_path_move_test.go | 10 +- internal/upgrade/skill_publication.go | 153 +++++++++ internal/upgrade/skill_publication_test.go | 314 ++++++++++++++++++ .../upgrade/skill_publish_rename_darwin.go | 9 + .../upgrade/skill_publish_rename_linux.go | 9 + .../upgrade/skill_publish_rename_other.go | 12 + .../upgrade/skill_publish_rename_windows.go | 17 + .../upgrade/skill_publish_transaction_test.go | 45 ++- test/scripts/install_js_smoke.mjs | 71 +++- 20 files changed, 934 insertions(+), 135 deletions(-) create mode 100644 internal/app/skill_setup_publication_race_test.go create mode 100644 internal/upgrade/skill_publication.go create mode 100644 internal/upgrade/skill_publication_test.go create mode 100644 internal/upgrade/skill_publish_rename_darwin.go create mode 100644 internal/upgrade/skill_publish_rename_linux.go create mode 100644 internal/upgrade/skill_publish_rename_other.go create mode 100644 internal/upgrade/skill_publish_rename_windows.go diff --git a/.changes/996-canonical-agent-skills.md b/.changes/996-canonical-agent-skills.md index 2608ccd6..b04fdd62 100644 --- a/.changes/996-canonical-agent-skills.md +++ b/.changes/996-canonical-agent-skills.md @@ -2,4 +2,4 @@ category: Fixed --- -- **Canonical Agent Skills** (#996) — installs bundled DWS Skills once under `~/.agents/skills`, migrates duplicate Agent copies, and matches the upstream 76-Agent registry across Go, npm, Shell, and PowerShell. Non-universal Agents use links (Windows junctions) with safe copy fallback; custom/XDG homes and OpenClaw legacy aliases are preserved. Upgrades now back up and restore Skills safely across external volumes by staging, lexically copying links (including dangling links), verifying contents, and deleting the source only after publication succeeds. Standalone installers verify every downloaded release asset against `checksums.txt`, and the npm engine declaration now reflects the actual Node 16.7+ API floor. +- **Canonical Agent Skills** (#996) — installs bundled DWS Skills once under `~/.agents/skills`, migrates duplicate Agent copies, and matches the upstream 76-Agent registry across Go, npm, Shell, and PowerShell. Non-universal Agents use links (Windows junctions) with safe copy fallback; custom/XDG homes and OpenClaw legacy aliases are preserved. Upgrades now back up and restore Skills safely across external volumes by staging, lexically copying links (including dangling links), verifying contents, and deleting the source only after publication succeeds. Atomic no-replace publication and identity-checked quarantine rollback preserve concurrent user changes instead of overwriting or recursively deleting them. Standalone installers verify every downloaded release asset against `checksums.txt`, and the npm engine declaration now reflects the actual Node 16.7+ API floor. diff --git a/build/npm/install.js b/build/npm/install.js index 5550d449..d64e2ef0 100755 --- a/build/npm/install.js +++ b/build/npm/install.js @@ -698,10 +698,133 @@ function samePhysicalDir(left, right) { } } +function writeSkillFingerprintField(hash, value) { + const bytes = Buffer.from(String(value), "utf8"); + hash.update(String(bytes.length), "utf8"); + hash.update(":", "utf8"); + hash.update(bytes); +} + +function skillPathFingerprint(target) { + const hash = crypto.createHash("sha256"); + const visit = (current) => { + const stat = fs.lstatSync(current, { bigint: true }); + writeSkillFingerprintField(hash, Number(stat.mode & 0o170000n)); + writeSkillFingerprintField(hash, Number(stat.mode & 0o777n)); + if (stat.isDirectory()) { + const names = fs.readdirSync(current).sort((a, b) => Buffer.from(a).compare(Buffer.from(b))); + writeSkillFingerprintField(hash, names.length); + for (const name of names) { + writeSkillFingerprintField(hash, name); + visit(path.join(current, name)); + } + return; + } + if (stat.isSymbolicLink()) { + writeSkillFingerprintField(hash, fs.readlinkSync(current)); + return; + } + if (stat.isFile()) { + writeSkillFingerprintField(hash, stat.size.toString()); + hash.update(fs.readFileSync(current)); + return; + } + throw new Error(`unsupported Skill path type: ${current}`); + }; + visit(target); + return hash.digest("hex"); +} + +function captureSkillPublication(source, destination) { + const stat = fs.lstatSync(source, { bigint: true }); + return { + destination, + device: stat.dev.toString(), + inode: stat.ino.toString(), + fingerprint: skillPathFingerprint(source), + }; +} + +function skillPublicationMatches(target, publication) { + const stat = fs.lstatSync(target, { bigint: true }); + return stat.dev.toString() === publication.device + && stat.ino.toString() === publication.inode + && skillPathFingerprint(target) === publication.fingerprint; +} + +function removeSkillPathLexically(target) { + const stat = fs.lstatSync(target); + if (stat.isDirectory() && !stat.isSymbolicLink()) { + fs.rmSync(target, { recursive: true, force: true }); + } else { + fs.unlinkSync(target); + } +} + +function skillPathExistsLexically(target) { + try { + fs.lstatSync(target); + return true; + } catch (err) { + if (err && err.code === "ENOENT") return false; + throw err; + } +} + +function rollbackPublishedSkillPath(publication, options = {}) { + const renameFn = options.renameFn || fs.renameSync; + const removeFn = options.removeFn || removeSkillPathLexically; + const destination = publication.destination; + const quarantineRoot = fs.mkdtempSync(path.join(path.dirname(destination), `.${path.basename(destination)}.rollback-`)); + const quarantine = path.join(quarantineRoot, "payload"); + try { + renameFn(destination, quarantine); + } catch (err) { + fs.rmSync(quarantineRoot, { recursive: true, force: true }); + if (err && err.code === "ENOENT") return; + throw new Error(`failed to quarantine published Skill ${destination}: ${err.message}`); + } + let owned = false; + try { + owned = skillPublicationMatches(quarantine, publication); + } catch (err) { + throw new Error(`refusing to delete unverifiable Skill ${destination}; object retained at ${quarantine}: ${err.message}`); + } + if (!owned) { + throw new Error(`refusing to delete non-transaction Skill ${destination}; concurrent object retained at ${quarantine}`); + } + removeFn(quarantine); + fs.rmSync(quarantineRoot, { recursive: true, force: true }); +} + +function publishCanonicalLinkNoReplace(staged, destination, options = {}) { + const publication = captureSkillPublication(staged, destination); + if (process.platform === "win32") { + const renameFn = options.publishRenameFn || fs.renameSync; + renameFn(staged, destination); + } else { + const linkFn = options.publishLinkFn || ((source, target) => { + const result = childProcess.spawnSync("ln", ["-P", source, target], { encoding: "utf8" }); + if (result.error) throw result.error; + if (result.status !== 0) { + const detail = (result.stderr || result.stdout || "ln -P failed").trim(); + const error = new Error(detail); + if (skillPathExistsLexically(target)) error.code = "EEXIST"; + throw error; + } + }); + linkFn(staged, destination); + } + if (!skillPublicationMatches(destination, publication)) { + throw new Error(`published Skill identity changed before confirmation: ${destination}`); + } + return publication; +} + function publishCanonicalLinksAtomically(homeDir, canonicalBase, baseDir, names, victims, options = {}) { const moveOptions = skillMoveOptions(options); - const publishRenameFn = options.publishRenameFn || fs.renameSync; - const publishRemoveFn = options.publishRemoveFn || ((target) => fs.rmSync(target, { recursive: true, force: true })); + const publishRemoveFn = options.publishRemoveFn || removeSkillPathLexically; + const rollbackRenameFn = options.rollbackRenameFn || fs.renameSync; fs.mkdirSync(baseDir, { recursive: true }); const realBaseDir = fs.realpathSync(baseDir); const stageRoot = fs.mkdtempSync(path.join(baseDir, ".dws-link-set.tmp-")); @@ -713,9 +836,9 @@ function publishCanonicalLinksAtomically(homeDir, canonicalBase, baseDir, names, const restoreErrors = []; for (let i = published.length - 1; i >= 0; i -= 1) { try { - publishRemoveFn(published[i]); + rollbackPublishedSkillPath(published[i], { renameFn: rollbackRenameFn, removeFn: publishRemoveFn }); } catch (err) { - restoreErrors.push(`remove ${published[i]}: ${err.message}`); + restoreErrors.push(`remove ${published[i].destination}: ${err.message}`); } } for (let i = backups.length - 1; i >= 0; i -= 1) { @@ -752,8 +875,8 @@ function publishCanonicalLinksAtomically(homeDir, canonicalBase, baseDir, names, backupAndRemoveSkillDir(homeDir, victim, backups, moveOptions); } for (const item of staged) { - publishRenameFn(item.staged, item.dest); - published.push(item.dest); + const publication = publishCanonicalLinkNoReplace(item.staged, item.dest, options); + published.push(publication); } } catch (err) { try { @@ -807,6 +930,7 @@ function publishManagedMultiSkillSetAtomically( const copyFn = options.copyFn || copyChildren; const renameFn = options.renameFn || fs.renameSync; const removeFn = options.removeFn || ((dir) => fs.rmSync(dir, { recursive: true, force: true })); + const rollbackRenameFn = options.rollbackRenameFn || fs.renameSync; const moveOptions = skillMoveOptions(options); fs.mkdirSync(baseDir, { recursive: true }); const stageRoot = fs.mkdtempSync(path.join(baseDir, ".dws-multi-set.tmp-")); @@ -818,9 +942,9 @@ function publishManagedMultiSkillSetAtomically( const restoreErrors = []; for (let i = published.length - 1; i >= 0; i -= 1) { try { - removeFn(published[i]); + rollbackPublishedSkillPath(published[i], { renameFn: rollbackRenameFn, removeFn }); } catch (err) { - restoreErrors.push(`remove ${published[i]}: ${err.message}`); + restoreErrors.push(`remove ${published[i].destination}: ${err.message}`); } } for (let i = backups.length - 1; i >= 0; i -= 1) { @@ -855,8 +979,11 @@ function publishManagedMultiSkillSetAtomically( } for (const item of staged) { + const publication = captureSkillPublication(item.staged, item.dest); + if (skillPathExistsLexically(item.dest)) throw new Error(`Skill publish destination already exists: ${item.dest}`); renameFn(item.staged, item.dest); - published.push(item.dest); + if (!skillPublicationMatches(item.dest, publication)) throw new Error(`published Skill identity changed before confirmation: ${item.dest}`); + published.push(publication); } } catch (err) { try { @@ -883,6 +1010,7 @@ function publishManagedMonoSkillSetAtomically( const copyFn = options.copyFn || copyChildren; const renameFn = options.renameFn || fs.renameSync; const removeFn = options.removeFn || ((dir) => fs.rmSync(dir, { recursive: true, force: true })); + const rollbackRenameFn = options.rollbackRenameFn || fs.renameSync; const moveOptions = skillMoveOptions(options); fs.mkdirSync(baseDir, { recursive: true }); const stageRoot = fs.mkdtempSync(path.join(baseDir, ".dws-mono-set.tmp-")); @@ -895,9 +1023,9 @@ function publishManagedMonoSkillSetAtomically( const restoreErrors = []; for (let i = published.length - 1; i >= 0; i -= 1) { try { - removeFn(published[i]); + rollbackPublishedSkillPath(published[i], { renameFn: rollbackRenameFn, removeFn }); } catch (err) { - restoreErrors.push(`remove ${published[i]}: ${err.message}`); + restoreErrors.push(`remove ${published[i].destination}: ${err.message}`); } } for (let i = backups.length - 1; i >= 0; i -= 1) { @@ -927,8 +1055,11 @@ function publishManagedMonoSkillSetAtomically( backupAndRemoveSkillDir(homeDir, victim, backups, moveOptions); } - published.push(destDir); + const publication = captureSkillPublication(stagedDir, destDir); + if (skillPathExistsLexically(destDir)) throw new Error(`Skill publish destination already exists: ${destDir}`); renameFn(stagedDir, destDir); + if (!skillPublicationMatches(destDir, publication)) throw new Error(`published Skill identity changed before confirmation: ${destDir}`); + published.push(publication); } catch (err) { try { restore(); diff --git a/internal/app/skill_setup.go b/internal/app/skill_setup.go index 9a4ebfb8..56858494 100644 --- a/internal/app/skill_setup.go +++ b/internal/app/skill_setup.go @@ -143,6 +143,8 @@ var ( skillSetupReadState = skillstate.Read skillSetupWriteState = skillstate.Write skillSetupRemoveState = skillstate.Remove + skillSetupPublishPath = upgrade.PublishSkillPathNoReplace + skillSetupRollbackPaths = upgrade.RollbackSkillPathPublications skillSetupNow = time.Now skillSetupFoldPathCase = runtime.GOOS == "windows" ) @@ -2035,13 +2037,8 @@ func stageSkillSetupTarget(plan *skillSetupPlan, target skillSetupTargetPlan) (s // restoreSkillSetupTarget removes a partially published replacement and // restores every original directory from its exact backup path. -func restoreSkillSetupTarget(published []string, backups []skillSetupBackedUpDir) error { - var restoreErr error - for i := len(published) - 1; i >= 0; i-- { - if err := skillSetupRemoveAll(published[i]); err != nil { - restoreErr = errors.Join(restoreErr, fmt.Errorf("移除失败发布目录 %s: %w", published[i], err)) - } - } +func restoreSkillSetupTarget(published []upgrade.SkillPathPublication, backups []skillSetupBackedUpDir) error { + restoreErr := skillSetupRollbackPaths(published) for i := len(backups) - 1; i >= 0; i-- { item := backups[i] if _, err := skillSetupLstat(item.original); err == nil { @@ -2088,18 +2085,17 @@ func backupSkillSetupTarget(home string, planned []skillSetupBackup, out io.Writ } func publishSkillSetupTarget(staged []skillSetupStagedDir, backups []skillSetupBackedUpDir) error { - published := make([]string, 0, len(staged)) + published := make([]upgrade.SkillPathPublication, 0, len(staged)) for _, item := range staged { - // Record before rename so rollback also removes a destination created by - // a platform-specific partial failure. - published = append(published, item.dest) - if err := skillSetupPublishRename(item.staged, item.dest); err != nil { + publication, err := skillSetupPublishPath(item.staged, item.dest) + if err != nil { publishErr := fmt.Errorf("发布 Skill 失败 %s: %w", item.dest, err) if restoreErr := restoreSkillSetupTarget(published, backups); restoreErr != nil { return errors.Join(publishErr, fmt.Errorf("Skill setup 回滚不完整: %w", restoreErr)) } return publishErr } + published = append(published, publication) } return nil } diff --git a/internal/app/skill_setup_canonical_failures_test.go b/internal/app/skill_setup_canonical_failures_test.go index c1b82a19..c5d6b7fd 100644 --- a/internal/app/skill_setup_canonical_failures_test.go +++ b/internal/app/skill_setup_canonical_failures_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/upgrade" ) func canonicalFailurePlan(t *testing.T) (string, *skillSetupPlan) { @@ -62,7 +63,9 @@ func TestCrossPlatformCoverageSkillSetupCanonicalHomeBackupAndPublishFailures(t t.Run("publish", func(t *testing.T) { home, plan := canonicalFailurePlan(t) testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) - testseam.Swap(t, &skillSetupPublishRename, func(string, string) error { return errors.New("publish denied") }) + testseam.Swap(t, &skillSetupPublishPath, func(string, string) (upgrade.SkillPathPublication, error) { + return upgrade.SkillPathPublication{}, errors.New("publish denied") + }) if _, _, err := executeSkillSetupPlan(plan, &bytes.Buffer{}, &bytes.Buffer{}); err == nil || !strings.Contains(err.Error(), "canonical Skill 发布失败") { t.Fatalf("publish failure = %v", err) } diff --git a/internal/app/skill_setup_cleanup_coverage_test.go b/internal/app/skill_setup_cleanup_coverage_test.go index 6fea3de5..b346eb3d 100644 --- a/internal/app/skill_setup_cleanup_coverage_test.go +++ b/internal/app/skill_setup_cleanup_coverage_test.go @@ -15,6 +15,7 @@ import ( "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/skillprovenance" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/skillstate" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/upgrade" ) func useManagedSkillNames(t *testing.T, names ...string) { @@ -336,12 +337,12 @@ func TestCrossPlatformCoverageSkillSetupTransactionFailuresRestoreOldSet(t *test return originalBackup(homeDir, dir) }) } else { - originalRename := skillSetupPublishRename - testseam.Swap(t, &skillSetupPublishRename, func(oldPath, newPath string) error { + originalPublish := skillSetupPublishPath + testseam.Swap(t, &skillSetupPublishPath, func(oldPath, newPath string) (upgrade.SkillPathPublication, error) { if newPath == second && strings.HasPrefix(filepath.Base(filepath.Dir(oldPath)), ".dws-setup-set-") { - return failure + return upgrade.SkillPathPublication{}, failure } - return originalRename(oldPath, newPath) + return originalPublish(oldPath, newPath) }) } @@ -432,8 +433,8 @@ func TestCrossPlatformCoverageSkillSetupTransactionFailureEdges(t *testing.T) { t.Run("restore failure aggregation", func(t *testing.T) { t.Run("remove published", func(t *testing.T) { - testseam.Swap(t, &skillSetupRemoveAll, func(string) error { return failure }) - if err := restoreSkillSetupTarget([]string{"published"}, nil); !errors.Is(err, failure) { + testseam.Swap(t, &skillSetupRollbackPaths, func([]upgrade.SkillPathPublication) error { return failure }) + if err := restoreSkillSetupTarget([]upgrade.SkillPathPublication{{Destination: "published"}}, nil); !errors.Is(err, failure) { t.Fatalf("remove published error = %v", err) } }) @@ -490,7 +491,9 @@ func TestCrossPlatformCoverageSkillSetupTransactionFailureEdges(t *testing.T) { }) t.Run("publish rollback failure", func(t *testing.T) { - testseam.Swap(t, &skillSetupPublishRename, func(string, string) error { return failure }) + testseam.Swap(t, &skillSetupPublishPath, func(string, string) (upgrade.SkillPathPublication, error) { + return upgrade.SkillPathPublication{}, failure + }) testseam.Swap(t, &skillSetupRestoreBackup, func(string, string) error { return failure }) testseam.Swap(t, &skillSetupLstat, func(string) (os.FileInfo, error) { return nil, os.ErrNotExist }) testseam.Swap(t, &skillSetupMkdirAll, func(string, os.FileMode) error { return nil }) @@ -531,12 +534,12 @@ func TestCrossPlatformCoverageSkillSetupTransactionFailureEdges(t *testing.T) { t.Run("after publish failure", func(t *testing.T) { plan := newPlan(t) - originalRename := skillSetupPublishRename - testseam.Swap(t, &skillSetupPublishRename, func(oldPath, newPath string) error { + originalPublish := skillSetupPublishPath + testseam.Swap(t, &skillSetupPublishPath, func(oldPath, newPath string) (upgrade.SkillPathPublication, error) { if strings.HasPrefix(filepath.Base(filepath.Dir(oldPath)), ".dws-setup-set-") { - return failure + return upgrade.SkillPathPublication{}, failure } - return originalRename(oldPath, newPath) + return originalPublish(oldPath, newPath) }) originalRemoveAll := skillSetupRemoveAll cleanupErr := errors.New("cleanup after publish failure") diff --git a/internal/app/skill_setup_plan_test.go b/internal/app/skill_setup_plan_test.go index 8330d6e9..406b318a 100644 --- a/internal/app/skill_setup_plan_test.go +++ b/internal/app/skill_setup_plan_test.go @@ -48,16 +48,13 @@ func TestCrossPlatformCoverageSkillSetupPlanPreviewDeclineAndExecutionMatch(t *t backupCalls, copyCalls := []string{}, 0 testseam.Swap(t, &skillSetupBackupAndRemove, func(_ string, path string) (string, error) { backupCalls = append(backupCalls, path) + if err := os.RemoveAll(path); err != nil { + return "", err + } return "backup", nil }) testseam.Swap(t, &skillSetupCopyDir, func(string, string) error { copyCalls++; return nil }) testseam.Swap(t, &skillSetupWriteFile, func(string, []byte, os.FileMode) error { return nil }) - testseam.Swap(t, &skillSetupPublishRename, func(src, dest string) error { - if err := os.RemoveAll(dest); err != nil { - return err - } - return os.Rename(src, dest) - }) dryRunCmd := skillSetupCoverageCommand(t, skillSetupModeMulti, false) var dryRunOut bytes.Buffer dryRunCmd.SetOut(&dryRunOut) @@ -119,6 +116,9 @@ func TestCrossPlatformCoverageSkillSetupPlanPreviewDeclineAndExecutionMatch(t *t // A filtered multi plan replaces only selected same-name skills and leaves // unselected siblings out of the backup set. + if err := os.MkdirAll(filepath.Join(dest, "dws"), 0o755); err != nil { + t.Fatal(err) + } filtered, err := buildSkillSetupPlan(skillSetupModeMulti, source, []string{dest}, []string{"dingtalk-a"}, true) if err != nil { t.Fatal(err) diff --git a/internal/app/skill_setup_publication_race_test.go b/internal/app/skill_setup_publication_race_test.go new file mode 100644 index 00000000..8d954e3a --- /dev/null +++ b/internal/app/skill_setup_publication_race_test.go @@ -0,0 +1,85 @@ +package app + +import ( + "errors" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/upgrade" +) + +func TestCrossPlatformCoverageSkillSetupRollbackRetainsConcurrentReplacement(t *testing.T) { + home := t.TempDir() + base := filepath.Join(home, ".agents", "skills") + first := filepath.Join(base, "dingtalk-first") + second := filepath.Join(base, "dingtalk-second") + writeSkillSetupFile(t, first, "old first") + writeSkillSetupFile(t, second, "old second") + backups, err := backupSkillSetupTarget(home, []skillSetupBackup{{Path: first}, {Path: second}}, io.Discard) + if err != nil { + t.Fatal(err) + } + stageRoot := filepath.Join(base, ".stage") + stagedFirst := filepath.Join(stageRoot, "dingtalk-first") + stagedSecond := filepath.Join(stageRoot, "dingtalk-second") + writeSkillSetupFile(t, stagedFirst, "new first") + writeSkillSetupFile(t, stagedSecond, "new second") + + failure := errors.New("injected second setup publication failure") + originalPublish := skillSetupPublishPath + calls := 0 + testseam.Swap(t, &skillSetupPublishPath, func(staged, destination string) (upgrade.SkillPathPublication, error) { + calls++ + if calls == 2 { + if err := os.RemoveAll(first); err != nil { + t.Fatal(err) + } + writeSkillSetupFile(t, first, "concurrent") + return upgrade.SkillPathPublication{}, failure + } + return originalPublish(staged, destination) + }) + + err = publishSkillSetupTarget([]skillSetupStagedDir{ + {staged: stagedFirst, dest: first}, + {staged: stagedSecond, dest: second}, + }, backups) + if !errors.Is(err, failure) || !strings.Contains(err.Error(), "拒绝删除非本事务") { + t.Fatalf("setup transaction error = %v", err) + } + assertSkillSetupFile(t, first, "concurrent") + assertSkillSetupFile(t, second, "old second") + var firstBackup string + for _, item := range backups { + if item.original == first { + firstBackup = item.backup + break + } + } + if firstBackup == "" { + t.Fatal("first backup was not recorded") + } + assertSkillSetupFile(t, firstBackup, "old first") +} + +func writeSkillSetupFile(t *testing.T, dir, content string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func assertSkillSetupFile(t *testing.T, dir, want string) { + t.Helper() + content, err := os.ReadFile(filepath.Join(dir, "SKILL.md")) + if err != nil || string(content) != want { + t.Fatalf("Skill content at %s = %q, %v; want %q", dir, content, err, want) + } +} diff --git a/internal/upgrade/canonical_skills_test.go b/internal/upgrade/canonical_skills_test.go index eee92fa4..9008fc31 100644 --- a/internal/upgrade/canonical_skills_test.go +++ b/internal/upgrade/canonical_skills_test.go @@ -416,8 +416,8 @@ func TestCrossPlatformCoverageUpgradeAgentBranchFailures(t *testing.T) { if mode == "multi" { useUpgradeManagedNames(t, "dingtalk-chat") } - origRename := upgradeRename - testseam.Swap(t, &upgradeRename, func(src, dst string) error { + origRename := skillPathRenameNoReplace + testseam.Swap(t, &skillPathRenameNoReplace, func(src, dst string) error { if src == victim { return errors.New("retire denied") } diff --git a/internal/upgrade/paths.go b/internal/upgrade/paths.go index 1eab7cf1..05551c79 100644 --- a/internal/upgrade/paths.go +++ b/internal/upgrade/paths.go @@ -140,26 +140,28 @@ var universalSkillDirs = map[string]bool{ } var ( - upgradeUserHomeDir = os.UserHomeDir - upgradeExecutable = os.Executable - upgradeEvalSymlinks = filepath.EvalSymlinks - upgradeCopyDir = copyDir - upgradeEnsureDir = ensureDir - upgradeRemoveAll = os.RemoveAll - upgradeMkdirAll = os.MkdirAll - upgradeMkdirTemp = os.MkdirTemp - upgradeReadDir = os.ReadDir - upgradeStat = os.Stat - upgradeLstat = os.Lstat - upgradeSymlink = os.Symlink - upgradeRel = filepath.Rel - upgradeGetenv = os.Getenv - upgradeBuildProvenance = skillprovenance.Build - upgradeReadSkillState = skillstate.Read - upgradeBackupStamp = func() string { return time.Now().UTC().Format("20060102-150405") } - upgradeWriteSkillState = skillstate.Write - upgradeNow = time.Now - upgradeFoldPathCase = runtime.GOOS == "windows" + upgradeUserHomeDir = os.UserHomeDir + upgradeExecutable = os.Executable + upgradeEvalSymlinks = filepath.EvalSymlinks + upgradeCopyDir = copyDir + upgradeEnsureDir = ensureDir + upgradeRemoveAll = os.RemoveAll + upgradeMkdirAll = os.MkdirAll + upgradeMkdirTemp = os.MkdirTemp + upgradeReadDir = os.ReadDir + upgradeStat = os.Stat + upgradeLstat = os.Lstat + upgradeSymlink = os.Symlink + upgradeRel = filepath.Rel + upgradeGetenv = os.Getenv + upgradeBuildProvenance = skillprovenance.Build + upgradeReadSkillState = skillstate.Read + upgradePublishSkillPath = PublishSkillPathNoReplace + upgradeRollbackPublishedSkillPaths = RollbackSkillPathPublications + upgradeBackupStamp = func() string { return time.Now().UTC().Format("20060102-150405") } + upgradeWriteSkillState = skillstate.Write + upgradeNow = time.Now + upgradeFoldPathCase = runtime.GOOS == "windows" ) // skillBackupSubdir is the user-level directory where skill directories are @@ -492,13 +494,8 @@ func uniqueSkillDirs(paths []string) []string { // restoreSkillSet removes any newly published directories, then restores all // original directories in reverse backup order. -func restoreSkillSet(published []string, backups []backedUpSkillDir) error { - var restoreErr error - for i := len(published) - 1; i >= 0; i-- { - if err := upgradeRemoveAll(published[i]); err != nil { - restoreErr = errors.Join(restoreErr, fmt.Errorf("移除失败发布目录 %s: %w", published[i], err)) - } - } +func restoreSkillSet(published []SkillPathPublication, backups []backedUpSkillDir) error { + restoreErr := upgradeRollbackPublishedSkillPaths(published) for i := len(backups) - 1; i >= 0; i-- { backup := backups[i] if err := upgradeMkdirAll(filepath.Dir(backup.original), dirPermShared); err != nil { @@ -538,16 +535,17 @@ func publishStagedSkillSet(homeDir string, staged []stagedSkillDir, victims []st if err != nil { return err } - published := make([]string, 0, len(staged)) + published := make([]SkillPathPublication, 0, len(staged)) for _, skill := range staged { - if err := upgradeRename(skill.staged, skill.dest); err != nil { - publishErr := fmt.Errorf("发布 Skill 失败 %s: %w", skill.dest, err) + publication, publishErr := upgradePublishSkillPath(skill.staged, skill.dest) + if publishErr != nil { + publishErr := fmt.Errorf("发布 Skill 失败 %s: %w", skill.dest, publishErr) if restoreErr := restoreSkillSet(published, backups); restoreErr != nil { return errors.Join(publishErr, fmt.Errorf("恢复原 Skill 集合失败: %w", restoreErr)) } return publishErr } - published = append(published, skill.dest) + published = append(published, publication) } return nil } diff --git a/internal/upgrade/paths_multi_test.go b/internal/upgrade/paths_multi_test.go index a0d67135..6ba5d481 100644 --- a/internal/upgrade/paths_multi_test.go +++ b/internal/upgrade/paths_multi_test.go @@ -625,8 +625,8 @@ func TestCrossPlatformCoverageUpgradeSkillLocationsMultiFallbackCleanupFailure(t } useUpgradeManagedNames(t, filepath.Base(staleDir)) - origRename := upgradeRename - testseam.Swap(t, &upgradeRename, func(src, dst string) error { + origRename := skillPathRenameNoReplace + testseam.Swap(t, &skillPathRenameNoReplace, func(src, dst string) error { if strings.Contains(src, "dingtalk-stale") { return errors.New("injected backup failure") } @@ -814,14 +814,14 @@ func TestCrossPlatformCoverageBackupAndRemoveSkillDirEdges(t *testing.T) { if err := os.MkdirAll(victim4, 0o755); err != nil { t.Fatal(err) } - testseam.Swap(t, &upgradeRename, func(string, string) error { return errors.New("rename denied") }) + testseam.Swap(t, &skillPathRenameNoReplace, func(string, string) error { return errors.New("rename denied") }) if _, err := backupAndRemoveSkillDir(home, victim4); err == nil || !strings.Contains(err.Error(), "备份技能目录失败") { t.Fatalf("rename error = %v", err) } if _, err := os.Stat(victim4); err != nil { t.Fatalf("victim4 must survive rename failure: %v", err) } - testseam.Swap(t, &upgradeRename, os.Rename) + testseam.Swap(t, &skillPathRenameNoReplace, renameSkillPathNoReplace) } // TestCrossPlatformCoveragePruneSkillBackupsEdges pins the backup retention: @@ -949,11 +949,11 @@ func TestCrossPlatformCoverageMonoUpgradeBackupAndFallbackEdges(t *testing.T) { os.MkdirAll(filepath.Join(home, ".agents", "skills", "dws"), 0o755) os.WriteFile(filepath.Join(home, ".agents", "skills", "dws", "SKILL.md"), []byte("old"), 0o644) os.MkdirAll(filepath.Join(home, ".claude"), 0o755) - testseam.Swap(t, &upgradeRename, func(src, dst string) error { + testseam.Swap(t, &skillPathRenameNoReplace, func(src, dst string) error { if strings.Contains(src, ".agents") { return errors.New("backup denied") } - return os.Rename(src, dst) + return renameSkillPathNoReplace(src, dst) }) result, err := UpgradeSkillLocations(mono) if err == nil || len(result.Failed()) != 1 || len(result.Succeeded()) != 0 { @@ -1014,7 +1014,7 @@ func TestCrossPlatformCoverageCleanupLeftoversEdges(t *testing.T) { // Backup failure of a multi leftover aborts cleanupMultiLeftovers. os.MkdirAll(filepath.Join(base, "dingtalk-stale"), 0o755) useUpgradeManagedNames(t, "dingtalk-stale") - testseam.Swap(t, &upgradeRename, func(string, string) error { return errors.New("backup denied") }) + testseam.Swap(t, &skillPathRenameNoReplace, func(string, string) error { return errors.New("backup denied") }) if err := cleanupMultiLeftovers(home, base); err == nil || !strings.Contains(err.Error(), "备份并清理 multi 残留失败") { t.Fatalf("cleanupMultiLeftovers backup error = %v", err) } @@ -1024,19 +1024,19 @@ func TestCrossPlatformCoverageCleanupLeftoversEdges(t *testing.T) { if err := cleanupOppositeModeLeftovers(home, base, map[string]bool{}); err == nil || !strings.Contains(err.Error(), "备份并清理对面模式残留失败") { t.Fatalf("opposite cleanup mono backup error = %v", err) } - testseam.Swap(t, &upgradeRename, os.Rename) + testseam.Swap(t, &skillPathRenameNoReplace, renameSkillPathNoReplace) // Backup failure of a stale (non-mono) skill aborts with its own message. - testseam.Swap(t, &upgradeRename, func(src, dst string) error { + testseam.Swap(t, &skillPathRenameNoReplace, func(src, dst string) error { if strings.Contains(src, "dingtalk-stale") { return errors.New("backup denied") } - return os.Rename(src, dst) + return renameSkillPathNoReplace(src, dst) }) if err := cleanupOppositeModeLeftovers(home, base, map[string]bool{}); err == nil || !strings.Contains(err.Error(), "备份并清理对面模式残留失败") { t.Fatalf("opposite cleanup stale backup error = %v", err) } - testseam.Swap(t, &upgradeRename, os.Rename) + testseam.Swap(t, &skillPathRenameNoReplace, renameSkillPathNoReplace) // Success matrix: mono leftover + stale skill removed into backups, bundle // skill and regular file preserved. diff --git a/internal/upgrade/skill_path_move.go b/internal/upgrade/skill_path_move.go index 2c28be90..e9f9539c 100644 --- a/internal/upgrade/skill_path_move.go +++ b/internal/upgrade/skill_path_move.go @@ -11,23 +11,24 @@ import ( ) var ( - skillPathRename = func(src, dst string) error { return upgradeRename(src, dst) } - skillPathCopy = copySkillPathLexically - skillPathVerify = verifySkillPathCopy - skillPathRemoveAll = os.RemoveAll - skillPathMkdirAll = os.MkdirAll - skillPathMkdirTemp = os.MkdirTemp - skillPathChmod = os.Chmod - skillPathLstat = os.Lstat - skillPathMkdir = os.Mkdir - skillPathReadDir = os.ReadDir - skillPathReadlink = os.Readlink - skillPathSymlink = os.Symlink - skillPathOpen = os.Open - skillPathOpenFile = os.OpenFile - skillPathCopyBytes = io.Copy - skillPathSync = func(file *os.File) error { return file.Sync() } - skillPathWalkDir = filepath.WalkDir + skillPathRename = func(src, dst string) error { return upgradeRename(src, dst) } + skillPathRenameNoReplace = renameSkillPathNoReplace + skillPathCopy = copySkillPathLexically + skillPathVerify = verifySkillPathCopy + skillPathRemoveAll = os.RemoveAll + skillPathMkdirAll = os.MkdirAll + skillPathMkdirTemp = os.MkdirTemp + skillPathChmod = os.Chmod + skillPathLstat = os.Lstat + skillPathMkdir = os.Mkdir + skillPathReadDir = os.ReadDir + skillPathReadlink = os.Readlink + skillPathSymlink = os.Symlink + skillPathOpen = os.Open + skillPathOpenFile = os.OpenFile + skillPathCopyBytes = io.Copy + skillPathSync = func(file *os.File) error { return file.Sync() } + skillPathWalkDir = filepath.WalkDir ) // moveSkillPathRecoverably moves one managed Skill path without weakening the @@ -44,7 +45,7 @@ func moveSkillPathRecoverably(src, dst string) (err error) { if err := skillPathMkdirAll(filepath.Dir(dst), dirPermShared); err != nil { return fmt.Errorf("创建移动目标目录失败 %s: %w", filepath.Dir(dst), err) } - if renameErr := skillPathRename(src, dst); renameErr == nil { + if renameErr := skillPathRenameNoReplace(src, dst); renameErr == nil { return nil } else if !isCrossDeviceError(renameErr) { return fmt.Errorf("移动 Skill 路径失败 %s -> %s: %w", src, dst, renameErr) @@ -85,7 +86,7 @@ func moveSkillPathRecoverably(src, dst string) (err error) { return fmt.Errorf("准备跨设备 Skill staging 发布失败 %s: %w", stage, err) } } - if err := skillPathRename(stage, dst); err != nil { + if err := skillPathRenameNoReplace(stage, dst); err != nil { return fmt.Errorf("发布跨设备 Skill 备份失败 %s: %w", dst, err) } if stageInfo.IsDir() { diff --git a/internal/upgrade/skill_path_move_test.go b/internal/upgrade/skill_path_move_test.go index 4140fd6a..6d598590 100644 --- a/internal/upgrade/skill_path_move_test.go +++ b/internal/upgrade/skill_path_move_test.go @@ -140,7 +140,7 @@ func TestCrossPlatformCoverageSkillPathCrossDeviceFailures(t *testing.T) { t.Run("non cross-device rename never copies", func(t *testing.T) { src, dst := makeSkillMoveFixture(t) copied := false - testseam.Swap(t, &skillPathRename, func(string, string) error { return os.ErrPermission }) + testseam.Swap(t, &skillPathRenameNoReplace, func(string, string) error { return os.ErrPermission }) testseam.Swap(t, &skillPathCopy, func(string, string) error { copied = true; return nil }) if err := moveSkillPathRecoverably(src, dst); !errors.Is(err, os.ErrPermission) { t.Fatalf("error = %v", err) @@ -181,8 +181,8 @@ func TestCrossPlatformCoverageSkillPathCrossDeviceFailures(t *testing.T) { t.Run("staging publish failure", func(t *testing.T) { src, dst := makeSkillMoveFixture(t) - originalRename := skillPathRename - testseam.Swap(t, &skillPathRename, func(from, to string) error { + originalRename := skillPathRenameNoReplace + testseam.Swap(t, &skillPathRenameNoReplace, func(from, to string) error { if from == src && to == dst { return testCrossDeviceError() } @@ -402,8 +402,8 @@ func TestCrossPlatformCoverageVerifySkillPathCopyMismatches(t *testing.T) { func forceCrossDeviceRename(t *testing.T, src, dst string) { t.Helper() - originalRename := skillPathRename - testseam.Swap(t, &skillPathRename, func(from, to string) error { + originalRename := skillPathRenameNoReplace + testseam.Swap(t, &skillPathRenameNoReplace, func(from, to string) error { if from == src && to == dst { return testCrossDeviceError() } diff --git a/internal/upgrade/skill_publication.go b/internal/upgrade/skill_publication.go new file mode 100644 index 00000000..f4315435 --- /dev/null +++ b/internal/upgrade/skill_publication.go @@ -0,0 +1,153 @@ +package upgrade + +import ( + "crypto/sha256" + "errors" + "fmt" + "hash" + "os" + "path/filepath" + "sort" +) + +// SkillPathPublication records enough immutable identity to prove that a +// destination still belongs to the transaction that published it. The +// fingerprint is intentionally private so callers cannot forge records. +type SkillPathPublication struct { + Destination string + fingerprint [sha256.Size]byte + identity os.FileInfo +} + +// PublishSkillPathNoReplace atomically publishes a staged path without ever +// replacing a destination created after the backup phase. +func PublishSkillPathNoReplace(staged, destination string) (SkillPathPublication, error) { + identity, err := skillPathLstat(staged) + if err != nil { + return SkillPathPublication{}, fmt.Errorf("读取待发布 Skill 身份失败 %s: %w", staged, err) + } + fingerprint, err := fingerprintSkillPath(staged) + if err != nil { + return SkillPathPublication{}, fmt.Errorf("计算待发布 Skill 身份失败 %s: %w", staged, err) + } + if err := skillPathRenameNoReplace(staged, destination); err != nil { + return SkillPathPublication{}, fmt.Errorf("目标必须不存在的 Skill 发布失败 %s: %w", destination, err) + } + return SkillPathPublication{Destination: destination, fingerprint: fingerprint, identity: identity}, nil +} + +// RollbackSkillPathPublications removes only objects that can still be proven +// to have been published by this transaction. Each live path is first claimed +// into a private sibling quarantine. A concurrent replacement is restored when +// possible, otherwise retained in quarantine and reported explicitly. +func RollbackSkillPathPublications(publications []SkillPathPublication) error { + var rollbackErr error + for i := len(publications) - 1; i >= 0; i-- { + if err := rollbackSkillPathPublication(publications[i]); err != nil { + rollbackErr = errors.Join(rollbackErr, err) + } + } + return rollbackErr +} + +func rollbackSkillPathPublication(publication SkillPathPublication) (err error) { + destination := publication.Destination + quarantineRoot, err := skillPathMkdirTemp(filepath.Dir(destination), "."+filepath.Base(destination)+".rollback-") + if err != nil { + return fmt.Errorf("创建 Skill 回滚隔离目录失败 %s: %w", destination, err) + } + quarantine := filepath.Join(quarantineRoot, "payload") + cleanupRoot := func() error { + if cleanupErr := skillPathRemoveAll(quarantineRoot); cleanupErr != nil { + return fmt.Errorf("清理 Skill 回滚隔离目录失败 %s: %w", quarantineRoot, cleanupErr) + } + return nil + } + + if err := skillPathRename(destination, quarantine); err != nil { + cleanupErr := cleanupRoot() + if os.IsNotExist(err) { + return cleanupErr + } + return errors.Join(fmt.Errorf("隔离待回滚 Skill 失败 %s: %w", destination, err), cleanupErr) + } + + actualIdentity, identityErr := skillPathLstat(quarantine) + actual, fingerprintErr := fingerprintSkillPath(quarantine) + if identityErr == nil && fingerprintErr == nil && publication.identity != nil && + os.SameFile(publication.identity, actualIdentity) && actual == publication.fingerprint { + if removeErr := removePublishedSkillSource(quarantine); removeErr != nil { + return fmt.Errorf("移除事务发布的 Skill 失败 %s（隔离于 %s）: %w", destination, quarantine, removeErr) + } + return cleanupRoot() + } + + verificationErr := errors.Join(identityErr, fingerprintErr) + if verificationErr == nil { + verificationErr = errors.New("发布对象身份已变化") + } + if restoreErr := skillPathRenameNoReplace(quarantine, destination); restoreErr != nil { + return fmt.Errorf("拒绝删除非本事务 Skill %s: %v；并发对象保留于 %s，恢复原路径失败: %w", destination, verificationErr, quarantine, restoreErr) + } + return errors.Join( + fmt.Errorf("拒绝删除非本事务 Skill %s: %w", destination, verificationErr), + cleanupRoot(), + ) +} + +func fingerprintSkillPath(path string) ([sha256.Size]byte, error) { + h := sha256.New() + if err := fingerprintSkillPathInto(h, path); err != nil { + return [sha256.Size]byte{}, err + } + var result [sha256.Size]byte + copy(result[:], h.Sum(nil)) + return result, nil +} + +func fingerprintSkillPathInto(h hash.Hash, path string) error { + info, err := skillPathLstat(path) + if err != nil { + return err + } + writeFingerprintField(h, info.Mode().Type().String()) + writeFingerprintField(h, info.Mode().Perm().String()) + switch { + case info.IsDir(): + entries, err := skillPathReadDir(path) + if err != nil { + return err + } + sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) + writeFingerprintField(h, fmt.Sprintf("%d", len(entries))) + for _, entry := range entries { + writeFingerprintField(h, entry.Name()) + if err := fingerprintSkillPathInto(h, filepath.Join(path, entry.Name())); err != nil { + return err + } + } + return nil + case info.Mode()&os.ModeSymlink != 0: + target, err := skillPathReadlink(path) + if err != nil { + return err + } + writeFingerprintField(h, target) + return nil + case info.Mode().IsRegular(): + digest, err := skillPathFileDigest(path) + if err != nil { + return err + } + writeFingerprintField(h, fmt.Sprintf("%d", info.Size())) + _, err = h.Write(digest[:]) + return err + default: + return fmt.Errorf("不支持识别特殊 Skill 路径 %s（mode=%s）", path, info.Mode()) + } +} + +func writeFingerprintField(h hash.Hash, value string) { + _, _ = fmt.Fprintf(h, "%d:", len(value)) + _, _ = h.Write([]byte(value)) +} diff --git a/internal/upgrade/skill_publication_test.go b/internal/upgrade/skill_publication_test.go new file mode 100644 index 00000000..470b03cc --- /dev/null +++ b/internal/upgrade/skill_publication_test.go @@ -0,0 +1,314 @@ +package upgrade + +import ( + "crypto/sha256" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" +) + +func TestCrossPlatformCoverageSkillPublicationNoClobberAndOwnedRollback(t *testing.T) { + t.Run("existing destination is never replaced", func(t *testing.T) { + root := t.TempDir() + staged := filepath.Join(root, "staged") + destination := filepath.Join(root, "destination") + seedUpgradeSkill(t, staged, "new", false) + seedUpgradeSkill(t, destination, "concurrent", false) + + if _, err := PublishSkillPathNoReplace(staged, destination); err == nil { + t.Fatal("expected no-replace publication to fail") + } + assertUpgradeSkillContent(t, destination, "concurrent") + assertUpgradeSkillContent(t, staged, "new") + }) + + t.Run("owned publication is removed", func(t *testing.T) { + root := t.TempDir() + staged := filepath.Join(root, "staged") + destination := filepath.Join(root, "destination") + seedUpgradeSkill(t, staged, "new", false) + publication, err := PublishSkillPathNoReplace(staged, destination) + if err != nil { + t.Fatal(err) + } + if err := RollbackSkillPathPublications([]SkillPathPublication{publication}); err != nil { + t.Fatal(err) + } + if _, err := os.Lstat(destination); !os.IsNotExist(err) { + t.Fatalf("owned publication remains: %v", err) + } + }) + + t.Run("concurrent replacement is quarantined instead of deleted", func(t *testing.T) { + root := t.TempDir() + staged := filepath.Join(root, "staged") + destination := filepath.Join(root, "destination") + seedUpgradeSkill(t, staged, "new", false) + publication, err := PublishSkillPathNoReplace(staged, destination) + if err != nil { + t.Fatal(err) + } + if err := os.RemoveAll(destination); err != nil { + t.Fatal(err) + } + // Even a byte-for-byte identical replacement is not owned by this + // transaction: inode identity, not content alone, is authoritative. + seedUpgradeSkill(t, destination, "new", false) + + err = RollbackSkillPathPublications([]SkillPathPublication{publication}) + if err == nil || !strings.Contains(err.Error(), "拒绝删除非本事务") { + t.Fatalf("replacement rollback error = %v", err) + } + assertUpgradeSkillContent(t, destination, "new") + }) +} + +func TestCrossPlatformCoverageSkillPublicationFailureEdges(t *testing.T) { + failure := errors.New("injected publication failure") + + t.Run("publish identity", func(t *testing.T) { + if _, err := PublishSkillPathNoReplace(filepath.Join(t.TempDir(), "missing"), filepath.Join(t.TempDir(), "dest")); err == nil { + t.Fatal("expected identity failure") + } + }) + + t.Run("publish fingerprint", func(t *testing.T) { + root := t.TempDir() + staged := filepath.Join(root, "staged") + seedUpgradeSkill(t, staged, "value", false) + originalLstat := skillPathLstat + calls := 0 + testseam.Swap(t, &skillPathLstat, func(path string) (os.FileInfo, error) { + calls++ + if calls == 2 { + return nil, failure + } + return originalLstat(path) + }) + if _, err := PublishSkillPathNoReplace(staged, filepath.Join(root, "dest")); !errors.Is(err, failure) { + t.Fatal("expected fingerprint failure") + } + }) + + t.Run("rollback temp", func(t *testing.T) { + testseam.Swap(t, &skillPathMkdirTemp, func(string, string) (string, error) { return "", failure }) + if err := RollbackSkillPathPublications([]SkillPathPublication{{Destination: "dest"}}); !errors.Is(err, failure) { + t.Fatalf("temp error = %v", err) + } + }) + + t.Run("missing destination cleanup", func(t *testing.T) { + if err := RollbackSkillPathPublications([]SkillPathPublication{{Destination: filepath.Join(t.TempDir(), "missing")}}); err != nil { + t.Fatal(err) + } + }) + + t.Run("missing destination cleanup failure", func(t *testing.T) { + testseam.Swap(t, &skillPathRemoveAll, func(string) error { return failure }) + if err := RollbackSkillPathPublications([]SkillPathPublication{{Destination: filepath.Join(t.TempDir(), "missing")}}); !errors.Is(err, failure) { + t.Fatalf("cleanup error = %v", err) + } + }) + + t.Run("quarantine rename", func(t *testing.T) { + destination := filepath.Join(t.TempDir(), "destination") + seedUpgradeSkill(t, destination, "value", false) + testseam.Swap(t, &skillPathRename, func(string, string) error { return failure }) + if err := RollbackSkillPathPublications([]SkillPathPublication{{Destination: destination}}); !errors.Is(err, failure) { + t.Fatalf("quarantine error = %v", err) + } + }) + + t.Run("owned remove", func(t *testing.T) { + root := t.TempDir() + staged := filepath.Join(root, "staged") + destination := filepath.Join(root, "destination") + seedUpgradeSkill(t, staged, "value", false) + publication, err := PublishSkillPathNoReplace(staged, destination) + if err != nil { + t.Fatal(err) + } + originalRemove := skillPathRemoveAll + testseam.Swap(t, &skillPathRemoveAll, func(path string) error { + if filepath.Base(path) == "payload" { + return failure + } + return originalRemove(path) + }) + if err := RollbackSkillPathPublications([]SkillPathPublication{publication}); !errors.Is(err, failure) { + t.Fatalf("remove error = %v", err) + } + }) + + t.Run("quarantined fingerprint", func(t *testing.T) { + root := t.TempDir() + staged := filepath.Join(root, "staged") + destination := filepath.Join(root, "destination") + seedUpgradeSkill(t, staged, "value", false) + publication, err := PublishSkillPathNoReplace(staged, destination) + if err != nil { + t.Fatal(err) + } + originalLstat := skillPathLstat + testseam.Swap(t, &skillPathLstat, func(path string) (os.FileInfo, error) { + if filepath.Base(path) == "payload" { + return nil, failure + } + return originalLstat(path) + }) + if err := RollbackSkillPathPublications([]SkillPathPublication{publication}); !errors.Is(err, failure) { + t.Fatalf("fingerprint error = %v", err) + } + }) + + t.Run("mismatch restore and cleanup", func(t *testing.T) { + root := t.TempDir() + staged := filepath.Join(root, "staged") + destination := filepath.Join(root, "destination") + seedUpgradeSkill(t, staged, "published", false) + publication, err := PublishSkillPathNoReplace(staged, destination) + if err != nil { + t.Fatal(err) + } + if err := os.RemoveAll(destination); err != nil { + t.Fatal(err) + } + seedUpgradeSkill(t, destination, "replacement", false) + originalRemove := skillPathRemoveAll + testseam.Swap(t, &skillPathRemoveAll, func(path string) error { + if strings.Contains(filepath.Base(path), ".destination.rollback-") { + return failure + } + return originalRemove(path) + }) + if err := RollbackSkillPathPublications([]SkillPathPublication{publication}); !errors.Is(err, failure) { + t.Fatalf("mismatch cleanup error = %v", err) + } + assertUpgradeSkillContent(t, destination, "replacement") + }) + + t.Run("mismatch restore failure", func(t *testing.T) { + root := t.TempDir() + staged := filepath.Join(root, "staged") + destination := filepath.Join(root, "destination") + seedUpgradeSkill(t, staged, "published", false) + publication, err := PublishSkillPathNoReplace(staged, destination) + if err != nil { + t.Fatal(err) + } + if err := os.RemoveAll(destination); err != nil { + t.Fatal(err) + } + seedUpgradeSkill(t, destination, "replacement", false) + testseam.Swap(t, &skillPathRenameNoReplace, func(string, string) error { return failure }) + if err := RollbackSkillPathPublications([]SkillPathPublication{publication}); !errors.Is(err, failure) || !strings.Contains(err.Error(), "并发对象保留") { + t.Fatalf("mismatch restore error = %v", err) + } + }) + + t.Run("fingerprint read failures and lexical types", func(t *testing.T) { + t.Run("directory read", func(t *testing.T) { + dir := t.TempDir() + testseam.Swap(t, &skillPathReadDir, func(string) ([]os.DirEntry, error) { return nil, failure }) + if _, err := fingerprintSkillPath(dir); !errors.Is(err, failure) { + t.Fatalf("read dir error = %v", err) + } + }) + t.Run("recursive child", func(t *testing.T) { + dir := t.TempDir() + child := filepath.Join(dir, "child") + if err := os.WriteFile(child, []byte("value"), 0o644); err != nil { + t.Fatal(err) + } + originalLstat := skillPathLstat + testseam.Swap(t, &skillPathLstat, func(path string) (os.FileInfo, error) { + if path == child { + return nil, failure + } + return originalLstat(path) + }) + if _, err := fingerprintSkillPath(dir); !errors.Is(err, failure) { + t.Fatalf("child error = %v", err) + } + }) + t.Run("symlink read", func(t *testing.T) { + link := filepath.Join(t.TempDir(), "link") + if err := os.Symlink("missing", link); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + testseam.Swap(t, &skillPathReadlink, func(string) (string, error) { return "", failure }) + if _, err := fingerprintSkillPath(link); !errors.Is(err, failure) { + t.Fatalf("readlink error = %v", err) + } + }) + t.Run("file digest", func(t *testing.T) { + file := filepath.Join(t.TempDir(), "file") + if err := os.WriteFile(file, []byte("value"), 0o644); err != nil { + t.Fatal(err) + } + testseam.Swap(t, &skillPathOpen, func(string) (*os.File, error) { return nil, failure }) + if _, err := fingerprintSkillPath(file); !errors.Is(err, failure) { + t.Fatalf("digest error = %v", err) + } + }) + t.Run("special", func(t *testing.T) { + testseam.Swap(t, &skillPathLstat, func(string) (os.FileInfo, error) { + return skillPathFakeInfo{mode: os.ModeNamedPipe}, nil + }) + if err := fingerprintSkillPathInto(sha256.New(), "special"); err == nil { + t.Fatal("expected special path error") + } + }) + }) +} + +func TestCrossPlatformCoverageUpgradeRollbackRetainsConcurrentReplacement(t *testing.T) { + home := t.TempDir() + base := filepath.Join(home, ".agents", "skills") + first := filepath.Join(base, "dingtalk-first") + second := filepath.Join(base, "dingtalk-second") + seedUpgradeSkill(t, first, "old first", false) + seedUpgradeSkill(t, second, "old second", false) + stageRoot := filepath.Join(base, ".stage") + stagedFirst := filepath.Join(stageRoot, "dingtalk-first") + stagedSecond := filepath.Join(stageRoot, "dingtalk-second") + seedUpgradeSkill(t, stagedFirst, "new first", false) + seedUpgradeSkill(t, stagedSecond, "new second", false) + + failure := errors.New("injected second publication failure") + originalPublish := upgradePublishSkillPath + calls := 0 + testseam.Swap(t, &upgradePublishSkillPath, func(staged, destination string) (SkillPathPublication, error) { + calls++ + if calls == 2 { + if err := os.RemoveAll(first); err != nil { + t.Fatal(err) + } + seedUpgradeSkill(t, first, "concurrent", false) + return SkillPathPublication{}, failure + } + return originalPublish(staged, destination) + }) + + err := publishStagedSkillSet(home, []stagedSkillDir{ + {staged: stagedFirst, dest: first}, + {staged: stagedSecond, dest: second}, + }, []string{first, second}) + if !errors.Is(err, failure) || !strings.Contains(err.Error(), "拒绝删除非本事务") { + t.Fatalf("transaction error = %v", err) + } + assertUpgradeSkillContent(t, first, "concurrent") + assertUpgradeSkillContent(t, second, "old second") + matches, globErr := filepath.Glob(filepath.Join(home, ".dws", "skill-backups", "*", ".agents-skills-dingtalk-first", "SKILL.md")) + if globErr != nil || len(matches) != 1 { + t.Fatalf("retained old backup = %v, %v", matches, globErr) + } + content, readErr := os.ReadFile(matches[0]) + if readErr != nil || string(content) != "old first" { + t.Fatalf("retained backup content = %q, %v", content, readErr) + } +} diff --git a/internal/upgrade/skill_publish_rename_darwin.go b/internal/upgrade/skill_publish_rename_darwin.go new file mode 100644 index 00000000..06722ef1 --- /dev/null +++ b/internal/upgrade/skill_publish_rename_darwin.go @@ -0,0 +1,9 @@ +//go:build darwin + +package upgrade + +import "golang.org/x/sys/unix" + +func renameSkillPathNoReplace(source, destination string) error { + return unix.RenamexNp(source, destination, unix.RENAME_EXCL) +} diff --git a/internal/upgrade/skill_publish_rename_linux.go b/internal/upgrade/skill_publish_rename_linux.go new file mode 100644 index 00000000..7936592a --- /dev/null +++ b/internal/upgrade/skill_publish_rename_linux.go @@ -0,0 +1,9 @@ +//go:build linux + +package upgrade + +import "golang.org/x/sys/unix" + +func renameSkillPathNoReplace(source, destination string) error { + return unix.Renameat2(unix.AT_FDCWD, source, unix.AT_FDCWD, destination, unix.RENAME_NOREPLACE) +} diff --git a/internal/upgrade/skill_publish_rename_other.go b/internal/upgrade/skill_publish_rename_other.go new file mode 100644 index 00000000..bdd0986b --- /dev/null +++ b/internal/upgrade/skill_publish_rename_other.go @@ -0,0 +1,12 @@ +//go:build !linux && !darwin && !windows + +package upgrade + +import ( + "fmt" + "runtime" +) + +func renameSkillPathNoReplace(_, _ string) error { + return fmt.Errorf("当前平台 %s 不支持安全的原子 no-replace rename", runtime.GOOS) +} diff --git a/internal/upgrade/skill_publish_rename_windows.go b/internal/upgrade/skill_publish_rename_windows.go new file mode 100644 index 00000000..7ac5afb1 --- /dev/null +++ b/internal/upgrade/skill_publish_rename_windows.go @@ -0,0 +1,17 @@ +//go:build windows + +package upgrade + +import "golang.org/x/sys/windows" + +func renameSkillPathNoReplace(source, destination string) error { + sourcePtr, err := windows.UTF16PtrFromString(source) + if err != nil { + return err + } + destinationPtr, err := windows.UTF16PtrFromString(destination) + if err != nil { + return err + } + return windows.MoveFile(sourcePtr, destinationPtr) +} diff --git a/internal/upgrade/skill_publish_transaction_test.go b/internal/upgrade/skill_publish_transaction_test.go index 02afa793..0e372b28 100644 --- a/internal/upgrade/skill_publish_transaction_test.go +++ b/internal/upgrade/skill_publish_transaction_test.go @@ -92,8 +92,8 @@ func TestCrossPlatformCoverageMultiUpgradeTransactionPreservesOldSet(t *testing. t.Run("backup failure restores earlier victims", func(t *testing.T) { home, base, multiRoot, skills, skillSet := seedMultiUpgradeTarget(t) - originalRename := upgradeRename - testseam.Swap(t, &upgradeRename, func(src, dest string) error { + originalRename := skillPathRenameNoReplace + testseam.Swap(t, &skillPathRenameNoReplace, func(src, dest string) error { if src == filepath.Join(base, "dingtalk-b") { return failure } @@ -107,14 +107,14 @@ func TestCrossPlatformCoverageMultiUpgradeTransactionPreservesOldSet(t *testing. t.Run("publish failure rolls back complete set", func(t *testing.T) { home, base, multiRoot, skills, skillSet := seedMultiUpgradeTarget(t) - originalRename := upgradeRename + originalPublish := upgradePublishSkillPath failed := false - testseam.Swap(t, &upgradeRename, func(src, dest string) error { + testseam.Swap(t, &upgradePublishSkillPath, func(src, dest string) (SkillPathPublication, error) { if !failed && strings.Contains(src, ".dws-upgrade-multi-") && filepath.Base(dest) == "dingtalk-b" { failed = true - return failure + return SkillPathPublication{}, failure } - return originalRename(src, dest) + return originalPublish(src, dest) }) if err := publishMultiUpgradeTarget(home, base, multiRoot, skills, skillSet); !errors.Is(err, failure) { t.Fatalf("publish failure = %v, want injected failure", err) @@ -156,14 +156,14 @@ func TestCrossPlatformCoverageMonoUpgradeTransactionPreservesOldSet(t *testing.T t.Run("publish failure rolls back mono and multi", func(t *testing.T) { home, base, monoRoot := seed(t) - originalRename := upgradeRename + originalPublish := upgradePublishSkillPath failed := false - testseam.Swap(t, &upgradeRename, func(src, dest string) error { + testseam.Swap(t, &upgradePublishSkillPath, func(src, dest string) (SkillPathPublication, error) { if !failed && strings.Contains(src, ".dws-upgrade-mono-") && filepath.Base(dest) == "dws" { failed = true - return failure + return SkillPathPublication{}, failure } - return originalRename(src, dest) + return originalPublish(src, dest) }) if err := publishMonoUpgradeTarget(home, base, monoRoot); !errors.Is(err, failure) { t.Fatalf("publish failure = %v, want injected failure", err) @@ -211,10 +211,10 @@ func TestCrossPlatformCoverageSkillPublishTransactionFailureEdges(t *testing.T) }) t.Run("restore reports removal and directory failures", func(t *testing.T) { - testseam.Swap(t, &upgradeRemoveAll, func(string) error { return failure }) + testseam.Swap(t, &upgradeRollbackPublishedSkillPaths, func([]SkillPathPublication) error { return failure }) testseam.Swap(t, &upgradeMkdirAll, func(string, os.FileMode) error { return restoreFailure }) err := restoreSkillSet( - []string{filepath.Join(t.TempDir(), "published")}, + []SkillPathPublication{{Destination: filepath.Join(t.TempDir(), "published")}}, []backedUpSkillDir{{original: filepath.Join(t.TempDir(), "old"), backup: filepath.Join(t.TempDir(), "backup")}}, ) if !errors.Is(err, failure) || !errors.Is(err, restoreFailure) { @@ -223,7 +223,7 @@ func TestCrossPlatformCoverageSkillPublishTransactionFailureEdges(t *testing.T) }) t.Run("restore reports rename failure", func(t *testing.T) { - testseam.Swap(t, &upgradeRename, func(string, string) error { return restoreFailure }) + testseam.Swap(t, &skillPathRenameNoReplace, func(string, string) error { return restoreFailure }) err := restoreSkillSet(nil, []backedUpSkillDir{{original: filepath.Join(t.TempDir(), "old"), backup: filepath.Join(t.TempDir(), "backup")}}) if !errors.Is(err, restoreFailure) { t.Fatalf("restore rename error = %v", err) @@ -236,8 +236,8 @@ func TestCrossPlatformCoverageSkillPublishTransactionFailureEdges(t *testing.T) second := filepath.Join(home, "second") seedUpgradeSkill(t, first, "first", false) seedUpgradeSkill(t, second, "second", false) - originalRename := upgradeRename - testseam.Swap(t, &upgradeRename, func(src, dest string) error { + originalRename := skillPathRenameNoReplace + testseam.Swap(t, &skillPathRenameNoReplace, func(src, dest string) error { switch { case src == second: return failure @@ -259,16 +259,15 @@ func TestCrossPlatformCoverageSkillPublishTransactionFailureEdges(t *testing.T) staged := filepath.Join(home, "skills", ".stage", "dws") seedUpgradeSkill(t, old, "old", false) seedUpgradeSkill(t, staged, "new", false) - originalRename := upgradeRename - testseam.Swap(t, &upgradeRename, func(src, dest string) error { - switch { - case src == staged: - return failure - case strings.Contains(filepath.ToSlash(src), skillBackupSubdir): + testseam.Swap(t, &upgradePublishSkillPath, func(src, dest string) (SkillPathPublication, error) { + return SkillPathPublication{}, failure + }) + originalRename := skillPathRenameNoReplace + testseam.Swap(t, &skillPathRenameNoReplace, func(src, dest string) error { + if strings.Contains(filepath.ToSlash(src), skillBackupSubdir) { return restoreFailure - default: - return originalRename(src, dest) } + return originalRename(src, dest) }) err := publishStagedSkillSet(home, []stagedSkillDir{{staged: staged, dest: old}}, []string{old}) if !errors.Is(err, failure) || !errors.Is(err, restoreFailure) { diff --git a/test/scripts/install_js_smoke.mjs b/test/scripts/install_js_smoke.mjs index ba0ad970..8142f74e 100644 --- a/test/scripts/install_js_smoke.mjs +++ b/test/scripts/install_js_smoke.mjs @@ -287,7 +287,7 @@ function runCrossDeviceMoveContract() { } fs.renameSync(source, target); }, - publishRenameFn() { throw new Error("link publish failed"); }, + publishLinkFn() { throw new Error("link publish failed"); }, }), /link publish failed/, ); @@ -687,6 +687,75 @@ scenario("cache copy failure preserves the previous complete cache", () => { } }); +scenario("canonical link publication never clobbers or deletes concurrent user data", () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "dws-installjs-canonical-race-")); + try { + { + const home = path.join(tmp, "no-clobber-home"); + const canonical = path.join(home, ".agents", "skills"); + const base = path.join(home, ".cursor", "skills"); + const destination = path.join(base, "dingtalk-chat"); + writeFile(path.join(canonical, "dingtalk-chat", "SKILL.md"), "new\n"); + assert.throws( + () => publishCanonicalLinksAtomically(home, canonical, base, ["dingtalk-chat"], [], { + publishLinkFn(_source, target) { + writeFile(path.join(target, "concurrent-user-data.txt"), "must survive\n"); + throw Object.assign(new Error("concurrent destination exists"), { code: "EEXIST" }); + }, + }), + /concurrent destination exists/, + ); + assert.equal(fs.readFileSync(path.join(destination, "concurrent-user-data.txt"), "utf8"), "must survive\n"); + } + + { + const home = path.join(tmp, "rollback-home"); + const canonical = path.join(home, ".agents", "skills"); + const base = path.join(home, ".cursor", "skills"); + const first = path.join(base, "dingtalk-first"); + const second = path.join(base, "dingtalk-second"); + for (const name of ["dingtalk-first", "dingtalk-second"]) { + writeFile(path.join(canonical, name, "SKILL.md"), `new ${name}\n`); + writeFile(path.join(base, name, "SKILL.md"), `old ${name}\n`); + } + let publishCalls = 0; + assert.throws( + () => publishCanonicalLinksAtomically( + home, + canonical, + base, + ["dingtalk-first", "dingtalk-second"], + [first, second], + { + publishLinkFn(source, target) { + publishCalls += 1; + if (publishCalls === 1) { + const result = childProcess.spawnSync("ln", ["-P", source, target], { encoding: "utf8" }); + if (result.status !== 0) throw new Error(result.stderr || "ln -P failed"); + return; + } + fs.unlinkSync(first); + writeFile(path.join(first, "concurrent-user-data.txt"), "must survive\n"); + throw new Error("injected second canonical publish failure"); + }, + }, + ), + /concurrent object retained|rollback failed/, + ); + assert.equal(fs.readFileSync(path.join(first, "SKILL.md"), "utf8"), "old dingtalk-first\n"); + assert.equal(fs.readFileSync(path.join(second, "SKILL.md"), "utf8"), "old dingtalk-second\n"); + const retained = fs.readdirSync(base) + .filter((name) => name.startsWith(".dingtalk-first.rollback-")) + .map((name) => path.join(base, name, "payload", "concurrent-user-data.txt")) + .filter((candidate) => fs.existsSync(candidate)); + assert.equal(retained.length, 1, "concurrent replacement must be retained in quarantine"); + assert.equal(fs.readFileSync(retained[0], "utf8"), "must survive\n"); + } + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } +}); + scenario("multi set publish failure restores the complete previous set", () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "dws-installjs-multi-set-")); const home = path.join(tmp, "home");