Address Copilot review comments (round 4)

- DESIGN doc: mark Targeted Skill Activation TODO as implemented
- ValidateCommand: fix misleading --plugin-dir comment
- Comparator: set PluginScore to null when no plugin runs exist
- ComparatorTests: copy missing fields in rehydrated ScenarioComparison
- AgentRunner: fix GetPluginClient docstring, route RunAgentCore through it
- AgentRunner: stage isolated skill to temp dir for true single-skill isolation
- dashboard.js: fix triple chart comment to match actual series names
- RunnerTests: update assertions for new isolated skill staging behavior

Author: JanKrivanek

eng/dashboard/dashboard.js

@@ -523,7 +523,7 @@
     }
   }
 
-  // Helper: create a triple line chart (baseline, isolated, plugin)
+  // Helper: create a triple line chart with three series (e.g., Skill / Plugin / Vanilla quality)
   function createTripleChart(container, title, entries, nameA, nameB, nameC, labelA, labelB, labelC, colorA, colorB, colorC) {
     const div = document.createElement('div');
     div.className = 'chart-container';

eng/skill-validator/DESIGN-whole-plugin-loading.md

@@ -0,0 +1,153 @@
+# Design: Load Whole Plugin Instead of Single Skill
+
+## Problem Statement
+
+The skill validator evaluates whether individual skills improve agent performance. Previously, it loaded **only the skill under test** via `SessionConfig.SkillDirectories`. In production, users load entire **plugins** (all skills, agents, MCP servers via `plugin.json`). This meant the validator evaluated skills in unrealistic isolation — cross-skill interactions were never tested.
+
+## Three-Run Evaluation Model
+
+Every skill evaluation performs **three concurrent agent runs**:
+
+| Run Type | Skills Loaded | Client | Purpose |
+|----------|--------------|--------|---------|
+| **Baseline** | None (`SkillDirectories = []`) | No-plugin client | Vanilla agent reference |
+| **Skilled-Isolated** | Only the skill under test (`SkillDirectories = [skillParent]`) | No-plugin client | Isolated skill improvement (original behavior) |
+| **Skilled-Plugin** | Entire plugin via `--plugin-dir` | Per-plugin client | Production-realistic — skill within full plugin context |
+
+The **final verdict** uses `min(skilled-isolated, skilled-plugin)` — a skill passes only if it performs well both in isolation and within its plugin context.
+
+## Architecture
+
+> All file paths below are relative to `eng/skill-validator/src/` unless noted otherwise.
+
+### Plugin Loading via `SkillDirectories` (Manual Enumeration)
+
+~~Plugins were originally loaded via the CLI's `--plugin-dir` flag through `CopilotClientOptions.CliArgs`. However, `--plugin-dir` is **not honored by the SDK**.~~
+
+Instead, plugin skills are loaded **manually**: `BuildSessionConfig` reads `plugin.json`, resolves the plugin's `skills` path, and passes that directory via `SessionConfig.SkillDirectories`. The SDK scans it for subdirectories containing `SKILL.md` files — the same code path used for isolated skill loading, just pointing at the entire plugin's skills directory instead of a single skill's parent.
+
+MCP servers from `plugin.json` are also passed through `SessionConfig.McpServers` (resolved during skill discovery via `FindPluginMcpServers`).
+
+### Shared CopilotClient
+
+Since `--plugin-dir` is not used, **all runs share the same `CopilotClient`**. There is no per-plugin client pool. Baseline, skilled-isolated, and skilled-plugin runs all use `GetSharedClient()`. The difference between run types is entirely in `SessionConfig` (what `SkillDirectories` and `McpServers` are set to).
+
+**File**: `Services/AgentRunner.cs`
+- `GetSharedClient(bool verbose)`: Returns the shared no-plugin client; used by all runs
+- `ResolvePluginSkillDirectories(string pluginRoot)`: Reads `plugin.json`, resolves skills path, returns it for `SkillDirectories`
+- `StopAllClients()`: Stops all clients at shutdown
+- `CaptureGitHubToken()`: Captures token once at startup, passes to all clients via `CopilotClientOptions.GitHubToken`
+
+### Session Config (`BuildSessionConfig`)
+
+**File**: `Services/AgentRunner.cs`
+
+Signature: `BuildSessionConfig(SkillInfo? skill, string? pluginRoot, string model, string workDir, ...)`
+
+| Parameter combination | Run type | `SkillDirectories` | `McpServers` |
+|----------------------|----------|-------------------|-------------|
+| `skill=null, pluginRoot=null` | Baseline | `[]` | `null` |
+| `skill=X, pluginRoot=null` | Skilled-isolated | `[skillPath]` | From skill's `McpServers` |
+| `skill=X, pluginRoot=Y` | Skilled-plugin | `[resolvedSkillsDir]` | From skill's `McpServers` |
+
+`resolvedSkillsDir` is computed by `ResolvePluginSkillDirectories`: reads `plugin.json`, resolves the `skills` path relative to the plugin root, and returns it. The SDK scans this directory for subdirectories containing `SKILL.md`.
+
+Each session gets a unique `ConfigDir` (temp directory) for isolation.
+
+### Permission Checking
+
+**File**: `Services/AgentRunner.cs` — `CheckPermission(request, workDir, skillPath, pluginRoot)`
+
+Allows access to `workDir`, `skillPath`, and `pluginRoot` directories. The `pluginRoot` parameter was added so the agent can read sibling skills from the plugin during plugin runs.
+
+### Orchestration
+
+**File**: `Commands/ValidateCommand.cs`
+
+1. **Discovery**: `SkillDiscovery.GroupSkillsByPlugin(allSkills)` groups skills by plugin root. Skills without a `plugin.json` ancestor are errors.
+2. **Client creation**: Pre-creates a `CopilotClient` per plugin with `--plugin-dir`.
+3. **Execution** (`ExecuteRun`): Launches 3 concurrent `RunAgent` calls, evaluates assertions/constraints on all 3, judges all 3 independently, runs pairwise judge on baseline vs. worse-scoring skilled run.
+4. **Aggregation** (`ExecuteScenario`): Averages N runs per type, computes `min(isolated, plugin)` per-run scores for confidence intervals.
+5. **Verdict** (`EvaluateSkill`): Checks activation in both skilled runs independently — fails if either is not activated.
+
+### Scoring
+
+**File**: `Services/Comparator.cs`
+
+- `CompareScenario` is unchanged — called twice (baseline vs. isolated, baseline vs. plugin)
+- `ComputeVerdict` exposes both `IsolatedScore` and `PluginScore` on `SkillVerdict`
+- Effective `ImprovementScore` is `min(isolated, plugin)` per scenario
+- `ComputeNormalizedGain` uses `min(isolatedQuality, pluginQuality)` for the final gain calculation
+
+### Models
+
+**File**: `Models/Models.cs`
+
+```csharp
+public sealed class ScenarioComparison
+{
+    public required RunResult Baseline { get; init; }
+    public required RunResult SkilledIsolated { get; init; }
+    public RunResult? SkilledPlugin { get; init; }          // nullable for CompareScenario output
+    public double ImprovementScore { get; init; }           // min(isolated, plugin)
+    public double IsolatedImprovementScore { get; init; }
+    public double PluginImprovementScore { get; init; }
+    public SkillActivationInfo? SkillActivationIsolated { get; set; }
+    public SkillActivationInfo? SkillActivationPlugin { get; set; }
+    // ... other fields
+}
+
+public sealed class SkillVerdict
+{
+    public double? IsolatedScore { get; set; }   // avg isolated improvement
+    public double? PluginScore { get; set; }     // avg plugin improvement
+    // ... other fields
+}
+```
+
+### Reporting & Dashboard
+
+**File**: `Services/Reporter.cs` — Console output shows three columns (baseline, isolated, plugin) with an "Effective score: min(...)" summary line.
+
+**File**: `eng/dashboard/dashboard.js` — `createTripleChart` renders 3 lines per skill (gray=baseline, blue=isolated, green=plugin).
+
+**File**: `eng/dashboard/generate-benchmark-data.ps1` — Emits three data series per skill.
+
+## Targeted Skill Activation Detection (Implemented)
+
+**File**: `Services/MetricsCollector.cs`
+
+`ExtractSkillActivation` accepts an optional `targetSkillName` parameter. When set (as it is for both isolated and plugin runs), only events matching the target skill count towards activation — preventing sibling-skill false positives in plugin runs. Callers in `ValidateCommand.ExecuteRun` pass `skill.Name` for both the isolated and plugin activation checks. The `ExtraTools` heuristic remains as a fallback for skills that don't emit `SkillInvokedEvent`.
+
+## Execution Flow
+
+```
+ValidateCommand.Run
+├── DiscoverSkills
+├── GroupSkillsByPlugin → { pluginRoot → (plugin, [skills]) }
+├── GetSharedClient()                         → used by judge & baseline runs
+│
+├── for each skill (parallel by --parallel-skills):
+│   └── EvaluateSkill
+│       └── for each scenario (parallel by --parallel-scenarios):
+│           └── ExecuteScenario
+│               └── for each run (parallel by --parallel-runs):
+│                   ├── RunAgent(baseline)         → shared client, SkillDirectories=[]
+│                   ├── RunAgent(skilled-isolated)  → shared client, SkillDirectories=[skillParent]
+│                   ├── RunAgent(skilled-plugin)    → shared client, SkillDirectories=[pluginSkillsDir]
+│                   ├── Judge all 3 independently
+│                   ├── PairwiseJudge: baseline vs. worse-scoring skilled
+│                   └── Activation check on both skilled runs
+│
+└── StopAllClients + CleanupWorkDirs
+```
+
+## Edge Cases
+
+1. **Standalone skills (no plugin)**: Treated as errors by `GroupSkillsByPlugin`. All skills in this repository are under `plugins/*/skills/`.
+
+2. **Multiple plugins**: Each plugin's skills are resolved independently via `ResolvePluginSkillDirectories`. All share the same client.
+
+3. **Cost**: 4 judge calls per scenario run (baseline + isolated + plugin + pairwise) — 2× the previous cost.
+
+4. **Resource overhead**: Single CLI process shared by all runs.

eng/skill-validator/src/Commands/ValidateCommand.cs

@@ -701,7 +701,7 @@ private static async Task<RunExecutionResult> ExecuteRun(
             // 2. Skilled-isolated: single skill only (current behavior)
             AgentRunner.RunAgent(new RunOptions(scenario, skill, skill.EvalPath, config.Model, config.Verbose,
                 PluginRoot: null, Log: runLog)),
-            // 3. Skilled-plugin: entire plugin loaded via --plugin-dir
+            // 3. Skilled-plugin: load entire plugin from plugin root directory
             AgentRunner.RunAgent(new RunOptions(scenario, skill, skill.EvalPath, config.Model, config.Verbose,
                 PluginRoot: pluginRoot, Log: runLog)));
         var baselineMetrics = agentTasks[0];

eng/skill-validator/src/Services/AgentRunner.cs

@@ -40,11 +40,10 @@ public static void CaptureGitHubToken()
     }
 
     /// <summary>
-    /// Returns a CopilotClient configured for the given plugin.
-    /// The client is created once per plugin root and reused.
-    /// Note: --plugin-dir is NOT honored by the SDK, so all clients share
-    /// the same configuration. Plugin skills are loaded manually via
-    /// SkillDirectories in BuildSessionConfig instead.
+    /// Returns a shared CopilotClient, keyed by plugin root for future
+    /// per-plugin configuration. Currently all clients share the same
+    /// options because --plugin-dir is NOT honored by the SDK; plugin
+    /// skills are loaded via SkillDirectories in BuildSessionConfig.
     /// </summary>
     public static async Task<CopilotClient> GetPluginClient(
         string? pluginRoot, bool verbose)
@@ -91,10 +90,10 @@ public static Task<CopilotClient> GetSharedClient(bool verbose)
     /// <summary>Stop all plugin clients (including the no-plugin client).</summary>
     public static async Task StopAllClients()
     {
-        foreach (var (_, client) in _pluginClients)
+        foreach (var (key, client) in _pluginClients)
         {
             try { await client.StopAsync(); }
-            catch { /* best effort */ }
+            catch (Exception ex) { Console.Error.WriteLine($"Warning: failed to stop client '{key}': {ex.Message}"); }
         }
         _pluginClients.Clear();
     }
@@ -290,8 +289,10 @@ internal static SessionConfig BuildSessionConfig(
 
         // Three run types:
         // 1. Baseline (skill == null, pluginRoot == null): no skills, no MCP.
-        // 2. Skilled-isolated (skill != null, pluginRoot == null): single skill via SkillDirectories.
-        // 3. Skilled-plugin (skill != null, pluginRoot != null): entire plugin loaded manually
+        // 2. Skilled-isolated (skill != null, pluginRoot == null): ONLY the target skill
+        //    is loaded — we stage it into a temp directory so the SDK doesn't
+        //    discover sibling skills from the same parent.
+        // 3. Skilled-plugin (skill != null, pluginRoot != null): entire plugin loaded
         //    via SkillDirectories (--plugin-dir is NOT honored by SDK).
         //
         // For skilled-plugin, we enumerate all skill directories from plugin.json
@@ -303,7 +304,17 @@ internal static SessionConfig BuildSessionConfig(
         }
         else if (skill is not null)
         {
-            skillDirs = [skillPath!];
+            // Stage the single skill into a temp directory so the SDK discovers
+            // only this skill — not every sibling that shares the same parent.
+            var isoStageDir = Path.Combine(Path.GetTempPath(), $"sv-iso-{Guid.NewGuid():N}");
+            Directory.CreateDirectory(isoStageDir);
+            _workDirs.Add(isoStageDir);
+
+            var stagedSkillDir = Path.Combine(isoStageDir, Path.GetFileName(skill.Path));
+            Directory.CreateDirectory(stagedSkillDir);
+            File.WriteAllText(Path.Combine(stagedSkillDir, "SKILL.md"), skill.SkillMdContent);
+
+            skillDirs = [isoStageDir];
         }
         else
         {
@@ -390,9 +401,9 @@ private static async Task<RunMetrics> RunAgentCore(RunOptions options, Cancellat
 
         try
         {
-            // All runs use the shared client — plugin skills are loaded manually
+            // All runs use the same client — plugin skills are loaded manually
             // via SkillDirectories (--plugin-dir is not honored by SDK).
-            var client = await GetSharedClient(options.Verbose);
+            var client = await GetPluginClient(options.PluginRoot, options.Verbose);
 
             await using var session = await client.CreateSessionAsync(
                 BuildSessionConfig(options.Skill, options.PluginRoot, options.Model, workDir, options.Skill?.McpServers, options.AdditionalSkills, options.Log, options.Verbose));

eng/skill-validator/src/Services/Comparator.cs

@@ -138,7 +138,9 @@ public static SkillVerdict ComputeVerdict(
             ConfidenceInterval = ci,
             IsSignificant = significant,
             IsolatedScore = comparisons.Average(c => c.IsolatedImprovementScore),
-            PluginScore = comparisons.Average(c => c.PluginImprovementScore),
+            PluginScore = comparisons.Any(c => c.SkilledPlugin != null)
+                ? comparisons.Where(c => c.SkilledPlugin != null).Average(c => c.PluginImprovementScore)
+                : null,
             Reason = reason,
             FailureKind = passed ? null : "threshold",
         };

eng/skill-validator/tests/ComparatorTests.cs

@@ -216,6 +216,10 @@ public void FailsWhenPluginRunRegressesTaskCompletion()
             SkilledPlugin = MakeRunResult(taskCompleted: false, tokenEstimate: 100, overallScore: 5),
             ImprovementScore = comparison.ImprovementScore,
             Breakdown = comparison.Breakdown,
+            IsolatedImprovementScore = comparison.IsolatedImprovementScore,
+            PluginImprovementScore = comparison.PluginImprovementScore,
+            IsolatedBreakdown = comparison.IsolatedBreakdown,
+            PluginBreakdown = comparison.PluginBreakdown,
         };
 
         var verdict = Comparator.ComputeVerdict(MockSkill, [comparison], 0.0, true);

eng/skill-validator/tests/RunnerTests.cs

@@ -19,11 +19,16 @@ public class BuildSessionConfigTests
         McpServers: null);
 
     [Fact]
-    public void SetsSkillDirectoriesToParentOfSkillPath()
+    public void SetsSkillDirectoriesToStagedIsolationDir()
     {
         var config = AgentRunner.BuildSessionConfig(MockSkill, null, "gpt-4.1", "C:\\tmp\\work");
         Assert.Single(config.SkillDirectories!);
-        Assert.Equal(Path.GetDirectoryName(MockSkill.Path), config.SkillDirectories![0]);
+        // Isolated skills are now staged into a temp directory so the SDK
+        // discovers only the target skill, not siblings.
+        var stageDir = config.SkillDirectories![0];
+        Assert.StartsWith(Path.GetTempPath(), stageDir);
+        var stagedSkillDir = Path.Combine(stageDir, Path.GetFileName(MockSkill.Path));
+        Assert.True(File.Exists(Path.Combine(stagedSkillDir, "SKILL.md")));
     }
 
     [Fact]
@@ -53,9 +58,10 @@ public async Task AdditionalSkillsStageOnlyVerifiedSkillDirs()
             var config = AgentRunner.BuildSessionConfig(MockSkill, pluginRoot: null, "gpt-4.1", "C:\\tmp\\work",
                 additionalSkills: additionalSkills);
 
-            // Primary skill parent + one staging directory for additional skills
+            // Primary skill staged dir + one staging directory for additional skills
             Assert.Equal(2, config.SkillDirectories!.Count);
-            Assert.Equal(Path.GetDirectoryName(MockSkill.Path), config.SkillDirectories[0]);
+            // First dir is the isolated skill staging directory
+            Assert.StartsWith(Path.GetTempPath(), config.SkillDirectories[0]);
 
             var stageDir = config.SkillDirectories[1];
             Assert.StartsWith(Path.GetTempPath(), stageDir);
@@ -311,9 +317,9 @@ public void PluginRootWithPluginJsonResolvesSkillDirectories()
     public void PluginRootNullPreservesSkillDirectories()
     {
         var config = AgentRunner.BuildSessionConfig(MockSkill, null, "gpt-4.1", "C:\\tmp\\work");
-        // Without pluginRoot, SkillDirectories should contain the skill's parent
+        // Without pluginRoot, SkillDirectories should contain the staged isolation dir
         Assert.Single(config.SkillDirectories!);
-        Assert.Equal(Path.GetDirectoryName(MockSkill.Path), config.SkillDirectories![0]);
+        Assert.StartsWith(Path.GetTempPath(), config.SkillDirectories![0]);
     }
 }