From 3395044cae5d3adce35d33050dc09966c2a12793 Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Thu, 11 Jun 2026 13:07:12 +0100 Subject: [PATCH 1/2] feat(skills): validation and shadowing signals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - analyzeSkills in @posthog/core: pure, no-I/O health analysis over the discovered skills. Rules: missing/empty description, frontmatter name vs directory name mismatch, oversized SKILL.md (context-cost warning, 32 kB threshold), and cross-source name collisions with explicit precedence (repo > user > marketplace > bundled) — losers are marked shadowed by the winner. - SkillInfo gains skillMdBytes (computed during discovery) to feed the size rule. - UI: amber health badge with tooltip on SkillCard; per-issue callouts in the detail panel ("Shadowed by the repository skill named ..."). - Unit tests cover every rule, the precedence matrix, the boundary size, and multi-issue accumulation. Generated-By: PostHog Code Task-Id: f4e84f1a-19c9-490c-9b98-47787a7dddcf --- .../core/src/skills/analyzeSkills.test.ts | 133 ++++++++++++++++++ packages/core/src/skills/analyzeSkills.ts | 104 ++++++++++++++ packages/shared/src/skills.ts | 2 + packages/ui/src/features/skills/SkillCard.tsx | 25 +++- .../src/features/skills/SkillDetailPanel.tsx | 30 +++- .../ui/src/features/skills/SkillsView.tsx | 5 + .../src/services/skills/schemas.ts | 1 + .../services/skills/skill-discovery.test.ts | 3 + .../src/services/skills/skill-discovery.ts | 1 + 9 files changed, 301 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/skills/analyzeSkills.test.ts create mode 100644 packages/core/src/skills/analyzeSkills.ts diff --git a/packages/core/src/skills/analyzeSkills.test.ts b/packages/core/src/skills/analyzeSkills.test.ts new file mode 100644 index 000000000..dbefe3c39 --- /dev/null +++ b/packages/core/src/skills/analyzeSkills.test.ts @@ -0,0 +1,133 @@ +import type { SkillInfo } from "@posthog/shared"; +import { describe, expect, it } from "vitest"; +import { analyzeSkills, OVERSIZED_SKILL_MD_BYTES } from "./analyzeSkills"; + +function makeSkill(overrides: Partial): SkillInfo { + return { + name: "my-skill", + description: "Does a thing", + source: "user", + path: "/home/.claude/skills/my-skill", + editable: true, + skillMdBytes: 1024, + ...overrides, + }; +} + +describe("analyzeSkills", () => { + it("returns no issues for a healthy skill", () => { + expect(analyzeSkills([makeSkill({})])).toEqual({}); + }); + + it.each([ + ["empty description", ""], + ["whitespace description", " "], + ])("flags a missing description: %s", (_label, description) => { + const skill = makeSkill({ description }); + + const analysis = analyzeSkills([skill]); + + expect(analysis[skill.path]).toEqual([ + expect.objectContaining({ type: "missing-description" }), + ]); + }); + + it("flags a frontmatter name that differs from the directory name", () => { + const skill = makeSkill({ + name: "Pretty Name", + path: "/home/.claude/skills/my-skill", + }); + + const analysis = analyzeSkills([skill]); + + expect(analysis[skill.path]).toEqual([ + expect.objectContaining({ type: "name-mismatch" }), + ]); + }); + + it("flags an oversized SKILL.md", () => { + const skill = makeSkill({ skillMdBytes: OVERSIZED_SKILL_MD_BYTES + 1 }); + + const analysis = analyzeSkills([skill]); + + expect(analysis[skill.path]).toEqual([ + expect.objectContaining({ type: "oversized-manifest" }), + ]); + }); + + it("does not flag SKILL.md exactly at the size limit", () => { + const skill = makeSkill({ skillMdBytes: OVERSIZED_SKILL_MD_BYTES }); + + expect(analyzeSkills([skill])).toEqual({}); + }); + + describe("shadowing", () => { + it.each([ + ["repo", "user"], + ["repo", "marketplace"], + ["repo", "bundled"], + ["user", "marketplace"], + ["user", "bundled"], + ["marketplace", "bundled"], + ] as const)("%s shadows %s", (winnerSource, loserSource) => { + const winner = makeSkill({ + source: winnerSource, + path: `/roots/${winnerSource}/my-skill`, + }); + const loser = makeSkill({ + source: loserSource, + path: `/roots/${loserSource}/my-skill`, + }); + + const analysis = analyzeSkills([loser, winner]); + + expect(analysis[winner.path]).toBeUndefined(); + expect(analysis[loser.path]).toEqual([ + expect.objectContaining({ type: "shadowed" }), + ]); + }); + + it("does not flag skills with distinct names", () => { + const a = makeSkill({ name: "alpha", path: "/roots/user/alpha" }); + const b = makeSkill({ + name: "beta", + path: "/roots/repo/beta", + source: "repo", + }); + + expect(analyzeSkills([a, b])).toEqual({}); + }); + + it("marks every loser when three sources collide", () => { + const repo = makeSkill({ source: "repo", path: "/roots/repo/my-skill" }); + const user = makeSkill({ source: "user", path: "/roots/user/my-skill" }); + const bundled = makeSkill({ + source: "bundled", + path: "/roots/bundled/my-skill", + editable: false, + }); + + const analysis = analyzeSkills([bundled, user, repo]); + + expect(analysis[repo.path]).toBeUndefined(); + expect(analysis[user.path]?.[0]?.message).toContain("repository"); + expect(analysis[bundled.path]?.[0]?.message).toContain("repository"); + }); + }); + + it("accumulates multiple issues on one skill", () => { + const skill = makeSkill({ + name: "Other Name", + description: "", + skillMdBytes: OVERSIZED_SKILL_MD_BYTES * 2, + }); + + const analysis = analyzeSkills([skill]); + + expect(analysis[skill.path]?.map((i) => i.type)).toEqual([ + "missing-description", + "name-mismatch", + "oversized-manifest", + ]); + }); +}); diff --git a/packages/core/src/skills/analyzeSkills.ts b/packages/core/src/skills/analyzeSkills.ts new file mode 100644 index 000000000..c1130c326 --- /dev/null +++ b/packages/core/src/skills/analyzeSkills.ts @@ -0,0 +1,104 @@ +import type { SkillInfo, SkillSource } from "@posthog/shared"; + +export type SkillIssueType = + | "missing-description" + | "name-mismatch" + | "oversized-manifest" + | "shadowed"; + +export interface SkillIssue { + type: SkillIssueType; + message: string; +} + +/** Issues keyed by skill path. Paths without issues are absent. */ +export type SkillAnalysis = Record; + +/** SKILL.md is injected into agent context; warn when it gets expensive. */ +export const OVERSIZED_SKILL_MD_BYTES = 32 * 1024; + +/** + * Precedence when two skills share a name: the most specific source wins. + * Repo skills beat user skills beat marketplace plugins beat bundled ones. + */ +const SOURCE_PRECEDENCE: SkillSource[] = [ + "repo", + "user", + "marketplace", + "bundled", +]; + +const SOURCE_LABEL: Record = { + repo: "repository", + user: "user", + marketplace: "marketplace", + bundled: "bundled", +}; + +function precedence(source: SkillSource): number { + const index = SOURCE_PRECEDENCE.indexOf(source); + return index === -1 ? SOURCE_PRECEDENCE.length : index; +} + +function directoryName(skillPath: string): string { + return skillPath.split(/[/\\]/).filter(Boolean).pop() ?? skillPath; +} + +/** Pure health analysis over the discovered skills. No I/O. */ +export function analyzeSkills(skills: SkillInfo[]): SkillAnalysis { + const analysis: SkillAnalysis = {}; + const push = (skill: SkillInfo, issue: SkillIssue) => { + const issues = analysis[skill.path] ?? []; + issues.push(issue); + analysis[skill.path] = issues; + }; + + for (const skill of skills) { + if (!skill.description.trim()) { + push(skill, { + type: "missing-description", + message: + "No description — agents rely on it to decide when to use this skill", + }); + } + + const dirName = directoryName(skill.path); + if (skill.name !== dirName) { + push(skill, { + type: "name-mismatch", + message: `Frontmatter name "${skill.name}" does not match the directory name "${dirName}"`, + }); + } + + if (skill.skillMdBytes > OVERSIZED_SKILL_MD_BYTES) { + push(skill, { + type: "oversized-manifest", + message: `SKILL.md is ${Math.round(skill.skillMdBytes / 1024)} kB — large manifests add context cost every time the skill is used`, + }); + } + } + + const byName = new Map(); + for (const skill of skills) { + const group = byName.get(skill.name); + if (group) group.push(skill); + else byName.set(skill.name, [skill]); + } + + for (const [name, group] of byName) { + if (group.length < 2) continue; + const sorted = [...group].sort( + (a, b) => precedence(a.source) - precedence(b.source), + ); + const winner = sorted[0]; + if (!winner) continue; + for (const shadowed of sorted.slice(1)) { + push(shadowed, { + type: "shadowed", + message: `Shadowed by the ${SOURCE_LABEL[winner.source]} skill named "${name}"`, + }); + } + } + + return analysis; +} diff --git a/packages/shared/src/skills.ts b/packages/shared/src/skills.ts index 23b5be5d0..f13cdac49 100644 --- a/packages/shared/src/skills.ts +++ b/packages/shared/src/skills.ts @@ -8,6 +8,8 @@ export interface SkillInfo { repoName?: string; /** Whether the skill lives in a directory we own on the user's behalf. */ editable: boolean; + /** Size of SKILL.md in bytes (context-cost signal). */ + skillMdBytes: number; } export interface SkillFileEntry { diff --git a/packages/ui/src/features/skills/SkillCard.tsx b/packages/ui/src/features/skills/SkillCard.tsx index 840421b44..7dbe6a090 100644 --- a/packages/ui/src/features/skills/SkillCard.tsx +++ b/packages/ui/src/features/skills/SkillCard.tsx @@ -1,6 +1,16 @@ -import { Folder, Package, Storefront, User } from "@phosphor-icons/react"; +import { + Folder, + Package, + Storefront, + User, + Warning, +} from "@phosphor-icons/react"; +import type { + SkillAnalysis, + SkillIssue, +} from "@posthog/core/skills/analyzeSkills"; import type { SkillInfo, SkillSource } from "@posthog/shared"; -import { Badge, Box, Flex, Text } from "@radix-ui/themes"; +import { Badge, Box, Flex, Text, Tooltip } from "@radix-ui/themes"; import { useEffect, useRef } from "react"; export const SOURCE_CONFIG: Record< @@ -28,6 +38,7 @@ interface SkillCardProps { /** When true, scroll this card into view once (used for deep-linked skills). */ scrollIntoView?: boolean; onScrolledIntoView?: () => void; + issues?: SkillIssue[]; } export function SkillCard({ @@ -36,6 +47,7 @@ export function SkillCard({ onClick, scrollIntoView, onScrolledIntoView, + issues = [], }: SkillCardProps) { const config = SOURCE_CONFIG[skill.source]; const Icon = config?.icon ?? Package; @@ -76,6 +88,12 @@ export function SkillCard({ )} + {issues.length > 0 && ( + issue.message).join("\n")}> + + + )} + {skill.repoName && ( {skill.repoName} @@ -92,6 +110,7 @@ interface SkillSectionProps { onSelect: (path: string) => void; scrollToPath: string | null; onScrolledIntoView: () => void; + analysis?: SkillAnalysis; } export function SkillSection({ @@ -101,6 +120,7 @@ export function SkillSection({ onSelect, scrollToPath, onScrolledIntoView, + analysis, }: SkillSectionProps) { return ( @@ -116,6 +136,7 @@ export function SkillSection({ onClick={() => onSelect(skill.path)} scrollIntoView={scrollToPath === skill.path} onScrolledIntoView={onScrolledIntoView} + issues={analysis?.[skill.path]} /> ))} diff --git a/packages/ui/src/features/skills/SkillDetailPanel.tsx b/packages/ui/src/features/skills/SkillDetailPanel.tsx index afaa3e1c8..a72fb9566 100644 --- a/packages/ui/src/features/skills/SkillDetailPanel.tsx +++ b/packages/ui/src/features/skills/SkillDetailPanel.tsx @@ -4,8 +4,10 @@ import { LockSimple, PencilSimple, Trash, + Warning, X, } from "@phosphor-icons/react"; +import type { SkillIssue } from "@posthog/core/skills/analyzeSkills"; import type { SkillInfo } from "@posthog/shared"; import { CodeMirrorEditor } from "@posthog/ui/features/code-editor/components/CodeMirrorEditor"; import { MarkdownRenderer } from "@posthog/ui/features/editor/components/MarkdownRenderer"; @@ -16,6 +18,7 @@ import { Badge, Box, Button, + Callout, Dialog, Flex, ScrollArea, @@ -44,9 +47,14 @@ function stripFrontmatter(content: string): string { interface SkillDetailPanelProps { skill: SkillInfo; onClose: () => void; + issues?: SkillIssue[]; } -export function SkillDetailPanel({ skill, onClose }: SkillDetailPanelProps) { +export function SkillDetailPanel({ + skill, + onClose, + issues = [], +}: SkillDetailPanelProps) { const config = SOURCE_CONFIG[skill.source]; const [selectedFile, setSelectedFile] = useState("SKILL.md"); @@ -224,6 +232,26 @@ export function SkillDetailPanel({ skill, onClose }: SkillDetailPanelProps) { )} + + {issues.length > 0 && ( + + {issues.map((issue) => ( + + + + + + {issue.message} + + + ))} + + )} {files.length > 1 && ( diff --git a/packages/ui/src/features/skills/SkillsView.tsx b/packages/ui/src/features/skills/SkillsView.tsx index 590481c75..3eaf3d5ad 100644 --- a/packages/ui/src/features/skills/SkillsView.tsx +++ b/packages/ui/src/features/skills/SkillsView.tsx @@ -1,4 +1,5 @@ import { Lightbulb, MagnifyingGlass, Plus } from "@phosphor-icons/react"; +import { analyzeSkills } from "@posthog/core/skills/analyzeSkills"; import type { SkillInfo, SkillSource } from "@posthog/shared"; import { Box, @@ -69,6 +70,8 @@ export function SkillsView() { setSelectedPath(null); }, []); + const analysis = useMemo(() => analyzeSkills(skills), [skills]); + const grouped = useMemo(() => { const map = new Map(); for (const source of SOURCE_ORDER) { @@ -171,6 +174,7 @@ export function SkillsView() { onSelect={handleSelect} scrollToPath={scrollToPath} onScrolledIntoView={handleScrolledIntoView} + analysis={analysis} /> ); })} @@ -192,6 +196,7 @@ export function SkillsView() { )} diff --git a/packages/workspace-server/src/services/skills/schemas.ts b/packages/workspace-server/src/services/skills/schemas.ts index 8de5c0605..ce21e8ef8 100644 --- a/packages/workspace-server/src/services/skills/schemas.ts +++ b/packages/workspace-server/src/services/skills/schemas.ts @@ -9,6 +9,7 @@ export const skillInfo = z.object({ path: z.string(), repoName: z.string().optional(), editable: z.boolean(), + skillMdBytes: z.number(), }); export const listSkillsOutput = z.array(skillInfo); diff --git a/packages/workspace-server/src/services/skills/skill-discovery.test.ts b/packages/workspace-server/src/services/skills/skill-discovery.test.ts index ec267020e..944ae0959 100644 --- a/packages/workspace-server/src/services/skills/skill-discovery.test.ts +++ b/packages/workspace-server/src/services/skills/skill-discovery.test.ts @@ -69,6 +69,9 @@ describe("readSkillMetadataFromDir", () => { path: path.join(skillsDir, "my-skill"), repoName: "my-repo", editable: true, + skillMdBytes: Buffer.byteLength( + "---\nname: Pretty Name\ndescription: Does a thing\n---\nbody", + ), }, ]); }); diff --git a/packages/workspace-server/src/services/skills/skill-discovery.ts b/packages/workspace-server/src/services/skills/skill-discovery.ts index 92421044e..8cbb42a0f 100644 --- a/packages/workspace-server/src/services/skills/skill-discovery.ts +++ b/packages/workspace-server/src/services/skills/skill-discovery.ts @@ -131,6 +131,7 @@ export async function readSkillMetadataFromDir( path: skillPath, ...(repoName ? { repoName } : {}), editable: isEditableSource(source), + skillMdBytes: Buffer.byteLength(content, "utf-8"), } satisfies SkillInfo; } catch { return null; From 108c18ec132e98d6e92bab0ab408930ea4d306bb Mon Sep 17 00:00:00 2001 From: Peter Kirkham Date: Fri, 12 Jun 2026 08:16:28 +0100 Subject: [PATCH 2/2] fix(skills): render one tooltip line per issue, name the winning repo when shadowed Review feedback on #2607: newline-joined tooltip text collapses in HTML, so issues render as separate lines now; shadow messages include the winning repo's name so same-name skills across two open repos are distinguishable. Generated-By: PostHog Code Task-Id: f4e84f1a-19c9-490c-9b98-47787a7dddcf --- packages/core/src/skills/analyzeSkills.test.ts | 18 ++++++++++++++++++ packages/core/src/skills/analyzeSkills.ts | 5 ++++- packages/ui/src/features/skills/SkillCard.tsx | 12 +++++++++++- 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/core/src/skills/analyzeSkills.test.ts b/packages/core/src/skills/analyzeSkills.test.ts index dbefe3c39..2fd1aa554 100644 --- a/packages/core/src/skills/analyzeSkills.test.ts +++ b/packages/core/src/skills/analyzeSkills.test.ts @@ -98,6 +98,24 @@ describe("analyzeSkills", () => { expect(analyzeSkills([a, b])).toEqual({}); }); + it("flags the later repo when two open repos share a skill name", () => { + const first = makeSkill({ + source: "repo", + repoName: "repo-a", + path: "/roots/repo-a/.claude/skills/my-skill", + }); + const second = makeSkill({ + source: "repo", + repoName: "repo-b", + path: "/roots/repo-b/.claude/skills/my-skill", + }); + + const analysis = analyzeSkills([first, second]); + + expect(analysis[first.path]).toBeUndefined(); + expect(analysis[second.path]?.[0]?.message).toContain("repo-a"); + }); + it("marks every loser when three sources collide", () => { const repo = makeSkill({ source: "repo", path: "/roots/repo/my-skill" }); const user = makeSkill({ source: "user", path: "/roots/user/my-skill" }); diff --git a/packages/core/src/skills/analyzeSkills.ts b/packages/core/src/skills/analyzeSkills.ts index c1130c326..76b768f11 100644 --- a/packages/core/src/skills/analyzeSkills.ts +++ b/packages/core/src/skills/analyzeSkills.ts @@ -92,10 +92,13 @@ export function analyzeSkills(skills: SkillInfo[]): SkillAnalysis { ); const winner = sorted[0]; if (!winner) continue; + const winnerLabel = winner.repoName + ? `${SOURCE_LABEL[winner.source]} (${winner.repoName})` + : SOURCE_LABEL[winner.source]; for (const shadowed of sorted.slice(1)) { push(shadowed, { type: "shadowed", - message: `Shadowed by the ${SOURCE_LABEL[winner.source]} skill named "${name}"`, + message: `Shadowed by the ${winnerLabel} skill named "${name}"`, }); } } diff --git a/packages/ui/src/features/skills/SkillCard.tsx b/packages/ui/src/features/skills/SkillCard.tsx index 7dbe6a090..a2a7aed19 100644 --- a/packages/ui/src/features/skills/SkillCard.tsx +++ b/packages/ui/src/features/skills/SkillCard.tsx @@ -89,7 +89,17 @@ export function SkillCard({ {issues.length > 0 && ( - issue.message).join("\n")}> + + {issues.map((issue) => ( + + {issue.message} + + ))} + + } + > )}