Skip to content

docs: define community contribution review and merge policy - #1413

Merged
Cedric Vidal (cedricvidal) merged 4 commits into
mainfrom
manekinekko-community-contribution-process
Sep 25, 2026
Merged

Cedric Vidal (cedricvidal) merged 4 commits into
mainfrom
manekinekko-community-contribution-process

Conversation

@manekinekko

Copy link
Copy Markdown
Member

Summary

Adds a community contribution review and merge process to CONTRIBUTING.md for 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 --check passed. No runtime tests were run because this change only updates documentation.

Documentation and compatibility

Updates CONTRIBUTING.md with the proposed team process. No API, CLI, or runtime compatibility changes.

Checklist

  • N/A: Portal feature parity.
  • N/A: Storybook component changes.
  • N/A: Database migrations.
  • N/A: Dependencies, lockfile, and third-party notices.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Medium severity · 1 Low severity

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.

Comment thread CONTRIBUTING.md Resolved
Comment thread CONTRIBUTING.md Resolved
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>

@cedricvidal Cedric Vidal (cedricvidal) left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.yml agentic workflows, and to package.json scripts/lifecycle hooks. Also: never add pull_request_target or secrets-bearing triggers to handle fork PRs.

  2. 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 CODEOWNERS file 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 CODEOWNERS that 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.

Smaller suggestions

  1. Extend the high-risk list. Consider adding new or upgraded dependencies (supply chain, plus the NOTICE regeneration the template already asks for) and breaking changes to packages/shared, since every app depends on it. Also consider changes to project scoping / data isolation. The by-id invariant in app-design.md is exactly the kind of thing a routine reviewer can miss.

  2. 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 keep Co-authored-by trailers for anyone else who pushed commits. Use a Conventional Commit title with the PR number (e.g. fix(docs): … (#1409))."

  3. 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-author label 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>
@manekinekko

Copy link
Copy Markdown
Member Author

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.

@manekinekko Wassim Chegham (manekinekko) added the type: documentation Documentation additions, corrections, and improvements. label Sep 24, 2026
@cedricvidal
Cedric Vidal (cedricvidal) merged commit d4db47c into main Sep 25, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: documentation Documentation additions, corrections, and improvements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants