From f012f8e880742bbebf677fe54978b517c9ebd3bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Fri, 17 Apr 2026 16:22:31 -0400 Subject: [PATCH] refactor(skills): drop company_skills.markdown column, read from disk via IPC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `company_skills.markdown` stored the full SKILL.md body. The on-disk file under `profile/skills/{companyId}/{slug}/SKILL.md` is what pi- coding-agent actually reads via `loadSkills` at runtime. The DB column was a duplicate — import wrote to both, renderer read from DB. Any discrepancy between disk and DB was invisible until the agent behaved differently from what the user saw in SkillDetail. Disk becomes the single source of truth. DB keeps name / description / metadata / source info / file inventory — the cheap lookup row. UI reads SKILL.md content live via a new IPC channel. ## Migration v37 (rebuild company_skills without markdown) Table-rebuild pattern matching v35/v36: `fn:` form, PRAGMA walk for drift columns, row-count assertion before DROP, `defer_foreign_keys` for the `agent_skill_assignments(skill_id)` reference. Indexes recreated. Idempotent via `schema_migrations` ledger. Existing DB rows' markdown content is discarded — but the on-disk SKILL.md files are the authoritative copy, so no user-visible data loss. Any agent using those skills keeps working; the UI just starts reading from disk instead of the DB cache. ## Code changes - `src/main/database/skills.ts`: drop `markdown` from read mapping, upsert input type, INSERT column/bind list, ON CONFLICT DO UPDATE. - `src/shared/types/records-core.ts`: `SkillRecord.markdown` removed. - `src/main/skills/import.ts` + `import-github.ts`: drop `markdown:` from 4 `saveCompanySkill` call sites. Disk-write via `materializeSkillFiles` unchanged (it's what the file agent reads). - `src/main/skills/index.ts`: expose `skillFilesDir` for the new IPC handler to resolve `${profile}/skills/${companyId}/${slug}/SKILL.md`. - `src/shared/contracts/channels.ts` + `schemas-execution.ts` + `desktop-api/skills.ts`: new `getSkillMarkdown` channel `{ companyId, slug } → DesktopResult`. - `src/preload/index.ts`: expose `getSkillMarkdown(companyId, slug)`. - `src/main/ipc-skill-handlers.ts`: handler validates schema, ensures company, resolves path, `readFile`, returns content. Fails closed with `SKILL_NOT_FOUND` on ENOENT, `SKILL_READ_FAILED` on other read errors. - `src/renderer/components/skills/SkillDetail.tsx`: `useEffect` fetches markdown via IPC when the skill detail loads; shows "Loading…" until resolved, surfaces read errors. Preserves `stripFrontmatter`. - Tests: drop `markdown:` from 2 fixture sites (`skill-types.test.ts`, `skill-aware-assignment.spec.ts`). ## Verification - typecheck / lint / 578 tests green - `grep 'skill\.markdown' src/` → 0 DB-record reads - `grep '\.markdown\b' src/main/database/skills.ts` → 0 - Remaining `.markdown` references are all `ghSkill.markdown` (GitHub fetcher's intermediate struct feeding `materializeSkillFiles` to disk) and parameter names in `stripFrontmatter(markdown: string)` / fs-writer helpers — none read from a DB row. --- src/main/database.ts | 2 +- src/main/database/schema.ts | 90 +++++++++++++++++++ src/main/database/skills.ts | 10 +-- src/main/ipc-skill-handlers.ts | 25 ++++++ src/main/skills/import-github.ts | 1 - src/main/skills/import.ts | 3 - src/main/skills/index.ts | 1 + src/preload/index.ts | 2 + .../components/skills/SkillDetail.tsx | 26 +++++- src/shared/contracts/channels.ts | 1 + src/shared/contracts/desktop-api/skills.ts | 1 + src/shared/contracts/schemas-execution.ts | 5 ++ src/shared/types/records-core.ts | 1 - .../skill-aware-assignment.spec.ts | 2 - tests/unit/skill-types.test.ts | 1 - 15 files changed, 155 insertions(+), 16 deletions(-) diff --git a/src/main/database.ts b/src/main/database.ts index 51a7f25a..968c599d 100644 --- a/src/main/database.ts +++ b/src/main/database.ts @@ -36,7 +36,7 @@ export class AppDatabase extends AppDatabaseBase { // ─── Skills ──────────────────────────────────────────────────────────────── - upsertCompanySkill(skill: { id: string; companyId: string; key: string; slug: string; name: string; description: string | null; markdown: string; sourceType: string; sourceLocator: string | null; sourceRef: string | null; trustLevel: string; compatibility: string; fileInventory: string; metadata: string | null; createdAt: string; updatedAt: string }) { return this.skills.upsertCompanySkill(skill); } + upsertCompanySkill(skill: { id: string; companyId: string; key: string; slug: string; name: string; description: string | null; sourceType: string; sourceLocator: string | null; sourceRef: string | null; trustLevel: string; compatibility: string; fileInventory: string; metadata: string | null; createdAt: string; updatedAt: string }) { return this.skills.upsertCompanySkill(skill); } listCompanySkills(companyId: string) { return this.skills.listCompanySkills(companyId); } getCompanySkill(companyId: string, skillId: string) { return this.skills.getCompanySkill(companyId, skillId); } deleteCompanySkill(companyId: string, skillId: string) { return this.skills.deleteCompanySkill(companyId, skillId); } diff --git a/src/main/database/schema.ts b/src/main/database/schema.ts index 51e3b26a..fe4543ef 100644 --- a/src/main/database/schema.ts +++ b/src/main/database/schema.ts @@ -739,6 +739,96 @@ export function runMigrations( ); }, }, + { + // Drop the `markdown` column from company_skills. SKILL.md on disk + // (profile/skills/{companyId}/{slug}/SKILL.md) is the authoritative + // source of truth read by pi-coding-agent's loadSkills. The DB column + // duplicated that content. UI now loads the body live via IPC. + // + // SQLite can't DROP COLUMN cleanly pre-3.35 and even the supported path + // is implemented as an invisible rebuild, so we do an explicit + // drift-safe rebuild mirroring v35/v36: + // 1. Snapshot row count + // 2. PRAGMA table_info to discover the live column set + // 3. CREATE company_skills_new without `markdown` + // 4. Copy every kept column + // 5. Assert copied count matches + // 6. DROP old, RENAME new, recreate indexes + // + // FK from agent_skill_assignments(skill_id) -> company_skills(id) must + // survive. PRAGMA defer_foreign_keys = ON keeps checks deferred until + // transaction commit (by which time RENAME has restored the table). + version: 37, name: "drop_company_skills_markdown", + fn: () => { + if (!tableExists(db, "company_skills")) { + log.info("[migration v37] company_skills table absent; skipping"); + return; + } + if (!columnExists(db, "company_skills", "markdown")) { + log.info("[migration v37] company_skills.markdown already absent; skipping"); + return; + } + + const beforeCount = (db + .prepare("SELECT COUNT(*) as cnt FROM company_skills") + .get() as { cnt: number }).cnt; + + const existingCols = (db + .prepare("PRAGMA table_info(company_skills)") + .all() as Array<{ name: string; type: string; notnull: number; dflt_value: unknown; pk: number }>); + const keepCols = existingCols.filter((c) => c.name !== "markdown"); + + db.exec("PRAGMA defer_foreign_keys = ON"); + + // Declared v14 column set (minus markdown). Drift columns preserved + // verbatim from PRAGMA metadata. + const declaredCols = new Map([ + ["id", "TEXT PRIMARY KEY"], + ["company_id", "TEXT NOT NULL"], + ["key", "TEXT NOT NULL"], + ["slug", "TEXT NOT NULL"], + ["name", "TEXT NOT NULL"], + ["description", "TEXT"], + ["source_type", "TEXT NOT NULL DEFAULT 'local_path'"], + ["source_locator", "TEXT"], + ["source_ref", "TEXT"], + ["trust_level", "TEXT NOT NULL DEFAULT 'markdown_only'"], + ["compatibility", "TEXT NOT NULL DEFAULT 'compatible'"], + ["file_inventory", "TEXT NOT NULL DEFAULT '[]'"], + ["metadata", "TEXT"], + ["created_at", "TEXT NOT NULL"], + ["updated_at", "TEXT NOT NULL"], + ]); + + const ddl = keepCols.map((c) => { + const declared = declaredCols.get(c.name); + if (declared) return `${c.name} ${declared}`; + return `${c.name} ${c.type || "TEXT"}`; + }); + + db.exec(`CREATE TABLE company_skills_new (${ddl.join(", ")})`); + + const copyCols = keepCols.map((c) => c.name).join(", "); + db.exec(`INSERT INTO company_skills_new (${copyCols}) SELECT ${copyCols} FROM company_skills`); + + const afterCount = (db + .prepare("SELECT COUNT(*) as cnt FROM company_skills_new") + .get() as { cnt: number }).cnt; + if (afterCount !== beforeCount) { + throw new Error( + `company_skills copy row count mismatch: before=${beforeCount} after=${afterCount}; aborting before DROP`, + ); + } + + db.exec("DROP TABLE company_skills"); + db.exec("ALTER TABLE company_skills_new RENAME TO company_skills"); + + db.exec("CREATE UNIQUE INDEX IF NOT EXISTS idx_company_skills_company_key ON company_skills(company_id, key)"); + db.exec("CREATE INDEX IF NOT EXISTS idx_company_skills_company_name ON company_skills(company_id, name)"); + + log.info(`[migration v37] dropped company_skills.markdown; preserved ${afterCount} row(s)`); + }, + }, ]; for (const migration of migrations) { diff --git a/src/main/database/skills.ts b/src/main/database/skills.ts index 786e9ff4..ec926c80 100644 --- a/src/main/database/skills.ts +++ b/src/main/database/skills.ts @@ -15,7 +15,6 @@ export function buildSkillMethods( slug: row.slug, name: row.name, description: row.description ?? null, - markdown: row.markdown ?? "", sourceType: (row.source_type ?? "local_path") as SkillSourceType, sourceLocator: row.source_locator ?? null, sourceRef: row.source_ref ?? null, @@ -31,7 +30,7 @@ export function buildSkillMethods( return { upsertCompanySkill(skill: { id: string; companyId: string; key: string; slug: string; name: string; - description: string | null; markdown: string; sourceType: string; + description: string | null; sourceType: string; sourceLocator: string | null; sourceRef: string | null; trustLevel: string; compatibility: string; fileInventory: string; metadata: string | null; createdAt: string; updatedAt: string; @@ -43,15 +42,14 @@ export function buildSkillMethods( getDb().prepare(` INSERT INTO company_skills ( - id, company_id, key, slug, name, description, markdown, + id, company_id, key, slug, name, description, source_type, source_locator, source_ref, trust_level, compatibility, file_inventory, metadata, created_at, updated_at - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(company_id, key) DO UPDATE SET slug = excluded.slug, name = excluded.name, description = excluded.description, - markdown = excluded.markdown, source_ref = excluded.source_ref, trust_level = excluded.trust_level, file_inventory = excluded.file_inventory, @@ -59,7 +57,7 @@ export function buildSkillMethods( updated_at = excluded.updated_at `).run( skill.id, skill.companyId, skill.key, skill.slug, - skill.name, skill.description, skill.markdown, + skill.name, skill.description, skill.sourceType, skill.sourceLocator, skill.sourceRef, skill.trustLevel, skill.compatibility, skill.fileInventory, skill.metadata, skill.createdAt, skill.updatedAt, diff --git a/src/main/ipc-skill-handlers.ts b/src/main/ipc-skill-handlers.ts index 151a0e0b..66f35afb 100644 --- a/src/main/ipc-skill-handlers.ts +++ b/src/main/ipc-skill-handlers.ts @@ -1,8 +1,11 @@ +import { join } from "node:path"; +import { readFile } from "node:fs/promises"; import { IPC_CHANNELS, importSkillSchema, listSkillsSchema, getSkillDetailSchema, + getSkillMarkdownSchema, deleteSkillSchema, assignSkillToAgentSchema, removeSkillFromAgentSchema, @@ -82,6 +85,28 @@ export function registerSkillHandlers({ return svc.getDetail(parsed.companyId, parsed.skillId); }); + // ---- Read SKILL.md body live from disk ---- + registerHandle(IPC_CHANNELS.getSkillMarkdown, async (_event, payload) => { + const parsed = getSkillMarkdownSchema.parse(payload); + const companyError = ensureCompanyExists(parsed.companyId); + if (companyError) return companyError; + const svcResult = requireSkillService(); + if (svcResult.error) return svcResult.error; + const svc = svcResult.service; + const dir = svc.skillFilesDir(parsed.companyId, parsed.slug); + const path = join(dir, "SKILL.md"); + try { + const content = await readFile(path, "utf-8"); + return ok(content); + } catch (err) { + const e = err as NodeJS.ErrnoException; + if (e.code === "ENOENT") { + return fail("SKILL_NOT_FOUND", `SKILL.md not found at ${path}`); + } + return fail("SKILL_READ_FAILED", e.message ?? String(err)); + } + }); + // ---- Delete a skill ---- registerHandle(IPC_CHANNELS.deleteSkill, async (_event, payload) => { const parsed = deleteSkillSchema.parse(payload); diff --git a/src/main/skills/import-github.ts b/src/main/skills/import-github.ts index 0049d1f9..026e3d93 100644 --- a/src/main/skills/import-github.ts +++ b/src/main/skills/import-github.ts @@ -162,7 +162,6 @@ export function createGitHubImporter({ db, logger, skillFilesDir }: GitHubImport slug: rawSlug, name, description, - markdown: ghSkill.markdown, sourceType, sourceLocator: url, sourceRef: commitSha, diff --git a/src/main/skills/import.ts b/src/main/skills/import.ts index d6036ace..2542b8f7 100644 --- a/src/main/skills/import.ts +++ b/src/main/skills/import.ts @@ -181,7 +181,6 @@ export function createSkillImporter({ db, profileDir: _profileDir, logger, skill slug: rawSlug, name: (typeof parsed.frontmatter.name === "string" ? parsed.frontmatter.name : null) ?? rawSlug, description: typeof parsed.frontmatter.description === "string" ? parsed.frontmatter.description : null, - markdown, sourceType: "local_path", sourceLocator: resolvedPath, sourceRef: contentHash, @@ -234,7 +233,6 @@ export function createSkillImporter({ db, profileDir: _profileDir, logger, skill slug: rawSlug, name: (typeof parsed.frontmatter.name === "string" ? parsed.frontmatter.name : null) ?? rawSlug, description: typeof parsed.frontmatter.description === "string" ? parsed.frontmatter.description : null, - markdown, sourceType: "local_path", sourceLocator: skillDir, sourceRef: contentHash, @@ -302,7 +300,6 @@ export function createSkillImporter({ db, profileDir: _profileDir, logger, skill slug: rawSlug, name: (typeof parsed.frontmatter.name === "string" ? parsed.frontmatter.name : null) ?? rawSlug, description: typeof parsed.frontmatter.description === "string" ? parsed.frontmatter.description : null, - markdown, sourceType: "url", sourceLocator: url, sourceRef: null, diff --git a/src/main/skills/index.ts b/src/main/skills/index.ts index 140b1afd..982d3a6f 100644 --- a/src/main/skills/index.ts +++ b/src/main/skills/index.ts @@ -58,5 +58,6 @@ export function createSkillService({ db, profileDir, logger }: SkillServiceDepen listRuntimeSkillEntries: runtime.listRuntimeSkillEntries, scanWorkspaceSkills: runtime.scanWorkspaceSkills, seedBuiltinSkills: runtime.seedBuiltinSkills, + skillFilesDir, }; } diff --git a/src/preload/index.ts b/src/preload/index.ts index 1fcfdc79..a6c5ed1c 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -86,6 +86,8 @@ const api: DesktopApi = { ipcRenderer.invoke(IPC_CHANNELS.listSkills, { companyId }), getSkillDetail: (companyId, skillId) => ipcRenderer.invoke(IPC_CHANNELS.getSkillDetail, { companyId, skillId }), + getSkillMarkdown: (companyId, slug) => + ipcRenderer.invoke(IPC_CHANNELS.getSkillMarkdown, { companyId, slug }), deleteSkill: (companyId, skillId) => ipcRenderer.invoke(IPC_CHANNELS.deleteSkill, { companyId, skillId }), assignSkillToAgent: (companyId, agentId, skillId) => diff --git a/src/renderer/components/skills/SkillDetail.tsx b/src/renderer/components/skills/SkillDetail.tsx index cce0d960..d9e9da99 100644 --- a/src/renderer/components/skills/SkillDetail.tsx +++ b/src/renderer/components/skills/SkillDetail.tsx @@ -96,6 +96,8 @@ export function SkillDetail({ const [checkingUpdate, setCheckingUpdate] = useState(false); const [confirmingDelete, setConfirmingDelete] = useState(false); const [expandedDirs, setExpandedDirs] = useState>(new Set()); + const [skillMarkdown, setSkillMarkdown] = useState(null); + const [markdownError, setMarkdownError] = useState(null); useEffect(() => { let cancelled = false; @@ -119,6 +121,24 @@ export function SkillDetail({ return () => { cancelled = true; }; }, [companyId, skillId, addToast, setSelectedSkillFilePath]); + useEffect(() => { + if (!detail) return; + let cancelled = false; + setSkillMarkdown(null); + setMarkdownError(null); + async function load(companyIdArg: string, slugArg: string) { + const result = await window.agentCompany.getSkillMarkdown(companyIdArg, slugArg); + if (cancelled) return; + if (result.ok) { + setSkillMarkdown(result.data); + } else { + setMarkdownError(result.error.message); + } + } + void load(detail.companyId, detail.slug); + return () => { cancelled = true; }; + }, [detail]); + const handleCheckUpdate = useCallback(async () => { if (!detail) return; setCheckingUpdate(true); @@ -173,7 +193,11 @@ export function SkillDetail({ // Determine content for the file viewer const isSkillMd = selectedSkillFilePath === "SKILL.md"; const fileContent = isSkillMd - ? stripFrontmatter(detail.markdown ?? "") + ? (markdownError + ? markdownError + : skillMarkdown === null + ? t("skill.loading") + : stripFrontmatter(skillMarkdown)) : null; const selectedFileEntry = !isSkillMd diff --git a/src/shared/contracts/channels.ts b/src/shared/contracts/channels.ts index 9e63ab43..63b1ea55 100644 --- a/src/shared/contracts/channels.ts +++ b/src/shared/contracts/channels.ts @@ -75,6 +75,7 @@ export const IPC_CHANNELS = { importSkill: "agent-company:import-skill", listSkills: "agent-company:list-skills", getSkillDetail: "agent-company:get-skill-detail", + getSkillMarkdown: "agent-company:get-skill-markdown", deleteSkill: "agent-company:delete-skill", assignSkillToAgent: "agent-company:assign-skill-to-agent", removeSkillFromAgent: "agent-company:remove-skill-from-agent", diff --git a/src/shared/contracts/desktop-api/skills.ts b/src/shared/contracts/desktop-api/skills.ts index eb60f736..38fc75e9 100644 --- a/src/shared/contracts/desktop-api/skills.ts +++ b/src/shared/contracts/desktop-api/skills.ts @@ -5,6 +5,7 @@ export type SkillsApi = { importSkill: (companyId: string, source: string) => Promise>; listSkills: (companyId: string) => Promise>; getSkillDetail: (companyId: string, skillId: string) => Promise>; + getSkillMarkdown: (companyId: string, slug: string) => Promise>; deleteSkill: (companyId: string, skillId: string) => Promise>; assignSkillToAgent: (companyId: string, agentId: string, skillId: string) => Promise>; removeSkillFromAgent: (companyId: string, agentId: string, skillId: string) => Promise>; diff --git a/src/shared/contracts/schemas-execution.ts b/src/shared/contracts/schemas-execution.ts index 357393d0..b6ade778 100644 --- a/src/shared/contracts/schemas-execution.ts +++ b/src/shared/contracts/schemas-execution.ts @@ -70,6 +70,11 @@ export const getSkillDetailSchema = z.object({ skillId: z.string().min(1), }); +export const getSkillMarkdownSchema = z.object({ + companyId: z.string().min(1), + slug: z.string().min(1), +}); + export const deleteSkillSchema = z.object({ companyId: z.string().min(1), skillId: z.string().min(1), diff --git a/src/shared/types/records-core.ts b/src/shared/types/records-core.ts index 47267eac..514da28d 100644 --- a/src/shared/types/records-core.ts +++ b/src/shared/types/records-core.ts @@ -327,7 +327,6 @@ export interface SkillRecord { slug: string; name: string; description: string | null; - markdown: string; sourceType: SkillSourceType; sourceLocator: string | null; sourceRef: string | null; diff --git a/tests/integration/skill-aware-assignment.spec.ts b/tests/integration/skill-aware-assignment.spec.ts index 3332ea0e..16cbe4d8 100644 --- a/tests/integration/skill-aware-assignment.spec.ts +++ b/tests/integration/skill-aware-assignment.spec.ts @@ -82,7 +82,6 @@ describe("skill-aware capability registry", () => { slug: "react-ui", name: "React UI", description: "Build React components with hooks, Tailwind, and accessible layouts", - markdown: "# React UI\nUse when building React components with hooks and Tailwind.", sourceType: "local_path", sourceLocator: null, sourceRef: null, @@ -159,7 +158,6 @@ describe("skill-aware capability registry", () => { slug: "react-ui", name: "React UI", description: "Use when building React interfaces", - markdown: "# React UI\nUse when building React interfaces.", sourceType: "local_path", sourceLocator: null, sourceRef: null, diff --git a/tests/unit/skill-types.test.ts b/tests/unit/skill-types.test.ts index f80cf1cd..93167f5d 100644 --- a/tests/unit/skill-types.test.ts +++ b/tests/unit/skill-types.test.ts @@ -58,7 +58,6 @@ describe("skill-types", () => { slug: "slug", name: "Test Skill", description: null, - markdown: "# Test", sourceType: "github", sourceLocator: "https://github.com/owner/repo", sourceRef: "abc123",