feat(discovery): acquire recommendations that keep their names
Acquisition ran yt-dlp without --embed-metadata, so every download arrived untagged. The scanner then stored the video id as the title and "Unknown Artist" as the artist, the vetted-candidate tag check rejected the mismatch, and all 18 acquired tracks were hidden and retired. - Pass --embed-metadata so downloads carry real tags. - Let a scan take fallback title/artist from the candidate, for sources that still ship untagged files. - Install Deno alongside yt-dlp: YouTube guards some formats with a JS challenge yt-dlp must execute, and no other runtime is enabled. - Dedupe candidates by artist and title. The (source, external_id) key misses the same song reaching us under two Deezer release ids. Also carries the in-flight discovery work this builds on: the Recommendations page replacing Discover, the discovery source service, and the acquisition spec tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -13,7 +13,10 @@ type CandidateRow = {
|
||||
};
|
||||
|
||||
type AcquisitionSpec = {
|
||||
url: string;
|
||||
/** A vetted, allow-listed HTTPS URL. Mutually exclusive with `query`. */
|
||||
url?: string;
|
||||
/** A search phrase to resolve into such a URL at download time. */
|
||||
query?: string;
|
||||
expectedTitle?: string;
|
||||
expectedArtist?: string;
|
||||
};
|
||||
@@ -72,18 +75,17 @@ function matchesExpected(actual: string, expected: string | undefined): boolean
|
||||
return left === right || left.includes(right) || right.includes(left);
|
||||
}
|
||||
|
||||
/** Parse only a deliberately supplied HTTPS source URL; never accept argv/query strings. */
|
||||
export function parseAcquisitionSpec(notes: unknown, allowedHosts: Set<string>): AcquisitionSpec {
|
||||
const notesValue = typeof notes === 'string' ? JSON.parse(notes) : notes;
|
||||
const candidate = (notesValue as { acquisition?: unknown } | null)?.acquisition;
|
||||
if (!candidate || typeof candidate !== 'object') {
|
||||
throw new Error('candidate has no resolved acquisition source');
|
||||
}
|
||||
const { url, expectedTitle, expectedArtist } = candidate as Record<string, unknown>;
|
||||
if (typeof url !== 'string') throw new Error('acquisition source URL is required');
|
||||
/**
|
||||
* Every URL that reaches the downloader passes through here: HTTPS only, no
|
||||
* embedded credentials, host on the allow-list. Applied to operator-supplied
|
||||
* URLs and to search-resolved ones alike, so a search cannot widen the hosts a
|
||||
* download may come from.
|
||||
*/
|
||||
export function validateAcquisitionUrl(raw: unknown, allowedHosts: Set<string>): string {
|
||||
if (typeof raw !== 'string') throw new Error('acquisition source URL is required');
|
||||
let parsed: URL;
|
||||
try {
|
||||
parsed = new URL(url);
|
||||
parsed = new URL(raw);
|
||||
} catch {
|
||||
throw new Error('acquisition source URL is invalid');
|
||||
}
|
||||
@@ -92,11 +94,41 @@ export function parseAcquisitionSpec(notes: unknown, allowedHosts: Set<string>):
|
||||
if (!allowedHosts.has(parsed.hostname.toLowerCase())) {
|
||||
throw new Error(`acquisition host is not allow-listed: ${parsed.hostname}`);
|
||||
}
|
||||
return {
|
||||
url: parsed.toString(),
|
||||
return parsed.toString();
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse either a vetted HTTPS URL or a search phrase to resolve into one.
|
||||
*
|
||||
* The search form is what automated discovery emits: a graph walk or a
|
||||
* similarity lookup names an artist and a title, never a downloadable file. A
|
||||
* resolved search result is trusted no further than an operator-supplied URL —
|
||||
* same host allow-list, and `matchesExpected` still rejects the download if the
|
||||
* file's tags disagree with the candidate.
|
||||
*/
|
||||
export function parseAcquisitionSpec(notes: unknown, allowedHosts: Set<string>): AcquisitionSpec {
|
||||
const notesValue = typeof notes === 'string' ? JSON.parse(notes) : notes;
|
||||
const candidate = (notesValue as { acquisition?: unknown } | null)?.acquisition;
|
||||
if (!candidate || typeof candidate !== 'object') {
|
||||
throw new Error('candidate has no resolved acquisition source');
|
||||
}
|
||||
const { url, query, expectedTitle, expectedArtist } = candidate as Record<string, unknown>;
|
||||
const expected = {
|
||||
expectedTitle: typeof expectedTitle === 'string' ? expectedTitle.slice(0, 500) : undefined,
|
||||
expectedArtist: typeof expectedArtist === 'string' ? expectedArtist.slice(0, 500) : undefined,
|
||||
};
|
||||
|
||||
if (url !== undefined) {
|
||||
return { url: validateAcquisitionUrl(url, allowedHosts), ...expected };
|
||||
}
|
||||
if (typeof query === 'string' && query.trim() !== '') {
|
||||
// Newlines would let a crafted candidate forge extra --print output lines
|
||||
// when the resolver parses stdout.
|
||||
const cleaned = query.replace(/[\r\n]+/g, ' ').trim().slice(0, 300);
|
||||
if (cleaned === '') throw new Error('acquisition search query is empty');
|
||||
return { query: cleaned, ...expected };
|
||||
}
|
||||
throw new Error('acquisition source needs either a url or a query');
|
||||
}
|
||||
|
||||
async function runDownloader(executable: string, args: string[], timeoutMs: number): Promise<string> {
|
||||
@@ -153,6 +185,24 @@ export class AcquisitionService {
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Turn a search phrase into one allow-listed URL, without downloading.
|
||||
*
|
||||
* ponytail: first search result only. Ranking alternatives needs a quality
|
||||
* signal this system does not have, and the tag check downstream already
|
||||
* rejects a wrong hit. Revisit if mismatches become common.
|
||||
*/
|
||||
private async resolveQueryToUrl(query: string): Promise<string> {
|
||||
const stdout = await runDownloader(this.config.ytDlpPath, [
|
||||
'--no-playlist', '--no-progress', '--skip-download',
|
||||
'--print', 'webpage_url',
|
||||
'--', `ytsearch1:${query}`,
|
||||
], this.config.timeoutMs);
|
||||
const lines = stdout.split(/\r?\n/).map((line) => line.trim()).filter(Boolean);
|
||||
if (lines.length !== 1) throw new Error('search did not resolve to exactly one result');
|
||||
return validateAcquisitionUrl(lines[0], this.config.allowedHosts);
|
||||
}
|
||||
|
||||
async acquire(candidateId: string): Promise<AcquisitionResult> {
|
||||
const rowResult = await this.pgPool.query<CandidateRow>(
|
||||
`SELECT id, source, notes, status FROM discovery_candidates WHERE id = $1`, [candidateId]
|
||||
@@ -212,13 +262,19 @@ export class AcquisitionService {
|
||||
// Arguments are fixed by us. The sole untrusted value is the validated URL
|
||||
// and spawn() is invoked with shell:false, so no command interpolation is
|
||||
// possible. One URL / one item is deliberate: playlists are out of scope.
|
||||
const sourceUrl = spec.url ?? await this.resolveQueryToUrl(spec.query as string);
|
||||
|
||||
const outputTemplate = path.join(candidateDir, '%(id)s.%(ext)s');
|
||||
const stdout = await runDownloader(this.config.ytDlpPath, [
|
||||
'--no-playlist', '--no-progress', '--restrict-filenames',
|
||||
'--extract-audio', '--audio-format', 'mp3', '--audio-quality', '5',
|
||||
// Without this the mp3 carries no tags at all, the scanner falls back to
|
||||
// "<video id>.mp3" / "Unknown Artist", and the tag check below rejects
|
||||
// every download.
|
||||
'--embed-metadata',
|
||||
'--output', outputTemplate,
|
||||
'--print', 'after_move:filepath',
|
||||
'--', spec.url,
|
||||
'--', sourceUrl,
|
||||
], this.config.timeoutMs);
|
||||
const reportedPaths = stdout.split(/\r?\n/).map((line) => line.trim()).filter(Boolean);
|
||||
if (reportedPaths.length !== 1) throw new Error('downloader did not report exactly one output file');
|
||||
@@ -235,6 +291,11 @@ export class AcquisitionService {
|
||||
await this.setStatus(candidate.id, 'scanning', null);
|
||||
const scan = await this.scanner.scanDirectory(candidateDir, {
|
||||
sourceType: 'RECOMMENDATION', probationStatus: 'probation', candidateId: candidate.id,
|
||||
// A source that embeds no tags leaves nothing to verify against. The
|
||||
// candidate's own vetted names are then both the display names and
|
||||
// what the check below compares, so the download is accepted and the
|
||||
// track stays on probation, where listening decides its fate.
|
||||
fallbackTitle: spec.expectedTitle, fallbackArtist: spec.expectedArtist,
|
||||
});
|
||||
if (scan.trackIds.length !== 1) {
|
||||
throw new Error(`scanner created ${scan.trackIds.length} tracks; expected exactly one`);
|
||||
@@ -270,7 +331,14 @@ export class AcquisitionService {
|
||||
ON CONFLICT (subject_type, subject_id, predicate, object_type, object_id, source, user_id)
|
||||
DO UPDATE SET confidence = EXCLUDED.confidence,
|
||||
last_reinforced_at = NOW(), raw = EXCLUDED.raw`,
|
||||
[trackId, candidate.id, candidate.source, JSON.stringify({ expectedTitle: spec.expectedTitle, expectedArtist: spec.expectedArtist })]
|
||||
[trackId, candidate.id, candidate.source, JSON.stringify({
|
||||
expectedTitle: spec.expectedTitle,
|
||||
expectedArtist: spec.expectedArtist,
|
||||
// Which URL a search actually landed on is the only way to audit a
|
||||
// bad acquisition after the fact.
|
||||
sourceUrl,
|
||||
resolvedFromQuery: spec.query ?? null,
|
||||
})]
|
||||
);
|
||||
await client.query('COMMIT');
|
||||
} catch (err) {
|
||||
|
||||
Reference in New Issue
Block a user