review: ten rules a board-support PR walked straight through

.pr_agent.toml

@@ -49,6 +49,18 @@ Prioritise, in order:
    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.
+   "Additive, so it changes no existing board" is the right exemption for a load_<vendor>
+   sensor case arm and is not a licence for the shared overlay: etc/wireless/usb has
+   accumulated 46 arms keyed on retail model names while 18 builder devices override the
+   whole file, and those 46 are grandfathered, not precedent.
+   Question a devmem write in a shipped script. It used to be the only way to tell a
+   camera something is soldered to a pad, and it is the worst way: undone by anything
+   that later exports the pad, and lost at the next reflash. The camera can be told
+   instead - /api/v1/pinmux reports and sets what each pad is wired to, which is what
+   the pins page in OpenIPC/majestic-webui draws, and nightMode drives an IR
+   illuminator from configuration. A devmem line under general/overlay/ has to say why
+   configuration cannot express it and - since rc.local is shared - why every other
+   camera should execute 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
@@ -65,14 +77,44 @@ Prioritise, in order:
    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.
+   Detect a binary by the DIFF MARKER, never the extension. A hunk rendered as
+   "Binary files a/... and b/... differ" is a blob whatever it is named - an executable
+   with no extension at all reads to a text-diff summariser as "contains no textual
+   changes", and that is how one shipped past review into general/overlay/, which is
+   copied verbatim into every image. Report it, and say you cannot see its contents.
+   A register initialisation table copied out of a camera vendor's own driver is the same
+   provenance failure written in C: unverifiable against a datasheet, and replacing a
+   whole table rather than adding the deltas a board needs changes every other board
+   using that sensor.
+   A new *.patch is fine against a third-party upstream (22 of the tree's 23 patched
+   packages - ffmpeg, mbedtls, vtund, libre, baresip, siproxd, zerotier, libwebsockets,
+   f2fs-tools, mini-snmpd, uacme, nabto, mavlink-router, the Realtek drivers). Against a
+   package whose *_SITE resolves to an openipc repository it is a finding ONLY when it
+   substitutes for the upstream fix rather than bridging to it: no pull request named
+   against the owning repo, or a wholesale replacement rather than a minimal delta, or
+   nothing saying it goes at the next *_VERSION bump. libevent-openipc is the tree's one
+   legitimate instance - a single hunk dropping a build-time feature check - so do not
+   report this as an absolute prohibition.
 
 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-<vendor>/ - 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
+   builder. Hardware probing and bring-up tools belong to OpenIPC/ipctool. HiSilicon
+   sensor drivers and open SDK code belong to OpenIPC/openhisilicon, which is what
+   hisilicon-opensdk fetches; Sigmastar sensor drivers belong to OpenIPC/sensors. Bugs in
+   the majestic streamer belong to majestic's maintainers. Redirecting a contributor is a
    normal, useful review outcome; say which repo and why.
+   Say which SEAM too, not only which repo. The tree already has one for every
+   board-specific thing, and a contributor who has not been shown it writes a new init
+   script with the pin numbers typed in: /usr/share/openipc/customizer.sh and
+   /usr/share/openipc/muxes.sh (both run by S30customizer), the declarative pin map
+   /usr/share/openipc/gpio.conf, late-overlays.list, and
+   general/scripts/excludes/<model>_<variant>.list. Builder ships all of them per device
+   at those same paths under devices/<board>/. An excludes list added to THIS repo is not
+   per-board - the key is <model>_<variant>, which the generic board answers to as well.
+   A comment explaining why a workaround is needed has usually already named the
+   repository that owns the fix. Read it as a redirect and quote it back.
 
 5. Monkey-patching in place of a fix. Dynamic-linker injection shims - a library forced
    ahead of the real one through the linker's LD_* environment knob, spelled out in
@@ -109,6 +151,33 @@ Prioritise, in order:
    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.
+   The overlay's mirror image of this needs saying separately, because general/overlay/
+   needs no Config.in and no .mk - anything dropped there ships, so a file nothing on the
+   camera ever opens passes every dead-code check while costing every board of every
+   vendor, several of which sit within 32 KB of their squashfs cap. But reachability is
+   SEMANTIC: a sensor .ini installed by a files/sensor/config/*.ini wildcard and chosen
+   at runtime from the configured sensor name is reachable, as is a module loaded by
+   `modprobe <name>` and an IQ profile named through isp.iqProfile. Trace the mechanism
+   before raising anything - path, basename, a name built at runtime, or an install glob
+   a documented convention selects from all count. The finding is the residue: a file
+   whose name appears nowhere, in a directory no consumer knows about.
+
+8. Commands a shipped script calls must exist on the image. A camera has no package
+   manager and often no network, and a missing command does not fail loudly - sh prints
+   "not found" to a console nobody reads and carries on, so the script reports success
+   having done nothing. Busybox applets and binaries installed by a package the board's
+   defconfig selects are the whole set. ipctool is the trap that looks like an exception:
+   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, whose ipctool arm downloads the tool from the ipctool repository's "latest"
+   release into /tmp on first use. That is a convenience for a person at a shell, and in
+   a boot script it is a network fetch during boot, from a floating tag, repeated every
+   boot because /tmp is tmpfs, and a silent no-op until the network is up.
+   Relatedly, a shipped script bounces the streamer with /etc/init.d/S95majestic restart.
+   A hand-rolled killall-and-relaunch loses the -s argument, the start-stop-daemon -x
+   lookup, the ten-second wait for the old process to release the sensor HAL, and the
+   trap '' HUP that S95majestic carries because the disposition survives exec() where a
+   handler does not.
 
 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

CLAUDE.md

@@ -35,6 +35,8 @@ else. Check this first — it costs a minute and can save an evening.
 | Kernel source and kernel patches | [OpenIPC/linux][linux] |
 | Support for one specific retail camera model | [OpenIPC/builder][builder] |
 | Probing, identification, bring-up tooling | [OpenIPC/ipctool][ipctool] |
+| HiSilicon sensor drivers and open SDK code | [OpenIPC/openhisilicon][openhisilicon] |
+| Sigmastar sensor drivers | [OpenIPC/sensors][sensors] |
 | A bug in the video stream itself | majestic's maintainers, not a shim here |
 | Documentation, device notes, how-tos | [OpenIPC/wiki][wiki], [docs][docs] |
 
@@ -148,15 +150,34 @@ Summarised from `pr_compliance_checklist.yaml`; the reasoning is in `best_practi
   every caller, is invisible to anyone debugging the process, and freezes the real bug in place.
 - **No binaries without buildable source.** Nothing lifted out of a factory image, nothing a
   script in the same pull request generated. A `PROVENANCE.md` documents the problem; it does not
-  solve it.
+  solve it. The test is the diff marker — anything rendered `Binary files ... differ` — not the
+  extension; an executable with no extension is still a blob, and `general/overlay/` is never the
+  place for one. A register table copied out of a camera vendor's driver is the same problem
+  written in C.
+- **A patch against an OpenIPC-owned package is a bridge, not a fix.** 22 of the tree's 23 patched
+  packages target third-party upstreams. If a package's `*_SITE` is an `openipc` repository the fix
+  belongs there; a patch here passes only when the pull request against the owning repository is
+  named, the patch is the minimal delta, and it goes at the next `*_VERSION` bump —
+  `libevent-openipc` is the one instance and the model.
+- **Nothing ships that nothing runs.** An overlay file needs no `Config.in`, so nothing catches a
+  file the camera never opens. Trace the consumer before adding one — reachability is semantic, so
+  a basename, a name built at runtime, or an install glob a convention selects from all count.
+- **Shipped scripts call only what the image contains.** Busybox applets, or binaries a package in
+  the board's defconfig installs. `ipctool` looks installed and is not: the package ships
+  `ipcinfo`, and `/usr/sbin/ipctool` is an `extutils` arm that downloads the tool at first use.
 - **No runtime patching of vendor blob memory**, and no `kallsyms` address hooking.
 - **`*_SITE` points at an OpenIPC-org repository or a documented upstream**, never a personal
   fork, and a `*_VERSION` bump never becomes less specific than the pin it replaces — a full
   40-character SHA for anything new. `Config.in` help text must name the URL the `.mk` fetches.
 - **No board-specific value in shared files.** A sensor name, I2C address, GPIO number,
   resolution, MAC prefix or IP literal does not belong in `general/overlay/` or in a shared
-  `load_<vendor>` default. Extending a case arm, or adding a sensor to a package's list, is
-  additive and fine.
+  `load_<vendor>` default. Extending a `load_<vendor>` case arm, or adding a sensor to a
+  package's list, is additive and fine; that exemption does not extend to the overlay.
+  Per-board values have their own seams — `/usr/share/openipc/customizer.sh` and
+  `/usr/share/openipc/muxes.sh`, both run by `S30customizer`; the declarative pin map
+  `/usr/share/openipc/gpio.conf`; `late-overlays.list`; and
+  `general/scripts/excludes/<model>_<variant>.list`. All of them live per-device in
+  OpenIPC/builder under `devices/<board>/`, at the same paths.
 
 ## Verifying a change
 
@@ -341,4 +362,6 @@ Maintainers append the pull request number at merge time; you do not need to.
 [docs]: https://docs.openipc.org
 [ipctool]: https://github.com/OpenIPC/ipctool
 [linux]: https://github.com/OpenIPC/linux
+[openhisilicon]: https://github.com/OpenIPC/openhisilicon
+[sensors]: https://github.com/OpenIPC/sensors
 [wiki]: https://github.com/OpenIPC/wiki