PR review process?

[relic664]

Is there any sort of PR review process? My PR is listed as stale after only 3 days (!) with no review, meanwhile maintainers are committing directly with no PR, maintainers are continually merging their own PRs with no review, and performing very little review of other PRs.

Thats fine if thats how this project is going to be run, but its worth disclosing as potential contributors would like to know that before spending time to write a PR.

[alteixeira20]

That is fair feedback, and I understand how it can look from the outside.

One clarification: when I use stale pr, I do not mean “this PR is old” or “this PR is not worth reviewing.” I mean the branch currently needs a rebase / conflict resolution before it is useful to spend reviewer time on it. If that label is being applied too broadly or unclearly, then the labeling/process should be tightened.

On my side, the small direct merges I made around the test suite were not meant to bypass review for feature work. They were narrow maintenance slices to make the test suite and CI easier to keep green, because broken or noisy tests were making unrelated PRs harder to evaluate. I kept those changes small so they could be reverted or audited easily if needed.

For project-direction changes, UI/UX changes, product features, and anything with broader behavior impact, I agree those should go through normal maintainer judgment, and I would defer final approval/merge decisions to @pewdiepie-archdaemon.

I also agree that the project would benefit from a clearer written review process: what ready for review, needs work, needs-validation, blocked, and stale pr mean; what contributors should do when a PR needs attention; and which kinds of changes require maintainer sign-off.

If you want my eyes on a PR, feel free to tag me directly in the PR thread or use the button to ask for a review (in your case you should ask @pewdiepie-archdaemon for review/approval)

Best regards,
Alexandre.

[sacb0y]

IMO @pewdiepie-archdaemon should deligate at least specific general features to individuals. Lay out some ground rules and let them handle the PRs for that topic. having one person handle all feature requests sounds like hell.

This would also stop people from bouncing around unfocused, it seems like bugs major to me that prevent my use go to the way side cause there's just random people doing things. While on something like blender there's a "Modeling tools guy" that always looks at bugs and PR's for that kind of issue.

Plus there's also no versioning in this repo at all to group PRs and ultimately pushes to main. So if a state of the repo works well for some people they won't know what to roll back to if things break.

[alteixeira20]

I agree with this direction.

Having people responsible for specific areas could help a lot, as long as “owner” means routing/review responsibility, not unilateral control. For example, someone familiar with models/providers, memory, UI, CI, install scripts, security, etc. could help triage and review that area so PRs do not just sit waiting for one person to notice them.

I also agree that versioning / rollback points are important. If the repo is moving quickly, users need some way to know “this was a known-good state” instead of guessing which commit to roll back to when something breaks.

I think these are probably two concrete follow-up tracks:

• define maintainer/reviewer areas so PRs and bugs get routed better;
• define a lightweight release/versioning policy so users have stable points to install or roll back to.

Both feel related to the PR review process, but they may deserve focused issues so they do not get lost inside this discussion.

[sacb0y]

In addition like I see on blender this list should be known so that people responsible can be tagged when a PR is up for review. This would also probably help tone down people tagging pewds all the time.

[RaresKeY]

This is a valid concern. I think the project would benefit from a clearer PR review process, and that should also apply to people with write access.

I’m not saying anyone’s code is bad or that direct commits are being made in bad faith. But having a fresh pair of eyes on changes before they go in is reasonable, especially for anything beyond very small maintenance fixes. Even with good intentions, people make mistakes, and review helps catch those before they affect the project.

[alteixeira20]

I agree with you @RaresKeY .

I think the standard should be clearer and should apply to people with write access too. At the same time, while the project is still stabilizing, I think there is a narrow case for very small maintenance/self-merged changes if they are low-risk, well-scoped, validated, easy to audit, and do not change product behavior, add features, touch security/owner-scope areas, or make open PRs harder to review.

For anything broader than that, proper review should be expected.

I think the productive next step is to move this into #2528 or a linked issue and write down a minimal PR/review process: when review is required, when direct maintenance is acceptable, what the review labels mean, and what contributors should do when a PR is marked stale or needs validation.

[RosenTomov]

At first I was also thrown off that stuff was being merged willy-nilly, I thought, "Alright, if that's how it's gonna be, maybe I should apply to be a maintainer just to get my own stuff merged." But I had to be honest with myself and just linked the prs hoping they'd get noticed #551 (comment)

I've decided just to keep my stuff merged on a separate branch and use that.

Anyway, the easy half of this is largely a configuration problem, add branch protection

Set something like:
Require a pull request before merging
Require approvals (1)
Require review from Code Owners
Require status checks to pass before merging
Require branches to be up to date before merging
Require conversation resolution before merging

And like @sacb0y mentioned, you can split responsibility per area in a CODEOWNERS file so PRs get routed to the right reviewer instead of all landing on one person.