Repository navigation
Conversation
TomasKorbar
left a comment
TomasKorbar
left a comment
There was a problem hiding this comment.
This agent should not focus on the review of the changes. The task is to verify results of the CI pipeline if there is a failure. You should not check whether patches apply what changelog entries there are etc. Changelog entries and Patch additions are done by 90% from deterministic steps, thus rerunning them would result in the same failure and loop. The task of the MR verification agent is to wait until CI finishes and review its result only when there is a failure. Also if you would be checking spec file changes, you would have to verify that they conform to package rules, because maintainer can request his own formats/processes etc, which is something that should be done in the particular steps already.
| The MR verification agent performs that mechanical pass first. It reads the MR, | ||
| checks it against a fixed list of RHEL packaging invariants, and posts a review | ||
| comment. It does not replace the human reviewer and it does not merge anything. | ||
| Its value is that a maintainer opening a Ymir MR already knows where to look. |
There was a problem hiding this comment.
There's heavy overlap here with the proposals in https://gitlab.cee.redhat.com/linux-engineering/wg-ai-sdlc/-/merge_requests/10
I think it makes more sense to have an agentic review process in RoG that runs on every MR.
| that another AI agent opened against a dist-git repository. Your job is to decide whether a | ||
| human maintainer could merge it as-is, and to report every problem you find. | ||
|
|
||
| You are a REVIEWER, not an author. You must never modify the working tree, never create or |
There was a problem hiding this comment.
Personally I think constraints like this are better enforced in code than in text. If we want the working tree to be read-only we can...just mount -o remount,ro on it or so.
| - Every `PatchN:` tag has a matching `%patchN` / `%autopatch` application (or the package | ||
| uses `%autosetup -S git`, in which case tag order is what matters). |
There was a problem hiding this comment.
I think nowadays there's an rpmlint check for this? We should really try to make most checks use deterministic tools where possible.
And of course the initial drafter agent should be running those same tools too...
Also an important thing to note is that we still have fully deterministic CI in RoK that is likely running some of this stuff already too.
|
|
||
| 2. **Patch provenance and content** | ||
| - Each added `.patch` file carries an upstream commit reference (`commit <sha>` header, | ||
| `Upstream Status:`, or a URL) — a patch with no provenance is a blocker. |
There was a problem hiding this comment.
This exists as https://docs.fedoraproject.org/tr/packaging-guidelines/PatchUpstreamStatus/
This one is interesting because we have a lot of history here and people do it different ways, but I think this could be a deterministic check ideally - at least if there aren't prior non-compliant patches.
| CVE fix that touches forty files is suspicious. | ||
| - The patch does not carry unrelated hunks, refactors, or version bumps. | ||
| - The patch applies: use `run_package_prep` to confirm, when the task tells you the source | ||
| tarballs are available. A patch that applies with heavy fuzz, or fails, is a blocker. |
There was a problem hiding this comment.
Again though, if the patch fails the build fails; it's not clear to me that it's valuable to have this patch agent also rerunning rhpkg prep...
3 participants
TODO:
packit/packit.dev.Fixes
Related to
Merge before/after
RELEASE NOTES BEGIN
Packit now supports automatic ordering of ☕ after all checks pass.
RELEASE NOTES END