mpckit/docs/review-fixes-plan.md

MPCkit review fixes: work plan

Written 2026-11-12. Nine PRs, worked one at a time, fix the findings of the MPCkit source code review of e32802a (v0.5.1, 2026-11-12). PRs 1-4 ship the security fixes as 0.5.2; PRs 5-9 ship the hardening work as 0.5.3. Finding IDs (S1, M3, R1 and so on) refer to that review. Each PR below repeats what an agent needs from it, so the plan works without access to the review.

Decisions

These were settled with the user on 2026-11-12. Agents follow them and do not reopen them.

Topic	Decision
Decompression limit (S2)	256 MB of decompressed project data (lowered from 512 MB on 2026-11-12, during PR 2: a blank project is 3 MB and 256 MB holds about half a million recorded events).
Bound paths (S4)	Approved: LoadPath and AssignSamplePath stop being bound; export methods stop taking a path.
Pad labels (M6)	Always the "A1" format (bank letter, pad number without a leading zero).
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.

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.

Guidelines for agents

These apply to every PR. Where they conflict with a PR's tasks, ask before going on.

Branches and PRs

1. Work on one PR at a time, in the order below. Start each branch from the latest origin/main after the previous PR is merged.
2. Name the branch exactly as the plan does. Never push to main: it is protected on Gogs.
3. Push with git push -u origin <branch>, then give bajdev the compare link: https://git.monoexp.com/bajdev/mpckit/compare/main...<branch>. Agents cannot open PRs; bajdev opens and merges them.
4. Before adding commits to a pushed branch, check it is not merged yet (git fetch --prune, git branch -r --merged origin/main).
5. After bajdev says a PR is merged: git fetch --prune, delete the branch on Gogs by hand only if it is fully in origin/main (git push origin --delete <branch>), then git branch -d <branch>. MPCkit is not on the deploy pipeline, so nothing deletes it automatically.
6. End every commit message with the attribution line the session gives. Apart from that line, never put a model name in commits, code or PR records.

PR records

Every PR adds pull-requests/<branch>/<pr-summary>.md, like the existing ones (see pull-requests/mpckit-casing/):

• First line: # <PR title>, then Branch: `<branch>` → `main`.
• What changed, in plain words, naming the finding IDs fixed.
• Notes: decisions made along the way, anything left for later, questions for bajdev.
• Testing: what ran on the server, then numbered steps for bajdev's Windows laptop (cmd or PowerShell commands, not sh).

Checks before every push

The VPS has Node 24, the GTK 4 and WebKitGTK 6 build files and Xvfb, so all of these run there:

export PATH=$PATH:~/go/bin
go test ./...
go vet ./...
staticcheck ./...
govulncheck ./...
(cd frontend && npm ci && npm run check && npm run build)
wails3 build

npm ci needs the lockfile from PR 8; until then use npm install and do not commit the lockfile it writes. govulncheck will keep reporting GO-2026-6859 until Go 1.27.4 is installed; mention it in Testing, do not try to work around it. To check that the app starts, run timeout 15 xvfb-run -a bin/mpckit and read its log.

Code

• Match the code around the change: short functions, comments that say why, plain error messages. New identifiers follow the existing naming.
• Every bug fix comes with a test that fails before the fix. Build crafted project files inside the test (decode a file from testdata/device, change the tree, write it to t.TempDir()), rather than committing new binary fixtures.
• Do not change: testdata/device/* and testdata/accepted/*; the byte-identical round trip of .xpj files (the xpj tests prove it); the edit recipes in docs/xpj-format-and-scope.md; device limits in project.
• No new Go modules or npm packages without asking bajdev.
• After changing a bound method in internal/app, regenerate the bindings with wails3 task common:generate:bindings and commit frontend/bindings.
• Svelte 5: props are live, so a handler that clears the state behind a prop reads the prop first. Wails types Go slices as T[] | null; normalise with ?? [].
• The app is "MPCkit" wherever people see it; file names, the module path and mpckit-cli stay lowercase. Keep the "not affiliated with Akai Professional" notes.

Releases (PRs 4 and 9)

The README's "Releasing" paragraph lists the files that carry the version; change all of them. After bajdev merges, tag the merge commit with an annotated tag: git tag -a v0.5.2 -m "MPCkit 0.5.2 (beta)" <merge commit>, then git push origin v0.5.2.

PR order

0.5.2: security fixes
  PR 1 harden-load  →  PR 2 limit-decompression  →  PR 3 safe-export  →  PR 4 release-0-5-2 (tag v0.5.2)
0.5.3: hardening
  PR 5 unbind-paths  →  PR 6 atomic-edits  →  PR 7 a1-labels  →  PR 8 build-cleanup  →  PR 9 release-0-5-3 (tag v0.5.3)

Each PR starts from main after the previous one is merged. The order matters in three places: PR 3 reuses PR 1's file name check, PR 6 builds on the nodes PR 1 stores on Project, and PRs 5 and 7 both change internal/app and the frontend.

PRs and tasks

#	Branch	Fixes	Release
1	harden-load	S1 (load side), S3, M2	0.5.2
2	limit-decompression	S2	0.5.2
3	safe-export	S1 (export side and assign), M1, M5, R1 (part)	0.5.2
4	release-0-5-2	Version 0.5.2	0.5.2
5	unbind-paths	S4	0.5.3
6	atomic-edits	M3, M4	0.5.3
7	a1-labels	M6	0.5.3
8	build-cleanup	S6 (without CI), M7, M8, R1 (rest)	0.5.3
9	release-0-5-3	Version 0.5.3	0.5.3

PR 1. Reject malformed projects at load (harden-load)

Why. Pool file names come from the project file and are joined to folders as they are, so ../../../secret.txt reads and writes outside the project (S1). Missing fields cause nil-pointer panics: a pool entry without path in project.Load, a sequence without bpm in Sequences(). In the app, a panic in the file-drop handler exits MPCkit (S3). The project's main nodes are looked up again on every call with the error dropped (M2).

Tasks

• In project.wrap, resolve samples, tracks[0].samples, tracks[0].program.drum.instruments and sequences once and keep them on Project; pool(), trackPool(), pads() and sequences() return the stored nodes.
• In wrap or Load, check every pool entry has a string path, and every pad layer's sampleFile is a string or missing. Each name must be a plain file name: not empty, not . or .., no /, \ or control characters, and filepath.Base(name) == name. Refuse the file otherwise, naming the bad entry.
• Check each sequence has a numeric key, an object value, a number bpm and lengthBars, and a string name. Refuse the file with a clear error otherwise.
• Make xpj.Node.Int, Float and Lookup safe on a nil node (return an error or nil), and replace remaining direct .Text, .Int() and .Float() uses on lookups in project with text(), num() or a checked equivalent. This covers data.version in Load.
• In main.go, set PanicHandler in application.Options: log the panic and emit the error event with a short message, so a missed case cannot close the app.
• Add FuzzLoad in internal/project: seed it with the files in testdata/device, write each fuzzed input to t.TempDir(), load it and call Pad, Sequences and PoolFiles; none may panic.

Done when

• Tests show that loading a project with a pool path of ../x.wav, a/b.wav or .. fails, and so does a project missing path or bpm, each with an error naming the problem.
• go test -fuzz=FuzzLoad -fuzztime=2m ./internal/project finds nothing.
• All existing tests pass unchanged, including the round-trip tests.

PR 2. Cap decompressed size (limit-decompression)

Why. xpj.Read reads the whole gzip stream into memory. A 1 MB file padded with spaces used 3.1 GB and 10 seconds (S2).

Tasks

• Add const MaxDecompressed = 256 << 20 in internal/xpj with a comment giving the reason.
• In Read, read through io.LimitReader(zr, MaxDecompressed+1) and return xpj: project is larger than 256 MB when decompressed past the limit.

Done when

• A test writes a gzip stream that decompresses to just over the limit and gets the error. Build the stream in the test with gzip.NewWriter; do not commit it.
• A file at the limit still decodes.

PR 3. Safe export (safe-export)

Why. Export copies each pool file from its source and writes it into the data folder by name (S1, export side). It follows symlinks, so a link in a shared kit's data folder exports whatever it points to. With overwrite set, Folder deletes the old export before moving the new one in; if a rename fails, both are lost, although the comment promises otherwise (M1, R1). The export package has the lowest coverage, 74.2%, mostly on these paths (M5). Found after PR 1: AssignSample names a new pool file after the WAV's own file name, so on Linux or macOS a file called kick\one.wav is accepted and exported, and loading that export then fails PR 1's check. On Windows and on the device's card the backslash would also read as a folder separator.

Tasks

• In collect, use os.Lstat and refuse a source that is a symlink or not a regular file; report it like a missing file.
• In Folder and writeZip, refuse a pool name that is not a plain file name (same rule as PR 1), as a second line of defence.
• In AssignSample, make the new pool file name pass PlainFileName before uniqueFile: replace /, \ and control characters with _; if the result is empty, . or .., use sample.wav. The sample name (stem) follows from the cleaned file name. Do this in one helper in project with a comment giving the reason.
• Rewrite the overwrite step in Folder: rename the old .xpj and data folder to temporary names inside the temp folder, rename the new pair into place, then delete the old pair. If the second rename fails, put the old pair back.
• If dest does not exist, return export folder does not exist: <dest> instead of the bare stat error.
• Correct the Folder comment to say what the code now guarantees.
• Add tests for: a symlinked source, a bad pool name, overwrite when the new rename fails (make the destination name unusable, for example a directory where the .xpj should go), and a missing destination.

Done when

• Assigning a WAV named kick\one.wav (create it in the test; Linux allows the backslash) gives the pool name kick_one.wav, and the exported project loads again without error.
• Export of a project whose data folder holds a symlink fails and writes nothing.
• An overwrite that fails leaves the previous export complete.
• go test -cover ./internal/export shows at least 85%.

PR 4. Release 0.5.2 (release-0-5-2)

Tasks

• Change the version to 0.5.2 in every file the README's "Releasing" paragraph lists, and update the stale example in the Version comment in internal/app/app.go.
• Add toolchain go1.27.4 to go.mod so builds pick up the net/http fix (GO-2026-6859). If Go 1.27.4 is not yet released, stop and ask bajdev.
• The PR record lists what 0.5.2 fixes, for the beta testers.

Done when the laptop build shows "Version 0.5.2" in About and govulncheck ./... reports no vulnerabilities. After the merge, tag v0.5.2 ("MPCkit 0.5.2 (beta)").

PR 5. Stop exposing raw paths to the webview (unbind-paths)

Why. Any script in the webview can call LoadPath, AssignSamplePath, ExportFolder and ExportZip with any path; ExportFolder with overwrite set deletes a folder (S4). The frontend never calls the first two.

Tasks

• Mark LoadPath and AssignSamplePath with //wails:ignore. main.go's file-drop handler still calls AssignSamplePath from Go.
• Have ChooseExportFolder and ChooseZipPath store the chosen path on the Service; ExportFolder(overwrite) and ExportZip(overwrite) use it and fail if none was chosen. Clear it after a successful export and on New and Load.
• Update frontend/src/lib/store.svelte.ts (exportWith) for the new signatures; regenerate the bindings.
• Update internal/app tests.

Done when the generated service.ts has no method taking a path, export with and without overwrite still works on the laptop, and npm run check is clean.

PR 6. All-or-nothing edits (atomic-edits)

Why. AssignSample clears the pad and may add a pool entry before steps that can fail, so a failure leaves the pad empty. SetTune and SetMuteGroup set Dirty even when the write failed (M3). app writes p.Dirty and export reads p.Sources directly (M4).

Tasks

• In each edit method, do the lookups and checks that can fail first, then change the tree; set Dirty only after success.
• Add Project.MarkExported() and Project.Source(file) (string, bool); make Dirty and Sources unexported and update app, export, the CLI and the tools.
• Tests: a failing AssignSample (for example a pad whose layersv is empty) leaves the pad and the pool as they were, and Dirty unchanged.

Done when those tests pass and no package outside project writes project fields.

PR 7. One pad label format: "A1" (a1-labels)

Why. app.Label and slots.ts give "A01"; project.SlotName and the CLI use "A1". Decision: "A1" everywhere (M6).

Tasks

• Keep project.SlotName as the one Go implementation; remove app.Label and use SlotName.
• Change slotLabel in frontend/src/lib/slots.ts to "A1" and fix the comments that say "A01" (app.go, SlotGrid.svelte, slots.ts).
• Do not change sequence names such as "Sequence 01": that is the device's naming, not a slot label.
• Update tests that expect "A01".

Done when the pad grid, pad panel, sequence grid, confirm dialogs and the CLI summary all show "A1" style labels on the laptop, and the layout still fits the window with no scrolling.

PR 8. Build and repository clean-up (build-cleanup)

Why. No lockfile, so builds can differ (S6). Server-mode and Docker tasks from the Wails template would expose the bound methods over HTTP (S6). The version lives in several files and Service.Version() is unused (M7). Template leftovers hide the real build steps (M8). The wav package comment says it does not decode audio, but it does (R1).

Tasks

• Commit frontend/package-lock.json from npm install. Remove minimum-release-age from .npmrc (npm warns on it) or ask bajdev whether to keep it for pnpm.
• Remove the build:server, run:server, build:docker, run:docker and setup:docker tasks and their entries in the root Taskfile.yml.
• Remove the unused blocks in build/config.yml: the ios comment block, the fileAssociations sample and other: My Other Data.
• Remove the bound Service.Version() (the frontend uses About()); regenerate the bindings.
• Fix the wav package comment.

Done when npm ci works from the lockfile, wails3 build still builds on the server and the laptop, and wails3 task --list shows no server or Docker tasks.

PR 9. Release 0.5.3 (release-0-5-3)

Same steps as PR 4, with 0.5.3. The PR record lists what changed for testers, including the new "A1" labels. After the merge, tag v0.5.3 ("MPCkit 0.5.3 (beta)").

Backlog

Not scheduled. Each can become its own PR later, following the same guidelines; none blocks a release.

Finding	What	Where
S5	Preview decode holds a long sample in memory three times; stream frames to the response instead	internal/app/audio.go
M9	Dialog cancel is detected by comparing error text; pin it with a test	dialogs.go
M10	Internal error text (xpj: path ...) reaches the user; show one plain sentence and log the detail	internal/app, main.go
M11	state() stats every pool file and scans all events on each call, under the lock	internal/app/app.go
R2	Markup lines up to 250 characters; no formatter configured	App.svelte, SequencePanel.svelte
R3	One-letter names in long scopes (s, n, b, l in save())	SequencePanel.svelte, app.go
R4	Same helper names with different meanings (wrap, edit)	internal/project, internal/xpj, internal/app
R5	Long setup functions; split into named helpers	main.go, cmd/mpckit-cli/main.go
R6	Clever code without a note ("\x00" map keys, NaN-safe BPM check, order of checks in serveAudio)	pads.go, sequences.go, audio.go
R7	Three modals repeat the same backdrop, dialog and Escape code	frontend/src/lib/*Modal.svelte, PadPicker.svelte

Dropped: R8 (PR record layout) and CI (part of S6), by decision.