From 50324699d20b89848adbd03eb4e4d469e6cee8a9 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Sun, 16 Aug 2026 15:46:02 +0300 Subject: [PATCH 1/3] qodo: add review standards, compliance gates, and a PR template Mirrors the three-file Qodo Merge setup already in use on majestic: .pr_agent.toml points the review agent at best_practices.md (judgement calls) and pr_compliance_checklist.yaml (binary gates). Rules were derived from PRs that were actually closed: #2241 device-under-test sensor hardcoded into general/overlay/ S70vendor and a shared load_hisilicon default; 1200 lines of unwired tools/imx662/; HISILICON_OPENSDK_SITE repointed at a personal fork while Config.in still named OpenIPC/openhisilicon #2260 toolchain flag with an unrun test plan and no hardware evidence #2234 vendor .ko files extracted from factory firmware, runtime patching of blob .bss, generated facade .so blobs, LD_PRELOAD in S95majestic, a kernel patch, and a single-board script They judge blast radius, provenance, evidence, and repo ownership. They deliberately do not judge writing style: .pr_agent.toml instructs the agent not to speculate about AI authorship or flag comment density and formatting, which is unfalsifiable and false-positives on thorough contributors. Checked against merged #2267, #2248, and #2266 so ordinary work does not trip a gate. Two carve-outs came out of that: adding a sensor to a load script is additive and passes (#2248 extends a case arm and changes no default), and the shell-convention gate is scoped to scripts that ship in the image, not .github/ or contrib/ (#2266). Co-Authored-By: Claude Opus 4.8 --- .github/PULL_REQUEST_TEMPLATE.md | 56 +++++ .pr_agent.toml | 91 ++++++++ best_practices.md | 352 +++++++++++++++++++++++++++++++ pr_compliance_checklist.yaml | 192 +++++++++++++++++ 4 files changed, 691 insertions(+) create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 .pr_agent.toml create mode 100644 best_practices.md create mode 100644 pr_compliance_checklist.yaml diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000000..4952135ea3 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,56 @@ + + +## Problem + + + +## Hardware tested on + + + +## Evidence + + + +Before: + +``` + +``` + +After: + +``` + +``` + +## Scope + + + +- [ ] No kernel patches under `general/package/all-patches/linux/` (those go to [OpenIPC/linux](https://github.com/OpenIPC/linux)) +- [ ] No files specific to a single retail camera model (those go to [OpenIPC/builder](https://github.com/OpenIPC/builder)) +- [ ] No probing or bring-up tooling (that goes to [OpenIPC/ipctool](https://github.com/OpenIPC/ipctool)) +- [ ] Nothing under `general/overlay/` or in a shared `load_` script hardcodes a value specific to my board +- [ ] Package sources come from an OpenIPC repository, pinned to a full 40-character SHA or a tag +- [ ] No `LD_PRELOAD`, and no binaries that cannot be rebuilt from source +- [ ] New code is selected by a defconfig, so CI actually builds it diff --git a/.pr_agent.toml b/.pr_agent.toml new file mode 100644 index 0000000000..72dbee7874 --- /dev/null +++ b/.pr_agent.toml @@ -0,0 +1,91 @@ +# Qodo code-review configuration. +# +# Review standards live in two companion files at the repo root: +# best_practices.md - judgement-based rules, imported as Review Standards +# pr_compliance_checklist.yaml - binary hard gates +# +# Reference: https://docs.qodo.ai/install-and-configure/configuration-overview/configuration-file + +[github_app] +pr_commands = [ + "/agentic_describe", + "/agentic_review", +] + +[review_agent] +comments_location_policy = "both" + +# 1 = all severities inline, 2 = high+medium, 3 = high only. +# 2 keeps blast-radius and provenance findings inline without burying the diff in nits. +inline_comments_severity_threshold = 2 + +issues_user_guidelines = """ +OpenIPC/firmware is a Buildroot tree that builds images for roughly 90 boards across 13 +SoC vendors. general/overlay/ and general/package/ are shared: one file there reaches +every camera of a family. Those cameras are installed in places nobody can physically +reach, there is no staged rollout, and a bad change is discovered only after sysupgrade +has already written it to flash. Weight findings by blast radius, not by diff size - a +two-line change to a shared defconfig outranks a hundred lines in a new leaf package. + +Prioritise, in order: + +1. Device-specific values written into generic configuration. general/overlay/ ships to + every camera, and the *-osdrv-*/files/script/load_ scripts serve a whole SoC + family. A sensor, I2C address, GPIO number, or resolution that is true only of the + contributor's bench must not become the default for everyone else. This breaks already + deployed cameras at upgrade time, which is the worst failure mode this project has. + +2. Buildroot defaults replaced instead of extended. BR2_TARGET_OPTIMIZATION and its peers + are whole-value assignments: setting one to a single new flag silently drops the -Os + -pipe that every package was built with. The symptom is a size and performance + regression across the entire image, with nothing in the diff naming it. + +3. Provenance. A package SITE must point at an OpenIPC-org repository or a documented + upstream, never a contributor's personal fork - a fork is a single point of failure for + every board that builds the package, and it can be rewritten or deleted without notice. + Version pins must be full 40-character SHAs or tags. Config.in help text must name the + URL the .mk actually fetches. Binaries lifted out of a camera's factory firmware are + not a source of supply: there is no source, no vendor SDK, and no way to rebuild them + for the next kernel. + +4. Changes that belong to a different repository. Kernel code and kernel patches belong to + OpenIPC/linux. Support for one retail camera model belongs to OpenIPC/builder, under + devices/common/br-ext-chip-/ - general/overlay/usr/sbin/sysupgrade already + encodes this split, routing lite|ultimate|neo to firmware and everything else to + builder. Hardware probing and bring-up tools belong to OpenIPC/ipctool. Bugs in the + majestic streamer belong to majestic's maintainers. Redirecting a contributor is a + normal, useful review outcome; say which repo and why. + +5. Monkey-patching in place of a fix. LD_PRELOAD shims are not acceptable anywhere in the + OpenIPC tree, in firmware or in builder. Neither is a kernel module that rewrites a + vendor blob's memory at runtime, nor a generated facade library checked in as a binary. + Each of these is bound to one exact build of one blob and fails silently at the next + vendor drop. + +6. Claims without evidence. A behaviour change needs a stated symptom and before/after + output from real hardware. A test plan whose boxes are unchecked is weaker than no test + plan, because it asserts verification that did not happen. A PR that says it was not + tested on hardware is not reviewable. + +7. Dead code and scope creep. A new source file that no Config.in selects and no .mk + builds is not compiled by CI, so nothing proves it even builds. And a diff must do what + its title says: a PR adding one sensor has no business repointing a SITE that every + board of that vendor consumes. + +Do not report: speculation about whether a change was written with AI assistance - judge +the diff on blast radius, provenance, evidence, and repo ownership, never on writing +style, comment density, formatting, or phrasing; shell style preferences that busybox ash +accepts; missing unit tests for vendor driver code that cannot run off-target; or the +naming of vendor blobs that predate the diff. +""" + +compliance_user_guidelines = """ +Apply pr_compliance_checklist.yaml literally and only to lines the diff adds or changes. +Do not raise a finding for pre-existing code that the diff merely moves or reindents. + +Note especially: a device-specific value added to general/overlay/ or to a shared +load_ script fails the blast-radius gate even when it is correct for the board the +contributor tested, because it changes the default for cameras already in the field. And +LD_PRELOAD has no sanctioned use in this tree - a majestic bug that appears to need one is +a majestic issue, not a firmware change. +""" diff --git a/best_practices.md b/best_practices.md new file mode 100644 index 0000000000..d1496a5916 --- /dev/null +++ b/best_practices.md @@ -0,0 +1,352 @@ +# OpenIPC firmware review standards + +Rules for reviewing changes to OpenIPC/firmware — a Buildroot tree that builds images +for roughly 90 boards across 13 SoC vendors. `general/overlay/` and `general/package/` +are shared, so one file there reaches every camera of a family. Those cameras sit in +places nobody can physically reach, there is no staged rollout, and a bad change is +found only after `sysupgrade` has already written it to flash. + +Each rule below was written from a pull request that was actually closed. The +referenced PR is the one that motivated it. + +--- + +## 1. Blast radius: generic configuration is not your bench + +The tree has no per-contributor scope. A file under `general/overlay/` is copied into +every image; a `load_` script serves an entire SoC family. Weight a finding by +how many cameras the changed file reaches, not by how many lines the diff touches. + +### 1.1 Never make your device-under-test the default in `general/overlay/` + +`general/overlay/etc/init.d/S70vendor` runs on every OpenIPC camera. It deliberately +carries no board knowledge: it asks `ipcinfo -v` which vendor this is and hands off. +Pinning a sensor there overrides the autodetected sensor on every camera of every +vendor — including cameras already deployed, which pick the change up at upgrade time +and lose video. + +```sh +# ❌ every camera now claims to have the contributor's sensor — #2241 +load_"$vendor" -i -s imx662 + +# ✅ +load_"$vendor" -i +``` + +Flag any diff under `general/overlay/` that introduces a sensor name, I2C address, +GPIO number, resolution, MAC prefix, or IP literal. Ask which boards the value is +wrong for; if the answer is "all but mine", it does not belong there. + +### 1.2 A shared load script keeps its family default + +`general/package/-osdrv-*/files/script/load_` sets the fallback sensor +for a whole SoC family. Changing that default to the sensor on the contributor's desk +silently repoints every other board in the family. + +```sh +# ❌ hi3516cv6xx family default retargeted at one bench board — #2241 +SNS_TYPE0=imx662; + +# ✅ leave the family default; pass -s imx662 from the board's own configuration +SNS_TYPE0=sc4336p; +``` + +The same PR did contain the correct way to add a sensor — a new entry in the sensor +list the package already builds: + +```make +# ✅ additive, changes no existing board's behaviour +HISILICON_OPENSDK_SENSORS_hi3516cv6xx = \ + ... + smart_sc500ai/libsns_sc500ai \ + sony_imx662/libsns_imx662 +``` + +Editing a load script is not itself a problem — adding a sensor to one is normal and +expected. `#2248` added GC2023-over-MIPI to `hi3516cv200` by extending an existing case +arm, and changed no board's default: + +```sh +# ✅ additive — merged as #2248 +- ov2710_mipi | ov2735_i2c_1080p | ov2735) ++ ov2710_mipi | ov2735_i2c_1080p | ov2735 | gc2023_mipi) +``` + +Flag only a change to an existing `SNS_TYPE*`, default resolution, or default profile in +a shared load script. Extending a case arm, adding a new `.ini`, or adding a sensor to a +package's sensor list changes no existing board and needs no such scrutiny. + +### 1.3 A toolchain flag lands on every package in the image + +`BR2_TARGET_OPTIMIZATION` is appended to `TOOLCHAIN_WRAPPER_OPTS` +(`buildroot-2023.02.1/toolchain/toolchain-wrapper.mk`), so it is injected into every +compilation unit the board builds — kernel modules, vendor SDK glue, busybox, majestic, +all of it. That is a whole-image ABI and codegen change, not a defconfig tweak, and it +is invisible in a diff that shows one added line per board. + +``` +# ❌ no symptom named, no size or stability measurement — #2260 +BR2_TARGET_OPTIMIZATION="-mno-unaligned-access" +``` + +Note this string is separate from the `-O` level, which comes from `BR2_OPTIMIZE_*` +(`buildroot-2023.02.1/package/Makefile.in`); do not claim one clobbers the other. + +Flag any added or changed `BR2_TARGET_OPTIMIZATION`, `BR2_TARGET_LDFLAGS`, or +`BR2_GLOBAL_PATCH_DIR` without a named symptom and a before/after image-size and +boot check on the affected board. If the flag fixes one package, fix that package. + +--- + +## 2. Provenance of sources and binaries + +Every board that selects a package inherits wherever that package points. Provenance +is therefore a whole-fleet property, not a detail of one contributor's workflow. + +### 2.1 A package `SITE` points at OpenIPC, not a personal fork + +A contributor's fork can be force-pushed, made private, or deleted, and every board +that builds the package breaks at the next clean build. It also puts unreviewed code +into images that carry the project's name. + +```make +# ❌ every HiSilicon board now builds from one contributor's account — #2241 +HISILICON_OPENSDK_SITE = $(call github,diegok3,openhisilicon,$(HISILICON_OPENSDK_VERSION)) + +# ✅ +HISILICON_OPENSDK_SITE = $(call github,openipc,openhisilicon,$(HISILICON_OPENSDK_VERSION)) +``` + +Flag any `*_SITE` that names a user account rather than an OpenIPC-org repository or a +documented upstream project. The fix is to land the change in the upstream repo first +and then bump the version here. + +### 2.2 A version bump must not make the pin less specific + +The tree is not uniform here: most packages pin a full 40-character SHA +(`AWS_PRODUCER_VERSION`), `hisilicon-opensdk` has always used an 8-character short SHA, +and a number of packages still track `HEAD`. Full SHAs are preferred for anything new, +but a bump that keeps a package's established format is not a defect — `#2267` bumped +`ff20187b` to `2d637e35` and is the current convention for that package. + +What is a defect is a bump that loosens the pin — fewer characters than the value it +replaces, or a branch name in place of a commit: + +```make +# ❌ 8 hex digits replaced by 7, in the same PR that repointed SITE — #2241 +HISILICON_OPENSDK_VERSION = 6c46247 + +# ✅ same format as the value it replaces +HISILICON_OPENSDK_VERSION = 2d637e35 +``` + +Flag a `*_VERSION` that is shorter than the value it replaces, or that changes a commit +pin to a branch. Prefer a full 40-character SHA for a newly added package. + +### 2.3 `Config.in` help text names the URL the `.mk` actually fetches + +`#2241` repointed `HISILICON_OPENSDK_SITE` while the help text still told the reader +the code came from `OpenIPC/openhisilicon`. A reviewer or a downstream user checking +provenance reads the help text and gets the wrong answer. + +Flag any `*_SITE` change whose corresponding `Config.in` help text, README, or comment +still names the previous source. + +### 2.4 Binaries lifted from factory firmware are not a supply chain + +`#2234` checked in 43 `.ko` modules pulled out of a camera's stock image, with a +`PROVENANCE.md` stating plainly that no source exists and no vendor SDK was obtained. +There is then no way to rebuild them for the next kernel, no way to fix a bug in them, +and no way to know they work on any board other than the one they came off. As the +maintainer put it when closing it: there is no way to build a functional streamer out +of extracted blobs and support it in the long run. + +Flag any added `.ko`, `.so`, `.bin`, or firmware image that cannot be traced to a +vendor SDK release or a buildable source tree. A `PROVENANCE.md` documents the problem; +it does not solve it. + +--- + +## 3. Repo boundaries + +Most rejected work is not wrong — it is in the wrong repository. Redirecting a +contributor is a normal and useful review outcome, so name the target repo and why. + +### 3.1 Kernel code and kernel patches belong to OpenIPC/linux + +`general/package/all-patches/linux/` exists for patches that have already been through +kernel review. A new device-tree or driver patch landed here is invisible to everyone +working on the kernel and is silently dropped at the next kernel bump. + +`#2234` added `0902-arm-dts-xm72050200-describe-the-watchdog-as-the-sp805-it-is.patch` +this way. + +Flag any new file under `general/package/all-patches/linux/`. Redirect to +**OpenIPC/linux**; a patch that lands there needs no patch here. + +### 3.2 Support for one retail camera model belongs to OpenIPC/builder + +`general/overlay/usr/sbin/sysupgrade` already encodes this split: variants +`lite|ultimate|neo` upgrade from `OpenIPC/firmware`, everything else from +`OpenIPC/builder`. Board-specific work in this tree is dead weight for the other 89 +boards and is not covered by this repo's CI matrix. + +`#2234` added `general/package/goke-osdrv-gk7205v500/files/script/ircut_gk7201v200`, +a single-board IR-cut driver. + +Four signals that a PR is mis-targeted: + +1. the author says the package must not be enabled on generic SoC defconfigs; +2. no defconfig selects it, so it is dead code in this repo's CI; +3. it collides with an existing first-class package at the same installed path; +4. the author maintains a downstream board repo. + +Flag these and redirect to **OpenIPC/builder**, under +`devices/common/br-ext-chip-/`, with a per-board defconfig that selects the +package. + +### 3.3 Probing and bring-up tools belong to OpenIPC/ipctool + +`#2241` added `tools/imx662/vi_raw_capture.c`, `recv_raw.py`, `i2c_test.c`, and +`i2c_recovery.c` — about 1200 lines of sensor bring-up tooling. Nothing in the build +system referenced them, so they shipped in no image and CI never even compiled them. + +Flag new diagnostic, capture, or register-poking utilities. Redirect to +**OpenIPC/ipctool**, which exists for exactly this and already ships to cameras. + +### 3.4 Bugs in the streamer belong to majestic + +Majestic is closed source with its own maintainers. A firmware-side workaround for a +majestic bug leaves the bug in place for every other platform and becomes permanent. + +Flag changes to `general/package/majestic/files/*` that alter how majestic runs in +order to compensate for how majestic behaves. Redirect the contributor to file the +underlying issue with the majestic maintainers. + +--- + +## 4. No monkey-patching + +Each pattern below binds the image to one exact build of one vendor blob. None of them +fails loudly; they fail at the next vendor drop, in the field, on someone else's board. + +### 4.1 `LD_PRELOAD` is never the fix + +Preload shims are not acceptable anywhere in the OpenIPC tree — not in +OpenIPC/firmware and not in OpenIPC/builder. A shim intercepts a symbol for every +caller, is invisible to anyone debugging the process, and freezes the underlying bug +in place because nobody upstream ever sees it. + +```sh +# ❌ — #2234, in general/package/majestic/files/S95majestic +if [ -e /dev/vpss_ctx_fix ] && [ -f /usr/lib/libgk_shim.so ]; then + export LD_PRELOAD=/usr/lib/libgk_shim.so +fi +``` + +Flag any `LD_PRELOAD` in a shipped script, package file, or overlay. If the bug is in +majestic, it goes to majestic's maintainers; if it is in a vendor SDK, it goes to the +vendor SDK. + +### 4.2 No runtime patching of vendor blob memory + +`#2234` added a 948-line kernel module (`general/package/gk7201-fixup/src/gk7201_fixup.c`) +that reached into loaded vendor modules and rewrote object tables in their `.bss`. Those +offsets are valid for one build of one blob. The next vendor release moves them, and the +result is not a clean failure — it is memory corruption on a camera in a ceiling. + +Flag any code that writes to another module's data section, patches an address derived +from `kallsyms`, or hooks a vendor symbol at runtime. + +### 4.3 No generated facade libraries checked in as binaries + +`#2234` shipped `gen_xmedia_shim.sh`, which emitted six `libhi_*.so` forwarding stubs +that were then committed as blobs. Checking in the generator does not make the binary +reviewable — nobody rebuilds it, and the committed `.so` is what actually ships. + +Flag any committed binary produced by a script in the same PR. If it is generated, the +build system generates it; if the build system cannot, the change needs the real SDK. + +--- + +## 5. Evidence + +This project cannot test in CI what it most needs to know. The board matrix proves an +image builds; only the contributor's hardware can prove it boots and streams. That +makes the evidence in the PR description part of the change under review. + +### 5.1 State the symptom before the fix + +`#2260` changed a toolchain flag on two boards with no description of what was broken. +Without a symptom there is nothing to reproduce, nothing to regression-test, and no way +to tell whether the change helped or merely moved the failure. + +Flag a behaviour-changing PR whose description names no symptom, no affected board, and +no way to observe the problem. "Improves stability" is not a symptom. + +### 5.2 A test plan that was never run is worse than none + +`#2260` shipped this, unchecked, and was closed for silence: + +``` +- [ ] Rebuild userspace; toolchain wrapper passes -mno-unaligned-access +- [ ] SSH + WPA2 stable on GK7102 (validated with rebuilt SDK toolchain) +``` + +An unchecked box asserts that verification was planned and implies it happened. It is +strictly worse than an honest "not tested", because it costs a reviewer the time to +discover otherwise. + +Flag any PR description containing unchecked checkboxes under a test or verification +heading. Ask for the output, not the checkmark. + +### 5.3 "Not tested on hardware" is not reviewable + +A PR that states it was never run on a camera cannot be merged, and no amount of code +reading substitutes. This applies equally to hedges — "should work on", "untested but", +"in theory this also fixes". + +Flag and close. The contributor is welcome to reopen with output from a real board. + +--- + +## 6. Dead code and scope + +### 6.1 New sources must be reachable from a defconfig + +A package needs a `Config.in` entry, a `.mk` that builds it, and at least one +defconfig that selects it. Without all three, CI never compiles the code, so nothing +proves it builds — let alone runs. `#2241`'s `tools/imx662/` had none of them. + +Flag added `.c`/`.py`/script files that no `Config.in` selects and no `.mk` installs. +Either wire them up, or they belong in another repo (§3). + +### 6.2 The diff must do what the title says + +`#2241` was titled "Add Sony IMX662 sensor to build system". It also repointed +`HISILICON_OPENSDK_SITE` to a personal fork — a change affecting every HiSilicon board, +which no reviewer reading the title would look for. + +Flag files in the diff that the stated purpose does not explain, especially shared +`.mk`, defconfig, and overlay files. Ask for them to be split into their own PR. + +--- + +## 7. Things that must not reach `master` + +These are hard gates rather than judgement calls; `pr_compliance_checklist.yaml` +enforces them. Summarised here because they are the most common review findings: + +- `LD_PRELOAD` in any shipped script, package file, or overlay. +- Binaries extracted from a camera's factory firmware, or any `.ko`/`.so`/`.bin` with + no vendor SDK or buildable source behind it. +- New kernel patches under `general/package/all-patches/linux/` — those go to + OpenIPC/linux. +- A `*_SITE` pointing at a personal fork, or a `*_VERSION` that is an abbreviated SHA. +- A sensor, GPIO, I2C address, or other board-specific value written into + `general/overlay/` or into a shared `load_` default. +- Single-board scripts and packages in the shared tree — those go to OpenIPC/builder. +- New sources that no `Config.in` selects and no defconfig builds. +- `insmod` where the tree uses `modprobe`, or an OpenSDK module not named `open_*`. +- A test plan whose boxes are unchecked, or a description stating the change was not + tested on hardware. diff --git a/pr_compliance_checklist.yaml b/pr_compliance_checklist.yaml new file mode 100644 index 0000000000..6f3914c0fd --- /dev/null +++ b/pr_compliance_checklist.yaml @@ -0,0 +1,192 @@ +# Hard gates for OpenIPC firmware pull requests. +# +# These are binary pass/fail checks, deliberately narrow and objective. Nuanced, +# judgement-based review standards live in best_practices.md instead. +# +# Schema: https://docs.qodo.ai/v1/features/custom-compliance + +pr_compliances: + - title: "No LD_PRELOAD in shipped files" + compliance_label: true + objective: > + Preload shims are not acceptable anywhere in the OpenIPC tree. They intercept a + symbol for every caller, are invisible to anyone debugging the process, and freeze + the underlying bug in place because the upstream maintainer never sees it. + success_criteria: > + The diff adds no LD_PRELOAD assignment or export to any script, package file, or + overlay file. A bug in majestic is reported to majestic's maintainers; a bug in a + vendor SDK is reported to the vendor. + failure_criteria: > + The diff adds LD_PRELOAD in general/overlay/, in general/package/*/files/, in an + init script such as S95majestic, or in a .mk that writes one at install time. + + - title: "No binaries without a buildable source" + compliance_label: true + objective: > + A blob taken out of a camera's stock image cannot be rebuilt for the next kernel, + cannot be fixed, and is not known to work on any board other than the one it came + off. Firmware built that way cannot be supported. + success_criteria: > + Any added .ko, .so, .a, .bin, or firmware image traces to a vendor SDK release or + to a source tree the build system compiles. Existing vendor blobs may be moved or + bumped. + failure_criteria: > + The diff adds a binary extracted from factory firmware, a binary generated by a + script in the same PR, or any .ko/.so/.bin whose origin the PR cannot name. A + PROVENANCE-style file documenting the absence of source is itself a failure. + + - title: "Package SITE is an OpenIPC or upstream repository" + compliance_label: true + objective: > + Every board selecting a package inherits wherever that package points. A personal + fork can be force-pushed, made private, or deleted, breaking every board at the + next clean build, and it puts unreviewed code into images carrying the project name. + success_criteria: > + Added or changed *_SITE values name an OpenIPC-org repository or a documented + upstream project. Landing the change upstream first and bumping the version here + is the sanctioned route. + failure_criteria: > + The diff points a *_SITE at a personal user account, for example + $(call github,,,...) where is not openipc and is not the + package's established upstream. + + - title: "Version pins do not loosen, and help text agrees with SITE" + compliance_label: true + objective: > + Help text naming a different source than the .mk fetches gives anyone auditing + provenance the wrong answer. A pin that gets less specific than the value it + replaces silently widens what a clean build may fetch. + success_criteria: > + A bumped *_VERSION keeps at least the specificity of the value it replaces, and any + Config.in help text, README, or comment naming the source matches the *_SITE the + .mk actually fetches. Note the tree's conventions differ per package: most pin a + full 40-character SHA, hisilicon-opensdk pins an 8-character short SHA, and several + packages track HEAD. Keeping a package's established format is not a finding. + failure_criteria: > + The diff sets a *_VERSION to fewer hex characters than the value it replaces, or + replaces a commit pin with a branch name, or changes a *_SITE while leaving help + text or comments naming the previous source. Prefer a full 40-character SHA only + for a newly added package. + + - title: "No device-specific values in generic configuration" + compliance_label: true + objective: > + general/overlay/ is copied into every image and load_ scripts serve a whole + SoC family. A value true only of the contributor's bench becomes the default for + cameras already deployed, which pick it up at upgrade time and lose video. + success_criteria: > + Files under general/overlay/ stay board-agnostic and continue to derive per-board + behaviour from autodetection or configuration. Adding sensor support to a load + script is expected and passes as long as it is additive — extending an existing + case arm, adding a new .ini, or adding an entry to a package's sensor list, none of + which change an existing board's behaviour. + failure_criteria: > + The diff writes a sensor name, I2C address, GPIO number, resolution, MAC prefix, or + IP literal into general/overlay/, or changes an existing SNS_TYPE*, default + resolution, or default profile in a shared *-osdrv-*/files/script/load_. + + - title: "Toolchain-wide flags are justified and measured" + compliance_label: true + objective: > + BR2_TARGET_OPTIMIZATION and BR2_TARGET_LDFLAGS are injected into every compilation + unit a board builds, so they are whole-image codegen and ABI changes presented as a + one-line defconfig edit. + success_criteria: > + Any added or changed BR2_TARGET_OPTIMIZATION, BR2_TARGET_LDFLAGS, or + BR2_GLOBAL_PATCH_DIR names the symptom it fixes and reports a before/after image + size and a boot check on the affected board. A fix scoped to one package is + preferred. + failure_criteria: > + The diff adds or changes one of these values with no named symptom and no + before/after measurement from real hardware. + + - title: "New sources are wired into the build" + compliance_label: true + objective: > + Code that no defconfig selects is never compiled by CI, so nothing proves it + builds, let alone runs. It ships in no image and rots. + success_criteria: > + Added source files belong to a package with a Config.in entry and a .mk that + builds and installs them, and at least one defconfig selects that package. Data + files added under a path an existing install rule already globs — for example a new + .ini under files/sensor/config/ installed by a *.ini wildcard — need no .mk change + and pass. + failure_criteria: > + The diff adds .c, .py, or script files with no Config.in entry, no .mk rule, and no + defconfig selecting them — for example a new top-level tools/ directory. + + - title: "No kernel patches in this repository" + compliance_label: true + objective: > + general/package/all-patches/linux/ carries patches that have already been through + kernel review. A new patch landed here is invisible to everyone working on the + kernel and is dropped at the next kernel bump. + success_criteria: > + The diff adds no new patch under general/package/all-patches/linux/. Kernel changes + are opened against OpenIPC/linux, after which no patch is needed here. + failure_criteria: > + The diff adds a device-tree, driver, or any other kernel patch file under + general/package/all-patches/linux/. + + - title: "No single-board support in the shared tree" + compliance_label: true + objective: > + Support for one retail camera model is dead weight for the other boards and is not + covered by this repository's CI matrix. general/overlay/usr/sbin/sysupgrade already + routes lite|ultimate|neo to firmware and every other variant to builder. + success_criteria: > + Added packages and scripts apply to an SoC family. Work specific to one retail model + is opened against OpenIPC/builder under devices/common/br-ext-chip-/ with a + per-board defconfig. + failure_criteria: > + The diff adds a file whose name or content targets exactly one board, or the author + states the package must not be enabled on generic SoC defconfigs, or it collides + with an existing first-class package at the same installed path. + + - title: "No runtime patching of vendor code" + compliance_label: true + objective: > + Offsets into a vendor blob are valid for one build of that blob. The next vendor + release moves them, and the result is not a clean failure but memory corruption on + a camera nobody can reach. + success_criteria: > + Added code calls documented vendor SDK entry points. Fixes to vendor behaviour are + raised with the vendor or implemented in the open SDK. + failure_criteria: > + The diff adds code that writes into another module's data section, resolves a + vendor symbol through kallsyms in order to patch it, or hooks a vendor function at + runtime. + + - title: "Shipped scripts follow tree conventions" + compliance_label: true + objective: > + Scripts in the image run under busybox ash, not bash, and the tree standardised on + modprobe so module dependencies resolve. .github/workflows/shell-tests.yml already + parse-checks every shipped script against busybox ash, but it is deliberately + syntax-only — a bashism that parses still fails on the device, so review covers it. + success_criteria: > + Shell scripts that ship in the image parse under busybox ash and avoid bashisms, + load kernel modules with modprobe, and name OpenSDK modules open_* rather than with + a vendor-specific prefix. This gate applies only to scripts installed onto a camera + — general/overlay/ and general/package/*/files/. Scripts under .github/ and + contrib/ run on CI runners or a developer's machine under bash and are out of scope. + failure_criteria: > + The diff adds bashisms to a script that ships in the image, calls insmod where the + surrounding tree uses modprobe, or introduces an OpenSDK module under a + vendor-prefixed name such as gk7205v200_* or hi3516ev200_*. Do not raise this for + anything under .github/ or contrib/. + + - title: "Hardware evidence is present and honest" + compliance_label: true + objective: > + CI proves an image builds; only the contributor's hardware can prove it boots and + streams. The evidence in the description is therefore part of the change under + review, and an unchecked box asserts verification that did not happen. + success_criteria: > + A behaviour-changing PR names the symptom and the affected board, and shows + before/after output — logs, dmesg, stream behaviour, or measurements — from a real + camera. Any checkbox under a test or verification heading is checked. + failure_criteria: > + The description leaves checkboxes unchecked under a test or verification heading, + states the change was not tested on hardware, or hedges with "should work", + "untested but", or "in theory" in place of output from a board. From d8f7f9653820bf7ac46a3b9f4213aea2c2927a50 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Sun, 16 Aug 2026 16:02:24 +0300 Subject: [PATCH 2/3] qodo: fix review findings, add merge gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three findings from Qodo's review of this PR: 1. (high) .pr_agent.toml told the agent that BR2_TARGET_OPTIMIZATION "silently drops the -Os -pipe", contradicting best_practices.md in the same PR, which says the -O level comes independently from BR2_OPTIMIZE_* and warns reviewers not to make that claim. The toml half was left over from an earlier draft. Both now describe the same mechanism: the string is appended to TOOLCHAIN_WRAPPER_OPTS in toolchain/toolchain-wrapper.mk and reaches every compilation unit, which is why it needs measurement — not because it drops -Os. 2. (medium) best_practices.md cited buildroot-2023.02.1/... paths. Buildroot is not vendored: .gitignore excludes buildroot-*/ and the Makefile downloads BR_VER (2024.02.10) into output-/buildroot-$(BR_VER)/. References are now version- agnostic and say where to find the files. Mechanism re-verified against 2024.02.10 upstream: toolchain-wrapper.mk:22 and Makefile.in:117-135,173 are unchanged from what the rule describes. 3. (medium) The PR template demanded a full 40-character SHA while best_practices.md and the checklist both allow a package's established format (hisilicon-opensdk pins 8 chars, several track HEAD). Template now matches: a bump must not lose specificity, and a new package should pin 40 characters. Also adds .github/workflows/qodo-gate.yml, mirroring OpenIPC/devourer. Branch protection alone cannot hold a PR for a review that has not been posted yet — Qodo answers a minute or two after open, so a green-CI merge can race past it. The gate stays red until Qodo has reviewed the PR once and every thread it opened is resolved. Not pinned to head, so responding to review does not start a re-review treadmill; handle_push_trigger stays off for the same reason and /review summons one on demand. Making it binding needs the check added to branch protection on master. Co-Authored-By: Claude Opus 4.8 --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/workflows/qodo-gate.yml | 181 +++++++++++++++++++++++++++++++ .pr_agent.toml | 11 +- best_practices.md | 18 +-- 4 files changed, 200 insertions(+), 12 deletions(-) create mode 100644 .github/workflows/qodo-gate.yml diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 4952135ea3..8f0746bc41 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -51,6 +51,6 @@ After: - [ ] No files specific to a single retail camera model (those go to [OpenIPC/builder](https://github.com/OpenIPC/builder)) - [ ] No probing or bring-up tooling (that goes to [OpenIPC/ipctool](https://github.com/OpenIPC/ipctool)) - [ ] Nothing under `general/overlay/` or in a shared `load_` script hardcodes a value specific to my board -- [ ] Package sources come from an OpenIPC repository, pinned to a full 40-character SHA or a tag +- [ ] Package sources come from an OpenIPC repository, and any version bump keeps at least the specificity of the pin it replaces (a new package should pin a full 40-character SHA) - [ ] No `LD_PRELOAD`, and no binaries that cannot be rebuilt from source - [ ] New code is selected by a defconfig, so CI actually builds it diff --git a/.github/workflows/qodo-gate.yml b/.github/workflows/qodo-gate.yml new file mode 100644 index 0000000000..9ba5a2a7b0 --- /dev/null +++ b/.github/workflows/qodo-gate.yml @@ -0,0 +1,181 @@ +# Merge gate for the Qodo code-review agent. +# +# Mirrors OpenIPC/devourer's qodo-gate.yml, which has already been through the +# failure modes documented below. Keep the two in sync when either is fixed. +# +# Why: branch protection's "require conversation resolution" only blocks +# unresolved threads that EXIST at merge time. Qodo posts its review a minute +# or two after the PR opens, so a "merge when CI is green" flow can race past +# it. This required check stays red until (a) Qodo has reviewed the PR at least +# once — the first, whole-diff pass is the valuable one — and (b) every review +# thread Qodo opened is resolved. +# +# Deliberately NOT pinned to the current head: requiring a review of every +# follow-up commit turns each review-response push into a fresh summon, and +# each re-review re-scans the diff and opens a new batch of ever-smaller +# findings — an unbounded fix/re-review treadmill. The first review catches +# the substance; thread resolution keeps each finding accountable (address it +# or dismiss it with rationale, in the thread, before resolving); follow-up +# commits are maintainer judgment, exactly as with a human reviewer who does +# not re-review every fixup. This repo also leaves handle_push_trigger off in +# .pr_agent.toml for the same reason — re-review on demand with /review. +# +# Event-driven: re-evaluates when the PR updates, when a review is submitted, +# and when someone replies in a review thread. GitHub's workflow parser +# rejects the documented `pull_request_review_thread` trigger ("Unexpected +# value" — verified empirically on devourer, zero-job "workflow file issue" +# run), so plain thread resolution does not auto-retrigger — after resolving +# the last thread, leave a reply (retriggers); the check reads the live +# resolution state each run. Escape hatch for a Qodo outage: the +# `skip-qodo-gate` label passes the check (label changes re-trigger it). +# +# A passing run also re-runs this workflow's earlier FAILED runs on the same +# head commit. Each trigger event creates its own workflow run, and branch +# protection's rollup counts every run of a required check on the commit — a +# fresh green run sits beside the stale red ones rather than superseding +# them, so the PR stays BLOCKED until each red run is re-run by hand. Only a +# passing run re-runs others and a re-run that passes finds nothing red left, +# so it converges; if threads are genuinely unresolved the re-runs go red +# again and the gate still holds. +name: qodo-gate +on: + pull_request: + types: [opened, reopened, synchronize, ready_for_review, labeled, unlabeled] + pull_request_review: + types: [submitted] + pull_request_review_comment: + types: [created, deleted] + +permissions: + contents: read + pull-requests: read + actions: write + +jobs: + qodo-gate: + runs-on: ubuntu-latest + steps: + - name: Require a Qodo review with all its threads resolved + env: + GH_TOKEN: ${{ github.token }} + PR: ${{ github.event.pull_request.number }} + REPO_OWNER: ${{ github.repository_owner }} + REPO_NAME: ${{ github.event.repository.name }} + run: | + set -euo pipefail + BOT='qodo-free-for-open-source-projects' + + json=$(gh api graphql \ + -F owner="$REPO_OWNER" -F name="$REPO_NAME" -F pr="$PR" \ + -f query=' + query($owner: String!, $name: String!, $pr: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $pr) { + labels(first: 100) { nodes { name } } + } + } + }') + + if echo "$json" | jq -e --arg l skip-qodo-gate \ + '.data.repository.pullRequest.labels.nodes[] | select(.name == $l)' \ + >/dev/null; then + echo "PASS: skip-qodo-gate label set (Qodo outage escape hatch)" + exit 0 + fi + + # Any review by the bot counts — the first pass reviews the whole + # diff, and its later in-place updates edit the same review object, + # so one review object existing == the PR has been Qodo-reviewed. + # Paginated: a busy PR accumulates well over 100 review objects + # (every inline reply wraps itself in one), and the bot's first + # review is the OLDEST — exactly what a last-100 window loses + # first. --paginate applies --jq per page, so emit ids and count + # lines. REST spells the bot login with a [bot] suffix, unlike + # GraphQL, so match on the prefix. + reviewed=$(gh api "repos/$REPO_OWNER/$REPO_NAME/pulls/$PR/reviews" \ + --paginate --jq ".[] + | select(.user.login | startswith(\"$BOT\")) + | .id" | wc -l) + if [ "$reviewed" -eq 0 ]; then + echo "FAIL: no Qodo review on this PR yet — it reviews new PRs" + echo "automatically within a couple of minutes; comment /review to" + echo "summon one, then re-run this check once it answers. (Outage?" + echo "Apply the skip-qodo-gate label.)" + exit 1 + fi + + # Unresolved Qodo threads, paginated (a long-lived PR can exceed one + # 100-thread page; a truncated read must never produce a false pass). + unresolved=0 + cursor="" + while :; do + args=( -F owner="$REPO_OWNER" -F name="$REPO_NAME" -F pr="$PR" ) + [ -n "$cursor" ] && args+=( -F cursor="$cursor" ) + page=$(gh api graphql "${args[@]}" \ + -f query=' + query($owner: String!, $name: String!, $pr: Int!, $cursor: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $pr) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + isResolved + comments(first: 10) { nodes { author { login } } } + } + } + } + } + }') + # A thread is Qodo's if ANY of its first comments is by the bot — + # first-comment-only attribution loses the thread when the bot's + # opening comment is deleted while replies remain (and deletion + # re-triggers this check, so that would be a false pass). + n=$(echo "$page" | jq --arg b "$BOT" \ + '[.data.repository.pullRequest.reviewThreads.nodes[] + | select(.isResolved | not) + | select([.comments.nodes[].author.login] | index($b))] | length') + unresolved=$((unresolved + n)) + more=$(echo "$page" | jq -r \ + '.data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage') + [ "$more" = "true" ] || break + cursor=$(echo "$page" | jq -r \ + '.data.repository.pullRequest.reviewThreads.pageInfo.endCursor') + done + + if [ "$unresolved" -gt 0 ]; then + echo "FAIL: $unresolved unresolved Qodo review thread(s) — address" + echo "or explicitly dismiss each finding in its thread, then mark" + echo "it resolved. Plain resolution does not auto-retrigger this" + echo "check: leave a reply in a thread (retriggers) — the passing" + echo "run then sweeps this red run off the commit itself." + exit 1 + fi + + echo "PASS: Qodo review present, all its threads resolved" + + # Sweep stale red runs of this gate off the head commit, so the pass + # above is the one the branch-protection rollup sees. Best-effort: a + # failed re-run request must not turn a PASS into a FAIL. + - name: Re-run this gate's earlier failed runs on this commit + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + THIS_RUN: ${{ github.run_id }} + run: | + set -u + # The listing is guarded too, not just the reruns: with an unguarded + # pipeline a transient list failure would be the step's exit code — + # exactly the PASS-into-FAIL this step promises not to produce. + if ! ids=$(gh run list --repo "$REPO" --workflow qodo-gate \ + --commit "$HEAD_SHA" --json databaseId,conclusion \ + --jq '.[] | select(.conclusion == "failure") | .databaseId'); then + echo "sweep skipped: could not list this workflow's runs" + exit 0 + fi + for id in $ids; do + [ "$id" = "$THIS_RUN" ] && continue + echo "re-running failed qodo-gate run $id" + gh run rerun "$id" --repo "$REPO" --failed || true + done + exit 0 diff --git a/.pr_agent.toml b/.pr_agent.toml index 72dbee7874..12aa2c13fa 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -35,10 +35,13 @@ Prioritise, in order: contributor's bench must not become the default for everyone else. This breaks already deployed cameras at upgrade time, which is the worst failure mode this project has. -2. Buildroot defaults replaced instead of extended. BR2_TARGET_OPTIMIZATION and its peers - are whole-value assignments: setting one to a single new flag silently drops the -Os - -pipe that every package was built with. The symptom is a size and performance - regression across the entire image, with nothing in the diff naming it. +2. Toolchain-wide flags added without measurement. BR2_TARGET_OPTIMIZATION is appended to + TOOLCHAIN_WRAPPER_OPTS in Buildroot's toolchain/toolchain-wrapper.mk, so it is baked + into the compiler wrapper and reaches every compilation unit the board builds. That is + a whole-image codegen and ABI change arriving as one added line per defconfig, and it + needs a named symptom plus a before/after size and boot check. Do not claim it drops + the -O level: that comes independently from BR2_OPTIMIZE_* in package/Makefile.in, and + asserting otherwise is a false finding this project has already seen made. 3. Provenance. A package SITE must point at an OpenIPC-org repository or a documented upstream, never a contributor's personal fork - a fork is a single point of failure for diff --git a/best_practices.md b/best_practices.md index d1496a5916..45498ac600 100644 --- a/best_practices.md +++ b/best_practices.md @@ -78,19 +78,23 @@ package's sensor list changes no existing board and needs no such scrutiny. ### 1.3 A toolchain flag lands on every package in the image -`BR2_TARGET_OPTIMIZATION` is appended to `TOOLCHAIN_WRAPPER_OPTS` -(`buildroot-2023.02.1/toolchain/toolchain-wrapper.mk`), so it is injected into every -compilation unit the board builds — kernel modules, vendor SDK glue, busybox, majestic, -all of it. That is a whole-image ABI and codegen change, not a defconfig tweak, and it -is invisible in a diff that shows one added line per board. +`BR2_TARGET_OPTIMIZATION` is appended to `TOOLCHAIN_WRAPPER_OPTS` in Buildroot's +`toolchain/toolchain-wrapper.mk`, so it is baked into the compiler wrapper and reaches +every compilation unit the board builds — kernel modules, vendor SDK glue, busybox, +majestic, all of it. That is a whole-image ABI and codegen change, not a defconfig tweak, +and it is invisible in a diff that shows one added line per board. + +(Buildroot is not vendored here. `make` downloads the version in `BR_VER` at the top of +the `Makefile` into `output-/buildroot-$(BR_VER)/`, so read these files there.) ``` # ❌ no symptom named, no size or stability measurement — #2260 BR2_TARGET_OPTIMIZATION="-mno-unaligned-access" ``` -Note this string is separate from the `-O` level, which comes from `BR2_OPTIMIZE_*` -(`buildroot-2023.02.1/package/Makefile.in`); do not claim one clobbers the other. +Note this string is separate from the `-O` level, which comes from `BR2_OPTIMIZE_*` in +`package/Makefile.in`; do not claim one clobbers the other. That false finding has +already been made on a PR here. Flag any added or changed `BR2_TARGET_OPTIMIZATION`, `BR2_TARGET_LDFLAGS`, or `BR2_GLOBAL_PATCH_DIR` without a named symptom and a before/after image-size and From 90c86842cff989e80d1accca3262b4933e7133c4 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Sun, 16 Aug 2026 16:04:08 +0300 Subject: [PATCH 3/3] qodo: move shell portability out of the hard gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth review finding: the "Shipped scripts follow tree conventions" gate required scripts to "avoid bashisms" without defining the term, which is not a binary check and so does not belong in the compliance file at all — it contradicts that file's own premise that gates are objective. Worse, on this target the rule would flag working code. The shipped busybox is built with CONFIG_ASH_BASH_COMPAT=y (general/package/busybox/busybox.config:1138), so `function name()` and friends are valid on the device, and four scripts that run on real hardware are rejected by dash: both hi3516cv6xx and hi3519dv500 load_hisilicon, msc313e auto_run.sh, infinity6e zoom.sh. This is already documented at length in .github/scripts/test_shell_parse.sh, which deliberately checks against ash and nothing stricter. The gate now keeps only what is mechanical — modprobe over insmod, open_* naming — and explicitly tells reviewers not to raise portability there, since shell-tests.yml already covers syntax. The judgement half moves to best_practices.md §7.1, which names the constructs busybox ash genuinely lacks (arrays, declare/typeset, ${var^^}, ${!var}, herestrings), requires the reviewer to say which one applies, and records why dash is the wrong bar. Co-Authored-By: Claude Opus 4.8 --- best_practices.md | 29 ++++++++++++++++++++++++++++- pr_compliance_checklist.yaml | 28 +++++++++++++++------------- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/best_practices.md b/best_practices.md index 45498ac600..f38e239d3b 100644 --- a/best_practices.md +++ b/best_practices.md @@ -336,7 +336,34 @@ Flag files in the diff that the stated purpose does not explain, especially shar --- -## 7. Things that must not reach `master` +## 7. Shipped shell scripts + +### 7.1 Portability is judged against busybox ash as built here, not against dash + +`.github/workflows/shell-tests.yml` parse-checks every shipped script against busybox +ash, but only for syntax — `sh -n` does not execute, so a construct that parses can still +misbehave. That leaves a real gap for review, and it is tempting to close it by demanding +"no bashisms". Do not. + +The target is not a minimal POSIX shell. The shipped busybox is built with +`CONFIG_ASH_BASH_COMPAT=y` (`general/package/busybox/busybox.config`), so `function +name()` and similar are valid on the device. Four scripts that work on real hardware — +both `hi3516cv6xx` and `hi3519dv500` `load_hisilicon`, `msc313e auto_run.sh`, and +`infinity6e zoom.sh` — are rejected by dash and run fine on cameras. Judging against a +stricter shell than the target turns working code red. + +So raise portability only for constructs busybox ash genuinely lacks — bash arrays +(`arr=(...)`), `declare`/`typeset`, `${var^^}`/`${var,,}` case conversion, `${!var}` +indirect expansion, and herestrings (`<<<`) — and say which one you mean and why it +fails. This is a judgement call, deliberately not a compliance gate. + +Flag a genuinely unsupported construct in a script under `general/overlay/` or +`general/package/*/files/`. Do not flag style, and do not flag anything under `.github/` +or `contrib/`, which run under bash away from the device. + +--- + +## 8. Things that must not reach `master` These are hard gates rather than judgement calls; `pr_compliance_checklist.yaml` enforces them. Summarised here because they are the most common review findings: diff --git a/pr_compliance_checklist.yaml b/pr_compliance_checklist.yaml index 6f3914c0fd..8b15a43d85 100644 --- a/pr_compliance_checklist.yaml +++ b/pr_compliance_checklist.yaml @@ -160,21 +160,23 @@ pr_compliances: - title: "Shipped scripts follow tree conventions" compliance_label: true objective: > - Scripts in the image run under busybox ash, not bash, and the tree standardised on - modprobe so module dependencies resolve. .github/workflows/shell-tests.yml already - parse-checks every shipped script against busybox ash, but it is deliberately - syntax-only — a bashism that parses still fails on the device, so review covers it. + The tree standardised on modprobe so module dependencies resolve, and on open_* + naming for OpenSDK modules that replace vendor-prefixed blobs. Both are mechanical + and checkable from the diff. success_criteria: > - Shell scripts that ship in the image parse under busybox ash and avoid bashisms, - load kernel modules with modprobe, and name OpenSDK modules open_* rather than with - a vendor-specific prefix. This gate applies only to scripts installed onto a camera - — general/overlay/ and general/package/*/files/. Scripts under .github/ and - contrib/ run on CI runners or a developer's machine under bash and are out of scope. + Shell scripts that ship in the image load kernel modules with modprobe, and OpenSDK + modules are named open_* rather than with a vendor-specific prefix. This gate + applies only to files installed onto a camera — general/overlay/ and + general/package/*/files/. Scripts under .github/ and contrib/ run on CI runners or + a developer's machine under bash and are out of scope. failure_criteria: > - The diff adds bashisms to a script that ships in the image, calls insmod where the - surrounding tree uses modprobe, or introduces an OpenSDK module under a - vendor-prefixed name such as gk7205v200_* or hi3516ev200_*. Do not raise this for - anything under .github/ or contrib/. + The diff calls insmod where the surrounding tree uses modprobe, or introduces an + OpenSDK module under a vendor-prefixed name such as gk7205v200_* or hi3516ev200_*. + Do not raise this for anything under .github/ or contrib/. Do not raise shell + portability here: syntax is already enforced by .github/workflows/shell-tests.yml + against busybox ash, and "avoid bashisms" is not binary on this target — the shipped + busybox sets CONFIG_ASH_BASH_COMPAT=y, so constructs a portability linter would + reject are valid in the field. That judgement lives in best_practices.md §7.1. - title: "Hardware evidence is present and honest" compliance_label: true