Two things firmware does that openHop did not.
Dispatch by type, not by text. simple_room_server::onPeerDataRecv routes
CLI_DATA/CLI_COMMAND to the admin CLI and turns PLAIN into a post;
simple_repeater has no chat function, so it runs every accepted type
through handleCommand with no text test at all. openHop asked
_is_cli_command(message_text) for both, and so got both wrong: a PLAIN
"set ..." ran as a command instead of being posted, a command whose text
was not in the prefix list was published to the room, and an admin's
mistyped repeater command vanished silently instead of being answered.
Guard replays. Both servers open the branch with
`sender_timestamp >= client->last_timestamp`, treat equality as a retry
they accept but do not re-run, and drop anything older. openHop checked
neither, so replaying an admin command with the same timestamp and
different attempt bits re-executed it -- packet dedup does not catch that
because the bytes differ. Note ACL._is_replay is not reusable here: its
strict <= rejects the equal case this branch accepts.
Both read from packet.decrypted, which openhop_core now publishes. A core
too old to report them falls back to the previous text-prefix behaviour
and logs a warning rather than dropping every message.
simple_room_server::onPeerDataRecv takes the
`(client->permissions & PERM_ACL_ROLE_MASK) == PERM_ACL_GUEST` branch for a
PLAIN message and stores nothing. openHop called add_post for anyone who
authenticated, so knowing the guest password was enough to write to a room:
add_post caps length and rate-limits per client, but never looks at the role.
Also warns once per message when openhop_core does not publish txt_type,
so the open text-type gate is visible in the log rather than silent.
simple_repeater::onPeerDataRecv and simple_room_server::onPeerDataRecv
both open their TXT_MSG branch by rejecting any flags outside
{PLAIN, CLI_DATA, CLI_COMMAND}. openHop had no such gate: it decided what
a message was purely from whether the text looked like a command, so a
SIGNED_PLAIN -- a room post, whose 4-byte author prefix the core handler
has already stripped, leaving bare text -- ran as an admin CLI command
whenever it happened to start with a command prefix, and was stored as a
post on a room server that firmware would have dropped.
CLI_COMMAND (3) joined the accepted set when firmware split "a CLI
command" out of CLI_DATA (MeshCore 2c0ace25); CLI_DATA stays in it
because that is what every released client still sends.
Reads the type from packet.decrypted, which the core text handler now
publishes. A core too old to report it leaves the gate open rather than
dropping every message.
`pre-commit run --all-files` fails on `dev` at the `ruff-format` hook, so every
open pull request is red on a check that has nothing to do with its own diff.
Both files arrived with 72da7ee (pr-436) formatted by an earlier ruff.
Purely cosmetic, and exactly what `ruff format` produces:
- `sqlite_handler.py` — a generator expression and an f-string concatenation
that both now fit inside the line limit unsplit.
- `test_companion_import_repeater_contacts.py` — the blank line ruff requires
after a module docstring.
- Introduced tests for the plugin catalogue installation and update flows in `test_plugin_catalogue_install.py`.
- Added unit tests for the GitHub Releases plugin client in `test_plugin_github_releases.py`.
- Implemented IPC tests for the plugin manager Unix socket protocol in `test_plugin_ipc.py`.
- Created lifecycle and installation tests for the plugin manager in `test_plugin_manager_lifecycle.py`.
- Added tests for plugin manifest parsing in `test_plugin_manifest.py`.
- Developed tests for plugin filesystem layout and state management in `test_plugin_storage.py`.
"Import repeater contacts" wrote nothing whenever a contact-type filter was
supplied, and reported success having imported zero rows.
`adverts.contact_type` holds the *display* name written through
`handler_helpers.discovery.NODE_TYPE_NAMES` — "Chat Node", "Repeater",
"Room Server", "Sensor". The import API accepts MeshCore's names — "companion",
"repeater", "room_server", "sensor", validated in
`companion_endpoints.import_repeater_contacts`. `companion_import_repeater_contacts`
compared them directly:
query += f" AND contact_type IN ({placeholders})"
so "repeater" never matched "Repeater", "room_server" never matched
"Room Server", and "companion" never matched "Chat Node" at all. On a live
repeater with 285 adverts, every one of the four permitted values selected 0
rows, as did all four together; only an unfiltered import worked.
A second defect had the same cause. The adv_type lookup normalised the stored
name (`lower()`, spaces to underscores) but mapped it with the API-name table,
where "chat_node" is absent — so chat contacts imported as adv_type 0 rather
than 1, even on an unfiltered import. That is silently wrong rather than empty,
and needs a manual `UPDATE companion_contacts SET adv_type=1 WHERE adv_type=0`
to repair.
Both sites derived the mapping independently, which is how they drifted. They
now share one `_ADVERT_TYPE_BY_STORED` table keyed on the normalised stored
form: the filter reverse-maps the requested API names onto the stored keys and
compares `LOWER(REPLACE(TRIM(contact_type), ' ', '_'))`, and the adv_type comes
from the same table. Filtering and limiting stay in SQL. An unrecognised type
still selects nothing rather than silently widening the query.
Tested against the real stored forms: 6 of the 8 new tests fail on current dev
(each of the four type filters, all four combined, and the adv_type mapping)
and all 8 pass with this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`SQLiteHandler.__init__` runs `_init_database()` and then `_run_migrations()`.
`_init_database` created `idx_packets_upstream_time`, which spans
`upstream_hash` and `upstream_hash_size` -- columns that, on a database
predating migration 13, only appear once `add_upstream_hash_to_packets` has
added them in the *later* migration step.
Fresh installs were fine, because the `CREATE TABLE` carries both columns. Any
existing database raised `no such column: upstream_hash` part-way through
`_init_database`, which caught and logged it. Every statement after the failing
index was therefore skipped -- including the `room_messages` and
`room_client_sync` tables -- and the daemon started with a partially-created
schema while reporting itself healthy: service active, radio initialised,
companions serving, HTTP up, and no packets recorded.
The `add_upstream_hash_to_packets` migration already creates this index, right
after adding the columns it depends on, so the copy in `_init_database` was
redundant as well as too early. Remove it and leave the index to the migration
that owns those columns. All three paths still end up with it: a fresh database
gets the columns from `CREATE TABLE` and the index when the migration runs, an
un-migrated database gets both from the migration, and an already-migrated one
kept the index from its earlier run.
Observed on a Raspberry Pi 4 upgrading a ~13 MB production database from 1.1.1;
the repeater was off-air for four minutes before the cause was found. It
self-heals on a second start, because the migration still runs after the failed
init, which makes it easy to misread as harmless.
`tests/test_sqlite_upgrade_existing_db.py` builds a database with the
pre-migration-13 schema and asserts `_init_database` runs to completion, keyed
on tables created after the old failure point. Asserting only on the upstream
columns is not sufficient -- the migration adds those either way.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`RepeaterDaemon._shutdown` arms an exit watchdog -- a `threading.Timer` that
calls `os._exit(0)` after `SHUTDOWN_EXIT_GRACE_S` (5 s). That is right in
production: it guarantees SIGTERM is honoured.
`test_run_starts_http_and_handles_dispatcher_cancelled_gracefully` awaits
`daemon.run()`, whose `finally` reaches `_shutdown()`, so it armed a real
watchdog and returned. Five seconds later the timer fired and took the whole
pytest process down.
The failure mode is what makes this expensive: `os._exit(0)` exits with **status
zero**. pytest produces no summary line and a truncated progress bar, so the run
looks like it simply ended -- CI reads exit 0 as green on a run that never
finished, and any failure after that point is invisible. Locally the suite
appeared to hang at ~77% with no failures reported; it was actually being killed
there.
It also misattributes itself. The test blamed is whichever happened to be
running five seconds later, so it moves between runs
(`test_route_login_server_companion_only_skips_login_helper` and
`test_route_login_server_hash_collision_offers_to_both` on two runs here) and
never points at the cause. Finding it needed wrapping `_arm_exit_watchdog` to
log the arming test.
Two changes:
- Patch `_arm_exit_watchdog` for that test, matching the guard the shutdown
tests in `test_main_py_coverage.py` already use.
- Add `tests/conftest.py` with an autouse fixture that fails any test leaving a
live `shutdown-watchdog` timer, and cancels it. This blames the responsible
test at the moment it leaks, rather than an unrelated one later. Verified it
catches the original bug: reverting the one-line fix turns the silent
process-kill into a teardown error naming the right test.
`test_exit_watchdog_is_a_daemon_timer_that_forces_exit` is unaffected -- it sets
a 0.05 s grace and asserts on a patched `os._exit` before returning, so it
leaves nothing behind.
With this the suite runs to completion: 3 failed, 1580 passed. Those 3 are
pre-existing on `dev` and unrelated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit fixed battery_mv in the STATS_TYPE_CORE frame, but that is
not the path companion apps use to display battery. CMD_GET_BATT_AND_STORAGE
goes through `CompanionFrameServer._get_batt_and_storage()`, an openhop_core
extension hook documented as "Default: all zeros" that the repeater never
overrode — so apps showed 0% regardless of what any sensor reported.
Override it in the repeater's frame server: battery from the same sensor lookup
added for core stats, storage from the filesystem holding the configured
storage_dir. Both are wired in from main.py alongside stats_getter.
Found by deploying the previous fix and observing that Home Assistant showed the
modem battery correctly while the phone app still showed 0% — two different
paths, only one of which had been fixed.
`_get_companion_stats` hardcoded `battery_mv: 0`, so every MeshCore companion
client connected to a Python repeater showed no battery, even when a sensor
plug-in was reporting one. The protocol side is already complete: the frame
server packs `battery_mv` into the STATS_TYPE_CORE frame and clients request it
via CMD_GET_BATT_AND_STORAGE.
Surface the first sensor reading that publishes a usable battery voltage,
accepting either `battery_voltage_mv` or `battery_voltage_v`. This covers an
attached UPS HAT as well as the battery of an openHop modem the repeater is
driving, which is the case that prompted this: a solar-powered modem whose
battery was invisible to the companion app right up until it went flat.
Returns 0 when no sensor reports a battery, which is what clients already
expect from a mains-powered node, and the lookup never raises — stats must
not be able to take down the companion connection.
agessaman