From 8496e55baf2fc7ff08660a96c1790deb15e7b655 Mon Sep 17 00:00:00 2001 From: paul Date: Tue, 23 Jun 2026 00:29:59 +0000 Subject: [PATCH] refactor: extract testable resolve-core + cover album routing; check.sh checks .cjs (closes task/resolver-orchestration-testable) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Source: task/resolver-orchestration-testable (plan/steward-linker-quality) — new album orchestration in resolver.js had zero node:test coverage. - Extract the pure album/artist orchestration into service/resolve-core.js (resolveAll/resolveOne/pickDirect/cacheKey) taking injected search/classify fns; resolver.js becomes a thin server wiring the real Spotify search + ollama classify. No behavior change. - Add service/resolver.test.js: self-titled artist+album tie-break, LLM-album -> Google fallback, kind-aware cache separation, plus gate/drop/error paths. - scripts/check.sh: node --check now also matches *.cjs (extract.test.cjs). - AGENTS.md Testing: note resolve-core coverage + the extract.test.cjs extractor. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 10 ++- scripts/check.sh | 2 +- service/resolve-core.js | 96 ++++++++++++++++++++++++++ service/resolver.js | 95 ++++++-------------------- service/resolver.test.js | 141 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 265 insertions(+), 79 deletions(-) create mode 100644 service/resolve-core.js create mode 100644 service/resolver.test.js diff --git a/AGENTS.md b/AGENTS.md index c75df73..ea7ba56 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,9 +39,13 @@ user's PAT). Do **not** use `towl-orchestrate` / worktree+PR here. ## Testing (zero-dep) `node --test` over `service/*.test.js` exercises `lib.js` (norm + the exact-name match -+ link builders). Keep pure logic in `lib.js` (not `resolver.js`, which has the -server side effect) so it stays testable without opening a port. **Add/extend a test -with every logic change.** The extension's in-browser behavior is **not** headlessly ++ link builders), `llm.js` (the classifier prompt/parse), and `resolve-core.js` (the +album/artist orchestration — kindHint tie-break, kind-aware cache key, allowFallback +gating — driven with injected fake search/classify fns, no network). Keep pure logic in +`lib.js`/`resolve-core.js` (not `resolver.js`, which has the server side effect) so it stays +testable without opening a port. `extension/extract.test.cjs` covers the pure extractor +(artist/cue/album candidates) and must be extended with each detection change. **Add/extend a +test with every logic change.** The extension's in-browser behavior is **not** headlessly testable in this environment — validate `content.js` with `node --check` and a documented manual check on a real reddit thread (`about:debugging` → load temporary add-on). diff --git a/scripts/check.sh b/scripts/check.sh index e509548..913d7ad 100755 --- a/scripts/check.sh +++ b/scripts/check.sh @@ -5,7 +5,7 @@ set -euo pipefail cd "$(dirname "$0")/.." echo "== node --check (syntax) ==" -for f in $(find service extension -name '*.js'); do +for f in $(find service extension \( -name '*.js' -o -name '*.cjs' \)); do node --check "$f" echo " ok $f" done diff --git a/service/resolve-core.js b/service/resolve-core.js new file mode 100644 index 0000000..a781cfe --- /dev/null +++ b/service/resolve-core.js @@ -0,0 +1,96 @@ +// Pure album/artist resolution orchestration, decoupled from the HTTP server and the live +// Spotify/ollama clients so `node --test` can drive it with injected fakes. resolver.js wires +// the real `search` (a type=artist,album Spotify lookup behind the exact-name gate) and `classify` +// (the ollama LLM gate) into these functions; the cache-key logic, the kindHint tie-break +// ordering, and the allowFallback gating live here as pure logic. + +import { norm, linkResult } from "./lib.js"; + +// Cap on how many distinct candidates one resolveAll call considers. +export const MAX_CANDIDATES = 100; + +// Decide the single direct-link {url, kind} for a name from a search result, or null. `result` +// is what an injected `search(name)` returns: { artist, album } where each is the exact-match +// hit's external url (string) or null/absent. `kindHint` ("artist" | "album", from the LLM gate) +// breaks the tie when BOTH an artist and an album match the same string (e.g. a self-titled +// album): album-first when hinted "album", else artist-first (the artist-first MVP default). +export function pickDirect(result, kindHint) { + const artist = result?.artist ?? null; + const album = result?.album ?? null; + const order = kindHint === "album" + ? [["album", album], ["artist", artist]] + : [["artist", artist], ["album", album]]; + for (const [kind, url] of order) { + if (url) return { url, kind }; + } + return null; +} + +// The cache key for a name's direct-link lookup. It includes the kind hint because the same +// string can resolve to an artist OR an album depending on which the gate confirmed, so a +// hint-blind key would cross-contaminate the two. +export const cacheKey = (name, kindHint) => `${kindHint || "artist"}:${norm(name)}`; + +// Resolve one name to its link object, or null. `search(name) -> { artist, album }` is injected +// (the live Spotify lookup, or a fake). `cache` is a Map(cacheKey -> {url, kind}|null) memoizing +// the search half; the link object is rebuilt per call so a name's result never depends on a +// stale gate decision. allowFallback: when the gate already confirmed this name, a missing direct +// link still yields a Google->Bandcamp fallback link (instead of null). kindHint biases the +// lookup tie-break and the fallback link's kind; an actual exact hit's kind wins when there is one. +export async function resolveOne(name, { search, cache, allowFallback, kindHint }) { + if (!norm(name)) return null; + try { + const key = cacheKey(name, kindHint); + let hit; + if (cache && cache.has(key)) { + hit = cache.get(key); + } else { + hit = pickDirect(await search(name, kindHint), kindHint); + if (cache) cache.set(key, hit); + } + if (hit) return linkResult(name, hit.url, hit.kind); // direct Spotify page (artist or album) + return allowFallback ? linkResult(name, null, kindHint) : null; // google fallback, or unconfirmed + } catch (e) { + console.error(`resolve "${name}": ${e.message}`); // transient -> caller sees null this time + return null; + } +} + +// Resolve a batch of candidate names to a { name -> link|null } map. Dependencies are injected: +// search(name, kindHint) -> { artist, album } (live Spotify lookup or a fake; required) +// classify(names) -> Map(norm(name) -> "artist"|"album") | null (the optional LLM gate; when +// it returns null — no gate, or LLM down — the Spotify exact-match IS the gate: only names +// actually on Spotify link. When it returns a Map, names absent from it are dropped, names in +// it may use the Google fallback when not on Spotify, and the mapped kind routes the lookup.) +// cache Map for memoizing the search half (optional) +export async function resolveAll(candidates, { search, classify, cache } = {}) { + const uniq = [...new Set(candidates.map((c) => String(c).trim()).filter(Boolean))].slice(0, MAX_CANDIDATES); + + let confirmed = null; + if (classify && uniq.length) { + try { + confirmed = await classify(uniq); + } catch (e) { + console.error(`llm classify: ${e.message}`); // LLM down -> fall back to the Spotify gate + confirmed = null; + } + } + + const out = {}; + await Promise.all( + uniq.map(async (name) => { + if (confirmed && !confirmed.has(norm(name))) { + out[name] = null; // LLM said this is not an artist/album + return; + } + const kindHint = confirmed ? confirmed.get(norm(name)) : undefined; + out[name] = await resolveOne(name, { + search, + cache, + allowFallback: confirmed != null, + kindHint, + }); + }), + ); + return out; +} diff --git a/service/resolver.js b/service/resolver.js index ad2db26..cc5b13e 100644 --- a/service/resolver.js +++ b/service/resolver.js @@ -15,8 +15,9 @@ import { createServer } from "node:http"; import { readFileSync } from "node:fs"; -import { norm, pickArtist, pickAlbum, linkResult } from "./lib.js"; +import { pickArtist, pickAlbum } from "./lib.js"; import { buildClassifyMessages, parseClassification, DEFAULT_MODEL } from "./llm.js"; +import { resolveAll as resolveAllCore } from "./resolve-core.js"; // --- minimal .env loader (no dependency) ------------------------------------ try { @@ -34,10 +35,10 @@ const USE_SPOTIFY = Boolean(SPOTIFY_ID && SPOTIFY_SECRET); const OLLAMA_URL = (process.env.OLLAMA_URL || "").replace(/\/$/, ""); // presence enables the LLM gate const OLLAMA_MODEL = process.env.OLLAMA_MODEL || DEFAULT_MODEL; const USE_LLM = Boolean(OLLAMA_URL); -const MAX_CANDIDATES = 100; -// Cache only the network half (Spotify direct-url lookup); the final link object is rebuilt -// per request so a name's result never depends on a stale gate decision. norm(name) -> url|null +// Cache only the network half (the Spotify direct-url lookup); resolve-core rebuilds the final +// link object per request so a name's result never depends on a stale gate decision. The key +// includes the kind hint (see resolve-core's cacheKey). const spCache = new Map(); // --- Spotify (optional): direct-artist-link lookup -------------------------- @@ -58,36 +59,21 @@ async function spotifyToken() { return spToken; } -// The direct Spotify url for an exact name match across artist AND album, or null (no creds / -// no exact hit). One search call covers both types; `kindHint` ("artist" | "album", from the -// LLM gate) decides which exact hit wins when BOTH match the same string (e.g. a self-titled -// album) — default prefers the artist, matching the artist-first MVP behaviour. Returns -// { url, kind } so the caller can shape an artist vs album link. -async function spotifyDirect(name, kindHint) { - if (!USE_SPOTIFY) return null; +// The injected `search` for resolve-core: one type=artist,album Spotify search per candidate, +// returning the exact-name-match hits as { artist, album } where each is the external Spotify url +// (string) or null. No creds -> both null (nothing links). resolve-core's pickDirect applies the +// kindHint tie-break and shapes the link; the cache (spCache) is owned by resolve-core too. +async function search(name) { + if (!USE_SPOTIFY) return { artist: null, album: null }; const token = await spotifyToken(); const url = `https://api.spotify.com/v1/search?type=artist,album&limit=5&q=${encodeURIComponent(name)}`; const r = await fetch(url, { headers: { authorization: `Bearer ${token}` } }); if (!r.ok) throw new Error(`spotify search http ${r.status}`); const j = await r.json(); - const artist = pickArtist(name, j.artists?.items); - const album = pickAlbum(name, j.albums?.items); - const order = kindHint === "album" ? [["album", album], ["artist", artist]] : [["artist", artist], ["album", album]]; - for (const [kind, hit] of order) { - const u = hit?.external_urls?.spotify; - if (u) return { url: u, kind }; - } - return null; -} - -async function spotifyDirectCached(name, kindHint) { - // Cache key includes the kind hint: the same string can resolve to an artist OR an album - // depending on which the gate confirmed, so a hint-blind cache would cross-contaminate. - const key = `${kindHint || "artist"}:${norm(name)}`; - if (spCache.has(key)) return spCache.get(key); - const hit = await spotifyDirect(name, kindHint); - spCache.set(key, hit); - return hit; + return { + artist: pickArtist(name, j.artists?.items)?.external_urls?.spotify ?? null, + album: pickAlbum(name, j.albums?.items)?.external_urls?.spotify ?? null, + }; } // --- LLM gate (optional): which candidates are artists/albums? -------------- @@ -109,52 +95,11 @@ async function ollamaClassify(candidates) { } // --- resolve -------------------------------------------------------------- -// allowFallback: when the gate has already confirmed this name is an artist/album, a missing -// Spotify direct link still yields a Google->Bandcamp fallback link (instead of null). -// kindHint ("artist" | "album", from the LLM gate or undefined) biases the Spotify lookup and -// the fallback link's kind; the actual exact Spotify hit's kind wins when there is one. -async function resolveOne(name, allowFallback, kindHint) { - if (!norm(name)) return null; - try { - const sp = await spotifyDirectCached(name, kindHint); - if (sp) return linkResult(name, sp.url, sp.kind); // direct Spotify page (artist or album) - return allowFallback ? linkResult(name, null, kindHint) : null; // google fallback, or unconfirmed - } catch (e) { - console.error(`resolve "${name}": ${e.message}`); // transient -> caller sees null this time - return null; - } -} - -async function resolveAll(candidates) { - const uniq = [...new Set(candidates.map((c) => String(c).trim()).filter(Boolean))].slice(0, MAX_CANDIDATES); - - // Gate: which names may link? null => no LLM gate, the Spotify exact-match (artist OR album) - // is the gate (only names actually on Spotify link). A Map(name -> "artist"|"album") => the - // LLM's verdict; names not in it are dropped, names in it may use the Google fallback when - // not on Spotify, and the mapped kind routes the Spotify lookup to the right type. - let confirmed = null; - if (USE_LLM && uniq.length) { - try { - confirmed = await ollamaClassify(uniq); - } catch (e) { - console.error(`llm classify: ${e.message}`); // LLM down -> fall back to the Spotify gate - confirmed = null; - } - } - - const out = {}; - await Promise.all( - uniq.map(async (name) => { - if (confirmed && !confirmed.has(norm(name))) { - out[name] = null; // LLM said this is not an artist/album - return; - } - const kindHint = confirmed ? confirmed.get(norm(name)) : undefined; - out[name] = await resolveOne(name, confirmed != null, kindHint); - }), - ); - return out; -} +// Run the pure orchestration (resolve-core) with the live clients injected: the Spotify `search`, +// the ollama `classify` gate (null when no LLM is configured -> the Spotify exact-match is the +// gate), and the shared spCache. +const resolveAll = (candidates) => + resolveAllCore(candidates, { search, classify: USE_LLM ? ollamaClassify : null, cache: spCache }); // --- HTTP ------------------------------------------------------------------- function readBody(req) { diff --git a/service/resolver.test.js b/service/resolver.test.js new file mode 100644 index 0000000..579db3f --- /dev/null +++ b/service/resolver.test.js @@ -0,0 +1,141 @@ +// Unit tests for the resolve-core orchestration (run with `node --test`). Zero-dep, hermetic: +// the live Spotify/ollama clients are replaced by injected fake search/classify functions, so no +// network and no real timers. Covers the album routing the HTTP server can't unit-test directly. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { resolveAll, resolveOne, pickDirect, cacheKey } from "./resolve-core.js"; + +const ARTIST_URL = "https://open.spotify.com/artist/abc"; +const ALBUM_URL = "https://open.spotify.com/album/xyz"; + +// A fake `search` from a { norm(name) -> { artist, album } } table (urls or null). +const fakeSearch = (table) => (name) => Promise.resolve(table[name.trim().toLowerCase()] ?? { artist: null, album: null }); + +// A fake `classify` returning a Map(norm(name) -> kind) from a plain { name: kind } object. +const fakeClassify = (verdicts) => (names) => { + void names; + return Promise.resolve(new Map(Object.entries(verdicts).map(([n, k]) => [n.toLowerCase(), k]))); +}; + +test("pickDirect tie-break: kindHint='album' prefers the album, else artist-first", () => { + const both = { artist: ARTIST_URL, album: ALBUM_URL }; + assert.deepEqual(pickDirect(both, "album"), { url: ALBUM_URL, kind: "album" }); + assert.deepEqual(pickDirect(both, "artist"), { url: ARTIST_URL, kind: "artist" }); + assert.deepEqual(pickDirect(both, undefined), { url: ARTIST_URL, kind: "artist" }); + // single-sided hits ignore the hint + assert.deepEqual(pickDirect({ artist: ARTIST_URL, album: null }, "album"), { url: ARTIST_URL, kind: "artist" }); + assert.equal(pickDirect({ artist: null, album: null }, "album"), null); +}); + +test("cacheKey separates the same name by kind hint, defaults to artist", () => { + assert.equal(cacheKey("Tool", "album"), "album:tool"); + assert.equal(cacheKey("Tool", "artist"), "artist:tool"); + assert.equal(cacheKey("Tool", undefined), "artist:tool"); +}); + +test("self-titled artist+album tie-break routes to the kindHint'd URL", async () => { + // A self-titled name that is BOTH an exact artist hit and an exact album hit on Spotify. + const search = fakeSearch({ "bad religion": { artist: ARTIST_URL, album: ALBUM_URL } }); + + // kindHint='album' -> the album URL. + const asAlbum = await resolveOne("Bad Religion", { search, kindHint: "album", allowFallback: true }); + assert.equal(asAlbum.kind, "album"); + assert.equal(asAlbum.spotify, ALBUM_URL); + assert.deepEqual(asAlbum.primary, { platform: "spotify", url: ALBUM_URL }); + + // undefined and 'artist' both -> the artist URL (artist-first default). + for (const hint of [undefined, "artist"]) { + const asArtist = await resolveOne("Bad Religion", { search, kindHint: hint, allowFallback: true }); + assert.equal(asArtist.kind, "artist", `hint=${hint}`); + assert.equal(asArtist.spotify, ARTIST_URL); + assert.deepEqual(asArtist.primary, { platform: "spotify", url: ARTIST_URL }); + } +}); + +test("album confirmed by the LLM but absent from Spotify falls back to a Google search", async () => { + // The LLM gate confirms the name is an album; Spotify has no exact hit -> Google fallback. + const search = fakeSearch({}); // nothing on Spotify + const classify = fakeClassify({ "Some Local Release": "album" }); + + const out = await resolveAll(["Some Local Release"], { search, classify }); + const r = out["Some Local Release"]; + assert.ok(r, "confirmed name should resolve, not drop"); + assert.equal(r.kind, "album"); + assert.equal(r.primary.platform, "google"); + assert.match(r.primary.url, /^https:\/\/www\.google\.com\/search\?q=/); + assert.ok(r.primary.url.includes("bandcamp")); + assert.equal(r.spotify, undefined); +}); + +test("LLM confirms artist+album for two names and routes each kind's Spotify lookup", async () => { + const search = fakeSearch({ + devourment: { artist: ARTIST_URL, album: null }, + "reek of putrefaction": { artist: null, album: ALBUM_URL }, + }); + const classify = fakeClassify({ Devourment: "artist", "Reek of Putrefaction": "album" }); + + const out = await resolveAll(["Devourment", "Reek of Putrefaction"], { search, classify }); + assert.equal(out["Devourment"].kind, "artist"); + assert.equal(out["Devourment"].spotify, ARTIST_URL); + assert.equal(out["Reek of Putrefaction"].kind, "album"); + assert.equal(out["Reek of Putrefaction"].spotify, ALBUM_URL); +}); + +test("names the LLM did not confirm are dropped (null)", async () => { + const search = fakeSearch({ "real band": { artist: ARTIST_URL, album: null } }); + const classify = fakeClassify({ "Real Band": "artist" }); // "the best part" not in the map + + const out = await resolveAll(["Real Band", "the best part"], { search, classify }); + assert.ok(out["Real Band"]); + assert.equal(out["the best part"], null); +}); + +test("cache-key kind separation: same name as artist vs album does not cross-contaminate", async () => { + const cache = new Map(); + // One name exists on Spotify as BOTH; the kind hint must keep the two lookups distinct. + const search = fakeSearch({ "self titled": { artist: ARTIST_URL, album: ALBUM_URL } }); + + const asArtist = await resolveOne("Self Titled", { search, cache, kindHint: "artist", allowFallback: true }); + const asAlbum = await resolveOne("Self Titled", { search, cache, kindHint: "album", allowFallback: true }); + + assert.equal(asArtist.spotify, ARTIST_URL); + assert.equal(asArtist.kind, "artist"); + assert.equal(asAlbum.spotify, ALBUM_URL); + assert.equal(asAlbum.kind, "album"); + // Two distinct cache entries, keyed by kind — no cross-contamination. + assert.equal(cache.size, 2); + assert.deepEqual(cache.get("artist:self titled"), { url: ARTIST_URL, kind: "artist" }); + assert.deepEqual(cache.get("album:self titled"), { url: ALBUM_URL, kind: "album" }); +}); + +test("with no LLM gate (classify null) the Spotify exact-match IS the gate; unconfirmed -> null", async () => { + // On Spotify -> links; not on Spotify and no confirmation -> null (no Google fallback). + const search = fakeSearch({ devourment: { artist: ARTIST_URL, album: null } }); + + const out = await resolveAll(["Devourment", "Not A Band"], { search, classify: null }); + assert.equal(out["Devourment"].kind, "artist"); + assert.equal(out["Devourment"].spotify, ARTIST_URL); + assert.equal(out["Not A Band"], null); // not confirmed, not on Spotify -> dropped +}); + +test("LLM-down (classify throws) falls back to the Spotify exact-match gate", async () => { + const search = fakeSearch({ devourment: { artist: ARTIST_URL, album: null } }); + const classify = () => Promise.reject(new Error("ollama down")); + + const out = await resolveAll(["Devourment", "Not A Band"], { search, classify }); + assert.equal(out["Devourment"].spotify, ARTIST_URL); // Spotify hit still links + assert.equal(out["Not A Band"], null); // no fallback once the LLM gate is gone +}); + +test("resolveAll dedups, trims, drops blanks, and caps the candidate list", async () => { + const search = fakeSearch({ tool: { artist: ARTIST_URL, album: null } }); + const out = await resolveAll(["Tool", " Tool ", "", " "], { search, classify: null }); + assert.deepEqual(Object.keys(out), ["Tool"]); // " Tool " collapses to "Tool"; blanks gone + assert.equal(out["Tool"].spotify, ARTIST_URL); +}); + +test("resolveOne swallows a search error and yields null for that name", async () => { + const search = () => Promise.reject(new Error("spotify 503")); + const r = await resolveOne("Tool", { search, allowFallback: true }); + assert.equal(r, null); +});