review: eleven rules a board-support PR walked straight through - #2451
Closed
openipc-ai wants to merge 1 commit into
Closed
openipc-ai wants to merge 1 commit into
openipc-ai wants to merge 1 commit into
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, builder or majestic reads. 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. majestic #751 landed four days before #2446 and its commit message is the rule: somebody who solders a lamp to a board writes devmem into rc.local because nothing in the camera knows the pad exists, and that line is undone by anything that exports the pad and lost on the next reflash.
openipc-ai
requested review from
cronyx,
flyrouter and
viktorxda
as code owners
September 19, 2026 06:40
PR Summary by QodoExpand PR compliance rules for board-support changes
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Collaborator
Author
|
Closing immediately — this PR body and diff quote a private source tree. Reopening with that content removed. |
Code Review by Qodo
1. Valid package sources fail review
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+199
to
+201
| Every added file under general/overlay/ or a package's files/ directory is read at | ||
| its installed path by a script, init file, package, or configuration somewhere in | ||
| firmware, builder or majestic. Documentation and license text shipped on purpose |
There was a problem hiding this comment.
1. Valid package sources fail review 🐞 Bug ≡ Correctness
The new reachability gate requires every added file under a package's files/ directory to be read at an installed path, even when the file is a build-time source that has no installed path. Adding another source like openipc_ethaddr.c, which is copied into the kernel tree and compiled there, therefore cannot satisfy this hard compliance gate.
Agent Prompt
## Issue description
The shipped-file reachability gate treats every file under a package's `files/` directory as a runtime file, although that directory also contains build-time sources which are compiled rather than installed at their source path.
## Fix Focus Areas
- pr_compliance_checklist.yaml[192-208]
## Recommended Fix
Limit this gate to files actually installed into the target root filesystem. Explicitly exempt source, patch, and other build-time inputs that an existing build rule consumes, while leaving runtime overlay and package payload files subject to the installed-path reachability check.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+220
to
+224
| The diff calls, from a shipped script, a command no package installs. ipctool is | ||
| the specific trap: general/package/ipctool/ipctool.mk installs ipcinfo and nothing | ||
| else, so BR2_PACKAGE_IPCTOOL=y puts no "ipctool" on the image. /usr/sbin/ipctool | ||
| is a symlink to extutils, which curls the tool from the ipctool repository's | ||
| "latest" release into /tmp on first use — an interactive convenience, and in a |
There was a problem hiding this comment.
2. Reviewers misdiagnose missing commands 🐞 Bug ≡ Correctness
The new command-availability gate says /usr/sbin/ipctool is a symlink to extutils, but neither the overlay nor any installation rule creates that symlink. On images produced solely by this tree, invoking ipctool fails immediately instead of reaching the documented download arm, so reviews will attribute the failure to network timing and a floating release rather than the absent command.
Agent Prompt
## Issue description
The new guidance claims that `/usr/sbin/ipctool` resolves to `extutils`, but this repository installs only `ipcinfo` and does not create the required symlink.
## Fix Focus Areas
- pr_compliance_checklist.yaml[220-227]
- best_practices.md[613-625]
- CLAUDE.md[162-164]
- .pr_agent.toml[158-164]
## Recommended Fix
State that `ipctool` is absent from images produced by this repository and therefore fails with command-not-found. Mention the dormant `extutils` download arm only conditionally, unless the repository is also changed to create the symlink intentionally.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+179
to
+182
| success_criteria: > | ||
| A new *.patch sits in a package whose *_SITE names a third-party upstream. Every | ||
| patch in the tree today does — ffmpeg, mbedtls, vtund, libre, siproxd, zerotier, | ||
| libwebsockets, f2fs-tools, webrtc-audio-processing and the Realtek drivers. |
There was a problem hiding this comment.
3. Patch reviews rely on a false inventory 🐞 Bug ⚙ Maintainability
The new patch gate states that every existing package patch targets a third-party upstream, but libevent-openipc fetches OpenIPC/libevent and carries a patch against that fetched source. This false premise appears in both reviewer guidance and a hard compliance criterion, so maintainers cannot tell whether the existing package is a grandfathered exception or the new rule was defined from an incomplete inventory.
Agent Prompt
## Issue description
The new guidance claims no existing package patches an OpenIPC repository, despite `libevent-openipc` carrying exactly such a patch.
## Fix Focus Areas
- pr_compliance_checklist.yaml[172-190]
- best_practices.md[386-405]
- CLAUDE.md[157-159]
- .pr_agent.toml[87-91]
## Recommended Fix
Remove the claim that every current patch targets third-party code and define the prohibition prospectively for new patches. Identify `libevent-openipc` as existing debt or migrate its CMake change into the OpenIPC repository and bump the pinned version before retaining the absolute inventory claim.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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./etc/ir/nrxsetand
/etc/ir/nrx_night_06.txtare read by nothing in firmware, builder or majestic, and thePR's own two comments disagree about whether they are used at all. 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. majestic #751 landed four days before#2446 and its commit message is the rule: "Somebody who solders an IR lamp, a button or an
I²C sensor to their board writes devmem into rc.local, because nothing in the camera knows
the pad exists. That line is undone by anything that exports the pad and lost on the next
reflash."
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