diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 000000000..8f0746bc4 --- /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, 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 000000000..9ba5a2a7b --- /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 new file mode 100644 index 000000000..12aa2c13f --- /dev/null +++ b/.pr_agent.toml @@ -0,0 +1,94 @@ +# 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. 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 + 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 000000000..f38e239d3 --- /dev/null +++ b/best_practices.md @@ -0,0 +1,383 @@ +# 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` 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_*` 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 +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. 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: + +- `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 000000000..8b15a43d8 --- /dev/null +++ b/pr_compliance_checklist.yaml @@ -0,0 +1,194 @@ +# 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: > + 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 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 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 + 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.