review: teach the gates what a broken cli line looks like

[widgetii]

Problem

Two mistakes around cli are completely silent on a camera, and neither the compliance
gates nor best_practices.md had anything to say about either. Both have shipped.

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's t40_lite_movols-mo-805p has been shipping six of them behind
a trailing colon:

cli -s .video0.bitrate: 4000
cli -s .video0.rcMode: avbr

Bitrate, rate-control mode, profile, GOP size, GOP mode and OSD size have never applied on
that camera.

The signal. majestic reloads on SIGHUP and on nothing else.
general/package/sigmastar-osdrv-infinity6e/files/script/zoom.sh sends killall -10 in
nine places. Signal 10 is SIGUSR1, which majestic's bundled thread pool catches to park a
thread and never resumes (#2365) — so those crops were written to the file and never
applied.

And a signal is only safe once majestic can catch it. S95majestic starts it with
start-stop-daemon -b, which returns at the fork, so it is visible to pidof long before
main() installs a handler — and until then SIGHUP's default action is to terminate.
Measured on a lab hi3516ev200:

process visible, SigCgt=0000000000000000 -> SIGHUP would KILL it
SIGHUP handler installed, SigCgt=0000000000004007

What this adds

• pr_compliance_checklist.yaml — one rule taking the binary half: a literal path
  carrying a stray character or an empty component, and a signal that is not SIGHUP. It
  explicitly exempts a path built at runtime (cli -g ".$1" in builder's
  uvc-gadget-setup is correct and undecidable from a diff), anything under .github/ or
  contrib/, and cli used against another file through an explicit -i.
• best_practices.md §7.2 — the judgement half and the reasoning, next to §7.1's
  existing note on shell portability.

Why a rule rather than only a linter

Both. OpenIPC/builder#146 lints the path half mechanically, over the tree where the 709
shipped cli -s lines actually live — that is the deterministic, free check, and it found
the six bad lines immediately. What a linter cannot judge is whether a well-formed key is
one a given majestic build declares (it varies by vendor and flavour), or whether adding a
killall to a boot-time script is safe. That is what the review rule is for.

Evidence

This is review configuration: it reaches no image, and the selector agrees.

$ git diff --name-only origin/master | python3 .github/scripts/ci-matrix.py --stdin
ci-matrix: 0/99 boards (needs_build=False) --- nothing that reaches a build
needs-build=false

$ python3 .github/scripts/ci-matrix.py --self-test
ci-matrix: self-test ok (99 boards, 134 packages, 56 cases)

$ python3 -c "import yaml; d=yaml.safe_load(open('pr_compliance_checklist.yaml')); print(len(d['pr_compliances']), 'rules')"
13 rules

Hardware tested on

Not applicable, and the template says so: this is review configuration and cannot alter
what the firmware does on a camera. The SigCgt measurement quoted above comes from the
work in #2366, on a lab hi3516ev200; nothing in this diff runs anywhere but Qodo.

Scope

• No kernel patches under general/package/all-patches/linux/
• No files specific to a single retail camera model
• No probing or bring-up tooling
• Nothing under general/overlay/ or in a shared load_<vendor> script hardcodes a value specific to my board
• No LD_PRELOAD, and no binaries that cannot be rebuilt from source

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Add review gates for safe CLI paths and Majestic reloads

✨ Enhancement 📝 Documentation ⚙️ Configuration changes 🕐 10-20 Minutes

AI Description

• Add compliance checks for malformed literal CLI paths and incorrect Majestic reload signals.
• Document silent configuration failures, SIGHUP timing hazards, and justified review exceptions.

Diagram

graph TD
  A["PR Diff"] --> B["Qodo Gate"] --> C["Literal CLI Check"] --> D{"Pass or Flag"}
  B --> E["Reload Signal Check"] --> D
  F["Scoped Exemptions"] --> B
  G["Best Practices"] --> H["Reviewer Judgment"] --> D

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Firmware-tree shell linter

• ➕ Scans the complete tree rather than only changed lines
• ➕ Provides deterministic and immediate feedback for malformed literal paths
• ➖ Cannot reliably evaluate dynamically constructed paths
• ➖ Cannot confirm that a well-formed key exists in every Majestic variant
• ➖ Requires maintaining parsing logic for shell syntax and CLI option combinations

2. Runtime Majestic schema validation

• ➕ Could reject well-formed but unsupported keys at the point of use
• ➕ Would protect scripts and manual commands outside pull-request review
• ➖ Requires broader runtime and compatibility changes
• ➖ Available settings vary across vendors and firmware flavors
• ➖ Does not independently solve unsafe or incorrect process signalling

Recommendation: Keep the narrow compliance rule and supporting reviewer guidance because they add high-confidence protection without changing firmware behavior. Continue pairing this approach with tree-wide linting where scripts are owned; consider runtime schema validation separately if Majestic can expose a stable, flavor-aware settings schema.

Files changed (2) +58 / -0

Documentation (1) +33 / -0

best_practices.mdDocument CLI path and Majestic reload review guidance +33/-0

Document CLI path and Majestic reload review guidance

• Adds section 7.2 explaining why malformed CLI setting paths fail silently, why Majestic reloads require SIGHUP, and why immediate boot-time signalling can terminate Majestic. It also defines applicable directories and context-dependent cases requiring reviewer judgment.

best_practices.md

Other (1) +25 / -0

pr_compliance_checklist.yamlAdd compliance rule for CLI paths and reload signals +25/-0

Add compliance rule for CLI paths and reload signals

• Adds an objective review rule that flags malformed literal paths passed to CLI tools and non-SIGHUP Majestic reload signals. The rule exempts runtime-generated paths, explicit alternate input files, and scripts under '.github/' or 'contrib/'.

pr_compliance_checklist.yaml

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Canonical paths appear invalid ✓ Resolved 🐞 Bug ≡ Correctness

Description

The success criteria forbid empty path components without exempting yaml-cli’s canonical leading
root dot, so literal paths such as .video0.size can be rejected as having an empty first
component. Because compliance instructions require literal application, valid newly added cli and
sensor_cli calls may fail the gate.

Code

pr_compliance_checklist.yaml[R191-193]

+      Every literal path passed to cli/yaml-cli/sensor_cli -s, -g or -d looks like a YAML
+      key path — components of letters, digits, underscore, dot or dash, no trailing
+      separator, no empty component. A path built at runtime, such as `cli -g ".$1"`, is

Evidence

The cli and sensor_cli wrappers pass paths directly to yaml-cli, and shipped scripts
consistently use a leading dot for valid paths. The review configuration explicitly tells the
compliance agent to apply checklist wording literally.

general/overlay/usr/sbin/extutils[14-20]
general/package/ingenic-osdrv-t31/files/script/load_ingenic[37-48]
.pr_agent.toml[120-122]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Clarify the path grammar so yaml-cli's canonical leading root dot is explicitly valid while consecutive dots and trailing separators remain invalid.
## Issue Context
Repository calls consistently use paths such as `.video0.size` and `.sensor.width`, while the compliance configuration is applied literally.
## Fix Focus Areas
- pr_compliance_checklist.yaml[190-204]
- best_practices.md[414-422]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Nested package scripts excluded ✓ Resolved 🐞 Bug ≡ Correctness

Description

The scope general/package/*/files/ only describes packages one directory below general/package,
so the rule misses shipped scripts in nested package layouts such as
general/package/legacy/datalink/files/. A newly introduced malformed cli path in those scripts
would therefore evade the new gate and guidance.

Code

pr_compliance_checklist.yaml[R197-199]

+      The diff adds a cli path carrying a stray character — a trailing colon or equals is
+      the shipped case — or an empty component, in general/overlay/ or
+      general/package/*/files/. Or it signals majestic with anything other than SIGHUP:

Evidence

The nested legacy package contains numerous literal cli setting paths, but its files directory
does not match the stated one-level general/package/*/files/ scope.

general/package/legacy/datalink/files/tweaksys[45-53]
general/package/legacy/datalink/files/tweaksys[93-100]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Expand the package-file scope to include nested package directories rather than only one directory level beneath `general/package`.
## Issue Context
The repository contains shipped cli calls under nested paths such as `general/package/legacy/datalink/files/tweaksys`.
## Fix Focus Areas
- pr_compliance_checklist.yaml[196-204]
- best_practices.md[438-440]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Lifecycle signals fail reload gate ✓ Resolved 🐞 Bug ≡ Correctness

Description

The failure criteria reject every non-SIGHUP signal sent to majestic rather than only signals
intended to reload configuration. This incorrectly fails intentional lifecycle operations such as
sysupgrade’s SIGQUIT, which releases majestic’s SDK while deliberately keeping the process alive.

Code

pr_compliance_checklist.yaml[R199-201]

+      general/package/*/files/. Or it signals majestic with anything other than SIGHUP:
+      signal 10 is SIGUSR1, which the bundled thread pool catches to park a thread and
+      never resumes, so `killall -10 majestic` reloads nothing. Do not raise this for a

Evidence

Sysupgrade sends signal 3 intentionally, and its comments document that SIGQUIT releases the SDK
without terminating majestic. The added blanket criterion would classify the same legitimate pattern
as a failure if added or changed in a future diff.

general/overlay/usr/sbin/sysupgrade[474-480]
general/overlay/usr/sbin/sysupgrade[524-531]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Restrict the non-SIGHUP failure criterion to signals used as configuration reload attempts, and preserve intentional stop or lifecycle signals.
## Issue Context
Sysupgrade intentionally sends SIGQUIT to release majestic's SDK; this is not a reload request and must not fail the rule.
## Fix Focus Areas
- pr_compliance_checklist.yaml[194-204]
- best_practices.md[424-440]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Guidance contradicts configured gate ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The guidance says neither check is a compliance gate even though this PR adds both checks as a
compliance_label: true checklist entry. This gives the review system conflicting instructions
about whether violations are hard failures or judgment-based findings.

Code

best_practices.md[R438-440]

+Raise both against files under `general/overlay/` and `general/package/*/files/`. Neither
+is a compliance gate: a dynamic path (`cli -g ".$1"`) is legitimate and undecidable from
+the diff, and there are reasons to signal by hand.

Evidence

The checklist entry explicitly sets compliance_label: true, the repository describes checklist
entries as hard gates, and the review-agent configuration requires literal checklist application.

pr_compliance_checklist.yaml[181-183]
best_practices.md[444-447]
.pr_agent.toml[120-122]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Remove the contradiction between best-practices guidance and the newly configured compliance gate.
## Issue Context
The checklist marks the combined path and signal rule as a compliance rule, while the documentation says neither check is a compliance gate.
## Fix Focus Areas
- best_practices.md[438-440]
- pr_compliance_checklist.yaml[181-204]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗