From 31b4e046cf6392f5a5495a1c2cd5d066b6da2fa1 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 19 Apr 2026 19:58:35 -0700 Subject: [PATCH 1/4] feat(skills): add Registry for publishing bundled Agent Skills over MCP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a reusable `skills` package that lets an MCP server publish server-bundled Agent Skills (SKILL.md files shipped in the binary) per the skills-over-MCP SEP (SEP-2133): - skills.Bundled describes one skill (name, description, embedded content, optional icons, optional enabled predicate for runtime gating on toolsets/feature-flags/headers) - skills.Registry collects entries, declares the `io.modelcontextprotocol/skills` extension capability on the server, and installs each SKILL.md as an MCP resource plus a skill://index.json discovery document conforming to the agentskills.io/discovery/0.2.0 schema The package has no GitHub-specific state — any MCP server author can drop it in to publish bundled skills with a small amount of wiring. Co-Authored-By: Claude Opus 4.7 (1M context) --- skills/registry.go | 165 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 skills/registry.go diff --git a/skills/registry.go b/skills/registry.go new file mode 100644 index 000000000..0705c63f9 --- /dev/null +++ b/skills/registry.go @@ -0,0 +1,165 @@ +package skills + +import ( + "context" + "encoding/json" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// Well-known identifiers from the skills-over-MCP SEP (SEP-2133) and the +// Agent Skills discovery index schema (agentskills.io). +const ( + // IndexURI is the well-known URI for the per-server discovery index. + IndexURI = "skill://index.json" + // ExtensionKey is the MCP capability extension identifier that a server + // MUST declare when it publishes skill:// resources. + ExtensionKey = "io.modelcontextprotocol/skills" + // IndexSchema is the $schema value servers MUST emit in their index. + IndexSchema = "https://schemas.agentskills.io/discovery/0.2.0/schema.json" +) + +// Bundled describes a single server-bundled Agent Skill — a SKILL.md the +// server ships in its binary and serves at a stable skill:// URI. +type Bundled struct { + // Name is the skill name. Must match the SKILL.md frontmatter `name` + // and the final segment of the skill-path in the URI. + Name string + // Description is the text shown to the agent in the discovery index. + // Should describe both what the skill does and when to use it. + Description string + // Content is the SKILL.md body (typically a //go:embed string). + Content string + // Icons, if non-empty, are attached to the SKILL.md MCP resource so + // hosts that render icons in their resource list can show one. + Icons []mcp.Icon + // Enabled, if set, is called to determine whether this skill should + // be published on the current server instance. Leave nil for "always + // publish". Useful for gating on a toolset, feature flag, or request + // context in per-request server builds. + Enabled func() bool +} + +// URI returns the skill's canonical SKILL.md URI: skill:///SKILL.md. +func (b Bundled) URI() string { return "skill://" + b.Name + "/SKILL.md" } + +func (b Bundled) enabled() bool { return b.Enabled == nil || b.Enabled() } + +// Registry is the set of bundled skills a server publishes. Build one at +// server-construction time with New().Add(...).Add(...); then call +// DeclareCapability before mcp.NewServer and Install after. +type Registry struct { + entries []Bundled +} + +// New returns an empty registry. +func New() *Registry { return &Registry{} } + +// Add appends a bundled skill and returns the registry for chaining. +func (r *Registry) Add(b Bundled) *Registry { + r.entries = append(r.entries, b) + return r +} + +// Enabled returns the subset of entries currently enabled. +func (r *Registry) Enabled() []Bundled { + var out []Bundled + for _, e := range r.entries { + if e.enabled() { + out = append(out, e) + } + } + return out +} + +// DeclareCapability adds the skills-over-MCP extension to the provided +// ServerOptions.Capabilities if any entry is currently enabled. Must be +// called BEFORE mcp.NewServer since capabilities are captured at +// construction. +func (r *Registry) DeclareCapability(opts *mcp.ServerOptions) { + if opts == nil || len(r.Enabled()) == 0 { + return + } + if opts.Capabilities == nil { + opts.Capabilities = &mcp.ServerCapabilities{} + } + opts.Capabilities.AddExtension(ExtensionKey, nil) +} + +// Install registers each enabled skill's SKILL.md as an MCP resource and +// publishes the skill://index.json discovery document. +func (r *Registry) Install(s *mcp.Server) { + enabled := r.Enabled() + if len(enabled) == 0 { + return + } + + for _, e := range enabled { + e := e + s.AddResource( + &mcp.Resource{ + URI: e.URI(), + Name: e.Name + "_skill", + Description: e.Description, + MIMEType: "text/markdown", + Icons: e.Icons, + }, + func(_ context.Context, _ *mcp.ReadResourceRequest) (*mcp.ReadResourceResult, error) { + return &mcp.ReadResourceResult{ + Contents: []*mcp.ResourceContents{{ + URI: e.URI(), + MIMEType: "text/markdown", + Text: e.Content, + }}, + }, nil + }, + ) + } + + indexJSON := buildIndex(enabled) + s.AddResource( + &mcp.Resource{ + URI: IndexURI, + Name: "skills_index", + Description: "Agent Skill discovery index for this server.", + MIMEType: "application/json", + }, + func(_ context.Context, _ *mcp.ReadResourceRequest) (*mcp.ReadResourceResult, error) { + return &mcp.ReadResourceResult{ + Contents: []*mcp.ResourceContents{{ + URI: IndexURI, + MIMEType: "application/json", + Text: indexJSON, + }}, + }, nil + }, + ) +} + +// IndexEntry matches the agentskills.io discovery schema, with MCP-specific +// fields: `url` holds the MCP resource URI; `digest` is omitted because +// integrity is handled by the authenticated MCP connection. +type IndexEntry struct { + Name string `json:"name"` + Type string `json:"type"` + Description string `json:"description"` + URL string `json:"url"` +} + +// IndexDoc is the top-level shape of skill://index.json. +type IndexDoc struct { + Schema string `json:"$schema"` + Skills []IndexEntry `json:"skills"` +} + +func buildIndex(entries []Bundled) string { + doc := IndexDoc{Schema: IndexSchema, Skills: make([]IndexEntry, len(entries))} + for i, e := range entries { + doc.Skills[i] = IndexEntry{Name: e.Name, Type: "skill-md", Description: e.Description, URL: e.URI()} + } + b, err := json.Marshal(doc) + if err != nil { + panic("skills: failed to marshal index: " + err.Error()) + } + return string(b) +} From 890ea55408c9ca7d7a922afc940b0d37e69e2fb2 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 19 Apr 2026 19:58:52 -0700 Subject: [PATCH 2/4] feat(skills): bundle the pull-requests review skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship a SKILL.md at skills/pull-requests/SKILL.md that documents the pending-review workflow for submitting a multi-comment GitHub pull request review (pull_request_review_write → add_comment_to_pending_review → submit_pending). The skill file lives at the repo root so it is also usable as a plain agent skill by any consumer that scans the repo (Claude Code, the agent-skills CLI), independent of this server. Register the skill via skills.Registry in NewMCPServer, gated on the pull_requests toolset being enabled. Both stdio and HTTP transports pick it up since both routes through NewMCPServer. The pull_requests toolset's inline server instructions are unchanged — skill-aware hosts discover the skill through the extension capability, skill://index.json, and resources/list; older hosts continue to receive the same inline instructions they always have. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/github/bundled_skills.go | 37 ++++++ pkg/github/bundled_skills_test.go | 210 ++++++++++++++++++++++++++++++ pkg/github/server.go | 11 ++ skills/bundled.go | 16 +++ skills/pull-requests/SKILL.md | 8 ++ 5 files changed, 282 insertions(+) create mode 100644 pkg/github/bundled_skills.go create mode 100644 pkg/github/bundled_skills_test.go create mode 100644 skills/bundled.go create mode 100644 skills/pull-requests/SKILL.md diff --git a/pkg/github/bundled_skills.go b/pkg/github/bundled_skills.go new file mode 100644 index 000000000..5f295eb0f --- /dev/null +++ b/pkg/github/bundled_skills.go @@ -0,0 +1,37 @@ +package github + +import ( + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/octicons" + "github.com/github/github-mcp-server/skills" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// bundledSkills builds the registry of Agent Skills this server ships. +// Each entry's Enabled closure gates its publication on the relevant +// toolset being enabled under the given inventory. +// +// Adding a new server-bundled skill is one entry here plus a //go:embed +// line in package skills. +func bundledSkills(inv *inventory.Inventory) *skills.Registry { + return skills.New().Add(skills.Bundled{ + Name: "pull-requests", + Description: "Submit a multi-comment GitHub pull request review using the pending-review workflow. Use when leaving line-specific feedback on a pull request, when asked to review a PR, or whenever creating any review with more than one comment.", + Content: skills.PullRequestsSKILL, + Icons: octicons.Icons("light-bulb"), + Enabled: func() bool { return inv.IsToolsetEnabled(ToolsetMetadataPullRequests.ID) }, + }) +} + +// DeclareSkillsExtensionIfEnabled adds the skills-over-MCP extension +// (SEP-2133) to the server's capabilities when any bundled skill is +// currently enabled. Must be called before mcp.NewServer. +func DeclareSkillsExtensionIfEnabled(opts *mcp.ServerOptions, inv *inventory.Inventory) { + bundledSkills(inv).DeclareCapability(opts) +} + +// RegisterBundledSkills registers all enabled server-bundled skills and +// the skill://index.json discovery document on the given server. +func RegisterBundledSkills(s *mcp.Server, inv *inventory.Inventory) { + bundledSkills(inv).Install(s) +} diff --git a/pkg/github/bundled_skills_test.go b/pkg/github/bundled_skills_test.go new file mode 100644 index 000000000..13eda7245 --- /dev/null +++ b/pkg/github/bundled_skills_test.go @@ -0,0 +1,210 @@ +package github + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/github/github-mcp-server/pkg/translations" + "github.com/github/github-mcp-server/skills" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// pullRequestsSkillURI is the canonical URI of the bundled pull-requests +// skill, derived from the skills.Bundled entry so tests never drift from +// the single source of truth. +var pullRequestsSkillURI = skills.Bundled{Name: "pull-requests"}.URI() + +// Test_PullRequestsSkill_EmbeddedContent verifies the SEP structural requirement +// that the frontmatter `name` field matches the final segment of the skill-path +// in the URI, and that the substantive tool-sequence content is preserved. +func Test_PullRequestsSkill_EmbeddedContent(t *testing.T) { + require.NotEmpty(t, skills.PullRequestsSKILL, "SKILL.md must be embedded") + + // Normalize line endings so the test is robust to git's autocrlf behavior + // on Windows checkouts — the embedded SKILL.md may arrive as CRLF. + md := strings.ReplaceAll(skills.PullRequestsSKILL, "\r\n", "\n") + require.True(t, strings.HasPrefix(md, "---\n"), "SKILL.md must begin with YAML frontmatter") + + end := strings.Index(md[4:], "\n---\n") + require.GreaterOrEqual(t, end, 0, "SKILL.md must have closing frontmatter fence") + frontmatter := md[4 : 4+end] + + var frontmatterName string + for _, line := range strings.Split(frontmatter, "\n") { + if strings.HasPrefix(line, "name:") { + frontmatterName = strings.TrimSpace(strings.TrimPrefix(line, "name:")) + break + } + } + require.NotEmpty(t, frontmatterName, "SKILL.md frontmatter must declare `name`") + assert.Equal(t, "pull-requests", frontmatterName, "frontmatter name must match final skill-path segment in %s", pullRequestsSkillURI) + + body := md[4+end+5:] + assert.Contains(t, body, "## PR review workflow") + assert.Contains(t, body, "pull_request_review_write", "review workflow content must be preserved") + assert.Contains(t, body, "submit_pending", "the distinctive tool method must be present") +} + +// Test_BundledSkills_Registration verifies that skill resources are +// registered when the backing toolset is enabled, and omitted when it is not. +func Test_BundledSkills_Registration(t *testing.T) { + ctx := context.Background() + + t.Run("registers when pull_requests toolset enabled", func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{string(ToolsetMetadataPullRequests.ID)}). + Build() + require.NoError(t, err) + + srv := mcp.NewServer(&mcp.Implementation{Name: "test"}, &mcp.ServerOptions{ + Capabilities: &mcp.ServerCapabilities{Resources: &mcp.ResourceCapabilities{}}, + }) + RegisterBundledSkills(srv, inv) + + mimes := map[string]string{} + for _, r := range listResources(t, ctx, srv) { + mimes[r.URI] = r.MIMEType + } + assert.Equal(t, "text/markdown", mimes[pullRequestsSkillURI]) + assert.Equal(t, "application/json", mimes[skills.IndexURI]) + }) + + t.Run("omits when pull_requests toolset disabled", func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{string(ToolsetMetadataContext.ID)}). + Build() + require.NoError(t, err) + + srv := mcp.NewServer(&mcp.Implementation{Name: "test"}, &mcp.ServerOptions{ + Capabilities: &mcp.ServerCapabilities{Resources: &mcp.ResourceCapabilities{}}, + }) + RegisterBundledSkills(srv, inv) + + for _, r := range listResources(t, ctx, srv) { + assert.NotEqual(t, pullRequestsSkillURI, r.URI) + assert.NotEqual(t, skills.IndexURI, r.URI) + } + }) +} + +// Test_BundledSkills_ReadContent verifies that reading the skill resource +// returns the embedded SKILL.md content, and the index resource returns a JSON +// document matching the SEP discovery schema shape. +func Test_BundledSkills_ReadContent(t *testing.T) { + ctx := context.Background() + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{string(ToolsetMetadataPullRequests.ID)}). + Build() + require.NoError(t, err) + + srv := mcp.NewServer(&mcp.Implementation{Name: "test"}, &mcp.ServerOptions{ + Capabilities: &mcp.ServerCapabilities{Resources: &mcp.ResourceCapabilities{}}, + }) + RegisterBundledSkills(srv, inv) + + session := connectClient(t, ctx, srv) + + t.Run("SKILL.md content", func(t *testing.T) { + res, err := session.ReadResource(ctx, &mcp.ReadResourceParams{URI: pullRequestsSkillURI}) + require.NoError(t, err) + require.Len(t, res.Contents, 1) + assert.Equal(t, "text/markdown", res.Contents[0].MIMEType) + assert.Equal(t, skills.PullRequestsSKILL, res.Contents[0].Text) + }) + + t.Run("index.json matches SEP discovery schema", func(t *testing.T) { + res, err := session.ReadResource(ctx, &mcp.ReadResourceParams{URI: skills.IndexURI}) + require.NoError(t, err) + require.Len(t, res.Contents, 1) + assert.Equal(t, "application/json", res.Contents[0].MIMEType) + + var idx skills.IndexDoc + require.NoError(t, json.Unmarshal([]byte(res.Contents[0].Text), &idx)) + assert.Equal(t, skills.IndexSchema, idx.Schema) + require.Len(t, idx.Skills, 1) + assert.Equal(t, "pull-requests", idx.Skills[0].Name) + assert.Equal(t, "skill-md", idx.Skills[0].Type) + assert.Equal(t, pullRequestsSkillURI, idx.Skills[0].URL) + assert.NotEmpty(t, idx.Skills[0].Description) + }) +} + +// Test_DeclareSkillsExtensionIfEnabled verifies that the skills-over-MCP +// extension (SEP-2133) is declared in ServerOptions.Capabilities when the +// pull_requests toolset is enabled, and is absent when it is not. +func Test_DeclareSkillsExtensionIfEnabled(t *testing.T) { + t.Run("declares when pull_requests enabled", func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{string(ToolsetMetadataPullRequests.ID)}). + Build() + require.NoError(t, err) + + opts := &mcp.ServerOptions{} + DeclareSkillsExtensionIfEnabled(opts, inv) + + require.NotNil(t, opts.Capabilities) + _, ok := opts.Capabilities.Extensions[skills.ExtensionKey] + assert.True(t, ok, "skills extension must be declared") + }) + + t.Run("does not declare when pull_requests disabled", func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{string(ToolsetMetadataContext.ID)}). + Build() + require.NoError(t, err) + + opts := &mcp.ServerOptions{} + DeclareSkillsExtensionIfEnabled(opts, inv) + + if opts.Capabilities != nil { + _, ok := opts.Capabilities.Extensions[skills.ExtensionKey] + assert.False(t, ok, "skills extension must NOT be declared when no skills will be registered") + } + }) + + t.Run("preserves other extensions already declared", func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{string(ToolsetMetadataPullRequests.ID)}). + Build() + require.NoError(t, err) + + opts := &mcp.ServerOptions{ + Capabilities: &mcp.ServerCapabilities{}, + } + opts.Capabilities.AddExtension("io.example/other", map[string]any{"k": "v"}) + + DeclareSkillsExtensionIfEnabled(opts, inv) + + _, hasSkills := opts.Capabilities.Extensions[skills.ExtensionKey] + _, hasOther := opts.Capabilities.Extensions["io.example/other"] + assert.True(t, hasSkills) + assert.True(t, hasOther, "existing extensions must not be overwritten") + }) +} + +// listResources enumerates resources/list via an in-memory client session. +func listResources(t *testing.T, ctx context.Context, srv *mcp.Server) []*mcp.Resource { + t.Helper() + session := connectClient(t, ctx, srv) + res, err := session.ListResources(ctx, &mcp.ListResourcesParams{}) + require.NoError(t, err) + return res.Resources +} + +// connectClient wires an in-memory transport and returns a connected client session. +func connectClient(t *testing.T, ctx context.Context, srv *mcp.Server) *mcp.ClientSession { + t.Helper() + clientT, serverT := mcp.NewInMemoryTransports() + _, err := srv.Connect(ctx, serverT, nil) + require.NoError(t, err) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client"}, nil) + session, err := client.Connect(ctx, clientT, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = session.Close() }) + return session +} diff --git a/pkg/github/server.go b/pkg/github/server.go index ee41e90e9..93a400059 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -101,6 +101,11 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci } } + // Declare the skills-over-MCP extension (SEP-2133) when bundled skills + // will be registered. Must happen before NewServer() since capabilities + // are captured at construction. + DeclareSkillsExtensionIfEnabled(serverOpts, inv) + ghServer := NewServer(cfg.Version, cfg.Translator("SERVER_NAME", "github-mcp-server"), cfg.Translator("SERVER_TITLE", "GitHub MCP Server"), serverOpts) // Add middlewares. Order matters - for example, the error context middleware should be applied last so that it runs FIRST (closest to the handler) to ensure all errors are captured, @@ -119,6 +124,12 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci // enable toolsets or tools explicitly that do need registration). inv.RegisterAll(ctx, ghServer, deps) + // Register server-bundled Agent Skills (skills-over-MCP SEP prototype). + // Each entry is toolset-gated internally. Lives here (not in the ghmcp + // bootstrap) so it applies to both stdio and HTTP transports — the HTTP + // handler builds an mcp.Server per request via this same constructor. + RegisterBundledSkills(ghServer, inv) + // Register dynamic toolset management tools (enable/disable) - these are separate // meta-tools that control the inventory, not part of the inventory itself if cfg.DynamicToolsets { diff --git a/skills/bundled.go b/skills/bundled.go new file mode 100644 index 000000000..6afb7bb21 --- /dev/null +++ b/skills/bundled.go @@ -0,0 +1,16 @@ +// Package skills exposes the server-bundled Agent Skills shipped with this +// binary. The skill files themselves live as ordinary SKILL.md files under +// this directory — they are readable by any agent-skills consumer that +// scans repositories for skills (e.g. Claude Code, the agent-skills CLI), +// and are embedded into the server binary via //go:embed for delivery +// over MCP as skill:// resources. +// +// Keeping the skill content at this top-level location makes the files +// the primary, reusable artifact; the MCP server is one of several +// possible consumers. +package skills + +import _ "embed" + +//go:embed pull-requests/SKILL.md +var PullRequestsSKILL string diff --git a/skills/pull-requests/SKILL.md b/skills/pull-requests/SKILL.md new file mode 100644 index 000000000..9c8d80b84 --- /dev/null +++ b/skills/pull-requests/SKILL.md @@ -0,0 +1,8 @@ +--- +name: pull-requests +description: Submit a multi-comment GitHub pull request review using the pending-review workflow (pull_request_review_write → add_comment_to_pending_review → submit_pending). Use when leaving line-specific feedback on a pull request, when asked to review a PR, or whenever creating any review with more than one comment. +--- + +## PR review workflow + +PR review workflow: Always use 'pull_request_review_write' with method 'create' to create a pending review, then 'add_comment_to_pending_review' to add comments, and finally 'pull_request_review_write' with method 'submit_pending' to submit the review for complex reviews with line-specific comments. From 0c1d89cf67299058b074855bc6a921a4fc953e74 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 19 Apr 2026 20:50:01 -0700 Subject: [PATCH 3/4] docs(skill): expand pull-requests SKILL.md beyond inline instructions The bundled skill now reads differently from the inline server instructions rather than mirroring them, showing the point of progressive disclosure: a skill-aware host spends no extra context on every request but can pull the deeper version on demand. Restructure into "When to use", "Workflow" (numbered steps), and "Caveats" sections per the agentskills.io spec's recommended body shape. Add content that is *not* in the inline instructions: - when to skip this flow (single top-level comment / approve without inline feedback) - the pending-ness hinges on omitting `event` in step 1 - specific line-ref parameter names (path, line, side, startLine, startSide) - the concrete `APPROVE | REQUEST_CHANGES | COMMENT` event values - pending reviews are invisible to the PR author and can be deleted via `method: "delete_pending"` Update the embedded-content test to check for the new `## Workflow` section header and a second tool-name presence assertion. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/github/bundled_skills_test.go | 3 ++- skills/pull-requests/SKILL.md | 22 ++++++++++++++++++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/pkg/github/bundled_skills_test.go b/pkg/github/bundled_skills_test.go index 13eda7245..7a48b5085 100644 --- a/pkg/github/bundled_skills_test.go +++ b/pkg/github/bundled_skills_test.go @@ -44,8 +44,9 @@ func Test_PullRequestsSkill_EmbeddedContent(t *testing.T) { assert.Equal(t, "pull-requests", frontmatterName, "frontmatter name must match final skill-path segment in %s", pullRequestsSkillURI) body := md[4+end+5:] - assert.Contains(t, body, "## PR review workflow") + assert.Contains(t, body, "## Workflow", "skill body must carry the workflow section") assert.Contains(t, body, "pull_request_review_write", "review workflow content must be preserved") + assert.Contains(t, body, "add_comment_to_pending_review", "review workflow content must be preserved") assert.Contains(t, body, "submit_pending", "the distinctive tool method must be present") } diff --git a/skills/pull-requests/SKILL.md b/skills/pull-requests/SKILL.md index 9c8d80b84..faa47b8c7 100644 --- a/skills/pull-requests/SKILL.md +++ b/skills/pull-requests/SKILL.md @@ -3,6 +3,24 @@ name: pull-requests description: Submit a multi-comment GitHub pull request review using the pending-review workflow (pull_request_review_write → add_comment_to_pending_review → submit_pending). Use when leaving line-specific feedback on a pull request, when asked to review a PR, or whenever creating any review with more than one comment. --- -## PR review workflow +## When to use -PR review workflow: Always use 'pull_request_review_write' with method 'create' to create a pending review, then 'add_comment_to_pending_review' to add comments, and finally 'pull_request_review_write' with method 'submit_pending' to submit the review for complex reviews with line-specific comments. +Use this skill when submitting a pull request review that will include more than one comment, especially line-specific comments placed on particular files or diff lines. + +**Skip this flow** — call `pull_request_review_write` with `method: "create"` and supply `body` and `event` directly — when: + +- Leaving a single top-level comment with no line references. +- Approving or requesting changes without inline feedback. + +## Workflow + +Submit a multi-comment review using the three-step pending-review flow: + +1. **Open a pending review.** Call `pull_request_review_write` with `method: "create"` **and no `event`**. Omitting `event` is what makes the review pending instead of submitting it immediately. +2. **Add each comment.** Call `add_comment_to_pending_review` once per comment, supplying `path` and a line reference (`line`/`side` for a single line, or `startLine`/`startSide` plus `line`/`side` for a multi-line range). This tool requires that a pending review already exists for the current user on this PR. +3. **Submit the review.** Call `pull_request_review_write` with `method: "submit_pending"`, an optional summary `body`, and an `event` indicating the review state — one of `APPROVE`, `REQUEST_CHANGES`, or `COMMENT`. + +## Caveats + +- **Always complete step 3.** A pending review is invisible to the PR author until `submit_pending` is called. If you stop partway through, the draft stays on the reviewer's side and can be resumed later or removed with `method: "delete_pending"`. +- **Do not pass `event` in step 1.** Providing `event` to `create` submits the review immediately and leaves no pending review for `add_comment_to_pending_review` to attach to. From 86f0549407225cd3c64afcfca871f6bbd9cb3b55 Mon Sep 17 00:00:00 2001 From: olaservo Date: Sun, 19 Apr 2026 21:23:52 -0700 Subject: [PATCH 4/4] feat(skill): add inbox-triage skill for notifications toolset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ship a second bundled skill that walks the agent through systematic GitHub notifications triage: enumerate with list_notifications, partition by reason (review_requested / mention / assign / security_alert — high; author / comment / state_change — medium; ci_activity / subscribed — low), act on high-priority items, then dismiss with state "done" or "read" per the skill's rule. Demonstrates: - Multiple bundled skills in one server (registry now has two entries). - Per-skill toolset gating — pull-requests gates on pull_requests, inbox-triage gates on the non-default notifications toolset, so enabling one does not force the other. - Cross-skill reference (the inbox-triage workflow points at the pull-requests skill when handling review_requested items). - Skills teaching workflow judgment (priority buckets) that tool descriptions alone cannot encode. Tests cover the symmetric structural checks, per-toolset registration paths, the multi-skill index.json shape, and the capability declaration firing on either toolset. Co-Authored-By: Claude Opus 4.7 (1M context) --- pkg/github/bundled_skills.go | 22 +++-- pkg/github/bundled_skills_test.go | 129 +++++++++++++++++++++++++++++- skills/bundled.go | 3 + skills/inbox-triage/SKILL.md | 48 +++++++++++ 4 files changed, 191 insertions(+), 11 deletions(-) create mode 100644 skills/inbox-triage/SKILL.md diff --git a/pkg/github/bundled_skills.go b/pkg/github/bundled_skills.go index 5f295eb0f..36ed56d0d 100644 --- a/pkg/github/bundled_skills.go +++ b/pkg/github/bundled_skills.go @@ -14,13 +14,21 @@ import ( // Adding a new server-bundled skill is one entry here plus a //go:embed // line in package skills. func bundledSkills(inv *inventory.Inventory) *skills.Registry { - return skills.New().Add(skills.Bundled{ - Name: "pull-requests", - Description: "Submit a multi-comment GitHub pull request review using the pending-review workflow. Use when leaving line-specific feedback on a pull request, when asked to review a PR, or whenever creating any review with more than one comment.", - Content: skills.PullRequestsSKILL, - Icons: octicons.Icons("light-bulb"), - Enabled: func() bool { return inv.IsToolsetEnabled(ToolsetMetadataPullRequests.ID) }, - }) + return skills.New(). + Add(skills.Bundled{ + Name: "pull-requests", + Description: "Submit a multi-comment GitHub pull request review using the pending-review workflow. Use when leaving line-specific feedback on a pull request, when asked to review a PR, or whenever creating any review with more than one comment.", + Content: skills.PullRequestsSKILL, + Icons: octicons.Icons("light-bulb"), + Enabled: func() bool { return inv.IsToolsetEnabled(ToolsetMetadataPullRequests.ID) }, + }). + Add(skills.Bundled{ + Name: "inbox-triage", + Description: "Systematically triage the current user's GitHub notifications inbox — enumerate unread items, prioritize by notification reason (review requests, mentions, assignments, security alerts), act on the high-priority ones, then dismiss the rest. Use when the user asks \"what should I work on?\", \"catch me up on GitHub\", \"triage my inbox\", \"what needs my attention?\", or otherwise wants to clear their notifications backlog.", + Content: skills.InboxTriageSKILL, + Icons: octicons.Icons("bell"), + Enabled: func() bool { return inv.IsToolsetEnabled(ToolsetMetadataNotifications.ID) }, + }) } // DeclareSkillsExtensionIfEnabled adds the skills-over-MCP extension diff --git a/pkg/github/bundled_skills_test.go b/pkg/github/bundled_skills_test.go index 7a48b5085..48269d16c 100644 --- a/pkg/github/bundled_skills_test.go +++ b/pkg/github/bundled_skills_test.go @@ -13,10 +13,13 @@ import ( "github.com/stretchr/testify/require" ) -// pullRequestsSkillURI is the canonical URI of the bundled pull-requests -// skill, derived from the skills.Bundled entry so tests never drift from -// the single source of truth. -var pullRequestsSkillURI = skills.Bundled{Name: "pull-requests"}.URI() +// pullRequestsSkillURI / inboxTriageSkillURI are the canonical URIs of the +// bundled skills, derived from skills.Bundled so tests never drift from the +// single source of truth. +var ( + pullRequestsSkillURI = skills.Bundled{Name: "pull-requests"}.URI() + inboxTriageSkillURI = skills.Bundled{Name: "inbox-triage"}.URI() +) // Test_PullRequestsSkill_EmbeddedContent verifies the SEP structural requirement // that the frontmatter `name` field matches the final segment of the skill-path @@ -50,6 +53,35 @@ func Test_PullRequestsSkill_EmbeddedContent(t *testing.T) { assert.Contains(t, body, "submit_pending", "the distinctive tool method must be present") } +// Test_InboxTriageSkill_EmbeddedContent verifies the SEP structural +// requirements for the inbox-triage skill and that its substantive tool +// references are preserved. +func Test_InboxTriageSkill_EmbeddedContent(t *testing.T) { + require.NotEmpty(t, skills.InboxTriageSKILL, "SKILL.md must be embedded") + + md := strings.ReplaceAll(skills.InboxTriageSKILL, "\r\n", "\n") + require.True(t, strings.HasPrefix(md, "---\n"), "SKILL.md must begin with YAML frontmatter") + + end := strings.Index(md[4:], "\n---\n") + require.GreaterOrEqual(t, end, 0, "SKILL.md must have closing frontmatter fence") + frontmatter := md[4 : 4+end] + + var frontmatterName string + for _, line := range strings.Split(frontmatter, "\n") { + if strings.HasPrefix(line, "name:") { + frontmatterName = strings.TrimSpace(strings.TrimPrefix(line, "name:")) + break + } + } + require.NotEmpty(t, frontmatterName, "SKILL.md frontmatter must declare `name`") + assert.Equal(t, "inbox-triage", frontmatterName, "frontmatter name must match final skill-path segment in %s", inboxTriageSkillURI) + + body := md[4+end+5:] + assert.Contains(t, body, "## Workflow") + assert.Contains(t, body, "list_notifications", "triage workflow must reference list_notifications") + assert.Contains(t, body, "dismiss_notification", "triage workflow must reference dismiss_notification") +} + // Test_BundledSkills_Registration verifies that skill resources are // registered when the backing toolset is enabled, and omitted when it is not. func Test_BundledSkills_Registration(t *testing.T) { @@ -87,9 +119,53 @@ func Test_BundledSkills_Registration(t *testing.T) { for _, r := range listResources(t, ctx, srv) { assert.NotEqual(t, pullRequestsSkillURI, r.URI) + assert.NotEqual(t, inboxTriageSkillURI, r.URI) assert.NotEqual(t, skills.IndexURI, r.URI) } }) + + t.Run("registers inbox-triage when notifications toolset enabled", func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{string(ToolsetMetadataNotifications.ID)}). + Build() + require.NoError(t, err) + + srv := mcp.NewServer(&mcp.Implementation{Name: "test"}, &mcp.ServerOptions{ + Capabilities: &mcp.ServerCapabilities{Resources: &mcp.ResourceCapabilities{}}, + }) + RegisterBundledSkills(srv, inv) + + uris := map[string]string{} + for _, r := range listResources(t, ctx, srv) { + uris[r.URI] = r.MIMEType + } + assert.Equal(t, "text/markdown", uris[inboxTriageSkillURI]) + assert.NotContains(t, uris, pullRequestsSkillURI, "only notifications enabled — pull-requests should not be registered") + assert.Equal(t, "application/json", uris[skills.IndexURI]) + }) + + t.Run("registers both when both toolsets enabled", func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{ + string(ToolsetMetadataPullRequests.ID), + string(ToolsetMetadataNotifications.ID), + }). + Build() + require.NoError(t, err) + + srv := mcp.NewServer(&mcp.Implementation{Name: "test"}, &mcp.ServerOptions{ + Capabilities: &mcp.ServerCapabilities{Resources: &mcp.ResourceCapabilities{}}, + }) + RegisterBundledSkills(srv, inv) + + uris := map[string]struct{}{} + for _, r := range listResources(t, ctx, srv) { + uris[r.URI] = struct{}{} + } + assert.Contains(t, uris, pullRequestsSkillURI) + assert.Contains(t, uris, inboxTriageSkillURI) + assert.Contains(t, uris, skills.IndexURI) + }) } // Test_BundledSkills_ReadContent verifies that reading the skill resource @@ -134,6 +210,37 @@ func Test_BundledSkills_ReadContent(t *testing.T) { }) } +// Test_BundledSkills_Index_MultipleSkills verifies that all enabled skills +// appear in the discovery index, not just the first one. +func Test_BundledSkills_Index_MultipleSkills(t *testing.T) { + ctx := context.Background() + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{ + string(ToolsetMetadataPullRequests.ID), + string(ToolsetMetadataNotifications.ID), + }). + Build() + require.NoError(t, err) + + srv := mcp.NewServer(&mcp.Implementation{Name: "test"}, &mcp.ServerOptions{ + Capabilities: &mcp.ServerCapabilities{Resources: &mcp.ResourceCapabilities{}}, + }) + RegisterBundledSkills(srv, inv) + + session := connectClient(t, ctx, srv) + res, err := session.ReadResource(ctx, &mcp.ReadResourceParams{URI: skills.IndexURI}) + require.NoError(t, err) + + var idx skills.IndexDoc + require.NoError(t, json.Unmarshal([]byte(res.Contents[0].Text), &idx)) + names := map[string]string{} + for _, s := range idx.Skills { + names[s.Name] = s.URL + } + assert.Equal(t, pullRequestsSkillURI, names["pull-requests"]) + assert.Equal(t, inboxTriageSkillURI, names["inbox-triage"]) +} + // Test_DeclareSkillsExtensionIfEnabled verifies that the skills-over-MCP // extension (SEP-2133) is declared in ServerOptions.Capabilities when the // pull_requests toolset is enabled, and is absent when it is not. @@ -167,6 +274,20 @@ func Test_DeclareSkillsExtensionIfEnabled(t *testing.T) { } }) + t.Run("declares when notifications enabled (any skill triggers declaration)", func(t *testing.T) { + inv, err := NewInventory(translations.NullTranslationHelper). + WithToolsets([]string{string(ToolsetMetadataNotifications.ID)}). + Build() + require.NoError(t, err) + + opts := &mcp.ServerOptions{} + DeclareSkillsExtensionIfEnabled(opts, inv) + + require.NotNil(t, opts.Capabilities) + _, ok := opts.Capabilities.Extensions[skills.ExtensionKey] + assert.True(t, ok, "skills extension must be declared when any bundled skill is enabled") + }) + t.Run("preserves other extensions already declared", func(t *testing.T) { inv, err := NewInventory(translations.NullTranslationHelper). WithToolsets([]string{string(ToolsetMetadataPullRequests.ID)}). diff --git a/skills/bundled.go b/skills/bundled.go index 6afb7bb21..abc7fc80d 100644 --- a/skills/bundled.go +++ b/skills/bundled.go @@ -14,3 +14,6 @@ import _ "embed" //go:embed pull-requests/SKILL.md var PullRequestsSKILL string + +//go:embed inbox-triage/SKILL.md +var InboxTriageSKILL string diff --git a/skills/inbox-triage/SKILL.md b/skills/inbox-triage/SKILL.md new file mode 100644 index 000000000..57382959f --- /dev/null +++ b/skills/inbox-triage/SKILL.md @@ -0,0 +1,48 @@ +--- +name: inbox-triage +description: Systematically triage the current user's GitHub notifications inbox — enumerate unread items, prioritize by notification reason (review requests, mentions, assignments, security alerts), act on the high-priority ones, then dismiss the rest. Use when the user asks "what should I work on?", "catch me up on GitHub", "triage my inbox", "what needs my attention?", or otherwise wants to clear their notifications backlog. +--- + +## When to use + +Use this skill when the user asks about their GitHub inbox, pending work, or outstanding notifications — any of: + +- "What should I work on next?" +- "Catch me up on GitHub." +- "Triage my inbox." +- "What needs my attention?" +- "Clear my notifications." + +## Workflow + +1. **Enumerate.** Call `list_notifications` with `filter: "default"` (unread only — the common case). Switch to `filter: "include_read"` only if the user explicitly asks for a full sweep. Pass `since` as an RFC3339 timestamp to scope to recent activity (e.g. the last day or since the last triage). + +2. **Partition by `reason`.** Each notification carries a `reason` field. Group into priority buckets: + + - **High — act or respond promptly:** + - `review_requested` — someone is waiting on your review. + - `mention` / `team_mention` — you were @-referenced. + - `assign` — you were assigned an issue or PR. + - `security_alert` — security advisory or Dependabot alert. + - **Medium — read and decide:** + - `author` — updates on threads you opened. + - `comment` — replies on threads you participated in. + - `state_change` — issue/PR closed or reopened. + - **Low — usually safe to mark read without reading:** + - `ci_activity` — workflow runs. Look only if you own CI for this repo. + - `subscribed` — repo-watch updates on threads you haven't participated in. + +3. **Drill in on high-priority.** For each high-priority notification, call `get_notification_details` to inspect the item, then take the appropriate action — leave a review (see the `pull-requests` skill), comment, close, etc. + +4. **Dismiss as you go.** After acting on (or deciding to skip) each high-priority item, call `dismiss_notification` with the `threadID` and a `state`: + - `state: "done"` archives the notification so it no longer appears in default queries. Use for items you've fully resolved. + - `state: "read"` keeps the notification visible but marks it acknowledged. Use for "I've seen this, coming back later." + +5. **Bulk-close the noise.** After the high-priority pass, if a large medium/low bucket remains and the user is comfortable, call `mark_all_notifications_read`. Only do this with explicit user approval — a blanket mark-read can bury something the partitioning rules missed. + +## Caveats + +- **`read` vs `done` matters.** `read` leaves the notification in the default inbox; `done` removes it. Pick intentionally based on whether there's follow-up. +- **Silence chatty threads.** If one issue/PR is generating a flood, call `manage_notification_subscription` with action `ignore` to silence that specific thread. For an entire noisy repository, use `manage_repository_notification_subscription`. +- **Surface decisions, don't hide them.** After each bucket, summarize to the user what you acted on, what you dismissed, and what's left open for them. Do not silently mark-read a pile of notifications. +- **Respect scope.** If the user narrows to a specific repo ("triage my inbox for `owner/repo`"), pass `owner` and `repo` to `list_notifications` rather than filtering client-side after fetching everything.