From 1bda6f336f4ee16626c001c302c1ac2c38ecf637 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Sat, 5 Sep 2026 13:12:47 +0300 Subject: [PATCH 1/2] ci: check the setting paths shipped scripts hand to cli `cli -s` cannot fail. yaml-cli stores whatever dotted path it is given, creating intermediate mappings as it goes, and exits 0; majestic then silently ignores a key it does not recognise. So a typo in a setting path applies nothing and reports nothing, from the moment it is written to the end of the device's life. t40_lite_movols-mo-805p has been shipping six of them: cli -s .video0.bitrate: 4000 cli -s .video0.rcMode: avbr The path is `.video0.bitrate:`, not `.video0.bitrate`. Bitrate, rate-control mode, profile, GOP size, GOP mode and OSD size have never applied on that camera. The trailing colons are dropped here, including the one on the commented-out `.onvif.enabled:`, which was a trap for whoever uncommented it. The linter checks shape and nothing else. Whether a well-formed key is one a given majestic build actually declares can only be answered by the running binary -- it varies by vendor and by flavour -- while shape is decidable from the tree, costs nothing, and is the whole of the class that shipped. A path built at runtime is skipped rather than guessed at: uvc-gadget-setup reads `cli -g ".$1"`, which is fine and unknowable here. It carries a floor check for the same reason lint-workflow-shell.py does. A matcher that quietly stops matching finds no bad paths, which looks exactly like a clean tree -- so finding no cli invocation at all is itself a failure. 80 scripts scanned, 610 invocations, 6 bad paths, all in the one device. --- .github/scripts/lint-cli-paths.py | 170 ++++++++++++++++++ .github/workflows/lint.yml | 9 + .../overlay/usr/share/openipc/customizer.sh | 14 +- 3 files changed, 186 insertions(+), 7 deletions(-) create mode 100755 .github/scripts/lint-cli-paths.py diff --git a/.github/scripts/lint-cli-paths.py b/.github/scripts/lint-cli-paths.py new file mode 100755 index 000000000..2fbf8419e --- /dev/null +++ b/.github/scripts/lint-cli-paths.py @@ -0,0 +1,170 @@ +#!/usr/bin/env python3 +"""Check the setting paths every shipped script hands to `cli`. + +WHY THIS EXISTS. `cli -s` writes a value into /etc/majestic.yaml and cannot +fail: yaml-cli stores whatever dotted path it is given, creating the +intermediate mappings as it goes, and exits 0. Nothing downstream complains +either, because majestic simply never reads a key it does not recognise. A typo +in a setting path is therefore completely silent from the moment it is written +to the end of the device's life. + +devices/t40_lite_movols-mo-805p/general/overlay/usr/share/openipc/customizer.sh +is what this file is made of. Six of its lines carry a trailing colon: + + cli -s .video0.bitrate: 4000 + cli -s .video0.rcMode: avbr + +so the path is `.video0.bitrate:`, which is not `.video0.bitrate`. Those six +settings -- bitrate, rate-control mode, profile, GOP size, GOP mode and OSD +size -- have never applied on that camera, and no build, no review and no boot +ever said so. The device has shipped that way. + +SCOPE is the shape of the path, deliberately, and nothing else. Whether +`.video0.bitrate` is a key this majestic build actually declares is a question +only the running binary can answer (`curl localhost/api/v1/config.json` lists +what a given build has), and it varies by vendor and by flavour. Shape is what +can be decided from the tree alone, it is free, and it is the whole of the +class that shipped. + +A path built at runtime is skipped rather than guessed at -- +devices/gk7205v200_otg_generic/.../uvc-gadget-setup reads `cli -g ".$1"` out of +a helper, which is fine and unknowable here. + +Usage: + lint-cli-paths.py # check the tree + lint-cli-paths.py --self-test # check this checker still catches things +""" + +import re +import sys +from pathlib import Path + +# `cli`, `sensor_cli` and the `yaml-cli` binary underneath them all take the +# same grammar: a mode flag, then the dotted path. `wifibroadcast cli` is the +# same applet against /etc/wfb.yaml and is caught by the same rule. +INVOCATION = re.compile( + r"\b(?:yaml-cli|sensor_cli|cli)\s+" + r"(?:-[a-zA-Z]+\s+\S+\s+)*" # any -i/-o pair that came first + r"(-s|--set|-g|--get|-d|--del)\s+" + r"(\"[^\"]*\"|'[^']*'|\S+)" +) + +# A leading dot is convention rather than syntax (yaml-cli splits on "." and +# drops empty components), so it is optional here. What is not optional is that +# every component is a plausible YAML key. +WELL_FORMED = re.compile(r"^\.?[A-Za-z0-9_][A-Za-z0-9_.-]*$") + +# Interpolation of any kind means the path is not decidable from the tree. +DYNAMIC = re.compile(r"[$`]") + + +def path_is_malformed(path): + """True when this is a path yaml-cli will store and majestic never read.""" + if DYNAMIC.search(path): + return False + if not WELL_FORMED.match(path): + return True + # Empty components and a trailing separator address nothing. yaml-cli drops + # them silently rather than refusing, which is what makes them worth + # catching here. + return ".." in path or path.endswith(".") + + +def findings_in(text): + """(line number, offending path, source line) for each bad path in `text`.""" + out = [] + for lineno, line in enumerate(text.splitlines(), 1): + if line.lstrip().startswith("#"): + continue + for match in INVOCATION.finditer(line): + path = match.group(2).strip("\"'") + if path_is_malformed(path): + out.append((lineno, path, line.strip())) + return out + + +def is_shell(p): + """Shipped shell, by extension or by shebang. Markdown is not scanned: the + documentation spells paths as `.` on purpose.""" + if p.suffix == ".sh": + return True + try: + with p.open("rb") as f: + return f.read(2) == b"#!" and b"sh" in f.readline() + except OSError: + return False + + +def check_tree(root): + scanned = 0 + bad = [] + for p in sorted(Path(root).rglob("*")): + if not p.is_file() or ".git/" in str(p) or not is_shell(p): + continue + try: + text = p.read_text(errors="ignore") + except OSError: + continue + if not INVOCATION.search(text): + continue + scanned += 1 + for lineno, path, line in findings_in(text): + bad.append((p, lineno, path, line)) + + # A scan that finds nothing to check looks exactly like a clean run, which + # is the failure mode every silent-typo bug in this file is made of. + if scanned == 0: + print("FAIL: no script in the tree invokes cli -- is the matcher broken?") + return 1 + + for p, lineno, path, line in bad: + print(f"{p}:{lineno}: malformed setting path {path!r}") + print(f" {line}") + if bad: + print(f"\nFAIL: {len(bad)} malformed cli setting path(s) in {scanned} script(s)") + print("A path yaml-cli stores but majestic never reads applies nothing,") + print("and says nothing. Fix the path, or use a variable if it is dynamic.") + return 1 + print(f"ok cli setting paths well formed in {scanned} script(s)") + return 0 + + +def self_test(): + cases = [ + ("cli -s .video0.bitrate 4096", False, "plain set"), + ("cli -s .video0.bitrate: 4000", True, "the movols trailing colon"), + ("cli -s .osd.size: 0.8", True, "trailing colon, float value"), + ("cli -g .video0.size", False, "get"), + ("cli -d .outgoing.server", False, "delete"), + ("cli -s .isp.sensorConfig /etc/sensors/imx335.ini", False, "path-like value"), + ('cli -g ".$1"', False, "runtime path is not decidable"), + ("cli -s .nightMode.irCutPin1 11", False, "digits in a component"), + ("cli -s .outgoing.0 rtmp://a/b", False, "sequence index"), + ("cli -s .video0..fps 20", True, "empty component"), + ("cli -s .video0. 20", True, "trailing separator"), + ("cli -s .video0.fps=20 x", True, "= is not a separator"), + ("yaml-cli -i /etc/wfb.yaml -s .wireless.txpower 20", False, "explicit -i first"), + ("# cli -s .video0.bitrate: 4000", False, "commented out"), + ("sensor_cli -s .sensor.width 1920", False, "sensor_cli applet"), + ] + rc = 0 + for line, want_bad, name in cases: + got_bad = bool(findings_in(line)) + if got_bad != want_bad: + verb = "flag" if want_bad else "accept" + print(f"FAIL self-test: expected to {verb} {name}: {line!r}") + rc = 1 + else: + print(f"ok self-test: {name}") + return rc + + +def main(): + if "--self-test" in sys.argv[1:]: + return self_test() + root = sys.argv[1] if len(sys.argv) > 1 else "." + return check_tree(root) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index cb57e7953..f43a8c8a2 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -72,3 +72,12 @@ jobs: # by commits in that repository and not by anything in a PR here. - name: Check the drift checker still describes the tree run: python3 .github/scripts/check-firmware-drift.py --self-test + # `cli -s` cannot fail: yaml-cli stores whatever dotted path it is given, + # and majestic silently ignores a key it does not recognise. So a typo in + # a setting path applies nothing and says nothing, for the life of the + # device. t40_lite_movols-mo-805p shipped six of them behind a trailing + # colon. Same shape as the checks above -- no runners, no matrix, seconds. + - name: Check the cli path linter still catches what it should + run: python3 .github/scripts/lint-cli-paths.py --self-test + - name: Check every shipped cli setting path + run: python3 .github/scripts/lint-cli-paths.py diff --git a/devices/t40_lite_movols-mo-805p/general/overlay/usr/share/openipc/customizer.sh b/devices/t40_lite_movols-mo-805p/general/overlay/usr/share/openipc/customizer.sh index 82d29ac33..d7363b078 100644 --- a/devices/t40_lite_movols-mo-805p/general/overlay/usr/share/openipc/customizer.sh +++ b/devices/t40_lite_movols-mo-805p/general/overlay/usr/share/openipc/customizer.sh @@ -12,24 +12,24 @@ fw_setenv upgrade 'https://github.com/OpenIPC/builder/releases/download/latest/t # cli -s .isp.blkCnt 4 cli -s .video0.codec h265 -cli -s .video0.bitrate: 4000 -cli -s .video0.rcMode: avbr -cli -s .video0.profile: high -cli -s .video0.gopSize: 5 -cli -s .video0.gopMode: smart +cli -s .video0.bitrate 4000 +cli -s .video0.rcMode avbr +cli -s .video0.profile high +cli -s .video0.gopSize 5 +cli -s .video0.gopMode smart cli -s .nightMode.irCutPin1 122 cli -s .nightMode.irCutPin2 123 cli -s .nightMode.backlightPin 119 cli -s .osd.enabled true cli -s .osd.template "%d.%m.%Y %H:%M:%S" -cli -s .osd.size: 0.8 +cli -s .osd.size 0.8 #cli -s .audio.enabled true #cli -s .audio.codec aac #cli -s .audio.srate 48000 #cli -s .audio.volume 95 #cli -s .audio.gain 25 #cli -s .audio.speakerPin 7 -#cli -s .onvif.enabled: true +#cli -s .onvif.enabled true # # Set osmem and rmem # From c2c18cd05b57f76fdcfcae6a168aef578d46899f Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:32:16 +0300 Subject: [PATCH 2/2] ci: recheck the merged result when the cli linter itself changes lint.yml's push trigger names the checkers it guards one by one, and the new one was not on the list -- so a master push touching only .github/scripts/lint-cli-paths.py would have skipped the very job that exists because #121 shipped a broken merged result nothing re-checked. Same reasoning as the lint-workflow-shell.py entry beside it, and the same cost: seconds, no runners. --- .github/workflows/lint.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index f43a8c8a2..731c31b62 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -16,6 +16,7 @@ on: paths: - '.github/workflows/**' - '.github/scripts/lint-workflow-shell.py' + - '.github/scripts/lint-cli-paths.py' workflow_dispatch: # Reads the tree and reports; writes nothing. Declared rather than inherited so