Repository navigation
docs: define community contribution review and merge policy - #1413
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues regarding ownership and inactivity-closure criteria remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This documentation-only PR adds a community contribution review and merge policy to CONTRIBUTING.md.
Changes:
- Defines ownership, review readiness, approvals, and feedback expectations.
- Documents safe execution, squash merging, and inactivity follow-up.
- Review comments request clearer ownership, closure criteria, and demo-scope alignment.
| File | Description |
|---|---|
CONTRIBUTING.md |
Adds the community contribution review and merge policy. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Clarify the process for handling inactive PRs and reminders. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Clarified requirements for PR readiness review. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Thanks for writing this down. Most of my comments are about making the rules enforceable or accurate for how our CI and repo setup work today. None of this blocks merging. The first two are the ones I'd most like to see addressed, here or in a follow-up.
Main points
-
Make the untrusted-code guidance concrete for Actions. The principle is correct. Maintainers will act on it most often when they click "Approve and run workflows" for first-time contributors. Suggest adding: read the diff before approving workflow runs, and give extra scrutiny to changes under
.github/workflows/, including the gh-aw.md/.lock.ymlagentic workflows, and topackage.jsonscripts/lifecycle hooks. Also: never addpull_request_targetor secrets-bearing triggers to handle fork PRs. -
Ownership and area review need a mechanism. Community contributors can't provide it. Steps 1 and 3 assume someone assigns an owner and brings in "someone familiar with the affected area". A community contributor has no way to know who that is. The repo has no
CODEOWNERSfile today and no workflow that auto-assigns or requests reviewers. Without that, the three-business-day target and the two-approval rule depend on someone noticing the PR. Suggest either:- adding a
CODEOWNERSthat routes high-risk paths to the right people or teams (packages/db-migrations/,.github/workflows/,packages/github-auth/,apps/token-manager/, auth/RBAC code,packages/shared/) and has a catch-all*entry pointing at a maintainers team so every PR gets a reviewer request automatically, or - a team-level auto-assignment (GitHub team review assignment, round-robin or load-balanced) or a small workflow that assigns an owner when an external PR opens.
Then the doc can describe what happens instead of asking someone to remember. With branch protection's "require review from code owners" and "dismiss stale approvals" turned on, it also enforces "someone familiar with the affected area" and "approvals cover the latest substantive changes". The mechanism can land in a follow-up, but the doc should name it.
- adding a
Smaller suggestions
-
Extend the high-risk list. Consider adding new or upgraded dependencies (supply chain, plus the
NOTICEregeneration the template already asks for) and breaking changes topackages/shared, since every app depends on it. Also consider changes to project scoping / data isolation. The by-id invariant inapp-design.mdis exactly the kind of thing a routine reviewer can miss. -
Squash-merge details. GitHub keeps the PR author as the author of the squash commit, but anyone else who pushed commits is only credited if a
Co-authored-by:trailer is kept. Suggest: "Keep the contributor as the commit author and keepCo-authored-bytrailers for anyone else who pushed commits. Use a Conventional Commit title with the PR number (e.g.fix(docs): … (#1409))." -
Inactive-PR timing is ambiguous. Is "close after four weeks without a response" four weeks from the original request or from the reminder? Suggest "close two weeks after the reminder (four weeks total) if there's still no response". A
waiting-on-authorlabel would make these PRs easy to find.
Nits
- Lines 196 and 238 go past the ~100-column wrap used in the rest of the file.
- It could help to link this section from "Submitting a pull request" so contributors know what to expect, e.g. an acknowledgement within three business days and the inactivity policy.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Cedric Vidal (@cedricvidal) Addressed your feedback in 836ee5b. The policy now gives concrete Actions approval guidance, expands high-risk review to dependencies, shared-package breaking changes and data isolation, clarifies squash attribution and commit titles, and makes closure two weeks after the reminder (four weeks total waiting on the contributor). I also added the submission-section link and fixed line wrapping. For routing, I took your suggested follow-up approach: the doc explicitly names CODEOWNERS, team review assignment, code-owner protection and stale-approval dismissal as setup work, with a designated triage maintainer handling assignment in the meantime. The waiting-on-author label is also named as follow-up work. This PR does not configure those mechanisms or claim they are already enabled. |


Summary
Adds a community contribution review and merge process to
CONTRIBUTING.mdfor team agreement.The proposal covers ownership and a three-business-day acknowledgement target, CLA and readiness checks, one maintainer approval for routine changes and two for higher-risk changes, clear feedback, safe handling of external code, merge requirements, squash merging, and post-merge follow-up. It also proposes a reminder after two weeks of inactivity and closure after four weeks without a response.
Reviewers should confirm the approval thresholds, response targets, squash-merge policy, and inactivity timelines before adopting this process. This is documentation only; it does not change branch protections or CI configuration.
Demo
N/A. Documentation-only change.
Testing
git diff --checkpassed. No runtime tests were run because this change only updates documentation.Documentation and compatibility
Updates
CONTRIBUTING.mdwith the proposed team process. No API, CLI, or runtime compatibility changes.Checklist