fix(cli,mdns): address Copilot review

  * MdnsAdvertiser.update_device_state() rebuilt TXT without the 'id'
    field, breaking the Discovery.md contract after the first state
    update. Cache the persisted id on register and route both register
    and update through a shared _build_txt(state) helper. New unit test
    pins all TXT keys (including 'id') across both call sites.

  * Resolver-level ambiguity now raises AmbiguousServerError, a
    FPBCLIError subclass with exit_code=2, and main()'s handler exits
    via e.exit_code. Previously every FPBCLIError exited 1, so scripts
    couldn't distinguish 'needs disambiguation' from runtime failures —
    contradicting the documented ladder.

  * The two 'pass --server-url to choose:' prompts (the new resolver
    and the legacy resolve_server_url) now point users at
    '-s <handle>' instead. --server-url is hidden from --help and
    deprecated; the suggestion has to match.

  * Discovery.md said the mDNS instance name was 'FPBInject on
    <hostname>', but the advertiser actually emits 'FPBInject on
    <hostname>:<port>' (the port suffix is what makes the client-side
    'handle' value possible and lets multiple servers per host
    coexist). Spec updated.

  * main() now passes legacy kwargs (port / baudrate / direct /
    server_url / token) alongside plan= so downstream wrappers that
    monkeypatch FPBCLI construction see compatible call args. The
    plan kwarg is the actual source of truth; legacy kwargs are
    ignored when plan is present.

Tests: 92/92 PASS on new test files; test_cli_coexistence 43/43 PASS;
run_tests.py --coverage --target 85 -> 85.5%.

Author: W-Mai

Tools/WebServer/Docs/Discovery.md

@@ -13,7 +13,7 @@ This document specifies the mDNS / DNS-SD service that FPBInject WebServer insta
 | Field | Value |
 |---|---|
 | Service type | `_fpbinject._tcp.local.` |
-| Instance name | `FPBInject on <hostname>` (RFC 6763 §4.1.1 user-visible name) |
+| Instance name | `FPBInject on <hostname>:<port>` (RFC 6763 §4.1.1 user-visible name; the port suffix lets multiple servers on one host coexist and is the source of the client-side ``handle`` value). |
 | Port | The TCP port the WebServer is listening on (default `5500`, follows `--port`). |
 | Address | All interfaces zeroconf binds to (IPv4 only by default). |
 | Server hostname | `<hostname>.local.` |

Tools/WebServer/cli/fpb_cli.py

@@ -64,9 +64,20 @@
 
 
 class FPBCLIError(Exception):
-    """CLI specific errors"""
+    """CLI-specific error. ``exit_code`` defaults to 1.
 
-    pass
+    Raise the ``AmbiguousServerError`` subclass when more than one server
+    matches a discovery handle so main() can exit ``2`` (the documented
+    ladder code for "needs disambiguation").
+    """
+
+    exit_code = 1
+
+
+class AmbiguousServerError(FPBCLIError):
+    """Multi-match on discovery handle / mDNS browse; exits ``2``."""
+
+    exit_code = 2
 
 
 class DeviceState(DeviceStateBase):
@@ -1378,7 +1389,7 @@ def _resolve_handle_to_url(value: str, *, source: str) -> str:
         for s in matches:
             msg.append(f"  {s.handle}  {s.url}")
         msg.append("Be more specific (use 'host:port' form).")
-        raise FPBCLIError("\n".join(msg))
+        raise AmbiguousServerError("\n".join(msg))
 
     chosen = matches[0]
     if kind == "host_port":
@@ -1546,16 +1557,14 @@ def resolve_connection_plan(args) -> ConnectionPlan:
             _classify_url(s.url, token=token, source="mdns"), port, baudrate
         )
 
-    print(
-        "Multiple FPBInject servers discovered; pass --server-url to choose:",
-        file=sys.stderr,
-    )
+    lines = [
+        "Multiple FPBInject servers discovered; pass -s <handle> to choose:",
+    ]
     for s in servers:
-        print(
-            f"  {s.url}  version={s.version}  auth={s.auth}  device={s.device}",
-            file=sys.stderr,
+        lines.append(
+            f"  -s {s.handle}    version={s.version}  auth={s.auth}  device={s.device}"
         )
-    sys.exit(2)
+    raise AmbiguousServerError("\n".join(lines))
 
 
 def resolve_server_url(args):
@@ -1596,12 +1605,12 @@ def resolve_server_url(args):
             )
         return s.url
     print(
-        "Multiple FPBInject servers discovered; pass --server-url to choose:",
+        "Multiple FPBInject servers discovered; pass -s <handle> to choose:",
         file=sys.stderr,
     )
     for s in servers:
         print(
-            f"  {s.url}  version={s.version}  auth={s.auth}  device={s.device}",
+            f"  -s {s.handle}    version={s.version}  auth={s.auth}  device={s.device}",
             file=sys.stderr,
         )
     sys.exit(2)
@@ -2062,16 +2071,21 @@ def main():
         plan = resolve_connection_plan(args)
         cli = FPBCLI(
             verbose=args.verbose,
+            port=args.port,
+            baudrate=args.baudrate,
             elf_path=elf_path,
             compile_commands=args.compile_commands,
             tx_chunk_size=args.tx_chunk_size,
             tx_chunk_delay=args.tx_chunk_delay,
             max_retries=args.max_retries,
+            direct=args.direct,
+            server_url=plan.server_url,
+            token=args.token,
             plan=plan,
         )
     except FPBCLIError as e:
         print(f"Error: {e}", file=sys.stderr)
-        sys.exit(1)
+        sys.exit(e.exit_code)
     args.server_url = plan.server_url
 
     try:
@@ -2139,7 +2153,7 @@ def main():
             cli.server_stop(port)
     except FPBCLIError as e:
         cli.output_error(str(e))
-        sys.exit(1)
+        sys.exit(e.exit_code)
     except KeyboardInterrupt:
         print("\nInterrupted by user", file=sys.stderr)
         sys.exit(130)

Tools/WebServer/services/mdns_advertiser.py

@@ -99,18 +99,23 @@ def __init__(
         self._registered = False
         self._prev_sigint = None
         self._prev_sigterm = None
+        self._server_id = ""
 
-    def _build_info(self) -> ServiceInfo:
-        hostname = socket.gethostname() or "fpbinject"
-        instance = f"FPBInject on {hostname}:{self._port}"
-        properties = {
+    def _build_txt(self, device_state: str) -> dict:
+        return {
             "txtvers": TXT_SCHEMA_VERSION,
             "version": self._version,
             "auth": self._auth_mode,
-            "device": "none",
+            "device": device_state,
             "path": self._path,
-            "id": _load_or_create_server_id(),
+            "id": self._server_id,
         }
+
+    def _build_info(self) -> ServiceInfo:
+        hostname = socket.gethostname() or "fpbinject"
+        instance = f"FPBInject on {hostname}:{self._port}"
+        self._server_id = _load_or_create_server_id()
+        properties = self._build_txt("none")
         return ServiceInfo(
             type_=SERVICE_TYPE,
             name=f"{instance}.{SERVICE_TYPE}",
@@ -190,13 +195,7 @@ def update_device_state(self, state: str) -> None:
             return
         if state not in ("none", "connected"):
             raise ValueError(f"state must be 'none' or 'connected', got {state!r}")
-        new_props = {
-            "txtvers": TXT_SCHEMA_VERSION,
-            "version": self._version,
-            "auth": self._auth_mode,
-            "device": state,
-            "path": self._path,
-        }
+        new_props = self._build_txt(state)
         try:
             self._info._set_properties(new_props)
             self._zc.update_service(self._info)

Tools/WebServer/tests/test_handle_resolution.py

@@ -141,7 +141,7 @@ def test_host_only_unique_resolves(self):
 
     @patch.dict(os.environ, {}, clear=True)
     def test_host_only_ambiguous_raises(self):
-        from cli.fpb_cli import FPBCLIError
+        from cli.fpb_cli import AmbiguousServerError
 
         resolve = _import_resolver()
         with patch(
@@ -151,9 +151,10 @@ def test_host_only_ambiguous_raises(self):
                 _server("127.0.0.1", 5501, handle="bench:5501"),
             ],
         ):
-            with self.assertRaises(FPBCLIError) as cm:
+            with self.assertRaises(AmbiguousServerError) as cm:
                 resolve(_ns(server="bench"))
         self.assertIn("ambiguous", str(cm.exception))
+        self.assertEqual(cm.exception.exit_code, 2)
 
     @patch.dict(os.environ, {}, clear=True)
     def test_handle_no_match_raises(self):

Tools/WebServer/tests/test_mdns_advertiser.py

@@ -145,6 +145,18 @@ def test_update_device_state_calls_update_service(self, MockZeroconf):
         adv.update_device_state("connected")
         self.assertTrue(zc.update_service.called)
 
+    @patch("services.mdns_advertiser.Zeroconf")
+    def test_update_device_state_preserves_id_and_all_txt_keys(self, MockZeroconf):
+        adv = _make_advertiser()
+        adv.register()
+        register_keys = set(adv._build_txt("none").keys())
+
+        connected_props = adv._build_txt("connected")
+        self.assertEqual(set(connected_props.keys()), register_keys)
+        self.assertEqual(connected_props["id"], adv._server_id)
+        self.assertEqual(connected_props["device"], "connected")
+        self.assertNotEqual(connected_props["id"], "")
+
 
 class TestMdnsAdvertiserIdempotentUnregister(unittest.TestCase):
     """unregister() is safe to call repeatedly."""

Tools/WebServer/tests/test_mdns_discovery.py

@@ -304,8 +304,9 @@ def test_two_results_lists_and_exits_2(self):
                 with self.assertRaises(SystemExit) as cm:
                     resolve_server_url(_ns(server_url=None, requires_server=True))
             self.assertEqual(cm.exception.code, 2)
-            self.assertIn("10.0.0.10", err.getvalue())
-            self.assertIn("10.0.0.11", err.getvalue())
+            self.assertIn("10.0.0.10:5500", err.getvalue())
+            self.assertIn("10.0.0.11:5500", err.getvalue())
+            self.assertIn("-s ", err.getvalue())
 
 
 # ---- cmd_discover() JSON output (S8) ----------------------------------------

Tools/WebServer/tests/test_resolve_connection_plan.py

@@ -240,7 +240,9 @@ def test_one_remote_result_returns_remote_proxy(self):
         self.assertEqual(plan.server_url, "http://192.168.1.20:5500")
 
     @patch.dict(os.environ, {}, clear=True)
-    def test_two_results_lists_and_exits_2(self):
+    def test_two_results_raises_ambiguous_server_error(self):
+        from cli.fpb_cli import AmbiguousServerError
+
         resolve = _import_resolver()
         servers = [
             _fake_server(host="10.0.0.10", port=5500),
@@ -249,13 +251,12 @@ def test_two_results_lists_and_exits_2(self):
         with patch("cli.fpb_cli.list_cli_servers", return_value=[]), patch(
             "cli.fpb_cli._localhost_status_ok", return_value=False
         ), patch("cli.fpb_cli.discover_sync", return_value=servers):
-            err = io.StringIO()
-            with redirect_stderr(err):
-                with self.assertRaises(SystemExit) as cm:
-                    resolve(_ns())
-            self.assertEqual(cm.exception.code, 2)
-            self.assertIn("10.0.0.10", err.getvalue())
-            self.assertIn("10.0.0.11", err.getvalue())
+            with self.assertRaises(AmbiguousServerError) as cm:
+                resolve(_ns())
+        self.assertEqual(cm.exception.exit_code, 2)
+        self.assertIn("10.0.0.10:5500", str(cm.exception))
+        self.assertIn("10.0.0.11:5500", str(cm.exception))
+        self.assertIn("-s ", str(cm.exception))
 
 
 class TestPlanProperties(unittest.TestCase):