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 switches them from toolset-gated
loading to always-on, matching how upstream's skills load.

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

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

Load consistency:
- Drop the per-skill `Enabled func() bool` closures so both skills
  register unconditionally, matching upstream's "everything always
  available" semantics.
- Drop the per-skill `Icons` field. The `light-bulb` octicon used by
  pull-requests didn't resolve in this fork's icon set anyway, and
  shipping no icons is the simpler, more consistent default.
- The Registry's `Enabled` closure mechanism stays in place for future
  use; just no shipped skill currently exercises it. A new
  Test_DeclareSkillsExtensionIfEnabled subtest covers the
  empty-registry 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 from 4 subtests to 3.

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

Author: olaservo

pkg/github/bundled_skills.go

@@ -2,32 +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.
-// Each entry's Enabled closure gates its publication on the relevant
-// toolset being enabled under the given inventory.
+//
+// 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 {
+func bundledSkills(_ *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,
 		})
 }
 

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 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()
+	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,98 +74,40 @@ 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 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 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, 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)
+	// 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
@@ -174,7 +116,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{string(ToolsetMetadataContext.ID)}).
 		Build()
 	require.NoError(t, err)
 
@@ -186,11 +128,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) {
@@ -202,23 +144,31 @@ func Test_BundledSkills_ReadContent(t *testing.T) {
 		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)
+
+		// Index size must equal the number of currently-enabled bundled skills.
+		assert.Len(t, idx.Skills, len(bundledSkills(inv).Enabled()))
+
+		// The review-pr skill must be present.
+		var found *skills.IndexEntry
+		for i := range idx.Skills {
+			if idx.Skills[i].Name == "review-pr" {
+				found = &idx.Skills[i]
+				break
+			}
+		}
+		require.NotNil(t, found, "review-pr must appear in the index")
+		assert.Equal(t, "skill-md", found.Type)
+		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)
 
@@ -237,29 +187,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("does not declare when pull_requests disabled", func(t *testing.T) {
+	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()
@@ -268,29 +206,14 @@ func Test_DeclareSkillsExtensionIfEnabled(t *testing.T) {
 		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("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")
+		assert.True(t, ok, "always-on skills register, so the extension must be declared")
 	})
 
 	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)
 
@@ -306,6 +229,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")
+		}
+	})
 }
 
 // listResources enumerates resources/list via an in-memory client session.

skills/bundled.go

@@ -12,8 +12,8 @@ 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