From d532e4ea86d5cfd51b37bc08c04eadeeb2080d1a Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 8 Aug 2026 08:14:27 -0700 Subject: [PATCH] fix(webconfig): correct reply classification and the missing MyMesh commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- scripts/webconfig_cli_audit.py | 47 ++++++++++++++++++++++++--- scripts/webconfig_mock_server.py | 23 +++++++++++++ src/helpers/esp32/WebConfigServer.cpp | 21 +++++++++--- webui/index.html | 22 ++++++++++--- 4 files changed, 100 insertions(+), 13 deletions(-) diff --git a/scripts/webconfig_cli_audit.py b/scripts/webconfig_cli_audit.py index 1bca892f..4868c705 100644 --- a/scripts/webconfig_cli_audit.py +++ b/scripts/webconfig_cli_audit.py @@ -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 diff --git a/scripts/webconfig_mock_server.py b/scripts/webconfig_mock_server.py index 103204bd..e85852c9 100644 --- a/scripts/webconfig_mock_server.py +++ b/scripts/webconfig_mock_server.py @@ -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 `. + + 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": diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index 92ab5e89..3eb0fb4e 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -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) { diff --git a/webui/index.html b/webui/index.html index 6d8e87ae..7d4a1107 100644 --- a/webui/index.html +++ b/webui/index.html @@ -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