address audit findings: subtitle rescan decisions, scan limit, parseId, setup gate

worked through AUDIT.md. triage:
- finding 2 (subtitle rescan wipes decisions): confirmed. /:id/rescan now
  snapshots custom_titles and calls reanalyze() after the stream delete/
  insert, mirroring the review rescan flow. exported reanalyze + titleKey
  from review.ts so both routes share the logic.
- finding 3 (scan limit accepts NaN/negatives): confirmed. extracted
  parseScanLimit into a pure helper, added unit tests covering NaN,
  negatives, floats, infinity, numeric strings. invalid input 400s and
  releases the scan_running lock.
- finding 4 (parseId lenient): confirmed. tightened the regex to /^\d+$/
  so "42abc", "abc42", "+42", "42.0" all return null. rewrote the test
  that codified the old lossy behaviour.
- finding 5 (setup_complete set before jellyfin test passes): confirmed.
  the /jellyfin endpoint still persists url+key unconditionally, but now
  only flips setup_complete=1 on a successful connection test.
- finding 6 (swallowed errors): partial. the mqtt restart and version-
  fetch swallows are intentional best-effort with downstream surfaces
  (getMqttStatus, UI fallback). only the scan.ts db-update swallow was
  a real visibility gap — logs via logError now.
- finding 1 (auth): left as-is. redacting secrets on GET without auth
  on POST is security theater; real fix is an auth layer, which is a
  design decision not a bugfix. audit removed from the tree.
- lint fail on ffmpeg.test.ts: formatted.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

package.json

@@ -1,6 +1,6 @@
 {
 	"name": "netfelix-audio-fix",
-	"version": "2026.04.14.16",
+	"version": "2026.04.14.17",
 	"scripts": {
 		"dev:server": "NODE_ENV=development bun --hot server/index.tsx",
 		"dev:client": "vite",

server/api/__tests__/scan.test.ts

@@ -0,0 +1,31 @@
+import { describe, expect, test } from "bun:test";
+import { parseScanLimit } from "../scan";
+
+describe("parseScanLimit", () => {
+	test("accepts positive integers and nullish/empty as no-limit", () => {
+		expect(parseScanLimit(5)).toEqual({ ok: true, value: 5 });
+		expect(parseScanLimit(1)).toEqual({ ok: true, value: 1 });
+		expect(parseScanLimit(10_000)).toEqual({ ok: true, value: 10_000 });
+		expect(parseScanLimit(null)).toEqual({ ok: true, value: null });
+		expect(parseScanLimit(undefined)).toEqual({ ok: true, value: null });
+		expect(parseScanLimit("")).toEqual({ ok: true, value: null });
+	});
+
+	test("coerces numeric strings (env var path) but rejects garbage", () => {
+		expect(parseScanLimit("7")).toEqual({ ok: true, value: 7 });
+		expect(parseScanLimit("abc")).toEqual({ ok: false });
+		expect(parseScanLimit("12abc")).toEqual({ ok: false });
+	});
+
+	test("rejects the footguns that would silently disable the cap", () => {
+		// NaN: processed >= NaN never trips → cap never fires.
+		expect(parseScanLimit(Number.NaN)).toEqual({ ok: false });
+		// Negative: off-by-one bugs in Math.min(limit, total).
+		expect(parseScanLimit(-1)).toEqual({ ok: false });
+		expect(parseScanLimit(0)).toEqual({ ok: false });
+		// Float: Math.min is fine but percentage math breaks on non-integers.
+		expect(parseScanLimit(1.5)).toEqual({ ok: false });
+		// Infinity is technically a number but has no business as a cap.
+		expect(parseScanLimit(Number.POSITIVE_INFINITY)).toEqual({ ok: false });
+	});
+});

server/api/review.ts

@@ -130,11 +130,16 @@ function loadItemDetail(db: ReturnType<typeof getDb>, itemId: number) {
  * composite of (type, language, stream_index, title) so user overrides
  * survive stream-id changes when Jellyfin re-probes metadata.
  */
-function titleKey(s: { type: string; language: string | null; stream_index: number; title: string | null }): string {
+export function titleKey(s: {
+	type: string;
+	language: string | null;
+	stream_index: number;
+	title: string | null;
+}): string {
 	return `${s.type}|${s.language ?? ""}|${s.stream_index}|${s.title ?? ""}`;
 }
 
-function reanalyze(db: ReturnType<typeof getDb>, itemId: number, preservedTitles?: Map<string, string>): void {
+export function reanalyze(db: ReturnType<typeof getDb>, itemId: number, preservedTitles?: Map<string, string>): void {
 	const item = db.prepare("SELECT * FROM media_items WHERE id = ?").get(itemId) as MediaItem;
 	if (!item) return;
 

server/api/scan.ts

@@ -10,6 +10,19 @@ import { loadLibrary as loadSonarrLibrary, isUsable as sonarrUsable } from "../s
 
 const app = new Hono();
 
+/**
+ * Validate a scan `limit` input. Must be a positive integer or absent —
+ * NaN/negatives/non-numerics would disable the progress cap
+ * (`processed >= NaN` never trips) or produce bogus totals via
+ * `Math.min(NaN, …)`. Exported for unit tests.
+ */
+export function parseScanLimit(raw: unknown): { ok: true; value: number | null } | { ok: false } {
+	if (raw == null || raw === "") return { ok: true, value: null };
+	const n = typeof raw === "number" ? raw : Number(raw);
+	if (!Number.isInteger(n) || n <= 0) return { ok: false };
+	return { ok: true, value: n };
+}
+
 // ─── State ────────────────────────────────────────────────────────────────────
 
 let scanAbort: AbortController | null = null;
@@ -63,10 +76,14 @@ app.post("/start", async (c) => {
 		return c.json({ ok: false, error: "Scan already running" }, 409);
 	}
 
-	const body = await c.req.json<{ limit?: number }>().catch(() => ({ limit: undefined }));
-	const formLimit = body.limit ?? null;
-	const envLimit = process.env.SCAN_LIMIT ? Number(process.env.SCAN_LIMIT) : null;
-	const limit = formLimit ?? envLimit ?? null;
+	const body = await c.req.json<{ limit?: unknown }>().catch(() => ({ limit: undefined }));
+	const formLimit = parseScanLimit(body.limit);
+	const envLimit = parseScanLimit(process.env.SCAN_LIMIT);
+	if (!formLimit.ok || !envLimit.ok) {
+		db.prepare("UPDATE config SET value = '0' WHERE key = 'scan_running'").run();
+		return c.json({ ok: false, error: "limit must be a positive integer" }, 400);
+	}
+	const limit: number | null = formLimit.value ?? envLimit.value ?? null;
 	setConfig("scan_limit", limit != null ? String(limit) : "");
 
 	runScan(limit).catch((err) => {
@@ -239,8 +256,11 @@ async function runScan(limit: number | null = null): Promise<void> {
 				db
 					.prepare("UPDATE media_items SET scan_status = 'error', scan_error = ? WHERE jellyfin_id = ?")
 					.run(String(err), jellyfinItem.Id);
-			} catch {
-				/* ignore */
+			} catch (dbErr) {
+				// Failed to persist the error status — log it so the incident
+				// doesn't disappear silently. We can't do much more; the outer
+				// loop continues so the scan still finishes.
+				logError(`Failed to record scan error for ${jellyfinItem.Id}:`, dbErr);
 			}
 			emitSse("log", { name: jellyfinItem.Name, type: jellyfinItem.Type, status: "error", file: jellyfinItem.Path });
 		}

server/api/settings.ts

@@ -26,12 +26,15 @@ app.post("/jellyfin", async (c) => {
 	// { ok, saved, testError } shape to decide what message to show.
 	setConfig("jellyfin_url", url);
 	setConfig("jellyfin_api_key", apiKey);
-	setConfig("setup_complete", "1");
 
 	const result = await testJellyfin({ url, apiKey });
 
-	// Best-effort admin discovery only when the connection works; ignore failures.
+	// Only mark setup complete when the connection actually works. Setting
+	// setup_complete=1 on a failing test would let the user click past the
+	// wizard into an app that then dies on the first Jellyfin call.
 	if (result.ok) {
+		setConfig("setup_complete", "1");
+		// Best-effort admin discovery only when the connection works; ignore failures.
 		try {
 			const users = await getUsers({ url, apiKey });
 			const admin = users.find((u) => u.Name === "admin") ?? users[0];

server/api/subtitles.ts

@@ -6,6 +6,7 @@ import { error as logError } from "../lib/log";
 import { parseId } from "../lib/validate";
 import { getItem, mapStream, normalizeLanguage, refreshItem } from "../services/jellyfin";
 import type { MediaItem, MediaStream, ReviewPlan, StreamDecision, SubtitleFile } from "../types";
+import { reanalyze, titleKey } from "./review";
 
 const app = new Hono();
 
@@ -405,6 +406,30 @@ app.post("/:id/rescan", async (c) => {
 
 	await refreshItem(jfCfg, item.jellyfin_id);
 
+	// Snapshot custom_titles before the DELETE cascades stream_decisions away,
+	// so reanalyze() can re-attach them to the corresponding new stream rows.
+	// Without this rescanning subtitles also wipes per-audio-stream title
+	// overrides the user made in the review UI.
+	const preservedTitles = new Map<string, string>();
+	const oldTitleRows = db
+		.prepare(`
+			SELECT ms.type, ms.language, ms.stream_index, ms.title, sd.custom_title
+			FROM stream_decisions sd
+			JOIN media_streams ms ON ms.id = sd.stream_id
+			JOIN review_plans rp ON rp.id = sd.plan_id
+			WHERE rp.item_id = ? AND sd.custom_title IS NOT NULL
+		`)
+		.all(id) as {
+		type: string;
+		language: string | null;
+		stream_index: number;
+		title: string | null;
+		custom_title: string;
+	}[];
+	for (const r of oldTitleRows) {
+		preservedTitles.set(titleKey(r), r.custom_title);
+	}
+
 	const fresh = await getItem(jfCfg, item.jellyfin_id);
 	if (fresh) {
 		const insertStream = db.prepare(`
@@ -412,6 +437,10 @@ app.post("/:id/rescan", async (c) => {
 				title, is_default, is_forced, is_hearing_impaired, channels, channel_layout, bit_rate, sample_rate)
 			VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
 		`);
+		// DELETE cascades to stream_decisions via FK. reanalyze() below
+		// rebuilds them from the fresh streams; without it the plan would
+		// keep status='done'/'approved' but reference zero decisions, and
+		// ffmpeg would emit a no-op command.
 		db.prepare("DELETE FROM media_streams WHERE item_id = ?").run(id);
 		for (const jStream of fresh.MediaStreams ?? []) {
 			if (jStream.IsExternal) continue;
@@ -435,6 +464,8 @@ app.post("/:id/rescan", async (c) => {
 		}
 	}
 
+	reanalyze(db, id, preservedTitles);
+
 	const detail = loadDetail(db, id);
 	if (!detail) return c.notFound();
 	return c.json(detail);

server/lib/__tests__/validate.test.ts

@@ -15,8 +15,12 @@ describe("parseId", () => {
 		expect(parseId(undefined)).toBe(null);
 	});
 
-	test("parses leading integer from mixed strings (parseInt semantics)", () => {
-		expect(parseId("42abc")).toBe(42);
+	test("rejects mixed alphanumeric strings (strict — route params must be wholly numeric)", () => {
+		expect(parseId("42abc")).toBe(null);
+		expect(parseId("abc42")).toBe(null);
+		expect(parseId("42 ")).toBe(null);
+		expect(parseId("+42")).toBe(null);
+		expect(parseId("42.0")).toBe(null);
 	});
 });
 

server/lib/validate.ts

@@ -1,8 +1,12 @@
 import type { Context } from "hono";
 
-/** Parse a route param as a positive integer id. Returns null if invalid. */
+/**
+ * Parse a route param as a positive integer id. Returns null if invalid.
+ * Strict: rejects mixed strings like "42abc" that Number.parseInt would
+ * accept — route params must be wholly numeric or the request is bad.
+ */
 export function parseId(raw: string | undefined): number | null {
-	if (!raw) return null;
+	if (!raw || !/^\d+$/.test(raw)) return null;
 	const n = Number.parseInt(raw, 10);
 	return Number.isFinite(n) && n > 0 ? n : null;
 }

server/services/__tests__/ffmpeg.test.ts

@@ -185,7 +185,15 @@ describe("buildCommand", () => {
 	test("writes canonical 'ENG - CODEC · CHANNELS' title on every kept audio stream", () => {
 		const streams = [
 			stream({ id: 1, type: "Video", stream_index: 0 }),
-			stream({ id: 2, type: "Audio", stream_index: 1, codec: "ac3", channels: 6, language: "eng", title: "Audio Description" }),
+			stream({
+				id: 2,
+				type: "Audio",
+				stream_index: 1,
+				codec: "ac3",
+				channels: 6,
+				language: "eng",
+				title: "Audio Description",
+			}),
 			stream({ id: 3, type: "Audio", stream_index: 2, codec: "dts", channels: 1, language: "deu" }),
 			stream({ id: 4, type: "Audio", stream_index: 3, codec: "aac", channels: 2, language: null }),
 		];