ci: check the setting paths shipped scripts hand to cli - #146
Merged
Conversation
`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.
widgetii
requested review from
cronyx,
flyrouter and
viktorxda
as code owners
September 5, 2026 10:13
PR Summary by QodoLint shipped CLI paths and fix malformed Movols settings
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
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.
widgetii
added a commit
to OpenIPC/firmware
that referenced
this pull request
Sep 5, 2026
Two mistakes around `cli` are silent on the camera, and neither the compliance gates nor best_practices.md had anything to say about either. The path. `cli -s` cannot fail: yaml-cli stores whatever dotted path it is handed, creating the intermediate mappings as it goes, and exits 0; majestic then ignores a key it does not recognise. So a typo applies nothing and reports nothing for the life of the device. OpenIPC/builder shipped six of them on t40_lite_movols-mo-805p behind a trailing colon -- bitrate, rate-control mode, profile, GOP size, GOP mode and OSD size, none of which ever applied. The signal. majestic reloads on SIGHUP and on nothing else. infinity6e's zoom.sh sends `killall -10` in nine places; signal 10 is SIGUSR1, which the bundled thread pool catches to park a thread and never resumes (#2365), so those crops were never applied either. And a signal is only safe once majestic can catch it: S95majestic starts it with start-stop-daemon -b, so it is visible to pidof with SigCgt still 0000000000000000, and SIGHUP's default action is to terminate. The compliance rule takes the binary half -- a literal path carrying a stray character or an empty component, and a signal that is not SIGHUP -- and explicitly exempts a path built at runtime, which is legitimate and undecidable from a diff. §7.2 of best_practices.md takes the judgement half and the reasoning. OpenIPC/builder#146 lints the path half of this mechanically, over the tree where the 709 shipped `cli -s` lines actually live.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
cli -scannot fail. yaml-cli stores whatever dotted path it is given, creating theintermediate 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 — not at
build time, not at first boot, not ever.
devices/t40_lite_movols-mo-805p/general/overlay/usr/share/openipc/customizer.shhas beenshipping six of them:
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. It is the only device in
the tree with the bug, and nothing in CI or review was ever in a position to notice.
What this adds
.github/scripts/lint-cli-paths.py, wired into the existinglintjob beside the twocheckers already there. No runners, no matrix, seconds.
It checks shape and nothing else, deliberately. 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, and
curl localhost/api/v1/config.jsonis the thing that knows.Shape is decidable from the tree alone, costs nothing, and is the whole of the class that
shipped here.
A path built at runtime is skipped rather than guessed at:
devices/gk7205v200_otg_generic/.../uvc-gadget-setupreadscli -g ".$1"out of a helper,which is correct and unknowable statically. Markdown is not scanned either —
CLAUDE.mdspells paths as
.<path>on purpose.It carries a floor check for the same reason
lint-workflow-shell.pydoes: a matcher thatquietly stops matching finds no bad paths, which looks exactly like a clean tree. Scanning
zero
cliinvocations is therefore itself a failure.Also in this PR
The six paths are fixed, since the linter fails on them otherwise. That includes the
commented-out
#cli -s .onvif.enabled:, which was a trap waiting for whoever uncommented it.Evidence
Before:
After:
Context
This came out of OpenIPC/firmware#2366, which makes
cli -sask majestic to apply thechange and refuses a malformed path at runtime. That helps the next camera; it does nothing
for a path already committed here, which is what this catches.
Not tested on a camera, and it does not need to be: the linter is CI-only and never reaches
an image. The one file it changes is a device customizer whose six edited lines have never
had any effect — the change is from "silently does nothing" to "does what it says", and the
script still parses under busybox ash.