refactor(skills): rename to match github#2374 catalogue, drop per-skill gating

Renames the two original skills to the names Sam uses in
github#2374, and makes their load behavior consistent
with the 25 skills imported from that PR.

- pull-requests → review-pr
- inbox-triage  → handle-notifications

Both renames preserve our richer content (parameter-level workflow detail
for review-pr; High/Medium/Low priority taxonomy + read/done semantics +
discover-mcp-skills cross-link for handle-notifications). Only the names
change, plus the cross-reference inside handle-notifications/SKILL.md.

Load consistency:
- Drop the per-skill `Enabled func() bool` closures. All bundled skills
  now register unconditionally, matching the 25 imported skills and
  Sam's github#2374 semantics.
- Drop the per-skill `Icons` field (octicons.Icons("light-bulb") didn't
  resolve in this fork's icon set anyway, and the imported skills set
  no icons).
- The Registry's gating mechanism stays in place for future use; just
  no shipped skill currently exercises it. Test_DeclareSkillsExtensionIfEnabled's
  "empty registry" subtest covers the gating-off path directly.

Tests:
- Test_PullRequestsSkill_EmbeddedContent → Test_ReviewPRSkill_EmbeddedContent
- Test_InboxTriageSkill_EmbeddedContent → Test_HandleNotificationsSkill_EmbeddedContent
- Test_BundledSkills_Registration (toolset-gating subtests) collapsed into
  Test_BundledSkills_RegisterRegardlessOfToolset
- Test_DeclareSkillsExtensionIfEnabled simplified: 4 subtests → 3, since
  the toolset-specific subtests are now redundant with always-on semantics

Caveat for downstream consumers: any client hardcoding the old URIs
(skill://github/pull-requests/SKILL.md, skill://github/inbox-triage/SKILL.md)
needs to update to the new ones.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: olaservo

pkg/github/bundled_skills.go

@@ -2,35 +2,30 @@ 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.
 //
-// The first two entries are toolset-gated — they only register when the
-// relevant toolset is enabled. The remaining workflow-oriented skills
-// (ported from github/github-mcp-server#2374) are always-on; their
-// `allowed-tools` frontmatter is advisory metadata, not a gate.
+// All bundled skills load uniformly: always-on, no per-skill toolset
+// gating, no icons. Their `allowed-tools` frontmatter is advisory only.
+// The Registry's `Enabled` closure is still available for future use
+// (e.g. feature-flagging a skill behind an experimental toolset).
 //
 // 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) },
+			Name:        "review-pr",
+			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.",
+			Content:     skills.ReviewPRSKILL,
 		}).
 		Add(skills.Bundled{
-			Name:        "inbox-triage",
+			Name:        "handle-notifications",
 			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) },
+			Content:     skills.HandleNotificationsSKILL,
 		}).
 		Add(skills.Bundled{
 			Name:        "get-context",

pkg/github/bundled_skills_test.go

@@ -13,23 +13,23 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// pullRequestsSkillURI / inboxTriageSkillURI are the canonical URIs of the
-// bundled skills, derived from skills.Bundled so tests never drift from the
-// single source of truth.
+// reviewPRSkillURI / handleNotificationsSkillURI are the canonical URIs of the
+// two skills with the most distinctive content. 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()
+	reviewPRSkillURI            = skills.Bundled{Name: "review-pr"}.URI()
+	handleNotificationsSkillURI = skills.Bundled{Name: "handle-notifications"}.URI()
 )
 
-// Test_PullRequestsSkill_EmbeddedContent verifies the SEP structural requirement
+// Test_ReviewPRSkill_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")
+func Test_ReviewPRSkill_EmbeddedContent(t *testing.T) {
+	require.NotEmpty(t, skills.ReviewPRSKILL, "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")
+	md := strings.ReplaceAll(skills.ReviewPRSKILL, "\r\n", "\n")
 	require.True(t, strings.HasPrefix(md, "---\n"), "SKILL.md must begin with YAML frontmatter")
 
 	end := strings.Index(md[4:], "\n---\n")
@@ -44,7 +44,7 @@ func Test_PullRequestsSkill_EmbeddedContent(t *testing.T) {
 		}
 	}
 	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)
+	assert.Equal(t, "review-pr", frontmatterName, "frontmatter name must match final skill-path segment in %s", reviewPRSkillURI)
 
 	body := md[4+end+5:]
 	assert.Contains(t, body, "## Workflow", "skill body must carry the workflow section")
@@ -53,13 +53,13 @@ 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")
+// Test_HandleNotificationsSkill_EmbeddedContent verifies the SEP structural
+// requirements for the handle-notifications skill and that its substantive
+// tool references are preserved.
+func Test_HandleNotificationsSkill_EmbeddedContent(t *testing.T) {
+	require.NotEmpty(t, skills.HandleNotificationsSKILL, "SKILL.md must be embedded")
 
-	md := strings.ReplaceAll(skills.InboxTriageSKILL, "\r\n", "\n")
+	md := strings.ReplaceAll(skills.HandleNotificationsSKILL, "\r\n", "\n")
 	require.True(t, strings.HasPrefix(md, "---\n"), "SKILL.md must begin with YAML frontmatter")
 
 	end := strings.Index(md[4:], "\n---\n")
@@ -74,103 +74,41 @@ func Test_InboxTriageSkill_EmbeddedContent(t *testing.T) {
 		}
 	}
 	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)
+	assert.Equal(t, "handle-notifications", frontmatterName, "frontmatter name must match final skill-path segment in %s", handleNotificationsSkillURI)
 
 	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) {
+// Test_BundledSkills_RegisterRegardlessOfToolset verifies that bundled skills
+// load uniformly — they're always-on and don't depend on which toolsets the
+// inventory has enabled. Per-skill toolset gating remains available via the
+// Registry's Enabled closure (covered by registry-level unit tests, not here)
+// but no shipped skill currently uses it.
+func Test_BundledSkills_RegisterRegardlessOfToolset(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 toolset-gated skills when their toolsets are 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)
-
-		uris := map[string]struct{}{}
-		for _, r := range listResources(t, ctx, srv) {
-			uris[r.URI] = struct{}{}
-		}
-		// Toolset-gated skills must be absent.
-		assert.NotContains(t, uris, pullRequestsSkillURI, "pull-requests is gated on pull_requests toolset")
-		assert.NotContains(t, uris, inboxTriageSkillURI, "inbox-triage is gated on notifications toolset")
-		// Always-on skills (e.g. get-context) and the index remain registered.
-		assert.Contains(t, uris, skills.Bundled{Name: "get-context"}.URI(), "always-on skill must still register")
-		assert.Contains(t, uris, skills.IndexURI, "index is published whenever any skill is enabled")
-	})
-
-	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)
+	// Pick a minimal toolset (context only) — doesn't enable pull_requests
+	// or notifications, the toolsets the renamed skills used to be gated on.
+	inv, err := NewInventory(translations.NullTranslationHelper).
+		WithToolsets([]string{string(ToolsetMetadataContext.ID)}).
+		Build()
+	require.NoError(t, err)
 
-		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])
+	srv := mcp.NewServer(&mcp.Implementation{Name: "test"}, &mcp.ServerOptions{
+		Capabilities: &mcp.ServerCapabilities{Resources: &mcp.ResourceCapabilities{}},
 	})
+	RegisterBundledSkills(srv, inv)
 
-	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)
-	})
+	uris := map[string]string{}
+	for _, r := range listResources(t, ctx, srv) {
+		uris[r.URI] = r.MIMEType
+	}
+	assert.Equal(t, "text/markdown", uris[reviewPRSkillURI], "review-pr registers regardless of toolset")
+	assert.Equal(t, "text/markdown", uris[handleNotificationsSkillURI], "handle-notifications registers regardless of toolset")
+	assert.Equal(t, "application/json", uris[skills.IndexURI])
 }
 
 // Test_BundledSkills_ReadContent verifies that reading the skill resource
@@ -179,7 +117,7 @@ func Test_BundledSkills_Registration(t *testing.T) {
 func Test_BundledSkills_ReadContent(t *testing.T) {
 	ctx := context.Background()
 	inv, err := NewInventory(translations.NullTranslationHelper).
-		WithToolsets([]string{string(ToolsetMetadataPullRequests.ID)}).
+		WithToolsets([]string{"all"}).
 		Build()
 	require.NoError(t, err)
 
@@ -191,11 +129,11 @@ func Test_BundledSkills_ReadContent(t *testing.T) {
 	session := connectClient(t, ctx, srv)
 
 	t.Run("SKILL.md content", func(t *testing.T) {
-		res, err := session.ReadResource(ctx, &mcp.ReadResourceParams{URI: pullRequestsSkillURI})
+		res, err := session.ReadResource(ctx, &mcp.ReadResourceParams{URI: reviewPRSkillURI})
 		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)
+		assert.Equal(t, skills.ReviewPRSKILL, res.Contents[0].Text)
 	})
 
 	t.Run("index.json matches SEP discovery schema", func(t *testing.T) {
@@ -208,33 +146,31 @@ func Test_BundledSkills_ReadContent(t *testing.T) {
 		require.NoError(t, json.Unmarshal([]byte(res.Contents[0].Text), &idx))
 		assert.Equal(t, skills.IndexSchema, idx.Schema)
 
-		// Index size must equal the number of currently-enabled bundled skills.
-		assert.Len(t, idx.Skills, len(bundledSkills(inv).Enabled()))
+		// Index size must equal the number of currently-enabled bundled skills + templates.
+		registry := bundledSkills(inv)
+		assert.Len(t, idx.Skills, len(registry.Enabled())+len(registry.EnabledTemplates()))
 
-		// The pull-requests skill (toolset-gated, currently enabled) must be present.
+		// The review-pr skill must be present.
 		var found *skills.IndexEntry
 		for i := range idx.Skills {
-			if idx.Skills[i].Name == "pull-requests" {
+			if idx.Skills[i].Name == "review-pr" {
 				found = &idx.Skills[i]
 				break
 			}
 		}
-		require.NotNil(t, found, "pull-requests must appear in the index")
+		require.NotNil(t, found, "review-pr must appear in the index")
 		assert.Equal(t, "skill-md", found.Type)
-		assert.Equal(t, pullRequestsSkillURI, found.URL)
+		assert.Equal(t, reviewPRSkillURI, found.URL)
 		assert.NotEmpty(t, found.Description)
 	})
 }
 
-// Test_BundledSkills_Index_MultipleSkills verifies that all enabled skills
+// Test_BundledSkills_Index_MultipleSkills verifies that all bundled 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),
-		}).
+		WithToolsets([]string{string(ToolsetMetadataContext.ID)}).
 		Build()
 	require.NoError(t, err)
 
@@ -253,31 +189,17 @@ func Test_BundledSkills_Index_MultipleSkills(t *testing.T) {
 	for _, s := range idx.Skills {
 		names[s.Name] = s.URL
 	}
-	assert.Equal(t, pullRequestsSkillURI, names["pull-requests"])
-	assert.Equal(t, inboxTriageSkillURI, names["inbox-triage"])
+	assert.Equal(t, reviewPRSkillURI, names["review-pr"])
+	assert.Equal(t, handleNotificationsSkillURI, names["handle-notifications"])
 }
 
 // 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.
+// extension (SEP-2133) is declared in ServerOptions.Capabilities whenever the
+// server has any bundled skill or template entry to publish, and that
+// declaration is additive (doesn't clobber other extensions).
 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("declares even when no toolset-gated skills enabled (always-on skills exist)", func(t *testing.T) {
-		// Even with only the context toolset, always-on bundled skills (e.g. get-context)
-		// register, so the extension capability MUST still be declared.
+	t.Run("declares for any non-empty registry", func(t *testing.T) {
+		// All bundled skills are always-on, so any inventory triggers the declaration.
 		inv, err := NewInventory(translations.NullTranslationHelper).
 			WithToolsets([]string{string(ToolsetMetadataContext.ID)}).
 			Build()
@@ -291,23 +213,9 @@ func Test_DeclareSkillsExtensionIfEnabled(t *testing.T) {
 		assert.True(t, ok, "always-on skills register, so the extension must be declared")
 	})
 
-	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)}).
+			WithToolsets([]string{string(ToolsetMetadataContext.ID)}).
 			Build()
 		require.NoError(t, err)
 
@@ -323,6 +231,17 @@ func Test_DeclareSkillsExtensionIfEnabled(t *testing.T) {
 		assert.True(t, hasSkills)
 		assert.True(t, hasOther, "existing extensions must not be overwritten")
 	})
+
+	t.Run("does not declare for an empty registry", func(t *testing.T) {
+		// Tests the Registry's failure path directly — bypasses bundledSkills()
+		// since no shipped configuration produces an empty registry.
+		opts := &mcp.ServerOptions{}
+		skills.New().DeclareCapability(opts)
+		if opts.Capabilities != nil {
+			_, ok := opts.Capabilities.Extensions[skills.ExtensionKey]
+			assert.False(t, ok, "empty registry must not declare the extension")
+		}
+	})
 }
 
 // Test_BundledSkills_AllRegistered_WhenAllToolsetsEnabled verifies that with the

skills/bundled.go

@@ -12,11 +12,11 @@ package skills
 
 import _ "embed"
 
-//go:embed pull-requests/SKILL.md
-var PullRequestsSKILL string
+//go:embed review-pr/SKILL.md
+var ReviewPRSKILL string
 
-//go:embed inbox-triage/SKILL.md
-var InboxTriageSKILL string
+//go:embed handle-notifications/SKILL.md
+var HandleNotificationsSKILL string
 
 //go:embed get-context/SKILL.md
 var GetContextSKILL string