Stabilization: slow down risky merges with clear PR review rules

[RaresKeY]

Summary

I think review/merge policy should be a priority stabilization item before PR volume scales further.

This is not about calling out one maintainer or one PR. The current workflow lets good-faith reviewers move very fast, but it does not clearly define what approval means, when a latest commit needs re-review, or which PRs need independent review before merge.

I would like us to agree on a small v1/stabilization policy for reviews and merges.

Problem or Motivation

Right now, a PR can move from latest commit to approval to merge very quickly, including in sensitive areas. That works for tiny docs or typo fixes, but it is risky for auth, owner scope, data-loss, CI, tool execution, migrations, and broad refactors.

The failure mode is predictable: remaining issues are found after merge, then fixed in follow-up issues/PRs. That creates avoidable churn and makes it harder to know whether dev is stabilizing.

This is already showing up in concrete review work. I have been reviewing quick merges after the fact and finding real bugs that still reproduce on current dev. That is useful cleanup work, but it is the wrong place for the project to discover issues in risky changes.

One recent example is #3397. The PR was in auth/owner-scope code, had requested changes, and had public review feedback calling out owner-rename bugs. The final commit landed on 2026-06-09 at 08:16 UTC, was approved at 08:19 UTC, and was merged seconds later. After merge, more remaining bugs still had to be called out and split into follow-up trackers:

• User rename can leave auth renamed when owner migration fails #3607 - user rename can leave auth renamed when owner migration fails.
• User rename leaves active research jobs under the old owner #3608 - user rename leaves active research jobs under the old owner.
• User rename leaves upload metadata under the old owner #3610 - user rename leaves upload metadata under the old owner.
• fix(auth): skill owner rename misses mixed-case frontmatter and usage keys #3611 / fix(auth): case-insensitive skill owner match on rename #3614 - skills rename still needed a follow-up case-insensitive owner-key fix.

The point is not that this one PR was uniquely bad. The point is that the current process let a high-risk PR look resolved because there was review activity, while the latest code still needed more careful review before merge.

Proposed Solution

• Self-authored PRs need approval from another maintainer/collaborator before merge.
• Risky PRs need two real reviews before merge.
• After a changes-requested review, the PR is not considered resolved until a reviewer re-reviews the latest commit.
• Author comments or generated follow-up patches do not count as review by themselves.
• If a fix uses AI-generated code, the author should read it, understand it, and locally validate it before asking for re-review. Please do not treat generated follow-up code as ready just because it answers the review comment.
• The merger should verify that the approval applies to the latest commit and that required validation is clear.
• Tiny docs/typo/CI-unblocker PRs can have a fast path, but the fast path should be explicit.

Risky Areas

Require stronger review for:

• auth, owner scope, permissions, API tokens
• security and trust boundaries
• data loss, migrations, persistence, file ownership
• CI/release/deployment workflows
• tool execution, shell/file/database access
• broad refactors or route/schema changes
• changes that were previously requested/fixed with multiple follow-up commits

Reviewer Responsibilities

A review should say enough to show what was checked. Looks good is fine for tiny changes, but for risky areas the reviewer should identify the current head, relevant validation, and any assumptions.

Merge Owner Responsibilities

Before merging, the person pressing merge should check:

• the latest commit is the one that was reviewed;
• requested changes were re-reviewed after the fixing commit;
• required validation is present or the gap is explicitly accepted;
• the PR is not a self-authored change without independent review;
• any fast-path exception is obvious from the PR scope.

Alternatives Considered

We can keep relying on informal judgment, but that makes review quality uneven and leaves contributors unsure what standard they are being held to.

We can also move this straight into branch protection, but that may be too rigid before the policy is agreed. A lightweight written policy first seems easier to tune.

Open Questions

• Should risky PRs require two collaborator reviews, or one collaborator plus one trusted contributor review?
• Should self-authored PRs be blocked from self-merge by branch protection where possible?
• What should qualify for the fast path?
• Should the agreed rule live in the existing CONTRIBUTING.md Pull Requests section, branch protection, or both?

Prior Art / Related Issues

This follows existing repo discussion rather than starting from scratch:

• PR review process? #2872 - direct discussion about PR review process, self-merges, and unclear expectations.
• PLEASE STOP MERGING TO MAIN until proper CI, security checks and developer workflows are in place #2487 - earlier workflow/CI/security concern that led to the dev/main split and stricter PR expectations.
• How should we coordinate as contributors/maintainers? #2528 - contributor/maintainer coordination thread.
• Stabilization v1: candidate intake #3163 - Stabilization v1 intake discussion.
• Add a pull request review template #3126 / docs: add pull request review template #3128 - proposed PR review template issue and open PR. That helps reviewers write better review comments; this issue is about when review is required before merge.
• [Proposal]: CODEOWNERS 🗳️ #593 - CODEOWNERS / review ownership proposal.
• Proposal: CI baseline (correctness + security) gate for the repo #2288 - CI baseline / correctness / security gate tracker.
• AI contributor guidance: disclose the model used on PRs + guardrail for AI-filed issues #2421 - AI contributor guidance and model disclosure.
• Proposal: Architecture & Codebase Structure v3 #605 - broader architecture/codebase structure proposal with branch protection / CODEOWNERS / CI as critical-track work.
• fix(auth): sync file-backed and in-memory owner caches on user rename #3397 - concrete auth/owner-scope example where follow-up bugs remained after a fast final approve/merge.
• User rename can leave auth renamed when owner migration fails #3607, User rename leaves active research jobs under the old owner #3608, User rename leaves upload metadata under the old owner #3610, fix(auth): skill owner rename misses mixed-case frontmatter and usage keys #3611, fix(auth): case-insensitive skill owner match on rename #3614 - follow-up owner-rename trackers/patches from that review path.

Are you willing to implement this?

Partially. I can help turn the agreed policy into a small CONTRIBUTING.md update. #3128 already covers the review-comment template side. Maintainers should own the final review/merge rule.

[mchamsteryt2]

I agree, this repo needs some quality control because most pull requests are sometimes just blindly merged (no offense to the maintainers)

[RaresKeY]

I agree, this repo needs some quality control because most pull requests are just blindly merged (no offense to the maintainers)

I agree with the quality-control concern, but I would phrase it a bit more narrowly. I do not think “blindly merged” is fair as a blanket statement; there is real review happening and maintainers are catching issues.

The process gap I am trying to track here is that risky PRs can still move from latest commit to approval/merge very quickly, and it is not always clear that the final code got enough independent review before merge.

So the action I want is not blame. It is a simple rule set: slow down risky areas, re-review after requested changes, make sure approval applies to the latest commit, and have the merge owner check validation before pressing merge.

[vdmkenny]

I agree with the spirit: risky areas deserve more care than typos, and the merger should be confident the approval covers the latest commit.

One pushback on the framing though. The fast tempo is not blind merging. On the review path I am part of, every PR gets CI (pytest, node, compileall) plus an actual diff read, frequently aided by a model like Claude Opus, and for anything touching auth, owner scope, migrations, or tool execution I pull the branch and run the relevant tests or reproduce the bug locally before approving. The speed comes from volume pressure, not from skipping the work. Conflating "merged quickly" with "merged carelessly" is not quite fair to the reviewers doing that verification, and a policy written on that premise risks adding ceremony without adding safety.

Where I do think there is concrete, low-friction wins, all enforced by the platform rather than by the merger remembering:

• Enable the repo-wide branch protection option "Dismiss stale pull request approvals when new commits are pushed." That directly fixes the fix(auth): sync file-backed and in-memory owner caches on user rename #3397 failure mode you describe (approval landing, then later commits riding in under a stale green check), and makes "re-review the latest commit" automatic instead of a written rule.
• Block self-authored PRs from being merged by their own author, and require approval from a maintainer other than the last committer. That removes the self-merge gap without a judgment call.

The two-reviews-for-risky-areas point is reasonable to write down, but it is a maintainer call on where to draw the line. The three platform settings above are the pieces I would turn on today. cc @pewdiepie-archdaemon

[RaresKeY]

I opened a focused Stabilization v1 follow-up for the security-sensitive surface checklist:

https://github.com/pewdiepie-archdaemon/odysseus/discussions/4128

This keeps the broader review/merge policy here, while the Discussion tries to define the concrete reviewer convention for sensitive PR surfaces.

[RaresKeY]

@pewdiepie-archdaemon when you have time, could you take a look at this again?

One process concern I want to make more concrete: PRs touching hard-to-validate areas can still land before earlier review comments are explicitly resolved on the latest head. This overlaps with concerns already raised in #1986, and I think this issue is the right place to define the review/merge rule.

Concrete examples:

• fix(cookbook): allow local Windows Diffusers serving #4077 merged after review called out that remote Windows Diffusers still looked unsupported. That still appears present on current dev: the UI can offer Diffusers for a remote Windows target, but the remote Windows serve runner appends the command directly in PowerShell, while the helper-script copy/rewrite for scripts/diffusion_server.py only exists in the non-Windows remote path.

• fix(cookbook): stop local Windows process trees #4073 also looks concerning: the recursive Windows process-tree kill appears wired through the kill-session path, but the main Stop/Kill/retry path still goes through _tmuxGracefulKill(), which uses Stop-Process -Id $p -Force and removes session files. That was the P1 concern raised before merge.

Could we settle a short-term rule for hard-to-validate areas like remote hosts, shell runners, install flows, model serving, tokens, and auth/admin boundaries? Even a temporary rule like “check review/comment history before approval, then require fresh non-author review after the latest commit with explicit validation or explicit gating for untested target platforms” would help avoid letting unresolved review gaps sit in dev.