Skip to content

qodo: review standards, compliance gates, and a PR template #2268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
widgetii merged 3 commits into from
Aug 16, 2026
+908 −0
Merged

qodo: review standards, compliance gates, and a PR template #2268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5032469
qodo: add review standards, compliance gates, and a PR template
widgetii Aug 16, 2026
d8f7f96
qodo: fix review findings, add merge gate
widgetii Aug 16, 2026
90c8684
qodo: move shell portability out of the hard gates
widgetii Aug 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<!--
Review standards for this repository live in best_practices.md and
pr_compliance_checklist.yaml at the repo root. Reading them first is the fastest
way to get a PR merged.

Not every change needs every section — a typo fix does not need an oscilloscope
trace. But a change that alters what runs on a camera needs all four.
-->

## Problem

<!--
The symptom, and which board(s) show it. "Improves stability" is not a symptom.
If this adds new hardware support, say which SoC and which sensor instead.
-->

## Hardware tested on

<!--
SoC and board, e.g. "gk7205v200, Xiaomi Dafang". CI proves an image builds; only
your board can prove it boots and streams.

If you have not run this on a camera, the PR is not ready — say so here and open
it as a draft rather than leaving the section blank.
-->

## Evidence

<!--
Before and after: logs, dmesg, ipcinfo output, stream behaviour, image size.
Paste the output. A description of the output is not the output.
-->

Before:

```

```

After:

```

```

## Scope

<!-- Tick only what you have actually checked. An unticked box is fine; a wrongly ticked one is not. -->

- [ ] 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_<vendor>` 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
181 changes: 181 additions & 0 deletions .github/workflows/qodo-gate.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
94 changes: 94 additions & 0 deletions .pr_agent.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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_<vendor> 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-<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
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_<vendor> 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.
"""
Loading
Loading