From 49b956b46c9005efc32dcd940ac583624591ecbc Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Fri, 31 Jul 2026 16:20:36 +0200 Subject: [PATCH 1/5] feat: add canonical skills system with auto-sync to agent discovery roots --- .agents/shared/hooks/README.md | 4 +- .agents/shared/hooks/inject-rules.mjs | 20 +- .../hooks/inject-rules.test-helpers.mjs | 17 +- .agents/shared/hooks/inject-rules.test.mjs | 49 +-- .agents/shared/metrics/README.md | 2 +- .gitignore | 8 + AGENTS.md | 10 +- package.json | 6 +- pnpm-lock.yaml | 12 + scripts/sync-skills.mjs | 315 ++++++++++++++++++ scripts/sync-skills.test.mjs | 259 ++++++++++++++ skills/README.md | 186 +++++++++++ skills/local/.gitignore | 3 + skills/local/README.md | 22 ++ .../skills => skills/shared}/rules/README.md | 46 ++- .../rules/core-action-decoded-input/SKILL.md | 0 .../shared/rules/dialog-conventions/SKILL.md | 0 .../rules/error-and-monitoring/SKILL.md | 0 .../rules/plugin-slot-registration/SKILL.md | 0 .../shared/rules/plugin-visibility/SKILL.md | 0 .../shared/rules/query-and-cache/SKILL.md | 0 .../shared/rules/rule-authoring/SKILL.md | 6 +- 22 files changed, 898 insertions(+), 67 deletions(-) create mode 100755 scripts/sync-skills.mjs create mode 100644 scripts/sync-skills.test.mjs create mode 100644 skills/README.md create mode 100644 skills/local/.gitignore create mode 100644 skills/local/README.md rename {.agents/shared/skills => skills/shared}/rules/README.md (75%) rename .agents/shared/skills/rules/core-action-decoded-input.md => skills/shared/rules/core-action-decoded-input/SKILL.md (100%) rename .agents/shared/skills/rules/dialog-conventions.md => skills/shared/rules/dialog-conventions/SKILL.md (100%) rename .agents/shared/skills/rules/error-and-monitoring.md => skills/shared/rules/error-and-monitoring/SKILL.md (100%) rename .agents/shared/skills/rules/plugin-slot-registration.md => skills/shared/rules/plugin-slot-registration/SKILL.md (100%) rename .agents/shared/skills/rules/plugin-visibility.md => skills/shared/rules/plugin-visibility/SKILL.md (100%) rename .agents/shared/skills/rules/query-and-cache.md => skills/shared/rules/query-and-cache/SKILL.md (100%) rename .agents/shared/skills/rules/rule-authoring.md => skills/shared/rules/rule-authoring/SKILL.md (90%) diff --git a/.agents/shared/hooks/README.md b/.agents/shared/hooks/README.md index d8d466ca42..78e73327b8 100644 --- a/.agents/shared/hooks/README.md +++ b/.agents/shared/hooks/README.md @@ -8,7 +8,7 @@ In plain English: we want the agent to get the right project-specific advice rig 1. The agent says it is about to edit a file. 2. The loader looks at that file path. -3. It finds any matching rule docs under `.agents/shared/skills/rules/`. +3. It finds any matching rule docs under `skills/shared/rules/`. 4. It bundles those matches into `additionalContext`. 5. The agent-specific adapter hands that context to the runtime in whatever proprietary shape that runtime expects. @@ -42,7 +42,7 @@ If another runtime needs a different wrapper, it should reuse the shared loader This spike is real because all of these are present: -- checked-in shared rules under `.agents/shared/skills/rules/` +- checked-in shared rules under `skills/shared/rules/` - a shared loader that can be run directly from the CLI - a Claude adapter that uses the shared loader - contract tests proving the generic and Claude outputs carry the same rule content diff --git a/.agents/shared/hooks/inject-rules.mjs b/.agents/shared/hooks/inject-rules.mjs index 719d5bc1eb..8290890f8f 100644 --- a/.agents/shared/hooks/inject-rules.mjs +++ b/.agents/shared/hooks/inject-rules.mjs @@ -3,7 +3,7 @@ // roots, matches their `globs` field against the file being edited, // and emits matched rule content in the shape an adapter asks for. // -// Spec: .agents/shared/skills/rules/README.md +// Spec: skills/shared/rules/README.md import { appendFileSync, @@ -62,7 +62,7 @@ export const canonicalize = (path) => { export const DEFAULT_REPO_ROOT = canonicalize( resolve(dirname(fileURLToPath(import.meta.url)), '..', '..', '..'), ); -export const DEFAULT_RULE_ROOTS = ['.agents/shared/skills/rules']; +export const DEFAULT_RULE_ROOTS = ['skills/shared/rules']; export const TRIGGER_TOOLS = new Set(['Edit', 'Write', 'MultiEdit']); export const parseFrontmatter = (content) => { @@ -136,13 +136,17 @@ export const collectRules = ({ continue; } - for (const entry of readdirSync(dir)) { - if (!entry.endsWith('.md') || entry === 'README.md') { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + if (!entry.isDirectory()) { continue; } - const fullPath = join(dir, entry); - const content = readFileSync(fullPath, 'utf8'); + const skillMd = join(dir, entry.name, 'SKILL.md'); + if (!existsSync(skillMd)) { + continue; + } + + const content = readFileSync(skillMd, 'utf8'); const { meta, body } = parseFrontmatter(content); if (meta.kind !== 'rule') { @@ -150,10 +154,10 @@ export const collectRules = ({ } rules.push({ - name: meta.name || entry.replace(/\.md$/, ''), + name: meta.name || entry.name, globs: meta.globs, body, - source: relative(repoRoot, fullPath), + source: relative(repoRoot, skillMd), }); } } diff --git a/.agents/shared/hooks/inject-rules.test-helpers.mjs b/.agents/shared/hooks/inject-rules.test-helpers.mjs index 9db250b452..b236ecaf8b 100644 --- a/.agents/shared/hooks/inject-rules.test-helpers.mjs +++ b/.agents/shared/hooks/inject-rules.test-helpers.mjs @@ -24,6 +24,15 @@ export const registerTmpCleanup = () => { }); }; +/** + * Write a rule-skill into a rules root as //SKILL.md. + */ +export const writeRule = (ruleDir, name, frontmatter, body) => { + const dir = join(ruleDir, name); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, 'SKILL.md'), `---\n${frontmatter}\n---\n${body}`); +}; + export const registerGuardrailsContractSuite = ({ suiteName, buildResult, @@ -37,9 +46,11 @@ export const registerGuardrailsContractSuite = ({ tmp = mkdtempSync(join(tmpdir(), TMP_PREFIX)); ruleDir = join(tmp, 'rules'); mkdirSync(ruleDir); - writeFileSync( - join(ruleDir, 'q.md'), - '---\nname: q\nglobs: src/**/api/**\nkind: rule\n---\nquery rule body', + writeRule( + ruleDir, + 'q', + 'name: q\nglobs: src/**/api/**\nkind: rule', + 'query rule body', ); mkdirSync(join(tmp, 'src', 'shared', 'api'), { recursive: true }); }); diff --git a/.agents/shared/hooks/inject-rules.test.mjs b/.agents/shared/hooks/inject-rules.test.mjs index 6be8886f88..83f807b5b0 100644 --- a/.agents/shared/hooks/inject-rules.test.mjs +++ b/.agents/shared/hooks/inject-rules.test.mjs @@ -34,6 +34,7 @@ import { registerGuardrailsContractSuite, registerTmpCleanup, TMP_PREFIX, + writeRule, } from './inject-rules.test-helpers.mjs'; registerTmpCleanup(); @@ -127,10 +128,12 @@ describe('collectRules', () => { rmSync(tmp, { recursive: true, force: true }); }); - it('discovers rule files with kind: rule', () => { - writeFileSync( - join(ruleDir, 'foo.md'), - '---\nname: foo\nglobs: src/foo/**\nkind: rule\n---\nfoo body', + it('discovers rule skills with kind: rule', () => { + writeRule( + ruleDir, + 'foo', + 'name: foo\nglobs: src/foo/**\nkind: rule', + 'foo body', ); const rules = collectRules({ repoRoot: tmp, ruleRoots: ['rules'] }); assert.equal(rules.length, 1); @@ -139,7 +142,7 @@ describe('collectRules', () => { assert.equal(rules[0].body.trim(), 'foo body'); }); - it('skips README.md', () => { + it('ignores a stray README.md at the rule-root level', () => { writeFileSync( join(ruleDir, 'README.md'), '---\nname: readme\nkind: rule\nglobs: **\n---\nx', @@ -148,12 +151,12 @@ describe('collectRules', () => { assert.equal(rules.length, 0); }); - it('skips files without kind: rule', () => { - writeFileSync( - join(ruleDir, 'cmd.md'), - '---\nname: cmd\nkind: command\nglobs: **\n---\nx', - ); - writeFileSync(join(ruleDir, 'plain.md'), '# no frontmatter'); + it('skips skills without kind: rule', () => { + // A SKILL.md without kind: rule is a workflow skill, not a rule. + writeRule(ruleDir, 'cmd', 'name: cmd\nkind: command\nglobs: **', 'x'); + // A directory without SKILL.md is invisible to the loader. + mkdirSync(join(ruleDir, 'plain')); + writeFileSync(join(ruleDir, 'plain', 'notes.md'), '# no frontmatter'); const rules = collectRules({ repoRoot: tmp, ruleRoots: ['rules'] }); assert.equal(rules.length, 0); }); @@ -169,13 +172,17 @@ describe('collectRules', () => { it('merges across multiple rule roots', () => { const dir2 = join(tmp, 'local'); mkdirSync(dir2); - writeFileSync( - join(ruleDir, 'shared.md'), - '---\nname: shared\nglobs: src/**\nkind: rule\n---\nshared', + writeRule( + ruleDir, + 'shared', + 'name: shared\nglobs: src/**\nkind: rule', + 'shared', ); - writeFileSync( - join(dir2, 'local.md'), - '---\nname: local\nglobs: src/**\nkind: rule\n---\nlocal', + writeRule( + dir2, + 'local', + 'name: local\nglobs: src/**\nkind: rule', + 'local', ); const rules = collectRules({ repoRoot: tmp, @@ -197,9 +204,11 @@ describe('buildRuleMatch', () => { tmp = mkdtempSync(join(tmpdir(), TMP_PREFIX)); ruleDir = join(tmp, 'rules'); mkdirSync(ruleDir); - writeFileSync( - join(ruleDir, 'q.md'), - '---\nname: q\nglobs: src/**/api/**\nkind: rule\n---\nquery rule body', + writeRule( + ruleDir, + 'q', + 'name: q\nglobs: src/**/api/**\nkind: rule', + 'query rule body', ); mkdirSync(join(tmp, 'src', 'shared', 'api'), { recursive: true }); }); diff --git a/.agents/shared/metrics/README.md b/.agents/shared/metrics/README.md index 9d15a9b2bc..21eab1d2ed 100644 --- a/.agents/shared/metrics/README.md +++ b/.agents/shared/metrics/README.md @@ -14,7 +14,7 @@ Telemetry for the rule-skills spike. Answers one question: **is this spike pulli ## Why this exists -The rule-skills system at `.agents/shared/skills/rules/` is a spike. It has a real maintenance cost (loader, frontmatter discipline, contributor education) and a speculative benefit (agents make fewer convention mistakes). We need numbers to know whether the trade is worth it. +The rule-skills system at `skills/shared/rules/` is a spike. It has a real maintenance cost (loader, frontmatter discipline, contributor education) and a speculative benefit (agents make fewer convention mistakes). We need numbers to know whether the trade is worth it. The metrics in this folder answer: diff --git a/.gitignore b/.gitignore index f8440ba3e4..42199e2621 100644 --- a/.gitignore +++ b/.gitignore @@ -65,6 +65,14 @@ next-env.d.ts .agents/** !.agents/shared/ !.agents/shared/** +# generated skill discovery roots (pnpm skills:sync) +.agents/skills/ +.claude/skills/ +.codex/skills/ +.cursor/skills/ +.gemini/skills/ +# skills CLI generated lockfile (regenerated by pnpm skills:sync) +skills-lock.json .playwright-mcp .patches .omp diff --git a/AGENTS.md b/AGENTS.md index 16a254a1a8..38830999c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,7 +46,7 @@ Full wiring conventions (dynamic imports, definitions map, params) live in the ` Two parallel trees, each split into `shared/` (checked in) and `local/` (gitignored): -- `.agents/shared/` — agent-neutral commons: rule-skills, loader, metrics. Consumed by any runtime. +- `.agents/shared/` — agent-neutral commons: guardrails loader, metrics. Rule-skills live at `skills/shared/rules/` (see below). Consumed by any runtime. - `.agents/local/` — IC-personal agent-neutral stuff (drafts, personal skills, metric buffer). - `.claude/shared/` — Claude-specific shared wiring (the adapter hook). Tiny on purpose. - `.claude/` (root) — Claude's required fixed paths: `settings.json` (checked in), `settings.local.json` and `CLAUDE.md` (gitignored, IC-personal). @@ -57,13 +57,13 @@ Gitignore exposes `.agents/shared/**` and `.claude/shared/**` (plus `.claude/set Narrow, prescriptive guardrails scoped by file path. Each rule fires only when the file you're editing matches its `globs` field. -Rules live at `.agents/shared/skills/rules/*.md` — always checked in, never per-IC. A rule that's worth firing on every PR is by definition a shared convention; personal preferences belong in `.claude/CLAUDE.md` or IC settings, not in the rule stream. +Rules live at `skills/shared/rules//SKILL.md` — always checked in, never per-IC. A rule that's worth firing on every PR is by definition a shared convention; personal preferences belong in `.claude/CLAUDE.md` or IC settings, not in the rule stream. -The shared loader lives at `.agents/shared/hooks/inject-rules.mjs`. The rule stream stays agent-agnostic; only the proprietary adapter shape differs. Claude Code consumes it via `.claude/shared/hooks/inject-rules.mjs`. Spec: `.agents/shared/skills/rules/README.md`. +The shared loader lives at `.agents/shared/hooks/inject-rules.mjs`. The rule stream stays agent-agnostic; only the proprietary adapter shape differs. Claude Code consumes it via `.claude/shared/hooks/inject-rules.mjs`. Spec: `skills/shared/rules/README.md`. -In plain English: this is a lazy-loaded guardrails system. Instead of putting every subtle convention in the root prompt, we keep narrow rules in Markdown and load only the ones that match the file being edited. The MVP/POC and its proof live in `.agents/shared/skills/rules/README.md` and `.agents/shared/hooks/README.md`. +In plain English: this is a lazy-loaded guardrails system. Instead of putting every subtle convention in the root prompt, we keep narrow rules in Markdown and load only the ones that match the file being edited. The MVP/POC and its proof live in `skills/shared/rules/README.md` and `.agents/shared/hooks/README.md`. -To author a new rule, copy an existing one in `.agents/shared/skills/rules/` and follow the README — the `rule-authoring` rule-skill auto-injects when you edit anything in that folder. +To author a new rule, copy an existing one in `skills/shared/rules/` and follow the README — the `rule-authoring` rule-skill auto-injects when you edit anything in that folder. Authorship is bottom-up: when a code review surfaces a non-obvious convention, or you catch yourself fixing the same class of mistake more than once, propose a rule-skill update. Don't pre-write rules speculatively. diff --git a/package.json b/package.json index 84133d8f30..b4efb0b452 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,8 @@ "test:changed": "turbo run test:changed", "test:coverage": "pnpm validate:changesets && pnpm test:release-summary && turbo run test:coverage", "test:guardrails": "node --test .agents/shared/hooks/*.test.mjs .claude/shared/hooks/*.test.mjs", - "test:hooks": "pnpm test:guardrails", + "test:skills": "node --test scripts/sync-skills.test.mjs", + "test:hooks": "pnpm test:guardrails && pnpm test:skills", "test:release-summary": "node --test '.github/workflows/scripts/*.test.js'", "validate:changesets": "node .github/workflows/scripts/validateChangesets.js", "guardrails:record": "node .agents/shared/hooks/record-metrics.mjs", @@ -19,6 +20,8 @@ "lint": "turbo run lint", "lint:check": "turbo run lint:check", "type-check": "turbo run type-check", + "skills:sync": "node scripts/sync-skills.mjs", + "postinstall": "pnpm run skills:sync", "prepare": "husky" }, "devDependencies": { @@ -27,6 +30,7 @@ "@changesets/cli": "catalog:", "husky": "catalog:", "lint-staged": "catalog:", + "skills": "1.5.20", "turbo": "catalog:", "ultracite": "catalog:", "vercel": "catalog:", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 713f0b5f0e..742960c25a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -205,6 +205,9 @@ importers: lint-staged: specifier: 'catalog:' version: 17.0.8 + skills: + specifier: 1.5.20 + version: 1.5.20 turbo: specifier: 'catalog:' version: 2.10.5 @@ -7768,6 +7771,11 @@ packages: sisteransi@1.0.5: resolution: {integrity: sha512-bLGGlR1QxBcynn2d5YmDX4MGjlZvy2MRBDRNHLJ8VI6l6+9FUiyTFNJ0IveOSP0bcXgVDPRcfGqA0pjaqUpfVg==} + skills@1.5.20: + resolution: {integrity: sha512-lPl5KzMfTW+qwHFwc8t6R+wAqmdmSHw1+HWbGdJ/FZYbWLdB34bAZNFWiencM5DVoRaKAgXArmfTWMlNAbl9Gg==} + engines: {node: '>=22.20.0'} + hasBin: true + slash@3.0.0: resolution: {integrity: sha512-g9Q1haeby36OSStwb4ntCGGGaKsaVSjQ68fBxoQcutl5fS1vuY18H3wSt3jFyFtrkx+Kz0V1G85A4MyAdDMi2Q==} engines: {node: '>=8'} @@ -17446,6 +17454,10 @@ snapshots: sisteransi@1.0.5: {} + skills@1.5.20: + dependencies: + yaml: 2.9.0 + slash@3.0.0: {} slice-ansi@7.1.2: diff --git a/scripts/sync-skills.mjs b/scripts/sync-skills.mjs new file mode 100755 index 0000000000..fc7c7aa41b --- /dev/null +++ b/scripts/sync-skills.mjs @@ -0,0 +1,315 @@ +#!/usr/bin/env node +// Canonical skills synchronization. +// +// Discovers skills under skills/shared/*/SKILL.md and skills/local/*/SKILL.md, +// installs them (copy mode, non-interactive) into the agent discovery roots +// supported by the pinned `skills` CLI, then validates the generated filesystem. +// +// Skips: CI=1, SKIP_SKILLS_SYNC=1, or missing devDependency (intentional omit). +// Never modifies canonical source files. Idempotent. Cross-platform. + +import { execFileSync } from 'node:child_process'; +import { + existsSync, + readdirSync, + readFileSync, + rmSync, + statSync, +} from 'node:fs'; +import { dirname, join, relative, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const repoRoot = resolve(__dirname, '..'); +const skillsRoot = join(repoRoot, 'skills'); +const sharedDir = join(skillsRoot, 'shared'); +const localDir = join(skillsRoot, 'local'); + +const GENERATED_ROOTS = [ + join(repoRoot, '.agents', 'skills'), + join(repoRoot, '.claude', 'skills'), +]; + +const AGENT_FLAGS = [ + '-a', + 'codex', + '-a', + 'claude-code', + '-a', + 'cursor', + '-a', + 'gemini-cli', +]; + +// --- skip conditions --- + +if (process.env.CI === '1' || process.env.CI === 'true') { + console.log('[skills] CI detected — skipping sync.'); + process.exit(0); +} +if (process.env.SKIP_SKILLS_SYNC === '1') { + console.log('[skills] SKIP_SKILLS_SYNC=1 — skipping sync.'); + process.exit(0); +} + +// --- locate the pinned CLI binary --- + +const cliBin = join(repoRoot, 'node_modules', '.bin', 'skills'); +if (!existsSync(cliBin)) { + console.log( + '[skills] skills CLI not installed (devDependency omitted) — skipping sync.', + ); + process.exit(0); +} + +// --- discovery --- + +/** + * Enumerate direct child skill directories under a catalog dir. + * Returns [{ name, dir, catalog }] where dir contains SKILL.md. + * Rejects category-level SKILL.md (SKILL.md directly under shared/ or local/). + */ +function discoverCatalog(catalogDir, catalogName) { + const skills = []; + if (!existsSync(catalogDir)) { + return skills; + } + + // Reject a category-level SKILL.md — it would be discovered as a skill + // named "shared" or "local", which violates the catalog contract. + const categorySkill = join(catalogDir, 'SKILL.md'); + if (existsSync(categorySkill)) { + console.error( + `[skills] ERROR: found SKILL.md directly under skills/${catalogName}/ — categories must not be skills.`, + ); + process.exit(1); + } + + for (const entry of readdirSync(catalogDir, { withFileTypes: true })) { + if (!entry.isDirectory()) { + continue; + } + const skillDir = join(catalogDir, entry.name); + const skillMd = join(skillDir, 'SKILL.md'); + if (!existsSync(skillMd)) { + continue; + } + + // Validate frontmatter name matches directory. + const parsed = parseFrontmatter(skillMd); + if (!parsed?.name) { + console.error( + `[skills] ERROR: skills/${catalogName}/${entry.name}/SKILL.md missing required frontmatter field "name".`, + ); + process.exit(1); + } + if (parsed.name !== entry.name) { + console.error( + `[skills] ERROR: skills/${catalogName}/${entry.name}/SKILL.md frontmatter name "${parsed.name}" does not match directory "${entry.name}".`, + ); + process.exit(1); + } + if (!parsed.description) { + console.error( + `[skills] ERROR: skills/${catalogName}/${entry.name}/SKILL.md missing required frontmatter field "description".`, + ); + process.exit(1); + } + + skills.push({ name: entry.name, dir: skillDir, catalog: catalogName }); + } + return skills; +} + +function parseFrontmatter(skillMdPath) { + const content = readFileSync(skillMdPath, 'utf-8'); + const match = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); + if (!match) { + return null; + } + const yaml = match[1]; + const nameMatch = yaml.match(/^name:\s*(.+)$/m); + const descMatch = yaml.match(/^description:\s*(.+)$/m); + return { + name: nameMatch?.[1]?.trim().replace(/^['"]|['"]$/g, '') || null, + description: descMatch?.[1]?.trim() || null, + }; +} + +const sharedSkills = discoverCatalog(sharedDir, 'shared'); +const rulesSkills = discoverCatalog(join(sharedDir, 'rules'), 'shared/rules'); +const localSkills = discoverCatalog(localDir, 'local'); +// --- duplicate name check across catalogs --- + +const allSkills = [...sharedSkills, ...rulesSkills, ...localSkills]; +const seenNames = new Set(); +for (const skill of allSkills) { + if (seenNames.has(skill.name)) { + const holders = allSkills + .filter((s) => s.name === skill.name) + .map((s) => `skills/${s.catalog}/${s.name}`); + console.error( + `[skills] ERROR: duplicate skill name "${skill.name}" found in: ${holders.join(', ')}. Skill names must be unique across shared and local.`, + ); + process.exit(1); + } + seenNames.add(skill.name); +} + +if (allSkills.length === 0) { + console.log('[skills] No skills found — nothing to sync.'); + process.exit(0); +} + +console.log( + `[skills] Discovered ${allSkills.length} skill(s): ${allSkills.map((s) => s.name).join(', ')}`, +); + +// --- clean generated roots before install (idempotent reconciliation) --- + +// Remove any existing generated skill directories so the CLI install is clean +// and stale skills are reconciled. Only remove directories that contain a +// SKILL.md (to avoid nuking unrelated content an agent may have placed). +for (const root of GENERATED_ROOTS) { + if (!existsSync(root)) { + continue; + } + for (const entry of readdirSync(root, { withFileTypes: true })) { + if (!entry.isDirectory()) { + continue; + } + const staleSkillMd = join(root, entry.name, 'SKILL.md'); + if (existsSync(staleSkillMd)) { + rmSync(join(root, entry.name), { recursive: true, force: true }); + } + } +} + +// --- install via pinned CLI (copy mode, non-interactive) --- + +// The CLI discovers skills under the given path via --full-depth and installs +// all of them (--skill '*') to the configured agents. --copy avoids symlinks +// into the gitignored .agents/skills/ canonical store. +try { + const args = [ + 'add', + skillsRoot, + '--full-depth', + '--skill', + '*', + ...AGENT_FLAGS, + '--copy', + '--yes', + ]; + console.log(`[skills] Running: skills ${args.join(' ')}`); + execFileSync(cliBin, args, { cwd: repoRoot, stdio: 'inherit' }); +} catch { + console.error('[skills] ERROR: skills CLI failed.'); + process.exit(1); +} + +// --- validate generated filesystem --- + +/** + * Validate that every canonical skill exists at each generated root with a + * SKILL.md, the directory name matches the skill name, and categories were + * flattened (no shared/ or local/ subdir in the generated root). + */ +function validateRoot(root) { + if (!existsSync(root)) { + // CLI only writes to roots for detected agents; missing root is not + // necessarily an error, but we log it. + console.log( + `[skills] Note: generated root ${relative(repoRoot, root)} does not exist (agent may not be detected).`, + ); + return; + } + + // Flattening check: no shared/ or local/ inside generated root. + for (const category of ['shared', 'local']) { + const categoryDir = join(root, category); + if (existsSync(categoryDir)) { + console.error( + `[skills] ERROR: category directory "${category}" found in generated root ${relative(repoRoot, root)} — categories were not flattened.`, + ); + process.exit(1); + } + } + + for (const skill of allSkills) { + const generatedDir = join(root, skill.name); + const generatedSkillMd = join(generatedDir, 'SKILL.md'); + if (!existsSync(generatedDir)) { + console.error( + `[skills] ERROR: skill "${skill.name}" missing from generated root ${relative(repoRoot, root)}.`, + ); + process.exit(1); + } + if (!existsSync(generatedSkillMd)) { + console.error( + `[skills] ERROR: SKILL.md missing for skill "${skill.name}" in ${relative(repoRoot, generatedDir)}.`, + ); + process.exit(1); + } + + // Validate supporting files were preserved (skip README.md which the CLI excludes). + const sourceEntries = readdirSync(skill.dir, { withFileTypes: true }) + .filter((e) => e.name !== 'SKILL.md' && e.name !== 'README.md') + .map((e) => e.name); + for (const entry of sourceEntries) { + const generatedEntry = join(generatedDir, entry); + if (!existsSync(generatedEntry)) { + console.error( + `[skills] ERROR: supporting file "${entry}" for skill "${skill.name}" missing in ${relative(repoRoot, generatedDir)}.`, + ); + process.exit(1); + } + } + } +} + +for (const root of GENERATED_ROOTS) { + validateRoot(root); +} + +// --- executable permissions check --- + +for (const skill of allSkills) { + const scriptsDir = join(skill.dir, 'scripts'); + if (!existsSync(scriptsDir)) { + continue; + } + for (const entry of readdirSync(scriptsDir, { withFileTypes: true })) { + if (!entry.isFile()) { + continue; + } + const scriptPath = join(scriptsDir, entry.name); + const sourceMode = statSync(scriptPath).mode; + const isExecutable = (sourceMode & 0o111) !== 0; + if (!isExecutable) { + continue; + } + // Verify the executable bit survived in generated copies. + for (const root of GENERATED_ROOTS) { + const generatedScript = join( + root, + skill.name, + 'scripts', + entry.name, + ); + if (existsSync(generatedScript)) { + const genMode = statSync(generatedScript).mode; + if ((genMode & 0o111) === 0) { + console.error( + `[skills] ERROR: executable bit lost on ${relative(repoRoot, generatedScript)}.`, + ); + process.exit(1); + } + } + } + } +} + +console.log( + `[skills] Sync complete: ${allSkills.length} skill(s) installed to ${GENERATED_ROOTS.map((r) => relative(repoRoot, r)).join(', ')}.`, +); diff --git a/scripts/sync-skills.test.mjs b/scripts/sync-skills.test.mjs new file mode 100644 index 0000000000..25a56ed6d2 --- /dev/null +++ b/scripts/sync-skills.test.mjs @@ -0,0 +1,259 @@ +// Skills system contract tests. +// +// Validates the canonical skills layout, frontmatter, name-directory +// consistency, flattening, and git-ignore contract. Run via: +// node --test scripts/sync-skills.test.mjs +// pnpm test:skills + +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import { existsSync, readdirSync, readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { test } from 'node:test'; + +const repoRoot = join(new URL('.', import.meta.url).pathname, '..'); +const skillsRoot = join(repoRoot, 'skills'); +const sharedDir = join(skillsRoot, 'shared'); +const localDir = join(skillsRoot, 'local'); +const rulesDir = join(sharedDir, 'rules'); +const GENERATED_ROOTS = [ + join(repoRoot, '.agents', 'skills'), + join(repoRoot, '.claude', 'skills'), +]; + +/** + * Parse YAML frontmatter from a SKILL.md file. + * Returns { name, description } or null if no frontmatter. + */ +function parseFrontmatter(skillMdPath) { + const content = readFileSync(skillMdPath, 'utf-8'); + const match = content.match(/^---\r?\n([\s\S]*?)\r?\n---/); + if (!match) { + return null; + } + const yaml = match[1]; + const nameMatch = yaml.match(/^name:\s*(.+)$/m); + const descMatch = yaml.match(/^description:\s*(.+)$/m); + return { + name: nameMatch?.[1]?.trim().replace(/^['"]|['"]$/g, '') || null, + description: descMatch?.[1]?.trim() || null, + }; +} + +/** + * Enumerate skill directories under a catalog dir. + */ +function discoverSkills(catalogDir) { + if (!existsSync(catalogDir)) { + return []; + } + return readdirSync(catalogDir, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .filter((e) => existsSync(join(catalogDir, e.name, 'SKILL.md'))) + .map((e) => ({ name: e.name, dir: join(catalogDir, e.name) })); +} + +const sharedSkills = discoverSkills(sharedDir); +const rulesSkills = discoverSkills(rulesDir); +const localSkills = discoverSkills(localDir); +const allSkills = [...sharedSkills, ...rulesSkills, ...localSkills]; + +// --- directory structure --- + +test('skills/shared/ exists with the rules sub-catalog', () => { + assert.ok(existsSync(sharedDir), 'skills/shared/ must exist'); + assert.ok( + existsSync(rulesDir), + 'skills/shared/rules/ must exist (rule-skills are shared)', + ); + assert.ok( + rulesSkills.length > 0, + 'skills/shared/rules/ must contain at least one rule-skill', + ); +}); + +test('skills/local/ exists with .gitignore and README.md', () => { + assert.ok(existsSync(localDir), 'skills/local/ must exist'); + assert.ok( + existsSync(join(localDir, '.gitignore')), + 'skills/local/.gitignore must exist', + ); + assert.ok( + existsSync(join(localDir, 'README.md')), + 'skills/local/README.md must exist', + ); +}); + +test('no category-level SKILL.md directly under shared/, shared/rules/, or local/', () => { + for (const dir of [sharedDir, rulesDir, localDir]) { + assert.ok( + !existsSync(join(dir, 'SKILL.md')), + `SKILL.md must not exist directly under ${dir}`, + ); + } +}); + +test('no empty agents/, references/, scripts/, assets/, or evals/ directories', () => { + for (const skill of allSkills) { + for (const subdir of [ + 'agents', + 'references', + 'scripts', + 'assets', + 'evals', + ]) { + const dir = join(skill.dir, subdir); + if (existsSync(dir)) { + const entries = readdirSync(dir); + assert.ok( + entries.length > 0, + `${skill.name}/${subdir}/ must not be empty`, + ); + } + } + } +}); + +// --- frontmatter validation --- + +test('every skill has valid frontmatter with name and description', () => { + for (const skill of allSkills) { + const skillMd = join(skill.dir, 'SKILL.md'); + const parsed = parseFrontmatter(skillMd); + assert.ok(parsed, `${skill.name}/SKILL.md must have frontmatter`); + assert.ok(parsed.name, `${skill.name}/SKILL.md must have a name field`); + assert.ok( + parsed.description, + `${skill.name}/SKILL.md must have a description field`, + ); + } +}); + +test('frontmatter name matches containing directory', () => { + for (const skill of allSkills) { + const parsed = parseFrontmatter(join(skill.dir, 'SKILL.md')); + assert.equal( + parsed.name, + skill.name, + `${skill.name}/SKILL.md frontmatter name must match directory`, + ); + } +}); + +// --- name uniqueness --- + +test('skill names are unique across shared, rules, and local', () => { + const names = allSkills.map((s) => s.name); + const duplicates = names.filter((n, i) => names.indexOf(n) !== i); + assert.deepEqual( + duplicates, + [], + `duplicate skill names: ${duplicates.join(', ')}`, + ); +}); + +// --- generated roots (only if sync has run) --- + +test('generated roots are flattened (no shared/ or local/ subdir)', { + skip: !GENERATED_ROOTS.some(existsSync) ? 'sync has not run' : undefined, +}, () => { + for (const root of GENERATED_ROOTS) { + if (!existsSync(root)) { + continue; + } + for (const category of ['shared', 'local']) { + const categoryDir = join(root, category); + assert.ok( + !existsSync(categoryDir), + `${category}/ must not exist in generated root ${root}`, + ); + } + } +}); + +test('every canonical skill exists in each generated root', { + skip: !GENERATED_ROOTS.some(existsSync) ? 'sync has not run' : undefined, +}, () => { + for (const root of GENERATED_ROOTS) { + if (!existsSync(root)) { + continue; + } + for (const skill of allSkills) { + const generatedDir = join(root, skill.name); + const generatedSkillMd = join(generatedDir, 'SKILL.md'); + assert.ok( + existsSync(generatedDir), + `skill ${skill.name} missing from ${root}`, + ); + assert.ok( + existsSync(generatedSkillMd), + `SKILL.md missing for ${skill.name} in ${root}`, + ); + } + } +}); + +// --- git-ignore contract --- + +test('generated skill roots are gitignored', () => { + for (const root of ['.agents/skills', '.claude/skills']) { + const result = execFileSync( + 'git', + ['check-ignore', '-v', `${root}/test`], + { + cwd: repoRoot, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }, + ).toString(); + assert.ok(result.includes(root), `${root} must be gitignored`); + } +}); + +test('canonical shared rule-skills are not gitignored', () => { + for (const skill of rulesSkills) { + const skillMd = `skills/shared/rules/${skill.name}/SKILL.md`; + let ignored = false; + try { + execFileSync('git', ['check-ignore', skillMd], { + cwd: repoRoot, + stdio: ['pipe', 'pipe', 'pipe'], + }); + ignored = true; + } catch { + ignored = false; + } + assert.ok(!ignored, `${skillMd} must not be gitignored`); + } +}); + +test('local skill contents are gitignored', () => { + for (const skill of localSkills) { + const skillMd = `skills/local/${skill.name}/SKILL.md`; + const result = execFileSync('git', ['check-ignore', '-v', skillMd], { + cwd: repoRoot, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }).toString(); + assert.ok( + result.includes('skills/local/.gitignore'), + `${skillMd} must be gitignored by skills/local/.gitignore`, + ); + } +}); + +test('local .gitignore and README.md are not gitignored', () => { + for (const file of ['skills/local/.gitignore', 'skills/local/README.md']) { + let ignored = false; + try { + execFileSync('git', ['check-ignore', file], { + cwd: repoRoot, + stdio: ['pipe', 'pipe', 'pipe'], + }); + ignored = true; + } catch { + ignored = false; + } + assert.ok(!ignored, `${file} must not be gitignored`); + } +}); diff --git a/skills/README.md b/skills/README.md new file mode 100644 index 0000000000..b0d2bb01fd --- /dev/null +++ b/skills/README.md @@ -0,0 +1,186 @@ +# Skills + +Canonical, auto-synchronized agent skills for this repository. One source of truth under `skills/`, installed into every supported coding agent's discovery root on `pnpm install`. + +## Layout + +``` +skills/ +├── shared/ # committed, repo-wide skills +│ ├── rules/ # rule-skills — hook-driven guardrails (kind: rule) +│ │ ├── README.md +│ │ └── / +│ │ └── SKILL.md +│ └── / # workflow skills +│ ├── SKILL.md # portable frontmatter + core instructions +│ ├── references/ # detailed specs, long examples (loaded on demand) +│ ├── scripts/ # deterministic helpers, validation tools +│ ├── assets/ # templates, boilerplate, static data +│ └── evals/ # activation tests, output-quality fixtures +├── local/ # private, developer-specific skills (gitignored) +│ ├── .gitignore # keeps everything here untracked except itself + README +│ ├── README.md +│ └── / +│ └── ... +└── README.md # this file +``` + +### Catalog rules + +- `skills/shared/` contains committed, repository-wide skills. +- `skills/local/` contains private developer-specific skills — untracked by Git. +- Each skill has its own directory with a `SKILL.md`. +- `SKILL.md` never sits directly under `skills/shared/` or `skills/local/` — `shared` and `local` are catalog categories, not skill names. +- Skill names must be unique across both catalogs (and the `rules` sub-catalog). +- No empty `references/`, `scripts/`, `assets/`, or `evals/` directories — only create them when they contain files. + +## Two skill families + +### Workflow skills + +Invocable capabilities — the agent selects them based on the task (model-invoked) or the user triggers them explicitly (user-invoked). Installed to agent discovery roots by `pnpm skills:sync`. New workflow skills start in `skills/local/` (dogfooded privately) and are promoted to `skills/shared/` once they earn their keep — committed skills must be repo-portable, not coupled to infrastructure this repository doesn't have. + +### Rule-skills + +Path-scoped guardrails — *constraints* on the agent's work, not invocable capabilities. Discriminated by `kind: rule` in frontmatter. Injected lazily by the PreToolUse hook when the edited file matches the rule's `globs` field. The hook loader reads them directly from `skills/shared/rules//SKILL.md`; the CLI sync also installs them to the generated roots so agents that discover `SKILL.md` files can enumerate them. + +Spec: `skills/shared/rules/README.md`. + +## SKILL.md + +Portable discovery metadata and core instructions. Frontmatter: + +```yaml +--- +name: skill-name +description: Describe what the skill does and when an agent should use it. +--- +``` + +- `name` must match the containing directory (kebab-case). +- `description` must be specific enough to support reliable activation — include realistic language users are likely to use. +- Keep portable metadata and core instructions in `SKILL.md`. +- Do not move all frontmatter into `agents/`. + +Rule-skills add two required fields: + +```yaml +globs: apps/app/src/**/api/** +kind: rule +``` + +## agents/ + +Provider-specific machine configuration lives under `/agents/`. Only create files a supported host officially consumes — do not invent speculative manifests. + +## Progressive disclosure + +Keep in `SKILL.md`: core workflow, essential rules, inputs/outputs, decision points, completion criteria, direct links to supporting material. + +Move to `references/`: detailed specifications, long examples, schemas, domain docs, conditional guidance — material that should only enter context when needed. Link supporting files directly from `SKILL.md`; avoid deep chains and duplicated instructions. + +Move to `scripts/`: deterministic transformations, validation tools, executable helpers. Keep script output concise and agent-readable. + +Move to `assets/`: templates, boilerplate, static data, files intended to be copied or modified as output. + +Move to `evals/`: activation test cases, expected outcomes, input fixtures, critical output-quality checks. + +Do not split content merely to populate directories. + +## Invocation behavior + +Two classes: + +- **Model-invoked** — the agent may select it based on the task. Write descriptions with clear activation conditions and realistic user language. +- **User-invoked** — runs only through an explicit command or request. Keep descriptions concise; configure explicit-only invocation where the host supports it. + +## Synchronization + +`pnpm install` triggers `postinstall` → `pnpm skills:sync`, which discovers all workflow skills under `skills/shared/*/SKILL.md`, rule-skills under `skills/shared/rules/*/SKILL.md`, and local skills under `skills/local/*/SKILL.md`, then installs them (copy mode, non-interactive) into the agent discovery roots. + +```bash +pnpm install # installs deps + syncs skills +pnpm skills:sync # re-sync without installing deps +``` + +### Skip + +```bash +SKIP_SKILLS_SYNC=1 pnpm install # skip sync +pnpm install --ignore-scripts # skips all lifecycle scripts including postinstall +``` + +On Windows, set the env var in your shell: `set SKIP_SKILLS_SYNC=1 && pnpm install` (cmd) or `$env:SKIP_SKILLS_SYNC=1; pnpm install` (PowerShell). + +Sync also skips when `CI` is set or when the `skills` devDependency is intentionally omitted. + +### Generated discovery roots + +The pinned `skills` CLI installs to these project-level roots: + +| Root | Agents | +|---|---| +| `.agents/skills/` | universal store (Codex, Cursor, Gemini CLI, and others that read `.agents/skills/`) | +| `.claude/skills/` | Claude Code | + +All generated roots are gitignored — do not commit generated skill copies and do not edit them. They are reproducible from the canonical source under `skills/`. + +### What the wrapper does + +`scripts/sync-skills.mjs`: + +1. Discovers workflow skills under `skills/shared/*/SKILL.md`, rule-skills under `skills/shared/rules/*/SKILL.md`, and local skills under `skills/local/*/SKILL.md`. +2. Rejects duplicate skill names across all catalogs. +3. Rejects category-level `SKILL.md` files. +4. Validates frontmatter `name` matches the directory and `description` is present. +5. Reconciles stale generated skills (removes generated dirs with no canonical source). +6. Installs all skills via the pinned CLI with `--copy --yes --full-depth` to Codex, Claude Code, Cursor, and Gemini CLI. +7. Validates the generated filesystem: every canonical skill exists at each root, `SKILL.md` present, supporting files preserved, categories flattened, executable bits retained. +8. Fails on any inconsistency even if the CLI reported success. + +## Adding a new skill + +### Shared workflow skill + +```sh +mkdir skills/shared/my-skill +# create skills/shared/my-skill/SKILL.md with name + description frontmatter +pnpm skills:sync +git add skills/shared/my-skill +``` + +### Shared rule-skill + +```sh +mkdir skills/shared/rules/my-rule +# create skills/shared/rules/my-rule/SKILL.md with name + description + globs + kind: rule +pnpm skills:sync +git add skills/shared/rules/my-rule +``` + +The `rule-authoring` rule-skill auto-injects when you edit anything under `skills/shared/rules/`. + +### Local (private) + +```sh +mkdir skills/local/my-skill +# create skills/local/my-skill/SKILL.md with name + description frontmatter +pnpm skills:sync +``` + +Local skills are auto-ignored by `skills/local/.gitignore` — no `git add` needed. + +## Duplicate names + +The sync wrapper rejects duplicate skill names across `shared`, `shared/rules`, and `local` with a clear error naming both locations. Fix by renaming one of the directories (and its `SKILL.md` `name` field) before re-running. + +## Validation + +```bash +pnpm test:skills # structure, frontmatter, flattening, git-ignore contract +pnpm test:guardrails # rule-skill loader + adapter contract tests +``` + +## CLI + +The `skills` CLI ([npm](https://www.npmjs.com/package/skills), [source](https://github.com/vercel-labs/skills)) is pinned as an exact devDependency. The wrapper uses the local `node_modules/.bin/skills` — no global install, no `npx`. \ No newline at end of file diff --git a/skills/local/.gitignore b/skills/local/.gitignore new file mode 100644 index 0000000000..7c9d611b59 --- /dev/null +++ b/skills/local/.gitignore @@ -0,0 +1,3 @@ +* +!.gitignore +!README.md diff --git a/skills/local/README.md b/skills/local/README.md new file mode 100644 index 0000000000..b24b01fea6 --- /dev/null +++ b/skills/local/README.md @@ -0,0 +1,22 @@ +# Local skills + +Private, developer-specific skills. Intentionally untracked. + +## Rules + +- Local skills are private and intentionally untracked by Git. +- Each local skill belongs at `skills/local//SKILL.md`. +- `SKILL.md` must not exist directly under `skills/local/` — it always lives inside a named skill directory. +- Local skill names must not collide with shared skill names under `skills/shared/`. +- Local skills participate in synchronization automatically — `pnpm skills:sync` discovers and installs them alongside shared skills. +- Generated agent copies (under `.agents/skills/`, `.claude/skills/`, etc.) must not be edited. They are reproducible from the canonical source here. + +## Adding a local skill + +```sh +mkdir skills/local/my-skill +# create skills/local/my-skill/SKILL.md with name + description frontmatter +pnpm skills:sync +``` + +The nested `.gitignore` keeps everything here private except this README and itself. diff --git a/.agents/shared/skills/rules/README.md b/skills/shared/rules/README.md similarity index 75% rename from .agents/shared/skills/rules/README.md rename to skills/shared/rules/README.md index 5a8b693ea4..0660ccf8c9 100644 --- a/.agents/shared/skills/rules/README.md +++ b/skills/shared/rules/README.md @@ -2,10 +2,14 @@ Prescriptive, path-scoped guardrails that ride on the universal `skills` primitive but are *constraints* on the agent's work, not invocable capabilities. Discriminated from workflow skills by `kind: rule` in frontmatter. -Each rule-skill is a single Markdown file with frontmatter declaring which paths it applies to. The shared loader at `.agents/shared/hooks/inject-rules.mjs` walks the rules folder, matches each rule's `globs` field against the file the agent is about to edit, and emits one agent-agnostic rule stream lazily for whichever adapter is calling it. +Each rule-skill is its own directory containing a `SKILL.md` with frontmatter declaring which paths it applies to. The shared loader at `.agents/shared/hooks/inject-rules.mjs` walks the rules folder, matches each rule's `globs` field against the file the agent is about to edit, and emits one agent-agnostic rule stream lazily for whichever adapter is calling it. The frontmatter field is named `globs` to match the convention emerging across agent runtimes (Cursor's `.cursor/rules/` reads `globs:` natively). Our rules live at a different path, so we don't conflict with any harness's native loader — but a contributor who copies a rule to a runtime-conventional location gets pickup for free. +## Folder + +Rules live at `skills/shared/rules//SKILL.md` and are always checked in. By design there is no per-IC override layer — a rule that's worth firing on every PR is by definition a shared convention, not a personal preference. Personal agent customizations belong elsewhere (`.claude/CLAUDE.md`, IC settings), not in the rule stream. + ## MVP in plain English This spike is trying to solve a simple problem: the agent should get the right project rule at the moment it becomes relevant, instead of carrying the whole project handbook in every prompt. @@ -29,28 +33,13 @@ In a codebase this size, many mistakes are not syntax mistakes. They are "you to Rule-skills are meant to catch that kind of error early. -## Why this is an MVP/POC - -This is deliberately narrow. It proves four things, and no more: - -- rules can be written as plain Markdown with simple frontmatter -- rules can be matched lazily from file paths -- the matching logic can live in a shared place -- agent-specific runtimes can wrap that shared logic without changing the underlying rule stream - -It does **not** claim that every runtime already supports the same local hook mechanism. The point of the spike is that the reusable core is small and testable, even if adapters differ. - -## Folder - -Rules live at `.agents/shared/skills/rules/` and are always checked in. By design there is no per-IC override layer — a rule that's worth firing on every PR is by definition a shared convention, not a personal preference. Personal agent customizations belong elsewhere (`.claude/CLAUDE.md`, IC settings), not in the rule stream. - ## Proof this spike works If someone asks "is this just a concept doc?" the concrete proof is: -- shared rule files exist in `.agents/shared/skills/rules/` -- the shared loader exists in `.agents/shared/hooks/inject-rules.mjs` -- the Claude adapter exists in `.claude/shared/hooks/inject-rules.mjs` +- shared rule skills exist in `skills/shared/rules//SKILL.md` +- the shared loader exists at `.agents/shared/hooks/inject-rules.mjs` +- the Claude adapter exists at `.claude/shared/hooks/inject-rules.mjs` - tests prove the generic and Claude paths carry the same injected rule content Verification commands: @@ -73,10 +62,10 @@ kind: rule | Field | Required | Notes | |---|---|---| -| `name` | yes | Short, kebab-case identifier. | +| `name` | yes | Short, kebab-case identifier. Must match the containing directory. | | `description` | yes | One sentence. Surfaces when the agent enumerates available rules. | | `globs` | yes | Comma-separated globs, paths relative to repo root. `**` is recursive, `*` is single-segment. | -| `kind` | yes | Must be `rule` for the hook to pick it up. Distinguishes from `kind: command` workflow skills. | +| `kind` | yes | Must be `rule` for the hook to pick it up. Distinguishes from workflow skills. | ## Authoring guidance @@ -96,6 +85,15 @@ What to avoid: If the rule grows past one screen, ask whether the extra content is non-obvious or just thorough — if the latter, trim it. +## Adding a new rule + +```sh +mkdir skills/shared/rules/my-rule +# create skills/shared/rules/my-rule/SKILL.md with name + description + globs + kind: rule +``` + +The `rule-authoring` rule-skill auto-injects when you edit anything under `skills/shared/rules/`. + ## Adapter contract The shared loader is a Node script that accepts either: @@ -118,14 +116,14 @@ To inspect what would fire for a path, run the hook directly: ```sh node .agents/shared/hooks/inject-rules.mjs \ - --file "$PWD/src/shared/api/daoService/queries/useDao.ts" + --file "$PWD/apps/app/src/shared/api/daoService/queries/useDao.ts" ``` To inspect the Claude adapter output specifically: ```sh -echo '{"tool_name":"Edit","tool_input":{"file_path":"'"$PWD"'/src/shared/api/daoService/queries/useDao.ts"}}' \ +echo '{"tool_name":"Edit","tool_input":{"file_path":"'"$PWD"'/apps/app/src/shared/api/daoService/queries/useDao.ts"}}' \ | node .claude/shared/hooks/inject-rules.mjs ``` -Empty output = no rules matched. JSON output with `additionalContext` = those rules will be injected. +Empty output = no rules matched. JSON output with `additionalContext` = those rules will be injected. \ No newline at end of file diff --git a/.agents/shared/skills/rules/core-action-decoded-input.md b/skills/shared/rules/core-action-decoded-input/SKILL.md similarity index 100% rename from .agents/shared/skills/rules/core-action-decoded-input.md rename to skills/shared/rules/core-action-decoded-input/SKILL.md diff --git a/.agents/shared/skills/rules/dialog-conventions.md b/skills/shared/rules/dialog-conventions/SKILL.md similarity index 100% rename from .agents/shared/skills/rules/dialog-conventions.md rename to skills/shared/rules/dialog-conventions/SKILL.md diff --git a/.agents/shared/skills/rules/error-and-monitoring.md b/skills/shared/rules/error-and-monitoring/SKILL.md similarity index 100% rename from .agents/shared/skills/rules/error-and-monitoring.md rename to skills/shared/rules/error-and-monitoring/SKILL.md diff --git a/.agents/shared/skills/rules/plugin-slot-registration.md b/skills/shared/rules/plugin-slot-registration/SKILL.md similarity index 100% rename from .agents/shared/skills/rules/plugin-slot-registration.md rename to skills/shared/rules/plugin-slot-registration/SKILL.md diff --git a/.agents/shared/skills/rules/plugin-visibility.md b/skills/shared/rules/plugin-visibility/SKILL.md similarity index 100% rename from .agents/shared/skills/rules/plugin-visibility.md rename to skills/shared/rules/plugin-visibility/SKILL.md diff --git a/.agents/shared/skills/rules/query-and-cache.md b/skills/shared/rules/query-and-cache/SKILL.md similarity index 100% rename from .agents/shared/skills/rules/query-and-cache.md rename to skills/shared/rules/query-and-cache/SKILL.md diff --git a/.agents/shared/skills/rules/rule-authoring.md b/skills/shared/rules/rule-authoring/SKILL.md similarity index 90% rename from .agents/shared/skills/rules/rule-authoring.md rename to skills/shared/rules/rule-authoring/SKILL.md index 39a706b3d3..ed6be24fef 100644 --- a/.agents/shared/skills/rules/rule-authoring.md +++ b/skills/shared/rules/rule-authoring/SKILL.md @@ -1,7 +1,7 @@ --- name: rule-authoring description: How to author or revise a rule-skill — fires when editing files inside the rule folders themselves. -globs: .agents/shared/skills/rules/** +globs: skills/shared/rules/** kind: rule --- @@ -11,8 +11,8 @@ You're editing inside a rule-skills folder. Follow the conventions below so the ## Canon -- `.agents/shared/skills/rules/README.md` — full spec for the rule-skill convention (frontmatter, glob semantics, hook contract). -- `.agents/shared/skills/rules/query-and-cache.md`, `plugin-slot-registration.md` — reference shapes. +- `skills/shared/rules/README.md` — full spec for the rule-skill convention (frontmatter, glob semantics, hook contract). +- `skills/shared/rules/query-and-cache/SKILL.md`, `plugin-slot-registration/SKILL.md` — reference shapes. - `.agents/shared/hooks/inject-rules.mjs` — the shared loader; how rules get matched and injected. ## Authoring shape (sweet spot) From 7c9196f86f64d3f40f51af69f3232801d10b7ad3 Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Tue, 4 Aug 2026 15:28:30 +0200 Subject: [PATCH 2/5] feat(skills): add stacked PR decomposition skill --- .../shared/stacked-pr-decomposition/SKILL.md | 308 ++++++++++++++++++ .../agents/openai.yaml | 6 + 2 files changed, 314 insertions(+) create mode 100644 skills/shared/stacked-pr-decomposition/SKILL.md create mode 100644 skills/shared/stacked-pr-decomposition/agents/openai.yaml diff --git a/skills/shared/stacked-pr-decomposition/SKILL.md b/skills/shared/stacked-pr-decomposition/SKILL.md new file mode 100644 index 0000000000..dd707b4a11 --- /dev/null +++ b/skills/shared/stacked-pr-decomposition/SKILL.md @@ -0,0 +1,308 @@ +--- +name: stacked-pr-decomposition +description: Use when grokking a new ticket or task that may warrant splitting into ordered mergeable PR layers, retrospectively decomposing an existing branch into stacked PRs, or opening a PR where the changeset spans concerns or feels risky +--- + +# Stacked PR Decomposition + +## Overview + +A **stacked PR** is an ordered chain of normal pull requests: the bottom PR targets trunk (`main`), and each higher PR targets the branch below it. GitHub now groups such chains into a first-class **Stack** — a stack map in the PR UI, rules/CI evaluated against the stack's ultimate target branch, and bottom-up direct or merge-queue merge. The CLI (`gh stack`) automates branch creation, cascading rebases, and submission. + +This skill decides **whether** a piece of work is stack-shaped and, if so, **how to slice it into ordered, reviewable layers** and **delegate each layer** to an agent or author. The determining variable is the quality of the split, not the tool. + +**Announce at start:** "I'm using the stacked-pr-decomposition skill to assess and plan layering." + +## When to Use + +```dot +digraph when_stackable { + rankdir=TB; + big [label="Work feels big\n(>1 concern, >~250 LOC,\nor many steps)", shape=diamond]; + seams [label="Can you name 2-5\nordered, independent seams?", shape=diamond]; + stable [label="Are the seams stable\n(not moving daily)?", shape=diamond]; + lower_safe [label="Can the lowest layer\nmerge or land behind a flag/shim?", shape=diamond]; + stack [label="Plan a stack:\n2-5 ordered layers", shape=box]; + spike [label="Spike first:\ndraft branch / design\nbefore opening a stack", shape=box]; + single [label="One PR is fine", shape=box]; + + big -> seams [label="yes"]; + seams -> stable [label="yes"]; + stable -> lower_safe [label="yes"]; + lower_safe -> stack; + seams -> single [label="no"]; + stable -> spike [label="no - unstable"]; +} +``` + +**Use when ANY are true:** +- A ticket/prompt spans multiple concerns (backend + frontend, schema + behavior, refactor + feature) that could be reviewed by different audiences +- Early layers are strategically valuable even before the full feature ships (contract expansion, seam extraction, test scaffolding) +- The work forms a natural dependency chain: lower leaves (models, schema, ports) feed higher leaves (API routes, UI) +- A single PR would exceed ~250 changed LOC or ~25 files, or mix refactor + migration + behavior +- You are decomposing a spec/plan and want each slice to be independently mergeable +- A branch already contains the full work and you need to materialize it into reviewable stacked PRs without losing compile/test safety + +**Do NOT use when:** +- The seams are unstable — invariants changing daily, adjacent layers rewriting each other +- The work is exploratory or tightly coupled with no clean cut lines (use a short-lived draft branch or design spike first) +- It's cross-fork OSS contribution — GitHub Stacks are same-repository only +- The team lacks reliable CI/bot infra at scale; a fragile stack amplifies noise, it doesn't fix it + +## When in the Lifecycle + +Decomposition is not one planning step — it tightens as you learn the code. Inject at three moments: + +1. **Intake (after brainstorming, before recon)** — detect shape only. Output a flag: "stack candidate" or "single PR." Do NOT write layers blind; seams guessed before reading code churn. +2. **After codebase recon, before writing-plans** — real seams now visible (where the legacy caller lives, where a flag would go, which files are ports vs. behavior). Write the dependency story, pick the style, size layers. Feed the layer list into superpowers:writing-plans. +3. **At each layer's commit/PR gate (during executing-plans)** — re-run the Detection Signals + Red Flags. Did the layer bleed into the next? Is the bottom still green? Open the PR only if it passes. + +The bottom layer is often the recon itself: refactor-first / migration-first stacks make the *first* mergeable PR the thing that exposes the real seam for the next layer. You don't plan the whole stack blind — you plan the boring bottom, ship it, and the next seam becomes visible. + +**Retrospective split is first-class.** If implementation already exists, do not pretend you are still at intake. Work backwards from the diff: group files by concern, choose the boring bottom layer, then reconstruct intermediate commits/branches so every layer still compiles and remains merge-safe. + +```dot +digraph lifecycle { + rankdir=LR; + intake [label="Intake:\ndetect shape", shape=box]; + recon [label="Read code:\nfind real seams", shape=box]; + plan [label="writing-plans:\nlayer per concern", shape=box]; + exec [label="executing-plans:\nimplement layer", shape=box]; + gate [label="Commit gate:\nhealth-check this layer", shape=diamond]; + next [label="Next layer", shape=box]; + redo [label="Re-decompose", shape=box]; + done [label="Stack merged", shape=box]; + + intake -> recon -> plan -> exec -> gate; + gate -> next [label="pass"]; + next -> exec; + gate -> redo [label="seams shifted"]; + redo -> plan; + gate -> done [label="last layer"]; +} +``` + +## Detection Signals + +Score each proposed layer. From the research synthesis: + +| Signal | Threshold (heuristic) | Why | +|---|---|---| +| Per-layer size | flag >400 review-relevant LOC; warn >250 LOC or >25 files | Review effectiveness drops as review size grows; generated/test-heavy LOC is cheaper but still affects navigation | +| Stack depth | review carefully >5; exceptional >7 | Deeper stacks = more navigation + dependency burden | +| Lower-layer first response | slow vs repo P75 | Latency compounds when every layer depends on the one below | +| Lower-layer check flakiness | red/green oscillation in last 3 runs | Unstable seams or noisy checks erode throughput | +| Rebase frequency after first review | >2 rebases on bottom | Trickiest part of stacking; repeated rebases = unstable foundation | +| Reviewer spread | every layer same reviewer set despite different concerns | Split along implementation order, not review value | +| Adjacent-layer file overlap | heavy overlap (unless deliberate migration) | Weakens atomicity; boundary may be arbitrary | + +Count **review-relevant LOC**, not raw churn. Tests, snapshots, generated files, locale JSON, and lockfiles review differently from behavior code; they can still make navigation hard, but they should not force a bad seam by themselves. + +A stack is healthy when lower layers are **boring** (low-surprise, high-reusability, low-review-ambiguity) and each layer passes the **dependency-story test**: you can explain why PR N must sit above PR N-1. If you cannot, the split is not good enough. + +## Stack Styles + +Pick the style whose bottom layer is the most boring. The best stacks make the bottom mergeable before the top exists. + +### Refactor-first +Extract seam → rename/cleanup → add behavior on stable names/types. Early layers review easily and merge quickly; risk is renaming that erodes confidence. + +### Migration-first (expand/contract) +Expand schema/contract → dual-write/migrate → cut over → cleanup. Lower-contract change lowers risk and eases rollback; needs disciplined compatibility windows. + +### API-contract-first +Spec/interface stubs → consumers wire to contract → local behavior → cleanup. Pros: reviewers align on contract first. Cons: up-front design can slow purely local changes. + +### Feature-flagged implementation +Flag plumbing → inactive path → incremental implementation → enable → remove flag. Lets small layers land safely on trunk; flags create operational debt if not cleaned up. + +### Branch-by-abstraction +Introduce abstraction → route old calls through it → add new impl → switch → remove old. Avoids giant replacement branch drift; supports progressive cutover; needs careful abstraction design. + +## Decomposition Workflow + +1. **Map the dependency story.** Write one sentence per proposed layer saying what it depends on. Models/schema/ports at the bottom; behavior/UI at the top. +2. **Make the bottom boring.** Prefer refactor-first, contract expansion, ACL introduction, or test scaffolding as the lowest layer. +3. **Ensure ship-safety.** If a lower layer lands before higher behavior, hide incomplete user-visible behavior behind a flag or parallel-change shim. Unfinished behavior must never be exposed by a merged lower layer. +4. **Assign seats per layer.** Route reviewers by concern, not by ticket. Different concerns often deserve different reviewers. +5. **Keep the bottom green.** Lower layers must satisfy checks/reviews before higher layers can merge. If rebases cascade from the bottom, pause and restack — do not pile work on unstable foundations. +6. **Size each layer to one concern.** If a layer mixes refactor + migration + behavior, split again. + +## Retrospective Split Workflow + +Use this when one branch already contains the work and the task is "split this into a stack." + +1. **Inventory the diff by concern, not by chronology.** Group files into contracts/models, shared utilities, data plumbing, UI/behavior, tests, and cleanup. The first grouping is allowed to move files between layers; the final stack is not a replay of your commit history. +2. **Place shared foundations low.** If two upper layers need the same graph utility, type, feature flag, or test helper, put it in the lowest layer that can merge safely. Do not duplicate or let adjacent layers rewrite it. +3. **Keep every intermediate branch compiling.** After slicing layer N, run the narrow check that proves N is valid without layers above it. A layer that only passes with unmerged higher code is not a layer. +4. **Handle shared files deliberately.** For `en.json`, package manifests, lockfiles, route maps, and generated registries, pick one: all related edits in the first layer that needs them; split by stable key namespace; or defer cosmetic/generated churn to cleanup. Never let every layer casually touch the same shared file. +5. **Co-locate tests with the behavior they prove.** Contract tests go with contract layers; UI tests with UI layers. If tests dominate LOC, mention that in the stack map instead of splitting a coherent behavior layer only to satisfy a raw line threshold. +6. **Write the stack map after slicing.** For each layer: purpose, base branch, files, dependency sentence, ship-safety statement, and verification command. + +## Delegation + +When subagents/parallel agents are available, each layer can be delegated once its dependency layer's contract is fixed: + +- **Slice first, delegate per layer.** Treat the stack decomposition as the top-level plan (your job — do not outsource it). Then fan out one agent per layer only where layers are genuinely independent given the fixed contract. +- **Order gates, not parallel everywhere.** The bottom layer (schema/port/contract) is everyone's prerequisite — implement it inline or in a first wave. Only fan out layers that depend solely on already-fixed lower contracts. +- **Each agent gets:** its layer's files, the contract signature it consumes from the layer below, the contract signature it exposes to the layer above, and the constraint "do not change files outside your layer." +- **One cohesive story in branch names/titles:** `extract-auth-port` → `add-auth-contract` → `implement-oauth-flow` → `remove-legacy-auth`. The branch name is part of the review navigation model; expected order: foundation → integration → specialization. +- **CI choreography matches merge choreography.** Fast smoke tests on every layer; broader integration tests on the lowest mergeable layer; full-stack/staging checks on the top layer before a full stack merge. + +A 2-4 layer stack with a fast CI loop and shared context usually improves review quality without much process overhead. Beyond 5 layers, navigation and dependency burden dominate. + +## DDD Seams (when domain modeling is available) + +When the domain tells you the cut lines, the stack feels clean to review: + +- **Bounded context** = candidate for a single stack (usually too large for one PR). +- **Aggregate** = candidate for an individual layer (transactional boundary; external refs go through the aggregate root). +- **Anti-corruption layer** = good bottom layer when crossing contexts or extracting from legacy: introduce translator/facade first, route callers, add new model, remove legacy last. +- **Branch-by-abstraction** = replace one aggregate impl/repository/integration path: abstraction → migrate callers → new impl → cutover → cleanup. + +If a change crosses many aggregates or contexts, assume the **stack boundary** is wrong unless it's a compatibility/migration layer. + +## GitHub Stack Mechanics + +Source: GitHub Docs — https://docs.github.com/en/pull-requests/how-tos/stacked-pull-requests + +**Status and constraints:** Stacked PRs are in public preview. Every PR must be in the **same repository** and form a single linear chain; cross-fork stacks and branching stack shapes are not supported. GitHub Desktop does not support stacks. If a stack is fully merged, it is closed; adding branches later creates a new stack. + +**Dependency rule:** if code in one layer depends on code in another, the dependency must be in the same branch or a lower branch. Put shared types, schemas, utilities, flags, and generated/shared-file ownership low enough that all higher layers can compile against them. + +**Structure:** bottom PR base = stack trunk (`main`, default branch, release branch, etc.); each higher PR base = the branch below it. Each PR shows only the diff between its branch and the branch below it, so each layer needs a focused review story. + +**Rules and CI:** every PR in the stack is evaluated against the stack base, not just its direct branch base. Required reviews, CODEOWNERS, required checks, and `pull_request` workflows targeting the base apply to every layer. GitHub exposes stack metadata at `github.event.pull_request.stack`; use it to skip expensive redundant jobs when appropriate. + +**Merging:** merges happen bottom-up. Merging the top PR merges the whole stack; merging a mid-stack PR merges that PR and everything below it while upper PRs remain open and retarget. `gh stack merge` is all-or-nothing for the selected segment; merge queues enqueue the selected segment but may land PRs in separate groups as the queue processes them. Stacked merges cannot bypass merge requirements. + +**Rebasing and sync:** rebasing is the fragile part. Use the GitHub UI server-side rebase or `gh stack rebase` for cascading rebases. `gh stack sync` fetches, reconciles remote/local stack state, fast-forwards trunk, cascades rebase if trunk moved, pushes with `--force-with-lease` when needed, syncs PR state, links open PRs into a stack, and optionally prunes merged local branches. + +**CLI setup:** `gh stack` requires GitHub CLI and the extension: +```shell +gh extension install github/gh-stack +gh auth login +``` +Quick path: `gh stack init` → commit bottom layer → `gh stack add ` for each higher layer → `gh stack push` → `gh stack submit` → `gh stack view`. `gh stack add -Am "message"` can stage, commit, and add the next branch in one step. + +**Useful commands:** `view` shows ordering/status; `up`/`down`/`top`/`bottom` navigate; `submit` pushes and creates/updates PRs; `link` turns existing branches/PRs into a stack without local tracking; `modify` is required to reorder/restructure a stack and needs a clean linear working tree; `unstack` dissolves stack tracking while preserving branches/PRs. + +**Manual branches:** still fine when the team is not using `gh stack`: create branches bottom-up, set each PR base to the branch below it, and keep the stack map in PR descriptions. Use `gh stack` when you want CLI-assisted navigation, submit/link/sync automation, cascading rebases, or reordering via `modify`; it is convenience, not a correctness requirement. + +**Programmatic tooling:** legacy pull-request merge endpoints cannot merge stacks; tools/bots need the Stacks API. Webhooks include stack data when PRs join/move/leave stacks; REST/GraphQL expose stack membership for dashboards and automation. + +## Review Feedback + Rebase Loop + +When review feedback lands on a mid-stack PR, fix it on the branch that owns the change — not on the top branch as a workaround. + +1. Navigate to the owning branch: `gh stack checkout ` or `gh stack up/down/top/bottom`. +2. Make the fix, stage, and commit it there. +3. Cascade the update through dependent layers: `gh stack rebase` (or `gh stack rebase --upstack` when starting from the changed branch). +4. Push the rebased stack: `gh stack push`. This uses `--force-with-lease` and retriggers CI on affected PRs. +5. Return to the working layer with `gh stack top` or `gh stack checkout `. + +If the repo requires signed commits, avoid the GitHub website **Rebase stack** button; server-side rebases are unsigned. Use `gh stack rebase` locally, then `gh stack push`. + +## Merge Gate + Troubleshooting + +Before merging a PR or contiguous segment, verify: every PR below it is approved, checks pass, the stack is linear, and the current PR satisfies rules for the stack base. You cannot merge a mid-stack PR alone; PRs below it always merge with it. + +## Merge Strategy + +Do **not** flatten the stack into one giant PR after review. The point is separate review units with explicit dependencies. + +Default for a feature that should land atomically: get every layer reviewed/approved, keep the stack green and linear, then use the GitHub UI merge box on the **top PR** (or `gh stack merge`) once. The merge box shows whole-stack status; GitHub lands the top plus every unmerged PR below it as one contiguous bottom-up operation. + +Merge lower layers earlier when they are independently useful and merge-safe: foundation refactors, schema/contract expansion, inert flags, test scaffolding, or cleanup that does not expose unfinished behavior. Merging a mid-stack PR lands it plus everything below it; upper PRs remain open and retarget. After any bottom/partial merge, run `gh stack sync --prune` before continuing. + +Use merge queue normally if the repo requires it; queued stacks stay ordered, but very large stacks may split across consecutive merge groups. If a lower PR is ejected, all PRs above it are ejected too. + +Choose: +| Situation | Merge posture | +|---|---| +| User-visible feature must appear all at once | Approve all layers, merge top once | +| Lower layer improves code safely by itself | Merge that bottom/mid contiguous segment early | +| Lower layer exposes incomplete behavior | Keep it unmerged or hide behind a flag/shim | +| Review uncovers bad seam | Restack before merging; don't flatten as a shortcut | + + +| Symptom | Fix | +|---|---| +| Merge box shows **Rebase stack** | Stack is not linear; run `gh stack rebase && gh stack push` or use the website rebase if unsigned commits are acceptable | +| `gh stack rebase` conflicts | Resolve conflict markers, `git add`, then `gh stack rebase --continue`; use `--abort` to restore pre-rebase state | +| `gh stack sync` stops on conflict | It restores original branches; run `gh stack rebase`, resolve, then `gh stack push` | +| `gh stack modify` will not start | Need active stack, clean working tree, no rebase, no queued PR, linear history | +| Middle PR closed | Upper PRs are blocked; unstack/restructure with `gh stack modify`, then recreate/link | +| Merge queue ejects one PR | All PRs above it are ejected too; fix the cause and re-add the stack | +| Large stack in merge queue | Queue may exceed group max by 50%; larger stacks split across consecutive groups | + +For local state after bottom merges, run `gh stack sync --prune`: fetch, fast-forward trunk, rebase remaining branches onto it, push, sync PR status, and delete merged local branches. + +For CI cost, use stack metadata: lowest unmerged PR when `github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref`; top PR when `github.event.pull_request.stack.position == github.event.pull_request.stack.size`. + + +## Checklists + +### Author +- Define the dependency story before opening the stack. If you can't explain why PR N must sit above N-1, the split isn't good enough. +- One concern per layer; if it mixes refactor + migration + behavior, split again. +- Prefer boring lower layers: schema expansion, abstraction extraction, interface stabilization, ACL introduction, pure refactor. +- Submit as soon as a layer is ready; mark unfinished layers as draft. Do not wait for the whole feature. +- Route reviewers by layer, not by ticket. +- Keep the bottom green. Lower layers must pass checks/reviews before higher layers merge. +- If rebases keep cascading from the bottom, pause and restack. +- Hide incomplete user-visible behavior behind a flag or parallel-change path before landing lower layers. +- For retrospective splits, prove each branch compiles without layers above it; do not rely on the full original branch. +- Name shared files (`en.json`, package manifests, lockfiles, generated registries) in the stack map and explain which layer owns each one. +- Address review feedback on the branch that owns the change, then cascade with `gh stack rebase` / `gh stack push`. +- After bottom merges, run `gh stack sync --prune` before continuing work. + +### Reviewer +- Read the stack map first: is this a foundation, mid-layer, or top? +- Judge the current layer against its stated purpose, not the full future feature. +- Ask whether the layer could merge safely today. If not, is a flag/shim/migration step missing? +- Look for overlap with adjacent layers — heavy overlap often means arbitrary boundaries. +- Be stricter on bottom layers about API shape, invariants, names — mistakes multiply upward. +- Approve good-enough lower layers that improve code health; prolonged blocking defeats throughput. +- Watch check quality, not just color — false positives and flaky mergeability checks are especially damaging in stacks. +- If the stack is deeper than the domain warrants, say so. +- Request changes on the layer that owns the problem. Do not ask the author to patch a lower-layer issue in a higher PR. +- Check whether requested changes force a cascade; if they do, expect upper-layer CI to rerun. + +## Common Mistakes + +| Mistake | Fix | +|---|---| +| Stacking to escape one giant PR | Stacking trades review size for dependency maintenance; only worth it with stable seams | +| Opening a stack before seams are stable | Spike/design first; stacking unstable work = churn amplifier | +| Mixing concerns in one layer | Split again; one concern per layer | +| Same reviewers on every layer despite different concerns | Route reviewers by layer concern | +| Bottom layer not green | Lower layers must pass before higher layers merge; keep bottom green | +| Letting rebases cascade without restacking | Pause, restack; don't pile on unstable foundations | +| Exposing incomplete behavior via a merged lower layer | Use a feature flag or parallel-change path | +| Treating stacking as ritual for exploratory work | Use a draft branch / design spike instead | +| Ignoring CI fan-out on every layer | Use stack metadata to gate heavy jobs (lowest unmerged / top only) | +| Treating a retrospective split like greenfield planning | Work backwards from the existing diff; every reconstructed layer must compile alone | +| Letting every layer touch the same shared file | Assign ownership: one layer, stable key namespace, or cleanup-only generated churn | +| Splitting only by raw LOC when tests dominate | Count review-relevant LOC; keep coherent behavior and explain test-heavy churn | +| Fixing review feedback on the top branch | Checkout the owning branch, commit there, rebase/push upward | +| Using website rebase in signed-commit repos | Rebase locally with `gh stack rebase`; server-side rebase commits are unsigned | +| Trying to merge a mid-stack PR by itself | Merge segments are contiguous from the lowest unmerged PR upward | +| Continuing after a bottom merge without syncing | Run `gh stack sync --prune` so remaining branches retarget cleanly | +| Flattening the stack into one PR after review | Keep layers as PRs; merge top once for atomic landing or merge safe lower segments early | + +## Red Flags — Reconsider the Split + +- You cannot articulate the dependency story (why N sits above N-1). +- The bottom layer is not merge-safe and not hidden behind a flag/shim. +- Every layer has the same reviewer set despite different concerns. +- Heavy adjacent-layer file overlap that is not a deliberate migration. +- Rebase cascades from the bottom after review has started. +- Each layer passes checks only in isolation — the "green stack illusion." +- Retrospective layers only compile when checked out with later branches. +- Shared files (`en.json`, lockfiles, package manifests, generated registries) are touched by many adjacent layers without an owner. +- Review feedback is patched in a higher layer than the code it changes. +- A stack needs unsigned server-side rebase in a signed-commit repo. +- A closed middle PR blocks upper PRs. + +These mean: do not open the stack yet. Stabilize seams, or ship a single focused PR instead. \ No newline at end of file diff --git a/skills/shared/stacked-pr-decomposition/agents/openai.yaml b/skills/shared/stacked-pr-decomposition/agents/openai.yaml new file mode 100644 index 0000000000..314d6761ea --- /dev/null +++ b/skills/shared/stacked-pr-decomposition/agents/openai.yaml @@ -0,0 +1,6 @@ +interface: + display_name: "Stacked PR Decomposition" + short_description: "Slice risky changes into ordered, mergeable PR layers" + default_prompt: "Use $stacked-pr-decomposition to slice this change into ordered mergeable layers with health checks and delegation gates." +policy: + allow_implicit_invocation: true From ef863794e1e7bccb47e0dd96b2320abd8e3a124e Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Wed, 5 Aug 2026 17:56:19 +0200 Subject: [PATCH 3/5] fix(skills): scope sync to universal+claude-code, harden CI guard, fix docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - sync-skills.mjs: replace `-a codex/cursor/gemini-cli` (all resolve to the same universal .agents/skills store, causing 3x clean+recopy) with `-a universal -a claude-code`, so each generated root is written once. - sync-skills.mjs: CI skip now treats any truthy CI except '0'/'false' as CI, not only '1'/'true'. - sync-skills.mjs: correct the README.md-skip comment — the pinned CLI (1.5.20) excludes only metadata.json, not README.md. - .gitignore: drop .codex/skills, .cursor/skills, .gemini/skills; the pinned CLI never creates them (those agents map to .agents/skills). - skills/README.md: accurate install-target description + explicit note that node_modules-provided skills are not auto-authorized (experimental_sync is deliberately unused). --- .gitignore | 3 --- scripts/sync-skills.mjs | 16 +++++++++------- skills/README.md | 10 ++++++++-- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/.gitignore b/.gitignore index 42199e2621..11d65d6bf1 100644 --- a/.gitignore +++ b/.gitignore @@ -68,9 +68,6 @@ next-env.d.ts # generated skill discovery roots (pnpm skills:sync) .agents/skills/ .claude/skills/ -.codex/skills/ -.cursor/skills/ -.gemini/skills/ # skills CLI generated lockfile (regenerated by pnpm skills:sync) skills-lock.json .playwright-mcp diff --git a/scripts/sync-skills.mjs b/scripts/sync-skills.mjs index fc7c7aa41b..5605d66097 100755 --- a/scripts/sync-skills.mjs +++ b/scripts/sync-skills.mjs @@ -31,19 +31,19 @@ const GENERATED_ROOTS = [ ]; const AGENT_FLAGS = [ + // codex, cursor, and gemini-cli all resolve to the same universal .agents/skills + // store in the pinned CLI, so passing each would clean+recopy that dir once per + // agent. `universal` writes .agents/skills exactly once; claude-code writes + // .claude/skills. Together they cover both generated roots with no repeated work. '-a', - 'codex', + 'universal', '-a', 'claude-code', - '-a', - 'cursor', - '-a', - 'gemini-cli', ]; // --- skip conditions --- -if (process.env.CI === '1' || process.env.CI === 'true') { +if (process.env.CI && process.env.CI !== '0' && process.env.CI !== 'false') { console.log('[skills] CI detected — skipping sync.'); process.exit(0); } @@ -252,7 +252,9 @@ function validateRoot(root) { process.exit(1); } - // Validate supporting files were preserved (skip README.md which the CLI excludes). + // Validate supporting files were preserved. README.md is intentionally not + // required in generated output: it is often a category/skill doc, not an + // agent-consumed asset, and the pinned CLI (1.5.20) excludes only metadata.json. const sourceEntries = readdirSync(skill.dir, { withFileTypes: true }) .filter((e) => e.name !== 'SKILL.md' && e.name !== 'README.md') .map((e) => e.name); diff --git a/skills/README.md b/skills/README.md index b0d2bb01fd..3676e1176c 100644 --- a/skills/README.md +++ b/skills/README.md @@ -134,7 +134,7 @@ All generated roots are gitignored — do not commit generated skill copies and 3. Rejects category-level `SKILL.md` files. 4. Validates frontmatter `name` matches the directory and `description` is present. 5. Reconciles stale generated skills (removes generated dirs with no canonical source). -6. Installs all skills via the pinned CLI with `--copy --yes --full-depth` to Codex, Claude Code, Cursor, and Gemini CLI. +6. Installs all skills via the pinned CLI (`skills add … --copy --yes --full-depth`) to the two generated roots — the universal store (`.agents/skills`, read by Codex, Cursor, Gemini CLI, and others) and Claude Code (`.claude/skills`). It passes `-a universal -a claude-code`: the `universal` target writes `.agents/skills` exactly once, rather than once per universal agent. 7. Validates the generated filesystem: every canonical skill exists at each root, `SKILL.md` present, supporting files preserved, categories flattened, executable bits retained. 8. Fails on any inconsistency even if the CLI reported success. @@ -183,4 +183,10 @@ pnpm test:guardrails # rule-skill loader + adapter contract tests ## CLI -The `skills` CLI ([npm](https://www.npmjs.com/package/skills), [source](https://github.com/vercel-labs/skills)) is pinned as an exact devDependency. The wrapper uses the local `node_modules/.bin/skills` — no global install, no `npx`. \ No newline at end of file +The `skills` CLI ([npm](https://www.npmjs.com/package/skills), [source](https://github.com/vercel-labs/skills)) is pinned as an exact devDependency. The wrapper uses the local `node_modules/.bin/skills` — no global install, no `npx`. + +## Dependency-provided skills + +The CLI also ships an `experimental_sync` command that discovers skills bundled inside `node_modules` packages. This repository deliberately does **not** use it. `pnpm skills:sync` installs only the reviewed, checked-in catalog under `skills/` — a package appearing in the dependency graph never turns its `SKILL.md` into an authorized agent instruction here. Adopting a package-provided skill is an explicit, manual decision: review it, then copy it into `skills/shared/` or `skills/local/`. + +Note: `pnpm skills:sync` (repository catalog reconciliation) is a different operation from the CLI's `experimental_sync` (dependency discovery). Same word, opposite trust model. \ No newline at end of file From 880370216999b9752cb96861d24d6a7014d50ca8 Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Wed, 5 Aug 2026 17:56:22 +0200 Subject: [PATCH 4/5] docs(skills): record rebase + shared-file learnings in stacked-pr-decomposition - Preserve lower-layer approvals with `gh stack rebase --upstack --no-trunk`; a full trunk rebase rewrites unchanged lower layers' SHAs and dismisses their reviews. - Verify shared-file diffs (en.json/generated registries) against base before pushing; a rebase/re-apply can silently drop unrelated keys. --- skills/shared/stacked-pr-decomposition/SKILL.md | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/skills/shared/stacked-pr-decomposition/SKILL.md b/skills/shared/stacked-pr-decomposition/SKILL.md index dd707b4a11..6c91462fe7 100644 --- a/skills/shared/stacked-pr-decomposition/SKILL.md +++ b/skills/shared/stacked-pr-decomposition/SKILL.md @@ -136,6 +136,7 @@ Use this when one branch already contains the work and the task is "split this i 2. **Place shared foundations low.** If two upper layers need the same graph utility, type, feature flag, or test helper, put it in the lowest layer that can merge safely. Do not duplicate or let adjacent layers rewrite it. 3. **Keep every intermediate branch compiling.** After slicing layer N, run the narrow check that proves N is valid without layers above it. A layer that only passes with unmerged higher code is not a layer. 4. **Handle shared files deliberately.** For `en.json`, package manifests, lockfiles, route maps, and generated registries, pick one: all related edits in the first layer that needs them; split by stable key namespace; or defer cosmetic/generated churn to cleanup. Never let every layer casually touch the same shared file. + - **Verify shared-file diffs before pushing.** A rebase or partial re-apply of `en.json`, lockfiles, or generated registries can silently drop unrelated entries. Diff each shared file against the base (`git diff ...HEAD -- `); it must contain only your layer's keys. Restore accidental deletions byte-for-byte from `main` (locale files are the common victim — a stray deletion removes keys other layers or pages still reference). 5. **Co-locate tests with the behavior they prove.** Contract tests go with contract layers; UI tests with UI layers. If tests dominate LOC, mention that in the stack map instead of splitting a coherent behavior layer only to satisfy a raw line threshold. 6. **Write the stack map after slicing.** For each layer: purpose, base branch, files, dependency sentence, ship-safety statement, and verification command. @@ -197,10 +198,14 @@ When review feedback lands on a mid-stack PR, fix it on the branch that owns the 1. Navigate to the owning branch: `gh stack checkout ` or `gh stack up/down/top/bottom`. 2. Make the fix, stage, and commit it there. -3. Cascade the update through dependent layers: `gh stack rebase` (or `gh stack rebase --upstack` when starting from the changed branch). -4. Push the rebased stack: `gh stack push`. This uses `--force-with-lease` and retriggers CI on affected PRs. +3. Cascade **only upward**: `gh stack rebase --upstack --no-trunk` from the changed branch. This rebases the changed layer and everything above it onto each other, without re-fetching or re-parenting the stack onto trunk. +4. Push: `gh stack push`. It force-with-leases only the branches whose SHA actually changed; untouched lower branches are no-ops. 5. Return to the working layer with `gh stack top` or `gh stack checkout `. +**Preserve lower-layer approvals — do not re-parent onto trunk for every edit.** Branch protection's "dismiss stale reviews on push" dismisses a layer's approval whenever its head SHA changes. A plain `gh stack rebase` (or `gh stack sync`) fetches trunk and re-parents the *whole* stack onto the latest `main`; if `main` moved, that rewrites even approved layers **below** your change and dismisses their approvals for no functional reason. `--upstack --no-trunk` touches only the changed layer and up, so an L3 fix dismisses at most L3–L5 and never L1–L2. Reach for a full trunk rebase (`gh stack rebase` / `gh stack sync`) only when you have a genuine conflict with `main`, or as one deliberate pre-merge catch-up — moments when you expect to re-collect approvals anyway. + +**Lockfile / generated-file conflicts during a rebase:** never hand-merge `pnpm-lock.yaml` (or other generated files). Take the base side and regenerate: `git checkout --ours pnpm-lock.yaml && pnpm install`, confirm `pnpm install --frozen-lockfile` exits clean (that is exactly what CI runs), `git add`, then `gh stack rebase --continue`. + If the repo requires signed commits, avoid the GitHub website **Rebase stack** button; server-side rebases are unsigned. Use `gh stack rebase` locally, then `gh stack push`. ## Merge Gate + Troubleshooting @@ -254,7 +259,7 @@ For CI cost, use stack metadata: lowest unmerged PR when `github.event.pull_requ - Hide incomplete user-visible behavior behind a flag or parallel-change path before landing lower layers. - For retrospective splits, prove each branch compiles without layers above it; do not rely on the full original branch. - Name shared files (`en.json`, package manifests, lockfiles, generated registries) in the stack map and explain which layer owns each one. -- Address review feedback on the branch that owns the change, then cascade with `gh stack rebase` / `gh stack push`. +- Address review feedback on the branch that owns the change, then cascade **upward only** with `gh stack rebase --upstack --no-trunk` (not a full trunk rebase) so lower-layer approvals survive; then `gh stack push`. - After bottom merges, run `gh stack sync --prune` before continuing work. ### Reviewer @@ -290,6 +295,10 @@ For CI cost, use stack metadata: lowest unmerged PR when `github.event.pull_requ | Trying to merge a mid-stack PR by itself | Merge segments are contiguous from the lowest unmerged PR upward | | Continuing after a bottom merge without syncing | Run `gh stack sync --prune` so remaining branches retarget cleanly | | Flattening the stack into one PR after review | Keep layers as PRs; merge top once for atomic landing or merge safe lower segments early | +| Re-parenting the whole stack onto `main` for every mid-stack edit | Use `gh stack rebase --upstack --no-trunk`; full trunk rebase only for a real `main` conflict or a deliberate pre-merge sync | +| Full `gh stack rebase`/`sync` per fix, dismissing lower approvals | Moving trunk rewrites approved lower layers' SHAs → "dismiss stale reviews" fires; scope with `--upstack --no-trunk` | +| Hand-merging `pnpm-lock.yaml` conflict markers | Take base (`git checkout --ours`) + `pnpm install` to regenerate; verify `--frozen-lockfile`; then `--continue` | +| Rebase/re-apply silently drops unrelated `en.json`/generated keys | Diff each shared file against base before pushing; it must contain only your layer's keys; restore accidental deletions byte-for-byte from `main` | ## Red Flags — Reconsider the Split @@ -302,6 +311,7 @@ For CI cost, use stack metadata: lowest unmerged PR when `github.event.pull_requ - Retrospective layers only compile when checked out with later branches. - Shared files (`en.json`, lockfiles, package manifests, generated registries) are touched by many adjacent layers without an owner. - Review feedback is patched in a higher layer than the code it changes. +- Every small mid-stack fix triggers a full-stack rebase onto a moving `main`, dismissing lower-layer approvals each time. - A stack needs unsigned server-side rebase in a signed-commit repo. - A closed middle PR blocks upper PRs. From 1777daafa3b3658ab987734d0ef879496069f916 Mon Sep 17 00:00:00 2001 From: Kevin Davis Date: Wed, 5 Aug 2026 18:52:27 +0200 Subject: [PATCH 5/5] docs(skills): add stack merge-order guidance to stacked-pr-decomposition Review bottom-up (1->N), merge once from the top PR. Document the bottom-by-bottom merge trap (each bottom merge rebases the layer above -> dismiss-stale-reviews wipes its approval) and the web merge button's misleading label; prefer gh stack merge. --- skills/shared/stacked-pr-decomposition/SKILL.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/skills/shared/stacked-pr-decomposition/SKILL.md b/skills/shared/stacked-pr-decomposition/SKILL.md index 6c91462fe7..1a25d332cb 100644 --- a/skills/shared/stacked-pr-decomposition/SKILL.md +++ b/skills/shared/stacked-pr-decomposition/SKILL.md @@ -218,6 +218,12 @@ Do **not** flatten the stack into one giant PR after review. The point is separa Default for a feature that should land atomically: get every layer reviewed/approved, keep the stack green and linear, then use the GitHub UI merge box on the **top PR** (or `gh stack merge`) once. The merge box shows whole-stack status; GitHub lands the top plus every unmerged PR below it as one contiguous bottom-up operation. +**Review up, merge from the top.** Approve bottom-up (layer 1 → N), then merge **once from the top PR (N)** — never one layer at a time from the bottom. Reviewing climbs up; the single top merge lands the whole stack from the top down. + +**Merging the bottom PR one layer at a time is the trap.** It looks right — layer 1 is closest to `main` — but landing just the bottom PR retargets the layer above onto `main` and rebases it; the SHA change trips branch protection's "dismiss stale reviews on push" and dismisses that layer's approval. Repeat up the stack and a 5-layer stack costs four needless re-reviews — the same stale-review mechanism as a mid-stack rebase (see **Review Feedback + Rebase Loop**). Merging from the top lands the whole approved group at once, with no intermediate retarget. + +**The web merge button is easy to misread.** Its label reflects only what that PR's box lands, with no hint it belongs to a stack: on the **bottom** PR it reads "Merge pull request" (singular — merges only that layer and starts the rebase cascade above); on the **top** PR it reads "Merge pull requests (N)" — the count is the only signal that one click lands the whole stack, and you see it only if you open the top PR. `gh stack merge` is the least ambiguous path; make it (or the top PR's box) the default and reserve the per-PR web button for a deliberate partial merge. + Merge lower layers earlier when they are independently useful and merge-safe: foundation refactors, schema/contract expansion, inert flags, test scaffolding, or cleanup that does not expose unfinished behavior. Merging a mid-stack PR lands it plus everything below it; upper PRs remain open and retarget. After any bottom/partial merge, run `gh stack sync --prune` before continuing. Use merge queue normally if the repo requires it; queued stacks stay ordered, but very large stacks may split across consecutive merge groups. If a lower PR is ejected, all PRs above it are ejected too. @@ -225,7 +231,7 @@ Use merge queue normally if the repo requires it; queued stacks stay ordered, bu Choose: | Situation | Merge posture | |---|---| -| User-visible feature must appear all at once | Approve all layers, merge top once | +| User-visible feature must appear all at once | Approve all layers, then `gh stack merge` (or merge the top PR) once | | Lower layer improves code safely by itself | Merge that bottom/mid contiguous segment early | | Lower layer exposes incomplete behavior | Keep it unmerged or hide behind a flag/shim | | Review uncovers bad seam | Restack before merging; don't flatten as a shortcut | @@ -293,6 +299,7 @@ For CI cost, use stack metadata: lowest unmerged PR when `github.event.pull_requ | Fixing review feedback on the top branch | Checkout the owning branch, commit there, rebase/push upward | | Using website rebase in signed-commit repos | Rebase locally with `gh stack rebase`; server-side rebase commits are unsigned | | Trying to merge a mid-stack PR by itself | Merge segments are contiguous from the lowest unmerged PR upward | +| Merging the bottom PR one layer at a time | Each bottom merge rebases the layer above → its approval is dismissed; merge from the top (`gh stack merge`) so the whole approved stack lands at once | | Continuing after a bottom merge without syncing | Run `gh stack sync --prune` so remaining branches retarget cleanly | | Flattening the stack into one PR after review | Keep layers as PRs; merge top once for atomic landing or merge safe lower segments early | | Re-parenting the whole stack onto `main` for every mid-stack edit | Use `gh stack rebase --upstack --no-trunk`; full trunk rebase only for a real `main` conflict or a deliberate pre-merge sync |