Compare commits
817 changed files with 28615 additions and 159799 deletions
|
|
@ -1,287 +0,0 @@
|
|||
---
|
||||
name: board-ops
|
||||
description: "gh recipes for the Inspector project boards — add a card, move its Status, set or re-score its Priority, delete a card, and recover from a deleted single-select option. Covers board #28 (v2) and board #11 (v1), their node/field/option IDs, and the option-deletion hazard."
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# Board operations
|
||||
|
||||
The rules about *what* a card's Status and Priority should be live in
|
||||
[`AGENTS.md`](../../../AGENTS.md) under **Issue-driven Work Style**. This skill is
|
||||
the *mechanics*: the exact `gh` calls, the IDs, and the ways the board can be
|
||||
damaged.
|
||||
|
||||
Related: `/issue-create` (the five-step create flow), `/issue-triage` (sweeping
|
||||
unboarded issues in, the priority rubric, the board audit).
|
||||
|
||||
## Which board
|
||||
|
||||
| Version label | Board | Owner | Has a Priority field? |
|
||||
| --- | --- | --- | --- |
|
||||
| `v2` | [#28](https://github.com/orgs/modelcontextprotocol/projects/28) | `modelcontextprotocol` | Yes |
|
||||
| `v1` | [#11](https://github.com/orgs/modelcontextprotocol/projects/11) | `modelcontextprotocol` | **No** |
|
||||
|
||||
Both are **org** projects, so every command takes `--owner modelcontextprotocol`.
|
||||
The two projects have their own field and option IDs and none of them are
|
||||
interchangeable — a #28 id passed to #11 is rejected with "option Id does not
|
||||
belong to the field", so the mistake is at least loud.
|
||||
|
||||
**Only issues go on a board — never PRs, never draft cards.** A PR is tracked
|
||||
through the card of the issue it closes.
|
||||
|
||||
## V2 board (#28) IDs
|
||||
|
||||
The project node id and the field ids are stable. The **option** ids are **not** —
|
||||
they are regenerated whenever a single-select field's option list is edited (see
|
||||
the hazard below). If any option id here is rejected, re-fetch:
|
||||
|
||||
```sh
|
||||
# Swap "Status" for "Priority" to fetch the other field's options.
|
||||
gh project field-list 28 --owner modelcontextprotocol --format json \
|
||||
| jq '.fields[] | select(.name=="Status") | .options'
|
||||
```
|
||||
|
||||
| Thing | ID |
|
||||
| --- | --- |
|
||||
| Project node ID | `PVT_kwDOCt2Azc4BJVxt` |
|
||||
| Status field ID | `PVTSSF_lADOCt2Azc4BJVxtzg5iI8c` |
|
||||
| Priority field ID | `PVTSSF_lADOCt2Azc4BJVxtzg5iJE4` |
|
||||
|
||||
Status option IDs (`--single-select-option-id`) — **last verified 2026-08-01**.
|
||||
|
||||
| Status | Option ID | Means |
|
||||
| --- | --- | --- |
|
||||
| Incoming | `721a3d4c` | Arrived unboarded, awaiting review — **no milestone** |
|
||||
| Todo | `fbdaf21e` | Approved (a milestone was assigned) |
|
||||
| In Progress | `195df262` | Active work, whatever the surface |
|
||||
| In Review | `159c8a02` | A PR is open |
|
||||
| Done | `259d6aab` | **Shipped** — a merged PR, or a parent whose last sub-issue closed |
|
||||
|
||||
Priority option IDs — **last verified 2026-08-01**. Derive the level with the
|
||||
rubric in `/issue-triage`; don't eyeball it.
|
||||
|
||||
| Priority | Option ID | Rubric total |
|
||||
| --- | --- | --- |
|
||||
| Urgent | `79628723` | 12+ |
|
||||
| High | `0a877460` | 9–11 |
|
||||
| Medium | `da944a9c` | 6–8 |
|
||||
| Low | `d67ac7ce` | ≤5 |
|
||||
|
||||
## V1 board (#11) IDs
|
||||
|
||||
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.
|
||||
|
||||
| Thing | ID |
|
||||
| --- | --- |
|
||||
| Project node ID | `PVT_kwDOCt2Azc4BA5sz` |
|
||||
| Status field ID | `PVTSSF_lADOCt2Azc4BA5szzgzkS-g` |
|
||||
|
||||
Status option IDs — **last verified 2026-08-01**.
|
||||
|
||||
| Status | Option ID |
|
||||
| --- | --- |
|
||||
| Incoming | `831820cf` |
|
||||
| Todo | `f75ad846` |
|
||||
| In Progress | `47fc9ee4` |
|
||||
| In Review | `0439b2bf` |
|
||||
| Done | `98236657` |
|
||||
|
||||
There is **no Priority field on this board** — the priority rubric is v2-only.
|
||||
Don't try to set one here; the field id doesn't exist.
|
||||
|
||||
## Recipes
|
||||
|
||||
### Add a card and set its fields
|
||||
|
||||
```sh
|
||||
# Prints the item id (PVTI_…); capture it.
|
||||
ITEM_ID=$(gh project item-add 28 --owner modelcontextprotocol --url <issue-url> --format json --jq '.id')
|
||||
|
||||
# Status → Todo (an issue you filed through the create flow 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
|
||||
```
|
||||
|
||||
Each `item-edit` sets **one** field, so setting both takes two calls — there is
|
||||
no combined form.
|
||||
|
||||
For an issue swept in at triage, the only difference is Status → **Incoming**
|
||||
(`721a3d4c`) and that you do **not** set a milestone.
|
||||
|
||||
For **v1**, the same shape against board #11:
|
||||
|
||||
```sh
|
||||
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
|
||||
```
|
||||
|
||||
### Move an existing card
|
||||
|
||||
Look the item id up by issue number rather than 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.
|
||||
|
||||
⚠️ **Filter by repository, not by number alone.** These are **org** projects and
|
||||
issue numbers are **repo-local**, so an unfiltered `select` can match another
|
||||
repo's issue that happens to share the number — board #11 really does carry a
|
||||
`modelcontextprotocol/servers` card — and then moves or deletes the wrong card,
|
||||
or passes two ids at once (Copilot).
|
||||
|
||||
```sh
|
||||
ITEM_ID=$(gh project item-list 28 --owner modelcontextprotocol --format json --limit 500 \
|
||||
--jq '.items[] | select(.content.repository=="modelcontextprotocol/inspector"
|
||||
and .content.number==<ISSUE_NUMBER>) | .id')
|
||||
# e.g. Status → In Review, when its PR opens
|
||||
gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" \
|
||||
--field-id PVTSSF_lADOCt2Azc4BJVxtzg5iI8c --single-select-option-id 159c8a02
|
||||
```
|
||||
|
||||
### Delete a card
|
||||
|
||||
**`Done` means the work shipped.** An issue closed as duplicate / won't fix /
|
||||
not planned / obsolete / superseded shipped nothing, so its card is **deleted**,
|
||||
not parked in Done:
|
||||
|
||||
```sh
|
||||
ITEM_ID=$(gh project item-list 28 --owner modelcontextprotocol --format json --limit 500 \
|
||||
--jq '.items[] | select(.content.repository=="modelcontextprotocol/inspector"
|
||||
and .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 duplicate sitting there makes that record wrong in a way nobody can detect
|
||||
later.
|
||||
|
||||
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**:
|
||||
|
||||
```sh
|
||||
gh api repos/modelcontextprotocol/inspector/issues/<N> -X PATCH \
|
||||
-f state=closed -f state_reason=duplicate
|
||||
```
|
||||
|
||||
(or "Mark as duplicate" in the web UI, which additionally records a
|
||||
duplicate-of link).
|
||||
|
||||
## ⚠️ The option-deletion hazard
|
||||
|
||||
**Never add, rename, or remove an option on a single-select board field (Status
|
||||
or Priority) with the `updateProjectV2Field` GraphQL mutation unless you pass
|
||||
every existing option's `id`.** That mutation does a **full replace** of the
|
||||
option list: resending options by name/color/description without their `id`s
|
||||
makes GitHub **delete all existing options and mint new ones**, which **orphans
|
||||
that field's value on every card on the board** *and* invalidates every option
|
||||
id in the tables above. This has happened once, on Status (~197 items
|
||||
reconstructed by inference).
|
||||
|
||||
Safe alternatives, in order of preference:
|
||||
|
||||
1. **Add or rename an option in the GitHub web UI** (Project → the field's
|
||||
settings). This preserves the ids of untouched options.
|
||||
⚠️ **Deleting is different, in the UI as much as in the API**: removing an
|
||||
option blanks that field's value on every card that held it, with no undo and
|
||||
no warning that says so.
|
||||
2. If you must script it, first `gh api graphql` the current options **with their
|
||||
`id`s**, then call `updateProjectV2Field` echoing back every existing option
|
||||
**including its `id`**, appending only the new one.
|
||||
`ProjectV2SingleSelectFieldOptionInput.id` is an optional `String`, so a mixed
|
||||
list works. Verify afterward that no card lost its value — snapshot
|
||||
`gh project item-list … --format json` before and after and diff; don't just
|
||||
spot-check. Send those dumps to `$BOARD_TMP` too, for the reason above.
|
||||
|
||||
Both the `Incoming` Status option and the Urgent/High/Medium/Low Priority
|
||||
options were added this way (#1891), with the before/after diff confirming all
|
||||
264 cards kept their Status.
|
||||
|
||||
`gh project item-add` and `gh project item-edit` are always safe — they set a
|
||||
card's value and never touch the field schema.
|
||||
|
||||
### Always snapshot before touching a field's options
|
||||
|
||||
One command, and it is the difference between a five-minute restore and
|
||||
reconstructing statuses by inference:
|
||||
|
||||
⚠️ **Write it outside the repo.** [The boards are private](../issue-triage/SKILL.md),
|
||||
so a snapshot is a full dump of item IDs and every card's Status and Priority.
|
||||
Left in the working tree it is one `git add -A` away from being published in a
|
||||
PR (Copilot).
|
||||
|
||||
```sh
|
||||
BOARD_TMP=$(mktemp -d)
|
||||
gh project item-list 28 --owner modelcontextprotocol --format json --limit 600 \
|
||||
> "$BOARD_TMP/board-snapshot.json"
|
||||
echo "snapshot: $BOARD_TMP/board-snapshot.json" # note the path; you need it to recover
|
||||
```
|
||||
|
||||
### Recovering from a deleted option
|
||||
|
||||
This has happened twice — once via the API (~197 items, reconstructed by
|
||||
inference) and once via the UI (the `Done` column, 247 items, restored from a
|
||||
snapshot in minutes). With a snapshot the recovery is mechanical.
|
||||
|
||||
The recipe below is written for a deleted **Status** option. For a deleted
|
||||
**Priority** option it is the same three steps with two substitutions: read
|
||||
`.priority` instead of `.status` (`gh project item-list --format json` exposes
|
||||
each single-select field under its lowercased name, so both keys are present),
|
||||
and pass the Priority field id `PVTSSF_lADOCt2Azc4BJVxtzg5iJE4`.
|
||||
|
||||
```sh
|
||||
# 0. Same temp dir the snapshot went to — keep every dump out of the worktree.
|
||||
BOARD_TMP=${BOARD_TMP:-$(mktemp -d)}
|
||||
|
||||
# 1. Which cards lost their value, and what did they hold?
|
||||
gh project item-list 28 --owner modelcontextprotocol --format json --limit 600 \
|
||||
> "$BOARD_TMP/board-broken.json"
|
||||
jq -r '[.items[]|select(.status==null)|.id]' "$BOARD_TMP/board-broken.json" \
|
||||
> "$BOARD_TMP/lost-ids.json"
|
||||
jq -r --slurpfile L "$BOARD_TMP/lost-ids.json" '($L[0]) as $lost
|
||||
| [.items[] | select(.id as $i | $lost|index($i)) | .status // "(none)"]
|
||||
| group_by(.) | map({s:.[0],c:length}) | .[] | "was \(.s): \(.c)"' \
|
||||
"$BOARD_TMP/board-snapshot.json"
|
||||
|
||||
# 2. Recreate the option, echoing every surviving option's id (see above).
|
||||
# NOTE: the recreated option gets a NEW id — the deleted one never comes back.
|
||||
|
||||
# 3. Re-apply it to the orphaned cards.
|
||||
for id in $(jq -r '.[]' "$BOARD_TMP/lost-ids.json"); do
|
||||
gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$id" \
|
||||
--field-id PVTSSF_lADOCt2Azc4BJVxtzg5iI8c --single-select-option-id <NEW_OPTION_ID>
|
||||
sleep 0.4
|
||||
done
|
||||
```
|
||||
|
||||
Step 1's grouping is the safety check: confirm the orphaned set is exactly the
|
||||
cards that held the deleted option, so you don't overwrite a card someone
|
||||
legitimately moved in the meantime.
|
||||
|
||||
Because the recreated option carries a **new id**, the tables above and every
|
||||
reference to it must be updated in the same change — `grep` the old id across
|
||||
the repo. The `Done` id has been `248a3910` and is now `259d6aab` for exactly
|
||||
this reason.
|
||||
|
||||
## ⚠️ Two different "Priority" fields
|
||||
|
||||
An issue page shows two fields named Priority, and they are unrelated. **Ours is
|
||||
the one under _Projects → Inspector V2_.**
|
||||
|
||||
| Where it appears | What it is | Ours? |
|
||||
| --- | --- | --- |
|
||||
| **Projects → Inspector V2 → Priority** | The **project board** field on #28 (`PVTSSF_lADOCt2Azc4BJVxtzg5iJE4`) | ✅ Yes |
|
||||
| **Fields → Priority** (above _Projects_) | A GitHub **issue field**, `IFSS_kgDOAdAWeg`, defined at the **org** level and shared by every repo in it | ❌ No |
|
||||
|
||||
Nothing syncs them, in either direction, and they will happily disagree.
|
||||
**Never delete the org-level field** — it belongs to the whole org. Don't set it
|
||||
either; a value there is a *reporter's* opinion and feeds the rubric as a capped
|
||||
+1 signal bonus and nothing more (see `/issue-triage`).
|
||||
|
|
@ -1,30 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "Move the board card for issue 2189 to In Review.",
|
||||
"expect": "board-ops"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I set the Priority field on a card in the v2 project board?",
|
||||
"expect": "board-ops"
|
||||
},
|
||||
{
|
||||
"prompt": "The project board rejected my Status option ID. How do I get the current ones?",
|
||||
"expect": "board-ops"
|
||||
},
|
||||
{
|
||||
"prompt": "I'm about to edit the options on one of the board's single-select fields. Should I do anything first?",
|
||||
"expect": "board-ops"
|
||||
},
|
||||
{
|
||||
"prompt": "Several cards lost their Priority value after the field was edited. How do I put it back?",
|
||||
"expect": "board-ops"
|
||||
},
|
||||
{
|
||||
"prompt": "Explain the difference between TCP and UDP.",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "What year was the first Moon landing?",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
|
|
@ -1,134 +0,0 @@
|
|||
---
|
||||
name: issue-create
|
||||
description: Create a tracked issue in this repo end to end — version label, type label, milestone, board card, Status and Priority. Use when asked to file, create, open or raise an issue; when picking its labels or its milestone; when checking whether one already exists; or when work discovered mid-task needs an issue of its own.
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# Creating an issue
|
||||
|
||||
An issue **you** create is not "created" until every step below that applies to
|
||||
its version is done. A label is a repo tag, the milestone is a release bucket,
|
||||
and the board is a separate org project — `--label v2` does **not** add a board
|
||||
card, and adding a card does **not** set a Status or a Priority.
|
||||
|
||||
| # | Step | v2 | v1 |
|
||||
| --- | --- | --- | --- |
|
||||
| 1 | **Version label** — exactly one of `v1` / `v2` | ✅ | ✅ |
|
||||
| 2 | **Type label** — exactly one of `bug` / `enhancement` / `documentation` / `chore` / `question` | ✅ | ✅ |
|
||||
| 3 | **Milestone** | ✅ | ❌ — every milestone is a v2 release bucket, so there is none to take |
|
||||
| 4 | **A card on the board for that version** — `v2` → #28, `v1` → #11 | ✅ | ✅ |
|
||||
| 5 | **Status** on that card | ✅ | ✅ |
|
||||
| 6 | **Priority** on that card | ✅ | ❌ — board #11 has no such field |
|
||||
|
||||
The two ❌ rows are not oversights to fix later: the field does not exist for
|
||||
that line, and `AGENTS.md` states both exceptions (Copilot).
|
||||
|
||||
Set the labels and milestone at **create time**, never by backfilling: an
|
||||
unlabeled issue belongs to no version line and appears in no version-filtered
|
||||
query, and an unmilestoned one drops out of release planning silently.
|
||||
|
||||
**Never create a duplicate.** Check the board for a matching item first.
|
||||
**Never create a draft card** (a board card with no issue number) — every board
|
||||
item is a real GitHub issue.
|
||||
|
||||
## 0. Check the board first
|
||||
|
||||
Search **all** states, not just open. A closed issue is still a duplicate — it
|
||||
may have been completed, rejected, or filed and superseded — and refiling it
|
||||
loses that history. When one matches, the call is whether to reopen it or file
|
||||
genuinely new work; either way you need to have seen it (Copilot).
|
||||
|
||||
```sh
|
||||
gh issue list --repo modelcontextprotocol/inspector --state all --limit 1000 \
|
||||
--search "<keywords>" --json number,title,state,stateReason,labels,milestone
|
||||
```
|
||||
|
||||
## 1. Pick the labels
|
||||
|
||||
**Version.** `v2` is the default for anything new. `v1` is reserved for the
|
||||
narrow case of patching the deprecated line (security fixes only). If the target
|
||||
version isn't obvious, it's `v2` — only ask when the issue is specifically a fix
|
||||
*for released v1 behavior* and it's unclear whether v2 still has the bug.
|
||||
|
||||
**Type.** Independent of the version label; every issue needs both.
|
||||
|
||||
| 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 | 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 — deps, build/CI tooling, refactors | 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.
|
||||
|
||||
## 2. Pick the milestone
|
||||
|
||||
If the user didn't specify one, default to the **current** milestone: the open
|
||||
one with the nearest due date.
|
||||
|
||||
```sh
|
||||
gh api repos/modelcontextprotocol/inspector/milestones --jq \
|
||||
'map(select(.state=="open")) | sort_by(.due_on) | .[] | "\(.title)\tdue \(.due_on[0:10])\topen=\(.open_issues)"'
|
||||
```
|
||||
|
||||
Milestones are **release** buckets (`v2.1.0`, `v2.2.0`, …), so pick by *when the
|
||||
work ships*, not by size. If a new issue plainly can't make the current
|
||||
milestone, say so and put it in the next one rather than leaving it blank.
|
||||
Sub-issues normally inherit their parent's milestone.
|
||||
|
||||
⚠️ **Every milestone is a v2 release bucket.** There is no v1 bucket, so a `v1`
|
||||
issue cannot satisfy the milestone rule — don't drop it in a v2.x one. Leave it
|
||||
unmilestoned and say so.
|
||||
|
||||
## 3. Create it
|
||||
|
||||
```sh
|
||||
gh issue create --repo modelcontextprotocol/inspector \
|
||||
--title "<title>" \
|
||||
--label v2 --label bug \
|
||||
--milestone "v2.5.0" \
|
||||
--body "<body>"
|
||||
```
|
||||
|
||||
## 4. Board it, in Todo
|
||||
|
||||
Filing an issue for work you intend to happen **is** approving it, so it starts
|
||||
in **Todo** with its milestone already set — not in Incoming, which is the queue
|
||||
for issues nobody has evaluated yet. Work you are starting immediately goes
|
||||
straight to **In Progress**.
|
||||
|
||||
Run the **add-card recipe in `/board-ops`** with **Status → Todo** and the
|
||||
Priority you scored. The project, field and option IDs live there and only
|
||||
there: an option ID is regenerated whenever its field's option list is edited,
|
||||
so a second copy here would go stale silently and break issue creation even
|
||||
after `board-ops` was fixed.
|
||||
|
||||
For a **v1** issue, the same recipe against board #11 — no Priority, that board
|
||||
has no such field.
|
||||
|
||||
The priority rubric is in `/issue-triage`.
|
||||
|
||||
## Note on issues that arrive from elsewhere
|
||||
|
||||
An issue opened through the GitHub UI — by an outside reporter *or* by a
|
||||
maintainer — arrives with **no milestone and no card**, and that is normal on
|
||||
arrival rather than a defect to fix the moment it lands: it comes into the
|
||||
system through `/issue-triage` instead, and starts in **Incoming**, because
|
||||
nobody has approved it.
|
||||
|
||||
Its **labels are partly set already**, which the triage pass has to account for:
|
||||
blank issues are disabled, so it came through a form in
|
||||
[`.github/ISSUE_TEMPLATE/`](../../../.github/ISSUE_TEMPLATE) (Copilot).
|
||||
|
||||
| Form | Applies | Still missing |
|
||||
| --- | --- | --- |
|
||||
| Bug report | `bug` | the version label — the form carries a version-line **dropdown** a maintainer reads at triage, because a `labels:` list is static and GitHub cannot map an answer to a label |
|
||||
| Feature request | `enhancement`, `v2` | nothing — v1 takes security fixes only and cannot receive a feature, so `v2` is correct by construction |
|
||||
|
||||
So a bug report needs its version label applied from the dropdown answer, and a
|
||||
feature request usually needs no label work at all. ⚠️ GitHub serves the chooser
|
||||
from the **default branch**, so a form edited on `v2/main` does not take effect
|
||||
until the next milestone merge.
|
||||
|
|
@ -1,30 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "Create an issue titled 'TUI crashes on an empty catalog' and set it up the way this repo wants.",
|
||||
"expect": "issue-create"
|
||||
},
|
||||
{
|
||||
"prompt": "Which milestone should a new issue go in, and how do I find the current one?",
|
||||
"expect": "issue-create"
|
||||
},
|
||||
{
|
||||
"prompt": "Before I file this, how do I check nobody has reported it already?",
|
||||
"expect": "issue-create"
|
||||
},
|
||||
{
|
||||
"prompt": "I found a bug while working on something else. How do I get it tracked in this repo?",
|
||||
"expect": "issue-create"
|
||||
},
|
||||
{
|
||||
"prompt": "What do I need to do beyond `gh issue create` for a new issue to be properly tracked here?",
|
||||
"expect": "issue-create"
|
||||
},
|
||||
{
|
||||
"prompt": "What is the chemical symbol for potassium?",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "Reverse this string: stressed",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
|
|
@ -1,299 +0,0 @@
|
|||
---
|
||||
name: issue-triage
|
||||
description: Triage unboarded Inspector issues — the two-pass sweep, the priority rubric and its score comment, and the board audit that checks the whole board against its invariants. Use for "triage new issues" or when scoring an issue's Priority.
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# Triaging issues
|
||||
|
||||
**An issue needs triage when it has no board card — regardless of who filed
|
||||
it.** That is the whole test, and it is deliberately about *state*, not
|
||||
authorship. The milestone is not part of the test; it decides *where the card
|
||||
lands* (milestoned → **Todo**, unmilestoned → **Incoming**), so an
|
||||
approved-but-unboarded issue is still swept in rather than skipped (Copilot). 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 `/issue-create` flow. Write access makes
|
||||
the board *reachable*, not automatic. What puts an issue in Todo is somebody
|
||||
performing the approval, and an issue nobody did that for has not been approved,
|
||||
whoever's name is on it.
|
||||
|
||||
**"Triage new issues" means running pass 1 over every unboarded open issue, then
|
||||
the board audit.** 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)
|
||||
**and the type label** (`bug` / `enhancement` / `documentation` / `chore` /
|
||||
`question` — see `/issue-create` for the table). 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 below (v2 only), and **record the score in a
|
||||
comment**. 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.
|
||||
|
||||
**The one exception is an unboarded issue that already carries a milestone.**
|
||||
Someone recorded the approval and only the card is missing, so board it straight
|
||||
into **Todo**, keeping the milestone. Otherwise triage would silently un-approve
|
||||
work a maintainer had already scheduled.
|
||||
|
||||
### Finding the unboarded ones
|
||||
|
||||
Diff 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
|
||||
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.
|
||||
|
||||
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 priority rubric
|
||||
|
||||
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.
|
||||
|
||||
**Axis 1 — Severity / impact (1–5).** How bad is it when it happens?
|
||||
|
||||
| Score | Means |
|
||||
| --- | --- |
|
||||
| 1 | Cosmetic — a typo, a misaligned control, a wording nit. |
|
||||
| 2 | Minor friction with an easy workaround. |
|
||||
| 3 | A real feature is broken or missing; the workaround is annoying or partial. |
|
||||
| 4 | A core workflow is unusable, or the Inspector reports something false about the server under test. |
|
||||
| 5 | Data loss, a security vulnerability, or a release that is broken on arrival for everyone. |
|
||||
|
||||
**Axis 2 — Urgency / staleness (1–5).** How time-sensitive or neglected is it?
|
||||
|
||||
| Score | Means |
|
||||
| --- | --- |
|
||||
| 1 | No time pressure; nothing waits on it. |
|
||||
| 2 | Wanted eventually. |
|
||||
| 3 | Wanted this milestone, or has sat >90 days with no activity. |
|
||||
| 4 | Blocking other work, or tied to a dated external dependency (an SDK release, a spec deadline). |
|
||||
| 5 | Blocking a release, or actively hurting users on a published version right now. |
|
||||
|
||||
**Signal indicators (+1 each — not an axis of their own).** Corroborating
|
||||
evidence that the two axes may have undercounted:
|
||||
|
||||
- Carries a `bug` or security-related label
|
||||
- Linked to a milestone — i.e. **already approved**. 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
|
||||
- The reporter set `Fields → Priority` to **Urgent or High** — **+1, flat,
|
||||
whichever of the two they picked.** It does not map to a band, and `Urgent`
|
||||
earns exactly what `High` earns.
|
||||
|
||||
**Bands.** Axes give 2–10 and there are six bonuses, so the total runs 2–16.
|
||||
|
||||
| Total | Priority | Meaning |
|
||||
| --- | --- | --- |
|
||||
| 12+ | **Urgent** | Drop what you're doing. |
|
||||
| 9–11 | **High** | Next up after current work. |
|
||||
| 6–8 | **Medium** | Scheduled normally. |
|
||||
| ≤5 | **Low** | Nice to have; may sit. |
|
||||
|
||||
Severity alone doesn't reach Urgent: a 5/5 with no corroborating signals totals
|
||||
10 and lands **High**. That's deliberate — Urgent is reserved for a severe
|
||||
problem that something *else* also confirms is burning, and a band that
|
||||
everything qualifies for stops carrying information. Override the band when it's
|
||||
plainly wrong, but say why in the issue; a rubric nobody may overrule is a rubric
|
||||
people route around.
|
||||
|
||||
### 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.
|
||||
|
||||
The org-level `Fields → Priority` is the opposite. It renders on the public
|
||||
issue page and is **not** part of maintainer triage, so any value there is
|
||||
**untrusted** — we didn't put it there, and it carries a preference rather than
|
||||
an assessment. That asymmetry is why it earns a flat +1 and nothing more:
|
||||
|
||||
- **It counts for something.** Someone flagging their own issue is real
|
||||
information about how much it hurts them.
|
||||
- **It cannot decide an outcome.** The bonus is capped, identical for `Urgent`
|
||||
and `High`, and can lift an issue at most one band. Nothing a reporter can type
|
||||
reaches Urgent by itself: Urgent needs 12, so the issue must already sit at 11
|
||||
on maintainer-assessed axes — at which point the reporter is not the reason.
|
||||
- **Never map the value across.** A reporter selecting `Urgent` does not make the
|
||||
card Urgent. Doing that would hand queue position to anyone with a GitHub
|
||||
account, and the queue would sort by assertiveness instead of impact.
|
||||
|
||||
Don't lean on GitHub's permission gate to enforce this. Whether an outside
|
||||
reporter can set that field today is an implementation detail that can change
|
||||
without notice; the rule holds either way, because it rests on *who assessed the
|
||||
issue* rather than on who was technically able to click.
|
||||
|
||||
### Recording the score
|
||||
|
||||
The board stores only the *result* — a card reading `High` with no trace of how
|
||||
it got there — and since the boards are private, that result is also invisible
|
||||
to the reporter. So **post the arithmetic as a comment on the issue.** Without
|
||||
it, a later re-scoring has no way to tell a considered judgment from a guess.
|
||||
|
||||
```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 were misapplied.
|
||||
|
||||
## 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`. A non-zero
|
||||
count means the board contradicts a rule, not that the rule needs revisiting.
|
||||
|
||||
| Check | Invariant | Fix |
|
||||
| --- | --- | --- |
|
||||
| Double-boarded | An issue has a card on **one** board, the one matching its version label | Delete the wrong-board card |
|
||||
| Non-Issue items | **Only issues go on a board** — never PRs, never drafts | Delete the item |
|
||||
| No Status | Every card carries a Status | Set one — `Incoming` if unmilestoned, else by where it actually is |
|
||||
| `Incoming` **with** a milestone (#28) | Incoming ⇔ no milestone | Approval was never recorded: move to **Todo**, or clear the milestone |
|
||||
| Past Incoming **without** a milestone (#28) | Everything past Incoming ⇔ milestoned | Claims an approval nobody made: milestone it, or move back to Incoming |
|
||||
| Wrong board for label | `v1` → #11, `v2` → #28 | Move the card to the right board |
|
||||
| Not exactly one version label | Every issue carries **exactly one** of `v1`/`v2` | Apply the missing one, or remove the extra |
|
||||
| Not exactly one type label | Every issue carries **exactly one** of the five types | Classify it, or remove the extra |
|
||||
| No Priority (#28) | Every board item is prioritized | Score it with the rubric |
|
||||
| Closed, not shipped, still carded | **Done means the work shipped** | Delete the card |
|
||||
| Open, but carded `Done` | A card in Done ⇒ its issue is closed | Close the issue, or move the card back |
|
||||
|
||||
```sh
|
||||
D=$(mktemp -d); R=modelcontextprotocol/inspector
|
||||
# --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:{st:.state, sr:(.stateReason // ""),
|
||||
lab:[.labels[].name], ms:(.milestone.title // null)}}) | from_entries) as $M
|
||||
| def own($s): [$s[].items[]
|
||||
# A DRAFT card has no `.content.repository`, so filtering on equality
|
||||
# alone drops the very items the "non-Issue" check exists to find.
|
||||
| select((.content.repository // null) == null or .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
|
||||
| {
|
||||
"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[] | select(.s=="Incoming" and ms(.n)!=null) | .n],
|
||||
"past Incoming, no ms": [$B28[] | select(.s!=null and .s!="Incoming" and .s!="Done"
|
||||
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, not exactly 1 version label":
|
||||
[$o[0][] | select(.state=="OPEN")
|
||||
| select(([.labels[].name] | map(select(IN("v1","v2"))) | length) != 1)
|
||||
| .number],
|
||||
"open, not exactly 1 type label":
|
||||
[$o[0][] | select(.state=="OPEN")
|
||||
| select(([.labels[].name]
|
||||
| map(select(IN("bug","enhancement","documentation","chore","question")))
|
||||
| length) != 1)
|
||||
| .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],
|
||||
"open, but carded Done": [($B28[], $B11[]) | select(.s=="Done" and isopen(.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.
|
||||
- **`Done` is checked from both sides.** One direction catches a card parked in
|
||||
Done for work that never shipped; the other catches an issue still **open**
|
||||
under a Done card. The second is not hypothetical here — closing keywords do
|
||||
not fire on `v2/main`, so every v2 issue is closed **by hand** after its PR
|
||||
merges, and forgetting that step is the normal way this breaks (Copilot). The
|
||||
other checks cannot see it: they either gate on `isopen` or skip `Done`.
|
||||
- **The two milestone checks are #28-only.** Every milestone in this repo is a
|
||||
v2 release bucket, so a `v1` issue has none it could take — running the
|
||||
Incoming⇔milestone invariant over board #11 would flag every card on it for a
|
||||
state it cannot reach.
|
||||
- **Count the labels; don't test for presence.** The invariant is *exactly
|
||||
one*, so a predicate that only asks "is any version label present" passes an
|
||||
issue carrying **both** `v1` and `v2` — which belongs to two lines at once
|
||||
and shows up in both filtered queries. Same for the five type labels (Copilot).
|
||||
- **Keep drafts inside `own`.** A draft card carries no `.content.repository`,
|
||||
so a plain equality filter removes it before the "non-Issue" check can see it —
|
||||
and that check then reports `0` while the invariant it states (no drafts) is
|
||||
being violated (Copilot). The filter admits an item with no repository and
|
||||
excludes only cards that name a *different* one.
|
||||
- **`$M` holds closed issues too** — the lookup is built from
|
||||
`gh issue list --state all`, which it has to be, because the last check reads
|
||||
closed issues' state reasons. So `isopen` is not there to cope with a missing
|
||||
entry; it is there because the *invariants* are about open work. A closed
|
||||
issue legitimately sits in `Done` with whatever milestone it had, and a check
|
||||
that did not gate on `isopen` would report every one of them (Copilot).
|
||||
|
||||
**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.
|
||||
|
|
@ -1,30 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "Triage the new issues.",
|
||||
"expect": "issue-triage"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I decide what Priority an incoming issue deserves?",
|
||||
"expect": "issue-triage"
|
||||
},
|
||||
{
|
||||
"prompt": "Audit the project board for anything that has drifted.",
|
||||
"expect": "issue-triage"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I find the issues that aren't on either board yet?",
|
||||
"expect": "issue-triage"
|
||||
},
|
||||
{
|
||||
"prompt": "After scoring an issue against the rubric, what do I do with the score?",
|
||||
"expect": "issue-triage"
|
||||
},
|
||||
{
|
||||
"prompt": "Who wrote the novel Moby-Dick?",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "Convert 45 degrees Celsius to Fahrenheit.",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
|
|
@ -1,344 +0,0 @@
|
|||
---
|
||||
name: local-dev
|
||||
description: Install, build and run the Inspector locally, and place a dependency correctly. Use when starting the web, cli, tui or launcher client; when asking whether a build or rebuild is needed before a run script works; when an install, an @inspector/core import or a client bundle fails at run time; when adding or bumping a dependency and choosing which package.json declares it; or when a bundler external, a version skew or a git worktree is the suspect.
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# Local development
|
||||
|
||||
The **rules** a reviewer cites (TypeScript, Mantine/React, testing, the gate)
|
||||
stay in [`AGENTS.md`](../../../AGENTS.md). This skill is how to get the thing
|
||||
running and where a package goes.
|
||||
|
||||
## Install
|
||||
|
||||
Requires Node `>=22.19.0`.
|
||||
|
||||
```sh
|
||||
npm install # at the REPO ROOT
|
||||
```
|
||||
|
||||
v2 is **not** an npm workspace — each client under `clients/*` keeps its own
|
||||
`package.json` and `node_modules`. A single root `npm install` is still all you
|
||||
need: the root `postinstall` (`scripts/install-clients.mjs`) cascades
|
||||
`npm install` into `clients/web`, `clients/cli`, `clients/tui`, and
|
||||
`clients/launcher`.
|
||||
|
||||
- **Fresh clone:** `npm install` at the root.
|
||||
- **After a pull that changes a client's dependencies:** re-run `npm install` at
|
||||
the root to re-sync every client.
|
||||
|
||||
The cascade is dev-only — it exits early when the package is installed under
|
||||
`node_modules`, and a consumer never needs it because the tarball ships
|
||||
**prebuilt** artifacts (each client's `build/`, plus `clients/web/dist` and
|
||||
`clients/web/static`) rather than sources to compile. Set
|
||||
`INSPECTOR_SKIP_CLIENT_INSTALL=1` to skip it. The `files` allowlist itself is
|
||||
[`docs/publishing.md`](../../../docs/publishing.md).
|
||||
|
||||
⚠️ **In a git worktree, do a real `npm install`.** A symlinked `node_modules`
|
||||
passes lint, tests and coverage, then fails every Storybook story file on Vite's
|
||||
`fs.allow`.
|
||||
|
||||
## Running
|
||||
|
||||
For day-to-day **web** iteration, run Vite directly — fast HMR, no launcher
|
||||
build needed:
|
||||
|
||||
```sh
|
||||
cd clients/web && npm run dev
|
||||
```
|
||||
|
||||
The launcher-driven scripts run the **built** launcher, so `npm run build`
|
||||
first:
|
||||
|
||||
```sh
|
||||
npm run build # web → cli → tui → launcher
|
||||
npm run web # prod web launcher against clients/web/dist
|
||||
npm run web:dev # web launcher in --dev mode (Vite)
|
||||
```
|
||||
|
||||
Individual builds: `build:web`, `build:cli`, `build:tui`, `build:launcher`. The
|
||||
web build produces both the browser SPA (`clients/web/dist`, Vite) and the Node
|
||||
prod-server runner (`clients/web/build`, tsup).
|
||||
|
||||
To run the CLI or TUI: `node clients/launcher/build/index.js --cli …` /
|
||||
`--tui …`.
|
||||
|
||||
## The `@inspector/core` alias
|
||||
|
||||
`core/` holds the logic shared by all three clients and intentionally has **no
|
||||
`package.json`** — it is not published on its own. Each client bundles it via a
|
||||
build-time alias:
|
||||
|
||||
- **CLI / TUI** — `esbuildOptions.alias` in `tsup.config.ts` maps
|
||||
`@inspector/core` → the repo `core/` directory, with
|
||||
`noExternal: [/^@inspector\/core/]` inlining it.
|
||||
- **Web** — **two** configs, because the client is two builds:
|
||||
`clients/web/vite.config.ts` for the browser app, and
|
||||
`clients/web/tsup.runner.config.ts` for the Node backend runner, which
|
||||
defines the alias itself. Dependency or debugging work on the runner belongs
|
||||
in the tsup config, not the Vite one (Copilot).
|
||||
|
||||
## Where a dependency goes
|
||||
|
||||
**The rules are in [`AGENTS.md`](../../../AGENTS.md) → Dependency placement, and
|
||||
they are not restated here.** Read them there and come back for the *why* — what
|
||||
each rule is defending against, what it looked like when it was violated, and how
|
||||
to tell you have hit one.
|
||||
|
||||
Work the rules in order: which manifest declares it, then `dependencies` vs
|
||||
`devDependencies`, then whether the bundlers must also externalise it.
|
||||
|
||||
### Why `core/`'s runtime dependencies are root-only
|
||||
|
||||
A per-client declaration installs a **second copy** that drifts from the root's.
|
||||
Not theoretical: it put two versions of `ext-apps` (1.7.4 / 1.7.5) and of the
|
||||
transitive v1 `@modelcontextprotocol/sdk` (1.29.0 / 1.30.0) in the tree at once
|
||||
(#1970). A second copy of `client`/`core` is exactly the failure the `dedupe` +
|
||||
`server.deps.inline` workaround in `vitest.shared.mts` exists for.
|
||||
|
||||
The same reasoning extends to anything reached only through root-owned code that
|
||||
has no manifest of its own (`test-servers/src`, `core/`). #2195 made that the
|
||||
general case: `ajv`, `atomically`, `chokidar`, `hono`, `@napi-rs/keyring`,
|
||||
`pino`, `react` and `zod` moved to the root, joining `proper-lockfile` and
|
||||
`undici`, which were already there.
|
||||
|
||||
Keep two distinctions straight, because AGENTS.md's rules split on them:
|
||||
|
||||
- **Root-declared is not the same as `core/`-imported.** `commander`, `open` and
|
||||
`@hono/node-server` are root `dependencies` too, but they are reached only
|
||||
from client code. Only the `core/` set has to appear in *all three* bundler
|
||||
`external` lists.
|
||||
- **Root-declared is not the same as aliased.** The `vitest.shared.mts` pins and
|
||||
the `clients/web/tsconfig.*.json` `paths` cover the packages whose resolution
|
||||
is genuinely ambiguous, which is two different situations: the importer is
|
||||
`core/`, which has no manifest to resolve from, **or** a copy still sits in
|
||||
the client install and would win. The second case is the one that is easy to
|
||||
get wrong — `chokidar` (under `vite`), `zod` (under
|
||||
`eslint-plugin-react-hooks`), `open` (under Storybook) and `react` (a peer of
|
||||
`react-dom` and `ink`) are all present in a client's `node_modules` without
|
||||
being declared there, so "we deleted the declaration" is not the same as "it
|
||||
resolves from the root now". **Check the install, don't reason from the
|
||||
manifest.** `ajv`, `commander` and `undici` carry no pin because no client
|
||||
install has a copy of them at all.
|
||||
|
||||
The point of deleting the client-side copies rather than merely keeping them in
|
||||
step is that **a package installs only into an install root that declares it**.
|
||||
Aligned duplicate declarations still drift the next time someone bumps one of
|
||||
them; no declaration at all cannot. `npm run verify:dep-lockstep` is the detector
|
||||
for the skew — in both its tiers since #2226 — and consolidation is what removes
|
||||
the opportunity.
|
||||
|
||||
Two consequences that read as bugs and are not:
|
||||
|
||||
- **`clients/cli` and `clients/launcher` declare no runtime dependencies.** Their
|
||||
manifests carry `devDependencies` only. Everything they import at runtime is
|
||||
root-declared and resolves by walking up from the client directory.
|
||||
- **A client's `node_modules` still contains some of these names.** They arrive
|
||||
transitively (`chokidar` under `vite`, `react` as a peer of `react-dom` and
|
||||
`ink`). That is why the `vitest.shared.mts` pins matter: an unpinned specifier
|
||||
would resolve the nearest copy, which is the transitive one, not the root's.
|
||||
|
||||
When a package moves to the root, its `vitest.shared.mts` pin has to move with
|
||||
it — a `path.resolve(dirname, …)` pin left behind points at a directory that no
|
||||
longer exists, or (for the transitive cases above) at the duplicate the pin was
|
||||
supposed to rule out.
|
||||
|
||||
**`react` and `react-dom` are the exception, and stay pinned per client.** They
|
||||
are a matched pair — `react-dom` reaches into React internals — and `react-dom`
|
||||
is still web-declared, so npm resolves it and its React peer together inside
|
||||
`clients/web/node_modules`. Pointing
|
||||
`react` at the root while `react-dom` resolves from the client would pair a
|
||||
renderer with a React it was not installed against, which is the same split the
|
||||
pin exists to prevent, arrived at from the other side. `dedupe` still collapses
|
||||
each install to one copy, which is what actually has to hold.
|
||||
|
||||
### Why the shared toolchain is root-only
|
||||
|
||||
Every client's `validate` shells out to `prettier`, `eslint`, `tsc` and `vitest`,
|
||||
which makes them look like they belong beside the scripts that call them. They
|
||||
do not need to be: `npm run` prepends **every ancestor** `node_modules/.bin` to
|
||||
`PATH`, and Node and TypeScript walk parent `node_modules` /
|
||||
`node_modules/@types` the same way — so a client that declares no toolchain at
|
||||
all still resolves the root's copy. `clients/launcher` declares no
|
||||
`devDependencies` whatsoever and its `validate` is unchanged.
|
||||
|
||||
What a per-client declaration *does* buy is a second copy free to drift, and it
|
||||
had (#2196): `globals` sat at `^17.7.0` at the root against `^17.4.0` in all four
|
||||
clients, and `typescript-eslint` at `^8.65.0` against `^8.56.1`. Nothing failed —
|
||||
which is the point. A lint or format tool that differs per client makes the gate's
|
||||
verdict a function of *where you ran it*, and the exact `prettier` pin (#1790)
|
||||
only means something when there is one of it.
|
||||
|
||||
⚠️ The line is **used by every client**, not "used by one" and not "is it
|
||||
toolchain". `tsx`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`,
|
||||
`vite-node` and each client's own `@types/*` are toolchain too and stay where
|
||||
they are — hoisting them would make every client install the union of all four.
|
||||
So do the ones **more than one** client declares without all of them doing so:
|
||||
`tsup` sits in web, cli and tui, and `vite` in web and tui on top of the root
|
||||
*runtime* `dependency` that `--web --dev` needs. Neither is in scope here;
|
||||
whether to consolidate them is a separate call with a separate rationale (`vite`
|
||||
especially, since its root declaration is a `dependency`, not a
|
||||
`devDependency`).
|
||||
|
||||
#### What the walk-up does *not* buy you
|
||||
|
||||
⚠️ **Deleting a client's declaration does not always delete the copy** — and
|
||||
where a copy survives, it is the one that wins. Two mechanisms put one back,
|
||||
neither of which the manifest mentions:
|
||||
|
||||
- **An unmet peer.** npm auto-installs a peer into the install that needs it, and
|
||||
a client install has no visibility into the root's tree, so the root's copy
|
||||
cannot satisfy it. Web's `eslint-plugin-react-refresh` / `eslint-plugin-storybook`
|
||||
and the TUI's `eslint-plugin-react-hooks` each pull a client-local `eslint`;
|
||||
web's Storybook/Vitest stack pulls a local `typescript` and `vitest`.
|
||||
- **A hoisted transitive.** `@types/express` brings `@types/node` into web and
|
||||
cli's trees on its own.
|
||||
|
||||
Those copies sit *nearer* than the root's, so `clients/web/node_modules/.bin`
|
||||
precedes the root bin directory on `PATH` and TypeScript resolves the nearest
|
||||
`node_modules/@types`. Verify with `npm exec -- which eslint` from the client
|
||||
rather than assuming — the assumption is what made the first cut of #2196 claim
|
||||
more than it delivered (Copilot).
|
||||
|
||||
So the consolidation buys **one declaration and one place to bump**, not one copy
|
||||
on disk. The two mechanisms are **not** equally safe, and neither is a guarantee:
|
||||
|
||||
- A **peer** copy is at least constrained by its holder's peer range. That is a
|
||||
real pin only when the range is exact — `@vitest/browser-playwright` pins
|
||||
`vitest` to a single version, which is why the trio below is pinned too. For a
|
||||
wide range (`eslint-plugin-react-refresh` accepts `^9 || ^10`) the copies agree
|
||||
only because npm happens to resolve the same latest in both installs, which is
|
||||
a coincidence that holds until it doesn't.
|
||||
- A **transitive** copy is constrained by nothing of ours whatsoever, and one had
|
||||
already diverged: cli's `@types/node` was `24.13.1` against the root's
|
||||
`24.13.3`, on `v2/main` too — a declared `^24.12.4` loses to a nearer
|
||||
transitive. It is pinned with an `overrides` entry in `clients/cli` since
|
||||
#2226.
|
||||
|
||||
✅ **`verify:dep-lockstep` gates both of those since #2226.** Its second tier
|
||||
compares every package **any** install *declares* — `dependencies`,
|
||||
`devDependencies` and `optionalDependencies`, unioned across the root and all
|
||||
four clients — against every **top-level** copy in every install, independent of
|
||||
what a `tsc` program resolves. So a tool *binary* that no program loads
|
||||
(`eslint`, `typescript`, `vitest`) and a transitive copy that no single program
|
||||
meets (the cli `@types/node` above) are both in scope now, as is a skew between
|
||||
two **clients** with no root copy involved (`@types/react`, web against tui).
|
||||
|
||||
⚠️ **Two gaps remain, and they are why the by-hand check below is still worth
|
||||
running.** The tier reads **lockfiles**, so a copy you installed by hand and
|
||||
never committed is invisible to it; and it compares only names some manifest
|
||||
declares, so a purely transitive package no manifest anywhere names stays the
|
||||
program tier's business. When you change what a client declares, check by hand
|
||||
from that client:
|
||||
|
||||
```sh
|
||||
cd clients/web && npm exec -- which eslint prettier tsc vitest
|
||||
```
|
||||
|
||||
#### Why the vitest trio is pinned exactly
|
||||
|
||||
`@vitest/browser-playwright` declares an **exact** peer on `vitest` (`"vitest":
|
||||
"4.1.10"`, not a range), so that package — not the root's range — decides which
|
||||
`vitest` lands in `clients/web`. Left floating, the root resolves the newest
|
||||
patch while web's peer stays pinned to the older one, and web's tests then run on
|
||||
one `vitest` while loading a `@vitest/coverage-v8` provider built against
|
||||
another. Both would still pass, which is the bad part.
|
||||
|
||||
So `vitest` and `@vitest/coverage-v8` at the root and `@vitest/browser-playwright`
|
||||
in `clients/web` are all pinned **exactly**, and a bump edits all three in one
|
||||
change — the same discipline as the exact `prettier` pin (#1790).
|
||||
|
||||
### Why runtime consumption decides `dependencies`
|
||||
|
||||
The client builds externalise npm packages, so a published install resolves them
|
||||
from the **root** manifest — and devDependencies are not installed for
|
||||
consumers. A runtime import parked there therefore **breaks the published
|
||||
package while passing every local check**, which is the worst shape a mistake
|
||||
can take here.
|
||||
|
||||
Two live examples worth knowing:
|
||||
|
||||
- `express` is test-only, so it is a devDependency.
|
||||
- `vite` and `@vitejs/plugin-react` look like build tooling but are root
|
||||
`dependencies`, because `clients/web/server/start-vite-dev-server.ts` imports
|
||||
them at runtime for `--web --dev`. Moving them would break that for consumers
|
||||
while every local check stayed green.
|
||||
|
||||
### Why a root dependency must also be externalised
|
||||
|
||||
tsup and Vite externalise what the **client's** `package.json` declares, and a
|
||||
root-only dependency is in none of them — so it is **bundled**, silently. For a
|
||||
CJS package inlined into an ESM bundle that is fatal: esbuild leaves a
|
||||
`Dynamic require of "path" is not supported` shim that throws at *import* time,
|
||||
so the binary dies before it parses a flag (`proper-lockfile`, #2082).
|
||||
|
||||
`undici` (#2067) is the worse variant, because it is `import()`ed lazily: the
|
||||
binary boots fine and only the proxied path dies — with esbuild's rewritten
|
||||
specifier (`import("./undici-HXPKCIY3.js")`) meaning **no user-side install can
|
||||
ever satisfy it**. It was declared in `clients/cli/package.json`, so the CLI
|
||||
looked correct while web and TUI silently inlined 1.05 MB of it.
|
||||
|
||||
**How to tell.** The ordinary tiers cannot: unit and integration tests run
|
||||
against source, where the real package is on the resolution path, and the smokes
|
||||
never take the lazy path. `npm run verify:bundle-externals` reads the **built
|
||||
output** for exactly this reason — the config and the output disagreed for four
|
||||
releases.
|
||||
|
||||
⚠️ **Probing by hand with `node -e` falsely succeeds.** `node -e` exposes a
|
||||
global `require` that satisfies esbuild's `typeof require !== "undefined"`
|
||||
guard, so the chunk looks loadable when it is not. Reproduce from a real `.mjs`
|
||||
file.
|
||||
|
||||
### Why React-rendering packages are the exception
|
||||
|
||||
An externalised package resolves its own `react` from wherever npm placed
|
||||
**it** — beside a React satisfying *that package's* peer range, which is looser
|
||||
than ours in every case here. `ink-form` and `ink-scroll-view` declare `">=18"`,
|
||||
so a consumer's React 18 satisfies them and hoists them while our React 19 nests
|
||||
underneath: the bundle renders through one React, those packages call hooks on
|
||||
another, and the TUI crashes on the first hook (#1952).
|
||||
|
||||
`ink` is exempt on **cost** (~1.4 MB of `react-reconciler` + `yoga-layout`, plus
|
||||
a `createRequire` banner). ⚠️ **Never justify that exemption by a peer range** —
|
||||
it briefly read "its `">=19"` peer keeps npm honest", which is false: a consumer
|
||||
pinning React 19.0 satisfies `">=19"` while a narrower range of ours nests
|
||||
underneath. What makes it safe is the *root `react` range staying open to the
|
||||
whole major*, so npm can dedupe. `clients/tui/__tests__/tsupConfig.test.ts`
|
||||
enforces the whole split, the exemption included.
|
||||
|
||||
### Why a version skew is worth aligning rather than working around
|
||||
|
||||
Because v2 is not a workspace, the same package can appear **twice in one `tsc`
|
||||
program** — a client's own install plus the root's, reached through `core/` or
|
||||
`test-servers/src`. At the same version that is harmless; on a skew TypeScript
|
||||
must relate two structurally-distinct declarations, which for a
|
||||
recursive-generic surface is exponential. A zod `4.3.6` / `4.4.3` skew exhausted
|
||||
the 4 GB tsc heap outright with `TS2589` (#1896).
|
||||
|
||||
⚠️ **Raising the heap hides the class rather than fixing it.** Align the
|
||||
versions; `npm run verify:dep-lockstep` is the guard, and it runs **two tiers**.
|
||||
The first derives its candidate set from what actually enters each `tsc`
|
||||
program, so a package whose declarations arrive only through another package's
|
||||
`.d.ts` is still seen. The second (#2226) compares every **declared** package's
|
||||
top-level copies across installs whatever any program loads, so a transitive or
|
||||
peer-shadow drift no program can meet is caught too.
|
||||
|
||||
Aligning a stale install is `npm update <pkg>` there — it moves the lockfile
|
||||
within the declared range without widening the range, which
|
||||
`npm install <pkg>@<version>` would. A transitive copy that will not move takes
|
||||
an `overrides` entry in that install (see the next section).
|
||||
|
||||
### Why `overrides` beats `npm audit fix`
|
||||
|
||||
`tsup@8.5.1` declares `esbuild: ^0.27.0`, and the advisory covers
|
||||
`0.27.3 - 0.28.0` with `0.27.7` the last 0.27.x — so there is no *upward* escape
|
||||
inside that range, and `npm audit fix` "resolves" it by silently **downgrading**
|
||||
to `0.27.2` across three installs (~700 lines of lockfile churn for a low-severity
|
||||
dev-only advisory; tried and reverted in #2058). The override forces one deduped
|
||||
copy above the range instead.
|
||||
|
||||
Two costs: it puts `tsup` on an esbuild **major it does not declare**, so
|
||||
`npm run build` — not `npm audit` — is the real gate on such a pin; and it is
|
||||
invisible to the audit once clean, so **drop it when `tsup` widens its range**
|
||||
rather than carrying it forever.
|
||||
|
|
@ -1,30 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "What is the difference between the launcher-driven run scripts and starting a client directly, and when should I use each?",
|
||||
"expect": "local-dev"
|
||||
},
|
||||
{
|
||||
"prompt": "I added a package and now one of the client bundles fails at run time. What do I check?",
|
||||
"expect": "local-dev"
|
||||
},
|
||||
{
|
||||
"prompt": "Do I need to rebuild anything before I can run `npm run web` in this repo?",
|
||||
"expect": "local-dev"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I run the CLI from the built launcher in this repo instead of via npx?",
|
||||
"expect": "local-dev"
|
||||
},
|
||||
{
|
||||
"prompt": "Do I need to do anything special to set this repo up inside a git worktree?",
|
||||
"expect": "local-dev"
|
||||
},
|
||||
{
|
||||
"prompt": "Write a haiku about autumn.",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "Explain the difference between let and const in JavaScript.",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
|
|
@ -1,328 +0,0 @@
|
|||
---
|
||||
name: pr-flow
|
||||
description: Take an issue through to a merged PR in this repo, and what to do at each step. Use when asked to open, create or submit a PR; when a DCO or signoff check fails; when requesting a Copilot review or responding to review comments; when naming a branch; when attaching screenshots to a PR; or when closing out after a merge.
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# PR flow
|
||||
|
||||
Rules this procedure enforces live in [`AGENTS.md`](../../../AGENTS.md)
|
||||
(Issue-driven Work Style, Contributing). Board mechanics are in `/board-ops`;
|
||||
the gate itself is `/pre-push-gate`.
|
||||
|
||||
**Pull requests against this repo are opened by the repo maintainers only.**
|
||||
Having write access is not authorization to open one — anyone else files a
|
||||
detailed issue and a maintainer takes it from there.
|
||||
|
||||
## 1. Start from an issue
|
||||
|
||||
**Every PR references an issue. No exceptions, regardless of who opens it.** A
|
||||
PR with no linked issue has no board card, so the work is invisible to the
|
||||
project board and untracked. If there's no issue yet, create one first with
|
||||
`/issue-create` — don't open the PR and backfill.
|
||||
|
||||
Move the issue's card to **In Progress** (`/board-ops`).
|
||||
|
||||
## 2. Branch
|
||||
|
||||
**Branch names start with the target version segment** — the first path segment
|
||||
is the version whose base branch the PR targets, then the type, then a slug:
|
||||
|
||||
```
|
||||
v2/fix/2071-oauth-resource-metadata
|
||||
v2/chore/2146-rename-local-gate
|
||||
v1/fix/proxy-ssrf-pin
|
||||
```
|
||||
|
||||
Not `fix/oauth-resource-metadata`. This keeps the two lines legible in
|
||||
`git branch -a` and in the PR list once v1 and v2 branches coexist on the same
|
||||
remote, and it matches the base branches themselves (`v2/main`, `v1/main`).
|
||||
|
||||
**Cut the branch from the base it will target** — `v2/main` for v2 work,
|
||||
`v1/main` for v1. The two lines have unrelated histories, so a `v1/fix/…`
|
||||
branch cut from `v2/main` arrives at `v1/main` carrying the whole v2 tree
|
||||
(Copilot). And never cut from a milestone-merge branch — it carries release-only
|
||||
commits that will show up in your PR's diff.
|
||||
|
||||
Working in a git worktree is fine and often preferable. ⚠️ **A worktree needs a
|
||||
real `npm install`, not a symlinked `node_modules`** — a symlink passes
|
||||
lint/test/coverage and then fails all Storybook story files on Vite's
|
||||
`fs.allow`.
|
||||
|
||||
For order-dependent PRs on one issue, **stack** them: each branch is based on the
|
||||
previous one, not all cut from `v2/main`.
|
||||
|
||||
## 3. Sign off every commit
|
||||
|
||||
**The DCO check is a hard merge gate.** The [probot DCO
|
||||
app](https://probot.github.io/apps/dco/) fails the PR unless each commit carries
|
||||
a `Signed-off-by: Name <email>` trailer whose name **and** email match either the
|
||||
commit's author or its committer. Its only exemptions are merge commits and
|
||||
bot-authored commits; there is no partial credit — one unsigned commit out of six
|
||||
fails the whole check.
|
||||
|
||||
**Prevent it with `git commit -s`.** Two things that look like automation and are
|
||||
not:
|
||||
|
||||
- ⚠️ **`git config format.signOff true` does nothing here.** Despite the name it
|
||||
only defaults the `-s` flag for `git format-patch`; `git commit` never reads it,
|
||||
and there is no `commit.signoff` equivalent.
|
||||
- ⚠️ **A `prepare-commit-msg` hook works, but think before installing one.** The
|
||||
trailer is a certification, and a hook makes it on your behalf for _every_
|
||||
commit, including work you merely cherry-picked. Inside that hook,
|
||||
`git var GIT_AUTHOR_IDENT` returns your config identity rather than the
|
||||
preserved author, so it cannot even tell it is signing for someone else.
|
||||
|
||||
**Repairing already-pushed commits** means rewriting them:
|
||||
|
||||
```sh
|
||||
git rebase HEAD~<n> --signoff
|
||||
git push --force-with-lease
|
||||
```
|
||||
|
||||
Use `--force-with-lease` rather than `--force`, and only rewrite when you are the
|
||||
sole author and nobody else has based work on the branch. The two apparent
|
||||
alternatives are not alternatives: the app's empty "remediation commit" flow
|
||||
requires `allowRemediationCommits.individual` and this repo ships no
|
||||
`.github/dco.yml`, so it runs disabled; and the override button anyone with write
|
||||
access sees only silences the check without anyone certifying anything.
|
||||
|
||||
The signoff is a [Developer Certificate of
|
||||
Origin](https://developercertificate.org/) assertion made in **your own name**. It
|
||||
does not claim you wrote the code, so signing off a cherry-pick is legitimate.
|
||||
What is never acceptable is fabricating _someone else's_ certification.
|
||||
|
||||
## 4. Run the gate
|
||||
|
||||
`npm run format`, then `npm run local:gate`. See `/pre-push-gate` — `npm run
|
||||
validate` is **not** a substitute.
|
||||
|
||||
## 5. Screenshots, for any UI change
|
||||
|
||||
Any change to the web UI or the TUI must show its result: capture before/after
|
||||
screenshots (or a short GIF for an interaction) into a **`pr-screenshots/`
|
||||
folder off the repo root**, creating it if it doesn't exist. That folder is
|
||||
**gitignored** — the images are working artifacts staged for upload, never
|
||||
committed — so attach them to the PR body from there rather than referencing an
|
||||
in-repo path. Name them for what they show (`tools-tab-before.png`), not
|
||||
`Screenshot 2026-07-31 at 14.02.11.png`.
|
||||
|
||||
### 5a. Capture settings — web
|
||||
|
||||
Everything in 5a is about a **browser** capture and assumes Playwright driving
|
||||
the web client. A **TUI** change has no viewport and no `fullPage` mode: size
|
||||
the terminal so no line wraps or truncates, and go straight to 5b, which applies
|
||||
to every image regardless of how it was taken.
|
||||
|
||||
**Shoot the web client at 1280×900, full page.** It is the one size already
|
||||
written down anywhere in the repo — `scripts/smoke-web-tabs.mjs` and
|
||||
`scripts/smoke-web-elicitation.mjs` set exactly that viewport (the other two web
|
||||
smokes set none) — and adopting it as the standard here is what makes a reviewer
|
||||
comparing two PRs compare the same thing. The older shots checked into
|
||||
`specification/screenshots/` were taken at assorted sizes, which is the problem,
|
||||
not the precedent. Prefer a full-page shot over a
|
||||
Playwright `clip` region: a clip sized to one panel cuts off anything placed
|
||||
beside it, and two clips of different sizes make a before/after pair hard to
|
||||
read as a pair.
|
||||
|
||||
⚠️ **Widening the window does not widen the Monitor sidebar.** The
|
||||
main/sidebar split is a draggable divider whose width is stored independently of
|
||||
the viewport (`localStorage["inspector.monitor.width"]`, default **420px**,
|
||||
clamped to **320–720**), so a bigger screen grows the _content_ column and
|
||||
leaves the sidebar exactly as clipped as it was. Both levers have to be set, and
|
||||
only one of them is obvious. On #2234 this cost three full re-captures: the
|
||||
first set clipped the sidebar, the second still clipped it after only the window
|
||||
was widened, and the third worked once the divider itself was moved.
|
||||
|
||||
**So when a shot includes the Monitor sidebar, set its width explicitly** —
|
||||
give it enough room that no row truncates, favoring the sidebar over the
|
||||
left-hand list, which usually has room to give up. Two ways, in order of
|
||||
preference:
|
||||
|
||||
```js
|
||||
// Deterministic: seed the stored width before the app loads.
|
||||
await context.addInitScript(() =>
|
||||
localStorage.setItem("inspector.monitor.width", "640"),
|
||||
);
|
||||
```
|
||||
|
||||
```js
|
||||
// Or drive the divider itself — it is a keyboard-operable ARIA separator,
|
||||
// and ArrowLeft widens the sidebar one 16px step per press.
|
||||
const handle = page.getByRole("separator", {
|
||||
name: "Resize monitoring sidebar",
|
||||
});
|
||||
await handle.focus();
|
||||
for (let i = 0; i < 14; i++) await handle.press("ArrowLeft");
|
||||
```
|
||||
|
||||
Two more mechanics worth setting before the shutter:
|
||||
|
||||
- **Wait ~900ms after switching the main view.** The Servers→Tools switch is a
|
||||
crossfade, so an immediate shot renders _both_ views stacked translucently and
|
||||
reads as a broken app. Waiting on a locator in the incoming view is not enough —
|
||||
the outgoing one is still fading.
|
||||
- **Mark focus when the change is about focus.** Tab order and keybinding fixes
|
||||
look identical at rest, so after driving the keystroke, `page.evaluate` over
|
||||
`document.activeElement`, outline it, and log its tag + `aria-label` — that
|
||||
line is the actual assertion and the image is the evidence. **Say in the PR
|
||||
body that the outline is script-added**, not app UI.
|
||||
|
||||
### 5b. Read the shot back before uploading — web and TUI
|
||||
|
||||
**Open every image and confirm nothing is cut off at either edge** — no
|
||||
truncated row, clipped badge, or value running under a panel border, and no
|
||||
half-faded view. This is a real check with your own eyes, not a formality: a
|
||||
clipped screenshot is worse than no screenshot, because a reviewer reads the
|
||||
truncation as a rendering bug in the feature under review and files it back at
|
||||
you. Re-shoot rather than shipping one that "mostly" shows the change.
|
||||
|
||||
### 5c. Upload
|
||||
|
||||
To host them, upload to GitHub's attachment endpoint with your `gh` token. Two
|
||||
mechanics, both of which bite:
|
||||
|
||||
- The parameters go in the **query string**, with the raw bytes as the body. A
|
||||
JSON body fails with a misleading "Invalid name for request".
|
||||
- ⚠️ **Do not put the token in argv.** `-H "Authorization: token $(gh auth
|
||||
token)"` puts your credential in curl's command line, where any local user or
|
||||
process can read it off the process table while the upload runs (Copilot).
|
||||
Feed it through `--config -` instead: curl reads its options from stdin, so
|
||||
the token never becomes an argument.
|
||||
|
||||
```sh
|
||||
printf 'header = "Authorization: token %s"\n' "$(gh auth token)" | curl -sS --config - \
|
||||
-X POST --data-binary @pr-screenshots/tools-tab-after.png \
|
||||
"https://uploads.github.com/user-attachments/assets?repository_id=<REPO_ID>&name=tools-tab-after.png&content_type=image/png"
|
||||
```
|
||||
|
||||
(The token is still in the shell's environment and in `printf`'s _stdin_, which
|
||||
is not world-readable the way `/proc/<pid>/cmdline` is.)
|
||||
|
||||
## 6. Open the PR
|
||||
|
||||
Target the base branch matching the work: **`v2/main`** for v2, **`v1/main`**
|
||||
for v1. Never `main`.
|
||||
|
||||
The body's **first line is `Closes #<ISSUE_NUMBER>`**.
|
||||
|
||||
```sh
|
||||
gh pr create --repo modelcontextprotocol/inspector \
|
||||
--base v2/main --label v2 \
|
||||
--title "<title>" --body "Closes #<N>
|
||||
|
||||
<what changed and why>"
|
||||
```
|
||||
|
||||
⚠️ Closing keywords only auto-link and auto-close for PRs targeting the repo's
|
||||
**default branch** (`main`). Because v2 PRs target `v2/main`, `Closes #N` there
|
||||
is only a cross-reference — it will **not** create a hard link or close the issue
|
||||
on merge. Keep it anyway, so the issues close if/when `v2/main` reaches `main`.
|
||||
There is no `gh` flag for manual linking; closing keywords are the only
|
||||
mechanism GitHub exposes.
|
||||
|
||||
Move the card to **In Review**.
|
||||
|
||||
## 7. Request a Copilot review
|
||||
|
||||
Only the GraphQL `requestReviews` mutation with the Copilot **bot id** works —
|
||||
REST, `gh pr edit --add-reviewer`, `userIds`, and `copilot-swe-agent` all fail or
|
||||
silently drop.
|
||||
|
||||
```sh
|
||||
PR_ID=$(gh pr view <N> --repo modelcontextprotocol/inspector --json id --jq .id)
|
||||
gh api graphql -f query='
|
||||
mutation($pr:ID!,$bot:[ID!]!) {
|
||||
requestReviews(input:{pullRequestId:$pr, botIds:$bot, union:true}) {
|
||||
pullRequest { id }
|
||||
}
|
||||
}' -f pr="$PR_ID" -f bot='BOT_kgDOCnlnWA'
|
||||
```
|
||||
|
||||
Poll for the review with a `startswith` match — the review login carries a
|
||||
`[bot]` suffix. **Put that poll in one backgrounded loop that exits when the
|
||||
round lands, and wait for its notification** rather than re-fetching once per
|
||||
turn; a review is remote state the harness cannot observe, which is exactly the
|
||||
exception described in [Waiting on long-running
|
||||
work](../../../AGENTS.md#waiting-on-long-running-work) — and exactly where the
|
||||
poll belongs when one is needed.
|
||||
|
||||
```sh
|
||||
EXPECTED=1 # the review COUNT you are waiting to reach — see below
|
||||
while :; do
|
||||
# Capture first, so a gh failure stops the loop instead of being swallowed by
|
||||
# a pipeline. --slurp cannot be combined with --jq, hence the separate jq.
|
||||
raw=$(gh api --paginate --slurp \
|
||||
repos/modelcontextprotocol/inspector/pulls/<N>/reviews) || {
|
||||
echo "gh api failed ($?) — not retrying blind" >&2; exit 1; }
|
||||
n=$(jq '[.[][] | select(.user.login | startswith("copilot-pull-request-reviewer"))] | length' <<<"$raw") || {
|
||||
echo "jq failed ($?) on an unexpected response shape" >&2; exit 1; }
|
||||
case $n in '' | *[!0-9]*) echo "not a count: '$n'" >&2; exit 1 ;; esac
|
||||
[ "$n" -ge "$EXPECTED" ] && break
|
||||
sleep 30
|
||||
done
|
||||
```
|
||||
|
||||
`EXPECTED` is the review **count** you are waiting to reach, so it is `1` only
|
||||
on the first round — on round two the first round's review is still there and an
|
||||
existence check returns immediately. `sleep 30` is the remote-API floor the rule
|
||||
above sets. **Every step that can fail exits the loop rather than
|
||||
retrying.** Piping the count straight into `awk` would make an auth or API error
|
||||
read as a count of `0`; and a `jq` failure on an unexpected shape leaves `n`
|
||||
empty, whereupon `[ "" -ge 1 ]` exits non-zero, `break` never fires, and the job
|
||||
sleeps and retries forever — the same unbounded wait, reached from the other
|
||||
end. A background task that can never succeed is worse than one that never
|
||||
started, because it looks like progress. Give the inline comments a further ~60s after the body lands; they
|
||||
arrive late (see step 8).
|
||||
|
||||
## 8. Respond to the review
|
||||
|
||||
- It is **not** necessary to implement every suggestion. Implementing one a
|
||||
different way, or declining it with a reason, is fine.
|
||||
- After making the changes, **reply to each review comment in its own thread**
|
||||
with what was done, or why it was declined. That inline reply is the primary
|
||||
response and it is not optional — each review comment is a discussion thread
|
||||
with its own resolve state, and a reply _in_ the thread is the only thing a
|
||||
reviewer reading that thread sees. It does **not** resolve the thread:
|
||||
resolving is a separate act — the "Resolve conversation" button, or the
|
||||
`resolveReviewThread` GraphQL mutation — and it is the reviewer's to make. The
|
||||
reply is what makes resolving it defensible.
|
||||
|
||||
```sh
|
||||
# Fetch the round's comments by REVIEW id — the unpaginated /reviews listing
|
||||
# hides later rounds behind your own replies.
|
||||
# --paginate: this endpoint returns 30 per page, and a round you only half
|
||||
# fetch is a round you only half answer.
|
||||
gh api --paginate repos/modelcontextprotocol/inspector/pulls/<N>/reviews/<REVIEW_ID>/comments \
|
||||
--jq '.[]|"\(.id) \(.path):\(.line)\n\(.body)"'
|
||||
|
||||
# Reply into one thread, keyed by the comment id from above.
|
||||
gh api repos/modelcontextprotocol/inspector/pulls/<N>/comments/<COMMENT_ID>/replies \
|
||||
-f body='Fixed in <sha> — …'
|
||||
```
|
||||
|
||||
- ⚠️ **Then mirror the round at PR level, in addition — never instead.** Inline
|
||||
replies go hidden once the fix is pushed, because the threads become outdated,
|
||||
so a summary comment is what keeps the round readable afterwards. It does
|
||||
**not** discharge the per-comment replies: a rollup bullet cannot be connected
|
||||
back to the thread it answers, so the thread stays open with a finding and
|
||||
silence in it, and by round three matching bullets to comments is
|
||||
reconstruction rather than reading.
|
||||
- ⚠️ Always read the **"Suppressed comments"** block in the review body. Those
|
||||
findings have no comment id, so they have no thread to reply into — the
|
||||
PR-level mirror is the only place they can be answered, and it is the one case
|
||||
where answering there is the whole response.
|
||||
- ⚠️ **Copilot's inline comments lag its review body.** The body's "generated N
|
||||
comments" count lands first; fetch by recency and reconcile. Repeated
|
||||
re-review silence means the session ended.
|
||||
|
||||
## 9. Merge and close out
|
||||
|
||||
**On merge of a v2 PR, manually close its issue and move the board item to
|
||||
Done**, since auto-close won't fire on `v2/main`. Use the move-a-card recipe in
|
||||
`/board-ops` — the option IDs are unstable, so this file names the column and
|
||||
nothing else.
|
||||
|
||||
`Done` is only for work that **shipped** — a merged PR, or a parent whose last
|
||||
sub-issue closed. Anything else (duplicate, won't fix, not planned, obsolete)
|
||||
means nothing shipped, so **delete the card** instead; see `/board-ops`.
|
||||
|
|
@ -1,30 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "I've finished the fix for issue 2071. Take it through to a pull request.",
|
||||
"expect": "pr-flow"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I get Copilot to review a PR in this repo?",
|
||||
"expect": "pr-flow"
|
||||
},
|
||||
{
|
||||
"prompt": "The DCO check is red on my branch. How do I fix the commits I already pushed?",
|
||||
"expect": "pr-flow"
|
||||
},
|
||||
{
|
||||
"prompt": "Copilot left a pile of review comments on my PR. How do I answer them so nothing gets lost?",
|
||||
"expect": "pr-flow"
|
||||
},
|
||||
{
|
||||
"prompt": "My PR is open and green. Walk me through getting it merged and closed out here.",
|
||||
"expect": "pr-flow"
|
||||
},
|
||||
{
|
||||
"prompt": "What does this regex match? /^[a-z]+$/",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "Translate 'good morning' into Spanish.",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
|
|
@ -1,214 +0,0 @@
|
|||
---
|
||||
name: pre-push-gate
|
||||
description: Diagnose a failing stage of this repo's pre-push gate, and run it correctly. Use when npm run local:gate or npm run validate fails; when lint, coverage, a smoke, or Storybook goes red; when a stage behaves differently inside a git worktree; when the whole run times out; or when deciding what to run before pushing.
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# The pre-push gate
|
||||
|
||||
**The rule** (stated in [`AGENTS.md`](../../../AGENTS.md)): run `npm run format`
|
||||
before committing and **`npm run local:gate`** before pushing. `npm run
|
||||
validate` is the fast inner-loop check and is **not** a substitute — it runs no
|
||||
coverage gate, no smokes, and no Storybook tests.
|
||||
|
||||
The reference for what each stage covers, and the CI-vs-local split, is
|
||||
[`docs/quality-gate.md`](../../../docs/quality-gate.md). This skill is how to
|
||||
run it and what to do when it goes red.
|
||||
|
||||
## The two commands
|
||||
|
||||
```sh
|
||||
cd <repo root>
|
||||
npm run format # auto-fix: core/, scripts/, the shared surface, every client
|
||||
npm run local:gate # several minutes
|
||||
```
|
||||
|
||||
The stages it runs, in order, are listed in
|
||||
[`docs/quality-gate.md`](../../../docs/quality-gate.md) — deliberately in one
|
||||
place only. That list drifted apart across three copies while this PR was in
|
||||
review, which is the argument for not making a fourth. `npm run local:gate`
|
||||
prints each stage as it starts, so the running command is the other reliable
|
||||
answer.
|
||||
|
||||
It runs **every check** GitHub CI runs (which additionally runs `npm install`,
|
||||
and runs `coverage` as a parallel job), plus two local-only steps. So the
|
||||
direction that matters holds: **passing `local:gate` locally means every check
|
||||
CI applies has already passed on your machine** — the strongest predictor of a
|
||||
green CI there is here, though not a proof (a different OS, and the bare test
|
||||
pass noted below). The reverse does not hold at all.
|
||||
|
||||
One difference in *invocations*, not checks: CI runs each client's unit suite
|
||||
twice — bare inside `validate`, instrumented inside `coverage` — on two
|
||||
parallel runners, while the gate runs it **once**, instrumented (#2341). The
|
||||
gate's first stage is `local:validate`, which is `validate` minus each client's
|
||||
`test` leg; `npm run validate` itself is unchanged. The reasoning is in
|
||||
[`AGENTS.md`](../../../AGENTS.md#mandatory-pre-push-gate).
|
||||
|
||||
⚠️ **There is no `npm run ci`.** The gate was renamed to `local:gate` (#2146)
|
||||
precisely because `npm ci` is a built-in that clean-installs from the lockfile
|
||||
and does _not_ run this script. `npm run ci` now fails with npm's missing-script
|
||||
error.
|
||||
|
||||
## Verify by exit code, not by grepping output
|
||||
|
||||
Prettier failures are `[warn]` lines that match no obvious failure pattern, so a
|
||||
grep-based check reports success on a red run. Capture the status:
|
||||
|
||||
```sh
|
||||
npm run local:gate; echo "EXIT=$?"
|
||||
```
|
||||
|
||||
⚠️ If you run it as a background task, the harness's "exit code 0" notification
|
||||
describes the _wrapper_, not the gate — read the `EXIT=` line.
|
||||
|
||||
**Background it and then wait for that notification** — do not spend turns
|
||||
watching it. The gate takes several minutes, and re-running `tail` or an
|
||||
`echo ok` once per turn until it lands tells you nothing the completion
|
||||
notification would not have; see [Waiting on long-running
|
||||
work](../../../AGENTS.md#waiting-on-long-running-work). Waiting out one run this
|
||||
way cost ~80 consecutive no-op turns on #2250.
|
||||
|
||||
## Diagnosing a failing stage
|
||||
|
||||
### `verify:format-coverage` / `format:check`
|
||||
|
||||
Something isn't formatted, or a tracked source file is covered by no
|
||||
`format:check` glob. Run the **root** `npm run format` (it covers `core/`,
|
||||
`scripts/`, the shared surface, and every client) — not a single client's.
|
||||
|
||||
### `verify:typecheck-coverage`
|
||||
|
||||
A tracked `.ts`/`.tsx`/`.mts`/`.cts` lands in no tsconfig project. Usually a new
|
||||
top-level file in a client whose build config roots at `./src`; add it to that
|
||||
client's `tsconfig.test.json` (or the src config's `include`).
|
||||
|
||||
### `verify:skills` / `verify:skills:cli`
|
||||
|
||||
A `.claude/skills` manifest does not parse, declares no invocation mode, or a
|
||||
model-invoked skill is missing its eval cases. `verify:skills:cli` is the
|
||||
authoritative validator and fetches a pinned CLI over the network if you have
|
||||
none installed — so it is also the one stage that will fail offline.
|
||||
|
||||
### `verify:dep-lockstep`
|
||||
|
||||
A dependency reaching one `tsc` program from two installs resolves to two
|
||||
versions. **Align the versions** — bump it in every install that declares it.
|
||||
Do not raise the heap with `--max-old-space-size`; that hides the class rather
|
||||
than fixing it.
|
||||
|
||||
### `verify:test-timeouts`
|
||||
|
||||
A Vitest project resolves to a wall-clock budget nobody stated, or stopped
|
||||
loading `vitest.setup.shared.mts`. (A `retry` itself fails at **runtime**, from
|
||||
that setup file, with a message naming the test — not here.) The shared values live in `vitest.shared.mts` (`TIMEOUTS` /
|
||||
`INTEGRATION_TIMEOUTS`) and every project spreads one of them — so **raise a
|
||||
budget there**, not with a per-suite `}, 30_000)` argument, which only moves the
|
||||
one site and leaves every future file on the default. A per-suite raise is
|
||||
right only where the work is genuinely different (real cross-process lock
|
||||
contention, a full OAuth round trip); say so at the site. `retry` stays unset:
|
||||
it turns a load-induced red into a silent green on the only pre-push gate here.
|
||||
|
||||
A **failing test** is a different problem from a budget — read the failure
|
||||
before reaching for a number. An assertion that races is #1596's class and is
|
||||
fixed with fake timers or an awaited condition, not with headroom.
|
||||
|
||||
### `lint`
|
||||
|
||||
**There is no warning tier** — every `lint` script runs `--max-warnings 0`, so a
|
||||
warning fails exactly as an error does. Fix the finding; don't widen a
|
||||
`globalIgnores` or drop the rule. If a rule genuinely must be waived on a line,
|
||||
use its inline disable comment **with a one-line justification**.
|
||||
|
||||
### `typecheck` passes locally but CI's `tsc -b` fails
|
||||
|
||||
⚠️ The **incremental cache** hides type errors. Re-run with `tsc -b --force`, or
|
||||
in a fresh worktree.
|
||||
|
||||
### `coverage`
|
||||
|
||||
The per-file gate is ≥90 on **all four** dimensions (lines, statements,
|
||||
functions, branches). A genuinely unreachable branch is annotated at the source
|
||||
with a justified `/* v8 ignore … -- <reason> */`, never waved through by
|
||||
lowering the gate. See `/testing` for the acceptable reasons.
|
||||
|
||||
Also check the file is inside a gated directory — the web coverage `include` is
|
||||
a whitelist, so a module placed outside it falls out of the gate silently.
|
||||
|
||||
### `smoke:web*`
|
||||
|
||||
⚠️ **An orphaned prod web server from a previous run fakes a rejection.** It
|
||||
answers the readiness probe with _its_ token, and the deep link comes back
|
||||
`data-deeplink="rejected"` with no error. Assert the port is free before
|
||||
blaming the change.
|
||||
|
||||
### `local:storybook`
|
||||
|
||||
⚠️ In a worktree with a **symlinked `node_modules`** every story file fails on
|
||||
Vite's `fs.allow`. Do a real `npm install` in the worktree.
|
||||
|
||||
### Everything times out at once
|
||||
|
||||
⚠️ Two `npm run local:gate` runs on one machine starve each other — under four
|
||||
worktrees ~326 tests timed out at 5s (#2323), and even two collide
|
||||
deterministically on the web smokes' fixed ports. Since #2339 the gate takes a
|
||||
**machine-wide lease**, so a second run queues rather than overlapping; if
|
||||
everything is still timing out at once, look for what is _not_ the gate: a
|
||||
bypassed lease (`INSPECTOR_SKIP_GATE_LEASE` set in that shell), a bare
|
||||
`npm run coverage` or `test:storybook` in another session, or Spotlight
|
||||
indexing a fresh `node_modules` (a `mdworker` storm after `npm install` in a
|
||||
new worktree pushed the load average to 20 for ten minutes). Do not write a
|
||||
"wait until the machine is clear" loop — two of them deadlock on each other,
|
||||
and a `pgrep -f "npm run local:gate"` loop matches _itself_ and never exits.
|
||||
|
||||
### Waiting on the lease
|
||||
|
||||
A gate that starts with
|
||||
|
||||
```
|
||||
gate-lease: pid 12345 in /Users/you/Projects/mcp-inspector-wt-1, running for 2m10s holds the gate lease; waiting …
|
||||
```
|
||||
|
||||
is queued behind another worktree's gate, and will start the moment it
|
||||
releases (it re-checks every 2s and prints `still waiting` once a minute). The
|
||||
holder's pid and worktree are in the line, so you can decide whether to wait
|
||||
or to stop that gate. A holder that was **killed** — a closed terminal, an
|
||||
OOM'd session — stops refreshing its lock and is taken over after 30s; nothing
|
||||
needs cleaning up by hand. The one exception is a dead holder's lock directory
|
||||
that cannot be removed (a stray file inside it, or permissions): the takeover
|
||||
fails, the waiter keeps waiting, and the wait runs to its 45-minute cap naming
|
||||
the path — remove that directory by hand. The cap is a total wait budget, counted
|
||||
from the waiter's first attempt and not reset as the queue ahead of it drains,
|
||||
so a queue of healthy gates deeper than it covers — about ten, at ~4.5 minutes
|
||||
each — reaches it too. So the give-up happens against a live gate that has
|
||||
hung, a stale lock that would not go away, or a queue that deep; never on its
|
||||
own.
|
||||
|
||||
`INSPECTOR_SKIP_GATE_LEASE=1 npm run local:gate` runs without the lease. It is
|
||||
for a measurement that needs contention; it does not get a result sooner,
|
||||
because the queued run finishes before an overlapped one would.
|
||||
|
||||
## Local-only steps
|
||||
|
||||
Two stages have no GitHub CI counterpart, each deliberately:
|
||||
|
||||
- **`smoke:web:firefox`** — the three browser-driven web smokes again under
|
||||
Firefox. Trialled as a CI job and removed (#2086): across a dozen runs it never
|
||||
disagreed with Chromium, and `playwright install --with-deps` carries a real
|
||||
flake surface. Kept in front of a human about to push instead.
|
||||
- **`smoke:tui`** — needs a real TTY. It _is_ invoked in CI via `npm run smoke`
|
||||
and self-skips there on `process.env.CI`, so it needs no guarding.
|
||||
|
||||
A guard (`scripts/lib/workflow-gate.mjs`, run by `npm run test:scripts`) fails
|
||||
the suite if a workflow invokes a `local:*` script, a non-Chromium engine pass,
|
||||
or sets `SMOKE_BROWSER` to anything but a literal `chromium`. Don't work around
|
||||
it — the split is the design.
|
||||
|
||||
## Publish-only check
|
||||
|
||||
`npm run pack:verify` builds, packs the real tarball, installs it into a clean
|
||||
throwaway consumer, and drives the installed `mcp-inspector` bin end to end. It
|
||||
needs **network access**, so it is a local/release check: it is **not** a
|
||||
`local:gate` stage, and CI runs it only in the `publish` job on a published
|
||||
GitHub Release — never on a push or PR. Run it when touching packaging (the
|
||||
`files` allowlist, a bundler `external` list, anything read from disk at
|
||||
runtime), and as its own step when cutting a release (the `release` skill).
|
||||
|
|
@ -1,30 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "npm run local:gate just failed at the coverage stage. What do I do?",
|
||||
"expect": "pre-push-gate"
|
||||
},
|
||||
{
|
||||
"prompt": "The gate's Storybook stage is failing for me. How do I diagnose it?",
|
||||
"expect": "pre-push-gate"
|
||||
},
|
||||
{
|
||||
"prompt": "Everything in the gate started timing out at once. What causes that?",
|
||||
"expect": "pre-push-gate"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I check whether a stage of the gate really passed?",
|
||||
"expect": "pre-push-gate"
|
||||
},
|
||||
{
|
||||
"prompt": "The gate failed at the lint stage. How do I work out what is wrong?",
|
||||
"expect": "pre-push-gate"
|
||||
},
|
||||
{
|
||||
"prompt": "What is 17 multiplied by 23?",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "Write a limerick about a cat.",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
|
|
@ -1,106 +0,0 @@
|
|||
---
|
||||
name: project-structure
|
||||
description: Where things live in the Inspector repo — which client owns which surface, what is in core/ versus clients/web, and what each top-level directory is for. Use when locating code, deciding where a new module belongs, or answering which package owns or contains a given area of the codebase.
|
||||
user-invocable: false
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# Project structure
|
||||
|
||||
The Inspector is one npm package, `@modelcontextprotocol/inspector`, with three
|
||||
clients (Web, TUI, CLI) over a shared `core/`. It is **not** an npm workspace —
|
||||
each client under `clients/*` keeps its own `package.json` and `node_modules`
|
||||
(see `/local-dev`).
|
||||
|
||||
```
|
||||
inspector/
|
||||
├── clients/
|
||||
│ ├── web/ Vite + React + Mantine SPA with a Node backend
|
||||
│ │ ├── src/ Browser app: components, hooks, lib/, utils/, theme/
|
||||
│ │ ├── server/ Node-only dev/prod backend wiring (see below)
|
||||
│ │ └── static/ sandbox_proxy.html — served for the MCP Apps tab
|
||||
│ ├── cli/ Scriptable CLI (tsup bundle, @inspector/core alias)
|
||||
│ ├── tui/ Ink + React terminal UI (tsup bundle)
|
||||
│ └── launcher/ The `mcp-inspector` bin; dispatches to web/cli/tui in-process
|
||||
├── core/ Shared code, consumed via the `@inspector/core` alias (no package.json)
|
||||
├── test-servers/ Composable MCP test servers + JSON configs used by tests and by hand
|
||||
├── scripts/ Root build/verify tooling: install cascade, smokes, the verify:* guards
|
||||
├── docs/ Task-oriented guides (see docs/README-style index in the root README)
|
||||
├── specification/ Design/build specifications
|
||||
└── AGENTS.md The rules contract — read this before changing anything
|
||||
```
|
||||
|
||||
## `core/` — the shared runtime
|
||||
|
||||
Its entry point is the **`InspectorClient`** class, which owns the connection to
|
||||
an MCP server, the request/response lifecycle, and a set of state stores.
|
||||
|
||||
| Directory | Owns |
|
||||
| --- | --- |
|
||||
| `core/mcp/` | `InspectorClient`, transports, state stores, config import, URI templates, task/subscription/App-elicitation protocol helpers |
|
||||
| `core/mcp/node/` | Node stdio transport factory; `proxyFetch.ts` (the shared HTTPS_PROXY/NO_PROXY fetch) |
|
||||
| `core/mcp/remote/` | Browser HTTP/SSE transport + remote logger/fetch, and (under `node/`) the Hono backend it talks to |
|
||||
| `core/mcp/state/` | The stores `core/react/` hooks read |
|
||||
| `core/auth/` | OAuth end to end — providers, discovery, storage, endpoint overrides, scopes, revocation, mid-session recovery — split into isomorphic logic plus `browser/`, `node/` and `remote/` backends |
|
||||
| `core/auth/node/` | Node OAuth storage + loopback callback server, **and** the `SecretStore` backends (keychain / file / memory) and their selection policy |
|
||||
| `core/client/` | Install-level client config (`client.json`): browser-safe parse plus Node load/save, remote backend, secrets, runner |
|
||||
| `core/json/` | JSON + parameter/argument conversion; the schema normalizations all three form builders share (nullable unions, root composition) and the tool-schema portability lint |
|
||||
| `core/react/` | React hooks over the state stores — consumed by both the web and TUI React trees. Every subscription reads its snapshot **during render** via `useSyncExternalStore` (#1955); `useStoreSnapshot.ts` caches the fresh-value-per-read getters |
|
||||
| `core/node/` | Node-only helpers: version reader, host normalization/detection |
|
||||
| `core/storage/` | File I/O helpers used by the OAuth persist backends |
|
||||
| `core/logging/` | Silent pino logger singleton |
|
||||
|
||||
`core/` is isomorphic (browser + Node) and has **no `package.json`** — it is not
|
||||
published on its own. Its tests live in `clients/web/src/test/core/`, and its
|
||||
browser-consumed runtime is inside the web coverage gate.
|
||||
|
||||
## `clients/web/server/` — the Node backend
|
||||
|
||||
| File | Role |
|
||||
| --- | --- |
|
||||
| `vite-hono-plugin.ts` | Hono middleware on the Vite dev server |
|
||||
| `server.ts` | Standalone Hono prod server |
|
||||
| `start-vite-dev-server.ts` | In-process Vite starter for the launcher |
|
||||
| `web-server-config.ts` | Env parsing, initial-config payload, startup banner |
|
||||
| `sandbox-controller.ts` | The MCP Apps sandbox HTTP server |
|
||||
| `app-origin-controller.ts` | The dedicated app origin for `_meta.ui.domain` |
|
||||
| `inject-auth-token.ts` | Embeds the API token into served `index.html` |
|
||||
| `resolve-bind-host.ts` | Bind-host policy (defaults to `127.0.0.1`; refuses a wildcard bind without the opt-in) |
|
||||
| `browser-externalized-builtin-gate.ts` | Fails `vite build` on a browser-externalized Node built-in |
|
||||
| `ensure-web-build.ts` | Builds `clients/web/dist` on demand for prod `--web` |
|
||||
|
||||
Each of these files carries a header comment explaining the *why*; read the
|
||||
source rather than looking for a second copy of it here.
|
||||
|
||||
## Web source layout: `src/lib` vs `src/utils`
|
||||
|
||||
**`utils` = functions that compute; `lib` = things that instantiate, adapt, or
|
||||
touch the environment.** If it does I/O or wraps a subsystem it's `lib`; if it's
|
||||
a pure transform it's `utils`. Cross-directory imports point **one way,
|
||||
`lib → utils`**.
|
||||
|
||||
The full rule, its carve-outs (domain types, diagnostic logging, type-only core
|
||||
imports), and the coverage-whitelist hazard are in
|
||||
[`AGENTS.md`](../../../AGENTS.md).
|
||||
|
||||
## Web components
|
||||
|
||||
Presentational ("dumb") components: they take data and callbacks as props and
|
||||
contain only display logic. State comes from the `@inspector/core` hooks, wired
|
||||
in near the top of the tree. Element components live in
|
||||
`clients/web/src/components/elements/`, theme variants in
|
||||
`clients/web/src/theme/`, each Mantine component's variants in its own file.
|
||||
|
||||
## Where to put a new file
|
||||
|
||||
| It is… | It goes in |
|
||||
| --- | --- |
|
||||
| Logic two or more clients need | `core/<area>/` |
|
||||
| Browser-only React or DOM code | `clients/web/src/` |
|
||||
| A pure transform used by web | `clients/web/src/utils/` |
|
||||
| A stateful adapter / subsystem wrapper used by web | `clients/web/src/lib/` |
|
||||
| Node-only web backend wiring | `clients/web/server/` |
|
||||
| A build/verify script | `scripts/` (with a sibling `*.test.mjs` if it has pure logic) |
|
||||
| A test fixture MCP server | `test-servers/src/` + a config in `test-servers/configs/` |
|
||||
|
||||
Test placement is a separate question with its own rules — see `/testing`.
|
||||
|
|
@ -1,30 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "Something in the web client's Node backend needs changing but I do not know which piece handles what. How do I work that out?",
|
||||
"expect": "project-structure"
|
||||
},
|
||||
{
|
||||
"prompt": "I am adding a module that both the browser side and the Node side need to run. How do I decide where it goes in this repo?",
|
||||
"expect": "project-structure"
|
||||
},
|
||||
{
|
||||
"prompt": "Does browser-only transport code belong in core or in the web client?",
|
||||
"expect": "project-structure"
|
||||
},
|
||||
{
|
||||
"prompt": "I have a new file and I'm not sure where it goes. How do I decide in this repo?",
|
||||
"expect": "project-structure"
|
||||
},
|
||||
{
|
||||
"prompt": "I need to find where a feature is implemented but I do not know which client owns it. How do I work that out in this repo?",
|
||||
"expect": "project-structure"
|
||||
},
|
||||
{
|
||||
"prompt": "Convert this array of strings to uppercase.",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "What is the capital of France?",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
|
|
@ -1,260 +0,0 @@
|
|||
---
|
||||
name: release
|
||||
description: "Cut an Inspector v2 release — two PRs and then a GitHub Release. PR 1 puts the npm audit, any fixes it forces, and the version bump on v2/main; PR 2 merges v2/main into main and is smoke-tested from the production build with a ledger artifact for the maintainers; the maintainer then tags and publishes through the GitHub UI. Also covers the v1 line and what the publish jobs gate on."
|
||||
disable-model-invocation: true
|
||||
---
|
||||
|
||||
# Cutting a release
|
||||
|
||||
Background on what ships and why (the `files` allowlist, the bundling rules, the
|
||||
container image) is in [`docs/publishing.md`](../../../docs/publishing.md). This
|
||||
skill is the procedure.
|
||||
|
||||
A v2 release is cut from **`main`**, after the milestone's work has been merged
|
||||
there from `v2/main` — not from `v2/main` itself. The v1 line releases
|
||||
independently from `v1/main` to the `v1-latest` tag and never touches `main`.
|
||||
|
||||
Publishing is automated by two release-gated jobs in
|
||||
`.github/workflows/main.yml` (`github.event_name == 'release'`), both
|
||||
`needs: [build, coverage]` — so a release cannot publish with either the build
|
||||
job or the coverage gate red:
|
||||
|
||||
- **`publish`** — runs `npm run pack:verify` as the pre-publish gate, asserts the
|
||||
release tag matches the root `package.json` version, then `npm publish
|
||||
--access public --provenance`.
|
||||
- **`publish-github-container-registry`** — the GHCR image.
|
||||
|
||||
## The shape: two PRs, then the Release
|
||||
|
||||
There is **one version number** (only the root `package.json` has one — the
|
||||
clients carry none), and the release moves through **two pull requests** in
|
||||
order. They are not interchangeable and neither one's content belongs on the
|
||||
other.
|
||||
|
||||
| | PR 1 — prep | PR 2 — the milestone merge |
|
||||
| --- | --- | --- |
|
||||
| Branch | `v2/chore/<ISSUE>-bump-<X-Y-Z>`, cut from `origin/v2/main` | the milestone-merge branch, cut from `origin/main` |
|
||||
| Base | **`v2/main`** | **`main`** |
|
||||
| Carries | the `npm audit` report, **any fixes the audit forces**, and the **version bump** — all three, one PR | the milestone's work, arriving whole from `v2/main`. **No commits of its own.** |
|
||||
| Verified by | `npm run local:gate` | `npm run local:gate`, **plus** `npm run pack:verify` (not a gate stage), **plus** a hand-driven smoke of every contribution in the milestone, from the **production build**, written up as a **ledger artifact** |
|
||||
| Merged when | reviewed and green | the ledger is reviewed by the maintainers and clean |
|
||||
|
||||
Then, and only then, a maintainer tags and publishes the **GitHub Release**
|
||||
(step 3), which is what triggers the publish jobs.
|
||||
|
||||
⚠️ **Do not fold the two together.** The bump must exist on `v2/main` before the
|
||||
merge (see [Why the bump goes on `v2/main` first](#why-the-bump-goes-on-v2main-first-2010)),
|
||||
and PR 2 must stay a pure merge — a commit authored on the merge branch is a
|
||||
change that exists downstream of `v2/main` and nothing carries it back.
|
||||
|
||||
## 1. PR 1 — audit, audit fixes and the bump, on `v2/main`
|
||||
|
||||
All three are part of the milestone's work, so all three belong on the develop
|
||||
branch and flow into `main` together, **in the same PR** — audit first, so the
|
||||
bump sits on top of a tree you have just checked, and so a reviewer sees the
|
||||
report and the fixes it forced as one change.
|
||||
|
||||
```sh
|
||||
# Branch from the REMOTE ref, and read the version only once you are on it.
|
||||
# A default clone has just `main` checked out, so a local `v2/main` may not
|
||||
# exist and `package.json` here is `main`'s — the released version, not the one
|
||||
# you are bumping from (Copilot).
|
||||
git fetch origin v2/main
|
||||
git checkout -b v2/chore/<ISSUE>-bump-<X-Y-Z> origin/v2/main
|
||||
|
||||
# Audit every install that has its own lockfile — root and each client.
|
||||
# REPORT ONLY. Read the output; do not let npm mutate the tree (see below).
|
||||
npm audit --audit-level=high
|
||||
for c in web cli tui launcher; do (cd "clients/$c" && npm audit --audit-level=high); done
|
||||
|
||||
node -p "require('./package.json').version" # what is on v2/main now
|
||||
npm version minor --no-git-tag-version # or major / patch; bump only, no tag
|
||||
node -p "require('./package.json').version" # confirm, then PR → v2/main
|
||||
```
|
||||
|
||||
Anything it reports is fixed **deliberately** — a direct bump, or an
|
||||
`overrides` entry — and each fix is its own commit, gated by
|
||||
`npm run local:gate` before the version bump goes on top.
|
||||
|
||||
⚠️ **Do not run `npm audit fix`, with or without `--force`.**
|
||||
[Dependency placement](../../../AGENTS.md#dependency-placement) rules it out,
|
||||
and the reason is not `--force`: plain `audit fix` resolves an advisory that has
|
||||
no *upward* escape inside a declared range by silently **downgrading**. That is
|
||||
not hypothetical here — `tsup@8.5.1` declares `esbuild: ^0.27.0` against an
|
||||
advisory covering `0.27.3 - 0.28.0`, and `audit fix` walked three installs back
|
||||
to `0.27.2` (~700 lines of lockfile churn for a low-severity dev-only advisory;
|
||||
tried and reverted in #2058, written up in the `local-dev` skill). `local:gate`
|
||||
does not detect a version regression, so nothing downstream would have caught
|
||||
it. `--force` is worse again — it applies fixes *outside* the declared range,
|
||||
trading a known vulnerability for an unvetted major.
|
||||
|
||||
So the release step is the **report**, and the judgment stays with a person.
|
||||
Where `audit` names something with no in-range fix, pin it with `overrides`;
|
||||
where it needs a major, that is its own issue and its own PR, not a release-day
|
||||
edit. If something can't be resolved before the release ships, say so in the
|
||||
release notes and leave it to the alert-driven pipeline (#2229) rather than
|
||||
forcing it here.
|
||||
|
||||
This step is a **backstop, not a substitute** for #2229's alert-driven issues —
|
||||
those are what surface a transitive vulnerability well before a release is cut,
|
||||
tracked and fixed as their own PRs. This exists so a release is never gated on
|
||||
remembering to check `npm audit` separately.
|
||||
|
||||
The branch name carries the version you are bumping **to**, so it is named after
|
||||
that second reading. If you want it before branching:
|
||||
`git show origin/v2/main:package.json | node -p "JSON.parse(require('fs').readFileSync(0)).version"`.
|
||||
|
||||
⚠️ **Never copy a version out of this file.** It would be a version that has
|
||||
already shipped by the time you read it, and following it would cut a release
|
||||
branch named for the wrong release (Copilot).
|
||||
|
||||
⚠️ **`--no-git-tag-version` is load-bearing.** A bare `npm version` also tags,
|
||||
and the tag would land on a `v2/main` commit — but the release must be cut from
|
||||
`main`, so the tag has to point at the merge commit there (step 3). Tagging here
|
||||
creates a tag on a commit that is never released.
|
||||
|
||||
**PR 1 merges before PR 2 is opened.** The merge branch is cut from `main` and
|
||||
takes `v2/main` whole, so opening it early means merging a `v2/main` that does
|
||||
not yet carry the bump.
|
||||
|
||||
## 2. PR 2 — merge `v2/main` → `main`, smoke-test it, and write the ledger
|
||||
|
||||
Through the usual milestone-merge branch. It now carries the bump, so the
|
||||
release lands on `main` with the version already correct.
|
||||
|
||||
Between steps 1 and 2 the two branches **do** differ, and that is expected, not
|
||||
drift: `v2/main` reads the version being built while `main` still reads the one
|
||||
currently released. What this ordering removes is *post-release* drift — once the
|
||||
milestone merge lands they agree again, and `v2/main` is never left **behind**
|
||||
`main`. If you see `v2/main` ahead of `main`, a release is in flight; if you see
|
||||
it behind, something went wrong.
|
||||
|
||||
### 2a. Smoke-test the release candidate from the production build
|
||||
|
||||
The merge branch's tree **is** the release candidate. Check that rather than
|
||||
assume it — the merge commit's tree and `origin/v2/main`'s must be identical:
|
||||
|
||||
```sh
|
||||
git rev-parse origin/v2/main^{tree}
|
||||
git rev-parse <merge-commit>^{tree} # must print the same hash
|
||||
```
|
||||
|
||||
Then drive it. Work from a **dedicated worktree** with its own full
|
||||
`npm install` (a symlinked `node_modules` passes lint and tests and then fails
|
||||
every story file), run `npm run local:gate` there, and exercise the app from the
|
||||
**production build** — the packaged bin and the built bundles, not `vite dev`.
|
||||
The `local-dev`, `test-servers` and `pre-push-gate` skills cover the mechanics.
|
||||
|
||||
**Then run `npm run pack:verify` there as its own step.** It is what proves the
|
||||
tarball a consumer installs actually resolves, and ⚠️ **`local:gate` does not
|
||||
run it** — `local:gate:stages` has no packaging stage, and a green gate says
|
||||
nothing about the published tarball (#2380). CI runs it only in the `publish`
|
||||
job, which fires on the published GitHub Release — after the tag exists — so
|
||||
skipping it here means the first signal of a broken package arrives too late to
|
||||
stop the release. It needs network access; record its result (tarball size and
|
||||
the `pack:verify OK` line) for the ledger below.
|
||||
|
||||
**Every contribution closed in the milestone gets driven, not read.** The bar is
|
||||
observed behavior from the running app — a rendered panel, a status attribute, a
|
||||
server's own stderr — against a real test server, through whichever clients the
|
||||
change touches (web, CLI, TUI). "Its tests pass" is not evidence for this step;
|
||||
the gate already said that. For a change with no observable surface, the
|
||||
evidence is the thing that holds it — a probe that makes the guard fire, a
|
||||
counted before/after, a resolved binary path.
|
||||
|
||||
### 2b. The ledger artifact
|
||||
|
||||
Write the results up as a **published artifact** for the maintainers to review,
|
||||
and link it from PR 2. Shape it like the
|
||||
[v2.5.0 ledger](https://claude.ai/code/artifact/6f25d292-3623-419f-af7f-26aba57247ef):
|
||||
|
||||
- **Masthead** — repo, PR number and merge commit, version, date; and a
|
||||
standfirst saying what tree was tested and that its hash matches
|
||||
`origin/v2/main`, plus whether the milestone payload is complete (the only
|
||||
issue left open should be the merge itself).
|
||||
- **Verdict band** — `local:gate` and `pack:verify` results, milestone issues
|
||||
verified as `N / N`, distinct test count, regressions found.
|
||||
- **The automated gate** — one cell per `local:gate` stage with its number
|
||||
(file counts, test counts, smoke count), and a note on what is new this
|
||||
milestone.
|
||||
- **The packaging check** — `pack:verify` in its own cell, apart from the gate
|
||||
stages because it is not one of them: its result and the tarball size.
|
||||
- **One section per theme**, each a table of *Issue · What was driven ·
|
||||
Observed · Status*. One row per closed issue, issue-linked, with the actual
|
||||
output in the Observed cell.
|
||||
- **Notes / findings** — anything that is a caveat rather than a pass, called
|
||||
out rather than folded into a row.
|
||||
|
||||
A row that says "verified" without saying what was run is not a ledger entry.
|
||||
|
||||
### 2c. When the smoke finds something
|
||||
|
||||
**The fix goes on `v2/main`, never on the merge branch.** File the issue, fix it
|
||||
through an ordinary PR against `v2/main`, then merge `v2/main` into the merge
|
||||
branch again so the fix arrives the same way everything else did. That keeps the
|
||||
merge tree byte-identical to `origin/v2/main` — which is both the invariant
|
||||
checked in 2a and the reason a finding here does not create a commit that only
|
||||
exists downstream (#2000 → #2092; #2215 → #2216–2224).
|
||||
|
||||
Re-run the affected part of the smoke afterwards and update the ledger; it is
|
||||
the artifact the maintainers approve the merge on.
|
||||
|
||||
## 3. Tag and publish the Release
|
||||
|
||||
**Normally this is done by a maintainer through the GitHub UI**, after PR 2 has
|
||||
merged: *Releases → Draft a new release → Choose a tag → type the bare `x.y.z`
|
||||
→ Create new tag on publish*, with **Target: `main`**, then generate the notes
|
||||
and publish. Publishing the Release is what fires the `publish` and
|
||||
`publish-github-container-registry` jobs.
|
||||
|
||||
The equivalent by hand, for when the UI is not an option — derive the tag from
|
||||
the version that just landed rather than typing one, since a hard-coded tag is
|
||||
either already taken (so `git tag` aborts) or, worse, wrong:
|
||||
|
||||
```sh
|
||||
git fetch origin main
|
||||
VERSION=$(git show origin/main:package.json | node -p "JSON.parse(require('fs').readFileSync(0)).version")
|
||||
echo "$VERSION" # sanity-check before tagging
|
||||
git tag "$VERSION" origin/main && git push origin "$VERSION"
|
||||
# then draft & publish a GitHub Release for that tag → triggers `publish`
|
||||
```
|
||||
|
||||
⚠️ **Tag `origin/main`, not your local `HEAD`.** `git checkout main && git pull`
|
||||
resolves through whatever merge-or-rebase strategy you have configured, so a
|
||||
divergent local `main` can quietly produce or replay local commits. Tagging
|
||||
`HEAD` there tags a commit that is not on `origin/main`, and `git push origin
|
||||
<tag>` pushes only the tag — leaving a release whose commit was never published.
|
||||
The UI path avoids this by construction: the target is `main` itself.
|
||||
|
||||
⚠️ **No `v` prefix.** This repo's release tags are bare `x.y.z` — which is why
|
||||
the command above tags `$VERSION` and not `v$VERSION`, and why the tag typed
|
||||
into the UI carries no prefix either. npm's own `tag-version-prefix` defaults to
|
||||
`v` and the repo sets no `.npmrc`, so a bare `npm version` would have produced a
|
||||
mismatched tag; tagging by hand is what keeps it right. (The workflow's assert
|
||||
step strips a leading `v` before comparing, so a `v`-prefixed tag would still
|
||||
publish — it would just be inconsistent with every previous release.)
|
||||
|
||||
The release's target commit selects which workflow runs, so this only publishes
|
||||
when a release is cut from a commit carrying the v2 workflow.
|
||||
|
||||
## Why the bump goes on `v2/main` first (#2010)
|
||||
|
||||
It used to happen on the milestone-merge branch, which is cut from `main` — so
|
||||
the bump existed only *downstream* of `v2/main` and nothing carried it back.
|
||||
`v2/main` sat at `2.0.0` through both the 2.1.0 and 2.2.0 releases. That is not
|
||||
cosmetic: a branch cut from a milestone-merge branch silently carries the bump
|
||||
into an unrelated PR (this happened on #2009, where a container bugfix arrived
|
||||
with a `2.0.0 → 2.2.0` diff), and anything reading the version in development
|
||||
reported a version two releases old.
|
||||
|
||||
⚠️ **Never close a drift by merging `main` into `v2/main`.** `main` carries the
|
||||
entire pre-v2 v1 history (~230 commits `v2/main` does not have), so a back-merge
|
||||
grafts all of it into the develop branch's log permanently in order to deliver a
|
||||
two-file change. Bumping first means there is nothing to back-merge.
|
||||
|
||||
## The v1 line
|
||||
|
||||
Flat: `feature branch → v1/main → npm v1-latest`, with no merge into `main` at
|
||||
any point. The two lines publish independently under separate dist-tags, so a v1
|
||||
fix does **not** need forward-porting to reach users on
|
||||
`npx @modelcontextprotocol/inspector@v1-latest`.
|
||||
|
|
@ -1,263 +0,0 @@
|
|||
---
|
||||
name: test-servers
|
||||
description: Run a composable MCP test server by hand — pick the showcase config for a feature or bug, build it, and connect with the right protocol era. Use when a change, a PR or a smoke test needs a real server to exercise it; when reproducing a reported bug by hand; when choosing which fixture or protocol era to run; when a fixture keeps serving stale code after an edit; or when the config or preset you need does not exist yet.
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# Running a test server
|
||||
|
||||
`test-servers/` provides **composable MCP servers** so tests and manual checks
|
||||
exercise a real server over a real transport instead of mocks. A server is
|
||||
assembled from **presets** (fixture factories in
|
||||
`test-servers/src/preset-registry.ts`) and configured declaratively with a JSON
|
||||
file under `test-servers/configs/`.
|
||||
|
||||
The full catalogue of showcase configs — one per feature, each with what to click
|
||||
and what the broken build did — is
|
||||
[`docs/test-servers.md`](../../../docs/test-servers.md). This skill is how to
|
||||
run one.
|
||||
|
||||
## Three ways to use a fixture — pick the right one first
|
||||
|
||||
A fixture is stood up in **one of three shapes**, and most of what follows is
|
||||
about the third. Establish which one you are in before reading further, because
|
||||
the showcase **config file and the protocol-era table** belong to that one alone.
|
||||
|
||||
⚠️ **The cut is how the server is stood up, not who is driving.** Automated and
|
||||
by-hand is the wrong axis: the composable-config shape has *both* kinds of
|
||||
consumer, and a smoke that spawns it needs every bit of the config and era
|
||||
guidance a person at two terminals does.
|
||||
|
||||
| Shape | Server runs | Config file | Consumers |
|
||||
| --- | --- | --- | --- |
|
||||
| **In-process HTTP** | inside the test process, built from the API | none — options are constructor args | integration tests, CLI tests, **`smoke:cli`** |
|
||||
| **Spawned stdio** | a child process the transport (or the binary under test) starts | none — the stdio fixture runs its default config | integration tests, the CLI suites, **`smoke:cli`**, **`smoke:tui`** |
|
||||
| **Spawned composable HTTP** | a child process started with `--config` | **yes** — a showcase `--config <name>.json` | the **web** smokes, `pack:verify`, **and** a person by hand |
|
||||
|
||||
⚠️ **"A smoke" is not a shape** — `smoke:cli` uses the first two and `smoke:tui`
|
||||
the second, while only the config-driven web smokes use the third. Pick by the
|
||||
row, never by the caller's category.
|
||||
|
||||
- **In-process HTTP — `createTestServerHttp`.** The caller *constructs* the
|
||||
server and owns its lifecycle. No subprocess, no JSON config, no showcase
|
||||
config to pick. This is the shape for anything needing HTTP/SSE, a specific
|
||||
tool set, or the modern handler — and it is not test-only: `scripts/smoke-cli.mjs`
|
||||
starts one in the smoke process so it can read back the headers the CLI sent.
|
||||
- **Spawned stdio — `getTestMcpServerCommand()`.** The test hands the built
|
||||
fixture's `{ command, args }` to a stdio transport (or to the built CLI), and
|
||||
the transport spawns it. A subprocess *is* started, but **still no config
|
||||
file**: that entry point runs the stdio server's default config, so there is
|
||||
nothing to pick. Reach for it when stdio is the point (`InspectorClient` over
|
||||
stdio, the CLI's out-of-process E2E suite) and the default tool set is enough.
|
||||
A caller may also just *name* the built entry rather than connect to it —
|
||||
`smoke:cli` and `smoke:tui` write it into a `--catalog` as
|
||||
`{ type: "stdio", command: node, args: [<built entry>] }` — which is still
|
||||
this shape, and still the build.
|
||||
- **Spawned composable HTTP — `server-composable.js --config <name>.json`.**
|
||||
Picking the showcase config and the protocol era applies **to this shape**,
|
||||
whoever starts it. Two consumers, and they differ only in who runs the second
|
||||
process:
|
||||
- **A script.** `scripts/smoke-web-elicitation.mjs` spawns it directly;
|
||||
`scripts/lib/mcp-app-flow.mjs` (`startMcpAppServer`) does it for
|
||||
`smoke:web:app`, `smoke:web:tabs` and `pack:verify`. These are automated and
|
||||
config-driven, and the whole of this skill applies to them. ⚠️ **Not every
|
||||
smoke is here** — `smoke:cli` and `smoke:tui` use the two shapes above and
|
||||
pick no config at all.
|
||||
- **You, in a terminal**, with the Inspector in another — `Run one by hand`
|
||||
below.
|
||||
|
||||
⚠️ **The build applies to all three.** Every shape resolves
|
||||
`test-servers/build/` — the two API-driven ones through the
|
||||
`@modelcontextprotocol/inspector-test-server` alias, the composable one by
|
||||
running the emitted `.js` directly — so `Build first` and its stale-build hazard
|
||||
are **not** guidance for one path. Read that section whichever shape you are in.
|
||||
|
||||
### Automated, in-process HTTP: build the server from the API
|
||||
|
||||
```ts
|
||||
import {
|
||||
createTestServerHttp,
|
||||
type TestServerHttp,
|
||||
createTestServerInfo,
|
||||
createEchoTool,
|
||||
} from "@modelcontextprotocol/inspector-test-server";
|
||||
|
||||
let server: TestServerHttp | null = null;
|
||||
|
||||
afterEach(async () => {
|
||||
// Stop it even when the assertion threw, or the port leaks into the next test.
|
||||
if (server) {
|
||||
try {
|
||||
await server.stop();
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
server = null;
|
||||
}
|
||||
});
|
||||
|
||||
it("…", async () => {
|
||||
const started = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo("excluded-tools-test", "1.0.0"),
|
||||
tools: [createEchoTool()],
|
||||
// `modern: {}` opts the fixture into the 2026-07-28 handler; omit it for legacy.
|
||||
});
|
||||
await started.start();
|
||||
server = started;
|
||||
|
||||
// `started.url` is the bound URL — read it, never reconstruct it from a port.
|
||||
// …connect an InspectorClient to it and assert.
|
||||
});
|
||||
```
|
||||
|
||||
The reference test is
|
||||
[`clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts`](../../../clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts)
|
||||
— read it before writing a new one; it is the shape fixture-backed integration
|
||||
tests follow when the server is built in-process. (Stdio-backed ones follow the
|
||||
next subsection instead.)
|
||||
|
||||
Four mechanics of this path:
|
||||
|
||||
- **The factories come from one barrel.** `createTestServerHttp` /
|
||||
`createTestServerStdio` build the server; the `create*Tool`,
|
||||
`create*Resource` and `create*Prompt` fixtures in
|
||||
`test-servers/src/test-server-fixtures.ts` populate it;
|
||||
`createTestServerInfo` fills in `serverInfo`. Prefer an existing fixture
|
||||
factory to hand-writing a `ToolDefinition` — that is what makes the fixture a
|
||||
shared one.
|
||||
- **`start()` then `stop()`, and `stop()` in an `afterEach`.** The server binds a
|
||||
real port, so a test that throws before stopping leaks it into the rest of the
|
||||
file.
|
||||
- **Read `started.url`.** `createTestServerHttp` resolves through
|
||||
`findAvailablePort()`, which walks upward when the port is taken, so an
|
||||
assumed port is the same bug the two-process path has.
|
||||
- **Era is a constructor option, not a config file.** `modern: {}` on the config
|
||||
object selects the modern handler; the client side picks its own negotiation
|
||||
(`eraToVersionNegotiation`). The showcase-config era table below does not
|
||||
apply.
|
||||
|
||||
⚠️ **The barrel is an alias to the BUILD, not to the source** —
|
||||
`vitest.shared.mts` maps `@modelcontextprotocol/inspector-test-server` to
|
||||
`test-servers/build/index.js`. So `Build first` applies to this path in full,
|
||||
stale-build hazard included: an edit to `test-servers/src` that is not rebuilt
|
||||
is invisible to an in-process test exactly as it is to a spawned one.
|
||||
|
||||
### Automated, spawned stdio: hand over the command
|
||||
|
||||
```ts
|
||||
import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-server";
|
||||
|
||||
const { command, args } = getTestMcpServerCommand();
|
||||
const client = new InspectorClient(
|
||||
{ type: "stdio", command, args },
|
||||
{ environment: { transport: createTransportNode } },
|
||||
);
|
||||
await client.connect();
|
||||
// … afterEach → client.disconnect(), which is what stops the child.
|
||||
```
|
||||
|
||||
`getTestMcpServerCommand()` returns `node <test-servers/build/test-server-stdio.js>`.
|
||||
Three consequences:
|
||||
|
||||
- **You do not own the process, the transport does.** There is no `start()` /
|
||||
`stop()` pair — disconnecting the client is what reaps the child, so the
|
||||
`afterEach` that matters is `client.disconnect()`.
|
||||
- **No config is selected and none can be.** That entry point starts the stdio
|
||||
server on its **default** config, so the showcase-config table and the
|
||||
protocol-era guidance below do not apply. If the case needs a specific tool
|
||||
set or the modern handler, it is an in-process HTTP test, not this.
|
||||
- **It is still the build.** The path comes from the module's own resolved
|
||||
location under the alias, so it is `test-servers/build/`, with the same
|
||||
staleness hazard.
|
||||
|
||||
The same command feeds the CLI's out-of-process E2E suite
|
||||
(`clients/cli/__tests__/e2e.test.ts`), which spawns the built CLI *and* lets it
|
||||
spawn the fixture. Reference tests for this shape:
|
||||
`clients/web/src/test/integration/mcp/inspectorClient-response-rejected.test.ts`
|
||||
and `clients/cli/__tests__/methods.test.ts`.
|
||||
|
||||
## Build first
|
||||
|
||||
Every shape above resolves generated output — the in-process one imports the
|
||||
barrel, which is **aliased to `test-servers/build/index.js`**, and the stdio and
|
||||
composable-config ones run emitted `.js` as real subprocesses. So the build must
|
||||
exist whichever one you are in:
|
||||
|
||||
```sh
|
||||
cd clients/web && npm run test-servers:build # tsc -p test-servers → test-servers/build/
|
||||
```
|
||||
|
||||
Scripts reach this through `scripts/ensure-test-servers.mjs`, which builds
|
||||
**unconditionally** (once per process per repo root).
|
||||
|
||||
⚠️ **Unconditional emit is not a clean.** A **deleted** source file leaves its
|
||||
stale `.js` behind, existence checks pass against it, and anything still
|
||||
importing that module silently runs the old code — reported not as staleness but
|
||||
as a product failure in whatever was being tested. So after deleting or renaming
|
||||
a source file:
|
||||
|
||||
```sh
|
||||
rm -rf test-servers/build
|
||||
```
|
||||
|
||||
The `.tsbuildinfo` is pinned inside `build/` so that clean actually invalidates
|
||||
the cache.
|
||||
|
||||
## Run one by hand (two processes)
|
||||
|
||||
This is the **spawned composable HTTP** shape from the section above, driven by
|
||||
you rather than by a smoke script — the config and era guidance is the same
|
||||
either way. Two processes: the test server, then the Inspector.
|
||||
|
||||
```sh
|
||||
# 1. The server, from the repo root, with the config you picked:
|
||||
node test-servers/build/server-composable.js --config test-servers/configs/<name>.json
|
||||
```
|
||||
|
||||
```sh
|
||||
# 2. The Inspector, in another terminal (needs a built launcher — `npm run build`):
|
||||
node clients/launcher/build/index.js --web
|
||||
```
|
||||
|
||||
Then add the server in the Inspector using the URL the first process announced.
|
||||
|
||||
Two mechanics that bite:
|
||||
|
||||
- **The server announces its URL on _stderr_**, not stdout (`console.error` in
|
||||
`server-composable.ts`). Watching stdout alone looks like a server that never
|
||||
started.
|
||||
- **The bound port is not necessarily the config's.** `createTestServerHttp`
|
||||
resolves through `findAvailablePort()`, which walks upward when the configured
|
||||
port is taken — so read the announced URL rather than assuming.
|
||||
|
||||
## Pick the right protocol era
|
||||
|
||||
Each config in [`docs/test-servers.md`](../../../docs/test-servers.md) says
|
||||
which era to connect with. The default is **legacy**; configs setting
|
||||
`transport.modern` need **Protocol Era = Modern**. Connecting with the wrong era
|
||||
usually looks like a missing capability rather than an error.
|
||||
|
||||
## Common starting points
|
||||
|
||||
| Want to see | Config |
|
||||
| --- | --- |
|
||||
| An MCP App in the Apps tab | `mcp-app-http.json` (legacy) |
|
||||
| An App-rendered elicitation | `app-elicitation-http.json` (legacy) |
|
||||
| `Mcp-*` headers + the modern error taxonomy | `modern-network-http.json` |
|
||||
| A tool result's `structuredContent` section | `structured-output-http.json` (legacy) |
|
||||
| RFC 6570 resource-template expansion | `rfc6570-templates-http.json` |
|
||||
| OAuth token revocation on clear | `oauth-revocation-http.json` (legacy) |
|
||||
| A token endpoint the SDK refuses | `oauth-insecure-token-endpoint-http.json` (legacy) |
|
||||
| Cancelling a call mid-flight | `cancellation-modern-http.json` (modern) |
|
||||
|
||||
## Adding a config or preset
|
||||
|
||||
- Presets live in `test-servers/src/preset-registry.ts`; configs in
|
||||
`test-servers/configs/*.json`.
|
||||
- A new showcase config gets a row in `docs/test-servers.md` saying what to do
|
||||
and what the broken build did — the "what it looked like broken" half is what
|
||||
makes the fixture reproducible later.
|
||||
- ⚠️ **An `outputSchema` override must ride a tool that returns structured
|
||||
content.** A conforming client validates the result against the advertised
|
||||
schema, so an override on a preset returning none makes every call fail with
|
||||
"declares an output schema but returned no structured content".
|
||||
|
|
@ -1,44 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "I want to reproduce the OAuth token revocation behaviour by hand. Which test server do I run?",
|
||||
"expect": "test-servers"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I start one of the composable MCP test servers with a config file?",
|
||||
"expect": "test-servers"
|
||||
},
|
||||
{
|
||||
"prompt": "I need to run a real MCP server locally to exercise this change by hand. Which one do I run, and how do I start it?",
|
||||
"expect": "test-servers"
|
||||
},
|
||||
{
|
||||
"prompt": "Which protocol era should I connect with when I run one of these fixtures by hand?",
|
||||
"expect": "test-servers"
|
||||
},
|
||||
{
|
||||
"prompt": "I need a fixture combination that doesn't exist yet. How do I add one?",
|
||||
"expect": "test-servers"
|
||||
},
|
||||
{
|
||||
"prompt": "Write an integration test that exercises tool listing end to end.",
|
||||
"chain": [
|
||||
"testing",
|
||||
"test-servers"
|
||||
]
|
||||
},
|
||||
{
|
||||
"prompt": "Add end-to-end coverage for the tool-list pagination path.",
|
||||
"chain": [
|
||||
"testing",
|
||||
"test-servers"
|
||||
]
|
||||
},
|
||||
{
|
||||
"prompt": "Sort this list alphabetically: banana, apple, cherry.",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "What is the boiling point of water in Fahrenheit?",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
|
|
@ -1,297 +0,0 @@
|
|||
---
|
||||
name: testing
|
||||
description: Run, place and fix tests in this repo. Use when choosing which npm command runs a given suite (web unit, web integration, Storybook, cli, tui, launcher, scripts); when deciding where a new test file belongs — beside its source, under src/test/, or in a client's __tests__/; when a per-file coverage check fails or a v8 ignore is in question; when asking which test tier spawns the built binary rather than importing it; or when rendering, mounting or asserting on Mantine components and their transitions in a test.
|
||||
disable-model-invocation: false
|
||||
---
|
||||
|
||||
# Testing
|
||||
|
||||
**Every change needs tests, and every file must clear ≥90% on lines,
|
||||
statements, functions, and branches.** That rule and the React/Mantine
|
||||
conventions live in [`AGENTS.md`](../../../AGENTS.md); this skill is where a
|
||||
test goes, how to run it, and how to clear the gate.
|
||||
|
||||
## Before you write it: does the test use a `test-servers/` fixture?
|
||||
|
||||
**If it does, load the `test-servers` skill now — that is step one, before
|
||||
choosing a location or writing a line.**
|
||||
|
||||
The condition is **"does this test depend on a fixture from `test-servers/`?"**
|
||||
— not which tier it lands in, and not which directory it lands in. There are two
|
||||
ways to depend on one, and they need different halves of that skill:
|
||||
|
||||
- **It connects to a fixture.** An integration test that connects; an
|
||||
end-to-end test that connects; a smoke that drives a connected flow; a
|
||||
coverage gap only reachable over a real connection; reproducing a reported bug
|
||||
against a server. These need the whole procedure — the staleness hazard, and
|
||||
then whichever half matches how the server is stood up.
|
||||
⚠️ **Which shape you need depends on what is driving, and there are three.**
|
||||
All three are the **"Three ways to use a fixture"** section of `test-servers`,
|
||||
which names the entry point and a reference for each — read the right third:
|
||||
- **An integration or CLI test → in-process HTTP.**
|
||||
`createTestServerHttp(...)` / `.start()` / `.stop()`, with the test owning
|
||||
the lifecycle. Use it when the case needs HTTP or SSE, a specific tool set,
|
||||
or the modern handler (`modern: {}` is a constructor option). **No showcase
|
||||
config and no era table apply.**
|
||||
- **An integration or CLI test where stdio is the point → spawned stdio.**
|
||||
`getTestMcpServerCommand()` handed to a stdio transport or to the built CLI,
|
||||
which spawns it. A subprocess *is* started, but it runs the stdio fixture's
|
||||
**default** config, so there is still nothing to pick — and nothing to
|
||||
override, so if the case needs a specific tool set it is an in-process HTTP
|
||||
test instead.
|
||||
- **A config-driven web smoke, or `pack:verify` → spawned composable HTTP.**
|
||||
`smoke:web:elicitation`, `smoke:web:app`, `smoke:web:tabs` and `pack:verify`
|
||||
all spawn `server-composable.js --config <name>.json`. **The showcase-config
|
||||
and protocol-era guidance applies to you in full** — being automated does
|
||||
not exempt a smoke from it.
|
||||
|
||||
⚠️ **"A smoke" is not a shape, so do not route by that word.** `smoke:cli`
|
||||
uses the first two — an in-process `createTestServerHttp` for the header
|
||||
round-trip, and the built stdio entry in a `--catalog` for the connect checks
|
||||
— and `smoke:tui` uses the stdio entry alone. Only the web smokes above are
|
||||
config-driven. Pick by what the caller actually stands up.
|
||||
|
||||
What applies to all three is that section's build warning.
|
||||
⚠️ **Connecting is a strong hint, not the rule.** A few integration tests
|
||||
deliberately hand-roll a JSON-RPC server because the composable fixture
|
||||
*cannot* produce what they assert on — `inspectorClient-malformed-list.test.ts`
|
||||
and `listSalvage-era.test.ts` need wire shapes the SDK's own server refuses to
|
||||
emit. Real transport, real client, no `test-servers/` dependency. Check
|
||||
whether a fixture can express the case before reaching for one.
|
||||
- **It names or runs the built fixture without connecting.** `smoke:tui` boots
|
||||
the TUI against a catalog whose stdio command *is* the built fixture, then
|
||||
asserts it survives. No transport is driven and no protocol era applies, but
|
||||
the **build and staleness** half lands on it in full.
|
||||
|
||||
⚠️ **"A build ran" is not the dependency — using the artefact is.**
|
||||
`clients/web`'s `pretest` runs `test-servers:build` before *every* unit run, so
|
||||
the fixture is on disk for tests that never reference it. What counts is whether
|
||||
the test **starts, spawns, configures, or hands a built entry to the subject
|
||||
under test**. That last clause is what covers `smoke:tui`, which drives no
|
||||
transport at all and still depends on the fixture — see the build-only bullet
|
||||
above.
|
||||
|
||||
⚠️ **And *importing* the package is not the dependency either.** The barrel
|
||||
exports plain functions as well as server factories, so a test can import from
|
||||
it and never stand a server up — `src/test/core/mcp/test-server-scope.test.ts`
|
||||
imports `createScopeCheckMiddleware` and friends to unit-test the scope
|
||||
middleware as a pure function, with no `start()` anywhere in the file. None of
|
||||
the procedure applies to it — no config, no era, no lifecycle — it is an
|
||||
ordinary unit test that happens to import its subject from that package. Ask
|
||||
whether a *server* runs, not whether the import line is present.
|
||||
|
||||
So the condition does **not** hold when the test renders a component from
|
||||
fixture props, exercises a pure function or a parser, or is a smoke that touches
|
||||
no fixture — `smoke:launcher` checks `--help`, and `smoke:web` /
|
||||
`smoke:web:browser` only assert the SPA is served and paints.
|
||||
|
||||
⚠️ **Neither the tier nor the folder decides this.** `src/test/integration/`
|
||||
holds `storage/store-id.test.ts`, which validates a string, and `mcp/import/*`,
|
||||
which parses config files, right beside the tests that drive a live connection.
|
||||
They sit there for the node env and the 30s timeout, not because they connect —
|
||||
placement is the project manifest, so it cannot also be the fixture trigger.
|
||||
Ask what the test *does*, not where it lives.
|
||||
|
||||
**In the connecting case**, the test drives a **real server over a real
|
||||
transport, never a mock**, and picking the fixture, building it, and connecting
|
||||
with the right protocol era is a procedure this skill does not carry. Writing
|
||||
one without `test-servers` means hand-rolling a fixture that already exists, or
|
||||
mocking the thing the tier exists to avoid mocking. **In the build-only case**,
|
||||
none of the transport or protocol-era guidance applies — what you need from
|
||||
`test-servers` is how to build the fixture and why a stale build keeps serving
|
||||
old code.
|
||||
|
||||
## Where the test file goes
|
||||
|
||||
**Side-by-side by default; `src/test/` only for what can't be co-located; and
|
||||
the Node clients are different.**
|
||||
|
||||
### `clients/web` — side-by-side
|
||||
|
||||
`<Name>.test.tsx` (or `.test.ts` for non-React modules) **next to the source**.
|
||||
Components, hooks, `lib/`, `utils/`. This is the overwhelming majority; a
|
||||
web-owned test living under `src/test/` instead is a bug.
|
||||
|
||||
`clients/web/src/test/` is for the three things that *cannot* be co-located:
|
||||
|
||||
1. **Tests of the repo-root `core/` package** → `src/test/core/…`, mirroring the
|
||||
`core/` folder layout. `core/` physically lives outside `clients/web/`, is
|
||||
consumed via the `@inspector/core` alias, and has no test harness of its own.
|
||||
This includes `core/json/*` and `core/client/*`.
|
||||
2. **The `integration` project** → `src/test/integration/…`, mirroring the
|
||||
`core/` source layout (`mcp/`, `mcp/node/`, `mcp/remote/`, `auth/`,
|
||||
`auth/node/`, `storage/`). **Placement is the manifest** — any file under that
|
||||
folder is picked up by the integration project (node env, 30s timeouts) via a
|
||||
folder glob; there is no enumeration to keep in sync. ⚠️ Placement is *not*
|
||||
the fixture trigger, though — this folder holds pure parser and storage tests
|
||||
alongside the connecting ones. If the test you are adding here **needs a
|
||||
fixture from `test-servers/`, load that skill first**; the fixture is half of
|
||||
that test. Connecting is a strong hint but not the rule — see the
|
||||
hand-rolled-server exception above.
|
||||
3. **Shared test infrastructure** — `renderWithMantine.tsx`, `setup.ts`,
|
||||
`fixtures/`, `scrollAreaStoryAssertions.ts`.
|
||||
|
||||
### `clients/cli`, `clients/tui`, `clients/launcher` — a top-level `__tests__/`
|
||||
|
||||
**All** their tests, not beside their source. Their `tsconfig.json` excludes
|
||||
`**/*.test.*` and their `tsconfig.test.json` includes `__tests__/**/*`, so a
|
||||
co-located `src/**/*.test.*` lands in **no** tsconfig project and fails
|
||||
`npm run verify:typecheck-coverage` (#1791).
|
||||
|
||||
### Root tooling — `scripts/*.test.mjs`
|
||||
|
||||
A new `scripts/*.mjs` helper with pure logic gets a sibling `*.test.mjs`, run by
|
||||
`npm run test:scripts` (node's built-in runner; the root has no vitest harness by
|
||||
design). ⚠️ Keep the filename `*.test.mjs` — `node --test` silently **skips** a
|
||||
file its glob misses and still exits 0.
|
||||
|
||||
## Running them
|
||||
|
||||
| Scope | From | Command |
|
||||
| --- | --- | --- |
|
||||
| Web unit | `clients/web` | `npm run test` (`test:watch` while iterating) |
|
||||
| Web integration | `clients/web` | `npm run test:integration` |
|
||||
| Web Storybook play fns | `clients/web` | `npm run test:storybook` |
|
||||
| CLI | `clients/cli` | `npm run test` (`pretest` builds test-servers + the bin) |
|
||||
| TUI | `clients/tui` | `npm run test` |
|
||||
| Launcher | `clients/launcher` | `npm run test` |
|
||||
| Root tooling | repo root | `npm run test:scripts` |
|
||||
| Everything, fast | repo root | `npm run validate` |
|
||||
| The coverage gate | repo root | `npm run coverage` |
|
||||
|
||||
There is **no aggregate root `test` script** — each client self-validates.
|
||||
|
||||
In unit tests that expect error output, **suppress it from the console**.
|
||||
|
||||
## The tiers, shallowest first
|
||||
|
||||
unit (`test`, per client) → web integration (`test:integration`, real
|
||||
transports/servers) → out-of-process (`clients/cli/__tests__/e2e.test.ts`,
|
||||
spawns the built binary) → smokes through the built launcher (`npm run smoke`) →
|
||||
Storybook play functions (`test:storybook`) → the published-tarball check
|
||||
(`npm run pack:verify`, local/release only — needs network).
|
||||
|
||||
⚠️ **Depth in that list is not the fixture boundary, and the boundary cuts
|
||||
across the tiers rather than along them.** Needing `test-servers/`: the web
|
||||
integration tests **that drive one**, the out-of-process CLI tests, the smokes
|
||||
that connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`,
|
||||
`smoke:web:tabs`), `pack:verify`, and **`smoke:tui`** — which never asserts a
|
||||
round trip but calls `ensureTestServers({ requires: ["stdio"] })` and hands the
|
||||
built fixture to the TUI as its catalog's stdio command. Not needing it: the
|
||||
pure tests inside the same integration project, the connecting tests that
|
||||
deliberately hand-roll a server, `smoke:launcher`, `smoke:web` and
|
||||
`smoke:web:browser` (all three stop at boot without a fixture), and every
|
||||
Storybook play function (fixture props). **Load the `test-servers` skill as soon
|
||||
as a task puts you on the fixture side of that line** — whichever tier it sits
|
||||
in.
|
||||
|
||||
`validate` runs the per-client `test` scripts — so web **unit** plus cli's
|
||||
out-of-process `e2e.test.ts`, but **not** web's integration project, which runs
|
||||
inside the `coverage` gate. CI therefore has no separate `test:integration` step.
|
||||
|
||||
## The coverage gate
|
||||
|
||||
**Per-file ≥90 on all four dimensions**, CI-enforced, across web, cli, tui and
|
||||
launcher. New code must clear 90 on every dimension.
|
||||
|
||||
Scope notes:
|
||||
|
||||
- The **web** coverage `include` (in `clients/web/vite.config.ts`) also covers
|
||||
the shared `core/` runtime the browser consumes — `core/mcp`, `core/react`,
|
||||
`core/auth`, `core/storage`, `core/logging`, `core/node`, `core/json`,
|
||||
`core/client`.
|
||||
- ⚠️ That `include` is a **whitelist** naming `components`/`hooks`/`theme`/
|
||||
`lib`/`utils`/`server`. A module placed **outside** those directories falls out
|
||||
of the gate entirely, silently. The documented exceptions are `src/App.tsx`
|
||||
(a composition root at ~42% branch coverage — gating it is a dedicated
|
||||
decomposition effort) and the `src/main.tsx` / `src/index.ts` bootstraps.
|
||||
- **CLI** tests run **in-process** by importing `runCli()`
|
||||
(`__tests__/helpers/cli-runner.ts`) so `src` is measured; `src/index.ts` is the
|
||||
only exclusion. `commander` uses `.exitOverride()` so a parse error throws
|
||||
instead of tearing down the test worker.
|
||||
- **TUI** covers **all of `src/**`, React surface included**. Components mount
|
||||
through `__tests__/helpers/renderTui.tsx` — `ink-testing-library`'s `render`
|
||||
with every frame ANSI-stripped — alongside the passthrough doubles in the same
|
||||
directory; keypresses are driven through stdin. The only exclusion is
|
||||
`src/tui-servers.ts` (a pure re-export, excluded so it doesn't surface as a
|
||||
misleading 0/0 row).
|
||||
⚠️ **Import `render` from that helper, not from `ink-testing-library`.** Ink
|
||||
writes styling *inside* the styled run, so `<Text underline>I</Text>nfo`
|
||||
reaches the frame buffer with escapes between `I` and `nfo` and a plain
|
||||
`toContain("Info")` fails against a component that is rendering correctly. It
|
||||
only shows up where chalk emits color — a developer whose shell exports
|
||||
`FORCE_COLOR` — so CI, which has no TTY, stays green on a suite that is red
|
||||
for them (#2207). If a frame assertion fails on a string you can plainly see
|
||||
in the printed diff, that is the tell. Reach `stdout.lastFrame()` on the
|
||||
returned instance for the raw bytes.
|
||||
|
||||
### When a `v8 ignore` is justified
|
||||
|
||||
A genuinely-unreachable branch is annotated at the source rather than waved
|
||||
through by lowering the gate. The acceptable reasons are enumerated in
|
||||
[`AGENTS.md`](../../../AGENTS.md) — do not reach for one that is not on that
|
||||
list.
|
||||
|
||||
## React tests: `renderWithMantine`
|
||||
|
||||
**Always render through `renderWithMantine`** (`src/test/renderWithMantine.tsx`)
|
||||
— it wraps in `MantineProvider` with the project theme. Do not hand-roll a bare
|
||||
`MantineProvider`; it skips the project theme and the helper's options and drifts
|
||||
from every other test.
|
||||
|
||||
Note the justification has changed and the old one is wrong: it sets
|
||||
`env="test"`, which makes Mantine skip the animated **render** — but it does
|
||||
**not** stop the timers. `env` is read only at `Transition.mjs`'s render branch,
|
||||
while `useTransition()` runs before that check and still schedules real
|
||||
`window.setTimeout`s (opening a `<Modal>` schedules three 200ms timers). What
|
||||
prevents a timer outliving its file — and throwing an uncaught
|
||||
`ReferenceError: window is not defined` that fails the **whole run**, attributed
|
||||
to an innocent file (#1760) — is the **leaked-timer safety net in
|
||||
`src/test/setup.ts`**, which is global and covers every unit test however it
|
||||
renders. The rule stands on consistency, not on timer safety.
|
||||
|
||||
- **Forced color scheme:** pass the option —
|
||||
`renderWithMantine(ui, { colorScheme: "dark" })` — rather than hand-rolling
|
||||
`defaultColorScheme="dark"`.
|
||||
- **Mid-flight transition state** (e.g. asserting a `data-anim="out"` cell during
|
||||
an exit crossfade) is the only reason to use `renderWithMantineTransitions`.
|
||||
Pass `settleMs` derived from the component's real animation duration **plus
|
||||
the helper's shared slack** — `HEADER_ANIM_MS + RAF_SLACK_MS`, both imported,
|
||||
never a literal: the first term tracks the component and the second tracks how
|
||||
busy the machine is, and only the second should move when the machine gets
|
||||
busier (#2323). Do **not** also use `vi.useFakeTimers()` in that test:
|
||||
the auto-settle awaits a real `setTimeout`, so under fake timers it **throws**
|
||||
with a message telling you to call `vi.useRealTimers()` first — it does not
|
||||
silently skip. That is deliberate (a deadlock would otherwise hang until the
|
||||
project's `hookTimeout`), but it means the combination fails the test rather
|
||||
than degrading. If the test unmounts the tree
|
||||
itself use the `unmount()` the helper returns. The mechanism is documented at
|
||||
length on the helper — read there before changing it.
|
||||
|
||||
## Storybook play functions
|
||||
|
||||
Every screen and element component has a `*.stories.tsx`; play functions double
|
||||
as interaction tests, run headless in CI and in the local gate.
|
||||
|
||||
⚠️ **`expect(...)` from `storybook/test` returns a promise.** Storybook
|
||||
instruments it, so every `expect` in a play function is awaited — as is any
|
||||
shared helper that wraps one.
|
||||
|
||||
## Test servers, not mocks
|
||||
|
||||
The tests that drive MCP behaviour over a transport use a real server rather
|
||||
than a mock, and **for the ones that get that server from `test-servers/`, load
|
||||
the skill and use all of it**: which showcase config covers the feature, which
|
||||
protocol era to connect with, how to add a combination that does not exist yet,
|
||||
and why a fixture can keep serving stale code after an edit.
|
||||
|
||||
**A test that only *names* the built fixture needs that skill too, for a
|
||||
narrower reason.** `smoke:tui` boots the TUI against a catalog whose stdio
|
||||
command is the build output and asserts it survives — it opens no transport, so
|
||||
config choice and protocol era do not apply to it, but **building the fixture
|
||||
and the staleness hazard do.** Load the skill and take that half.
|
||||
|
||||
A pure test that happens to live in the integration project, and a smoke that
|
||||
references no fixture, need neither (see the tier list above) — and note that
|
||||
`clients/web`'s `pretest` builds `test-servers/` before every unit run, so its
|
||||
presence on disk says nothing about whether your test depends on it.
|
||||
|
|
@ -1,30 +0,0 @@
|
|||
[
|
||||
{
|
||||
"prompt": "Which test tier in this repo spawns the built binary rather than importing it?",
|
||||
"expect": "testing"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I run just the TUI suite, and is there one command that runs everything?",
|
||||
"expect": "testing"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I write a test for a TUI component in this repo?",
|
||||
"expect": "testing"
|
||||
},
|
||||
{
|
||||
"prompt": "How am I supposed to invoke the CLI from a new test in this repo?",
|
||||
"expect": "testing"
|
||||
},
|
||||
{
|
||||
"prompt": "How do I run the web integration tests, and do they run as part of the normal test command?",
|
||||
"expect": "testing"
|
||||
},
|
||||
{
|
||||
"prompt": "What does this regex do? /^\\d{3}-\\d{4}$/",
|
||||
"expect": null
|
||||
},
|
||||
{
|
||||
"prompt": "Rename the variable `foo` to `bar` in this snippet.",
|
||||
"expect": null
|
||||
}
|
||||
]
|
||||
1
.gitattributes
vendored
1
.gitattributes
vendored
|
|
@ -1,2 +1 @@
|
|||
* text=auto eol=lf
|
||||
package-lock.json linguist-generated=true
|
||||
|
|
|
|||
Some files were not shown because too many files have changed in this diff Show more
Loading…
Reference in a new issue