diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index afec663..832f862 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -19,6 +19,62 @@ or files, and the "so-what" for future readers. --- +## 2026-07-17 — the archive banner was ~100% false, and structurally so + +Tags: `bug`, `design`. + +Reported from daily use: *"Major bugs are experienced daily by me in the +content comparisons... very misleading banners so frequent that I ignore +them."* The banner was not mistuned. It could not have worked. + +`shouldOfferArchive` compared **raw HTML strings**. The two sides are +different substrates and always were: + +- capture → `article.content`, Readability `innerHTML`, wrapped in + `
` +- relay → `markdownToHtml(markdown)`, rebuilt from the event + +For any **multi-paragraph** article — every real one — these cannot +match: `markdownToHtml` joins paragraphs with `\n\n` while Readability +emits `

` with no separator. So neither the equality guard nor the +containment guard could fire, and with the default `'always'` mode +probing unconditionally, the banner fired on every published article, +every visit, carrying no information. Once a banner is always on, it is +off. + +**Worth recording precisely, because the first cut of this was stated too +strongly:** containment is not unreachable in *principle*. A +single-paragraph body has no `\n\n` to introduce, so it IS a clean +substring of its Readability wrapper and the guard suppressed correctly. +Short pieces behaved; real articles did not. That asymmetry is why the +flood looked erratic rather than total. +`tests/archive-banner.test.mjs` pins both halves, with the relay fixture +taken verbatim from real `markdownToHtml` output. + +**Fix:** gate on the canonical 13.4 hash, which was already correct on +both sides and simply never consulted — the published `x` tag (read back +as `_articleHash`), the archive row's `articleHash`, and +`state.articleHash` agree by construction. The gate only ever +*suppresses*: equal hashes ⇒ same canonical content ⇒ nothing to offer, +in any mode. A missing hash (older rows, pre-13.4 events) or a real +difference falls through to the prior heuristics untouched, so `'richer'` +stays conservative. + +Two things fell out of doing it: the decision moved to +`reader/archive-banner.js` (pure, so it can be tested — `index.js` is not +importable from tests), and `checkArchiveAvailability` now computes the +current hash itself when `state.articleHash` has not landed. That hash is +filled by an async IIFE at load while the probe runs on a 100ms timer — +the probe can win, and silently degrading to the unsound body compare is +exactly the bug. When the user has edited, the hash is legitimately stale, +so it stays null and the body heuristics answer, as before. + +**Not fixed here:** the markdown→HTML→markdown escape doubling that makes +a Load-archive round trip mint a NEW `x` tag (see 2026-07-08, "PDF stack, +round three" — fixed for the PDF path only; articles still re-derive). +Until that lands, a hash difference after a round trip may be an artifact +rather than a real edit. + ## 2026-07-17 — "Load archive" served the WRONG ARTICLE: `#r` is not an identity index Tags: `bug`, `design`. diff --git a/src/reader/archive-banner.js b/src/reader/archive-banner.js new file mode 100644 index 0000000..c6e8f61 --- /dev/null +++ b/src/reader/archive-banner.js @@ -0,0 +1,82 @@ +// Archive-banner decision — pure, so it can be tested. +// +// The reader offers an archived body over the current capture when the +// two differ. Deciding "differ" on the BODIES was the bug: the two +// sides are not comparable and never were. +// +// capture side → article.content = Readability innerHTML, wrapped +// in

+// relay side → markdownToHtml(markdown), rebuilt from the event +// +// For any MULTI-paragraph article — i.e. every real one — these cannot +// match: markdownToHtml joins paragraphs with "\n\n" while Readability +// emits "

" with no separator, so neither the equality guard nor +// the containment guard below can fire. With the default 'always' mode +// probing unconditionally, the banner fired on every published article, +// every visit — ~100% false, which trained the user to ignore it. +// +// (A single-paragraph body has no "\n\n" to introduce, so it IS a clean +// substring of its wrapper and the containment guard did suppress. Short +// pieces behaved; real articles did not. tests/archive-banner.test.mjs +// pins both halves.) +// +// The canonical Phase-13.4 article hash is the sound test, and it was +// already present on both sides, simply never consulted: the published +// `x` tag (read back as `_articleHash`), the archive row's +// `articleHash`, and the reader's `state.articleHash` agree by +// construction. +// +// No chrome.*, no DOM — the caller supplies the bodies and the hashes. + +/** + * Should an archived body be surfaced over the current capture? + * + * The hash gate only ever SUPPRESSES: a match means the same canonical + * content, so there is nothing to offer in any mode. A missing hash + * (older cache rows, a pre-13.4 event) or a genuine difference falls + * through to the body heuristics, unchanged from before. + * + * Sensitivity modes (Options → Advanced → Archive banner): + * 'richer' — the archive must be ≥1.3× longer AND >1000 chars. + * 'always' — any non-trivial difference; skips byte-identical bodies + * and skips when the archive is strictly contained in the + * current body (the current is a superset, so the archive + * can only lose information). + * + * @param {string} currentBody the capture on screen + * @param {string} archiveBody the candidate archived body + * @param {string} mode 'always' | 'richer' + * @param {string|null} [currentHash] canonical hash of currentBody + * @param {string|null} [archiveHash] canonical hash of archiveBody + * @returns {boolean} + */ +export function shouldOfferArchive(currentBody, archiveBody, mode, currentHash, archiveHash) { + if (!archiveBody) return false; + if (currentHash && archiveHash && currentHash === archiveHash) return false; + if (mode === 'richer') { + return archiveBody.length > currentBody.length * 1.3 && archiveBody.length > 1000; + } + if (archiveBody === currentBody) return false; + if (currentBody && currentBody.includes(archiveBody)) return false; + return true; +} + +/** + * Human-readable reason the banner is showing. Length-based on purpose: + * it describes the SIZE difference the user can act on, and says only + * that the bodies differ when neither is meaningfully longer. + * + * @param {string} currentBody + * @param {string} archiveBody + * @returns {string} + */ +export function describeMetric(currentBody, archiveBody) { + const cur = currentBody.length; + const arc = archiveBody.length; + if (cur > 0 && arc >= cur * 1.3) { + return `Archive is ${(arc / Math.max(cur, 1)).toFixed(1)}× longer`; + } + if (arc > cur) return `Archive is ${arc - cur} chars longer`; + if (arc < cur) return `Archive is ${cur - arc} chars shorter`; + return 'Archive differs from current capture'; +} diff --git a/src/reader/index.js b/src/reader/index.js index 0f3f222..d18ea24 100644 --- a/src/reader/index.js +++ b/src/reader/index.js @@ -41,6 +41,7 @@ import { loadFlags, isEnabled } from '../shared/metadata/feature-flags.js'; import { ForensicModel, ForensicBaseline } from '../shared/forensic-model.js'; import { openFindingModal, openBaselineModal } from '../shared/forensic-modal.js'; import { renderFindingsBar } from './findings-section.js'; +import { shouldOfferArchive, describeMetric } from './archive-banner.js'; import { openLlmReview } from './llm-review.js'; import { capturePdfToArticle } from './pdf-capture.js'; import { pageOfOffset, pageFragmentSelector } from '../shared/pdf-layout.js'; @@ -561,6 +562,24 @@ async function checkArchiveAvailability() { const currentBody = state.article.content || ''; const currentLen = currentBody.length; + // The canonical hash of what's on screen — the only sound way to + // ask "is the archive the same content?" (see shouldOfferArchive). + // `state.articleHash` is filled by an async IIFE at load and this + // probe runs on a 100ms timer, so it can win that race; compute it + // here rather than silently degrading to the unsound body compare. + // When the user has edited, the hash is legitimately stale — leave + // it null and let the body heuristics answer, as before. + let currentHash = null; + if (!state.hashDirty) { + currentHash = state.articleHash; + if (!currentHash) { + try { + currentHash = await canonicalArticleHash( + EventBuilder.assembleArticleBody(hashableArticle(state.article))); + } catch (_) { currentHash = null; } + } + } + // 1. Try local cache first — then through the alias map (a prior // capture of the same piece may key under the alias-resolved // original of this address). @@ -574,7 +593,7 @@ async function checkArchiveAvailability() { } if (cached && cached.article && cached.article.content) { const cachedBody = cached.article.content; - if (shouldOfferArchive(currentBody, cachedBody, mode)) { + if (shouldOfferArchive(currentBody, cachedBody, mode, currentHash, cached.articleHash)) { renderArchiveBanner({ source: 'cache', cachedAt: cached.cachedAt, @@ -597,7 +616,8 @@ async function checkArchiveAvailability() { }); if (resp && resp.ok && resp.found && resp.article) { const reconstructedBody = resp.article.content || ''; - if (shouldOfferArchive(currentBody, reconstructedBody, mode)) { + if (shouldOfferArchive(currentBody, reconstructedBody, mode, + currentHash, resp.article._articleHash)) { renderArchiveBanner({ source: 'relay', author: resp.authorPubkey, @@ -630,37 +650,6 @@ async function loadPreferences() { }); } -/** - * Decide whether an archived body is worth surfacing over the current - * capture, given the user's sensitivity preference. - * - * 'richer' keeps the prior 1.3×/1000-char threshold. - * 'always' shows whenever the archive is non-trivially different — - * skip byte-identical matches and skip when the archive is - * strictly contained in the current body (the current is a - * superset, so the archive can only lose information). - */ -function shouldOfferArchive(currentBody, archiveBody, mode) { - if (!archiveBody) return false; - if (mode === 'richer') { - return archiveBody.length > currentBody.length * 1.3 && archiveBody.length > 1000; - } - if (archiveBody === currentBody) return false; - if (currentBody && currentBody.includes(archiveBody)) return false; - return true; -} - -function describeMetric(currentBody, archiveBody) { - const cur = currentBody.length; - const arc = archiveBody.length; - if (cur > 0 && arc >= cur * 1.3) { - return `Archive is ${(arc / Math.max(cur, 1)).toFixed(1)}× longer`; - } - if (arc > cur) return `Archive is ${arc - cur} chars longer`; - if (arc < cur) return `Archive is ${cur - arc} chars shorter`; - return 'Archive differs from current capture'; -} - /** * Render the archive banner above the article body. Two actions: * diff --git a/tests/archive-banner.test.mjs b/tests/archive-banner.test.mjs new file mode 100644 index 0000000..65a6aac --- /dev/null +++ b/tests/archive-banner.test.mjs @@ -0,0 +1,137 @@ +// Archive-banner decision (reader/archive-banner.js). +// +// Reported from daily use: *"Major bugs are experienced daily by me in +// the content comparisons... very misleading banners so frequent that I +// ignore them."* The banner was ~100% false, and structurally so — not +// a tuning problem. +// +// `shouldOfferArchive` compared raw HTML strings. The capture side is +// Readability innerHTML (wrapped in

); the +// relay side is markdownToHtml(markdown). For any multi-paragraph +// article — i.e. every real one — the two cannot match: markdownToHtml +// joins paragraphs with "\n\n" while Readability emits "

" with +// no separator, so neither the equality guard nor the containment guard +// can fire. With 'always' (the default) probing unconditionally, the +// banner fired on every published article, every visit, forever. +// +// (Precisely: containment is not unreachable in *principle* — a +// single-paragraph body IS a clean substring of its Readability +// wrapper, and then the old guard did suppress. The flood came from +// real, multi-paragraph articles. The fixture below is built from the +// actual markdownToHtml output so the claim is the true one.) +// +// The canonical 13.4 hash is the sound test and was already correct on +// both sides. These tests pin the failure mode (so nobody "fixes" the +// banner by tuning a threshold again) and the hash gate that closes it. + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +const { shouldOfferArchive, describeMetric } = + await import('../src/reader/archive-banner.js'); + +const HASH_A = 'a1b2c3d4e5f60718293a4b5c6d7e8f90a1b2c3d4e5f60718293a4b5c6d7e8f90'; +const HASH_B = 'ffffffff11111111222222223333333344444444555555556666666677777777'; + +// Same two-paragraph article, two substrates. RELAY_HTML is verbatim +// ContentExtractor.markdownToHtml('...\n\n...') output; CAPTURE_HTML is +// the Readability shape of the same prose. +const CAPTURE_HTML = '

The reporting rests on unnamed sources.

A second paragraph follows.

'; +const RELAY_HTML = '

The reporting rests on unnamed sources.

\n\n

A second paragraph follows.

'; + +// --- the root cause, pinned --------------------------------------------------- + +test('THE BUG: same article, two substrates — the body guards cannot fire', () => { + // The whole disease. Both guards are false for identical content, + // so pre-fix the 'always' path offered a banner for an article + // that had not changed at all. + assert.notEqual(CAPTURE_HTML, RELAY_HTML, 'equality guard cannot fire'); + assert.equal(CAPTURE_HTML.includes(RELAY_HTML), false, + 'containment cannot fire: markdownToHtml separates paragraphs with \\n\\n, Readability does not'); + assert.equal(shouldOfferArchive(CAPTURE_HTML, RELAY_HTML, 'always'), true, + 'without hashes this still offers — the pre-fix behavior, preserved as a fallback'); +}); + +test('THE FIX: equal hashes suppress the banner even though the bodies differ', () => { + assert.equal(shouldOfferArchive(CAPTURE_HTML, RELAY_HTML, 'always', HASH_A, HASH_A), false, + 'same canonical content ⇒ nothing to offer'); +}); + +test('the hash gate suppresses in every mode', () => { + for (const mode of ['always', 'richer']) { + assert.equal(shouldOfferArchive('short', 'x'.repeat(5000), mode, HASH_A, HASH_A), false, + `${mode}: a hash match wins over the length heuristic`); + } +}); + +// --- the gate only ever suppresses -------------------------------------------- + +test('differing hashes do NOT force a banner — the mode still decides', () => { + // 'richer' must stay conservative: a real difference that is not + // meaningfully fuller is still not worth interrupting for. + assert.equal(shouldOfferArchive('x'.repeat(2000), 'y'.repeat(2100), 'richer', HASH_A, HASH_B), false, + 'richer: not 1.3× longer ⇒ no banner, hashes notwithstanding'); + assert.equal(shouldOfferArchive('x'.repeat(500), 'y'.repeat(5000), 'richer', HASH_A, HASH_B), true, + 'richer: genuinely fuller ⇒ banner'); + assert.equal(shouldOfferArchive(CAPTURE_HTML, RELAY_HTML, 'always', HASH_A, HASH_B), true, + 'always: a real difference ⇒ banner'); +}); + +test('a missing hash degrades to the prior behavior, never to a wrong suppression', () => { + // Older cache rows and pre-13.4 events carry no hash. Falling back + // is honest; suppressing on a half-known comparison would hide a + // real difference. + assert.equal(shouldOfferArchive(CAPTURE_HTML, RELAY_HTML, 'always', HASH_A, null), true); + assert.equal(shouldOfferArchive(CAPTURE_HTML, RELAY_HTML, 'always', null, HASH_A), true); + assert.equal(shouldOfferArchive(CAPTURE_HTML, RELAY_HTML, 'always', null, null), true); + assert.equal(shouldOfferArchive(CAPTURE_HTML, RELAY_HTML, 'always', '', ''), true, + 'empty strings are not a match'); +}); + +// --- prior behavior, unchanged ------------------------------------------------ + +test('no archive body is never an offer', () => { + assert.equal(shouldOfferArchive('anything', '', 'always'), false); + assert.equal(shouldOfferArchive('anything', null, 'always'), false); + assert.equal(shouldOfferArchive('anything', '', 'always', HASH_A, HASH_B), false); +}); + +test('cache path: byte-identical bodies still suppress without hashes', () => { + // The local-cache path CAN byte-match (same substrate both sides), + // which is why this guard was reachable there and the flood was + // worst on the relay path. + assert.equal(shouldOfferArchive(CAPTURE_HTML, CAPTURE_HTML, 'always'), false); +}); + +test('an archive strictly contained in the current body suppresses', () => { + assert.equal(shouldOfferArchive('

full body here

', 'full body', 'always'), false, + 'the current capture is a superset — the archive can only lose information'); +}); + +test('the single-paragraph case: containment DOES fire, which is why the flood looked erratic', () => { + // A one-paragraph body has no "\n\n" to introduce, so the relay HTML + // is a clean substring of its Readability wrapper and the old guard + // suppressed correctly. Short pieces behaved; real articles did not. + // Pinned so the distinction is not mistaken for a regression later. + const oneParaCapture = '

Only one paragraph.

'; + const oneParaRelay = '

Only one paragraph.

'; + assert.equal(oneParaCapture.includes(oneParaRelay), true); + assert.equal(shouldOfferArchive(oneParaCapture, oneParaRelay, 'always'), false); +}); + +test('richer keeps the 1.3×/1000-char threshold exactly', () => { + assert.equal(shouldOfferArchive('x'.repeat(1000), 'y'.repeat(1300), 'richer'), false, + 'exactly 1.3× is not > 1.3×'); + assert.equal(shouldOfferArchive('x'.repeat(1000), 'y'.repeat(1301), 'richer'), true); + assert.equal(shouldOfferArchive('x'.repeat(500), 'y'.repeat(1000), 'richer'), false, + '2× longer but not >1000 chars'); +}); + +// --- describeMetric ----------------------------------------------------------- + +test('describeMetric reports the size difference the user can act on', () => { + assert.equal(describeMetric('x'.repeat(1000), 'y'.repeat(3000)), 'Archive is 3.0× longer'); + assert.equal(describeMetric('x'.repeat(1000), 'y'.repeat(1100)), 'Archive is 100 chars longer'); + assert.equal(describeMetric('x'.repeat(1000), 'y'.repeat(900)), 'Archive is 100 chars shorter'); + assert.equal(describeMetric('x'.repeat(1000), 'y'.repeat(1000)), 'Archive differs from current capture'); +});