# MPCkit review fixes: work plan Written 2026-11-12; PR 8b added 2026-11-13. Ten 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, including 8b, 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 and 8b. 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. ## 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 `, then give bajdev the compare link: `https://git.monoexp.com/bajdev/mpckit/compare/main...`. 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 `), then `git branch -d `. 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//.md`, like the existing ones (see `pull-requests/mpckit-casing/`): - First line: `# `, then ``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)" `, 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 8b bound-surface → 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 four places: PR 3 reuses PR 1's file name check, PR 6 builds on the nodes PR 1 stores on `Project`, PRs 5 and 7 both change `internal/app` and the frontend, and PR 8b finishes what PR 5 started and must be merged before PR 9. ## 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 | | 8b | `bound-surface` | S4 (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: ` 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 8b. Bind only what the window needs (`bound-surface`) **Why.** PR 5 hid `LoadPath` and `AssignSamplePath` with `//wails:ignore`, but that directive only keeps them out of the generated `frontend/bindings`. At runtime Wails 3.1.0 registers every exported method of a bound type by reflection (`getMethods` in `pkg/application/bindings.go`), and reflection cannot see comments. So a script in the window can still call `Call.ByName("git.monoexp.com/bajdev/mpckit/internal/app.Service.LoadPath", "")`; on 2026-11-13 a scratch build did exactly that and Go loaded the file. The same holds for `AssignSamplePath` and the other Go-only methods (`Dirty`, `SetCloser`, `AudioMiddleware`). Also, when the user declines "Replace existing files?", Go keeps the chosen export target, so a later `ExportFolder(true)` would overwrite it without asking. **Tasks** - [ ] Keep `Service` as the type `main.go` binds, with only the methods the frontend calls. Move the Go-only methods (`LoadPath`, `AssignSamplePath`, `Dirty`, `SetCloser`, `AudioMiddleware`) to a second, unbound type in `internal/app` that shares the same state, for example `type Host struct{ s *Service }`, with `NewService` returning both. `Service.Load` and `AssignSampleDialog` call unexported helpers that `Host` also uses. Remove the `//wails:ignore` directives that are no longer needed. - [ ] Update `main.go` to use `Host` for the close hook, the file-drop handler and the audio middleware. - [ ] Add `TestBoundMethods` in `internal/app`: list the exported methods of `*Service` by reflection and compare them with an explicit list in the test. Its failure message says that a new bound method must not take a file path and must be added to the list on purpose. - [ ] Add a bound `CancelExport()` that forgets the chosen targets, and call it from `exportWith` in `store.svelte.ts` when the user declines the replace prompt. - [ ] Regenerate the bindings; update the `internal/app` tests. **Done when** - `TestBoundMethods` passes, and fails if a method such as `LoadPath(string)` is added back to `Service`. - In a scratch build (temporary lines in `frontend/src/main.ts`, not committed), `Call.ByName(".../app.Service.LoadPath", path)` is rejected with "unknown bound method name". Say in the PR record how this was checked. - On the laptop: Load, file drop onto a pad, the window-close prompt, export to folder and zip (with and without replace, and declining replace), and sample preview all work as before. ### 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.