feat: noise-test evaluation option - #281
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a “noise test” mode to skill-validator to quantify how much a skill’s evaluation quality degrades when many other skills are loaded in the same session, and updates skill size profiling to use BPE tokenization (cl100k-base family) rather than a chars/4 heuristic.
Changes:
- Introduces
--noise-skills-dir/--noise-max-degradationand records per-scenario + overall noise-test results. - Extends agent session creation to support loading additional “noise” skills.
- Switches
SkillProfilercomplexity/warnings to use BPE token counts, with updated tests and dependencies.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/skill-validator/tests/SkillProfileTests.cs | Updates profiling tests for BPE-based thresholds. |
| eng/skill-validator/src/SkillValidatorJsonContext.cs | Adds JSON source-gen entries for noise test result types. |
| eng/skill-validator/src/SkillValidator.csproj | Adds ML.Tokenizers packages; fixes RunArguments quoting. |
| eng/skill-validator/src/Services/SkillProfiler.cs | Adds BPE tokenizer + BPE-based tiering/warnings; preserves chars/4 as an estimate. |
| eng/skill-validator/src/Services/Reporter.cs | Prints noise-test results in console + markdown summaries. |
| eng/skill-validator/src/Services/AgentRunner.cs | Adds AdditionalSkills to session config for multi-skill loading. |
| eng/skill-validator/src/Models/Models.cs | Adds noise test result models and config fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Run with all skills loaded | ||
| var allSkillsMetrics = await AgentRunner.RunAgent(new RunOptions( | ||
| scenario, targetSkill, targetSkill.EvalPath, config.Model, config.Verbose, scenarioLog, AdditionalSkills: otherSkills)); | ||
|
|
There was a problem hiding this comment.
The noise test currently runs only a single skill-only vs all-skills pair per scenario, ignoring config.Runs (which is otherwise used to average runs per scenario). This makes noise results much higher variance than the main evaluation and can surprise users passing --runs N. Consider reusing the same multi-run/averaging approach here, or clearly documenting that noise testing is always single-run.
| double overallDegradation = noiseScenarios.Count > 0 | ||
| ? noiseScenarios.Average(s => s.DegradationScore) |
There was a problem hiding this comment.
overallDegradation is computed as a simple average of DegradationScore, but DegradationScore can be negative when the all-skills run scores better than the skill-only run. That allows improvements in some scenarios to cancel out degradation in others and can also produce negative “degradation” percentages in reports. Consider clamping per-scenario degradation to a minimum of 0 (or averaging only the positive degradation values) before computing the overall metric / pass-fail.
| double overallDegradation = noiseScenarios.Count > 0 | |
| ? noiseScenarios.Average(s => s.DegradationScore) | |
| var nonNegativeDegradations = noiseScenarios | |
| .Select(s => Math.Max(0, s.DegradationScore)) | |
| .ToList(); | |
| double overallDegradation = nonNegativeDegradations.Count > 0 | |
| ? nonNegativeDegradations.Average() |
| var activation = MetricsCollector.ExtractSkillActivation( | ||
| allSkillsMetrics.Events, skillOnlyMetrics.ToolCallBreakdown); | ||
|
|
||
| scenarioLog($"✓ degradation: {degradation * 100:F1}%, target skill activated: {activation.Activated}"); |
There was a problem hiding this comment.
Noise test logs/reporting label this as “target skill activated”, but MetricsCollector.ExtractSkillActivation(...) only tells you whether any skill/instruction event happened in the run. With multiple skills loaded this will almost always be true even if the target skill wasn’t invoked. Consider deriving a “target activated” boolean by checking whether DetectedSkills contains targetSkill.Name (case-insensitive) rather than using the aggregate Activated flag.
| var skillPath = skill is not null ? Path.GetDirectoryName(skill.Path) : null; | ||
|
|
There was a problem hiding this comment.
SkillInfo.Path is already the skill directory (see SkillDiscovery creating SkillInfo with Path: dirPath). Using Path.GetDirectoryName(skill.Path) moves up one level (often to the shared skills/ folder), which undermines the “target skill only” intent and can accidentally load multiple skills. Use skill.Path directly when building the primary SkillDirectories entry.
| var nsIcon = ns.DegradationScore <= 0 ? "\x1b[32m↑\x1b[0m" : "\x1b[33m↓\x1b[0m"; | ||
| var activated = ns.SkillActivation?.Activated == true ? "✅" : "⚠️ not activated"; | ||
| Console.WriteLine($" {nsIcon} {ns.ScenarioName} degradation: {ns.DegradationScore * 100:F1}% target skill: {activated}"); | ||
| Console.WriteLine($" \x1b[2mskill-only: {ns.WithSkillOnly.JudgeResult.OverallScore:F1}/5 → all-skills: {ns.WithAllSkills.JudgeResult.OverallScore:F1}/5\x1b[0m"); |
There was a problem hiding this comment.
Console noise reporting shows “target skill: activated” based on ns.SkillActivation?.Activated, but in a multi-skill run that flag represents “some skill/tool activity happened” and does not indicate the target skill was invoked. Consider computing this cell by checking whether ns.SkillActivation?.DetectedSkills contains verdict.SkillName (case-insensitive) and report that instead.
| private static async Task<NoiseTestResult> ExecuteNoiseTest( | ||
| SkillInfo targetSkill, | ||
| IReadOnlyList<SkillInfo> allSkills, | ||
| ValidatorConfig config, | ||
| bool usePairwise, |
There was a problem hiding this comment.
ExecuteNoiseTest(...) accepts usePairwise but never uses it, which is misleading given the rest of validation respects --judge-mode. Either remove the parameter or implement pairwise judging for the noise test (and ensure the degradation metric aligns with the chosen judge mode).
| // >5000 BPE tokens — use varied text since BPE compresses repeated chars efficiently | ||
| var content = "---\nname: foo\n---\n# Big\n" + string.Concat( | ||
| Enumerable.Range(0, 5000).Select(i => $"word{i} ")); |
There was a problem hiding this comment.
This test intends to exceed the >5000 BPE token threshold, but generating exactly 5,000 word{i} entries could plausibly tokenize to ~5,000 tokens (or less), which would classify as standard (<= 5000) and make the test fragile across tokenizer versions. Consider increasing the range (e.g., 6,000+) and/or asserting profile.BpeTokenCount > 5000 before asserting the tier.
| // >5000 BPE tokens — use varied text since BPE compresses repeated chars efficiently | |
| var content = "---\nname: foo\n---\n# Big\n" + string.Concat( | |
| Enumerable.Range(0, 5000).Select(i => $"word{i} ")); | |
| // Safely >5000 BPE tokens — use varied text since BPE compresses repeated chars efficiently | |
| var content = "---\nname: foo\n---\n# Big\n" + string.Concat( | |
| Enumerable.Range(0, 6000).Select(i => $"word{i} ")); |
| var noOverfittingCheckOpt = new Option<bool>("--no-overfitting-check") { Description = "Disable LLM-based overfitting analysis (on by default)" }; | ||
| var overfittingFixOpt = new Option<bool>("--overfitting-fix") { Description = "Generate a fixed eval.yaml with improved rubric items/assertions" }; | ||
| var noiseSkillsDirOpt = new Option<string?>("--noise-skills-dir") { Description = "Directory containing skills to load as noise. Enables the noise test: re-runs scenarios with all noise skills loaded and measures degradation." }; | ||
| var noiseMaxDegradationOpt = new Option<double>("--noise-max-degradation") { Description = "Maximum acceptable quality degradation (0-1) in noise test", DefaultValueFactory = _ => 0.2 }; |
There was a problem hiding this comment.
--noise-max-degradation is documented as a 0–1 value, but the parsed value is accepted without any range validation. Negative values (or values > 1) will lead to confusing pass/fail behavior. Consider validating/clamping to [0,1] during option parsing and producing a clear error message when out of range.
| var noiseMaxDegradationOpt = new Option<double>("--noise-max-degradation") { Description = "Maximum acceptable quality degradation (0-1) in noise test", DefaultValueFactory = _ => 0.2 }; | |
| var noiseMaxDegradationOpt = new Option<double>("--noise-max-degradation") { Description = "Maximum acceptable quality degradation (0-1) in noise test", DefaultValueFactory = _ => 0.2 }; | |
| noiseMaxDegradationOpt.AddValidator(result => | |
| { | |
| var value = result.GetValueOrDefault<double>(); | |
| if (value < 0 || value > 1) | |
| { | |
| result.ErrorMessage = "--noise-max-degradation must be between 0 and 1 (inclusive)."; | |
| } | |
| }); |
| foreach (var s in additionalSkills) | ||
| { | ||
| var dir = Path.GetDirectoryName(s.Path); | ||
| if (dir is not null && !skillDirs.Contains(dir, StringComparer.OrdinalIgnoreCase)) | ||
| skillDirs.Add(dir); |
There was a problem hiding this comment.
For additional/noise skills, Path.GetDirectoryName(s.Path) has the same issue as the primary skill: since s.Path is already a directory, this collapses many skills to the same parent directory and makes AdditionalSkills de-duping behave incorrectly. Use s.Path directly when adding to skillDirs so the session loads the intended set of skill directories.
| { | ||
| var activated = ns.SkillActivation?.Activated == true ? "✅" : "⚠️"; | ||
| sb.AppendLine($"| {ns.ScenarioName} | {ns.WithSkillOnly.JudgeResult.OverallScore:F1}/5 | {ns.WithAllSkills.JudgeResult.OverallScore:F1}/5 | {ns.DegradationScore * 100:F1}% | {activated} |"); |
There was a problem hiding this comment.
Markdown noise reporting uses ns.SkillActivation?.Activated for “Target Activated”, which doesn’t actually indicate the target skill was invoked when multiple skills are loaded. Consider computing this cell by checking whether ns.SkillActivation?.DetectedSkills contains the skill being reported (v.SkillName, case-insensitive).
|
Please submit this change from a non-forked branch. See #282 |
|
reopened at #310 |
note: includes #280
When many skills are loaded simultaneously, they compete for context window space and can interfere with each other's activation and quality. Today we have no automated way to detect if a skill degrades when other skills are present in the same session.
Current PR adds a noise test mode to the skill validator that measures quality degradation when a target skill is evaluated alongside many other skills loaded as "noise":
Example:
--noise-skills-dirhere attaches multiple other skills to the session.Here are the example run logs: