qodo: review standards, compliance gates, and a PR template - #2268
Conversation
Mirrors the three-file Qodo Merge setup already in use on majestic: .pr_agent.toml points the review agent at best_practices.md (judgement calls) and pr_compliance_checklist.yaml (binary gates). Rules were derived from PRs that were actually closed: #2241 device-under-test sensor hardcoded into general/overlay/ S70vendor and a shared load_hisilicon default; 1200 lines of unwired tools/imx662/; HISILICON_OPENSDK_SITE repointed at a personal fork while Config.in still named OpenIPC/openhisilicon #2260 toolchain flag with an unrun test plan and no hardware evidence #2234 vendor .ko files extracted from factory firmware, runtime patching of blob .bss, generated facade .so blobs, LD_PRELOAD in S95majestic, a kernel patch, and a single-board script They judge blast radius, provenance, evidence, and repo ownership. They deliberately do not judge writing style: .pr_agent.toml instructs the agent not to speculate about AI authorship or flag comment density and formatting, which is unfalsifiable and false-positives on thorough contributors. Checked against merged #2267, #2248, and #2266 so ordinary work does not trip a gate. Two carve-outs came out of that: adding a sensor to a load script is additive and passes (#2248 extends a case arm and changes no default), and the shell-convention gate is scoped to scripts that ship in the image, not .github/ or contrib/ (#2266). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Summary by QodoAdd Qodo review standards, compliance gates, and PR template
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
Addresses the three findings from Qodo's review of this PR: 1. (high) .pr_agent.toml told the agent that BR2_TARGET_OPTIMIZATION "silently drops the -Os -pipe", contradicting best_practices.md in the same PR, which says the -O level comes independently from BR2_OPTIMIZE_* and warns reviewers not to make that claim. The toml half was left over from an earlier draft. Both now describe the same mechanism: the string is appended to TOOLCHAIN_WRAPPER_OPTS in toolchain/toolchain-wrapper.mk and reaches every compilation unit, which is why it needs measurement — not because it drops -Os. 2. (medium) best_practices.md cited buildroot-2023.02.1/... paths. Buildroot is not vendored: .gitignore excludes buildroot-*/ and the Makefile downloads BR_VER (2024.02.10) into output-<board>/buildroot-$(BR_VER)/. References are now version- agnostic and say where to find the files. Mechanism re-verified against 2024.02.10 upstream: toolchain-wrapper.mk:22 and Makefile.in:117-135,173 are unchanged from what the rule describes. 3. (medium) The PR template demanded a full 40-character SHA while best_practices.md and the checklist both allow a package's established format (hisilicon-opensdk pins 8 chars, several track HEAD). Template now matches: a bump must not lose specificity, and a new package should pin 40 characters. Also adds .github/workflows/qodo-gate.yml, mirroring OpenIPC/devourer. Branch protection alone cannot hold a PR for a review that has not been posted yet — Qodo answers a minute or two after open, so a green-CI merge can race past it. The gate stays red until Qodo has reviewed the PR once and every thread it opened is resolved. Not pinned to head, so responding to review does not start a re-review treadmill; handle_push_trigger stays off for the same reason and /review summons one on demand. Making it binding needs the check added to branch protection on master. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fourth review finding: the "Shipped scripts follow tree conventions" gate
required scripts to "avoid bashisms" without defining the term, which is
not a binary check and so does not belong in the compliance file at all —
it contradicts that file's own premise that gates are objective.
Worse, on this target the rule would flag working code. The shipped
busybox is built with CONFIG_ASH_BASH_COMPAT=y
(general/package/busybox/busybox.config:1138), so `function name()` and
friends are valid on the device, and four scripts that run on real
hardware are rejected by dash: both hi3516cv6xx and hi3519dv500
load_hisilicon, msc313e auto_run.sh, infinity6e zoom.sh. This is already
documented at length in .github/scripts/test_shell_parse.sh, which
deliberately checks against ash and nothing stricter.
The gate now keeps only what is mechanical — modprobe over insmod, open_*
naming — and explicitly tells reviewers not to raise portability there,
since shell-tests.yml already covers syntax.
The judgement half moves to best_practices.md §7.1, which names the
constructs busybox ash genuinely lacks (arrays, declare/typeset,
${var^^}, ${!var}, herestrings), requires the reviewer to say which one
applies, and records why dash is the wrong bar.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
❌ PR-Agent failed to apply 'local' repo settings The configuration file needs to be a valid TOML, please fix it. Error message: Configuration content: |
The gate looked only for a review OBJECT by the bot. Qodo creates one when it has inline comments to hang on the diff, so every PR it finds something in has one — which is why this passed on #2268 and on every devourer PR checked. A clean PR gets no review object at all. On #2269 Qodo reported "Bugs (0), Rule violations (0), Requirement gaps (0)" as a plain issue comment and created nothing else, so the gate failed with "no Qodo review on this PR yet" and could never go green — the check blocked hardest precisely when there was nothing to fix. Accept either signal: a review object, or an issue comment headed "Code Review by Qodo". The header is matched specifically rather than "any comment by the bot", because Qodo also posts "Qodo is busy working" (a placeholder written before it has read anything), "PR Summary by Qodo" (/describe output), and the "failed to apply 'local' repo settings" error. Counting those would pass a PR the agent never reviewed. Verified against the live API: #2269 now 0 objects + 1 comment -> PASS, #2268 1 + 1 -> PASS, and the three noise comments are all rejected. Thread resolution is unaffected: a zero-finding PR opens no threads, so the unresolved count is 0 and the gate falls through to PASS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Problem
Maintainers are spending review cycles rejecting pull requests that look plausible but are structurally wrong for this project. The objections are consistent, but they live in maintainers' heads and get re-typed by hand every time.
Three recently closed PRs, and what was actually wrong with each:
general/overlay/etc/init.d/S70vendorand the sharedload_hisiliconSNS_TYPE0default. The overlay ships to every camera, so this changes the sensor on already-deployed cameras at upgrade time. It also added ~1200 lines of bring-up tooling undertools/imx662/that noConfig.inselects and CI never compiles, and repointedHISILICON_OPENSDK_SITEat a personal fork whileConfig.in:8still namedOpenIPC/openhisilicon..kofiles extracted from a camera's factory firmware, a kernel module rewriting vendor blob.bssat runtime, generatedlibhi_*.sofacades committed as blobs,LD_PRELOADinS95majestic, a kernel patch, and a single-board ircut script.What this adds
Mirrors the Qodo Merge setup already in use on majestic —
.pr_agent.tomlpoints the review agent at two companion files:best_practices.mdpr_compliance_checklist.yaml.github/PULL_REQUEST_TEMPLATE.mdThe rules cover blast radius (generic config vs one bench), provenance (package
SITE, version pins, blobs), repo boundaries (kernel → OpenIPC/linux, single board → OpenIPC/builder, probing tools → OpenIPC/ipctool, streamer bugs → majestic), no monkey-patching (LD_PRELOAD, runtime blob patching), hardware evidence, and dead code.They judge substance, not style.
.pr_agent.tomlexplicitly instructs the agent not to speculate about whether a change was AI-written, and not to flag writing style, comment density, or formatting. Those signals are unfalsifiable and punish thorough contributors. A change is judged on blast radius, provenance, evidence, and repo ownership.Evidence
No hardware involved — this is review configuration and documentation, and it changes nothing in any image. Verified as follows.
Both files parse, and the gate schema matches majestic's key-for-key:
Retrospective test — each closed PR trips the rules it should, confirmed against the real diffs:
False-positive test — the more important one. Walked the same rules over merged #2267, #2248, and #2266. None trips a gate. Two carve-outs were needed to get there, and both are now written into the rules:
casearm in a sharedload_hisilicon. That is additive and changes no board's default, so it must pass — only a change to an existingSNS_TYPE*, default resolution, or default profile is a finding. Its new.iniis picked up by an existingfiles/sensor/config/*.iniinstall glob and needs no.mkchange..github/scripts, which run under bash on CI runners. The shell-convention gate is scoped to scripts that ship in the image (general/overlay/,general/package/*/files/) and excludes.github/andcontrib/.Two corrections worth flagging
While verifying, two of my own draft rules turned out to be wrong and were fixed:
The bot's finding on gk7102: BR2_TARGET_OPTIMIZATION=-mno-unaligned-access #2260 was factually incorrect. It claimed
BR2_TARGET_OPTIMIZATION="-mno-unaligned-access"clobbers-Os -pipeand de-optimises every package. It does not. That kconfig string is appended toTOOLCHAIN_WRAPPER_OPTS(buildroot-2023.02.1/toolchain/toolchain-wrapper.mk:22), while the-Olevel comes independently fromBR2_OPTIMIZE_*(buildroot-2023.02.1/package/Makefile.in:114-133); Buildroot's own help text says "gcc optimization level is defined in build options". §1.3 now describes the real mechanism — a whole-image codegen change needing measurement — and tells reviewers not to repeat the clobbering claim.A "full 40-char SHA" gate would have failed hisilicon-opensdk: bump ff20187b → 2d637e35 (V4 mic tone on cold boot, osal timer teardown) #2267, i.e. this repo's own most recent merged commit. The tree is not uniform: 15 packages pin 40 characters,
hisilicon-opensdkhas always pinned 8, and several trackHEAD. The gate is now "a bump must not make the pin less specific", which still catches Add Sony IMX662 sensor to build system #2241's 8→7 regression without failing ordinary bumps.Scope
general/package/all-patches/linux/general/overlay/or in a sharedload_<vendor>script hardcodes a board-specific valueLD_PRELOAD, and no binariesNotes for reviewers
[github_app] pr_commandssets which commands run when a PR is opened, reopened, or marked ready for review; it does not decide whether they run. This PR therefore changes what the existing automatic review says, not when it fires. (An earlier draft of this description claimed review was manual-only. That was wrong.)handle_push_triggerdefaults to off, so a PR is reviewed on open, not on each new commit. Sethandle_push_trigger = truepluspush_commandsif that is wanted.pr_compliance_checklist.yamlholds only binary, objectively checkable gates; anything needing judgement lives inbest_practices.md.🤖 Generated with Claude Code