Refuse the wrong answer instead of quietly giving it
Four places where the CLI answered a question the caller had not asked. `--profile yagi` naming no profile fell through to ordinary discovery, so a typo in a scheduled advert transmitted from whichever radio happened to be attached. Naming a profile is a claim about which device; an unkeepable claim is a bad argument, not a default. `--sort bogus` sorted by the default and `--category long_hual` 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 "no records" from "you misspelled the discipline". `ContactsSort.from_name` still forgives an unknown name, because a saved preference from an older build must not wedge the menu's list; the command line refuses instead. A `--port` that would not open reported exit 4 and "the connection to the device was lost", describing a connection there had never been. Every failure while acquiring the device is the other status: nothing was transmitted. It now says which port, and why. A message body could hold a newline, which ended its record early and left the remainder indented under the other columns as a second record with an empty TIME — the one field a stranger fills in, breaking the one rule the format has. script.oneline folds it flat. And an unnamed node read as `""` in one listing and `-` in the next: seven of the ten places that print a name were claiming "called nothing" where they meant "never said". script.name draws the distinction once, which is also the one the JSON rendering will need. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
369e29e44a
commit
98c635a62e
13 files changed
+232
-23
No files matched your search
@@ -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,
|
||||
|
||||
+29
-4
@@ -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
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
+12
-9
@@ -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
|
||||
@@ -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)),
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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"),
|
||||
|
||||
@@ -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
|
||||
|
||||
+54
-4
@@ -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:
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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 ----------------------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
Reference in new issue
Block a user