ソースを参照

Plan: add PR 8b, bind only what the window needs

//wails:ignore only hides methods from the generated bindings; Wails
registers every exported method of the bound Service by reflection, so
the window can still call LoadPath and AssignSamplePath by name. PR 8b
moves the Go-only methods to an unbound type, pins the bound method set
with a test, and forgets the export target when replace is declined.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude 5 時間 前
親
コミット
f6af7c4fdc

+ 23 - 4
docs/review-fixes-plan.md

@@ -1,6 +1,6 @@
 # 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.
+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
 
@@ -11,7 +11,7 @@ These were settled with the user on 2026-11-12. Agents follow them and do not re
 | 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. |
+| 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. |
@@ -77,10 +77,10 @@ The README's "Releasing" paragraph lists the files that carry the version; chang
 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)
+  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 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.
+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
 
@@ -94,6 +94,7 @@ Each PR starts from `main` after the previous one is merged. The order matters i
 | 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`)
@@ -212,6 +213,24 @@ Each PR starts from `main` after the previous one is merged. The order matters i
 
 **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", "<any path>")`; 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)").

+ 23 - 0
pull-requests/plan-bound-surface/add-pr-8b-bound-surface.md

@@ -0,0 +1,23 @@
+# Plan: add PR 8b, bind only what the window needs
+
+Branch: `plan-bound-surface` → `main`
+
+## What changed
+
+`docs/review-fixes-plan.md` gets a new PR 8b (`bound-surface`), between PR 8 and PR 9, part of 0.5.3.
+
+Found while checking PR 5 on `main` (`d66ff0a`): `//wails:ignore` only keeps a method out of the generated bindings. Wails 3.1.0 registers every exported method of the bound `Service` at runtime by reflection, so the window can still call `LoadPath` and `AssignSamplePath` by name with any path.
+
+- **Tasks:** move the Go-only methods (`LoadPath`, `AssignSamplePath`, `Dirty`, `SetCloser`, `AudioMiddleware`) to an unbound type that `main.go` uses; add `TestBoundMethods`, which pins the list of bound methods; add `CancelExport()` so a declined replace forgets the export target.
+- **Done when:** the test passes, a scratch build's `Call.ByName(".../app.Service.LoadPath", path)` is rejected, and the laptop checks pass.
+- The PR table, the order and the release line now include 8b.
+
+No code changes.
+
+## Notes
+
+Merge this before the PR 8b agent starts. PR 9 must wait for PR 8b.
+
+## Testing
+
+Documentation only. Checked on 2026-11-13: a throwaway build of `main` (`f56cd2d`) whose `main.ts` called `Call.ByName("git.monoexp.com/bajdev/mpckit/internal/app.Service.LoadPath", ".../testdata/device/Demo02.xpj")` loaded the project (the call returned its state). Nothing from that build was committed.