diff --git a/meshterm/cli.py b/meshterm/cli.py index 6c78543..977a2df 100644 --- a/meshterm/cli.py +++ b/meshterm/cli.py @@ -152,6 +152,14 @@ def main_callback( quiet=quiet or json_output, ) + if profile is not None and settings.resolve_profile(profile) is None: + # Falling through to ordinary discovery would pick whichever radio is attached, so + # a typo in a scheduled `--profile yagi config advert` transmits from the wrong + # node rather than failing. Naming a profile is a claim about *which* device, and + # an unkeepable claim is a bad argument. + known = ", ".join(sorted(settings.profiles)) or "none are defined" + raise typer.BadParameter(f"no device profile named {profile!r} (known: {known})") + app_ctx = AppContext( preferences=prefs, console=console, diff --git a/meshterm/context.py b/meshterm/context.py index 0c84c82..76383c0 100644 --- a/meshterm/context.py +++ b/meshterm/context.py @@ -439,6 +439,31 @@ class AppContext: getattr(device, "endpoint", None) if (self._active_transport == "tcp") else None ) + async def _open(self, what: str) -> None: + """Open :attr:`_device`, reporting a failure to open as a *selection* failure. + + The two device exit statuses answer different questions — ``NO_DEVICE`` says + nothing was transmitted and the caller should look at what is plugged in; + ``DEVICE`` says the radio was reached and the operation failed, so retrying is + reasonable. Every failure on this path is the first kind: the connection does not + exist yet, so nothing has been sent over it. It used to come back as ``DEVICE`` + with the message "the connection to the device was lost", which describes a + connection there had never been — and ``--port NOSUCHPORT`` is the most common way + to reach this line. + + Args: + what: The thing being opened, for the message (a port, an address, a host). + + Raises: + DeviceSelectionError: If the connection cannot be established. + """ + from .core.selection import DeviceSelectionError + + try: + await self._device.connect() + except Exception as exc: # noqa: BLE001 - reclassified, then re-raised + raise DeviceSelectionError(f"could not open {what}: {exc}") from exc + async def device(self) -> Device: """Return a connected :class:`Device`, opening the connection on first use. @@ -462,7 +487,7 @@ class AppContext: self._active_port = None self._active_address = None self._active_endpoint = None - await self._device.connect() + await self._open("the simulator") return self._device # A network endpoint (an explicit ``--tcp``, a TCP profile, a device picked at startup, @@ -477,7 +502,7 @@ class AppContext: self._active_endpoint = f"{tcp_host}:{tcp_port}" self._active_port = None self._active_address = None - await self._device.connect() + await self._open(f"TCP companion {tcp_host}:{tcp_port}") await self._remember_connected() await self._reconcile_channels() return self._device @@ -511,7 +536,7 @@ class AppContext: self._active_address = ble_address self._active_port = None self._active_endpoint = None - await self._device.connect() + await self._open(f"BLE companion {ble_address}") await self._remember_connected() await self._reconcile_channels() return self._device @@ -530,7 +555,7 @@ class AppContext: self._active_port = resolution.port self._active_address = None self._active_endpoint = None - await self._device.connect() + await self._open(f"serial port {resolution.port}") await self._remember_connected() await self._reconcile_channels() return self._device diff --git a/meshterm/tools/channels.py b/meshterm/tools/channels.py index 8488630..93d8b2d 100644 --- a/meshterm/tools/channels.py +++ b/meshterm/tools/channels.py @@ -95,7 +95,7 @@ class ChannelsTool(Tool): for slot in slots: table.add_row( str(slot.idx), - script.quote(slot.name), + script.name(slot.name), "public" if slot.is_public else "private", slot.hash, ) diff --git a/meshterm/tools/chat.py b/meshterm/tools/chat.py index b9fd14f..5fb827b 100644 --- a/meshterm/tools/chat.py +++ b/meshterm/tools/chat.py @@ -347,9 +347,9 @@ class ChatTool(Tool): where = f"#{message.channel}" if message.is_channel else (message.sender or "") fields = [ script.stamp(message.received_at), - script.quote(where) if where else script.NONE, + script.name(where), script.number(message.snr, "+.1f"), - message.text, + script.oneline(message.text), ] ctx.console.print((" " * script.GUTTER).join(fields), highlight=False) @@ -387,13 +387,14 @@ class ChatTool(Tool): for conversation in rows: last = lasts.get(conversation.key) table.add_row( - script.quote(conversation.label), + script.name(conversation.label), "channel" if conversation.is_channel else "direct", str(ctx.chat.unread(conversation.key)), script.stamp(last.created_at) if last is not None else script.NONE, # Not truncated to 40 cells the way the picker's preview is: nothing here - # wraps, so there is no reason to cut a message short. - last.text if last is not None else script.NONE, + # wraps, so there is no reason to cut a message short — but a body that + # holds a newline would still end the record, so it is folded flat. + script.oneline(last.text) if last is not None else script.NONE, ) if rows: ctx.ui.show(table) @@ -839,8 +840,10 @@ def _history_table(label: str, messages: list[ChatMessage]) -> Table: always the *other* party — the sender of an inbound message, and empty on a channel broadcast, which has no one party it went to. - ``TEXT`` is last and unquoted: it is the rest of the line, and a message body is the - one field that can hold absolutely anything. + ``TEXT`` is last and unquoted: it is the rest of the line. A message body can hold + absolutely anything, which is why it goes through :func:`~meshterm.ui.script.oneline` + — "the rest of the line" stops being true the moment the body holds a newline, and + the remainder then reads as a second record with an empty ``TIME``. Args: label: The conversation's display name (the caller named it; the columns don't). @@ -857,8 +860,8 @@ def _history_table(label: str, messages: list[ChatMessage]) -> Table: table.add_row( script.stamp(message.created_at), "out" if message.outbound else "in", - script.quote(peer) if peer else script.NONE, + script.name(peer), script.number(message.snr, "+.1f"), - message.text, + script.oneline(message.text), ) return table diff --git a/meshterm/tools/contacts.py b/meshterm/tools/contacts.py index 6dffe71..eda5727 100644 --- a/meshterm/tools/contacts.py +++ b/meshterm/tools/contacts.py @@ -99,6 +99,7 @@ class ContactsTool(Tool): app: The Typer application. """ from ..cli import run_tool_command + from ..ui.widgets import ContactsSort @app.command(name=self.name, help=self.help) def _contacts( @@ -106,6 +107,13 @@ class ContactsTool(Tool): "heard", "--sort", "-s", help="Order contacts by: heard, name, packets" ), ) -> None: + # `ContactsSort.from_name` falls back to the first column for an unknown name, + # deliberately, so a saved preference written by an older build cannot wedge + # the menu's list. On the command line the same silence hides a typo: the + # caller asked for one order and got another, with nothing said. + if sort not in ContactsSort.names(): + choices = ", ".join(ContactsSort.names()) + raise typer.BadParameter(f"--sort must be one of: {choices} (got {sort!r})") run_tool_command(self, {"sort": sort}) @@ -131,7 +139,7 @@ def _listing(contacts: list[Contact], counts: dict[str, int]) -> Table: table = script.columns("NAME", "TYPE", "HEARD", "PKTS", "KEY", right=("PKTS",)) for contact in contacts: table.add_row( - script.quote(contact.name), + script.name(contact.name), NODE_TYPE_LABELS.get(contact.node_type, "unknown"), script.stamp(contact.last_seen), script.number(_contact_pkts(contact, counts)), diff --git a/meshterm/tools/courier.py b/meshterm/tools/courier.py index f16a6a9..1a32919 100644 --- a/meshterm/tools/courier.py +++ b/meshterm/tools/courier.py @@ -156,7 +156,7 @@ class CourierTool(Tool): table.add_row( str(m.ident), state, - script.quote(m.node_name), + script.name(m.node_name), # An entry with no scheduled time goes as soon as the contact is heard, # which is not a time and so is not one here either. script.stamp(m.not_before), diff --git a/meshterm/tools/devices.py b/meshterm/tools/devices.py index ed03d7c..9d7c905 100644 --- a/meshterm/tools/devices.py +++ b/meshterm/tools/devices.py @@ -152,7 +152,7 @@ def _listing(devices: list, known: dict, active_target: str | None) -> Table: table.add_row( device.target, device.transport, - script.quote(name), + script.name(name), script.quote(hardware) if hardware else script.NONE, "yes" if confirmed else _MAYBE.get(device.confidence, "no"), device.serial_number or script.NONE, diff --git a/meshterm/tools/monitor.py b/meshterm/tools/monitor.py index 4ad47f5..8038e68 100644 --- a/meshterm/tools/monitor.py +++ b/meshterm/tools/monitor.py @@ -102,7 +102,7 @@ class MonitorTool(Tool): fields = [ script.stamp(obs.observed_at), obs.node or script.NONE, - script.quote(obs.name), + script.name(obs.name), script.number(obs.snr, "+.1f"), script.number(obs.rssi, ".0f"), script.number(obs.lat, ".5f"), @@ -179,7 +179,7 @@ def _heard_table(heard: list[HeardNode]) -> Table: for node in heard: table.add_row( node.node or script.NONE, - script.quote(node.name), + script.name(node.name), str(node.count), script.number(node.median_snr, "+.1f"), script.number(node.best_snr, "+.1f"), diff --git a/meshterm/tools/records.py b/meshterm/tools/records.py index 6b19321..f6536f6 100644 --- a/meshterm/tools/records.py +++ b/meshterm/tools/records.py @@ -153,6 +153,14 @@ class TrophyCaseTool(Tool): ) -> None: tool_params: dict[str, Any] = {} if category: + # An unknown id used to select *every* discipline, so a misspelled + # `--category long_hual` came back as "no records" — exit 5, which is + # exactly what a real empty result looks like. A closed set refuses. + if category not in CATEGORY_BY_ID: + choices = ", ".join(CATEGORY_BY_ID) + raise typer.BadParameter( + f"--category must be one of: {choices} (got {category!r})" + ) tool_params["category"] = category if width: tool_params["width"] = width diff --git a/meshterm/ui/script.py b/meshterm/ui/script.py index a5f8474..e822d56 100644 --- a/meshterm/ui/script.py +++ b/meshterm/ui/script.py @@ -72,13 +72,25 @@ GUTTER = 2 NONE = "-" +#: The characters that would end a record early, and the escape each one takes. A record +#: is a line, so a value carrying a newline is not a value this format can hold — and the +#: fields most likely to carry one (a node name, a message body) are exactly the fields a +#: stranger fills in. The convention is the ordinary backslash one, so it reads back. +_BREAKS = {"\\": "\\\\", "\n": "\\n", "\r": "\\r", "\t": "\\t"} + + +def _escaped(text: str) -> str: + """``text`` with its backslashes doubled and its line breaks folded into escapes.""" + return "".join(_BREAKS.get(ch, ch) for ch in text) + + def quote(value: str | None) -> str: r"""Wrap a name in doublequotes, escaping the ones inside it. A node name is user-supplied text that can hold a space, a comma, or a quote, so it is never a field on its own — quoting is what makes it one. The escaping is the - ordinary backslash convention and covers the backslash too, so the value reads back - unambiguously. + ordinary backslash convention and covers the backslash and the line breaks too, so the + value reads back unambiguously and cannot end the record it sits in. Args: value: The name to quote. ``None`` and the empty string both give ``""``. @@ -86,8 +98,46 @@ def quote(value: str | None) -> str: Returns: The quoted name, e.g. ``"Yagi-Repeater"`` or ``"He said \"hi\""``. """ - text = value or "" - return '"' + text.replace("\\", "\\\\").replace('"', '\\"') + '"' + return '"' + _escaped(value or "").replace('"', '\\"') + '"' + + +def name(value: str | None) -> str: + r"""A node's name as a field: quoted when there is one, :data:`NONE` when there isn't. + + :func:`quote` renders a missing name as ``""``, which is a different claim — *this node + is called nothing* rather than *this node never told us what it is called*. The CLI has + one token for the second, and seven of the ten places that printed a name were making + the first claim by accident, so the same unnamed node read as ``""`` in one listing and + ``-`` in the next. + + It is also the distinction the JSON rendering needs: an absent name is ``null`` and an + empty one is ``""``, and it cannot be recovered later from a field that flattened both. + + Args: + value: The name, or ``None``/empty where the node never supplied one. + + Returns: + The quoted name, or :data:`NONE`. + """ + return quote(value) if value else NONE + + +def oneline(value: str | None) -> str: + r"""``value`` as a field that can end a record without breaking it. + + For the one field a listing leaves unquoted because it *is* the rest of the line — a + message body. "The rest of the line" and "anything at all" are not compatible: a body + holding a newline ended its record early and left the remainder indented under the + other columns, reading as a second record with an empty ``TIME``. A body is also the + field a stranger fills in, so that is not a hypothetical shape of message. + + Args: + value: The free text. ``None`` gives :data:`NONE`. + + Returns: + The text with its line breaks and tabs escaped, and no quotes around it. + """ + return NONE if value is None else _escaped(value) def stamp(when: datetime | None) -> str: diff --git a/meshterm/ui/widgets.py b/meshterm/ui/widgets.py index 549771d..f1a3e3e 100644 --- a/meshterm/ui/widgets.py +++ b/meshterm/ui/widgets.py @@ -1037,6 +1037,16 @@ class ContactsSort: columns: tuple[str, ...] = _SORT_COLUMNS opens_ascending: dict[str, bool] = field(default_factory=lambda: _SORT_OPENS_ASCENDING) + @classmethod + def names(cls, columns: tuple[str, ...] = _SORT_COLUMNS) -> tuple[str, ...]: + """The orders :meth:`from_name` accepts — what a caller may legitimately ask for. + + :meth:`from_name` answers "which sort is this?" and forgives anything it does not + recognise. A surface that wants to *refuse* an unknown order needs the set itself, + which is this. + """ + return columns + @classmethod def from_name( cls, diff --git a/tests/test_cli_contract.py b/tests/test_cli_contract.py index 31b0c87..a57f7c0 100644 --- a/tests/test_cli_contract.py +++ b/tests/test_cli_contract.py @@ -280,6 +280,61 @@ def test_a_destructive_command_without_yes_is_a_silent_usage_error(run, args) -> assert "--yes" in result.stderr +@pytest.mark.parametrize( + "args,flag", + [ + (("contacts", "--sort", "bogus"), "--sort"), + (("records", "--category", "long_hual"), "--category"), + ], + ids=["contacts-sort", "records-category"], +) +def test_a_value_outside_a_closed_set_is_refused_rather_than_ignored(run, args, flag) -> None: # noqa: ANN001 + """A typo in a closed-set option used to be answered with a different question. + + ``--sort bogus`` quietly sorted by the default, and ``--category long_hual`` quietly + selected *every* discipline — which then came back as exit ``5``, the code that means + a real empty result. A caller branching on ``5`` could not tell "there are no records" + from "you misspelled the discipline", which is the worst answer a closed set can give. + """ + result = run(*args) + assert result.exit_code == exitcodes.USAGE + assert result.stdout == "" + assert flag in result.stderr + + +def test_a_profile_that_names_nothing_is_refused_before_anything_transmits(run) -> None: # noqa: ANN001 + """``--profile`` is a claim about *which* radio, and an unkeepable one is not a default. + + An unknown name used to fall through to ordinary discovery, so a scheduled + ``--profile yagi config advert`` with a typo transmitted from whichever companion + happened to be attached — the one failure mode where being quietly wrong puts a packet + on the air. + """ + result = run("--profile", "nosuchprofile", "info") + assert result.exit_code == exitcodes.USAGE + assert result.stdout == "" + assert "nosuchprofile" in result.stderr + + +def test_a_port_that_will_not_open_is_no_device_not_a_device_failure(run) -> None: # noqa: ANN001 + """The two device statuses answer different questions, and this one answered wrong. + + ``3`` says nothing was transmitted — look at what is plugged in. ``4`` says the radio + was reached and the operation failed, so a retry is reasonable. An unopenable + ``--port`` reported ``4``, with the message "the connection to the device was lost" + describing a connection there had never been. The manual's own ``case $?`` recipe + branches on exactly this split. + """ + from typer.testing import CliRunner + + from meshterm.cli import app + + result = CliRunner().invoke(app, ["--port", "NOSUCHPORT99", "info"]) + assert result.exit_code == exitcodes.NO_DEVICE + assert "could not open" in result.stderr + assert "lost" not in result.stderr + + def test_an_empty_result_writes_nothing_to_stdout_at_all(run) -> None: # noqa: ANN001 """Not a "no channels configured" line: the status carries it, so the pipe stays clean.""" result = run("channels", "list") diff --git a/tests/test_script_output.py b/tests/test_script_output.py index f4dc371..55c8b59 100644 --- a/tests/test_script_output.py +++ b/tests/test_script_output.py @@ -66,6 +66,48 @@ def test_a_quoted_name_survives_a_round_trip_through_the_escaping() -> None: assert body.replace('\\"', '"').replace("\\\\", "\\") == name +@pytest.mark.parametrize( + "body,expected", + [ + ("line one\nline two", "line one\\nline two"), + ("tabbed\there", "tabbed\\there"), + ("carriage\rreturn", "carriage\\rreturn"), + ("back\\slash", "back\\\\slash"), + ('a "quoted" body', 'a "quoted" body'), + ("plain", "plain"), + ], +) +def test_a_message_body_can_never_end_its_own_record(body: str, expected: str) -> None: + """``TEXT`` is the rest of the line, and "the rest of the line" has to stay one line. + + A body holding a newline used to end its record early and leave the remainder indented + under the other columns, reading as a second record with an empty ``TIME``. The body is + also the one field a stranger fills in, so this is not a hypothetical message. Quotes + inside it are left alone: nothing wraps it, so nothing needs escaping from. + """ + assert script.oneline(body) == expected + assert "\n" not in script.oneline(body) + + +def test_a_name_holding_a_newline_cannot_end_its_record_either() -> None: + """The same hazard one field over — and a name is broadcast by its own node.""" + assert script.quote("two\nlines") == '"two\\nlines"' + + +@pytest.mark.parametrize("absent", [None, ""]) +def test_a_node_that_never_gave_a_name_reads_as_absent_not_as_empty(absent: str | None) -> None: + """``""`` says "called nothing"; ``-`` says "never said". They are different facts. + + Seven of the ten places that printed a name made the first claim by accident, so the + same unnamed node read as ``""`` in one listing and ``-`` in the next. One helper now + answers it, and the distinction is the one the JSON rendering needs too — an absent + name is ``null``, an empty one is ``""``, and a field that flattened both cannot say + which it meant. + """ + assert script.name(absent) == script.NONE + assert script.name("Alice") == '"Alice"' + + # -- timestamps ----------------------------------------------------------------------