Skip to content

Remove literal-address -s dialling; keep only -s mdns: discovery - #56

Merged
chrisuthe merged 1 commit into
mainfrom
chrisuthe/task/remove-literal-address-s-dialling-keep-only-s
Sep 15, 2026
Merged

chrisuthe merged 1 commit into
mainfrom
chrisuthe/task/remove-literal-address-s-dialling-keep-only-s

Conversation

@chrisuthe

Copy link
Copy Markdown
Member

The Sendspin spec has two ways to connect: the server discovers the player's _sendspin._tcp advertisement, or the player discovers a server on _sendspin-server._tcp and connects to it. Dialling a typed-in host[:port] or ws:// URL is neither, so -s / server = now take only mdns:[<name>].

Breaking: any other -s or server value (host, host:port, ws:// URL, bare mdns, empty) exits 1 at parse time with one error: line that says address dialling was removed and points to -s mdns:[<name>] or being discovered. From a config file it names the file and line. The value is never echoed, since it may hold credentials.

  • Remove parse_server_url(), redact_url_userinfo(), USERINFO_MASK, DEFAULT_REMOTE_SERVER_PORT and Options::server_url
  • OutboundMode always dials a discovered server; LastDial answers nothing for an empty server_id
  • --help, the no-mDNS parse error and the no-mDNS runtime line no longer offer an address
  • Drop the smoke test's credential-redaction check
  • Update the README, wiki, config example and systemd unit comments; add ROADMAP item 26

-s mdns: behaviour is unchanged: the advertisement is suppressed, browsing and last-server preference work as before, the retry backoff still applies, and SENDSPIN_SERVER_URL is still exported for the dialled server.

Testing

  • mDNS build: 417/417 unit tests
  • -DSENDSPIN_CLI_WITH_MDNS=OFF build: 416/416 unit tests
  • scripts/smoke_test.sh: every check passed
  • Checked by hand: each removed form exits 1 without echoing the value, and -s "mdns:Living Room" browses and finds servers on the LAN

The spec has a player connect only to a server it discovers, so a typed-in host, host:port or ws:// URL is now a parse error (from a config file too) that names the removal and never quotes the value. parse_server_url() and redact_url_userinfo() go with it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation consistently removes literal-address dialing while preserving and testing mDNS discovery behavior.

Pull request overview

Removes literal server dialing and restricts outbound connections to mDNS-discovered servers.

Changes:

  • Rejects non-mdns: server values without exposing them.
  • Simplifies outbound dialing and URL tracking.
  • Updates tests, documentation, examples, and packaging comments.
File summaries
File Description
src/cli.cpp Enforces discovery-only server parsing.
src/cli.h Removes literal-address APIs and state.
src/main.cpp Always uses discovery for outbound mode.
src/outbound.cpp Rejects unverifiable dial records.
src/outbound.h Updates LastDial contract.
src/mdns_common.cpp Updates IPv6 URL comment.
src/hooks.h Documents discovery-only URL export.
src/audio_sink.cpp Removes obsolete parser reference.
tests/cli_test.cpp Tests address rejection and secrecy.
tests/config_file_test.cpp Tests configuration behavior.
tests/discovery_test.cpp Tests updated LastDial semantics.
scripts/smoke_test.sh Removes obsolete redaction test.
README.md Documents mDNS-only usage.
packaging/sendspin-cli.service.in Updates service comment.
packaging/sendspin-cli.conf.example Updates configuration example.
docs/wiki/Troubleshooting.md Updates no-mDNS guidance.
docs/wiki/Home.md Describes discovery-based outbound mode.
docs/wiki/Getting-Started-on-Linux.md Updates setup instructions.
docs/wiki/Controlling-the-Player.md Clarifies exported server URL semantics.
docs/wiki/Configuration.md Documents accepted server values.
docs/wiki/Advanced-Usage.md Removes address-dialing examples.
docs/ROADMAP.md Records the shipped breaking change.
Review details
  • Files reviewed: 22/22 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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

@chrisuthe
chrisuthe marked this pull request as ready for review September 15, 2026 18:47
@chrisuthe
chrisuthe merged commit 9b47032 into main Sep 15, 2026
15 checks passed
chrisuthe added a commit that referenced this pull request Sep 17, 2026
The 'What the scaffold already does' line still said -s dials a literal
address; #56 removed that. Item 5 and item 26 stay as the historical record.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants