Address Codex review on #18: all four findings were correct

**Entry title leaked the username (P1).** async_step_user names the entry
after the account, so the title *is* the username, and async_redact_data
only matches keys called username or password. "title" is now redacted
too.

My own test claimed to cover this and did not: the ConfigEntry double
defaulted to title="alice" while the test looked for
"alice@example.com", so the two never met. The double now models what
the config flow actually does.

**Free-form notes could reach a public issue (P1).** The sample bottle
denied three location fields and published everything else, including
prose someone wrote — tasting and cellar notes can say anything about
anyone. A denylist also has to be right about every column CellarTracker
adds in future.

The sample is now an allowlist of the fields the integration itself
reads or derives, which is what debugging it needs. Nothing is lost:
"columns" still lists every column name, and that is the signal schema
drift actually needs.

**Backoff could shorten a long schedule (P2).** The options schema sets
a minimum interval and no maximum, so a daily poll is configurable — and
capping at MAX_BACKOFF turned it into a six-hourly one while being rate
limited. Four times the requests, and the exact opposite of backing off,
in the method whose docstring promises never to poll sooner than
configured. The cap now bounds what the server can ask for, and the
configured interval is applied as the floor on top of it.

**The sync timestamp never reached its sensor (P2).** always_update=False
suppresses listener notification when a payload compares equal to the
previous one, and a cellar's inventory is identical between most polls.
The timestamp was held outside the payload, so it took part in no
comparison and the sensor would have shown the last time a *bottle*
changed while claiming to show the last successful sync — the feature
not working as described.

It is carried inside the payload now. That also means no two payloads
ever compare equal, so always_update=False could never suppress anything
and has been dropped rather than left as a flag that reads like it does
something.

9 tests added, 329 passing.

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

Author: claude

custom_components/cellar_tracker/cellar_data.py

@@ -260,7 +260,10 @@ def __init__(self, hass: HomeAssistant, entry: ConfigEntry):
             # entry, so unloading cancels a poll still in flight.
             config_entry=entry,
             update_interval=scan_interval,
-            always_update=False,
+            # Every payload now carries the time of the poll that produced it,
+            # so no two ever compare equal and this could never suppress an
+            # update. Left at the default rather than kept as a flag that
+            # reads like it does something.
         )
 
         # Consecutive polls that reported an empty cellar after it held stock.
@@ -332,7 +335,13 @@ def _backoff_for(self, retry_after: int | None) -> timedelta:
         """
         configured = int(self._scan_interval.total_seconds())
         seconds = retry_after if retry_after is not None else configured * 2
-        return timedelta(seconds=min(max(seconds, configured), MAX_BACKOFF))
+
+        # Cap what the *server* can ask for, then apply the configured interval
+        # as the floor. Doing it the other way round let the cap undercut a
+        # schedule longer than six hours - the options schema sets a minimum
+        # and no maximum, so a daily poll became six-hourly while being rate
+        # limited, which is the opposite of backing off.
+        return timedelta(seconds=max(min(seconds, MAX_BACKOFF), configured))
 
     def _restore_interval(self) -> None:
         """Undo a backoff once CellarTracker is answering again."""
@@ -508,7 +517,14 @@ async def _async_update_data(self) -> dict:
             # than leaving the coordinator to log a traceback.
             raise UpdateFailed(f"Malformed CellarTracker export: {err}") from err
 
+        # Carried inside the payload, not just alongside it. The coordinator
+        # compares payloads to decide whether to notify listeners, and a
+        # cellar's inventory is identical between most polls - so a timestamp
+        # held outside would never reach the sensor, and "last synchronised"
+        # would quietly come to mean "last time a bottle changed".
         self._last_success = dt_util.utcnow()
+        data["last_success"] = self._last_success
+
         self._restore_interval()
         return data
 

custom_components/cellar_tracker/diagnostics.py

@@ -3,9 +3,10 @@
 A diagnostics download is routinely pasted into a public issue, so the question
 is not only what would help us but what the user would regret publishing.
 
-Redacted: the password, and the username too - it is half of a credential pair
-and names a real CellarTracker account. Per bottle, the Barcode, Location and
-Bin, which describe someone's home and are no use in diagnosing a parsing bug.
+Redacted: the password, the username, and the entry title - which is the
+username, because that is what the config flow names the entry after. Per
+bottle, everything outside a small allowlist, so a column nobody reviewed
+cannot walk into a public issue.
 
 Kept: the column list, which is the signal that matters. Every "no 'iWine'
 column" report comes down to CellarTracker having changed its export, and the
@@ -21,9 +22,29 @@
 from homeassistant.const import CONF_PASSWORD, CONF_USERNAME
 from homeassistant.core import HomeAssistant
 
-TO_REDACT = {CONF_PASSWORD, CONF_USERNAME}
+# "title" as well as the credential keys: async_step_user names the entry
+# after the account, so the title *is* the username for every entry this
+# integration creates. Redacting only the username field would have published
+# it one line further down.
+TO_REDACT = {CONF_PASSWORD, CONF_USERNAME, "title"}
 
-BOTTLE_REDACT = {"Barcode", "Location", "Bin"}
+# The sample bottle is an allowlist rather than a denylist. A denylist has to
+# be right about every column CellarTracker has today and every one it adds
+# later, and the export already carries free-form prose - tasting and cellar
+# notes - that can say anything at all about anyone. These are the fields the
+# integration itself reads or derives, which is what debugging it needs.
+#
+# Nothing is lost by omitting the rest: "columns" below still lists every
+# column name, which is the signal schema drift actually needs.
+SAMPLE_FIELDS = (
+    "iWine",
+    "Wine",
+    "Vintage",
+    "Valuation",
+    "BeginConsume",
+    "EndConsume",
+    "unique_bottle_id",
+)
 
 
 async def async_get_config_entry_diagnostics(
@@ -51,6 +72,8 @@ async def async_get_config_entry_diagnostics(
         # One row is enough to show how the export is shaped. Shipping the whole
         # cellar would be both useless and a privacy problem.
         "sample_bottle": (
-            async_redact_data(bottles[0], BOTTLE_REDACT) if bottles else None
+            {field: bottles[0][field] for field in SAMPLE_FIELDS if field in bottles[0]}
+            if bottles
+            else None
         ),
     }

tests/test_diagnostics.py

@@ -21,7 +21,10 @@
 import asyncio
 import json
 
-from cellar_tracker.diagnostics import async_get_config_entry_diagnostics
+from cellar_tracker.diagnostics import (
+    SAMPLE_FIELDS,
+    async_get_config_entry_diagnostics,
+)
 from conftest import ConfigEntry, ViewHass
 
 PASSWORD = "hunter2-do-not-leak"
@@ -35,6 +38,9 @@
     "Barcode": "BC-77-4412",
     "Location": "Cellar under the stairs",
     "Bin": "A4",
+    "BottleNote": "bought for Anna's 40th, keep for her",
+    "CNotes": "cellar note naming the neighbour who has the spare key",
+    "PNotes": "private note",
     "unique_bottle_id": "abcdef0123456789",
 }
 
@@ -51,6 +57,9 @@ def __init__(self, data=None, last_update_success=True):
 def diagnostics(coordinator) -> dict:
     entry = ConfigEntry(
         entry_id="a",
+        # async_step_user sets the title to the username. The double defaulted
+        # to something else, which is why the leak below went unnoticed.
+        title=USERNAME,
         data={"username": USERNAME, "password": PASSWORD, "currency": "SEK"},
         options={"scan_interval": 21600},
     )
@@ -80,11 +89,12 @@ def test_the_bottle_sample_hides_where_the_wine_lives():
     report = diagnostics(_Coordinator(stocked()))
     sample = report["sample_bottle"]
 
-    # Asserted field by field rather than by searching the rendered report:
-    # a substring search gives false positives against unrelated values, and
-    # a short Bin like "A4" appears inside plenty of innocent text.
+    # Absent rather than redacted: the sample is an allowlist, so these were
+    # never copied in. Asserted field by field because a substring search over
+    # the rendered report gives false positives - a short Bin like "A4"
+    # appears inside plenty of innocent text.
     for field in ("Barcode", "Location", "Bin"):
-        assert sample[field] == "**REDACTED**", f"{field} was not redacted"
+        assert field not in sample, f"{field} must not reach the report"
 
     assert "Cellar under the stairs" not in rendered(report)
 
@@ -124,3 +134,38 @@ def test_a_coordinator_that_never_refreshed_produces_a_report():
     report = diagnostics(_Coordinator(None, last_update_success=False))
     assert report["totals"]["total_bottles"] is None
     assert report["sample_bottle"] is None
+
+
+# --------------------------------------------------------------------------
+# Reported by Codex on #18
+# --------------------------------------------------------------------------
+def test_the_entry_title_does_not_leak_the_username():
+    """The title *is* the username for every entry this integration creates."""
+    report = diagnostics(_Coordinator(stocked()))
+    assert USERNAME not in rendered(report)
+    assert report["entry"]["title"] == "**REDACTED**"
+
+
+def test_free_form_notes_never_reach_the_report():
+    """Tasting and cellar notes are prose someone wrote; they can say anything."""
+    sample = diagnostics(_Coordinator(stocked()))["sample_bottle"]
+
+    for field in ("BottleNote", "CNotes", "PNotes"):
+        assert field not in sample, f"{field} is free-form and must not be published"
+
+
+def test_the_sample_is_an_allowlist_not_a_denylist():
+    """A denylist ships every column CellarTracker adds in future, unreviewed."""
+    sample = diagnostics(_Coordinator(stocked()))["sample_bottle"]
+    assert set(sample) <= set(SAMPLE_FIELDS)
+
+
+def test_an_unknown_column_is_omitted_rather_than_published():
+    bottle = {**BOTTLE, "SomeColumnAddedNextYear": "who knows what this holds"}
+    report = diagnostics(
+        _Coordinator({"total_bottles": 1, "total_value": 0.0, "bottles": [bottle]})
+    )
+
+    assert "SomeColumnAddedNextYear" not in report["sample_bottle"]
+    # ...but its existence is still visible, which is what schema drift needs.
+    assert "SomeColumnAddedNextYear" in report["columns"]

tests/test_inventory_validation.py

@@ -29,6 +29,15 @@
 SINGLE_LINE_ERROR_PAGE = "<html><title>503 Service Unavailable</title></html>"
 
 STOCKED = {"total_bottles": 412, "total_value": 9000.0, "bottles": []}
+def inventory_of(data: dict) -> dict:
+    """The payload without the poll timestamp.
+
+    Every successful poll carries ``last_success`` so the coordinator's payload
+    comparison always differs; these tests are about the inventory it describes.
+    """
+    return {key: value for key, value in data.items() if key != "last_success"}
+
+
 EMPTY = {
     "total_bottles": 0,
     "total_value": 0.0,
@@ -183,7 +192,7 @@ def test_last_bottle_drunk_recovers_within_two_polls():
 
     with pytest.raises(UpdateFailed):
         asyncio.run(coordinator._async_update_data())
-    assert asyncio.run(coordinator._async_update_data()) == EMPTY
+    assert inventory_of(asyncio.run(coordinator._async_update_data())) == EMPTY
 
 
 # --------------------------------------------------------------------------
@@ -198,4 +207,4 @@ def test_coordinator_passes_previous_data_through():
 
 def test_coordinator_first_poll_of_empty_account_succeeds():
     coordinator = build_coordinator(returns=[], previous=None)
-    assert asyncio.run(coordinator._async_update_data()) == EMPTY
+    assert inventory_of(asyncio.run(coordinator._async_update_data())) == EMPTY

tests/test_last_sync_sensor.py

@@ -97,3 +97,40 @@ def test_the_coordinator_records_when_it_last_succeeded():
 
     assert coordinator.last_success is not None
     assert coordinator.last_success.tzinfo is not None
+
+
+# --------------------------------------------------------------------------
+# Reported by Codex on #18
+# --------------------------------------------------------------------------
+def test_the_timestamp_is_part_of_the_coordinator_payload():
+    """Otherwise always_update=False suppresses the update that carries it.
+
+    A cellar's inventory is identical between most polls, so the coordinator
+    compares the new payload equal to the old and notifies no listeners. A
+    timestamp held outside that payload therefore never reaches the sensor,
+    and "last synchronised" would silently mean "last time a bottle changed".
+    """
+    from cellar_tracker.cellar_data import WineCellarData
+    from conftest import FakeHass, FakeSession
+
+    export = "iWine\tWine\tValuation\n1\tBarolo\t45.50"
+    hass = FakeHass()
+    hass.session = FakeSession(text=export)
+    coordinator = WineCellarData(hass, ConfigEntry(data={"username": "a", "password": "b"}))
+
+    first = asyncio.run(coordinator._async_update_data())
+    coordinator.data = first
+    second = asyncio.run(coordinator._async_update_data())
+
+    assert "last_success" in first
+    assert first["bottles"] == second["bottles"], "the fixture must be unchanged"
+    assert first != second, (
+        "two polls of an unchanged cellar produced equal payloads, so the "
+        "coordinator would notify no listeners and the timestamp would stall"
+    )
+
+
+def test_the_sensor_reads_the_timestamp_the_payload_carries():
+    stamp = datetime(2026, 9, 2, 8, 30, tzinfo=UTC)
+    sensor = build(_Coordinator(last_success=stamp))
+    assert sensor.native_value == stamp

tests/test_rate_limit.py

@@ -116,3 +116,43 @@ def test_throttling_is_logged_without_alarm(caplog):
 
     assert "429" in caplog.text
     assert not [r for r in caplog.records if r.levelname in ("WARNING", "ERROR")]
+
+
+# --------------------------------------------------------------------------
+# Reported by Codex on #18: the cap must never undercut the configured interval
+# --------------------------------------------------------------------------
+DAILY = 86400
+
+
+def daily(**session_kwargs) -> WineCellarData:
+    hass = FakeHass()
+    hass.session = FakeSession(**session_kwargs)
+    entry = ConfigEntry(
+        data={"username": "alice", "password": "s3cret", "scan_interval": DAILY}
+    )
+    return WineCellarData(hass, entry)
+
+
+def test_a_daily_schedule_is_not_shortened_by_the_cap():
+    """The options schema sets a floor, not a ceiling, so this is reachable.
+
+    Capping at MAX_BACKOFF turned a 24-hour schedule into a six-hourly one
+    *while being rate limited* - four times the requests, and the exact
+    opposite of backing off.
+    """
+    coordinator = daily(raise_for_status=throttled("1800"))
+    refresh(coordinator)
+    assert coordinator.update_interval >= timedelta(seconds=DAILY)
+
+
+def test_a_daily_schedule_with_no_retry_after_is_not_shortened():
+    coordinator = daily(raise_for_status=throttled(None))
+    refresh(coordinator)
+    assert coordinator.update_interval >= timedelta(seconds=DAILY)
+
+
+def test_a_server_still_cannot_extend_a_daily_schedule_without_bound():
+    """The cap still applies on top of the configured interval, not under it."""
+    coordinator = daily(raise_for_status=throttled("604800"))
+    refresh(coordinator)
+    assert coordinator.update_interval == timedelta(seconds=DAILY)