From 8dee4cf08ec3f90e7b66b2b96788d90798db2b7c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 18 Sep 2026 20:13:09 -0400 Subject: [PATCH 1/6] docs: add the security-advisory skill and carve advisory drafts out of the board audit (#2443) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A privately reported vulnerability arrives through GitHub's advisory flow, never as an issue, and none of that procedure was written down. Sixteen advisories sit in `triage` today with hand-made draft cards on board #28. - New `security-advisory` skill: the draft card and its `[GHSA-` title prefix; verifying WHO OWNS THE CODE PATH before assessing severity (#2409 was a loopback/HTTPS finding that lived in `@modelcontextprotocol/client`, not here, and was withdrawn); accepting vs closing; the private fork; publishing; the public issue afterwards. Accepting and publishing are marked human-gated — both are outward-facing and publishing is irreversible. - Records the API facts that are easy to get wrong: the private-fork POST CREATES a fork rather than probing for one (read `.private_fork` first), there is no comment API for advisories in REST or GraphQL so reporter coordination is UI-only, deleting a private fork needs `delete_repo`, and the board step cannot be automated — no `PROJECT_TOKEN` exists in this org and `GITHUB_TOKEN` cannot hold `organization projects: write`. - AGENTS.md: carve advisory drafts out of "no draft cards" as the single exception, and add the skill to the index. Refresh the listing-budget figure. - issue-triage: the board audit's `non-Issue on a board` check counted all 16 drafts and would have stayed permanently non-zero. It now excludes drafts by TITLE PREFIX only, so a stray draft is still reported — and by title, since a draft has no issue number to print. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/issue-triage/SKILL.md | 20 ++- .claude/skills/security-advisory/SKILL.md | 161 ++++++++++++++++++ .../skills/security-advisory/evals/evals.json | 34 ++++ AGENTS.md | 8 +- 4 files changed, 218 insertions(+), 5 deletions(-) create mode 100644 .claude/skills/security-advisory/SKILL.md create mode 100644 .claude/skills/security-advisory/evals/evals.json diff --git a/.claude/skills/issue-triage/SKILL.md b/.claude/skills/issue-triage/SKILL.md index 0511119e1..bdff1d00d 100644 --- a/.claude/skills/issue-triage/SKILL.md +++ b/.claude/skills/issue-triage/SKILL.md @@ -200,7 +200,7 @@ count means the board contradicts a rule, not that the rule needs revisiting. | Check | Invariant | Fix | | --- | --- | --- | | Double-boarded | An issue has a card on **one** board, the one matching its version label | Delete the wrong-board card | -| Non-Issue items | **Only issues go on a board** — never PRs, never drafts | Delete the item | +| Non-Issue items | **Only issues go on a board** — never PRs, never drafts, *except* an advisory draft titled `[GHSA-…]` | Delete the item | | No Status | Every card carries a Status | Set one — `Incoming` if unmilestoned, else by where it actually is | | `Incoming` **with** a milestone (#28) | Incoming ⇔ no milestone | Approval was never recorded: move to **Todo**, or clear the milestone | | Past Incoming **without** a milestone (#28) | Everything past Incoming ⇔ milestoned | Claims an approval nobody made: milestone it, or move back to Incoming | @@ -235,7 +235,12 @@ jq -nr --slurpfile o "$D/i.json" --slurpfile a "$D/b28.json" --slurpfile b "$D/b | [own($b)[] | select(.content.type=="Issue") | {n:.content.number, s:.status}] as $B11 | { "double-boarded": [$B28[].n | select(. as $n | [$B11[].n]|index($n))], - "non-Issue on a board": [(own($a)[], own($b)[]) | select(.content.type!="Issue") | .content.number], + # An advisory draft card is the ONE legitimate non-Issue item (see AGENTS.md). + # It is identified by its `[GHSA-` title prefix and nothing else, so a stray + # draft is still reported. Reports the TITLE, since a draft has no number. + "non-Issue on a board": [(own($a)[], own($b)[]) | select(.content.type!="Issue") + | (.content.title // "(untitled)") + | select(startswith("[GHSA-") | not)], "no Status": [($B28[], $B11[]) | select(.s==null) | .n], "Incoming w/ milestone": [$B28[] | select(.s=="Incoming" and ms(.n)!=null) | .n], "past Incoming, no ms": [$B28[] | select(.s!=null and .s!="Incoming" and .s!="Done" @@ -286,6 +291,17 @@ Two things the queries must account for, both learned the hard way: and that check then reports `0` while the invariant it states (no drafts) is being violated (Copilot). The filter admits an item with no repository and excludes only cards that name a *different* one. +- **Advisory drafts are carved out of that check by TITLE, not by type.** A + GitHub security advisory is private until it is published, so it is tracked by + a draft card titled `[GHSA-xxxx-yyyy-zzzz] - …` — the one exception `AGENTS.md` + grants to "no draft cards", and the `security-advisory` skill is the flow. There + are enough of them open at any time that counting them would pin this check + permanently non-zero, and a check that never prints `0` stops being read at + all. The discriminator is deliberately the **title prefix** and nothing + broader: exempting *all* drafts, or every card whose Status is `Incoming`, + would let an ordinary stray draft through, which is the defect the check + exists for. So a draft titled anything else is still reported — by title, + since a draft has no issue number to print. - **`$M` holds closed issues too** — the lookup is built from `gh issue list --state all`, which it has to be, because the last check reads closed issues' state reasons. So `isopen` is not there to cope with a missing diff --git a/.claude/skills/security-advisory/SKILL.md b/.claude/skills/security-advisory/SKILL.md new file mode 100644 index 000000000..70df8fba5 --- /dev/null +++ b/.claude/skills/security-advisory/SKILL.md @@ -0,0 +1,161 @@ +--- +name: security-advisory +description: "Take a privately reported vulnerability through this repo's security advisory flow — board it, verify who owns the code path, accept or reject, fix it in the private fork, publish, then file the public issue. Use when a vulnerability is reported privately; when deciding whether an advisory is ours to fix; when looking up or creating its private fork; when answering a reporter; or when a GHSA-titled board card needs handling." +disable-model-invocation: false +--- + +# Handling a security advisory + +Private vulnerability reporting is enabled on this repo and +[`SECURITY.md`](../../../SECURITY.md) routes every report to it — the issue +chooser deliberately has no security template, because a vulnerability report +must not open a public issue. So an advisory never arrives as an issue, and for +most of its life it must **not** become one. + +Two steps in this flow are **outward-facing and irreversible-ish, and both stay +human-gated**: **accepting** an advisory (the reporter sees it) and +**publishing** it (it becomes public, assigns a CVE, and credits the reporter — +there is no unpublish). Never automate either, never bulk-apply them, and never +take either step because a checklist said to. Everything else here is mechanics. + +Related: `/board-ops` (the card IDs and recipes), `/issue-create` and +`/pr-flow` for the public issue and the eventual release. + +## The flow + +| # | Step | Gate | +| --- | --- | --- | +| 1 | Advisory lands in state `triage` → **draft card** on board #28 | Mechanical | +| 2 | **Verify the claim — including who owns the code path** | Judgment | +| 3 | Valid → **accept** (`triage` → `draft`); invalid → close with a reason | **Human only** | +| 4 | Create the **private fork**, fix and review there | Mechanical | +| 5 | Merge, release, then **publish** the advisory | **Human only** | +| 6 | After the release, file the public (closed) issue and convert the card | Mechanical | + +### 1. Board it as a draft card + +An advisory is private, so a public issue tracking it would disclose it before a +fix exists. It therefore gets a **draft card** — the one documented exception to +[`AGENTS.md`](../../../AGENTS.md#issue-driven-work-style)'s "every board item is +a real GitHub issue". + +- **Title:** `[GHSA-xxxx-yyyy-zzzz] - `. That `[GHSA-` prefix + is not cosmetic: the board audit in `/issue-triage` keys its draft carve-out + on it, so a card titled any other way is reported as a stray draft. +- **Body:** `**Advisory:** ` on the first line, then severity and + reported date, then the advisory description. The link first, because a + maintainer reading the card has no other route back to the private advisory. +- **Status `Incoming`**, plus a Priority scored with the `/issue-triage` rubric. + `Incoming` is correct even though somebody clearly triaged it to make the + card: nobody has approved shipping a fix yet, and a draft card has no + milestone to carry the approval. + +The card is made **by hand**. There is no `PROJECT_TOKEN` in this org and +`organization projects: write` is a permission `GITHUB_TOKEN` structurally +cannot hold, so a board write is unreachable from Actions — the same constraint +`AGENTS.md` records for the dependency sweeps. **Do not propose a nightly +workflow for this;** that approach was tried and abandoned for exactly this +reason. + +```sh +gh api repos/modelcontextprotocol/inspector/security-advisories \ + --jq '.[] | select(.state=="triage") + | "\(.ghsa_id)\t\(.severity)\t\(.summary)"' +``` + +### 2. Verify the claim — and who owns the code path + +Before assessing severity, establish that the vulnerable code is **ours**. A +report can be entirely accurate about behavior the Inspector merely exhibits +because an SDK does it. + +⚠️ **This is not hypothetical.** #2409 — a loopback/HTTPS-exemption finding — +read as an Inspector defect and turned out to live in +`@modelcontextprotocol/client` (`typescript-sdk#2591`). The reporter withdrew +it. Had ownership been checked after the severity assessment rather than before, +the fix would have been written against the wrong repo. + +So: reproduce it, find the code, and check whether that code is first-party or +reached through a dependency. An advisory against upstream code is closed here +with a pointer to the upstream issue — it is not ours to accept or publish. + +### 3. Accept, or close + +**Valid and ours → accept.** In the UI this is "Accept and open as draft"; it +moves the advisory `triage` → `draft`. The state is readable as `state` and +`submission.accepted` on the API object. + +**Invalid, out of scope, or upstream → close** with a comment saying which, and +why. A reporter who is told nothing reasonably assumes they were ignored. + +⚠️ **Accepting is a human act, always.** It is visible to the reporter and it +commits this project to treating the report as a real vulnerability. Nothing in +this skill authorizes taking it — surface the recommendation and let a +maintainer click. + +⚠️ **There is no comment API for security advisories.** Not in REST (the +advisory object exposes no comments endpoint) and not in GraphQL +(`RepositoryAdvisory` is not commentable, and no advisory-comment mutation +exists). Comments are **UI-only**, so every exchange with a reporter is manual — +you cannot script the reply, and you cannot read the thread back with `gh`. + +### 4. The private fork + +Accepted advisories are fixed in a **private fork** GitHub creates for the +advisory: a private repo named `-` in the org. + +⚠️ **Read `private_fork` FIRST. The POST is not a probe — it CREATES one.** +Calling it to "check whether a fork exists" makes one, in the org, which then +needs cleaning up. This was learned the hard way. + +```sh +# Idempotency check — does one already exist? +gh api repos/modelcontextprotocol/inspector/security-advisories/ \ + --jq '.private_fork // "none"' + +# Only if that printed "none": +gh api -X POST \ + repos/modelcontextprotocol/inspector/security-advisories//forks +# → 202 Accepted; the fork appears shortly afterwards. +``` + +⚠️ **Deleting a private fork needs the `delete_repo` OAuth scope, which a +default `gh` token does not carry.** So a fork created by mistake is not +something you can quietly undo — it takes a re-scoped token or an admin in the +UI. That asymmetry is the whole reason for the read-first rule above. + +Fix and review inside the fork. Its PRs and commits are private, so none of the +normal public review flow applies; the diff comes back to `v2/main` as an +ordinary commit at merge time. + +### 5. Merge, release, publish + +Publish **after** the fix has shipped in a release, never before — publishing +discloses the vulnerability, so doing it while users have no upgrade available +hands out a working exploit. + +⚠️ **Publishing is irreversible and human-gated.** It makes the advisory public, +requests a **CVE**, and credits the reporter. There is no undo. Same rule as +accepting: recommend, never perform. + +### 6. File the public issue afterwards + +Once the advisory is published, the work becomes ordinary board history: file a +public issue recording what shipped, **close it** (the work is already done), +and convert the draft card to that issue so the board stops carrying a draft. +Label and milestone it per `/issue-create`; `Done` is correct here, because the +fix genuinely shipped. + +## API facts worth not re-deriving + +All verified against the live API. + +| Thing | Fact | +| --- | --- | +| States | `triage` → `draft` (accepted) → `published`; or `closed` | +| Accepted? | `submission.accepted` on the advisory object, alongside `state` | +| Private fork | `POST …/security-advisories/{ghsa_id}/forks` → `202`, private repo `-` in the org | +| Fork idempotency | Read `.private_fork` first — the POST creates, it does not probe | +| Fork deletion | Needs the `delete_repo` OAuth scope; a default `gh` token lacks it | +| Comments | **No API at all**, REST or GraphQL. UI-only | +| Board writes | Not automatable — no `PROJECT_TOKEN`, and `GITHUB_TOKEN` cannot hold `organization projects: write` | diff --git a/.claude/skills/security-advisory/evals/evals.json b/.claude/skills/security-advisory/evals/evals.json new file mode 100644 index 000000000..06743379e --- /dev/null +++ b/.claude/skills/security-advisory/evals/evals.json @@ -0,0 +1,34 @@ +[ + { + "prompt": "Someone just reported a vulnerability against this repo privately. What do I do with it?", + "expect": "security-advisory" + }, + { + "prompt": "How do I fix a privately reported vulnerability without the patch being visible before the release goes out?", + "expect": "security-advisory" + }, + { + "prompt": "How do I reply to a reporter who filed a vulnerability privately?", + "expect": "security-advisory" + }, + { + "prompt": "A privately reported vulnerability turns out to be in an upstream package rather than our own code. How do I close it out?", + "expect": "security-advisory" + }, + { + "prompt": "When is it safe to make a privately reported vulnerability public, and who makes that call here?", + "expect": "security-advisory" + }, + { + "prompt": "Walk me through taking an accepted vulnerability report all the way to a shipped, disclosed fix.", + "expect": "security-advisory" + }, + { + "prompt": "What is the capital of Portugal?", + "expect": null + }, + { + "prompt": "Rename the local variable `tmp` to `buffer` in this snippet: `const tmp = 1; return tmp + 1;`", + "expect": null + } +] diff --git a/AGENTS.md b/AGENTS.md index 3a4bf713d..5fc98cae0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,6 +25,7 @@ users invoke them by name. | [`pr-flow`](.claude/skills/pr-flow/SKILL.md) | Branch naming, DCO signoff, screenshots, opening the PR, requesting a Copilot review, responding, closing out | Model-invoked, or `/pr-flow` | | [`pre-push-gate`](.claude/skills/pre-push-gate/SKILL.md) | Running `npm run local:gate` and diagnosing a failing stage | Model-invoked, or `/pre-push-gate` | | [`release`](.claude/skills/release/SKILL.md) | Cutting a release: bump on `v2/main`, milestone merge, tag `origin/main`, publish | `/release` | +| [`security-advisory`](.claude/skills/security-advisory/SKILL.md) | A privately reported vulnerability end to end: the draft card, verifying who owns the code path, accepting, the private fork, publishing, the public issue afterwards | Model-invoked, or `/security-advisory` | | [`test-servers`](.claude/skills/test-servers/SKILL.md) | Picking and running a showcase test server; the stale-build hazard | Model-invoked, or `/test-servers` | Longer-form human documentation lives in [`docs/`](./docs) — see the table in the @@ -255,7 +256,7 @@ node/field/option IDs, and the option-deletion hazard` was cut at `#28`, so 90 skill at all: it is absent from the listing and the Skill tool refuses it. The costs are asymmetric — a spurious load costs ~250 characters, a missed one costs a wrong base branch or an unsigned commit — and the budget is not tight - (nine of the ten are model-invoked today and total ~3.2k of 4k). Reserve + (ten of the eleven are model-invoked today and total ~3.7k of 4k). Reserve `true` for a procedure that is genuinely only ever started deliberately — `release` is the only one left, because nobody cuts a release by implication. ⚠️ **A `true` skill cannot be reached by another skill either.** If a @@ -267,7 +268,7 @@ node/field/option IDs, and the option-deletion hazard` was cut at `#28`, so 90 cases (n=4) and `testing` from 3/5 to 2/5, while the six new skills all measured 100% and every negative case stayed clean. So the ceiling is attention, not characters — we were at 2.8k of a 4k budget throughout _that - experiment_ (it is ~3.2k now; the point is that nothing was near the cap). Adding + experiment_ (it is ~3.7k now; the point is that nothing was near the cap). Adding a skill therefore has a cost paid by the _existing_ ones, which only `skills:eval` can see. **Re-run the full eval after any flip _or description edit_**, not just the changed skill's own cases. @@ -341,7 +342,7 @@ node/field/option IDs, and the option-deletion hazard` was cut at `#28`, so 90 overflows, and drops the least-invoked entries **first** — which are exactly the model-invoked skills that must fire on their own. `verify:skills` prints the current cost against the budget recorded in `scripts/lib/skill-manifest.mjs` - (3,234/4,000 characters as of this writing) and fails when it is exceeded. Raise + (3,679/4,000 characters as of this writing) and fails when it is exceeded. Raise the budget deliberately, or tighten a description; each entry is capped at 1,536 characters regardless, so **put the key use case first**. @@ -364,6 +365,7 @@ skills; the rules are here. - **Before starting work, check the board for the relevant item.** - **Every board item is a real GitHub issue.** No draft cards. Before creating a new issue, check the board for a matching item — **never create a duplicate**. + - **The one exception is a GitHub security advisory**, which is tracked by a **draft card** titled `[GHSA-xxxx-yyyy-zzzz] - `. An advisory is private until it is published, so a real issue would disclose the vulnerability before a fix exists — the thing the whole advisory flow is for. The card is made by hand (no `PROJECT_TOKEN` exists in this org, and `GITHUB_TOKEN` cannot hold `organization projects: write`), and it is converted to a real issue once the advisory is published. The `[GHSA-` prefix is load-bearing: it is what the board audit's draft carve-out keys on, so **any other draft card is still a defect to delete**. The flow itself — verifying who owns the code path, accepting, the private fork, publishing — is the `security-advisory` skill. **Accepting and publishing an advisory are outward-facing and stay human-gated; never automate or bulk-apply either.** - **Only issues go on a board — never PRs.** A PR gets the `v2` label but is tracked through its linked issue's card (via `Closes #N`), not its own board item. - **Label by version — every issue and every PR, no exceptions.** Exactly one of `v1` (work targeting `v1/main`, the deprecated security-fix-only line) or `v2` (active development; the default for anything new). There is no unlabeled state and no "decide later": an issue with neither label belongs to no version line and is invisible to every version-filtered query. Set it at **create time** (`gh issue create --label v2 …`), never by backfilling. **If the target version isn't obvious, it's `v2`.** - **Label by type — exactly one of `bug` / `enhancement` / `documentation` / `chore` / `question`** on every issue you create or triage. The version label says which line the work belongs to; the type label says what kind of work it is, and the two are independent. Don't force the binary: pressing a docs task or a dependency pin into `enhancement` degrades it to "not a bug", at which point filtering by it stops telling you anything. A **PR** needs no type label — it is classified through the issue it closes. From 41efd17cafc0a03355ff62170216a7b708cc93e9 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 18 Sep 2026 21:24:24 -0400 Subject: [PATCH 2/6] docs: address Copilot review round 1 on #2444 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven of nine findings were defects in what this PR added. - Upstream advisories: "close with a pointer to the upstream issue" would have disclosed an unfixed vulnerability on someone else's behalf. Route it through the upstream's own private channel; reference a public issue only after they publish. - Step 6 was impossible as written: GitHub's "Convert to issue" creates a NEW issue from the draft and cannot bind a card to one filed separately. Convert first, then label/milestone/close/move, and skip /issue-create's add-card step. - Closing a rejected or upstream advisory now says to DELETE its draft card — nothing shipped, and the audit's non-Issue check no longer looks at it. - Drop the /pr-flow pointer from Related and say why: it requires a public issue and a public PR, which is the disclosure this flow delays. - `--paginate` on the triage-advisory listing; the endpoint pages at 30 and a truncated inventory reads as "nothing pending". - Record the Priority arithmetic in the draft body — a draft card has no comments, so /issue-triage's "record the score in a comment" cannot be met. - The exemption needed a replacement check. $B28/$B11 are Issue-only, so the non-Issue check was the ONLY one seeing a draft; exempting it alone made a half-made advisory card invisible to the whole audit. Added `GHSA draft missing Status/Priority`, verified against a synthetic set. - board-ops and issue-create both still said draft cards are never allowed, contradicting the new AGENTS.md carve-out. Both now name the exception. Declined: chain eval cases for the /board-ops, /issue-triage and /issue-create pointers. docs/skill-authoring.md puts a chain case in the TARGET skill's file and requires it be measured, and skills:eval is deliberately outside the gate. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/board-ops/SKILL.md | 6 ++ .claude/skills/issue-create/SKILL.md | 5 +- .claude/skills/issue-triage/SKILL.md | 17 ++++++ .claude/skills/security-advisory/SKILL.md | 68 +++++++++++++++++++---- 4 files changed, 83 insertions(+), 13 deletions(-) diff --git a/.claude/skills/board-ops/SKILL.md b/.claude/skills/board-ops/SKILL.md index 0a8f58251..c5a103578 100644 --- a/.claude/skills/board-ops/SKILL.md +++ b/.claude/skills/board-ops/SKILL.md @@ -29,6 +29,12 @@ belong to the field", so the mistake is at least loud. **Only issues go on a board — never PRs, never draft cards.** A PR is tracked through the card of the issue it closes. +**The one exception is a GitHub security advisory**, tracked by a draft card +titled `[GHSA-xxxx-yyyy-zzzz] - …` because a real issue would disclose it before +a fix exists. The flow is `/security-advisory`; every recipe below applies to +that card unchanged, except that an advisory draft is found by title rather than +by issue number. + ## V2 board (#28) IDs The project node id and the field ids are stable. The **option** ids are **not** — diff --git a/.claude/skills/issue-create/SKILL.md b/.claude/skills/issue-create/SKILL.md index 5fe54eca0..9e50fde1a 100644 --- a/.claude/skills/issue-create/SKILL.md +++ b/.claude/skills/issue-create/SKILL.md @@ -29,7 +29,10 @@ query, and an unmilestoned one drops out of release planning silently. **Never create a duplicate.** Check the board for a matching item first. **Never create a draft card** (a board card with no issue number) — every board -item is a real GitHub issue. +item is a real GitHub issue. The single exception is a **GitHub security +advisory**, which is private until it is published and so cannot be tracked by +an issue at all; see `/security-advisory`. Nothing you reach through *this* +flow is that case. ## 0. Check the board first diff --git a/.claude/skills/issue-triage/SKILL.md b/.claude/skills/issue-triage/SKILL.md index bdff1d00d..dbd3b81cf 100644 --- a/.claude/skills/issue-triage/SKILL.md +++ b/.claude/skills/issue-triage/SKILL.md @@ -202,6 +202,7 @@ count means the board contradicts a rule, not that the rule needs revisiting. | Double-boarded | An issue has a card on **one** board, the one matching its version label | Delete the wrong-board card | | Non-Issue items | **Only issues go on a board** — never PRs, never drafts, *except* an advisory draft titled `[GHSA-…]` | Delete the item | | No Status | Every card carries a Status | Set one — `Incoming` if unmilestoned, else by where it actually is | +| GHSA draft missing Status/Priority | An exempted advisory draft still carries both | Set them — `/security-advisory` | | `Incoming` **with** a milestone (#28) | Incoming ⇔ no milestone | Approval was never recorded: move to **Todo**, or clear the milestone | | Past Incoming **without** a milestone (#28) | Everything past Incoming ⇔ milestoned | Claims an approval nobody made: milestone it, or move back to Incoming | | Wrong board for label | `v1` → #11, `v2` → #28 | Move the card to the right board | @@ -241,6 +242,15 @@ jq -nr --slurpfile o "$D/i.json" --slurpfile a "$D/b28.json" --slurpfile b "$D/b "non-Issue on a board": [(own($a)[], own($b)[]) | select(.content.type!="Issue") | (.content.title // "(untitled)") | select(startswith("[GHSA-") | not)], + # $B28/$B11 hold only Issue items, so the Status and Priority checks below + # cannot see an advisory draft. Exempting drafts from the check above would + # therefore have made a half-made advisory card invisible to the whole + # audit; this is the narrow replacement. + "GHSA draft missing Status/Priority": + [own($a)[] | select(.content.type=="DraftIssue" + and ((.content.title // "") | startswith("[GHSA-"))) + | select(.status==null or .priority==null) + | (.content.title[0:24])], "no Status": [($B28[], $B11[]) | select(.s==null) | .n], "Incoming w/ milestone": [$B28[] | select(.s=="Incoming" and ms(.n)!=null) | .n], "past Incoming, no ms": [$B28[] | select(.s!=null and .s!="Incoming" and .s!="Done" @@ -302,6 +312,13 @@ Two things the queries must account for, both learned the hard way: would let an ordinary stray draft through, which is the defect the check exists for. So a draft titled anything else is still reported — by title, since a draft has no issue number to print. + ⚠️ **The exemption had to come with a replacement check.** `$B28` and `$B11` + are built from `Issue` items only, so the `no Status` and `no Priority` + checks never see a draft — before the carve-out the non-Issue check was the + *only* thing looking at one, and exempting drafts there alone would have made + a half-made advisory card invisible to the entire audit. Hence + `GHSA draft missing Status/Priority`, which reads the item-level `.status` + and `.priority` that `item-list` exposes for a draft as it does for an issue. - **`$M` holds closed issues too** — the lookup is built from `gh issue list --state all`, which it has to be, because the last check reads closed issues' state reasons. So `isopen` is not there to cope with a missing diff --git a/.claude/skills/security-advisory/SKILL.md b/.claude/skills/security-advisory/SKILL.md index 70df8fba5..15b3fb8d7 100644 --- a/.claude/skills/security-advisory/SKILL.md +++ b/.claude/skills/security-advisory/SKILL.md @@ -18,8 +18,13 @@ human-gated**: **accepting** an advisory (the reporter sees it) and there is no unpublish). Never automate either, never bulk-apply them, and never take either step because a checklist said to. Everything else here is mechanics. -Related: `/board-ops` (the card IDs and recipes), `/issue-create` and -`/pr-flow` for the public issue and the eventual release. +Related: `/board-ops` (the card IDs and recipes) and `/issue-create`, for the +public issue **after** publication. + +⚠️ **`/pr-flow` does not apply to the fix itself.** It requires a public issue +and a public PR against `v2/main` — the disclosure this flow exists to delay. +The fix is reviewed inside the private fork (step 4), and `/pr-flow` becomes +relevant only once the advisory is published. ## The flow @@ -49,6 +54,16 @@ a real GitHub issue". `Incoming` is correct even though somebody clearly triaged it to make the card: nobody has approved shipping a fix yet, and a draft card has no milestone to carry the approval. + ⚠️ **Put the score's arithmetic in the draft body**, under the description. + `/issue-triage` says to record it as an issue comment, and a draft card has + no comments — so without this the Priority is a bare word with nothing behind + it, and a later re-scoring cannot tell a judgment from a guess. Write the two + axes, the bonuses you claimed, and the total, exactly as the comment form + would. + ⚠️ **Set both fields.** The board audit's non-Issue check now exempts + `[GHSA-` drafts, so a half-made card no longer trips it; the audit carries a + narrow replacement check (see `/issue-triage`) and it is the only thing + looking. The card is made **by hand**. There is no `PROJECT_TOKEN` in this org and `organization projects: write` is a permission `GITHUB_TOKEN` structurally @@ -58,7 +73,9 @@ workflow for this;** that approach was tried and abandoned for exactly this reason. ```sh -gh api repos/modelcontextprotocol/inspector/security-advisories \ +# --paginate: this endpoint returns 30 per page, and an inventory that silently +# stops at the first page is worse than none — it reads as "nothing pending". +gh api --paginate repos/modelcontextprotocol/inspector/security-advisories \ --jq '.[] | select(.state=="triage") | "\(.ghsa_id)\t\(.severity)\t\(.summary)"' ``` @@ -76,8 +93,17 @@ it. Had ownership been checked after the severity assessment rather than before, the fix would have been written against the wrong repo. So: reproduce it, find the code, and check whether that code is first-party or -reached through a dependency. An advisory against upstream code is closed here -with a pointer to the upstream issue — it is not ours to accept or publish. +reached through a dependency. An advisory against upstream code is not ours to +accept or publish. + +⚠️ **"Upstream's problem" is not a reason to say it in public.** A genuine +unfixed vulnerability handed to a public upstream issue is disclosed — by us, +on someone else's behalf, before they have a fix. Route it through **that +project's own private reporting channel** (its `SECURITY.md`, or its advisory +form), and only reference a public upstream issue once the upstream has +published. Where the reporter would rather carry it over themselves, say so and +let them. #2409 took the benign version of this path: the reporter withdrew the +report here and raised it upstream. ### 3. Accept, or close @@ -88,6 +114,12 @@ moves the advisory `triage` → `draft`. The state is readable as `state` and **Invalid, out of scope, or upstream → close** with a comment saying which, and why. A reporter who is told nothing reasonably assumes they were ignored. +⚠️ **Closing an advisory leaves its draft card behind — delete it.** Nothing +shipped, so `Done` would be a false record and `Incoming` would claim work is +still queued; `AGENTS.md` deletes a card in exactly this situation, and a +rejected advisory's card is now invisible to the audit's non-Issue check by +construction. The delete recipe is in `/board-ops`. + ⚠️ **Accepting is a human act, always.** It is visible to the reporter and it commits this project to treating the report as a real vulnerability. Nothing in this skill authorizes taking it — surface the recommendation and let a @@ -138,13 +170,25 @@ hands out a working exploit. requests a **CVE**, and credits the reporter. There is no undo. Same rule as accepting: recommend, never perform. -### 6. File the public issue afterwards - -Once the advisory is published, the work becomes ordinary board history: file a -public issue recording what shipped, **close it** (the work is already done), -and convert the draft card to that issue so the board stops carrying a draft. -Label and milestone it per `/issue-create`; `Done` is correct here, because the -fix genuinely shipped. +### 6. Convert the card afterwards + +Once the advisory is published, the work becomes ordinary board history and the +draft card becomes a real issue. + +⚠️ **Convert FIRST — the order is not interchangeable.** GitHub's "Convert to +issue" creates a **new** issue from the draft; there is no way to point an +existing card at an issue you filed separately. Filing the issue by hand and +then converting produces two issues and two cards, which is why this step reads +the way it does: + +1. **Convert the draft card to an issue** on board #28 (the card keeps its + place and its field values; the issue is created from the card's title and + body). +2. Apply `v2` and a type label, and a milestone — the one the fix shipped in. + Do **not** run `/issue-create`'s add-card step: the card already exists. +3. **Close it.** The work shipped before the issue existed. +4. Move the card to **`Done`** — correct here, because the fix genuinely + shipped. ## API facts worth not re-deriving From 2bba258f3897aa112b6fc46de263288a2390302b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 18 Sep 2026 21:40:11 -0400 Subject: [PATCH 3/6] docs: address Copilot review round 2 on #2444 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three findings were defects in what this PR added. - The audit's carve-out matched on the title prefix alone, across BOTH boards and ANY non-Issue type. So a pull request titled `[GHSA-…]` — a plausible title for a security fix — was exempted, and so was an advisory draft misfiled on #11, where the replacement field check does not look either, so both checks would have read 0. The exemption is now `DraftIssue` AND the `[GHSA-` prefix AND board #28; #11 reports every non-Issue item it carries. Verified on a synthetic set: the exempt draft passes, while a stray draft, a GHSA-titled PR and a #11 advisory draft are all still reported. - board-ops claimed every recipe applied to an advisory draft unchanged. It does not: a draft has no repository and no issue number, so every `select(.content.repository==… and .content.number==…)` matches nothing and `item-add --url` has no URL. Added the title-based item-id lookup, keyed on the bracketed GHSA id rather than on words from the free-text summary. - The step table still said step 6 files the public issue and then converts the card — the impossible ordering the body below it was fixed to reject. The row now names conversion as what creates the issue. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/board-ops/SKILL.md | 20 +++++++++++++++++--- .claude/skills/issue-triage/SKILL.md | 23 +++++++++++++++++------ .claude/skills/security-advisory/SKILL.md | 2 +- 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/.claude/skills/board-ops/SKILL.md b/.claude/skills/board-ops/SKILL.md index c5a103578..4eb7bd2d5 100644 --- a/.claude/skills/board-ops/SKILL.md +++ b/.claude/skills/board-ops/SKILL.md @@ -31,9 +31,23 @@ through the card of the issue it closes. **The one exception is a GitHub security advisory**, tracked by a draft card titled `[GHSA-xxxx-yyyy-zzzz] - …` because a real issue would disclose it before -a fix exists. The flow is `/security-advisory`; every recipe below applies to -that card unchanged, except that an advisory draft is found by title rather than -by issue number. +a fix exists. The flow is `/security-advisory`. + +⚠️ **A draft card has no repository and no issue number, so the lookups below +cannot find one.** Every `select(.content.repository==… and .content.number==…)` +matches nothing against a draft, and `item-add --url` has no URL to be given. +Look it up by **title** instead, then feed that item id to `item-edit` or +`item-delete` exactly as usual: + +```sh +ITEM_ID=$(gh project item-list 28 --owner modelcontextprotocol --format json --limit 500 \ + --jq '.items[] | select(.content.type=="DraftIssue") + | select(.content.title | startswith("[GHSA-xxxx-yyyy-zzzz]")) | .id') +``` + +Match on the **bracketed GHSA id**, not on words from the summary — a summary is +free text and two advisories can share one. Advisory drafts live on #28 only; +`/issue-triage`'s audit reports one found anywhere else. ## V2 board (#28) IDs diff --git a/.claude/skills/issue-triage/SKILL.md b/.claude/skills/issue-triage/SKILL.md index dbd3b81cf..ba7f02f52 100644 --- a/.claude/skills/issue-triage/SKILL.md +++ b/.claude/skills/issue-triage/SKILL.md @@ -200,7 +200,7 @@ count means the board contradicts a rule, not that the rule needs revisiting. | Check | Invariant | Fix | | --- | --- | --- | | Double-boarded | An issue has a card on **one** board, the one matching its version label | Delete the wrong-board card | -| Non-Issue items | **Only issues go on a board** — never PRs, never drafts, *except* an advisory draft titled `[GHSA-…]` | Delete the item | +| Non-Issue items | **Only issues go on a board** — never PRs, never drafts, *except* a `[GHSA-…]` **draft** on **#28** | Delete the item | | No Status | Every card carries a Status | Set one — `Incoming` if unmilestoned, else by where it actually is | | GHSA draft missing Status/Priority | An exempted advisory draft still carries both | Set them — `/security-advisory` | | `Incoming` **with** a milestone (#28) | Incoming ⇔ no milestone | Approval was never recorded: move to **Todo**, or clear the milestone | @@ -237,11 +237,15 @@ jq -nr --slurpfile o "$D/i.json" --slurpfile a "$D/b28.json" --slurpfile b "$D/b | { "double-boarded": [$B28[].n | select(. as $n | [$B11[].n]|index($n))], # An advisory draft card is the ONE legitimate non-Issue item (see AGENTS.md). - # It is identified by its `[GHSA-` title prefix and nothing else, so a stray - # draft is still reported. Reports the TITLE, since a draft has no number. - "non-Issue on a board": [(own($a)[], own($b)[]) | select(.content.type!="Issue") - | (.content.title // "(untitled)") - | select(startswith("[GHSA-") | not)], + # The exemption is narrowed three ways, and each one matters: DRAFTS only + # (a GHSA-titled PR is still reported), board #28 ONLY (an advisory has no + # business on #11), and the `[GHSA-` title prefix (a stray draft is still + # reported). Reports the TITLE, since a draft has no number. + "non-Issue on a board": [(own($a)[] | select(.content.type!="Issue" + and ((.content.type=="DraftIssue" + and ((.content.title // "") | startswith("[GHSA-"))) | not))), + (own($b)[] | select(.content.type!="Issue"))] + | map(.content.title // "(untitled)"), # $B28/$B11 hold only Issue items, so the Status and Priority checks below # cannot see an advisory draft. Exempting drafts from the check above would # therefore have made a half-made advisory card invisible to the whole @@ -312,6 +316,13 @@ Two things the queries must account for, both learned the hard way: would let an ordinary stray draft through, which is the defect the check exists for. So a draft titled anything else is still reported — by title, since a draft has no issue number to print. + ⚠️ **The title prefix alone is not enough, because a title is not a type and + not a board.** Matched on its own it would also exempt a **pull request** + whose title happens to start `[GHSA-` — a plausible title for a security fix + — and an advisory draft misfiled on **#11**, where the replacement field check + below does not look either, so both checks would read `0`. The exemption is + therefore `DraftIssue` **and** `[GHSA-` **and** board #28; #11 still reports + every non-Issue item it carries. ⚠️ **The exemption had to come with a replacement check.** `$B28` and `$B11` are built from `Issue` items only, so the `no Status` and `no Priority` checks never see a draft — before the carve-out the non-Issue check was the diff --git a/.claude/skills/security-advisory/SKILL.md b/.claude/skills/security-advisory/SKILL.md index 15b3fb8d7..ba28af92d 100644 --- a/.claude/skills/security-advisory/SKILL.md +++ b/.claude/skills/security-advisory/SKILL.md @@ -35,7 +35,7 @@ relevant only once the advisory is published. | 3 | Valid → **accept** (`triage` → `draft`); invalid → close with a reason | **Human only** | | 4 | Create the **private fork**, fix and review there | Mechanical | | 5 | Merge, release, then **publish** the advisory | **Human only** | -| 6 | After the release, file the public (closed) issue and convert the card | Mechanical | +| 6 | After the release, **convert** the draft card — that is what creates the public issue | Mechanical | ### 1. Board it as a draft card From b2d44fff73c64669f89e9963ec2429c6b87811dd Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 19 Sep 2026 08:51:04 -0400 Subject: [PATCH 4/6] docs: address Copilot review round 3 on #2444 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings were defects in what this PR added. - The skill assumed v2 throughout: the fix "comes back to `v2/main`", and step 6 applied the `v2` label unconditionally. But an advisory is very nearly the ONLY work the v1 line ever receives — SECURITY.md supports v1 for security fixes only, published under `v1-latest` — so the skill was wrong exactly where v1 matters most. A v1-only advisory would have been merged to a branch where the bug does not exist; one affecting both lines could have been published with v1 still unpatched, which is the worst outcome this flow can produce. Step 2 now ends in a SET of affected lines, with the per-line branch and dist-tag table and the note that the two publish independently so a v1 fix is not forward-ported. Step 4 merges to each affected branch; step 5 states that "shipped" means shipped on every affected line before publishing. Step 6's labels, milestone and board branch by line — `v1` takes no milestone (every milestone is a v2 release bucket) and is carded on #11, which has no Priority field. The reporter's v2/v1/both answer is described as what SECURITY.md actually is — a request in "What to Include", not a required form field — so it is read and then verified rather than trusted. - The frontmatter description and the Related line still said to "file" the public issue, contradicting step 6, which round 1 established must CONVERT the existing draft card. Both now say convert. Listing budget re-checked: 3,711/4,000, and the recorded figure in AGENTS.md moved with it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/security-advisory/SKILL.md | 75 +++++++++++++++++++---- AGENTS.md | 4 +- 2 files changed, 64 insertions(+), 15 deletions(-) diff --git a/.claude/skills/security-advisory/SKILL.md b/.claude/skills/security-advisory/SKILL.md index ba28af92d..793868e4d 100644 --- a/.claude/skills/security-advisory/SKILL.md +++ b/.claude/skills/security-advisory/SKILL.md @@ -1,6 +1,6 @@ --- name: security-advisory -description: "Take a privately reported vulnerability through this repo's security advisory flow — board it, verify who owns the code path, accept or reject, fix it in the private fork, publish, then file the public issue. Use when a vulnerability is reported privately; when deciding whether an advisory is ours to fix; when looking up or creating its private fork; when answering a reporter; or when a GHSA-titled board card needs handling." +description: "Take a privately reported vulnerability through this repo's security advisory flow — board it, verify who owns the code path, accept or reject, fix it in the private fork, ship to every affected release line, publish, then convert the card. Use when a vulnerability is reported privately; when deciding whether an advisory is ours to fix; when looking up or creating its private fork; when answering a reporter; or when a GHSA-titled board card needs handling." disable-model-invocation: false --- @@ -19,22 +19,24 @@ there is no unpublish). Never automate either, never bulk-apply them, and never take either step because a checklist said to. Everything else here is mechanics. Related: `/board-ops` (the card IDs and recipes) and `/issue-create`, for the -public issue **after** publication. +labels and milestone the converted card takes **after** publication. The public +issue is not *filed* — step 6 **converts** the draft card, which is what creates +it. ⚠️ **`/pr-flow` does not apply to the fix itself.** It requires a public issue -and a public PR against `v2/main` — the disclosure this flow exists to delay. -The fix is reviewed inside the private fork (step 4), and `/pr-flow` becomes -relevant only once the advisory is published. +and a public PR against the release branch — the disclosure this flow exists to +delay. The fix is reviewed inside the private fork (step 4), and `/pr-flow` +becomes relevant only once the advisory is published. ## The flow | # | Step | Gate | | --- | --- | --- | | 1 | Advisory lands in state `triage` → **draft card** on board #28 | Mechanical | -| 2 | **Verify the claim — including who owns the code path** | Judgment | +| 2 | **Verify the claim** — who owns the code path, and **which release lines are affected** | Judgment | | 3 | Valid → **accept** (`triage` → `draft`); invalid → close with a reason | **Human only** | | 4 | Create the **private fork**, fix and review there | Mechanical | -| 5 | Merge, release, then **publish** the advisory | **Human only** | +| 5 | Merge **to every affected line**, release each, then **publish** the advisory | **Human only** | | 6 | After the release, **convert** the draft card — that is what creates the public issue | Mechanical | ### 1. Board it as a draft card @@ -80,7 +82,7 @@ gh api --paginate repos/modelcontextprotocol/inspector/security-advisories \ | "\(.ghsa_id)\t\(.severity)\t\(.summary)"' ``` -### 2. Verify the claim — and who owns the code path +### 2. Verify the claim — who owns the code path, and which lines it affects Before assessing severity, establish that the vulnerable code is **ours**. A report can be entirely accurate about behavior the Inspector merely exhibits @@ -105,6 +107,32 @@ published. Where the reporter would rather carry it over themselves, say so and let them. #2409 took the benign version of this path: the reporter withdrew the report here and raised it upstream. +#### Which release lines are affected — ask it here, not at merge time + +⚠️ **An advisory is very nearly the only work the v1 line ever receives**, so +this is exactly where assuming v2 does the most damage. `SECURITY.md` supports +v1 for **security fixes only**, published under the `v1-latest` dist-tag, and +its "What to Include" asks the reporter to state "whether it affects v2, v1, or +both". Read what they said and then check it yourself — it is a request, not a +required form field, so it is often absent and it is never authoritative when +present. A v1-only advisory assumed to be +v2 gets merged to a branch where the bug does not exist, and one affecting both +lines leaves v1 **unpatched** while the advisory is published, which is the +worst outcome this whole flow can produce. + +So the outcome of step 2 is a **set** of affected lines, and each one is +shipped on its own terms: + +| Line | Branch | Flow | Publishes to | +| --- | --- | --- | --- | +| v2 | `v2/main` | `fix branch → v2/main → (milestone) main` | `latest` | +| v1 | `v1/main` | `fix branch → v1/main`, flat — **no merge into `main`** | `v1-latest` | + +**The two lines publish independently under separate dist-tags, so a v1 fix is +not forward-ported** — if v2 is affected too, that is a second fix on `v2/main`, +not a merge. Branch names carry the version segment either way +(`v1/fix/…`, `v2/fix/…`). + ### 3. Accept, or close **Valid and ours → accept.** In the UI this is "Accept and open as draft"; it @@ -157,15 +185,24 @@ something you can quietly undo — it takes a re-scoped token or an admin in the UI. That asymmetry is the whole reason for the read-first rule above. Fix and review inside the fork. Its PRs and commits are private, so none of the -normal public review flow applies; the diff comes back to `v2/main` as an -ordinary commit at merge time. +normal public review flow applies; the diff comes back as an ordinary commit at +merge time, **to the branch of each line step 2 found affected** — `v2/main` +for v2, `v1/main` for v1. -### 5. Merge, release, publish +### 5. Merge to every affected line, release, publish Publish **after** the fix has shipped in a release, never before — publishing discloses the vulnerability, so doing it while users have no upgrade available hands out a working exploit. +⚠️ **"Shipped" means shipped on *every* affected line.** The two lines release +independently under separate dist-tags, so v2 reaching `latest` says nothing +about `v1-latest`. Publishing with one line still unpatched discloses a live +vulnerability to the users who have no fix — and they are the users least able +to move, since v1 is the deprecated line they are on because upgrading is hard. +Cutting each release is `/release` for v2; a v1 fix publishes straight from +`v1/main`. + ⚠️ **Publishing is irreversible and human-gated.** It makes the advisory public, requests a **CVE**, and credits the reporter. There is no undo. Same rule as accepting: recommend, never perform. @@ -184,8 +221,19 @@ the way it does: 1. **Convert the draft card to an issue** on board #28 (the card keeps its place and its field values; the issue is created from the card's title and body). -2. Apply `v2` and a type label, and a milestone — the one the fix shipped in. - Do **not** run `/issue-create`'s add-card step: the card already exists. +2. Apply a **type label** and the **version label of the line the fix shipped + on**, then a milestone — and those two are not independent: + + | Affected | Version label | Milestone | Board | + | --- | --- | --- | --- | + | v2 | `v2` | the release the fix shipped in | #28 — the converted card is already there | + | v1 | `v1` | **none** — every milestone is a v2 release bucket | **#11**, which has no Priority field | + | both | one issue per line, labelled and boarded as above | | | + + Do **not** run `/issue-create`'s add-card step for the converted card: it + already exists. A **v1** issue does need a card created on #11, because the + draft lived on #28 — and a v1 advisory's draft card on #28 is deleted once + its #11 issue exists, rather than left behind claiming v2 work. 3. **Close it.** The work shipped before the issue existed. 4. Move the card to **`Done`** — correct here, because the fix genuinely shipped. @@ -203,3 +251,4 @@ All verified against the live API. | Fork deletion | Needs the `delete_repo` OAuth scope; a default `gh` token lacks it | | Comments | **No API at all**, REST or GraphQL. UI-only | | Board writes | Not automatable — no `PROJECT_TOKEN`, and `GITHUB_TOKEN` cannot hold `organization projects: write` | +| Affected lines | `SECURITY.md` **asks** for v2 / v1 / both — a request, not a required field. Read it, never rely on it | diff --git a/AGENTS.md b/AGENTS.md index 5fc98cae0..b5944e819 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,7 @@ users invoke them by name. | [`pr-flow`](.claude/skills/pr-flow/SKILL.md) | Branch naming, DCO signoff, screenshots, opening the PR, requesting a Copilot review, responding, closing out | Model-invoked, or `/pr-flow` | | [`pre-push-gate`](.claude/skills/pre-push-gate/SKILL.md) | Running `npm run local:gate` and diagnosing a failing stage | Model-invoked, or `/pre-push-gate` | | [`release`](.claude/skills/release/SKILL.md) | Cutting a release: bump on `v2/main`, milestone merge, tag `origin/main`, publish | `/release` | -| [`security-advisory`](.claude/skills/security-advisory/SKILL.md) | A privately reported vulnerability end to end: the draft card, verifying who owns the code path, accepting, the private fork, publishing, the public issue afterwards | Model-invoked, or `/security-advisory` | +| [`security-advisory`](.claude/skills/security-advisory/SKILL.md) | A privately reported vulnerability end to end: the draft card, who owns the code path, which release lines are affected, accepting, the private fork, publishing, converting the card | Model-invoked, or `/security-advisory` | | [`test-servers`](.claude/skills/test-servers/SKILL.md) | Picking and running a showcase test server; the stale-build hazard | Model-invoked, or `/test-servers` | Longer-form human documentation lives in [`docs/`](./docs) — see the table in the @@ -342,7 +342,7 @@ node/field/option IDs, and the option-deletion hazard` was cut at `#28`, so 90 overflows, and drops the least-invoked entries **first** — which are exactly the model-invoked skills that must fire on their own. `verify:skills` prints the current cost against the budget recorded in `scripts/lib/skill-manifest.mjs` - (3,679/4,000 characters as of this writing) and fails when it is exceeded. Raise + (3,711/4,000 characters as of this writing) and fails when it is exceeded. Raise the budget deliberately, or tighten a description; each entry is capped at 1,536 characters regardless, so **put the key use case first**. From 4da164ac3fa7c89e77914e4d780ef63dc51b0725 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 19 Sep 2026 09:07:31 -0400 Subject: [PATCH 5/6] docs: address Copilot review round 4 on #2444 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four accepted, one accepted in part. - The card body no longer carries the vulnerability description. Project access and advisory access are SEPARATE permission sets, so the board's audience is not the advisory's audience. The boards are private, so this is a wider audience than intended rather than a public leak — but a repro or PoC belongs with the people handling it. Link plus triage metadata only. - Publishing does NOT assign a CVE or credit the reporter. A CVE request is optional and a credit must be explicitly added and then accepted. Both are now stated as things to do BEFORE publishing, which is more useful than the overclaim was: an unadded reporter is simply never credited and nothing reports it. - The card now moves In Progress / In Review with the work. It being private is the reason to do this, not to skip it — the fork is invisible to anyone not on the advisory, so the card is the only signal the work exists. - The "both lines" case gets a deterministic sequence: the draft converts exactly once, so v2 inherits the conversion on #28 and v1 is filed separately on #11. A v1-ONLY advisory deletes the #28 draft rather than converting it, since a converted card would put a v1 issue on #28. - The `/release` pointer added in round 3 is removed. It was a dead end twice over: `release` is `disable-model-invocation: true`, which AGENTS.md says a skill cannot reach, and it moves through two PUBLIC PRs. Declined the second half of that last one — designing a security-release path that publishes without the public-PR sequence. The premise does not hold: the patch stops being secret at MERGE, since merging the private fork puts an ordinary public commit on the release branch, so no release path can keep it private. Stated that directly instead, with the actionable consequence: the merge-to-publish window is exposure, not secrecy, so merge close to the release. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/security-advisory/SKILL.md | 94 +++++++++++++++++++---- 1 file changed, 78 insertions(+), 16 deletions(-) diff --git a/.claude/skills/security-advisory/SKILL.md b/.claude/skills/security-advisory/SKILL.md index 793868e4d..c33ac6ced 100644 --- a/.claude/skills/security-advisory/SKILL.md +++ b/.claude/skills/security-advisory/SKILL.md @@ -12,11 +12,18 @@ chooser deliberately has no security template, because a vulnerability report must not open a public issue. So an advisory never arrives as an issue, and for most of its life it must **not** become one. -Two steps in this flow are **outward-facing and irreversible-ish, and both stay -human-gated**: **accepting** an advisory (the reporter sees it) and -**publishing** it (it becomes public, assigns a CVE, and credits the reporter — -there is no unpublish). Never automate either, never bulk-apply them, and never -take either step because a checklist said to. Everything else here is mechanics. +Two steps in this flow are **outward-facing, and both stay human-gated**: +**accepting** an advisory (the reporter sees it) and **publishing** it (it +becomes public, and there is no unpublish). Never automate either, never +bulk-apply them, and never take either step because a checklist said to. +Everything else here is mechanics. + +⚠️ **A CVE and the credits are *choices made at publish time*, not effects of +publishing.** Requesting a CVE is an optional action on the advisory, and a +credit appears only when someone is explicitly added **and accepts** it. They +are named here because they are the parts a maintainer must not forget — the +reporter's credit especially, since nothing prompts for it — not because +publishing performs them. Related: `/board-ops` (the card IDs and recipes) and `/issue-create`, for the labels and milestone the converted card takes **after** publication. The public @@ -50,8 +57,18 @@ a real GitHub issue". is not cosmetic: the board audit in `/issue-triage` keys its draft carve-out on it, so a card titled any other way is reported as a stray draft. - **Body:** `**Advisory:** ` on the first line, then severity and - reported date, then the advisory description. The link first, because a - maintainer reading the card has no other route back to the private advisory. + reported date. The link first, because a maintainer reading the card has no + other route back to the private advisory. + ⚠️ **Do not copy the vulnerability description onto the card.** Project + access and advisory access are **separate permission sets**, so the board's + audience is not the advisory's audience — anyone with project access reads + the card, whether or not they are an advisory collaborator. The boards are + private ([`/issue-triage`](../issue-triage/SKILL.md)), so this is a wider + audience than intended rather than a public leak, but a reproduction or a PoC + is the part worth keeping to the people handling it. The card carries the + **link and triage metadata only**; the link is how a reader with access gets + the details, and the absence of details is how a reader without access is + told they do not have them. - **Status `Incoming`**, plus a Priority scored with the `/issue-triage` rubric. `Incoming` is correct even though somebody clearly triaged it to make the card: nobody has approved shipping a fix yet, and a draft card has no @@ -189,6 +206,15 @@ normal public review flow applies; the diff comes back as an ordinary commit at merge time, **to the branch of each line step 2 found affected** — `v2/main` for v2, `v1/main` for v1. +⚠️ **Move the card as the work moves.** `AGENTS.md`'s lifecycle applies to this +card like any other: **`In Progress`** when the fix is started, **`In Review`** +when the fork's PR is open. The card being private is not a reason to skip it — +it is the reason to do it, since the fork is invisible to everyone who is not on +the advisory, and this card is the only place the rest of the team can see the +work exists at all. A card that sits in `Incoming` until it jumps to `Done` +reports "unreviewed, nobody committed to it" for the entire time somebody is +actively fixing it. + ### 5. Merge to every affected line, release, publish Publish **after** the fix has shipped in a release, never before — publishing @@ -200,12 +226,29 @@ independently under separate dist-tags, so v2 reaching `latest` says nothing about `v1-latest`. Publishing with one line still unpatched discloses a live vulnerability to the users who have no fix — and they are the users least able to move, since v1 is the deprecated line they are on because upgrading is hard. -Cutting each release is `/release` for v2; a v1 fix publishes straight from -`v1/main`. -⚠️ **Publishing is irreversible and human-gated.** It makes the advisory public, -requests a **CVE**, and credits the reporter. There is no undo. Same rule as -accepting: recommend, never perform. +⚠️ **The patch stops being secret at MERGE, not at publish — and no release +path changes that.** Merging the private fork puts an ordinary public commit on +`v2/main` or `v1/main`, readable by anyone, and a v2 release then moves it +through **two public PRs** on its way to `main`. So the window between merge and +publish is not a period of secrecy to protect; it is a period of **exposure to +anyone reading commits**, which is why it should be short. Merge close to the +release rather than early, and publish as soon as the release is out. + +**Do not hand this off to the release skill.** It is `disable-model-invocation: +true`, so a pointer to it from here is a dead end for the model anyway — a +maintainer invokes `/release` themselves. Say which lines need a release and +stop there. A v1 fix takes no merge into `main` at all and publishes straight +from `v1/main`, so it does not go through that procedure. + +⚠️ **Publishing is irreversible and human-gated.** It makes the advisory +public, and there is no undo. Same rule as accepting: recommend, never perform. + +**Before publishing, do the two things publishing will not do for you:** +request the **CVE** (optional, and the advisory is the only place to ask) and +**add the reporter to the credits** — a credit is an explicit addition the +person then has to accept, so an unadded reporter is simply never credited, and +that is the failure nobody notices because nothing reports it. ### 6. Convert the card afterwards @@ -228,12 +271,31 @@ the way it does: | --- | --- | --- | --- | | v2 | `v2` | the release the fix shipped in | #28 — the converted card is already there | | v1 | `v1` | **none** — every milestone is a v2 release bucket | **#11**, which has no Priority field | - | both | one issue per line, labelled and boarded as above | | | + | both | **two issues**, one per line — see below | | | Do **not** run `/issue-create`'s add-card step for the converted card: it - already exists. A **v1** issue does need a card created on #11, because the - draft lived on #28 — and a v1 advisory's draft card on #28 is deleted once - its #11 issue exists, rather than left behind claiming v2 work. + already exists. + + **The draft converts exactly once, so "both" needs a stated order.** Every + issue carries exactly one version label and lives on one board, and there is + only ever one draft card — so one line inherits it and the other gets a + fresh issue: + + 1. **Convert the draft into the `v2` issue on #28.** v2 takes the + conversion because the draft is already on #28 and v2 is the line with a + milestone to record. + 2. **File the `v1` issue separately** through `/issue-create` — `v1`, a type + label, **no milestone**, and a card on **#11** (Status only; that board + has no Priority field). This one *is* filed rather than converted, which + is not a contradiction of step 6: there is no second draft to convert. + 3. Cross-link the two so neither reads as the whole story, then **close + both** and move both cards to `Done`. + + **For a v1-only advisory** the draft is on the wrong board and cannot be + moved there by converting: file the `v1` issue on #11 as in (2), then + **delete** the #28 draft rather than converting it — a converted card would + put a `v1` issue on #28, which the board audit reports as a wrong-board + card. 3. **Close it.** The work shipped before the issue existed. 4. Move the card to **`Done`** — correct here, because the fix genuinely shipped. From 856c82f5d90072452f21420cb1251ce4070fc4ee Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 19 Sep 2026 09:23:27 -0400 Subject: [PATCH 6/6] docs: address Copilot review round 5 on #2444 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three findings were internal contradictions in text this PR added. - Step 1 required a rubric Priority, whose FIRST axis is severity, while step 2 says ownership is established before severity — so the procedure forced the exact order #2409 is the warning against. The step-1 score is now explicitly PROVISIONAL, recorded as such and dated in the draft body, and re-scored at the end of step 2 when ownership and the affected lines are known. The provisional line stays when re-scored so the change of view is legible. - The flow table, the frontmatter description and the Related line all made conversion universal and triggered on "after the release". Both were wrong: a v1-only advisory FILES on #11 and deletes the draft, and the trigger is PUBLICATION, since the release ships the fix while the advisory can still be private — a public issue opened in that gap describes an undisclosed vulnerability. All three sites are now per-line and post-publication, and step 6 states the reason rather than just the rule. - AGENTS.md's carve-out said the draft is always converted once published, contradicting the per-line procedure. It now states the per-line outcome. Listing budget re-checked after the description edit: 3,729/4,000, with the recorded figure moved to match. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/security-advisory/SKILL.md | 62 ++++++++++++++++------- AGENTS.md | 4 +- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/.claude/skills/security-advisory/SKILL.md b/.claude/skills/security-advisory/SKILL.md index c33ac6ced..ce2594d24 100644 --- a/.claude/skills/security-advisory/SKILL.md +++ b/.claude/skills/security-advisory/SKILL.md @@ -1,6 +1,6 @@ --- name: security-advisory -description: "Take a privately reported vulnerability through this repo's security advisory flow — board it, verify who owns the code path, accept or reject, fix it in the private fork, ship to every affected release line, publish, then convert the card. Use when a vulnerability is reported privately; when deciding whether an advisory is ours to fix; when looking up or creating its private fork; when answering a reporter; or when a GHSA-titled board card needs handling." +description: "Take a privately reported vulnerability through this repo's security advisory flow — board it, verify who owns the code path, accept or reject, fix it in the private fork, ship to every affected release line, publish, then turn the card into public tracking. Use when a vulnerability is reported privately; when deciding whether an advisory is ours to fix; when looking up or creating its private fork; when answering a reporter; or when a GHSA-titled board card needs handling." disable-model-invocation: false --- @@ -26,9 +26,15 @@ reporter's credit especially, since nothing prompts for it — not because publishing performs them. Related: `/board-ops` (the card IDs and recipes) and `/issue-create`, for the -labels and milestone the converted card takes **after** publication. The public -issue is not *filed* — step 6 **converts** the draft card, which is what creates -it. +labels, milestone and board that public tracking takes — **after publication, +never merely after the release**, since the release ships the fix while the +advisory may still be private. + +⚠️ **How that tracking is created depends on the affected lines**, and only the +v2 path is a conversion: a v2 issue is **converted** from the draft (filing one +separately would duplicate both the issue and the card), while a v1 issue is +**filed** on #11, because the draft is on #28 and cannot move there. Step 6 has +the per-line sequence. ⚠️ **`/pr-flow` does not apply to the fix itself.** It requires a public issue and a public PR against the release branch — the disclosure this flow exists to @@ -44,7 +50,7 @@ becomes relevant only once the advisory is published. | 3 | Valid → **accept** (`triage` → `draft`); invalid → close with a reason | **Human only** | | 4 | Create the **private fork**, fix and review there | Mechanical | | 5 | Merge **to every affected line**, release each, then **publish** the advisory | **Human only** | -| 6 | After the release, **convert** the draft card — that is what creates the public issue | Mechanical | +| 6 | **After publication**, turn the card into public tracking — per line: convert (v2), or file on #11 and delete the draft (v1) | Mechanical | ### 1. Board it as a draft card @@ -69,16 +75,25 @@ a real GitHub issue". **link and triage metadata only**; the link is how a reader with access gets the details, and the absence of details is how a reader without access is told they do not have them. -- **Status `Incoming`**, plus a Priority scored with the `/issue-triage` rubric. - `Incoming` is correct even though somebody clearly triaged it to make the - card: nobody has approved shipping a fix yet, and a draft card has no - milestone to carry the approval. - ⚠️ **Put the score's arithmetic in the draft body**, under the description. - `/issue-triage` says to record it as an issue comment, and a draft card has - no comments — so without this the Priority is a bare word with nothing behind - it, and a later re-scoring cannot tell a judgment from a guess. Write the two - axes, the bonuses you claimed, and the total, exactly as the comment form - would. +- **Status `Incoming`**, plus a **provisional** Priority scored with the + `/issue-triage` rubric. `Incoming` is correct even though somebody clearly + triaged it to make the card: nobody has approved shipping a fix yet, and a + draft card has no milestone to carry the approval. + ⚠️ **Provisional is not a hedge — it is the only honest score at this + point.** The rubric's first axis is *severity*, and step 2 says ownership is + established **before** severity, precisely because #2409 looked severe right + up until it turned out not to be ours. At step 1 you have a report and + nothing verified, so score what the report claims, mark it provisional in the + body, and **re-score it at the end of step 2**, when you know whether the + code is ours and which lines it reaches. An advisory that turns out to be + upstream has its card deleted rather than re-scored (step 3). + ⚠️ **Put the score's arithmetic in the draft body**, marked provisional and + dated. `/issue-triage` says to record it as an issue comment, and a draft + card has no comments — so without this the Priority is a bare word with + nothing behind it, and the step-2 re-score cannot tell what it is revising. + Write the two axes, the bonuses you claimed, and the total, exactly as the + comment form would; leave the provisional line in place when you re-score and + add the new one under it, so the change of view is legible. ⚠️ **Set both fields.** The board audit's non-Issue check now exempts `[GHSA-` drafts, so a half-made card no longer trips it; the audit carries a narrow replacement check (see `/issue-triage`) and it is the only thing @@ -150,6 +165,12 @@ not forward-ported** — if v2 is affected too, that is a second fix on `v2/main not a merge. Branch names carry the version segment either way (`v1/fix/…`, `v2/fix/…`). +**Now re-score the card's Priority**, replacing the provisional one from step 1. +This is the first point at which the rubric's severity axis has anything solid +under it: you know the code is ours, you have reproduced it, and you know how +many lines it reaches — and "affects both lines" is itself a severity input the +provisional score could not have had. + ### 3. Accept, or close **Valid and ours → accept.** In the UI this is "Accept and open as draft"; it @@ -250,10 +271,15 @@ request the **CVE** (optional, and the advisory is the only place to ask) and person then has to accept, so an unadded reporter is simply never credited, and that is the failure nobody notices because nothing reports it. -### 6. Convert the card afterwards +### 6. After publication, turn the card into public tracking + +**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 +describes a vulnerability the advisory has not disclosed yet. Wait for step 5 +to finish. -Once the advisory is published, the work becomes ordinary board history and the -draft card becomes a real issue. +Once it has, the work becomes ordinary board history — by conversion for v2, by +filing for v1. ⚠️ **Convert FIRST — the order is not interchangeable.** GitHub's "Convert to issue" creates a **new** issue from the draft; there is no way to point an diff --git a/AGENTS.md b/AGENTS.md index b5944e819..63d6b8cf6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -342,7 +342,7 @@ node/field/option IDs, and the option-deletion hazard` was cut at `#28`, so 90 overflows, and drops the least-invoked entries **first** — which are exactly the model-invoked skills that must fire on their own. `verify:skills` prints the current cost against the budget recorded in `scripts/lib/skill-manifest.mjs` - (3,711/4,000 characters as of this writing) and fails when it is exceeded. Raise + (3,729/4,000 characters as of this writing) and fails when it is exceeded. Raise the budget deliberately, or tighten a description; each entry is capped at 1,536 characters regardless, so **put the key use case first**. @@ -365,7 +365,7 @@ skills; the rules are here. - **Before starting work, check the board for the relevant item.** - **Every board item is a real GitHub issue.** No draft cards. Before creating a new issue, check the board for a matching item — **never create a duplicate**. - - **The one exception is a GitHub security advisory**, which is tracked by a **draft card** titled `[GHSA-xxxx-yyyy-zzzz] - `. An advisory is private until it is published, so a real issue would disclose the vulnerability before a fix exists — the thing the whole advisory flow is for. The card is made by hand (no `PROJECT_TOKEN` exists in this org, and `GITHUB_TOKEN` cannot hold `organization projects: write`), and it is converted to a real issue once the advisory is published. The `[GHSA-` prefix is load-bearing: it is what the board audit's draft carve-out keys on, so **any other draft card is still a defect to delete**. The flow itself — verifying who owns the code path, accepting, the private fork, publishing — is the `security-advisory` skill. **Accepting and publishing an advisory are outward-facing and stay human-gated; never automate or bulk-apply either.** + - **The one exception is a GitHub security advisory**, which is tracked by a **draft card** titled `[GHSA-xxxx-yyyy-zzzz] - `. An advisory is private until it is published, so a real issue would disclose the vulnerability before a fix exists — the thing the whole advisory flow is for. The card is made by hand (no `PROJECT_TOKEN` exists in this org, and `GITHUB_TOKEN` cannot hold `organization projects: write`), and it becomes public tracking **once the advisory is published** — never merely once the fix ships, since the release can precede publication. How depends on the line: a `v2` issue is **converted** from the draft on #28, while a `v1` issue is **filed** on #11 and the #28 draft is deleted, because a draft cannot convert onto another board and a `v1` issue on #28 is a wrong-board card. An advisory affecting both lines produces one issue per line. The `[GHSA-` prefix is load-bearing: it is what the board audit's draft carve-out keys on, so **any other draft card is still a defect to delete**. The flow itself — verifying who owns the code path, accepting, the private fork, publishing — is the `security-advisory` skill. **Accepting and publishing an advisory are outward-facing and stay human-gated; never automate or bulk-apply either.** - **Only issues go on a board — never PRs.** A PR gets the `v2` label but is tracked through its linked issue's card (via `Closes #N`), not its own board item. - **Label by version — every issue and every PR, no exceptions.** Exactly one of `v1` (work targeting `v1/main`, the deprecated security-fix-only line) or `v2` (active development; the default for anything new). There is no unlabeled state and no "decide later": an issue with neither label belongs to no version line and is invisible to every version-filtered query. Set it at **create time** (`gh issue create --label v2 …`), never by backfilling. **If the target version isn't obvious, it's `v2`.** - **Label by type — exactly one of `bug` / `enhancement` / `documentation` / `chore` / `question`** on every issue you create or triage. The version label says which line the work belongs to; the type label says what kind of work it is, and the two are independent. Don't force the binary: pressing a docs task or a dependency pin into `enhancement` degrades it to "not a bug", at which point filtering by it stops telling you anything. A **PR** needs no type label — it is classified through the issue it closes.