PR flow

Rules this procedure enforces live in AGENTS.md (Issue-driven Work Style, Contributing). Board mechanics are in /board-ops; the gate itself is /pre-push-gate.

Pull requests against this repo are opened by the repo maintainers only. Having write access is not authorization to open one — anyone else files a detailed issue and a maintainer takes it from there.

1. Start from an issue

Every PR references an issue. No exceptions, regardless of who opens it. A PR with no linked issue has no board card, so the work is invisible to the project board and untracked. If there's no issue yet, create one first with /issue-create — don't open the PR and backfill.

Move the issue's card to In Progress (/board-ops).

2. Branch

Branch names start with the target version segment — the first path segment is the version whose base branch the PR targets, then the type, then a slug:

v2/fix/2071-oauth-resource-metadata
v2/chore/2146-rename-local-gate
v1/fix/proxy-ssrf-pin

Not fix/oauth-resource-metadata. This keeps the two lines legible in git branch -a and in the PR list once v1 and v2 branches coexist on the same remote, and it matches the base branches themselves (v2/main, v1/main).

Cut the branch from the base it will target — v2/main for v2 work, v1/main for v1. The two lines have unrelated histories, so a v1/fix/… branch cut from v2/main arrives at v1/main carrying the whole v2 tree (Copilot). And never cut from a milestone-merge branch — it carries release-only commits that will show up in your PR's diff.

Working in a git worktree is fine and often preferable. ⚠️ A worktree needs a real npm install, not a symlinked node_modules — a symlink passes lint/test/coverage and then fails all Storybook story files on Vite's fs.allow.

For order-dependent PRs on one issue, stack them: each branch is based on the previous one, not all cut from v2/main.

3. Sign off every commit

The DCO check is a hard merge gate. The probot DCO app fails the PR unless each commit carries a Signed-off-by: Name <email> trailer whose name and email match either the commit's author or its committer. Its only exemptions are merge commits and bot-authored commits; there is no partial credit — one unsigned commit out of six fails the whole check.

Prevent it with git commit -s. 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 already-pushed commits means rewriting them:

git rebase HEAD~<n> --signoff
git push --force-with-lease

Use --force-with-lease rather than --force, and only rewrite when you are the sole author and nobody else has based work on the branch. The two apparent alternatives are not alternatives: the app's empty "remediation commit" flow requires allowRemediationCommits.individual and this repo ships no .github/dco.yml, so it runs disabled; and the override button anyone with write access sees only silences the check without anyone certifying anything.

The signoff is a Developer Certificate of Origin assertion made in your own name. It does not claim you wrote the code, so signing off a cherry-pick is legitimate. What is never acceptable is fabricating someone else's certification.

4. Run the gate

npm run format, then npm run local:gate. See /pre-push-gate — npm run validate is not a substitute.

5. Screenshots, for any UI change

Any change to the web UI or the TUI must show its result: capture before/after screenshots (or a short GIF for an interaction) into a pr-screenshots/ folder off the repo root, creating it if it doesn't exist. That folder is gitignored — the images are working artifacts staged for upload, never committed — so attach them to the PR body from there rather than referencing an in-repo path. Name them for what they show (tools-tab-before.png), not Screenshot 2026-07-31 at 14.02.11.png.

5a. Capture settings — web

Everything in 5a is about a browser capture and assumes Playwright driving the web client. A TUI change has no viewport and no fullPage mode: size the terminal so no line wraps or truncates, and go straight to 5b, which applies to every image regardless of how it was taken.

Shoot the web client at 1280×900, full page. It is the one size already written down anywhere in the repo — scripts/smoke-web-tabs.mjs and scripts/smoke-web-elicitation.mjs set exactly that viewport (the other two web smokes set none) — and adopting it as the standard here is what makes a reviewer comparing two PRs compare the same thing. The older shots checked into specification/screenshots/ were taken at assorted sizes, which is the problem, not the precedent. Prefer a full-page shot over a Playwright clip region: a clip sized to one panel cuts off anything placed beside it, and two clips of different sizes make a before/after pair hard to read as a pair.

⚠️ Widening the window does not widen the Monitor sidebar. The main/sidebar split is a draggable divider whose width is stored independently of the viewport (localStorage["inspector.monitor.width"], default 420px, clamped to 320–720), so a bigger screen grows the content column and leaves the sidebar exactly as clipped as it was. Both levers have to be set, and only one of them is obvious. On #2234 this cost three full re-captures: the first set clipped the sidebar, the second still clipped it after only the window was widened, and the third worked once the divider itself was moved.

So when a shot includes the Monitor sidebar, set its width explicitly — give it enough room that no row truncates, favoring the sidebar over the left-hand list, which usually has room to give up. Two ways, in order of preference:

// Deterministic: seed the stored width before the app loads.
await context.addInitScript(() =>
  localStorage.setItem("inspector.monitor.width", "640"),
);

// Or drive the divider itself — it is a keyboard-operable ARIA separator,
// and ArrowLeft widens the sidebar one 16px step per press.
const handle = page.getByRole("separator", {
  name: "Resize monitoring sidebar",
});
await handle.focus();
for (let i = 0; i < 14; i++) await handle.press("ArrowLeft");

Two more mechanics worth setting before the shutter:

• Wait ~900ms after switching the main view. The Servers→Tools switch is a crossfade, so an immediate shot renders both views stacked translucently and reads as a broken app. Waiting on a locator in the incoming view is not enough — the outgoing one is still fading.
• Mark focus when the change is about focus. Tab order and keybinding fixes look identical at rest, so after driving the keystroke, page.evaluate over document.activeElement, outline it, and log its tag + aria-label — that line is the actual assertion and the image is the evidence. Say in the PR body that the outline is script-added, not app UI.

5b. Read the shot back before uploading — web and TUI

Open every image and confirm nothing is cut off at either edge — no truncated row, clipped badge, or value running under a panel border, and no half-faded view. This is a real check with your own eyes, not a formality: a clipped screenshot is worse than no screenshot, because a reviewer reads the truncation as a rendering bug in the feature under review and files it back at you. Re-shoot rather than shipping one that "mostly" shows the change.

5c. Upload

To host them, upload to GitHub's attachment endpoint with your gh token. Two mechanics, both of which bite:

• The parameters go in the query string, with the raw bytes as the body. A JSON body fails with a misleading "Invalid name for request".
• ⚠️ Do not put the token in argv. -H "Authorization: token $(gh auth token)" puts your credential in curl's command line, where any local user or process can read it off the process table while the upload runs (Copilot). Feed it through --config - instead: curl reads its options from stdin, so the token never becomes an argument.

printf 'header = "Authorization: token %s"\n' "$(gh auth token)" | curl -sS --config - \
  -X POST --data-binary @pr-screenshots/tools-tab-after.png \
  "https://uploads.github.com/user-attachments/assets?repository_id=<REPO_ID>&name=tools-tab-after.png&content_type=image/png"

(The token is still in the shell's environment and in printf's stdin, which is not world-readable the way /proc/<pid>/cmdline is.)

6. Open the PR

Target the base branch matching the work: v2/main for v2, v1/main for v1. Never main.

The body's first line is Closes #<ISSUE_NUMBER>.

gh pr create --repo modelcontextprotocol/inspector \
  --base v2/main --label v2 \
  --title "<title>" --body "Closes #<N>

<what changed and why>"

⚠️ Closing keywords only auto-link and auto-close for PRs targeting the repo's default branch (main). Because v2 PRs target v2/main, Closes #N there is only a cross-reference — it will not create a hard link or close the issue on merge. Keep it anyway, so the issues close if/when v2/main reaches main. There is no gh flag for manual linking; closing keywords are the only mechanism GitHub exposes.

Move the card to In Review.

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.

PR_ID=$(gh pr view <N> --repo modelcontextprotocol/inspector --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'

Poll for the review with a startswith match — the review login carries a [bot] suffix. Put that poll in one backgrounded loop that exits when the round lands, and wait for its notification rather than re-fetching once per turn; a review is remote state the harness cannot observe, which is exactly the exception described in Waiting on long-running work — and exactly where the poll belongs when one is needed.

EXPECTED=1   # the review COUNT you are waiting to reach — see below
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/inspector/pulls/<N>/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" ] && break
  sleep 30
done

EXPECTED is the review count you are waiting 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 returns immediately. sleep 30 is the remote-API floor the rule above sets. Every step that can fail exits the loop rather than retrying. Piping the count straight into awk would make an auth or API error read as a count of 0; and a jq failure on an unexpected shape leaves n empty, whereupon [ "" -ge 1 ] exits non-zero, break never fires, and the job sleeps and retries forever — the same unbounded wait, reached from the other end. A background task that can never succeed is worse than one that never started, because it looks like progress. Give the inline comments a further ~60s after the body lands; they arrive late (see step 8).

8. Respond to the review

• It is not necessary to implement every suggestion. Implementing one a different way, or declining it with a reason, is fine.

• After making the changes, reply to each review comment in its own thread with what was done, or why it was declined. That inline reply is the primary response and it is not optional — each review comment is a discussion thread with its own resolve state, and a reply in the thread is the only thing a reviewer reading that thread sees. It does not resolve the thread: resolving is a separate act — the "Resolve conversation" button, or the resolveReviewThread GraphQL mutation — and it is the reviewer's to make. The reply is what makes resolving it defensible.

  # Fetch the round's comments by REVIEW id — the unpaginated /reviews listing
  # hides later rounds behind your own replies.
  # --paginate: this endpoint returns 30 per page, and a round you only half
  # fetch is a round you only half answer.
  gh api --paginate repos/modelcontextprotocol/inspector/pulls/<N>/reviews/<REVIEW_ID>/comments \
    --jq '.[]|"\(.id) \(.path):\(.line)\n\(.body)"'
  
  # Reply into one thread, keyed by the comment id from above.
  gh api repos/modelcontextprotocol/inspector/pulls/<N>/comments/<COMMENT_ID>/replies \
    -f body='Fixed in <sha> — …'
  

• ⚠️ Then mirror the round at PR level, in addition — never instead. Inline replies go hidden once the fix is pushed, because the threads become outdated, so a summary comment is what keeps the round readable afterwards. It does not discharge the per-comment replies: a rollup bullet cannot be connected back to the thread it answers, so the thread stays open with a finding and silence in it, and by round three matching bullets to comments is reconstruction rather than reading.

• ⚠️ Always read the "Suppressed comments" block in the review body. Those findings have no comment id, so they have no thread to reply into — the PR-level mirror is the only place they can be answered, and it is the one case where answering there is the whole response.

• ⚠️ Copilot's inline comments lag its review body. The body's "generated N comments" count lands first; fetch by recency and reconcile. Repeated re-review silence means the session ended.

9. Merge and close out

On merge of a v2 PR, manually close its issue and move the board item to Done, since auto-close won't fire on v2/main. Use the move-a-card recipe in /board-ops — the option IDs are unstable, so this file names the column and nothing else.

Done is only for work that shipped — a merged PR, or a parent whose last sub-issue closed. Anything else (duplicate, won't fix, not planned, obsolete) means nothing shipped, so delete the card instead; see /board-ops.