diff --git a/meshterm/core/device_config.py b/meshterm/core/device_config.py index cf45a23..14b4a01 100644 --- a/meshterm/core/device_config.py +++ b/meshterm/core/device_config.py @@ -96,6 +96,14 @@ def parse_value(spec: SettingSpec, raw: Any, snapshot: dict | None = None) -> An """ text = str(raw).strip() if spec.value_type == "str": + # `config show` prints an empty string as the two characters `""`, because a key + # with nothing after it reads as a truncated line rather than as an empty value. + # A value the dump prints has to be one `set` takes back (CLAUDE.md, "A value must + # round-trip"), so the quotes come off here — otherwise feeding a dump back in + # silently replaces every empty setting with a pair of quote marks, and the next + # dump looks identical, so nothing ever tells you. + if text == '""': + text = "" if spec.max_length is not None and len(text) > spec.max_length: raise DeviceConfigError( f"{spec.key}: must be at most {spec.max_length} characters, got {len(text)}" diff --git a/meshterm/persistence/logging.py b/meshterm/persistence/logging.py index e4c8263..5d8b996 100644 --- a/meshterm/persistence/logging.py +++ b/meshterm/persistence/logging.py @@ -101,8 +101,14 @@ def configure_logging( logger.propagate = False if not quiet: + # markup=False: a log line carries whatever the device said — a node name, a + # firmware error — and Rich would read `[...]` in it as a style tag. That made the + # *error path* the one most likely to fail: reporting a fault on a node called + # `[/]Bob` raised MarkupError from inside the handler, replacing the diagnostic + # with a traceback about the diagnostic. No log call in the package marks up its + # message, so parsing them buys nothing. rich_handler = RichHandler( - console=console, rich_tracebacks=True, show_path=False, markup=True + console=console, rich_tracebacks=True, show_path=False, markup=False ) rich_handler.setLevel(level) logger.addHandler(rich_handler) diff --git a/meshterm/tools/about.py b/meshterm/tools/about.py index 0c755e5..0b5e070 100644 --- a/meshterm/tools/about.py +++ b/meshterm/tools/about.py @@ -80,7 +80,7 @@ class _AboutTool(Tool): Returns: A :class:`ToolResult` naming the page that was shown. """ - ctx.ui.show(self.page().without_qr()) + ctx.ui.show(self.page().for_script()) return ToolResult(summary={"page": self.name}) diff --git a/meshterm/tools/config.py b/meshterm/tools/config.py index 95fabdd..ae0e5cf 100644 --- a/meshterm/tools/config.py +++ b/meshterm/tools/config.py @@ -364,7 +364,8 @@ async def _show(ctx: AppContext, device: Device, snapshot: dict) -> None: for _category, specs in settings_by_category(): for spec in specs: value = _script_value(spec, spec.getter(snapshot)) - rows.append((spec.key, conceal(value) if spec.key == PIN_KEY else value)) + masked = conceal(value, absent=script.NONE) if spec.key == PIN_KEY else value + rows.append((spec.key, masked)) # Custom variables are experimental firmware fields with no spec, so they are namespaced # rather than mixed in — a reader can tell which lines `config set` will take. rows.extend((f"custom.{key}", value) for key, value in sorted(custom.items())) diff --git a/meshterm/ui/config_editor.py b/meshterm/ui/config_editor.py index 4fd9d77..a52d113 100644 --- a/meshterm/ui/config_editor.py +++ b/meshterm/ui/config_editor.py @@ -306,7 +306,7 @@ PIN_KEY = "device_pin" _PIN_WIDTH = len(str(get_spec(PIN_KEY).maximum)) -def conceal(value: str) -> str: +def conceal(value: str, *, absent: str = "?") -> str: """``value`` as a row of :data:`_PIN_WIDTH` mask bullets. A value the device could not report is not a secret, it is an absence: ``format_value`` @@ -314,8 +314,16 @@ def conceal(value: str) -> str: something to reveal when there is nothing. So only a real value is concealed, and :func:`has_pin` asks the same question the other way round — a page with nothing to conceal advertises no key for it. + + Args: + value: The already-rendered value to mask. + absent: How *this* surface writes "the device never reported one". The menu's + token is ``?``; the scripted CLI's is :data:`~meshterm.ui.script.NONE`. The + rule is the same on both and the glyph is not, so the caller names it — a + ``conceal`` that knew only the menu's token masked the CLI's absence too, and + six bullets told the reader the radio was PIN-locked when it was not. """ - return value if value == "?" else MASK_MARK * _PIN_WIDTH + return value if value == absent else MASK_MARK * _PIN_WIDTH def has_pin(snapshot: dict) -> bool: diff --git a/meshterm/ui/markdown.py b/meshterm/ui/markdown.py index af41160..4f9a2b9 100644 --- a/meshterm/ui/markdown.py +++ b/meshterm/ui/markdown.py @@ -99,13 +99,17 @@ class Block: :attr:`MarkdownDoc.sections`). label: The heading's plain text, for a landmark; empty otherwise. qr: Whether this block is a ``qr`` fence's scannable code. Marked so a surface - that cannot use one can drop it (see :meth:`MarkdownDoc.without_qr`). + that cannot use one can drop it (see :meth:`MarkdownDoc.for_script`). + rule: Whether this block is a ``---`` horizontal rule. Marked for the same + reason: a rule is drawn to the full width of whatever prints it, and the + scripted console is 16384 cells wide. """ renderable: RenderableType heading: bool = False label: str = "" qr: bool = False + rule: bool = False class MarkdownDoc: @@ -127,21 +131,28 @@ class MarkdownDoc: """The page's section headings, in order — the landmarks a jump steps by.""" return tuple(block.label for block in self.blocks if block.heading) - def without_qr(self) -> MarkdownDoc: - """The same page with its scannable codes left out — and the blank line each left behind. + def for_script(self) -> MarkdownDoc: + """The same page with what only a screen can use left out, and no gap where it stood. - A QR is a second rendering of a URL the page already prints as a link, drawn for a - phone pointed at a screen. Redirected into a file it is a block of block characters - wrapped around nothing new, so the scripted CLI takes this copy instead. + Two blocks are drawn for an eye and are worse than useless in a pipe: + + * A **QR** is a second rendering of a URL the page already prints as a link, drawn + for a phone pointed at a screen. Redirected into a file it is a block of block + characters wrapped around nothing new. + * A **rule** is sized to the console it is printed on, and the scripted console is + :data:`~meshterm.ui.script.WIDTH` cells wide — so the ``---`` above a colophon + arrives as one 16384-character line, tens of kilobytes of ``─`` in a page of + prose. It is also a frame, which the scripted CLI does not draw. The blank line + on each side already says what the rule said. Returns: A new document; this one is untouched. """ kept: list[Block] = [] for block in self.blocks: - if block.qr: - # The blank separator that opened the QR's own block goes with it, so - # dropping a code does not leave two blank lines where it stood. + if block.qr or block.rule: + # The blank separator that opened the dropped block goes with it, so + # taking one out does not leave two blank lines where it stood. if kept and isinstance(kept[-1].renderable, Text) and not kept[-1].renderable.plain: kept.pop() continue @@ -215,8 +226,9 @@ def _document(nodes: Sequence[SyntaxTreeNode]) -> list[Block]: heading = node.type == "heading" and node.tag == landmark label = _inline(node.children[0], heading=True).plain if heading else "" qr = node.type == "fence" and node.info.strip() == "qr" + rule = node.type == "hr" for rendered in _block(node, indent=INDENT, depth=0): - blocks.append(Block(rendered, heading=heading, label=label, qr=qr)) + blocks.append(Block(rendered, heading=heading, label=label, qr=qr, rule=rule)) heading = False # a heading is one block; never mark a stray second return blocks diff --git a/meshterm/ui/script.py b/meshterm/ui/script.py index e0c22dd..a5f8474 100644 --- a/meshterm/ui/script.py +++ b/meshterm/ui/script.py @@ -153,7 +153,15 @@ def columns(*headers: str, right: Sequence[str] = ()) -> Table: header, justify="right" if header in wanted else "left", no_wrap=True, - overflow="ignore", + # "crop", not "ignore". Both refuse to elide — neither ever writes the "…" + # that would make a key or a route unusable to the caller — but Rich's + # ``Text.wrap`` returns early on "ignore" (text.py: ``if overflow == + # "ignore": lines.append(line); continue``) and that early return is *above* + # the justify step, so "ignore" silently discards ``justify="right"``. The + # two settings differ only past the console's 16384 cells, where "crop" cuts + # and "ignore" also cuts; they differ on every line before that, where only + # "crop" aligns. + overflow="crop", ) return table @@ -179,8 +187,8 @@ def pairs(rows: Iterable[tuple[str, str]]) -> Table: expand=False, padding=(0, GUTTER, 0, 0), ) - table.add_column(no_wrap=True, overflow="ignore") - table.add_column(no_wrap=True, overflow="ignore") + table.add_column(no_wrap=True, overflow="crop") + table.add_column(no_wrap=True, overflow="crop") for key, value in rows: table.add_row(key, value) return table @@ -294,6 +302,12 @@ def console(stream: Any = None) -> Console: soft_wrap=True, highlight=False, emoji=False, + # A node broadcasts its own name, so every name on this console is remote data. + # Rich reads `[...]` as a style tag: `[bold]Loud` printed as `Loud` — silently + # corrupted, and no longer the string that identifies the node — while `[/]Bob` + # raised MarkupError and took the whole command down with it. Nothing the scripted + # CLI prints is ever marked up, so the parser has nothing to do here but misfire. + markup=False, ) @@ -312,7 +326,7 @@ def stderr_console() -> Console: from .theme import active_theme - return Console(file=sys.stderr, theme=active_theme()) + return Console(file=sys.stderr, theme=active_theme(), markup=False, emoji=False) # -- the safety net ------------------------------------------------------------------ @@ -349,7 +363,7 @@ def flatten(renderable: RenderableType) -> list[RenderableType]: renderable.padding = (0, GUTTER, 0, 0) for column in renderable.columns: column.no_wrap = True - column.overflow = "ignore" + column.overflow = "crop" column.ratio = None return [renderable] if isinstance(renderable, Group): diff --git a/tests/test_cli_contract.py b/tests/test_cli_contract.py index abbb4f4..31b0c87 100644 --- a/tests/test_cli_contract.py +++ b/tests/test_cli_contract.py @@ -41,10 +41,20 @@ _LISTINGS: list[tuple[str, list[str], list[str]]] = [ @pytest.fixture() -def run(tmp_path: Path): # noqa: ANN201 - a closure over the runner - """Invoke the real CLI against the simulator and a scratch database.""" +def run(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): # noqa: ANN201 - a closure + """Invoke the real CLI against the simulator, in a config directory of its own. + + ``--db`` moves the history and *nothing else*: the contact cache, the outbox, the + channel cache, the stored admin passwords and any device profiles all live in the + config directory regardless. So a suite that set only ``--db`` read whatever was in + the developer's own ``~/.meshterm`` — which is how a contact named ``[/]Bob``, planted + by hand on one machine, could crash this suite on that machine and pass everywhere + else. ``$MESHTERM_HOME`` is the override that moves the whole directory, and it is + the one a test wants. + """ from meshterm.cli import app + monkeypatch.setenv("MESHTERM_HOME", str(tmp_path / "home")) runner = CliRunner() def invoke(*args: str): # noqa: ANN202 @@ -120,6 +130,70 @@ def test_a_config_line_can_be_typed_back_into_config_set(run) -> None: # noqa: assert run("config", "get", "name").output.strip() == "MockCompanion" +def test_every_setting_show_prints_is_one_set_takes_back(run) -> None: # noqa: ANN001 + """The round-trip claim, checked on every setting rather than on the two easy ones. + + ``config show > f`` and feeding ``f`` back in is the obvious thing to do with a dump, + and the failure mode is silent: an empty string printed as ``""`` used to parse back + as *the two quote characters*, and the next dump looked identical, so nothing ever + said the setting had been replaced. A refusal is fine here — you cannot set a value + the firmware never reported — but a value that parses to something else is not. + """ + from meshterm.core import device_config as dc + from meshterm.tools.config import _script_value + + probes = { + "str": ["", "Yagi-Repeater", "a name with spaces"], + "bool": [True, False], + "int": [0, 1, 12], + "float": [0.0, 3.5], + } + checked = 0 + for _category, specs in dc.settings_by_category(): + for spec in specs: + if spec.value_type == "enum" and spec.choices: + values: list = list(spec.choices) + else: + values = probes.get(spec.value_type, []) + for value in values: + printed = _script_value(spec, value) + try: + parsed = dc.parse_value(spec, printed, {}) + except dc.DeviceConfigError: + continue # a loud refusal is honest; a wrong value is not + checked += 1 + assert parsed == value, f"{spec.key}: show printed {printed!r} -> {parsed!r}" + assert checked > 40, f"only {checked} round-trips exercised" + + +def test_an_unreported_pin_is_not_masked_as_though_one_were_set(run) -> None: # noqa: ANN001 + """Bullets say "there is a secret here". An absence must not borrow that claim. + + ``conceal`` already declines to mask the *menu's* absence token for exactly this + reason, but the scripted token is ``-``, which it had never been shown — so a radio + that had never reported a PIN dumped six bullets, and the reader (or the bug report + they pasted it into) read that as PIN-locked. + """ + from meshterm.ui import script + from meshterm.ui.config_editor import MASK_MARK, conceal + + # The rule at the boundary where it broke: a real value is masked, an absence is not, + # and each surface says which glyph it writes an absence with. + assert conceal("1234", absent=script.NONE) == MASK_MARK * 6 + assert conceal(script.NONE, absent=script.NONE) == script.NONE + assert conceal("?") == "?" + + # And the two surfaces must never disagree about the same radio: `show` wears bullets + # only where `get` has something to conceal. + values = dict( + line.split(None, 1) for line in result_lines(run("config", "show")) if " " in line + ) + got = run("config", "get", "device_pin").output.strip() + assert (MASK_MARK in values["device_pin"]) == (got != script.NONE), ( + f"show says {values['device_pin']!r} while get says {got!r}" + ) + + def test_config_get_prints_the_bare_value(run) -> None: # noqa: ANN001 """The caller named the key; repeating it back is one more thing to strip off.""" assert run("config", "get", "tx_power").output.strip() == "20" diff --git a/tests/test_script_output.py b/tests/test_script_output.py index d913ddd..f4dc371 100644 --- a/tests/test_script_output.py +++ b/tests/test_script_output.py @@ -148,6 +148,25 @@ def test_the_scripted_console_emits_no_escape_sequence_at_all() -> None: assert not _ANSI.search(_rendered(loud)) +@pytest.mark.parametrize( + "name", + ["[bold]Loud", "[/]Bob", "[red]a[/red]", ":fire:Hot", "[[weird]]", "100% [done]"], +) +def test_a_node_name_is_never_read_as_rich_markup(name: str) -> None: + """A node broadcasts its own name, so every name printed here is remote data. + + Rich reads ``[...]`` as a style tag unless told not to. That made a name into two + different bugs: ``[bold]Loud`` printed as ``Loud`` — silently corrupted, and no longer + the string that identifies the node to whoever reads the output — and ``[/]Bob`` raised + ``MarkupError`` and took the whole command down. ``:fire:`` is the same hazard one + parser over. Neither is hypothetical: a name is the one field a stranger controls. + """ + table = script.columns("NAME", "TYPE") + table.add_row(script.quote(name), "node") + record = _rendered(table).splitlines()[1] + assert record.startswith(script.quote(name)), record + + def test_the_scripted_console_never_wraps_a_record_onto_a_second_line() -> None: """A record is a line: wrapping would put half its fields under the wrong headings.""" table = script.columns("KEY", "VALUE") @@ -191,6 +210,36 @@ def test_columns_align_so_every_record_splits_the_same_way() -> None: assert starts == {header.index("TYPE")} # every TYPE value opens under its heading +def test_a_numeric_column_really_right_aligns() -> None: + """A column of magnitudes is only comparable by eye if the digits line up. + + Rich's ``Text.wrap`` returns early on ``overflow="ignore"`` — and that early return + sits *above* the justify step, so asking for ``overflow="ignore"`` throws + ``justify="right"`` away without a word. Every ``right=`` in the CLI was a no-op + until the columns asked for ``"crop"``, which refuses to elide just as firmly and + still aligns. Checking where a field *starts* cannot see this; only its end can. + """ + table = script.columns("NAME", "PKTS", right=("PKTS",)) + table.add_row(script.quote("Alice"), "1174") + table.add_row(script.quote("Bob"), "3") + header, *records = _rendered(table).splitlines() + assert len({len(line) for line in records}) == 1, records + assert {len(line) for line in records} == {len(header)} + + +def test_no_column_elides_however_long_its_value_runs() -> None: + """A key cut short is not something a caller can hand back to ``--to`` or ``--path``. + + The guard against eliding and the guard against mis-alignment are the same setting, + so a change to one silently moves the other; both are pinned here. + """ + table = script.columns("KEY", "PKTS", right=("PKTS",)) + table.add_row("d4" * 32, "7") + out = _rendered(table) + assert "…" not in out + assert "d4" * 32 in out + + # -- framing --------------------------------------------------------------------------