ci: watch for drift from firmware instead of finding it by accident - #129
Merged
Conversation
builder.sh copies devices/<item>/* over a fresh firmware clone, so a device directory that ships its own copy of a firmware file replaces it outright. A fleet-wide change in firmware reaches every device that only references a path and none that ships its own copy, silently, because nothing in either repository is looking. One comment on OpenIPC/firmware#2308 turned up three instances of that in an afternoon: 13 board configs that missed the CONFIG_VT sweep (#126), 97 defconfigs that kept BR2_PACKAGE_JSONFILTER after firmware retired it and put three devices over their rootfs cap (#128), and excludes lists naming files that no longer exist (firmware#2313 reports those at build time). The trigger is the design. Every one of those was caused by a commit in firmware while this repository sat untouched, so a pull_request check would never have fired for any of them -- nobody opens a builder PR when firmware changes. firmware-drift.yml runs on a schedule and files an issue rather than failing: drift someone else introduced is not a reason to redden a nightly that is otherwise fine. Only the self-test gates a merge here, out of lint.yml. Two checks, because the two kinds of drift are not the same shape. Shadowed files: comparing content is useless, a builder copy is *supposed* to differ. What matters is whether firmware's version moved since a human last reconciled them, so each of the 13 entries pins the blob it was reconciled against. Re-pinning is the act of looking again. The mapping is hand-authored -- nothing can infer that gk7205v200.generic-fpv derives from gk7205v200.generic. Defconfig symbols: nothing here copies a firmware defconfig, so pinning does not apply. Every BR2_PACKAGE_*=y is resolved against buildroot, firmware and this tree and sorted into "resolves nowhere" (a dead line kconfig ignores) and "resolves, but no firmware defconfig selects it" (needs an allowlist entry). An allowlist entry can also fence a symbol to the devices that need it; without that, writing JSONFILTER down as builder-only for devices/apfpv would equally bless it creeping back onto 95 unrelated defconfigs, which is #128 verbatim. Replayed against the three regressions, each reproduced by hand on a clean tree: firmware moves a shadowed config -> br-ext-chip-goke/board/gk7205v200/gk7205v200.generic.config moved in firmware since this copy was reconciled (2026-08-25). builder copy: devices/common/.../gk7205v200.generic-fpv.config pinned 7450c6803f6d, firmware now f97dce84c0f8 firmware commits since: 85233f0e kernel: drop CONFIG_VT and the busybox applets ... (#2308) jsonfilter creeps back onto a non-apfpv device -> BR2_PACKAGE_JSONFILTER is allowlisted only for apfpv/*/configs/*_defconfig, but 1 other defconfig(s) select it. firmware retires a symbol builder still selects -> BR2_PACKAGE_LIBUBOX is selected here but by no firmware defconfig. Resolving against buildroot is not optional and getting it wrong is how a checker lies. Buildroot is vendored by neither repository, so a first pass over OpenIPC packages alone called HOSTAPD, IW, PHP, UHTTPD, LIBZIP and BWM_NG dead; all six are upstream. Matching only "Config.in" then called PHP_EXT_ZIP dead, because buildroot declares php's extensions in package/php/Config.ext. Without --buildroot that half is skipped and says so rather than inventing findings. Two symbols really do resolve nowhere today -- BR2_PACKAGE_APFPV_GREG on the four greg-generic devices and BR2_PACKAGE_WIFIBROADCAST_EXT in devices/common. Both are recorded as known-dead and reported as a notice on every run rather than removed: whether those devices still want the packages is a maintainer's call, not a side effect of adding a check. The three new paths are classified in ci-matrix.py so this PR and every future re-pin cost no device builds. Without that a blob update is a 107-device run.
openipc-ai
requested review from
cronyx,
flyrouter and
viktorxda
as code owners
August 25, 2026 19:46
PR Summary by QodoDetect and report drift from OpenIPC firmware
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
7 tasks
Qodo caught the shadow list omitting the device-local t21 and t31 kernel configs. It was right, and the hole was much bigger than the five it named: a hand-written list of 13 board configs against 93 same-path shadows covering 35 firmware paths. The missing ones are not incidental -- load_goke, load_hisilicon, load_sigmastar and four vendor .mk files are all shadowed by some device, and those are exactly the shared files a fleet-wide fix lands in. firmware regression-tests load_hisilicon's os_mem_size derivation; two devices here ship their own copy of that script and would not have seen the fix. Listing them by hand was the wrong shape. builder.sh copies devices/<item>/* over the firmware clone, so any file at a path firmware also has replaces it -- which makes same-path shadows discoverable rather than something to remember. discover_shadows() walks devices/ and reports anything sitting on a firmware path with no entry in firmware-drift.json, so the list cannot silently rot and a device added tomorrow cannot quietly introduce an unreconciled shadow. The manifest goes from 13 entries to 101: the 88 discovered ones, plus the 13 hand-authored entries that discovery cannot find because the name differs from the file they replace -- gk7205v200.generic-fpv shadows gk7205v200.generic, and nothing can infer that. Two self-test cases for the new path: a same-path shadow with no entry is reported, and an entry silences it. Verified against the tree by dropping the load_hisilicon entries, which surfaces both devices immediately.
openipc-ai
force-pushed
the
ci/detect-drift-from-firmware
branch
from
August 25, 2026 21:36
cf63374 to
6b5b677
Compare
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.
Second of two PRs adding drift detection between this repository and OpenIPC/firmware. The other is OpenIPC/firmware#2313, which handles the excludes lists at build time.
Why
builder.shcopiesdevices/<item>/*over a fresh firmware clone, so a device directory that ships its own copy of a firmware file replaces it outright. A fleet-wide change in firmware reaches every device that only references a path and none that ships its own copy — silently, because nothing in either repository is looking.One comment on firmware#2308 turned up three instances of that in an afternoon:
CONFIG_VTsweepBR2_PACKAGE_JSONFILTERafter firmware retired itThe trigger is the design
Every one of those was caused by a commit in firmware while builder sat untouched. A
pull_requestcheck here would never have fired for any of them — nobody opens a builder PR when firmware changes.So
firmware-drift.ymlruns on a schedule (05:00, an hour after firmware's nightly so they don't race for a picture of the same HEAD) and files an issue rather than failing: drift someone else introduced is not a reason to redden a nightly that is otherwise fine. It reopens and re-bodies one issue rather than duplicating, so a week of unattended drift is one thread, not seven. Only the--self-testgates a merge here, out oflint.yml.Two checks, because the two kinds of drift aren't the same shape
Shadowed files. Comparing content is useless — a builder copy is supposed to differ, that's why it exists. What matters is whether firmware's version moved since a human last reconciled them, so each entry pins the blob SHA it was reconciled against. Re-pinning is the act of looking again.
Shadows are discovered, not listed.
builder.shcopiesdevices/<item>/*over the firmware clone, so any file at a path firmware also has replaces it — which makes same-path shadows findable rather than something to remember.discover_shadows()walksdevices/and reports anything sitting on a firmware path with no entry in the manifest, so the list cannot silently rot and a device added tomorrow cannot quietly introduce an unreconciled shadow.The manifest is now 101 entries: 88 discovered, plus 13 hand-authored ones discovery cannot find because the name differs from the file they replace (
gk7205v200.generic-fpvshadowsgk7205v200.generic, and nothing can infer that).Defconfig symbols. Nothing here copies a firmware defconfig, so pinning doesn't apply. Every
BR2_PACKAGE_*=yis resolved against buildroot, firmware and this tree, then sorted into resolves nowhere (a dead line kconfig ignores) and resolves, but no firmware defconfig selects it (needs an allowlist entry). An allowlist entry can also fence a symbol to the devices that need it — without that, writingJSONFILTERdown as builder-only fordevices/apfpvwould equally bless it creeping back onto 95 unrelated defconfigs, which is #128 verbatim.Replayed against all three regressions
Each reproduced by hand on a clean tree:
Resolving against buildroot is not optional
Getting this wrong is how a checker lies, and mine did twice before it was right:
HOSTAPD,IW,PHP,UHTTPD,LIBZIPandBWM_NGdead. All six are upstream Buildroot packages — buildroot is vendored by neither repository.Config.inthen calledPHP_EXT_ZIPdead, because buildroot declares php's extensions inpackage/php/Config.ext.Both are false findings the checker invented rather than found. Without
--buildrootthat half is now skipped and says so:Two symbols really are dead
BR2_PACKAGE_APFPV_GREG(the four*_apfpv_greg-generic-*devices) andBR2_PACKAGE_WIFIBROADCAST_EXT(devices/common, sitting next toWIFIBROADCAST_NGwhich does exist — probably a rename that left it behind) resolve to no Kconfig in buildroot, firmware or here. kconfig ignores those lines.They are recorded as
known_dead_symbolsand reported as a notice on every run rather than removed. Whether those devices still want the packages is a maintainer's call, not a side effect of adding a check — and a notice keeps the question visible instead of blessing it.Cost
The three new paths are classified in
ci-matrix.py, so every future re-pin costs zero device builds:Without that, updating a pinned blob would be a 107-device run — and re-pinning is meant to be routine.
This PR itself still builds all 107, and should: it edits
ci-matrix.py, which is deliberately absent fromNO_BUILD_SCRIPTSbecause it is what decides the matrix and cannot be trusted to decide a smaller one for itself. Same reason #127 built everything.Test plan
python3 .github/scripts/check-firmware-drift.py --self-test— ok (13 shadowed files, 38 builder-only symbols, 2 known-dead)master+ buildroot 2024.02.10 — no drift, 2 notices--buildrootand skips the half it cannot answerpython3 .github/scripts/ci-matrix.py --self-test— ok (107 devices, 15 smoke, 39 cases; was 36)python3 .github/scripts/lint-workflow-shell.py— 31 run blocks parse cleanworkflow_dispatchis on it for a first manual run.