fix(webconfig): correct reply classification and the missing MyMesh commands

Two things hardware turned up.

The whole terminal came back red. The endpoint decided a command had succeeded
by testing its reply for an "OK" prefix — the convention the config batch relies
on, and a safe one there because every allowlisted setter uses it. The CLI
reaches the whole surface, where success has no single shape: setters answer
"OK...", getters answer "> value", `erase` answers "File system erase: OK". Only
failure is uniform ("Err", "ERR:", "Error:"), so that is what the CLI now tests
for.

Colour was the visible half. The other half was worse: _batch_all_ok went false
the moment a sequence contained a `get`, so a script ending in `reboot` was told
some commands had failed and the reboot was withheld.

Replies are green now and red means the node said no, which is what red should
have meant all along. The "> " a getter prefixes its value with is dropped for
display — on the serial console it sets the value apart, here it collides with
the prompt glyph that means "you typed this". The mock emits that marker too;
had it done so from the start, this would have shown up before the flash.

Second: discover.neighbors and discover.scopes did not autocomplete, because
MyMesh::handleCommand intercepts a few commands before delegating to CommonCLI
and the table was built by reading CommonCLI alone. setperm, `get acl` and
`shutdown` were missing for the same reason.

The audit could not have caught that: it drove every command the table offered
and checked the mock answered, which only finds gaps in one direction. It now
also reads the command literals the firmware dispatches on — across CommonCLI,
CommonCLI_Observer and MyMesh — and fails on any the table does not offer. That
check found `shutdown` immediately.

122 commands, all answered, none missing. 22 batch + 14 keys tests pass.
This commit is contained in:
agessaman
2026-08-08 08:14:54 -07:00
parent d7109c185c
commit d532e4ea86
4 changed files with 100 additions and 13 deletions
+43 -4
View File
@@ -64,7 +64,33 @@ def table():
return gets + plain
# set -> get pairs, checking a value survives the round trip.
# Where top-level commands are implemented. MyMesh handles a few before
# delegating to CommonCLI, which is exactly how discover.* stayed missing from
# the table for so long: grepping CommonCLI alone does not see them.
COMMAND_SOURCES = [
"src/helpers/CommonCLI.cpp",
"src/helpers/CommonCLI_Observer.cpp",
"examples/simple_repeater/MyMesh.cpp",
]
# Firmware commands the table deliberately does not offer.
NOT_OFFERED = {
"tls.bundletest", # TLS debugging, not an operator command
}
def firmware_commands():
"""Top-level command literals the firmware dispatches on."""
found = set()
for rel in COMMAND_SOURCES:
path = os.path.join(HERE, "..", rel)
try:
src = open(path, encoding="utf-8").read()
except OSError:
continue
for lit in re.findall(r'(?:mem|str)n?cmp\(\s*command\s*,\s*"([^"]+)"', src):
found.add(lit.strip())
return found - NOT_OFFERED
ROUND_TRIPS = [
("set radio.watchdog 30", "get radio.watchdog", "30"),
("set dutycycle 25", "get dutycycle", "25.0"),
@@ -153,15 +179,28 @@ def main():
print(" FAIL %-30s %s" % (cmd, reply))
failures += unexpected
# The reverse direction: a command the firmware implements but the table
# never offers is invisible to the check above, because the check only ever
# drives what the table already knows about.
offered = " ".join(cmds) + " " + " ".join(
re.findall(r'\["([^"]+)","', open(INDEX_HTML, encoding="utf-8").read()))
missing = sorted(c for c in firmware_commands() if c not in offered)
print("\nfirmware commands not in the table: %d" % len(missing))
for c in missing:
print(" MISSING %s" % c)
failures += [(c, "not offered by autocomplete") for c in missing]
results = cli.run([c for probe in ROUND_TRIPS for c in probe[:2]])
print("\nround-trips : %d" % len(ROUND_TRIPS))
for i, (setc, getc, want) in enumerate(ROUND_TRIPS):
setr, getr = results[i * 2][1], results[i * 2 + 1][1]
if setr["ok"] and getr["reply"] == want:
# `get` answers "> value"; compare the value, as the terminal displays it
got = re.sub(r"^>\s?", "", getr["reply"])
if setr["ok"] and got == want:
continue
print(" FAIL %-30s got %r, wanted %r (set: %s)"
% (getc, getr["reply"], want, setr["reply"]))
failures.append((getc, getr["reply"]))
% (getc, got, want, setr["reply"]))
failures.append((getc, got))
print("\n%s" % ("FAILED: %d" % len(failures) if failures else "all clear"))
return 1 if failures else 0
+23
View File
@@ -449,6 +449,7 @@ GETTERS = {
"%2d. %s%s" % (i + 1, n, "" if nd == "none" else " (needs %s)" % nd)
for i, (n, nd) in enumerate(PRESETS)),
"role": lambda c: "Repeater",
"acl": lambda c: "a1b2c3d4e5f60718 perms 3\n1122334455667788 perms 1",
# not its own pref: the CLI derives it from airtime_factor both ways
"dutycycle": lambda c: "%.1f" % (100.0 / (c["radio"]["af"] + 1)),
"mqtt.config.valid": lambda c: (
@@ -475,6 +476,18 @@ def cli_mqtt_status(cfg):
def cli_get(cfg, key):
"""Reply to `get <key>`.
CommonCLI::handleGetCmd answers `> value` — the marker sets the value apart
on the serial console. Reproduced here because it is load-bearing: a reply
that starts with "> " does not start with "OK", which is what made the
firmware's first cut mark every getter a failure.
"""
ok, val = _cli_get_value(cfg, key)
return (ok, "> " + val) if ok else (ok, val)
def _cli_get_value(cfg, key):
if key in GETTERS:
val = GETTERS[key](cfg)
return (True, val) if val is not None else (False, "Error: unsupported")
@@ -536,6 +549,16 @@ def run_cli(cfg, line):
if cmd == "neighbors":
return True, ("d4e5f60718 -71 dBm snr 9.5 2m ago\n"
"1122334455 -94 dBm snr 2.0 14m ago")
# Handled by MyMesh::handleCommand before it delegates to CommonCLI.
if cmd == "discover.neighbors":
return True, "OK - Discover sent"
if cmd == "discover.scopes":
return True, "OK - scopes queued (18s discovery remaining)"
if cmd.startswith("setperm "):
parts = cmd[8:].split()
if len(parts) != 2 or not _hex64(parts[0]):
return False, "Err - bad params"
return True, "OK"
if cmd == "clock sync":
return True, "OK - clock set: %s UTC" % time.strftime("%H:%M - %d/%m/%Y", time.gmtime())
if cmd == "region":
+16 -5
View File
@@ -43,6 +43,16 @@ static inline bool wcIsDeferredReboot(const char* cmd) {
static inline bool wcCliEchoesSecret(const char* cmd) {
return strncmp(cmd, "password ", 9) == 0;
}
// Success has no single shape across the CLI: setters answer "OK...", getters
// answer "> value", and a few answer free-form ("File system erase: OK"). Only
// FAILURE is uniform — an "Err"/"ERR:"/"Error:" prefix. So the CLI must test for
// the failure prefix, where the config batch can test for "OK" because every
// allowlisted setter uses it. Testing for "OK" here marked every `get` a
// failure, which turned the terminal red and withheld requested reboots.
static inline bool wcCliReplyIsOk(const char* r) {
return !((r[0] == 'E' || r[0] == 'e') && (r[1] == 'R' || r[1] == 'r') &&
(r[2] == 'R' || r[2] == 'r'));
}
// Constant-time-ish comparison so login timing doesn't leak a prefix match.
static bool fixedTimeEquals(const char* a, const char* b, size_t max_len) {
@@ -349,10 +359,11 @@ void WebConfigServer::drainBatch(uint32_t now) {
// Overwrite it: the command cannot fail, so there is nothing to report.
// Config entries carry the key; a CLI entry is matched on the command.
if (wcIsAdminPasswordKey(e.key) || wcCliEchoesSecret(e.cmd)) strcpy(e.reply, "OK");
// Success convention across every allowlisted setter is an "OK" prefix
// (the UI relies on the same test); anything else is a rejection.
_batch_all_ok = WebConfigBatch::nextAllOk(_batch_all_ok,
strncmp(e.reply, "OK", 2) == 0);
// A config batch is all allowlisted setters, which uniformly answer "OK";
// the CLI reaches the whole surface, where only failure has a fixed shape.
_batch_all_ok = WebConfigBatch::nextAllOk(
_batch_all_ok, _batch_kind == BATCH_CLI ? wcCliReplyIsOk(e.reply)
: strncmp(e.reply, "OK", 2) == 0);
}
_batch_last_cmd = millis();
// Config entries are named by their (non-secret) key. A CLI command is
@@ -1114,7 +1125,7 @@ void WebConfigServer::handleCliResult(AsyncWebServerRequest* req) {
JsonObject r = results.createNestedObject();
// The command is deliberately NOT echoed: it may hold a password or token,
// and the client already has the sequence it sent. It matches by index.
r["ok"] = strncmp(_batch[i].reply, "OK", 2) == 0;
r["ok"] = wcCliReplyIsOk(_batch[i].reply);
r["reply"] = (const char*)_batch[i].reply;
}
if (final_read) {
+18 -4
View File
@@ -150,7 +150,9 @@ body.tab-cli{padding-bottom:0}
.term-out>div{white-space:pre-wrap;word-break:break-word}
.term-out .cmd{color:#fff}
.term-out .cmd:before{content:"> ";color:var(--tgrn)}
.term-out .rep{color:#a9b7c6}
/* A reply is green because it succeeded; red is reserved for a node that said
no. Anything else reads every `get` as a failure. */
.term-out .rep{color:#7ee787}
.term-out .err{color:var(--terr)}
.term-out .sys{color:var(--tdim)}
.term-out .gap{height:.55em}
@@ -1444,6 +1446,11 @@ var CLI_VERBS=[
["memory","Heap and PSRAM free/min/largest block"],
["neighbors","Nodes heard recently, with RSSI and age"],
["neighbor.remove ","Drop one neighbour {64-hex-char-key}"],
// Handled by MyMesh before it delegates to CommonCLI — a whole second
// command surface the table missed until discover.* turned up absent.
["discover.neighbors","Ask neighbours to identify themselves"],
["discover.scopes","Collect neighbour scopes (needs the neighbors build)"],
["setperm ","Set a node's ACL permissions {64-hex-char-key} {int8}"],
["advert","Send an advert now (flooded)"],
["advert.zerohop","Send an advert neighbours will not repeat"],
["tempradio ","Try radio params without saving {freq,bw,sf,cr}"],
@@ -1480,6 +1487,7 @@ var CLI_VERBS=[
["reboot","Restart the node"],
["clkreboot","Restart the node, preserving the clock"],
["poweroff","Power the node off"],
["shutdown","Power the node off (same as poweroff)"],
["erase","Erase the filesystem — settings and identity"]
];
/* [key, description, mode, values]
@@ -1490,6 +1498,7 @@ var CLI_KEYS=[
["lat","Advert latitude",0],
["lon","Advert longitude",0],
["public.key","This node's public key",1],
["acl","Access control list (per-node permissions)",1],
["prv.key","Restore an identity {64-hex-char-key}",2],
["role","Node role",1],
["radio","Radio parameters {freq,bw,sf,cr}",0],
@@ -1975,9 +1984,14 @@ function cliPoll(reqid,cmds,from,status,errs,idles){
// The node never echoes the command back — it may hold a password or a
// token, and we already have the sequence we sent. Match by index.
cliEcho("cmd",cmds[from+i]||"");
// `ok` is advisory; the node's own OK/Error convention is authoritative
var ok=(x.ok!=null)?x.ok:!/^\s*(err|error)\b/i.test(x.reply||"");
if(x.reply)cliEcho(ok?"rep":"err",x.reply);
// `ok` is advisory; the node's own convention is authoritative, and only
// failure has a fixed shape there ("Err"/"ERR:"/"Error:").
var ok=(x.ok!=null)?x.ok:!/^\s*err/i.test(x.reply||"");
// Getters answer "> value" — that leading marker is the serial console's
// way of setting a value apart, and here it collides with the prompt glyph
// that means "you typed this". Drop it; the colour already says "reply".
var reply=(x.reply||"").replace(/^>\s?/,"");
if(reply)cliEcho(ok?"rep":"err",reply);
});
from+=(r.results||[]).length;
// "running" also covers "finished, but more results are still to be paged