From 7924e84fb67022d905006560d9c2fb2d7061ddef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Mon, 17 Aug 2026 17:33:55 +0800 Subject: [PATCH] fix(skills): fall back to copy when link publication fails Creating the staged symlink usually succeeds, so the link strategy really fails at publish time: renameSkillPathNoReplace has no atomic no-clobber primitive for a symlink source and refuses it whenever the kernel flag is unavailable (NFS, FUSE, overlayfs). Gating the copy fallback on staging alone therefore left every non-universal Agent unconfigured on exactly the filesystems the fallback exists to support. Retry the whole target transaction as a direct copy after a failure in any phase, but only when the failed attempt fully restored the originals. The converter also re-adds the replacement backups the link plan deliberately skips for destinations already pointing at canonical, which a copy must replace and no-replace publication would otherwise reject with EEXIST. --- internal/app/skill_setup.go | 160 ++++++++++++---- internal/app/skill_setup_canonical_test.go | 204 +++++++++++++++++++++ 2 files changed, 325 insertions(+), 39 deletions(-) diff --git a/internal/app/skill_setup.go b/internal/app/skill_setup.go index 344809c7..7a615734 100644 --- a/internal/app/skill_setup.go +++ b/internal/app/skill_setup.go @@ -188,6 +188,11 @@ const ( skillSetupBackupReplace = "same-name Skill" ) +// errSkillSetupStateUncertain marks a failure whose rollback could not fully +// restore the Agent directory. The target is left in a partially modified +// state, so no automatic retry may run over it. +var errSkillSetupStateUncertain = errors.New("Skill setup 状态不确定") + func newSkillSetupCommand() *cobra.Command { cmd := &cobra.Command{ Use: "setup", @@ -2010,7 +2015,7 @@ func backupSkillSetupTarget(home string, planned []skillSetupBackup, out io.Writ backup, err := skillSetupBackupAndRemove(home, item.Path) if err != nil { if restoreErr := restoreSkillSetupTarget(nil, backups); restoreErr != nil { - return nil, errors.Join(err, fmt.Errorf("恢复已备份 Skill 失败: %w", restoreErr)) + return nil, errors.Join(err, fmt.Errorf("恢复已备份 Skill 失败: %w", restoreErr), errSkillSetupStateUncertain) } return nil, err } @@ -2036,7 +2041,7 @@ func publishSkillSetupTarget(staged []skillSetupStagedDir, backups []skillSetupB 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 errors.Join(publishErr, fmt.Errorf("Skill setup 回滚不完整: %w", restoreErr), errSkillSetupStateUncertain) } return publishErr } @@ -2045,6 +2050,90 @@ func publishSkillSetupTarget(staged []skillSetupStagedDir, backups []skillSetupB return nil } +// skillSetupAttemptPhase identifies which stage of a single target transaction +// failed so the caller can keep its per-phase diagnostics. +type skillSetupAttemptPhase int + +const ( + skillSetupAttemptDone skillSetupAttemptPhase = iota + skillSetupAttemptStage + skillSetupAttemptBackup + skillSetupAttemptPublish +) + +// skillSetupCopyFallbackTarget converts a canonical-link target into a +// direct-copy target. buildSkillSetupPlan omits the replacement backup for a +// destination that already points at canonical, because linking would leave it +// untouched. A copy must replace that destination instead, and +// PublishSkillPathNoReplace refuses to overwrite it, so those paths are +// re-added before the retry. +func skillSetupCopyFallbackTarget(plan *skillSetupPlan, target skillSetupTargetPlan) (skillSetupTargetPlan, error) { + fallback := target + fallback.LinkCanonical = false + fallback.Backups = append([]skillSetupBackup(nil), target.Backups...) + seen := make(map[string]bool, len(fallback.Backups)) + for _, backup := range fallback.Backups { + seen[backup.Path] = true + } + replacements := []string{target.Destination} + if plan.Mode == skillSetupModeMulti { + replacements = make([]string, 0, len(plan.MultiSkillNames)) + for _, name := range plan.MultiSkillNames { + replacements = append(replacements, filepath.Join(target.Destination, name)) + } + } + for _, path := range replacements { + if seen[path] { + continue + } + if _, err := skillSetupLstat(path); err != nil { + if errors.Is(err, os.ErrNotExist) { + continue + } + return skillSetupTargetPlan{}, fmt.Errorf("检查将被替换的 Skill 失败 %s: %w", path, err) + } + seen[path] = true + fallback.Backups = append(fallback.Backups, skillSetupBackup{Path: path, Reason: skillSetupBackupReplace}) + } + sort.Slice(fallback.Backups, func(i, j int) bool { return fallback.Backups[i].Path < fallback.Backups[j].Path }) + return fallback, nil +} + +// runSkillSetupTargetAttempt performs one complete stage → backup → publish +// transaction for a single Agent target. On failure it reports the phase that +// failed; unless the error wraps errSkillSetupStateUncertain the original +// directories are restored, so the caller may retry with another strategy. +func runSkillSetupTargetAttempt( + plan *skillSetupPlan, + target skillSetupTargetPlan, + home string, + out, errOut io.Writer, +) ([]skillSetupStagedDir, skillSetupAttemptPhase, error) { + stageRoot, staged, stageErr := stageSkillSetupTarget(plan, target) + if stageErr != nil { + return nil, skillSetupAttemptStage, stageErr + } + backups, backupErr := backupSkillSetupTarget(home, target.Backups, out) + if backupErr != nil { + if cleanupErr := skillSetupRemoveAll(stageRoot); cleanupErr != nil { + backupErr = errors.Join(backupErr, fmt.Errorf("清理 Skill staging 失败 %s: %w", stageRoot, cleanupErr)) + } + return nil, skillSetupAttemptBackup, backupErr + } + publishErr := publishSkillSetupTarget(staged, backups) + cleanupErr := skillSetupRemoveAll(stageRoot) + if publishErr != nil { + if cleanupErr != nil { + publishErr = errors.Join(publishErr, fmt.Errorf("清理 Skill staging 失败 %s: %w", stageRoot, cleanupErr)) + } + return nil, skillSetupAttemptPublish, publishErr + } + if cleanupErr != nil { + fmt.Fprintf(errOut, " ⚠️ Skill staging 清理失败 %s: %v\n", stageRoot, cleanupErr) + } + return staged, skillSetupAttemptDone, nil +} + func executeSkillSetupPlan(plan *skillSetupPlan, out, errOut io.Writer) (installed, skipped int, err error) { home, homeErr := skillSetupUserHomeDir() hasCanonicalDependents := false @@ -2086,49 +2175,42 @@ func executeSkillSetupPlan(plan *skillSetupPlan, out, errOut io.Writer) (install continue } - stageRoot, staged, stageErr := stageSkillSetupTarget(plan, target) - if stageErr != nil && target.LinkCanonical { + staged, phase, attemptErr := runSkillSetupTargetAttempt(plan, target, home, out, errOut) + // A canonical link can fail either while staging (no symlink support) or + // while publishing (the no-replace rename fallback used on NFS, FUSE and + // overlayfs refuses a symlink source). Both mean links are unusable here, + // so retry the whole transaction as a direct copy — but only when the + // failed attempt left the original directories intact. + if attemptErr != nil && target.LinkCanonical && !errors.Is(attemptErr, errSkillSetupStateUncertain) { fmt.Fprintf(errOut, " ℹ️ %s 无法使用共享安装方式，正在自动改用兼容安装\n", target.Destination) - fallback := target - fallback.LinkCanonical = false - stageRoot, staged, stageErr = stageSkillSetupTarget(plan, fallback) + fallback, fallbackErr := skillSetupCopyFallbackTarget(plan, target) + if fallbackErr != nil { + attemptErr = errors.Join(attemptErr, fallbackErr) + } else { + staged, phase, attemptErr = runSkillSetupTargetAttempt(plan, fallback, home, out, errOut) + } } - if stageErr != nil { - if isCanonical && hasCanonicalDependents { - return installed, skipped + perTarget, fmt.Errorf("canonical Skill staging 失败 %s: %w", target.Destination, stageErr) + if attemptErr != nil { + switch phase { + case skillSetupAttemptBackup: + if isCanonical && hasCanonicalDependents { + return installed, skipped + perTarget, fmt.Errorf("canonical Skill 备份失败，已执行回滚 %s: %w", target.Destination, attemptErr) + } + fmt.Fprintf(errOut, " ✗ Skill 备份失败，已执行回滚，跳过整个 Agent 目标 %s: %v\n", target.Destination, attemptErr) + case skillSetupAttemptPublish: + if isCanonical && hasCanonicalDependents { + return installed, skipped + perTarget, fmt.Errorf("canonical Skill 发布失败，已执行回滚 %s: %w", target.Destination, attemptErr) + } + fmt.Fprintf(errOut, " ✗ Skill 发布失败，已执行回滚，跳过整个 Agent 目标 %s: %v\n", target.Destination, attemptErr) + default: + if isCanonical && hasCanonicalDependents { + return installed, skipped + perTarget, fmt.Errorf("canonical Skill staging 失败 %s: %w", target.Destination, attemptErr) + } + fmt.Fprintf(errOut, " ✗ Skill staging 失败，保留原集合 %s: %v\n", target.Destination, attemptErr) } - fmt.Fprintf(errOut, " ✗ Skill staging 失败，保留原集合 %s: %v\n", target.Destination, stageErr) skipped += perTarget continue } - backups, backupErr := backupSkillSetupTarget(home, target.Backups, out) - if backupErr != nil { - if cleanupErr := skillSetupRemoveAll(stageRoot); cleanupErr != nil { - backupErr = errors.Join(backupErr, fmt.Errorf("清理 Skill staging 失败 %s: %w", stageRoot, cleanupErr)) - } - if isCanonical && hasCanonicalDependents { - return installed, skipped + perTarget, fmt.Errorf("canonical Skill 备份失败，已执行回滚 %s: %w", target.Destination, backupErr) - } - fmt.Fprintf(errOut, " ✗ Skill 备份失败，已执行回滚，跳过整个 Agent 目标 %s: %v\n", target.Destination, backupErr) - skipped += perTarget - continue - } - publishErr := publishSkillSetupTarget(staged, backups) - cleanupErr := skillSetupRemoveAll(stageRoot) - if publishErr != nil { - if cleanupErr != nil { - publishErr = errors.Join(publishErr, fmt.Errorf("清理 Skill staging 失败 %s: %w", stageRoot, cleanupErr)) - } - if isCanonical && hasCanonicalDependents { - return installed, skipped + perTarget, fmt.Errorf("canonical Skill 发布失败，已执行回滚 %s: %w", target.Destination, publishErr) - } - fmt.Fprintf(errOut, " ✗ Skill 发布失败，已执行回滚，跳过整个 Agent 目标 %s: %v\n", target.Destination, publishErr) - skipped += perTarget - continue - } - if cleanupErr != nil { - fmt.Fprintf(errOut, " ⚠️ Skill staging 清理失败 %s: %v\n", stageRoot, cleanupErr) - } for _, item := range staged { fmt.Fprintf(out, " ✓ %s\n", item.dest) } diff --git a/internal/app/skill_setup_canonical_test.go b/internal/app/skill_setup_canonical_test.go index 1932e55b..246739f2 100644 --- a/internal/app/skill_setup_canonical_test.go +++ b/internal/app/skill_setup_canonical_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 TestCrossPlatformCoverageSkillSetupCanonicalTargetsAndAgentCapabilities(t *testing.T) { @@ -270,6 +271,209 @@ func TestCrossPlatformCoverageSkillSetupCanonicalCopyFallbackMessage(t *testing. } } +// Publishing a staged symlink has no atomic no-clobber primitive, so +// renameSkillPathNoReplace refuses it whenever the kernel flag is unavailable +// (NFS, FUSE, overlayfs). Creating the link still succeeds, so the failure +// surfaces at publish time and must still degrade to a direct copy. +func rejectSymlinkPublish(t *testing.T) { + t.Helper() + realPublish := skillSetupPublishPath + testseam.Swap(t, &skillSetupPublishPath, func(staged, dest string) (upgrade.SkillPathPublication, error) { + if info, err := os.Lstat(staged); err == nil && info.Mode()&os.ModeSymlink != 0 { + return upgrade.SkillPathPublication{}, errors.New("平台不支持原子 no-replace rename: 源路径类型不支持降级发布") + } + return realPublish(staged, dest) + }) +} + +func TestCrossPlatformCoverageSkillSetupCopyFallbackWhenPublishRejectsSymlink(t *testing.T) { + home := t.TempDir() + canonical := filepath.Join(home, ".agents", "skills") + claude := filepath.Join(home, ".claude", "skills") + src := t.TempDir() + skillSrc := filepath.Join(src, "dingtalk-chat") + if err := os.MkdirAll(skillSrc, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillSrc, "SKILL.md"), []byte("chat"), 0o644); err != nil { + t.Fatal(err) + } + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + rejectSymlinkPublish(t) + + plan, err := buildSkillSetupPlan(skillSetupModeMulti, src, []string{canonical, claude}, []string{"dingtalk-chat"}, false) + if err != nil { + t.Fatal(err) + } + var out, errOut bytes.Buffer + installed, skipped, err := executeSkillSetupPlan(plan, &out, &errOut) + if err != nil || installed != 2 || skipped != 0 { + t.Fatalf("execute = installed %d skipped %d err %v; stderr=%s", installed, skipped, err, errOut.String()) + } + if !strings.Contains(errOut.String(), "自动改用兼容安装") { + t.Fatalf("human-readable fallback message missing: %s", errOut.String()) + } + info, err := os.Lstat(filepath.Join(claude, "dingtalk-chat")) + if err != nil || !info.IsDir() || info.Mode()&os.ModeSymlink != 0 { + t.Fatalf("copy fallback = %#v, %v", info, err) + } + body, err := os.ReadFile(filepath.Join(claude, "dingtalk-chat", "SKILL.md")) + if err != nil || string(body) != "chat" { + t.Fatalf("copied payload = %q, %v", body, err) + } +} + +func TestCrossPlatformCoverageSkillSetupCopyFallbackReplacesExistingCanonicalLink(t *testing.T) { + home := t.TempDir() + canonical := filepath.Join(home, ".agents", "skills") + claude := filepath.Join(home, ".claude", "skills") + src := t.TempDir() + names := []string{"dingtalk-chat", "dingtalk-doc"} + for _, name := range names { + dir := filepath.Join(src, name) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(name), 0o644); err != nil { + t.Fatal(err) + } + } + // Canonical already holds one Skill and Claude already links to it, so the + // link plan records no backup for that destination. + if err := os.MkdirAll(filepath.Join(canonical, "dingtalk-chat"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(canonical, "dingtalk-chat", "SKILL.md"), []byte("dingtalk-chat"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(claude, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join("..", "..", ".agents", "skills", "dingtalk-chat"), filepath.Join(claude, "dingtalk-chat")); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + rejectSymlinkPublish(t) + + plan, err := buildSkillSetupPlan(skillSetupModeMulti, src, []string{canonical, claude}, names, false) + if err != nil { + t.Fatal(err) + } + var out, errOut bytes.Buffer + installed, skipped, err := executeSkillSetupPlan(plan, &out, &errOut) + if err != nil || skipped != 0 { + t.Fatalf("execute = installed %d skipped %d err %v; stderr=%s", installed, skipped, err, errOut.String()) + } + for _, name := range names { + path := filepath.Join(claude, name) + info, statErr := os.Lstat(path) + if statErr != nil || !info.IsDir() || info.Mode()&os.ModeSymlink != 0 { + t.Fatalf("%s = %#v, %v; stderr=%s", path, info, statErr, errOut.String()) + } + body, readErr := os.ReadFile(filepath.Join(path, "SKILL.md")) + if readErr != nil || string(body) != name { + t.Fatalf("%s payload = %q, %v", path, body, readErr) + } + } +} + +func TestCrossPlatformCoverageSkillSetupCopyFallbackKeepsPlannedBackups(t *testing.T) { + home := t.TempDir() + canonical := filepath.Join(home, ".agents", "skills") + claude := filepath.Join(home, ".claude", "skills") + src := t.TempDir() + names := []string{"dingtalk-chat", "dingtalk-doc"} + for _, name := range names { + dir := filepath.Join(src, name) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(name), 0o644); err != nil { + t.Fatal(err) + } + // Claude already holds a real private copy of every bundled Skill, so the + // plan records a replacement backup for each one. + old := filepath.Join(claude, name) + if err := os.MkdirAll(old, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(old, "SKILL.md"), []byte("stale"), 0o644); err != nil { + t.Fatal(err) + } + } + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + rejectSymlinkPublish(t) + + plan, err := buildSkillSetupPlan(skillSetupModeMulti, src, []string{canonical, claude}, names, false) + if err != nil { + t.Fatal(err) + } + var out, errOut bytes.Buffer + installed, skipped, err := executeSkillSetupPlan(plan, &out, &errOut) + if err != nil || skipped != 0 { + t.Fatalf("execute = installed %d skipped %d err %v; stderr=%s", installed, skipped, err, errOut.String()) + } + for _, name := range names { + body, readErr := os.ReadFile(filepath.Join(claude, name, "SKILL.md")) + if readErr != nil || string(body) != name { + t.Fatalf("%s payload = %q, %v", name, body, readErr) + } + } +} + +func TestCrossPlatformCoverageSkillSetupCopyFallbackReportsUnreadableReplacement(t *testing.T) { + home := t.TempDir() + canonical := filepath.Join(home, ".agents", "skills") + claude := filepath.Join(home, ".claude", "skills") + src := t.TempDir() + names := []string{"dingtalk-chat", "dingtalk-doc"} + for _, name := range names { + dir := filepath.Join(src, name) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(name), 0o644); err != nil { + t.Fatal(err) + } + } + if err := os.MkdirAll(filepath.Join(canonical, "dingtalk-chat"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(canonical, "dingtalk-chat", "SKILL.md"), []byte("dingtalk-chat"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(claude, 0o755); err != nil { + t.Fatal(err) + } + linked := filepath.Join(claude, "dingtalk-chat") + if err := os.Symlink(filepath.Join("..", "..", ".agents", "skills", "dingtalk-chat"), linked); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil }) + rejectSymlinkPublish(t) + // The already-linked destination carries no planned backup, so the converter + // has to stat it; an error other than "not exist" must abort the retry. + testseam.Swap(t, &skillSetupLstat, func(path string) (os.FileInfo, error) { + if path == linked { + return nil, os.ErrPermission + } + return os.Lstat(path) + }) + + plan, err := buildSkillSetupPlan(skillSetupModeMulti, src, []string{canonical, claude}, names, false) + if err != nil { + t.Fatal(err) + } + var out, errOut bytes.Buffer + installed, skipped, err := executeSkillSetupPlan(plan, &out, &errOut) + if err != nil || installed != 2 || skipped != 2 { + t.Fatalf("execute = installed %d skipped %d err %v; stderr=%s", installed, skipped, err, errOut.String()) + } + if !strings.Contains(errOut.String(), "检查将被替换的 Skill 失败") { + t.Fatalf("converter failure not reported: %s", errOut.String()) + } +} + func TestCrossPlatformCoverageUpstreamAgentEnumerationAndEffectiveRoots(t *testing.T) { home := t.TempDir() testseam.Swap(t, &skillSetupUserHomeDir, func() (string, error) { return home, nil })