diff --git a/CHANGELOG.md b/CHANGELOG.md index 8794e0c..a80ffb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ All notable changes to this project are documented in this file. - Direct `@modelcontextprotocol/sdk` (root and `packages/efficiency-agent`) moved from `^1.30.0` to `^1.32.1`. The advisory range was `<1.31.0` (OAuth client could send credentials to an authorization server chosen by the MCP server). - `overrides.proxy-addr` is `^2.0.8`. express still declares `^2.0.7`, which resolved to the vulnerable `2.0.7` (IPv4-mapped IPv6 trust spoofing). +### Fixed — skill markdown graph clients stay open (Windows sqlite EBUSY) + +- `importSkillsFromMarkdownRuntime`, `exportSkillsToMarkdownRuntime`, and `extractDialogueKnowledgeRuntime` now `close()` the graph client in `finally`. Default `auto` transport opens better-sqlite3; leaving that handle open makes a later `rmSync` of `graphflow-graph.sqlite` fail with `EBUSY` on Windows. + ## [Unreleased] ### Fixed — 第 0 步性能与健壮性(迁移 Rust 前的架构债清偿,复审登记项全闭环) diff --git a/src/surfaces/cli/runtime/knowledge.ts b/src/surfaces/cli/runtime/knowledge.ts index fc7875d..6542ac0 100644 --- a/src/surfaces/cli/runtime/knowledge.ts +++ b/src/surfaces/cli/runtime/knowledge.ts @@ -35,6 +35,17 @@ function resolveRuntimeConfig( ); } +async function closeGraphClient(client: GraphClient): Promise { + // sqlite holds a Windows file lock until close(). Import/export/extract + // used to return with the handle open, so test cleanup (and any later + // unlink of graphflow-graph.sqlite) failed with EBUSY on win32. + try { + await client.close?.(); + } catch { + /* ignore close races */ + } +} + export interface SkillMarkdownExportResult { outputDir: string; fileCount: number; @@ -52,59 +63,63 @@ export async function exportSkillsToMarkdownRuntime( ): Promise { const config = resolveRuntimeConfig(configPath, options?.rootDir); const client = createGraphClient(config); - const snapshot = client.readSnapshot?.() ?? { nodes: [], edges: [] }; - const workspaceRoot = config.graphPolicy.workspaceRoot ?? process.cwd(); - const outputDir = options?.outputDir - ? (isAbsolute(options.outputDir) - ? options.outputDir - : join(workspaceRoot, options.outputDir)) - : join(workspaceRoot, ".graphflow", "skills", "markdown"); - mkdirSync(outputDir, { recursive: true }); + try { + const snapshot = client.readSnapshot?.() ?? { nodes: [], edges: [] }; + const workspaceRoot = config.graphPolicy.workspaceRoot ?? process.cwd(); + const outputDir = options?.outputDir + ? (isAbsolute(options.outputDir) + ? options.outputDir + : join(workspaceRoot, options.outputDir)) + : join(workspaceRoot, ".graphflow", "skills", "markdown"); + mkdirSync(outputDir, { recursive: true }); - let bytes = 0; - let fileCount = 0; - let skippedComposites = 0; - let referenceFileCount = 0; - const invalid: Array<{ file: string; violations: string[] }> = []; - const usedDirs = new Set(); - for (const node of snapshot.nodes) { - if (node.type !== "Skill") continue; - const state = parseSkillState(node.content); - if (!state) { - if (node.content.includes('"kind":"composite"')) skippedComposites += 1; - continue; - } - // agentskills.io layout: one directory per skill, SKILL.md inside, and - // the directory name MUST equal the spec name. Oversized guidance moves - // to references/ (progressive disclosure) so the body stays a pointer. - let dirName = skillDirectoryFor(state); - const base = toSpecName(state.name); - let suffix = 2; - while (usedDirs.has(dirName.toLowerCase())) { - dirName = `${base}-${suffix}`; - suffix += 1; - } - usedDirs.add(dirName.toLowerCase()); - const bundle = skillToSkillMarkdownBundle(state); - // skills-ref gate: validate the whole bundle — dangling pointers to - // references/ files and orphan reference files both count as invalid. - const violations = validateSkillBundle(bundle); - const relPath = `${dirName}/SKILL.md`; - if (violations.length > 0) invalid.push({ file: relPath, violations }); - const skillDir = join(outputDir, dirName); - mkdirSync(skillDir, { recursive: true }); - writeFileSync(join(skillDir, "SKILL.md"), bundle.markdown, "utf8"); - bytes += Buffer.byteLength(bundle.markdown); - fileCount += 1; - for (const reference of bundle.references) { - const refPath = join(skillDir, ...reference.path.split("/")); - mkdirSync(dirname(refPath), { recursive: true }); - writeFileSync(refPath, reference.content, "utf8"); - bytes += Buffer.byteLength(reference.content); - referenceFileCount += 1; + let bytes = 0; + let fileCount = 0; + let skippedComposites = 0; + let referenceFileCount = 0; + const invalid: Array<{ file: string; violations: string[] }> = []; + const usedDirs = new Set(); + for (const node of snapshot.nodes) { + if (node.type !== "Skill") continue; + const state = parseSkillState(node.content); + if (!state) { + if (node.content.includes('"kind":"composite"')) skippedComposites += 1; + continue; + } + // agentskills.io layout: one directory per skill, SKILL.md inside, and + // the directory name MUST equal the spec name. Oversized guidance moves + // to references/ (progressive disclosure) so the body stays a pointer. + let dirName = skillDirectoryFor(state); + const base = toSpecName(state.name); + let suffix = 2; + while (usedDirs.has(dirName.toLowerCase())) { + dirName = `${base}-${suffix}`; + suffix += 1; + } + usedDirs.add(dirName.toLowerCase()); + const bundle = skillToSkillMarkdownBundle(state); + // skills-ref gate: validate the whole bundle — dangling pointers to + // references/ files and orphan reference files both count as invalid. + const violations = validateSkillBundle(bundle); + const relPath = `${dirName}/SKILL.md`; + if (violations.length > 0) invalid.push({ file: relPath, violations }); + const skillDir = join(outputDir, dirName); + mkdirSync(skillDir, { recursive: true }); + writeFileSync(join(skillDir, "SKILL.md"), bundle.markdown, "utf8"); + bytes += Buffer.byteLength(bundle.markdown); + fileCount += 1; + for (const reference of bundle.references) { + const refPath = join(skillDir, ...reference.path.split("/")); + mkdirSync(dirname(refPath), { recursive: true }); + writeFileSync(refPath, reference.content, "utf8"); + bytes += Buffer.byteLength(reference.content); + referenceFileCount += 1; + } } + return { outputDir, fileCount, bytes, skippedComposites, referenceFileCount, invalid }; + } finally { + await closeGraphClient(client); } - return { outputDir, fileCount, bytes, skippedComposites, referenceFileCount, invalid }; } export interface SkillMarkdownImportResult { @@ -174,58 +189,62 @@ export async function importSkillsFromMarkdownRuntime( const files = collectMarkdownFiles(inputPath); const client = createGraphClient(config); - let imported = 0; - let updated = 0; - let skipped = 0; - const invalid: Array<{ file: string; violations: string[] }> = []; - for (const file of files) { - const raw = readFileSync(file, "utf8"); - // Spec gate: reject files that violate agentskills.io shape before parsing. - // Missing description is advisory (importable); bad name/shape is rejected. - const violations = validateSkillMarkdown(raw).filter( - (v) => !v.startsWith("description is required") - ); - if (violations.length > 0) { - invalid.push({ file, violations }); - skipped += 1; - continue; - } - const state = parseSkillMarkdown(raw); - if (!state) { - skipped += 1; - continue; - } - // agentskills.io: the parent directory name must equal the skill name. - // Only enforced for spec-layout files; flat legacy exports (name.md at - // the scan root) stay importable. - if (isSpecLayoutFile(file)) { - const parentDir = basename(dirname(file)); - const specName = toSpecName(state.name); - if (parentDir.toLowerCase() !== specName.toLowerCase()) { - invalid.push({ - file, - violations: [ - `directory name "${parentDir}" must equal the skill name "${specName}" (agentskills.io)`, - ], - }); + try { + let imported = 0; + let updated = 0; + let skipped = 0; + const invalid: Array<{ file: string; violations: string[] }> = []; + for (const file of files) { + const raw = readFileSync(file, "utf8"); + // Spec gate: reject files that violate agentskills.io shape before parsing. + // Missing description is advisory (importable); bad name/shape is rejected. + const violations = validateSkillMarkdown(raw).filter( + (v) => !v.startsWith("description is required") + ); + if (violations.length > 0) { + invalid.push({ file, violations }); skipped += 1; continue; } + const state = parseSkillMarkdown(raw); + if (!state) { + skipped += 1; + continue; + } + // agentskills.io: the parent directory name must equal the skill name. + // Only enforced for spec-layout files; flat legacy exports (name.md at + // the scan root) stay importable. + if (isSpecLayoutFile(file)) { + const parentDir = basename(dirname(file)); + const specName = toSpecName(state.name); + if (parentDir.toLowerCase() !== specName.toLowerCase()) { + invalid.push({ + file, + violations: [ + `directory name "${parentDir}" must equal the skill name "${specName}" (agentskills.io)`, + ], + }); + skipped += 1; + continue; + } + } + const previousUpdatedAt = await existingSkillUpdatedAt(client, state.id); + if ( + previousUpdatedAt !== undefined && + !options?.force && + previousUpdatedAt >= state.updatedAt + ) { + skipped += 1; + continue; + } + await client.upsertNodes([{ id: state.id, type: "Skill", content: serializeAtomic(state) }]); + if (previousUpdatedAt === undefined) imported += 1; + else updated += 1; } - const previousUpdatedAt = await existingSkillUpdatedAt(client, state.id); - if ( - previousUpdatedAt !== undefined && - !options?.force && - previousUpdatedAt >= state.updatedAt - ) { - skipped += 1; - continue; - } - await client.upsertNodes([{ id: state.id, type: "Skill", content: serializeAtomic(state) }]); - if (previousUpdatedAt === undefined) imported += 1; - else updated += 1; + return { inputPath, imported, updated, skipped, total: files.length, invalid }; + } finally { + await closeGraphClient(client); } - return { inputPath, imported, updated, skipped, total: files.length, invalid }; } export interface DialogueKnowledgeExtractionResult { @@ -254,39 +273,43 @@ export async function extractDialogueKnowledgeRuntime( ? options.sessionId : dialogueSessionIdFor(options.sessionId, workspaceRoot); const client = createGraphClient(config); - const turns = await listDialogueTurns(client, { - ...(sessionId ? { sessionId } : {}), - ...(options?.limit !== undefined ? { limit: options.limit } : {}), - }); - const records: KnowledgeTurnRecord[] = turns.map((turn) => ({ - turnId: turn.id, - query: turn.userQuery, - reply: turn.assistantReply, - })); - const fragment = extractEngineeringKnowledgeGraphFragment({ turns: records }); + try { + const turns = await listDialogueTurns(client, { + ...(sessionId ? { sessionId } : {}), + ...(options?.limit !== undefined ? { limit: options.limit } : {}), + }); + const records: KnowledgeTurnRecord[] = turns.map((turn) => ({ + turnId: turn.id, + query: turn.userQuery, + reply: turn.assistantReply, + })); + const fragment = extractEngineeringKnowledgeGraphFragment({ turns: records }); - // The extractor records source turn IDs in metadata. Emit one provenance - // edge per actual dialogue-turn node so Concept/Requirement remain auditable. - const edges: GraphEdge[] = []; - for (const node of fragment.nodes) { - const metadata = node.metadata as { - sourceTurnIds?: string[]; - }; - for (const sourceId of metadata.sourceTurnIds ?? []) { - edges.push({ from: node.id, to: sourceId, relation: "derived_from" }); + // The extractor records source turn IDs in metadata. Emit one provenance + // edge per actual dialogue-turn node so Concept/Requirement remain auditable. + const edges: GraphEdge[] = []; + for (const node of fragment.nodes) { + const metadata = node.metadata as { + sourceTurnIds?: string[]; + }; + for (const sourceId of metadata.sourceTurnIds ?? []) { + edges.push({ from: node.id, to: sourceId, relation: "derived_from" }); + } } - } - const apply = options?.apply ?? true; - if (apply) { - if (fragment.nodes.length > 0) await client.upsertNodes(fragment.nodes); - if (edges.length > 0) await client.upsertEdges(edges); + const apply = options?.apply ?? true; + if (apply) { + if (fragment.nodes.length > 0) await client.upsertNodes(fragment.nodes); + if (edges.length > 0) await client.upsertEdges(edges); + } + return { + scannedTurns: turns.length, + requirements: fragment.nodes.filter((node) => node.type === "Requirement").length, + concepts: fragment.nodes.filter((node) => node.type === "Concept").length, + edges: edges.length, + applied: apply, + }; + } finally { + await closeGraphClient(client); } - return { - scannedTurns: turns.length, - requirements: fragment.nodes.filter((node) => node.type === "Requirement").length, - concepts: fragment.nodes.filter((node) => node.type === "Concept").length, - edges: edges.length, - applied: apply, - }; } diff --git a/tests/skill-markdown-client-close.test.ts b/tests/skill-markdown-client-close.test.ts new file mode 100644 index 0000000..31495f3 --- /dev/null +++ b/tests/skill-markdown-client-close.test.ts @@ -0,0 +1,108 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it, vi } from "vitest"; + +import * as factory from "../src/graph/client-factory"; +import { skillToSkillMarkdown } from "../src/learning/skill-markdown"; +import type { SkillState } from "../src/learning/skill-types"; +import { + exportSkillsToMarkdownRuntime, + extractDialogueKnowledgeRuntime, + importSkillsFromMarkdownRuntime, +} from "../src/surfaces/cli/runtime/knowledge"; + +const skill = (guidance: string): SkillState => + ({ + id: "s1", + name: "Prefer Targeted Reads", + score: 1, + uses: 2, + lastOutcome: "pass", + updatedAt: 100, + outcomeKind: "proven", + guidance, + }) as SkillState; + +const dirs: string[] = []; + +afterEach(() => { + vi.restoreAllMocks(); + while (dirs.length > 0) { + const dir = dirs.pop(); + if (!dir) continue; + try { + rmSync(dir, { + recursive: true, + force: true, + ...(process.platform === "win32" ? { maxRetries: 10, retryDelay: 100 } : {}), + }); + } catch { + /* best-effort cleanup */ + } + } +}); + +function newWorkspace(): string { + const dir = mkdtempSync(join(tmpdir(), "gf-skill-close-")); + dirs.push(dir); + return dir; +} + +/** + * Wrap every client this process creates and record whether `close()` ran. + * Default transport is sqlite when better-sqlite3 is present; either way the + * runtime must release the handle before returning, or Windows `rmSync` of + * graphflow-graph.sqlite fails with EBUSY. + */ +function trackCloses(options?: { failUpsert?: boolean }): { closed: boolean }[] { + const records: { closed: boolean }[] = []; + const original = factory.createGraphClient; + vi.spyOn(factory, "createGraphClient").mockImplementation((config) => { + const client = original(config); + const innerClose = client.close?.bind(client); + const record = { closed: false }; + client.close = () => { + record.closed = true; + return innerClose?.(); + }; + if (options?.failUpsert) { + client.upsertNodes = () => Promise.reject(new Error("disk full")); + } + records.push(record); + return client; + }); + return records; +} + +describe("skill markdown runtimes close the graph client", () => { + it("closes after export, import, and dialogue-knowledge extract", async () => { + const records = trackCloses(); + const ws = newWorkspace(); + const inDir = join(ws, "in"); + mkdirSync(inDir, { recursive: true }); + writeFileSync(join(inDir, "prefer-targeted-reads.md"), skillToSkillMarkdown(skill("- a"))); + + await exportSkillsToMarkdownRuntime(undefined, { rootDir: ws, outputDir: join(ws, "out") }); + await importSkillsFromMarkdownRuntime(undefined, { rootDir: ws, inputPath: inDir }); + await extractDialogueKnowledgeRuntime(undefined, { rootDir: ws, apply: false }); + + expect(records.length).toBeGreaterThanOrEqual(3); + expect(records.every((record) => record.closed)).toBe(true); + }); + + it("closes when a write fails after the client is open", async () => { + const records = trackCloses({ failUpsert: true }); + const ws = newWorkspace(); + const inDir = join(ws, "in"); + mkdirSync(inDir, { recursive: true }); + writeFileSync(join(inDir, "prefer-targeted-reads.md"), skillToSkillMarkdown(skill("- a"))); + + await expect( + importSkillsFromMarkdownRuntime(undefined, { rootDir: ws, inputPath: inDir }) + ).rejects.toThrow(/disk full/); + expect(records).toHaveLength(1); + expect(records[0]?.closed).toBe(true); + }); +});