ci: add LLM review rules - #29950
Conversation
|
@1715173329 What do you think? Is it too strict, or do you have any suggestions on what to add? You previously said I’m a bit of a bully regarding formalities, but since I’m still the only one reviewing PRs here, I want to get it right. It’s a pity no one else is doing them. |
| PRs targeting `openwrt-NN.NN` branches or titled `[X.Y] ...` are | ||
| backports. Their diffs should match the upstream commit on `master` |
There was a problem hiding this comment.
nitpick: we can have unique minor lts update for stable branches.
| - Absolute paths must be used (starting with `/`). | ||
| - Directories must end with a trailing slash `/`. | ||
| - Individual files must NOT end with a trailing slash. | ||
| - No indentation is allowed (do not start lines with spaces or tabs). |
There was a problem hiding this comment.
add some indentation tips for other recipes? like use 2 spaces for define Package/$(PKG_NAME) and define Package/$(PKG_NAME)/description and tabs for define Build/* / define Package/$(PKG_NAME)/install?
There was a problem hiding this comment.
Thanks for suggestion. Added. :)
This looks good overall to me. Honestly, I'm a lazy guy and I'm not very strict about patch rules. But I guess the same rules are (already) applied to other repos too. Of course, for complex patches, we should still describe them properly.
On the other hand, I hope we don't get too fixated on CI results, or on making every single check pass. They should be treated more as a reference, since CI can't cover every possible situation, and I don't think we need to follow every CI requirement. Sometimes contributors' PR may have small formatting issues that could be nitpicked, but I'm still glad to merge them.
As I mentioned above, I'm lazy, so when I see a PR with obvious formatting issues, I'll probably just choose to ignore them. Since I started working late last year, the time I can spend contributing to the open-source community has decreased significantly. I can no longer spend a whole day here like I used to. |
|
Possible false positive in the conffiles parser. PR #29343 ❌ Package/darkstat/conffiles line 'define Build/Configure' must not Raw byte inspection via od -c confirms the conffiles section is |
|
This is not right pull request. Even bot says what you should do. |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (640f3fd "ci: soften and extend LLM review rules"); its message matches the diff. Two questions inline on the new "Makefile indentation" section, where the stated rule disagrees with dominant in-tree practice.
Generated by Claude Code
| - **Two spaces** for metadata blocks, whose body is a list of | ||
| `KEY:=value` lines: `define Package/<name>`, | ||
| `define Package/<name>/config`, `define KernelPackage/<name>`. |
There was a problem hiding this comment.
define Package/<name>/config doesn't match this bullet on either count: its body is Kconfig syntax (config …, bool "…", depends on …, source "…"), not a list of KEY:=value lines, and in this feed it is predominantly tab-indented — 788 tab-indented body lines across 82 Makefiles vs 220 space-indented across 62, e.g. libs/libtins/Makefile:38-40. A reviewer following this bullet would flag the more common in-tree form as wrong, which is the opposite of what the "only raise indentation on lines the PR actually touches" paragraph is going for. The reviewer feedback that prompted this section only asked about Package/<name>, /description, Build/* and /install. Was including /config here deliberate, or should it move to the tabs bullet (or be left unspecified like /description)?
Generated by Claude Code
| - **Tabs** for blocks whose body is shell run by make: | ||
| `define Build/*` (`Build/Configure`, `Build/Compile`, | ||
| `Build/InstallDev`, …), `define Package/<name>/install`, and the | ||
| `preinst` / `postinst` / `prerm` / `postrm` scriptlets. |
There was a problem hiding this comment.
The preinst / postinst / prerm / postrm scriptlets are not recipes, so by the rationale one line above ("make only requires tabs where the block is a recipe") they don't belong in the tabs bullet. They go through BuildPackVariable in include/package-pack.mk:41-47, which echoes the block verbatim into a file — exactly the same mechanism as conffiles (registered a few lines below), which this section correctly describes as taking no indentation. Any leading whitespace simply ends up inside the generated shell script.
In-tree the unindented form dominates for these blocks: 53 Makefiles with unindented scriptlet bodies vs 27 using tabs (455 vs 143 lines), e.g. admin/memtest86plus/Makefile:94-100, where the shebang and body sit at column 0. Should the scriptlets be listed with conffiles under "no indentation" instead?
Generated by Claude Code
Add `.github/llm-review-rules.md` to define project-specific patterns and guidelines for the LLM review bot, alongside the equivalents in openwrt/openwrt and openwrt/luci. The rules cover what the review routine cannot infer from the diff alone: quilt-managed patch refresh, the generic version check and the test.sh / test-version.sh overrides, procd init scripts, conffiles layout, source and mirror macro preferences, and Makefile indentation per block type. They also set the review posture. Formatting that changes what gets built is treated as a defect, while cosmetic style is not worth blocking a merge over; findings the formality bot already posts are not repeated; and CI results are a reference rather than a gate. Patches only need to be upstream-ready when they are candidates for upstream, since many here are OpenWrt-specific hacks with nowhere to go. Stable branches may carry a minor LTS update that has no counterpart on master, so not every stable-branch PR is a cherry-pick. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5 participants
Add
.github/llm-review-rules.mdto define project-specific patterns and guidelines for the LLM review bot.Follow-up of #29888 to add review rules.
I took a quick look at it further, and I think there are so many rules. I'm not sure if that's good or not, but maybe... who knows? 🤷 🧙