fix: give the fifteen hand-built album paths the guard resolve_path has (#534)

Extracts the guarded album-path core into handlers/_shared._album_dir, beside
_normalize_slug and _is_path_confined, so the helper a caller reaches for is the
one carrying the guards. Replaces fifteen hand-built album paths across six
handler modules; _genre_dir/_albums_dir cover the two truncations.

Only resolve_path ever applied the resolved confinement check, so confine is a
per-call-site decision tracking each site's prior behaviour — True at
resolve_path, False at the sites operating on an album directory that already
exists, which may legitimately be a symlink pointing outside its root
(test_symlinked_audio_dir_passes). The lexical traversal guard always applies.

_album_dir raises, and each call site reports the failure per its own contract:
_resolve_audio_dir returns its (error_json, None) tuple, update_album_status
adds a release_issues entry, and migrate_audio_layout records a per-album skip
so one unresolvable album cannot discard the report of files already moved.

resolve_path's output is unchanged except one deliberate tightening: genre='..'
is now rejected rather than returning …/albums/../al.

Adds 30 tests across tests/unit/handlers/test_album_paths.py and
test_album_path_call_sites.py.

Co-Authored-By: Claude <noreply@anthropic.com>

Author: MrReasonable

CHANGELOG.md

@@ -10,6 +10,10 @@ This project uses [Conventional Commits](https://conventionalcommits.org/) and [
 - **The lint/type-check toolchain is pinned exactly instead of floating** ([#532](https://github.com/bitwize-music-studio/claude-ai-music-skills/issues/532)) — `requirements.txt` pinned all 16 runtime deps with `==`, but every entry in `requirements-test.txt` used `>=`, so `ruff`, `mypy` and `bandit` resolved to whatever was newest on PyPI at the moment CI ran. Those three decide the Lint and Security Scan verdicts, and unlike a test runner they change their answer on unchanged code — a new rule or a widened check reddens a commit nobody touched, and re-running an old green build no longer reproduces it. The drift was already visible: the file read `ruff>=0.15.21` while CI had been installing `0.16.0`, which is why Dependabot closed #522 as redundant. `ruff`, `mypy` and `bandit` are now `==` pins (`cache: 'pip'` never mitigated this — it caches wheels, but pip still resolves to newest). The `pytest` stack stays on `>=`: it changes what runs, not what counts as a violation. A parametrized test in `tests/unit/shared/test_pinned_dependencies.py` keeps the three from silently loosening again.
 
 ### Fixed
+- **The album directory shape is written down once, and the callers that built it by hand now get the traversal guard** — `#529` removed `tools/shared/paths.py` on the grounds that it was the *unguarded* variant of path resolution and "the obvious helper for a contributor to reach for", leaving `handlers/core.py:resolve_path` as the single implementation. That diagnosis was right, but the duplication it was meant to prevent was already there and survived the removal: `Path(root) / "artists" / artist / "albums" / genre / slug` and its two truncations appeared at fifteen sites across six handler modules. The reason is structural rather than carelessness — `resolve_path` is an `async` MCP tool that returns a JSON string, so no library code can call it, and every caller that needed a path had no option but to respell it. Only `resolve_path` applied `_normalize_slug` and a confinement check; the rest interpolated straight in, which is the same weakness `#529` deleted a file over. The guarded core is now `handlers/_shared._album_dir`, living beside `_normalize_slug` and `_is_path_confined` so the helper a caller reaches for is the one that carries the guards, with `_genre_dir`/`_albums_dir` for the two truncations callers wanted. Still one implementation — just a callable one.
+  - Two guards, not one, and they are not the same guard. A **lexical** pass always runs: it rejects a `..` or separator-carrying `artist`, `genre` or `subdir` *before* the layout is rendered, and rejects an absolute segment outright rather than silently relativizing it into a confined-but-wrong path. A **resolved** confinement check (`resolved.is_relative_to(root)`) is separate, because it is not a traversal guard — it also rejects a symlinked album directory, which is a *supported* layout (`test_symlinked_audio_dir_passes`). Only `resolve_path` ever applied it, so each call site passes the `confine` value matching what it did before centralising: `True` at `resolve_path`, `False` at the sites that operate on an album directory which already exists (`_resolve_audio_dir`, `update_album_status`, `migrate_audio_layout`, `rename_album`, `validate_album_structure`). It defaults to `True` so a *new* caller who forgets fails closed and loudly.
+  - **`resolve_path`'s output is unchanged except for one deliberate tightening:** a `genre` of `..` used to return `…/albums/../al`, and is now rejected with the same `Resolved path escapes root directory` error every other escape returns. Every other input, including `genre="/etc"`, returns exactly what it did before.
+  - `_album_dir` raises, so each call site catches it and reports the failure the way its own contract says: `_resolve_audio_dir` returns its `(error_json, None)` tuple rather than raising past the twenty-two call sites that branch on it; `update_album_status` adds a `release_issues` entry rather than collapsing the aggregated gate into one opaque error; and `migrate_audio_layout` records a per-album skip, so one unresolvable album cannot discard the report of files it has already physically moved into `originals/`.
 - **`tracks_completed` no longer flips between two different numbers depending on which code path last ran** ([#523](https://github.com/bitwize-music-studio/claude-ai-music-skills/issues/523)) — `reference/state-schema.md` defines the field as "Number of tracks with completed status", but only the incremental track-change path counted the track files. A full rebuild (`scan_albums`) and the incremental README-changed branch both counted the album README's `## Tracklist` table instead. Since `update_track_field` rewrites a track file without touching that table, the two drifted the moment a track's status changed: `rebuild_state()` — the documented remedy for a stale cache — discarded the correct count and reinstated the README's stale one, and editing only the README silently reset it. This also made `list_albums` and `get_album_progress` disagree about the same album, and made the CLI print a README-derived numerator over an actual-file denominator (a finished album could render as `[0/12 tracks]`). All three sites now derive the count from the track files through a single `_count_completed_tracks` helper. `parse_album_readme` still reports what the README table claims, but nothing feeds it into state.
 - **A corrupt `ideas` or `skills` section in state.json no longer crashes `python -m tools.state update`** ([#525](https://github.com/bitwize-music-studio/claude-ai-music-skills/issues/525)) — `incremental_update` type-guards its top-level state sections so a wrong-typed one returns `None` and the caller falls back to a full rebuild (the `#393` contract), but the guard only covered `config` and `albums`. The function also does `state['ideas'].get('file_mtime')` and `state['skills'].get('skills_root')`, so a non-mapping value in either raised `AttributeError` straight past `cmd_update`'s `is None` fallback and aborted the CLI with a traceback — the exact failure the guard exists to prevent. All four sections are now guarded; both are re-derived from disk on a rebuild, so falling back loses nothing.
 - **`.gitignore` no longer lists `TESTING.md` as a build artifact** ([#526](https://github.com/bitwize-music-studio/claude-ai-music-skills/issues/526)) — the entry sat in the "Build artifacts" block between `*.egg-info/` and `.coverage`, but `TESTING.md` is a tracked contributor doc referenced from `skills/test/test-definitions.md`. The rule was inert (`.gitignore` does not affect already-tracked files) but would have silently dropped the doc for anyone who ran `git rm --cached` or re-added it after a delete.

servers/bitwize-music-server/handlers/_shared.py

@@ -159,6 +159,123 @@ def _normalize_slug(name: str) -> str:
     return slug
 
 
+# The album directory shape, written down once. Every album mirrors this same
+# relative path under content_root, audio_root and documents_root.
+#
+# Two callers want the layout truncated rather than whole, so the segments above
+# ``{genre}`` are named first and the full shape is built from them. The
+# truncation is then a shared prefix rather than string surgery on a rendered
+# template, which would couple to the literal ``{genre}`` token.
+_ALBUMS_SEGMENTS = ("artists", "{artist}", "albums")
+ALBUM_LAYOUT_SEGMENTS = (*_ALBUMS_SEGMENTS, "{genre}", "{album}")
+ALBUM_LAYOUT = "/".join(ALBUM_LAYOUT_SEGMENTS)
+
+PATH_ESCAPES_ROOT = "Resolved path escapes root directory"
+
+
+def _album_dir(
+    root: str | Path,
+    *,
+    artist: str,
+    genre: str,
+    album: str,
+    subdir: str = "",
+    confine: bool = True,
+) -> Path:
+    """Resolve one album's directory under *root*, with the traversal guards applied.
+
+    This is the guarded resolution that ``core.py:resolve_path`` performs;
+    ``resolve_path`` is the MCP-tool wrapper around it. It lives here, beside
+    ``_normalize_slug`` and ``_is_path_confined``, so that the helper a caller
+    reaches for is the one that carries the guards — #529 removed a
+    ``tools/shared/paths.py`` that interpolated the slug straight in, and the
+    stated risk was precisely that a contributor would reach for the unguarded
+    variant.
+
+    *root* is whichever of content_root, audio_root or documents_root is
+    wanted; the shape below it is identical for all three.
+
+    Args:
+        root: Root directory from config.
+        artist: Artist name from config.
+        genre: Genre slug.
+        album: Album slug. Normalized here — callers need not pre-normalize,
+            and passing an already-normalized slug is idempotent.
+        subdir: Optional child directory, e.g. ``"tracks"``. Included in the
+            confinement check rather than appended after it.
+        confine: Also require the *resolved* path to stay within *root*.
+
+            This is a per-call-site decision, not a global one, because only
+            ``resolve_path`` ever applied it: pass what the site did before.
+            ``True`` at ``resolve_path``. ``False`` at the sites that operate on
+            an album directory that already exists, because such a directory is
+            allowed to be a symlink pointing outside its root
+            (``test_symlinked_audio_dir_passes``) and resolving rejects that
+            supported layout. The lexical guard below applies either way.
+
+            Defaults to ``True`` so a new caller who forgets fails closed and
+            loudly — a break gets found, a silently dropped guard does not.
+
+    Returns:
+        The resolved directory. Not created.
+
+    Raises:
+        ValueError: *album* contains a path separator, a null byte or a
+            traversal sequence (from ``_normalize_slug``); *artist*, *genre* or
+            *subdir* is a traversal or carries a separator; or, under
+            ``confine``, the result escapes *root* despite all of that.
+    """
+    normalized = _normalize_slug(album)
+
+    # Lexical guard, always on, and applied to the caller-supplied values
+    # *before* the layout is rendered. _normalize_slug already rejects traversal
+    # and separators in the album slug, but artist and genre come from config
+    # and state without passing through it, and subdir is a caller literal.
+    #
+    # Checking before rendering rather than after matters: splitting the
+    # rendered template would turn genre="/etc" into a bare "etc" segment and
+    # hand back a confined-but-wrong path. develop rejected that input, and a
+    # wrong path is a worse failure mode than an error.
+    for value in (artist, genre, subdir):
+        if value and (value == ".." or "/" in value or "\\" in value or "\0" in value):
+            raise ValueError(PATH_ESCAPES_ROOT)
+
+    relative = ALBUM_LAYOUT.format(artist=artist, genre=genre, album=normalized)
+    base = Path(root)
+    for segment in [*relative.split("/"), subdir]:
+        if not segment:
+            # An empty genre collapses, exactly as Path("a") / "" always has.
+            continue
+        base = base / segment
+
+    # Resolved confinement, opt-out. This is the check resolve_path has always
+    # applied, and it catches what the lexical pass cannot: a symlink *inside*
+    # the album path that points outside the root.
+    if confine and not base.resolve().is_relative_to(Path(root).resolve()):
+        raise ValueError(PATH_ESCAPES_ROOT)
+
+    return base
+
+
+def _albums_dir(root: str | Path, *, artist: str) -> Path:
+    """Directory holding every genre for one artist — the layout above ``{genre}``.
+
+    Used by the callers that sweep across genres, because album slugs are
+    globally unique rather than unique per genre (#392).
+
+    ``artist`` is trusted config — it is the same component ``_album_dir``
+    guards, and is unguarded here only because it comes from the user's own
+    ``artist_name`` rather than from a tool argument. Callers that append an
+    album slug use ``_is_path_confined`` or ``_album_dir``.
+    """
+    return Path(root).joinpath(*(s.format(artist=artist) for s in _ALBUMS_SEGMENTS))
+
+
+def _genre_dir(root: str | Path, *, artist: str, genre: str) -> Path:
+    """Directory holding every album of one genre — ``_album_dir``'s parent."""
+    return _albums_dir(root, artist=artist) / genre
+
+
 def _json_sanitize(value: Any) -> Any:
     """Recursively replace non-finite floats (inf/-inf/nan) with None.
 
@@ -556,7 +673,23 @@ def _resolve_audio_dir(album_slug: str, subfolder: str = "") -> tuple[str | None
         return _safe_json({
             "error": f"Genre not found for album '{album_slug}'. Ensure album exists in state.",
         }), None
-    audio_path = Path(audio_root) / "artists" / artist / "albums" / genre / normalized
+    # confine=False preserves this funnel's prior behaviour: it never had a
+    # resolved check, and an album's audio directory is allowed to be a symlink
+    # pointing outside audio_root — the layout validate_album_structure passes
+    # (test_symlinked_audio_dir_passes) must resolve here too, since every audio
+    # tool (master, polish, qc, transcribe, promo, sheet music) comes through
+    # this function. The lexical traversal guard still applies.
+    #
+    # The catch is the contract: this returns (error_json_or_None, Path_or_None),
+    # and every caller branches on the first element. A raise would skip all of
+    # those structured error paths, exactly as the _normalize_slug catch above
+    # exists to prevent.
+    try:
+        audio_path = _album_dir(
+            audio_root, artist=artist, genre=genre, album=normalized, confine=False,
+        )
+    except ValueError as exc:
+        return _safe_json({"error": str(exc)}), None
     if subfolder:
         if not _is_path_confined(audio_path, subfolder):
             return _safe_json({

servers/bitwize-music-server/handlers/album_ops.py

@@ -14,11 +14,14 @@
     _SECTION_NAMES,
     STATUS_UNKNOWN,
     TRACK_COMPLETED_STATUSES,
+    _album_dir,
+    _albums_dir,
     _extract_code_block,
     _extract_markdown_section,
     _find_album_or_error,
     _find_slug_dirs,
     _find_wav_source_dir,
+    _genre_dir,
     _get_valid_genres,
     _is_path_confined,
     _normalize_slug,
@@ -202,7 +205,18 @@ async def validate_album_structure(
     artist = config.get("artist_name", "")
     album_path = album.get("path", "")
     genre = album.get("genre", "")
-    audio_path = str(Path(audio_root) / "artists" / artist / "albums" / genre / normalized)
+    # confine=False: an album's audio directory is allowed to be a symlink
+    # pointing outside audio_root (test_symlinked_audio_dir_passes). This is a
+    # read-only existence check, and the lexical traversal guard still applies.
+    #
+    # Caught rather than raised: this handler returns a JSON report, and a raise
+    # would discard the checks already accumulated.
+    try:
+        audio_path = str(_album_dir(
+            audio_root, artist=artist, genre=genre, album=normalized, confine=False,
+        ))
+    except ValueError as exc:
+        return _safe_json({"error": str(exc)})
 
     passed = 0
     failed = 0
@@ -375,7 +389,7 @@ async def create_album_structure(
                     "generation.additional_genres in config.",
         })
 
-    albums_base = Path(content_root) / "artists" / artist / "albums" / genre_slug
+    albums_base = _genre_dir(content_root, artist=artist, genre=genre_slug)
     # Defense-in-depth: verify slug stays within the genre directory
     if not _is_path_confined(albums_base, normalized):
         return _safe_json({"error": "Invalid album slug: would escape album directory"})
@@ -386,7 +400,7 @@ async def create_album_structure(
 
     # Album slugs are globally unique across genres (#392) — sweep the
     # filesystem (cache may be stale) for the slug under every genre.
-    albums_root = Path(content_root) / "artists" / artist / "albums"
+    albums_root = _albums_dir(content_root, artist=artist)
     collisions = _find_slug_dirs(albums_root, normalized)
     if collisions:
         existing = collisions[0]

servers/bitwize-music-server/handlers/core.py

@@ -23,6 +23,7 @@
     TRACK_GENERATED,
     TRACK_IN_PROGRESS,
     TRACK_NOT_STARTED,
+    _album_dir,
     _extract_code_block,
     _extract_markdown_section,
     _find_track_or_error,
@@ -690,14 +691,19 @@ async def resolve_path(path_type: str, album_slug: str, genre: str = "") -> str:
         "audio": audio_root,
         "documents": documents_root,
     }
-    root_dir = Path(root_map[path_type]).resolve()
-    base = Path(root_map[path_type]) / "artists" / artist / "albums" / genre / normalized
-    if path_type == "tracks":
-        base = base / "tracks"
-
-    # Defense-in-depth: verify resolved path stays within its root directory
-    if not base.resolve().is_relative_to(root_dir):
-        return _safe_json({"error": "Resolved path escapes root directory"})
+    try:
+        base = _album_dir(
+            root_map[path_type],
+            artist=artist,
+            genre=genre,
+            album=normalized,
+            subdir="tracks" if path_type == "tracks" else "",
+            # Stated rather than inherited: this is the one site that has always
+            # applied the resolved confinement check, and it must keep it.
+            confine=True,
+        )
+    except ValueError as exc:
+        return _safe_json({"error": str(exc)})
 
     resolved = str(base)
 

servers/bitwize-music-server/handlers/maintenance.py

@@ -8,7 +8,7 @@
 from typing import Any
 
 from handlers import _shared
-from handlers._shared import _normalize_slug, _resolve_audio_dir, _safe_json
+from handlers._shared import _album_dir, _normalize_slug, _resolve_audio_dir, _safe_json
 
 logger = logging.getLogger(__name__)
 
@@ -221,7 +221,27 @@ async def migrate_audio_layout(
             skipped_count += 1
             continue
 
-        audio_dir = Path(audio_root) / "artists" / artist / "albums" / genre / slug
+        # confine=False: this site never had a resolved check, and it operates on
+        # album directories that already exist — which may be symlinks pointing
+        # outside audio_root. The lexical traversal guard still applies.
+        #
+        # Caught per album rather than allowed to propagate: the call sits inside
+        # the loop, so in migrate-all mode a raise on album N would discard the
+        # whole results report after files for albums 1..N-1 had already been
+        # physically moved into originals/.
+        try:
+            audio_dir = _album_dir(
+                audio_root, artist=artist, genre=genre, album=slug, confine=False,
+            )
+        except ValueError as exc:
+            results.append({
+                "slug": slug,
+                "status": "skipped",
+                "files_moved": [],
+                "skip_reason": f"could not resolve audio path: {exc}",
+            })
+            skipped_count += 1
+            continue
 
         if not audio_dir.is_dir():
             results.append({