From 99128cefd6668c313fca194f22f76169422b1ceb Mon Sep 17 00:00:00 2001 From: OpenCode Bot Date: Fri, 10 Jul 2026 22:57:36 +0200 Subject: [PATCH] fix(skills): repair prod routing, deploy gate & telemetry gaps from skill-system audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the skills FTS fix (cc19df9). Audit of the skill invocation/usage path surfaced runtime/deploy gaps that the FTS fix alone did not cover. Routing quality (skills-store.ts): - route() drops non-task skills from recommendations (agent/command/lifecycle types + _routing-index) and over-fetches to preserve recall; these stay readable via skill_read/agent_read/command_read. - recencyBoost normalized to [0,1] so a heavily-loaded skill can't out-rank a perfect lexical match on an unrelated query. - get() falls back to agent:/command: prefixes for a bare name (exact match still wins), so skill_read('builder') no longer 404s. - deadSkills() excludes protected System/Lifecycle categories (was false-positive noise in skill_health). - Removed dead applySkillDecay/deprecateSkills prepared stmts (stale 10/30 thresholds; live path uses constants 5/20). Deployment (entrypoint.sh, import-skills.ts, cli.ts): - Boot import is now version-gated on a content hash of the bundled catalog, not first-boot-only. /data is a persistent volume, so unchanged-count redeploys previously skipped import and new skills never reached prod. Import stays additive (idempotent, FTS-safe upsert). - import-skills --full: opt-in full-sync that soft-deprecates active skills absent from the bundle (reversible; never touches System/Lifecycle or drafts). Concurrency & hardening (db.ts, routes/skills.ts): - openDb sets busy_timeout=5000 (WAL + per-call connections vs the 24h decay UPDATE could hit SQLITE_BUSY with no retry). - /telemetry/skill-usage: optional token gate via SKILLBRAIN_TELEMETRY_TOKEN (localhost-only endpoint feeds reinforceSkill / ~22% of routing weight). - GET /api/skills/health now requireAdmin, matching sibling write routes. Tests (storage 25, codegraph 294 green): - New skills-store-fts-rebuild: corrupts the external-content FTS index (stale term + orphan + missing) and asserts migration 035's one-time rebuild repairs it — the prod-critical path that was previously untested. - New skills-store-routing: route() exclusion, get() prefix fallback, deadSkills protected-category filter. - import-skills: --full prune deprecates removed skills, protects System/Lifecycle. Docs: - packages/codegraph/docs/DEPLOY-SKILLS.md: two-DB model, propagation mechanisms, full-sync, telemetry reality. - CLAUDE.md: corrected the skill-apply-hook note (local apply signal does not reach prod ranking in the remote-MCP setup). Co-Authored-By: Claude Opus 4.8 --- CLAUDE.md | 35 +++++- packages/codegraph/docs/DEPLOY-SKILLS.md | 74 ++++++++++++ packages/codegraph/entrypoint.sh | 31 ++++- packages/codegraph/src/cli.ts | 6 +- packages/codegraph/src/mcp/routes/skills.ts | 18 ++- packages/storage/src/db.ts | 5 + packages/storage/src/import-skills.ts | 48 +++++++- packages/storage/src/skills-store.ts | 51 ++++++--- packages/storage/tests/import-skills.test.ts | 40 +++++++ .../tests/skills-store-fts-rebuild.test.ts | 104 +++++++++++++++++ .../tests/skills-store-routing.test.ts | 106 ++++++++++++++++++ 11 files changed, 489 insertions(+), 29 deletions(-) create mode 100644 packages/codegraph/docs/DEPLOY-SKILLS.md create mode 100644 packages/storage/tests/skills-store-fts-rebuild.test.ts create mode 100644 packages/storage/tests/skills-store-routing.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 340bb9b..3487b50 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -111,6 +111,37 @@ To use: read `.claude/command/{name}.md` and follow its protocol. --- +## Codex Integration (second-opinion runtime) + +Codex CLI runs in parallel to Claude via the official `codex@openai-codex` plugin. Use for second opinions, deeper investigations, or to offload long tasks while Claude continues other work. + +### Slash commands +| Command | Use when | +|---------|----------| +| `/codex:review` | Second-opinion code review of working tree or branch diff | +| `/codex:adversarial-review` | Aggressive review — challenges design choices, not just bugs | +| `/codex:rescue ` | Hand a substantial debugging/implementation task to Codex | +| `/codex:status` | List running Codex tasks | +| `/codex:result` | Fetch result of a finished Codex task | +| `/codex:cancel` | Cancel a running Codex task | +| `/codex:setup` | Verify/configure Codex runtime | + +Flags: `--wait` (foreground) / `--background` (detach), `--write` (default for rescue, off for reviews), `--resume` (continue last Codex thread in this repo), `--fresh` (start new thread). + +### Custom subagents (user-level, available in all projects) +Invoke via the `Agent` tool with `subagent_type`: + +| Subagent | When to dispatch | +|----------|------------------| +| `codex-rescue` | Generic rescue — Claude is stuck or needs second implementation pass | +| `codex-security-auditor` | Before merging auth/secrets/input changes — OWASP Top 10 read-only audit | +| `codex-perf-analyst` | Before shipping a page that targets Lighthouse > 90 — RSC/bundle/data-fetching audit | +| `codex-coolify-validator` | Before deploying — catches hardcoded paths, missing env vars, Docker footguns | + +All custom subagents run **read-only** by default and default to `--background`. They forward a pre-framed prompt to `codex-companion.mjs task`; do not call Codex directly from the main thread when one of these specialized agents fits. + +--- + ## Auto-hooks (project-scoped) The following are wired in `.claude/settings.json` and run automatically — do **not** invoke them manually unless the task changes mid-session. @@ -118,11 +149,11 @@ The following are wired in `.claude/settings.json` and run automatically — do | Event | Script | Purpose | |-------|--------|---------| | `SessionStart` | `.claude/scripts/load_project_context.sh` | Prints 5-layer Cortex briefing into the session as system context | -| `PostToolUse` matcher `Skill` | `.claude/scripts/skill-apply-hook.sh` | Inserts an `applied` row into `.codegraph/graph.db skill_usage` so health/route ranking sees real apply signal | +| `PostToolUse` matcher `Skill` | `.claude/scripts/skill-apply-hook.sh` | Inserts an `applied` row into the **local** `.codegraph/graph.db skill_usage` | Implications: - Skip manual `cortex_briefing` calls at the start of a session — the briefing is already in context. -- The `Skill` tool (built-in Claude Code skills) is auto-instrumented. For MCP skills loaded via `skill_read`, telemetry is recorded server-side and is not affected by these hooks. +- The `Skill` tool (built-in Claude Code skills) is auto-instrumented into the **local** DB. Because the MCP `codegraph` server proxies to prod (`memory.fl1.it`), these local `applied` rows do **not** reach prod's route/health ranking — only MCP `skill_read`/`skill_apply`/`skill_route` record telemetry server-side. See [`packages/codegraph/docs/DEPLOY-SKILLS.md`](packages/codegraph/docs/DEPLOY-SKILLS.md) for the full two-DB / telemetry picture and how to deploy skill changes to prod. - Both scripts walk upward from `cwd` to locate `.codegraph/graph.db`, so they work from any subdir of the workspace. --- diff --git a/packages/codegraph/docs/DEPLOY-SKILLS.md b/packages/codegraph/docs/DEPLOY-SKILLS.md new file mode 100644 index 0000000..b272f2e --- /dev/null +++ b/packages/codegraph/docs/DEPLOY-SKILLS.md @@ -0,0 +1,74 @@ +# Deploying skill-catalog changes to production + +The skill subsystem spans **two databases**, and updates propagate through +**two independent mechanisms**. Knowing which is which avoids the classic +"I merged new skills but prod didn't change" trap. + +## Two databases + +| DB | Who reads it | Skills | +|----|--------------|--------| +| **Production** — `/data/.codegraph/graph.db` on `memory.fl1.it` | Every `skill_route` / `skill_read` at runtime (the MCP `codegraph` server is a proxy to `SKILLBRAIN_MCP_URL=https://memory.fl1.it/mcp`) | source of truth for routing | +| **Local** — `/.codegraph/graph.db` | Direct `sqlite3` queries and the project `skill-apply-hook.sh` only | dev/inspection copy | + +The local DB is **not** what Claude routes against. Verifying a fix locally is +necessary but not sufficient — you must deploy for prod to change. + +## How changes reach prod + +### 1. Schema / FTS fixes → automatic on every boot +`openDb()` runs `runMigrations()` unconditionally, and the MCP server calls +`openDb()` at startup. So any new migration (e.g. `035_skills_fts_triggers.sql`, +whose one-time `rebuild` repairs a corrupted FTS index) applies on the next +redeploy with no manual step. + +### 2. Skill *content* → version-gated import on boot +`entrypoint.sh` imports the bundled catalog (`/app/data`) when **either**: +- the DB has zero skills (first boot), **or** +- the bundle's content hash differs from the last imported hash + (`/data/.codegraph/catalog.hash`). + +`/data` is a **persistent named volume**, so before the hash gate a redeploy +with an unchanged count would *skip* import entirely and new skills never +shipped. Now a changed bundle re-imports automatically. The boot import is +**additive** (idempotent, FTS-safe upsert) — it never deprecates. + +### 3. Removing skills → manual full-sync +Boot import never prunes (so dashboard-created skills aren't wiped). To make the +DB exactly mirror the filesystem bundle — deprecating skills whose files were +deleted — run inside the container: + +```bash +node dist/cli.js import-skills /data --full +``` + +`--full` soft-deprecates (status `deprecated`, reversible) any **active** skill +absent from the bundle, and **never** touches `System` / `Lifecycle` categories +or `pending` drafts. + +## Telemetry reality (usage / apply signal) + +`route()` ranking derives ~22% of its score from `skill_usage` (recency + +project affinity). That table is fed on prod **only** by MCP calls +(`skill_read` → `loaded`, `skill_apply` → `applied`, `skill_route` → `routed`), +which `recordUsage()` writes server-side. + +It is **not** fed by: +- the project hook `.claude/scripts/skill-apply-hook.sh` — it writes to the + **local** DB, which prod never reads; +- the user hook `~/.config/skillbrain/skill-tracker.sh` — it POSTs to + `SKILLBRAIN_TELEMETRY_URL` (default `http://localhost:7777`, usually nothing). + The `/telemetry/skill-usage` endpoint is localhost-only and, when + `SKILLBRAIN_TELEMETRY_TOKEN` is set, token-gated. + +So the built-in **`Skill` tool** (superpowers etc.) apply-signal does not reach +prod ranking in the default remote-MCP setup. If you want it to, point +`SKILLBRAIN_TELEMETRY_URL` at a reachable, authenticated endpoint — or prefer +the MCP `skill_apply` tool, which records server-side. + +## Routing hygiene + +`route()` excludes non-task skills from recommendations: `agent:*`, `command:*`, +`lifecycle` types, and `_routing-index`. They remain fully readable via +`skill_read` / `agent_read` / `command_read` — they're just kept out of ranked +task suggestions so specific domain/process skills surface. diff --git a/packages/codegraph/entrypoint.sh b/packages/codegraph/entrypoint.sh index 21e4dc1..24fc35d 100644 --- a/packages/codegraph/entrypoint.sh +++ b/packages/codegraph/entrypoint.sh @@ -2,13 +2,25 @@ # SkillBrain entrypoint — import skills on first boot, then start MCP HTTP server DATA_DIR="${SKILLBRAIN_ROOT:-/data}" +HASH_FILE="$DATA_DIR/.codegraph/catalog.hash" # Check if skills are already imported SKILL_COUNT=$(sqlite3 "$DATA_DIR/.codegraph/graph.db" "SELECT COUNT(*) FROM skills;" 2>/dev/null || echo "0") -if [ "$SKILL_COUNT" = "0" ] || [ "$SKILL_COUNT" = "" ]; then - echo "First boot: importing skills from bundled data..." +# Hash the bundled skill catalog. /data is a PERSISTENT volume, so a plain +# "import only when empty" gate means new/edited git-tracked skills never reach +# prod on redeploy. Comparing a content hash of /app/data against the last +# imported hash re-runs the import whenever the bundle changes. Import is an +# idempotent, FTS-safe upsert (stable rowid + migration 035 triggers), so +# re-running is cheap and additive. If sha1sum is unavailable the hash is empty +# and we fall back to the original first-boot-only behaviour. +bundle_hash() { + find /app/data -type f 2>/dev/null | LC_ALL=C sort | xargs sha1sum 2>/dev/null | sha1sum 2>/dev/null | cut -d' ' -f1 +} +BUNDLE_HASH=$(bundle_hash) +STORED_HASH=$(cat "$HASH_FILE" 2>/dev/null || echo "") +run_import() { # Create workspace structure that import-skills expects mkdir -p "$DATA_DIR/.opencode" "$DATA_DIR/.agents/skills" @@ -26,15 +38,26 @@ if [ "$SKILL_COUNT" = "0" ] || [ "$SKILL_COUNT" = "" ]; then done fi - # Run import + # Run import (additive — never prunes on boot; use `import-skills --full` manually for that) node dist/cli.js import-skills "$DATA_DIR" 2>&1 # Cleanup symlinks (data stays in SQLite) rm -rf "$DATA_DIR/.opencode" "$DATA_DIR/.agents" + # Record the imported bundle hash so unchanged redeploys skip the import. + [ -n "$BUNDLE_HASH" ] && printf '%s' "$BUNDLE_HASH" > "$HASH_FILE" +} + +if [ "$SKILL_COUNT" = "0" ] || [ "$SKILL_COUNT" = "" ]; then + echo "First boot: importing skills from bundled data..." + run_import echo "Skills import complete." +elif [ -n "$BUNDLE_HASH" ] && [ "$BUNDLE_HASH" != "$STORED_HASH" ]; then + echo "Bundled skill catalog changed ($STORED_HASH -> $BUNDLE_HASH): re-importing..." + run_import + echo "Skills re-import complete." else - echo "Skills already loaded: $SKILL_COUNT items" + echo "Skills already loaded and catalog unchanged: $SKILL_COUNT items" fi # Daily backup on boot (keeps last 30 days) diff --git a/packages/codegraph/src/cli.ts b/packages/codegraph/src/cli.ts index 139d579..68155dc 100644 --- a/packages/codegraph/src/cli.ts +++ b/packages/codegraph/src/cli.ts @@ -88,14 +88,16 @@ program .command('import-skills') .description('Import skills, agents, and commands from filesystem into SQLite') .argument('[path]', 'Path to workspace root', '.') - .action(async (targetPath: string) => { + .option('--full', 'Full-sync: also deprecate active skills no longer present in the bundle (protects System/Lifecycle)') + .action(async (targetPath: string, opts: { full?: boolean }) => { try { const { importSkills } = await import('@skillbrain/storage') - const result = importSkills(targetPath) + const result = importSkills(targetPath, { prune: !!opts.full }) console.log(`✅ Import complete:`) console.log(` Skills: ${result.skills}`) console.log(` Agents: ${result.agents}`) console.log(` Commands: ${result.commands}`) + if (opts.full) console.log(` Pruned (deprecated): ${result.pruned}`) } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err) console.error(`[codegraph] Import failed: ${msg}`) diff --git a/packages/codegraph/src/mcp/routes/skills.ts b/packages/codegraph/src/mcp/routes/skills.ts index ab84433..2fc84bd 100644 --- a/packages/codegraph/src/mcp/routes/skills.ts +++ b/packages/codegraph/src/mcp/routes/skills.ts @@ -103,7 +103,10 @@ export function createSkillsRouter(ctx: RouteContext): Router { // Health dashboard — MUST be declared before `/api/skills/:name`, // otherwise Express matches "health" as the :name param and routes to detail. - router.get('/api/skills/health', (_req, res) => { + // Admin-gated (like the sibling write routes): the report exposes internal + // analytics — dead skills, confidence trends, cooccurrence — that shouldn't be + // world-readable. If a non-admin dashboard view needs it, swap for a lighter guard. + router.get('/api/skills/health', ctx.requireAdmin, (_req, res) => { try { const db = openDb(ctx.skillbrainRoot) const store = new SkillsStore(db) @@ -172,9 +175,20 @@ export function createSkillsRouter(ctx: RouteContext): Router { } }) - // Telemetry: client-side Skill tool usage + // Telemetry: client-side Skill tool usage. + // localhost-only, and — because recording an 'applied' event calls + // reinforceSkill (confidence +1) which feeds ~22% of route() ranking weight — + // optionally token-gated so a co-tenant local process can't poison ranking. + // Enforced only when SKILLBRAIN_TELEMETRY_TOKEN is set (backward compatible). router.post('/telemetry/skill-usage', json({ limit: '8kb' }), (req, res) => { if (!ctx.isLocalhost(req)) { res.status(403).json({ error: 'localhost only' }); return } + const requiredToken = process.env.SKILLBRAIN_TELEMETRY_TOKEN + if (requiredToken) { + const auth = typeof req.headers.authorization === 'string' ? req.headers.authorization : '' + const provided = (typeof req.headers['x-telemetry-token'] === 'string' ? req.headers['x-telemetry-token'] : '') + || auth.replace(/^Bearer\s+/i, '') + if (provided !== requiredToken) { res.status(401).json({ error: 'invalid telemetry token' }); return } + } const { skill, action, sessionId, project, task, tool } = (req.body || {}) as { skill?: string; action?: string; sessionId?: string project?: string; task?: string; tool?: string diff --git a/packages/storage/src/db.ts b/packages/storage/src/db.ts index 9a3a1f1..cda56d9 100644 --- a/packages/storage/src/db.ts +++ b/packages/storage/src/db.ts @@ -36,6 +36,11 @@ export function openDb(repoPath: string): Database.Database { db.pragma('journal_mode = WAL') db.pragma('foreign_keys = ON') db.pragma('synchronous = NORMAL') + // Every MCP tool opens its own short-lived connection; the 24h decay job + // UPDATEs all active skills and can collide with concurrent recordUsage() + // writers. Without a busy_timeout that surfaces as SQLITE_BUSY with no retry — + // wait up to 5s for the lock instead. + db.pragma('busy_timeout = 5000') runMigrations(db) diff --git a/packages/storage/src/import-skills.ts b/packages/storage/src/import-skills.ts index 7f1c66c..2af68ba 100644 --- a/packages/storage/src/import-skills.ts +++ b/packages/storage/src/import-skills.ts @@ -215,7 +215,25 @@ function walkDir(dir: string, callback: (file: string, name: string) => void): v } } -export function importSkills(workspacePath: string): { skills: number; agents: number; commands: number } { +export interface ImportSkillsOptions { + /** + * Full-sync mode. After upserting the filesystem catalog, soft-deprecate any + * ACTIVE skill that is no longer present in the discovered set — treating the + * filesystem bundle as the complete source of truth. Off by default because + * the additive path is safe for incremental/boot imports; enable only when you + * want the DB to exactly mirror the files (e.g. an intentional catalog sync). + * + * Protections: never touches System/Lifecycle categories, and only flips + * `active` rows (leaves `pending` drafts and already-`deprecated` rows alone). + * Reversible: sets status='deprecated', it does not delete. + */ + prune?: boolean +} + +export function importSkills( + workspacePath: string, + opts: ImportSkillsOptions = {}, +): { skills: number; agents: number; commands: number; pruned: number } { const db = openDb(workspacePath) const store = new SkillsStore(db) @@ -348,10 +366,27 @@ export function importSkills(workspacePath: string): { skills: number; agents: n // Batch insert store.upsertBatch(skills) + // Optional full-sync: deprecate active skills that vanished from the bundle. + let pruned = 0 + if (opts.prune) { + const discovered = new Set(skills.map((s) => s.name)) + const activeNames = (db + .prepare(`SELECT name FROM skills WHERE status = 'active' AND category NOT IN ('System','Lifecycle')`) + .all() as { name: string }[]).map((r) => r.name) + const toPrune = activeNames.filter((n) => !discovered.has(n)) + if (toPrune.length > 0) { + const nowIso = new Date().toISOString() + const upd = db.prepare(`UPDATE skills SET status = 'deprecated', updated_at = ? WHERE name = ?`) + const tx = db.transaction((names: string[]) => { for (const n of names) upd.run(nowIso, n) }) + tx(toPrune) + pruned = toPrune.length + } + } + closeDb(db) const domainCount = skills.filter((s) => s.type === 'domain').length - return { skills: domainCount, agents: agentCount, commands: commandCount } + return { skills: domainCount, agents: agentCount, commands: commandCount, pruned } } function isLifecycleSkill(name: string): boolean { @@ -411,11 +446,14 @@ function extractTags(name: string, content: string): string[] { // CLI entry point if (process.argv[1]?.endsWith('import-skills.js')) { - const workspace = process.argv[2] || process.cwd() - console.log(`Importing skills from: ${workspace}`) - const result = importSkills(workspace) + const args = process.argv.slice(2) + const prune = args.includes('--full') || args.includes('--prune') + const workspace = args.find((a) => !a.startsWith('--')) || process.cwd() + console.log(`Importing skills from: ${workspace}${prune ? ' (full-sync: prune enabled)' : ''}`) + const result = importSkills(workspace, { prune }) console.log(`✅ Import complete:`) console.log(` Skills: ${result.skills}`) console.log(` Agents: ${result.agents}`) console.log(` Commands: ${result.commands}`) + if (prune) console.log(` Pruned (deprecated): ${result.pruned}`) } diff --git a/packages/storage/src/skills-store.ts b/packages/storage/src/skills-store.ts index 0337856..a31fa7e 100644 --- a/packages/storage/src/skills-store.ts +++ b/packages/storage/src/skills-store.ts @@ -89,6 +89,16 @@ export interface SkillUsageRow { // covers the realistic case (single store opened per request). const DEDUP_WINDOW_MS = 1000 +// Skills excluded from task-driven routing (route()). They are reached through +// other channels — session entrypoints (lifecycle), the Agent tool (agent:*), +// slash commands (command:*) — or are meta-documents whose broad keyword +// coverage crowds out the specific domain/process skill a task needs +// (_routing-index catalogues ~150 skills, so it lexically matches almost any +// query). All remain fully readable via skill_read / agent_read / command_read; +// this only keeps them out of the ranked task recommendations. +const ROUTING_EXCLUDED_TYPES: ReadonlySet = new Set(['agent', 'command', 'lifecycle']) +const ROUTING_EXCLUDED_NAMES: ReadonlySet = new Set(['_routing-index']) + export class SkillsStore { private static _telemetryWarned = false private static _telemetryFailures = 0 @@ -195,7 +205,7 @@ export class SkillsStore { SELECT s.name as skillName, COUNT(u.id) as count FROM skills s LEFT JOIN skill_usage u ON u.skill_name = s.name AND u.ts >= datetime('now', ?) - WHERE s.status = 'active' + WHERE s.status = 'active' AND s.category NOT IN ('System','Lifecycle') GROUP BY s.name HAVING SUM(CASE WHEN u.action = 'loaded' THEN 1 ELSE 0 END) = 0 AND SUM(CASE WHEN u.action = 'applied' THEN 1 ELSE 0 END) = 0 @@ -228,16 +238,9 @@ export class SkillsStore { sessions_since_validation = COALESCE(sessions_since_validation, 0) + 1 WHERE status = 'active' `), - applySkillDecay: this.db.prepare(` - UPDATE skills SET - confidence = MAX(COALESCE(confidence, 5) - 1, 1), - updated_at = ? - WHERE sessions_since_validation >= 10 AND COALESCE(confidence, 5) > 1 AND status = 'active' - `), - deprecateSkills: this.db.prepare(` - UPDATE skills SET status = 'deprecated', updated_at = ? - WHERE sessions_since_validation >= 30 AND COALESCE(confidence, 5) < 3 AND status = 'active' - `), + // Confidence decay + deprecation are issued as inline SQL in applyDecay() + // (with PROTECTED_CATS and the thresholds from constants.ts, 5 / 20). The + // old prepared statements here hard-coded 10 / 30 and were never referenced. // Recent load count for a skill in the last N hours (for usage boost in route()) recentLoadCount: this.db.prepare(` SELECT COUNT(*) as count FROM skill_usage @@ -375,7 +378,18 @@ export class SkillsStore { get(name: string): Skill | undefined { const row = this.stmts.get.get(name) as any - return row ? this.rowToSkill(row) : undefined + if (row) return this.rowToSkill(row) + // Fallback: route() returns prefixed names (agent:builder, command:frontend), + // but callers frequently pass the bare name. Exact match always wins first + // (a domain skill named X beats agent:X); only when there is no exact hit do + // we try the agent:/command: forms so skill_read('builder') doesn't 404. + if (!name.includes(':')) { + for (const prefix of ['agent:', 'command:']) { + const alt = this.stmts.get.get(prefix + name) as any + if (alt) return this.rowToSkill(alt) + } + } + return undefined } list(type?: SkillType, category?: string): Skill[] { @@ -409,7 +423,12 @@ export class SkillsStore { } route(taskDescription: string, limit = 5, activeSkills: string[] = [], project?: string): Skill[] { - const results = this.search(taskDescription, limit * 3) + // Over-fetch, then drop skills that shouldn't compete for task routing + // (agent/command/lifecycle types + _routing-index). Over-fetching keeps + // recall so the exclusion doesn't starve the final top-`limit`. + const results = this.search(taskDescription, limit * 5).filter( + (r) => !ROUTING_EXCLUDED_TYPES.has(r.skill.type) && !ROUTING_EXCLUDED_NAMES.has(r.skill.name), + ) if (results.length === 0) return [] const maxBm25 = Math.abs(Math.min(...results.map((r) => r.rank))) @@ -429,7 +448,11 @@ export class SkillsStore { return Math.log1p(loaded + applied * 3) } catch { return 0 } })() - const recencyBoost = recentCount / (Math.log1p(100)) + // Normalize to [0,1] like the other boosts so recency can contribute at + // most its 0.12 weight and never exceeds the 0.38 BM25 budget — otherwise a + // heavily-loaded skill (loaded+applied*3 > 100) out-ranks a perfect lexical + // match on an unrelated query. + const recencyBoost = Math.min(recentCount / Math.log1p(100), 1) const coocCount = this.getCooccurrenceCount(r.skill.name, activeSkills) const coocBoost = Math.min(coocCount / 100, 1.0) diff --git a/packages/storage/tests/import-skills.test.ts b/packages/storage/tests/import-skills.test.ts index adcc1b6..441f772 100644 --- a/packages/storage/tests/import-skills.test.ts +++ b/packages/storage/tests/import-skills.test.ts @@ -69,4 +69,44 @@ describe('importSkills()', () => { db.close() } }) + + it('--full prune deprecates skills removed from the bundle but protects System/Lifecycle', () => { + const workspace = makeWorkspace() + const writeSkill = (name: string, desc: string) => { + const dir = path.join(workspace, '.claude', 'skill', name) + fs.mkdirSync(dir, { recursive: true }) + fs.writeFileSync(path.join(dir, 'SKILL.md'), `---\nname: ${name}\ndescription: ${desc}\n---\n# ${name}\n`) + } + writeSkill('foo', 'Foo skill') + writeSkill('bar', 'Bar skill') + importSkills(workspace) + + // Seed protected infra directly in the DB (not backed by files). + const dbPath = path.join(workspace, '.codegraph', 'graph.db') + const seed = new Database(dbPath) + const now = new Date().toISOString() + const ins = seed.prepare( + `INSERT INTO skills (name, category, description, content, type, tags, lines, updated_at, status) + VALUES (?, ?, ?, ?, ?, '[]', 1, ?, 'active')`, + ) + ins.run('_routing-index', 'System', 'idx', '# idx', 'domain', now) + ins.run('using-superpowers', 'Lifecycle', 'lc', '# lc', 'lifecycle', now) + seed.close() + + // Remove bar from the bundle, then full-sync. + fs.rmSync(path.join(workspace, '.claude', 'skill', 'bar'), { recursive: true, force: true }) + const result = importSkills(workspace, { prune: true }) + expect(result.pruned).toBe(1) + + const db = new Database(dbPath) + try { + const status = (n: string) => (db.prepare('SELECT status AS s FROM skills WHERE name = ?').get(n) as { s: string } | undefined)?.s + expect(status('foo')).toBe('active') // still in bundle + expect(status('bar')).toBe('deprecated') // removed → pruned + expect(status('_routing-index')).toBe('active') // System protected + expect(status('using-superpowers')).toBe('active') // Lifecycle protected + } finally { + db.close() + } + }) }) diff --git a/packages/storage/tests/skills-store-fts-rebuild.test.ts b/packages/storage/tests/skills-store-fts-rebuild.test.ts new file mode 100644 index 0000000..c863072 --- /dev/null +++ b/packages/storage/tests/skills-store-fts-rebuild.test.ts @@ -0,0 +1,104 @@ +/* + * Synapse — The intelligence layer for AI workflows + * Copyright (c) 2026 Daniel De Vecchi + * + * Licensed under AGPL-3.0-or-later. + * See LICENSE for details. + * + * Commercial license: daniel@pixarts.eu + */ + +// Prod-critical regression: migration 035's one-time `rebuild` must REPAIR an +// already-corrupted external-content FTS index, not just keep a fresh one in +// sync. This is exactly the production scenario the fix targets — prod's index +// was orphaned by the old INSERT OR REPLACE churn, and applying 035 on redeploy +// is what heals it. The forward-trigger tests never corrupt the index first, so +// this behaviour was previously unverified. +// +// Note: skills_fts is an FTS5 EXTERNAL-CONTENT table, so orphans/missing rows +// are NOT visible via `SELECT rowid FROM skills_fts` (that reconstructs rows +// from the content table). The reliable corruption signals are the built-in +// `integrity-check` (throws on mismatch) and a `fts5: missing row` throw when +// MATCH hits a posting whose content row was deleted. + +import { readFileSync } from 'node:fs' +import Database from 'better-sqlite3' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { runMigrations } from '../src/migrator.js' +import { SkillsStore } from '../src/skills-store.js' + +const MIGRATION_035 = readFileSync( + new URL('../src/migrations/035_skills_fts_triggers.sql', import.meta.url), + 'utf-8', +) + +function makeDb(): Database.Database { + const db = new Database(':memory:') + runMigrations(db) + return db +} + +const skill = (over: Record = {}): any => ({ + name: 'x', + category: 'SEO', + description: 'desc', + content: '# body', + type: 'domain', + tags: [], + lines: 1, + updatedAt: new Date().toISOString(), + ...over, +}) + +const match = (db: Database.Database, term: string): string[] => + (db.prepare('SELECT name FROM skills_fts WHERE skills_fts MATCH ?').all(term) as { name: string }[]).map((r) => r.name) +const integrityCheck = (db: Database.Database) => + db.exec("INSERT INTO skills_fts(skills_fts) VALUES('integrity-check')") + +describe('migration 035 rebuild repairs a pre-corrupted FTS index', () => { + let db: Database.Database + beforeEach(() => { db = makeDb() }) + afterEach(() => { db.close() }) + + it('heals stale terms, orphans, and missing entries after the index is corrupted', () => { + const store = new SkillsStore(db) + store.upsert(skill({ name: 'a', content: '# alpha' })) + store.upsert(skill({ name: 'b', content: '# bravo' })) + expect(match(db, 'alpha')).toEqual(['a']) + expect(() => integrityCheck(db)).not.toThrow() + + // --- Simulate the pre-fix corruption --- + // Drop the triggers so raw writes desync the external-content index, exactly + // as the old INSERT OR REPLACE path did. + db.exec('DROP TRIGGER IF EXISTS skills_fts_ai; DROP TRIGGER IF EXISTS skills_fts_ad; DROP TRIGGER IF EXISTS skills_fts_au;') + + // (1) Stale term: content changes but the index still holds the old term. + db.prepare('UPDATE skills SET content = ? WHERE name = ?').run('# alphanew', 'a') + // (2) Orphan: skills row deleted, index posting left dangling. + db.prepare('DELETE FROM skills WHERE name = ?').run('b') + // (3) Missing: new skills row with no index entry. + db.prepare( + `INSERT INTO skills (name, category, description, content, type, tags, lines, updated_at, status) + VALUES ('c', 'SEO', 'desc', '# charlie', 'domain', '[]', 1, ?, 'active')`, + ).run(new Date().toISOString()) + + // Corruption is real: the index now disagrees with the content table. + // (We assert via lookups, which are version-independent — the no-arg + // integrity-check only verifies the index internally, not against external + // content. 'bravo' is intentionally NOT queried here: reading a deleted + // external-content row can raise `fts5: missing row`.) + expect(match(db, 'alpha')).toEqual(['a']) // STALE hit: 'a' no longer contains "alpha" + expect(match(db, 'alphanew')).toEqual([]) // new term not indexed + expect(match(db, 'charlie')).toEqual([]) // 'c' missing from index + + // --- Apply migration 035 to the corrupted DB (drop/recreate triggers + rebuild) --- + db.exec(MIGRATION_035) + + // --- Everything is repaired --- + expect(() => integrityCheck(db)).not.toThrow() + expect(match(db, 'alphanew')).toEqual(['a']) // stale term reindexed + expect(match(db, 'alpha')).toEqual([]) // old term gone + expect(match(db, 'charlie')).toEqual(['c']) // missing entry added + expect(match(db, 'bravo')).toEqual([]) // orphan dropped (row was deleted) + }) +}) diff --git a/packages/storage/tests/skills-store-routing.test.ts b/packages/storage/tests/skills-store-routing.test.ts new file mode 100644 index 0000000..e07dd6c --- /dev/null +++ b/packages/storage/tests/skills-store-routing.test.ts @@ -0,0 +1,106 @@ +/* + * Synapse — The intelligence layer for AI workflows + * Copyright (c) 2026 Daniel De Vecchi + * + * Licensed under AGPL-3.0-or-later. + * See LICENSE for details. + * + * Commercial license: daniel@pixarts.eu + */ + +// Behaviour added by the skill-system audit fixes: +// - route() excludes agent/command/lifecycle types + _routing-index +// - get() falls back to agent:/command: prefixes for a bare name +// - deadSkills() excludes protected System/Lifecycle categories + +import Database from 'better-sqlite3' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { runMigrations } from '../src/migrator.js' +import { SkillsStore, type SkillType } from '../src/skills-store.js' + +function makeDb(): Database.Database { + const db = new Database(':memory:') + runMigrations(db) + return db +} + +const skill = (over: Partial> & { name: string; type?: SkillType; category?: string; content?: string }): any => ({ + category: 'Backend', + description: 'desc', + content: '# body', + type: 'domain', + tags: [], + lines: 1, + updatedAt: new Date().toISOString(), + ...over, +}) + +describe('route(): non-task skills are excluded from recommendations', () => { + let db: Database.Database + beforeEach(() => { db = makeDb() }) + afterEach(() => { db.close() }) + + it('keeps domain/process matches, drops agent/command/lifecycle and _routing-index', () => { + const store = new SkillsStore(db) + const body = '# nextjs app router server components tailwind' + store.upsert(skill({ name: 'nextjs-domain', type: 'domain', category: 'Frontend', content: body })) + store.upsert(skill({ name: 'nextjs-process', type: 'process', category: 'Process', content: body })) + store.upsert(skill({ name: 'using-superpowers', type: 'lifecycle', category: 'Lifecycle', content: body })) + store.upsert(skill({ name: 'agent:nextjs-agent', type: 'agent', category: 'Agents', content: body })) + store.upsert(skill({ name: 'command:nextjs-cmd', type: 'command', category: 'Commands', content: body })) + store.upsert(skill({ name: '_routing-index', type: 'domain', category: 'System', content: body })) + + const names = store.route('nextjs app router server components', 10).map((s) => s.name) + + expect(names).toContain('nextjs-domain') + expect(names).toContain('nextjs-process') + expect(names).not.toContain('using-superpowers') + expect(names).not.toContain('agent:nextjs-agent') + expect(names).not.toContain('command:nextjs-cmd') + expect(names).not.toContain('_routing-index') + }) +}) + +describe('get(): agent:/command: prefix fallback', () => { + let db: Database.Database + beforeEach(() => { db = makeDb() }) + afterEach(() => { db.close() }) + + it('resolves a bare name to a prefixed entry when there is no exact match', () => { + const store = new SkillsStore(db) + store.upsert(skill({ name: 'agent:planner', type: 'agent', category: 'Agents' })) + store.upsert(skill({ name: 'command:frontend', type: 'command', category: 'Commands' })) + + expect(store.get('planner')?.name).toBe('agent:planner') + expect(store.get('frontend')?.name).toBe('command:frontend') + expect(store.get('nope')).toBeUndefined() + }) + + it('prefers an exact match over the prefixed fallback', () => { + const store = new SkillsStore(db) + store.upsert(skill({ name: 'builder', type: 'domain', category: 'Backend' })) + store.upsert(skill({ name: 'agent:builder', type: 'agent', category: 'Agents' })) + + expect(store.get('builder')?.type).toBe('domain') + expect(store.get('agent:builder')?.type).toBe('agent') + }) +}) + +describe('deadSkills(): protected categories are never flagged dead', () => { + let db: Database.Database + beforeEach(() => { db = makeDb() }) + afterEach(() => { db.close() }) + + it('excludes System and Lifecycle skills even with zero usage', () => { + const store = new SkillsStore(db) + store.upsert(skill({ name: 'lonely-domain', type: 'domain', category: 'Backend' })) + store.upsert(skill({ name: '_routing-index', type: 'domain', category: 'System' })) + store.upsert(skill({ name: 'using-superpowers', type: 'lifecycle', category: 'Lifecycle' })) + + const dead = store.deadSkills(30, 20).map((r) => r.skillName) + + expect(dead).toContain('lonely-domain') + expect(dead).not.toContain('_routing-index') + expect(dead).not.toContain('using-superpowers') + }) +})