Stop reader mode passing a site's cookie notice off as the article

Reported live: reader mode on a New Republic feature showed nothing but
the cookie disclaimer, and had cached it.

Reproduced outside the app by running ReaderExtractor's own injected
script (Readability.js and all) against the real page. With the article
body present, extraction is perfect. With the body absent from the DOM at
the moment the script runs, Readability's first pass -- which discards
"unlikely candidates", and the Didomi banner's class is
didomi-notice-banner -- comes back under its own 500-character threshold,
so it retries with that stripping switched off. The banner's single
paragraph is 510 characters, i.e. just over the threshold, so it comes
back as the article with the right title attached.

Nothing on the Kotlin side looked at that. Any non-blank content counted
as ExtractionResult.Success, was written to articles.extractedContent,
and was never reconsidered -- so one bad load became a permanent cookie
disclaimer, which is the "extractedContent cached: true" in the report.

ConsentNotice recognises such a result: a hard plain-text length cap
first (nothing long is ever rejected, however much consent boilerplate it
quotes), then either a known CMP container id -- Readability strips
classes but keeps ids -- or two or more distinct consent phrases.

The accept/retry/give-up decision moves out of the WebView callback into
decideEvalOutcome so it can be tested without a device. A consent notice
is now "not ready yet", which routes it into the retry the extractor
already does for an empty result; if it survives all six attempts that is
what the page serves, and it fails as ExtractionFailure.ConsentInterstitial
rather than being accepted.

A fixed extractor cannot repair what is already on disk, so
loadCachedExtraction drops a cached notice on read and re-extracts, in
both ReaderScreen and LinkReaderScreen. No schema change: the repair
happens when the article is next opened. Reader mode is still never
turned on heuristically -- replacing a discarded notice does not flip it.

The bug report now also carries the last extraction error, without which
a report about bad reader content cannot say whether extraction failed.

Tests: 59 -> 78.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B

11 files changed, 581 insertions, 26 deletions

diff --git a/.coverage-baseline b/.coverage-baseline
index 2db2f6e..36e5a8a 100644
--- a/.coverage-baseline
+++ b/.coverage-baseline
@@ -1 +1 @@
-5.40
+15.17
diff --git a/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt b/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt
new file mode 100644
index 0000000..313d467
--- /dev/null
+++ b/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt
@@ -0,0 +1,41 @@
+package org.terst.feedreader.data
+
+import org.terst.feedreader.readability.ConsentNotice
+
+/** What a cached `articles.extractedContent` value turned out to be worth. */
+data class CachedExtraction(
+    /** The content to show, or null if there is nothing usable. */
+    val content: String?,
+    /**
+     * True when a cached consent notice was thrown away. The caller has nothing to
+     * show and should extract again -- the cache is the reason the article looked
+     * broken, so dropping it silently would just leave a blank reader.
+     */
+    val discardedConsentNotice: Boolean
+)
+
+/**
+ * Reads a cached extraction, dropping it if it is the site's cookie/consent notice
+ * rather than the article.
+ *
+ * Needed because the extractor's own guard can only keep a bad extraction from
+ * being written in future. The reported bug ("reader mode just extracts cookie
+ * disclaimer", with `extractedContent cached: true`) was already on disk: every
+ * subsequent open read it straight back out, so no amount of fixing the extractor
+ * would have changed what the user saw. A poisoned row is cleared here rather than
+ * in a schema migration, so it is repaired whenever the article is next opened --
+ * including for anything extracted before this check existed.
+ *
+ * [clearCache] is passed in rather than taking the DAO so this stays a plain
+ * decision that can be tested without a database.
+ */
+suspend fun loadCachedExtraction(
+    cached: String?,
+    clearCache: suspend () -> Unit
+): CachedExtraction {
+    if (!ConsentNotice.looksLikeConsentNotice(cached)) {
+        return CachedExtraction(content = cached, discardedConsentNotice = false)
+    }
+    clearCache()
+    return CachedExtraction(content = null, discardedConsentNotice = true)
+}
diff --git a/app/src/main/java/org/terst/feedreader/readability/ConsentNotice.kt b/app/src/main/java/org/terst/feedreader/readability/ConsentNotice.kt
new file mode 100644
index 0000000..5699595
--- /dev/null
+++ b/app/src/main/java/org/terst/feedreader/readability/ConsentNotice.kt
@@ -0,0 +1,132 @@
+package org.terst.feedreader.readability
+
+/**
+ * Recognises an extraction result that is the page's cookie/consent notice rather
+ * than its article.
+ *
+ * Why this is needed at all: Readability's first pass discards "unlikely
+ * candidates" by class/id, which a consent banner normally is. But when that pass
+ * comes back under Readability's own 500-character threshold -- which is what
+ * happens whenever the real article body is not in the DOM at the moment the
+ * script runs -- it retries with that stripping switched OFF. A consent banner is
+ * then a perfectly ordinary candidate, and the ones sites actually ship are long
+ * enough to clear the threshold (the reported New Republic/Didomi notice is 510
+ * characters). Readability hands it back as the article, with the right title
+ * attached, and nothing downstream could tell the difference.
+ *
+ * The bar for rejecting something has to be high: throwing away a real article is
+ * worse than showing a cookie notice. So two independent, deliberately narrow
+ * signals, and a hard length cap on both -- a real article body is far longer than
+ * any banner, so nothing long is ever rejected no matter what it says.
+ */
+object ConsentNotice {
+
+    /**
+     * Plain-text length past which a result is never treated as a banner, however
+     * much consent boilerplate it contains. Readability's own `charThreshold` is
+     * 500; this leaves generous room above it for the longest real banners
+     * (multi-paragraph "we and our 847 partners" notices) while staying far below
+     * any actual article.
+     */
+    private const val MAX_NOTICE_TEXT_LENGTH = 2_000
+
+    /** How many distinct consent phrases a result needs before wording alone convicts it. */
+    private const val MIN_PHRASE_MATCHES = 2
+
+    /**
+     * Container ids belonging to consent-management platforms. Readability strips
+     * class attributes but preserves ids, so the CMP's own container id survives
+     * into the extracted HTML -- the most precise signal available, since no real
+     * article body is ever inside one of these.
+     */
+    private val CMP_CONTAINER_IDS = listOf(
+        "didomi-host",
+        "didomi-notice",
+        "didomi-popup",
+        "onetrust-banner-sdk",
+        "onetrust-consent-sdk",
+        "onetrust-policy-text",
+        "optanon-alert-box-wrapper",
+        "cybotcookiebotdialog",
+        "qc-cmp2-container",
+        "sp_message_container",
+        "truste-consent-track",
+        "usercentrics-root",
+        "usercentrics-cmp-ui",
+        "osano-cm-dialog",
+        "iubenda-cs-banner",
+        "cookie-law-info-bar",
+        "cookie-notice",
+        "cookie-banner",
+        "cookieconsent",
+        "cmpbox",
+    )
+
+    /**
+     * Phrases that only really occur in consent boilerplate. Individually weak --
+     * an article can mention any one of them -- so [MIN_PHRASE_MATCHES] are
+     * required, and only within the length cap.
+     */
+    private val CONSENT_PHRASES = listOf(
+        "uses cookies",
+        "use cookies",
+        "using cookies",
+        "tracking technologies",
+        "by continuing to use this",
+        "by continuing to browse",
+        "accept all cookies",
+        "reject all",
+        "manage your preferences",
+        "manage preferences",
+        "cookie preferences",
+        "cookie policy",
+        "cookie settings",
+        "we and our partners",
+        "our advertising and analytics",
+        "your privacy choices",
+        "consent to our use of cookies",
+        "personalise content and ads",
+        "personalize content and ads",
+    )
+
+    /**
+     * True when [html] is a consent/cookie notice rather than article content.
+     *
+     * Takes the stored HTML rather than Readability's `textContent` so the same
+     * check works on a value already cached in `articles.extractedContent`, where
+     * the text form was never kept.
+     */
+    fun looksLikeConsentNotice(html: String?): Boolean {
+        if (html.isNullOrBlank()) return false
+
+        // Length is judged on the text, not the markup: a banner wrapped in enough
+        // nested divs and inline styles clears 2KB of characters while still being
+        // three sentences of text. Checked first, so nothing long is ever rejected
+        // however much consent boilerplate it happens to quote.
+        val text = plainText(html)
+        val withinNoticeLength = text.isNotBlank() && text.length <= MAX_NOTICE_TEXT_LENGTH
+
+        return withinNoticeLength && (hasCmpContainer(html) || readsAsConsentBoilerplate(text))
+    }
+
+    private fun hasCmpContainer(html: String): Boolean {
+        val markup = html.lowercase()
+        return CMP_CONTAINER_IDS.any { markup.contains("id=\"$it") || markup.contains("id='$it") }
+    }
+
+    private fun readsAsConsentBoilerplate(text: String): Boolean {
+        val lowerText = text.lowercase()
+        return CONSENT_PHRASES.count { lowerText.contains(it) } >= MIN_PHRASE_MATCHES
+    }
+
+    /**
+     * Crude tag strip -- enough to measure length and match phrases. Deliberately
+     * not a parser: this runs on every article open, and getting the character
+     * count roughly right is all the decision needs.
+     */
+    private fun plainText(html: String): String =
+        html.replace(TAG_REGEX, " ").replace(WHITESPACE_REGEX, " ").trim()
+
+    private val TAG_REGEX = Regex("<[^>]*>")
+    private val WHITESPACE_REGEX = Regex("\\s+")
+}
diff --git a/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt b/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt
new file mode 100644
index 0000000..ed0033d
--- /dev/null
+++ b/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt
@@ -0,0 +1,61 @@
+package org.terst.feedreader.readability
+
+/** What to do with one `evaluateJavascript` result. */
+sealed interface EvalDecision {
+    /** Nothing more to try: this is the extraction's outcome. */
+    data class Done(val result: ExtractionResult) : EvalDecision
+
+    /** The page probably was not ready; inject the script again. */
+    data object Retry : EvalDecision
+}
+
+/**
+ * Decides, for one script result, whether extraction succeeded, should be
+ * retried, or has failed and why.
+ *
+ * Lifted out of the WebView callback it used to live inside so it can be
+ * exercised without a device. The bug that prompted that: a result consisting
+ * only of the page's cookie-consent banner was non-blank and error-free, so it
+ * counted as a success, was cached to `articles.extractedContent`, and was never
+ * reconsidered. Reported live as "reader mode just extracts cookie disclaimer".
+ */
+internal fun decideEvalOutcome(
+    article: ExtractedArticle?,
+    rawResult: String,
+    attempt: Int,
+    maxAttempts: Int
+): EvalDecision {
+    val readabilityError = article?.error?.takeIf { it.isNotBlank() }
+    // A banner is exactly what a half-loaded page produces -- the consent script
+    // runs early, the article body may not be in the DOM yet -- so this is not
+    // usable, which routes it into the same retry the extractor already does for
+    // an empty result rather than being accepted.
+    val isConsentNotice = article != null &&
+        readabilityError == null &&
+        ConsentNotice.looksLikeConsentNotice(article.content)
+    val usable = article != null &&
+        readabilityError == null &&
+        !article.content.isNullOrBlank() &&
+        !isConsentNotice
+
+    return when {
+        usable -> EvalDecision.Done(ExtractionResult.Success(article!!))
+
+        // Readability ran and objected: that verdict will not change.
+        readabilityError != null ->
+            EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.ReadabilityError(readabilityError)))
+
+        attempt < maxAttempts -> EvalDecision.Retry
+
+        // Every attempt came back with nothing but the consent notice, so this is
+        // what the page actually serves -- report it as its own failure rather
+        // than passing a cookie disclaimer off as the article.
+        isConsentNotice ->
+            EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.ConsentInterstitial))
+
+        article == null ->
+            EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.BadResult(rawResult)))
+
+        else -> EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.NoContent))
+    }
+}
diff --git a/app/src/main/java/org/terst/feedreader/readability/ExtractionFailure.kt b/app/src/main/java/org/terst/feedreader/readability/ExtractionFailure.kt
index 532b109..63fbdb2 100644
--- a/app/src/main/java/org/terst/feedreader/readability/ExtractionFailure.kt
+++ b/app/src/main/java/org/terst/feedreader/readability/ExtractionFailure.kt
@@ -28,6 +28,12 @@ sealed interface ExtractionFailure {
 
     /** The JS result could not be decoded. */
     data class BadResult(val raw: String) : ExtractionFailure
+
+    /**
+     * Readability returned content, but it was the page's cookie/consent notice,
+     * not the article -- see [ConsentNotice].
+     */
+    data object ConsentInterstitial : ExtractionFailure
 }
 
 private const val MILLIS_PER_SECOND = 1_000
@@ -47,6 +53,7 @@ fun ExtractionFailure.userMessage(): String = when (this) {
     is ExtractionFailure.ReadabilityError -> "Couldn't parse the article ($message)"
     ExtractionFailure.NoContent -> "No article text found on the page"
     is ExtractionFailure.BadResult -> "Unexpected result from the page"
+    ExtractionFailure.ConsentInterstitial -> "Only the site's cookie notice came through, not the article"
 }
 
 /**
@@ -61,4 +68,6 @@ fun ExtractionFailure.diagnostic(): String = when (this) {
     is ExtractionFailure.ReadabilityError -> "ReadabilityError $message"
     ExtractionFailure.NoContent -> "NoContent (Readability returned no usable body)"
     is ExtractionFailure.BadResult -> "BadResult raw=${raw.take(RAW_DIAGNOSTIC_CHARS)}"
+    ExtractionFailure.ConsentInterstitial ->
+        "ConsentInterstitial (extracted content was the page's cookie/consent notice)"
 }
diff --git a/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt b/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
index 5061444..21633b4 100644
--- a/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
+++ b/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
@@ -436,10 +436,9 @@ object ReaderExtractor {
                  * Runs the extraction script, retrying while the result looks like
                  * the page simply was not ready yet.
                  *
-                 * A Readability error is final -- retrying will not change what the
-                 * parser thinks of the markup. A null result or empty content is
-                 * not: both are what an injection against a half-loaded document
-                 * produces, and the next attempt usually succeeds.
+                 * The accept/retry/give-up decision itself lives in
+                 * [decideEvalOutcome], which is testable without a WebView; this
+                 * only decodes the payload and carries the decision out.
                  */
                 private fun evaluate(view: WebView, script: String, attempt: Int) {
                     if (finished) return
@@ -451,24 +450,18 @@ object ReaderExtractor {
                         val article = unwrapped?.let {
                             runCatching { json.decodeFromString<ExtractedArticle>(it) }.getOrNull()
                         }
-                        val usable = article != null &&
-                            article.error.isNullOrBlank() &&
-                            !article.content.isNullOrBlank()
 
-                        when {
-                            usable -> finish(ExtractionResult.Success(article!!))
-
-                            // Readability ran and objected: that verdict will not change.
-                            article != null && !article.error.isNullOrBlank() ->
-                                fail(ExtractionFailure.ReadabilityError(article.error))
-
-                            attempt < MAX_EVAL_ATTEMPTS ->
+                        when (
+                            val decision = decideEvalOutcome(
+                                article = article,
+                                rawResult = rawResult ?: "null",
+                                attempt = attempt,
+                                maxAttempts = MAX_EVAL_ATTEMPTS
+                            )
+                        ) {
+                            is EvalDecision.Done -> finish(decision.result)
+                            EvalDecision.Retry ->
                                 view.postDelayed({ evaluate(view, script, attempt + 1) }, EVAL_RETRY_DELAY_MS)
-
-                            article == null ->
-                                fail(ExtractionFailure.BadResult(rawResult ?: "null"))
-
-                            else -> fail(ExtractionFailure.NoContent)
                         }
                     }
                 }
diff --git a/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt b/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt
index 4ed0722..72f6c1f 100644
--- a/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt
+++ b/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt
@@ -51,6 +51,7 @@ import org.terst.feedreader.data.DEFAULT_READER_FONT_SIZE_PX
 import org.terst.feedreader.data.Settings
 import org.terst.feedreader.data.ensureStandaloneArticle
 import org.terst.feedreader.data.saveArticleToStash
+import org.terst.feedreader.data.loadCachedExtraction
 import org.terst.feedreader.data.urlToArticleId
 import org.terst.feedreader.readability.ReaderExtractor
 import java.net.URLEncoder
@@ -95,9 +96,15 @@ fun LinkReaderScreen(url: String, onBack: () -> Unit, onOpenLink: (String) -> Un
     LaunchedEffect(url) {
         val existing = db.articleDao().getById(articleId)
         article = existing
-        if (existing?.extractedContent != null) {
-            extractedTitle = existing.title
-            extractedContent = existing.extractedContent
+        // A cached extraction can be the site's cookie-consent notice rather than the
+        // article (see CachedExtraction) -- drop it and extract again instead of showing
+        // a cookie disclaimer as the page.
+        val cached = loadCachedExtraction(existing?.extractedContent) {
+            db.articleDao().clearExtractedContent(articleId)
+        }
+        if (cached.content != null) {
+            extractedTitle = existing?.title
+            extractedContent = cached.content
         } else {
             val extracted = ReaderExtractor.extract(context, url)
             val content = extracted?.content
diff --git a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt
index 6240b77..f08088b 100644
--- a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt
+++ b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt
@@ -68,6 +68,7 @@ import org.terst.feedreader.data.Settings
 import org.terst.feedreader.data.ensureStandaloneArticle
 import org.terst.feedreader.data.saveArticleToStash
 import org.terst.feedreader.data.setReaderModeEnabled
+import org.terst.feedreader.data.loadCachedExtraction
 import org.terst.feedreader.readability.ReaderExtractor
 import java.net.URLEncoder
 
@@ -219,15 +220,26 @@ private fun ReaderContent(
         feed = loadedFeed
 
         feedContent = loaded.feedContentHtml
-        readerContent = loaded.extractedContent
+        // A previously-cached extraction can be the site's cookie-consent notice rather
+        // than the article (reported live on a New Republic feature). That is dropped
+        // from the cache on read, which leaves nothing to show -- so re-extract, or the
+        // repair would just turn a wrong article into a blank one.
+        val cachedExtraction = loadCachedExtraction(loaded.extractedContent) {
+            db.articleDao().clearExtractedContent(articleId)
+        }
+        readerContent = cachedExtraction.content
 
         // Reader mode is NEVER heuristic/automatic (no more "content looks thin, switch
         // for you") -- it strictly follows the feed's persisted, explicitly-set opt-in.
         // Once a feed has it on, articles from it default to the extracted view and
         // auto-extract on open (that's the point of persisting it per-feed: no re-tapping
-        // per article), but nothing here ever turns it on by itself.
+        // per article), but nothing here ever turns it on by itself. Replacing a discarded
+        // consent notice does not turn it on either -- for a feed-less saved article there
+        // is no feed to opt in, and the reader falls back to the extracted content anyway.
         if (loadedFeed?.readerModeEnabled == true) {
             isReaderMode = true
+        }
+        if (loadedFeed?.readerModeEnabled == true || cachedExtraction.discardedConsentNotice) {
             if (readerContent == null) {
                 isExtracting = true
                 val result = ReaderExtractor.extractResult(context, loaded.link)
@@ -509,6 +521,10 @@ private fun ReaderContent(
                             appendLine("Session isReaderMode: $isReaderMode")
                             appendLine("isSaved: ${a?.isSaved}, stashArtifactId: ${a?.stashArtifactId}")
                             appendLine("extractedContent cached: ${readerContent != null}")
+                            // Without this, a report about bad/absent reader content could
+                            // not say whether extraction had failed, let alone how -- which
+                            // is exactly what the cookie-disclaimer report ran into.
+                            appendLine("Last extraction error: ${extractionError ?: "none"}")
                         }
                         val clipboard = context.getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager
                         clipboard.setPrimaryClip(ClipData.newPlainText("Bug report", report))
diff --git a/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt b/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt
new file mode 100644
index 0000000..72dc73f
--- /dev/null
+++ b/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt
@@ -0,0 +1,67 @@
+package org.terst.feedreader.data
+
+import kotlinx.coroutines.test.runTest
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * The reported bug came with "extractedContent cached: true", which is the half
+ * that a fixed extractor cannot reach: the cookie disclaimer was already written
+ * to `articles.extractedContent`, and every open after that read it straight back
+ * out without ever looking at it again.
+ *
+ * So a poisoned cache has to be recognised and dropped on read, not just kept out
+ * on write.
+ */
+class CachedExtractionTest {
+
+    private val cookieNotice =
+        "<div id=\"readability-page-1\" class=\"page\"><div id=\"didomi-host\"><p>This website uses " +
+            "cookies and other tracking technologies (“Cookies”) to enhance user experience, to " +
+            "display targeted ads, and to analyze performance and user activity on our website. For " +
+            "more information about our use of Cookies and our privacy practices, see our Privacy " +
+            "Policy. By continuing to use this website, you agree to our Terms and Conditions.</p>" +
+            "</div></div>"
+
+    @Test
+    fun `real cached content is used as-is and the cache is left alone`() = runTest {
+        var cleared = false
+        val content = "<p>" + "The actual article body. ".repeat(60) + "</p>"
+        val result = loadCachedExtraction(content) { cleared = true }
+        assertEquals(content, result.content)
+        assertFalse(result.discardedConsentNotice)
+        assertFalse("a good cache must not be cleared", cleared)
+    }
+
+    @Test
+    fun `a cached consent notice is discarded and deleted`() = runTest {
+        var cleared = false
+        val result = loadCachedExtraction(cookieNotice) { cleared = true }
+        assertNull("the notice must not be shown as the article", result.content)
+        assertTrue(result.discardedConsentNotice)
+        assertTrue("the poisoned row must be cleared so a re-extract can replace it", cleared)
+    }
+
+    @Test
+    fun `nothing cached touches nothing`() = runTest {
+        var cleared = false
+        val result = loadCachedExtraction(null) { cleared = true }
+        assertNull(result.content)
+        assertFalse(result.discardedConsentNotice)
+        assertFalse(cleared)
+    }
+
+    // A blank value is as good as nothing cached -- it is not a consent notice, so
+    // reporting it as a discarded one would make the caller re-extract on a false
+    // premise.
+    @Test
+    fun `a blank cached value is not reported as a discarded notice`() = runTest {
+        var cleared = false
+        val result = loadCachedExtraction("   ") { cleared = true }
+        assertFalse(result.discardedConsentNotice)
+        assertFalse(cleared)
+    }
+}
diff --git a/app/src/test/java/org/terst/feedreader/readability/ConsentNoticeTest.kt b/app/src/test/java/org/terst/feedreader/readability/ConsentNoticeTest.kt
new file mode 100644
index 0000000..7f4e9cc
--- /dev/null
+++ b/app/src/test/java/org/terst/feedreader/readability/ConsentNoticeTest.kt
@@ -0,0 +1,140 @@
+package org.terst.feedreader.readability
+
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * Reported live: reader mode on a New Republic feature article showed nothing but
+ * the site's cookie disclaimer, and cached it.
+ *
+ * Reproduced outside the app by running the extractor's own injected script
+ * (ReaderExtractor's `script`, Readability.js and all) against the real page: when
+ * the article body is present, extraction is perfect. When the body is NOT in the
+ * DOM at the moment Readability runs, Readability's own fallback pass kicks in --
+ * its first pass strips "unlikely candidates" (which the consent banner is, its
+ * class says `didomi-notice-banner`), comes back under its 500-character
+ * threshold, and so it retries with that stripping disabled. The Didomi notice's
+ * single paragraph is 510 characters, i.e. just over that threshold, so it is
+ * accepted as the article.
+ *
+ * [EXTRACTED_COOKIE_NOTICE] below is not a hand-written approximation of that --
+ * it is the verbatim `content` string that reproduction produced.
+ *
+ * Nothing on the Kotlin side looked at that: any non-blank `content` counted as a
+ * successful extraction, got written to `articles.extractedContent`, and was never
+ * re-checked. Hence a permanently cached cookie disclaimer.
+ */
+class ConsentNoticeTest {
+
+    @Test
+    fun `the reported New Republic extraction is recognised as a consent notice`() {
+        assertTrue(ConsentNotice.looksLikeConsentNotice(EXTRACTED_COOKIE_NOTICE))
+    }
+
+    // Readability strips class attributes but keeps ids, so the CMP container's id
+    // survives into the extracted HTML and is the most precise signal available.
+    @Test
+    fun `other consent-management platforms are recognised by their container id`() {
+        val cmpIds = listOf(
+            "didomi-host",
+            "onetrust-banner-sdk",
+            "onetrust-consent-sdk",
+            "CybotCookiebotDialog",
+            "qc-cmp2-container",
+            "sp_message_container_1234",
+            "truste-consent-track",
+            "usercentrics-root",
+            "osano-cm-dialog",
+            "iubenda-cs-banner",
+            "optanon-alert-box-wrapper",
+            "cookie-law-info-bar",
+        )
+        for (id in cmpIds) {
+            val html = """<div id="readability-page-1"><div id="$id"><p>Please review your choices.</p></div></div>"""
+            assertTrue("$id should be recognised as a consent container", ConsentNotice.looksLikeConsentNotice(html))
+        }
+    }
+
+    // Not every cookie banner is a known CMP. A short result whose text is
+    // unmistakably consent boilerplate counts too, with no id to go on.
+    @Test
+    fun `a short unbranded cookie banner is recognised from its wording`() {
+        val html = """
+            <div id="readability-page-1"><div><p>We use cookies and similar tracking technologies
+            to personalise content and ads. By continuing to use this site you consent to our use
+            of cookies. You can manage your preferences at any time.</p></div></div>
+        """.trimIndent()
+        assertTrue(ConsentNotice.looksLikeConsentNotice(html))
+    }
+
+    // One stray phrase is not enough -- plenty of ordinary writing mentions cookies.
+    @Test
+    fun `a single incidental phrase is not enough`() {
+        val html = """<div id="readability-page-1"><div><p>The recipe calls for cookies.
+            We use cookies in the frosting too.</p></div></div>"""
+        assertFalse(ConsentNotice.looksLikeConsentNotice(html))
+    }
+
+    // The whole point is to not throw away real articles. A genuine article ABOUT
+    // cookie-consent law would hit every phrase there is; length is what separates
+    // it from a banner. Nothing long is ever rejected.
+    @Test
+    fun `a long article about cookie consent is never rejected`() {
+        val paragraph = "<p>We use cookies and other tracking technologies, and by continuing " +
+            "to use this website you consent to our use of cookies -- that is the boilerplate " +
+            "the GDPR produced, and this article is about where it came from. You can manage " +
+            "your preferences, in theory.</p>"
+        val html = "<div id=\"readability-page-1\"><div>${paragraph.repeat(12)}</div></div>"
+        assertTrue("fixture must be past the length cap", html.length > 2_000)
+        assertFalse(ConsentNotice.looksLikeConsentNotice(html))
+    }
+
+    // Short posts are normal (kottke-style linkblogging is called out in
+    // ReaderExtractor's own comments) and must survive.
+    @Test
+    fun `an ordinary short post is not a consent notice`() {
+        val html = """
+            <div id="readability-page-1"><div><p>A lovely short note about a bridge in Lisbon,
+            with one link and nothing else. This is the entire post.</p></div></div>
+        """.trimIndent()
+        assertFalse(ConsentNotice.looksLikeConsentNotice(html))
+    }
+
+    @Test
+    fun `null and blank content are not consent notices`() {
+        assertFalse(ConsentNotice.looksLikeConsentNotice(null))
+        assertFalse(ConsentNotice.looksLikeConsentNotice(""))
+        assertFalse(ConsentNotice.looksLikeConsentNotice("   \n  "))
+    }
+
+    // Markup must not count towards the length budget: a banner wrapped in enough
+    // nested divs and inline styles to exceed the cap in raw characters is still a
+    // banner.
+    @Test
+    fun `length is measured on text not markup`() {
+        val padding = "<div style=\"${"border-color: rgb(0, 0, 0); ".repeat(80)}\">"
+        val html = padding + EXTRACTED_COOKIE_NOTICE + "</div>"
+        assertTrue("fixture must be past the length cap in raw characters", html.length > 2_000)
+        assertTrue(ConsentNotice.looksLikeConsentNotice(html))
+    }
+
+    companion object {
+        /**
+         * Verbatim output of running ReaderExtractor's injected script against the
+         * reported page in a state where the article body was absent. Do not tidy
+         * this up -- its value is that it is exactly what the app produced.
+         */
+        const val EXTRACTED_COOKIE_NOTICE: String =
+            "<div id=\"readability-page-1\" class=\"page\"><div id=\"didomi-host\" data-testid=\"notice\" " +
+                "data-nosnippet=\"true\" lang=\"en\" data-lang-dir=\"ltr\"><p>This website uses cookies and " +
+                "other tracking technologies (“Cookies”) to enhance user experience, to display " +
+                "targeted ads, and to analyze performance and user activity on our website. We also may " +
+                "share information about your activity on our website with our advertising and analytics " +
+                "providers. For more information about our use of Cookies and our privacy practices, see " +
+                "our <a href=\"https://newrepublic.com/pages/privacy\" target=\"_blank\">Privacy Policy</a>. " +
+                "By continuing to use this website, you agree to our " +
+                "<a href=\"https://newrepublic.com/pages/terms-and-conditions\" target=\"_blank\">Terms and " +
+                "Conditions</a>, including our dispute resolution provisions.</p></div></div>"
+    }
+}
diff --git a/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt b/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt
new file mode 100644
index 0000000..8995e22
--- /dev/null
+++ b/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt
@@ -0,0 +1,89 @@
+package org.terst.feedreader.readability
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * The decision ReaderExtractor makes about each `evaluateJavascript` result:
+ * accept it, inject again because the page probably was not ready, or give up
+ * with a reason. It used to live inline in a WebView callback, which meant the
+ * only way to exercise it was on a device against a live page.
+ *
+ * The case that mattered and was never covered: a result that is perfectly
+ * well-formed and non-blank, and is the page's cookie-consent banner rather than
+ * the article. That counted as a success, got cached, and stayed cached.
+ */
+class EvalOutcomeTest {
+
+    private val maxAttempts = 6
+
+    private fun decide(article: ExtractedArticle?, attempt: Int, raw: String = "\"{}\"") =
+        decideEvalOutcome(article, raw, attempt, maxAttempts)
+
+    private fun article(content: String?, error: String? = null) =
+        ExtractedArticle(title = "t", content = content, textContent = content, error = error)
+
+    @Test
+    fun `real content is accepted on the first attempt`() {
+        val a = article("<p>" + "The actual article body. ".repeat(60) + "</p>")
+        val decision = decide(a, attempt = 1)
+        assertEquals(EvalDecision.Done(ExtractionResult.Success(a)), decision)
+    }
+
+    // Retrying cannot change Readability's opinion of the markup, so a thrown
+    // error is final however early it arrives.
+    @Test
+    fun `a readability error is final immediately`() {
+        val decision = decide(article(null, error = "TypeError: x is null"), attempt = 1)
+        assertEquals(
+            EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.ReadabilityError("TypeError: x is null"))),
+            decision
+        )
+    }
+
+    @Test
+    fun `an undecodable result retries and then reports a bad result`() {
+        assertEquals(EvalDecision.Retry, decide(null, attempt = 1, raw = "null"))
+        val last = decide(null, attempt = maxAttempts, raw = "null")
+        assertEquals(EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.BadResult("null"))), last)
+    }
+
+    @Test
+    fun `blank content retries and then reports no content`() {
+        assertEquals(EvalDecision.Retry, decide(article(""), attempt = 1))
+        assertEquals(
+            EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.NoContent)),
+            decide(article(""), attempt = maxAttempts)
+        )
+    }
+
+    // A cookie banner is exactly what a half-loaded page yields -- the banner is
+    // injected early, the article body may not be in the DOM yet -- so the right
+    // first move is the retry the extractor already does for an empty result.
+    @Test
+    fun `a consent notice retries rather than being accepted`() {
+        val decision = decide(article(ConsentNoticeTest.EXTRACTED_COOKIE_NOTICE), attempt = 1)
+        assertEquals(EvalDecision.Retry, decision)
+    }
+
+    @Test
+    fun `a consent notice that survives every attempt is reported as a failure`() {
+        val decision = decide(article(ConsentNoticeTest.EXTRACTED_COOKIE_NOTICE), attempt = maxAttempts)
+        assertEquals(
+            EvalDecision.Done(ExtractionResult.Failure(ExtractionFailure.ConsentInterstitial)),
+            decision
+        )
+    }
+
+    // The failure has to be distinguishable from "no content": one means the page
+    // has no article, the other means the page would not show it without consent.
+    @Test
+    fun `the consent failure says what happened`() {
+        val f: ExtractionFailure = ExtractionFailure.ConsentInterstitial
+        assertTrue(f.userMessage().isNotBlank())
+        assertTrue(f.diagnostic().isNotBlank())
+        assertTrue(f.userMessage().lowercase().contains("cookie") || f.userMessage().lowercase().contains("consent"))
+        assertTrue(f.userMessage() != ExtractionFailure.NoContent.userMessage())
+    }
+}