mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-22 21:01:40 +00:00
fix/folder-library-hover-preview
100 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
K. O. A.
|
35c0d0ea0d
|
Pass per-stem name/description through to the stems payloads (#1013)
Some checks are pending
ship-ci / ci (push) Waiting to run
feedpak 1.16.0 (spec §5.3) added two OPTIONAL presentational fields to a
stems[] entry: `name` (display label, Readers fall back to the id) and
`description` (free text). The server dropped both while normalizing
manifest stems, so no client could ever display them.
Pass them through at the one place stem descriptors are built
(sloppak.load_song) and let both payload builders — the WS `ready` stems
list and the REST `/api/song/{f}?stems=1` preload list, which are pinned
against each other by test — carry them forward. Omit-when-absent, so a
stem without the fields does not grow null keys; non-string or blank
values are dropped rather than surfaced.
No behaviour change for existing packs or clients: the fields are
additive and every consumer that reads {id,url,default} keeps working
unchanged. The stems plugin / stem mixer display work lands separately.
Signed-off-by: topkoa <topkoa@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
ChrisBeWithYou
|
f7942f3689
|
fix(gp8): confine registry asset matching to the declared directory (#1011)
`<EmbeddedFilePath>` is matched on filename stem so a format variant of the same recording can win — an `.ogg` beside the declared `.mp3` is copied out losslessly rather than transcoded. But the search spanned every directory in the archive, so an unrelated file that merely shared the stem could stand in for the declared asset: exactly the substitution the registry lookup added in #1007 exists to prevent. Candidates are now confined to the registry path's own directory. A genuinely absent asset still falls through to the legacy stem match and then the first audio asset, as documented. Found by an adversarial pass over #1007 rather than a report — no known file triggers it, since GP8 writes embedded audio to Content/Assets/ and that is the only directory scanned. It needs a hand-edited archive to reach. Both tests fail on main and pass here; their ZIP ordering is deliberate, so the fall-through target differs from the decoy (otherwise fixed and unfixed code return the same file and the tests prove nothing). Claude-Session: https://claude.ai/code/session_01SFDokqh2H6mEjk1Kgbi6JW Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
ChrisBeWithYou
|
1cd6f2dd65
|
fix(gp8): AssetId is a key into <Assets>, not a filename stem (#1007)
* fix(gp8): AssetId is a key into <Assets>, not a filename stem
GPIF declares the backing track's audio as:
<BackingTrack><AssetId>0</AssetId>
<Assets><Asset id="0">
<EmbeddedFilePath>Content/Assets/<hash>.mp3</EmbeddedFilePath>
so AssetId indexes the <Assets> registry, which names the exact path in
the ZIP. `_resolve_audio_asset` instead compared it against each audio
file's FILENAME STEM. GP8 names embedded files by hash while ids are
small integers, so that match essentially never hit: every such file
logged "declared AssetId not found" and fell through to "first audio
asset". Silently correct while a file carries exactly ONE audio asset —
but with two, a backing track declaring id 1 resolved to asset 0, i.e.
the wrong recording, for both extract_sync and extract_audio.
Found while verifying embedded-audio extraction for a reported GP8
import; that file logged the warning on the normal path.
- `_asset_path_from_registry()` reads <Asset id=N><EmbeddedFilePath>,
normalising separators (a writer may emit backslashes). It never
decides a path exists — the caller verifies membership in the archive,
since the value comes out of the file and a stale entry must fall
through rather than resolve to nothing.
- Resolution is now a ladder: registry → legacy stem match → first audio
asset. Steps 2 and 3 are the previous behaviour, kept so existing
files and odd shapes are unaffected. Same-stem OGG preference is
preserved on the registry path too, so quality behaviour is unchanged.
Tests: registry resolution on the real-world shape (integer id, hashed
filename), the second asset finally being reachable (the actual bug), a
registry entry pointing at a missing file falling through, backslash
normalisation, OGG preference among same-stem duplicates, malformed and
absent registries degrading, and the legacy stem match still working.
Suite 1725 passed vs 1720 on main, same 99 pre-existing env failures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
* docs(changelog): record the GP8 AssetId resolution fix
Every other change in this release notes itself; this one shipped without
an entry, and the GP import path has had three fixes in two days — the
history is worth being able to read later.
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
---------
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
ChrisBeWithYou
|
32ed564006
|
fix(gp2rs): write arrangement XML as UTF-8 so GP import survives non-ASCII metadata (#984)
The GP→arrangement-XML writers persisted their output with
`Path.write_text(xml_str)` and no explicit encoding. On Windows that uses
the cp1252 default, so a non-ASCII metadata character — e.g. the © in an
album name like "Chrysalis©1982" — was written as the lone byte 0xA9.
The XML is read back as UTF-8 (expat's default), where 0xA9 is an invalid
start byte, so `parse_arrangement` died with:
xml.etree.ElementTree.ParseError: not well-formed (invalid token): line 10, column 22
and the whole Guitar Pro import failed (HTTP 500). All three arrangement
XML writes (gp2rs.py, gp2rs_gpx.py ×2) now pin encoding="utf-8".
CI runs on Linux (UTF-8 default) so the bug was invisible there; the new
test pins the locale-independent contract at the source level plus a
round-trip of a © album name.
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
ChrisBeWithYou
|
1712803dc7
|
feat(notation_lift): authored per-note hands steer the split — heuristic only guesses the rest (#992)
The keys LH/RH hand arc, the lift slice. split_hands was purely heuristic (mean pitch vs middle C / largest-gap), so ANY edit to a keys arrangement re-derived hand splits that could contradict the score's authored grand-staff assignment — the documented "produces wrong hand splits" failure, now that authored hands actually reach the wire (editor #299 emits per-note `hand`; core #990 round-trips it). - decode_wire_notes carries `hand` through ('lh'/'rh' strict enum; junk → None so a hand-edited pack can't steer the split). - split_hands: an authored hand always wins, and explicit notes are REMOVED from their simultaneous group BEFORE the heuristic math runs — one authored assignment must never skew its chordmates' guesses (e.g. an authored LH melody note above middle C dragging the group mean down and flipping the rest). All-explicit groups skip the heuristic entirely; unassigned notes behave exactly as before. Design per the piano-pedagogy review of the arc: binary lh/rh + absent = unassigned; per-note explicit > heuristic precedence; crossing-hands textures are exactly why the override is load-bearing. Tests: five new in test_notation_lift.py (authored wins incl. a crossing-hands case, group-removal-before-math with exact mean arithmetic, all-explicit group, junk enum, decode carry-through); the decode shape pin updated for the new key. Suite: 1723 passed (+5 vs main; the pre-existing env failures reproduce identically on pristine main). Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
ChrisBeWithYou
|
00fce2772d
|
feat(song): per-note keys hand assignment (hand) on the Note wire (#990)
The editor's keys LH/RH hand arc needs a per-note hand assignment
('lh'/'rh', from a MusicXML grand staff import today, hand-editable
later) to survive a sloppak save → reload: the editor already emits it
on the wire, but note_from_wire dropped unknown keys, so the field died
on every reopen.
- Note gains `hand: str | None = None` (None = unassigned; the
heuristic hand split keeps owning unassigned notes). Distinct from
`right_hand` (the bass plucking finger) — hence the spelled-out
`hand` wire key, since `rh` is taken.
- note_to_wire emits it default-omitted and validates on emit; older
readers ignore it (feedpak: unknown note keys are permitted).
- note_from_wire decodes it as a strict enum — anything but 'lh'/'rh'
(junk, wrong case, bools) falls back to unassigned rather than
poisoning downstream hand-split / hands-separate practice logic.
Groundwork consumers land separately: notation_lift.split_hands
respecting per-note overrides, and the editor's hand surface.
Editor counterpart: feedBack-plugin-editor #299.
Tests: three new wire round-trip tests in tests/test_song.py (literal
key, default-omitted, junk rejection both directions), matching the
teaching-marks test style.
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
Co-authored-by: ChrisBeWithYou <chris@rifflarr.local>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
ChrisBeWithYou
|
1745b13ba7
|
feat(playlists): flag songs that are not in your current tuning (#1009)
* feat(playlists): flag songs that are not in your current tuning
Making the library's tuning filter instrument-aware does not repair playlists
already built under the old guitar-first behaviour. Those keep their
wrong-tuning songs, so a player still hits a surprise retune mid-practice and
reasonably concludes nothing was fixed.
Adds a per-playlist check: each row is marked against the player's current
tuning, with a summary ("3 of 24 songs are not in your tuning"), a filter to
show only those, and an explicit removal that lists every affected song by
title and states they stay in the library. Flagging is the feature -- nothing
is ever removed without being asked for, and removal reuses the existing
per-song DELETE rather than adding a bulk destructive endpoint.
Reuses the tuner capability's coverage report and `window.feedBack
.workingTuning`, the same pair the library cards already score against,
rather than introducing another source of truth.
Two deliberate departures:
- A coverage report reads "not covered" both for a real mismatch and for a
bail-out it could not evaluate. Only a report carrying an actual reason
counts as a mismatch; the rest render as unknown. This differs from the
library grid, which paints every not-covered song amber -- acceptable on a
grid, not on a hand-curated playlist where a false warning costs trust.
- With no tuning perspective available it makes no claim at all, rather than
defaulting to guitar and reproducing the original bug in a new place.
Playlist rows carry `tuning_offsets` and `bass_only`; a tuning *name* cannot
be scored, since two "Custom Tuning" rows are different tunings.
Fully correct once the instrument-aware tuning filter lands. That dependency
is confined to `rowTuningForCheck()` in static/v3/playlists.js, marked SEAM.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SFDokqh2H6mEjk1Kgbi6JW
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
* build(tailwind): regenerate for the playlist tuning-check classes
CI's tailwind-fresh gate rebuilds static/tailwind.min.css and hard-fails if
the committed file differs. The new chip/summary/filter markup introduces
classes the previous build never saw.
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
* fix(playlists): stay within the shipped Tailwind class set
Reverts the regenerated static/tailwind.min.css and reworks the tuning-check
markup to use only classes already in the committed sheet.
Regenerating that file is not reproducible off CI: nothing pins tailwindcss,
autoprefixer or caniuse-lite, so a local `npx -y tailwindcss@3.4.19` resolves
different browser data and rewrites unrelated bytes -- a clean checkout of
main rebuilds with the -webkit-backdrop-filter prefixes dropped. Committing
that output fails the tailwind-fresh gate no matter how many times it is
regenerated.
Six utilities were new: bg-fb-good/10, border-fb-accent/50,
hover:bg-fb-accent/10, list-disc, list-inside, max-h-48, plus gap-x-3/gap-y-2.
Substituted bg-fb-good/30, the amber border already used by the mismatch
state, hover:bg-fb-card, a literal bullet in a div, max-h-32 and gap-3. Visual
intent is unchanged.
The removal-confirm test pinned the <li> markup; it now accepts either
wrapper, since what it guards is that every song is named and escaped ahead
of any DELETE, not which element wraps it.
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
* Use instrument tuning in playlist checks
---------
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
ChrisBeWithYou
|
cc75cb876a
|
fix(library): tuning filter answers for your instrument, not always guitar (#1003)
* fix(library): tuning filter answers for your instrument, not always guitar The library indexed exactly one tuning per song, chosen guitar-first (lead > rhythm > combo, bass only as a last resort), and nothing consulted the player's instrument. A bassist filtering by tuning was shown the guitar chart's tuning, so playlists built by tuning contained songs needing a retune. Reported by a tester building bass practice sets; Covet "Shibuya" is the clean case, with a custom guitar tuning over a standard bass chart. Indexes each arrangement role's own tuning and makes the facet, filter, sort and labels answer for one perspective. `guitar-lead` reads the original unprefixed columns and adds no payload keys, so the default response is unchanged. The same defect existed inside guitar -- lead and rhythm charts can disagree -- so perspective is three-valued (guitar-lead, guitar-rhythm, bass) driven by one PERSPECTIVES table rather than parallel column families. Songs with no chart for the perspective fall back to the song-level tuning rather than vanishing (18 of 59 packs in the test library have no bass chart), but the fallback is marked inferred in the facet counts and on the row instead of being silently coalesced. "Only real charts" reuses the existing `arrangements_has` filter rather than adding one. Bass-specific handling, from measured content: - Bass tuning arrays are padded to six entries; charts never reference string index 4 or 5. Truncated to four before naming and grouping. - Grouping uses a canonical open-pitch key, so [-2,0,0,0] and [-2,0,0,0,0,0] are one facet row instead of two. - Offsets above +1 semitone are refused a name. Bassists tune down, near never up; one pack ships [5,5,5,5,4,4] (A-D-G-C, unplayable, and its own notes sit in the song's real key under standard tuning). Naming that would send a player to retune to a tuning that does not exist. Rhythm deliberately does not truncate -- padding is a bass finding, and cutting a seven-string array would invent a tuning the chart lacks. Adds an opt-in `tuning_match=playable` mode alongside exact match: a chart is offered when your lowest open pitch is at or below its lowest open pitch, so a five-string bass covers four-string standard and drop-D with no retune. Open strings only -- note range is not indexed and the scan stays manifest-only -- so it fails conservative: unknown low pitch is excluded, and the upper bound is unchecked and documented rather than guessed. Existing installs would otherwise never populate: the tree-signature fast path reports "unchanged" forever on a settled library. Rows with NULL marker columns re-extract, and the fast path is disabled until that backfill converges (writes use '' rather than NULL, so it self-clears). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFDokqh2H6mEjk1Kgbi6JW Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> * test(v3): accept the tuning-perspective indirection in the badge guard The album-art badge now reads shownTuningName(), so the source-pattern guard no longer matched the inline `tuning_name || tuning` form and CI went red. Accept the helper, and pin the helper's own fallback in a companion test so the guard still fails if a guitar player's tuning label is ever dropped. Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> --------- Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
ChrisBeWithYou
|
f0d9c3abc0
|
feat(playlists): manual drag order + Sort A-Z for the playlist list (#1004)
Playlists could only ever be listed alphabetically (system playlists first). Users who group playlists by purpose had no way to put the ones they reach for daily at the front. Adds a nullable `position` column and orders by `(system_key IS NULL), (position IS NULL), position, name COLLATE NOCASE`, so manually-ordered playlists lead, unpositioned ones keep sorting alphabetically behind them, and system playlists stay pinned first. Drag-reorder mirrors the existing within-playlist song reorder, adapted for grid tiles (insert side decided on the horizontal midpoint since tiles flow left-to-right then wrap). System playlists are neither drag sources nor drop targets. `POST /api/playlists/reorder` requires an exact permutation of the current non-system ids, so a duplicate, omission, extra, unknown id, or a system id is rejected rather than silently producing duplicate positions; booleans are rejected explicitly because `sorted([True, 2]) == sorted([1, 2])` would otherwise slip through the permutation check. `POST /api/playlists/sort-alpha` clears the manual order again. Claude-Session: https://claude.ai/code/session_01SFDokqh2H6mEjk1Kgbi6JW Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
0b4b174d33
|
perf(scan): skip full library re-stat when the tree is unchanged (#979)
Some checks are pending
ship-ci / ci (push) Waiting to run
* perf(scan): skip full library re-stat when the tree is unchanged
Startup scans globbed the whole DLC tree twice (*.feedpak, *.wem) and
stat()'d every file to detect changes — ~100k filesystem round trips on
a 50k-song library, and painful on a slow NTFS-3G FUSE mount (the "big
drive churns on every launch" report).
Adds/removes/renames of songs all bump the mtime of the containing
directory (verified on the target mount), so after a full pass we persist
{reldir: mtime_ns} for every library dir (scan_dir_signature.json, keyed
by DLC path). The next scan re-stats only those dirs — a handful vs 100k
ops — and skips the entire listing/stat pass when none changed.
Blind spot: a pack rewritten in place under the same name bumps the file
mtime but not its dir's. Rare for a song library, and the manual Refresh
(/api/rescan + /api/rescan/full) now passes force=True to always do the
full pass. force threads through kick_scan -> _scan_runner and coalesces
like the rescan-pending flag.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(scan): track directory-form songs' own dir in the signature
CodeRabbit: _library_dirs recorded only each song's parent. For a
directory-form song (loose-song folder or directory sloppak bundle),
adding/removing/replacing a file INSIDE the folder bumps that folder's
own mtime, not its parent's — so the fast path would skip a rescan it
should run. Record the song's own dir when f.is_dir(). File-form
sloppaks (a single .feedpak zip) aren't dirs, so the flat file library
is unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
939c98214b
|
feat(song-info): publish the playable stem list so stems can preload (fixes the 698ms freeze) (#972)
* feat(song-info): publish the playable stem list, so stems can preload
The stems plugin could only learn its stem list from the highway's WS `ready`,
which arrives once the highway is already up. So it fetched, decoded, and then
handed every stem's PCM to its audio worklet — copying the WHOLE SONG — with the
player already on screen.
For a 4-minute 6-stem pack that is over half a GIGABYTE of memcpy, in one frame,
on the main thread. Measured on a real load: a 698 ms frame, right as the
song-credits card appeared, with the venue video visibly stopping. That is the
"the video pauses when the author appears" report.
GET /api/song/{f}?stems=1 now returns the same list — [{id, url, default}] plus
full_mix_url — so the plugin can start the whole load at `song:loading`, before
the highway (and the venue) is drawn, where a stalled frame costs nothing.
Nothing about the work changes; only WHEN.
Opt-in via the query param so the library's own metadata calls — the hot path —
pay nothing. Deliberately NOT stored in the metadata cache: that is a
fixed-column table, and widening it would mean a schema migration plus a stale
row for every song already scanned, to cache something that is a plain manifest
read on an already-unpacked pack.
The safety property: REST and the WS must publish the SAME list. If they
disagreed the plugin would preload a graph and then throw it away and rebuild —
strictly worse than not preloading. So both now resolve `default` through one
shared helper (stem_default_on, extracted from load_song), and a test rebuilds
the WS's payload from load_song and requires the REST helper to produce the
identical list, rather than pinning either against a snapshot.
Also pinned: the mixdown is lifted OUT of the stem list (spec 5.3 — `full` is
not a layer; listing it beside the instruments would play the whole song on top
of the stems) while staying reachable as full_mix_url, a single-`full` pack keeps
it as its only playable stem, and an unreadable pack yields an empty list rather
than failing the request. Full suite 2608 passed.
Consumed by feedBack-plugin-stems (preloadSong).
* fix(song-info): call load_song for the stem payload — do not reimplement it
CodeRabbit caught a real bug, and it would have hit most real libraries.
load_song() falls back to the DEPRECATED `original_audio:` key when a pack has no
reserved `full` stem — which is every pack written before feedpak 1.15.0. My
payload rebuilt the full-mix rule from extract_meta and returned None for those:
REST would say "no full mix" while the WS said there was one.
Worse than a wrong field: the plugin would preload a graph WITHOUT the pristine
mix and — because the stem signature still matched — never rebuild. Unity
playback would silently downgrade to the lossy stem recombination.
That is exactly the drift this PR claims to prevent, and my test had a hole: I
only covered packs that carry a `full` stem.
So stop reimplementing. The payload now calls load_song, whose LoadedSloppak
already carries the partitioned stems and the resolved full mix, and builds the
URLs exactly as ws_highway does. Drift is now impossible by construction rather
than by agreement. extract_meta is reverted to its original shape (it never
needed to change), and the shared stem_default_on helper stays as the one place
`default: off` is resolved.
Tests rewritten to compare against load_song — the WS's own function — for a
reserved-`full` pack, a LEGACY original_audio pack (the case that was broken), and
a single-`full` pack. Also documents the `?stems=1` contract in CHANGELOG.md.
Full suite green.
|
||
|
Byron Gamatos
|
e729c44d5b
|
perf(paths): resolve the library root once, not on every path check (#966)
`Path.resolve()` is a filesystem call — it lstats every component of the path.
`_resolve_dlc_path` and `safe_join` both re-resolved their ROOT on every single
call, and those run once per song, per art fetch, per scanned row.
Found while profiling a 2-fps report: on a real 50,944-song library the server
was issuing ~23,500 stat/lstat calls per second, re-walking the same three
parent directories over and over, and burning ~50% of a core doing it. It is
worst exactly where big libraries live — the library was on an NTFS-3G (FUSE)
mount, where every stat is a userspace round trip through mount.ntfs-3g (itself
visible in top). The cost was the constant re-resolution, not the work.
A root is fixed for the life of the process, so resolve it once
(safepath.resolved_root, lru_cache). Measured, 5,000 lookups against a real
library path:
before: 15,264 stat syscalls (54.2 ms)
after: 277 stat syscalls ( 0.8 ms) 55x fewer
Containment is unchanged, which is the part that matters:
- safe_join still resolves the CANDIDATE on every call — following its
symlinks IS the zip-slip / traversal defence, so it is never cached. Only
the server-owned root is.
- _resolve_dlc_path keeps its lexical containment check (deliberately does not
follow symlinks, so junction-mounted libraries keep working).
Tradeoff, documented on resolved_root: if a root's symlink is re-pointed at a
NEW target while the server runs, the old target stays in effect until restart.
Fine for a library path fixed at startup; the cache is keyed on the Path, so
switching library dir is a different key.
Tests: root resolved once across 500 lookups (the regression), a different root
is a different entry, and the containment contract re-pinned — traversal,
Windows drive-absolute, backslash, NUL, empty, and a symlink escaping the root
must still be refused. Full suite green.
|
||
|
K. O. A.
|
0d35228d56
|
fix: remote transcription posts to /transcribe, not /align (stem-splitter#17) (#959)
Some checks are pending
ship-ci / ci (push) Waiting to run
* fix: remote transcription posts to /transcribe, not /align (stem-splitter#17)
transcribe_vocals_remote() POSTed the vocal stem to /align. That endpoint is FORCED ALIGNMENT —
"here are the lyrics, tell me when each word is sung" — and its `text` field is required. We have
no lyrics; transcribing them is the entire point. So the server rejected every request with a 422
from FastAPI's validation layer, before its handler ever ran, and remote transcription has never
worked for anyone.
It now posts to /transcribe (added in feedBack-demucs-server#14), which takes only the audio.
`language` moves from the query string to the FORM BODY, where the server actually reads it
(Form("")). As a query param it was silently ignored, so an explicit hint did nothing and
Whisper's auto-detection quietly decided instead — loading the wrong wav2vec2 aligner. It
"worked", it was just wrong, which is the failure mode that hides for months.
Error bodies are no longer cut at 300 chars. The body IS the diagnosis: a 422's JSON names the
field it rejected, a 500's traceback answers on its LAST line. Both got decapitated — which is
part of why this stayed invisible for so long. The message explaining the bug was inside the part
that got cut.
Nothing caught any of this because every test of this module tested the MAPPER, fed a hand-written
dict. The mapper was always fine. The request was never exercised, and the request was the bug.
tests/test_lyrics_transcribe_remote.py now pins it: the endpoint, the form field, the multipart
upload, the bearer token, an instrumental returning no lyrics rather than an error, and a 404
saying the server is too old. Verified they FAIL against /align + params.
Signed-off-by: topkoa <topkoa@gmail.com>
* fix: make the error-body cap an actual bound; correct the docstring's endpoint
- _err_body() appended the truncation marker AFTER slicing to _MAX_ERR_BODY, so the result could
exceed the cap it exists to enforce (4014 chars for a 4000 bound). A cap that is only a
suggestion surprises exactly the callers who trust it — a log line, a job record persisted to
disk and re-read on every load. The marker now fits inside the bound.
It also stripped after measuring, so a short JSON body followed by kilobytes of trailing
whitespace got truncated: real content cut to make room for blanks. Strip first, then measure.
- The public docstring still advertised /align — the exact contract this PR exists to change, in
the one place a reader would look for it. It now says what the function does and why, and that
an older server answers 404.
Found by Copilot and CodeRabbit on #959.
Signed-off-by: topkoa <topkoa@gmail.com>
* fix: keep the exception line when truncating — the tail is the answer
_err_body() kept only the HEAD of an over-long body. On a traceback the last line is the
diagnosis, and the docstring said exactly that while the code threw it away: a 4000-char window
holding "Traceback (most recent call last)" and none of the exception is a window onto nothing.
Same mistake as the 300-char cap it replaced, one level up — cutting off precisely the part the
function exists to preserve.
Head AND tail now, both inside the bound: two thirds head (what was being attempted), one third
tail (what actually went wrong), with the marker between them. Verified the test FAILS against
head-only truncation.
Found by Copilot on #959.
Signed-off-by: topkoa <topkoa@gmail.com>
* fix: every failure out of transcribe_vocals_remote() is a RuntimeError; 404 says why
The docstring promised one failure mode — RuntimeError — and the caller (_maybe_transcribe_lyrics)
catches exactly that so one song's failed lyrics don't take down the batch around it. But a DNS
failure, a timeout, a reset connection or an unreadable stem escaped as requests.RequestException
or OSError, walked straight past that handler, and turned "this song's lyrics failed" into "the
whole batch died".
A 404 now explains itself. Bare "404" sends someone hunting for a typo in their server URL; the
real answer is that their server predates /transcribe, and we are the only ones in a position to
know that.
Found by Copilot on #959.
Signed-off-by: topkoa <topkoa@gmail.com>
---------
Signed-off-by: topkoa <topkoa@gmail.com>
|
||
|
Byron Gamatos
|
329cc86315
|
fix(sloppak): the full mix is a stem — drop the invented original_audio key (#946)
* fix(sloppak): the full mix is a stem — drop the invented `original_audio` key (#933) Core read, served, and depended on `original_audio:` — a top-level manifest key this repo invented in #583 that the feedpak spec never defined. The format already had a home for the pre-separation mixdown: it is a stem. feedpak 1.15.0 (feedpak-spec#53) RESERVES the id `full` for it, so read it from there. The key existed to work around a bug in our own reader. The packer's comment said so plainly: "we must NOT list the full mix as a playable stem — the player sums every entry in `stems` and does not gate playback on `default`, so a listed full mix plays on top of the stems". Faced with a reader that would double the song, the packer put the mixdown outside `stems` and invented a key to point at it. The fix belongs in the reader, and that is what this is. load_song() now partitions the stem list: `full` comes out as LoadedSloppak.full_mix, the instruments stay in .stems. Nothing that sums stems or draws one fader per stem can see the mixdown, so retaining it is safe — which is what lets the packer put it where the format says it goes. - ws_highway: `song_info` gains full_mix_url / has_full_mix. The old original_audio_url / has_original_audio remain as deprecated aliases for one release so an older stems plugin keeps working (#945). - `stems` on the wire, and stem_ids / stem_count in the library index, are now INSTRUMENT stems only — a separated pack that retains its mixdown no longer advertises a bogus "full" chip or an inflated stem count. - enrichment: fingerprint against the mixdown wherever it lives. This widens coverage — _song_audio_file() previously returned None for any pack without the invented key, so fingerprinting silently did nothing for nearly every pack. - sloppak: `original_audio:` is still READ as a deprecated fallback, because every pack in the wild carries it and would otherwise lose its pristine mix. tools/migrate_full_mix_stem.py rewrites those packs into the spec shape (original/full.ogg -> stems/full.ogg, add the `full` stem at default:off, drop the key); the fallback and the aliases die with #945. The spec gate keeps the debt honest: the grandfather entry now tracks #945, and the gate fails if it goes stale. Verified: spec gate OK (4/4, incl. ingesting the spec's new example pack that retains `full`); 2493 python tests, 995 js tests; migrator round-tripped over real packs from the library and the results pass the spec's reference validator. * fix(migrate): discover directory-form packs instead of silently skipping them iter_packs() searched only files, so a directory-form pack (`song.sloppak/`, the authoring shape) was walked INTO and never yielded — silently missed by a run that's meant to be exhaustive. Discover suffix-named directories too (yielded whole, not descended into), and route packs through migrate_pack/verify_pack. Directory packs are REPORTED as `dir-form-unsupported`, not rewritten in place: a single-file pack is replaced atomically (a fully-built temp archive swapped in with one os.replace), but a populated directory can't be swapped that way, so an interrupted in-place rewrite could leave an authoring pack half-migrated. The status is a problem status, so it counts against the run's exit code and shows in the summary — the operator re-packs or migrates it as a `.feedpak` instead of it vanishing from the report. Addresses a CodeRabbit review finding. Signed-off-by: Kris Anderson <topkoa@gmail.com> * fix(migrate): verify requires an explicit `off` on a retained full mix verify_zip accepted any non-truthy `default` on a multi-stem `full` (missing, empty, boolean, `false`/`no`/`0`, malformed) as "ok". But core defaults an ABSENT `default` to True — ON (lib/sloppak.py: `s.get("default", True)`) — and treats an empty/unrecognized string as ON too, so a migrated-shape pack whose `full` stem has a missing or blank default beside instrument stems would actually play the mixdown on open and double the song. verify was certifying that as safe. Require an explicit normalized `off` beside instrument stems: `on`-ish values are reported `full-stem-default-on` (actively plays), everything that is not a normalized `off` is reported `full-stem-default-not-off`. The migrator already writes the literal `off`, so its own output is unaffected; this also certifies the pack is in the tool's canonical, most-portable shape. The len>1 gate is kept, so a sole `full` stem (which IS the audio) is not policed. Adds parametrized coverage for missing / empty / boolean / off-ish / malformed defaults, and a sole-full-stem case. Addresses a CodeRabbit review finding. Signed-off-by: Kris Anderson <topkoa@gmail.com> --------- Signed-off-by: Kris Anderson <topkoa@gmail.com> Co-authored-by: Kris Anderson <topkoa@gmail.com> |
||
|
Byron Gamatos
|
d876ded00f
|
fix(sloppak): bound the unpack cache; add read_member_bytes() so callers stop unpacking whole songs (#950)
* fix(sloppak): bound the unpack cache, and add a way to read a song without unpacking it A tester's sloppak_cache reached 60 GB from an 1800-song library — his entire library, unpacked, none of it played. Stems are already-compressed audio, so an unpacked pack is ~1.1x its zip: the cache is a second, DECOMPRESSED copy of every song it touches. It had no size cap, no LRU, and no cleanup of any kind — not even when the song itself was deleted. Two halves: 1. resolve_source_dir() now evicts least-recently-used songs to stay under a cap (FEEDBACK_SLOPPAK_CACHE_MAX_MB, default 4 GB ≈ 130 songs of recency; 0 disables). The sweep runs on unpack — the only moment the cache grows — so it can't drift. An evicted song is dropped from _source_cache too: get_cached_source_dir() is the only thing media.py consults before falling back, so a stale path there would 404 every stem for the rest of the process instead of re-unpacking. get_cached_source_dir() now also verifies the dir still exists, which makes "just delete sloppak_cache/ to reclaim disk" safe advice. 2. read_member_bytes() reads ONE file out of a pack without unpacking it — the same trick read_cover_bytes() uses so the library grid doesn't explode every pack to show a cover. Unpacking a whole song to read a few KB of JSON is ~45x write amplification; doing it in a loop over the library is what produced the 60 GB. rig_builder's library-wide tone batch is the caller that did exactly that (fixed separately); this gives it, and everyone else, the right primitive. Eviction is concurrency-safe: unpacks run 2-at-a-time, so a dir being written is marked in-flight and the sweep skips it — checked and rmtree'd under one hold of the guard, and the marker is released even if the unpack raises (a leaked marker would make that dir permanently un-evictable). read_member_bytes normalizes both the requested path AND the archive's stored member names through safe_join, taking the last match — so './arrangements/x.json', backslash members from Windows tooling, and duplicate members that normalize to the same path all read back exactly as unpack-then-read did. Zip-slip is rejected before anything is opened. Tests: tests/test_sloppak_unpack_cache.py. All bite-tested (reverted each fix, watched it fail) — including one that was passing vacuously: a freshly-unpacked dir is the most-recently-used, so the LRU never reaches it and the in-flight race test proved nothing until the packs were sized to force the sweep that far. * test: split semicolon-joined statements (E702) CodeRabbit on #950. Style only; no behaviour change. |
||
|
Byron Gamatos
|
18d77d2d41
|
feat(library): effective genre falls back to MusicBrainz enrichment (#949)
* feat(library): effective genre falls back to MusicBrainz enrichment Converted packs rarely carry a genres manifest key — on Byron's real library 1188/1190 songs had no genre, starving the genre facet and the career passport rack (2 usable genres) while song_enrichment already held MB genres for 636 matched songs. The effective-genre expression now resolves: per-song override → pack genre → json_extract(enrichment.genres, '$[0]') for MATCHED rows only (review/failed candidates could carry the wrong recording's genres). Same fast-path gating as before: the plain indexed column is used unless overrides or enrichment genres actually exist; stand-in DBs without the table degrade via the OperationalError guard. Career passports pick this up automatically through _effective_genre_expr(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: changelog names manual rows in the enrichment fallback Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Byron Gamatos
|
3e57ba0345
|
fix(career): hours polish — recency stamp + non-2xx POST is a failure (#947)
CodeRabbit follow-up on #942: - add_play_seconds() now stamps last_played_at (like touch_position): an unscored play that ran to the natural end WAS played — recent / Continue ordering must see it. Resume position stays untouched. - stats-recorder post() treats non-2xx as failure: a 4xx/5xx JSON error body parsed as an object read as success, silently dropping the accrued seconds instead of re-queuing them. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Byron Gamatos
|
0fc6a4beed
|
feat(career): hours-per-genre odometer — honest wall-clock play time (#942)
Career v2, WS2. Nothing measured play time before (the achievements plugin's final-position shortcut double-counts loops and mis-reads seeks). Now: - stats-recorder.js accrues WALL-CLOCK seconds across song:play/resume ↔ pause/stop/ended spans (single spans clamp at 2h against suspend inflation) and piggybacks them as `seconds` on the POSTs it already sends; failed POSTs restore the accumulator; a session reset flushes first so time can't re-attribute to the next song/arrangement. - POST /api/stats accepts optional `seconds` (finite, 0 < s ≤ 6h) on the scored and position branches, plus a new seconds-only branch for unscored plays that ran to the natural end — banks time WITHOUT touching the resume position (song:ended must not overwrite Continue) and still counts as playing today for the streak. - song_stats gains additive idempotent `seconds_total`; record_session/ touch_position accrue, new add_play_seconds() for the seconds-only path; the legacy-encoding stats merge sums seconds across duplicates. - Passports surface it: "14.2 h in Blues" under the badge stamp and on the shelf cover sub-line — a true fact that only grows, never a target or a meter (Stage 5 post-cap, per the career design). Tests: seconds accrual/validation/seconds-only branch (stats API), per-instrument-and-genre summing (career), fmtHours formatting (vm). Full suites: pytest 2480, JS 1165. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
ChrisBeWithYou
|
ffc52f13ce |
Harden freqs_to_midis against NaN/Inf; badges read exact tuningMidis
Follow-ups to #829 (CodeRabbit's review nit + the consumer adoption the PR body promised): - freqs_to_midis: reject non-finite frequencies (NaN/Infinity) — a provider handing one through would otherwise raise inside int(round(...)) and 500 GET /api/tunings. Tests cover nan/inf/-inf alongside the existing garbage cases. - v3 instrument badge: TUNING_NOTE now prefers the exact integer midis the server serves (tuningMidis) over reconstructing the note from the lowest string's frequency via log2 against a hardcoded 440 — which can land a semitone off at non-440 reference pitches. Frequency path kept as the fallback for older cached responses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MS2YFb6UUSwJVV6CmEa25i Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com> |
||
|
Byron Gamatos
|
0a6e0309e5
|
fix(tailwind): stop the dev server rewriting a tracked file (#911) (#918)
The runtime stylesheet moves to CONFIG_DIR. static/tailwind.min.css is never written again.
━━━ TWO DIFFERENT THINGS WERE SHARING ONE PATH ━━━
static/tailwind.min.css a BUILD ARTEFACT. Committed, image-baked, generated by scanning
the in-tree plugins only. CI's tailwind-fresh check verifies it.
the RUNTIME sheet PER-INSTALL STATE. Additionally scans whatever the user installed
into FEEDBACK_PLUGINS_DIR, so it differs machine to machine.
Writing the second over the first meant that MERELY RUNNING THE DEV SERVER from a git checkout
silently modified a tracked file. `git add -A` then swept a 100KB reshuffle of minified CSS
into the commit and ci/tailwind-fresh went red with a diff that explains nothing — on a PR
whose real change touched no Tailwind classes at all. It also wrote app state into the app
directory, which is read-only in some deploys.
A new route serves the runtime sheet when there is one and falls back to the committed one
otherwise. It is registered BEFORE the /static mount, which would otherwise swallow the path.
━━━ A PERSISTED SHEET MUST NOT OUTLIVE ITS REASON (Codex [P2] x2) ━━━
1. THE USER REMOVES THEIR PLUGINS. Startup only rebuilds when user plugins exist, so nothing
would ever overwrite the stale sheet — and it still carries classes for plugins that are
gone. With no user plugins the COMMITTED sheet is complete by definition. Guarded.
2. THE APP IS UPGRADED, and my first guard for this was WRONG. I compared mtimes. Codex: that
is not a freshness signal across install methods — archives and container images routinely
PRESERVE SOURCE MTIMES, so a just-shipped stylesheet can carry an OLDER timestamp than a
runtime sheet a user built days ago. The mtime check then calls the stale one FRESH and it
masks the new core CSS indefinitely — permanently, if no Tailwind toolchain is present to
trigger a rebuild.
Freshness is decided by CONTENT now. Each runtime build stamps a sidecar with the sha256 of
the committed sheet it was made from. Core ships new CSS -> that file changes -> the hash
changes -> the runtime sheet is correctly judged stale. Timestamps only gesture at the
question that hashing answers.
Falling back to the committed sheet is always safe: at worst it lacks a just-installed plugin's
classes for the seconds until the async rebuild lands.
VERIFIED END TO END. Ran the real dev server with 3 plugins installed: it rebuilt Tailwind over
them (123,291 bytes), wrote the sheet + sidecar to CONFIG_DIR, still served /static/
tailwind.min.css at 200 — and `git diff` on the tracked file came back CLEAN.
8 tests. Bite-tested: reverting to the shared path fails 3, dropping the staleness guards fails
2 more.
pytest 2425, pyflakes 0, Codex 0.
Closes #911
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
79825af28e
|
fix(demo): the janitor re-entry guard actually works now (#902) (#909)
The guard in startup_events() read:
if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
and not _DEMO_JANITOR_STARTED:
`and` binds tighter than `or`, so that is `A or (B and C)`. The not-already-started half
never ran when the env var was truthy — the only case that reaches it at all. A second
startup started a SECOND janitor thread, overwrote the handle, and shutdown then joined
only the last: the first leaked and kept firing registered hooks hourly, forever.
The guard now lives INSIDE start_janitor(). A caller cannot get operator precedence wrong
if there is nothing left for it to get wrong.
━━━ THREE WAYS TO WRITE THIS GUARD WRONG. I HIT ALL THREE. ━━━
1. NO GUARD — the original bug. Double-start, orphaned thread.
2. GUARD ON THE FLAG (`if _DEMO_JANITOR_STARTED: return`). Codex [P2]. stop_janitor()
DELIBERATELY leaves that flag True when a hook outruns its join timeout, so that a later
startup cannot spawn a janitor beside a live one. But the hook usually finishes a moment
later: the thread exits and the flag is stale. A flag-keyed guard then refuses to start a
replacement for the rest of the process — demo cleanup silently dead. (The original bug
accidentally MASKED this by always starting.)
3. GUARD ON LIVENESS ALONE (`if thread.is_alive(): return`). Codex [P2], second pass. A
timed-out stop leaves the old thread ALIVE BUT DOOMED — its stop event is set and it
exits as soon as its current hook returns. Treating that as a running janitor skips the
replacement, and we are back at (2) a second later.
So: a janitor counts as running only if its thread is alive AND it has not been told to stop.
━━━ AND EACH JANITOR NOW OWNS ITS STOP EVENT ━━━
start_janitor() used to `_DEMO_JANITOR_STOP.clear()` a single SHARED Event. Start a
replacement while a doomed thread is still finishing a hook and that clear RESURRECTS it: it
loops back to wait(), sees the flag cleared, and carries on. Two janitors — the exact bug we
started from. A fresh Event per janitor makes it impossible; the old thread waits on its own
event, which stays set, so it can only exit.
Env semantics UNCHANGED, verified across every value ("", "1", "0", "true", "false", "off"):
the old expression and demo_mode_enabled() agree on all of them. The only behavioural change
is the idempotency fix.
FOUR tests, and each of the three wrong guards fails a different subset:
no guard -> 2 fail (double start; orphaned thread)
guard on the flag -> 2 fail (never restarts after a timed-out stop)
liveness alone -> 1 fail (no replacement for a doomed janitor)
liveness + not-stopping -> all pass
pytest 2416, pyflakes 0, Codex 0.
Closes #902
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
f5d448af5c
|
refactor(server): carve demo mode into lib/demo_mode.py (R3b) (#903)
lib/demo_mode.py (342). server.py 1,870 -> 1,649.
The read-only request guard (its 96-entry blocked-route table + the middleware) and the
hourly session janitor (registry, hook runner, thread). Bodies VERBATIM.
THE MIDDLEWARE NEEDS `app`, SO THE MODULE TAKES IT. _demo_mode_guard is an
@app.middleware("http") and cannot exist without an app object. Rather than have a module
under lib/ reach for a global, it exposes install(app) and server.py — which owns the app —
hands it over. The janitor is symmetrical: start_janitor() / stop_janitor(), called from
server.py's startup and shutdown hooks, where the process lifecycle actually lives.
register_demo_janitor_hook IS PART OF THE PLUGIN CONTRACT. It is a key in plugin_context,
so plugins hold it as a LIVE REFERENCE from setup(). server.py imports this exact object
and puts it in the dict unchanged — identity preserved, and
tests/test_plugin_context_contract.py (#898, merged) fails if that ever stops being true.
This is the first carve that guard has actually protected.
━━━ stop_janitor()'s ORDER IS LOAD-BEARING ━━━
The obvious way to write it — clear the "started" flag, then join — is WRONG, and I wrote
it that way first. server.py's original deliberately returns EARLY, leaving
_DEMO_JANITOR_STARTED True and the thread handle intact, when the thread outlives the join:
# Leave _DEMO_JANITOR_STARTED True so a new janitor is not
# spawned by a subsequent startup while the old one is alive.
Clearing the flag first quietly reintroduces exactly the double-janitor leak the flag
exists to prevent. Preserved byte-for-byte, and the reason is now written down at the
function rather than only at its single call site.
━━━ A BUG MOVED VERBATIM, ON PURPOSE ━━━
if getenv_compat("FEEDBACK_DEMO_MODE") or getenv_compat("FEEDBACK_DEMO_MODE") == "1" \
and not _DEMO_JANITOR_STARTED:
`and` binds tighter than `or`, so this is `A or (B and C)` — the not-already-started
re-entry guard is DEAD whenever the env var is truthy, which is the only case that runs.
A second startup leaks a janitor thread (the handle is overwritten, so shutdown joins only
the last). Verified. Preserved exactly and filed as issue #902: a carve whose whole value
is being provably behaviour-neutral is not the place to change behaviour.
pyflakes caught three more missing imports on the way in (uuid, warnings x2). Five carves,
ten missing imports, every one a NameError on a live path.
pytest 2399, pyflakes 0, Codex 0.
Refs #48
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
8f014e6a30
|
refactor(server): carve the library scanner into lib/scan.py (R3b) (#901)
lib/scan.py (326). server.py 2,098 -> 1,870.
The background scan, its spawn ProcessPoolExecutor, and the kick/runner plumbing that
serialises passes. Bodies VERBATIM except the seam reads.
Everything shared is read LATE off appstate — the same contract every module in
lib/routers/ uses, and it is not cosmetic: tests monkeypatch CONFIG_DIR and swap meta_db,
so a value captured at import time pins the wrong one for the life of the process.
CONFIG_DIR -> appstate.config_dir
meta_db -> appstate.meta_db
_default_settings -> appstate.default_settings()
_stat_for_cache -> appstate.stat_for_cache()
━━━ THE SCAN STATUS IS REBOUND, NOT MUTATED ━━━
_background_scan does `global _scan_status; _scan_status = {**INIT, ...}` at every stage
transition. It REPLACES the dict; it never updates it in place. So nothing may hold that
dict by value — a reference captured once goes permanently stale at the first stage change
and would report "listing" forever while the scan ran to completion.
Hence `scan.status()`, a getter, and hence appstate publishes scan_status as a CALLABLE.
appstate.py already said so in a comment; this is the code that makes it true. (Same for
the plugin_context entry, which was already `lambda: dict(_scan_status)` — late-bound, so
it survives the move unchanged. The contract test from #898 covers it.)
━━━ appstate.server_root: A TRAP CLOSED PERMANENTLY ━━━
_background_scan seeds the builtin content, which needs the directory holding server.py.
`Path(__file__).resolve().parent` is correct in server.py and silently WRONG anywhere under
lib/ — it yields lib/, which holds no docs/ or data/ — and it fails by finding NOTHING
rather than by raising, so the seeds would just quietly never run.
lib/builtin_content.py (#900) closed that by taking the root as a parameter. This adds the
other half: server.py publishes it ONCE as appstate.server_root, so no module under lib/
ever has a reason to derive it. Documented at the slot.
pyflakes caught two more missing imports on the way in (loosefolder_mod, enrichment) —
each a NameError on a live scan path, and the suite would have handed them over one failure
at a time. It stays part of every server.py slice.
TESTS. The two scan fixtures (test_settings_api::scan_module,
test_feedpak_extension::scan_server) patched server._make_scan_executor to swap the spawn
pool for an in-process ThreadPool; they now patch it on lib/scan.py. Worth noting WHY that
still works: the fixtures re-import `server` per test, but `scan` stays cached in
sys.modules — and it picks up the fresh CONFIG_DIR anyway, because the appstate reads are
late-bound. The seam is doing exactly the job it was built for.
━━━ TEST ISOLATION: A REGRESSION THE CARVE ITSELF CREATED (Codex [P2]) ━━━
background_scan() deliberately NEVER sets running=False — ownership of that flag lives in
_scan_runner, so a kick_scan() racing the terminal write cannot observe a stale False and
start a second runner. Correct in production.
But the scan fixtures call background_scan() DIRECTLY, skipping the runner. That was
harmless while the state lived on `server`, which the fixtures RE-IMPORT per test. It is
NOT harmless now: `scan` stays cached in sys.modules across sys.modules.pop("server"), so
the status dict OUTLIVES the test. One direct call leaves the shared scanner marked
"running" forever, and every later scan or rescan returns "already in progress" and quietly
does nothing.
Verified: after a direct call, kick_scan() returns False and starts no scan at all.
The suite passed anyway, on ordering luck — which is exactly how this class of bug ships.
tests/conftest.py::reset_scan_state now snapshots and restores lib/scan.py's module state
around the two fixtures that drive it directly.
pytest 2398, pyflakes 0, Codex 0.
Refs #48
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
70dbe45e27
|
refactor(server): carve builtin-content seeding into lib/builtin_content.py (R3b) (#900)
lib/builtin_content.py (321 lines moved). server.py 2,418 -> 2,098.
The calibration/diagnostic sloppaks and the starter library: _copy_builtin_packs,
_write_builtin_pack, the two seed helpers, their source tables, and the seed marker.
━━━ THE ONE SIGNATURE CHANGE, AND WHY THE CARVE IS UNSAFE WITHOUT IT ━━━
server.py has:
def _feedBack_server_root() -> Path:
return Path(__file__).resolve().parent
That is correct IN server.py: the repo root in dev, resources/feedBack when bundled — the
tree that actually holds docs/ and data/.
Move that body into lib/ unchanged and it keeps working, silently, and returns lib/. There
is no docs/diagnostics under lib/, so every seed would find nothing, log "source missing"
at debug, and return. Nothing raises. Nothing fails. The starter library simply never
appears, and the calibration sloppak is never seeded — on a fresh install, in the field.
A verbatim move whose MEANING changed because __file__ did.
So this module cannot compute a root: `server_root` is a PARAMETER, and server.py — the
only place that legitimately knows where it lives — passes it in. The trap is now
structurally impossible rather than merely avoided. (_copy_builtin_packs already took the
root that way; the two seed helpers now do too.)
Everything else is byte-identical. CONFIG_DIR is read late as appstate.config_dir and the
DLC root through dlc_paths._get_dlc_dir — the same seam every router in lib/routers/ uses,
late-bound because tests monkeypatch it.
━━━ PYFLAKES FOUND THREE MISSING IMPORTS THE TESTS WOULD HAVE FOUND ONE AT A TIME ━━━
The moved code uses `secrets`, `stat` and `tempfile`; none was in my import block. Each is
a NameError on a live path. `python3 -m pyflakes` names all three in one shot — this is the
Python twin of the no-undef gate that guarded every frontend carve, and it should run on
every server.py slice from here.
It also flagged a PRE-EXISTING one I deliberately did not touch: server.py's
TuningProviderRegistry.get_merged() calls `logger.exception(...)` in an except handler and
there is no `logger` in the module (it is `log`). So a raising tuning provider takes down
the merged-tunings call for everyone, with a NameError naming the wrong problem. Filed as
issue #899 rather than smuggled into a carve whose whole value is being behaviour-neutral.
The constants lost their underscore prefix: they cross a module boundary now (the seed
tests read them), so `_BUILTIN_STARTER_SOURCES` was a lie.
pytest 2397, pyflakes 0, Codex 0. Guarded by the plugin_context contract test (#898).
Refs #48
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
5e30138c87
|
refactor(server): extract the artist routes into routers/artist.py (R3) (#870)
The artist page + external-links payload (/api/artist/{name}/page, /links,
/links/refresh) plus their exclusive helpers (_artist_links_payload,
_artist_links_from_mb, the URL-slot table) move to lib/routers/artist.py. Bodies
verbatim except @app->@router and the seam reads (meta_db->appstate.meta_db,
CONFIG_DIR->appstate.config_dir, _default_settings->appstate.default_settings).
MB link enrichment is reached as enrichment.X; the URL-safety validator is
imported from lib/library_registry.py. No new seams.
server.py: 2,507 -> 2,413 (-94).
Verified: pyflakes clean; route set unchanged (143); full pytest 2395 passed.
eslint 0.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
0547f55844
|
refactor(server): extract the media/file-serving routes into routers/media.py (R3) (#869)
Song audio (/audio/{f}), the local-audio-path resolver (/api/audio-local-path),
and raw sloppak-member serving (/api/sloppak/{f}/file/{rel}) — plus the shared
_resolve_sloppak_local_file helper — move to lib/routers/media.py. Bodies verbatim
except @app->@router and the cache/static path seams (AUDIO_CACHE_DIR->
appstate.audio_cache_dir, STATIC_DIR->appstate.static_dir, SLOPPAK_CACHE_DIR->
appstate.sloppak_cache_dir — all already in the seam). No new slots.
The two test fixtures that redirect STATIC_DIR to a temp dir now also patch
appstate.static_dir (the moved routes read the seam, not server's global).
server.py: 2,638 -> 2,507 (-131).
Verified: pyflakes clean; route set identical (143), all unique-path (no
catch-all, no shadowing); full pytest 2395 passed (the audio-local-path +
sloppak-file-traversal cases). eslint 0. Boot smoke: /audio, /api/sloppak/{}/file
serve 404 for unknown, 0 tracebacks.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
b7624b7e65
|
refactor(server): extract the enrichment route handlers into routers/enrichment.py (R3) (#868)
The 14 /api/enrichment/* routes — status, kick/cancel, per-song state, the Match-Review queue (accept/reject/pick/search/rematch/refresh/states), and the AcoustID fingerprint identify endpoints — plus the route-exclusive candidate sanitizer move to lib/routers/enrichment.py. Bodies verbatim except @app->@router and the seam reads (meta_db->appstate.meta_db, CONFIG_DIR->appstate.config_dir). The enrichment engine (transport, matcher, worker, upload caps) already lives in lib/enrichment.py from the earlier subsystem move and is reached as enrichment.X. No new seams. server.py: 2,925 -> 2,638 (-287). Verified: pyflakes clean; route set identical (143); full pytest 2396 passed (the enrichment route + Match-Review + identify cases, which fake the network on the enrichment module). eslint 0. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
f09c4a217f
|
refactor(server): extract library + collections routes + the provider registry (R3) (#866)
The library query surface (songs/albums/artists/stats/genres/tuning-names/ practice-suggestions), the provider list/art/sync endpoints, and collection CRUD move to lib/routers/library.py. The registry itself — LibraryProviderRegistry, LocalLibraryProvider, SmartCollectionProvider, the collection-provider lifecycle (_sync/_unregister_collection_provider), and the shared query/collection helpers (_library_filter_args, _split_csv, _sanitize_collection_rules, _safe_art_redirect_url, the filter-key sets) — moves to lib/library_registry.py. The PLUGIN CONTRACT is untouched: server.py still constructs the singleton (LocalLibraryProvider needs meta_db), still exposes register_library_provider / unregister_library_provider to plugins via plugin_context (with the per-plugin ownership scoping in plugins/__init__.py), and injects the registry + local provider into appstate. The router reads appstate.library_providers / appstate.local_library_provider at call time; the provider classes are duck-typed so no plugin imports a base class. Acyclic: library_registry imports routers.art (for LocalLibraryProvider.get_art) + appstate, never server. server.py: 3,692 -> 2,925 (-767). Verified: pyflakes clean; ORDERED route table identical set (library block mounts at one site — all exact/specific-path, no catch-all, no shadowing); full pytest 2397 passed (incl the plugin register/unregister + collection-as-provider tests). eslint 0. Boot smoke: /api/library + providers list "local"; a created collection surfaces as a `collection:N` provider through the seam; collection CRUD clean. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
bbdff4e10f
|
refactor(server): extract the song routes into routers/song.py (R3) (#864)
Upload/delete, the catalog-metadata write-back, user-meta, overrides, gap-fill,
and the per-song info payload (11 routes) move to lib/routers/song.py with their
exclusive helpers (the atomic upload commit + the song-IO lock, the upload caps,
the gap-fill proposal builders). Bodies verbatim except @app->@router and the
seam reads: meta_db->appstate.meta_db, art_override_paths->appstate.art_override_paths,
and the scan/ingest helpers that stay in server.py (the scan lifecycle owns them)
-> new appstate seam callables: kick_scan, invalidate_song_caches, stat_for_cache,
and scan_status() (a getter — the underlying dict is reassigned). The gap-fill
MBID/ISRC regexes are reached as enrichment.X; _MULTIPART_OVERHEAD_SLACK (shared
with the staying AcoustID-identify route) moves to lib/enrichment.py beside
_ACOUSTID_MAX_UPLOAD_BYTES.
ROUTE ORDER: song_router mounts AFTER art_router — get_song_info's catch-all
`/api/song/{filename:path}` would otherwise shadow `/api/song/{path}/art*`
(Starlette matches first-registered; the :path converter is greedy).
server.py: 4,478 -> 3,692 (-786).
Verified: pyflakes clean; ORDERED route table preserves the specific-before-catch-all
invariant; full pytest 2397 passed (incl the art/cover 304 + CAA-fetch tests that
caught the shadowing before it was fixed). eslint 0.
BEHAVIORAL — needs an on-device pass (upload a sloppak, edit metadata write-back,
delete a song) before merge.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
7258e1066a
|
refactor(server): extract the settings routes into routers/settings.py (R3) (#863)
GET/POST /api/settings, /api/settings/reset, and the two-phase atomic export/import bundle (/api/settings/export|import) move to lib/routers/settings.py with their exclusive helpers (the relpath allowlist validator, the atomic writer, the library-DB snapshot + sqlite integrity gate, the config-type validator, the bundle schema). Bodies verbatim except @app->@router and the seam reads: meta_db->appstate.meta_db, CONFIG_DIR->appstate.config_dir, _running_version->appstate.running_version(), and _default_settings-> appstate.default_settings (the canonical defaults builder stays in server.py — the scan + artist-links code share it — and is injected as a new seam callable). server.py: 5,539 -> 4,478 (-1,061). Verified: pyflakes clean (bar the pre-existing File/safe_join/tuning_name/ET); route table IDENTICAL (143); full pytest 2397 passed (154 settings cases incl the export→import round-trip + library-DB snapshot/restore + relpath-allowlist SSRF/ traversal guards, retargeted onto the settings module). eslint 0. BEHAVIORAL — needs an on-device settings export→import round-trip sign-off before merge (do not merge on green CI alone). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
73127d5416
|
refactor(server): extract the album-art routes into routers/art.py (R3) (#862)
The six song-art routes — GET /api/song/{f}/art, .../art/cover-search,
.../art/candidates, POST .../art/upload, .../art/url, DELETE /api/art/{f}/override
— plus their exclusive helpers (the ETag/304 response machinery, _save_art_override,
_url_host_is_internal, _fetch_art_url + the art size/redirect caps) move to
lib/routers/art.py. Bodies verbatim except @app->@router and the seam reads:
meta_db->appstate.meta_db, ART_CACHE_DIR->appstate.art_cache_dir, and the three
shared art helpers that stay in server.py (used by the song/delete routes too)
-> appstate.<callable> (_song_pack_art_exists, _art_override_paths — already
seam-injected for the enrichment worker — plus a new art_safe_name slot). The
CAA / release-search transport lives in lib/enrichment.py and is reached as
enrichment.X. LocalLibraryProvider.get_art now calls art_router.get_song_art.
server.py: 5,988 -> 5,540 (-448).
Verified: pyflakes clean (bar the pre-existing File/safe_join/tuning_name/ET);
route table IDENTICAL (143); full pytest 2399 passed (33 art serve/candidates/
override/url cases incl the SSRF-guard _url_host_is_internal + _fetch_art_url
size-cap tests, retargeted onto the art module); test_packaging 43; eslint 0.
Boot smoke: /art 404, /art/candidates 404, DELETE /override 200 from the router.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
165475d115
|
refactor(server): move the metadata-enrichment subsystem into lib/enrichment.py (R3) (#861)
* refactor(server): move the metadata-enrichment subsystem into lib/enrichment.py (R3)
MusicBrainz / Cover-Art-Archive / AcoustID transport, the match-scorer glue, and
the background enrichment worker (~930 lines, 61 defs) leave server.py as one
cohesive unit. Bodies are verbatim; the only changes are seam reads:
meta_db / config_dir / sloppak_cache_dir / art_cache_dir -> appstate.<slot>
_song_pack_art_exists / _art_override_paths (stay in server.py for the art +
delete routes) -> appstate.<callable> (new seam slots, injected by reference)
_env_flag -> env_compat.env_flag_compat (the existing identical helper)
_artist_title_from_filename -> imported from metadata_db (its home)
the User-Agent VERSION lookup: Path(__file__).parent ->
Path(__file__).resolve().parents[1] (lib/enrichment.py -> app root)
server.py drives the worker through the module (import enrichment; the routes +
scan lifecycle call enrichment.X). Tests that faked the network on `server`
(_mb_http_get, _enrich_network_enabled, _caa_http_get, ...) now patch the same
names on `enrichment` — the module attribute is resolved at call time, so one
setattr reaches both the routes and the worker's internal callers. Acyclic:
enrichment imports appstate/appconfig/dlc_paths/metadata_db/mb_match/
acoustid_match/sloppak/loosefolder, never server.
server.py: 6,917 -> 5,988 (-929).
Verified: pyflakes clean (bar the pre-existing File/safe_join/tuning_name/ET);
route table IDENTICAL (143); full pytest 2400 passed (140 enrichment/art cases
incl the offline-safety + transport-error-pauses-pass contracts that fake the
network); test_packaging 44 passed (enrichment.py resolves under lib/); eslint 0.
Boot smoke: /api/enrichment/status + POST /kick serve from the new module.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: reset enrichment worker state between tests (CodeRabbit)
lib/enrichment.py now owns the worker, and it stays imported for the whole
session while the `server` fixtures pop-and-reimport `server` — so the cancel
Event / status dict / caches would leak across tests, and a stale `_enrich_cancel`
could short-circuit a later direct `_background_enrich()`. An autouse conftest
fixture clears that process-global state before each test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: tighten the enrichment-reset fixture (CodeRabbit)
Narrow the import guard to ImportError (not blind Exception, BLE001), and stop
clearing _caa_index_locks — it's guarded by _caa_index_locks_guard, so an
unlocked clear() would race a still-alive worker, and its per-release mutexes
carry no test state.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
508829c012
|
refactor(server): extract /api/tunings into routers/tunings.py + lib/appconfig.py (R3) (#858)
Some checks are pending
ship-ci / ci (push) Waiting to run
The merged-tuning-catalog route moves to lib/routers/tunings.py, verbatim except
@app->@router, CONFIG_DIR->appstate.config_dir, and the two seam substrates it
needed:
- lib/appconfig.py — the pure config.json reader `_load_config` (used by ~11
server sites + future config-reading routers). server.py re-imports it, so
those call sites and any `server._load_config` test reference are unchanged.
- appstate.tuning_providers — the TuningProviderRegistry instance injected by
reference (a stable object mutated in place via register()/unregister()), so
the router reads the same registry plugins populate through plugin_context.
The instance stays defined in server.py, so `server.tuning_providers` still
resolves — zero test retargets.
The tuning constants (DEFAULT_REFERENCE_PITCH, TUNING_PRESET_MIDIS,
freqs_to_midis) already live in lib/tunings.py and are imported directly.
server.py: 6,960 -> 6,917.
Verified: pyflakes clean (bar the pre-existing unused `tuning_name` import);
route table IDENTICAL (143); full pytest 2400 passed (110 tuning/config cases);
eslint 0. Boot smoke: /api/tunings serves referencePitch + tunings + tuningMidis.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
cce95cbd1e
|
refactor(server): extract the diagnostics routes into routers/diagnostics.py (R3) (#857)
The three /api/diagnostics/* routes (export, preview, hardware) plus their exclusive payload-cap helpers and the `_diag_*` normalisers. Bodies verbatim except @app->@router, CONFIG_DIR->appstate.config_dir, _running_version()-> appstate.running_version() (a new seam slot; the impl stays in server.py where the settings region also calls it), and the builtin-plugins lookup in _diag_plugins_roots: Path(__file__).parent -> Path(__file__).resolve().parents[2] (routers -> lib -> app root; plugins/ ships at the app root in every packaging path). The pure caps/normalisers (_diag_cap_console/_dict/_contributions, _diag_coerce_bool, _diag_normalize_include, _DIAG_MAX_*) are re-exported from server.py so the existing `server._diag_*` / `server._DIAG_*` tests keep resolving — none of them monkeypatch these, so no test retargets. server.py: 7,216 -> 6,960. Verified: pyflakes clean (bar the intentional re-export lines); route table IDENTICAL (143); full pytest 2399 passed (77 diag/packaging + 122 diagnostic- matched cases incl the cap/coerce/normalize suites); eslint 0; Codex pending. Boot smoke: /hardware, /preview, and POST /export (200 application/zip) all serve from the new router location. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
32ebc7671e
|
refactor(server): extract the version route into routers/version.py (R3) (#856)
Some checks are pending
ship-ci / ci (push) Waiting to run
GET /api/version + its exclusive _safe_http_url URL validator. Bodies verbatim except @app -> @router and the VERSION-file lookup: Path(__file__).parent (the app root when this lived at the top level) -> Path(__file__).resolve().parents[2] (routers -> lib -> app root). VERSION ships at the app root in every packaging path (Dockerfile COPY VERSION /app/, desktop bundle). server.py: 7,275 -> 7,214. Verified: pyflakes clean; route table IDENTICAL (143); pytest 2401 passed (33 in test_version_endpoint, incl the URL-validation + env-override cases); eslint 0. Boot smoke: /api/version returns the real version + validated source/license URLs. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
46f3be7fd7
|
refactor(server): extract XP + per-song stats into routers/stats.py (R3) (#855)
The last of the progression cluster. XP award (1 route) + per-song practice stats
(record/recent/best/top/per-song, 5 routes) — meta_db-only apart from the two
seam accessors record_stats uses (get_progression_content, builtin_diagnostic_
filename, both landed by shop/progression). Bodies verbatim; @app -> @router,
meta_db -> appstate.meta_db, _as_int from metadata_db, _clean_str from reqfields.
Three scattered source blocks (xp, the stats block, and the separated
/api/stats/{filename:path} which the /api/library/practice-suggestions route
splits off) are assembled into one module and mounted once. Registration order
is preserved WHERE IT MATTERS: the /api/stats/{filename:path} catch-all is
assembled LAST inside the router, so it still can't shadow the fixed /recent
/best /top paths — verified against the live route table (recent/best/top all
precede the catch-all) and the route SET is identical to origin/main (143).
server.py: 7,478 -> 7,275.
Verified: pyflakes clean; route set identical + catch-all-last; pytest 2401
passed (113 across song_stats/profile/progression); eslint 0. Boot smoke:
/stats/recent /best /top all 200 (not shadowed), xp/award 200.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
4cc8fa3b4d
|
refactor(server): extract the profile routes into routers/profile.py (R3) (#854)
6 routes (get/set profile, bundled+custom avatars, avatar upload/serve, progress) + the exclusive _list_bundled_avatars helper. Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db, CONFIG_DIR/STATIC_DIR -> appstate.config_dir/ static_dir (seam), _clean_str from reqfields, _get_progression_content() -> appstate.get_progression_content(). No STATIC_DIR test retarget: _list_bundled_avatars reads appstate.static_dir but test_profile_api doesn't patch STATIC (only the sloppak/audio/traversal suites do, for handlers that stay in server.py). server.py: 7,594 -> 7,478. Verified: pyflakes clean; route table IDENTICAL (143); pytest 2401 passed (61 in test_profile_api); eslint 0. Boot smoke: GET /api/profile 200 (drives get_progression_content), /avatars lists via appstate.static_dir, /progress 200. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
f9f33320ac
|
refactor(server): extract the progression routes into routers/progression.py (R3) (#853)
4 routes (overview/add-paths/onboarding/events, spec 010) + their EXCLUSIVE helpers (_goal_ui_progress, _progression_overview 112L) + the _PROGRESSION_EVENT_TYPES whitelist. Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db, _clean_str from reqfields. The two SHARED server accessors read through the seam: get_progression_content (added for shop #851) and builtin_diagnostic_filename (new slot — a trivial const-returning fn shared with the stats router's api_record_stats). Both are injected via the second appstate.configure() after their defs (the import-top configure runs before them). The cache + fns stay in server.py, so test_progression_api's server._progression_content patch is untouched — 0 retarget. server.py: 7,798 -> 7,594. Verified: pyflakes clean; route table IDENTICAL (143); both seam accessors wired; pytest 2401 passed (63 in test_progression_api); eslint 0. Boot smoke: GET /api/progression 200 (drives _progression_overview + both accessors), events 400 on bad body. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
5f58af4faa
|
refactor(server): extract the shop routes + inject get_progression_content into the seam (R3) (#851)
The progression-content substrate: `_get_progression_content` (a lazy, double-checked-locking content cache) is now published into the appstate seam as a CALLABLE. The cache global + lock + the function stay in server.py (startup uses it, and test_progression_api patches `server._progression_content` directly), so ZERO test retargeting — routers just call `appstate.get_progression_content()`. Because the accessor is defined at server.py:1152 but the import-top configure() runs at :346, a second `appstate.configure(get_progression_content=...)` publishes it right after the def (configure is idempotent/additive). First consumer: routers/shop.py (3 routes: buy/equip/list). Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db, _clean_str from reqfields, _get_progression_content() -> appstate.get_progression_content(). This unblocks stats/progression/profile next (all share the accessor). server.py: 7,880 -> 7,845. Verified: pyflakes clean; route table IDENTICAL (143); pytest 2401 passed (test_progression_api's server._progression_content patch still works via the kept cache); packaging guard; eslint 0. Boot smoke: GET /api/shop 200 (drives appstate.get_progression_content), buy 400 on bad body. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
ea8834862d
|
refactor(server): batch the small meta_db user-state endpoints into routers/library_extras.py (R3) (#850)
work keeper-chart prefs (3) + favorites toggle + tags list + saved toggle + session/continue — 7 routes across 5 domains, all meta_db-only (0 setattr, 0 helpers). Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db, _clean_str from reqfields. These were scattered singletons with no natural neighbor, so they're grouped as "small library/user-state endpoints" and mounted once. All paths are distinct and non-overlapping, so registering them together doesn't change routing: verified the route SET is identical to origin/main (143) AND that no moved path shadows or is shadowed by another (order-independence check). server.py: 7,880 -> 7,833. Verified: pyflakes clean; route set identical + order-independent; pytest 2401 passed; packaging guard 45; eslint 0. Boot smoke: tags/session GET 200, favorites/saved toggle (400 on missing filename), work/charts 200. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
cbc65458e3
|
refactor(server): extract the wishlist routes into routers/wanted.py (R3) (#847)
Free after _clean_str moved to lib (#841): wanted's deps are JSONResponse, _clean_str (reqfields), app + meta_db (seam). 3 routes (list/add/remove), 0 setattr targets, 0 helpers to relocate. Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db. include_router at the original site; 143-route table identical. No test retargeting. server.py: 8,003 -> 7,974 (this branch is independent of the chart PR). Verified: pyflakes clean; route table identical; pytest 2401 passed (52 in test_wanted_api); eslint 0. Boot smoke: add wishlist entry -> list -> delete, 400 on missing artist+title (_clean_str path). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
7c87538d6b
|
refactor(server): extract the chart routes into routers/chart.py (R3) (#846)
Unblocked by the DLC-path substrate (#843): chart's only server-module deps are now app + meta_db (both seam); _get_dlc_dir/_resolve_dlc_path come from dlc_paths, sloppak/loose detection from the shared lib modules. 4 routes (split/unsplit/ work/fileinfo), meta_db-only otherwise. 0 setattr targets, 0 helpers to relocate. Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db. include_router at the original site; 143-route table identical to origin/main. No test retargeting. server.py: 8,003 -> 7,909. Verified: pyflakes clean on the router; no new undefined/dead in server.py; route table identical; pytest 2401 passed (74 across work_charts/context_menu/ group_filter/packaging); eslint 0. Boot smoke: chart/work 200, chart/fileinfo resolves the real pack path through _resolve_dlc_path. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
c8701991cb
|
fix(ws_highway): swallow a mid-stream WebSocketDisconnect quietly (#845)
* fix(ws_highway): swallow a mid-stream WebSocketDisconnect quietly Pre-existing (byte-identical to origin/main), flagged by CodeRabbit on #844: the outer try's only guard was `except Exception as e: log.exception("highway_ws unhandled error")`. The inner `except WebSocketDisconnect` covers ONLY the post-`ready` keep-alive loop, and the drum_tab/notation sends have their own localized guards — but the ~13 other streamed sends (beats, sections, notes, chords, anchors, …) fall through to the blanket handler. Since WebSocketDisconnect is an Exception subclass, a routine tab-close mid-load was logged as an error at whichever send was in flight. Fix: a dedicated `except WebSocketDisconnect: return` before the blanket handler, matching the two localized guards and the lib/ coding guideline. tests/test_ws_highway_disconnect.py drives highway_ws with a fake websocket that raises WebSocketDisconnect on the first streamed send (`loading`), over a real minimal sloppak. Negative-checked: removing the guard makes it fail (the disconnect is logged as `highway_ws unhandled error`); the fix passes. Uses a raw handler on `feedBack.server` rather than caplog, since configure_logging() reroutes that logger through structlog where caplog doesn't observe it. Full suite green. Closes the review thread on #844. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ws_highway): assert streaming stops after disconnect (exactly one send) — CodeRabbit The stub now raises only on the first send and the test asserts ws.sends == 1, so a handler that caught the disconnect and kept streaming would fail. Still negative-checked: removing the guard fails the test. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
514461167e
|
refactor(server): extract the highway WebSocket into routers/ws_highway.py (R3) (#844)
The single largest handler in server.py — the 902-line /ws/highway/{filename}
chart streamer — plus its 3 exclusive helpers (_pick_smart_arrangement,
_sanitize_authors, _sanitized_song_offset). server.py: 9,008 -> 8,003 (-1,005,
the biggest single R3 cut).
Clean move despite the size: the handler's only server-module deps are the 4
path constants + 3 exclusive helpers + app + log. Everything else it uses is
either NESTED inside the handler (_evict_audio_cache, _fill_scale_degree,
_manifest_entries, _tone_names, _xml_rank, _send_keepalives) or imported from
the shared lib modules (song/audio/sloppak/drums/notation/dlc_paths/metadata_db).
- Path constants read through the appstate seam: added static_dir /
sloppak_cache_dir / audio_cache_dir slots (config_dir already there);
server.py configures them. Bodies otherwise verbatim (@app.websocket ->
@router.websocket, PATHS -> appstate.*, log -> module logger).
- sloppak_cache_dir IS setattr-patched, so the 3 test_highway_ws_* suites now
also `setattr(appstate, "sloppak_cache_dir", ...)` next to their existing
server patch. _sanitize_authors unit tests import it from routers.ws_highway
(it moved). No other test churn.
- Removed 23 now-dead imports from server.py (song/audio/drums/notation/
bisect/contextvars/structlog/WebSocket*/_arr_smart_sort_key) — diffed against
the origin/main unused-import baseline so only NEWLY-dead ones went.
owns_tmp (assigned, never read) moved verbatim — it's pre-existing dead on
origin/main too; left as-is to keep the move faithful.
Verified: route table identical to origin/main (143, paths/methods/order);
handler body verbatim spot-checked; pyflakes clean (server has no new
undefined/dead); pytest 2400 passed; packaging guard 53; eslint 0. Boot smoke:
the highway WS streams the full chart (song_info/beats/sections/notes/chords/
notation/anchors/drum_tab -> ready) BYTE-for-byte the same message sequence as
origin/main across arrangements 0/1/2, zero tracebacks.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Byron Gamatos
|
0dcc9136b6
|
refactor(server): move DLC path resolution to lib/dlc_paths.py (R3 substrate) (#843)
The keystone for the path-dependent routers (ws_highway, song, audio-local-path, sloppak): the "where do the song files live + safe containment" layer leaves server.py so a router can reach it. - `_resolve_dlc_path` (pure — args only) moves verbatim. - `_get_dlc_dir` reads the env-derived paths through the appstate seam (appstate.dlc_dir/dlc_dir_env/config_dir) instead of module globals, so lib/dlc_paths.py does no import-time IO. - appstate gains `dlc_dir`/`dlc_dir_env` slots (config_dir already there); server.py configures them. All three are env-derived, so a setenv+reimport fixture reconfigures them for free — ZERO setattr retargeting. - server.py RE-EXPORTS both (`from dlc_paths import _get_dlc_dir, _resolve_dlc_path`), so its 24+16 call sites AND the tests that reach `server._get_dlc_dir()` / `server._resolve_dlc_path()` directly (test_dlc_junction, test_highway_ws_*) resolve unchanged — no test edits. server.py: 9,085 -> 9,008. Verified: _resolve_dlc_path byte-identical to origin/main; _get_dlc_dir's only change is the three path identifiers -> appstate.*; pyflakes clean; route table identical (143); pytest 2407 passed (test_dlc_junction + both highway_ws suites green via re-export); packaging guard 52; eslint 0. Boot smoke: library scans (8 songs, _get_dlc_dir), a real song resolves + serves art (_resolve_dlc_path), and the highway WS reaches `ready` with notes. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
6da01c55a4
|
fix(playlists): split cover decode (400) from persist (500), unique temp, existence re-check (#842)
Two pre-existing cover-upload issues CodeRabbit flagged on #841 (verbatim move, so correctly not fixed there): 1. One `except Exception as e` wrapped BOTH the PIL decode and the img.save/ tmp.replace, returned 400 for both, and echoed `e` — so a disk/permission failure was mislabeled a client error and could leak a filesystem path. Now: decode/validation -> 400 "Invalid image" (generic); save/replace failure -> logged 500 "could not save cover" (no detail). 2. A shared `{pid}.png.tmp` let two concurrent uploads clobber each other's temp file. Now a unique `tempfile.mkstemp` in the cover dir, atomic replace to publish. Plus an existence re-check just before publishing so an upload that raced a playlist delete can't leave an orphan cover. mkstemp itself is INSIDE the try (Codex catch): an unwritable dir / full disk raises there and is the same persistence failure as save/replace, so it hits the logged generic-500 path instead of escaping as an unhandled 500. Cleanup guards `tmp is not None` for the mkstemp-failed case. Did NOT add a full per-playlist critical section (CodeRabbit's "heavy lift"): FeedBack is single-user (Principle I), so a cover upload racing a delete on the same id can't happen — documented in the code rather than building a lock framework for a precluded race. tests/test_playlist_cover_errors.py pins all of it; negative-checked three ways: the old single-except-400 shape fails the 500 + no-leak-400 tests, and moving mkstemp back outside the try fails the temp-creation-500 test. Fix passes 5/5. Full suite 2405 passed; boot smoke: valid cover 200, bad image -> generic "Invalid image" 400, no .tmp litter. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
a883f9213f
|
refactor(server): extract the playlists routes into routers/playlists.py (R3) (#841)
The biggest router yet — 12 playlist routes + custom covers — and the first that needs the config-path seam. server.py: 9,302 -> 9,085 (-217). Three causally-linked pieces, all required for playlists: - `config_dir` joins the appstate seam (the plan always put the path constants there; deferred in S3, needed now). It's env-derived, so the ~49 pop-and-reimport fixtures reconfigure it for free — ZERO setattr retargeting. STATIC_DIR/SLOPPAK_CACHE_DIR (patched via setattr) stay in server.py until a router that reads them is extracted, and get retargeted then. - `_clean_str` (pure request-field sanitizer, 14 callers) -> lib/reqfields.py; server.py imports it back. Unblocks wanted/saved/collections/profile/... later. - routers/playlists.py: bodies verbatim, `@app`->`@router`, `meta_db`-> `appstate.meta_db`, `CONFIG_DIR`->`appstate.config_dir`, `_clean_str` from reqfields, `_ART_CACHE_HEADERS` as a local const (art keeps server.py's). The two exclusive cover helpers (_playlist_cover_path/_url) move with it. include_router at the original site; full 143-route table identical to origin/main. One test retarget: test_playlists_api called `server._playlist_cover_path` directly -> now imports it from routers.playlists (reads appstate.config_dir, which the `server` fixture configures). Verified: pyflakes clean; route table identical; pytest 2401 passed (28 in playlists+collections+appstate); packaging guard 51 (auto-picked up reqfields); eslint 0; boot smoke drives create/rename/add-song/cover-upload/serve/delete — the cover writes 1.png under CONFIG_DIR THROUGH appstate.config_dir and serves 200 with an mtime cache-bust token; a wrong-typed name field still 400s via _clean_str; demo untouched. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
4fd0cd49e7
|
fix(loops): take the DB lock for the count+insert and the list read (#840)
Two pre-existing races in the loops routes, flagged by CodeRabbit on #839 (the verbatim extraction moved them unchanged from server.py, so they correctly weren't fixed there): 1. save_loop computed `COUNT(*)` OUTSIDE meta_db._lock, then inserted inside it. Two simultaneous unnamed POSTs read the same count and both mint "Loop N". Fix: one lock scope around COUNT + INSERT. 2. list_loops read the shared single connection (check_same_thread=False) with no lock, so it could overlap a POST/DELETE commit. Fix: read under the lock, like every writer. Low severity in context — FeedBack is single-user (Principle I), so concurrent unnamed-loop POSTs essentially can't happen — but each fix is one lock scope. tests/test_loops_concurrency.py pins both with a threading.Barrier that releases 16 workers into save_loop at once. Negative-checked: reverting the COUNT back outside the lock fails the uniqueness assertion 5/5 runs; the fix passes 3/3. pytest 2400 passed; on-device two unnamed POSTs -> ['Loop 1','Loop 2']. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
b41361eb1b
|
refactor(server): extract the loops routes into routers/loops.py (R3) (#839)
Third router. Practice loops (saved A/B regions per song): GET/POST/DELETE /api/loops, meta_db-only (0 setattr targets, 0 helpers to relocate per router_scan.py). Bodies verbatim; @app -> @router, meta_db -> appstate.meta_db. include_router at the original site; 143-route table identical to origin/main. server.py: 9,337 -> 9,301. No test retargeting (test_demo_mode only names the paths in middleware regexes). Verified: pyflakes clean; route table identical; pytest 2398 passed; packaging guard green; eslint 0; boot smoke drives POST (auto-names "Loop N") / GET / DELETE / missing-fields error, and demo mode 403s both writes while allowing the read. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Byron Gamatos
|
6c98aba433
|
refactor(server): extract the artist-alias routes into routers/artist_aliases.py (R3) (#838)
* refactor(server): extract the artist-alias routes into routers/artist_aliases.py (R3) Second router, picked by router_scan.py: the artist-aliases / Tidy-up (P4) group ranks at 0 monkeypatch.setattr targets and 0 helpers to relocate. 5 routes (list/set/merge/delete aliases + raw-artist picker), all meta_db-only. Bodies verbatim; only @app.<m> -> @router.<m> and meta_db -> appstate.meta_db. include_router mounts at the original site; full 143-route table identical to origin/main (paths, methods, order). No test retargets: test_artist_alias drives via TestClient(server.app) + server.meta_db, neither of which moved. Verified: pyflakes clean on the router; no new undefined in server.py; JSONResponse still used in server.py (not dead); pytest 2398 passed (18 in test_artist_alias); packaging guard green; eslint 0; boot smoke drives all 5 routes end-to-end (set ACDC->AC/DC, read back, 400 on missing fields, delete). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: credit both router extractions in the size-exemptions rationale (CodeRabbit) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |