Commit graph

2 commits

Author SHA1 Message Date
Matt Johnson
2d359ea6f1 fix(tests): stop poisoning sys.modules session-wide with a bare MagicMock
Three test files (test_llm_scoping, test_fix_meshcore_save_and_llm_test,
test_config_partial_save_merge) did `sys.modules.setdefault(_mod, MagicMock())`
to stub an optional import. `setdefault` installs the MagicMock into
sys.modules for the ENTIRE pytest session even when the real package is
present — so any LATER test that does `await <that module>.<coro>(...)`
(e.g. `await aiosqlite.connect(...)`) fails with
"object MagicMock can't be used in 'await' expression" / "Event loop is closed".

This is what made test_fire_tracker_phase4::test_natural_language_fire_
question_routes_to_llm pass in isolation but fail in full-suite order — the
leak came from an earlier file, not the victim. #140 hardened the victim's
own config/history isolation but couldn't fix an external sys.modules poison.

Fix: only fall back to the MagicMock when the real module genuinely fails to
import (guarded assignment), so a present package is never replaced. Root-cause
fix in the polluters, not a skip on the victim.

Suite: 2422 passed, 0 failed, 72 skipped — fully green (was 6 failed before
#140, then 1 order-dependent failure after). Confirmed deterministic across
repeated full runs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-17 20:24:44 +00:00
c04daa6e4d
fix(config): merge partial PUT bodies instead of resetting to defaults (#155)
Saving the "Auto-advert interval" dropdown on the MeshCore Companion page
took BOTH radios offline on 2026-07-17 06:46:52. One click, full outage.

The page PUT a single-key body to /api/config/connection:

    {"meshcore_advert_interval_seconds": 10800}

_dict_to_dataclass() builds kwargs only from the keys present in the body
and lets `cls(**kwargs)` default the rest, so every OMITTED field was reset
to its dataclass default and written to disk:

    type:                 tcp   -> serial            (Meshtastic offline)
    tcp_host:             192.168.1.100 -> <lost>    (LOCAL_FIELDS, see below)
    tcp_port:             4404  -> 4403              (wrong meshmonitor vnode)
    meshcore_host:        192.168.1.253 -> ''        (MeshCore off; blank = off)
    meshcore_conn_type:   serial -> tcp              (wrong transport)
    meshcore_serial_port: /dev/meshcore-rak -> ''    (RAK radio lost)

It was silent twice over. `connection` is restart-required, so the running
process kept the good in-memory config while the file sat gutted, waiting
for any restart to detonate. And save_section() writes the domain file
FIRST and local.yaml SECOND: meshtastic.yaml hit the disk already gutted,
then the local.yaml write (which owns connection.tcp_host via LOCAL_FIELDS)
died on `[Errno 13] Permission denied` -- so tcp_host landed in neither
file, and the 500 that would have named the cause was swallowed by the UI.
The operator saw nothing happen.

This was never one page's bug: PUT /api/config/{section} was destructive on
a partial payload for EVERY section. Other callers only survive because they
happen to spread the full object first.

Fixes, in depth:

* Route (the durable fix): merge the body over the CURRENT live section
  before coercing, so omitted keys keep their live values while present
  keys -- including '' / False / [] -- still apply. The base is the live
  config, the same values GET serves, so a partial PUT now lands exactly
  where a full-object PUT from that same GET would. Full-object callers are
  unaffected. Fixed at the HTTP boundary, not in _dict_to_dataclass():
  absent-key-means-default is CORRECT at config-load time, where a file
  legitimately omits fields it does not override.

* Nested semantics keyed off the dataclass schema, not "is it a dict":
  nested dataclass fields DEEP-MERGE (a partial region_routes must not drop
  sibling cells), while bare dict/list fields REPLACE at the key (cells,
  toggles, destinations, rules are dynamic maps -- deep-merging them would
  resurrect deleted keys and make deletion impossible, the mirror image of
  the bug being fixed).

* Page: send the full connection object like every other caller does.

* Errors are visible: the save handler no longer swallows the exception,
  and updateConfig() surfaces the server's `detail` rather than a bare
  "API error: 500", which is what hid Permission denied from the operator.

* Default advert interval 10800 -> 86400 (24h). 3h is far too frequent a
  default for a public mesh; the UI "(default)" label moves to match.

Tests: tests/test_config_partial_save_merge.py reproduces the outage with
the exact payload, and pins merge semantics across connection AND
notifications, intentional clearing, deep-merge, and map-deletion.

Co-authored-by: Matt Johnson <mj@k7zvx.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-17 01:28:41 -06:00