From 943f37a40a861d099952b7f95739815057f72e1d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 5 Aug 2026 14:37:31 -0400 Subject: [PATCH 01/11] docs: make assigning a milestone the act that moves a card to Todo AGENTS.md had no rule linking a milestone to a board Status, and its two existing rules contradicted each other: every new issue was to be milestoned at create time, yet new items were to start in Incoming, with Todo reserved for work a maintainer had approved. If every issue is milestoned at birth, "has a milestone" carries no approval information and nothing records the moment of sign-off. Make assigning a milestone the approval act, giving one invariant: Incoming <=> no milestone; everything past it <=> milestoned. External reports and maintainer-filed issues then differ only in whether approval has already happened -- an outside reporter cannot set a label, milestone, or board field, so their issue arriving with none of them is normal rather than a defect to fix on arrival. - scope the "not created until five things" callout to maintainer-filed issues - split the Status bullets into the two cases - scope the milestone mandate to issues you create, stating the external exception explicitly - add a "Triaging externally-filed issues" section: the two passes, plus a gh/jq snippet that diffs open issues against the board to find unboarded ones - reword both prose descriptions of the Incoming/Todo line - reword the rubric's "Linked to a milestone" bonus as "already approved" and note it is the re-scoring case only; under the old rules it was a constant offset on every issue, shifting every band boundary down by one - update both gh recipes (#28 and #11) to default to Todo, with an Incoming variant for triage No mirror to .github/copilot-instructions.md: board and milestone mechanics are on that file's deliberately-absent list. Closes #1930 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --- AGENTS.md | 67 ++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b86db380..4eb96861 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,7 +148,9 @@ If you've already built a change locally, share the **prompt** you used and scre All work should be driven by items on the project board. -> **A v2 issue is not "created" until it is labeled `v2`, given a milestone, AND on board #28 with a Status *and* a Priority set.** Labeling alone is not enough — a label is a repo tag, the milestone is a release bucket, and the board is a separate org project. Applying `--label v2` does **not** add the item to the board, and adding it to the board does **not** set a Status or a Priority. All five are distinct steps; do all five (see the recipes below). **Only issues go on the board — never PRs.** A PR still gets the `v2` label, but it is tracked through its linked issue's card (via `Closes #N`), not its own board item. +> **A v2 issue *you* create is not "created" until it is labeled `v2`, given a milestone, AND on board #28 with a Status *and* a Priority set.** Labeling alone is not enough — a label is a repo tag, the milestone is a release bucket, and the board is a separate org project. Applying `--label v2` does **not** add the item to the board, and adding it to the board does **not** set a Status or a Priority. All five are distinct steps; do all five (see the recipes below). **Only issues go on the board — never PRs.** A PR still gets the `v2` label, but it is tracked through its linked issue's card (via `Closes #N`), not its own board item. +> +> **This describes an issue a maintainer files, not one an outside reporter files.** An external reporter can't set a label, a milestone, or a board field, so their issue arrives with none of them — that is normal and not a defect to fix at the moment it lands. It is brought into the system by [triage](#triaging-externally-filed-issues) instead. The two paths differ in exactly one thing: filing an issue yourself *is* approving it, so it starts in **Todo** with a milestone; an external report has not been approved by anyone yet, so it starts in **Incoming** with no milestone. - Before starting work, check the board for the relevant item. - **Every board item is a real GitHub issue.** Do not create draft items (board cards with no issue number). If you find work that needs tracking, create an actual issue and add that to the board. Before creating a new issue, check the board for a matching item to avoid duplicates — **never create a duplicate**. @@ -156,11 +158,12 @@ All work should be driven by items on the project board. - `v1` — work targeting `v1/main` (the deprecated line: security fixes only) - `v2` — work targeting `v2/main` (active development; the default for anything new) Set the label at **create time** — `gh issue create --label v2 ...`, `gh pr create --label v2 ...` — never by backfilling later, since unlabeled items are exactly the ones missed when filtering by version. **If the target version isn't obvious, it's `v2`**: v2 is where all new work goes, and `v1` is reserved for the narrow case of patching the deprecated line. Only ask when the issue is specifically a fix *for released v1 behavior* and it's unclear whether v2 still has the bug. Note the label is a repo tag and is **not** the board — see the callout above; a `v2` issue also needs a board card with a Status **and** a Priority (a `v1` one needs a Status; board #11 has no Priority field). -- **Prioritize every new issue.** Every new issue must have a Priority (Urgent, High, Medium, or Low) set at creation time. Priority is a **board field**, not a label, so it lives on the card and an unboarded issue has nowhere to store it. Derive it with the rubric in [Setting issue priority](#setting-issue-priority) rather than asserting it — an unscored "this feels urgent" is exactly what the rubric exists to replace. +- **Prioritize every board item.** Every issue must have a Priority (Urgent, High, Medium, or Low) on its card — set when the card is created, whether that's at issue-creation time (yours) or at triage (an external report). Priority is a **board field**, not a label, so it lives on the card and an unboarded issue has nowhere to store it. Derive it with the rubric in [Setting issue priority](#setting-issue-priority) rather than asserting it — an unscored "this feels urgent" is exactly what the rubric exists to replace. - **Add the issue to the board and set Status and Priority.** After creating an issue, add it to the board for its version — **`v2` → board #28**, **`v1` → board #11** — and set the fields. (PRs are never added to either board — they're tracked through their linked issue's card.) This is the step most easily forgotten because it needs several IDs — copy the recipes below verbatim, and take them from the section for the right board; the two projects' ids are not interchangeable. - - **New and untriaged → `Incoming`.** This is the **default status for a new item on either board.** An issue nobody has evaluated yet belongs in **Incoming**, not Todo. Todo means a maintainer approved it and it is ready to be picked up; using Todo as the inbox erases that distinction and quietly promotes unreviewed work into the queue. Anything filed by an outside reporter starts in Incoming. Work you are starting immediately goes straight to In Progress. + - **An issue you create → `Todo`.** You only file an issue for work you intend to happen, so filing it is approving it. It gets a milestone and lands in Todo, ready to be picked up. Work you are starting immediately goes straight to **In Progress** instead. + - **An externally-filed issue → `Incoming`, no milestone.** Nobody has evaluated it yet, so it is not approved and gets no release bucket. See [Triaging externally-filed issues](#triaging-externally-filed-issues). Never park an unreviewed report in Todo — Todo asserts a maintainer signed off, and using it as the inbox erases that distinction and quietly promotes unreviewed work into the queue. - **Priority is v2-only.** Board #28 has a Priority field; board #11 does not. A v1 issue gets a Status and nothing else. -- **Every new issue gets a milestone — no exceptions.** Set it at create time with `gh issue create --milestone ...`. **If the user didn't specify one, default to the current milestone**: the open milestone with the nearest due date. Never leave an issue unmilestoned pending a decision — an unmilestoned issue drops out of release planning silently, the same way an unlabeled one drops out of version filtering. Moving it later is one command; noticing it was never set is the hard part. Get the current milestone with: +- **Every issue you create gets a milestone — no exceptions.** Set it at create time with `gh issue create --milestone <title> ...`, and place it in **Todo**. **If the user didn't specify one, default to the current milestone**: the open milestone with the nearest due date. Never leave an issue you filed unmilestoned pending a decision — an unmilestoned issue drops out of release planning silently, the same way an unlabeled one drops out of version filtering. Moving it later is one command; noticing it was never set is the hard part. (An **externally-filed** issue is the deliberate exception: it stays unmilestoned in Incoming until a maintainer approves it — there, the *absence* of a milestone is the signal that nobody has scheduled it yet.) Get the current milestone with: ```sh # Open milestones, soonest due date first — the first row is the current one. @@ -179,6 +182,32 @@ All work should be driven by items on the project board. - **On merge of a v2 PR, manually close its issue and move the board item to Done** (option id `259d6aab`), since auto-close won't fire on `v2/main`. Keep the `Closes #N` line anyway so the issues close automatically if/when `v2/main` is eventually merged to `main`. - If new tasks are discovered or requested during development, create issues and add them to the board. +### Triaging externally-filed issues + +An outside reporter has no board access, so their issue lands with **no label, no milestone, and no card**. That is the expected arrival state, not a backlog of defects — the issue enters the system through triage, in two distinct passes. + +**Pass 1 — sweep them onto the board (no approval implied).** Find the open issues with no card and bring each one in: + +1. Apply the version label (`v2` unless it's a fix for released v1 behavior — see [Label by version](#issue-driven-work-style)). +2. Add it to the board for that version. +3. Set Status to **`Incoming`**. +4. Set Priority with the [rubric](#setting-issue-priority) (v2 only). This is an *assessment*, not an approval — it's how the queue gets ordered for the maintainer who reviews it next. +5. **Leave the milestone unset.** Nobody has committed to shipping it yet, and an empty milestone is precisely what marks it as awaiting review. + +Find the unboarded ones by diffing the open issues against the board: + +```sh +gh issue list --repo modelcontextprotocol/inspector --state open --limit 1000 --json number > /tmp/open.json +gh project item-list 28 --owner modelcontextprotocol --format json --limit 700 \ + | jq -r --slurpfile o /tmp/open.json \ + '[.items[]|select(.content.type=="Issue")|.content.number] as $B + | [$o[0][].number] - $B | "unboarded: \(.)"' +``` + +**Pass 2 — approve what should ship.** A maintainer reads the Incoming column and, for each issue worth doing, **assigns a milestone** and moves the card to **Todo** (or **In Progress** if picking it up now). That is the whole approval gesture; see the [Status recipe](#v2-board-28-gh-recipes). + +Issues that shouldn't ship stay in Incoming (or get closed). **Incoming is the review queue, and "milestoned" is the line between reviewed and not** — which is why the milestone stays off in pass 1 and why the rubric can treat a milestone as a real signal rather than a formality every issue carries. + ## Setting issue priority Every issue gets a **Priority on its board card**, set when you add the issue to the board. Score it rather than assert it: rate two axes 1–5, add the signal bonuses, and read the total off the band table. The point is that two people triaging the same issue land in the same place, and that the reasoning survives in a form someone can argue with later. @@ -219,7 +248,7 @@ Every issue gets a **Priority on its board card**, set when you add the issue to **Signal indicators (bonuses, +1 each — not an axis of their own).** These are corroborating evidence that the two axes may have undercounted, so they adjust the total rather than standing alone: - Carries a `bug` or security-related label -- Linked to a milestone +- Linked to a milestone — i.e. **already approved** by a maintainer. Note this is the *re-scoring* case: an externally-filed issue being scored in triage has no milestone yet by rule, so it never earns this one. If you find yourself applying it to every issue in a batch, the milestone is being used as a formality rather than as approval, and the bonus has become a constant that discriminates nothing. - High engagement (many comments or reactions) - Assigned to someone - A sub-issue of a larger epic @@ -270,7 +299,7 @@ Don't lean on GitHub's permission gate to enforce this. Whether an outside repor - v2 - https://github.com/orgs/modelcontextprotocol/projects/28 (active board — all new work goes here) - v1 - https://github.com/orgs/modelcontextprotocol/projects/11 (legacy inspector version, no new activity except security fixes) - **Both boards start new items in `Incoming`.** A card only leaves Incoming when a maintainer has looked at it and approved the work — that is what Todo means on either board. The two boards are otherwise separate projects with their own field and option ids; never reuse one board's ids against the other (they are rejected with "option Id does not belong to the field", so the mistake is at least loud). + **On both boards, `Incoming` is the review queue for externally-filed issues** — swept in at triage with a Priority but deliberately **no milestone** (see [Triaging externally-filed issues](#triaging-externally-filed-issues)). A card only leaves Incoming when a maintainer has looked at it and approved the work by assigning a milestone, at which time it should move to **Todo** — unless the maintainer has chosen to work on it, in which case it should move to **In Progress**. Assigning the milestone *is* the approval act, so the two always go together: a milestoned card sitting in Incoming is a card whose approval was never recorded, and a Todo card with no milestone claims an approval nobody made. An issue **you** file skips Incoming entirely — filing it is approving it, so it starts milestoned in Todo. The two boards are otherwise separate projects with their own field and option ids; never reuse one board's ids against the other (they are rejected with "option Id does not belong to the field", so the mistake is at least loud). #### V2 board (#28) `gh` recipes @@ -298,7 +327,7 @@ Status option IDs (`--single-select-option-id`) — **last verified 2026-08-01** | In Review | `159c8a02` | | Done | `259d6aab` | -Use **Incoming** for newly filed, untriaged work, **Todo** once a maintainer has approved it and it's ready to pick up, **In Progress** for general active work (regardless of surface), **In Review** once a PR is open, and **Done** on merge. The Incoming/Todo line is the one that matters: Todo asserts approval, so an unreviewed issue parked there is a false claim that someone signed off on it. +Use **Incoming** for an externally-filed issue awaiting review (no milestone yet), **Todo** once a maintainer has approved it by assigning a milestone — including an issue you filed yourself, which starts here — **In Progress** for general active work (regardless of surface), **In Review** once a PR is open, and **Done** on merge. The Incoming/Todo line is the one that matters: Todo asserts approval, so an unreviewed issue parked there is a false claim that someone signed off on it. The milestone is the machine-checkable form of that claim — Incoming ⇔ no milestone, everything past it ⇔ milestoned. Priority option IDs (`--single-select-option-id`) — **last verified 2026-08-01**. Derive the level with the rubric in [Setting issue priority](#setting-issue-priority); don't eyeball it. @@ -364,16 +393,26 @@ gh project item-edit \ --single-select-option-id 195df262 ``` -The full one-liner for a **new** issue — add it, then set Status and Priority (both are required; here Incoming + Medium): +The full one-liner for an issue **you just created** — add it, then set Status and Priority (both required). It goes to **Todo**, because filing it was the approval, and it already carries the milestone you passed to `gh issue create`: ```sh ITEM_ID=$(gh project item-add 28 --owner modelcontextprotocol --url <issue-url> --format json --jq '.id') -# Status → Incoming -gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iI8c --single-select-option-id 721a3d4c +# Status → Todo (an issue you filed is approved by definition) +gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iI8c --single-select-option-id fbdaf21e # Priority → Medium gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iJE4 --single-select-option-id da944a9c ``` +For an **externally-filed** issue being swept in at triage, the only difference is the Status option — **Incoming** (`721a3d4c`) instead of Todo — and that you do **not** set a milestone: + +```sh +ITEM_ID=$(gh project item-add 28 --owner modelcontextprotocol --url <issue-url> --format json --jq '.id') +# Status → Incoming (awaiting maintainer review; no milestone yet) +gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iI8c --single-select-option-id 721a3d4c +# Priority → Medium (an assessment for queue ordering, not an approval) +gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iJE4 --single-select-option-id da944a9c +``` + Each `item-edit` sets **one** field, so setting both takes two calls — there is no combined form. For an issue **already on the board** (moving an existing card, e.g. to **In Review** when its PR opens, or re-scoring its Priority), look its item id up by issue number instead of re-adding it. Keep `--limit` above the board's item count (~265 as of 2026-08-01) — past it `item-list` truncates silently, `select` matches nothing, and `item-edit --id ""` fails with an opaque node-resolution error rather than saying the limit was too low: @@ -388,7 +427,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i #### V1 board (#11) `gh` recipes -The v1 line takes **security fixes only**, so this board sees little traffic — but a v1 issue still gets a card, and it starts in **Incoming** like a v2 one. Board #11 is a separate org project with **its own ids**; none of the #28 ids above work here. +The v1 line takes **security fixes only**, so this board sees little traffic — but a v1 issue still gets a card, and the same Incoming/Todo split applies: one **you** file starts in **Todo**, an **externally-filed** one starts in **Incoming** awaiting review. Board #11 is a separate org project with **its own ids**; none of the #28 ids above work here. | Thing | ID | | --- | --- | @@ -408,9 +447,11 @@ Status option IDs — **last verified 2026-08-01**. There is **no Priority field on this board** — the priority rubric applies to v2 only. Don't try to set one here; the field id doesn't exist. ```sh -# Add a v1 issue to board #11 and put it in Incoming. +# Add a v1 issue to board #11. Swap the option id for the case you're in: +# 831820cf = Incoming — an externally-filed issue awaiting review (no milestone) +# f75ad846 = Todo — an issue you filed yourself (milestoned at create time) ITEM_ID=$(gh project item-add 11 --owner modelcontextprotocol --url <issue-url> --format json --jq '.id') -gh project item-edit --project-id PVT_kwDOCt2Azc4BA5sz --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BA5szzgzkS-g --single-select-option-id 831820cf +gh project item-edit --project-id PVT_kwDOCt2Azc4BA5sz --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BA5szzgzkS-g --single-select-option-id f75ad846 ``` The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe above apply to **this board too** — same mutation, same failure mode, different ids. Note that three cards on #11 already carry no Status; that predates the `Incoming` addition (verified by before/after diff on 2026-08-01) and is not evidence of an orphaning event. From 04f1a56f934ce5144612e927b7d63025a3098aaa Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Wed, 5 Aug 2026 15:40:49 -0400 Subject: [PATCH 02/11] =?UTF-8?q?docs:=20make=20triage=20runnable=20?= =?UTF-8?q?=E2=80=94=20state-based=20test,=20both-board=20sweep,=20board?= =?UTF-8?q?=20audit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the same issue: the triage rules were clear on the happy path but three gaps meant a bare "triage new issues" would diverge from what a maintainer actually does. Define triage by STATE, not authorship. An issue needs triage when it arrives with no board card and no milestone — whoever filed it. Framing it as "externally-filed" was wrong: a maintainer opening an issue by hand in the GitHub UI lands in exactly the same state, because write access makes the board reachable, not automatic. What puts an issue in Todo is somebody performing the approval, not who owns the account. Renamed the section and reworded every dependent passage accordingly. Fix the discovery query, which diffed open issues against board #28 only. Verified against the live boards: it reports #1929 as unboarded when that issue is correctly carded on #11, so following it literally double-boards the issue — recreating a defect a past sweep introduced. Now diffs against the union of both boards, filtered by .content.repository (an org project can hold other repos' issues; #11 carries one from modelcontextprotocol/ servers), and prints each hit's destination, since an unboarded issue that already has a milestone is approved and belongs in Todo, not Incoming. Add "The board audit" — the other drift classes a single-issue rule can't catch (double-boarded, non-Issue items, statusless cards, the Incoming ⇔ milestone invariant in both directions, wrong board for label, missing version label, missing Priority), as a table of invariant + fix plus one runnable jq check that should print 0 across the board. Both snippets were executed verbatim against the live boards. Add "Recording the score". The rubric claimed its reasoning "survives in a form someone can argue with later", but the board stores only the result and is private, so nothing kept that promise. Triage now posts the axes, the bonuses claimed, and the total as a comment. Also scope pass 2 explicitly to a human — deciding what ships in which release is not inferable from a rubric — and correct the stale note about three statusless cards on #11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --- AGENTS.md | 141 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 117 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4eb96861..29819c3f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -150,7 +150,7 @@ All work should be driven by items on the project board. > **A v2 issue *you* create is not "created" until it is labeled `v2`, given a milestone, AND on board #28 with a Status *and* a Priority set.** Labeling alone is not enough — a label is a repo tag, the milestone is a release bucket, and the board is a separate org project. Applying `--label v2` does **not** add the item to the board, and adding it to the board does **not** set a Status or a Priority. All five are distinct steps; do all five (see the recipes below). **Only issues go on the board — never PRs.** A PR still gets the `v2` label, but it is tracked through its linked issue's card (via `Closes #N`), not its own board item. > -> **This describes an issue a maintainer files, not one an outside reporter files.** An external reporter can't set a label, a milestone, or a board field, so their issue arrives with none of them — that is normal and not a defect to fix at the moment it lands. It is brought into the system by [triage](#triaging-externally-filed-issues) instead. The two paths differ in exactly one thing: filing an issue yourself *is* approving it, so it starts in **Todo** with a milestone; an external report has not been approved by anyone yet, so it starts in **Incoming** with no milestone. +> **This describes an issue created through the flow below — not every issue that appears in the repo.** An issue opened by hand, in the GitHub UI, arrives with no label, no milestone, and no card. That is true of an outside reporter's (they have no board access) *and* of a maintainer's (write access makes the board reachable, not automatic). Either way it is normal on arrival, not a defect to fix the moment it lands; it comes into the system through [triage](#triaging-unboarded-issues) instead. The two paths differ in exactly one thing: doing the create flow *is* the approval, so the issue starts in **Todo** with a milestone; an issue that arrives unboarded and unmilestoned has been approved by nobody, so it starts in **Incoming** with no milestone. - Before starting work, check the board for the relevant item. - **Every board item is a real GitHub issue.** Do not create draft items (board cards with no issue number). If you find work that needs tracking, create an actual issue and add that to the board. Before creating a new issue, check the board for a matching item to avoid duplicates — **never create a duplicate**. @@ -158,12 +158,12 @@ All work should be driven by items on the project board. - `v1` — work targeting `v1/main` (the deprecated line: security fixes only) - `v2` — work targeting `v2/main` (active development; the default for anything new) Set the label at **create time** — `gh issue create --label v2 ...`, `gh pr create --label v2 ...` — never by backfilling later, since unlabeled items are exactly the ones missed when filtering by version. **If the target version isn't obvious, it's `v2`**: v2 is where all new work goes, and `v1` is reserved for the narrow case of patching the deprecated line. Only ask when the issue is specifically a fix *for released v1 behavior* and it's unclear whether v2 still has the bug. Note the label is a repo tag and is **not** the board — see the callout above; a `v2` issue also needs a board card with a Status **and** a Priority (a `v1` one needs a Status; board #11 has no Priority field). -- **Prioritize every board item.** Every issue must have a Priority (Urgent, High, Medium, or Low) on its card — set when the card is created, whether that's at issue-creation time (yours) or at triage (an external report). Priority is a **board field**, not a label, so it lives on the card and an unboarded issue has nowhere to store it. Derive it with the rubric in [Setting issue priority](#setting-issue-priority) rather than asserting it — an unscored "this feels urgent" is exactly what the rubric exists to replace. +- **Prioritize every board item.** Every issue must have a Priority (Urgent, High, Medium, or Low) on its card — set when the card is created, whether that's at issue-creation time (yours) or at triage (an unboarded one). Priority is a **board field**, not a label, so it lives on the card and an unboarded issue has nowhere to store it. Derive it with the rubric in [Setting issue priority](#setting-issue-priority) rather than asserting it — an unscored "this feels urgent" is exactly what the rubric exists to replace. - **Add the issue to the board and set Status and Priority.** After creating an issue, add it to the board for its version — **`v2` → board #28**, **`v1` → board #11** — and set the fields. (PRs are never added to either board — they're tracked through their linked issue's card.) This is the step most easily forgotten because it needs several IDs — copy the recipes below verbatim, and take them from the section for the right board; the two projects' ids are not interchangeable. - **An issue you create → `Todo`.** You only file an issue for work you intend to happen, so filing it is approving it. It gets a milestone and lands in Todo, ready to be picked up. Work you are starting immediately goes straight to **In Progress** instead. - - **An externally-filed issue → `Incoming`, no milestone.** Nobody has evaluated it yet, so it is not approved and gets no release bucket. See [Triaging externally-filed issues](#triaging-externally-filed-issues). Never park an unreviewed report in Todo — Todo asserts a maintainer signed off, and using it as the inbox erases that distinction and quietly promotes unreviewed work into the queue. + - **An issue that arrives unboarded and unmilestoned → `Incoming`.** Nobody has evaluated it yet, so it is not approved and gets no release bucket — whoever filed it. See [Triaging unboarded issues](#triaging-unboarded-issues). Never park an unreviewed issue in Todo — Todo asserts a maintainer signed off, and using it as the inbox erases that distinction and quietly promotes unreviewed work into the queue. - **Priority is v2-only.** Board #28 has a Priority field; board #11 does not. A v1 issue gets a Status and nothing else. -- **Every issue you create gets a milestone — no exceptions.** Set it at create time with `gh issue create --milestone <title> ...`, and place it in **Todo**. **If the user didn't specify one, default to the current milestone**: the open milestone with the nearest due date. Never leave an issue you filed unmilestoned pending a decision — an unmilestoned issue drops out of release planning silently, the same way an unlabeled one drops out of version filtering. Moving it later is one command; noticing it was never set is the hard part. (An **externally-filed** issue is the deliberate exception: it stays unmilestoned in Incoming until a maintainer approves it — there, the *absence* of a milestone is the signal that nobody has scheduled it yet.) Get the current milestone with: +- **Every issue you create gets a milestone — no exceptions.** Set it at create time with `gh issue create --milestone <title> ...`, and place it in **Todo**. **If the user didn't specify one, default to the current milestone**: the open milestone with the nearest due date. Never leave an issue you filed unmilestoned pending a decision — an unmilestoned issue drops out of release planning silently, the same way an unlabeled one drops out of version filtering. Moving it later is one command; noticing it was never set is the hard part. (An issue that arrives **unboarded** is the deliberate exception: it stays unmilestoned in Incoming until a maintainer approves it — there, the *absence* of a milestone is the signal that nobody has scheduled it yet.) Get the current milestone with: ```sh # Open milestones, soonest due date first — the first row is the current one. @@ -182,32 +182,106 @@ All work should be driven by items on the project board. - **On merge of a v2 PR, manually close its issue and move the board item to Done** (option id `259d6aab`), since auto-close won't fire on `v2/main`. Keep the `Closes #N` line anyway so the issues close automatically if/when `v2/main` is eventually merged to `main`. - If new tasks are discovered or requested during development, create issues and add them to the board. -### Triaging externally-filed issues +### Triaging unboarded issues -An outside reporter has no board access, so their issue lands with **no label, no milestone, and no card**. That is the expected arrival state, not a backlog of defects — the issue enters the system through triage, in two distinct passes. +**An issue needs triage when it arrives with no board card and no milestone — regardless of who filed it.** That is the whole test, and it is deliberately about *state*, not authorship. -**Pass 1 — sweep them onto the board (no approval implied).** Find the open issues with no card and bring each one in: +It's tempting to frame this as "issues from outside reporters," but that's the wrong line. An outside reporter has no board access, so their issue necessarily lands this way — but so does a **maintainer's**, whenever they open one by hand instead of through the [documented create flow](#issue-driven-work-style). Write access makes the board *reachable*, not automatic. What puts an issue in Todo is somebody performing the approval — labeling it, milestoning it, and carding it — and an issue nobody did that for has not been approved, whoever's name is on it. + +So don't check the author's permissions; check whether the work was done. Arriving unboarded and unmilestoned is a normal state, not a defect to fix at the moment it lands — the issue enters the system through triage, in two distinct passes. + +**"Triage new issues" means running pass 1 over every unboarded open issue, then the [board audit](#the-board-audit) below.** Pass 2 is a human judgment call and is never done unprompted. + +#### Pass 1 — sweep them onto the board (no approval implied) + +For each open issue with no card: 1. Apply the version label (`v2` unless it's a fix for released v1 behavior — see [Label by version](#issue-driven-work-style)). -2. Add it to the board for that version. +2. Add it to the board for that version — **`v2` → #28, `v1` → #11**. 3. Set Status to **`Incoming`**. -4. Set Priority with the [rubric](#setting-issue-priority) (v2 only). This is an *assessment*, not an approval — it's how the queue gets ordered for the maintainer who reviews it next. +4. Set Priority with the [rubric](#setting-issue-priority) (v2 only), and **record the score in a comment** (see [Recording the score](#recording-the-score)). This is an *assessment*, not an approval — it's how the queue gets ordered for the maintainer who reviews it next. 5. **Leave the milestone unset.** Nobody has committed to shipping it yet, and an empty milestone is precisely what marks it as awaiting review. -Find the unboarded ones by diffing the open issues against the board: +**The one exception is an unboarded issue that already carries a milestone.** Someone recorded the approval and only the card is missing, so don't demote it to Incoming — board it straight into **Todo**, keeping the milestone. Otherwise triage would silently un-approve work a maintainer had already scheduled. + +Find the unboarded ones by diffing the open issues against **both boards**. Diffing against #28 alone is wrong: a `v1` issue correctly carded on #11 is reported as unboarded and gets double-boarded (a real defect a past sweep introduced — #1929 reproduced it). ```sh -gh issue list --repo modelcontextprotocol/inspector --state open --limit 1000 --json number > /tmp/open.json -gh project item-list 28 --owner modelcontextprotocol --format json --limit 700 \ - | jq -r --slurpfile o /tmp/open.json \ - '[.items[]|select(.content.type=="Issue")|.content.number] as $B - | [$o[0][].number] - $B | "unboarded: \(.)"' +D=$(mktemp -d) +gh issue list --repo modelcontextprotocol/inspector --state open --limit 1000 \ + --json number,milestone > "$D/open.json" +# Union of BOTH boards, filtered to this repo — org boards can hold other repos' issues. +for P in 28 11; do + gh project item-list $P --owner modelcontextprotocol --format json --limit 700 \ + | jq '[.items[] | select(.content.type=="Issue" + and .content.repository=="modelcontextprotocol/inspector") + | .content.number]' +done | jq -s 'add' > "$D/boarded.json" +# Prints the destination too: milestoned already → Todo, otherwise → Incoming. +jq -r --slurpfile b "$D/boarded.json" \ + '.[] | select(.number as $n | ($b[0]|index($n))|not) + | "#\(.number)\t→ \(if .milestone then "Todo (has milestone \(.milestone.title))" else "Incoming" end)"' \ + "$D/open.json" ``` -**Pass 2 — approve what should ship.** A maintainer reads the Incoming column and, for each issue worth doing, **assigns a milestone** and moves the card to **Todo** (or **In Progress** if picking it up now). That is the whole approval gesture; see the [Status recipe](#v2-board-28-gh-recipes). +#### Pass 2 — approve what should ship + +A maintainer reads the Incoming column and, for each issue worth doing, **assigns a milestone** and moves the card to **Todo** (or **In Progress** if picking it up now). That is the whole approval gesture; see the [Status recipe](#v2-board-28-gh-recipes). + +This pass is **a judgment call reserved for a human** — deciding what ships in which release is not something to infer from a rubric. Never promote a card out of Incoming as part of a routine sweep; "triage new issues" stops at the end of pass 1. Issues that shouldn't ship stay in Incoming (or get closed). **Incoming is the review queue, and "milestoned" is the line between reviewed and not** — which is why the milestone stays off in pass 1 and why the rubric can treat a milestone as a real signal rather than a formality every issue carries. +#### The board audit + +Sweeping in the unboarded issues is only the most visible defect class. A board drifts in several other ways that no single-issue rule catches, so **finish a triage run with the audit below** — every check should print `0`. Each maps to a rule stated elsewhere in this document; a non-zero count means the board contradicts the 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 (`gh project item-delete`) | +| Non-Issue items | **Only issues go on a board** — never PRs, never drafts | Delete the item; the PR is tracked via its issue's `Closes #N` | +| No Status | Every card carries a Status | Set one — `Incoming` if unmilestoned, else by where it actually is | +| `Incoming` **with** a milestone | Incoming ⇔ no milestone | Approval was never recorded: move to **Todo**, or clear the milestone | +| Past Incoming **without** a milestone | 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 | +| No version label | Every issue carries exactly one of `v1`/`v2` | Apply it (`v2` unless it's a fix for released v1 behavior) | +| No Priority (#28) | Every board item is prioritized | Score it with the [rubric](#setting-issue-priority) | + +```sh +D=$(mktemp -d); R=modelcontextprotocol/inspector +gh issue list --repo $R --state open --limit 1000 --json number,labels,milestone > "$D/i.json" +for P in 28 11; do gh project item-list $P --owner modelcontextprotocol \ + --format json --limit 700 > "$D/b$P.json"; done +jq -nr --slurpfile o "$D/i.json" --slurpfile a "$D/b28.json" --slurpfile b "$D/b11.json" --arg R "$R" ' + ($o[0] | map({key:(.number|tostring), + value:{lab:[.labels[].name], ms:(.milestone.title // null)}}) | from_entries) as $I + | def own($s): [$s[].items[] | select(.content.repository==$R)]; + [own($a)[] | select(.content.type=="Issue") | {n:.content.number, s:.status, p:.priority}] as $B28 + | [own($b)[] | select(.content.type=="Issue") | {n:.content.number, s:.status}] as $B11 + | def ms($n): ($I[($n|tostring)].ms // null); + def lab($n): ($I[($n|tostring)].lab // []); + def open($n): ($I[($n|tostring)] != null); + { + "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], + "no Status": [($B28[], $B11[]) | select(.s==null) | .n], + "Incoming w/ milestone": [($B28[], $B11[]) | select(.s=="Incoming" and ms(.n)!=null) | .n], + "past Incoming, no ms": [($B28[], $B11[]) | select(.s!=null and .s!="Incoming" and .s!="Done" + and open(.n) and ms(.n)==null) | .n], + "v1 label on #28": [$B28[] | select(lab(.n)|index("v1")) | .n], + "v2 label on #11": [$B11[] | select(lab(.n)|index("v2")) | .n], + "open, no version label":[$o[0][] | select(([.labels[].name]|index("v1") or index("v2"))|not) | .number], + "#28 open, no Priority": [$B28[] | select(.p==null and open(.n)) | .n] + } | to_entries[] | "\(.value|length)\t\(.key)\t\(.value[0:10])"' +``` + +Two things the queries must account for, both learned the hard way: + +- **Filter by repository.** These are **org** projects and can hold cards from any repo in the org — board #11 currently carries one `modelcontextprotocol/servers` issue. Without the `.content.repository` filter it reads as a statusless-card defect and "fixing" it would mean editing another repo's tracking. +- **Only open issues have a `$I` entry.** A closed issue still has a card (correctly, in `Done`), so a check that treats "no milestone found" as a violation must gate on `open(.n)` or it flags every closed card. + +**Do not "fix" a `Done` card that is missing a milestone.** Cards predating a rule are not defects to backfill in bulk — the audit exists to stop *new* drift, and rewriting settled history destroys the record of when a rule started being enforced. + ## Setting issue priority Every issue gets a **Priority on its board card**, set when you add the issue to the board. Score it rather than assert it: rate two axes 1–5, add the signal bonuses, and read the total off the band table. The point is that two people triaging the same issue land in the same place, and that the reasoning survives in a form someone can argue with later. @@ -248,7 +322,7 @@ Every issue gets a **Priority on its board card**, set when you add the issue to **Signal indicators (bonuses, +1 each — not an axis of their own).** These are corroborating evidence that the two axes may have undercounted, so they adjust the total rather than standing alone: - Carries a `bug` or security-related label -- Linked to a milestone — i.e. **already approved** by a maintainer. Note this is the *re-scoring* case: an externally-filed issue being scored in triage has no milestone yet by rule, so it never earns this one. If you find yourself applying it to every issue in a batch, the milestone is being used as a formality rather than as approval, and the bonus has become a constant that discriminates nothing. +- Linked to a milestone — i.e. **already approved** by a maintainer. Note this is the *re-scoring* case: an issue being scored in a pass-1 triage has no milestone yet by rule, so it never earns this one. If you find yourself applying it to every issue in a batch, the milestone is being used as a formality rather than as approval, and the bonus has become a constant that discriminates nothing. - High engagement (many comments or reactions) - Assigned to someone - A sub-issue of a larger epic @@ -267,6 +341,25 @@ Note that severity alone doesn't reach Urgent: a 5/5 with no corroborating signa Set the resulting level on the board card with the Priority recipe in the [V2 board (#28) `gh` recipes](#v2-board-28-gh-recipes) below. +### Recording the score + +The board stores only the *result* — a card reading `High` with no trace of how it got there. Since [the boards are private](#trust-boundary-who-can-set-what), that result is also invisible to the reporter. So **when you score an issue during triage, post the arithmetic as a comment on the issue.** Without it the rubric's promise — that the reasoning "survives in a form someone can argue with later" — is not kept by anything, and a later re-scoring has no way to tell a considered judgment from a guess. + +Keep it to the two axes, the bonuses claimed, and the total: + +```sh +gh issue comment <N> --repo modelcontextprotocol/inspector --body \ +'**Triage:** Priority **Medium** (total 7) + +- Severity 3 — a real feature is broken; workaround is partial +- Urgency 2 — wanted eventually, nothing blocked on it +- Bonuses: +1 `bug` label, +1 reporter set Fields → Priority to High + +Board: #28, Status `Incoming` (awaiting maintainer review — no milestone yet).' +``` + +Name the bonuses you claimed rather than just summing them — the milestone bonus in particular should be conspicuously absent on a pass-1 triage, and a comment that lists it is a visible sign the [approval semantics](#triaging-unboarded-issues) were misapplied. + ### Trust boundary: who can set what **The boards are private** (`public: false`, both #28 and #11 — verified 2026-08-01). The Status and Priority a maintainer assigns are visible only to people with project access: a reporter cannot see them, cannot set them, and will never learn how their issue was scored. Board priority is a maintainers' working queue, not a published commitment. @@ -299,7 +392,7 @@ Don't lean on GitHub's permission gate to enforce this. Whether an outside repor - v2 - https://github.com/orgs/modelcontextprotocol/projects/28 (active board — all new work goes here) - v1 - https://github.com/orgs/modelcontextprotocol/projects/11 (legacy inspector version, no new activity except security fixes) - **On both boards, `Incoming` is the review queue for externally-filed issues** — swept in at triage with a Priority but deliberately **no milestone** (see [Triaging externally-filed issues](#triaging-externally-filed-issues)). A card only leaves Incoming when a maintainer has looked at it and approved the work by assigning a milestone, at which time it should move to **Todo** — unless the maintainer has chosen to work on it, in which case it should move to **In Progress**. Assigning the milestone *is* the approval act, so the two always go together: a milestoned card sitting in Incoming is a card whose approval was never recorded, and a Todo card with no milestone claims an approval nobody made. An issue **you** file skips Incoming entirely — filing it is approving it, so it starts milestoned in Todo. The two boards are otherwise separate projects with their own field and option ids; never reuse one board's ids against the other (they are rejected with "option Id does not belong to the field", so the mistake is at least loud). + **On both boards, `Incoming` is the review queue for issues that arrived unboarded** — swept in at triage with a Priority but deliberately **no milestone** (see [Triaging unboarded issues](#triaging-unboarded-issues)). A card only leaves Incoming when a maintainer has looked at it and approved the work by assigning a milestone, at which time it should move to **Todo** — unless the maintainer has chosen to work on it, in which case it should move to **In Progress**. Assigning the milestone *is* the approval act, so the two always go together: a milestoned card sitting in Incoming is a card whose approval was never recorded, and a Todo card with no milestone claims an approval nobody made. An issue created through the documented flow skips Incoming entirely — doing that flow is approving it, so it starts milestoned in Todo. The two boards are otherwise separate projects with their own field and option ids; never reuse one board's ids against the other (they are rejected with "option Id does not belong to the field", so the mistake is at least loud). #### V2 board (#28) `gh` recipes @@ -327,7 +420,7 @@ Status option IDs (`--single-select-option-id`) — **last verified 2026-08-01** | In Review | `159c8a02` | | Done | `259d6aab` | -Use **Incoming** for an externally-filed issue awaiting review (no milestone yet), **Todo** once a maintainer has approved it by assigning a milestone — including an issue you filed yourself, which starts here — **In Progress** for general active work (regardless of surface), **In Review** once a PR is open, and **Done** on merge. The Incoming/Todo line is the one that matters: Todo asserts approval, so an unreviewed issue parked there is a false claim that someone signed off on it. The milestone is the machine-checkable form of that claim — Incoming ⇔ no milestone, everything past it ⇔ milestoned. +Use **Incoming** for an issue that arrived unboarded and is awaiting review (no milestone yet), **Todo** once a maintainer has approved it by assigning a milestone — including an issue you created through the documented flow, which starts here — **In Progress** for general active work (regardless of surface), **In Review** once a PR is open, and **Done** on merge. The Incoming/Todo line is the one that matters: Todo asserts approval, so an unreviewed issue parked there is a false claim that someone signed off on it. The milestone is the machine-checkable form of that claim — Incoming ⇔ no milestone, everything past it ⇔ milestoned. Priority option IDs (`--single-select-option-id`) — **last verified 2026-08-01**. Derive the level with the rubric in [Setting issue priority](#setting-issue-priority); don't eyeball it. @@ -403,7 +496,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BJVxtzg5iJE4 --single-select-option-id da944a9c ``` -For an **externally-filed** issue being swept in at triage, the only difference is the Status option — **Incoming** (`721a3d4c`) instead of Todo — and that you do **not** set a milestone: +For an issue **swept in at triage** (arrived unboarded, no milestone), the only difference is the Status option — **Incoming** (`721a3d4c`) instead of Todo — and that you do **not** set a milestone: ```sh ITEM_ID=$(gh project item-add 28 --owner modelcontextprotocol --url <issue-url> --format json --jq '.id') @@ -427,7 +520,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i #### V1 board (#11) `gh` recipes -The v1 line takes **security fixes only**, so this board sees little traffic — but a v1 issue still gets a card, and the same Incoming/Todo split applies: one **you** file starts in **Todo**, an **externally-filed** one starts in **Incoming** awaiting review. Board #11 is a separate org project with **its own ids**; none of the #28 ids above work here. +The v1 line takes **security fixes only**, so this board sees little traffic — but a v1 issue still gets a card, and the same Incoming/Todo split applies: one created through the documented flow starts in **Todo**, one that arrived unboarded starts in **Incoming** awaiting review. Board #11 is a separate org project with **its own ids**; none of the #28 ids above work here. | Thing | ID | | --- | --- | @@ -448,13 +541,13 @@ There is **no Priority field on this board** — the priority rubric applies to ```sh # Add a v1 issue to board #11. Swap the option id for the case you're in: -# 831820cf = Incoming — an externally-filed issue awaiting review (no milestone) -# f75ad846 = Todo — an issue you filed yourself (milestoned at create time) +# 831820cf = Incoming — arrived unboarded, awaiting review (no milestone) +# f75ad846 = Todo — created through the documented flow (milestoned at create time) ITEM_ID=$(gh project item-add 11 --owner modelcontextprotocol --url <issue-url> --format json --jq '.id') gh project item-edit --project-id PVT_kwDOCt2Azc4BA5sz --id "$ITEM_ID" --field-id PVTSSF_lADOCt2Azc4BA5szzgzkS-g --single-select-option-id f75ad846 ``` -The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe above apply to **this board too** — same mutation, same failure mode, different ids. Note that three cards on #11 already carry no Status; that predates the `Incoming` addition (verified by before/after diff on 2026-08-01) and is not evidence of an orphaning event. +The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe above apply to **this board too** — same mutation, same failure mode, different ids. Note that board #11 also carries **one card from another repo** (`modelcontextprotocol/servers`), which has no Status. That is why every audit query filters on `.content.repository` — an org project can hold items from any repo in the org, and an unfiltered check reports that card as a statusless-card defect. It is not ours to fix. (The three *inspector* cards that carried no Status on 2026-08-01 predated the `Incoming` addition and have since been set; neither case was evidence of an orphaning event.) ### Always test new or modified code - Ensure all code has corresponding tests From a9238426a6fdb3561163b1686dc5b38f47f7a582 Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Wed, 5 Aug 2026 16:25:45 -0400 Subject: [PATCH 03/11] =?UTF-8?q?docs:=20Done=20means=20shipped=20?= =?UTF-8?q?=E2=80=94=20remove,=20don't=20park,=20issues=20closed=20any=20o?= =?UTF-8?q?ther=20way?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A board card in Done asserts that the work shipped, and Done is read as the record of what a milestone actually delivered. Nothing said so, so an issue closed as a duplicate or not-planned was being moved to Done like any other close — which makes that record wrong in a way no later reader can detect: counting Done cards can no longer tell a shipped fix from a report closed as a duplicate of one. New rule: exactly two things earn a card a place in Done — its PR merged, or it is a parent whose last sub-issue closed. Every other close (duplicate, won't fix, not planned, obsolete, superseded) means nothing shipped, so the card is deleted instead. Deleting a card touches only the board; the issue keeps its labels and comments and stays searchable, so nothing is lost. Also notes that the close reason is the machine-readable form of the same distinction, and that `gh issue close --reason` cannot express `duplicate` — it accepts only completed/not planned, so duplicate must be set through the API or the web UI's "Mark as duplicate" (which also records a duplicate-of link). Adds the invariant to the board audit as a tenth check. That required the audit to read closed issues too, so its fetch moves to --state all with stateReason, `open(n)` becomes a state test rather than "present in the map", and --limit must now clear the repo's total issue count, not just the open ones. The two wrong-board label checks are scoped to open issues in the same pass — over all history they flag four long-closed cards whose labels nobody intends to rewrite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --- AGENTS.md | 51 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 29819c3f..6a04eb37 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -180,6 +180,21 @@ All work should be driven by items on the project board. - **Attach screenshots as proof of functionality.** Any change to the web UI or the TUI must show its result: capture before/after screenshots (or a short GIF for an interaction) and put them in a **`pr-screenshots/` folder off the repo root**, creating it if it doesn't exist. That folder is **gitignored** — the images are working artifacts staged for upload, never committed to the source tree — so attach them to the PR body from there rather than referencing an in-repo path. Name them for what they show (`tools-tab-before.png`, `tools-tab-after.png`), not `Screenshot 2026-07-31 at 14.02.11.png`. - **Link the PR to its issue — mandatory for every PR, from anyone.** No PR is opened without an issue to reference; if one doesn't exist yet, create it first (labeled and on the board) rather than opening the PR and backfilling. Note also that only the **repo maintainers** open PRs at all (see [Contributing](#contributing)) — everyone else files a detailed issue. The PR body's **first line must be `Closes #<ISSUE_NUMBER>`**. ⚠️ Note: closing keywords only auto-link/auto-close for PRs targeting the repo's **default branch** (`main`). Because v2 PRs target `v2/main` (a non-default branch), `Closes #N` there is only a cross-reference — it will **not** create a hard link or close the issue on merge. (There is no `gh` flag for manual linking — `gh pr edit` has no `--add-issue`; closing keywords are the only mechanism GitHub exposes, and they're gated to the default branch.) - **On merge of a v2 PR, manually close its issue and move the board item to Done** (option id `259d6aab`), since auto-close won't fire on `v2/main`. Keep the `Closes #N` line anyway so the issues close automatically if/when `v2/main` is eventually merged to `main`. +- **`Done` means the work shipped. An issue closed for any other reason is REMOVED from the board, not moved to Done.** Exactly two things earn a card a place in Done: + 1. Its **PR merged**, or + 2. it is a **parent whose last sub-issue closed** — the work shipped across its children rather than through one PR of its own. + + Anything else — duplicate, won't fix, not planned, obsolete, superseded — means nothing shipped, so **delete the card**: + + ```sh + ITEM_ID=$(gh project item-list 28 --owner modelcontextprotocol --format json --limit 500 \ + --jq '.items[] | select(.content.number==<ISSUE_NUMBER>) | .id') + gh project item-delete 28 --owner modelcontextprotocol --id "$ITEM_ID" + ``` + + Deleting the card removes it from the board only — **the issue itself is untouched**, keeps its labels and comments, and stays searchable and linkable forever. Nothing is lost; the board simply stops claiming the work was delivered. + + Done is read as the record of what a milestone actually delivered, so a card parked there asserts something shipped. A duplicate sitting in Done makes that record wrong in a way nobody can detect later — counting Done cards can no longer distinguish a shipped fix from a report closed as a duplicate of one. Board columns are a workflow signal; GitHub's issue list is the archive. The close **reason** is the machine-readable form of the same distinction: `gh issue close --reason` accepts only `completed` and `not planned`, so **`duplicate` must be set through the API** — `gh api repos/OWNER/REPO/issues/N -X PATCH -f state=closed -f state_reason=duplicate` — or via "Mark as duplicate" in the web UI, which additionally records a duplicate-of link. - If new tasks are discovered or requested during development, create issues and add them to the board. ### Triaging unboarded issues @@ -246,32 +261,42 @@ Sweeping in the unboarded issues is only the most visible defect class. A board | Wrong board for label | `v1` → #11, `v2` → #28 | Move the card to the right board | | No version label | Every issue carries exactly one of `v1`/`v2` | Apply it (`v2` unless it's a fix for released v1 behavior) | | No Priority (#28) | Every board item is prioritized | Score it with the [rubric](#setting-issue-priority) | +| Closed, not shipped, still carded | **Done means the work shipped** — a card closed as duplicate/not-planned is deleted, not parked | Delete the card (`gh project item-delete`) | ```sh D=$(mktemp -d); R=modelcontextprotocol/inspector -gh issue list --repo $R --state open --limit 1000 --json number,labels,milestone > "$D/i.json" +# --limit must exceed the repo's TOTAL issue count (884 as of 2026-08-05), not just the open ones — +# the last check below reads closed issues' state reasons. +gh issue list --repo $R --state all --limit 2000 \ + --json number,state,stateReason,labels,milestone > "$D/i.json" for P in 28 11; do gh project item-list $P --owner modelcontextprotocol \ --format json --limit 700 > "$D/b$P.json"; done jq -nr --slurpfile o "$D/i.json" --slurpfile a "$D/b28.json" --slurpfile b "$D/b11.json" --arg R "$R" ' - ($o[0] | map({key:(.number|tostring), - value:{lab:[.labels[].name], ms:(.milestone.title // null)}}) | from_entries) as $I + ($o[0] | map({key:(.number|tostring), value:{st:.state, sr:(.stateReason // ""), + lab:[.labels[].name], ms:(.milestone.title // null)}}) | from_entries) as $M | def own($s): [$s[].items[] | select(.content.repository==$R)]; + def I($n): ($M[($n|tostring)] // null); + def ms($n): (I($n).ms // null); + def lab($n): (I($n).lab // []); + def isopen($n): (I($n).st == "OPEN"); + def shipped($n): (I($n).sr == "COMPLETED"); [own($a)[] | select(.content.type=="Issue") | {n:.content.number, s:.status, p:.priority}] as $B28 | [own($b)[] | select(.content.type=="Issue") | {n:.content.number, s:.status}] as $B11 - | def ms($n): ($I[($n|tostring)].ms // null); - def lab($n): ($I[($n|tostring)].lab // []); - def open($n): ($I[($n|tostring)] != null); - { + | { "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], "no Status": [($B28[], $B11[]) | select(.s==null) | .n], "Incoming w/ milestone": [($B28[], $B11[]) | select(.s=="Incoming" and ms(.n)!=null) | .n], "past Incoming, no ms": [($B28[], $B11[]) | select(.s!=null and .s!="Incoming" and .s!="Done" - and open(.n) and ms(.n)==null) | .n], - "v1 label on #28": [$B28[] | select(lab(.n)|index("v1")) | .n], - "v2 label on #11": [$B11[] | select(lab(.n)|index("v2")) | .n], - "open, no version label":[$o[0][] | select(([.labels[].name]|index("v1") or index("v2"))|not) | .number], - "#28 open, no Priority": [$B28[] | select(.p==null and open(.n)) | .n] + and isopen(.n) and ms(.n)==null) | .n], + "v1 label on #28": [$B28[] | select(isopen(.n) and (lab(.n)|index("v1"))) | .n], + "v2 label on #11": [$B11[] | select(isopen(.n) and (lab(.n)|index("v2"))) | .n], + "open, no version label":[$o[0][] | select(.state=="OPEN") + | select(([.labels[].name]|index("v1") or index("v2"))|not) | .number], + "#28 open, no Priority": [$B28[] | select(.p==null and isopen(.n)) | .n], + "closed unshipped, still carded": + [($B28[], $B11[]) | select(I(.n)!=null and (isopen(.n)|not) + and (shipped(.n)|not)) | .n] } | to_entries[] | "\(.value|length)\t\(.key)\t\(.value[0:10])"' ``` @@ -420,7 +445,7 @@ Status option IDs (`--single-select-option-id`) — **last verified 2026-08-01** | In Review | `159c8a02` | | Done | `259d6aab` | -Use **Incoming** for an issue that arrived unboarded and is awaiting review (no milestone yet), **Todo** once a maintainer has approved it by assigning a milestone — including an issue you created through the documented flow, which starts here — **In Progress** for general active work (regardless of surface), **In Review** once a PR is open, and **Done** on merge. The Incoming/Todo line is the one that matters: Todo asserts approval, so an unreviewed issue parked there is a false claim that someone signed off on it. The milestone is the machine-checkable form of that claim — Incoming ⇔ no milestone, everything past it ⇔ milestoned. +Use **Incoming** for an issue that arrived unboarded and is awaiting review (no milestone yet), **Todo** once a maintainer has approved it by assigning a milestone — including an issue you created through the documented flow, which starts here — **In Progress** for general active work (regardless of surface), **In Review** once a PR is open, and **Done** on merge — and *only* on merge (or a parent's last sub-issue closing). An issue closed for any other reason has its **card deleted**, because [Done asserts the work shipped](#issue-driven-work-style). The Incoming/Todo line is the one that matters: Todo asserts approval, so an unreviewed issue parked there is a false claim that someone signed off on it. The milestone is the machine-checkable form of that claim — Incoming ⇔ no milestone, everything past it ⇔ milestoned. Priority option IDs (`--single-select-option-id`) — **last verified 2026-08-01**. Derive the level with the rubric in [Setting issue priority](#setting-issue-priority); don't eyeball it. From d0b683d4bfa9e313cbe11877bcb59eab1fd8a7b6 Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Wed, 5 Aug 2026 17:04:02 -0400 Subject: [PATCH 04/11] docs: require a type label (bug/enhancement/documentation/chore/question) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The version label says which line work belongs to; nothing said what KIND of work it is. In practice that meant reaching for bug or enhancement and pressing everything else into one of them — a migration guide and a tsup->tsdown swap both landing as "enhancement", which degrades the label to "not a bug" and makes filtering by it meaningless. Every issue created or triaged now carries exactly one of bug, enhancement, documentation, chore, or question, independent of its v1/v2 label. Adds a table of what each is and is not for, and states the anti-pattern explicitly so the two familiar labels don't absorb the other three. A PR needs no type label — it is classified through the issue it closes, the same way it is tracked through that issue's board card. Wires the requirement into the three places that would otherwise drift: the "not created until" callout, triage pass 1 (an outside reporter can set neither label, so both are applied in the same step), and the board audit, which gains an "open, no type label" check and a matching row in the invariant table. Note `chore` did not exist as a repo label and was created for this; the existing `dependencies` label is Dependabot's and scoped to PRs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU --- AGENTS.md | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6a04eb37..3a482145 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,7 +148,7 @@ If you've already built a change locally, share the **prompt** you used and scre All work should be driven by items on the project board. -> **A v2 issue *you* create is not "created" until it is labeled `v2`, given a milestone, AND on board #28 with a Status *and* a Priority set.** Labeling alone is not enough — a label is a repo tag, the milestone is a release bucket, and the board is a separate org project. Applying `--label v2` does **not** add the item to the board, and adding it to the board does **not** set a Status or a Priority. All five are distinct steps; do all five (see the recipes below). **Only issues go on the board — never PRs.** A PR still gets the `v2` label, but it is tracked through its linked issue's card (via `Closes #N`), not its own board item. +> **A v2 issue *you* create is not "created" until it is labeled `v2` **and** typed (`bug`/`enhancement`/`documentation`/`chore`/`question`), given a milestone, AND on board #28 with a Status *and* a Priority set.** Labeling alone is not enough — a label is a repo tag, the milestone is a release bucket, and the board is a separate org project. Applying `--label v2` does **not** add the item to the board, and adding it to the board does **not** set a Status or a Priority. All five are distinct steps; do all five (see the recipes below). **Only issues go on the board — never PRs.** A PR still gets the `v2` label, but it is tracked through its linked issue's card (via `Closes #N`), not its own board item. > > **This describes an issue created through the flow below — not every issue that appears in the repo.** An issue opened by hand, in the GitHub UI, arrives with no label, no milestone, and no card. That is true of an outside reporter's (they have no board access) *and* of a maintainer's (write access makes the board reachable, not automatic). Either way it is normal on arrival, not a defect to fix the moment it lands; it comes into the system through [triage](#triaging-unboarded-issues) instead. The two paths differ in exactly one thing: doing the create flow *is* the approval, so the issue starts in **Todo** with a milestone; an issue that arrives unboarded and unmilestoned has been approved by nobody, so it starts in **Incoming** with no milestone. @@ -158,6 +158,19 @@ All work should be driven by items on the project board. - `v1` — work targeting `v1/main` (the deprecated line: security fixes only) - `v2` — work targeting `v2/main` (active development; the default for anything new) Set the label at **create time** — `gh issue create --label v2 ...`, `gh pr create --label v2 ...` — never by backfilling later, since unlabeled items are exactly the ones missed when filtering by version. **If the target version isn't obvious, it's `v2`**: v2 is where all new work goes, and `v1` is reserved for the narrow case of patching the deprecated line. Only ask when the issue is specifically a fix *for released v1 behavior* and it's unclear whether v2 still has the bug. Note the label is a repo tag and is **not** the board — see the callout above; a `v2` issue also needs a board card with a Status **and** a Priority (a `v1` one needs a Status; board #11 has no Priority field). +- **Label by type — every issue you create or triage carries exactly one of `bug` / `enhancement` / `documentation` / `chore` / `question`.** 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 — every issue needs both. Set it at create time (`gh issue create --label v2 --label bug ...`) or, for one arriving through [triage](#triaging-unboarded-issues), in the same pass that applies the version label. + + | Type | Use for | Not for | + | --- | --- | --- | + | `bug` | Something is broken, wrong, or regressed against its intended behavior | A missing capability that was never built | + | `enhancement` | A new capability, or extending an existing one — features, spec support, tracking issues for either | A cleanup with no behavior change | + | `documentation` | Prose deliverables — READMEs, guides, `specification/` docs, `AGENTS.md` rules | Code that happens to need a doc update | + | `chore` | Maintenance with no user-facing behavior change — dependency work, build/CI tooling, refactors, cleanup | Anything a user would notice | + | `question` | An open question or discussion with no agreed deliverable yet | Work someone has already decided to do | + + **Don't force the binary.** `bug` and `enhancement` are the two most reached for, and 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. If an issue is really a migration guide, say `documentation`; if it is a `tsup` → `tsdown` migration, say `chore`. + + A **PR** does not need a type label — it is classified through the issue it closes, the same way it is tracked through that issue's board card. - **Prioritize every board item.** Every issue must have a Priority (Urgent, High, Medium, or Low) on its card — set when the card is created, whether that's at issue-creation time (yours) or at triage (an unboarded one). Priority is a **board field**, not a label, so it lives on the card and an unboarded issue has nowhere to store it. Derive it with the rubric in [Setting issue priority](#setting-issue-priority) rather than asserting it — an unscored "this feels urgent" is exactly what the rubric exists to replace. - **Add the issue to the board and set Status and Priority.** After creating an issue, add it to the board for its version — **`v2` → board #28**, **`v1` → board #11** — and set the fields. (PRs are never added to either board — they're tracked through their linked issue's card.) This is the step most easily forgotten because it needs several IDs — copy the recipes below verbatim, and take them from the section for the right board; the two projects' ids are not interchangeable. - **An issue you create → `Todo`.** You only file an issue for work you intend to happen, so filing it is approving it. It gets a milestone and lands in Todo, ready to be picked up. Work you are starting immediately goes straight to **In Progress** instead. @@ -211,7 +224,7 @@ So don't check the author's permissions; check whether the work was done. Arrivi For each open issue with no card: -1. Apply the version label (`v2` unless it's a fix for released v1 behavior — see [Label by version](#issue-driven-work-style)). +1. Apply the version label (`v2` unless it's a fix for released v1 behavior — see [Label by version](#issue-driven-work-style)) **and the type label** (`bug` / `enhancement` / `documentation` / `chore` / `question` — see [Label by type](#issue-driven-work-style)). An outside reporter cannot set either, so both are applied here. 2. Add it to the board for that version — **`v2` → #28, `v1` → #11**. 3. Set Status to **`Incoming`**. 4. Set Priority with the [rubric](#setting-issue-priority) (v2 only), and **record the score in a comment** (see [Recording the score](#recording-the-score)). This is an *assessment*, not an approval — it's how the queue gets ordered for the maintainer who reviews it next. @@ -260,6 +273,7 @@ Sweeping in the unboarded issues is only the most visible defect class. A board | Past Incoming **without** a milestone | 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 | | No version label | Every issue carries exactly one of `v1`/`v2` | Apply it (`v2` unless it's a fix for released v1 behavior) | +| No type label | Every issue carries one of `bug`/`enhancement`/`documentation`/`chore`/`question` | Classify it per [Label by type](#issue-driven-work-style) | | No Priority (#28) | Every board item is prioritized | Score it with the [rubric](#setting-issue-priority) | | Closed, not shipped, still carded | **Done means the work shipped** — a card closed as duplicate/not-planned is deleted, not parked | Delete the card (`gh project item-delete`) | @@ -293,6 +307,10 @@ jq -nr --slurpfile o "$D/i.json" --slurpfile a "$D/b28.json" --slurpfile b "$D/b "v2 label on #11": [$B11[] | select(isopen(.n) and (lab(.n)|index("v2"))) | .n], "open, no version label":[$o[0][] | select(.state=="OPEN") | select(([.labels[].name]|index("v1") or index("v2"))|not) | .number], + "open, no type label": [$o[0][] | select(.state=="OPEN") + | select(([.labels[].name] | index("bug") or index("enhancement") + or index("documentation") or index("chore") + or index("question"))|not) | .number], "#28 open, no Priority": [$B28[] | select(.p==null and isopen(.n)) | .n], "closed unshipped, still carded": [($B28[], $B11[]) | select(I(.n)!=null and (isopen(.n)|not) From 51bbe9996b8b051812c888ebcda33ba688366439 Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Tue, 11 Aug 2026 09:15:08 -0400 Subject: [PATCH 05/11] fix(web): keep the text being typed in a schema number field (#1888) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SchemaForm` drove Mantine's `NumberInput` directly off the parent's numeric value. `NumberInput` reports the raw string while the text does not parse, so pressing `.` after `1` produced `"1."`, which the handler collapsed to `undefined` — and because the control is fully controlled, the `value` prop immediately rewrote the box back to `1`. The decimal point was discarded on every keystroke, making `1.5` unreachable. A trailing zero, a lone leading `-`, and an exponent failed the same way. Introduce `SchemaNumberInput`, which holds the in-progress text as the source of truth for what is displayed while the parent still only ever sees a `number | undefined`. Draft and value re-sync only when they genuinely diverge, via `useValueChange` (adjust-state-during-render) rather than an effect, so an external reset still rewrites the box but an in-progress `"1."` is left alone. Also pass `allowDecimal` from the schema, so an `integer` field rejects the decimal point instead of accepting a value its schema forbids — matching what the TUI already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 --- .../groups/SchemaForm/SchemaForm.test.tsx | 135 ++++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 87 ++++++++++- 2 files changed, 216 insertions(+), 6 deletions(-) diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index c534fb29..a381add1 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -1,3 +1,4 @@ +import { useState } from "react"; import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import type { InspectorFormSchema } from "../../../utils/jsonUtils"; @@ -228,6 +229,140 @@ describe("SchemaForm", () => { expect(lastCall.count).toBeUndefined(); }); + describe("number fields (#1888)", () => { + // The real callers keep the form values in state and feed them back in, so + // the bug only reproduces against a genuinely controlled SchemaForm: an + // uncontrolled render never rewrites the box and would pass either way. + function ControlledSchemaForm({ + schema, + initialValues = {}, + onChange, + }: { + schema: InspectorFormSchema; + initialValues?: Record<string, unknown>; + onChange: (values: Record<string, unknown>) => void; + }) { + const [values, setValues] = + useState<Record<string, unknown>>(initialValues); + return ( + <SchemaForm + schema={schema} + values={values} + onChange={(next) => { + setValues(next); + onChange(next); + }} + /> + ); + } + + const numberSchema: InspectorFormSchema = { + type: "object", + properties: { + divisor: { type: "number", title: "Divisor" }, + }, + }; + + it("lets a decimal be typed all the way through", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + <ControlledSchemaForm schema={numberSchema} onChange={onChange} />, + ); + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + await user.type(input, "1.5"); + expect(input.value).toBe("1.5"); + expect(onChange).toHaveBeenLastCalledWith({ divisor: 1.5 }); + }); + + it("keeps the trailing decimal point visible mid-entry", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + <ControlledSchemaForm schema={numberSchema} onChange={onChange} />, + ); + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + await user.type(input, "1."); + // The point survives on screen even though "1." parses to plain 1. + expect(input.value).toBe("1."); + expect(onChange).toHaveBeenLastCalledWith({ divisor: 1 }); + }); + + it("keeps a trailing zero after the decimal point", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + <ControlledSchemaForm schema={numberSchema} onChange={onChange} />, + ); + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + await user.type(input, "1.50"); + expect(input.value).toBe("1.50"); + expect(onChange).toHaveBeenLastCalledWith({ divisor: 1.5 }); + }); + + it("reports a lone minus sign as no value while leaving it typed", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + <ControlledSchemaForm schema={numberSchema} onChange={onChange} />, + ); + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + await user.type(input, "-"); + expect(input.value).toBe("-"); + expect(onChange).toHaveBeenLastCalledWith({ divisor: undefined }); + await user.type(input, "2.5"); + expect(onChange).toHaveBeenLastCalledWith({ divisor: -2.5 }); + }); + + it("rejects a decimal point in an integer field", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + count: { type: "integer", title: "Count" }, + }, + }; + renderWithMantine( + <ControlledSchemaForm schema={schema} onChange={onChange} />, + ); + const input = screen.getByLabelText(/Count/) as HTMLInputElement; + await user.type(input, "1.5"); + expect(input.value).toBe("15"); + expect(onChange).toHaveBeenLastCalledWith({ count: 15 }); + }); + + it("re-syncs the displayed text when the value changes externally", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + function Harness() { + const [values, setValues] = useState<Record<string, unknown>>({ + divisor: 1.5, + }); + return ( + <> + <button type="button" onClick={() => setValues({ divisor: 42 })}> + Load example + </button> + <SchemaForm + schema={numberSchema} + values={values} + onChange={(next) => { + setValues(next); + onChange(next); + }} + /> + </> + ); + } + renderWithMantine(<Harness />); + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + expect(input.value).toBe("1.5"); + await user.click(screen.getByRole("button", { name: "Load example" })); + expect(input.value).toBe("42"); + }); + }); + it("falls back to empty/const labels for oneOf items missing const and title", () => { const onChange = vi.fn(); const schema: InspectorFormSchema = { diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 30e39795..0e67d854 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -8,7 +8,9 @@ import { Text, TextInput, } from "@mantine/core"; +import { useState } from "react"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; +import { useValueChange } from "../../../hooks/useValueChange"; import type { InspectorFormSchema } from "../../../utils/jsonUtils"; const FieldLabel = Text.withProps({ @@ -49,6 +51,79 @@ function toEnumData( return values; } +/** + * Interpret whatever Mantine's `NumberInput` reported as the JSON value for the + * field. It emits a `number` once the text parses cleanly, and the **raw string** + * while it does not — `""` when cleared, but also the in-progress `"1."`, `"-"`, + * and `"1e"`. Anything that is not a finite number becomes `undefined`, which is + * how an absent optional argument is represented everywhere else in this form. + */ +function toNumericValue(raw: string | number): number | undefined { + if (typeof raw === "number") { + return Number.isFinite(raw) ? raw : undefined; + } + if (raw.trim() === "") { + return undefined; + } + const parsed = Number(raw); + return Number.isFinite(parsed) ? parsed : undefined; +} + +interface SchemaNumberInputProps { + label: string; + description?: string; + withAsterisk: boolean; + disabled: boolean; + value: number | undefined; + min?: number; + max?: number; + allowDecimal: boolean; + onChange: (value: number | undefined) => void; +} + +/** + * A `NumberInput` that keeps the text the user is typing, not just the number it + * currently parses to. + * + * Driving `NumberInput` directly off the parent's numeric value makes a decimal + * impossible to enter (#1888): typing `.` after `1` produces the unparseable + * string `"1."`, the numeric value stays `1`, and the controlled `value` prop + * immediately rewrites the box back to `"1"` — so the `.` vanishes and `1.5` can + * never be reached. Trailing zeros (`"1.50"`) and a lone leading `"-"` fail the + * same way. + * + * So the raw text is held here as the source of truth for what is *displayed*, + * while the parent still only ever sees a `number | undefined`. The two are + * re-synced only when the parent's value genuinely diverges from what the draft + * parses to, which leaves an external reset (a cleared form, a loaded example) + * working while an in-progress `"1."` — whose parse is `1`, matching the value we + * just emitted — is left alone. + */ +function SchemaNumberInput({ + value, + onChange, + ...inputProps +}: SchemaNumberInputProps) { + const [draft, setDraft] = useState<string | number>(value ?? ""); + + useValueChange(value, (next) => { + if (!Object.is(toNumericValue(draft), next)) { + setDraft(next ?? ""); + } + }); + + return ( + <NumberInput + {...inputProps} + value={draft} + onChange={(next) => { + setDraft(next); + onChange(toNumericValue(next)); + }} + /> + ); +} + export interface SchemaFormProps { schema: InspectorFormSchema; values: Record<string, unknown>; @@ -156,19 +231,19 @@ export function SchemaForm({ // number or integer if (fieldSchema.type === "number" || fieldSchema.type === "integer") { return ( - <NumberInput + <SchemaNumberInput key={fieldName} label={label} description={description} withAsterisk={isRequired} disabled={disabled} - value={(rawValue as number) ?? ""} + value={rawValue as number | undefined} min={fieldSchema.minimum} max={fieldSchema.maximum} - onChange={(val) => { - const numericValue = typeof val === "string" ? undefined : val; - handleFieldChange(fieldName, numericValue); - }} + // An `integer` field rejects the decimal point outright rather than + // accepting a value the schema forbids. + allowDecimal={fieldSchema.type === "number"} + onChange={(val) => handleFieldChange(fieldName, val)} /> ); } From 6f47eaa1438fe6907ba19a1737e0bea3bf99568b Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Tue, 11 Aug 2026 09:23:43 -0400 Subject: [PATCH 06/11] fix: render a tool result's structuredContent in the Tools screen (#1908) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool declaring an `outputSchema` returns its real payload in `structuredContent`; the `content[]` blocks usually only summarize it. v1 rendered that payload as its own inspectable JSON section — v2's ToolResultPanel only ever walked `result.content`, so the payload was dropped with no label, section, or hint that it existed. Adds a `StructuredOutputPanel` group: a collapsible "Structured Output" box (bordered like the existing "Resource Links" box) rendering the payload as pretty-printed, syntax-highlighted, copyable JSON, bounded so a large payload scrolls within itself. ToolResultPanel renders it below the content blocks — and also when `content` is empty (where "No results yet" was simply wrong) and below the alert on an error result, so it is never silently dropped. Adds a `list_items` test-server preset (nested payload, the issue's repro shape) and a `structured-output-http.json` showcase config, documented in the README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt --- README.md | 7 ++ .../StructuredOutputPanel.stories.tsx | 90 ++++++++++++++++ .../StructuredOutputPanel.test.tsx | 87 +++++++++++++++ .../StructuredOutputPanel.tsx | 102 ++++++++++++++++++ .../ToolResultPanel.stories.tsx | 18 ++++ .../ToolResultPanel/ToolResultPanel.test.tsx | 72 +++++++++++++ .../ToolResultPanel/ToolResultPanel.tsx | 38 +++++-- .../configs/structured-output-http.json | 15 +++ test-servers/src/preset-registry.ts | 3 + test-servers/src/test-server-fixtures.ts | 42 ++++++++ 10 files changed, 465 insertions(+), 9 deletions(-) create mode 100644 clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx create mode 100644 clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx create mode 100644 clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx create mode 100644 test-servers/configs/structured-output-http.json diff --git a/README.md b/README.md index 13792764..c691e8e9 100644 --- a/README.md +++ b/README.md @@ -138,6 +138,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `modern-network-http.json` | Network tab: `Mcp-*` headers + error taxonomy | [#1628](https://github.com/modelcontextprotocol/inspector/issues/1628) | | `xmcpheader-modern-http.json` | Tools tab: `x-mcp-header` mirroring and exclusions | [#1632](https://github.com/modelcontextprotocol/inspector/issues/1632) | | `pagination-http.json` | Page-by-page list fetching | [#1721](https://github.com/modelcontextprotocol/inspector/issues/1721) | +| `structured-output-http.json` | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | @@ -206,6 +207,12 @@ Under SDK v2 a `tools/call` rejecting with `-32602` renders as a distinct error Turn on **"Fetch Lists One Page at a Time"** (Server Settings — the `paginatedLists` setting, or the **Paginated** switch in a list sidebar) and the lists load page 1 only (4 items) with a **Load next page** control and an _N pages loaded_ status. Each click fetches the next 4 and appends them; Refresh resets to page 1. With the switch off (the default), the same lists auto-aggregate all three pages on connect. +#### Structured output + +`structured-output-http.json` serves `list_items` (nested `structuredContent` — objects inside arrays inside an object, the shape from [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908)), `get_temp` (a flat three-key payload), and `echo` (no `outputSchema` at all). It is a plain streamable-HTTP server — connect with the **default (legacy)** protocol era. + +Run `list_items` from the Tools tab: the result panel shows the `content[]` text summary ("Found 2 items.") **and** a collapsible **Structured Output** section rendering the schema-validated payload as pretty-printed, copyable JSON. That section is what v2 was dropping — a tool declaring an `outputSchema` returns its real data there, and the text block usually only summarizes it. Run `echo` to confirm the section is absent when a result carries no `structuredContent`. + #### Advertised extensions `advertised-extensions-http.json` serves `echo` (always) and a `get_weather` tool **gated on the `io.modelcontextprotocol/tasks` extension** (`extensionGatedTools`): the tool is registered but starts disabled, and the server enables it on `notifications/initialized` only when the client declared that extension in its `capabilities.extensions`. diff --git a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx new file mode 100644 index 00000000..443c78ab --- /dev/null +++ b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx @@ -0,0 +1,90 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, userEvent, waitFor, within } from "storybook/test"; +import { StructuredOutputPanel } from "./StructuredOutputPanel"; + +const nested = { + items: [ + { id: 1, name: "Item A", tags: ["foo", "bar"] }, + { id: 2, name: "Item B", tags: ["baz"] }, + ], + total: 2, +}; + +const flat = { + temperature: 25, + unit: "C", + city: "San Francisco", +}; + +// Long enough that the payload exceeds the section's max height and scrolls +// within it rather than pushing the rest of the result panel down. +const large = { + rows: Array.from({ length: 60 }, (_, index) => ({ + id: index + 1, + label: `Row ${index + 1}`, + value: index * 7, + })), + total: 60, +}; + +const meta: Meta<typeof StructuredOutputPanel> = { + title: "Groups/StructuredOutputPanel", + component: StructuredOutputPanel, +}; + +export default meta; +type Story = StoryObj<typeof StructuredOutputPanel>; + +export const Nested: Story = { + args: { + structuredContent: nested, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + expect( + canvas.getByRole("heading", { name: "Structured Output" }), + ).toBeInTheDocument(); + // The section starts expanded, so the payload is visible without a click. + // Asserted on the container's text: once the Prism grammar loads, the JSON + // is split across per-token elements, so no single node holds the value. + await waitFor(() => + expect(canvasElement.textContent).toContain('"Item A"'), + ); + }, +}; + +export const Flat: Story = { + args: { + structuredContent: flat, + }, +}; + +export const Large: Story = { + args: { + structuredContent: large, + }, +}; + +// Collapsing hides the payload; expanding brings it back. +export const Collapsed: Story = { + args: { + structuredContent: nested, + defaultExpanded: false, + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const toggle = canvas.getByRole("button", { + name: "Expand structured output", + }); + expect(toggle).toHaveAttribute("aria-expanded", "false"); + await userEvent.click(toggle); + await waitFor(() => + expect( + canvas.getByRole("button", { name: "Collapse structured output" }), + ).toHaveAttribute("aria-expanded", "true"), + ); + await waitFor(() => + expect(canvasElement.textContent).toContain('"Item A"'), + ); + }, +}; diff --git a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx new file mode 100644 index 00000000..abdaf91e --- /dev/null +++ b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.test.tsx @@ -0,0 +1,87 @@ +import { describe, it, expect, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; +import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { StructuredOutputPanel } from "./StructuredOutputPanel"; + +// The real CodeHighlight dynamic-imports the Prism runtime and each grammar +// (its own behavior is covered in CodeHighlight.test.tsx). Stub it so these +// tests assert on the JSON handed to it, synchronously. +vi.mock("../../elements/CodeHighlight/CodeHighlight", () => ({ + CodeHighlight: ({ language, code }: { language: string; code: string }) => ( + <pre data-testid="code-highlight" data-language={language}> + {code} + </pre> + ), +})); + +const structuredContent = { + items: [ + { id: 1, name: "Item A", tags: ["foo", "bar"] }, + { id: 2, name: "Item B", tags: ["baz"] }, + ], + total: 2, +}; + +describe("StructuredOutputPanel", () => { + it("renders a labeled section with the payload as pretty-printed JSON", () => { + renderWithMantine( + <StructuredOutputPanel structuredContent={structuredContent} />, + ); + expect( + screen.getByRole("heading", { name: "Structured Output" }), + ).toBeInTheDocument(); + const code = screen.getByTestId("code-highlight"); + expect(code).toHaveAttribute("data-language", "json"); + expect(code).toHaveTextContent(/"total": 2/); + // Nested values are inspectable field by field, not summarized away. + expect(code).toHaveTextContent(/"name": "Item A"/); + expect(code).toHaveTextContent(/"tags"/); + }); + + it("starts expanded by default", () => { + renderWithMantine( + <StructuredOutputPanel structuredContent={structuredContent} />, + ); + expect( + screen.getByRole("button", { name: "Collapse structured output" }), + ).toHaveAttribute("aria-expanded", "true"); + }); + + it("starts collapsed when defaultExpanded is false", () => { + renderWithMantine( + <StructuredOutputPanel + structuredContent={structuredContent} + defaultExpanded={false} + />, + ); + expect( + screen.getByRole("button", { name: "Expand structured output" }), + ).toHaveAttribute("aria-expanded", "false"); + }); + + it("toggles between expanded and collapsed", async () => { + const user = userEvent.setup(); + renderWithMantine( + <StructuredOutputPanel structuredContent={structuredContent} />, + ); + await user.click( + screen.getByRole("button", { name: "Collapse structured output" }), + ); + const toggle = screen.getByRole("button", { + name: "Expand structured output", + }); + expect(toggle).toHaveAttribute("aria-expanded", "false"); + await user.click(toggle); + expect( + screen.getByRole("button", { name: "Collapse structured output" }), + ).toHaveAttribute("aria-expanded", "true"); + }); + + it("renders an empty structured payload rather than nothing", () => { + renderWithMantine(<StructuredOutputPanel structuredContent={{}} />); + expect( + screen.getByRole("heading", { name: "Structured Output" }), + ).toBeInTheDocument(); + expect(screen.getByTestId("code-highlight")).toHaveTextContent("{}"); + }); +}); diff --git a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx new file mode 100644 index 00000000..0f088e97 --- /dev/null +++ b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.tsx @@ -0,0 +1,102 @@ +import { useState } from "react"; +import { + Collapse, + Group, + Paper, + ScrollArea, + Stack, + Title, +} from "@mantine/core"; +import type { CallToolResult } from "@modelcontextprotocol/client"; +import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; +import { ExpandToggle } from "../../elements/ExpandToggle/ExpandToggle"; + +export interface StructuredOutputPanelProps { + /** The result's `structuredContent` — the tool's schema-validated payload. */ + structuredContent: NonNullable<CallToolResult["structuredContent"]>; + /** Whether the section starts expanded. Defaults to `true`. */ + defaultExpanded?: boolean; +} + +// Bordered box matching the "Resource Links" group in the result panel, so the +// two supplementary sections of a tool result read as siblings. +const StructuredBox = Paper.withProps({ + withBorder: true, + radius: "md", + p: "md", + variant: "panel", +}); + +const StructuredInner = Stack.withProps({ + gap: "sm", +}); + +const HeaderRow = Group.withProps({ + justify: "space-between", + wrap: "nowrap", +}); + +// h4 (size h5) for the same reason as the "Resource Links" heading: the panel's +// "Results" title is h3, so a sub-box heading is h4 and the heading order never +// skips a level (axe `heading-order`). +const StructuredHeader = Title.withProps({ + order: 4, + size: "h5", +}); + +// Caps the payload so a large structured result scrolls within the box instead +// of pushing the content blocks out of view. `Autosize` sizes to the content up +// to `mah`, so a small object still takes only what it needs. +const StructuredScroll = ScrollArea.Autosize.withProps({ + mah: 400, + type: "auto", + scrollbars: "y", + offsetScrollbars: true, +}); + +/** + * Collapsible "Structured Output" section for a tool result's + * `structuredContent` (#1908). A tool declaring an `outputSchema` returns its + * real payload here — the `content[]` blocks usually only summarize it — so v1 + * rendered it as its own inspectable JSON section. Without this, the payload is + * dropped from the Tools screen entirely, with no hint it was ever returned. + * + * The JSON is pretty-printed and syntax-highlighted through {@link ContentViewer} + * (an `application/json` text block), so it is copyable and scannable field by + * field. + */ +export function StructuredOutputPanel({ + structuredContent, + defaultExpanded = true, +}: StructuredOutputPanelProps) { + const [expanded, setExpanded] = useState(defaultExpanded); + + return ( + <StructuredBox> + <StructuredInner> + <HeaderRow> + <StructuredHeader>Structured Output</StructuredHeader> + <ExpandToggle + expanded={expanded} + onToggle={() => setExpanded((value) => !value)} + ariaLabel={`${expanded ? "Collapse" : "Expand"} structured output`} + /> + </HeaderRow> + {/* Content stays mounted across a collapse (Mantine `Collapse`), so the + highlighted JSON isn't re-rendered from scratch on every toggle. */} + <Collapse in={expanded}> + <StructuredScroll> + <ContentViewer + block={{ + type: "text", + text: JSON.stringify(structuredContent, null, 2), + }} + mimeType="application/json" + copyable + /> + </StructuredScroll> + </Collapse> + </StructuredInner> + </StructuredBox> + ); +} diff --git a/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.stories.tsx b/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.stories.tsx index 1134f657..683bb423 100644 --- a/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.stories.tsx +++ b/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.stories.tsx @@ -173,6 +173,24 @@ export const ResourceLinksWithLongText: Story = { decorators: fillHeightDecorators, }; +// A tool declaring an `outputSchema` returns its real payload in +// `structuredContent`, which the text block only summarizes (#1908). It gets +// its own collapsible "Structured Output" section below the content blocks. +export const StructuredOutput: Story = { + args: { + result: { + content: [{ type: "text", text: "Found 2 items." }], + structuredContent: { + items: [ + { id: 1, name: "Item A", tags: ["foo", "bar"] }, + { id: 2, name: "Item B", tags: ["baz"] }, + ], + total: 2, + }, + }, + }, +}; + export const ErrorResult: Story = { args: { result: { diff --git a/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.test.tsx b/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.test.tsx index 1ebaea7b..1be9f2c3 100644 --- a/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.test.tsx +++ b/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.test.tsx @@ -109,6 +109,78 @@ describe("ToolResultPanel", () => { expect(screen.getByText("divider")).toBeInTheDocument(); }); + describe("structuredContent (#1908)", () => { + const structured = { items: [{ id: 1, name: "Item A" }], total: 1 }; + + it("renders a Structured Output section alongside the content blocks", () => { + renderWithMantine( + <ToolResultPanel + result={{ ...okResult, structuredContent: structured }} + onClear={() => {}} + />, + ); + expect(screen.getByText("ok")).toBeInTheDocument(); + expect( + screen.getByRole("heading", { name: "Structured Output" }), + ).toBeInTheDocument(); + }); + + it("omits the section when the result has no structuredContent", () => { + renderWithMantine( + <ToolResultPanel result={okResult} onClear={() => {}} />, + ); + expect( + screen.queryByRole("heading", { name: "Structured Output" }), + ).not.toBeInTheDocument(); + }); + + it("renders the section instead of the empty state when content is empty", () => { + renderWithMantine( + <ToolResultPanel + result={{ content: [], structuredContent: structured }} + onClear={() => {}} + />, + ); + expect(screen.queryByText("No results yet")).not.toBeInTheDocument(); + expect( + screen.getByRole("heading", { name: "Structured Output" }), + ).toBeInTheDocument(); + }); + + it("renders the section below the alert on an error result", () => { + renderWithMantine( + <ToolResultPanel + result={{ ...errorResult, structuredContent: structured }} + onClear={() => {}} + />, + ); + expect(screen.getByText("Tool Error")).toBeInTheDocument(); + expect( + screen.getByRole("heading", { name: "Structured Output" }), + ).toBeInTheDocument(); + }); + + it("renders the section alongside a Resource Links box", () => { + renderWithMantine( + <ToolResultPanel + result={{ + content: [ + { type: "resource_link", uri: "demo://r/1", name: "One" }, + ], + structuredContent: structured, + }} + onClear={() => {}} + />, + ); + expect( + screen.getByRole("heading", { name: "Resource Links" }), + ).toBeInTheDocument(); + expect( + screen.getByRole("heading", { name: "Structured Output" }), + ).toBeInTheDocument(); + }); + }); + it("invokes onClear when the close button is clicked", async () => { const user = userEvent.setup(); const onClear = vi.fn(); diff --git a/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.tsx b/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.tsx index 08db06a1..33ca7761 100644 --- a/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.tsx +++ b/clients/web/src/components/groups/ToolResultPanel/ToolResultPanel.tsx @@ -14,6 +14,7 @@ import type { } from "@modelcontextprotocol/client"; import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; import { ResourceLink } from "../ResourceLink/ResourceLink"; +import { StructuredOutputPanel } from "../StructuredOutputPanel/StructuredOutputPanel"; import { resultHasResourceLinks } from "./toolResultUtils"; export interface ToolResultPanelProps { @@ -217,6 +218,16 @@ export function ToolResultPanel({ // available height (and scrolls inside). Plain text/image results keep the // scroll-within-card body so a short result doesn't reserve empty height. const hasLinks = resultHasResourceLinks(result); + // A tool with an `outputSchema` returns its real payload in + // `structuredContent`, which the `content[]` blocks typically only summarize + // (#1908). Render it as its own section — including alongside an error or an + // empty `content` array, so it is never silently dropped. + const structuredNode = result.structuredContent ? ( + <StructuredOutputPanel + key="structured-output" + structuredContent={result.structuredContent} + /> + ) : null; const segmentNodes = segments.map((segment) => { if (segment.kind === "links") { @@ -254,22 +265,31 @@ export function ToolResultPanel({ </HeaderRow> {result.isError ? ( <ResultScroll> - <ErrorAlert> - {result.content - .filter((b) => b.type === "text") - .map((b) => b.text) - .join("\n")} - </ErrorAlert> + <ResultStack> + <ErrorAlert> + {result.content + .filter((b) => b.type === "text") + .map((b) => b.text) + .join("\n")} + </ErrorAlert> + {structuredNode} + </ResultStack> </ResultScroll> - ) : result.content.length === 0 ? ( + ) : result.content.length === 0 && !structuredNode ? ( <ResultScroll> <Text c="dimmed">No results yet</Text> </ResultScroll> ) : hasLinks ? ( - <FillStack>{segmentNodes}</FillStack> + <FillStack> + {segmentNodes} + {structuredNode} + </FillStack> ) : ( <ResultScroll> - <ResultStack>{segmentNodes}</ResultStack> + <ResultStack> + {segmentNodes} + {structuredNode} + </ResultStack> </ResultScroll> )} </PanelStack> diff --git a/test-servers/configs/structured-output-http.json b/test-servers/configs/structured-output-http.json new file mode 100644 index 00000000..48b9ae93 --- /dev/null +++ b/test-servers/configs/structured-output-http.json @@ -0,0 +1,15 @@ +{ + "serverInfo": { + "name": "structured-output-showcase", + "version": "1.0.0" + }, + "tools": [ + { "preset": "list_items" }, + { "preset": "get_temp" }, + { "preset": "echo" } + ], + "transport": { + "type": "streamable-http", + "port": 6601 + } +} diff --git a/test-servers/src/preset-registry.ts b/test-servers/src/preset-registry.ts index 6c7e6679..806769b3 100644 --- a/test-servers/src/preset-registry.ts +++ b/test-servers/src/preset-registry.ts @@ -34,6 +34,7 @@ import { createGetAnnotatedMessageTool, createGetTempTool, createGetTempExtraTool, + createListItemsTool, createAddResourceTool, createRemoveResourceTool, createAddToolTool, @@ -158,6 +159,8 @@ function resolveToolPreset( return createGetTempTool(); case "get_temp_extra": return createGetTempExtraTool(); + case "list_items": + return createListItemsTool(); case "add_resource": return createAddResourceTool(); case "remove_resource": diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index a87311ef..5a05b07b 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1048,6 +1048,48 @@ export function createGetTempTool(): ToolDefinition { }; } +/** Output schema for list_items: a nested list of items plus a total. */ +const ListItemsOutputSchema = z.object({ + items: z + .array( + z.object({ + id: z.number().describe("Item id"), + name: z.string().describe("Item name"), + tags: z.array(z.string()).describe("Item tags"), + }), + ) + .describe("The items"), + total: z.number().describe("Number of items"), +}); + +/** + * Create a "list_items" tool returning a short `content` summary alongside a + * DEEPLY NESTED `structuredContent` (objects inside arrays inside an object) — + * the shape from #1908, where the structured payload carries everything the + * text block only summarizes. Exercises the Structured Output section of the + * Tools screen result panel, which `get_temp`'s flat three-key object does not. + */ +export function createListItemsTool(): ToolDefinition { + return { + name: "list_items", + description: "Returns a list of items with nested structured output", + inputSchema: {}, + outputSchema: ListItemsOutputSchema, + handler: async () => { + const items = [ + { id: 1, name: "Item A", tags: ["foo", "bar"] }, + { id: 2, name: "Item B", tags: ["baz"] }, + ]; + return { + content: [ + { type: "text" as const, text: `Found ${items.length} items.` }, + ], + structuredContent: { items, total: items.length }, + }; + }, + }; +} + /** * Create a "get_temp_extra" tool that declares the same output schema as * get_temp but returns an EXTRA, undeclared property in structuredContent. From 3173b264f7cb3f407e142f13a56bb2007016e8de Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Tue, 11 Aug 2026 09:34:41 -0400 Subject: [PATCH 07/11] fix(docker): publish on loopback by default; make a mounted state dir writable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1964. - README's Docker recipes dropped v1's `127.0.0.1:` prefix, so `-p 6274:6274` published the Inspector on every host interface. The container's `HOST=0.0.0.0` governs the container's interfaces, not the host's, so `DANGEROUSLY_BIND_ALL_INTERFACES` never covered this. Verified against a real container: from a LAN peer, `GET /` served the injected `MCP_INSPECTOR_API_TOKEN`, and that token then unlocked `/api/*` with no `Origin` header (server.ts:241 allows origin-less requests). Restore the prefix on both recipes, on the port-remap examples, and in the web README. - Adding a server failed inside the container the moment you mounted a volume to keep it. Docker seeds a named volume's ownership from the image's directory at the mount point, and creates it `root:root` when that directory is absent — so the non-root `node` user got `EACCES ... open '/home/node/.mcp-inspector/mcp.json.tmp-*'`. Create the dir in the image, owned by `node`, and document the volume recipe (plus the bind-mount `--user` caveat) — without a volume the catalog lives in the writable layer and `--rm` throws it away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a --- Dockerfile | 9 +++++++++ README.md | 18 +++++++++++++++--- clients/web/README.md | 2 +- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/Dockerfile b/Dockerfile index 986231a0..f87a32ce 100644 --- a/Dockerfile +++ b/Dockerfile @@ -39,6 +39,15 @@ EXPOSE 6274 # (core/storage/store-io.ts), so set it explicitly to the node user's writable # home and work from there. ENV HOME=/home/node + +# Create the runtime-state dir up front, owned by `node`. Docker seeds a named +# volume's ownership from the image's directory at the mount point — and when +# that directory does not exist it creates it `root:root`, which the non-root +# `node` user then cannot write. Without this, `-v inspector-data:/home/node/.mcp-inspector` +# (the recipe for keeping your saved servers across `docker run --rm`) makes +# every "add server" fail with `EACCES ... open '/home/node/.mcp-inspector/mcp.json.tmp-*'`. +RUN mkdir -p /home/node/.mcp-inspector && chown -R node:node /home/node/.mcp-inspector + USER node WORKDIR /home/node diff --git a/README.md b/README.md index 13792764..d000f5ad 100644 --- a/README.md +++ b/README.md @@ -319,14 +319,26 @@ A container image is published to GHCR (`ghcr.io/modelcontextprotocol/inspector` ```bash # run the web UI (reads the auth token from the container logs) -docker run --rm -p 6274:6274 ghcr.io/modelcontextprotocol/inspector +docker run --rm -p 127.0.0.1:6274:6274 ghcr.io/modelcontextprotocol/inspector # or build the image locally docker build -t mcp-inspector . -docker run --rm -p 6274:6274 mcp-inspector +docker run --rm -p 127.0.0.1:6274:6274 mcp-inspector ``` -The image defaults to `--web` bound to `0.0.0.0:6274` with browser auto-open disabled; override the args to run another mode (`docker run --rm ghcr.io/modelcontextprotocol/inspector --cli …`). Pass `-e MCP_INSPECTOR_API_TOKEN=…` to set a known token (otherwise one is generated and printed in the logs), or `-e DANGEROUSLY_OMIT_AUTH=true` to disable auth. Binding `0.0.0.0` (all network interfaces) is refused by default outside a container — it exposes the process-spawning backend to the local network — so the image opts in explicitly with `DANGEROUSLY_BIND_ALL_INTERFACES=true` (already set in the `Dockerfile`); a bare `HOST=0.0.0.0` without that flag exits with an error. If you **remap the published port** (`-p 8080:6274`), the browser's origin (`http://localhost:8080`) no longer matches the in-container port, so set `-e ALLOWED_ORIGINS=http://localhost:8080,http://127.0.0.1:8080` (or run `-e CLIENT_PORT=8080 -p 8080:8080`) or connects will 403. `ALLOWED_ORIGINS` **replaces** the default list rather than merging, so list every loopback form you'll browse from (see the [web README](./clients/web/README.md#host-binding--the-origin-allow-list)). The image runs as the non-root `node` user and has a `HEALTHCHECK` that probes the web UI — it assumes the default `--web` mode, so add `--no-healthcheck` when running `--cli`/`--tui` (which have no web server). +**Keep the `127.0.0.1:` prefix on the published port.** A bare `-p 6274:6274` publishes on **every host interface**, putting the Inspector on your local network. The container's `HOST=0.0.0.0` is a separate concern — it governs the _container's_ interfaces, not the host's — so the `DANGEROUSLY_BIND_ALL_INTERFACES` opt-in that guards a wildcard bind outside a container does not cover this. It matters more here than for an ordinary web app: the backend spawns processes on request, `GET /` embeds the API token into the served HTML, and a request arriving with **no** `Origin` header skips the origin allow-list entirely — so for any non-browser client the API token is the only guard. Publish wider only deliberately, and pair it with a known `MCP_INSPECTOR_API_TOKEN`. + +**Keeping the servers you add.** The Inspector saves your server list to `$HOME/.mcp-inspector/mcp.json`, which in the image is `/home/node/.mcp-inspector/mcp.json` — inside the container's writable layer, so `--rm` discards it and every run starts with an empty list. Mount a volume there to keep it: + +```bash +docker run --rm -p 127.0.0.1:6274:6274 \ + -v mcp-inspector-data:/home/node/.mcp-inspector \ + ghcr.io/modelcontextprotocol/inspector +``` + +The same volume also persists OAuth tokens and stored state, so an authorized server stays authorized across runs. Use `-e MCP_CATALOG_PATH=/some/other/path.json` to put the catalog somewhere else — mount a volume covering whatever directory you point it at. If you **bind-mount a host directory** instead of a named volume (`-v "$PWD/inspector-data:/home/node/.mcp-inspector"`), the directory keeps its host ownership, so on Linux add `--user "$(id -u):$(id -g)"` or `chown` it to uid `1000` — otherwise the non-root `node` user can't write and adding a server fails with `EACCES`. + +The image defaults to `--web` bound to `0.0.0.0:6274` with browser auto-open disabled; override the args to run another mode (`docker run --rm ghcr.io/modelcontextprotocol/inspector --cli …`). Pass `-e MCP_INSPECTOR_API_TOKEN=…` to set a known token (otherwise one is generated and printed in the logs), or `-e DANGEROUSLY_OMIT_AUTH=true` to disable auth. Binding `0.0.0.0` (all network interfaces) is refused by default outside a container — it exposes the process-spawning backend to the local network — so the image opts in explicitly with `DANGEROUSLY_BIND_ALL_INTERFACES=true` (already set in the `Dockerfile`); a bare `HOST=0.0.0.0` without that flag exits with an error. If you **remap the published port** (`-p 127.0.0.1:8080:6274`), the browser's origin (`http://localhost:8080`) no longer matches the in-container port, so set `-e ALLOWED_ORIGINS=http://localhost:8080,http://127.0.0.1:8080` (or run `-e CLIENT_PORT=8080 -p 127.0.0.1:8080:8080`) or connects will 403. `ALLOWED_ORIGINS` **replaces** the default list rather than merging, so list every loopback form you'll browse from (see the [web README](./clients/web/README.md#host-binding--the-origin-allow-list)). The image runs as the non-root `node` user and has a `HEALTHCHECK` that probes the web UI — it assumes the default `--web` mode, so add `--no-healthcheck` when running `--cli`/`--tui` (which have no web server). ## Contributing — `AGENTS.md` and `CLAUDE.md` diff --git a/clients/web/README.md b/clients/web/README.md index c835aaf2..a3facd92 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -172,7 +172,7 @@ The guard blocks only the **wildcard** all-interfaces addresses. Binding a **spe - **Bind a specific address.** `HOST=192.168.1.50` (a LAN IP) or a public IP works directly: the default origin allow-list follows the bind host, so `allowedOrigins` becomes `http://<that-host>:PORT` and a browser hitting that address is accepted with no extra config. (The host is canonicalized the way a browser is — so `HOST=127.1` advertises `http://127.0.0.1:PORT`, an IPv6 bind host is bracketed as `http://[2001:db8::1]:PORT`, and the port is dropped when it's the http default `:80` — matching what the browser sends.) - **Behind TLS or a reverse proxy**, the browser's `Origin` becomes the public origin (e.g. `https://inspector.example.com`, often without a port), which won't match the auto-derived `http://<bind-host>:PORT`. Set `ALLOWED_ORIGINS` to the real public origin(s): `ALLOWED_ORIGINS=https://inspector.example.com`. -- **Using the `0.0.0.0` wildcard** (opt-in via `DANGEROUSLY_BIND_ALL_INTERFACES=true`, as the Docker image does): a wildcard bind also serves loopback, so the default allow-list is the loopback trio plus the canonical wildcard origins (`http://0.0.0.0:PORT`, `http://[::]:PORT`), and **local access works out of the box** — `docker run -p 6274:6274` browsed at `http://localhost:6274` connects with no extra config. Reaching it at a **non-loopback** address (a LAN IP, a public hostname) still needs `ALLOWED_ORIGINS` — but since that **replaces** the default, keep the loopback forms in the list if you also browse locally: `ALLOWED_ORIGINS=http://localhost:PORT,http://127.0.0.1:PORT,http://192.168.1.50:PORT,https://inspector.example.com`. +- **Using the `0.0.0.0` wildcard** (opt-in via `DANGEROUSLY_BIND_ALL_INTERFACES=true`, as the Docker image does): a wildcard bind also serves loopback, so the default allow-list is the loopback trio plus the canonical wildcard origins (`http://0.0.0.0:PORT`, `http://[::]:PORT`), and **local access works out of the box** — `docker run -p 127.0.0.1:6274:6274` browsed at `http://localhost:6274` connects with no extra config. Reaching it at a **non-loopback** address (a LAN IP, a public hostname) still needs `ALLOWED_ORIGINS` — but since that **replaces** the default, keep the loopback forms in the list if you also browse locally: `ALLOWED_ORIGINS=http://localhost:PORT,http://127.0.0.1:PORT,http://192.168.1.50:PORT,https://inspector.example.com`. The bind-host guard and the `ALLOWED_ORIGINS` allow-list apply to both the prod server and `--dev`. Note that in **`--dev`** the Vite dev server _additionally_ enforces its own `server.allowedHosts` Host-header check, whose default accepts loopback and IP-literal hosts. The host you **bind** is auto-allowed (Vite adds the resolved `server.host` — which this config sets from `HOST` — to the allow-list), so `HOST=<hostname>` works out of the box under `--dev` too. What needs an explicit `server.allowedHosts` entry is reaching the dev server at a **different** name than the one bound — e.g. a wildcard bind reached by hostname, or a reverse-proxy domain. For those, prefer the prod server (`mcp-inspector --web`) or add the host to `server.allowedHosts`. From 24a978633736cf1492d7e2fc53d1a8e332343798 Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Tue, 11 Aug 2026 10:02:59 -0400 Subject: [PATCH 08/11] fix(web): reset a number field's draft text when the form changes entity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The draft/value re-sync compares parsed numbers, so it cannot see a reset to an *equal* value. `ToolDetailPanel` is not keyed by tool, so its fields are reused across selections: type `-` (draft `"-"`, value `undefined`), switch to a tool with a same-named number field and no default, and the value is `undefined` on both sides — no divergence is detected and the stale `-` stays in the box for the new tool. Add a `resetKey` prop carrying the identity of whatever the form edits, and vary the number field's React key with it so no in-progress text can outlive the entity it was typed into. `ToolDetailPanel` passes the tool name; nested object forms inherit it. The schema object is no substitute — callers rebuild it every render, so its identity is unstable. The regression test drives the switch with `fireEvent` rather than `user.click`: a real click also blurs the input, and Mantine sanitizes an incomplete value on blur, which masked the defect entirely. A paired test asserts a *stable* `resetKey` does not remount, since remounting on every render would wipe the draft and reinstate #1888. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 --- .../groups/SchemaForm/SchemaForm.test.tsx | 76 ++++++++++++++++++- .../groups/SchemaForm/SchemaForm.tsx | 31 +++++++- .../ToolDetailPanel/ToolDetailPanel.tsx | 4 + 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index a381add1..32263e9a 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -2,7 +2,11 @@ import { useState } from "react"; import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import type { InspectorFormSchema } from "../../../utils/jsonUtils"; -import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { + fireEvent, + renderWithMantine, + screen, +} from "../../../test/renderWithMantine"; import { SchemaForm } from "./SchemaForm"; describe("SchemaForm", () => { @@ -332,6 +336,76 @@ describe("SchemaForm", () => { expect(onChange).toHaveBeenLastCalledWith({ count: 15 }); }); + it("drops in-progress text when resetKey moves the form to another entity", async () => { + const user = userEvent.setup(); + // Both "tools" expose the same-named number field with no default, so the + // value is `undefined` before and after the switch. The value comparison + // sees no divergence, and only resetKey can tell the field to start over. + const schema: InspectorFormSchema = { + type: "object", + properties: { divisor: { type: "number", title: "Divisor" } }, + }; + function Harness() { + const [tool, setTool] = useState("tool-a"); + const [values, setValues] = useState<Record<string, unknown>>({}); + return ( + <> + <button + type="button" + onClick={() => { + setTool("tool-b"); + // What ToolsScreen does on select: replace the form values. + setValues({}); + }} + > + Switch tool + </button> + <SchemaForm + schema={schema} + values={values} + onChange={setValues} + resetKey={tool} + /> + </> + ); + } + renderWithMantine(<Harness />); + const input = () => screen.getByLabelText(/Divisor/) as HTMLInputElement; + await user.type(input(), "-"); + expect(input().value).toBe("-"); + // fireEvent, not user.click: a real click also blurs the input, and + // Mantine sanitizes an incomplete value on blur — which would mask + // whether the switch itself cleared the draft. This drives the state + // change without the blur, isolating the reset to resetKey. + fireEvent.click(screen.getByRole("button", { name: "Switch tool" })); + expect(input().value).toBe(""); + }); + + it("keeps in-progress text across re-renders of the same entity", async () => { + const user = userEvent.setup(); + // The counterpart to the test above: a stable resetKey must NOT remount + // the field, or every keystroke would wipe the draft and reinstate #1888. + const schema: InspectorFormSchema = { + type: "object", + properties: { divisor: { type: "number", title: "Divisor" } }, + }; + function Harness() { + const [values, setValues] = useState<Record<string, unknown>>({}); + return ( + <SchemaForm + schema={schema} + values={values} + onChange={setValues} + resetKey="tool-a" + /> + ); + } + renderWithMantine(<Harness />); + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + await user.type(input, "1.5"); + expect(input.value).toBe("1.5"); + }); + it("re-syncs the displayed text when the value changes externally", async () => { const user = userEvent.setup(); const onChange = vi.fn(); diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 0e67d854..efcb32e3 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -98,6 +98,11 @@ interface SchemaNumberInputProps { * parses to, which leaves an external reset (a cleared form, a loaded example) * working while an in-progress `"1."` — whose parse is `1`, matching the value we * just emitted — is left alone. + * + * That value comparison cannot see a reset to an *equal* value, so the caller is + * additionally expected to vary this component's React key via `SchemaForm`'s + * `resetKey` when it switches which entity the form edits. See the note on that + * prop for the case it covers. */ function SchemaNumberInput({ value, @@ -129,6 +134,25 @@ export interface SchemaFormProps { values: Record<string, unknown>; onChange: (values: Record<string, unknown>) => void; disabled?: boolean; + /** + * Stable identity of whatever this form is editing — a tool name, a request + * id. Pass it whenever the same mounted form is reused for a *different* + * entity, which is the case for the Tools tab: `ToolDetailPanel` is not keyed + * by tool, so selecting another tool re-renders the same field components. + * + * It exists because the number field's draft/value re-sync compares parsed + * numbers, and so cannot detect a reset to an equal value. Type `-` (draft + * `"-"`, value `undefined`), then switch to a tool with a same-named number + * field and no default: the value is `undefined` on both sides, no divergence + * is seen, and the stale `-` would otherwise be left in the box for the new + * tool to continue from. Varying `resetKey` remounts the field instead, so no + * in-progress text can outlive the entity it was typed into. + * + * Omit it when the form is mounted fresh per entity (the elicitation panels), + * where unmounting already discards the draft. The schema object itself is no + * substitute — callers rebuild it every render, so its identity is unstable. + */ + resetKey?: string; } function getDefaultValue(fieldSchema: InspectorFormSchema): unknown { @@ -153,6 +177,7 @@ export function SchemaForm({ values, onChange, disabled = false, + resetKey, }: SchemaFormProps) { const properties = schema.properties ?? {}; const requiredFields = schema.required ?? []; @@ -232,7 +257,9 @@ export function SchemaForm({ if (fieldSchema.type === "number" || fieldSchema.type === "integer") { return ( <SchemaNumberInput - key={fieldName} + // The only field holding local state, so the only one that has to be + // remounted when `resetKey` says the form moved to another entity. + key={resetKey === undefined ? fieldName : `${resetKey}:${fieldName}`} label={label} description={description} withAsterisk={isRequired} @@ -318,6 +345,8 @@ export function SchemaForm({ handleFieldChange(fieldName, nestedValues) } disabled={disabled} + // Sub-fields belong to the same entity, so they reset with it. + resetKey={resetKey} /> </IndentedStack> </Stack> diff --git a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx index a74c63a9..fbb6d26d 100644 --- a/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx +++ b/clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx @@ -317,6 +317,10 @@ export function ToolDetailPanel({ values={formValues} onChange={onFormChange} disabled={isExecuting} + // This panel is reused across tool selections rather than remounted, + // so the form needs the tool name to drop another tool's + // in-progress field text. See SchemaFormProps.resetKey. + resetKey={name} /> {progress && <ProgressDisplay params={progress} />} From 5be2b3e87b7779174aa6e813c06f3fc829498388 Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Tue, 11 Aug 2026 10:15:28 -0400 Subject: [PATCH 09/11] =?UTF-8?q?docs:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20volume=20upgrade=20path,=20and=20drop=20the=20token?= =?UTF-8?q?=20claim?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A named volume created by an image predating the ownership fix keeps its root ownership if it is non-empty, since Docker only applies the image directory's ownership to an *empty* volume. Verified both halves: an empty pre-existing volume repairs itself on the first run of the fixed image, while one holding a file stays root-owned and still EACCESes. Document the distinction and the one-shot `chown` repair (verified to restore writes). - "pair it with a known MCP_INSPECTOR_API_TOKEN" was bad advice for a wider publication: the same paragraph notes `GET /` discloses the token, so a custom one is harvested exactly as a generated one is. Point at a real access-control boundary instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SFXDQBEjvjCEBhmYkmw79a --- README.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index d000f5ad..a9562079 100644 --- a/README.md +++ b/README.md @@ -326,7 +326,7 @@ docker build -t mcp-inspector . docker run --rm -p 127.0.0.1:6274:6274 mcp-inspector ``` -**Keep the `127.0.0.1:` prefix on the published port.** A bare `-p 6274:6274` publishes on **every host interface**, putting the Inspector on your local network. The container's `HOST=0.0.0.0` is a separate concern — it governs the _container's_ interfaces, not the host's — so the `DANGEROUSLY_BIND_ALL_INTERFACES` opt-in that guards a wildcard bind outside a container does not cover this. It matters more here than for an ordinary web app: the backend spawns processes on request, `GET /` embeds the API token into the served HTML, and a request arriving with **no** `Origin` header skips the origin allow-list entirely — so for any non-browser client the API token is the only guard. Publish wider only deliberately, and pair it with a known `MCP_INSPECTOR_API_TOKEN`. +**Keep the `127.0.0.1:` prefix on the published port.** A bare `-p 6274:6274` publishes on **every host interface**, putting the Inspector on your local network. The container's `HOST=0.0.0.0` is a separate concern — it governs the _container's_ interfaces, not the host's — so the `DANGEROUSLY_BIND_ALL_INTERFACES` opt-in that guards a wildcard bind outside a container does not cover this. It matters more here than for an ordinary web app: the backend spawns processes on request, `GET /` embeds the API token into the served HTML, and a request arriving with **no** `Origin` header skips the origin allow-list entirely — so for any non-browser client the API token is the only guard. Publishing wider needs a real access-control boundary in front of the Inspector — a reverse proxy that authenticates, an SSH tunnel, a private network. Setting your own `MCP_INSPECTOR_API_TOKEN` does **not** substitute: `GET /` discloses whatever token is in use, so a custom one is harvested exactly as easily as a generated one. **Keeping the servers you add.** The Inspector saves your server list to `$HOME/.mcp-inspector/mcp.json`, which in the image is `/home/node/.mcp-inspector/mcp.json` — inside the container's writable layer, so `--rm` discards it and every run starts with an empty list. Mount a volume there to keep it: @@ -338,6 +338,14 @@ docker run --rm -p 127.0.0.1:6274:6274 \ The same volume also persists OAuth tokens and stored state, so an authorized server stays authorized across runs. Use `-e MCP_CATALOG_PATH=/some/other/path.json` to put the catalog somewhere else — mount a volume covering whatever directory you point it at. If you **bind-mount a host directory** instead of a named volume (`-v "$PWD/inspector-data:/home/node/.mcp-inspector"`), the directory keeps its host ownership, so on Linux add `--user "$(id -u):$(id -g)"` or `chown` it to uid `1000` — otherwise the non-root `node` user can't write and adding a server fails with `EACCES`. +**Upgrading from an image before this fix?** Earlier images did not create `/home/node/.mcp-inspector`, so Docker created the volume's mount point as `root` and the non-root `node` user couldn't write to it. An **empty** volume repairs itself on the first run of a current image (Docker applies the image directory's ownership to an empty volume), but one that already has files in it keeps its old `root` ownership and still fails with `EACCES`. Fix it once: + +```bash +docker run --rm -u 0 --entrypoint chown \ + -v mcp-inspector-data:/data ghcr.io/modelcontextprotocol/inspector \ + -R node:node /data +``` + The image defaults to `--web` bound to `0.0.0.0:6274` with browser auto-open disabled; override the args to run another mode (`docker run --rm ghcr.io/modelcontextprotocol/inspector --cli …`). Pass `-e MCP_INSPECTOR_API_TOKEN=…` to set a known token (otherwise one is generated and printed in the logs), or `-e DANGEROUSLY_OMIT_AUTH=true` to disable auth. Binding `0.0.0.0` (all network interfaces) is refused by default outside a container — it exposes the process-spawning backend to the local network — so the image opts in explicitly with `DANGEROUSLY_BIND_ALL_INTERFACES=true` (already set in the `Dockerfile`); a bare `HOST=0.0.0.0` without that flag exits with an error. If you **remap the published port** (`-p 127.0.0.1:8080:6274`), the browser's origin (`http://localhost:8080`) no longer matches the in-container port, so set `-e ALLOWED_ORIGINS=http://localhost:8080,http://127.0.0.1:8080` (or run `-e CLIENT_PORT=8080 -p 127.0.0.1:8080:8080`) or connects will 403. `ALLOWED_ORIGINS` **replaces** the default list rather than merging, so list every loopback form you'll browse from (see the [web README](./clients/web/README.md#host-binding--the-origin-allow-list)). The image runs as the non-root `node` user and has a `HEALTHCHECK` that probes the web UI — it assumes the default `--web` mode, so add `--no-healthcheck` when running `--cli`/`--tui` (which have no web server). ## Contributing — `AGENTS.md` and `CLAUDE.md` From 6fd03d6de4442a841648836280f74f970ff4d491 Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Tue, 11 Aug 2026 10:22:05 -0400 Subject: [PATCH 10/11] fix(web): don't round unrepresentable numbers, and reset app-switch drafts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2. Three findings, all real. 1. Precision. Mantine's `NumberInput` returns the raw *string* rather than a number once the value reaches `Number.MAX_SAFE_INTEGER` (see its `isValidNumber` guard), precisely because JS cannot hold it. Parsing that turned `90071992547409910` into `90071992547409904` and would have sent the server a number the user never typed. An inspector must not misreport what it transmits, so such input now reports no value — which is also what this field did before #1888, so nothing regresses. Only the integer part overflows, so long decimals stay parsed. 2. `AppDetailPanel` has the same reuse pattern as `ToolDetailPanel`: `AppsScreen.handleSelect` swaps `selectedAppName` + `formValues` in place without remounting, so switching between apps with a same-named number field could retain an equal-valued draft. It now passes `resetKey` too. 3. An exponent was never reachable: `NumberInput` masks input through `NumericFormat`, which rejects `e`. Dropped that claim from the doc comment rather than implying `"1e"` is a case being handled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119LaPQ3v4NyBK3ZTdM9j84 --- .../AppDetailPanel/AppDetailPanel.test.tsx | 24 ++++++++++++++ .../groups/AppDetailPanel/AppDetailPanel.tsx | 6 ++++ .../groups/SchemaForm/SchemaForm.test.tsx | 28 +++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 31 ++++++++++++++++--- 4 files changed, 84 insertions(+), 5 deletions(-) diff --git a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx index 0c40d0ae..effbd5a2 100644 --- a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx +++ b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.test.tsx @@ -149,4 +149,28 @@ describe("AppDetailPanel", () => { await user.click(screen.getByRole("button", { name: /open app/i })); expect(onOpenApp).toHaveBeenCalledTimes(1); }); + + it("drops a previous app's in-progress number text when the app changes", async () => { + const user = userEvent.setup(); + // AppsScreen swaps selectedAppName + formValues in place rather than + // remounting this panel, so two apps exposing a same-named number field + // share the field component. Both values here are undefined, which the + // draft/value re-sync cannot distinguish — only the resetKey identity can. + const numberFieldTool = (name: string): Tool => ({ + name, + title: name, + inputSchema: { + type: "object", + properties: { scale: { type: "number", title: "Scale" } }, + }, + }); + const { rerender } = renderWithMantine( + <AppDetailPanel {...baseProps} tool={numberFieldTool("app_a")} />, + ); + const input = () => screen.getByLabelText(/Scale/) as HTMLInputElement; + await user.type(input(), "-"); + expect(input().value).toBe("-"); + rerender(<AppDetailPanel {...baseProps} tool={numberFieldTool("app_b")} />); + expect(input().value).toBe(""); + }); }); diff --git a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx index 51bf14c6..8d5a2135 100644 --- a/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx +++ b/clients/web/src/components/groups/AppDetailPanel/AppDetailPanel.tsx @@ -76,6 +76,12 @@ export function AppDetailPanel({ values={formValues} onChange={onFormChange} disabled={isOpening} + // Like ToolDetailPanel, this panel is reused across app selections + // rather than remounted (AppsScreen.handleSelect swaps + // selectedAppName + formValues in place), so the form needs the app + // tool's name to drop another app's in-progress field text. See + // SchemaFormProps.resetKey. + resetKey={tool.name} /> <OpenAppButton diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 32263e9a..5b9f064a 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -336,6 +336,34 @@ describe("SchemaForm", () => { expect(onChange).toHaveBeenLastCalledWith({ count: 15 }); }); + it("reports no value for a magnitude JS cannot hold exactly", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + <ControlledSchemaForm schema={numberSchema} onChange={onChange} />, + ); + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + // Past Number.MAX_SAFE_INTEGER, Mantine stops emitting a number and hands + // back the raw string to avoid destroying precision. Number() would round + // this to ...904, so parsing it would send a value the user never typed. + await user.type(input, "90071992547409910"); + expect(input.value).toBe("90071992547409910"); + expect(onChange).toHaveBeenLastCalledWith({ divisor: undefined }); + }); + + it("still parses a long decimal, which stays exactly representable", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + renderWithMantine( + <ControlledSchemaForm schema={numberSchema} onChange={onChange} />, + ); + const input = screen.getByLabelText(/Divisor/) as HTMLInputElement; + // Guards the safe-integer check against over-rejecting: the fractional + // digits are not what overflows, so this must not be dropped. + await user.type(input, "3.14159265358979"); + expect(onChange).toHaveBeenLastCalledWith({ divisor: 3.14159265358979 }); + }); + it("drops in-progress text when resetKey moves the form to another entity", async () => { const user = userEvent.setup(); // Both "tools" expose the same-named number field with no default, so the diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index efcb32e3..232b5327 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -53,10 +53,26 @@ function toEnumData( /** * Interpret whatever Mantine's `NumberInput` reported as the JSON value for the - * field. It emits a `number` once the text parses cleanly, and the **raw string** - * while it does not — `""` when cleared, but also the in-progress `"1."`, `"-"`, - * and `"1e"`. Anything that is not a finite number becomes `undefined`, which is - * how an absent optional argument is represented everywhere else in this form. + * field. Anything that is not a finite number becomes `undefined`, which is how + * an absent optional argument is represented everywhere else in this form. + * + * `NumberInput` emits a `number` only when the text both parses *and* is exactly + * representable; otherwise it hands back the **raw string** (see its + * `isValidNumber` guard). Two quite different situations produce a string, and + * they are treated differently here: + * + * 1. **Mid-entry text** — `""` when cleared, plus `"1."`, `"1.50"`, and a lone + * `"-"`. These are parsed: `"1."` really does mean `1`. (Note that an + * exponent is *not* in this set — `NumberInput` masks input through + * `NumericFormat`, which rejects `e` outright, so `"1e"` can never be typed.) + * 2. **Values JS cannot hold exactly** — anything at or beyond + * `Number.MAX_SAFE_INTEGER`. `Number("90071992547409910")` silently yields + * `90071992547409904`, so parsing here would send the server a number the + * user never entered. An inspector must not misreport what it transmits, so + * these report no value instead — which is also what this field did with such + * input before #1888, making it no regression. Preserving them properly needs + * an exact-serialization path down the whole `tools/call` chain, which is a + * separate concern from being able to type a decimal. */ function toNumericValue(raw: string | number): number | undefined { if (typeof raw === "number") { @@ -66,7 +82,12 @@ function toNumericValue(raw: string | number): number | undefined { return undefined; } const parsed = Number(raw); - return Number.isFinite(parsed) ? parsed : undefined; + if (!Number.isFinite(parsed)) { + return undefined; + } + // Case 2 above. The integer part is what overflows exact representation; the + // fractional digits are bounded by the same guard and stay lossless. + return Number.isSafeInteger(Math.trunc(parsed)) ? parsed : undefined; } interface SchemaNumberInputProps { From 8049b4f7c8d3fb6cf2d6e9adf99aa6334c100f8a Mon Sep 17 00:00:00 2001 From: cliffhall <cliff@futurescale.com> Date: Tue, 11 Aug 2026 10:28:01 -0400 Subject: [PATCH 11/11] test: cover the structured-output cap and the list_items showcase end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps from the Copilot review on #1975: - The `Large` story asserted nothing, so dropping the section's `mah` (or applying it to the wrong element) would still pass CI. Its play function now measures the real ScrollArea viewport: the visible height stays at the cap and the overflow is scrollable rather than clipped. - Nothing exercised the `list_items` fixture, so a preset-registration typo would leave the documented showcase broken with a green suite. A new integration test loads `structured-output-http.json` through loadConfig/resolveConfig — covering the file, the registry entry, and the fixture — connects, calls the tool, and asserts both the summary block and the nested structuredContent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt --- .../StructuredOutputPanel.stories.tsx | 19 +++++++ .../integration/mcp/inspectorClient.test.ts | 49 +++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx index 443c78ab..c66760f0 100644 --- a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx +++ b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx @@ -59,10 +59,29 @@ export const Flat: Story = { }, }; +// A payload taller than the section's cap scrolls inside it rather than +// pushing the rest of the result panel down. Asserted on the real geometry, +// so dropping `mah` (or moving it to the wrong element) fails here. export const Large: Story = { args: { structuredContent: large, }, + play: async ({ canvasElement }) => { + await waitFor(() => + expect(canvasElement.textContent).toContain('"Row 60"'), + ); + const viewport = canvasElement.querySelector( + ".mantine-ScrollArea-viewport", + ); + if (!(viewport instanceof HTMLElement)) { + throw new Error("scroll viewport not found"); + } + // Capped: the visible height stays at the section's `mah`, well under the + // payload's natural height… + expect(viewport.clientHeight).toBeLessThanOrEqual(400); + // …and the overflow is reachable by scrolling rather than clipped away. + expect(viewport.scrollHeight).toBeGreaterThan(viewport.clientHeight); + }, }; // Collapsing hides the payload; expanding brings it back. diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 7937b219..1a1645d8 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { resolve } from "node:path"; import * as z from "zod/v4"; import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { @@ -45,6 +46,8 @@ import { createAddResourceTool, createAddToolTool, createAddPromptTool, + loadConfig, + resolveConfig, } from "@modelcontextprotocol/inspector-test-server"; import type { MessageEntry, @@ -1214,6 +1217,52 @@ describe("InspectorClient", () => { }); }); + describe("Structured output showcase config (#1908)", () => { + // Drives the shipped `structured-output-http.json` end to end — the file + // itself, the `list_items` preset registration, and the fixture's nested + // payload. A typo in any of the three fails here rather than leaving the + // documented showcase quietly broken. + const showcaseConfigPath = resolve( + import.meta.dirname, + "../../../../../../test-servers/configs/structured-output-http.json", + ); + + it("serves list_items with a summary block and a nested structuredContent", async () => { + server = createTestServerHttp( + resolveConfig(loadConfig(showcaseConfigPath)), + ); + await server.start(); + + client = new InspectorClient( + { type: "streamable-http", url: server.url }, + { environment: { transport: createTransportNode } }, + ); + await client.connect(); + + const tool = await getTool(client, "list_items"); + expect(tool.outputSchema).toBeDefined(); + + const result = await client.callTool(tool, {}); + expect(result.success).toBe(true); + + // The text block only summarizes — the payload is the structured half. + const content = result.result!.content as Array<{ + type: string; + text?: string; + }>; + expect(content[0].type).toBe("text"); + expect(content[0].text).toBe("Found 2 items."); + + expect(result.result!.structuredContent).toEqual({ + items: [ + { id: 1, name: "Item A", tags: ["foo", "bar"] }, + { id: 2, name: "Item B", tags: ["baz"] }, + ], + total: 2, + }); + }); + }); + describe("Default metadata (server-wide _meta)", () => { function metaOf(req: { message: unknown }): Record<string, unknown> { const params = (req.message as { params?: { _meta?: unknown } }).params;