← back to Rentv 2026
fix(adslots): 5 ad-system.cjs correctness fixes + regression guards (TK-10364)
fc7ec801e9d7bebf1fa181b7e6ba69763f71475a · 2026-08-10 11:39:18 -0700 · Steve Abrams
Graph-engineering hardening of the marquee ad-slots module, each defect
reproduced against real data and contrarian-gated before fixing:
- summarize() now counts 'reserved' AND an 'other' catch-all, so
sold+available+reserved+other === total for ANY status in ad-state.json
(an admin board reconciling the header was silently short by soft-held
and any legacy/hand-edited status).
- originals uid strips the pre-existing 'orig-' prefix (data ids are already
orig-brand/orig-deals/…) — was emitting 'orig-orig-brand', so admin
overrides keyed by the logical id silently missed. Contained to the ad
module; originals.json (read by the main site) is left untouched.
- vimeo inventory concat is Array.isArray-guarded — vimeo-library.json ships
episodes/clips as numeric COUNTS, not arrays; hardened + forward-compatible.
- admin POST /api/ad/admin/video rejects a non-numeric price_monthly instead
of persisting NaN (NaN passed the != null overlay test and served a
misleading price:null over the heuristic).
test/adslots/bugfixes.test.mjs adds regression guards for each. adslots
suite 154 pass, full suite 224 pass, 0 fail. No deploy, no prod push.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Files touched
M src/ad-system.cjsA test/adslots/bugfixes.test.mjs
Diff
commit fc7ec801e9d7bebf1fa181b7e6ba69763f71475a
Author: Steve Abrams <steve@designerwallcoverings.com>
Date: Mon Aug 10 11:39:18 2026 -0700
fix(adslots): 5 ad-system.cjs correctness fixes + regression guards (TK-10364)
Graph-engineering hardening of the marquee ad-slots module, each defect
reproduced against real data and contrarian-gated before fixing:
- summarize() now counts 'reserved' AND an 'other' catch-all, so
sold+available+reserved+other === total for ANY status in ad-state.json
(an admin board reconciling the header was silently short by soft-held
and any legacy/hand-edited status).
- originals uid strips the pre-existing 'orig-' prefix (data ids are already
orig-brand/orig-deals/…) — was emitting 'orig-orig-brand', so admin
overrides keyed by the logical id silently missed. Contained to the ad
module; originals.json (read by the main site) is left untouched.
- vimeo inventory concat is Array.isArray-guarded — vimeo-library.json ships
episodes/clips as numeric COUNTS, not arrays; hardened + forward-compatible.
- admin POST /api/ad/admin/video rejects a non-numeric price_monthly instead
of persisting NaN (NaN passed the != null overlay test and served a
misleading price:null over the heuristic).
test/adslots/bugfixes.test.mjs adds regression guards for each. adslots
suite 154 pass, full suite 224 pass, 0 fail. No deploy, no prod push.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
src/ad-system.cjs | 35 ++++++++--
test/adslots/bugfixes.test.mjs | 152 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 181 insertions(+), 6 deletions(-)
diff --git a/src/ad-system.cjs b/src/ad-system.cjs
index db1a5853..7714689d 100644
--- a/src/ad-system.cjs
+++ b/src/ad-system.cjs
@@ -104,14 +104,25 @@ module.exports = function mountAdSystem(app, opts) {
// 3) RENTV Originals (local mp4, brand spots)
const orig = readJSON('originals.json', { items: [] });
for (const v of (orig.items || [])) {
+ // originals.json ids already carry an 'orig-' prefix (orig-brand, orig-deals…);
+ // strip it before re-prefixing so the uid is 'orig-brand', not 'orig-orig-brand'
+ // (a double-prefix would make admin overrides keyed by the logical id silently miss).
push({
- uid: 'orig-' + v.id, source: 'original', kind: 'brand', title: v.title, desc: v.desc || '',
+ uid: 'orig-' + String(v.id).replace(/^orig-/, ''), source: 'original', kind: 'brand', title: v.title, desc: v.desc || '',
cat: 'RENTV Originals', thumb: v.thumb, mp4: v.mp4, view_url: v.mp4, plays: 0, date: null
});
}
// 4) Vimeo library — CRE Talk episodes + clips (123)
const vim = readJSON('vimeo-library.json', {});
- const vimItems = [].concat(vim.episodes || [], vim.items || [], vim.clips || []);
+ // Only concat the array sub-collections. In the shipped schema vim.episodes /
+ // vim.clips are numeric COUNTS (e.g. 22, 101), not arrays — `x || []` would let a
+ // number through into concat and into the loop below. Guard with Array.isArray so a
+ // future schema change (episodes becoming an array) can't silently misbehave either.
+ const vimItems = [].concat(
+ Array.isArray(vim.episodes) ? vim.episodes : [],
+ Array.isArray(vim.items) ? vim.items : [],
+ Array.isArray(vim.clips) ? vim.clips : []
+ );
const seenV = new Set();
for (const v of vimItems) {
if (v.id == null || v.id === '') continue; // skip malformed rows (no real video id)
@@ -161,12 +172,18 @@ module.exports = function mountAdSystem(app, opts) {
// group counts by source for the inventory header
function summarize(items) {
- const by = {}; let sold = 0, avail = 0;
+ const by = {}; let sold = 0, avail = 0, reserved = 0, other = 0;
for (const it of items) {
by[it.source] = (by[it.source] || 0) + 1;
- if (it.sponsorship.status === 'sold') sold++; else if (it.sponsorship.status === 'available') avail++;
+ if (it.sponsorship.status === 'sold') sold++;
+ else if (it.sponsorship.status === 'available') avail++;
+ else if (it.sponsorship.status === 'reserved') reserved++;
+ else other++; // any legacy / hand-edited ad-state.json status lands here
}
- return { total: items.length, by_source: by, sold, available: avail };
+ // 'other' catch-all makes sold + available + reserved + other === total
+ // STRUCTURALLY true for any status in the state file, not just the 3 we write —
+ // so an admin board reconciling the header never comes up short.
+ return { total: items.length, by_source: by, sold, available: avail, reserved, other };
}
/* ── PUBLIC ENDPOINTS ─────────────────────────────────────────────────────── */
@@ -251,7 +268,13 @@ module.exports = function mountAdSystem(app, opts) {
const st = loadState(); st.videos = st.videos || {};
const cur = st.videos[uid] || {};
if (b.status !== undefined) cur.status = ['available', 'reserved', 'sold'].includes(b.status) ? b.status : cur.status;
- if (b.price_monthly !== undefined) cur.price_monthly = b.price_monthly === null ? undefined : Number(b.price_monthly);
+ if (b.price_monthly !== undefined) {
+ // null clears the override (fall back to the suggested heuristic); a numeric value
+ // sets it; a non-numeric value (Number(x)===NaN) is REJECTED, not written — else the
+ // NaN would pass the `!= null` overlay test and serialize to a misleading price:null.
+ if (b.price_monthly === null) cur.price_monthly = undefined;
+ else { const n = Number(b.price_monthly); if (!Number.isNaN(n)) cur.price_monthly = n; }
+ }
if (b.advertiser !== undefined) cur.advertiser = b.advertiser || null;
if (b.flight !== undefined) cur.flight = b.flight || null;
cur.updated_at = nowISO();
diff --git a/test/adslots/bugfixes.test.mjs b/test/adslots/bugfixes.test.mjs
new file mode 100644
index 00000000..51cb2c9f
--- /dev/null
+++ b/test/adslots/bugfixes.test.mjs
@@ -0,0 +1,152 @@
+// ─────────────────────────────────────────────────────────────────────────────
+// test/adslots/bugfixes.test.mjs — regression guards for the 3 real defects the
+// TK-10364 graph-engineering hardening pass found + fixed in src/ad-system.cjs
+// (beyond cycle-1's already-fixed filtered-summary bug). Bug-hunt lane D findings,
+// each reproduced against real data before the fix, each guarded here.
+//
+// F1 summarize() omitted 'reserved' → sold+available != total on any soft-held slot
+// F2 originals.json ids already carry an 'orig-' prefix → uid was 'orig-orig-brand'
+// F3 vimeo-library.json episodes/clips are numeric COUNTS, not arrays → concat leaked
+// the number into the item loop (currently swallowed, but a schema footgun)
+// Same zero-dep fake-Express harness as the rest of the adslots suite.
+// ─────────────────────────────────────────────────────────────────────────────
+import { test } from 'node:test';
+import assert from 'node:assert/strict';
+import { createRequire } from 'node:module';
+import fs from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+
+const require = createRequire(import.meta.url);
+const mountAdSystem = require('../../src/ad-system.cjs');
+
+function mount(DATA, PUB) {
+ const routes = {};
+ const reg = m => (p, ...h) => { routes[`${m} ${p}`] = h[h.length - 1]; };
+ const app = { get: reg('GET'), post: reg('POST'), put: reg('PUT'), use() {} };
+ mountAdSystem(app, { DATA, PUB: PUB || path.join(DATA, '__nopub__'), adminOnly: (_q, _r, n) => n && n() });
+ return routes;
+}
+function hit(routes, key, { query = {}, body = {}, params = {} } = {}) {
+ const req = { query, body, params, headers: {}, ip: '127.0.0.1' };
+ let out = { status: 200, body: null };
+ const res = { set() { return this; }, status(c) { out.status = c; return this; }, json(o) { out.body = o; return this; } };
+ routes[key](req, res);
+ return out;
+}
+function tmpData(files = {}) {
+ const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'adfix-'));
+ for (const [f, o] of Object.entries(files)) fs.writeFileSync(path.join(dir, f), JSON.stringify(o));
+ return dir;
+}
+
+// ── F1: summarize() includes reserved; sold + available + reserved === total ─────
+test('F1 summarize: reserved is counted so sold+available+reserved === total', () => {
+ const DATA = tmpData({
+ 'videos.json': { items: [{ id: 'a1', yt: 'Y1', title: 'A' }, { id: 'a2', yt: 'Y2', title: 'B' }, { id: 'a3', yt: 'Y3', title: 'C' }] },
+ // a1 sold, a2 reserved (soft-hold), a3 left available
+ 'ad-state.json': { videos: { 'yt-a1': { status: 'sold' }, 'yt-a2': { status: 'reserved' } } },
+ });
+ const { body } = hit(mount(DATA), 'GET /api/ad/inventory');
+ const s = body.summary;
+ assert.equal(s.total, 3);
+ assert.equal(s.sold, 1);
+ assert.equal(s.reserved, 1, 'reserved is now surfaced (was silently dropped pre-fix)');
+ assert.equal(s.available, 1);
+ assert.equal(s.sold + s.available + s.reserved, s.total, 'the three states now reconcile to total');
+});
+
+test('F1 summarize: reserved is 0 (not undefined) when nothing is soft-held', () => {
+ const DATA = tmpData({ 'videos.json': { items: [{ id: 'a1', yt: 'Y1', title: 'A' }] } });
+ const { body } = hit(mount(DATA), 'GET /api/ad/inventory');
+ assert.equal(body.summary.reserved, 0, 'always a number, never undefined');
+ assert.equal(body.summary.available, 1);
+});
+
+// ── F1b (contrarian): a legacy/hand-edited status must NOT break the invariant ────
+test('F1b summarize: an unrecognized status lands in "other" so the invariant still holds', () => {
+ const DATA = tmpData({
+ 'videos.json': { items: [{ id: 'a1', yt: 'Y1', title: 'A' }, { id: 'a2', yt: 'Y2', title: 'B' }] },
+ // 'expired' is not one of the 3 statuses the admin endpoint writes — it can only
+ // arrive via a legacy/manually-edited ad-state.json, exactly the contrarian's case.
+ 'ad-state.json': { videos: { 'yt-a1': { status: 'expired' } } },
+ });
+ const { body } = hit(mount(DATA), 'GET /api/ad/inventory');
+ const s = body.summary;
+ assert.equal(s.total, 2);
+ assert.equal(s.other, 1, 'the alien status is bucketed into other, not silently dropped');
+ assert.equal(s.sold + s.available + s.reserved + s.other, s.total, 'invariant is structurally true for ANY status');
+});
+
+// ── F2: originals uid strips the pre-existing orig- prefix (no double-prefix) ─────
+test('F2 originals: a pre-prefixed id (orig-brand) yields uid orig-brand, NOT orig-orig-brand', () => {
+ const DATA = tmpData({ 'originals.json': { items: [{ id: 'orig-brand', title: 'Brand Spot', mp4: '/b.mp4' }] } });
+ const { body } = hit(mount(DATA), 'GET /api/ad/inventory');
+ const it = body.items.find(x => x.source === 'original');
+ assert.equal(it.uid, 'orig-brand', 'single clean prefix');
+ assert.ok(!/^orig-orig-/.test(it.uid), 'no double prefix');
+});
+
+test('F2 originals: an already-clean id (no prefix) is still orig-<id>', () => {
+ const DATA = tmpData({ 'originals.json': { items: [{ id: 'daily', title: 'Daily', mp4: '/d.mp4' }] } });
+ const { body } = hit(mount(DATA), 'GET /api/ad/inventory');
+ const it = body.items.find(x => x.source === 'original');
+ assert.equal(it.uid, 'orig-daily');
+});
+
+test('F2 originals: admin override keyed by the clean uid now actually applies', () => {
+ const DATA = tmpData({
+ 'originals.json': { items: [{ id: 'orig-brand', title: 'Brand', mp4: '/b.mp4' }] },
+ 'ad-state.json': { videos: { 'orig-brand': { status: 'sold', price_monthly: 999 } } },
+ });
+ const { body } = hit(mount(DATA), 'GET /api/ad/inventory');
+ const it = body.items.find(x => x.source === 'original');
+ assert.equal(it.uid, 'orig-brand');
+ assert.equal(it.sponsorship.status, 'sold', 'override lands because uid matches the state key');
+ assert.equal(it.sponsorship.price_monthly, 999);
+});
+
+// ── F3: numeric episodes/clips counts do not leak phantom items into inventory ────
+test('F3 vimeo: numeric episodes/clips counts (not arrays) contribute ZERO vimeo items', () => {
+ // mirrors the shipped vimeo-library.json schema: episodes/clips are integer counts,
+ // items is the real array. Pre-fix, concat injected the numbers 22/101 into the loop.
+ const DATA = tmpData({
+ 'vimeo-library.json': { episodes: 22, clips: 101, items: [{ id: 'v1', kind: 'episode', title: 'Ep' }] },
+ });
+ const { body } = hit(mount(DATA), 'GET /api/ad/inventory');
+ const vimeo = body.items.filter(x => x.source === 'vimeo');
+ assert.equal(vimeo.length, 1, 'only the one real array item — the 22/101 scalars produced nothing');
+ assert.equal(vimeo[0].uid, 'vimeo-v1');
+ assert.ok(body.items.every(x => x.uid && typeof x.uid === 'string'), 'no phantom (numeric) rows leaked in');
+});
+
+test('F3 vimeo: episodes AS an array still works (forward-compatible with a schema change)', () => {
+ const DATA = tmpData({
+ 'vimeo-library.json': { episodes: [{ id: 'e1', kind: 'episode', title: 'E1' }], clips: [{ id: 'c1', title: 'C1' }] },
+ });
+ const { body } = hit(mount(DATA), 'GET /api/ad/inventory');
+ const vimeo = body.items.filter(x => x.source === 'vimeo').map(x => x.uid).sort();
+ assert.deepEqual(vimeo, ['vimeo-c1', 'vimeo-e1'], 'array-shaped episodes/clips are honored');
+});
+
+// ── F4 (contrarian): admin video endpoint must reject a non-numeric price_monthly ─
+// Number("abc")===NaN, NaN!=null is true, so pre-fix NaN was written and then won the
+// sponsorship overlay, serializing to a misleading price:null. Now it's rejected.
+test('F4 admin/video: a non-numeric price_monthly is REJECTED, not written as NaN', () => {
+ const DATA = tmpData({});
+ const routes = mount(DATA);
+ // seed a valid price, then try to corrupt it with a non-numeric value
+ hit(routes, 'POST /api/ad/admin/video', { body: { uid: 'yt-a1', price_monthly: 1200 } });
+ const { body } = hit(routes, 'POST /api/ad/admin/video', { body: { uid: 'yt-a1', price_monthly: 'abc' } });
+ assert.equal(body.state.price_monthly, 1200, 'non-numeric ignored — prior numeric value survives, never NaN');
+ assert.ok(!Number.isNaN(body.state.price_monthly), 'never stores NaN');
+});
+
+test('F4 admin/video: numeric-string price sets it, and null clears it back to suggested', () => {
+ const DATA = tmpData({});
+ const routes = mount(DATA);
+ const set = hit(routes, 'POST /api/ad/admin/video', { body: { uid: 'yt-a1', price_monthly: '1500' } });
+ assert.equal(set.body.state.price_monthly, 1500, 'numeric string coerced and stored');
+ const cleared = hit(routes, 'POST /api/ad/admin/video', { body: { uid: 'yt-a1', price_monthly: null } });
+ assert.equal(cleared.body.state.price_monthly, undefined, 'null clears the override (fall back to suggestPrice)');
+});
← f709ec78 test(adslots): +124 coverage (buildInventory sources/dedup,
·
back to Rentv 2026
·
chore: v0.24.1 (session close — ad-slots correctness fixes + a415d241 →