fix: honour the contract that justifies a silent swallow, and log state removal on change (#308, #312)

[simons-plugins]

Closes #308. Closes #312.

#308 — the silence was fine; the contract behind it was not implemented

I filed #308 claiming SurveyLog._persist's silent swallow was the wrong shape. Reading the code properly says otherwise — the silence is deliberate and documented:

Deliberately silent: the caller supplies the save hook and logs there if it wants to. A failed save costs one repeated INFO on the next start, which is a strictly better outcome than an exception escaping into create_devices.

And NodeDeviceTombstones._persist warns instead because its failure resurrects a deliberately-deleted device, which is not cosmetic. The asymmetry is reasoned and stands.

The actual defect is that DiagnosticsMenuMixin._save_survey_log — the hook that comment points at — had no try/except and no logging:

def _save_survey_log(self, blob: str) -> None:
    self.pluginPrefs[SURVEY_LOG_PREF] = blob
    indigo.server.savePluginPrefs()

So a raising savePluginPrefs() reached _persist, was swallowed, and nobody logged anything. The comment described a contract nothing honoured.

The hook now catches and warns, naming the failure and its consequence. _persist stays silent, and its comment now states a fact rather than an intention.

#312 — log on change, not on evaluation

The state-removal INFO reprinted on every rebuild with the same answer — ~3 lines per plugin start, every start, measured across a week of jarvis logs.

The filtering is correct and unchanged: Indigo calls getDeviceStateList on each rebuild, so re-applying it every time is right. Only the cadence was wrong. The comment justifying INFO — "a state disappearing silently breaks any trigger or control page bound to it" — holds the first time a device loses a set of states, not the fortieth reprint of an unchanged answer.

device_settings.RemovedStateLog records device id → fingerprint of the removed set, so a device that later loses a different state still logs. deviceDeleted forgets the entry for every device type. getDeviceStateList fails open to logging every time if the store is somehow unwired.

Why persisted rather than an in-memory latch: the jarvis data shows the repeats are per-restart, not within-session — an in-memory dict would have suppressed almost nothing.

Worth flagging

This makes three stores of the same "RLock + JSON blob in pluginPrefs" shape (NodeDeviceTombstones, SurveyLog, RemovedStateLog). A shared base was assessed and rejected at two during the refactor arc; the skeleton is still only ~20-25 lines each and the domain logic genuinely differs (a set, a fingerprint map, a different persist-failure policy per store).

If a third store to suppress a log line reads as too much machinery for the problem, the cheaper alternative is to demote the message to DEBUG and drop the store entirely. Say the word and I'll swap it.

Numbers

3882 passing on both 3.11 and 3.13 (3873 + 9 new). pylint 9.48, unchanged. No existing test changed or deleted — both touched test files are additions only.

Version 2026.28.9. No [release] marker.

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f7164495-9726-4d3f-b6c4-45a413d6252c

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}