Five ways the scripted output lied, and the name that crashed it

The review pass found the CLI breaking its own written rules.

A node broadcasts its own name, so every name the CLI prints is remote
data — and Rich read `[...]` in it as a style tag. A node called
`[bold]Loud` printed as `Loud`, silently no longer the string that
identifies it; a node called `[/]Bob` raised MarkupError and took the
command down. The log handler then failed the same way while reporting
it, replacing the diagnostic with a traceback about the diagnostic. No
log line and no scripted line is ever marked up, so all three consoles
now say so.

`right=` never right-aligned anything. Rich's `Text.wrap` returns early
on `overflow="ignore"`, above the justify step, so asking for one threw
the other away — every numeric lane in the CLI was left-aligned while
CLAUDE.md, script.py's own docstring and the manual all said otherwise.
"crop" refuses to elide just as firmly and still aligns.

`config show` printed an empty string as `""` and `config set` read
those two characters literally, so feeding a dump back in replaced every
empty setting with a pair of quote marks — and the next dump looked
identical, so nothing ever said so. It also masked an *unreported* PIN
behind six bullets, telling the reader the radio was PIN-locked when it
was not: `conceal` already declined to mask the menu's absence token and
had simply never been shown the CLI's.

`meshterm about` drew the `---` above its colophon as a rule sized to
the console, which on a 16384-cell console is one 48KB line in a 4KB
page. A rule is a frame; the scripted copy drops it with the QR.

The contract fixture set only `--db`, which moves the history and
nothing else — so the suite read whatever was in the developer's own
~/.meshterm. That is how `[/]Bob`, planted by hand on one machine, could
crash the suite there and pass everywhere else. $MESHTERM_HOME moves the
whole directory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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)}"

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)

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})
 
 

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()))

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:

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
 

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):

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"

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 --------------------------------------------------------------------------