docs: add PR quality expectations and issue-first workflow#724
docs: add PR quality expectations and issue-first workflow#724
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: c588b702047d51d13da829fa ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
| 3. **Get agreement on the approach** before writing code. This includes the scope of the change, the implementation strategy, and any architectural considerations. | ||
| 4. **Submit your PR** with a clear description of the feature and links back to the discussion. | ||
|
|
||
| > [!WARNING] | ||
| > **Do not open a PR for a feature that hasn't been discussed and approved in an issue.** Even if the implementation looks correct, PRs without prior agreement will be closed. We know it can be tempting to jump straight to code, but aligning on the "what" and "why" first leads to better outcomes for everyone. | ||
|
|
||
| * Before submitting, please read the [branching strategy](#branching-strategy) and [code review guidelines](#code-review-guidelines). | ||
|
|
||
| ### Do you have questions about the code or about Apollo MCP Server itself? | ||
|
|
There was a problem hiding this comment.
🟡 The WARNING block in the new feature workflow says "discussed and approved in an issue", but Step 1 explicitly offers Community forums as an equivalent alternative path. A contributor who used the forums path would technically violate the WARNING. The WARNING should read "discussed and approved in a GitHub issue or Community forum thread" to match the two paths offered in Step 1.
Extended reasoning...
What the bug is: Step 1 of the new feature workflow offers two equivalent paths for initiating discussion: opening a GitHub issue with the enhancement label, or starting a discussion on the Apollo Community forums. However, the WARNING block immediately after Step 4 uses the phrase "discussed and approved in an issue", which specifically refers to GitHub issues only — not Community forum threads.
The specific code path: In the diff, the new ### Do you intend to add a new feature or change an existing one? section adds Step 1 as: "Open an issue describing the feature using the enhancement label, or start a discussion on the Apollo Community forums." The WARNING then states: *"Do not open a PR for a feature that hasn't been discussed and approved in an issue."*
Why existing code doesn't prevent it: The WARNING was written to enforce the issue-first workflow, but the author of the WARNING didn't account for the forums alternative introduced in Step 1 of the same section. The two pieces of text are contradictory as written.
Impact: A contributor who followed Step 1 via the Community forums path — in good faith, exactly as the documentation suggests — would then read the WARNING and either (a) believe their PR will be closed despite following the documented process, or (b) be confused about whether the WARNING applies to them. Step 2 adds further confusion by saying "A maintainer will respond on the issue when we're ready", implying forum discussions eventually need a GitHub issue, but this is never made explicit.
How to fix it: Change the WARNING from "discussed and approved in an issue" to "discussed and approved in a GitHub issue or Community forum thread". Alternatively, revise Step 1 to make GitHub issues the only required path (with forums as supplementary/optional) and update Step 2 accordingly.
Step-by-step proof:
- Contributor reads Step 1: "Open an issue ... or start a discussion on the Apollo Community forums." — they choose the forums path.
- Contributor posts on Apollo Community forums, gets positive community feedback but no maintainer response yet.
- Contributor implements the feature and opens a PR.
- Contributor reads the WARNING: "Do not open a PR for a feature that hasn't been discussed and approved in an issue."
- The contributor's feature was discussed on forums, not in a GitHub issue — they are technically in violation of the WARNING, despite following the documented Step 1 workflow.
|
|
||
| Excited about Apollo MCP Server and want to make it better? We're excited too! We welcome anyone who wants to contribute or provide constructive feedback, no matter their level of experience. | ||
|
|
||
| > [!IMPORTANT] | ||
| > **Please do not open a pull request before the team has had a chance to discuss the issue.** We appreciate the enthusiasm, but unsolicited PRs (especially for features or non-trivial changes) will be closed if there hasn't been prior agreement on the approach. The workflow is: **open an issue first, wait for a maintainer to respond, and only start coding once we've agreed on a path forward.** This saves everyone's time and keeps the project moving in the right direction. | ||
|
|
||
| ### PR quality expectations | ||
|
|
||
| Every pull request should reflect a genuine understanding of the problem and the codebase. We will close PRs that: | ||
|
|
||
| * Were submitted without a prior issue discussion or maintainer sign-off. | ||
| * Contain low-quality or AI-generated code that hasn't been reviewed, tested, or adapted to the project's conventions by the author. | ||
| * Make broad, speculative changes unrelated to a specific agreed-upon issue. | ||
|
|
||
| We're not against using AI tools to assist your work, but you are responsible for every line of code you submit. If you can't explain your changes, debug failures, or respond to review feedback, the PR isn't ready. **Authorship means accountability.** | ||
|
|
||
| ### Bug Reporting | ||
|
|
||
| > [!WARNING] |
There was a problem hiding this comment.
🟡 The PR quality expectations bullet ("Were submitted without a prior issue discussion or maintainer sign-off") reads as an unconditional rule with no exceptions, but the bug fix section explicitly allows small fixes under ~20 lines to be submitted directly without prior discussion. The IMPORTANT block softens its phrasing with "especially for features or non-trivial changes," but the quality expectations list has no such qualifier, creating contradictory guidance for contributors. Consider adding a parenthetical exception like "(except small, focused bug fixes under ~20 lines with tests)" to the PR quality expectations bullet, or explicitly noting the exception in the IMPORTANT block.
Extended reasoning...
The CONTRIBUTING.md introduces an IMPORTANT callout stating "Please do not open a pull request before the team has had a chance to discuss the issue," followed by a PR quality expectations section that lists "Were submitted without a prior issue discussion or maintainer sign-off" as a close reason. These two sections together present a unified message: all PRs need prior discussion. However, the bug fix section lower in the document explicitly carves out an exception: "Small, focused bug fixes (under ~20 lines of code) with accompanying tests can be submitted directly as a pull request."\n\nThe IMPORTANT block does include a softening qualifier — "especially for features or non-trivial changes" — which implies that small, trivial fixes might be exempt. However, this inference is indirect; a contributor reading only the top sections would not clearly understand that small bug fixes are exempt. More critically, the PR quality expectations bullet has no such qualifier at all. It reads as an absolute rule.\n\nA concrete example of the confusion: A contributor finds a one-line typo fix in an error message and writes a test for it. They read the IMPORTANT block and the PR quality expectations, conclude they must open an issue first, do so, wait for maintainer response, and only then submit. This is unnecessary friction for a trivially small fix. Alternatively, a contributor submits such a fix directly, and a maintainer who only remembers the quality expectations bullet may close it as "no prior issue discussion" — despite the bug fix section explicitly permitting this.\n\nThe fix is straightforward: add a parenthetical to the PR quality expectations bullet noting the small bug fix exception, e.g., change "Were submitted without a prior issue discussion or maintainer sign-off" to "Were submitted without a prior issue discussion or maintainer sign-off (exception: small bug fixes under ~20 lines with tests)." Alternatively, add a similar note to the IMPORTANT callout. The bug fix section already has the clearest and most actionable wording — the other sections just need to align with it.\n\nAll verifiers agreed this inconsistency is real. No refutations were raised. Severity is nit since the overall intent can be inferred from careful reading, but it is a genuine documentation inconsistency that would confuse first-time contributors.
Inspired by https://github.com/apollographql/apollo-client/blob/main/CONTRIBUTING.md.