From 6a2be6be6bbc06eaa2aaefc1d3e86efd96a969a8 Mon Sep 17 00:00:00 2001 From: Nicole Finateri Date: Fri, 10 Jul 2026 12:01:41 -0700 Subject: [PATCH] fix(store): guard new_engagement against None in store_findings update path `.get("engagement_score", 0)` only substitutes 0 for an *absent* key, so a present-but-None value reached `max(None, existing)` on the update branch and raised TypeError ("'>' not supported between instances of 'float' and 'NoneType'"). Guard `new_engagement` with `or 0`, symmetric with the existing-row side already guarded on the same line. The standard `findings_from_report` pipeline coerces None->0 at the producer, so this hardens the public `store_findings(List[Dict[str, Any]])` boundary for arbitrary callers rather than fixing a reachable pipeline crash. Adds a regression test in tests/test_store.py. --- CHANGELOG.md | 4 +++ skills/last30days/scripts/store.py | 2 +- tests/test_store.py | 43 ++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 325b62cf5..6a5420372 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Native Grok Build (xAI) plugin and marketplace lane: `.grok-plugin/plugin.json` + `.grok-plugin/marketplace.json` so `grok plugin install mvanhorn/last30days-skill` and `grok plugin marketplace add mvanhorn/last30days-skill` work as first-class install paths. The self-hosted catalog uses a bare Git URL source (tracks HEAD); submitting to the official `xai-org/plugin-marketplace` remains a post-merge SHA-pinned outbound PR documented in `AGENTS.md`. +### Fixed + +- `store_findings` no longer raises `TypeError` when a re-sighted finding carries `engagement_score: None`. `.get("engagement_score", 0)` only substitutes `0` for an *absent* key, so a present-but-`None` value reached `max(None, …)` on the update branch — `new_engagement` is now guarded with `or 0`, symmetric with the existing-row side. Hardens the public `store_findings(List[Dict[str, Any]])` boundary for arbitrary callers; the standard `findings_from_report` pipeline already coerces `None`→0 at the producer, so this closes the API boundary rather than a reachable pipeline crash. Regression test added in `tests/test_store.py`. + ## [3.11.0] - 2026-07-05 ### Added diff --git a/skills/last30days/scripts/store.py b/skills/last30days/scripts/store.py index 4e058c016..d20b9fcc3 100644 --- a/skills/last30days/scripts/store.py +++ b/skills/last30days/scripts/store.py @@ -424,7 +424,7 @@ def store_findings( for url, f in with_urls: existing = existing_by_url.get(url) - new_engagement = f.get("engagement_score", 0) + new_engagement = f.get("engagement_score") or 0 if existing: update_rows.append(( max(new_engagement, existing["engagement_score"] or 0), diff --git a/tests/test_store.py b/tests/test_store.py index dccf620b6..370eddf0a 100644 --- a/tests/test_store.py +++ b/tests/test_store.py @@ -848,5 +848,48 @@ def test_get_new_findings_filters_by_date(temp_db, sample_report): assert len(new_findings) == 4 + +def test_store_findings_none_engagement_on_update_does_not_crash(temp_db): + """Regression: store_findings must not TypeError when an update-path finding + carries engagement_score=None. + + .get("engagement_score", 0) only substitutes 0 for an *absent* key, so a + present-but-None value reached max(None, existing) on the update branch. + store_findings accepts arbitrary List[Dict[str, Any]] callers, so the + boundary must stay null-safe (engagement_score is float | None in schema.py). + """ + topic_id = store.add_topic("None Engagement Topic")["id"] + url = "https://example.com/none-engagement" + base = { + "source": "reddit", + "source_url": url, + "source_title": "T", + "author": "", + "content": "", + "summary": "", + "relevance_score": 0.5, + } + + # First sighting carries a real engagement score. + run1 = store.record_run(topic_id, source_mode="test", status="running") + store.store_findings(run1, topic_id, [{**base, "engagement_score": 5.0}]) + + # Re-sighting the same URL with engagement_score=None takes the UPDATE branch; + # before the fix this raised TypeError from max(None, 5.0). + run2 = store.record_run(topic_id, source_mode="test", status="running") + counts = store.store_findings(run2, topic_id, [{**base, "engagement_score": None}]) + assert counts["updated"] == 1 + + con = sqlite3.connect(str(temp_db)) + try: + stored = con.execute( + "SELECT engagement_score FROM findings WHERE source_url = ?", (url,) + ).fetchone()[0] + finally: + con.close() + # max(None -> 0, existing 5.0) keeps the higher real score. + assert stored == 5.0 + + if __name__ == "__main__": pytest.main([__file__, "-v"])