Skip to content

fix(store): guard store_findings against None engagement_score on update path#796

Open
nicolefinateri wants to merge 1 commit into
mvanhorn:mainfrom
nicolefinateri:fix/store-findings-none-engagement
Open

fix(store): guard store_findings against None engagement_score on update path#796
nicolefinateri wants to merge 1 commit into
mvanhorn:mainfrom
nicolefinateri:fix/store-findings-none-engagement

Conversation

@nicolefinateri

@nicolefinateri nicolefinateri commented Jul 10, 2026

Copy link
Copy Markdown

What

store_findings raises TypeError when a re-sighted finding carries engagement_score: None.

Why

On the update branch the incoming score is read with a defaulted .get, then compared:

new_engagement = f.get("engagement_score", 0)            # store.py:427
...
max(new_engagement, existing["engagement_score"] or 0)   # store.py:430

dict.get(key, 0) only substitutes 0 when the key is absent. A key that is present with value None passes straight through, so new_engagement becomes None and max(None, <float>) raises:

TypeError: '>' not supported between instances of 'float' and 'NoneType'

Note the asymmetry: the existing side on line 430 is already guarded with or 0, but the new_engagement side is not. engagement_score is declared float | None in lib/schema.py, and lib/fusion.py can re-introduce None via metadata.get("engagement_score"), so None is a first-class runtime value for this field.

Scope (honest framing)

This is boundary hardening, not a fix for a crash reachable through the normal pipeline. The two producers that feed store_findings via findings_from_reportfinding_from_candidate (candidate.engagement or 0) and the supplement dict (item.engagement_score or 0.0) — already coerce None → 0 before the dict is built. But store_findings(List[Dict[str, Any]]) is a public function that accepts arbitrary dicts, so any other or future caller passing a raw finding with engagement_score: None hits the crash. This makes the boundary null-safe and symmetric with line 430.

Fix

-            new_engagement = f.get("engagement_score", 0)
+            new_engagement = f.get("engagement_score") or 0

Test

Adds test_store_findings_none_engagement_on_update_does_not_crash to tests/test_store.py: it stores a finding with a real score, re-sights the same URL with engagement_score: None, and asserts no crash plus that max(None→0, existing) preserves the higher real score. The test reproduces the TypeError on main and passes with this change.

Verified locally (Python 3.12.13, pytest 9.1.0, deps from uv.lock):

  • tests/test_store.py28 passed (the 27 existing tests + this one)
  • store-consumer blast radius (test_store, test_internals_v3, test_watchlist_commands, test_fusion_v3, test_signals_v3, test_briefing_v3, test_schema_v3) — 179 passed, 0 failed

…e 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.
@nicolefinateri nicolefinateri marked this pull request as ready for review July 10, 2026 21:08
@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes store_findings tolerate None engagement scores on re-sighted findings. The main changes are:

  • Normalizes missing or falsy incoming engagement_score values to 0 before update comparisons.
  • Adds a regression test for the None update path.
  • Documents the boundary hardening in the changelog.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
skills/last30days/scripts/store.py Normalizes incoming engagement scores before comparing them with existing stored values.
tests/test_store.py Adds coverage for a re-sighted finding whose engagement score is None.
CHANGELOG.md Records the store_findings null-safety fix.

Reviews (1): Last reviewed commit: "fix(store): guard new_engagement against..." | Re-trigger Greptile

23241a6749 added a commit to 23241a6749/last30days-skill that referenced this pull request Jul 14, 2026
… TypeError on arithmetic/sort/sum/max

dict.get(key, default) only substitutes the default when the key is
absent. A key that exists with value None passes through, producing
None instead of the intended default. This causes TypeError in:
- arithmetic (reactions + comments)
- list.sort() with key function
- max() comparisons
- sum() over generator expressions

Fixes 17 occurrences across 8 files:
- github.py: item.get("comments", 0) used in reactions + comments
- briefing.py: f.get("engagement_score", 0) used in max()/sum()
- youtube_yt.py: .get("views", 0) inside sort key
- instagram.py: same sort-key pattern
- tiktok.py: same sort-key pattern
- hackernews.py: .get("points", 0) in sort key
- signals.py: .get("views", 0) in engagement floor check
- store.py: .get("engagement_score", 0) / .get("relevance_score", 0)
  in store_sightings (same root cause as PR mvanhorn#796)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant