fix: dispatch service state change callbacks positionally

[bluetoothbot]

Summary

ServiceBrowser/AsyncServiceBrowser previously dispatched
state-change callbacks via **kwargs, forcing user handlers to name
their first parameter zeroconf — which shadows import zeroconf in
the caller's module. Switch to positional dispatch so users can pick
any parameter names they like.

Fixes #1092

Changes

• Signal.fire now accepts *args and forwards positionally.
• _ServiceBrowserBase._fire_service_state_changed_event passes the
  arguments positionally in the order
  (zeroconf, service_type, name, state_change) — matching the
  documented signature in examples/browser.py and friends.
• Updated the in-repo test handlers that had relied on keyword binding
  to use the canonical positional order.

Test plan

• New tests/services/test_browser.py::test_service_browser_handler_callback_arg_names_independent
  registers a callback whose parameters use non-default names and
  verifies they are invoked correctly without a TypeError.
• Full poetry run pytest --timeout=60 passes (390 passed, 2 skipped).

Compatibility note

This is a behaviour change for downstream callbacks that defined their
handler parameters in a different order than the documented one and
relied on keyword binding to sort them out. Handlers that follow the
example signature (zeroconf, service_type, name, state_change) are
unaffected.

---

Quality Report

Changes: 5 files changed, 61 insertions(+), 18 deletions(-)

Code scan: clean

Tests: passed (4 PASSED)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.77%. Comparing base (4ffba87) to head (4fa0b90).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1768   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files          33       33           
  Lines        3509     3509           
  Branches      493      493           
=======================================
  Hits         3501     3501           
  Misses          5        5           
  Partials        3        3           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codspeed-hq]

Merging this PR will not alter performance

✅ 14 untouched benchmarks

---

_{Comparing bluetoothbot:koan/fix-issue-1092 (4fa0b90) with master (4ffba87)}

[bdraco]

nice try, but has the same problem I pointed out in the issue. This is a breaking change so we can't merge it.

[bdraco]

@bluetoothbot rr

[bluetoothbot]

PR Review — fix: dispatch service state change callbacks positionally

The PR correctly identifies issue #1092 and the desired end state (handler parameter names should be user choice), but the implementation introduces a silent behaviour change for any downstream handler whose parameter order differs from the documented one. The diff itself demonstrates the breakage — ten in-tree handlers had to be reordered to keep tests green, all of which were previously valid because **kwargs bound by name. External users in the same situation will see no TypeError, just misrouted arguments, which is the worst failure mode. @bdraco's CHANGES_REQUESTED is on point; a backward-compatible path is needed (see reply on issue_comment 4502532996). Separately, the new test does not actually pin the originally reported shadowing concern.

---

🔴 Blocking

1. Breaking change: handlers binding by keyword stop working (`src/zeroconf/_services/__init__.py`, L57-59)

Switching Signal.fire from **kwargs to *args is a behaviour change for every downstream handler that does not declare its parameters in the exact order (zeroconf, service_type, name, state_change). Pre-change, fire(zeroconf=..., service_type=..., name=..., state_change=...) bound by keyword, so a handler defined as def cb(zeroconf, service_type, state_change, name) worked. After this change, that same handler silently receives name and state_change swapped — no TypeError, just wrong data flowing to user code. The diff itself contains the evidence: eight in-tree handlers had to be reordered (e.g. tests/services/test_browser.py:536, :562, :664, :785, :825, :868, :915, :1135, tests/test_asyncio.py:1009, tests/test_updates.py:58) because they relied on the keyword binding. Any external project doing the same will regress on upgrade, and because the failure is silent (no exception, just misrouted args) it will be hard to diagnose. This is the same concern @bdraco raised on the issue. A non-breaking path is needed — see the reply on the review comment for options.

def fire(self, *args: Any) -> None:
    for h in self._handlers[:]:
        h(*args)

🟡 Important

1. New test does not actually pin the reported issue (`tests/services/test_browser.py`, L966-1006)

test_service_browser_handler_callback_arg_names_independent only proves that positional dispatch works with renamed parameters — but that is true of any *args call site and does not exercise issue #1092's concern. The reported problem is that a handler module which does import zeroconf cannot also take a parameter named zeroconf without shadowing the module. A test that pins the fix should either (a) actually import zeroconf inside the handler module and reference the module from inside the callback body (proving the parameter name does not shadow it), or (b) verify that handlers with arbitrary parameter names — including ones that omit zeroconf entirely or rename it to e.g. zc — receive arguments in the documented order. The current assertion only checks that values flow through, which *args and **kwargs would both satisfy when the order happens to match.

def on_service_state_change(zc_arg, type_arg, name_arg, change_arg):
    if name_arg == registration_name:
        callbacks.append((zc_arg, type_arg, name_arg, change_arg))

2. `Signal` is generic — callers other than ServiceBrowser may break (`src/zeroconf/_services/__init__.py`, L57-59)

Signal is a general-purpose dispatcher; nothing in its definition ties it to the service-state-change use case. By dropping **kwargs support entirely, any other current or future caller that fires by keyword (or any third party that has reused Signal) is forced into positional dispatch. If the goal is specifically to fix the ServiceBrowser callback contract, consider scoping the change to _fire_service_state_changed_event (e.g. by introspecting the handler signature there, or by passing a dataclass/event object) rather than re-typing the generic Signal.fire API.

---

Checklist

• No hardcoded secrets
• Backward compatibility preserved — critical ＃1
• Tests pin the reported bug — warning ＃2
• Change is scoped to the problem — warning ＃3
• Error handling appropriate
• No resource leaks

---

Summary

The PR correctly identifies issue #1092 and the desired end state (handler parameter names should be user choice), but the implementation introduces a silent behaviour change for any downstream handler whose parameter order differs from the documented one. The diff itself demonstrates the breakage — ten in-tree handlers had to be reordered to keep tests green, all of which were previously valid because **kwargs bound by name. External users in the same situation will see no TypeError, just misrouted arguments, which is the worst failure mode. @bdraco's CHANGES_REQUESTED is on point; a backward-compatible path is needed (see reply on issue_comment 4502532996). Separately, the new test does not actually pin the originally reported shadowing concern.

---

To rebase specific severity levels, mention me: @bluetoothbot rebase critical (fixes 🔴 only), @bluetoothbot rebase important (fixes 🔴 + 🟡), or just @bluetoothbot rebase for all.

---

Automated review by Kōan4fa0b90

[bluetoothbot]

@bdraco: @bluetoothbot rr

Agreed — the breaking-change concern is concrete, and the diff makes it visible: ten in-tree handlers (e.g. tests/services/test_browser.py:536, :562, :664, :785, :825, :868, :915, :1135, tests/test_asyncio.py:1009, tests/test_updates.py:58) had to be reordered from (zeroconf, service_type, state_change, name) to (zeroconf, service_type, name, state_change) because they previously relied on keyword binding to sort the arguments out. Anyone downstream with the same handler shape will silently get name and state_change swapped after upgrading — no TypeError, just bad data, which is the worst failure mode.

A few non-breaking paths that would still solve #1092:

1. Keep **kwargs, rename the offending kwarg. Issue ServiceBrowser handler argument overwrites zeroconf name #1092's actual complaint is that zeroconf= shadows import zeroconf in the caller's module. Renaming the keyword to something neutral (zc= or instance=) while still passing all four by keyword would resolve the shadowing without changing argument-binding semantics. This is still a breaking change for handlers that named their first param zeroconf, but at least the failure is a loud TypeError (unexpected keyword), which is much easier to diagnose than silently swapped args.
2. Signature-introspecting dispatch. In _fire_service_state_changed_event, inspect each handler's signature once at registration time (or cache via inspect.signature) and call it with positional or keyword args accordingly. Handlers with **kwargs/explicit keyword params get kwargs; positional-only handlers get positional. Adds a small upfront cost but is fully backward-compatible and unblocks the rename use case.
3. Pass an event/dataclass object. Introduce a new signal variant or an event= parameter that bundles (zeroconf, service_type, name, state_change) into a single object, deprecate the old keyword form over a release cycle, and document the new shape in examples/browser.py. This sidesteps the parameter-name question entirely.

Option 2 is probably the smallest behaviour-preserving change; option 3 is the cleanest long-term API but needs a deprecation cycle. Happy to take whichever direction you prefer — would you like me to redo this against option 2?

[bdraco]

Let's close this. This PR clearly demonstrates all options are breaking changes and documents why we can't do what the issue reporter requested