docs: [No QA] Document MelvinBot workflow for contributors and C+#93066
docs: [No QA] Document MelvinBot workflow for contributors and C+#93066neil-marcellini wants to merge 7 commits into
Conversation
Adds a dedicated guide for proposal review, C+ PR ownership, and CME merge on MelvinBot issues, and links it prominently from CONTRIBUTING.md. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@ShridharGoel Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Co-authored-by: Cursor <cursoragent@cursor.com>
Fixes spellcheck failure on the Melv acronym in CONTRIBUTING.md. Co-authored-by: Cursor <cursoragent@cursor.com>
C+ who cannot edit Melvin-authored PR descriptions should post the full body or checklist in a details block and ask Melvin to apply it; CMEs may review that comment directly. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Reorder Phase 1 so C+ recommends via 🎀👀🎀, CME approves, then C+ asks Melvin to implement—not immediately after C+ review. Co-authored-by: Cursor <cursoragent@cursor.com>
| @@ -153,7 +156,8 @@ This helps future investigators understand the history and current status of err | |||
|
|
|||
| ### Propose a solution for the job | |||
| 4. Proposals must only be posted after the `Help Wanted` label is added. Any proposals submitted beforehand will be ignored and not reviewed. Do not post proposals in Slack. | |||
There was a problem hiding this comment.
Should we clarify "Proposals by contributors...." ? Because Melvin will post before Help Wanted label is applied.
There was a problem hiding this comment.
I think it's okay that Melvin follows different rules. This guide is aimed at contributors.
There was a problem hiding this comment.
Got it, I was just thinking what if some contributor cites this and asks why a proposal (Melvin's) posted before the "Help Wanted" label is being considered.
| → C+ recommends acceptance (🎀👀🎀) → CME approves proposal | ||
| → C+ asks Melvin to implement (only after CME approval) | ||
| → Not accepted: C+ or CME explains why | ||
| → Melvin opens draft PR → C+ tweaks, tests, posts PR body/checklist in a PR comment, asks Melvin to apply it, self-reviews (including Reviewer checklist) |
There was a problem hiding this comment.
Is C+ now expected to add the videos in both the author and reviewer checklists? Earlier we were adding only in the reviewer checklist
| Once the assigned CME has approved Melvin's proposal (the same acceptance step as for any contributor proposal), the C+ comments on the issue asking Melvin to implement, for example: | ||
|
|
||
| ``` | ||
| @MelvinBot please implement your proposal |
There was a problem hiding this comment.
Should we also add the link of the proposal, to ensure that Melvin only uses it's own proposal (and the latest one - since sometimes it posts a new proposal when asked to update) ?
|
|
||
| Before requesting final review, the assigned C+ must: | ||
|
|
||
| 1. **Manually tweak the PR** if Melvin's implementation needs corrections. |
There was a problem hiding this comment.
Worth mentioning that we'll need to ask by tagging @MelvinBot and explaining the needed changes.
| ## Why this process exists | ||
|
|
||
| Melvin can open pull requests before a human has validated the approach. Without a clear process, contributors waste time on unapproved work, and internal engineers may review PRs that should not have been started. Proposal review must happen **before** implementation. |
There was a problem hiding this comment.
This seems unnecessary bloating the guide IMO, the first paragraph coves a lot of this
| C+ members who cannot edit the PR body directly should use this workaround: | ||
|
|
||
| 1. On the **pull request**, post a comment with the exact content you want in the PR description. Put the full body inside a `<details>` block so the PR thread stays readable. Include the complete [PR template](https://github.com/Expensify/App/blob/main/.github/PULL_REQUEST_TEMPLATE.md) when possible: Explanation of Change, Fixed Issues, Tests, Offline tests, QA Steps, a fully checked PR Author Checklist, and Screenshots/Videos sections as needed. | ||
| 2. In the same comment, ask Melvin to copy that content into the pull request body, for example: |
There was a problem hiding this comment.
No we don't want contributors to workaround Melvin's permissions like this. Leaving the comment is fine
| ## Quick reference: what not to do | ||
|
|
||
| | Do not | Why | | ||
| |--------|-----| | ||
| | Open a PR before a proposal is accepted | Wastes contributor and reviewer time if the approach is wrong | | ||
| | Ask `@MelvinBot` to implement before the CME approves the proposal | Implementation must wait for CME approval, not just C+ recommendation | | ||
| | Skip C+ proposal review because Melvin wrote it | Melvin proposals need the same validation as human proposals | | ||
| | Send Melvin's draft PR for CME review without testing and checklist completion | The C+ is the human author and owns quality | | ||
| | Assume you can edit Melvin's PR description in the GitHub UI | Post the body in a PR comment and ask `@MelvinBot` to apply it | | ||
| | Expect a second C+ or contributor review after C+ submission | Only the CME reviews before merge | |
There was a problem hiding this comment.
Unnecessary bloating the guide IMO
3 participants
Explanation of Change
(Neil's AI agent)
Adds
contributingGuides/HOW_TO_WORK_WITH_MELVINBOT.mddocumenting how contributors, C+, and CMEs work with MelvinBot on App issues: proposal review before implementation, C+ PR ownership after Melvin implements, and CME review/merge. Links the guide prominently fromCONTRIBUTING.mdandHOW_TO_BECOME_A_CONTRIBUTOR_PLUS.md.Fixed Issues
$ #93064
PROPOSAL:
Tests
N/A
Offline tests
N/A — documentation only
QA Steps
N/A — documentation only
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A
Android: Native
N/A — documentation only
Android: mWeb Chrome
N/A — documentation only
iOS: Native
N/A — documentation only
iOS: mWeb Safari
N/A — documentation only
MacOS: Chrome / Safari
N/A — documentation only