From 7b64576ea1966ab4f59f045f3e79a1cdcac539e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Fri, 14 Aug 2026 15:35:25 +0800 Subject: [PATCH] fix(skills): protect shell link rollback from races --- scripts/install-skills.sh | 77 +++++++++++++++++++++-- scripts/install.sh | 83 +++++++++++++++++++++++-- test/scripts/install_script_test.go | 95 +++++++++++++++++++++++++++++ 3 files changed, 243 insertions(+), 12 deletions(-) diff --git a/scripts/install-skills.sh b/scripts/install-skills.sh index 4a7c486b..33f80429 100755 --- a/scripts/install-skills.sh +++ b/scripts/install-skills.sh @@ -238,6 +238,49 @@ restore_multi_skill_set() { [ "$_rms_ok" -eq 1 ] } +# A link publication manifest stores destination/target/inode triples. Rollback +# only removes links that still have the identity created by this transaction; +# a path concurrently replaced with a file, directory, or new link is preserved. +skill_link_inode() { + _sli_entry="$(ls -di "$1" 2>/dev/null)" || return 1 + printf '%s\n' "$_sli_entry" | awk '{print $1}' +} + +skill_link_matches() { + [ -L "$1" ] || return 1 + _slm_target="$(readlink "$1" 2>/dev/null)" || return 1 + [ "$_slm_target" = "$2" ] || return 1 + _slm_expected_inode="${3-}" + [ -z "$_slm_expected_inode" ] || [ "$(skill_link_inode "$1")" = "$_slm_expected_inode" ] +} + +restore_linked_skill_set() { + _rls_published="$1" + _rls_backups="$2" + _rls_ok=1 + if [ -f "$_rls_published" ]; then + while IFS= read -r _rls_dest && IFS= read -r _rls_target && IFS= read -r _rls_inode; do + [ -n "$_rls_dest" ] || continue + if skill_link_matches "$_rls_dest" "$_rls_target" "$_rls_inode"; then + rm -f "$_rls_dest" || _rls_ok=0 + else + printf ' ⚠️ 跳过回滚已被并发修改的 Skill 路径: %s\n' "$_rls_dest" + _rls_ok=0 + fi + done < "$_rls_published" + fi + restore_multi_skill_set /dev/null "$_rls_backups" || _rls_ok=0 + [ "$_rls_ok" -eq 1 ] +} + +cleanup_nested_staged_link() { + _cnsl_nested="$1/$2" + _cnsl_target="$3" + if skill_link_matches "$_cnsl_nested" "$_cnsl_target"; then + rm -f "$_cnsl_nested" + fi +} + # publish_skill_cache # Stages a complete sibling cache before publishing it. Any copy or publish # failure leaves the previous cache in place (or in the reported recovery dir @@ -537,6 +580,7 @@ link_canonical_skills_to_base() { _lcs_base_real="$(CDPATH= cd -- "$_lcs_base" && pwd -P)" || return 1 _lcs_stage="$(mktemp -d "$_lcs_base/.dws-link-set.XXXXXX")" || return 1 _lcs_backups="$_lcs_stage/.backups"; _lcs_published="$_lcs_stage/.published" + _lcs_stage_token="$(basename "$_lcs_stage")" : > "$_lcs_backups" || { rm -rf "$_lcs_stage"; return 1; } : > "$_lcs_published" || { rm -rf "$_lcs_stage"; return 1; } if [ "$_lcs_mode" = "mono" ]; then @@ -553,7 +597,8 @@ link_canonical_skills_to_base() { if same_physical_skill_root "$_lcs_base/$_lcs_name" "$_lcs_canonical/$_lcs_name"; then continue; fi _lcs_target_real="$(CDPATH= cd -- "$_lcs_canonical/$_lcs_name" && pwd -P)" || { rm -rf "$_lcs_stage"; return 1; } _lcs_link_target="$(awk -v from="$_lcs_base_real" -v to="$_lcs_target_real" 'BEGIN { nf=split(from,f,"/"); nt=split(to,t,"/"); i=1; while(i<=nf&&i<=nt&&f[i]==t[i])i++; out=""; for(j=i;j<=nf;j++)if(f[j]!="")out=out"../"; for(j=i;j<=nt;j++)if(t[j]!="")out=out t[j](j> "$_lcs_published" || { - restore_multi_skill_set "$_lcs_published" "$_lcs_backups" || true; rm -rf "$_lcs_stage"; return 1; + _lcs_dest="$_lcs_base/$_lcs_name" + _lcs_stage_name="${_lcs_stage_token}.${_lcs_name}" + _lcs_staged="$_lcs_stage/$_lcs_stage_name" + _lcs_link_target="$(readlink "$_lcs_staged" 2>/dev/null)" || { + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true; rm -rf "$_lcs_stage"; return 1 } - if ! mv "$_lcs_stage/$_lcs_name" "$_lcs_base/$_lcs_name"; then - restore_multi_skill_set "$_lcs_published" "$_lcs_backups" || true; rm -rf "$_lcs_stage"; return 1 + # Hard-link the staged symlink itself. This never overwrites a + # concurrently-created destination. If that destination became a + # directory, the unique staged basename can only be linked inside it; + # post-validation detects that case and removes only our link. + if ! ln -P "$_lcs_staged" "$_lcs_dest" 2>/dev/null || ! skill_link_matches "$_lcs_dest" "$_lcs_link_target"; then + cleanup_nested_staged_link "$_lcs_dest" "$_lcs_stage_name" "$_lcs_link_target" || true + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true; rm -rf "$_lcs_stage"; return 1 fi - printf ' ↪ Skills → %s\n' "$_lcs_base/$_lcs_name" + _lcs_inode="$(skill_link_inode "$_lcs_dest")" || { + if skill_link_matches "$_lcs_dest" "$_lcs_link_target"; then rm -f "$_lcs_dest" || true; fi + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true; rm -rf "$_lcs_stage"; return 1 + } + if ! rm -f "$_lcs_staged"; then + if skill_link_matches "$_lcs_dest" "$_lcs_link_target" "$_lcs_inode"; then rm -f "$_lcs_dest" || true; fi + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true; rm -rf "$_lcs_stage"; return 1 + fi + if ! printf '%s\n%s\n%s\n' "$_lcs_dest" "$_lcs_link_target" "$_lcs_inode" >> "$_lcs_published"; then + if skill_link_matches "$_lcs_dest" "$_lcs_link_target" "$_lcs_inode"; then rm -f "$_lcs_dest" || true; fi + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true; rm -rf "$_lcs_stage"; return 1 + fi + printf ' ↪ Skills → %s\n' "$_lcs_dest" done rm -rf "$_lcs_stage" } diff --git a/scripts/install.sh b/scripts/install.sh index b494a402..a6586676 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -257,6 +257,49 @@ restore_multi_skill_set() { [ "$_rms_ok" -eq 1 ] } +# A link publication manifest stores destination/target/inode triples. Rollback +# only removes links that still have the identity created by this transaction; +# a path concurrently replaced with a file, directory, or new link is preserved. +skill_link_inode() { + _sli_entry="$(ls -di "$1" 2>/dev/null)" || return 1 + printf '%s\n' "$_sli_entry" | awk '{print $1}' +} + +skill_link_matches() { + [ -L "$1" ] || return 1 + _slm_target="$(readlink "$1" 2>/dev/null)" || return 1 + [ "$_slm_target" = "$2" ] || return 1 + _slm_expected_inode="${3-}" + [ -z "$_slm_expected_inode" ] || [ "$(skill_link_inode "$1")" = "$_slm_expected_inode" ] +} + +restore_linked_skill_set() { + _rls_published="$1" + _rls_backups="$2" + _rls_ok=1 + if [ -f "$_rls_published" ]; then + while IFS= read -r _rls_dest && IFS= read -r _rls_target && IFS= read -r _rls_inode; do + [ -n "$_rls_dest" ] || continue + if skill_link_matches "$_rls_dest" "$_rls_target" "$_rls_inode"; then + rm -f "$_rls_dest" || _rls_ok=0 + else + say " ⚠️ 跳过回滚已被并发修改的 Skill 路径: $_rls_dest" + _rls_ok=0 + fi + done < "$_rls_published" + fi + restore_multi_skill_set /dev/null "$_rls_backups" || _rls_ok=0 + [ "$_rls_ok" -eq 1 ] +} + +cleanup_nested_staged_link() { + _cnsl_nested="$1/$2" + _cnsl_target="$3" + if skill_link_matches "$_cnsl_nested" "$_cnsl_target"; then + rm -f "$_cnsl_nested" + fi +} + # publish_skill_cache # Copies a complete cache into a sibling staging directory, then publishes it # with rename. Copy/publish failures retain the previous cache; if restoration @@ -773,6 +816,7 @@ link_canonical_skills_to_base() { _lcs_stage="$(mktemp -d "$_lcs_base/.dws-link-set.XXXXXX")" || return 1 _lcs_backups="$_lcs_stage/.backups" _lcs_published="$_lcs_stage/.published" + _lcs_stage_token="$(basename "$_lcs_stage")" : > "$_lcs_backups" || { rm -rf "$_lcs_stage"; return 1; } : > "$_lcs_published" || { rm -rf "$_lcs_stage"; return 1; } if [ "$_lcs_mode" = "mono" ]; then @@ -791,7 +835,8 @@ link_canonical_skills_to_base() { fi _lcs_target_real="$(CDPATH= cd -- "$_lcs_canonical/$_lcs_name" && pwd -P)" || { rm -rf "$_lcs_stage"; return 1; } _lcs_link_target="$(awk -v from="$_lcs_base_real" -v to="$_lcs_target_real" 'BEGIN { nf=split(from,f,"/"); nt=split(to,t,"/"); i=1; while(i<=nf&&i<=nt&&f[i]==t[i])i++; out=""; for(j=i;j<=nf;j++)if(f[j]!="")out=out"../"; for(j=i;j<=nt;j++)if(t[j]!="")out=out t[j](j> "$_lcs_published" || { - restore_multi_skill_set "$_lcs_published" "$_lcs_backups" || true + _lcs_dest="$_lcs_base/$_lcs_name" + _lcs_stage_name="${_lcs_stage_token}.${_lcs_name}" + _lcs_staged="$_lcs_stage/$_lcs_stage_name" + _lcs_link_target="$(readlink "$_lcs_staged" 2>/dev/null)" || { + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true rm -rf "$_lcs_stage" return 1 } - if ! mv "$_lcs_stage/$_lcs_name" "$_lcs_base/$_lcs_name"; then - restore_multi_skill_set "$_lcs_published" "$_lcs_backups" || true + # Hard-link the staged symlink itself. This never overwrites a + # concurrently-created destination. If that destination became a + # directory, the unique staged basename can only be linked inside it; + # post-validation detects that case and removes only our link. + if ! ln -P "$_lcs_staged" "$_lcs_dest" 2>/dev/null || ! skill_link_matches "$_lcs_dest" "$_lcs_link_target"; then + cleanup_nested_staged_link "$_lcs_dest" "$_lcs_stage_name" "$_lcs_link_target" || true + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true rm -rf "$_lcs_stage" return 1 fi - say " ↪ Skills → $_lcs_base/$_lcs_name" + _lcs_inode="$(skill_link_inode "$_lcs_dest")" || { + if skill_link_matches "$_lcs_dest" "$_lcs_link_target"; then rm -f "$_lcs_dest" || true; fi + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true + rm -rf "$_lcs_stage" + return 1 + } + if ! rm -f "$_lcs_staged"; then + if skill_link_matches "$_lcs_dest" "$_lcs_link_target" "$_lcs_inode"; then rm -f "$_lcs_dest" || true; fi + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true + rm -rf "$_lcs_stage" + return 1 + fi + if ! printf '%s\n%s\n%s\n' "$_lcs_dest" "$_lcs_link_target" "$_lcs_inode" >> "$_lcs_published"; then + if skill_link_matches "$_lcs_dest" "$_lcs_link_target" "$_lcs_inode"; then rm -f "$_lcs_dest" || true; fi + restore_linked_skill_set "$_lcs_published" "$_lcs_backups" || true + rm -rf "$_lcs_stage" + return 1 + fi + say " ↪ Skills → $_lcs_dest" done rm -rf "$_lcs_stage" } diff --git a/test/scripts/install_script_test.go b/test/scripts/install_script_test.go index 44620d0a..d23f5e44 100644 --- a/test/scripts/install_script_test.go +++ b/test/scripts/install_script_test.go @@ -1503,6 +1503,101 @@ func TestInstallerShellLinksZCodeRootToCanonical(t *testing.T) { } } +func TestInstallerShellLinkPublicationRacePreservesConcurrentDirectories(t *testing.T) { + for _, scriptName := range []string{"install.sh", "install-skills.sh"} { + scriptName := scriptName + t.Run(scriptName, func(t *testing.T) { + t.Parallel() + 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) + + home := t.TempDir() + base := filepath.Join(home, ".zcode", "skills") + first := filepath.Join(base, "dingtalk-first") + second := filepath.Join(base, "dingtalk-second") + mustWriteFile(t, filepath.Join(home, ".agents", "skills", "dingtalk-first", "SKILL.md"), []byte("first\n"), 0o644) + mustWriteFile(t, filepath.Join(home, ".agents", "skills", "dingtalk-second", "SKILL.md"), []byte("second\n"), 0o644) + + harness := `. "$DWS_TEST_LIBRARY" +ln() { + if [ "$1" = -P ] && [ "$3" = "$DWS_TEST_SECOND" ]; then + rm -f "$DWS_TEST_FIRST" + mkdir -p "$DWS_TEST_FIRST" "$DWS_TEST_SECOND" + printf '%s\n' first-user-data > "$DWS_TEST_FIRST/user.txt" + printf '%s\n' second-user-data > "$DWS_TEST_SECOND/user.txt" + fi + command ln "$@" +} +if link_canonical_skills_to_base "$HOME" "$DWS_TEST_BASE" multi; then + exit 2 +fi +identity_dest="$DWS_TEST_BASE/identity-link" +identity_manifest="$HOME/identity.manifest" +command ln -s ../same-target "$identity_dest" +identity_inode="$(skill_link_inode "$identity_dest")" +printf '%s\n%s\n%s\n' "$identity_dest" ../same-target "$identity_inode" > "$identity_manifest" +rm -f "$identity_dest" +command ln -s ../same-target "$identity_dest" +if restore_linked_skill_set "$identity_manifest" /dev/null; then + exit 3 +fi +` + cmd := exec.Command("sh", "-c", harness) + cmd.Env = append(os.Environ(), + "HOME="+home, + "DWS_TEST_LIBRARY="+library, + "DWS_TEST_BASE="+base, + "DWS_TEST_FIRST="+first, + "DWS_TEST_SECOND="+second, + ) + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("%s race harness failed: %v\n%s", scriptName, err, output) + } + if !strings.Contains(string(output), "跳过回滚已被并发修改的 Skill 路径: "+first) { + t.Fatalf("%s did not report identity-protected rollback:\n%s", scriptName, output) + } + identityDest := filepath.Join(base, "identity-link") + if target, err := os.Readlink(identityDest); err != nil || target != "../same-target" { + t.Fatalf("%s replacement link was removed: target=%q, err=%v", scriptName, target, err) + } + for path, want := range map[string]string{ + filepath.Join(first, "user.txt"): "first-user-data\n", + filepath.Join(second, "user.txt"): "second-user-data\n", + } { + got, err := os.ReadFile(path) + if err != nil || string(got) != want { + t.Fatalf("%s concurrent data at %s = %q, err=%v", scriptName, path, got, err) + } + } + for _, dir := range []string{first, second} { + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("%s read concurrent directory %s: %v", scriptName, dir, err) + } + if len(entries) != 1 || entries[0].Name() != "user.txt" { + t.Fatalf("%s transaction artifacts remain in %s: %v", scriptName, dir, entries) + } + } + if matches, err := filepath.Glob(filepath.Join(base, ".dws-link-set.*")); err != nil || len(matches) != 0 { + t.Fatalf("%s staging leftovers = %v, err=%v", scriptName, matches, err) + } + }) + } +} + func TestInstallerShellUsesUpstreamXDGAndCustomAgentRoots(t *testing.T) { for _, scriptName := range []string{"install.sh", "install-skills.sh"} { t.Run(scriptName, func(t *testing.T) {