From 5e6d71ced46ca1cd4c0d16f84882d43696275f9c Mon Sep 17 00:00:00 2001 From: Bryan Matthew Simonson <7519963+bryanmatthewsimonson@users.noreply.github.com> Date: Thu, 16 Jul 2026 22:12:43 -0700 Subject: [PATCH] =?UTF-8?q?fix(reader):=20the=20archive=20banner=20was=20~?= =?UTF-8?q?100%=20false=20=E2=80=94=20gate=20on=20the=20hash?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shouldOfferArchive compared raw HTML strings, and the two sides are different substrates: the capture is Readability innerHTML (wrapped in
), the relay side is markdownToHtml(md). For any multi-paragraph article — every real one — they cannot match: markdownToHtml joins paragraphs with "\n\n", Readability emits "

". Neither the equality guard nor the containment guard could fire, and 'always' (the default) probes unconditionally, so the banner fired on every published article, every visit, carrying no information. Stated precisely: containment is not unreachable in principle. A single-paragraph body IS a clean substring of its wrapper, so the guard suppressed correctly there. Short pieces behaved; real articles did not. The tests pin both halves, with the relay fixture taken verbatim from real markdownToHtml output. Gate on the canonical 13.4 hash, which was already correct on both sides and never consulted — the published `x` tag (_articleHash), the archive row's articleHash, and state.articleHash agree by construction. The gate only ever SUPPRESSES: equal hashes ⇒ same content ⇒ nothing to offer, in any mode. A missing hash or a real difference falls through to the prior heuristics untouched, so 'richer' stays conservative. The decision moves to reader/archive-banner.js (pure — index.js is not importable from tests). checkArchiveAvailability now computes the current hash itself when state.articleHash has not landed: it is filled by an async IIFE at load while the probe runs on a 100ms timer, and silently degrading to the unsound body compare is exactly the bug. An edited article leaves the hash null and the body heuristics answer, as before. Does not address the markdown→HTML→markdown escape doubling that mints a new x tag on a Load-archive round trip (fixed for PDFs only, 2026-07-08). No wire-format change. 1739 tests green; lint 0 errors. Co-Authored-By: Claude Fable 5 --- docs/JOURNAL.md | 56 ++++++++++++++ src/reader/archive-banner.js | 82 ++++++++++++++++++++ src/reader/index.js | 55 ++++++-------- tests/archive-banner.test.mjs | 137 ++++++++++++++++++++++++++++++++++ 4 files changed, 297 insertions(+), 33 deletions(-) create mode 100644 src/reader/archive-banner.js create mode 100644 tests/archive-banner.test.mjs diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 637f00e..5e74eea 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 005e9e5..4dc3f9f 100644 --- a/src/reader/index.js +++ b/src/reader/index.js @@ -40,6 +40,7 @@ import { loadFlags, isEnabled } from '../shared/metadata/feature-flags.js'; import { ForensicModel } 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'; @@ -560,6 +561,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). @@ -573,7 +592,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, @@ -596,7 +615,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, @@ -629,37 +649,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'); +});