diff --git a/.claude/skills/add-mcp-server/SKILL.md b/.claude/skills/add-mcp-server/SKILL.md index 78cd7a09e..f9df42d2a 100644 --- a/.claude/skills/add-mcp-server/SKILL.md +++ b/.claude/skills/add-mcp-server/SKILL.md @@ -101,7 +101,7 @@ For complete templates with env vars, permissions, provenance: see [references/c "io.modelcontextprotocol.registry/publisher-provided": { "io.github.stacklok": { "https://api.example.com/mcp": { - "tier": "Official", + "tier": "Community", "status": "Active", "tags": ["remote", "category1"], "tools": ["tool_name"], @@ -124,7 +124,7 @@ For complete templates with OAuth, custom_metadata: see [references/remote-serve | **Overview** | Markdown string starting with `## Title\n\n` followed by 3-5 sentences | | **Name format** | `io.github.stacklok/` | | **Title** | Human-readable display name (e.g., `"Fetch"`, `"GitHub (Remote)"`, `"AWS Knowledge Bases"`) | -| **Tier** | Exactly `"Official"` or `"Community"` | +| **Tier** | Exactly `"Official"` or `"Community"`. Default to `"Community"` — `"Official"` is reserved for servers maintained by the ToolHive team, the MCP spec authors, or the platform owner of the integrated service | | **Status** | Exactly `"Active"` or `"Deprecated"` | | **Remote tags** | Must include `"remote"` in tags | | **Remote transport** | `"streamable-http"` or `"sse"` only (NEVER `"stdio"`) | diff --git a/.claude/skills/mcp-review/SKILL.md b/.claude/skills/mcp-review/SKILL.md index e98c0f88e..e8750cd51 100644 --- a/.claude/skills/mcp-review/SKILL.md +++ b/.claude/skills/mcp-review/SKILL.md @@ -14,6 +14,15 @@ You are an expert reviewer for the ToolHive Registry. Evaluate server.json files For detailed field specs, see [server-json-spec.md](references/server-json-spec.md). For full registry inclusion criteria, see [registry-criteria.md](references/registry-criteria.md) and [server-criteria.md](references/server-criteria.md). +## Registry Layout (read first) + +The repo has two server registries, and submissions land in the wrong one often enough to check first: + +- `registries/toolhive/servers/` — the **contribution path**. All new submissions go here. +- `registries/official/` — a **curated subset** maintained by the ToolHive team. Contributors should not hand-add entries here. + +Both normalize every entry under the **`io.github.stacklok`** publisher namespace (`name: io.github.stacklok/` and the `_meta` publisher key), with a repo-hosted `icon.svg`. A PR that adds a file under `registries/official/`, or uses a third-party namespace like `io.github.`, is a finding — not a style nit. `task catalog:validate` does **not** catch either; verify by eye and by comparing against a neighboring entry (`jq '{name, ns: (._meta[...]|keys)}' /server.json`). + ## Review Workflow ### Step 1: Identify Change Scope @@ -28,18 +37,31 @@ Determine what you're reviewing: Read the server.json and check, in order: -1. **Server type** — `packages` (container) or `remotes` (remote)? -2. **Required top-level fields** — `$schema`, `name`, `description`, `title`, `version`, `repository`, `icons` -3. **Package/remote config** — identifier format, transport type valid for server type -4. **Extension key match** — `_meta` key must exactly equal `packages[0].identifier` or `remotes[0].url` -5. **Extension fields** — `tier`, `status`, `tools`, `overview` all present and valid -6. **Icons** — `icons` array present with correct `icon.svg` URL -7. **Overview format** — starts with `## Title\n\n` followed by 3-5 sentences -8. **No auto-populated fields** — reject if `metadata.*` or `tool_definitions` present in new submissions -9. **Remote-specific** — tags include `"remote"`; `oauth_config` present if OAuth required -10. **Container-specific** — `transport.url` present when type is `streamable-http` +1. **Location & namespace** — file is under `registries/toolhive/servers//`; `name` and the `_meta` publisher key are both `io.github.stacklok` (see [Registry Layout](#registry-layout-read-first)) +2. **Server type** — `packages` (container) or `remotes` (remote)? +3. **Required top-level fields** — `$schema`, `name`, `description`, `title`, `version`, `repository`, `icons` +4. **Package/remote config** — identifier format, transport type valid for server type +5. **Extension key match** — `_meta` key must exactly equal `packages[0].identifier` or `remotes[0].url` +6. **Extension fields** — `tier`, `status`, `tools`, `overview` all present and valid +7. **Tier matches maintainer** — `Official` is reserved for the ToolHive team, the MCP spec authors, or the platform owner of the integrated service. A third-party contributor's server is `Community`, regardless of what the PR claims (don't trust the field — check who maintains the repo) +8. **Icons** — repo-hosted `icon.svg` (`mimeType: image/svg+xml`) AND an `icon.svg` file present in the server dir. Flag external icon URLs (e.g. a vendor PNG) — they're a stability/supply-chain risk and break the convention +9. **Overview format** — starts with `## Title\n\n` followed by 3-5 sentences +10. **No auto-populated fields** — reject if `metadata.*` or `tool_definitions` present in new submissions +11. **Remote-specific** — tags include `"remote"`; `oauth_config` present if OAuth required +12. **Container-specific** — `transport.url` present when type is `streamable-http` + +Run `task catalog:validate` to catch schema-level issues. Note its limits: it validates the JSON schema, **not** the registry-location, namespace, tier-semantics, or icon-host conventions above — a passing validate does not mean the entry is correctly placed or classified. + +**Remote endpoint check (remotes only):** confirm the URL actually serves MCP and the advertised `tools` match. Don't trust the PR description: + +```bash +curl -sS -X POST "" \ + -H "Content-Type: application/json" -H "Accept: application/json, text/event-stream" \ + -d '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18","capabilities":{},"clientInfo":{"name":"review","version":"0"}}}' +# then tools/list (id:2, method:"tools/list") and compare names against the entry's `tools` +``` -Run `task catalog:validate` to catch schema-level issues. +A plain `GET` often 404s on streamable-HTTP endpoints — that's not a failure; use the `initialize` POST. If the endpoint needs auth, note it and confirm `oauth_config`/auth handling is declared. ### Step 3: Security Review @@ -66,6 +88,8 @@ See [server-criteria.md](references/server-criteria.md) for the full checklist a 5. **Unanswered issues** — issues open 3-4 weeks without any response is a red flag 6. **Recent activity** — check last 5 commits for recency 7. **Releases** — list recent releases; confirm semver tags and changelog +8. **Repo & author maturity** — repo age, star count, contributor diversity, and GitHub account age. A days-old repo, zero stars, no releases, and a single brand-new account are a cluster of red flags (e.g. `gh repo view / --json createdAt,stargazerCount,licenseInfo,pushedAt`) +9. **Documentation relevance** — the README must actually describe *this* server and its tools. A generic or boilerplate README (e.g. an unmodified upstream template that documents unrelated demo tools) fails the documentation criterion even when a README file exists **Inclusion criteria summary:** @@ -121,13 +145,16 @@ Focus review on changed aspects, not full re-review. | Check | Status | Notes | |-------|--------|-------| +| Location & namespace (`toolhive/`, `io.github.stacklok`) | Pass/Fail | | | Required top-level fields | Pass/Fail | | | Package/Remote config | Pass/Fail | | | Extension key match | Pass/Fail | | +| Tier matches maintainer | Pass/Fail | | | Transport valid | Pass/Fail | | -| Icons present | Pass/Fail | | +| Remote endpoint serves MCP (remotes) | Pass/Fail/N/A | | +| Icons (repo-hosted SVG + file present) | Pass/Fail | | | Overview format | Pass/Fail | | -| Tools listed | Pass/Fail | | +| Tools listed (match live server) | Pass/Fail | | | No auto-populated fields | Pass/Fail | | | Tags (remote tag if applicable) | Pass/Fail | | @@ -154,6 +181,18 @@ Focus review on changed aspects, not full re-review. Run `task catalog:validate` to verify spec compliance. ``` +## Submitting the Verdict + +When posting to GitHub, the verdict must carry its blocking state — a plain comment does **not** gate the merge. Map the verdict to the right `gh` mechanism: + +| Verdict | Command | Effect | +|---------|---------|--------| +| APPROVE | `gh pr review --approve --body-file ` | Approves; unblocks merge | +| REQUEST_CHANGES / REJECT | `gh pr review --request-changes --body-file ` | **Blocks** merge until resolved | +| Non-binding notes only | `gh pr review --comment --body-file ` | Review comment, no gate | + +`gh pr comment` posts an ordinary comment that does **not** block — only use it for FYI notes, never to record a REJECT/REQUEST_CHANGES decision. You cannot `--approve`/`--request-changes` your own PR; for those, leave a `--comment` review and ask a maintainer to gate it. + ## Error Handling | Situation | Action | @@ -164,9 +203,31 @@ Run `task catalog:validate` to verify spec compliance. | `task catalog:validate` fails | Report the exact error; it must pass before approval | | Unclear whether server needs OAuth | Check the upstream README/docs for auth requirements | | Provenance info unavailable | Flag as missing — expected for all servers per registry criteria | +| Entry added under `registries/official/` or with a non-`io.github.stacklok` namespace | Flag as a structural finding; direct the contributor to `registries/toolhive/servers/` under the stacklok namespace | +| Remote endpoint unreachable or doesn't speak MCP | Re-check with the `initialize` POST (not a bare `GET`); if it still fails, this blocks approval — the URL is the entire contract for a remote | ## Quick Reference +### Severity (what blocks vs. what's a note) + +| Requirement | Severity | +|-------------|----------| +| Open source + permissive license | Required (hard gate — no license = reject) | +| Spec compliance (`task catalog:validate` passes) | Required | +| Correct location & `io.github.stacklok` namespace | Required | +| Tier matches maintainer identity | Required | +| Remote endpoint serves MCP + tools match | Required (remotes) | +| No known critical/high CVEs | Required | +| Secrets `isSecret: true`, no filesystem paths | Required | +| Semver versioning | Required | +| Documentation describes this server | Required | +| Pinned deps / Actions pinned to SHAs | Required | +| Provenance (Sigstore / attestations) | Expected | +| Security scanning in CI, `SECURITY.md` | Expected | +| Repo/author maturity, stars, community traction | Weighed (a cluster of misses can sink an otherwise-valid entry) | + +A `Required` miss is REQUEST_CHANGES or REJECT. `Expected` misses are called out and weigh on the verdict. `Weighed` signals inform borderline calls. + ### Valid Values | Field | Options | diff --git a/.claude/skills/skill-review/SKILL.md b/.claude/skills/skill-review/SKILL.md index 54fc6daa6..acb0b08f0 100644 --- a/.claude/skills/skill-review/SKILL.md +++ b/.claude/skills/skill-review/SKILL.md @@ -196,6 +196,18 @@ Skill shadowing (same name/description, different behavior) is the primary stabi Run `task catalog:validate` and `thv skills validate` to verify compliance. ``` +## Submitting the Verdict + +When posting to GitHub, the verdict must carry its blocking state — a plain comment does **not** gate the merge. Map the verdict to the right `gh` mechanism: + +| Verdict | Command | Effect | +|---------|---------|--------| +| APPROVE | `gh pr review --approve --body-file ` | Approves; unblocks merge | +| REQUEST_CHANGES / REJECT | `gh pr review --request-changes --body-file ` | **Blocks** merge until resolved | +| Non-binding notes only | `gh pr review --comment --body-file ` | Review comment, no gate | + +`gh pr comment` posts an ordinary comment that does **not** block — only use it for FYI notes, never to record a REJECT/REQUEST_CHANGES decision. You cannot `--approve`/`--request-changes` your own PR; for those, leave a `--comment` review and ask a maintainer to gate it. + ## Error Handling | Situation | Action |