Add a test that broken fields cannot panic

FuzzLoad mutates gzip bytes, so almost no input gets past the CRC.
TestBrokenFieldsDoNotPanic breaks DemoFile's fields one at a time
(null, text, numbers, empty list or object, deleted) and runs every
read and most edits on the result. It covers 1,134 cases in about 30
seconds; -short skips it. Without this branch's load checks it reports
67 panics.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/project/load_test.go

@@ -1,8 +1,10 @@
 package project
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -166,3 +168,159 @@ func FuzzLoad(f *testing.F) {
 		p.PoolFiles()
 	})
 }
+
+// TestBrokenFieldsDoNotPanic changes, one at a time, each field that project
+// reads in DemoFile: replaced by null, text, a negative or large number, an
+// empty list or object, or deleted. Whatever wrap makes of the result, reads
+// and edits must not panic. Only the first item of each list is changed, so
+// the test stays fast; a full sweep (all fields to depth 5, 12,726 cases)
+// found nothing either and takes about 10 minutes.
+func TestBrokenFieldsDoNotPanic(t *testing.T) {
+	if testing.Short() {
+		t.Skip("slow")
+	}
+	base, err := xpj.ReadFile(filepath.Join(testdata, "device", "DemoFile.xpj"))
+	must(t, err)
+	d, err := base.Data()
+	must(t, err)
+
+	events := "sequences[0].value.trackClipMaps[0][0].value.eventList.events"
+	roots := []struct {
+		path  string
+		depth int
+	}{
+		{"version", 0}, {"currentSequence", 0}, {"tracks[0].name", 0},
+		{"samples", 3}, {"tracks[0].samples", 2},
+		{"tracks[0].program.drum.instruments", 1},
+		{"tracks[0].program.drum.instruments[0]", 1},
+		{"tracks[0].program.drum.instruments[0].layersv", 2},
+		{"tracks[0].program.drum.instruments[0].layersv[0].sliceInfo", 1},
+		{"sequences", 3},
+		{"sequences[0].value.trackClipMaps", 4},
+		{events, 3},
+	}
+	// The first event is automation; add the first note event too.
+	evs, err := d.Get(events)
+	must(t, err)
+	for i, ev := range evs.Items {
+		if num(ev.Lookup("type")) == eventNote {
+			roots = append(roots, struct {
+				path  string
+				depth int
+			}{fmt.Sprintf("%s[%d]", events, i), 1}, struct {
+				path  string
+				depth int
+			}{fmt.Sprintf("%s[%d].note.note", events, i), 0})
+			break
+		}
+	}
+	var paths []string
+	for _, r := range roots {
+		n, err := d.Get(r.path)
+		must(t, err)
+		fieldPaths(n, r.path, r.depth, &paths)
+	}
+
+	changes := []func(parent *xpj.Node, key string, i int){
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewNull()) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewString("../x.wav")) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewInt(-1)) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewInt(1000)) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewArray()) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewObject()) },
+		func(p *xpj.Node, k string, i int) {
+			if k != "" {
+				p.DeleteKey(k)
+			} else {
+				_ = p.Remove(i)
+			}
+		},
+	}
+	names := []string{"null", "text", "-1", "1000", "list", "object", "deleted"}
+	for _, path := range paths {
+		parentPath, key, i := splitPath(path)
+		for c, change := range changes {
+			f := base.Clone()
+			fd, _ := f.Data()
+			parent, err := fd.Get(parentPath)
+			if err != nil {
+				continue // a key holding '.' or '[' cannot be addressed by path
+			}
+			change(parent, key, i)
+			func() {
+				defer func() {
+					if r := recover(); r != nil {
+						t.Errorf("%s %s: panic: %v", path, names[c], r)
+					}
+				}()
+				p, err := wrap(f, "Broken", "")
+				if err != nil {
+					return
+				}
+				exercise(p)
+			}()
+		}
+	}
+	t.Logf("%d fields, %d cases", len(paths), len(paths)*len(changes))
+}
+
+// fieldPaths lists path and the paths below n to depth levels, taking only
+// the first item of each list.
+func fieldPaths(n *xpj.Node, path string, depth int, out *[]string) {
+	*out = append(*out, path)
+	if depth == 0 {
+		return
+	}
+	if len(n.Items) > 0 {
+		fieldPaths(n.Items[0], path+"[0]", depth-1, out)
+	}
+	for _, m := range n.Members {
+		fieldPaths(m.Value, path+"."+m.Key, depth-1, out)
+	}
+}
+
+// splitPath splits a path into its parent and last step: a key, or an index
+// when key is "".
+func splitPath(path string) (parent, key string, i int) {
+	if strings.HasSuffix(path, "]") {
+		open := strings.LastIndexByte(path, '[')
+		i, _ = strconv.Atoi(path[open+1 : len(path)-1])
+		return path[:open], "", i
+	}
+	dot := strings.LastIndexByte(path, '.')
+	if dot < 0 {
+		return "", path, 0
+	}
+	return path[:dot], path[dot+1:], 0
+}
+
+func replace(parent *xpj.Node, key string, i int, v *xpj.Node) {
+	if key != "" {
+		_ = parent.SetKey(key, v)
+	} else if i < len(parent.Items) {
+		parent.Items[i] = v
+	}
+}
+
+// exercise runs every read and most edits on p.
+func exercise(p *Project) {
+	for slot := range Pads {
+		_, _ = p.Pad(slot)
+		p.PadHasNotes(slot)
+	}
+	p.PoolFiles()
+	for slot := range 2 {
+		_ = p.SetTune(slot, 1, 1)
+		_ = p.SetMuteGroup(slot, 1)
+		_ = p.CopyPad(slot, slot+20)
+		_ = p.MovePad(slot+20, slot+40)
+		_ = p.SwapPads(slot, slot+1)
+		_ = p.ClearPad(slot + 1)
+	}
+	for _, s := range p.Sequences() {
+		_ = p.SetSequence(s.Slot, "Changed", 100, 4)
+	}
+	_ = p.AddSequence(50)
+	_ = p.RemoveSequence(50)
+	_ = p.RemoveSequence(0)
+}

pull-requests/harden-load/reject-malformed-projects-at-load.md

@@ -21,6 +21,7 @@ PR 1 of the review fixes plan (`docs/review-fixes-plan.md`). Part of 0.5.2; the
 - `TestLoadRejectsMalformed` builds 16 broken copies of DemoFile in `t.TempDir()` and checks each error. The cases cover pool paths `../x.wav`, `a/b.wav`, `..\b.wav`, `..`, an empty name, a control character, a bad track pool entry, and a missing or non-text `path`. They also cover a bad layer `sampleFile`, and a sequence missing `bpm`, `key`, `value` or `lengthBars`, or with a non-text name.
 - Other new tests cover a layer with no `sampleFile` (still loads), a missing `data.version` (warning), `PlainFileName`, and `xpj` getters on a nil node.
 - `FuzzLoad` is the fuzz target the plan describes.
+- `TestBrokenFieldsDoNotPanic` breaks DemoFile's fields one at a time, then runs every read and most edits on the result. None may panic. It covers 162 fields and 1,134 cases, and takes about 30 seconds on the server; `go test -short` skips it. See "Fuzzing" below for why it was added.
 - Against the old code, the path cases load without error and the missing-`path` case panics.
 
 ## Notes
@@ -33,24 +34,22 @@ Choices the plan left open. Say if any should change:
 - **Two messages after a panic in a bound method.** Wails already turns such a panic into a failed call, and the frontend shows that error. With the handler set, the handler's message is also sent, and both go to the same banner, so the last one shows.
 - **The fuzzer's progress line is wrong for this target.** It shows `execs: 9 (0/sec)` for most of the run. The fuzz function itself counted 42,097 calls in 15 seconds, so the 2-minute run made about 300,000.
 
-**Question for bajdev: `FuzzLoad` barely reaches the project code.** The seeds are gzip files, and gzip checks a CRC at the end of the stream, so almost every mutated input fails at decompression. In a 15-second sample:
+**Fuzzing.** `FuzzLoad` barely reaches the project code. The seeds are gzip files, and gzip checks a CRC at the end of the stream, so almost every mutated input fails at decompression. In a 15-second sample:
 - 5,885 inputs ran.
 - 5,882 failed in gzip or flate.
 - Only the 3 unchanged seeds loaded.
 
 Fuzzing the decompressed JSON does not help either: each project is about 3 MB uncompressed, too big for Go's fuzzer to mutate usefully.
 
-To test the code anyway, I ran a throwaway sweep, not committed:
+So I first ran a full sweep, not committed:
 - It changed each of 1,818 fields that `project` reads in DemoFile in 7 ways: null, text, -1, 1000, an empty list, an empty object, or deleted.
-- After each change, it ran every read and edit: Pad, PadHasNotes, Sequences, PoolFiles, tune, mute group, copy, move, swap, clear, sequence edits, and Encode.
-- Result: 12,726 cases, 0 panics. It takes about 10 minutes.
+- After each change, it ran every read and edit.
+- Result: 12,726 cases, 0 panics, in about 10 minutes.
 
-Options:
-1. Keep `FuzzLoad` as it is (it meets the plan).
-2. Also commit the sweep as a test, cut down to run in under a minute, e.g. the first pad and sequence only.
-3. Replace `FuzzLoad` with a fuzz target that changes the decoded tree rather than the gzip bytes.
-
-I'd suggest 2. It is a few lines in a follow-up PR or in this one if you prefer.
+bajdev chose to keep `FuzzLoad` and also commit a cut-down sweep that runs in under a minute: `TestBrokenFieldsDoNotPanic`.
+- It changes only the first item of each list. For pads it covers the first pad's own fields, its layers and `sliceInfo`, rather than every field below the pad.
+- It checks that nothing panics, not that the file is refused. Some changes load and are handled as an empty value.
+- With the load checks and nil guards turned off, it reports 67 panics; with them, none.
 
 ## Testing
 
@@ -59,13 +58,14 @@ I'd suggest 2. It is a few lines in a follow-up PR or in this one if you prefer.
 - `go vet ./...` and `staticcheck ./...` are clean.
 - `govulncheck ./...` reports only GO-2026-6859 in the standard library's `net/http`. That needs Go 1.27.4 (PR 4).
 - `go test -fuzz=FuzzLoad -fuzztime=2m ./internal/project` found nothing.
+- `TestBrokenFieldsDoNotPanic` passes in about 30 seconds.
 - `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.
 - `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 harden-load`
-2. `go test ./internal/project ./internal/xpj`: both `ok`. On Windows, `filepath` also treats `\` and drive letters as path parts, so this checks the name rule there.
+2. `go test ./internal/project ./internal/xpj`: both `ok` (the project tests take about half a minute). On Windows, `filepath` also treats `\` and drive letters as path parts, so this checks the name rule there.
 3. `wails3 dev`: load `testdata\device\DemoFile.xpj`. It opens as before: pads A1–A4, B1 and B2 filled; sequences Intro and Sequence 02.
 4. Load `testdata\accepted\MpckitT1.xpj`, then `MpckitT3.xpj`. Both open with no error or warning.
 5. Drop a WAV on a pad, change its tune and mute group, then add and remove a sequence. All work as before.