diff --git a/.claude/skills/board-ops/SKILL.md b/.claude/skills/board-ops/SKILL.md index 02808f21fa..bb75edece9 100644 --- a/.claude/skills/board-ops/SKILL.md +++ b/.claude/skills/board-ops/SKILL.md @@ -299,8 +299,10 @@ fi # Then the two chained item-edit calls from "Add a card and set its fields". ``` -**Convert** it to an issue only after the advisory is **published**. The web UI -has "Convert to issue" on the card; the API equivalent is: +**Convert** it to an issue when its fix is ready to merge, keeping its opaque +title; a follow-up card converts only after the advisory is **published**. When +each happens, and what the issue gets, is the `/security-advisory` flow. The web +UI has "Convert to issue" on the card; the API equivalent is: ```sh REPO_ID=$(gh api repos/modelcontextprotocol/servers --jq .node_id) || REPO_ID= diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index 4f3fe0bc45..4138280cf4 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -539,8 +539,8 @@ added to the draft by hand: - **Every advisory this release fixes**, as `--advisory GHSA-xxxx-yyyy-zzzz`, once each: the `[GHSA-…]` cards on the board for the milestone's work - (`security-advisory`). Its fix shipped from a private fork with no public - issue, so its reporters are no issue's author. A draft advisory works. + (`security-advisory`). Its issue is converted from the card by a + maintainer, so its reporters are no issue's author. A draft advisory works. - **Every outside contributor credited on one of the milestone's issues**, as `--thank "@login (#PR), , for #ISSUE"`, once per line of the prototype paragraph. The preview lists each candidate (below). diff --git a/.claude/skills/security-advisory/SKILL.md b/.claude/skills/security-advisory/SKILL.md index f63f22317d..a591c51c4a 100644 --- a/.claude/skills/security-advisory/SKILL.md +++ b/.claude/skills/security-advisory/SKILL.md @@ -26,15 +26,18 @@ things safe to say in public about the backlog are **aggregate counts** (how many are in each state). A branch named after the bug, or a commit message that describes it, discloses it as surely as an issue would. -Related: `/board-ops` holds the board IDs and the draft-card recipes, and -`/issue-create` gives public tracking its labels and milestone **after -publication**, never merely after the release (step 6). - -⚠️ **The PR flow does not apply to the fix itself.** It needs a public issue -and a public PR against `v2/main`, which is the disclosure this flow exists to -delay. The fix is reviewed inside the private fork (step 4), and `AGENTS.md` -records that fork PR as an exception to its PR rules (no public issue link, no -labels, tracked by the draft card). +Related: `/board-ops` holds the board IDs and the draft-card recipes, +`/issue-create` gives the converted issue its labels and milestone (step 5), +and `/pr-flow` opens the public PR that brings the fix to `v2/main` (step 5). + +⚠️ **The fix is written and reviewed in the private fork, then merged through +an opaque public PR.** The fork's PR (step 4) links no public issue, carries no +labels and is tracked by the draft card; it is never merged. When the fix is +ready to merge, the card converts to an issue that keeps its opaque title, and +a public PR from the same opaque branch closes it (step 5). That PR follows +`/pr-flow` except that **nothing in it describes the vulnerability** and it +gets **no Copilot review**. `AGENTS.md` records both PRs as an exception to +its PR rules. ## The flow @@ -44,8 +47,8 @@ labels, tracked by the draft card). | 2 | **Verify the claim**: which server, and does the code belong to it or to an SDK? | Judgment | | 3 | Valid and ours → **accept** (`triage` → `draft`); otherwise close with a reason | **Human only** | | 4 | Create the **private fork**, fix and review there | Mechanical | -| 5 | Merge to `v2/main`, milestone merge, **release**, close the fork's PR, then **publish** | **Human only** (closing the fork's PR is mechanical) | -| 6 | **After publication**, convert the draft card into a public issue | Mechanical | +| 5 | Close to the release: **convert the card** to an opaque issue, open the opaque **public PR**, merge it, close the fork's PR; then milestone merge, **release**, **publish** | **Human only** for merging, releasing and publishing (converting, the PR and closing the fork's PR are mechanical) | +| 6 | **After publication**, give the issue its public title and the advisory link | Mechanical | There is **one release line**. Every fix lands on `v2/main` and ships through the next milestone merge to `main` and a release, as any other change does @@ -197,7 +200,8 @@ moves the advisory `triage` → `draft`. The API shows it as `state` and normally assigning a milestone, and a draft card cannot carry one, so for an advisory draft **accepting the advisory is the approval**. `AGENTS.md` records this as the advisory exemption to its `Incoming` ⇔ milestone rule. The milestone -arrives with the public issue in step 6, where the ordinary rule resumes. +arrives when the card converts to an issue in step 5, where the ordinary rule +resumes. **Invalid, out of scope, a duplicate, or an SDK's → close** with a comment saying which, and why; for an SDK's, say where it was routed. A reporter who is @@ -249,8 +253,10 @@ any other change: a regression test that fails before the fix, and the gate under **Before pushing**. Name the branch `v2/fix/` (lowercase, **no descriptive slug**, since a branch name outlives the fork once merged), and keep commit messages equally opaque until -publication. **The fork's PR targets `v2/main`**, never `main`: check its base -before anyone merges it. +publication. **The fork's PR targets `v2/main`**, never `main`, so the diff +reviewed there is the diff that lands. It is where the review happens; it is +**not merged**. The same branch is pushed to this repository and merged +through a public PR (step 5), and the fork's PR is then closed. ⚠️ **Move the card as the work moves.** `In Progress` when the fix is started, `In Review` when the fork's PR is open. The fork is invisible to everyone not on @@ -290,8 +296,10 @@ body nobody will read again. already accepted, and asking for the work approves it. Otherwise it goes in `Incoming`. Score its Priority with step 1's table, using the severity of the residual exposure rather than the advisory's. -- **Say in the fix PR's body what the fix leaves open**, and link the - follow-up. The advisory's text must say it too when it is published (step 5). +- **Say in the fork PR's body what the fix leaves open**, and name the + follow-up card. The public PR (step 5) says nothing about it, since + describing the gap describes the vulnerability. The advisory's text must say + it too when it is published (step 5). Create it with `/board-ops`'s draft-card recipe, using the follow-up title in place of `[$GHSA]`. @@ -308,35 +316,114 @@ of three cases: - It is non-security work outside the advisory's scope, which gets a public issue **after publication**, like any other finding. -### 5. Merge, release, publish (human only) +### 5. Bring the fix to `v2/main`, release, publish Publish **after** the fix has shipped in a release, never before: publishing discloses the vulnerability, and doing it while users have no upgrade hands out a working exploit. **Shipped** means the fixed version of that server is on npm or PyPI. -⚠️ **The patch stops being secret at MERGE, not at publish.** Merging the fork -puts an ordinary public commit on `v2/main`, readable by anyone, and it then -travels through the milestone merge to `main` before `release.yml` publishes it. -The time between merge and publish is not a period of secrecy; it is a period -of **exposure to anyone reading commits**, so keep it short: merge close to the -release, and publish as soon as the release is out. +⚠️ **The patch stops being secret when its branch reaches this repository, not +at publish.** Pushing the fork's branch here (below) makes its commits readable +by anyone, the public PR shows the diff, and the merge then travels through the +milestone merge to `main` before `release.yml` publishes it. The time between +that push and the publish is not a period of secrecy; it is a period of +**exposure to anyone reading commits**, so keep it short: start this step close +to the release, merge as soon as CI passes, and publish as soon as the release +is out. The release itself is a maintainer action (`release.yml` runs only when a maintainer publishes a GitHub Release, behind the `release` environment's approval; see [`RELEASING.md`](../../../RELEASING.md)). Say which server needs a release and stop there. -⚠️ **Publishing is irreversible and human-only.** It makes the advisory public, -with no undo. Same rule as accepting: recommend, never perform. +⚠️ **Merging, releasing and publishing are human-only.** Publishing is also +irreversible: it makes the advisory public, with no undo. Same rule as +accepting: recommend, never perform. Converting the card, opening the public +PR and closing the fork's PR are mechanical. + +#### Convert the card to an opaque issue + +When the fix's review in the fork is done and the release is near, convert the +draft card to an issue, so the public PR has an issue to close and a card that +tracks it. + +⚠️ **Convert the draft; do not file a new issue.** "Convert to issue" creates a +**new** issue from the draft card, and there is no way to point an existing card +at an issue filed separately, so filing by hand and then converting produces two +issues and two cards. + +1. **Keep the opaque title**, `[GHSA-xxxx-yyyy-zzzz]`, and the + link-and-triage body (step 1). The issue takes the card's title and body, + and the advisory is still private, so no summary may appear yet (step 6 + adds it). The advisory link in the body opens only for people on the + advisory. +2. **Convert the draft card to an issue** in `modelcontextprotocol/servers` + (the card keeps its place and its field values). The recipe is in + `/board-ops`. +3. Apply the labels and milestone from `/issue-create` (steps 1 and 2): `v2`, + one type label (normally `bug`), the server's scope label, and the milestone + of the release the fix will ship in. **Skip its board step**: the converted + card is already on #43. + +A **follow-up card stays a draft** here: its write-up describes the gap, so it +converts only after publication (step 6). + +#### Open the public PR + +Push the fork's branch to this repository under the **same opaque name**, +`v2/fix/`, and open the PR with `/pr-flow` steps 4 and 6. The fork has +no CI, so run the gate on exactly what you push: + +```sh +GHSA= +BRANCH="v2/fix/$(tr '[:upper:]' '[:lower:]' <<<"$GHSA")" +FORK=$(gh api repos/modelcontextprotocol/servers/security-advisories/$GHSA \ + --jq '.private_fork.full_name // empty') || FORK= +[ -n "$FORK" ] || echo "no private fork for $GHSA (or the lookup failed)" >&2 +WT="../$(basename "$PWD")-${BRANCH#v2/fix/}" +git fetch origin v2/main +# The push runs only after the gate exits 0, from the worktree holding the fork's head. +[ -n "$FORK" ] && git fetch "https://github.com/$FORK.git" "$BRANCH" \ + && git worktree add --detach "$WT" FETCH_HEAD \ + && cd "$WT" && npm install && npm run local:gate \ + && git push origin "HEAD:refs/heads/$BRANCH" +``` + +If the branch no longer merges cleanly onto `origin/v2/main`, resolve that in +the fork, where the review is, before pushing. + +The worktree is on a detached `HEAD`, so `gh pr create` cannot infer the +branch: add `--head "$BRANCH"` to `/pr-flow`'s create command. + +- **Title**: `fix(): GHSA-xxxx-yyyy-zzzz`, and nothing more. +- **Body**: `Closes #` (the converted issue) on the first line, then + `/pr-flow`'s sections, **describing no part of the vulnerability**: what it + is ("the fix for GHSA-xxxx-yyyy-zzzz, reviewed in the advisory's private + fork; details stay with the advisory until it is published"), the gate's + result, and each checklist item answered. Client evidence stays with the + advisory; say so under **How Has This Been Tested?** rather than recording + scenarios that describe the attack. +- Label it `v2`, link it with `addCloseIssueReferences` and read the link back + (`/pr-flow` step 6), and move the card to **In Review** if it is not there + already. +- ⚠️ **No Copilot review: skip `/pr-flow` steps 7 to 9.** Review comments on a + public PR are public, and a reviewer's findings would describe the + vulnerability. The review happened in the fork. Do not request one. + +#### Merge it, then close the issue + +A maintainer merges it once CI passes. Then close the issue by hand and move +its card to **Done** (`/pr-flow` step 10), as for any PR into `v2/main`: +`Done` means the work shipped, and it has. #### Close the fork's PR once its fix is on `v2/main` **GitHub will not publish an advisory while its private fork has an open pull request.** The advisory page says "you must merge or close outstanding pull requests" and **Publish advisory** stays disabled. The fix reaches `v2/main` -through a PR in this repository, so the fork's own PR is left open with -nothing still to merge, and blocks publication until someone closes it. +through the public PR above, so the fork's own PR is left open with nothing +still to merge, and blocks publication until someone closes it. Closing it is mechanical and reversible (a PR can be reopened), and it neither closes nor publishes the advisory. Close a PR **only once its head is on @@ -381,9 +468,10 @@ fork, so check each of these first: - **Nothing that matters lives only in the fork.** Publishing deletes it. Review findings worth keeping, a residual-exposure note, a follow-up's write-up: copy each onto the advisory (if it belongs in what is published) or - onto the draft card that tracks it (a follow-up's write-up goes in **its - follow-up card's** `## Write-up` section, where step 6 reads it), within step - 4's confidentiality limits, before the button is pressed. + onto the follow-up draft card that tracks it (a follow-up's write-up goes in + **its follow-up card's** `## Write-up` section, where step 6 reads it), within + step 4's confidentiality limits, before the button is pressed. Never onto the + converted issue, which is public. - **The affected product**: ecosystem (`npm` or `pip`) and package name, as published (`@modelcontextprotocol/server-filesystem`, `mcp-server-git`). @@ -402,33 +490,25 @@ fork, so check each of these first: platforms or cases are still exposed, so a user who upgrades does not believe they are fully covered. -### 6. After publication, convert the card into public tracking +### 6. After publication, give the issue its public title -**The trigger is publication, not the release.** The release ships the fix -while the advisory can still be private, and a public issue opened in that gap +**The trigger is publication, not the merge or the release.** Both happen while +the advisory can still be private, and a summary added to the issue in that gap describes a vulnerability the advisory has not disclosed yet. -⚠️ **Convert the draft; do not file a new issue.** "Convert to issue" creates a -**new** issue from the draft card, and there is no way to point an existing card -at an issue filed separately, so filing by hand and then converting produces two -issues and two cards. +1. **Retitle the issue** `[GHSA-xxxx-yyyy-zzzz] - `. Only + now, with the advisory public, may the summary appear. +2. **Replace the body's triage lines** with a link to the now-public advisory. -1. **Give the card its public title first**, `[GHSA-xxxx-yyyy-zzzz] - ` (edit the draft's title on the card), since the issue takes the - card's title. Only now, with the advisory public, may the summary appear. -2. **Convert the draft card to an issue** in `modelcontextprotocol/servers` - (the card keeps its place and its field values). The recipe is in - `/board-ops`. -3. Apply the labels and milestone from `/issue-create` (steps 1 and 2): `v2`, - one type label (normally `bug`), the server's scope label, and the milestone - of the release the fix shipped in. **Skip its board step**: the converted - card is already on #43. -4. Replace the body's triage lines with a link to the now-public advisory, and - **close** the issue: the work shipped before the issue existed. -5. Move the card to **`Done`**, which is right here because the fix shipped. +```sh +gh issue edit --repo modelcontextprotocol/servers \ + --title "[GHSA-xxxx-yyyy-zzzz] - " --body-file +``` + +The issue is already closed and its card in `Done` (step 5); neither changes. -**A follow-up card converts at the same time, but stays open**, because its -work has not shipped: +**A follow-up card converts now, and stays open**, because its work has not +shipped: 1. Retitle it `[GHSA-xxxx-yyyy-zzzz follow-up] - `. 2. Convert it. diff --git a/AGENTS.md b/AGENTS.md index 8fe94f01a7..a823456c1c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -359,15 +359,22 @@ every commit a pull request adds; one unsigned commit fails it. The repair work is invisible to the board. If there is no issue yet, create it first. This holds for a maintainer's own one-line fix as much as for a feature. -- **Exception: a security advisory's fix, in its private fork.** A PR in the - advisory's temporary private fork (`servers-ghsa-xxxx-yyyy-zzzz`) links no - public issue, carries no labels, and is tracked by the advisory's `[GHSA-…]` draft - card rather than an issue's card, because each of those would disclose the - vulnerability before a fix ships. For the same reason the work-start rules - on assigning an issue and naming the branch `v2//-` - do not apply: its branch is `v2/fix/`, with **no descriptive - slug**. It still targets `v2/main`, and it is reviewed inside the fork. The - flow is the `security-advisory` skill. +- **Exception: a security advisory's fix.** It is written and reviewed in a + PR in the advisory's temporary private fork (`servers-ghsa-xxxx-yyyy-zzzz`), + which links no public issue, carries no labels, and is tracked by the + advisory's `[GHSA-…]` draft card rather than an issue's card, because each + of those would disclose the vulnerability before a fix ships. For the same + reason the work-start rules on assigning an issue and naming the branch + `v2//-` do not apply: its branch is + `v2/fix/`, with **no descriptive slug**. It targets `v2/main`, and + it is **not merged**. When the fix is ready to merge, close to the release, + the draft card converts to an issue that keeps its opaque title, and that + branch is pushed here under the same name and merged through a **public PR** + that `Closes` that issue and follows every rule above, except that its + title and body describe no part of the vulnerability and it gets **no + Copilot review loop**: review comments on a public PR are public, and the + review already happened in the fork. The fork's PR is then closed. The flow + is the `security-advisory` skill. - **Exception: a milestone's merge PR.** It is the one PR that targets `main`. It is a pure merge of `v2/main` (its branch is `origin/v2/main` with no @@ -480,13 +487,16 @@ here. card** titled with the bare id, `[GHSA-xxxx-yyyy-zzzz]`, because a real issue would disclose the vulnerability before a fix exists. The title stays that opaque until publication: board access is wider than advisory access, - and a summary can name the server or the attack. It becomes a public - issue by conversion **once the advisory is published**, never merely once - the fix ships. Work the fix leaves open is tracked by a second draft card, - `[GHSA-xxxx-yyyy-zzzz follow-up]`, under the same rules. It stays open - after conversion, because its work has not shipped. The `[GHSA-` prefix is - load-bearing: **any other draft card is still a defect to delete**. The flow - is the `security-advisory` skill. + and a summary can name the server or the attack. It converts to an issue + **when its fix is ready to merge**, keeping that opaque title, so the + public PR that brings the fix to `v2/main` has an issue to close; the + summary joins the title only **once the advisory is published**, never + merely once the fix ships. Work the fix leaves open is tracked by a second + draft card, `[GHSA-xxxx-yyyy-zzzz follow-up]`, under the same rules, except + that it converts only once the advisory is published, since its write-up + describes the gap. It stays open after conversion, because its work has + not shipped. The `[GHSA-` prefix is load-bearing: **any other draft card + is still a defect to delete**. The flow is the `security-advisory` skill. **Accepting, closing and publishing an advisory, and replying to its reporter, are human-only; never automate or bulk-apply any of them.** - **Only issues go on the board, never PRs.** A PR is tracked through its linked @@ -511,8 +521,8 @@ here. approval. A `[GHSA-` advisory draft is exempt: a draft card cannot carry a milestone, so its approval act is **accepting the advisory**, which moves it - `Incoming` → `Todo`. Its milestone arrives with the public issue after - publication. + `Incoming` → `Todo`. Its milestone arrives when the card converts to an + issue, before the fix's public PR opens. - **`Done` means the work shipped**: its PR merged, or it is a parent whose last sub-issue closed. A duplicate, won't-fix, not-planned or superseded issue shipped nothing, so its card is **deleted** rather than moved to Done. @@ -573,7 +583,9 @@ here. so **link it explicitly** with the `addCloseIssueReferences` GraphQL mutation right after opening it. **On merge, close the issue by hand and move its card to Done.** Keep the `Closes` line anyway. -- **After opening a PR, run a Copilot review loop to convergence, unprompted.** +- **After opening a PR, run a Copilot review loop to convergence, unprompted** + (except a security advisory's public fix PR; see + [Contributing](#contributing)). Request a review, wait for the round to post or for Copilot's session to end without one, answer every comment, and request again only when you pushed a fix or the last round ended without a review. Stop on the **first** clean diff --git a/docs/ai-software-factory.md b/docs/ai-software-factory.md index e0d5356ead..85e46296ab 100644 --- a/docs/ai-software-factory.md +++ b/docs/ai-software-factory.md @@ -212,8 +212,9 @@ A privately reported vulnerability is tracked by a draft board card titled with the bare advisory id, `[GHSA-xxxx-yyyy-zzzz]`, because a public issue would disclose it before a fix exists. The `security-advisory` skill covers checking whether the code is this repository's or the SDK's underneath, the -private fork the fix is built in, and turning the card into public tracking -once the advisory is published. **Accepting, closing and publishing an +private fork the fix is built in, converting the card to an issue with the +same opaque title so an opaque public PR can bring the fix to `v2/main`, and +giving that issue its public title once the advisory is published. **Accepting, closing and publishing an advisory, and replying to its reporter, are human-only acts**, never automated or bulk-applied. diff --git a/scripts/release-notes.mjs b/scripts/release-notes.mjs index fe5eb39362..046473de83 100644 --- a/scripts/release-notes.mjs +++ b/scripts/release-notes.mjs @@ -38,11 +38,12 @@ // Two kinds of credit never come from an issue's author, and were once pasted // in by hand, which is how v2.0.0 published without its Thanks (#5250): // -// * An advisory's fix ships from its private fork with no public issue, so -// each `--advisory GHSA-…` is read for its credits (a draft works: notes are -// drafted before advisories publish). Declined credits, bots and -// maintainers are dropped; a credit still pending acceptance is listed and -// flagged, since the reporter may yet decline it. +// * An advisory's issue is converted from its board card by a maintainer, +// never filed by its reporter, so each `--advisory GHSA-…` is read for its +// credits (a draft works: notes are drafted before advisories publish). +// Declined credits, bots and maintainers are dropped; a credit still +// pending acceptance is listed and flagged, since the reporter may yet +// decline it. // * An outside PR is closed and its author credited on an issue (Harvest), so // each `--thank "@login (#ref), "` is a line of the prototype // paragraph. Which issues carry such a credit is a reading, not a lookup,