[chrismoorhouse]

Summary

Fixes #88.

_processServiceRequest() drops every incoming service request whose sender address is 127.0.0.1. When a BACnet router or device runs on the same host (e.g. bacnet-stack router-mstp bridging BACnet/IP on loopback to MS/TP), everything it forwards — including unconfirmed COV notifications — arrives from localhost and is silently discarded, so events like covNotifyUnconfirmed never fire even though the SubscribeCOV itself is acknowledged.

Change

Restrict the filter to the client's own address and port (127.0.0.1 + :port suffix for non-default ports, matching the transport's sender formatting). Genuine same-port self-echoes are already de-duplicated byte-for-byte by the transport layer (_lastSendMessages), so the original intent of the check is preserved.

Verification

• npm run build clean
• npm run test:unit — 204/204 pass
• Tested end-to-end on a loopback router topology (client on 127.0.0.1:47809, bacnet-stack router-mstp on lo:47808 ↔ MS/TP): before this change COV notifications were dropped with Received and skipped localhost service request; after it, covNotifyUnconfirmed fires and notifications flow.

[Copilot]

Copilot review overview

🟡 Changes recommended

The behavior fix needs automated regression coverage for same-host requests and self-echo filtering.

Review effort: Balanced
Findings: 1

Open (1)

• Add regression tests for localhost port filtering · New

What changed in this PR

Fixes localhost service requests being incorrectly discarded when sent by same-host BACnet routers.

Changes:

• Filters only requests matching the client’s localhost address and port.
• Updates diagnostic logging to distinguish self-originated requests.

File	Description
src/​lib/​client.ts	Narrows localhost service-request filtering to the client socket.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[robertsLando]

Thanks, the fix is correct for the case in #88 and narrower than before. Two things before merge: regression tests and the comment wording. The rest is optional.

What the old filter actually did. The transport formats sender.address as rinfo.address plus :port only when the port isn't 47808 (src/lib/transport.ts:51-54). So the old === '127.0.0.1' check only skipped traffic from 127.0.0.1:47808, not all of localhost:

• Client on 47808: old and new filter behave the same.
• Client on another port, router on 127.0.0.1:47808: router traffic was dropped before and now gets through. This is the fix.
• Client and router both on 47808 on the same host (possible since reuseAddr defaults to true): still dropped, and can't be told apart by address. Worth narrowing the title/description to say the fix applies when the client runs on a non-default port.

Requested: regression tests. Nothing in test/ exercises the self-skip in _processServiceRequest, so reverting to the blanket check or dropping it entirely would keep CI green. test/unit/client.spec.ts already builds clients via Object.create(BACnetClient.prototype) with a stub _settings, so the method can be called directly. Cases:

1. port 47809, sender 127.0.0.1 -> emitted
2. port 47809, sender 127.0.0.1:47809 -> skipped
3. port 47808, sender 127.0.0.1 -> skipped (no-suffix branch)
4. non-loopback sender -> emitted

Unchanged by this PR, for reference: own broadcast echoes come back from the LAN IP and are de-duplicated in the transport (transport.ts:33-43); the socket is udp4, so IPv6-mapped loopback never shows up.

[robertsLando]

The last sentence overstates the transport de-dup. _lastSendMessages is only filled when send() has no receiver (transport.ts:84-96), i.e. broadcasts. A unicast to 127.0.0.1:<ownPort> is not de-duplicated there, and that's exactly the case this filter still exists for. As written, someone could read this and delete the check as redundant.

Suggest something like: "Skip unicast requests from this client's own socket. Broadcast echoes are de-duplicated by the transport layer."

[robertsLando]

Two small points (both optional):

• || DEFAULT_BACNET_PORT never applies: the constructor already sets the default (client.ts:215).
• "Own" comes from _settings.port, not the socket actually bound. With a custom options.transport bound to, say, 47810, _settings.port stays 47808, so a router on 47808 is dropped again while real self-traffic from :47810 isn't. Not a regression (same as before), but the transport already emits its real address on listening (transport.ts:58-62) and client.ts:239 discards it. Could be a follow-up.

[robertsLando]

Nit / follow-up: this is the third inline copy of the "omit :port when it's 47808" rule (also transport.ts:51-54 and bvlc.ts:83-85). If the transport format ever changes, selfAddress stops matching and self-requests silently stop being filtered. A shared formatter would keep them in sync.