Extract the pack-path containment guard into one helper

Every manifest key that names a file carried its own copy of the same
traversal guard: resolve, prove containment under source_dir, warn and
skip on ValueError, warn and skip on OSError. Seven copies —
original_audio, drum_tab, arrangement, notation, song_timeline, lyrics,
keys — which is seven chances for the next side-file to get a security
check subtly wrong by copying the wrong neighbour.

Route them all through `_resolve_pack_path(source_dir, rel, label)`.

Deliberately preserved, because each was load-bearing:

- Both exception branches, with their different messages. ValueError
  means the path resolved outside the pack (a crafted or broken
  manifest); OSError means it could not be resolved at all (symlink
  loop, permissions). They send an operator to different places.
- Per-call-site control flow. The helper returns `Path | None` and says
  nothing about what to do next, so the two sites that return, the one
  that continues, and the four that fall through to an `is not None`
  test each keep the shape they had.
- The existence-check asymmetry. Some sites test `.exists()` (or
  `.is_file()`) after resolving and some do not, which is intentional —
  a missing optional side-file is silent, a missing arrangement skips an
  entry — so existence stays out of the helper entirely.

Pure refactor: no behaviour change and no new validation. Log output is
byte-identical (the hardcoded labels become a `%s` argument rendering to
the same text). Full suite is unchanged at 2774 passed / 4 skipped
before and after, and the five loader-level traversal tests that used to
cover five separate copies of the guard now all exercise the same
function.

Signed-off-by: gionnibgud <gionnibgud@gmail.com>

lib/sloppak.py

@@ -121,6 +121,41 @@ def partition_stems(stems: list[dict]) -> tuple[dict | None, list[dict]]:
     return full, [s for s in stems if str(s.get("id", "")) != FULL_MIX_STEM_ID]
 
 
+def _resolve_pack_path(source_dir: Path, rel: str, label: str) -> Path | None:
+    """Resolve a manifest-relative path, contained inside the pack. None if not.
+
+    Every manifest key that names a file routes through here. A crafted manifest
+    must not read outside the sloppak directory via path traversal
+    (e.g. `../../etc`), and a symlink loop or permission error on `.resolve()`
+    must disable that one file rather than abort the whole load — so both
+    failures are caught, and both are warnings rather than raises.
+
+    The two branches log differently on purpose: a `ValueError` means the path
+    resolved *outside* the pack (a crafted or broken manifest), an `OSError`
+    means it could not be resolved at all (symlink loop, permissions). Reading
+    "escapes source_dir" in the logs and reading "resolution failed" lead an
+    operator to very different places, so the distinction is worth two lines.
+
+    Returns the resolved path — **existence is NOT checked here**. Callers
+    differ on that deliberately: a missing optional side-file is silent, while a
+    missing arrangement skips an entry, so each caller keeps its own `.exists()`
+    (or `.is_file()`) test and its own control flow.
+
+    `label` names the manifest key in the log message ("keys", "song_timeline",
+    a drum part's id, …).
+    """
+    try:
+        p = (source_dir / rel).resolve()
+        p.relative_to(source_dir.resolve())
+    except ValueError:
+        log.warning("sloppak: %s path %r escapes source_dir — skipped", label, rel)
+        return None
+    except OSError as e:
+        log.warning("sloppak: %s path resolution failed (%s) — skipped", label, e)
+        return None
+    return p
+
+
 def _legacy_full_mix(manifest: dict, source_dir: Path) -> str | None:
     """Full mix from the DEPRECATED `original_audio:` manifest key, or None.
 
@@ -152,16 +187,8 @@ def _legacy_full_mix(manifest: dict, source_dir: Path) -> str | None:
     if not isinstance(rel_raw, str) or not rel_raw.strip():
         return None
     rel = rel_raw.strip()
-    try:
-        target = (source_dir / rel).resolve()
-        target.relative_to(source_dir.resolve())
-    except ValueError:
-        log.warning("sloppak: original_audio path %r escapes source_dir — skipped", rel)
-        return None
-    except OSError as e:
-        log.warning("sloppak: original_audio path resolution failed (%s) — skipped", e)
-        return None
-    if not target.is_file():
+    target = _resolve_pack_path(source_dir, rel, "original_audio")
+    if target is None or not target.is_file():
         return None
     log.info(
         "sloppak: pack uses the deprecated `original_audio:` key (%r) — the full mix "
@@ -749,20 +776,8 @@ def _load_drum_tab_file(source_dir: Path, rel: str, label: str) -> dict | None:
     permissive — a missing file disables that part silently; a traversal,
     parse, or validation failure disables it with a warning, never aborting
     the load."""
-    # Constrain to source_dir to prevent a crafted manifest from reading
-    # files outside the sloppak directory via path traversal (e.g. ../../etc).
-    # Wrap both resolve() calls in a broad handler: symlink loops and
-    # permission errors on .resolve() should disable drums, not abort load.
-    try:
-        dt_path = (source_dir / rel).resolve()
-        dt_path.relative_to(source_dir.resolve())
-    except ValueError:
-        log.warning("sloppak: %s path %r escapes source_dir — skipped", label, rel)
-        return None
-    except OSError as e:
-        log.warning("sloppak: %s path resolution failed (%s) — skipped", label, e)
-        return None
-    if not dt_path.exists():
+    dt_path = _resolve_pack_path(source_dir, rel, label)
+    if dt_path is None or not dt_path.exists():
         return None
     try:
         raw = load_json(dt_path)
@@ -909,16 +924,8 @@ def load_song(
             continue
         data = None
         if rel:
-            try:
-                arr_path = (source_dir / rel).resolve()
-                arr_path.relative_to(source_dir.resolve())
-            except ValueError:
-                log.warning("sloppak: arrangement path %r escapes source_dir — skipped", rel)
-                continue
-            except OSError as e:
-                log.warning("sloppak: arrangement path resolution failed (%s) — skipped", e)
-                continue
-            if not arr_path.exists():
+            arr_path = _resolve_pack_path(source_dir, rel, "arrangement")
+            if arr_path is None or not arr_path.exists():
                 continue
             try:
                 data = load_json(arr_path)
@@ -980,15 +987,7 @@ def load_song(
         notation_rel = notation_rel.strip()
         if not notation_rel:
             continue
-        try:
-            nt_path = (source_dir / notation_rel).resolve()
-            nt_path.relative_to(source_dir.resolve())
-        except ValueError:
-            log.warning("sloppak: notation path %r escapes source_dir — skipped", notation_rel)
-            nt_path = None
-        except OSError as e:
-            log.warning("sloppak: notation path resolution failed (%s) — skipped", e)
-            nt_path = None
+        nt_path = _resolve_pack_path(source_dir, notation_rel, "notation")
         raw_nt = None
         if nt_path is not None and nt_path.exists():
             try:
@@ -1061,15 +1060,7 @@ def load_song(
     time_sigs_data: list | None = None
     song_timeline_rel = manifest.get("song_timeline")
     if isinstance(song_timeline_rel, str) and song_timeline_rel:
-        try:
-            st_path = (source_dir / song_timeline_rel).resolve()
-            st_path.relative_to(source_dir.resolve())
-        except ValueError:
-            log.warning("sloppak: song_timeline path %r escapes source_dir — skipped", song_timeline_rel)
-            st_path = None
-        except OSError as e:
-            log.warning("sloppak: song_timeline path resolution failed (%s) — skipped", e)
-            st_path = None
+        st_path = _resolve_pack_path(source_dir, song_timeline_rel, "song_timeline")
         if st_path is not None and st_path.exists():
             try:
                 raw = load_json(st_path)
@@ -1159,15 +1150,7 @@ def load_song(
     # downstream through the WS path.
     lyrics_rel = manifest.get("lyrics")
     if isinstance(lyrics_rel, str) and lyrics_rel:
-        try:
-            lyr_path = (source_dir / lyrics_rel).resolve()
-            lyr_path.relative_to(source_dir.resolve())
-        except ValueError:
-            log.warning("sloppak: lyrics path %r escapes source_dir — skipped", lyrics_rel)
-            lyr_path = None
-        except OSError as e:
-            log.warning("sloppak: lyrics path resolution failed (%s) — skipped", e)
-            lyr_path = None
+        lyr_path = _resolve_pack_path(source_dir, lyrics_rel, "lyrics")
         if lyr_path is not None and lyr_path.exists():
             try:
                 raw = load_json(lyr_path)
@@ -1272,15 +1255,7 @@ def load_song(
     keys_data: dict | None = None
     keys_rel = manifest.get("keys")
     if isinstance(keys_rel, str) and keys_rel:
-        try:
-            k_path = (source_dir / keys_rel).resolve()
-            k_path.relative_to(source_dir.resolve())
-        except ValueError:
-            log.warning("sloppak: keys path %r escapes source_dir — skipped", keys_rel)
-            k_path = None
-        except OSError as e:
-            log.warning("sloppak: keys path resolution failed (%s) — skipped", e)
-            k_path = None
+        k_path = _resolve_pack_path(source_dir, keys_rel, "keys")
         if k_path is not None and k_path.exists():
             try:
                 raw = load_json(k_path)

tests/test_content_packs_vst.py

@@ -0,0 +1,91 @@
+"""VST-pack slicing (tools/content_packs.build_vst_pack): each platform pack
+keeps only its own binaries + the shared bundle files, drops the rest, and is
+reproducible."""
+import zipfile
+from pathlib import Path
+
+from tools import content_packs
+
+
+def _fake_vst_tree(root: Path):
+    # One fat .vst3 with all three platform binaries + shared files, plus a
+    # src/ build tree that must never ship.
+    c = root / "amps" / "Foo.vst3" / "Contents"
+    (c / "MacOS").mkdir(parents=True)
+    (c / "x86_64-win").mkdir(parents=True)
+    (c / "x86_64-linux").mkdir(parents=True)
+    (c / "Resources").mkdir(parents=True)
+    (c / "MacOS" / "Foo").write_bytes(b"mac-binary")
+    (c / "x86_64-win" / "Foo.vst3").write_bytes(b"win-binary")
+    (c / "x86_64-linux" / "Foo.so").write_bytes(b"linux-binary")
+    (c / "Info.plist").write_bytes(b"<plist/>")
+    (c / "Resources" / "moduleinfo.json").write_bytes(b"{}")
+    (root / "src" / "build").mkdir(parents=True)
+    (root / "src" / "build" / "junk.o").write_bytes(b"objfile")
+
+
+def _names(zip_path):
+    with zipfile.ZipFile(zip_path) as zf:
+        return set(zf.namelist())
+
+
+def test_slice_keeps_target_platform_and_shared_drops_foreign(tmp_path):
+    root = tmp_path / "vst"
+    _fake_vst_tree(root)
+    content_packs.build_vst_pack(root, tmp_path / "mac.zip", "mac")
+    names = _names(tmp_path / "mac.zip")
+
+    base = "amps/Foo.vst3/Contents"
+    assert f"{base}/MacOS/Foo" in names                 # target binary kept
+    assert f"{base}/Info.plist" in names                # shared kept
+    assert f"{base}/Resources/moduleinfo.json" in names  # shared kept
+    assert f"{base}/x86_64-win/Foo.vst3" not in names    # foreign dropped
+    assert f"{base}/x86_64-linux/Foo.so" not in names    # foreign dropped
+    assert not any(n.startswith("src/") for n in names)  # build trees never ship
+
+
+def test_each_platform_gets_its_own_binary(tmp_path):
+    root = tmp_path / "vst"
+    _fake_vst_tree(root)
+    wanted = {"mac": "MacOS/Foo", "win": "x86_64-win/Foo.vst3", "linux": "x86_64-linux/Foo.so"}
+    for plat, rel in wanted.items():
+        content_packs.build_vst_pack(root, tmp_path / f"{plat}.zip", plat)
+        names = _names(tmp_path / f"{plat}.zip")
+        assert f"amps/Foo.vst3/Contents/{rel}" in names
+        others = [v for k, v in wanted.items() if k != plat]
+        for o in others:
+            assert f"amps/Foo.vst3/Contents/{o}" not in names
+
+
+def test_slice_is_reproducible(tmp_path):
+    root = tmp_path / "vst"
+    _fake_vst_tree(root)
+    a = content_packs.build_vst_pack(root, tmp_path / "a.zip", "linux")
+    b = content_packs.build_vst_pack(root, tmp_path / "b.zip", "linux")
+    assert a == b and a["sha256"]
+
+
+def test_slice_pins_create_system_for_cross_runner_reproducibility(tmp_path, monkeypatch):
+    # ZipInfo defaults create_system from the host OS (0 on Windows, 3 on Unix),
+    # and it lands in the central directory — so without an explicit pin the same
+    # tree hashes differently on a Windows runner, breaking the precomputable-hash
+    # guarantee exactly where it matters (native .vst3 are built on Windows). A
+    # same-machine reproducibility test can't catch that; simulate win32 and
+    # assert the pin forces 3 regardless.
+    monkeypatch.setattr(zipfile.sys, "platform", "win32")
+    root = tmp_path / "vst"
+    _fake_vst_tree(root)
+    content_packs.build_vst_pack(root, tmp_path / "w.zip", "linux")
+    with zipfile.ZipFile(tmp_path / "w.zip") as zf:
+        assert all(i.create_system == 3 for i in zf.infolist())
+
+
+def test_unknown_platform_rejected(tmp_path):
+    root = tmp_path / "vst"
+    _fake_vst_tree(root)
+    try:
+        content_packs.build_vst_pack(root, tmp_path / "x.zip", "bsd")
+    except ValueError as e:
+        assert "unknown platform" in str(e)
+    else:
+        raise AssertionError("build_vst_pack accepted an unknown platform")

tools/content_packs.py

@@ -74,6 +74,52 @@ def build_pack(src_dir: Path, out_zip: Path) -> dict:
     return {"sha256": hashlib.sha256(data).hexdigest(), "bytes": len(data)}
 
 
+# rig_builder ships "fat" .vst3 bundles carrying all three platforms inside
+# Contents/. A pack for one platform keeps that platform's binary dir + the
+# shared bundle files, and drops the other two.
+VST_PLATFORM_DIRS = {"mac": "MacOS", "win": "x86_64-win", "linux": "x86_64-linux"}
+
+
+def build_vst_pack(vst_root: Path, out_zip: Path, platform: str) -> dict:
+    """Reproducibly zip the VST tree keeping only `platform`'s binaries.
+
+    Slices each fat .vst3: everything is kept except the two foreign platform
+    dirs (MacOS / x86_64-win / x86_64-linux) and the vst/src build trees. Arc
+    names are relative to vst_root so the download endpoint extracts straight
+    into <plugin>/vst/. Same reproducible-build guarantees as build_pack.
+    """
+    if platform not in VST_PLATFORM_DIRS:
+        raise ValueError(f"unknown platform {platform!r} (want mac/win/linux)")
+    foreign = set(VST_PLATFORM_DIRS.values()) - {VST_PLATFORM_DIRS[platform]}
+    files = []
+    for p in sorted(vst_root.rglob("*"), key=lambda q: q.as_posix()):
+        if not p.is_file():
+            continue
+        rel = p.relative_to(vst_root)
+        if rel.parts and rel.parts[0] == "src":      # skip C++/JUCE build trees
+            continue
+        if set(rel.parts) & foreign:                 # drop foreign-platform binaries
+            continue
+        files.append((p, rel))
+    if not files:
+        raise ValueError(f"no VST files to pack for {platform} in {vst_root}")
+    out_zip.parent.mkdir(parents=True, exist_ok=True)
+    with zipfile.ZipFile(out_zip, "w", zipfile.ZIP_STORED) as zf:
+        for p, rel in files:
+            info = zipfile.ZipInfo(rel.as_posix(), date_time=(1980, 1, 1, 0, 0, 0))
+            info.compress_type = zipfile.ZIP_STORED
+            # Pin create_system like build_pack: ZipInfo defaults it from the
+            # host OS (0 on Windows, 3 on Unix), which would otherwise make the
+            # same pack hash differently across runners. VST packs are the most
+            # likely to be built on Windows (native .vst3), so without this pin
+            # the precomputable-hash guarantee breaks exactly where it's needed.
+            info.create_system = 3
+            info.external_attr = 0o644 << 16
+            zf.writestr(info, p.read_bytes())
+    data = out_zip.read_bytes()
+    return {"sha256": hashlib.sha256(data).hexdigest(), "bytes": len(data)}
+
+
 def manifest_entry(out_zip: Path, url: str) -> dict:
     """Pack info as the download-path expects it: {url, sha256, bytes}."""
     return {"url": url,
@@ -96,25 +142,47 @@ def pack_url(pack_id: str, version: int, repo: str = REPO) -> str:
             f"{pack_tag(pack_id, version)}/{pack_asset(pack_id, version)}")
 
 
-def publish(pack_id: str, version: int, zip_path: Path, repo: str = REPO) -> None:
+# VST packs use the same immutable per-pack convention, keyed by platform:
+# tag `vst-<plat>-v<N>`, asset `vst-<plat>-pack-v<N>.zip`. The manifest they
+# emit is keyed by platform (mac/win/linux) — the shape the rig_builder plugin's
+# data/vst_packs.json consumes.
+def vst_tag(platform: str, version: int) -> str:
+    return f"vst-{platform}-v{version}"
+
+
+def vst_asset(platform: str, version: int) -> str:
+    return f"vst-{platform}-pack-v{version}.zip"
+
+
+def vst_url(platform: str, version: int, repo: str = REPO) -> str:
+    return (f"https://github.com/{repo}/releases/download/"
+            f"{vst_tag(platform, version)}/{vst_asset(platform, version)}")
+
+
+def _publish_release(tag: str, zip_path: Path, title: str, notes: str,
+                     repo: str = REPO) -> None:
     """Create the per-pack release if missing, then upload the versioned zip.
 
     Tags are immutable: a media change means a new version (v1 → v2), never a
     re-upload — so no --clobber. gh errors if the asset already exists, which is
     the right guard against overwriting a published, referenced pack.
     """
-    tag = pack_tag(pack_id, version)
     if subprocess.run(["gh", "release", "view", tag, "--repo", repo],
                       capture_output=True).returncode != 0:
         subprocess.run(
             ["gh", "release", "create", tag, "--repo", repo, "--latest=false",
-             "--title", f"{pack_id.capitalize()} venue pack v{version}",
-             "--notes", "Opt-in career venue pack. Not a code release."],
+             "--title", title, "--notes", notes],
             check=True)
     subprocess.run(
         ["gh", "release", "upload", tag, str(zip_path), "--repo", repo], check=True)
 
 
+def publish(pack_id: str, version: int, zip_path: Path, repo: str = REPO) -> None:
+    _publish_release(pack_tag(pack_id, version), zip_path,
+                     f"{pack_id.capitalize()} venue pack v{version}",
+                     "Opt-in career venue pack. Not a code release.", repo)
+
+
 def _pack_id(src_dir: Path) -> str:
     return src_dir.name
 
@@ -129,6 +197,10 @@ def main(argv=None) -> int:
                     help="write zips here + a file:// manifest.json; no upload")
     ap.add_argument("--publish", action="store_true",
                     help="create/upload the per-pack release; emit release URLs")
+    ap.add_argument("--vst", action="store_true",
+                    help="slice one rig VST root (src[0]) into per-platform "
+                         "vst-<plat>-v<N> packs; manifest keyed by platform "
+                         "(the shape rig_builder's data/vst_packs.json wants)")
     ap.add_argument("--manifest", type=Path,
                     help="write the {id: {url,sha256,bytes}} map here (default: stdout)")
     ap.add_argument("--selfcheck", action="store_true", help="run the round-trip demo and exit")
@@ -141,16 +213,30 @@ def main(argv=None) -> int:
 
     out_dir = args.local if args.local else Path(args.src[0]).parent / "_packs"
     manifest = {}
-    for src in args.src:
-        pid = _pack_id(src)
-        zip_path = out_dir / pack_asset(pid, args.version)
-        build_pack(src, zip_path)
-        if args.publish:
-            publish(pid, args.version, zip_path)
-            url = pack_url(pid, args.version)
-        else:
-            url = (out_dir.resolve() / zip_path.name).as_uri()
-        manifest[pid] = manifest_entry(zip_path, url)
+    if args.vst:
+        vst_root = args.src[0]
+        for plat in VST_PLATFORM_DIRS:
+            zip_path = out_dir / vst_asset(plat, args.version)
+            build_vst_pack(vst_root, zip_path, plat)
+            if args.publish:
+                _publish_release(vst_tag(plat, args.version), zip_path,
+                                 f"Rig VST pack ({plat}) v{args.version}",
+                                 "Opt-in per-platform rig VST pack. Not a code release.")
+                url = vst_url(plat, args.version)
+            else:
+                url = (out_dir.resolve() / zip_path.name).as_uri()
+            manifest[plat] = manifest_entry(zip_path, url)
+    else:
+        for src in args.src:
+            pid = _pack_id(src)
+            zip_path = out_dir / pack_asset(pid, args.version)
+            build_pack(src, zip_path)
+            if args.publish:
+                publish(pid, args.version, zip_path)
+                url = pack_url(pid, args.version)
+            else:
+                url = (out_dir.resolve() / zip_path.name).as_uri()
+            manifest[pid] = manifest_entry(zip_path, url)
 
     out = json.dumps(manifest, indent=2)
     if args.manifest:
@@ -183,6 +269,23 @@ def _selfcheck() -> int:
         with zipfile.ZipFile(zip_path) as zf:
             names = zf.namelist()
         assert set(names) == {"manifest.json", "bored.mp4"}, names
+
+        # VST slice: keep target platform + shared, drop foreign, reproducible.
+        c = td / "vst" / "Foo.vst3" / "Contents"
+        for d in ("MacOS", "x86_64-win", "x86_64-linux", "Resources"):
+            (c / d).mkdir(parents=True)
+        (c / "MacOS" / "Foo").write_bytes(b"mac")
+        (c / "x86_64-linux" / "Foo.so").write_bytes(b"linux")
+        (c / "Info.plist").write_bytes(b"<plist/>")
+        vzip = td / vst_asset("linux", 1)
+        vinfo = build_vst_pack(td / "vst", vzip, "linux")
+        assert vinfo == build_vst_pack(td / "vst", td / "v2.zip", "linux"), \
+            "vst slice is not reproducible"
+        with zipfile.ZipFile(vzip) as zf:
+            vnames = set(zf.namelist())
+        assert "Foo.vst3/Contents/x86_64-linux/Foo.so" in vnames
+        assert "Foo.vst3/Contents/Info.plist" in vnames
+        assert not any("MacOS" in n for n in vnames), vnames
     print("content_packs selfcheck: ok")
     return 0