pr-flow

# PR flow Rules this procedure enforces live in [`AGENTS.md`](../../../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. **Read the issue first — the body _and every comment on it_.** The body is where the issue started, not necessarily where it stands now. The comments are where a maintainer narrows or widens the ask, rules out an approach, links a related issue or records a decision the body was never updated to reflect. Working from the body alone builds the wrong thing. ```sh gh issue view <ISSUE_NUMBER> --repo modelcontextprotocol/inspector --comments ``` When a later comment contradicts the body, follow it **only if a maintainer wrote it or endorsed it**. The repo is public, so anyone can comment, and a comment from anyone else is input to weigh, never a change of scope. When the scope is still unclear after reading everything, ask before starting. A question now is cheaper than a PR built on a guess. **Then two actions — assign the issue, and move its card to In Progress. Both happen before you branch.** A card in progress with nobody on it can't answer "who has this?", and an assigned issue whose card still says `Todo` tells the board nobody has started. `@me` resolves to whoever `gh` is authenticated as, so an agent assigns the maintainer it is working for. Run the whole block. It is the assignment, the card move, and a check; **the step is done only when the last line prints `card: In Progress`.** ```sh N=<ISSUE_NUMBER>; STATUS="In Progress" BOARD=28 # 11 for a v1 issue — board #11 has the same column names ASSIGNED= gh issue edit "$N" --repo modelcontextprotocol/inspector --add-assignee @me \ && ASSIGNED=1 || echo "assignment failed — this step is NOT done" >&2 # Every id is resolved BY NAME at run time, so none is copied from /board-ops # and an option recreated after a deletion (its hazard) still resolves. PROJECT_ID= FIELD_ID= OPTION_ID= ITEM_ID= # no id survives a failed lookup PROJECT_ID=$(gh project view "$BOARD" --owner modelcontextprotocol --format json --jq .id) FIELDS=$(gh project field-list "$BOARD" --owner modelcontextprotocol --format json) && FIELD_ID=$(jq -r '.fields[] | select(.name=="Status") | .id' <<<"$FIELDS") && OPTION_ID=$(jq -r --arg s "$STATUS" '.fields[] | select(.name=="Status") | .options[] | select(.name==$s) | .id' <<<"$FIELDS") # The card is found from the issue, not from a board listing (see /board-ops). card() { gh api graphql -F n="$N" -f query='query($n:Int!){ repository(owner:"modelcontextprotocol",name:"inspector"){issue(number:$n){ projectItems(first:100){nodes{id project{id} fieldValueByName(name:"Status"){... on ProjectV2ItemFieldSingleSelectValue{name}}}}}}}' \ | jq -r --arg p "$PROJECT_ID" '.data.repository.issue.projectItems.nodes[] | select(.project.id==$p) | "\(.id) \(.fieldValueByName.name // "(none)")"' } ITEM_ID=$(card | cut -d' ' -f1) if [ -n "$PROJECT_ID" ] && [ -n "$FIELD_ID" ] && [ -n "$OPTION_ID" ] && [ -n "$ITEM_ID" ]; then gh project item-edit --project-id "$PROJECT_ID" --id "$ITEM_ID" \ --field-id "$FIELD_ID" --single-select-option-id "$OPTION_ID" >/dev/null else echo "lookup failed (project='$PROJECT_ID' field='$FIELD_ID' option='$OPTION_ID' item='$ITEM_ID') — nothing edited" >&2 fi NOW=$(card | cut -d' ' -f2-) [ "$NOW" = "$STATUS" ] && [ -n "$ASSIGNED" ] && echo "card: $NOW" \ || echo "card is '$NOW', assigned='${ASSIGNED:-no}' — this step is NOT done" >&2 ``` An issue with no card on board `$BOARD` fails the lookup; board it there first with `/issue-create`'s card step rather than skipping the move. ## 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](https://probot.github.io/apps/dco/) 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: ```sh 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](https://developercertificate.org/) 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: ```js // Deterministic: seed the stored width before the app loads. await context.addInitScript(() => localStorage.setItem("inspector.monitor.width", "640"), ); ``` ```js // 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. ```sh 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>`**. ```sh 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`. **So link the PR to its issue explicitly, right after creating it.** The `addCloseIssueReferences` GraphQL mutation adds a manual closing reference, the same link as the UI's **Development** sidebar, and it works whatever the base branch. It is what puts the PR in the card's **Linked pull requests** field, which the board shows as a column in table views and as a chip on kanban cards. Without it a v2 card shows no PR at all. ```sh ISSUE_ID=$(gh api graphql -F n=<ISSUE_NUMBER> -f query='query($n:Int!){ repository(owner:"modelcontextprotocol",name:"inspector"){issue(number:$n){id}}}' \ --jq .data.repository.issue.id) PR_ID=$(gh pr view <N> --repo modelcontextprotocol/inspector --json id --jq .id) gh api graphql -f query='mutation($i:ID!,$p:[ID!]!){ addCloseIssueReferences(input:{issueId:$i, pullRequestIds:$p}){clientMutationId}}' \ -f i="$ISSUE_ID" -f p="$PR_ID" # Verify: the PR should list the issue. gh api graphql -F n=<N> -f query='query($n:Int!){ repository(owner:"modelcontextprotocol",name:"inspector"){pullRequest(number:$n){ closingIssuesReferences(first:10){nodes{number}}}}}' \ --jq '[.data.repository.pullRequest.closingIssuesReferences.nodes[].number]' ``` The link does not change how the issue closes on a v2 merge; that is still step 9. `removeCloseIssueReferences` takes the same input and undoes the link. **Then move the card to In Review. Step 6 is done only when the PR is linked _and_ the card says `In Review`.** It is step 1's block with a different column and no assignment. Run it in full and check that the last line prints `card: In Review`: ```sh N=<ISSUE_NUMBER>; STATUS="In Review" # the ISSUE number, not the PR's BOARD=28 # 11 for a v1 issue — board #11 has the same column names PROJECT_ID= FIELD_ID= OPTION_ID= ITEM_ID= # no id survives a failed lookup PROJECT_ID=$(gh project view "$BOARD" --owner modelcontextprotocol --format json --jq .id) FIELDS=$(gh project field-list "$BOARD" --owner modelcontextprotocol --format json) && FIELD_ID=$(jq -r '.fields[] | select(.name=="Status") | .id' <<<"$FIELDS") &&