RFC: Revised review and maintenance practices #3421
Replies: 13 comments 48 replies
|
I kind of feel like we need a multi-approach here for review, Expert knowledge is a good filter for preventing problems, and ps "security, and networking" but I almost believe we need to separate the role between maintainer of scope and maintainer of code, AI is good, but not that good. |
|
When you say "maintainer of scope", I assume you mean product features? This is where a product manager and other product stakeholders typically adjust the roadmap based on user/customer needs and the marketplace. This is always less structured in an open-source project, but that doesn't mean we shouldn't have that sort of leadership. Maybe a steering committee that understands the broader ecosystem's needs and has a clear vision for the project. This sort of structure adds complexity and friction, but I do feel like Lemonade has reached the point where it needs this. Right now, I think we default to @jeremyfowers and what we typically call the core maintainers. But actually, I think this could include people who aren't code contributors/committers. I have no clue who those people might be, but we might want to step back and see if there are stakeholders we aren't currently considering. |
|
I’m aligned. The “2 reviewers, one of whom is the expert/owner” was intended to cover this concept. The idea wasn’t for both reviewers to do a deep code review, but rather one to check the shape of the PR and the other to check the code. |
yeah that is correct :) |
|
As per discord, happy to help in anyway I can, whatever route you choose to take moving forward. As already demonstrated I can test and find / report bugs!! so happy to do that as and when needed. Or of course anything I can be useful for. |
|
@PCAssistSoftware I really appreciate this! I just updated this RFC with something you could potentially help a lot with:
If we do a good job of following this policy, it would be super helpful to have someone not only volunteer to be a tester (like you're currently doing with GUI3), but also to help organize and recruit groups of testers. For example #3427 and #3426 could both really use some unbiased human testers. |
|
I am definitely more than happy to continue to be a tester, and will keep an eye on both Discord and GitHub to see what new PR need looking at, or if anyone specifically wants me to test something they can tag me on either and I will get testing!!. As someone who uses Lemonade everyday for both personal and own business I can always spend as much time as needed to test things out, especially when GUI3 gets released (can't face going back to GUI2 now :)) I will have a read through both of those mentioned issues and make any comments necessary. As for organize and recruiting groups of testers, not sure how I would do that or how that would work, but definitely open for further discussion. |
|
A really helpful CC prompt for the proposed flake detection automation:
This has generated leads that led to 6 very nice PRs so far. |
|
I dislike the requirement for an issue before a PR is started or made. To my mind it should not be necessary to get permission to try contribute something. There's a thing about this open source project that is broken, or unimplemented, or doesn't work quite like I think it should ... instead of complaining, why don't I just fix it? |
|
An issue doesn’t have to be a complaint though? It can say “I intend to solve problem X with design Y, what do you think?” The alternative, which we discussed in the meeting, was to keep the discussion on the PR. First, a high-level debate about the contribution directly on the PR, then a code review after the debate was settled. The main reason we leaned away from on-PR discussion is because ideas often naturally span multiple PRs, for example the 5 MCP Server related PRs open right now that are easily discussed in a single issue. I also think there is nothing wrong with opening a draft PR as an example implementation of the issue ( @abn did a great job of this with the sandboxing work) ; it’s just that the high-level conversation needs a place to happen. Fixes that don’t alter the UX spec are also always welcome as PRs, without an issue first. Hopefully this helps explain the position a bit better? Anyone is welcome to disagree, and I thank you as always for your thoughts @ckuethe. The one thing we can’t have is a free-for-all any longer, so we need concrete proposals to introduce some order. |
|
An example of what fear is anomalyco/opencode#6231 - people have been saying for months "hey, opencode should fetch the available models from the Cognitively, I conflate complaint, concern, correction, contribution ... ("c" what I did there?) There's a change I'd like to see and waiting for a consensus before I start working on something puts my motivation/focus at risk. My li'l squirrel brain gets bored and distracted. Anyway, I can dislike things and still tolerate them (eg. traffic lights. i get they're useful but that doesn't mean I like them) ... just figured I'd share my comments. |
This is the biggest concern I have with the current proposal. |
|
At the risk of making the policy too complex, I would be fine with a 3-tiered policy:
Thoughts? My hope is that the new middle tier would capture most of the contributions you’re referring to. |
Proposal: Expert Review Teams + a Controlled AI Review RungLet's say we continue the 2 Reviewer per PR policy, one concern I have is how we handle the large number of small and routine PRs without making two full human reviews mandatory for every single change. I think we can keep the safety of the two-review rule while adding a controlled fast path based on the subject-area expertise we already document in the maintainers table (https://github.com/lemonade-sdk/lemonade/blob/main/docs/dev/contribute.md) or how we can agree otherwise, this is just an example or mock-up. Having those what GH calls teams might help getting the right reviewers onto the right topic for them. Give each expert team an AI reviewerI would then give each of these areas a dedicated AI review agent. This should not be a generic "review this diff" bot. The agent should be heavily specialized for its area, we can define later exactly how. One element will be a dedicated document suitable for AI reviewer guidance e.g. like llama.md for the agent who is most focused on that aspect etc. These .md files have the team as codeowners and at least one has to approve a change there. The goal is to make the agent useful as an additional subject-area expert, not as a replacement for the humans responsible for that area. Safety: One Repo Manager Bot, many expert agentsImportantly, I would not give every expert agent its own GitHub identity. There should be one Repo Manager Bot that is the only AI identity capable of posting an approving review. Behind it we could have CI Agent, Security Agent, GUI Agent, etc., but they all submit their result through the same GitHub bot account. That gives us a useful security property: AI can never satisfy the two-review requirement by itself. Even if two different expert agents both think a PR is good, GitHub still only sees one Repo Manager Bot. There can therefore only ever be one qualifying AI approval. At least one human must always be involved. Human-controlled delegationThe bot should also not be freely callable by every contributor. Only members of the corresponding expert team, plus repo admins, should be able to delegate a review to that area's agent. It also covers an important case: if two experts are already involved because one wrote the change and the other reviewed it, forcing a third human into a tiny fix does not necessarily improve quality. The second approval could come from the specialized agent after an independent review. Likewise, the bot could be used as a third reviewer when two humans want another opinion on something tricky. An example of an implementation concept - for those who have interestStrict boundaries for bot approvalsI think an AI approval should have significantly stricter rules than a human approval. At minimum:
And I would add one particularly important escalation rule: If the agent detects a scope expansion, architecture change, significant UX change, security-sensitive change outside its normal bounds, or anything else that belongs in the highest review rung, it should submit So the agent's uncertainty makes review stricter, never weaker. Bot permissionsFor GitHub to count the Repo Manager Bot's approval it would need The bot can use a narrowly scoped credential that permits reading the repository and posting PR reviews, but not pushing code, modifying repository contents, changing settings, merging PRs, etc. The GitHub identity has the role required for its review to count, while the credential actually available to the service exposes only the operations it needs. Together with the single shared bot identity, this gives us two useful guarantees:
How I see the review rungsRoughly: Small/routine PR
Small PR where author + reviewer are both subject-area experts
Normal PR
Large / RFC PR
Scope / architecture / major UX / exceptional PR
I think this preserves the core idea behind the RFC: slow down where decisions actually matter, improve maintainer visibility, and make it much harder for significant changes to slip through. At the same time, it avoids spending two humans on every typo, isolated bug fix, CI cleanup, or small implementation correction when the people responsible for that area are already comfortable with the change. The important distinction for me is that AI does not get authority over the project. The existing expert teams keep the authority and gain another tool they can deliberately use. |
|
It might be helpful to edit this down to a concise description of the core idea. We can get into the details later if the core idea is popular. |
|
It would be helpful to add me to New backends since I made the new acceptance requirements for adding backends :P just saying lol and maybe even llama.cpp since I contribute to llama.cpp |
|
It’s better to view the table on here as a mockup, its not authoritative and doesn’t need to be debated at this point. |
|
I focused the post a bit so it is more accessible. |
|
Just posting my thoughts now that I've had time to read the RFC. The 3-tier covers most of what I would have raised, and it complements @fl0rianr 's expert team proposal. The tiers decide whether a change needs discussion, the teams decide who runs it. I'd rather those become one policy than two separate proposals. The case I know best is #2660. It went through five review rounds with multiple engaged reviewers. It did get listed as a breaking change in the v11.5.0 release notes. The change hit the benign same-origin case, a user's own browser against their own server. There was a migration path via the What actually failed is the classification step during review. Removing the same-origin fallback was treated as (rightfully so) an internal security hardening, so nobody asked whether it was user facing, who was affected, or if the migration path was acceptable. That question is what would have caught it before release. A release note saying "breaking" cannot prevent breakage. So the cheapest high value addition is a required declaration on any PR that touches existing behavior: does this change user facing behavior, who is affected, what's the migration path. Thirty seconds to answer, and it is the one question that would have caught this before it shipped. It also gives the triage agent a concrete field to check instead of a judgement call. A few things I'd want defined before this ships. First, a definition of done for an RFC issue. "Open until all interested maintainers comment", one side waits on the PR, the other waits forever on the issue. I'd make the outcome a filled in template: scope, affected users, breaking change declaration, migration path, named reviewers, closed by a designated owner. A default comment window of about 7 days, with escalation to a designated owner on expiry. A one page artifact with a status, not a standards document, is where structured spec earns its keep. Second, triage is a new bottleneck, so time to first response is the metric most at risk. I would not promise a hard SLA, that tends to not survive contact with volunteers. But I would track the number and set a plausible target tiered by complexity: fast on trivial fixes, a few days on routine work, about a week on the breaking scope changes. Last, I'd like this RFC, and any RFC, to be explicitly revisable. Decisions should be terminal: once approved, approved. So we can fix what does not work without reopening settled decisions. That lowers the pressure to be right on day one, and it makes it safe to hold a strong opinion loosely. |
|
I will add lifecycle details if we are good with the overall concept. |
On the AI content policy in the body. I agree with the intent, the design phase is exactly where human effort should be concentrated. But I'd soften an explicit "no AI generated content" rule. Mostly a practical question: who decides what counts as AI generated? That only gets harder. And it penalizes the benign case, using an LLM to condense rambling thoughts into something reviewable is not the failure mode we're worried about. The failure mode is low signal content, whatever the tool. So maybe the rule should target that. Authors are responsible for communicating their intent clearly and concisely, and reviewers can ask for a rewrite when it isn't. Same intent, but enforceable in spirit instead of through provenance. |
This is what I am leaning towards. IMO we need something in the contribute.md guide like:
An example usage would be like:
The purpose of having the rule citable is so reviewers don’t need to justify asking for the rewrite each time. That said, authors are still personally responsible for high-quality discourse. If someone posts an AI slop comment, I “Rule 1” it, and they just feed it into their AI like “claude reply to @jeremyfowers comment” and give me more slop back, that conversation is over. No human in their right mind will spend time arguing with an LLM. |
|
PS. I wonder if we replace our entire AGENTS.md file with the just rules for “What the text must adhere to at best effort:“ we might all have a better time 😂 |
|
I completely disagree with using AI on an entire PR, I would agree only if there is a requirement to have some human written text like the basic, problem / solution in human readable text. I say the end as a joke. But I would expect if the person expects me to review their PR they would respect me enough to actually have written something themselves, it also forces the user to understand or think about what they are doing more and it gives me the confidence that the person submitting the PR can reply to my comments without AI talking for them, because if that's the case why dont I just use AI to review everything and never open GitHub myself. Like this is a mutual two way street here and we need to respect each other. I'm not saying anyone is not respecting someone but this is my honest take. There is a reason why we have bugs right now and it's very apparent it's related to the pure use of AI with little to no human review on things. Some of us are reviewing ourselves but some arn't or cant, anyways, my point is this. :) I hope I dont ruffle anyones feathers here with this, im just being a bit blunt. |
|
You make a great point, @Geramy, about "human readable text" but I completely disagree with your conclusion. That's what I like about @jeremyfowers "Rule 1. High quality discourse is essential". It focuses on the right part of the problem. |
|
@SlawomirNowaczyk you could be right here I guess if it’s readable it’s readable sure. But AI usually sounds and reads like AI now that doesn’t mean someone who’s good at using AI can’t “fix it” but the majority won’t so I can agree with you on that. But usually it’s not the case. |
|
Another question that I have is around how far to lean on real RFC practice. Examples to look at are likely the KEP (Kubernetes) and PEP (Python) processes to ensure the shape of what we are building allows for adapting to growth. While strictly speaking this is a later problem, it does not hurt to be aligned. In the same vein, it would be good to have a defined lifecycle, eg: |
|
I like the idea of trying it out for a little bit of time: the concept of "big changes should be discussed on design level before we get boggled down into code details" certainly makes sense... I believe we are all worrying about the specific definitions (what is big enough?) and it's virtually impossible to come up with a fool-proof definition -- but should be relatively easy to develop community-based understanding of the best practices. Let's see how often -- in practice -- we end up telling contributors "this must be discussed as an issue before the PR", and if it leads to more improvements or more frustrations :) |
|
I only partially agree with "if you put in a lot of work and it is declined it's most frustrating". It's the most frustrating if that rejection is arbitrary and unexpected. If we have a clear policy which we can point people to, it's at least a little bit better. |
|
One thing not mentioned on this RFC, but is certainly true, is that Working Groups are still a thing, and any work within this scope of a Working Group is pre-approved and doesn’t require its own RFC. For example, development of the Smart Router, as long as it doesn’t inflict breaking changes or any blast radius outside the Smart Router, is pretty much all pre-approved already. There are basically two kinds of PRs we’re trying to catch with this new policy:
|
|
I think we should instead phrase it as a "if you submit features nobody asked for, don't be surprised if it's rejected" type of rule. Obviously we should be flexible, but adding features for literally one user just because that user has submitted a functional PR of a feature is a great idea for terrible feature-and-code-bloat and the project to become an unreadable and unmaintainable mess. In this respect, adding an issue or a discussion topic does wonders for answering the "am I the only person in the world who wants this feature in this shape?" question, which a lot of contributors (especially the ones who use AI and thus don't necessarily invest a lot of effort into their PRs) should be asking themselves. |
I'd strongly oppose such a rule. Yes, sometimes users propose features for their unique, bizarre situations... but ultimately, most of the progress in an open source project comes from people who add things they see as useful. I don't think we should by default assume nobody else will find that useful. |
|
I don't mean "reject features outright". I mean just what I wrote - "don't be surprised if it gets rejected". Obviously, I don't mean to deter people from proposing features they deem useful, because oftentimes others will find them useful too. But that's not always the case. And in those cases where the use-case is too specific and the code is too heavy, such rejections are perfectly fine. I'm writing this because I've noticed a trend among some PR authors to think they have a right to get their PR merged just because they submitted it and it works. There should be a way of communicating that this is simply not the case. |
|
For RFC discussions - something I have noticed from the discussion on a few recent RFCs is that there is no way to mark as accepted, ignored, resolved and it turns into a large amount of noisy text that isn't threaded. 3 things come to mind that can help.
|
|
I think issues are better suited especially as issue fields offer a lot of flexibility. So make an issue template for an RFC and can then track status data like accepted, ignored, resolved etc. And this can only be modified by write or triage access. Would fit in well for the repo manager etc. |
|
I really like that Discussions offer native support for threaded conversations (like this one!). I find it really hard to track multi-threaded conversations on issues. I also like that Discussions have upvotes, which is a clearer voting signal than emoji.
This is ideal for me as well… after some discourse converges the main body should receive updates (see my Update 1, Update 2 above on this one).
@fl0rianr is there anything stopping us from making a Discussion template? I know that discussions can carry labels, so we can use those for |
|
labels should work so GH discussions are possible. |
|
Ok cool, in that case I have a strong preference for using Discussions instead of Issues for the RFCs. For example in your RFC @fl0rianr the conversation is now re-visiting a topic that already reached consensus... it would be much easier if that topic was its own thread, and we could just declare the thread finished. |
|
I'll just chime in on the CI maintenance part: I think this should be split into actual "CI review" parts and a "TODO" part (with the TODO out of scope of this document). The critical TODO part is fixing the existing CIs to account for the non-deterministic failures (i.e. auditing the flaky CIs to weed out the non-deterministic parts, removing internet dependencies as much as possible etc.), while the maintenance/review should emphasize that CIs need quality control and list the explicit things we find during the audit that cause the flakiness. |
|
That is fair. When I started this RFC I was addressing my two biggest pain points in one shot, but it’s clear from discussion that this RFC can be totally focused on review policy. @superm1 already opened a dedicated RFC for a CI TODO so we can migrate CI discussion there. |
|
I agree on the point to limit internet access, but does that mean we are final on adding any new test e.g. with model downloads? What if someone provides a new backend or a proper validation for a new one that is missing it. Just a detail, I'm fine with the rest. Policies like please avoid whenever possible... are more flexible but none is a guarantee no such test does slip in. |
In this case:
|
|
I want us to change the PR template, to ban the use of AI prose in PR bodies, the consequences are they have to fix it, if they don't, it gets closed. Plainly: write your own PR description. If you paste in text an AI wrote for you, a reviewer will ask you to rewrite it in your own words before anyone reviews the code. I am not opposed to AIs bickering in the comments between each other although it gets very expressive sometimes haha. |
|
I don't think this works out as so many PR descriptions are AI written.
|
the Idea is we tell them to fix it or close the PR, this is what llama.cpp does and it seems their developers are happier for it. |
I heard so as well ;) |
|
There's already quite a bit of discussion about this here: #3421 (comment) Overall, I agree with the conclusion there, that we should focus on the contents, not the tool. One can make a great PR description using AI, and a crappy PR without one. |
|
I don't agree with this honestly. |
|
Closing since the associated PR is close to merging :) |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
DRAFT: Nothing written here is decided; this is a request for comment (RFC) that uses a draft proposal as the foundation for a robust discussion.
Update 1: at the biweekly maintainers meeting we discussed the RFC, and agreed we should try issue-driven roadmapping for new features. The PR Triage section of this RFC has been rewritten for this new approach.
Update 2: added AI content policy for AI issues.
Update 3 (August 31): Replaced the old CI section with specific test-related review policies.
Background
During 2026, Lemonade has had a relaxed PR review policy: any 1 reviewer, out of ~15 individuals with
writepermissions, could approve and merge a PR, without any visibility or discussion with any other maintainers. The high volume of contributions and weekly release schedule brought us to a state where repo admins are not fully aware of what they are releasing. In turn, there have been at least 3 instances where a very significant change has been released that is misaligned to the project's principles, leading to lengthy debate, troubleshooting, roll-back, etc.I'm not writing this to blame anyone. As the "person in charge", I take 100% responsibility. I'm writing this because we need a change and I want everyone to be able to contribute to defining the next era.
Finally, it's worth calling out that everyone's incredible contributions and velocity made the project what it is today. I would not rewind the clock. Rather, we've entered both a new era for global software development and a new stage of project maturity, and we need to adapt to these changes.
Current Status
We are still working through the problems we've identified. In the meantime, I'm restricting merges into main branch such that only PRs that have been heavily vetted will merge and enter the next release. The goal of this discussion is to identify and migrate to a less restrictive policy as soon as is feasible.
First Principles
Any solution should adhere to this guidelines, in priority order:
Specific Problems
Here are the specific root problems I believe we face (not comprehensive; feel free to propose more):
Proposed Solutions
Don't be Afraid (to slow down)
The project started with a motto: Don't Be Afraid (DBA), which we used to encourage ourselves to ship fast. Let's keep the same motto, but with a different meaning: now that the project has matured, don't be afraid to move at a comfortable pace. Don't be afraid keep the project scope reasonable and aligned. Don't be afraid to slow down.
Issue-Driven Roadmap
All PRs that adds to project scope, adds a new feature, or implements a comparably impactful change, must be linked to a GitHub RFC issue where that feature has been approved for addition to Lemonade.
AI-generated content is strongly discouraged in any issue, including this kind. Participants are encouraged to request authors to rewrite any AI content in their own words.
Definitions:
This RFC Issue must remain open for debate until the maintainers are in agreement that:
Crucially, the issue must remain open until all maintainers with an interest in the subject have had a chance to either comment or explicitly decline to comment. Everyone should do their best to notify the relevant maintainers as early as possible after the issue has been opened.
Authors who open PRs that fit the definitions above, but without a linked approved issue, should be instructed to open an issue before the PR is reviewed.
PR Triage and Review Rungs
Every PR needs to be triaged before it is reviewed. The primary goal of triage is to assess whether the PR required an RFC issue (see above) or is a routine change that doesn't require an RFC.
Additional goals of triage for RFC PRs:
Non non-RFC PRs:
Testing Specifics
We need new, specific rules for testing that will improve the overall robustness of Lemonade for users and developers.
*disruptive changes examples: replacing the entire GUI, refactoring the build system, adding tools to MCPs (thus adding capabilities to non-deterministic agents), any breaking change, and anything involve automatic migration.Conclusion
If you've read this far, read at all, contributed to Lemonade, used Lemonade, told your friends, thank you :) Here's to the next year of adventure in local AI!
DRAFT: Nothing written here is decided; this is a request for comment (RFC) that uses a draft proposal as the foundation for a robust discussion.
All reactions