|
|
@@ -0,0 +1,232 @@
|
|
|
+# 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) | 512 MB of decompressed project data. |
|
|
|
+| 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), 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 = 512 << 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 512 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).
|
|
|
+
|
|
|
+**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.
|
|
|
+- [ ] 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**
|
|
|
+
|
|
|
+- 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.
|
