Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in the new reachability probe implementation and wiring (notably STUN attribute decoding control flow and IPv6/socket behavior) plus a misleading analyzer release note entry.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds two pieces of “pre-gate” multiplayer work across the codebase: (1) a new generator warning that checks declared client→server bandwidth against a recorded design budget, and (2) an opt-in, developer-mode-only launcher NAT reachability probe with structurally address-free results and reporting held closed.
Changes:
- Add
TFMP014warning in the multiplayer contract generator for declared inbound bandwidth budget overflow, with generator tests and docs. - Add reachability probe domain/data implementation (STUN binding + RFC 5780 behavior discovery), persistence + policy gates, and a Dev-only Flutter pane to run it locally.
- Expand documentation around privacy/capabilities and the multiplayer feasibility gate with the new partial-evidence records (including internal decision records).
File summaries
| File | Description |
|---|---|
| tests/TopiaForge.Mods.Multiplayer.Generators.Tests/Program.cs | Adds tests for the new TFMP014 inbound budget diagnostic and ensures it doesn’t affect lock identity. |
| src/TopiaForge.Mods.Multiplayer.Generators/MultiplayerContractGenerator.cs | Implements per-connection inbound design budget constant + TFMP014 warning logic. |
| src/TopiaForge.Mods.Multiplayer.Generators/AnalyzerReleases.Unshipped.md | Documents the newly added analyzer rule. |
| packages/launcher_domain/test/reachability_probe_test.dart | Unit tests for reachability classification, policy gates, and report shape. |
| packages/launcher_domain/lib/src/reachability_probe.dart | Domain types for NAT observation/classification (address-free by construction). |
| packages/launcher_domain/lib/src/reachability_probe_policy.dart | Opt-in settings, refusal reasons, reporting gate constant, and gateway API. |
| packages/launcher_domain/lib/launcher_domain.dart | Exports the new reachability probe domain APIs. |
| packages/launcher_data/test/reachability_probe_test.dart | Data-layer tests for STUN codec, server parsing, runner sequencing, and service persistence/gating. |
| packages/launcher_data/lib/src/reachability/stun_transport.dart | UDP STUN transport abstraction + implementation for the probe. |
| packages/launcher_data/lib/src/reachability/stun_message.dart | Minimal STUN codec (binding req/resp) used by the probe. |
| packages/launcher_data/lib/src/reachability/reachability_probe_runner.dart | Runs the STUN transaction sequence and reduces it to boolean evidence. |
| packages/launcher_data/lib/src/reachability_probe_service.dart | Persists opt-in settings, enforces policy, and runs one local probe. |
| packages/launcher_data/lib/launcher_data.dart | Exports reachability probe data-layer APIs/services. |
| docs/PrivacyAndCapabilities.md | Records the decision around IP exposure and capability disclosure for future transport work. |
| docs/MultiplayerHostingFeasibility.md | Updates gate notes with where partial evidence lives (including TFMP014 and probe). |
| docs/Multiplayer.md | Adds documented bandwidth budget + explains what TFMP014 does/doesn’t cover. |
| docs/internal/MultiplayerTransportOptions.md | Internal decision record covering vendor analysis, budgets, topology, and spend controls. |
| docs/internal/LauncherReachabilityProbe.md | Internal design/approval dependency record for the launcher probe. |
| apps/topiaforge_launcher_flutter/test/widget_test.dart | Wires new reachability widget test cases into the suite. |
| apps/topiaforge_launcher_flutter/test/widget_reachability_test_cases.dart | Widget tests for Dev-only reachability pane behavior and opt-in gates. |
| apps/topiaforge_launcher_flutter/lib/src/screens/developer_screen.dart | Adds the reachability pane to the Dev screen layout. |
| apps/topiaforge_launcher_flutter/lib/src/screens/developer_reachability_pane.dart | New Dev-only UI pane to enable/run the probe and show results locally. |
| apps/topiaforge_launcher_flutter/lib/src/screens.dart | Registers the new Dev reachability pane part. |
| apps/topiaforge_launcher_flutter/lib/src/launcher_state.dart | Adds reachability probe settings and last-run result to state. |
| apps/topiaforge_launcher_flutter/lib/src/launcher_reachability_actions.dart | Adds bloc handlers for probe settings + running the probe. |
| apps/topiaforge_launcher_flutter/lib/src/launcher_event.dart | Adds reachability-related launcher events. |
| apps/topiaforge_launcher_flutter/lib/src/launcher_event_dispatch.dart | Wires reachability events into the central dispatch switch. |
| apps/topiaforge_launcher_flutter/lib/src/launcher_bloc.dart | Adds probe gateway dependency + loads settings during initial load. |
| apps/topiaforge_launcher_flutter/lib/src/launcher_bloc_helpers.dart | Extracts shared _guard + snapshot projection helpers from launcher_bloc.dart. |
| apps/topiaforge_launcher_flutter/lib/src/launcher_app.dart | Plumbs ReachabilityProbeGateway? into the bloc from the app root. |
| apps/topiaforge_launcher_flutter/lib/main.dart | Instantiates ReachabilityProbeService and reads TOPIAFORGE_REACHABILITY_SERVERS. |
Review details
- Files reviewed: 31/31 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d52fe85 to
96777e3
Compare
`UdpStunTransport.bind` always bound `anyIPv4`, but `StunServerList` parses bracketed IPv6 literals and hands them straight to `send`. A configured IPv6 server therefore reached an unconnected IPv4 socket, where `send` throws rather than reporting a miss; the service's `SocketException` catch turned that into "Could not open a UDP socket", which is not what happened. Binding for a family exposed the deeper version of the same problem. A run uses one unconnected socket for every transaction and decides mapping by comparing reflexive endpoints across a server's address and port. Endpoints in different families are not comparable, so a mixed server list produces a mapping verdict that means nothing — the IPv4-only socket was accidentally hiding that by making every IPv6 entry fail. The family is therefore a property of the run. `StunTransportFactory` takes it, `bind` selects `anyIPv4` or `anyIPv6` and keeps only same-family interface addresses for `matchesLocalEndpoint`, and the service derives it from the configured servers and refuses a list that mixes families instead of quietly probing one of them. `anyIPv6` is not dual-stack everywhere — Windows defaults `IPV6_V6ONLY` on and Dart cannot clear it on a datagram socket — so choosing beats assuming. `request` also drops a wrong-family destination rather than letting `send` throw, since the transport's contract is that a transaction reports evidence and never raises. `reachability_probe_test.dart` reached 560 lines with the new cases, past the 500-line cap CI enforces, so the service group moves to a part file. launcher_data 375/375. Reported by Copilot on #68.
…llowed `_guard` emits its success message only while `state.isBusy` is still set, so a callback that clears `isBusy` itself has to supply its own message. Every other handler in the bloc does one or the other. The reachability settings-changed handler did neither: it emitted `isBusy: false` with no `statusMessage`, so saving the opt-in reported nothing at all and 'Reachability probe settings saved.' was dead text. It now leaves `isBusy` alone and lets `_guard` emit. The probe-run handler is unchanged; it already supplies its own message alongside `isBusy: false`, which is the other half of the same contract. The existing opt-in widget test now asserts the message reaches the status bar, and fails on the previous code. Flutter launcher 59/59. Reported by Copilot on #68.
The Notes column in this file describes each rule's invariant, not its emit condition — TFMP008 reads "Generated command codecs fit the declared payload limit" and fires when they do not. TFMP014's row followed that convention but read as a bare statement of fact, which is easy to take for the condition that produces the warning. "must fit" keeps the convention and removes the ambiguity, matching TFMP001's "must be non-generic top-level partial classes". Raised by Copilot on #68 as an inverted note; the table's convention is what made it read that way.
b790a2d to
024e812
Compare
96777e3 to
39b4dc4
Compare
`UdpStunTransport.bind` always bound `anyIPv4`, but `StunServerList` parses bracketed IPv6 literals and hands them straight to `send`. A configured IPv6 server therefore reached an unconnected IPv4 socket, where `send` throws rather than reporting a miss; the service's `SocketException` catch turned that into "Could not open a UDP socket", which is not what happened. Binding for a family exposed the deeper version of the same problem. A run uses one unconnected socket for every transaction and decides mapping by comparing reflexive endpoints across a server's address and port. Endpoints in different families are not comparable, so a mixed server list produces a mapping verdict that means nothing — the IPv4-only socket was accidentally hiding that by making every IPv6 entry fail. The family is therefore a property of the run. `StunTransportFactory` takes it, `bind` selects `anyIPv4` or `anyIPv6` and keeps only same-family interface addresses for `matchesLocalEndpoint`, and the service derives it from the configured servers and refuses a list that mixes families instead of quietly probing one of them. `anyIPv6` is not dual-stack everywhere — Windows defaults `IPV6_V6ONLY` on and Dart cannot clear it on a datagram socket — so choosing beats assuming. `request` also drops a wrong-family destination rather than letting `send` throw, since the transport's contract is that a transaction reports evidence and never raises. `reachability_probe_test.dart` reached 560 lines with the new cases, past the 500-line cap CI enforces, so the service group moves to a part file. launcher_data 375/375. Reported by Copilot on #68.
…llowed `_guard` emits its success message only while `state.isBusy` is still set, so a callback that clears `isBusy` itself has to supply its own message. Every other handler in the bloc does one or the other. The reachability settings-changed handler did neither: it emitted `isBusy: false` with no `statusMessage`, so saving the opt-in reported nothing at all and 'Reachability probe settings saved.' was dead text. It now leaves `isBusy` alone and lets `_guard` emit. The probe-run handler is unchanged; it already supplies its own message alongside `isBusy: false`, which is the other half of the same contract. The existing opt-in widget test now asserts the message reaches the status bar, and fails on the previous code. Flutter launcher 59/59. Reported by Copilot on #68.
The Notes column in this file describes each rule's invariant, not its emit condition — TFMP008 reads "Generated command codecs fit the declared payload limit" and fires when they do not. TFMP014's row followed that convention but read as a bare statement of fact, which is easy to take for the condition that produces the warning. "must fit" keeps the convention and removes the ambiguity, matching TFMP001's "must be non-generic top-level partial classes". Raised by Copilot on #68 as an inverted note; the table's convention is what made it read that way.
024e812 to
251a178
Compare
39b4dc4 to
abbcf21
Compare
`UdpStunTransport.bind` always bound `anyIPv4`, but `StunServerList` parses bracketed IPv6 literals and hands them straight to `send`. A configured IPv6 server therefore reached an unconnected IPv4 socket, where `send` throws rather than reporting a miss; the service's `SocketException` catch turned that into "Could not open a UDP socket", which is not what happened. Binding for a family exposed the deeper version of the same problem. A run uses one unconnected socket for every transaction and decides mapping by comparing reflexive endpoints across a server's address and port. Endpoints in different families are not comparable, so a mixed server list produces a mapping verdict that means nothing — the IPv4-only socket was accidentally hiding that by making every IPv6 entry fail. The family is therefore a property of the run. `StunTransportFactory` takes it, `bind` selects `anyIPv4` or `anyIPv6` and keeps only same-family interface addresses for `matchesLocalEndpoint`, and the service derives it from the configured servers and refuses a list that mixes families instead of quietly probing one of them. `anyIPv6` is not dual-stack everywhere — Windows defaults `IPV6_V6ONLY` on and Dart cannot clear it on a datagram socket — so choosing beats assuming. `request` also drops a wrong-family destination rather than letting `send` throw, since the transport's contract is that a transaction reports evidence and never raises. `reachability_probe_test.dart` reached 560 lines with the new cases, past the 500-line cap CI enforces, so the service group moves to a part file. launcher_data 375/375. Reported by Copilot on #68.
…llowed `_guard` emits its success message only while `state.isBusy` is still set, so a callback that clears `isBusy` itself has to supply its own message. Every other handler in the bloc does one or the other. The reachability settings-changed handler did neither: it emitted `isBusy: false` with no `statusMessage`, so saving the opt-in reported nothing at all and 'Reachability probe settings saved.' was dead text. It now leaves `isBusy` alone and lets `_guard` emit. The probe-run handler is unchanged; it already supplies its own message alongside `isBusy: false`, which is the other half of the same contract. The existing opt-in widget test now asserts the message reaches the status bar, and fails on the previous code. Flutter launcher 59/59. Reported by Copilot on #68.
The Notes column in this file describes each rule's invariant, not its emit condition — TFMP008 reads "Generated command codecs fit the declared payload limit" and fires when they do not. TFMP014's row followed that convention but read as a bare statement of fact, which is easy to take for the condition that produces the warning. "must fit" keeps the convention and removes the ambiguity, matching TFMP001's "must be non-generic top-level partial classes". Raised by Copilot on #68 as an inverted note; the table's convention is what made it read that way.
251a178 to
7067cb8
Compare
abbcf21 to
b9b07cd
Compare
|
Not ready to merge, but worth keeping — this needs retargeting rather than closing. The feature itself looks sound; the branch it sits on does not.
|
| check runs | |
|---|---|
this PR's head 7067cb8 |
1 (Required / Registry validation) |
dev head |
19 |
The workflows never fired because the base branch does not match their trigger. In particular CodeQL has never run on these 3,201 lines, which include a hand-rolled STUN binary parser and a raw UDP socket. The codeql-high-critical ruleset has no bypass actors, so if that analysis surfaces a high alert, the PR becomes unmergeable by anyone — better to learn that now than after a rebase.
Three commits are already superseded
a5ab363 and b935fff are #67's work, now on dev via #84. abbcf21's docs/CustomWorlds.md link fix is on dev verbatim. These should be dropped, not merged — they are the entire source of the 5 conflicts.
The four commits that are actually yours: a2bd6fa, d9d9f90, e50cc91, 7067cb8.
Suggested path
git rebase --onto origin/dev abbcf21 feat/launcher-reachability-probe
gh pr edit 68 --base dev
That drops the three superseded commits and keeps the four probe commits, after which the full matrix runs for the first time.
One thing to check when it does
P0-PRIV-01 is blocking and was explicitly not softened for 0.x. A developer flag gates activation, not classification — please confirm the capability declarations cover outbound STUN traffic, and that nothing contacts a host before the flag is enabled. Worth settling before the register has to answer it.
|
Unity PR artifacts are ready.
Workflow run: Unity Source Validation #281 Artifacts are temporary and expire according to the workflow retention policy. |
The multiplayer transport cost model turns on P(host unreachable): relay demand is P x (lobby size - 1), drawn once per lobby because reachability is a property of the host. Across plausible P the monthly relay volume spans about a factor of five, which dwarfs any per-byte optimisation available to us, and nobody has measured P. Published NAT-type distributions come from populations that do not resemble ours, so guessing at P is guessing at the answer rather than at a rounding error. The launcher can measure it without anything the hosting feasibility gate blocks: it never touches Robotopia, loads the game, runs a session, or needs a headless server or a transport. It probes the launcher's own host, which is the host a session would run on anyway, and it already makes network calls for update checks and the package registry, so the capability surface is not new. Deliberately not an ICE agent: no candidate gathering, no pairing, no connectivity checks, no nomination. STUN binding requests only. Reporting stays blocked and the pane sits behind the developer flag, so nothing leaves a machine yet. Two internal decision records land with it; they stay under docs/internal/ because they carry monetary figures and that directory is excluded from release zips. The multiplayer contract generator gains a matching analyzer rule. Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
`UdpStunTransport.bind` always bound `anyIPv4`, but `StunServerList` parses bracketed IPv6 literals and hands them straight to `send`. A configured IPv6 server therefore reached an unconnected IPv4 socket, where `send` throws rather than reporting a miss; the service's `SocketException` catch turned that into "Could not open a UDP socket", which is not what happened. Binding for a family exposed the deeper version of the same problem. A run uses one unconnected socket for every transaction and decides mapping by comparing reflexive endpoints across a server's address and port. Endpoints in different families are not comparable, so a mixed server list produces a mapping verdict that means nothing — the IPv4-only socket was accidentally hiding that by making every IPv6 entry fail. The family is therefore a property of the run. `StunTransportFactory` takes it, `bind` selects `anyIPv4` or `anyIPv6` and keeps only same-family interface addresses for `matchesLocalEndpoint`, and the service derives it from the configured servers and refuses a list that mixes families instead of quietly probing one of them. `anyIPv6` is not dual-stack everywhere — Windows defaults `IPV6_V6ONLY` on and Dart cannot clear it on a datagram socket — so choosing beats assuming. `request` also drops a wrong-family destination rather than letting `send` throw, since the transport's contract is that a transaction reports evidence and never raises. `reachability_probe_test.dart` reached 560 lines with the new cases, past the 500-line cap CI enforces, so the service group moves to a part file. launcher_data 375/375. Reported by Copilot on #68. Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
…llowed `_guard` emits its success message only while `state.isBusy` is still set, so a callback that clears `isBusy` itself has to supply its own message. Every other handler in the bloc does one or the other. The reachability settings-changed handler did neither: it emitted `isBusy: false` with no `statusMessage`, so saving the opt-in reported nothing at all and 'Reachability probe settings saved.' was dead text. It now leaves `isBusy` alone and lets `_guard` emit. The probe-run handler is unchanged; it already supplies its own message alongside `isBusy: false`, which is the other half of the same contract. The existing opt-in widget test now asserts the message reaches the status bar, and fails on the previous code. Flutter launcher 59/59. Reported by Copilot on #68. Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
The Notes column in this file describes each rule's invariant, not its emit condition — TFMP008 reads "Generated command codecs fit the declared payload limit" and fires when they do not. TFMP014's row followed that convention but read as a bare statement of fact, which is easy to take for the condition that produces the warning. "must fit" keeps the convention and removes the ambiguity, matching TFMP001's "must be non-generic top-level partial classes". Raised by Copilot on #68 as an inverted note; the table's convention is what made it read that way. Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
Keep the UDP listener alive for sequential transactions and drain pending work on close. Add loopback failure regressions. Disclose source IP and port exposure before opt-in. Preserve empty server defaults, activation gates, and disabled aggregate reporting. Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
057fecd to
8ada9f0
Compare
Match actual UDP response origins to CHANGE-REQUEST flags before accepting filtering evidence. Preserve the transaction when an unrelated source responds. Keep unsupported discovery unknown: require a usable alternate and explicit completed filtering probes. Add loopback origin and unsupported-server regressions before fixing each defect. Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
Handle nonblocking responder backpressure without increasing deadlines or weakening delivery assertions. Preserve one listener per owned socket until teardown. Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
|
Addressed the routing, CI, and privacy concerns in the rebased branch, now at The pane now discloses that configured STUN servers see the source IP address and port before opt-in. Developer mode, saved opt-in, explicit Run, empty server defaults, and disabled aggregate reporting remain intact. P0-PRIV-01 still requires real approval; local address-free results do not grant a privacy exemption. Review also fixed the socket being closed after its first transaction, acceptance of replies from incorrect sources, and unsupported discovery being reported as restrictive filtering. Each defect has regressions established before its fix, including an owned loopback source matrix. No public STUN server was contacted. Final local verification: 891 domain tests, 525 data tests (four existing Windows symlink skips), 84 launcher tests, 56 focused probe/socket tests, clean analyzers/formatting/line limits, fresh Windows launcher build, all eleven SDK packages/seven C# harnesses, and repository/documentation audits. The old Windows data-test divergence did not recur. A transient SDK-output rewrite race and responder-backpressure failure were corrected/isolated without waiving checks; the final full data run is green. Exact final-head verification is complete: CI 34177635608 and CodeQL 34177633789 passed on |
Behavior
Adds a manually triggered developer reachability diagnostic and TFMP014 validation for declared client-to-server bandwidth exceeding the per-connection budget. This PR targets current
devindependently; the superseded local-world stack and historical merge were removed during rebase.The probe defaults off, requires developer mode, saved opt-in, and an explicit Run probe action, and has no built-in server list. It classifies STUN observations without retaining observed addresses in domain results or logs. Configured STUN servers and their advertised alternate endpoints see the request source IP address and port; the pane discloses exposure before opt-in. No aggregate result is uploaded,
reportingApprovedremains false, and P0-PRIV-01 remains a release blocker. No server destination or privacy approval is introduced.The UDP transport keeps one subscription for the socket lifetime. Successful requests, timeouts, and late responses cannot close or complete another transaction; close drains pending work and overlapping requests are refused. This repairs the observed failure where only the first request reached a real loopback responder.
Only replies whose actual address and port match the requested source changes count as evidence. Missing or unusable OTHER-ADDRESS support stops discovery after the initial binding; filtering remains unknown. A completed supported probe may still report restrictive filtering when no reply arrives.
Current launch previews, profile persistence, request-correlated session outcomes, and subscription cleanup are preserved through the helper extraction. Existing review fixes for address-family binding, settings feedback, and TFMP014 wording are retained.
Verification
9e0c7ef3f0f956cef232f7ef16d3d3a0e9c1584bpassed CI 34177635608 and CodeQL 34177633789. All 31 reported checks and all five required gates passed, including Windows/Linux data tests, all seven templates, and complete Linux documentation publication. GitHub reports the branch clean against dev.No public server measurement, game execution, transport feasibility approval, or release acceptance is claimed.