kernel: rust: add initial prompts#39
Conversation
| @@ -0,0 +1,43 @@ | |||
| # Rust Subsystem Details | |||
|
|
|||
| General coding guidelines in `Documentation/rust/coding-guidelines.rst`. | |||
There was a problem hiding this comment.
| General coding guidelines in `Documentation/rust/coding-guidelines.rst`. | |
| Read the general coding guidelines in `Documentation/rust/coding-guidelines.rst`. |
Perhaps?
There was a problem hiding this comment.
It's actually better to not have any such instructions in per-subsystem prompts. Sashiko runs a phase 0 where it decides which prompts to load and it can easily load the documentation from the kernel tree as well. This helps to preserve the token context and is overall more efficient.
There was a problem hiding this comment.
Do you mean you will add this in phase 0, or that we should inline everything we want here even if it is "duplicated" (perhaps summarized?), or that the phase 0 dynamically decides what to load (if so, is that decision deterministic)?
i.e. it would be nice to know what will be loaded. I have also a list of "short sentence rules" I am accumulating with things we typically have to point out in reviews, like conventions etc.; so I was planning to put that in Documentation/rust/ and then link it here as well.
Thanks!
There was a problem hiding this comment.
I added this mainly to get agent to report about import style issues. I can also just inline the parts about vertical style if that's better?
I use it with /kreview command, which does not go and check for Documentation/rust/coding-guidelines.rst without this prompt.
There was a problem hiding this comment.
I think the kernel docs count as subsystem knowledge? Jakub requested that we have the networking .md do something similar.
|
|
||
| For abstractions *ONLY*: Functions that are small or forwarding to a binding call should be annotated with `#[inline]`. Leaf crates like drivers are exempt. | ||
|
|
||
| REPORT as nit if used incorrectly. |
There was a problem hiding this comment.
For bugs, I see the repository marks this with bold. Not sure if we are supposed to do it for nits or not, e.g.:
| REPORT as nit if used incorrectly. | |
| **REPORT as nits**: if used incorrectly. |
(If so, there is another one below)
There was a problem hiding this comment.
Can we, please, stick with critical/high/medium/low severity as currently used by Sashiko?
There was a problem hiding this comment.
It's easy to confuse LLMs with controversial output requirements, so it's better to use the same wording consistently across all prompts.
There was a problem hiding this comment.
It's easy to confuse LLMs with controversial output requirements, so it's better to use the same wording consistently across all prompts.
Yeah, that is why I mentioned the bolding, since I saw the bolding elsewhere in this repository.
The nits vs. level is yet another thing we should be consistent. From a quick look, this repository uses **REPORT as bugs** a lot -- do you mean those (and this one etc.) should be updated e.g. **REPORT as high severity**: or similar?
There was a problem hiding this comment.
Roman and I briefly discussed this yesterday, but didn't really come to any conclusions. The wording requirements of inline-template.md would need to be changed to allow some kind of severity language on a per-issue basis. We can certainly do this, but it'll take a few iterations to hold back LLM's natural tendency to be dramatic about bugs.
Another option is to put a json snippet at the bottom that has tags for each issue. I personally like that better because it's parsable by other tools, but I'm open to other ideas. We could always do both.
There was a problem hiding this comment.
Even if we don't report it in the inline review, it's better to be consistent across prompts, otherwise the LLM has to guess each time how "nit" maps to whatever severity notion we use elsewhere.
Re json snippet: it works too, my only concerns is that for large patches with many findings it might be hard to map inline comments to the long list below. Maybe we can simple enumerate comments like [1], [2], etc and refer to them in json?
There was a problem hiding this comment.
Let me try testing the prompt without these two REPORT instructions. If it works then I'll just drop these for now.
There was a problem hiding this comment.
Thank you!
It feels like we need a prompt for subsystem prompts. I'll take a stab on this...
There was a problem hiding this comment.
Unfortunately without these two REPORT instructions, gemini-cli (sampling size = 1) doesn't report on the obviously missing #[inline]s. With the instructions (sampling size = many) in my experience it reliably catch those issues.
There was a problem hiding this comment.
Maybe we can simple enumerate comments like [1], [2], etc and refer to them in json?
Yes, that's what I had in mind
|
Once @ojeda signs off I'm happy to take this, just let me know. |
|
A couple of small issues, otherwise LGTM. Thanks! |
This addresses the most common false positives produced by LLM due to differences between kernel and userspace Rust. It also tell it about our coding guidelines so it can spot things like missing INVARIANT comments. Signed-off-by: Gary Guo <gary@garyguo.net>
|
I think all review comments have been resolved (the ongoing discussions are more general topics and not specific to this PR?) |
|
Yeah, let's get something in so that the reviews (hopefully! :) improve, since people is already looking into the reviews (which is great). We can iterate further later and add more details etc. |
|
Thanks everyone, merged. |
|
Thanks Chris! |
This addresses the most common false positives produced by LLM due to differences between kernel and userspace Rust.
It also tell it about our coding guidelines so it can spot things like missing INVARIANT comments.
cc @ojeda @rgushchin
For the trigger, I tested "any Rust code" and it seems to be working well. Should it be
rust/, **.rsinstead?