fix(response_template): address review findings 1-4 and 7

Follow-up to the PR #254 integration fixes, from an xhigh review of the
merged branch. Five of fourteen findings; the other nine are written up in
.scratch/pr254-deferred-review-findings.md (untracked).

- Re-raise CancelledError from resolve_template_async. gather's
  `return_exceptions=True` captures it like any other exception, so the
  BaseException branch was swallowing it: a render cancelled during shutdown
  logged "shortening failed" and carried on to transmit.
- Warn once per template, not once per render. The unresolved-template
  warning runs on the inbound message path, so a shorten_url filter in
  Test_Command.response_format wrote a WARNING per matching message,
  unthrottled, to a rotating 5 MB log.
- Add a `urlencode` filter. A quoted literal substitutes nested fields
  verbatim, which is right for prose and wrong for URLs: `sender` is whatever
  a remote node advertises, and an unencoded `&`, `#` or space rewrote the
  query, truncated at a fragment, or malformed the link that was then POSTed
  to the shortener. `/` is encoded too, since an interpolated field is one
  path segment.
- Distinguish the two ways shortening can be unavailable. A template needing
  resolution with no config now warns at the resolver instead of returning {}
  indistinguishably from "nothing to do"; a pre-pass that ran but could not
  shorten logs at debug, since that is transient network trouble rather than
  a misconfiguration and must not escalate on the message path.
- Cover the path_command wiring. _format_path_reply_prefix is the only place
  the pre-pass is called and had no test, so a dropped `await` or a missing
  `shortened=` would have disabled shortening silently.

The warn-once cache is module state, so its test fixture is autouse: without
it a test that renders an unresolved template suppresses the warning in
whichever test runs next, which is an order-dependent failure.

Every template shipped in config.ini.example still renders identically to the
regex parser this replaced.

.gitignore

@@ -188,4 +188,5 @@ local/service_plugins/*
 
 # Local debugging scratch (captured configs and logs — may contain real settings)
 _debug/
+.scratch/
 aqi_http_cache.sqlite

CHANGELOG.md

@@ -22,6 +22,9 @@ semantic versioning.
   a packet on a full URL. The request is made off the event loop before rendering
   starts; if it fails the clause is dropped rather than sent unshortened, so a
   shortener outage costs the link instead of a second transmission.
+- `urlencode` percent-encodes a value before it is interpolated into a URL built by
+  a quoted literal, for fields a remote node controls (`sender`, and anything derived
+  from message content) where a stray `&` or `#` would otherwise rewrite the link.
 - Shlink is supported as a URL shortener backend alongside v.gd / is.gd, selected
   with `short_url_website_service` under `[External_Data]` (`gd`, the default, or
   `shlink`). Shlink authenticates with `short_url_website_api_key`.

docs/path-command-config.md

@@ -33,6 +33,11 @@ reply_prefix = "{path_distance|prefix_if_nonempty:📏 }\n"
 reply_prefix = {packet_hash|if_nonempty:"https://scope.example.net/#/packets/{packet_hash}"}
 ```
   The `LITERAL` argument may itself be a double-quoted string containing nested `{field}` placeholders (expanded before the filter runs), so the link above still carries the packet hash even though the field being gated on (`packet_hash`) and the field inside the literal are the same one.
+- `urlencode` percent-encodes a value for safe interpolation into a URL. A quoted literal substitutes nested fields verbatim, which is correct for prose and wrong for links: `{sender}` is whatever name a remote node advertises, so an unencoded `&`, `#`, `?` or space rewrites the query, truncates the URL at a fragment, or malforms it. `{packet_hash}` is hex and needs no encoding, but anything a remote node controls does:
+
+```ini
+reply_prefix = {sender|if_nonempty:"https://scope.example.net/#/nodes/{sender|urlencode}"|shorten_url}
+```
 - `shorten_url` replaces the value with a short link from the shortener configured under `[External_Data]` (`short_url_website`, `short_url_website_service` — `gd` for v.gd/is.gd-compatible or `shlink`, and `short_url_website_api_key`, which shlink requires). Chain it after building the link so only the final URL is sent over RF:
 
 ```ini

modules/response_template.py

@@ -13,6 +13,7 @@ from __future__ import annotations
 
 import asyncio
 from typing import Any, Callable
+from urllib.parse import quote
 
 from .url_shortener import shorten_url
 from .utils import message_hop_count, message_path_bytes_per_hop
@@ -96,14 +97,22 @@ def _filter_shorten_url(value: str, ctx: dict[str, Any], args: str) -> str:
         return value
 
     if isinstance(resolved, dict):
-        return resolved.get(value, '')
+        short = resolved.get(value)
+        if short:
+            return short
+        # A resolved mapping that lacks this URL means the pre-pass ran and the
+        # shortener could not answer. Debug, not warning: that is a transient
+        # network condition on the message path, not a misconfiguration.
+        logger = ctx.get('logger')
+        if logger is not None:
+            logger.debug("No shortened form for %r; dropping the clause", value)
+        return ''
 
-    logger = ctx.get('logger')
-    if logger is not None:
-        logger.warning(
-            "shorten_url used in a template rendered without resolve_template_async(); "
-            "dropping the clause rather than blocking the event loop"
-        )
+    _warn_unresolved_once(
+        ctx.get('logger'),
+        str(ctx.get('template') or ''),
+        'the caller did not run resolve_template_async()',
+    )
     return ''
 
 
@@ -114,12 +123,27 @@ def _filter_if_nonempty(value: str, ctx: dict[str, Any], args: str) -> str:
     return args
 
 
+def _filter_urlencode(value: str, ctx: dict[str, Any], args: str) -> str:
+    """Percent-encode *value* for safe interpolation into a URL.
+
+    A quoted literal substitutes nested field values verbatim, which is right for
+    prose but wrong the moment the literal is a URL: ``sender`` is whatever name a
+    remote node advertises, so an unencoded ``&``, ``#``, ``?`` or space silently
+    rewrites the link's query, truncates it at a fragment, or malforms it outright.
+    Encodes ``/`` too, since an interpolated field is a single path segment.
+    """
+    if not value:
+        return ''
+    return quote(value, safe='')
+
+
 RESPONSE_TEMPLATE_FILTERS: dict[str, FilterFn] = {
     'pathbytes_min': _filter_pathbytes_min,
     'pathbytes': _filter_pathbytes_min,
     'hops_min': _filter_hops_min,
     'prefix_if_nonempty': _filter_prefix_if_nonempty,
     'if_nonempty': _filter_if_nonempty,
+    'urlencode': _filter_urlencode,
     'shorten_url': _filter_shorten_url,
 }
 
@@ -129,6 +153,25 @@ RESPONSE_TEMPLATE_FILTERS: dict[str, FilterFn] = {
 # a chain whenever its literal needs a pipe.
 _GREEDY_ARG_FILTERS = frozenset({'prefix_if_nonempty'})
 
+# Templates already warned about, so a misconfiguration is reported once rather than
+# once per inbound message. Bounded by the number of templates in config.
+_UNRESOLVED_WARNED: set[str] = set()
+
+
+def _warn_unresolved_once(logger: Any, template: str, reason: str) -> None:
+    """Warn that ``shorten_url`` cannot resolve here, at most once per template.
+
+    This filter runs on the inbound message path, so an unconditional warning is one
+    log line per message forever on a device writing to a rotating 5 MB file.
+    """
+    if logger is None or template in _UNRESOLVED_WARNED:
+        return
+    _UNRESOLVED_WARNED.add(template)
+    logger.warning(
+        "shorten_url in template %r cannot resolve (%s); dropping the clause rather "
+        "than blocking the event loop", template, reason,
+    )
+
 
 class _TemplateParser:
     """Finite-state parser for ``{field|filter:arg|...}``-style placeholders.
@@ -310,6 +353,7 @@ def format_piped_template(
         'prefix_hex_chars': prefix_hex_chars,
         'config': config,
         'shortened': shortened,
+        'template': template,
     }
     if logger is not None:
         logger.debug("Rendering response template %r with fields %r", template, fields)
@@ -345,7 +389,10 @@ async def resolve_template_async(
     Returns an empty mapping when there is nothing to do, which renders exactly as
     an unresolved template would.
     """
-    if config is None or not template_needs_resolution(template):
+    if not template_needs_resolution(template):
+        return {}
+    if config is None:
+        _warn_unresolved_once(logger, template, 'no config was supplied to resolve it')
         return {}
 
     pending: set[str] = set()
@@ -355,6 +402,7 @@ async def resolve_template_async(
         'prefix_hex_chars': prefix_hex_chars,
         'config': config,
         'shortened': pending,
+        'template': template,
     }
     _TemplateParser(template, fields, ctx, logger).render()
     if not pending:
@@ -367,6 +415,11 @@ async def resolve_template_async(
     )
     resolved: dict[str, str] = {}
     for url, short in zip(urls, results, strict=True):
+        # Cancellation is not a shortening failure. `return_exceptions=True` captures
+        # it like any other, so swallowing it here would let a cancelled render carry
+        # on and transmit during shutdown.
+        if isinstance(short, asyncio.CancelledError):
+            raise short
         if isinstance(short, BaseException):
             if logger is not None:
                 logger.debug("Shortening %r failed: %s", url, short)

tests/unit/test_path_command_reply_prefix_shortening.py

@@ -0,0 +1,73 @@
+#!/usr/bin/env python3
+"""PathCommand wires the shorten_url pre-pass into its reply prefix.
+
+_format_path_reply_prefix is the only place resolve_template_async() is called, so
+without these a dropped `await` or a missing `shortened=` would disable shortening
+silently: the clause just stops appearing and nothing fails.
+"""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from modules.commands.path_command import PathCommand
+from modules.models import MeshMessage
+
+LONG = "https://scope.example.net/#/packets/ABCDEF12"
+TEMPLATE = '{packet_hash|if_nonempty:"https://scope.example.net/#/packets/{packet_hash}"|shorten_url}'
+
+
+def _msg():
+    return MeshMessage(content="path", sender_id="!aabbccdd", is_dm=True)
+
+
+@pytest.fixture
+def cmd(mock_bot):
+    c = PathCommand(mock_bot)
+    c.path_reply_prefix = TEMPLATE
+    c.get_standard_placeholder_fields = MagicMock(return_value={"packet_hash": "ABCDEF12"})
+    c._format_path_distance = MagicMock(return_value="")
+    return c
+
+
+@pytest.mark.unit
+@pytest.mark.asyncio
+async def test_reply_prefix_uses_the_shortened_link(cmd):
+    with patch(
+        "modules.response_template.shorten_url", return_value="https://v.gd/abc"
+    ) as shorten:
+        out = await cmd._format_path_reply_prefix(_msg())
+
+    assert out == "https://v.gd/abc\n"
+    shorten.assert_called_once()
+    assert shorten.call_args[0][0] == LONG
+
+
+@pytest.mark.unit
+@pytest.mark.asyncio
+async def test_reply_prefix_drops_the_clause_when_shortening_fails(cmd):
+    """An unreachable shortener costs the link, not a second transmission."""
+    with patch("modules.response_template.shorten_url", return_value=""):
+        assert await cmd._format_path_reply_prefix(_msg()) == ""
+
+
+@pytest.mark.unit
+@pytest.mark.asyncio
+async def test_reply_prefix_never_shortens_from_the_sync_render(cmd):
+    """The HTTP call must happen in the pre-pass, not inside format_piped_template."""
+    with patch("modules.url_shortener.requests.get") as get, \
+         patch("modules.url_shortener.requests.post") as post, \
+         patch("modules.response_template.shorten_url", return_value="https://v.gd/abc"):
+        await cmd._format_path_reply_prefix(_msg())
+    get.assert_not_called()
+    post.assert_not_called()
+
+
+@pytest.mark.unit
+@pytest.mark.asyncio
+async def test_prefix_without_shorten_url_makes_no_request(cmd):
+    cmd.path_reply_prefix = "{packet_hash|prefix_if_nonempty:# }"
+    with patch("modules.response_template.shorten_url") as shorten:
+        out = await cmd._format_path_reply_prefix(_msg())
+    assert out == "# ABCDEF12\n"
+    shorten.assert_not_called()

tests/unit/test_response_template.py

@@ -493,3 +493,122 @@ async def test_resolve_template_async_skips_a_gated_clause():
 @pytest.mark.asyncio
 async def test_resolve_template_async_is_a_noop_without_config():
     assert await resolve_template_async(_LINK_TEMPLATE, {"packet_hash": "A"}) == {}
+
+
+@pytest.fixture(autouse=True)
+def _fresh_warn_state():
+    """Reset the module-level warn-once cache around every test in this file.
+
+    `_UNRESOLVED_WARNED` deduplicates the unresolved-template warning across the
+    process, so without this a test that renders such a template silently suppresses
+    the warning in whichever test runs next — an order-dependent failure. Autouse
+    because any future test here could trip on it.
+    """
+    from modules import response_template
+
+    response_template._UNRESOLVED_WARNED.clear()
+    yield
+    response_template._UNRESOLVED_WARNED.clear()
+
+
+@pytest.mark.unit
+def test_urlencode_escapes_a_field_interpolated_into_a_url():
+    """`sender` is whatever a remote node advertises; unencoded it rewrites the URL."""
+    template = '{sender|if_nonempty:"https://x.example/u/{sender|urlencode}"}'
+    out = format_piped_template(template, {"sender": "bob&admin=1 #frag"})
+    assert out == "https://x.example/u/bob%26admin%3D1%20%23frag"
+
+
+@pytest.mark.unit
+def test_urlencode_escapes_slashes_too():
+    """An interpolated field is one path segment, not a path."""
+    assert format_piped_template('{"p/{a|urlencode}"}', {"a": "x/../y"}) == "p/x%2F..%2Fy"
+
+
+@pytest.mark.unit
+def test_urlencode_leaves_an_empty_value_empty():
+    assert format_piped_template("{missing|urlencode}", {}) == ""
+
+
+@pytest.mark.unit
+def test_unresolved_shorten_url_warns_once_per_template():
+    """This runs on the inbound message path; an unconditional warning would be one
+    log line per message forever."""
+    logger = Mock()
+    for _ in range(5):
+        assert format_piped_template(_LINK_TEMPLATE, {"packet_hash": "AB"}, logger=logger) == ""
+    assert logger.warning.call_count == 1
+
+
+@pytest.mark.unit
+def test_a_second_distinct_template_still_warns():
+    logger = Mock()
+    format_piped_template(_LINK_TEMPLATE, {"packet_hash": "AB"}, logger=logger)
+    format_piped_template('{a|shorten_url}', {"a": "https://other.example"}, logger=logger)
+    assert logger.warning.call_count == 2
+
+
+@pytest.mark.unit
+def test_a_resolved_mapping_that_misses_does_not_warn():
+    """A pre-pass that ran but could not shorten is a transient network condition,
+    not a misconfiguration — it must not escalate to WARNING on the message path."""
+    logger = Mock()
+    out = format_piped_template(
+        _LINK_TEMPLATE, {"packet_hash": "AB"}, logger=logger, shortened={}
+    )
+    assert out == ""
+    logger.warning.assert_not_called()
+    logger.debug.assert_called()
+
+
+@pytest.mark.unit
+@pytest.mark.asyncio
+async def test_resolve_template_async_warns_when_config_is_missing():
+    """Regression: this used to return {} indistinguishably from 'nothing to do', so
+    the render dropped the clause with no diagnostic anywhere."""
+    logger = Mock()
+    assert await resolve_template_async(_LINK_TEMPLATE, {"packet_hash": "AB"}, logger=logger) == {}
+    logger.warning.assert_called_once()
+
+
+@pytest.mark.unit
+@pytest.mark.asyncio
+async def test_resolve_template_async_does_not_warn_without_shorten_url():
+    logger = Mock()
+    assert await resolve_template_async("{d|hops_min:1}", {"d": "1km"}, logger=logger) == {}
+    logger.warning.assert_not_called()
+
+
+@pytest.mark.unit
+@pytest.mark.asyncio
+async def test_resolve_template_async_propagates_cancellation():
+    """`return_exceptions=True` captures CancelledError like any other exception;
+    swallowing it would let a cancelled render carry on and transmit at shutdown."""
+    import asyncio
+
+    cfg = configparser.ConfigParser()
+    cfg.add_section("External_Data")
+
+    async def _cancelled(*a, **k):
+        raise asyncio.CancelledError()
+
+    with patch("modules.response_template.shorten_url", _cancelled):
+        with pytest.raises(asyncio.CancelledError):
+            await resolve_template_async(
+                _LINK_TEMPLATE, {"packet_hash": "AB"}, config=cfg
+            )
+
+
+@pytest.mark.unit
+@pytest.mark.asyncio
+async def test_resolve_template_async_still_swallows_ordinary_failures():
+    cfg = configparser.ConfigParser()
+    cfg.add_section("External_Data")
+
+    async def _boom(*a, **k):
+        raise RuntimeError("shortener exploded")
+
+    with patch("modules.response_template.shorten_url", _boom):
+        assert await resolve_template_async(
+            _LINK_TEMPLATE, {"packet_hash": "AB"}, config=cfg
+        ) == {}