mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-11 11:19:24 +00:00
library: opt-in gap-fill — write confirmed missing metadata into the pack (R4a) (#724)
* library: opt-in gap-fill — write confirmed missing metadata into the pack (R4a)
The write-back contract agreed with the spec chair (alignment doc §7),
made executable, now that feedpak-spec 1.14.0 (mbid/isrc) is merged:
opt-in + user-initiated, adds ABSENT keys only, spec'd-keys allowlist,
values only from a CONFIRMED identity, atomic write + .bak. Single-song
only — batch write-back stays an open question with the chair.
- songmeta.gap_fill_sloppak: append-only manifest writer. Every added
key is absent by definition, so the new lines are APPENDED — the
author's existing bytes (key order, comments, formatting) survive
verbatim, unlike the metadata editor's full re-serialize. Directory
form gets a one-time manifest.yaml.bak + temp + atomic replace; zip
form reuses the editor's backup/temp/replace rewriter. Raises on any
already-present key: the never-clobber rule lives in the writer, not
just the callers.
- GET /api/song/{fn}/gap-fill: read-only preview — which of
album/year/genres/mbid/isrc are missing from the file (absent or
empty; year 0 = empty), with the values the enrichment match
supplies. Only a CONFIRMED identity is eligible (matched or a user
pin); review-tier rows are refused until a human confirms —
wrong-match > fast, same as everywhere else in the enrichment layer.
- POST /api/song/{fn}/gap-fill {keys}: writes the user-confirmed
subset. Proposals are RECOMPUTED under _song_io_lock, so a key that
gained an author value between preview and confirm is skipped, never
replaced. mbid/isrc written in canonical form only (validated).
DB stays scanner-consistent (album/year/genre columns + mtime/size
re-stat, cache invalidation + scan kick — the metadata editor's
contract). Demo mode blocks the write.
- Details drawer (Identity section): "Write missing info to file…" →
per-key checkbox confirm ("Only adds what's missing — nothing already
in the file is changed. A backup (.bak) is kept.") → written
confirmation; not-eligible states explain themselves. v3 only; no
new tailwind classes.
- Rides along: _manifest_exact_ids now strips ISRC display separators
(spec 1.14.0's strip rule) — a hand-authored "AU-AP0-90-00045" hits
the exact-match tier instead of silently falling back to text.
Tests: tests/test_gap_fill.py (10) — preview eligibility incl.
review-refusal + empty-as-gap, author-bytes-preserved-verbatim on dir
AND zip (with .bak content pinned), skip-not-replace on the mixed
request, the writer's ValueError guard, key validation, demo block,
DB sync; +1 hyphenated-ISRC test in test_mb_enrichment.py. 46 targeted
green; full-suite failure set A/B-identical to the main base (39
env/pre-existing).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nm7tHs1Yvjjtnnu4nzJgdN
* gap-fill: align preview with append-only writer (no cleared-value 500)
The R4a preview offered present-but-empty manifest values (album: '',
genres: [], year: 0) as gaps, but the append-only writer's never-clobber
guard raises on ANY key already present — so a user-confirmed POST for
those keys turned into a 500 "write failed" instead of filling the gap.
Appending can't fill an empty-but-present key anyway (it would duplicate
the YAML key).
Fix: _gap_fill_manifest_absent now treats only genuinely-MISSING keys as
gaps; a present-but-empty value is left to the metadata editor (which
re-serializes and can replace in place). This closes the preview→POST
mismatch — the preview never offers what the writer would refuse.
Tests: test_preview_treats_empty_values_as_gaps replaced by
test_preview_excludes_present_but_empty_keys (present-but-empty not
offered; genuinely-absent still offered) + test_write_present_but_empty_
key_is_refused_not_500 (POST → clean 409, file untouched, no .bak; a
genuinely-absent key alongside still writes). Closes the write-path blind
spot in the original empty-value test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: byrongamatos <xasiklas@gmail.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
byrongamatos
parent
7ef52cdd66
commit
8e953e8bc4
@@ -107,6 +107,59 @@ def write_sloppak_metadata(path: Path, fields: dict) -> bool:
|
|||||||
return _rewrite_zip_manifest(path, dumped)
|
return _rewrite_zip_manifest(path, dumped)
|
||||||
|
|
||||||
|
|
||||||
|
def gap_fill_sloppak(path: Path, additions: dict) -> bool:
|
||||||
|
"""Append ABSENT top-level keys to a sloppak manifest (the gap-fill
|
||||||
|
contract: user-initiated, adds missing keys only, never replaces
|
||||||
|
anything the author set).
|
||||||
|
|
||||||
|
Unlike ``write_sloppak_metadata`` this does NOT re-serialize the
|
||||||
|
manifest — because every added key is absent by definition, the new
|
||||||
|
lines can simply be appended, so the author's existing bytes (key
|
||||||
|
order, comments, formatting) survive verbatim. Directory form gets a
|
||||||
|
one-time ``manifest.yaml.bak`` + temp + atomic replace; zip form goes
|
||||||
|
through the same backup/temp/replace rewriter the metadata editor
|
||||||
|
uses. Returns True if anything was written; raises ``ValueError`` if
|
||||||
|
a requested key already exists (callers are expected to have checked
|
||||||
|
— this is the last-line never-clobber guard)."""
|
||||||
|
import sloppak as sloppak_mod
|
||||||
|
|
||||||
|
path = Path(path)
|
||||||
|
if not additions:
|
||||||
|
return False
|
||||||
|
manifest = sloppak_mod.load_manifest(path) or {}
|
||||||
|
clash = sorted(k for k in additions if k in manifest)
|
||||||
|
if clash:
|
||||||
|
raise ValueError("gap-fill refused: key(s) already present: " + ", ".join(clash))
|
||||||
|
|
||||||
|
if path.is_dir():
|
||||||
|
mf = path / "manifest.yaml"
|
||||||
|
if not mf.exists() and (path / "manifest.yml").exists():
|
||||||
|
mf = path / "manifest.yml"
|
||||||
|
original = mf.read_text(encoding="utf-8")
|
||||||
|
else:
|
||||||
|
with zipfile.ZipFile(str(path), "r") as zin:
|
||||||
|
names = zin.namelist()
|
||||||
|
manifest_name = "manifest.yaml"
|
||||||
|
for cand in ("manifest.yaml", "manifest.yml"):
|
||||||
|
if cand in names:
|
||||||
|
manifest_name = cand
|
||||||
|
break
|
||||||
|
original = zin.read(manifest_name).decode("utf-8")
|
||||||
|
|
||||||
|
appended = original if original.endswith("\n") or not original else original + "\n"
|
||||||
|
appended += yaml.safe_dump(additions, sort_keys=False, allow_unicode=True)
|
||||||
|
|
||||||
|
if path.is_dir():
|
||||||
|
backup = mf.with_name(mf.name + ".bak")
|
||||||
|
if not backup.exists():
|
||||||
|
shutil.copy2(mf, backup)
|
||||||
|
tmp = mf.with_name(mf.name + ".tmp")
|
||||||
|
tmp.write_text(appended, encoding="utf-8")
|
||||||
|
tmp.replace(mf)
|
||||||
|
return True
|
||||||
|
return _rewrite_zip_manifest(path, appended)
|
||||||
|
|
||||||
|
|
||||||
def write_song_metadata(path: Path, fields: dict) -> bool:
|
def write_song_metadata(path: Path, fields: dict) -> bool:
|
||||||
"""Persist edited title/artist/album/year into the song's file.
|
"""Persist edited title/artist/album/year into the song's file.
|
||||||
|
|
||||||
|
|||||||
@@ -244,6 +244,8 @@ _DEMO_BLOCKED: list[tuple[str, re.Pattern]] = [
|
|||||||
# rate limit; Get-info exposes filesystem paths.
|
# rate limit; Get-info exposes filesystem paths.
|
||||||
("POST", re.compile(r"^/api/enrichment/refresh/.+$")),
|
("POST", re.compile(r"^/api/enrichment/refresh/.+$")),
|
||||||
("GET", re.compile(r"^/api/chart/.+/fileinfo$")),
|
("GET", re.compile(r"^/api/chart/.+/fileinfo$")),
|
||||||
|
# Gap-fill (R4a) rewrites pack files on disk — never for demo visitors.
|
||||||
|
("POST", re.compile(r"^/api/song/.+/gap-fill$")),
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
@@ -5711,6 +5713,11 @@ def _manifest_exact_ids(filename: str) -> dict:
|
|||||||
if _MBID_RE.match(mbid):
|
if _MBID_RE.match(mbid):
|
||||||
out["mbid"] = mbid
|
out["mbid"] = mbid
|
||||||
isrc = str(manifest.get("isrc", "") or "").strip().upper()
|
isrc = str(manifest.get("isrc", "") or "").strip().upper()
|
||||||
|
# Spec 1.14.0: the stored form is the bare 12-char code, but ISRCs
|
||||||
|
# circulate hyphenated in the wild (US-ABC-24-00001) — the separators
|
||||||
|
# are presentation, not part of the code, so a hand-authored display
|
||||||
|
# form still matches (consumers SHOULD strip before comparing).
|
||||||
|
isrc = isrc.replace("-", "").replace(" ", "")
|
||||||
if _ISRC_RE.match(isrc):
|
if _ISRC_RE.match(isrc):
|
||||||
out["isrc"] = isrc
|
out["isrc"] = isrc
|
||||||
return out
|
return out
|
||||||
@@ -10231,6 +10238,158 @@ def update_song_meta(filename: str, data: dict):
|
|||||||
return {"ok": True, "persisted": persisted}
|
return {"ok": True, "persisted": persisted}
|
||||||
|
|
||||||
|
|
||||||
|
# ── Gap-fill: write CONFIRMED missing metadata into the pack (R4a) ────────────
|
||||||
|
# The agreed write-back contract (spec-alignment §7): opt-in + user-initiated
|
||||||
|
# (nothing here runs in the background), adds ABSENT keys only (never replaces
|
||||||
|
# an author-set value — the writer refuses, and existing manifest bytes are
|
||||||
|
# preserved verbatim by appending), spec'd-keys allowlist, values only from a
|
||||||
|
# CONFIRMED identity (an auto/exact match or a user pin — review-tier rows are
|
||||||
|
# not eligible until a human confirms), atomic write + .bak. Single-song only;
|
||||||
|
# batch write-back stays an open question with the spec chair.
|
||||||
|
_GAP_FILL_KEYS = ("album", "year", "genres", "mbid", "isrc")
|
||||||
|
|
||||||
|
|
||||||
|
def _gap_fill_manifest_absent(manifest: dict, key: str) -> bool:
|
||||||
|
"""A key is a GAP only when it's genuinely MISSING from the manifest.
|
||||||
|
|
||||||
|
Gap-fill is append-only: the writer's never-clobber guard raises on ANY
|
||||||
|
key already present, and appending a second `album:` line to a manifest
|
||||||
|
that already carries `album: ''` would just create a duplicate YAML key.
|
||||||
|
So a present-but-empty value (None / '' / [] / year 0) is NOT a gap the
|
||||||
|
append-only writer can fill — offering it in the preview would only lead
|
||||||
|
to a POST the writer refuses. Present-but-empty keys are therefore left
|
||||||
|
to the metadata editor (which re-serializes and can replace in place)."""
|
||||||
|
return key not in manifest
|
||||||
|
|
||||||
|
|
||||||
|
def _gap_fill_proposals(cache_key: str, resolved) -> tuple[dict, str]:
|
||||||
|
"""What gap-fill could add for this song: (proposals, reason). Empty
|
||||||
|
proposals explain themselves via reason — 'not-sloppak', 'no-match'
|
||||||
|
(nothing confirmed yet), 'review' (a human hasn't confirmed the match),
|
||||||
|
or 'nothing-missing'."""
|
||||||
|
if resolved is None or not resolved.exists() or not sloppak_mod.is_sloppak(resolved):
|
||||||
|
return {}, "not-sloppak"
|
||||||
|
row = meta_db.get_enrichment(cache_key)
|
||||||
|
if not row or row.get("match_state") not in ("matched", "manual"):
|
||||||
|
state = (row or {}).get("match_state")
|
||||||
|
return {}, ("review" if state == "review" else "no-match")
|
||||||
|
try:
|
||||||
|
manifest = sloppak_mod.load_manifest(resolved) or {}
|
||||||
|
except Exception:
|
||||||
|
return {}, "not-sloppak"
|
||||||
|
out = {}
|
||||||
|
album = (row.get("canon_album") or "").strip()
|
||||||
|
if album and _gap_fill_manifest_absent(manifest, "album"):
|
||||||
|
out["album"] = album
|
||||||
|
year = (row.get("canon_year") or "").strip()
|
||||||
|
if year.isdigit() and int(year) and _gap_fill_manifest_absent(manifest, "year"):
|
||||||
|
out["year"] = int(year)
|
||||||
|
genres = [str(g) for g in (row.get("genres") or []) if isinstance(g, str) and g.strip()]
|
||||||
|
if genres and _gap_fill_manifest_absent(manifest, "genres"):
|
||||||
|
out["genres"] = genres
|
||||||
|
# Identity keys (feedpak spec 1.14.0) — written in canonical form only.
|
||||||
|
mbid = (row.get("mb_recording_id") or "").strip().lower()
|
||||||
|
if _MBID_RE.match(mbid) and _gap_fill_manifest_absent(manifest, "mbid"):
|
||||||
|
out["mbid"] = mbid
|
||||||
|
isrc = (row.get("isrc") or "").strip().upper().replace("-", "").replace(" ", "")
|
||||||
|
if _ISRC_RE.match(isrc) and _gap_fill_manifest_absent(manifest, "isrc"):
|
||||||
|
out["isrc"] = isrc
|
||||||
|
return out, ("" if out else "nothing-missing")
|
||||||
|
|
||||||
|
|
||||||
|
@app.get("/api/song/{filename:path}/gap-fill")
|
||||||
|
def get_song_gap_fill(filename: str):
|
||||||
|
"""Preview what "Write missing info to file" would add — the Details
|
||||||
|
drawer renders its confirm list straight from this. Read-only."""
|
||||||
|
dlc = _get_dlc_dir()
|
||||||
|
cache_key, resolved = filename, None
|
||||||
|
if dlc:
|
||||||
|
resolved = _resolve_dlc_path(dlc, filename)
|
||||||
|
if resolved is None:
|
||||||
|
return JSONResponse({"error": "forbidden"}, 403)
|
||||||
|
try:
|
||||||
|
cache_key = resolved.relative_to(dlc.resolve()).as_posix()
|
||||||
|
except ValueError:
|
||||||
|
pass
|
||||||
|
proposals, reason = _gap_fill_proposals(cache_key, resolved)
|
||||||
|
row = meta_db.get_enrichment(cache_key) or {}
|
||||||
|
return {
|
||||||
|
"eligible": bool(proposals),
|
||||||
|
"reason": reason,
|
||||||
|
"match_state": row.get("match_state"),
|
||||||
|
"missing": [{"key": k, "value": v} for k, v in proposals.items()],
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
@app.post("/api/song/{filename:path}/gap-fill")
|
||||||
|
def post_song_gap_fill(filename: str, data: dict):
|
||||||
|
"""Write the user-confirmed subset of the preview into the pack file.
|
||||||
|
Proposals are recomputed under the io lock, so a key that gained an
|
||||||
|
author value between preview and confirm is skipped, never replaced."""
|
||||||
|
keys = (data or {}).get("keys")
|
||||||
|
if not isinstance(keys, list) or not keys:
|
||||||
|
return JSONResponse({"error": "keys must be a non-empty list"}, 400)
|
||||||
|
bad = [k for k in keys if k not in _GAP_FILL_KEYS]
|
||||||
|
if bad:
|
||||||
|
return JSONResponse(
|
||||||
|
{"error": "unknown key(s): " + ", ".join(sorted(set(map(str, bad))))}, 400)
|
||||||
|
|
||||||
|
dlc = _get_dlc_dir()
|
||||||
|
cache_key, resolved = filename, None
|
||||||
|
if dlc:
|
||||||
|
resolved = _resolve_dlc_path(dlc, filename)
|
||||||
|
if resolved is None:
|
||||||
|
return JSONResponse({"error": "forbidden"}, 403)
|
||||||
|
try:
|
||||||
|
cache_key = resolved.relative_to(dlc.resolve()).as_posix()
|
||||||
|
except ValueError:
|
||||||
|
pass
|
||||||
|
|
||||||
|
with _song_io_lock:
|
||||||
|
proposals, reason = _gap_fill_proposals(cache_key, resolved)
|
||||||
|
additions = {k: proposals[k] for k in _GAP_FILL_KEYS if k in keys and k in proposals}
|
||||||
|
skipped = sorted(set(keys) - set(additions))
|
||||||
|
if not additions:
|
||||||
|
return JSONResponse({"error": "nothing to write", "reason": reason,
|
||||||
|
"skipped": skipped}, 409)
|
||||||
|
try:
|
||||||
|
import songmeta
|
||||||
|
songmeta.gap_fill_sloppak(resolved, additions)
|
||||||
|
except Exception:
|
||||||
|
log.warning("gap-fill write failed for %s", cache_key, exc_info=True)
|
||||||
|
return JSONResponse({"error": "write failed"}, 500)
|
||||||
|
|
||||||
|
# Keep the cache row consistent with what the scanner would now derive
|
||||||
|
# (same contract as the metadata editor above): sync the columns the
|
||||||
|
# scan reads from the keys we appended, then re-stat so the row stays
|
||||||
|
# cache-fresh.
|
||||||
|
fields = {}
|
||||||
|
if "album" in additions:
|
||||||
|
fields["album"] = additions["album"]
|
||||||
|
if "year" in additions:
|
||||||
|
fields["year"] = str(additions["year"])
|
||||||
|
if "genres" in additions:
|
||||||
|
fields["genre"] = additions["genres"][0]
|
||||||
|
with meta_db._lock:
|
||||||
|
updates = [f"{field} = ?" for field in fields]
|
||||||
|
params = list(fields.values())
|
||||||
|
try:
|
||||||
|
mtime, size = _stat_for_cache(resolved)
|
||||||
|
updates += ["mtime = ?", "size = ?"]
|
||||||
|
params += [mtime, size]
|
||||||
|
except OSError:
|
||||||
|
pass
|
||||||
|
if updates:
|
||||||
|
params.append(cache_key)
|
||||||
|
meta_db.conn.execute(
|
||||||
|
f"UPDATE songs SET {', '.join(updates)} WHERE filename = ?", params)
|
||||||
|
meta_db.conn.commit()
|
||||||
|
|
||||||
|
_invalidate_song_caches(cache_key)
|
||||||
|
_kick_scan()
|
||||||
|
return {"ok": True, "written": additions, "skipped": skipped}
|
||||||
|
|
||||||
|
|
||||||
@app.post("/api/song/{filename:path}/art/upload")
|
@app.post("/api/song/{filename:path}/art/upload")
|
||||||
async def upload_song_art_b64(filename: str, data: dict):
|
async def upload_song_art_b64(filename: str, data: dict):
|
||||||
"""Upload custom album art as base64 PNG/JPG."""
|
"""Upload custom album art as base64 PNG/JPG."""
|
||||||
|
|||||||
+76
-1
@@ -2461,6 +2461,7 @@
|
|||||||
diff: (meta.user_difficulty != null ? meta.user_difficulty : null),
|
diff: (meta.user_difficulty != null ? meta.user_difficulty : null),
|
||||||
notes: meta.notes || '', tags: (meta.tags || []).slice(),
|
notes: meta.notes || '', tags: (meta.tags || []).slice(),
|
||||||
fav: !!song.favorite, artDataUrl: null,
|
fav: !!song.favorite, artDataUrl: null,
|
||||||
|
gap: null, gapSel: null, // gap-fill (R4a): preview state + selected keys
|
||||||
};
|
};
|
||||||
|
|
||||||
const overlay = document.createElement('div');
|
const overlay = document.createElement('div');
|
||||||
@@ -2487,6 +2488,41 @@
|
|||||||
if (first) { try { first.focus({ preventScroll: true }); const n = first.value.length; first.setSelectionRange(n, n); } catch (_) { /* */ } }
|
if (first) { try { first.focus({ preventScroll: true }); const n = first.value.length; first.setSelectionRange(n, n); } catch (_) { /* */ } }
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Gap-fill (R4a) block inside the drawer's Identity section: preview →
|
||||||
|
// per-key confirm → written. Adds ABSENT keys only; the server re-checks
|
||||||
|
// under its io lock, so this UI can never replace an author-set value.
|
||||||
|
const GAP_KEY_LABELS = { album: 'Album', year: 'Year', genres: 'Genres', mbid: 'MusicBrainz ID', isrc: 'ISRC' };
|
||||||
|
function gapFillHtml(st) {
|
||||||
|
const g = st.gap;
|
||||||
|
if (!g) return '<button data-gapfill-check class="text-xs text-fb-textDim hover:text-fb-text">Write missing info to file…</button>';
|
||||||
|
if (g.loading) return '<div class="text-xs text-fb-textDim">Checking the file…</div>';
|
||||||
|
if (g.written) {
|
||||||
|
const names = Object.keys(g.written).map((k) => GAP_KEY_LABELS[k] || k).join(', ');
|
||||||
|
return '<div class="text-xs text-fb-text">✓ Added to file: ' + esc(names) + '</div>';
|
||||||
|
}
|
||||||
|
if (!g.eligible) {
|
||||||
|
const why = {
|
||||||
|
'not-sloppak': 'Only feedpak songs can be written to.',
|
||||||
|
'no-match': 'No confirmed match yet — nothing verified to write.',
|
||||||
|
'review': 'This song’s match is waiting for review — confirm it first.',
|
||||||
|
'nothing-missing': 'Nothing missing — the file already has all of this.',
|
||||||
|
}[g.reason] || 'Could not check the file. Try again.';
|
||||||
|
return '<div class="text-xs text-fb-textDim">' + esc(why) + '</div>';
|
||||||
|
}
|
||||||
|
const rows = (g.missing || []).map((m) => {
|
||||||
|
const val = Array.isArray(m.value) ? m.value.join(', ') : String(m.value);
|
||||||
|
return '<label class="flex items-center gap-2 text-sm text-fb-text">' +
|
||||||
|
'<input type="checkbox" data-gapfill-key="' + esc(m.key) + '"' + (st.gapSel && st.gapSel.has(m.key) ? ' checked' : '') + '>' +
|
||||||
|
'<span class="text-fb-textDim shrink-0">' + esc(GAP_KEY_LABELS[m.key] || m.key) + '</span>' +
|
||||||
|
'<span class="truncate" title="' + esc(val) + '">' + esc(val) + '</span></label>';
|
||||||
|
}).join('');
|
||||||
|
return '<div class="space-y-2">' +
|
||||||
|
'<div class="text-xs font-semibold uppercase tracking-wider text-fb-textDim">Write to file</div>' + rows +
|
||||||
|
'<div class="text-[0.6875rem] text-fb-textDim">Only adds what’s missing — nothing already in the file is changed. A backup (.bak) is kept beside the file.</div>' +
|
||||||
|
'<div class="flex gap-2"><button data-gapfill-write class="bg-fb-primary hover:bg-fb-primaryHi text-white px-3 py-1.5 rounded-lg text-xs font-semibold">Write to file</button>' +
|
||||||
|
'<button data-gapfill-cancel class="px-3 py-1.5 bg-fb-card/60 hover:bg-fb-card border border-fb-border/50 rounded-lg text-xs text-fb-text">Cancel</button></div></div>';
|
||||||
|
}
|
||||||
|
|
||||||
function detailsHtml(song, st, vocab) {
|
function detailsHtml(song, st, vocab) {
|
||||||
const art = st.artDataUrl || artUrl(song);
|
const art = st.artDataUrl || artUrl(song);
|
||||||
const diffBtns = [1, 2, 3, 4, 5].map((n) =>
|
const diffBtns = [1, 2, 3, 4, 5].map((n) =>
|
||||||
@@ -2519,7 +2555,8 @@
|
|||||||
'<div class="space-y-3"><div class="flex items-center gap-2"><div class="text-xs font-semibold uppercase tracking-wider text-fb-textDim">Identity</div>' +
|
'<div class="space-y-3"><div class="flex items-center gap-2"><div class="text-xs font-semibold uppercase tracking-wider text-fb-textDim">Identity</div>' +
|
||||||
'<span class="text-[0.625rem] px-1.5 py-0.5 rounded-full bg-gray-800/70 text-fb-textDim border border-gray-700" title="These came from the song's feedpak. Editing them writes back to the file.">From pack</span></div>' +
|
'<span class="text-[0.625rem] px-1.5 py-0.5 rounded-full bg-gray-800/70 text-fb-textDim border border-gray-700" title="These came from the song's feedpak. Editing them writes back to the file.">From pack</span></div>' +
|
||||||
field('det-title', 'Title', st.t) + field('det-artist', 'Artist', st.a) + field('det-album', 'Album', st.al) +
|
field('det-title', 'Title', st.t) + field('det-artist', 'Artist', st.a) + field('det-album', 'Album', st.al) +
|
||||||
'<div><label for="det-year" class="text-xs text-fb-textDim mb-1 block">Year</label><input type="text" inputmode="numeric" id="det-year" value="' + esc(st.y) + '" placeholder="e.g. 2024" class="w-full bg-fb-card border border-fb-border/60 rounded-lg px-3 py-2 text-sm text-fb-text outline-none focus:border-fb-primary/60"></div></div>' +
|
'<div><label for="det-year" class="text-xs text-fb-textDim mb-1 block">Year</label><input type="text" inputmode="numeric" id="det-year" value="' + esc(st.y) + '" placeholder="e.g. 2024" class="w-full bg-fb-card border border-fb-border/60 rounded-lg px-3 py-2 text-sm text-fb-text outline-none focus:border-fb-primary/60"></div>' +
|
||||||
|
'<div data-det-gapfill>' + gapFillHtml(st) + '</div></div>' +
|
||||||
|
|
||||||
// Personal practice layer — local, never shared
|
// Personal practice layer — local, never shared
|
||||||
'<div class="space-y-3 pt-1"><div class="text-xs font-semibold uppercase tracking-wider text-fb-textDim">Your practice <span class="normal-case font-normal text-fb-textDim/70">· stays on this device</span></div>' +
|
'<div class="space-y-3 pt-1"><div class="text-xs font-semibold uppercase tracking-wider text-fb-textDim">Your practice <span class="normal-case font-normal text-fb-textDim/70">· stays on this device</span></div>' +
|
||||||
@@ -2586,6 +2623,44 @@
|
|||||||
|
|
||||||
$('[data-det-save]')?.addEventListener('click', () => saveDetails(song, st));
|
$('[data-det-save]')?.addEventListener('click', () => saveDetails(song, st));
|
||||||
$('[data-det-remove]')?.addEventListener('click', () => removeFromLibrary(song));
|
$('[data-det-remove]')?.addEventListener('click', () => removeFromLibrary(song));
|
||||||
|
|
||||||
|
// Gap-fill (R4a): user-initiated write of CONFIRMED missing info into
|
||||||
|
// the pack file. The server recomputes proposals under its io lock, so
|
||||||
|
// a key that gained an author value since the preview is skipped.
|
||||||
|
$('[data-gapfill-check]')?.addEventListener('click', async () => {
|
||||||
|
st.gap = { loading: true }; render();
|
||||||
|
let d = null;
|
||||||
|
try { const r = await fetch('/api/song/' + enc(song.filename) + '/gap-fill'); if (r.ok) d = await r.json(); } catch (_) { /* offline */ }
|
||||||
|
st.gap = d || { eligible: false, reason: 'error' };
|
||||||
|
st.gapSel = new Set(((d && d.missing) || []).map((m) => m.key));
|
||||||
|
render();
|
||||||
|
});
|
||||||
|
drawer.querySelectorAll('[data-gapfill-key]').forEach((cb) => cb.addEventListener('change', () => {
|
||||||
|
const k = cb.getAttribute('data-gapfill-key');
|
||||||
|
if (!st.gapSel) st.gapSel = new Set();
|
||||||
|
if (cb.checked) st.gapSel.add(k); else st.gapSel.delete(k);
|
||||||
|
}));
|
||||||
|
$('[data-gapfill-cancel]')?.addEventListener('click', () => { st.gap = null; render(); });
|
||||||
|
$('[data-gapfill-write]')?.addEventListener('click', async () => {
|
||||||
|
const keys = st.gapSel ? Array.from(st.gapSel) : [];
|
||||||
|
if (!keys.length) return;
|
||||||
|
let d = null, ok = false;
|
||||||
|
try {
|
||||||
|
const r = await fetch('/api/song/' + enc(song.filename) + '/gap-fill', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ keys }) });
|
||||||
|
ok = r.ok; d = await r.json();
|
||||||
|
} catch (_) { /* offline */ }
|
||||||
|
if (!ok || !d || !d.written) {
|
||||||
|
if (window.fbNotify) { try { window.fbNotify.show({ title: 'Write failed', message: 'Could not write to the file. Please try again.', icon: '⚠️', accent: '#EF4444' }); } catch (e) { /* */ } }
|
||||||
|
st.gap = null; render(); return;
|
||||||
|
}
|
||||||
|
// Reflect what landed in the open drawer (and keep saveDetails'
|
||||||
|
// changed-detection honest by updating both sides), then refresh
|
||||||
|
// the grid quietly.
|
||||||
|
if (d.written.album != null) { st.al = String(d.written.album); song.album = st.al; }
|
||||||
|
if (d.written.year != null) { st.y = String(d.written.year); song.year = d.written.year; }
|
||||||
|
st.gap = { written: d.written }; render();
|
||||||
|
try { reload(); } catch (_) { /* not on the songs grid */ }
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
// Normalize + append a tag to the drawer's working set (mirrors the server's
|
// Normalize + append a tag to the drawer's working set (mirrors the server's
|
||||||
|
|||||||
@@ -0,0 +1,259 @@
|
|||||||
|
"""Tests for the R4a gap-fill write-back — the §7 contract made executable:
|
||||||
|
user-initiated, adds ABSENT keys only (author bytes preserved verbatim),
|
||||||
|
spec'd-keys allowlist, values only from a CONFIRMED match, atomic + .bak.
|
||||||
|
|
||||||
|
No network anywhere: matches are seeded straight into the enrichment cache
|
||||||
|
(as the P8 matcher would have written them).
|
||||||
|
"""
|
||||||
|
|
||||||
|
import importlib
|
||||||
|
import sys
|
||||||
|
import zipfile
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
import yaml
|
||||||
|
from fastapi.testclient import TestClient
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture()
|
||||||
|
def server(tmp_path, monkeypatch, isolate_logging):
|
||||||
|
monkeypatch.setenv("CONFIG_DIR", str(tmp_path / "config"))
|
||||||
|
dlc = tmp_path / "dlc"
|
||||||
|
dlc.mkdir()
|
||||||
|
monkeypatch.setenv("DLC_DIR", str(dlc))
|
||||||
|
monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1")
|
||||||
|
sys.modules.pop("server", None)
|
||||||
|
srv = importlib.import_module("server")
|
||||||
|
try:
|
||||||
|
yield srv
|
||||||
|
finally:
|
||||||
|
conn = getattr(getattr(srv, "meta_db", None), "conn", None)
|
||||||
|
if conn is not None:
|
||||||
|
conn.close()
|
||||||
|
sys.modules.pop("server", None)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture()
|
||||||
|
def client(server):
|
||||||
|
return TestClient(server.app)
|
||||||
|
|
||||||
|
|
||||||
|
BASE_MANIFEST = ("# my hand-made pack\n"
|
||||||
|
"title: Thunderstruck\n"
|
||||||
|
"artist: AC/DC # the real one\n"
|
||||||
|
"duration: 292\n"
|
||||||
|
"arrangements: []\n"
|
||||||
|
"stems: []\n")
|
||||||
|
|
||||||
|
|
||||||
|
def make_dir_sloppak(server, name, manifest=BASE_MANIFEST):
|
||||||
|
d = server.DLC_DIR / name
|
||||||
|
d.mkdir(parents=True)
|
||||||
|
(d / "manifest.yaml").write_text(manifest, encoding="utf-8")
|
||||||
|
_put_db(server, name)
|
||||||
|
return d
|
||||||
|
|
||||||
|
|
||||||
|
def make_zip_sloppak(server, name, manifest=BASE_MANIFEST):
|
||||||
|
p = server.DLC_DIR / name
|
||||||
|
with zipfile.ZipFile(p, "w", zipfile.ZIP_DEFLATED) as z:
|
||||||
|
z.writestr("manifest.yaml", manifest)
|
||||||
|
z.writestr("stems/full.ogg", b"OggS-fake")
|
||||||
|
_put_db(server, name)
|
||||||
|
return p
|
||||||
|
|
||||||
|
|
||||||
|
def _put_db(server, name):
|
||||||
|
server.meta_db.put(name, 0, 0, {
|
||||||
|
"title": "Thunderstruck", "artist": "AC/DC", "album": "", "year": "",
|
||||||
|
"duration": 292, "arrangements": [{"name": "Lead", "index": 0}],
|
||||||
|
})
|
||||||
|
|
||||||
|
|
||||||
|
def seed_match(server, fn, state="matched", **overrides):
|
||||||
|
"""Seed a confirmed enrichment row as the P8 matcher would have."""
|
||||||
|
cand = {"recording_id": "12345678-abcd-4ef0-9876-0123456789ab",
|
||||||
|
"release_id": "rel-1", "artist_id": "art-1",
|
||||||
|
"artist": "AC/DC", "title": "Thunderstruck",
|
||||||
|
"album": "The Razors Edge", "year": "1990",
|
||||||
|
"genres": ["hard rock", "rock"], "isrc": "AUAP09000045"}
|
||||||
|
cand.update(overrides)
|
||||||
|
song = server.meta_db.enrichment_song_row(fn)
|
||||||
|
h = server.meta_db.enrichment_content_hash(
|
||||||
|
song["artist"], song["title"], song["album"], song["duration"])
|
||||||
|
server.meta_db.apply_enrichment_match(
|
||||||
|
fn, h, state, source="text", score=1.0, cand=cand,
|
||||||
|
candidates=([cand] if state == "review" else None))
|
||||||
|
|
||||||
|
|
||||||
|
# ── preview ───────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
def test_preview_offers_only_absent_confirmed_keys(server, client):
|
||||||
|
make_dir_sloppak(server, "a.sloppak")
|
||||||
|
seed_match(server, "a.sloppak")
|
||||||
|
d = client.get("/api/song/a.sloppak/gap-fill").json()
|
||||||
|
assert d["eligible"] is True
|
||||||
|
got = {m["key"]: m["value"] for m in d["missing"]}
|
||||||
|
assert got == {"album": "The Razors Edge", "year": 1990,
|
||||||
|
"genres": ["hard rock", "rock"],
|
||||||
|
"mbid": "12345678-abcd-4ef0-9876-0123456789ab",
|
||||||
|
"isrc": "AUAP09000045"}
|
||||||
|
|
||||||
|
|
||||||
|
def test_preview_excludes_author_set_keys(server, client):
|
||||||
|
make_dir_sloppak(server, "a.sloppak",
|
||||||
|
BASE_MANIFEST + "album: Live Bootleg\nyear: 1991\n")
|
||||||
|
seed_match(server, "a.sloppak")
|
||||||
|
got = {m["key"] for m in client.get("/api/song/a.sloppak/gap-fill").json()["missing"]}
|
||||||
|
assert "album" not in got and "year" not in got
|
||||||
|
assert {"genres", "mbid", "isrc"} <= got
|
||||||
|
|
||||||
|
|
||||||
|
def test_preview_excludes_present_but_empty_keys(server, client):
|
||||||
|
"""Gap-fill is append-only, so a present-but-empty value (album: '',
|
||||||
|
year: 0) is NOT a gap the writer can fill — appending would duplicate the
|
||||||
|
key, and the never-clobber guard refuses any present key. The preview must
|
||||||
|
therefore not offer it (those are the metadata editor's job to re-serialize),
|
||||||
|
while genuinely-absent keys are still offered."""
|
||||||
|
make_dir_sloppak(server, "a.sloppak", BASE_MANIFEST + "album: ''\nyear: 0\n")
|
||||||
|
seed_match(server, "a.sloppak")
|
||||||
|
got = {m["key"] for m in client.get("/api/song/a.sloppak/gap-fill").json()["missing"]}
|
||||||
|
assert "album" not in got and "year" not in got
|
||||||
|
assert {"genres", "mbid", "isrc"} <= got
|
||||||
|
|
||||||
|
|
||||||
|
def test_write_present_but_empty_key_is_refused_not_500(server, client):
|
||||||
|
"""The preview↔writer contract must agree: a POST for a present-but-empty
|
||||||
|
key is turned away with a clean 409 (never offered → skipped), never a 500
|
||||||
|
from the writer's never-clobber guard, and the file is left untouched."""
|
||||||
|
d = make_dir_sloppak(server, "a.sloppak", BASE_MANIFEST + "album: ''\nyear: 0\n")
|
||||||
|
seed_match(server, "a.sloppak")
|
||||||
|
before = (d / "manifest.yaml").read_text(encoding="utf-8")
|
||||||
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album", "year"]})
|
||||||
|
assert r.status_code == 409
|
||||||
|
assert sorted(r.json()["skipped"]) == ["album", "year"]
|
||||||
|
assert (d / "manifest.yaml").read_text(encoding="utf-8") == before
|
||||||
|
assert not (d / "manifest.yaml.bak").exists() # nothing written → no backup
|
||||||
|
# A genuinely-absent key alongside the empty ones still writes cleanly.
|
||||||
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album", "genres"]})
|
||||||
|
assert r.status_code == 200
|
||||||
|
assert r.json() == {"ok": True, "written": {"genres": ["hard rock", "rock"]},
|
||||||
|
"skipped": ["album"]}
|
||||||
|
|
||||||
|
|
||||||
|
def test_preview_requires_confirmed_match(server, client):
|
||||||
|
make_dir_sloppak(server, "a.sloppak")
|
||||||
|
d = client.get("/api/song/a.sloppak/gap-fill").json()
|
||||||
|
assert d["eligible"] is False and d["reason"] == "no-match"
|
||||||
|
seed_match(server, "a.sloppak", state="review")
|
||||||
|
d = client.get("/api/song/a.sloppak/gap-fill").json()
|
||||||
|
assert d["eligible"] is False and d["reason"] == "review"
|
||||||
|
# A user-pinned match is confirmed.
|
||||||
|
seed_match(server, "a.sloppak", state="manual")
|
||||||
|
assert client.get("/api/song/a.sloppak/gap-fill").json()["eligible"] is True
|
||||||
|
|
||||||
|
|
||||||
|
# ── writing ───────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
def test_write_dir_form_appends_and_preserves_author_bytes(server, client):
|
||||||
|
d = make_dir_sloppak(server, "a.sloppak")
|
||||||
|
seed_match(server, "a.sloppak")
|
||||||
|
r = client.post("/api/song/a.sloppak/gap-fill",
|
||||||
|
json={"keys": ["album", "year", "genres", "mbid", "isrc"]})
|
||||||
|
assert r.status_code == 200
|
||||||
|
body = r.json()
|
||||||
|
assert set(body["written"]) == {"album", "year", "genres", "mbid", "isrc"}
|
||||||
|
text = (d / "manifest.yaml").read_text(encoding="utf-8")
|
||||||
|
# The author's original bytes — comments included — survive verbatim as a
|
||||||
|
# prefix; the additions are appended after them.
|
||||||
|
assert text.startswith(BASE_MANIFEST)
|
||||||
|
manifest = yaml.safe_load(text)
|
||||||
|
assert manifest["album"] == "The Razors Edge"
|
||||||
|
assert manifest["year"] == 1990
|
||||||
|
assert manifest["genres"] == ["hard rock", "rock"]
|
||||||
|
assert manifest["mbid"] == "12345678-abcd-4ef0-9876-0123456789ab"
|
||||||
|
assert manifest["isrc"] == "AUAP09000045"
|
||||||
|
# Backup + DB sync (the row must match what the scanner would derive).
|
||||||
|
assert (d / "manifest.yaml.bak").read_text(encoding="utf-8") == BASE_MANIFEST
|
||||||
|
row = client.get("/api/song/a.sloppak").json()
|
||||||
|
assert row["album"] == "The Razors Edge"
|
||||||
|
assert str(row["year"]) == "1990"
|
||||||
|
|
||||||
|
|
||||||
|
def test_write_zip_form_appends_with_backup(server, client):
|
||||||
|
p = make_zip_sloppak(server, "a.sloppak")
|
||||||
|
seed_match(server, "a.sloppak")
|
||||||
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album", "mbid"]})
|
||||||
|
assert r.status_code == 200
|
||||||
|
with zipfile.ZipFile(p) as z:
|
||||||
|
text = z.read("manifest.yaml").decode("utf-8")
|
||||||
|
assert text.startswith(BASE_MANIFEST)
|
||||||
|
manifest = yaml.safe_load(text)
|
||||||
|
assert manifest["album"] == "The Razors Edge"
|
||||||
|
assert manifest["mbid"] == "12345678-abcd-4ef0-9876-0123456789ab"
|
||||||
|
assert "year" not in manifest # unrequested keys untouched
|
||||||
|
assert z.read("stems/full.ogg") == b"OggS-fake"
|
||||||
|
bak = p.with_name(p.name + ".bak")
|
||||||
|
assert bak.exists()
|
||||||
|
with zipfile.ZipFile(bak) as z:
|
||||||
|
assert z.read("manifest.yaml").decode("utf-8") == BASE_MANIFEST
|
||||||
|
|
||||||
|
|
||||||
|
def test_write_never_replaces_author_values(server, client):
|
||||||
|
d = make_dir_sloppak(server, "a.sloppak", BASE_MANIFEST + "album: Live Bootleg\n")
|
||||||
|
seed_match(server, "a.sloppak")
|
||||||
|
# Requesting a present key: skipped, not replaced; nothing else requested
|
||||||
|
# → 409 and the file is untouched.
|
||||||
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album"]})
|
||||||
|
assert r.status_code == 409
|
||||||
|
assert r.json()["skipped"] == ["album"]
|
||||||
|
assert (d / "manifest.yaml").read_text(encoding="utf-8").endswith("album: Live Bootleg\n")
|
||||||
|
# Mixed request: the gap is written, the author value survives.
|
||||||
|
r = client.post("/api/song/a.sloppak/gap-fill", json={"keys": ["album", "year"]})
|
||||||
|
assert r.status_code == 200
|
||||||
|
assert r.json() == {"ok": True, "written": {"year": 1990}, "skipped": ["album"]}
|
||||||
|
manifest = yaml.safe_load((d / "manifest.yaml").read_text(encoding="utf-8"))
|
||||||
|
assert manifest["album"] == "Live Bootleg"
|
||||||
|
assert manifest["year"] == 1990
|
||||||
|
|
||||||
|
|
||||||
|
def test_writer_last_line_guard(server):
|
||||||
|
"""The lib-level never-clobber guard holds even if a caller skips the
|
||||||
|
proposal check."""
|
||||||
|
import songmeta
|
||||||
|
d = make_dir_sloppak(server, "a.sloppak", BASE_MANIFEST + "album: Kept\n")
|
||||||
|
with pytest.raises(ValueError):
|
||||||
|
songmeta.gap_fill_sloppak(d, {"album": "Clobber"})
|
||||||
|
assert "Kept" in (d / "manifest.yaml").read_text(encoding="utf-8")
|
||||||
|
|
||||||
|
|
||||||
|
def test_write_validates_keys(server, client):
|
||||||
|
make_dir_sloppak(server, "a.sloppak")
|
||||||
|
seed_match(server, "a.sloppak")
|
||||||
|
assert client.post("/api/song/a.sloppak/gap-fill",
|
||||||
|
json={"keys": ["title"]}).status_code == 400
|
||||||
|
assert client.post("/api/song/a.sloppak/gap-fill",
|
||||||
|
json={"keys": []}).status_code == 400
|
||||||
|
assert client.post("/api/song/a.sloppak/gap-fill", json={}).status_code == 400
|
||||||
|
|
||||||
|
|
||||||
|
def test_demo_mode_blocks_write(tmp_path, monkeypatch, isolate_logging):
|
||||||
|
"""The middleware turns the write route away before any handler runs —
|
||||||
|
demo visitors can never rewrite pack files."""
|
||||||
|
monkeypatch.setenv("CONFIG_DIR", str(tmp_path / "config"))
|
||||||
|
dlc = tmp_path / "dlc"
|
||||||
|
dlc.mkdir()
|
||||||
|
monkeypatch.setenv("DLC_DIR", str(dlc))
|
||||||
|
monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1")
|
||||||
|
monkeypatch.setenv("FEEDBACK_DEMO_MODE", "1")
|
||||||
|
sys.modules.pop("server", None)
|
||||||
|
srv = importlib.import_module("server")
|
||||||
|
try:
|
||||||
|
r = TestClient(srv.app).post("/api/song/a.sloppak/gap-fill",
|
||||||
|
json={"keys": ["album"]})
|
||||||
|
assert r.status_code == 403
|
||||||
|
finally:
|
||||||
|
conn = getattr(getattr(srv, "meta_db", None), "conn", None)
|
||||||
|
if conn is not None:
|
||||||
|
conn.close()
|
||||||
|
sys.modules.pop("server", None)
|
||||||
@@ -262,6 +262,20 @@ def test_manifest_isrc_tier1(server, mb):
|
|||||||
assert mb.search_calls == []
|
assert mb.search_calls == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_manifest_isrc_display_hyphens_stripped(server, mb):
|
||||||
|
"""Spec 1.14.0: the hyphenated display form (AU-AP0-90-00045) is
|
||||||
|
presentation only — the reader strips separators, so a hand-authored
|
||||||
|
manifest still hits the exact tier with the bare 12-char code."""
|
||||||
|
_write_sloppak_manifest(server, "a.sloppak", "isrc: AU-AP0-90-00045\n")
|
||||||
|
_put(server, "a.sloppak")
|
||||||
|
mb.isrc_lookups["AUAP09000045"] = {"recordings": [mb_doc()]}
|
||||||
|
server._background_enrich()
|
||||||
|
row = server.meta_db.get_enrichment("a.sloppak")
|
||||||
|
assert row["match_state"] == "matched"
|
||||||
|
assert row["match_source"] == "isrc"
|
||||||
|
assert mb.search_calls == []
|
||||||
|
|
||||||
|
|
||||||
def test_bad_manifest_mbid_falls_through_to_text(server, mb):
|
def test_bad_manifest_mbid_falls_through_to_text(server, mb):
|
||||||
mbid = "12345678-abcd-4ef0-9876-0123456789ab"
|
mbid = "12345678-abcd-4ef0-9876-0123456789ab"
|
||||||
_write_sloppak_manifest(server, "a.sloppak", f"mbid: {mbid}\n")
|
_write_sloppak_manifest(server, "a.sloppak", f"mbid: {mbid}\n")
|
||||||
|
|||||||
Reference in New Issue
Block a user