diff --git a/biome.jsonc b/biome.jsonc index f2bf55073a..caf730ceb2 100644 --- a/biome.jsonc +++ b/biome.jsonc @@ -209,6 +209,18 @@ } } }, + { + // Core test files run in Node.js — relax the node:* restriction for tests only. + // Production code in packages/core must never import node:* (enforced above). + "includes": ["packages/core/src/**/*.test.ts"], + "linter": { + "rules": { + "style": { + "noRestrictedImports": "off" + } + } + } + }, { "includes": [ "packages/api-client/src/**/*.ts", diff --git a/packages/core/src/skills/analyzeSkills.test.ts b/packages/core/src/skills/analyzeSkills.test.ts index 2fd1aa5549..db5af1286a 100644 --- a/packages/core/src/skills/analyzeSkills.test.ts +++ b/packages/core/src/skills/analyzeSkills.test.ts @@ -1,6 +1,11 @@ import type { SkillInfo } from "@posthog/shared"; import { describe, expect, it } from "vitest"; -import { analyzeSkills, OVERSIZED_SKILL_MD_BYTES } from "./analyzeSkills"; +import { + AMBIGUOUS_TRIGGER_VERBS, + analyzeSkills, + hasAmbiguousTrigger, + OVERSIZED_SKILL_MD_BYTES, +} from "./analyzeSkills"; function makeSkill(overrides: Partial): SkillInfo { return { @@ -133,6 +138,114 @@ describe("analyzeSkills", () => { }); }); + describe("ambiguous-trigger", () => { + it("flags a bundled skill with an ambiguous verb and no TRIGGER guard", () => { + const skill = makeSkill({ + source: "bundled", + description: "Investigate why a metric dropped using PostHog data.", + }); + + const analysis = analyzeSkills([skill]); + + expect(analysis[skill.path]).toEqual([ + expect.objectContaining({ type: "ambiguous-trigger" }), + ]); + }); + + it.each(["user", "repo", "marketplace", "codex"] as const)( + "does not flag a %s skill — guard requirement is bundled-only", + (source) => { + const skill = makeSkill({ + source, + description: "Investigate why a metric dropped.", + }); + + expect(analyzeSkills([skill])).toEqual({}); + }, + ); + + it("does not flag a bundled skill whose description has a TRIGGER guard", () => { + const skill = makeSkill({ + source: "bundled", + description: + "Investigate the experiment results.\nTRIGGER when: user provides an experiment ID.\nDO NOT TRIGGER when: user is editing code.", + }); + + expect(analyzeSkills([skill])).toEqual({}); + }); + + it.each([ + [ + "Use when", + "Investigates a PostHog error. Use when the user pastes an issue URL.", + ], + [ + "Use whenever", + "Debug PostHog Surveys. Use whenever a support ticket is pasted.", + ], + [ + "Read when", + "Audit PostHog flags. Read when the user asks to health-check flags.", + ], + [ + "TRIGGER when", + "Investigate traces.\nTRIGGER when: user pastes a trace URL.", + ], + [ + "trigger when (lowercase)", + "Investigate traces. ONLY trigger when the user pastes a trace URL.", + ], + ])( + "does not flag a bundled skill with a %s guard", + (_label, description) => { + const skill = makeSkill({ source: "bundled", description }); + expect(analyzeSkills([skill])).toEqual({}); + }, + ); + + it("does not flag a bundled skill with no ambiguous verbs", () => { + const skill = makeSkill({ + source: "bundled", + description: "Run an A/B test experiment in PostHog.", + }); + + expect(analyzeSkills([skill])).toEqual({}); + }); + + it("reports the matched verbs in the message", () => { + const skill = makeSkill({ + source: "bundled", + description: "Debug and diagnose why the dashboard is slow.", + }); + + const [issue] = analyzeSkills([skill])[skill.path] ?? []; + + expect(issue?.message).toContain("debug"); + expect(issue?.message).toContain("diagnose"); + }); + + it("hasAmbiguousTrigger returns false for a skill that is already guarded", () => { + const skill = makeSkill({ + source: "bundled", + description: + "Investigate the issue.\nTRIGGER when: user says investigate.", + }); + + expect(hasAmbiguousTrigger(skill)).toBe(false); + }); + + it("AMBIGUOUS_TRIGGER_VERBS contains at least 5 verbs (sentinel)", () => { + expect(AMBIGUOUS_TRIGGER_VERBS.length).toBeGreaterThanOrEqual(5); + }); + + it.each(["investigate", "debug", "diagnose"] as const)( + "AMBIGUOUS_TRIGGER_VERBS includes the core coding-overlap verb %s", + (verb) => { + expect(AMBIGUOUS_TRIGGER_VERBS).toContain(verb); + }, + ); + }); + it("accumulates multiple issues on one skill", () => { const skill = makeSkill({ name: "Other Name", diff --git a/packages/core/src/skills/analyzeSkills.ts b/packages/core/src/skills/analyzeSkills.ts index f376671240..15c11ea2d4 100644 --- a/packages/core/src/skills/analyzeSkills.ts +++ b/packages/core/src/skills/analyzeSkills.ts @@ -4,7 +4,8 @@ export type SkillIssueType = | "missing-description" | "name-mismatch" | "oversized-manifest" - | "shadowed"; + | "shadowed" + | "ambiguous-trigger"; export interface SkillIssue { type: SkillIssueType; @@ -17,6 +18,36 @@ export type SkillAnalysis = Record; /** SKILL.md is injected into agent context; warn when it gets expensive. */ export const OVERSIZED_SKILL_MD_BYTES = 32 * 1024; +/** + * Verbs that appear in both PostHog-analytics descriptions and everyday coding + * requests. A bundled skill whose description contains one of these without an + * explicit TRIGGER guard risks being invoked when the user is doing unrelated + * coding work — the model picks the closest-matching skill even when none is + * appropriate. + */ +export const AMBIGUOUS_TRIGGER_VERBS = [ + "investigate", + "diagnose", + "debug", + "explore", + "analyze", + "audit", + "instrument", + "set up", +] as const; + +export function hasAmbiguousTrigger(skill: SkillInfo): boolean { + if (skill.source !== "bundled") return false; + const desc = skill.description.toLowerCase(); + const hasVerb = AMBIGUOUS_TRIGGER_VERBS.some((v) => desc.includes(v)); + // Accepted guard patterns (any is sufficient): + // "TRIGGER when(ever)", "Use when(ever)", "Read when(ever)", "DO NOT TRIGGER when" + const hasGuard = /\b(trigger|use|read) when(ever)?\b/i.test( + skill.description, + ); + return hasVerb && !hasGuard; +} + /** * Precedence when two skills share a name: the most specific source wins. * Repo skills beat user skills beat marketplace plugins beat bundled ones. @@ -64,6 +95,16 @@ export function analyzeSkills(skills: SkillInfo[]): SkillAnalysis { }); } + if (hasAmbiguousTrigger(skill)) { + const matched = AMBIGUOUS_TRIGGER_VERBS.filter((v) => + skill.description.toLowerCase().includes(v), + ); + push(skill, { + type: "ambiguous-trigger", + message: `Description uses generic verb(s) [${matched.join(", ")}] without a TRIGGER guard — add "TRIGGER when: …\\nDO NOT TRIGGER when: …" to prevent false invocations on coding tasks`, + }); + } + const dirName = directoryName(skill.path); if (skill.name !== dirName) { push(skill, { diff --git a/packages/core/src/skills/bundled-skill-corpus.test.ts b/packages/core/src/skills/bundled-skill-corpus.test.ts new file mode 100644 index 0000000000..e9e66c531b --- /dev/null +++ b/packages/core/src/skills/bundled-skill-corpus.test.ts @@ -0,0 +1,174 @@ +/** + * Corpus quality tests for the bundled PostHog plugin skills. + * + * These run against the real skill files that ship with the app and assert + * structural properties that prevent overfitting: being steered toward a + * PostHog analytics skill when the user is doing unrelated coding work. + * + * Tests SKIP when the bundled skills directory doesn't exist (fresh checkout + * before `pnpm build`). Once built, every failure names a skill that needs + * a TRIGGER guard in its description. + */ + +import * as fs from "node:fs"; +import * as path from "node:path"; +import type { SkillInfo } from "@posthog/shared"; +import { describe, expect, it } from "vitest"; +import { + AMBIGUOUS_TRIGGER_VERBS, + analyzeSkills, + hasAmbiguousTrigger, +} from "./analyzeSkills"; + +const BUNDLED_SKILLS_DIR = path.resolve( + __dirname, + "../../../../apps/code/.vite/build/plugins/posthog/skills", +); + +function loadBundledSkills(): SkillInfo[] | null { + if (!fs.existsSync(BUNDLED_SKILLS_DIR)) return null; + const entries = fs.readdirSync(BUNDLED_SKILLS_DIR, { withFileTypes: true }); + const skills: SkillInfo[] = []; + + for (const entry of entries) { + if (!entry.isDirectory() && !entry.isSymbolicLink()) continue; + const skillPath = path.join(BUNDLED_SKILLS_DIR, entry.name); + const manifestPath = path.join(skillPath, "SKILL.md"); + if (!fs.existsSync(manifestPath)) continue; + + const content = fs.readFileSync(manifestPath, "utf-8"); + const nameMatch = content.match(/^name:\s*(.+)$/m); + // Handles four YAML description forms: + // folded: description: >\n line\n line + // double: description: "text" + // single: description: 'text' + // plain: description: text on one line + const foldedMatch = content.match( + /^description:\s*>-?\s*\n((?:[ \t]+.+\n?)+)/m, + ); + const quotedMatch = content.match( + /^description:\s*(?:"([\s\S]+?)"|'([\s\S]+?)')\s*$/m, + ); + const plainMatch = content.match(/^description:\s+([^>'"|\n].+)$/m); + const rawDesc = foldedMatch + ? foldedMatch[1] + ?.split("\n") + .map((l) => l.trim()) + .join(" ") + .trim() + : quotedMatch + ? (quotedMatch[1] ?? quotedMatch[2] ?? "").trim() + : (plainMatch?.[1] ?? "").trim(); + + skills.push({ + name: nameMatch?.[1]?.trim() ?? entry.name, + description: rawDesc, + source: "bundled", + path: skillPath, + editable: false, + skillMdBytes: Buffer.byteLength(content, "utf-8"), + }); + } + + return skills.length > 0 ? skills : null; +} + +const skills = loadBundledSkills(); + +describe.skipIf(skills === null)("bundled posthog skill corpus", () => { + // TypeScript cannot narrow `skills` inside the callback even though + // describe.skipIf guarantees the suite only runs when skills !== null. + const corpus = skills as SkillInfo[]; + + it("loads a non-trivial number of skills (sanity check)", () => { + expect(corpus.length).toBeGreaterThan(50); + }); + + it("every skill has a non-empty description", () => { + const missing = corpus.filter((s) => !s.description.trim()); + expect(missing.map((s) => s.name)).toEqual([]); + }); + + /** + * Overfitting guard: every bundled skill whose description uses a + * coding-overlap verb must have an explicit TRIGGER guard. + * + * Without a guard the model sees "investigate why a metric dropped" and + * "investigate why a test is failing" as equally close to the same set + * of skills — picking a PostHog analytics skill for coding work. + * + * Fix: add to the skill's description field — + * TRIGGER when: + * DO NOT TRIGGER when: + */ + it("every ambiguous-verb skill has a TRIGGER guard in its description", () => { + const violations = corpus.filter(hasAmbiguousTrigger); + + if (violations.length > 0) { + const lines = violations.map((s) => { + const matched = AMBIGUOUS_TRIGGER_VERBS.filter((v) => + s.description.toLowerCase().includes(v), + ); + return ` ${s.name} [${matched.join(", ")}]`; + }); + console.error( + `${violations.length} skill(s) need a TRIGGER guard:\n${lines.join("\n")}`, + ); + } + + expect(violations.map((s) => s.name)).toEqual([]); + }); + + /** + * Intent-collision guard: no action verb should appear in more than 8 + * *unguarded* descriptions. Skills that already have a Use-when or TRIGGER + * guard are self-disambiguating — the model picks between them on domain + * specifics. Unguarded skills sharing the same verb give the model no basis + * to choose and cause false invocations in coding sessions. + */ + it("no action verb dominates more than 8 unguarded skill descriptions", () => { + const collisions: string[] = []; + + for (const word of AMBIGUOUS_TRIGGER_VERBS) { + const unguarded = corpus.filter( + (s) => + s.description.toLowerCase().includes(word) && hasAmbiguousTrigger(s), + ); + if (unguarded.length > 8) { + collisions.push( + `"${word}" in ${unguarded.length} unguarded descriptions: ${unguarded.map((s) => s.name).join(", ")}`, + ); + } + } + + if (collisions.length > 0) { + console.error( + `Intent collisions:\n${collisions.map((c) => ` ${c}`).join("\n")}`, + ); + } + + expect(collisions).toEqual([]); + }); + + it("analyzeSkills reports no issues on the full corpus", () => { + const analysis = analyzeSkills(corpus); + const allIssues = Object.entries(analysis).flatMap(([p, issues]) => + issues.map((issue) => ({ + skill: path.basename(p), + type: issue.type, + message: issue.message, + })), + ); + + if (allIssues.length > 0) { + console.error( + "Corpus issues:\n" + + allIssues + .map((i) => ` [${i.type}] ${i.skill}: ${i.message}`) + .join("\n"), + ); + } + + expect(allIssues).toEqual([]); + }); +});