feat(FN-1513): regression tests for skill override wiring and resolver fallback semantics

- Added excludedSkillPaths to SkillSelectionResult for tracking explicitly
  disabled skill paths from -prefix patterns
- Extended createSkillsOverrideFromSelection to distinguish disabled
  skills (exist but excluded) from missing skills (not found)
- Added regression tests:
  - Excluded skill paths tracking in resolveSessionSkills
  - Disabled vs missing skill diagnostics distinction
  - Deterministic ordering verification
  - Skills override wiring in createKbAgent
- Updated existing tests to use new interface
This commit is contained in:
gsxdsm
2026-04-14 08:41:37 -07:00
parent 383995a434
commit b6c26d308b
3 changed files with 209 additions and 11 deletions

View File

@@ -788,6 +788,7 @@ describe("createKbAgent", () => {
const selection = {
allowedSkillPaths: new Set(["/path/nonexistent"]),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: true,
};

View File

@@ -131,6 +131,9 @@ describe("resolveSessionSkills", () => {
expect(result.allowedSkillPaths.size).toBe(1);
expect(result.allowedSkillPaths.has("skills/foo/SKILL.md")).toBe(false);
expect(result.allowedSkillPaths.has("skills/bar/SKILL.md")).toBe(true);
// Verify excludedSkillPaths is also populated
expect(result.excludedSkillPaths.size).toBe(1);
expect(result.excludedSkillPaths.has("skills/foo/SKILL.md")).toBe(true);
});
it("exclusion pattern removes previously added entry", () => {
@@ -143,6 +146,24 @@ describe("resolveSessionSkills", () => {
});
expect(result.allowedSkillPaths.size).toBe(0);
expect(result.excludedSkillPaths.size).toBe(1);
expect(result.excludedSkillPaths.has("skills/foo/SKILL.md")).toBe(true);
});
it("tracks excluded paths from multiple exclusion patterns", () => {
const dir = createMockProjectDir({
skills: ["-skills/foo/SKILL.md", "-skills/bar/SKILL.md"],
});
const result = resolveSessionSkills({
projectRootDir: dir,
});
expect(result.filterActive).toBe(true);
expect(result.allowedSkillPaths.size).toBe(0);
expect(result.excludedSkillPaths.size).toBe(2);
expect(result.excludedSkillPaths.has("skills/foo/SKILL.md")).toBe(true);
expect(result.excludedSkillPaths.has("skills/bar/SKILL.md")).toBe(true);
});
});
@@ -158,6 +179,7 @@ describe("resolveSessionSkills", () => {
// Last + wins
expect(result.allowedSkillPaths.has("skills/foo/SKILL.md")).toBe(true);
expect(result.excludedSkillPaths.has("skills/foo/SKILL.md")).toBe(false);
});
it("last entry wins (exclusion after inclusion)", () => {
@@ -171,6 +193,7 @@ describe("resolveSessionSkills", () => {
// Last - wins
expect(result.allowedSkillPaths.has("skills/foo/SKILL.md")).toBe(false);
expect(result.excludedSkillPaths.has("skills/foo/SKILL.md")).toBe(true);
});
});
@@ -367,6 +390,7 @@ describe("createSkillsOverrideFromSelection", () => {
it("returns base unchanged", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(),
excludedSkillPaths: new Set(),
diagnostics: [],
filterActive: false,
};
@@ -391,6 +415,7 @@ describe("createSkillsOverrideFromSelection", () => {
it("filters skills by allowedSkillPaths", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(["/path/foo"]),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: true,
};
@@ -413,6 +438,7 @@ describe("createSkillsOverrideFromSelection", () => {
it("appends warning diagnostic for allowed paths not matching any skill", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(["/path/nonexistent"]),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: true,
};
@@ -436,6 +462,7 @@ describe("createSkillsOverrideFromSelection", () => {
it("checks requested names against discovered skills (case-insensitive)", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: true,
};
@@ -463,6 +490,7 @@ describe("createSkillsOverrideFromSelection", () => {
it("preserves base diagnostics alongside new diagnostics", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(["/path/foo"]),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: true,
};
@@ -488,6 +516,7 @@ describe("createSkillsOverrideFromSelection", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(["/path/nonexistent"]),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: true,
};
@@ -516,6 +545,7 @@ describe("createSkillsOverrideFromSelection", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(["/path/foo"]),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: true,
};
@@ -540,5 +570,132 @@ describe("createSkillsOverrideFromSelection", () => {
consoleErrorSpy.mockRestore();
});
it("produces warning diagnostic for disabled skills (exists but excluded by patterns)", () => {
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
// Simulate a skill that exists but was disabled by project exclusion pattern
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set<string>(),
excludedSkillPaths: new Set(["/path/disabled-skill"]),
diagnostics: [],
filterActive: true,
};
const override = createSkillsOverrideFromSelection(selection, {
sessionPurpose: "executor",
});
// Skill exists in discovered skills but was excluded
const base = {
skills: [
{ name: "disabled-skill", filePath: "/path/disabled-skill", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
],
diagnostics: [],
};
const result = override(base);
// Skill should be filtered out (excluded)
expect(result.skills).toHaveLength(0);
// Should produce warning diagnostic for disabled skill (ResourceDiagnostic only supports warning|error|collision)
expect(result.diagnostics).toHaveLength(1);
expect(result.diagnostics[0].type).toBe("warning");
expect(result.diagnostics[0].message).toContain("disabled");
expect(result.diagnostics[0].message).toContain("disabled-skill");
// Verify logging
expect(consoleErrorSpy).toHaveBeenCalled();
const lastCall = consoleErrorSpy.mock.calls[consoleErrorSpy.mock.calls.length - 1][0] as string;
expect(lastCall).toContain("disabled");
consoleErrorSpy.mockRestore();
});
it("distinguishes missing skills (not found) from disabled skills (excluded) via message content", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(["/path/allowed-skill", "/path/missing-skill"]),
excludedSkillPaths: new Set(["/path/disabled-skill"]),
diagnostics: [],
filterActive: true,
};
const override = createSkillsOverrideFromSelection(selection);
// All three skills exist in discovered skills
const base = {
skills: [
{ name: "allowed-skill", filePath: "/path/allowed-skill", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
{ name: "disabled-skill", filePath: "/path/disabled-skill", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
{ name: "other", filePath: "/path/other", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
],
diagnostics: [],
};
const result = override(base);
// Only "allowed-skill" should pass through (in allowed paths AND not in excluded paths)
// "disabled-skill" is excluded by patterns
// "other" is not in allowed paths
expect(result.skills).toHaveLength(1);
expect(result.skills[0].name).toBe("allowed-skill");
// Should have 2 diagnostics:
// 1. Warning for missing-skill (allowed path not found in discovered skills)
// 2. Warning for disabled-skill (exists but was excluded by patterns)
// Both are "warning" type since ResourceDiagnostic only supports warning|error|collision
// The distinction is made via message content
expect(result.diagnostics).toHaveLength(2);
const missingDiag = result.diagnostics.find(d => d.message.includes("missing-skill"));
expect(missingDiag).toBeDefined();
expect(missingDiag!.message).toContain("missing-skill");
expect(missingDiag!.message).toContain("not found");
const disabledDiag = result.diagnostics.find(d => d.message.includes("disabled-skill"));
expect(disabledDiag).toBeDefined();
expect(disabledDiag!.message).toContain("disabled-skill");
expect(disabledDiag!.message).toContain("disabled");
});
it("returns skills in deterministic order (same input = same output order)", () => {
const selection: SkillSelectionResult = {
allowedSkillPaths: new Set(["/path/c", "/path/b", "/path/a"]),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: true,
};
const override = createSkillsOverrideFromSelection(selection);
// Input order: c, a, b
const base1 = {
skills: [
{ name: "c", filePath: "/path/c", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
{ name: "a", filePath: "/path/a", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
{ name: "b", filePath: "/path/b", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
],
diagnostics: [],
};
// Same input order: c, a, b (should produce same output)
const base2 = {
skills: [
{ name: "c", filePath: "/path/c", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
{ name: "a", filePath: "/path/a", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
{ name: "b", filePath: "/path/b", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false },
],
diagnostics: [],
};
const result1 = override(base1);
const result2 = override(base2);
// Both results should have the same skills in the same order
expect(result1.skills.map(s => s.name)).toEqual(result2.skills.map(s => s.name));
// Order is preserved from input order (deterministic = consistent)
expect(result1.skills.map(s => s.name)).toEqual(["c", "a", "b"]);
});
});
});

View File

@@ -56,6 +56,13 @@ export interface SkillSelectionResult {
*/
allowedSkillPaths: Set<string>;
/**
* Set of skill file paths that were explicitly excluded by project patterns.
* These paths were disabled via -prefix patterns.
* Used by skillsOverride to distinguish "disabled" (exists but excluded) from "missing" (doesn't exist).
*/
excludedSkillPaths: Set<string>;
/**
* Diagnostics about configured/requested skills.
*/
@@ -200,12 +207,13 @@ export function resolveSessionSkills(context: SkillSelectionContext): SkillSelec
if (!hasPatterns && !hasRequestedNames) {
return {
allowedSkillPaths: new Set<string>(),
excludedSkillPaths: new Set<string>(),
diagnostics: [],
filterActive: false,
};
}
// Build allowed set from patterns
// Build allowed and excluded sets from patterns
// Last entry wins for duplicate paths: we track the "final decision" per path
const finalDecisions = new Map<string, boolean>(); // true = allowed, false = excluded
@@ -215,11 +223,14 @@ export function resolveSessionSkills(context: SkillSelectionContext): SkillSelec
finalDecisions.set(path, !isExclusion);
}
// Build allowed set from final decisions
// Build allowed and excluded sets from final decisions
const allowedSet = new Set<string>();
const excludedSet = new Set<string>();
for (const [path, allowed] of finalDecisions) {
if (allowed) {
allowedSet.add(path);
} else {
excludedSet.add(path);
}
}
@@ -260,6 +271,7 @@ export function resolveSessionSkills(context: SkillSelectionContext): SkillSelec
return {
allowedSkillPaths: allowedSet,
excludedSkillPaths: excludedSet,
diagnostics,
filterActive,
};
@@ -274,6 +286,8 @@ export function resolveSessionSkills(context: SkillSelectionContext): SkillSelec
export interface SkillsOverrideOptions {
/** Set of allowed skill paths */
allowedSkillPaths: Set<string>;
/** Set of explicitly excluded skill paths (from -patterns). If not provided, defaults to empty set. */
excludedSkillPaths?: Set<string>;
/** Whether filtering is active */
filterActive: boolean;
/** Requested skill names for diagnostic purposes */
@@ -293,7 +307,7 @@ export function createSkillsOverrideFromSelection(
selection: SkillSelectionResult,
options: Omit<SkillsOverrideOptions, "allowedSkillPaths" | "filterActive"> = {},
): (base: { skills: Skill[]; diagnostics: ResourceDiagnostic[] }) => { skills: Skill[]; diagnostics: ResourceDiagnostic[] } {
const { allowedSkillPaths, filterActive } = selection;
const { allowedSkillPaths, excludedSkillPaths, filterActive } = selection;
const { requestedSkillNames, sessionPurpose } = options;
return (base: { skills: Skill[]; diagnostics: ResourceDiagnostic[] }) => {
@@ -309,28 +323,54 @@ export function createSkillsOverrideFromSelection(
const hasRequestedNames = Boolean(requestedSkillNames && requestedSkillNames.length > 0);
// Filter skills
// Skills must match the inclusion criteria AND not be in the exclusion list
const hasExcluded = excludedSkillPaths.size > 0;
let filteredSkills: Skill[];
if (hasRequestedNames) {
// Filter by requested names (case-insensitive match)
const requestedNamesLower = new Set(requestedSkillNames!.map((n) => n.toLowerCase()));
filteredSkills = base.skills.filter((skill) => requestedNamesLower.has(skill.name.toLowerCase()));
filteredSkills = base.skills.filter(
(skill) => requestedNamesLower.has(skill.name.toLowerCase()) && !excludedSkillPaths.has(skill.filePath)
);
} else if (hasPatterns) {
// Filter by file path
filteredSkills = base.skills.filter((skill) => allowedSkillPaths.has(skill.filePath));
// Filter by file path (in allowed set AND not in excluded set)
filteredSkills = base.skills.filter(
(skill) => allowedSkillPaths.has(skill.filePath) && !excludedSkillPaths.has(skill.filePath)
);
} else if (hasExcluded) {
// Only exclusions set - filter out excluded skills
filteredSkills = base.skills.filter((skill) => !excludedSkillPaths.has(skill.filePath));
} else {
// No filter criteria - this shouldn't happen if filterActive is true
filteredSkills = base.skills;
}
// Build diagnostics for missing skills
// Build diagnostics for missing and disabled skills
const newDiagnostics: ResourceDiagnostic[] = [];
// Check for configured patterns that don't match any discovered skill
// Note: At this point, we have access to base.skills for validation
// Check for excluded paths that DO match a discovered skill (disabled)
// These are skills that exist but were explicitly excluded by project patterns
const purpose = sessionPurpose ? ` [${sessionPurpose}]` : "";
const discoveredPaths = new Set(base.skills.map((s) => s.filePath));
for (const excludedPath of excludedSkillPaths) {
if (discoveredPaths.has(excludedPath)) {
// Skill exists but was disabled by project patterns
// Use "warning" type since ResourceDiagnostic only supports warning|error|collision
newDiagnostics.push({
type: "warning",
message: `Skill at '${excludedPath}' exists but is disabled by project execution settings${purpose}`,
path: excludedPath,
});
}
// If the path doesn't match any discovered skill, it's not a disabled skill - it's just not relevant
}
// Check for configured patterns (allowed paths) that don't match any discovered skill
// Note: At this point, we have access to base.skills for validation
for (const allowedPath of allowedSkillPaths) {
const hasMatch = base.skills.some((skill) => skill.filePath === allowedPath);
if (!hasMatch) {
if (!discoveredPaths.has(allowedPath)) {
// Allowed path doesn't match any discovered skill - this is a missing/invalid pattern
newDiagnostics.push({
type: "warning",
message: `Configured skill pattern '${allowedPath}' not found in discovered skills${purpose}`,