From 008364e922473e4e0e0290094b60d56e8d224c88 Mon Sep 17 00:00:00 2001 From: jay Date: Tue, 9 Jun 2026 20:10:27 -0400 Subject: [PATCH] Why-it-belongs: top-up requires all three fields (idempotency fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Codex: generate_summary treated why_belongs alone as a complete explanation, but get_explanation requires all three — so a partial older row (e.g. only why_belongs) would never top up and the page would fall back forever. Now the fully-cached check requires summary + what_happened + why_matters + why_belongs. Test covers the partial-row top-up. Co-Authored-By: Claude Opus 4.8 (1M context) --- goodnews/summarize.py | 9 ++++++--- tests/test_explain.py | 24 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/goodnews/summarize.py b/goodnews/summarize.py index e12392f..9616840 100644 --- a/goodnews/summarize.py +++ b/goodnews/summarize.py @@ -136,10 +136,13 @@ def generate_summary(conn: sqlite3.Connection, article_id: int, client: LocalMod explanation is topped up on the next call (so existing pages gain the section). """ existing = conn.execute( - "SELECT summary, why_belongs FROM article_summaries WHERE article_id = ?", (article_id,) + "SELECT summary, what_happened, why_matters, why_belongs FROM article_summaries WHERE article_id = ?", + (article_id,), ).fetchone() - if existing and existing["summary"] and existing["why_belongs"]: - return existing["summary"] # summary + a complete explanation already cached + # Fully cached only when the explanation is COMPLETE (all three) — matches + # get_explanation(), so a partial older row gets topped up on the next call. + if existing and existing["summary"] and existing["what_happened"] and existing["why_matters"] and existing["why_belongs"]: + return existing["summary"] row = conn.execute( "SELECT a.title, a.description, a.canonical_url, a.duplicate_of, s.accepted " "FROM articles a LEFT JOIN article_scores s ON s.article_id = a.id WHERE a.id = ?", diff --git a/tests/test_explain.py b/tests/test_explain.py index b5cb181..12cd2ce 100644 --- a/tests/test_explain.py +++ b/tests/test_explain.py @@ -60,3 +60,27 @@ def test_share_renders_structured_or_fallback(): # no explanation → the single "Why it’s here" reason line is the calm fallback fb = share.render_share_page(art, "http://b", summary="sum", explanation=None) assert "Why it’s here" in fb and "calm reason" in fb + + +def test_generate_tops_up_partial_explanation(tmp_path, monkeypatch): + c = connect(str(tmp_path / "p.db")); init_db(c); _seed_article(c) + # an older row: summary + why_belongs only (what_happened/why_matters missing) + c.execute("INSERT INTO article_summaries (article_id,summary,why_belongs) VALUES (1,'old summary','b')") + c.commit() + assert summarize.get_explanation(c, 1) is None # incomplete → falls back + + calls = {"explain": 0} + def fake_explain(client, t, s, b): + calls["explain"] += 1 + return {"what_happened": "w", "why_matters": "m", "why_belongs": "b2"} + monkeypatch.setattr(summarize, "explain_article", fake_explain) + monkeypatch.setattr(summarize, "_fetch_text", lambda url: "body") + + class FakeClient: + model = "test" + def chat_text(self, messages): return "should not be used for summary (kept)" + + out = summarize.generate_summary(c, 1, client=FakeClient()) + assert out == "old summary" # existing summary reused, not regenerated + assert calls["explain"] == 1 # explanation WAS topped up + assert summarize.get_explanation(c, 1) == {"what_happened": "w", "why_matters": "m", "why_belongs": "b2"}