pr-flow

# PR flow The rules this procedure carries out live in [`AGENTS.md`](../../../AGENTS.md) (**Contributing**, **Issue-driven work style**, **Responding to code reviews**, **Waiting on long-running work**). Board mechanics, including every board ID, are in `/board-ops`; creating a missing issue is `/issue-create`. This file names board columns and nothing else. **Pull requests against this repo are opened by the maintainers only.** Write access is not authorization to open one. Anyone else files a detailed issue (`/issue-create`) and a maintainer takes it from there. ## 1. Start from an issue **Every PR closes an issue.** A PR with no linked issue has no board card, so the work is invisible to the board. If there is no issue yet, create one first with `/issue-create`; never open the PR and backfill. **Read the issue in full, every comment included.** Scope changes, maintainer decisions and "not this, that" corrections are usually in the comments, not the body: ```sh gh issue view <N> --repo modelcontextprotocol/servers --comments ``` Then assign it to yourself and move its card to **In Progress** (the move-a-card recipe in `/board-ops`): ```sh gh issue edit <N> --repo modelcontextprotocol/servers --add-assignee @me ``` ## 2. Branch `v2/<type>/<N>-<slug>`, cut from **`origin/v2/main`**: ```sh git fetch origin v2/main git switch -c v2/fix/4810-filesystem-symlink-escape origin/v2/main ``` `<type>` follows the issue's type label: `bug` → `fix`, `enhancement` → `feat`, `documentation` → `docs`, `chore` → `chore`. A `question` has no branch type, because answering one ships nothing: if it turns out to need a change, relabel the issue with the type that change is, then branch. The `v2/` prefix matches the base branch and keeps the work legible in `git branch -a` next to `main`. **Never cut from `main`.** It is the release line and does not carry `v2/main`'s history, so the PR diff would show unrelated changes. **Stacking.** When one issue's work depends on another PR that has not merged yet, branch from that PR's branch and open the PR with `--base` set to it. Say in the body that it is stacked and on which PR. When the lower PR merges, retarget the stacked one to `v2/main` (`gh pr edit <PR> --base v2/main`) and rebase it if GitHub reports conflicts. ## 3. Sign off every commit (DCO) This repo adopted the [Developer Certificate of Origin](https://developercertificate.org/) (maintainer decision on #4861). Commit with **`git commit -s`**, every time. The rule is checked twice, by the same script (`scripts/verify-dco.mjs`): - **`npm run verify:dco`**, a stage of the pre-push gate (step 4), over `origin/v2/main..HEAD`: the commits you are about to push. Name another range with `-- --base <rev> --head <rev>` (a stacked branch's base, say). - **The `DCO signoff` job** (`.github/workflows/dco.yml`), on every pull request except into `main`, over the PR's `base.sha..head.sha`. What it checks: each commit carries a `Signed-off-by: Name <email>` trailer whose name **and** email match the commit's author or its committer. Merge commits and bot-authored commits are exempt. There is no partial credit: one unsigned commit out of six fails the whole check. The failure names each unsigned commit and prints the repair below. Two things that look like automation and are not: - ⚠️ **`git config format.signOff true` does nothing here.** Despite the name, it only defaults the `-s` flag for `git format-patch`. `git commit` never reads it, and there is no `commit.signoff` equivalent. - ⚠️ **A `prepare-commit-msg` hook works, but think before installing one.** The trailer is a certification, and a hook makes it on your behalf for _every_ commit, including work you merely cherry-picked. Inside that hook, `git var GIT_AUTHOR_IDENT` returns your config identity rather than the preserved author, so it cannot even tell it is signing for someone else. **Repairing commits already made** means rewriting them, onto the merge base the failure prints: ```sh git rebase --signoff <merge-base> git push --force-with-lease ``` `--signoff` signs each commit as you, the committer. Use `--force-with-lease` rather than `--force`, and rewrite only when you are the sole author and nobody has based work on the branch (a stacked PR above yours has). Nothing else clears the check: an empty "remediation" commit signs nothing that came before it, and the check reads every commit in the range. The signoff is a DCO assertion made in **your own name**. It does not claim you wrote the code, so signing off a cherry-pick is legitimate. Fabricating _someone else's_ certification never is. ## 4. Run the gate Format, then run the pre-push gate, and fix what fails before pushing: ```sh npm run format # TypeScript; in a Python server: uv run --frozen ruff format . npm run local:gate; echo "EXIT=$?" ``` `local:gate` runs every check CI runs, for both languages, whatever the change touched. Verify by the exit code, not by grepping the output. What each stage checks and how to fix a red one is `/pre-push-gate`; the stage list is [`docs/quality-gate.md`](../../../docs/quality-gate.md). One thing the gate deliberately leaves out: | Change touches | Also run | | --- | --- | | A skill's description, or a new skill | `npm run skills:eval` for the **whole** suite (spends model calls; not in the gate or CI). See `AGENTS.md` **Maintaining the skills** | ## 5. Client evidence A PR shows the change working, under the body's **How Has This Been Tested?** section (step 6). Here that means evidence from clients, not screenshots. **A change to server behavior** (a tool, resource, prompt, capability, transport or error it returns) records, for **both** the Inspector V2 and an LLM client, in **both** spec eras (a 2026-07-28 client and a 2025-11-25 client, per #4857): - what the client was asked to do (the Inspector action, or the prompt given to the LLM client), and - what it returned (the result, trimmed to what shows the change). A before/after pair is best when the change fixes a bug: the same request against `v2/main` and against the branch. **A change with no client-observable surface** (docs, skills, workflows, gate tooling, CI) carries a **targeted probe** instead: the one thing that proves it. For example, the guard made to fire on a planted defect and then pass, the workflow run that exercised the new job, the query that reads back the state a skill's recipe produced, or the `skills:eval` scores for a skill. Put the evidence in the PR body under **How Has This Been Tested?**. ## 6. Open the PR Base **`v2/main`** (or the lower branch, when stacked). **Never `main`.** Label it `v2`. The body's **first line is `Closes #<N>`**. The body carries these sections, in order. `.github/pull_request_template.md` is only the "issues, not PRs" banner that turns outside PRs away, so this list is where the structure lives: - **Description**: what changed and why. - **Server Details**: the server (or "none (repository-wide)") and what in it changed (tools, resources, prompts, docs, …). - **Motivation and Context**: the problem it solves; usually a pointer to the issue. - **How Has This Been Tested?**: the evidence from step 5. - **Breaking Changes**: whether users must change their client configuration. - **Types of changes**: tick each that applies: bug fix, new feature, breaking change, documentation update. - **Checklist**: answer every item. Tick each that holds, and tick and mark each that does not apply "(not applicable: <why>)"; leave none blank: - [ ] I have read the [MCP Protocol Documentation](https://modelcontextprotocol.io) - [ ] My changes follow MCP security best practices - [ ] I have updated the server's README accordingly - [ ] I have added a changeset (`npm run changeset`) if this changes what a TypeScript server publishes - [ ] I have tested this with an LLM client - [ ] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have documented all environment variables and configuration options - **Additional context** (optional): implementation notes or design decisions. Write the body to a file and pass it with `--body-file`. A body passed inline in double quotes goes through the shell, so every backtick in its Markdown runs as a command substitution and `$VAR` expands. Keep the file outside the worktree, where `git add -A` cannot pick it up. ```sh BODY=$(mktemp) cat > "$BODY" <<'EOF' Closes #<N> <the sections above, with the evidence> EOF gh pr create --repo modelcontextprotocol/servers \ --base v2/main --label v2 --title "<title>" --body-file "$BODY" ``` ⚠️ Closing keywords only link and auto-close for PRs that target the default branch (`main`). Against `v2/main`, `Closes #N` is only a cross-reference. Keep the line anyway, and **link the PR to the issue explicitly** right after opening it: ```sh PR=<PR_NUMBER>; N=<ISSUE_NUMBER> PR_ID=$(gh pr view "$PR" --repo modelcontextprotocol/servers --json id --jq .id) ISSUE_ID=$(gh issue view "$N" --repo modelcontextprotocol/servers --json id --jq .id) gh api graphql -f query=' mutation($issue:ID!,$pr:ID!) { addCloseIssueReferences(input:{issueId:$issue, pullRequestIds:[$pr]}) { issue { number } } }' -f issue="$ISSUE_ID" -f pr="$PR_ID" # Read the link back: the issue number must be listed. gh pr view "$PR" --repo modelcontextprotocol/servers \ --json closingIssuesReferences --jq '[.closingIssuesReferences[].number]' ``` The link does **not** make the issue close on merge into `v2/main`; that is still done by hand in step 9. Then move the card to **In Review** (`/board-ops`). The PR itself never goes on the board. ## 7. Request a Copilot review Only the GraphQL `requestReviews` mutation with the Copilot **bot id** works. REST, `gh pr edit --add-reviewer`, `userIds` and `copilot-swe-agent` all fail or silently drop the request. ```sh PR_ID=$(gh pr view <PR> --repo modelcontextprotocol/servers --json id --jq .id) gh api graphql -f query=' mutation($pr:ID!,$bot:[ID!]!) { requestReviews(input:{pullRequestId:$pr, botIds:$bot, union:true}) { pullRequest { id } } }' -f pr="$PR_ID" -f bot='BOT_kgDOCnlnWA' ``` A round usually lands in about five minutes. Wait for it with **one backgrounded loop that exits when the round lands, or when its deadline passes**, and wait for its notification rather than re-fetching once per turn. A review is remote state the harness cannot observe, which is the exception in `AGENTS.md` **Waiting on long-running work**, and this loop is where that poll belongs. ```sh PR=<PR> EXPECTED=1 # the review COUNT to reach; see below DEADLINE=$(( $(date +%s) + 20*60 )) while :; do # Capture first, so a gh failure stops the loop instead of being swallowed by # a pipeline. --slurp cannot be combined with --jq, hence the separate jq. raw=$(gh api --paginate --slurp \ repos/modelcontextprotocol/servers/pulls/$PR/reviews) || { echo "gh api failed ($?); not retrying blind" >&2; exit 1; } n=$(jq '[.[][] | select(.user.login | startswith("copilot-pull-request-reviewer"))] | length' <<<"$raw") || { echo "jq failed ($?) on an unexpected response shape" >&2; exit 1; } case $n in '' | *[!0-9]*) echo "not a count: '$n'" >&2; exit 1 ;; esac [ "$n" -ge "$EXPECTED" ] && { echo "round landed ($n reviews)"; break; } [ "$(date +%s)" -ge "$DEADLINE" ] && { echo "SILENT: no review by the deadline"; break; } sleep 30 done ``` `EXPECTED` is the review **count** to reach, so it is `1` only on the first round; on round two the first round's review is still there, and an existence check would return at once. Match the login with `startswith`: it carries a `[bot]` suffix. `sleep 30` is the remote-API floor `AGENTS.md` sets. **Every step that can fail exits the loop rather than retrying**: piping the count into `awk` would make an auth error read as `0`, and an empty count from a `jq` failure would make the comparison fail forever, so the job sleeps and retries without end. A background task that can never succeed is worse than none, because it looks like progress. Once the body lands, give the inline comments about another 60 seconds: they arrive after it (step 8). ## 8. Respond to the review Judge each finding against **the issue the PR closes**, as `AGENTS.md` **Responding to code reviews** says: fix defects in what the PR added; decline scope creep (pre-existing behavior, new capabilities, hardening the issue did not ask for) with a reason, and file an issue (`/issue-create`) for any that is worth doing on its own. Implementing a suggestion differently is fine. - **Reply to each review comment in its own thread**, saying what was done (with the commit) or why it was declined. That reply is the primary response, and it is not optional: each comment is a thread with its own resolve state, and a reply _in_ it is the only thing a reader of that thread sees. Replying does not resolve the thread; resolving is the reviewer's act. ```sh # The round's comments, by REVIEW id. The unpaginated /reviews listing hides # later rounds behind your own replies, and this endpoint returns 30 per page: # a round you only half fetch is a round you only half answer. gh api --paginate repos/modelcontextprotocol/servers/pulls/<PR>/reviews/<REVIEW_ID>/comments \ --jq '.[]|"\(.id) \(.path):\(.line)\n\(.body)\n"' # Reply into one thread, keyed by the comment id from above. gh api repos/modelcontextprotocol/servers/pulls/<PR>/comments/<COMMENT_ID>/replies \ -f body='Fixed in <sha>: …' ``` - ⚠️ **Then post a PR-level summary of the round, in addition, never instead.** Inline replies go hidden once the fix is pushed, because the threads become outdated, so the summary is what keeps the round readable afterwards. It does not replace the per-thread replies: a summary bullet cannot be connected back to the thread it answers. ```sh # Quoted heredoc, for the same reason as the PR body in step 6: Markdown # backticks inside a double-quoted --body run as commands. gh pr comment <PR> --repo modelcontextprotocol/servers --body-file - <<'EOF' Copilot round <k>: … EOF ``` Per-thread replies (`-f body=…` above) need the same care: single-quote the body, or read it from a file with `-F body=@<file>`. - ⚠️ **Read the "Suppressed comments" block in the review body.** Those findings have no comment id, so no thread to reply into. The PR-level summary is the only place to answer them. - ⚠️ **Inline comments lag the review body.** The body's "generated N comments" count lands first. Fetch by review id, and reconcile the count before answering. Push the fixes (signed off, step 3) before requesting the next round. ## 9. Another round, or stop Request another round (step 7) in one of two cases, and no other: - **You pushed a fix** in response to the last round. Raise `EXPECTED` by one. - **The last round was silent** (no review by the poll's deadline). Nothing was