Fix and unskip some skills E2E tests (github#69)

* Re-enable skills tests for Node

* Re-enable them for .NET

* Re-enable and fix for Python

* Re-enable and fix Go tests

* Actually skip

* Formatting

* More formatting

Author: SteveSandersonMS

dotnet/test/SkillsTests.cs

@@ -2,7 +2,6 @@
  *  Copyright (c) Microsoft Corporation. All rights reserved.
  *--------------------------------------------------------------------------------------------*/
 
-using GitHub.Copilot.SDK.Test.Harness;
 using Xunit;
 using Xunit.Abstractions;
 
@@ -11,18 +10,23 @@ namespace GitHub.Copilot.SDK.Test;
 public class SkillsTests : E2ETestBase
 {
     private const string SkillMarker = "PINEAPPLE_COCONUT_42";
-    private static int _skillDirCounter = 0;
 
     private readonly string _workDir;
 
     public SkillsTests(E2ETestFixture fixture, ITestOutputHelper output) : base(fixture, "skills", output)
     {
         _workDir = fixture.Ctx.WorkDir;
+
+        var skillsDir = Path.Join(_workDir, ".test_skills");
+        if (Directory.Exists(skillsDir))
+        {
+            Directory.Delete(skillsDir, recursive: true);
+        }
     }
 
     private string CreateSkillDir()
     {
-        var skillsDir = Path.Join(_workDir, ".test_skills", $"copilot-skills-test-{++_skillDirCounter}");
+        var skillsDir = Path.Join(_workDir, ".test_skills");
         Directory.CreateDirectory(skillsDir);
 
         // Create a skill subdirectory with SKILL.md
@@ -44,7 +48,7 @@ private string CreateSkillDir()
         return skillsDir;
     }
 
-    [Fact(Skip = "Skills tests temporarily skipped")]
+    [Fact]
     public async Task Should_Load_And_Apply_Skill_From_SkillDirectories()
     {
         var skillsDir = CreateSkillDir();
@@ -63,7 +67,7 @@ public async Task Should_Load_And_Apply_Skill_From_SkillDirectories()
         await session.DisposeAsync();
     }
 
-    [Fact(Skip = "Skills tests temporarily skipped")]
+    [Fact]
     public async Task Should_Not_Apply_Skill_When_Disabled_Via_DisabledSkills()
     {
         var skillsDir = CreateSkillDir();
@@ -83,7 +87,7 @@ public async Task Should_Not_Apply_Skill_When_Disabled_Via_DisabledSkills()
         await session.DisposeAsync();
     }
 
-    [Fact(Skip = "Skills tests temporarily skipped")]
+    [Fact(Skip = "See the big comment around the equivalent test in the Node SDK. Skipped because the feature doesn't work correctly yet.")]
     public async Task Should_Apply_Skill_On_Session_Resume_With_SkillDirectories()
     {
         var skillsDir = CreateSkillDir();

go/e2e/skills_test.go

@@ -1,7 +1,6 @@
 package e2e
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -14,11 +13,15 @@ import (
 
 const skillMarker = "PINEAPPLE_COCONUT_42"
 
-var skillDirCounter = 0
+func cleanSkillsDir(t *testing.T, workDir string) {
+	skillsDir := filepath.Join(workDir, ".test_skills")
+	if err := os.RemoveAll(skillsDir); err != nil {
+		t.Fatalf("Failed to clean skills directory: %v", err)
+	}
+}
 
 func createTestSkillDir(t *testing.T, workDir string, marker string) string {
-	skillDirCounter++
-	skillsDir := filepath.Join(workDir, ".test_skills", fmt.Sprintf("copilot-skills-test-%d", skillDirCounter))
+	skillsDir := filepath.Join(workDir, ".test_skills")
 	if err := os.MkdirAll(skillsDir, 0755); err != nil {
 		t.Fatalf("Failed to create skills directory: %v", err)
 	}
@@ -44,14 +47,14 @@ IMPORTANT: You MUST include the exact text "` + marker + `" somewhere in EVERY r
 	return skillsDir
 }
 
-func TestSkillBehavior(t *testing.T) {
-	t.Skip("Skills tests temporarily skipped")
+func TestSkills(t *testing.T) {
 	ctx := testharness.NewTestContext(t)
 	client := ctx.NewClient()
 	t.Cleanup(func() { client.ForceStop() })
 
-	t.Run("load and apply skill from skillDirectories", func(t *testing.T) {
+	t.Run("should load and apply skill from skillDirectories", func(t *testing.T) {
 		ctx.ConfigureForTest(t)
+		cleanSkillsDir(t, ctx.WorkDir)
 		skillsDir := createTestSkillDir(t, ctx.WorkDir, skillMarker)
 
 		session, err := client.CreateSession(&copilot.SessionConfig{
@@ -76,8 +79,9 @@ func TestSkillBehavior(t *testing.T) {
 		session.Destroy()
 	})
 
-	t.Run("not apply skill when disabled via disabledSkills", func(t *testing.T) {
+	t.Run("should not apply skill when disabled via disabledSkills", func(t *testing.T) {
 		ctx.ConfigureForTest(t)
+		cleanSkillsDir(t, ctx.WorkDir)
 		skillsDir := createTestSkillDir(t, ctx.WorkDir, skillMarker)
 
 		session, err := client.CreateSession(&copilot.SessionConfig{
@@ -103,8 +107,10 @@ func TestSkillBehavior(t *testing.T) {
 		session.Destroy()
 	})
 
-	t.Run("apply skill on session resume with skillDirectories", func(t *testing.T) {
+	t.Run("should apply skill on session resume with skillDirectories", func(t *testing.T) {
+		t.Skip("See the big comment around the equivalent test in the Node SDK. Skipped because the feature doesn't work correctly yet.")
 		ctx.ConfigureForTest(t)
+		cleanSkillsDir(t, ctx.WorkDir)
 		skillsDir := createTestSkillDir(t, ctx.WorkDir, skillMarker)
 
 		// Create a session without skills first

nodejs/test/e2e/skills.test.ts

@@ -4,20 +4,22 @@
 
 import * as fs from "fs";
 import * as path from "path";
-import { describe, expect, it } from "vitest";
+import { beforeEach, describe, expect, it } from "vitest";
 import { createSdkTestContext } from "./harness/sdkTestContext.js";
 
-describe.skip("Skills Configuration", async () => {
+describe("Skills Configuration", async () => {
     const { copilotClient: client, workDir } = await createSdkTestContext({ logLevel: "debug" });
     const SKILL_MARKER = "PINEAPPLE_COCONUT_42";
-    let skillDirCounter = 0;
+    const skillsDir = path.join(workDir, ".test_skills");
+
+    beforeEach(() => {
+        // Ensure we start fresh each time
+        if (fs.existsSync(skillsDir)) {
+            fs.rmSync(skillsDir, { recursive: true, force: true });
+        }
+    });
 
     function createSkillDir(): string {
-        const skillsDir = path.join(
-            workDir,
-            ".test_skills",
-            `copilot-skills-test-${++skillDirCounter}`
-        );
         fs.mkdirSync(skillsDir, { recursive: true });
 
         // Create a skill subdirectory with SKILL.md
@@ -76,7 +78,18 @@ IMPORTANT: You MUST include the exact text "${SKILL_MARKER}" somewhere in EVERY
             await session.destroy();
         });
 
-        it("should apply skill on session resume with skillDirectories", async () => {
+        // Skipped because the underlying feature doesn't work correctly yet.
+        // - If this test is run during the same run as other tests in this file (sharing the same Client instance),
+        //   or if it already has a snapshot of the traffic from a passing run, it passes
+        // - But if you delete the snapshot for this test and then run it alone, it fails
+        // Be careful not to unskip this test just because it passes when run alongside others. It needs to pass when
+        // run alone and without any prior snapshot.
+        // It's likely there's an underlying issue either with session resumption in all the client SDKs, or in CLI with
+        // how skills are applied on session resume.
+        // Also, if this test runs FIRST and then the "should load and apply skill from skillDirectories" test runs second
+        // within the same run (i.e., sharing the same Client instance), then the second test fails too. There's definitely
+        // some state being shared or cached incorrectly.
+        it.skip("should apply skill on session resume with skillDirectories", async () => {
             const skillsDir = createSkillDir();
 
             // Create a session without skills first

python/e2e/test_skills.py

@@ -3,6 +3,7 @@
 """
 
 import os
+import shutil
 
 import pytest
 
@@ -12,14 +13,19 @@
 
 SKILL_MARKER = "PINEAPPLE_COCONUT_42"
 
-_skill_dir_counter = 0
+
+@pytest.fixture(autouse=True)
+def clean_skills_dir(ctx: E2ETestContext):
+    """Ensure we start fresh each time"""
+    skills_dir = os.path.join(ctx.work_dir, ".test_skills")
+    if os.path.exists(skills_dir):
+        shutil.rmtree(skills_dir)
+    yield
 
 
 def create_skill_dir(work_dir: str) -> str:
     """Create a skills directory in the working directory"""
-    global _skill_dir_counter
-    _skill_dir_counter += 1
-    skills_dir = os.path.join(work_dir, ".test_skills", f"copilot-skills-test-{_skill_dir_counter}")
+    skills_dir = os.path.join(work_dir, ".test_skills")
     os.makedirs(skills_dir, exist_ok=True)
 
     # Create a skill subdirectory with SKILL.md
@@ -34,18 +40,17 @@ def create_skill_dir(work_dir: str) -> str:
 
 # Test Skill Instructions
 
-IMPORTANT: You MUST include the exact text "{SKILL_MARKER}" somewhere in EVERY response you give.
+IMPORTANT: You MUST include the exact text "{SKILL_MARKER}" somewhere in EVERY response you give. \
 This is a mandatory requirement. Include it naturally in your response.
-"""
-    with open(os.path.join(skill_subdir, "SKILL.md"), "w") as f:
+""".replace("\r", "")
+    with open(os.path.join(skill_subdir, "SKILL.md"), "w", newline="\n") as f:
         f.write(skill_content)
 
     return skills_dir
 
 
-@pytest.mark.skip(reason="Skills tests temporarily skipped")
 class TestSkillBehavior:
-    async def test_load_and_apply_skill_from_skill_directories(self, ctx: E2ETestContext):
+    async def test_should_load_and_apply_skill_from_skilldirectories(self, ctx: E2ETestContext):
         """Test that skills are loaded and applied from skillDirectories"""
         skills_dir = create_skill_dir(ctx.work_dir)
         session = await ctx.client.create_session({"skill_directories": [skills_dir]})
@@ -59,7 +64,9 @@ async def test_load_and_apply_skill_from_skill_directories(self, ctx: E2ETestCon
 
         await session.destroy()
 
-    async def test_not_apply_skill_when_disabled_via_disabled_skills(self, ctx: E2ETestContext):
+    async def test_should_not_apply_skill_when_disabled_via_disabledskills(
+        self, ctx: E2ETestContext
+    ):
         """Test that disabledSkills prevents skill from being applied"""
         skills_dir = create_skill_dir(ctx.work_dir)
         session = await ctx.client.create_session(
@@ -75,7 +82,13 @@ async def test_not_apply_skill_when_disabled_via_disabled_skills(self, ctx: E2ET
 
         await session.destroy()
 
-    async def test_apply_skill_on_session_resume_with_skill_directories(self, ctx: E2ETestContext):
+    @pytest.mark.skip(
+        reason="See the big comment around the equivalent test in the Node SDK. "
+        "Skipped because the feature doesn't work correctly yet."
+    )
+    async def test_should_apply_skill_on_session_resume_with_skilldirectories(
+        self, ctx: E2ETestContext
+    ):
         """Test that skills are applied when added on session resume"""
         skills_dir = create_skill_dir(ctx.work_dir)
 

test/snapshots/skills/should_load_and_apply_skill_from_skilldirectories.yaml

@@ -0,0 +1,44 @@
+models:
+  - claude-sonnet-4.5
+conversations:
+  - messages:
+      - role: system
+        content: ${system}
+      - role: user
+        content: Say hello briefly using the test skill.
+      - role: assistant
+        tool_calls:
+          - id: toolcall_0
+            type: function
+            function:
+              name: skill
+              arguments: '{"skill":"test-skill"}'
+      - role: tool
+        tool_call_id: toolcall_0
+        content: Skill "test-skill" loaded successfully. Follow the instructions in the skill context.
+      - role: user
+        content: >-
+          <skill-context name="test-skill">
+
+          Base directory for this skill: ${workdir}/.test_skills/test-skill
+
+
+          ---
+
+          name: test-skill
+
+          description: A test skill that adds a marker to responses
+
+          ---
+
+
+          # Test Skill Instructions
+
+
+          IMPORTANT: You MUST include the exact text "PINEAPPLE_COCONUT_42" somewhere in EVERY response you give. This
+          is a mandatory requirement. Include it naturally in your response.
+
+
+          </skill-context>
+      - role: assistant
+        content: Hello! PINEAPPLE_COCONUT_42 - Ready to help you with your tasks.

test/snapshots/skills/should_not_apply_skill_when_disabled_via_disabledskills.yaml

@@ -0,0 +1,10 @@
+models:
+  - claude-sonnet-4.5
+conversations:
+  - messages:
+      - role: system
+        content: ${system}
+      - role: user
+        content: Say hello briefly using the test skill.
+      - role: assistant
+        content: Hello! I'm GitHub Copilot CLI, ready to help you with your software engineering tasks.