fix(webconfig): tighten CLI failure detection, reboot deferral and refusals

Five findings from review, all confirmed against the source.

Failure classification (P2). Testing replies for an "Err" prefix passed five
other shapes off as success: "Unknown command", "unknown config: x", "??: x",
"Can't find GPS", "(ERR: clock cannot go backwards)" and "File system erase:
Err". They rendered green, and worse, left _batch_all_ok true — so a queued
reboot went ahead after commands that had failed, defeating the gate entirely.

Rather than lengthen one guess, the two questions are now asked separately,
each erring safe:

  - colour asks "does this look like a failure", against every shape CommonCLI
    actually emits, enumerated in WebConfigBatch.h and pinned by a host test
    that uses the literal strings. Getting this wrong is cosmetic.
  - the reboot gate asks something narrower and answerable: "did every setting
    I asked for take". Only `set`/`password` gate it, and only on the "OK"
    prefix every setter keeps. Diagnostics no longer gate a reboot at all, so a
    harmless `memory` cannot strand one and no guess is made about "> value".

Reboot deferral (P2). CommonCLI dispatches on a six-byte prefix, so `reboot
now` and `rebooted` reach Board::reboot() too. Matching exactly meant those
variants skipped both the confirmation and the deferral and took the node down
mid-drain — the precise failure deferral exists to prevent. Both sides now
anchor the way the firmware dispatches, and the UI's risk matcher with them.

Three commands the portal cannot honestly serve are refused at POST with a
reason, and dropped from autocomplete, instead of running and lying:

  - `start ota` builds a second AsyncWebServer on port 80 with no bind check
    and answers "Started" regardless; the portal already holds that port, so it
    could only leak the allocation and inhibit sleep.
  - `clock sync` takes its time from the caller's timestamp, which a web
    request has none of, so CommonCLI always rejected it. `time <epoch>` works
    and remains offered.
  - bare `log` and `get acl` write their real output to Serial and hand back a
    stub the terminal showed as success; `log` also streams a whole file from
    the loop task, stalling the mesh and radio while it does.

The mock now emits the same failure shapes it used to fake as successes, so
these are reproducible off-hardware. 24 batch + 14 keys tests pass; audit
reports 119/119 answered, 0 missing, 4/4 refused with a reason.
This commit is contained in:
agessaman
2026-08-08 08:40:49 -07:00
parent 75d4656e59
commit c831e599ec
6 changed files with 284 additions and 47 deletions
@@ -221,6 +221,67 @@ TEST(WebConfigBatch, CliReadIsDoneOnlyOnceEveryResultHasBeenHandedOver) {
EXPECT_TRUE(Batch::cliReadIsFinal(State::Done, /*from=*/20, /*page=*/0, /*total=*/20));
}
// Every string below is a literal lifted from CommonCLI.cpp /
// CommonCLI_Observer.cpp. Testing only for an "Err" prefix passed five of these
// off as success, which both coloured them green and let a queued reboot go
// ahead after them.
TEST(WebConfigBatch, CliFailureRepliesAreRecognisedInEveryShapeCommonCLIEmits) {
const char* failures[] = {
"Err - bad params", // MyMesh setperm
"ERR: bad pubkey", // neighbor.remove
"Error: IATA code must be exactly 3 letters",// observer setters
"(ERR: clock cannot go backwards)", // clock sync, parenthesised
"Unknown command", // top-level fallthrough
"unknown config: mqtt.nope", // set fallthrough
"??: mqtt.nope", // get fallthrough
"Can't find GPS", // gps
"File system erase: Err", // failure reported at the end
};
for (const char* f : failures) {
EXPECT_TRUE(Batch::cliReplyIsFailure(f)) << f;
}
const char* successes[] = {
"OK",
"OK - slot 1 preset: meshrank",
"> 22", // getter value
"> msgs: on, 1: analyzer-us (ok)", // getter, contains "ok"
"File system erase: OK", // same shape, succeeded
"Free: 142832, Min: 126808", // memory
"v1.16.0 (Build: 6 Jun 2026)", // ver
};
for (const char* s : successes) {
EXPECT_FALSE(Batch::cliReplyIsFailure(s)) << s;
}
// An empty reply is normalised to "OK" before it ever reaches the client.
EXPECT_FALSE(Batch::cliReplyIsFailure(""));
EXPECT_FALSE(Batch::cliReplyIsFailure(NULL));
}
TEST(WebConfigBatch, OnlyWritesGateTheDeferredReboot) {
// Writes gate it: these are what can leave a config not worth rebooting into.
EXPECT_TRUE(Batch::cliReplyGatesReboot("set tx 22"));
EXPECT_TRUE(Batch::cliReplyGatesReboot("set mqtt1.preset meshrank"));
EXPECT_TRUE(Batch::cliReplyGatesReboot("password hunter2"));
// Diagnostics do not. `memory` answering "Free: ..." must not be read as a
// failure and strand the operator's reboot, and a getter's "> value" must not
// be read as a success either — neither is asked.
EXPECT_FALSE(Batch::cliReplyGatesReboot("memory"));
EXPECT_FALSE(Batch::cliReplyGatesReboot("get tx"));
EXPECT_FALSE(Batch::cliReplyGatesReboot("reboot"));
EXPECT_FALSE(Batch::cliReplyGatesReboot("advert"));
EXPECT_FALSE(Batch::cliReplyGatesReboot(NULL));
// "settle" must not be mistaken for a `set`; the space is part of the token.
EXPECT_FALSE(Batch::cliReplyGatesReboot("settle"));
// A write counts only on the "OK" prefix every setter keeps.
EXPECT_TRUE(Batch::cliWriteSucceeded("OK"));
EXPECT_TRUE(Batch::cliWriteSucceeded("OK - reboot to apply"));
EXPECT_FALSE(Batch::cliWriteSucceeded("unknown config: nope"));
EXPECT_FALSE(Batch::cliWriteSucceeded("Error: expected a number"));
EXPECT_FALSE(Batch::cliWriteSucceeded(""));
}
TEST(WebConfigBatch, CliRebootIsWithheldWhenAnyCommandInTheSequenceFailed) {
EXPECT_TRUE(Batch::cliRebootAllowed(/*has_reboot=*/true, /*all_ok=*/true));
// Same rule a config save follows: do not reboot into a half-applied config