review: ten rules a board-support PR walked straight through - #2452
Merged
Merged
Conversation
#2446 (Imou Cue 2 board support) is the shape of PR these files exist to catch, and the automated review caught eight of twelve problems in it. The four it missed, and the five it half-caught, are all gaps in what we wrote down rather than gaps in the reviewer. The binaries gate enumerated .ko/.so/.a/.bin, so an executable with no extension at all -- general/overlay/etc/ir/nrxset -- was reported back as "contains no textual changes", which is exactly what a blob looks like to anything reading a diff as text. The gate now keys on the "Binary files ... differ" marker, which is the one signal that survives, and says outright that general/overlay/ is never the place for a compiled artefact. A register table copied out of a vendor's driver gets the same treatment: it is the same provenance failure written in C. Three things the tree already owns had no rule at all. ipctool is not on a camera -- the package installs ipcinfo, and /usr/sbin/ipctool is an extutils arm that curls the tool from a "latest" release into tmpfs at first use -- so calling it from rc.local twelve seconds into boot on a WiFi-only board is four bad properties at once. An overlay file needs no Config.in, so nothing caught /etc/ir/nrxset and nrx_night_06.txt, which nothing in firmware or builder reads and which the same PR's rc.local never invokes, whatever its comment says. And S95majestic carries an -s argument, a start-stop-daemon lookup, a ten-second wait for the sensor HAL and a trap '' HUP that its own comment measured at 25 deaths out of 25 on a hi3516ev200 -- all of which a hand-rolled killall-and-relaunch throws away, on that same SoC. Every patch in this tree targets an upstream we cannot commit to. A patch against hisilicon-opensdk does not: its SITE is openipc/openhisilicon, and the file being patched is checked in there. That repository and OpenIPC/sensors join the redirect table, which listed only linux, builder, ipctool and majestic. The rest is telling contributors what already exists. customizer.sh, muxes.sh, gpio.conf, late-overlays.list and the excludes lists are a complete per-device mechanism, shipped per device in builder at the same paths -- and documented nowhere, which is why somebody writes an S01leds with GPIO 0, 4 and 9 typed into it instead. "Move it to builder" is half an answer; say which seam it becomes. Two smaller notes go with it: the "additive, so it changes nothing" exemption belongs to load_<vendor> case arms and not to the overlay, where etc/wireless/usb has quietly reached 46 retail-model arms; and an excludes list added here is keyed <model>_<variant>, which is the generic board's key too, so #2446's list took libsns_imx307.so away from hi3516ev200_lite itself. Lastly, devmem in a shipped script is now a question rather than a given. It is the worst way to record what is soldered to a pad -- undone by anything that later exports it, lost at the next reflash -- and the camera can be told instead, through the /api/v1/pinmux surface the pins page in OpenIPC/majestic-webui draws.
openipc-ai
requested review from
cronyx,
flyrouter and
viktorxda
as code owners
September 19, 2026 06:43
PR Summary by QodoExpand review gates for board-support changes
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Both come from the bot review on the pull request that added them, and both were right. The patch rule claimed no patch in the tree targets an OpenIPC-org repository. That is false: libevent-openipc carries 0001-CMakeLists-remove-epoll_pwait2-check.patch against github.com/OpenIPC/libevent. My own check missed it because the SITE grep matched LIBEVENT_OPENIPC_SITE_METHOD first and took that line. Counted properly it is 22 of 23, not 23 of 23 -- and the one exception is the legitimate shape the rule should have described from the start: one hunk, a build fix, obviously temporary, riding ahead of a bump. So the gate no longer refuses every such patch. It asks whether the patch is a bridge or a substitute -- is the upstream pull request named, is it the minimal delta, does it go at the next *_VERSION bump -- which still catches #2446, whose 114-entry table replacement is none of those. The reachability rule was worse than the bot said. It asked that an added file be "read at its installed path", but most of this tree's data is never named by anyone: hisilicon-osdrv-hi3516ev200 installs files/sensor/config/*.ini by wildcard and the one that gets used is chosen at runtime from the configured sensor, a module is reached by `modprobe <name>`, an IQ profile through isp.iqProfile. A new sensor ini would have failed the gate while working perfectly. It also contradicted a gate five entries above it in the same file, which already exempts a data file landing under a path an existing install rule globs. Both now say the same thing: trace the mechanism -- path, basename, a name built at runtime, or an install glob a convention selects from -- and raise only the residue, which is what /etc/ir/ actually is. A rule that fires on correct work costs more than the rule is worth, and best_practices.md already carries one warning about a false finding this project has seen made. These two would have been the next.
The rule now asks whether a patch against an OpenIPC-owned package is a bridge to an upstream pull request or a substitute for one, but it was still titled "No patches against OpenIPC's own packages". A title is what gets quoted back in a finding, so on a legitimate libevent-shaped patch this one would have delivered exactly the message the rule was just corrected to stop delivering.
7 tasks done
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
PR #2446 (Imou Cue 2 board support) is exactly the kind of change
best_practices.mdandpr_compliance_checklist.yamlexist to catch: a whole retail camera filed into the sharedtree, with a binary blob, a boot-time download, and a sensor patch against one of our own
repositories. The automated review found eight of the twelve problems in it. The four
it missed and the five it half-caught are gaps in what we wrote down, not in the reviewer —
so this writes them down.
No hardware; this is review configuration only.
What was missing
The binaries gate could not see a binary. It enumerated
.ko/.so/.a/.bin, sogeneral/overlay/etc/ir/nrxset— an executable with no extension — came back from thedescribe pass as "the supplied patch contains no textual changes", which is precisely what
a blob looks like to anything reading a diff as text. The gate now keys on the
Binary files ... differmarker, which is the one signal that always survives, and saysoutright that
general/overlay/is never the place for a compiled artefact. A registertable copied out of a vendor's driver is folded in as the same failure written in C.
ipctoolis not on a camera.general/package/ipctool/ipctool.mkinstallsipcinfoand nothing else;
/usr/sbin/ipctoolis a symlink toextutils, whoseipctool)arm curlsthe tool from the
latestrelease into/tmpon first use. In a boot script that is anetwork fetch during boot, from a floating tag, repeated every boot because
/tmpis tmpfs,and a silent no-op until the network is up. #2446 ran
ipctool i2csetfromrc.localtwelve seconds in, on a WiFi-only board.
An overlay file needs no
Config.in, so nothing catches one nothing reads. Nothing infirmware or builder reads
/etc/ir/nrxsetor/etc/ir/nrx_night_06.txt, and therc.localthe same PR ships never invokes
nrxset— while the comment above that code says it does.Its own two comments also contradict each other about whether the files are needed. Cost
lands on every board, several of which sit within 32 KB of their cap.
S95majesticis not a thin wrapper. It passes-s, finds the daemon withstart-stop-daemon -xrather than a pidfile, waits up to ten seconds for the sensor HAL tobe released, and starts under
trap '' HUP— its own comment records 25 deaths out of 25without that, measured on a hi3516ev200. A hand-rolled
killall majestic; sleep 3; majestic &throws all of it away, which is what #2446 shipped, on that same SoC.
Every patch in this tree targets an upstream we cannot commit to — ffmpeg, mbedTLS,
vtund, libre, siproxd, ZeroTier, libwebsockets, f2fs-tools, the Realtek drivers. A patch
against
hisilicon-opensdkdoes not: itsSITEisopenipc/openhisilicon, and the filebeing patched is checked in there. OpenIPC/openhisilicon and OpenIPC/sensors join the
redirect table, which listed only linux, builder, ipctool and majestic.
And the tree's per-device seams are documented nowhere.
customizer.sh,muxes.sh,gpio.conf,late-overlays.listand the excludes lists are a complete mechanism, shippedper device in builder at the same paths (96, 15, 18 and 9 devices respectively) — and absent
from the wiki, which is why somebody writes an
S01ledswith GPIO 0, 4 and 9 typed into it."Move it to builder" is half an answer; review should say which seam it becomes. Two smaller
notes ride along: the additive, so it changes nothing exemption belongs to
load_<vendor>case arms and not to the overlay, where
etc/wireless/usbhas quietly reached 46retail-model arms in 327 lines; and an excludes list added here is keyed
<model>_<variant>, the generic board's key too, so #2446's list tooklibsns_imx307.soand
default.iniaway fromhi3516ev200_liteitself.devmemin a shipped script is now a question. It is the worst way to record what issoldered to a pad — undone by anything that later exports it, lost at the next reflash —
and the camera can be told instead, through the
/api/v1/pinmuxsurface that the pins pagein OpenIPC/majestic-webui draws.
Changes
best_practices.mdpr_compliance_checklist.yaml.pr_agent.tomlCLAUDE.md#2446is still open.best_practices.md's header said every rule traces to a closed PR;that sentence now admits the occasional open one, rather than the citations quietly being
wrong.
Hardware tested on
Not applicable, and not hedging: this is review configuration, documentation and repository
metadata, which
pr_compliance_checklist.yamland the template both name as exempt. No filehere reaches an image.
Evidence
The checks this change class does admit:
Both edited config files still parse, and
.pr_agent.tomlstill carries no token that tripsQodo's hardened loader (the failure mode from #2268):
Scope
general/package/all-patches/linux/(those go to OpenIPC/linux)general/overlay/or in a sharedload_<vendor>script hardcodes a value specific to my boardLD_PRELOAD, and no binaries that cannot be rebuilt from source