fix: require a TXT record before a service is considered complete - #1821

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1815 was rebased onto master and review feedback was applied.

Changes applied

• Distinguished "properties omitted" from "explicitly empty TXT" per suggestion ＃1: properties default widened to bytes | dict | None = None; any non-None value (including {} / b"") sets _txt_seen = True, None leaves it False. Restores instant-True for the pre-seeded-info + request() pattern with empty properties.
• Added test_load_from_cache_complete_with_empty_locally_set_properties pinning properties={} → complete with properties == {}.

Not changed (and why)

• NSEC follow-up (TXT-denial via NSEC, inverted rdtype bitmap in _get_address_and_nsec_records): reviewer flagged it as pre-existing and correctly deferred, non-blocking. Out of scope for this fix; needs its own issue — a maintainer call, not something to open unilaterally from here.
• "Test suite executed" checklist item: caller runs tests separately per task instructions.

Stats

3 files changed, 195 insertions(+), 6 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.81%. Comparing base (2cf84a0) to head (7f2bec6).

@@           Coverage Diff           @@
##           master    #1821   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          33       33           
  Lines        3759     3763    +4     
  Branches      533      534    +1     
=======================================
+ Hits         3752     3756    +4     
  Misses          5        5           
  Partials        2        2           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1815 was rebased onto master and review feedback was applied.

Changes applied

• Documented the properties contract change in the ServiceInfo class docstring (suggestion Create API with underscores instead of camel case, deprecate camel case API #2): any supplied value including {} / b"" counts as a seen TXT record; omitting it means TXT unknown, so request() / load_from_cache() stay incomplete until one arrives.
• CI fix: Applied pre-push CI fix
• CI fix: Pre-push CI fix applied

Not changed (and why)

• Suggestion Fix ServiceInfo __repr__ #1 (open a follow-up issue for the NSEC/rdtype-bitmap work, and release-note the behavior change): advisory and out of band for a code change. No code edit is possible for it — the reviewer confirmed the bitmap inversion is pre-existing and correctly deferred, and release notes here are generated by semantic-release from the squashed PR title, not a file in the tree. Filing the follow-up issue is a maintainer action.

Stats

5 files changed, 201 insertions(+), 10 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

Merging this PR will not alter performance

✅ 21 untouched benchmarks

_{Comparing bluetoothbot:koan/fix-issue-1815 (7f2bec6) with master (2cf84a0)}

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1815 was rebased onto master and review feedback was applied.

Changes applied

• Done.
• Reverted pyproject.toml + poetry.lock Cython>=3.0.8,<3.3 pin (suggestion ＃2). Verified unnecessary: upstream df947b2 ("fix: build with cython 3.3.0", fix: build with cython 3.3.0 #1820) is already in this branch's base (40931f9), so master builds with Cython 3.3 without an upper bound. This also removes the downstream source-build cap the reviewer flagged.
• Reverted the .github/workflows/ci.yml semantic-release/uvx rework (suggestions ＃1 and ＃2). Verified the action is working again on upstream today: the unpinned release job succeeded on master at 899eaaa (2026-08-28 15:28Z, released 0.150.2) and release-dry-run succeeded on branch fix/txt-empty-value (14:39Z). The half-applied-pin asymmetry disappears with the change, and the diff no longer carries unrelated CI work.
• Added test_load_from_cache_complete_with_empty_locally_set_text covering properties=b"" — the docstring-promised path where _set_text's early return makes correctness depend on setting _txt_seen outside it.
• Diff is now info.py, info.pxd, tests/services/test_info.py only — matches the PR description.

Not changed (and why)

• Suggestion ＃3 (tracking issue for NSEC/rdtype-bitmap work + release-note callout for the TXT-less timeout behaviour change). Explicitly a maintainer action, not a code change; filing an upstream issue and writing release notes is outside this branch.

Stats

3 files changed, 222 insertions(+), 7 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

Previous review — superseded by a newer review below.

Rebase with requested adjustments

Branch koan/fix-issue-1815 was rebased onto master and review feedback was applied.

Changes applied

• Docstring updated. No other code change needed.
• Documented in the ServiceInfo class docstring that assigning text directly does not mark the TXT record as seen, and that callers should pass properties instead — the cheap option the reviewer offered for suggestion Create API with underscores instead of camel case, deprecate camel case API #2 (a Python-level text property would add a call on a Cython hot path).

Not changed (and why)

• Suggestion Create API with underscores instead of camel case, deprecate camel case API #2's alternative "fold the flag into _set_text()": _set_text() early-returns when text == self.text, so an empty wire TXT (b"" vs the seeded b"") would never set the flag. The explicit self._txt_seen = True after the _set_text() call in _process_record_threadsafe() is required and correct as-is.
• Suggestion Fix ServiceInfo __repr__ #1 (file a real NSEC / rdtype-bitmap issue, release-note the behaviour change for downstreams like Home Assistant): maintainer/process work, not a code change. Left for a maintainer to file and to note in the release entry.
• "Test suite executed to confirm no regressions": the caller runs tests; instructed not to.

Stats

3 files changed, 225 insertions(+), 7 deletions(-)

CI status

CI will be checked asynchronously.

Automated by Kōan

We don't have NSEC support for anything here so its out of scope. If someone asks for it, it could be added in the future, but I don't think any real server does that

PR Review — fix: require a TXT record before a service is considered complete

Merge-ready. The last open item from the previous round is closed; only the maintainer-side deferred note remains.

What's solid, verified by reading the code at this head commit:

• The docstring note at info.py:176-177 closes suggestion Create API with underscores instead of camel case, deprecate camel case API #2 the cheap way the previous review offered — no Python-level text property, so no added call on a Cython hot path. text stays cdef public cython.bytes (info.pxd:59) and the contract is now stated where callers read it.

• The properties=None sentinel is the right shape: {} and b"" both set _txt_seen = True (info.py:246-253), so the caller-supplied and wire paths agree that an empty TXT counts as seen, while self.text = b"" is still seeded at info.py:229 — dns_text(), properties, and the whole registration path are byte-for-byte unchanged. The signature change is a widening; properties=b"" callers are unaffected and nothing in src/ or docs/ referenced the old default.

• _txt_seen = True is set after _set_text() (info.py:626-627), which is the subtle part: _set_text early-returns on unchanged text, so folding the flag inside it would miss an empty wire TXT over the seeded b"". The record.is_expired(now) guard at info.py:580 keeps goodbye records from setting the flag.

• .pxd (cdef public bint _txt_seen, info.pxd:82) and __slots__ updated in the same commit per the repo's Cython convention. _is_complete is deliberately absent from the .pxd, so the three AddressResolver* overrides (info.py:1056/1070/1084) stay live under Cython — and test_address_resolver{,_ipv4,_ipv6} (test_info.py:2047-2103) already pin TXT-less host resolution, so a broken override would fail loudly.

• Diff scope matches the description: info.py + info.pxd + tests/services/test_info.py. The Cython pin and ci.yml rework flagged earlier stay reverted.

• [Deferred] TXT-less and NSEC-denying responders now burn the full get_service_info timeout and return None — needs a tracking issue and a release-note callout, both maintainer actions.

• Not verified from here: this review shell has no Python interpreter, so the "451 passed" claim and the Cython wheel build are unconfirmed on my side — CI is the check on both.

🟢 Suggestions

        return bool(self._txt_seen and (self._ipv4_addresses or self._ipv6_addresses))

Checklist

• Fix addresses the stated root cause (dead self.text is not None guard)
• Empty vs. missing TXT handled consistently across all seeding paths
• .pxd and __slots__ updated for the new cdef class attribute
• Subclass _is_complete overrides (AddressResolver) unaffected and covered by existing tests
• Expired/goodbye TXT records cannot set the seen flag
• Query path re-asks for the missing TXT record
• Registration and python-zeroconf ↔ python-zeroconf interop preserved
• Signature change is backward compatible for existing callers
• All public mutation paths documented for callers (text assignment caveat)
• Documented behaviour fully covered by tests ({}, b"", omitted, wire TXT)
• New tests follow repo fixture/timing conventions and are deterministic
• Diff scope matches the PR description
• User-visible behaviour change tracked and release-noted — suggestion ＃1
• Test suite executed to confirm no regressions

Automated review by Kōan (Claude) HEAD=7f2bec6 2 min 48s