Merge branch 'safe-export' of bajdev/mpckit into main

docs/review-fixes-plan.md

@@ -14,6 +14,8 @@ These were settled with the user on 2026-11-12. Agents follow them and do not re
 | Releases | 0.5.2 after PRs 1-3, 0.5.3 after PRs 5-8. PRs in between do not change the version. |
 | CI (S6) | Out of scope. No CI files, no `task check`. |
 | PR records (R8) | Keep the current layout. R8 is dropped. |
+| Linked WAVs (PR 3) | A WAV the user picks that is a link is followed at assign: the pool file is named after the link, and its source is the file the link points to. Export refuses any source that is a link or sits in a linked folder. Decided 2026-11-12. |
+| New sample names (PR 3) | Besides `/`, `\` and control characters, the characters the device's FAT card forbids (`: * ? " < > \|`) also become `_`, and a name Windows reserves for a device (`CON.wav`, `aux.wav`, `com1.wav`) gets `_` added (`CON_.wav`). Decided 2026-11-12 and 2026-11-13. |
 
 Open: none. An agent that meets a choice this plan does not settle stops and asks bajdev in the PR record's Notes, instead of guessing.
 

internal/app/app.go

@@ -8,7 +8,6 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
-	"os"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -223,11 +222,10 @@ func (s *Service) state() State {
 			MaxBars:      project.MaxBars,
 		},
 	}
-	missing := map[string]bool{} // pool file -> source missing
+	missing := map[string]bool{} // pool file -> source missing, as export sees it
 	for _, f := range p.PoolFiles() {
 		src := p.Sources[f]
-		info, err := os.Stat(src)
-		missing[f] = src == "" || err != nil || !info.Mode().IsRegular()
+		missing[f] = src == "" || !export.Usable(src)
 	}
 	for slot := 0; slot < project.Pads; slot++ {
 		v, err := p.Pad(slot)

internal/app/app_test.go

@@ -510,3 +510,21 @@ func TestCloseWindow(t *testing.T) {
 		t.Error("closer not called")
 	}
 }
+
+func TestLinkedFileIsMissing(t *testing.T) {
+	// A link in the data folder is shown as missing, as export treats it.
+	dir := t.TempDir()
+	path := copyProject(t, dir)
+	snare := filepath.Join(dir, "DemoFile"+project.DataSuffix, "80s-Snr-Dreamer SA.wav")
+	outside := filepath.Join(dir, "outside.wav")
+	must(t, os.Rename(snare, outside))
+	if err := os.Symlink(outside, snare); err != nil {
+		t.Skipf("cannot create a symlink: %v", err)
+	}
+	s, _ := newService()
+	res, err := s.LoadPath(path)
+	must(t, err)
+	if st := res.State; st.MissingCount != 1 || !st.Pads[1].Missing {
+		t.Errorf("missing count %d, A02 missing %v; want 1, true", st.MissingCount, st.Pads[1].Missing)
+	}
+}

internal/export/export.go

@@ -46,7 +46,21 @@ type item struct {
 	src  string
 }
 
-// collect checks that every pool file has a source that exists.
+// Usable reports whether src can be exported: a regular file, not a link,
+// in a folder that is not a link either. A link in a shared kit, or a data
+// folder that is itself a link, could otherwise export any file it points
+// to. Links the user picks are resolved when the sample is assigned.
+func Usable(src string) bool {
+	st, err := os.Lstat(src)
+	if err != nil || !st.Mode().IsRegular() {
+		return false
+	}
+	dir, err := os.Lstat(filepath.Dir(src))
+	return err == nil && dir.IsDir()
+}
+
+// collect checks that every pool file has a usable source. A source that
+// is a link or not a regular file is reported as missing.
 func collect(p *project.Project) ([]item, error) {
 	var items []item
 	missing := &MissingError{}
@@ -57,7 +71,7 @@ func collect(p *project.Project) ([]item, error) {
 			missing.Paths = append(missing.Paths, "no source recorded")
 			continue
 		}
-		if st, err := os.Stat(src); err != nil || !st.Mode().IsRegular() {
+		if !Usable(src) {
 			missing.Files = append(missing.Files, f)
 			missing.Paths = append(missing.Paths, src)
 			continue
@@ -80,11 +94,31 @@ func encode(p *project.Project) ([]byte, error) {
 
 func dataDir(p *project.Project) string { return p.Name + project.DataSuffix }
 
+// checkNames refuses pool file names that would reach outside the data
+// folder. Load and AssignSample already ensure this; it is checked again
+// where files are written.
+func checkNames(items []item) error {
+	for _, it := range items {
+		if err := project.PlainFileName(it.name); err != nil {
+			return fmt.Errorf("sample %q: %w", it.name, err)
+		}
+	}
+	return nil
+}
+
+// rename is os.Rename; tests replace it to make a rename fail.
+var rename = os.Rename
+
 // Folder writes <dest>/<Name>.xpj and <dest>/<Name>_[ProjectData]/ and
-// returns the path of the .xpj. Everything is written under a temporary
-// name in dest first and renamed at the end, so a failure leaves any
-// existing project as it was. If either target exists and overwrite is
+// returns the path of the .xpj. If either target exists and overwrite is
 // false, nothing is written and an *ExistsError is returned.
+//
+// The new pair is written in full to a temporary folder in dest first. To
+// replace an existing pair, Folder moves it into that temporary folder,
+// moves the new pair into place, and only then deletes the old pair. If
+// any step fails, the old pair is moved back, so an existing export is
+// either replaced completely or left as it was. If even moving it back
+// fails, the error says where the old pair was kept.
 func Folder(p *project.Project, dest string, overwrite bool) (string, error) {
 	if err := project.ValidName(p.Name); err != nil {
 		return "", fmt.Errorf("project name: %w", err)
@@ -93,6 +127,12 @@ func Folder(p *project.Project, dest string, overwrite bool) (string, error) {
 	if err != nil {
 		return "", err
 	}
+	if err := checkNames(items); err != nil {
+		return "", err
+	}
+	if _, err := os.Stat(dest); errors.Is(err, os.ErrNotExist) {
+		return "", fmt.Errorf("export folder does not exist: %s", dest)
+	}
 	xpjPath := filepath.Join(dest, p.Name+".xpj")
 	dirPath := filepath.Join(dest, dataDir(p))
 	if !overwrite {
@@ -109,7 +149,12 @@ func Folder(p *project.Project, dest string, overwrite bool) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	defer os.RemoveAll(tmp)
+	keepTmp := false
+	defer func() {
+		if !keepTmp {
+			os.RemoveAll(tmp)
+		}
+	}()
 	tmpData := filepath.Join(tmp, dataDir(p))
 	if err := os.Mkdir(tmpData, 0o755); err != nil {
 		return "", err
@@ -124,22 +169,42 @@ func Folder(p *project.Project, dest string, overwrite bool) (string, error) {
 		return "", err
 	}
 
-	// Replace: the old pair goes only once the new pair is complete.
+	// Move each existing target aside, then each new one into place. A
+	// failure undoes the moves already made, newest first.
+	old := filepath.Join(tmp, "old")
+	if err := os.Mkdir(old, 0o755); err != nil {
+		return "", err
+	}
+	type move struct{ from, to string }
+	var moves []move
+	var moveErr error
 	if overwrite {
-		if err := os.RemoveAll(dirPath); err != nil {
-			return "", err
+		for _, target := range existing(dirPath, xpjPath) {
+			m := move{target, filepath.Join(old, filepath.Base(target))}
+			if moveErr = rename(m.from, m.to); moveErr != nil {
+				break
+			}
+			moves = append(moves, m)
 		}
-		if err := os.Remove(xpjPath); err != nil && !errors.Is(err, os.ErrNotExist) {
-			return "", err
+	}
+	if moveErr == nil {
+		for _, m := range []move{{tmpData, dirPath}, {tmpXPJ, xpjPath}} {
+			if moveErr = rename(m.from, m.to); moveErr != nil {
+				break
+			}
+			moves = append(moves, m)
 		}
 	}
-	if err := os.Rename(tmpData, dirPath); err != nil {
-		return "", err
+	if moveErr == nil {
+		return xpjPath, nil // the deferred RemoveAll deletes the old pair
 	}
-	if err := os.Rename(tmpXPJ, xpjPath); err != nil {
-		return "", err
+	for i := len(moves) - 1; i >= 0; i-- {
+		if err := rename(moves[i].to, moves[i].from); err != nil {
+			keepTmp = true
+			return "", fmt.Errorf("%w; the previous export could not be put back and is kept in %s", moveErr, old)
+		}
 	}
-	return xpjPath, nil
+	return "", moveErr
 }
 
 func existing(paths ...string) []string {
@@ -210,6 +275,9 @@ func Zip(p *project.Project, path string, overwrite bool) error {
 }
 
 func writeZip(w io.Writer, p *project.Project, body []byte, items []item) error {
+	if err := checkNames(items); err != nil {
+		return err
+	}
 	now := time.Now()
 	zw := zip.NewWriter(w)
 	// The .xpj is already gzip-compressed: store it.

internal/export/export_test.go

@@ -5,14 +5,18 @@ import (
 	"bytes"
 	"compress/gzip"
 	"errors"
+	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
+	"runtime"
 	"sort"
 	"strings"
 	"testing"
 
 	"git.monoexp.com/bajdev/mpckit/internal/project"
+	"git.monoexp.com/bajdev/mpckit/internal/xpj"
 )
 
 const testdata = "../../testdata"
@@ -214,3 +218,280 @@ func TestZip(t *testing.T) {
 		t.Errorf("temporary files left behind: %v", got)
 	}
 }
+
+// snapshot returns every file and folder below dir with its contents
+// ("<dir>" for a folder).
+func snapshot(t *testing.T, dir string) map[string]string {
+	t.Helper()
+	out := map[string]string{}
+	err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
+		if err != nil || path == dir {
+			return err
+		}
+		rel, _ := filepath.Rel(dir, path)
+		if d.IsDir() {
+			out[rel] = "<dir>"
+			return nil
+		}
+		b, err := os.ReadFile(path)
+		out[rel] = string(b)
+		return err
+	})
+	must(t, err)
+	return out
+}
+
+func sameSnapshot(t *testing.T, got, want map[string]string) {
+	t.Helper()
+	if len(got) != len(want) {
+		t.Errorf("%d entries, want %d: %v", len(got), len(want), keys(got))
+	}
+	for k, v := range want {
+		if got[k] != v {
+			t.Errorf("%s changed or missing", k)
+		}
+	}
+}
+
+func keys(m map[string]string) []string {
+	var out []string
+	for k := range m {
+		out = append(out, k)
+	}
+	sort.Strings(out)
+	return out
+}
+
+// symlink makes a link, or skips the test where the system does not allow
+// it (Windows without developer mode).
+func symlink(t *testing.T, target, link string) {
+	t.Helper()
+	if err := os.Symlink(target, link); err != nil {
+		t.Skipf("cannot create a symlink: %v", err)
+	}
+}
+
+func TestSymlinkedSource(t *testing.T) {
+	// A copy of MpckitT1 whose data folder holds a link instead of a WAV.
+	src := t.TempDir()
+	must(t, os.WriteFile(filepath.Join(src, "Kit.xpj"), readFile(t, filepath.Join(testdata, "accepted", "MpckitT1.xpj")), 0o644))
+	data := filepath.Join(src, "Kit"+project.DataSuffix)
+	must(t, os.Mkdir(data, 0o755))
+	acc := filepath.Join(testdata, "accepted", "MpckitT1"+project.DataSuffix)
+	must(t, os.WriteFile(filepath.Join(data, "Linn 9000 kick.wav"), readFile(t, filepath.Join(acc, "Linn 9000 kick.wav")), 0o644))
+	outside := filepath.Join(t.TempDir(), "secret.txt")
+	must(t, os.WriteFile(outside, []byte("secret"), 0o644))
+	link := filepath.Join(data, "Linn 9000 kick001.wav")
+	symlink(t, outside, link)
+
+	if Usable(link) || !Usable(filepath.Join(data, "Linn 9000 kick.wav")) || Usable(data) || Usable(outside+"x") {
+		t.Error("Usable: wrong answer for a link, a file, a folder or a missing file")
+	}
+	p, _, err := project.Load(filepath.Join(src, "Kit.xpj"))
+	must(t, err)
+	dest := t.TempDir()
+	_, err = Folder(p, dest, false)
+	var missing *MissingError
+	if !errors.As(err, &missing) || strings.Join(missing.Files, ",") != "Linn 9000 kick001.wav" {
+		t.Fatalf("Folder: %v", err)
+	}
+	if err := Zip(p, filepath.Join(dest, "kit.zip"), false); !errors.As(err, &missing) {
+		t.Fatalf("Zip: %v", err)
+	}
+	if got := listDir(t, dest); len(got) != 0 {
+		t.Errorf("failed export wrote %v", got)
+	}
+}
+
+func TestLinkedDataFolder(t *testing.T) {
+	// The whole data folder is a link to a folder holding a file with a
+	// pool file's name.
+	src := t.TempDir()
+	must(t, os.WriteFile(filepath.Join(src, "Kit.xpj"), readFile(t, filepath.Join(testdata, "accepted", "MpckitT1.xpj")), 0o644))
+	elsewhere := t.TempDir()
+	for _, f := range []string{"Linn 9000 kick.wav", "Linn 9000 kick001.wav"} {
+		must(t, os.WriteFile(filepath.Join(elsewhere, f), []byte("private"), 0o644))
+	}
+	symlink(t, elsewhere, filepath.Join(src, "Kit"+project.DataSuffix))
+
+	p, _, err := project.Load(filepath.Join(src, "Kit.xpj"))
+	must(t, err)
+	dest := t.TempDir()
+	_, err = Folder(p, dest, false)
+	var missing *MissingError
+	if !errors.As(err, &missing) || len(missing.Files) != 2 {
+		t.Fatalf("Folder: %v", err)
+	}
+	if got := listDir(t, dest); len(got) != 0 {
+		t.Errorf("failed export wrote %v", got)
+	}
+}
+
+func TestBadPoolName(t *testing.T) {
+	p := buildT1(t)
+	// Load and AssignSample never produce such a name; change the tree
+	// directly to reach the export check.
+	d, err := p.File().Data()
+	must(t, err)
+	src := p.Sources["Linn 9000 kick.wav"]
+	must(t, d.Set("samples[0].path", xpj.NewString("../evil.wav")))
+	p.Sources["../evil.wav"] = src
+
+	dest := filepath.Join(t.TempDir(), "out")
+	must(t, os.Mkdir(dest, 0o755))
+	want := `sample "../evil.wav": file name "../evil.wav" contains a folder separator`
+	if _, err := Folder(p, dest, false); err == nil || err.Error() != want {
+		t.Errorf("Folder: got %v, want %q", err, want)
+	}
+	if err := Zip(p, filepath.Join(dest, "kit.zip"), false); err == nil || err.Error() != want {
+		t.Errorf("Zip: got %v, want %q", err, want)
+	}
+	if got := listDir(t, dest); len(got) != 0 {
+		t.Errorf("failed export wrote %v", got)
+	}
+	if _, err := os.Stat(filepath.Join(filepath.Dir(dest), "evil.wav")); err == nil {
+		t.Error("export wrote outside the data folder")
+	}
+}
+
+func TestMissingDestination(t *testing.T) {
+	dest := filepath.Join(t.TempDir(), "gone")
+	_, err := Folder(buildT1(t), dest, false)
+	if want := "export folder does not exist: " + dest; err == nil || err.Error() != want {
+		t.Errorf("got %v, want %q", err, want)
+	}
+}
+
+// failRename makes the listed renames from now fail, counting from 1, until
+// the test ends.
+func failRename(t *testing.T, fail ...int) {
+	t.Helper()
+	calls := 0
+	rename = func(from, to string) error {
+		calls++
+		for _, n := range fail {
+			if calls == n {
+				return &os.LinkError{Op: "rename", Old: from, New: to, Err: errors.New("test failure")}
+			}
+		}
+		return os.Rename(from, to)
+	}
+	t.Cleanup(func() { rename = os.Rename })
+}
+
+func TestOverwriteFailureKeepsOldExport(t *testing.T) {
+	// The renames of an overwrite: 1 old data folder aside, 2 old .xpj
+	// aside, 3 new data folder in, 4 new .xpj in. Failing any of them
+	// leaves the previous export exactly as it was.
+	for n := 1; n <= 4; n++ {
+		t.Run(fmt.Sprint(n), func(t *testing.T) {
+			dest := t.TempDir()
+			_, err := Folder(buildT1(t), dest, false)
+			must(t, err)
+			before := snapshot(t, dest)
+
+			changed := buildT1(t)
+			must(t, changed.ClearPad(16))
+			failRename(t, n)
+			if _, err := Folder(changed, dest, true); err == nil || !strings.Contains(err.Error(), "test failure") {
+				t.Fatalf("Folder: %v", err)
+			}
+			sameSnapshot(t, snapshot(t, dest), before)
+		})
+	}
+}
+
+func TestOverwriteUndoFails(t *testing.T) {
+	dest := t.TempDir()
+	_, err := Folder(buildT1(t), dest, false)
+	must(t, err)
+	before := snapshot(t, dest)
+
+	// The new .xpj cannot be moved in (4), and moving the new data folder
+	// back out fails too (5): the old pair stays in the temporary folder.
+	failRename(t, 4, 5)
+	_, err = Folder(buildT1(t), dest, true)
+	if err == nil || !strings.Contains(err.Error(), "could not be put back and is kept in") {
+		t.Fatalf("Folder: %v", err)
+	}
+	_, kept, _ := strings.Cut(err.Error(), "is kept in ")
+	if got := snapshot(t, kept); len(got) != len(before) {
+		t.Errorf("kept %v, want the %d entries of the old export", keys(got), len(before))
+	}
+}
+
+func TestOverwriteHalfPair(t *testing.T) {
+	// Only the .xpj of an earlier export is there: it is replaced and the
+	// data folder created.
+	dest := t.TempDir()
+	must(t, os.WriteFile(filepath.Join(dest, "MpckitT1.xpj"), []byte("old"), 0o644))
+	_, err := Folder(buildT1(t), dest, true)
+	must(t, err)
+	if got := listDir(t, dest); strings.Join(got, "|") != "MpckitT1.xpj|MpckitT1_[ProjectData]" {
+		t.Errorf("dest holds %v", got)
+	}
+	if string(readFile(t, filepath.Join(dest, "MpckitT1.xpj"))) == "old" {
+		t.Error("old .xpj not replaced")
+	}
+}
+
+func TestBackslashNameExportsAndLoads(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Windows file names cannot hold a backslash")
+	}
+	src := filepath.Join(t.TempDir(), `kick\one.wav`)
+	must(t, os.WriteFile(src, readFile(t, filepath.Join(testdata, "accepted", "MpckitT1"+project.DataSuffix, "Linn 9000 kick.wav")), 0o644))
+	p, err := project.New("Backslash")
+	must(t, err)
+	must(t, p.AssignSample(0, src))
+	if got := p.PoolFiles(); len(got) != 1 || got[0] != "kick_one.wav" {
+		t.Fatalf("pool = %q, want kick_one.wav", got)
+	}
+	dest := t.TempDir()
+	path, err := Folder(p, dest, false)
+	must(t, err)
+	q, _, err := project.Load(path)
+	must(t, err)
+	if v, err := q.Pad(0); err != nil || v.File != "kick_one.wav" || v.SampleName != "kick_one" {
+		t.Errorf("reloaded pad A1 = %+v, %v", v, err)
+	}
+}
+
+func TestExportErrors(t *testing.T) {
+	dir := t.TempDir()
+	file := filepath.Join(dir, "file")
+	must(t, os.WriteFile(file, nil, 0o644))
+
+	bad := buildT1(t)
+	bad.Name = "a/b"
+	if _, err := Folder(bad, dir, false); err == nil || !strings.HasPrefix(err.Error(), "project name:") {
+		t.Errorf("Folder with a bad name: %v", err)
+	}
+	if err := Zip(bad, filepath.Join(dir, "kit.zip"), false); err == nil || !strings.HasPrefix(err.Error(), "project name:") {
+		t.Errorf("Zip with a bad name: %v", err)
+	}
+
+	unknown := buildT1(t)
+	delete(unknown.Sources, "Linn 9000 kick.wav")
+	_, err := Folder(unknown, dir, false)
+	if err == nil || !strings.Contains(err.Error(), "Linn 9000 kick.wav (no source recorded)") {
+		t.Errorf("no source: %v", err)
+	}
+
+	if _, err := Folder(buildT1(t), file, false); err == nil {
+		t.Error("Folder into a file: no error")
+	}
+	if err := Zip(buildT1(t), filepath.Join(dir, "gone", "kit.zip"), false); err == nil {
+		t.Error("Zip into a missing folder: no error")
+	}
+
+	_, err = Folder(buildT1(t), dir, false)
+	must(t, err)
+	_, err = Folder(buildT1(t), dir, false)
+	if want := "already exists: " + filepath.Join(dir, "MpckitT1.xpj") + ", " + filepath.Join(dir, "MpckitT1_[ProjectData]"); err == nil || err.Error() != want {
+		t.Errorf("got %v, want %q", err, want)
+	}
+	if got := listDir(t, dir); strings.Join(got, "|") != "MpckitT1.xpj|MpckitT1_[ProjectData]|file" {
+		t.Errorf("failed exports left %v", got)
+	}
+}

internal/project/pads.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"path/filepath"
 	"strings"
+	"unicode"
 
 	"git.monoexp.com/bajdev/mpckit/internal/wav"
 	"git.monoexp.com/bajdev/mpckit/internal/xpj"
@@ -117,15 +118,23 @@ func (p *Project) AssignSample(slot int, src string) error {
 	if err != nil {
 		return err
 	}
-	info, err := wav.ReadFile(abs)
+	name := filepath.Base(abs)
+	// Export refuses a source that is a link, so that a link planted in a
+	// shared kit's data folder cannot pull in other files. A link the user
+	// picks themselves is fine: record the file it points to.
+	target, err := filepath.EvalSymlinks(abs)
 	if err != nil {
-		return fmt.Errorf("%s: %w", filepath.Base(abs), err)
+		return fmt.Errorf("%s: %w", name, err)
+	}
+	info, err := wav.ReadFile(target)
+	if err != nil {
+		return fmt.Errorf("%s: %w", name, err)
 	}
 	if err := info.Validate(); err != nil {
-		return fmt.Errorf("%s: %w", filepath.Base(abs), err)
+		return fmt.Errorf("%s: %w", name, err)
 	}
 
-	file := p.fileForSource(abs)
+	file := p.fileForSource(target)
 	if file == "" {
 		// A new pool entry is needed. Clearing the pad first may free one.
 		freed := 0
@@ -141,11 +150,11 @@ func (p *Project) AssignSample(slot int, src string) error {
 	// Keep the reused entry even if this pad was its only user.
 	p.clear(slot, file)
 	if file == "" {
-		file = p.uniqueFile(filepath.Base(abs))
+		file = p.uniqueFile(poolFileName(name))
 		if err := p.addPoolEntry(file); err != nil {
 			return err
 		}
-		p.Sources[file] = abs
+		p.Sources[file] = target
 	}
 
 	pad := p.pad(slot)
@@ -182,6 +191,34 @@ func (p *Project) inPool(file string) bool {
 	return false
 }
 
+// poolFileName turns the name of a WAV the user picked into a pool file
+// name that passes PlainFileName on every system and is legal on the
+// device's FAT card. Linux and macOS allow names such as "kick\one.wav",
+// "a:b.wav" or "con.wav"; exported as they are, the backslash reads as a
+// folder on Windows and the card, the colon cannot be written to the card,
+// Windows cannot create a file named after a device, and Load refuses the
+// project. Each such character becomes "_", and a device name gets "_"
+// added ("con.wav" becomes "con_.wav").
+func poolFileName(name string) string {
+	clean := strings.Map(func(r rune) rune {
+		if unicode.IsControl(r) || strings.ContainsRune(`/\:*?"<>|`, r) {
+			return '_'
+		}
+		return r
+	}, name)
+	if clean == "" || clean == "." || clean == ".." {
+		return "sample.wav"
+	}
+	if reservedOnWindows(clean) {
+		base, ext, _ := strings.Cut(clean, ".")
+		clean = base + "_"
+		if ext != "" {
+			clean += "." + ext
+		}
+	}
+	return clean
+}
+
 func stem(file string) string { return strings.TrimSuffix(file, filepath.Ext(file)) }
 
 // uniqueFile returns base, or base with a 001, 002 ... suffix before the

internal/project/project.go

@@ -296,17 +296,23 @@ func ValidName(name string) error {
 	if strings.HasSuffix(name, ".") || strings.HasSuffix(name, " ") {
 		return errors.New("name cannot end with a dot or a space")
 	}
-	base := strings.ToUpper(name)
-	if i := strings.IndexByte(base, '.'); i >= 0 {
-		base = base[:i]
+	if reservedOnWindows(name) {
+		return fmt.Errorf("%q is a reserved name on Windows", name)
 	}
+	return nil
+}
+
+// reservedOnWindows reports whether Windows treats name as a device, such as
+// "CON" or "com1.wav": the part before the first dot is what counts.
+func reservedOnWindows(name string) bool {
+	base, _, _ := strings.Cut(strings.ToUpper(name), ".")
 	switch base {
 	case "CON", "PRN", "AUX", "NUL",
 		"COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9",
 		"LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9":
-		return fmt.Errorf("%q is a reserved name on Windows", name)
+		return true
 	}
-	return nil
+	return false
 }
 
 // Slot helpers. A slot is 0-127: bank x 16 + pad - 1.

internal/project/project_test.go

@@ -242,8 +242,12 @@ func TestAssignAndPoolReuse(t *testing.T) {
 	if p.SampleCount() != 1 || padOf(t, p, 0).File != "kick.wav" {
 		t.Errorf("reassign same source: %d entries, pad %+v", p.SampleCount(), padOf(t, p, 0))
 	}
-	if got := p.Sources["kick.wav"]; got != kick {
-		t.Errorf("source = %q, want %q", got, kick)
+	// The source is the file itself, with any links resolved (macOS's
+	// temporary folder is behind one).
+	want, err := filepath.EvalSymlinks(kick)
+	must(t, err)
+	if got := p.Sources["kick.wav"]; got != want {
+		t.Errorf("source = %q, want %q", got, want)
 	}
 	// Replacing drops the old entry when no pad uses it.
 	snare := writeWAV(t, filepath.Join(dir, "snare.wav"), 50)
@@ -565,3 +569,53 @@ func TestNames(t *testing.T) {
 		}
 	}
 }
+
+func TestPoolFileName(t *testing.T) {
+	for in, want := range map[string]string{
+		"kick.wav":        "kick.wav",
+		`kick\one.wav`:    "kick_one.wav",
+		"a:b*c?.wav":      "a_b_c_.wav",
+		`"q"<>|.wav`:      "_q____.wav",
+		"tab\there.wav":   "tab_here.wav",
+		"Ünïcode 80s.wav": "Ünïcode 80s.wav",
+		"":                "sample.wav",
+		".":               "sample.wav",
+		"..":              "sample.wav",
+		"CON.wav":         "CON_.wav",
+		"aux.wav":         "aux_.wav",
+		"com1.loop.wav":   "com1_.loop.wav",
+		"Lpt9":            "Lpt9_",
+		"console.wav":     "console.wav",
+		"com10.wav":       "com10.wav",
+	} {
+		got := poolFileName(in)
+		if got != want {
+			t.Errorf("poolFileName(%q) = %q, want %q", in, got, want)
+		}
+		if err := PlainFileName(got); err != nil || reservedOnWindows(got) {
+			t.Errorf("poolFileName(%q) = %q: not a safe name", in, got)
+		}
+	}
+}
+
+func TestAssignFollowsLink(t *testing.T) {
+	dir := t.TempDir()
+	kick := writeWAV(t, filepath.Join(dir, "library", "kick.wav"), 100)
+	link := filepath.Join(dir, "favourite.wav")
+	if err := os.Symlink(kick, link); err != nil {
+		t.Skipf("cannot create a symlink: %v", err)
+	}
+	p := newProject(t)
+	must(t, p.AssignSample(0, link))
+	want, err := filepath.EvalSymlinks(kick)
+	must(t, err)
+	// The pool file is named after the link; it is copied from the target.
+	if got := p.PoolFiles(); len(got) != 1 || got[0] != "favourite.wav" || p.Sources["favourite.wav"] != want {
+		t.Errorf("pool %q, source %q; want favourite.wav from %q", got, p.Sources["favourite.wav"], want)
+	}
+	// The target itself is the same source: no second pool entry.
+	must(t, p.AssignSample(1, kick))
+	if p.SampleCount() != 1 {
+		t.Errorf("%d pool entries, want 1", p.SampleCount())
+	}
+}

pull-requests/safe-export/safe-export-and-clean-sample-names.md

@@ -0,0 +1,80 @@
+# Safe export and clean sample names
+
+Branch: `safe-export` → `main`
+
+PR 3 of the review fixes plan (`docs/review-fixes-plan.md`). Part of 0.5.2; the version does not change in this PR.
+
+## What changed
+
+**Export refuses links (S1, export side).**
+- A sample whose source is a link, or not a regular file, is reported as missing, the same way as a deleted file, and nothing is written.
+- So is a sample whose folder is itself a link. This was not in the plan; see Notes.
+- The check is `export.Usable`. The app uses the same check for the missing-file badges and missing count, so a pad with a linked source shows as missing and the export buttons are disabled, matching what export would do.
+
+**Export checks names again (S1).** `Folder` and the zip writer refuse a sample file name that is not a plain file name, using PR 1's rule (`project.PlainFileName`). This is a second line of defence: load and assign already ensure it.
+
+**New sample names are cleaned at assign (S1, assign).**
+- A new pool file name is passed through `poolFileName` in `project` before the 001, 002 suffixes are added. A comment there gives the reason.
+- The helper replaces `/`, `\`, control characters, and the characters the device's FAT card forbids (`: * ? " < > |`) with `_`. A name Windows reserves for a device (`CON`, `PRN`, `AUX`, `NUL`, `COM1`–`COM9`, `LPT1`–`LPT9`, in any case and with any extension) gets `_` after that part, so `CON.wav` becomes `CON_.wav`. A name that comes out empty, `.` or `..` becomes `sample.wav`. The sample name follows from the cleaned file name.
+- So `kick\one.wav`, which Linux and macOS allow, becomes `kick_one.wav`, and its export loads again.
+- Load still accepts existing names; only new assignments change.
+
+**Links you pick yourself still work.** When you assign a WAV that is a link, MPCkit follows it. The pool file is named after the link, and the source is the file it points to. Picking the target itself later reuses the same pool entry.
+
+**Overwrite cannot lose the old export (M1, R1).** With overwrite set, `Folder` used to delete the old `.xpj` and data folder before moving the new ones in, so a failed rename lost both. Now:
+1. The new pair is written in full to a temporary folder inside the destination, as before.
+2. The old pair is moved into that temporary folder.
+3. The new pair is moved into place.
+4. The old pair is deleted only after that.
+
+If any move fails, the moves already made are undone, newest first, and the old export is back as it was. If even putting it back fails, the temporary folder is kept, and the error says where the old export is. The `Folder` comment now says exactly this.
+
+**Missing destination.** Exporting into a folder that no longer exists now says `export folder does not exist: <dest>` instead of a bare `stat` error.
+
+**Tests (M5).** Coverage of `internal/export` is up from 74.2% to 87.8%. New tests:
+- `TestSymlinkedSource`: a kit whose data folder holds a link to a file outside. Folder and Zip export both fail, naming the file, and write nothing.
+- `TestLinkedDataFolder`: a kit whose data folder is itself a link. Same result.
+- `TestBadPoolName`: a pool name `../evil.wav` is refused by Folder and Zip export, and nothing is written.
+- `TestOverwriteFailureKeepsOldExport`: makes each of the 4 renames of an overwrite fail in turn. Every time, the previous export is byte-for-byte as it was.
+- `TestOverwriteUndoFails`: the new `.xpj` cannot be moved in, and putting things back fails too. The error names the folder that holds the old export, and the export is complete there.
+- `TestOverwriteHalfPair`: an overwrite where only the old `.xpj` exists.
+- `TestMissingDestination`.
+- `TestExportErrors`: a bad project name, no recorded source, a destination that is a file, a zip into a missing folder, and the "already exists" message.
+- `TestBackslashNameExportsAndLoads`: assigns a WAV named `kick\one.wav`, gets `kick_one.wav`, exports, and loads the export.
+- In `project` and `app`: `TestPoolFileName`, `TestAssignFollowsLink` and `TestLinkedFileIsMissing`.
+
+Against the old code, every new export test fails.
+
+## Notes
+
+- **Decisions (bajdev, 2026-11-12).** Two choices the plan left open were settled while starting this PR, and are now in the plan's Decisions table:
+  - Links the user picks are followed at assign.
+  - FAT-illegal characters are cleaned too.
+- **Linked data folder (not in the plan).** `Lstat` checks only the last part of a path. A kit whose whole `_[ProjectData]` folder is a link to, say, `~/.ssh`, with a pool entry named `id_rsa`, would still have exported that file. `Usable` now also requires the file's own folder to be a real folder.
+  - Folders higher up may still be links; a project kept in a linked music folder works as before.
+  - Samples you assign are unaffected, since their paths are resolved at assign.
+- **Making a rename fail.** The plan suggested a directory where the `.xpj` should go. With the new overwrite, that directory is simply moved aside and replaced, so it no longer fails. The tests instead swap the package's `rename` (normally `os.Rename`) for one that fails on chosen calls. This reaches every step on every system.
+- **The source shown for a linked WAV.** It is the file the link points to, not the link.
+- **Windows device names (bajdev, 2026-11-13).** These are renamed too. `ValidName` and `poolFileName` now share one list, `reservedOnWindows`.
+- **What the user sees.** Renaming is silent, like the 001 suffixes added for duplicate names. The pad shows the new name (`kick_one`), so the change is visible but not explained. Only names that could not be exported as they are change, and none of them can be created on Windows anyway.
+- **On Windows, the link tests skip** unless developer mode is on, because creating links needs it. `TestBackslashNameExportsAndLoads` also skips there, because Windows file names cannot hold `\`.
+
+## Testing
+
+**On the server:**
+- `go test ./...` passes, including the round-trip tests, unchanged.
+- `go test -cover ./internal/export` shows 87.8%.
+- `go vet ./...` (also with `GOOS=windows`) and `staticcheck ./...` are clean.
+- `govulncheck ./...` reports only GO-2026-6859, which waits for Go 1.27.4 in PR 4.
+- `npm install`, `npm run check` (0 errors, 0 warnings) and `npm run build` pass. The lockfile `npm install` wrote was not committed.
+- `wails3 build` builds, and `timeout 15 xvfb-run -a bin/mpckit` starts and runs. The log holds only the usual headless GTK warnings.
+
+**On the laptop (PowerShell, in the repo):**
+1. `git fetch; git checkout safe-export`
+2. `go test -cover ./internal/export`: `ok`, coverage 87.8%. That is the same as on the server: the tests that skip on Windows add no coverage of their own.
+3. `go test ./...`: all `ok`.
+4. `wails3 dev`: load `testdata\device\DemoFile.xpj`, choose Export to folder and pick an empty folder. It writes `DemoFile.xpj` and `DemoFile_[ProjectData]` with 6 WAVs.
+5. Change a pad's tune and export to the same folder again. Confirm the overwrite prompt. The export is replaced, and no `.mpckit-export-…` folder is left behind.
+6. Load the exported `DemoFile.xpj`. It opens with the changed tune, and no pad shows as missing.
+7. Export as zip. The zip holds the `.xpj` and the data folder with 6 WAVs.
+8. Assign a WAV from your sample library to an empty pad. It gets the WAV's own name, as before.